-
Notifications
You must be signed in to change notification settings - Fork 87
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
api: create new endpoint for access/users #1673
api: create new endpoint for access/users #1673
Conversation
52df833
to
967d7b6
Compare
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.
Reviewed with @jrcastro2. Left a few comments. LGTU
Thank you for extensive tests 🚀
967d7b6
to
ce5b6c4
Compare
d741bf5
to
1a2e551
Compare
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.
Thanks a lot!
Apart from the review with @ptamarit, here are some comments:
- Naming consistency: I see we have
RDMGrantsAccessResource
,RDMGrantUserAccessResource
(see the pluralgrants
) - The records service is growing a lot, would it make sense to split these new
*_grant_by_subject
method calls? access grants users
andaccess grants
live in the same service since they are similar (related to record access), however, they seem to be different now in terms of functionality.
We can also chat IRL so I can get better context about the change.
8abd3ae
to
2764dd7
Compare
2764dd7
to
d0b85b6
Compare
d0b85b6
to
aada311
Compare
2006032
to
1073274
Compare
* add resource and service layer * add error handerls * add links * cover with tests * closes inveniosoftware#1671
d53fed3
to
a9e065b
Compare
access/users
endpoint #1671