[lib] Add unit tests for user-facing permissions to verify the result
ClosedPublic
Actions

Authored by rohan on Jul 11 2023, 2:41 PM.

Details

Reviewers

atul
ginsu

Commits

rCOMMad673e2bd253: [lib] Add unit tests for user-facing permissions to verify the result

Summary

A couple of unit tests that verifies the permissions from getRolePermissionBlobs for Members match the thread permissions associated with user-surfaced permissions that Members would have. For admins, this verifies that the compilation of all universalCommunityPermissions and thread permissions associated with all user-surfaced permissions matched the permissions blob.

It made the most sense to check the Members for both COMMUNITY_ROOT and COMMUNITY_ANNOUNCEMENT_ROOT, but the Admins permissions blob should remain the same so I only did that once.

Originally brought up in D8380

ENG-4314

Depends on D8445

Test Plan

yarn workspace lib test - all tests pass

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Jul 11 2023, 2:41 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 11 2023, 2:41 PM

rohan edited the summary of this revision. (Show Details)Jul 11 2023, 2:41 PM

Harbormaster completed remote builds in B20883: Diff 28605.Jul 11 2023, 2:58 PM

rohan requested review of this revision.Jul 11 2023, 2:58 PM

rohan added a child revision: D8502: [keyserver/lib] Update the change_role message spec to include a role name field in the DB.Jul 14 2023, 1:34 PM

rohan added a child revision: D8540: [keyserver/lib] Make shimUnsupportedMessageInfo and shimUnsupportedRawMessageInfos async functions.Jul 18 2023, 3:10 PM

rohan removed a child revision: D8502: [keyserver/lib] Update the change_role message spec to include a role name field in the DB.Jul 18 2023, 3:11 PM

seems good, but would feel better if @atul could also take a look too when he's back

atul accepted this revision.Jul 23 2023, 2:10 PM

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

rohan removed a child revision: D8540: [keyserver/lib] Make shimUnsupportedMessageInfo and shimUnsupportedRawMessageInfos async functions.Jul 26 2023, 9:29 AM

rohan added a child revision: D8502: [keyserver/lib] Update the change_role message spec to include a role name field in the DB.

ashoat added inline comments.Jul 27 2023, 1:03 PM

lib/types/thread-permission-types.test.js
57 ↗	(On Diff #28605)	That diff is long and I'm having a hard time finding the context Do Markdown-formatted comments render in your IDE? Confused why you're using Markdown here Weird spacing issues... two spaces after period, and line 62 has an extraneous space at the start I'm struggling to make sense of your comment. You seem to be explaining why we set `descendant_open_voiced` instead of `descendant_voiced`, but I don't understand why `adminsPermissionsBlob` has `descendant_voiced`, or more generally why these two don't match

rohan added inline comments.Jul 28 2023, 8:32 AM

lib/types/thread-permission-types.test.js
57 ↗	(On Diff #28605)	Sorry, I'll pull the exact comment here for what the context was 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. No I originally was going to annotate this diff but thought it better to leave an inline code comment instead for future visibility. I can remove the markdown formatting and just link the diff Will fix that In the current app, admins (like you), can see secret channels in the community, and they don't have to 'join' the thread to start speaking (this would because they have `descendant_know_of`). While this makes sense for `Admins`, I thought it was more ideal for people who have the `know_of_secret_channels` user-surfaced permission to have to still join them in order to speak in them. If you'd prefer for the behavior to match `Admins`, and they talk in secret channels without having to join them, it should just be a one line change in the `guaranteedCommunityPermissions` in D8420, adding `descendant_voiced`. If so, in theory this would mean the two blobs will be identical and I would expect the `deepDiff` to be `{}`.

rohan attached a referenced file: F662175: Screenshot 2023-07-28 at 11.27.14 AM.png. (Show Details)Jul 28 2023, 8:32 AM

ashoat added inline comments.Jul 28 2023, 1:11 PM

lib/types/thread-permission-types.test.js
57 ↗	(On Diff #28605)	Thanks for explaining! Can you just link this diff comment in the code? https://phab.comm.dev/D8478#inline-55680

atul added inline comments.Jul 28 2023, 1:29 PM

lib/types/thread-permission-types.test.js
57 ↗	(On Diff #28605)	FWIW VS Code and Jetbrains IDEs do render Markdown comments in various contexts (eg when hovering over symbol annotated w/ comment) However, they won't render markdown in editor unless in "Reader mode" (which is usually off by default for files in repo, but on by default for libraries/imported deps):