-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add mutation to clear user roles and facilities #7978
Conversation
d550010
to
594a1fd
Compare
594a1fd
to
c622546
Compare
public ApiUser markUserRolesAndFacilitiesAsDeleted(@Argument String username) { | ||
return _us.markUserRolesAndFacilitiesAsDeleted(username); |
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.
only nit is can we name these clearUserRolesandFacilities
or something like that? "mark as deleted" to me implies that there's an isDeleted flag we are flipping and that this is reversible, but we're just clearing them for good
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.
Will do! thank you, merethe!
Quality Gate passedIssues Measures |
@mehansen method has been renamed -- ready for rereview! |
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.
LGTM! Created a new user with roles on dev2 and confirmed these were deleted after the mutation
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.
Tested on dev2 with a new user whose rolls were deleted after successfully calling the new endpoint -- LGTM! Thanks for your work on this
public ApiUser clearUserRolesAndFacilities(String username) { | ||
ApiUser foundUser = | ||
_apiUserRepo.findByLoginEmail(username).orElseThrow(NonexistentUserException::new); | ||
foundUser.clearRolesAndFacilities(); |
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.
worth considering for a followup PR: could we stick a check for whether the found user is a site admin? Think this would entail extending our site admin validation code since from what I can tell, it grabs that information from the currently-logged in user auth properties, but think that
- doing an explicit check is the safest way to make sure we don't end up in a unsupported state
- Having a "is this email in the site admin group" is a generally useful method to have.
Since all this is behind a feature flag / this mutation is needed to clean up a testing env, no need to address here, but to consider as a follow up
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.
Paired with @mehansen to remove roles and facilities of prod site admin users on 08/05/2024. |
BACKEND PULL REQUEST
Related Issue
Changes Proposed
ApiUserRole
andApiUserFacility
entries as deletedAdditional Information
Testing