Skip to content

feat: add CompetencyRuleProfile and seed the system default - #6

Open
jesperhodge wants to merge 2 commits into
jesperhodge/cbe-641-05-rule-payloadsfrom
jesperhodge/cbe-641-06-rule-profile
Open

feat: add CompetencyRuleProfile and seed the system default#6
jesperhodge wants to merge 2 commits into
jesperhodge/cbe-641-05-rule-payloadsfrom
jesperhodge/cbe-641-06-rule-profile

Conversation

@jesperhodge

@jesperhodge jesperhodge commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Part 6 of 7 for issue openedx#641. Based on part 5. This is the largest part of the stack, because scope_code and scope immutability are one design and splitting them would leave a reviewer holding half of it.

# Branch What it adds
1 cbe-641-01-adr ADR-0002 scope_code amendment (openedx#809)
2 cbe-641-02-deps-and-layering django-simple-history, .importlinter (#2)
3 cbe-641-03-taxonomy-overrides-org taxonomy_overrides_org, models/ package (#3)
4 cbe-641-04-criteria-group CompetencyCriteriaGroup (#4)
5 cbe-641-05-rule-payloads RuleType, validate_rule_payload (#5)
6 cbe-641-06-rule-profile CompetencyRuleProfile + seed (#6)
7 cbe-641-07-criterion CompetencyCriterion (#7)

This PR settles its own three on_delete values, and tests them itself. organization is
PROTECT, course and competency_taxonomy are CASCADE. on_delete expresses containment,
not protection: it governs deletion of the row a foreign key points at, never the row holding
it. An Organization is not a competency definition record, so a profile never controls whether
one can be deleted; a course- or taxonomy-scoped profile, on the other hand, is meaningless once
its scope is gone, so those two cascade. test_rule_profile_deletion.py covers all three,
including under MySQL's non-deferred constraint semantics. There used to be a separate part 8
collecting every model's delete tests in one place; it added nothing of its own once each
model's own tests moved to the PR that declares its foreign keys, so it is closed.

⚠️ Two things to decide here, carried over from openedx#800.

1. course is CASCADE, not PROTECT. AC 26 lists it as PROTECT and calls all nine
values across the stack final. PROTECT here would make a course run permanently undeletable
the moment any profile is scoped to it, even with zero learner data, which is stricter than
anything openedx#641 needs, and matches what you asked for on openedx#800. If accepted, AC 26 needs updating
so the ticket and the code agree. (CompetencyCriteriaGroup.course in part 4 has the same
deviation, for the same reason.)

2. organization is PROTECT, and the AC and your openedx#800 comments disagree with each other.
This PR follows the AC. You wrote:

PROTECT is no good. The org delete shouldn't be blocked. Instead, use models.SET() to run
code to archive the rule_profile, but only if no learners are connected.

The same comment thread also touched course ("Use models.SET() and archive, only if there
are no learners connected") and competency_taxonomy ("Maybe."), so all three of this model's
foreign keys are collected below rather than split across the two decisions above.

Worth knowing before deciding: the "only if no learners are connected" half needs openedx#642's
Student*Status tables to know the answer, so it is not implementable in this slice.
models.SET() cannot archive the profile either way. It sets the foreign key column on
the referencing row to a value; it has no way to set archived=True on that row, so "archive
the rule_profile" is not something this hook can express. Archiving on delete would need a
pre_delete signal or an application-layer function that runs before the collector does. A
callable passed to models.SET() also runs inside the collector's walk, which is a constrained
place for application logic.

And the nearest implementable substitute has a real defect. If organization and course
became SET_NULL, the org or course delete would succeed and the profile would survive
unscoped, which is half of what you asked for. But scope_code is recomputed only in save()
(see below), so a profile whose course_id the collector nulled keeps a stale scope_code
still claiming that course. The next time anything saves that row, scope_code recomputes to
the blank all-null scope and collides with the seeded system default, raising IntegrityError.
That is a landmine, so this PR does not ship it.

What stands in this PR, pending your call:

Field Value Effect if left as is
organization PROTECT Org deletes stay blocked. Contradicts your comment until openedx#642 lands; edx-organizations deactivates rather than deletes, so a blocked org delete is close to hypothetical
course CASCADE Deviates from AC 26 as described above
competency_taxonomy CASCADE What AC 25 requires; your "Maybe." neither endorsed nor rejected it

The options for organization and course I can see:

Option Effect Cost
Leave as is, open a follow-up on openedx#642 Org deletes stay blocked for now Contradicts your comment until openedx#642 lands
SET_NULL on both, plus recompute scope_code in the same change Org and course deletes succeed, profile survives Needs scope_code maintained outside save(), a design change to the section below
pre_delete signal that archives the profile Closest to what you described New application-layer machinery, and the learner check still waits for openedx#642

Tell me which and I will make the change here; it is contained to this PR.

What this does

Adds CompetencyRuleProfile, a reusable evaluation rule scoped to at most one of an organization, a course or a taxonomy, plus the single row scoped to none of them. That last row is the rule every criterion falls back to when nothing more specific applies, so a deployment that installs this app and adds no profiles of its own gets an 80% threshold. A data migration seeds it with Grade >= 0.8.

The one design worth reading carefully: scope_code

At most one profile may exist per distinct scope. That cannot be a plain unique constraint over the three nullable scope columns, because SQL never treats two NULLs as equal, so two rows both with organization_id=5 and the other two null would not collide. It cannot be a conditional constraint either: that compiles to a partial index, MySQL does not support partial indexes, and Django raises only a non-fatal models.W036 warning and silently skips creating the constraint there, while SQLite does support them and would hide the gap in a local run. ADR-0002 Rejected Alternative 6 records this.

So the scope collapses into one derived scope_code column, "org:X,course:Y,taxonomy:Z" with a blank segment per null, carrying a single unconditional unique constraint. It goes null while a profile is archived, which is what frees the scope for a replacement, using the same NULL rule that made a plain constraint useless. The three scope columns are never cleared, so nothing is lost and an archived profile stays restorable, which answers the question you raised on openedx#800 about whether archiving discards the scope. It does not.

There is a second reason scope_code is a plain column written in save() rather than a GeneratedField, and it is about Django's delete collector: a generated column would recompute from a foreign key the collector nulls mid-cascade, and collide with whichever row already holds the resulting blank scope. test_rule_profile_deletion.py pins this under MySQL's non-deferred constraint semantics, reproduced on SQLite by monkeypatching can_defer_constraint_checks.

Scope is immutable after creation. Editing may change rule_type, rule_payload and archived only, so criteria already resolved to a profile are never silently re-governed. clean() compares against what is actually persisted rather than a value cached at load time, so a .only()/.defer() load cannot bypass it; there is a test for that. It does not cover QuerySet.update() or bulk_create(), which never build a model instance, which is why the invariants that matter are database check constraints rather than clean() alone.

Reviewing this PR

test_rule_profile.py is sectioned so each section is one claim: schema, the at-most-one-scope rule, scope_code and archiving, that the payload validator is actually wired into save(), scope immutability, then index 9, history and the seeded row. Payload shapes are covered exhaustively and without a database in part 5; the tests here only prove a model save reaches that validator. test_rule_profile_deletion.py is the sibling module for this model's own on_delete values.

Verifying

pytest tests/openedx_learning --no-cov -q                            # 76 passed
python manage.py makemigrations openedx_learning --check --dry-run   # no changes detected

Run this suite against MySQL, not only SQLite. Issue openedx#641 asks for it specifically, because the scope_code constraint is the one thing SQLite cannot tell you the truth about:

pytest tests/openedx_learning --no-cov -q --ds=mysql_test_settings --reuse-db
docker exec oel-mysql-test mysql -utest_oel_user -ptest_oel_pass test_oel_db -e \
  "SHOW INDEX FROM openedx_learning_competencyruleprofile WHERE Key_name LIKE '%scope_code%';"
# expect one row with Non_unique = 0

By hand, the archive-and-replace cycle is the most interesting behavior:

from openedx_learning.models import CompetencyRuleProfile
from organizations.api import ensure_organization
from organizations.models import Organization

ensure_organization("Acme")
org = Organization.objects.get(short_name="Acme")
payload = {"op": "gte", "value": 0.8, "scale": "percent"}

first = CompetencyRuleProfile.objects.create(organization=org, rule_type="Grade", rule_payload=payload)
first.scope_code                       # 'org:<id>,course:,taxonomy:'
CompetencyRuleProfile.objects.create(organization=org, rule_type="Grade", rule_payload=payload)  # IntegrityError
first.archived = True
first.save()
first.refresh_from_db()
first.scope_code                       # None: the slot is free
first.organization_id                  # still set: nothing was lost
CompetencyRuleProfile.objects.create(organization=org, rule_type="Grade", rule_payload=payload)  # now allowed

Refs openedx#641

jesperhodge and others added 2 commits September 11, 2026 10:18
A reusable evaluation rule scoped to at most one of an organization, a course
or a taxonomy, plus the one row scoped to none of them, which is the rule every
criterion falls back to. A deployment that adds no profiles of its own gets an
80% threshold. See ADR-0002 Decision 3.

Uniqueness per scope cannot be a plain constraint over the three nullable scope
columns, because SQL never treats two NULLs as equal, and it cannot be a
conditional constraint either, because MySQL has no partial unique indexes and
Django silently skips creating one there. So the scope collapses into a derived
scope_code column with one unconditional unique constraint. scope_code goes
null while a profile is archived, which frees that scope for a replacement; the
three scope columns are never cleared, so nothing is lost.

Scope is immutable after creation, so criteria already resolved to a profile
are never silently re-scoped.

Deleting a scope owner takes the profile scoped to it, so course and
competency_taxonomy cascade, per ADR-0002 Decision 7. organization does not: an
Organization is not a competency definition record, and edx-organizations
retires one by clearing its active flag rather than deleting the row, so PROTECT
there refuses a delete that should not be happening.

Refs openedx#641

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
These six tests only need CompetencyRuleProfile, so they belong with the
on_delete values this PR already declares, not in the cross-model part 8
suite they were drafted alongside.
@jesperhodge
jesperhodge force-pushed the jesperhodge/cbe-641-06-rule-profile branch from 98a6bfa to e5fa9a1 Compare September 11, 2026 14:18
@jesperhodge
jesperhodge removed this pull request from stack #11 September 11, 2026 14:29
@jesperhodge
jesperhodge added this pull request to stack #12 September 11, 2026 14:33
@jesperhodge
jesperhodge removed this pull request from stack #12 September 11, 2026 14:34
@jesperhodge
jesperhodge added this pull request to stack #13 September 11, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant