Merged in task/dspace-cris-2025_02_x/DSC-3043_new (pull request DSpace#5284)

[DSC-3043] Async-load full report to avoid stale chart data

Approved-by: Francesco Molinaro

Author: atarix83

src/app/statistics-page/cris-statistics-page/cris-statistics-page.component.html

@@ -68,7 +68,7 @@ <h2>{{'statistics.categories.title' | translate}}</h2>
                     [category]="selectedCategory"
                     [reports]="reports"
                     [selectedReportId]="selectedReportId"
-                    [selectedFullReport]="(selectedFullReport$ | async)">
+                    [selectedFullReport]="selectedFullReport$">
                   </ds-statistics-chart>
                 }
               </div>

src/app/statistics-page/cris-statistics-page/cris-statistics-page.component.spec.ts

@@ -226,71 +226,169 @@ describe('CrisStatisticsPageComponent', () => {
     expect(component.selectedReportId).toBeUndefined();
   });
 
-  it('should set selectedReportId to the first report id if no reportType query param is present', () => {
+  it('should emit null on selectedFullReport$ before loading the new report', () => {
     const category = { id: 'category1' };
-    const reports = [{ id: 'report1', reportType: 'type1' }, { id: 'report2', reportType: 'type2' }];
+    const reports = [{ id: 'report1', reportType: 'type1' }];
     spyOn(component, 'getReports$').and.returnValue(of(reports as UsageReport[]));
     spyOn(component, 'getReportId').and.returnValue(of(null));
     spyOn(component, 'getCategoryId').and.returnValue(of(null));
     spyOn(component, 'setStatisticsState');
 
+    const emitted: (UsageReport | null)[] = [];
+    (component as any).selectedFullReport$.subscribe((value) => emitted.push(value));
+
     component.getUserReports(category);
 
-    expect(component.setStatisticsState).toHaveBeenCalledWith('report1', 'category1');
-    expect(component.selectedReportId).toBe('report1');
+    // first emitted value is the initial null from the BehaviorSubject,
+    // second is the reset to null done at the start of getUserReports
+    expect(emitted[0]).toBeNull();
+    expect(emitted[1]).toBeNull();
   });
 
-  it('should set selectedReportId to the report id matching the reportType query param', () => {
-    const category = { id: 'category1' };
-    const reports = [{ id: 'report1', reportType: 'type1' }, { id: 'report2', reportType: 'type2' }];
-    spyOn(component, 'getReports$').and.returnValue(of(reports as UsageReport[]));
-    spyOn(component, 'getReportId').and.returnValue(of(null));
-    spyOn(component, 'getCategoryId').and.returnValue(of(null));
-    spyOn(component, 'setStatisticsState');
+  describe('loadFullReport', () => {
 
-    component.getUserReports(category, 'type2');
+    beforeEach(() => {
+      (component as any).fullReportCache.clear();
+      (component as any).inFlightRequests.clear();
+    });
+
+    it('should return of(null) and reset currentReportId when reportId is falsy', (done: DoneFn) => {
+      (component as any).currentReportId = 'something';
+
+      component.loadFullReport(null).subscribe((result) => {
+        expect(result).toBeNull();
+        expect((component as any).currentReportId).toBeNull();
+        done();
+      });
+    });
+
+    it('should return the cached report without calling the service when present in cache', (done: DoneFn) => {
+      const reportId = '1911e8a4-6939-490c-b58b-a5d70f8d91fb_TotalVisits';
+      const cachedReport = { id: reportId } as UsageReport;
+      (component as any).fullReportCache.set(reportId, cachedReport);
+      spyOn(usageReportServiceStub, 'findById').and.callThrough();
+
+      component.loadFullReport(reportId).subscribe((result) => {
+        expect(result).toBe(cachedReport);
+        expect(usageReportServiceStub.findById).not.toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('should call usageReportService.findById when the report is not cached', (done: DoneFn) => {
+      const reportId = '1911e8a4-6939-490c-b58b-a5d70f8d91fb_TotalVisits';
+      spyOn(usageReportServiceStub, 'findById').and.callThrough();
+
+      component.loadFullReport(reportId).subscribe((result) => {
+        expect(usageReportServiceStub.findById).toHaveBeenCalledWith(reportId, false, true);
+        expect(result.id).toBe(reportId);
+        expect((component as any).currentReportId).toBe(reportId);
+        done();
+      });
+    });
+
+    it('should reuse the same observable for concurrent requests of the same reportId', () => {
+      const reportId = '1911e8a4-6939-490c-b58b-a5d70f8d91fb_TotalVisits';
+      spyOn(usageReportServiceStub, 'findById').and.callThrough();
+
+      const first$ = component.loadFullReport(reportId);
+      const second$ = component.loadFullReport(reportId);
+
+      expect(first$).toBe(second$);
+      expect(usageReportServiceStub.findById).toHaveBeenCalledTimes(1);
+    });
 
-    expect(component.setStatisticsState).toHaveBeenCalledWith('report2', 'category1');
-    expect(component.selectedReportId).toBe('report2');
   });
 
-  it('should set selectedReportId to the first report id if reportType query param does not match any report', () => {
-    const category = { id: 'category1' };
-    const reports = [{ id: 'report1', reportType: 'type1' }, { id: 'report2', reportType: 'type2' }];
-    spyOn(component, 'getReports$').and.returnValue(of(reports as UsageReport[]));
-    spyOn(component, 'getReportId').and.returnValue(of(null));
-    spyOn(component, 'getCategoryId').and.returnValue(of(null));
-    spyOn(component, 'setStatisticsState');
+  describe('setReportId', () => {
 
-    component.getUserReports(category, 'non_existing_type');
+    beforeEach(() => {
+      (component as any).fullReportCache.clear();
+    });
+
+    it('should cache the report and update selectedFullReport$ when reportId matches currentReportId', () => {
+      const reportId = 'report1';
+      const report = { id: reportId } as UsageReport;
+      (component as any).currentReportId = reportId;
+
+      (component as any).setReportId(report, reportId);
+
+      expect((component as any).fullReportCache.get(reportId)).toBe(report);
+      expect((component as any).selectedFullReport$.value).toBe(report);
+    });
+
+    it('should not update selectedFullReport$ when reportId does not match currentReportId (stale response)', () => {
+      const report = { id: 'oldReport' } as UsageReport;
+      (component as any).currentReportId = 'newerReport';
+      (component as any).selectedFullReport$.next(null);
+
+      (component as any).setReportId(report, 'oldReport');
+
+      expect((component as any).selectedFullReport$.value).toBeNull();
+      // it is still cached for later reuse even if stale
+      expect((component as any).fullReportCache.get('oldReport')).toBe(report);
+    });
+
+    it('should not cache a null/falsy report', () => {
+      (component as any).currentReportId = 'report1';
+
+      (component as any).setReportId(null, 'report1');
+
+      expect((component as any).fullReportCache.has('report1')).toBe(false);
+      expect((component as any).selectedFullReport$.value).toBeNull();
+    });
 
-    expect(component.setStatisticsState).toHaveBeenCalledWith('report1', 'category1');
-    expect(component.selectedReportId).toBe('report1');
   });
 
-  it('should set selectedReportId and categoryId from state if they exist', () => {
-    const category = { id: 'category1' };
-    const reports = [{ id: 'report1', reportType: 'type1' }, { id: 'report2', reportType: 'type2' }];
-    spyOn(component, 'getReports$').and.returnValue(of(reports as UsageReport[]));
-    spyOn(component, 'getReportId').and.returnValue(of('report1'));
-    spyOn(component, 'getCategoryId').and.returnValue(of('category1'));
-    spyOn(component, 'setStatisticsState');
+  describe('changeReport', () => {
 
-    component.getUserReports(category);
+    it('should update statistics state, selectedReportId and selectedFullReport$', (done: DoneFn) => {
+      const reportId = '1911e8a4-6939-490c-b58b-a5d70f8d91fb_TotalVisits';
+      spyOn(component, 'getCategoryId').and.returnValue(of('category1'));
+      spyOn(component, 'setStatisticsState');
+
+      component.changeReport(reportId);
+
+      (component as any).selectedFullReport$.subscribe((report: UsageReport) => {
+        if (report) {
+          expect(component.setStatisticsState).toHaveBeenCalledWith(reportId, 'category1');
+          expect(component.selectedReportId).toBe(reportId);
+          expect(report.id).toBe(reportId);
+          done();
+        }
+      });
+    });
+
+    it('should emit null on selectedFullReport$ before loading the new report', () => {
+      spyOn(component, 'getCategoryId').and.returnValue(of('category1'));
+      spyOn(component, 'setStatisticsState');
+
+      const emitted: (UsageReport | null)[] = [];
+      (component as any).selectedFullReport$.subscribe((value) => emitted.push(value));
+
+      component.changeReport('1911e8a4-6939-490c-b58b-a5d70f8d91fb_TotalVisits');
+
+      expect(emitted[0]).toBeNull();
+      expect(emitted[1]).toBeNull();
+    });
 
-    expect(component.setStatisticsState).toHaveBeenCalledWith('report1', 'category1');
   });
 
-  it('should handle null category gracefully', () => {
-    spyOn(component, 'getReports$').and.returnValue(of([]));
-    spyOn(component, 'getReportId').and.returnValue(of(null));
-    spyOn(component, 'getCategoryId').and.returnValue(of(null));
-    spyOn(component, 'setStatisticsState');
+  describe('clearFullReportCache', () => {
 
-    component.getUserReports(null);
+    it('should clear caches, reset currentReportId and emit null on selectedFullReport$', () => {
+      (component as any).fullReportCache.set('r1', { id: 'r1' } as UsageReport);
+      (component as any).inFlightRequests.set('r1', of({ id: 'r1' } as UsageReport));
+      (component as any).currentReportId = 'r1';
+
+      (component as any).clearFullReportCache();
+
+      expect((component as any).fullReportCache.size).toBe(0);
+      expect((component as any).inFlightRequests.size).toBe(0);
+      expect((component as any).currentReportId).toBeNull();
+      expect((component as any).selectedFullReport$.value).toBeNull();
+    });
 
-    expect(component.setStatisticsState).not.toHaveBeenCalled();
-    expect(component.selectedReportId).toBeUndefined();
   });
 
 });

src/app/statistics-page/cris-statistics-page/cris-statistics-page.component.ts

@@ -46,6 +46,7 @@ import { SiteDataService } from '../../core/data/site-data.service';
 import { redirectOn4xx } from '../../core/shared/authorized.operators';
 import { DSpaceObject } from '../../core/shared/dspace-object.model';
 import {
+  getFirstCompletedRemoteData,
   getFirstSucceededRemoteData,
   getRemoteDataPayload,
 } from '../../core/shared/operators';
@@ -143,8 +144,7 @@ export class CrisStatisticsPageComponent implements OnInit, OnDestroy {
   /**
    * The fully loaded report (with points) for the currently selected chart report.
    */
-  private selectedFullReportSubject = new BehaviorSubject<UsageReport | null>(null);
-  selectedFullReport$ = this.selectedFullReportSubject.asObservable();
+  protected selectedFullReport$ = new BehaviorSubject<UsageReport | null>(null);
 
   constructor(
     protected route: ActivatedRoute,
@@ -249,14 +249,13 @@ export class CrisStatisticsPageComponent implements OnInit, OnDestroy {
    * @param reportType
    */
   getUserReports(category, reportType = this.route?.snapshot?.queryParams?.reportType) {
-    this.reports$ =
-      of(category)
-        .pipe(
-          switchMap(c => c == null ? [] : this.getReports$(c.id)),
-        );
+    this.selectedFullReport$.next(null);
+    this.reports$ = of(category).pipe(
+      switchMap(c => c == null ? [] : this.getReports$(c.id)),
+    );
     combineLatest([
       this.reports$, this.getReportId(), this.getCategoryId(),
-    ]).subscribe(([report, reportId, categoryId]) => {
+    ]).pipe(switchMap(([report, reportId, categoryId]) => {
       if (!reportId && !categoryId) {
         let reportToShowId = report[0].id;
         if (reportType) {
@@ -270,7 +269,11 @@ export class CrisStatisticsPageComponent implements OnInit, OnDestroy {
       } else {
         this.setStatisticsState(reportId, categoryId);
       }
-      this.loadFullReport(this.selectedReportId || reportId || report[0]?.id);
+      const availableReportId = this.selectedReportId || reportId || report[0]?.id;
+      return combineLatest([this.loadFullReport(availableReportId), of(availableReportId)]);
+    }),
+    ).subscribe(([fullReport, reportId]: [UsageReport, string]) => {
+      this.setReportId(fullReport, reportId);
     });
   }
 
@@ -339,58 +342,52 @@ export class CrisStatisticsPageComponent implements OnInit, OnDestroy {
    * @param reportId
    */
   changeReport(reportId: string) {
-    this.getCategoryId().subscribe((categoryId) => {
-      this.setStatisticsState(reportId, categoryId);
-      this.selectedReportId = reportId;
-      this.loadFullReport(reportId);
-    });
+    this.selectedFullReport$.next(null);
+    this.getCategoryId()
+      .pipe(switchMap((categoryId) => combineLatest([this.loadFullReport(reportId), of(categoryId)])))
+      .subscribe(([fullReport, categoryId]: [UsageReport, string]) => {
+        this.setStatisticsState(reportId, categoryId);
+        this.selectedReportId = reportId;
+        this.setReportId(fullReport, reportId);
+      });
   }
 
   /**
    * Load the full report (with points) for the given report id via findOne.
    * @param reportId the composite id (uuid_reportKey)
    */
-  loadFullReport(reportId: string) {
+  loadFullReport(reportId: string): Observable<UsageReport|null> {
     if (!reportId) {
       this.currentReportId = null;
-      this.selectedFullReportSubject.next(null);
-      return;
+      return of(null);
     }
     this.currentReportId = reportId;
     const cachedReport = this.fullReportCache.get(reportId);
     if (cachedReport) {
-      this.selectedFullReportSubject.next(cachedReport);
-      return;
+      return of(cachedReport);
     }
-    this.selectedFullReportSubject.next(null);
-    let request$ = this.inFlightRequests.get(reportId);
-    if (!request$) {
-      request$ = this.usageReportService.findById(reportId, false, true).pipe(
-        getFirstSucceededRemoteData(),
+
+    let fullReport$: Observable<UsageReport> = this.inFlightRequests.get(reportId);
+    if (!fullReport$) {
+      fullReport$ = this.usageReportService.findById(reportId, false, true).pipe(
+        getFirstCompletedRemoteData(),
         getRemoteDataPayload(),
-        take(1),
         shareReplay({ bufferSize: 1, refCount: true }),
         finalize(() => {
           this.inFlightRequests.delete(reportId);
         }),
       );
-      this.inFlightRequests.set(reportId, request$);
+      this.inFlightRequests.set(reportId, fullReport$);
     }
-    request$.subscribe((fullReport: UsageReport) => {
-      if (fullReport) {
-        this.fullReportCache.set(reportId, fullReport);
-      }
-      if (reportId === this.currentReportId) {
-        this.selectedFullReportSubject.next(fullReport);
-      }
-    });
+    return fullReport$;
+
   }
 
   private clearFullReportCache() {
     this.fullReportCache.clear();
     this.inFlightRequests.clear();
     this.currentReportId = null;
-    this.selectedFullReportSubject.next(null);
+    this.selectedFullReport$.next(null);
   }
 
   /**
@@ -415,6 +412,15 @@ export class CrisStatisticsPageComponent implements OnInit, OnDestroy {
     );
   }
 
+  private setReportId(report: UsageReport, reportId) {
+    if (report) {
+      this.fullReportCache.set(reportId, report);
+    }
+    if (reportId === this.currentReportId) {
+      this.selectedFullReport$.next(report);
+    }
+  }
+
   /**
    * Clean the statistics state in component's destroy
    */

src/app/statistics-page/cris-statistics-page/statistics-chart/statistics-chart.component.html

@@ -3,6 +3,7 @@
 }
 
 @if (reports) {
+  @let selectedReport = selectedReport$ | async;
   <div class="container">
     <div class="row">
       <div class="col-md-12 mb-40">
@@ -18,13 +19,13 @@ <h2>{{'statistics.reports.title' | translate}}</h2>
             </li>
           }
         </ul>
-        @if (!selectedReport || !selectedFullReport) {
+        @if (!selectedReport || (selectedFullReport | async) === null || (selectedFullReport | async) === undefined) {
           <ds-loading></ds-loading>
         }
-        @if (selectedReport && selectedFullReport) {
+        @if (selectedReport && (selectedFullReport | async) !== null) {
           <ds-statistics-chart-wrapper [type]="selectedReport.viewMode"
                                        [categoryType]="categoryType"
-                                       [report]="selectedFullReport">
+                                       [report]="selectedFullReport | async">
           </ds-statistics-chart-wrapper>
         }
       </div>