diff --git a/.annotation_safe_list.yml b/.annotation_safe_list.yml index 6b9f74d07..65b803cd4 100644 --- a/.annotation_safe_list.yml +++ b/.annotation_safe_list.yml @@ -77,6 +77,12 @@ openedx_content.Unit: ".. no_pii:": "This model has no PII" openedx_content.UnitVersion: ".. no_pii:": "This model has no PII" +openedx_learning.HistoricalCompetencyCriteriaGroup: + ".. no_pii:": "This model has no PII" +openedx_learning.HistoricalCompetencyCriterion: + ".. no_pii:": "This model has no PII" +openedx_learning.HistoricalCompetencyRuleProfile: + ".. no_pii:": "This model has no PII" social_django.Association: ".. no_pii:": "This model has no PII" social_django.Code: diff --git a/.importlinter b/.importlinter index 17dd176f6..107d4c428 100644 --- a/.importlinter +++ b/.importlinter @@ -7,6 +7,7 @@ root_packages = openedx_learning openedx_content + openedx_catalog openedx_tagging openedx_django_lib openedx_core @@ -23,8 +24,16 @@ layers = # particular, openedx_tagging must never know that CBE exists. openedx_learning - # Content: authoring-side models and APIs. + # Content (authoring-side models and APIs) above Catalog (CatalogCourse/CourseRun): + # docs/openedx_learning/decisions/0007-pathway-catalog-content-split.rst ("Dependency + # direction: openedx_content knows about openedx_catalog, never the reverse") settles this + # direction, even though that ADR is still Draft rather than Accepted. A `layers` contract is + # a strict total order, and ranking them asserts only the half nobody disputes -- catalog + # never reaches up into content -- while still allowing the direction that ADR already + # depends on. CompetencyCriteriaGroup and CompetencyRuleProfile (openedx_learning) scope to a + # CourseRun, so both still need to sit below openedx_learning. openedx_content + openedx_catalog # Tagging is very simple & fundamental. Should probably not depend on any other Django apps. openedx_tagging diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index d4da9fec3..e2a8b6c33 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, as it is for any other live profile (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). - 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, recomputed by the model's ``save()`` and never set directly, 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:"``, or ``"org:,course:,taxonomy:"`` for the system default row. It is null while a profile is archived, and non-null while it is live. Collapsing the scope into one column 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 a scope. Nulling it while archived is what frees an archived profile's scope for a replacement, without needing the conditional unique index MySQL does not support. It is a plain column rather than a ``GeneratedField`` because Django's delete collector nulls a nullable cascading foreign key before issuing the DELETE on backends that cannot defer constraint checks, and a generated column would recompute from that nulled value and collide with whichever row already holds the resulting blank scope. A plain column is untouched by that nulling. 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: @@ -309,6 +309,8 @@ Decision - Once a related row exists in ``StudentCompetencyCriteriaStatus``, deletion of the associated competency definition row still succeeds, but as an archive (soft delete) instead of a hard delete: the row is hidden from authoring and new associations but remains queryable, so existing learner status rows stay resolvable. This archive-vs-hard-delete rule applies to ``oel_tagging_tag``, ``oel_tagging_taxonomy``, ``CompetencyTaxonomy``, ``oel_tagging_objecttag``, ``CompetencyCriteriaGroup``, and ``CompetencyCriteria``; see :ref:`openedx-learning-adr-0003` Decision 3 for ``oel_tagging_objecttag``'s own archive rule and traceability exception. - ``StudentCompetencyCriteriaStatus`` is what determines whether a record is protected. ``StudentCompetencyCriteriaGroupStatus`` and ``StudentCompetencyStatus`` are roll-up tables derived from it (Decision 6) and are not independently checked for this purpose: :ref:`openedx-learning-adr-0004` writes the leaf table synchronously with the grade but rolls the two roll-up tables up later via an asynchronous task, which can lag behind the leaf or, per that ADR's Decision 5, need manual recovery. Checking only the roll-up tables could therefore miss real learner progress that has not rolled up yet. - Direct deletion of a ``CompetencyRuleProfile`` is never a hard delete; retirement is always archive-only, via a normal update to its ``archived`` column (Decision 3). However, if a taxonomy or course that is associated with a taxonomy- or course-scoped profile is deleted, then this profile will be deleted along with it. + - ``on_delete`` on the criteria tables expresses containment, not protection: a row whose referent is gone is meaningless, so ``CompetencyCriteriaGroup.parent``, ``.tag`` and ``.course``, ``CompetencyCriterion.group`` and ``.object_tag``, and ``CompetencyRuleProfile.course`` and ``.competency_taxonomy`` all cascade. ``CompetencyCriterion.rule_profile`` stays ``PROTECT``, which is what makes "a profile is never hard-deleted by a direct delete" hold at the ORM layer. ``CompetencyRuleProfile.organization`` stays ``PROTECT`` because an ``Organization`` is not a competency definition record and ``edx-organizations`` deactivates organizations rather than deleting them. The tree links additionally have to cascade for a mechanical reason: Django's collector looks up referencing rows in the database rather than in the set it has already decided to delete, so a parent and child reached in the same batch would still trip ``PROTECT`` and abort the walk partway down. Those cascading edges are what carries a delete down to the ``PROTECT`` on the learner status tables, which is where this decision is actually enforced. + - Known limitation: deleting a ``CompetencyTaxonomy`` whose taxonomy-scoped profile is assigned to a ``CompetencyCriterion`` raises ``ProtectedError`` naming that criterion, even though the criterion would also be cascade-deleted in the same operation through the tag chain, for the same collector reason above. This is unreachable until scoped profiles can be authored. The fix at that point is a fifth reassignment event on Decision 4: when a profile's scope owner is being deleted, reassign every criterion off that profile before the cascade proceeds. .. image:: images/CompetencyCriteriaModel.png :alt: Competency Criteria Model @@ -417,7 +419,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 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/requirements/base.in b/requirements/base.in index 626a551be..7292e10c3 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -17,3 +17,5 @@ rules<4.0 # Django extension for rules-based authorization check tomlkit # Parses and writes TOML configuration files edx-organizations # Implemented the "Organization" model that CatalogCourse/CourseRun are keyed to + +django-simple-history # History tracking for CBE criteria definitions, per ADR-0003 diff --git a/requirements/base.txt b/requirements/base.txt index 48993cf90..dce7b5bc3 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -58,7 +58,9 @@ django-crum==0.7.9 django-model-utils==5.0.0 # via edx-organizations django-simple-history==3.13.0 - # via edx-organizations + # via + # -r requirements/base.in + # edx-organizations django-waffle==5.0.0 # via # edx-django-utils diff --git a/src/openedx_learning/applets/cbe/models/__init__.py b/src/openedx_learning/applets/cbe/models/__init__.py new file mode 100644 index 000000000..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 66% rename from src/openedx_learning/applets/cbe/models.py rename to src/openedx_learning/applets/cbe/models/competency_taxonomy.py index 7cbd8cb3a..5eea3f8b7 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,17 @@ class CompetencyTaxonomy(Taxonomy): .. no_pii: """ + taxonomy_overrides_org = models.BooleanField( + default=False, + help_text=_( + "Resolves a tie when assigning a CompetencyRuleProfile to a CompetencyCriterion (ADR-0002 " + "Decision 4): if both an organization-scoped profile and a taxonomy-scoped profile from this " + "taxonomy apply to the same criterion, False (the default) assigns the organization-scoped " + "profile, and True assigns this taxonomy's own profile instead, so it cannot be locally " + "weakened by an organization." + ), + ) + 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..a3b61d8b7 --- /dev/null +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -0,0 +1,368 @@ +""" +The CompetencyAchievementCriteria models: CompetencyCriteriaGroup, CompetencyRuleProfile, and +CompetencyCriterion. + +See :ref:`openedx-learning-adr-0002` for the design, including Decision 7 for why each foreign +key here is CASCADE or PROTECT and for the one known case where a taxonomy delete reports the +wrong blocking object. See :ref:`openedx-learning-adr-0003` Decisions 1 and 2 for why these +three 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 CompetencyAchievementCriteria expression tree. + + A single CompetencyAchievementCriteria is one root CompetencyCriteriaGroup plus all of its + descendant groups and leaf :class:`CompetencyCriterion` rows. ``logic_operator`` says how + this group's 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. + ] + # No constraint tying `logic_operator` to child count, and no UniqueConstraint on (parent, + # ordering): a child group cannot be saved until its parent's primary key exists, so + # neither has a single-row state to check at save time. See ADR-0002 Decision 2. + + +class CompetencyRuleProfile(models.Model): + """ + A reusable default evaluation rule, optionally scoped to a taxonomy, course, or organization. + + Each row is scoped by at most one of ``organization``, ``course``, and ``competency_taxonomy``, + enforced by the check constraint below; the row with all three null is the system default, + seeded once by migration and never created or deleted through the profile API. See ADR-0002 + Decision 3 for how a :class:`CompetencyCriterion` is assigned one of these, and Decision 4 for + what happens when more than one scope's profile could apply to the same criterion. + + Editing a profile may change ``rule_type``/``rule_payload`` only: the scope fields + (``organization``, ``course``, ``competency_taxonomy``) are immutable after creation, so that + criteria already resolved to this profile's scope are never silently re-governed. ``clean()`` + enforces this by comparing the current scope columns against what is actually persisted for + this row, so the check holds regardless of whether this instance was loaded with a partial + ``.only()``/``.defer()`` that skipped some scope columns. It does not cover a bulk + ``QuerySet.update()``, since that path never loads or constructs a model instance at all. + + ``rule_payload``'s shape (see :func:`~openedx_learning.applets.cbe.rule_payloads.validate_rule_payload`) + is likewise validated from ``clean()``, reached from both ``objects.create()`` and a plain + ``instance.save()`` via ``full_clean()``. A bulk ``QuerySet.update()``, ``bulk_create()``, and a + DRF serializer that writes straight to the database are NOT covered: none of them build or save + a model instance, so ``clean()`` never runs. + + .. no_pii: + """ + + 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. Deliberately a plain column + # rather than a GeneratedField. See ADR-0002 Decision 3. + scope_code = models.CharField( + max_length=255, + null=True, + editable=False, + help_text=_( + "Derived from organization/course/competency_taxonomy; null while archived, otherwise " + "\"org:X,course:Y,taxonomy:Z\" with each segment blank when that scope column is null." + ), + ) + rule_type = models.CharField(max_length=32, choices=RuleType) + rule_payload = models.JSONField( + help_text=_("Structured payload keyed by rule_type; see validate_rule_payload for the shape it must match.") + ) + archived = models.BooleanField( + default=False, + help_text=_( + "Set instead of deleting a profile that's no longer wanted. Archived profiles are hidden from " + "authoring and new associations but remain queryable, so existing criteria stay resolvable." + ), + ) + + # 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. `using` keeps a non-default-database + # instance from being compared against the wrong alias. + persisted_scope = ( + CompetencyRuleProfile.objects.using(self._state.db) + .filter(pk=self.pk) + .values_list("organization_id", "course_id", "competency_taxonomy_id") + .first() + ) + if persisted_scope is None: + return + current_scope = (self.organization_id, self.course_id, self.competency_taxonomy_id) + if current_scope != 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 + scope_ids = (self.organization_id, self.course_id, self.competency_taxonomy_id) + return "org:{},course:{},taxonomy:{}".format(*("" if pk is None else pk for pk in scope_ids)) + + def save(self, *args, **kwargs): + """Recompute scope_code, then persist this profile after full_clean() re-validates it.""" + self.scope_code = self._compute_scope_code() + # Ensure that we run the validations/defaults defined in clean(). + # But don't validate_unique(); it just runs extra queries and the database enforces it anyways. + self.full_clean(validate_unique=False, validate_constraints=False) + super().save(*args, **kwargs) + + +class CompetencyCriterion(models.Model): + """ + A leaf node in a CompetencyAchievementCriteria tree: one tag/object association plus its rule. + + A null ``rule_profile`` does NOT mean "resolve the applicable profile at read time." ADR-0002 + Decision 4 resolves which profile (or override) applies at four specific write events + (creation, a more specific profile appearing later, an author setting a per-criterion + override, and an override being cleared back to matching the computed profile), and stores + the result. ``rule_profile`` is null only when an author has set a per-criterion override; in + every other case it holds the id of the profile that was resolved at the relevant write event + and is never re-resolved dynamically. Do not add a property, manager method, or other helper + that recomputes it; that would contradict the ADR. + + When ``rule_type_override`` is set, its ``rule_payload_override``'s shape (see + :func:`~openedx_learning.applets.cbe.rule_payloads.validate_rule_payload`) is validated from + ``clean()``, reached from both ``objects.create()`` and a plain ``instance.save()`` via + ``full_clean()``. A bulk ``QuerySet.update()``, ``bulk_create()``, and a DRF serializer that + writes straight to the database are NOT covered: none of them build or save a model instance, + so ``clean()`` never runs. + + .. no_pii: + """ + + uuid = immutable_uuid_field() + group = models.ForeignKey( + CompetencyCriteriaGroup, + db_column="competency_criteria_group_id", + on_delete=models.CASCADE, + related_name="criteria", + help_text=_("The CompetencyCriteriaGroup this leaf criterion belongs to."), + ) + object_tag = models.ForeignKey( + ObjectTag, + db_column="oel_tagging_objecttag_id", + on_delete=models.CASCADE, + related_name="competency_criteria", + help_text=_("The tag/object association that this criterion evaluates."), + ) + rule_profile = models.ForeignKey( + CompetencyRuleProfile, + null=True, + blank=True, + db_column="competency_rule_profile_id", + on_delete=models.PROTECT, + related_name="criteria", + help_text=_("The profile this criterion uses by default. Null only when overrides are set instead."), + ) + rule_type_override = models.CharField(max_length=32, choices=RuleType, null=True, blank=True) + rule_payload_override = models.JSONField(null=True, blank=True) + + history = HistoricalRecords() + + class Meta: + # No db_table override: the table is Django's default, openedx_learning_competencycriterion. + # verbose_name/verbose_name_plural are set explicitly because Django's default pluralization + # of "CompetencyCriterion" is "competency criterions". See ADR-0002 Decision 4. + verbose_name = _("Competency Criterion") + verbose_name_plural = _("Competency Criteria") + constraints = [ + models.CheckConstraint( + condition=( + Q( + rule_profile__isnull=False, + rule_type_override__isnull=True, + rule_payload_override__isnull=True, + ) + | Q( + rule_profile__isnull=True, + rule_type_override__isnull=False, + rule_payload_override__isnull=False, + ) + ), + name="oel_cbe_criterion_profile_xor_override_check", + violation_error_message=_( + "A CompetencyCriterion must have either a rule_profile with no overrides, or both override " + "fields set with no rule_profile. Never both, never neither." + ), + ), + ] + + def clean(self): + """Validate the override rule_payload's shape, when a per-criterion override is set.""" + super().clean() + if self.rule_type_override is not None: + validate_rule_payload(self.rule_type_override, self.rule_payload_override) + + def save(self, *args, **kwargs): + """Persist this criterion, after full_clean() re-validates the override payload, if set.""" + self.full_clean(validate_unique=False, validate_constraints=False) + super().save(*args, **kwargs) diff --git a/src/openedx_learning/applets/cbe/rule_payloads.py b/src/openedx_learning/applets/cbe/rule_payloads.py new file mode 100644 index 000000000..24205c4c5 --- /dev/null +++ b/src/openedx_learning/applets/cbe/rule_payloads.py @@ -0,0 +1,88 @@ +""" +Rule payload shapes for CBE evaluation rules, and the validator that checks a raw payload against +the shape its rule_type defines. See :ref:`openedx-learning-adr-0002` Decision 3 for the payload +contract. ``RuleType`` declares exactly the rule types with a shape defined here, so a rule type +can never be offered as a choice without also being saveable. These messages reach an API caller +or admin form, so they must not leak internal class or function names. +""" +from __future__ import annotations + +from typing import Any, Callable + +from django.core.exceptions import ValidationError +from django.db import models +from django.utils.translation import gettext_lazy as _ + +__all__ = [ + "RuleType", + "validate_rule_payload", +] + + +class RuleType(models.TextChoices): + """ + The evaluation rule types a CompetencyRuleProfile or CompetencyCriterion override can use. + + Declares exactly the rule types with a defined rule_payload shape below, i.e. exactly the keys + of ``_RULE_PAYLOAD_SPECS``: see this module's own docstring for why the two are never allowed + to drift apart. + """ + + GRADE = "Grade", _("Grade") + + +_GRADE_OPERATORS = {"gte", "lte", "eq"} + + +def _validate_grade_payload(payload: dict) -> None: + """Validate a Grade payload's op, value, and scale. Keys are already checked.""" + if payload["op"] not in _GRADE_OPERATORS: + raise ValidationError(_("The 'op' in a 'Grade' rule_payload must be one of: gte, lte, eq.")) + value = payload["value"] + # isinstance(True, int) is True in Python, so a bool needs excluding explicitly. + if isinstance(value, bool) or not isinstance(value, (int, float)) or not 0.0 <= value <= 1.0: + raise ValidationError( + _( + "The 'value' in a 'Grade' rule_payload must be a fraction between 0.0 and 1.0 inclusive " + "(e.g. 0.8 for a passing grade of 80%%), not %(value)r." + ) + % {"value": value} + ) + if payload["scale"] != "percent": + raise ValidationError(_("The 'scale' in a 'Grade' rule_payload must be 'percent'.")) + + +# The required keys and validator for each rule type that has a defined payload shape. RuleType +# declares exactly these types, so a rule type can never be offered as a choice without being +# saveable. Adding one is an entry here, a validator, and the matching RuleType member. +_RULE_PAYLOAD_SPECS: dict[str, tuple[frozenset[str], Callable[[dict], None]]] = { + RuleType.GRADE: (frozenset({"op", "value", "scale"}), _validate_grade_payload), +} + + +def validate_rule_payload(rule_type: str, payload: Any) -> None: + """ + Raise ValidationError unless ``payload`` matches the shape ADR-0002 Decision 3 defines for + ``rule_type``, including when ``rule_type`` has no defined shape at all. + """ + spec = _RULE_PAYLOAD_SPECS.get(rule_type) + if spec is None: + raise ValidationError( + _("Rule type '%(rule_type)s' is not supported yet; only 'Grade' has a defined rule_payload shape.") + % {"rule_type": rule_type} + ) + expected_keys, validate_values = spec + if not isinstance(payload, dict): + raise ValidationError(_("A '%(rule_type)s' rule_payload must be a JSON object.") % {"rule_type": rule_type}) + missing = sorted(expected_keys - payload.keys()) + unexpected = sorted(payload.keys() - expected_keys) + if missing or unexpected: + raise ValidationError( + _("A '%(rule_type)s' rule_payload has the wrong keys: missing %(missing)s; unexpected %(unexpected)s.") + % { + "rule_type": rule_type, + "missing": ", ".join(missing) or _("none"), + "unexpected": ", ".join(unexpected) or _("none"), + } + ) + validate_values(payload) diff --git a/src/openedx_learning/migrations/0002_competency_criteria.py b/src/openedx_learning/migrations/0002_competency_criteria.py new file mode 100644 index 000000000..dbcf1854a --- /dev/null +++ b/src/openedx_learning/migrations/0002_competency_criteria.py @@ -0,0 +1,166 @@ +# Generated by Django 5.2.16 on 2026-09-01 19:00 + +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', '0001_initial'), + ('organizations', '0004_auto_20230727_2054'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.AddField( + model_name='competencytaxonomy', + name='taxonomy_overrides_org', + field=models.BooleanField(default=False, help_text="Resolves a tie when assigning a CompetencyRuleProfile to a CompetencyCriterion (ADR-0002 Decision 4): if both an organization-scoped profile and a taxonomy-scoped profile from this taxonomy apply to the same criterion, False (the default) assigns the organization-scoped profile, and True assigns this taxonomy's own profile instead, so it cannot be locally weakened by an organization."), + ), + 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='CompetencyRuleProfile', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), + ('scope_code', models.CharField(editable=False, help_text='Derived from organization/course/competency_taxonomy; null while archived, otherwise "org:X,course:Y,taxonomy:Z" with each segment blank when that scope column is null.', max_length=255, null=True)), + ('rule_type', models.CharField(choices=[('Grade', 'Grade')], max_length=32)), + ('rule_payload', models.JSONField(help_text='Structured payload keyed by rule_type; see validate_rule_payload for the shape it must match.')), + ('archived', models.BooleanField(default=False, help_text="Set instead of deleting a profile that's no longer wanted. Archived profiles are hidden from authoring and new associations but remain queryable, so existing criteria stay resolvable.")), + ('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')), + ], + ), + 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.PROTECT, related_name='criteria', to='openedx_learning.competencyruleprofile')), + ], + options={ + 'verbose_name': 'Competency Criterion', + 'verbose_name_plural': 'Competency Criteria', + }, + ), + migrations.CreateModel( + name='HistoricalCompetencyCriteriaGroup', + fields=[ + ('id', models.BigIntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('uuid', models.UUIDField(db_index=True, default=uuid.uuid4, editable=False, verbose_name='UUID')), + ('name', openedx_django_lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, default='', 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.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='HistoricalCompetencyRuleProfile', + fields=[ + ('id', models.BigIntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('rule_type', models.CharField(choices=[('Grade', 'Grade')], max_length=32)), + ('rule_payload', models.JSONField(help_text='Structured payload keyed by rule_type; see validate_rule_payload for the shape it must match.')), + ('archived', models.BooleanField(default=False, help_text="Set instead of deleting a profile that's no longer wanted. Archived profiles are hidden from authoring and new associations but remain queryable, so existing criteria stay resolvable.")), + ('uuid', models.UUIDField(db_index=True, default=uuid.uuid4, editable=False, verbose_name='UUID')), + ('history_id', models.AutoField(primary_key=True, serialize=False)), + ('history_date', models.DateTimeField(db_index=True)), + ('history_change_reason', models.CharField(max_length=100, null=True)), + ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), + ('competency_taxonomy', models.ForeignKey(blank=True, db_constraint=False, help_text='The competency taxonomy this profile is scoped to, if any.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='openedx_learning.competencytaxonomy')), + ('course', models.ForeignKey(blank=True, db_constraint=False, help_text='The course run this profile is scoped to, if any.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='openedx_catalog.courserun')), + ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), + ('organization', models.ForeignKey(blank=True, db_constraint=False, help_text='The organization this profile is scoped to, if any.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='organizations.organization')), + ], + options={ + 'verbose_name': 'historical competency rule profile', + 'verbose_name_plural': 'historical competency rule profiles', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': ('history_date', 'history_id'), + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + migrations.AddIndex( + model_name='competencycriteriagroup', + index=models.Index(fields=['tag', 'course'], name='openedx_lea_oel_tag_737416_idx'), + ), + migrations.AddConstraint( + model_name='competencyruleprofile', + constraint=models.UniqueConstraint(fields=('scope_code',), name='oel_cbe_ruleprofile_scope_code_uniq'), + ), + migrations.AddConstraint( + model_name='competencyruleprofile', + constraint=models.CheckConstraint(condition=models.Q(models.Q(('course__isnull', True), ('organization__isnull', True)), models.Q(('competency_taxonomy__isnull', True), ('organization__isnull', True)), models.Q(('competency_taxonomy__isnull', True), ('course__isnull', True)), _connector='OR'), name='oel_cbe_ruleprofile_scope_check', violation_error_message='A CompetencyRuleProfile may be scoped to at most one of organization, course, and competency_taxonomy.'), + ), + migrations.AddConstraint( + model_name='competencyruleprofile', + constraint=models.CheckConstraint(condition=models.Q(models.Q(('archived', True), ('scope_code__isnull', True)), models.Q(('archived', False), ('scope_code__isnull', False)), _connector='OR'), name='oel_cbe_ruleprofile_archived_scope_code_check', violation_error_message='An archived CompetencyRuleProfile must have a null scope_code; a live one must not.'), + ), + migrations.AddConstraint( + model_name='competencycriterion', + constraint=models.CheckConstraint(condition=models.Q(models.Q(('rule_payload_override__isnull', True), ('rule_profile__isnull', False), ('rule_type_override__isnull', True)), models.Q(('rule_payload_override__isnull', False), ('rule_profile__isnull', True), ('rule_type_override__isnull', False)), _connector='OR'), name='oel_cbe_criterion_profile_xor_override_check', violation_error_message='A CompetencyCriterion must have either a rule_profile with no overrides, or both override fields set with no rule_profile. Never both, never neither.'), + ), + ] diff --git a/src/openedx_learning/migrations/0003_seed_default_rule_profile.py b/src/openedx_learning/migrations/0003_seed_default_rule_profile.py new file mode 100644 index 000000000..6206d3323 --- /dev/null +++ b/src/openedx_learning/migrations/0003_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', '0002_competency_criteria'), + ] + + operations = [ + migrations.RunPython(seed_default_rule_profile, remove_default_rule_profile), + ] 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..ab5c92da3 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/conftest.py @@ -0,0 +1,67 @@ +"""Shared fixtures for the CBE criteria test modules: schema, deletion, and tree-integration tests.""" +import pytest +from organizations.api import ensure_organization +from organizations.models import Organization + +from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_learning.models import CompetencyCriteriaGroup, CompetencyRuleProfile, CompetencyTaxonomy +from openedx_tagging.models import ObjectTag, Tag + + +@pytest.fixture(name="organization") +def _organization() -> Organization: + """An Organization for use as a scope in these tests.""" + ensure_organization("Org1") + return Organization.objects.get(short_name="Org1") + + +@pytest.fixture(name="organization2") +def _organization2() -> Organization: + """A second Organization, distinct from `organization`, for use as a scope in these tests.""" + ensure_organization("Org2") + return Organization.objects.get(short_name="Org2") + + +@pytest.fixture(name="course_run") +def _course_run(organization: Organization) -> CourseRun: + """A CourseRun for use as a scope in these tests.""" + catalog_course = CatalogCourse.objects.create(org=organization, course_code="Python100") + return CourseRun.objects.create(catalog_course=catalog_course, run_code="Fall2026") + + +@pytest.fixture(name="competency_taxonomy") +def _competency_taxonomy() -> CompetencyTaxonomy: + """A CompetencyTaxonomy for use as a scope, and as the home taxonomy for `tag`.""" + return CompetencyTaxonomy.objects.create(name="Nursing", export_id="nursing-v1") + + +@pytest.fixture(name="tag") +def _tag(competency_taxonomy: CompetencyTaxonomy) -> Tag: + """A Tag, from `competency_taxonomy`, for use as the competency a criteria tree evaluates.""" + return Tag.objects.create(taxonomy=competency_taxonomy, value="Writing Poetry") + + +@pytest.fixture(name="object_tag") +def _object_tag(competency_taxonomy: CompetencyTaxonomy, tag: Tag) -> ObjectTag: + """An ObjectTag associating `tag` with a made-up content object, for use as a criterion's target.""" + return ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+p1", + taxonomy=competency_taxonomy, + tag=tag, + ) + + +@pytest.fixture(name="group") +def _group(tag: Tag) -> CompetencyCriteriaGroup: + """A root CompetencyCriteriaGroup for `tag`, for use as a criterion's parent group.""" + return CompetencyCriteriaGroup.objects.create(tag=tag) + + +@pytest.fixture(name="default_rule_profile") +def _default_rule_profile() -> CompetencyRuleProfile: + """The system-default CompetencyRuleProfile seeded by migration 0003.""" + return CompetencyRuleProfile.objects.get( + organization__isnull=True, + course__isnull=True, + competency_taxonomy__isnull=True, + ) diff --git a/tests/openedx_learning/applets/cbe/test_criteria_deletion.py b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py new file mode 100644 index 000000000..b1f82534e --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py @@ -0,0 +1,444 @@ +""" +Delete-behavior tests for CompetencyCriteriaGroup, CompetencyRuleProfile, and CompetencyCriterion. + +See ADR-0002 Decision 7 for why each foreign key here is CASCADE or PROTECT, including the two +that assert a different value than issue #641 itself specifies, and see the "residual tension" +section below for the one case where a PROTECT raises naming the wrong object. + +Fixtures shared with test_criteria_models.py and test_criteria_trees.py live in this directory's +conftest.py. +""" +import pytest +from django.apps import apps +from django.db import connection +from django.db.models import ProtectedError +from organizations.models import Organization + +from openedx_catalog.models import CourseRun +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + RuleType, +) +from openedx_tagging.models import ObjectTag, Tag + +pytestmark = pytest.mark.django_db + +_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} + + +# ============================================================================================== +# One test per foreign key. The PROTECT ones inspect `protected_objects`, since more than one +# protected relationship can fire on one delete. The CASCADE ones assert the referencing row +# existed before the delete and is gone after, not just that no exception was raised. +# ============================================================================================== + + +def test_deleting_a_group_also_deletes_its_child_groups(tag: Tag) -> None: + """ + Deleting a CompetencyCriteriaGroup cascades to any child group referencing it via `parent`: + the delete succeeds and the child row is gone too. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + assert CompetencyCriteriaGroup.objects.filter(pk=child.pk).exists() + + root.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=root.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=child.pk).exists() + + +def test_deleting_a_tag_also_deletes_its_competency_criteria_groups(tag: Tag, group: CompetencyCriteriaGroup) -> None: + """ + Deleting a Tag cascades to any CompetencyCriteriaGroup referencing it via `tag`: the delete + succeeds and the group row is gone. Also confirms django-simple-history records the cascaded + removal as its own historical row (history_type='-'), not silently: an author or auditor + reviewing history for a group that vanished this way still finds why it did. + """ + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + group_pk = group.pk + + tag.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group_pk).exists() + + historical_group = apps.get_model("openedx_learning", "HistoricalCompetencyCriteriaGroup") + assert historical_group.objects.filter(id=group_pk, history_type="-").exists() + + +def test_deleting_a_course_run_also_deletes_its_course_scoped_criteria_groups( + tag: Tag, course_run: CourseRun +) -> None: + """ + Deleting a CourseRun cascades to any CompetencyCriteriaGroup scoped to it via `course`: the + delete succeeds and the group row is gone too. A course-scoped criteria tree has no meaning + once the course run it evaluates against no longer exists. + """ + group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run) + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + + course_run.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + + +def test_deleting_an_organization_with_a_scoped_profile_raises_protected_error_naming_the_profile( + organization2: Organization, +) -> None: + """ + Deleting an Organization that a CompetencyRuleProfile references via `organization` raises + ProtectedError naming the profile. + + Uses `organization2`, which this test never attaches a CatalogCourse to, instead of + `organization` (the one `course_run` uses elsewhere in this module): CatalogCourse.org is + itself PROTECT, so deleting an organization with a CatalogCourse attached raises + ProtectedError regardless of whether a CompetencyRuleProfile references it too, and this + test would pass for the wrong reason. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization2, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + with pytest.raises(ProtectedError) as exc_info: + organization2.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyRuleProfile) and obj.pk == profile.pk for obj in protected) + + +def test_deleting_a_course_run_with_a_scoped_rule_profile_also_deletes_the_profile( + course_run: CourseRun, +) -> None: + """ + Deleting a CourseRun cascades to any CompetencyRuleProfile scoped to it via `course`: the + delete succeeds and the profile row is gone too. A CompetencyRuleProfile is never hard-deleted + by a *direct* delete of the profile itself (ADR-0002 Decision 7); that does not stop it being + cascaded away as a side effect of deleting the course it is scoped to, once nothing else (no + CompetencyCriterion still assigned to it) protects it -- a course is only ever hard-deleted + once nothing beneath it needs protecting. + """ + profile = CompetencyRuleProfile.objects.create( + course=course_run, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + assert CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + course_run.delete() + + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + +def test_deleting_a_taxonomy_with_a_scoped_rule_profile_also_deletes_the_profile( + competency_taxonomy: CompetencyTaxonomy, +) -> None: + """ + Deleting a CompetencyTaxonomy cascades to any CompetencyRuleProfile scoped to it via + `competency_taxonomy`: the delete succeeds and the profile row is gone too, as #641 + requires. A CompetencyRuleProfile is never hard-deleted by a *direct* delete of the profile itself + (ADR-0002 Decision 7); that does not stop it being cascaded away as a side effect of deleting + the taxonomy it is scoped to, once nothing else protects it. Nothing changes behaviorally in + this MVP, since only the all-null system-default profile exists otherwise, so this scenario + cannot arise until a taxonomy-scoped profile is actually created, which no authoring screen + does yet. See the "residual tension" section below for what happens instead when a + CompetencyCriterion is still assigned to the scoped profile being cascaded away. + """ + profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + assert CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + competency_taxonomy.delete() + + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + +def test_deleting_a_group_also_deletes_its_criteria( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting a CompetencyCriteriaGroup cascades to any CompetencyCriterion referencing it via + `group`: the delete succeeds and the criterion row is gone too. + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + group.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +def test_deleting_an_object_tag_also_deletes_its_criteria( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting an ObjectTag cascades to any CompetencyCriterion referencing it via `object_tag`: the + delete succeeds and the criterion row is gone too. Doubles as the "OURS" half of #641's + Deletions criterion for oel_tagging_objecttag, since ObjectTag has only this one hop down to + CompetencyCriterion. + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + object_tag.delete() + + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +def test_deleting_a_rule_profile_referenced_by_a_criterion_raises_protected_error( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting a CompetencyRuleProfile that a CompetencyCriterion references via `rule_profile` + raises ProtectedError. + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + + with pytest.raises(ProtectedError) as exc_info: + default_rule_profile.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in protected) + + +# ============================================================================================== +# Residual tension: see ADR-0002 Decision 7's "Known limitation" bullet for why deleting a +# taxonomy whose scoped profile is assigned to a criterion raises ProtectedError naming that +# criterion, even though the criterion would also be cascade-deleted in the same operation. +# ============================================================================================== + + +def test_taxonomy_delete_blocked_by_its_scoped_profile_names_the_criterion_not_the_profile( + competency_taxonomy: CompetencyTaxonomy, group: CompetencyCriteriaGroup, object_tag: ObjectTag +) -> None: + """ + Deleting a CompetencyTaxonomy whose taxonomy-scoped profile is itself assigned to a criterion + raises ProtectedError naming the CRITERION, not the profile actually being cascaded away. + + The taxonomy delete cascades into the profile (`competency_taxonomy` is CASCADE), and only then + discovers the profile is referenced by the criterion via `rule_profile` (PROTECT). Django's + PROTECT handler raises unconditionally whenever a referencing row exists in the database; it + never checks whether that same row is also already part of the same delete's pending set, so it + fires here even though this exact criterion would also be reached and removed via the tag chain + (Tag -> CompetencyCriteriaGroup.tag -> CompetencyCriterion.group, all CASCADE) if the profile + hadn't blocked the walk first. This is a spurious, confusing failure -- an author deleting a + taxonomy is told a criterion is in the way, when nothing about that criterion actually survives + the delete either -- but it is not reachable in this MVP (see the section header above), so + this pins the current behavior rather than working around it with a schema change. + """ + profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + criterion = CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=profile) + + with pytest.raises(ProtectedError) as exc_info: + competency_taxonomy.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in protected) + assert not any(isinstance(obj, CompetencyRuleProfile) and obj.pk == profile.pk for obj in protected) + # Nothing was actually removed: the whole operation raised before any DELETE executed. + assert CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +# ============================================================================================== +# MySQL cannot defer foreign-key constraint checks, and Django's CASCADE handler reads that flag +# directly: it nulls a nullable cascading foreign key before the DELETE. On SQLite that nulling +# never happens, so the tests below monkeypatch the flag to reproduce it. Without the monkeypatch +# they pass against broken and correct code alike, so do not drop it. +# ============================================================================================== + + +def test_taxonomy_delete_cascades_its_scoped_profile_under_mysql_collector_semantics( + monkeypatch: pytest.MonkeyPatch, competency_taxonomy: CompetencyTaxonomy +) -> None: + """ + Deleting a CompetencyTaxonomy with a taxonomy-scoped profile succeeds and cascades the profile + away even under MySQL's non-deferred constraint semantics, the same as it does under ordinary + SQLite semantics (see test_deleting_a_taxonomy_with_a_scoped_rule_profile_also_deletes_the_ + profile above). Nulling the profile's `competency_taxonomy_id` before deleting it leaves + `scope_code` alone, so it cannot collide with the seeded system-default profile's identical + blank scope and raise IntegrityError instead of completing the cascade. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + competency_taxonomy.delete() + + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + +def test_course_run_delete_cascades_its_course_scoped_criteria_group_under_mysql_collector_semantics( + monkeypatch: pytest.MonkeyPatch, tag: Tag, course_run: CourseRun +) -> None: + """ + Deleting a CourseRun with a course-scoped CompetencyCriteriaGroup succeeds and cascades the + group away even under MySQL's non-deferred constraint semantics. `course` is one of the two + nullable foreign keys this change turns from PROTECT to CASCADE, so it shares the exact + pre-delete-nulling collector path the taxonomy case above does; unlike scope_code, + CompetencyCriteriaGroup carries no uniqueness constraint a null `course_id` could collide with, + so this path is expected to just succeed. Pinned here anyway, alongside the taxonomy case, + since a future fix to one foreign key without the other would otherwise go unnoticed. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run) + + course_run.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + + +def test_course_run_delete_cascades_its_scoped_rule_profile_under_mysql_collector_semantics( + monkeypatch: pytest.MonkeyPatch, course_run: CourseRun +) -> None: + """ + Deleting a CourseRun with a course-scoped CompetencyRuleProfile succeeds and cascades the + profile away even under MySQL's non-deferred constraint semantics, the same as the taxonomy + case above: `course` is CompetencyRuleProfile's other newly-CASCADE foreign key, and shares the + same pre-delete-nulling collector path and the same scope_code collision this fix removes. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + profile = CompetencyRuleProfile.objects.create( + course=course_run, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + course_run.delete() + + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + +def test_deleting_two_taxonomies_together_cascades_both_their_scoped_profiles_away( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """ + Deleting two CompetencyTaxonomy rows in one `.delete()` call, each with its own taxonomy-scoped + profile, succeeds and cascades both profiles away -- neither profile's scope_code collides with + the other's, even though both get their `competency_taxonomy_id` nulled in the same collector + batch under MySQL's non-deferred constraint semantics. + + Same path as the single-taxonomy MySQL case above, but confirms it does not get worse when two + scope owners are collected in the same collector pass: before scope_code became a plain column, + nulling both profiles' `competency_taxonomy_id` in the same batch drove both scope_code values + to the identical blank "org:,course:,taxonomy:" string and raised IntegrityError on whichever + row the database processed second. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + taxonomy1 = CompetencyTaxonomy.objects.create(name="Nursing Two Taxonomy Delete", export_id="nursing-two-del") + taxonomy2 = CompetencyTaxonomy.objects.create(name="Welding Two Taxonomy Delete", export_id="welding-two-del") + profile1 = CompetencyRuleProfile.objects.create( + competency_taxonomy=taxonomy1, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile2 = CompetencyRuleProfile.objects.create( + competency_taxonomy=taxonomy2, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + CompetencyTaxonomy.objects.filter(pk__in=[taxonomy1.pk, taxonomy2.pk]).delete() + + assert not CompetencyRuleProfile.objects.filter(pk__in=[profile1.pk, profile2.pk]).exists() + + +# ============================================================================================== +# Transitive deletion tests: deleting an oel_tagging.Tag, a CompetencyCriteriaGroup at depth, an +# oel_tagging.ObjectTag, or an oel_tagging.Taxonomy, with no learner status beneath the target, +# must succeed and take the whole referencing criteria tree with it. The other half of each, a +# ProtectedError once a learner status row exists beneath it, needs #642's Student*Status tables +# and is deliberately not stubbed here. See test_criteria_trees.py for the fuller integrative +# version, asserting exactly which rows survive rather than only that a cascade fired. +# ============================================================================================== + + +def test_tag_delete_with_no_status_cascades_whole_criteria_tree( + tag: Tag, group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting an oel_tagging.Tag with no learner status beneath it succeeds and cascades away + every CompetencyCriteriaGroup and CompetencyCriterion that references it, transitively: + Tag -> CompetencyCriteriaGroup.tag (CASCADE) -> CompetencyCriterion.group (CASCADE). + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + tag.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +def test_group_delete_at_depth_cascades_descendants_and_their_criteria( + tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting a CompetencyCriteriaGroup that is not a root removes it, every descendant group, and + every CompetencyCriterion under any of them, while leaving the rest of the tree (here, the + root) alone. + + Builds a genuinely nested tree, root -> child -> grandchild, with criteria at two different + levels (on `child` and on `grandchild`), so "at depth" and "every descendant" both mean + something: a shallower tree could pass this by accident. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + grandchild = CompetencyCriteriaGroup.objects.create(tag=tag, parent=child) + child_criterion = CompetencyCriterion.objects.create( + group=child, object_tag=object_tag, rule_profile=default_rule_profile + ) + grandchild_criterion = CompetencyCriterion.objects.create( + group=grandchild, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert CompetencyCriteriaGroup.objects.filter(pk=root.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=child.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=grandchild.pk).exists() + assert CompetencyCriterion.objects.filter(pk=child_criterion.pk).exists() + assert CompetencyCriterion.objects.filter(pk=grandchild_criterion.pk).exists() + + child.delete() + + assert CompetencyCriteriaGroup.objects.filter(pk=root.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=child.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=grandchild.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=child_criterion.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=grandchild_criterion.pk).exists() + + +def test_taxonomy_delete_cascades_every_tag_and_its_criteria( + competency_taxonomy: CompetencyTaxonomy, + tag: Tag, + group: CompetencyCriteriaGroup, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Deleting an oel_tagging.Taxonomy collects every Tag beneath it (Tag.taxonomy is CASCADE), so + the tag-deletion cases above hold transitively through a taxonomy delete too. This asserts the + succeeding case (no learner status beneath the tag), which is what #641's Deletions criterion + for taxonomy-level deletion requires "at minimum". + + Chain exercised: CompetencyTaxonomy -> Tag (CASCADE) -> CompetencyCriteriaGroup.tag (CASCADE) + -> CompetencyCriterion.group (CASCADE). + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert Tag.objects.filter(pk=tag.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + competency_taxonomy.delete() + + assert not Tag.objects.filter(pk=tag.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() diff --git a/tests/openedx_learning/applets/cbe/test_criteria_models.py b/tests/openedx_learning/applets/cbe/test_criteria_models.py new file mode 100644 index 000000000..cb03239f0 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_models.py @@ -0,0 +1,727 @@ +""" +Tests for CompetencyCriteriaGroup, CompetencyRuleProfile, and CompetencyCriterion. + +Fixtures shared with test_criteria_deletion.py and test_criteria_trees.py live in this directory's +conftest.py. +""" +import pytest +from django.apps import apps +from django.core.exceptions import ValidationError +from django.db import connection, models, transaction +from django.db.utils import IntegrityError +from organizations.models import Organization + +from openedx_catalog.models import CatalogCourse, CourseRun +# Private: the payload-spec registry, compared against RuleType's declared choices below. +from openedx_learning.applets.cbe.rule_payloads import _RULE_PAYLOAD_SPECS +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + LogicOperator, + RuleType, +) +from openedx_tagging.models import ObjectTag, Tag, Taxonomy + +pytestmark = pytest.mark.django_db + +_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} + +# One (rule_type, payload) pair per way ADR-0002 Decision 3 says a rule_payload can be invalid. +_INVALID_GRADE_PAYLOADS = [ + pytest.param(RuleType.GRADE, {"op": "startswith", "value": 0.8, "scale": "percent"}, id="bad_op"), + pytest.param(RuleType.GRADE, {"op": "gte", "value": 80, "scale": "percent"}, id="value_80_not_0_8"), + pytest.param(RuleType.GRADE, {"op": "gte", "value": 1.5, "scale": "percent"}, id="value_out_of_range"), + pytest.param(RuleType.GRADE, {"op": "gte", "scale": "percent"}, id="missing_key"), + pytest.param(RuleType.GRADE, {**_GRADE_PAYLOAD, "extra": 1}, id="extra_key"), + pytest.param(RuleType.GRADE, ["not", "a", "dict"], id="non_dict"), + pytest.param(RuleType.GRADE, {"op": "gte", "value": 0.8, "scale": "raw"}, id="wrong_scale"), + pytest.param(RuleType.GRADE, {"op": "gte", "value": True, "scale": "percent"}, id="boolean_value"), + # "View" is a plain string, not RuleType.VIEW: RuleType declares only rule types that have a + # defined payload spec (see test_rule_type_choices_match_rule_types_with_a_defined_payload_spec + # below), so an unsupported rule type is, by construction, one that isn't a RuleType member at + # all. Behaviorally identical either way, since a TextChoices member IS its string value. + pytest.param("View", _GRADE_PAYLOAD, id="unsupported_rule_type"), +] + + +# ============================================================================================== +# Schema and columns. CompetencyTaxonomy's own taxonomy_overrides_org default is covered in +# test_models.py, not duplicated here. +# ============================================================================================== + + +def test_group_has_exactly_the_columns_adr_0002_decision_2_lists() -> None: + """ + CompetencyCriteriaGroup's columns are exactly the ones ADR-0002 Decision 2 lists, with + `parent`, `course`, and `logic_operator` optional and the rest required. `tag` keeps the + legacy `oel_tagging_tag_id` column name. No `archived` column yet; that arrives with #642. + """ + fields = [f for f in CompetencyCriteriaGroup._meta.get_fields() if f.concrete] + assert {f.name for f in fields} == { + "id", "uuid", "parent", "tag", "course", "name", "ordering", "logic_operator", + } + assert {f.name for f in fields if f.null} == {"parent", "course", "logic_operator"} + assert CompetencyCriteriaGroup._meta.get_field("parent").remote_field.model is CompetencyCriteriaGroup + assert CompetencyCriteriaGroup._meta.get_field("tag").remote_field.model is Tag + assert CompetencyCriteriaGroup._meta.get_field("tag").db_column == "oel_tagging_tag_id" + assert CompetencyCriteriaGroup._meta.get_field("course").remote_field.model is CourseRun + + +def test_rule_profile_has_exactly_the_columns_adr_0002_decision_3_lists() -> None: + """ + CompetencyRuleProfile's columns are exactly the ones ADR-0002 Decision 3 lists, with + `organization`, `course`, `competency_taxonomy`, and `scope_code` nullable and the rest + required. `scope_code` is nullable, not "never null": it is null exactly while a profile is + archived, which is what frees that scope's unique slot for a replacement. See ADR-0002 + Decision 3. + """ + fields = [f for f in CompetencyRuleProfile._meta.get_fields() if f.concrete] + assert {f.name for f in fields} == { + "id", "uuid", "organization", "course", "competency_taxonomy", "scope_code", "rule_type", + "rule_payload", "archived", + } + assert {f.name for f in fields if f.null} == {"organization", "course", "competency_taxonomy", "scope_code"} + assert CompetencyRuleProfile._meta.get_field("organization").remote_field.model is Organization + assert CompetencyRuleProfile._meta.get_field("course").remote_field.model is CourseRun + assert CompetencyRuleProfile._meta.get_field("competency_taxonomy").remote_field.model is CompetencyTaxonomy + + +def test_criterion_has_exactly_the_columns_adr_0002_decision_4_lists() -> None: + """ + CompetencyCriterion's columns are exactly the ones ADR-0002 Decision 4 lists, with + `rule_profile`, `rule_type_override`, and `rule_payload_override` optional and the rest + required. No `archived` column yet; that arrives with #642. Carries no Meta.db_table + override, so the table is Django's default name for the class. + """ + fields = [f for f in CompetencyCriterion._meta.get_fields() if f.concrete] + assert {f.name for f in fields} == { + "id", "uuid", "group", "object_tag", "rule_profile", "rule_type_override", "rule_payload_override", + } + assert {f.name for f in fields if f.null} == {"rule_profile", "rule_type_override", "rule_payload_override"} + assert CompetencyCriterion._meta.get_field("group").db_column == "competency_criteria_group_id" + assert CompetencyCriterion._meta.get_field("object_tag").db_column == "oel_tagging_objecttag_id" + assert CompetencyCriterion._meta.get_field("rule_profile").db_column == "competency_rule_profile_id" + assert CompetencyCriterion._meta.db_table == "openedx_learning_competencycriterion" + + +# ============================================================================================== +# Constraints and validation. +# ============================================================================================== + + +@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) + 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. + """ + unique_constraints = [ + c for c in CompetencyCriteriaGroup._meta.constraints if isinstance(c, models.UniqueConstraint) + ] + assert not any({"parent", "ordering"} <= set(c.fields) for c in unique_constraints) + + parent = CompetencyCriteriaGroup.objects.create(tag=tag) + sibling_a = CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent, ordering=1) + sibling_b = CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent, ordering=1) + assert sibling_a.ordering == sibling_b.ordering == 1 + + +@pytest.mark.parametrize( + "scope_kwargs", + [ + pytest.param({"organization": True}, id="organization_only"), + pytest.param({"course": True}, id="course_only"), + pytest.param({"competency_taxonomy": True}, id="competency_taxonomy_only"), + pytest.param({}, id="no_scope_system_default"), + ], +) +def test_rule_profile_scope_check_constraint_accepts_at_most_one_scope_field( + scope_kwargs: dict, + organization: Organization, + course_run: CourseRun, + competency_taxonomy: CompetencyTaxonomy, +) -> None: + """ + The scope check constraint accepts a CompetencyRuleProfile scoped to at most one of + organization, course, or competency_taxonomy, including none of them (the system default). + See ADR-0002 Decision 3. + """ + # Free the all-null slot the seed migration (0003) occupies, so the "no scope" case can be + # tested in isolation from scope_code's own uniqueness constraint, which has its own tests. + CompetencyRuleProfile.objects.filter( + organization__isnull=True, course__isnull=True, competency_taxonomy__isnull=True + ).delete() + + resolved_kwargs: dict[str, object] = {} + if scope_kwargs.get("organization"): + resolved_kwargs["organization"] = organization + if scope_kwargs.get("course"): + resolved_kwargs["course"] = course_run + if scope_kwargs.get("competency_taxonomy"): + resolved_kwargs["competency_taxonomy"] = competency_taxonomy + + profile = CompetencyRuleProfile.objects.create( + rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD, **resolved_kwargs + ) + assert profile.pk is not None + + +@pytest.mark.parametrize( + "scoped_fields", + [ + pytest.param(("organization", "course"), id="organization_and_course"), + pytest.param(("organization", "competency_taxonomy"), id="organization_and_taxonomy"), + pytest.param(("course", "competency_taxonomy"), id="course_and_taxonomy"), + pytest.param(("organization", "course", "competency_taxonomy"), id="all_three"), + ], +) +def test_rule_profile_scope_check_constraint_rejects_more_than_one_scope_field( + scoped_fields: tuple[str, ...], + organization: Organization, + course_run: CourseRun, + competency_taxonomy: CompetencyTaxonomy, +) -> None: + """ + The scope check constraint rejects a CompetencyRuleProfile scoped to any two of organization, + course, and competency_taxonomy, or to all three. See ADR-0002 Decision 3. + """ + available_values = {"organization": organization, "course": course_run, "competency_taxonomy": competency_taxonomy} + scope_kwargs = {field_name: available_values[field_name] for field_name in scoped_fields} + + with pytest.raises(IntegrityError): + with transaction.atomic(): + CompetencyRuleProfile.objects.create(rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD, **scope_kwargs) + + +def test_scope_code_matches_org_course_taxonomy_format_for_each_scope_shape( + organization: Organization, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy +) -> None: + """ + A live (non-archived) profile's scope_code is "org:X,course:Y,taxonomy:Z", with each segment + left blank when the corresponding scope column is null. See ADR-0002 Decision 3. + """ + CompetencyRuleProfile.objects.filter( + organization__isnull=True, course__isnull=True, competency_taxonomy__isnull=True + ).delete() + + all_null = CompetencyRuleProfile.objects.create(rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD) + org_only = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + course_only = CompetencyRuleProfile.objects.create( + course=course_run, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + taxonomy_only = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + for profile in (all_null, org_only, course_only, taxonomy_only): + profile.refresh_from_db() + + assert all_null.scope_code == "org:,course:,taxonomy:" + assert org_only.scope_code == f"org:{organization.pk},course:,taxonomy:" + assert course_only.scope_code == f"org:,course:{course_run.pk},taxonomy:" + assert taxonomy_only.scope_code == f"org:,course:,taxonomy:{competency_taxonomy.pk}" + + +def test_scope_code_is_null_once_archived_and_non_null_while_live(organization: Organization) -> None: + """ + scope_code is non-null while a profile is live, and becomes null once it is archived. An + archived profile no longer holds its scope's unique slot, which is what lets a replacement be + created for that same scope (see test_archiving_a_profile_frees_its_scope_for_a_replacement + below); a profile that stayed occupying a non-null scope_code after archiving would block that + forever. This is a deliberate design point, not an oversight: a plain nullable column, written + explicitly whenever a profile is saved, rather than a database-computed value that can never + tell "archived" apart from "live" on its own. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.refresh_from_db() + assert profile.scope_code == f"org:{organization.pk},course:,taxonomy:" + + profile.archived = True + profile.save() + profile.refresh_from_db() + assert profile.scope_code is None + + +def test_archiving_a_profile_frees_its_scope_for_a_replacement(organization: Organization) -> None: + """ + Once a profile scoped to a given organization/course/taxonomy is archived, a brand new profile + may be created for that exact same scope: the archived row's scope_code goes to null and stops + occupying the unique slot, so it no longer collides with the replacement's non-null scope_code. + """ + original = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + original.archived = True + original.save() + + replacement = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + replacement.refresh_from_db() + original.refresh_from_db() + + assert original.scope_code is None + assert replacement.scope_code == f"org:{organization.pk},course:,taxonomy:" + + +def test_two_live_profiles_cannot_share_the_same_scope(organization: Organization) -> None: + """ + Two live CompetencyRuleProfile rows cannot share the same scope. In particular, two rows that + both set only `organization` (leaving course and competency_taxonomy null) collide, which is + exactly the case a plain UniqueConstraint on the three raw nullable columns would not catch, + since SQL never treats two NULLs as equal. See ADR-0002 Decision 3. + """ + CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + with pytest.raises(IntegrityError): + with transaction.atomic(): + CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + +@pytest.mark.parametrize( + "invalid_kwargs", + [ + pytest.param( + {"rule_type_override": RuleType.GRADE, "rule_payload_override": _GRADE_PAYLOAD, "use_profile": True}, + id="both_set", + ), + pytest.param({"use_profile": False}, id="neither_set"), + pytest.param({"rule_payload_override": _GRADE_PAYLOAD, "use_profile": False}, id="only_payload_override_set"), + ], +) +def test_criterion_profile_xor_override_check_constraint_rejects_invalid_states( + invalid_kwargs: dict, + group: CompetencyCriteriaGroup, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A CompetencyCriterion must have either a rule_profile with no overrides, or both override + fields set with no rule_profile, never both and never neither. See ADR-0002 Decision 4. + + Covers the three invalid states that reach the database's check constraint: both set, neither + set, and only rule_payload_override set. The fourth invalid state, only rule_type_override set, + is caught earlier by save()'s own validation instead and raises ValidationError before the + database is ever touched; see test_setting_a_rule_type_override_without_a_payload_is_rejected_by_save + below for that case, and why it raises a different exception type than these three. + """ + use_profile = invalid_kwargs.pop("use_profile") + kwargs = dict(invalid_kwargs) + if use_profile: + kwargs["rule_profile"] = default_rule_profile + + with pytest.raises(IntegrityError): + with transaction.atomic(): + CompetencyCriterion.objects.create(group=group, object_tag=object_tag, **kwargs) + + +def test_criterion_accepts_either_a_rule_profile_or_both_overrides( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Both valid states of the profile-xor-overrides check constraint save successfully: a + rule_profile with no overrides, and both override fields set with no rule_profile. + See ADR-0002 Decision 4. + """ + with_profile = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert with_profile.pk is not None + + with_overrides = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_type_override=RuleType.GRADE, rule_payload_override=_GRADE_PAYLOAD + ) + assert with_overrides.pk is not None + + +def test_setting_a_rule_type_override_without_a_payload_is_rejected_by_save( + group: CompetencyCriteriaGroup, object_tag: ObjectTag +) -> None: + """ + Setting only rule_type_override, leaving rule_payload_override null, is caught by save()'s + own validation before it ever reaches the database: save() validates rule_payload_override's + shape whenever rule_type_override is set, and None is not a valid shape for any rule type, so + this raises ValidationError. The database's check constraint would also reject this same row, + for the same underlying reason (an override with no real payload), but save() never lets it + get there. This is why two similar-looking invalid override states raise different exception + types: this one is caught by save()'s validate_rule_payload call, while the other three (see + test_criterion_profile_xor_override_check_constraint_rejects_invalid_states above) reach the + database's check constraint, because the payload save() inspects for them is either valid or, + when rule_type_override itself is null, not inspected at all. + """ + with pytest.raises(ValidationError): + CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_type_override=RuleType.GRADE) + + +@pytest.mark.parametrize("rule_type, payload", _INVALID_GRADE_PAYLOADS) +def test_rule_profile_full_clean_rejects_invalid_payload(rule_type: str, payload: object) -> None: + """ + full_clean() raises ValidationError for a CompetencyRuleProfile on every documented way a + rule_payload can be invalid: a bad op, a value given on a 0-100 scale instead of 0.0-1.0, a + value outside that range, a missing or extra key, a non-dict payload, a wrong scale, a + boolean value, and a rule_type with no defined payload shape yet. See ADR-0002 Decision 3. + """ + profile = CompetencyRuleProfile(rule_type=rule_type, rule_payload=payload) + with pytest.raises(ValidationError): + profile.full_clean() + + +def test_rule_profile_full_clean_value_message_names_the_fraction_convention(organization: Organization) -> None: + """ + full_clean()'s error for a rule_payload 'value' given on a 0-100 scale (e.g. 80) names the + 0.0-1.0 fraction convention. Every other invalid-payload test here only asserts the exception + type; this one asserts the message content. + """ + profile = CompetencyRuleProfile( + organization=organization, rule_type=RuleType.GRADE, rule_payload={"op": "gte", "value": 80, "scale": "percent"} + ) + with pytest.raises(ValidationError) as exc_info: + profile.full_clean() + + message = " ".join(exc_info.value.messages) + assert "fraction between 0.0 and 1.0" in message + + +def test_rule_profile_full_clean_extra_key_message_names_the_key(organization: Organization) -> None: + """ + full_clean()'s error for an unrecognized rule_payload key names that key in our own domain + language (e.g. "unexpected extra"). + """ + profile = CompetencyRuleProfile( + organization=organization, + rule_type=RuleType.GRADE, + rule_payload={**_GRADE_PAYLOAD, "extra": 1}, + ) + with pytest.raises(ValidationError) as exc_info: + profile.full_clean() + + message = " ".join(exc_info.value.messages) + assert "extra" in message + + +@pytest.mark.parametrize("rule_type, payload", _INVALID_GRADE_PAYLOADS) +def test_criterion_full_clean_rejects_invalid_override_payload( + rule_type: str, payload: object, group: CompetencyCriteriaGroup, object_tag: ObjectTag +) -> None: + """ + full_clean() raises ValidationError for a CompetencyCriterion's rule_payload_override on the + same invalid shapes as CompetencyRuleProfile.rule_payload. See ADR-0002 Decision 3. + """ + criterion = CompetencyCriterion( + group=group, object_tag=object_tag, rule_type_override=rule_type, rule_payload_override=payload + ) + with pytest.raises(ValidationError): + criterion.full_clean() + + +def test_rule_type_choices_match_rule_types_with_a_defined_payload_spec() -> None: + """ + RuleType's declared choices (what a serializer or an admin form offers an author) must contain + exactly the rule types that can actually be saved. ADR-0002 Decision 3 defines a rule_payload + shape per rule_type, and a rule_type with no defined shape is always rejected by + validate_rule_payload's "not supported yet" branch, regardless of payload content. RuleType + therefore declares only the rule types with a payload-spec entry (currently just Grade); a + future rule type not yet built (a "View" or "MasteryLevel") is neither a RuleType member nor a + declared choice until both its spec class and its RuleType member land together. This pins + that invariant so declaring a new RuleType member and forgetting its payload spec (or vice + versa) fails a test instead of shipping a dead-end choice. + """ + declared_rule_types = {choice_value for choice_value, _label in RuleType.choices} + enforced_rule_types = set(_RULE_PAYLOAD_SPECS.keys()) + assert declared_rule_types == enforced_rule_types + + +def test_criterion_rule_profile_is_not_recomputed_once_a_more_specific_profile_appears( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, + competency_taxonomy: CompetencyTaxonomy, +) -> None: + """ + A criterion's stored rule_profile is not resolved dynamically at read time: creating a new, + more specific profile later does not silently re-govern a criterion that already resolved to a + less specific one. See ADR-0002 Decision 4, which lists the specific write events that DO + reassign a criterion (not exercised here) and states that no other path may recompute it. This + guards against a property, manager method, or signal handler being added that would violate + that rule by resolving the FK on every read instead of only at those write events. + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + + CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + criterion.refresh_from_db() + assert criterion.rule_profile_id == default_rule_profile.pk + + +# ============================================================================================== +# Scope immutability. Each scope field gets its own rejection test; rule_type, rule_payload, and +# archived changing on the same row is asserted separately as the case that must still work. +# ============================================================================================== + + +def test_scope_immutability_rejects_organization_change( + organization: Organization, organization2: Organization +) -> None: + """ + Changing a CompetencyRuleProfile's `organization` after creation raises ValidationError on + save(). See ADR-0002 Decision 3. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.organization = organization2 + with pytest.raises(ValidationError): + profile.save() + + +def test_scope_immutability_rejects_course_change(organization: Organization, course_run: CourseRun) -> None: + """ + Changing a CompetencyRuleProfile's `course` after creation raises ValidationError on save(). + See ADR-0002 Decision 3. + """ + other_catalog_course = CatalogCourse.objects.create(org=organization, course_code="Python200") + other_course_run = CourseRun.objects.create(catalog_course=other_catalog_course, run_code="Spring2027") + + profile = CompetencyRuleProfile.objects.create( + course=course_run, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.course = other_course_run + with pytest.raises(ValidationError): + profile.save() + + +def test_scope_immutability_rejects_taxonomy_change(competency_taxonomy: CompetencyTaxonomy) -> None: + """ + Changing a CompetencyRuleProfile's `competency_taxonomy` after creation raises ValidationError + on save(). See ADR-0002 Decision 3. + """ + other_taxonomy = CompetencyTaxonomy.objects.create(name="Welding", export_id="welding-v1") + + profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.competency_taxonomy = other_taxonomy + with pytest.raises(ValidationError): + profile.save() + + +def test_scope_immutability_allows_rule_type_rule_payload_and_archived_to_change(organization: Organization) -> None: + """ + Only rule_type, rule_payload, and archived may change after creation; changing any of them (as + opposed to a scope field) succeeds. See ADR-0002 Decision 3. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.rule_type = RuleType.GRADE + profile.rule_payload = {"op": "lte", "value": 0.5, "scale": "percent"} + profile.archived = True + profile.save() + + profile.refresh_from_db() + assert profile.rule_payload == {"op": "lte", "value": 0.5, "scale": "percent"} + assert profile.archived is True + + +def test_scope_immutability_enforced_after_deferred_load( + organization: Organization, organization2: Organization +) -> None: + """ + Scope immutability is enforced even when the profile was loaded with .only()/.defer() and so + never loaded the scope columns into this instance in the first place. + _check_scope_immutable() always queries the persisted scope directly (see its docstring), so a + partial load is not a way to bypass this check. + + Uses a second organization rather than setting the scope to None: a null scope would collide + with the seeded system-default row, so the unique constraint would raise IntegrityError and + the scope guard would never be reached. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + deferred = CompetencyRuleProfile.objects.only("id", "rule_type").get(pk=profile.pk) + + deferred.organization = organization2 + with pytest.raises(ValidationError): + deferred.save() + + +# NOTE: there is no test that `_check_scope_immutable()`'s fallback query targets +# `self._state.db` rather than the default alias. Proving that needs an instance loaded from a +# second database alias, and this suite configures only "default". Adding a second alias makes +# pytest-django run the whole migration history against it, which fails in +# openedx_content/backcompat/collections/migrations/0004_collection_key.py: its `generate_keys` +# RunPython step queries Collection.objects without `.using(schema_editor.connection.alias)`, so +# it always hits "default". That is a pre-existing bug in an unrelated app, but it breaks +# database setup for the entire session, not just this test. The alternative, assigning +# `instance._state.db` directly, is idiomatic in Django's own tests but trips this repo's +# enabled pylint `protected-access` check, and silencing that is not allowed. The one-line +# `.using(self._state.db)` in the model is correct by inspection; this is a known test gap. + + +# ============================================================================================== +# Indexes, history. +# ============================================================================================== + + +def test_database_indexes_from_adr_decision_5_are_present() -> None: + """ + The real database tables carry the ADR-0002 Decision 5 indexes this migration is responsible + for: positions 1, 2, 4, 5 (all covering indexes), and 9 (unique). Positions 2, 4, and 5 come + from Django's automatic per-ForeignKey index rather than an explicit models.Index; this test + introspects the database, not the model, so it holds regardless of which mechanism produced + the index. Positions 3, 6, 7, 8, and 10 belong to tables this migration doesn't create. + """ + with connection.cursor() as cursor: + group_constraints = connection.introspection.get_constraints(cursor, CompetencyCriteriaGroup._meta.db_table) + criterion_constraints = connection.introspection.get_constraints(cursor, CompetencyCriterion._meta.db_table) + profile_constraints = connection.introspection.get_constraints(cursor, CompetencyRuleProfile._meta.db_table) + + def is_indexed(constraints: dict, columns: list[str]) -> bool: + # Compare the ordered column list, not a set: column order is the whole point of a + # composite index. An index on (course_id, oel_tagging_tag_id) would satisfy a set + # comparison against ADR index 1 just as well as (oel_tagging_tag_id, course_id), but + # only the tag-first ordering also serves tag-only lookups. + return any(c["columns"] == columns and c["index"] for c in constraints.values()) + + # 1: CompetencyCriteriaGroup(tag, course), the one explicit composite index. + assert is_indexed(group_constraints, ["oel_tagging_tag_id", "course_id"]) + # 2: CompetencyCriteriaGroup(parent). + assert is_indexed(group_constraints, ["parent_id"]) + # 4: CompetencyCriteria(object_tag). + assert is_indexed(criterion_constraints, ["oel_tagging_objecttag_id"]) + # 5: CompetencyCriteria(group). + assert is_indexed(criterion_constraints, ["competency_criteria_group_id"]) + # 9: CompetencyRuleProfile(scope_code), unique. + assert any( + set(c["columns"]) == {"scope_code"} and c["unique"] for c in profile_constraints.values() + ) + + +def test_history_recorded_for_group_profile_and_criterion( + organization: Organization, + group: CompetencyCriteriaGroup, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + HistoricalRecords() is applied to CompetencyCriteriaGroup, CompetencyRuleProfile, and + CompetencyCriterion: each is registered in the app registry under its expected Historical* + name, and editing an instance writes a row there. See ADR-0003 Decisions 1 and 2. + + Historical* models are looked up via the app registry rather than the `.history` attribute + because simple_history installs `.history` as a runtime descriptor with no type stubs, which + mypy cannot type; apps.get_model() returns something mypy can call `.objects` on. + """ + historical_group = apps.get_model("openedx_learning", "HistoricalCompetencyCriteriaGroup") + historical_profile = apps.get_model("openedx_learning", "HistoricalCompetencyRuleProfile") + historical_criterion = apps.get_model("openedx_learning", "HistoricalCompetencyCriterion") + + group.name = "Poetry Mastery" + group.save() + assert historical_group.objects.filter(id=group.pk).count() == 2 + + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.rule_payload = {"op": "gte", "value": 0.9, "scale": "percent"} + profile.save() + assert historical_profile.objects.filter(id=profile.pk).count() == 2 + + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + criterion.rule_profile = None + criterion.rule_type_override = RuleType.GRADE + criterion.rule_payload_override = _GRADE_PAYLOAD + criterion.save() + assert historical_criterion.objects.filter(id=criterion.pk).count() == 2 + + +def test_history_not_recorded_for_tag_taxonomy_or_competencytaxonomy(competency_taxonomy: CompetencyTaxonomy) -> None: + """ + django-simple-history is NOT applied to oel_tagging_tag, oel_tagging_taxonomy, or + CompetencyTaxonomy: none of the three has a `.history` attribute, and no Historical* model is + registered for any of them. See ADR-0003 Decisions 1 and 2 for why history tracking stops at + the CBE-specific models and does not reach back into the generic tagging models they build on. + """ + assert not hasattr(Tag, "history") + assert not hasattr(Taxonomy, "history") + assert not hasattr(competency_taxonomy, "history") + + for app_label, model_name in [ + ("oel_tagging", "HistoricalTag"), + ("oel_tagging", "HistoricalTaxonomy"), + ("openedx_learning", "HistoricalCompetencyTaxonomy"), + ]: + with pytest.raises(LookupError): + apps.get_model(app_label, model_name) + + +# ============================================================================================== +# Migrations. No-makemigrations-drift and running this suite against MySQL are verified by +# running manage.py / the MySQL settings module, not by a unit test. +# ============================================================================================== + + +def test_migration_seeds_exactly_one_system_default_rule_profile() -> None: + """ + Migration 0003 seeds exactly one system-default CompetencyRuleProfile: all three scope + columns null, not archived, Grade >= 0.8 (80%). See ADR-0002 Decision 3. + """ + profile = CompetencyRuleProfile.objects.get( + organization__isnull=True, course__isnull=True, competency_taxonomy__isnull=True + ) + assert profile.archived is False + assert profile.rule_type == RuleType.GRADE + assert profile.rule_payload == _GRADE_PAYLOAD diff --git a/tests/openedx_learning/applets/cbe/test_criteria_trees.py b/tests/openedx_learning/applets/cbe/test_criteria_trees.py new file mode 100644 index 000000000..1b1988744 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_trees.py @@ -0,0 +1,144 @@ +""" +Integrative tests for CompetencyAchievementCriteria trees. + +test_criteria_deletion.py proves each foreign key cascades or protects correctly in isolation. +That is not the same claim as "deleting somewhere in the middle of a realistic tree leaves exactly +the right rows behind and nothing else": a per-foreign-key test can pass while a wider tree still +ends up with an orphaned group, a criterion pointing at nothing, or a sibling branch disturbed by +a delete that should not have touched it. The tests here build a wider tree on purpose and assert +the full surviving/removed row set, not just that a cascade fired somewhere. + +Fixtures shared with test_criteria_models.py and test_criteria_deletion.py live in this directory's +conftest.py. +""" +import pytest + +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + RuleType, +) +from openedx_tagging.models import ObjectTag, Tag + +pytestmark = pytest.mark.django_db + +_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} + + +def test_object_tag_delete_leaves_a_childless_criteria_group_behind( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting an ObjectTag cascades away the CompetencyCriterion that references it, but leaves the + CompetencyCriteriaGroup that housed that criterion in place, even when it was the group's only + criterion and the group now has no children of any kind (no criteria, no child groups). + + This is a deliberately accepted outcome, not a bug: CompetencyCriteriaGroup does not reference + ObjectTag at all (only CompetencyCriterion does), so nothing about deleting an ObjectTag gives + the collector a reason to reach the group. A childless group left behind this way is inert (it + evaluates no criteria and contributes nothing to its parent's logic_operator combination) and + is exactly the state authoring tooling must already handle for a group edited down to zero + children, so no additional cleanup path exists for this narrower case either. Pinned here so a + future change one way or the other (cascading the now-childless group away, or continuing to + leave it) is a deliberate decision, not an accidental side effect of something else. + """ + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + object_tag.delete() + + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + assert CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + assert not CompetencyCriteriaGroup.objects.get(pk=group.pk).criteria.exists() + + +def test_deleting_a_middle_group_removes_its_subtree_but_leaves_the_rest_of_the_tree_untouched( + tag: Tag, competency_taxonomy: CompetencyTaxonomy, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + Deleting a CompetencyCriteriaGroup partway down a realistic tree removes exactly that group, + every descendant beneath it, and every criterion under any of them -- and nothing else. A + sibling branch of the deleted group, with its own criterion, survives completely unchanged. + + Tree built here, all under one root: + + root + |-- branch_to_delete (criterion: profile-assigned, via default_rule_profile) + | `-- grandchild (criterion: override, no rule_profile) + `-- surviving_sibling (criterion: profile-assigned, via a taxonomy-scoped profile) + + `branch_to_delete` is deleted. This exercises criteria at two different tree depths (on + `branch_to_delete` itself and on its child `grandchild`) with a genuine mix of the two ways a + criterion can be governed (a stored `rule_profile` vs. per-criterion overrides), and confirms + `surviving_sibling` and its own criterion are byte-for-byte untouched: same primary keys, still + present, in a tree that shares a root with the subtree that just got removed. A test that only + checks "the deleted branch is gone" cannot tell a correct cascade apart from one that + over-deletes into a sibling it should never have reached; this test can. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag, name="root") + branch_to_delete = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, name="branch_to_delete") + grandchild = CompetencyCriteriaGroup.objects.create(tag=tag, parent=branch_to_delete, name="grandchild") + surviving_sibling = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, name="surviving_sibling") + + branch_object_tag = ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+branch", taxonomy=competency_taxonomy, tag=tag + ) + grandchild_object_tag = ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+grandchild", taxonomy=competency_taxonomy, tag=tag + ) + sibling_object_tag = ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+sibling", taxonomy=competency_taxonomy, tag=tag + ) + + taxonomy_scoped_profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + branch_criterion = CompetencyCriterion.objects.create( + group=branch_to_delete, object_tag=branch_object_tag, rule_profile=default_rule_profile + ) + grandchild_criterion = CompetencyCriterion.objects.create( + group=grandchild, + object_tag=grandchild_object_tag, + rule_type_override=RuleType.GRADE, + rule_payload_override=_GRADE_PAYLOAD, + ) + sibling_criterion = CompetencyCriterion.objects.create( + group=surviving_sibling, object_tag=sibling_object_tag, rule_profile=taxonomy_scoped_profile + ) + + all_group_pks = {root.pk, branch_to_delete.pk, grandchild.pk, surviving_sibling.pk} + all_criterion_pks = {branch_criterion.pk, grandchild_criterion.pk, sibling_criterion.pk} + existing_group_pks = set(CompetencyCriteriaGroup.objects.filter(pk__in=all_group_pks).values_list("pk", flat=True)) + existing_criterion_pks = set( + CompetencyCriterion.objects.filter(pk__in=all_criterion_pks).values_list("pk", flat=True) + ) + assert existing_group_pks == all_group_pks + assert existing_criterion_pks == all_criterion_pks + + branch_to_delete.delete() + + remaining_group_pks = set( + CompetencyCriteriaGroup.objects.filter(pk__in=all_group_pks).values_list("pk", flat=True) + ) + remaining_criterion_pks = set( + CompetencyCriterion.objects.filter(pk__in=all_criterion_pks).values_list("pk", flat=True) + ) + + # Exactly the root and the surviving sibling remain; the deleted branch and its child are gone. + assert remaining_group_pks == {root.pk, surviving_sibling.pk} + # Exactly the sibling's criterion remains; both criteria under the deleted branch are gone, + # regardless of whether they were profile-assigned or override-governed. + assert remaining_criterion_pks == {sibling_criterion.pk} + + # The surviving sibling and its criterion are not merely "still present somewhere" but the + # exact same rows, untouched by the delete of an unrelated branch under the same root. + surviving_sibling.refresh_from_db() + sibling_criterion.refresh_from_db() + assert surviving_sibling.parent_id == root.pk + assert sibling_criterion.group_id == surviving_sibling.pk + assert sibling_criterion.rule_profile_id == taxonomy_scoped_profile.pk 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