feat: support editing an existing tag's external_id on taxonomy re-im… - #804
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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
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.
3 participants
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.