Repository navigation
feat: add CompetencyMasteryStatus and StudentCompetencyStatus models - #852
Conversation
|
Thanks for the pull request, @mgwozdz-unicon! 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. |
ormsbee
left a comment
There was a problem hiding this comment.
Sorry, one tiny, tiny thing, and then I'll merge. Thank you.
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython(forward, migrations.RunPython.noop), |
There was a problem hiding this comment.
Sorry, I mis-communicated this. When I made the nit about how everything could just be deleted, I meant it could be:
CompetencyMasteryStatus = apps.get_model("openedx_learning", "CompetencyMasteryStatus")
CompetencyMasteryStatus.objects.delete()Since this is the only thing that adds any rows to that model, as the model was just created. Doing a noop is technically incorrect here, since if I rewind to migration 0008, I would expect to have an empty table, and this noop would mean that the seeded values would be there. It is very unlikely that anyone will ever run into this situation, but please restore a deletion of some sort.
There was a problem hiding this comment.
Got it, restored the deletion so rewinding to 0008 leaves an empty table (ae491fa).
|
@mgwozdz-unicon: Happy to merge this. Please squash the commits and give it a good summary commit message? Thank you. |
Add two of openedx#642's four learner-status models. CompetencyMasteryStatus is the mastery-rank lookup table, seeded with ids 10, 20 and 30 so the database can compare ranks directly. StudentCompetencyStatus holds one row per learner and competency tag, updated in place, and a check constraint keeps AttemptedNotDemonstrated out of it. Deleting a user deletes their status rows, and a tag with status rows cannot be deleted. Part of openedx#642. Co-authored-by: Jesper Hodge <jhodge@unicon.net>
ae491fa to
7bc5dd4
Compare
|
@ormsbee This is ready for merge. Thank you very much! |
Description
This PR adds two of #642's four models.
CompetencyMasteryStatusis the lookup table of the three mastery ranks (AttemptedNotDemonstrated,PartiallyAttempted,Demonstrated).StudentCompetencyStatusrecords a learner's current mastery status for one competency (aTag), with one row per learner per tag, updated in place rather than appended.This PR replaces #830. Jesper is out sick, and I'm taking over #642 so it can merge.
Supporting information
StudentCompetencyCriteriaStatusandStudentCompetencyCriteriaGroupStatus, are in StudentCompetencyCriteriaStatus and StudentCompetencyCriteriaGroupStatus data models jesperhodge/openedx-core#20, feat: add StudentCompetencyCriteriaStatus for per-criterion mastery jesperhodge/openedx-core#21 and feat: add StudentCompetencyCriteriaGroupStatus for per-group mastery jesperhodge/openedx-core#22. I will open them as a separate follow-up PR once this one merges.Note
Implemented with an AI agent (Claude Code), with a human directing the work and reviewing the code and decisions.
How to review this PR
Every commit up to and including
f3226fbis unchanged from jesperhodge#17 and jesperhodge#18, with the same SHAs. That code was reviewed there: @tbain approved both, and @ormsbee's review is addressed by the commits afterf3226fb, so those are the only new code to review. In the Commits tab, they are:8aaa5783f30ab40008drops the table anyway. Replaced byae491fabelow.29dde41CompetencyMasteryStatususes aSmallAutoFieldprimary key, so every learner row's foreign key is 2 bytes instead of 8.1e46c44f18c12fStudentCompetencyStatusrejectsAttemptedNotDemonstrated. A learner can still demonstrate a competency later in another course, so a top-level row never records "not demonstrated".ae491fa0008leaves an empty table, as0008created it.The rest of the review comments (the tag-taxonomy check and the composite indexes) didn't need code changes in this PR, and I've replied to each of them on the fork PRs.
Migration numbering
This PR's migrations are
0008_competency_mastery_status,0009_seed_competency_mastery_statusesand0010_studentcompetencystatus, which follow0007onmain. #847 also adds anopenedx_learningmigration0008. Whichever of the two merges second renumbers its migrations after the other, because Django rejects a migration graph with two leaf nodes.Upgrade note for a local database
If your development database already applied migrations
0008to0010from Jesper's branch, migrateopenedx_learningback to0007on that code, or reset the database, before switching to this branch. The migration names are unchanged, but the seeded ids and the check constraint are not, so Django would not re-run them.Testing instructions
Confirm there is no migration drift:
By hand, the two behaviors most worth confirming are the "one row per learner and tag" constraint and that a
Tagin use cannot be deleted out from under a recorded status:CI's MySQL job is the one that matters for the check constraint above. SQLite enforces it too, but MySQL is the backend #642 was written against.
Checks run
tox -e django52): 939 passed. The suite also covers the check constraint onbulk_create()andQuerySet.update(), which skip model validation.tox -e quality,tox -e docsandtox -e package: all pass.0008(table empty), back to0007(table gone), and forward again: all three migrations apply and unapply cleanly, and the seeded ids are 10, 20 and 30.🤖 Generated with Claude Code