[AShatsila]

Description

Closes #814.

An archived tag still holds its value and external_id, because the per-taxonomy unique constraints include archived rows. Before this change, creating or importing a tag with such a value failed with a plain "already exists" message that pointed at a tag the administrator cannot see, and an import could update or revive the archived tag. This PR makes create and import refuse those collisions with a message that names the cause, and pins that export and re-import leave archived tags out.

Stacked PR. This PR is stacked on #856 (#776, adds Tag.archived) and tbain#1 (#778, hides archived records from read paths and export). Until those two merge, the diff also shows their seven commits. The #814 change is only the last seven commits, 2c8b34d through 14534a3: compare view. #778 keeps its own PR and is reviewed there. Please do not merge this PR before #856 and tbain#1. Once they land, I will rebase this branch onto main so that only the #814 commits remain.

What changes

Create a tag (Taxonomy.add_tag, and the REST serializer's validate_tag_value):

• A value held by an archived tag is refused with Tag value '<value>' belongs to an archived tag in this taxonomy. Use a different value. The serializer returns the same text in its own quoting style.
• An external_id held by an archived tag is refused with Tag external_id '<id>' belongs to an archived tag in this taxonomy. Use a different external_id.
• A value or external_id held by an unarchived tag keeps its existing message byte for byte.
• Adding a tag under an archived tag, or under a tag below one, is refused with Cannot add a tag under the archived tag '<value>'.
• Values are stripped before they are compared, as Tag.clean strips them before saving, so " Archaea " gets the archived message rather than an IntegrityError.

Import (TagImportPlan.generate_actions):

• The archived subtree, meaning every archived tag and every tag under one, comes from the new TagQuerySet.in_archived_subtree(). Tree depth is capped by TAXONOMY_MAX_DEPTH, so it checks the tag and five ancestors with a fixed-size query, however many tags are archived.
• A new plan-level check looks at every row's value, id, previous_id and parent_id. A row that points at a tag in the archived subtree produces one ArchivedTagCollisionError, for example Row 2's value (Photosynthesis) belongs to an archived tag in this taxonomy. Use a different value. For previous_id, the advice is An import cannot change it. instead, because previous_id names the tag the row means to rename. When any row fails, the plan builds no actions, so nothing is imported.
• The match runs in the database, not in Python. The import actions look rows up through the column collation, so the check must use the same comparison. MySQL's utf8mb4_unicode_ci also ignores accents and trailing spaces, which Python's casefold() does not. A parity test asserts that the plan reports an ArchivedTagCollisionError exactly when the database finds a match, for Photosynthesis/PHOTOSYNTHESIS, Straße/STRASSE and Café/Cafe. The Café case only differs on MySQL.
• The check runs one query when nothing in the taxonomy is archived. Otherwise it adds one query per chunk of 200 rows, and per-field lookups only inside a chunk that matched.
• A replace import (replace=True) never deletes an archived tag or any tag under it. The export leaves that subtree out, so its absence from the file is no reason to delete it. An archived tag whose unarchived parent is deleted is moved to the root, as any kept child already is.

Export: no code change. Tests pin that JSON and CSV exports omit archived tags (#778's behavior), that an export imports cleanly into an empty taxonomy, and that a replace re-import of a taxonomy's own export keeps its archived subtrees.

Decisions beyond the ticket

The ticket's acceptance criteria cover archived tags themselves. These rules were added during implementation and review. Please push back on any of them:

1. A tag under an archived tag is treated as outside the taxonomy, just like the archived tag. The export already leaves the subtree out, so create, import and the replace sweep now agree with it.
2. A replace import keeps archived subtrees instead of deleting them.
3. A parent_id or previous_id that points at an archived tag is a plan error.
4. An external_id held by an archived tag gets its own message on create, alongside the value message the ticket asks for.
5. When any row collides with an archived tag, the plan builds no actions at all, rather than skipping only the colliding rows. This is how acceptance criterion 6 ("the taxonomy is left as it was") is met.

These rules, and the open questions below, are recorded in a proposed ADR, docs/openedx_tagging/decisions/0014-archived-tags-on-create-import-export.rst. Its decisions that belong to #778, #780 or product are marked as awaiting confirmation.

Open questions for reviewers

• Message copy. The approved thread on [Arch] Implementation approach for competency data delete/edit guardrails #655 suggested telling the administrator to "contact an operator to manually un-archive it". This PR uses "Use a different value", because no restore path exists yet. The ticket asks for copy review, so please confirm or suggest wording.
• Replace-import deletes after [BE] Branch tag and taxonomy deletes between archiving and hard delete #780. [BE] Branch tag and taxonomy deletes between archiving and hard delete #780 makes delete_tags() archive a tag instead of deleting it when something depends on it. A replace import deletes through its own DeleteTag action, which [BE] Branch tag and taxonomy deletes between archiving and hard delete #780 does not mention, so after [BE] Branch tag and taxonomy deletes between archiving and hard delete #780 a replace import could still hard-delete a depended-upon tag. Should [BE] Branch tag and taxonomy deletes between archiving and hard delete #780's scope cover import, or should this get its own issue?
• Reading of AC 8. The round-trip test reads "contains only the tags that were not archived" as "contains only what the export holds". Under that reading, an unarchived tag under an archived tag is absent from the result, because [BE] Exclude archived tagging records from read paths and block edits to them #778's export leaves it out. Please confirm.
• Tags under an archived tag (for the [BE] Exclude archived tagging records from read paths and block edits to them #778 owner). Can an unarchived tag exist under an archived one at all? If archiving always covers the whole subtree, as [BE] Branch tag and taxonomy deletes between archiving and hard delete #780's rules produce, this PR's subtree rule only guards against direct database edits. If it can exist, the listings and update_tag from [BE] Exclude archived tagging records from read paths and block edits to them #778 still treat such a tag as live while create and import do not.

Known limits, not addressed here

• When a row points at an archived tag, the plan stops before other plan-level checks, so errors such as cycles show up only after the archived rows are fixed.
• Renaming a tag to an archived value through the Python method Taxonomy.update_tag raises an IntegrityError. This predates the change and is outside [BE] Treat an archived tag as no longer part of its taxonomy on create, import, and export #814. The REST rename path is covered by the serializer.

Testing instructions

pytest tests/openedx_tagging --no-cov -q

Locally on SQLite: 709 passed. mypy, pylint, pycodestyle, isort and lint-imports are clean, makemigrations --check reports no changes, and the Sphinx docs build with the new ADR. MySQL was not run locally, so the MySQL collation cases rely on CI.

Manual check through the API. Nothing in the application archives a tag yet, so archive one in a Django shell first:

from openedx_tagging.models import Tag
Tag.objects.filter(taxonomy_id=<id>, value="Photosynthesis").update(archived=True)

Then:

1. POST /tagging/rest_api/v1/taxonomies/<id>/tags/ with {"tag": "Photosynthesis"} returns 400 with the archived-tag message.
2. Importing a JSON file that contains a tag with value Photosynthesis fails, the import log lists the archived-tag error, and the taxonomy is unchanged.
3. Exporting the taxonomy does not list Photosynthesis.

AI usage

The implementation, tests, and this description were written with Claude Code (Claude Opus 5.5), and reviewed and directed by the author. The change was reviewed by separate AI review passes before opening.

🤖 Generated with Claude Code

[openedx-webhooks]

Thanks for the pull request, @AShatsila!

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 flagged this while I was looking into why #814 was inccorectly tagged as a frontend ticket. FYI @tbain for your review:

The PR description looks like it was carried over from the #778 work and not updated for the #814 commits on top of it. Three places disagree with the code in this branch:

• "Closes [BE] Exclude archived tagging records from read paths and block edits to them #778" is the only closing reference, but the title and branch are for [BE] Treat an archived tag as no longer part of its taxonomy on create, import, and export #814, and the last four commits implement [BE] Treat an archived tag as no longer part of its taxonomy on create, import, and export #814's acceptance criteria: the archived-specific error on create, refusing import rows that match an archived tag, and the export-then-import round-trip test.
• "Related" lists [BE] Treat an archived tag as no longer part of its taxonomy on create, import, and export #814 as separate work that "covers creating or importing over an archived tag's value", but this PR does that work.
• "Known gaps" says "Importing into an active taxonomy can still rename or re-parent an archived tag in it ([BE] Treat an archived tag as no longer part of its taxonomy on create, import, and export #814)", which the commit "refuse import rows that point at an archived tag" fixes.

@AShatsila Could you update the description so it matches what the branch delivers? Either "Closes #778 and closes #814" with the #814 behavior described alongside the #778 behavior, or, if you'd rather keep the two tickets reviewable on their own, split the #814 commits into a second PR stacked on this one. I lean toward the single PR since the #814 import changes build directly on the unarchived() queryset and import plan changes here, but either works.

[tbain]

Couple of small inline comments from me, and then a few items from the AI (T3: Fable/GPT). Otherwise looks good! the AI review noted from Query efficiency concerns that I think are the highest priority. As always, though, if there's any issues with these items definitely push back as needed.

T3 Review:

---

Review of PR 927: treat an archived tag as no longer part of its taxonomy on create, import, and export

This review covers only the four commits by AShatsila (2c8b34d, c41131e, 2a627e2, 78e19ac) that implement ticket 814 (#814) on top of the unmerged work for tickets 776 (#776) and 778 (#778). The change is close to ready: all eight acceptance criteria are implemented. The items under "Must fix before merge" block merging, and the items under "Decisions to make or write down" should be settled or recorded before merge rather than left implicit in code.

Review Details

Prerequisites, deferred items, what passed, questions, and the action list

Prerequisites (skip if you know the tagging app)

Taxonomy and Tag. A Taxonomy is a named set of tags. A Tag belongs to one taxonomy, has a value (the text shown to users) and an external_id (a stable identifier used in import and export files), and may have a parent tag. The database enforces that, within one taxonomy, no two tags share a value and no two share an external_id (unique_together in Tag.Meta). On MySQL, which production runs, both columns compare case-insensitively and accent-insensitively; locally tests run on SQLite, which compares case-insensitively for ASCII only.

Lineage. Each tag stores a lineage string: the values of its ancestors and itself, from the root down, each followed by a tab character. A root tag "Bio" has lineage Bio<tab>; its child "Cells" has Bio<tab>Cells<tab>. Because of the trailing tab, "every tag whose lineage starts with X's lineage" is exactly X and its descendants, and a tag named "Biology" is not matched by "Bio". The change uses this to find the subtree under an archived tag.

Archived. Ticket 776 (unmerged, stacked under this PR) adds a boolean archived field to Tag, Taxonomy and ObjectTag. Ticket 778 (also unmerged, stacked under this PR) hides archived rows from read paths, including the export, through an unarchived() queryset method. Nothing in the application sets archived to true yet; ticket 780 will make deleting a tag that learner mastery depends on archive it instead. There is no way to un-archive a tag except by editing the database. An archived tag keeps its value and external_id, and the unique constraints have no exception for archived rows, so those values stay taken.

Import plan. Importing a file builds a TagImportPlan: the parser turns the file into TagItem rows (each with id, value, optional parent_id and optional previous_id for a rename), generate_actions validates the rows and builds create, rename, update-parent and delete actions, and execute runs them inside one transaction. With replace=True the plan also deletes every existing tag the file does not mention. Errors found while planning are collected in plan.errors and written to the import task's log; if any exist, nothing is executed.

What this change adds. On create (Taxonomy.add_tag and the REST serializer's validate_tag_value), a value or external_id held by an archived tag is refused with a message saying so, and a parent that is archived or that sits under an archived tag is refused. On import, a new validator _validate_no_row_targets_an_archived_tag rejects every row whose value, id, previous_id or parent_id matches an archived tag or a tag under one, using database queries so the comparison follows the column collation, and when any row is rejected the plan builds no actions. The replace sweep no longer deletes archived tags or tags under them, and an archived child whose parent is deleted is moved to the root. The helper _out_of_taxonomy_tags returns the archived tags and their descendants by ORing one lineage-prefix condition per archived tag.

Sibling tickets. 655 (#655) is the closed design thread where archiving was agreed. 778 (#778) owns hiding archived rows on read paths and left one question open: whether an unarchived tag under an archived parent should be hidden too. 780 (#780) owns turning tag and taxonomy deletes into archive-or-delete. 779 (#779) does the same for tag associations.

Things the review looked at and is leaving to other tickets

• A replace import hard-deleting a deletion-locked tag after 780 lands: 780, or a follow-up, must route the import's deletes through the archive-or-delete decision. It can do so without undoing anything here, because the sweep's exclusion set only grows.
• Plan-level errors such as cycles being hidden until archived-row errors are fixed: the PR description states it, and stopping before building actions is what criterion 6 asks for.
• Taxonomy.update_tag raising IntegrityError when renaming to an archived value: pre-existing, outside 814, stated in the PR description. The REST rename path is covered by the serializer.
• The REST response for an archived external_id being a bare list while the value error is keyed by field: identical to the pre-existing duplicate-external_id shape, and the frontend reads the first string of an array body.

What passed

The branch merges cleanly into openedx/main; on the merged tree makemigrations --check reports no changes and the import linter keeps all three contracts. At the head commit the tagging suite passes (705 tests, 49 subtests), and pylint, mypy, pycodestyle, pydocstyle and isort are clean. All eight acceptance criteria are implemented. For each new behavior, a reviewer agent broke the code in a temporary worktree (for example, removed the validator call or reverted the replace sweep) and ran the tagging suite. A test that keeps passing against broken code is reported as not covering that behavior. Every criterion has a test that failed when its behavior was broken, except criteria 2 and 3, whose tests assert exact messages and creation and were not broken on purpose. Of fifteen factual claims in the PR description, commit messages and new comments, thirteen were confirmed, one is wrong (the query-count docstring) and one holds only for SQLite 3.32 or later (the chunk-size comment). All of this ran on SQLite; MySQL was not available, so statements about it are inferred from documented behavior. An independent adversarial pass by a different model family (GPT-6.1-Sol) found the SQLite depth failure, the per-row query cost, the whitespace bypass and the plan-to-execute window, all included below.

Questions for you

1. Which reading of criterion 8 do you want: "only what the export holds", which omits the unarchived child of an archived tag, or "every unarchived tag"? This is the same decision as the first item under "Decisions to make or write down".
2. Will you take the bounded parent-chain predicate for the SQLite failure, and if so as a shared TagQuerySet method that add_tag uses too?
3. Has 778's owner answered the descendant question since 2026-10-22?

Action list

1. Replace the per-archived-tag OR chain in _out_of_taxonomy_tags with a fixed parent-chain predicate, and add a test with more than 1,000 archived tags.
2. Add a test that exercises the chunked pre-filter with the collision in the last chunk.
3. Make the collation parity test assert an ArchivedTagCollisionError, and fix the PR description's sentence about it.
4. Correct the query-count docstring on the validator.
5. Get the 778/655 owners' decision on tags under an archived tag, and write a proposed ADR on archiving.
6. Ask 780's owner about re-rooting and about the import's deletes; link the answer or a follow-up issue from the PR description.
7. Confirm the message wording with product.
8. Re-check archived tags inside execute, or qualify the "never deleted this way" docstring.
9. Restrict the per-field error lookups to chunks that matched.
10. Strip whitespace before the archived lookups in add_tag and the validator.
11. Document the new ValueError cases on add_tag_to_taxonomy.
12. Reword the previous_id message; consider one error per row.
13. Parameterize the duplicated tests and drop the overlapping replace test.
14. Fix the first five small things listed below (the PR description item is covered by item 15).
15. Update the PR description: the two undeclared decisions, how to archive a tag for manual testing, and the title form.

Must fix before merge

Importing into a taxonomy that has accumulated enough archived tags fails on SQLite.

Details

The archived-subtree query fails on SQLite at about 1,000 archived tags. _out_of_taxonomy_tags in import_plan.py ORs one lineage__startswith condition per archived tag into a single query. SQLite limits expression-tree depth to 1,000, so once a taxonomy has about 1,000 archived tags the first exists() call of every import raises OperationalError: Expression tree is too large. The replace sweep embeds the same chain in a NOT IN subquery and fails from about 500. import_tags catches the error, returns false and marks the task failed, so even an import of one harmless new tag is refused. Three independent reviewer passes reproduced this with 600 to 20,000 archived tags. MySQL has no such depth limit (inferred, not run), but its query text grows by roughly 100 bytes per archived tag and is sent two or three times per import. The repository requires the code to work on SQLite, MySQL and PostgreSQL, and ticket 780 will make every delete of a depended-upon tag an archive, so archived tags accumulate for the life of a taxonomy.

Fix: tree depth is capped at TAXONOMY_MAX_DEPTH (5), so a fixed six-term predicate, Q(archived=True) | Q(parent__archived=True) | ... | Q(parent__parent__parent__parent__parent__archived=True), selects the same rows at constant query size. It follows the parent__parent__parent_id pattern in models/base.py. Put it on TagQuerySet next to unarchived() so the ancestor check in add_tag can share it, and add a test that creates more than 1,000 archived tags and imports one row.

A new part of the import check has no test that would notice if it were removed.

Details

The chunked pre-filter has no test. The validator splits the file's rows into chunks of 500 and runs one existence query per chunk. Making it validate only the first chunk, or setting the chunk size to 1, fails no test. A probe with 1,200 rows and a collision at row 1,101 was caught, so the code works, but nothing will notice if it stops. Add a test that patches IN_CHUNK_SIZE to a small number and places the colliding row in the last chunk.

A new test claims to check the import's archived-tag check but would still pass without it.

Details

The collation parity test cannot tell whether the validator exists. test_generate_actions_archived_match_follows_db_collation in test_import_plan.py asserts that the plan has errors exactly when a database lookup finds a collision. With the validator deleted, the create action reports its own "duplicated value" error, so the plan still has errors and the test still passes. On SQLite the Café/Cafe arm has both sides false, so it asserts nothing there. The PR description's sentence "A parity test asserts that the plan's verdict equals the database's" promises more than the test delivers. Assert that an ArchivedTagCollisionError is present exactly when the lookup finds a collision, and note in a comment that the accent case only bites on MySQL.

A new docstring misstates how expensive the new import check is.

Details

A new docstring states a wrong query count. The docstring of _validate_no_row_targets_an_archived_tag says the no-match case "costs one query per chunk of rows". Measured with Django's query capture, it costs one query when the taxonomy has no archived tags, and three when it has one and the file fits in one chunk (the lineage fetch, the exists() on the archived set, and the chunk's exists()). Correct it to "one query when nothing is archived, otherwise two plus one per chunk".

Decisions to make or write down

It is undecided whether a tag under an archived tag counts as outside its taxonomy everywhere, and the decision is not recorded.

Details

Whether a tag under an archived tag is out of the taxonomy everywhere, and where that is recorded. This change treats such a tag as out of the taxonomy on three surfaces: add_tag refuses it as a parent (the ancestor check in models/base.py), the import validator refuses any row naming it, and the replace sweep keeps it. The export from ticket 778 already omits it. But 778's listings and update_tag still treat it as live, and the author of 778's implementation asked on 2026-10-22 whether to hide it, with no answer yet. The effect: an administrator sees "Child Of Archived" in the editor and can rename it, but adding a sub-tag under it fails with "Cannot add a tag under the archived tag 'Archived Parent'", which names a tag they cannot see, and any import row naming the child is refused. The same decision settles the PR's own question about acceptance criterion 8, because the round-trip result omits that child.

No ADR describes archiving, and the repository's instructions ask for one on significant API changes. Get the 778 and 655 owners' answer before merge; the review's architecture pass recommends "out of the taxonomy everywhere", since the export already behaves that way. Then write a short proposed ADR under docs/openedx_tagging/decisions/ stating what archived means, the descendant rule, that values and external_ids stay reserved, that there is no restore path, and how import and export behave.

When a replace import deletes a parent tag, its archived child is moved to the top level, and whether that fits the planned deletion rules is unsettled.

Details

Re-rooting an archived child during a replace import. When the sweep deletes an unarchived parent, _build_delete_actions moves its archived child to the root, rewriting the child's parent, depth and lineage. Ticket 780's criterion 6 says a parent with a depended-upon subtag is archived rather than deleted, and 778's criterion 9 says an archived record is unchanged by an edit. Re-rooting is the safer interim choice, because the alternative is the cascade on Tag.parent deleting the archived child and the learner history behind it. Ask 780's owner. If this merges as is, say in the import_tags docstring that re-rooting is interim until the import's deletes go through 780's archive-or-delete decision.

After the planned deletion work lands, a replace import can still permanently delete a tag that learner mastery depends on.

Details

After 780, a replace import can still hard-delete a tag that learner mastery depends on. The sweep excludes only archived subtrees, and the DeleteTag action calls Tag.delete() directly; the association's tag foreign key is set to null, so a locked association loses its tag. Ticket 780 covers delete_tags() and taxonomy delete only, not the import. The PR description already raises this. Decide whether 780's scope grows to cover the import's deletes or a follow-up issue is filed, and link it from the PR description.

The wording of the new error message for archived tags has not been confirmed with product.

Details

Message copy. The ticket's criteria say the error should tell the administrator that "a different value should be used", and the code does. The design thread on 655 had spoken of telling them to contact an operator to un-archive the tag. The ticket is the specification, so no code change is proposed; product should confirm the wording, as the PR already asks.

An archive made after an import is planned but before it runs is not noticed.

Details

An archive committed between planning and execution is not seen by the plan. import_tags builds the plan outside the transaction that wraps execute. A reviewer reproduced on SQLite that archiving a tag right after generate_actions lets a planned rename update the archived tag and a planned replace delete remove it. The window is a single request on a path where nothing archives yet, and every plan-time check in the import has the same shape, so this is a pre-existing class of race. But the new import_tags docstring sentence "Archived tags and the tags under them are never deleted this way" has no such qualification. Either re-run the archived check inside execute's transaction or soften the docstring to "a tag archived when the plan is built".

Improvements worth doing

Reporting an archived-tag collision makes far more database queries than it needs.

Details

The error path runs up to four queries per row for every row in the file. Once any chunk's existence query matches, the reporting loop runs a .first() query per populated field per row, each carrying the full archived-lineage predicate. Measured: 1,507 queries for a 501-row file with one collision, 2,007 with all four fields populated. A 10,000-row file would cost about 30,000 sequential queries inside the synchronous import request. Keep the per-chunk results and run the per-field lookups only inside chunks that matched, which caps the extra work at about 2,000 queries per colliding chunk.

Values with stray leading or trailing spaces skip the new archived-tag check and fail with a less helpful database error.

Details

Leading or trailing whitespace bypasses the archived explanation. The parsers keep whitespace, and Tag.clean strips value and external_id when saving. A row with value " Archived " misses the validator, becomes a rename action, and fails at execution with a uniqueness IntegrityError in the log instead of the archived message; taxonomy.add_tag(" Archived ") raises IntegrityError instead of the explanatory ValueError. The pre-existing duplicate check had the same hole, and the REST path is unaffected because Django REST Framework trims string input, so this is hardening: strip the compared values the way Tag.clean does before the lookups in add_tag and the validator.

The public function for adding a tag does not document the new ways it can refuse.

Details

The stable API docstring does not mention the new refusals. add_tag_to_taxonomy in openedx_tagging/api.py is public API without an UNSTABLE marker and is re-exported by openedx-platform. Only the model method's docstring was updated. Add one RST sentence listing the ValueError cases: duplicate or archived value, duplicate or archived external_id, parent archived or under an archived tag.

One new error message gives advice that cannot be followed, and a row that matches on two fields produces two errors.

Details

Two message problems. For a previous_id collision the message ends "Use a different previous_id", which cannot be followed, since previous_id names the one tag the row means to rename; say instead that the tag is archived and an import cannot change it. And a single row that matches on two fields produces two errors, which the PR description lists as a known limit; one error per row would read better.

Several new tests repeat each other, and one tests something other than this change.

Details

Test duplication. Several new tests are one behavior with two inputs and should be one parameterized test using ddt, which the modules already use: the archived-value and different-case pair in test_import_plan.py, the id, previous_id and parent_id triple there (the under-archived variant in the same file already uses @ddt.data with a field argument), and the archived-value pair in test_api.py, where the second test asserts only a substring. test_add_tag_generated_external_id_skips_archived_id asserts only that the generated id is not "respiration" and exercises id generation from the stacked base, not this change. The API-level and plan-level replace tests pin the same sweep; keep one and assert in it that the unarchived child tag_4 survives.

Small things

• A code comment is now inaccurate.

  Details

  A comment in import_plan.py, in the loop that keeps existing tags the file never mentions, says this "only happens with replace=False". After this change, archived subtrees also stay with replace=True.

• A validator's return value reads backwards at its call site.

  Details

  _validate_no_row_targets_an_archived_tag returns true when it rejected a row, so the call site if self._validate_...(tags): return reads as "if valid, stop", and the sibling validators return nothing. Rename it to _reject_rows_targeting_archived_tags, or return nothing and check self.errors.

• The same set of tags is called by four different names.

  Details

  The names are "out of the taxonomy" (and _out_of_taxonomy_tags returns tags that are in taxonomy.tag_set), "no longer part of the taxonomy", "an archived tag or under one", and "archived subtree". Pick one; "archived subtree" names the mechanism.

• An existing docstring paragraph has a typo.

  Details

  The import_tags docstring paragraph the change extends still says "delete all not readed Tag".

• A comment explaining a size limit is imprecise.

  Details

  The comment on IN_CHUNK_SIZE says it keeps each __in list under SQLite's parameter limit, but one chunk query carries up to 2,002 bound parameters (500 values, 1,500 ids, and two more). That is fine on SQLite 3.32 or later, which allows 32,766, but above the 999 that Django's SQLite backend declares. Name the limit you mean, or chunk by total parameter count.

• The PR description omits some decisions and testing information, and its title is not in conventional-commit form.

  Details

  The description lists three decisions beyond the ticket but not two others the code makes: the separate message for an archived external_id on create, and building no actions at all when any row collides (the latter appears only under "Known limits"). The manual testing steps never say how to put a tag into the archived state, which today needs a shell or admin step. The title fix/814: ... is not conventional-commit form; the commits are, but the squash-merge title decides the release type.

[tbain]

Would it be possible to break this out into a well named, defined function as well, similar to out_of_taxonomy currently does?

[tbain]

Could re-rename this to a more meaningful name? Something like _find_tags_outside_<something> or _find_unassigned_archived_tags()? The name is vague to the point where I'm not, at a glance, able to understand what this does exactly.

[tbain]

Similar to above, would it be possible to break this out into a well named discreet function that makes the overall set of steps in this _validate_no_row_targets_an_archived_tag clearer?

[tbain]

Approved! This looked good, I appreciate your clarifying the import_plan changes, and AI (T3, Fable) asserts that all of its findings have been addressed.

The description for this PR needs to be updated, however. That's the only lingering issue.

---

AI re-review report:

Re-review of PR 927 after the author's fixes

PR: #927. Ticket: #814. The previous review, on 2026-11-05, covered the commits after 2bd9204 up to and including 78e19ac. This re-review covers the two commits pushed since, 68f3f74 and 08a9964, at head 08a9964.

Verdict. The two new commits resolve every blocking item and every inline comment from the previous round and introduce no regression. What remains is outside the code: the PR description was not updated, and the three decisions that belong to the owners of tickets 778 and 780 and to product are recorded in the new proposed ADR as awaiting confirmation but have not been answered. The change is ready to merge once the description is updated. The ADR can stay in Proposed status through merge, until the owners of 778 and 780 and product confirm their parts.

Prerequisites (skip if you read the previous review)

• Archived subtree. Every archived tag together with every tag under one. The new TagQuerySet.in_archived_subtree() returns it by checking the tag and each of its ancestors, up to the tree depth cap of six levels, so the query has the same size whatever the number of archived tags. Source: the method's docstring and TAXONOMY_MAX_DEPTH in src/openedx_tagging/models/base.py.
• Replace sweep. With replace=True, the import plan deletes every existing tag the file does not mention. The sweep leaves the archived subtree alone. Source: TagImportPlan.generate_actions.
• Re-rooting. When the sweep deletes an unarchived parent, its archived child is moved to the root instead of being cascade-deleted. Source: the import_tags docstring in src/openedx_tagging/import_export/api.py.
• Chunked pre-filter. The import validator splits the file's rows into chunks and runs one existence query per chunk against the archived subtree. Only a chunk that matched gets per-row lookups. Source: _reject_rows_targeting_archived_subtree in import_plan.py.
• Parity test. test_generate_actions_archived_match_follows_db_collation asserts that the plan refuses a row exactly when a database lookup finds a collision, for value pairs that collide under some collations and not others.

Were the review comments fixed?

The three inline comments by tbain

Comment	Status	Where it landed
Break the chunked existence check out into a named function	Fixed	_chunk_matches_archived_subtree in import_plan.py
Rename _out_of_taxonomy_tags to something meaningful	Fixed	It became TagQuerySet.in_archived_subtree() in models/base.py. The validator is now _reject_rows_targeting_archived_subtree.
Break the per-row error reporting out into a named function	Fixed	_report_archived_subtree_matches, with _row_keys holding the field list both helpers share

The fifteen action items from the AI review

#	Item	Status	Notes
1	Replace the per-archived-tag OR chain with a fixed parent-chain predicate; test with more than 1,000 archived tags	Fixed	in_archived_subtree() compiles to five LEFT OUTER self-joins with one bound parameter. test_generate_actions_with_many_archived_tags creates 1,100 archived tags and runs the plan with and without replace.
2	Test the chunked pre-filter with the collision in the last chunk	Fixed	test_generate_actions_archived_collision_in_last_chunk patches IN_CHUNK_SIZE to 2. It fails when the validator looks only at the first chunk.
3	Make the parity test require an ArchivedTagCollisionError; fix the PR description's sentence about it	Code fixed, description not	The test filters for that error class and gained a Photosynthesis/PHOTOSYNTHESIS arm that collides on every database, so it asserts something on SQLite too. It fails when the validator is removed. The PR description still describes the test with the old wording.
4	Correct the query-count docstring	Fixed	The new docstring matches the code: one query when nothing is archived, one more per chunk, plus up to four lookups per row in a matched chunk.
5	Get the 778 and 655 owners' decision on tags under an archived tag; write a proposed ADR	ADR written, decision open	docs/openedx_tagging/decisions/0014-archived-tags-on-create-import-export.rst records the subtree rule and marks the descendant question as awaiting the 778 owner. Tickets 778, 780 and 655 have no comment since 2026-10-22.
6	Ask 780's owner about re-rooting and about the import's deletes; link the answer or a follow-up issue	Recorded, not answered	Both sit under the ADR's "Known limitations" as awaiting the 780 owner. The import_tags docstring calls re-rooting interim. No follow-up issue is linked.
7	Confirm the message wording with product	Recorded, not answered	The ADR's "Future considerations" marks it as awaiting product.
8	Re-check archived tags inside execute, or qualify the "never deleted this way" docstring	Fixed by qualifying	The docstring and the ADR now say a tag in an archived subtree "when the plan is built" is not deleted. That was the accepted alternative.
9	Run the per-field lookups only in chunks that matched	Fixed	A clean chunk costs one query.
10	Strip whitespace before the archived lookups	Fixed	_row_keys strips every compared value, and add_tag strips tag_value and external_id first. The " Tag 3 " and " Archaea " test arms pin it.
11	Document the new ValueError cases on add_tag_to_taxonomy	Fixed	One RST sentence in src/openedx_tagging/api.py.
12	Reword the previous_id message; consider one error per row	Fixed	The message ends "An import cannot change it." for previous_id. Reporting stops at a row's first matching field, pinned by test_generate_actions_one_archived_error_per_row.
13	Parameterize the duplicated tests; drop the overlapping replace test	Fixed	Five single-row tests became one ddt test with five arms, and the two add_tag value tests became one with three arms. The API-level replace test was dropped for the plan-level one, which states in a comment that tag_4 under the archived tag_3 survives. test_add_tag_generated_external_id_skips_archived_id was dropped as suggested. Generated-id collisions are still covered by test_save_resolves_generated_external_id_collision in test_models.py, though no test pins the archived case specifically.
14	Fix the small things: stale comment, inverted return value, four names for one concept, "readed" typo, chunk-size comment	Fixed	The loop comment now says archived subtrees also stay with replace=True. The validator is named for what it does. Every docstring and message says "archived subtree". The typo is gone. IN_CHUNK_SIZE dropped to 200, with a comment that four parameters per row keep a chunk under the 999 that Django's SQLite backend assumes; 200 rows give at most 800.
15	Update the PR description: the two undeclared decisions, how to archive a tag for manual testing, conventional-commit title	Not done	See below.

The PR description is still the one from the previous round

The description has not changed since 2026-11-05. The two new commits made these statements wrong or incomplete:

• It says the 814 change is "the last four commits, 2c8b34d through 78e19ac" and links a compare view ending at 78e19ac. There are now six commits, ending at 08a9964.
• "Known limits" says a row matching on both value and id gets two errors. The code now reports one error per row.
• The "ADR" open question says an ADR "would be a separate docs PR and does not block this one". The ADR is in this PR.
• The sentence "A parity test asserts that the plan's verdict equals the database's" describes the old test. The test now asserts the archived-tag error specifically.
• "Decisions beyond the ticket" still omits two decisions the code makes: the separate message for an archived external_id on create, and building no actions at all when any row collides.
• The manual testing steps do not say how to put a tag into the archived state, which today needs a shell or admin step.
• The title is still fix/814: ..., which is not conventional-commit form. The commitlint check passes because it checks commit messages, but the squash-merge title decides the release type.

The comment by mgwozdz-unicon on 2026-11-04 asked that the description close 814 rather than 778. That is done: it reads "Closes #814".

Are the new changes architecturally sound?

Yes. The rule for which tags are outside the taxonomy moved from the import module into the model layer, as a TagQuerySet method next to unarchived(), and the import validator and the replace sweep both call it. Its query size is bounded by the depth cap instead of by archive history. The validator is now three small functions with one job each. The ADR is in the right place, has the right status, and names the owner of each open decision instead of settling it silently.

Four small things remain, none blocking:

1. add_tag still has its own copy of the subtree rule. Its ancestor check in models/base.py splits the parent's lineage and looks the values up, rather than asking in_archived_subtree() whether the parent is in it. The two implementations agree today. The previous review suggested sharing the queryset method so there is one place to change. Suggestion.
2. The ADR contradicts itself on restoring a tag. Its Context says "Nothing in the application can restore an archived tag; only a direct database edit can", while its "Known limitations" says an operator can change archived on the Django admin edit page. The admin is part of the application. Say "no API or REST endpoint can restore it". Nit.
3. The ADR's links to other ADRs render as broken links. It links ADR 0003 and ADR 0011 as `name <path.rst>`_, which Sphinx renders as an external link to a .rst file that does not exist in the HTML output. ADRs 0012 and 0013 use the same pattern, so this is a copied convention, but ADR 0011 uses :ref: and the targets exist: openedx-learning-adr-0003, and the openedx-tagging-adr-0014 label this ADR defines. Nit.
4. A docstring describes a future. The import_tags docstring says re-rooting happens "until the import's deletes follow the archive-or-delete rule of #780". A docstring should state current behavior. The ADR already carries the "until". Nit.

Did the changes break anything that worked before?

No. Checks run in a temporary worktree at 08a9964, against the worktree's source:

• The tagging suite passes with 709 tests and 49 subtests. The previous head had 705 tests; the new tests account for the difference.
• pylint is clean on every changed file. mypy reports the same two pre-existing errors in openedx_content as before and none in openedx_tagging. pydocstyle output differs from the previous head only by line numbers. lint-imports keeps all three contracts.
• Sphinx builds the new ADR without a warning, and the decisions index picks it up through its glob.
• Mutation checks: making the validator look only at the first chunk fails exactly the last-chunk test. Removing the validator call fails twelve tests, including the parity test, which passed against broken code last time.
• CI on the PR head is green on every job, including the test job, which runs on MySQL. The previous review could not run MySQL, so the collation arms and the self-join query are now exercised on both databases.

The behaviors pinned in the previous round still hold: the replace sweep keeps archived subtrees, an archived child of a deleted parent is re-rooted, export omits archived tags, and the round trip into an empty taxonomy succeeds.

Are the acceptance criteria met?

All eight criteria of ticket 814 were met at the previous head and remain met. The two new commits change how the import check is computed, not what it accepts or refuses.

AC	Behavior	Evidence at 08a9964
1	Adding a value held by an archived tag is refused with an archived-tag message	add_tag in models/base.py; test_add_tag_with_archived_value_is_refused, three arms
2	A value held by an unarchived tag still reports a duplicate	The "already exists" message is unchanged and its existing tests pass
3	A free value is created	The test in tests/openedx_tagging/test_api.py that adds "Respiration" and asserts the created tag's value passes
4	Importing an archived value is refused with a message, and the archived tag is not touched	_reject_rows_targeting_archived_subtree; the first arm of test_generate_actions_archived_tag_rejected; the API-level test asserts the taxonomy snapshot is unchanged
5	Importing an archived external id is refused with a message	The id, previous_id and parent_id arms of the same test
6	A colliding import changes nothing else	The plan returns before building actions; test_generate_actions_reports_every_archived_collision asserts no actions; the API test asserts an equal snapshot
7	An archived tag is absent from an export	Unchanged from ticket 778; the export tests pass
8	Export then import into an empty taxonomy succeeds and holds only unarchived tags	The round-trip tests in import_export/test_api.py pass. The ADR states the reading used: the result holds what the export holds, so an unarchived tag under an archived tag is absent. That reading waits on the 778 owner.

What to do before merging

1. Update the PR description: the commit range and compare link, the "two errors per row" limit, the parity-test sentence, the two undeclared decisions, the ADR's location, how to archive a tag for the manual check, and a conventional-commit title such as fix: treat an archived tag as outside its taxonomy on create, import, and export.
2. Get the 778 owner's answer on tags under an archived tag and the 780 owner's answer on re-rooting and on the import's deletes, or file a follow-up issue and link it from the ADR and the description.
3. Optionally, the four small items above.