Add submission section for item embargo #1475

atarix83 · 2021-12-23T18:42:45Z

References

Description

This PR add a new submission section in order to deal with the access condition for the item during the submission

Instructions for Reviewers

Create a new submission and test the new submission section

List of changes in this PR:

Created a new submission section component for the access conditions
Created a new config service for retrieving access conditions configuration

Include guidance for how to test or review your PR. This may include: steps to reproduce a bug, screenshots or description of a new feature, or reasons behind specific changes.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
My PR passes TSLint validation using yarn run lint
My PR doesn't introduce circular dependencies
My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

lgtm-com · 2021-12-23T18:51:00Z

This pull request introduces 4 alerts when merging bb64058 into ba268d4 - view on LGTM.com

new alerts:

4 for Unused variable, import, function or class

tdonohue

@atarix83 : Thanks for the hard work on this. Mostly this "works" and the code seems reasonable (though see inline comments below for minor suggestions). That said, I ran into a few obvious bugs.

First, this new "Item Access Conditions" section doesn't work in Production mode (yarn start). It only works in Dev Mode (yarn start:dev). In Production mode, the UI section is empty:
Sometimes, the access conditions end up overlapping oddly. I seem to be able to reliably reproduce this by doing the following: (1) Add one or more Item access conditions, (2) upload a file, (3) click Edit on the file, (4) close the popup modal. Then you see this odd alignment/overlap:
After submitting an Item with two access conditions (as I can create multiple), I found that only one of the two access conditions are inherited to the "ORIGINAL" bundle or the bitstream in that bundle. Here's what it looks like (In case, I added both an embargo & openaccess, which I know isn't really "valid". But, in the below screenshot, you'll see the Item & LICENSE bundle get both access conditions, while the Bitstream & ORIGINAL Bundle only get the embargo)
- I'm not sure if this is a UI or backend error yet, as I haven't dug into it.
I think the "Discoverable" checkbox needs help text. See my suggested text inline below.
There is an LGTM alert above that needs resolving.

Overall, this code seems to work (in Dev mode only though) and looks good.

src/app/core/config/models/config-submission-access.model.ts

src/app/core/submission/models/submission-accesses.model.ts

src/app/core/submission/models/submission-accesses.resource-type.ts

src/app/core/submission/models/submission-accesses.model.ts

src/app/submission/sections/accesses/section-accesses.component.ts

src/app/submission/sections/accesses/section-accesses.service.ts

src/assets/i18n/en.json5

…e submission.module

# Conflicts: # src/app/core/core.module.ts # src/app/submission/objects/submission-objects.effects.ts # src/app/submission/submission.module.ts

atarix83 · 2022-01-24T19:37:59Z

@tdonohue

feedback addressed

lgtm-com · 2022-01-24T19:44:18Z

This pull request introduces 2 alerts when merging 89c9e9a into 4fdd3b8 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

tdonohue

@atarix83 : I re-tested this today, and it's working better than before, but I'm still seeing a few issues:

(New Issue) After applying this PR, the "Edit Bitstream" button in the Submission form no longer works (Upload a file, and click "Edit"). This error appears in DevTools Console:

core.js:6210 ERROR NullInjectorError: R3InjectorError(e)[e -> e -> e]: 
  NullInjectorError: No provider for e!
    at aa.get (core.js:11116:27)
    at ga.get (core.js:11283:33)
    at ga.get (core.js:11283:33)
    at ga.get (core.js:11283:33)
    at qm.get (core.js:25337:33)
    at Object.get (core.js:25051:35)
    at ln (core.js:3389:39)
    at un (core.js:3501:12)
    at Module.pl (core.js:14707:12)
    at Ht.e.ɵfac [as factory] (section-upload-file-edit.component.ts:61:54)

I still can reproduce the previously reported issue (see bullet 3 in that previous review) where when you add multiple access permissions to an Item, only the first one is inherited to the ORIGINAL Bundle and any Bitstreams in that Bundle. (However, other bundles, like LICENSE properly inherit all access permissions.)
- I'm not yet sure whether this is a frontend or backend bug. But, it's unexpected behavior, as you don't know which access permissions will be applied to the ORIGINAL Bitstreams...it seems to be whichever access permission you apply first.
- We should either find a way to fix this bug, or potentially disable the ability to add multiple access conditions at the Item level, as it appears we have some code which expects there to only be one access condition.
Finally, there are new minor LGTM issues listed above.

Overall, this functionality works, but it's still got a few bugs to fix.

atarix83 · 2022-01-25T09:44:21Z

@tdonohue

(New Issue) After applying this PR, the "Edit Bitstream" button in the Submission form no longer works (Upload a file, and click "Edit"). This error appears in DevTools Console:

