Page MenuHomePhabricator

[lib] Introduce `useThreadHasPermission` hook
ClosedPublic

Authored by atul on Tue, Apr 23, 12:29 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 1, 11:37 PM
Unknown Object (File)
Mon, Apr 29, 11:59 AM
Unknown Object (File)
Mon, Apr 29, 10:38 AM
Unknown Object (File)
Sun, Apr 28, 12:36 PM
Unknown Object (File)
Sun, Apr 28, 9:59 AM
Unknown Object (File)
Sat, Apr 27, 3:10 PM
Unknown Object (File)
Tue, Apr 23, 8:58 PM
Unknown Object (File)
Tue, Apr 23, 8:57 PM
Subscribers
None

Details

Summary
  1. Currently, getCurrentUser handles mutating currentUser.permissions if threadFrozenDueToBlock(...) is true.
  2. The current threadFrozenDueToBlock(...) logic depends on memberHasAdminPowers which depends on sketchy CHANGE_ROLE check.
  3. We want to update memberHasAdminPowers to instead determine whether member is admin based on whether they have ADMIN_ROLE in community thread.
  4. Determining whether a member has ADMIN_ROLE in community thread is straightforward from React context, but not practical from non-React context.
  5. As a result, we want to move threadFrozenDueToBlock from getCurrentUser to useThreadHasPermission where we can compute derived permissions "on demand" from React context.
  6. As part of that, we're going to update threadHasPermission callsites ON CLIENT (ThreadInfo only) to use useThreadHasPermission.
  7. Once we've refactored all callsites to be able to use useThreadHasPermission, we will update it to use an updated version of threadFrozenDueToBlock which looks at community thread to determine ADMIN_ROLE using specialRole vs. sketchy CHANGE_ROLE check.

This diff introduces a "hook version" of threadHasPermission that can be used throughout client components. It only handles ThreadInfo and handles filtering of disabled permissions using threadFrozenDueToBlock check.

The next diffs in the stack aim to update all client permission checks to use this hook so we can remove the permission mutation logic from getCurrentUser.


Depends on D11742

Test Plan

Added log statements to tooltipActionUtils to ensure that result of useThreadHasPermission and threadHasPermission are identical.

Will also test block scenario to ensure that permissions are correctly filtered out.

Diff Detail

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

Event Timeline

Adding @ashoat as reviewer since we spent time discussing this approach.

lib/shared/thread-utils.js
165–171 ↗(On Diff #39423)

This filtering is currently handled in getCurrentUser:

f6c698.png (680×1 px, 122 KB)

so it's redundant/noop for now until we replace all checks to use useThreadHasPermission and can remove the logic from getCurrentUser.

atul requested review of this revision.Tue, Apr 23, 12:51 PM
tomek added inline comments.
lib/shared/thread-utils.js
165–173 ↗(On Diff #39423)

Depending on how expensive these checks are, we can consider memoizing permissions and hasPermission result.

This revision is now accepted and ready to land.Wed, Apr 24, 4:38 AM
lib/shared/thread-utils.js
165–173 ↗(On Diff #39423)

I think it makes sense to memoize. Also to memoize permissions above to potentially avoid call to filterOutDisabledPermissions(...).

Will update this diff.

rebase before addressing feedback

memoize return value of useThreadHasPermission

This revision was landed with ongoing or failed builds.Wed, Apr 24, 1:00 PM
This revision was automatically updated to reflect the committed changes.