Details

Reviewers

ashoat
tomek
varun
ginsu

Commits

rCOMM92a8efadedbf: [lib] Replace `threadMemberHasPermission` logic in `appendUserInfo`

Summary

Instead of checking KNOW_OF using threadMemberHasPermission which requires member.permissions for all members, we use the member's role permissions to determine if they have KNOW_OF.

This was the one usage of threadMemberHasPermission, so now it can be fully removed as well.

Depends on D12158

Test Plan

Added log statements at each step and ensured that values were as expected:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.May 31 2024, 3:14 PM

stage all

atul added inline comments.May 31 2024, 3:17 PM

lib/shared/search-utils.js
76 ↗	(On Diff #40816)	Will remove these comments before landing.

atul edited the test plan for this revision. (Show Details)May 31 2024, 3:18 PM

Harbormaster returned this revision to the author for changes because remote builds failed.May 31 2024, 3:18 PM

Harbormaster failed remote builds in B29336: Diff 40816!

atul added inline comments.May 31 2024, 3:20 PM

lib/shared/search-utils.js
93–94 ↗	(On Diff #40816)	Given that this is a role permission instead of computed thread permission, I probably want to use `parseThreadPermissionString` to check if the base permission of any role permission is `KNOW_OF`? Will need to do a little more reading since I don't remember details of the role permission -> computed thread permission logic. Would be good to hear from @ashoat/@tomek if they know off the top of their head.

Harbormaster completed remote builds in B29336: Diff 40816.May 31 2024, 3:32 PM

atul requested review of this revision.May 31 2024, 3:32 PM

Harbormaster completed remote builds in B29335: Diff 40815.May 31 2024, 3:34 PM

atul added a child revision: D12265: [lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions`.Jun 1 2024, 9:21 PM

ashoat accepted this revision.Jun 2 2024, 1:55 PM

ashoat added inline comments.

lib/shared/search-utils.js
93–94 ↗	(On Diff #40816)	Hmm, good question. I actually don't think using `parseThreadPermissionString` here is necessary... looking at the code in `lib/permissions/thread-permissions.js`, it seems like we do that conversion when generating permissions for a child thread from a parent, and not when checking the permissions of a given thread. A good way to confirm this would be to just check if admin and non-admin members of a community root thread in your local environment have `know_of` in their permissions blob, and not just eg. `descendant_know_of`.

This revision is now accepted and ready to land.Jun 2 2024, 1:55 PM

atul added inline comments.Jun 11 2024, 1:01 PM

lib/shared/search-utils.js
93–94 ↗	(On Diff #40816)	A good way to confirm this would be to just check if admin and non-admin members of a community root thread in your local environment have `know_of` in their permissions blob, and not just eg. `descendant_know_of`. Confirmed that for admin member: And for non-admin member: The `know_of` bit ( which is least significant) of binary representation was set to true.