Curators can now update the curator relationship #3536
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Looks good! Tested locally with admin + global curator/basic + curator/basic user |
| <> | ||
| {/* Confirmation modal - only shown when users try to demote themselves */} | ||
| {showDemoteConfirm && pendingRoleChange && ( | ||
| <GenericConfirmModal |
There was a problem hiding this comment.
Would refresh user here or an alternative- user should likely be redirected to chat page
| user_making_change_curator_groups = fetch_user_groups_for_user( | ||
| db_session=db_session, | ||
| user_id=user_making_change.id, | ||
| # only check if the user making the change is a curator if they are a curator |
There was a problem hiding this comment.
like a group level curator? "only check if the user making the change is a curator if they are a curator" seems circular no?
There was a problem hiding this comment.
sure i guess it could be worded "only enforce the existance of a curator relationship if they are a curator, ..."
| raise ValueError( | ||
| f"User '{user.email}' is an admin and therefore has all permissions " | ||
| f"user making change {user_making_change.email} is not a curator," | ||
| f" admin, or global_curator for group '{user_group_id}'" |
There was a problem hiding this comment.
This also seems wrong, "global curator for group" is nonsensical no?
There was a problem hiding this comment.
they are not a global curator for this group is true
There was a problem hiding this comment.
Ok let me know if I'm interpreting this correctly, it should likely be something like "is not an admin, a global curator, or a group curator for the group "
| # only check if the user making the change is a curator if they are a curator | ||
| # otherwise, they are a global_curator and can update the curator relationship | ||
| # for any group they are a member of | ||
| only_curator_groups=user_making_change.role == UserRole.CURATOR, |
There was a problem hiding this comment.
Logic should be something like, if they are an admin or global curator, don't run this check at all. If they are a normal curator, run this check and this flag is true, if they are anything else, raise an exception
There was a problem hiding this comment.
Technically with this being a local function, it's probably not going to cause any problems if we don't have this extremely clear logic but still better to do it this way no?
There was a problem hiding this comment.
this is handled on line 453
There was a problem hiding this comment.
I see that but there is a different organization of code that I prefer, for example, this flag is always True, but if they are a normal user or some other role type, we throw an exception somewhere above.
| "of a curator. If you'd like this user to only have curator permissions, " | ||
| "you must update their role to BASIC then assign them to be CURATOR in the " | ||
| "appropriate groups." | ||
| ) | ||
| elif target_user.role == UserRole.GLOBAL_CURATOR: | ||
| raise ValueError( | ||
| f"User '{target_user.email}' is a global_curator and therefore has all " |
There was a problem hiding this comment.
Is this propagated to the frontend? Best way typically is to raise a backend only error, catch it near the top near the API call and translate it to a user friendly error message.
We don't want to float up "dev facing" errors to the end user, even if it's admins/curator users. So nothing like "variable_underscore_something" in user facing flows.
There was a problem hiding this comment.
unsure, the propagation of errors to the frontend is currently unstandardized. probably worth taking a cohesive pass if we would like to enforce standards
There was a problem hiding this comment.
Chatted with Chris, it's not worth the overhead to standardize this. The only comment here is to not have dev facing terms and user facing terms in the same message. Actually we should just never have dev facing terms in any user facing flows.
| user_making_change: User | None = None, | ||
| ) -> None: | ||
| target_user = fetch_user_by_id(db_session, set_curator_request.user_id) | ||
| if not target_user: |
There was a problem hiding this comment.
Use explicit Nones where possible
| db_session: Session, | ||
| user_group_id: int, | ||
| set_curator_request: SetCuratorRequest, | ||
| user_making_change: User | None = None, |
There was a problem hiding this comment.
pref not default this to None
Description
Curators can now promote basic users to curators of a group and demote them
curators can also demote themselves but a modal pops up asking for confirmation:
How Has This Been Tested?
Watch the video. A in top right is admin. other 2 accounts are curator/basic
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.