Tag import: parent_id isn't re-validated when it matches the pre-import parent #858

Summary

UpdateParentTag.applies_for (src/openedx_tagging/import_export/actions.py:342-355) decides whether a row's parent needs updating by comparing the row's parent_id against the tag's pre-import parent. When they match textually, it concludes "no change" and skips the row entirely, so _validate_parent (the method that actually resolves parent_id against renames happening in the same file) never runs. This produces two distinct, confirmed-wrong outcomes, both tracing back to the same root cause.

Problem A: a stale parent_id is silently accepted for an existing child (violates #673's own AC)

Issue #673's acceptance criteria include this scenario:

Scenario: a child row referencing its parent's pre-import external_id is rejected
Given a taxonomy contains parent competency P (external_id "COMP-P") and child competency C beneath it
When the admin imports a file with a row previous_id "COMP-P" / id "COMP-P2"
And the same file has a row for C with parent_id "COMP-P"
Then the wizard shows a validation error identifying the child row
And the error states that the parent_id names an id the same file renames away, and names the new id to use instead

"C beneath it" means C already exists in the taxonomy before the import, not that it's being created by the same file. Confirmed against the current implementation: renaming tag_1 to tag_50 while an existing tag_2 (already a child of tag_1) keeps parent_id="tag_1" in its own row produces no error. The plan reads #2: No changes needed for <TagItem> (tag_2 / Tag 2), and the import succeeds.

The existing regression tests for this AC scenario (test_generate_actions_parent_id_stale_after_plain_rename_rejected, test_import_rename_referencing_stale_old_id_rejected) both use a brand-new tag as the child row, which goes through CreateTag, a different code path that always calls _validate_parent regardless. Neither test exercises an already-existing child going through UpdateParentTag, which is the literal case the AC describes and the one that's broken.

Problem B: a valid end-state parent_id is wrongly rejected in a swap

Not covered by #673's AC at all (no scenario combines the swap scenario with children referencing the swapped tags), found during review of a first attempt at fixing Problem A.

A first fix (commit aa9ea33, reverted in a8e9d67) added a pre-pass that rejected any parent_id naming an id that is both vacated by one rename row and claimed by a different rename row in the same file (a swap or cycle). That overshoots: in a swap, both ids are simultaneously vacated and claimed, so the check fired on every parent_id naming either id, including a child row whose parent_id already names the correct end-state id. Reproduction:

{"tags": [
  {"id": "tag_3", "value": "Tag 1", "previous_id": "tag_1"},
  {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"},
  {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_3"},
  {"id": "tag_4", "value": "Tag 4", "parent_id": "tag_1"}
]}

With aa9ea33 in place, both tag_2's and tag_4's rows are rejected, even though tag_2.parent_id="tag_3" correctly names the tag that will hold tag_3 after the swap. There is no value of parent_id that passes for a full-replace swap of two parents with children, so this case can't be imported through the Studio wizard at all with that fix in place.

Root cause and proposed fix

Both problems come from keying the "does this need validation" decision on the wrong condition. The correct condition is: does this row's own parent_id match its own current (pre-import) parent? That's the one reading that's genuinely ambiguous (it's exactly the condition UpdateParentTag.applies_for uses to skip validation), regardless of whether the id involved happens to be contended by another row:

current = self.taxonomy.tag_set.filter(
    external_id=tag.previous_id or tag.id
).select_related("parent").first()
if (
    current is None
    or current.parent is None
    or current.parent.external_id.casefold() != tag.parent_id.casefold()
):
    continue

Traced through both problem cases:

• Problem A (tag_2, no previous_id, parent_id="tag_1"): current resolves to tag_2 itself, current.parent.external_id is "tag_1" (pre-import), which matches tag.parent_id exactly, so the row is flagged and rejected, correctly.
• Problem B (tag_2, parent_id="tag_3", a forward reference to a tag that doesn't exist under that id yet pre-import): current is still tag_2 itself, but current.parent.external_id is "tag_1", which does not match tag.parent_id="tag_3", so the row is not flagged here; it proceeds to its own UpdateParentTag/_validate_parent, which already resolves the swap's end-state id correctly via the existing execution order (renames run before parent updates).
• The accepted forward-reference case for a brand-new tag (test_generate_actions_parent_id_new_id_after_earlier_rename_accepted) also still works: current resolves to None (the new tag doesn't exist pre-import), so the row is never flagged here and falls through to CreateTag's own _validate_parent.

This single condition replaces the need for any separate "is this id contended" check, so there's one rule instead of two.

Also fix, in the same pass: the current comment/docstring near this logic claims the plain-rename case is already safe because of _validate_parent. That's not accurate: a row with no other change becomes WithoutChanges and never calls _validate_parent at all. It only comes out correct today because nothing physically moves for a plain rename; Problem A shows that "coming out correct" and "being validated" are different things once an AC scenario demands an explicit rejection regardless of outcome.

Acceptance criteria for the fix

• Problem A's exact AC scenario from [BE] Support editing an existing tag's external_id (Competency ID) on taxonomy re-import #673 (existing child, plain rename, stale parent_id) is rejected with an error naming the row and the correct new id to use.
• Problem B's repro (swap with two children, each referencing the opposite swap partner's correct end-state id) succeeds and leaves both children on the correct physical parent.
• A genuinely ambiguous swap reference (a child's parent_id naming the pre-import/stale id of a tag involved in a swap) is still rejected.
• test_generate_actions_parent_id_new_id_after_earlier_rename_accepted (forward reference to a plain rename's new id) and test_generate_actions_rename_external_id_replace_skips_delete* (replace-mode survival) continue to pass unchanged.

Starting point

Branch ufedaseyeu/parent-id-swap-ambiguity-wip (based on the reverted aa9ea33) holds the first, incorrect ("contended"-keyed) implementation and its tests. Useful as a reference for what doesn't work and for the method/exception skeleton, not as logic to reuse as-is.

References

• [BE] Support editing an existing tag's external_id (Competency ID) on taxonomy re-import #673 (the PR this follow-up splits off from), specifically its "a child row referencing its parent's pre-import external_id is rejected" and "two competencies swap external_ids in one import" acceptance scenarios.(
  feat: support editing an existing tag's external_id on taxonomy re-im… #804 (comment)
  feat: support editing an existing tag's external_id on taxonomy re-im… #804 (comment)
  )
• ADR 0010, docs/openedx_tagging/decisions/0010-mutable-tag-external-id.rst (the design this import mechanism implements).