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/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..c3682b504 100644 --- a/projects/dev.py +++ b/projects/dev.py @@ -37,6 +37,12 @@ # Open edX Organizations (dependency for openedx_catalog) "organizations", + # django-simple-history: registers its template tag libraries and admin integration + # (SimpleHistoryAdmin) and its management commands (populate_history, clean_old_history, + # clean_duplicate_history). HistoricalRecords() works without this app installed, but nothing + # else it provides does, and the package ships no AppConfig or system check to warn you. + "simple_history", + # Our Apps "openedx_catalog", "openedx_learning", diff --git a/requirements/base.in b/requirements/base.in index 626a551be..7292e10c3 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -17,3 +17,5 @@ rules<4.0 # Django extension for rules-based authorization check tomlkit # Parses and writes TOML configuration files edx-organizations # Implemented the "Organization" model that CatalogCourse/CourseRun are keyed to + +django-simple-history # History tracking for CBE criteria definitions, per ADR-0003 diff --git a/requirements/base.txt b/requirements/base.txt index c7dac57c3..c89949f77 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -58,7 +58,9 @@ django-crum==0.7.9 django-model-utils==5.0.0 # via edx-organizations django-simple-history==3.13.0 - # via edx-organizations + # via + # -r requirements/base.in + # edx-organizations django-waffle==5.0.0 # via # edx-django-utils diff --git a/src/openedx_learning/applets/cbe/models/__init__.py b/src/openedx_learning/applets/cbe/models/__init__.py new file mode 100644 index 000000000..997f57d57 --- /dev/null +++ b/src/openedx_learning/applets/cbe/models/__init__.py @@ -0,0 +1,15 @@ +""" +Models for Competency-Based Education (CBE). +""" + +from .competency_taxonomy import CompetencyTaxonomy +from .criteria import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, LogicOperator, RuleType + +__all__ = [ + "CompetencyTaxonomy", + "CompetencyCriteriaGroup", + "CompetencyCriterion", + "CompetencyRuleProfile", + "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..06b842e25 --- /dev/null +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -0,0 +1,446 @@ +""" +Models for CompetencyAchievementCriteria: CompetencyCriteriaGroup, CompetencyRuleProfile, CompetencyCriterion. + +See :ref:`openedx-learning-adr-0002` for the design this module implements, and +:ref:`openedx-learning-adr-0003` for why these three models (and not CompetencyTaxonomy) carry +``django-simple-history`` tracking. + +Seven of the nine foreign keys here are ``on_delete=models.CASCADE``: both ``CompetencyCriteriaGroup`` +tree links (``parent``, ``tag``), its ``course`` scope, both ``CompetencyCriterion`` links (``group``, +``object_tag``), and ``CompetencyRuleProfile``'s ``course`` and ``competency_taxonomy`` scope links. +The other two stay ``models.PROTECT``: ``CompetencyCriterion.rule_profile`` and +``CompetencyRuleProfile.organization``. + +CASCADE expresses containment: a row on the CASCADE side is meaningless once its referent is gone, +so its own delete has no separate policy to enforce. The tree links (``parent``, ``tag``, ``group``, +``object_tag``) also have to be CASCADE for a mechanical reason: Django's collector looks up +referencing rows in the database rather than in the set it has already decided to delete, so even a +parent and child reached in the same batch would trip PROTECT and abort the walk partway down. Those +same CASCADE edges are what carries the collector down to the PROTECT that actually enforces +ADR-0002 Decision 7 for learner data: #642's three ``Student*Status`` foreign keys, one and two +levels below the tag, are reached only by walking these edges, never relaxed by them. + +``CompetencyRuleProfile.course`` and ``.competency_taxonomy`` are CASCADE for an ADR-level reason, +not a mechanical one: Decision 7 (as amended) says a taxonomy or course is only ever hard-deleted +once nothing beneath it needs protecting, so a profile scoped to it is safe to remove at the same +time rather than blocking that delete. ``.organization`` stays PROTECT because an ``Organization`` +is not a competency-definition record covered by that reasoning, and ``edx-organizations`` +deactivates orgs rather than deleting them. + +``rule_profile`` staying PROTECT is Decision 7's actual backstop for a profile itself: a +CompetencyRuleProfile is never hard-deleted by a *direct* delete (retirement is archive-only), and +this is what makes that hold at the ORM layer, by blocking any attempt to delete one out from under +a criterion still assigned to it. + +One non-obvious consequence of the collector's database-not-pending-set lookup described above: +deleting a CompetencyTaxonomy whose taxonomy-scoped profile is itself assigned to a +CompetencyCriterion raises ProtectedError naming that criterion, even though the criterion would +also be cascade-deleted in the same operation through the tag chain. See +test_criteria_deletion.py's "residual tension" section for what this needs before it can be fixed +(a fifth ADR-0002 Decision 4 reassignment event), and why it cannot be reached with this phase's +data. +""" +from __future__ import annotations + +from 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 _RULE_PAYLOAD_SPECS, RuleType, validate_rule_payload +from .competency_taxonomy import CompetencyTaxonomy + +__all__ = [ + "CompetencyCriteriaGroup", + "CompetencyCriterion", + "CompetencyRuleProfile", + "LogicOperator", + "RuleType", + "validate_rule_payload", +] + +# The declared choices for both rule_type fields below, derived from the payload-spec registry +# (see rule_payloads.py) rather than hand-listed, so a rule type can never be offered as a choice +# without also having a payload spec that makes it actually saveable. +_RULE_TYPE_CHOICES = [(rule_type, RuleType(rule_type).label) for rule_type in _RULE_PAYLOAD_SPECS] + + +class LogicOperator(models.TextChoices): + """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. + ] + # ADR-0002 Decision 2 explicitly excludes two constraints here, both for the same reason: + # a child group cannot be saved until its parent's primary key exists, so at the moment a + # parent group is being validated/saved, its clean() always sees zero children, whether or + # not more are about to be attached. There's no single-row state at save time to check either + # of these against: + # - A constraint tying `logic_operator` to child count. + # - A UniqueConstraint on (parent, ordering), which would need to see all siblings, not just + # the row being saved. + + +class CompetencyRuleProfile(models.Model): + """ + A reusable default evaluation rule, optionally scoped to a taxonomy, course, or organization. + + Each row is scoped by at most one of ``organization``, ``course``, and ``competency_taxonomy``, + enforced by the check constraint below; the row with all three null is the system default, + seeded once by migration and never created or deleted through the profile API. See ADR-0002 + Decision 3 for how a :class:`CompetencyCriterion` is assigned one of these, and Decision 4 for + what happens when more than one scope's profile could apply to the same criterion. + + Editing a profile may change ``rule_type``/``rule_payload`` only: the scope fields + (``organization``, ``course``, ``competency_taxonomy``) are immutable after creation, so that + criteria already resolved to this profile's scope are never silently re-governed. ``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."), + ) + # A plain column, written explicitly in save() below, NOT a database GeneratedField: null + # while archived, and the "org:X,course:Y,taxonomy:Z" string (see save()) while live. This is + # what lets the UniqueConstraint below stay a plain, unconditional one on every backend this + # project supports, including MySQL, which does not support the conditional/partial unique + # indexes that a naive "unique unless archived" rule would otherwise need (see ADR-0002 + # Rejected Alternative 6): SQL never treats two NULLs as equal, so any number of archived rows + # may share a scope while exactly one live row holds it. A plain column also can't be rewritten + # by Django's collector, unlike a GeneratedField: deleting a scope owner (a CompetencyTaxonomy + # or CourseRun) whose foreign key here is nullable and CASCADE nulls that one column on this + # row before deleting it, on any backend that can't defer constraint checks (MySQL); a + # GeneratedField would recompute from that nulled value and could collide with another row + # already occupying the resulting blank scope, raising IntegrityError instead of completing + # the cascade. A plain column is untouched by that nulling, so this row keeps its true + # scope_code, unseen by anyone, until the row itself is deleted. + scope_code = models.CharField( + max_length=255, + null=True, + editable=False, + help_text=_( + "Derived from organization/course/competency_taxonomy; null while archived, otherwise " + "\"org:X,course:Y,taxonomy:Z\" with each segment blank when that scope column is null. " + "Recomputed in save(); never set directly." + ), + ) + rule_type = models.CharField(max_length=32, choices=_RULE_TYPE_CHOICES) + rule_payload = models.JSONField( + help_text=_("Structured payload keyed by rule_type; see validate_rule_payload for the shape it must match.") + ) + 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 = [ + # A plain, unconditional UniqueConstraint, deliberately: scope_code is a plain, + # always-non-null-while-live column (see its definition above), not a conditional + # index over the raw nullable scope columns. MySQL (this project's tested and + # production database) does not support conditional/partial unique indexes -- Django + # only raises a non-fatal system-check warning (models.W036) and silently skips + # creating such a constraint there, while SQLite (used for quick local test runs) + # does support them and would mask the gap in that environment. See ADR-0002 Rejected + # Alternative 6. + models.UniqueConstraint(fields=["scope_code"], name="oel_cbe_ruleprofile_scope_code_uniq"), + models.CheckConstraint( + # Expressed as "at least two of the three scope columns are null", i.e. at most one + # 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 + # Always queries the database directly, rather than comparing against a value cached at + # load time: that avoids a deferred/only() load, or a refresh_from_db() call, silently + # bypassing this check. Explicitly targets self._state.db, the alias this instance + # actually belongs to, so an instance loaded from a non-default database is not silently + # compared against the wrong one. Guarded against the row having since been deleted, in + # which case there's nothing left to compare against either. + persisted_scope = ( + CompetencyRuleProfile.objects.using(self._state.db) + .filter(pk=self.pk) + .values_list("organization_id", "course_id", "competency_taxonomy_id") + .first() + ) + if persisted_scope is None: + return + current_scope = (self.organization_id, self.course_id, self.competency_taxonomy_id) + if current_scope != 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 save(self, *args, **kwargs): + """Recompute scope_code, then persist this profile after full_clean() re-validates it.""" + self.scope_code = None if self.archived else ( + f"org:{'' if self.organization_id is None else self.organization_id}," + f"course:{'' if self.course_id is None else self.course_id}," + f"taxonomy:{'' if self.competency_taxonomy_id is None else self.competency_taxonomy_id}" + ) + # validate_unique and validate_constraints are left to the database: the unique and check + # constraints above enforce them identically and without the extra queries full_clean() + # would otherwise run to pre-check them in Python. Matches CourseRun.save() at + # src/openedx_catalog/models/course_run.py. Neither the non-editable scope_code nor the + # nullable override-style fields on this model cause full_clean() to reject an otherwise + # valid row: Django's own Field.validate() skips every check for a field with + # editable=False, and a blank=True field with an empty value is skipped by clean_fields() + # before validation runs at all. + self.full_clean(validate_unique=False, validate_constraints=False) + super().save(*args, **kwargs) + + +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=_RULE_TYPE_CHOICES, null=True, blank=True) + rule_payload_override = models.JSONField(null=True, blank=True) + + history = HistoricalRecords() + + class Meta: + # No db_table override, so the table is Django's default, + # openedx_learning_competencycriterion. ADR-0002 Decision 4's heading reads + # "CompetencyCriterion concept (CompetencyCriteria database table)", which names the + # domain concept the way every other heading in that ADR does rather than instructing a + # rename. No model anywhere in src/ overrides db_table, so every table in this library + # is _. + # + # Django's default pluralization of "CompetencyCriterion" is the ungrammatical + # "competency criterions"; set both explicitly, matching ADR-0002 Decision 4's + # terminology (one leaf is a criterion, the collection is CompetencyCriteria) and + # following CompetencyTaxonomy, which sets both for the same reason. + verbose_name = _("Competency Criterion") + verbose_name_plural = _("Competency Criteria") + constraints = [ + models.CheckConstraint( + condition=( + Q( + rule_profile__isnull=False, + rule_type_override__isnull=True, + rule_payload_override__isnull=True, + ) + | Q( + rule_profile__isnull=True, + rule_type_override__isnull=False, + rule_payload_override__isnull=False, + ) + ), + name="oel_cbe_criterion_profile_xor_override_check", + violation_error_message=_( + "A CompetencyCriterion must have either a rule_profile with no overrides, or both override " + "fields set with no rule_profile. Never both, never neither." + ), + ), + ] + + def clean(self): + """Validate the override rule_payload's shape, when a per-criterion override is set.""" + super().clean() + if self.rule_type_override is not None: + validate_rule_payload(self.rule_type_override, self.rule_payload_override) + + def save(self, *args, **kwargs): + """Persist this criterion, after full_clean() re-validates the override payload, if set.""" + # See CompetencyRuleProfile.save() above for why validate_unique/validate_constraints are + # skipped here too, and why the nullable override fields don't trip full_clean() when unset. + self.full_clean(validate_unique=False, validate_constraints=False) + super().save(*args, **kwargs) diff --git a/src/openedx_learning/applets/cbe/rule_payloads.py b/src/openedx_learning/applets/cbe/rule_payloads.py new file mode 100644 index 000000000..4c781f2ee --- /dev/null +++ b/src/openedx_learning/applets/cbe/rule_payloads.py @@ -0,0 +1,152 @@ +""" +Rule payload shapes for CBE evaluation rules, and the parser that validates a raw payload against +the shape its rule_type defines. + +See :ref:`openedx-learning-adr-0002` Decision 3 for the payload contract this enforces. Each +supported rule_type has exactly one spec class here (currently only :class:`GradeRule`) and one +matching entry in ``_RULE_PAYLOAD_SPECS``; :data:`RuleType` declares only the rule types that have +both, so a rule type can never be offered as a choice (see ``models/criteria.py``, which derives +both models' ``choices=`` from this same registry) without also being possible to save. Adding a +new rule type (for example ``MasteryLevel``) is one spec class plus one registry entry, and the +``RuleType`` member that goes with them. +""" +from __future__ import annotations + +from typing import Any + +from attrs import Attribute, define, field, fields +from django.core.exceptions import ValidationError +from django.db import models +from django.utils.translation import gettext_lazy as _ + +__all__ = [ + "GradeRule", + "RuleType", + "parse_rule_payload", + "validate_rule_payload", +] + + +class RuleType(models.TextChoices): + """ + The evaluation rule types a CompetencyRuleProfile or CompetencyCriterion override can use. + + Declares exactly the rule types with a defined rule_payload shape below, i.e. exactly the keys + of ``_RULE_PAYLOAD_SPECS``: see this module's own docstring for why the two are never allowed + to drift apart. + """ + + GRADE = "Grade", _("Grade") + + +def _validate_op(_instance: object, _attribute: Attribute, value: object) -> None: + """Reject an 'op' outside the Grade rule's allowed comparison operators.""" + if value not in {"gte", "lte", "eq"}: + raise ValueError(_("The 'op' in a 'Grade' rule_payload must be one of: gte, lte, eq.")) + + +def _validate_grade_value(_instance: object, _attribute: Attribute, value: object) -> None: + """Reject a Grade rule's 'value' unless it's a non-boolean number in [0.0, 1.0].""" + # isinstance(True, int) is True in Python, so a bool would otherwise pass the numeric check below. + if isinstance(value, bool) or not isinstance(value, (int, float)): + raise TypeError(_("The 'value' in a 'Grade' rule_payload must be a number, not a boolean.")) + if not 0.0 <= value <= 1.0: + raise ValueError( + _( + "The 'value' in a 'Grade' rule_payload must be a fraction between 0.0 and 1.0 inclusive " + "(e.g. 0.8 for a passing grade of 80%%), not %(value)r." + ) + % {"value": value} + ) + + +def _validate_scale(_instance: object, _attribute: Attribute, value: object) -> None: + """Reject a Grade rule's 'scale' unless it's exactly 'percent'.""" + if value != "percent": + raise ValueError(_("The 'scale' in a 'Grade' rule_payload must be 'percent'.")) + + +@define(frozen=True, kw_only=True) +class GradeRule: + """ + The rule_payload shape for RuleType.GRADE, per ADR-0002 Decision 3. + + Constructing one *is* the validation: kw_only means an unknown key raises TypeError + ("unexpected keyword argument") and a missing key raises TypeError ("missing ... required + keyword-only argument") from Python's own call handling, and a present-but-bad value raises + ValueError or TypeError from the field validators below. Frozen because this describes a + fixed spec and is never mutated after construction. + """ + + op: str = field(validator=_validate_op) + value: float = field(validator=_validate_grade_value) + scale: str = field(validator=_validate_scale) + + +# One spec class per RuleType with a defined rule_payload shape. Add a class and an entry here (and +# the matching RuleType member above) to support a new rule type. +_RULE_PAYLOAD_SPECS: dict[str, type] = { + RuleType.GRADE: GradeRule, +} + + +def parse_rule_payload(rule_type: str, payload: Any) -> Any: + """ + Validate ``payload`` against the shape ADR-0002 Decision 3 defines for ``rule_type``, and + return the constructed, frozen spec object (for example a :class:`GradeRule`) on success. + + Raises ``django.core.exceptions.ValidationError`` on any mismatch, including a ``rule_type`` + with no defined payload shape at all. Prefer this over :func:`validate_rule_payload` at a call + site that wants the parsed, typed fields (for example read-time rule evaluation), not just the + pass/fail check. + """ + spec_class = _RULE_PAYLOAD_SPECS.get(rule_type) + if spec_class is None: + raise ValidationError( + _("Rule type '%(rule_type)s' is not supported yet; only 'Grade' has a defined rule_payload shape.") + % {"rule_type": rule_type} + ) + # Checked separately, before construction: `spec_class(**payload)` on a non-dict payload + # (e.g. a list) raises "argument after ** must be a mapping", which is a confusing message to + # surface to an author. + if not isinstance(payload, dict): + raise ValidationError(_("A '%(rule_type)s' rule_payload must be a JSON object.") % {"rule_type": rule_type}) + + # Also checked separately, before construction, rather than left to Python's own kw_only + # TypeError: that TypeError's text is "GradeRule.__init__() missing/got an unexpected keyword + # argument ...", a Python traceback fragment that leaks an internal class name to whoever + # edits this payload (a course author, via the admin). Deriving the expected keys from the + # spec class itself keeps that class the single source of truth for the key set, while owning + # the message in our own domain language instead of Python's. + expected_keys = {attr.name for attr in fields(spec_class)} + missing_keys = sorted(expected_keys - payload.keys()) + unexpected_keys = sorted(payload.keys() - expected_keys) + if missing_keys or unexpected_keys: + problems = [] + if missing_keys: + problems.append(_("missing %(keys)s") % {"keys": ", ".join(missing_keys)}) + if unexpected_keys: + problems.append(_("unexpected %(keys)s") % {"keys": ", ".join(unexpected_keys)}) + raise ValidationError( + _("A '%(rule_type)s' rule_payload has the wrong keys: %(problems)s.") + % {"rule_type": rule_type, "problems": "; ".join(str(problem) for problem in problems)} + ) + + # Past the key check above, this can only fail on a value a field validator rejects. + try: + return spec_class(**payload) + except (TypeError, ValueError) as exc: + # Surface the underlying message (our custom validators' text) rather than replacing it + # with something generic: that message is what full_clean() or save() surfaces to an + # admin or API caller. + raise ValidationError(str(exc)) from exc + + +def validate_rule_payload(rule_type: str, payload: Any) -> None: + """ + Validate ``payload`` against the shape ADR-0002 Decision 3 defines for ``rule_type``. + + A thin wrapper around :func:`parse_rule_payload`, for a call site (``clean()``/``save()`` on + both CBE models) that only needs the pass/fail check and has no use for the parsed object. + """ + parse_rule_payload(rule_type, payload) diff --git a/src/openedx_learning/migrations/0002_competency_criteria.py b/src/openedx_learning/migrations/0002_competency_criteria.py new file mode 100644 index 000000000..1681b5d81 --- /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. Recomputed in save(); never set directly.', max_length=255, null=True)), + ('rule_type', models.CharField(choices=[('Grade', 'Grade')], max_length=32)), + ('rule_payload', models.JSONField(help_text='Structured payload keyed by rule_type; see validate_rule_payload for the shape it must match.')), + ('archived', models.BooleanField(default=False, help_text="Set instead of deleting a profile that's no longer wanted. Archived profiles are hidden from authoring and new associations but remain queryable, so existing criteria stay resolvable.")), + ('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..9a1f4ab66 --- /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..a0b4a2661 100644 --- a/test_settings.py +++ b/test_settings.py @@ -54,6 +54,11 @@ def root(*args): "organizations", # django-rules based authorization 'rules.apps.AutodiscoverRulesConfig', + # django-simple-history: registers its template tag libraries and admin integration + # (SimpleHistoryAdmin) and its management commands (populate_history, clean_old_history, + # clean_duplicate_history). HistoricalRecords() works without this app installed, but nothing + # else it provides does, and the package ships no AppConfig or system check to warn you. + "simple_history", # Our own apps "openedx_tagging", "openedx_content", diff --git a/tests/openedx_learning/applets/cbe/conftest.py b/tests/openedx_learning/applets/cbe/conftest.py new file mode 100644 index 000000000..27953f7d0 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/conftest.py @@ -0,0 +1,73 @@ +""" +Shared fixtures for the CBE criteria test modules (schema, deletion, and tree-integration tests). + +Every fixture here used to be duplicated verbatim across test_criteria_models.py and +test_criteria_deletion.py. Consolidated here so both files, plus test_criteria_trees.py, share one +definition. +""" +import pytest +from organizations.api import ensure_organization +from organizations.models import Organization + +from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_learning.models import CompetencyCriteriaGroup, CompetencyRuleProfile, CompetencyTaxonomy +from openedx_tagging.models import ObjectTag, Tag + + +@pytest.fixture(name="organization") +def _organization() -> Organization: + """An Organization for use as a scope in these tests.""" + ensure_organization("Org1") + return Organization.objects.get(short_name="Org1") + + +@pytest.fixture(name="organization2") +def _organization2() -> Organization: + """A second Organization, distinct from `organization`, for use as a scope in these tests.""" + ensure_organization("Org2") + return Organization.objects.get(short_name="Org2") + + +@pytest.fixture(name="course_run") +def _course_run(organization: Organization) -> CourseRun: + """A CourseRun for use as a scope in these tests.""" + catalog_course = CatalogCourse.objects.create(org=organization, course_code="Python100") + return CourseRun.objects.create(catalog_course=catalog_course, run_code="Fall2026") + + +@pytest.fixture(name="competency_taxonomy") +def _competency_taxonomy() -> CompetencyTaxonomy: + """A CompetencyTaxonomy for use as a scope, and as the home taxonomy for `tag`.""" + return CompetencyTaxonomy.objects.create(name="Nursing", export_id="nursing-v1") + + +@pytest.fixture(name="tag") +def _tag(competency_taxonomy: CompetencyTaxonomy) -> Tag: + """A Tag, from `competency_taxonomy`, for use as the competency a criteria tree evaluates.""" + return Tag.objects.create(taxonomy=competency_taxonomy, value="Writing Poetry") + + +@pytest.fixture(name="object_tag") +def _object_tag(competency_taxonomy: CompetencyTaxonomy, tag: Tag) -> ObjectTag: + """An ObjectTag associating `tag` with a made-up content object, for use as a criterion's target.""" + return ObjectTag.objects.create( + object_id="block-v1:Org1+Python100+Fall2026+problem+p1", + taxonomy=competency_taxonomy, + tag=tag, + ) + + +@pytest.fixture(name="group") +def _group(tag: Tag) -> CompetencyCriteriaGroup: + """A root CompetencyCriteriaGroup for `tag`, for use as a criterion's parent group.""" + return CompetencyCriteriaGroup.objects.create(tag=tag) + + +@pytest.fixture(name="default_rule_profile") +def _default_rule_profile() -> CompetencyRuleProfile: + """The system-default CompetencyRuleProfile seeded by migration 0003.""" + return CompetencyRuleProfile.objects.get( + organization__isnull=True, + course__isnull=True, + competency_taxonomy__isnull=True, + ) diff --git a/tests/openedx_learning/applets/cbe/test_criteria_deletion.py b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py new file mode 100644 index 000000000..6762beaa9 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_deletion.py @@ -0,0 +1,501 @@ +""" +Delete-behavior tests for CompetencyCriteriaGroup, CompetencyRuleProfile, and CompetencyCriterion. + +Two of the nine foreign keys here assert a different on_delete value than issue #641 itself +specifies, because on_delete expresses containment (is this row meaningless once its referent is +gone?), never a protection policy: + +- CompetencyCriteriaGroup.course must be CASCADE, not PROTECT: a course-scoped criteria tree is + meaningless once its course run is gone. PROTECT would make a course run permanently + undeletable the moment any competency criteria exist for it, even with zero learner data, which + is a different (and stricter) guarantee than anything #641 actually needs. +- CompetencyRuleProfile.course must be CASCADE, not PROTECT, for the same reason, and because a + taxonomy or course is only ever hard-deleted once nothing beneath it needs protecting (see + ADR-0002 Decision 7's amended text): a course-scoped profile is safe to remove along with its + course rather than blocking the delete. + +CompetencyRuleProfile.competency_taxonomy is also CASCADE, matching #641's own AC25. A +CompetencyRuleProfile's own "never hard-deleted" rule (Decision 7) governs a *direct* delete of a +profile; it does not stop a profile from being cascaded away as a side effect of deleting the +taxonomy or course it is scoped to, once nothing else protects it. When something else does +protect it -- a CompetencyCriterion still assigned to it via the PROTECT'd `rule_profile` foreign +key -- deleting the taxonomy or course still raises ProtectedError, exactly as it would for any +other row a PROTECT relationship blocks; see the "residual tension" section below for the one +case where that ProtectedError names the wrong object. + +Fixtures shared with test_criteria_models.py and test_criteria_trees.py live in this directory's +conftest.py. +""" +import pytest +from django.apps import apps +from django.db import connection +from django.db.models import ProtectedError +from organizations.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 assert ProtectedError and inspect `protected_objects` +# to confirm which relationship actually fired: several protected relationships can fire on one +# delete (see test_deleting_an_organization_with_a_scoped_profile_raises_protected_error_naming_ +# the_profile below for a real trap of that kind, where CatalogCourse.org is also PROTECT), so a +# bare `pytest.raises(ProtectedError)` would not actually prove which foreign key did the +# protecting. The CASCADE ones assert the delete succeeds and that the referencing row is actually +# gone from the database afterward, not merely that no exception was raised, and assert the +# referencing row existed beforehand, so the "gone" assertion can't pass because a fixture never +# created it in the first place. +# ============================================================================================== + + +def test_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, matching #641's + AC25. A CompetencyRuleProfile is never hard-deleted by a *direct* delete of the profile itself + (ADR-0002 Decision 7); that does not stop it being cascaded away as a side effect of deleting + the taxonomy it is scoped to, once nothing else protects it. Nothing changes behaviorally in + this MVP, since only the all-null system-default profile exists otherwise, so this scenario + cannot arise until a taxonomy-scoped profile is actually created, which no authoring screen + does yet. See the "residual tension" section below for what happens instead when a + CompetencyCriterion is still assigned to the scoped profile being cascaded away. + """ + profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + 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 DECISION-on-delete.md's REVISION section): with `competency_taxonomy` +# CASCADE and `rule_profile` PROTECT, deleting a taxonomy whose scoped profile is itself assigned +# to a criterion raises ProtectedError, because Django's collector looks up rows that reference the +# profile in the database rather than in the set it has already decided to delete -- it fires even +# though that same criterion would also be cascade-deleted in this same operation, via the separate +# Tag -> CompetencyCriteriaGroup.tag -> CompetencyCriterion.group chain. Confirmed, accepted defect +# for this MVP: no code path creates a taxonomy-scoped profile at all, so it cannot be reached with +# real data. The fix, when scoped profiles are built, is a fifth reassignment event on ADR-0002 +# Decision 4 (which currently names four): "the profile's scope owner is being deleted" reassigns +# every criterion off that profile in an application-layer function, before the cascade proceeds. +# Not built in this change; #641 scopes it out. +# ============================================================================================== + + +def test_taxonomy_delete_with_a_criterion_assigned_its_scoped_profile_raises_protected_error_naming_the_criterion( + competency_taxonomy: CompetencyTaxonomy, group: CompetencyCriteriaGroup, object_tag: ObjectTag +) -> None: + """ + Deleting a CompetencyTaxonomy whose taxonomy-scoped profile is itself assigned to a criterion + raises ProtectedError naming the CRITERION, not the profile actually being cascaded away. + + The taxonomy delete cascades into the profile (`competency_taxonomy` is CASCADE), and only then + discovers the profile is referenced by the criterion via `rule_profile` (PROTECT). Django's + PROTECT handler raises unconditionally whenever a referencing row exists in the database; it + never checks whether that same row is also already part of the same delete's pending set, so it + fires here even though this exact criterion would also be reached and removed via the tag chain + (Tag -> CompetencyCriteriaGroup.tag -> CompetencyCriterion.group, all CASCADE) if the profile + hadn't blocked the walk first. This is a spurious, confusing failure -- an author deleting a + taxonomy is told a criterion is in the way, when nothing about that criterion actually survives + the delete either -- but it is not reachable in this MVP (see the section header above), so + this pins the current behavior rather than working around it with a schema change. + """ + profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + criterion = CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=profile) + + with pytest.raises(ProtectedError) as exc_info: + competency_taxonomy.delete() + + protected = exc_info.value.protected_objects + assert any(isinstance(obj, CompetencyCriterion) and obj.pk == criterion.pk for obj in protected) + assert not any(isinstance(obj, CompetencyRuleProfile) and obj.pk == profile.pk for obj in protected) + # Nothing was actually removed: the whole operation raised before any DELETE executed. + assert CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + + +# ============================================================================================== +# MySQL collector semantics, reproduced on SQLite by monkeypatching can_defer_constraint_checks. +# +# MySQL cannot defer foreign-key constraint checks (can_defer_constraint_checks is False there). +# django.db.models.deletion.CASCADE reads that flag directly: whenever a cascading foreign key is +# nullable and constraints can't be deferred, it nulls that column on every row about to be +# cascade-deleted (via collector.add_field_update) BEFORE the actual DELETE, to avoid a transient +# FK violation under non-deferred constraint checking. On ordinary SQLite semantics (deferred +# constraints allowed), this nulling never happens at all, so a defect that only shows up via this +# path is invisible on the fast local suite and only ever caught by the separate MySQL CI job +# (AC8). Monkeypatching the flag reproduces it here instead. Do not "simplify" these tests by +# dropping the monkeypatch: without it, neither scenario below reproduces anything, on either the +# broken or the fixed code. +# +# This used to be where the shipped bug lived: when scope_code was a database GeneratedField, this +# same pre-delete nulling of a profile's scope foreign key recomputed scope_code, colliding it with +# whatever other row already held that now-blank scope (the seeded system-default row, or a second +# profile nulled in the same batch) and raising IntegrityError instead of completing the cascade. +# Making scope_code a plain column written only in save() (see models/criteria.py) fixes this: the +# collector's nulling touches only the real scope foreign key column, never scope_code, so a +# profile being cascade-deleted keeps its true scope_code, unseen by anyone, until the row is gone. +# ============================================================================================== + + +def test_taxonomy_delete_cascades_its_scoped_profile_under_mysql_collector_semantics( + monkeypatch: pytest.MonkeyPatch, competency_taxonomy: CompetencyTaxonomy +) -> None: + """ + Deleting a CompetencyTaxonomy with a taxonomy-scoped profile succeeds and cascades the profile + away even under MySQL's non-deferred constraint semantics, the same as it does under ordinary + SQLite semantics (see test_deleting_a_taxonomy_with_a_scoped_rule_profile_also_deletes_the_ + profile above). Confirms the fix described in this section's header actually holds under the + collector path that used to trigger the bug: nulling the profile's `competency_taxonomy_id` + before deleting it does not touch `scope_code`, which would otherwise collide with the seeded + system-default profile's identical blank scope and raise IntegrityError instead of completing. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + profile = CompetencyRuleProfile.objects.create( + competency_taxonomy=competency_taxonomy, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + competency_taxonomy.delete() + + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + +def test_course_run_delete_cascades_its_course_scoped_criteria_group_under_mysql_collector_semantics( + monkeypatch: pytest.MonkeyPatch, tag: Tag, course_run: CourseRun +) -> None: + """ + Deleting a CourseRun with a course-scoped CompetencyCriteriaGroup succeeds and cascades the + group away even under MySQL's non-deferred constraint semantics. `course` is one of the two + nullable foreign keys this change turns from PROTECT to CASCADE, so it shares the exact + pre-delete-nulling collector path the taxonomy case above does; unlike scope_code, + CompetencyCriteriaGroup carries no uniqueness constraint a null `course_id` could collide with, + so this path is expected to just succeed. Pinned here anyway, alongside the taxonomy case, + since a future fix to one foreign key without the other would otherwise go unnoticed. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run) + + course_run.delete() + + assert not CompetencyCriteriaGroup.objects.filter(pk=group.pk).exists() + + +def test_course_run_delete_cascades_its_scoped_rule_profile_under_mysql_collector_semantics( + monkeypatch: pytest.MonkeyPatch, course_run: CourseRun +) -> None: + """ + Deleting a CourseRun with a course-scoped CompetencyRuleProfile succeeds and cascades the + profile away even under MySQL's non-deferred constraint semantics, the same as the taxonomy + case above: `course` is CompetencyRuleProfile's other newly-CASCADE foreign key, and shares the + same pre-delete-nulling collector path and the same scope_code collision this fix removes. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + profile = CompetencyRuleProfile.objects.create( + course=course_run, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + course_run.delete() + + assert not CompetencyRuleProfile.objects.filter(pk=profile.pk).exists() + + +def test_deleting_two_taxonomies_together_cascades_both_their_scoped_profiles_away( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """ + Deleting two CompetencyTaxonomy rows in one `.delete()` call, each with its own taxonomy-scoped + profile, succeeds and cascades both profiles away -- neither profile's scope_code collides with + the other's, even though both get their `competency_taxonomy_id` nulled in the same collector + batch under MySQL's non-deferred constraint semantics. + + Same path as the single-taxonomy MySQL case above, but confirms it does not get worse when two + scope owners are collected in the same collector pass: before scope_code became a plain column, + nulling both profiles' `competency_taxonomy_id` in the same batch drove both scope_code values + to the identical blank "org:,course:,taxonomy:" string and raised IntegrityError on whichever + row the database processed second. + """ + monkeypatch.setattr(type(connection.features), "can_defer_constraint_checks", False, raising=False) + taxonomy1 = CompetencyTaxonomy.objects.create(name="Nursing Two Taxonomy Delete", export_id="nursing-two-del") + taxonomy2 = CompetencyTaxonomy.objects.create(name="Welding Two Taxonomy Delete", export_id="welding-two-del") + profile1 = CompetencyRuleProfile.objects.create( + competency_taxonomy=taxonomy1, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile2 = CompetencyRuleProfile.objects.create( + competency_taxonomy=taxonomy2, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + CompetencyTaxonomy.objects.filter(pk__in=[taxonomy1.pk, taxonomy2.pk]).delete() + + assert not CompetencyRuleProfile.objects.filter(pk__in=[profile1.pk, profile2.pk]).exists() + + +# ============================================================================================== +# Transitive deletion tests required by #641's Deletions criteria: deleting an oel_tagging.Tag, +# a CompetencyCriteriaGroup at depth, an oel_tagging.ObjectTag, or an oel_tagging.Taxonomy, when +# no learner status exists beneath the target, must succeed and take the whole referencing +# criteria tree with it. +# +# Each of these criteria also has a "raises ProtectedError when a learner status row exists +# beneath it" half. That half is NOT covered here: it needs #642's Student*Status tables, which +# do not exist on this branch, and #642's own criterion says those tests belong in the slice that +# follows #641, once those tables exist. This file does not stub, mock, or fake a status model to +# test them; their absence here is deliberate, not an oversight. +# +# See test_criteria_trees.py for the fuller integrative version of this shape: a wider tree with a +# surviving sibling branch and a mix of profile-assigned and override criteria, asserting exactly +# which rows survive rather than only that a cascade fired. +# ============================================================================================== + + +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..09c1aab2b --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criteria_models.py @@ -0,0 +1,845 @@ +""" +Tests for CompetencyCriteriaGroup, CompetencyRuleProfile, and CompetencyCriterion. + +Fixtures shared with test_criteria_deletion.py and test_criteria_trees.py live in this directory's +conftest.py. +""" +import uuid as uuid_module + +import pytest +from django.apps import apps +from django.core.exceptions import ValidationError +from django.db import connection, models, transaction +from django.db.utils import IntegrityError +from organizations.models import Organization + +from openedx_catalog.models import CatalogCourse, CourseRun +# _RULE_PAYLOAD_SPECS is private: it's the payload-spec registry itself, the one place that +# defines which rule types can actually be saved, which is exactly what +# test_rule_type_choices_match_rule_types_with_a_defined_payload_spec below needs to compare +# RuleType's declared choices against. +from openedx_learning.applets.cbe.models.criteria import _RULE_PAYLOAD_SPECS +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + 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 (AC1, AC2, AC6, AC12, AC17, AC22, AC33, AC34, AC23). CompetencyTaxonomy's own +# taxonomy_overrides_org default (AC1) is covered in test_models.py, not duplicated here. +# ============================================================================================== + + +def test_group_columns_match_adr_decision_2(course_run: CourseRun, tag: Tag) -> None: + """ + CompetencyCriteriaGroup has exactly the columns ADR-0002 Decision 2 lists: a nullable self-FK + `parent`, a required `tag` (db_column oel_tagging_tag_id), a nullable `course` targeting + openedx_catalog.CourseRun, `name`, `ordering`, and `logic_operator`, plus `id`. + """ + group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run) + + parent_field = CompetencyCriteriaGroup._meta.get_field("parent") + assert parent_field.null is True + assert parent_field.remote_field.model is CompetencyCriteriaGroup + + tag_field = CompetencyCriteriaGroup._meta.get_field("tag") + assert tag_field.null is False + assert tag_field.remote_field.model is Tag + assert tag_field.db_column == "oel_tagging_tag_id" + + course_field = CompetencyCriteriaGroup._meta.get_field("course") + assert course_field.null is True + assert course_field.remote_field.model is CourseRun + + assert CompetencyCriteriaGroup._meta.get_field("name").null is False + assert CompetencyCriteriaGroup._meta.get_field("ordering").null is False + assert CompetencyCriteriaGroup._meta.get_field("logic_operator").null is True + + assert group.course_id == course_run.pk + + +def test_rule_profile_columns_match_adr_decision_3() -> None: + """ + CompetencyRuleProfile has exactly the columns ADR-0002 Decision 3 lists: nullable + `organization`, `course`, and `competency_taxonomy` scope fields, `scope_code`, `rule_type`, + `rule_payload`, and `archived` (defaulting to False), plus `id`. + + `scope_code` is nullable, not "never null" as an earlier reading of AC7 (issue #641) required: + see DECISION-on-delete.md deviation 3. It is null exactly while a profile is archived (see + test_scope_code_is_null_once_archived_and_non_null_while_live below); this is what lets an + archived profile stop occupying its scope's unique slot. + """ + organization_field = CompetencyRuleProfile._meta.get_field("organization") + assert organization_field.null is True + assert organization_field.remote_field.model is Organization + + course_field = CompetencyRuleProfile._meta.get_field("course") + assert course_field.null is True + assert course_field.remote_field.model is CourseRun + + taxonomy_field = CompetencyRuleProfile._meta.get_field("competency_taxonomy") + assert taxonomy_field.null is True + assert taxonomy_field.remote_field.model is CompetencyTaxonomy + + assert CompetencyRuleProfile._meta.get_field("scope_code").null is True + assert CompetencyRuleProfile._meta.get_field("rule_type").null is False + assert CompetencyRuleProfile._meta.get_field("rule_payload").null is False + assert CompetencyRuleProfile._meta.get_field("archived").default is False + + +def test_criterion_columns_match_adr_decision_4( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> None: + """ + CompetencyCriterion has exactly the columns ADR-0002 Decision 4 lists: required `group` and + `object_tag`, a nullable `rule_profile`, and nullable `rule_type_override` / + `rule_payload_override`, plus `id`. The model is named CompetencyCriterion (singular; the + table holds many, individually a criterion), and carries no Meta.db_table override, so the + table is Django's default name for that class. + """ + assert CompetencyCriterion.__name__ == "CompetencyCriterion" + assert CompetencyCriterion._meta.db_table == "openedx_learning_competencycriterion" + + group_field = CompetencyCriterion._meta.get_field("group") + assert group_field.null is False + assert group_field.db_column == "competency_criteria_group_id" + + object_tag_field = CompetencyCriterion._meta.get_field("object_tag") + assert object_tag_field.null is False + assert object_tag_field.db_column == "oel_tagging_objecttag_id" + + rule_profile_field = CompetencyCriterion._meta.get_field("rule_profile") + assert rule_profile_field.null is True + assert rule_profile_field.db_column == "competency_rule_profile_id" + + assert CompetencyCriterion._meta.get_field("rule_type_override").null is True + assert CompetencyCriterion._meta.get_field("rule_payload_override").null is True + + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile + ) + assert criterion.rule_profile_id == default_rule_profile.pk + + +@pytest.mark.parametrize( + "model", + [CompetencyCriteriaGroup, CompetencyRuleProfile, CompetencyCriterion], + ids=["group", "rule_profile", "criterion"], +) +def test_uuid_is_a_stable_unique_non_editable_external_identifier(model: type[models.Model]) -> None: + """ + All three models carry a `uuid` external identifier: unique, not editable (so it can never be + set through a form), and defaulting to a freshly generated uuid4 for every new row. + """ + uuid_field = model._meta.get_field("uuid") + assert isinstance(uuid_field, models.UUIDField) + assert uuid_field.unique is True + assert uuid_field.editable is False + assert uuid_field.null is False + assert uuid_field.default is uuid_module.uuid4 + + +def test_group_has_no_columns_beyond_adr_decision_2() -> None: + """ + CompetencyCriteriaGroup's concrete field set is exactly {id, uuid, parent, tag, course, name, + ordering, logic_operator}: no more, no less. In particular, no `archived` column exists on + this model (that responsibility belongs to a later change; see the module's own history of + which ticket owns which model's archive column). + """ + concrete_field_names = {f.name for f in CompetencyCriteriaGroup._meta.get_fields() if f.concrete} + assert concrete_field_names == {"id", "uuid", "parent", "tag", "course", "name", "ordering", "logic_operator"} + + +def test_rule_profile_has_no_columns_beyond_adr_decision_3() -> None: + """ + CompetencyRuleProfile's concrete field set is exactly {id, organization, course, + competency_taxonomy, scope_code, rule_type, rule_payload, archived, uuid}: no more, no less. + """ + concrete_field_names = {f.name for f in CompetencyRuleProfile._meta.get_fields() if f.concrete} + assert concrete_field_names == { + "id", "organization", "course", "competency_taxonomy", "scope_code", "rule_type", "rule_payload", + "archived", "uuid", + } + + +def test_criterion_has_no_columns_beyond_adr_decision_4() -> None: + """ + CompetencyCriterion's concrete field set is exactly {id, uuid, group, object_tag, rule_profile, + rule_type_override, rule_payload_override}: no more, no less. In particular, no `archived` + column exists on this model. + """ + concrete_field_names = {f.name for f in CompetencyCriterion._meta.get_fields() if f.concrete} + assert concrete_field_names == { + "id", "uuid", "group", "object_tag", "rule_profile", "rule_type_override", "rule_payload_override", + } + + +# ============================================================================================== +# Constraints and validation (AC4, AC5, AC7, AC9, AC11, AC13, AC14, AC15). +# ============================================================================================== + + +def test_group_logic_operator_accepts_and_or_and_null_regardless_of_child_count(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. + """ + for logic_operator in (LogicOperator.AND, LogicOperator.OR, None): + childless = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=logic_operator) + assert childless.pk is not None + + parent = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=logic_operator) + CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent) + CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent) + assert CompetencyCriteriaGroup.objects.filter(parent=parent).count() == 2 + + +def test_group_parent_and_child_relationship(tag: Tag) -> None: + """ + A CompetencyCriteriaGroup's parent is null for a root and points at its parent for a child. + See ADR-0002 Decision 2. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=None) + assert root.parent is None + + child = 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. + Before this, archiving a profile meant that scope could never be used again, since the archived + row's scope_code stayed non-null and permanently held the unique slot. + """ + original = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + original.archived = True + original.save() + + replacement = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + replacement.refresh_from_db() + original.refresh_from_db() + + assert original.scope_code is None + assert replacement.scope_code == f"org:{organization.pk},course:,taxonomy:" + + +def test_scope_code_unique_constraint_is_unconditional() -> None: + """ + No UniqueConstraint on CompetencyRuleProfile carries a `condition`. A conditional + UniqueConstraint compiles to a partial index, which this project's MySQL backend does not + support: Django would only raise a non-fatal system-check warning (models.W036) and silently + skip creating the constraint, leaving uniqueness unenforced in production, while SQLite (used + for local test runs) supports partial indexes and would mask the gap. See ADR-0002 Rejected + Alternative 6. + """ + unique_constraints = [c for c in CompetencyRuleProfile._meta.constraints if isinstance(c, models.UniqueConstraint)] + assert unique_constraints + for constraint in unique_constraints: + assert constraint.condition is None + + +def test_two_live_profiles_cannot_share_the_same_scope(organization: Organization) -> None: + """ + Two live CompetencyRuleProfile rows cannot share the same scope. In particular, two rows that + both set only `organization` (leaving course and competency_taxonomy null) collide, which is + exactly the case a plain UniqueConstraint on the three raw nullable columns would not catch, + since SQL never treats two NULLs as equal. See ADR-0002 Decision 3. + """ + CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + with pytest.raises(IntegrityError): + with transaction.atomic(): + CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + + +@pytest.mark.parametrize( + "invalid_kwargs", + [ + pytest.param( + {"rule_type_override": RuleType.GRADE, "rule_payload_override": _GRADE_PAYLOAD, "use_profile": True}, + id="both_set", + ), + pytest.param({"use_profile": False}, id="neither_set"), + pytest.param({"rule_payload_override": _GRADE_PAYLOAD, "use_profile": False}, id="only_payload_override_set"), + ], +) +def test_criterion_profile_xor_override_check_constraint_rejects_invalid_states( + invalid_kwargs: dict, + group: CompetencyCriteriaGroup, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A CompetencyCriterion must have either a rule_profile with no overrides, or both override + fields set with no rule_profile, never both and never neither. See ADR-0002 Decision 4. + + Covers the three invalid states that reach the database's check constraint: both set, neither + set, and only rule_payload_override set. The fourth invalid state, only rule_type_override set, + is caught earlier by save()'s own validation instead and raises ValidationError before the + database is ever touched; see test_criterion_save_validates_override_payload_before_constraint + 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_criterion_save_validates_override_payload_before_constraint( + group: CompetencyCriteriaGroup, object_tag: ObjectTag +) -> None: + """ + Setting only rule_type_override, leaving rule_payload_override null, is caught by save()'s + own validation before it ever reaches the database: save() validates rule_payload_override's + shape whenever rule_type_override is set, and None is not a valid shape for any rule type, so + this raises ValidationError. The database's check constraint would also reject this same row, + for the same underlying reason (an override with no real payload), but save() never lets it + get there. This is why two similar-looking invalid override states raise different exception + types: this one is caught by save()'s validate_rule_payload call, while the other three (see + test_criterion_profile_xor_override_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, not attrs' generic default message for a failed validator (which + would say nothing about fractions or percentages) and not a Python traceback fragment. + Guards against exactly the message-quality regression a naive attrs implementation of + validate_rule_payload could introduce silently, since every other invalid-payload test here + only asserts the exception type. + """ + profile = CompetencyRuleProfile( + organization=organization, rule_type=RuleType.GRADE, rule_payload={"op": "gte", "value": 80, "scale": "percent"} + ) + with pytest.raises(ValidationError) as exc_info: + profile.full_clean() + + message = " ".join(exc_info.value.messages) + assert "fraction between 0.0 and 1.0" in message + # Must not leak the attrs spec class's name or any Python call-mechanics fragment: a course + # author editing this payload in the admin should never see "GradeRule.__init__()". + assert "__init__" not in message + assert "GradeRule" not in message + + +def test_rule_profile_full_clean_extra_key_message_names_the_key(organization: Organization) -> None: + """ + full_clean()'s error for an unrecognized rule_payload key names that key in our own domain + language (e.g. "unexpected extra"), not attrs' generic default message for a failed validator + and not Python's own kw_only TypeError text ("GradeRule.__init__() got an unexpected keyword + argument 'extra'"), which leaks the internal spec class's name to a course author editing + this payload in the admin. Guards against exactly that regression, which a test asserting + only that the key name appears in the message would not catch, since the leaky Python message + also contains the key name. + """ + profile = CompetencyRuleProfile( + organization=organization, + rule_type=RuleType.GRADE, + rule_payload={**_GRADE_PAYLOAD, "extra": 1}, + ) + with pytest.raises(ValidationError) as exc_info: + profile.full_clean() + + message = " ".join(exc_info.value.messages) + assert "extra" in message + assert "__init__" not in message + assert "GradeRule" not in message + + +@pytest.mark.parametrize("rule_type, payload", _INVALID_GRADE_PAYLOADS) +def test_criterion_full_clean_rejects_invalid_override_payload( + rule_type: str, payload: object, group: CompetencyCriteriaGroup, object_tag: ObjectTag +) -> 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 (AC11). Each scope field gets its own rejection test; rule_type, rule_payload, +# and archived changing on the same row is asserted separately as the case that must still work. +# ============================================================================================== + + +def test_scope_immutability_rejects_organization_change( + organization: Organization, organization2: Organization +) -> None: + """ + Changing a CompetencyRuleProfile's `organization` after creation raises ValidationError on + save(). See ADR-0002 Decision 3. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + profile.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: setting it to None would + make scope_code collide with the seeded system-default row, so the unique constraint would + raise IntegrityError instead of the scope guard, and the test would pass for the wrong + reason. Do not "simplify" this back to None. + """ + profile = CompetencyRuleProfile.objects.create( + organization=organization, rule_type=RuleType.GRADE, rule_payload=_GRADE_PAYLOAD + ) + deferred = CompetencyRuleProfile.objects.only("id", "rule_type").get(pk=profile.pk) + + 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 (AC16, AC19, AC20). +# ============================================================================================== + + +def test_database_indexes_from_adr_decision_5_are_present() -> None: + """ + The real database tables carry the ADR-0002 Decision 5 indexes this migration is responsible + for: positions 1, 2, 4, 5 (all covering indexes), and 9 (unique). Positions 2, 4, and 5 come + 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 (AC10). AC21 (no makemigrations drift) and AC8 (this suite also runs against MySQL) +# are verified by running manage.py / the MySQL settings module, not by a unit test. +# ============================================================================================== + + +def test_migration_seeds_exactly_one_system_default_rule_profile() -> None: + """ + Migration 0003 seeds exactly one system-default CompetencyRuleProfile: all three scope + columns null, not archived, Grade >= 0.8 (80%). See ADR-0002 Decision 3. + """ + 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