Page MenuHomePhabricator

[lib] Replace `filterThreadEditDetailedPermissions` with `filterThreadPermissions`
ClosedPublic

Authored by atul on Apr 7 2023, 1:14 PM.
Tags
None
Referenced Files
F3356653: D7363.diff
Sat, Nov 23, 7:48 PM
Unknown Object (File)
Wed, Nov 20, 7:11 PM
Unknown Object (File)
Sun, Nov 10, 7:21 PM
Unknown Object (File)
Sun, Nov 10, 7:08 PM
Unknown Object (File)
Sun, Nov 10, 2:15 PM
Unknown Object (File)
Sun, Nov 10, 1:56 PM
Unknown Object (File)
Sun, Nov 10, 1:14 PM
Unknown Object (File)
Sun, Nov 10, 7:18 AM
Subscribers

Details

Summary

We were previously filtering out some permissions using filterThreadEditDetailedPermissions for older clients.

Similarly, we want to filter out edit_thread_avatar permissions for old clients. The current filterThreadEditDetailedPermissions only handles one particlar filtering.

We could just add another filterThreadEditAvatarPermission function that similarly takes (ThreadPermissionsInfo, boolean), but then things get messy at the callsite where we have to run permissions through two separate filtering functions. This might be fine for now... but it's not super extensible when we add further permissions.

We could separately create a function that handles all of the filtering and takes a bunch of boolean arguments for each set of permissions that we might want to exclude. This would also kind of get messy as every time we add a permission we'll need to change the function signature and all of the callsites.

Instead, I think the cleanest solution is to construct a filterThreadPermissions directly in rawThreadInfoFromServerThreadInfo based on the RawThreadInfoOptions that have been passed in and use that filterThreadPermissions function throughout rawThreadInfoFromServerThreadInfo. It also keeps everything "in one place," which I think makes things more extensible (next diff will extend filterThreadPermissions for filtering avatar permissions).

Test Plan

Manually set filterDetailedThreadEditPermissions and log the filtered return value of filterThreadPermissions to ensure that things are being filtered out as expected.

Also check D7364 for more thorough testing with filterThreadEditAvatarPermission.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Apr 7 2023, 1:22 PM
atul edited the test plan for this revision. (Show Details)

Accepting but I think my comment needs to be addressed before landing

lib/shared/thread-utils.js
799 ↗(On Diff #24837)

We didn't do this originally, but we should've: roles also needs to be filtered, as there's a permissions blob in there. If we don't do this then the work is for naught, as the RawThreadInfo will still be different between client/server

This revision is now accepted and ready to land.Apr 7 2023, 5:27 PM
lib/shared/thread-utils.js
799 ↗(On Diff #24837)

Ah yeah thanks for noting that, I noticed this yesterday when I was looking at Redux store and was going to ask if we should do the same.

I'll update this diff in place before landing (and after further testing).

rebase before addressing feedback

address feedback so roles has permissions filtered out

ServerThreadInfo (has thread_edit_avatar):

b0befc.png (1×1 px, 284 KB)

ThreadInfo (doesn't have thread_edit_avatar):

5c2592.png (1×1 px, 365 KB)