Clarin9/Fix "latest version" notice link ignoring the /repository base href (#876) (#1428)

* Clarin9/Fix "latest version" notice link ignoring the base href (#876)

The `item.version.notice` translation embeds a raw `<a href='{{destination}}'>`
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/<uuid>`, and `<base href>` does not apply to
root-relative URLs, so on a sub-path deployment (`<base href="/repository/">`)
the link pointed at `/items/<uuid>` 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* Shorten the item page url comments

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: milanmajchrak

src/app/item-page/versions/notice/item-versions-notice.component.spec.ts

@@ -1,3 +1,7 @@
+import {
+  APP_BASE_HREF,
+  Location,
+} from '@angular/common';
 import { NO_ERRORS_SCHEMA } from '@angular/core';
 import {
   ComponentFixture,
@@ -7,7 +11,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 +68,7 @@ describe('ItemVersionsNoticeComponent', () => {
   const versionHistoryServiceSpy = jasmine.createSpyObj('versionHistoryService',
     ['getVersions', 'getLatestVersionFromHistory$', 'isLatest$' ],
   );
+  const locationStub = jasmine.createSpyObj('location', ['prepareExternalUrl']);
 
   beforeEach(waitForAsync(() => {
 
@@ -73,6 +81,7 @@ describe('ItemVersionsNoticeComponent', () => {
       ],
       providers: [
         { provide: VersionHistoryDataService, useValue: versionHistoryServiceSpy },
+        { provide: Location, useValue: locationStub },
       ],
       schemas: [NO_ERRORS_SCHEMA],
     }).compileComponents();
@@ -84,6 +93,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. <base href="/repository/">
+    locationStub.prepareExternalUrl.and.callFake((url: string) => `/repository${url}`);
   }));
 
   describe('when the item is the latest version', () => {
@@ -121,6 +132,87 @@ describe('ItemVersionsNoticeComponent', () => {
     });
   });
 
+  describe('getItemPage', () => {
+    beforeEach(() => {
+      initComponentWithItem(firstItem);
+    });
+
+    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).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', () => {
+      const translate = TestBed.inject(TranslateService);
+      translate.setTranslation('en', {
+        'item.version.notice': 'The latest version can be found <a href=\'{{destination}}\'>here</a>.',
+      }, 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');
+    });
+  });
+
+  describe('getItemPage with the real Location', () => {
+    // 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' },
+      // 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({
+        // no RouterTestingModule on purpose: its SpyLocation/MockLocationStrategy ignore APP_BASE_HREF
+        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;

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,17 @@ export class ItemVersionsNoticeComponent implements OnInit {
   }
 
   /**
-   * Get the item page url
+   * Get the item page url, resolved against the base href. The url lands in the raw `<a href>` of the
+   * `item.version.notice` translation, so the browser resolves it and not the router - a plain
+   * `/items/<uuid>` would ignore `<base href="/repository/">`. No-op when the base href is `/`.
+   *
+   * Undefined while the latest version is still loading.
+   *
    * @param item The item for which the url is requested
    */
-  getItemPage(item: Item): string {
+  getItemPage(item: Item | undefined): string | undefined {
     if (hasValue(item)) {
-      return getItemPageRoute(item);
+      return this.location.prepareExternalUrl(getItemPageRoute(item));
     }
   }
 }