Page MenuHomePhabricator

[native] Introduce `updateClientDBThreadStoreThreadInfos` utility
ClosedPublic

Authored by atul on Apr 24 2023, 12:08 PM.
Tags
None
Referenced Files
F3186003: D7590.id25688.diff
Fri, Nov 8, 2:00 PM
F3185333: D7590.id25618.diff
Fri, Nov 8, 12:48 PM
Unknown Object (File)
Sun, Nov 3, 2:37 AM
Unknown Object (File)
Thu, Oct 31, 4:32 PM
Unknown Object (File)
Thu, Oct 31, 4:32 PM
Unknown Object (File)
Thu, Oct 31, 4:32 PM
Unknown Object (File)
Thu, Oct 31, 4:32 PM
Unknown Object (File)
Thu, Oct 31, 4:32 PM
Subscribers

Details

Summary

updateClientDBThreadStoreThreadInfos handles converting CommCoreModule representation of ClientDBThreadInfos to "JS side" representation of RawThreadInfos, applying some sort of migrationFunc, and then converting RawThreadInfos back to ClientDBThreadInfos and updating the SQLite DB. More detailed description of each step can be seen as code comments in the diff.

It's a common pattern (at this point) to make updates to the SQLiteDB BEFORE we REHYDRATE Redux. To avoid duplicating business logic and to reuse all the rich functionality we already have on the JS side we convert from CommCoreModule representation to JS representation, make the changes, then convert back. This utility function handles all of that and accepts a single migrationFunc to update the contents of each ThreadInfo.

The functionality in this diff is mostly from migration 36 where it was shown to be correct. In addition it was used in D7551 where it also was shown to be correct (the issues there were related to the permissions business logic, not the behavior of the code in this diff... so although this diff is "dangling," its functionality has been used + tested.

This functionality will shortly be consumed by the client version of updateRolesAndPermissionsForAllThreads. Separating this from that because they're both complex "chunks" of code and IMO should be reviewed separately.

Test Plan

This logic was lifted from migration 36 where it was shown to be correct.

I also tested this logic thoroughly as part of D7551. While there were issues with my persistMigrationForThreadAvatarPermission logic, the code within this diff worked perfectly as expected.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.Apr 24 2023, 12:09 PM
ashoat added inline comments.
native/redux/client-db-utils.js
41–43

Why not use Object.values here?

This revision is now accepted and ready to land.Apr 24 2023, 12:32 PM

PS – a good way to introduce a use case for this "dangling function" would have been to refactor existing migrations to use it. In this case I'm aware of how it'll be used, but I still feel strongly that it's easier to review diffs that avoid this "dangling function" pattern

PS – a good way to introduce a use case for this "dangling function" would have been to refactor existing migrations to use it.

Yeah agree it was difficult because refactoring @rohan's migration to use this would be involved because it interfaces with both threadStore and messageStore and it'd be hard to pull out just the threads stuff.

native/redux/client-db-utils.js
41–43

Why not use Object.values here?

I followed what @rohan did since I know it works.

My guess is that he ran into flow issues with Object.values? (cc @rohan)

Could probably use the values(...) function in lib/utils/objects/values(...).

native/redux/client-db-utils.js
41–43

Ah yeah, that's probably the best option

native/redux/client-db-utils.js
41–43

Yeah if I remember right I did run into a Flow issue if I used Object.values when followed by the call to convertRawThreadInfoToClientDBThreadInfo

This revision was landed with ongoing or failed builds.Apr 25 2023, 10:46 AM
This revision was automatically updated to reflect the committed changes.
native/redux/client-db-utils.js
41–43 ↗(On Diff #25689)

I was hoping we would update to using values here, I think it's more readable

native/redux/client-db-utils.js
41–43 ↗(On Diff #25689)

Ah dang I made the changes but when I went back to computer to land I stashed them instead of amending them...will update