[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. |
AdamF42
force-pushed
the
task/main/DURACOM-453
branch
from
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
…le, fix missing config in field models
|
Hi @KevinVdV , many thanks for the feedback, much appreciated. I have fixed the issue 2 and 3, now it should be possible to set the security level also on date fields and in the admin page there won't be an empty column. Regarding the issue 1 we are following the same logic of other columns, like the language, which is always shown even if empty so I am not sure any action is necessary, I see although the difference from the other columns as this one is configurable field by field. For the issue 4 I agree the namig might not be as intuitive, we can decide upon a better one considering although that we have multiple edit modes based on the user role and the labels in question are the following: "menu.section.FULL": "Edit all the details", "menu.section.OWNER": "Edit", "menu.section.DIRECTOR": "Edit", "menu.section.INVESTIGATOR": "Edit", |
|
@FrancescoMolinaro : I've noticed this PR has resulted in several e2e test failures. Are you working on cleaning those up? (It appears they've existed for some time) |
|
Hi @tdonohue , the e2e are failing because some of the new endpoints are missing on the test instance, in specific we miss the endpoint for security settings and the new projections added for the workspace item. For the remaining failing tests, my-dspace.cy.ts and submission.cy.ts , I think would be better to wait for the rest PR to be merged, as the submission is impacted by the changes in this PR and would be best to test the whole flow correctly. In alternative I could adapt the loading mechanism of the submission to not necessarely expect security settings and to use a fallback call in case the workspace item fails to load with the newly added embed parameters and projections. I would really appreciate to hear your thoughts on this matter and thanks for the feedback. |
|
@FrancescoMolinaro : Sorry, I wasn't aware the tests were failing simply because they depend on the REST API. Since that's the case, I recommend noting those expected test failures in the description of this PR. The best solution would then be for a reviewer/tester to run the tests manually with the REST API updates installed to verify they all pass locally. Then, once both PRs are approved, we can just ensure the REST API PR is merged first, so that we can trigger a re-run of the tests here to doublecheck that they also pass in our CI process via GitHub. |
|
Hi @tdonohue no problem at all, I forgot to highlight this problem in the description, I have now noted the failures so that is clear why there are failures, many thanks for the feedback and the suggestion. |
|
Hi @FrancescoMolinaro, |
tdonohue
moved this from 🙋 Needs Reviewers Assigned
to 👀 Under Review
in DSpace 10.0 Release
|
@FrancescoMolinaro : Could you quickly rebase this PR? It has merge conflicts. Thanks! |
|
Hi @tdonohue , conflicts have been resolved, this should be now aligned with main. |
|
Hi @FrancescoMolinaro, |
|
@FrancescoMolinaro : Because #5134 was just merged, this PR will need to be rebased on |
|
Hi @tdonohue , this PR should be again aligned. |
tdonohue
added
component: submission
component: Item
tdonohue
left a comment
tdonohue
left a comment
There was a problem hiding this comment.
@FrancescoMolinaro : Thanks for this PR! I gave it a code review today. Mostly the code looks great, but I've found some minor things that I'd recommend addressing (mostly additional code comments / TypeDocs).
If any of these are too complex to address now, we can always create a follow-up ticket to address them later. However, I'm hopeful this will not be too complex to cleanup (even though I did add 21 inline comments below).
I have not yet tested this feature. But, I plan to still do so today. I'll submit any testing feedback as a separate comment/review.
|
|
||
| "menu.section.workflow": "Administer Workflow", | ||
|
|
||
| "menu.section.FULL": "Edit all the details", |
There was a problem hiding this comment.
I think we should rename this to simply be "Edit" or "Edit Item".
I'd also recommend this be displayed first in the menu, directly above "Administer", as I think this should be the recommended way to edit item metadata/files.
There was a problem hiding this comment.
I noticed just now that @KevinVdV had a similar comment about this menu option. I feel "Edit all the details" is misleading here because the "Administer" menu offers more details (e.g. permissions, etc) which you can edit. So, I'd rather this menu option just say "Edit" or I'm OK with something like "Edit via form" or "Edit metadata and files"
| import { ConfigObject } from './models/config.model'; | ||
|
|
||
| @Injectable({ providedIn: 'root' }) | ||
| export class SubmissionDefinitionsConfigDataService extends ConfigDataService { |
There was a problem hiding this comment.
Please add TypeDocs or code comments to describe this new data service, including the findAll() method below
| import { excludeFromEquals } from '../../utilities/equals.decorators'; | ||
|
|
||
| /** | ||
| * Describes a EditItem mode |
There was a problem hiding this comment.
Could we provide a bit more details in these TypeDocs to describe where/how EditItem mode is used?
| /** | ||
| * A model class for a EditItem. | ||
| */ | ||
| @typedObject |
There was a problem hiding this comment.
Same here. Could we provide a bit more details in these TypeDocs to describe where/how EditItem is used?
| /** | ||
| * A model class for a security configuration of metadata. | ||
| */ | ||
| @typedObject |
There was a problem hiding this comment.
Same here. Could we provide a bit more details in these TypeDocs to describe where/how these securitysettings are used?
| })); | ||
| }))); | ||
|
|
||
| removeSection$ = createEffect(() => this.actions$.pipe( |
There was a problem hiding this comment.
Please add a comment to describe this new effect, similar to all other effects in this file
| } | ||
| } | ||
|
|
||
| protected changeSecurityLevel(pathCombiner: JsonPatchOperationPathCombiner, |
There was a problem hiding this comment.
As this is a larger method, please add TypeDocs to describe its behavior
|
|
||
| "mydspace.show.workspace": "Your submissions", | ||
|
|
||
| "mydspace.show.otherworkspace": "Other Workspace Submissions", |
There was a problem hiding this comment.
Should we consider changing this text to be "Shared Submissions"? It seems like that might be a name that will make more sense to users on the MyDSpace page.
| @@ -0,0 +1,11 @@ | |||
| import { Config } from './config.interface'; | |||
|
|
|||
| export interface MetadataSecurityConfig extends Config { | |||
There was a problem hiding this comment.
Please add a basic description to both of these interfaces. It just helps make it clear which configs they reference
| value: 0, | ||
| icon: 'fa fa-globe', | ||
| color: 'green', | ||
| }, |
There was a problem hiding this comment.
These configurations and their default values should be added to config.example.yml. We also should provide a description there for how the configuration is used and what valid values exist.
Could we also consider moving this configuration under the existing item configurations? Currently, these security settings seem item specific. So, for example, they might appear like this in config.example.yml:
item:
security:
levels:
Or even under item > edit:
item:
edit:
security:
levels:
Either of those approaches would be fine with me. I just feel this shouldn't be a top-level setting because these security settings are not applicable on all types of objects.
There was a problem hiding this comment.
@FrancescoMolinaro and @AdamF42 : I've finished my initial testing of this feature. I'm just submitting my results in this frontend review even though I know some of the fixes will require backend changes. Overall, I've found this works well!
But, I have found some bugs / possible issues. If any of these are too complex to fix now, then we could create follow-up tickets for the effort:
- First, a minor thing. The "Edit all the details" menu option should have an icon next to it. As you can see in the screenshot in the description of this PR, it's missing an icon. I'd recommend using the pencil for this Edit menu, and perhaps change the "Administer" menu option to use either a wrench or gear icon (I think I have a slight preference for the wrench)
- The "Edit all the details" menu does not appear for traditional Items (i.e. Items which are not Entities). Is there a way we could support traditional items as well? Or is this only possible to do for Entities?
- When using the "Edit all the details" menu to edit an existing Item, if I upload a new file and click "Save and return", then I return to the Item page but the new file does not appear on that page. If I reload the page, then the new file appears. It appears there's a caching issue.
- If I click on "Edit all the details" menu for a Project or a Person, then the page hangs indefinitely. Behind the scenes, I see there's an error in the backend log which says the submission-process named
project-editorperson-editdoesn't exist. Could we add all the-editforms (on the backend) for all default entities to avoid this issue?- Additionally, if I create an
person-editform, I still get an error. But it says I'm now missing anadmin-person-editform (maybe because I'm logged in as an Admin?) - Alternatively, we could see if there's a way to avoid displaying the "Edit all the details" menu if this submission-process configuration is missing. Or maybe show the menu option but disable it (to make it clear it's not configured).
- Additionally, if I create an
- When creating/editing a Collection, I verified the "Submission definition" option works well. However, I wonder if we should find a way to hide all the
*-editforms because these forms are only used for editing objects.- For example, I selected the
publication-editdefinition for a collection and I noticed that the License section is then missing from the submission form for new items. So, it might be best to see if we can avoid people selecting these*-editforms as they are not meant for new submissions.
- For example, I selected the
- Finally, I tested the "Shared Workspace" options at the Collection level. They work well overall. However, I'm confused by the "Other Workspace Submissions" view on my MyDSpace. I only have a single Collection which has "Shared Workspace" enabled, but I see Items under "Other Workspace Submissions" that belong to other collections. Is this expected behavior? I expected that I'd only see Items in that menu which belong to a Collection having a "Shared Workspace".
- I tested this by creating one Collection with a "Shared Workspace" as an Admin
- Started a new submission to that collection (again as an Admin). I didn't complete it.
- Logged out and logged in as a different user who only has Submit privileges to that Collection (This user is a Collection Admin in other collections but not this new Collection)
- When I visit that user's MyDSpace, I see the following:
- On the "Your Submissions" menu, I see only one item still in the "Workspace"
- If I change to the "Other Workspace Submissions", then I see 86 items which are still in the "Workspace". I'm not sure why I'm seeing so many.
- However, I was able to verify that this user can edit/update the shared submission that was started from my Admin account.
Other than the bugs I listed above, other features appear to be working! (I've also done some initial basic testing of the metadata security settings, and they appear to be working well so far)
4 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.