feat: add mastery status lookup and learner competency status models - #802
feat: add mastery status lookup and learner competency status models#802jesperhodge wants to merge 1 commit into
Conversation
Implements the part of openedx#642 that does not depend on openedx#641: the shared CompetencyMasteryStatus lookup table and StudentCompetencyStatus, which holds one row per learner per competency. The leaf-level and group-level status models follow once openedx#641 lands, since their foreign keys point at tables openedx#641 creates. The three status values are seeded with pinned primary keys in rank order, lowest to highest. That is what makes the ordering visible to the database, so raising a learner's status can be written as one conditional UPDATE guarded by status_id < new_status_id, as ADR-0004 Decision 4 requires. It is also forced by the check constraint: MySQL does not allow a subquery in a CHECK, so the allow list has to compare status_id against literals. Two deliberate divergences from the acceptance criteria, both explained in the pull request: - created and modified use manual_date_time_field() rather than auto_now_add/auto_now. auto_now never fires on QuerySet.update(), which is the write path this ticket exists to enable, so it would leave the column stale exactly where it matters. - The model class is CompetencyMasteryStatus, singular, following Django convention and the same resolution openedx#641 applies to CompetencyCriterion. Every new foreign key is on_delete=PROTECT with a TODO(openedx#799) comment. That is a fail-closed placeholder, not a per-foreign-key decision; openedx#799 sets the real values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
openedx-webhooks
added
the
open-source-contribution
|
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. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. Where 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. |
2 participants
What this is
Part of #642. It adds the shared mastery status lookup table and the competency-level
learner status table, plus the two migrations that create and seed them.
This does not close #642. Two of that issue's three learner-status models have foreign
keys into tables #641 creates (
CompetencyCriteriaandCompetencyCriteriaGroup), and#641 has not merged, so those two models have nothing to point at. They follow in a second
pull request. The scope split is recorded as a comment on #642.
CompetencyMasteryStatuslookup modelStudentCompetencyCriteriaStatusStudentCompetencyStatusStudentCompetencyCriteriaGroupStatusThe whole-feature gates #642 carries (all ten indexes present,
make pii_checkat 100%across every model #613 adds, and migrations applying from scratch across all three merged
tickets) all need #641's tables to exist before they mean anything, so they are verified in
slice 2.
How the status ordering works
The three status values are seeded with pinned primary keys in rank order, lowest to
highest:
AttemptedNotDemonstratedis 1,PartiallyAttemptedis 2,Demonstratedis 3.A
MasteryStatusenum names them so no integer literal appears in model code.Pinning the order into the primary key is what makes it visible to the database. Raising a
learner's status is then a single statement,
UPDATE ... SET status_id = %s WHERE ... AND status_id < %s, with no join and no subquery. That is what ADR-0004 Decision 4 needs: aread, a comparison in Python, and a write would let two concurrent writers each read the
same old value, and the later write would then lower what the earlier one had raised.
Pinned ids are not just convenient here, they are forced. The constraint that keeps
AttemptedNotDemonstratedout ofStudentCompetencyStatushas to be a single-rowCHECK,because MySQL does not allow a subquery inside one. A single-row check can only compare the
row's own
status_idagainst literals, so stable ids are required either way, and aseparate
rankcolumn would be a second source of truth for the same ordering. It wouldalso be an extra column, which the acceptance criteria cap at ADR-0002 Decision 6's two.
The cost, stated plainly: a fourth status can be added above or below the existing three,
but not between them. If that is ever needed, a
rankcolumn can be added then andbackfilled from
id.Seeding a system-owned lookup row at a pinned id follows existing precedent in this repo,
src/openedx_tagging/migrations/0012_language_taxonomy.py.Deliberate divergences from the acceptance criteria
1.
createdandmodifiedusemanual_date_time_field(), notauto_now_add/auto_now.The criteria name the auto flags. They cannot work on this model.
auto_nowis applied byDateTimeField.pre_save, which only runs onModel.save();QuerySet.update()carriesonly the values passed to it. So
auto_now=Truewould leavemodifiedstale on exactlythe conditional-
UPDATEpath that ADR-0004 Decision 4 mandates and that this PR exists toenable. A field that looks automatic but silently is not is worse than one that makes the
caller pass a value.
manual_date_time_field()is also this repo's own convention for this shape of model. Itis used throughout the publishing and versioning core (
LearningPackage,PublishableEntity,PublishLog,DraftChangeLog,Content), where one logicaloperation writes many rows that should share one timestamp, and
auto_now_addis used inperipheral single-row models. Learner status writes are the former: ADR-0004 opens by
noting that one grade change updates the leaf and every row above it, for many learners at
once. Because Decision 3 commits each level separately and celery may retry, those rows are
written at several wall-clock times, so a caller-supplied timestamp is the only way one
cascade's rows carry one "as of" value.
The field names are unchanged.
createdandmodifiedare what OEP-38 mandates and whatboth this repo and openedx-platform use.
2. The model class is
CompetencyMasteryStatus, singular.#642, #613 and ADR-0002 all write
CompetencyMasteryStatuses. Django models are singular byconvention, and #641 already applies this same resolution within this ticket family, naming
its class
CompetencyCriterionfor theCompetencyCriteriatable. Table naming follows#640's precedent, letting Django derive it.
3. Field names are idiomatic Django, so index 8's columns differ from the ADR's spelling.
ADR-0002 writes the foreign key columns as
oel_tagging_tag_idandstatus_id. The fieldshere are
tagandstatus, matchingObjectTag.tagin this repo, so index 8 lands on(user_id, tag_id)rather than(user_id, oel_tagging_tag_id). Reviewers checking thatcriterion literally should not read this as a miss.
Deletion behaviour
Every new foreign key is
on_delete=PROTECTwith aTODO(#799)comment, per #642'sinstruction. This is a fail-closed placeholder, not a per-foreign-key decision, and there
are no delete tests. #799 sets the real values.
One gap worth knowing about, which I have also raised on #799: that issue's criteria scope
themselves to "FKs from
Student*Statusmodels to definition models", but these models alsocarry foreign keys to the user model and to the status lookup table, which are not
definition models and which no criterion there currently covers.
Verification
Run from this branch against the repo's own tooling. Everything below passed:
pytest tests/openedx_learning(SQLite)pytest tests/openedx_learning(MySQL 8.4)makemigrations openedx_learning --check --dry-runpylint,pycodestyle,pydocstyle,isort,mypylint-importsNothing was suppressed to get there: no
# noqa,# pylint: disable, or# type: ignorewas added.
The MySQL run matters here, because the check constraint is the centre of this change and a
green SQLite suite is not evidence for it. Inspecting the schema MySQL actually built:
Two pre-existing failures on
mainare unrelated to this branch and are not addressed here:code_annotations --lintreportsopenedx_content.Draftandopenedx_content.PublishableEntityVersionas both annotated and safelisted, andpydocstylereports a missing package docstring on
tests/openedx_learning/__init__.py. Both reproduceon an untouched checkout. The two models added here carry inline
.. no_pii:annotations andneither appears in the uncovered list.
Tests
Eleven behaviours in
tests/openedx_learning/applets/cbe/test_mastery.py: the seed'scontents and rank order; uniqueness of
status; the conditional raise being a no-op againsta higher stored value and effective against a lower one, asserted on the row count returned
by
update(); rejection ofAttemptedNotDemonstratedoncreate(),bulk_create()andQuerySet.update(), since none of the last two callclean(); acceptance of the twopermitted values; the one-row-per-learner-and-competency constraint;
createdandmodifiedbeing required and UTC-validated; a conditional raise carrying
modifiedwithout touchingcreated; and the absence of any history package.Admin
Both models get a bare-bones page subclassing
ReadOnlyModelAdminfromopenedx_django_lib.admin_utils, whose docstring is the standing instruction to do sorather than subclass
ModelAdmindirectly. Read-only is also right on the merits: thelookup table is immutable configuration per ADR-0002 Decision 6.1, and an editable
StudentCompetencyStatuspage would be the staff-correction path, which ADR-0004 Decision 6requires to take a row lock and recompute every ancestor. None of that exists yet.
Merge order
#642 is meant to merge after #641, and #641 is in progress with its own
0002and0003migrations in this same app. This branch will need a rebase and a renumber to
0004/0005once #641 lands. Docstrings refer to the seed migration by name rather than number so that
renumbering does not leave stale references. #641 also converts
applets/cbe/models.pyinto amodels/package; slice 2 moves these models into it asmodels/mastery.py, which needs no migration since a model's table name comes from its applabel and class name, not its module path.
🤖 Generated with Claude Code