feat: settle delete behavior for the competency criteria models - #8
Closed
jesperhodge wants to merge 4 commits into
Closed
feat: settle delete behavior for the competency criteria models#8jesperhodge wants to merge 4 commits into
jesperhodge wants to merge 4 commits into
Conversation
This was referenced Sep 9, 2026
jesperhodge
force-pushed
the
jesperhodge/cbe-641-07-criterion
branch
from
September 9, 2026 21:48
68df7e3 to
196ca9d
Compare
jesperhodge
force-pushed
the
jesperhodge/cbe-641-08-delete-behavior
branch
from
September 9, 2026 21:48
5886317 to
a7e7e66
Compare
jesperhodge
force-pushed
the
jesperhodge/cbe-641-07-criterion
branch
from
September 10, 2026 03:03
196ca9d to
6e5e0b9
Compare
jesperhodge
force-pushed
the
jesperhodge/cbe-641-08-delete-behavior
branch
2 times, most recently
from
September 10, 2026 19:03
c43a249 to
bfc9896
Compare
jesperhodge
force-pushed
the
jesperhodge/cbe-641-07-criterion
branch
from
September 10, 2026 19:03
6e5e0b9 to
a28a706
Compare
jesperhodge
added this pull request to stack #11
September 11, 2026 13:17
ADR-0002 Decision 3 stores an evaluation rule as a rule_type plus a JSON rule_payload whose shape that type defines, rather than as fixed op/value/scale columns, so a future rule type can add its own fields without a migration. The cost of JSON is that nothing enforces the shape, so this adds the validator the two criteria models will call from clean(). Grade is the only supported type. Its value is a fraction from 0.0 to 1.0, not a number out of 100, which is the mistake an author is most likely to make, so the out-of-range message names the convention rather than only rejecting the value. RuleType declares exactly the types that have a payload spec, so a type can never be offered as a choice without being saveable. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
A leaf points at one ObjectTag, meaning one specific piece of tagged content, and takes its pass rule either from a shared CompetencyRuleProfile or from its own inline override pair. A check constraint enforces ADR-0002 Decision 4's invariant: never both, never neither. The stored rule_profile is not resolved at read time. Decision 4 assigns it at four named write events and stores the result, so a criterion that already resolved to a less specific profile is not silently re-governed when a more specific one appears later. Computing that assignment is authoring-API work and is not here. group and object_tag cascade, per ADR-0002 Decision 7: a leaf means nothing without the group above it or the content association it evaluates. rule_profile is RESTRICT rather than PROTECT. Both refuse a direct profile delete while any criterion is assigned to it, which is what makes a profile archive-only at the ORM layer. They differ once the profile is deleted as part of a larger operation: PROTECT raises for any referencing row it finds in the database, so deleting a CompetencyTaxonomy would fail naming a criterion the same operation was already about to remove, while RESTRICT ignores rows that are themselves being deleted and lets that cascade through. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each model now declares its own on_delete values, so what is left to prove is the behavior they produce together, which no single model's tests can reach: that deleting a Tag or a Taxonomy takes the whole authored tree with it, that a scope owner's deletion carries its profile away, and that a direct profile delete is still refused. Two cases turn on RESTRICT rather than PROTECT on CompetencyCriterion. rule_profile. Deleting a CompetencyTaxonomy whose scoped profile is assigned to a criterion now succeeds, because the same operation is already deleting that criterion through the tag chain; PROTECT raised there, naming a criterion that was about to be removed anyway. Deleting a CourseRun is still refused when the criterion assigned to its scoped profile sits in a tree with no course scope, which Decision 4 permits, because that criterion really would be left pointing at a deleted profile. The deletion paths also run under MySQL's collector semantics while still on SQLite, by setting can_defer_constraint_checks to false, so the pre-delete nulling of a nullable cascading foreign key is exercised in the fast local suite rather than only in CI. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jesperhodge
force-pushed
the
jesperhodge/cbe-641-07-criterion
branch
from
September 11, 2026 13:27
a28a706 to
e5c161c
Compare
jesperhodge
force-pushed
the
jesperhodge/cbe-641-08-delete-behavior
branch
from
September 11, 2026 13:27
bfc9896 to
f9c8ae4
Compare
jesperhodge
force-pushed
the
jesperhodge/cbe-641-07-criterion
branch
from
September 11, 2026 14:05
e5c161c to
d84daef
Compare
Owner
Author
|
Closing this one. Every test here turned out to only need the model(s) that already exist by the time it could be written: the MySQL-collector tests split between whichever model's So there was nothing left that actually needed a PR of its own on top of part 7. All 21 tests now live in:
Part 7 is now the top of the stack. Nothing in this branch is lost — it's superseded, not discarded. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 8 of 8 for issue openedx#641. Based on part 7. Parts 4, 6, and 7 each declare their own
on_deletevalues now, as they add the foreign key that value governs. This PR adds noon_deletevalue of its own; it adds the tests that exercise those decisions together, one cross-model limitation those decisions produce, and the tests pinning the MySQL collector behaviorCompetencyRuleProfile.scope_code's design depends on.cbe-641-01-adrscope_codeamendment (openedx#809)cbe-641-02-deps-and-layering.importlinter(#2)cbe-641-03-taxonomy-overrides-orgtaxonomy_overrides_org,models/package (#3)cbe-641-04-criteria-groupCompetencyCriteriaGroup(#4)cbe-641-05-rule-payloadsRuleType,validate_rule_payload(#5)cbe-641-06-rule-profileCompetencyRuleProfile+ seed (#6)cbe-641-07-criterionCompetencyCriterion(#7)cbe-641-08-delete-behaviorWhere each decision lives
Each of the nine foreign keys below is declared, with its real
on_deletevalue, in the part that adds it. This PR adds none of them; it is a reference for reading the tests that follow.CompetencyCriteriaGroup.parentCASCADECompetencyCriteriaGroup.tagCASCADECompetencyCriteriaGroup.courseCASCADECompetencyRuleProfile.organizationPROTECTCompetencyRuleProfile.courseCASCADECompetencyRuleProfile.competency_taxonomyCASCADECompetencyCriterion.groupCASCADECompetencyCriterion.object_tagCASCADECompetencyCriterion.rule_profileRESTRICTEach part above explains its own values and, where relevant, the AC deviations and open questions attached to them.
on_deleteexpresses containment, not protection: it governs deletion of the row a foreign key points at, never the row holding it. The seven cascading edges are how Django's collector walks down the tree, and they are load-bearing rather than a relaxation: openedx#642 putsPROTECTon the learner status tables one and two levels below the tag, and the collector reaches those only along edges markedCASCADE.One deviation from openedx#641's acceptance criteria that belongs to this PR
ProtectedErrortests that AC 27 does not ask for. AC 27 says only the cascade half is tested here, because theProtectedErrorhalf needs openedx#642's tables. This PR tests threeProtectedErrorcases anyway: an organization with a scoped profile, a profile referenced by a criterion, and the known limitation below. None needs a status row, so they cost nothing and pin real behavior. Additive, not a violation.One known limitation, pinned rather than worked around
Deleting a
CompetencyTaxonomywhose taxonomy-scoped profile is assigned to aCompetencyCriterionraisesProtectedErrornaming the criterion, not the profile actually being cascaded away, becausePROTECTchecks the database rather than the collector's own pending set. That criterion would also be cascade-deleted in the same operation through the tag chain, so the failure is spurious.It is unreachable in this phase: no code path creates a taxonomy-scoped profile. ADR-0002 Decision 7 records the fix for when scoped profiles are built, a fifth reassignment event on Decision 4.
test_taxonomy_delete_blocked_by_its_scoped_profile_names_the_criterion_not_the_profilepins the current behavior so that changing it later is deliberate.Reviewing this PR
test_criteria_deletion.pyopens with the table above, then works through each model's foreign keys, the transitive cases openedx#641 requires, the known limitation, and finally the MySQL collector path.test_criteria_trees.pyholds the two integrative tests, which are where a per-foreign-key test cannot help you:test_deleting_a_middle_group_removes_its_subtree_but_leaves_the_rest_of_the_tree_untouchedbuilds a tree with a surviving sibling branch and a mix of profile-assigned and override-governed criteria, then asserts the exact surviving row set. A test that only checks "the deleted branch is gone" cannot distinguish a correct cascade from one that over-deletes into a sibling.test_object_tag_delete_leaves_a_childless_criteria_group_behindpins an accepted outcome, not a bug:CompetencyCriteriaGroupnever referencesObjectTag, so nothing gives the collector a reason to reach the group, and it is left inert and childless.The three
..._under_mysql_collector_semanticstests monkeypatchcan_defer_constraint_checkstoFalse. Without that, the code path they exercise never runs on SQLite and they pass against broken and correct code alike. That path is also whyCompetencyRuleProfile.scope_codeis a plain column rather than aGeneratedField, reasoned through in part 6 (#6).What is deliberately not here
Every "raises
ProtectedErrorbecause a learner status row exists" case. Those need openedx#642's threeStudent*Statustables, and nothing here stubs or fakes a status model to stand in. Until openedx#642 merges,maincarries a cascade chain with noPROTECTat the bottom, so deleting a tag removes the whole authored tree and nothing objects. That window is expected and harmless, because the learner status tables do not exist yet.Also not here: any
delete()override, archive-versus-delete branch, or deletion-lock field. openedx#655 governs that and it lands in openedx#674, openedx#675, openedx#716, openedx#776 and openedx#778.Verifying
By the time part 7 lands, every one of the nine values above is already real; there is nothing left "undecided" going into this PR. What this PR adds is the cross-model and edge-case coverage a per-field test cannot give you, such as the two integrative tests above and the known limitation below.
Refs openedx#641