Page MenuHomePhabricator

[native] Redux persist migration to remove incorrect permissions from threads
ClosedPublic

Authored by rohan on Oct 26 2023, 9:01 AM.
Tags
None
Referenced Files
F3514223: D9608.id32789.diff
Sun, Dec 22, 4:01 AM
F3513159: D9608.diff
Sat, Dec 21, 10:59 PM
Unknown Object (File)
Thu, Dec 19, 9:23 PM
Unknown Object (File)
Tue, Dec 17, 4:42 AM
Unknown Object (File)
Sun, Dec 15, 11:08 PM
Unknown Object (File)
Sun, Dec 15, 11:08 PM
Unknown Object (File)
Sun, Dec 15, 11:08 PM
Unknown Object (File)
Sun, Dec 15, 11:08 PM
Subscribers

Details

Summary

Although we handled the keyserver DB in D9599, it'll be good to perform a 'synced migration' where we also update the client's threads to prevent an inconsistency report. This diff runs a persist migration on native to handle this. The expected outcome is the client's threads no longer have the permissions in permissionsToRemoveInMigration

I followed migration 36 quite closely in terms of handling client DB threads

Depends on D9599

Resolves https://linear.app/comm/issue/ENG-5254/add-a-migration-to-fix-existing-communities-that-have-this-permission

Test Plan

Performed three different tests:

  1. Manually tested the code that updated threadInfos:
const threadStore = { ... } // threadStore contents here
const permissionsToRemoveInMigration = [ ... ] // permissionsToRemoveInMigration contents here

const updatedThreadInfos = {};
const threadInfos = threadStore.threadInfos;
for (const threadID in threadInfos) {
  const threadInfo = threadInfos[threadID];
  const { roles } = threadInfo;

  const updatedRoles = {};
  for (const roleID in roles) {
    const role = roles[roleID];
    const { permissions: rolePermissions } = role;
    const updatedPermissions = {};
    for (const permission in rolePermissions) {
        if (!permissionsToRemoveInMigration.includes(permission)) {
            updatedPermissions[permission] = rolePermissions[permission];
        }
    }
    updatedRoles[roleID] = { ...role, permissions: updatedPermissions };
  }
  

  const updatedThreadInfo = {
    ...threadInfo,
    roles: updatedRoles,
  };

  // Nothing should log here (testing: nothing logs)
  for (const roleID in updatedRoles) {
    const role = updatedRoles[roleID];
    const { permissions: rolePermissions } = role;
    for (const permission in rolePermissions) {
      if (permissionsToRemoveInMigration.includes(permission)) {
        console.log('found incorrect permission: ', roleID);
      }
    }
  }

  updatedThreadInfos[threadID] = updatedThreadInfo;
}
  1. Confirmed the results of the migration itself:
    • Open native and run yarn redux-devtools to inspect threadStore
    • Verified that some roles had some of the incorrect permissions
    • Upgraded the persist version to trigger the migration
    • Confirmed that the migration succeeded
    • Checked the redux devtools for the same roles and confirmed that there were some less permissions (introspecting into the permissions confirmed that the expected permissions were removed)
    • Waited a few minutes and confirmed that the client did not detect any inconsistencies
  1. Wrote unit tests and ran yarn workspace native test

Diff Detail

Repository
rCOMM Comm
Branch
ENG-5180
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan requested review of this revision.Oct 26 2023, 9:45 AM

Looks good, two points

  1. I'd consider moving this from lib/utils to native/redux since this is kind of a one-off function and IMO makes sense to keep "closer" to the code that runs migrations.
  2. persistMigrationToRemoveDescendantOpenVoiced looks correct based on my reading, but it'd also be pretty straightforward to write unit tests for if you cut/paste RawThreadInfos from your Redux store. Won't block this diff on that, but would be nice to have.
lib/utils/role-utils.js
125–127

Guessing this is to handle logged out case? Might be worth adding a comment here to clarify that?

This revision is now accepted and ready to land.Oct 26 2023, 10:15 AM
lib/utils/role-utils.js
138–143

You could do something like:

Object.fromEntries(
  Object.entries(rolePermissions || {})
    .filter(([permission]) => permission !== 'descendant_open_voiced')
);

but what you have might be more readable

Address feedback and add unit tests

native/redux/remove-select-role-permissions.js
1 ↗(On Diff #32438)

Nit: extra newline here

native/redux/remove-select-role-permissions.test.js
1 ↗(On Diff #32438)

Nit: extra newline here

rohan retitled this revision from [native] Redux persist migration to remove descendant_open_voiced from threads to [native] Redux persist migration to remove incorrect permissions from threads.Nov 6 2023, 7:59 AM
rohan edited the summary of this revision. (Show Details)
rohan edited the test plan for this revision. (Show Details)
rohan edited the test plan for this revision. (Show Details)

Use permissionsToRemoveInMigration instead of just removing descendant_open_voiced and update unit tests.

For reviewers, the only real change is instead of just removing descendant_open_voiced, we remove all permissions part of the permissionsToRemoveInMigration array (a list of permissions)

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