Details

Reviewers

ashoat
atul
ginsu

Commits

rCOMM5bf8ab7922de: [native] Support the edit_role action on the keyserver

Summary

This should be the final diff in the editing flow for custom roles, adding support on the keyserver to now support editing roles. It's important to note that since we pass the edited role name and permissions, we needed something concrete to identify what role is being edited, so that's why there is a new parameter representing the existing role ID that should always exist if being edited (it's optional since it's only defined in the editing flow, but not in the role creation flow).

ENG-4370

Depends on D8524

Test Plan

Created a new role without certain permissions (specifically manage pins and react to messages), tested to verify that a user assigned that role lacks those permissions client-side. Then, edited the role to include those permissions and observed that the same user now could manage pins and react to messages

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Jul 19 2023, 7:21 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 19 2023, 7:21 AM

Harbormaster completed remote builds in B21043: Diff 28839.Jul 19 2023, 8:29 AM

rohan requested review of this revision.Jul 19 2023, 8:29 AM

rohan planned changes to this revision.Jul 19 2023, 12:54 PM

rohan added a child revision: D8574: [keyserver/lib] Enable the option to delete custom roles.

Call updateRole to propagate updates correctly

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

rohan edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B21059: Diff 28858.Jul 19 2023, 1:58 PM

rohan added inline comments.Jul 19 2023, 2:06 PM

