Page MenuHomePhabricator

[native] Populate the create role screen
ClosedPublic

Authored by rohan on Jun 30 2023, 7:54 AM.
Tags
None
Referenced Files
F2064328: D8391.id28296.diff
Fri, Jun 21, 6:33 AM
F2056886: D8391.id28500.diff
Thu, Jun 20, 2:13 PM
Unknown Object (File)
Thu, Jun 20, 1:05 PM
Unknown Object (File)
Thu, Jun 20, 4:43 AM
Unknown Object (File)
Thu, Jun 20, 3:19 AM
Unknown Object (File)
Wed, Jun 19, 3:36 PM
Unknown Object (File)
Wed, Jun 19, 2:01 PM
Unknown Object (File)
Wed, Jun 19, 11:12 AM
Subscribers

Details

Summary

This diff adds the necessary content to the create roles screen, i.e. a role name input and a panel of selectable permissions for the given role. The onSave functionality will be added in a later
diff, as well as the removal of the eslint-disable-next-line no-unused-vars.

Depends on D8379

ENG-4172

Test Plan

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  1. Pass in an isEmpty flag instead of the entire selectedPermissions array in the clearPermissionsText memo
  1. Pass a function to the setter in onEnumValuePress
  1. Add the defintion of configurableCommunityPermissions in this diff since this is where it is used

Remove console.log statement

ashoat requested changes to this revision.Jul 3 2023, 11:31 AM

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:

  1. 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.
  2. 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,
];
This revision now requires changes to proceed.Jul 3 2023, 11:31 AM
  1. 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.
  1. 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

ashoat requested changes to this revision.Jul 5 2023, 10:52 AM

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

This revision now requires changes to proceed.Jul 5 2023, 10:52 AM

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?

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:

  1. 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.
  1. 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',
    }
]
  1. 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

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

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 ↗(On Diff #28427)

Maybe this could alternatively be called EDIT_COMMUNITY_CALENDAR

272–315 ↗(On Diff #28427)

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

ashoat requested changes to this revision.Jul 6 2023, 12:25 PM

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 ↗(On Diff #28427)

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 ↗(On Diff #28427)

That might be better, but maybe just EDIT_CALENDAR? (I feel like "community" is implied, since these permissions are all per-community)

90 ↗(On Diff #28427)

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 ↗(On Diff #28427)

Can we use the term "voiced" instead of "speak"? It matches threadPermissions language. (Originally pulled from IRC lingo)

272–315 ↗(On Diff #28427)

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 ↗(On Diff #28427)

We typically don't capitalize the second word like this

37 ↗(On Diff #28427)

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 ↗(On Diff #28427)

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 ↗(On Diff #28427)

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

This revision now requires changes to proceed.Jul 6 2023, 12:25 PM
rohan marked 8 inline comments as done.Jul 6 2023, 2:52 PM
rohan added inline comments.
native/roles/create-roles-screen.react.js
75 ↗(On Diff #28427)

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 ↗(On Diff #28427)

Yeah exactly!

  1. Add a code comment explaining what userSurfacedPermissions is used for, and how it is different from threadPermissions
  2. Rename some user-surfaced permissions / user-facing permission options to address feedback
  3. Recreate configurableCommunityOptions as a mapping between UserSurfacedPermission and a set of permission strings
  4. Fix capitalization of 'New Role' --> 'New role'
  5. Make React state $ReadOnly
  6. Implement onEnumValuePress without needing it to recalculate everytime selectedPermissions changes
  7. Change whiteText --> panelForegroundLabel
ashoat added inline comments.
lib/types/thread-permission-types.js
96–99 ↗(On Diff #28486)

The line "When a new permission is added" should probably appear above threadPermissions, since that's where the new permission will be added

145 ↗(On Diff #28486)

Let's make it clearer in the description instead of repeating the same term

This revision is now accepted and ready to land.Jul 7 2023, 12:08 PM

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:

  1. Filtered the user surfaced permission options in a hook in lib so I can reuse it on web
  2. In preparation to address web feedback, changed the selected options to a $ReadOnlySet instead of a $ReadOnlyArray