-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Decouple user email from role name #18966
Conversation
4e35f14
to
84a30a2
Compare
5f1502a
to
d06cf20
Compare
Now that role name doesn't have to be unique, we don't want to pass args like "private role" or "shared role" on role creation.
Because we no longer can match role name to user email
Reason: decouple user email from private role naming: emails can be changed or redacted; user id in user-role-association + role type is sufficient to tie a user to a private role. The description (i.e., "this is a private role for a user" is inferrable from the role name ("private role"), which is assigned by default.
We must use a union because when we retrieve roles with a query, we check against: 1) role name 2) email of associated user for private roles We factor out this select into a helper method, which we then test extensively.
d06cf20
to
16f1d3f
Compare
This is ready for review. |
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.
Looks pretty good to me! Thanks!
I'll let others have a second review, especially about the query part, but it does look a lot cleaner to me.
Co-authored-by: David López <[email protected]>
Co-authored-by: David López <[email protected]>
not sure I understand this, I presume that's something we used to have a user interface for ? does that need to be added back ? |
Co-authored-by: Marius van den Beek <[email protected]>
Co-authored-by: Marius van den Beek <[email protected]>
Sorry, this is confusing out of context! It's for the populator - see my previous response, I think it covers it. |
Thanks @jdavcs! |
This needed a migration rebase prior to merge, we have two heads now @jdavcs |
Sorry! A new migration sneaked in. I'll handle this. |
The idea is to decouple emails from role names. In this approach, we remove the unique constraint from the
role.name
field in the database, which allows the creation of generic roles like "private role" or "sharing role", which do not depend on a user's email and are identified by their type and a relationship stored in the user_role_association table. However, when an admin user manually creates a role in the admin UI, they will be required to pick a unique role name. Otherwise, an admin may accidentally create duplicate roles with names that are intended to distinguish them from other roles (e.g. we don't want multiple role names like "Foo lab").Notes:
This adds a new API endpoint for retrieving user roles (currently used to retrieve a user's private role, which is no longer possible by retrieving all roles and selecting the one with the name matching the user's email).
How to test the changes?
(Select all options that apply)
License