Details

Reviewers

ashoat
ginsu
tomek
varun

Commits

rCOMM5922947499f8: [lib] Remove `getCurrentUser`

Summary

We needed getCurrentUser in order to "patch" permissions based on block. However, we're now considering filtered permissions as part of useThreadsWithPermission so we can handle blocks as part of permission check vs. RawThreadInfo -> ThreadInfo. This greatly simplifies deprecation of memberHasAdminPowers on the client.

Depends on D12154

Test Plan

flow + ensuring that useExistingThreadInfoFinder, threadInfoFromRawThreadInfo, etc. continue to work as expected.

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.May 21 2024, 9:06 AM

atul published this revision for review.May 21 2024, 9:06 AM

Harbormaster completed remote builds in B29123: Diff 40491.May 21 2024, 10:00 AM

i'm probably not a good reviewer of this code

Great to see this!

I'd like to understand a bit more why this is safe to do at this time. I tried looking through your Linear task tree, but couldn't find where this is tracked.

Can you share where you're tracking all permissions checks on the client (ideally on Linear), so I can confirm that they're all updated to use hooks that check block status?

This revision now requires changes to proceed.May 24 2024, 3:07 AM

atul added a child revision: D12245: [lib] Pull `threadPermissionsDisabledByBlock` out from `threadPermissions`.May 29 2024, 11:19 PM

atul mentioned this in D12246: [lib] Narrow `threadHasPermission` so it only accepts `ThreadPermissionNotAffectedByBlock`.May 29 2024, 11:32 PM

rebase before addressing feedback

atul planned changes to this revision.May 30 2024, 11:25 AM

Harbormaster completed remote builds in B29314: Diff 40787.May 30 2024, 11:40 AM

atul mentioned this in rCOMMa9069cb74360: [lib] Narrow `threadHasPermission` so it only accepts….May 30 2024, 12:45 PM

I'd like to understand a bit more why this is safe to do at this time.

We should be confident in this change after flow changes in D12246. Let me know if there's any questions or details I can clarify.

I tried looking through your Linear task tree, but couldn't find where this is tracked.

Can you share where you're tracking all permissions checks on the client (ideally on Linear), so I can confirm that they're all updated to use hooks that check block status?

Going to go through and update Linear task tree right now.

atul requested review of this revision.May 30 2024, 1:27 PM

update and amend

atul added inline comments.May 30 2024, 7:25 PM

lib/shared/thread-utils.js
1160–1163 ↗	(On Diff #40798)	This can just be `return updatedThread` now.

Harbormaster completed remote builds in B29323: Diff 40798.May 30 2024, 7:27 PM

remove unnecessary spread

Harbormaster completed remote builds in B29324: Diff 40799.May 30 2024, 8:00 PM

atul added a child revision: D12254: [lib] Fix `containingThreadID` typo in `usePotentialMemberItems`.May 30 2024, 8:37 PM

Thanks @atul. I'm still a little confused, so three more follow-up questions:

Can you share where you're tracking all permissions checks on the client (ideally on Linear), so I can confirm that they're all updated to use hooks that check block status?
I found a client-related task on Linear that was listed as in-progress. I left a small question on Linear, can you respond there?
Regarding another client-related task on Linear that was listed as in-progress, I sent a message on Comm. Can you respond there?

Passing back to respond to the above

This revision now requires changes to proceed.May 31 2024, 12:08 PM

atul added a child revision: D12259: [lib] Replace `threadMemberHasPermission` logic in `appendUserInfo`.May 31 2024, 3:14 PM

Can you share where you're tracking all permissions checks on the client (ideally on Linear), so I can confirm that they're all updated to use hooks that check block status?

Places that used threadHasPermission that needed to be updated to use useThreadHasPermission are listed here: https://linear.app/comm/issue/ENG-7254/use-hook-instead-of-mutating-permissions-in-getminimallyencodeduser

In addition, we know that all checks that check permissions affected by block are going through useThreadHasPermission because the threadHasPermission function has been updated to only accept ThreadPermissionNotAffectedByBlock: