Show Identifiers section

[kshepherd]

Attribution: Donated by The Library Code with the support of Technische Universität Hamburg and Technische Informationsbibliothek

References

• Requires DSpace/DSpace#8423

Description

This pull request adds the Angular UI implementation of DSpace/DSpace#8423

List of changes in this PR:

• Model and component for Show Identifiers section
• i18n text for template
• Unit tests for component

For test instructions and notes, see DSpace/DSpace#8423

Checklist

• [x] 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.
• [x] My PR passes TSLint validation using yarn run lint
• [x] My PR doesn't introduce circular dependencies
• [x] My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• [x] My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.

[lgtm-com[bot]]

This pull request introduces 2 alerts when merging 4aa3214972a83b5cbffc1f68df96e4cd9636afb7 into 4da017a0ee051a361fac44482c20a05ff5253146 - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request introduces 2 alerts when merging f3a1236b02cf75f184db92bbfe865ac9434423bb into 4da017a0ee051a361fac44482c20a05ff5253146 - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request introduces 10 alerts when merging e88aa9ef5fd949f7a6c6dd13dfa24a348b96f70e into c1f6993144c10ba81a95943e87696092dbcdb286 - view on LGTM.com

new alerts:

• 10 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request introduces 12 alerts when merging 606a9f5ad570c4f581fc0c0081bec61f2cdf69b2 into c1f6993144c10ba81a95943e87696092dbcdb286 - view on LGTM.com

new alerts:

• 12 for Unused variable, import, function or class

[tdonohue]

@kshepherd : This PR is also failing tests & it has LGTM alerts needing resolution. Could you see if they are easy to resolve? Same thing here...we are starting to run out of time to get these reviewed in time for 7.4

[lgtm-com[bot]]

This pull request introduces 13 alerts when merging d0cb35662bc2c72923958945eb4a6f2741741c11 into a2f8a5ccfa114fc766b7fa6fd1a6d199cb152b4b - view on LGTM.com

new alerts:

• 13 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request introduces 13 alerts when merging faa882e524bb56c28bdc4d3a536753be5c3be63f into a2f8a5ccfa114fc766b7fa6fd1a6d199cb152b4b - view on LGTM.com

new alerts:

• 13 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request introduces 9 alerts when merging 2a33dd6590ea35f51727230c95292bf5c6069cac into a2f8a5ccfa114fc766b7fa6fd1a6d199cb152b4b - view on LGTM.com

new alerts:

• 9 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request introduces 9 alerts when merging e8cf26b2897ba41de517385953f9ba91ffc9b879 into 927dfdad1c9ddcf9c7dfcb12d0e7a4858bd862bb - view on LGTM.com

new alerts:

• 9 for Unused variable, import, function or class

[kshepherd]

@tdonohue there are some tests still failing with this and the DSpace PR, as well as some possible tidy-up needed to the way I wired up request GET and POST methods for the /identifiers path + link. Some devs from 4Science are helping out by looking at this (both PRs). I appreciate this is a big change and time is running short for 7.4. If a reschedule is needed to meet deadlines for priority 7.4 release tasks then that is completely understandable.

Documentation to help review / test and forming draft of 7.x doco: https://wiki.lyrasis.org/display/~kshepherd/Identifier+submission+section%2C+pre-registration

[lgtm-com[bot]]

This pull request introduces 9 alerts when merging 9178eee84271d7cbeb664962ece6608e1211cdb9 into c876055855b1818515ffce77e7ccb5ea955cba29 - view on LGTM.com

new alerts:

• 9 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request introduces 9 alerts when merging 8a9a31062d8375bda1ef27f17dd984246c170622 into dc4b4ffe4d16daf43b6c7706058b104ed8c25beb - view on LGTM.com

new alerts:

• 9 for Unused variable, import, function or class

[kshepherd]

The 2 failed 16.x tests are related to failed e2e tests, a connection timeout trying to get search or mydspace config for workspaceitems. I don't know how this would be related, or if there's a way for me to easily trace any backend problems. Any tips?

Update: Fixed with a re-run

[github-actions[bot]]

Hi @kshepherd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

---

This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

[kshepherd]

@tdonohue I believe I have addressed your concerns here:

1. As per REST Contract discussion, the data model for the identifiers returned in the identifiers REST endpoint has been made more uniform, and matches the submission section data model for identifiers, too
2. Simplified display means an identifier will only be displayed in the Submission Section if it exists. There is no longer any placeholder "no DOI has been..." message in either case.
3. The "Register DOI" button now performs a more generic 'registerIdentifier' operation which takes a required query parameter 'type'. Currently only type=doi is implemented
4. Unit and integration tests are updated to fix any problems and support this behaviour

Documentation: https://wiki.lyrasis.org/display/~kshepherd/Identifier+submission+section%2C+pre-registration

[kshepherd]

@atarix83 I have pushed changes that tidy up the names, and also use the new endpoints as per the DSpace REST refactor for this PR. I have not successfully rewritten the nested subscription code yet. I think I know the technique I want to use (subscribe to showRegister$ so that I can detect changes in that logic, and essentially call .next() on operations) but just the way it's originally written, where the auth checks really weren't designed to be read more than once (i think?) is making it tricky for me to design properly. Should have more progress on that soon, I hope. Any tips are welcome.

I expect to see some lint / test errors as I haven't fixed all of those yet, I also have some comments / doc to add. Pushing what I have now just to show progress,

[github-actions[bot]]

Hi @kshepherd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

---

This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

[pnbecker]

@tdonohue If @atarix83 agrees that @kshepherd explaination above is acceptable for him this PR is ready to be merged. As mentioned in the backend, I tested it again without and with DOIs activated. It works like a charm.

[mirkoscherf]

"curation-task.task.registerdoi.label" has been changed here to "curation-task.task.register-doi.label" and now the label text is not displayed properly as can be seen on demo7.dspace.org.

[tdonohue]

@mirkoscherf : Please feel free to log a bug ticket or send us a PR to fix the issue. It's not recommended to continue discussions after a PR is merged/accepted, simply because we cannot reopen the code for additional editing (as it's already merged into our codebase). Any new fixes need to come in a new PR. Thanks!

[mirkoscherf]

@tdonohue Alright, will do!