this is already present in the main, however now it's fixed

I still can reproduce the previously reported issue (see bullet 3 in that previous review) where when you add multiple access permissions to an Item, only the first one is inherited to the ORIGINAL Bundle and any Bitstreams in that Bundle. (However, other bundles, like LICENSE properly inherit all access permissions.)
I'm not yet sure whether this is a frontend or backend bug. But, it's unexpected behavior, as you don't know which access permissions will be applied to the ORIGINAL Bitstreams...it seems to be whichever access permission you apply first.
We should either find a way to fix this bug, or potentially disable the ability to add multiple access conditions at the Item level, as it appears we have some code which expects there to only be one access condition.

this is an issue on backend side, we'are going to fix in the DSpace/DSpace#8091

Finally, there are new minor LGTM issues listed above.

fixed

tdonohue

@atarix83 : Thanks for the quick updates! I re-tested today & can verify now that all issues are fixed, except the one you said you'll fix in the backend PR.

So, I'm basically a +1 on this PR, but I want to give it one final test once the backend is ready to re-test again.

@artlowel : It'd be good to get your feedback on this PR as well, so that we can move it forward quickly once the backend is completed.

tdonohue

👍 Thanks @atarix83 ! Adding my official +1, as I just re-tested this with the updated backend PR and all my feedback has been resolved (the backend PR updates fixed the policy inheritance issue I was seeing).

artlowel

Thanks @atarix83!

It works well for the most part

The biggest issue I found however is that these access conditions have drag and drop handles, but don't appear to be draggable.

When you first choose embargo, fill out an "access from" date, and then select lease. The access from datepicker is disabled but not cleared. The same thing is possible if you do it the other way around. It doesn't affect the actual permissions if you save though, so it's a bit confusing but not a major issue

The alignment of the delete button looks off. The same goes for the date fields if there's an error message

The error issue should be easy to fix by using d-flex and align-items-start on the div containing all the fields. The delete button is shared with all the other submission fields though so that might be tricky to fix for this without breaking it elsewhere. Perhaps a background color, some padding and a border can make it clearer what the button aligns with? e.g.:

I would consider the drag handle on a field that isn't draggable a problematic issue, the others can be dealt with later if there isn't a quick easy fix

artlowel · 2022-01-20T13:25:13Z

src/app/shared/testing/form-event.stub.ts

+        defaultPrevented: false,
+        eventPhase: 0,
+        isTrusted: true,
+        path: ['input#accessCondition-0-endDate.form-control.ng-touched.ng-dirty.ng-valid', 'div.input-group.ng-touched.ng-valid.ng-pristine', 'ds-dynamic-date-picker-inline.ng-star-inserted, div, div, div, div.ng-touched.ng-valid.ng-pristine, ds-dynamic-form-control-container.col-6.ng-star-inserted, div#accessCondition-0-accessConditionGroup.form-row.ng-star-inserted.ng-touched.ng-valid.ng-pristine, ds-dynamic-form-group.ng-star-inserted, div, div, div, div.pl-1.pr-1.ng-touched.ng-valid.ng-pristine, ds-dynamic-form-control-container.form-group.flex-fill.access-condition-group.ng-star-inserted.ng-to…, div.cdk-drag.cdk-drag-handle.form-row.cdk-drag-disabled.ng-star-inserted.ng-touched.ng-valid.ng-pris…, div#cdk-drop-list-5.cdk-drop-list, div#accessCondition.ng-star-inserted.ng-touched.ng-valid.ng-pristine, ds-dynamic-form-array.ng-star-inserted, div, div, div, div.form-group.ng-touched.ng-valid.ng-pristine, ds-dynamic-form-control-container.ng-star-inserted, ds-dynamic-form.ng-touched.ng-valid.ng-pristine, form.form-horizontal.ng-touched.ng-valid.ng-pristine, div.container-fluid, ds-form.ng-star-inserted, ds-section-accesses.ng-star-inserted, div#sectionContent_AccessConditionDefaultConfiguration.ng-star-inserted, div.card-body, div#AccessConditionDefaultConfiguration.collapse.show.ng-star-inserted, div.card.ng-star-inserted, ngb-accordion.accordion, div#section_AccessConditionDefaultConfiguration.section-focus, ds-submission-section-container.ng-star-inserted, div.submission-form-content, div.container-fluid, ds-submission-form, div.submission-submit-container, ds-submission-edit.ng-star-inserted, ds-themed-submission-edit.ng-star-inserted, div.ng-tns-c392-0, main.main-content.ng-tns-c392-0, div.inner-wrapper.ng-tns-c392-0.ng-trigger.ng-trigger-slideSidebarPadding, div.outer-wrapper.ng-tns-c392-0.ng-star-inserted, ds-root.ng-tns-c392-0.ng-star-inserted, ds-themed-root, ds-app, body, html.wf-droidsans-n4-active.wf-active, document, Window'],


This looks like it could break the moment even the slightest thing changes about the feature later. I assume the exact path isn't really that important to test the functionality. Is there any way to make this more generic?

@artlowel removed

benbosman · 2022-01-27T13:36:59Z

While testing the backend, I noticed I was unable to remove an access policy if there's only one.
How can this be removed using the UI, ensuring the collection default policies will be used again (the default)

atarix83 · 2022-01-27T15:04:36Z

@artlowel @benbosman

The issues reported are present also in the edit bitstream access conditions form, since the implementations are similar. So, if you are agree, I'd say to create a new issue and resolved in afollow-up PR for both.

tdonohue · 2022-01-27T16:44:20Z

@atarix83 , @artlowel and @benbosman : I've verified that @atarix83 is mostly correct. Some of these flaws exist in the current "Edit Bitstream" form (for Bitstream embargo functionality), but a few do not. I've broken them out below, including workarounds where possible.

Flaws that exist in both Item & Bitstream Embargo forms. (These should be moved to 7.3, and I'll create a ticket for each & flag as high priority)

Drag & drop handles appear, but they are not draggable/reorderable. Either it should work or the drag & drop option should not appear. (No workaround, but these handles are just misleading / usability issue. No errors occur if you try to use them.) Created ticket Embargo / Access Conditions appear draggable/reorderable, but they are not. #1504
If you select a single "Access condition type" you cannot delete or remove it (delete button is disabled). Created ticket Embargo / Access Condition cannot be deleted in Submission form if only one remains #1505 . NOTE, there is a workaround:
- Workaround is to click "+Add more" which adds an empty Access condition. Then you can delete the previous access condition, leaving only the empty one. (It's not ideal, but it's better than having to start a new submission)

Flaws that ONLY exist in the Item Embargo form: (These should ideally be fixed in this PR, @atarix83 )

Delete button is misaligned (It appears for me on first load of the form). @artlowel reported with a screenshot, and I see the same thing
When an error is displayed, the "Grant access" fields become misaligned. @artlowel reported with a screenshot & I see the same thing
Issue reported by @artlowel where if you select "Embargo" and enter in a "Grant access from" date...then switch to "lease", that "Grant access from" date is only disabled & not cleared. I can reproduce this and it doesn't occur on the Bitstream Embargo form (on that form, the previous selected date is cleared). Here's a screenshot of the improper behavior... The "Grant access from" date is still there even though I've switched to "lease"

My recommendation would be for @atarix83 to do the minor cleanup to get this form working the same as the Bitstream embargo form. The other two bugs I'll create tickets for & link up to this comment once they are created.

atarix83 · 2022-01-28T10:23:41Z

@tdonohue @artlowel @benbosman

here the list of issues I have fixed :

The alignment of the delete button looks good now
problem with drag and drop icon is resolved for both item and bitstream access conditions form, now it's no longer showed
problem with aligment when errors are showed existed on both forms and I've resolved by adding an hint for the form's fields (like in the previous screenshot), I didn't find other ways
problem with date value when switching through access conditions is resolved

artlowel

Thanks @atarix83!

It looks a lot better, and I don't see any side effects on other fields.

One more very minor inline comment:

artlowel · 2022-01-28T12:12:29Z

.../shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.scss

+.drag-disable {
+  visibility: hidden !important;
+  &:hover, &:focus {
+    cursor: grab;


Perhaps this should be cursor: default? Currently you still see the hand when hovering where the drag handle would be

tdonohue

👍 Looks good to me too, @atarix83 !

I've verified that the previous feedback has been addressed. Also verified that this fixes #1504 (by hiding those draggable options)

As a sidenote, I'm still seeing the hand icon appear (instead of cursor or pointer) when you hover over the "draggable" section. It's very minor though, so if we need to fix it later, no big deal. Either way, this PR will be merged as soon as the backend PRs are re-reviewed and ready. Thanks!

[DSC-1468] port of GLAM-353 entity type dropdown is now organized in alphabetical order Approved-by: Andrea Barbasso

rezartatis and others added 5 commits December 9, 2021 18:22

[CST-4878] Created component

31fd89a

[CST-4878] Finished working on embargo add part of form and unit testing

f04f4b4

Merge remote-tracking branch 'origin/main' into CST-4506_item_embargo

ebbae16

[CST-4506] fix absolute paths

db25e27

[CST-4506] fix test

bb64058

tdonohue assigned atarix83 Dec 23, 2021

tdonohue added high priority new feature component: submission labels Dec 23, 2021

tdonohue added this to the 7.2 milestone Dec 23, 2021

tdonohue requested review from artlowel and tdonohue December 23, 2021 19:17

tdonohue requested changes Jan 11, 2022

View reviewed changes

[CST-4505] Fixed canChangeDiscoverable managment and unit testing

768c7f8

github-actions bot added the merge conflict label Jan 13, 2022

tdonohue mentioned this pull request Jan 19, 2022

Embargo Item Metadata DSpace/DSpace#8091

Merged

6 tasks

atarix83 added 4 commits January 20, 2022 18:29

[CST-4506] Optimize submission sections components declarations in th…

b81cc10

…e submission.module

[CST-4506] fix SubmissionSectionFormComponent name

532d8fb

[CST-4506] Address feedback

973ceb3

Merge remote-tracking branch 'origin/main' into CST-4506_item_embargo

89c9e9a

# Conflicts: # src/app/core/core.module.ts # src/app/submission/objects/submission-objects.effects.ts # src/app/submission/submission.module.ts

atarix83 requested a review from tdonohue January 24, 2022 19:37

tdonohue removed the merge conflict label Jan 24, 2022

tdonohue requested changes Jan 24, 2022

View reviewed changes

atarix83 added 2 commits January 25, 2022 10:40

[CST-4506] Fix issue with submission bitstream edit

04c8cb7

[CST-4506] Fix LGTM issues

db3d760

tdonohue self-requested a review January 25, 2022 15:07

tdonohue reviewed Jan 25, 2022

View reviewed changes

tdonohue self-requested a review January 25, 2022 19:54

tdonohue approved these changes Jan 25, 2022

View reviewed changes

artlowel requested changes Jan 27, 2022

View reviewed changes

[CST-4506] remove unused mock event properties

42cc68f

This was referenced Jan 27, 2022

Embargo / Access Conditions appear draggable/reorderable, but they are not. #1504

Closed

Embargo / Access Condition cannot be deleted in Submission form if only one remains #1505

Open

atarix83 added 7 commits January 27, 2022 18:58

[CST-4506] fix issue with delete button alignment

8a668a5

[CST-4506] Disable drag and drop for access conditions form group

9019b80

[CST-4506] Add i18n labels for item access conditions form

4ca2e9a

[CST-4506] Fix bitstream access condition form layout

232baf5

[CST-4506] Fix item access condition remove operation

6c720b8

[CST-4506] Fix item access condition form layout

f991b4e

[CST-4506] Clean date fields when switching through access conditions

4e5bf6e

atarix83 requested a review from artlowel January 28, 2022 11:53

artlowel approved these changes Jan 28, 2022

View reviewed changes

[CST-4506] use default cursor when drag and drop is disabled

67d6c6f

tdonohue self-requested a review January 28, 2022 15:33

tdonohue approved these changes Jan 28, 2022

View reviewed changes

tdonohue merged commit 0fbd48e into DSpace:main Jan 31, 2022

tdonohue mentioned this pull request Jan 31, 2022

Make an Item private (from Submission UI) #768

Closed

abollini deleted the CST-4506_item_embargo branch February 1, 2022 08:22

4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Jun 3, 2024

Merged in DSC-1468 (pull request DSpace#1475)

e531378

[DSC-1468] port of GLAM-353 entity type dropdown is now organized in alphabetical order Approved-by: Andrea Barbasso

Add submission section for item embargo #1475

Add submission section for item embargo #1475

Uh oh!

Conversation

atarix83 commented Dec 23, 2021 • edited by tdonohue Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Description

Instructions for Reviewers

Checklist

Uh oh!

lgtm-com bot commented Dec 23, 2021

Uh oh!

tdonohue left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atarix83 commented Jan 24, 2022

Uh oh!

lgtm-com bot commented Jan 24, 2022

Uh oh!

tdonohue left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atarix83 commented Jan 25, 2022

Uh oh!

tdonohue left a comment

Choose a reason for hiding this comment

Uh oh!

tdonohue left a comment

Choose a reason for hiding this comment

Uh oh!

artlowel left a comment

Choose a reason for hiding this comment

Uh oh!

artlowel Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

atarix83 Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

benbosman commented Jan 27, 2022

Uh oh!

atarix83 commented Jan 27, 2022

Uh oh!

tdonohue commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atarix83 commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artlowel left a comment

Choose a reason for hiding this comment

Uh oh!

artlowel Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

atarix83 Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

tdonohue left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

atarix83 commented Dec 23, 2021 •

edited by tdonohue

Loading

tdonohue left a comment •

edited

Loading

tdonohue left a comment •

edited

Loading

tdonohue commented Jan 27, 2022 •

edited

Loading

atarix83 commented Jan 28, 2022 •

edited

Loading

tdonohue left a comment •

edited

Loading