fix: make tag import parent_id handling correct and order-independent

[ufedaseyeuconsultant]

Description

Follow-up to #673 (merged in #804). It closes #858 and finishes the parts of #673's acceptance criteria that the merged PR left open.

Problem

UpdateParentTag.applies_for decides whether a row's parent needs updating by comparing the row's parent_id with the tag's pre-import parent external_id. When they match, the row is "no change" and _validate_parent never runs for it. Two cases came out wrong:

• An existing child whose row keeps parent_id="tag_1" while the same file renames tag_1 to tag_50 was accepted silently. [BE] Support editing an existing tag's external_id (Competency ID) on taxonomy re-import #673's scenario "a child row referencing its parent's pre-import external_id is rejected" is about exactly this case. The existing regression tests only covered a brand-new child, which takes another code path.
• A first attempt at the fix (reverted) rejected every parent_id that named an id both given up and claimed in a swap. That also rejected a child that already names the correct end-state id, so two swapped parents with children could not be imported at all.

The root cause is the same in both: validation was keyed on the wrong condition. The only genuinely ambiguous reading is a row whose own parent_id equals its own current parent's pre-import id while the same file renames that parent to another id.

While fixing this, a randomized end-state test showed that neighboring cases failed the same way (the plan looked valid, then the import crashed or produced a wrong tree). #673 also requires that the outcome does not depend on the order of the rows, which was not true for parent references. This PR fixes those too. #858 now describes each of these extra fixes with its own acceptance criteria.

What changed

All code is in src/openedx_tagging/import_export/.

Validation, all at the Plan step and before any write:

• A row whose parent_id is its current parent's pre-import id is rejected when the same file renames that parent. The error names the row and the parent's new id. A child that names the end-state id of a swapped parent is still accepted. A case variant of the stale id (TAG_1 for tag_1) is treated like the exact spelling.
• A parent_id that makes a tag its own ancestor (a self-parent, or a cycle through existing tags) is rejected. Cycles made only of new or renamed rows keep the existing "Unknown parent tag" error and are not reported twice.
• An import whose result would be deeper than the taxonomy depth limit is rejected. Before, it either failed at execute time or, through the descendant update in Tag.save, stored tags deeper than the limit.

Order independence:

• Parent references used to work only if the parent's row came first, because each action is validated and executed in file order. Rows are now ordered by parent dependency before the actions are built. A file that is already in a valid order produces the same plan as before.
• With replace=True, kept children of a deleted parent are now promoted to root before any DeleteTag runs. Before, a kept tag under a deleted parent that itself sat under a deleted ancestor was removed by the cascade and the import failed at execute time.
• Some restructures are only valid if tags are moved to the root first, for example a deep tag moved to root while new children are created under it. An in-memory simulation (_ParentForest) checks the parent effects of the action list (cycle, depth, missing tag) without touching the database. If a step would be invalid, the plan is rebuilt once: every existing tag whose parent changes is first moved to the root, and then the rows are applied with each parent before its children. Ordinary plans never take this path.

The plan-level checks share one query for the taxonomy's tags instead of querying per row. The rationale for each rule is in the docstrings of the methods in import_plan.py, and the TagImportPlan docstring defines the terms they use.

Changes after the review

In src, the changes that followed the review touch only docstrings, comments, names and the structure of conditions. The behavior is unchanged, and the randomized end-state test described below gives the same counts as before. _validate_parent_id_not_stale explains the swap ambiguity with a concrete example and splits its conditions into named booleans, the terms the plan-level checks use are defined once, and the private safe flag of _generate_actions is now called detach_first. The tests were cut to one test per scenario, from 88 to 58 new test functions in tests/openedx_tagging/import_export (219 to 189 counted with grep "def test_", against 131 on main). Where a deleted test covered a case the kept test did not, the case moved into the kept test. The tests that stay use keyword arguments, tag names that say their role, and a before/after tree in each docstring.

Who is affected

• Platform Administrators who import taxonomies with the previous_id column: some files that were accepted and then misbehaved are now rejected with an error that names the row, and some files that were rejected only because of their row order now import.
• Developers extending the import actions: the order of the actions in a plan can change (see below).

User-visible changes

• New Plan errors:
  • Row N's parent_id (X) names an id this file renames away. Use 'Y' as the parent_id.
  • Row N's parent_id (X) creates a parent cycle.
  • This import would leave tags deeper than 6 levels, or build an invalid parent hierarchy, so it cannot be applied.
• "Row N" is the N-th data row of the file, counting from 1, like the existing DuplicateFinalIdError. In a CSV file that is spreadsheet line N + 1, because the header is line 1. The CSV parser's own errors use spreadsheet line numbers, so the two numbers differ by one. In a JSON file it is the N-th entry of tags.
• The "Unknown parent tag" message now ends with "Add a row for the parent to your file." because a parent no longer has to come before its child.
• With replace=True the plan lists the promotion of kept children to root before the delete actions. Two expected plans in the existing tests (test_plan_3, test_generate_actions_3) changed only in action order.
• A plan that had to be rebuilt with the affected tags moved to the root first shows extra "Update the parent of X ... to None" lines at the start. They are the same action the delete phase already uses.

Supporting information

• Issue: Tag import: parent_id isn't re-validated when it matches the pre-import parent #858, which describes the original ask and each extra fix in this PR with acceptance criteria.
• 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 by this PR).
• Review comments on feat: support editing an existing tag's external_id on taxonomy re-im… #804 that led to this follow-up:
  • 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)
• AI tools: Claude Code. Claude Sonnet 5.5 wrote the original implementation and tests; Claude Opus 5.5 (claude-opus-5-5) made the changes that followed the review.

Testing instructions

Automated:

1. pytest tests/openedx_tagging/import_export/ -o addopts="" (243 tests; -o addopts="" skips the coverage flags that tox.ini adds).
2. pytest tests/openedx_tagging -o addopts="" (629 tests, to catch interactions with the rest of the tagging tests).
3. The quality checks as CI runs them: lint-imports, pylint src tests test_utils manage.py test_settings.py mysql_test_settings.py, pycodestyle, isort --check-only, pydocstyle src and mypy --show-traceback (mypy also checks the tests).

TestImportAcceptanceScenarios in tests/openedx_tagging/import_export/test_api.py runs #673's scenarios at the level the wizard uses (import_tags, export_tags, the import log), for JSON and CSV, with replace=True. Each test names the #673 scenario it covers by its title. The scenarios that older tests in TestImportExportApi already cover, and the swap criteria of #858, are listed in the class docstring with the tests that cover them.

Manual, through the taxonomy import endpoint or the Studio import wizard. Create a taxonomy with tag_1 (root), tag_2 (child of tag_1), tag_3 (root) and tag_4 (child of tag_3), with values Tag 1 to Tag 4. Then import each file below (the import is a full replace, so each file lists the whole taxonomy):

File (JSON tags rows)	Expected
{id: tag_50, value: "Tag 1", previous_id: tag_1}, {id: tag_2, value: "Tag 2", parent_id: tag_1}, {id: tag_3, value: "Tag 3"}, {id: tag_4, value: "Tag 4", parent_id: tag_3}	Rejected at the Plan step: "Row 2's parent_id (tag_1) names an id this file renames away. Use 'tag_50' as the parent_id." Nothing changes.
{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}	Imports. Each child stays on the same physical parent, and the export shows tag_2 under tag_3 and tag_4 under tag_1.
The same rows as the second file, with the two child rows first	Same result as the second file.

Other information

• No migrations, no model changes, no new dependencies, nothing in openedx-platform needs to change.
• Existing tests that changed:
  • test_plan_3 and test_generate_actions_3 (test_import_plan.py): the expected plan lists the promotion of the kept child before the deletes. Only the action order differs.
  • The tests in test_actions.py and test_import_plan.py that assert the full text of the "Unknown parent tag" message, for the new wording.
