-
Notifications
You must be signed in to change notification settings - Fork 32
feat: add competency criteria models for CBE authoring layer #800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
jesperhodge
wants to merge
16
commits into
openedx:main
Choose a base branch
from
jesperhodge:jesperhodge/feat--641-competency-criteria-models
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
a6f3b04
feat: add competency criteria models for CBE authoring layer
jesperhodge ca484f4
docs: drop superseded #799 references from criteria models
jesperhodge acbfbe4
test: cover on_delete behavior for every CBE criteria foreign key
jesperhodge 4a62ab3
feat: cascade criteria deletes from tag, group and objecttag
jesperhodge 233b857
refactor: align criteria models with the 2026-09-03 revision of #641
jesperhodge 8f484e0
refactor: define rule payload shapes as attrs specs, validate on save
jesperhodge 83b9f82
chore: register simple_history and layer openedx_catalog
jesperhodge 091724e
fix: free an archived rule profile's scope and correct on_delete values
jesperhodge 5380a64
test: cover every #641 acceptance criterion and the deletion edge cases
jesperhodge 82e224e
docs: record the scope_code and on_delete decisions in ADR-0002
jesperhodge 3a79dd2
refactor: validate rule payloads with one function instead of attrs s…
jesperhodge 5c0d427
refactor: shrink criteria model comments and derive rule_type choices…
jesperhodge f094cb7
chore: trim the duplicated simple_history comment and quote migration…
jesperhodge a85cb6c
test: merge the schema tests and cut the criteria test commentary
jesperhodge 2853f03
test: point two stale references at sources a reader can follow
jesperhodge 13a98de
Merge remote-tracking branch 'origin/main' into jesperhodge/feat--641…
jesperhodge File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,8 +120,8 @@ Decision | |
| Relationship to other concepts: | ||
|
|
||
| - Each row is scoped by at most one of taxonomy, course, or organization (or by none, for the system default). A check constraint enforces that at most one of ``organization_id``, ``course_id``, and ``competency_taxonomy_id`` is non-null per row. See Decision 4 for how a criterion is assigned a profile when rows in more than one of these scopes could apply to it. | ||
| - At most one profile row may exist per distinct scope value. This is enforced by a unique constraint on the generated ``scope_code`` column (Decision 5), not a plain unique constraint on the three raw scope columns; see the ``scope_code`` column definition below for why. | ||
| - The system default is the single profile row where all three scope fields are null. ``scope_code`` is never null, including for this row (see below), so its singularity is enforced by the same unique constraint as every other profile rather than a separate procedural guarantee; it is seeded once via migration and never created or deleted through the profile API. If/when a REST API or application-layer/service code exists for editing a profile's ``rule_type``/``rule_payload``, the system default row would be editable through it like any other profile. Until then, only an operator can edit it directly (for example via Django admin or SQL). | ||
| - At most one profile row may exist per distinct scope value. This is enforced by a unique constraint on the derived ``scope_code`` column (Decision 5), not a plain unique constraint on the three raw scope columns; see the ``scope_code`` column definition below for why. | ||
| - The system default is the single profile row where all three scope fields are null. ``scope_code`` is non-null for this row, as it is for any other live profile (see below), so its singularity is enforced by the same unique constraint as every other profile rather than a separate procedural guarantee; it is seeded once via migration and never created or deleted through the profile API. If/when a REST API or application-layer/service code exists for editing a profile's ``rule_type``/``rule_payload``, the system default row would be editable through it like any other profile. Until then, only an operator can edit it directly (for example via Django admin or SQL). | ||
| - Is referenced by ``CompetencyCriterion``, which may override its type/payload. | ||
| - Never hard-deleted; retirement is archive-only (Decision 7). | ||
|
|
||
|
|
@@ -131,7 +131,7 @@ Decision | |
| 2. ``organization_id``: The ``organization_id`` of the organization that this competency rule profile is scoped to. Null if it is not scoped to a specific organization. | ||
| 3. ``course_id``: The ``course_id`` of the course that this competency rule profile is scoped to. Null if it is not scoped to a specific course. | ||
| 4. ``competency_taxonomy_id``: The ``CompetencyTaxonomy.taxonomy_ptr_id`` of the competency taxonomy that this competency rule profile is scoped to. Null if it is not scoped to a specific taxonomy. | ||
| 5. ``scope_code``: A database-generated column that is always in the fixed, trivially-parseable format ``"org:X,course:Y,taxonomy:Z"``, with each segment left blank when the corresponding scope column is null: for example ``"org:5,course:,taxonomy:"``, ``"org:,course:12,taxonomy:"``, ``"org:,course:,taxonomy:7"``, or ``"org:,course:,taxonomy:"`` for the system default. ``scope_code`` is therefore never null, including for the system default row. This exists because SQL never treats two ``NULL`` values as equal for uniqueness purposes, so a plain unique constraint across the three nullable scope columns would not stop two rows from sharing the same scope (for example two rows both with ``organization_id=5`` and the other two columns null). Collapsing the scope into one generated, always-non-null column sidesteps that, and does so identically on every database backend this project supports, including MySQL, which does not support the conditional/partial unique indexes that would otherwise be the usual fix. ``scope_code`` embeds internal ID references and exists solely to enforce uniqueness; it is not intended to be exported or exposed outside this system. | ||
| 5. ``scope_code``: A plain column, recomputed by the model's ``save()`` and never set directly, in the fixed, trivially-parseable format ``"org:X,course:Y,taxonomy:Z"``, with each segment left blank when the corresponding scope column is null: for example ``"org:5,course:,taxonomy:"``, or ``"org:,course:,taxonomy:"`` for the system default row. It is null while a profile is archived, and non-null while it is live. Collapsing the scope into one column exists because SQL never treats two ``NULL`` values as equal for uniqueness purposes, so a plain unique constraint across the three nullable scope columns would not stop two rows from sharing a scope. Nulling it while archived is what frees an archived profile's scope for a replacement, without needing the conditional unique index MySQL does not support. It is a plain column rather than a ``GeneratedField`` because Django's delete collector nulls a nullable cascading foreign key before issuing the DELETE on backends that cannot defer constraint checks, and a generated column would recompute from that nulled value and collide with whichever row already holds the resulting blank scope. A plain column is untouched by that nulling. | ||
| 6. ``rule_type``: “View”, “Grade”, “MasteryLevel” (Only “Grade” will be supported for now) | ||
| 7. ``rule_payload``: JSON payload keyed by ``rule_type`` to avoid freeform strings. It is structured JSON (not arbitrary freeform data): each ``rule_type`` defines the allowed payload shape and required keys, and validation enforces this contract. JSON is used instead of fixed columns like ``op``, ``value``, and ``scale`` so that future rule types (for example, ``MasteryLevel`` thresholds or plugin-defined evaluators such as CEL-based rules) can add their own fields without repeated schema migrations or many nullable columns. Examples: | ||
|
|
||
|
|
@@ -309,6 +309,8 @@ Decision | |
| - Once a related row exists in ``StudentCompetencyCriteriaStatus``, deletion of the associated competency definition row still succeeds, but as an archive (soft delete) instead of a hard delete: the row is hidden from authoring and new associations but remains queryable, so existing learner status rows stay resolvable. This archive-vs-hard-delete rule applies to ``oel_tagging_tag``, ``oel_tagging_taxonomy``, ``CompetencyTaxonomy``, ``oel_tagging_objecttag``, ``CompetencyCriteriaGroup``, and ``CompetencyCriteria``; see :ref:`openedx-learning-adr-0003` Decision 3 for ``oel_tagging_objecttag``'s own archive rule and traceability exception. | ||
| - ``StudentCompetencyCriteriaStatus`` is what determines whether a record is protected. ``StudentCompetencyCriteriaGroupStatus`` and ``StudentCompetencyStatus`` are roll-up tables derived from it (Decision 6) and are not independently checked for this purpose: :ref:`openedx-learning-adr-0004` writes the leaf table synchronously with the grade but rolls the two roll-up tables up later via an asynchronous task, which can lag behind the leaf or, per that ADR's Decision 5, need manual recovery. Checking only the roll-up tables could therefore miss real learner progress that has not rolled up yet. | ||
| - Direct deletion of a ``CompetencyRuleProfile`` is never a hard delete; retirement is always archive-only, via a normal update to its ``archived`` column (Decision 3). However, if a taxonomy or course that is associated with a taxonomy- or course-scoped profile is deleted, then this profile will be deleted along with it. | ||
| - ``on_delete`` on the criteria tables expresses containment, not protection: a row whose referent is gone is meaningless, so ``CompetencyCriteriaGroup.parent``, ``.tag`` and ``.course``, ``CompetencyCriterion.group`` and ``.object_tag``, and ``CompetencyRuleProfile.course`` and ``.competency_taxonomy`` all cascade. ``CompetencyCriterion.rule_profile`` stays ``PROTECT``, which is what makes "a profile is never hard-deleted by a direct delete" hold at the ORM layer. ``CompetencyRuleProfile.organization`` stays ``PROTECT`` because an ``Organization`` is not a competency definition record and ``edx-organizations`` deactivates organizations rather than deleting them. The tree links additionally have to cascade for a mechanical reason: Django's collector looks up referencing rows in the database rather than in the set it has already decided to delete, so a parent and child reached in the same batch would still trip ``PROTECT`` and abort the walk partway down. Those cascading edges are what carries a delete down to the ``PROTECT`` on the learner status tables, which is where this decision is actually enforced. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unreadable, unclear |
||
| - Known limitation: deleting a ``CompetencyTaxonomy`` whose taxonomy-scoped profile is assigned to a ``CompetencyCriterion`` raises ``ProtectedError`` naming that criterion, even though the criterion would also be cascade-deleted in the same operation through the tag chain, for the same collector reason above. This is unreachable until scoped profiles can be authored. The fix at that point is a fifth reassignment event on Decision 4: when a profile's scope owner is being deleted, reassign every criterion off that profile before the cascade proceeds. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs review |
||
|
|
||
| .. image:: images/CompetencyCriteriaModel.png | ||
| :alt: Competency Criteria Model | ||
|
|
@@ -417,7 +419,7 @@ Rejected Alternatives | |
| 2. Requires reconciling profiles whenever an organization is added to or removed from a taxonomy. | ||
| 3. Organization and taxonomy are not naturally nested (a taxonomy can belong to many organizations and vice versa), so forcing one to always contain the other does not reflect the actual relationship between them. | ||
|
|
||
| 6. Enforce ``CompetencyRuleProfile`` scope uniqueness with per-scope conditional/partial unique constraints (Django ``UniqueConstraint(condition=Q(...))``) directly on the three nullable scope columns, instead of a generated ``scope_code`` column (Decision 3). | ||
| 6. Enforce ``CompetencyRuleProfile`` scope uniqueness with per-scope conditional/partial unique constraints (Django ``UniqueConstraint(condition=Q(...))``) directly on the three nullable scope columns, instead of the derived ``scope_code`` column (Decision 3). | ||
|
|
||
| 1. Pros | ||
|
|
||
|
|
||
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| """ | ||
| Models for Competency-Based Education (CBE). | ||
| """ | ||
|
|
||
| from ..rule_payloads import RuleType | ||
| from .competency_taxonomy import CompetencyTaxonomy | ||
| from .criteria import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, LogicOperator | ||
|
|
||
| __all__ = [ | ||
| "CompetencyCriteriaGroup", | ||
| "CompetencyCriterion", | ||
| "CompetencyRuleProfile", | ||
| "CompetencyTaxonomy", | ||
| "LogicOperator", | ||
| "RuleType", | ||
| ] |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't archiving an operation that is intended to possibly restored later? If we remove the scope, don't we loose that data? Maybe we should do `"org:X,course:Y,taxonomy:Z" with possible blank segments, but for archived items, we do something like "org:X,course:Y,taxonomy:Z,archive-version:1" to keep it restorable and unique