Address ENG-9618.
Code handling migrating user from old flows to signed device lists
Details
Details
Manual testing - using console logs I confirmed that:
- migration code is run only when latest backup info is absent, and latest device list was unsigned
- device list is being reordered and signed device list update is sent during migration
- userkeys backup is created
- normal backup creation runs otherwise
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Keeping it as a draft becasue we discussed with @kamil that it's best to merge it with BackupHandler - this will solve issues with synchronizing the two handlers
| native/backup/backup-handler.js | ||
|---|---|---|
| 82 ↗ | (On Diff #46460) | What's the point of this ref? |
| native/backup/backup-handler.js | ||
|---|---|---|
| 182–185 ↗ | (On Diff #46460) | You should add migrationRunning.current here or you can rely on backupUploadInProgress, but make sure to set it to false when early returning or throwing error |
| 195–200 ↗ | (On Diff #46460) | You should avoid throwing here, backupUploadInProgress will be set to true forever now. (this applies to error below) In testUserKeysRestore we throw an error but later we show an alert with that message, maybe we could do the same here to show the message to staff? |
| 244–288 ↗ | (On Diff #46460) | is there a chance to make this and reorderAndSignDeviceList to be part of useDeviceListUpdate to have code for updating the device list encapsulated in one place? |
| 267 ↗ | (On Diff #46460) | is there a task tracking implementing this? |
| 299–305 ↗ | (On Diff #46460) | this code can now be executed on a non-primary device I think. Can this be put at the top and the early-exit to make this code cleaner? |
| native/backup/backup-handler.js | ||
|---|---|---|
| 82 ↗ | (On Diff #46460) | Hmm, it was needed when the handlers were separate, but now when they are merged, the migrationRunning ref is actually not needed, its purpose is already fully covered by the backupUploadInProgress. I'll remove it |
| 182–185 ↗ | (On Diff #46460) | After analyzing it, the migrationRunning ref is actually not needed, its purpose is already fully covered by the backupUploadInProgress |
| 195–200 ↗ | (On Diff #46460) | Good call |
| 267 ↗ | (On Diff #46460) | I was about to make a separate diff for it, but it became short and simple so I'd rather fixup it into this one |
| 299–305 ↗ | (On Diff #46460) | It's not that simple:
The solution is to duplicate conditions for the above if plus the primary device condition, and early exit before creating this promise |
| native/backup/backup-handler.js | ||
|---|---|---|
| 202 ↗ | (On Diff #46530) | Maybe or merge with condition below? |
| 291–293 ↗ | (On Diff #46530) | shouldn't we call this in a loop, and also check if we're still primary before uploading backup keys? |
| 316–320 ↗ | (On Diff #46530) | Now, this covers both regular backup upload and migration, I would prefer to somehow differentiate (use different try-catch) so this could be easier to debug if needed |
| 244–288 ↗ | (On Diff #46460) | this comment was skipped - but perhaps we can make it in a separate diff |
| native/backup/backup-handler.js | ||
|---|---|---|
| 202 ↗ | (On Diff #46530) | Good idea |
| 291–293 ↗ | (On Diff #46530) | I thought about this a bit and considered a few scenarios. After doing some testing, I think this works the best:
|
| 316–320 ↗ | (On Diff #46530) | Good idea, but two try-catch blocks introduced too much nesting here, so I'll make error message conditional basing on shouldDoMigration |
| native/backup/backup-handler.js | ||
|---|---|---|
| 244–288 ↗ | (On Diff #46460) | Not sure if this is a good idea. The reorderAndSignDeviceList is exceptional and should be used only in one place - this migration code. |