[CST-18964] PR review

Author: vins01-4science

src/app/bitstream-page/bitstream-download-page/bitstream-download-page.component.spec.ts

@@ -1,4 +1,7 @@
-import { CommonModule } from '@angular/common';
+import {
+  CommonModule,
+  Location,
+} from '@angular/common';
 import { PLATFORM_ID } from '@angular/core';
 import {
   ComponentFixture,
@@ -14,13 +17,15 @@ import { of as observableOf } from 'rxjs';
 
 import { getForbiddenRoute } from '../../app-routing-paths';
 import { AuthService } from '../../core/auth/auth.service';
+import { DSONameService } from '../../core/breadcrumbs/dso-name.service';
 import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
 import { SignpostingDataService } from '../../core/data/signposting-data.service';
 import { HardRedirectService } from '../../core/services/hard-redirect.service';
 import { ServerResponseService } from '../../core/services/server-response.service';
 import { Bitstream } from '../../core/shared/bitstream.model';
 import { FileService } from '../../core/shared/file.service';
 import { createSuccessfulRemoteDataObject } from '../../shared/remote-data.utils';
+import { MatomoService } from '../../statistics/matomo.service';
 import { BitstreamDownloadPageComponent } from './bitstream-download-page.component';
 
 describe('BitstreamDownloadPageComponent', () => {
@@ -33,10 +38,13 @@ describe('BitstreamDownloadPageComponent', () => {
   let hardRedirectService: HardRedirectService;
   let activatedRoute;
   let router;
+  let location: Location;
+  let dsoNameService: DSONameService;
 
   let bitstream: Bitstream;
   let serverResponseService: jasmine.SpyObj<ServerResponseService>;
   let signpostingDataService: jasmine.SpyObj<SignpostingDataService>;
+  let matomoService: jasmine.SpyObj<MatomoService>;
 
   const mocklink = {
     href: 'http://test.org',
@@ -54,6 +62,7 @@ describe('BitstreamDownloadPageComponent', () => {
     authService = jasmine.createSpyObj('authService', {
       isAuthenticated: observableOf(true),
       setRedirectUrl: {},
+      getShortlivedToken: observableOf('token'),
     });
     authorizationService = jasmine.createSpyObj('authorizationSerivice', {
       isAuthorized: observableOf(true),
@@ -63,9 +72,18 @@ describe('BitstreamDownloadPageComponent', () => {
       retrieveFileDownloadLink: observableOf('content-url-with-headers'),
     });
 
-    hardRedirectService = jasmine.createSpyObj('fileService', {
+    hardRedirectService = jasmine.createSpyObj('hardRedirectService', {
       redirect: {},
     });
+
+    location = jasmine.createSpyObj('location', {
+      back: {},
+    });
+
+    dsoNameService = jasmine.createSpyObj('dsoNameService', {
+      getName: 'Test Bitstream',
+    });
+
     bitstream = Object.assign(new Bitstream(), {
       uuid: 'bitstreamUuid',
       _links: {
@@ -94,6 +112,8 @@ describe('BitstreamDownloadPageComponent', () => {
     signpostingDataService = jasmine.createSpyObj('SignpostingDataService', {
       getLinks: observableOf([mocklink, mocklink2]),
     });
+    matomoService = jasmine.createSpyObj('MatomoService', ['appendVisitorId']);
+    matomoService.appendVisitorId.and.callFake((link) => observableOf(link));
   }
 
   function initTestbed() {
@@ -108,7 +128,10 @@ describe('BitstreamDownloadPageComponent', () => {
         { provide: HardRedirectService, useValue: hardRedirectService },
         { provide: ServerResponseService, useValue: serverResponseService },
         { provide: SignpostingDataService, useValue: signpostingDataService },
+        { provide: MatomoService, useValue: matomoService },
         { provide: PLATFORM_ID, useValue: 'server' },
+        { provide: Location, useValue: location },
+        { provide: DSONameService, useValue: dsoNameService },
       ],
     })
       .compileComponents();
@@ -142,9 +165,11 @@ describe('BitstreamDownloadPageComponent', () => {
         component = fixture.componentInstance;
         fixture.detectChanges();
       });
-      it('should redirect to the content link', () => {
-        expect(hardRedirectService.redirect).toHaveBeenCalledWith('bitstream-content-link');
-      });
+      it('should redirect to the content link', waitForAsync(() => {
+        fixture.whenStable().then(() => {
+          expect(hardRedirectService.redirect).toHaveBeenCalledWith('bitstream-content-link');
+        });
+      }));
       it('should add the signposting links', () => {
         expect(serverResponseService.setHeader).toHaveBeenCalled();
       });
@@ -159,9 +184,11 @@ describe('BitstreamDownloadPageComponent', () => {
         component = fixture.componentInstance;
         fixture.detectChanges();
       });
-      it('should redirect to an updated content link', () => {
-        expect(hardRedirectService.redirect).toHaveBeenCalledWith('content-url-with-headers');
-      });
+      it('should redirect to an updated content link', waitForAsync(() => {
+        fixture.whenStable().then(() => {
+          expect(hardRedirectService.redirect).toHaveBeenCalledWith('content-url-with-headers');
+        });
+      }));
     });
     describe('when the user is not authorized and logged in', () => {
       beforeEach(waitForAsync(() => {
@@ -174,9 +201,11 @@ describe('BitstreamDownloadPageComponent', () => {
         component = fixture.componentInstance;
         fixture.detectChanges();
       });
-      it('should navigate to the forbidden route', () => {
-        expect(router.navigateByUrl).toHaveBeenCalledWith(getForbiddenRoute(), { skipLocationChange: true });
-      });
+      it('should navigate to the forbidden route', waitForAsync(() => {
+        fixture.whenStable().then(() => {
+          expect(router.navigateByUrl).toHaveBeenCalledWith(getForbiddenRoute(), { skipLocationChange: true });
+        });
+      }));
     });
     describe('when the user is not authorized and not logged in', () => {
       beforeEach(waitForAsync(() => {
@@ -190,10 +219,12 @@ describe('BitstreamDownloadPageComponent', () => {
         component = fixture.componentInstance;
         fixture.detectChanges();
       });
-      it('should navigate to the login page', () => {
-        expect(authService.setRedirectUrl).toHaveBeenCalled();
-        expect(router.navigateByUrl).toHaveBeenCalledWith('login');
-      });
+      it('should navigate to the login page', waitForAsync(() => {
+        fixture.whenStable().then(() => {
+          expect(authService.setRedirectUrl).toHaveBeenCalled();
+          expect(router.navigateByUrl).toHaveBeenCalledWith('login');
+        });
+      }));
     });
   });
 });

src/app/bitstream-page/bitstream-download-page/bitstream-download-page.component.ts

@@ -44,6 +44,7 @@ import {
   hasValue,
   isNotEmpty,
 } from '../../shared/empty.util';
+import { MatomoService } from '../../statistics/matomo.service';
 
 @Component({
   selector: 'ds-bitstream-download-page',
@@ -73,6 +74,7 @@ export class BitstreamDownloadPageComponent implements OnInit {
     public dsoNameService: DSONameService,
     private signpostingDataService: SignpostingDataService,
     private responseService: ServerResponseService,
+    private matomoService: MatomoService,
     @Inject(PLATFORM_ID) protected platformId: string,
   ) {
     this.initPageLinks();
@@ -109,14 +111,20 @@ export class BitstreamDownloadPageComponent implements OnInit {
               return [isAuthorized, isLoggedIn, bitstream, fileLink];
             }));
         } else {
-          return [[isAuthorized, isLoggedIn, bitstream, '']];
+          return [[isAuthorized, isLoggedIn, bitstream, bitstream._links.content.href]];
         }
       }),
-    ).subscribe(([isAuthorized, isLoggedIn, bitstream, fileLink]: [boolean, boolean, Bitstream, string]) => {
+      switchMap(([isAuthorized, isLoggedIn, bitstream, fileLink]: [boolean, boolean, Bitstream, string]) =>
+        this.matomoService.appendVisitorId(fileLink)
+          .pipe(
+            map((fileLinkWithVisitorId) => [isAuthorized, isLoggedIn, bitstream, fileLinkWithVisitorId]),
+          ),
+      ),
+    ).subscribe(([isAuthorized, isLoggedIn, , fileLink]: [boolean, boolean, Bitstream, string]) => {
       if (isAuthorized && isLoggedIn && isNotEmpty(fileLink)) {
         this.hardRedirectService.redirect(fileLink);
       } else if (isAuthorized && !isLoggedIn) {
-        this.hardRedirectService.redirect(bitstream._links.content.href);
+        this.hardRedirectService.redirect(fileLink);
       } else if (!isAuthorized && isLoggedIn) {
         this.router.navigateByUrl(getForbiddenRoute(), { skipLocationChange: true });
       } else if (!isAuthorized && !isLoggedIn) {

src/app/shared/cookies/browser-orejime.service.spec.ts

@@ -28,6 +28,7 @@ import { ANONYMOUS_STORAGE_NAME_OREJIME } from './orejime-configuration';
 describe('BrowserOrejimeService', () => {
   const trackingIdProp = 'google.analytics.key';
   const trackingIdTestValue = 'mock-tracking-id';
+  const matomoTrackingId = 'matomo-tracking-id';
   const googleAnalytics = 'google-analytics';
   const recaptchaProp = 'registration.verification.enabled';
   const recaptchaValue = 'true';
@@ -310,9 +311,11 @@ describe('BrowserOrejimeService', () => {
   describe('initialize google analytics configuration', () => {
     let GOOGLE_ANALYTICS_KEY;
     let REGISTRATION_VERIFICATION_ENABLED_KEY;
+    let MATOMO_ENABLED;
     beforeEach(() => {
       GOOGLE_ANALYTICS_KEY = clone((service as any).GOOGLE_ANALYTICS_KEY);
       REGISTRATION_VERIFICATION_ENABLED_KEY = clone((service as any).REGISTRATION_VERIFICATION_ENABLED_KEY);
+      MATOMO_ENABLED = clone((service as any).MATOMO_ENABLED);
       spyOn((service as any), 'getUser$').and.returnValue(observableOf(user));
       translateService.get.and.returnValue(observableOf('loading...'));
       spyOn(service, 'addAppMessages');
@@ -361,6 +364,15 @@ describe('BrowserOrejimeService', () => {
               name: trackingIdTestValue,
               values: ['false'],
             }),
+          )
+          .withArgs(MATOMO_ENABLED)
+          .and
+          .returnValue(
+            createSuccessfulRemoteDataObject$({
+              ... new ConfigurationProperty(),
+              name: matomoTrackingId,
+              values: ['false'],
+            }),
           );
 
       service.initialize();
@@ -380,6 +392,15 @@ describe('BrowserOrejimeService', () => {
               name: trackingIdTestValue,
               values: ['false'],
             }),
+          )
+          .withArgs(MATOMO_ENABLED)
+          .and
+          .returnValue(
+            createSuccessfulRemoteDataObject$({
+              ... new ConfigurationProperty(),
+              name: matomoTrackingId,
+              values: ['false'],
+            }),
           );
       service.initialize();
       expect(service.orejimeConfig.apps).not.toContain(jasmine.objectContaining({ name: googleAnalytics }));
@@ -398,6 +419,15 @@ describe('BrowserOrejimeService', () => {
               name: trackingIdTestValue,
               values: ['false'],
             }),
+          )
+          .withArgs(MATOMO_ENABLED)
+          .and
+          .returnValue(
+            createSuccessfulRemoteDataObject$({
+              ... new ConfigurationProperty(),
+              name: matomoTrackingId,
+              values: ['false'],
+            }),
           );
       service.initialize();
       expect(service.orejimeConfig.apps).not.toContain(jasmine.objectContaining({ name: googleAnalytics }));

src/app/shared/cookies/browser-orejime.service.ts

@@ -90,6 +90,7 @@ export class BrowserOrejimeService extends OrejimeService {
 
   private readonly GOOGLE_ANALYTICS_SERVICE_NAME = 'google-analytics';
 
+  private readonly MATOMO_ENABLED = 'matomo.enabled';
 
   /**
    * Initial Orejime configuration
@@ -134,7 +135,13 @@ export class BrowserOrejimeService extends OrejimeService {
       ),
     );
 
-    const hideMatomo$ = observableOf(!(environment.matomo?.trackerUrl && environment.matomo?.siteId));
+    const hideMatomo$ =
+      this.configService.findByPropertyName(this.MATOMO_ENABLED).pipe(
+        getFirstCompletedRemoteData(),
+        map((remoteData) =>
+          !remoteData.hasSucceeded || !remoteData.payload || isEmpty(remoteData.payload.values) || remoteData.payload.values[0].toLowerCase() !== 'true',
+        ),
+      );
 
     const appsToHide$: Observable<string[]> = observableCombineLatest([hideGoogleAnalytics$, hideRegistrationVerification$, hideMatomo$]).pipe(
       map(([hideGoogleAnalytics, hideRegistrationVerification, hideMatomo]) => {

src/app/statistics/matomo.service.spec.ts

@@ -1,4 +1,8 @@
-import { TestBed } from '@angular/core/testing';
+import {
+  fakeAsync,
+  TestBed,
+  tick,
+} from '@angular/core/testing';
 import {
   MatomoInitializerService,
   MatomoTracker,
@@ -7,25 +11,34 @@ import { MatomoTestingModule } from 'ngx-matomo-client/testing';
 import { of } from 'rxjs';
 
 import { environment } from '../../environments/environment';
+import { ConfigurationDataService } from '../core/data/configuration-data.service';
 import {
   NativeWindowRef,
   NativeWindowService,
 } from '../core/services/window.service';
+import { ConfigurationProperty } from '../core/shared/configuration-property.model';
 import { OrejimeService } from '../shared/cookies/orejime.service';
-import { MatomoService } from './matomo.service';
+import { createSuccessfulRemoteDataObject$ } from '../shared/remote-data.utils';
+import {
+  MATOMO_SITE_ID,
+  MATOMO_TRACKER_URL,
+  MatomoService,
+} from './matomo.service';
 
 describe('MatomoService', () => {
   let service: MatomoService;
   let matomoTracker: jasmine.SpyObj<MatomoTracker>;
   let matomoInitializer: jasmine.SpyObj<MatomoInitializerService>;
   let orejimeService: jasmine.SpyObj<OrejimeService>;
   let nativeWindowService: jasmine.SpyObj<NativeWindowRef>;
+  let configService: jasmine.SpyObj<ConfigurationDataService>;
 
   beforeEach(() => {
-    matomoTracker = jasmine.createSpyObj('MatomoTracker', ['setConsentGiven', 'forgetConsentGiven']);
+    matomoTracker = jasmine.createSpyObj('MatomoTracker', ['setConsentGiven', 'forgetConsentGiven', 'getVisitorId']);
     matomoInitializer = jasmine.createSpyObj('MatomoInitializerService', ['initializeTracker']);
     orejimeService = jasmine.createSpyObj('OrejimeService', ['getSavedPreferences']);
     nativeWindowService = jasmine.createSpyObj('NativeWindowService', [], { nativeWindow: {} });
+    configService = jasmine.createSpyObj('ConfigurationDataService', ['findByPropertyName']);
 
     TestBed.configureTestingModule({
       imports: [MatomoTestingModule.forRoot()],
@@ -34,6 +47,7 @@ describe('MatomoService', () => {
         { provide: MatomoInitializerService, useValue: matomoInitializer },
         { provide: OrejimeService, useValue: orejimeService },
         { provide: NativeWindowService, useValue: nativeWindowService },
+        { provide: ConfigurationDataService, useValue: configService },
       ],
     });
 
@@ -50,9 +64,23 @@ describe('MatomoService', () => {
     expect(nativeWindowService.nativeWindow.changeMatomoConsent).toBe(service.changeMatomoConsent);
   });
 
+  it('should call setConsentGiven when consent is true', () => {
+    service.changeMatomoConsent(true);
+    expect(matomoTracker.setConsentGiven).toHaveBeenCalled();
+  });
+
+  it('should call forgetConsentGiven when consent is false', () => {
+    service.changeMatomoConsent(false);
+    expect(matomoTracker.forgetConsentGiven).toHaveBeenCalled();
+  });
+
   it('should initialize tracker with correct parameters in production', () => {
     environment.production = true;
-    environment.matomo = { siteId: '1', trackerUrl: 'http://example.com' };
+    configService.findByPropertyName.withArgs(MATOMO_TRACKER_URL).and.returnValue(
+      createSuccessfulRemoteDataObject$(Object.assign(new ConfigurationProperty(),{ values: ['http://example.com'] })),
+    );
+    configService.findByPropertyName.withArgs(MATOMO_SITE_ID).and.returnValue(
+      createSuccessfulRemoteDataObject$(Object.assign(new ConfigurationProperty(), { values: ['1'] })));
     orejimeService.getSavedPreferences.and.returnValue(of({ matomo: true }));
 
     service.init();
@@ -72,13 +100,17 @@ describe('MatomoService', () => {
     expect(matomoInitializer.initializeTracker).not.toHaveBeenCalled();
   });
 
-  it('should call setConsentGiven when consent is true', () => {
-    service.changeMatomoConsent(true);
-    expect(matomoTracker.setConsentGiven).toHaveBeenCalled();
-  });
+  describe('with visitorId set', () => {
+    beforeEach(() => {
+      matomoTracker.getVisitorId.and.returnValue(Promise.resolve('12345'));
+    });
+
+    it('should add trackerId parameter', fakeAsync(() => {
+      service.appendVisitorId('http://example.com/')
+        .subscribe(url => expect(url).toEqual('http://example.com/?trackerId=12345'));
+      tick();
+    }));
 
-  it('should call forgetConsentGiven when consent is false', () => {
-    service.changeMatomoConsent(false);
-    expect(matomoTracker.forgetConsentGiven).toHaveBeenCalled();
   });
+
 });