Page MenuHomePhabricator

[lib] Narrow `threadHasPermission` so it only accepts `ThreadPermissionNotAffectedByBlock`
ClosedPublic

Authored by atul on May 29 2024, 11:32 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 10, 10:53 PM
Unknown Object (File)
Thu, Oct 10, 10:53 PM
Unknown Object (File)
Thu, Oct 10, 10:53 PM
Unknown Object (File)
Thu, Oct 10, 10:53 PM
Unknown Object (File)
Thu, Oct 10, 10:53 PM
Unknown Object (File)
Thu, Oct 10, 10:53 PM
Unknown Object (File)
Thu, Oct 10, 10:52 PM
Unknown Object (File)
Thu, Oct 10, 10:52 PM
Subscribers
None

Details

Summary

We want permissions which can be disabled by block to go through useThreadHasPermission which calls useThreadsWithPermission which potentially filters out disabled permissions if threadFrozenDueToBlock is true.

However, this additional logic isn't necessary for permissions unaffected by block.

This change gives us confidence that permissions that can be affected by block are "going through" useThreadHasPermission and permission checks going forward where permission may be affected by block won't go through threadHasPermission.

This change also gives us the confidence to unblock D12158 since we know that permission checks for permissions that may be affected by block are being handled via useThreadHasPermission, which means we can avoid potentially filtering out permissions when permissions for ThreadInfo are set in getCurrentUser.


Depends on D12245

Test Plan

flow

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 29 2024, 11:44 PM
Harbormaster failed remote builds in B29302: Diff 40773!
atul requested review of this revision.May 30 2024, 12:03 AM

The high level goal of this work was to remove non-viewer/non-admin computed Member permissions to reduce space taken up by them on the clients.

The member permissions checks go through hasPermission and permissionLookup. These are both consumed by threadHasPermission for current user/viewer permission checks. We will continue sending/persisting computed permissions for current user.

The other two usages of hasPermission are memberHasAdminPowers and threadMemberHasPermission. A majority of work in this stack was related to memberHasAdminPowers which previously required computed permissions for each user in the thread.

The other usage is in threadMemberHasPermission which has a single usage in appendUserInfo which checks the KNOW_OF permission within usePotentialMemberItems. Since we always have communityThreadInfo within usePotentialMemberItems, we can determine whether a member has KNOW_OF based on their role in the community root. Once we switch to using that instead of computed permissions we should be able to filter out non-viewer/non-admin permissions from ThreadInfos. The task to update appendUserInfo is here: https://linear.app/comm/issue/ENG-6770/update-appenduserinfo

This revision is now accepted and ready to land.May 30 2024, 6:43 AM

Ah, got to land D11753 before I can land this one. Will update that diff to get it land-able.

rebase before addressing feedback

now we should be good to land this