Page MenuHomePhabricator

[native][web] Add migration to unshim Blob-hosted multimedia
ClosedPublic

Authored by bartek on Thu, Nov 7, 3:37 AM.
Tags
None
Referenced Files
F3331130: D13889.id45728.diff
Wed, Nov 20, 8:03 PM
F3328210: D13889.id45728.diff
Wed, Nov 20, 1:27 PM
Unknown Object (File)
Sun, Nov 17, 5:59 AM
Unknown Object (File)
Sat, Nov 16, 3:14 PM
Unknown Object (File)
Sat, Nov 16, 1:32 AM
Unknown Object (File)
Fri, Nov 15, 10:27 AM
Unknown Object (File)
Thu, Nov 14, 8:38 PM
Unknown Object (File)
Thu, Nov 14, 5:08 PM
Subscribers

Details

Summary

Point 3c from ENG-6459.

Depends on D13888

Test Plan
  1. Before applying D13888 and this, I sent some encrypted multimedia messages. They appeared as robotext
  2. Applied both diffs and ran migration
  3. Multimedia messages appeared normally

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Thu, Nov 7, 4:50 AM
bartek added inline comments.
web/redux/persist.js
721–724 ↗(On Diff #45656)

As stated in https://phab.comm.dev/D13888#inline-78351, we weren't shimming this on web so I created a no-op migration to keep state version consistent across platforms

This revision is now accepted and ready to land.Thu, Nov 7, 5:31 AM
ashoat added inline comments.
native/redux/persist.js
1499

It's hard to believe this migration was tested, as it appears to me that it throws an exception on every single run

This revision is now accepted and ready to land.Thu, Nov 7, 7:20 PM
native/redux/persist.js
1499

There was an additional error here in not passing in handleReduxMigrationFailure. See D11928

  1. Split out legacyUnshimClientDB for existing usages
  2. Introduce new unshimClientDB
  3. Do the same for web

I did not do any testing. @bartek will need to test this before landing, thoroughly this time.

Setting @tomek as a blocking reviewer as I made significant changes here and he is most familiar with the migrations code.

native/redux/persist.js
1315 ↗(On Diff #45703)

Context in D11928 for why this is the only place on native where we pass handleReduxMigrationFailure to legacyUnshimClientDB

This revision now requires review to proceed.Thu, Nov 7, 8:33 PM

I talked with @tomek, and we concluded that I had previously done the test plan wrong, and the migration didn't actually run. In short, I was logging in with state version 85 from the beginning.

New test plan:

  1. Set stateVersion in redux/persist.js to 84
  2. Log out / log in again (clear app state) and log in with state 84.
  3. Close the app. Bump state version to 85
  4. Open the app. Now the migration runs

yarn dev logs:

LOG  redux-persist: migrationKeys [85]
LOG  redux-persist: running migration for versionKey 85

I was inspired by migration [35] which looked like this. It confused me because we also had DEPRECATED_unshimMessageStore(). I thought the latter was for "legacy" migrations, and the former is up to date.

This revision is now accepted and ready to land.Fri, Nov 8, 4:42 AM

Thanks for explaining and sharing the details & logs!