From a7faf7d449a44ce793bfe4b72cf7b377445ae181 Mon Sep 17 00:00:00 2001 From: Alexandre Vryghem Date: Mon, 16 Oct 2023 22:16:22 +0200 Subject: [PATCH] 107671: Fix handle theme not working with canonical prefix https://hdl.handle.net/ --- .../core/data/configuration-data.service.ts | 1 - .../curation-form.component.spec.ts | 12 +-- .../curation-form/curation-form.component.ts | 82 +++++++++-------- src/app/shared/handle.service.spec.ts | 86 ++++++++++++------ src/app/shared/handle.service.ts | 82 ++++++++++++----- .../configuration-data.service.stub.ts | 14 +++ .../theme-support/theme.service.spec.ts | 43 ++++++--- src/app/shared/theme-support/theme.service.ts | 71 ++++++++------- src/config/theme.model.spec.ts | 87 ++++++++++++++----- src/config/theme.model.ts | 34 +++++--- 10 files changed, 347 insertions(+), 165 deletions(-) create mode 100644 src/app/shared/testing/configuration-data.service.stub.ts diff --git a/src/app/core/data/configuration-data.service.ts b/src/app/core/data/configuration-data.service.ts index de044e25e3..557e13f57b 100644 --- a/src/app/core/data/configuration-data.service.ts +++ b/src/app/core/data/configuration-data.service.ts @@ -1,4 +1,3 @@ -/* eslint-disable max-classes-per-file */ import { Injectable } from '@angular/core'; import { Observable } from 'rxjs'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; diff --git a/src/app/curation-form/curation-form.component.spec.ts b/src/app/curation-form/curation-form.component.spec.ts index dc70b925e8..a0bdee21f4 100644 --- a/src/app/curation-form/curation-form.component.spec.ts +++ b/src/app/curation-form/curation-form.component.spec.ts @@ -1,4 +1,4 @@ -import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing'; +import { ComponentFixture, TestBed, waitForAsync, fakeAsync, flush } from '@angular/core/testing'; import { TranslateModule } from '@ngx-translate/core'; import { CUSTOM_ELEMENTS_SCHEMA } from '@angular/core'; import { CurationFormComponent } from './curation-form.component'; @@ -16,6 +16,7 @@ import { ConfigurationDataService } from '../core/data/configuration-data.servic import { ConfigurationProperty } from '../core/shared/configuration-property.model'; import { getProcessDetailRoute } from '../process-page/process-page-routing.paths'; import { HandleService } from '../shared/handle.service'; +import { of as observableOf } from 'rxjs'; describe('CurationFormComponent', () => { let comp: CurationFormComponent; @@ -54,7 +55,7 @@ describe('CurationFormComponent', () => { }); handleService = { - normalizeHandle: (a) => a + normalizeHandle: (a: string) => observableOf(a), } as any; notificationsService = new NotificationsServiceStub(); @@ -151,12 +152,13 @@ describe('CurationFormComponent', () => { ], []); }); - it(`should show an error notification and return when an invalid dsoHandle is provided`, () => { + it(`should show an error notification and return when an invalid dsoHandle is provided`, fakeAsync(() => { comp.dsoHandle = 'test-handle'; - spyOn(handleService, 'normalizeHandle').and.returnValue(null); + spyOn(handleService, 'normalizeHandle').and.returnValue(observableOf(null)); comp.submit(); + flush(); expect(notificationsService.error).toHaveBeenCalled(); expect(scriptDataService.invoke).not.toHaveBeenCalled(); - }); + })); }); diff --git a/src/app/curation-form/curation-form.component.ts b/src/app/curation-form/curation-form.component.ts index 2c0e900a66..cc2c14f89f 100644 --- a/src/app/curation-form/curation-form.component.ts +++ b/src/app/curation-form/curation-form.component.ts @@ -1,22 +1,22 @@ -import { ChangeDetectorRef, Component, Input, OnInit } from '@angular/core'; +import { ChangeDetectorRef, Component, Input, OnDestroy, OnInit } from '@angular/core'; import { ScriptDataService } from '../core/data/processes/script-data.service'; import { UntypedFormControl, UntypedFormGroup } from '@angular/forms'; -import { getFirstCompletedRemoteData } from '../core/shared/operators'; -import { find, map } from 'rxjs/operators'; +import { getFirstCompletedRemoteData, getFirstSucceededRemoteDataPayload } from '../core/shared/operators'; +import { map } from 'rxjs/operators'; import { NotificationsService } from '../shared/notifications/notifications.service'; import { TranslateService } from '@ngx-translate/core'; import { hasValue, isEmpty, isNotEmpty } from '../shared/empty.util'; import { RemoteData } from '../core/data/remote-data'; import { Router } from '@angular/router'; -import { ProcessDataService } from '../core/data/processes/process-data.service'; import { Process } from '../process-page/processes/process.model'; import { ConfigurationDataService } from '../core/data/configuration-data.service'; import { ConfigurationProperty } from '../core/shared/configuration-property.model'; -import { Observable } from 'rxjs'; +import { Observable, Subscription } from 'rxjs'; import { getProcessDetailRoute } from '../process-page/process-page-routing.paths'; import { HandleService } from '../shared/handle.service'; export const CURATION_CFG = 'plugin.named.org.dspace.curate.CurationTask'; + /** * Component responsible for rendering the Curation Task form */ @@ -24,7 +24,7 @@ export const CURATION_CFG = 'plugin.named.org.dspace.curate.CurationTask'; selector: 'ds-curation-form', templateUrl: './curation-form.component.html' }) -export class CurationFormComponent implements OnInit { +export class CurationFormComponent implements OnDestroy, OnInit { config: Observable>; tasks: string[]; @@ -33,10 +33,11 @@ export class CurationFormComponent implements OnInit { @Input() dsoHandle: string; + subs: Subscription[] = []; + constructor( private scriptDataService: ScriptDataService, private configurationDataService: ConfigurationDataService, - private processDataService: ProcessDataService, private notificationsService: NotificationsService, private translateService: TranslateService, private handleService: HandleService, @@ -45,6 +46,10 @@ export class CurationFormComponent implements OnInit { ) { } + ngOnDestroy(): void { + this.subs.forEach((sub: Subscription) => sub.unsubscribe()); + } + ngOnInit(): void { this.form = new UntypedFormGroup({ task: new UntypedFormControl(''), @@ -52,16 +57,15 @@ export class CurationFormComponent implements OnInit { }); this.config = this.configurationDataService.findByPropertyName(CURATION_CFG); - this.config.pipe( - find((rd: RemoteData) => rd.hasSucceeded), - map((rd: RemoteData) => rd.payload) - ).subscribe((configProperties) => { + this.subs.push(this.config.pipe( + getFirstSucceededRemoteDataPayload(), + ).subscribe((configProperties: ConfigurationProperty) => { this.tasks = configProperties.values .filter((value) => isNotEmpty(value) && value.includes('=')) .map((value) => value.split('=')[1].trim()); this.form.get('task').patchValue(this.tasks[0]); this.cdr.detectChanges(); - }); + })); } /** @@ -77,33 +81,41 @@ export class CurationFormComponent implements OnInit { */ submit() { const taskName = this.form.get('task').value; - let handle; + let handle$: Observable; if (this.hasHandleValue()) { - handle = this.handleService.normalizeHandle(this.dsoHandle); - if (isEmpty(handle)) { - this.notificationsService.error(this.translateService.get('curation.form.submit.error.head'), - this.translateService.get('curation.form.submit.error.invalid-handle')); - return; - } + handle$ = this.handleService.normalizeHandle(this.dsoHandle).pipe( + map((handle: string | null) => { + if (isEmpty(handle)) { + this.notificationsService.error(this.translateService.get('curation.form.submit.error.head'), + this.translateService.get('curation.form.submit.error.invalid-handle')); + } + return handle; + }), + ); } else { - handle = this.handleService.normalizeHandle(this.form.get('handle').value); - if (isEmpty(handle)) { - handle = 'all'; - } + handle$ = this.handleService.normalizeHandle(this.form.get('handle').value).pipe( + map((handle: string | null) => isEmpty(handle) ? 'all' : handle), + ); } - this.scriptDataService.invoke('curate', [ - { name: '-t', value: taskName }, - { name: '-i', value: handle }, - ], []).pipe(getFirstCompletedRemoteData()).subscribe((rd: RemoteData) => { - if (rd.hasSucceeded) { - this.notificationsService.success(this.translateService.get('curation.form.submit.success.head'), - this.translateService.get('curation.form.submit.success.content')); - this.router.navigateByUrl(getProcessDetailRoute(rd.payload.processId)); - } else { - this.notificationsService.error(this.translateService.get('curation.form.submit.error.head'), - this.translateService.get('curation.form.submit.error.content')); + this.subs.push(handle$.subscribe((handle: string) => { + if (hasValue(handle)) { + this.subs.push(this.scriptDataService.invoke('curate', [ + { name: '-t', value: taskName }, + { name: '-i', value: handle }, + ], []).pipe( + getFirstCompletedRemoteData(), + ).subscribe((rd: RemoteData) => { + if (rd.hasSucceeded) { + this.notificationsService.success(this.translateService.get('curation.form.submit.success.head'), + this.translateService.get('curation.form.submit.success.content')); + void this.router.navigateByUrl(getProcessDetailRoute(rd.payload.processId)); + } else { + this.notificationsService.error(this.translateService.get('curation.form.submit.error.head'), + this.translateService.get('curation.form.submit.error.content')); + } + })); } - }); + })); } } diff --git a/src/app/shared/handle.service.spec.ts b/src/app/shared/handle.service.spec.ts index b326eb0416..a499bdd464 100644 --- a/src/app/shared/handle.service.spec.ts +++ b/src/app/shared/handle.service.spec.ts @@ -1,47 +1,79 @@ import { HandleService } from './handle.service'; +import { TestBed } from '@angular/core/testing'; +import { ConfigurationDataServiceStub } from './testing/configuration-data.service.stub'; +import { ConfigurationDataService } from '../core/data/configuration-data.service'; +import { of as observableOf } from 'rxjs'; describe('HandleService', () => { let service: HandleService; + let configurationService: ConfigurationDataServiceStub; + beforeEach(() => { - service = new HandleService(); + configurationService = new ConfigurationDataServiceStub(); + + TestBed.configureTestingModule({ + providers: [ + { provide: ConfigurationDataService, useValue: configurationService }, + ], + }); + service = TestBed.inject(HandleService); }); describe(`normalizeHandle`, () => { - it(`should simply return an already normalized handle`, () => { - let input, output; + it('should normalize a handle url with custom conical prefix with trailing slash', (done: DoneFn) => { + service.canonicalPrefix$ = observableOf('https://hdl.handle.net/'); - input = '123456789/123456'; - output = service.normalizeHandle(input); - expect(output).toEqual(input); - - input = '12.3456.789/123456'; - output = service.normalizeHandle(input); - expect(output).toEqual(input); + service.normalizeHandle('https://hdl.handle.net/123456789/123456').subscribe((handle: string | null) => { + expect(handle).toBe('123456789/123456'); + done(); + }); }); - it(`should normalize a handle url`, () => { - let input, output; + it('should normalize a handle url with custom conical prefix without trailing slash', (done: DoneFn) => { + service.canonicalPrefix$ = observableOf('https://hdl.handle.net'); - input = 'https://hdl.handle.net/handle/123456789/123456'; - output = service.normalizeHandle(input); - expect(output).toEqual('123456789/123456'); - - input = 'https://rest.api/server/handle/123456789/123456'; - output = service.normalizeHandle(input); - expect(output).toEqual('123456789/123456'); + service.normalizeHandle('https://hdl.handle.net/123456789/123456').subscribe((handle: string | null) => { + expect(handle).toBe('123456789/123456'); + done(); + }); }); - it(`should return null if the input doesn't contain a handle`, () => { - let input, output; + describe('should simply return an already normalized handle', () => { + it('123456789/123456', (done: DoneFn) => { + service.normalizeHandle('123456789/123456').subscribe((handle: string | null) => { + expect(handle).toBe('123456789/123456'); + done(); + }); + }); - input = 'https://hdl.handle.net/handle/123456789'; - output = service.normalizeHandle(input); - expect(output).toBeNull(); + it('12.3456.789/123456', (done: DoneFn) => { + service.normalizeHandle('12.3456.789/123456').subscribe((handle: string | null) => { + expect(handle).toBe('12.3456.789/123456'); + done(); + }); + }); + }); - input = 'something completely different'; - output = service.normalizeHandle(input); - expect(output).toBeNull(); + it('should normalize handle urls starting with handle', (done: DoneFn) => { + service.normalizeHandle('https://rest.api/server/handle/123456789/123456').subscribe((handle: string | null) => { + expect(handle).toBe('123456789/123456'); + done(); + }); + }); + + it('should return null if the input doesn\'t contain a valid handle', (done: DoneFn) => { + service.normalizeHandle('https://hdl.handle.net/123456789').subscribe((handle: string | null) => { + expect(handle).toBeNull(); + done(); + }); + }); + + it('should return null if the input doesn\'t contain a handle', (done: DoneFn) => { + service.normalizeHandle('something completely different').subscribe((handle: string | null) => { + expect(handle).toBeNull(); + done(); + }); }); }); }); diff --git a/src/app/shared/handle.service.ts b/src/app/shared/handle.service.ts index da0f17f7de..56b3922753 100644 --- a/src/app/shared/handle.service.ts +++ b/src/app/shared/handle.service.ts @@ -1,7 +1,18 @@ import { Injectable } from '@angular/core'; -import { isNotEmpty, isEmpty } from './empty.util'; +import { isEmpty, hasNoValue } from './empty.util'; +import { ConfigurationDataService } from '../core/data/configuration-data.service'; +import { getFirstCompletedRemoteData } from '../core/shared/operators'; +import { map, take } from 'rxjs/operators'; +import { ConfigurationProperty } from '../core/shared/configuration-property.model'; +import { Observable } from 'rxjs'; +import { RemoteData } from '../core/data/remote-data'; -const PREFIX_REGEX = /handle\/([^\/]+\/[^\/]+)$/; +export const CANONICAL_PREFIX_KEY = 'handle.canonical.prefix'; + +const PREFIX_REGEX = (prefix: string | undefined) => { + const formattedPrefix: string = prefix?.replace(/\/$/, ''); + return new RegExp(`(${formattedPrefix ? formattedPrefix + '|' : '' }handle)\/([^\/]+\/[^\/]+)$`); +}; const NO_PREFIX_REGEX = /^([^\/]+\/[^\/]+)$/; @Injectable({ @@ -9,33 +20,62 @@ const NO_PREFIX_REGEX = /^([^\/]+\/[^\/]+)$/; }) export class HandleService { + canonicalPrefix$: Observable; + + constructor( + protected configurationService: ConfigurationDataService, + ) { + this.canonicalPrefix$ = this.configurationService.findByPropertyName(CANONICAL_PREFIX_KEY).pipe( + getFirstCompletedRemoteData(), + take(1), + map((configurationPropertyRD: RemoteData) => { + if (configurationPropertyRD.hasSucceeded) { + return configurationPropertyRD.payload.values.length >= 1 ? configurationPropertyRD.payload.values[0] : undefined; + } else { + return undefined; + } + }), + ); + } /** * Turns a handle string into the default 123456789/12345 format * - * @param handle the input handle + * When the handle.canonical.prefix doesn't end with handle, be sure to expose the variable so that the + * frontend can find the handle * - * normalizeHandle('123456789/123456') // '123456789/123456' - * normalizeHandle('12.3456.789/123456') // '12.3456.789/123456' - * normalizeHandle('https://hdl.handle.net/handle/123456789/123456') // '123456789/123456' - * normalizeHandle('https://rest.api/server/handle/123456789/123456') // '123456789/123456' - * normalizeHandle('https://rest.api/server/handle/123456789') // null + * @param handle the input handle + * @return + *
    + *
  • normalizeHandle('123456789/123456') // '123456789/123456'
    • + *
  • normalizeHandle('12.3456.789/123456') // '12.3456.789/123456'
    • + *
  • normalizeHandle('https://hdl.handle.net/123456789/123456') // '123456789/123456'
    • + *
  • normalizeHandle('https://rest.api/server/handle/123456789/123456') // '123456789/123456'
    • + *
  • normalizeHandle('https://rest.api/server/handle/123456789') // null
    • + *
