Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid duplicating crowdanki_uuid when cloning note model #136

Merged
merged 1 commit into from
Aug 22, 2021

Conversation

aplaice
Copy link
Collaborator

@aplaice aplaice commented Jul 3, 2021

Fix #121, fix #123.

Testing

  1. Ensure that you have a deck (say test) with at least one note of note type (say) Basic.
  2. Export test with CrowdAnki.
  3. (Optionally check that the Basic note type has been correctly exported, and that in total, one note type has been exported (e.g. use jq < deck.json '.note_models | length').)
  4. Create a new note type (Tools > Manage note types > Add > Clone: Basic > OK), say called Basic_test.
  5. In the test deck create a note of type Basic_test.
  6. Inspect the exported deck.json. (e.g. with jq < deck.json '.note_models | length')

Expected result (and the result with this PR)

  1. There are two exported note models.

Actual result (before this PR)

  1. There's only one exported note model.

Alternatively, inspect the note models with a debug console script:

repeated = {}
for model in filter(lambda model: 'crowdanki_uuid' in model,
                    self.col.models.all()):
    cu = model["crowdanki_uuid"]
    if cu in repeated:
        repeated[cu] = True
    else:
        repeated[cu] = False

if any(repeated.values()):
    print("WARNING! There are repeated Note Model uuids!")
else:
    print("There are no repeated Note Model uuids! Everything is OK!")

Limitations

AnkiDroid and AnkiMobile

Unfortunately, this PR does not prevent a user from duplicating a note model's crowdanki_uuid when cloning a note model on AnkiDroid or AnkiMobile. Hopefully most users create new note models on desktop!

Add-ons that clone note models

As described in the relevant docstring, if an add-on clones a note-model with add=True, then (on Anki ≥ 2.1.45) we won't catch the duplicated UUID. (This could be dealt with relatively easily.)

(Looking quickly, it seems that there are cases of add-ons cloning note models:

https://github.com/search?q=models.copy+anki&type=Code )

Future work

(This is for my own reference, and for informational purposes.)

Checking for duplicate crowdanki_uuids during export

It might be worth adding code that checks for duplicate crowdanki_uuids when exporting, in order to deal with the already duplicated crowdanki_uuids in users' databases and those that will be duplicated in the future when duplicating note models on AnkiDroid and AnkiMobile (as described above). However, that is likely far more complicated to do correctly (how do we determine which is the original note model and which the new one?), so I'll leave it for a future refinement.

Debug console script

Also, like mentioned in #135, I'll try to write (ASAP) a debug console script for diagnosing and fixing all the recent crowdanki_uuid issues.

(Alternatively, should we have a menu-item, similar to the built-in Tools > Check database for checking the UUID issues?)

Checking for issues during import

As discussed in #121 and #123, the duplicate crowdanki_uuids issue leads to incomplete exports of note models. (CrowdAnki thinks that several different note models are the same, so exports only one of them.) This can be particularly problematic during import if the number of fields of a note doesn't match the number of fields in the (incorrect) note type.

It might be nice to check, during import, that the number of fields in a note matches the number of fields in the note type.

Creating a hook

I'll ask upstream (in Anki) whether we could have a hook that runs just before or just after a note model was duplicated (similar to deck_conf_did_add_config), in order that we can remove the monkey patching here!

@Stvad
Copy link
Owner

Stvad commented Jul 15, 2021

thanks! I should be able to take a look at this in a ~week. btw, can you please reach ot to me on email/twitter dm liked from my profile?

@AlexFicachi
Copy link

Looking forward to having this PR merged :)

@aplaice
Copy link
Collaborator Author

aplaice commented Aug 20, 2021

Looking forward to having this PR merged :)

It not being merged is now my fault. :) (I want to triple-check that everything works as expected, especially with the latest version of Anki, and in case there are adverse reactions with the other PRs — there shouldn't be, as they touch different parts of the code, but I'd prefer not to replace one set of subtle bugs for another.)

I'll hopefully get it done soon (I haven't had time recently)!

Fix Stvad#121, Stvad#123.

Unfortunately, this does not prevent a user from duplicating a note
model's crowdanki_uuid when cloning a note model on AnkiDroid or
AnkiMobile.  Hopefully most users create new note models on desktop!
@aplaice aplaice force-pushed the disambiguate_cloned_notemodel branch from 73ba475 to 977eb5a Compare August 22, 2021 21:21
@aplaice
Copy link
Collaborator Author

aplaice commented Aug 22, 2021

With Anki 2.1.46 (the latest), the "manual" tests for #127 and for #136 both pass (I'd like to convert them into automated tests, but that can wait). (The existing "mamba" tests also all pass.)

@aplaice aplaice merged commit 72b6938 into Stvad:master Aug 22, 2021
@aplaice aplaice deleted the disambiguate_cloned_notemodel branch August 22, 2021 21:26
aplaice added a commit to aplaice/CrowdAnki that referenced this pull request Dec 5, 2021
The issue of note model UUIDs being cloned along with the note models
themselves has been fixed (Stvad#136).  However, many users' collections
will already contain such duplicated note model UUIDs.  Also, if a
user clones a note type on another platform, then the UUID will still
be duplicated.

The disambiguation is run before export and snapshot, since that's
when it's most needed to avoid broken `deck.json`s.

In the long, run we could perhaps switch to running the code only
after syncing (since our "attack surface" would be cloning of note
models on other platforms).

Running `disambiguate_note_model_uuids` takes 10 ms on a collection
with 20 note types (without duplicates), which is IMO an acceptable
overhead.

The file `disambiguate_uuids.py` could also contain the (far lower
priority) disambiguation of deck config UUIDs (see Stvad#135).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong Note Type is Exported Export incomplete and import throws error
3 participants