dspace-angular
dspace-angular copied to clipboard
Openaire suggestions (publication claim)
References
- Related to https://github.com/DSpace/RestContract/pull/192
- Related to https://github.com/DSpace/DSpace/pull/8280
- Includes https://github.com/DSpace/dspace-angular/pull/1562
Unique code in this PR can be found via this diff: https://github.com/4Science/dspace-angular/compare/CST-5337...4Science:dspace-angular:CST-5249_suggestion
The suggestion feature requires the researcher profile feature, so it is required to:
- uncomment the dspace.object.owner section in the authority.cfg
- uncomment the property researcher-profile.entity-type = Person in the researcher-profile.cfg
The features included in this PR are the result of the OpenAIRE Call Innovation funded project "Enrich local data via the OpenAIRE Graph” awarded by 4Science (https://www.openaire.eu/open-call-winner-phase-1-4science). It provides a closer integration between DSpace and two OpenAIRE services, the Notification Broker and the OpenAIRE REST API. Detailed documentation about the aims of the project, the implementation and the configuration options is available at https://4science.github.io/oaire-eld/#/publication-claim This PR regarding only the publication claim.
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!
- [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.
- [X] 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.
This pull request introduces 27 alerts when merging a798deddf2496529e4e88257599772b4b6b738ce into 37ebe259f3642fc2b42f7186eb0fe5a74d8bab7c - view on LGTM.com
new alerts:
- 27 for Unused variable, import, function or class
This pull request introduces 16 alerts when merging de6b5332877e4b6e067800dbe6f8a0b134ddc6b8 into fae355a71372d6f3ab25e699d3e69671a5b68124 - view on LGTM.com
new alerts:
- 16 for Unused variable, import, function or class
This pull request introduces 16 alerts when merging d8e96d80904ceb9dd28a08fb60f3230d25bcee71 into fae355a71372d6f3ab25e699d3e69671a5b68124 - view on LGTM.com
new alerts:
- 16 for Unused variable, import, function or class
Hi @LucaGiamminonni, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!
Hello @tdonohue , I was going trough your requested changes and I have a doubt about point 2 of comment https://github.com/DSpace/dspace-angular/pull/1638#pullrequestreview-1851957186.
In specific the issue related to the breadcrumb. If logged as EPerson the "Publication Claim" step is missing from the breadcrumb because the page is restricted for admins.
We used the same approach that is already in place for restricted collections/communities, please find below an example taken from the "Person" page:
Restricted (no admin or no privileges for collection/community):
Not restricted:
Would this be acceptable? Otherwise to have the "Publication claim" step visible in the breadcrumb for EPerson we would have to make the page accessible also to non administrative users.
Thanks in advance for your feedback
@FrancescoMolinaro : Your suggestion is acceptable. But, I then would recommend changing the breadcrumb that is displayed.
Currently, the breadcrumb displays the person's name
However, that's not specific enough to this page. This page relates to publication claim or specifically suggestions related to publication claim.
So, I'd recommend the breadcrumb say something like:
Home * Publication Claim for Tim Donohue
(or just "Publication Claim")
Or
Home * Suggestions for Tim Donohue
(or just "Suggestions")
Or something similar to that. Having just the person's name is not detailed enough, as this page is not their Researcher Page or profile. Instead the purpose of this page is to show the user's suggestions for publication claim.
Hello @tdonohue , thanks again for the feedback, I have opted for "Suggestion for ..." so that is in line with the title of the page:
In case you find it repetitive we could change it to "Publication claim for ..."
@FrancescoMolinaro : That seems fine to me. Just wanted to make sure that a page has an appropriate "title" in the breadcrumbs.
Hi @LucaGiamminonni, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!
@frabacche and @abollini: I also tested this again from the frontend & re-reviewed the code. Overall, it is looking better, but I'm still finding a few bugs/issues.
I'd recommend calling this page "Publication Claim" (singular) as that's more similar to "Quality Assurance" (the other page under "notifications"). Also, the breadcrumbs on the "Publication Claim" page should likely say "Publication Claim" instead of "Suggestions" Having two names for this page is confusing:
When logged in as an EPerson with suggestions in "Publication Claim", the breadcrumbs disappear if you click on the link to look at your suggestions. The breadcrumbs here should likely be "Home > Publication Claim > Tim Donohue". Additionally, the title of this page says "OpenAIRE" twice. We likely could simplify it to just
Suggestions for [name] from OpenAIRE GraphI'm not sure if this is a bug, but the "DateScorer" is always returning 0 and I'm not sure how to "set a min/max date in the profile"? What does that mean? It also doesn't seem to be possible to add "education achievements" in DSpace yet. Maybe we need to fix this text?
If you are logged in as an EPerson with Suggestions, you cannot get back to the suggestions page if you initially visit the page & then leave. Here's what I see:
- Login as an EPerson who has a Profile with Suggestions
- After login, go to your Profile (or MyDSpace). You will see the notice that suggestions exist. Click "Review the suggestions"
- That brings you to the suggestions page. Now, go immediately back to your Profile or MyDSpace (without performing any actions on the suggestions page)
- The Notice on those pages is now gone. There is no way to get back to your suggestions.
- If you logout & login again, the notification will appear again. This appears to be the only way to get back to the suggestions page.
- I'd recommend that either the Notification always exist on your Profile / MyDSpace until you take an action on all suggestions. Or, maybe there needs to be a "Publication Claim" link that appears in the User dropdown menu when you have suggestions?
Beyond those bugs, I've rereviewed the code. Most of my feedback has been addressed (thanks!). However, there are still some areas that were overlooked which I've highlighted again below. Overall, this is looking better!
Hi @tdonohue , points 1,2 and 4 have been addressed, point 3 is a backend specification and @frabacche will get there from the backend request. I hope this works for you.
@frabacche and @abollini: I also tested this again from the frontend & re-reviewed the code. Overall, it is looking better, but I'm still finding a few bugs/issues.
I'd recommend calling this page "Publication Claim" (singular) as that's more similar to "Quality Assurance" (the other page under "notifications"). Also, the breadcrumbs on the "Publication Claim" page should likely say "Publication Claim" instead of "Suggestions" Having two names for this page is confusing:
When logged in as an EPerson with suggestions in "Publication Claim", the breadcrumbs disappear if you click on the link to look at your suggestions. The breadcrumbs here should likely be "Home > Publication Claim > Tim Donohue". Additionally, the title of this page says "OpenAIRE" twice. We likely could simplify it to just
Suggestions for [name] from OpenAIRE GraphI'm not sure if this is a bug, but the "DateScorer" is always returning 0 and I'm not sure how to "set a min/max date in the profile"? What does that mean? It also doesn't seem to be possible to add "education achievements" in DSpace yet. Maybe we need to fix this text?
If you are logged in as an EPerson with Suggestions, you cannot get back to the suggestions page if you initially visit the page & then leave. Here's what I see:
- Login as an EPerson who has a Profile with Suggestions
- After login, go to your Profile (or MyDSpace). You will see the notice that suggestions exist. Click "Review the suggestions"
- That brings you to the suggestions page. Now, go immediately back to your Profile or MyDSpace (without performing any actions on the suggestions page)
- The Notice on those pages is now gone. There is no way to get back to your suggestions.
- If you logout & login again, the notification will appear again. This appears to be the only way to get back to the suggestions page.
- I'd recommend that either the Notification always exist on your Profile / MyDSpace until you take an action on all suggestions. Or, maybe there needs to be a "Publication Claim" link that appears in the User dropdown menu when you have suggestions?
Beyond those bugs, I've rereviewed the code. Most of my feedback has been addressed (thanks!). However, there are still some areas that were overlooked which I've highlighted again below. Overall, this is looking better!
Please consider the DateScorer surely can have a different score and is calculated on the backend (see corresponding backend pr at: https://github.com/DSpace/DSpace/pull/8280) . At the moment the score can be 0 or 10. The Issue Date of the publication of suggestion is compared with a range. If the issue date is on the inside of the range datescorer returns 10; if it is on the outside of the range datascorer returns 0. The first range used is the one configured onto the researcher profile set with the following metadatas: person.range.maxdate, person.range.mindate. This configuration is not mandatory so if it does not exist - the educational date is considered - even if it is not configured on the spring context. Finally the birthdate is considered.
@FrancescoMolinaro and @frabacche : This is almost ready to merge. However, I'm encountering two (new) bugs:
When I click the "Ignore Suggestion" button, that particular suggestion remains on the page until I click reload in the browser. Here's what I'm doing:
- Running the UI in production mode (
yarn build:prod; yarn serve:ssr)- Login as an EPerson with a Researcher Profile that has suggestions
- From one of the notifications, go to the list of suggestions
- Click "Ignore Suggestion" button. Nothing appears to happen. (However, behind the scenes, I see the request did occur & succeed.)
- Click Reload in the browser window. The suggestion now correctly disappears.
- (This appears to be a caching or reloading issue, where the button doesn't cause the list of suggestions to refresh.)
If I click "Approve & Import" on a suggestion, I can successfully create a
Publicationentity. But, thatPublicationis NOT linked to thePersonentity that the suggestion was for. Here's what I'm doing:
- Again, running the UI in production mode
- Login as an EPerson that has suggestions for his/her Researcher Page (Person entity)
- Once you see the notification, click on it to visit the suggestions
- Click "Accept & Import" on one of them. Create a new Publication.
- Upload a file, accept the license and Submit
- The Publication is created. But, it is NOT linked to my Person entity.
- Instead, I have to go to the Person entity, edit it & manually create a relationship to the new Publication, (Edit Person -> Relationships tab -> Add Publication). My assumption is this relationship should be created automatically because this Person is claiming this Publication as their own.
Beyond those two bugs, everything else appears to be working well.
Finally, I have one minor code suggestion below. Beyond that the code also looks good now.
- Ignore bug has been solved
- Suggestion import is a submission of a new item, so it's quite isolated. Author name is received and looked up on the workspace item: even though a single result is given - the link must be made by hand for safety reasons.
@frabacche : Thanks for the quick fixes/response.
This is almost working. I'm basically a +1. But, I'm still finding that the "Ignore Suggestions" button only reloads sometimes.
Here's a video of the behavior I'm seeing.
- I'm running the UI in production mode (
yarn build:prod; yarn serve:ssr).- As you can see, if I click the "Ignore Suggestions" button once, it works. But, clicking it a second time on a different suggestion will NOT work (unless I click it twice)
This is the only remaining feedback that I have on this PR. If it's not easy to solve, we can log a bug ticket. But, ideally, it'd be nice to get this button working properly.
Hi @tdonohue, I have made an improvement to the ignore suggestion button and it seems working in production mode (yarn build:prod; yarn serve:ssr).
Could you please give me confirmation once you have time to test it?
Many thanks in advance
Hi @tdonohue would you kindly restart the build process? We are afraid there's something randomly failing here. Thank you
@FrancescoMolinaro and @frabacche : I've retested today and verified that the "Ignore Suggestion" button is now working properly.
However, I've noticed another odd issue in my testing. Whenever I click on the "review the suggestions" link in a notification, it reloads the entire application. In other words, it switches over to Server Side Rendering (SSR) instead of remaining on the client side (CSR).
You can see this most easily by opening up your browser DevTools & clicking on that link. When you do so, you'll see on the "Network" tab that it waits on SSR to complete, and then reloads all the Javascript/CSS/logos for the application.
This seems like an unnecessary server-side request. Is there a way to fix this link to ensure it's remaining on the client side (like every other link in the application)?
I see no other issues at this time. So, I'm +1 this PR, but I think we need to fix the unnecessary SSR request.
Hello @tdonohue , I have fixed the unnecessary reload in SSR, the issue was due to the use of the normal href interpolated into the translation. The normal HTML href on an achor tag seems to be always triggering a page reload.
May I kindly ask you to retest?
Thanks in advance
NOTE Adding a needs documentation label to this PR until we have fully documented this in our pre-8.0 docs wiki space: https://wiki.lyrasis.org/display/DSDOC8x/
Documentation for this feature is now at https://wiki.lyrasis.org/display/DSDOC8x/Publication+Claim






