From 2e8701a08d6fb6a6c9245b8826a58c6e661985c5 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Mon, 12 Oct 2026 20:31:35 +0300 Subject: [PATCH 1/5] fix: make tag import parent_id handling correct and order-independent A row whose parent_id equals its current parent's pre-import external_id is a "no change" row for UpdateParentTag, so _validate_parent never ran for it. An existing child naming a parent that the same file renames away was accepted silently. Plan-level checks now run before any action is built: - reject a parent_id that names a parent renamed away in the same file, and name the parent's new id, while still accepting a child that names the end-state id of a swapped parent; - reject parent cycles and self-parents through existing tags; - reject an import whose result would be deeper than the taxonomy depth limit, instead of failing at execute time or storing too-deep tags. Parent references no longer depend on row order. Rows are ordered by parent dependency before actions are built, kept children of deleted tags are promoted before any delete cascades, and a dry run of the parent effects rebuilds a plan detach-first when an intermediate step would create a cycle or exceed the depth limit. Ordinary plans are unchanged. Plan-level validations share one query for the existing tags instead of querying per row. Co-Authored-By: Claude Sonnet 5.5 --- src/openedx_tagging/import_export/actions.py | 20 +- .../import_export/exceptions.py | 44 + .../import_export/import_plan.py | 464 ++++++- .../import_export/test_actions.py | 2 +- .../openedx_tagging/import_export/test_api.py | 1135 ++++++++++++++++- .../import_export/test_import_plan.py | 759 ++++++++++- 6 files changed, 2408 insertions(+), 16 deletions(-) diff --git a/src/openedx_tagging/import_export/actions.py b/src/openedx_tagging/import_export/actions.py index 96aced3f1..980e0b7c0 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -146,7 +146,7 @@ def _validate_parent(self, indexed_actions) -> ImportActionError | None: action=self, message=_( "Unknown parent tag ({parent_id}). " - "You need to add parent before the child in your file." + "Add a row for the parent to your file." ).format(parent_id=self.tag.parent_id), ) return None @@ -343,12 +343,30 @@ def applies_for(cls, taxonomy: Taxonomy, tag: TagItem, indexed_actions=None) -> """ This action applies whenever there is a change on the parent. + A row whose parent_id equals the current parent's external_id is "no + change" and skips `_validate_parent`. TagImportPlan validates the + invalid cases of such rows itself: a parent renamed away, and parent + cycles. The exception is a parent that this import deletes while a + rename reclaims its id: the row applies, to move the child under the + renamed tag. + See ImportAction._resolve_update_target for when this doesn't apply regardless of the parent change (a rename_external_id row). """ taxonomy_tag = cls._resolve_update_target(taxonomy, tag) if taxonomy_tag is None: return False + parent = taxonomy_tag.parent + # The parent is deleted by this import and a rename reclaims its id, so + # the child must move under the renamed tag instead of being promoted. + if ( + parent is not None + and parent.external_id == tag.parent_id + and indexed_actions + and any(a.tag.id.casefold() == parent.external_id.casefold() for a in indexed_actions["delete"]) + and any(a.tag.id.casefold() == parent.external_id.casefold() for a in indexed_actions["rename_external_id"]) + ): + return True return ( taxonomy_tag.parent is not None and taxonomy_tag.parent.external_id != tag.parent_id diff --git a/src/openedx_tagging/import_export/exceptions.py b/src/openedx_tagging/import_export/exceptions.py index 1f1caf1f6..fddbbcd12 100644 --- a/src/openedx_tagging/import_export/exceptions.py +++ b/src/openedx_tagging/import_export/exceptions.py @@ -162,3 +162,47 @@ class InvalidCSVField(TagParserError): def __init__(self, tag: dict | None, field: str, row: int, **kargs): super().__init__(tag, **kargs) self.message = _("Invalid '{field}' field on the row {row}").format(field=field, row=row) + + +class ParentIdRenamedAwayError(TagImportError): + """ + Exception raised when a row's parent_id is its current parent's pre-import + id, and the same import renames that parent to a different id. + + The id is stale or, after a swap, ambiguous (it may also name the tag that + takes it over), so the importer asks for the parent's new id instead. + """ + + def __init__(self, row: int, parent_id: str, new_id: str, **kargs): + super().__init__(**kargs) + self.message = _( + "Row {row}'s parent_id ({parent_id}) names an id this file renames away. " + "Use '{new_id}' as the parent_id." + ).format(row=row, parent_id=parent_id, new_id=new_id) + + +class ParentCycleError(TagImportError): + """ + Exception raised when the parent_id of a row makes the tag its own + ancestor once the import is applied. + """ + + def __init__(self, row: int, parent_id: str, **kargs): + super().__init__(**kargs) + self.message = _( + "Row {row}'s parent_id ({parent_id}) creates a parent cycle." + ).format(row=row, parent_id=parent_id) + + +class InvalidParentHierarchyError(TagImportError): + """ + Exception raised when applying the import, even with its parent moves ordered + safely, would build a tag tree that is too deep or otherwise invalid. + """ + + def __init__(self, max_levels: int, **kargs): + super().__init__(**kargs) + self.message = _( + "This import would leave tags deeper than {max_levels} levels, or build an invalid " + "parent hierarchy, so it cannot be applied." + ).format(max_levels=max_levels) diff --git a/src/openedx_tagging/import_export/import_plan.py b/src/openedx_tagging/import_export/import_plan.py index b39cbddda..0d035e2cc 100644 --- a/src/openedx_tagging/import_export/import_plan.py +++ b/src/openedx_tagging/import_export/import_plan.py @@ -3,10 +3,13 @@ """ from __future__ import annotations +from typing import NamedTuple + from attrs import define from django.db import transaction from ..models import Tag, TagImportTask, Taxonomy +from ..models.base import TAXONOMY_MAX_DEPTH from .actions import ( DeleteTag, ImportAction, @@ -16,7 +19,28 @@ WithoutChanges, available_actions, ) -from .exceptions import DuplicateFinalIdError, StaleIdTargetedByPlainRowError, TagImportError +from .exceptions import ( + DuplicateFinalIdError, + InvalidParentHierarchyError, + ParentCycleError, + ParentIdRenamedAwayError, + StaleIdTargetedByPlainRowError, + TagImportError, +) + + +class ExistingTag(NamedTuple): + """ + The fields of an existing tag that the plan-level validations need. + """ + + pk: int + parent_pk: int | None + parent_external_id: str | None + + +# Existing tags by casefolded external_id. +ExistingTags = dict[str, ExistingTag] @define @@ -40,6 +64,123 @@ def __str__(self): return f"<{self.__class__.__name__}> ({self.value})" +class _ParentForest: + """ + In-memory copy of a taxonomy's parent links, used to dry-run the parent effects of import actions. + + Tags are identified by pk, and new tags get negative ids. Tags are looked up by their casefolded + external_id, which follows renames. + """ + + def __init__(self, existing: ExistingTags): + self.node_of: dict[str, int] = {} + self.key_of: dict[int, str] = {} + self.parent: dict[int, int | None] = {} + self.children: dict[int, set[int]] = {} + self.new_nodes = 0 + for key, info in existing.items(): + self.node_of[key] = info.pk + self.key_of[info.pk] = key + self.parent[info.pk] = info.parent_pk + if info.parent_pk is not None: + self.children.setdefault(info.parent_pk, set()).add(info.pk) + + def _depth(self, node: int) -> int: + """ + Return the number of ancestors of ``node``. + """ + result, ancestor = 0, self.parent.get(node) + while ancestor is not None and result <= len(self.parent) + 1: + result += 1 + ancestor = self.parent.get(ancestor) + return result + + def _height(self, node: int) -> int: + """ + Return the number of levels below ``node``, stopping once it exceeds the maximum depth. + """ + result, level = 0, [node] + while level and result <= TAXONOMY_MAX_DEPTH: + level = [child for member in level for child in self.children.get(member, ())] + result += 1 if level else 0 + return result + + def _attach(self, node: int, new_parent: int | None) -> bool: + """ + Move ``node`` under ``new_parent``; return False if that makes a cycle or a tree too deep. + """ + if new_parent is not None: + ancestor: int | None = new_parent + for _step in range(len(self.parent) + 2): + if ancestor is None: + break + if ancestor == node: + return False + ancestor = self.parent.get(ancestor) + if self._depth(new_parent) + 1 + self._height(node) > TAXONOMY_MAX_DEPTH: + return False + old_parent = self.parent.get(node) + if old_parent is not None: + self.children.get(old_parent, set()).discard(node) + self.parent[node] = new_parent + if new_parent is not None: + self.children.setdefault(new_parent, set()).add(node) + return True + + def _rekey(self, node: int, key: str) -> None: + """ + Make ``node``, new or existing, answer to the external_id ``key``. + """ + self.node_of.pop(self.key_of.get(node, ""), None) + self.key_of[node] = key + self.node_of[key] = node + + def _delete(self, key: str) -> None: + """ + Remove the tag with external_id ``key`` and all its descendants, if it exists. + """ + doomed = self.node_of.get(key) + pending = [doomed] if doomed is not None else [] + while pending: + node = pending.pop() + pending.extend(self.children.pop(node, ())) + old_parent = self.parent.pop(node, None) + if old_parent is not None: + self.children.get(old_parent, set()).discard(node) + self.node_of.pop(self.key_of.pop(node, ""), None) + + def apply(self, action: ImportAction) -> bool: + """ + Apply the parent effects of ``action``; return False if it would fail at execution. + """ + tag = action.tag + if action.name == "stage_external_id": + if action.target_pk not in self.parent: + return False + self._rekey(action.target_pk, f"staged:{action.target_pk}") + elif action.name == "delete": + self._delete(tag.id.casefold()) + elif action.name in ("update_parent", "create", "rename_external_id"): + new_parent = self.node_of.get(tag.parent_id.casefold()) if tag.parent_id else None + if tag.parent_id and new_parent is None: + return False + if action.name == "create": + self.new_nodes -= 1 + self.parent[self.new_nodes] = None + if not self._attach(self.new_nodes, new_parent): + return False + self._rekey(self.new_nodes, tag.id.casefold()) + elif action.name == "update_parent": + target = self.node_of.get(tag.id.casefold()) + return target is not None and self._attach(target, new_parent) + else: + target = action.target_pk if action.target_pk in self.parent else None + if target is None or not self._attach(target, new_parent): + return False + self._rekey(target, tag.id.casefold()) + return True + + class TagImportPlan: """ Class with functions to build an import plan and excute the plan @@ -112,6 +253,9 @@ def _get_tag_id(self, tag: Tag) -> str: def _build_delete_actions(self, tags: dict): """ Adds delete actions for `tags` + + Every kept child is promoted to a root before the first delete, because + deleting a tag also deletes all its descendants. """ for tag in tags.values(): for child in tag.children.all(): @@ -129,6 +273,7 @@ def _build_delete_actions(self, tags: dict): ), ) + for tag in tags.values(): # Delete action self._build_action( DeleteTag, @@ -228,6 +373,294 @@ def _build_staging_actions(self, tags: list[TagItem]) -> None: self._build_action(StageTagExternalIdForSwap, tag, target_pk=target_pk) self.indexed_actions["_vacated_pks"] = vacated_pks + def _existing_tags(self) -> ExistingTags: + """ + Return the taxonomy's tags keyed by casefolded external_id, with their pk, parent pk and + parent external_id. + + One query, so the plan-level validations don't query per row. + """ + return { + external_id.casefold(): ExistingTag(pk, parent_pk, parent_external_id) + for external_id, pk, parent_pk, parent_external_id in self.taxonomy.tag_set.values_list( + "external_id", "pk", "parent_id", "parent__external_id", + ) + } + + def _validate_parent_id_not_stale(self, tags: list[TagItem], existing: ExistingTags) -> None: + """ + Reject a row whose ``parent_id`` is its current parent's pre-import id + when this import renames that parent to another id. + + The error names the parent's new id. A case-only rename is not + flagged. After a swap, a child naming the stale id of a swapped parent + is rejected, and one naming the end-state id is accepted. A case + variant of the stale id counts too when a rename row claims that id, + because ids are case-insensitive. Rows are reported by 1-based + position in the parsed row list, not a file line. Must run after + ``_build_staging_actions``, which fills ``indexed_actions["_vacated_pks"]``. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + vacated_pks: dict[int, str] = self.indexed_actions["_vacated_pks"] + if not vacated_pks: + return + rename_final_ids = { + tag.id.casefold() for tag in tags + if tag.previous_id and tag.id != tag.previous_id + } + for position, tag in enumerate(tags, start=1): + if not tag.parent_id: + continue + current = existing.get((tag.previous_id or tag.id).casefold()) + parent_key = tag.parent_id.casefold() + if current is None or (current.parent_external_id or "").casefold() != parent_key: + continue + # An exact match is a "no change" row for UpdateParentTag.applies_for, which skips + # _validate_parent. A case variant reaches _validate_parent as a parent change and + # is reported there, except when a rename row claims the id: it then passes there. + if current.parent_external_id != tag.parent_id and parent_key not in rename_final_ids: + continue + new_id = vacated_pks.get(current.parent_pk) if current.parent_pk is not None else None + is_rename = bool(tag.previous_id) and tag.id != tag.previous_id + # A rename row's own validation already reports a stale parent_id, + # except in a swap, where the id is also another rename's final id. + if ( + new_id is not None + and new_id.casefold() != parent_key + and (not is_rename or parent_key in rename_final_ids) + ): + self.errors.append(ParentIdRenamedAwayError(position, tag.parent_id, new_id)) + + def _end_state_parents( + self, tags: list[TagItem], existing: ExistingTags, + ) -> tuple[dict[str, str], dict[str, int]]: + """ + Return the end-state parent graph and the position of each id's first row. + + The graph maps a casefolded final id to its parent's casefolded final id: + each row contributes ``id -> parent_id``, and each remaining tag (not + named by any row's id or previous_id, and not deleted) contributes its + id and its current parent's id, or that parent's new id if it is renamed. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + :return: the graph, and the position in ``tags`` of each final id's first row. + """ + # Not set until _build_staging_actions runs; callers may use this before it. + vacated_pks: dict[int, str] = self.indexed_actions.get("_vacated_pks", {}) + deleted_ids = {action.tag.id.casefold() for action in self.indexed_actions["delete"]} + parent_of: dict[str, str] = {} + row_position: dict[str, int] = {} + represented: set[str] = set() + for position, tag in enumerate(tags): + key = tag.id.casefold() + represented.add(key) + if tag.previous_id: + represented.add(tag.previous_id.casefold()) + if key in row_position: + continue + row_position[key] = position + if tag.parent_id: + parent_of[key] = tag.parent_id.casefold() + for key, tag_info in existing.items(): + parent_id = tag_info.parent_external_id + if key in represented or key in deleted_ids or tag_info.parent_pk is None or parent_id is None: + continue + if parent_id.casefold() in deleted_ids: + continue + parent_of[key] = vacated_pks.get(tag_info.parent_pk, parent_id).casefold() + return parent_of, row_position + + def _validate_no_parent_cycles(self, tags: list[TagItem], existing: ExistingTags) -> None: + """ + Reject a row whose ``parent_id`` makes a tag its own ancestor (a + self-parent is a cycle of length one) once this import is applied. + + The check runs on the end-state graph from ``_end_state_parents``. A cycle + is reported once, on its first row in the parsed row list. + A cycle of definer rows is skipped only when none of its ids is held by + an existing tag, because only then does ``_validate_parent`` report it + (as an unknown parent). Must run after ``_build_staging_actions``. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + parent_of, row_position = self._end_state_parents(tags, existing) + is_definer_row = {key: self._defines_id(tags[position], existing) for key, position in row_position.items()} + + state: dict[str, int] = {} # 1 on the current path, 2 once fully walked + for tag in tags: + path: list[str] = [] + node: str | None = tag.id.casefold() + while node is not None and node not in state: + state[node] = 1 + path.append(node) + node = parent_of.get(node) + if node is not None and state[node] == 1: + cycle = path[path.index(node):] + rows = [row_position[key] for key in cycle if key in row_position] + # _validate_parent only reports a cycle of definer rows when no + # id in it is held by an existing tag (it finds that one in the DB). + if rows and ( + not all(is_definer_row.get(key, False) for key in cycle) + or any(key in existing for key in cycle) + ): + position = min(rows) + self.errors.append(ParentCycleError(position + 1, tags[position].parent_id or "")) + for key in path: + state[key] = 2 + + @staticmethod + def _defines_id(tag: TagItem, existing: ExistingTags) -> bool: + """ + Return whether the row defines its final id: it is a rename row, or it + creates a tag, so the id is not held by an existing tag. + """ + is_rename = bool(tag.previous_id) and tag.id != tag.previous_id + return is_rename or tag.id.casefold() not in existing + + def _order_rows_by_parent_dependency(self, tags: list[TagItem], existing: ExistingTags) -> list[TagItem]: + """ + Return ``tags`` reordered so that parent references resolve and no move + builds a cycle. + + A row follows the row that defines its ``parent_id`` (a rename row or a + new-tag row with that final id, case-insensitive). A row that moves an + existing tag under a new parent also follows the rows of the tags on + that parent's end-state ancestor chain that currently sit inside its + subtree, so a move out of a subtree runs before a move into it. + Validation and execution both follow action order, so this makes both + independent of row order. Only a row and its dependencies are moved: + unrelated rows between them end up after both. Rows that are already in + a valid order keep their position, so their plan is unchanged. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + definers: dict[str, int] = {} + for position, tag in enumerate(tags): + if self._defines_id(tag, existing): + definers.setdefault(tag.id.casefold(), position) + parent_of, row_position = self._end_state_parents(tags, existing) + parent_pk_of = {info.pk: info.parent_pk for info in existing.values()} + + def dependencies(position: int) -> list[int]: + """ + Return the positions of the rows that must run before the row at ``position``. + """ + tag = tags[position] + if not tag.parent_id: + return [] + result = [] + definer = definers.get(tag.parent_id.casefold()) + if definer is not None: + result.append(definer) + current = existing.get((tag.previous_id or tag.id).casefold()) + if current is None: + return result + node: str | None = tag.parent_id.casefold() + visited: set[str] = set() + while node is not None and node not in visited: + visited.add(node) + q_position = row_position.get(node) + q_row = tags[q_position] if q_position is not None else None + q_current = existing.get((q_row.previous_id or q_row.id).casefold()) if q_row else None + if q_current is not None and q_current.pk != current.pk: + ancestor_pk = q_current.parent_pk + while ancestor_pk is not None and ancestor_pk != current.pk: + ancestor_pk = parent_pk_of.get(ancestor_pk) + if ancestor_pk is not None and q_position is not None: + result.append(q_position) + node = parent_of.get(node) + return result + + return self._order_by_dependencies(tags, [dependencies(position) for position in range(len(tags))]) + + @staticmethod + def _order_by_dependencies(tags: list[TagItem], dependencies: list[list[int]]) -> list[TagItem]: + """ + Return ``tags`` in an order where each row follows the rows listed in + ``dependencies[position]``, keeping file order otherwise. A dependency + cycle is broken at its second visit, leaving a best-effort order. + """ + ordered: list[int] = [] + seen: set[int] = set() # emitted or in progress; the latter breaks cycles + for start in range(len(tags)): + stack = [(start, False)] + while stack: + position, done = stack.pop() + if done: + ordered.append(position) + continue + if position in seen: + continue + seen.add(position) + stack.append((position, True)) + stack.extend((dependency, False) for dependency in reversed(dependencies[position])) + return [tags[position] for position in ordered] + + def _order_rows_parent_first(self, tags: list[TagItem], existing: ExistingTags) -> list[TagItem]: + """ + Return ``tags`` with each row after the row whose final id is its ``parent_id``. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + _parent_of, row_position = self._end_state_parents(tags, existing) + dependencies = [ + [row_position[tag.parent_id.casefold()]] + if tag.parent_id and row_position.get(tag.parent_id.casefold(), position) != position else [] + for position, tag in enumerate(tags) + ] + return self._order_by_dependencies(tags, dependencies) + + def _build_detach_actions(self, tags: list[TagItem], existing: ExistingTags) -> None: + """ + Add an update-to-root action for every existing tag whose row changes its parent. + + After these actions every remaining parent link is an unchanged one, so no later step + sees a tree deeper or more cyclic than the final one. Tags that the delete phase has + already promoted are skipped. Must run before ``_build_staging_actions``, because the + actions find their tag by its pre-import external_id. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + end_id_by_pk: dict[int, str] = {} + for tag in tags: + current = existing.get((tag.previous_id or tag.id).casefold()) + if current is not None: + end_id_by_pk.setdefault(current.pk, tag.id) + deleted_ids = {action.tag.id.casefold() for action in self.indexed_actions["delete"]} + for tag in tags: + current = existing.get((tag.previous_id or tag.id).casefold()) + if current is None or current.parent_pk is None or current.parent_external_id is None: + continue + if current.parent_external_id.casefold() in deleted_ids: + continue # already promoted by the delete phase + parent_end_id = end_id_by_pk.get(current.parent_pk, current.parent_external_id).casefold() + if (tag.parent_id or "").casefold() != parent_end_id: + self._build_action( + UpdateParentTag, TagItem(id=tag.previous_id or tag.id, value=tag.value, parent_id=None), + ) + + def _parent_changes_are_valid(self, existing: ExistingTags) -> bool: + """ + Return whether every parent change in the built actions leaves a valid tree. + + Actions run in order and ``Tag.save`` validates the depth of each step, so a plan can + fail at execution although its end state is valid. This simulates the parent effects of + ``self.actions`` in memory, with no database access, and returns False if a step would + exceed ``TAXONOMY_MAX_DEPTH``, use a tag that an earlier step removed, or create a cycle + (a second guard: ``_validate_no_parent_cycles`` already rejects cyclic end states). + + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + forest = _ParentForest(existing) + return all(forest.apply(action) for action in self.actions) + def generate_actions( self, tags: list[TagItem], @@ -244,6 +677,22 @@ def generate_actions( but then its parent needs to be updated because its parent is deleted. Those two actions should be merged. """ + existing = self._existing_tags() + self._generate_actions(tags, replace, existing, safe=False) + # A plan whose intermediate states would be invalid is rebuilt detach-first. If that + # is still invalid, no order of the actions works. + if not self.errors and not self._parent_changes_are_valid(existing): + self._generate_actions(tags, replace, existing, safe=True) + if not self._parent_changes_are_valid(existing): + self.errors.append(InvalidParentHierarchyError(TAXONOMY_MAX_DEPTH + 1)) + + def _generate_actions(self, tags: list[TagItem], replace: bool, existing: ExistingTags, safe: bool) -> None: + """ + Build the actions for ``tags``, with the rows ordered by ``_order_rows_by_parent_dependency``. + + In ``safe`` mode the existing tags whose parent changes are first detached to roots and + the rows are ordered by ``_order_rows_parent_first`` instead. + """ self.actions.clear() self.errors.clear() self._init_indexed_actions() @@ -286,12 +735,23 @@ def generate_actions( # Delete all not readed tags self._build_delete_actions(tags_for_delete) + if safe: + self._build_detach_actions(tags, existing) + # Stage tags whose external_id is contended by another rename row in # this same import, so a swap or an N-cycle of renames has a valid # execution order regardless of how the rows are ordered in the file. self._build_staging_actions(tags) - for tag in tags: + # Reject parent references that the per-row actions never validate. + self._validate_parent_id_not_stale(tags, existing) + self._validate_no_parent_cycles(tags, existing) + + if safe: + rows = self._order_rows_parent_first(tags, existing) + else: + rows = self._order_rows_by_parent_dependency(tags, existing) + for tag in rows: has_action = False # Check all available actions and add which ones should be executed diff --git a/tests/openedx_tagging/import_export/test_actions.py b/tests/openedx_tagging/import_export/test_actions.py index cbec11b54..c237ee31d 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -132,7 +132,7 @@ def test_validate_parent(self, parent_id: str, expected: bool): ( "Action error in 'import_action' (#100): " "Unknown parent tag (tag_100). " - "You need to add parent before the child in your file." + "Add a row for the parent to your file." ) ) diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index 65517f8e7..8b5c1983a 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -1,14 +1,18 @@ """ Test for import/export API """ +import csv +import itertools import json -from io import BytesIO +from contextlib import contextmanager +from io import BytesIO, StringIO +from django.db import transaction from django.test.testcases import TestCase import openedx_tagging.import_export.api as import_export_api from openedx_tagging.import_export import ParserFormat -from openedx_tagging.models import Tag, TagImportTask, TagImportTaskState, Taxonomy +from openedx_tagging.models import ObjectTag, Tag, TagImportTask, TagImportTaskState, Taxonomy from .mixins import TestImportExportMixin @@ -51,6 +55,27 @@ def setUp(self) -> None: ) return super().setUp() + def _import_rows(self, rows: list[dict], replace: bool = False): + """ + Import `rows` as a JSON file and return (result, log), checking the + task log matches the last import log. + """ + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + BytesIO(json.dumps({"tags": rows}).encode()), + self.parser_format, + replace=replace, + ) + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + return result, log + + def _snapshot(self) -> set: + """ + Return the (pk, external_id, parent pk) of every tag in the taxonomy. + """ + return set(self.taxonomy.tag_set.values_list("pk", "external_id", "parent_id")) + def test_check_status(self) -> None: TagImportTask.create(self.taxonomy) status = import_export_api.get_last_import_status(self.taxonomy) @@ -1052,6 +1077,367 @@ def test_import_rename_referencing_stale_old_id_rejected(self) -> None: assert not self.taxonomy.tag_set.filter(external_id="tag_50").exists() assert not self.taxonomy.tag_set.filter(external_id="tag_60").exists() + def test_import_existing_child_names_renamed_away_parent_rejected(self) -> None: + """ + An existing child (tag_2 under tag_1) that keeps parent_id="tag_1" + while the file renames tag_1 to tag_50 is rejected in either row + order, and neither the taxonomy nor its export changes. + """ + rename = {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"} + child = {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_1"} + snapshot_before = self._snapshot() + export_before = import_export_api.export_tags(self.taxonomy, self.parser_format) + for rows in ([rename, child], [child, rename]): + with self.subTest(first=rows[0]["id"]): + result, log = self._import_rows(rows) + assert not result + assert f"Row {rows.index(child) + 1}'s parent_id (tag_1) names an id this file renames away." in log + assert "Use 'tag_50' as the parent_id." in log + assert "Traceback" not in log + + assert self._snapshot() == snapshot_before + assert import_export_api.export_tags(self.taxonomy, self.parser_format) == export_before + + def test_import_child_and_renamed_parent_row_claiming_freed_id(self) -> None: + """ + tag_3 is renamed to tag_1, an id the same replace=True import frees by + omitting its holder. A child row naming tag_1 succeeds in either row + order: the renamed tag keeps its pk, the old holder is gone, and the + child is nested under the renamed tag. + """ + pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + pk_3 = self.taxonomy.tag_set.get(external_id="tag_3").pk + child = {"id": "tag_4", "value": "Tag 4", "parent_id": "tag_1"} + rename = {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"} + for rows in ([child, rename], [rename, child]): + with self.subTest(first=rows[0]["id"]), transaction.atomic(): + result, log = self._import_rows(rows, replace=True) + assert "Traceback" not in log + assert result + + assert Tag.objects.get(pk=pk_3).external_id == "tag_1" + assert not Tag.objects.filter(pk=pk_1).exists() + assert self.taxonomy.tag_set.get(external_id="tag_4").parent_id == pk_3 + transaction.set_rollback(True) + + def test_import_swap_with_children_in_any_row_order(self) -> None: + """ + The swap with children on end-state ids gives the same result in + every one of the 24 row orders. + """ + pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + pk_2 = self.taxonomy.tag_set.get(external_id="tag_2").pk + pk_3 = self.taxonomy.tag_set.get(external_id="tag_3").pk + pk_4 = self.taxonomy.tag_set.get(external_id="tag_4").pk + rows = [ + {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"}, + {"id": "tag_3", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_3"}, + {"id": "tag_4", "value": "Tag 4", "parent_id": "tag_1"}, + ] + for order in itertools.permutations(range(4)): + with self.subTest(order=order), transaction.atomic(): + result, log = self._import_rows([rows[i] for i in order], replace=True) + assert "Traceback" not in log + assert result + + assert Tag.objects.get(pk=pk_1).external_id == "tag_3" + assert Tag.objects.get(pk=pk_3).external_id == "tag_1" + assert Tag.objects.get(pk=pk_2).parent_id == pk_1 + assert Tag.objects.get(pk=pk_4).parent_id == pk_3 + + output = import_export_api.export_tags(self.taxonomy, self.parser_format) + exported_tags = json.loads(output).get("tags") + for tag in exported_tags: + assert not tag.get("id", "").startswith("oel-import-staging:") + exported_by_id = {tag["id"]: tag for tag in exported_tags} + assert exported_by_id["tag_2"]["parent_id"] == "tag_3" + assert exported_by_id["tag_4"]["parent_id"] == "tag_1" + transaction.set_rollback(True) + + def test_import_swap_children_stale_ids_rejected_in_any_row_order(self) -> None: + """ + Children naming the stale pre-import ids are rejected in every one of + the 24 row orders, each error reports the row's file position, and + the taxonomy is unchanged. + """ + rows = [ + {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"}, + {"id": "tag_3", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_1"}, + {"id": "tag_4", "value": "Tag 4", "parent_id": "tag_3"}, + ] + before = self._snapshot() + new_ids = {"tag_2": ("tag_1", "tag_3"), "tag_4": ("tag_3", "tag_1")} + for order in itertools.permutations(range(4)): + with self.subTest(order=order): + file_rows = [rows[i] for i in order] + result, log = self._import_rows(file_rows, replace=True) + assert not result + assert "Traceback" not in log + for position, row in enumerate(file_rows, start=1): + if row["id"] in new_ids: + stale_id, new_id = new_ids[row["id"]] + assert f"Row {position}'s parent_id ({stale_id}) names an id this file renames away." in log + assert f"Use '{new_id}' as the parent_id." in log + assert self._snapshot() == before + + def test_import_child_of_invalid_definer_row_rejected_in_any_order(self) -> None: + """ + A child whose parent_id names a rename row with an unmatched + previous_id, or a new-tag row with a duplicate value, is rejected + cleanly in either row order and leaves the taxonomy unchanged. + """ + cases = { + "unmatched previous_id": ( + {"id": "tag_90", "value": "Tag 90", "previous_id": "tag_99"}, + "Unknown previous_id (tag_99). No tag with that external_id exists in this taxonomy.", + ), + "duplicate value": ({"id": "tag_90", "value": "Tag 1"}, None), + } + child = {"id": "tag_91", "value": "Tag 91", "parent_id": "tag_90"} + before = self._snapshot() + for name, (definer, cause) in cases.items(): + for rows in ([definer, child], [child, definer]): + with self.subTest(case=name, first=rows[0]["id"]): + result, log = self._import_rows(rows) + assert not result + assert "Traceback" not in log + assert "Output errors" in log + if cause: + assert cause in log + assert self._snapshot() == before + + def test_import_cyclic_parent_ids_rejected(self) -> None: + """ + Two new tags naming each other as parent are rejected with an unknown + parent error, with and without replace, and nothing is created. + """ + rows = [ + {"id": "tag_90", "value": "Tag 90", "parent_id": "tag_91"}, + {"id": "tag_91", "value": "Tag 91", "parent_id": "tag_90"}, + ] + before = self._snapshot() + for replace in (False, True): + with self.subTest(replace=replace): + result, log = self._import_rows(rows, replace=replace) + assert not result + assert "Unknown parent tag" in log + assert "Traceback" not in log + assert self._snapshot() == before + + def test_import_child_of_new_row_with_value_freed_by_replace(self) -> None: + """ + With replace=True, a new row reusing the value of an existing tag the + file omits is accepted in every row order, because the replace-delete + frees the value, and its child is nested under it. If the file keeps + the existing tag, the same value is a real duplicate and is rejected. + """ + definer = {"id": "tag_90", "value": "Tag 1"} + child = {"id": "tag_91", "value": "Tag 91", "parent_id": "tag_90"} + kept = {"id": "tag_1", "value": "Tag 1"} + before = self._snapshot() + for order in itertools.permutations(range(2)): + with self.subTest(case="omitted", order=order), transaction.atomic(): + result, log = self._import_rows([[definer, child][i] for i in order], replace=True) + assert "Traceback" not in log + assert result + parent = self.taxonomy.tag_set.get(external_id="tag_90") + assert self.taxonomy.tag_set.get(external_id="tag_91").parent_id == parent.pk + transaction.set_rollback(True) + for order in itertools.permutations(range(3)): + with self.subTest(case="kept", order=order): + result, log = self._import_rows([[definer, child, kept][i] for i in order], replace=True) + assert not result + assert "Traceback" not in log + assert "Output errors" in log + assert self._snapshot() == before + + def test_import_longer_and_mixed_parent_cycles_rejected_in_any_row_order(self) -> None: + """ + A three-tag cycle of new tags, and a cycle through a rename row (the + rename row's parent_id names a new tag whose parent_id names the + rename row's new id), are rejected cleanly in every row order and + leave the taxonomy unchanged. + """ + cases = { + "three new tags": [ + {"id": "tag_90", "value": "Tag 90", "parent_id": "tag_91"}, + {"id": "tag_91", "value": "Tag 91", "parent_id": "tag_92"}, + {"id": "tag_92", "value": "Tag 92", "parent_id": "tag_90"}, + ], + "rename row and new tag": [ + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1", "parent_id": "tag_90"}, + {"id": "tag_90", "value": "Tag 90", "parent_id": "tag_50"}, + ], + } + before = self._snapshot() + for name, rows in cases.items(): + for order in itertools.permutations(range(len(rows))): + with self.subTest(case=name, order=order): + result, log = self._import_rows([rows[i] for i in order]) + assert not result + assert "Traceback" not in log + assert "Output errors" in log + assert self._snapshot() == before + + def test_import_parent_cycles_through_existing_tags_rejected(self) -> None: + """ + A cycle that includes an existing tag is rejected once, in either row + order, and leaves the taxonomy unchanged: an existing and a new tag as + each other's parent, a rename row and an existing tag, a self-parent, and a cycle through tag_2, which + is a child of tag_1 and not in the file. + """ + cases = { + "existing and new tag": ( + [ + {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_90"}, + {"id": "tag_90", "value": "Tag 90", "parent_id": "tag_2"}, + ], + "creates a parent cycle.", + ), + "self parent": ( + [{"id": "tag_2", "value": "Tag 2", "parent_id": "tag_2"}], + "Row 1's parent_id (tag_2) creates a parent cycle.", + ), + "rename row and existing tag": ( + [ + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1", "parent_id": "tag_2"}, + {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_50"}, + ], + "creates a parent cycle.", + ), + "through a tag not in the file": ( + [{"id": "tag_1", "value": "Tag 1", "parent_id": "tag_2"}], + "Row 1's parent_id (tag_2) creates a parent cycle.", + ), + } + before = self._snapshot() + for name, (rows, message) in cases.items(): + for order in itertools.permutations(range(len(rows))): + with self.subTest(case=name, order=order): + result, log = self._import_rows([rows[i] for i in order]) + assert not result + assert "Traceback" not in log + assert log.count(message) == 1 + assert self._snapshot() == before + + def test_import_existing_tag_moved_under_later_new_parent(self) -> None: + """ + An existing tag moved under a new parent defined later in the file is + accepted, and the tag ends up under the new parent. + """ + result, log = self._import_rows([ + {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_90"}, + {"id": "tag_90", "value": "Tag 90"}, + ]) + assert result + assert "Traceback" not in log + parent = self.taxonomy.tag_set.get(external_id="tag_90") + assert self.taxonomy.tag_set.get(external_id="tag_2").parent_id == parent.pk + + def test_import_swap_rename_child_stale_parent_id_rejected_once(self) -> None: + """ + In a tag_1 <-> tag_3 swap, a rename row (tag_2 -> tag_20) naming the + stale parent id tag_1 is rejected, and the error appears exactly once. + """ + before = self._snapshot() + result, log = self._import_rows([ + {"id": "tag_3", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"}, + {"id": "tag_20", "value": "Tag 2", "previous_id": "tag_2", "parent_id": "tag_1"}, + ]) + assert not result + assert "Traceback" not in log + assert log.count("Row 3's parent_id (tag_1) names an id this file renames away.") == 1 + assert self._snapshot() == before + + def test_import_rename_child_stale_parent_id_without_swap_reported_once(self) -> None: + """ + Without a swap, a rename row (tag_2 -> tag_20) naming the stale parent + id tag_1 gets only the rename action's error, once. + """ + before = self._snapshot() + result, log = self._import_rows([ + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_20", "value": "Tag 2", "previous_id": "tag_2", "parent_id": "tag_1"}, + ]) + assert not result + assert "Traceback" not in log + assert log.count("Parent tag tag_1 is renamed to tag_50 in this file; use tag_50 as the parent_id.") == 1 + assert "names an id this file renames away" not in log + assert self._snapshot() == before + + def test_import_case_only_parent_rename_keeps_child_parent(self) -> None: + """ + Renaming a parent only by case succeeds, and a child that keeps the + old casing in parent_id stays under the same parent. + """ + pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + pk_2 = self.taxonomy.tag_set.get(external_id="tag_2").pk + result, log = self._import_rows([ + {"id": "TAG_1", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_1"}, + ]) + assert result + assert "Traceback" not in log + assert Tag.objects.get(pk=pk_1).external_id == "TAG_1" + assert Tag.objects.get(pk=pk_2).parent_id == pk_1 + + def test_import_new_child_before_new_parent_row(self) -> None: + """ + A new child row listed before its new parent row succeeds, the child + is nested under the new parent, and the export shows its parent_id. + """ + result, log = self._import_rows([ + {"id": "tag_91", "value": "Tag 91", "parent_id": "tag_90"}, + {"id": "tag_90", "value": "Tag 90"}, + ]) + assert result + assert "Traceback" not in log + parent = self.taxonomy.tag_set.get(external_id="tag_90") + assert self.taxonomy.tag_set.get(external_id="tag_91").parent_id == parent.pk + + output = import_export_api.export_tags(self.taxonomy, self.parser_format) + exported_by_id = {tag["id"]: tag for tag in json.loads(output).get("tags")} + assert exported_by_id["tag_91"]["parent_id"] == "tag_90" + + def test_import_child_first_references_plain_rename_new_id(self) -> None: + """ + A new child row listed before the plain rename (tag_1 -> tag_50) of + its parent, and naming the new id, succeeds and is nested under the + renamed tag. + """ + pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + result, log = self._import_rows([ + {"id": "tag_60", "value": "Tag 60", "parent_id": "tag_50"}, + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"}, + ]) + assert result + assert "Traceback" not in log + assert Tag.objects.get(pk=pk_1).external_id == "tag_50" + assert self.taxonomy.tag_set.get(external_id="tag_60").parent_id == pk_1 + + def test_import_plain_and_rename_children_of_renamed_parent_in_reverse_order(self) -> None: + """ + A plain child and a rename-row child both naming the renamed parent's + new id, listed before the parent's rename row, succeed and end up + under the renamed parent. + """ + pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + pk_2 = self.taxonomy.tag_set.get(external_id="tag_2").pk + result, log = self._import_rows([ + {"id": "tag_20", "value": "Tag 2", "previous_id": "tag_2", "parent_id": "tag_50"}, + {"id": "tag_60", "value": "Tag 60", "parent_id": "tag_50"}, + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"}, + ]) + assert result + assert "Traceback" not in log + assert Tag.objects.get(pk=pk_1).external_id == "tag_50" + assert Tag.objects.get(pk=pk_2).external_id == "tag_20" + assert Tag.objects.get(pk=pk_2).parent_id == pk_1 + assert self.taxonomy.tag_set.get(external_id="tag_60").parent_id == pk_1 + def test_import_same_value_without_external_id(self) -> None: new_taxonomy = Taxonomy(name="New taxonomy") new_taxonomy.save() @@ -1072,3 +1458,748 @@ def test_import_same_value_without_external_id(self) -> None: replace=True, ) assert result + + +def _row(tag_id: str, value: str, parent_id: str | None = None, previous_id: str | None = None) -> dict: + """ + Build an import row, leaving out the optional fields that are not set. + """ + row = {"id": tag_id, "value": value} + if parent_id: + row["parent_id"] = parent_id + if previous_id: + row["previous_id"] = previous_id + return row + + +class TestImportAcceptanceScenarios(TestCase): + """ + The acceptance scenarios of the external_id rename import (#673, #858), run the way the + Studio import wizard does: a full replace import of a CSV or JSON file, checked through + the import log and the exported file. + """ + + formats = (ParserFormat.JSON, ParserFormat.CSV) + + def setUp(self) -> None: + super().setUp() + self.taxonomy = Taxonomy.objects.create(name="Competencies") + + def _render(self, rows: list[dict], parser_format: ParserFormat) -> BytesIO: + """ + Render `rows` as an import file of the given format. + """ + if parser_format == ParserFormat.JSON: + return BytesIO(json.dumps({"tags": rows}).encode()) + buffer = StringIO() + writer = csv.DictWriter(buffer, fieldnames=["id", "value", "parent_id", "previous_id"]) + writer.writeheader() + writer.writerows(rows) + return BytesIO(buffer.getvalue().encode()) + + def _import(self, rows: list[dict], parser_format: ParserFormat, plan_only: bool = False): + """ + Run a full replace import and return (result, log, plan). + """ + result, task, plan = import_export_api.import_tags( + self.taxonomy, + self._render(rows, parser_format), + parser_format, + replace=True, + plan_only=plan_only, + ) + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + assert "Traceback" not in log + return result, log, plan + + def _seed(self, rows: list[dict]) -> dict[str, int]: + """ + Create the starting tags and return their pks by external_id. + """ + result, _log, _plan = self._import(rows, ParserFormat.JSON) + assert result + return {tag.external_id: tag.pk for tag in self.taxonomy.tag_set.all()} + + def _associate(self, pks: dict[str, int], *external_ids: str) -> dict[int, int]: + """ + Tag a content object with each tag and return the ObjectTag pks by tag pk. + """ + object_tags = {} + for external_id in external_ids: + tag = Tag.objects.get(pk=pks[external_id]) + object_tag = ObjectTag.objects.create( + object_id=f"object-{tag.pk}", taxonomy=self.taxonomy, tag=tag, _value=tag.value, + ) + object_tags[tag.pk] = object_tag.pk + return object_tags + + def _assert_associations_kept(self, object_tags: dict[int, int]) -> None: + """ + Check each ObjectTag still points to the same tag, not a recreated one. + """ + for tag_pk, object_tag_pk in object_tags.items(): + assert ObjectTag.objects.get(pk=object_tag_pk).tag_id == tag_pk + + def _export(self, parser_format: ParserFormat) -> str: + """ + Export the taxonomy in the given format. + """ + return import_export_api.export_tags(self.taxonomy, parser_format) + + def _exported_rows(self, parser_format: ParserFormat) -> list[dict]: + """ + Parse the export and check it only carries the id, value and parent_id fields. + """ + output = self._export(parser_format) + if parser_format == ParserFormat.JSON: + rows = json.loads(output)["tags"] + fields = {key for row in rows for key in row} + else: + reader = csv.DictReader(StringIO(output)) + assert reader.fieldnames == ["id", "value", "parent_id"] + rows = list(reader) + fields = set(reader.fieldnames) + assert fields <= {"id", "value", "parent_id"} + return [{"id": row["id"], "value": row["value"], "parent_id": row.get("parent_id") or None} for row in rows] + + def _exported_ids(self, parser_format: ParserFormat) -> dict[str, str]: + """ + Return the exported id of each tag, by tag value. + """ + return {row["value"]: row["id"] for row in self._exported_rows(parser_format)} + + def _exported_parents(self, parser_format: ParserFormat) -> dict[str, str | None]: + """ + Return the exported parent_id of each tag, by tag value. + """ + return {row["value"]: row["parent_id"] for row in self._exported_rows(parser_format)} + + @contextmanager + def _rolled_back(self): + """ + Run a block in a savepoint that is rolled back, so a scenario can be repeated. + """ + with transaction.atomic(): + yield + transaction.set_rollback(True) + + def _assert_rejected(self, rows: list[dict], parser_format: ParserFormat, *messages: str) -> str: + """ + Check the import is rejected, its log has each message, and nothing changed. + """ + snapshot = set(self.taxonomy.tag_set.values_list("pk", "external_id", "value", "parent_id")) + before = self._export(parser_format) + result, log, _plan = self._import(rows, parser_format) + assert not result + for message in messages: + assert message in log + assert set(self.taxonomy.tag_set.values_list("pk", "external_id", "value", "parent_id")) == snapshot + assert self._export(parser_format) == before + return log + + def test_rename_shows_in_export_without_duplicate_or_previous_id(self) -> None: + """ + S1, S2, S15: a rename shows in the export once, as the new id, with no row for the old + id and no previous_id field, and the tag keeps its pk and its content associations. + """ + pks = self._seed([_row("COMP-A", "Alpha")]) + object_tags = self._associate(pks, "COMP-A") + for parser_format in self.formats: + with self.subTest(parser_format=parser_format), self._rolled_back(): + count = len(self._exported_rows(parser_format)) + result, _log, _plan = self._import([_row("COMP-B", "Alpha", previous_id="COMP-A")], parser_format) + assert result + assert [row["id"] for row in self._exported_rows(parser_format)] == ["COMP-B"] + assert len(self._exported_rows(parser_format)) == count + assert self.taxonomy.tag_set.count() == 1 + assert Tag.objects.get(pk=pks["COMP-A"]).external_id == "COMP-B" + self._assert_associations_kept(object_tags) + + def test_reimport_with_previous_id_equal_to_id_is_idempotent(self) -> None: + """ + S3: a row whose previous_id equals its id succeeds and the export does not change. + """ + self._seed([_row("COMP-B", "Beta")]) + for parser_format in self.formats: + with self.subTest(parser_format=parser_format), self._rolled_back(): + before = self._export(parser_format) + result, _log, _plan = self._import([_row("COMP-B", "Beta", previous_id="COMP-B")], parser_format) + assert result + assert self._export(parser_format) == before + + def test_new_id_without_previous_id_is_created(self) -> None: + """ + S4: a row with a new id and no previous_id creates a tag that shows in the export. + """ + self._seed([_row("COMP-A", "Alpha")]) + for parser_format in self.formats: + with self.subTest(parser_format=parser_format), self._rolled_back(): + result, _log, _plan = self._import( + [_row("COMP-A", "Alpha"), _row("COMP-Z", "Zulu")], parser_format, + ) + assert result + assert self._exported_ids(parser_format) == {"Alpha": "COMP-A", "Zulu": "COMP-Z"} + + def test_unmatched_previous_id_rejected(self) -> None: + """ + S5: an unmatched previous_id is rejected with the id in the log, nothing is introduced, + and when several rows have one, every id is in the same log. + """ + self._seed([_row("COMP-A", "Alpha"), _row("COMP-C", "Gamma")]) + for parser_format in self.formats: + with self.subTest(parser_format=parser_format): + self._assert_rejected( + [_row("COMP-B", "Alpha", previous_id="COMP-GHOST"), _row("COMP-C", "Gamma")], + parser_format, + "COMP-GHOST", + ) + self._assert_rejected( + [ + _row("COMP-B", "Alpha", previous_id="COMP-GHOST-1"), + _row("COMP-D", "Gamma", previous_id="COMP-GHOST-2"), + ], + parser_format, + "COMP-GHOST-1", + "COMP-GHOST-2", + ) + + def test_rename_onto_id_of_kept_tag_rejected(self) -> None: + """ + S6, S8: a rename onto an id whose holder the same file keeps is rejected, in either row + order, whether the holder is kept as a plain row or as a row with previous_id equal to id. + """ + self._seed([_row("COMP-Z", "Xray"), _row("COMP-A", "Yank")]) + rename = _row("COMP-Z", "Yank", previous_id="COMP-A") + keeps = { + "plain": _row("COMP-Z", "Xray"), + "previous_id equal to id": _row("COMP-Z", "Xray", previous_id="COMP-Z"), + } + for parser_format in self.formats: + for name, keep in keeps.items(): + for rows in ([rename, keep], [keep, rename]): + with self.subTest(parser_format=parser_format, keep=name, first=rows[0]["value"]): + self._assert_rejected(rows, parser_format, "COMP-Z") + + def test_rename_onto_id_freed_by_omitting_holder(self) -> None: + """ + S7: a rename onto an id the same file frees by omitting its holder succeeds in every row + order: the holder is gone, the renamed tag keeps its pk and associations, and the export + shows it. + """ + pks = self._seed([ + _row("COMP-Z", "Xray"), _row("COMP-A", "Yank"), _row("COMP-W", "Whisky"), _row("COMP-V", "Victor"), + ]) + object_tags = self._associate(pks, "COMP-A") + rows = [ + _row("COMP-Z", "Yank", previous_id="COMP-A"), _row("COMP-W", "Whisky"), _row("COMP-V", "Victor"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(3)): + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order], parser_format) + assert result + assert not Tag.objects.filter(pk=pks["COMP-Z"]).exists() + assert Tag.objects.get(pk=pks["COMP-A"]).external_id == "COMP-Z" + self._assert_associations_kept(object_tags) + assert self._exported_ids(parser_format) == { + "Yank": "COMP-Z", "Whisky": "COMP-W", "Victor": "COMP-V", + } + + def test_swap_keeps_tags_and_associations(self) -> None: + """ + S9: a swap succeeds in either row order, creates and deletes nothing, keeps both tags' + pks and associations, and the export shows the swapped ids. + """ + pks = self._seed([_row("COMP-A", "Alpha"), _row("COMP-B", "Beta")]) + object_tags = self._associate(pks, "COMP-A", "COMP-B") + rows = [_row("COMP-B", "Alpha", previous_id="COMP-A"), _row("COMP-A", "Beta", previous_id="COMP-B")] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order], parser_format) + assert result + assert self.taxonomy.tag_set.count() == 2 + assert Tag.objects.get(pk=pks["COMP-A"]).external_id == "COMP-B" + assert Tag.objects.get(pk=pks["COMP-B"]).external_id == "COMP-A" + self._assert_associations_kept(object_tags) + assert self._exported_ids(parser_format) == {"Alpha": "COMP-B", "Beta": "COMP-A"} + + def test_three_way_rotation_in_any_row_order(self) -> None: + """ + S10: a three-way rotation succeeds in all six row orders, and each tag holds the id its + own row specified. + """ + pks = self._seed([_row("COMP-A", "Alpha"), _row("COMP-B", "Beta"), _row("COMP-C", "Gamma")]) + rows = [ + _row("COMP-B", "Alpha", previous_id="COMP-A"), + _row("COMP-C", "Beta", previous_id="COMP-B"), + _row("COMP-A", "Gamma", previous_id="COMP-C"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(3)): + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order], parser_format) + assert result + assert Tag.objects.get(pk=pks["COMP-A"]).external_id == "COMP-B" + assert Tag.objects.get(pk=pks["COMP-B"]).external_id == "COMP-C" + assert Tag.objects.get(pk=pks["COMP-C"]).external_id == "COMP-A" + assert self._exported_ids(parser_format) == { + "Alpha": "COMP-B", "Beta": "COMP-C", "Gamma": "COMP-A", + } + + def test_rename_chain_colliding_with_kept_tag_rejected_whole(self) -> None: + """ + S11: a rename chain that ends on an id the file keeps is rejected whole in all six row + orders, with the id in the log and all three tags unchanged. + """ + self._seed([_row("COMP-A", "Alpha"), _row("COMP-B", "Beta"), _row("COMP-C", "Gamma")]) + rows = [ + _row("COMP-B", "Alpha", previous_id="COMP-A"), + _row("COMP-C", "Beta", previous_id="COMP-B"), + _row("COMP-C", "Gamma"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(3)): + with self.subTest(parser_format=parser_format, order=order): + self._assert_rejected([rows[i] for i in order], parser_format, "COMP-C") + + def test_child_names_new_id_of_renamed_parent(self) -> None: + """ + S12: a child row naming its renamed parent's new id succeeds in either row order, the + child stays under the same parent tag, and the export shows it nested under the new id. + """ + pks = self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", parent_id="COMP-P")]) + rows = [_row("COMP-P2", "Parent", previous_id="COMP-P"), _row("COMP-K", "Kid", parent_id="COMP-P2")] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order], parser_format) + assert result + assert Tag.objects.get(pk=pks["COMP-K"]).parent_id == pks["COMP-P"] + assert self._exported_ids(parser_format)["Parent"] == "COMP-P2" + assert self._exported_parents(parser_format)["Kid"] == "COMP-P2" + + def test_child_names_pre_import_id_of_renamed_parent_rejected(self) -> None: + """ + S13, A: an existing child naming its parent's pre-import id is rejected in either row + order, with its row position and the new id in the log, and nothing changes. + """ + self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", parent_id="COMP-P")]) + rows = [_row("COMP-P2", "Parent", previous_id="COMP-P"), _row("COMP-K", "Kid", parent_id="COMP-P")] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order): + file_rows = [rows[i] for i in order] + child_row = file_rows.index(rows[1]) + 1 + self._assert_rejected( + file_rows, + parser_format, + f"Row {child_row}'s parent_id (COMP-P) names an id this file renames away.", + "Use 'COMP-P2' as the parent_id.", + ) + + def test_parent_and_child_both_renamed(self) -> None: + """ + S14: renaming a parent and its child together succeeds in either row order, and the + export shows both new ids with the child nested under the parent. + """ + pks = self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", parent_id="COMP-P")]) + rows = [ + _row("COMP-P2", "Parent", previous_id="COMP-P"), + _row("COMP-K2", "Kid", parent_id="COMP-P2", previous_id="COMP-K"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order], parser_format) + assert result + assert Tag.objects.get(pk=pks["COMP-K"]).parent_id == pks["COMP-P"] + assert self._exported_ids(parser_format) == {"Parent": "COMP-P2", "Kid": "COMP-K2"} + assert self._exported_parents(parser_format) == {"Parent": None, "Kid": "COMP-P2"} + + def test_export_has_no_previous_id_and_round_trips_without_changes(self) -> None: + """ + S16: the export only has id, value and parent_id, and importing it back unchanged plans + no change. + """ + self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", parent_id="COMP-P"), _row("COMP-A", "Alpha")]) + for parser_format in self.formats: + with self.subTest(parser_format=parser_format): + exported = self._exported_rows(parser_format) + assert len(exported) == 3 + result, _log, plan = self._import( + [{key: value for key, value in row.items() if value} for row in exported], + parser_format, + plan_only=True, + ) + assert result + assert plan is not None + assert {action.name for action in plan.actions} == {"without_changes"} + + def test_swap_with_children_in_representative_orders(self) -> None: + """ + B, C: in a swap where each parent has a child, children naming the end-state ids succeed + and stay on their physical parents, and children naming the stale ids are rejected with + their row positions, in several row orders and both formats. + """ + pks = self._seed([ + _row("COMP-A", "Alpha"), _row("COMP-AK", "Alpha kid", parent_id="COMP-A"), + _row("COMP-B", "Beta"), _row("COMP-BK", "Beta kid", parent_id="COMP-B"), + ]) + swap = [_row("COMP-B", "Alpha", previous_id="COMP-A"), _row("COMP-A", "Beta", previous_id="COMP-B")] + accepted = swap + [ + _row("COMP-AK", "Alpha kid", parent_id="COMP-B"), _row("COMP-BK", "Beta kid", parent_id="COMP-A"), + ] + stale = swap + [ + _row("COMP-AK", "Alpha kid", parent_id="COMP-A"), _row("COMP-BK", "Beta kid", parent_id="COMP-B"), + ] + orders = [(0, 1, 2, 3), (2, 3, 0, 1), (2, 0, 3, 1), (3, 2, 1, 0)] + for parser_format in self.formats: + for order in orders: + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([accepted[i] for i in order], parser_format) + assert result + assert Tag.objects.get(pk=pks["COMP-AK"]).parent_id == pks["COMP-A"] + assert Tag.objects.get(pk=pks["COMP-BK"]).parent_id == pks["COMP-B"] + assert self._exported_parents(parser_format) == { + "Alpha": None, "Beta": None, "Alpha kid": "COMP-B", "Beta kid": "COMP-A", + } + with self.subTest(parser_format=parser_format, order=order, stale=True): + file_rows = [stale[i] for i in order] + self._assert_rejected( + file_rows, + parser_format, + f"Row {file_rows.index(stale[2]) + 1}'s parent_id (COMP-A) names an id this file renames away.", + f"Row {file_rows.index(stale[3]) + 1}'s parent_id (COMP-B) names an id this file renames away.", + ) + + def test_every_rejected_row_is_in_the_one_log(self) -> None: + """ + Several rejected rows of different kinds each have their message in the same log, since + the wizard shows the log as one block. + """ + self._seed([ + _row("COMP-P", "Parent"), + _row("COMP-K1", "Kid 1", parent_id="COMP-P"), + _row("COMP-K2", "Kid 2", parent_id="COMP-P"), + _row("COMP-A", "Alpha"), + ]) + rows = [ + _row("COMP-P2", "Parent", previous_id="COMP-P"), + _row("COMP-K1", "Kid 1", parent_id="COMP-P"), + _row("COMP-K2", "Kid 2", parent_id="COMP-P"), + _row("COMP-B", "Alpha", previous_id="COMP-GHOST"), + ] + for parser_format in self.formats: + with self.subTest(parser_format=parser_format): + self._assert_rejected( + rows, + parser_format, + "Row 2's parent_id (COMP-P) names an id this file renames away.", + "Row 3's parent_id (COMP-P) names an id this file renames away.", + "COMP-GHOST", + ) + + def test_rename_onto_freed_id_forming_parent_cycle_rejected(self) -> None: + """ + A rename onto an id the same import frees can close a parent cycle through the + renamed tag: a self-parent, and two renames naming each other as parent. Both are + rejected at the plan step with exactly one cycle error, in either row order. + """ + self._seed([_row(tag_id, f"Value {tag_id}") for tag_id in ("COMP-X", "COMP-Y")]) + self_parent = [_row("COMP-X", "Value COMP-Y", parent_id="COMP-X", previous_id="COMP-Y")] + for parser_format in self.formats: + with self.subTest(parser_format=parser_format, case="self parent"): + log = self._assert_rejected(self_parent, parser_format, "creates a parent cycle.") + assert log.count("creates a parent cycle.") == 1 + self._seed([_row(tag_id, f"Value {tag_id}") for tag_id in ("COMP-A", "COMP-B", "COMP-C", "COMP-D")]) + rows = [ + _row("COMP-A", "Value COMP-C", parent_id="COMP-B", previous_id="COMP-C"), + _row("COMP-B", "Value COMP-D", parent_id="COMP-A", previous_id="COMP-D"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order, case="two renames"): + log = self._assert_rejected([rows[i] for i in order], parser_format, "creates a parent cycle.") + assert log.count("creates a parent cycle.") == 1 + + def test_child_of_deleted_holder_moves_under_tag_renamed_onto_its_id(self) -> None: + """ + A child of a tag that the import deletes, while another tag is renamed onto the + deleted tag's id, ends up under the renamed tag, not as a root, in either row order. + """ + pks = self._seed([_row("COMP-X", "Xray"), _row("COMP-Y", "Yank"), _row("COMP-C", "Child", parent_id="COMP-X")]) + object_tags = self._associate(pks, "COMP-C") + rows = [_row("COMP-X", "Yank", previous_id="COMP-Y"), _row("COMP-C", "Child", parent_id="COMP-X")] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order], parser_format) + assert result + assert not Tag.objects.filter(pk=pks["COMP-X"]).exists() + assert Tag.objects.get(pk=pks["COMP-C"]).parent_id == pks["COMP-Y"] + assert Tag.objects.get(pk=pks["COMP-Y"]).external_id == "COMP-X" + self._assert_associations_kept(object_tags) + assert self._exported_parents(parser_format) == {"Yank": None, "Child": "COMP-X"} + + def test_kept_tags_under_deleted_ancestors_become_roots(self) -> None: + """ + A kept tag whose parent is deleted by the import becomes a root, also when that + parent sits under a deleted ancestor, which would otherwise cascade to the kept tag. + """ + cases = { + "grandchild of a deleted chain": ( + [("t0", None), ("t1", "t0"), ("t5", "t1")], ["t5"], + ), + "kept tags at two depths": ( + [("t0", None), ("t1", "t0"), ("t2", "t1"), ("t3", "t2"), ("t4", "t3")], ["t2", "t4"], + ), + "child of a deleted root": ([("t0", None), ("t1", "t0")], ["t1"]), + } + for name, (structure, kept) in cases.items(): + for parser_format in self.formats: + with self.subTest(case=name, parser_format=parser_format), self._rolled_back(): + pks = self._seed([_row(tag_id, f"Value {tag_id}", parent) for tag_id, parent in structure]) + result, _log, _plan = self._import( + [_row(tag_id, f"Value {tag_id}") for tag_id in kept], parser_format, + ) + assert result + assert set(self.taxonomy.tag_set.values_list("pk", flat=True)) == {pks[t] for t in kept} + assert all(tag.parent_id is None for tag in self.taxonomy.tag_set.all()) + assert self._exported_parents(parser_format) == {f"Value {t}": None for t in kept} + + def _assert_tree_after_import( + self, + rows: list[dict], + expected_parents: dict[str, str | None], + pks: dict[str, int], + orders: list[tuple[int, ...]], + parser_formats: tuple[ParserFormat, ...], + *, + static_rows: tuple[dict, ...] = (), + ) -> None: + """ + Import `rows` in each row order and format, followed by `static_rows` (the other tags + the replace import has to keep), and check that every tag in + `expected_parents` (external_id to parent external_id) ends up under that parent in the + database and the export, and that the tags in `pks` keep their pk. Each import is + rolled back. + """ + for parser_format in parser_formats: + for order in orders: + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order] + list(static_rows), parser_format) + assert result + for tag_id, pk in pks.items(): + assert Tag.objects.get(pk=pk).external_id == tag_id + db_parents = { + tag.external_id: tag.parent.external_id if tag.parent else None + for tag in self.taxonomy.tag_set.select_related("parent") + } + assert db_parents == expected_parents + exported = {row["id"]: row["parent_id"] for row in self._exported_rows(parser_format)} + assert exported == expected_parents + + def test_ancestry_inversion_in_any_row_order(self) -> None: + """ + A child of t0 becomes t0's ancestor (a new tag g0 is put between them), which only + works if it moves out of t0's subtree first. It succeeds in all 24 row orders in JSON and + in four of them in CSV, and ends with the expected tree. + """ + pks = self._seed([_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t0")]) + rows = [_row("t0", "Zero", "g0"), _row("t1", "One", "g0"), _row("t2", "Two"), _row("g0", "New", "t2")] + expected = {"t0": "g0", "t1": "g0", "t2": None, "g0": "t2"} + orders = list(itertools.permutations(range(4))) + self._assert_tree_after_import(rows, expected, pks, orders, (ParserFormat.JSON,)) + self._assert_tree_after_import(rows, expected, pks, orders[::6], (ParserFormat.CSV,)) + + def test_parent_and_child_swap_places(self) -> None: + """ + A parent and its child swap places in either row order. + """ + pks = self._seed([_row("t0", "Zero"), _row("t1", "One", "t0")]) + rows = [_row("t0", "Zero", "t1"), _row("t1", "One")] + orders = list(itertools.permutations(range(2))) + self._assert_tree_after_import(rows, {"t0": "t1", "t1": None}, pks, orders, self.formats) + + def test_three_level_chain_reversed(self) -> None: + """ + A chain t0 > t1 > t2 is reversed to t2 > t1 > t0 in all six row orders. + """ + pks = self._seed([_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1")]) + rows = [_row("t2", "Two"), _row("t1", "One", "t2"), _row("t0", "Zero", "t1")] + expected = {"t2": None, "t1": "t2", "t0": "t1"} + orders = list(itertools.permutations(range(3))) + self._assert_tree_after_import(rows, expected, pks, orders, self.formats) + + def test_tag_moved_under_its_former_grandchild(self) -> None: + """ + A root t0 is moved under t2, its former grandchild, which is left a child of t1 + (now a root), in all six row orders. + """ + pks = self._seed([_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1")]) + rows = [_row("t0", "Zero", "t2"), _row("t1", "One"), _row("t2", "Two", "t1")] + expected = {"t0": "t2", "t1": None, "t2": "t1"} + orders = list(itertools.permutations(range(3))) + self._assert_tree_after_import(rows, expected, pks, orders, self.formats) + + def _seed_deep_chain(self) -> dict[str, int]: + """ + Create the chain t0 > t1 > t2 > t3 > t5 and return the pks by external_id. + """ + return self._seed([ + _row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1"), + _row("t3", "Three", "t2"), _row("t5", "Five", "t3"), + ]) + + def test_deep_tag_moved_to_root_with_new_children_in_any_row_order(self) -> None: + """ + t5 is the deepest tag of a chain and moves to the root while new tags g0 and g1 are put + under it. Creating them first would exceed the maximum depth, so the import detaches t5 + first. It succeeds in all six row orders in JSON and in three of them in CSV. + """ + pks = self._seed_deep_chain() + rows = [_row("g1", "New 1", "g0"), _row("g0", "New 0", "t5"), _row("t5", "Five")] + chain = (_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1"), _row("t3", "Three", "t2")) + expected = {"t0": None, "t1": "t0", "t2": "t1", "t3": "t2", "t5": None, "g0": "t5", "g1": "g0"} + orders = list(itertools.permutations(range(3))) + self._assert_tree_after_import(rows, expected, pks, orders, (ParserFormat.JSON,), static_rows=chain) + self._assert_tree_after_import(rows, expected, pks, orders[::2], (ParserFormat.CSV,), static_rows=chain) + + def test_deep_tag_under_renamed_parent_moved_to_root_in_any_row_order(self) -> None: + """ + The same move when t5's old parent t3 is renamed in the same import: all 24 row orders + in JSON succeed and t5 keeps its pk. + """ + pks = self._seed_deep_chain() + rows = [ + _row("g1", "New 1", "g0"), _row("g0", "New 0", "t5"), _row("t5", "Five"), + _row("r3", "Three", "t2", "t3"), + ] + expected = {"t0": None, "t1": "t0", "t2": "t1", "r3": "t2", "t5": None, "g0": "t5", "g1": "g0"} + orders = list(itertools.permutations(range(4))) + kept_pks = {tag_id: pk for tag_id, pk in pks.items() if tag_id != "t3"} | {"r3": pks["t3"]} + static = (_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1")) + self._assert_tree_after_import(rows, expected, kept_pks, orders, (ParserFormat.JSON,), static_rows=static) + + def test_subtree_moved_under_deep_tag_before_it_is_shallowed(self) -> None: + """ + A three-level subtree s0 > s1 > s2 moves under c3, which itself moves to the root, in + every row order and both formats. + """ + pks = self._seed([ + _row("c0", "C0"), _row("c1", "C1", "c0"), _row("c2", "C2", "c1"), _row("c3", "C3", "c2"), + _row("s0", "S0"), _row("s1", "S1", "s0"), _row("s2", "S2", "s1"), + ]) + rows = [_row("s0", "S0", "c3"), _row("c3", "C3"), _row("s1", "S1", "s0"), _row("s2", "S2", "s1")] + expected = {"c0": None, "c1": "c0", "c2": "c1", "c3": None, "s0": "c3", "s1": "s0", "s2": "s1"} + orders = list(itertools.permutations(range(4))) + static = (_row("c0", "C0"), _row("c1", "C1", "c0"), _row("c2", "C2", "c1")) + self._assert_tree_after_import(rows, expected, pks, orders, (ParserFormat.JSON,), static_rows=static) + self._assert_tree_after_import(rows, expected, pks, orders[::6], (ParserFormat.CSV,), static_rows=static) + + def test_three_new_levels_under_moved_deep_tag(self) -> None: + """ + Three new tags chained under t5, which moves to the root, succeed in all 24 row orders. + """ + pks = self._seed_deep_chain() + rows = [ + _row("t5", "Five"), _row("g0", "New 0", "t5"), _row("g1", "New 1", "g0"), _row("g2", "New 2", "g1"), + ] + expected = { + "t0": None, "t1": "t0", "t2": "t1", "t3": "t2", "t5": None, "g0": "t5", "g1": "g0", "g2": "g1", + } + orders = list(itertools.permutations(range(4))) + chain = (_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1"), _row("t3", "Three", "t2")) + self._assert_tree_after_import(rows, expected, pks, orders, (ParserFormat.JSON,), static_rows=chain) + + def test_import_that_leaves_tags_too_deep_is_rejected(self) -> None: + """ + An import that would put a tag at the seventh level, by adding a child to the deepest + tag of a six-level chain or by moving a chain under another, cannot be applied in any + order, so it is rejected at the plan step and nothing changes. + """ + chain = [("a", None), ("b", "a"), ("c", "b"), ("d", "c"), ("e", "d"), ("f", "e")] + two_chains = [("a", None), ("b", "a"), ("c", "b"), ("d", "c"), ("w", None), ("x", "w"), ("y", "x"), ("z", "y")] + cases = { + "child of the deepest tag": ( + chain, [_row(tag_id, f"Value {tag_id}", parent) for tag_id, parent in chain] + + [_row("g", "Value g", "f")], + ), + "chain moved under another": ( + two_chains, + [_row(t, f"Value {t}", "d" if t == "w" else parent) for t, parent in two_chains], + ), + } + message = "This import would leave tags deeper than 6 levels" + for name, (structure, rows) in cases.items(): + self._seed([_row(tag_id, f"Value {tag_id}", parent) for tag_id, parent in structure]) + for parser_format in self.formats: + with self.subTest(case=name, parser_format=parser_format): + log = self._assert_rejected(rows, parser_format, message) + assert log.count(message) == 1 + self.taxonomy.tag_set.all().delete() + + def test_swap_child_naming_parent_id_in_another_case(self) -> None: + """ + In a swap, ids are case-insensitive: a child naming its parent's stale id in another case + is rejected once, with its row position and the new id, like the exact spelling. A child + naming the end-state ids in another case is accepted and stays under its parent. + """ + pks = self._seed([ + _row("COMP-A", "Alpha"), _row("COMP-AK", "Alpha kid", "COMP-A"), + _row("COMP-B", "Beta"), _row("COMP-BK", "Beta kid", "COMP-B"), + ]) + swap = [_row("COMP-B", "Alpha", previous_id="COMP-A"), _row("COMP-A", "Beta", previous_id="COMP-B")] + stale = swap + [_row("COMP-AK", "Alpha kid", "comp-a"), _row("COMP-BK", "Beta kid", "comp-a")] + accepted = swap + [_row("COMP-AK", "Alpha kid", "comp-b"), _row("COMP-BK", "Beta kid", "comp-a")] + expected = {"COMP-B": None, "COMP-A": None, "COMP-AK": "COMP-B", "COMP-BK": "COMP-A"} + moved_pks = { + "COMP-B": pks["COMP-A"], "COMP-A": pks["COMP-B"], "COMP-AK": pks["COMP-AK"], "COMP-BK": pks["COMP-BK"], + } + orders = [(0, 1, 2, 3), (2, 3, 0, 1), (3, 2, 1, 0), (2, 0, 3, 1)] + self._assert_tree_after_import(accepted, expected, moved_pks, orders, self.formats) + for parser_format in self.formats: + for order in orders: + with self.subTest(parser_format=parser_format, order=order, stale=True): + file_rows = [stale[i] for i in order] + position = file_rows.index(stale[2]) + 1 + log = self._assert_rejected( + file_rows, + parser_format, + f"Row {position}'s parent_id (comp-a) names an id this file renames away.", + "Use 'COMP-B' as the parent_id.", + ) + assert log.count("names an id this file renames away") == 1 + + def test_plain_rename_child_naming_parent_id_in_another_case_reported_once(self) -> None: + """ + When a parent is plainly renamed, a child naming its old id in another case gets only + the parent validation error, in either row order and format. + """ + self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", "COMP-P")]) + rows = [_row("COMP-P2", "Parent", previous_id="COMP-P"), _row("COMP-K", "Kid", "comp-p")] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order): + log = self._assert_rejected( + [rows[i] for i in order], + parser_format, + "Parent tag comp-p is renamed to COMP-P2 in this file; use COMP-P2 as the parent_id.", + ) + assert "names an id this file renames away" not in log + + def test_case_only_parent_rename_with_case_variant_child(self) -> None: + """ + Renaming a parent only by case, a child naming it in another case stays under it. + """ + pks = self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", "COMP-P")]) + for child_parent in ("comp-p", "Comp-P"): + rows = [_row("Comp-P", "Parent", previous_id="COMP-P"), _row("COMP-K", "Kid", child_parent)] + expected = {"Comp-P": None, "COMP-K": "Comp-P"} + kept_pks = {"Comp-P": pks["COMP-P"], "COMP-K": pks["COMP-K"]} + with self.subTest(child_parent=child_parent): + self._assert_tree_after_import( + rows, expected, kept_pks, list(itertools.permutations(range(2))), self.formats, + ) diff --git a/tests/openedx_tagging/import_export/test_import_plan.py b/tests/openedx_tagging/import_export/test_import_plan.py index 9fd40b074..4aa9cfdce 100644 --- a/tests/openedx_tagging/import_export/test_import_plan.py +++ b/tests/openedx_tagging/import_export/test_import_plan.py @@ -1,12 +1,19 @@ """ Test for import_plan functions """ +import itertools +import uuid +from unittest import mock + import ddt # type: ignore[import] +from django.db import connection from django.test.testcases import TestCase +from django.test.utils import CaptureQueriesContext -from openedx_tagging.import_export.actions import CreateTag +from openedx_tagging.import_export.actions import CreateTag, DeleteTag, RenameTagExternalId, UpdateParentTag from openedx_tagging.import_export.exceptions import TagImportError -from openedx_tagging.import_export.import_plan import TagImportPlan, TagItem +from openedx_tagging.import_export.import_plan import ExistingTag, TagImportPlan, TagItem, _ParentForest +from openedx_tagging.models import Tag, Taxonomy from .test_actions import TestImportActionMixin @@ -181,6 +188,10 @@ def test_build_delete_actions(self) -> None: True, 0, [ + { + 'name': 'update_parent', + 'id': 'tag_4', + }, { 'name': 'delete', 'id': 'tag_1', @@ -189,10 +200,6 @@ def test_build_delete_actions(self) -> None: 'name': 'delete', 'id': 'tag_2', }, - { - 'name': 'update_parent', - 'id': 'tag_4', - }, { 'name': 'delete', 'id': 'tag_3', @@ -265,7 +272,7 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "Conflict with 'create' (#2) and action #1: Duplicated external_id tag.\n" "Action error in 'rename' (#3): Duplicated tag value with tag in database (external_id=tag_2).\n" "Action error in 'update_parent' (#4): Unknown parent tag (tag_100). " - "You need to add parent before the child in your file.\n" + "Add a row for the parent to your file.\n" "Conflict with 'create' (#5) and action #2: Duplicated tag value.\n" "Conflict with 'rename' (#7) and action #1: Duplicated tag value.\n" ), @@ -319,10 +326,10 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions True, "Import plan for Import Taxonomy Test\n" "--------------------------------\n" - "#1: Delete tag (tag_1 / Tag 1)\n" - "#2: Delete tag (tag_2 / Tag 2)\n" - "#3: Update the parent of (tag_4 / Tag 4) from parent " + "#1: Update the parent of (tag_4 / Tag 4) from parent " " (tag_3 / Tag 3) to None\n" + "#2: Delete tag (tag_1 / Tag 1)\n" + "#3: Delete tag (tag_2 / Tag 2)\n" "#4: Delete tag (tag_3 / Tag 3)\n" "#5: No changes needed for (tag_4 / Tag 4)\n" ), @@ -619,6 +626,738 @@ def test_generate_actions_parent_id_new_id_after_earlier_rename_accepted(self) - self.import_plan.generate_actions(tags=tags, replace=False) self.assertEqual(self.import_plan.errors, []) + def test_generate_actions_existing_child_names_renamed_away_parent_rejected(self) -> None: + """ + tag_2 is an existing child of tag_1. tag_1 is renamed to tag_50 and + tag_2's row keeps parent_id="tag_1", the pre-import id. The row is + "no change" for UpdateParentTag, so only the plan-level check can + reject it, and the error names the row and the new id. + """ + tags = [ + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_2', value='Tag 2', parent_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.errors), 1) + self.assertIn("Row 2's parent_id (tag_1)", str(self.import_plan.errors[0])) + self.assertIn("Use 'tag_50' as the parent_id.", str(self.import_plan.errors[0])) + + def test_generate_actions_swap_rename_child_stale_parent_id_rejected_once(self) -> None: + """ + In a tag_1 <-> tag_3 swap, a rename row (tag_2 -> tag_20) that names + the stale parent id "tag_1" is reported exactly once. The rename + action's own parent validation lets it through because "tag_1" is + also a rename row's final id, so the plan-level check reports it. + """ + tags = [ + TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_20', value='Tag 2', previous_id='tag_2', parent_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.errors), 1) + self.assertIn("Row 3's parent_id (tag_1)", str(self.import_plan.errors[0])) + + def test_generate_actions_rename_child_stale_parent_id_without_swap_not_duplicated(self) -> None: + """ + Without a swap, a rename row (tag_2 -> tag_20) that names the stale + parent id "tag_1" is reported only by the rename action's parent + validation, not a second time by the plan-level check. + """ + tags = [ + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_20', value='Tag 2', previous_id='tag_2', parent_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.errors), 1) + self.assertIn( + "Parent tag tag_1 is renamed to tag_50 in this file; use tag_50 as the parent_id.", + str(self.import_plan.errors[0]), + ) + + def test_generate_actions_case_only_parent_rename_not_flagged(self) -> None: + """ + A case-only rename of the parent (tag_1 -> TAG_1) keeps the id + resolving to the same tag, so a child that keeps parent_id="tag_1" + is not stale. + """ + tags = [ + TagItem(id='TAG_1', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_2', value='Tag 2', parent_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + + def test_generate_actions_swap_children_accepted_in_any_row_order(self) -> None: + """ + The tag_1 <-> tag_3 swap with children on end-state ids is accepted + in every one of the 24 row orders. + """ + rows = [ + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_2', value='Tag 2', parent_id='tag_3'), + TagItem(id='tag_4', value='Tag 4', parent_id='tag_1'), + ] + for order in itertools.permutations(range(4)): + with self.subTest(order=order): + self.import_plan.generate_actions(tags=[rows[i] for i in order], replace=False) + self.assertEqual(self.import_plan.errors, []) + + def test_generate_actions_swap_stale_ids_rejected_in_any_row_order(self) -> None: + """ + Children that name the stale pre-import ids are rejected in every one + of the 24 row orders, and each error reports the row's file position. + """ + rows = [ + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_2', value='Tag 2', parent_id='tag_1'), + TagItem(id='tag_4', value='Tag 4', parent_id='tag_3'), + ] + for order in itertools.permutations(range(4)): + with self.subTest(order=order): + file_rows = [rows[i] for i in order] + self.import_plan.generate_actions(tags=file_rows, replace=False) + messages = [str(error) for error in self.import_plan.errors] + self.assertEqual(len(messages), 2) + new_ids = {'tag_1': 'tag_3', 'tag_3': 'tag_1'} + for position, row in enumerate(file_rows, start=1): + if row.parent_id: + prefix = f"Row {position}'s parent_id ({row.parent_id})" + matching = [m for m in messages if m.startswith(prefix)] + self.assertEqual(len(matching), 1) + self.assertIn(f"Use '{new_ids[row.parent_id]}' as the parent_id.", matching[0]) + + def test_generate_actions_new_child_before_new_parent_accepted(self) -> None: + """ + A new child row listed before its new parent row is accepted, and the + plan creates the parent first. + """ + tags = [ + TagItem(id='tag_90', value='Tag 90', parent_id='tag_91'), + TagItem(id='tag_91', value='Tag 91'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + self.assertEqual([action.tag.id for action in self.import_plan.actions], ['tag_91', 'tag_90']) + + def test_generate_actions_valid_row_order_keeps_action_order(self) -> None: + """ + A file whose parents already come before their children keeps its + row order in the plan. + """ + tags = [ + TagItem(id='tag_91', value='Tag 91'), + TagItem(id='tag_92', value='Tag 92', parent_id='tag_91'), + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_60', value='Tag 60', parent_id='tag_50'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + self.assertEqual( + [action.tag.id for action in self.import_plan.actions], + ['tag_91', 'tag_92', 'tag_50', 'tag_60'], + ) + + def test_generate_actions_cyclic_parent_ids_rejected(self) -> None: + """ + Two new rows that name each other as parent terminate with an error. + """ + tags = [ + TagItem(id='tag_90', value='Tag 90', parent_id='tag_91'), + TagItem(id='tag_91', value='Tag 91', parent_id='tag_90'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.errors), 1) + self.assertIn("Unknown parent tag", str(self.import_plan.errors[0])) + + def test_generate_actions_deep_reversed_chain_terminates(self) -> None: + """ + A 1100-row chain of new tags listed child first is ordered without hitting the recursion + limit and every row is planned. The chain is far too deep to apply, so the plan reports it. + """ + tags = [ + TagItem(id=f'n{i}', value=f'N{i}', parent_id=f'n{i - 1}' if i else None) + for i in reversed(range(1100)) + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.actions), 1100) + self.assertEqual(self.import_plan.actions[0].tag.id, 'n0') + self.assertEqual(len(self.import_plan.errors), 1) + self.assertIn('deeper than 6 levels', str(self.import_plan.errors[0])) + + def test_plan_validations_do_not_query_per_row(self) -> None: + """ + The ordering and the plan-level validations add no queries, however many rows have a + parent_id: the plan costs the same with them replaced by no-ops. + """ + tags = [ + TagItem(id=f'n{i}', value=f'N{i}', parent_id='tag_1' if i % 2 == 0 else f'n{i - 1}') + for i in range(200) + ] + with CaptureQueriesContext(connection) as real: + self.import_plan.generate_actions(tags=tags, replace=False) + with ( + mock.patch.object(TagImportPlan, '_validate_parent_id_not_stale'), + mock.patch.object(TagImportPlan, '_validate_no_parent_cycles'), + mock.patch.object( + TagImportPlan, '_order_rows_by_parent_dependency', side_effect=lambda rows, existing: list(rows), + ), + CaptureQueriesContext(connection) as without_validations, + ): + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(real), len(without_validations)) + + def test_rows_ordered_as_definers_match_create_and_rename_applies_for(self) -> None: + """ + A row is planned before a child that names its id exactly when it is a rename row or + CreateTag.applies_for holds: existing tags (also with a different case) are not + definers, new tags (also with a different case) and rename rows are, including a + rename onto the id of an existing tag. + """ + rows = [ + TagItem(id='tag_1', value='Tag 1'), + TagItem(id='TAG_2', value='Tag 2'), + TagItem(id='tag_90', value='Tag 90'), + TagItem(id='Tag_91', value='Tag 91'), + TagItem(id='tag_50', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_4', value='Tag 1', previous_id='tag_1'), + ] + planned_first = set() + for row in rows: + child = TagItem(id='tag_95', value='Tag 95', parent_id=row.id) + self.import_plan.generate_actions(tags=[child, row], replace=False) + ids = [ + action.tag.id for action in self.import_plan.actions if action.name != 'stage_external_id' + ] + if ids.index(row.id) < ids.index('tag_95'): + planned_first.add(row.id) + expected = { + row.id for row in rows + if RenameTagExternalId.applies_for(self.taxonomy, row) or CreateTag.applies_for(self.taxonomy, row) + } + self.assertEqual(expected, {'tag_90', 'Tag_91', 'tag_50', 'tag_4'}) + self.assertEqual(planned_first, expected) + + def test_generate_actions_three_level_chain_defined_later_accepted(self) -> None: + """ + A definer whose own parent is defined later in the file is still + planned after that parent: a child, its parent and its grandparent + listed in reverse are created grandparent first. + """ + tags = [ + TagItem(id='tag_92', value='Tag 92', parent_id='tag_91'), + TagItem(id='tag_91', value='Tag 91', parent_id='tag_90'), + TagItem(id='tag_90', value='Tag 90'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + self.assertEqual( + [action.tag.id for action in self.import_plan.actions], + ['tag_90', 'tag_91', 'tag_92'], + ) + + def test_generate_actions_existing_tag_moved_under_later_new_parent_accepted(self) -> None: + """ + An existing tag moved under a new parent that is defined later in + the file is accepted, and the parent is created first. + """ + tags = [ + TagItem(id='tag_2', value='Tag 2', parent_id='tag_90'), + TagItem(id='tag_90', value='Tag 90'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + self.assertEqual(self.import_plan.actions[0].name, 'create') + self.assertEqual(self.import_plan.actions[0].tag.id, 'tag_90') + + def test_generate_actions_mixed_case_parent_id_defined_later_accepted(self) -> None: + """ + A parent_id that differs only in case from the id of a later new row + still finds that row as its definer. + """ + tags = [ + TagItem(id='tag_91', value='Tag 91', parent_id='TAG_90'), + TagItem(id='Tag_90', value='Tag 90'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + self.assertEqual(self.import_plan.actions[0].tag.id, 'Tag_90') + + def _cycle_messages(self, tags: list[TagItem]) -> list[str]: + """ + Generate the actions for `tags` and return the error messages. + """ + self.import_plan.generate_actions(tags=tags, replace=False) + return [str(error) for error in self.import_plan.errors] + + def test_generate_actions_self_parent_of_existing_tag_rejected(self) -> None: + """ + An existing tag that names itself as its parent is a cycle of length + one and is reported once. + """ + messages = self._cycle_messages([TagItem(id='tag_2', value='Tag 2', parent_id='tag_2')]) + self.assertEqual(messages, ["Row 1's parent_id (tag_2) creates a parent cycle."]) + + def test_generate_actions_existing_and_new_tag_cycle_rejected_once(self) -> None: + """ + An existing tag and a new tag naming each other as parent are rejected + with exactly one cycle error, in either row order. + """ + rows = [ + TagItem(id='tag_2', value='Tag 2', parent_id='tag_90'), + TagItem(id='tag_90', value='Tag 90', parent_id='tag_2'), + ] + for order in itertools.permutations(range(2)): + with self.subTest(order=order): + messages = self._cycle_messages([rows[i] for i in order]) + self.assertEqual(len(messages), 1) + self.assertIn("Row 1's parent_id", messages[0]) + self.assertIn("creates a parent cycle.", messages[0]) + + def test_generate_actions_cycle_through_db_only_tag_rejected(self) -> None: + """ + tag_2 is a child of tag_1 and is not in the file. A row moving tag_1 + under tag_2 closes a cycle through that DB-only tag. + """ + messages = self._cycle_messages([TagItem(id='tag_1', value='Tag 1', parent_id='tag_2')]) + self.assertEqual(messages, ["Row 1's parent_id (tag_2) creates a parent cycle."]) + + def test_generate_actions_cycle_of_definer_rows_reports_only_unknown_parent(self) -> None: + """ + A cycle made only of new-tag rows, or of a rename row and a new-tag + row, is reported by the parent validation as an unknown parent and not + a second time as a cycle. + """ + cases = { + 'new tags': [ + TagItem(id='tag_90', value='Tag 90', parent_id='tag_91'), + TagItem(id='tag_91', value='Tag 91', parent_id='tag_90'), + ], + 'rename and new tag': [ + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1', parent_id='tag_90'), + TagItem(id='tag_90', value='Tag 90', parent_id='tag_50'), + ], + } + for name, rows in cases.items(): + for order in itertools.permutations(range(2)): + with self.subTest(case=name, order=order): + messages = self._cycle_messages([rows[i] for i in order]) + self.assertEqual(len(messages), 1) + self.assertIn("Unknown parent tag", messages[0]) + + def test_generate_actions_self_parent_after_rename_onto_freed_id_rejected(self) -> None: + """ + tag_3 is renamed onto tag_1, which the same replace import frees, and names + itself as parent. The cycle is reported at the plan step. + """ + tags = [TagItem(id='tag_1', value='Tag 3', previous_id='tag_3', parent_id='tag_1')] + self.import_plan.generate_actions(tags=tags, replace=True) + messages = [str(error) for error in self.import_plan.errors] + self.assertEqual(messages, ["Row 1's parent_id (tag_1) creates a parent cycle."]) + + def test_generate_actions_child_of_deleted_holder_follows_rename_onto_its_id(self) -> None: + """ + tag_2 is a child of tag_1, which the replace import deletes while tag_3 is + renamed onto the id tag_1. Its row plans an update_parent action to tag_1, + which runs after the rename, instead of leaving it promoted to a root. + """ + tags = [ + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_2', value='Tag 2', parent_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=True) + self.assertEqual(self.import_plan.errors, []) + row_updates = [ + action for action in self.import_plan.indexed_actions['update_parent'] + if action.tag.id == 'tag_2' and action.tag.parent_id == 'tag_1' + ] + self.assertEqual(len(row_updates), 1) + self.assertGreater( + self.import_plan.actions.index(row_updates[0]), + self.import_plan.actions.index(self.import_plan.indexed_actions['rename_external_id'][0]), + ) + + def test_defines_id_is_shared_by_ordering_and_cycle_check(self) -> None: + """ + Both the ordering and the cycle check use one rule for which rows define their id: with + it patched to never define, a child stays before its late parent row and a cycle of new + tags is reported as a parent cycle instead of an unknown parent. + """ + late_parent = [ + TagItem(id='tag_91', value='Tag 91', parent_id='tag_90'), + TagItem(id='tag_90', value='Tag 90'), + ] + new_cycle = [ + TagItem(id='tag_90', value='Tag 90', parent_id='tag_91'), + TagItem(id='tag_91', value='Tag 91', parent_id='tag_90'), + ] + with mock.patch.object(TagImportPlan, '_defines_id', return_value=False): + self.import_plan.generate_actions(tags=late_parent, replace=False) + self.assertEqual(self.import_plan.actions[0].tag.id, 'tag_91') + self.import_plan.generate_actions(tags=new_cycle, replace=False) + self.assertTrue(any('creates a parent cycle.' in str(e) for e in self.import_plan.errors)) + + def _plan_for_new_taxonomy(self, name: str, structure: list[tuple[str, str | None]]) -> TagImportPlan: + """ + Create a taxonomy with the given (external_id, parent external_id) tags, in + parent-first order, and return a plan for it. Each tag's value is "Tag ". + """ + taxonomy = Taxonomy.objects.create(name=name, export_id=uuid.uuid4().hex) + tags: dict[str, Tag] = {} + for external_id, parent_id in structure: + tags[external_id] = Tag.objects.create( + taxonomy=taxonomy, + value=f'Tag {external_id}', + external_id=external_id, + parent=tags[parent_id] if parent_id else None, + ) + return TagImportPlan(taxonomy) + + def _row(self, tag_id: str, parent_id: str | None = None) -> TagItem: + """ + Build the import row for the tag created by `_plan_for_new_taxonomy`. + """ + return TagItem(id=tag_id, value=f'Tag {tag_id}', parent_id=parent_id) + + def _assert_promotions_precede_deletes(self, plan: TagImportPlan) -> None: + """ + Check every delete action comes after every promotion of a kept child to a root. + """ + names = [action.name for action in plan.actions] + deletes = [index for index, name in enumerate(names) if name == 'delete'] + self.assertTrue(deletes) + self.assertEqual(deletes, list(range(deletes[0], deletes[-1] + 1))) + self.assertNotIn('create', names[:deletes[0]]) + self.assertEqual(set(names[:deletes[0]]), {'update_parent'}) + + def test_generate_actions_kept_grandchild_of_deleted_chain_promoted_before_deletes(self) -> None: + """ + t5 is kept under t1, which is deleted along with its parent t0. Deleting t0 + would cascade to t1 and t5, so t5 is promoted to a root before any delete. + """ + plan = self._plan_for_new_taxonomy('chain', [('t0', None), ('t1', 't0'), ('t5', 't1')]) + plan.generate_actions(tags=[self._row('t5')], replace=True) + self.assertEqual(plan.errors, []) + self._assert_promotions_precede_deletes(plan) + + def test_generate_actions_kept_tags_at_two_depths_under_deleted_ancestors(self) -> None: + """ + In a five-level chain with t0, t1 and t3 deleted, the kept t2 and t4 are both + promoted before any delete. + """ + structure = [('t0', None), ('t1', 't0'), ('t2', 't1'), ('t3', 't2'), ('t4', 't3')] + plan = self._plan_for_new_taxonomy('deep', structure) + plan.generate_actions(tags=[self._row('t2'), self._row('t4')], replace=True) + self.assertEqual(plan.errors, []) + self._assert_promotions_precede_deletes(plan) + promoted = { + action.tag.id for action in plan.actions[:2] if action.name == 'update_parent' + } + self.assertEqual(promoted, {'t2', 't4'}) + + def test_generate_actions_kept_child_of_deleted_root_promoted(self) -> None: + """ + A kept child of a deleted root is promoted to a root before the delete. + """ + plan = self._plan_for_new_taxonomy('root', [('t0', None), ('t1', 't0')]) + plan.generate_actions(tags=[self._row('t1')], replace=True) + self.assertEqual(plan.errors, []) + self.assertEqual([action.name for action in plan.actions[:2]], ['update_parent', 'delete']) + + def _update_parent_index(self, plan: TagImportPlan, tag_id: str, parent_id: str | None) -> int: + """ + Return the index of the update_parent action that sets `tag_id` under `parent_id`. + """ + return next( + index for index, action in enumerate(plan.actions) + if action.name == 'update_parent' and action.tag.id == tag_id and action.tag.parent_id == parent_id + ) + + def test_generate_actions_ancestry_inversion_accepted_in_any_row_order(self) -> None: + """ + t2 is a child of t0 and becomes t0's ancestor (g0 is new, under t2). In all 24 row + orders the plan is clean and t2 moves out of t0's subtree before t0 moves under g0. + """ + rows = [ + self._row('t0', 'g0'), self._row('t1', 'g0'), self._row('t2'), self._row('g0', 't2'), + ] + plan = self._plan_for_new_taxonomy('inversion', [('t0', None), ('t1', 't0'), ('t2', 't0')]) + for order in itertools.permutations(range(4)): + with self.subTest(order=order): + plan.generate_actions(tags=[rows[i] for i in order], replace=False) + self.assertEqual(plan.errors, []) + self.assertLess( + self._update_parent_index(plan, 't2', None), self._update_parent_index(plan, 't0', 'g0'), + ) + + def test_generate_actions_parent_and_child_swap_places_in_any_row_order(self) -> None: + """ + A parent and its child swap places: the child moves to the root before the parent + moves under it, in either row order. + """ + rows = [self._row('t0', 't1'), self._row('t1')] + plan = self._plan_for_new_taxonomy('swap', [('t0', None), ('t1', 't0')]) + for order in itertools.permutations(range(2)): + with self.subTest(order=order): + plan.generate_actions(tags=[rows[i] for i in order], replace=False) + self.assertEqual(plan.errors, []) + self.assertLess( + self._update_parent_index(plan, 't1', None), self._update_parent_index(plan, 't0', 't1'), + ) + + def test_generate_actions_three_level_inversion_accepted_in_any_row_order(self) -> None: + """ + A three-level chain t0 > t1 > t2 is reversed: each move runs only after the tag it + moves under has left its subtree, in all six row orders. + """ + rows = [self._row('t2'), self._row('t1', 't2'), self._row('t0', 't1')] + plan = self._plan_for_new_taxonomy('reversal', [('t0', None), ('t1', 't0'), ('t2', 't1')]) + for order in itertools.permutations(range(3)): + with self.subTest(order=order): + plan.generate_actions(tags=[rows[i] for i in order], replace=False) + self.assertEqual(plan.errors, []) + moves = [ + self._update_parent_index(plan, 't2', None), + self._update_parent_index(plan, 't1', 't2'), + self._update_parent_index(plan, 't0', 't1'), + ] + self.assertEqual(moves, sorted(moves)) + + def test_generate_actions_valid_move_order_keeps_action_order(self) -> None: + """ + A file whose parent moves are already in a valid order keeps its row order in the plan. + """ + plan = self._plan_for_new_taxonomy('valid', [('t0', None), ('t1', 't0'), ('t2', 't0')]) + rows = [self._row('t2'), self._row('g0', 't2'), self._row('t0', 'g0'), self._row('t1', 'g0')] + plan.generate_actions(tags=rows, replace=False) + self.assertEqual(plan.errors, []) + self.assertEqual([action.tag.id for action in plan.actions], ['t2', 'g0', 't0', 't1']) + + def _deep_chain_structure(self) -> list[tuple[str, str | None]]: + """ + Return a chain t0 > t1 > t2 > t3 > t5 of five levels, the deepest the taxonomy allows + before a child of t5 would be a sixth level (parent depth 4, so the tag below it is 5). + """ + return [('t0', None), ('t1', 't0'), ('t2', 't1'), ('t3', 't2'), ('t5', 't3')] + + def _passes_dry_run(self, plan: TagImportPlan) -> bool: + """ + Return whether the plan's actions pass the parent dry run, on the taxonomy as it is now. + """ + forest = _ParentForest({ + tag.external_id.casefold(): ExistingTag( + tag.pk, tag.parent_id, tag.parent.external_id if tag.parent else None, + ) + for tag in plan.taxonomy.tag_set.select_related('parent') + }) + return all(forest.apply(action) for action in plan.actions) + + def _assert_detached_first(self, plan: TagImportPlan, tag_id: str) -> None: + """ + Check the plan starts with the update-to-root of `tag_id`, before any other action + of its rows, and that the plan passes the dry run. + """ + first = plan.actions[0] + self.assertEqual((first.name, first.tag.id, first.tag.parent_id), ('update_parent', tag_id, None)) + self.assertTrue(self._passes_dry_run(plan)) + + def test_generate_actions_deep_parent_move_detaches_before_attaching(self) -> None: + """ + t5 is moved to the root while new tags g0 and g1 are put under it. Creating them + under t5 before it moves would make g1 a seventh level. All six row orders plan clean, + and the hazardous one (new tags first) detaches t5 before any create. + """ + rows = [self._row('g1', 'g0'), self._row('g0', 't5'), self._row('t5')] + plan = self._plan_for_new_taxonomy('hazard', self._deep_chain_structure()) + for order in itertools.permutations(range(3)): + with self.subTest(order=order): + plan.generate_actions(tags=[rows[i] for i in order], replace=False) + self.assertEqual(plan.errors, []) + self.assertTrue(self._passes_dry_run(plan)) + plan.generate_actions(tags=rows, replace=False) + self._assert_detached_first(plan, 't5') + + def test_generate_actions_ordinary_import_has_no_detach_actions(self) -> None: + """ + An import whose steps are all valid keeps its plan: no update-to-root is added. + """ + plan = self._plan_for_new_taxonomy('ordinary', self._deep_chain_structure()[:3]) + plan.generate_actions( + tags=[self._row('t0'), self._row('t1', 't0'), self._row('t2', 't1'), self._row('n0', 't2')], + replace=False, + ) + self.assertEqual(plan.errors, []) + self.assertEqual( + [action.name for action in plan.actions], ['without_changes'] * 3 + ['create'], + ) + + def test_generate_actions_deep_tag_under_renamed_parent_moved_to_root(self) -> None: + """ + t5's parent t3 is renamed to r3 while t5 moves to the root and new tags go under it: + all 24 row orders plan clean, and the hazardous one detaches t5 first. + """ + rows = [ + self._row('g1', 'g0'), self._row('g0', 't5'), self._row('t5'), + TagItem(id='r3', value='Tag t3', previous_id='t3', parent_id='t2'), + ] + plan = self._plan_for_new_taxonomy('renamed', self._deep_chain_structure()) + for order in itertools.permutations(range(4)): + with self.subTest(order=order): + plan.generate_actions(tags=[rows[i] for i in order], replace=False) + self.assertEqual(plan.errors, []) + self.assertTrue(self._passes_dry_run(plan)) + plan.generate_actions(tags=rows, replace=False) + self._assert_detached_first(plan, 't5') + + def test_generate_actions_subtree_moved_under_deep_tag_before_it_is_shallowed(self) -> None: + """ + A three-level subtree s0 > s1 > s2 is moved under c3, which is itself moved to the root. + Attaching s0 under c3 while c3 is still at depth 3 would make s2 a seventh level. + """ + structure = [('c0', None), ('c1', 'c0'), ('c2', 'c1'), ('c3', 'c2'), ('s0', None), ('s1', 's0'), ('s2', 's1')] + rows = [self._row('s0', 'c3'), self._row('c3'), self._row('s1', 's0'), self._row('s2', 's1')] + plan = self._plan_for_new_taxonomy('subtree', structure) + for order in itertools.permutations(range(len(rows))): + with self.subTest(order=order): + plan.generate_actions(tags=[rows[i] for i in order], replace=False) + self.assertEqual(plan.errors, []) + self.assertTrue(self._passes_dry_run(plan)) + + def test_generate_actions_three_level_new_chain_under_moved_deep_tag(self) -> None: + """ + Three new tags are chained under t5, which moves to the root: all 24 row orders plan clean. + """ + rows = [self._row('t5'), self._row('g0', 't5'), self._row('g1', 'g0'), self._row('g2', 'g1')] + plan = self._plan_for_new_taxonomy('three', self._deep_chain_structure()) + for order in itertools.permutations(range(4)): + with self.subTest(order=order): + plan.generate_actions(tags=[rows[i] for i in order], replace=False) + self.assertEqual(plan.errors, []) + self.assertTrue(self._passes_dry_run(plan)) + + def test_parent_dry_run_accepts_valid_and_rejects_invalid_sequences(self) -> None: + """ + The dry run accepts a valid move, and rejects a cycle, a tree that gets too deep, and a + reference to a tag removed by an earlier delete (a delete also removes descendants). + """ + # A chain c0 > ... > c4 of five levels, and a root s0 with a child s1. + tags = { + 'c0': ExistingTag(1, None, None), 'c1': ExistingTag(2, 1, 'c0'), 'c2': ExistingTag(3, 2, 'c1'), + 'c3': ExistingTag(4, 3, 'c2'), 'c4': ExistingTag(5, 4, 'c3'), + 's0': ExistingTag(6, None, None), 's1': ExistingTag(7, 6, 's0'), + } + taxonomy = Taxonomy() + + def update(tag_id: str, parent_id: str | None) -> UpdateParentTag: + return UpdateParentTag(taxonomy, TagItem(id=tag_id, value='x', parent_id=parent_id), 1) + + delete = DeleteTag(taxonomy, TagItem(id='c1', value='x'), 1) + self.assertTrue(_ParentForest(tags).apply(update('s1', 'c0'))) + self.assertFalse(_ParentForest(tags).apply(update('c0', 'c4'))) + self.assertFalse(_ParentForest(tags).apply(update('s0', 'c4'))) + forest = _ParentForest(tags) + self.assertTrue(forest.apply(delete)) + self.assertFalse(forest.apply(update('s0', 'c2'))) + forest = _ParentForest(tags) + self.assertTrue(forest.apply(delete)) + self.assertTrue(forest.apply(update('s0', 'c0'))) + + def test_generate_actions_rebuilds_a_hazardous_plan_only_once(self) -> None: + """ + When the dry run keeps failing, the plan is rebuilt detach-first once, checked once more, + and reported as invalid instead of being rebuilt again. + """ + plan = self._plan_for_new_taxonomy('once', self._deep_chain_structure()) + rows = [self._row('g0', 't5'), self._row('t5')] + with mock.patch.object(TagImportPlan, '_parent_changes_are_valid', return_value=False) as dry_run: + plan.generate_actions(tags=rows, replace=False) + self.assertEqual(dry_run.call_count, 2) + self.assertEqual(len(plan.errors), 1) + first = plan.actions[0] + self.assertEqual((first.name, first.tag.id, first.tag.parent_id), ('update_parent', 't5', None)) + + def test_generate_actions_rejects_a_new_tag_below_the_maximum_depth(self) -> None: + """ + A tag added under a tag that is already at the deepest level cannot be applied in any + order, so the plan has the invalid hierarchy error instead of an applicable plan. + """ + structure = [('a', None), ('b', 'a'), ('c', 'b'), ('d', 'c'), ('e', 'd'), ('f', 'e')] + plan = self._plan_for_new_taxonomy('too deep', structure) + rows = [self._row(tag_id, parent) for tag_id, parent in structure] + [self._row('g', 'f')] + plan.generate_actions(tags=rows, replace=True) + self.assertEqual(len(plan.errors), 1) + self.assertIn('deeper than 6 levels', str(plan.errors[0])) + + def test_generate_actions_rejects_a_subtree_moved_too_deep(self) -> None: + """ + Moving the chain w > x > y > z under d (a > b > c > d) would put z at the seventh level, + so the plan has the invalid hierarchy error. + """ + structure = [('a', None), ('b', 'a'), ('c', 'b'), ('d', 'c'), ('w', None), ('x', 'w'), ('y', 'x'), ('z', 'y')] + plan = self._plan_for_new_taxonomy('moved too deep', structure) + rows = [self._row(tag_id, 'd' if tag_id == 'w' else parent) for tag_id, parent in structure] + plan.generate_actions(tags=rows, replace=True) + self.assertEqual(len(plan.errors), 1) + self.assertIn('deeper than 6 levels', str(plan.errors[0])) + + def test_generate_actions_swap_case_variant_of_stale_parent_id_rejected(self) -> None: + """ + In a tag_1 <-> tag_3 swap, a child naming the stale id "tag_1" with another case is + rejected like the exact spelling, with its row position and the new id. + """ + tags = [ + TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_2', value='Tag 2', parent_id='TAG_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + messages = [str(error) for error in self.import_plan.errors] + self.assertEqual(len(messages), 1) + self.assertIn("Row 3's parent_id (TAG_1)", messages[0]) + self.assertIn("Use 'tag_3' as the parent_id.", messages[0]) + + def test_generate_actions_swap_case_variant_of_end_state_parent_id_accepted(self) -> None: + """ + In the same swap, children naming the end-state ids with another case are accepted. + """ + tags = [ + TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_2', value='Tag 2', parent_id='TAG_3'), + TagItem(id='tag_4', value='Tag 4', parent_id='TAG_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + + def test_generate_actions_plain_rename_case_variant_of_parent_id_reported_once(self) -> None: + """ + When tag_1 is plainly renamed to tag_50, a child naming "TAG_1" gets only the parent + validation error, not a second one for the same row. + """ + tags = [ + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_2', value='Tag 2', parent_id='TAG_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + messages = [str(error) for error in self.import_plan.errors] + self.assertEqual(len(messages), 1) + self.assertIn('Parent tag TAG_1 is renamed to tag_50 in this file', messages[0]) + + def test_generate_actions_case_only_parent_rename_with_case_variant_child_accepted(self) -> None: + """ + Renaming tag_1 only by case keeps the id, so a child naming it in either case is accepted. + """ + for parent_id in ('tag_1', 'Tag_1', 'TAG_1'): + with self.subTest(parent_id=parent_id): + tags = [ + TagItem(id='TAG_1', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_2', value='Tag 2', parent_id=parent_id), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + def test_generate_actions_genuine_collision_not_staged(self) -> None: """ A rename targeting an id held by an unrelated tag that is not From c30ab5c8971b2d9ad74187c7cf2fa672563d114a Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Tue, 13 Oct 2026 18:18:35 +0300 Subject: [PATCH 2/5] test: fix mypy error and taxonomy id assumption in tag import tests CI ran `mypy` over the tests and rejected a `list[tuple[int, ...]]` parameter, because list is invariant. Use `Sequence`. `test_get_taxonomy` asserted that taxonomy id 200 does not exist. On MySQL, auto-increment ids are not rolled back with the per-test transaction, so the new import tests created enough taxonomies for a later test to receive id 200. The test now uses an id one above the largest existing one, and the new tests create their taxonomy once per class instead of once per test or per plan, so they create fewer taxonomies than before. Co-Authored-By: Claude Sonnet 5.5 --- .../openedx_tagging/import_export/test_api.py | 11 ++-- .../import_export/test_import_plan.py | 50 +++++++++++-------- tests/openedx_tagging/test_api.py | 3 +- 3 files changed, 37 insertions(+), 27 deletions(-) diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index 8b5c1983a..11cf8952f 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -4,6 +4,7 @@ import csv import itertools import json +from collections.abc import Sequence from contextlib import contextmanager from io import BytesIO, StringIO @@ -1480,10 +1481,12 @@ class TestImportAcceptanceScenarios(TestCase): """ formats = (ParserFormat.JSON, ParserFormat.CSV) + taxonomy: Taxonomy - def setUp(self) -> None: - super().setUp() - self.taxonomy = Taxonomy.objects.create(name="Competencies") + @classmethod + def setUpTestData(cls) -> None: + super().setUpTestData() + cls.taxonomy = Taxonomy.objects.create(name="Competencies") def _render(self, rows: list[dict], parser_format: ParserFormat) -> BytesIO: """ @@ -1974,7 +1977,7 @@ def _assert_tree_after_import( rows: list[dict], expected_parents: dict[str, str | None], pks: dict[str, int], - orders: list[tuple[int, ...]], + orders: Sequence[tuple[int, ...]], parser_formats: tuple[ParserFormat, ...], *, static_rows: tuple[dict, ...] = (), diff --git a/tests/openedx_tagging/import_export/test_import_plan.py b/tests/openedx_tagging/import_export/test_import_plan.py index 4aa9cfdce..88b504f75 100644 --- a/tests/openedx_tagging/import_export/test_import_plan.py +++ b/tests/openedx_tagging/import_export/test_import_plan.py @@ -2,7 +2,6 @@ Test for import_plan functions """ import itertools -import uuid from unittest import mock import ddt # type: ignore[import] @@ -24,6 +23,13 @@ class TestTagImportPlan(TestImportActionMixin, TestCase): Test for import plan functions """ + structure_taxonomy: Taxonomy + + @classmethod + def setUpTestData(cls) -> None: + super().setUpTestData() + cls.structure_taxonomy = Taxonomy.objects.create(name="Structure taxonomy") + def setUp(self) -> None: super().setUp() self.import_plan = TagImportPlan(self.taxonomy) @@ -999,21 +1005,21 @@ def test_defines_id_is_shared_by_ordering_and_cycle_check(self) -> None: self.import_plan.generate_actions(tags=new_cycle, replace=False) self.assertTrue(any('creates a parent cycle.' in str(e) for e in self.import_plan.errors)) - def _plan_for_new_taxonomy(self, name: str, structure: list[tuple[str, str | None]]) -> TagImportPlan: + def _plan_for_new_taxonomy(self, structure: list[tuple[str, str | None]]) -> TagImportPlan: """ - Create a taxonomy with the given (external_id, parent external_id) tags, in - parent-first order, and return a plan for it. Each tag's value is "Tag ". + Create the given (external_id, parent external_id) tags, in parent-first order, in an + empty taxonomy shared by the class, and return a plan for it. Each tag's value is + "Tag ". A test calls this once, and its tags are rolled back with the test. """ - taxonomy = Taxonomy.objects.create(name=name, export_id=uuid.uuid4().hex) tags: dict[str, Tag] = {} for external_id, parent_id in structure: tags[external_id] = Tag.objects.create( - taxonomy=taxonomy, + taxonomy=self.structure_taxonomy, value=f'Tag {external_id}', external_id=external_id, parent=tags[parent_id] if parent_id else None, ) - return TagImportPlan(taxonomy) + return TagImportPlan(self.structure_taxonomy) def _row(self, tag_id: str, parent_id: str | None = None) -> TagItem: """ @@ -1037,7 +1043,7 @@ def test_generate_actions_kept_grandchild_of_deleted_chain_promoted_before_delet t5 is kept under t1, which is deleted along with its parent t0. Deleting t0 would cascade to t1 and t5, so t5 is promoted to a root before any delete. """ - plan = self._plan_for_new_taxonomy('chain', [('t0', None), ('t1', 't0'), ('t5', 't1')]) + plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0'), ('t5', 't1')]) plan.generate_actions(tags=[self._row('t5')], replace=True) self.assertEqual(plan.errors, []) self._assert_promotions_precede_deletes(plan) @@ -1048,7 +1054,7 @@ def test_generate_actions_kept_tags_at_two_depths_under_deleted_ancestors(self) promoted before any delete. """ structure = [('t0', None), ('t1', 't0'), ('t2', 't1'), ('t3', 't2'), ('t4', 't3')] - plan = self._plan_for_new_taxonomy('deep', structure) + plan = self._plan_for_new_taxonomy(structure) plan.generate_actions(tags=[self._row('t2'), self._row('t4')], replace=True) self.assertEqual(plan.errors, []) self._assert_promotions_precede_deletes(plan) @@ -1061,7 +1067,7 @@ def test_generate_actions_kept_child_of_deleted_root_promoted(self) -> None: """ A kept child of a deleted root is promoted to a root before the delete. """ - plan = self._plan_for_new_taxonomy('root', [('t0', None), ('t1', 't0')]) + plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0')]) plan.generate_actions(tags=[self._row('t1')], replace=True) self.assertEqual(plan.errors, []) self.assertEqual([action.name for action in plan.actions[:2]], ['update_parent', 'delete']) @@ -1083,7 +1089,7 @@ def test_generate_actions_ancestry_inversion_accepted_in_any_row_order(self) -> rows = [ self._row('t0', 'g0'), self._row('t1', 'g0'), self._row('t2'), self._row('g0', 't2'), ] - plan = self._plan_for_new_taxonomy('inversion', [('t0', None), ('t1', 't0'), ('t2', 't0')]) + plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0'), ('t2', 't0')]) for order in itertools.permutations(range(4)): with self.subTest(order=order): plan.generate_actions(tags=[rows[i] for i in order], replace=False) @@ -1098,7 +1104,7 @@ def test_generate_actions_parent_and_child_swap_places_in_any_row_order(self) -> moves under it, in either row order. """ rows = [self._row('t0', 't1'), self._row('t1')] - plan = self._plan_for_new_taxonomy('swap', [('t0', None), ('t1', 't0')]) + plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0')]) for order in itertools.permutations(range(2)): with self.subTest(order=order): plan.generate_actions(tags=[rows[i] for i in order], replace=False) @@ -1113,7 +1119,7 @@ def test_generate_actions_three_level_inversion_accepted_in_any_row_order(self) moves under has left its subtree, in all six row orders. """ rows = [self._row('t2'), self._row('t1', 't2'), self._row('t0', 't1')] - plan = self._plan_for_new_taxonomy('reversal', [('t0', None), ('t1', 't0'), ('t2', 't1')]) + plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0'), ('t2', 't1')]) for order in itertools.permutations(range(3)): with self.subTest(order=order): plan.generate_actions(tags=[rows[i] for i in order], replace=False) @@ -1129,7 +1135,7 @@ def test_generate_actions_valid_move_order_keeps_action_order(self) -> None: """ A file whose parent moves are already in a valid order keeps its row order in the plan. """ - plan = self._plan_for_new_taxonomy('valid', [('t0', None), ('t1', 't0'), ('t2', 't0')]) + plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0'), ('t2', 't0')]) rows = [self._row('t2'), self._row('g0', 't2'), self._row('t0', 'g0'), self._row('t1', 'g0')] plan.generate_actions(tags=rows, replace=False) self.assertEqual(plan.errors, []) @@ -1170,7 +1176,7 @@ def test_generate_actions_deep_parent_move_detaches_before_attaching(self) -> No and the hazardous one (new tags first) detaches t5 before any create. """ rows = [self._row('g1', 'g0'), self._row('g0', 't5'), self._row('t5')] - plan = self._plan_for_new_taxonomy('hazard', self._deep_chain_structure()) + plan = self._plan_for_new_taxonomy(self._deep_chain_structure()) for order in itertools.permutations(range(3)): with self.subTest(order=order): plan.generate_actions(tags=[rows[i] for i in order], replace=False) @@ -1183,7 +1189,7 @@ def test_generate_actions_ordinary_import_has_no_detach_actions(self) -> None: """ An import whose steps are all valid keeps its plan: no update-to-root is added. """ - plan = self._plan_for_new_taxonomy('ordinary', self._deep_chain_structure()[:3]) + plan = self._plan_for_new_taxonomy(self._deep_chain_structure()[:3]) plan.generate_actions( tags=[self._row('t0'), self._row('t1', 't0'), self._row('t2', 't1'), self._row('n0', 't2')], replace=False, @@ -1202,7 +1208,7 @@ def test_generate_actions_deep_tag_under_renamed_parent_moved_to_root(self) -> N self._row('g1', 'g0'), self._row('g0', 't5'), self._row('t5'), TagItem(id='r3', value='Tag t3', previous_id='t3', parent_id='t2'), ] - plan = self._plan_for_new_taxonomy('renamed', self._deep_chain_structure()) + plan = self._plan_for_new_taxonomy(self._deep_chain_structure()) for order in itertools.permutations(range(4)): with self.subTest(order=order): plan.generate_actions(tags=[rows[i] for i in order], replace=False) @@ -1218,7 +1224,7 @@ def test_generate_actions_subtree_moved_under_deep_tag_before_it_is_shallowed(se """ structure = [('c0', None), ('c1', 'c0'), ('c2', 'c1'), ('c3', 'c2'), ('s0', None), ('s1', 's0'), ('s2', 's1')] rows = [self._row('s0', 'c3'), self._row('c3'), self._row('s1', 's0'), self._row('s2', 's1')] - plan = self._plan_for_new_taxonomy('subtree', structure) + plan = self._plan_for_new_taxonomy(structure) for order in itertools.permutations(range(len(rows))): with self.subTest(order=order): plan.generate_actions(tags=[rows[i] for i in order], replace=False) @@ -1230,7 +1236,7 @@ def test_generate_actions_three_level_new_chain_under_moved_deep_tag(self) -> No Three new tags are chained under t5, which moves to the root: all 24 row orders plan clean. """ rows = [self._row('t5'), self._row('g0', 't5'), self._row('g1', 'g0'), self._row('g2', 'g1')] - plan = self._plan_for_new_taxonomy('three', self._deep_chain_structure()) + plan = self._plan_for_new_taxonomy(self._deep_chain_structure()) for order in itertools.permutations(range(4)): with self.subTest(order=order): plan.generate_actions(tags=[rows[i] for i in order], replace=False) @@ -1269,7 +1275,7 @@ def test_generate_actions_rebuilds_a_hazardous_plan_only_once(self) -> None: When the dry run keeps failing, the plan is rebuilt detach-first once, checked once more, and reported as invalid instead of being rebuilt again. """ - plan = self._plan_for_new_taxonomy('once', self._deep_chain_structure()) + plan = self._plan_for_new_taxonomy(self._deep_chain_structure()) rows = [self._row('g0', 't5'), self._row('t5')] with mock.patch.object(TagImportPlan, '_parent_changes_are_valid', return_value=False) as dry_run: plan.generate_actions(tags=rows, replace=False) @@ -1284,7 +1290,7 @@ def test_generate_actions_rejects_a_new_tag_below_the_maximum_depth(self) -> Non order, so the plan has the invalid hierarchy error instead of an applicable plan. """ structure = [('a', None), ('b', 'a'), ('c', 'b'), ('d', 'c'), ('e', 'd'), ('f', 'e')] - plan = self._plan_for_new_taxonomy('too deep', structure) + plan = self._plan_for_new_taxonomy(structure) rows = [self._row(tag_id, parent) for tag_id, parent in structure] + [self._row('g', 'f')] plan.generate_actions(tags=rows, replace=True) self.assertEqual(len(plan.errors), 1) @@ -1296,7 +1302,7 @@ def test_generate_actions_rejects_a_subtree_moved_too_deep(self) -> None: so the plan has the invalid hierarchy error. """ structure = [('a', None), ('b', 'a'), ('c', 'b'), ('d', 'c'), ('w', None), ('x', 'w'), ('y', 'x'), ('z', 'y')] - plan = self._plan_for_new_taxonomy('moved too deep', structure) + plan = self._plan_for_new_taxonomy(structure) rows = [self._row(tag_id, 'd' if tag_id == 'w' else parent) for tag_id, parent in structure] plan.generate_actions(tags=rows, replace=True) self.assertEqual(len(plan.errors), 1) diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index a2a8a8e2a..ee403b36c 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -67,7 +67,8 @@ def test_create_taxonomy_without_export_id(self) -> None: def test_get_taxonomy(self) -> None: tax1 = tagging_api.get_taxonomy(1) assert tax1 == self.taxonomy - no_tax = tagging_api.get_taxonomy(200) + unused_id = (Taxonomy.objects.order_by("-pk").values_list("pk", flat=True).first() or 0) + 1 + no_tax = tagging_api.get_taxonomy(unused_id) assert no_tax is None def test_get_taxonomy_by_export_id(self) -> None: From cb13974895130ce9b55ee4c060a670cf22f35cb1 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 14 Oct 2026 17:16:22 +0300 Subject: [PATCH 3/5] docs: explain the stale parent_id swap ambiguity and define terms The plan-level validations used terms such as "renamed away", "stale", "definer row" and "detach-first" without defining them, and the stale-parent check packed several cases into one condition. Define the terms once in the TagImportPlan docstring, explain the swap ambiguity with a concrete example, say what "Row N" means for the admin in a CSV and a JSON file, split the conditions into named booleans, and rename the private `safe` flag to `detach_first`. Behavior is unchanged. Co-Authored-By: Claude Opus 5.5 --- .../import_export/exceptions.py | 13 +- .../import_export/import_plan.py | 200 +++++++++++------- 2 files changed, 131 insertions(+), 82 deletions(-) diff --git a/src/openedx_tagging/import_export/exceptions.py b/src/openedx_tagging/import_export/exceptions.py index fddbbcd12..ee7c3619c 100644 --- a/src/openedx_tagging/import_export/exceptions.py +++ b/src/openedx_tagging/import_export/exceptions.py @@ -169,8 +169,10 @@ class ParentIdRenamedAwayError(TagImportError): Exception raised when a row's parent_id is its current parent's pre-import id, and the same import renames that parent to a different id. - The id is stale or, after a swap, ambiguous (it may also name the tag that - takes it over), so the importer asks for the parent's new id instead. + After the import that id names no tag or, after a swap of ids, a different + tag, so the message asks for the parent's new id instead. ``row`` is the + 1-based position of the row among the file's data rows: row 1 is the first + row after a CSV header, or the first entry of a JSON file's ``tags``. """ def __init__(self, row: int, parent_id: str, new_id: str, **kargs): @@ -185,6 +187,8 @@ class ParentCycleError(TagImportError): """ Exception raised when the parent_id of a row makes the tag its own ancestor once the import is applied. + + ``row`` is numbered like in ``ParentIdRenamedAwayError``. """ def __init__(self, row: int, parent_id: str, **kargs): @@ -196,8 +200,9 @@ def __init__(self, row: int, parent_id: str, **kargs): class InvalidParentHierarchyError(TagImportError): """ - Exception raised when applying the import, even with its parent moves ordered - safely, would build a tag tree that is too deep or otherwise invalid. + Exception raised when applying the import would build a tag tree that is too + deep or otherwise invalid, even when the importer first moves every tag whose + parent changes to the root. """ def __init__(self, max_levels: int, **kargs): diff --git a/src/openedx_tagging/import_export/import_plan.py b/src/openedx_tagging/import_export/import_plan.py index 0d035e2cc..95157688a 100644 --- a/src/openedx_tagging/import_export/import_plan.py +++ b/src/openedx_tagging/import_export/import_plan.py @@ -66,7 +66,8 @@ def __str__(self): class _ParentForest: """ - In-memory copy of a taxonomy's parent links, used to dry-run the parent effects of import actions. + In-memory copy of a taxonomy's parent links, used to simulate the parent effects of import + actions without writing to the database. Tags are identified by pk, and new tags get negative ids. Tags are looked up by their casefolded external_id, which follows renames. @@ -139,8 +140,8 @@ def _delete(self, key: str) -> None: """ Remove the tag with external_id ``key`` and all its descendants, if it exists. """ - doomed = self.node_of.get(key) - pending = [doomed] if doomed is not None else [] + deleted_node = self.node_of.get(key) + pending = [deleted_node] if deleted_node is not None else [] while pending: node = pending.pop() pending.extend(self.children.pop(node, ())) @@ -157,6 +158,7 @@ def apply(self, action: ImportAction) -> bool: if action.name == "stage_external_id": if action.target_pk not in self.parent: return False + # A staged tag holds a temporary external_id until its rename runs. self._rekey(action.target_pk, f"staged:{action.target_pk}") elif action.name == "delete": self._delete(tag.id.casefold()) @@ -184,6 +186,19 @@ def apply(self, action: ImportAction) -> bool: class TagImportPlan: """ Class with functions to build an import plan and excute the plan + + Terms used by the plan-level validations below: + + - A *rename row* has a ``previous_id`` that differs from its ``id``. It renames the tag + that holds ``previous_id`` to ``id``. + - A tag is *renamed away* from an id when a rename row gives it a different id. Its + pre-import id is then *stale*: after the import that id names no tag, or, after a + swap of ids, a different tag. + - The *end state* is the taxonomy as it will be once the whole import is applied. A + row's ``parent_id`` names its parent's id in the end state. + - "Row N" in an error is the N-th data row of the file, counting from 1. 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). In a JSON file it is the N-th entry of ``tags``. """ actions: list[ImportAction] @@ -306,7 +321,8 @@ def _validate_no_duplicate_final_ids(self, tags: list[TagItem]) -> None: Rows are identified by their 1-based position in `tags`, not `TagItem.index`: index is parser-assigned and optional, and left at its default for hand-built rows (e.g. in tests), while position - is always defined. + is always defined. For the admin, that position is "Row N" as the + class docstring defines it. """ positions_by_id: dict[str, list[int]] = {} display_id_by_key: dict[str, str] = {} @@ -389,16 +405,22 @@ def _existing_tags(self) -> ExistingTags: def _validate_parent_id_not_stale(self, tags: list[TagItem], existing: ExistingTags) -> None: """ - Reject a row whose ``parent_id`` is its current parent's pre-import id - when this import renames that parent to another id. + Reject a row whose ``parent_id`` is the stale id of the row's current parent, + that is, the parent's pre-import id when this import renames that parent away. + + A plain rename makes the id meaningless. A swap makes it ambiguous. For example, + before the import ``tag_2`` is under ``tag_1``, and the file swaps the ids of + ``tag_1`` and ``tag_3``. After the import the id ``tag_1`` belongs to the tag that + was ``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``". The row is + rejected, and the error tells the admin to use ``tag_3``, the parent's new id. + A child that names the end-state id is accepted: ``tag_4``, under the old + ``tag_3``, may say ``parent_id: tag_1``. - The error names the parent's new id. A case-only rename is not - flagged. After a swap, a child naming the stale id of a swapped parent - is rejected, and one naming the end-state id is accepted. A case - variant of the stale id counts too when a rename row claims that id, - because ids are case-insensitive. Rows are reported by 1-based - position in the parsed row list, not a file line. Must run after - ``_build_staging_actions``, which fills ``indexed_actions["_vacated_pks"]``. + A rename that only changes the parent's capitalization is not flagged. A case + variant of the stale id (``TAG_1`` for ``tag_1``) is rejected like the exact + spelling. Must run after ``_build_staging_actions``, which fills + ``indexed_actions["_vacated_pks"]`` with the new id of every renamed tag. :param tags: the rows of the import file, in file order. :param existing: the taxonomy's tags, from ``_existing_tags``. @@ -413,25 +435,29 @@ def _validate_parent_id_not_stale(self, tags: list[TagItem], existing: ExistingT for position, tag in enumerate(tags, start=1): if not tag.parent_id: continue - current = existing.get((tag.previous_id or tag.id).casefold()) + existing_tag = existing.get((tag.previous_id or tag.id).casefold()) + if existing_tag is None: + continue # A new tag has no current parent. parent_key = tag.parent_id.casefold() - if current is None or (current.parent_external_id or "").casefold() != parent_key: - continue - # An exact match is a "no change" row for UpdateParentTag.applies_for, which skips - # _validate_parent. A case variant reaches _validate_parent as a parent change and - # is reported there, except when a rename row claims the id: it then passes there. - if current.parent_external_id != tag.parent_id and parent_key not in rename_final_ids: - continue - new_id = vacated_pks.get(current.parent_pk) if current.parent_pk is not None else None - is_rename = bool(tag.previous_id) and tag.id != tag.previous_id - # A rename row's own validation already reports a stale parent_id, - # except in a swap, where the id is also another rename's final id. - if ( - new_id is not None - and new_id.casefold() != parent_key - and (not is_rename or parent_key in rename_final_ids) - ): - self.errors.append(ParentIdRenamedAwayError(position, tag.parent_id, new_id)) + names_current_parent = (existing_tag.parent_external_id or "").casefold() == parent_key + if not names_current_parent: + continue # A parent change, which _validate_parent checks. + new_parent_id = vacated_pks.get(existing_tag.parent_pk) if existing_tag.parent_pk is not None else None + if new_parent_id is None or new_parent_id.casefold() == parent_key: + continue # The parent keeps its id, or only changes its capitalization. + + # With the exact spelling, UpdateParentTag.applies_for sees "no change" and + # _validate_parent never runs, so only this check can catch the row. A case + # variant is a parent change for UpdateParentTag, and _validate_parent reports + # it, except when a rename row claims the id: _validate_parent then accepts it. + is_exact_spelling = existing_tag.parent_external_id == tag.parent_id + is_claimed_by_rename = parent_key in rename_final_ids + # A rename row's own validation reports a stale parent_id, except in a swap, + # where the stale id is also another rename's final id. + is_rename_row = bool(tag.previous_id) and tag.id != tag.previous_id + is_reported_elsewhere = is_rename_row and not is_claimed_by_rename + if (is_exact_spelling or is_claimed_by_rename) and not is_reported_elsewhere: + self.errors.append(ParentIdRenamedAwayError(position, tag.parent_id, new_parent_id)) def _end_state_parents( self, tags: list[TagItem], existing: ExistingTags, @@ -466,10 +492,12 @@ def _end_state_parents( parent_of[key] = tag.parent_id.casefold() for key, tag_info in existing.items(): parent_id = tag_info.parent_external_id - if key in represented or key in deleted_ids or tag_info.parent_pk is None or parent_id is None: - continue + if key in represented or key in deleted_ids: + continue # The tag's row gives its parent, or the tag is deleted. + if tag_info.parent_pk is None or parent_id is None: + continue # A root tag. if parent_id.casefold() in deleted_ids: - continue + continue # The delete phase promotes the tag to root. parent_of[key] = vacated_pks.get(tag_info.parent_pk, parent_id).casefold() return parent_of, row_position @@ -479,16 +507,17 @@ def _validate_no_parent_cycles(self, tags: list[TagItem], existing: ExistingTags self-parent is a cycle of length one) once this import is applied. The check runs on the end-state graph from ``_end_state_parents``. A cycle - is reported once, on its first row in the parsed row list. - A cycle of definer rows is skipped only when none of its ids is held by - an existing tag, because only then does ``_validate_parent`` report it - (as an unknown parent). Must run after ``_build_staging_actions``. + is reported once, on its first row in the file. A cycle made only of rows + that define their id (see ``_defines_id``) and whose ids no existing tag + holds is skipped: ``_validate_parent`` already reports it as an unknown + parent, because none of those ids exists in the database yet. + Must run after ``_build_staging_actions``. :param tags: the rows of the import file, in file order. :param existing: the taxonomy's tags, from ``_existing_tags``. """ parent_of, row_position = self._end_state_parents(tags, existing) - is_definer_row = {key: self._defines_id(tags[position], existing) for key, position in row_position.items()} + row_defines_id = {key: self._defines_id(tags[position], existing) for key, position in row_position.items()} state: dict[str, int] = {} # 1 on the current path, 2 once fully walked for tag in tags: @@ -501,12 +530,12 @@ def _validate_no_parent_cycles(self, tags: list[TagItem], existing: ExistingTags if node is not None and state[node] == 1: cycle = path[path.index(node):] rows = [row_position[key] for key in cycle if key in row_position] - # _validate_parent only reports a cycle of definer rows when no - # id in it is held by an existing tag (it finds that one in the DB). - if rows and ( - not all(is_definer_row.get(key, False) for key in cycle) - or any(key in existing for key in cycle) - ): + # A cycle made only of ids that no existing tag holds. + is_reported_as_unknown_parent = ( + all(row_defines_id.get(key, False) for key in cycle) + and not any(key in existing for key in cycle) + ) + if rows and not is_reported_as_unknown_parent: position = min(rows) self.errors.append(ParentCycleError(position + 1, tags[position].parent_id or "")) for key in path: @@ -515,8 +544,9 @@ def _validate_no_parent_cycles(self, tags: list[TagItem], existing: ExistingTags @staticmethod def _defines_id(tag: TagItem, existing: ExistingTags) -> bool: """ - Return whether the row defines its final id: it is a rename row, or it - creates a tag, so the id is not held by an existing tag. + Return whether the row defines its final id, so that rows naming that id as + ``parent_id`` depend on it: a rename row, or a row that creates a tag (its id is + not held by an existing tag). """ is_rename = bool(tag.previous_id) and tag.id != tag.previous_id return is_rename or tag.id.casefold() not in existing @@ -539,10 +569,10 @@ def _order_rows_by_parent_dependency(self, tags: list[TagItem], existing: Existi :param tags: the rows of the import file, in file order. :param existing: the taxonomy's tags, from ``_existing_tags``. """ - definers: dict[str, int] = {} + defining_row: dict[str, int] = {} for position, tag in enumerate(tags): if self._defines_id(tag, existing): - definers.setdefault(tag.id.casefold(), position) + defining_row.setdefault(tag.id.casefold(), position) parent_of, row_position = self._end_state_parents(tags, existing) parent_pk_of = {info.pk: info.parent_pk for info in existing.values()} @@ -554,25 +584,30 @@ def dependencies(position: int) -> list[int]: if not tag.parent_id: return [] result = [] - definer = definers.get(tag.parent_id.casefold()) - if definer is not None: - result.append(definer) - current = existing.get((tag.previous_id or tag.id).casefold()) - if current is None: + parent_row = defining_row.get(tag.parent_id.casefold()) + if parent_row is not None: + result.append(parent_row) + moved_tag = existing.get((tag.previous_id or tag.id).casefold()) + if moved_tag is None: return result + # Walk the new parent's end-state ancestor chain. node: str | None = tag.parent_id.casefold() visited: set[str] = set() while node is not None and node not in visited: visited.add(node) - q_position = row_position.get(node) - q_row = tags[q_position] if q_position is not None else None - q_current = existing.get((q_row.previous_id or q_row.id).casefold()) if q_row else None - if q_current is not None and q_current.pk != current.pk: - ancestor_pk = q_current.parent_pk - while ancestor_pk is not None and ancestor_pk != current.pk: - ancestor_pk = parent_pk_of.get(ancestor_pk) - if ancestor_pk is not None and q_position is not None: - result.append(q_position) + ancestor_position = row_position.get(node) + ancestor_row = tags[ancestor_position] if ancestor_position is not None else None + ancestor_tag = ( + existing.get((ancestor_row.previous_id or ancestor_row.id).casefold()) if ancestor_row else None + ) + if ancestor_tag is not None and ancestor_tag.pk != moved_tag.pk: + # Walk the ancestor's current parents to see if it sits inside the moved subtree. + pk = ancestor_tag.parent_pk + while pk is not None and pk != moved_tag.pk: + pk = parent_pk_of.get(pk) + is_inside_moved_subtree = pk is not None + if is_inside_moved_subtree and ancestor_position is not None: + result.append(ancestor_position) node = parent_of.get(node) return result @@ -609,11 +644,14 @@ def _order_rows_parent_first(self, tags: list[TagItem], existing: ExistingTags) :param existing: the taxonomy's tags, from ``_existing_tags``. """ _parent_of, row_position = self._end_state_parents(tags, existing) - dependencies = [ - [row_position[tag.parent_id.casefold()]] - if tag.parent_id and row_position.get(tag.parent_id.casefold(), position) != position else [] - for position, tag in enumerate(tags) - ] + dependencies: list[list[int]] = [] + for position, tag in enumerate(tags): + parent_row = row_position.get(tag.parent_id.casefold()) if tag.parent_id else None + # A row whose parent_id is its own id has no other row to follow. + if parent_row is not None and parent_row != position: + dependencies.append([parent_row]) + else: + dependencies.append([]) return self._order_by_dependencies(tags, dependencies) def _build_detach_actions(self, tags: list[TagItem], existing: ExistingTags) -> None: @@ -637,7 +675,7 @@ def _build_detach_actions(self, tags: list[TagItem], existing: ExistingTags) -> for tag in tags: current = existing.get((tag.previous_id or tag.id).casefold()) if current is None or current.parent_pk is None or current.parent_external_id is None: - continue + continue # A new tag or a root tag has nothing to detach. if current.parent_external_id.casefold() in deleted_ids: continue # already promoted by the delete phase parent_end_id = end_id_by_pk.get(current.parent_pk, current.parent_external_id).casefold() @@ -678,20 +716,26 @@ def generate_actions( Those two actions should be merged. """ existing = self._existing_tags() - self._generate_actions(tags, replace, existing, safe=False) - # A plan whose intermediate states would be invalid is rebuilt detach-first. If that - # is still invalid, no order of the actions works. + self._generate_actions(tags, replace, existing, detach_first=False) + # A plan can have a valid end state while one of its steps is invalid, for example + # a deep tag that gets new children before it moves up. Such a plan is rebuilt with + # detach_first (see _generate_actions). If that is still invalid, no order of the + # actions works. if not self.errors and not self._parent_changes_are_valid(existing): - self._generate_actions(tags, replace, existing, safe=True) + self._generate_actions(tags, replace, existing, detach_first=True) if not self._parent_changes_are_valid(existing): self.errors.append(InvalidParentHierarchyError(TAXONOMY_MAX_DEPTH + 1)) - def _generate_actions(self, tags: list[TagItem], replace: bool, existing: ExistingTags, safe: bool) -> None: + def _generate_actions( + self, tags: list[TagItem], replace: bool, existing: ExistingTags, detach_first: bool, + ) -> None: """ Build the actions for ``tags``, with the rows ordered by ``_order_rows_by_parent_dependency``. - In ``safe`` mode the existing tags whose parent changes are first detached to roots and - the rows are ordered by ``_order_rows_parent_first`` instead. + With ``detach_first``, every existing tag whose parent changes is first moved to the + root (``_build_detach_actions``), and the rows are then ordered parent first + (``_order_rows_parent_first``). No step then builds a tree deeper or more cyclic than + the end state. """ self.actions.clear() self.errors.clear() @@ -735,7 +779,7 @@ def _generate_actions(self, tags: list[TagItem], replace: bool, existing: Existi # Delete all not readed tags self._build_delete_actions(tags_for_delete) - if safe: + if detach_first: self._build_detach_actions(tags, existing) # Stage tags whose external_id is contended by another rename row in @@ -747,7 +791,7 @@ def _generate_actions(self, tags: list[TagItem], replace: bool, existing: Existi self._validate_parent_id_not_stale(tags, existing) self._validate_no_parent_cycles(tags, existing) - if safe: + if detach_first: rows = self._order_rows_parent_first(tags, existing) else: rows = self._order_rows_by_parent_dependency(tags, existing) From 61ef2e4c62ad25c46d7bbff8b9f60e2b957d4364 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 14 Oct 2026 17:16:22 +0300 Subject: [PATCH 4/5] test: cut duplicate tag import tests and make the rest readable Delete tests that repeat another test of the same scenario, at the plan level and through the API, and tests that pin implementation details instead of behavior. Cases that only a deleted test covered moved into the kept test. Coverage of the import modules is unchanged. The remaining tests use keyword arguments for rows, name tags after their role, describe the tree before and after the import, refer to the #673 scenarios by title, and explain why a test samples row orders. Co-Authored-By: Claude Opus 5.5 --- .../openedx_tagging/import_export/test_api.py | 1230 ++++++++++------- .../import_export/test_import_plan.py | 688 +++------ 2 files changed, 961 insertions(+), 957 deletions(-) diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index 11cf8952f..633945528 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -13,6 +13,7 @@ import openedx_tagging.import_export.api as import_export_api from openedx_tagging.import_export import ParserFormat +from openedx_tagging.import_export.import_plan import TagItem from openedx_tagging.models import ObjectTag, Tag, TagImportTask, TagImportTaskState, Taxonomy from .mixins import TestImportExportMixin @@ -1080,9 +1081,13 @@ def test_import_rename_referencing_stale_old_id_rejected(self) -> None: def test_import_existing_child_names_renamed_away_parent_rejected(self) -> None: """ - An existing child (tag_2 under tag_1) that keeps parent_id="tag_1" - while the file renames tag_1 to tag_50 is rejected in either row - order, and neither the taxonomy nor its export changes. + An existing child that keeps its parent's pre-import id is rejected when the file + renames that parent, in either row order, and neither the taxonomy nor its export + changes. + + Before File rows + tag_1 tag_50 (previous_id tag_1) + tag_2 tag_2 (parent_id tag_1) """ rename = {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"} child = {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_1"} @@ -1101,10 +1106,15 @@ def test_import_existing_child_names_renamed_away_parent_rejected(self) -> None: def test_import_child_and_renamed_parent_row_claiming_freed_id(self) -> None: """ - tag_3 is renamed to tag_1, an id the same replace=True import frees by - omitting its holder. A child row naming tag_1 succeeds in either row - order: the renamed tag keeps its pk, the old holder is gone, and the - child is nested under the renamed tag. + With replace=True the file omits tag_1, which frees the id tag_1, and renames tag_3 + onto it. A child row naming tag_1 succeeds in either row order: the renamed tag keeps + its pk, the old tag_1 is gone, and the child is under the renamed tag. + + Before After + tag_1 tag_1 (was tag_3) + tag_2 tag_4 + tag_3 + tag_4 """ pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk pk_3 = self.taxonomy.tag_set.get(external_id="tag_3").pk @@ -1123,8 +1133,14 @@ def test_import_child_and_renamed_parent_row_claiming_freed_id(self) -> None: def test_import_swap_with_children_in_any_row_order(self) -> None: """ - The swap with children on end-state ids gives the same result in - every one of the 24 row orders. + Swapping the ids of two parents whose children name the end-state ids gives the same + result in every row order. Each child stays on the same physical parent. + + Before After + tag_1 tag_3 (was tag_1) + tag_2 tag_2 + tag_3 tag_1 (was tag_3) + tag_4 tag_4 """ pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk pk_2 = self.taxonomy.tag_set.get(external_id="tag_2").pk @@ -1136,6 +1152,8 @@ def test_import_swap_with_children_in_any_row_order(self) -> None: {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_3"}, {"id": "tag_4", "value": "Tag 4", "parent_id": "tag_1"}, ] + # Every one of the 24 row orders, because the swap and the children's references + # resolve differently depending on which rows come first. for order in itertools.permutations(range(4)): with self.subTest(order=order), transaction.atomic(): result, log = self._import_rows([rows[i] for i in order], replace=True) @@ -1158,9 +1176,15 @@ def test_import_swap_with_children_in_any_row_order(self) -> None: def test_import_swap_children_stale_ids_rejected_in_any_row_order(self) -> None: """ - Children naming the stale pre-import ids are rejected in every one of - the 24 row orders, each error reports the row's file position, and - the taxonomy is unchanged. + Children that name the stale pre-import ids of swapped parents are rejected in every + row order. Each error names the child's row number and its parent's new id, and the + taxonomy is unchanged. + + Before File rows + tag_1 tag_1 (previous_id tag_3) + tag_2 tag_3 (previous_id tag_1) + tag_3 tag_2 (parent_id tag_1) + tag_4 tag_4 (parent_id tag_3) """ rows = [ {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"}, @@ -1170,6 +1194,7 @@ def test_import_swap_children_stale_ids_rejected_in_any_row_order(self) -> None: ] before = self._snapshot() new_ids = {"tag_2": ("tag_1", "tag_3"), "tag_4": ("tag_3", "tag_1")} + # Every one of the 24 row orders, because each error must name the row's own number. for order in itertools.permutations(range(4)): with self.subTest(order=order): file_rows = [rows[i] for i in order] @@ -1183,23 +1208,27 @@ def test_import_swap_children_stale_ids_rejected_in_any_row_order(self) -> None: assert f"Use '{new_id}' as the parent_id." in log assert self._snapshot() == before - def test_import_child_of_invalid_definer_row_rejected_in_any_order(self) -> None: + def test_import_child_of_invalid_parent_row_rejected_in_any_order(self) -> None: """ - A child whose parent_id names a rename row with an unmatched - previous_id, or a new-tag row with a duplicate value, is rejected - cleanly in either row order and leaves the taxonomy unchanged. + A new child whose parent_id names a row that is itself rejected (a rename with an + unmatched previous_id, or a new tag with a duplicate value) is rejected cleanly in + either row order and leaves the taxonomy unchanged. + + File rows (two cases) + new_parent (previous_id tag_99) or new_parent (value "Tag 1", held by tag_1) + new_child (parent_id new_parent) """ cases = { "unmatched previous_id": ( - {"id": "tag_90", "value": "Tag 90", "previous_id": "tag_99"}, + {"id": "new_parent", "value": "New parent", "previous_id": "tag_99"}, "Unknown previous_id (tag_99). No tag with that external_id exists in this taxonomy.", ), - "duplicate value": ({"id": "tag_90", "value": "Tag 1"}, None), + "duplicate value": ({"id": "new_parent", "value": "Tag 1"}, None), } - child = {"id": "tag_91", "value": "Tag 91", "parent_id": "tag_90"} + child = {"id": "new_child", "value": "New child", "parent_id": "new_parent"} before = self._snapshot() - for name, (definer, cause) in cases.items(): - for rows in ([definer, child], [child, definer]): + for name, (parent, cause) in cases.items(): + for rows in ([parent, child], [child, parent]): with self.subTest(case=name, first=rows[0]["id"]): result, log = self._import_rows(rows) assert not result @@ -1211,44 +1240,54 @@ def test_import_child_of_invalid_definer_row_rejected_in_any_order(self) -> None def test_import_cyclic_parent_ids_rejected(self) -> None: """ - Two new tags naming each other as parent are rejected with an unknown - parent error, with and without replace, and nothing is created. + Two new tags naming each other as parent are rejected, with and without replace, + and nothing is created. The cycle is reported once, as an unknown parent, because + neither id exists before the import. + + File rows + new_a (parent_id new_b) + new_b (parent_id new_a) """ rows = [ - {"id": "tag_90", "value": "Tag 90", "parent_id": "tag_91"}, - {"id": "tag_91", "value": "Tag 91", "parent_id": "tag_90"}, + {"id": "new_a", "value": "New A", "parent_id": "new_b"}, + {"id": "new_b", "value": "New B", "parent_id": "new_a"}, ] before = self._snapshot() for replace in (False, True): with self.subTest(replace=replace): result, log = self._import_rows(rows, replace=replace) assert not result - assert "Unknown parent tag" in log + assert log.count("Unknown parent tag") == 1 + assert "creates a parent cycle" not in log assert "Traceback" not in log assert self._snapshot() == before def test_import_child_of_new_row_with_value_freed_by_replace(self) -> None: """ - With replace=True, a new row reusing the value of an existing tag the - file omits is accepted in every row order, because the replace-delete - frees the value, and its child is nested under it. If the file keeps - the existing tag, the same value is a real duplicate and is rejected. + With replace=True, a new tag may reuse the value of an existing tag that the file + omits, because the delete of the omitted tag frees the value. Its child is nested + under it in every row order. If the file keeps the existing tag, the same value is a + real duplicate and the import is rejected. + + Before After (file omits tag_1) + tag_1 new_parent (value "Tag 1") + tag_2 new_child """ - definer = {"id": "tag_90", "value": "Tag 1"} - child = {"id": "tag_91", "value": "Tag 91", "parent_id": "tag_90"} + new_parent = {"id": "new_parent", "value": "Tag 1"} + new_child = {"id": "new_child", "value": "New child", "parent_id": "new_parent"} kept = {"id": "tag_1", "value": "Tag 1"} before = self._snapshot() for order in itertools.permutations(range(2)): with self.subTest(case="omitted", order=order), transaction.atomic(): - result, log = self._import_rows([[definer, child][i] for i in order], replace=True) + result, log = self._import_rows([[new_parent, new_child][i] for i in order], replace=True) assert "Traceback" not in log assert result - parent = self.taxonomy.tag_set.get(external_id="tag_90") - assert self.taxonomy.tag_set.get(external_id="tag_91").parent_id == parent.pk + parent = self.taxonomy.tag_set.get(external_id="new_parent") + assert self.taxonomy.tag_set.get(external_id="new_child").parent_id == parent.pk transaction.set_rollback(True) for order in itertools.permutations(range(3)): with self.subTest(case="kept", order=order): - result, log = self._import_rows([[definer, child, kept][i] for i in order], replace=True) + result, log = self._import_rows([[new_parent, new_child, kept][i] for i in order], replace=True) assert not result assert "Traceback" not in log assert "Output errors" in log @@ -1256,20 +1295,24 @@ def test_import_child_of_new_row_with_value_freed_by_replace(self) -> None: def test_import_longer_and_mixed_parent_cycles_rejected_in_any_row_order(self) -> None: """ - A three-tag cycle of new tags, and a cycle through a rename row (the - rename row's parent_id names a new tag whose parent_id names the - rename row's new id), are rejected cleanly in every row order and - leave the taxonomy unchanged. + A cycle of three new tags, and a cycle through a rename row, are rejected in every + row order and leave the taxonomy unchanged. Each cycle is reported once, as an + unknown parent, because none of its ids exists before the import. + + File rows (two cases) + new_a (parent_id new_b) tag_50 (previous_id tag_1, parent_id new_tag) + new_b (parent_id new_c) new_tag (parent_id tag_50) + new_c (parent_id new_a) """ cases = { "three new tags": [ - {"id": "tag_90", "value": "Tag 90", "parent_id": "tag_91"}, - {"id": "tag_91", "value": "Tag 91", "parent_id": "tag_92"}, - {"id": "tag_92", "value": "Tag 92", "parent_id": "tag_90"}, + {"id": "new_a", "value": "New A", "parent_id": "new_b"}, + {"id": "new_b", "value": "New B", "parent_id": "new_c"}, + {"id": "new_c", "value": "New C", "parent_id": "new_a"}, ], "rename row and new tag": [ - {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1", "parent_id": "tag_90"}, - {"id": "tag_90", "value": "Tag 90", "parent_id": "tag_50"}, + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1", "parent_id": "new_tag"}, + {"id": "new_tag", "value": "New tag", "parent_id": "tag_50"}, ], } before = self._snapshot() @@ -1279,84 +1322,73 @@ def test_import_longer_and_mixed_parent_cycles_rejected_in_any_row_order(self) - result, log = self._import_rows([rows[i] for i in order]) assert not result assert "Traceback" not in log - assert "Output errors" in log + assert log.count("Unknown parent tag") == 1 + assert "creates a parent cycle" not in log assert self._snapshot() == before def test_import_parent_cycles_through_existing_tags_rejected(self) -> None: """ - A cycle that includes an existing tag is rejected once, in either row - order, and leaves the taxonomy unchanged: an existing and a new tag as - each other's parent, a rename row and an existing tag, a self-parent, and a cycle through tag_2, which - is a child of tag_1 and not in the file. + A cycle that includes an existing tag is rejected in every row order and leaves the + taxonomy unchanged. The error is reported once, on the cycle's first row in the file. + + Before Cases (file rows) + tag_1 existing and new tag: tag_2 (parent_id new_tag), new_tag (parent_id tag_2) + tag_2 self parent: tag_2 (parent_id tag_2) + rename row and existing tag: tag_50 (previous_id tag_1, parent_id tag_2), + tag_2 (parent_id tag_50) + through a tag not in the file: tag_1 (parent_id tag_2) """ cases = { - "existing and new tag": ( - [ - {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_90"}, - {"id": "tag_90", "value": "Tag 90", "parent_id": "tag_2"}, - ], - "creates a parent cycle.", - ), - "self parent": ( - [{"id": "tag_2", "value": "Tag 2", "parent_id": "tag_2"}], - "Row 1's parent_id (tag_2) creates a parent cycle.", - ), - "rename row and existing tag": ( - [ - {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1", "parent_id": "tag_2"}, - {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_50"}, - ], - "creates a parent cycle.", - ), - "through a tag not in the file": ( - [{"id": "tag_1", "value": "Tag 1", "parent_id": "tag_2"}], - "Row 1's parent_id (tag_2) creates a parent cycle.", - ), + "existing and new tag": [ + {"id": "tag_2", "value": "Tag 2", "parent_id": "new_tag"}, + {"id": "new_tag", "value": "New tag", "parent_id": "tag_2"}, + ], + "self parent": [{"id": "tag_2", "value": "Tag 2", "parent_id": "tag_2"}], + "rename row and existing tag": [ + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1", "parent_id": "tag_2"}, + {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_50"}, + ], + "through a tag not in the file": [{"id": "tag_1", "value": "Tag 1", "parent_id": "tag_2"}], } before = self._snapshot() - for name, (rows, message) in cases.items(): + for name, rows in cases.items(): for order in itertools.permutations(range(len(rows))): with self.subTest(case=name, order=order): - result, log = self._import_rows([rows[i] for i in order]) + file_rows = [rows[i] for i in order] + result, log = self._import_rows(file_rows) assert not result assert "Traceback" not in log - assert log.count(message) == 1 + assert log.count("creates a parent cycle.") == 1 + assert f"Row 1's parent_id ({file_rows[0]['parent_id']}) creates a parent cycle." in log assert self._snapshot() == before def test_import_existing_tag_moved_under_later_new_parent(self) -> None: """ - An existing tag moved under a new parent defined later in the file is + An existing tag moved under a new parent whose row comes later in the file is accepted, and the tag ends up under the new parent. + + Before After + tag_1 tag_1 + tag_2 new_parent + tag_2 """ result, log = self._import_rows([ - {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_90"}, - {"id": "tag_90", "value": "Tag 90"}, + {"id": "tag_2", "value": "Tag 2", "parent_id": "new_parent"}, + {"id": "new_parent", "value": "New parent"}, ]) assert result assert "Traceback" not in log - parent = self.taxonomy.tag_set.get(external_id="tag_90") + parent = self.taxonomy.tag_set.get(external_id="new_parent") assert self.taxonomy.tag_set.get(external_id="tag_2").parent_id == parent.pk - def test_import_swap_rename_child_stale_parent_id_rejected_once(self) -> None: - """ - In a tag_1 <-> tag_3 swap, a rename row (tag_2 -> tag_20) naming the - stale parent id tag_1 is rejected, and the error appears exactly once. - """ - before = self._snapshot() - result, log = self._import_rows([ - {"id": "tag_3", "value": "Tag 1", "previous_id": "tag_1"}, - {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"}, - {"id": "tag_20", "value": "Tag 2", "previous_id": "tag_2", "parent_id": "tag_1"}, - ]) - assert not result - assert "Traceback" not in log - assert log.count("Row 3's parent_id (tag_1) names an id this file renames away.") == 1 - assert self._snapshot() == before - def test_import_rename_child_stale_parent_id_without_swap_reported_once(self) -> None: """ - Without a swap, a rename row (tag_2 -> tag_20) naming the stale parent - id tag_1 gets only the rename action's error, once. + Without a swap, a renamed child that names its parent's stale id gets only the rename + action's error, once. + + Before File rows + tag_1 tag_50 (previous_id tag_1) + tag_2 tag_20 (previous_id tag_2, parent_id tag_1) """ before = self._snapshot() result, log = self._import_rows([ @@ -1371,65 +1403,84 @@ def test_import_rename_child_stale_parent_id_without_swap_reported_once(self) -> def test_import_case_only_parent_rename_keeps_child_parent(self) -> None: """ - Renaming a parent only by case succeeds, and a child that keeps the - old casing in parent_id stays under the same parent. + Renaming a parent only by case succeeds in either row order, and a child that names + the parent in any case stays under the same parent. + + Before After + tag_1 TAG_1 + tag_2 tag_2 (parent_id tag_1, TAG_1 or Tag_1) """ pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk pk_2 = self.taxonomy.tag_set.get(external_id="tag_2").pk - result, log = self._import_rows([ - {"id": "TAG_1", "value": "Tag 1", "previous_id": "tag_1"}, - {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_1"}, - ]) - assert result - assert "Traceback" not in log - assert Tag.objects.get(pk=pk_1).external_id == "TAG_1" - assert Tag.objects.get(pk=pk_2).parent_id == pk_1 + rename = {"id": "TAG_1", "value": "Tag 1", "previous_id": "tag_1"} + for parent_id in ("tag_1", "TAG_1", "Tag_1"): + child = {"id": "tag_2", "value": "Tag 2", "parent_id": parent_id} + for rows in ([rename, child], [child, rename]): + with self.subTest(parent_id=parent_id, first=rows[0]["id"]), transaction.atomic(): + result, log = self._import_rows(rows) + assert result + assert "Traceback" not in log + assert Tag.objects.get(pk=pk_1).external_id == "TAG_1" + assert Tag.objects.get(pk=pk_2).parent_id == pk_1 + transaction.set_rollback(True) def test_import_new_child_before_new_parent_row(self) -> None: """ - A new child row listed before its new parent row succeeds, the child - is nested under the new parent, and the export shows its parent_id. + A new child row listed before its new parent row succeeds, the child is nested under + the new parent, and the export shows its parent_id. + + File rows After + new_child (parent_id new_parent) new_parent + new_parent new_child """ result, log = self._import_rows([ - {"id": "tag_91", "value": "Tag 91", "parent_id": "tag_90"}, - {"id": "tag_90", "value": "Tag 90"}, + {"id": "new_child", "value": "New child", "parent_id": "new_parent"}, + {"id": "new_parent", "value": "New parent"}, ]) assert result assert "Traceback" not in log - parent = self.taxonomy.tag_set.get(external_id="tag_90") - assert self.taxonomy.tag_set.get(external_id="tag_91").parent_id == parent.pk + parent = self.taxonomy.tag_set.get(external_id="new_parent") + assert self.taxonomy.tag_set.get(external_id="new_child").parent_id == parent.pk output = import_export_api.export_tags(self.taxonomy, self.parser_format) exported_by_id = {tag["id"]: tag for tag in json.loads(output).get("tags")} - assert exported_by_id["tag_91"]["parent_id"] == "tag_90" + assert exported_by_id["new_child"]["parent_id"] == "new_parent" def test_import_child_first_references_plain_rename_new_id(self) -> None: """ - A new child row listed before the plain rename (tag_1 -> tag_50) of - its parent, and naming the new id, succeeds and is nested under the - renamed tag. + A new child listed before the row that renames its parent, and naming the parent's + new id, succeeds and is nested under the renamed tag. + + Before After + tag_1 tag_50 (was tag_1) + tag_2 tag_2 + new_child """ pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk result, log = self._import_rows([ - {"id": "tag_60", "value": "Tag 60", "parent_id": "tag_50"}, + {"id": "new_child", "value": "New child", "parent_id": "tag_50"}, {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"}, ]) assert result assert "Traceback" not in log assert Tag.objects.get(pk=pk_1).external_id == "tag_50" - assert self.taxonomy.tag_set.get(external_id="tag_60").parent_id == pk_1 + assert self.taxonomy.tag_set.get(external_id="new_child").parent_id == pk_1 def test_import_plain_and_rename_children_of_renamed_parent_in_reverse_order(self) -> None: """ - A plain child and a rename-row child both naming the renamed parent's - new id, listed before the parent's rename row, succeed and end up - under the renamed parent. + A new child and a renamed child, both naming the renamed parent's new id and listed + before the parent's rename row, succeed and end up under the renamed parent. + + Before After + tag_1 tag_50 (was tag_1) + tag_2 tag_20 (was tag_2) + new_child """ pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk pk_2 = self.taxonomy.tag_set.get(external_id="tag_2").pk result, log = self._import_rows([ {"id": "tag_20", "value": "Tag 2", "previous_id": "tag_2", "parent_id": "tag_50"}, - {"id": "tag_60", "value": "Tag 60", "parent_id": "tag_50"}, + {"id": "new_child", "value": "New child", "parent_id": "tag_50"}, {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"}, ]) assert result @@ -1437,7 +1488,7 @@ def test_import_plain_and_rename_children_of_renamed_parent_in_reverse_order(sel assert Tag.objects.get(pk=pk_1).external_id == "tag_50" assert Tag.objects.get(pk=pk_2).external_id == "tag_20" assert Tag.objects.get(pk=pk_2).parent_id == pk_1 - assert self.taxonomy.tag_set.get(external_id="tag_60").parent_id == pk_1 + assert self.taxonomy.tag_set.get(external_id="new_child").parent_id == pk_1 def test_import_same_value_without_external_id(self) -> None: new_taxonomy = Taxonomy(name="New taxonomy") @@ -1461,23 +1512,29 @@ def test_import_same_value_without_external_id(self) -> None: assert result -def _row(tag_id: str, value: str, parent_id: str | None = None, previous_id: str | None = None) -> dict: - """ - Build an import row, leaving out the optional fields that are not set. - """ - row = {"id": tag_id, "value": value} - if parent_id: - row["parent_id"] = parent_id - if previous_id: - row["previous_id"] = previous_id - return row - - class TestImportAcceptanceScenarios(TestCase): """ - The acceptance scenarios of the external_id rename import (#673, #858), run the way the - Studio import wizard does: a full replace import of a CSV or JSON file, checked through - the import log and the exported file. + The acceptance scenarios of #673 and #858, run the way the Studio import wizard runs an + import: a full replace import of a CSV or JSON file, checked through the import log and + the exported file. + + Rows are written as ``TagItem`` objects, whose fields are the file's columns. Some #673 + scenarios are covered by tests in ``TestImportExportApi`` and are not repeated here: + + - "re-import is idempotent (previous_id equals the new id)": + ``test_import_rename_external_id_previous_id_equals_id_is_noop``. + - "previous_id is present but matches no existing tag (likely typo)": + ``test_import_rename_external_id_unmatched_previous_id_rejected``. + - "a rename claims an id the same import frees by removing its holder": + ``test_import_rename_external_id_reuses_id_freed_by_replace_delete``, and + ``test_import_child_and_renamed_parent_row_claiming_freed_id`` for both row orders. + - "a child row referencing its parent's pre-import external_id is rejected": + ``test_import_existing_child_names_renamed_away_parent_rejected``. + - "a parent and its child are both renamed in one import": + ``test_import_rename_parent_and_child_both_renamed``. + + The #858 swap criteria are covered by ``test_import_swap_with_children_in_any_row_order`` + and ``test_import_swap_children_stale_ids_rejected_in_any_row_order``. """ formats = (ParserFormat.JSON, ParserFormat.CSV) @@ -1488,19 +1545,32 @@ def setUpTestData(cls) -> None: super().setUpTestData() cls.taxonomy = Taxonomy.objects.create(name="Competencies") - def _render(self, rows: list[dict], parser_format: ParserFormat) -> BytesIO: + def _render(self, rows: list[TagItem], parser_format: ParserFormat) -> BytesIO: """ - Render `rows` as an import file of the given format. + Render ``rows`` as an import file of the given format, leaving out unset fields. """ + file_rows = [ + { + key: value + for key, value in ( + ("id", row.id), + ("value", row.value), + ("parent_id", row.parent_id), + ("previous_id", row.previous_id), + ) + if value + } + for row in rows + ] if parser_format == ParserFormat.JSON: - return BytesIO(json.dumps({"tags": rows}).encode()) + return BytesIO(json.dumps({"tags": file_rows}).encode()) buffer = StringIO() writer = csv.DictWriter(buffer, fieldnames=["id", "value", "parent_id", "previous_id"]) writer.writeheader() - writer.writerows(rows) + writer.writerows(file_rows) return BytesIO(buffer.getvalue().encode()) - def _import(self, rows: list[dict], parser_format: ParserFormat, plan_only: bool = False): + def _import(self, rows: list[TagItem], parser_format: ParserFormat, plan_only: bool = False): """ Run a full replace import and return (result, log, plan). """ @@ -1516,7 +1586,7 @@ def _import(self, rows: list[dict], parser_format: ParserFormat, plan_only: bool assert "Traceback" not in log return result, log, plan - def _seed(self, rows: list[dict]) -> dict[str, int]: + def _seed(self, rows: list[TagItem]) -> dict[str, int]: """ Create the starting tags and return their pks by external_id. """ @@ -1587,31 +1657,51 @@ def _rolled_back(self): yield transaction.set_rollback(True) - def _assert_rejected(self, rows: list[dict], parser_format: ParserFormat, *messages: str) -> str: + def _assert_rejected( + self, + rows: list[TagItem], + parser_format: ParserFormat, + *, + messages: Sequence[str], + error_count: int | None = None, + ) -> str: """ - Check the import is rejected, its log has each message, and nothing changed. + Check the import is rejected, its log has each of ``messages``, and nothing changed. + + With ``error_count``, also check the plan has exactly that many errors. """ snapshot = set(self.taxonomy.tag_set.values_list("pk", "external_id", "value", "parent_id")) before = self._export(parser_format) - result, log, _plan = self._import(rows, parser_format) + result, log, plan = self._import(rows, parser_format) assert not result for message in messages: assert message in log + if error_count is not None: + assert plan is not None + assert len(plan.errors) == error_count assert set(self.taxonomy.tag_set.values_list("pk", "external_id", "value", "parent_id")) == snapshot assert self._export(parser_format) == before return log def test_rename_shows_in_export_without_duplicate_or_previous_id(self) -> None: """ - S1, S2, S15: a rename shows in the export once, as the new id, with no row for the old - id and no previous_id field, and the tag keeps its pk and its content associations. + #673 scenarios "rename an external_id, verified via CSV export", "rename an + external_id, verified via JSON export" and "renaming an external_id preserves the + competency's content associations": the export shows the tag once, with its new id, + no row for the old id and no previous_id field, and the tag keeps its pk and its + content associations. + + Before After + COMP-A COMP-B (same tag) """ - pks = self._seed([_row("COMP-A", "Alpha")]) + pks = self._seed([TagItem(id="COMP-A", value="Alpha")]) object_tags = self._associate(pks, "COMP-A") for parser_format in self.formats: with self.subTest(parser_format=parser_format), self._rolled_back(): count = len(self._exported_rows(parser_format)) - result, _log, _plan = self._import([_row("COMP-B", "Alpha", previous_id="COMP-A")], parser_format) + result, _log, _plan = self._import( + [TagItem(id="COMP-B", value="Alpha", previous_id="COMP-A")], parser_format, + ) assert result assert [row["id"] for row in self._exported_rows(parser_format)] == ["COMP-B"] assert len(self._exported_rows(parser_format)) == count @@ -1619,104 +1709,63 @@ def test_rename_shows_in_export_without_duplicate_or_previous_id(self) -> None: assert Tag.objects.get(pk=pks["COMP-A"]).external_id == "COMP-B" self._assert_associations_kept(object_tags) - def test_reimport_with_previous_id_equal_to_id_is_idempotent(self) -> None: - """ - S3: a row whose previous_id equals its id succeeds and the export does not change. - """ - self._seed([_row("COMP-B", "Beta")]) - for parser_format in self.formats: - with self.subTest(parser_format=parser_format), self._rolled_back(): - before = self._export(parser_format) - result, _log, _plan = self._import([_row("COMP-B", "Beta", previous_id="COMP-B")], parser_format) - assert result - assert self._export(parser_format) == before - def test_new_id_without_previous_id_is_created(self) -> None: """ - S4: a row with a new id and no previous_id creates a tag that shows in the export. + #673 scenario "no previous_id, new id (create behavior preserved)": a row with a new + id and no previous_id creates a tag that shows in the export. + + Before After + COMP-A COMP-A + COMP-Z """ - self._seed([_row("COMP-A", "Alpha")]) + self._seed([TagItem(id="COMP-A", value="Alpha")]) for parser_format in self.formats: with self.subTest(parser_format=parser_format), self._rolled_back(): result, _log, _plan = self._import( - [_row("COMP-A", "Alpha"), _row("COMP-Z", "Zulu")], parser_format, + [TagItem(id="COMP-A", value="Alpha"), TagItem(id="COMP-Z", value="Zulu")], parser_format, ) assert result assert self._exported_ids(parser_format) == {"Alpha": "COMP-A", "Zulu": "COMP-Z"} - def test_unmatched_previous_id_rejected(self) -> None: - """ - S5: an unmatched previous_id is rejected with the id in the log, nothing is introduced, - and when several rows have one, every id is in the same log. - """ - self._seed([_row("COMP-A", "Alpha"), _row("COMP-C", "Gamma")]) - for parser_format in self.formats: - with self.subTest(parser_format=parser_format): - self._assert_rejected( - [_row("COMP-B", "Alpha", previous_id="COMP-GHOST"), _row("COMP-C", "Gamma")], - parser_format, - "COMP-GHOST", - ) - self._assert_rejected( - [ - _row("COMP-B", "Alpha", previous_id="COMP-GHOST-1"), - _row("COMP-D", "Gamma", previous_id="COMP-GHOST-2"), - ], - parser_format, - "COMP-GHOST-1", - "COMP-GHOST-2", - ) - def test_rename_onto_id_of_kept_tag_rejected(self) -> None: """ - S6, S8: a rename onto an id whose holder the same file keeps is rejected, in either row - order, whether the holder is kept as a plain row or as a row with previous_id equal to id. + #673 scenarios "new id collides with a different tag's external_id in the same + taxonomy" and "a rename onto an id whose holder the same import keeps is rejected": + the rename is rejected in either row order, whether the file keeps the holder as a + plain row or as a row whose previous_id equals its id. + + Before File rows + COMP-Z COMP-Z (previous_id COMP-A) + COMP-A COMP-Z (the holder, kept) """ - self._seed([_row("COMP-Z", "Xray"), _row("COMP-A", "Yank")]) - rename = _row("COMP-Z", "Yank", previous_id="COMP-A") + self._seed([TagItem(id="COMP-Z", value="Xray"), TagItem(id="COMP-A", value="Yank")]) + rename = TagItem(id="COMP-Z", value="Yank", previous_id="COMP-A") keeps = { - "plain": _row("COMP-Z", "Xray"), - "previous_id equal to id": _row("COMP-Z", "Xray", previous_id="COMP-Z"), + "plain": TagItem(id="COMP-Z", value="Xray"), + "previous_id equal to id": TagItem(id="COMP-Z", value="Xray", previous_id="COMP-Z"), } for parser_format in self.formats: for name, keep in keeps.items(): for rows in ([rename, keep], [keep, rename]): - with self.subTest(parser_format=parser_format, keep=name, first=rows[0]["value"]): - self._assert_rejected(rows, parser_format, "COMP-Z") - - def test_rename_onto_id_freed_by_omitting_holder(self) -> None: - """ - S7: a rename onto an id the same file frees by omitting its holder succeeds in every row - order: the holder is gone, the renamed tag keeps its pk and associations, and the export - shows it. - """ - pks = self._seed([ - _row("COMP-Z", "Xray"), _row("COMP-A", "Yank"), _row("COMP-W", "Whisky"), _row("COMP-V", "Victor"), - ]) - object_tags = self._associate(pks, "COMP-A") - rows = [ - _row("COMP-Z", "Yank", previous_id="COMP-A"), _row("COMP-W", "Whisky"), _row("COMP-V", "Victor"), - ] - for parser_format in self.formats: - for order in itertools.permutations(range(3)): - with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): - result, _log, _plan = self._import([rows[i] for i in order], parser_format) - assert result - assert not Tag.objects.filter(pk=pks["COMP-Z"]).exists() - assert Tag.objects.get(pk=pks["COMP-A"]).external_id == "COMP-Z" - self._assert_associations_kept(object_tags) - assert self._exported_ids(parser_format) == { - "Yank": "COMP-Z", "Whisky": "COMP-W", "Victor": "COMP-V", - } + with self.subTest(parser_format=parser_format, keep=name, first=rows[0].value): + self._assert_rejected(rows, parser_format, messages=["COMP-Z"]) def test_swap_keeps_tags_and_associations(self) -> None: """ - S9: a swap succeeds in either row order, creates and deletes nothing, keeps both tags' - pks and associations, and the export shows the swapped ids. + #673 scenario "two competencies swap external_ids in one import": the swap succeeds + in either row order, creates and deletes nothing, keeps both tags' pks and content + associations, and the export shows the swapped ids. + + Before After + COMP-A COMP-B (was COMP-A) + COMP-B COMP-A (was COMP-B) """ - pks = self._seed([_row("COMP-A", "Alpha"), _row("COMP-B", "Beta")]) + pks = self._seed([TagItem(id="COMP-A", value="Alpha"), TagItem(id="COMP-B", value="Beta")]) object_tags = self._associate(pks, "COMP-A", "COMP-B") - rows = [_row("COMP-B", "Alpha", previous_id="COMP-A"), _row("COMP-A", "Beta", previous_id="COMP-B")] + rows = [ + TagItem(id="COMP-B", value="Alpha", previous_id="COMP-A"), + TagItem(id="COMP-A", value="Beta", previous_id="COMP-B"), + ] for parser_format in self.formats: for order in itertools.permutations(range(2)): with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): @@ -1730,14 +1779,23 @@ def test_swap_keeps_tags_and_associations(self) -> None: def test_three_way_rotation_in_any_row_order(self) -> None: """ - S10: a three-way rotation succeeds in all six row orders, and each tag holds the id its - own row specified. + #673 scenario "three competencies rotate external_ids in one import": the rotation + succeeds in all six row orders, and each tag holds the id its own row specified. + + Before After + COMP-A COMP-B (was COMP-A) + COMP-B COMP-C (was COMP-B) + COMP-C COMP-A (was COMP-C) """ - pks = self._seed([_row("COMP-A", "Alpha"), _row("COMP-B", "Beta"), _row("COMP-C", "Gamma")]) + pks = self._seed([ + TagItem(id="COMP-A", value="Alpha"), + TagItem(id="COMP-B", value="Beta"), + TagItem(id="COMP-C", value="Gamma"), + ]) rows = [ - _row("COMP-B", "Alpha", previous_id="COMP-A"), - _row("COMP-C", "Beta", previous_id="COMP-B"), - _row("COMP-A", "Gamma", previous_id="COMP-C"), + TagItem(id="COMP-B", value="Alpha", previous_id="COMP-A"), + TagItem(id="COMP-C", value="Beta", previous_id="COMP-B"), + TagItem(id="COMP-A", value="Gamma", previous_id="COMP-C"), ] for parser_format in self.formats: for order in itertools.permutations(range(3)): @@ -1753,64 +1811,44 @@ def test_three_way_rotation_in_any_row_order(self) -> None: def test_rename_chain_colliding_with_kept_tag_rejected_whole(self) -> None: """ - S11: a rename chain that ends on an id the file keeps is rejected whole in all six row - orders, with the id in the log and all three tags unchanged. + #673 scenario "a rename chain that collides with a competency the file keeps is + rejected whole": the import is rejected in all six row orders, with the id in the log + and all three tags unchanged. + + Before File rows + COMP-A COMP-B (previous_id COMP-A) + COMP-B COMP-C (previous_id COMP-B) + COMP-C COMP-C (kept) """ - self._seed([_row("COMP-A", "Alpha"), _row("COMP-B", "Beta"), _row("COMP-C", "Gamma")]) + self._seed([ + TagItem(id="COMP-A", value="Alpha"), + TagItem(id="COMP-B", value="Beta"), + TagItem(id="COMP-C", value="Gamma"), + ]) rows = [ - _row("COMP-B", "Alpha", previous_id="COMP-A"), - _row("COMP-C", "Beta", previous_id="COMP-B"), - _row("COMP-C", "Gamma"), + TagItem(id="COMP-B", value="Alpha", previous_id="COMP-A"), + TagItem(id="COMP-C", value="Beta", previous_id="COMP-B"), + TagItem(id="COMP-C", value="Gamma"), ] for parser_format in self.formats: for order in itertools.permutations(range(3)): with self.subTest(parser_format=parser_format, order=order): - self._assert_rejected([rows[i] for i in order], parser_format, "COMP-C") + self._assert_rejected([rows[i] for i in order], parser_format, messages=["COMP-C"]) def test_child_names_new_id_of_renamed_parent(self) -> None: """ - S12: a child row naming its renamed parent's new id succeeds in either row order, the - child stays under the same parent tag, and the export shows it nested under the new id. - """ - pks = self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", parent_id="COMP-P")]) - rows = [_row("COMP-P2", "Parent", previous_id="COMP-P"), _row("COMP-K", "Kid", parent_id="COMP-P2")] - for parser_format in self.formats: - for order in itertools.permutations(range(2)): - with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): - result, _log, _plan = self._import([rows[i] for i in order], parser_format) - assert result - assert Tag.objects.get(pk=pks["COMP-K"]).parent_id == pks["COMP-P"] - assert self._exported_ids(parser_format)["Parent"] == "COMP-P2" - assert self._exported_parents(parser_format)["Kid"] == "COMP-P2" + #673 scenario "a child row references its renamed parent's new external_id": the + import succeeds in either row order, the child stays under the same parent tag, and + the export shows it nested under the new id. - def test_child_names_pre_import_id_of_renamed_parent_rejected(self) -> None: + Before After + COMP-P COMP-P2 (was COMP-P) + COMP-K COMP-K """ - S13, A: an existing child naming its parent's pre-import id is rejected in either row - order, with its row position and the new id in the log, and nothing changes. - """ - self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", parent_id="COMP-P")]) - rows = [_row("COMP-P2", "Parent", previous_id="COMP-P"), _row("COMP-K", "Kid", parent_id="COMP-P")] - for parser_format in self.formats: - for order in itertools.permutations(range(2)): - with self.subTest(parser_format=parser_format, order=order): - file_rows = [rows[i] for i in order] - child_row = file_rows.index(rows[1]) + 1 - self._assert_rejected( - file_rows, - parser_format, - f"Row {child_row}'s parent_id (COMP-P) names an id this file renames away.", - "Use 'COMP-P2' as the parent_id.", - ) - - def test_parent_and_child_both_renamed(self) -> None: - """ - S14: renaming a parent and its child together succeeds in either row order, and the - export shows both new ids with the child nested under the parent. - """ - pks = self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", parent_id="COMP-P")]) + pks = self._seed([TagItem(id="COMP-P", value="Parent"), TagItem(id="COMP-K", value="Kid", parent_id="COMP-P")]) rows = [ - _row("COMP-P2", "Parent", previous_id="COMP-P"), - _row("COMP-K2", "Kid", parent_id="COMP-P2", previous_id="COMP-K"), + TagItem(id="COMP-P2", value="Parent", previous_id="COMP-P"), + TagItem(id="COMP-K", value="Kid", parent_id="COMP-P2"), ] for parser_format in self.formats: for order in itertools.permutations(range(2)): @@ -1818,21 +1856,31 @@ def test_parent_and_child_both_renamed(self) -> None: result, _log, _plan = self._import([rows[i] for i in order], parser_format) assert result assert Tag.objects.get(pk=pks["COMP-K"]).parent_id == pks["COMP-P"] - assert self._exported_ids(parser_format) == {"Parent": "COMP-P2", "Kid": "COMP-K2"} - assert self._exported_parents(parser_format) == {"Parent": None, "Kid": "COMP-P2"} + assert self._exported_ids(parser_format)["Parent"] == "COMP-P2" + assert self._exported_parents(parser_format)["Kid"] == "COMP-P2" def test_export_has_no_previous_id_and_round_trips_without_changes(self) -> None: """ - S16: the export only has id, value and parent_id, and importing it back unchanged plans - no change. + #673 scenario "export does not carry previous_id (round-trip safety), CSV and JSON": + the export only has id, value and parent_id, and importing it back unchanged plans no + change. + + Before and after + COMP-P + COMP-K + COMP-A """ - self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", parent_id="COMP-P"), _row("COMP-A", "Alpha")]) + self._seed([ + TagItem(id="COMP-P", value="Parent"), + TagItem(id="COMP-K", value="Kid", parent_id="COMP-P"), + TagItem(id="COMP-A", value="Alpha"), + ]) for parser_format in self.formats: with self.subTest(parser_format=parser_format): exported = self._exported_rows(parser_format) assert len(exported) == 3 result, _log, plan = self._import( - [{key: value for key, value in row.items() if value} for row in exported], + [TagItem(id=row["id"], value=row["value"], parent_id=row["parent_id"]) for row in exported], parser_format, plan_only=True, ) @@ -1840,101 +1888,110 @@ def test_export_has_no_previous_id_and_round_trips_without_changes(self) -> None assert plan is not None assert {action.name for action in plan.actions} == {"without_changes"} - def test_swap_with_children_in_representative_orders(self) -> None: - """ - B, C: in a swap where each parent has a child, children naming the end-state ids succeed - and stay on their physical parents, and children naming the stale ids are rejected with - their row positions, in several row orders and both formats. - """ - pks = self._seed([ - _row("COMP-A", "Alpha"), _row("COMP-AK", "Alpha kid", parent_id="COMP-A"), - _row("COMP-B", "Beta"), _row("COMP-BK", "Beta kid", parent_id="COMP-B"), - ]) - swap = [_row("COMP-B", "Alpha", previous_id="COMP-A"), _row("COMP-A", "Beta", previous_id="COMP-B")] - accepted = swap + [ - _row("COMP-AK", "Alpha kid", parent_id="COMP-B"), _row("COMP-BK", "Beta kid", parent_id="COMP-A"), - ] - stale = swap + [ - _row("COMP-AK", "Alpha kid", parent_id="COMP-A"), _row("COMP-BK", "Beta kid", parent_id="COMP-B"), - ] - orders = [(0, 1, 2, 3), (2, 3, 0, 1), (2, 0, 3, 1), (3, 2, 1, 0)] - for parser_format in self.formats: - for order in orders: - with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): - result, _log, _plan = self._import([accepted[i] for i in order], parser_format) - assert result - assert Tag.objects.get(pk=pks["COMP-AK"]).parent_id == pks["COMP-A"] - assert Tag.objects.get(pk=pks["COMP-BK"]).parent_id == pks["COMP-B"] - assert self._exported_parents(parser_format) == { - "Alpha": None, "Beta": None, "Alpha kid": "COMP-B", "Beta kid": "COMP-A", - } - with self.subTest(parser_format=parser_format, order=order, stale=True): - file_rows = [stale[i] for i in order] - self._assert_rejected( - file_rows, - parser_format, - f"Row {file_rows.index(stale[2]) + 1}'s parent_id (COMP-A) names an id this file renames away.", - f"Row {file_rows.index(stale[3]) + 1}'s parent_id (COMP-B) names an id this file renames away.", - ) - def test_every_rejected_row_is_in_the_one_log(self) -> None: """ - Several rejected rows of different kinds each have their message in the same log, since - the wizard shows the log as one block. + Several rejected rows of different kinds each have their message in the same log, + because the wizard shows the log as one block (#673). The row numbers count data + rows, so they are the same in JSON and CSV. + + Before File rows + COMP-P 1: COMP-P2 (previous_id COMP-P) + COMP-K1 2: COMP-K1 (parent_id COMP-P, stale) + COMP-K2 3: COMP-K2 (parent_id COMP-P, stale) + COMP-A 4: COMP-B (previous_id COMP-GHOST, unmatched) """ self._seed([ - _row("COMP-P", "Parent"), - _row("COMP-K1", "Kid 1", parent_id="COMP-P"), - _row("COMP-K2", "Kid 2", parent_id="COMP-P"), - _row("COMP-A", "Alpha"), + TagItem(id="COMP-P", value="Parent"), + TagItem(id="COMP-K1", value="Kid 1", parent_id="COMP-P"), + TagItem(id="COMP-K2", value="Kid 2", parent_id="COMP-P"), + TagItem(id="COMP-A", value="Alpha"), ]) rows = [ - _row("COMP-P2", "Parent", previous_id="COMP-P"), - _row("COMP-K1", "Kid 1", parent_id="COMP-P"), - _row("COMP-K2", "Kid 2", parent_id="COMP-P"), - _row("COMP-B", "Alpha", previous_id="COMP-GHOST"), + TagItem(id="COMP-P2", value="Parent", previous_id="COMP-P"), + TagItem(id="COMP-K1", value="Kid 1", parent_id="COMP-P"), + TagItem(id="COMP-K2", value="Kid 2", parent_id="COMP-P"), + TagItem(id="COMP-B", value="Alpha", previous_id="COMP-GHOST"), ] for parser_format in self.formats: with self.subTest(parser_format=parser_format): self._assert_rejected( rows, parser_format, - "Row 2's parent_id (COMP-P) names an id this file renames away.", - "Row 3's parent_id (COMP-P) names an id this file renames away.", - "COMP-GHOST", + messages=[ + "Row 2's parent_id (COMP-P) names an id this file renames away.", + "Row 3's parent_id (COMP-P) names an id this file renames away.", + "COMP-GHOST", + ], ) def test_rename_onto_freed_id_forming_parent_cycle_rejected(self) -> None: """ - A rename onto an id the same import frees can close a parent cycle through the - renamed tag: a self-parent, and two renames naming each other as parent. Both are - rejected at the plan step with exactly one cycle error, in either row order. + A rename onto an id that the same import frees can close a parent cycle through the + renamed tag. Both cases below are rejected at the Plan step with exactly one error, + the cycle error on the cycle's first row, in either row order. + + Self parent: + Before File rows + COMP-X COMP-X (previous_id COMP-Y, parent_id COMP-X) + COMP-Y + + Two renames naming each other as parent: + Before File rows + COMP-A COMP-A (previous_id COMP-C, parent_id COMP-B) + COMP-B COMP-B (previous_id COMP-D, parent_id COMP-A) + COMP-C + COMP-D """ - self._seed([_row(tag_id, f"Value {tag_id}") for tag_id in ("COMP-X", "COMP-Y")]) - self_parent = [_row("COMP-X", "Value COMP-Y", parent_id="COMP-X", previous_id="COMP-Y")] + self._seed([TagItem(id="COMP-X", value="Xray"), TagItem(id="COMP-Y", value="Yank")]) + self_parent = [TagItem(id="COMP-X", value="Yank", parent_id="COMP-X", previous_id="COMP-Y")] for parser_format in self.formats: with self.subTest(parser_format=parser_format, case="self parent"): - log = self._assert_rejected(self_parent, parser_format, "creates a parent cycle.") - assert log.count("creates a parent cycle.") == 1 - self._seed([_row(tag_id, f"Value {tag_id}") for tag_id in ("COMP-A", "COMP-B", "COMP-C", "COMP-D")]) + self._assert_rejected( + self_parent, + parser_format, + messages=["Row 1's parent_id (COMP-X) creates a parent cycle."], + error_count=1, + ) + self._seed([ + TagItem(id="COMP-A", value="Alpha"), TagItem(id="COMP-B", value="Beta"), + TagItem(id="COMP-C", value="Gamma"), TagItem(id="COMP-D", value="Delta"), + ]) rows = [ - _row("COMP-A", "Value COMP-C", parent_id="COMP-B", previous_id="COMP-C"), - _row("COMP-B", "Value COMP-D", parent_id="COMP-A", previous_id="COMP-D"), + TagItem(id="COMP-A", value="Gamma", parent_id="COMP-B", previous_id="COMP-C"), + TagItem(id="COMP-B", value="Delta", parent_id="COMP-A", previous_id="COMP-D"), ] for parser_format in self.formats: for order in itertools.permutations(range(2)): with self.subTest(parser_format=parser_format, order=order, case="two renames"): - log = self._assert_rejected([rows[i] for i in order], parser_format, "creates a parent cycle.") - assert log.count("creates a parent cycle.") == 1 + file_rows = [rows[i] for i in order] + self._assert_rejected( + file_rows, + parser_format, + messages=[f"Row 1's parent_id ({file_rows[0].parent_id}) creates a parent cycle."], + error_count=1, + ) def test_child_of_deleted_holder_moves_under_tag_renamed_onto_its_id(self) -> None: """ - A child of a tag that the import deletes, while another tag is renamed onto the - deleted tag's id, ends up under the renamed tag, not as a root, in either row order. + The file omits COMP-X, so COMP-X is deleted, and renames COMP-Y onto the id COMP-X. + The child of the deleted tag names COMP-X, so it ends up under the renamed tag, not + as a root, in either row order. + + Before After + COMP-X COMP-X (was COMP-Y) + COMP-C COMP-C + COMP-Y """ - pks = self._seed([_row("COMP-X", "Xray"), _row("COMP-Y", "Yank"), _row("COMP-C", "Child", parent_id="COMP-X")]) + pks = self._seed([ + TagItem(id="COMP-X", value="Xray"), + TagItem(id="COMP-Y", value="Yank"), + TagItem(id="COMP-C", value="Child", parent_id="COMP-X"), + ]) object_tags = self._associate(pks, "COMP-C") - rows = [_row("COMP-X", "Yank", previous_id="COMP-Y"), _row("COMP-C", "Child", parent_id="COMP-X")] + rows = [ + TagItem(id="COMP-X", value="Yank", previous_id="COMP-Y"), + TagItem(id="COMP-C", value="Child", parent_id="COMP-X"), + ] for parser_format in self.formats: for order in itertools.permutations(range(2)): with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): @@ -1948,46 +2005,70 @@ def test_child_of_deleted_holder_moves_under_tag_renamed_onto_its_id(self) -> No def test_kept_tags_under_deleted_ancestors_become_roots(self) -> None: """ - A kept tag whose parent is deleted by the import becomes a root, also when that - parent sits under a deleted ancestor, which would otherwise cascade to the kept tag. + A kept tag whose parent the import deletes becomes a root, also when that parent sits + under a deleted ancestor, whose delete would otherwise cascade to the kept tag. + + Before (three cases) After + deleted_root > deleted_child > kept_tag kept_tag + deleted_1 > deleted_2 > kept_upper > deleted_3 kept_upper + > kept_lower kept_lower + deleted_root > kept_child kept_child """ cases = { "grandchild of a deleted chain": ( - [("t0", None), ("t1", "t0"), ("t5", "t1")], ["t5"], + [ + TagItem(id="deleted_root", value="Deleted root"), + TagItem(id="deleted_child", value="Deleted child", parent_id="deleted_root"), + TagItem(id="kept_tag", value="Kept tag", parent_id="deleted_child"), + ], + ["kept_tag"], ), "kept tags at two depths": ( - [("t0", None), ("t1", "t0"), ("t2", "t1"), ("t3", "t2"), ("t4", "t3")], ["t2", "t4"], + [ + TagItem(id="deleted_1", value="Deleted 1"), + TagItem(id="deleted_2", value="Deleted 2", parent_id="deleted_1"), + TagItem(id="kept_upper", value="Kept upper", parent_id="deleted_2"), + TagItem(id="deleted_3", value="Deleted 3", parent_id="kept_upper"), + TagItem(id="kept_lower", value="Kept lower", parent_id="deleted_3"), + ], + ["kept_upper", "kept_lower"], + ), + "child of a deleted root": ( + [ + TagItem(id="deleted_root", value="Deleted root"), + TagItem(id="kept_child", value="Kept child", parent_id="deleted_root"), + ], + ["kept_child"], ), - "child of a deleted root": ([("t0", None), ("t1", "t0")], ["t1"]), } - for name, (structure, kept) in cases.items(): + for name, (before, kept_ids) in cases.items(): + # The file lists only the kept tags, without a parent. + kept = [TagItem(id=tag.id, value=tag.value) for tag in before if tag.id in kept_ids] for parser_format in self.formats: with self.subTest(case=name, parser_format=parser_format), self._rolled_back(): - pks = self._seed([_row(tag_id, f"Value {tag_id}", parent) for tag_id, parent in structure]) - result, _log, _plan = self._import( - [_row(tag_id, f"Value {tag_id}") for tag_id in kept], parser_format, - ) + pks = self._seed(before) + result, _log, _plan = self._import(kept, parser_format) assert result - assert set(self.taxonomy.tag_set.values_list("pk", flat=True)) == {pks[t] for t in kept} + assert set(self.taxonomy.tag_set.values_list("pk", flat=True)) == {pks[t.id] for t in kept} assert all(tag.parent_id is None for tag in self.taxonomy.tag_set.all()) - assert self._exported_parents(parser_format) == {f"Value {t}": None for t in kept} + assert self._exported_parents(parser_format) == {t.value: None for t in kept} def _assert_tree_after_import( self, - rows: list[dict], + *, + rows: list[TagItem], expected_parents: dict[str, str | None], pks: dict[str, int], orders: Sequence[tuple[int, ...]], parser_formats: tuple[ParserFormat, ...], - *, - static_rows: tuple[dict, ...] = (), + static_rows: tuple[TagItem, ...] = (), ) -> None: """ - Import `rows` in each row order and format, followed by `static_rows` (the other tags - the replace import has to keep), and check that every tag in - `expected_parents` (external_id to parent external_id) ends up under that parent in the - database and the export, and that the tags in `pks` keep their pk. Each import is - rolled back. + Import ``rows`` in each of ``orders`` and each of ``parser_formats``, followed by + ``static_rows`` (the other tags that the replace import has to keep). Check that every + tag in ``expected_parents`` (external_id to parent external_id) ends up under that + parent in the database and the export, and that each tag in ``pks`` (external_id + after the import to pk) keeps its pk. Each import is rolled back. """ for parser_format in parser_formats: for order in orders: @@ -2006,163 +2087,351 @@ def _assert_tree_after_import( def test_ancestry_inversion_in_any_row_order(self) -> None: """ - A child of t0 becomes t0's ancestor (a new tag g0 is put between them), which only - works if it moves out of t0's subtree first. It succeeds in all 24 row orders in JSON and - in four of them in CSV, and ends with the expected tree. + A child of old_root becomes old_root's ancestor, with a new tag between them. That + only works if new_root leaves old_root's subtree before old_root moves under it. + + Before After + old_root new_root + sibling new_middle + new_root old_root + sibling """ - pks = self._seed([_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t0")]) - rows = [_row("t0", "Zero", "g0"), _row("t1", "One", "g0"), _row("t2", "Two"), _row("g0", "New", "t2")] - expected = {"t0": "g0", "t1": "g0", "t2": None, "g0": "t2"} + pks = self._seed([ + TagItem(id="old_root", value="Old root"), + TagItem(id="sibling", value="Sibling", parent_id="old_root"), + TagItem(id="new_root", value="New root", parent_id="old_root"), + ]) + rows = [ + TagItem(id="old_root", value="Old root", parent_id="new_middle"), + TagItem(id="sibling", value="Sibling", parent_id="new_middle"), + TagItem(id="new_root", value="New root"), + TagItem(id="new_middle", value="New middle", parent_id="new_root"), + ] + expected = {"old_root": "new_middle", "sibling": "new_middle", "new_root": None, "new_middle": "new_root"} orders = list(itertools.permutations(range(4))) - self._assert_tree_after_import(rows, expected, pks, orders, (ParserFormat.JSON,)) - self._assert_tree_after_import(rows, expected, pks, orders[::6], (ParserFormat.CSV,)) + # All 24 row orders in JSON. The CSV parser yields the same rows in the same order, so + # 4 orders in CSV are enough and keep the test fast. + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders, parser_formats=(ParserFormat.JSON,), + ) + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders[::6], parser_formats=(ParserFormat.CSV,), + ) def test_parent_and_child_swap_places(self) -> None: """ - A parent and its child swap places in either row order. + A parent and its child swap places, in either row order. + + Before After + old_parent old_child + old_child old_parent """ - pks = self._seed([_row("t0", "Zero"), _row("t1", "One", "t0")]) - rows = [_row("t0", "Zero", "t1"), _row("t1", "One")] - orders = list(itertools.permutations(range(2))) - self._assert_tree_after_import(rows, {"t0": "t1", "t1": None}, pks, orders, self.formats) + pks = self._seed([ + TagItem(id="old_parent", value="Old parent"), + TagItem(id="old_child", value="Old child", parent_id="old_parent"), + ]) + rows = [ + TagItem(id="old_parent", value="Old parent", parent_id="old_child"), + TagItem(id="old_child", value="Old child"), + ] + self._assert_tree_after_import( + rows=rows, + expected_parents={"old_parent": "old_child", "old_child": None}, + pks=pks, + orders=list(itertools.permutations(range(2))), + parser_formats=self.formats, + ) def test_three_level_chain_reversed(self) -> None: """ - A chain t0 > t1 > t2 is reversed to t2 > t1 > t0 in all six row orders. + A three-level chain is turned upside down, in all six row orders. + + Before After + top bottom + middle middle + bottom top """ - pks = self._seed([_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1")]) - rows = [_row("t2", "Two"), _row("t1", "One", "t2"), _row("t0", "Zero", "t1")] - expected = {"t2": None, "t1": "t2", "t0": "t1"} - orders = list(itertools.permutations(range(3))) - self._assert_tree_after_import(rows, expected, pks, orders, self.formats) + pks = self._seed([ + TagItem(id="top", value="Top"), + TagItem(id="middle", value="Middle", parent_id="top"), + TagItem(id="bottom", value="Bottom", parent_id="middle"), + ]) + rows = [ + TagItem(id="bottom", value="Bottom"), + TagItem(id="middle", value="Middle", parent_id="bottom"), + TagItem(id="top", value="Top", parent_id="middle"), + ] + self._assert_tree_after_import( + rows=rows, + expected_parents={"bottom": None, "middle": "bottom", "top": "middle"}, + pks=pks, + orders=list(itertools.permutations(range(3))), + parser_formats=self.formats, + ) def test_tag_moved_under_its_former_grandchild(self) -> None: """ - A root t0 is moved under t2, its former grandchild, which is left a child of t1 - (now a root), in all six row orders. - """ - pks = self._seed([_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1")]) - rows = [_row("t0", "Zero", "t2"), _row("t1", "One"), _row("t2", "Two", "t1")] - expected = {"t0": "t2", "t1": None, "t2": "t1"} - orders = list(itertools.permutations(range(3))) - self._assert_tree_after_import(rows, expected, pks, orders, self.formats) + A root is moved under its former grandchild, which stays under middle, now a root, + in all six row orders. - def _seed_deep_chain(self) -> dict[str, int]: + Before After + top middle + middle bottom + bottom top """ - Create the chain t0 > t1 > t2 > t3 > t5 and return the pks by external_id. - """ - return self._seed([ - _row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1"), - _row("t3", "Three", "t2"), _row("t5", "Five", "t3"), + pks = self._seed([ + TagItem(id="top", value="Top"), + TagItem(id="middle", value="Middle", parent_id="top"), + TagItem(id="bottom", value="Bottom", parent_id="middle"), ]) + rows = [ + TagItem(id="top", value="Top", parent_id="bottom"), + TagItem(id="middle", value="Middle"), + TagItem(id="bottom", value="Bottom", parent_id="middle"), + ] + self._assert_tree_after_import( + rows=rows, + expected_parents={"top": "bottom", "middle": None, "bottom": "middle"}, + pks=pks, + orders=list(itertools.permutations(range(3))), + parser_formats=self.formats, + ) + + # The chain level_1 > level_2 > level_3 > level_4 > deep_tag: deep_tag is at the fifth + # level, so a child of deep_tag is at the sixth level, the deepest the taxonomy allows. + deep_chain = ( + TagItem(id="level_1", value="Level 1"), + TagItem(id="level_2", value="Level 2", parent_id="level_1"), + TagItem(id="level_3", value="Level 3", parent_id="level_2"), + TagItem(id="level_4", value="Level 4", parent_id="level_3"), + ) + deep_tag = TagItem(id="deep_tag", value="Deep tag", parent_id="level_4") def test_deep_tag_moved_to_root_with_new_children_in_any_row_order(self) -> None: """ - t5 is the deepest tag of a chain and moves to the root while new tags g0 and g1 are put - under it. Creating them first would exceed the maximum depth, so the import detaches t5 - first. It succeeds in all six row orders in JSON and in three of them in CSV. + The deepest tag of a chain moves to the root while new tags are put under it. + Creating new_grandchild before deep_tag moves would put it at a seventh level, so the + import moves deep_tag to the root first. + + Before After + level_1 > ... > level_4 level_1 > ... > level_4 + deep_tag deep_tag + new_child + new_grandchild """ - pks = self._seed_deep_chain() - rows = [_row("g1", "New 1", "g0"), _row("g0", "New 0", "t5"), _row("t5", "Five")] - chain = (_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1"), _row("t3", "Three", "t2")) - expected = {"t0": None, "t1": "t0", "t2": "t1", "t3": "t2", "t5": None, "g0": "t5", "g1": "g0"} + pks = self._seed([*self.deep_chain, self.deep_tag]) + rows = [ + TagItem(id="new_grandchild", value="New grandchild", parent_id="new_child"), + TagItem(id="new_child", value="New child", parent_id="deep_tag"), + TagItem(id="deep_tag", value="Deep tag"), + ] + expected = { + "level_1": None, "level_2": "level_1", "level_3": "level_2", "level_4": "level_3", + "deep_tag": None, "new_child": "deep_tag", "new_grandchild": "new_child", + } orders = list(itertools.permutations(range(3))) - self._assert_tree_after_import(rows, expected, pks, orders, (ParserFormat.JSON,), static_rows=chain) - self._assert_tree_after_import(rows, expected, pks, orders[::2], (ParserFormat.CSV,), static_rows=chain) + # All 6 row orders in JSON. The CSV parser yields the same rows in the same order, so + # 3 orders in CSV are enough and keep the test fast. + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders, + parser_formats=(ParserFormat.JSON,), static_rows=self.deep_chain, + ) + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders[::2], + parser_formats=(ParserFormat.CSV,), static_rows=self.deep_chain, + ) def test_deep_tag_under_renamed_parent_moved_to_root_in_any_row_order(self) -> None: """ - The same move when t5's old parent t3 is renamed in the same import: all 24 row orders - in JSON succeed and t5 keeps its pk. + The same move as above while deep_tag's old parent is renamed in the same import. + deep_tag keeps its pk. + + Before After + level_1 > level_2 > level_3 level_1 > level_2 > level_3 + level_4 renamed_level_4 (was level_4) + deep_tag deep_tag + new_child + new_grandchild """ - pks = self._seed_deep_chain() + pks = self._seed([*self.deep_chain, self.deep_tag]) rows = [ - _row("g1", "New 1", "g0"), _row("g0", "New 0", "t5"), _row("t5", "Five"), - _row("r3", "Three", "t2", "t3"), + TagItem(id="new_grandchild", value="New grandchild", parent_id="new_child"), + TagItem(id="new_child", value="New child", parent_id="deep_tag"), + TagItem(id="deep_tag", value="Deep tag"), + TagItem(id="renamed_level_4", value="Level 4", parent_id="level_3", previous_id="level_4"), ] - expected = {"t0": None, "t1": "t0", "t2": "t1", "r3": "t2", "t5": None, "g0": "t5", "g1": "g0"} - orders = list(itertools.permutations(range(4))) - kept_pks = {tag_id: pk for tag_id, pk in pks.items() if tag_id != "t3"} | {"r3": pks["t3"]} - static = (_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1")) - self._assert_tree_after_import(rows, expected, kept_pks, orders, (ParserFormat.JSON,), static_rows=static) + expected = { + "level_1": None, "level_2": "level_1", "level_3": "level_2", "renamed_level_4": "level_3", + "deep_tag": None, "new_child": "deep_tag", "new_grandchild": "new_child", + } + kept_pks = {tag_id: pk for tag_id, pk in pks.items() if tag_id != "level_4"} | { + "renamed_level_4": pks["level_4"], + } + # All 24 row orders, in JSON only: the test above already runs this kind of move + # through the CSV parser. + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=kept_pks, orders=list(itertools.permutations(range(4))), + parser_formats=(ParserFormat.JSON,), static_rows=self.deep_chain[:3], + ) def test_subtree_moved_under_deep_tag_before_it_is_shallowed(self) -> None: """ - A three-level subtree s0 > s1 > s2 moves under c3, which itself moves to the root, in - every row order and both formats. - """ + A three-level subtree moves under moved_up, which itself moves to the root. Attaching + subtree_root while moved_up is still at the fourth level would put + subtree_grandchild at a seventh level. + + Before After + chain_1 > chain_2 > chain_3 chain_1 > chain_2 > chain_3 + moved_up moved_up + subtree_root subtree_root + subtree_child subtree_child + subtree_grandchild subtree_grandchild + """ + chain = ( + TagItem(id="chain_1", value="Chain 1"), + TagItem(id="chain_2", value="Chain 2", parent_id="chain_1"), + TagItem(id="chain_3", value="Chain 3", parent_id="chain_2"), + ) pks = self._seed([ - _row("c0", "C0"), _row("c1", "C1", "c0"), _row("c2", "C2", "c1"), _row("c3", "C3", "c2"), - _row("s0", "S0"), _row("s1", "S1", "s0"), _row("s2", "S2", "s1"), + *chain, + TagItem(id="moved_up", value="Moved up", parent_id="chain_3"), + TagItem(id="subtree_root", value="Subtree root"), + TagItem(id="subtree_child", value="Subtree child", parent_id="subtree_root"), + TagItem(id="subtree_grandchild", value="Subtree grandchild", parent_id="subtree_child"), ]) - rows = [_row("s0", "S0", "c3"), _row("c3", "C3"), _row("s1", "S1", "s0"), _row("s2", "S2", "s1")] - expected = {"c0": None, "c1": "c0", "c2": "c1", "c3": None, "s0": "c3", "s1": "s0", "s2": "s1"} + rows = [ + TagItem(id="subtree_root", value="Subtree root", parent_id="moved_up"), + TagItem(id="moved_up", value="Moved up"), + TagItem(id="subtree_child", value="Subtree child", parent_id="subtree_root"), + TagItem(id="subtree_grandchild", value="Subtree grandchild", parent_id="subtree_child"), + ] + expected = { + "chain_1": None, "chain_2": "chain_1", "chain_3": "chain_2", "moved_up": None, + "subtree_root": "moved_up", "subtree_child": "subtree_root", "subtree_grandchild": "subtree_child", + } orders = list(itertools.permutations(range(4))) - static = (_row("c0", "C0"), _row("c1", "C1", "c0"), _row("c2", "C2", "c1")) - self._assert_tree_after_import(rows, expected, pks, orders, (ParserFormat.JSON,), static_rows=static) - self._assert_tree_after_import(rows, expected, pks, orders[::6], (ParserFormat.CSV,), static_rows=static) + # All 24 row orders in JSON. The CSV parser yields the same rows in the same order, so + # 4 orders in CSV are enough and keep the test fast. + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders, + parser_formats=(ParserFormat.JSON,), static_rows=chain, + ) + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders[::6], + parser_formats=(ParserFormat.CSV,), static_rows=chain, + ) def test_three_new_levels_under_moved_deep_tag(self) -> None: """ - Three new tags chained under t5, which moves to the root, succeed in all 24 row orders. + Three new tags are chained under deep_tag, which moves to the root. + + Before After + level_1 > ... > level_4 level_1 > ... > level_4 + deep_tag deep_tag + new_child + new_grandchild + new_great_grandchild """ - pks = self._seed_deep_chain() + pks = self._seed([*self.deep_chain, self.deep_tag]) rows = [ - _row("t5", "Five"), _row("g0", "New 0", "t5"), _row("g1", "New 1", "g0"), _row("g2", "New 2", "g1"), + TagItem(id="deep_tag", value="Deep tag"), + TagItem(id="new_child", value="New child", parent_id="deep_tag"), + TagItem(id="new_grandchild", value="New grandchild", parent_id="new_child"), + TagItem(id="new_great_grandchild", value="New great-grandchild", parent_id="new_grandchild"), ] expected = { - "t0": None, "t1": "t0", "t2": "t1", "t3": "t2", "t5": None, "g0": "t5", "g1": "g0", "g2": "g1", + "level_1": None, "level_2": "level_1", "level_3": "level_2", "level_4": "level_3", "deep_tag": None, + "new_child": "deep_tag", "new_grandchild": "new_child", "new_great_grandchild": "new_grandchild", } - orders = list(itertools.permutations(range(4))) - chain = (_row("t0", "Zero"), _row("t1", "One", "t0"), _row("t2", "Two", "t1"), _row("t3", "Three", "t2")) - self._assert_tree_after_import(rows, expected, pks, orders, (ParserFormat.JSON,), static_rows=chain) + # All 24 row orders, in JSON only: the moves of a deep tag above already run through + # the CSV parser. + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=list(itertools.permutations(range(4))), + parser_formats=(ParserFormat.JSON,), static_rows=self.deep_chain, + ) def test_import_that_leaves_tags_too_deep_is_rejected(self) -> None: """ - An import that would put a tag at the seventh level, by adding a child to the deepest - tag of a six-level chain or by moving a chain under another, cannot be applied in any - order, so it is rejected at the plan step and nothing changes. + An import that would put a tag at the seventh level cannot be applied in any order, + so it is rejected at the Plan step with that one error, and nothing changes. + + Child of the deepest tag: + Before File rows + level_1 > ... > level_6 the same six tags, and new_leaf (parent_id level_6) + + Chain moved under another: + Before File rows + deep_1 > ... > deep_4 the same eight tags, with moved_1 (parent_id deep_4) + moved_1 > ... > moved_4 """ - chain = [("a", None), ("b", "a"), ("c", "b"), ("d", "c"), ("e", "d"), ("f", "e")] - two_chains = [("a", None), ("b", "a"), ("c", "b"), ("d", "c"), ("w", None), ("x", "w"), ("y", "x"), ("z", "y")] + def chain(prefix: str, length: int) -> list[TagItem]: + return [ + TagItem(id=f"{prefix}_{i}", value=f"{prefix} {i}", parent_id=f"{prefix}_{i - 1}" if i > 1 else None) + for i in range(1, length + 1) + ] + + six_levels = chain("level", 6) + deep, moved = chain("deep", 4), chain("moved", 4) cases = { "child of the deepest tag": ( - chain, [_row(tag_id, f"Value {tag_id}", parent) for tag_id, parent in chain] - + [_row("g", "Value g", "f")], + six_levels, six_levels + [TagItem(id="new_leaf", value="New leaf", parent_id="level_6")], ), "chain moved under another": ( - two_chains, - [_row(t, f"Value {t}", "d" if t == "w" else parent) for t, parent in two_chains], + deep + moved, deep + [TagItem(id="moved_1", value="moved 1", parent_id="deep_4")] + moved[1:], ), } message = "This import would leave tags deeper than 6 levels" - for name, (structure, rows) in cases.items(): - self._seed([_row(tag_id, f"Value {tag_id}", parent) for tag_id, parent in structure]) + for name, (before, rows) in cases.items(): + self._seed(before) for parser_format in self.formats: with self.subTest(case=name, parser_format=parser_format): - log = self._assert_rejected(rows, parser_format, message) + log = self._assert_rejected(rows, parser_format, messages=[message], error_count=1) assert log.count(message) == 1 self.taxonomy.tag_set.all().delete() def test_swap_child_naming_parent_id_in_another_case(self) -> None: """ - In a swap, ids are case-insensitive: a child naming its parent's stale id in another case - is rejected once, with its row position and the new id, like the exact spelling. A child - naming the end-state ids in another case is accepted and stays under its parent. + Ids are case-insensitive. In a swap, a child naming its parent's stale id in another + case is rejected once, with its row number and the parent's new id, like the exact + spelling. Children naming the end-state ids in another case are accepted and stay + under their parents. + + Before After (accepted file) + COMP-A COMP-B (was COMP-A) + COMP-AK COMP-AK (parent_id comp-b) + COMP-B COMP-A (was COMP-B) + COMP-BK COMP-BK (parent_id comp-a) + + In the rejected file, COMP-AK says parent_id comp-a, the stale id of its parent. """ pks = self._seed([ - _row("COMP-A", "Alpha"), _row("COMP-AK", "Alpha kid", "COMP-A"), - _row("COMP-B", "Beta"), _row("COMP-BK", "Beta kid", "COMP-B"), + TagItem(id="COMP-A", value="Alpha"), + TagItem(id="COMP-AK", value="Alpha kid", parent_id="COMP-A"), + TagItem(id="COMP-B", value="Beta"), + TagItem(id="COMP-BK", value="Beta kid", parent_id="COMP-B"), ]) - swap = [_row("COMP-B", "Alpha", previous_id="COMP-A"), _row("COMP-A", "Beta", previous_id="COMP-B")] - stale = swap + [_row("COMP-AK", "Alpha kid", "comp-a"), _row("COMP-BK", "Beta kid", "comp-a")] - accepted = swap + [_row("COMP-AK", "Alpha kid", "comp-b"), _row("COMP-BK", "Beta kid", "comp-a")] + swap = [ + TagItem(id="COMP-B", value="Alpha", previous_id="COMP-A"), + TagItem(id="COMP-A", value="Beta", previous_id="COMP-B"), + ] + stale = swap + [ + TagItem(id="COMP-AK", value="Alpha kid", parent_id="comp-a"), + TagItem(id="COMP-BK", value="Beta kid", parent_id="comp-a"), + ] + accepted = swap + [ + TagItem(id="COMP-AK", value="Alpha kid", parent_id="comp-b"), + TagItem(id="COMP-BK", value="Beta kid", parent_id="comp-a"), + ] expected = {"COMP-B": None, "COMP-A": None, "COMP-AK": "COMP-B", "COMP-BK": "COMP-A"} moved_pks = { "COMP-B": pks["COMP-A"], "COMP-A": pks["COMP-B"], "COMP-AK": pks["COMP-AK"], "COMP-BK": pks["COMP-BK"], } + # A sample of 4 row orders (renames first, children first, reversed, mixed): the + # tests of the exact spelling already run all 24 orders. orders = [(0, 1, 2, 3), (2, 3, 0, 1), (3, 2, 1, 0), (2, 0, 3, 1)] - self._assert_tree_after_import(accepted, expected, moved_pks, orders, self.formats) + self._assert_tree_after_import( + rows=accepted, expected_parents=expected, pks=moved_pks, orders=orders, parser_formats=self.formats, + ) for parser_format in self.formats: for order in orders: with self.subTest(parser_format=parser_format, order=order, stale=True): @@ -2171,38 +2440,35 @@ def test_swap_child_naming_parent_id_in_another_case(self) -> None: log = self._assert_rejected( file_rows, parser_format, - f"Row {position}'s parent_id (comp-a) names an id this file renames away.", - "Use 'COMP-B' as the parent_id.", + messages=[ + f"Row {position}'s parent_id (comp-a) names an id this file renames away.", + "Use 'COMP-B' as the parent_id.", + ], ) assert log.count("names an id this file renames away") == 1 def test_plain_rename_child_naming_parent_id_in_another_case_reported_once(self) -> None: """ - When a parent is plainly renamed, a child naming its old id in another case gets only - the parent validation error, in either row order and format. + When the parent is renamed without a swap, a child naming the parent's old id in + another case gets only the parent validation error, in either row order and format. + + Before File rows + COMP-P COMP-P2 (previous_id COMP-P) + COMP-K COMP-K (parent_id comp-p) """ - self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", "COMP-P")]) - rows = [_row("COMP-P2", "Parent", previous_id="COMP-P"), _row("COMP-K", "Kid", "comp-p")] + self._seed([TagItem(id="COMP-P", value="Parent"), TagItem(id="COMP-K", value="Kid", parent_id="COMP-P")]) + rows = [ + TagItem(id="COMP-P2", value="Parent", previous_id="COMP-P"), + TagItem(id="COMP-K", value="Kid", parent_id="comp-p"), + ] for parser_format in self.formats: for order in itertools.permutations(range(2)): with self.subTest(parser_format=parser_format, order=order): log = self._assert_rejected( [rows[i] for i in order], parser_format, - "Parent tag comp-p is renamed to COMP-P2 in this file; use COMP-P2 as the parent_id.", + messages=[ + "Parent tag comp-p is renamed to COMP-P2 in this file; use COMP-P2 as the parent_id.", + ], ) assert "names an id this file renames away" not in log - - def test_case_only_parent_rename_with_case_variant_child(self) -> None: - """ - Renaming a parent only by case, a child naming it in another case stays under it. - """ - pks = self._seed([_row("COMP-P", "Parent"), _row("COMP-K", "Kid", "COMP-P")]) - for child_parent in ("comp-p", "Comp-P"): - rows = [_row("Comp-P", "Parent", previous_id="COMP-P"), _row("COMP-K", "Kid", child_parent)] - expected = {"Comp-P": None, "COMP-K": "Comp-P"} - kept_pks = {"Comp-P": pks["COMP-P"], "COMP-K": pks["COMP-K"]} - with self.subTest(child_parent=child_parent): - self._assert_tree_after_import( - rows, expected, kept_pks, list(itertools.permutations(range(2))), self.formats, - ) diff --git a/tests/openedx_tagging/import_export/test_import_plan.py b/tests/openedx_tagging/import_export/test_import_plan.py index 88b504f75..b21fd8706 100644 --- a/tests/openedx_tagging/import_export/test_import_plan.py +++ b/tests/openedx_tagging/import_export/test_import_plan.py @@ -9,7 +9,7 @@ from django.test.testcases import TestCase from django.test.utils import CaptureQueriesContext -from openedx_tagging.import_export.actions import CreateTag, DeleteTag, RenameTagExternalId, UpdateParentTag +from openedx_tagging.import_export.actions import CreateTag, DeleteTag, UpdateParentTag from openedx_tagging.import_export.exceptions import TagImportError from openedx_tagging.import_export.import_plan import ExistingTag, TagImportPlan, TagItem, _ParentForest from openedx_tagging.models import Tag, Taxonomy @@ -634,10 +634,15 @@ def test_generate_actions_parent_id_new_id_after_earlier_rename_accepted(self) - def test_generate_actions_existing_child_names_renamed_away_parent_rejected(self) -> None: """ - tag_2 is an existing child of tag_1. tag_1 is renamed to tag_50 and - tag_2's row keeps parent_id="tag_1", the pre-import id. The row is - "no change" for UpdateParentTag, so only the plan-level check can - reject it, and the error names the row and the new id. + An existing child that keeps its parent's pre-import id is rejected when the file + renames that parent, and the error names the row and the parent's new id. + + Before File rows + tag_1 tag_50 (previous_id tag_1) + tag_2 tag_2 (parent_id tag_1) + + The tag_2 row is "no change" for UpdateParentTag, so only the plan-level check can + reject it. """ tags = [ TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), @@ -650,10 +655,16 @@ def test_generate_actions_existing_child_names_renamed_away_parent_rejected(self def test_generate_actions_swap_rename_child_stale_parent_id_rejected_once(self) -> None: """ - In a tag_1 <-> tag_3 swap, a rename row (tag_2 -> tag_20) that names - the stale parent id "tag_1" is reported exactly once. The rename - action's own parent validation lets it through because "tag_1" is - also a rename row's final id, so the plan-level check reports it. + In a swap, a child that is itself renamed and names its parent's stale id is reported + exactly once. + + Before File rows + tag_1 tag_3 (previous_id tag_1) + tag_2 tag_1 (previous_id tag_3) + tag_3 tag_20 (previous_id tag_2, parent_id tag_1) + + The rename action's own parent validation accepts "tag_1", because it is also another + rename row's final id, so the plan-level check reports it. """ tags = [ TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), @@ -666,9 +677,12 @@ def test_generate_actions_swap_rename_child_stale_parent_id_rejected_once(self) def test_generate_actions_rename_child_stale_parent_id_without_swap_not_duplicated(self) -> None: """ - Without a swap, a rename row (tag_2 -> tag_20) that names the stale - parent id "tag_1" is reported only by the rename action's parent - validation, not a second time by the plan-level check. + Without a swap, a renamed child that names its parent's stale id is reported only by + the rename action's parent validation, not a second time by the plan-level check. + + Before File rows + tag_1 tag_50 (previous_id tag_1) + tag_2 tag_20 (previous_id tag_2, parent_id tag_1) """ tags = [ TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), @@ -683,9 +697,12 @@ def test_generate_actions_rename_child_stale_parent_id_without_swap_not_duplicat def test_generate_actions_case_only_parent_rename_not_flagged(self) -> None: """ - A case-only rename of the parent (tag_1 -> TAG_1) keeps the id - resolving to the same tag, so a child that keeps parent_id="tag_1" - is not stale. + A rename that only changes the parent's capitalization keeps the id pointing at the + same tag, so a child that keeps parent_id "tag_1" is accepted. + + Before After + tag_1 TAG_1 + tag_2 tag_2 """ tags = [ TagItem(id='TAG_1', value='Tag 1', previous_id='tag_1'), @@ -696,8 +713,14 @@ def test_generate_actions_case_only_parent_rename_not_flagged(self) -> None: def test_generate_actions_swap_children_accepted_in_any_row_order(self) -> None: """ - The tag_1 <-> tag_3 swap with children on end-state ids is accepted - in every one of the 24 row orders. + Children that name the end-state ids of swapped parents are accepted in every row + order. Each child stays on the same physical parent. + + Before After + tag_1 tag_3 (was tag_1) + tag_2 tag_2 + tag_3 tag_1 (was tag_3) + tag_4 tag_4 """ rows = [ TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), @@ -705,6 +728,7 @@ def test_generate_actions_swap_children_accepted_in_any_row_order(self) -> None: TagItem(id='tag_2', value='Tag 2', parent_id='tag_3'), TagItem(id='tag_4', value='Tag 4', parent_id='tag_1'), ] + # Planning is fast, so every one of the 24 row orders is checked. for order in itertools.permutations(range(4)): with self.subTest(order=order): self.import_plan.generate_actions(tags=[rows[i] for i in order], replace=False) @@ -712,8 +736,14 @@ def test_generate_actions_swap_children_accepted_in_any_row_order(self) -> None: def test_generate_actions_swap_stale_ids_rejected_in_any_row_order(self) -> None: """ - Children that name the stale pre-import ids are rejected in every one - of the 24 row orders, and each error reports the row's file position. + Children that name the stale pre-import ids of swapped parents are rejected in every + row order. Each error names the child's row number and its parent's new id. + + Before File rows + tag_1 tag_1 (previous_id tag_3) + tag_2 tag_3 (previous_id tag_1) + tag_3 tag_2 (parent_id tag_1) + tag_4 tag_4 (parent_id tag_3) """ rows = [ TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), @@ -721,6 +751,7 @@ def test_generate_actions_swap_stale_ids_rejected_in_any_row_order(self) -> None TagItem(id='tag_2', value='Tag 2', parent_id='tag_1'), TagItem(id='tag_4', value='Tag 4', parent_id='tag_3'), ] + # Planning is fast, so every one of the 24 row orders is checked. for order in itertools.permutations(range(4)): with self.subTest(order=order): file_rows = [rows[i] for i in order] @@ -735,53 +766,37 @@ def test_generate_actions_swap_stale_ids_rejected_in_any_row_order(self) -> None self.assertEqual(len(matching), 1) self.assertIn(f"Use '{new_ids[row.parent_id]}' as the parent_id.", matching[0]) - def test_generate_actions_new_child_before_new_parent_accepted(self) -> None: - """ - A new child row listed before its new parent row is accepted, and the - plan creates the parent first. - """ - tags = [ - TagItem(id='tag_90', value='Tag 90', parent_id='tag_91'), - TagItem(id='tag_91', value='Tag 91'), - ] - self.import_plan.generate_actions(tags=tags, replace=False) - self.assertEqual(self.import_plan.errors, []) - self.assertEqual([action.tag.id for action in self.import_plan.actions], ['tag_91', 'tag_90']) - def test_generate_actions_valid_row_order_keeps_action_order(self) -> None: """ - A file whose parents already come before their children keeps its - row order in the plan. + A file whose parents already come before their children keeps its row order in the + plan. + + Before After + tag_1 new_parent + new_child + tag_50 (was tag_1) + renamed_tag_child """ tags = [ - TagItem(id='tag_91', value='Tag 91'), - TagItem(id='tag_92', value='Tag 92', parent_id='tag_91'), + TagItem(id='new_parent', value='New parent'), + TagItem(id='new_child', value='New child', parent_id='new_parent'), TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), - TagItem(id='tag_60', value='Tag 60', parent_id='tag_50'), + TagItem(id='renamed_tag_child', value='Renamed tag child', parent_id='tag_50'), ] self.import_plan.generate_actions(tags=tags, replace=False) self.assertEqual(self.import_plan.errors, []) self.assertEqual( [action.tag.id for action in self.import_plan.actions], - ['tag_91', 'tag_92', 'tag_50', 'tag_60'], + ['new_parent', 'new_child', 'tag_50', 'renamed_tag_child'], ) - def test_generate_actions_cyclic_parent_ids_rejected(self) -> None: - """ - Two new rows that name each other as parent terminate with an error. - """ - tags = [ - TagItem(id='tag_90', value='Tag 90', parent_id='tag_91'), - TagItem(id='tag_91', value='Tag 91', parent_id='tag_90'), - ] - self.import_plan.generate_actions(tags=tags, replace=False) - self.assertEqual(len(self.import_plan.errors), 1) - self.assertIn("Unknown parent tag", str(self.import_plan.errors[0])) - def test_generate_actions_deep_reversed_chain_terminates(self) -> None: """ - A 1100-row chain of new tags listed child first is ordered without hitting the recursion - limit and every row is planned. The chain is far too deep to apply, so the plan reports it. + A 1100-row chain of new tags listed child first is ordered without hitting the + recursion limit, and every row is planned. The chain is far too deep to apply, so the + plan reports it. + + File rows: n1099 (parent_id n1098), ..., n1 (parent_id n0), n0 """ tags = [ TagItem(id=f'n{i}', value=f'N{i}', parent_id=f'n{i - 1}' if i else None) @@ -795,8 +810,11 @@ def test_generate_actions_deep_reversed_chain_terminates(self) -> None: def test_plan_validations_do_not_query_per_row(self) -> None: """ - The ordering and the plan-level validations add no queries, however many rows have a - parent_id: the plan costs the same with them replaced by no-ops. + The row ordering and the plan-level validations add no queries, however many rows + have a parent_id: the plan costs the same with them replaced by no-ops. + + File rows: 200 new tags, every second one under tag_1 and the others under the new + tag before them. """ tags = [ TagItem(id=f'n{i}', value=f'N{i}', parent_id='tag_1' if i % 2 == 0 else f'n{i - 1}') @@ -815,159 +833,87 @@ def test_plan_validations_do_not_query_per_row(self) -> None: self.import_plan.generate_actions(tags=tags, replace=False) self.assertEqual(len(real), len(without_validations)) - def test_rows_ordered_as_definers_match_create_and_rename_applies_for(self) -> None: - """ - A row is planned before a child that names its id exactly when it is a rename row or - CreateTag.applies_for holds: existing tags (also with a different case) are not - definers, new tags (also with a different case) and rename rows are, including a - rename onto the id of an existing tag. - """ - rows = [ - TagItem(id='tag_1', value='Tag 1'), - TagItem(id='TAG_2', value='Tag 2'), - TagItem(id='tag_90', value='Tag 90'), - TagItem(id='Tag_91', value='Tag 91'), - TagItem(id='tag_50', value='Tag 3', previous_id='tag_3'), - TagItem(id='tag_4', value='Tag 1', previous_id='tag_1'), - ] - planned_first = set() - for row in rows: - child = TagItem(id='tag_95', value='Tag 95', parent_id=row.id) - self.import_plan.generate_actions(tags=[child, row], replace=False) - ids = [ - action.tag.id for action in self.import_plan.actions if action.name != 'stage_external_id' - ] - if ids.index(row.id) < ids.index('tag_95'): - planned_first.add(row.id) - expected = { - row.id for row in rows - if RenameTagExternalId.applies_for(self.taxonomy, row) or CreateTag.applies_for(self.taxonomy, row) - } - self.assertEqual(expected, {'tag_90', 'Tag_91', 'tag_50', 'tag_4'}) - self.assertEqual(planned_first, expected) - def test_generate_actions_three_level_chain_defined_later_accepted(self) -> None: """ - A definer whose own parent is defined later in the file is still - planned after that parent: a child, its parent and its grandparent - listed in reverse are created grandparent first. + Three new tags listed child first are created parent first. + + File rows After + new_child (parent_id new_parent) new_grandparent + new_parent (parent_id new_grandparent) new_parent + new_grandparent new_child """ tags = [ - TagItem(id='tag_92', value='Tag 92', parent_id='tag_91'), - TagItem(id='tag_91', value='Tag 91', parent_id='tag_90'), - TagItem(id='tag_90', value='Tag 90'), + TagItem(id='new_child', value='New child', parent_id='new_parent'), + TagItem(id='new_parent', value='New parent', parent_id='new_grandparent'), + TagItem(id='new_grandparent', value='New grandparent'), ] self.import_plan.generate_actions(tags=tags, replace=False) self.assertEqual(self.import_plan.errors, []) self.assertEqual( [action.tag.id for action in self.import_plan.actions], - ['tag_90', 'tag_91', 'tag_92'], + ['new_grandparent', 'new_parent', 'new_child'], ) def test_generate_actions_existing_tag_moved_under_later_new_parent_accepted(self) -> None: """ - An existing tag moved under a new parent that is defined later in - the file is accepted, and the parent is created first. + An existing tag moved under a new parent whose row comes later in the file is + accepted, and the parent is created first. + + Before After + tag_1 tag_1 + tag_2 new_parent + tag_2 """ tags = [ - TagItem(id='tag_2', value='Tag 2', parent_id='tag_90'), - TagItem(id='tag_90', value='Tag 90'), + TagItem(id='tag_2', value='Tag 2', parent_id='new_parent'), + TagItem(id='new_parent', value='New parent'), ] self.import_plan.generate_actions(tags=tags, replace=False) self.assertEqual(self.import_plan.errors, []) self.assertEqual(self.import_plan.actions[0].name, 'create') - self.assertEqual(self.import_plan.actions[0].tag.id, 'tag_90') + self.assertEqual(self.import_plan.actions[0].tag.id, 'new_parent') def test_generate_actions_mixed_case_parent_id_defined_later_accepted(self) -> None: """ - A parent_id that differs only in case from the id of a later new row - still finds that row as its definer. + A parent_id that differs only in case from the id of a later new row still makes the + child wait for that row, because ids are case-insensitive. + + File rows After + new_child (parent_id NEW_PARENT) New_Parent + New_Parent new_child """ tags = [ - TagItem(id='tag_91', value='Tag 91', parent_id='TAG_90'), - TagItem(id='Tag_90', value='Tag 90'), + TagItem(id='new_child', value='New child', parent_id='NEW_PARENT'), + TagItem(id='New_Parent', value='New parent'), ] self.import_plan.generate_actions(tags=tags, replace=False) self.assertEqual(self.import_plan.errors, []) - self.assertEqual(self.import_plan.actions[0].tag.id, 'Tag_90') - - def _cycle_messages(self, tags: list[TagItem]) -> list[str]: - """ - Generate the actions for `tags` and return the error messages. - """ - self.import_plan.generate_actions(tags=tags, replace=False) - return [str(error) for error in self.import_plan.errors] + self.assertEqual(self.import_plan.actions[0].tag.id, 'New_Parent') def test_generate_actions_self_parent_of_existing_tag_rejected(self) -> None: """ - An existing tag that names itself as its parent is a cycle of length - one and is reported once. - """ - messages = self._cycle_messages([TagItem(id='tag_2', value='Tag 2', parent_id='tag_2')]) - self.assertEqual(messages, ["Row 1's parent_id (tag_2) creates a parent cycle."]) - - def test_generate_actions_existing_and_new_tag_cycle_rejected_once(self) -> None: - """ - An existing tag and a new tag naming each other as parent are rejected - with exactly one cycle error, in either row order. - """ - rows = [ - TagItem(id='tag_2', value='Tag 2', parent_id='tag_90'), - TagItem(id='tag_90', value='Tag 90', parent_id='tag_2'), - ] - for order in itertools.permutations(range(2)): - with self.subTest(order=order): - messages = self._cycle_messages([rows[i] for i in order]) - self.assertEqual(len(messages), 1) - self.assertIn("Row 1's parent_id", messages[0]) - self.assertIn("creates a parent cycle.", messages[0]) - - def test_generate_actions_cycle_through_db_only_tag_rejected(self) -> None: - """ - tag_2 is a child of tag_1 and is not in the file. A row moving tag_1 - under tag_2 closes a cycle through that DB-only tag. - """ - messages = self._cycle_messages([TagItem(id='tag_1', value='Tag 1', parent_id='tag_2')]) - self.assertEqual(messages, ["Row 1's parent_id (tag_2) creates a parent cycle."]) - - def test_generate_actions_cycle_of_definer_rows_reports_only_unknown_parent(self) -> None: - """ - A cycle made only of new-tag rows, or of a rename row and a new-tag - row, is reported by the parent validation as an unknown parent and not - a second time as a cycle. - """ - cases = { - 'new tags': [ - TagItem(id='tag_90', value='Tag 90', parent_id='tag_91'), - TagItem(id='tag_91', value='Tag 91', parent_id='tag_90'), - ], - 'rename and new tag': [ - TagItem(id='tag_50', value='Tag 1', previous_id='tag_1', parent_id='tag_90'), - TagItem(id='tag_90', value='Tag 90', parent_id='tag_50'), - ], - } - for name, rows in cases.items(): - for order in itertools.permutations(range(2)): - with self.subTest(case=name, order=order): - messages = self._cycle_messages([rows[i] for i in order]) - self.assertEqual(len(messages), 1) - self.assertIn("Unknown parent tag", messages[0]) + An existing tag that names itself as its parent is a cycle of length one and is + reported once. - def test_generate_actions_self_parent_after_rename_onto_freed_id_rejected(self) -> None: + Before File rows + tag_1 tag_2 (parent_id tag_2) + tag_2 """ - tag_3 is renamed onto tag_1, which the same replace import frees, and names - itself as parent. The cycle is reported at the plan step. - """ - tags = [TagItem(id='tag_1', value='Tag 3', previous_id='tag_3', parent_id='tag_1')] - self.import_plan.generate_actions(tags=tags, replace=True) + self.import_plan.generate_actions(tags=[TagItem(id='tag_2', value='Tag 2', parent_id='tag_2')], replace=False) messages = [str(error) for error in self.import_plan.errors] - self.assertEqual(messages, ["Row 1's parent_id (tag_1) creates a parent cycle."]) + self.assertEqual(messages, ["Row 1's parent_id (tag_2) creates a parent cycle."]) def test_generate_actions_child_of_deleted_holder_follows_rename_onto_its_id(self) -> None: """ - tag_2 is a child of tag_1, which the replace import deletes while tag_3 is - renamed onto the id tag_1. Its row plans an update_parent action to tag_1, - which runs after the rename, instead of leaving it promoted to a root. + With replace=True the file omits tag_1, so tag_1 is deleted, and renames tag_3 onto + the id tag_1. The tag_2 row names tag_1, so tag_2 moves under the renamed tag after + the rename runs, instead of being promoted to a root. + + Before After + tag_1 tag_1 (was tag_3) + tag_2 tag_2 + tag_3 + tag_4 """ tags = [ TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), @@ -985,213 +931,69 @@ def test_generate_actions_child_of_deleted_holder_follows_rename_onto_its_id(sel self.import_plan.actions.index(self.import_plan.indexed_actions['rename_external_id'][0]), ) - def test_defines_id_is_shared_by_ordering_and_cycle_check(self) -> None: - """ - Both the ordering and the cycle check use one rule for which rows define their id: with - it patched to never define, a child stays before its late parent row and a cycle of new - tags is reported as a parent cycle instead of an unknown parent. - """ - late_parent = [ - TagItem(id='tag_91', value='Tag 91', parent_id='tag_90'), - TagItem(id='tag_90', value='Tag 90'), - ] - new_cycle = [ - TagItem(id='tag_90', value='Tag 90', parent_id='tag_91'), - TagItem(id='tag_91', value='Tag 91', parent_id='tag_90'), - ] - with mock.patch.object(TagImportPlan, '_defines_id', return_value=False): - self.import_plan.generate_actions(tags=late_parent, replace=False) - self.assertEqual(self.import_plan.actions[0].tag.id, 'tag_91') - self.import_plan.generate_actions(tags=new_cycle, replace=False) - self.assertTrue(any('creates a parent cycle.' in str(e) for e in self.import_plan.errors)) - - def _plan_for_new_taxonomy(self, structure: list[tuple[str, str | None]]) -> TagImportPlan: + def _plan_for_new_taxonomy(self, tags: list[TagItem]) -> TagImportPlan: """ - Create the given (external_id, parent external_id) tags, in parent-first order, in an - empty taxonomy shared by the class, and return a plan for it. Each tag's value is - "Tag ". A test calls this once, and its tags are rolled back with the test. + Create ``tags`` (parents listed before their children) in an empty taxonomy shared by + the class, and return a plan for that taxonomy. A test calls this once, and the tags + are rolled back with the test. """ - tags: dict[str, Tag] = {} - for external_id, parent_id in structure: - tags[external_id] = Tag.objects.create( + created: dict[str, Tag] = {} + for tag in tags: + created[tag.id] = Tag.objects.create( taxonomy=self.structure_taxonomy, - value=f'Tag {external_id}', - external_id=external_id, - parent=tags[parent_id] if parent_id else None, + value=tag.value, + external_id=tag.id, + parent=created[tag.parent_id] if tag.parent_id else None, ) return TagImportPlan(self.structure_taxonomy) - def _row(self, tag_id: str, parent_id: str | None = None) -> TagItem: - """ - Build the import row for the tag created by `_plan_for_new_taxonomy`. - """ - return TagItem(id=tag_id, value=f'Tag {tag_id}', parent_id=parent_id) - - def _assert_promotions_precede_deletes(self, plan: TagImportPlan) -> None: - """ - Check every delete action comes after every promotion of a kept child to a root. - """ - names = [action.name for action in plan.actions] - deletes = [index for index, name in enumerate(names) if name == 'delete'] - self.assertTrue(deletes) - self.assertEqual(deletes, list(range(deletes[0], deletes[-1] + 1))) - self.assertNotIn('create', names[:deletes[0]]) - self.assertEqual(set(names[:deletes[0]]), {'update_parent'}) - - def test_generate_actions_kept_grandchild_of_deleted_chain_promoted_before_deletes(self) -> None: - """ - t5 is kept under t1, which is deleted along with its parent t0. Deleting t0 - would cascade to t1 and t5, so t5 is promoted to a root before any delete. - """ - plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0'), ('t5', 't1')]) - plan.generate_actions(tags=[self._row('t5')], replace=True) - self.assertEqual(plan.errors, []) - self._assert_promotions_precede_deletes(plan) - - def test_generate_actions_kept_tags_at_two_depths_under_deleted_ancestors(self) -> None: - """ - In a five-level chain with t0, t1 and t3 deleted, the kept t2 and t4 are both - promoted before any delete. - """ - structure = [('t0', None), ('t1', 't0'), ('t2', 't1'), ('t3', 't2'), ('t4', 't3')] - plan = self._plan_for_new_taxonomy(structure) - plan.generate_actions(tags=[self._row('t2'), self._row('t4')], replace=True) - self.assertEqual(plan.errors, []) - self._assert_promotions_precede_deletes(plan) - promoted = { - action.tag.id for action in plan.actions[:2] if action.name == 'update_parent' - } - self.assertEqual(promoted, {'t2', 't4'}) - - def test_generate_actions_kept_child_of_deleted_root_promoted(self) -> None: - """ - A kept child of a deleted root is promoted to a root before the delete. - """ - plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0')]) - plan.generate_actions(tags=[self._row('t1')], replace=True) - self.assertEqual(plan.errors, []) - self.assertEqual([action.name for action in plan.actions[:2]], ['update_parent', 'delete']) - - def _update_parent_index(self, plan: TagImportPlan, tag_id: str, parent_id: str | None) -> int: - """ - Return the index of the update_parent action that sets `tag_id` under `parent_id`. + def test_generate_actions_valid_move_order_keeps_action_order(self) -> None: """ - return next( - index for index, action in enumerate(plan.actions) - if action.name == 'update_parent' and action.tag.id == tag_id and action.tag.parent_id == parent_id - ) + A file whose parent moves are already in a valid order keeps its row order in the + plan. - def test_generate_actions_ancestry_inversion_accepted_in_any_row_order(self) -> None: - """ - t2 is a child of t0 and becomes t0's ancestor (g0 is new, under t2). In all 24 row - orders the plan is clean and t2 moves out of t0's subtree before t0 moves under g0. + Before After + old_root new_root + old_child new_middle + new_root old_root + old_child """ + plan = self._plan_for_new_taxonomy([ + TagItem(id='old_root', value='Old root'), + TagItem(id='old_child', value='Old child', parent_id='old_root'), + TagItem(id='new_root', value='New root', parent_id='old_root'), + ]) rows = [ - self._row('t0', 'g0'), self._row('t1', 'g0'), self._row('t2'), self._row('g0', 't2'), + TagItem(id='new_root', value='New root'), + TagItem(id='new_middle', value='New middle', parent_id='new_root'), + TagItem(id='old_root', value='Old root', parent_id='new_middle'), + TagItem(id='old_child', value='Old child', parent_id='new_middle'), ] - plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0'), ('t2', 't0')]) - for order in itertools.permutations(range(4)): - with self.subTest(order=order): - plan.generate_actions(tags=[rows[i] for i in order], replace=False) - self.assertEqual(plan.errors, []) - self.assertLess( - self._update_parent_index(plan, 't2', None), self._update_parent_index(plan, 't0', 'g0'), - ) - - def test_generate_actions_parent_and_child_swap_places_in_any_row_order(self) -> None: - """ - A parent and its child swap places: the child moves to the root before the parent - moves under it, in either row order. - """ - rows = [self._row('t0', 't1'), self._row('t1')] - plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0')]) - for order in itertools.permutations(range(2)): - with self.subTest(order=order): - plan.generate_actions(tags=[rows[i] for i in order], replace=False) - self.assertEqual(plan.errors, []) - self.assertLess( - self._update_parent_index(plan, 't1', None), self._update_parent_index(plan, 't0', 't1'), - ) - - def test_generate_actions_three_level_inversion_accepted_in_any_row_order(self) -> None: - """ - A three-level chain t0 > t1 > t2 is reversed: each move runs only after the tag it - moves under has left its subtree, in all six row orders. - """ - rows = [self._row('t2'), self._row('t1', 't2'), self._row('t0', 't1')] - plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0'), ('t2', 't1')]) - for order in itertools.permutations(range(3)): - with self.subTest(order=order): - plan.generate_actions(tags=[rows[i] for i in order], replace=False) - self.assertEqual(plan.errors, []) - moves = [ - self._update_parent_index(plan, 't2', None), - self._update_parent_index(plan, 't1', 't2'), - self._update_parent_index(plan, 't0', 't1'), - ] - self.assertEqual(moves, sorted(moves)) - - def test_generate_actions_valid_move_order_keeps_action_order(self) -> None: - """ - A file whose parent moves are already in a valid order keeps its row order in the plan. - """ - plan = self._plan_for_new_taxonomy([('t0', None), ('t1', 't0'), ('t2', 't0')]) - rows = [self._row('t2'), self._row('g0', 't2'), self._row('t0', 'g0'), self._row('t1', 'g0')] plan.generate_actions(tags=rows, replace=False) self.assertEqual(plan.errors, []) - self.assertEqual([action.tag.id for action in plan.actions], ['t2', 'g0', 't0', 't1']) - - def _deep_chain_structure(self) -> list[tuple[str, str | None]]: - """ - Return a chain t0 > t1 > t2 > t3 > t5 of five levels, the deepest the taxonomy allows - before a child of t5 would be a sixth level (parent depth 4, so the tag below it is 5). - """ - return [('t0', None), ('t1', 't0'), ('t2', 't1'), ('t3', 't2'), ('t5', 't3')] - - def _passes_dry_run(self, plan: TagImportPlan) -> bool: - """ - Return whether the plan's actions pass the parent dry run, on the taxonomy as it is now. - """ - forest = _ParentForest({ - tag.external_id.casefold(): ExistingTag( - tag.pk, tag.parent_id, tag.parent.external_id if tag.parent else None, - ) - for tag in plan.taxonomy.tag_set.select_related('parent') - }) - return all(forest.apply(action) for action in plan.actions) - - def _assert_detached_first(self, plan: TagImportPlan, tag_id: str) -> None: - """ - Check the plan starts with the update-to-root of `tag_id`, before any other action - of its rows, and that the plan passes the dry run. - """ - first = plan.actions[0] - self.assertEqual((first.name, first.tag.id, first.tag.parent_id), ('update_parent', tag_id, None)) - self.assertTrue(self._passes_dry_run(plan)) - - def test_generate_actions_deep_parent_move_detaches_before_attaching(self) -> None: - """ - t5 is moved to the root while new tags g0 and g1 are put under it. Creating them - under t5 before it moves would make g1 a seventh level. All six row orders plan clean, - and the hazardous one (new tags first) detaches t5 before any create. - """ - rows = [self._row('g1', 'g0'), self._row('g0', 't5'), self._row('t5')] - plan = self._plan_for_new_taxonomy(self._deep_chain_structure()) - for order in itertools.permutations(range(3)): - with self.subTest(order=order): - plan.generate_actions(tags=[rows[i] for i in order], replace=False) - self.assertEqual(plan.errors, []) - self.assertTrue(self._passes_dry_run(plan)) - plan.generate_actions(tags=rows, replace=False) - self._assert_detached_first(plan, 't5') + self.assertEqual( + [action.tag.id for action in plan.actions], ['new_root', 'new_middle', 'old_root', 'old_child'], + ) def test_generate_actions_ordinary_import_has_no_detach_actions(self) -> None: """ - An import whose steps are all valid keeps its plan: no update-to-root is added. + An import whose steps are all valid keeps its plan: no move to the root is added + before the other actions. + + Before After + level_1 level_1 + level_2 level_2 + level_3 level_3 + new_leaf """ - plan = self._plan_for_new_taxonomy(self._deep_chain_structure()[:3]) + chain = [ + TagItem(id='level_1', value='Level 1'), + TagItem(id='level_2', value='Level 2', parent_id='level_1'), + TagItem(id='level_3', value='Level 3', parent_id='level_2'), + ] + plan = self._plan_for_new_taxonomy(chain) plan.generate_actions( - tags=[self._row('t0'), self._row('t1', 't0'), self._row('t2', 't1'), self._row('n0', 't2')], + tags=chain + [TagItem(id='new_leaf', value='New leaf', parent_id='level_3')], replace=False, ) self.assertEqual(plan.errors, []) @@ -1199,119 +1001,57 @@ def test_generate_actions_ordinary_import_has_no_detach_actions(self) -> None: [action.name for action in plan.actions], ['without_changes'] * 3 + ['create'], ) - def test_generate_actions_deep_tag_under_renamed_parent_moved_to_root(self) -> None: - """ - t5's parent t3 is renamed to r3 while t5 moves to the root and new tags go under it: - all 24 row orders plan clean, and the hazardous one detaches t5 first. - """ - rows = [ - self._row('g1', 'g0'), self._row('g0', 't5'), self._row('t5'), - TagItem(id='r3', value='Tag t3', previous_id='t3', parent_id='t2'), - ] - plan = self._plan_for_new_taxonomy(self._deep_chain_structure()) - for order in itertools.permutations(range(4)): - with self.subTest(order=order): - plan.generate_actions(tags=[rows[i] for i in order], replace=False) - self.assertEqual(plan.errors, []) - self.assertTrue(self._passes_dry_run(plan)) - plan.generate_actions(tags=rows, replace=False) - self._assert_detached_first(plan, 't5') - - def test_generate_actions_subtree_moved_under_deep_tag_before_it_is_shallowed(self) -> None: - """ - A three-level subtree s0 > s1 > s2 is moved under c3, which is itself moved to the root. - Attaching s0 under c3 while c3 is still at depth 3 would make s2 a seventh level. - """ - structure = [('c0', None), ('c1', 'c0'), ('c2', 'c1'), ('c3', 'c2'), ('s0', None), ('s1', 's0'), ('s2', 's1')] - rows = [self._row('s0', 'c3'), self._row('c3'), self._row('s1', 's0'), self._row('s2', 's1')] - plan = self._plan_for_new_taxonomy(structure) - for order in itertools.permutations(range(len(rows))): - with self.subTest(order=order): - plan.generate_actions(tags=[rows[i] for i in order], replace=False) - self.assertEqual(plan.errors, []) - self.assertTrue(self._passes_dry_run(plan)) - - def test_generate_actions_three_level_new_chain_under_moved_deep_tag(self) -> None: - """ - Three new tags are chained under t5, which moves to the root: all 24 row orders plan clean. - """ - rows = [self._row('t5'), self._row('g0', 't5'), self._row('g1', 'g0'), self._row('g2', 'g1')] - plan = self._plan_for_new_taxonomy(self._deep_chain_structure()) - for order in itertools.permutations(range(4)): - with self.subTest(order=order): - plan.generate_actions(tags=[rows[i] for i in order], replace=False) - self.assertEqual(plan.errors, []) - self.assertTrue(self._passes_dry_run(plan)) - def test_parent_dry_run_accepts_valid_and_rejects_invalid_sequences(self) -> None: """ - The dry run accepts a valid move, and rejects a cycle, a tree that gets too deep, and a - reference to a tag removed by an earlier delete (a delete also removes descendants). + ``_ParentForest`` accepts a valid move, and rejects a cycle, a tree that gets too + deep, and a reference to a tag that an earlier delete removed (a delete also removes + the descendants). + + Before + level_1 + level_2 + level_3 + level_4 + level_5 + other_root + other_child """ - # A chain c0 > ... > c4 of five levels, and a root s0 with a child s1. tags = { - 'c0': ExistingTag(1, None, None), 'c1': ExistingTag(2, 1, 'c0'), 'c2': ExistingTag(3, 2, 'c1'), - 'c3': ExistingTag(4, 3, 'c2'), 'c4': ExistingTag(5, 4, 'c3'), - 's0': ExistingTag(6, None, None), 's1': ExistingTag(7, 6, 's0'), + 'level_1': ExistingTag(pk=1, parent_pk=None, parent_external_id=None), + 'level_2': ExistingTag(pk=2, parent_pk=1, parent_external_id='level_1'), + 'level_3': ExistingTag(pk=3, parent_pk=2, parent_external_id='level_2'), + 'level_4': ExistingTag(pk=4, parent_pk=3, parent_external_id='level_3'), + 'level_5': ExistingTag(pk=5, parent_pk=4, parent_external_id='level_4'), + 'other_root': ExistingTag(pk=6, parent_pk=None, parent_external_id=None), + 'other_child': ExistingTag(pk=7, parent_pk=6, parent_external_id='other_root'), } taxonomy = Taxonomy() - def update(tag_id: str, parent_id: str | None) -> UpdateParentTag: + def move(tag_id: str, parent_id: str | None) -> UpdateParentTag: return UpdateParentTag(taxonomy, TagItem(id=tag_id, value='x', parent_id=parent_id), 1) - delete = DeleteTag(taxonomy, TagItem(id='c1', value='x'), 1) - self.assertTrue(_ParentForest(tags).apply(update('s1', 'c0'))) - self.assertFalse(_ParentForest(tags).apply(update('c0', 'c4'))) - self.assertFalse(_ParentForest(tags).apply(update('s0', 'c4'))) + delete_level_2 = DeleteTag(taxonomy, TagItem(id='level_2', value='x'), 1) + self.assertTrue(_ParentForest(tags).apply(move('other_child', 'level_1'))) + # level_1 under its own descendant is a cycle. + self.assertFalse(_ParentForest(tags).apply(move('level_1', 'level_5'))) + # other_child would end up at a seventh level. + self.assertFalse(_ParentForest(tags).apply(move('other_root', 'level_5'))) forest = _ParentForest(tags) - self.assertTrue(forest.apply(delete)) - self.assertFalse(forest.apply(update('s0', 'c2'))) + self.assertTrue(forest.apply(delete_level_2)) + self.assertFalse(forest.apply(move('other_root', 'level_3'))) forest = _ParentForest(tags) - self.assertTrue(forest.apply(delete)) - self.assertTrue(forest.apply(update('s0', 'c0'))) - - def test_generate_actions_rebuilds_a_hazardous_plan_only_once(self) -> None: - """ - When the dry run keeps failing, the plan is rebuilt detach-first once, checked once more, - and reported as invalid instead of being rebuilt again. - """ - plan = self._plan_for_new_taxonomy(self._deep_chain_structure()) - rows = [self._row('g0', 't5'), self._row('t5')] - with mock.patch.object(TagImportPlan, '_parent_changes_are_valid', return_value=False) as dry_run: - plan.generate_actions(tags=rows, replace=False) - self.assertEqual(dry_run.call_count, 2) - self.assertEqual(len(plan.errors), 1) - first = plan.actions[0] - self.assertEqual((first.name, first.tag.id, first.tag.parent_id), ('update_parent', 't5', None)) - - def test_generate_actions_rejects_a_new_tag_below_the_maximum_depth(self) -> None: - """ - A tag added under a tag that is already at the deepest level cannot be applied in any - order, so the plan has the invalid hierarchy error instead of an applicable plan. - """ - structure = [('a', None), ('b', 'a'), ('c', 'b'), ('d', 'c'), ('e', 'd'), ('f', 'e')] - plan = self._plan_for_new_taxonomy(structure) - rows = [self._row(tag_id, parent) for tag_id, parent in structure] + [self._row('g', 'f')] - plan.generate_actions(tags=rows, replace=True) - self.assertEqual(len(plan.errors), 1) - self.assertIn('deeper than 6 levels', str(plan.errors[0])) - - def test_generate_actions_rejects_a_subtree_moved_too_deep(self) -> None: - """ - Moving the chain w > x > y > z under d (a > b > c > d) would put z at the seventh level, - so the plan has the invalid hierarchy error. - """ - structure = [('a', None), ('b', 'a'), ('c', 'b'), ('d', 'c'), ('w', None), ('x', 'w'), ('y', 'x'), ('z', 'y')] - plan = self._plan_for_new_taxonomy(structure) - rows = [self._row(tag_id, 'd' if tag_id == 'w' else parent) for tag_id, parent in structure] - plan.generate_actions(tags=rows, replace=True) - self.assertEqual(len(plan.errors), 1) - self.assertIn('deeper than 6 levels', str(plan.errors[0])) + self.assertTrue(forest.apply(delete_level_2)) + self.assertTrue(forest.apply(move('other_root', 'level_1'))) def test_generate_actions_swap_case_variant_of_stale_parent_id_rejected(self) -> None: """ - In a tag_1 <-> tag_3 swap, a child naming the stale id "tag_1" with another case is - rejected like the exact spelling, with its row position and the new id. + In a swap, a child naming its parent's stale id in another case is rejected like the + exact spelling, with its row number and the parent's new id. + + Before File rows + tag_1 tag_3 (previous_id tag_1) + tag_2 tag_1 (previous_id tag_3) + tag_3 tag_2 (parent_id TAG_1) """ tags = [ TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), @@ -1326,7 +1066,14 @@ def test_generate_actions_swap_case_variant_of_stale_parent_id_rejected(self) -> def test_generate_actions_swap_case_variant_of_end_state_parent_id_accepted(self) -> None: """ - In the same swap, children naming the end-state ids with another case are accepted. + In a swap, children naming the end-state ids of their parents in another case are + accepted. + + Before After + tag_1 tag_3 (was tag_1) + tag_2 tag_2 (parent_id TAG_3) + tag_3 tag_1 (was tag_3) + tag_4 tag_4 (parent_id TAG_1) """ tags = [ TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), @@ -1339,8 +1086,12 @@ def test_generate_actions_swap_case_variant_of_end_state_parent_id_accepted(self def test_generate_actions_plain_rename_case_variant_of_parent_id_reported_once(self) -> None: """ - When tag_1 is plainly renamed to tag_50, a child naming "TAG_1" gets only the parent - validation error, not a second one for the same row. + When the parent is renamed without a swap, a child naming the parent's old id in + another case gets only the parent validation error, not a second one for the same row. + + Before File rows + tag_1 tag_50 (previous_id tag_1) + tag_2 tag_2 (parent_id TAG_1) """ tags = [ TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), @@ -1351,19 +1102,6 @@ def test_generate_actions_plain_rename_case_variant_of_parent_id_reported_once(s self.assertEqual(len(messages), 1) self.assertIn('Parent tag TAG_1 is renamed to tag_50 in this file', messages[0]) - def test_generate_actions_case_only_parent_rename_with_case_variant_child_accepted(self) -> None: - """ - Renaming tag_1 only by case keeps the id, so a child naming it in either case is accepted. - """ - for parent_id in ('tag_1', 'Tag_1', 'TAG_1'): - with self.subTest(parent_id=parent_id): - tags = [ - TagItem(id='TAG_1', value='Tag 1', previous_id='tag_1'), - TagItem(id='tag_2', value='Tag 2', parent_id=parent_id), - ] - self.import_plan.generate_actions(tags=tags, replace=False) - self.assertEqual(self.import_plan.errors, []) - def test_generate_actions_genuine_collision_not_staged(self) -> None: """ A rename targeting an id held by an unrelated tag that is not From 72f9f020337da6d00ce870d6b95a91257ed94a45 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 14 Oct 2026 17:25:58 +0300 Subject: [PATCH 5/5] test: restore the original test_get_taxonomy The change to test_get_taxonomy was only needed while the new import tests created a taxonomy per test, which moved the MySQL auto-increment counter onto id 200. The new tests now create their taxonomy once per class, so the original test passes in CI order again. Co-Authored-By: Claude Opus 5.5 --- tests/openedx_tagging/test_api.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index ee403b36c..a2a8a8e2a 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -67,8 +67,7 @@ def test_create_taxonomy_without_export_id(self) -> None: def test_get_taxonomy(self) -> None: tax1 = tagging_api.get_taxonomy(1) assert tax1 == self.taxonomy - unused_id = (Taxonomy.objects.order_by("-pk").values_list("pk", flat=True).first() or 0) + 1 - no_tax = tagging_api.get_taxonomy(unused_id) + no_tax = tagging_api.get_taxonomy(200) assert no_tax is None def test_get_taxonomy_by_export_id(self) -> None: