From 2883edfb446da8635ed593639502281078617dc4 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 1 Oct 2026 10:25:08 -0400 Subject: [PATCH 1/3] feat: add competency criteria models for CBE authoring layer Add the CBE criteria models: CompetencyRuleProfile, CompetencyCriteriaGroup and CompetencyCriterion (the criteria tree's AND/OR node and leaf), the rule payload contract and its validation, and CompetencyTaxonomy.taxonomy_overrides_org. Delete behavior cascades from tags, groups and object tags. Register django-simple-history, layer openedx_catalog beside openedx_content in the import contracts, and record the scope_code and on_delete decisions in ADR-0002. Co-Authored-By: Claude Sonnet 5.5 --- .annotation_safe_list.yml | 6 + .../0002-competency-criteria-model.rst | 14 +- mypy.ini | 3 + projects/dev.py | 4 + .../applets/containers/models.py | 2 +- .../applets/cbe/models/__init__.py | 16 + .../competency_taxonomy.py} | 14 +- .../applets/cbe/models/criteria.py | 347 +++++++++++++++ .../applets/cbe/rule_payloads.py | 109 +++++ ...mpetencytaxonomy_taxonomy_overrides_org.py | 18 + .../0003_competencycriteriagroup.py | 65 +++ .../migrations/0004_competencyruleprofile.py | 63 +++ .../0005_seed_default_rule_profile.py | 49 +++ .../migrations/0006_competencycriterion.py | 61 +++ ...0007_alter_criterion_override_help_text.py | 33 ++ test_settings.py | 3 + .../openedx_learning/applets/cbe/conftest.py | 67 +++ .../applets/cbe/test_criteria_group.py | 108 +++++ .../cbe/test_criteria_group_deletion.py | 104 +++++ .../applets/cbe/test_criteria_trees.py | 104 +++++ .../applets/cbe/test_criterion.py | 158 +++++++ .../applets/cbe/test_criterion_deletion.py | 250 +++++++++++ .../applets/cbe/test_models.py | 8 + .../applets/cbe/test_rule_payloads.py | 130 ++++++ .../applets/cbe/test_rule_profile.py | 408 ++++++++++++++++++ .../applets/cbe/test_rule_profile_deletion.py | 113 +++++ 26 files changed, 2251 insertions(+), 6 deletions(-) create mode 100644 src/openedx_learning/applets/cbe/models/__init__.py rename src/openedx_learning/applets/cbe/{models.py => models/competency_taxonomy.py} (72%) create mode 100644 src/openedx_learning/applets/cbe/models/criteria.py create mode 100644 src/openedx_learning/applets/cbe/rule_payloads.py create mode 100644 src/openedx_learning/migrations/0002_competencytaxonomy_taxonomy_overrides_org.py create mode 100644 src/openedx_learning/migrations/0003_competencycriteriagroup.py 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 src/openedx_learning/migrations/0006_competencycriterion.py create mode 100644 src/openedx_learning/migrations/0007_alter_criterion_override_help_text.py create mode 100644 tests/openedx_learning/applets/cbe/conftest.py create mode 100644 tests/openedx_learning/applets/cbe/test_criteria_group.py create mode 100644 tests/openedx_learning/applets/cbe/test_criteria_group_deletion.py create mode 100644 tests/openedx_learning/applets/cbe/test_criteria_trees.py create mode 100644 tests/openedx_learning/applets/cbe/test_criterion.py create mode 100644 tests/openedx_learning/applets/cbe/test_criterion_deletion.py create mode 100644 tests/openedx_learning/applets/cbe/test_rule_payloads.py create mode 100644 tests/openedx_learning/applets/cbe/test_rule_profile.py create mode 100644 tests/openedx_learning/applets/cbe/test_rule_profile_deletion.py diff --git a/.annotation_safe_list.yml b/.annotation_safe_list.yml index 6b9f74d07..65b803cd4 100644 --- a/.annotation_safe_list.yml +++ b/.annotation_safe_list.yml @@ -77,6 +77,12 @@ openedx_content.Unit: ".. no_pii:": "This model has no PII" openedx_content.UnitVersion: ".. no_pii:": "This model has no PII" +openedx_learning.HistoricalCompetencyCriteriaGroup: + ".. no_pii:": "This model has no PII" +openedx_learning.HistoricalCompetencyCriterion: + ".. no_pii:": "This model has no PII" +openedx_learning.HistoricalCompetencyRuleProfile: + ".. no_pii:": "This model has no PII" social_django.Association: ".. no_pii:": "This model has no PII" social_django.Code: diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index a4d60c509..c0e258cf5 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -120,8 +120,8 @@ Decision Relationship to other concepts: - Each row is scoped by at most one of taxonomy, course, or organization (or by none, for the system default). A check constraint enforces that at most one of ``organization_id``, ``course_id``, and ``competency_taxonomy_id`` is non-null per row. See Decision 4 for how a criterion is assigned a profile when rows in more than one of these scopes could apply to it. - - At most one profile row may exist per distinct scope value. This is enforced by a unique constraint on the generated ``scope_code`` column (Decision 5), not a plain unique constraint on the three raw scope columns; see the ``scope_code`` column definition below for why. - - The system default is the single profile row where all three scope fields are null. ``scope_code`` is never null, including for this row (see below), so its singularity is enforced by the same unique constraint as every other profile rather than a separate procedural guarantee; it is seeded once via migration and never created or deleted through the profile API. If/when a REST API or application-layer/service code exists for editing a profile's ``rule_type``/``rule_payload``, the system default row would be editable through it like any other profile. Until then, only an operator can edit it directly (for example via Django admin or SQL). + - At most one profile row may exist per distinct scope value. This is enforced by a unique constraint on the derived ``scope_code`` column (Decision 5), not a plain unique constraint on the three raw scope columns; see the ``scope_code`` column definition below for why. + - The system default is the single profile row where all three scope fields are null. ``scope_code`` is non-null for this row to not collide with archived rule profiles. - Is referenced by ``CompetencyCriterion``, which may override its type/payload. - Never hard-deleted; retirement is archive-only (Decision 7). @@ -131,7 +131,7 @@ Decision 2. ``organization_id``: The ``organization_id`` of the organization that this competency rule profile is scoped to. Null if it is not scoped to a specific organization. 3. ``course_id``: The ``course_id`` of the course that this competency rule profile is scoped to. Null if it is not scoped to a specific course. 4. ``competency_taxonomy_id``: The ``CompetencyTaxonomy.taxonomy_ptr_id`` of the competency taxonomy that this competency rule profile is scoped to. Null if it is not scoped to a specific taxonomy. - 5. ``scope_code``: A database-generated column that is always in the fixed, trivially-parseable format ``"org:X,course:Y,taxonomy:Z"``, with each segment left blank when the corresponding scope column is null: for example ``"org:5,course:,taxonomy:"``, ``"org:,course:12,taxonomy:"``, ``"org:,course:,taxonomy:7"``, or ``"org:,course:,taxonomy:"`` for the system default. ``scope_code`` is therefore never null, including for the system default row. This exists because SQL never treats two ``NULL`` values as equal for uniqueness purposes, so a plain unique constraint across the three nullable scope columns would not stop two rows from sharing the same scope (for example two rows both with ``organization_id=5`` and the other two columns null). Collapsing the scope into one generated, always-non-null column sidesteps that, and does so identically on every database backend this project supports, including MySQL, which does not support the conditional/partial unique indexes that would otherwise be the usual fix. ``scope_code`` embeds internal ID references and exists solely to enforce uniqueness; it is not intended to be exported or exposed outside this system. + 5. ``scope_code``: A plain column in the format ``"org:X,course:Y,taxonomy:Z"``, with each segment left blank when the corresponding scope column is null: for example ``"org:5,course:,taxonomy:"``, or ``"org:,course:,taxonomy:"`` for the system default row. It is non-null when it is live, and null while archived. This frees an archived profile's scope for a replacement. 6. ``rule_type``: “View”, “Grade”, “MasteryLevel” (Only “Grade” will be supported for now) 7. ``rule_payload``: JSON payload keyed by ``rule_type`` to avoid freeform strings. It is structured JSON (not arbitrary freeform data): each ``rule_type`` defines the allowed payload shape and required keys, and validation enforces this contract. JSON is used instead of fixed columns like ``op``, ``value``, and ``scale`` so that future rule types (for example, ``MasteryLevel`` thresholds or plugin-defined evaluators such as CEL-based rules) can add their own fields without repeated schema migrations or many nullable columns. Examples: @@ -417,7 +417,7 @@ Rejected Alternatives 2. Requires reconciling profiles whenever an organization is added to or removed from a taxonomy. 3. Organization and taxonomy are not naturally nested (a taxonomy can belong to many organizations and vice versa), so forcing one to always contain the other does not reflect the actual relationship between them. -6. Enforce ``CompetencyRuleProfile`` scope uniqueness with per-scope conditional/partial unique constraints (Django ``UniqueConstraint(condition=Q(...))``) directly on the three nullable scope columns, instead of a generated ``scope_code`` column (Decision 3). +6. Enforce ``CompetencyRuleProfile`` scope uniqueness with per-scope conditional/partial unique constraints (Django ``UniqueConstraint(condition=Q(...))``) directly on the three nullable scope columns, instead of the derived ``scope_code`` column (Decision 3). 1. Pros @@ -452,3 +452,9 @@ Changelog course-scoped subtree, not just the top one, and can't change after creation. Simplified retrieval scope and dropped the pagination note, since both assumed course-date windowing, which #676's new read path doesn't use. + +2026-09-09: + +* ``scope_code`` on ``CompetencyRuleProfile`` is now computed by + application code instead of being database-generated, and is set to null while a + profile is archived, freeing its scope for a replacement. diff --git a/mypy.ini b/mypy.ini index b383a8816..665714903 100644 --- a/mypy.ini +++ b/mypy.ini @@ -12,5 +12,8 @@ files = [mypy-organizations.*] follow_untyped_imports = True +[mypy-simple_history.*] +follow_untyped_imports = True + [mypy.plugins.django-stubs] django_settings_module = "projects.dev" diff --git a/projects/dev.py b/projects/dev.py index 28348acab..02f5824e2 100644 --- a/projects/dev.py +++ b/projects/dev.py @@ -37,6 +37,10 @@ # Open edX Organizations (dependency for openedx_catalog) "organizations", + # Required for django-simple-history's admin integration and management commands; + # HistoricalRecords() alone does not need it. + "simple_history", + # Our Apps "openedx_catalog", "openedx_learning", diff --git a/src/openedx_content/applets/containers/models.py b/src/openedx_content/applets/containers/models.py index b01ed18fe..d2f94bde3 100644 --- a/src/openedx_content/applets/containers/models.py +++ b/src/openedx_content/applets/containers/models.py @@ -160,7 +160,7 @@ class Meta: ), ] - def __str__(self) -> str: # pylint: disable=invalid-str-returned + def __str__(self) -> str: return self.type_code diff --git a/src/openedx_learning/applets/cbe/models/__init__.py b/src/openedx_learning/applets/cbe/models/__init__.py new file mode 100644 index 000000000..71b2bf501 --- /dev/null +++ b/src/openedx_learning/applets/cbe/models/__init__.py @@ -0,0 +1,16 @@ +""" +Models for Competency-Based Education (CBE). +""" + +from ..rule_payloads import RuleType +from .competency_taxonomy import CompetencyTaxonomy +from .criteria import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, LogicOperator + +__all__ = [ + "CompetencyCriteriaGroup", + "CompetencyCriterion", + "CompetencyRuleProfile", + "CompetencyTaxonomy", + "LogicOperator", + "RuleType", +] diff --git a/src/openedx_learning/applets/cbe/models.py b/src/openedx_learning/applets/cbe/models/competency_taxonomy.py similarity index 72% rename from src/openedx_learning/applets/cbe/models.py rename to src/openedx_learning/applets/cbe/models/competency_taxonomy.py index 7cbd8cb3a..edd03d71e 100644 --- a/src/openedx_learning/applets/cbe/models.py +++ b/src/openedx_learning/applets/cbe/models/competency_taxonomy.py @@ -1,6 +1,9 @@ """ -Models for Competency-Based Education (CBE). +The CompetencyTaxonomy model. """ +from django.db import models +from django.utils.translation import gettext_lazy as _ + from openedx_tagging.models import Taxonomy __all__ = [ @@ -35,6 +38,15 @@ class CompetencyTaxonomy(Taxonomy): .. no_pii: """ + taxonomy_overrides_org = models.BooleanField( + default=False, + help_text=_( + "If both an organization-scoped CompetencyRuleProfile and a taxonomy-scoped CompetencyRuleProfile " + "from this taxonomy apply to the same criterion, False (the default) assigns the organization-scoped " + "profile, and True assigns this taxonomy's own profile." + ), + ) + class Meta: verbose_name = "Competency Taxonomy" verbose_name_plural = "Competency Taxonomies" diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py new file mode 100644 index 000000000..ea23196fc --- /dev/null +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -0,0 +1,347 @@ +""" +The Competency Criteria tree models: CompetencyCriteriaGroup, CompetencyRuleProfile, and +CompetencyCriterion. + +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. +""" +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 ObjectTag, Tag + +from ..rule_payloads import RuleType, validate_rule_payload +from .competency_taxonomy import CompetencyTaxonomy + +__all__ = [ + "CompetencyCriteriaGroup", + "CompetencyCriterion", + "CompetencyRuleProfile", + "LogicOperator", +] + + +class LogicOperator(models.TextChoices): + """How a CompetencyCriteriaGroup combines its child nodes.""" + + AND = "AND", _("And") + OR = "OR", _("Or") + + +class CompetencyCriteriaGroup(models.Model): + """ + An internal AND/OR node in a Competency Criteria tree. + + A single Competency Criteria tree is one root CompetencyCriteriaGroup plus all of its + descendant groups and leaf :class:`CompetencyCriterion` rows. ``logic_operator`` says how + this group's own children combine. ``ordering`` gives this group's own position among its + siblings under their shared parent, which read-time evaluation and event-driven recomputation + rely on for deterministic, short-circuiting evaluation order. A group's children can be a mix + of child groups and leaf criteria, and only CompetencyCriteriaGroup carries an ``ordering`` + field, so that mix has no total order; #641 accepts this deliberately. See ADR-0002 Decision 2. + + .. no_pii: + """ + + uuid = immutable_uuid_field() + parent = models.ForeignKey( + "self", + null=True, + blank=True, + on_delete=models.CASCADE, + related_name="child_groups", + help_text=_("The parent CompetencyCriteriaGroup. Null means this group is a tree root."), + ) + tag = models.ForeignKey( + Tag, + db_column="oel_tagging_tag_id", + on_delete=models.CASCADE, + related_name="competency_criteria_groups", + help_text=_("The competency (tag) that this criteria tree evaluates mastery of."), + ) + course = models.ForeignKey( + CourseRun, + null=True, + blank=True, + on_delete=models.CASCADE, + related_name="competency_criteria_groups", + help_text=_("The course run that scopes this criteria tree for evaluation windowing, if any."), + ) + name = case_insensitive_char_field( + max_length=255, blank=True, default="", help_text=_("A human-readable label for this group, if any.") + ) + ordering = models.PositiveIntegerField( + default=0, + help_text=_( + "Deterministic sibling evaluation sequence. Used to short-circuit evaluation and to order " + "child scans during event-driven recomputation." + ), + ) + logic_operator = models.CharField( + max_length=3, + choices=LogicOperator, + null=True, + blank=True, + help_text=_( + "How this group's children combine. Null only for a group with a single child, where combining " + "logic is moot; the application layer treats null the same as OR." + ), + ) + + history = HistoricalRecords() + + class Meta: + indexes = [ + # ADR-0002 Decision 5, index 1: lookups by competency tag and course scope. + models.Index(fields=["tag", "course"]), + # ADR-0002 Decision 5 also lists an index on `parent` (index 2), but Django already + # indexes every ForeignKey column by default, so a second explicit one here would only + # cost write throughput without adding any read benefit. + ] + + +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. When more than + one scope could apply to the same criterion, the most specific one wins (course beats + organization/taxonomy), and taxonomy_overrides_org breaks the one remaining tie between an + organization- and a taxonomy-scoped profile. See ADR-0002 Decision 3 & 4 for more details. + + 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 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." + ), + ) + + # 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) + + +class CompetencyCriterion(models.Model): + """ + A leaf node in a Competency Criteria tree: one tag/object association plus its rule. + + A null `rule_profile` does not mean "resolve at read time." ADR-0002 Decision 4 resolves and + stores the applicable profile (or override) at specific write events only; `rule_profile` is + null only when a per-criterion override is set instead. Do not add a property, manager method, + or other helper that recomputes it; that would contradict the ADR. + + .. no_pii: + """ + + uuid = immutable_uuid_field() + group = models.ForeignKey( + CompetencyCriteriaGroup, + db_column="competency_criteria_group_id", + 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, + help_text=_("Overrides rule_profile's rule_type for this criterion. Set only when rule_profile is null."), + ) + rule_payload_override = models.JSONField( + null=True, blank=True, + help_text=_("Overrides rule_profile's rule_payload for this criterion. Set only when rule_profile is null."), + ) + + history = HistoricalRecords() + + class Meta: + 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/applets/cbe/rule_payloads.py b/src/openedx_learning/applets/cbe/rule_payloads.py new file mode 100644 index 000000000..931eba76e --- /dev/null +++ b/src/openedx_learning/applets/cbe/rule_payloads.py @@ -0,0 +1,109 @@ +""" +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 Literal, TypedDict, get_args + +from django.core.exceptions import ValidationError +from django.db import models +from django.utils.translation import gettext_lazy as _ + +__all__ = [ + "GradeRulePayload", + "RuleType", + "validate_rule_payload", +] + + +class RuleType(models.TextChoices): + """ + The evaluation rule types a CompetencyRuleProfile or CompetencyCriterion override can use. + + Only rule types with a defined ``rule_payload`` shape are listed here. ADR-0002 Decision 3 + also names ``View`` and ``MasteryLevel`` as rule types to consider adding later, once someone defines + their payload shape; until then, adding a member here without a matching branch in + ``validate_rule_payload`` fails a test rather than shipping an option nothing can validate. + """ + + GRADE = "Grade", _("Grade") + + +GradeOperator = Literal["gte", "lte", "eq"] + +_GRADE_OPERATORS: frozenset[str] = frozenset(get_args(GradeOperator)) + + +class GradeRulePayload(TypedDict): + """ + The stored shape of a ``RuleType.GRADE`` rule_payload, for annotating a dict already known to be + well-formed. Declarative only: ``_validate_grade_rule_payload`` is what rejects a bad payload, while + these annotations are the single declaration of the payload's key set. + """ + + op: GradeOperator + value: float + scale: Literal["percent"] + + +def _validate_grade_rule_payload(payload: dict[str, object]) -> 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 would slip through a plain numeric + # check and read as the fraction 1.0, silently meaning "100%". bool subclasses int, so + # the type checker doesn't catch this either. + 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'.")) + + +_GRADE_PAYLOAD_KEYS: frozenset[str] = frozenset(GradeRulePayload.__annotations__) + + +def _validate_payload_keys(rule_type: str, payload: object, expected_keys: frozenset[str]) -> dict[str, object]: + """Raise ValidationError unless ``payload`` is a JSON object with exactly ``expected_keys``.""" + 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"), + } + ) + return payload + + +def validate_rule_payload(rule_type: str, payload: object) -> None: + """ + Raise ValidationError unless ``payload`` is a JSON object with exactly the keys ``rule_type``'s + shape requires and values within that shape's constraints -- for ``Grade``, ``op`` one of + ``gte``, ``lte``, ``eq``, ``value`` a fraction from 0.0 to 1.0, and ``scale`` equal to + ``"percent"`` -- including when ``rule_type`` has no defined shape at all. + """ + match rule_type: + case RuleType.GRADE: + grade_payload = _validate_payload_keys(rule_type, payload, _GRADE_PAYLOAD_KEYS) + _validate_grade_rule_payload(grade_payload) + case _: + raise ValidationError( + _("Rule type '%(rule_type)s' is not supported yet; only 'Grade' has a defined rule_payload shape.") + % {"rule_type": rule_type} + ) diff --git a/src/openedx_learning/migrations/0002_competencytaxonomy_taxonomy_overrides_org.py b/src/openedx_learning/migrations/0002_competencytaxonomy_taxonomy_overrides_org.py new file mode 100644 index 000000000..f20ab5929 --- /dev/null +++ b/src/openedx_learning/migrations/0002_competencytaxonomy_taxonomy_overrides_org.py @@ -0,0 +1,18 @@ +# Generated by Django 5.2.16 on 2026-09-09 21:15 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_learning', '0001_initial'), + ] + + operations = [ + migrations.AddField( + model_name='competencytaxonomy', + name='taxonomy_overrides_org', + field=models.BooleanField(default=False, help_text="If both an organization-scoped CompetencyRuleProfile and a taxonomy-scoped CompetencyRuleProfile from this taxonomy apply to the same criterion, False (the default) assigns the organization-scoped profile, and True assigns this taxonomy's own profile."), + ), + ] diff --git a/src/openedx_learning/migrations/0003_competencycriteriagroup.py b/src/openedx_learning/migrations/0003_competencycriteriagroup.py new file mode 100644 index 000000000..e2f42609c --- /dev/null +++ b/src/openedx_learning/migrations/0003_competencycriteriagroup.py @@ -0,0 +1,65 @@ +# Generated by Django 5.2.16 on 2026-09-10 18:32 + +import uuid + +import django.db.models.deletion +import simple_history.models +from django.conf import settings +from django.db import migrations, models + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0021_remove_system_defined_add_read_only'), + ('openedx_catalog', '0001_initial'), + ('openedx_learning', '0002_competencytaxonomy_taxonomy_overrides_org'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name='CompetencyCriteriaGroup', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), + ('name', openedx_django_lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, default='', help_text='A human-readable label for this group, if any.', max_length=255)), + ('ordering', models.PositiveIntegerField(default=0, help_text='Deterministic sibling evaluation sequence. Used to short-circuit evaluation and to order child scans during event-driven recomputation.')), + ('logic_operator', models.CharField(blank=True, choices=[('AND', 'And'), ('OR', 'Or')], help_text="How this group's children combine. Null only for a group with a single child, where combining logic is moot; the application layer treats null the same as OR.", max_length=3, null=True)), + ('course', models.ForeignKey(blank=True, help_text='The course run that scopes this criteria tree for evaluation windowing, if any.', null=True, on_delete=django.db.models.deletion.CASCADE, related_name='competency_criteria_groups', to='openedx_catalog.courserun')), + ('parent', models.ForeignKey(blank=True, help_text='The parent CompetencyCriteriaGroup. Null means this group is a tree root.', null=True, on_delete=django.db.models.deletion.CASCADE, related_name='child_groups', to='openedx_learning.competencycriteriagroup')), + ('tag', models.ForeignKey(db_column='oel_tagging_tag_id', help_text='The competency (tag) that this criteria tree evaluates mastery of.', on_delete=django.db.models.deletion.CASCADE, related_name='competency_criteria_groups', to='oel_tagging.tag')), + ], + ), + migrations.CreateModel( + name='HistoricalCompetencyCriteriaGroup', + fields=[ + ('id', models.BigIntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('uuid', models.UUIDField(db_index=True, default=uuid.uuid4, editable=False, verbose_name='UUID')), + ('name', openedx_django_lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, default='', help_text='A human-readable label for this group, if any.', max_length=255)), + ('ordering', models.PositiveIntegerField(default=0, help_text='Deterministic sibling evaluation sequence. Used to short-circuit evaluation and to order child scans during event-driven recomputation.')), + ('logic_operator', models.CharField(blank=True, choices=[('AND', 'And'), ('OR', 'Or')], help_text="How this group's children combine. Null only for a group with a single child, where combining logic is moot; the application layer treats null the same as OR.", max_length=3, null=True)), + ('history_id', models.AutoField(primary_key=True, serialize=False)), + ('history_date', models.DateTimeField(db_index=True)), + ('history_change_reason', models.CharField(max_length=100, null=True)), + ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), + ('course', models.ForeignKey(blank=True, db_constraint=False, help_text='The course run that scopes this criteria tree for evaluation windowing, if any.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='openedx_catalog.courserun')), + ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), + ('parent', models.ForeignKey(blank=True, db_constraint=False, help_text='The parent CompetencyCriteriaGroup. Null means this group is a tree root.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='openedx_learning.competencycriteriagroup')), + ('tag', models.ForeignKey(blank=True, db_column='oel_tagging_tag_id', db_constraint=False, help_text='The competency (tag) that this criteria tree evaluates mastery of.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='oel_tagging.tag')), + ], + options={ + 'verbose_name': 'historical competency criteria group', + 'verbose_name_plural': 'historical competency criteria groups', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': ('history_date', 'history_id'), + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + migrations.AddIndex( + model_name='competencycriteriagroup', + index=models.Index(fields=['tag', 'course'], name='openedx_lea_oel_tag_737416_idx'), + ), + ] diff --git a/src/openedx_learning/migrations/0004_competencyruleprofile.py b/src/openedx_learning/migrations/0004_competencyruleprofile.py new file mode 100644 index 000000000..422a54801 --- /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', '0007_historicalorganization'), + 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 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)), + ('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 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')), + ('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/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/src/openedx_learning/migrations/0007_alter_criterion_override_help_text.py b/src/openedx_learning/migrations/0007_alter_criterion_override_help_text.py new file mode 100644 index 000000000..4539f3c23 --- /dev/null +++ b/src/openedx_learning/migrations/0007_alter_criterion_override_help_text.py @@ -0,0 +1,33 @@ +# Generated by Django 5.2.17 on 2026-09-22 18:35 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_learning', '0006_competencycriterion'), + ] + + operations = [ + migrations.AlterField( + model_name='competencycriterion', + name='rule_payload_override', + field=models.JSONField(blank=True, help_text="Overrides rule_profile's rule_payload for this criterion. Set only when rule_profile is null.", null=True), + ), + migrations.AlterField( + model_name='competencycriterion', + name='rule_type_override', + field=models.CharField(blank=True, choices=[('Grade', 'Grade')], help_text="Overrides rule_profile's rule_type for this criterion. Set only when rule_profile is null.", max_length=32, null=True), + ), + migrations.AlterField( + model_name='historicalcompetencycriterion', + name='rule_payload_override', + field=models.JSONField(blank=True, help_text="Overrides rule_profile's rule_payload for this criterion. Set only when rule_profile is null.", null=True), + ), + migrations.AlterField( + model_name='historicalcompetencycriterion', + name='rule_type_override', + field=models.CharField(blank=True, choices=[('Grade', 'Grade')], help_text="Overrides rule_profile's rule_type for this criterion. Set only when rule_profile is null.", max_length=32, null=True), + ), + ] diff --git a/test_settings.py b/test_settings.py index be0a84904..b3cc99ec1 100644 --- a/test_settings.py +++ b/test_settings.py @@ -54,6 +54,9 @@ def root(*args): "organizations", # django-rules based authorization 'rules.apps.AutodiscoverRulesConfig', + # Required for django-simple-history's admin integration and management commands; + # HistoricalRecords() alone does not need it. + "simple_history", # Our own apps "openedx_tagging", "openedx_content", diff --git a/tests/openedx_learning/applets/cbe/conftest.py b/tests/openedx_learning/applets/cbe/conftest.py new file mode 100644 index 000000000..58935b6f0 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/conftest.py @@ -0,0 +1,67 @@ +"""Shared fixtures for the CBE criteria test modules.""" +import pytest +from organizations.api import ensure_organization +from organizations.models import Organization + +from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_learning.models import CompetencyCriteriaGroup, CompetencyRuleProfile, CompetencyTaxonomy +from openedx_tagging.models import ObjectTag, Tag + + +@pytest.fixture(name="organization") +def _organization() -> Organization: + """An Organization for use as a scope in these tests.""" + ensure_organization("Org1") + return Organization.objects.get(short_name="Org1") + + +@pytest.fixture(name="organization2") +def _organization2() -> Organization: + """A second Organization, distinct from `organization`, for use as a scope in these tests.""" + ensure_organization("Org2") + return Organization.objects.get(short_name="Org2") + + +@pytest.fixture(name="course_run") +def _course_run(organization: Organization) -> CourseRun: + """A CourseRun for use as a scope in these tests.""" + catalog_course = CatalogCourse.objects.create(org=organization, course_code="Python100") + return CourseRun.objects.create(catalog_course=catalog_course, run_code="Fall2026") + + +@pytest.fixture(name="competency_taxonomy") +def _competency_taxonomy() -> CompetencyTaxonomy: + """A CompetencyTaxonomy for use as a scope, and as the home taxonomy for `tag`.""" + return CompetencyTaxonomy.objects.create(name="Nursing", export_id="nursing-v1") + + +@pytest.fixture(name="tag") +def _tag(competency_taxonomy: CompetencyTaxonomy) -> Tag: + """A Tag, from `competency_taxonomy`, for use as the competency a criteria tree evaluates.""" + return Tag.objects.create(taxonomy=competency_taxonomy, value="Writing Poetry") + + +@pytest.fixture(name="object_tag") +def _object_tag(competency_taxonomy: CompetencyTaxonomy, tag: Tag) -> ObjectTag: + """An ObjectTag associating `tag` with a made-up content object, a criterion's target.""" + return ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+p1", + taxonomy=competency_taxonomy, + tag=tag, + ) + + +@pytest.fixture(name="group") +def _group(tag: Tag) -> CompetencyCriteriaGroup: + """A root CompetencyCriteriaGroup for `tag`, for use as a criterion's parent group.""" + return CompetencyCriteriaGroup.objects.create(tag=tag) + + +@pytest.fixture(name="default_rule_profile") +def _default_rule_profile() -> CompetencyRuleProfile: + """The system-default CompetencyRuleProfile seeded by migration 0005.""" + return CompetencyRuleProfile.objects.get( + organization__isnull=True, + course__isnull=True, + competency_taxonomy__isnull=True, + ) diff --git a/tests/openedx_learning/applets/cbe/test_criteria_group.py b/tests/openedx_learning/applets/cbe/test_criteria_group.py new file mode 100644 index 000000000..39bb755f0 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_group.py @@ -0,0 +1,108 @@ +""" +Tests for CompetencyCriteriaGroup, the internal AND/OR node of a Competency Criteria +tree. + +Each test name states the behavior it pins. Reading top to bottom gives the model's contract: +its columns, its tree shape, the two constraints ADR-0002 Decision 2 deliberately leaves out, +then its indexes and history. + +Delete behavior is not covered here. Nothing in this module deletes a row that another row +points at. See test_criteria_group_deletion.py, in this same change, for this model's own +`on_delete` values and the tests that exercise them. + +Fixtures live in this directory's conftest.py. +""" +import pytest +from django.apps import apps + +from openedx_learning.models import CompetencyCriteriaGroup, LogicOperator +from openedx_tagging.models import Tag + +pytestmark = pytest.mark.django_db + + +# --------------------------------------------------------------------------------------------- +# Tree shape, and the two constraints ADR-0002 Decision 2 deliberately leaves out + + +# --------------------------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "logic_operator", + [ + pytest.param(LogicOperator.AND, id="and"), + pytest.param(LogicOperator.OR, id="or"), + pytest.param(None, id="null"), + ], +) +def test_group_logic_operator_accepts_and_or_and_null_regardless_of_child_count( + logic_operator: str | None, tag: Tag +) -> None: + """ + logic_operator accepts AND, OR, or null. Nothing at the data layer constrains it by how many + children the group actually has: a group with zero children and a group with two children both + save successfully with any of the three values. See ADR-0002 Decision 2; the database cannot + see a group's future children at save time (a child's parent FK cannot point at a row that + doesn't have a primary key yet), so this is enforced nowhere at this layer, deliberately. + """ + childless = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=logic_operator) + assert childless.pk is not None + + parent = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=logic_operator) + CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent) + CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent) + parent.refresh_from_db() + assert parent.logic_operator == logic_operator + assert CompetencyCriteriaGroup.objects.filter(parent=parent).count() == 2 + + +def test_a_root_group_has_a_null_parent_and_a_child_points_at_the_group_it_was_created_under(tag: Tag) -> None: + """ + A CompetencyCriteriaGroup's parent is null for a root and points at its parent for a child. + See ADR-0002 Decision 2. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=None) + assert root.parent is None + + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, logic_operator=LogicOperator.AND) + assert child.parent == root + + +def test_group_has_no_unique_constraint_on_parent_and_ordering(tag: Tag) -> None: + """ + No UniqueConstraint on (parent, ordering) exists: two sibling groups may share the same + `ordering` value. A parent's clean() cannot see its own future children at save time (a + child's FK can't point at a not-yet-existing parent row), so there is no single-row state to + check a per-parent uniqueness rule against, and none is declared. See ADR-0002 Decision 2. + """ + parent = CompetencyCriteriaGroup.objects.create(tag=tag) + sibling_a = CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent, ordering=1) + sibling_b = CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent, ordering=1) + assert sibling_a.ordering == sibling_b.ordering == 1 + + +# --------------------------------------------------------------------------------------------- +# Indexes and history + + +# --------------------------------------------------------------------------------------------- + + +def test_editing_a_group_writes_a_historical_row(tag: Tag) -> None: + """ + HistoricalRecords() is applied to CompetencyCriteriaGroup: the Historical model is registered + under its expected name, and creating then editing a group leaves two rows in it. See + ADR-0003 Decision 1. + + The Historical model is looked up through the app registry rather than the `.history` + attribute because simple_history installs `.history` as a runtime descriptor with no type + stubs, which mypy cannot type. + """ + historical_group = apps.get_model("openedx_learning", "HistoricalCompetencyCriteriaGroup") + group = CompetencyCriteriaGroup.objects.create(tag=tag) + + group.name = "Poetry Mastery" + group.save() + + assert historical_group.objects.filter(id=group.pk).count() == 2 diff --git a/tests/openedx_learning/applets/cbe/test_criteria_group_deletion.py b/tests/openedx_learning/applets/cbe/test_criteria_group_deletion.py new file mode 100644 index 000000000..895bebc8e --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_group_deletion.py @@ -0,0 +1,104 @@ +"""Delete-behavior tests for CompetencyCriteriaGroup's own foreign keys.""" +import pytest +from django.apps import apps + +from openedx_catalog.models import CourseRun +from openedx_learning.models import CompetencyCriteriaGroup, CompetencyTaxonomy +from openedx_tagging.models import Tag + +pytestmark = pytest.mark.django_db + + +# --------------------------------------------------------------------------------------------- +# 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. + + `course` is a nullable cascading foreign key. MySQL cannot defer foreign-key constraint + checks, so Django's collector nulls such a key before the DELETE; SQLite never does. A + regression in that MySQL path therefore fails this test in CI (which runs MySQL) while still + passing locally on SQLite. + """ + group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run) + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + + course_run.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + + +def test_deleting_a_group_at_depth_also_deletes_every_descendant_group(tag: Tag) -> None: + """ + Deleting a CompetencyCriteriaGroup removes not just its direct children but every group + beneath it at any depth: `parent` is a self-referential CASCADE, so a single delete has + Django's collector walk the whole subtree, not just one level. Deleting the root and checking + the grandchild is what actually exercises that recursion. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + grandchild = CompetencyCriteriaGroup.objects.create(tag=tag, parent=child) + + root.delete() + + assert not 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() + + +def test_deleting_a_taxonomy_also_deletes_its_tags_criteria_groups(competency_taxonomy: CompetencyTaxonomy) -> None: + """ + Deleting a CompetencyTaxonomy cascades through every Tag it owns (already CASCADE in + openedx_tagging) and, transitively, through this model's own `tag` CASCADE: every + CompetencyCriteriaGroup for a tag under that taxonomy is gone too. + """ + tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Writing Poetry") + group = CompetencyCriteriaGroup.objects.create(tag=tag) + + competency_taxonomy.delete() + + assert not Tag.objects.filter(pk=tag.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=group.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..3e1296f3e --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_trees.py @@ -0,0 +1,104 @@ +"""Integrative test for the Competency Criteria tree: deleting a group partway down a wider, +realistic tree removes exactly that subtree and leaves an untouched sibling branch alone. + +Fixtures shared with the other test modules in this directory live in its 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_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) + | `-- grandchild (criterion) + `-- surviving_sibling (criterion) + + `branch_to_delete` is deleted. This exercises criteria at two different tree depths (on + `branch_to_delete` itself and on its child `grandchild`), 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. + """ + 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 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..d700dda6c --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criterion.py @@ -0,0 +1,158 @@ +"""Tests for CompetencyCriterion, a leaf of a Competency Criteria tree.""" +import pytest +from django.apps import apps +from django.core.exceptions import ValidationError +from django.db import transaction +from django.db.utils import IntegrityError + +from openedx_learning.models import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, 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"), +] + + +# --------------------------------------------------------------------------------------------- +# Either a rule_profile or both overrides. Never both, never neither. + + +# --------------------------------------------------------------------------------------------- + + +@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. + """ + 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_profile_xor_override_check_constraint_holds_via_bulk_create( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + The profile-xor-overrides check constraint also rejects a bulk_create() that sets both a + rule_profile and the override fields, even though bulk_create() never builds and saves an + individual model instance, so clean()/full_clean() never runs. This confirms the invariant is + enforced by the database's own check constraint, not merely by save()'s validation. + """ + with pytest.raises(IntegrityError): + with transaction.atomic(): + CompetencyCriterion.objects.bulk_create([ + CompetencyCriterion( + group=group, + object_tag=object_tag, + rule_profile=default_rule_profile, + rule_type_override=RuleType.GRADE, + rule_payload_override=_GRADE_PAYLOAD, + ) + ]) + + +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: + 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() + + +# --------------------------------------------------------------------------------------------- +# History + + +# --------------------------------------------------------------------------------------------- + + +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 & 4 for more info. + """ + 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 diff --git a/tests/openedx_learning/applets/cbe/test_criterion_deletion.py b/tests/openedx_learning/applets/cbe/test_criterion_deletion.py new file mode 100644 index 000000000..0c78d335f --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criterion_deletion.py @@ -0,0 +1,250 @@ +"""Delete-behavior tests for CompetencyCriterion's own foreign keys, and the transitive and +scope-owner cases that only exist once this model completes the criteria tree.""" +import pytest +from django.apps import apps +from django.db.models import RestrictedError + +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"} + + +# --------------------------------------------------------------------------------------------- +# 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_a_cascaded_criterion_removal_is_recorded_in_history( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + A criterion 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 criterion + that vanished this way still finds why it did. + """ + historical_criterion = apps.get_model("openedx_learning", "HistoricalCompetencyCriterion") + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + criterion_pk = criterion.pk + + group.delete() + + assert historical_criterion.objects.filter(id=criterion_pk, history_type="-").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. + """ + 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. 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) + + +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 the group now has no + children of any kind. + + This is deliberate, not a bug: CompetencyCriteriaGroup has no foreign key to ObjectTag, so + nothing about this delete gives Django's collector a reason to reach the group. Cleaning up a + now-childless group is authoring-API/application-layer work, not something an on_delete value + can express here. + """ + 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() + + +# --------------------------------------------------------------------------------------------- +# 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_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 + this taxonomy delete succeeds and cascades away the group and criterion beneath its tag too, + the same as a direct tag delete. + + 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() diff --git a/tests/openedx_learning/applets/cbe/test_models.py b/tests/openedx_learning/applets/cbe/test_models.py index b38c25928..7b113cb8a 100644 --- a/tests/openedx_learning/applets/cbe/test_models.py +++ b/tests/openedx_learning/applets/cbe/test_models.py @@ -44,6 +44,14 @@ def test_plain_taxonomy_has_no_competencytaxonomy() -> None: _ = plain.competencytaxonomy +def test_taxonomy_overrides_org_defaults_false(competency_taxonomy: CompetencyTaxonomy) -> None: + """ + taxonomy_overrides_org defaults to False, so an organization-scoped profile wins the + contested case by default until an author opts a taxonomy out. See ADR-0002 Decision 1. + """ + assert competency_taxonomy.taxonomy_overrides_org is False + + def test_delete_cascades_both_directions() -> None: """ Deleting the parent Taxonomy removes the CompetencyTaxonomy row, and deleting 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..47c342793 --- /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. + +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 + +from openedx_learning.applets.cbe.rule_payloads import GradeRulePayload, RuleType, validate_rule_payload + +_GRADE_PAYLOAD: GradeRulePayload = {"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. + """ + 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 "missing scale, value" in message + assert "unexpected extra" 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) + + +@pytest.mark.parametrize("rule_type", list(RuleType)) +def test_every_rule_type_choice_has_a_validation_branch(rule_type: RuleType) -> None: + """ + Guards against a future RuleType member shipping with no matching validate_rule_payload + branch: adding one without a branch would offer an author a choice that always rejects, + since it falls through to the catch-all "not supported yet" case regardless of payload. + + Runs against every current and future RuleType member automatically, since it parametrizes + over list(RuleType) rather than naming Grade specifically. An empty payload is used because + it is wrong for every currently defined shape; the assertion below only checks that the + rejection reason is shape-specific (e.g. missing keys), not "unsupported rule type." + """ + with pytest.raises(ValidationError) as exc_info: + validate_rule_payload(rule_type, {}) + + assert "not supported yet" not in " ".join(exc_info.value.messages) 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..944834d31 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_rule_profile.py @@ -0,0 +1,408 @@ +"""Tests for CompetencyRuleProfile, the reusable evaluation rule a CompetencyCriterion draws from.""" +import pytest +from django.apps import apps +from django.core.exceptions import ValidationError +from django.db import transaction +from django.db.utils import IntegrityError +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 + +_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} + + +def test_creating_a_rule_profile_persists_its_columns(organization: Organization) -> None: + """ + Creating a CompetencyRuleProfile with values for `organization`, `rule_type`, and + `rule_payload`, then reading the row back from the database, returns those same values, plus + `course` and `competency_taxonomy` left null and `archived` defaulting to False. See ADR-0002 + Decision 3. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + persisted = CompetencyRuleProfile.objects.get(pk=profile.pk) + assert persisted.organization == organization + assert persisted.course is None + assert persisted.competency_taxonomy is None + assert persisted.rule_type == RuleType.GRADE + assert persisted.rule_payload == _GRADE_PAYLOAD + assert persisted.archived is False + + +# --------------------------------------------------------------------------------------------- +# 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_can_be_created_with_any_single_scope_or_no_scope( + scope_kwargs: dict, + organization: Organization, + course_run: CourseRun, + competency_taxonomy: CompetencyTaxonomy, +) -> None: + """ + A CompetencyRuleProfile can be created scoped to organization, course, or competency_taxonomy + alone, or to none of them (the system default). The check constraint's rejection of more than + one scope field at once is covered separately, by + test_rule_profile_scope_check_constraint_rejects_more_than_one_scope_field below. + """ + # Free the all-null slot the seed migration (0005) 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_archiving_a_profile_nulls_scope_code_and_frees_its_scope_for_a_replacement( + organization: Organization, +) -> None: + """ + Archiving a profile nulls its scope_code, which frees that scope for a brand new profile: a + replacement may now be created for the exact same organization/course/taxonomy, since the + archived row no longer occupies the unique scope_code slot. + """ + original = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + original.refresh_from_db() + assert original.scope_code == f"org:{organization.pk},course:,taxonomy:" + + original.archived = True + original.save() + original.refresh_from_db() + assert original.scope_code is None + + replacement = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + replacement.refresh_from_db() + assert replacement.scope_code == f"org:{organization.pk},course:,taxonomy:" + + +def test_archiving_the_seeded_system_default_frees_its_scope_for_a_replacement( + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Archiving the seeded system-default profile (all three scope fields null) nulls its + scope_code, same as for a scoped profile, which frees the all-null scope for a brand new + system-default row. + """ + assert default_rule_profile.scope_code == "org:,course:,taxonomy:" + + default_rule_profile.archived = True + default_rule_profile.save() + default_rule_profile.refresh_from_db() + assert default_rule_profile.scope_code is None + + replacement = CompetencyRuleProfile.objects.create(rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD) + replacement.refresh_from_db() + assert replacement.scope_code == "org:,course:,taxonomy:" + + +def test_two_live_profiles_cannot_share_the_same_scope(organization: Organization) -> None: + """Two live CompetencyRuleProfile rows cannot share the same scope: two rows that both set + only `organization` collide.""" + 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 (here, `value: 80` instead of a + 0.0-1.0 fraction) 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 + + +# --------------------------------------------------------------------------------------------- +# 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_payload_and_archived_to_change(organization: Organization) -> None: + """ + Only rule_type, rule_payload, and archived may change after creation; changing rule_payload and + archived (as opposed to a scope field) succeeds. rule_type isn't exercised here: RuleType has + only one member right now, so there's no other value to change it to. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + 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() + + +# --------------------------------------------------------------------------------------------- +# History and the seeded system default + + +# --------------------------------------------------------------------------------------------- + + +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 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( + 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 + + +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) 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..1ad734e14 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_rule_profile_deletion.py @@ -0,0 +1,113 @@ +"""Delete-behavior tests for CompetencyRuleProfile's own foreign keys.""" +import pytest +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. + """ + 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 +# 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 a regression in this path fails in CI (which runs MySQL) but can +# still pass locally on SQLite. 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_deleting_two_taxonomies_together_cascades_both_their_scoped_profiles_away() -> None: + """ + Deleting two CompetencyTaxonomy rows in one `.delete()` call, each with its own taxonomy-scoped + profile, succeeds and cascades both away, exercising the collector's multi-row nulling path + rather than the single-row path test_deleting_a_taxonomy_with_a_scoped_rule_profile_also_ + deletes_the_profile above covers. + """ + 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 d7b9ea97925ac43e25596c25b83a9f66c912e085 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 1 Oct 2026 10:44:44 -0400 Subject: [PATCH 2/3] build: declare django-simple-history as a dependency The CBE criteria models use simple_history. Upstream's move to uv and pyproject.toml removed requirements/base.in, which declared dependencies before, so the dependency is declared in pyproject.toml and uv.lock. Co-Authored-By: Claude Sonnet 5.5 --- pyproject.toml | 1 + uv.lock | 2 ++ 2 files changed, 3 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index 91b16ee9a..450510586 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -40,6 +40,7 @@ dependencies = [ "rules<4.0", "tomlkit", "edx-organizations", + "django-simple-history", ] [project.urls] diff --git a/uv.lock b/uv.lock index bab5c92ca..749be57d1 100644 --- a/uv.lock +++ b/uv.lock @@ -1565,6 +1565,7 @@ dependencies = [ { name = "attrs" }, { name = "celery" }, { name = "django" }, + { name = "django-simple-history" }, { name = "djangorestframework" }, { name = "edx-drf-extensions" }, { name = "edx-organizations" }, @@ -1694,6 +1695,7 @@ requires-dist = [ { name = "attrs" }, { name = "celery" }, { name = "django" }, + { name = "django-simple-history" }, { name = "djangorestframework", specifier = "<4.0" }, { name = "edx-drf-extensions" }, { name = "edx-organizations" }, From db18957de79009c19a56ebc98f3e9585a6f245d9 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 1 Oct 2026 11:00:37 -0400 Subject: [PATCH 3/3] fix: resolve doc8 and pylint CI failures Strip the carriage returns from three lines of ADR-0002 that doc8 rejects, and keep ContainerType.__str__ as it is on main, including its pylint suppression, since the pylint version CI uses still reports E0307 without it. Co-Authored-By: Claude Sonnet 5.5 --- .../decisions/0002-competency-criteria-model.rst | 6 +++--- src/openedx_content/applets/containers/models.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index c0e258cf5..334eedd53 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -455,6 +455,6 @@ Changelog 2026-09-09: -* ``scope_code`` on ``CompetencyRuleProfile`` is now computed by - application code instead of being database-generated, and is set to null while a - profile is archived, freeing its scope for a replacement. +* ``scope_code`` on ``CompetencyRuleProfile`` is now computed by + application code instead of being database-generated, and is set to null while a + profile is archived, freeing its scope for a replacement. diff --git a/src/openedx_content/applets/containers/models.py b/src/openedx_content/applets/containers/models.py index d2f94bde3..b01ed18fe 100644 --- a/src/openedx_content/applets/containers/models.py +++ b/src/openedx_content/applets/containers/models.py @@ -160,7 +160,7 @@ class Meta: ), ] - def __str__(self) -> str: + def __str__(self) -> str: # pylint: disable=invalid-str-returned return self.type_code