From ec32430b51e37bc22540fdc05725483ad7200b21 Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Fri, 30 Oct 2026 01:33:52 +0400 Subject: [PATCH] feat: enforce one root criteria group per competency in the database Add CompetencyCriteriaGroup.scope_code, set in save() to "tag:" for a root and null for every other group, with a unique constraint on it. Until now one root per competency relied on the Tag row lock that associate_competency_criterion() takes: MySQL has no partial unique indexes, and the (tag, course, parent) constraint treats a root's null course and parent as distinct, so two roots for one competency would both save. The lock stays, but it now only serializes the duplicate-criterion check. A group's tag, parent and course are immutable after creation; save() rejects a change to any of them with a ValidationError, so a root's code never has to change. An archived root keeps its code and is reactivated rather than replaced: each learner has one StudentCompetencyStatus row per competency, derived from one root, so a live root beside archived ones would force every rollup and read to pick one. A check constraint allows a code only on a root. It runs one way, "a coded row is a root", because MySQL cannot defer constraint checks and Django's delete collector nulls parent and course before cascading. Below the root the column stays null: a leaf has no natural key to put there, and the one-way check means non-null would not make the rule stronger. Migration 0017 raises DuplicateRootCriteriaGroupsError on existing duplicate roots before any schema change, since MySQL commits each schema change immediately, then adds the column, backfills roots, and adds both constraints. ADR-0002 is amended: Decision 2 gains scope_code and the immutable columns, Decision 3 contrasts the two scope_code columns, Decision 7 states the MVP archive rule, and Rejected Alternative 7 records the replace-the-root design. Related to openedx/openedx-core#872 Co-Authored-By: Claude Opus 5.5 Co-Authored-By: Claude Fable 5.1 --- .../0002-competency-criteria-model.rst | 41 +++-- src/openedx_learning/applets/cbe/api.py | 15 +- .../applets/cbe/models/criteria.py | 60 +++++++- .../0017_criteria_group_scope_code.py | 88 +++++++++++ .../openedx_learning/applets/cbe/test_api.py | 9 +- .../applets/cbe/test_criteria_group.py | 144 +++++++++++++++++- .../cbe/test_criteria_group_deletion.py | 32 +++- .../applets/cbe/test_views.py | 20 +-- tests/openedx_learning/test_migrations.py | 123 +++++++++++++++ 9 files changed, 489 insertions(+), 43 deletions(-) create mode 100644 src/openedx_learning/migrations/0017_criteria_group_scope_code.py create mode 100644 tests/openedx_learning/test_migrations.py diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index 334eedd53..df4aa42c9 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -81,13 +81,14 @@ Decision This new database table will have the following columns: 1. ``id``: unique primary key - 2. ``parent_id``: The ``CompetencyCriteriaGroup.id`` of the parent group. Null means this is a root group. - 3. ``oel_tagging_tag_id``: The ``oel_tagging_tag.id`` for the competency represented by this criteria tree. - 4. ``course_id``: Nullable foreign key for the course that scopes this criteria tree. Within a course-scoped subtree, every group carries the same ``course_id``, not only its topmost group; immutable after creation. Root groups always have ``course_id`` null. + 2. ``parent_id``: The ``CompetencyCriteriaGroup.id`` of the parent group. Null means this is a root group. Immutable after creation. + 3. ``oel_tagging_tag_id``: The ``oel_tagging_tag.id`` for the competency represented by this criteria tree. Immutable after creation. + 4. ``course_id``: Nullable foreign key for the course that scopes a course-scoped subtree. Only the course-level group carries it; the groups below it leave ``course_id`` null and belong to the course through that ancestor. Immutable after creation. Root groups always have ``course_id`` null. 5. ``name``: string 6. ``ordering``: Indicates evaluation sequence number for this criteria group. This defines deterministic evaluation order for siblings during read-time evaluation and event-driven recomputation, and enables short-circuit evaluation. 7. ``logic_operator``: Either “AND” or “OR” or null. This determines how children are combined at a group node ("AND" or "OR"). - 8. ``archived``: Boolean, defaults to false. Set instead of deleting a group once a student status exists for it. Archived groups are hidden from authoring and new associations but remain queryable, so existing ``CompetencyCriteria`` rows and learner status rows stay resolvable. + 8. ``archived``: Boolean, defaults to false. Set instead of deleting a group once a student status exists for it. Archived groups are hidden from authoring and new associations but remain queryable, so existing ``CompetencyCriteria`` rows and learner status rows stay resolvable. For MVP, archiving a group or a criterion is permanent, with the root as the one reversible exception: a new criterion for an archived root's competency reactivates that root instead of creating a second one, because archiving it for good would remove the ability to ever associate criteria with that competency again. Archiving a course-level group blocks that course for that competency; archiving a leaf closes only that leaf to new criteria. More unarchival capabilities may be added after MVP. + 9. ``scope_code``: Derived from ``oel_tagging_tag_id``, set on the root group and null on every other group, whether or not the root is archived. It exists so the database can hold at most one root per competency, the same way Decision 3's derived ``scope_code`` holds at most one live profile per scope. Example: A root group uses "OR" with two child groups. @@ -109,8 +110,8 @@ Decision - Empty groups: Persisted criteria definitions should not contain empty groups. Authoring flows may temporarily create empty groups while editing, but backend validation must reject them. - Mixed tree depths: Backend supports deeply nested groups. Current frontend authoring constraint is a maximum depth of 3 layers total, using zero-indexed depth (``0=root``, ``1=course-scope group``, ``2=leaf criteria/group``). - - Retrieval scope: A read always includes the root scope (all nodes where ``course_id is null``), plus any selected subset of course-scoped groups, each returned as a complete subtree, never truncated. - - Practical size and growth: Total rows in ``CompetencyCriteriaGroup`` are expected to grow over time as course runs are added; this ADR sets no global DB row cap. No max total node-count cap is required per root group. For ``course_id is null`` branches, expected size is small (realistically <=500 nodes). + - Retrieval scope: A read always includes the root group, plus any selected subset of course-scoped groups, each returned as a complete subtree, never truncated. + - Practical size and growth: Total rows in ``CompetencyCriteriaGroup`` are expected to grow over time as course runs are added; this ADR sets no global DB row cap. No max total node-count cap is required per root group. Each course-scoped subtree is expected to stay small (realistically <=500 nodes). 3. ``CompetencyRuleProfile`` concept (database table) @@ -131,7 +132,7 @@ Decision 2. ``organization_id``: The ``organization_id`` of the organization that this competency rule profile is scoped to. Null if it is not scoped to a specific organization. 3. ``course_id``: The ``course_id`` of the course that this competency rule profile is scoped to. Null if it is not scoped to a specific course. 4. ``competency_taxonomy_id``: The ``CompetencyTaxonomy.taxonomy_ptr_id`` of the competency taxonomy that this competency rule profile is scoped to. Null if it is not scoped to a specific taxonomy. - 5. ``scope_code``: A plain column in the format ``"org:X,course:Y,taxonomy:Z"``, with each segment left blank when the corresponding scope column is null: for example ``"org:5,course:,taxonomy:"``, or ``"org:,course:,taxonomy:"`` for the system default row. It is non-null when it is live, and null while archived. This frees an archived profile's scope for a replacement. + 5. ``scope_code``: A plain column in the format ``"org:X,course:Y,taxonomy:Z"``, with each segment left blank when the corresponding scope column is null: for example ``"org:5,course:,taxonomy:"``, or ``"org:,course:,taxonomy:"`` for the system default row. It is non-null when it is live, and null while archived. This frees an archived profile's scope for a replacement. ``CompetencyCriteriaGroup.scope_code`` (Decision 2) is kept while archived instead: each learner has one ``StudentCompetencyStatus`` row per competency, derived from the competency's root, so an archived root is reactivated rather than replaced, and every rollup and read can rely on exactly one root existing. 6. ``rule_type``: “View”, “Grade”, “MasteryLevel” (Only “Grade” will be supported for now) 7. ``rule_payload``: JSON payload keyed by ``rule_type`` to avoid freeform strings. It is structured JSON (not arbitrary freeform data): each ``rule_type`` defines the allowed payload shape and required keys, and validation enforces this contract. JSON is used instead of fixed columns like ``op``, ``value``, and ``scale`` so that future rule types (for example, ``MasteryLevel`` thresholds or plugin-defined evaluators such as CEL-based rules) can add their own fields without repeated schema migrations or many nullable columns. Examples: @@ -246,8 +247,10 @@ Decision 6. ``StudentCompetencyCriteriaStatus(user_id, competency_criteria_id)`` (unique -- each learner status table holds exactly one row per learner and target entity, updated in place; see ADR 0003 Decision 5) 7. ``StudentCompetencyCriteriaGroupStatus(user_id, competency_criteria_group_id)`` (unique) 8. ``StudentCompetencyStatus(user_id, oel_tagging_tag_id)`` (unique) - 9. ``CompetencyRuleProfile(scope_code)`` (unique -- at most one profile per distinct scope value; a plain unique constraint on the three raw nullable scope columns would not enforce this, since SQL never treats two ``NULL`` values as equal and this project's MySQL backend does not support the conditional/partial unique indexes that would otherwise route around that; see the ``scope_code`` column in Decision 3) - 10. ``CompetencyMasteryStatuses(status)`` (unique) + 9. ``CompetencyCriteriaGroup(scope_code)`` (unique -- at most one root group per competency, archived or not; see the ``scope_code`` column in Decision 2) + 10. ``CompetencyCriteriaGroup(oel_tagging_tag_id, course_id, parent_id)`` (unique -- at most one course-level group per competency and course) + 11. ``CompetencyRuleProfile(scope_code)`` (unique -- at most one profile per distinct scope value; a plain unique constraint on the three raw nullable scope columns would not enforce this, since SQL never treats two ``NULL`` values as equal and this project's MySQL backend does not support the conditional/partial unique indexes that would otherwise route around that; see the ``scope_code`` column in Decision 3) + 12. ``CompetencyMasteryStatuses(status)`` (unique) 6. Learner progress status concepts (``StudentCompetency*Status`` database tables) @@ -309,6 +312,7 @@ Decision - Once a related row exists in ``StudentCompetencyCriteriaStatus``, deletion of the associated competency definition row still succeeds, but as an archive (soft delete) instead of a hard delete: the row is hidden from authoring and new associations but remains queryable, so existing learner status rows stay resolvable. This archive-vs-hard-delete rule applies to ``oel_tagging_tag``, ``oel_tagging_taxonomy``, ``CompetencyTaxonomy``, ``oel_tagging_objecttag``, ``CompetencyCriteriaGroup``, and ``CompetencyCriteria``; see :ref:`openedx-learning-adr-0003` Decision 3 for ``oel_tagging_objecttag``'s own archive rule and traceability exception. - ``StudentCompetencyCriteriaStatus`` is what determines whether a record is protected. ``StudentCompetencyCriteriaGroupStatus`` and ``StudentCompetencyStatus`` are roll-up tables derived from it (Decision 6) and are not independently checked for this purpose: :ref:`openedx-learning-adr-0004` writes the leaf table synchronously with the grade but rolls the two roll-up tables up later via an asynchronous task, which can lag behind the leaf or, per that ADR's Decision 5, need manual recovery. Checking only the roll-up tables could therefore miss real learner progress that has not rolled up yet. - Direct deletion of a ``CompetencyRuleProfile`` is never a hard delete; retirement is always archive-only, via a normal update to its ``archived`` column (Decision 3). However, if a taxonomy or course that is associated with a taxonomy- or course-scoped profile is deleted, then this profile will be deleted along with it. + - For MVP, archiving a ``CompetencyCriteriaGroup`` or a ``CompetencyCriteria`` row is permanent, with one exception: an archived root ``CompetencyCriteriaGroup`` is reactivated when a new criterion is added for its competency (Decision 2). ``oel_tagging_objecttag`` follows :ref:`openedx-learning-adr-0003` Decision 3 instead. .. image:: images/CompetencyCriteriaModel.png :alt: Competency Criteria Model @@ -428,6 +432,16 @@ Rejected Alternatives 1. Silently does not work on this project's tested and production database backend. Django compiles a conditional ``UniqueConstraint`` to a partial index, which MySQL does not support; Django raises only a non-fatal system-check warning (``models.W036``) and skips creating the constraint, leaving the uniqueness rule completely unenforced at the database level. 2. The gap would surface only as a data-integrity incident under concurrent writes, not as a test or migration failure, since SQLite (used for quick local test runs) does support partial indexes and would mask the problem in that environment. +7. Let a new root replace an archived root for the same competency, by nulling the root's ``scope_code`` while archived as ``CompetencyRuleProfile`` does (Decision 3), instead of reactivating the archived root. + + 1. Pros + + 1. Both ``scope_code`` columns follow the same rule. + 2. An archived tree is never modified again. + 2. Cons + + 1. Each learner has a single ``StudentCompetencyStatus`` row per competency, derived from the root. With a live root beside archived ones, every rollup and every read across learners' statuses would first have to decide which root counts. + Changelog --------- @@ -458,3 +472,12 @@ Changelog * ``scope_code`` on ``CompetencyRuleProfile`` is now computed by application code instead of being database-generated, and is set to null while a profile is archived, freeing its scope for a replacement. + +2026-10-22: + +* For issue #872: added ``scope_code`` to ``CompetencyCriteriaGroup`` so the database holds at + most one root per competency, made ``oel_tagging_tag_id`` and ``parent_id`` immutable after + creation, scoped permanent archiving to groups and criteria for MVP with the archived root as + the one reversible exception, and added Rejected Alternative 7. Corrected ``course_id`` and the root scope: only + the course-level group carries ``course_id``, which supersedes the 2026-09-04 entry's statement + that every group in a course-scoped subtree does. That entry is left as originally written. diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index abe29ebf8..47234491f 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -180,9 +180,9 @@ def get_competency_criteria_tree(tag: Tag) -> CompetencyCriteriaTree: .order_by(F("parent_id").asc(nulls_first=True), "ordering", "id") ) - # Walk down from the roots instead of relying on row order: ordering by parent_id does not - # guarantee a parent comes before its child once groups can be moved, since a parent may - # then have a larger id than its child. + # Walk down from the roots instead of relying on row order. A group's parent is fixed once + # saved, but a QuerySet.update() can still give a group a parent with a larger id, and + # ordering by parent_id would then visit that child before its parent. children_by_parent_id: dict[int | None, list[int]] = {} for group in groups: children_by_parent_id.setdefault(group.parent_id, []).append(group.id) @@ -217,7 +217,9 @@ def _create_leaf_group( tag/course pair can hold more than one leaf, one per criterion. The root and course-level group names are truncated to fit the name column; they are never shown in the UI. - Must run inside a transaction holding the tag row lock. + The root and course-level lookups need no lock of their own: the unique constraints hold one + root per tag and one course-level group per tag and course, and ``get_or_create()`` recovers + from a concurrent insert. The caller's tag lock is for ``_reject_duplicate_criterion()``. """ name_max_length = CompetencyCriteriaGroup._meta.get_field("name").max_length # Both get_or_create() lookups match archived rows deliberately, so an archived container is @@ -494,7 +496,10 @@ def associate_competency_criterion( with transaction.atomic(): # Lock order is always the tag, then the course run, so two creates can't deadlock. - # Serializes the duplicate check and root creation; MySQL has no partial unique index for one root per tag. + # The database enforces one root per tag; the tag lock only serializes _reject_duplicate_criterion(). That + # check is check-then-act on both paths: without group_id a request always creates a fresh leaf, and with + # group_id CompetencyCriterion has no unique constraint on (object_tag, group), so no index catches a + # concurrent copy. Tag.objects.select_for_update().get(pk=tag.pk) # Serializes the per-course dominance check on CBE's own row, not the catalog's CourseRun. # Created first, since a locking read of a row that doesn't exist yet locks nothing; get_or_create() diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py index 9fb7d0119..dd2ba7156 100644 --- a/src/openedx_learning/applets/cbe/models/criteria.py +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -49,6 +49,10 @@ class CompetencyCriteriaGroup(models.Model): of child groups and leaf criteria, and only CompetencyCriteriaGroup carries an ``ordering`` field, so that mix has no total order; #641 accepts this deliberately. See ADR-0002 Decision 2. + ``scope_code`` is derived in ``save()``, so writes that skip ``save()`` (``bulk_create()``, + ``QuerySet.update()``, ``loaddata``, raw SQL) can leave a root without a code or with a stale + one, and the one-root-per-competency rule then goes unenforced for that root. + .. no_pii: """ @@ -104,8 +108,20 @@ class CompetencyCriteriaGroup(models.Model): "criteria and existing learner status rows stay resolvable." ), ) + # Recomputed in save(); set for roots only, archived or not, so an archived root keeps blocking a second one. + # Null below the root: a leaf has no natural key to put here, and the check constraint below only says a + # coded row is a root, not that every root is coded, so non-null would not make the rule any stronger. + scope_code = models.CharField( + max_length=255, + null=True, + editable=False, + help_text=_( + "Derived from tag and parent; \"tag:X\" for a root group, null for every other group." + ), + ) - history = HistoricalRecords() + # scope_code is derived bookkeeping; tag and parent, which it derives from, are already tracked. + history = HistoricalRecords(excluded_fields=["scope_code"]) class Meta: indexes = [ @@ -121,7 +137,7 @@ class Meta: constraints = [ # Backstops _create_leaf_group()'s get_or_create() for course-level groups. It has no # `condition` because MySQL has no partial indexes: a unique index allows repeated - # NULLs, and `course` is NULL for roots and leaves. + # NULLs, and `course` is NULL for roots and leaves. Roots are covered by scope_code instead. models.UniqueConstraint( fields=["tag", "course", "parent"], name="oel_cbe_criteria_group_one_course_group_per_tag_course", @@ -129,8 +145,48 @@ class Meta: "A competency tag may have at most one course-level CompetencyCriteriaGroup per course." ), ), + # Over the derived scope_code column because MySQL has no partial unique index for `parent IS NULL`. + models.UniqueConstraint( + fields=["scope_code"], + name="oel_cbe_criteria_group_scope_code_uniq", + violation_error_message=_("A competency tag may have at most one root CompetencyCriteriaGroup."), + ), + # One-directional: on MySQL, which cannot defer constraint checks, Django's delete collector + # sets each descendant's nullable `parent` to NULL before deleting it, so "every root has a + # scope_code" would reject those rows mid-delete. + models.CheckConstraint( + condition=Q(scope_code__isnull=True) | Q(parent__isnull=True), + name="oel_cbe_criteria_group_scope_code_root_only_check", + violation_error_message=_("Only a root CompetencyCriteriaGroup may have a scope_code."), + ), ] + def _check_tree_position_immutable(self) -> None: + """Raise ValidationError if tag, parent or course no longer matches what is persisted for this row.""" + if self.pk is None: + return + # Queried rather than compared against a value cached at load time, so a deferred load or + # a refresh_from_db() cannot bypass the check. + persisted = ( + CompetencyCriteriaGroup.objects.filter(pk=self.pk).values_list("tag_id", "parent_id", "course_id").first() + ) + if persisted is not None and (self.tag_id, self.parent_id, self.course_id) != persisted: + raise ValidationError( + _("A CompetencyCriteriaGroup's tag, parent and course cannot be changed after creation.") + ) + + def _compute_scope_code(self) -> str | None: + """Return this group's scope_code: set for a root group, None for every other group.""" + if self.parent_id is not None: + return None + return f"tag:{self.tag_id}" + + def save(self, *args, **kwargs): + """Reject a moved or re-tagged group, then set scope_code; tag and parent are fixed, so the code is too.""" + self._check_tree_position_immutable() + self.scope_code = self._compute_scope_code() + super().save(*args, **kwargs) + class CompetencyRuleProfile(models.Model): """ diff --git a/src/openedx_learning/migrations/0017_criteria_group_scope_code.py b/src/openedx_learning/migrations/0017_criteria_group_scope_code.py new file mode 100644 index 000000000..7f50bb21f --- /dev/null +++ b/src/openedx_learning/migrations/0017_criteria_group_scope_code.py @@ -0,0 +1,88 @@ +""" +Add CompetencyCriteriaGroup.scope_code and the constraints that allow one root group per competency tag. +""" +# Generated by Django 5.2.18 on 2026-10-22 14:17 + +from django.db import migrations, models +from django.db.models import CharField, Count, Value +from django.db.models.functions import Cast, Concat + + +class DuplicateRootCriteriaGroupsError(Exception): + """Raised when a competency tag has more than one root CompetencyCriteriaGroup.""" + + +def reject_duplicate_roots(apps, schema_editor): + """Raise DuplicateRootCriteriaGroupsError if any competency tag has more than one root group.""" + CompetencyCriteriaGroup = apps.get_model("openedx_learning", "CompetencyCriteriaGroup") + roots = CompetencyCriteriaGroup.objects.filter(parent__isnull=True) + duplicate_tag_ids = ( + roots.values("tag_id") + .annotate(root_count=Count("id")) + .filter(root_count__gt=1) + .values_list("tag_id", flat=True) + ) + duplicates = [ + (tag_id, list(roots.filter(tag_id=tag_id).order_by("id").values_list("id", flat=True))) + for tag_id in sorted(duplicate_tag_ids) + ] + if duplicates: + details = "; ".join( + f"tag {tag_id} (root group ids: {', '.join(str(group_id) for group_id in group_ids)})" + for tag_id, group_ids in duplicates + ) + raise DuplicateRootCriteriaGroupsError( + "Each competency tag may have at most one root CompetencyCriteriaGroup, archived or not. " + f"Merge or remove the duplicate roots before migrating: {details}." + ) + + +def backfill_root_scope_codes(apps, schema_editor): + """Set scope_code on every root group, archived or not, to its tag's root scope code.""" + CompetencyCriteriaGroup = apps.get_model("openedx_learning", "CompetencyCriteriaGroup") + # The historical model has no custom save(), so scope_code is computed here instead. + CompetencyCriteriaGroup.objects.filter(parent__isnull=True).update( + scope_code=Concat(Value("tag:"), Cast("tag_id", output_field=CharField())), + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0022_tag_external_id_not_null'), + ('openedx_catalog', '0002_pathways'), + ('openedx_learning', '0016_add_competency_course_run'), + ] + + operations = [ + # Runs first because MySQL commits each schema change: a failure after AddField would leave + # the column behind with the migration unrecorded. + migrations.RunPython(reject_duplicate_roots, migrations.RunPython.noop), + migrations.AddField( + model_name='competencycriteriagroup', + name='scope_code', + field=models.CharField( + editable=False, + help_text='Derived from tag and parent; "tag:X" for a root group, null for every other group.', + max_length=255, + null=True, + ), + ), + migrations.RunPython(backfill_root_scope_codes, migrations.RunPython.noop), + migrations.AddConstraint( + model_name='competencycriteriagroup', + constraint=models.UniqueConstraint( + fields=('scope_code',), + name='oel_cbe_criteria_group_scope_code_uniq', + violation_error_message='A competency tag may have at most one root CompetencyCriteriaGroup.', + ), + ), + migrations.AddConstraint( + model_name='competencycriteriagroup', + constraint=models.CheckConstraint( + condition=models.Q(('scope_code__isnull', True), ('parent__isnull', True), _connector='OR'), + name='oel_cbe_criteria_group_scope_code_root_only_check', + violation_error_message='Only a root CompetencyCriteriaGroup may have a scope_code.', + ), + ), + ] diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index f4dbcbd68..40e6488c1 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -1231,11 +1231,10 @@ def test_get_competency_criteria_tree_keeps_a_group_moved_under_a_later_created_ group_b = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) group_c = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) # A sits under B and B under C, so ordering by parent_id puts A before B: a single pass in - # that order would judge A before its parent B was kept. - group_b.parent = group_c - group_b.save() - group_a.parent = group_b - group_a.save() + # that order would judge A before its parent B was kept. save() rejects a parent change, so + # build the shape the way only a bulk update can. + CompetencyCriteriaGroup.objects.filter(pk=group_b.pk).update(parent=group_c) + CompetencyCriteriaGroup.objects.filter(pk=group_a.pk).update(parent=group_b) object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) criterion = CompetencyCriterion.objects.create( group=group_a, object_tag=object_tag, rule_profile=default_rule_profile, diff --git a/tests/openedx_learning/applets/cbe/test_criteria_group.py b/tests/openedx_learning/applets/cbe/test_criteria_group.py index 4836c4292..80dff99e1 100644 --- a/tests/openedx_learning/applets/cbe/test_criteria_group.py +++ b/tests/openedx_learning/applets/cbe/test_criteria_group.py @@ -4,7 +4,8 @@ Each test name states the behavior it pins. Reading top to bottom gives the model's contract: its columns, its tree shape, the two constraints ADR-0002 Decision 2 deliberately leaves out, -then its indexes and history. +one root per competency and one course-level group per competency and course, then its indexes +and history. Delete behavior is not covered here. Nothing in this module deletes a row that another row points at. See test_criteria_group_deletion.py, in this same change, for this model's own @@ -14,7 +15,10 @@ """ import pytest from django.apps import apps +from django.core.exceptions import ValidationError +from django.db import IntegrityError, transaction +from openedx_catalog.models import CourseRun from openedx_learning.models import CompetencyCriteriaGroup, LogicOperator from openedx_tagging.models import Tag @@ -61,9 +65,11 @@ def test_group_logic_operator_accepts_and_or_and_null_regardless_of_child_count( childless = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=logic_operator) assert childless.pk is not None - parent = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=logic_operator) - CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent) - CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent) + # A second competency, since each competency tag may have only one root group. + other_tag = Tag.objects.create(taxonomy=tag.taxonomy, value="Writing Prose") + parent = CompetencyCriteriaGroup.objects.create(tag=other_tag, logic_operator=logic_operator) + CompetencyCriteriaGroup.objects.create(tag=other_tag, parent=parent) + CompetencyCriteriaGroup.objects.create(tag=other_tag, parent=parent) parent.refresh_from_db() assert parent.logic_operator == logic_operator assert CompetencyCriteriaGroup.objects.filter(parent=parent).count() == 2 @@ -94,6 +100,136 @@ def test_group_has_no_unique_constraint_on_parent_and_ordering(tag: Tag) -> None assert sibling_a.ordering == sibling_b.ordering == 1 +# --------------------------------------------------------------------------------------------- +# One root per competency, and one course-level group per competency and course + + +# --------------------------------------------------------------------------------------------- + + +def test_a_root_group_gets_a_scope_code_and_other_groups_do_not(tag: Tag, course_run: CourseRun) -> None: + second_run = CourseRun.objects.create(catalog_course=course_run.catalog_course, run_code="Spring2027") + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + second_course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=second_run, parent=root) + leaves = [ + CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level, archived=archived) + for archived in (False, False, True) + ] + + assert root.scope_code == f"tag:{tag.pk}" + assert course_level.scope_code is None + assert second_course_level.scope_code is None + assert all(leaf.scope_code is None for leaf in leaves) + + +def test_a_second_root_for_a_competency_is_rejected(tag: Tag) -> None: + CompetencyCriteriaGroup.objects.create(tag=tag) + + with pytest.raises(IntegrityError), transaction.atomic(): + CompetencyCriteriaGroup.objects.create(tag=tag) + + assert CompetencyCriteriaGroup.objects.filter(tag=tag, parent=None).count() == 1 + + +def test_a_second_root_is_rejected_when_the_existing_root_is_archived(tag: Tag) -> None: + root = CompetencyCriteriaGroup.objects.create(tag=tag, archived=True) + + with pytest.raises(IntegrityError), transaction.atomic(): + CompetencyCriteriaGroup.objects.create(tag=tag) + + root.refresh_from_db() + assert root.archived is True + assert root.scope_code == f"tag:{tag.pk}" + + +@pytest.mark.parametrize("existing_archived", [False, True], ids=["live", "archived"]) +@pytest.mark.parametrize("insert", ["create", "bulk_create"]) +def test_a_second_course_level_group_for_the_same_course_is_rejected( + existing_archived: bool, insert: str, tag: Tag, course_run: CourseRun +) -> None: + root = CompetencyCriteriaGroup.objects.create(tag=tag) + CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root, archived=existing_archived) + + with pytest.raises(IntegrityError), transaction.atomic(): + if insert == "create": + CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + else: + CompetencyCriteriaGroup.objects.bulk_create( + [CompetencyCriteriaGroup(tag=tag, course=course_run, parent=root)] + ) + + +def test_archiving_a_root_keeps_it_blocking_a_second_root(tag: Tag) -> None: + root = CompetencyCriteriaGroup.objects.create(tag=tag) + + root.archived = True + root.save(update_fields=["archived"]) + + root.refresh_from_db() + assert root.scope_code == f"tag:{tag.pk}" + with pytest.raises(IntegrityError), transaction.atomic(): + CompetencyCriteriaGroup.objects.create(tag=tag) + + +@pytest.mark.parametrize("changed_field", ["tag", "parent", "course"]) +def test_a_saved_group_cannot_change_its_tag_parent_or_course( + changed_field: str, tag: Tag, course_run: CourseRun +) -> None: + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + + if changed_field == "tag": + course_level.tag = Tag.objects.create(taxonomy=tag.taxonomy, value="Writing Prose") + elif changed_field == "parent": + course_level.parent = None + else: + course_level.course = CourseRun.objects.create(catalog_course=course_run.catalog_course, run_code="Spring2027") + + with pytest.raises(ValidationError): + course_level.save() + course_level.refresh_from_db() + assert course_level.tag_id == tag.pk + assert course_level.parent_id == root.pk + assert course_level.course_id == course_run.pk + + +def test_bulk_archiving_a_root_keeps_it_blocking_a_second_root(tag: Tag) -> None: + root = CompetencyCriteriaGroup.objects.create(tag=tag) + + CompetencyCriteriaGroup.objects.filter(pk=root.pk).update(archived=True) + + root.refresh_from_db() + assert root.scope_code == f"tag:{tag.pk}" + with pytest.raises(IntegrityError), transaction.atomic(): + CompetencyCriteriaGroup.objects.create(tag=tag) + + +def test_reactivating_an_archived_root_keeps_it_the_only_root(tag: Tag) -> None: + root = CompetencyCriteriaGroup.objects.create(tag=tag) + root.archived = True + root.save(update_fields=["archived"]) + + root.archived = False + root.save() + + root.refresh_from_db() + assert root.archived is False + assert root.scope_code == f"tag:{tag.pk}" + with pytest.raises(IntegrityError), transaction.atomic(): + CompetencyCriteriaGroup.objects.create(tag=tag) + assert CompetencyCriteriaGroup.objects.filter(tag=tag, parent=None).count() == 1 + + +def test_a_root_cannot_be_given_a_parent_behind_save(tag: Tag) -> None: + other_tag = Tag.objects.create(taxonomy=tag.taxonomy, value="Writing Prose") + root_a = CompetencyCriteriaGroup.objects.create(tag=tag) + root_b = CompetencyCriteriaGroup.objects.create(tag=other_tag) + + with pytest.raises(IntegrityError), transaction.atomic(): + CompetencyCriteriaGroup.objects.filter(pk=root_b.pk).update(parent=root_a) + + # --------------------------------------------------------------------------------------------- # Indexes and history diff --git a/tests/openedx_learning/applets/cbe/test_criteria_group_deletion.py b/tests/openedx_learning/applets/cbe/test_criteria_group_deletion.py index 895bebc8e..4ad2936b5 100644 --- a/tests/openedx_learning/applets/cbe/test_criteria_group_deletion.py +++ b/tests/openedx_learning/applets/cbe/test_criteria_group_deletion.py @@ -54,21 +54,26 @@ def test_deleting_a_course_run_also_deletes_its_course_scoped_criteria_groups( tag: Tag, course_run: CourseRun ) -> None: """ - Deleting a CourseRun cascades to any CompetencyCriteriaGroup scoped to it via `course`: the - delete succeeds and the group row is gone too. A course-scoped criteria tree has no meaning - once the course run it evaluates against no longer exists. + Deleting a CourseRun cascades to any CompetencyCriteriaGroup scoped to it via `course`, and + through that group's `parent` CASCADE to the leaf groups under it: the delete succeeds, the + course-level group and leaf rows are gone, and the root above them stays. A course-scoped + subtree has no meaning once the course run it evaluates against no longer exists. `course` is a nullable cascading foreign key. MySQL cannot defer foreign-key constraint checks, so Django's collector nulls such a key before the DELETE; SQLite never does. A regression in that MySQL path therefore fails this test in CI (which runs MySQL) while still passing locally on SQLite. """ - group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run) - assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + root = CompetencyCriteriaGroup.objects.create(tag=tag) + group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + leaves = [CompetencyCriteriaGroup.objects.create(tag=tag, parent=group) for _ in range(2)] + group_pks = [group.pk, *(leaf.pk for leaf in leaves)] + assert CompetencyCriteriaGroup.objects.filter(pk__in=group_pks).count() == 3 course_run.delete() - assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk__in=group_pks).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=root.pk).exists() def test_deleting_a_group_at_depth_also_deletes_every_descendant_group(tag: Tag) -> None: @@ -102,3 +107,18 @@ def test_deleting_a_taxonomy_also_deletes_its_tags_criteria_groups(competency_ta assert not Tag.objects.filter(pk=tag.pk).exists() assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + + +def test_deleting_a_root_with_descendants_frees_its_place(tag: Tag, course_run: CourseRun) -> None: + """Deleting a root deletes its whole subtree and frees the tag for a new root.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level_group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + leaves = [CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level_group) for _ in range(2)] + group_pks = [root.pk, course_level_group.pk, *(leaf.pk for leaf in leaves)] + assert CompetencyCriteriaGroup.objects.filter(pk__in=group_pks).count() == 4 + + root.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk__in=group_pks).exists() + new_root = CompetencyCriteriaGroup.objects.create(tag=tag) + assert new_root.scope_code == f"tag:{tag.pk}" diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 983d252ae..2b5f12edb 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -1199,6 +1199,7 @@ def test_full_tree_combines_multiple_courses_in_one_unscoped_response( def test_tree_response_never_references_a_group_it_left_out( user_client: APIClient, + organization: Organization, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, @@ -1207,23 +1208,18 @@ def test_tree_response_never_references_a_group_it_left_out( leaf = _create_leaf_group(tag, course_run) object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, rule_profile=default_rule_profile) - sibling_leaf = _create_leaf_group(tag, course_run) - sibling_object_tag = ObjectTag.objects.create( - object_id=usage_key(course_run, "p2"), taxonomy=tag.taxonomy, tag=tag, + # The kept leaf lives under a second course's course-level group, so archiving the first + # course-level group leaves it attached to a returned parent. + other_course_run = make_course_run(organization, "Python200", "Fall2026") + kept_leaf = _create_leaf_group(tag, other_course_run) + kept_object_tag = ObjectTag.objects.create( + object_id=usage_key(other_course_run, "p1"), taxonomy=tag.taxonomy, tag=tag, ) kept_criterion = CompetencyCriterion.objects.create( - group=sibling_leaf, object_tag=sibling_object_tag, rule_profile=default_rule_profile, + group=kept_leaf, object_tag=kept_object_tag, rule_profile=default_rule_profile, ) course_level = leaf.parent assert course_level is not None - # Both leaves share one course-level group, so detach the sibling under a kept parent. - kept_course_level = sibling_leaf.parent - assert kept_course_level is not None - assert kept_course_level.id == course_level.id - root = course_level.parent - assert root is not None - sibling_leaf.parent = root - sibling_leaf.save() course_level.archived = True course_level.save() diff --git a/tests/openedx_learning/test_migrations.py b/tests/openedx_learning/test_migrations.py new file mode 100644 index 000000000..36f171e4c --- /dev/null +++ b/tests/openedx_learning/test_migrations.py @@ -0,0 +1,123 @@ +""" +Tests for the 0017_criteria_group_scope_code migration's duplicate-root check and backfill. + +Uses the `migrator` pytest fixture from django_test_migrations to run the actual +migration against historical (frozen) model states, the same way it will run against +real downstream data. +""" +from __future__ import annotations + +import importlib + +import pytest +from django.db import IntegrityError, connection, transaction + +from openedx_learning.models import CompetencyCriteriaGroup + +MIGRATE_FROM = ("openedx_learning", "0016_add_competency_course_run") +MIGRATE_TO = ("openedx_learning", "0017_criteria_group_scope_code") + +DuplicateRootCriteriaGroupsError = importlib.import_module( + "openedx_learning.migrations.0017_criteria_group_scope_code" +).DuplicateRootCriteriaGroupsError + + +def _create_tag(apps, taxonomy, value): + """Create a root-level tag with a historical model.""" + Tag = apps.get_model("oel_tagging", "Tag") + return Tag.objects.create(taxonomy=taxonomy, value=value, external_id=value, depth=0, lineage=value + "\t") + + +def _create_taxonomy(apps): + """Create a taxonomy with a historical model.""" + Taxonomy = apps.get_model("oel_tagging", "Taxonomy") + return Taxonomy.objects.create(name="Migration Test", export_id="migration_test") + + +def _column_names(Group) -> set[str]: + with connection.cursor() as cursor: + description = connection.introspection.get_table_description(cursor, Group._meta.db_table) + return {column.name for column in description} + + +@pytest.mark.django_db +def test_backfill_sets_scope_code_on_roots_only(migrator) -> None: + """ + Every root group, archived or not, gets its tag's root scope_code; course-level groups + and leaves stay null. + """ + old_state = migrator.apply_initial_migration(MIGRATE_FROM) + apps = old_state.apps + Organization = apps.get_model("organizations", "Organization") + CatalogCourse = apps.get_model("openedx_catalog", "CatalogCourse") + CourseRun = apps.get_model("openedx_catalog", "CourseRun") + Group = apps.get_model("openedx_learning", "CompetencyCriteriaGroup") + + taxonomy = _create_taxonomy(apps) + tag_a = _create_tag(apps, taxonomy, "Competency A") + tag_b = _create_tag(apps, taxonomy, "Competency B") + org = Organization.objects.create(name="Org1", short_name="Org1") + catalog_course = CatalogCourse.objects.create(org=org, course_code="Math100", title="Math", language="en") + course_run = CourseRun.objects.create( + catalog_course=catalog_course, run_code="A", course_key="course-v1:Org1+Math100+A", title="Math", + ) + + root_a = Group.objects.create(tag=tag_a) + course_group = Group.objects.create(tag=tag_a, parent=root_a, course=course_run) + leaf = Group.objects.create(tag=tag_a, parent=course_group) + archived_leaf = Group.objects.create(tag=tag_a, parent=course_group, archived=True) + archived_root_b = Group.objects.create(tag=tag_b, archived=True) + + new_state = migrator.apply_tested_migration(MIGRATE_TO) + NewGroup = new_state.apps.get_model("openedx_learning", "CompetencyCriteriaGroup") + + assert NewGroup.objects.get(pk=root_a.pk).scope_code == f"tag:{tag_a.pk}" + assert NewGroup.objects.get(pk=archived_root_b.pk).scope_code == f"tag:{tag_b.pk}" + assert NewGroup.objects.get(pk=course_group.pk).scope_code is None + assert NewGroup.objects.get(pk=leaf.pk).scope_code is None + assert NewGroup.objects.get(pk=archived_leaf.pk).scope_code is None + # The live model's save() must produce the code the backfill wrote, or a second root would slip past it. This + # INSERT goes through the live model at state 0017, so a later migration that adds a column to this table breaks + # it; when that happens, compare the backfilled value with the live model's computed code instead. + with pytest.raises(IntegrityError), transaction.atomic(): + CompetencyCriteriaGroup.objects.create(tag_id=tag_a.pk) + + +@pytest.mark.django_db +def test_migration_reverses_cleanly(migrator) -> None: + """Migrating back to 0016 drops scope_code and keeps existing groups.""" + new_state = migrator.apply_initial_migration(MIGRATE_TO) + apps = new_state.apps + Group = apps.get_model("openedx_learning", "CompetencyCriteriaGroup") + tag = _create_tag(apps, _create_taxonomy(apps), "Competency A") + root = Group.objects.create(tag=tag, scope_code=f"tag:{tag.pk}") + + old_state = migrator.apply_tested_migration(MIGRATE_FROM) + OldGroup = old_state.apps.get_model("openedx_learning", "CompetencyCriteriaGroup") + + assert OldGroup.objects.filter(pk=root.pk).exists() + assert "scope_code" not in _column_names(OldGroup) + + +@pytest.mark.django_db +def test_duplicate_roots_stop_the_migration_before_any_schema_change(migrator) -> None: + """ + A tag with two roots, one of them archived, fails the migration with an error naming the + tag and both groups, and leaves the table without a scope_code column. + """ + old_state = migrator.apply_initial_migration(MIGRATE_FROM) + apps = old_state.apps + Group = apps.get_model("openedx_learning", "CompetencyCriteriaGroup") + tag = _create_tag(apps, _create_taxonomy(apps), "Competency A") + live_root = Group.objects.create(tag=tag) + archived_root = Group.objects.create(tag=tag, archived=True) + + try: + with pytest.raises(DuplicateRootCriteriaGroupsError) as excinfo: + migrator.apply_tested_migration(MIGRATE_TO) + + assert f"tag {tag.pk} (root group ids: {live_root.pk}, {archived_root.pk})" in str(excinfo.value) + assert "scope_code" not in _column_names(Group) + finally: + # The migrator fixture migrates to the latest state on teardown, which the duplicate would block. + archived_root.delete()