Page MenuHomePhabricator

[native] Navigate to the role creation screen with pre-populated fields for editing roles
ClosedPublic

Authored by rohan on Jul 17 2023, 11:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 9, 1:20 PM
Unknown Object (File)
Sat, Nov 9, 1:20 PM
Unknown Object (File)
Sat, Nov 9, 11:39 AM
Unknown Object (File)
Sat, Nov 9, 10:46 AM
Unknown Object (File)
Sat, Nov 9, 10:45 AM
Unknown Object (File)
Sat, Nov 9, 10:44 AM
Unknown Object (File)
Sat, Nov 9, 8:28 AM
Unknown Object (File)
Thu, Nov 7, 6:00 PM
Subscribers

Details

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 edited the test plan for this revision. (Show Details)
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?

lib/shared/thread-utils.js
1572 ↗(On Diff #28765)

Ah yeah I did, thanks for catching it

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 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
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?