Repository navigation
feat: add student status models for competency criteria and groups - #854
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.
I have just a couple of small requests related to tests that I should have included the last round, but missed. Once those are done, feel free to squash, and I'll approve and merge. Thank you.
Thank you.
Claude drafted the first pass of this review and ran the git range-diff, test suite, mypy, import-linter and makemigrations --check on the branch for me.
| """ | ||
| #642 acceptance-criteria tests for StudentCompetencyCriterionStatus that test_criterion_status.py | ||
| does not already cover. | ||
| """ |
There was a problem hiding this comment.
Please fold this module into test_criterion_status.py, and likewise the group one into test_criteria_group_status.py. "The acceptance criteria that the other file doesn't already cover" isn't a boundary the next person adding a test can apply, and it's already drifting: both files define the same criterion fixture, and a few cases here are within a line or two of ones over there (test_group_at_depth_delete_is_protected_by_a_criterion_status_beneath_it vs. test_root_group_delete_is_protected_by_a_criterion_status_two_levels_down, test_oel_tagging_taxonomy_delete_is_protected_by_a_criterion_status_beneath_its_tags vs. test_taxonomy_delete_is_protected_by_a_criterion_status, and test_criterion_status_accepts_every_status_value vs. test_attempted_not_demonstrated_is_accepted). The "deleting succeeds when nothing is beneath it" cases and the lower-over-higher one are worth keeping. I just want one module per model.
| assert list(rows.values_list("pk", "status_id")) == [(row.pk, MasteryStatus.DEMONSTRATED)] | ||
|
|
||
|
|
||
| def test_criterion_status_carries_created_and_modified() -> None: |
There was a problem hiding this comment.
Nit: What does this tell us that the rest of the module doesn't, given that every write in it passes created and modified? I'd drop this one, test_group_status_carries_created_and_modified in the group file, and test_criterion_status_migration_declares_the_swappable_user_dependency below. The group file already lost its migration-dependency twin after my last round, and I don't want to encourage tests that assert what a migration file says.
Add the last two of openedx#642's four learner-status models. StudentCompetencyCriterionStatus holds a learner's mastery status for one leaf criterion, and StudentCompetencyCriteriaGroupStatus holds it for one criteria group. Each holds one row per learner and criterion, or per learner and group, updated in place, and accepts all three status values. Deleting a user deletes their status rows, and a criterion, group, object tag, tag or taxonomy with status rows beneath it cannot be deleted. Closes openedx#642. Co-authored-by: Jesper Hodge <jhodge@unicon.net>
29f9bd3 to
e03180e
Compare
|
Thanks @ormsbee - I addressed both comments and squashed. |
Description
This PR adds the last two of #642's four models.
StudentCompetencyCriterionStatusrecords a learner's current mastery status for one leaf criterion, andStudentCompetencyCriteriaGroupStatusdoes the same for one criteria group. Each holds one row per learner and node, updated in place, and points at theCompetencyMasteryStatuslookup table added in #852. UnlikeStudentCompetencyStatus, both accept all three status values, includingAttemptedNotDemonstrated.This PR replaces Jesper's stack jesperhodge#20, jesperhodge#21 and jesperhodge#22. Jesper is out sick, and I'm taking over #642 so it can merge.
Supporting information
Note
Implemented with an AI agent (Claude Code), with a human directing the work and reviewing the code and decisions.
Deviation from the acceptance criteria
#642 and #613 say
createdusesauto_now_add=Trueandmodifiedusesauto_now=True. Both models usemanual_date_time_field()instead, the same asStudentCompetencyStatusin #852.auto_nowdoes not fire onQuerySet.update(), so it would leavemodifiedstale on the single conditionalUPDATEthat raises a status. #642's Open Questions already records this divergence.How to review this PR
The first 11 commits are Jesper's commits from jesperhodge#20 through jesperhodge#22, rebased onto
mainwith no conflicts. Their SHAs changed because #852 was squash-merged, butgit range-diffshows every one of them is patch-identical to the version reviewed on the fork, which ended at5e9c2df. @tbain approved jesperhodge#21 and jesperhodge#22 there.The last two commits address @ormsbee's review on the fork, so they are the only new code to review:
57d55b629f9bd3StudentCompetencyCriteriaStatusto the singularStudentCompetencyCriterionStatus, along with its migration and unique constraint. Its table is nowopenedx_learning_studentcompetencycriterionstatus.Migration numbering
This PR's migrations are
0011_studentcompetencycriterionstatusand0012_studentcompetencycriteriagroupstatus, which follow #852's0010onmain. #847 adds anopenedx_learningmigration too, so whichever of the two merges second renumbers its migrations after the other.Upgrade note for a local database
If your development database already applied migrations
0011and0012from Jesper's branch, migrateopenedx_learningback to0010on that code, or reset the database, before switching to this branch. Migration0011has a new name and creates a renamed table.Testing instructions
Confirm there is no migration drift:
The snippet below covers both models' main behavior. Run
python manage.py migrateon this branch, then paste the snippet intopython manage.py shell. The lines commented with an error are expected to raise, so in IPython, which stops a pasted block at its first exception, paste one block at a time.Checks run
tox -e django52): 973 passed.tox -e qualityandtox -e docs: both pass.0010, and forward again: both migrations apply and unapply cleanly.🤖 Generated with Claude Code