Clarin9/Fix "latest version" notice link ignoring the /repository base href (#876) - #1428
Merged
milanmajchrak merged 3 commits intoAug 6, 2026
Merged
Conversation
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>
There was a problem hiding this comment.
Pull request overview
Fixes the “latest version” notice link on item pages when the UI is deployed under a non-root base href (e.g. /repository/) by ensuring the URL interpolated into the raw-HTML translation is prepared with Angular’s Location.prepareExternalUrl() (matching RouterLink behavior).
Changes:
- Inject
Locationand wrapgetItemPageRoute(item)withlocation.prepareExternalUrl(...)before interpolating into the translation anchor. - Add unit tests covering base-href resolution and DOM-level verification of the rendered
<a href>.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/app/item-page/versions/notice/item-versions-notice.component.ts |
Prepares the interpolated destination URL against the app base href via Location.prepareExternalUrl. |
src/app/item-page/versions/notice/item-versions-notice.component.spec.ts |
Adds specs validating URL preparation and rendered anchor href under a sub-path deployment. |
…rtions - 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>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem description
Fixes A2 of dataquest-dev/dspace-customers#876 (analysed in dataquest-dev/dspace-customers#871).
Anonymous →
http://dev-6.pc:8603/repository/handle/11858/00-097C-0000-0023-9551-4(HamleDT 2.0, version 1 of 2).The notice "This is not the latest version of this item. The latest version can be found here." renders fine, but the
anchor is
href="/items/ff7c3631-…"— without the/repositoryprefix — so clicking it hits nginx and 404s.It is the only root-relative link on the page without the prefix; the version-history links
(
/repository/items/version/250) are correct. On 7.6.5 the same link is correct.Analysis
The
item.version.noticetranslation embeds a raw HTML anchor:and
ds-alertrenders it through[innerHTML](shared/alert/alert.component.html:4). So{{destination}}becomes aliteral browser-resolved
href, not an AngularrouterLink.getItemPage()fed it the bare router pathgetItemPageRoute(item)→/items/<uuid>, and<base href>only applies to relative URLs — root-relative onesalways resolve against the origin. Every other link on the page uses
[routerLink], andRouterLinkrenders itshrefthroughLocationStrategy.prepareExternalUrl(), which is exactly why only this one anchor is broken.The file is byte-identical to upstream DSpace 9.x here — 7.6.5 carried a LINDAT-only
destinationUrl$derived fromdocument.location.pathnamethat the v9 port dropped. This is an upstream bug that only shows up whenui.nameSpace !== '/'.Fix: run the route through
Location.prepareExternalUrl()— the same transformRouterLinkapplies — which is astrict no-op when the base href is
/, so vanilla deployments are unaffected.Re-porting the 7.6.5 hack was rejected: it slices the current pathname, so on the full item page
(
/repository/items/<uuid>/full, where the notice also renders) it would produce/repository/items/<uuid>/<latestUuid>→ 404, it ignores entity-type routing (/entities/publication/<uuid>), anddocument.locationis not meaningful under SSR.Tests
item-versions-notice.component.spec.ts— 3 new specs: the url is resolved against the base href, the unprefixedrouter path is what gets handed to
Location(guards against a future double-prefix), thehasValueguard stillshort-circuits, and a DOM-level assertion that the rendered
<a>really carries/repository/items/<uuid>.item-page/full+item-page/simplespecs 55/55, fullng lintclean.Out of scope (follow-ups, same defect, different features)
Three other places interpolate a root-relative router path into a raw-anchor translation and have the identical bug:
group-form.component(admin.access-control.groups.form.alert.workflowGroup),grant-deny-request-copy.component(grant-deny-request-copy.intro1) andsuggestions-page.component(suggestion.approveAndImport.success). Each needs the same one-liner; kept out of thisPR to keep it reviewable.
Sync verification
No translation files were changed — the fix is entirely on the value side of
{{destination}}.Manual Testing (if applicable)
/repositorydeployment: open version 1 of a versioned item, inspect the notice anchor →href="/repository/items/<latest-uuid>", and clicking it loads the latest version instead of a 404./repository/items/<uuid>/full.NAMESPACE=/instance the anchor is unchanged (/items/<uuid>).Copilot review