[native] Introduce `recursivelyUpdateRoles(ThreadTraversalNode)`
ClosedPublic
Actions

Authored by atul on Apr 25 2023, 1:20 AM.

Details

Reviewers

ashoat
ginsu

Commits

rCOMM392073bfb3dc: [native] Introduce `recursivelyUpdateRoles(ThreadTraversalNode)`

Summary

We introduce recursivelyUpdateRoles(ThreadTraversalNode) which updates threadInfo.roles.permissions corresponding to the node.threadID AND all child threads (node.children) recursively.

NOTE: We're not mutating or updating the ThreadTraversalNodes, we're simply using them for traversal. We ARE, however, mutating the threadInfos "directly" by mutating the ThreadStoreThreadInfos passed into updateRolesAndPermissions by indexing into it w/ node.threadID from the ThreadTraversalNodes.

Depends on D7600

Test Plan

Ran migration on existing threadStore and there was no diff
Added "atul" permission to voiced and observed that it was included thanks to migration: https://blob.sh/6b2616.png
Removed misc permissions and observed that they were excluded after migration

Later in the stack I add test cases to check that recursivelyUpdateRoles makes no changes to a "correct" threadStore retrieved from the keyserver on login AND that recursivelyUpdateRoles can construct correct threadInfo.roles.permission with all existing role permissions removed (AKA can do it from scratch solely based off of getRolePermissionBlobs.

WARNING: Right now we're updating roles.permissions for roles that exist for a given threadType. That means that we can handle updating of Members or Admins if they already exist for a threadType, but we CAN'T add or remove roles to threadInfo.roles... that requires the keyserver in order to generate the roleIDs. So we can't quite build up roles/permissions entirely from scratch as we can on the keyserver... though I'm not sure how likely it is that we add/remove a role for an existing thread type vs creating a new thread type. Either way, in that scenario the developer introducing the change would be responsible for ensuring there are no inconsistencies and that may involve additional work.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Apr 25 2023, 1:20 AM

Herald added a subscriber: tomek. · View Herald TranscriptApr 25 2023, 1:20 AM

atul edited the summary of this revision. (Show Details)Apr 25 2023, 1:23 AM

atul edited the test plan for this revision. (Show Details)Apr 25 2023, 1:27 AM

Harbormaster completed remote builds in B18724: Diff 25646.Apr 25 2023, 1:37 AM

atul requested review of this revision.Apr 25 2023, 1:37 AM

atul added a child revision: D7602: [native] Introduce `recursivelyUpdatePermissions(ThreadTraversalNode)`.Apr 25 2023, 1:41 AM

Please address inline comments before landing! If you feel strongly that the data types should not be read-only, can you please re-request review?

native/redux/update-roles-and-permissions.js
46–52 ↗	(On Diff #25646)	This mutates the `threadStoreInfos` passed to us. This is only possible due to an oversight: In `lib/types/thread-types.js`, `+roles: { [id: string]: RoleInfo }` should be `+roles: { +[id: string]: RoleInfo }`. That change appears to not introduce any type errors, so I'm going to put up a diff with it momentarily. Regarding this diff, I'd prefer if we could stick to treating our core data types as read-only.
57 ↗	(On Diff #25646)	Since we don't care about the result, we should use `forEach` instead of `map`