Page MenuHomePhabricator

[lib] Add `memberHasAdminRole` check to `useThreadsWithPermission`
ClosedPublic

Authored by atul on May 21 2024, 7:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Oct 18, 6:50 PM
Unknown Object (File)
Fri, Oct 18, 6:15 AM
Unknown Object (File)
Fri, Oct 18, 1:17 AM
Unknown Object (File)
Thu, Oct 17, 11:32 AM
Unknown Object (File)
Tue, Oct 15, 2:53 AM
Unknown Object (File)
Sat, Oct 12, 8:34 AM
Unknown Object (File)
Fri, Oct 11, 4:36 PM
Unknown Object (File)
Fri, Oct 11, 4:36 PM
Subscribers
None

Details

Summary

The logic here is the "updated version" of the threadOrParentThreadHasAdminRole early return check in threadIsWithBlockedUserOnly.

Rather than depending on the "sketchy CHANGE_ROLE check" that we were using in memberHasAdminPowers, we use the updated logic where we check if there's a member that has admin role in community root.

If there is, we know that we don't need to patch/filter permissions.

NOTE: If there isn't a member that has admin role, we continue with the blocked relationship checks in threadFrozenDueToBlock. We will need to modify threadFrozenDueToBlock so that we can skip the redundant threadOrParentThreadHasAdminRole check that uses the old logic. We'll probably add something like a skipAdminRoleCheck argument since we're handling that using the new logic BEFORE the call to threadIsWithBlockedUserOnly

Depends on D12145

Test Plan
  1. Added log statements in useThreadsWithPermission that included thread name, permission name, and whether permission was enabled or disabled. They looked like the following:

7dfd05.png (766×1 px, 338 KB)

  1. Used the app and navigated to a bunch of threads/screens to build up logs.
  1. Created script to compare logs from before and after the change to ensure that the values of permission for given thread remained same:

https://gist.github.com/atulsmadhugiri/3f1ac163e0f53b46f496001d4cedda3c

  1. Ran script and ensured that there were no differences (see output on bottom right):

a51118.png (1×2 px, 672 KB)

(In hindsight I should've just logged threadName, permissionName, boolean as "CSV" so it would be super simple to split on comma, but had already collected large volume of logs)

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D12149 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.May 21 2024, 8:04 AM

i'm probably not a good reviewer of this code

Sorry it took so long to review these

lib/shared/thread-utils.js
208–218 ↗(On Diff #40478)

Some minor adjustments I'd propose

This revision is now accepted and ready to land.May 24 2024, 2:51 AM

address feedback before updating test plan and before landing

fix logic to handle undefined case

This revision was landed with ongoing or failed builds.May 29 2024, 8:30 PM
This revision was automatically updated to reflect the committed changes.