feat: add CompetencyCriterion, the criteria tree's leaf - #7
jesperhodge wants to merge 2 commits into
Conversation
551b226 to
68df7e3
Compare
68df7e3 to
196ca9d
Compare
6e5e0b9 to
a28a706
Compare
a28a706 to
e5c161c
Compare
e5c161c to
d84daef
Compare
d84daef to
e7d08f7
Compare
e7d08f7 to
71e0b24
Compare
71e0b24 to
a458e18
Compare
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>
…n test down to this PR This model completes the criteria tree, so its own on_delete values, the transitive tag/taxonomy cascades that only exist once it does, the two RESTRICT-vs-PROTECT payoff/residual scenarios, and the tree-wide integration test all belong here, not in the cross-model part 8 suite they were drafted alongside.
a458e18 to
34b696a
Compare
mgwozdz-unicon
left a comment
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Requests from me; comment drafted by Claude:
Solid completion of the tree, with some genuinely good edge-case tests (the RESTRICT-vs-PROTECT pair, the childless-group case, the sibling-safety integration test). Requesting the changes below, all naming, docstring, and test-trimming items, plus one factual correction; no correctness bugs found.
src/openedx_learning/applets/cbe/models/criteria.py
1. Class docstring (lines 266-284): shorten considerably, and drop the validate_rule_payload cross-reference. A reader doesn't need the exact function path to understand the contract, just that the shape gets validated.
Current:
"""
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:
"""Requested:
"""
A leaf node in a Competency Criteria tree: one tag/object association plus its rule.
A null `rule_profile` does not mean "resolve at read time." ADR-0002 Decision 4 resolves and
stores the applicable profile (or override) at specific write events only; `rule_profile` is
null only when a per-criterion override is set instead. Do not add a property, manager method,
or other helper that recomputes it; that would contradict the ADR.
.. no_pii:
"""2. Module docstring (line 2): same rename as PR #4.
Current:
"""
The CompetencyAchievementCriteria models: CompetencyCriteriaGroup, CompetencyRuleProfile, and
CompetencyCriterion.
See :ref:`openedx-learning-adr-0002` Decisions 2, 3 and 4 for the design and Decision 7 for each
foreign key's delete behavior, and :ref:`openedx-learning-adr-0003` Decisions 1 and 2 for why
these models carry ``django-simple-history`` tracking and CompetencyTaxonomy does not.
"""Requested:
"""
The Competency Criteria tree models: CompetencyCriteriaGroup, CompetencyRuleProfile, and
CompetencyCriterion.
See :ref:`openedx-learning-adr-0002` Decisions 2, 3 and 4 for the design and Decision 7 for each
foreign key's delete behavior, and :ref:`openedx-learning-adr-0003` Decisions 1 and 2 for why
these models carry ``django-simple-history`` tracking and CompetencyTaxonomy does not.
"""3. Add help_text to rule_type_override and rule_payload_override. Every other field on this model has one; these two don't.
Current:
rule_type_override = models.CharField(max_length=32, choices=RuleType, null=True, blank=True)
rule_payload_override = models.JSONField(null=True, blank=True)Requested:
rule_type_override = models.CharField(
max_length=32, choices=RuleType, null=True, blank=True,
help_text=_("Overrides rule_profile's rule_type for this criterion. Set only when rule_profile is null."),
)
rule_payload_override = models.JSONField(
null=True, blank=True,
help_text=_("Overrides rule_profile's rule_payload for this criterion. Set only when rule_profile is null."),
)tests/openedx_learning/applets/cbe/test_criteria_trees.py
4. Module docstring: reorder and shorten. It currently opens by describing what three other test files do before ever saying what this one does.
Requested:
"""Integrative test for the Competency Criteria tree: deleting a group partway down a wider,
realistic tree removes exactly that subtree and leaves an untouched sibling branch alone.
Fixtures shared with the other test modules in this directory live in its conftest.py.
"""5. The single test's docstring: drop the rule-governance detail and the closing sentence. The test deletes a group; CompetencyCriterion.group is CASCADE regardless of whether a criterion is profile-assigned or override-governed, so mixing both governance styles into the setup is incidental, not signal. The closing sentence editorializes about the test's own value rather than describing behavior.
Current:
"""
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.
"""Requested:
"""
Deleting a CompetencyCriteriaGroup partway down a realistic tree removes exactly that group,
every descendant beneath it, and every criterion under any of them -- and nothing else. A
sibling branch of the deleted group, with its own criterion, survives completely unchanged.
Tree built here, all under one root:
root
|-- branch_to_delete (criterion)
| `-- grandchild (criterion)
`-- surviving_sibling (criterion)
`branch_to_delete` is deleted. This exercises criteria at two different tree depths (on
`branch_to_delete` itself and on its child `grandchild`), and confirms `surviving_sibling` and
its own criterion are byte-for-byte untouched: same primary keys, still present, in a tree that
shares a root with the subtree that just got removed.
"""tests/openedx_learning/applets/cbe/test_criterion.py
6. Module docstring: cut to one line. Same norm as elsewhere in this codebase.
Requested: """Tests for CompetencyCriterion, a leaf of a Competency Criteria tree."""
7. Lines 48-70 (approx.), test_criterion_has_exactly_the_columns_adr_0002_decision_4_lists: delete this test, and the "Schema" section banner above it. Checked both openedx-core (outside this app) and a sample of openedx-platform for a test whose entire purpose is asserting a model's exact field/column set via Meta/_meta.get_fields() introspection: found nothing anywhere else in either codebase. Same disposition as the index-existence test in item 12/13 below: not an established pattern here. Separately: its docstring says "No archived column yet; that arrives with openedx#642" — that's incorrect (both openedx#613 and openedx#641 say openedx#716 owns that column), but moot once the test is deleted. This test is the only thing under the # Schema header, so remove that banner too.
8. Lines 73-80: shorten to just line 74.
Current:
# ---------------------------------------------------------------------------------------------
# Either a rule_profile or both overrides. Never both, never neither.
# ADR-0002 Decision 4. Three of the four invalid states reach the database check constraint
# and raise IntegrityError. The fourth, rule_type_override set with no payload, is caught
# earlier by save()'s payload validation and raises ValidationError instead.
# ---------------------------------------------------------------------------------------------Requested:
# ---------------------------------------------------------------------------------------------
# Either a rule_profile or both overrides. Never both, never neither.
# ---------------------------------------------------------------------------------------------9. Lines 100-108: trim to just the one-line invariant statement.
Current:
"""
A CompetencyCriterion must have either a rule_profile with no overrides, or both override
fields set with no rule_profile, never both and never neither. See ADR-0002 Decision 4.
Covers the three invalid states that reach the database's check constraint: both set, neither
set, and only rule_payload_override set. The fourth invalid state, only rule_type_override set,
is caught earlier by save()'s own validation instead and raises ValidationError before the
database is ever touched; see test_setting_a_rule_type_override_without_a_payload_is_rejected_by_save
below for that case, and why it raises a different exception type than these three.
"""Requested:
"""
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.
"""10. Lines 139-153: delete the docstring entirely. Not required by this repo's lint config (missing-docstring is disabled repo-wide in pylintrc, and test_.+ is separately exempted even when the check is on), and the test name (test_setting_a_rule_type_override_without_a_payload_is_rejected_by_save) is already fully descriptive.
11. Lines 180-201, test_criterion_rule_profile_is_not_recomputed_once_a_more_specific_profile_appears: delete this test. rule_profile is a plain ForeignKey with no property, manager method, or signal touching it, so there's no concrete, plausible accidental-regression path this guards against, unlike, say, a parametrized enum-completeness test. Its protection is already redundant with the explicit prohibition stated in the class docstring (item 1 above).
12. Line 205: update the section header once item 13 lands.
Current: # Indexes 4 and 5, and history
Requested: # History
13. Lines 211-225, test_the_database_carries_adr_0002_decision_5_indexes_4_and_5: delete this test. Same disposition as items 7 and 12 above, and the equivalent tests in the PR #4 and PR #6 comments: no precedent anywhere else in either repo for introspecting the database schema directly to assert an index exists.
14. Line 233: trim to a bare ADR pointer.
Current:
"""
HistoricalRecords() is applied to CompetencyCriterion: creating a criterion and then switching
it from a profile to overrides leaves two rows in the Historical model. See ADR-0003
Decision 1, and Decision 4 for why that switch is an authoring event worth recording.
"""Requested:
"""
HistoricalRecords() is applied to CompetencyCriterion: creating a criterion and then switching
it from a profile to overrides leaves two rows in the Historical model. See ADR-0003
Decision 1 & 4 for more info.
"""tests/openedx_learning/applets/cbe/test_criterion_deletion.py
15. Module docstring: cut to one line. Same norm as elsewhere in this codebase; the foreign-key table and the RESTRICT-vs-PROTECT paragraph are already restated, more concretely, in individual tests' own docstrings below (see items 17 and 22).
Requested: """Delete-behavior tests for CompetencyCriterion's own foreign keys, and the transitive and scope-owner cases that only exist once this model completes the criteria tree."""
16. Lines 76-78: delete the last sentence. "Doubles as the 'OURS' half of openedx#641's Deletions criterion..." is unclear internal jargon that doesn't help a reader understand the test.
Current:
"""
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.
"""Requested:
"""
Deleting an ObjectTag cascades to any CompetencyCriterion referencing it via `object_tag`: the
delete succeeds and the criterion row is gone too.
"""17. Lines 93-99: shorten.
Current:
"""
Deleting a CompetencyRuleProfile that a CompetencyCriterion references via `rule_profile`
raises RestrictedError, which is what holds ADR-0002 Decision 7's "a profile is never
hard-deleted by a direct delete" at the ORM layer.
Nothing cascades from a profile down to a criterion, so the criterion is not part of this
delete and RESTRICT refuses, exactly as PROTECT would have.
"""Requested:
"""
Deleting a CompetencyRuleProfile that a CompetencyCriterion references via `rule_profile`
raises RestrictedError.
"""18. Lines 112-128 (childless-group test): trim the docstring, and state that any cleanup is application-layer work.
Current:
"""
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.
"""Requested:
"""
Deleting an ObjectTag cascades away the CompetencyCriterion that references it, but leaves the
CompetencyCriteriaGroup that housed that criterion in place, even when the group now has no
children of any kind.
This is deliberate, not a bug: CompetencyCriteriaGroup has no foreign key to ObjectTag, so
nothing about this delete gives Django's collector a reason to reach the group. Cleaning up a
now-childless group is authoring-API/application-layer work, not something an on_delete value
can express here.
"""19. Line 142, and the last sentence at 145-146: delete. Both are about this code's history/provenance rather than its current behavior.
Current:
# ---------------------------------------------------------------------------------------------
# Transitive deletes required by issue #641
# Deleting a Tag, a group at depth, or a Taxonomy takes the whole referencing criteria tree
# with it. Tag.taxonomy is already CASCADE in openedx_tagging, which is what makes the tag
# case hold transitively from a taxonomy. These only exist as of this PR, because they need
# CompetencyCriterion to complete the tree down to a leaf.
# ---------------------------------------------------------------------------------------------Requested:
# ---------------------------------------------------------------------------------------------
# Deleting a Tag, a group at depth, or a Taxonomy takes the whole referencing criteria tree
# with it. Tag.taxonomy is already CASCADE in openedx_tagging, which is what makes the tag
# case hold transitively from a taxonomy.
# ---------------------------------------------------------------------------------------------20. Lines 179-182: delete. Not necessary to trust or understand the test, which is already clear from its own assertions.
Current:
"""
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.
"""Requested:
"""
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.
"""21. Lines 172-205, test_group_delete_at_depth_cascades_descendants_and_their_criteria: delete this test. It overlaps with test_criteria_trees.py's single test, which builds the identical depth-2 shape (root -> child -> grandchild, criteria at two depths) plus an untouched sibling branch, and so already proves everything this test proves, plus more (the sibling-safety property). Keep test_criteria_trees.py; this one is the redundant half.
22. Lines 215-222: reword. The "requires 'at minimum'" phrasing is awkward and hard to parse.
Current:
"""
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).
"""Requested:
"""
Deleting an oel_tagging.Taxonomy collects every Tag beneath it (Tag.taxonomy is CASCADE), so
this taxonomy delete succeeds and cascades away the group and criterion beneath its tag too,
the same as a direct tag delete.
Chain exercised: CompetencyTaxonomy -> Tag (CASCADE) -> CompetencyCriteriaGroup.tag (CASCADE)
-> CompetencyCriterion.group (CASCADE).
"""2 participants
Part 7 of 7 for issue openedx#641. Based on part 6. Completes the criteria tree: this model's own
on_deletevalues, the tests for them, the transitive and scope-owner cases that only exist once the tree is complete, and the tree-wide integration test all land here.What this does
Adds
CompetencyCriterion, a leaf of the tree. A leaf points at oneObjectTag, meaning one specific piece of tagged content, and takes its pass rule either from a sharedCompetencyRuleProfileor from its own inline override pair. A check constraint enforces ADR-0002 Decision 4's invariant: eitherrule_profileis set and both override fields are null, orrule_profileis null and both overrides are set. Never both, never neither.The stored
rule_profileis never re-resolved at read time. Decision 4 assigns it at four named write events and stores the result. Creating a more specific profile later does not silently re-govern a criterion that already resolved to a less specific one; a test pins that, so a property or manager method that recomputed the foreign key on every read would fail rather than quietly contradicting the ADR. Computing the assignment itself is authoring-API work and is not here.The class is named
CompetencyCriterion, singular, and carries noMeta.db_tableoverride, so the table is Django's defaultopenedx_learning_competencycriterion. ADR-0002's heading, "CompetencyCriterionconcept (CompetencyCriteriadatabase table)", names the domain concept the way every other heading in that ADR does rather than instructing a rename, and no model insrc/overridesdb_tabletoday.Reviewing this PR
test_criterion.pyis ordered schema, then the either-profile-or-overrides invariant and every way to violate it, then override payload validation and the profile that is never re-resolved, then indexes and history.test_criterion_deletion.pyis the sibling module for this model's ownon_deletevalues and everything that only exists once it completes the tree.test_criteria_trees.pyholds the one test that needs a wider, mixed tree to make its point: it deletes a group partway down a realistic tree and asserts the exact surviving row set, which is where a per-foreign-key test cannot help you.The invariant is worth checking closely, because three of its four invalid states reach the database check constraint and raise
IntegrityError, while the fourth,rule_type_overrideset with no payload, is caught earlier bysave()'s payload validation and raisesValidationError. Two similar-looking mistakes therefore fail differently, andtest_setting_a_rule_type_override_without_a_payload_is_rejected_by_savedocuments why.Verifying
By hand, the invariant, which is the one a future API can most easily break:
And that the stored profile is not re-resolved at read time:
Refs openedx#641