feat: support editing an existing tag's external_id on taxonomy re-im… - #804
Conversation
|
Thanks for the pull request, @ufedaseyeuconsultant! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Claude and I worked through this together and are requesting the changes below before merge. The scope is right: everything stays inside src/openedx_tagging/import_export/, no model field or migration is added, previous_id is never persisted or exported, and the two validation fixes called out in the PR description (_validate_parent and _validate_value learning about RenameTagExternalId) are correct and covered by tests. The gaps below are all about same-import row interactions: several of them come down to validation checking only the current database state and rows processed earlier in the file, never rows later in the file or the DB state other same-import actions will produce once they run.
1. Two rows with the same previous_id pass validation, then crash at execute time instead of failing cleanly.
In src/openedx_tagging/import_export/actions.py, RenameTagExternalId._validate_new_id checks the new id against the database and against other queued actions' id, but never checks whether another row in the same import already claims this row's previous_id. Given a file with row 1 {previous_id: "A", id: "B"} and row 2 {previous_id: "A", id: "C"}: at validate time, both rows call taxonomy.tag_set.get(external_id="A") against the unmodified database and both find the same tag, since nothing has executed yet, so both rows validate cleanly (assuming B and C don't otherwise collide). At execute time, row 1's execute() renames tag A's external_id to B. Row 2's execute() then calls self.taxonomy.tag_set.get(external_id=self.tag.previous_id), i.e. looks up external_id="A" again, which no longer exists, and raises Tag.DoesNotExist. That's unhandled inside the @transaction.atomic() block in TagImportPlan.execute(), so it propagates to the broad except Exception in api.py's import_tags(), which logs the raw exception to the task log and reports the import as failed, instead of surfacing a clean "duplicate previous_id" validation error at the plan step the way the wizard's other rejections work. A duplicated previous_id value from a copy-paste mistake in the source spreadsheet hits this today. Can you add a same-import previous_id collision check to _validate_new_id, the same way it already checks for id collisions against prior RenameTagExternalId actions, and add a test with two rows sharing one previous_id to lock the fix in?
2. Reusing an external_id that a replace-mode delete is freeing up in the same import is rejected, and needs to be allowed.
RenameTagExternalId._validate_new_id's existence check (self.taxonomy.tag_set.filter(external_id=self.tag.id).exists()) in src/openedx_tagging/import_export/actions.py runs against the database as it is right now, not as it will be once other actions in the same import have run. If a file both omits tag Z (external_id="Z", which a replace-mode import will therefore delete) and renames some other tag onto id="Z", the rename row is rejected with "A tag with external_id (Z) already exists," because Z still exists in the database at validate time.
The fix looks like widening _validate_new_id: don't treat a collision as real if the colliding tag is itself in the tags_for_delete set that import_plan.py's generate_actions builds for the replace-mode delete sweep. Execution order should already be safe once that validation is relaxed: _build_delete_actions runs before the per-row action loop in generate_actions, so delete actions get lower indices and TagImportPlan.execute() runs them before the row-based rename that reuses the freed id. That said, this is inference from reading the ordering, not something we've run, so it needs a test that actually executes a replace-mode import doing this, not just a validation-level check.
3. Renaming two tags to each other's prior ids (a swap) needs to work, and it needs more than a validation fix.
Given a taxonomy with tag X (external_id="A") and tag Y (external_id="B"), a file with row 1 {previous_id: "A", id: "B"} and row 2 {previous_id: "B", id: "A"} fails at row 1 for the same reason as item 2: tag_set.filter(external_id="B").exists() is True because Y still has external_id="B" at validate time, so the row is rejected with "A tag with external_id (B) already exists." The same rejection happens in the opposite row order.
Here the underlying (taxonomy, external_id) unique_together constraint is enforced immediately on save(), not deferred, and that's true for every backend this project runs on (SQLite and MySQL don't support deferrable unique constraints the way Postgres does). So even if validation is relaxed the same way as item 2, executing the two renames in either order still hits that constraint: renaming X to B first collides with Y, which still holds B at that point, and renaming Y to A first collides with X, which still holds A. Whichever order execute() uses, the second save() raises IntegrityError, and since that's worse than today's behavior (a validation-time rejection would become a raw execute-time database error), this can't be closed by only touching _validate_new_id. Supporting the swap needs execute() to stage the affected tags through an intermediate external_id, or the action list to detect the cycle and reorder around it, and the plan should say which approach it's taking rather than leaving it to fall out of whatever execute() happens to do today. Whichever approach it takes, please add a test that runs an actual two-tag swap through import_tags() end to end, not just a generate_actions()-level check.
4. parent_id must reference a tag's desired end-state external_id, and _validate_parent needs to reject a stale one instead of accepting it.
The import already supports moving a tag to a different parent via UpdateParentTag, so parent_id already means "this tag's desired parent," not "the parent it had before this import." _validate_parent in actions.py doesn't enforce that consistently: a child row referencing the parent's new id only validates when the rename row comes first in the file, via the existing _search_action check against RenameTagExternalId, the same rule CreateTag already follows, but a child row referencing the parent's old id validates unconditionally, because taxonomy.tag_set.get(external_id=self.tag.parent_id) still finds the not-yet-renamed tag in the database. That's the same execute-time crash as item 1 whenever the rename runs first, and it gets worse once item 3's swap support lands, since a stale old id could then resolve to a different tag entirely instead of just failing. Can you tighten _validate_parent to reject a parent_id that matches a tag whose external_id is being vacated by another row's previous_id in this same import, instead of resolving it against live database state, and add tests for both the accepted new-id reference and the rejected old-id one?
5. The AC in #673 names a CSV-specific verification scenario, but the round-trip rename tests only run through JSON.
test_parsers.py has parallel CSV and JSON tests for parsing previous_id and for confirming it's excluded from export, so the parser layer is genuinely format-agnostic, _parse_tags in parsers.py handles import_only_fields the same way for both. But every test that runs a rename through the full import_tags() / export_tags() pipeline in test_api.py (test_import_rename_external_id_preserves_pk, test_import_rename_external_id_then_export, the two rejection tests) builds its import file with json.dumps(...). Issue #673 lists "rename an external_id, verified via CSV export" as its own acceptance scenario, separate from the JSON one. Can you add a CSV version of the preserves-pk/then-export pair, so the CSV path gets the same end-to-end coverage as JSON, not just parser-level coverage?
6. The replace-mode interaction, the primary path per the ticket, is only tested at plan-generation level, not through a real execute.
test_import_plan.py's test_generate_actions_rename_external_id_replace_skips_delete confirms that a renamed tag's old id is excluded from the generated delete-action list, which is the right check at that level. But nothing calls .execute() to confirm the tag actually survives in the database after import_tags(..., replace=True) runs end to end. Issue #673 says "the Studio taxonomy import is always a full replace... this is the primary path," so the case this PR exists to fix is exercised by the wizard exclusively with replace=True. Can you add one test_api.py test that runs a rename through import_tags(replace=True) and asserts the renamed tag is still present (not just that its delete action wasn't queued at the plan stage)?
7. Issue #673's idempotent re-import scenario, previous_id equal to id, has no end-to-end test.
RenameTagExternalId.applies_for's test data covers the unit-level guard (('tag_1', 'tag_1', False)), confirming the action correctly declines to fire when previous_id equals id. But no test_api.py test runs that case through import_tags(), so nothing confirms the AC's actual scenario: re-importing a tag with previous_id set to its own current external_id succeeds with no error, and a follow-up export still shows the same id. Can you add that as a test_api.py test alongside the other rename scenarios?
I'll follow up with @thelmick-unicon to make sure that the AC in the ticket cover all of the relevant cases.
Two rows sharing the same previous_id both resolve to the same tag at validate time, since nothing has executed yet. The second row's execute() then looks up that previous_id again after the first rename has already moved it away, raising an uncaught Tag.DoesNotExist instead of a clean rejection.
RenameTagExternalId's collision check ran against the database as it is right now, not as it will be once other actions in the same import have executed. A row renaming a tag onto an external_id that a replace-mode delete sweep is simultaneously freeing up (because that id's row is omitted from the file) was rejected as already existing, even though the delete runs before the rename at execute time. Also fixes the delete-sweep itself: a rename row's id is the new target, not confirmation that the tag currently holding it should survive, so it must not protect that tag from deletion the way a normal row's id does.
…deletion RenameTag.applies_for and UpdateParentTag.applies_for look up a tag by external_id against live database state, unaware that a replace-mode delete sweep might be about to remove that exact tag in the same import. A row reusing a freed-up external_id via RenameTagExternalId could therefore also trigger RenameTag/UpdateParentTag against the tag that's about to be deleted, producing a spurious duplicate-value conflict or an uncaught Tag.DoesNotExist once the delete runs first. Both applies_for methods now skip firing when the matched tag is already queued for deletion in the same import; indexed_actions is threaded through all six applies_for implementations so the check has what it needs at the single call site in generate_actions.
… import Two or more tags renaming onto each other's current external_id values in the same import (a swap, or a longer cycle) had no valid execution order: (taxonomy, external_id) is a DB-level unique constraint enforced per-statement on every backend this project runs on, so whichever rename executes first collides with the other tag still holding its target id. Add a StageTagExternalId action that moves a tag off a contended external_id, through a temporary placeholder, before the row claiming that id lands on it. TagImportPlan.generate_actions detects contention up front (a tag's current external_id is another row's target) and stages only those tags, so plain renames are unaffected. Actions now carry a resolved target_pk instead of re-resolving by external_id at execute time, since a staged tag's external_id no longer matches its previous_id by the time it executes. Also fixes RenameTag.applies_for and UpdateParentTag.applies_for, which looked up a tag by its new external_id and could therefore match the wrong tag for any previous_id rename row, not just contended ones. Amends ADR 0010 to document that renames within one import are now order-independent, and that this doesn't extend to swapping values.
…l_id parent_id already means a tag's desired end-state parent, not the parent it had before this import, since UpdateParentTag lets a row move a tag to a different parent mid-import. _validate_parent didn't enforce that consistently: a reference to a parent's new external_id only validated when the rename came first in the file, but a reference to the parent's old external_id validated unconditionally, since the not-yet-renamed tag still resolves live in the database during validation. That was already an execute-time crash whenever the rename ran first. It gets worse with same-import swaps: a stale old id can resolve to a different tag entirely instead of just failing. _build_staging_actions now records every rename row's resolved target pk, not only the ones contended enough to need staging, so _validate_parent can reject any parent_id matching a tag being renamed away from that exact external_id in this same import, regardless of whether another tag reuses it.
…port Closes end-to-end coverage gaps against issue openedx#673's acceptance criteria: existing rename round-trip tests only ran through JSON, the replace-mode path (the one Studio's import wizard always uses) was only checked at generate_actions() level, and previous_id == id had no end-to-end test.
Description
Implements ADR 0010: the tag import file format gains a new optional, import-only column, previous_id. When a row's previous_id matches an existing tag's external_id in the taxonomy, and the row's id differs from it, the import renames that tag's external_id in place (along with any other changed fields) instead of deleting the old tag and creating a new one. This preserves the tag's primary key and existing associations (e.g. CompetencyCriteria links) across an institution-driven identifier rename.
previous_id is transient: it's read from the import file, consumed while building the import plan, and never persisted on Tag or written back out on export. No model field, no migration.
Changes
Test coverage
previous_id parsing (CSV + JSON, absent/blank → None, never exported), RenameTagExternalId applies/validate/execute, the CreateTag guard, unmatched-previous_id and new-id-collision rejections, the two validation-gap regressions above, single-action generation (no spurious create+delete pair), replace-mode delete protection, and an end-to-end import→export round-trip that preserves PK and drops the old id.