Storage Migration: Sync before starting storage migration #13356

david-allison · 2023-02-28T01:29:10Z

Purpose / Description

If a user is going to perform a migration, they should be fully synced to ensure no data loss. We can't easily know if they're fully synced on the media side of things, so perform a sync and see.

When the user has performed a media sync, migrate.

The sync may complete while the user has the app closed.

In this case, withProgress may not execute (no window), so we conservatively pause the start of the migration until the AnkiDroid window has called onResume.

Also handle the case where the media sync cannot occur (via AlertDialog + migrate anyway).

Fixes

Approach

Define an AsyncOperation: similar to an AsyncDialogFragment, but not necessarily with a dialog

Define a MigrateStorageAfterSync
It passes this into sync, which calls it at the end of the operation

Define a onSyncCompleted() listener, as an interface so the sync functionality can be decoupled from DeckPicker in future.

Handle the activity lifecycle changes in DeckPicker, which required a minor rework of DialogHandler + AnkiActivity

How Has This Been Tested?

Learning

renderPage() needs a lot of work for the new Backend

Checklist

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

github-actions · 2023-02-28T01:29:24Z

Message to maintainers, this PR contains strings changes.

Before merging this PR, it is best to run the "Sync Translations" GitHub action, then make and merge a PR from the i18n_sync branch to get translations cleaned out.
Then merge this PR, and immediately do another translation PR so the huge change made by this PR's key changes are all by themselves.

Read more about updating strings on the wiki,

david-allison · 2023-03-07T02:09:19Z

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

-        val message = """${getString(R.string.migration_update_request)}
-            <br>
-            <br>${getString(ifYouUninstallMessageId)}"""
+        val message = getString(R.string.migration_update_request_requires_media_sync)


A user will now always be logged in

Displays a notification if the app is paused otherwise performs the operation Similar to 'DialogHandler', but doesn't require a dialog

We need to move the user's data to /Android/ to handle scoped storage changes. Before we perform this migration, we should ensure that users data is safe. To do this, we enforce a sync. A user should have completed a full media sync before the migration occurs. This ensures that their data is up to date and can be restored if anything goes awry with the migration Dialog wording as discussed on the Figma, with the exception of changing 'sync' to 'media sync' Issue 5304

.idea/dictionaries/android.xml

We need a media sync before a storage migration. If this has been disabled, it is the user's responsibility to perform a sync before this migration occurs There are currently two reasons: Fetch media on Sync is set to: * 'Only if unmetered' & connection is metered * Never Issue 5304

Caused bugs when performing a storage migration after syncing

If a migration requires a sync, and a sync completes when the app is paused, a notification occurs prompting the user to migrate. This is handled by `DialogHandler`, which runs in AnkiActivity.onResume BEFORE `loadDeckCounts`. This races with closing the collection for a storage migration. Instead, start the task and allow it to be stopped. Issue 5304

mikehardy

Seems fine - strings are relatively clear (that is: merge-able / cleared out from real translation changes), needs a good second look but is de-conflicted and seems ready to go

lukstbit

Reviewed the changes and it seems fine to me, so LGTM
I'm going to wait until tomorrow, if Brayan wants to take a look otherwise I'm going to merge it myself.

mikehardy · 2023-03-24T16:12:31Z

Given two sets of eyes and the ability to do follow-ons plus the current strings situation (clear: ready to merge) I'm going to go ahead and merge this one and keep the scoped storage train moving down the tracks. Thank you @lukstbit !

david-allison marked this pull request as draft February 28, 2023 01:29

github-actions bot added the Strings label Feb 28, 2023

david-allison force-pushed the storage-migration-sync branch 3 times, most recently from a86f5cd to ab47dc5 Compare February 28, 2023 01:37

david-allison mentioned this pull request Feb 28, 2023

Refactor: Legacy syncing #13357

Merged

5 tasks

david-allison added Review High Priority Request for high priority review Scoped storage labels Feb 28, 2023

This was referenced Feb 28, 2023

remove unused SyncStatus: 'INCONCLUSIVE' #13358

Merged

refactor: DialogHandler #13359

Merged

david-allison force-pushed the storage-migration-sync branch from ab47dc5 to 5c9861c Compare March 1, 2023 18:44

pavreh mentioned this pull request Mar 2, 2023

Scoped storage - sync before migration #13382

Closed

4 tasks

david-allison added the Needs Author Reply Waiting for a reply from the original author label Mar 6, 2023

david-allison force-pushed the storage-migration-sync branch 2 times, most recently from 70ee216 to c68a48f Compare March 7, 2023 00:06

david-allison added Needs Author Reply Waiting for a reply from the original author and removed Needs Author Reply Waiting for a reply from the original author labels Mar 7, 2023

david-allison force-pushed the storage-migration-sync branch from c260657 to ab70bdd Compare March 7, 2023 02:14

david-allison changed the title ~~[Pre-Review] Storage Migration: Refactor & Sync before starting storage migration~~ Storage Migration: Refactor & Sync before starting storage migration Mar 7, 2023

david-allison removed the Needs Author Reply Waiting for a reply from the original author label Mar 7, 2023

david-allison marked this pull request as ready for review March 7, 2023 02:17

david-allison changed the title ~~Storage Migration: Refactor & Sync before starting storage migration~~ Storage Migration: Sync before starting storage migration Mar 7, 2023

david-allison force-pushed the storage-migration-sync branch from ab70bdd to 337e791 Compare March 7, 2023 12:10

david-allison commented Mar 7, 2023

View reviewed changes

david-allison added 4 commits March 23, 2023 21:01

DeckPicker: Add 'onMediaSyncCompleted'

f83cf00

DeckPicker: Add 'migrateStorageAfterMediaSyncCompleted'

e0bd45f

Add Async Operation for 'Migrate'

68ceee8

Displays a notification if the app is paused otherwise performs the operation Similar to 'DialogHandler', but doesn't require a dialog

mikehardy force-pushed the storage-migration-sync branch from 337e791 to 885bc11 Compare March 24, 2023 02:05

mikehardy reviewed Mar 24, 2023

View reviewed changes

.idea/dictionaries/android.xml Outdated Show resolved Hide resolved

david-allison added 3 commits March 23, 2023 21:19

fix: Remove sync col access from renderPage

7d7d7ca

Caused bugs when performing a storage migration after syncing

mikehardy force-pushed the storage-migration-sync branch from 885bc11 to fe5e211 Compare March 24, 2023 02:23

mikehardy approved these changes Mar 24, 2023

View reviewed changes

mikehardy added Needs Second Approval Has one approval, one more approval to merge and removed Review High Priority Request for high priority review labels Mar 24, 2023

mikehardy requested review from BrayanDSO, lukstbit and Arthur-Milchior March 24, 2023 02:24

lukstbit approved these changes Mar 24, 2023

View reviewed changes

lukstbit added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Mar 24, 2023

mikehardy merged commit be62538 into ankidroid:main Mar 24, 2023

github-actions bot added this to the 2.16 release milestone Mar 24, 2023

github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Mar 24, 2023

david-allison mentioned this pull request Apr 27, 2023

fix: automated sync leads to "collection not open" #13747

Merged

5 tasks

david-allison deleted the storage-migration-sync branch April 27, 2023 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage Migration: Sync before starting storage migration #13356

Storage Migration: Sync before starting storage migration #13356

david-allison commented Feb 28, 2023 •

edited

Loading

github-actions bot commented Feb 28, 2023

david-allison Mar 7, 2023

mikehardy left a comment •

edited

Loading

lukstbit left a comment

mikehardy commented Mar 24, 2023

Storage Migration: Sync before starting storage migration #13356

Storage Migration: Sync before starting storage migration #13356

Conversation

david-allison commented Feb 28, 2023 • edited Loading

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Learning

Checklist

github-actions bot commented Feb 28, 2023

david-allison Mar 7, 2023

Choose a reason for hiding this comment

mikehardy left a comment • edited Loading

Choose a reason for hiding this comment

lukstbit left a comment

Choose a reason for hiding this comment

mikehardy commented Mar 24, 2023

david-allison commented Feb 28, 2023 •

edited

Loading

mikehardy left a comment •

edited

Loading