From 4e6e739a7740240ff02f70db245cdc0f8c603589 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 11:27:50 +0200 Subject: [PATCH 1/3] Clarin9/Fix "latest version" notice link ignoring the base href (#876) The `item.version.notice` translation embeds a raw `` anchor which the alert renders through `[innerHTML]`, so the interpolated value is resolved by the browser, not by the Angular router. `getItemPage()` returned the bare router path `/items/`, and `` does not apply to root-relative URLs, so on a sub-path deployment (``) the link pointed at `/items/` and 404'd. Resolve the route through `Location.prepareExternalUrl()` - the same transform `RouterLink` applies to its own `href` - which is a no-op when the base href is `/`, so vanilla deployments are unaffected. Co-Authored-By: Claude Opus 5 (1M context) --- .../item-versions-notice.component.spec.ts | 39 ++++++++++++++++++- .../notice/item-versions-notice.component.ts | 23 +++++++++-- 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts b/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts index fd3c5096359..8b0af655a4a 100644 --- a/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts +++ b/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts @@ -1,3 +1,4 @@ +import { Location } from '@angular/common'; import { NO_ERRORS_SCHEMA } from '@angular/core'; import { ComponentFixture, @@ -7,7 +8,10 @@ import { import { By } from '@angular/platform-browser'; import { NoopAnimationsModule } from '@angular/platform-browser/animations'; import { RouterTestingModule } from '@angular/router/testing'; -import { TranslateModule } from '@ngx-translate/core'; +import { + TranslateModule, + TranslateService, +} from '@ngx-translate/core'; import { of } from 'rxjs'; import { take } from 'rxjs/operators'; @@ -61,6 +65,7 @@ describe('ItemVersionsNoticeComponent', () => { const versionHistoryServiceSpy = jasmine.createSpyObj('versionHistoryService', ['getVersions', 'getLatestVersionFromHistory$', 'isLatest$' ], ); + const locationStub = jasmine.createSpyObj('location', ['prepareExternalUrl']); beforeEach(waitForAsync(() => { @@ -73,6 +78,7 @@ describe('ItemVersionsNoticeComponent', () => { ], providers: [ { provide: VersionHistoryDataService, useValue: versionHistoryServiceSpy }, + { provide: Location, useValue: locationStub }, ], schemas: [NO_ERRORS_SCHEMA], }).compileComponents(); @@ -84,6 +90,8 @@ describe('ItemVersionsNoticeComponent', () => { versionHistoryServiceSpy.getVersions.and.returnValue(createSuccessfulRemoteDataObject$(createPaginatedList(versions))); versionHistoryServiceSpy.getLatestVersionFromHistory$.and.returnValue(of(latestVersion)); versionHistoryServiceSpy.isLatest$.and.callFake(isLatestFcn); + // Simulate a UI deployed under a sub-path namespace, i.e. + locationStub.prepareExternalUrl.and.callFake((url: string) => `/repository${url}`); })); describe('when the item is the latest version', () => { @@ -121,6 +129,35 @@ describe('ItemVersionsNoticeComponent', () => { }); }); + describe('getItemPage', () => { + beforeEach(() => { + initComponentWithItem(firstItem); + }); + + it('should resolve the latest version item page url against the base href', () => { + expect(component.getItemPage(latestItem)).toEqual('/repository/items/latest_item_id'); + // The plain router path must be what is handed to Location, otherwise the prefix would be applied twice + expect(locationStub.prepareExternalUrl).toHaveBeenCalledWith('/items/latest_item_id'); + }); + + it('should not resolve a url when no item is provided', () => { + expect(component.getItemPage(undefined)).toBeUndefined(); + }); + + it('should render the notice anchor with the base href applied', () => { + const translate = TestBed.inject(TranslateService); + translate.setTranslation('en', { + 'item.version.notice': 'The latest version can be found here.', + }, true); + translate.use('en'); + fixture.detectChanges(); + + const anchor = fixture.debugElement.query(By.css('ds-alert a')); + expect(anchor).not.toBeNull(); + expect(anchor.nativeElement.getAttribute('href')).toEqual('/repository/items/latest_item_id'); + }); + }); + function initComponentWithItem(item: Item) { fixture = TestBed.createComponent(ItemVersionsNoticeComponent); component = fixture.componentInstance; diff --git a/src/app/item-page/versions/notice/item-versions-notice.component.ts b/src/app/item-page/versions/notice/item-versions-notice.component.ts index b44c1fb3411..77a877582c4 100644 --- a/src/app/item-page/versions/notice/item-versions-notice.component.ts +++ b/src/app/item-page/versions/notice/item-versions-notice.component.ts @@ -1,4 +1,7 @@ -import { AsyncPipe } from '@angular/common'; +import { + AsyncPipe, + Location, +} from '@angular/common'; import { Component, Input, @@ -84,7 +87,10 @@ export class ItemVersionsNoticeComponent implements OnInit { */ public AlertTypeEnum = AlertType; - constructor(private versionHistoryService: VersionHistoryDataService) { + constructor( + private versionHistoryService: VersionHistoryDataService, + private location: Location, + ) { } /** @@ -128,12 +134,21 @@ export class ItemVersionsNoticeComponent implements OnInit { } /** - * Get the item page url + * Get the item page url, resolved against the application's base href. + * + * The url is interpolated into the raw `` anchor of the + * `item.version.notice` translation, which the alert renders through `[innerHTML]`. + * It is therefore resolved by the browser and not by the Angular router, so a + * root-relative router path such as `/items/` would ignore `` and + * break every deployment served from a sub-path (e.g. ``). + * `Location.prepareExternalUrl` applies the exact same transformation `RouterLink` + * applies to its `href`, and is a no-op when the base href is `/`. + * * @param item The item for which the url is requested */ getItemPage(item: Item): string { if (hasValue(item)) { - return getItemPageRoute(item); + return this.location.prepareExternalUrl(getItemPageRoute(item)); } } } From 99b95315e1396ea29e5a56687cf901951b961cd9 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 12:28:46 +0200 Subject: [PATCH 2/3] Review feedback: cover the vanilla base href and tighten the spy assertions - getItemPage now declares the contract it always had: the template calls it while the latest version is still loading, so item and the returned url may be undefined (Copilot). - Reset the shared Location spy before acting, so the "the plain router path is what gets handed to Location" assertion cannot be satisfied by the call the template already made during initial rendering, and assert the hasValue guard really short-circuits (Copilot). - Add a describe that drops the Location stub and exercises the real PathLocationStrategy against APP_BASE_HREF '/', '/repository/' and '/repository' (the form express passes on the server), plus an entity-typed item. Without it the "no-op when the base href is /" claim - the case every vanilla install runs - lived only in a code comment. Co-Authored-By: Claude Opus 5 (1M context) --- .../item-versions-notice.component.spec.ts | 61 ++++++++++++++++++- .../notice/item-versions-notice.component.ts | 5 +- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts b/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts index 8b0af655a4a..5d9e80621d4 100644 --- a/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts +++ b/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts @@ -1,4 +1,7 @@ -import { Location } from '@angular/common'; +import { + APP_BASE_HREF, + Location, +} from '@angular/common'; import { NO_ERRORS_SCHEMA } from '@angular/core'; import { ComponentFixture, @@ -135,13 +138,18 @@ describe('ItemVersionsNoticeComponent', () => { }); it('should resolve the latest version item page url against the base href', () => { + locationStub.prepareExternalUrl.calls.reset(); + expect(component.getItemPage(latestItem)).toEqual('/repository/items/latest_item_id'); // The plain router path must be what is handed to Location, otherwise the prefix would be applied twice - expect(locationStub.prepareExternalUrl).toHaveBeenCalledWith('/items/latest_item_id'); + expect(locationStub.prepareExternalUrl).toHaveBeenCalledOnceWith('/items/latest_item_id'); }); it('should not resolve a url when no item is provided', () => { + locationStub.prepareExternalUrl.calls.reset(); + expect(component.getItemPage(undefined)).toBeUndefined(); + expect(locationStub.prepareExternalUrl).not.toHaveBeenCalled(); }); it('should render the notice anchor with the base href applied', () => { @@ -158,6 +166,55 @@ describe('ItemVersionsNoticeComponent', () => { }); }); + describe('getItemPage with the real Location', () => { + // Exercises the actual PathLocationStrategy instead of a stub, so the "no-op for the vanilla + // NAMESPACE=/ deployment" claim is covered rather than asserted only in a comment. + [ + { baseHref: '/', expected: '/items/latest_item_id' }, + { baseHref: '/repository/', expected: '/repository/items/latest_item_id' }, + // the form express hands to the SSR platform injector (req.baseUrl, no trailing slash) + { baseHref: '/repository', expected: '/repository/items/latest_item_id' }, + ].forEach(({ baseHref, expected }) => { + it(`should resolve the item page url against base href '${baseHref}'`, () => { + expect(createComponentWithBaseHref(baseHref).getItemPage(latestItem)).toEqual(expected); + }); + }); + + it('should keep the entity route shape and only add the prefix', () => { + const entityItem = Object.assign(new Item(), { + id: 'entity_item_id', + uuid: 'entity_item_id', + metadata: { 'dspace.entity.type': [{ value: 'Publication' }] }, + }); + + expect(createComponentWithBaseHref('/repository/').getItemPage(entityItem)) + .toEqual('/repository/entities/publication/entity_item_id'); + }); + + function createComponentWithBaseHref(baseHref: string): ItemVersionsNoticeComponent { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + // RouterTestingModule is deliberately left out: it contributes SpyLocation/MockLocationStrategy, + // which ignore APP_BASE_HREF. Without it the root-provided Location/PathLocationStrategy are used. + imports: [ + TranslateModule.forRoot(), + ItemVersionsNoticeComponent, + NoopAnimationsModule, + ], + providers: [ + { provide: VersionHistoryDataService, useValue: versionHistoryServiceSpy }, + { provide: APP_BASE_HREF, useValue: baseHref }, + ], + schemas: [NO_ERRORS_SCHEMA], + }); + + const realLocationFixture = TestBed.createComponent(ItemVersionsNoticeComponent); + realLocationFixture.componentInstance.item = firstItem; + realLocationFixture.detectChanges(); + return realLocationFixture.componentInstance; + } + }); + function initComponentWithItem(item: Item) { fixture = TestBed.createComponent(ItemVersionsNoticeComponent); component = fixture.componentInstance; diff --git a/src/app/item-page/versions/notice/item-versions-notice.component.ts b/src/app/item-page/versions/notice/item-versions-notice.component.ts index 77a877582c4..c79f556f0e6 100644 --- a/src/app/item-page/versions/notice/item-versions-notice.component.ts +++ b/src/app/item-page/versions/notice/item-versions-notice.component.ts @@ -144,9 +144,12 @@ export class ItemVersionsNoticeComponent implements OnInit { * `Location.prepareExternalUrl` applies the exact same transformation `RouterLink` * applies to its `href`, and is a no-op when the base href is `/`. * + * The template calls this while the latest version is still loading, so `item` - and therefore + * the returned url - may be undefined. + * * @param item The item for which the url is requested */ - getItemPage(item: Item): string { + getItemPage(item: Item | undefined): string | undefined { if (hasValue(item)) { return this.location.prepareExternalUrl(getItemPageRoute(item)); } From 58fc23d7ab6defdf5cc1ea83c13b9ce83be735e4 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 6 Aug 2026 09:10:15 +0200 Subject: [PATCH 3/3] Shorten the item page url comments Co-Authored-By: Claude Opus 5 (1M context) --- .../notice/item-versions-notice.component.spec.ts | 6 ++---- .../notice/item-versions-notice.component.ts | 15 ++++----------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts b/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts index 5d9e80621d4..db03559bc71 100644 --- a/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts +++ b/src/app/item-page/versions/notice/item-versions-notice.component.spec.ts @@ -167,8 +167,7 @@ describe('ItemVersionsNoticeComponent', () => { }); describe('getItemPage with the real Location', () => { - // Exercises the actual PathLocationStrategy instead of a stub, so the "no-op for the vanilla - // NAMESPACE=/ deployment" claim is covered rather than asserted only in a comment. + // the real PathLocationStrategy, so the "no-op for NAMESPACE=/" claim is actually covered [ { baseHref: '/', expected: '/items/latest_item_id' }, { baseHref: '/repository/', expected: '/repository/items/latest_item_id' }, @@ -194,8 +193,7 @@ describe('ItemVersionsNoticeComponent', () => { function createComponentWithBaseHref(baseHref: string): ItemVersionsNoticeComponent { TestBed.resetTestingModule(); TestBed.configureTestingModule({ - // RouterTestingModule is deliberately left out: it contributes SpyLocation/MockLocationStrategy, - // which ignore APP_BASE_HREF. Without it the root-provided Location/PathLocationStrategy are used. + // no RouterTestingModule on purpose: its SpyLocation/MockLocationStrategy ignore APP_BASE_HREF imports: [ TranslateModule.forRoot(), ItemVersionsNoticeComponent, diff --git a/src/app/item-page/versions/notice/item-versions-notice.component.ts b/src/app/item-page/versions/notice/item-versions-notice.component.ts index c79f556f0e6..47484ca215e 100644 --- a/src/app/item-page/versions/notice/item-versions-notice.component.ts +++ b/src/app/item-page/versions/notice/item-versions-notice.component.ts @@ -134,18 +134,11 @@ export class ItemVersionsNoticeComponent implements OnInit { } /** - * Get the item page url, resolved against the application's base href. + * Get the item page url, resolved against the base href. The url lands in the raw `` of the + * `item.version.notice` translation, so the browser resolves it and not the router - a plain + * `/items/` would ignore ``. No-op when the base href is `/`. * - * The url is interpolated into the raw `` anchor of the - * `item.version.notice` translation, which the alert renders through `[innerHTML]`. - * It is therefore resolved by the browser and not by the Angular router, so a - * root-relative router path such as `/items/` would ignore `` and - * break every deployment served from a sub-path (e.g. ``). - * `Location.prepareExternalUrl` applies the exact same transformation `RouterLink` - * applies to its `href`, and is a no-op when the base href is `/`. - * - * The template calls this while the latest version is still loading, so `item` - and therefore - * the returned url - may be undefined. + * Undefined while the latest version is still loading. * * @param item The item for which the url is requested */