• TestApiTagging.test_get_taxonomy (tests/openedx_tagging/test_api.py) is unchanged, but it failed in the first CI run of this PR. It asserts that taxonomy id 200 does not exist. CI runs on MySQL, where auto-increment ids are not rolled back with the per-test transaction, so the number of taxonomies created earlier in the run decides which id the "Free Text" taxonomy of that test's setUp gets. The new import tests created a taxonomy per test, 45 more than main before that test, so it got id 200. They now create their taxonomy once per class, 2 more than main, so in the same CI order the id is 157 (155 on main) and the test passes. The hard-coded id stays as fragile on MySQL as it is on main.
• Beyond the committed tests I ran a randomized end-state fuzz (not committed, about 5 minutes per large run): random valid end states over small taxonomies (renames, swaps, rotations, reclaimed ids, new tags, re-parenting, deletes by omission, deep chains, ancestry inversions, random capitalization of ids), imported in 4 row orders with replace True and False, compared with an expected tree. About 27,000 imports ended with no execute-time crash, no valid import rejected and no wrong tree. The fuzz found the delete cascade, the transient cycle and the transient depth overflow described above, and all three also fail on the base branch. After the review changes the same runs gave the same counts.
• The diff is large (about 2,470 added lines, about 1,900 of them tests). Suggested reading order:
  1. import_plan.py: the TagImportPlan docstring, which defines the terms, then _validate_parent_id_not_stale and _validate_no_parent_cycles, the direct fix for Tag import: parent_id isn't re-validated when it matches the pre-import parent #858 and its cycle follow-ups.
  2. import_plan.py: _order_rows_by_parent_dependency and _build_delete_actions, which make parent references independent of row order and fix the delete cascade.
  3. import_plan.py: _ParentForest, _parent_changes_are_valid and the rebuild with detach_first in generate_actions. This is the largest and most self-contained part and can be reviewed on its own.
  4. exceptions.py and the docstring in actions.py.
  5. Tests: start with TestImportAcceptanceScenarios, then the plan-level tests in test_import_plan.py.
• Limitations and decisions to look at:
  • Locally I could only run SQLite; the MySQL job in CI is the real check for the database-dependent parts. Plan-level keys use casefold(), as the existing plan checks do, while database lookups use the field's collation. For characters where the two disagree (accent or width variants on MySQL) a plan check could differ from the per-row action. I expect a missed check, not data corruption, but I did not verify it.
  • A child that wants to move under the swap partner of its current parent cannot say so: naming that id is exactly the ambiguous reading that [BE] Support editing an existing tag's external_id (Competency ID) on taxonomy re-import #673 requires rejecting.
  • replace=True still promotes the listed children of a deleted parent to root when nothing reclaims the deleted id. Existing tests pin this and I kept it.
  • When a plan has to be rebuilt with the affected tags moved to the root first, the plan is built twice. On a file where every tag of a 2,000-row taxonomy is renamed and moved, generate_actions took about 25 seconds, almost all of it the existing per-action validation. Ordinary imports do not pay this.
  • ADR 0010 is unchanged. If you want the parent-reference rule written down there, I can add a short paragraph.

---

🤖 Generated with Claude Code

[openedx-webhooks]

Thanks for the pull request, @ufedaseyeuconsultant!

This repository is currently maintained by @axim-engineering.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where 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:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

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:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[mgwozdz-unicon]

Claude and I both reviewed. Claude drafted this comment:

Requesting changes, mainly on readability and test size. The fix for #858 itself looks right to me, but it is a small part of a roughly 2,400-line diff, and the rest is hard to review with confidence in its current form. I've already read the whole diff, so please keep everything in this PR and push the changes below as new commits on top, without force-pushing, so I can review just what changed since this review.

