-
Notifications
You must be signed in to change notification settings - Fork 435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Openaire suggestions (publication claim) #1638
Conversation
This pull request introduces 27 alerts when merging a798ded into 37ebe25 - view on LGTM.com new alerts:
|
This pull request introduces 16 alerts when merging de6b533 into fae355a - view on LGTM.com new alerts:
|
This pull request introduces 16 alerts when merging d8e96d8 into fae355a - view on LGTM.com new alerts:
|
Hi @LucaGiamminonni, |
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. |
Please consider the DateScorer surely can have a different score and is calculated on the backend (see corresponding backend pr at: DSpace/DSpace#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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.)
- Running the UI in production mode (
- If I click "Approve & Import" on a suggestion, I can successfully create a
Publication
entity. But, thatPublication
is NOT linked to thePerson
entity 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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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. May I kindly ask you to retest? Thanks in advance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thank you @FrancescoMolinaro and @frabacche and @LucaGiamminonni (and whoever else has been involved with this PR). I've verified that the last fix works and all my feedback has now been addressed. Merging immediately.
NOTE Adding a |
Documentation for this feature is now at https://wiki.lyrasis.org/display/DSDOC8x/Publication+Claim |
References
Unique code in this PR can be found via this diff:
4Science/dspace-angular@CST-5337...4Science:dspace-angular:CST-5249_suggestion
The suggestion feature requires the researcher profile feature, so it is required to:
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.
How to Test
First, you must enable the following Researcher Profile settings by default:
Then, do the following:
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!
yarn run lint
package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.