feat: support editing an existing tag's external_id on taxonomy re-im…

[ufedaseyeuconsultant]

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

• src/openedx_tagging/import_export/parsers.py: Parser gets a new import_only_fields = ["previous_id"] class attribute, parsed with the same missing/blank → None coercion as optional_fields. Unlike optional_fields, it's never added to CSV headers, never required by _verify_header, and never emitted by export.
• src/openedx_tagging/import_export/import_plan.py: TagItem gains previous_id: str | None = None. In generate_actions's replace-mode delete sweep, a renamed tag's old id is now also excluded from deletion (its new id was never a delete-sweep key to begin with).
• src/openedx_tagging/import_export/actions.py: new RenameTagExternalId action (name = "rename_external_id"), registered in available_actions right before CreateTag. It fires when previous_id is set and differs from id; validates that previous_id matches an existing tag, that the new id doesn't collide with another tag (in the DB or already queued in this import), and value/parent as needed; execute() updates the matched tag's external_id, value, and parent in place. CreateTag.applies_for is guarded off for rename rows so they're never double-handled.
• Fixed a related gap surfaced during review: the shared _validate_value/_validate_parent helpers only recognized CreateTag/RenameTag as sources of a same-import forward reference. Without also checking RenameTagExternalId, a value collision between a rename row and another row in the same import could slip past validation and hit an uncaught IntegrityError at execute time, and a legitimate parent reference to a renamed-in tag would be falsely rejected. Both helpers now also check RenameTagExternalId entries.

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.

Tested via

• Unit tests
• Studio UI
• API

🤖 Generated with help of 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 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.

[mgwozdz-unicon]

Thanks for the update, I re-reviewed and all seven items from the previous review are addressed. Great work!

Three things I'd like addressed before merge.

1. Naming: rename StageTagExternalId to StageTagExternalIdForSwap, and is_staged_away to is_awaiting_placeholder_swap. Both current names read clearly once you've read their docstrings, but "staged" alone doesn't say what it's staged for, and a reader hitting either name cold has to go find the docstring to learn that a tag is moved to a placeholder id specifically so another row's rename can claim its old id, ahead of the tag's own rename landing on its final id right after. StageTagExternalIdForSwap and is_awaiting_placeholder_swap say that directly and share the same word, so the class and the check it feeds read as one connected idea instead of two.

2. Duplication in actions.py between RenameTag.applies_for and UpdateParentTag.applies_for. Both have the identical docstring paragraph about not applying when the matched tag is queued for deletion or is itself a previous_id rename row, and near-identical bodies: the same delete-queued check, the same early return for previous_id rows, differing only in the final comparison (value changed vs. parent changed). Please extract the shared check into one helper both call.

3. Confirmed bug: two rows in the same import can specify the same final id with nothing catching it, and depending on row order that either crashes the import or silently overwrites the wrong tag. Claude ran this end to end against your branch to confirm it. Given tag_1 and tag_3 swapping external_ids in one import, plus a third, unrelated row that also targets id: "tag_1" with a different value:

{"tags": [
  {"id": "tag_3", "value": "Tag 1", "previous_id": "tag_1"},
  {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"},
  {"id": "tag_1", "value": "Something Else Entirely"}
]}

If the third row is processed after the two rename rows, as above, the import reports success but silently overwrites the tag that the second row just renamed onto tag_1, discarding the value that row asked for. Move the third row to the front of the file instead, and the import fails, with an uncaught Tag.DoesNotExist in the task log. Neither outcome is right: this file should be rejected outright, since the second row and the third row both claim tag_1 as their final id, and the same tag can't simultaneously be the one renamed in from tag_3 and the one holding an unrelated row's new value.

That's the actual bug: a tag only ever gets staged by StageTagExternalIdForSwap (the rename from item 1) when its current external_id is already another row's explicit target id, so any row that collides with a staged tag is, by construction, also duplicating that other row's id. Please add a check that rejects two rows in the same import specifying the same final id, run before the swap-staging and per-row action logic, so a file like the one above fails cleanly at the Plan step with an error identifying both offending rows, the same way a duplicate previous_id already does.

[mgwozdz-unicon]

Thank you for addressing those! This looks good. My only remaining nit would be to remove where some of the docstrings talk about how things were "previously". In probably all cases except maybe the unit tests, this reads as AI bloat to me.

[kdmccormick]

Hey @ufedaseyeuconsultant , thanks for the PR. Here are couple preliminary comments. I'm still reviewing the business logic, should have that done today or tomorrow.

Please take a look through the Open edX AI policy and make sure you're following it: https://github.com/openedx/.github/blob/master/AI_POLICY.md

Also please share in the PR description how you manually tested this PR.

[kdmccormick]

this is very long and rambly. please reword it so that it's as straightforward as possible while still including the things a future developer would need to know (and nothing more).

you cannot count on LLMs to write good docstings; you need to review them yourself and edit them until they are easy for a human to read.

[ufedaseyeuconsultant]

done, check if docstring are ok now please

[kdmccormick]

Suggested change

	def __init__(self, taxonomy: Taxonomy, tag, index: int, target_pk: int | None = None):
	def __init__(self, taxonomy: Taxonomy, tag: TagItem, index: int, target_pk: int | None = None):

could you type-annotate this and all other instances of tag that you've touched? I believe the annotation here is TagItem, from ..import_plan.

not your fault that it was originally un-annotated, but going forward this will help with review and help make sure that the typechecker is finding as many bugs as possible in the new code.

[ufedaseyeuconsultant]

done

[kdmccormick]

@ufedaseyeuconsultant Please read and follow the AI Policy. https://github.com/openedx/.github/blob/master/AI_POLICY.md

[ufedaseyeuconsultant]

@kdmccormick Please let me know which part of the policy I need to correct. Are you referring to co-authorship?

[kdmccormick]

@ufedaseyeuconsultant yes I was, thank you for adding that. Working on another review round now

[ormsbee]

This is the same shape as the duplicate-final-id bug from the last round, but with distinct final ids, so _validate_no_duplicate_final_ids doesn't catch it. A row with no previous_id whose id is currently held by a tag that a different row is renaming away resolves to the departing tag here, and RenameTag/UpdateParentTag then fire against it.

Claude ran this against the test fixture (where tag_1 exists):

{"tags": [
  {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"},
  {"id": "tag_1", "value": "Brand New"}
]}

The plan it generates (this is what shows at the Plan step):

Import plan for Import Taxonomy Test
--------------------------------
#1: Rename external_id of tag with previous_id=tag_1 to 'tag_50' (value=Tag 1, parent_id=None).
#2: Rename tag value of <Tag> (tag_1 / Tag 1) to 'Brand New'

No errors, and (2) is pointed at the tag that (1) is about to rename. At execute time (1) moves the tag to tag_50, so (2)'s _get_tag() raises DoesNotExist('Tag matching query does not exist.'), and the task log ends like this, with no row identified:

#1: Rename external_id of tag with previous_id=tag_1 to 'tag_50' (value=Tag 1, parent_id=None). [Started]
Success
DoesNotExist('Tag matching query does not exist.')

Put the two rows in the other order and the plan is:

Import plan for Import Taxonomy Test
--------------------------------
#1: Rename tag value of <Tag> (tag_1 / Tag 1) to 'Brand New'
#2: Rename external_id of tag with previous_id=tag_1 to 'tag_50' (value=Tag 1, parent_id=None).

That one succeeds: (1) sets the value to "Brand New", (2) renames the tag and sets the value back to "Tag 1", and the "Brand New" row is gone with no error (the taxonomy ends up with tag_50 / Tag 1 and nothing else changed).

I think the minimum for this PR is to reject it at the plan step, and it can be done on the TagItems alone, next to _validate_no_duplicate_final_ids: a row without a previous_id (or with previous_id == id) may not have an id equal to another row's previous_id. A rename row landing on that id is the swap case you already support, so it's only the plain rows that need rejecting. Actually supporting "rename A→B and create a fresh A in the same file" would mean staging any tag whose current id is any row's final id, not just a rename row's target; I'd leave that for a follow-up unless it falls out easily. Either way, please add end-to-end tests for both row orders, like you did for the duplicate-final-id case.

[ormsbee]

Question: Is this branch reachable from generate_actions()? As far as I can tell, a row without previous_id always pops its own id out of tags_for_delete in the replace block, so no delete action can match a non-rename row's id by the time we get here, and rename rows have already returned None above. Claude instrumented this branch and ran test_api.py and test_import_plan.py: it never fired (only the two direct unit tests in test_actions.py reach it). If that's right, please remove it along with those two tests. That's one less rule for the next person to hold in their head when they're working out why a row got matched to the tag it did.

[ormsbee]

external_id is a case_insensitive_char_field, so the (taxonomy, external_id) unique constraint is case-insensitive (NOCASE on SQLite, utf8mb4_unicode_ci on MySQL). Every new Python-side comparison in this PR is case-sensitive, though: the dict keys here, target_ids in _build_staging_actions, is_freed_by_delete, and the _search_action lookups. So the plan and the database disagree about which ids are the same id.

Claude ran this file against the fixture:

{"tags": [
  {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"},
  {"id": "TAG_50", "value": "Tag 3", "previous_id": "tag_3"}
]}

The plan passes with no errors:

Import plan for Import Taxonomy Test
--------------------------------
#1: Rename external_id of tag with previous_id=tag_1 to 'tag_50' (value=Tag 1, parent_id=None).
#2: Rename external_id of tag with previous_id=tag_3 to 'TAG_50' (value=Tag 3, parent_id=None).

and (2) then fails at execute time with IntegrityError('UNIQUE constraint failed: oel_tagging_tag.taxonomy_id, oel_tagging_tag.external_id'). The same mismatch means a case-different swap is rejected as "already exists" instead of being staged:

{"tags": [
  {"id": "TAG_3", "value": "Tag 1", "previous_id": "tag_1"},
  {"id": "TAG_1", "value": "Tag 3", "previous_id": "tag_3"}
]}

Import plan for Import Taxonomy Test
--------------------------------
#1: Rename external_id of tag with previous_id=tag_1 to 'TAG_3' (value=Tag 1, parent_id=None).
#2: Rename external_id of tag with previous_id=tag_3 to 'TAG_1' (value=Tag 3, parent_id=None).

Output errors
--------------------------------
Action error in 'rename_external_id' (#1): A tag with external_id (TAG_3) already exists.
Action error in 'rename_external_id' (#2): A tag with external_id (TAG_1) already exists.

(Compare the same-case swap in my comment on StageTagExternalIdForSwap.__str__, which gets staged and goes through.) Please casefold() ids wherever this PR compares them in Python (the ADR 0012 code in Tag.save() does the same), and add a test for the case-different duplicate. Two plain create rows with tag_50/TAG_50 also get past the plan step today and die at execute time (with the same IntegrityError before the rebase onto main, and with the ValueError from the new Tag.save() check after it), so that part is pre-existing; I'd fix it while you're in there if it's a one-liner, but I'm not adding it to the scope otherwise.

[ormsbee]

Nit: Two things about how this reads in the error block the wizard shows. Every other error in that block refers to actions as #N, and so does the plan listing right above it, but this one says rows #1, #2. Under replace=True the delete actions come first. Claude ran this two-row file against the fixture with replace=True:

{"tags": [
  {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"},
  {"id": "tag_50", "value": "Brand New"}
]}

and the plan block reads:

Import plan for Import Taxonomy Test
--------------------------------
#1: Delete tag <TagItem> (tag_2 / Tag 2)
#2: Delete tag <TagItem> (tag_3 / Tag 3)
#3: Delete tag <TagItem> (tag_4 / Tag 4)
#4: Rename external_id of tag with previous_id=tag_1 to 'tag_50' (value=Tag 1, parent_id=None).
#5: Create a new tag with values (external_id=tag_50, value=Brand New, parent_id=None).

Output errors
--------------------------------
Duplicate id (tag_50): rows #1, #2 all claim it as their final id. Each row's id must be unique within a single import.

So rows #1, #2, read the way every other #N in that block reads, points at the two deletes rather than at (4) and (5). Saying "file rows 1 and 2" (no #) would avoid that. Also, a duplicated id on two plain create rows now produces both this error and the older Duplicated external_id tag. conflict for the same pair (see the expected output in test_generate_actions). That's fine, just flagging it in case you'd rather drop the older check.

[ormsbee]

Related to the case-sensitivity note on _validate_no_duplicate_final_ids: this lookup is case-insensitive, so a case-only rename (previous_id=tag_1, id=TAG_1) finds the tag's own row and gets rejected. Claude ran this one-row file against the fixture:

{"tags": [
  {"id": "TAG_1", "value": "Tag 1", "previous_id": "tag_1"}
]}

Import plan for Import Taxonomy Test
--------------------------------
#1: Rename external_id of tag with previous_id=tag_1 to 'TAG_1' (value=Tag 1, parent_id=None).

Output errors
--------------------------------
Action error in 'rename_external_id' (#1): A tag with external_id (TAG_1) already exists.

The database would be fine with that update, since it's the same row. Adding .exclude(external_id=self.tag.previous_id) to this query fixes it.

[ormsbee]

Per #673, the wizard shows the plan() output to the admin at the Plan step, so this is user-facing text. For the plain tag_1/tag_3 swap from your test_import_swap_external_ids:

{"tags": [
  {"id": "tag_3", "value": "Tag 1", "previous_id": "tag_1"},
  {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"}
]}

the admin sees:

Import plan for Import Taxonomy Test
--------------------------------
#1: Stage tag (pk=26) off its current external_id.
#2: Stage tag (pk=28) off its current external_id.
#3: Rename external_id of tag with previous_id=tag_1 to 'tag_3' (value=Tag 1, parent_id=None).
#4: Rename external_id of tag with previous_id=tag_3 to 'tag_1' (value=Tag 3, parent_id=None).

A taxonomy admin doesn't know what a pk is, and "stage" doesn't tell them what's about to happen to their tag. Something like:

    def __str__(self) -> str:
        return str(
            _(
                "Temporarily move tag {previous_id} off its external_id, "
                "so that another row in this file can take it."
            ).format(previous_id=self.tag.previous_id)
        )

Relatedly, the inherited __repr__ prints self.tag.id, which for this action is the id the staged tag is moving to, not the one it holds. For the same plan, the repr()s come out as:

Action stage_external_id (index=1,id=tag_3)
Action stage_external_id (index=2,id=tag_1)
Action rename_external_id (index=3,id=tag_3)
Action rename_external_id (index=4,id=tag_1)

so a log line saying id=tag_3 about a tag that is currently called tag_1 is going to confuse whoever is reading the task log in production.

[ormsbee]

#673's AC for this case says "the error states that the parent_id names an id the same file renames away, and names the new id to use instead." Right now it falls through to the generic "Unknown parent tag (tag_1). You need to add parent before the child in your file.", which is misleading here, since the parent is in the file, before the child. If _vacated_pks were a dict of pk -> new id (_build_staging_actions has both in hand), this branch could say something like "Parent tag_1 is renamed to tag_50 in this file; use tag_50 as the parent_id." That's the error someone fixing a spreadsheet of a few hundred competencies actually needs.

[ormsbee]

[Comment, no action needed] FWIW, indexed_actions has been "action name → list of actions" until now, and this is the first entry that's neither. The underscore flags it, and it works. It's just the kind of side channel where someone later adds a second one and _search_action trips over a set. If you make the dict change I asked for in _validate_parent, it may be worth passing it to validate() explicitly instead. Not asking for that in this PR.

[ormsbee]

Please bump the Status at the top of this ADR from Proposed to Accepted in this PR, since this is the PR that implements it.

[ormsbee]

Nit: #673 lists "a parent and its child are both renamed in one import" as its own scenario, and I don't see an end-to-end test for it. Claude ran this file, both plain and with replace=True:

{"tags": [
  {"id": "tag_10", "value": "Tag 1", "previous_id": "tag_1"},
  {"id": "tag_20", "value": "Tag 2", "parent_id": "tag_10", "previous_id": "tag_2"}
]}

Import plan for Import Taxonomy Test
--------------------------------
#1: Rename external_id of tag with previous_id=tag_1 to 'tag_10' (value=Tag 1, parent_id=None).
#2: Rename external_id of tag with previous_id=tag_2 to 'tag_20' (value=Tag 2, parent_id=tag_10).

(With replace=True it's the same two actions after Delete tag for tag_3 and tag_4.) Both execute cleanly and tag_20 ends up parented to tag_10, so this is just a request to pin it down with a test next to these.

[ormsbee]

The top level comment that should have been on my review:

---

Thanks for this, and for sticking with it through the earlier rounds. I came at it mostly from the angle of what happens when a file mixes previous_id rows with ordinary rows, since that's where the earlier rounds kept finding problems, and I found one more of the same shape. The core mechanism–resolving the rename target to a pk up front and staging contended ids through a placeholder inside the transaction–is sound, and I like that the end-to-end tests run the real import_tags() rather than mocking the plan.

---

Claude drafted the first pass of this review and ran the probes.

[ufedaseyeuconsultant]

@ormsbee all fixes have been applied and tested

[ormsbee]

Thanks for the quick turnaround. Nearly everything from my last round is in, and Claude re-ran the probe files from those comments against fb4180d: the plain-row/vacated-id file is rejected in both orders (and under replace=True), the tag_50/TAG_50 pair is rejected, the case-different swap now stages and goes through, and the case-only rename lands on the same pk. The _vacated_pks dict and the parent error message came out the way I was hoping.

Two leftovers from the casefold item, and one new thing:

1. The replace-mode delete sweep still looks previous_id up exact-case, so a case-different previous_id under replace=True deletes the tag and then crashes the rename (inline on generate_actions). This is the one I'd like fixed before merge.
2. Nit: a case-only rename now stages itself, which puts a "so that another row in this file can take it" line in the admin's plan when there is no other row (inline on _build_staging_actions).
3. Not from my last pass, and not blocking: a child row whose parent_id is the old id of a parent being renamed gets "No changes needed", because UpdateParentTag.applies_for compares against the pre-import database (inline). A follow-up issue is fine for that one.

After that, I'll squash and merge. Thank you.

---

Claude drafted the first pass of this review and ran the probes; each comment has the input file so you can reproduce it.

[ormsbee]

This is the one previous_id comparison from this PR that didn't get the casefold() treatment. tags_for_delete is keyed by the database's external_id, so a previous_id that differs only in case misses this pop, the tag gets a Delete action, and the rename then runs against a pk that's already gone. Claude ran this against the fixture with replace=True:

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

The plan validates clean, since the rename row resolves TAG_1 to tag_1 through the case-insensitive lookup, and the task log ends:

#1: Update the parent of <Tag> (tag_2 / Tag 2) from parent <Tag> (tag_1 / Tag 1) to None [Started]
Success
#2: Delete tag <TagItem> (tag_1 / Tag 1) [Started]
Success
#3: Rename external_id of tag with previous_id=TAG_1 to 'tag_50' (value=Tag 1, parent_id=None). [Started]
DoesNotExist('Tag matching query does not exist.')

The transaction rolls back, so nothing is lost, but it's the same raw exception in the task log as the other case-sensitivity cases, with no row identified. Maybe something like this? (just a sketch, I haven't run it):

            delete_keys_by_fold = {key.casefold(): key for key in tags_for_delete}
            for tag in tags:
                ...
                if tag.previous_id:
                    key = delete_keys_by_fold.get(tag.previous_id.casefold())
                    if key is not None:
                        tags_for_delete.pop(key)

Please add a replace=True test with a case-different previous_id next to the other replace-mode tests. The tag.id in tags_for_delete check three lines up has the same problem for a plain row (Claude ran a full-replace file with {"id": "TAG_1", "value": "Tag 1"} in it: it deletes tag_1 and then dies in the update_parent for tag_2), but that line predates this PR, so same deal as last time: fix it if it falls out of the same change, and otherwise I'm not adding it to the scope.

[ormsbee]

Nit: Now that target_ids is casefolded, a case-only rename finds its own target here and stages itself. Claude ran {"tags": [{"id": "TAG_1", "value": "Tag 1", "previous_id": "tag_1"}]} against the fixture, and the admin sees:

#1: Temporarily move tag tag_1 off its external_id, so that another row in this file can take it.
#2: Rename external_id of tag with previous_id=tag_1 to 'TAG_1' (value=Tag 1, parent_id=None).

It executes fine and ends with TAG_1 on the same pk, but there is no other row, so the first line is telling the admin something that isn't true. Skipping the staging when tag.previous_id.casefold() == tag.id.casefold() should be enough, since the only row that update could collide with is the one being updated.

[ormsbee]

[Nit, not blocking] This is new this round, not something from my last pass. This comparison is against the parent's external_id as it is before the import, so a child row whose parent_id is the old id of a parent being renamed in the same file reads as "no change": the row becomes WithoutChanges, and _validate_parent (with its new "is renamed to ... in this file" error) never runs. For a plain rename that happens to come out right, since the child stays on the same pk and the next export shows the new id. For a swap it doesn't. Claude ran this against the fixture with replace=True:

{"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_1"},
  {"id": "tag_4", "value": "Tag 4", "parent_id": "tag_3"}
]}

#1: Temporarily move tag tag_1 off its external_id, so that another row in this file can take it.
#2: Temporarily move tag tag_3 off its external_id, so that another row in this file can take it.
#3: Rename external_id of tag with previous_id=tag_1 to 'tag_3' (value=Tag 1, parent_id=None).
#4: Rename external_id of tag with previous_id=tag_3 to 'tag_1' (value=Tag 3, parent_id=None).
#5: No changes needed for <TagItem> (tag_2 / Tag 2)
#6: No changes needed for <TagItem> (tag_4 / Tag 4)

It succeeds, and afterwards tag_2 is under the tag now called tag_3 and tag_4 under the one now called tag_1, so an export disagrees with the file that was just imported. Meanwhile the same stale parent_id on a child whose parent is changing gets rejected (your test_import_rename_referencing_stale_old_id_rejected), so whether the rule applies depends on whether the row has some other change in it. The TagItem-level pass you added for the vacated-id check could catch this one too (a plain row's parent_id matching any rename row's previous_id), with the swap being the awkward case since that id is also being landed on. Given how far into the corner this is, I'd be fine with a follow-up issue if this needs to be discussed more; I mainly want to know whether you agree it's a gap.