[DSpace-CRIS] Administrative Edit of archived items via submission form and security configuration for metadata visibility (Frontend)#5145
Conversation
tdonohue
added
new feature
DSpace-CRIS merger
|
@FrancescoMolinaro : Same with this PR, please add in the description which DSpace-CRIS differences this PR implements/includes. |
|
Is there also a backend PR for the security configuration for metadata visibility? |
|
Hi @pnbecker yes there is a Rest PR but is not ready yet, I will link it as soon as possible. |
31cafeb to
ab0885e
Compare
|
Hi @FrancescoMolinaro, |
|
@pnbecker and @FrancescoMolinaro : It looks like the backend PR for this PR is DSpace/DSpace#11964. I'll update the description of this PR to make that clear |
tdonohue
changed the title
For the remaning open points :
|
|
NOTE: I've done final testing today and verified the "double upload" problem is fixed. The backend PR was just merged. Once the fresh Docker images for the backend are created (may take 30-60mins), I'm going to restart the tests in this PR to verify all e2e tests now pass. Assuming the e2e tests pass, this PR will also be merged as it's already at +2. Any outstanding issues (including those I reported above) should be moved to follow-up tickets so they can be addressed in 10.0 (or later if necessary) |
|
@FrancescoMolinaro : After merging the backend PR, all e2e tests except one ( |
|
@FrancescoMolinaro : After more analysis of the
In order to still move this PR along quickly, I'd propose we temporarily disable the failing test and create a follow-up ticket to fix this issue. The specific test to disable is the first one in that file, and we can disable it by changing the |
|
@FrancescoMolinaro : A follow-up to my last message. This PR definitely is causing the error in I've realized that if I load any Publication Entity page while running the UI in Dev Mode then the "Statistics" menu never appears in the header. Here's how to reproduce:
I'm still OK with temporarily disabling this e2e test unless it's obvious how to fix this bug. We can move that e2e test to a folllow-up ticket, if it's not able to be fixed in this PR. |
tdonohue
left a comment
tdonohue
left a comment
There was a problem hiding this comment.
@FrancescoMolinaro : In addition to the broken e2e test above, I stumbled on a few very minor issues in the i18n keys below. These would be good to fix quickly
|
|
||
| "mydspace.show.workspace": "Your submissions", | ||
|
|
||
| "mydspace.show.otherworkspace": "Shared Submissions", |
There was a problem hiding this comment.
In re-testing today, it appears this key should be updated to be mydspace.show.otherWorkspace, as the key mydspace.show.otherworkspace is no longer working. (The difference is that "Workspace" needs to be capitalized for this key to work)
|
|
||
| "workflow.search.results.head": "Workflow tasks", | ||
|
|
||
| "otherworkspace.search.results.head": "Workspace submissions", |
There was a problem hiding this comment.
Similarly here, this key should be updated to be otherWorkspace.search.results.head.
Can we also update the value to say "Shared submissions" to match the value of mydspace.show.otherWorkspace above?
|
@FrancescoMolinaro : Final note from me for today... For this issue:
I've found that I cannot reproduce this with a new Item. I.e. I cannot seem to create an Item from the UI which will cause this behavior. I suspect I have some old, corrupt data in my system which is resulting in this behavior (possibly the bug occurs when the Authority ID of a metadata field is invalid or points at a deleted object's UUID??). Regardless, in the User Interface, I cannot create an invalid Authority ID because it's validity is checked & saving the invalid value fails. So, it's possible this is only reproducible with corrupted data. For now, I'll hold off on creating a ticket until I can find a way to reproduce it reliably. |
|
Hi @tdonohue , thanks for the feedback and sorry for the delay, the e2e should be fixed now, the error was caused by an uncaught error in the new edit menu, I have also updated the labels as you noted, hopefully this is now ready for merge. |
tdonohue
left a comment
tdonohue
left a comment
There was a problem hiding this comment.
👍 Thanks @FrancescoMolinaro for the final cleanup (getting test fixed & i18n fixes). It looks correct to me and I verified the i18n keys are working properly now. Merging this with +2 approvals.
github-project-automation
Bot
moved this from 👍 Reviewer Approved
to ✅ Done
in DSpace 10.0 Release
|
@FrancescoMolinaro : Forgot to mention, I'm adding the "needs documentation" flag to this PR because the new features added here all need to be documented in our 10.x docs: https://wiki.lyrasis.org/display/DSDOC10x |
tdonohue
added
the
needs documentation
|
@FrancescoMolinaro and @AdamF42 : It looks like we have some basic docs on Editing in Submission mode, but we're still missing documentation of Metadata security (e.g. Once those docs can be added to the 10.x docs, we can remove the |
|
Metadata security docs now at https://wiki.lyrasis.org/spaces/DSDOC10x/pages/438698074/Metadata+security+configuration |
tdonohue
removed
the
needs documentation
…port DSpace#5145) Fix submission stuck on "Saving..." (serialize save effects, backport DSpace#5145)
* feat: show file checksum instead of format in full item view Replace the per-file "Format" line in the full item view file listing with a "File checksum" line (algorithm + value). Add the checkSum field to the Bitstream model so the REST value deserializes, and add the item.page.filesection.checksum i18n key. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: guard checksum display and show value only Wrap the full item view checksum dt/dd in *ngIf="file?.checkSum" and render only the checksum value (drop the algorithm prefix), so no stray colon shows when a bitstream has no checksum. Propagate item.page.filesection.checksum to all locale files with an English placeholder. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: align Bitstream checkSum with upstream ChecksumInfo Match DSpace/dspace-angular@main (commit 4f72074): use the exported ChecksumInfo interface and `checkSum: ChecksumInfo` positioned after `description`, replacing the inline anonymous type, to minimize conflicts on a future DSpace 9 migration. Display and i18n key are unchanged (upstream renders no checksum row in the full item view). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * revert: drop non-en filesection.checksum i18n key UoE instance only ships English; other locale .json5 files aren't maintained here. Revert the checksum key addition to all 27 non-en locales, keep it in en.json5 only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: drop redundant optional chaining on checkSum file is guaranteed by *ngFor and checkSum is truthy inside the *ngIf guard, so use plain file.checkSum / file.checkSum.value to match the surrounding template style. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: changed - File checksum: - to - Checksum: - * fix: added checksum algorithm (same style as upstream) * Fix submission stuck on "Saving..." by serializing save effects (switchMap -> concatMap) Backport of the save-effect change from upstream PR DSpace#5145 (commit a3c14c5, first released in DSpace 10.0; not present in 8.x/9.x). The four submission save effects used switchMap. When two save actions of the same type overlap (common during editing: unguarded section/upload/save-for-later saves, save-on-change on validation errors, or an enabled autosave timer), switchMap cancels the in-flight save's inner observable AFTER it dispatched StartTransactionPatchOperationsAction (commitPending=true) but BEFORE its response handler runs. Commit/Rollback are dispatched only inside that cancelled observable, so commitPending stays true forever; the next save is then dropped by the take(1)+filter(!commitPending) guard in submitJsonPatchOperations, no SAVE_*_SUCCESS/ERROR is emitted, savePending stays true, and the form is stuck on "Saving..." until the user reloads (losing unsaved metadata). concatMap serializes the save effects so each inner observable completes (dispatching Commit/Rollback and clearing commitPending) before the next save runs, eliminating the race. Only the four save effects are changed (saveSubmission$, saveForLaterSubmission$, saveSection$, saveAndDeposit$), matching upstream. deposit/discard/updateSection intentionally keep switchMap. The improved catchError from DSpace#5145 is not backported (it depends on parseErrorResponse and action signatures not present in 8.3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Harden submission form/vocabulary loading against permanent stuck spinners Fixes a class of "spinner spins forever, user must reload and loses typed metadata" bugs in the submission form. A component sets a loading/searching flag, loads data via a RemoteData pipe, and resets the flag ONLY on the success path using getFirstSucceededRemoteData()/...Payload()/...ListPayload(). DSpace turns HTTP errors into a NON-throwing failed RemoteData (state=Error), and the store-backed RemoteData observable never completes, so on a failed OR slow/ never-responding request the success-only operator never emits, the component's catchError (which only catches thrown errors) never fires, and the spinner stays forever. There is also no HTTP request timeout anywhere in the app. Inherited from vanilla DSpace 8.3 and NOT fixed upstream (the same pattern is still present on main/10.x), so there is no backport available -- this is a new hardening fix, following the same getFirstCompletedRemoteData approach upstream used for the whole-form loading fix (DSpace#4060). For each affected load: switch getFirstSucceededRemoteData* to getFirstCompletedRemoteData() (emits on success AND failure, then completes), extract the payload failure-safely, add timeout({ each: 30000 }) for never-responding requests, and reset the spinner flag in finalize() so it always clears. On failure/timeout the control shows an empty result instead of hanging. Sites fixed (all inherited from 8.3): - sections/form/section-form.component.ts (Describe section) -- bounded 30s timeout so it never spins forever; kept getFirstSucceededRemoteData for the item follow-link to avoid dereferencing an undefined submission object. - ds-dynamic-form-ui/models/list/dynamic-list.component.ts (radio/checkbox vocab, e.g. dc.type) - ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts (vocabulary dropdowns) - ds-dynamic-form-ui/models/onebox/dynamic-onebox.component.ts (authority typeahead) - ds-dynamic-form-ui/models/tag/dynamic-tag.component.ts (keyword/tag authority) - ds-dynamic-form-ui/models/lookup/dynamic-lookup.component.ts (lookup authority) - vocabulary-treeview/vocabulary-treeview.service.ts (hierarchical vocabulary tree) - collection-dropdown/collection-dropdown.component.ts (collection picker; parentCommunity was hanging reduce()) - submission/form/collection/submission-form-collection.component.ts (change-collection selector) - sections/cc-license/submission-section-cc-licenses.component.ts (CC-license section) - ds-dynamic-form-ui/models/relation-group/dynamic-relation-group.components.ts (relation chips on edit) Verified locally: `tsc --noEmit -p tsconfig.app.json` and `eslint` both pass with 0 errors on the changed files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(repro): add before/after reproduction of the dropdown loading hang Deterministic reproduction on a local DSpace 8.3 stack (published dspace-8_x-test backend images + this frontend). A Playwright script simulates a failing/slow controlled-vocabulary endpoint by returning HTTP 500 for /api/submission/vocabularies/{name}/entries, then opens the "Type" dropdown on a new submission's Describe form. - before: getFirstSucceededRemoteData ignores the failed RemoteData, so the "Loading..." indicator stays visible forever (measured stuck for the full 14s observation window). - after: getFirstCompletedRemoteData + finalize handles the failed RemoteData, the spinner clears immediately and the dropdown shows "No results found". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(repro): add before/after reproduction of the stuck "Saving..." hang Deterministic reproduction on a local DSpace 8.3 stack. A Playwright script delays every submission PATCH by ~3s (models a slow save) and fires two overlapping autosaves (two edits of the autosave-on-change dc.title field). - before (base, switchMap): the second save cancels the in-flight first save mid-transaction; commitPending is orphaned true; the second save is dropped by the take(1)+filter(!commitPending) guard (only 1 PATCH ever leaves the client); savePending never resets -> footer stuck on "Saving..." for the full 18s window. - after (this PR, concatMap): the saves are serialized; the first completes and clears commitPending before the second runs; "Saving..." clears within ~4s. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(repro): add never-responding-backend (timeout backstop) before/after Second reproduction covering the slow/never-responding path (not just HTTP 500). The vocabulary /entries request is held open forever (models a hung socket). - before (no timeout): "Loading..." stays stuck past 45s (never recovers). - after (timeout({each:30000})): the spinner clears ~30s after the request stalls and the dropdown shows "No results found". Clips are sped up ~3x. Confirms the timeout() backstop half of the fix, which the HTTP-500 reproduction did not exercise. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(item-page): show DOI field on simple item view while DOI registration is pending The DataShare simple item view renders the DOI ("Persistent Identifier") field via ds-item-page-uri-field, which filtered dc.identifier.uri for values starting with https://doi.org. The whole field wrapper was gated behind *ngIf="hasDoiLink", so when a record's DOI has not been registered yet (the DOI value is only written to dc.identifier.uri by the backend AFTER the doi-organiser scheduled task registers it), the field was hidden entirely and users saw no indication that a DOI exists or is coming. Restore the previous behaviour where the DOI field is always shown (with an empty value while registration is pending): - MetadataUriValuesComponent / ItemPageUriFieldComponent: add an opt-in `doiField` input. In DOI mode the field wrapper is always rendered (hideIfNoTextContent = false) and only https://doi.org values are shown as links. When false (default) the upstream generic behaviour is kept, which also repairs non-DOI URI fields in the base theme that were unintentionally hidden by the previous global filter. - datashare untyped-item: opt the DOI field into `[doiField]="true"`. Test-driven: added specs for the pending (no DOI yet), registered, and empty cases, plus the default (non-DOI) URI behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Reduce vocab-load timeout to 15s + show a user-friendly error notification Follow-up to the loading-spinner hardening: - Lower the request timeout backstop from 30s to 15s on all the hardened loads (a 30s spinner is too long for an interactive dropdown). - On a failed OR timed-out controlled-vocabulary / authority lookup, show a dismissible error toast ("Something went wrong while loading the options. Please try again.") instead of silently showing an empty "No results found", so the user knows to retry. Added a shared notifyVocabularyLoadError() helper on the DsDynamicVocabularyComponent base (via inject()) and wired it into the scrollable-dropdown and lookup loads. A genuinely empty (succeeded) result is unchanged -> still "No results found", no error toast. - Harden dynamic-onebox vocabulary$ (findVocabularyById): getFirstSucceededRemoteDataPayload -> getFirstCompletedRemoteData, so a failed vocabulary-metadata lookup resolves to "not hierarchical" (no tree button) instead of leaving isHierarchicalVocabulary$ pending forever. Null-safe guards on result?.hierarchical and vocabulary?.preloadLevel. tsc --noEmit and eslint pass with 0 errors. Verified in the local repro: on a 500, the Type dropdown now clears immediately, shows "No results found", AND a red "Something went wrong ... Please try again." toast appears. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(repro): refresh dropdown "after" clip to show the 15s + error-toast behaviour The after clip now shows the updated recovery: on a failed vocabulary load the Type dropdown clears immediately, shows "No results found", and a dismissible "Something went wrong ... Please try again." error toast appears. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(repro): refresh never-responding after clip for the 15s timeout (was 30s) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(item-page): collapse DOI/default URI rendering into a single wrapper Address review nits on the pending-DOI fix without changing behaviour: - Merge the doiField and default template branches into one ds-metadata-field-wrapper ([hideIfNoTextContent]="!doiField"), removing the duplicated link markup. - Add a `doiValues` getter that filters mdValues to https://doi.org values (restoring the `typeof value === 'string'` guard). In DOI mode the *ngFor and the `!last` separator are computed against this visible-DOI subset, so a trailing non-DOI value can no longer emit a stray separator after the last DOI. - Tighten the registered-case spec to assert the label header, and add a regression spec for multiple DOIs followed by a non-DOI value (only one separator between the DOIs). Verified locally: metadata-uri-values + item-page-uri-field specs 23/23 green, lint clean, build:prod (SSR production build) exits 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: provide NotificationsService stub in vocabulary component specs The DsDynamicVocabularyComponent base now injects NotificationsService (for the "something went wrong" error notification on a failed vocabulary load). Its subclasses' unit tests (scrollable-dropdown, onebox, tag, lookup) did not provide it, so component construction failed with "NullInjectorError: NotificationsService -> Store -> No provider for Store" (NotificationsService is providedIn root and depends on the ngrx Store, which the isolated TestBeds don't set up). Provide the existing NotificationsServiceStub in each of the four specs. Verified locally: those 4 spec files now run 54/54 SUCCESS (were 54 FAILED). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: remove reproduction media from the code branch The before/after clips are now hosted on the separate 'pr-repro-media' branch and referenced from this PR's description, so they are no longer part of the code diff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: remove reproduction media from the code branch The before/after clips are now hosted on the separate 'pr-repro-media' branch and referenced from this PR's description, so they are no longer part of the code diff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(item-page): load DOI resolver from backend config instead of hard-coding it The DOI field decides which dc.identifier.uri values are DOIs by matching the DOI resolver base URL. That URL is a backend setting (identifier.doi.resolver), so read it from the REST config endpoint instead of hard-coding "https://doi.org" in the UI. - MetadataUriValuesComponent now fetches identifier.doi.resolver via ConfigurationDataService on init (only in doiField mode) and uses it in doiValues. Falls back to the DSpace default https://doi.org (DOIServiceImpl#RESOLVER_DEFAULT) when the property is not exposed/defined, so it stays correct without any backend change. - Added a spec proving a value matching a custom configured resolver is treated as a DOI, and provided ConfigurationDataService stubs to the dependent specs (item-page-uri-field, publication, untyped-item). Requires the backend to expose identifier.doi.resolver in rest.properties.exposed (separate backend PR); until then the frontend uses the default resolver. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(item-page): show "DOI registration in progress" while a DOI is pending Instead of showing an empty DOI ("Persistent Identifier") field while the DOI is still queued for registration by the scheduled task, show a "DOI registration in progress" message. Once registration completes and the resolver value appears in dc.identifier.uri, the DOI link is shown instead. - metadata-uri-values: in doiField mode, render the item.page.doi.pending message when there is no DOI value yet (doiValues is empty). - add the item.page.doi.pending i18n key to the datashare theme and core en.json5. - specs: assert the message is shown when pending / no identifier, and NOT shown once a DOI is present. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Juraj Roka <95219754+jr-rk@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: milanmajchrak <minptai7@gmail.com>
6 participants
References
Relates to:
Description
With this PR we introduce a new menu voice in the item page menu to access the editing in submission form, even if archived.
the old menu voice for editing has been renamed to "Administer" to differentiate the different set of functionalities behind the menu.
The new menu will allow to open a submission form populated with the existing metadata for further enrichment of the item.
Along this feature, the possibility to pick wich form to use for each collection submission has been introduced:
This PR brings also a new configuration for the MyDspace page, called otherworkspace , which allow to have a shared workspace accessible to administrators of collection where they will be placed, to other Eperson according to a configurable set of metadata, other than to their submitter.
Along the editing changes, this PR brings also the possibility to define security levels for the metadata, this require a rest config as follows, inside metadata-security.cfg:
metadatavalue.visibility.settings = [0 1 2]
metadatavalue.visibility.Person.settings = [0 1]
metadatavalue.visibility.Person.dc.date.available.settings = [0 1]
as you can see is possible to have three different level values and is possible to define them globally, based on entity type or based on the couple entity type/metadata.
Configuration lookup follows a fallback logic, if for a given metadata, no value is defined, default metadata security settings for the entity (metadatavalue.visibility..settings) applies. If neither at entity level metadata security is defined, default metadatavalue.visibility.settings field value is used.
A null value means that for a metadata, or its fallback, no security rules must be proposed to the submitter or to the editor.
Out of the box, CRIS has 3 different security levels:
level 0 (Public): metadata value is available to all users, even Anonymous in case entity is available to them
level 1 (Trusted): metadata value is available only to logged in users members of a defined group, named “Trusted”, as a prerequisite, this group must be available in DSpace-CRIS 7 installation
level 2 (Admin and Owner): metadata value is available only to users belonging to the “Administrators” group or to the owner of the DSpace-CRIS 7 entity.
On the UI a toggle representing each value will be diplayed in submission or in administrative view:
The effectivness of the security level is although not yet visible in this PR, as it will require the CRIS custom layout feature that will be made available with DSpace/DSpace#11779.
Once the layout feature is ported the metadata will be visible based on the security level and the user permission.
List of changes in this PR:
Adapted collection form for submission definition and entity type selection.
Added new way to edit archived item in submission style.
Implemented possibility to define security levels on metadata.
Added possibility to define a shared workspace.
There are currently some failure on the e2e tests, due to missing rest changes on the test instance; the tests expected to fail are my-dspace.cy.ts and submission.cy.ts.
To review the e2e test locally the linked rest PR is needed.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.