*/ - normalizeHandle(handle: string): string { - let matches: string[]; - if (isNotEmpty(handle)) { - matches = handle.match(PREFIX_REGEX); - } + normalizeHandle(handle: string): Observable { + return this.canonicalPrefix$.pipe( + map((prefix: string | undefined) => { + let matches: string[]; + if (hasNoValue(handle)) { + return null; + } - if (isEmpty(matches) || matches.length < 2) { - matches = handle.match(NO_PREFIX_REGEX); - } + matches = handle.match(PREFIX_REGEX(prefix)); - if (isEmpty(matches) || matches.length < 2) { - return null; - } else { - return matches[1]; - } + if (isEmpty(matches) || matches.length < 3) { + matches = handle.match(NO_PREFIX_REGEX); + } + + if (isEmpty(matches) || matches.length < 2) { + return null; + } else { + return matches[matches.length - 1]; + } + }), + take(1), + ); } } diff --git a/src/app/shared/testing/configuration-data.service.stub.ts b/src/app/shared/testing/configuration-data.service.stub.ts new file mode 100644 index 0000000000..f17e2d7b5b --- /dev/null +++ b/src/app/shared/testing/configuration-data.service.stub.ts @@ -0,0 +1,14 @@ +import { Observable } from 'rxjs'; +import { RemoteData } from '../../core/data/remote-data'; +import { ConfigurationProperty } from '../../core/shared/configuration-property.model'; +import { createSuccessfulRemoteDataObject$ } from '../remote-data.utils'; + +export class ConfigurationDataServiceStub { + + findByPropertyName(_name: string): Observable> { + const configurationProperty = new ConfigurationProperty(); + configurationProperty.values = []; + return createSuccessfulRemoteDataObject$(configurationProperty); + } + +} diff --git a/src/app/shared/theme-support/theme.service.spec.ts b/src/app/shared/theme-support/theme.service.spec.ts index f56fb86cbc..c4669408e1 100644 --- a/src/app/shared/theme-support/theme.service.spec.ts +++ b/src/app/shared/theme-support/theme.service.spec.ts @@ -24,6 +24,8 @@ import { ROUTER_NAVIGATED } from '@ngrx/router-store'; import { ActivatedRouteSnapshot, Router } from '@angular/router'; import { CommonModule, DOCUMENT } from '@angular/common'; import { RouterMock } from '../mocks/router.mock'; +import { ConfigurationDataServiceStub } from '../testing/configuration-data.service.stub'; +import { ConfigurationDataService } from '../../core/data/configuration-data.service'; /** * LinkService able to mock recursively resolving DSO parent links @@ -49,6 +51,7 @@ class MockLinkService { describe('ThemeService', () => { let themeService: ThemeService; let linkService: LinkService; + let configurationService: ConfigurationDataServiceStub; let initialState; let ancestorDSOs: DSpaceObject[]; @@ -78,6 +81,7 @@ describe('ThemeService', () => { currentTheme: 'custom', }, }; + configurationService = new ConfigurationDataServiceStub(); } function setupServiceWithActions(mockActions) { @@ -96,6 +100,7 @@ describe('ThemeService', () => { provideMockActions(() => mockActions), { provide: DSpaceObjectDataService, useValue: mockDsoService }, { provide: Router, useValue: new RouterMock() }, + { provide: ConfigurationDataService, useValue: configurationService }, ] }); @@ -112,7 +117,7 @@ describe('ThemeService', () => { function spyOnPrivateMethods() { spyOn((themeService as any), 'getAncestorDSOs').and.returnValue(() => observableOf([dso])); - spyOn((themeService as any), 'matchThemeToDSOs').and.returnValue(new Theme({ name: 'custom' })); + spyOn((themeService as any), 'matchThemeToDSOs').and.returnValue(observableOf(new Theme({ name: 'custom' }))); spyOn((themeService as any), 'getActionForMatch').and.returnValue(new SetThemeAction('custom')); } @@ -283,13 +288,13 @@ describe('ThemeService', () => { beforeEach(() => { nonMatchingTheme = Object.assign(new Theme({ name: 'non-matching-theme' }), { - matches: () => false + matches: () => observableOf(false), }); itemMatchingTheme = Object.assign(new Theme({ name: 'item-matching-theme' }), { - matches: (url, dso) => (dso as any).type === ITEM.value + matches: (url, dso) => observableOf((dso as any).type === ITEM.value), }); communityMatchingTheme = Object.assign(new Theme({ name: 'community-matching-theme' }), { - matches: (url, dso) => (dso as any).type === COMMUNITY.value + matches: (url, dso) => observableOf((dso as any).type === COMMUNITY.value), }); dsos = [ Object.assign(new Item(), { @@ -313,8 +318,11 @@ describe('ThemeService', () => { themeService.themes = themes; }); - it('should return undefined', () => { - expect((themeService as any).matchThemeToDSOs(dsos, '')).toBeUndefined(); + it('should return undefined', (done: DoneFn) => { + (themeService as any).matchThemeToDSOs(dsos, '').subscribe((theme: Theme) => { + expect(theme).toBeUndefined(); + done(); + }); }); }); @@ -324,20 +332,31 @@ describe('ThemeService', () => { themeService.themes = themes; }); - it('should return the matching theme', () => { - expect((themeService as any).matchThemeToDSOs(dsos, '')).toEqual(itemMatchingTheme); + it('should return the matching theme', (done: DoneFn) => { + (themeService as any).matchThemeToDSOs(dsos, '').subscribe((theme: Theme) => { + expect(theme).toBe(itemMatchingTheme); + done(); + }); }); }); describe('when multiple themes match some of the DSOs', () => { - it('should return the first matching theme', () => { + it('should return the first matching theme (itemMatchingTheme)', (done: DoneFn) => { themes = [ nonMatchingTheme, itemMatchingTheme, communityMatchingTheme ]; themeService.themes = themes; - expect((themeService as any).matchThemeToDSOs(dsos, '')).toEqual(itemMatchingTheme); + (themeService as any).matchThemeToDSOs(dsos, '').subscribe((theme: Theme) => { + expect(theme).toBe(itemMatchingTheme); + done(); + }); + }); + it('should return the first matching theme (communityMatchingTheme)', (done: DoneFn) => { themes = [ nonMatchingTheme, communityMatchingTheme, itemMatchingTheme ]; themeService.themes = themes; - expect((themeService as any).matchThemeToDSOs(dsos, '')).toEqual(communityMatchingTheme); + (themeService as any).matchThemeToDSOs(dsos, '').subscribe((theme: Theme) => { + expect(theme).toBe(communityMatchingTheme); + done(); + }); }); }); }); @@ -382,6 +401,7 @@ describe('ThemeService', () => { const mockDsoService = { findById: () => createSuccessfulRemoteDataObject$(mockCommunity) }; + configurationService = new ConfigurationDataServiceStub(); TestBed.configureTestingModule({ imports: [ @@ -393,6 +413,7 @@ describe('ThemeService', () => { provideMockStore({ initialState }), { provide: DSpaceObjectDataService, useValue: mockDsoService }, { provide: Router, useValue: new RouterMock() }, + { provide: ConfigurationDataService, useValue: configurationService }, ] }); diff --git a/src/app/shared/theme-support/theme.service.ts b/src/app/shared/theme-support/theme.service.ts index 4ce976dd58..daf2e3960e 100644 --- a/src/app/shared/theme-support/theme.service.ts +++ b/src/app/shared/theme-support/theme.service.ts @@ -1,17 +1,13 @@ import { Injectable, Inject, Injector } from '@angular/core'; import { Store, createFeatureSelector, createSelector, select } from '@ngrx/store'; -import { BehaviorSubject, EMPTY, Observable, of as observableOf } from 'rxjs'; +import { BehaviorSubject, EMPTY, Observable, of as observableOf, from, concatMap } from 'rxjs'; import { ThemeState } from './theme.reducer'; import { SetThemeAction, ThemeActionTypes } from './theme.actions'; -import { expand, filter, map, switchMap, take, toArray } from 'rxjs/operators'; +import { defaultIfEmpty, expand, filter, map, switchMap, take, toArray } from 'rxjs/operators'; import { hasNoValue, hasValue, isNotEmpty } from '../empty.util'; import { RemoteData } from '../../core/data/remote-data'; import { DSpaceObject } from '../../core/shared/dspace-object.model'; -import { - getFirstCompletedRemoteData, - getFirstSucceededRemoteData, - getRemoteDataPayload -} from '../../core/shared/operators'; +import { getFirstCompletedRemoteData, getFirstSucceededRemoteData, getRemoteDataPayload } from '../../core/shared/operators'; import { HeadTagConfig, Theme, ThemeConfig, themeFactory } from '../../../config/theme.model'; import { NO_OP_ACTION_TYPE, NoOpAction } from '../ngrx/no-op.action'; import { followLink } from '../utils/follow-link-config.model'; @@ -219,7 +215,7 @@ export class ThemeService { // create new head tags (not yet added to DOM) const headTagFragment = this.document.createDocumentFragment(); this.createHeadTags(themeName) - .forEach(newHeadTag => headTagFragment.appendChild(newHeadTag)); + .forEach(newHeadTag => headTagFragment.appendChild(newHeadTag)); // add new head tags to DOM head.appendChild(headTagFragment); @@ -268,7 +264,7 @@ export class ThemeService { if (hasValue(headTagConfig.attributes)) { Object.entries(headTagConfig.attributes) - .forEach(([key, value]) => tag.setAttribute(key, value)); + .forEach(([key, value]) => tag.setAttribute(key, value)); } // 'class' attribute should always be 'theme-head-tag' for removal @@ -292,7 +288,7 @@ export class ThemeService { // and the current theme from the store const currentTheme$: Observable = this.store.pipe(select(currentThemeSelector)); - const action$ = currentTheme$.pipe( + const action$: Observable = currentTheme$.pipe( switchMap((currentTheme: string) => { const snapshotWithData = this.findRouteData(activatedRouteSnapshot); if (this.hasDynamicTheme === true && isNotEmpty(this.themes)) { @@ -302,8 +298,10 @@ export class ThemeService { // Start with the resolved dso and go recursively through its parents until you reach the top-level community return observableOf(dsoRD.payload).pipe( this.getAncestorDSOs(), - map((dsos: DSpaceObject[]) => { - const dsoMatch = this.matchThemeToDSOs(dsos, currentRouteUrl); + switchMap((dsos: DSpaceObject[]) => { + return this.matchThemeToDSOs(dsos, currentRouteUrl); + }), + map((dsoMatch: Theme) => { return this.getActionForMatch(dsoMatch, currentTheme); }) ); @@ -316,33 +314,41 @@ export class ThemeService { getFirstSucceededRemoteData(), getRemoteDataPayload(), this.getAncestorDSOs(), - map((dsos: DSpaceObject[]) => { - const dsoMatch = this.matchThemeToDSOs(dsos, currentRouteUrl); + switchMap((dsos: DSpaceObject[]) => { + return this.matchThemeToDSOs(dsos, currentRouteUrl); + }), + map((dsoMatch: Theme) => { return this.getActionForMatch(dsoMatch, currentTheme); }) ); } // check whether the route itself matches - const routeMatch = this.themes.find((theme: Theme) => theme.matches(currentRouteUrl, undefined)); - - return [this.getActionForMatch(routeMatch, currentTheme)]; + return from(this.themes).pipe( + concatMap((theme: Theme) => theme.matches(currentRouteUrl, undefined).pipe( + filter((result: boolean) => result === true), + map(() => theme), + take(1), + )), + take(1), + map((theme: Theme) => this.getActionForMatch(theme, currentTheme)) + ); + } else { + // If there are no themes configured, do nothing + return observableOf(new NoOpAction()); } - - // If there are no themes configured, do nothing - return [new NoOpAction()]; }), take(1), ); action$.pipe( - filter((action) => action.type !== NO_OP_ACTION_TYPE), - ).subscribe((action) => { + filter((action: SetThemeAction | NoOpAction) => action.type !== NO_OP_ACTION_TYPE), + ).subscribe((action: SetThemeAction | NoOpAction) => { this.store.dispatch(action); }); return action$.pipe( - map((action) => action.type === ThemeActionTypes.SET), + map((action: SetThemeAction | NoOpAction) => action.type === ThemeActionTypes.SET), ); } @@ -433,14 +439,17 @@ export class ThemeService { * @param currentRouteUrl The url for the current route * @private */ - private matchThemeToDSOs(dsos: DSpaceObject[], currentRouteUrl: string): Theme { - // iterate over the themes in order, and return the first one that matches - return this.themes.find((theme: Theme) => { - // iterate over the dsos's in order (most specific one first, so Item, Collection, - // Community), and return the first one that matches the current theme - const match = dsos.find((dso: DSpaceObject) => theme.matches(currentRouteUrl, dso)); - return hasValue(match); - }); + private matchThemeToDSOs(dsos: DSpaceObject[], currentRouteUrl: string): Observable { + return from(this.themes).pipe( + concatMap((theme: Theme) => from(dsos).pipe( + concatMap((dso: DSpaceObject) => theme.matches(currentRouteUrl, dso)), + filter((result: boolean) => result === true), + map(() => theme), + take(1), + )), + take(1), + defaultIfEmpty(undefined), + ); } /** diff --git a/src/config/theme.model.spec.ts b/src/config/theme.model.spec.ts index 79b5a1f32b..d5005cb245 100644 --- a/src/config/theme.model.spec.ts +++ b/src/config/theme.model.spec.ts @@ -9,12 +9,15 @@ import { Item } from '../app/core/shared/item.model'; import { ITEM } from '../app/core/shared/item.resource-type'; import { getItemModuleRoute } from '../app/item-page/item-page-routing-paths'; import { HandleService } from '../app/shared/handle.service'; +import { TestBed } from '@angular/core/testing'; +import { ConfigurationDataService } from '../app/core/data/configuration-data.service'; +import { ConfigurationDataServiceStub } from '../app/shared/testing/configuration-data.service.stub'; describe('Theme Models', () => { let theme: Theme; describe('RegExTheme', () => { - it('should return true when the regex matches the community\'s DSO route', () => { + it('should return true when the regex matches the community\'s DSO route', (done: DoneFn) => { theme = new RegExTheme({ name: 'community', regex: getCommunityModuleRoute() + '/.*', @@ -23,10 +26,13 @@ describe('Theme Models', () => { type: COMMUNITY.value, uuid: 'community-uuid', }); - expect(theme.matches('', dso)).toEqual(true); + theme.matches('', dso).subscribe((matches: boolean) => { + expect(matches).toBeTrue(); + done(); + }); }); - it('should return true when the regex matches the collection\'s DSO route', () => { + it('should return true when the regex matches the collection\'s DSO route', (done: DoneFn) => { theme = new RegExTheme({ name: 'collection', regex: getCollectionModuleRoute() + '/.*', @@ -35,10 +41,13 @@ describe('Theme Models', () => { type: COLLECTION.value, uuid: 'collection-uuid', }); - expect(theme.matches('', dso)).toEqual(true); + theme.matches('', dso).subscribe((matches: boolean) => { + expect(matches).toBeTrue(); + done(); + }); }); - it('should return true when the regex matches the item\'s DSO route', () => { + it('should return true when the regex matches the item\'s DSO route', (done: DoneFn) => { theme = new RegExTheme({ name: 'item', regex: getItemModuleRoute() + '/.*', @@ -47,32 +56,51 @@ describe('Theme Models', () => { type: ITEM.value, uuid: 'item-uuid', }); - expect(theme.matches('', dso)).toEqual(true); + theme.matches('', dso).subscribe((matches: boolean) => { + expect(matches).toBeTrue(); + done(); + }); }); - it('should return true when the regex matches the url', () => { + it('should return true when the regex matches the url', (done: DoneFn) => { theme = new RegExTheme({ name: 'url', regex: '.*partial.*', }); - expect(theme.matches('theme/partial/url/match', null)).toEqual(true); + theme.matches('theme/partial/url/match', null).subscribe((matches: boolean) => { + expect(matches).toBeTrue(); + done(); + }); }); - it('should return false when the regex matches neither the url, nor the DSO route', () => { + it('should return false when the regex matches neither the url, nor the DSO route', (done: DoneFn) => { theme = new RegExTheme({ name: 'no-match', regex: '.*no/match.*', }); - expect(theme.matches('theme/partial/url/match', null)).toEqual(false); + theme.matches('theme/partial/url/match', null).subscribe((matches: boolean) => { + expect(matches).toBeFalse(); + done(); + }); }); }); describe('HandleTheme', () => { - let handleService; + let handleService: HandleService; + + let configurationService: ConfigurationDataServiceStub; + beforeEach(() => { - handleService = new HandleService(); + configurationService = new ConfigurationDataServiceStub(); + + TestBed.configureTestingModule({ + providers: [ + { provide: ConfigurationDataService, useValue: configurationService }, + ], }); - it('should return true when the DSO\'s handle matches the theme\'s handle', () => { + handleService = TestBed.inject(HandleService); + }); + it('should return true when the DSO\'s handle matches the theme\'s handle', (done: DoneFn) => { theme = new HandleTheme({ name: 'matching-handle', handle: '1234/5678', @@ -82,9 +110,12 @@ describe('Theme Models', () => { uuid: 'item-uuid', handle: '1234/5678', }, handleService); - expect(theme.matches('', matchingDso)).toEqual(true); + theme.matches('', matchingDso).subscribe((matches: boolean) => { + expect(matches).toBeTrue(); + done(); + }); }); - it('should return false when the DSO\'s handle contains the theme\'s handle as a subpart', () => { + it('should return false when the DSO\'s handle contains the theme\'s handle as a subpart', (done: DoneFn) => { theme = new HandleTheme({ name: 'matching-handle', handle: '1234/5678', @@ -94,10 +125,13 @@ describe('Theme Models', () => { uuid: 'item-uuid', handle: '1234/567891011', }); - expect(theme.matches('', dso)).toEqual(false); + theme.matches('', dso).subscribe((matches: boolean) => { + expect(matches).toBeFalse(); + done(); + }); }); - it('should return false when the handles don\'t match', () => { + it('should return false when the handles don\'t match', (done: DoneFn) => { theme = new HandleTheme({ name: 'no-matching-handle', handle: '1234/5678', @@ -107,12 +141,15 @@ describe('Theme Models', () => { uuid: 'item-uuid', handle: '1234/6789', }); - expect(theme.matches('', dso)).toEqual(false); + theme.matches('', dso).subscribe((matches: boolean) => { + expect(matches).toBeFalse(); + done(); + }); }); }); describe('UUIDTheme', () => { - it('should return true when the DSO\'s UUID matches the theme\'s UUID', () => { + it('should return true when the DSO\'s UUID matches the theme\'s UUID', (done: DoneFn) => { theme = new UUIDTheme({ name: 'matching-uuid', uuid: 'item-uuid', @@ -121,10 +158,13 @@ describe('Theme Models', () => { type: ITEM.value, uuid: 'item-uuid', }); - expect(theme.matches('', dso)).toEqual(true); + theme.matches('', dso).subscribe((matches: boolean) => { + expect(matches).toBeTrue(); + done(); + }); }); - it('should return true when the UUIDs don\'t match', () => { + it('should return true when the UUIDs don\'t match', (done: DoneFn) => { theme = new UUIDTheme({ name: 'matching-uuid', uuid: 'item-uuid', @@ -133,7 +173,10 @@ describe('Theme Models', () => { type: COLLECTION.value, uuid: 'collection-uuid', }); - expect(theme.matches('', dso)).toEqual(false); + theme.matches('', dso).subscribe((matches: boolean) => { + expect(matches).toBeFalse(); + done(); + }); }); }); }); diff --git a/src/config/theme.model.ts b/src/config/theme.model.ts index 019540f18a..571d47a1d1 100644 --- a/src/config/theme.model.ts +++ b/src/config/theme.model.ts @@ -6,6 +6,8 @@ import { getDSORoute } from '../app/app-routing-paths'; import { HandleObject } from '../app/core/shared/handle-object.model'; import { Injector } from '@angular/core'; import { HandleService } from '../app/shared/handle.service'; +import { combineLatest, Observable, of as observableOf } from 'rxjs'; +import { map, take } from 'rxjs/operators'; export interface NamedThemeConfig extends Config { name: string; @@ -55,8 +57,8 @@ export class Theme { constructor(public config: NamedThemeConfig) { } - matches(url: string, dso: DSpaceObject): boolean { - return true; + matches(url: string, dso: DSpaceObject): Observable { + return observableOf(true); } } @@ -68,7 +70,7 @@ export class RegExTheme extends Theme { this.regex = new RegExp(this.config.regex); } - matches(url: string, dso: DSpaceObject): boolean { + matches(url: string, dso: DSpaceObject): Observable { let match; const route = getDSORoute(dso); @@ -80,25 +82,33 @@ export class RegExTheme extends Theme { match = url.match(this.regex); } - return hasValue(match); + return observableOf(hasValue(match)); } } export class HandleTheme extends Theme { - private normalizedHandle; + private normalizedHandle$: Observable; constructor(public config: HandleThemeConfig, protected handleService: HandleService ) { super(config); - this.normalizedHandle = this.handleService.normalizeHandle(this.config.handle); - + this.normalizedHandle$ = this.handleService.normalizeHandle(this.config.handle).pipe( + take(1), + ); } - matches(url: string, dso: T): boolean { - return hasValue(dso) && hasValue(dso.handle) - && this.handleService.normalizeHandle(dso.handle) === this.normalizedHandle; + matches(url: string, dso: T): Observable { + return combineLatest([ + this.handleService.normalizeHandle(dso?.handle), + this.normalizedHandle$, + ]).pipe( + map(([handle, normalizedHandle]: [string | null, string | null]) => { + return hasValue(dso) && hasValue(dso.handle) && handle === normalizedHandle; + }), + take(1), + ); } } @@ -107,8 +117,8 @@ export class UUIDTheme extends Theme { super(config); } - matches(url: string, dso: DSpaceObject): boolean { - return hasValue(dso) && dso.uuid === this.config.uuid; + matches(url: string, dso: DSpaceObject): Observable { + return observableOf(hasValue(dso) && dso.uuid === this.config.uuid); } }