Details
- Reviewers
atul ginsu ashoat - Commits
- rCOMM0d5c2d49e7fc: [native] Populate the create role screen
Navigated to the screen and typed in different role names, selected several permissions, tried clearing all permissions at once, and tried deselected some permissions individually.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
- Pass in an isEmpty flag instead of the entire selectedPermissions array in the clearPermissionsText memo
- Pass a function to the setter in onEnumValuePress
- Add the defintion of configurableCommunityPermissions in this diff since this is where it is used
I appreciate your response, but I don't think my concern here was addressed:
What if some of the entries in configurableCommunityPermissions refer to the same permission? How are we guaranteeing that that doesn't happen?
You've established a requirement that "each permission in configurableCommunityPermissions is only assigned to one user-facing configurable option". While that is true today, this requirement isn't clear from the code, and there's little to stop a future dev from breaking it.
I have two potential suggestions:
- You could write a unit test that iterates through the collection and makes sure this property is enforced. While this doesn't solve the readability concern, it DOES do the most important thing, which is to make sure the requirement is enforced.
- A better solution would also address the readability concern, and make it so somebody reading the code understands that there is such a requirement, and is not surprised to see eg. the implementation of isPermissionSelected depending on such a requirement. Perhaps configurableCommunityPermissions could be rewritten.
As an example, we could consider rewriting this:
const configurableCommunityPermissions = [ { permissions: [ threadPermissions.EDIT_ENTRIES, threadPermissionPropagationPrefixes.DESCENDANT + threadPermissions.EDIT_ENTRIES, ], title: 'Edit calendar', description: 'Allows members to edit the community calendar', }, ]
Into this:
const calendarEditPermission = { title: 'Edit calendar', description: 'Allows members to edit the community calendar', }; const descendantEditEntries = threadPermissionPropagationPrefixes.DESCENDANT + threadPermissions.EDIT_ENTRIES; const configurableCommunityPermissions = { [threadPermissions.EDIT_ENTRIES]: calendarEditPermission, [descendantEditEntries]: calendarEditPermission, ];
- Address readability concern and concretely map out one permission to one user-facing selectable option. Now, both manage_pins and descendant_manage_pins, for example, are separate entries but map to the same title/description that will be user-facing.
- Introduce a getKeysByValue helper method in objects.js, since the structure of configurableCommunityPermissions will benefit from this. Now, since we have something like the following:
Object.freeze({ [editEntries]: calendarEditPermission, [descendantEditEntries]: calendarEditPermission, [descendantKnowOf]: secretChannelsPermission, [descendantVisible]: secretChannelsPermission, [voiced]: voicedPermission, [editThreadName]: createAndEditChannelsPermission, [descendantEditThreadName]: createAndEditChannelsPermission, [editThreadDescription]: createAndEditChannelsPermission, [descendantEditThreadDescription]: createAndEditChannelsPermission, [editThreadColor]: createAndEditChannelsPermission, [descendantEditThreadColor]: createAndEditChannelsPermission, [createSubchannels]: createAndEditChannelsPermission, [descendantCreateSubchannels]: createAndEditChannelsPermission, [editThreadAvatar]: createAndEditChannelsPermission, [descendantEditThreadAvatar]: createAndEditChannelsPermission, )}
the helper method will make it easier to retrieve the associated keys for a given user-facing permission. So when a user selects the "Allows members to create new and edit existing channels" enum option, we can retrieve edit_thread_name, descendant_edit_thread_name, edit_thread_color, descendant_edit_thread_color, etc.
native/roles/create-roles-screen.react.js | ||
---|---|---|
115 ↗ | (On Diff #28418) | This is a Set so I used the ... operator to map over it |
After our discussion over chat, I think the new conclusion is that the client code for creating / managing roles shouldn't think about threadPermissions at all, and should instead use a new enum that reflects the list of user-facing permissions. The keyserver will handle converting from the new enum of user-facing permissions to threadPermissions... so some version of this data structure will likely still be necessary, but it will probably be keyserver-specific, and the format of the data structure might be different given differing requirements. Does that sound right to you @rohan?
lib/types/thread-permission-types.js | ||
---|---|---|
220–221 ↗ | (On Diff #28418) | We should type this as $ReadOnly |
224 ↗ | (On Diff #28418) | We should type this as $ReadOnly |
native/roles/create-roles-screen.react.js | ||
56 ↗ | (On Diff #28418) | Does TouchableOpacity have a disabled prop? It would probably be less expensive to use that, instead of unmounting / remounting TouchableOpacity when it becomes enabled / disabled |
Yes this sounds right, if we don't want the client to have to be aware of / think of threadPermissions, we could do the following:
- On the client, only use configurableCommunityPermissionsOptions (this is the Set created from configurableCommunityPermissions, with the user-selectable options, instead of using configurableCommunityPermissions which is a mapping of threadPermissions / threadPermissionPropagationPrefixes + threadPermissions to user-facing selectable options.
- Pass in all selected options as an array of ConfigurableCommunityPermissionOption, instead of a list of strings representing threadPermissions to the keyserver. Essentially, the keyserver would receive an array of:
[ { title: 'React to messages', description: 'Allows members to add reactions to messages' }, { title: 'Add members', description: 'Allows members to add other members to channels', } ]
- In role-creator.js, we use configurableCommunityPermissions to derive all of the associated permissions with these selected options using the new helper getKeysByValue, i.e [react_to_message, descendant_react_to_message, add_members, descendant_add_members].
This would address the separation of client-side interaction and thread permissions, but also use a predefined enum to create the mapping and validate the input on the keyserver. Does this approach address your concerns?
native/roles/create-roles-screen.react.js | ||
---|---|---|
56 ↗ | (On Diff #28418) | Looks like it does actually, I didn't know that |
- Pass in all selected options as an array of ConfigurableCommunityPermissionOption, instead of a list of strings representing threadPermissions to the keyserver. Essentially, the keyserver would receive an array of:
[ { title: 'React to messages', description: 'Allows members to add reactions to messages' }, { title: 'Add members', description: 'Allows members to add other members to channels', } ]
This doesn't make sense to me. Why are you passing user-facing copy in an API request? Why aren't you using the aforementioned new enum instead?
Currently, I have one new enum (configurableCommunityPermissions), and a set derived from that enum (configurableCommunityPermissionsOptions). configurableCommunityPermissions like we discussed is in the format of one permission -> one user-facing option.
As of right now, I'm mapping over each user-facing option in the create-roles screen with configurableCommunityPermissionsOptions, and then when one is selected, using the configurableCommunityPermissions enum to get the associated permissions. Once the 'Create' button is clicked, I pass all selected permissions derived from the enum to the keyserver, validate it, and then insert into the DB.
Per our discussion / your feedback, you mentioned "The keyserver will handle converting from the new enum of user-facing permissions to threadPermissions". So my understanding of your request is to gather the selected user-facing options and use the new enum configurableCommunityPermissions to map from selected options --> specific threadPermissions on the keyserver rather than the client.
I skimmed through the last comment, as it's been a lot of back-and-forth recently and I want to move quickly here. Please keep that in mind, as I might've missed something.
But overall it seems like we're still not on the same page. Today we have an enum called threadPermissions. You seem to want to use that enum for client / keyserver communication. Instead, we want to make sure we don't use threadPermissions for client / keyserver communication, and instead use a new enum. It's important that you don't confuse "enum" with "data structure"... you seem to think the new structures you've introduced count as an enum, but that's not how I'm using the term "enum".
We need a new enum that is separate and distinct from threadPermission, is defined in a similar way, and does NOT reference threadPermissions in any way, and does NOT have some sort of data structure or something. It's just a simple enum.
Introduce new enum & address overall feedback about client/keyserver communication design
lib/types/thread-permission-types.js | ||
---|---|---|
89 | Maybe this could alternatively be called EDIT_COMMUNITY_CALENDAR | |
272–315 | Although this is used in D8420, I had defined all of these permissions/prefix+permission combinations here, so I either had to remove them and redefine them in D8420 alongside configurableCommunityPermissions, or just define configurableCommunityPermissions in this diff. I chose to define configurableCommunityPermissions now and use it in the next diff, but if a reviewer feels strongly about defining this and all of the prefix+permissions in the next diff, I can do that as well |
Nice, I think the API has been figured out! Some comments inline, but none of them should require a big refactor
lib/types/thread-permission-types.js | ||
---|---|---|
88 | Can you add a code comment explaining what this is used for, and why it is different from threadPermissions? Make sure to talk about how it's per-community rather than per-thread, and how threadPermissions is the "real" system used on the keyserver for permissions checks, but userSurfacedPermissions is what we show in the UI | |
89 | That might be better, but maybe just EDIT_CALENDAR? (I feel like "community" is implied, since these permissions are all per-community) | |
90 | I think this should be KNOW_OF_SECRET_CHANNELS, to match threadPermissions language, and to reflect that it affects KNOW_OF, which is an extra-important permission (it's special-cased in a couple of places) | |
91 | Can we use the term "voiced" instead of "speak"? It matches threadPermissions language. (Originally pulled from IRC lingo) | |
272–315 | Thanks for explaining! I'm a little confused why this structure needs to exist in this format... I asked you to reimplement it as a map from "thread permission strings" (eg. descendant_add_members) when we were using those strings in the API, but now that we're no longer doing that, I'm not clear on why it's necessary I think we could simplify D8420 by making this a map from UserSurfacedPermission to a list of "permission strings" (eg. descendant_edit_entries) | |
native/roles/create-roles-screen.react.js | ||
35 | We typically don't capitalize the second word like this | |
37 | React state should always be $ReadOnly. It's considered bad practice to directly mutate React state, since direct mutations won't trigger rerenders the same way that setState calls do | |
75 | It looks like this if check is looking at selectedPermissions This has the same issue that I previously shared feedback on here We should be able to implement this without forcing onEnumValuePress to recalculate every time selectedPermissions changes Let me know if I'm missing something | |
153 | This is not portable for light mode. We should pretty much never use whiteText. It exists for things like overlaying text over user-generated images. I think you probably want panelForegroundLabel |
native/roles/create-roles-screen.react.js | ||
---|---|---|
75 | It should be doable, do you mean something like this? const onEnumValuePress = React.useCallback( (option: UserSurfacedPermissionOption) => setSelectedPermissions(currentPermissions => { if (currentPermissions.includes(option.userSurfacedPermission)) { return currentPermissions.filter( permission => permission !== option.userSurfacedPermission, ); } else { return [...currentPermissions, option.userSurfacedPermission]; } }), [], ); |
native/roles/create-roles-screen.react.js | ||
---|---|---|
75 | Yeah exactly! |
- Add a code comment explaining what userSurfacedPermissions is used for, and how it is different from threadPermissions
- Rename some user-surfaced permissions / user-facing permission options to address feedback
- Recreate configurableCommunityOptions as a mapping between UserSurfacedPermission and a set of permission strings
- Fix capitalization of 'New Role' --> 'New role'
- Make React state $ReadOnly
- Implement onEnumValuePress without needing it to recalculate everytime selectedPermissions changes
- Change whiteText --> panelForegroundLabel
After some testing, two changes need to be made:
In this diff, we need to add the descendant_join_thread permission if a role is granted the KNOW_OF_SECRET_CHANNELS user-facing permission. This is because if they can see the channel, they should be to join it.
diff --git a/lib/types/thread-permission-types.js b/lib/types/thread-permission-types.js index 198d010d2..4ff79aed3 100644 --- a/lib/types/thread-permission-types.js +++ b/lib/types/thread-permission-types.js @@ -135,9 +135,13 @@ const descendantKnowOf = threadPermissionPropagationPrefixes.DESCENDANT + threadPermissions.KNOW_OF; const descendantVisible = threadPermissionPropagationPrefixes.DESCENDANT + threadPermissions.VISIBLE; +const descendantJoinThread = + threadPermissionPropagationPrefixes.DESCENDANT + + threadPermissions.JOIN_THREAD; const knowOfSecretChannelsPermissions = new Set([ descendantKnowOf, descendantVisible, + descendantJoinThread, ]);
The second change is in the next diff when we introduce universalCommunityPermissions, where we need to change descendant_voiced to descendant_open_voiced. This is because for those who have the ability to see secret channels, if they just have voiced permissions, they can bypass needing to join the thread and can just start speaking. Adding this permission means they will have to first join, then they will have the ability to be voiced.
More testing followups:
Add the following permission strings to the user-surfaced permissions:
ADD_MEMBERS —> child_open_add_members
KNOW_OF_SECRET_CHANNELS —> child_join_thread, descendant_toplevel_join_thread
CREATE_AND_EDIT_CHANNELS —> descendant_toplevel_create_sidebars
Filter out userSurfacedPermissions.VOICED_IN_ANNOUNCEMENT_CHANNELS if the thread type is not a community announcement root, since we
guarantee voiced permissions on the keyserver if this is the case.
Just applying some feedback recieved for web here to keep things consistent:
- Filtered the user surfaced permission options in a hook in lib so I can reuse it on web
- In preparation to address web feedback, changed the selected options to a $ReadOnlySet instead of a $ReadOnlyArray