1. Please add the extra scope to #858's description.

#858 asks for one thing: reject a row whose parent_id equals its current parent's pre-import id when the same file renames that parent, and keep accepting a child that names the end-state id of a swapped parent. Most of the new code fixes other bugs your fuzz run found, which also fail on the base branch: parent cycles, the depth limit, parent references that depended on row order, kept children lost to the delete cascade, and steps that are only invalid partway through the import (the _ParentForest dry run and the detach-first rebuild). Those fixes are fine to keep in this PR, but #858 should describe each one and add acceptance criteria for it, so the ticket matches what this PR delivers.

2. Please make the code and docstrings readable for someone who wasn't in your head while writing it.

• The _validate_parent_id_not_stale docstring should explain the swap ambiguity with a concrete example, because that is the part a reader is most likely to get wrong. For example: before the import, tag_2 is under tag_1. The file swaps tag_1 and tag_3. After the import, the id tag_1 belongs to a different tag (the old tag_3), so a tag_2 row that still says parent_id: tag_1 could mean "stay with my parent" or "move under the tag now called tag_1". Because it is ambiguous, it is rejected, and the error tells the admin to use tag_3.
• The three-part if conditions in _validate_parent_id_not_stale are hard to follow. Splitting them into named booleans would let each one say what case it is.
• Terms like "renamed away", "stale", "definer row", "detach-first", and safe are used without being defined. Please define each term once where it is first used, or replace it with plain wording.
• "Rows are reported by 1-based position in the parsed row list, not a file line" should say what that means for the admin: "Row 1" is the first data row, so in a CSV it is one less than the spreadsheet line number.

3. Please cut the tests down to one test per scenario.

The PR adds 88 test functions, and many scenarios are tested twice: once at the plan level and once through the API, or once in this PR and once in an existing test on main. Below are the ones I'd delete. In each pair, the test I'd keep either covers more cases or checks the database result and the export, not just the plan. I compared these by reading them, so if you find a case that the deleted test covers and the kept one doesn't, keep it and tell me which.

Delete these tests in TestImportAcceptanceScenarios, which repeat a test that already exists:

• test_reimport_with_previous_id_equal_to_id_is_idempotent (main has test_import_rename_external_id_previous_id_equals_id_is_noop)
• test_unmatched_previous_id_rejected (main has test_import_rename_external_id_unmatched_previous_id_rejected)
• test_rename_onto_id_freed_by_omitting_holder (main has test_import_rename_external_id_reuses_id_freed_by_replace_delete)
• test_parent_and_child_both_renamed (main has test_import_rename_parent_and_child_both_renamed)
• test_child_names_pre_import_id_of_renamed_parent_rejected (this PR has test_import_existing_child_names_renamed_away_parent_rejected)
• test_swap_with_children_in_representative_orders (this PR has test_import_swap_with_children_in_any_row_order and test_import_swap_children_stale_ids_rejected_in_any_row_order)
• test_case_only_parent_rename_with_case_variant_child (this PR has test_import_case_only_parent_rename_keeps_child_parent)

Delete these tests in test_import_plan.py, which use the same rows as an API test in this PR:

