Details

Reviewers

atul
ginsu

Commits

rCOMM95fd9f9c2a3f: [native] Navigate to the role creation screen with pre-populated fields for…

Summary

This diff handles the edit roles action on the client-side. Keyserver support for editing roles will be a minimal change and will come in the following diff. I was able to re-use the create-roles screens/flow here, and passed in the existing permissions and role name to pre populate the fields.

ENG-4369

Depends on D8523

Test Plan

Confirmed that the navigation works as expected and the correct permissions get selected on navigation (also to note, the 'Create' text in the top right is changed in the next diff to 'Save' if editing a role):

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Jul 17 2023, 11:08 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 17 2023, 11:08 AM

rohan edited the summary of this revision. (Show Details)Jul 17 2023, 11:11 AM

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

rohan added a parent revision: D8523: [native] Display an action sheet with the 'edit role' option.

rohan edited the test plan for this revision. (Show Details)Jul 17 2023, 11:19 AM

Harbormaster completed remote builds in B20982: Diff 28765.Jul 17 2023, 11:25 AM

rohan requested review of this revision.Jul 17 2023, 11:25 AM

ashoat added inline comments.Jul 18 2023, 6:12 AM

lib/shared/thread-utils.js
1572 ↗	(On Diff #28765)	Weird that the values are read-only, but the object itself isn't. Did you mean to make it all read-only?

rohan added inline comments.Jul 18 2023, 8:15 AM

lib/shared/thread-utils.js
1572 ↗	(On Diff #28765)	Ah yeah I did, thanks for catching it

Make object read-only

Harbormaster completed remote builds in B20991: Diff 28776.Jul 18 2023, 8:34 AM

rohan added a child revision: D8564: [native] Support the edit_role action on the keyserver.Jul 19 2023, 7:21 AM

rohan added inline comments.Jul 21 2023, 8:08 AM

lib/shared/thread-utils.js
1574 ↗	(On Diff #28776)	This function essentially takes the existing roles in the community, and 'reverse maps' the permissions to user-facing permission enums (i.e. if the role has `manage_pins`, `descendant_manage_pins` `react_to_message` `descendant_react_to_message`, we can reverse map them to the user-facing permissions `MANAGE_PINS` and `REACT_TO_MESSAGES`. This will in turn help pre-populate the fields when editing a role, since client-side we don't consider any `threadPermissions` at all, and just consider user-facing permission enums. So in this case, we would know to pre-populate the checkboxes for those two permissions to be checked

atul accepted this revision.Jul 23 2023, 9:46 PM

atul added inline comments.

lib/shared/thread-utils.js
1574 ↗	(On Diff #28776)	Should we add a summarized version of this annotation as a comment above the function?
native/roles/community-roles-screen.react.js
58–61 ↗	(On Diff #28776)	Should we create a hook instead of memoizing function call?
native/roles/role-panel-entry.react.js
62 ↗	(On Diff #28776)	Is there some constant defined somewhere we can use instead of this "magic string"?

This revision is now accepted and ready to land.Jul 23 2023, 9:46 PM

rohan added inline comments.Jul 24 2023, 10:42 AM

native/roles/role-panel-entry.react.js
62 ↗	(On Diff #28776)	It's the option I wrote in myself on line 45 from the action sheet - would you prefer I define something like `const editRoleOption = "Edit role"` in the global scope instead?