[Draft] Refactor: Consolidate taxonomy, tag and object-tag writes into one module

This is a suggested approach, not the source of truth. The User Story, Description, and
Acceptance Criteria define what must be true when the work is done.

Summary

This is a strangler-pattern refactoring in three stages. Stages 1a and 1b move code and add
a guard test without changing behavior. Stage 2 adds the archived gates, which does change
behavior. The public Python API, the model classes, the REST URL contract and the exception
types callers already catch (ValueError, Tag.DoesNotExist) stay unchanged.

Approach

tagging_mutations.py is an internal module. Every function that writes a Taxonomy,
Tag or ObjectTag row lives there, and nothing else in the app writes those rows.

Rules that must hold on any save, regardless of caller, stay on the models as persistence
invariants: Tag.save (depth, lineage, lineage cascade to descendants, external_id
generation), Tag.clean, Taxonomy.clean, ObjectTag.clean, the ObjectTag.__init__
defaults, and the foreign-key on_delete behavior. Rules about whether an operation is
allowed at all live in the module. Read paths are out of scope: the unarchived()
queryset methods are already the single read-side mechanism.

In stage 2, each taxonomy or tag-tree write opens with a gate call. A gate is a two-line
function that raises ArchivedError. There are two, one for a taxonomy and one for a tag.

Why a new module instead of api.py. OEP-49 puts business logic in api.py, but
api.py is also the public contract that edx-platform and plugins import, and every name
in it is stable under the README's rule. The write module needs functions that are
internal today (move_tag and delete_tag for the import plan, update_taxonomy,
delete_taxonomy) and will need archive and unarchive operations for #779 and #780. Putting any of
these in api.py would make them public on day one. The module is private, and api.py decides what to
expose.

Why the model methods stay. Taxonomy.add_tag, update_tag and delete_tags have
callers in tests and possibly in plugins. Keeping them as delegates keeps stage 1 free of
behavior change, and they can go through the DEPR process separately. One thing does
change: api.add_tag_to_taxonomy and the import's CreateTag call the module directly, so
a Taxonomy subclass that overrode add_tag would no longer intercept those calls. No
subclass in openedx-core or edx-platform does. The ADR records this.

Why a guard test enforces the module boundary. Import Linter cannot tell a read from a
write, and the views legitimately import the model classes for querysets. A short test that
walks the AST of each source file for write calls outside an allow list is simple, fast, and
names the offending file and line. It is the only thing that keeps the module honest after
its authors move on. The docstring in api.py already tried the convention-only route.

Why ArchivedError subclasses ValueError. Every tag view already maps ValueError
to HTTP 400 and TagDoesNotExist to 404, and edx-platform catches ValueError from
tag_object. A subclass gives callers a precise type to catch without breaking any
existing handler. It also settles finding 6 of the #778 review: 400 everywhere for a write
on an archived record.

Why the free-text and read-only checks are not a gate. They already exist on every
path that needs them, with four different message strings that tests here and in
edx-platform assert on. A single gate would fix no bug and would change those messages.
Each check stays inline in its own function body and moves with it into the module.

Risks and controls

Files to create and modify

Additional context

• Finding 7 of the #778 review: the full-list replace in tag_object deletes hidden
  archived associations.
• ADR 0006 (docs/openedx_tagging/decisions/0006-tag-changes.rst) defines the copy-and-resync
  semantics of ObjectTag._value and _export_id that the moved code must preserve.
• ADR 0010 makes the import path the only place that changes Tag.external_id, so the import
  actions are a legitimate write entry point that the refactoring routes through the module,
  not a side path to remove.

The spec was written when the work was planned as three child tickets and is kept as
written. Read "child ticket" and "stage" below as implementation order within this one
issue.

This spec is the contract for the three child tickets. Line numbers refer to the
pr-tbain-1-778_archived_read_paths branch of openedx-core and will drift; function
names are the stable reference.

1. Principles

1. One module writes. Every INSERT, UPDATE or DELETE on oel_tagging_taxonomy,
   oel_tagging_tag and oel_tagging_objecttag originates in tagging_mutations.py. The
   two exceptions are Tag.save doing its own lineage cascade (persistence invariant) and
   the Django admin (raw bypass).
2. The module is internal. api.py is the public facade and decides which functions are
   public. Nothing outside openedx_tagging imports tagging_mutations.
3. Models keep persistence invariants only: things that must be true of a row no matter
   who wrote it. Whether an operation is allowed is the module's job.
4. A gate is a tiny function that raises. Every taxonomy and tag-tree write calls its gate
   first. No write function reimplements a gate's check inline.
5. Stage 1 is a move. If a test has to change in stage 1, the move is wrong.
6. Primitives take instances; public-shaped operations take values. rename_tag(taxonomy, tag, new_value) takes a Tag; update_tag(taxonomy, tag, new_value) takes a value,
   looks the tag up, runs the value-based checks, then calls rename_tag. The import plan
   already holds instances, so it calls primitives and inherits no lookup or check it does
   not run today.

2. Module layout

src/openedx_tagging/tagging_mutations.py
    """
    Module docstring: what lives here, the "one module writes" rule, the gates,
    and the admin/ORM bypass.
    """
    # Exceptions (stage 2)
    class ArchivedError(ValueError)

    # Gates (stage 2). No leading underscore: import_export/api.py calls one of them.
    def require_unarchived_taxonomy(taxonomy: Taxonomy) -> None
    def require_unarchived_tag(tag: Tag) -> None

    # Taxonomy (stage 1a)
    def create_taxonomy(name, description=None, enabled=True, allow_multiple=True,
                        allow_free_text=False, taxonomy_cls=None, export_id=None) -> Taxonomy
    def update_taxonomy(taxonomy: Taxonomy, **fields) -> Taxonomy
    def delete_taxonomy(taxonomy: Taxonomy) -> None

    # Tags, value-based (stage 1a): today's model method bodies, same parameter names
    def add_tag(taxonomy, tag_value, parent_tag_value=None, external_id=None) -> Tag
    def update_tag(taxonomy, tag, new_value) -> Tag
    def delete_tags(taxonomy, tags, with_subtags=False) -> None

    # Tags, instance-based primitives (stage 1a): today's import action bodies
    def rename_tag(taxonomy, tag: Tag, new_value) -> Tag
    def move_tag(taxonomy, tag: Tag, new_parent: Tag | None) -> Tag
    def delete_tag(tag: Tag) -> None

    # Object tags (resync in 1a, the rest in 1b)
    def resync_object_tags(object_tags=None) -> int
    def tag_object(object_id, taxonomy, tags, create_invalid=False, taxonomy_export_id=None) -> None
    def delete_object_tags(object_id) -> None
    def copy_tags(source_object_id, dest_object_id) -> None
    def unmark_copied_tags(object_id) -> None

add_tag, update_tag and delete_tags have the same names and parameters as the
Taxonomy methods they replace, plus the taxonomy as the first argument.

3. Where each piece of today's code goes

4. The gates (stage 2)

class ArchivedError(ValueError):
    """Raised when a write targets a taxonomy or tag that is archived."""


def require_unarchived_taxonomy(taxonomy: Taxonomy) -> None:
    """Raise ArchivedError if the taxonomy is archived. Deleting it is still allowed."""
    if taxonomy.archived:
        raise ArchivedError(_("This taxonomy is archived and cannot be edited."))


def require_unarchived_tag(tag: Tag) -> None:
    """Raise ArchivedError if the tag is archived."""
    if tag.archived:
        raise ArchivedError(_("Tag '{value}' is archived.").format(value=tag.value))

require_unarchived_tag does not look at tag.taxonomy.archived. At every call site the
tag was fetched through taxonomy.tag_set after require_unarchived_taxonomy(taxonomy)
already ran on that same taxonomy, so the check would be redundant.

Which write calls which gate, as the first statement after any lookup it needs:

The stage 1a archived check moved from Taxonomy.update_tag (base.py:667) is replaced in
stage 2 by these gate calls; it is the only inline archived check the module carries after
stage 2.

Plan versus execution. Gates in rename_tag and move_tag raise inside
TagImportPlan.execute, which import_tags catches and reports as a failed import
(import_export/api.py:154-157), so PUT tags/import/plan can return a clean plan while
PUT tags/import returns 400 for the same CSV. The 400 is the decided behavior. Plan-time
reporting of archived tags is #814's scope.

5. What api.py looks like after stage 1b

Reads are unchanged. Each write function keeps its public docstring and signature and
holds no model write call:

def add_tag_to_taxonomy(taxonomy, tag, parent_tag_value=None, external_id=None) -> Tag:
    """<existing docstring>"""
    new_tag = tagging_mutations.add_tag(taxonomy, tag, parent_tag_value, external_id)
    resync_object_tags(taxonomy.objecttag_set.all())
    return new_tag

Plus, in stage 2:

# Export these as part of the API
TagDoesNotExist = Tag.DoesNotExist
ArchivedError = tagging_mutations.ArchivedError

6. What the model methods look like after stage 1a

def add_tag(self, tag_value, parent_tag_value=None, external_id=None) -> Tag:
    """
    Add a tag to this taxonomy. The logic lives in openedx_tagging.tagging_mutations;
    this method is kept for existing callers.
    """
    from openedx_tagging import tagging_mutations  # local import avoids a module cycle
    return tagging_mutations.add_tag(self, tag_value, parent_tag_value, external_id)

Same for update_tag and delete_tags.

7. The guard test (stage 1b)

WRITE_ATTRS = {"save", "delete", "create", "update_or_create", "bulk_create"}
ALLOWED = {
    "tagging_mutations.py",
    "models/base.py",              # Tag.save lineage cascade; model-level super().save()
    "models/import_export.py",     # TagImportTask bookkeeping
    "import_export/api.py",        # TagImportTask.create and log writes
    "import_export/import_plan.py",  # task.save() for import-task status
}


def test_only_tagging_mutations_writes_models():
    offenders = []
    for path in SRC_ROOT.rglob("*.py"):
        rel = str(path.relative_to(SRC_ROOT))
        if "migrations" in path.parts or rel in ALLOWED:
            continue
        tree = ast.parse(path.read_text(), filename=rel)
        for node in ast.walk(tree):
            if (
                isinstance(node, ast.Call)
                and isinstance(node.func, ast.Attribute)
                and node.func.attr in WRITE_ATTRS
            ):
                offenders.append(f"{rel}:{node.lineno}: .{node.func.attr}(")
    assert not offenders, "Model writes outside tagging_mutations.py:\n" + "\n".join(offenders)

Parsing the AST instead of matching text means docstrings and comments cannot trigger it
(signal_handlers.py:30-43 mention .delete() in prose). A dry run of this test against the
branch before any change flags exactly the eleven call sites stage 1 moves (seven in api.py,
three in import_export/actions.py, one in rest_api/v1/views.py) and nothing else, so the
allow list is complete and the test passes once stage 1b lands. Known limits, accepted on
purpose: the allow list is per file, so a new write added to models/base.py is not
caught; queryset .update() is not scanned because of dict .update() false positives;
and writes that Django REST Framework performs inside its own mixins (UpdateModelMixin,
DestroyModelMixin) are invisible to it, which is why TaxonomyView must define
perform_update and perform_destroy explicitly. The test is a tripwire, not a proof.

8. Test matrix for stage 2

Module-level tests prove the gates; the guard test proves every write reaches the module;
REST tests pin the status codes. The import column therefore needs one test.

9. Explicitly out of scope

• Any read path. unarchived() stays as is.
• tag_object and object-tag archived semantics (#779). Related spots inside the module
  that #779 will touch, all now in one file: _get_current_tags returns archived rows, so a
  re-applied archived association stays archived; _check_new_tag_count counts archived
  rows toward the 100-tag cap; copy_tags uses update_or_create, which can match an
  archived destination row.
• Deleting archived records and the deletion_locked field (#780). The object-tag deletes
  it must consider are delete_object_tags, the replace step of tag_object, and the
  is_copied delete in copy_tags, plus the SET_NULL on Tag and Taxonomy deletes.
• Events for archived associations (#781). When archive and unarchive become module
  functions, update_taxonomy should reject an archived field so there is one way to
  archive and it emits the event.
• Import resolution to archived tags (#814). The lookups it needs to change are read-side
  (actions.py and import_plan.py tag_set lookups), outside this module. A
  pre-existing data-loss path belongs to it: export leaves archived tags out
  (test_parsers.py:221-230), the REST import always passes replace=True (views.py:362),
  and the replace set is tag_set.all() (import_plan.py:155), so an export, edit, re-import
  round trip hard-deletes every archived tag and nulls their object tags. This exists today
  and the refactoring does not change it.
• The Django admin. It is documented as the raw bypass.
• Deprecating the Taxonomy model methods (separate DEPR).
• Pre-existing debt the move exposes but does not cause: copy_tags skipping full_clean,
  the missing cycle check on reparent, three different duplicate-value semantics,
  TaxonomyView.update skipping full_clean. Each gets its own issue if wanted. The module
  makes each of them a one-place fix later.

10. Verification per stage

• Stage 1a and 1b: pytest tests/openedx_tagging tests/openedx_learning --no-cov with no
  test changed; lint-imports; pylint, pycodestyle, isort, mypy on touched files;
  makemigrations --check (no model changes expected).
• Stage 2: the test matrix above, plus the full suite.
• All stages: run edx-platform's openedx/core/djangoapps/content_tagging tests against
  the branch if a devstack is available, since edx-platform subclasses the views this
  touches. Report it as a gap if not run.