UoE/WP2-New features + bug fixes - #7
Merged
dspeed2 merged 67 commits intoJul 27, 2026
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… existing embargo Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…reen UoE/datashare: add "Keep embargo policies" option to the move-item screen
…eserving it for authorized admin roles
- gate the 5 admin-panel authorization requests behind isAuthenticated: anonymous bootstraps (incl. SSR renders) make zero authorization calls again - extract ADMIN_PANEL_FEATURES const with rationale for the deliberately excluded features (CanSubmit/CanEditItem/CoarNotifyEnabled) - rename createAdminMenuIfLoggedIn$ -> createAdminMenuIfAuthorized$, fix stale doc comment - fix lint: import sort/newlines, EOF newline - specs: anonymous-user cases for resolver + sidebar; fix AuthService stub provider (useValue class ref -> useClass) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- default canEditAccessConditions to false: authorization gates must fail closed until the site-admin check resolves - spec: authorization mock now returns an Observable (was a bare spy function -> '.pipe of undefined' crash when the TestComponent template ran ngOnInit) - spec: stub the component's ChangeDetectorRef in the ngOnInit tests; rendering the full dynamic form against the mocked TestBed caused lazy NullInjectorError (HttpClient) in afterAll, which disconnected Karma and killed the whole CI test run - spec: restore compileComponents() promise chaining - spec: add explicit admin-path test (form model contains accessConditions array for authorized users) and set canEditAccessConditions=true in the tests that build the form directly Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address review feedback: - add error handler to the jsonPatchByResourceID subscription so a failed PATCH no longer leaves the modal stuck in the saving state (+ regression spec) - soften the new authorization specs to assert behavior (presence/ absence of the accessConditions control) instead of exact form model shape Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- remove one-shot auth sampling and keep reactive auth/admin check in admin sidebar - gate sidebar on site-admin authorization only (FeatureID.AdministratorOf) - revert resolver/test/stub changes to match upstream behavior - remove admin panel visibility utility and non-standard test spy alias Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Align showMenu/hideMenu stub signatures with optional MenuID parameter so admin sidebar specs compile. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…flicker overlay) Reported: the page visibly flickers on reload. Root cause: provideClientHydration() is enabled, but DSpace's ThemedComponent (header/navbar, footer, ...) builds its real content imperatively via ViewContainerRef.createComponent() inside a client-side, async ngAfterViewInit. Angular hydration cannot reuse an imperatively-created component, so that subtree is destroyed and re-created client-side, visibly, after the rest of the page has already painted (e.g. the navbar pops in ~500ms after the banner/search box, pushing all content down). Same class of bug as upstream DSpace#3867. Fix, ported from customer/mendelu's Angular-18 hydration-safe variant of the same fix already shipped for other customer instances on this org's Angular-15/DSpace-7 branches: - src/index.html: inline bootstrap script clones (never moves - a move would break hydration on this Angular-17 app) the SSR-painted <ds-app> into a detached, opaque overlay before Angular's bundle runs. The overlay sits on top while hydration + the imperative themed re-render happen invisibly underneath on the untouched original, and self-disables for Cypress/WebDriver-flagged automation. - src/app/app.component.ts: removeSsrOverlayWhenContentVisible() drops the overlay once the auth/theme loader gate has opened AND the live <ds-app> DOM has settled (no element added/removed for 600ms, with real content present) - not on ApplicationRef.isStable, which stays busy long after the page is visually done. 15s hard fallback lives in index.html. - src/typings.d.ts: Window.__dspaceRemoveSsrOverlay global the two files above share. - src/app/app.component.spec.ts: regression test for the gate+settle removal logic. Reproduced and verified locally in Docker (Dockerfile.dist SSR build) against a local DSpace 8.x REST backend - see flicker-evidence/README.md for methodology and before/after screenshots.
…regression Restructure the admin-sidebar authorization specs so each test configures its stubs/spies and then triggers a single ngOnInit via a fresh component, removing the manual comp.ngOnInit() that ran after the shared detectChanges(). Addresses the duplicate-subscription review feedback (Copilot) on the three tests. Add a regression test that drives isAuthenticated() as a BehaviorSubject (false -> true) and asserts the admin menu is hidden then shown, so a re-introduced take(1)/first() on the auth gate would fail the suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Multi-file drag-and-drop uploads showed generic 'Upload successful'/'Upload failed' toasts, so users could not tell which file each notification referred to. Add a non-breaking onCompleteItemWithFile output on UploaderComponent that carries the client-side file name alongside the parsed response, and use it (plus the existing error payload) in SubmissionUploadFilesComponent to render per-file notifications, falling back to the generic keys when no name is available. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The emitter can pass undefined when the FileItem carries no file name; the interface claimed it was always present. Also cover the missing-file-name emit in the uploader spec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Omit fileName from UploaderCompleteEvent when unknown instead of emitting an explicit undefined, type the onUploadError output as EventEmitter<UploaderError>, and drop the explicit undefined fileName from the fallback spec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Selecting a value already present in another row of the same metadata field (e.g. the Funder dropdown) is now blocked: the change is reverted to the previous value and a warning notification is shown. Guard is scoped to scrollable dropdown models only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Statistics pagination is URL-driven with queryParamsHandling: 'merge', so a page/rpp selection made on one scope's statistics page persisted in the URL when navigating to another scope. Clear the table's own params on destroy, like other paginated components do. Verified on the seeded local instance that ds-pagination already renders and pages correctly at community, collection and item level via the shared StatisticsTableComponent (DSpace#726); no further frontend change is needed for lower-level pagination. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The stale stats-* params only survive 'merge' navigations (e.g. the navbar search form); plain navigations drop them. Clarify that clearPagination stages nulls applied on the service's next updateRoute, and why no navigation is issued from ngOnDestroy (racing the in-flight navigation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Let Angular invoke ngOnDestroy through fixture.destroy() on a component whose ngOnInit never ran (createComponent without detectChanges), instead of calling the hook directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Gray out (dsBtnDisabled + gray-200 background) and block options already selected in another row of the same repeatable field, so duplicates cannot be picked at all - mouse or keyboard. Also fix the duplicate-guard revert leaving the rejected value on screen: setCurrentValue before dispatchUpdate, no distinctUntilChanged on the control subscription, explicit model.value revert in the guard. Part of dataquest-dev/dspace-customers#624 (items 001, 005). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add !important to the disabled dropdown-option background/color so they win over Bootstrap's .dropdown-item.disabled, and re-enable pointer-events with a default cursor so hovering a used option shows a normal cursor (selection stays blocked by the dsBtnDisabled/onSelect guards). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
selectOnKeyDown opened the dropdown with sdRef.open() directly on Enter, bypassing openDropdown() so usedSiblingValues (and the option reload/page reset) were stale -> duplicates could be selected via the keyboard-open path. Route through openDropdown() instead. Addresses Copilot review on PR #26. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…abled-option prevention
… clone ids Incorporates the applicable Copilot review point from the sibling PR #16 onto this hydration-safe clone overlay, and records the reasoned outcome of the second one. - pointer-events: the full-viewport SSR freeze-frame now intercepts pointer input (pointer-events: auto) instead of letting clicks pass through to the still-hydrating live app the user cannot see; flipped to none the instant fade-out starts so the revealed app is interactive for the whole 150ms crossfade. - id-stripping (Copilot's other suggestion) is intentionally NOT applied: stripping ids from the clone unstyles every id-gated rule on it (global #main-content flex; header / #main-navbar under emulated encapsulation), which measured 40.65% clone-vs-final mismatch locally and turned the fade-out into the very jump the overlay removes. The clone keeps all ids and stays pixel-identical (0.4% removal diff); duplicate ids are harmless because the clone follows <ds-app> in document order. Rationale in the code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Independent, fully local repro (stock DSpace 8 backend on a clean DB in Docker + this branch's production SSR build) with high-fps CDP screencast capture. Quantifies the navbar-band deviation from the settled page: before (overlay off): navbar entirely missing mid-reload, up to 73.9% deviation, ~2.17s after (overlay on): frozen clone holds the full navbar, <=0.4% deviation, 0ms flicker Includes before/after/diff screenshots, methodology, and the id-stripping measurement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Binary before/after screenshots don't belong in the source tree. The reproduction methodology and the quantified before/after numbers live in the PR discussion instead. Net PR diff is now source-only (index.html, app.component.ts, app.component.spec.ts, typings.d.ts).
…fullscreen-loader watch) PR #23 masked the home-page RELOAD flicker but not the POST-LOGIN flash: login hard- redirects through /reload/<ts> -> /home and, while auth is re-established, root.component.html hides the whole .outer-wrapper (incl. #main-content) via `d-none` and paints .ds-full-screen-loader (white screen + spinner) -- on the current page right after submit AND across the reload. The old overlay was gone by then, so the user saw a ~1s white flash + content shift (admin sidebar). Verified locally in Docker (before: ~1.5s white; after: eliminated). This rewrites the inline bootstrap into one self-contained mask lifecycle that handles both: - installMask(): freeze a detached CLONE of the last real SSR/routed view (still never moves the DOM Angular hydrates; still keeps all ids so the frozen frame is pixel-identical). - A session-long MutationObserver re-masks with the last remembered real frame whenever the fullscreen auth/theme loader appears unmasked (covers login/logout, not just first load). - revealWhenPainted(): lift (150ms fade) only once the routed page is genuinely painted -- no .ds-full-screen-loader, #main-content AND the themed navbar have real layout height -- and the DOM has been quiet for 800ms. The navbar/settle gate is what stops an intermittent reload FOUC (revealing over a half-built/unstyled page). 15s hard cap per mask. - window.__dspaceRemoveSsrOverlay now just nudges the reveal, so AppComponent's existing DOM-settle hook (unchanged) still works; self-disables under Cypress/webdriver as before. Verification: 12/12 logins and 8/8 reloads with zero blank/FOUC frames locally (only the browser's own ~1-frame inter-document reload gap remains, which no in-page script can mask); independently reviewed by 3 UX passes over before/after filmstrips -> all APPROVED. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The overlay must be inlined (it runs before the deferred bundles), so it always appears in the served HTML — "view source" showed 200 readable lines. Minify it with terser at inline time: the served page now carries a single ~2.7kB line with a one-line banner pointing back to the source. src/anti-flicker-overlay.js stays the readable, formatted source; only the browser copy is compacted. Runtime behaviour is unchanged. Verified: served /home inline script is 2 lines (banner + minified body) instead of ~200; the overlay logic is intact (mask installs, click passes through, reveal works). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The datashare theme already renders CC-LICENSE/LICENSE in its own Licences section pinned to page 1. The base FileSectionComponent also fetched CC-LICENSE in lockstep with ORIGINAL on every "show more" click, using the same page number while isLastPage was derived from ORIGINAL only. Once ORIGINAL had more pages than CC-LICENSE, later clicks requested a CC-LICENSE page past its end, which the REST API answered with HTTP 500 - and it also rendered the licence file twice. Fetch ORIGINAL only here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…helpers - MAX_FILE_SIZE_GB/BYTES -> private readonly (used only inside the service; they are constants) - formatBytes: add TB/PB units and clamp the unit index so values >= 1 TB no longer render "undefined" Behaviour for in-range values (<= 20 GB upload cap) is unchanged; verified across ranges. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses Copilot review: clearing isLoading on the error branch enabled a retry that requested the next page rather than re-requesting the failed one, because currentPage is incremented up front. That error-handling tweak was not needed for the 500 fix, so getNextPage now matches upstream exactly (isLoading cleared only on success) - the fix is solely dropping the CC-LICENSE fetch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A genuine login now animates the admin sidebar into view. The auth effect flags the login (one-shot, never on a plain reload); the anti-flicker overlay, as it lifts on the /home the login lands on, adds a body class that runs a CSS keyframe sliding the sidebar in from the left. Crucially this is NOT the vanilla gutter animation, which slides the page content 55px — that is the same shift removed from the reload path, and bringing it back would reintroduce exactly that. Here only the fixed sidebar's transform animates; the content and its reserved gutter never move. So the entrance is visible on login while reload stays perfectly still. The flag is consumed only on a reveal that sticks (no re-mask within 250ms): login redirects through /reload -> /home with several mask/reveal cycles, and an earlier transient reveal must not eat the one-shot flag before the final, visible one. Respects prefers-reduced-motion. Measured, headed, on the real pages: login sidebar slides -55 -> 0, visible after the mask lifts; content 0px (3/3 runs) authenticated F5 content 0px, no entrance animation (flag absent on reload) anonymous F5 0px modal open content 0px click through mask navigates Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Expert review follow-ups: - Class doc on FileSectionComponent stating it lists ORIGINAL only and that licence bundles (CC-LICENSE/LICENSE) are rendered by the theme, so the base never fetches them (paging them in lockstep past their end caused the 500). - Spec now asserts findAllByItemAndBundleName is called with ORIGINAL and never with CC-LICENSE/LICENSE, so a regression re-introducing the second fetch fails. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ys put)" This reverts commit 6ad726e.
…ails Entering a path that does not exist (or a URL, or a path outside the allowed directories) failed the save with the generic "There was an issue when saving the item, please try again later." - which gives the administrator nothing to act on, since the real cause is the path they typed. The backend returns a 400, but DSpace genericises the message and the save effect discards the error, so the specific reason cannot reach the UI. Instead, the saveError effect now checks whether the failed save carried a pending local.bitstream.redirectToURL value - the rolled-back operations still hold it at that point - and, if so, shows a message that says what to check: an absolute path, to a file that exists on the server, inside an allowed directory, and not a URL. Every other save error keeps the original notice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… sentence The earlier message spelled out every possible cause; a plain 'the file could not be found at the path you entered' is enough. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot review: detectChanges() runs ngOnInit -> getNextPage, so the spy is already called before the click. Reset the spy first so the assertions verify the click behaviour, not ngOnInit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…8_x' into uoe/bitstream-upload-from-path
Copilot review: jasmine.anything() does not match undefined (comp.item is unset), weakening the negative assertions. Capture the bundle-name argument of every call and assert it equals exactly ['ORIGINAL'] - independent of the item argument, and failing if any licence bundle is fetched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…efined Address Copilot review: pendingOutcome and pathOutcome genuinely return a third state (undefined = no pending operation touches the field) alongside true/false, so declaring them boolean masked that. Typing them honestly and narrowing with a type guard makes the undefined branch explicit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The old comment claimed 'override to 5 (default) as per config.prod.yml', but the code sets pageSize = 25 and no config.prod.yml sets item.bitstream.pageSize (its default is 5). Reword to state the value is hard-coded to 25 on purpose to load more files per 'Show more' click. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
webpack/index-html-transform.ts imports terser directly (to minify the inlined anti-flicker overlay), but terser was only present transitively via the Angular build tooling. Declare it explicitly so the build does not rely on hoisting. Pinned to 5.29.1 to match the existing yarn.lock resolution; `yarn install --frozen-lockfile` passes with no lockfile change. Addresses a Copilot review note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s aren't squeezed The upstream `--ds-comcol-logo-max-width: 500px` cap limits the logo's *width*. Wide banner-style logos are therefore squeezed into a thin strip: e.g. the RESPIRE collection logo (6000x1100 PNG, 5.45:1) renders at only ~500x92, which users report as a "tiny logo". Raise the width cap to 100% so wide logos fill the header/content column. The `--ds-comcol-logo-max-height: 500px` cap is kept, so tall/square logos are still bounded and only genuinely wide banners get larger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Result-list/detail thumbnail width is capped by --ds-thumbnail-max-width (125px); customer reports thumbnails look small. Raise to 175px, which matches the backend-generated thumbnail resolution (thumbnail.maxwidth = 175), so it stays crisp with no backend change / thumbnail regeneration needed. Height (--ds-card-thumbnail-height) is intentionally left at 240px. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…260)
Renames the /mydspace "Show" options, the page headings beside them and
the Status filter values, and fixes the applied filter showing a raw
Solr token instead of a readable label.
- "Show" dropdown: "Submissions" -> "My submissions",
"Workflow tasks" -> "Submissions – in review"
- Status facet: "Workspace" -> "In draft", "Workflow" -> "In review",
"Validation" -> "For validation". "Archived" and "Waiting for
reviewer" keep their names, as the ticket asks
- Result-card badges and the Administer Workflow screen follow the same
wording, so a row cannot say "In draft" in the filter and "Workspace"
on its badge
- Avatar menu, breadcrumb, page title and "Back to Submissions" move to
"My submissions" together, so the path to the page reads consistently
Ticking a Status value used to relabel the checkbox and the applied
filter chip with the raw token ("item" for Archived,
"waitingforcontroller" for Waiting for reviewer). The unticked facet
list keys off FacetValue.value, which cerialize aliases onto the backend
label; the ticked option and the chip key off AppliedFilter, which
carries the Solr token untouched. Adding the lower-cased token aliases
makes all three render the same label, with no component change.
The screen-reader announcement and the chip aria-label passed the same
raw token through; both now resolve it through the key the visible text
already uses.
Also supersedes the December 2025 naming scheme left behind in the
DataShare theme file. It mapped workspace -> "In review" and workflow ->
"Draft" (the wrong way round), was reverted from src/assets/i18n/en.json5
in 457b86a, and declared two keys twice. It is inert today because
merge-i18n is wired into no build step, but one manual run would have
reinstated it over this change.
"Supervised items" is deliberately left untouched, pending the customer
decision on what it should be called or whether to hide it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dropdowns - key duplicate detection on authority (fallback value) so authority-controlled vocabularies (e.g. Funder) no longer accept the same entry twice - re-check live sibling values at commit time (selectEntry) to close the stale-set gap when a value is chosen in another row after opening - block mousedown on disabled options (dsBtnDisabled); options commit on mousedown - guard keyboard Enter against selecting while options are still loading - expose aria-selected on options - add reproduction/regression spec and a self-contained before/after demo
4b21906 resolved the en.json5 conflict by taking the source branch's whole file. That branch (uoe/move-keep-embargo-policies, cut 2026-06-30) predates three keys the customer already had from sync PR #6, so the cherry-pick silently deleted them: form.vocabulary.load-error item.page.filesection.checksum item.page.doi.pending All three are referenced by code shipped in this PR, and MissingTranslationHelper renders the raw key, so the UI would show "item.page.filesection.checksum" twice per bitstream on the full item record and "item.page.doi.pending" on items awaiting DOI registration. The datashare theme copy does not help: angular.json never copies src/themes/*/assets/i18n into the build and merge-i18n is a manual script, so src/assets/i18n/en.json5 is authoritative at runtime. Unit tests cannot catch this - TranslateLoaderMock returns {}, so the specs assert on the raw key and pass either way. en.json5 is taken whole from the source branch; the two-dot diff on that path is additions-only, so this restores the three keys and changes nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cherry-pick of dataquest-dev/uoe-dspace-datashare-angular 85124a0, authored 17 minutes after the last commit picked into this sync PR and so left outside the pick window. It is the only non-merge commit from the sync window that did not make it across. 2bc0bd6 faithfully carried Dockerfile.karma and demo/** over together with the dropdown fix; upstream then removed them again as local-only recording evidence. Without this commit the sync ships 13 files / 1,574,221 bytes - 98.7% of it incompressible .webm/.gif/.png - and git history is append-only, so a later deletion would not reclaim it. Dockerfile.karma also sits at the repo root with no CMD/ENTRYPOINT and an unpinned node:20-bookworm base, and demo/package.json is a second, unmanaged manifest that dependency scanners would pick up. Note: this keeps the blobs out of the customer's history only under a squash merge, as every previous sync PR was merged. Under a plain merge commit or a rebase merge, 2bc0bd6 still introduces them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e local-only demo harness Fix sync PR UoEMainLibrary#7: restore 3 deleted translations, drop the local-only demo harness
…alues Follow-up to #26 and #32. Both reported problems still reproduced on datashare-UoEMainLibrary-dspace-8_x, because they are not caused by the dropdown widget alone. Deleting a row silently rebound the surviving rows to the wrong controls ------------------------------------------------------------------------ getControlOfGroup() stamped a `startingIndex` on a group model the first time the row rendered and resolved that row's FormGroup as control.get([startingIndex]) from then on. Nothing re-synced it, while DynamicFormArrayModel re-indexes its groups on every insert/remove/move and the template binds formGroupName to that live index. After deleting a non-last row the two disagreed: surviving rows resolved to another row's control or to null, and after delete+add two rows could alias onto the same control. That is what let a value the user never touched be overwritten, and what made the duplicate check read stale sibling values. The live index is now the single authority. Drag and keyboard reordering moved the group models only, which is what the frozen index had been compensating for, so both now move the control alongside the model (moveGroupAndControl). DsDynamicFormControlContainerComponent.ngOnChanges also dereferenced a null `group` in the tick after a row was removed, throwing inside change detection and aborting the pass for the rest of the field. A click aimed at dismissing the menu erased the value ----------------------------------------------------- The menu is a full-width overlay drawn over the field's own bottom edge and the rows beneath it, and its first entry was the destructive "Clear selection" - so the spot a user naturally clicks to dismiss the dropdown wiped their selection, row after row. Clearing now sits at the end of the menu, visually separated, and the menu keeps a small gap below the input. The clear entry also carried both (click) and (mousedown) and therefore fired twice per interaction; it now has a single handler, like the options. Options deliberately keep committing on (mousedown): blurring the input flips showErrorMessages on a required field and forceShowErrorDetection() destroys and re-creates the control, so the element is gone before a click event could reach it. This is now documented in the template. Duplicate detection ------------------- usedSiblingValues was a snapshot refreshed only in openDropdown()/selectEntry(), so it went stale whenever a row was added, removed or edited - and the toggle caret opens the menu through NgbDropdown without calling openDropdown() at all, leaving the set empty and nothing greyed out. It is now derived from the live models, and every path that opens the menu refreshes the options via (openChange). #32's canonicalKey() keyed on `authority ?? value`, but the same entry arrives as a VocabularyEntry when picked in-session and as a FormFieldMetadataValueObject when rebuilt from stored metadata, and only one side may carry an authority. The keys were then incomparable and the duplicate went through. Identity is now the authority *and* the normalised value, and a match on either is a duplicate. Testing ------- 14 specs written first against the unfixed code and confirmed failing, covering the row/control binding across remove/insert/reorder and every duplicate and clearing path. Full suite: 5478 passing. Verified end to end on a dockerised DSpace 8 backend with the datashare theme: the three Type rows survive the dismissing click, the still-used values are greyed out after delete+add (input and caret alike), and what the server stores matches the form with no duplicate and nothing destroyed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both from the Copilot review of this PR; both are regressions this PR introduced. cancelKeyboardDragAndDrop() still moved the group models only, so Escape after a keyboard reorder restored the model order while leaving the FormArray as it was - the exact desync this PR set out to remove. It now goes through moveGroupAndControl() like the drag and arrow-key paths. Dropping the clear entry's (click) handler to stop it firing twice per mouse interaction also removed its keyboard activation, since a native button turns Enter into a click. It now carries (keydown.enter) alongside (mousedown), which mirrors how the options are bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dspeed2
merged commit Jul 27, 2026
ab5ea8c
into
UoEMainLibrary:datashare-UoEMainLibrary-dspace-8_x
6 checks passed
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
4 participants
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.
This PR includes:
New feature:
Keep this item's current embargowhen moving an item, withInherit policiesnow ticked by defaultBugfix:
Show moreerror on items with many files