• test_generate_actions_ancestry_inversion_accepted_in_any_row_order (keep test_ancestry_inversion_in_any_row_order)
• test_generate_actions_parent_and_child_swap_places_in_any_row_order (keep test_parent_and_child_swap_places)
• test_generate_actions_three_level_inversion_accepted_in_any_row_order (keep test_three_level_chain_reversed)
• test_generate_actions_deep_parent_move_detaches_before_attaching (keep test_deep_tag_moved_to_root_with_new_children_in_any_row_order)
• test_generate_actions_deep_tag_under_renamed_parent_moved_to_root (keep test_deep_tag_under_renamed_parent_moved_to_root_in_any_row_order)
• test_generate_actions_subtree_moved_under_deep_tag_before_it_is_shallowed (keep test_subtree_moved_under_deep_tag_before_it_is_shallowed)
• test_generate_actions_three_level_new_chain_under_moved_deep_tag (keep test_three_new_levels_under_moved_deep_tag)
• test_generate_actions_rejects_a_new_tag_below_the_maximum_depth and test_generate_actions_rejects_a_subtree_moved_too_deep (keep test_import_that_leaves_tags_too_deep_is_rejected)
• test_generate_actions_kept_grandchild_of_deleted_chain_promoted_before_deletes, test_generate_actions_kept_tags_at_two_depths_under_deleted_ancestors and test_generate_actions_kept_child_of_deleted_root_promoted (keep test_kept_tags_under_deleted_ancestors_become_roots)
• test_generate_actions_new_child_before_new_parent_accepted (keep test_import_new_child_before_new_parent_row)
• test_generate_actions_existing_and_new_tag_cycle_rejected_once and test_generate_actions_cycle_through_db_only_tag_rejected (keep test_import_parent_cycles_through_existing_tags_rejected)
• test_generate_actions_cyclic_parent_ids_rejected and test_generate_actions_cycle_of_definer_rows_reports_only_unknown_parent (keep test_import_cyclic_parent_ids_rejected and test_import_longer_and_mixed_parent_cycles_rejected_in_any_row_order)
• test_generate_actions_self_parent_after_rename_onto_freed_id_rejected (keep test_rename_onto_freed_id_forming_parent_cycle_rejected)
• test_generate_actions_case_only_parent_rename_with_case_variant_child_accepted (keep test_generate_actions_case_only_parent_rename_not_flagged and test_import_case_only_parent_rename_keeps_child_parent)

Delete this test in TestImportExportApi, which uses the same rows as a plan test:

• test_import_swap_rename_child_stale_parent_id_rejected_once (keep test_generate_actions_swap_rename_child_stale_parent_id_rejected_once)

Delete these tests, which pin the implementation rather than the behavior, so they would break on a refactor that changes nothing an admin can see:

• test_defines_id_is_shared_by_ordering_and_cycle_check, which patches the private _defines_id.
• test_generate_actions_rebuilds_a_hazardous_plan_only_once, which patches _parent_changes_are_valid and counts calls.
• test_rows_ordered_as_definers_match_create_and_rename_applies_for, which compares the ordering against CreateTag.applies_for and RenameTagExternalId.applies_for.

After these deletions, nothing uses the _passes_dry_run, _assert_detached_first, _assert_promotions_precede_deletes and _update_parent_index helpers in test_import_plan.py, so please delete them too. That also removes a circular check: _passes_dry_run checks a plan with _ParentForest, the same code the plan relies on, so it couldn't catch a bug in _ParentForest. test_parent_dry_run_accepts_valid_and_rejects_invalid_sequences tests _ParentForest directly and should stay.

For the tests that stay, please make them readable from the code alone. Tags named t0, g1, and s2, created through a positional _row(...) helper, make it hard to see what moves where. The test_import_plan.py style with keyword arguments is much easier to follow, and a short before/after tree in the docstring would help too. The "S1" to "S16" labels in the acceptance docstrings aren't defined anywhere a reader can find, so please remove them or say what they refer to. Where a test samples row orders (24 for JSON, 4 for CSV), add a comment explaining why.

4. Smaller items

• The PR description says it was generated with Claude Code but doesn't name the model. The commits say Claude Sonnet 5.5. Please name it in the description, since Open edX asks for the specific AI tool. The Open edX community has also asked that we use Opus or higher from now on, so please make the changes for this review with Opus or higher, not Sonnet.
• The test_get_taxonomy change in tests/openedx_tagging/test_api.py is only needed because of the number of taxonomies the new tests create. Once the tests are cut down, please check whether it is still needed.

[ufedaseyeuconsultant]

@mgwozdz-unicon Done