Conversation
|
Thanks for the pull request, @jesperhodge! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
| OR = "OR", _("Or") | ||
|
|
||
|
|
||
| def validate_rule_payload(rule_type: str, payload: Any) -> None: |
There was a problem hiding this comment.
I don't like payload: Any, there should be a type for the dict.
And I want to validate against that type.
| .. no_pii: | ||
| """ | ||
|
|
||
| # Set at from_db() time to the scope this row had when it was loaded from the database, so |
There was a problem hiding this comment.
Comment too hard to understand
| # from_db() is a classmethod, so it sets this through a local `instance` variable rather than | ||
| # `self`, which pylint's protected-access check can't tell apart from reaching into another | ||
| # object's internals. | ||
| loaded_scope: tuple[int | None, int | None, int | None] | None = None |
There was a problem hiding this comment.
What is loaded_scope and why is it a tuple of ints?
| Organization, | ||
| null=True, | ||
| blank=True, | ||
| on_delete=models.PROTECT, |
There was a problem hiding this comment.
organization & course on_delete should also be CASCADE, with Python logic elsewhere ensuring that no learners are linked to this (if they are linked, organization / course can still be deleted but rule profile stays.)
There was a problem hiding this comment.
I think this is not quite right. CASCADE should result in archival, not deletion.
Question to flag for later: what should happen if someone actually wants to modify (or rather, delete and then replace) a rule profile? Assuming this gets archived, do the archived ones still need to be unique? In that case they are never modifiable and we need some hard delete or overwrite mechanism, I guess.
There was a problem hiding this comment.
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.
| CourseRun, | ||
| null=True, | ||
| blank=True, | ||
| on_delete=models.PROTECT, |
There was a problem hiding this comment.
This also needs to be CASCADE but then it should stay if there are any actual learner's already connected to the mastery. Same as elsewhere
| """Capture the scope this row had when loaded, so clean()/save() can detect an edit to it.""" | ||
| instance = super().from_db(db, field_names, values) | ||
| # field_names holds attnames (e.g. "organization_id"), not field names. Only capture when |
There was a problem hiding this comment.
I have no idea what this is supposed to mean. Clarify what the intent is here.
There was a problem hiding this comment.
Why do we need to override from_db at all?
| ) | ||
| return instance | ||
|
|
||
| def _check_scope_immutable(self) -> None: |
There was a problem hiding this comment.
This seems pretty complicated code. Can it be simplified following KISS principle?
| 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=RuleType, null=True, blank=True) | ||
| rule_payload_override = models.JSONField(null=True, blank=True) |
There was a problem hiding this comment.
Use attrs, as in my other comment.
| null=True, | ||
| blank=True, | ||
| db_column="competency_rule_profile_id", | ||
| on_delete=models.PROTECT, |
There was a problem hiding this comment.
I think this is okay because rule profiles should be archived not deleted in general.
| ) | ||
| uuid = immutable_uuid_field() | ||
|
|
||
| history = HistoricalRecords(excluded_fields=["scope_code"]) |
There was a problem hiding this comment.
I wonder if the history and the archived flag somehow conflict with each other.
Also I wonder about this immutability idea anyway: if this should be immutable why a history?
Maybe immutability is not a clear concept in the issue. This will need further clarification from the architect.
| # ============================================================================================== | ||
|
|
||
|
|
||
| def test_group_parent_cascade(tag: Tag) -> None: |
There was a problem hiding this comment.
Test names are bad. They should all state the expected behavior. I don't care if that makes them long. E.g. "test_criteria_group_deletion" is bad, while "test_delete_criteria_group_cascades_to_child_groups" is good.
There was a problem hiding this comment.
Here's some reading for proper naming of tests. https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices
|
|
||
| def test_group_parent_cascade(tag: Tag) -> None: | ||
| """ | ||
| Deleting a CompetencyCriteriaGroup cascades to any child group referencing it via `parent`: |
There was a problem hiding this comment.
At least some of the tests need to be a bit more integrative. In that: sure, we have tested that the cascade is there, but it's not clear why. There should be at least some tests that look at the actual bad outcome that we want to avoid: for example, do we suddenly have orphaned child groups that do not serve any purpose?
|
|
||
|
|
||
| # ============================================================================================== | ||
| # Transitive deletion tests required by #641's Deletions criteria: deleting an oel_tagging.Tag, |
There was a problem hiding this comment.
Too unclear. I have no patience to decipher what this comment means. Either it's clear at a glance or it's useless.
a6a8e76 to
5380a64
Compare
| 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. |
There was a problem hiding this comment.
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
| - 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. |
There was a problem hiding this comment.
Unreadable, unclear
| - ``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. | ||
| - 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. |
| 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. |
There was a problem hiding this comment.
| 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. |
There was a problem hiding this comment.
Unnecessary explanation
| ``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. |
There was a problem hiding this comment.
| ``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. |
There was a problem hiding this comment.
Unnecessary explanation
| CourseRun, | ||
| null=True, | ||
| blank=True, | ||
| on_delete=models.CASCADE, |
There was a problem hiding this comment.
Use models.SET() and archive. only if there are no learners connected
| CompetencyTaxonomy, | ||
| null=True, | ||
| blank=True, | ||
| on_delete=models.CASCADE, |
| # A new, unsaved instance: there's no persisted scope yet to compare against. | ||
| return | ||
| # Queried rather than compared against a value cached at load time, so a deferred load or | ||
| # a refresh_from_db() cannot bypass the check. `using` keeps a non-default-database |
There was a problem hiding this comment.
What are we talking about specifically with non-default-database instance? Why would there be such a thing?
| # a refresh_from_db() cannot bypass the check. `using` keeps a non-default-database | ||
| # instance from being compared against the wrong alias. | ||
| persisted_scope = ( | ||
| CompetencyRuleProfile.objects.using(self._state.db) |
There was a problem hiding this comment.
| CompetencyRuleProfile.objects.using(self._state.db) | |
| CompetencyRuleProfile.objects |
| persisted_scope = ( | ||
| CompetencyRuleProfile.objects.using(self._state.db) | ||
| .filter(pk=self.pk) | ||
| .values_list("organization_id", "course_id", "competency_taxonomy_id") |
There was a problem hiding this comment.
Better take all 3 values and compare them against all 3 current values
… PR branch The stack on jesperhodge/openedx-core (#10, #2, #3, #4, #5, #6, #7) is where the openedx#641 code is written and reviewed. This branch only carries the stack tip's tree so that openedx#800 has something to squash-merge. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
I reviewed the Stacked PRs here jesperhodge#10 and approved.
ormsbee
left a comment
There was a problem hiding this comment.
I've approved everything in the stack of reviews in your fork, so as long as all of that is reflected here, this is good to merge. Please just squash the commits appropriately.
9e7f363 to
b88ee49
Compare
Add the CBE criteria models: CompetencyRuleProfile, CompetencyCriteriaGroup and CompetencyCriterion (the criteria tree's AND/OR node and leaf), the rule payload contract and its validation, and CompetencyTaxonomy.taxonomy_overrides_org. Delete behavior cascades from tags, groups and object tags. Register django-simple-history, layer openedx_catalog beside openedx_content in the import contracts, and record the scope_code and on_delete decisions in ADR-0002. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The CBE criteria models use simple_history. Upstream's move to uv and pyproject.toml removed requirements/base.in, which declared dependencies before, so the dependency is declared in pyproject.toml and uv.lock. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
b88ee49 to
d7b9ea9
Compare
Strip the carriage returns from three lines of ADR-0002 that doc8 rejects, and keep ContainerType.__str__ as it is on main, including its pylint suppression, since the pylint version CI uses still reports E0307 without it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@ormsbee rebased, resolved conflicts, and fixed the lint errors. |
openedx#800 landed the conversion of cbe/models.py into a models/ package that this commit used to carry, so the commit is empty. It stays so that jesperhodge#16 keeps one commit ahead of its base and the stack's PR targets stay as they are. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Description
This PR adds the database models that let course authors define competency-based grading criteria: what a learner has to do, and how well, to be marked competent in a given skill. It adds three new tables, one new setting on an existing table, and the migrations to create them. It's going in now so the rest of the competency-based education work (issue #641) has a solid foundation to build on.
Implements #641 .
Closes #641 .
Supporting information
Note
Implemented by an AI agent (Claude Code), with a human directing the work and reviewing the
code and decisions.
Parent ticket of 641: #613
How to review this PR
Review the stack on my fork, in the order in the table below.. Do not review this PR directly.
The seven reviewable parts live on
jesperhodge/openedx-core,each targeting the part below it, with the bottom one targeting a branch identical to upstream
main.This PR exists so that there is something on
openedx/openedx-coreto merge once the stackis approved. Its commit history is a series of merges from the stack and carries no information of
its own.
scope_codeis computed in application code and is null while a profile is archived; theon_deletevalue of every competency foreign key.django-simple-history; layersopenedx_catalogin.importlinter.CompetencyTaxonomy.taxonomy_overrides_org.CompetencyCriteriaGroup, the criteria tree's AND/OR node.RuleType,GradeRulePayload,validate_rule_payload.CompetencyRuleProfileand the seeded system default.CompetencyCriterion, the tree's leaf, plus the tree-wide tests.The design rationale, and each place the implementation deviates from #641, are recorded in the ADR
amendments in part 1 and in each part's description. Review comments belong on the part that owns
the code.
This PR's tree is identical to the stack tip. This branch's tree is part 7's tree, the head of
jesperhodge/cbe-641-07-criterion:Every change to the stack is applied to this branch as well, so the two stay identical.
How to merge
commitlintcheck: A squash merge takes this PR's title as the commit message, so nothing non-conventional reachesmain.Other information
Alezconsultant/665 criterion endpoint jesperhodge/openedx-core#14 for [BE] Create CompetencyCriteria #665); their authors will rebase them onto the squash commit before
opening them here.
Testing
I've put a few code snippets here that the AI agents provided which allow some manual testing.
CI's tests job runs the unit test suite against MySQL 8. That run is the one that matters for the
scope_codeand deletion behavior, which differ between the two backends.Part 4 —
CompetencyCriteriaGroup, the criteria tree's AND/OR nodeBy hand, confirm the tree links work and that a parent delete cascades to its children:
Part 5 — the rule payload contract
Part 6 —
CompetencyRuleProfileand the seeded system defaultRun this suite against MySQL, not only SQLite. Issue #641 asks for it specifically, because the
scope_codeconstraint is the one thing SQLite cannot tell you the truth about:By hand, the archive-and-replace cycle is the most interesting behavior:
Part 7 —
CompetencyCriterion, the tree's leafBy 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: