[lib] Instantiate user-surfaced roles, titles, and descriptions
AbandonedPublic
Actions

Authored by rohan on Jun 29 2023, 1:41 PM.

Details

Reviewers

ashoat
atul
ginsu

Summary

We need to define a set of permissions that will be configurable by admins creating new roles for their community. These are 'user-surfaced' permissions, so we group similar thread permissions together (i.e. edit_thread_color, edit_thread_avatar, etc). There is a distinct separation between configurable permissions and guaranteed permissions, since we need to ensure that every member in a community, despite the role, has a set of default permissions.

The permissions here are based on the existing permissions we have for existing Members and Admins in thread-permissions.js for communities, so that's where I selected what roles should use certain prefixes. Adding @ashoat as blocking since we discussed some of these in our sync & it involves user-facing copy with the titles and descriptions.

Depends on D8379

ENG-4171

Test Plan

The permissions can be tested later in the stack for when new, custom roles are created.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Jun 29 2023, 1:41 PM

Herald added a subscriber: tomek. · View Herald TranscriptJun 29 2023, 1:41 PM

rohan edited the summary of this revision. (Show Details)Jun 29 2023, 1:42 PM

rohan edited the summary of this revision. (Show Details)Jun 29 2023, 1:45 PM

Harbormaster completed remote builds in B20627: Diff 28278.Jun 29 2023, 2:00 PM

rohan requested review of this revision.Jun 29 2023, 2:00 PM

rohan added a child revision: D8391: [native] Populate the create role screen.Jun 30 2023, 7:54 AM

This diff is very difficult to review on its own, as the significance of the format of this collection is not clear until you see the callsite in D8391. In general, it's best to include an initial callsite when introducing a utility function / utility collection like this

Even with D8391, I can't tell how guarenteedCommunityPermissions is used. I also can't tell what eventually happens with the permissions. Do they need to eventually add up to something that matches existing role permissions blobs? As the reviewer, am I expected to manually verify that?

lib/types/thread-permission-types.js
230	Typo
230	I can't find a diff that uses this, so I have no idea how to review this diff. Ideally the initial use case should be inside this diff... having to dig for it in other diffs is already an issue

This revision now requires changes to proceed.Jul 2 2023, 6:58 PM

In D8380#247436, @ashoat wrote:

This diff is very difficult to review on its own, as the significance of the format of this collection is not clear until you see the callsite in D8391. In general, it's best to include an initial callsite when introducing a utility function / utility collection like this

That's fair, I initially split this all up to make things easier to review and to avoid a really big diff, but I'll take this in mind and do the following:

(1) Merge this with D8391, so configurableCommunityPermissions is defined with its callsite.
(2) Introduce guaranteedCommunityPermissions with its callsite

Even with D8391, I can't tell how guarenteedCommunityPermissions is used. I also can't tell what eventually happens with the permissions. Do they need to eventually add up to something that matches existing role permissions blobs? As the reviewer, am I expected to manually verify that?

This will be more clear when the diff with (2) is put up next, but the idea is that we allow certain permissions to be configurable / optional for roles, but we ensure that certain permissions are shared across all roles and cannot be removed / changed (i.e. leave_thread). In the new diff that will introduce both keyserver-support for creating roles and the callsite for guarenteedCommunityPermissions, we replicate the existing permissions format and end up with something like the below with some optionally selected configurableCommunityPermissions and all of the guarenteedCommunityPermissions.

{"know_of":true,"descendant_open_know_of":true,"descendant_voiced":true,"visible":true,"descendant_open_visible":true,"join_thread":true,"child_open_join_thread":true,"descendant_opentoplevel_join_thread":true,"create_sidebars":true,"leave_thread":true}

The best way I will test this is to create several roles with several different permissions (i.e. one role will be allowed to react to messages, while another role can pin and edit messages, etc) and assign them to users to test whether their permissions are correctly gated. I'll be including that with my test plan in the diff that introduces the keyserver support as mentioned above.

configurableCommunityPermissions is now defined in D8391 and guaranteedCommunityPermissions will be defined alongside its callsite in the diff that adds keyserver support for creating new roles

Thanks @rohan! Mostly followed up in D8391, but one other thing I wanted to note here:

In D8380#247703, @rohan wrote:
In D8380#247436, @ashoat wrote:

Even with D8391, I can't tell how guarenteedCommunityPermissions is used. I also can't tell what eventually happens with the permissions. Do they need to eventually add up to something that matches existing role permissions blobs? As the reviewer, am I expected to manually verify that?

This will be more clear when the diff with (2) is put up next, but the idea is that we allow certain permissions to be configurable / optional for roles, but we ensure that certain permissions are shared across all roles and cannot be removed / changed (i.e. leave_thread). In the new diff that will introduce both keyserver-support for creating roles and the callsite for guarenteedCommunityPermissions, we replicate the existing permissions format and end up with something like the below with some optionally selected configurableCommunityPermissions and all of the guarenteedCommunityPermissions.
{"know_of":true,"descendant_open_know_of":true,"descendant_voiced":true,"visible":true,"descendant_open_visible":true,"join_thread":true,"child_open_join_thread":true,"descendant_opentoplevel_join_thread":true,"create_sidebars":true,"leave_thread":true}
The best way I will test this is to create several roles with several different permissions (i.e. one role will be allowed to react to messages, while another role can pin and edit messages, etc) and assign them to users to test whether their permissions are correctly gated. I'll be including that with my test plan in the diff that introduces the keyserver support as mentioned above.

Regarding this, I think it'd be great to introduce a unit test alongside that diff that compiles together a default set of "user-facing permissions" and verifies that the result matches what we have in the existing app today (getRolePermissionBlobs).

In D8380#247887, @ashoat wrote:

Regarding this, I think it'd be great to introduce a unit test alongside that diff that compiles together a default set of "user-facing permissions" and verifies that the result matches what we have in the existing app today (getRolePermissionBlobs).

Sure, I'll do this prior to landing the native stack as the last diff, especially while D8391 and D8420 are still in review

https://linear.app/comm/issue/ENG-4314/add-a-unit-test-for-user-facing-permissions-to-verify-the-result

rohan mentioned this in D8478: [lib] Add unit tests for user-facing permissions to verify the result.Jul 11 2023, 2:41 PM

rohan removed a child revision: D8391: [native] Populate the create role screen.Jul 24 2023, 10:53 AM

rohan removed a parent revision: D8379: [native] Navigate to a screen to create a new role.

rohan mentioned this in rCOMMad673e2bd253: [lib] Add unit tests for user-facing permissions to verify the result.Jul 31 2023, 6:15 PM