It would be good to amend the Test Plan to include checking the state check mechanism. Eg. compile prod build that doesn't support avatar permission with old codeVersion, point it at local keyserver, run keyserver that optionally omits avatar permission – make sure state check mechanism is being called via redux-devtools (hopefully it works better for you than me), and that it isn't detecting any changes

Eg. compile prod build that doesn't support avatar permission with old codeVersion, point it at local keyserver, run keyserver that optionally omits avatar permission – make sure state check mechanism is being called via redux-devtools (hopefully it works better for you than me), and that it isn't detecting any changes

Not sure I completely follow?

Are you suggesting that I:

1. Locally update role-creator.js to exclude avatars
2. Run updateRolesAndPermissionsForAllThreads migration
3. Compile native build and connect to keyserver so I'm missing all thread_edit_avatar permissions
4. Add avatar permissions back to role-creator.js
5. Run updateRolesAndPermissionsForAllThreads migration
6. Observe whether there are any state check discrepancies when native re-connects to keyserver?
7. Manually set db_version of metadata table back to whatever it is on master so I'm "back in sync" with migrations

In D7364#219120, @atul wrote:

Eg. compile prod build that doesn't support avatar permission with old codeVersion, point it at local keyserver, run keyserver that optionally omits avatar permission – make sure state check mechanism is being called via redux-devtools (hopefully it works better for you than me), and that it isn't detecting any changes

Not sure I completely follow?

Are you suggesting that I:

1. Locally update role-creator.js to exclude avatars
2. Run updateRolesAndPermissionsForAllThreads migration
3. Compile native build and connect to keyserver so I'm missing all thread_edit_avatar permissions
4. Add avatar permissions back to role-creator.js
5. Run updateRolesAndPermissionsForAllThreads migration
6. Observe whether there are any state check discrepancies when native re-connects to keyserver?
7. Manually set db_version of metadata table back to whatever it is on master so I'm "back in sync" with migrations

Not quite... that looks like a good way to test an old keyserver with a new native build, but that sort of scenario won't happen in the wild since we'll likely update the keyserver before shipping a new build out.

Instead, I'm suggesting testing a new keyserver with an old native build. Breaking down my last comment:

1. compile prod build that doesn't support avatar permission with old codeVersion – here I suggest checking out an old version of the native codebase BEFORE your new permission and corresponding migration, and compiling a prod build (prod instead of dev so that it doesn't auto-refresh after you go back to master)
2. point it at local keyserver – here I note that a prod build will generally point to the production server, so you need to make sure your build points to your local keyserver. You can do that either by compiling a dev build first (so the urlPrefix config gets passed to the prod build from the dev build via redux-persist), or by manually configuring the prod build after logging in via the "Developer Settings" UI. Note that if you previously had a dev build of master, it probably won't work to deploy this "old" prod build, since the redux-persist migration version will be downgraded (you should have a redux-persist migration to add this new permission)
3. run keyserver that optionally omits avatar permission – here I suggest running the keyserver off of master + this diff, so that you're able to check how it behaves with older clients. The keyserver should be post-migration
4. make sure state check mechanism is being called via redux-devtools – here I suggest monitor Redux actions to see when the state check mechanism is dispatched, and to confirm it doesn't detect any changes

As a secondary test, you could then "upgrade" the client by compiling a new prod build off of master + this diff, and again check if the state check mechanism detects any changes. It should not detect any changes if you have implemented a redux-persist migration that will add your new permission on the client side.

The goal of all of this work is to make sure the state check mechanism doesn't have to do any "fixing". When we rely on the state check mechanism, it leads to performance issues as somebody's entire ThreadStore has to be replaced.

The goal of all of this work is to make sure the state check mechanism doesn't have to do any "fixing". When we rely on the state check mechanism, it leads to performance issues as somebody's entire ThreadStore has to be replaced.

I'm on the same page here.

Not quite... that looks like a good way to test an old keyserver with a new native build, but that sort of scenario won't happen in the wild since we'll likely update the keyserver before shipping a new build out.

Hm, I'm a bit confused here. The purpose of this diff is to make sure we don't send any avatar-related permissions to native clients until we've shipped a release with them fully enabled.

In order to test that I'm

1. Reverting role-creator to how it was before we introduced any avatar permissions.
2. Running updateRolesAndPermissionsForAllThreads migration to "go back" to how things were before avatar permissions and to reflect changes made to role-creator in step 1.
3. Logging in to native client and "pulling in" ThreadInfos from the keyserver without any avatar permissions.

This gets us to a "before" state where the native client doesn't have any avatar-related permissions.

4. We want to re-add the avatar permissions to role-creator and re-run the updateRolesAndPermissionsForAllThreads migration so that avatar permissions are in the database but we want them filtered out by rawThreadInfoFromServerThreadInfo.
5. Now we want to connect native to the keyserver

Our intention is that filterThreadPermissions will filter out avatar permissions and compute ThreadInfos just as before the migration. We want the permissions blobs on the native client and those computed by rawThreadInfoFromServerThreadInfo to be at parity to avoid pulling in entire ThreadStore as part of INCREMENTAL_STATE_SYNC.

To verify that the ThreadInfo.permissions remain the same, we check the INCREMENTAL_STATE_SYNC action to make sure we don't see any updates:

I'm suggesting testing a new keyserver with an old native build.

Hm, we haven't put in a hasMinCodeVersion check yet.. so all native clients are effectively "old builds" if I'm understanding correctly. We're hardcoding filterThreadEditAvatarPermission = true for the time being so I don't think checking out an old native build is necessary.

I suggest checking out an old version of the native codebase BEFORE your new permission and corresponding migration

Are you referring to keyserver migration to introduce avatar permissions, or redux-persist migration? If you're referring to redux-persist permission, that's still in progress... but testing old/new native clients there will definitely be a part of the Test Plan, I just don't think it's relevant in this diff yet.

As a secondary test, you could then "upgrade" the client by compiling a new prod build off of master + this diff, and again check if the state check mechanism detects any changes. It should not detect any changes if you have implemented a redux-persist migration that will add your new permission on the client side.

This will definitely be part of Test Plan for upcoming diff that handles the redux-persist migration

Ok I'm confused, here's the diff from before and after: https://www.diffchecker.com/P9mWVrym/

It looks like there is some inconsistency, but it doesn't seem related to my changes:

Okay so I was having trouble with Redux DevTools, so I tested by

1. Remove all avatar permissions from role-creator
2. Add updateRolesAndPermissionsForAllThreads migration to migration-config
3. Log into native app.
4. Set breakpoint in src/socket/session-utils:checkState(...)
5. Add avatar permissions back to role-creator
6. Add updateRolesAndPermissionsForAllThreads migration to migration-config
7. Observe that checkState(...) hits with state_check followed by state_validated

I think this is now good to land

Nice, let's land it!! Thanks for doing that