From ac725ed980c7607507bdcbdd9d030735bb4141f4 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 9 Sep 2026 18:08:42 -0400 Subject: [PATCH 1/3] 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 f06adb680..3bb7b4f1c 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", ] @@ -98,3 +106,153 @@ class Meta: # indexes every ForeignKey column by default, so a second explicit one here would only # cost write throughput without adding any read benefit. ] + + +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 c6fe507c55506385cc13d98325d047b561814bda Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Fri, 11 Sep 2026 10:02:18 -0400 Subject: [PATCH 2/3] test: move CompetencyRuleProfile's own delete tests down to this PR These six tests only need CompetencyRuleProfile, so they belong with the on_delete values this PR already declares, not in the cross-model part 8 suite they were drafted alongside. --- .../applets/cbe/test_rule_profile.py | 7 +- .../applets/cbe/test_rule_profile_deletion.py | 182 ++++++++++++++++++ 2 files changed, 186 insertions(+), 3 deletions(-) create mode 100644 tests/openedx_learning/applets/cbe/test_rule_profile_deletion.py diff --git a/tests/openedx_learning/applets/cbe/test_rule_profile.py b/tests/openedx_learning/applets/cbe/test_rule_profile.py index bf52132c8..d3d1afac4 100644 --- a/tests/openedx_learning/applets/cbe/test_rule_profile.py +++ b/tests/openedx_learning/applets/cbe/test_rule_profile.py @@ -9,9 +9,10 @@ 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. +Delete behavior is not covered here, except where a test frees the seeded system-default scope, +which nothing references. Nothing else in this module deletes a row that another row points at. +See test_rule_profile_deletion.py, in this same change, for this model's own `on_delete` values +and the tests that exercise them. Fixtures live in this directory's conftest.py. """ diff --git a/tests/openedx_learning/applets/cbe/test_rule_profile_deletion.py b/tests/openedx_learning/applets/cbe/test_rule_profile_deletion.py new file mode 100644 index 000000000..e252d9df9 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_rule_profile_deletion.py @@ -0,0 +1,182 @@ +""" +Delete-behavior tests for CompetencyRuleProfile's own foreign keys. + +| Foreign key | Value | Why | +| 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 | + +``on_delete`` expresses containment rather than protection (ADR-0002 Decision 7): it governs +deletion of the row a foreign key points *at*, never the row holding it. A CompetencyRuleProfile +is never hard-deleted by a *direct* delete of the profile itself; retirement is archive-only. +That does not stop it being cascaded away as a side effect of deleting the course or taxonomy it +is scoped to. + +Fixtures live in this directory's conftest.py. +""" +import pytest +from django.db import connection +from django.db.models import ProtectedError +from organizations.models import Organization + +from openedx_catalog.models import CourseRun +from openedx_learning.models import CompetencyRuleProfile, CompetencyTaxonomy, RuleType + +pytestmark = pytest.mark.django_db + +_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} + + +# --------------------------------------------------------------------------------------------- +# 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 +# test inspects 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. + """ + 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. 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. + """ + 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() + + +# --------------------------------------------------------------------------------------------- +# 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 `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_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 CASCADE foreign key, and shares the same + pre-delete-nulling collector path and the same scope_code collision this design avoids. + """ + 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() From 9e7cc98f11cae1b0c7b0fbd2884e985dbf5226cb Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Mon, 14 Sep 2026 11:54:00 -0400 Subject: [PATCH 3/3] fix: state the rule_payload shape in help_text instead of naming a function rule_payload's help_text told the reader to "see validate_rule_payload for the shape it must match". help_text renders in the Django admin and the DRF browsable API, so that is user-facing text pointing at an internal Python function the reader cannot open, which is what rule_payloads.py's own docstring forbids for the error messages beside it. It now states the shape. 0004 is amended in place because help_text is part of a field's deconstructed kwargs; makemigrations --check confirms no new migration is needed. Also cover the seeded system-default row against the payload contract. 0005_seed_default_rule_profile writes its payload as a literal and cannot check it: a historical migration must not import rule_payloads, and apps.get_model() returns a model without the custom clean(). Nothing else reconciled that literal with the validator, so tightening _validate_grade_payload would have left every deployment's default row invalid with no failing test. The new test is the one place the two meet. The neighbouring docstring credited the seeding to migration 0003, which is competencycriteriagroup. It is 0005. Co-Authored-By: Claude Opus 5 (1M context) --- .../applets/cbe/models/criteria.py | 5 ++++- .../migrations/0004_competencyruleprofile.py | 4 ++-- .../applets/cbe/test_rule_profile.py | 21 ++++++++++++++++++- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py index 3bb7b4f1c..cfad99f39 100644 --- a/src/openedx_learning/applets/cbe/models/criteria.py +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -163,7 +163,10 @@ class CompetencyRuleProfile(models.Model): ) 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.") + help_text=_( + 'Structured payload whose keys are set by rule_type. A "Grade" payload is ' + '{"op": "gte" | "lte" | "eq", "value": a fraction from 0.0 to 1.0, "scale": "percent"}.' + ) ) archived = models.BooleanField( default=False, diff --git a/src/openedx_learning/migrations/0004_competencyruleprofile.py b/src/openedx_learning/migrations/0004_competencyruleprofile.py index e351e3e2a..61dde047f 100644 --- a/src/openedx_learning/migrations/0004_competencyruleprofile.py +++ b/src/openedx_learning/migrations/0004_competencyruleprofile.py @@ -24,7 +24,7 @@ class Migration(migrations.Migration): ('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.')), + ('rule_payload', models.JSONField(help_text='Structured payload whose keys are set by rule_type. A "Grade" payload is {"op": "gte" | "lte" | "eq", "value": a fraction from 0.0 to 1.0, "scale": "percent"}.')), ('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)), @@ -50,7 +50,7 @@ class Migration(migrations.Migration): ('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.')), + ('rule_payload', models.JSONField(help_text='Structured payload whose keys are set by rule_type. A "Grade" payload is {"op": "gte" | "lte" | "eq", "value": a fraction from 0.0 to 1.0, "scale": "percent"}.')), ('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')), diff --git a/tests/openedx_learning/applets/cbe/test_rule_profile.py b/tests/openedx_learning/applets/cbe/test_rule_profile.py index d3d1afac4..13c461323 100644 --- a/tests/openedx_learning/applets/cbe/test_rule_profile.py +++ b/tests/openedx_learning/applets/cbe/test_rule_profile.py @@ -24,6 +24,7 @@ from organizations.models import Organization from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_learning.applets.cbe.rule_payloads import validate_rule_payload from openedx_learning.models import CompetencyRuleProfile, CompetencyTaxonomy, RuleType pytestmark = pytest.mark.django_db @@ -436,7 +437,7 @@ def test_scope_code_is_excluded_from_history() -> None: def test_migration_seeds_exactly_one_system_default_rule_profile() -> None: """ - Migration 0003 seeds exactly one system-default CompetencyRuleProfile: all three scope + Migration 0005 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( @@ -445,3 +446,21 @@ def test_migration_seeds_exactly_one_system_default_rule_profile() -> None: assert profile.archived is False assert profile.rule_type == RuleType.GRADE assert profile.rule_payload == _GRADE_PAYLOAD + + +def test_the_seeded_rule_payload_satisfies_the_payload_contract() -> None: + """ + The seeded system-default row's rule_payload passes validate_rule_payload. + + 0005_seed_default_rule_profile writes that payload as a literal and cannot check it itself: a + historical migration must not import rule_payloads, because that module changes while the + migration must not, and apps.get_model() returns a model reconstructed without the custom + clean(). This test is therefore the only place the seeded literal and the validator meet. + Without it, tightening _validate_grade_payload would leave the default row that every + deployment ships with invalid, and no test would fail. + """ + profile = CompetencyRuleProfile.objects.get( + organization__isnull=True, course__isnull=True, competency_taxonomy__isnull=True + ) + + validate_rule_payload(profile.rule_type, profile.rule_payload)