Page MenuHomePhabricator

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

Authored by rohan on Jul 11 2023, 2:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 7:59 PM
Unknown Object (File)
Wed, Nov 6, 7:34 PM
Unknown Object (File)
Wed, Oct 16, 9:18 PM
Unknown Object (File)
Wed, Oct 16, 9:17 PM
Unknown Object (File)
Wed, Oct 16, 9:17 PM
Unknown Object (File)
Wed, Oct 16, 9:17 PM
Unknown Object (File)
Wed, Oct 16, 9:17 PM
Unknown Object (File)
Wed, Oct 16, 9:13 PM
Subscribers

Details

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 requested review of this revision.Jul 11 2023, 2:58 PM
ginsu added 1 blocking reviewer(s): atul.

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

This revision is now accepted and ready to land.Jul 23 2023, 2:10 PM
lib/types/thread-permission-types.test.js
57 ↗(On Diff #28605)
  1. That diff is long and I'm having a hard time finding the context
  2. Do Markdown-formatted comments render in your IDE? Confused why you're using Markdown here
  3. Weird spacing issues... two spaces after period, and line 62 has an extraneous space at the start
  4. 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
lib/types/thread-permission-types.test.js
57 ↗(On Diff #28605)
  1. 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.

  1. 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
  1. Will fix that
  1. 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.

Screenshot 2023-07-28 at 11.27.14 AM.png (1×2 px, 238 KB)

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 {}.

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

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)

ce082a.png (208×1 px, 65 KB)

Screenshot 2023-07-28 at 1.22.31 PM.png (772×1 px, 208 KB)

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):

d7a3e3.png (1×1 px, 265 KB)

Update comment to just link to the Phabricator context