Conversation
|
This pull request introduces 2 alerts when merging ad39ad3 into 13dac1a - view on LGTM.com new alerts:
|
|
@LucaGiamminonni : You may have already noticed this, but this PR has lint errors, so it's failing to run tests. |
|
This pull request introduces 9 alerts when merging 3b550b5 into 37ebe25 - view on LGTM.com new alerts:
|
|
This pull request introduces 11 alerts when merging da1fb55 into 37ebe25 - view on LGTM.com new alerts:
|
| findById(uuid: string): Observable<ResearcherProfile> { | ||
| return this.dataService.findById(uuid, false) | ||
| .pipe ( getFinishedRemoteData(), | ||
| map((remoteData) => remoteData.payload)); | ||
| } |
There was a problem hiding this comment.
Please don't "override" methods on a dataservice by using delegation and giving them a different signature or return type. findById on a dataservice is expected to return an Observable<RemoteData<T>> and have useCacheIfAvailable etc in its signature
Doing this will be made impossible after we fix #742, but for now we have to manually enforce it
| * | ||
| * @param researcherProfile the profile to delete | ||
| */ | ||
| delete(researcherProfile: ResearcherProfile): Observable<boolean> { |
There was a problem hiding this comment.
Same comment as findByID above, though if I'm not mistaken this change stems from #1617 and I already commented on it there
src/app/item-page/orcid-page/orcid-auth/orcid-auth.component.html
Outdated
Show resolved
Hide resolved
src/app/item-page/orcid-page/orcid-sync/orcid-setting.component.ts
Outdated
Show resolved
Hide resolved
|
This pull request introduces 11 alerts when merging 733688b into dd6ff7f - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
While testing DSpace/DSpace#8263, I noticed:
- If the user didn't make their email address public in ORCID, there's no page displaying what the user should do (rather a very generic error page). I've reported this in DSpace/DSpace#8263 as well since a REST part has to be implemented for this as well
- When on the
/entities/person/uuid/orcidpage, there's no link back to the person page - The
/entities/person/uuid/orcidpage always states the Synchronization mode is manual, even if it's set to batch
|
This pull request introduces 11 alerts when merging 857a3c5 into 9889a9d - view on LGTM.com new alerts:
|
Please mind that the synchronization is from DSpace to ORCID registry.
This part will be clearer with the queue PR. Basically "Manual" synchronization will be triggered only by the user in the queue page, while "Batch" is done by a batch script that is running once a day
syncing capabilities are automatically provided once a user connects dsapce person profile with its own orcid profile, it' doesn't matter if synchronization is triggerd manually or done by batch script
I've updated the description you feedback shoud be addresse, please check them again |
tdonohue
left a comment
There was a problem hiding this comment.
@atarix83 : I see you just updated the text of the Synchronization note. As I previously noted, I'd recommend we add descriptions for each of these settings... if I was confused by them, then I'm certain others will be. Here's my suggestions for the text to add in each box:
- Synchronization Mode box - I'd recommend minor updates to the next text to say this:
Please select how you would like synchronization to ORCID to occur. The options include "Manual" (you must send your data to ORCID manually), or "Batch" (the system will send your data to ORCID via a scheduled script).
- Publication preferences box - I'd recommend we add descriptive text above the two options. Something like this:
Select whether to send your linked Publication entities to your ORCID record's list of works
- Funding preferences box - Again, we should add descriptive text like this:
Select whether to send your linked Project entities to your ORCID record's list of funding information.
- Profile Preferences box - Again, we should add descriptive text like this:
Select whether to send your biographical data or personal identifiers to your ORCID record.
Overall, as I said, this page seems fine (however I obviously cannot test Sync capabilities until the other sync functionality is added in later PRs). But, I do think the usability would be improved by adding simple descriptions to each of these boxes.
|
done this is the result |
There was a problem hiding this comment.
Thanks @LucaGiamminonni! It all works as it should.
I do have some feedback on style and usability:
ORCID button on person page
All other context sensitive "admin" style buttons we have here are icons. For consistency it would be better to use ORCID's "ID" logo here instead. Or even better, since we're getting multiple buttons here anyway it would be good to turn this into a dropdown menu on all dso pages, rather than putting a bunch of buttons next to each other. Though that may be out of the scope of this PR.
Reaching the ORCID settings page
I think it's not ideal that that button is the only way to reach the settings page. I think it would make sense to add ORCID as a tab to the edit item page for Person items. In that case I'd also change the route to /entities/person/${uuid}/edit/orcid
Or, if we keep it as a separate page, perhaps add a direct link to the auth nav dropdown in the header, and/or the EPerson profile page
Collapsible cards
As for the settings page itself, I'm not sure what purpose the collapsible outer cards serve. Why would you ever collapse them? Getting rid of the outer cards, and using simple headers should suffice to keep the sections separate, and it would get rid of that boxes-in-boxes look.
If they remain collapsible cards, you should be able to tell by looking at them: The header text shouldn't be a link, but styled like a card header, it should be left aligned, there should be a chevron on the right indicating the state etc.
Spacing for authorizations boxes
On XS screens the authorizations boxes are glued together, they should get some margin-bottom
Alert width
The "enable manual synchronization" alert is in a row, but not in a column. As a consequence it's wider than everything else:
|
changes in order to fix issue discussed on the rest PR is done DSpace/DSpace#8263 (comment) Plase have a look
updated using the orcid icon instead
We think this is out of scope of this PR. We agree to address in 7.4 release
outer boxes removed, this is the new layout
fixed |
tdonohue
left a comment
There was a problem hiding this comment.
@LucaGiamminonni and @atarix83 : Re-tested this today and it's working & looking good overall to me. I also tested the new update by unlinking a Researcher profile, then re-claiming it and connecting it to ORCID (this results in the new PATCH request which works well.)
A few minor things I noticed in re-testing today.
- When I update the ORCID Settings, the old ones are temporarily cached (i.e. the page doesn't refresh itself). Here's what I'm doing
- Login via ORCID, create a Researcher Profile. View that profile.
- Go to the Orcid settings page (/orcid)
- On that page change the Synchronization setting to "Batch" and enable one (or more) of the other preferences.
- Click "Update settings". You'll get a green success message.
- Click "Back" button.
- Now go immediately back to the ORCID settings page. You'll see your newly changed settings are missing.
- Click Reload in your browser. Now, your newly changed settings are on that page.
- NOTE: I've verified that the settings are properly saved. However, the page appears to cache your old settings until you click Reload in your browser. So, this is just a minor caching bug.
- I do agree with @artlowel that we might think about (in 7.4 or later) moving the ORCID Settings page to a tab on the "Edit Item" page. It is confusing to have the ORCID settings outside of the "Edit Item" page. That said, I think we can schedule this for later cleanup work & leave this PR as-is. We should just create a separate ticket for this action.
Beyond that, the updates all look good to me & they are working well. So, I'm basically a +1, though it might be nice to cleanup that caching issue if possible. Thanks @atarix83 and @LucaGiamminonni !
There was a problem hiding this comment.
👍 Thanks @atarix83 and @LucaGiamminonni ! All my prior feedback (including the caching issue previously reported) has been addressed. I retested today and everything is working well & I'm not finding any new bugs. So, I'm OK with moving this forward as-is... if we find any additional bugs later, we can clean them up in a future PR (and/or the next ORCID PR).
NOTE: I fixed the e2e test failures that were occurring on main and all recent PRs (which was caused by the Docker backend not starting properly). If you rebase this PR against main then the failures below should fix themselves.
|
@artlowel : If you have a chance to re-test / re-review, this now looks good to me. |
artlowel
left a comment
There was a problem hiding this comment.
Thanks @LucaGiamminonni and @atarix83!
|
Merging as this is at +2 and the backend was approved & merged. Thanks again @LucaGiamminonni and @atarix83 ! |
…g (pull request DSpace#1618) Task/dspace cris 2023 02 x/DSC-1629 authority source porting Approved-by: Stefano Maffei
References
Includes #1617
Requires DSpace/DSpace#8263
Description
This PR add a specific page to:
This new page is accesible from the person item page with a specific button placed on the top right of the page.
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!
yarn run lintpackage.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.