fix: reject stale parent ids on tag import and make parent changes order-independent

[ormsbee]

Preamble

@ufedaseyeuconsultant, @mgwozdz-unicon: I did this as an experiment, to see if a more capable model (Fable 5.1) could make something equivalent to #867 without as much code and complexity. I was particularly worried by _ParentForest and whether we really needed that many tests to run through our edge cases. I have not actually looked through this code in detail—what follows is entirely Claude-generated. I mostly wanted to see how well it could do for a first pass. Please take a look at this and see if it seems like a reasonable approach to you. Thank you.

Description

Alternative to #867 with the same fixes in about a third of the code. Closes #858.

Problem

UpdateParentTag.applies_for compared a row's parent_id with the parent's pre-import external_id. A row that kept the old id was "no change", so _validate_parent never ran for it, and a file that renamed tag_1 to tag_50 while a child row still said parent_id: tag_1 was accepted silently (#858).

Fixing that exposed the neighbouring problems #867 found: parent references only worked when the parent's row came first; a kept grandchild of a deleted chain was removed by the delete cascade; parent cycles and imports deeper than the limit passed the Plan step (Tag.save validates only the saved tag's depth, cascades descendants unchecked, and silently corrupts lineage on a cycle); and some valid restructures failed part-way through because an intermediate step was too deep or cyclic.

Approach

#867 searches for an action order that is valid at every intermediate step, which needs a dependency sort with subtree walks, an in-memory forest simulator, and a second plan build when the simulation fails. This PR always detaches first instead, so no simulation is needed:

1. End state once. TagImportPlan._end_state computes the tree as it will be after the import from one query. _validate_hierarchy rejects a parent cycle (once, on its first row) and a tree deeper than the limit there.
2. Detach, then attach. A synthesized DetachTag action (like StageTagExternalIdForSwap) moves every existing tag whose parent changes to the root, after staging and before the row actions. After that every tag is at or above its final depth and none is an ancestor of its new parent, so the row actions can run in any order without exceeding the depth limit or building a cycle.
3. Parents first. Rows are built after the row that creates or renames-in their parent. Each row has at most one such dependency, so the ordering is a short iterative walk, and a file whose parents already precede their children keeps its order.

For #858 itself, UpdateParentTag.applies_for now treats "same id as my current parent" as a parent change when this import gives that id to another tag, and _validate_parent rejects a row that names its own parent's pre-import id while a swap row takes that id, since it could mean "stay with my parent" or "move under the tag now called that". A child naming the end-state id of a swapped parent is accepted. _validate_parent recognises a renamed-in parent from _vacated_pks, so that check no longer depends on row order either.

What changed

All in src/openedx_tagging/import_export/:

• actions.py: _validate_parent rewritten without exception-as-control-flow, plus the own-parent check; UpdateParentTag.applies_for as above, and its description cached at plan time so the execution log doesn't read "from parent None" after the detach; new DetachTag action; "Unknown parent" message no longer tells the admin to reorder the file.
• import_plan.py: _existing_tags, _end_state, _validate_hierarchy, _build_detach_actions, _order_rows_parent_first; kept children are promoted before the first delete; a case-only rename no longer counts as vacating its id.
• exceptions.py: ParentCycleError, TagTooDeepError.

User-visible changes

• New Plan errors: Row N's parent_id (X) creates a parent cycle. and This import would leave tag X more than 6 levels deep. "Row N" is the N-th data row, as in the existing DuplicateFinalIdError.
• A stale parent id on an existing child is rejected with the existing message: Parent tag tag_1 is renamed to tag_50 in this file; use tag_50 as the parent_id.
• Unknown parent tag (X). Add a row for the parent to your file. replaces the old "add parent before the child" wording.
• A plan that moves an existing tag lists Move tag X to the root first, so that the parent changes in this file can run in any order. before the row actions. With replace=True, promotions of kept children now precede the deletes.

Supporting information

• Issue: Tag import: parent_id isn't re-validated when it matches the pre-import parent #858. Parent work: [BE] Support editing an existing tag's external_id (Competency ID) on taxonomy re-import #673, merged in feat: support editing an existing tag's external_id on taxonomy re-im… #804. Design: ADR 0010, unchanged.
• fix: make tag import parent_id handling correct and order-independent #867 is the other implementation of the same fixes; this PR is offered as a smaller alternative.

Testing instructions

Automated: pytest tests/openedx_tagging/import_export/ -o addopts="" (214 tests) and pytest tests/openedx_tagging -o addopts="". Quality: tox -e quality.

Manual, through the taxonomy import endpoint or the Studio import wizard, on a taxonomy with tag_1 (root), tag_2 under tag_1, tag_3 (root), tag_4 under tag_3:

File (JSON tags rows)	Expected
tag_50 (previous_id tag_1), tag_2 (parent_id tag_1), tag_3, tag_4 (parent_id tag_3)	Rejected at the Plan step: Parent tag tag_1 is renamed to tag_50 in this file; use tag_50 as the parent_id.
tag_3 (previous_id tag_1), tag_1 (previous_id tag_3), tag_2 (parent_id tag_3), tag_4 (parent_id tag_1), in any row order	Imports. Each child stays on the same physical parent; the export shows tag_2 under tag_3 and tag_4 under tag_1.
tag_1 (parent_id tag_2), plus the other three rows unchanged	Rejected: Row 1's parent_id (tag_2) creates a parent cycle.

Other information

• No migrations, no model changes, nothing in openedx-platform needs to change.
• Existing tests that changed: two replace=True plan expectations (promotion before deletes, order only); two plans that move an existing tag gain the detach line; the "Unknown parent" wording; one _validate_parent unit test now sets _vacated_pks instead of a built rename action.
• Design choices open to discussion: a tag that moves to the root gets both the detach line and its row's "update parent to None" line, which is a no-op by then. The too-deep error names the shallowest tag that would cross the limit, which can be an unchanged descendant rather than a row.
• AI tools: Claude Code with Claude Fable 5.1 (claude-fable-5-1).

🤖 Generated with Claude Code

[mgwozdz-unicon]

I had Opus do an analysis of this PR to compare it with #867 and it claims that it does cover the same ground with a small caveat and less testing. Claude drafted the following comment to share its analysis:

To compare the two PRs, I ran #867's tests against this branch's code, because #867's tests map one-to-one to the acceptance criteria in #858, which Seva updated on 10-14 to cover all the extra fixes. I ran them on SQLite only. test_import_plan.py from #867 couldn't run here, because it imports #867's private _ParentForest class, so I checked plan-level behavior with a couple of direct generate_actions calls instead.

Coverage. Behaviorally, this PR covers nearly all of #858. Out of 199 tests, 156 pass, including the swap, row-order, delete-cascade, depth-limit, intermediate-step and case-variant scenarios. Most of the 43 failures are only wording differences: this PR keeps the existing "Parent tag X is renamed to Y in this file" message, and its depth error names the tag rather than using the ticket's quoted text. Both of those are fine with me. One mixed cycle (a rename row plus a new tag) gets the cycle error here instead of "Unknown parent tag", which #858 doesn't specify either way. Two gaps remain:

1. The stale-parent error doesn't identify the row. ACs 1 and 3 and Additional fix 6 in Tag import: parent_id isn't re-validated when it matches the pre-import parent #858 require the error to name the child's row. Here the message reads Action error in 'update_parent' (#3): Parent tag COMP-P is renamed to COMP-P2 in this file; use COMP-P2 as the parent_id. In my test file the offending row was row 2, and the #3 is the action's position in the plan, so an admin can't map the error back to a line in their file. Adding the row number (the same "Row N" convention as DuplicateFinalIdError and the new ParentCycleError) or the child's id would close this.
2. Every re-parent of an existing tag now adds a "Move tag X to the root first" action, including a simple move of c from under a to under b in an otherwise unchanged file. That conflicts with two ACs in Tag import: parent_id isn't re-validated when it matches the pre-import parent #858: Additional fix 1 says a file that already lists parents before children produces the same plan as before, and Additional fix 5 says a plan whose steps are all valid has no extra move-to-root actions.

Whether the gaps matter. I don't think either one is a correctness problem. The row number is a small usability fix that I'd like to see in whichever PR we go with. The extra move-to-root lines are the price of the always-detach approach, and they're what lets this PR drop the in-memory simulation and the second plan build. I think that's a reasonable trade, and that the two ACs it breaks describe #867's mechanism more than an outcome an admin needs. If we go this way, I'd rather amend those two ACs in #858 than add the simulation back.

The bigger difference between the PRs is test depth, not behavior. This PR adds about 360 lines of tests against about 1,900 in #867. Whichever implementation we keep, I'd suggest bringing over #867's TestImportAcceptanceScenarios class (adjusted for message wording), because it is the part that ties each #858 criterion to a test across JSON, CSV and row orders.