Skip to content
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

Allow sharing permissions by ORCID #3196

Merged
merged 8 commits into from
Nov 11, 2024
Merged

Allow sharing permissions by ORCID #3196

merged 8 commits into from
Nov 11, 2024

Conversation

nellh
Copy link
Contributor

@nellh nellh commented Nov 7, 2024

Adds the option to add dataset permissions via ORCID.

Fixes #3190.

Screenshot From 2024-11-07 13-18-42

@nellh nellh requested a review from thinknoack November 7, 2024 21:20
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 51.42857% with 85 lines in your changes missing coverage. Please review.

Project coverage is 44.72%. Comparing base (0d7bdd7) to head (85ddbfc).
Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
...nneuro-server/src/graphql/resolvers/permissions.ts 12.76% 41 Missing ⚠️
.../src/scripts/dataset/routes/manage-permissions.jsx 0.00% 32 Missing ⚠️
...openneuro-server/src/graphql/resolvers/mutation.ts 0.00% 6 Missing ⚠️
...c/scripts/dataset/mutations/update-permissions.tsx 96.47% 3 Missing ⚠️
packages/openneuro-server/src/graphql/schema.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3196      +/-   ##
==========================================
+ Coverage   44.45%   44.72%   +0.26%     
==========================================
  Files         592      590       -2     
  Lines       37861    37835      -26     
  Branches     1131     1138       +7     
==========================================
+ Hits        16830    16920      +90     
+ Misses      20831    20717     -114     
+ Partials      200      198       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thinknoack
Copy link
Contributor

thinknoack commented Nov 8, 2024

@nellh - I know you are updating a few tests so send this back when you have those.

in addition here is a small note, when removing perms, the client/ui doesn't update the state for visual feedback until a refresh.

After a confirmed removal

Screenshot 2024-11-08 at 12 39 04 PM

After refresh

Screenshot 2024-11-08 at 12 39 19 PM

@nellh
Copy link
Contributor Author

nellh commented Nov 8, 2024

in addition here is a small note, when removing perms, the client/ui doesn't update the state for visual feedback until a refresh.

Thanks! That's because the remove mutation has the same issue we discussed where the API should return the entire fragment it modifies (all permissions instead of just the user) to update the client cache. I think that's a good bug to note (#3198) now that this generally fixes the client state issues with adding/removing permissions.

@nellh
Copy link
Contributor Author

nellh commented Nov 8, 2024

@thinknoack Added the test case we discussed in 85ddbfc.

@thinknoack thinknoack self-assigned this Nov 9, 2024
Copy link
Contributor

@thinknoack thinknoack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*Approving with small note

I have reviewed the code/file changes and tested with emails for both a new user and a original upload user. That all looks great.

*note - I will not be able to test the ORCID integration until I get my sandbox sorted out.

@nellh nellh merged commit 6b952af into master Nov 11, 2024
15 checks passed
@nellh nellh deleted the orcid-permissions branch November 11, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow permissions to be shared by ORCID
2 participants