DS-4515: Start new submission via Search or ID Lookup (i.e. submit external source)

[sourcedump]

References

• Link to JIRA DS-4515
• Fixes Start new submission by via Search or ID lookup (Live Import) #720
• Depends on Collection dropdown on MyDSpace & support enable/disable in Submission UI #708
• REST API: DS-4515 - Start new submission via Search or ID Lookup (using Live Import) DSpace#2824

Description

Pull request to merge the item submission from external source into the master branch. This feature includes the ability to search and submit an item whose metadata comes from an external source such as Pubmed.

Instructions for Reviewers

List of changes in this PR:

• A new button in MyDspace page ('Import from external source') that starts the import process. It leads to the page at '/import-external';
• A new page at '/import-external' where it's possible to submit a search to an external source;
• A new modal to preview the external source item metadata and run the import process;
• The most important part of the code is contained in 'src/app/+import-external-page' and 'src/app/submission/import-external'.

NB: from the -collection choice modal- onwards, the features are part of the Pull 708.

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 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 for any bug fixes, improvements or new features. A few reminders about what constitutes good tests:
  • Include tests for different user types (if behavior differs), including: (1) Anonymous user, (2) Logged in user (non-admin), and (3) Administrator.
  • Include tests for error scenarios, e.g. when errors/warnings should appear (or buttons should be disabled).
  • For bug fixes, include a test that reproduces the bug and proves it is fixed. For clarity, it may be useful to provide the test in a separate commit from the bug fix.
• 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.

[tdonohue]

@sourcedump and/or @atarix83 : Can we provide additional testing instructions for this PR? It looks like our PR template was not followed in the description above. Does this PR require any changes to the REST API, or is this able to run on latest master?

[sourcedump]

@tdonohue, sorry, my bad, for some reason I've missed the 'template' for the pull request. I'll write the instruction asap.

[lgtm-com]

This pull request introduces 4 alerts and fixes 1 when merging 9ada61d into 5cef15e - view on LGTM.com

new alerts:

• 4 for Unused variable, import, function or class

fixed alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 3 alerts and fixes 1 when merging 97b4b90 into bb70591 - view on LGTM.com

new alerts:

• 3 for Unused variable, import, function or class

fixed alerts:

• 1 for Unused variable, import, function or class

[atarix83]

@tdonohue I fixed merge with main branch, now it should work, could you try?
Currently the only external provider that seems to work is orcid, even if you try over official demo rest

[lgtm-com]

This pull request introduces 1 alert when merging 4779dd2 into 1a46031 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[tdonohue]

@atarix83 and @sourcedump : I've retested this today. The build now works (with no errors) when using yarn start. However, when testing with DSpace/DSpace#2824 as the backend, I cannot get any of the providers (including ORCID) to return results to the UI.

It's worth noting though that the REST API in #2824 works fine, so making ORCID REST requests like http://localhost:8080/server/api/integration/externalsources/orcidV2/entries?query=Donohue or even PubMed REST requests like http://localhost:8080/server/api/integration/externalsources/pubmed/entries?query=mathematics work perfectly OK. It's just that the UI doesn't seem to be making that same request?

[atarix83]

@tdonohue it seems a side effect of the problem described here #819

[sourcedump]

It's worth noting though that the REST API in #2824 works fine, so making ORCID REST requests like http://localhost:8080/server/api/integration/externalsources/orcidV2/entries?query=Donohue or even PubMed REST requests like http://localhost:8080/server/api/integration/externalsources/pubmed/entries?query=mathematics work perfectly OK. It's just that the UI doesn't seem to be making that same request?

Hello @tdonohue, I've made a new sync with the main branch to resolve conflicts and tested again the external import. I don't find any problem with the queries to the REST's endpoints. Ex.: for ORCID:

and for Pubmed:

As you can see the requests are well formed.

[lgtm-com]

This pull request introduces 1 alert when merging 25d912b into f1db0c0 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[tdonohue]

@sourcedump : It looks like recent changes to this PR have decreased the Code Coverage by over 52%? I'm not sure why that would be, as it almost sounds like it's now skipping a large number of tests.

Any ideas from you or maybe @artlowel perhaps? We cannot merge this until we determine how to correct the code coverage.

[lgtm-com]

This pull request introduces 1 alert when merging 94b4115 into cc618eb - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging 67bd19a into cc618eb - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging ee2f11f into cc618eb - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[tdonohue]

NOTE FOR REVIEWERS: As noted in today's meeting, this PR is actually passing Travis CI tests (even though it displays a red ❌). Travis attempted to run its tests twice & the successful tests can be seen at https://github.com/DSpace/dspace-angular/pull/715/checks?check_run_id=1032397599

[tdonohue]

👍 @sourcedump and @atarix83 : Retested this today, and it now works properly (with the DSpace/DSpace#2824 backend). I'm not able to reproduce past errors. While some external providers (Sherpa Journals/Publishes primarily) are not returning results, it looks like a backend issue...as the REST request is correct overall.

ONE MINOR ISSUE: I did notice in looking at the REST requests, that the query param is always sent twice in each request, as the first and last querystring param. So, the REST requests look like this: http://localhost:8080/server/api/integration/externalsources/[source]/entries?query=&sort=score,DESC&page=0&size=10&query=[actual query]

If you look closely, the first param is an empty query...and the last is the actual query. This doesn't seem to cause behavior issues, but it does make the requests look odd. So, ideally, if this is easy to fix, we may want to solve it before merging.

[atarix83]

@tdonohue
I found out the first query param comes from entries link on REST side.
you can see here :
https://dspace7.4science.cloud/server/#https://dspace7.4science.cloud/server/api/integration/externalsources/orcidV2?endpointMap

the entries link is

"entries": {
      "href": "https://dspace7.4science.cloud/server/api/integration/externalsources/orcidV2/entries?query="
    }

I think it should be resolved on rest side, do you agree?

[tdonohue]

@atarix83 : Yes, I agree with you. Let's resolve that extra query parameter on the REST side. I'll create a ticket for it. So, we can move forward this PR as-is. Thanks for digging into the problem!

[tdonohue]

Merging with 3 approvals. (NOTE: The red ❌ on this PR is not accurate. Travis strangely built this PR twice...it succeeded once, and then the second time GitHub thinks there was an error, even though clicking through the links shows it also succeeded).