keyserver/src/updaters/thread-permission-updaters.js
201–203 ↗	(On Diff #28858)	Here as well, it didn't make sense to keep this check since the old role will always be the intendedRole since when editing roles, we aren't actually assigning members a 'new' role, just updating their roles' permissions.
251–259 ↗	(On Diff #28858)	I deleted this because while the permissions may change pre and post edit, a case where just the role name was changed would cause an error since the thread would not be pushed to `membershipRows` and would not correctly be updated
keyserver/src/updaters/thread-updaters.js
157–160 ↗	(On Diff #28858)	This is a newly introduced `silenceNewMessages` option so I can 'silence' creating a new change role message when editing a role, as it doesn't really make sense to generate a change_role for every member when we're just editing a role (this was also agreed on in DES-69.

Rebase

Harbormaster completed remote builds in B21068: Diff 28868.Jul 19 2023, 3:26 PM

Rebase

Harbormaster completed remote builds in B21117: Diff 28939.Jul 21 2023, 8:26 AM

Adding @ashoat as blocking reviewer here since I don't have enough context on roles/permissions to give this a thorough review

atul added inline comments.Jul 23 2023, 10:05 PM

keyserver/src/creators/role-creator.js
157 ↗	(On Diff #28939)	Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could move `await dbQuery(query);` within both the `(action === 'create_role')` (line 139) and `else if (action === 'edit_role')` (line 144) blocks? Not a huge deal, whatever you prefer
170 ↗	(On Diff #28939)	Should we add this `.map` to the end of the `.filter` on lines 162-164?

Copying my comments over from before the rebase + responding to new comments

keyserver/src/creators/role-creator.js
157 ↗	(On Diff #28939)	Yeah we can, that should be cleaner too
170 ↗	(On Diff #28939)	Sure that makes sense
keyserver/src/updaters/thread-permission-updaters.js
201–204 ↗	(On Diff #28939)	Here as well, it didn't make sense to keep this check since the old role will always be the intendedRole since when editing roles, we aren't actually assigning members a 'new' role, just updating their roles' permissions.
251–259 ↗	(On Diff #28939)	I deleted this because while the permissions may change pre and post edit, a case where just the role name was changed would cause an error since the thread would not be pushed to membershipRows and would not correctly be updated
keyserver/src/updaters/thread-updaters.js
157–160 ↗	(On Diff #28939)	This is a newly introduced `silenceNewMessages` option so I can 'silence' creating a new change role message when editing a role, as it doesn't really make sense to generate a change_role for every member when we're just editing a role (this was also agreed on in DES-69

resigning since I don't have enough context on roles/permissions to give this a proper review

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

Rebase

Harbormaster completed remote builds in B21222: Diff 29066.Jul 26 2023, 9:16 AM

Address @atul's comments and only call updateRole if the role has existing members assigned to it (to prevent a server error)

rohan added inline comments.Jul 26 2023, 1:47 PM

keyserver/src/updaters/thread-permission-updaters.js
201–204 ↗	(On Diff #29077)	Comment on older revision about this change
251–259 ↗	(On Diff #29077)	Comment on older revision about this change
keyserver/src/updaters/thread-updaters.js
157–160 ↗	(On Diff #29077)	Comment on older revision about this change

Harbormaster completed remote builds in B21233: Diff 29077.Jul 26 2023, 2:06 PM

Ended up reviewing an earlier version of the diff, but my comments all apply to the current version as well

keyserver/src/creators/role-creator.js
158–160 ↗	(On Diff #28858)	This check should occur way earlier. You're letting the `dbQuery` call go through but then triggering an error... that's just going to lead to a corrupt state of the database! Probably the most idiomatic way to handle this would be to updating both the Flow type and the input validator to be a union of disjoint types. Then the check would automatically occur at the start during input validation, and you wouldn't need to check anything here since the Flow types would guarantee that `existingRoleID` would be set in this scenario.
172 ↗	(On Diff #28858)	Based on comment here, we'll probably want to keep the messages here (and get rid of the new `silenceNewMessages` functionality)
keyserver/src/updaters/thread-permission-updaters.js
201–203 ↗	(On Diff #28858)	(Read my next comment first.) Whereas the check below should only trigger if there are indeed no changes that should be written to the `memberships` table, this check is a little different. You're right that the "old role will always be the intendedRole" in your scenario. That said, my first point below still applies: The code in `thread-permission-updaters.js` is highly optimized because recalculating permissions is very expensive. If a change is necessary, we should find a way to achieve it without removing any optimizations. (To give you an idea: on my keyserver, doing a full recalculation of all permissions (which can happen when eg. a new `threadPermissions` type is introduced) can take around 15min.) As such, we need to update this check so that the `continue` does not happen, WITHOUT removing any existing optimizations. Can you find a way to do this with eg. a new `forcePermissionRecalculation` param in `ChangeRoleOptions`?
251–259 ↗	(On Diff #28858)	The code in `thread-permission-updaters.js` is highly optimized because recalculating permissions is very expensive. If a change is necessary, we should find a way to achieve it without removing any optimizations. (To give you an idea: on my keyserver, doing a full recalculation of all permissions (which can happen when eg. a new `threadPermissions` type is introduced) can take around 15min.) I don't understand the error you're describing. The "case where just the role name was changed" would ostensibly require no changes to any `memberships` rows, right? Is there a way to achieve what you want without causing no-op writes on the `memberships` table? I'm not sure if it's helpful, but it's worth noting that `commitMembershipChangeset` takes a `changedThreadIDs` parameter that can force the creation of `updateTypes.UPDATE_THREAD` for all clients.

This revision now requires changes to proceed.Jul 27 2023, 6:56 PM

Update the Flow types for RoleModificationRequest to be a disjoint union of CreateRoleAction and EditRoleAction (also updated the input validator in thread-responders
Updated role-creator to remove checks for existingRoleID, and instead Flow can now infer it’ll be present based on the request.action property (for me, Flow still struggled refining the type if I destructured action out of request). Used the Flow docs to write this.
Bring back the permission check optimizations
Introduce a forcePermissionRecalculation option to updateRole so when the permissions are not changed but the role name is changed, force the creation of updateTypes.UPDATE_THREAD in commitMembershipChangeset
Propagate forcePermissionRecalculation to createRole so when the permissions are edited, we can still make the required modifications to memberships by skipping the continue

Tested the following:

Creating a new role
Editing the permissions of an existing role
Editing the name of an existing role
Editing both the permissions and the name of an existing role

All worked with no errors

rohan added inline comments.Jul 29 2023, 10:05 AM

keyserver/src/creators/role-creator.js
172 ↗	(On Diff #28858)	I think we actually want it here, since this was introduced for a different reason. Currently, if 'jack' and 'brian' are assigned the 'Moderators' role, and I go in and edit the role's permissions, because we're calling `updateRole`, it'll generate a new message in chat saying 'you assigned jack and brian the "Moderators" role`. I think we want to avoid that since we didn't actually change anyone's role, we just edited it. Let me know if you disagree though! Demo (no silence messages option) Demo (silence messages option)

rohan attached a referenced file: F661161: Simulator Screen Recording - iPhone 14 Pro - 2023-07-28 at 01.40.25.mp4. (Show Details)Jul 29 2023, 10:06 AM

rohan attached a referenced file: F661155: Simulator Screen Recording - iPhone 14 Pro - 2023-07-28 at 01.42.07.mp4. (Show Details)

Harbormaster failed remote builds in B21338: Diff 29229!Jul 29 2023, 10:15 AM

Update callsite (to resolve build errors)

Harbormaster completed remote builds in B21339: Diff 29230.Jul 29 2023, 10:44 AM

rohan mentioned this in D8595: [web] Add save functionality to the create role modal.Jul 29 2023, 11:01 AM

I have a lot of small comments below. None of them are substantial, so I'll accept here, but please make sure to address comments before landing!

keyserver/src/creators/role-creator.js
172 ↗	(On Diff #28858)	This makes sense. Thanks for explaining!
keyserver/src/updaters/thread-permission-updaters.js
202 ↗	(On Diff #29230)	Not sure why you're casting this to a `boolean` here... I think you could just use `!forcePermissionRecalculation` directly below
keyserver/src/updaters/thread-updaters.js
152 ↗	(On Diff #29230)	I'm a little confused how Flow is allowing this. `forcePermissionRecalculation` appears to be a `?boolean`, but `ChangeRoleOptions` indicates that `forcePermissionRecalculation` must be a `boolean` if set. This is actually a case where the `!!` construction would be appropriate
156 ↗	(On Diff #29230)	Not sure why you cast to a `boolean` here. You only use this variable for an `if` check, which will resolve the same way regardless of where you do this cast
156–169 ↗	(On Diff #29230)	I think all of these lines could be simplified: const { viewerUpdates } = await commitMembershipChangeset( viewer, changeset, forcePermissionRecalculation ? { changedThreadIDs: new Set([request.threadID]) } : {}, // or undefined? );
171–179 ↗	(On Diff #29230)	This declaration could be moved into the conditional below
native/roles/create-roles-header-right-button.react.js
56 ↗	(On Diff #29230)	This `invariant` could also be avoided if you typed the params as a union of disjoint types, but I don't think it's as important as in the other case

This revision is now accepted and ready to land.Jul 31 2023, 10:09 AM

Address feedback

Rebase

ashoat mentioned this in D8680: [keyserver] Correctly set memberships last_message.Jul 31 2023, 12:32 PM

Harbormaster completed remote builds in B21398: Diff 29310.Jul 31 2023, 2:46 PM

This revision was landed with ongoing or failed builds.Jul 31 2023, 6:15 PM

Closed by commit rCOMM5bf8ab7922de: [native] Support the edit_role action on the keyserver (authored by rohan). · Explain Why

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMM5bf8ab7922de: [native] Support the edit_role action on the keyserver.

Harbormaster completed remote builds in B21419: Diff 29331.Jul 31 2023, 7:19 PM

[native] Support the edit_role action on the keyserver
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 29392

keyserver/src/creators/role-creator.js

keyserver/src/responders/thread-responders.js

keyserver/src/updaters/thread-permission-updaters.js

keyserver/src/updaters/thread-updaters.js

lib/types/thread-types.js

native/roles/create-roles-header-right-button.react.js

native/roles/create-roles-screen.react.js

native/roles/role-panel-entry.react.js

[native] Support the edit_role action on the keyserverClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 29392

keyserver/src/creators/role-creator.js

keyserver/src/responders/thread-responders.js

keyserver/src/updaters/thread-permission-updaters.js

keyserver/src/updaters/thread-updaters.js

lib/types/thread-types.js

native/roles/create-roles-header-right-button.react.js

native/roles/create-roles-screen.react.js

native/roles/role-panel-entry.react.js

[native] Support the edit_role action on the keyserver
ClosedPublic
Actions

Revision Contents
Changeset List