Details

Reviewers

Commits

Summary

RoleInfo type with permissions as $ReadOnlyArray<string> instead of ThreadRolePermissionsBlob.

Next diffs:

Introduce MinimallyEncodedThreadCurrentUserInfo
Introduce MinimallyEncodedRawThreadInfo
Higher level utilities for translating back/forth from RawThreadInfo <=> MinimallyEncodedRawThreadInfo. Found this to be cleaner API than encoding/decoding RawThreadInfo.currentUser.permissions and RawThreadInfo.members[memberID].permissions "manually" a bunch of different places.
Native refactoring + migrations
Web refactoring
Flipping the switch

Test Plan

Added some unit tests, will consume types in subsequent diffs.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

atul published this revision for review.Nov 6 2023, 9:46 AM

I don't understand why you're using word boundaries

lib/permissions/minimally-encoded-thread-permissions.js
192 ↗	(On Diff #32818)	What's the point of using these word boundaries instead of `^` and `$`? It seems like the word boundaries also would allow weird input like `hello a02 test`

This revision now requires changes to proceed.Nov 6 2023, 11:33 AM

lib/permissions/minimally-encoded-thread-permissions.js
192 ↗	(On Diff #32818)	Sorry, should be `^` and `$`. Will update and add failing unit test for `hello a02 test`

update regex to use ^ and $ instead of \b

was experimenting w/ \b when doing the "multiple 12-bit chunks in one bitmask" approach and failed to updated the boundaries

add failing hello a02 test case

ashoat added inline comments.

lib/permissions/minimally-encoded-thread-permissions.js
192	I think it might be a good idea to allow longer strings so that old clients don't break if in the future they get a longer input

This revision is now accepted and ready to land.Nov 6 2023, 11:43 AM

lib/permissions/minimally-encoded-thread-permissions.js
192	That's fair, the intention was that things should start failing when 65th permission is added to `baseRolePermissionEncoding`... but making this validator break may not be the right way to achieve that. Will update diff.