From b24e5aa89398df9cb654f3c08c4db9a9596e113c Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Tue, 1 Sep 2026 16:22:52 -0400 Subject: [PATCH 1/5] feat: add competency criteria models for CBE authoring layer Implements the authoring and definition half of the CBE data model from ADR-0002: CompetencyCriteriaGroup (internal AND/OR nodes), CompetencyRuleProfile (reusable scoped evaluation defaults) and CompetencyCriterion (leaf nodes). Also adds the taxonomy_overrides_org column that PR #712 left off CompetencyTaxonomy. CompetencyRuleProfile.scope_code is a generated, never-null column with a plain unique constraint. SQL never treats two NULLs as equal, so a unique constraint over the three nullable scope columns would accept two rows with the same scope, and the conditional UniqueConstraint that would normally fix that compiles to a partial index MySQL does not support. Both structural invariants are database check constraints rather than clean() checks, since DRF serializers, QuerySet.update() and bulk_create() never call full_clean(). Payload shape validation stays in clean(), per the issue. Every new foreign key is on_delete=PROTECT with a TODO(#799) comment. That is a fail-closed placeholder, not a per-key decision; #799 sets the real values once #655 lands. openedx_catalog joins .importlinter's root_packages and the src_layering contract, since CompetencyCriteriaGroup.course is the first foreign key from openedx_learning into that app. django-simple-history moves into base.in: it was only ever a transitive dependency of edx-organizations, and setup.py builds install_requires from base.in. Refs #641 Co-Authored-By: Claude Opus 5 (1M context) --- .annotation_safe_list.yml | 6 + .importlinter | 6 + mypy.ini | 3 + requirements/base.in | 2 + requirements/base.txt | 4 +- .../applets/cbe/models/__init__.py | 23 + .../competency_taxonomy.py} | 18 +- .../applets/cbe/models/criteria.py | 429 ++++++++++++++++++ .../migrations/0002_competency_criteria.py | 165 +++++++ .../0003_seed_default_rule_profile.py | 39 ++ .../applets/cbe/test_criteria_models.py | 402 ++++++++++++++++ .../applets/cbe/test_models.py | 8 + 12 files changed, 1103 insertions(+), 2 deletions(-) create mode 100644 src/openedx_learning/applets/cbe/models/__init__.py rename src/openedx_learning/applets/cbe/{models.py => models/competency_taxonomy.py} (60%) create mode 100644 src/openedx_learning/applets/cbe/models/criteria.py create mode 100644 src/openedx_learning/migrations/0002_competency_criteria.py create mode 100644 src/openedx_learning/migrations/0003_seed_default_rule_profile.py create mode 100644 tests/openedx_learning/applets/cbe/test_criteria_models.py diff --git a/.annotation_safe_list.yml b/.annotation_safe_list.yml index 6b9f74d07..65b803cd4 100644 --- a/.annotation_safe_list.yml +++ b/.annotation_safe_list.yml @@ -77,6 +77,12 @@ openedx_content.Unit: ".. no_pii:": "This model has no PII" openedx_content.UnitVersion: ".. no_pii:": "This model has no PII" +openedx_learning.HistoricalCompetencyCriteriaGroup: + ".. no_pii:": "This model has no PII" +openedx_learning.HistoricalCompetencyCriterion: + ".. no_pii:": "This model has no PII" +openedx_learning.HistoricalCompetencyRuleProfile: + ".. no_pii:": "This model has no PII" social_django.Association: ".. no_pii:": "This model has no PII" social_django.Code: diff --git a/.importlinter b/.importlinter index 17dd176f6..844575b91 100644 --- a/.importlinter +++ b/.importlinter @@ -7,6 +7,7 @@ root_packages = openedx_learning openedx_content + openedx_catalog openedx_tagging openedx_django_lib openedx_core @@ -26,6 +27,11 @@ layers = # Content: authoring-side models and APIs. openedx_content + # Catalog: CatalogCourse/CourseRun. CompetencyCriteriaGroup and CompetencyRuleProfile + # (openedx_learning) scope to a CourseRun, so this must sit below openedx_learning; it doesn't + # depend on tagging or content, so it can sit above openedx_tagging. + openedx_catalog + # Tagging is very simple & fundamental. Should probably not depend on any other Django apps. openedx_tagging diff --git a/mypy.ini b/mypy.ini index b383a8816..665714903 100644 --- a/mypy.ini +++ b/mypy.ini @@ -12,5 +12,8 @@ files = [mypy-organizations.*] follow_untyped_imports = True +[mypy-simple_history.*] +follow_untyped_imports = True + [mypy.plugins.django-stubs] django_settings_module = "projects.dev" diff --git a/requirements/base.in b/requirements/base.in index 626a551be..7292e10c3 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -17,3 +17,5 @@ rules<4.0 # Django extension for rules-based authorization check tomlkit # Parses and writes TOML configuration files edx-organizations # Implemented the "Organization" model that CatalogCourse/CourseRun are keyed to + +django-simple-history # History tracking for CBE criteria definitions, per ADR-0003 diff --git a/requirements/base.txt b/requirements/base.txt index c7dac57c3..c89949f77 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -58,7 +58,9 @@ django-crum==0.7.9 django-model-utils==5.0.0 # via edx-organizations django-simple-history==3.13.0 - # via edx-organizations + # via + # -r requirements/base.in + # edx-organizations django-waffle==5.0.0 # via # edx-django-utils diff --git a/src/openedx_learning/applets/cbe/models/__init__.py b/src/openedx_learning/applets/cbe/models/__init__.py new file mode 100644 index 000000000..9d71edfa4 --- /dev/null +++ b/src/openedx_learning/applets/cbe/models/__init__.py @@ -0,0 +1,23 @@ +""" +Models for Competency-Based Education (CBE). +""" + +from .competency_taxonomy import CompetencyTaxonomy +from .criteria import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + LogicOperator, + RuleType, + validate_rule_payload, +) + +__all__ = [ + "CompetencyTaxonomy", + "CompetencyCriteriaGroup", + "CompetencyCriterion", + "CompetencyRuleProfile", + "LogicOperator", + "RuleType", + "validate_rule_payload", +] diff --git a/src/openedx_learning/applets/cbe/models.py b/src/openedx_learning/applets/cbe/models/competency_taxonomy.py similarity index 60% rename from src/openedx_learning/applets/cbe/models.py rename to src/openedx_learning/applets/cbe/models/competency_taxonomy.py index 7cbd8cb3a..a0623fa6e 100644 --- a/src/openedx_learning/applets/cbe/models.py +++ b/src/openedx_learning/applets/cbe/models/competency_taxonomy.py @@ -1,6 +1,9 @@ """ -Models for Competency-Based Education (CBE). +The CompetencyTaxonomy model. """ +from django.db import models +from django.utils.translation import gettext_lazy as _ + from openedx_tagging.models import Taxonomy __all__ = [ @@ -35,6 +38,19 @@ class CompetencyTaxonomy(Taxonomy): .. no_pii: """ + taxonomy_overrides_org = models.BooleanField( + default=False, + help_text=_( + "Resolves a tie when assigning a CompetencyRuleProfile to a CompetencyCriterion (ADR-0002 " + "Decision 4): if both an organization-scoped profile and a taxonomy-scoped profile from this " + "taxonomy apply to the same criterion, False (the default) assigns the organization-scoped " + "profile, and True assigns this taxonomy's own profile instead, so it cannot be locally " + "weakened by an organization. Nothing reads this field yet: organization-scoped " + "CompetencyRuleProfile rows do not exist in this phase, so the tie it resolves cannot arise " + "until they do." + ), + ) + class Meta: verbose_name = "Competency Taxonomy" verbose_name_plural = "Competency Taxonomies" diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py new file mode 100644 index 000000000..d008bfa93 --- /dev/null +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -0,0 +1,429 @@ +""" +Models for CompetencyAchievementCriteria: CompetencyCriteriaGroup, CompetencyRuleProfile, CompetencyCriterion. + +See :ref:`openedx-learning-adr-0002` for the design this module implements, and +:ref:`openedx-learning-adr-0003` for why these three models (and not CompetencyTaxonomy) carry +``django-simple-history`` tracking. +""" +from __future__ import annotations + +from typing import Any + +from django.core.exceptions import ValidationError +from django.db import models +from django.db.models import F, Q, Value +from django.db.models.functions import Cast, Coalesce, Concat +from django.utils.translation import gettext_lazy as _ +from organizations.models import Organization +from simple_history.models import HistoricalRecords + +from openedx_catalog.models import CourseRun +from openedx_django_lib.fields import case_insensitive_char_field, immutable_uuid_field +from openedx_tagging.models import ObjectTag, Tag + +from .competency_taxonomy import CompetencyTaxonomy + +__all__ = [ + "CompetencyCriteriaGroup", + "CompetencyCriterion", + "CompetencyRuleProfile", + "LogicOperator", + "RuleType", + "validate_rule_payload", +] + + +class RuleType(models.TextChoices): + """The evaluation rule types a CompetencyRuleProfile or CompetencyCriterion override can use.""" + + VIEW = "View", _("View") + GRADE = "Grade", _("Grade") + MASTERY_LEVEL = "MasteryLevel", _("Mastery Level") + + +class LogicOperator(models.TextChoices): + """How a CompetencyCriteriaGroup combines its child nodes.""" + + AND = "AND", _("And") + OR = "OR", _("Or") + + +def validate_rule_payload(rule_type: str, payload: Any) -> None: + """ + Validate ``payload`` against the shape ADR-0002 Decision 3 defines for ``rule_type``. + + Only ``RuleType.GRADE`` has a defined payload shape in this phase. ``RuleType.VIEW`` and + ``RuleType.MASTERY_LEVEL`` are valid choices elsewhere but are rejected here, since no + payload contract exists for them yet. Raises ``django.core.exceptions.ValidationError`` on + any mismatch; never returns a value. + """ + if rule_type != RuleType.GRADE: + raise ValidationError( + _("Rule type '%(rule_type)s' is not supported yet; only 'Grade' has a defined rule_payload shape.") + % {"rule_type": rule_type} + ) + if not isinstance(payload, dict): + raise ValidationError(_("A 'Grade' rule_payload must be a JSON object.")) + + allowed_keys = {"op", "value", "scale"} + if set(payload.keys()) != allowed_keys: + raise ValidationError( + _("A 'Grade' rule_payload must have exactly these keys, no more and no fewer: op, value, scale.") + ) + + if payload.get("op") not in {"gte", "lte", "eq"}: + raise ValidationError(_("The 'op' in a 'Grade' rule_payload must be one of: gte, lte, eq.")) + + value = payload.get("value") + # isinstance(True, int) is True in Python, so a bool would otherwise pass the numeric check below. + if isinstance(value, bool) or not isinstance(value, (int, float)): + raise ValidationError(_("The 'value' in a 'Grade' rule_payload must be a number, not a boolean.")) + if not 0.0 <= value <= 1.0: + raise ValidationError( + _( + "The 'value' in a 'Grade' rule_payload must be a fraction between 0.0 and 1.0 inclusive " + "(e.g. 0.8 for a passing grade of 80%%), not %(value)r." + ) + % {"value": value} + ) + + if payload.get("scale") != "percent": + raise ValidationError(_("The 'scale' in a 'Grade' rule_payload must be 'percent'.")) + + +class CompetencyCriteriaGroup(models.Model): + """ + An internal AND/OR node in a CompetencyAchievementCriteria expression tree. + + A single CompetencyAchievementCriteria is one root CompetencyCriteriaGroup plus all of its + descendant groups and leaf :class:`CompetencyCriterion` rows. ``logic_operator`` says how + this group's children combine; ``ordering`` gives their deterministic evaluation sequence, + which read-time evaluation and event-driven recomputation both rely on for short-circuiting. + See ADR-0002 Decision 2. + + .. no_pii: + """ + + uuid = immutable_uuid_field() + parent = models.ForeignKey( + "self", + null=True, + blank=True, + # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior + on_delete=models.PROTECT, + related_name="child_groups", + help_text=_("The parent CompetencyCriteriaGroup. Null means this group is a tree root."), + ) + tag = models.ForeignKey( + Tag, + db_column="oel_tagging_tag_id", + # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior + on_delete=models.PROTECT, + related_name="competency_criteria_groups", + help_text=_("The competency (tag) that this criteria tree evaluates mastery of."), + ) + course = models.ForeignKey( + CourseRun, + null=True, + blank=True, + # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior + on_delete=models.PROTECT, + related_name="competency_criteria_groups", + help_text=_("The course run that scopes this criteria tree for evaluation windowing, if any."), + ) + name = case_insensitive_char_field(max_length=255, blank=True, default="") + ordering = models.PositiveIntegerField( + default=0, + help_text=_( + "Deterministic sibling evaluation sequence. Used to short-circuit evaluation and to order " + "child scans during event-driven recomputation." + ), + ) + logic_operator = models.CharField( + max_length=3, + choices=LogicOperator, + null=True, + blank=True, + help_text=_("How this group's children combine. Null until the group has children to combine."), + ) + + history = HistoricalRecords() + + class Meta: + indexes = [ + # ADR-0002 Decision 5, index 1: lookups by competency tag and course scope. + models.Index(fields=["tag", "course"]), + # ADR-0002 Decision 5 also lists an index on `parent` (index 2), but Django already + # indexes every ForeignKey column by default, so a second explicit one here would only + # cost write throughput without adding any read benefit. + ] + # ADR-0002 Decision 2 explicitly excludes two constraints here, both for the same reason: + # a child group cannot be saved until its parent's primary key exists, so at the moment a + # parent group is being validated/saved, its clean() always sees zero children, whether or + # not more are about to be attached. There's no single-row state at save time to check either + # of these against: + # - A constraint tying `logic_operator` to child count. + # - A UniqueConstraint on (parent, ordering), which would need to see all siblings, not just + # the row being saved. + + +class CompetencyRuleProfile(models.Model): + """ + A reusable default evaluation rule, optionally scoped to a taxonomy, course, or organization. + + Each row is scoped by at most one of ``organization``, ``course``, and ``competency_taxonomy``, + enforced by the check constraint below; the row with all three null is the system default, + seeded once by migration and never created or deleted through the profile API. See ADR-0002 + Decision 3 for how a :class:`CompetencyCriterion` is assigned one of these, and Decision 4 for + what happens when more than one scope's profile could apply to the same criterion. + + Editing a profile may change ``rule_type``/``rule_payload`` only: the scope fields + (``organization``, ``course``, ``competency_taxonomy``) are immutable after creation, so that + criteria already resolved to this profile's scope are never silently re-governed. This is + enforced in ``clean()`` and ``save()`` by comparing against the scope this row had when + loaded. That comparison covers every ``instance.save()``, including one loaded with + ``.only()``/``.defer()`` that skipped some scope columns, in which case the comparison falls + back to reading the persisted scope directly rather than skipping the check. It does not + cover a bulk ``QuerySet.update()``, since that path never loads or constructs a model + instance at all. + + .. no_pii: + """ + + # Set at from_db() time to the scope this row had when it was loaded from the database, so + # clean()/save() can detect an attempt to change it. None for a newly-constructed instance, + # meaning there's nothing yet to compare against. Deliberately not underscore-prefixed: + # from_db() is a classmethod, so it sets this through a local `instance` variable rather than + # `self`, which pylint's protected-access check can't tell apart from reaching into another + # object's internals. + loaded_scope: tuple[int | None, int | None, int | None] | None = None + + organization = models.ForeignKey( + Organization, + null=True, + blank=True, + # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior + on_delete=models.PROTECT, + related_name="competency_rule_profiles", + help_text=_("The organization this profile is scoped to, if any."), + ) + course = models.ForeignKey( + CourseRun, + null=True, + blank=True, + # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior + on_delete=models.PROTECT, + related_name="competency_rule_profiles", + help_text=_("The course run this profile is scoped to, if any."), + ) + competency_taxonomy = models.ForeignKey( + CompetencyTaxonomy, + null=True, + blank=True, + # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior + on_delete=models.PROTECT, + related_name="rule_profiles", + help_text=_("The competency taxonomy this profile is scoped to, if any."), + ) + # Always non-null, including for the system-default row (all three scope columns null), so a + # plain UniqueConstraint on this one column enforces "at most one profile row per distinct + # scope" identically on every backend. SQL never treats two NULLs as equal, so a unique + # constraint directly on the three nullable scope columns would let e.g. two rows that both + # set only organization_id=5 both exist. See ADR-0002 Decision 3. + scope_code = models.GeneratedField( + expression=Concat( + Value("org:"), + Coalesce(Cast(F("organization_id"), output_field=models.CharField(max_length=20)), Value("")), + Value(",course:"), + Coalesce(Cast(F("course_id"), output_field=models.CharField(max_length=20)), Value("")), + Value(",taxonomy:"), + Coalesce(Cast(F("competency_taxonomy_id"), output_field=models.CharField(max_length=20)), Value("")), + ), + output_field=models.CharField(max_length=255), + db_persist=True, + ) + rule_type = models.CharField(max_length=32, choices=RuleType) + rule_payload = models.JSONField( + help_text=_("Structured payload keyed by rule_type; see validate_rule_payload for the shape it must match.") + ) + archived = models.BooleanField( + default=False, + help_text=_( + "Set instead of deleting a profile that's no longer wanted. Archived profiles are hidden from " + "authoring and new associations but remain queryable, so existing criteria stay resolvable." + ), + ) + uuid = immutable_uuid_field() + + history = HistoricalRecords(excluded_fields=["scope_code"]) + + class Meta: + constraints = [ + # Do NOT add `condition=` here. A conditional UniqueConstraint compiles to a partial + # index, which MySQL (this project's tested and production database) does not support: + # Django only raises a non-fatal system-check warning (models.W036) and silently skips + # creating the constraint, leaving uniqueness completely unenforced there, while SQLite + # (used for quick local test runs) does support partial indexes and would mask the gap + # in that environment. See ADR-0002 Rejected Alternative 6. The generated `scope_code` + # column above exists specifically so a plain, unconditional UniqueConstraint works + # identically on every backend. + models.UniqueConstraint(fields=["scope_code"], name="oel_cbe_ruleprofile_scope_code_uniq"), + models.CheckConstraint( + # Expressed as "at least two of the three scope columns are null", i.e. at most one + # is non-null. + condition=( + Q(organization__isnull=True, course__isnull=True) + | Q(organization__isnull=True, competency_taxonomy__isnull=True) + | Q(course__isnull=True, competency_taxonomy__isnull=True) + ), + name="oel_cbe_ruleprofile_scope_check", + violation_error_message=_( + "A CompetencyRuleProfile may be scoped to at most one of organization, course, and " + "competency_taxonomy." + ), + ), + ] + + @classmethod + def from_db(cls, db, field_names, values): + """Capture the scope this row had when loaded, so clean()/save() can detect an edit to it.""" + instance = super().from_db(db, field_names, values) + # field_names holds attnames (e.g. "organization_id"), not field names. Only capture when + # all three are present and unloaded (not deferred), so this never triggers extra queries. + scope_attnames = {"organization_id", "course_id", "competency_taxonomy_id"} + if scope_attnames.issubset(field_names): + instance.loaded_scope = ( + instance.organization_id, + instance.course_id, + instance.competency_taxonomy_id, + ) + return instance + + def _check_scope_immutable(self) -> None: + """Raise ValidationError if the scope columns no longer match what was loaded from the database.""" + loaded_scope = self.loaded_scope + if loaded_scope is None: + if self.pk is None: + # A new, unsaved instance: there's no persisted scope yet to compare against. + return + # from_db() didn't capture the scope, because this instance came from a deferred/ + # only() load that skipped one or more scope columns. Read the persisted scope back + # from the database directly, rather than silently skipping the check: a deferred + # load must not be a way to bypass immutability. This costs one extra query, but only + # on this rare path, which is already paying for extra field-loading queries anyway. + # Guarded against the row having since been deleted, in which case there's nothing + # left to compare against either. + row = ( + CompetencyRuleProfile.objects + .filter(pk=self.pk) + .values_list("organization_id", "course_id", "competency_taxonomy_id") + .first() + ) + if row is None: + return + loaded_scope = row + current_scope = (self.organization_id, self.course_id, self.competency_taxonomy_id) + if current_scope != loaded_scope: + raise ValidationError( + _( + "A CompetencyRuleProfile's scope (organization, course, competency_taxonomy) cannot be " + "changed after creation." + ) + ) + + def clean(self): + """Validate scope immutability and the rule_payload shape for rule_type.""" + super().clean() + self._check_scope_immutable() + validate_rule_payload(self.rule_type, self.rule_payload) + + def save(self, *args, **kwargs): + """Persist this profile, after re-checking scope immutability.""" + self._check_scope_immutable() + super().save(*args, **kwargs) + self.loaded_scope = (self.organization_id, self.course_id, self.competency_taxonomy_id) + + +class CompetencyCriterion(models.Model): + """ + A leaf node in a CompetencyAchievementCriteria tree: one tag/object association plus its rule. + + A null ``rule_profile`` does NOT mean "resolve the applicable profile at read time." ADR-0002 + Decision 4 resolves which profile (or override) applies at four specific write events + (creation, a more specific profile appearing later, an author setting a per-criterion + override, and an override being cleared back to matching the computed profile), and stores + the result. ``rule_profile`` is null only when an author has set a per-criterion override; in + every other case it holds the id of the profile that was resolved at the relevant write event + and is never re-resolved dynamically. Do not add a property, manager method, or other helper + that recomputes it; that would contradict the ADR. + + .. no_pii: + """ + + uuid = immutable_uuid_field() + group = models.ForeignKey( + CompetencyCriteriaGroup, + db_column="competency_criteria_group_id", + # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior + on_delete=models.PROTECT, + related_name="criteria", + help_text=_("The CompetencyCriteriaGroup this leaf criterion belongs to."), + ) + object_tag = models.ForeignKey( + ObjectTag, + db_column="oel_tagging_objecttag_id", + # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior + on_delete=models.PROTECT, + related_name="competency_criteria", + help_text=_("The tag/object association that this criterion evaluates."), + ) + rule_profile = models.ForeignKey( + CompetencyRuleProfile, + null=True, + blank=True, + db_column="competency_rule_profile_id", + # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior + on_delete=models.PROTECT, + related_name="criteria", + help_text=_("The profile this criterion uses by default. Null only when overrides are set instead."), + ) + rule_type_override = models.CharField(max_length=32, choices=RuleType, null=True, blank=True) + rule_payload_override = models.JSONField(null=True, blank=True) + + history = HistoricalRecords() + + class Meta: + db_table = "openedx_learning_competencycriteria" + # Django's default pluralization of "CompetencyCriterion" is the ungrammatical + # "competency criterions"; set both explicitly, matching ADR-0002 Decision 4's + # terminology (one leaf is a criterion, the collection is CompetencyCriteria) and + # following CompetencyTaxonomy, which sets both for the same reason. + verbose_name = _("Competency Criterion") + verbose_name_plural = _("Competency Criteria") + constraints = [ + models.CheckConstraint( + condition=( + Q( + rule_profile__isnull=False, + rule_type_override__isnull=True, + rule_payload_override__isnull=True, + ) + | Q( + rule_profile__isnull=True, + rule_type_override__isnull=False, + rule_payload_override__isnull=False, + ) + ), + name="oel_cbe_criterion_profile_xor_override_check", + violation_error_message=_( + "A CompetencyCriterion must have either a rule_profile with no overrides, or both override " + "fields set with no rule_profile. Never both, never neither." + ), + ), + ] + + def clean(self): + """Validate the override rule_payload's shape, when a per-criterion override is set.""" + super().clean() + if self.rule_type_override is not None: + validate_rule_payload(self.rule_type_override, self.rule_payload_override) diff --git a/src/openedx_learning/migrations/0002_competency_criteria.py b/src/openedx_learning/migrations/0002_competency_criteria.py new file mode 100644 index 000000000..2c521900c --- /dev/null +++ b/src/openedx_learning/migrations/0002_competency_criteria.py @@ -0,0 +1,165 @@ +# Generated by Django 5.2.16 on 2026-09-01 19:00 + +import uuid + +import django.db.models.deletion +import django.db.models.functions.comparison +import django.db.models.functions.text +import simple_history.models +from django.conf import settings +from django.db import migrations, models + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0021_remove_system_defined_add_read_only'), + ('openedx_catalog', '0001_initial'), + ('openedx_learning', '0001_initial'), + ('organizations', '0004_auto_20230727_2054'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.AddField( + model_name='competencytaxonomy', + name='taxonomy_overrides_org', + field=models.BooleanField(default=False, help_text="Resolves a tie when assigning a CompetencyRuleProfile to a CompetencyCriterion (ADR-0002 Decision 4): if both an organization-scoped profile and a taxonomy-scoped profile from this taxonomy apply to the same criterion, False (the default) assigns the organization-scoped profile, and True assigns this taxonomy's own profile instead, so it cannot be locally weakened by an organization. Nothing reads this field yet: organization-scoped CompetencyRuleProfile rows do not exist in this phase, so the tie it resolves cannot arise until they do."), + ), + migrations.CreateModel( + name='CompetencyCriteriaGroup', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), + ('name', openedx_django_lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, default='', max_length=255)), + ('ordering', models.PositiveIntegerField(default=0, help_text='Deterministic sibling evaluation sequence. Used to short-circuit evaluation and to order child scans during event-driven recomputation.')), + ('logic_operator', models.CharField(blank=True, choices=[('AND', 'And'), ('OR', 'Or')], help_text="How this group's children combine. Null until the group has children to combine.", max_length=3, null=True)), + ('course', models.ForeignKey(blank=True, help_text='The course run that scopes this criteria tree for evaluation windowing, if any.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='competency_criteria_groups', to='openedx_catalog.courserun')), + ('parent', models.ForeignKey(blank=True, help_text='The parent CompetencyCriteriaGroup. Null means this group is a tree root.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='child_groups', to='openedx_learning.competencycriteriagroup')), + ('tag', models.ForeignKey(db_column='oel_tagging_tag_id', help_text='The competency (tag) that this criteria tree evaluates mastery of.', on_delete=django.db.models.deletion.PROTECT, related_name='competency_criteria_groups', to='oel_tagging.tag')), + ], + ), + migrations.CreateModel( + name='CompetencyRuleProfile', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('scope_code', models.GeneratedField(db_persist=True, expression=django.db.models.functions.text.Concat(models.Value('org:'), django.db.models.functions.comparison.Coalesce(django.db.models.functions.comparison.Cast(models.F('organization_id'), output_field=models.CharField(max_length=20)), models.Value('')), models.Value(',course:'), django.db.models.functions.comparison.Coalesce(django.db.models.functions.comparison.Cast(models.F('course_id'), output_field=models.CharField(max_length=20)), models.Value('')), models.Value(',taxonomy:'), django.db.models.functions.comparison.Coalesce(django.db.models.functions.comparison.Cast(models.F('competency_taxonomy_id'), output_field=models.CharField(max_length=20)), models.Value(''))), output_field=models.CharField(max_length=255))), + ('rule_type', models.CharField(choices=[('View', 'View'), ('Grade', 'Grade'), ('MasteryLevel', 'Mastery Level')], max_length=32)), + ('rule_payload', models.JSONField(help_text='Structured payload keyed by rule_type; see validate_rule_payload for the shape it must match.')), + ('archived', models.BooleanField(default=False, help_text="Set instead of deleting a profile that's no longer wanted. Archived profiles are hidden from authoring and new associations but remain queryable, so existing criteria stay resolvable.")), + ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), + ('competency_taxonomy', models.ForeignKey(blank=True, help_text='The competency taxonomy this profile is scoped to, if any.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='rule_profiles', to='openedx_learning.competencytaxonomy')), + ('course', models.ForeignKey(blank=True, help_text='The course run this profile is scoped to, if any.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='competency_rule_profiles', to='openedx_catalog.courserun')), + ('organization', models.ForeignKey(blank=True, help_text='The organization this profile is scoped to, if any.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='competency_rule_profiles', to='organizations.organization')), + ], + ), + migrations.CreateModel( + name='CompetencyCriterion', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), + ('rule_type_override', models.CharField(blank=True, choices=[('View', 'View'), ('Grade', 'Grade'), ('MasteryLevel', 'Mastery Level')], max_length=32, null=True)), + ('rule_payload_override', models.JSONField(blank=True, null=True)), + ('group', models.ForeignKey(db_column='competency_criteria_group_id', help_text='The CompetencyCriteriaGroup this leaf criterion belongs to.', on_delete=django.db.models.deletion.PROTECT, related_name='criteria', to='openedx_learning.competencycriteriagroup')), + ('object_tag', models.ForeignKey(db_column='oel_tagging_objecttag_id', help_text='The tag/object association that this criterion evaluates.', on_delete=django.db.models.deletion.PROTECT, related_name='competency_criteria', to='oel_tagging.objecttag')), + ('rule_profile', models.ForeignKey(blank=True, db_column='competency_rule_profile_id', help_text='The profile this criterion uses by default. Null only when overrides are set instead.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='criteria', to='openedx_learning.competencyruleprofile')), + ], + options={ + 'db_table': 'openedx_learning_competencycriteria', + 'verbose_name': 'Competency Criterion', + 'verbose_name_plural': 'Competency Criteria', + }, + ), + migrations.CreateModel( + name='HistoricalCompetencyCriteriaGroup', + fields=[ + ('id', models.BigIntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('uuid', models.UUIDField(db_index=True, default=uuid.uuid4, editable=False, verbose_name='UUID')), + ('name', openedx_django_lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, default='', max_length=255)), + ('ordering', models.PositiveIntegerField(default=0, help_text='Deterministic sibling evaluation sequence. Used to short-circuit evaluation and to order child scans during event-driven recomputation.')), + ('logic_operator', models.CharField(blank=True, choices=[('AND', 'And'), ('OR', 'Or')], help_text="How this group's children combine. Null until the group has children to combine.", max_length=3, null=True)), + ('history_id', models.AutoField(primary_key=True, serialize=False)), + ('history_date', models.DateTimeField(db_index=True)), + ('history_change_reason', models.CharField(max_length=100, null=True)), + ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), + ('course', models.ForeignKey(blank=True, db_constraint=False, help_text='The course run that scopes this criteria tree for evaluation windowing, if any.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='openedx_catalog.courserun')), + ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), + ('parent', models.ForeignKey(blank=True, db_constraint=False, help_text='The parent CompetencyCriteriaGroup. Null means this group is a tree root.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='openedx_learning.competencycriteriagroup')), + ('tag', models.ForeignKey(blank=True, db_column='oel_tagging_tag_id', db_constraint=False, help_text='The competency (tag) that this criteria tree evaluates mastery of.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='oel_tagging.tag')), + ], + options={ + 'verbose_name': 'historical competency criteria group', + 'verbose_name_plural': 'historical competency criteria groups', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': ('history_date', 'history_id'), + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + migrations.CreateModel( + name='HistoricalCompetencyCriterion', + fields=[ + ('id', models.BigIntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('uuid', models.UUIDField(db_index=True, default=uuid.uuid4, editable=False, verbose_name='UUID')), + ('rule_type_override', models.CharField(blank=True, choices=[('View', 'View'), ('Grade', 'Grade'), ('MasteryLevel', 'Mastery Level')], max_length=32, null=True)), + ('rule_payload_override', models.JSONField(blank=True, null=True)), + ('history_id', models.AutoField(primary_key=True, serialize=False)), + ('history_date', models.DateTimeField(db_index=True)), + ('history_change_reason', models.CharField(max_length=100, null=True)), + ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), + ('group', models.ForeignKey(blank=True, db_column='competency_criteria_group_id', db_constraint=False, help_text='The CompetencyCriteriaGroup this leaf criterion belongs to.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='openedx_learning.competencycriteriagroup')), + ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), + ('object_tag', models.ForeignKey(blank=True, db_column='oel_tagging_objecttag_id', db_constraint=False, help_text='The tag/object association that this criterion evaluates.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='oel_tagging.objecttag')), + ('rule_profile', models.ForeignKey(blank=True, db_column='competency_rule_profile_id', db_constraint=False, help_text='The profile this criterion uses by default. Null only when overrides are set instead.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='openedx_learning.competencyruleprofile')), + ], + options={ + 'verbose_name': 'historical Competency Criterion', + 'verbose_name_plural': 'historical Competency Criteria', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': ('history_date', 'history_id'), + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + migrations.CreateModel( + name='HistoricalCompetencyRuleProfile', + fields=[ + ('id', models.BigIntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('rule_type', models.CharField(choices=[('View', 'View'), ('Grade', 'Grade'), ('MasteryLevel', 'Mastery Level')], max_length=32)), + ('rule_payload', models.JSONField(help_text='Structured payload keyed by rule_type; see validate_rule_payload for the shape it must match.')), + ('archived', models.BooleanField(default=False, help_text="Set instead of deleting a profile that's no longer wanted. Archived profiles are hidden from authoring and new associations but remain queryable, so existing criteria stay resolvable.")), + ('uuid', models.UUIDField(db_index=True, default=uuid.uuid4, editable=False, verbose_name='UUID')), + ('history_id', models.AutoField(primary_key=True, serialize=False)), + ('history_date', models.DateTimeField(db_index=True)), + ('history_change_reason', models.CharField(max_length=100, null=True)), + ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), + ('competency_taxonomy', models.ForeignKey(blank=True, db_constraint=False, help_text='The competency taxonomy this profile is scoped to, if any.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='openedx_learning.competencytaxonomy')), + ('course', models.ForeignKey(blank=True, db_constraint=False, help_text='The course run this profile is scoped to, if any.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='openedx_catalog.courserun')), + ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), + ('organization', models.ForeignKey(blank=True, db_constraint=False, help_text='The organization this profile is scoped to, if any.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='organizations.organization')), + ], + options={ + 'verbose_name': 'historical competency rule profile', + 'verbose_name_plural': 'historical competency rule profiles', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': ('history_date', 'history_id'), + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + migrations.AddIndex( + model_name='competencycriteriagroup', + index=models.Index(fields=['tag', 'course'], name='openedx_lea_oel_tag_737416_idx'), + ), + migrations.AddConstraint( + model_name='competencyruleprofile', + constraint=models.UniqueConstraint(fields=('scope_code',), name='oel_cbe_ruleprofile_scope_code_uniq'), + ), + migrations.AddConstraint( + model_name='competencyruleprofile', + constraint=models.CheckConstraint(condition=models.Q(models.Q(('course__isnull', True), ('organization__isnull', True)), models.Q(('competency_taxonomy__isnull', True), ('organization__isnull', True)), models.Q(('competency_taxonomy__isnull', True), ('course__isnull', True)), _connector='OR'), name='oel_cbe_ruleprofile_scope_check', violation_error_message='A CompetencyRuleProfile may be scoped to at most one of organization, course, and competency_taxonomy.'), + ), + migrations.AddConstraint( + model_name='competencycriterion', + constraint=models.CheckConstraint(condition=models.Q(models.Q(('rule_payload_override__isnull', True), ('rule_profile__isnull', False), ('rule_type_override__isnull', True)), models.Q(('rule_payload_override__isnull', False), ('rule_profile__isnull', True), ('rule_type_override__isnull', False)), _connector='OR'), name='oel_cbe_criterion_profile_xor_override_check', violation_error_message='A CompetencyCriterion must have either a rule_profile with no overrides, or both override fields set with no rule_profile. Never both, never neither.'), + ), + ] diff --git a/src/openedx_learning/migrations/0003_seed_default_rule_profile.py b/src/openedx_learning/migrations/0003_seed_default_rule_profile.py new file mode 100644 index 000000000..28374c49e --- /dev/null +++ b/src/openedx_learning/migrations/0003_seed_default_rule_profile.py @@ -0,0 +1,39 @@ +""" +Seed the system-default CompetencyRuleProfile: the one row where every scope column is null. + +Per ADR-0002 Decision 3, this is the rule every CompetencyCriterion falls back to when nothing +more specific applies, so a deployment that adds no profiles of its own still gets an 80% +threshold. +""" +from django.db import migrations + + +def seed_default_rule_profile(apps, schema_editor): + """Create the all-null-scope CompetencyRuleProfile.""" + CompetencyRuleProfile = apps.get_model('openedx_learning', 'CompetencyRuleProfile') + CompetencyRuleProfile.objects.create( + rule_type='Grade', + rule_payload={'op': 'gte', 'value': 0.8, 'scale': 'percent'}, + archived=False, + ) + + +def remove_default_rule_profile(apps, schema_editor): + """Delete the all-null-scope CompetencyRuleProfile, reversing seed_default_rule_profile.""" + CompetencyRuleProfile = apps.get_model('openedx_learning', 'CompetencyRuleProfile') + CompetencyRuleProfile.objects.filter( + organization__isnull=True, + course__isnull=True, + competency_taxonomy__isnull=True, + ).delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_learning', '0002_competency_criteria'), + ] + + operations = [ + migrations.RunPython(seed_default_rule_profile, remove_default_rule_profile), + ] diff --git a/tests/openedx_learning/applets/cbe/test_criteria_models.py b/tests/openedx_learning/applets/cbe/test_criteria_models.py new file mode 100644 index 000000000..eade547c4 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_models.py @@ -0,0 +1,402 @@ +""" +Tests for CompetencyCriteriaGroup, CompetencyRuleProfile, and CompetencyCriterion. +""" +import pytest +from django.apps import apps +from django.core.exceptions import ValidationError +from django.db import connection, models, transaction +from django.db.utils import IntegrityError +from organizations.api import ensure_organization +from organizations.models import Organization + +from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + LogicOperator, + RuleType, +) +from openedx_tagging.models import ObjectTag, Tag + +pytestmark = pytest.mark.django_db + +_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} + +# One (rule_type, payload) pair per way ADR-0002 Decision 3 says a rule_payload can be invalid. +_INVALID_GRADE_PAYLOADS = [ + pytest.param(RuleType.GRADE, {"op": "startswith", "value": 0.8, "scale": "percent"}, id="bad_op"), + pytest.param(RuleType.GRADE, {"op": "gte", "value": 80, "scale": "percent"}, id="value_80_not_0_8"), + pytest.param(RuleType.GRADE, {"op": "gte", "value": 1.5, "scale": "percent"}, id="value_out_of_range"), + pytest.param(RuleType.GRADE, {"op": "gte", "scale": "percent"}, id="missing_key"), + pytest.param(RuleType.GRADE, {**_GRADE_PAYLOAD, "extra": 1}, id="extra_key"), + pytest.param(RuleType.GRADE, ["not", "a", "dict"], id="non_dict"), + pytest.param(RuleType.GRADE, {"op": "gte", "value": 0.8, "scale": "raw"}, id="wrong_scale"), + pytest.param(RuleType.GRADE, {"op": "gte", "value": True, "scale": "percent"}, id="boolean_value"), + pytest.param(RuleType.VIEW, _GRADE_PAYLOAD, id="unsupported_rule_type"), +] + + +@pytest.fixture(name="organization") +def _organization() -> Organization: + """An Organization for use as a scope in these tests.""" + ensure_organization("Org1") + return Organization.objects.get(short_name="Org1") + + +@pytest.fixture(name="organization2") +def _organization2() -> Organization: + """A second Organization, distinct from `organization`, for use as a scope in these tests.""" + ensure_organization("Org2") + return Organization.objects.get(short_name="Org2") + + +@pytest.fixture(name="course_run") +def _course_run(organization: Organization) -> CourseRun: + """A CourseRun for use as a scope in these tests.""" + catalog_course = CatalogCourse.objects.create(org=organization, course_code="Python100") + return CourseRun.objects.create(catalog_course=catalog_course, run_code="Fall2026") + + +@pytest.fixture(name="competency_taxonomy") +def _competency_taxonomy() -> CompetencyTaxonomy: + """A CompetencyTaxonomy for use as a scope, and as the home taxonomy for `tag`.""" + return CompetencyTaxonomy.objects.create(name="Nursing", export_id="nursing-v1") + + +@pytest.fixture(name="tag") +def _tag(competency_taxonomy: CompetencyTaxonomy) -> Tag: + """A Tag, from `competency_taxonomy`, for use as the competency a criteria tree evaluates.""" + return Tag.objects.create(taxonomy=competency_taxonomy, value="Writing Poetry") + + +@pytest.fixture(name="object_tag") +def _object_tag(competency_taxonomy: CompetencyTaxonomy, tag: Tag) -> ObjectTag: + """An ObjectTag associating `tag` with a made-up content object, for use as a criterion's target.""" + return ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+p1", + taxonomy=competency_taxonomy, + tag=tag, + ) + + +@pytest.fixture(name="group") +def _group(tag: Tag) -> CompetencyCriteriaGroup: + """A root CompetencyCriteriaGroup for `tag`, for use as a criterion's parent group.""" + return CompetencyCriteriaGroup.objects.create(tag=tag) + + +@pytest.fixture(name="default_rule_profile") +def _default_rule_profile() -> CompetencyRuleProfile: + """The system-default CompetencyRuleProfile seeded by migration 0003.""" + return CompetencyRuleProfile.objects.get( + organization__isnull=True, + course__isnull=True, + competency_taxonomy__isnull=True, + ) + + +def test_group_tree_and_logic_operator(tag: Tag) -> None: + """ + A CompetencyCriteriaGroup's parent is null for a root and points at its parent for a child, + and logic_operator accepts AND, OR, or null (the "no children yet" state). See ADR-0002 + Decision 2. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=None) + assert root.parent is None + + child_and = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, logic_operator=LogicOperator.AND) + assert child_and.parent == root + + child_or = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, logic_operator=LogicOperator.OR) + assert child_or.parent == root + + +def test_rule_profile_scope_check_constraint( + organization: Organization, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy +) -> None: + """ + The scope check constraint accepts a CompetencyRuleProfile scoped to at most one of + organization, course, or competency_taxonomy (including none of them), and rejects one scoped + to any two, or to all three. See ADR-0002 Decision 3. + """ + # Free the all-null slot the seed migration (0003) occupies, so the "all null" case below can + # be tested in isolation from the uniqueness constraint on scope_code, which is a separate + # constraint covered by its own tests. + CompetencyRuleProfile.objects.filter( + organization__isnull=True, course__isnull=True, competency_taxonomy__isnull=True + ).delete() + + accepted_scopes: list[dict] = [ + {"organization": organization}, + {"course": course_run}, + {"competency_taxonomy": competency_taxonomy}, + {}, + ] + for scope_kwargs in accepted_scopes: + with transaction.atomic(): + CompetencyRuleProfile.objects.create(rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD, **scope_kwargs) + + rejected_scopes: list[dict] = [ + {"organization": organization, "course": course_run}, + {"organization": organization, "competency_taxonomy": competency_taxonomy}, + {"course": course_run, "competency_taxonomy": competency_taxonomy}, + {"organization": organization, "course": course_run, "competency_taxonomy": competency_taxonomy}, + ] + for scope_kwargs in rejected_scopes: + with pytest.raises(IntegrityError): + with transaction.atomic(): + CompetencyRuleProfile.objects.create( + rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD, **scope_kwargs + ) + + +def test_scope_code_generated_value( + organization: Organization, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy +) -> None: + """ + scope_code is derived from the three scope columns as "org:X,course:Y,taxonomy:Z", with each + segment blank when the corresponding column is null. See ADR-0002 Decision 3. + """ + CompetencyRuleProfile.objects.filter( + organization__isnull=True, course__isnull=True, competency_taxonomy__isnull=True + ).delete() + + all_null = CompetencyRuleProfile.objects.create(rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD) + org_only = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + course_only = CompetencyRuleProfile.objects.create( + course=course_run, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + taxonomy_only = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + for profile in (all_null, org_only, course_only, taxonomy_only): + profile.refresh_from_db() + + assert all_null.scope_code == "org:,course:,taxonomy:" + assert org_only.scope_code == f"org:{organization.pk},course:,taxonomy:" + assert course_only.scope_code == f"org:,course:{course_run.pk},taxonomy:" + assert taxonomy_only.scope_code == f"org:,course:,taxonomy:{competency_taxonomy.pk}" + + +def test_scope_code_uniqueness(organization: Organization) -> None: + """ + Two CompetencyRuleProfile rows cannot share the same scope. In particular, two rows that both + set only `organization` (leaving course and competency_taxonomy null) collide, which is + exactly the case a plain UniqueConstraint on the three raw nullable columns would not catch, + since SQL never treats two NULLs as equal. See ADR-0002 Decision 3. + """ + CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + with pytest.raises(IntegrityError): + with transaction.atomic(): + CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + +def test_scope_code_unique_constraint_has_no_condition() -> None: + """ + No UniqueConstraint on CompetencyRuleProfile carries a `condition`. A conditional + UniqueConstraint compiles to a partial index, which this project's MySQL backend does not + support: Django would only raise a non-fatal system-check warning (models.W036) and silently + skip creating the constraint, leaving uniqueness unenforced in production, while SQLite (used + for local test runs) supports partial indexes and would mask the gap. See ADR-0002 Rejected + Alternative 6. + """ + unique_constraints = [c for c in CompetencyRuleProfile._meta.constraints if isinstance(c, models.UniqueConstraint)] + assert unique_constraints + for constraint in unique_constraints: + assert constraint.condition is None + + +def test_criterion_profile_xor_override_constraint( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + A CompetencyCriterion must have either a rule_profile with no overrides, or both override + fields set with no rule_profile, never both and never neither. See ADR-0002 Decision 4. + """ + CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=default_rule_profile) + CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_type_override=RuleType.GRADE, rule_payload_override=_GRADE_PAYLOAD + ) + + invalid_kwargs_list: list[dict] = [ + { # both set + "rule_profile": default_rule_profile, + "rule_type_override": RuleType.GRADE, + "rule_payload_override": _GRADE_PAYLOAD, + }, + {}, # neither set + {"rule_type_override": RuleType.GRADE}, # only the type override set + {"rule_payload_override": _GRADE_PAYLOAD}, # only the payload override set + ] + for kwargs in invalid_kwargs_list: + with pytest.raises(IntegrityError): + with transaction.atomic(): + CompetencyCriterion.objects.create(group=group, object_tag=object_tag, **kwargs) + + +@pytest.mark.parametrize("rule_type, payload", _INVALID_GRADE_PAYLOADS) +def test_rule_profile_full_clean_rejects_invalid_payload(rule_type: str, payload: object) -> None: + """ + full_clean() raises ValidationError for a CompetencyRuleProfile on every documented way a + rule_payload can be invalid: a bad op, a value given on a 0-100 scale instead of 0.0-1.0, a + value outside that range, a missing or extra key, a non-dict payload, a wrong scale, a + boolean value, and a rule_type with no defined payload shape yet. See ADR-0002 Decision 3. + """ + profile = CompetencyRuleProfile(rule_type=rule_type, rule_payload=payload) + with pytest.raises(ValidationError): + profile.full_clean() + + +@pytest.mark.parametrize("rule_type, payload", _INVALID_GRADE_PAYLOADS) +def test_criterion_full_clean_rejects_invalid_override_payload( + rule_type: str, payload: object, group: CompetencyCriteriaGroup, object_tag: ObjectTag +) -> None: + """ + full_clean() raises ValidationError for a CompetencyCriterion's rule_payload_override on the + same invalid shapes as CompetencyRuleProfile.rule_payload. See ADR-0002 Decision 3. + """ + criterion = CompetencyCriterion( + group=group, object_tag=object_tag, rule_type_override=rule_type, rule_payload_override=payload + ) + with pytest.raises(ValidationError): + criterion.full_clean() + + +def test_history_recorded_for_new_models_but_not_taxonomy( + organization: Organization, + group: CompetencyCriteriaGroup, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, + competency_taxonomy: CompetencyTaxonomy, +) -> None: + """ + HistoricalRecords() is applied to CompetencyCriteriaGroup, CompetencyRuleProfile, and + CompetencyCriterion: each is registered in the app registry under its expected + Historical* name, and editing an instance writes a row there. CompetencyTaxonomy has no + history at all. See ADR-0003 Decisions 1 and 2. + + Historical* models are looked up via the app registry rather than the `.history` attribute + because simple_history installs `.history` as a runtime descriptor with no type stubs, which + mypy cannot type; apps.get_model() returns something mypy can call `.objects` on. + """ + historical_group = apps.get_model("openedx_learning", "HistoricalCompetencyCriteriaGroup") + historical_profile = apps.get_model("openedx_learning", "HistoricalCompetencyRuleProfile") + historical_criterion = apps.get_model("openedx_learning", "HistoricalCompetencyCriterion") + + group.name = "Poetry Mastery" + group.save() + assert historical_group.objects.filter(id=group.pk).count() == 2 + + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.rule_payload = {"op": "gte", "value": 0.9, "scale": "percent"} + profile.save() + assert historical_profile.objects.filter(id=profile.pk).count() == 2 + + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + criterion.rule_profile = None + criterion.rule_type_override = RuleType.GRADE + criterion.rule_payload_override = _GRADE_PAYLOAD + criterion.save() + assert historical_criterion.objects.filter(id=criterion.pk).count() == 2 + + assert not hasattr(competency_taxonomy, "history") + + +def test_scope_immutability(organization: Organization, course_run: CourseRun) -> None: + """ + Changing a CompetencyRuleProfile's scope (organization, course, or competency_taxonomy) after + creation raises ValidationError on save(). Criteria store the profile id they were assigned + and never re-resolve it, so letting the scope change would silently re-govern every criterion + already pointing at this profile. This guard catches instance.save() but not a bulk + QuerySet.update(). See ADR-0002 Decision 3. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.organization = None + profile.course = course_run + with pytest.raises(ValidationError): + profile.save() + + +def test_scope_immutability_with_deferred_load(organization: Organization, organization2: Organization) -> None: + """ + Scope immutability is enforced even when the profile was loaded with .only()/.defer() and so + never had a complete `loaded_scope` captured by from_db(). Without falling back to read the + persisted scope back from the database, this edit would go through unchecked, because + _check_scope_immutable() would find `loaded_scope` still None and skip the comparison + entirely. + + Uses a second organization rather than setting the scope to None: setting it to None would + make scope_code collide with the seeded system-default row, so the unique constraint would + raise IntegrityError instead of the scope guard, and the test would pass for the wrong + reason. Do not "simplify" this back to None. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + deferred = CompetencyRuleProfile.objects.only("id", "rule_type").get(pk=profile.pk) + assert deferred.loaded_scope is None + + deferred.organization = organization2 + with pytest.raises(ValidationError): + deferred.save() + + +def test_adr_indexes_present() -> None: + """ + The real database tables carry the ADR-0002 Decision 5 indexes this migration is responsible + for: positions 1, 2, 4, 5 (all covering indexes), and 9 (unique). Positions 2, 4, and 5 come + from Django's automatic per-ForeignKey index rather than an explicit models.Index; this test + introspects the database, not the model, so it holds regardless of which mechanism produced + the index. Positions 3, 6, 7, 8, and 10 belong to tables this migration doesn't create. + """ + with connection.cursor() as cursor: + group_constraints = connection.introspection.get_constraints(cursor, CompetencyCriteriaGroup._meta.db_table) + criterion_constraints = connection.introspection.get_constraints(cursor, CompetencyCriterion._meta.db_table) + profile_constraints = connection.introspection.get_constraints(cursor, CompetencyRuleProfile._meta.db_table) + + def is_indexed(constraints: dict, columns: list[str]) -> bool: + # Compare the ordered column list, not a set: column order is the whole point of a + # composite index. An index on (course_id, oel_tagging_tag_id) would satisfy a set + # comparison against ADR index 1 just as well as (oel_tagging_tag_id, course_id), but + # only the tag-first ordering also serves tag-only lookups. + return any(c["columns"] == columns and c["index"] for c in constraints.values()) + + # 1: CompetencyCriteriaGroup(tag, course), the one explicit composite index. + assert is_indexed(group_constraints, ["oel_tagging_tag_id", "course_id"]) + # 2: CompetencyCriteriaGroup(parent). + assert is_indexed(group_constraints, ["parent_id"]) + # 4: CompetencyCriteria(object_tag). + assert is_indexed(criterion_constraints, ["oel_tagging_objecttag_id"]) + # 5: CompetencyCriteria(group). + assert is_indexed(criterion_constraints, ["competency_criteria_group_id"]) + # 9: CompetencyRuleProfile(scope_code), unique. + assert any( + set(c["columns"]) == {"scope_code"} and c["unique"] for c in profile_constraints.values() + ) + + +def test_seeded_default_rule_profile_exists() -> None: + """ + Migration 0003 seeds exactly one system-default CompetencyRuleProfile: all three scope + columns null, not archived, Grade >= 0.8 (80%). See ADR-0002 Decision 3. + """ + profile = CompetencyRuleProfile.objects.get( + organization__isnull=True, course__isnull=True, competency_taxonomy__isnull=True + ) + assert profile.archived is False + assert profile.rule_type == RuleType.GRADE + assert profile.rule_payload == _GRADE_PAYLOAD diff --git a/tests/openedx_learning/applets/cbe/test_models.py b/tests/openedx_learning/applets/cbe/test_models.py index b38c25928..7b113cb8a 100644 --- a/tests/openedx_learning/applets/cbe/test_models.py +++ b/tests/openedx_learning/applets/cbe/test_models.py @@ -44,6 +44,14 @@ def test_plain_taxonomy_has_no_competencytaxonomy() -> None: _ = plain.competencytaxonomy +def test_taxonomy_overrides_org_defaults_false(competency_taxonomy: CompetencyTaxonomy) -> None: + """ + taxonomy_overrides_org defaults to False, so an organization-scoped profile wins the + contested case by default until an author opts a taxonomy out. See ADR-0002 Decision 1. + """ + assert competency_taxonomy.taxonomy_overrides_org is False + + def test_delete_cascades_both_directions() -> None: """ Deleting the parent Taxonomy removes the CompetencyTaxonomy row, and deleting From 89641f4c4aac303899d6b1a962ec29a9034979d6 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 2 Sep 2026 10:45:40 -0400 Subject: [PATCH 2/5] docs: drop superseded #799 references from criteria models #655 closed with an approved design and #799 is now closed as superseded, so both halves of the nine repeated TODO comments were false: #799 does not own the on_delete question, and no follow-up ticket will set "the real" per-foreign-key values. Replaces those nine identical comments with one explanation in the module docstring, which also records the open question #655's design creates for CompetencyCriteriaGroup.tag and CompetencyCriterion.object_tag: that design keeps openedx_tagging ignorant of CBE and promises a plain hard delete for a tag no learner holds mastery against, which PROTECT turns into a ProtectedError whenever an author's criteria tree references the tag and nobody has been graded yet. The PROTECT values themselves are unchanged. They remain the fail-closed default until #655's reviewers settle the question. Refs #641, #655 Co-Authored-By: Claude Opus 5 (1M context) --- .../applets/cbe/models/criteria.py | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py index d008bfa93..7a9ce8044 100644 --- a/src/openedx_learning/applets/cbe/models/criteria.py +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -4,6 +4,24 @@ See :ref:`openedx-learning-adr-0002` for the design this module implements, and :ref:`openedx-learning-adr-0003` for why these three models (and not CompetencyTaxonomy) carry ``django-simple-history`` tracking. + +Every foreign key declared in this module uses ``on_delete=models.PROTECT``. This is the current +fail-closed default, not a settled decision: which delete behavior each of these foreign keys +should actually carry is an open question escalated against the approved design in #655, and no +ticket currently owns revisiting it. (#799 used to hold this question; it is now closed as +superseded.) + +Two of these foreign keys cross a real boundary and are the most likely to change: +``CompetencyCriteriaGroup.tag`` and ``CompetencyCriterion.object_tag``, both pointing into +``openedx_tagging``. #655's approved design deliberately keeps ``openedx_tagging`` ignorant that +CBE exists, so its archive-versus-delete branch reads only its own ``deletion_locked`` flag and +never calls into CBE to check for referencing criteria. #655 also says that deleting a tag no +learner holds mastery against should be a plain hard delete. ``PROTECT`` breaks that promise: it +turns the hard delete into a ``ProtectedError`` whenever an author's criteria tree references the +tag, which is the ordinary state at authoring time, before any learner has been graded. Whether +these two foreign keys stay ``PROTECT`` (with the tagging-side delete paths learning to clear +referencing rows first) or become ``CASCADE`` is not decided; that decision belongs to #655, not +to this module. """ from __future__ import annotations @@ -109,7 +127,6 @@ class CompetencyCriteriaGroup(models.Model): "self", null=True, blank=True, - # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior on_delete=models.PROTECT, related_name="child_groups", help_text=_("The parent CompetencyCriteriaGroup. Null means this group is a tree root."), @@ -117,7 +134,6 @@ class CompetencyCriteriaGroup(models.Model): tag = models.ForeignKey( Tag, db_column="oel_tagging_tag_id", - # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior on_delete=models.PROTECT, related_name="competency_criteria_groups", help_text=_("The competency (tag) that this criteria tree evaluates mastery of."), @@ -126,7 +142,6 @@ class CompetencyCriteriaGroup(models.Model): CourseRun, null=True, blank=True, - # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior on_delete=models.PROTECT, related_name="competency_criteria_groups", help_text=_("The course run that scopes this criteria tree for evaluation windowing, if any."), @@ -202,7 +217,6 @@ class CompetencyRuleProfile(models.Model): Organization, null=True, blank=True, - # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior on_delete=models.PROTECT, related_name="competency_rule_profiles", help_text=_("The organization this profile is scoped to, if any."), @@ -211,7 +225,6 @@ class CompetencyRuleProfile(models.Model): CourseRun, null=True, blank=True, - # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior on_delete=models.PROTECT, related_name="competency_rule_profiles", help_text=_("The course run this profile is scoped to, if any."), @@ -220,7 +233,6 @@ class CompetencyRuleProfile(models.Model): CompetencyTaxonomy, null=True, blank=True, - # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior on_delete=models.PROTECT, related_name="rule_profiles", help_text=_("The competency taxonomy this profile is scoped to, if any."), @@ -364,7 +376,6 @@ class CompetencyCriterion(models.Model): group = models.ForeignKey( CompetencyCriteriaGroup, db_column="competency_criteria_group_id", - # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior on_delete=models.PROTECT, related_name="criteria", help_text=_("The CompetencyCriteriaGroup this leaf criterion belongs to."), @@ -372,7 +383,6 @@ class CompetencyCriterion(models.Model): object_tag = models.ForeignKey( ObjectTag, db_column="oel_tagging_objecttag_id", - # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior on_delete=models.PROTECT, related_name="competency_criteria", help_text=_("The tag/object association that this criterion evaluates."), @@ -382,7 +392,6 @@ class CompetencyCriterion(models.Model): null=True, blank=True, db_column="competency_rule_profile_id", - # TODO(#799): provisional fail-closed value; #799 sets the real per-FK behavior on_delete=models.PROTECT, related_name="criteria", help_text=_("The profile this criterion uses by default. Null only when overrides are set instead."), From fd97d0d1daea097636035b7a23f37c33fbb6b3a6 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 2 Sep 2026 10:45:40 -0400 Subject: [PATCH 3/5] test: cover on_delete behavior for every CBE criteria foreign key #641 requires at least one test per foreign key asserting that deleting the referenced row matches what the field declares. All nine are PROTECT, so all nine assert ProtectedError, and each inspects ProtectedError.protected_objects rather than only the exception type: a single delete can trip several protected relationships, so a bare pytest.raises would not prove which foreign key did the protecting. Two cases needed isolating to avoid passing for the wrong reason. CatalogCourse.org is itself PROTECT, so the organization test uses an organization with no catalog course attached. Tag.taxonomy is CASCADE, so the competency_taxonomy test omits the tag and group fixtures. A tenth test pins the open #655 question in executable form: deleting a CompetencyTaxonomy whose tag carries a criteria tree raises ProtectedError today, though that design promises the delete succeeds when no learner status exists. It is the test that has to change if the reviewers move CompetencyCriteriaGroup.tag to CASCADE, and says so. Refs #641 Co-Authored-By: Claude Opus 5 (1M context) --- .../applets/cbe/test_criteria_deletion.py | 261 ++++++++++++++++++ 1 file changed, 261 insertions(+) create mode 100644 tests/openedx_learning/applets/cbe/test_criteria_deletion.py diff --git a/tests/openedx_learning/applets/cbe/test_criteria_deletion.py b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py new file mode 100644 index 000000000..ad4e62466 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py @@ -0,0 +1,261 @@ +""" +Delete-behavior tests for CompetencyCriteriaGroup, CompetencyRuleProfile, and CompetencyCriterion. + +Every foreign key these three models declare is currently `on_delete=models.PROTECT`; see the +module docstring in `openedx_learning.applets.cbe.models.criteria` for why that is the current +fail-closed value rather than a settled one, and what is still open about it on #655. +""" +import pytest +from django.db.models import ProtectedError +from organizations.api import ensure_organization +from organizations.models import Organization + +from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + RuleType, +) +from openedx_tagging.models import ObjectTag, Tag + +pytestmark = pytest.mark.django_db + +_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} + + +@pytest.fixture(name="organization") +def _organization() -> Organization: + """An Organization for use as a scope in these tests.""" + ensure_organization("Org1") + return Organization.objects.get(short_name="Org1") + + +@pytest.fixture(name="organization2") +def _organization2() -> Organization: + """A second Organization, distinct from `organization`, for use as a scope in these tests.""" + ensure_organization("Org2") + return Organization.objects.get(short_name="Org2") + + +@pytest.fixture(name="course_run") +def _course_run(organization: Organization) -> CourseRun: + """A CourseRun for use as a scope in these tests.""" + catalog_course = CatalogCourse.objects.create(org=organization, course_code="Python100") + return CourseRun.objects.create(catalog_course=catalog_course, run_code="Fall2026") + + +@pytest.fixture(name="competency_taxonomy") +def _competency_taxonomy() -> CompetencyTaxonomy: + """A CompetencyTaxonomy for use as a scope, and as the home taxonomy for `tag`.""" + return CompetencyTaxonomy.objects.create(name="Nursing", export_id="nursing-v1") + + +@pytest.fixture(name="tag") +def _tag(competency_taxonomy: CompetencyTaxonomy) -> Tag: + """A Tag, from `competency_taxonomy`, for use as the competency a criteria tree evaluates.""" + return Tag.objects.create(taxonomy=competency_taxonomy, value="Writing Poetry") + + +@pytest.fixture(name="object_tag") +def _object_tag(competency_taxonomy: CompetencyTaxonomy, tag: Tag) -> ObjectTag: + """An ObjectTag associating `tag` with a made-up content object, for use as a criterion's target.""" + return ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+p1", + taxonomy=competency_taxonomy, + tag=tag, + ) + + +@pytest.fixture(name="group") +def _group(tag: Tag) -> CompetencyCriteriaGroup: + """A root CompetencyCriteriaGroup for `tag`, for use as a criterion's parent group.""" + return CompetencyCriteriaGroup.objects.create(tag=tag) + + +@pytest.fixture(name="default_rule_profile") +def _default_rule_profile() -> CompetencyRuleProfile: + """The system-default CompetencyRuleProfile seeded by migration 0003.""" + return CompetencyRuleProfile.objects.get( + organization__isnull=True, + course__isnull=True, + competency_taxonomy__isnull=True, + ) + + +# ============================================================================================== +# One test per foreign key, all nine currently PROTECT. Each asserts on the raised +# ProtectedError's `protected_objects`, not just its type: several protected relationships can +# fire on one delete (see test_rule_profile_organization_protect and +# test_rule_profile_competency_taxonomy_protect below for two real traps of that kind), so a bare +# `pytest.raises(ProtectedError)` would not actually prove which foreign key did the protecting. +# ============================================================================================== + + +def test_group_parent_protect(tag: Tag) -> None: + """ + Deleting a CompetencyCriteriaGroup that another group's `parent` points at raises + ProtectedError. Django's PROTECT raises even though the referencing child group is not part + of this delete call; nothing about `parent` being a self-referential, tree-shaped + relationship exempts it from that. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + + with pytest.raises(ProtectedError) as exc_info: + root.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyCriteriaGroup) and obj.pk == child.pk for obj in protected) + + +def test_group_tag_protect(tag: Tag, group: CompetencyCriteriaGroup) -> None: + """Deleting a Tag that a CompetencyCriteriaGroup references via `tag` raises ProtectedError.""" + with pytest.raises(ProtectedError) as exc_info: + tag.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyCriteriaGroup) and obj.pk == group.pk for obj in protected) + + +def test_group_course_protect(tag: Tag, course_run: CourseRun) -> None: + """Deleting a CourseRun that a CompetencyCriteriaGroup references via `course` raises ProtectedError.""" + group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run) + + with pytest.raises(ProtectedError) as exc_info: + course_run.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyCriteriaGroup) and obj.pk == group.pk for obj in protected) + + +def test_rule_profile_organization_protect(organization2: Organization) -> None: + """ + Deleting an Organization that a CompetencyRuleProfile references via `organization` raises + ProtectedError naming the profile. + + Uses `organization2`, which this test never attaches a CatalogCourse to, instead of + `organization` (the one `course_run` uses elsewhere in this module): CatalogCourse.org is + itself PROTECT, so deleting an organization with a CatalogCourse attached raises + ProtectedError regardless of whether a CompetencyRuleProfile references it too, and this + test would pass for the wrong reason. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization2, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + with pytest.raises(ProtectedError) as exc_info: + organization2.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyRuleProfile) and obj.pk == profile.pk for obj in protected) + + +def test_rule_profile_course_protect(course_run: CourseRun) -> None: + """Deleting a CourseRun that a CompetencyRuleProfile references via `course` raises ProtectedError.""" + profile = CompetencyRuleProfile.objects.create( + course=course_run, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + with pytest.raises(ProtectedError) as exc_info: + course_run.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyRuleProfile) and obj.pk == profile.pk for obj in protected) + + +def test_rule_profile_competency_taxonomy_protect(competency_taxonomy: CompetencyTaxonomy) -> None: + """ + Deleting a CompetencyTaxonomy that a CompetencyRuleProfile references via + `competency_taxonomy` raises ProtectedError naming the profile. + + Deliberately does not use the `tag` or `group` fixtures: a Tag under this taxonomy would be + collected by Tag.taxonomy's CASCADE, and a CompetencyCriteriaGroup referencing that tag would + then hit its own `tag` PROTECT (see test_taxonomy_delete_blocked_by_group_tag_protection + below), which would raise ProtectedError without this test having exercised + CompetencyRuleProfile.competency_taxonomy at all. + """ + profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + with pytest.raises(ProtectedError) as exc_info: + competency_taxonomy.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyRuleProfile) and obj.pk == profile.pk for obj in protected) + + +def test_criterion_group_protect( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting a CompetencyCriteriaGroup that a CompetencyCriterion references via `group` raises + ProtectedError, even though the criterion is not itself part of this delete call. + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + + with pytest.raises(ProtectedError) as exc_info: + group.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in protected) + + +def test_criterion_object_tag_protect( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """Deleting an ObjectTag that a CompetencyCriterion references via `object_tag` raises ProtectedError.""" + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + + with pytest.raises(ProtectedError) as exc_info: + object_tag.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in protected) + + +def test_criterion_rule_profile_protect( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting a CompetencyRuleProfile that a CompetencyCriterion references via `rule_profile` + raises ProtectedError. + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + + with pytest.raises(ProtectedError) as exc_info: + default_rule_profile.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in protected) + + +def test_taxonomy_delete_blocked_by_group_tag_protection( + competency_taxonomy: CompetencyTaxonomy, group: CompetencyCriteriaGroup +) -> None: + """ + #655's approved design promises that deleting a CompetencyTaxonomy whose tag no learner holds + mastery against succeeds as a plain hard delete: Tag.taxonomy is CASCADE, so the tag is + collected along with the taxonomy. `PROTECT` on CompetencyCriteriaGroup.tag currently breaks + that promise instead: the delete collects `tag` via CASCADE, then `group`'s reference to that + tag hits PROTECT and the whole delete is refused, even though no learner has been graded + against it (there is no learner-status table yet at all). + + This conflict is open on #655 (see the module docstring in + openedx_learning.applets.cbe.models.criteria) and unresolved as of this writing. Whichever + way it resolves, this test is the one that has to change: if CompetencyCriteriaGroup.tag + moves to CASCADE, this becomes an assertion that the delete succeeds instead of raising. + """ + with pytest.raises(ProtectedError) as exc_info: + competency_taxonomy.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyCriteriaGroup) and obj.pk == group.pk for obj in protected) From 815717f21cead7e43020087162627324cbdfa169 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 2 Sep 2026 15:34:15 -0400 Subject: [PATCH 4/5] feat: cascade criteria deletes from tag, group and objecttag #655 decided the on_delete question on 2026-09-02, so the four foreign keys between definition tables become CASCADE: CompetencyCriteriaGroup.parent, CompetencyCriteriaGroup.tag, CompetencyCriterion.group and CompetencyCriterion.object_tag. The other five stay PROTECT and are now final. Deleting a Tag nobody holds mastery against has to succeed, and #655's design forbids openedx_tagging from knowing CBE exists, so the tagging-side path cannot clear the criteria tree first. CASCADE lets the delete take the tree with it. parent and group need it too, because Django's collector looks up referencing rows in the database rather than in the set it has already collected, so a parent and child reached in one batch would trip PROTECT and abort the walk partway down. This does not weaken ADR-0002 Decision 7. The four CASCADE links are what carries the collector down to the PROTECT that enforces it, on #642's Student*Status foreign keys one and two levels below the tag, which Django reaches only by walking CASCADE edges. Migration 0002 is edited in place rather than gaining an AlterField, since it is unmerged. The delete tests are reworked accordingly and extended with the transitive cases: a tag delete cascading a whole tree, a group delete at depth taking its descendants and their criteria, and a taxonomy delete reaching through tag to group to criterion. The matching "raises ProtectedError when a learner status exists" halves need #642's tables and belong to that slice, which a comment in the test file records. One cascade test also asserts django-simple-history writes a history_type='-' row per removed row, so the cascade is not silent for audit. Refs #641, #655 Co-Authored-By: Claude Opus 5 (1M context) --- .../applets/cbe/models/criteria.py | 54 +++-- .../migrations/0002_competency_criteria.py | 8 +- .../applets/cbe/test_criteria_deletion.py | 206 +++++++++++++----- 3 files changed, 186 insertions(+), 82 deletions(-) diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py index 7a9ce8044..265ec4431 100644 --- a/src/openedx_learning/applets/cbe/models/criteria.py +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -5,23 +5,35 @@ :ref:`openedx-learning-adr-0003` for why these three models (and not CompetencyTaxonomy) carry ``django-simple-history`` tracking. -Every foreign key declared in this module uses ``on_delete=models.PROTECT``. This is the current -fail-closed default, not a settled decision: which delete behavior each of these foreign keys -should actually carry is an open question escalated against the approved design in #655, and no -ticket currently owns revisiting it. (#799 used to hold this question; it is now closed as -superseded.) - -Two of these foreign keys cross a real boundary and are the most likely to change: -``CompetencyCriteriaGroup.tag`` and ``CompetencyCriterion.object_tag``, both pointing into -``openedx_tagging``. #655's approved design deliberately keeps ``openedx_tagging`` ignorant that -CBE exists, so its archive-versus-delete branch reads only its own ``deletion_locked`` flag and -never calls into CBE to check for referencing criteria. #655 also says that deleting a tag no -learner holds mastery against should be a plain hard delete. ``PROTECT`` breaks that promise: it -turns the hard delete into a ``ProtectedError`` whenever an author's criteria tree references the -tag, which is the ordinary state at authoring time, before any learner has been graded. Whether -these two foreign keys stay ``PROTECT`` (with the tagging-side delete paths learning to clear -referencing rows first) or become ``CASCADE`` is not decided; that decision belongs to #655, not -to this module. +Four of the nine foreign keys here are ``on_delete=models.CASCADE``: ``CompetencyCriteriaGroup.parent``, +``CompetencyCriteriaGroup.tag``, ``CompetencyCriterion.group``, and ``CompetencyCriterion.object_tag``. +The other five stay ``models.PROTECT``: ``CompetencyCriterion.rule_profile``, +``CompetencyCriteriaGroup.course``, ``CompetencyRuleProfile.course``, +``CompetencyRuleProfile.organization``, and ``CompetencyRuleProfile.competency_taxonomy``. + +The four are CASCADE because deleting a Tag nobody holds mastery against must succeed, and #655 +forbids ``openedx_tagging`` from knowing CBE exists, so the tagging side cannot clear the +criteria tree first; CASCADE lets the tag's delete take the tree with it. ``parent`` and ``group`` +also need CASCADE because Django's collector looks up referencing rows in the database rather +than in the set it has already decided to delete, so even a parent and child reached in the same +batch would trip ``PROTECT`` and abort the walk partway down. + +This is not a relaxation of ADR-0002 Decision 7: these four CASCADE links are what carries the +collector down to the ``PROTECT`` that enforces it, on #642's three ``Student*Status`` foreign +keys one and two levels below the tag, reached only by walking CASCADE edges. Turning any link in +that chain to ``SET_NULL`` would let a tag delete succeed while learner statuses for it still exist. + +``on_delete`` governs the row a foreign key points AT, never the row holding it, and fires on +every row the collector reaches, not only the row passed to ``delete()``. So ``rule_profile`` +staying PROTECT does not block a cascading tag delete; it only stops a CompetencyRuleProfile from +being deleted while a criterion references it, Decision 7's archive-only rule at the ORM layer. + +The other four: both ``course`` fields match ``openedx_catalog``'s own convention +(``CourseRun.catalog_course`` and ``CatalogCourse.org`` are PROTECT too), and ``SET_NULL`` would +make a course-level group read as a root group, breaking #675's root-group rejection. +``organization`` is PROTECT because ``edx-organizations`` deactivates orgs rather than deleting +them (``remove_organization()``). ``competency_taxonomy`` is PROTECT because a profile's scope is +immutable, so ``SET_NULL`` is forbidden and CASCADE would only move the failure one hop. """ from __future__ import annotations @@ -127,14 +139,14 @@ class CompetencyCriteriaGroup(models.Model): "self", null=True, blank=True, - on_delete=models.PROTECT, + on_delete=models.CASCADE, related_name="child_groups", help_text=_("The parent CompetencyCriteriaGroup. Null means this group is a tree root."), ) tag = models.ForeignKey( Tag, db_column="oel_tagging_tag_id", - on_delete=models.PROTECT, + on_delete=models.CASCADE, related_name="competency_criteria_groups", help_text=_("The competency (tag) that this criteria tree evaluates mastery of."), ) @@ -376,14 +388,14 @@ class CompetencyCriterion(models.Model): group = models.ForeignKey( CompetencyCriteriaGroup, db_column="competency_criteria_group_id", - on_delete=models.PROTECT, + on_delete=models.CASCADE, related_name="criteria", help_text=_("The CompetencyCriteriaGroup this leaf criterion belongs to."), ) object_tag = models.ForeignKey( ObjectTag, db_column="oel_tagging_objecttag_id", - on_delete=models.PROTECT, + on_delete=models.CASCADE, related_name="competency_criteria", help_text=_("The tag/object association that this criterion evaluates."), ) diff --git a/src/openedx_learning/migrations/0002_competency_criteria.py b/src/openedx_learning/migrations/0002_competency_criteria.py index 2c521900c..fcdd2c817 100644 --- a/src/openedx_learning/migrations/0002_competency_criteria.py +++ b/src/openedx_learning/migrations/0002_competency_criteria.py @@ -37,8 +37,8 @@ class Migration(migrations.Migration): ('ordering', models.PositiveIntegerField(default=0, help_text='Deterministic sibling evaluation sequence. Used to short-circuit evaluation and to order child scans during event-driven recomputation.')), ('logic_operator', models.CharField(blank=True, choices=[('AND', 'And'), ('OR', 'Or')], help_text="How this group's children combine. Null until the group has children to combine.", max_length=3, null=True)), ('course', models.ForeignKey(blank=True, help_text='The course run that scopes this criteria tree for evaluation windowing, if any.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='competency_criteria_groups', to='openedx_catalog.courserun')), - ('parent', models.ForeignKey(blank=True, help_text='The parent CompetencyCriteriaGroup. Null means this group is a tree root.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='child_groups', to='openedx_learning.competencycriteriagroup')), - ('tag', models.ForeignKey(db_column='oel_tagging_tag_id', help_text='The competency (tag) that this criteria tree evaluates mastery of.', on_delete=django.db.models.deletion.PROTECT, related_name='competency_criteria_groups', to='oel_tagging.tag')), + ('parent', models.ForeignKey(blank=True, help_text='The parent CompetencyCriteriaGroup. Null means this group is a tree root.', null=True, on_delete=django.db.models.deletion.CASCADE, related_name='child_groups', to='openedx_learning.competencycriteriagroup')), + ('tag', models.ForeignKey(db_column='oel_tagging_tag_id', help_text='The competency (tag) that this criteria tree evaluates mastery of.', on_delete=django.db.models.deletion.CASCADE, related_name='competency_criteria_groups', to='oel_tagging.tag')), ], ), migrations.CreateModel( @@ -62,8 +62,8 @@ class Migration(migrations.Migration): ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), ('rule_type_override', models.CharField(blank=True, choices=[('View', 'View'), ('Grade', 'Grade'), ('MasteryLevel', 'Mastery Level')], max_length=32, null=True)), ('rule_payload_override', models.JSONField(blank=True, null=True)), - ('group', models.ForeignKey(db_column='competency_criteria_group_id', help_text='The CompetencyCriteriaGroup this leaf criterion belongs to.', on_delete=django.db.models.deletion.PROTECT, related_name='criteria', to='openedx_learning.competencycriteriagroup')), - ('object_tag', models.ForeignKey(db_column='oel_tagging_objecttag_id', help_text='The tag/object association that this criterion evaluates.', on_delete=django.db.models.deletion.PROTECT, related_name='competency_criteria', to='oel_tagging.objecttag')), + ('group', models.ForeignKey(db_column='competency_criteria_group_id', help_text='The CompetencyCriteriaGroup this leaf criterion belongs to.', on_delete=django.db.models.deletion.CASCADE, related_name='criteria', to='openedx_learning.competencycriteriagroup')), + ('object_tag', models.ForeignKey(db_column='oel_tagging_objecttag_id', help_text='The tag/object association that this criterion evaluates.', on_delete=django.db.models.deletion.CASCADE, related_name='competency_criteria', to='oel_tagging.objecttag')), ('rule_profile', models.ForeignKey(blank=True, db_column='competency_rule_profile_id', help_text='The profile this criterion uses by default. Null only when overrides are set instead.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='criteria', to='openedx_learning.competencyruleprofile')), ], options={ diff --git a/tests/openedx_learning/applets/cbe/test_criteria_deletion.py b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py index ad4e62466..97d6c93bc 100644 --- a/tests/openedx_learning/applets/cbe/test_criteria_deletion.py +++ b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py @@ -1,11 +1,11 @@ """ Delete-behavior tests for CompetencyCriteriaGroup, CompetencyRuleProfile, and CompetencyCriterion. -Every foreign key these three models declare is currently `on_delete=models.PROTECT`; see the -module docstring in `openedx_learning.applets.cbe.models.criteria` for why that is the current -fail-closed value rather than a settled one, and what is still open about it on #655. +Four of these nine foreign keys are `on_delete=models.CASCADE` and five are `models.PROTECT`; see +the module docstring in `openedx_learning.applets.cbe.models.criteria` for which is which and why. """ import pytest +from django.apps import apps from django.db.models import ProtectedError from organizations.api import ensure_organization from organizations.models import Organization @@ -85,38 +85,49 @@ def _default_rule_profile() -> CompetencyRuleProfile: # ============================================================================================== -# One test per foreign key, all nine currently PROTECT. Each asserts on the raised -# ProtectedError's `protected_objects`, not just its type: several protected relationships can -# fire on one delete (see test_rule_profile_organization_protect and -# test_rule_profile_competency_taxonomy_protect below for two real traps of that kind), so a bare +# One test per foreign key. The five that stayed PROTECT assert ProtectedError and inspect +# `protected_objects` to confirm which relationship actually fired: several protected +# relationships can fire on one delete (see test_rule_profile_organization_protect below for a +# real trap of that kind, where CatalogCourse.org is also PROTECT), so a bare # `pytest.raises(ProtectedError)` would not actually prove which foreign key did the protecting. +# The four that became CASCADE assert the delete succeeds and that the referencing row is +# actually gone from the database afterward, not merely that no exception was raised, and assert +# the referencing row existed beforehand, so the "gone" assertion can't pass because a fixture +# never created it in the first place. # ============================================================================================== -def test_group_parent_protect(tag: Tag) -> None: +def test_group_parent_cascade(tag: Tag) -> None: """ - Deleting a CompetencyCriteriaGroup that another group's `parent` points at raises - ProtectedError. Django's PROTECT raises even though the referencing child group is not part - of this delete call; nothing about `parent` being a self-referential, tree-shaped - relationship exempts it from that. + Deleting a CompetencyCriteriaGroup cascades to any child group referencing it via `parent`: + the delete succeeds and the child row is gone too. """ root = CompetencyCriteriaGroup.objects.create(tag=tag) child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + assert CompetencyCriteriaGroup.objects.filter(pk=child.pk).exists() - with pytest.raises(ProtectedError) as exc_info: - root.delete() + root.delete() - protected = exc_info.value.protected_objects - assert any(isinstance(obj, CompetencyCriteriaGroup) and obj.pk == child.pk for obj in protected) + assert not CompetencyCriteriaGroup.objects.filter(pk=root.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=child.pk).exists() -def test_group_tag_protect(tag: Tag, group: CompetencyCriteriaGroup) -> None: - """Deleting a Tag that a CompetencyCriteriaGroup references via `tag` raises ProtectedError.""" - with pytest.raises(ProtectedError) as exc_info: - tag.delete() +def test_group_tag_cascade(tag: Tag, group: CompetencyCriteriaGroup) -> None: + """ + Deleting a Tag cascades to any CompetencyCriteriaGroup referencing it via `tag`: the delete + succeeds and the group row is gone. Also confirms django-simple-history records the cascaded + removal as its own historical row (history_type='-'), not silently: an author or auditor + reviewing history for a group that vanished this way still finds why it did. + """ + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + group_pk = group.pk - protected = exc_info.value.protected_objects - assert any(isinstance(obj, CompetencyCriteriaGroup) and obj.pk == group.pk for obj in protected) + tag.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group_pk).exists() + + historical_group = apps.get_model("openedx_learning", "HistoricalCompetencyCriteriaGroup") + assert historical_group.objects.filter(id=group_pk, history_type="-").exists() def test_group_course_protect(tag: Tag, course_run: CourseRun) -> None: @@ -170,11 +181,11 @@ def test_rule_profile_competency_taxonomy_protect(competency_taxonomy: Competenc Deleting a CompetencyTaxonomy that a CompetencyRuleProfile references via `competency_taxonomy` raises ProtectedError naming the profile. - Deliberately does not use the `tag` or `group` fixtures: a Tag under this taxonomy would be - collected by Tag.taxonomy's CASCADE, and a CompetencyCriteriaGroup referencing that tag would - then hit its own `tag` PROTECT (see test_taxonomy_delete_blocked_by_group_tag_protection - below), which would raise ProtectedError without this test having exercised - CompetencyRuleProfile.competency_taxonomy at all. + Deliberately does not use the `tag` or `group` fixtures: they are not needed to isolate this + relationship. Tag.taxonomy and CompetencyCriteriaGroup.tag are both CASCADE now, so a tag and + group under this taxonomy would just be silently left untouched by the aborted delete (the + whole operation rolls back once any PROTECT fires) rather than competing for the raised + error's `protected_objects`; keeping this test to only what it needs stays the clearer read. """ profile = CompetencyRuleProfile.objects.create( competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD @@ -187,37 +198,41 @@ def test_rule_profile_competency_taxonomy_protect(competency_taxonomy: Competenc assert any(isinstance(obj, CompetencyRuleProfile) and obj.pk == profile.pk for obj in protected) -def test_criterion_group_protect( +def test_criterion_group_cascade( group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile ) -> None: """ - Deleting a CompetencyCriteriaGroup that a CompetencyCriterion references via `group` raises - ProtectedError, even though the criterion is not itself part of this delete call. + Deleting a CompetencyCriteriaGroup cascades to any CompetencyCriterion referencing it via + `group`: the delete succeeds and the criterion row is gone too. """ criterion = CompetencyCriterion.objects.create( group=group, object_tag=object_tag, rule_profile=default_rule_profile ) + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() - with pytest.raises(ProtectedError) as exc_info: - group.delete() + group.delete() - protected = exc_info.value.protected_objects - assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in protected) + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() -def test_criterion_object_tag_protect( +def test_criterion_object_tag_cascade( group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile ) -> None: - """Deleting an ObjectTag that a CompetencyCriterion references via `object_tag` raises ProtectedError.""" + """ + Deleting an ObjectTag cascades to any CompetencyCriterion referencing it via `object_tag`: the + delete succeeds and the criterion row is gone too. Doubles as the "OURS" half of #641's + Deletions criterion for oel_tagging_objecttag, since ObjectTag has only this one hop down to + CompetencyCriterion. + """ criterion = CompetencyCriterion.objects.create( group=group, object_tag=object_tag, rule_profile=default_rule_profile ) + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() - with pytest.raises(ProtectedError) as exc_info: - object_tag.delete() + object_tag.delete() - protected = exc_info.value.protected_objects - assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in protected) + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() def test_criterion_rule_profile_protect( @@ -238,24 +253,101 @@ def test_criterion_rule_profile_protect( assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in protected) -def test_taxonomy_delete_blocked_by_group_tag_protection( - competency_taxonomy: CompetencyTaxonomy, group: CompetencyCriteriaGroup +# ============================================================================================== +# Transitive deletion tests required by #641's Deletions criteria: deleting an oel_tagging.Tag, +# a CompetencyCriteriaGroup at depth, an oel_tagging.ObjectTag, or an oel_tagging.Taxonomy, when +# no learner status exists beneath the target, must succeed and take the whole referencing +# criteria tree with it. +# +# Each of these criteria also has a "raises ProtectedError when a learner status row exists +# beneath it" half. That half is NOT covered here: it needs #642's Student*Status tables, which +# do not exist on this branch, and #642's own criterion says those tests belong in the slice that +# follows #641, once those tables exist. This file does not stub, mock, or fake a status model to +# test them; their absence here is deliberate, not an oversight. +# ============================================================================================== + + +def test_tag_delete_with_no_status_cascades_whole_criteria_tree( + tag: Tag, group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile ) -> None: """ - #655's approved design promises that deleting a CompetencyTaxonomy whose tag no learner holds - mastery against succeeds as a plain hard delete: Tag.taxonomy is CASCADE, so the tag is - collected along with the taxonomy. `PROTECT` on CompetencyCriteriaGroup.tag currently breaks - that promise instead: the delete collects `tag` via CASCADE, then `group`'s reference to that - tag hits PROTECT and the whole delete is refused, even though no learner has been graded - against it (there is no learner-status table yet at all). - - This conflict is open on #655 (see the module docstring in - openedx_learning.applets.cbe.models.criteria) and unresolved as of this writing. Whichever - way it resolves, this test is the one that has to change: if CompetencyCriteriaGroup.tag - moves to CASCADE, this becomes an assertion that the delete succeeds instead of raising. + Deleting an oel_tagging.Tag with no learner status beneath it succeeds and cascades away + every CompetencyCriteriaGroup and CompetencyCriterion that references it, transitively: + Tag -> CompetencyCriteriaGroup.tag (CASCADE) -> CompetencyCriterion.group (CASCADE). """ - with pytest.raises(ProtectedError) as exc_info: - competency_taxonomy.delete() + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() - protected = exc_info.value.protected_objects - assert any(isinstance(obj, CompetencyCriteriaGroup) and obj.pk == group.pk for obj in protected) + tag.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +def test_group_delete_at_depth_cascades_descendants_and_their_criteria( + tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting a CompetencyCriteriaGroup that is not a root removes it, every descendant group, and + every CompetencyCriterion under any of them, while leaving the rest of the tree (here, the + root) alone. + + Builds a genuinely nested tree, root -> child -> grandchild, with criteria at two different + levels (on `child` and on `grandchild`), so "at depth" and "every descendant" both mean + something: a shallower tree could pass this by accident. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + grandchild = CompetencyCriteriaGroup.objects.create(tag=tag, parent=child) + child_criterion = CompetencyCriterion.objects.create( + group=child, object_tag=object_tag, rule_profile=default_rule_profile + ) + grandchild_criterion = CompetencyCriterion.objects.create( + group=grandchild, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert CompetencyCriteriaGroup.objects.filter(pk=root.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=child.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=grandchild.pk).exists() + assert CompetencyCriterion.objects.filter(pk=child_criterion.pk).exists() + assert CompetencyCriterion.objects.filter(pk=grandchild_criterion.pk).exists() + + child.delete() + + assert CompetencyCriteriaGroup.objects.filter(pk=root.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=child.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=grandchild.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=child_criterion.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=grandchild_criterion.pk).exists() + + +def test_taxonomy_delete_cascades_every_tag_and_its_criteria( + competency_taxonomy: CompetencyTaxonomy, + tag: Tag, + group: CompetencyCriteriaGroup, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Deleting an oel_tagging.Taxonomy collects every Tag beneath it (Tag.taxonomy is CASCADE), so + the tag-deletion cases above hold transitively through a taxonomy delete too. This asserts the + succeeding case (no learner status beneath the tag), which is what #641's Deletions criterion + for taxonomy-level deletion requires "at minimum". + + Chain exercised: CompetencyTaxonomy -> Tag (CASCADE) -> CompetencyCriteriaGroup.tag (CASCADE) + -> CompetencyCriterion.group (CASCADE). + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert Tag.objects.filter(pk=tag.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + competency_taxonomy.delete() + + assert not Tag.objects.filter(pk=tag.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() From 9a3ce50ccbfaeb67cb9c0de0440d51f065f40b55 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 3 Sep 2026 16:46:19 -0400 Subject: [PATCH 5/5] feat: add mastery status lookup and the three learner status models Implements #642's lookup data and learner progress models: the CompetencyMasteryStatuses lookup plus StudentCompetencyCriteriaStatus, StudentCompetencyCriteriaGroupStatus and StudentCompetencyStatus, tracking a learner's mastery at the leaf, group and competency levels. Restacked onto #641. The models live in models/learner_status.py inside the package #641 creates, and the migrations are 0004 and 0005 parented onto #641's 0003_seed_default_rule_profile, so the app keeps a single migration leaf. An earlier revision of this branch added to a top-level models.py and numbered its migrations 0002 and 0003; both would have conflicted with #641 irreconcilably rather than merging. Status rank order lives in pinned primary keys rather than a rank column. The check constraint restricting StudentCompetencyStatus has to compare status_id against a literal, since MySQL allows no subquery in a CHECK, so the ids are load-bearing constants either way and a second ordering column could only drift from them. ADR-0002 Decision 6 also caps the lookup table at two columns. The node foreign keys are PROTECT and that is load-bearing, not defensive: #641's four definition-to-definition foreign keys are CASCADE, so these are the only thing that stops the collector walking from a Tag down through groups to criteria. They are deliberately stricter than the application-layer predicate, which ADR-0002 Decision 7 scopes to the leaf table alone, and fail closed. The user foreign key is CASCADE so this library cannot veto User.delete() platform-wide. Column names follow #641: idiomatic Python attributes with db_column set to the names ADR-0002 Decision 6 uses, so indexes 6, 7 and 8 land on competency_criteria_id, competency_criteria_group_id and oel_tagging_tag_id as specified. Timestamps use manual_date_time_field() rather than auto_now_add/auto_now. auto_now never fires on QuerySet.update(), which is the write path ADR-0004 Decision 4 mandates, so it would leave modified stale on exactly the path that matters. This diverges from the acceptance criteria and is flagged on the pull request. Twenty-four tests, including every ProtectedError case #613 describes. Those moved here from #641 because asserting one needs a Student*Status row and this is the change that creates those tables. Co-Authored-By: Claude Opus 5 (1M context) --- src/openedx_learning/applets/cbe/admin.py | 64 +- .../applets/cbe/models/__init__.py | 12 + .../applets/cbe/models/learner_status.py | 329 ++++++++ .../migrations/0004_learner_status_models.py | 79 ++ .../0005_seed_competency_mastery_statuses.py | 36 + .../applets/cbe/test_mastery.py | 726 ++++++++++++++++++ 6 files changed, 1245 insertions(+), 1 deletion(-) create mode 100644 src/openedx_learning/applets/cbe/models/learner_status.py create mode 100644 src/openedx_learning/migrations/0004_learner_status_models.py create mode 100644 src/openedx_learning/migrations/0005_seed_competency_mastery_statuses.py create mode 100644 tests/openedx_learning/applets/cbe/test_mastery.py diff --git a/src/openedx_learning/applets/cbe/admin.py b/src/openedx_learning/applets/cbe/admin.py index 71f7cf583..847dd88ba 100644 --- a/src/openedx_learning/applets/cbe/admin.py +++ b/src/openedx_learning/applets/cbe/admin.py @@ -3,10 +3,22 @@ """ from django.contrib import admin -from .models import CompetencyTaxonomy +from openedx_django_lib.admin_utils import ReadOnlyModelAdmin + +from .models import ( + CompetencyMasteryStatus, + CompetencyTaxonomy, + StudentCompetencyCriteriaGroupStatus, + StudentCompetencyCriteriaStatus, + StudentCompetencyStatus, +) __all__ = [ "CompetencyTaxonomyAdmin", + "CompetencyMasteryStatusAdmin", + "StudentCompetencyCriteriaStatusAdmin", + "StudentCompetencyCriteriaGroupStatusAdmin", + "StudentCompetencyStatusAdmin", ] @@ -18,4 +30,54 @@ class CompetencyTaxonomyAdmin(admin.ModelAdmin): list_filter = ["enabled"] +class CompetencyMasteryStatusAdmin(ReadOnlyModelAdmin): + """ + The CompetencyMasteryStatus model admin. + """ + list_display = ["id", "status"] + + +class StudentCompetencyCriteriaStatusAdmin(ReadOnlyModelAdmin): + """ + The StudentCompetencyCriteriaStatus model admin. + + Deliberately read-only: an editable page would be the staff-correction + path, which ADR-0004 Decision 6 requires to take a row lock and recompute + every ancestor status, and none of that machinery exists yet. + """ + list_display = ["user", "criterion", "status", "created", "modified"] + list_filter = ["status"] + list_select_related = ["user", "criterion", "status"] + + +class StudentCompetencyCriteriaGroupStatusAdmin(ReadOnlyModelAdmin): + """ + The StudentCompetencyCriteriaGroupStatus model admin. + + Deliberately read-only: an editable page would be the staff-correction + path, which ADR-0004 Decision 6 requires to take a row lock and recompute + every ancestor status, and none of that machinery exists yet. + """ + list_display = ["user", "group", "status", "created", "modified"] + list_filter = ["status"] + list_select_related = ["user", "group", "status"] + + +class StudentCompetencyStatusAdmin(ReadOnlyModelAdmin): + """ + The StudentCompetencyStatus model admin. + + Deliberately read-only: an editable page would be the staff-correction + path, which ADR-0004 Decision 6 requires to take a row lock and recompute + every ancestor status, and none of that machinery exists yet. + """ + list_display = ["user", "tag", "status", "created", "modified"] + list_filter = ["status"] + list_select_related = ["user", "tag", "status"] + + admin.site.register(CompetencyTaxonomy, CompetencyTaxonomyAdmin) +admin.site.register(CompetencyMasteryStatus, CompetencyMasteryStatusAdmin) +admin.site.register(StudentCompetencyCriteriaStatus, StudentCompetencyCriteriaStatusAdmin) +admin.site.register(StudentCompetencyCriteriaGroupStatus, StudentCompetencyCriteriaGroupStatusAdmin) +admin.site.register(StudentCompetencyStatus, StudentCompetencyStatusAdmin) diff --git a/src/openedx_learning/applets/cbe/models/__init__.py b/src/openedx_learning/applets/cbe/models/__init__.py index 9d71edfa4..da5159dc7 100644 --- a/src/openedx_learning/applets/cbe/models/__init__.py +++ b/src/openedx_learning/applets/cbe/models/__init__.py @@ -11,6 +11,13 @@ RuleType, validate_rule_payload, ) +from .learner_status import ( + CompetencyMasteryStatus, + MasteryStatus, + StudentCompetencyCriteriaGroupStatus, + StudentCompetencyCriteriaStatus, + StudentCompetencyStatus, +) __all__ = [ "CompetencyTaxonomy", @@ -20,4 +27,9 @@ "LogicOperator", "RuleType", "validate_rule_payload", + "MasteryStatus", + "CompetencyMasteryStatus", + "StudentCompetencyCriteriaStatus", + "StudentCompetencyCriteriaGroupStatus", + "StudentCompetencyStatus", ] diff --git a/src/openedx_learning/applets/cbe/models/learner_status.py b/src/openedx_learning/applets/cbe/models/learner_status.py new file mode 100644 index 000000000..8e4f09df8 --- /dev/null +++ b/src/openedx_learning/applets/cbe/models/learner_status.py @@ -0,0 +1,329 @@ +""" +Models tracking a learner's mastery status at each level of a competency criteria tree. + +:class:`StudentCompetencyCriteriaStatus`, :class:`StudentCompetencyCriteriaGroupStatus`, and +:class:`StudentCompetencyStatus` track the same fact -- a learner's current mastery rank -- at +the leaf (:class:`~openedx_learning.applets.cbe.models.criteria.CompetencyCriterion`), group +(:class:`~openedx_learning.applets.cbe.models.criteria.CompetencyCriteriaGroup`), and top +(:class:`~openedx_tagging.models.Tag`) levels of a criteria tree, respectively. All three share +one shape: a user, a foreign key to the node they track, a status, and caller-supplied +``created``/``modified`` timestamps. + +There is one row per learner per node, updated in place: finding a learner's current status at +any level is a lookup of that single row, not a query for the most recent of several (ADR-0003 +Decision 5). Each model accepts any status value its constraints permit; +the rules that decide *which* writes are allowed, that an automatic write may raise a status but +never lower it (ADR-0004 Decision 4), and that a staff correction may lower one (ADR-0004 +Decision 6), are enforced in the API layer, not here: by the time a write reaches these models +there is no caller context left to tell those cases apart. + +``created`` and ``modified`` are caller-supplied UTC datetimes, not automatic. A caller +performing a conditional raise must pass ``modified`` in the same ``update()`` call; there is no +``auto_now`` to do it for them, deliberately, because ``auto_now`` does not fire on +``QuerySet.update()`` and would silently leave the column stale on exactly that path. + +Each model's ``user`` foreign key is ``on_delete=models.CASCADE``, not ``PROTECT``: ``PROTECT`` +would let this library veto ``User.delete()`` platform-wide, from openedx-platform code that has +no reason to know CBE rows exist. A learner's status is a derived fact about that learner, so it +goes when they do. ``SET_NULL`` is not an option, because a null ``user_id`` would break the +``(user_id, node_id)`` uniqueness each model's in-place updates rest on. + +Each model's foreign key to the node it tracks (``criterion``, ``group``, or ``tag``) is +``on_delete=models.PROTECT``, and it is load-bearing, not defensive. #641's four foreign keys +that carry Django's collector down the criteria tree -- ``CompetencyCriteriaGroup.parent``, +``CompetencyCriteriaGroup.tag``, ``CompetencyCriterion.group``, and +``CompetencyCriterion.object_tag`` -- are ``CASCADE``, so deleting a ``Tag`` walks into its +groups and then their criteria. These ``PROTECT`` values are the only thing that stops that +walk, and they are what turns ADR-0002 Decision 7's guarantee into behavior: the delete succeeds +when no learner holds status beneath the row and raises ``ProtectedError`` when one does. Django +evaluates ``PROTECT`` on every row the collector reaches, not only the row passed to +``delete()``, which is why transitive cases work too. #675 re-implements the same predicate at +the API layer for a clean status code; this is the backstop for paths that never reach it. + +These three ``PROTECT`` values are deliberately stricter than the application-layer predicate. +ADR-0002 Decision 7, as amended for #655, names only the leaf table +``StudentCompetencyCriteriaStatus`` as determining whether a record is protected, and treats the +two roll-up tables as derived and not independently checked. The database makes no such +distinction, so a roll-up row with no leaf beneath it would also block a delete. That should not +occur, and failing closed is the right default for a backstop. + +Each model's ``status`` foreign key is also ``on_delete=models.PROTECT``, because the lookup +table it points to (:class:`CompetencyMasteryStatus`) is system-owned immutable data, seeded by +migration and never deleted. +""" +from django.conf import settings +from django.db import models +from django.utils.translation import gettext_lazy as _ + +from openedx_django_lib.fields import manual_date_time_field +from openedx_tagging.models import Tag + +from .criteria import CompetencyCriteriaGroup, CompetencyCriterion + +__all__ = [ + "MasteryStatus", + "CompetencyMasteryStatus", + "StudentCompetencyCriteriaStatus", + "StudentCompetencyCriteriaGroupStatus", + "StudentCompetencyStatus", +] + + +class MasteryStatus(models.IntegerChoices): + """ + Ranks of competency mastery. + + Each member's value is the pinned primary key of its ``CompetencyMasteryStatus`` + row, seeded by the ``seed_competency_mastery_statuses`` data migration. + + The integer value of each member is also its rank, lowest to highest mastery. + Pinning the rank order into the stored id is what lets raising a learner's + status be written as one conditional ``UPDATE`` + (``... WHERE status_id < new_status_id``) instead of a read, a comparison in + Python, and a write: ADR-0004 Decision 4 requires that, because two concurrent + writers doing read-compare-write can each read the same old value, and the + later of the two writes then lowers what the earlier one had already raised. + + Because the ids are pinned, a new status can be added above or below the + existing three, but never between them. + + This subclasses ``IntegerChoices`` rather than ``enum.IntEnum`` so that + Django's migration serializer writes a member as a bare integer instead of an + import of this module. That keeps an already-applied migration's meaning + independent of later edits to this enum. + """ + + ATTEMPTED_NOT_DEMONSTRATED = 1, "AttemptedNotDemonstrated" + PARTIALLY_ATTEMPTED = 2, "PartiallyAttempted" + DEMONSTRATED = 3, "Demonstrated" + + +class CompetencyMasteryStatus(models.Model): + """ + Lookup table of the mastery statuses a competency can be assigned. + + This table is system-owned lookup data, seeded by the + ``seed_competency_mastery_statuses`` data migration, and is + treated as immutable configuration rather than user-authored rows + (ADR-0002 Decision 6.1). See :class:`MasteryStatus` for the pinned ids and + names of its rows. + + .. no_pii: + """ + + # ADR-0002 Decision 5 index 10. + status = models.CharField(max_length=64, unique=True) + + def __str__(self) -> str: + """User-facing string representation of a CompetencyMasteryStatus.""" + return self.status + + class Meta: + verbose_name = "Competency Mastery Status" + verbose_name_plural = "Competency Mastery Statuses" + # id order is rank order (see MasteryStatus), so a default listing of + # this table already reads lowest to highest mastery. + ordering = ("id",) + + +class StudentCompetencyCriteriaStatus(models.Model): + """ + A learner's current mastery status for one leaf ``CompetencyCriterion``. + + There is one row per learner per criterion, updated in place: finding a + learner's current status is a lookup of that single row, not a query for the + most recent of several (ADR-0003 Decision 5). + + This model accepts any status value the constraints below permit. The rules + that decide *which* writes are allowed, that an automatic write may raise a + status but never lower it, and that a staff correction may lower one, are + enforced in the API layer, not here: by the time a write reaches this model + there is no caller context left to tell those two cases apart. + + ``created`` and ``modified`` are caller-supplied UTC datetimes, not automatic. + A caller performing a conditional raise must pass ``modified`` in the same + ``update()`` call; there is no ``auto_now`` to do it for them, deliberately, + because ``auto_now`` does not fire on ``QuerySet.update()`` and would silently + leave the column stale on exactly that path. + + This table stores a user foreign key and a status value, and no personal + data of its own. + + .. no_pii: + """ + + user = models.ForeignKey( + settings.AUTH_USER_MODEL, + on_delete=models.CASCADE, + related_name="competency_criteria_statuses", + ) + criterion = models.ForeignKey( + CompetencyCriterion, + db_column="competency_criteria_id", + on_delete=models.PROTECT, + related_name="student_statuses", + ) + status = models.ForeignKey( + CompetencyMasteryStatus, + on_delete=models.PROTECT, + related_name="student_criteria_statuses", + ) + created = manual_date_time_field() + modified = manual_date_time_field() + + def __str__(self) -> str: + """User-facing string representation of a StudentCompetencyCriteriaStatus.""" + return f"{self.user}: {self.criterion} = {self.status}" + + class Meta: + verbose_name = "Student Competency Criteria Status" + verbose_name_plural = "Student Competency Criteria Statuses" + constraints = [ + # ADR-0002 Decision 5 index 6. This is what makes "one row per + # learner and criterion" true, which is the precondition for the + # in-place conditional update described above: it is load-bearing, + # not a lookup optimisation. + models.UniqueConstraint( + fields=("user", "criterion"), + name="oex_learning_studentcriteriastatus_user_criterion_uniq", + ), + ] + + +class StudentCompetencyCriteriaGroupStatus(models.Model): + """ + A learner's current mastery status for one ``CompetencyCriteriaGroup`` node. + + There is one row per learner per group, updated in place: finding a + learner's current status is a lookup of that single row, not a query for the + most recent of several (ADR-0003 Decision 5). + + This model accepts any status value the constraints below permit. The rules + that decide *which* writes are allowed, that an automatic write may raise a + status but never lower it, and that a staff correction may lower one, are + enforced in the API layer, not here: by the time a write reaches this model + there is no caller context left to tell those two cases apart. + + ``created`` and ``modified`` are caller-supplied UTC datetimes, not automatic. + A caller performing a conditional raise must pass ``modified`` in the same + ``update()`` call; there is no ``auto_now`` to do it for them, deliberately, + because ``auto_now`` does not fire on ``QuerySet.update()`` and would silently + leave the column stale on exactly that path. + + This table stores a user foreign key and a status value, and no personal + data of its own. + + .. no_pii: + """ + + user = models.ForeignKey( + settings.AUTH_USER_MODEL, + on_delete=models.CASCADE, + related_name="competency_criteria_group_statuses", + ) + group = models.ForeignKey( + CompetencyCriteriaGroup, + db_column="competency_criteria_group_id", + on_delete=models.PROTECT, + related_name="student_statuses", + ) + status = models.ForeignKey( + CompetencyMasteryStatus, + on_delete=models.PROTECT, + related_name="student_criteria_group_statuses", + ) + created = manual_date_time_field() + modified = manual_date_time_field() + + def __str__(self) -> str: + """User-facing string representation of a StudentCompetencyCriteriaGroupStatus.""" + return f"{self.user}: {self.group} = {self.status}" + + class Meta: + verbose_name = "Student Competency Criteria Group Status" + verbose_name_plural = "Student Competency Criteria Group Statuses" + constraints = [ + # ADR-0002 Decision 5 index 7. This is what makes "one row per + # learner and group" true, which is the precondition for the + # in-place conditional update described above: it is load-bearing, + # not a lookup optimisation. + models.UniqueConstraint( + fields=("user", "group"), + name="oex_learning_studentcriteriagroupstatus_user_group_uniq", + ), + ] + + +class StudentCompetencyStatus(models.Model): + """ + A learner's current mastery status for one competency (``Tag``). + + There is one row per learner per competency, updated in place: finding a + learner's current status is a lookup of that single row, not a query for the + most recent of several (ADR-0003 Decision 5). + + This model accepts any status value the allow-list constraint below permits. + The rules that decide *which* writes are allowed, that an automatic write may + raise a status but never lower it, and that a staff correction may lower one, + are enforced in the API layer, not here: by the time a write reaches this + model there is no caller context left to tell those two cases apart. + + ``created`` and ``modified`` are caller-supplied UTC datetimes, not automatic. + A caller performing a conditional raise must pass ``modified`` in the same + ``update()`` call; there is no ``auto_now`` to do it for them, deliberately, + because ``auto_now`` does not fire on ``QuerySet.update()`` and would silently + leave the column stale on exactly that path. + + This table stores a user foreign key and a status value, and no personal + data of its own. + + .. no_pii: + """ + + user = models.ForeignKey( + settings.AUTH_USER_MODEL, + on_delete=models.CASCADE, + related_name="competency_mastery_statuses", + ) + tag = models.ForeignKey( + Tag, + db_column="oel_tagging_tag_id", + on_delete=models.PROTECT, + related_name="student_competency_statuses", + ) + status = models.ForeignKey( + CompetencyMasteryStatus, + on_delete=models.PROTECT, + related_name="student_competency_statuses", + ) + created = manual_date_time_field() + modified = manual_date_time_field() + + def __str__(self) -> str: + """User-facing string representation of a StudentCompetencyStatus.""" + return f"{self.user}: {self.tag} = {self.status}" + + class Meta: + verbose_name = "Student Competency Status" + verbose_name_plural = "Student Competency Statuses" + constraints = [ + # ADR-0002 Decision 5 index 8. This is what makes "one row per + # learner and competency" true, which is the precondition for the + # in-place conditional update above: it is load-bearing, not a + # lookup optimisation. + models.UniqueConstraint( + fields=("user", "tag"), + name="oex_learning_studentcompetencystatus_user_tag_uniq", + ), + # Allow list, not a negation of the excluded value: a future fourth + # status should be rejected here by default rather than silently + # permitted. + models.CheckConstraint( + condition=models.Q(status__in=(MasteryStatus.PARTIALLY_ATTEMPTED, MasteryStatus.DEMONSTRATED)), + name="oex_learning_studentcompetencystatus_status_allowed", + violation_error_message=_( + "A competency-level status may only be PartiallyAttempted or Demonstrated, " + "since it represents overall demonstration state, not an in-progress state." + ), + ), + ] diff --git a/src/openedx_learning/migrations/0004_learner_status_models.py b/src/openedx_learning/migrations/0004_learner_status_models.py new file mode 100644 index 000000000..31f55869d --- /dev/null +++ b/src/openedx_learning/migrations/0004_learner_status_models.py @@ -0,0 +1,79 @@ +# Generated by Django 5.2.16 on 2026-09-03 20:28 + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + +import openedx_django_lib.validators + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0021_remove_system_defined_add_read_only'), + ('openedx_learning', '0003_seed_default_rule_profile'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name='CompetencyMasteryStatus', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('status', models.CharField(max_length=64, unique=True)), + ], + options={ + 'verbose_name': 'Competency Mastery Status', + 'verbose_name_plural': 'Competency Mastery Statuses', + 'ordering': ('id',), + }, + ), + migrations.CreateModel( + name='StudentCompetencyCriteriaGroupStatus', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', models.DateTimeField(validators=[openedx_django_lib.validators.validate_utc_datetime])), + ('modified', models.DateTimeField(validators=[openedx_django_lib.validators.validate_utc_datetime])), + ('group', models.ForeignKey(db_column='competency_criteria_group_id', on_delete=django.db.models.deletion.PROTECT, related_name='student_statuses', to='openedx_learning.competencycriteriagroup')), + ('status', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, related_name='student_criteria_group_statuses', to='openedx_learning.competencymasterystatus')), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='competency_criteria_group_statuses', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'Student Competency Criteria Group Status', + 'verbose_name_plural': 'Student Competency Criteria Group Statuses', + 'constraints': [models.UniqueConstraint(fields=('user', 'group'), name='oex_learning_studentcriteriagroupstatus_user_group_uniq')], + }, + ), + migrations.CreateModel( + name='StudentCompetencyCriteriaStatus', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', models.DateTimeField(validators=[openedx_django_lib.validators.validate_utc_datetime])), + ('modified', models.DateTimeField(validators=[openedx_django_lib.validators.validate_utc_datetime])), + ('criterion', models.ForeignKey(db_column='competency_criteria_id', on_delete=django.db.models.deletion.PROTECT, related_name='student_statuses', to='openedx_learning.competencycriterion')), + ('status', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, related_name='student_criteria_statuses', to='openedx_learning.competencymasterystatus')), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='competency_criteria_statuses', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'Student Competency Criteria Status', + 'verbose_name_plural': 'Student Competency Criteria Statuses', + 'constraints': [models.UniqueConstraint(fields=('user', 'criterion'), name='oex_learning_studentcriteriastatus_user_criterion_uniq')], + }, + ), + migrations.CreateModel( + name='StudentCompetencyStatus', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', models.DateTimeField(validators=[openedx_django_lib.validators.validate_utc_datetime])), + ('modified', models.DateTimeField(validators=[openedx_django_lib.validators.validate_utc_datetime])), + ('status', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, related_name='student_competency_statuses', to='openedx_learning.competencymasterystatus')), + ('tag', models.ForeignKey(db_column='oel_tagging_tag_id', on_delete=django.db.models.deletion.PROTECT, related_name='student_competency_statuses', to='oel_tagging.tag')), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='competency_mastery_statuses', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'Student Competency Status', + 'verbose_name_plural': 'Student Competency Statuses', + 'constraints': [models.UniqueConstraint(fields=('user', 'tag'), name='oex_learning_studentcompetencystatus_user_tag_uniq'), models.CheckConstraint(condition=models.Q(('status__in', (2, 3))), name='oex_learning_studentcompetencystatus_status_allowed', violation_error_message='A competency-level status may only be PartiallyAttempted or Demonstrated, since it represents overall demonstration state, not an in-progress state.')], + }, + ), + ] diff --git a/src/openedx_learning/migrations/0005_seed_competency_mastery_statuses.py b/src/openedx_learning/migrations/0005_seed_competency_mastery_statuses.py new file mode 100644 index 000000000..d9f503ee4 --- /dev/null +++ b/src/openedx_learning/migrations/0005_seed_competency_mastery_statuses.py @@ -0,0 +1,36 @@ +from django.db import migrations + +# These ids and strings are literals, not references to `MasteryStatus` +# (`src/openedx_learning/applets/cbe/models.py`), and must stay that way: once +# applied, a migration has to keep meaning what it meant at the time it ran, so +# it cannot depend on a constant that a later edit to that enum could change. +# See `MasteryStatus` for the names these ids correspond to. + + +def forward(apps, schema_editor): + """ + Seed the three CompetencyMasteryStatus rows, in rank order. + """ + CompetencyMasteryStatus = apps.get_model("openedx_learning", "CompetencyMasteryStatus") + CompetencyMasteryStatus.objects.get_or_create(id=1, defaults={"status": "AttemptedNotDemonstrated"}) + CompetencyMasteryStatus.objects.get_or_create(id=2, defaults={"status": "PartiallyAttempted"}) + CompetencyMasteryStatus.objects.get_or_create(id=3, defaults={"status": "Demonstrated"}) + + +def revert(apps, schema_editor): # pragma: no cover + """ + Delete the seeded CompetencyMasteryStatus rows. + """ + CompetencyMasteryStatus = apps.get_model("openedx_learning", "CompetencyMasteryStatus") + CompetencyMasteryStatus.objects.filter(id__in=(1, 2, 3)).delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ("openedx_learning", "0004_learner_status_models"), + ] + + operations = [ + migrations.RunPython(forward, revert), + ] diff --git a/tests/openedx_learning/applets/cbe/test_mastery.py b/tests/openedx_learning/applets/cbe/test_mastery.py new file mode 100644 index 000000000..e523c7881 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_mastery.py @@ -0,0 +1,726 @@ +""" +Tests for the mastery status models: MasteryStatus, CompetencyMasteryStatus, +StudentCompetencyCriteriaStatus, StudentCompetencyCriteriaGroupStatus, and +StudentCompetencyStatus. + +The deletion tests below are this ticket's headline responsibility, not an afterthought. +#641's foreign keys that carry Django's collector down a criteria tree +(`CompetencyCriteriaGroup.parent`, `CompetencyCriteriaGroup.tag`, `CompetencyCriterion.group`, +`CompetencyCriterion.object_tag`) are all CASCADE, and +`tests/openedx_learning/applets/cbe/test_criteria_deletion.py` only asserts that CASCADE half of +each transitive case, because the tables that stop the walk -- this module's three PROTECT +foreign keys (`StudentCompetencyCriteriaStatus.criterion`, +`StudentCompetencyCriteriaGroupStatus.group`, `StudentCompetencyStatus.tag`) -- did not exist on +that branch. Every ProtectedError half of those transitive cases is asserted here instead, +alongside the succeeding (no status beneath the target) half, so each case is proven both ways. +""" +from datetime import datetime, timezone + +import pytest +from django.contrib.auth import get_user_model +from django.core.exceptions import ValidationError +from django.db import IntegrityError, transaction +from django.db.models import ProtectedError + +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyMasteryStatus, + CompetencyRuleProfile, + CompetencyTaxonomy, + MasteryStatus, + StudentCompetencyCriteriaGroupStatus, + StudentCompetencyCriteriaStatus, + StudentCompetencyStatus, +) +from openedx_tagging.models import ObjectTag, Tag + +pytestmark = pytest.mark.django_db + + +@pytest.fixture(name="competency_taxonomy") +def _competency_taxonomy() -> CompetencyTaxonomy: + """A CompetencyTaxonomy for use as a scope, and as the home taxonomy for `tag`.""" + return CompetencyTaxonomy.objects.create(name="Nursing", export_id="nursing-v1") + + +@pytest.fixture(name="tag") +def _tag(competency_taxonomy: CompetencyTaxonomy) -> Tag: + """A Tag, from `competency_taxonomy`, for use as the competency a criteria tree evaluates.""" + return Tag.objects.create(taxonomy=competency_taxonomy, value="Writing Poetry") + + +@pytest.fixture(name="object_tag") +def _object_tag(competency_taxonomy: CompetencyTaxonomy, tag: Tag) -> ObjectTag: + """An ObjectTag associating `tag` with a made-up content object, for use as a criterion's target.""" + return ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+p1", + taxonomy=competency_taxonomy, + tag=tag, + ) + + +@pytest.fixture(name="group") +def _group(tag: Tag) -> CompetencyCriteriaGroup: + """A root CompetencyCriteriaGroup for `tag`, for use as a criterion's parent group.""" + return CompetencyCriteriaGroup.objects.create(tag=tag) + + +@pytest.fixture(name="default_rule_profile") +def _default_rule_profile() -> CompetencyRuleProfile: + """The system-default CompetencyRuleProfile seeded by migration 0003.""" + return CompetencyRuleProfile.objects.get( + organization__isnull=True, + course__isnull=True, + competency_taxonomy__isnull=True, + ) + + +@pytest.fixture(name="user") +def _user(): + """ + Create a single learner for use in these tests. + + Deliberately unannotated: the user model is swappable, so this library must not + name a concrete one (edx-lint enforces that as `imported-auth-user`). + """ + return get_user_model().objects.create(username="learner") + + +@pytest.fixture(name="now") +def _now() -> datetime: + """A single UTC timestamp shared by writes in a test.""" + return datetime.now(timezone.utc) + + +@pytest.fixture(name="other_tag") +def _other_tag(competency_taxonomy: CompetencyTaxonomy) -> Tag: + """A second Tag, from the same taxonomy as `tag`, for a second competency-level status.""" + return Tag.objects.create(taxonomy=competency_taxonomy, value="Decimals") + + +@pytest.fixture(name="other_object_tag") +def _other_object_tag(competency_taxonomy: CompetencyTaxonomy, tag: Tag) -> ObjectTag: + """A second ObjectTag on `tag`, distinct from `object_tag`, for a second leaf criterion.""" + return ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+p2", + taxonomy=competency_taxonomy, + tag=tag, + ) + + +@pytest.fixture(name="criterion") +def _criterion( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> CompetencyCriterion: + """A leaf CompetencyCriterion under `group`, using the system-default rule profile.""" + return CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=default_rule_profile) + + +@pytest.fixture(name="child_group") +def _child_group(tag: Tag, group: CompetencyCriteriaGroup) -> CompetencyCriteriaGroup: + """A child CompetencyCriteriaGroup under `group`, for a tree the collector must walk two levels down.""" + return CompetencyCriteriaGroup.objects.create(tag=tag, parent=group) + + +@pytest.fixture(name="child_criterion") +def _child_criterion( + child_group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> CompetencyCriterion: + """A leaf CompetencyCriterion under `child_group`, for exercising a delete at depth.""" + return CompetencyCriterion.objects.create( + group=child_group, object_tag=object_tag, rule_profile=default_rule_profile + ) + + +# ============================================================================================== +# The lookup table and its rank ordering. +# ============================================================================================== + + +def test_seed_produces_three_rows_in_rank_order() -> None: + """ + The seed_competency_mastery_statuses data migration creates exactly the three + CompetencyMasteryStatus rows, with the pinned ids from MasteryStatus, and ids ascend in + rank (lowest to highest mastery). + """ + rows = list(CompetencyMasteryStatus.objects.order_by("id")) + assert [row.id for row in rows] == [ + MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED, + MasteryStatus.PARTIALLY_ATTEMPTED, + MasteryStatus.DEMONSTRATED, + ] + assert [row.status for row in rows] == [ + MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED.label, + MasteryStatus.PARTIALLY_ATTEMPTED.label, + MasteryStatus.DEMONSTRATED.label, + ] + + +def test_status_is_unique_on_lookup_table() -> None: + """ + CompetencyMasteryStatus.status is unique: a second row with a status string + that already exists raises IntegrityError. + """ + with pytest.raises(IntegrityError), transaction.atomic(): + CompetencyMasteryStatus.objects.create(status=MasteryStatus.DEMONSTRATED.label) + + +# ============================================================================================== +# The monotone conditional-update comparison, on the leaf level (where ADR-0004 Decision 1 has an +# automatic raise actually land) and on the top level (the backup's original coverage). +# ============================================================================================== + + +def test_conditional_raise_is_a_single_no_op_or_effective_update_on_leaf( + user, criterion: CompetencyCriterion, now: datetime, +) -> None: + """ + The monotone comparison works in a single statement on StudentCompetencyCriteriaStatus, the + leaf level. ADR-0004 Decision 1 writes the leaf synchronously with the learner's grade, so + this is where an automatic raise actually lands; the top-level equivalent below exercises the + same UPDATE shape one level up. + """ + scs = StudentCompetencyCriteriaStatus.objects.create( + user=user, + criterion=criterion, + status_id=MasteryStatus.DEMONSTRATED, + created=now, + modified=now, + ) + + # Already at the top rank: an attempted raise to a lower/equal rank changes nothing. + changed = StudentCompetencyCriteriaStatus.objects.filter( + user=user, criterion=criterion, status_id__lt=MasteryStatus.PARTIALLY_ATTEMPTED, + ).update(status_id=MasteryStatus.PARTIALLY_ATTEMPTED, modified=now) + assert changed == 0 + scs.refresh_from_db() + assert scs.status_id == MasteryStatus.DEMONSTRATED + + # Lower the stored status, then confirm the same shape raises it exactly once. + StudentCompetencyCriteriaStatus.objects.filter(pk=scs.pk).update(status_id=MasteryStatus.PARTIALLY_ATTEMPTED) + changed = StudentCompetencyCriteriaStatus.objects.filter( + user=user, criterion=criterion, status_id__lt=MasteryStatus.DEMONSTRATED, + ).update(status_id=MasteryStatus.DEMONSTRATED, modified=now) + assert changed == 1 + scs.refresh_from_db() + assert scs.status_id == MasteryStatus.DEMONSTRATED + + +def test_conditional_raise_is_a_single_no_op_or_effective_update(user, tag: Tag, now: datetime) -> None: + """ + The monotone comparison works in a single statement on StudentCompetencyStatus, the top + level: a conditional UPDATE guarded by status_id__lt is a no-op against an already-higher + status, and is the one write that takes effect when the stored status is lower. + """ + scs = StudentCompetencyStatus.objects.create( + user=user, + tag=tag, + status_id=MasteryStatus.DEMONSTRATED, + created=now, + modified=now, + ) + + # Already at the top rank: an attempted raise to a lower/equal rank changes nothing. + changed = StudentCompetencyStatus.objects.filter( + user=user, tag=tag, status_id__lt=MasteryStatus.PARTIALLY_ATTEMPTED, + ).update(status_id=MasteryStatus.PARTIALLY_ATTEMPTED, modified=now) + assert changed == 0 + scs.refresh_from_db() + assert scs.status_id == MasteryStatus.DEMONSTRATED + + # Lower the stored status, then confirm the same shape raises it exactly once. + StudentCompetencyStatus.objects.filter(pk=scs.pk).update(status_id=MasteryStatus.PARTIALLY_ATTEMPTED) + changed = StudentCompetencyStatus.objects.filter( + user=user, tag=tag, status_id__lt=MasteryStatus.DEMONSTRATED, + ).update(status_id=MasteryStatus.DEMONSTRATED, modified=now) + assert changed == 1 + scs.refresh_from_db() + assert scs.status_id == MasteryStatus.DEMONSTRATED + + +# ============================================================================================== +# The allow-list check constraint, which applies only to StudentCompetencyStatus, and its +# boundary: the other two models accept all three MasteryStatus values. +# ============================================================================================== + + +def test_attempted_not_demonstrated_rejected_on_create(user, tag: Tag, now: datetime) -> None: + """ + The allow-list constraint rejects AttemptedNotDemonstrated on a direct create(). + """ + with pytest.raises(IntegrityError), transaction.atomic(): + StudentCompetencyStatus.objects.create( + user=user, + tag=tag, + status_id=MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED, + created=now, + modified=now, + ) + + +def test_attempted_not_demonstrated_rejected_on_bulk_create(user, tag: Tag, now: datetime) -> None: + """ + The allow-list constraint rejects AttemptedNotDemonstrated on bulk_create(), which + bypasses Model.save() and so would otherwise skip any Python-side validation. + """ + with pytest.raises(IntegrityError), transaction.atomic(): + StudentCompetencyStatus.objects.bulk_create([ + StudentCompetencyStatus( + user=user, + tag=tag, + status_id=MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED, + created=now, + modified=now, + ) + ]) + + +def test_attempted_not_demonstrated_rejected_on_queryset_update(user, tag: Tag, now: datetime) -> None: + """ + The allow-list constraint also rejects AttemptedNotDemonstrated on QuerySet.update(), + the same write path the conditional raise above uses. + """ + scs = StudentCompetencyStatus.objects.create( + user=user, + tag=tag, + status_id=MasteryStatus.PARTIALLY_ATTEMPTED, + created=now, + modified=now, + ) + with pytest.raises(IntegrityError), transaction.atomic(): + StudentCompetencyStatus.objects.filter(pk=scs.pk).update( + status_id=MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED, + ) + + +def test_demonstrated_and_partially_attempted_both_accepted( + user, tag: Tag, other_tag: Tag, now: datetime, +) -> None: + """ + Both statuses the allow-list permits, Demonstrated and PartiallyAttempted, are + accepted on create(). + """ + demonstrated = StudentCompetencyStatus.objects.create( + user=user, + tag=tag, + status_id=MasteryStatus.DEMONSTRATED, + created=now, + modified=now, + ) + partially_attempted = StudentCompetencyStatus.objects.create( + user=user, + tag=other_tag, + status_id=MasteryStatus.PARTIALLY_ATTEMPTED, + created=now, + modified=now, + ) + assert demonstrated.status_id == MasteryStatus.DEMONSTRATED + assert partially_attempted.status_id == MasteryStatus.PARTIALLY_ATTEMPTED + + +def test_leaf_and_group_status_accept_attempted_not_demonstrated( + user, criterion: CompetencyCriterion, group: CompetencyCriteriaGroup, now: datetime, +) -> None: + """ + Unlike StudentCompetencyStatus, neither StudentCompetencyCriteriaStatus nor + StudentCompetencyCriteriaGroupStatus carries a check constraint on `status`: all three + MasteryStatus values, including AttemptedNotDemonstrated, are valid at the leaf and group + levels, because an in-progress state is meaningful there. The restriction to + PartiallyAttempted/Demonstrated is specific to the top-level competency status, which + represents overall demonstration, not an in-progress state. + """ + leaf = StudentCompetencyCriteriaStatus.objects.create( + user=user, + criterion=criterion, + status_id=MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED, + created=now, + modified=now, + ) + group_status = StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, + group=group, + status_id=MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED, + created=now, + modified=now, + ) + assert leaf.status_id == MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED + assert group_status.status_id == MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED + + +# ============================================================================================== +# One row per learner and node (ADR-0002 Decision 5 indexes 6, 7, and 8), one test per model. +# ============================================================================================== + + +def test_one_row_per_user_and_criterion_but_multiple_criteria_per_user( + user, criterion: CompetencyCriterion, other_object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + The (user, criterion) unique constraint rejects a second row for the same pair, but the + same learner may hold a status for a different criterion. + """ + # Same group as `criterion`, read off it rather than taking a separate `group` fixture + # argument, which would push this function over pylint's max-args. + other_criterion = CompetencyCriterion.objects.create( + group=criterion.group, object_tag=other_object_tag, rule_profile=default_rule_profile + ) + StudentCompetencyCriteriaStatus.objects.create( + user=user, + criterion=criterion, + status_id=MasteryStatus.PARTIALLY_ATTEMPTED, + created=now, + modified=now, + ) + with pytest.raises(IntegrityError), transaction.atomic(): + StudentCompetencyCriteriaStatus.objects.create( + user=user, + criterion=criterion, + status_id=MasteryStatus.DEMONSTRATED, + created=now, + modified=now, + ) + + # A different criterion for the same user is a different (user, criterion) pair, so it's allowed. + other = StudentCompetencyCriteriaStatus.objects.create( + user=user, + criterion=other_criterion, + status_id=MasteryStatus.DEMONSTRATED, + created=now, + modified=now, + ) + assert other.criterion_id == other_criterion.pk + + +def test_one_row_per_user_and_group_but_multiple_groups_per_user( + user, tag: Tag, group: CompetencyCriteriaGroup, now: datetime, +) -> None: + """ + The (user, group) unique constraint rejects a second row for the same pair, but the + same learner may hold a status for a different group. + """ + other_group = CompetencyCriteriaGroup.objects.create(tag=tag) + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, + group=group, + status_id=MasteryStatus.PARTIALLY_ATTEMPTED, + created=now, + modified=now, + ) + with pytest.raises(IntegrityError), transaction.atomic(): + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, + group=group, + status_id=MasteryStatus.DEMONSTRATED, + created=now, + modified=now, + ) + + # A different group for the same user is a different (user, group) pair, so it's allowed. + other = StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, + group=other_group, + status_id=MasteryStatus.DEMONSTRATED, + created=now, + modified=now, + ) + assert other.group_id == other_group.pk + + +def test_one_row_per_user_and_tag_but_multiple_tags_per_user( + user, tag: Tag, other_tag: Tag, now: datetime, +) -> None: + """ + The (user, tag) unique constraint rejects a second row for the same pair, but + the same learner may hold a status for a different tag. + """ + StudentCompetencyStatus.objects.create( + user=user, + tag=tag, + status_id=MasteryStatus.PARTIALLY_ATTEMPTED, + created=now, + modified=now, + ) + with pytest.raises(IntegrityError), transaction.atomic(): + StudentCompetencyStatus.objects.create( + user=user, + tag=tag, + status_id=MasteryStatus.DEMONSTRATED, + created=now, + modified=now, + ) + + # A different tag for the same user is a different (user, tag) pair, so it's allowed. + other = StudentCompetencyStatus.objects.create( + user=user, + tag=other_tag, + status_id=MasteryStatus.DEMONSTRATED, + created=now, + modified=now, + ) + assert other.tag_id == other_tag.pk + + +# ============================================================================================== +# created/modified: caller-supplied, required, and UTC-only. The field is defined identically +# (manual_date_time_field()) on all three models, so one model's coverage stands for all three. +# ============================================================================================== + + +def test_created_and_modified_are_required_and_must_be_utc(user, tag: Tag, now: datetime) -> None: + """ + created and modified are caller-supplied, not automatic: omitting either on create() + raises IntegrityError (NOT NULL, since there is no auto_now/auto_now_add default), and + passing a naive (non-UTC) datetime fails full_clean() with ValidationError, which is + the UTC validator manual_date_time_field() carries. + """ + with pytest.raises(IntegrityError), transaction.atomic(): + StudentCompetencyStatus.objects.create( + user=user, + tag=tag, + status_id=MasteryStatus.DEMONSTRATED, + ) + + naive_now = datetime.now() # deliberately naive, to trigger the UTC validator + scs = StudentCompetencyStatus( + user=user, + tag=tag, + status_id=MasteryStatus.DEMONSTRATED, + created=naive_now, + modified=now, + ) + with pytest.raises(ValidationError): + scs.full_clean() + + +def test_conditional_raise_can_carry_modified_without_touching_created(user, tag: Tag, now: datetime) -> None: + """ + A conditional raise can carry `modified` in the same UPDATE while leaving `created` + untouched, showing the mandated write path can keep `modified` honest without an + auto_now, which is why the field is caller-supplied rather than automatic. + """ + created_at = now + scs = StudentCompetencyStatus.objects.create( + user=user, + tag=tag, + status_id=MasteryStatus.PARTIALLY_ATTEMPTED, + created=created_at, + modified=created_at, + ) + + later = datetime.now(timezone.utc) + changed = StudentCompetencyStatus.objects.filter( + user=user, tag=tag, status_id__lt=MasteryStatus.DEMONSTRATED, + ).update(status_id=MasteryStatus.DEMONSTRATED, modified=later) + assert changed == 1 + + scs.refresh_from_db() + assert scs.status_id == MasteryStatus.DEMONSTRATED + assert scs.modified == later + assert scs.created == created_at + + +# ============================================================================================== +# No history package on any of the three learner status models (ADR-0003 Decision 5), unlike +# #641's criteria definition models. +# ============================================================================================== + + +def test_no_history_package_applied() -> None: + """ + None of the three learner status models has a `history` attribute. + + ADR-0003 Decision 1 gives django-simple-history to the three criteria definition models + (CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile) only. ADR-0003 Decision + 5 leaves how learner status history is retained undecided, so no history package is applied + to any of the three models here. + """ + assert not hasattr(StudentCompetencyCriteriaStatus, "history") + assert not hasattr(StudentCompetencyCriteriaGroupStatus, "history") + assert not hasattr(StudentCompetencyStatus, "history") + + +# ============================================================================================== +# Deletion. Each transitive case is proven both ways: ProtectedError when a status row exists +# somewhere beneath the target, and a full cascade of the criteria tree when none does. Across +# the tag/group/taxonomy cases, the blocking status row is deliberately placed at a different +# level each time (a competency status directly on the tag, a group status under a group, and a +# leaf status two levels below a taxonomy) so the suite as a whole exercises all three PROTECT +# foreign keys, not just one of them repeatedly. +# ============================================================================================== + + +def test_tag_delete_protected_by_competency_status_on_tag( + tag: Tag, group: CompetencyCriteriaGroup, criterion: CompetencyCriterion, user, now: datetime, +) -> None: + """ + Deleting a Tag raises ProtectedError when a StudentCompetencyStatus row references it + directly via `tag` (`on_delete=models.PROTECT`). This is the shallowest of the transitive + cases: the collector finds the blocking row on the tag itself, with no CASCADE hop needed + first. + """ + StudentCompetencyStatus.objects.create( + user=user, tag=tag, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + with pytest.raises(ProtectedError), transaction.atomic(): + tag.delete() + + assert Tag.objects.filter(pk=tag.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +def test_tag_delete_with_no_status_cascades_whole_criteria_tree( + tag: Tag, group: CompetencyCriteriaGroup, criterion: CompetencyCriterion, +) -> None: + """ + Deleting a Tag with no learner status beneath it succeeds and cascades away the whole + criteria tree hanging off it: Tag -> CompetencyCriteriaGroup.tag (CASCADE) -> + CompetencyCriterion.group (CASCADE). + """ + tag.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +def test_group_delete_at_depth_protected_by_leaf_status( + group: CompetencyCriteriaGroup, child_group: CompetencyCriteriaGroup, child_criterion: CompetencyCriterion, + user, now: datetime, +) -> None: + """ + Deleting a CompetencyCriteriaGroup raises ProtectedError when a + StudentCompetencyCriteriaStatus row exists on a criterion two levels below it: + `group` -> `child_group` via CompetencyCriteriaGroup.parent (CASCADE), then `child_group` -> + `child_criterion` via CompetencyCriterion.group (CASCADE), then `child_criterion` -> the + status row via StudentCompetencyCriteriaStatus.criterion (PROTECT). Deleting `group` makes + Django's collector walk both CASCADE hops before it reaches the PROTECT that stops it. + """ + StudentCompetencyCriteriaStatus.objects.create( + user=user, criterion=child_criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + with pytest.raises(ProtectedError), transaction.atomic(): + group.delete() + + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=child_group.pk).exists() + assert CompetencyCriterion.objects.filter(pk=child_criterion.pk).exists() + + +def test_group_delete_at_depth_with_no_status_cascades_descendants( + group: CompetencyCriteriaGroup, child_group: CompetencyCriteriaGroup, child_criterion: CompetencyCriterion, +) -> None: + """ + Deleting a CompetencyCriteriaGroup with no status rows beneath it succeeds and cascades away + its child group and that child's criterion, via the same two CASCADE hops + (CompetencyCriteriaGroup.parent, then CompetencyCriterion.group) that the protected case + above walks before hitting a PROTECT. + """ + group.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=child_group.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=child_criterion.pk).exists() + + +def test_object_tag_delete_protected_by_leaf_status( + object_tag: ObjectTag, criterion: CompetencyCriterion, user, now: datetime, +) -> None: + """ + Deleting an ObjectTag raises ProtectedError when a StudentCompetencyCriteriaStatus row + references its criterion, reached transitively: ObjectTag -> CompetencyCriterion.object_tag + (CASCADE) -> the status row via StudentCompetencyCriteriaStatus.criterion (PROTECT). + """ + StudentCompetencyCriteriaStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + with pytest.raises(ProtectedError), transaction.atomic(): + object_tag.delete() + + assert ObjectTag.objects.filter(pk=object_tag.pk).exists() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +def test_object_tag_delete_with_no_status_cascades_criterion( + object_tag: ObjectTag, criterion: CompetencyCriterion, +) -> None: + """ + Deleting an ObjectTag with no learner status on its criterion succeeds and cascades that + criterion away via CompetencyCriterion.object_tag (CASCADE). + """ + object_tag.delete() + + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +def test_taxonomy_delete_protected_by_group_status( + competency_taxonomy: CompetencyTaxonomy, tag: Tag, group: CompetencyCriteriaGroup, user, now: datetime, +) -> None: + """ + Deleting a CompetencyTaxonomy raises ProtectedError when a + StudentCompetencyCriteriaGroupStatus row exists on a group under one of its tags, reached + transitively: CompetencyTaxonomy -> Tag via Tag.taxonomy (CASCADE, in openedx_tagging) -> + `group` via CompetencyCriteriaGroup.tag (CASCADE) -> the status row via + StudentCompetencyCriteriaGroupStatus.group (PROTECT). + """ + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=group, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + with pytest.raises(ProtectedError), transaction.atomic(): + competency_taxonomy.delete() + + assert CompetencyTaxonomy.objects.filter(pk=competency_taxonomy.pk).exists() + assert Tag.objects.filter(pk=tag.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + + +def test_taxonomy_delete_with_no_status_cascades_every_tag_and_its_criteria( + competency_taxonomy: CompetencyTaxonomy, tag: Tag, group: CompetencyCriteriaGroup, + criterion: CompetencyCriterion, +) -> None: + """ + Deleting a CompetencyTaxonomy with no learner status beneath any of its tags succeeds and + cascades away every tag and its criteria tree: CompetencyTaxonomy -> Tag (CASCADE) -> + CompetencyCriteriaGroup.tag (CASCADE) -> CompetencyCriterion.group (CASCADE). This is the + same chain case 13 (`test_tag_delete_with_no_status_cascades_whole_criteria_tree`) proves, + reached transitively from one level higher. + """ + competency_taxonomy.delete() + + assert not Tag.objects.filter(pk=tag.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +def test_user_delete_removes_status_across_all_three_models( + user, tag: Tag, group: CompetencyCriteriaGroup, criterion: CompetencyCriterion, now: datetime, +) -> None: + """ + Deleting a User cascades to that learner's status rows in all three models + (`on_delete=models.CASCADE` on each model's `user` foreign key), while leaving the + competency definitions themselves untouched: the status is a derived fact about the + learner, so it goes when they do, but the criteria tree it was measuring does not. + """ + leaf = StudentCompetencyCriteriaStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + group_status = StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=group, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + competency_status = StudentCompetencyStatus.objects.create( + user=user, tag=tag, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + user.delete() + + assert not StudentCompetencyCriteriaStatus.objects.filter(pk=leaf.pk).exists() + assert not StudentCompetencyCriteriaGroupStatus.objects.filter(pk=group_status.pk).exists() + assert not StudentCompetencyStatus.objects.filter(pk=competency_status.pk).exists() + assert Tag.objects.filter(pk=tag.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists()