AnkiHubSoftware / ankihub_addon

3 stars 0 forks source link

[BUILD-299] Skip AnkiWeb full sync dialog when it's triggered by AnkiHub sync #946

Closed abdnh closed 6 months ago

abdnh commented 7 months ago

Related issues

https://ankihub.atlassian.net/browse/BUILD-299

Proposed changes

This PR makes it so that the AnkiWeb full sync confirmation dialog is skipped (by doing a full upload) when it's triggered by an AnkiHub sync. This is done as follows:

  1. We do an AnkiWeb sync before the AnkiHub sync (in sync_with_ankihub()) if a full sync is already required. This is intended to clear a potential full sync triggered by user changes not related to AnkiHub.
  2. After the AnkiWeb sync, we make note of the current schema modification time.
  3. After the AnkiHub sync, we compare the current schema modification time with the one stored in the previous step. If they are not equal, we know that the AnkiHub sync triggered a schema change that will require a full AnkiWeb sync and we store the timestamp in the private config.
  4. We patch Anki's aqt.sync.full_sync() to skip the confirmation dialog and do a full upload if the timestamp stored in the config is equal to the current schema modification time.

How to reproduce

  1. Subscribe to an AnkiHub deck and sync it.
  2. Unsubscribe from the deck from the web app then sync from the add-on.
  3. Now resubscribe to the deck and sync from the add-on. You should notice the Sync button at the top of the main window is now red, indicating that a full sync is required. In this case, the full sync is triggered by note type changes done by the add-on.
  4. Sync with AnkiWeb. Normally, Anki will show that dialog asking you whether to download or upload, but it shouldn't appear this time.
codecov[bot] commented 7 months ago

The author of this PR, abdnh, is not an activated member of this organization on Codecov. Please activate this user on Codecov to display this PR comment. Coverage data is still being uploaded to Codecov.io for purposes of overall coverage calculations. Please don't hesitate to email us at support@codecov.io with any questions.

abdnh commented 6 months ago

But they won't be told to do that in the two other situations that cause a schema change:

I pushed some changes to handle this. What do you think?

RisingOrange commented 6 months ago

@abdnh The changes look good, but I think we still need some more changes to avoid data loss.

I don't think we should skip the full sync dialog in these situations:

Unless we do these two things before:

We are now doing both of these safety measures for deck installations, but not for uninstalling a deck or for adjusting note types of a deck.

I have a suggestion for this PR:

In other PRs, we could make these further changes:

What do you think?

abdnh commented 6 months ago

We are now doing both of these safety measures for deck installations, but not for uninstalling a deck or for adjusting note types of a deck.

Good point! I forgot about that.

sentry-io[bot] commented 6 months ago

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

Did you find this useful? React with a 👍 or 👎