Page MenuHomePhabricator

[keyserver/lib/native] Store the number of pinned messages in ThreadInfo
ClosedPublic

Authored by rohan on Mar 27 2023, 4:59 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 1:56 AM
Unknown Object (File)
Sat, Dec 7, 10:17 AM
Unknown Object (File)
Fri, Dec 6, 9:53 PM
Unknown Object (File)
Fri, Dec 6, 9:53 PM
Unknown Object (File)
Fri, Dec 6, 9:52 PM
Unknown Object (File)
Fri, Dec 6, 9:14 PM
Unknown Object (File)
Thu, Dec 5, 8:34 PM
Unknown Object (File)
Thu, Dec 5, 8:34 PM
Subscribers

Details

Summary

We want to include pinnedCount inside ThreadInfos in order to avoid having to dynamically fetch this number each time a thread is opened. This diff handles this in two ways:

  1. We always want to include pinnedCount in ServerThreadInfo - then, based on the code version on native, we'd want to conditionally include it into the downstream ThreadInfo types (where we construct, for example, RawThreadInfo from ServerThreadInfo).
  1. Introduce a redux persist migration to target newly updated clients. Each step in the migration is commented to indicate its purpose. One case that is already handled but important to note: if an old client and a new client are in a thread together (pre and post message pinning versions), when we update the old client, we need to make sure we don't initialize pinnedCount to 0, but instead derive the pinnedCount based on the thread and existence of TOGGLE_PIN messages because there may be message pins in the thread.

Linear: https://linear.app/comm/issue/ENG-3396/store-the-number-of-pinned-messages-in-threadinfo

Depends on D7202

Test Plan
  1. Install native off master
  2. git checkout pinned_messages
  3. Go on the web client and pin several messages
  4. Add the redux-persist migration
  5. Open the native app
  6. Run yarn redux-devtools and confirm that threadStore has updated pinnedCount values.

Screenshot 2023-03-27 at 7.43.22 PM.png (806×1 px, 136 KB)

  1. Check the redux devtools on the web client and confirm that threadStore has the updated pinnedCount values.

Screenshot 2023-03-27 at 8.01.07 PM.png (798×1 px, 139 KB)

  1. Check that ThreadInfo contains pinnedCount:

Screenshot 2023-03-27 at 8.01.43 PM.png (600×1 px, 135 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Please address @ashoat's feedback before landing.

Read through this and it looks good. Please make sure you've thought through this diff very carefully and have tested it thoroughly. It's super critical to get these migrations right. Feel free to schedule something tomorrow if you want to go through it one more time synchronously on a Zoom call or anything.

This revision is now accepted and ready to land.Mar 28 2023, 4:47 PM

Planning changes to spend some more time testing before I land the stack + to address feedback

keyserver/src/fetchers/thread-fetchers.js
149 ↗(On Diff #24253)

I just want to confirm to prevent another review cycle, if we flip the condition and leave it as is:

hasCodeVersionBelow202 = !hasMinCodeVersion(viewer.platformDetails, 202) and includePinnedCount: hasCodeVersionBelow 202 would mean we don't include the pinnedCount for viewers that have a higher version than 202.

Does that mean we should make it something like:

excludePinnedCount: hasCodeVersionBelow202 and use that instead? That way if excludePinnedCount is not defined, sendPushNotifs can access the updated RawThreadInfo, and code versions above 202 should also be able to access pinnedCount.

Feel free to let me know if I'm missing something really obvious or if I've understood you correctly

Flip the condition to check for if we should include pinnedCount in ThreadInfo, to account for calls to the method that do not provide a third parameter

This revision is now accepted and ready to land.Apr 3 2023, 9:55 AM
rohan planned changes to this revision.Apr 3 2023, 9:56 AM

Addressed the feedback, but still planning changes for the persist migration.

rohan requested review of this revision.Apr 5 2023, 6:46 AM

Looks like it was https://phab.comm.dev/D7202?id=24499#inline-48113 all along, requesting review for the changes made to address feedback

This revision is now accepted and ready to land.Apr 5 2023, 6:46 AM
rohan requested review of this revision.Apr 5 2023, 6:46 AM

My feedback appears to have been addressed. Defer to @atul on the redux-persist migration

This revision is now accepted and ready to land.Apr 5 2023, 6:55 AM

Change code version to 205 and set avatar to NULL in Thread constructor
(spoke with @atul about this)

native/redux/persist.js
567 ↗(On Diff #24812)

As discussed in our 1:1: along with this, we need to also add messageTypes.TOGGLE_PIN to localUnshimTypes in lib/shared/unshim-utils.js – please do this before landing!

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
641–642 ↗(On Diff #24812)

This line is causing a crash because it's possible for pinnedCount to be missing in the RawThreadInfo. Let's wrap it in a conditional, and default to 0

Code version 205->207, add conditional for pinnedCount, and add TOGGLE_PIN to localUnshimTypes

Sorry... going to put 207 and 208 out today. There was an issue with the Android release so I need to kick it off again.

Code version 207 --> 209 (not landing yet)

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
642–644 ↗(On Diff #24872)

Note that @atul took a slightly different approach to this conditional in D7352. Might make sense to align approaches, but I don't feel too strongly

ashoat requested changes to this revision.Apr 9 2023, 5:14 AM

Last round of changes!

keyserver/src/fetchers/thread-fetchers.js
181 ↗(On Diff #24872)

Let's rename this to excludePinInfo or something so that it's clear that it's not just about the pinned count (it's going to need to cover the pin permission as well)

lib/shared/thread-utils.js
714 ↗(On Diff #24872)

Now that D7363 has landed, we can make sure we exclude the pin permission here. You can check D7364 for an example on how to do that

This revision now requires changes to proceed.Apr 9 2023, 5:14 AM
  1. Match C++ convention
  2. Rename excludePinnedCount to excludePinInfo
  3. Gate the threadPermissions.MANAGE_PINS based on the code version - test plan was largely following D7364

Before (native client has a code version BELOW the minimum code version for pinned messages but the client has the permission to manage pins)

Before (Low Code Version) (Permission Exists).png (1×3 px, 432 KB)

After (native client has a code version BELOW the minimum code version for pinned messages, and now the client does not have permission to manage pins)

After (Low Code Version) Permission Doesn't Exist.png (1×3 px, 458 KB)

Before (native client has a code version ABOVE the minimum code version for pinned messages so the client has the permission to manage pins)

Before (Above Code Version) Permission Exists.png (1×3 px, 426 KB)

After (native client has a code version ABOVE the minimum code version for pinned messages and the client still has permission to manage pins)

After (Above Code Version) Permission Exists.png (1×3 px, 432 KB)

Perfect!! As a last step before landing, can you try the test I described to @atul here? To run the Redux dev tools that I reference there, check out this doc. And the way you can see the state check mechanism being run is you should see a PROCESS_SERVER_REQUESTS action. If there's just one, that means the state check mechanism ran and saw no changes. If there are multiple such actions in a row, that means the state check mechanism detected changes. You should wait around 3-5ms after the socket connects to see the state check mechanism fire, but you might need to manipulate sessionCheckFrequency to get it to fire more frequently than once every 3 min (if you need to run it again soon after running it once).

This revision is now accepted and ready to land.Apr 9 2023, 6:36 AM

@rohan, @atul just finished testing D7364 and might have some pointers... it looks like Redux dev tools don't work particularly well

@rohan, @atul just finished testing D7364 and might have some pointers... it looks like Redux dev tools don't work particularly well

After reading through the test plans, these are the tests I'm running:

  1. git checkout master
  2. Run updateRolesAndPermissionsForAllThreads migration (to instantiate a keyserver without the new manage_pins permission)
  3. Build and log into native
  4. Confirm that the state of the app is as expected (i.e. no thread permission and no pinnedCount)
  5. git checkout pinned_messages
  6. Run the migrations for this stack (one includes a new thread permission)
  7. Update the persist migration for updating threadStore with the new permission
  8. Confirm that no inconsistencies are detected (only one PROCESS_SERVER_REQUESTS action made and no message indicating the system detected inconsistencies)

I'm running this for an instance where the native client's codeVersion is less than the minimum amount (209), and then one where the native's client codeVersion will be above the minimum (lowering the minimum down to 201 just to test this case).

(Once you're done with your test plan make sure to update the db_version in metadata table back to what's in migration-config)

Modify redux-persist migration to migrate threadStore to include the new manage_pins permission. Used https://github.com/CommE2E/comm/blob/master/native/redux/edit-thread-permission-migration.js as a reference

In D7211#219210, @atul wrote:

(Once you're done with your test plan make sure to update the db_version in metadata table back to what's in migration-config)

Made sure to do this, thanks for the heads up!

lib/shared/thread-utils.js
725 ↗(On Diff #24894)

Need to make this include descendants

Realized that for Admins, threadPermissions.MANAGE_PINS is true, but role-creator.js also includes the permission for descendants. Checked with D7364 to confirm that I definitely should be including
descendant_manage_pins in filterThreadPermissions as well

Great catch on the descendant permissions! Glad we did this testing

native/redux/manage-pins-permission-migration.js
53 ↗(On Diff #24896)

Does this need to be updated to handle the DESCENDANT permissions as well?

native/redux/manage-pins-permission-migration.js
53 ↗(On Diff #24896)

Yes

Include descendents_manage_pins in updated roles

rohan requested review of this revision.Apr 10 2023, 1:20 PM

Tested again after the changes

@atul has recently worked on the permission migration stuff and is more familiar with the SQLite redux-persist migration stuff, so will defer to him

This revision is now accepted and ready to land.Apr 11 2023, 8:30 AM