Details

Reviewers

ginsu
ashoat
atul

Commits

rCOMM102a7d037cf1: [keyserver/lib] Enable the option to delete custom roles

Summary

This diff handles the flow for deleting custom roles. There are two cases to consider here when a role is deleted: (1) if there are no members assigned to the role, just delete it. (2) If there are members assigned to the role, then we need to display in the Alert message that they will be assigned to the default role (I did not hardcode the role name 'Members' since we're allowing it to be renamed, so instead I pull the default role name from the threadInfo).

A lot of the code in this diff is the scaffolding for the deletion API, so I decided that covering it all in one diff would be more logical and easier to review.

ENG-4389

Depends on D8564

Test Plan

Confirmed that role deletion works successfully for both cases (1) and (2).

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Jul 19 2023, 12:53 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 19 2023, 12:53 PM

rohan edited the summary of this revision. (Show Details)Jul 19 2023, 12:54 PM

rohan added a parent revision: D8564: [native] Support the edit_role action on the keyserver.

rohan added inline comments.

native/roles/role-utils.react.js
43–50 ↗	(On Diff #28857)	cc @ashoat for the copy, these were pulled from the designs

rohan edited the test plan for this revision. (Show Details)Jul 19 2023, 12:58 PM

Harbormaster completed remote builds in B21058: Diff 28857.Jul 19 2023, 1:11 PM

rohan requested review of this revision.Jul 19 2023, 1:11 PM

Rebase

Harbormaster completed remote builds in B21118: Diff 28940.Jul 21 2023, 8:32 AM

rohan added a child revision: D8591: [web] Rename invite-links-menu to community-actions-menu.Jul 21 2023, 8:55 AM

rohan mentioned this in D8625: [web] Create the modals that will allow the user to confirm role deletion.Jul 25 2023, 9:25 AM

A lot of the code in this diff is the scaffolding for the deletion API, so I decided that covering it all in one diff would be more logical and easier to review.

Hey if it is not too inconvenient, would you be able to break down this diff further? I'm gonna spend some more time reviewing this more closely tomorrow, but as a reviewer it would really help me out if these diffs were smaller. I feel like it's also a bit easier for me to give a more quality review when I can just focus on specific parts instead of a bigger diff with a lot of different parts

atul resigned from this revision.Jul 26 2023, 12:09 AM

atul added a reviewer: ashoat.

Attempt refactor to split this diff

rohan removed a child revision: D8591: [web] Rename invite-links-menu to community-actions-menu.Jul 26 2023, 8:21 AM

rohan added a child revision: D8639: [keyserver/lib] Introduce the thread actions, endpoints, and responders for deleting a custom role.Jul 26 2023, 8:23 AM

rohan mentioned this in D8640: [native] Introduce client-side logic for deleting roles on native.Jul 26 2023, 8:26 AM

In D8574#254043, @ginsu wrote:

A lot of the code in this diff is the scaffolding for the deletion API, so I decided that covering it all in one diff would be more logical and easier to review.

Hey if it is not too inconvenient, would you be able to break down this diff further? I'm gonna spend some more time reviewing this more closely tomorrow, but as a reviewer it would really help me out if these diffs were smaller. I feel like it's also a bit easier for me to give a more quality review when I can just focus on specific parts instead of a bigger diff with a lot of different parts

Yeah! Here you go:

D8574 (this diff) - The function to delete the role on the keyserver and the types
D8639 - The scaffolding for deleting a role (endpoints, responders, actions, etc)
D8640 - Client-side logic for deleting roles, no changes from this diff just a straight copy of what was originally here

Harbormaster completed remote builds in B21216: Diff 29060.Jul 26 2023, 8:36 AM

rohan retitled this revision from [keyserver/lib/native] Enable the option to delete custom roles to [keyserver/lib] Enable the option to delete custom roles.Jul 26 2023, 9:02 AM

Thanks for breaking this diff up!

Accepting, but don't have a ton of context about createUpdates flow in the second half of this function, so would appreciate a second take on this. The first half looked good to me outside of one nit.

keyserver/src/deleters/role-deleters.js
66 ↗	(On Diff #29060)	If `defaultRole` is an id, a better variable name here could be `defaultRoleID`

Rename variable to defaultRoleID

Use the new silenceNewMessages option to prevent creating a message in the community for when a role with members is deleted and those members are defaulted to the Members (or whatever the default role is renamed to) role. This was discussed in DES-69.

Harbormaster completed remote builds in B21259: Diff 29103.Jul 27 2023, 3:25 AM

Harbormaster completed remote builds in B21260: Diff 29104.Jul 27 2023, 3:33 AM

ashoat requested changes to this revision.Jul 27 2023, 10:00 AM

ashoat added inline comments.

keyserver/src/deleters/role-deleters.js
35	This is deeply broken. I really wish my reviews weren't so critical on this team... @rohan and @ginsu both should have individually caught this. Both of you, please think carefully about how this change was put up and got through review.
56	Nit
67	What are we doing here to prevent deletion of the admin role? Do the designs indicate that it should be possible to delete the admin role? Is it handled by the `updateRole` call below? Ideally we could handle the admin role the same way we handle the `default_role`. Is there a task tracking that?
78	If we're silencing this message, we should at least have a message for the role deletion. Is that tracked somewhere?
85	Nit

This revision now requires changes to proceed.Jul 27 2023, 10:00 AM

rohan added inline comments.Jul 27 2023, 6:00 PM

keyserver/src/deleters/role-deleters.js
35	That's my fault, I should've realized to `await` the call. Sorry about that, I'll update this when I update this diff soon
67	I should've handled this here, I think the ideal way to check this until ENG-4155 is complete is to check the `ThreadInfo` for the role name associated with the `roleID`, and whether it's name is `Admins`
78	Based on previous discussion on Linear, I thought we wanted to both prevent showing members assigned the default role and avoid creating a new message type for role deletion. If we think that we should have at least one, I think the former is cleaner and more useful to users since we already show role change messages. Open to either though if you feel strongly

ashoat added inline comments.Jul 27 2023, 6:28 PM

keyserver/src/deleters/role-deleters.js
35	Thanks!! I know it was probably easy to miss because `checkThreadPermission` isn't an `async` function, it just returns a `Promise`
67	That makes sense
78	Ah, I definitely missed that first comment when I made the second one... Hmm, yeah I figured it would be good to at least have a robotext for one of these. It seems like it would be easier to just keep the message about roles changing, since that one already exists. Plus, deleting a role that has nobody probably isn't notable... it's the reassigning of roles that feels notable.

Correct indentation in queries
Remove the silence option and show the change role message when a role is deleted
Add a check to make sure the role provided is not the Admin role (tested this by passing in the Admin role ID for a local community and logged the boolean value & saw the ServerError log)
Add await to checkThreadPermission

rohan marked 2 inline comments as done.Jul 27 2023, 10:24 PM

rohan added inline comments.Jul 27 2023, 10:26 PM

keyserver/src/deleters/role-deleters.js
77 ↗	(On Diff #29155)	Could probably make this `if (roleID === defaultRoleID \|\| roleID === adminRoleID) {`

Harbormaster completed remote builds in B21287: Diff 29155.Jul 27 2023, 10:40 PM

Accepting since my comments are minor, but the one about a potential crash is serious. Please address comments before landing!

keyserver/src/deleters/role-deleters.js
72 ↗	(On Diff #29155)	You're going to cause a crash if we can't fetch the `threadInfo`. Instead, can you check for its existence, and throw a `ServerError` if it's not set?
77 ↗	(On Diff #29155)	I'd probably do that