From a6f3b0444bd7c0b0c4c42381474e7484ccb294bd Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Tue, 1 Sep 2026 16:22:52 -0400 Subject: [PATCH 1/9] 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 ca484f4d5e4028973c4e4f6be13c989e1d7462fe Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 2 Sep 2026 10:45:40 -0400 Subject: [PATCH 2/9] 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 acbfbe4bac72bb839051326d68eee5a50cbc8de5 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 2 Sep 2026 10:45:40 -0400 Subject: [PATCH 3/9] 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 4a62ab30bf8c97b0017a55ed7d84591278a9ac07 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 2 Sep 2026 15:34:15 -0400 Subject: [PATCH 4/9] 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 233b85707f28fee796d6b9e4037d6816248b6164 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Fri, 4 Sep 2026 10:51:30 -0400 Subject: [PATCH 5/9] refactor: align criteria models with the 2026-09-03 revision of #641 Four changes, all from #641's revision. CompetencyRuleProfile.competency_taxonomy becomes CASCADE, making the split five CASCADE and four PROTECT. The reason is a requirement rather than a mechanism: a rule profile must never be why a taxonomy delete fails. Once taxonomy-scoped profiles exist, deleting a taxonomy has to be blocked only when learner data is connected to it, and that check belongs in Python at the application layer, the way #655 settled it for every other record type. PROTECT would push the decision into the database, which cannot tell the two cases apart. Nothing changes behaviorally in MVP, because the only profile is the system default and its three scope columns are all null. openedx_content and openedx_catalog become independent siblings in the src_layering contract rather than separate ranks. A layers contract is a strict total order, so ranking them asserted both that openedx_content may import openedx_catalog and that openedx_catalog may never import openedx_content. src/openedx_catalog/ARCHITECTURE.md records that direction as explicitly undecided, so the sibling form, which forbids imports both ways, asserts only what is settled. The Meta.db_table override is dropped, so the leaf table is Django's default openedx_learning_competencycriterion. ADR-0002 Decision 4's heading names a domain concept rather than instructing a rename, and no model anywhere in src/ overrides db_table. The competency_taxonomy delete test becomes a cascade test to match. Refs #641 Co-Authored-By: Claude Opus 5 (1M context) --- .importlinter | 16 ++++---- .../applets/cbe/models/criteria.py | 37 ++++++++++++------- .../migrations/0002_competency_criteria.py | 3 +- .../applets/cbe/test_criteria_deletion.py | 33 +++++++---------- 4 files changed, 48 insertions(+), 41 deletions(-) diff --git a/.importlinter b/.importlinter index 844575b91..9dcea9f55 100644 --- a/.importlinter +++ b/.importlinter @@ -24,13 +24,15 @@ layers = # particular, openedx_tagging must never know that CBE exists. openedx_learning - # 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 + # Content (authoring-side models and APIs) and Catalog (CatalogCourse/CourseRun) as + # independent siblings, not one above the other: src/openedx_catalog/ARCHITECTURE.md records + # the direction between them as explicitly undecided ("Direction of this relationship TBD." + # on the Catalog <-> Content edge). A `layers` contract is a strict total order, so ranking + # one above the other would assert a direction nobody has chosen. Listing them side by side + # with `|` instead forbids imports both ways, asserting only what's actually settled: neither + # depends on the other yet. CompetencyCriteriaGroup and CompetencyRuleProfile + # (openedx_learning) scope to a CourseRun, so both still need to sit below openedx_learning. + openedx_content | openedx_catalog # Tagging is very simple & fundamental. Should probably not depend on any other Django apps. openedx_tagging diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py index 265ec4431..531dc1a20 100644 --- a/src/openedx_learning/applets/cbe/models/criteria.py +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -5,19 +5,25 @@ :ref:`openedx-learning-adr-0003` for why these three models (and not CompetencyTaxonomy) carry ``django-simple-history`` tracking. -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 +Five of the nine foreign keys here are ``on_delete=models.CASCADE``: ``CompetencyCriteriaGroup.parent``, +``CompetencyCriteriaGroup.tag``, ``CompetencyCriterion.group``, ``CompetencyCriterion.object_tag``, and +``CompetencyRuleProfile.competency_taxonomy``. The other four stay ``models.PROTECT``: +``CompetencyCriterion.rule_profile``, ``CompetencyCriteriaGroup.course``, +``CompetencyRuleProfile.course``, and ``CompetencyRuleProfile.organization``. + +The first 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. +``competency_taxonomy`` is CASCADE for a different reason: a rule profile must never be the +reason a taxonomy delete fails. Once taxonomy-scoped profiles exist, deleting a taxonomy has to +be blocked only when learner data is connected to it, and that check belongs in Python at the +application layer, the same way #655 settled it for every other record type. ``PROTECT`` would +push that decision into the database, which cannot tell the two cases apart. + 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 @@ -28,12 +34,11 @@ 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 +The other three: 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. +them (``remove_organization()``). """ from __future__ import annotations @@ -245,7 +250,7 @@ class CompetencyRuleProfile(models.Model): CompetencyTaxonomy, null=True, blank=True, - on_delete=models.PROTECT, + on_delete=models.CASCADE, related_name="rule_profiles", help_text=_("The competency taxonomy this profile is scoped to, if any."), ) @@ -414,7 +419,13 @@ class CompetencyCriterion(models.Model): history = HistoricalRecords() class Meta: - db_table = "openedx_learning_competencycriteria" + # No db_table override, so the table is Django's default, + # openedx_learning_competencycriterion. ADR-0002 Decision 4's heading reads + # "CompetencyCriterion concept (CompetencyCriteria database table)", which names the + # domain concept the way every other heading in that ADR does rather than instructing a + # rename. No model anywhere in src/ overrides db_table, so every table in this library + # is _. + # # 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 diff --git a/src/openedx_learning/migrations/0002_competency_criteria.py b/src/openedx_learning/migrations/0002_competency_criteria.py index fcdd2c817..6b2e4229b 100644 --- a/src/openedx_learning/migrations/0002_competency_criteria.py +++ b/src/openedx_learning/migrations/0002_competency_criteria.py @@ -50,7 +50,7 @@ class Migration(migrations.Migration): ('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')), + ('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.CASCADE, 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')), ], @@ -67,7 +67,6 @@ class Migration(migrations.Migration): ('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', }, diff --git a/tests/openedx_learning/applets/cbe/test_criteria_deletion.py b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py index 97d6c93bc..55228f6da 100644 --- a/tests/openedx_learning/applets/cbe/test_criteria_deletion.py +++ b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py @@ -1,7 +1,7 @@ """ Delete-behavior tests for CompetencyCriteriaGroup, CompetencyRuleProfile, and CompetencyCriterion. -Four of these nine foreign keys are `on_delete=models.CASCADE` and five are `models.PROTECT`; see +Five of these nine foreign keys are `on_delete=models.CASCADE` and four are `models.PROTECT`; see the module docstring in `openedx_learning.applets.cbe.models.criteria` for which is which and why. """ import pytest @@ -85,15 +85,15 @@ def _default_rule_profile() -> CompetencyRuleProfile: # ============================================================================================== -# One test per foreign key. The five that stayed PROTECT assert ProtectedError and inspect +# One test per foreign key. The four 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. +# The five that are 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. # ============================================================================================== @@ -176,26 +176,21 @@ def test_rule_profile_course_protect(course_run: CourseRun) -> None: 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: +def test_rule_profile_competency_taxonomy_cascade(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: 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. + Deleting a CompetencyTaxonomy cascades to any CompetencyRuleProfile referencing it via + `competency_taxonomy`: the delete succeeds and the profile row is gone too. Nothing changes + behaviorally in this MVP, since only the all-null system-default profile exists, so there is + no taxonomy-scoped profile for this CASCADE to ever actually remove yet. """ profile = CompetencyRuleProfile.objects.create( competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD ) + assert CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() - with pytest.raises(ProtectedError) as exc_info: - competency_taxonomy.delete() + competency_taxonomy.delete() - protected = exc_info.value.protected_objects - assert any(isinstance(obj, CompetencyRuleProfile) and obj.pk == profile.pk for obj in protected) + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() def test_criterion_group_cascade( From 8f484e0bbc0e41be3787d133809ea0dc6fc99ffd Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Fri, 4 Sep 2026 12:46:35 -0400 Subject: [PATCH 6/9] refactor: define rule payload shapes as attrs specs, validate on save Replaces the hand-rolled key-walking in validate_rule_payload with one attrs class per rule type, using the modern `from attrs import define, field` style already used in openedx_tagging and openedx_content. attrs is already a declared dependency, so nothing changes in requirements. The spec class is now the definition of the shape: constructing it does the checking, and the expected key set is derived from it via attrs.fields() rather than repeated in a literal. Adding MasteryLevel later is one class plus one registry entry. The field validators stay hand-written rather than using attrs.validators.in_(), because that helper's default message dumps the whole Attribute repr into the error, which a course author would see in the Django admin. Key errors are raised before construction for the same reason: Python's own TypeError names the offending key but leaks "GradeRule.__init__()" along with it. validate_rule_payload is now also called from save() on both models. clean() is reached only via full_clean(), so objects.create() and instance.save() previously bypassed payload validation entirely; this closes both. QuerySet.update(), bulk_create() and DRF serializers remain uncovered, because none of them builds or saves a model instance, and both model docstrings say so rather than implying more. CourseRun.save() is the existing precedent in this repo for validating in save(). One consequence, split rather than papered over: a criterion with rule_type_override set and no payload now raises ValidationError from save() before the check constraint sees it, so that case moves out of test_criterion_profile_xor_override_constraint into its own test. The other three invalid states still reach the constraint and still raise IntegrityError. The seed data migration is unaffected: apps.get_model() returns a historical model that does not carry the custom save(). Refs #641 Co-Authored-By: Claude Opus 5 (1M context) --- .../applets/cbe/models/criteria.py | 136 ++++++++++++++---- .../applets/cbe/test_criteria_models.py | 73 +++++++++- 2 files changed, 181 insertions(+), 28 deletions(-) diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py index 531dc1a20..6c85ffe2c 100644 --- a/src/openedx_learning/applets/cbe/models/criteria.py +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -24,10 +24,11 @@ application layer, the same way #655 settled it for every other record type. ``PROTECT`` would push that decision into the database, which cannot tell the two cases apart. -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. +This is not a relaxation of ADR-0002 Decision 7: the four tree links above (``parent``, ``tag``, +``group``, ``object_tag``) 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`` @@ -44,6 +45,7 @@ from typing import Any +from attrs import Attribute, define, field, fields from django.core.exceptions import ValidationError from django.db import models from django.db.models import F, Q, Value @@ -83,6 +85,58 @@ class LogicOperator(models.TextChoices): OR = "OR", _("Or") +def _validate_op(_instance: object, _attribute: Attribute, value: object) -> None: + """Reject an 'op' outside the Grade rule's allowed comparison operators.""" + if value not in {"gte", "lte", "eq"}: + raise ValueError(_("The 'op' in a 'Grade' rule_payload must be one of: gte, lte, eq.")) + + +def _validate_grade_value(_instance: object, _attribute: Attribute, value: object) -> None: + """Reject a Grade rule's 'value' unless it's a non-boolean number in [0.0, 1.0].""" + # 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 TypeError(_("The 'value' in a 'Grade' rule_payload must be a number, not a boolean.")) + if not 0.0 <= value <= 1.0: + raise ValueError( + _( + "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} + ) + + +def _validate_scale(_instance: object, _attribute: Attribute, value: object) -> None: + """Reject a Grade rule's 'scale' unless it's exactly 'percent'.""" + if value != "percent": + raise ValueError(_("The 'scale' in a 'Grade' rule_payload must be 'percent'.")) + + +@define(frozen=True, kw_only=True) +class GradeRule: + """ + The rule_payload shape for RuleType.GRADE, per ADR-0002 Decision 3. + + Constructing one *is* the validation: kw_only means an unknown key raises TypeError + ("unexpected keyword argument") and a missing key raises TypeError ("missing ... required + keyword-only argument") from Python's own call handling, and a present-but-bad value raises + ValueError or TypeError from the field validators below. Frozen because this describes a + fixed spec and is never mutated after construction. + """ + + op: str = field(validator=_validate_op) + value: float = field(validator=_validate_grade_value) + scale: str = field(validator=_validate_scale) + + +# One spec class per RuleType with a defined rule_payload shape. Add a class and an entry here to +# support a new rule type. RuleType.VIEW and RuleType.MASTERY_LEVEL have no entry, so they keep +# being rejected by validate_rule_payload as not supported yet. +_RULE_PAYLOAD_SPECS: dict[str, type] = { + RuleType.GRADE: GradeRule, +} + + def validate_rule_payload(rule_type: str, payload: Any) -> None: """ Validate ``payload`` against the shape ADR-0002 Decision 3 defines for ``rule_type``. @@ -92,38 +146,46 @@ def validate_rule_payload(rule_type: str, payload: Any) -> None: payload contract exists for them yet. Raises ``django.core.exceptions.ValidationError`` on any mismatch; never returns a value. """ - if rule_type != RuleType.GRADE: + spec_class = _RULE_PAYLOAD_SPECS.get(rule_type) + if spec_class is None: raise ValidationError( _("Rule type '%(rule_type)s' is not supported yet; only 'Grade' has a defined rule_payload shape.") % {"rule_type": rule_type} ) + # Checked separately, before construction: `spec_class(**payload)` on a non-dict payload + # (e.g. a list) raises "argument after ** must be a mapping", which is a confusing message to + # surface to an author. 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: + # Also checked separately, before construction, rather than left to Python's own kw_only + # TypeError: that TypeError's text is "GradeRule.__init__() missing/got an unexpected keyword + # argument ...", a Python traceback fragment that leaks an internal class name to whoever + # edits this payload (a course author, via the admin). Deriving the expected keys from the + # spec class itself keeps that class the single source of truth for the key set, while owning + # the message in our own domain language instead of Python's. + expected_keys = {attr.name for attr in fields(spec_class)} + missing_keys = sorted(expected_keys - payload.keys()) + unexpected_keys = sorted(payload.keys() - expected_keys) + if missing_keys or unexpected_keys: + problems = [] + if missing_keys: + problems.append(_("missing %(keys)s") % {"keys": ", ".join(missing_keys)}) + if unexpected_keys: + problems.append(_("unexpected %(keys)s") % {"keys": ", ".join(unexpected_keys)}) raise ValidationError( - _("A 'Grade' rule_payload must have exactly these keys, no more and no fewer: op, value, scale.") + _("A 'Grade' rule_payload has the wrong keys: %(problems)s.") + % {"problems": "; ".join(str(problem) for problem in problems)} ) - 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'.")) + # Past the key check above, this can only fail on a value a field validator rejects. + try: + spec_class(**payload) + except (TypeError, ValueError) as exc: + # Surface the underlying message (our custom validators' text) rather than replacing it + # with something generic: that message is what full_clean() or save() surfaces to an + # admin or API caller. + raise ValidationError(str(exc)) from exc class CompetencyCriteriaGroup(models.Model): @@ -219,6 +281,12 @@ class CompetencyRuleProfile(models.Model): cover a bulk ``QuerySet.update()``, since that path never loads or constructs a model instance at all. + ``rule_payload``'s shape (see :func:`validate_rule_payload`) is likewise validated from both + ``clean()`` and ``save()``, so ``objects.create()`` and a plain ``instance.save()`` are + covered without a caller needing to remember ``full_clean()``. A bulk ``QuerySet.update()``, + ``bulk_create()``, and a DRF serializer that writes straight to the database are NOT covered: + none of them build or save a model instance, so neither ``clean()`` nor ``save()`` ever runs. + .. no_pii: """ @@ -367,8 +435,9 @@ def clean(self): validate_rule_payload(self.rule_type, self.rule_payload) def save(self, *args, **kwargs): - """Persist this profile, after re-checking scope immutability.""" + """Persist this profile, after re-checking scope immutability and the rule_payload shape.""" self._check_scope_immutable() + validate_rule_payload(self.rule_type, self.rule_payload) super().save(*args, **kwargs) self.loaded_scope = (self.organization_id, self.course_id, self.competency_taxonomy_id) @@ -386,6 +455,13 @@ class CompetencyCriterion(models.Model): and is never re-resolved dynamically. Do not add a property, manager method, or other helper that recomputes it; that would contradict the ADR. + When ``rule_type_override`` is set, its ``rule_payload_override``'s shape (see + :func:`validate_rule_payload`) is validated from both ``clean()`` and ``save()``, so + ``objects.create()`` and a plain ``instance.save()`` are covered without a caller needing to + remember ``full_clean()``. A bulk ``QuerySet.update()``, ``bulk_create()``, and a DRF + serializer that writes straight to the database are NOT covered: none of them build or save a + model instance, so neither ``clean()`` nor ``save()`` ever runs. + .. no_pii: """ @@ -459,3 +535,9 @@ def clean(self): super().clean() if self.rule_type_override is not None: validate_rule_payload(self.rule_type_override, self.rule_payload_override) + + def save(self, *args, **kwargs): + """Persist this criterion, after re-checking the override rule_payload's shape, if set.""" + if self.rule_type_override is not None: + validate_rule_payload(self.rule_type_override, self.rule_payload_override) + super().save(*args, **kwargs) diff --git a/tests/openedx_learning/applets/cbe/test_criteria_models.py b/tests/openedx_learning/applets/cbe/test_criteria_models.py index eade547c4..7d6cc4420 100644 --- a/tests/openedx_learning/applets/cbe/test_criteria_models.py +++ b/tests/openedx_learning/applets/cbe/test_criteria_models.py @@ -220,6 +220,12 @@ def test_criterion_profile_xor_override_constraint( """ 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. + + Covers the three invalid states that reach the database's check constraint: both set, neither + set, and only rule_payload_override set. The fourth invalid state, only rule_type_override + set, is caught earlier by save()'s own validation instead and raises ValidationError before + the database is ever touched; see test_criterion_save_validates_override_payload_before_constraint + below for that case, and why it raises a different exception type than these three. """ CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=default_rule_profile) CompetencyCriterion.objects.create( @@ -233,7 +239,6 @@ def test_criterion_profile_xor_override_constraint( "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: @@ -242,6 +247,25 @@ def test_criterion_profile_xor_override_constraint( CompetencyCriterion.objects.create(group=group, object_tag=object_tag, **kwargs) +def test_criterion_save_validates_override_payload_before_constraint( + group: CompetencyCriteriaGroup, object_tag: ObjectTag +) -> None: + """ + Setting only rule_type_override, leaving rule_payload_override null, is caught by save()'s + own validation before it ever reaches the database: save() validates rule_payload_override's + shape whenever rule_type_override is set, and None is not a valid shape for any rule type, so + this raises ValidationError. The database's check constraint would also reject this same row, + for the same underlying reason (an override with no real payload), but save() never lets it + get there. This is why two similar-looking invalid override states raise different exception + types: this one is caught by save()'s validate_rule_payload call, while the other three (see + test_criterion_profile_xor_override_constraint above) reach the database's check constraint, + because the payload save() inspects for them is either valid or, when rule_type_override + itself is null, not inspected at all. + """ + with pytest.raises(ValidationError): + CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_type_override=RuleType.GRADE) + + @pytest.mark.parametrize("rule_type, payload", _INVALID_GRADE_PAYLOADS) def test_rule_profile_full_clean_rejects_invalid_payload(rule_type: str, payload: object) -> None: """ @@ -255,6 +279,53 @@ def test_rule_profile_full_clean_rejects_invalid_payload(rule_type: str, payload profile.full_clean() +def test_rule_profile_full_clean_value_message_names_the_fraction_convention(organization: Organization) -> None: + """ + full_clean()'s error for a rule_payload 'value' given on a 0-100 scale (e.g. 80) names the + 0.0-1.0 fraction convention, not attrs' generic default message for a failed validator (which + would say nothing about fractions or percentages) and not a Python traceback fragment. + Guards against exactly the message-quality regression a naive attrs implementation of + validate_rule_payload could introduce silently, since every other invalid-payload test here + only asserts the exception type. + """ + profile = CompetencyRuleProfile( + organization=organization, rule_type=RuleType.GRADE, rule_payload={"op": "gte", "value": 80, "scale": "percent"} + ) + with pytest.raises(ValidationError) as exc_info: + profile.full_clean() + + message = " ".join(exc_info.value.messages) + assert "fraction between 0.0 and 1.0" in message + # Must not leak the attrs spec class's name or any Python call-mechanics fragment: a course + # author editing this payload in the admin should never see "GradeRule.__init__()". + assert "__init__" not in message + assert "GradeRule" not in message + + +def test_rule_profile_full_clean_extra_key_message_names_the_key(organization: Organization) -> None: + """ + full_clean()'s error for an unrecognized rule_payload key names that key in our own domain + language (e.g. "unexpected extra"), not attrs' generic default message for a failed validator + and not Python's own kw_only TypeError text ("GradeRule.__init__() got an unexpected keyword + argument 'extra'"), which leaks the internal spec class's name to a course author editing + this payload in the admin. Guards against exactly that regression, which a test asserting + only that the key name appears in the message would not catch, since the leaky Python message + also contains the key name. + """ + profile = CompetencyRuleProfile( + organization=organization, + rule_type=RuleType.GRADE, + rule_payload={**_GRADE_PAYLOAD, "extra": 1}, + ) + with pytest.raises(ValidationError) as exc_info: + profile.full_clean() + + message = " ".join(exc_info.value.messages) + assert "extra" in message + assert "__init__" not in message + assert "GradeRule" not in message + + @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 From 83b9f825c9d2999a629029a3e50f29e7d055b7a6 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Sat, 5 Sep 2026 10:41:24 -0400 Subject: [PATCH 7/9] chore: register simple_history and layer openedx_catalog Add simple_history to INSTALLED_APPS in the test and dev settings. The CBE models declare HistoricalRecords(), and while the historical models are built under openedx_learning's own app label and so work without the entry, its absence breaks SimpleHistoryAdmin's history views, its template tag libraries and the populate_history/clean_old_history/clean_duplicate_history commands. The package ships no AppConfig and registers no system check, so nothing warns. Rank openedx_content above openedx_catalog in the src_layering contract rather than making them independent siblings. The sibling form forbids imports in both directions, including the one 0007-pathway-catalog-content-split.rst requires: "openedx_content knows about openedx_catalog, never the reverse." Ranking asserts only the settled half, that catalog never reaches up into content, and does not have to be loosened when pathway content lands. Co-Authored-By: Claude Opus 5 (1M context) --- .importlinter | 19 ++++++++++--------- projects/dev.py | 6 ++++++ test_settings.py | 5 +++++ 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/.importlinter b/.importlinter index 9dcea9f55..107d4c428 100644 --- a/.importlinter +++ b/.importlinter @@ -24,15 +24,16 @@ layers = # particular, openedx_tagging must never know that CBE exists. openedx_learning - # Content (authoring-side models and APIs) and Catalog (CatalogCourse/CourseRun) as - # independent siblings, not one above the other: src/openedx_catalog/ARCHITECTURE.md records - # the direction between them as explicitly undecided ("Direction of this relationship TBD." - # on the Catalog <-> Content edge). A `layers` contract is a strict total order, so ranking - # one above the other would assert a direction nobody has chosen. Listing them side by side - # with `|` instead forbids imports both ways, asserting only what's actually settled: neither - # depends on the other yet. CompetencyCriteriaGroup and CompetencyRuleProfile - # (openedx_learning) scope to a CourseRun, so both still need to sit below openedx_learning. - openedx_content | openedx_catalog + # Content (authoring-side models and APIs) above Catalog (CatalogCourse/CourseRun): + # docs/openedx_learning/decisions/0007-pathway-catalog-content-split.rst ("Dependency + # direction: openedx_content knows about openedx_catalog, never the reverse") settles this + # direction, even though that ADR is still Draft rather than Accepted. A `layers` contract is + # a strict total order, and ranking them asserts only the half nobody disputes -- catalog + # never reaches up into content -- while still allowing the direction that ADR already + # depends on. CompetencyCriteriaGroup and CompetencyRuleProfile (openedx_learning) scope to a + # CourseRun, so both still need to sit below openedx_learning. + openedx_content + openedx_catalog # Tagging is very simple & fundamental. Should probably not depend on any other Django apps. openedx_tagging diff --git a/projects/dev.py b/projects/dev.py index 28348acab..c3682b504 100644 --- a/projects/dev.py +++ b/projects/dev.py @@ -37,6 +37,12 @@ # Open edX Organizations (dependency for openedx_catalog) "organizations", + # django-simple-history: registers its template tag libraries and admin integration + # (SimpleHistoryAdmin) and its management commands (populate_history, clean_old_history, + # clean_duplicate_history). HistoricalRecords() works without this app installed, but nothing + # else it provides does, and the package ships no AppConfig or system check to warn you. + "simple_history", + # Our Apps "openedx_catalog", "openedx_learning", diff --git a/test_settings.py b/test_settings.py index be0a84904..a0b4a2661 100644 --- a/test_settings.py +++ b/test_settings.py @@ -54,6 +54,11 @@ def root(*args): "organizations", # django-rules based authorization 'rules.apps.AutodiscoverRulesConfig', + # django-simple-history: registers its template tag libraries and admin integration + # (SimpleHistoryAdmin) and its management commands (populate_history, clean_old_history, + # clean_duplicate_history). HistoricalRecords() works without this app installed, but nothing + # else it provides does, and the package ships no AppConfig or system check to warn you. + "simple_history", # Our own apps "openedx_tagging", "openedx_content", From 091724ef6e8240f39f5d327f37b1dc66e372a932 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Sat, 5 Sep 2026 10:41:39 -0400 Subject: [PATCH 8/9] fix: free an archived rule profile's scope and correct on_delete values scope_code becomes an ordinary column written in save(), null when the profile is archived, keeping the plain UniqueConstraint and adding a CheckConstraint tying the two. This fixes three defects. An archived profile used to occupy its scope's unique slot forever, so no replacement could ever be created for that scope; SQL never treats two NULLs as equal, so archived rows now share a scope freely while exactly one live row holds it, identically on MySQL and SQLite. A database GeneratedField was also rewritten whenever Django's collector nulled a nullable scope foreign key before deleting, which it does on any backend where can_defer_constraint_checks is false, colliding with the seeded default row on MySQL while passing on SQLite. A plain column is not rewritten by that update. It also avoids Django never populating a GeneratedField in memory on MySQL. Two on_delete values change, per ADR-0002 Decision 7 as amended by b5fae6b. CompetencyRuleProfile.course becomes CASCADE, which that amendment requires when it says a profile is deleted with "a taxonomy or course" it is scoped to. CompetencyCriteriaGroup.course becomes CASCADE for the same stated reason: a course is only hard-deleted once nothing beneath it needs protecting, so a course-scoped criteria tree is safe to remove with it rather than blocking the delete. Both deviate from #641, which lists them as PROTECT. Drop the loaded_scope cache and the from_db() override; _check_scope_immutable() now always reads the persisted scope, on self._state.db so a non-default alias is not silently skipped. save() calls full_clean() on both models instead of duplicating a hand-picked validation list that could drift from clean(). Move RuleType, the payload spec classes and the parser to rule_payloads.py, so the JSON schema is not trapped behind a module importing five models, and have it return the frozen GradeRule rather than discarding it. Both models derive their choices from the payload-spec registry, so a rule type can never be offered to an author and then rejected on save. Co-Authored-By: Claude Opus 5 (1M context) --- .../applets/cbe/models/__init__.py | 10 +- .../applets/cbe/models/competency_taxonomy.py | 4 +- .../applets/cbe/models/criteria.py | 411 +++++++----------- .../applets/cbe/rule_payloads.py | 152 +++++++ .../migrations/0002_competency_criteria.py | 32 +- .../0003_seed_default_rule_profile.py | 10 + 6 files changed, 338 insertions(+), 281 deletions(-) create mode 100644 src/openedx_learning/applets/cbe/rule_payloads.py diff --git a/src/openedx_learning/applets/cbe/models/__init__.py b/src/openedx_learning/applets/cbe/models/__init__.py index 9d71edfa4..997f57d57 100644 --- a/src/openedx_learning/applets/cbe/models/__init__.py +++ b/src/openedx_learning/applets/cbe/models/__init__.py @@ -3,14 +3,7 @@ """ from .competency_taxonomy import CompetencyTaxonomy -from .criteria import ( - CompetencyCriteriaGroup, - CompetencyCriterion, - CompetencyRuleProfile, - LogicOperator, - RuleType, - validate_rule_payload, -) +from .criteria import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, LogicOperator, RuleType __all__ = [ "CompetencyTaxonomy", @@ -19,5 +12,4 @@ "CompetencyRuleProfile", "LogicOperator", "RuleType", - "validate_rule_payload", ] diff --git a/src/openedx_learning/applets/cbe/models/competency_taxonomy.py b/src/openedx_learning/applets/cbe/models/competency_taxonomy.py index a0623fa6e..5eea3f8b7 100644 --- a/src/openedx_learning/applets/cbe/models/competency_taxonomy.py +++ b/src/openedx_learning/applets/cbe/models/competency_taxonomy.py @@ -45,9 +45,7 @@ class CompetencyTaxonomy(Taxonomy): "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." + "weakened by an organization." ), ) diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py index 6c85ffe2c..06b842e25 100644 --- a/src/openedx_learning/applets/cbe/models/criteria.py +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -5,51 +5,46 @@ :ref:`openedx-learning-adr-0003` for why these three models (and not CompetencyTaxonomy) carry ``django-simple-history`` tracking. -Five of the nine foreign keys here are ``on_delete=models.CASCADE``: ``CompetencyCriteriaGroup.parent``, -``CompetencyCriteriaGroup.tag``, ``CompetencyCriterion.group``, ``CompetencyCriterion.object_tag``, and -``CompetencyRuleProfile.competency_taxonomy``. The other four stay ``models.PROTECT``: -``CompetencyCriterion.rule_profile``, ``CompetencyCriteriaGroup.course``, -``CompetencyRuleProfile.course``, and ``CompetencyRuleProfile.organization``. - -The first 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. - -``competency_taxonomy`` is CASCADE for a different reason: a rule profile must never be the -reason a taxonomy delete fails. Once taxonomy-scoped profiles exist, deleting a taxonomy has to -be blocked only when learner data is connected to it, and that check belongs in Python at the -application layer, the same way #655 settled it for every other record type. ``PROTECT`` would -push that decision into the database, which cannot tell the two cases apart. - -This is not a relaxation of ADR-0002 Decision 7: the four tree links above (``parent``, ``tag``, -``group``, ``object_tag``) 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 three: 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()``). +Seven of the nine foreign keys here are ``on_delete=models.CASCADE``: both ``CompetencyCriteriaGroup`` +tree links (``parent``, ``tag``), its ``course`` scope, both ``CompetencyCriterion`` links (``group``, +``object_tag``), and ``CompetencyRuleProfile``'s ``course`` and ``competency_taxonomy`` scope links. +The other two stay ``models.PROTECT``: ``CompetencyCriterion.rule_profile`` and +``CompetencyRuleProfile.organization``. + +CASCADE expresses containment: a row on the CASCADE side is meaningless once its referent is gone, +so its own delete has no separate policy to enforce. The tree links (``parent``, ``tag``, ``group``, +``object_tag``) also have to be CASCADE for a mechanical reason: 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. Those +same CASCADE edges are what carries the collector down to the PROTECT that actually enforces +ADR-0002 Decision 7 for learner data: #642's three ``Student*Status`` foreign keys, one and two +levels below the tag, are reached only by walking these edges, never relaxed by them. + +``CompetencyRuleProfile.course`` and ``.competency_taxonomy`` are CASCADE for an ADR-level reason, +not a mechanical one: Decision 7 (as amended) says a taxonomy or course is only ever hard-deleted +once nothing beneath it needs protecting, so a profile scoped to it is safe to remove at the same +time rather than blocking that delete. ``.organization`` stays PROTECT because an ``Organization`` +is not a competency-definition record covered by that reasoning, and ``edx-organizations`` +deactivates orgs rather than deleting them. + +``rule_profile`` staying PROTECT is Decision 7's actual backstop for a profile itself: a +CompetencyRuleProfile is never hard-deleted by a *direct* delete (retirement is archive-only), and +this is what makes that hold at the ORM layer, by blocking any attempt to delete one out from under +a criterion still assigned to it. + +One non-obvious consequence of the collector's database-not-pending-set lookup described above: +deleting a CompetencyTaxonomy whose taxonomy-scoped profile is itself assigned to a +CompetencyCriterion raises ProtectedError naming that criterion, even though the criterion would +also be cascade-deleted in the same operation through the tag chain. See +test_criteria_deletion.py's "residual tension" section for what this needs before it can be fixed +(a fifth ADR-0002 Decision 4 reassignment event), and why it cannot be reached with this phase's +data. """ from __future__ import annotations -from typing import Any - -from attrs import Attribute, define, field, fields 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.db.models import Q from django.utils.translation import gettext_lazy as _ from organizations.models import Organization from simple_history.models import HistoricalRecords @@ -58,6 +53,7 @@ from openedx_django_lib.fields import case_insensitive_char_field, immutable_uuid_field from openedx_tagging.models import ObjectTag, Tag +from ..rule_payloads import _RULE_PAYLOAD_SPECS, RuleType, validate_rule_payload from .competency_taxonomy import CompetencyTaxonomy __all__ = [ @@ -69,13 +65,10 @@ "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") +# The declared choices for both rule_type fields below, derived from the payload-spec registry +# (see rule_payloads.py) rather than hand-listed, so a rule type can never be offered as a choice +# without also having a payload spec that makes it actually saveable. +_RULE_TYPE_CHOICES = [(rule_type, RuleType(rule_type).label) for rule_type in _RULE_PAYLOAD_SPECS] class LogicOperator(models.TextChoices): @@ -85,118 +78,17 @@ class LogicOperator(models.TextChoices): OR = "OR", _("Or") -def _validate_op(_instance: object, _attribute: Attribute, value: object) -> None: - """Reject an 'op' outside the Grade rule's allowed comparison operators.""" - if value not in {"gte", "lte", "eq"}: - raise ValueError(_("The 'op' in a 'Grade' rule_payload must be one of: gte, lte, eq.")) - - -def _validate_grade_value(_instance: object, _attribute: Attribute, value: object) -> None: - """Reject a Grade rule's 'value' unless it's a non-boolean number in [0.0, 1.0].""" - # 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 TypeError(_("The 'value' in a 'Grade' rule_payload must be a number, not a boolean.")) - if not 0.0 <= value <= 1.0: - raise ValueError( - _( - "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} - ) - - -def _validate_scale(_instance: object, _attribute: Attribute, value: object) -> None: - """Reject a Grade rule's 'scale' unless it's exactly 'percent'.""" - if value != "percent": - raise ValueError(_("The 'scale' in a 'Grade' rule_payload must be 'percent'.")) - - -@define(frozen=True, kw_only=True) -class GradeRule: - """ - The rule_payload shape for RuleType.GRADE, per ADR-0002 Decision 3. - - Constructing one *is* the validation: kw_only means an unknown key raises TypeError - ("unexpected keyword argument") and a missing key raises TypeError ("missing ... required - keyword-only argument") from Python's own call handling, and a present-but-bad value raises - ValueError or TypeError from the field validators below. Frozen because this describes a - fixed spec and is never mutated after construction. - """ - - op: str = field(validator=_validate_op) - value: float = field(validator=_validate_grade_value) - scale: str = field(validator=_validate_scale) - - -# One spec class per RuleType with a defined rule_payload shape. Add a class and an entry here to -# support a new rule type. RuleType.VIEW and RuleType.MASTERY_LEVEL have no entry, so they keep -# being rejected by validate_rule_payload as not supported yet. -_RULE_PAYLOAD_SPECS: dict[str, type] = { - RuleType.GRADE: GradeRule, -} - - -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. - """ - spec_class = _RULE_PAYLOAD_SPECS.get(rule_type) - if spec_class is None: - raise ValidationError( - _("Rule type '%(rule_type)s' is not supported yet; only 'Grade' has a defined rule_payload shape.") - % {"rule_type": rule_type} - ) - # Checked separately, before construction: `spec_class(**payload)` on a non-dict payload - # (e.g. a list) raises "argument after ** must be a mapping", which is a confusing message to - # surface to an author. - if not isinstance(payload, dict): - raise ValidationError(_("A 'Grade' rule_payload must be a JSON object.")) - - # Also checked separately, before construction, rather than left to Python's own kw_only - # TypeError: that TypeError's text is "GradeRule.__init__() missing/got an unexpected keyword - # argument ...", a Python traceback fragment that leaks an internal class name to whoever - # edits this payload (a course author, via the admin). Deriving the expected keys from the - # spec class itself keeps that class the single source of truth for the key set, while owning - # the message in our own domain language instead of Python's. - expected_keys = {attr.name for attr in fields(spec_class)} - missing_keys = sorted(expected_keys - payload.keys()) - unexpected_keys = sorted(payload.keys() - expected_keys) - if missing_keys or unexpected_keys: - problems = [] - if missing_keys: - problems.append(_("missing %(keys)s") % {"keys": ", ".join(missing_keys)}) - if unexpected_keys: - problems.append(_("unexpected %(keys)s") % {"keys": ", ".join(unexpected_keys)}) - raise ValidationError( - _("A 'Grade' rule_payload has the wrong keys: %(problems)s.") - % {"problems": "; ".join(str(problem) for problem in problems)} - ) - - # Past the key check above, this can only fail on a value a field validator rejects. - try: - spec_class(**payload) - except (TypeError, ValueError) as exc: - # Surface the underlying message (our custom validators' text) rather than replacing it - # with something generic: that message is what full_clean() or save() surfaces to an - # admin or API caller. - raise ValidationError(str(exc)) from exc - - 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. + this group's own children combine. ``ordering`` gives this group's own position among its + siblings under their shared parent, which read-time evaluation and event-driven recomputation + rely on for deterministic, short-circuiting evaluation order. A group's children can be a mix + of child groups and leaf criteria, and only CompetencyCriteriaGroup carries an ``ordering`` + field, so that mix has no total order; #641 accepts this deliberately. See ADR-0002 Decision 2. .. no_pii: """ @@ -221,11 +113,13 @@ class CompetencyCriteriaGroup(models.Model): CourseRun, null=True, blank=True, - on_delete=models.PROTECT, + on_delete=models.CASCADE, 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="") + name = case_insensitive_char_field( + max_length=255, blank=True, default="", help_text=_("A human-readable label for this group, if any.") + ) ordering = models.PositiveIntegerField( default=0, help_text=_( @@ -238,7 +132,10 @@ class CompetencyCriteriaGroup(models.Model): choices=LogicOperator, null=True, blank=True, - help_text=_("How this group's children combine. Null until the group has children to combine."), + help_text=_( + "How this group's children combine. Null only for a group with a single child, where combining " + "logic is moot; the application layer treats null the same as OR." + ), ) history = HistoricalRecords() @@ -273,31 +170,22 @@ class CompetencyRuleProfile(models.Model): 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. - - ``rule_payload``'s shape (see :func:`validate_rule_payload`) is likewise validated from both - ``clean()`` and ``save()``, so ``objects.create()`` and a plain ``instance.save()`` are - covered without a caller needing to remember ``full_clean()``. A bulk ``QuerySet.update()``, - ``bulk_create()``, and a DRF serializer that writes straight to the database are NOT covered: - none of them build or save a model instance, so neither ``clean()`` nor ``save()`` ever runs. + criteria already resolved to this profile's scope are never silently re-governed. ``clean()`` + enforces this by comparing the current scope columns against what is actually persisted for + this row, so the check holds regardless of whether this instance was loaded with a partial + ``.only()``/``.defer()`` that skipped some scope columns. It does not cover a bulk + ``QuerySet.update()``, since that path never loads or constructs a model instance at all. + + ``rule_payload``'s shape (see :func:`~openedx_learning.applets.cbe.rule_payloads.validate_rule_payload`) + is likewise validated from ``clean()``, reached from both ``objects.create()`` and a plain + ``instance.save()`` via ``full_clean()``. A bulk ``QuerySet.update()``, ``bulk_create()``, and a + DRF serializer that writes straight to the database are NOT covered: none of them build or save + a model instance, so ``clean()`` never runs. .. 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 - + uuid = immutable_uuid_field() organization = models.ForeignKey( Organization, null=True, @@ -310,7 +198,7 @@ class CompetencyRuleProfile(models.Model): CourseRun, null=True, blank=True, - on_delete=models.PROTECT, + on_delete=models.CASCADE, related_name="competency_rule_profiles", help_text=_("The course run this profile is scoped to, if any."), ) @@ -322,24 +210,31 @@ class CompetencyRuleProfile(models.Model): 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("")), + # A plain column, written explicitly in save() below, NOT a database GeneratedField: null + # while archived, and the "org:X,course:Y,taxonomy:Z" string (see save()) while live. This is + # what lets the UniqueConstraint below stay a plain, unconditional one on every backend this + # project supports, including MySQL, which does not support the conditional/partial unique + # indexes that a naive "unique unless archived" rule would otherwise need (see ADR-0002 + # Rejected Alternative 6): SQL never treats two NULLs as equal, so any number of archived rows + # may share a scope while exactly one live row holds it. A plain column also can't be rewritten + # by Django's collector, unlike a GeneratedField: deleting a scope owner (a CompetencyTaxonomy + # or CourseRun) whose foreign key here is nullable and CASCADE nulls that one column on this + # row before deleting it, on any backend that can't defer constraint checks (MySQL); a + # GeneratedField would recompute from that nulled value and could collide with another row + # already occupying the resulting blank scope, raising IntegrityError instead of completing + # the cascade. A plain column is untouched by that nulling, so this row keeps its true + # scope_code, unseen by anyone, until the row itself is deleted. + scope_code = models.CharField( + max_length=255, + null=True, + editable=False, + help_text=_( + "Derived from organization/course/competency_taxonomy; null while archived, otherwise " + "\"org:X,course:Y,taxonomy:Z\" with each segment blank when that scope column is null. " + "Recomputed in save(); never set directly." ), - output_field=models.CharField(max_length=255), - db_persist=True, ) - rule_type = models.CharField(max_length=32, choices=RuleType) + rule_type = models.CharField(max_length=32, choices=_RULE_TYPE_CHOICES) rule_payload = models.JSONField( help_text=_("Structured payload keyed by rule_type; see validate_rule_payload for the shape it must match.") ) @@ -350,20 +245,23 @@ class CompetencyRuleProfile(models.Model): "authoring and new associations but remain queryable, so existing criteria stay resolvable." ), ) - uuid = immutable_uuid_field() + # scope_code is excluded from history: it is a derived, non-editable bookkeeping column (see + # above), not an author-facing fact worth its own historical row -- the columns it derives + # from (organization, course, competency_taxonomy, archived) are already tracked, and are what + # an audit trail actually needs. 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. + # A plain, unconditional UniqueConstraint, deliberately: scope_code is a plain, + # always-non-null-while-live column (see its definition above), not a conditional + # index over the raw nullable scope columns. MySQL (this project's tested and + # production database) does not support conditional/partial unique indexes -- Django + # only raises a non-fatal system-check warning (models.W036) and silently skips + # creating such a constraint there, while SQLite (used for quick local test runs) + # does support them and would mask the gap in that environment. See ADR-0002 Rejected + # Alternative 6. 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 @@ -379,48 +277,41 @@ class Meta: "competency_taxonomy." ), ), + models.CheckConstraint( + # Keeps scope_code's invariant honest against QuerySet.update(), which bypasses + # save(): the database refuses the row rather than letting this get out of sync + # behind save()'s back. + condition=( + Q(archived=True, scope_code__isnull=True) | Q(archived=False, scope_code__isnull=False) + ), + name="oel_cbe_ruleprofile_archived_scope_code_check", + violation_error_message=_( + "An archived CompetencyRuleProfile must have a null scope_code; a live one must not." + ), + ), ] - @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 + """Raise ValidationError if the scope columns no longer match what is persisted for this row.""" + if self.pk is None: + # A new, unsaved instance: there's no persisted scope yet to compare against. + return + # Always queries the database directly, rather than comparing against a value cached at + # load time: that avoids a deferred/only() load, or a refresh_from_db() call, silently + # bypassing this check. Explicitly targets self._state.db, the alias this instance + # actually belongs to, so an instance loaded from a non-default database is not silently + # compared against the wrong one. Guarded against the row having since been deleted, in + # which case there's nothing left to compare against either. + persisted_scope = ( + CompetencyRuleProfile.objects.using(self._state.db) + .filter(pk=self.pk) + .values_list("organization_id", "course_id", "competency_taxonomy_id") + .first() + ) + if persisted_scope is None: + return current_scope = (self.organization_id, self.course_id, self.competency_taxonomy_id) - if current_scope != loaded_scope: + if current_scope != persisted_scope: raise ValidationError( _( "A CompetencyRuleProfile's scope (organization, course, competency_taxonomy) cannot be " @@ -435,11 +326,22 @@ def clean(self): validate_rule_payload(self.rule_type, self.rule_payload) def save(self, *args, **kwargs): - """Persist this profile, after re-checking scope immutability and the rule_payload shape.""" - self._check_scope_immutable() - validate_rule_payload(self.rule_type, self.rule_payload) + """Recompute scope_code, then persist this profile after full_clean() re-validates it.""" + self.scope_code = None if self.archived else ( + f"org:{'' if self.organization_id is None else self.organization_id}," + f"course:{'' if self.course_id is None else self.course_id}," + f"taxonomy:{'' if self.competency_taxonomy_id is None else self.competency_taxonomy_id}" + ) + # validate_unique and validate_constraints are left to the database: the unique and check + # constraints above enforce them identically and without the extra queries full_clean() + # would otherwise run to pre-check them in Python. Matches CourseRun.save() at + # src/openedx_catalog/models/course_run.py. Neither the non-editable scope_code nor the + # nullable override-style fields on this model cause full_clean() to reject an otherwise + # valid row: Django's own Field.validate() skips every check for a field with + # editable=False, and a blank=True field with an empty value is skipped by clean_fields() + # before validation runs at all. + self.full_clean(validate_unique=False, validate_constraints=False) super().save(*args, **kwargs) - self.loaded_scope = (self.organization_id, self.course_id, self.competency_taxonomy_id) class CompetencyCriterion(models.Model): @@ -456,11 +358,11 @@ class CompetencyCriterion(models.Model): that recomputes it; that would contradict the ADR. When ``rule_type_override`` is set, its ``rule_payload_override``'s shape (see - :func:`validate_rule_payload`) is validated from both ``clean()`` and ``save()``, so - ``objects.create()`` and a plain ``instance.save()`` are covered without a caller needing to - remember ``full_clean()``. A bulk ``QuerySet.update()``, ``bulk_create()``, and a DRF - serializer that writes straight to the database are NOT covered: none of them build or save a - model instance, so neither ``clean()`` nor ``save()`` ever runs. + :func:`~openedx_learning.applets.cbe.rule_payloads.validate_rule_payload`) is validated from + ``clean()``, reached from both ``objects.create()`` and a plain ``instance.save()`` via + ``full_clean()``. A bulk ``QuerySet.update()``, ``bulk_create()``, and a DRF serializer that + writes straight to the database are NOT covered: none of them build or save a model instance, + so ``clean()`` never runs. .. no_pii: """ @@ -489,7 +391,7 @@ class CompetencyCriterion(models.Model): 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_type_override = models.CharField(max_length=32, choices=_RULE_TYPE_CHOICES, null=True, blank=True) rule_payload_override = models.JSONField(null=True, blank=True) history = HistoricalRecords() @@ -537,7 +439,8 @@ def clean(self): validate_rule_payload(self.rule_type_override, self.rule_payload_override) def save(self, *args, **kwargs): - """Persist this criterion, after re-checking the override rule_payload's shape, if set.""" - if self.rule_type_override is not None: - validate_rule_payload(self.rule_type_override, self.rule_payload_override) + """Persist this criterion, after full_clean() re-validates the override payload, if set.""" + # See CompetencyRuleProfile.save() above for why validate_unique/validate_constraints are + # skipped here too, and why the nullable override fields don't trip full_clean() when unset. + self.full_clean(validate_unique=False, validate_constraints=False) super().save(*args, **kwargs) diff --git a/src/openedx_learning/applets/cbe/rule_payloads.py b/src/openedx_learning/applets/cbe/rule_payloads.py new file mode 100644 index 000000000..4c781f2ee --- /dev/null +++ b/src/openedx_learning/applets/cbe/rule_payloads.py @@ -0,0 +1,152 @@ +""" +Rule payload shapes for CBE evaluation rules, and the parser that validates a raw payload against +the shape its rule_type defines. + +See :ref:`openedx-learning-adr-0002` Decision 3 for the payload contract this enforces. Each +supported rule_type has exactly one spec class here (currently only :class:`GradeRule`) and one +matching entry in ``_RULE_PAYLOAD_SPECS``; :data:`RuleType` declares only the rule types that have +both, so a rule type can never be offered as a choice (see ``models/criteria.py``, which derives +both models' ``choices=`` from this same registry) without also being possible to save. Adding a +new rule type (for example ``MasteryLevel``) is one spec class plus one registry entry, and the +``RuleType`` member that goes with them. +""" +from __future__ import annotations + +from typing import Any + +from attrs import Attribute, define, field, fields +from django.core.exceptions import ValidationError +from django.db import models +from django.utils.translation import gettext_lazy as _ + +__all__ = [ + "GradeRule", + "RuleType", + "parse_rule_payload", + "validate_rule_payload", +] + + +class RuleType(models.TextChoices): + """ + The evaluation rule types a CompetencyRuleProfile or CompetencyCriterion override can use. + + Declares exactly the rule types with a defined rule_payload shape below, i.e. exactly the keys + of ``_RULE_PAYLOAD_SPECS``: see this module's own docstring for why the two are never allowed + to drift apart. + """ + + GRADE = "Grade", _("Grade") + + +def _validate_op(_instance: object, _attribute: Attribute, value: object) -> None: + """Reject an 'op' outside the Grade rule's allowed comparison operators.""" + if value not in {"gte", "lte", "eq"}: + raise ValueError(_("The 'op' in a 'Grade' rule_payload must be one of: gte, lte, eq.")) + + +def _validate_grade_value(_instance: object, _attribute: Attribute, value: object) -> None: + """Reject a Grade rule's 'value' unless it's a non-boolean number in [0.0, 1.0].""" + # 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 TypeError(_("The 'value' in a 'Grade' rule_payload must be a number, not a boolean.")) + if not 0.0 <= value <= 1.0: + raise ValueError( + _( + "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} + ) + + +def _validate_scale(_instance: object, _attribute: Attribute, value: object) -> None: + """Reject a Grade rule's 'scale' unless it's exactly 'percent'.""" + if value != "percent": + raise ValueError(_("The 'scale' in a 'Grade' rule_payload must be 'percent'.")) + + +@define(frozen=True, kw_only=True) +class GradeRule: + """ + The rule_payload shape for RuleType.GRADE, per ADR-0002 Decision 3. + + Constructing one *is* the validation: kw_only means an unknown key raises TypeError + ("unexpected keyword argument") and a missing key raises TypeError ("missing ... required + keyword-only argument") from Python's own call handling, and a present-but-bad value raises + ValueError or TypeError from the field validators below. Frozen because this describes a + fixed spec and is never mutated after construction. + """ + + op: str = field(validator=_validate_op) + value: float = field(validator=_validate_grade_value) + scale: str = field(validator=_validate_scale) + + +# One spec class per RuleType with a defined rule_payload shape. Add a class and an entry here (and +# the matching RuleType member above) to support a new rule type. +_RULE_PAYLOAD_SPECS: dict[str, type] = { + RuleType.GRADE: GradeRule, +} + + +def parse_rule_payload(rule_type: str, payload: Any) -> Any: + """ + Validate ``payload`` against the shape ADR-0002 Decision 3 defines for ``rule_type``, and + return the constructed, frozen spec object (for example a :class:`GradeRule`) on success. + + Raises ``django.core.exceptions.ValidationError`` on any mismatch, including a ``rule_type`` + with no defined payload shape at all. Prefer this over :func:`validate_rule_payload` at a call + site that wants the parsed, typed fields (for example read-time rule evaluation), not just the + pass/fail check. + """ + spec_class = _RULE_PAYLOAD_SPECS.get(rule_type) + if spec_class is None: + raise ValidationError( + _("Rule type '%(rule_type)s' is not supported yet; only 'Grade' has a defined rule_payload shape.") + % {"rule_type": rule_type} + ) + # Checked separately, before construction: `spec_class(**payload)` on a non-dict payload + # (e.g. a list) raises "argument after ** must be a mapping", which is a confusing message to + # surface to an author. + if not isinstance(payload, dict): + raise ValidationError(_("A '%(rule_type)s' rule_payload must be a JSON object.") % {"rule_type": rule_type}) + + # Also checked separately, before construction, rather than left to Python's own kw_only + # TypeError: that TypeError's text is "GradeRule.__init__() missing/got an unexpected keyword + # argument ...", a Python traceback fragment that leaks an internal class name to whoever + # edits this payload (a course author, via the admin). Deriving the expected keys from the + # spec class itself keeps that class the single source of truth for the key set, while owning + # the message in our own domain language instead of Python's. + expected_keys = {attr.name for attr in fields(spec_class)} + missing_keys = sorted(expected_keys - payload.keys()) + unexpected_keys = sorted(payload.keys() - expected_keys) + if missing_keys or unexpected_keys: + problems = [] + if missing_keys: + problems.append(_("missing %(keys)s") % {"keys": ", ".join(missing_keys)}) + if unexpected_keys: + problems.append(_("unexpected %(keys)s") % {"keys": ", ".join(unexpected_keys)}) + raise ValidationError( + _("A '%(rule_type)s' rule_payload has the wrong keys: %(problems)s.") + % {"rule_type": rule_type, "problems": "; ".join(str(problem) for problem in problems)} + ) + + # Past the key check above, this can only fail on a value a field validator rejects. + try: + return spec_class(**payload) + except (TypeError, ValueError) as exc: + # Surface the underlying message (our custom validators' text) rather than replacing it + # with something generic: that message is what full_clean() or save() surfaces to an + # admin or API caller. + raise ValidationError(str(exc)) from exc + + +def validate_rule_payload(rule_type: str, payload: Any) -> None: + """ + Validate ``payload`` against the shape ADR-0002 Decision 3 defines for ``rule_type``. + + A thin wrapper around :func:`parse_rule_payload`, for a call site (``clean()``/``save()`` on + both CBE models) that only needs the pass/fail check and has no use for the parsed object. + """ + parse_rule_payload(rule_type, payload) diff --git a/src/openedx_learning/migrations/0002_competency_criteria.py b/src/openedx_learning/migrations/0002_competency_criteria.py index 6b2e4229b..1681b5d81 100644 --- a/src/openedx_learning/migrations/0002_competency_criteria.py +++ b/src/openedx_learning/migrations/0002_competency_criteria.py @@ -3,8 +3,6 @@ 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 @@ -26,17 +24,17 @@ class Migration(migrations.Migration): 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."), + 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."), ), 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)), + ('name', openedx_django_lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, default='', help_text='A human-readable label for this group, if any.', 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')), + ('logic_operator', models.CharField(blank=True, choices=[('AND', 'And'), ('OR', 'Or')], help_text="How this group's children combine. Null only for a group with a single child, where combining logic is moot; the application layer treats null the same as OR.", 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.CASCADE, 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.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')), ], @@ -45,13 +43,13 @@ class Migration(migrations.Migration): 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)), + ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), + ('scope_code', models.CharField(editable=False, help_text='Derived from organization/course/competency_taxonomy; null while archived, otherwise "org:X,course:Y,taxonomy:Z" with each segment blank when that scope column is null. Recomputed in save(); never set directly.', max_length=255, null=True)), + ('rule_type', models.CharField(choices=[('Grade', 'Grade')], 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.CASCADE, 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')), + ('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.CASCADE, 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')), ], ), @@ -60,7 +58,7 @@ class Migration(migrations.Migration): 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_type_override', models.CharField(blank=True, choices=[('Grade', 'Grade')], 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.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')), @@ -76,9 +74,9 @@ class Migration(migrations.Migration): 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)), + ('name', openedx_django_lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, default='', help_text='A human-readable label for this group, if any.', 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)), + ('logic_operator', models.CharField(blank=True, choices=[('AND', 'And'), ('OR', 'Or')], help_text="How this group's children combine. Null only for a group with a single child, where combining logic is moot; the application layer treats null the same as OR.", 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)), @@ -101,7 +99,7 @@ class Migration(migrations.Migration): 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_type_override', models.CharField(blank=True, choices=[('Grade', 'Grade')], 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)), @@ -124,7 +122,7 @@ class Migration(migrations.Migration): 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_type', models.CharField(choices=[('Grade', 'Grade')], 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')), @@ -157,6 +155,10 @@ class Migration(migrations.Migration): 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='competencyruleprofile', + constraint=models.CheckConstraint(condition=models.Q(models.Q(('archived', True), ('scope_code__isnull', True)), models.Q(('archived', False), ('scope_code__isnull', False)), _connector='OR'), name='oel_cbe_ruleprofile_archived_scope_code_check', violation_error_message='An archived CompetencyRuleProfile must have a null scope_code; a live one must not.'), + ), 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 index 28374c49e..9a1f4ab66 100644 --- a/src/openedx_learning/migrations/0003_seed_default_rule_profile.py +++ b/src/openedx_learning/migrations/0003_seed_default_rule_profile.py @@ -7,14 +7,24 @@ """ from django.db import migrations +# Fixed rather than uuid.uuid4(), so this shared system-default row has the same external +# identifier in every deployment, not a fresh random one each time this migration runs. +_DEFAULT_RULE_PROFILE_UUID = "5b3e8f5c-3b0e-4b1a-9b1e-6b6b6b6b6b6b" + def seed_default_rule_profile(apps, schema_editor): """Create the all-null-scope CompetencyRuleProfile.""" CompetencyRuleProfile = apps.get_model('openedx_learning', 'CompetencyRuleProfile') CompetencyRuleProfile.objects.create( + uuid=_DEFAULT_RULE_PROFILE_UUID, rule_type='Grade', rule_payload={'op': 'gte', 'value': 0.8, 'scale': 'percent'}, archived=False, + # apps.get_model() returns a historical model reconstructed from migration state, which + # does not carry CompetencyRuleProfile's custom save() (and so never computes this). + # organization_id/course_id/competency_taxonomy_id are all null for this row, so every + # segment of the "org:X,course:Y,taxonomy:Z" format is blank. + scope_code='org:,course:,taxonomy:', ) From 5380a6449d207f9b21aa77cd15b17778ceb0fcda Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Sat, 5 Sep 2026 10:41:49 -0400 Subject: [PATCH 9/9] test: cover every #641 acceptance criterion and the deletion edge cases Add a test per acceptance criterion, plus the cases the previous per-foreign-key shape could not reach: a taxonomy or course run deleted with a scoped rule profile, two taxonomies deleted together, an archived profile's scope being reused, and an ObjectTag delete leaving a childless group behind. Add test_criteria_trees.py for whole-tree deletion, so a test proves the bad outcome is avoided rather than only that a cascade fired: it builds a root/branch/grandchild tree with criteria at two levels and a mix of profile-assigned and override criteria, deletes in the middle, and asserts exactly which rows survive. Run the deletion paths under MySQL's collector semantics while still on SQLite, by setting can_defer_constraint_checks to False. That is what makes this class of bug visible in the fast local suite instead of only in the MySQL CI job. Rename every test so the name states the expected behavior rather than the mechanism, and move the fixtures duplicated across both files into conftest.py. Co-Authored-By: Claude Opus 5 (1M context) --- .../openedx_learning/applets/cbe/conftest.py | 73 ++ .../applets/cbe/test_criteria_deletion.py | 337 ++++++--- .../applets/cbe/test_criteria_models.py | 708 +++++++++++++----- .../applets/cbe/test_criteria_trees.py | 144 ++++ 4 files changed, 1002 insertions(+), 260 deletions(-) create mode 100644 tests/openedx_learning/applets/cbe/conftest.py create mode 100644 tests/openedx_learning/applets/cbe/test_criteria_trees.py diff --git a/tests/openedx_learning/applets/cbe/conftest.py b/tests/openedx_learning/applets/cbe/conftest.py new file mode 100644 index 000000000..27953f7d0 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/conftest.py @@ -0,0 +1,73 @@ +""" +Shared fixtures for the CBE criteria test modules (schema, deletion, and tree-integration tests). + +Every fixture here used to be duplicated verbatim across test_criteria_models.py and +test_criteria_deletion.py. Consolidated here so both files, plus test_criteria_trees.py, share one +definition. +""" +import pytest +from organizations.api import ensure_organization +from organizations.models import Organization + +from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_learning.models import CompetencyCriteriaGroup, CompetencyRuleProfile, CompetencyTaxonomy +from openedx_tagging.models import ObjectTag, Tag + + +@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, + ) diff --git a/tests/openedx_learning/applets/cbe/test_criteria_deletion.py b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py index 55228f6da..6762beaa9 100644 --- a/tests/openedx_learning/applets/cbe/test_criteria_deletion.py +++ b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py @@ -1,16 +1,38 @@ """ Delete-behavior tests for CompetencyCriteriaGroup, CompetencyRuleProfile, and CompetencyCriterion. -Five of these nine foreign keys are `on_delete=models.CASCADE` and four are `models.PROTECT`; see -the module docstring in `openedx_learning.applets.cbe.models.criteria` for which is which and why. +Two of the nine foreign keys here assert a different on_delete value than issue #641 itself +specifies, because on_delete expresses containment (is this row meaningless once its referent is +gone?), never a protection policy: + +- CompetencyCriteriaGroup.course must be CASCADE, not PROTECT: a course-scoped criteria tree is + meaningless once its course run is gone. PROTECT would make a course run permanently + undeletable the moment any competency criteria exist for it, even with zero learner data, which + is a different (and stricter) guarantee than anything #641 actually needs. +- CompetencyRuleProfile.course must be CASCADE, not PROTECT, for the same reason, and because a + taxonomy or course is only ever hard-deleted once nothing beneath it needs protecting (see + ADR-0002 Decision 7's amended text): a course-scoped profile is safe to remove along with its + course rather than blocking the delete. + +CompetencyRuleProfile.competency_taxonomy is also CASCADE, matching #641's own AC25. A +CompetencyRuleProfile's own "never hard-deleted" rule (Decision 7) governs a *direct* delete of a +profile; it does not stop a profile from being cascaded away as a side effect of deleting the +taxonomy or course it is scoped to, once nothing else protects it. When something else does +protect it -- a CompetencyCriterion still assigned to it via the PROTECT'd `rule_profile` foreign +key -- deleting the taxonomy or course still raises ProtectedError, exactly as it would for any +other row a PROTECT relationship blocks; see the "residual tension" section below for the one +case where that ProtectedError names the wrong object. + +Fixtures shared with test_criteria_models.py and test_criteria_trees.py live in this directory's +conftest.py. """ import pytest from django.apps import apps +from django.db import connection 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_catalog.models import CourseRun from openedx_learning.models import ( CompetencyCriteriaGroup, CompetencyCriterion, @@ -25,79 +47,20 @@ _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. The four 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 five that are CASCADE assert the delete succeeds and that the referencing row is actually +# One test per foreign key. The PROTECT ones assert ProtectedError and inspect `protected_objects` +# to confirm which relationship actually fired: several protected relationships can fire on one +# delete (see test_deleting_an_organization_with_a_scoped_profile_raises_protected_error_naming_ +# the_profile 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 CASCADE ones 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_cascade(tag: Tag) -> None: +def test_deleting_a_group_also_deletes_its_child_groups(tag: Tag) -> None: """ Deleting a CompetencyCriteriaGroup cascades to any child group referencing it via `parent`: the delete succeeds and the child row is gone too. @@ -112,7 +75,7 @@ def test_group_parent_cascade(tag: Tag) -> None: assert not CompetencyCriteriaGroup.objects.filter(pk=child.pk).exists() -def test_group_tag_cascade(tag: Tag, group: CompetencyCriteriaGroup) -> None: +def test_deleting_a_tag_also_deletes_its_competency_criteria_groups(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 @@ -130,18 +93,25 @@ def test_group_tag_cascade(tag: Tag, group: CompetencyCriteriaGroup) -> None: assert historical_group.objects.filter(id=group_pk, history_type="-").exists() -def test_group_course_protect(tag: Tag, course_run: CourseRun) -> None: - """Deleting a CourseRun that a CompetencyCriteriaGroup references via `course` raises ProtectedError.""" +def test_deleting_a_course_run_also_deletes_its_course_scoped_criteria_groups( + tag: Tag, course_run: CourseRun +) -> None: + """ + Deleting a CourseRun cascades to any CompetencyCriteriaGroup scoped to it via `course`: the + delete succeeds and the group row is gone too. A course-scoped criteria tree has no meaning + once the course run it evaluates against no longer exists. + """ group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run) + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() - with pytest.raises(ProtectedError) as exc_info: - course_run.delete() + course_run.delete() - protected = exc_info.value.protected_objects - assert any(isinstance(obj, CompetencyCriteriaGroup) and obj.pk == group.pk for obj in protected) + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() -def test_rule_profile_organization_protect(organization2: Organization) -> None: +def test_deleting_an_organization_with_a_scoped_profile_raises_protected_error_naming_the_profile( + organization2: Organization, +) -> None: """ Deleting an Organization that a CompetencyRuleProfile references via `organization` raises ProtectedError naming the profile. @@ -163,25 +133,40 @@ def test_rule_profile_organization_protect(organization2: Organization) -> None: 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.""" +def test_deleting_a_course_run_with_a_scoped_rule_profile_also_deletes_the_profile( + course_run: CourseRun, +) -> None: + """ + Deleting a CourseRun cascades to any CompetencyRuleProfile scoped to it via `course`: the + delete succeeds and the profile row is gone too. A CompetencyRuleProfile is never hard-deleted + by a *direct* delete of the profile itself (ADR-0002 Decision 7); that does not stop it being + cascaded away as a side effect of deleting the course it is scoped to, once nothing else (no + CompetencyCriterion still assigned to it) protects it -- a course is only ever hard-deleted + once nothing beneath it needs protecting. + """ profile = CompetencyRuleProfile.objects.create( course=course_run, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD ) + assert CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() - with pytest.raises(ProtectedError) as exc_info: - course_run.delete() + course_run.delete() - protected = exc_info.value.protected_objects - assert any(isinstance(obj, CompetencyRuleProfile) and obj.pk == profile.pk for obj in protected) + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() -def test_rule_profile_competency_taxonomy_cascade(competency_taxonomy: CompetencyTaxonomy) -> None: +def test_deleting_a_taxonomy_with_a_scoped_rule_profile_also_deletes_the_profile( + competency_taxonomy: CompetencyTaxonomy, +) -> None: """ - Deleting a CompetencyTaxonomy cascades to any CompetencyRuleProfile referencing it via - `competency_taxonomy`: the delete succeeds and the profile row is gone too. Nothing changes - behaviorally in this MVP, since only the all-null system-default profile exists, so there is - no taxonomy-scoped profile for this CASCADE to ever actually remove yet. + Deleting a CompetencyTaxonomy cascades to any CompetencyRuleProfile scoped to it via + `competency_taxonomy`: the delete succeeds and the profile row is gone too, matching #641's + AC25. A CompetencyRuleProfile is never hard-deleted by a *direct* delete of the profile itself + (ADR-0002 Decision 7); that does not stop it being cascaded away as a side effect of deleting + the taxonomy it is scoped to, once nothing else protects it. Nothing changes behaviorally in + this MVP, since only the all-null system-default profile exists otherwise, so this scenario + cannot arise until a taxonomy-scoped profile is actually created, which no authoring screen + does yet. See the "residual tension" section below for what happens instead when a + CompetencyCriterion is still assigned to the scoped profile being cascaded away. """ profile = CompetencyRuleProfile.objects.create( competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD @@ -193,7 +178,7 @@ def test_rule_profile_competency_taxonomy_cascade(competency_taxonomy: Competenc assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() -def test_criterion_group_cascade( +def test_deleting_a_group_also_deletes_its_criteria( group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile ) -> None: """ @@ -211,7 +196,7 @@ def test_criterion_group_cascade( assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() -def test_criterion_object_tag_cascade( +def test_deleting_an_object_tag_also_deletes_its_criteria( group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile ) -> None: """ @@ -230,7 +215,7 @@ def test_criterion_object_tag_cascade( assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() -def test_criterion_rule_profile_protect( +def test_deleting_a_rule_profile_referenced_by_a_criterion_raises_protected_error( group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile ) -> None: """ @@ -248,6 +233,170 @@ def test_criterion_rule_profile_protect( assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in protected) +# ============================================================================================== +# Residual tension (see DECISION-on-delete.md's REVISION section): with `competency_taxonomy` +# CASCADE and `rule_profile` PROTECT, deleting a taxonomy whose scoped profile is itself assigned +# to a criterion raises ProtectedError, because Django's collector looks up rows that reference the +# profile in the database rather than in the set it has already decided to delete -- it fires even +# though that same criterion would also be cascade-deleted in this same operation, via the separate +# Tag -> CompetencyCriteriaGroup.tag -> CompetencyCriterion.group chain. Confirmed, accepted defect +# for this MVP: no code path creates a taxonomy-scoped profile at all, so it cannot be reached with +# real data. The fix, when scoped profiles are built, is a fifth reassignment event on ADR-0002 +# Decision 4 (which currently names four): "the profile's scope owner is being deleted" reassigns +# every criterion off that profile in an application-layer function, before the cascade proceeds. +# Not built in this change; #641 scopes it out. +# ============================================================================================== + + +def test_taxonomy_delete_with_a_criterion_assigned_its_scoped_profile_raises_protected_error_naming_the_criterion( + competency_taxonomy: CompetencyTaxonomy, group: CompetencyCriteriaGroup, object_tag: ObjectTag +) -> None: + """ + Deleting a CompetencyTaxonomy whose taxonomy-scoped profile is itself assigned to a criterion + raises ProtectedError naming the CRITERION, not the profile actually being cascaded away. + + The taxonomy delete cascades into the profile (`competency_taxonomy` is CASCADE), and only then + discovers the profile is referenced by the criterion via `rule_profile` (PROTECT). Django's + PROTECT handler raises unconditionally whenever a referencing row exists in the database; it + never checks whether that same row is also already part of the same delete's pending set, so it + fires here even though this exact criterion would also be reached and removed via the tag chain + (Tag -> CompetencyCriteriaGroup.tag -> CompetencyCriterion.group, all CASCADE) if the profile + hadn't blocked the walk first. This is a spurious, confusing failure -- an author deleting a + taxonomy is told a criterion is in the way, when nothing about that criterion actually survives + the delete either -- but it is not reachable in this MVP (see the section header above), so + this pins the current behavior rather than working around it with a schema change. + """ + profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + criterion = CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=profile) + + with pytest.raises(ProtectedError) as exc_info: + competency_taxonomy.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in protected) + assert not any(isinstance(obj, CompetencyRuleProfile) and obj.pk == profile.pk for obj in protected) + # Nothing was actually removed: the whole operation raised before any DELETE executed. + assert CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +# ============================================================================================== +# MySQL collector semantics, reproduced on SQLite by monkeypatching can_defer_constraint_checks. +# +# MySQL cannot defer foreign-key constraint checks (can_defer_constraint_checks is False there). +# django.db.models.deletion.CASCADE reads that flag directly: whenever a cascading foreign key is +# nullable and constraints can't be deferred, it nulls that column on every row about to be +# cascade-deleted (via collector.add_field_update) BEFORE the actual DELETE, to avoid a transient +# FK violation under non-deferred constraint checking. On ordinary SQLite semantics (deferred +# constraints allowed), this nulling never happens at all, so a defect that only shows up via this +# path is invisible on the fast local suite and only ever caught by the separate MySQL CI job +# (AC8). Monkeypatching the flag reproduces it here instead. Do not "simplify" these tests by +# dropping the monkeypatch: without it, neither scenario below reproduces anything, on either the +# broken or the fixed code. +# +# This used to be where the shipped bug lived: when scope_code was a database GeneratedField, this +# same pre-delete nulling of a profile's scope foreign key recomputed scope_code, colliding it with +# whatever other row already held that now-blank scope (the seeded system-default row, or a second +# profile nulled in the same batch) and raising IntegrityError instead of completing the cascade. +# Making scope_code a plain column written only in save() (see models/criteria.py) fixes this: the +# collector's nulling touches only the real scope foreign key column, never scope_code, so a +# profile being cascade-deleted keeps its true scope_code, unseen by anyone, until the row is gone. +# ============================================================================================== + + +def test_taxonomy_delete_cascades_its_scoped_profile_under_mysql_collector_semantics( + monkeypatch: pytest.MonkeyPatch, competency_taxonomy: CompetencyTaxonomy +) -> None: + """ + Deleting a CompetencyTaxonomy with a taxonomy-scoped profile succeeds and cascades the profile + away even under MySQL's non-deferred constraint semantics, the same as it does under ordinary + SQLite semantics (see test_deleting_a_taxonomy_with_a_scoped_rule_profile_also_deletes_the_ + profile above). Confirms the fix described in this section's header actually holds under the + collector path that used to trigger the bug: nulling the profile's `competency_taxonomy_id` + before deleting it does not touch `scope_code`, which would otherwise collide with the seeded + system-default profile's identical blank scope and raise IntegrityError instead of completing. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + competency_taxonomy.delete() + + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + +def test_course_run_delete_cascades_its_course_scoped_criteria_group_under_mysql_collector_semantics( + monkeypatch: pytest.MonkeyPatch, tag: Tag, course_run: CourseRun +) -> None: + """ + Deleting a CourseRun with a course-scoped CompetencyCriteriaGroup succeeds and cascades the + group away even under MySQL's non-deferred constraint semantics. `course` is one of the two + nullable foreign keys this change turns from PROTECT to CASCADE, so it shares the exact + pre-delete-nulling collector path the taxonomy case above does; unlike scope_code, + CompetencyCriteriaGroup carries no uniqueness constraint a null `course_id` could collide with, + so this path is expected to just succeed. Pinned here anyway, alongside the taxonomy case, + since a future fix to one foreign key without the other would otherwise go unnoticed. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run) + + course_run.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + + +def test_course_run_delete_cascades_its_scoped_rule_profile_under_mysql_collector_semantics( + monkeypatch: pytest.MonkeyPatch, course_run: CourseRun +) -> None: + """ + Deleting a CourseRun with a course-scoped CompetencyRuleProfile succeeds and cascades the + profile away even under MySQL's non-deferred constraint semantics, the same as the taxonomy + case above: `course` is CompetencyRuleProfile's other newly-CASCADE foreign key, and shares the + same pre-delete-nulling collector path and the same scope_code collision this fix removes. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + profile = CompetencyRuleProfile.objects.create( + course=course_run, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + course_run.delete() + + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + +def test_deleting_two_taxonomies_together_cascades_both_their_scoped_profiles_away( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """ + Deleting two CompetencyTaxonomy rows in one `.delete()` call, each with its own taxonomy-scoped + profile, succeeds and cascades both profiles away -- neither profile's scope_code collides with + the other's, even though both get their `competency_taxonomy_id` nulled in the same collector + batch under MySQL's non-deferred constraint semantics. + + Same path as the single-taxonomy MySQL case above, but confirms it does not get worse when two + scope owners are collected in the same collector pass: before scope_code became a plain column, + nulling both profiles' `competency_taxonomy_id` in the same batch drove both scope_code values + to the identical blank "org:,course:,taxonomy:" string and raised IntegrityError on whichever + row the database processed second. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + taxonomy1 = CompetencyTaxonomy.objects.create(name="Nursing Two Taxonomy Delete", export_id="nursing-two-del") + taxonomy2 = CompetencyTaxonomy.objects.create(name="Welding Two Taxonomy Delete", export_id="welding-two-del") + profile1 = CompetencyRuleProfile.objects.create( + competency_taxonomy=taxonomy1, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile2 = CompetencyRuleProfile.objects.create( + competency_taxonomy=taxonomy2, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + CompetencyTaxonomy.objects.filter(pk__in=[taxonomy1.pk, taxonomy2.pk]).delete() + + assert not CompetencyRuleProfile.objects.filter(pk__in=[profile1.pk, profile2.pk]).exists() + + # ============================================================================================== # 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 @@ -259,6 +408,10 @@ def test_criterion_rule_profile_protect( # 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. +# +# See test_criteria_trees.py for the fuller integrative version of this shape: a wider tree with a +# surviving sibling branch and a mix of profile-assigned and override criteria, asserting exactly +# which rows survive rather than only that a cascade fired. # ============================================================================================== diff --git a/tests/openedx_learning/applets/cbe/test_criteria_models.py b/tests/openedx_learning/applets/cbe/test_criteria_models.py index 7d6cc4420..09c1aab2b 100644 --- a/tests/openedx_learning/applets/cbe/test_criteria_models.py +++ b/tests/openedx_learning/applets/cbe/test_criteria_models.py @@ -1,15 +1,24 @@ """ Tests for CompetencyCriteriaGroup, CompetencyRuleProfile, and CompetencyCriterion. + +Fixtures shared with test_criteria_deletion.py and test_criteria_trees.py live in this directory's +conftest.py. """ +import uuid as uuid_module + 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 +# _RULE_PAYLOAD_SPECS is private: it's the payload-spec registry itself, the one place that +# defines which rule types can actually be saved, which is exactly what +# test_rule_type_choices_match_rule_types_with_a_defined_payload_spec below needs to compare +# RuleType's declared choices against. +from openedx_learning.applets.cbe.models.criteria import _RULE_PAYLOAD_SPECS from openedx_learning.models import ( CompetencyCriteriaGroup, CompetencyCriterion, @@ -18,7 +27,7 @@ LogicOperator, RuleType, ) -from openedx_tagging.models import ObjectTag, Tag +from openedx_tagging.models import ObjectTag, Tag, Taxonomy pytestmark = pytest.mark.django_db @@ -34,130 +43,290 @@ 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"), + # "View" is a plain string, not RuleType.VIEW: RuleType declares only rule types that have a + # defined payload spec (see test_rule_type_choices_match_rule_types_with_a_defined_payload_spec + # below), so an unsupported rule type is, by construction, one that isn't a RuleType member at + # all. Behaviorally identical either way, since a TextChoices member IS its string value. + pytest.param("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") +# ============================================================================================== +# Schema and columns (AC1, AC2, AC6, AC12, AC17, AC22, AC33, AC34, AC23). CompetencyTaxonomy's own +# taxonomy_overrides_org default (AC1) is covered in test_models.py, not duplicated here. +# ============================================================================================== -@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") +def test_group_columns_match_adr_decision_2(course_run: CourseRun, tag: Tag) -> None: + """ + CompetencyCriteriaGroup has exactly the columns ADR-0002 Decision 2 lists: a nullable self-FK + `parent`, a required `tag` (db_column oel_tagging_tag_id), a nullable `course` targeting + openedx_catalog.CourseRun, `name`, `ordering`, and `logic_operator`, plus `id`. + """ + group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run) + + parent_field = CompetencyCriteriaGroup._meta.get_field("parent") + assert parent_field.null is True + assert parent_field.remote_field.model is CompetencyCriteriaGroup + tag_field = CompetencyCriteriaGroup._meta.get_field("tag") + assert tag_field.null is False + assert tag_field.remote_field.model is Tag + assert tag_field.db_column == "oel_tagging_tag_id" -@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") + course_field = CompetencyCriteriaGroup._meta.get_field("course") + assert course_field.null is True + assert course_field.remote_field.model is CourseRun + assert CompetencyCriteriaGroup._meta.get_field("name").null is False + assert CompetencyCriteriaGroup._meta.get_field("ordering").null is False + assert CompetencyCriteriaGroup._meta.get_field("logic_operator").null is True -@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") + assert group.course_id == course_run.pk -@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") +def test_rule_profile_columns_match_adr_decision_3() -> None: + """ + CompetencyRuleProfile has exactly the columns ADR-0002 Decision 3 lists: nullable + `organization`, `course`, and `competency_taxonomy` scope fields, `scope_code`, `rule_type`, + `rule_payload`, and `archived` (defaulting to False), plus `id`. + `scope_code` is nullable, not "never null" as an earlier reading of AC7 (issue #641) required: + see DECISION-on-delete.md deviation 3. It is null exactly while a profile is archived (see + test_scope_code_is_null_once_archived_and_non_null_while_live below); this is what lets an + archived profile stop occupying its scope's unique slot. + """ + organization_field = CompetencyRuleProfile._meta.get_field("organization") + assert organization_field.null is True + assert organization_field.remote_field.model is Organization -@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, - ) + course_field = CompetencyRuleProfile._meta.get_field("course") + assert course_field.null is True + assert course_field.remote_field.model is CourseRun + + taxonomy_field = CompetencyRuleProfile._meta.get_field("competency_taxonomy") + assert taxonomy_field.null is True + assert taxonomy_field.remote_field.model is CompetencyTaxonomy + + assert CompetencyRuleProfile._meta.get_field("scope_code").null is True + assert CompetencyRuleProfile._meta.get_field("rule_type").null is False + assert CompetencyRuleProfile._meta.get_field("rule_payload").null is False + assert CompetencyRuleProfile._meta.get_field("archived").default is False + + +def test_criterion_columns_match_adr_decision_4( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + CompetencyCriterion has exactly the columns ADR-0002 Decision 4 lists: required `group` and + `object_tag`, a nullable `rule_profile`, and nullable `rule_type_override` / + `rule_payload_override`, plus `id`. The model is named CompetencyCriterion (singular; the + table holds many, individually a criterion), and carries no Meta.db_table override, so the + table is Django's default name for that class. + """ + assert CompetencyCriterion.__name__ == "CompetencyCriterion" + assert CompetencyCriterion._meta.db_table == "openedx_learning_competencycriterion" + + group_field = CompetencyCriterion._meta.get_field("group") + assert group_field.null is False + assert group_field.db_column == "competency_criteria_group_id" + object_tag_field = CompetencyCriterion._meta.get_field("object_tag") + assert object_tag_field.null is False + assert object_tag_field.db_column == "oel_tagging_objecttag_id" -@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) + rule_profile_field = CompetencyCriterion._meta.get_field("rule_profile") + assert rule_profile_field.null is True + assert rule_profile_field.db_column == "competency_rule_profile_id" + assert CompetencyCriterion._meta.get_field("rule_type_override").null is True + assert CompetencyCriterion._meta.get_field("rule_payload_override").null is True -@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, + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile ) + assert criterion.rule_profile_id == default_rule_profile.pk + + +@pytest.mark.parametrize( + "model", + [CompetencyCriteriaGroup, CompetencyRuleProfile, CompetencyCriterion], + ids=["group", "rule_profile", "criterion"], +) +def test_uuid_is_a_stable_unique_non_editable_external_identifier(model: type[models.Model]) -> None: + """ + All three models carry a `uuid` external identifier: unique, not editable (so it can never be + set through a form), and defaulting to a freshly generated uuid4 for every new row. + """ + uuid_field = model._meta.get_field("uuid") + assert isinstance(uuid_field, models.UUIDField) + assert uuid_field.unique is True + assert uuid_field.editable is False + assert uuid_field.null is False + assert uuid_field.default is uuid_module.uuid4 + + +def test_group_has_no_columns_beyond_adr_decision_2() -> None: + """ + CompetencyCriteriaGroup's concrete field set is exactly {id, uuid, parent, tag, course, name, + ordering, logic_operator}: no more, no less. In particular, no `archived` column exists on + this model (that responsibility belongs to a later change; see the module's own history of + which ticket owns which model's archive column). + """ + concrete_field_names = {f.name for f in CompetencyCriteriaGroup._meta.get_fields() if f.concrete} + assert concrete_field_names == {"id", "uuid", "parent", "tag", "course", "name", "ordering", "logic_operator"} + + +def test_rule_profile_has_no_columns_beyond_adr_decision_3() -> None: + """ + CompetencyRuleProfile's concrete field set is exactly {id, organization, course, + competency_taxonomy, scope_code, rule_type, rule_payload, archived, uuid}: no more, no less. + """ + concrete_field_names = {f.name for f in CompetencyRuleProfile._meta.get_fields() if f.concrete} + assert concrete_field_names == { + "id", "organization", "course", "competency_taxonomy", "scope_code", "rule_type", "rule_payload", + "archived", "uuid", + } + + +def test_criterion_has_no_columns_beyond_adr_decision_4() -> None: + """ + CompetencyCriterion's concrete field set is exactly {id, uuid, group, object_tag, rule_profile, + rule_type_override, rule_payload_override}: no more, no less. In particular, no `archived` + column exists on this model. + """ + concrete_field_names = {f.name for f in CompetencyCriterion._meta.get_fields() if f.concrete} + assert concrete_field_names == { + "id", "uuid", "group", "object_tag", "rule_profile", "rule_type_override", "rule_payload_override", + } + + +# ============================================================================================== +# Constraints and validation (AC4, AC5, AC7, AC9, AC11, AC13, AC14, AC15). +# ============================================================================================== -def test_group_tree_and_logic_operator(tag: Tag) -> None: +def test_group_logic_operator_accepts_and_or_and_null_regardless_of_child_count(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. + logic_operator accepts AND, OR, or null. Nothing at the data layer constrains it by how many + children the group actually has: a group with zero children and a group with two children both + save successfully with any of the three values. See ADR-0002 Decision 2; the database cannot + see a group's future children at save time (a child's parent FK cannot point at a row that + doesn't have a primary key yet), so this is enforced nowhere at this layer, deliberately. + """ + for logic_operator in (LogicOperator.AND, LogicOperator.OR, None): + childless = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=logic_operator) + assert childless.pk is not None + + parent = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=logic_operator) + CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent) + CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent) + assert CompetencyCriteriaGroup.objects.filter(parent=parent).count() == 2 + + +def test_group_parent_and_child_relationship(tag: Tag) -> None: + """ + A CompetencyCriteriaGroup's parent is null for a root and points at its parent for a child. + 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 + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, logic_operator=LogicOperator.AND) + assert child.parent == root -def test_rule_profile_scope_check_constraint( - organization: Organization, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy +def test_group_has_no_unique_constraint_on_parent_and_ordering(tag: Tag) -> None: + """ + No UniqueConstraint on (parent, ordering) exists: two sibling groups may share the same + `ordering` value. A parent's clean() cannot see its own future children at save time (a + child's FK can't point at a not-yet-existing parent row), so there is no single-row state to + check a per-parent uniqueness rule against, and none is declared. See ADR-0002 Decision 2. + """ + unique_constraints = [ + c for c in CompetencyCriteriaGroup._meta.constraints if isinstance(c, models.UniqueConstraint) + ] + assert not any({"parent", "ordering"} <= set(c.fields) for c in unique_constraints) + + parent = CompetencyCriteriaGroup.objects.create(tag=tag) + sibling_a = CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent, ordering=1) + sibling_b = CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent, ordering=1) + assert sibling_a.ordering == sibling_b.ordering == 1 + + +@pytest.mark.parametrize( + "scope_kwargs", + [ + pytest.param({"organization": True}, id="organization_only"), + pytest.param({"course": True}, id="course_only"), + pytest.param({"competency_taxonomy": True}, id="competency_taxonomy_only"), + pytest.param({}, id="no_scope_system_default"), + ], +) +def test_rule_profile_scope_check_constraint_accepts_at_most_one_scope_field( + scope_kwargs: dict, + 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. + organization, course, or competency_taxonomy, including none of them (the system default). + 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. + # Free the all-null slot the seed migration (0003) occupies, so the "no scope" case can be + # tested in isolation from scope_code's own uniqueness constraint, which has 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: + resolved_kwargs: dict[str, object] = {} + if scope_kwargs.get("organization"): + resolved_kwargs["organization"] = organization + if scope_kwargs.get("course"): + resolved_kwargs["course"] = course_run + if scope_kwargs.get("competency_taxonomy"): + resolved_kwargs["competency_taxonomy"] = competency_taxonomy + + profile = CompetencyRuleProfile.objects.create( + rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD, **resolved_kwargs + ) + assert profile.pk is not None + + +@pytest.mark.parametrize( + "scoped_fields", + [ + pytest.param(("organization", "course"), id="organization_and_course"), + pytest.param(("organization", "competency_taxonomy"), id="organization_and_taxonomy"), + pytest.param(("course", "competency_taxonomy"), id="course_and_taxonomy"), + pytest.param(("organization", "course", "competency_taxonomy"), id="all_three"), + ], +) +def test_rule_profile_scope_check_constraint_rejects_more_than_one_scope_field( + scoped_fields: tuple[str, ...], + organization: Organization, + course_run: CourseRun, + competency_taxonomy: CompetencyTaxonomy, +) -> None: + """ + The scope check constraint rejects a CompetencyRuleProfile scoped to any two of organization, + course, and competency_taxonomy, or to all three. See ADR-0002 Decision 3. + """ + available_values = {"organization": organization, "course": course_run, "competency_taxonomy": competency_taxonomy} + scope_kwargs = {field_name: available_values[field_name] for field_name in scoped_fields} + + with pytest.raises(IntegrityError): 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( +def test_scope_code_matches_org_course_taxonomy_format_for_each_scope_shape( 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. + A live (non-archived) profile's scope_code is "org:X,course:Y,taxonomy:Z", with each segment + left blank when the corresponding scope column is null. See ADR-0002 Decision 3. """ CompetencyRuleProfile.objects.filter( organization__isnull=True, course__isnull=True, competency_taxonomy__isnull=True @@ -182,24 +351,53 @@ def test_scope_code_generated_value( assert taxonomy_only.scope_code == f"org:,course:,taxonomy:{competency_taxonomy.pk}" -def test_scope_code_uniqueness(organization: Organization) -> None: +def test_scope_code_is_null_once_archived_and_non_null_while_live(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. + scope_code is non-null while a profile is live, and becomes null once it is archived. An + archived profile no longer holds its scope's unique slot, which is what lets a replacement be + created for that same scope (see test_archiving_a_profile_frees_its_scope_for_a_replacement + below); a profile that stayed occupying a non-null scope_code after archiving would block that + forever. This is a deliberate design point, not an oversight: a plain nullable column, written + explicitly whenever a profile is saved, rather than a database-computed value that can never + tell "archived" apart from "live" on its own. """ - CompetencyRuleProfile.objects.create( + profile = 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 - ) + profile.refresh_from_db() + assert profile.scope_code == f"org:{organization.pk},course:,taxonomy:" + + profile.archived = True + profile.save() + profile.refresh_from_db() + assert profile.scope_code is None -def test_scope_code_unique_constraint_has_no_condition() -> None: +def test_archiving_a_profile_frees_its_scope_for_a_replacement(organization: Organization) -> None: + """ + Once a profile scoped to a given organization/course/taxonomy is archived, a brand new profile + may be created for that exact same scope: the archived row's scope_code goes to null and stops + occupying the unique slot, so it no longer collides with the replacement's non-null scope_code. + Before this, archiving a profile meant that scope could never be used again, since the archived + row's scope_code stayed non-null and permanently held the unique slot. + """ + original = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + original.archived = True + original.save() + + replacement = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + replacement.refresh_from_db() + original.refresh_from_db() + + assert original.scope_code is None + assert replacement.scope_code == f"org:{organization.pk},course:,taxonomy:" + + +def test_scope_code_unique_constraint_is_unconditional() -> None: """ No UniqueConstraint on CompetencyRuleProfile carries a `condition`. A conditional UniqueConstraint compiles to a partial index, which this project's MySQL backend does not @@ -214,37 +412,77 @@ def test_scope_code_unique_constraint_has_no_condition() -> None: assert constraint.condition is None -def test_criterion_profile_xor_override_constraint( - group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +def test_two_live_profiles_cannot_share_the_same_scope(organization: Organization) -> None: + """ + Two live 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 + ) + + +@pytest.mark.parametrize( + "invalid_kwargs", + [ + pytest.param( + {"rule_type_override": RuleType.GRADE, "rule_payload_override": _GRADE_PAYLOAD, "use_profile": True}, + id="both_set", + ), + pytest.param({"use_profile": False}, id="neither_set"), + pytest.param({"rule_payload_override": _GRADE_PAYLOAD, "use_profile": False}, id="only_payload_override_set"), + ], +) +def test_criterion_profile_xor_override_check_constraint_rejects_invalid_states( + invalid_kwargs: dict, + 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. Covers the three invalid states that reach the database's check constraint: both set, neither - set, and only rule_payload_override set. The fourth invalid state, only rule_type_override - set, is caught earlier by save()'s own validation instead and raises ValidationError before - the database is ever touched; see test_criterion_save_validates_override_payload_before_constraint + set, and only rule_payload_override set. The fourth invalid state, only rule_type_override set, + is caught earlier by save()'s own validation instead and raises ValidationError before the + database is ever touched; see test_criterion_save_validates_override_payload_before_constraint below for that case, and why it raises a different exception type than these three. """ - 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 + use_profile = invalid_kwargs.pop("use_profile") + kwargs = dict(invalid_kwargs) + if use_profile: + kwargs["rule_profile"] = default_rule_profile + + with pytest.raises(IntegrityError): + with transaction.atomic(): + CompetencyCriterion.objects.create(group=group, object_tag=object_tag, **kwargs) + + +def test_criterion_accepts_either_a_rule_profile_or_both_overrides( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Both valid states of the profile-xor-overrides check constraint save successfully: a + rule_profile with no overrides, and both override fields set with no rule_profile. + See ADR-0002 Decision 4. + """ + with_profile = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile ) + assert with_profile.pk is not None - 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_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) + with_overrides = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_type_override=RuleType.GRADE, rule_payload_override=_GRADE_PAYLOAD + ) + assert with_overrides.pk is not None def test_criterion_save_validates_override_payload_before_constraint( @@ -258,9 +496,9 @@ def test_criterion_save_validates_override_payload_before_constraint( for the same underlying reason (an override with no real payload), but save() never lets it get there. This is why two similar-looking invalid override states raise different exception types: this one is caught by save()'s validate_rule_payload call, while the other three (see - test_criterion_profile_xor_override_constraint above) reach the database's check constraint, - because the payload save() inspects for them is either valid or, when rule_type_override - itself is null, not inspected at all. + test_criterion_profile_xor_override_check_constraint_rejects_invalid_states above) reach the + database's check constraint, because the payload save() inspects for them is either valid or, + when rule_type_override itself is null, not inspected at all. """ with pytest.raises(ValidationError): CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_type_override=RuleType.GRADE) @@ -341,74 +579,125 @@ def test_criterion_full_clean_rejects_invalid_override_payload( criterion.full_clean() -def test_history_recorded_for_new_models_but_not_taxonomy( - organization: Organization, - group: CompetencyCriteriaGroup, - object_tag: ObjectTag, - default_rule_profile: CompetencyRuleProfile, +def test_rule_type_choices_match_rule_types_with_a_defined_payload_spec() -> None: + """ + RuleType's declared choices (what a serializer or an admin form offers an author) must contain + exactly the rule types that can actually be saved. ADR-0002 Decision 3 defines a rule_payload + shape per rule_type, and a rule_type with no defined shape is always rejected by + validate_rule_payload's "not supported yet" branch, regardless of payload content. RuleType + therefore declares only the rule types with a payload-spec entry (currently just Grade); a + future rule type not yet built (a "View" or "MasteryLevel") is neither a RuleType member nor a + declared choice until both its spec class and its RuleType member land together. This pins + that invariant so declaring a new RuleType member and forgetting its payload spec (or vice + versa) fails a test instead of shipping a dead-end choice. + """ + declared_rule_types = {choice_value for choice_value, _label in RuleType.choices} + enforced_rule_types = set(_RULE_PAYLOAD_SPECS.keys()) + assert declared_rule_types == enforced_rule_types + + +def test_criterion_rule_profile_is_not_recomputed_once_a_more_specific_profile_appears( + 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. + A criterion's stored rule_profile is not resolved dynamically at read time: creating a new, + more specific profile later does not silently re-govern a criterion that already resolved to a + less specific one. See ADR-0002 Decision 4, which lists the specific write events that DO + reassign a criterion (not exercised here) and states that no other path may recompute it. This + guards against a property, manager method, or signal handler being added that would violate + that rule by resolving the FK on every read instead of only at those write events. """ - historical_group = apps.get_model("openedx_learning", "HistoricalCompetencyCriteriaGroup") - historical_profile = apps.get_model("openedx_learning", "HistoricalCompetencyRuleProfile") - historical_criterion = apps.get_model("openedx_learning", "HistoricalCompetencyCriterion") + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) - group.name = "Poetry Mastery" - group.save() - assert historical_group.objects.filter(id=group.pk).count() == 2 + CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + criterion.refresh_from_db() + assert criterion.rule_profile_id == default_rule_profile.pk + + +# ============================================================================================== +# Scope immutability (AC11). Each scope field gets its own rejection test; rule_type, rule_payload, +# and archived changing on the same row is asserted separately as the case that must still work. +# ============================================================================================== + +def test_scope_immutability_rejects_organization_change( + organization: Organization, organization2: Organization +) -> None: + """ + Changing a CompetencyRuleProfile's `organization` after creation raises ValidationError on + save(). See ADR-0002 Decision 3. + """ 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 + profile.organization = organization2 + with pytest.raises(ValidationError): + profile.save() - 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_rejects_course_change(organization: Organization, course_run: CourseRun) -> None: + """ + Changing a CompetencyRuleProfile's `course` after creation raises ValidationError on save(). + See ADR-0002 Decision 3. + """ + other_catalog_course = CatalogCourse.objects.create(org=organization, course_code="Python200") + other_course_run = CourseRun.objects.create(catalog_course=other_catalog_course, run_code="Spring2027") + + profile = CompetencyRuleProfile.objects.create( + course=course_run, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.course = other_course_run + with pytest.raises(ValidationError): + profile.save() -def test_scope_immutability(organization: Organization, course_run: CourseRun) -> None: +def test_scope_immutability_rejects_taxonomy_change(competency_taxonomy: CompetencyTaxonomy) -> 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. + Changing a CompetencyRuleProfile's `competency_taxonomy` after creation raises ValidationError + on save(). See ADR-0002 Decision 3. """ + other_taxonomy = CompetencyTaxonomy.objects.create(name="Welding", export_id="welding-v1") + profile = CompetencyRuleProfile.objects.create( - organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD ) - profile.organization = None - profile.course = course_run + profile.competency_taxonomy = other_taxonomy with pytest.raises(ValidationError): profile.save() -def test_scope_immutability_with_deferred_load(organization: Organization, organization2: Organization) -> None: +def test_scope_immutability_allows_rule_type_rule_payload_and_archived_to_change(organization: Organization) -> None: + """ + Only rule_type, rule_payload, and archived may change after creation; changing any of them (as + opposed to a scope field) succeeds. See ADR-0002 Decision 3. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.rule_type = RuleType.GRADE + profile.rule_payload = {"op": "lte", "value": 0.5, "scale": "percent"} + profile.archived = True + profile.save() + + profile.refresh_from_db() + assert profile.rule_payload == {"op": "lte", "value": 0.5, "scale": "percent"} + assert profile.archived is True + + +def test_scope_immutability_enforced_after_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. + never loaded the scope columns into this instance in the first place. + _check_scope_immutable() always queries the persisted scope directly (see its docstring), so a + partial load is not a way to bypass this check. 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 @@ -419,14 +708,31 @@ def test_scope_immutability_with_deferred_load(organization: Organization, organ 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: +# NOTE: there is no test that `_check_scope_immutable()`'s fallback query targets +# `self._state.db` rather than the default alias. Proving that needs an instance loaded from a +# second database alias, and this suite configures only "default". Adding a second alias makes +# pytest-django run the whole migration history against it, which fails in +# openedx_content/backcompat/collections/migrations/0004_collection_key.py: its `generate_keys` +# RunPython step queries Collection.objects without `.using(schema_editor.connection.alias)`, so +# it always hits "default". That is a pre-existing bug in an unrelated app, but it breaks +# database setup for the entire session, not just this test. The alternative, assigning +# `instance._state.db` directly, is idiomatic in Django's own tests but trips this repo's +# enabled pylint `protected-access` check, and silencing that is not allowed. The one-line +# `.using(self._state.db)` in the model is correct by inspection; this is a known test gap. + + +# ============================================================================================== +# Indexes, history (AC16, AC19, AC20). +# ============================================================================================== + + +def test_database_indexes_from_adr_decision_5_are_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 @@ -460,7 +766,73 @@ def is_indexed(constraints: dict, columns: list[str]) -> bool: ) -def test_seeded_default_rule_profile_exists() -> None: +def test_history_recorded_for_group_profile_and_criterion( + organization: Organization, + group: CompetencyCriteriaGroup, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> 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. 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 + + +def test_history_not_recorded_for_tag_taxonomy_or_competencytaxonomy(competency_taxonomy: CompetencyTaxonomy) -> None: + """ + django-simple-history is NOT applied to oel_tagging_tag, oel_tagging_taxonomy, or + CompetencyTaxonomy: none of the three has a `.history` attribute, and no Historical* model is + registered for any of them. See ADR-0003 Decisions 1 and 2 for why history tracking stops at + the CBE-specific models and does not reach back into the generic tagging models they build on. + """ + assert not hasattr(Tag, "history") + assert not hasattr(Taxonomy, "history") + assert not hasattr(competency_taxonomy, "history") + + for app_label, model_name in [ + ("oel_tagging", "HistoricalTag"), + ("oel_tagging", "HistoricalTaxonomy"), + ("openedx_learning", "HistoricalCompetencyTaxonomy"), + ]: + with pytest.raises(LookupError): + apps.get_model(app_label, model_name) + + +# ============================================================================================== +# Migrations (AC10). AC21 (no makemigrations drift) and AC8 (this suite also runs against MySQL) +# are verified by running manage.py / the MySQL settings module, not by a unit test. +# ============================================================================================== + + +def test_migration_seeds_exactly_one_system_default_rule_profile() -> 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. diff --git a/tests/openedx_learning/applets/cbe/test_criteria_trees.py b/tests/openedx_learning/applets/cbe/test_criteria_trees.py new file mode 100644 index 000000000..1b1988744 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_trees.py @@ -0,0 +1,144 @@ +""" +Integrative tests for CompetencyAchievementCriteria trees. + +test_criteria_deletion.py proves each foreign key cascades or protects correctly in isolation. +That is not the same claim as "deleting somewhere in the middle of a realistic tree leaves exactly +the right rows behind and nothing else": a per-foreign-key test can pass while a wider tree still +ends up with an orphaned group, a criterion pointing at nothing, or a sibling branch disturbed by +a delete that should not have touched it. The tests here build a wider tree on purpose and assert +the full surviving/removed row set, not just that a cascade fired somewhere. + +Fixtures shared with test_criteria_models.py and test_criteria_deletion.py live in this directory's +conftest.py. +""" +import pytest + +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"} + + +def test_object_tag_delete_leaves_a_childless_criteria_group_behind( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting an ObjectTag cascades away the CompetencyCriterion that references it, but leaves the + CompetencyCriteriaGroup that housed that criterion in place, even when it was the group's only + criterion and the group now has no children of any kind (no criteria, no child groups). + + This is a deliberately accepted outcome, not a bug: CompetencyCriteriaGroup does not reference + ObjectTag at all (only CompetencyCriterion does), so nothing about deleting an ObjectTag gives + the collector a reason to reach the group. A childless group left behind this way is inert (it + evaluates no criteria and contributes nothing to its parent's logic_operator combination) and + is exactly the state authoring tooling must already handle for a group edited down to zero + children, so no additional cleanup path exists for this narrower case either. Pinned here so a + future change one way or the other (cascading the now-childless group away, or continuing to + leave it) is a deliberate decision, not an accidental side effect of something else. + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + object_tag.delete() + + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriteriaGroup.objects.get(pk=group.pk).criteria.exists() + + +def test_deleting_a_middle_group_removes_its_subtree_but_leaves_the_rest_of_the_tree_untouched( + tag: Tag, competency_taxonomy: CompetencyTaxonomy, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting a CompetencyCriteriaGroup partway down a realistic tree removes exactly that group, + every descendant beneath it, and every criterion under any of them -- and nothing else. A + sibling branch of the deleted group, with its own criterion, survives completely unchanged. + + Tree built here, all under one root: + + root + |-- branch_to_delete (criterion: profile-assigned, via default_rule_profile) + | `-- grandchild (criterion: override, no rule_profile) + `-- surviving_sibling (criterion: profile-assigned, via a taxonomy-scoped profile) + + `branch_to_delete` is deleted. This exercises criteria at two different tree depths (on + `branch_to_delete` itself and on its child `grandchild`) with a genuine mix of the two ways a + criterion can be governed (a stored `rule_profile` vs. per-criterion overrides), and confirms + `surviving_sibling` and its own criterion are byte-for-byte untouched: same primary keys, still + present, in a tree that shares a root with the subtree that just got removed. A test that only + checks "the deleted branch is gone" cannot tell a correct cascade apart from one that + over-deletes into a sibling it should never have reached; this test can. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag, name="root") + branch_to_delete = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, name="branch_to_delete") + grandchild = CompetencyCriteriaGroup.objects.create(tag=tag, parent=branch_to_delete, name="grandchild") + surviving_sibling = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, name="surviving_sibling") + + branch_object_tag = ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+branch", taxonomy=competency_taxonomy, tag=tag + ) + grandchild_object_tag = ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+grandchild", taxonomy=competency_taxonomy, tag=tag + ) + sibling_object_tag = ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+sibling", taxonomy=competency_taxonomy, tag=tag + ) + + taxonomy_scoped_profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + branch_criterion = CompetencyCriterion.objects.create( + group=branch_to_delete, object_tag=branch_object_tag, rule_profile=default_rule_profile + ) + grandchild_criterion = CompetencyCriterion.objects.create( + group=grandchild, + object_tag=grandchild_object_tag, + rule_type_override=RuleType.GRADE, + rule_payload_override=_GRADE_PAYLOAD, + ) + sibling_criterion = CompetencyCriterion.objects.create( + group=surviving_sibling, object_tag=sibling_object_tag, rule_profile=taxonomy_scoped_profile + ) + + all_group_pks = {root.pk, branch_to_delete.pk, grandchild.pk, surviving_sibling.pk} + all_criterion_pks = {branch_criterion.pk, grandchild_criterion.pk, sibling_criterion.pk} + existing_group_pks = set(CompetencyCriteriaGroup.objects.filter(pk__in=all_group_pks).values_list("pk", flat=True)) + existing_criterion_pks = set( + CompetencyCriterion.objects.filter(pk__in=all_criterion_pks).values_list("pk", flat=True) + ) + assert existing_group_pks == all_group_pks + assert existing_criterion_pks == all_criterion_pks + + branch_to_delete.delete() + + remaining_group_pks = set( + CompetencyCriteriaGroup.objects.filter(pk__in=all_group_pks).values_list("pk", flat=True) + ) + remaining_criterion_pks = set( + CompetencyCriterion.objects.filter(pk__in=all_criterion_pks).values_list("pk", flat=True) + ) + + # Exactly the root and the surviving sibling remain; the deleted branch and its child are gone. + assert remaining_group_pks == {root.pk, surviving_sibling.pk} + # Exactly the sibling's criterion remains; both criteria under the deleted branch are gone, + # regardless of whether they were profile-assigned or override-governed. + assert remaining_criterion_pks == {sibling_criterion.pk} + + # The surviving sibling and its criterion are not merely "still present somewhere" but the + # exact same rows, untouched by the delete of an unrelated branch under the same root. + surviving_sibling.refresh_from_db() + sibling_criterion.refresh_from_db() + assert surviving_sibling.parent_id == root.pk + assert sibling_criterion.group_id == surviving_sibling.pk + assert sibling_criterion.rule_profile_id == taxonomy_scoped_profile.pk