From b148157fd57c6ebe010c35cdee9e29e575376470 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 9 Sep 2026 09:34:46 -0400 Subject: [PATCH 1/4] feat: add the CBE rule payload contract ADR-0002 Decision 3 stores an evaluation rule as a rule_type plus a JSON rule_payload whose shape that type defines, rather than as fixed op/value/scale columns, so a future rule type can add its own fields without a migration. The cost of JSON is that nothing enforces the shape, so this adds the validator the two criteria models will call from clean(). Grade is the only supported type. Its value is a fraction from 0.0 to 1.0, not a number out of 100, which is the mistake an author is most likely to make, so the out-of-range message names the convention rather than only rejecting the value. RuleType declares exactly the types that have a payload spec, so a type can never be offered as a choice without being saveable. Refs #641 Co-Authored-By: Claude Opus 5 (1M context) --- .../applets/cbe/rule_payloads.py | 88 ++++++++++++ .../applets/cbe/test_rule_payloads.py | 130 ++++++++++++++++++ 2 files changed, 218 insertions(+) create mode 100644 src/openedx_learning/applets/cbe/rule_payloads.py create mode 100644 tests/openedx_learning/applets/cbe/test_rule_payloads.py 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..24205c4c5 --- /dev/null +++ b/src/openedx_learning/applets/cbe/rule_payloads.py @@ -0,0 +1,88 @@ +""" +Rule payload shapes for CBE evaluation rules, and the validator that checks a raw payload against +the shape its rule_type defines. See :ref:`openedx-learning-adr-0002` Decision 3 for the payload +contract. ``RuleType`` declares exactly the rule types with a shape defined here, so a rule type +can never be offered as a choice without also being saveable. These messages reach an API caller +or admin form, so they must not leak internal class or function names. +""" +from __future__ import annotations + +from typing import Any, Callable + +from django.core.exceptions import ValidationError +from django.db import models +from django.utils.translation import gettext_lazy as _ + +__all__ = [ + "RuleType", + "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") + + +_GRADE_OPERATORS = {"gte", "lte", "eq"} + + +def _validate_grade_payload(payload: dict) -> None: + """Validate a Grade payload's op, value, and scale. Keys are already checked.""" + if payload["op"] not in _GRADE_OPERATORS: + raise ValidationError(_("The 'op' in a 'Grade' rule_payload must be one of: gte, lte, eq.")) + value = payload["value"] + # isinstance(True, int) is True in Python, so a bool needs excluding explicitly. + if isinstance(value, bool) or not isinstance(value, (int, float)) or 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["scale"] != "percent": + raise ValidationError(_("The 'scale' in a 'Grade' rule_payload must be 'percent'.")) + + +# The required keys and validator for each rule type that has a defined payload shape. RuleType +# declares exactly these types, so a rule type can never be offered as a choice without being +# saveable. Adding one is an entry here, a validator, and the matching RuleType member. +_RULE_PAYLOAD_SPECS: dict[str, tuple[frozenset[str], Callable[[dict], None]]] = { + RuleType.GRADE: (frozenset({"op", "value", "scale"}), _validate_grade_payload), +} + + +def validate_rule_payload(rule_type: str, payload: Any) -> None: + """ + Raise ValidationError unless ``payload`` matches the shape ADR-0002 Decision 3 defines for + ``rule_type``, including when ``rule_type`` has no defined shape at all. + """ + spec = _RULE_PAYLOAD_SPECS.get(rule_type) + if spec 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} + ) + expected_keys, validate_values = spec + if not isinstance(payload, dict): + raise ValidationError(_("A '%(rule_type)s' rule_payload must be a JSON object.") % {"rule_type": rule_type}) + missing = sorted(expected_keys - payload.keys()) + unexpected = sorted(payload.keys() - expected_keys) + if missing or unexpected: + raise ValidationError( + _("A '%(rule_type)s' rule_payload has the wrong keys: missing %(missing)s; unexpected %(unexpected)s.") + % { + "rule_type": rule_type, + "missing": ", ".join(missing) or _("none"), + "unexpected": ", ".join(unexpected) or _("none"), + } + ) + validate_values(payload) diff --git a/tests/openedx_learning/applets/cbe/test_rule_payloads.py b/tests/openedx_learning/applets/cbe/test_rule_payloads.py new file mode 100644 index 000000000..705d1a204 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_rule_payloads.py @@ -0,0 +1,130 @@ +""" +Tests for the CBE rule payload contract: RuleType and validate_rule_payload. + +These need no database. validate_rule_payload is a plain function over a dict, and the models +that call it from clean() arrive in later changes, so every behavior here is exercised directly +rather than through a model save. + +ADR-0002 Decision 3 defines one payload shape per rule_type. The single supported type is +"Grade", whose payload is {"op": ..., "value": ..., "scale": ...} where op is one of gte, lte or +eq, value is a fraction from 0.0 to 1.0 rather than a number out of 100, and scale is "percent". +""" +import pytest +from django.core.exceptions import ValidationError + +# Private: the payload-spec registry, compared against RuleType's declared choices below. +from openedx_learning.applets.cbe.rule_payloads import _RULE_PAYLOAD_SPECS, RuleType, validate_rule_payload + +_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} + + +def test_a_well_formed_grade_payload_is_accepted() -> None: + """A Grade payload with a valid op, a fraction value, and the percent scale raises nothing.""" + validate_rule_payload(RuleType.GRADE, _GRADE_PAYLOAD) + + +@pytest.mark.parametrize( + "op", + [pytest.param("gte", id="gte"), pytest.param("lte", id="lte"), pytest.param("eq", id="eq")], +) +def test_every_documented_comparison_operator_is_accepted(op: str) -> None: + """All three operators ADR-0002 Decision 3 lists are accepted, not just the seeded gte.""" + validate_rule_payload(RuleType.GRADE, {**_GRADE_PAYLOAD, "op": op}) + + +@pytest.mark.parametrize( + "value", + [pytest.param(0.0, id="lower_bound"), pytest.param(1.0, id="upper_bound"), pytest.param(1, id="int_one")], +) +def test_the_ends_of_the_zero_to_one_range_are_accepted(value: float) -> None: + """0.0 and 1.0 are both inside the range, and an int is a number as far as this rule cares.""" + validate_rule_payload(RuleType.GRADE, {**_GRADE_PAYLOAD, "value": value}) + + +# One (rule_type, payload) pair per way ADR-0002 Decision 3 says a rule_payload can be invalid. +_INVALID_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_above_range"), + pytest.param(RuleType.GRADE, {"op": "gte", "value": -0.1, "scale": "percent"}, id="value_below_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"), + # "View" is a plain string, not RuleType.VIEW: RuleType declares only rule types that have a + # payload spec, so an unsupported rule type is by construction not a RuleType member at all. + pytest.param("View", _GRADE_PAYLOAD, id="unsupported_rule_type"), +] + + +@pytest.mark.parametrize("rule_type, payload", _INVALID_PAYLOADS) +def test_every_documented_way_a_payload_can_be_wrong_raises_validation_error( + rule_type: str, payload: object +) -> None: + """ + Each invalid shape raises ValidationError rather than passing or raising something the caller + would not expect: a bad op, a value given out of 100 instead of as a fraction, a value outside + the range at either end, a missing or extra key, a non-dict payload, a wrong scale, a boolean + masquerading as a number, and a rule_type with no defined payload shape. + """ + with pytest.raises(ValidationError): + validate_rule_payload(rule_type, payload) + + +def test_a_boolean_value_is_rejected_even_though_python_calls_it_an_int() -> None: + """ + True is rejected. isinstance(True, int) is True in Python, so a bool would slip through a + plain numeric check, and True would then read as the fraction 1.0, silently meaning "100%". + """ + with pytest.raises(ValidationError): + validate_rule_payload(RuleType.GRADE, {**_GRADE_PAYLOAD, "value": True}) + + +def test_an_out_of_range_value_message_names_the_fraction_convention() -> None: + """ + The message for a value given out of 100 (for example 80) names the 0.0 to 1.0 fraction + convention, so an author who wrote 80 meaning 80% is told what to write instead. This is the + single most likely authoring mistake for this payload. + """ + with pytest.raises(ValidationError) as exc_info: + validate_rule_payload(RuleType.GRADE, {"op": "gte", "value": 80, "scale": "percent"}) + + assert "fraction between 0.0 and 1.0" in " ".join(exc_info.value.messages) + + +def test_a_wrong_keys_message_names_the_offending_keys() -> None: + """ + The message for a wrong key set names both what is missing and what is unexpected, so an + author can see which key to fix rather than being told only that the payload is invalid. + """ + with pytest.raises(ValidationError) as exc_info: + validate_rule_payload(RuleType.GRADE, {"op": "gte", "extra": 1}) + + message = " ".join(exc_info.value.messages) + assert "extra" in message + assert "value" in message and "scale" in message + + +def test_an_unsupported_rule_type_says_only_grade_is_defined() -> None: + """ + A rule_type with no payload shape is rejected with a message saying so, rather than being + silently accepted. ADR-0002 Decision 3 lists View and MasteryLevel as future types; neither + has a defined shape yet. + """ + with pytest.raises(ValidationError) as exc_info: + validate_rule_payload("MasteryLevel", {"level": 3}) + + assert "not supported yet" in " ".join(exc_info.value.messages) + + +def test_rule_type_declares_exactly_the_types_that_have_a_payload_spec() -> None: + """ + RuleType's choices, which is what a serializer or admin form offers an author, contain exactly + the rule types that can actually be saved. + + A rule_type with no payload spec is always rejected regardless of payload content, so + declaring a RuleType member without its spec would offer an author a dead-end choice. This + pins the invariant so adding one without the other fails a test instead of shipping. + """ + assert {value for value, _label in RuleType.choices} == set(_RULE_PAYLOAD_SPECS) From d783944be0f146271c0ba7dc2be80da6c6811485 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 9 Sep 2026 18:08:42 -0400 Subject: [PATCH 2/4] feat: add CompetencyRuleProfile and seed the system default A reusable evaluation rule scoped to at most one of an organization, a course or a taxonomy, plus the one row scoped to none of them, which is the rule every criterion falls back to. A deployment that adds no profiles of its own gets an 80% threshold. See ADR-0002 Decision 3. Uniqueness per scope cannot be a plain constraint over the three nullable scope columns, because SQL never treats two NULLs as equal, and it cannot be a conditional constraint either, because MySQL has no partial unique indexes and Django silently skips creating one there. So the scope collapses into a derived scope_code column with one unconditional unique constraint. scope_code goes null while a profile is archived, which frees that scope for a replacement; the three scope columns are never cleared, so nothing is lost. Scope is immutable after creation, so criteria already resolved to a profile are never silently re-scoped. Deleting a scope owner takes the profile scoped to it, so course and competency_taxonomy cascade, per ADR-0002 Decision 7. organization does not: an Organization is not a competency definition record, and edx-organizations retires one by clearing its active flag rather than deleting the row, so PROTECT there refuses a delete that should not be happening. Refs #641 Co-Authored-By: Claude Opus 5 (1M context) --- .annotation_safe_list.yml | 2 + .../applets/cbe/models/__init__.py | 5 +- .../applets/cbe/models/criteria.py | 166 ++++++- .../migrations/0004_competencyruleprofile.py | 63 +++ .../0005_seed_default_rule_profile.py | 49 ++ .../openedx_learning/applets/cbe/conftest.py | 19 +- .../applets/cbe/test_rule_profile.py | 446 ++++++++++++++++++ 7 files changed, 744 insertions(+), 6 deletions(-) create mode 100644 src/openedx_learning/migrations/0004_competencyruleprofile.py create mode 100644 src/openedx_learning/migrations/0005_seed_default_rule_profile.py create mode 100644 tests/openedx_learning/applets/cbe/test_rule_profile.py diff --git a/.annotation_safe_list.yml b/.annotation_safe_list.yml index 2d5847b8f..662feb441 100644 --- a/.annotation_safe_list.yml +++ b/.annotation_safe_list.yml @@ -79,6 +79,8 @@ openedx_content.UnitVersion: ".. no_pii:": "This model has no PII" openedx_learning.HistoricalCompetencyCriteriaGroup: ".. 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/src/openedx_learning/applets/cbe/models/__init__.py b/src/openedx_learning/applets/cbe/models/__init__.py index fe57c998e..6775df566 100644 --- a/src/openedx_learning/applets/cbe/models/__init__.py +++ b/src/openedx_learning/applets/cbe/models/__init__.py @@ -2,11 +2,14 @@ Models for Competency-Based Education (CBE). """ +from ..rule_payloads import RuleType from .competency_taxonomy import CompetencyTaxonomy -from .criteria import CompetencyCriteriaGroup, LogicOperator +from .criteria import CompetencyCriteriaGroup, CompetencyRuleProfile, LogicOperator __all__ = [ "CompetencyCriteriaGroup", + "CompetencyRuleProfile", "CompetencyTaxonomy", "LogicOperator", + "RuleType", ] diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py index d987ef230..623dac686 100644 --- a/src/openedx_learning/applets/cbe/models/criteria.py +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -1,22 +1,30 @@ """ -The CompetencyAchievementCriteria tree: CompetencyCriteriaGroup, the internal AND/OR node. +The CompetencyAchievementCriteria models: CompetencyCriteriaGroup, the internal AND/OR node, +and CompetencyRuleProfile, the reusable evaluation rule its leaves draw from. -See :ref:`openedx-learning-adr-0002` Decision 2 for the design and Decision 7 for why every -foreign key here cascades, and :ref:`openedx-learning-adr-0003` Decisions 1 and 2 for why this -model carries ``django-simple-history`` tracking and CompetencyTaxonomy does not. +See :ref:`openedx-learning-adr-0002` Decisions 2 and 3 for the design and Decision 7 for each +foreign key's delete behavior, and :ref:`openedx-learning-adr-0003` Decisions 1 and 2 for why +these models carry ``django-simple-history`` tracking and CompetencyTaxonomy does not. """ from __future__ import annotations +from django.core.exceptions import ValidationError from django.db import models +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 from openedx_catalog.models import CourseRun from openedx_django_lib.fields import case_insensitive_char_field, immutable_uuid_field from openedx_tagging.models import Tag +from ..rule_payloads import RuleType, validate_rule_payload +from .competency_taxonomy import CompetencyTaxonomy + __all__ = [ "CompetencyCriteriaGroup", + "CompetencyRuleProfile", "LogicOperator", ] @@ -101,3 +109,153 @@ class Meta: # No constraint tying `logic_operator` to child count, and no UniqueConstraint on (parent, # ordering): a child group cannot be saved until its parent's primary key exists, so # neither has a single-row state to check at save time. See ADR-0002 Decision 2. + + +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. + + A profile's scope is immutable after creation; only ``rule_type``, ``rule_payload`` and + ``archived`` may change. + + .. no_pii: + """ + + uuid = immutable_uuid_field() + organization = models.ForeignKey( + Organization, + null=True, + blank=True, + 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, + on_delete=models.CASCADE, + 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, + on_delete=models.CASCADE, + related_name="rule_profiles", + help_text=_("The competency taxonomy this profile is scoped to, if any."), + ) + # Recomputed in save(), never set directly: null while archived, so any number of archived + # rows may share a scope while exactly one live row holds it, which is what lets an archived + # profile be replaced. See ADR-0002 Decision 3. + 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." + ), + ) + 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=_( + "Hides a profile from authoring and from new associations while keeping it queryable, so " + "criteria already assigned to it stay resolvable." + ), + ) + + # 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 = [ + # Unconditional, over the derived scope_code column rather than the raw nullable + # scope columns: MySQL has no partial unique indexes and Django silently skips + # creating one there. 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 + # 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." + ), + ), + 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." + ), + ), + ] + + def _check_scope_immutable(self) -> None: + """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 + # Queried rather than compared against a value cached at load time, so a deferred load or + # a refresh_from_db() cannot bypass the check. + persisted_scope = ( + CompetencyRuleProfile.objects.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 != persisted_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 _compute_scope_code(self) -> str | None: + """Return this profile's scope_code, or None while it is archived.""" + if self.archived: + return None + # A blank segment, not "None", for an unset scope: ADR-0002 Decision 3 fixes this format. + org, course, taxonomy = self.organization_id, self.course_id, self.competency_taxonomy_id + return f"org:{org or ''},course:{course or ''},taxonomy:{taxonomy or ''}" + + def save(self, *args, **kwargs): + """On save: recompute and validate scope_code.""" + self.scope_code = self._compute_scope_code() + # validate_unique() is already enforced by the database. + self.full_clean(validate_unique=False, validate_constraints=False) + super().save(*args, **kwargs) diff --git a/src/openedx_learning/migrations/0004_competencyruleprofile.py b/src/openedx_learning/migrations/0004_competencyruleprofile.py new file mode 100644 index 000000000..e351e3e2a --- /dev/null +++ b/src/openedx_learning/migrations/0004_competencyruleprofile.py @@ -0,0 +1,63 @@ +# Generated by Django 5.2.16 on 2026-09-10 18:33 + +import uuid + +import django.db.models.deletion +import simple_history.models +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_catalog', '0001_initial'), + ('openedx_learning', '0003_competencycriteriagroup'), + ('organizations', '0005_competencyruleprofile'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name='HistoricalCompetencyRuleProfile', + 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', 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='Hides a profile from authoring and from new associations while keeping it queryable, so criteria already assigned to it stay resolvable.')), + ('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.CreateModel( + name='CompetencyRuleProfile', + 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')), + ('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.', 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='Hides a profile from authoring and from new associations while keeping it queryable, so criteria already assigned to it stay resolvable.')), + ('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.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')), + ], + options={ + 'constraints': [models.UniqueConstraint(fields=('scope_code',), name='oel_cbe_ruleprofile_scope_code_uniq'), 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.'), 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.')], + }, + ), + ] diff --git a/src/openedx_learning/migrations/0005_seed_default_rule_profile.py b/src/openedx_learning/migrations/0005_seed_default_rule_profile.py new file mode 100644 index 000000000..3414b015e --- /dev/null +++ b/src/openedx_learning/migrations/0005_seed_default_rule_profile.py @@ -0,0 +1,49 @@ +""" +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 + +# 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:", + ) + + +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", "0004_competencyruleprofile"), + ] + + operations = [ + migrations.RunPython(seed_default_rule_profile, remove_default_rule_profile), + ] diff --git a/tests/openedx_learning/applets/cbe/conftest.py b/tests/openedx_learning/applets/cbe/conftest.py index 60fefbc79..fb25fd213 100644 --- a/tests/openedx_learning/applets/cbe/conftest.py +++ b/tests/openedx_learning/applets/cbe/conftest.py @@ -4,7 +4,7 @@ from organizations.models import Organization from openedx_catalog.models import CatalogCourse, CourseRun -from openedx_learning.models import CompetencyCriteriaGroup, CompetencyTaxonomy +from openedx_learning.models import CompetencyCriteriaGroup, CompetencyRuleProfile, CompetencyTaxonomy from openedx_tagging.models import Tag @@ -15,6 +15,13 @@ def _organization() -> Organization: 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.""" @@ -38,3 +45,13 @@ def _tag(competency_taxonomy: CompetencyTaxonomy) -> Tag: 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 0005.""" + return CompetencyRuleProfile.objects.get( + organization__isnull=True, + course__isnull=True, + competency_taxonomy__isnull=True, + ) diff --git a/tests/openedx_learning/applets/cbe/test_rule_profile.py b/tests/openedx_learning/applets/cbe/test_rule_profile.py new file mode 100644 index 000000000..bf52132c8 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_rule_profile.py @@ -0,0 +1,446 @@ +""" +Tests for CompetencyRuleProfile, the reusable evaluation rule a CompetencyCriterion draws from. + +Each test name states the behavior it pins. Reading top to bottom gives the model's contract: +its columns, the at-most-one-scope rule, how scope_code encodes that scope and what archiving +does to it, that the payload validator is wired into save(), that a profile's scope can never +change after creation, and the index, history and seeded row. + +The payload shapes themselves are covered exhaustively and without a database in +test_rule_payloads.py. What matters here is only that a model save reaches that validator. + +Delete behavior is not covered here and not decided here. Nothing in this module deletes a row +that another row points at, except where a test frees the seeded system-default scope, which +nothing references. See the change that settles delete behavior for those tests. + +Fixtures live in this directory's conftest.py. +""" +import pytest +from django.apps import apps +from django.core.exceptions import ValidationError +from django.db import connection, transaction +from django.db.utils import IntegrityError +from organizations.models import Organization + +from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_learning.models import CompetencyRuleProfile, CompetencyTaxonomy, RuleType + +pytestmark = pytest.mark.django_db + +_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} + + +# --------------------------------------------------------------------------------------------- +# Schema + + +# --------------------------------------------------------------------------------------------- + + +def test_rule_profile_has_exactly_the_columns_adr_0002_decision_3_lists() -> None: + """ + CompetencyRuleProfile's columns are exactly the ones ADR-0002 Decision 3 lists, with + `organization`, `course`, `competency_taxonomy`, and `scope_code` nullable and the rest + required. `scope_code` is nullable, not "never null": it is null exactly while a profile is + archived, which is what frees that scope's unique slot for a replacement. See ADR-0002 + Decision 3. + """ + fields = [f for f in CompetencyRuleProfile._meta.get_fields() if f.concrete] + assert {f.name for f in fields} == { + "id", "uuid", "organization", "course", "competency_taxonomy", "scope_code", "rule_type", + "rule_payload", "archived", + } + assert {f.name for f in fields if f.null} == {"organization", "course", "competency_taxonomy", "scope_code"} + assert CompetencyRuleProfile._meta.get_field("organization").remote_field.model is Organization + assert CompetencyRuleProfile._meta.get_field("course").remote_field.model is CourseRun + assert CompetencyRuleProfile._meta.get_field("competency_taxonomy").remote_field.model is CompetencyTaxonomy + + +# --------------------------------------------------------------------------------------------- +# Scope: at most one of organization, course, competency_taxonomy + + +# --------------------------------------------------------------------------------------------- + + +@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 (the system default). + See ADR-0002 Decision 3. + """ + # 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() + + 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) + + +# --------------------------------------------------------------------------------------------- +# scope_code: how a scope is encoded, and what archiving does to it +# ADR-0002 Decision 3. scope_code is a plain column recomputed in save(), and it goes null +# while a profile is archived. SQL never treats two NULLs as equal, so any number of archived +# rows may share a scope while exactly one live row holds it, which is what lets an archived +# profile be replaced. + + +# --------------------------------------------------------------------------------------------- + + +def test_scope_code_matches_org_course_taxonomy_format_for_each_scope_shape( + organization: Organization, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy +) -> None: + """ + 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 + ).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_is_null_once_archived_and_non_null_while_live(organization: Organization) -> None: + """ + 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. + """ + profile = 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_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. + """ + 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_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 + ) + + +# --------------------------------------------------------------------------------------------- +# Payload validation is wired into save() + + +# --------------------------------------------------------------------------------------------- + + +def test_saving_a_profile_with_an_invalid_payload_raises_validation_error() -> None: + """ + A profile whose rule_payload does not match its rule_type is rejected by full_clean(), which + save() calls, so objects.create() raises rather than writing a rule nothing can evaluate. + + This proves only the wiring. test_rule_payloads.py covers every way a payload can be wrong. + """ + with pytest.raises(ValidationError): + CompetencyRuleProfile.objects.create( + rule_type=RuleType.GRADE, rule_payload={"op": "gte", "value": 80, "scale": "percent"} + ) + + +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. Every other invalid-payload test here only asserts the exception + type; this one asserts the message content. + """ + 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 + + +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"). + """ + 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 + + +# --------------------------------------------------------------------------------------------- +# Scope immutability +# Editing a profile may change rule_type, rule_payload and archived only. Its scope is fixed +# at creation, so criteria already resolved to that scope are never silently re-governed. + + +# --------------------------------------------------------------------------------------------- + + +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.organization = organization2 + with pytest.raises(ValidationError): + profile.save() + + +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_rejects_taxonomy_change(competency_taxonomy: CompetencyTaxonomy) -> None: + """ + 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( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.competency_taxonomy = other_taxonomy + with pytest.raises(ValidationError): + profile.save() + + +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 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: a null scope would collide + with the seeded system-default row, so the unique constraint would raise IntegrityError and + the scope guard would never be reached. + """ + 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) + + deferred.organization = organization2 + with pytest.raises(ValidationError): + deferred.save() + + +# --------------------------------------------------------------------------------------------- +# Index 9, history, and the seeded system default + + +# --------------------------------------------------------------------------------------------- + + +def test_the_database_carries_adr_0002_decision_5_index_9_as_unique() -> None: + """ + The real table carries ADR-0002 Decision 5's index 9 on scope_code, and it is unique. A plain + index there would not enforce one profile per scope. + + The constraint is unconditional on purpose. A conditional UniqueConstraint compiles to a + partial index, which MySQL does not support: Django raises only a models.W036 warning and + silently skips creating it, while SQLite does support partial indexes and would hide the gap + in a local run. See ADR-0002 Rejected Alternative 6. + """ + with connection.cursor() as cursor: + constraints = connection.introspection.get_constraints( + cursor, CompetencyRuleProfile._meta.db_table + ) + + assert any(set(c["columns"]) == {"scope_code"} and c["unique"] for c in constraints.values()) + + +def test_editing_a_profile_writes_a_historical_row(organization: Organization) -> None: + """ + HistoricalRecords() is applied to CompetencyRuleProfile: creating then editing a profile + leaves two rows in the Historical model. See ADR-0003 Decision 1. + """ + historical_profile = apps.get_model("openedx_learning", "HistoricalCompetencyRuleProfile") + 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 + + +def test_scope_code_is_excluded_from_history() -> None: + """ + The Historical model does not track scope_code. It is a derived bookkeeping column, and the + columns it derives from (the three scope fields and archived) are tracked instead, which is + what an audit trail actually needs. + """ + historical_profile = apps.get_model("openedx_learning", "HistoricalCompetencyRuleProfile") + + assert "scope_code" not in {f.name for f in historical_profile._meta.get_fields()} + + +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. + """ + 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 From e5c161c50d963688921c89a4bda9a33edea3c03a Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 9 Sep 2026 17:47:01 -0400 Subject: [PATCH 3/4] feat: add CompetencyCriterion, the criteria tree's leaf A leaf points at one ObjectTag, meaning one specific piece of tagged content, and takes its pass rule either from a shared CompetencyRuleProfile or from its own inline override pair. A check constraint enforces ADR-0002 Decision 4's invariant: never both, never neither. The stored rule_profile is not resolved at read time. Decision 4 assigns it at four named write events and stores the result, so a criterion that already resolved to a less specific profile is not silently re-governed when a more specific one appears later. Computing that assignment is authoring-API work and is not here. group and object_tag cascade, per ADR-0002 Decision 7: a leaf means nothing without the group above it or the content association it evaluates. rule_profile is RESTRICT rather than PROTECT. Both refuse a direct profile delete while any criterion is assigned to it, which is what makes a profile archive-only at the ORM layer. They differ once the profile is deleted as part of a larger operation: PROTECT raises for any referencing row it finds in the database, so deleting a CompetencyTaxonomy would fail naming a criterion the same operation was already about to remove, while RESTRICT ignores rows that are themselves being deleted and lets that cascade through. Refs #641 Co-Authored-By: Claude Opus 5 (1M context) --- .annotation_safe_list.yml | 2 + .../applets/cbe/models/__init__.py | 3 +- .../applets/cbe/models/criteria.py | 101 +++++++- .../migrations/0006_competencycriterion.py | 61 +++++ .../openedx_learning/applets/cbe/conftest.py | 12 +- .../applets/cbe/test_criterion.py | 244 ++++++++++++++++++ 6 files changed, 417 insertions(+), 6 deletions(-) create mode 100644 src/openedx_learning/migrations/0006_competencycriterion.py create mode 100644 tests/openedx_learning/applets/cbe/test_criterion.py diff --git a/.annotation_safe_list.yml b/.annotation_safe_list.yml index 662feb441..65b803cd4 100644 --- a/.annotation_safe_list.yml +++ b/.annotation_safe_list.yml @@ -79,6 +79,8 @@ 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: diff --git a/src/openedx_learning/applets/cbe/models/__init__.py b/src/openedx_learning/applets/cbe/models/__init__.py index 6775df566..71b2bf501 100644 --- a/src/openedx_learning/applets/cbe/models/__init__.py +++ b/src/openedx_learning/applets/cbe/models/__init__.py @@ -4,10 +4,11 @@ from ..rule_payloads import RuleType from .competency_taxonomy import CompetencyTaxonomy -from .criteria import CompetencyCriteriaGroup, CompetencyRuleProfile, LogicOperator +from .criteria import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, LogicOperator __all__ = [ "CompetencyCriteriaGroup", + "CompetencyCriterion", "CompetencyRuleProfile", "CompetencyTaxonomy", "LogicOperator", diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py index 623dac686..c69709722 100644 --- a/src/openedx_learning/applets/cbe/models/criteria.py +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -1,8 +1,8 @@ """ -The CompetencyAchievementCriteria models: CompetencyCriteriaGroup, the internal AND/OR node, -and CompetencyRuleProfile, the reusable evaluation rule its leaves draw from. +The CompetencyAchievementCriteria models: CompetencyCriteriaGroup, CompetencyRuleProfile, and +CompetencyCriterion. -See :ref:`openedx-learning-adr-0002` Decisions 2 and 3 for the design and Decision 7 for each +See :ref:`openedx-learning-adr-0002` Decisions 2, 3 and 4 for the design and Decision 7 for each foreign key's delete behavior, and :ref:`openedx-learning-adr-0003` Decisions 1 and 2 for why these models carry ``django-simple-history`` tracking and CompetencyTaxonomy does not. """ @@ -17,13 +17,14 @@ from openedx_catalog.models import CourseRun from openedx_django_lib.fields import case_insensitive_char_field, immutable_uuid_field -from openedx_tagging.models import Tag +from openedx_tagging.models import ObjectTag, Tag from ..rule_payloads import RuleType, validate_rule_payload from .competency_taxonomy import CompetencyTaxonomy __all__ = [ "CompetencyCriteriaGroup", + "CompetencyCriterion", "CompetencyRuleProfile", "LogicOperator", ] @@ -259,3 +260,95 @@ def save(self, *args, **kwargs): # validate_unique() is already enforced by the database. self.full_clean(validate_unique=False, validate_constraints=False) super().save(*args, **kwargs) + + +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. + + When ``rule_type_override`` is set, its ``rule_payload_override``'s shape (see + :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: + """ + + uuid = immutable_uuid_field() + group = models.ForeignKey( + CompetencyCriteriaGroup, + db_column="competency_criteria_group_id", + 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.CASCADE, + 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", + on_delete=models.RESTRICT, + 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: + # No db_table override: the table is Django's default, openedx_learning_competencycriterion. + # verbose_name/verbose_name_plural are set explicitly because Django's default pluralization + # of "CompetencyCriterion" is "competency criterions". See ADR-0002 Decision 4. + 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) + + def save(self, *args, **kwargs): + """Persist this criterion, after full_clean() re-validates the override payload, if set.""" + self.full_clean(validate_unique=False, validate_constraints=False) + super().save(*args, **kwargs) diff --git a/src/openedx_learning/migrations/0006_competencycriterion.py b/src/openedx_learning/migrations/0006_competencycriterion.py new file mode 100644 index 000000000..047118390 --- /dev/null +++ b/src/openedx_learning/migrations/0006_competencycriterion.py @@ -0,0 +1,61 @@ +# Generated by Django 5.2.16 on 2026-09-10 18:34 + +import uuid + +import django.db.models.deletion +import simple_history.models +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0021_remove_system_defined_add_read_only'), + ('openedx_learning', '0005_seed_default_rule_profile'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + 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=[('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)), + ('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='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=[('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')), + ('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.RESTRICT, related_name='criteria', to='openedx_learning.competencyruleprofile')), + ], + options={ + 'verbose_name': 'Competency Criterion', + 'verbose_name_plural': 'Competency Criteria', + 'constraints': [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/tests/openedx_learning/applets/cbe/conftest.py b/tests/openedx_learning/applets/cbe/conftest.py index fb25fd213..58935b6f0 100644 --- a/tests/openedx_learning/applets/cbe/conftest.py +++ b/tests/openedx_learning/applets/cbe/conftest.py @@ -5,7 +5,7 @@ from openedx_catalog.models import CatalogCourse, CourseRun from openedx_learning.models import CompetencyCriteriaGroup, CompetencyRuleProfile, CompetencyTaxonomy -from openedx_tagging.models import Tag +from openedx_tagging.models import ObjectTag, Tag @pytest.fixture(name="organization") @@ -41,6 +41,16 @@ def _tag(competency_taxonomy: CompetencyTaxonomy) -> Tag: 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, 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.""" diff --git a/tests/openedx_learning/applets/cbe/test_criterion.py b/tests/openedx_learning/applets/cbe/test_criterion.py new file mode 100644 index 000000000..5acb4b236 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criterion.py @@ -0,0 +1,244 @@ +""" +Tests for CompetencyCriterion, a leaf of a CompetencyAchievementCriteria tree. + +Each test name states the behavior it pins. A leaf points at one ObjectTag, meaning one specific +piece of tagged content, and takes its pass rule either from a shared CompetencyRuleProfile or +from its own inline override pair, never from both and never from neither. + +Reading top to bottom gives the model's contract: its columns, the either-profile-or-overrides +invariant and every way it can be violated, that an override payload is validated on save, that +the stored profile is never re-resolved at read time, and its indexes and history. + +Delete behavior is not covered here and not decided here. Nothing in this module deletes a row +that another row points at. See the change that settles delete behavior for those tests, which +is also where the tree-wide integration tests live. + +Fixtures live in this directory's conftest.py. +""" +import pytest +from django.apps import apps +from django.core.exceptions import ValidationError +from django.db import connection, transaction +from django.db.utils import IntegrityError + +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + RuleType, +) +from openedx_tagging.models import ObjectTag + +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. +# test_rule_payloads.py covers these shapes directly; here they only have to reach clean(). +_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", "scale": "percent"}, id="missing_key"), + pytest.param(RuleType.GRADE, ["not", "a", "dict"], id="non_dict"), +] + + +# --------------------------------------------------------------------------------------------- +# Schema + + +# --------------------------------------------------------------------------------------------- + + +def test_criterion_has_exactly_the_columns_adr_0002_decision_4_lists() -> None: + """ + CompetencyCriterion's columns are exactly the ones ADR-0002 Decision 4 lists, with + `rule_profile`, `rule_type_override`, and `rule_payload_override` optional and the rest + required. No `archived` column yet; that arrives with #642. Carries no Meta.db_table + override, so the table is Django's default name for the class. + """ + fields = [f for f in CompetencyCriterion._meta.get_fields() if f.concrete] + assert {f.name for f in fields} == { + "id", "uuid", "group", "object_tag", "rule_profile", "rule_type_override", "rule_payload_override", + } + assert {f.name for f in fields if f.null} == {"rule_profile", "rule_type_override", "rule_payload_override"} + assert CompetencyCriterion._meta.get_field("group").db_column == "competency_criteria_group_id" + assert CompetencyCriterion._meta.get_field("object_tag").db_column == "oel_tagging_objecttag_id" + assert CompetencyCriterion._meta.get_field("rule_profile").db_column == "competency_rule_profile_id" + assert CompetencyCriterion._meta.db_table == "openedx_learning_competencycriterion" + + +# --------------------------------------------------------------------------------------------- +# Either a rule_profile or both overrides. Never both, never neither. +# ADR-0002 Decision 4. Three of the four invalid states reach the database check constraint +# and raise IntegrityError. The fourth, rule_type_override set with no payload, is caught +# earlier by save()'s payload validation and raises ValidationError instead. + + +# --------------------------------------------------------------------------------------------- + + +@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_setting_a_rule_type_override_without_a_payload_is_rejected_by_save + below for that case, and why it raises a different exception type than these three. + """ + 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 + + 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_setting_a_rule_type_override_without_a_payload_is_rejected_by_save( + 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_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) + + +# --------------------------------------------------------------------------------------------- +# Override payload validation, and the profile that is never re-resolved + + +# --------------------------------------------------------------------------------------------- + + +@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_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: + """ + 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. + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + + 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 + + +# --------------------------------------------------------------------------------------------- +# Indexes 4 and 5, and history + + +# --------------------------------------------------------------------------------------------- + + +def test_the_database_carries_adr_0002_decision_5_indexes_4_and_5() -> None: + """ + The real table carries ADR-0002 Decision 5's index 4 on object_tag and index 5 on group. Both + come from Django's automatic per-ForeignKey index rather than an explicit models.Index, so + this introspects the database rather than the model and holds either way. + """ + with connection.cursor() as cursor: + constraints = connection.introspection.get_constraints(cursor, CompetencyCriterion._meta.db_table) + + def is_indexed(columns: list[str]) -> bool: + return any(c["columns"] == columns and c["index"] for c in constraints.values()) + + assert is_indexed(["oel_tagging_objecttag_id"]) + assert is_indexed(["competency_criteria_group_id"]) + + +def test_editing_a_criterion_writes_a_historical_row( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + HistoricalRecords() is applied to CompetencyCriterion: creating a criterion and then switching + it from a profile to overrides leaves two rows in the Historical model. See ADR-0003 + Decision 1, and Decision 4 for why that switch is an authoring event worth recording. + """ + historical_criterion = apps.get_model("openedx_learning", "HistoricalCompetencyCriterion") + 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 From f9c8ae48b449dac6c9589a2bff49406b2f8492a8 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 10 Sep 2026 14:37:50 -0400 Subject: [PATCH 4/4] test: cover the competency criteria delete paths end to end Each model now declares its own on_delete values, so what is left to prove is the behavior they produce together, which no single model's tests can reach: that deleting a Tag or a Taxonomy takes the whole authored tree with it, that a scope owner's deletion carries its profile away, and that a direct profile delete is still refused. Two cases turn on RESTRICT rather than PROTECT on CompetencyCriterion. rule_profile. Deleting a CompetencyTaxonomy whose scoped profile is assigned to a criterion now succeeds, because the same operation is already deleting that criterion through the tag chain; PROTECT raised there, naming a criterion that was about to be removed anyway. Deleting a CourseRun is still refused when the criterion assigned to its scoped profile sits in a tree with no course scope, which Decision 4 permits, because that criterion really would be left pointing at a deleted profile. The deletion paths also run under MySQL's collector semantics while still on SQLite, by setting can_defer_constraint_checks to false, so the pre-delete nulling of a nullable cascading foreign key is exercised in the fast local suite rather than only in CI. Refs #641 Co-Authored-By: Claude Opus 5 (1M context) --- .../applets/cbe/test_criteria_deletion.py | 544 ++++++++++++++++++ .../applets/cbe/test_criteria_trees.py | 144 +++++ 2 files changed, 688 insertions(+) create mode 100644 tests/openedx_learning/applets/cbe/test_criteria_deletion.py create mode 100644 tests/openedx_learning/applets/cbe/test_criteria_trees.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..3c8b64e23 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py @@ -0,0 +1,544 @@ +""" +Delete-behavior tests for the three CompetencyAchievementCriteria models. + +Every test that deletes a row another row points at lives here. Each model declares its own +``on_delete`` values in the change that adds it; this module is where the behavior those values +produce, especially across more than one model, is pinned. + +ADR-0002 Decision 7 in one sentence: ``on_delete`` expresses containment rather than protection. +It governs deletion of the row a foreign key points *at*, never the row holding it, so the seven +cascading edges are how Django's collector walks *down* the tree, and the two remaining edges are +what refuse a delete outright. + +| Foreign key | Value | Why | +| CompetencyCriteriaGroup.parent | CASCADE | a subtree is meaningless without its parent | +| CompetencyCriteriaGroup.tag | CASCADE | a criteria tree is meaningless without its competency | +| CompetencyCriteriaGroup.course | CASCADE | a course-scoped tree is meaningless without its run | +| CompetencyRuleProfile.organization | PROTECT | an Organization is not a competency record | +| CompetencyRuleProfile.course | CASCADE | a course-scoped profile goes with its run | +| CompetencyRuleProfile.competency_taxonomy | CASCADE | a taxonomy-scoped profile goes with its taxonomy | +| CompetencyCriterion.group | CASCADE | a leaf is meaningless without its group | +| CompetencyCriterion.object_tag | CASCADE | a leaf is meaningless without its content association | +| CompetencyCriterion.rule_profile | RESTRICT | a profile is never hard-deleted out from under a leaf | + +``rule_profile`` is RESTRICT rather than PROTECT because the two differ exactly where it matters +here. Both refuse a direct profile delete while a criterion is assigned to it. Only RESTRICT +ignores referencing rows that the same operation is already deleting, which is what lets a scope +owner's deletion carry its profile away instead of failing on a criterion that delete was about to +remove anyway. + +Only the cascade half of each case is asserted. Every matching "raises ProtectedError because a +learner status row exists" case needs #642's three Student*Status tables, and #642 is the change +that creates them, so those assertions belong there. Nothing here stubs or fakes a status model +to stand in for them. Until #642 merges, main carries a cascade chain with no PROTECT at the +bottom, so deleting a tag removes the whole authored tree and nothing objects. That window is +expected and harmless, because the learner status tables do not exist yet. + +Fixtures 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, RestrictedError +from organizations.models import Organization + +from openedx_catalog.models import 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"} + + +# --------------------------------------------------------------------------------------------- +# CompetencyCriteriaGroup's three foreign keys +# Each CASCADE test asserts the referencing row existed beforehand and is gone afterward, not +# merely that no exception was raised. + + +# --------------------------------------------------------------------------------------------- + + +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. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + assert CompetencyCriteriaGroup.objects.filter(pk=child.pk).exists() + + root.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=root.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=child.pk).exists() + + +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 + 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 + + 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_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() + + course_run.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + + +def test_a_cascaded_group_removal_is_recorded_in_history(tag: Tag) -> None: + """ + A group removed by a cascade, rather than by a direct delete, still gets its own historical + row with history_type '-'. An author or auditor reviewing history for a group that vanished + this way still finds why it did. + """ + historical_group = apps.get_model("openedx_learning", "HistoricalCompetencyCriteriaGroup") + group = CompetencyCriteriaGroup.objects.create(tag=tag) + group_pk = group.pk + + tag.delete() + + assert historical_group.objects.filter(id=group_pk, history_type="-").exists() + + +# --------------------------------------------------------------------------------------------- +# CompetencyRuleProfile's three foreign keys +# A profile is never hard-deleted by a direct delete; retirement is an archive. That does not +# stop a profile being cascaded away with the course or taxonomy it is scoped to. The PROTECT and +# RESTRICT tests inspect the exception's collected objects rather than only catching the +# exception, because several such relationships can fire on one delete. + + +# --------------------------------------------------------------------------------------------- + + +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. + + 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_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() + + course_run.delete() + + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + +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 scoped to it via + `competency_taxonomy`: the delete succeeds and the profile row is gone too, as #641 + requires. 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 + ) + assert CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + competency_taxonomy.delete() + + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + +# --------------------------------------------------------------------------------------------- +# CompetencyCriterion's three foreign keys + + +# --------------------------------------------------------------------------------------------- + + +def test_deleting_a_group_also_deletes_its_criteria( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + 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() + + group.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +def test_deleting_an_object_tag_also_deletes_its_criteria( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + 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() + + object_tag.delete() + + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +def test_deleting_a_rule_profile_referenced_by_a_criterion_raises_restricted_error( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting a CompetencyRuleProfile that a CompetencyCriterion references via `rule_profile` + raises RestrictedError, which is what holds ADR-0002 Decision 7's "a profile is never + hard-deleted by a direct delete" at the ORM layer. + + Nothing cascades from a profile down to a criterion, so the criterion is not part of this + delete and RESTRICT refuses, exactly as PROTECT would have. + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + + with pytest.raises(RestrictedError) as exc_info: + default_rule_profile.delete() + + restricted = exc_info.value.restricted_objects + assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in restricted) + + +# --------------------------------------------------------------------------------------------- +# Transitive deletes required by issue #641 +# Deleting a Tag, a group at depth, or a Taxonomy takes the whole referencing criteria tree +# with it. Tag.taxonomy is already CASCADE in openedx_tagging, which is what makes the tag +# case hold transitively from a taxonomy. + + +# --------------------------------------------------------------------------------------------- + + +def test_tag_delete_with_no_status_cascades_whole_criteria_tree( + tag: Tag, group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + 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). + """ + 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() + + 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() + + +# --------------------------------------------------------------------------------------------- +# Scope-owner deletes that reach a profile a criterion is assigned to +# These are what RESTRICT on CompetencyCriterion.rule_profile buys, and what it still refuses. +# Both are unreachable until a taxonomy- or course-scoped profile can be authored, which no code +# path does yet. See ADR-0002 Decision 7. + + +# --------------------------------------------------------------------------------------------- + + +def test_taxonomy_delete_reaching_its_scoped_profile_through_a_criterion_succeeds( + competency_taxonomy: CompetencyTaxonomy, group: CompetencyCriteriaGroup, object_tag: ObjectTag +) -> None: + """ + Deleting a CompetencyTaxonomy whose taxonomy-scoped profile is itself assigned to a criterion + succeeds, and takes the profile and the criterion with it. + + This is the case RESTRICT exists for. The delete reaches the profile through + `competency_taxonomy` (CASCADE) and reaches the criterion through the tag chain + (Tag -> CompetencyCriteriaGroup.tag -> CompetencyCriterion.group, all CASCADE). RESTRICT then + finds nothing left restricting the profile, because the only row referencing it is one this + same operation is already deleting. Under PROTECT this raised ProtectedError naming that + criterion, which was a spurious failure: an author deleting a taxonomy was told a criterion + was in the way, when nothing about that criterion survived the delete either. + """ + 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) + + competency_taxonomy.delete() + + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + + +def test_course_run_delete_is_refused_by_a_criterion_outside_its_scope( + course_run: CourseRun, group: CompetencyCriteriaGroup, object_tag: ObjectTag +) -> None: + """ + Deleting a CourseRun whose course-scoped profile is assigned to a criterion that the same + delete does NOT reach raises RestrictedError, and nothing is removed. + + A criterion's profile assignment is independent of its tree's `course` scope (ADR-0002 + Decision 4), so a criterion in a tree with `course=None`, which `group` is, can still be + assigned a course-scoped profile. Deleting that run collects the profile but not the + criterion, so RESTRICT correctly refuses: unlike the taxonomy case above, this criterion + really would have been left pointing at a deleted profile. ADR-0002 Decision 7 records this + as the residual case, whose fix is a fifth reassignment event on Decision 4. + """ + profile = CompetencyRuleProfile.objects.create( + course=course_run, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + criterion = CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=profile) + assert group.course is None + + with pytest.raises(RestrictedError) as exc_info: + course_run.delete() + + restricted = exc_info.value.restricted_objects + assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in restricted) + # Nothing was 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() + assert CourseRun.objects.filter(pk=course_run.pk).exists() + + +# --------------------------------------------------------------------------------------------- +# MySQL collector semantics, reproduced on SQLite +# MySQL cannot defer foreign-key constraint checks, and Django's CASCADE handler reads that +# flag directly: it nulls a nullable cascading foreign key before the DELETE. On SQLite that +# nulling never happens, so the tests below monkeypatch the flag to reproduce it. Without the +# monkeypatch they pass against broken and correct code alike, so do not drop it. This is also +# why CompetencyRuleProfile.scope_code is a plain column rather than a GeneratedField: a +# generated column would recompute from the nulled scope foreign key mid-cascade and collide +# with whichever row already holds the resulting blank scope. + + +# --------------------------------------------------------------------------------------------- + + +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 cascading foreign keys, so it shares the exact pre-delete-nulling collector path the + taxonomy case below 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_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). Nulling the profile's `competency_taxonomy_id` before deleting it leaves + `scope_code` alone, so it cannot collide with the seeded system-default profile's identical + blank scope and raise IntegrityError instead of completing the cascade. + """ + 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_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() 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