Repository navigation
feat: roll up learner competency statuses above a graded criterion - #870
alezconsultant wants to merge 2 commits into
Conversation
Add roll_up_competency_statuses, which recomputes every learner status above the criteria attached to the given object ids: each criteria group up to the root, the competency at the root, and every parent competency up to the top of the taxonomy. It runs in autocommit, takes no lock, and every write only raises a stored status, so concurrent, repeated and out-of-order runs for the same learner converge on the right values. Related to openedx#643 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for the pull request, @alezconsultant! 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. |
tbain
left a comment
There was a problem hiding this comment.
Human questions (Some of these are from Mary):
- (Comment cleanup - please clarify) For line 2 of rollup.py, what is meant by "changed criteria"?
- (Question) For line 50 of rollup.py, is it technically possible to get to this line? Is this a requirement of the AC? Probably okay to leave as a safe default for some unforeseen logic branch, but curious if this can even be gotten to, and if we can clean this up.
- (Please add comment) It might be good to add a comment above line 51 of rollup.py indicating that this is covering the case of LogicOperator.OR (or use an "else").
- (Comment cleanup - please clarify) I don't understand the comment on lines 54-65 in rollup.py.
- (Question, just to ensure this) Does DEMONSTRATED and partially_ATTEMPTED have a numerical value where max makes sense to use (e.g. line 67 of rollup.py)?
- (Question) rollup.py:154 - is it necessary to go through the entire array if we find a value that matches the "best" status, or can it break early if we get DEMONSTRATED for a given tag?
- (Comment cleanup - please clarify) rollyup.py:187 "# A lineage ends with a tab..." comment - what does that mean? we're not testing for a \t or anything? Is this comment out of date or superfluous?
- (Nitpick) test_tag_rollup.py:196 - the assert statement for
test_a_sub_competency_with_criteria_only_further_down_still_participates()seems overly concise, it makes the logic hard to understand at a glance. Could this just be made into 3 separate assert lines instead? - (Nitpick) test_tag_rollup.py:232
test_the_deepest_allowed_taxonomy_is_walked_to_the_top()has the same overly concise one-liner assert; could we make this 6 separate assert lines instead to make it easier to understand at a glance? - (Nitpick) test_tag_rollup.py:297,
test_competencies_graded_at_different_depths_converge_on_the_shared_ancestor()same overly concise format for assertions
Additionally, Codex raised these two bugs:
- Bug.
[rollup.py:41-55](https://github.com/openedx/openedx-core/blob/c1b178827389f64f9235d12e9761ec2e5145095b/src/openedx_learning/applets/cbe/rollup.py#L41-L55),[rollup.py:84-102](https://github.com/openedx/openedx-core/blob/c1b178827389f64f9235d12e9761ec2e5145095b/src/openedx_learning/applets/cbe/rollup.py#L84-L102): the implementation ignores each child’sorderingand always reads/materializes every sibling status. This does not implement the ADR/ticket’s required ordered short-circuit evaluation. For large groups, an already-determinedAND/ORresult still performs a full sibling scan. Preserve ordering and short-circuit status reads, with a regression test proving later siblings are not read.
This feels like something that needs to be addressed for performance concerns
- Suggestion.
[0002-competency-criteria-model.rst:252-268](https://github.com/openedx/openedx-core/blob/c1b178827389f64f9235d12e9761ec2e5145095b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst#L252-L268): the PR adds statuses for parent hierarchy tags and parent rollup semantics but does not amend Decision 6 as requested by the ticket, leaving the architectural documentation stale.
Sounds like a good thing to address!
Raw Codex output:
Details
Review completed against ticket #643 and PR #870 at head `c1b1788` (base `a58d833`). I found one actionable issue.-
Bug.
[rollup.py:41-55](https://github.com/openedx/openedx-core/blob/c1b178827389f64f9235d12e9761ec2e5145095b/src/openedx_learning/applets/cbe/rollup.py#L41-L55),[rollup.py:84-102](https://github.com/openedx/openedx-core/blob/c1b178827389f64f9235d12e9761ec2e5145095b/src/openedx_learning/applets/cbe/rollup.py#L84-L102): the implementation ignores each child’sorderingand always reads/materializes every sibling status. This does not implement the ADR/ticket’s required ordered short-circuit evaluation. For large groups, an already-determinedAND/ORresult still performs a full sibling scan. Preserve ordering and short-circuit status reads, with a regression test proving later siblings are not read. -
Suggestion.
[0002-competency-criteria-model.rst:252-268](https://github.com/openedx/openedx-core/blob/c1b178827389f64f9235d12e9761ec2e5145095b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst#L252-L268): the PR adds statuses for parent hierarchy tags and parent rollup semantics but does not amend Decision 6 as requested by the ticket, leaving the architectural documentation stale.
Verification:
- 81 targeted tests passed.
pycodestyle,pydocstyle,pylint, and targetedmypypassed.- No migrations were added.
- The PR includes AI attribution and manual testing steps.
c1b1788 to
6752464
Compare
8-10. Tests fixed. As for the bug and the suggestion. I ran into two issues with ordering. First, only |
feat: roll up learner competency statuses above a graded criterion
Summary
Adds
roll_up_competency_statuses(*, user_id, object_ids)to the CBE applet and exports it fromopenedx_learning.api. After a grade has committed, it recomputes every learner status above the criteria attached toobject_ids:StudentCompetencyStatusat the root group.The function is meant to be called from the Celery task that runs after the grade transaction commits.
How it works
ANDis Demonstrated when every child is,AttemptedNotDemonstratedas soon as any child is, and otherwisePartiallyAttemptedif any child has a status.ORis Demonstrated as soon as any child is,AttemptedNotDemonstratedonly when every child is, and otherwisePartiallyAttempted. A null operator behaves asOR. A group with nothing attempted beneath it gets no row.AttemptedNotDemonstratedis written asPartiallyAttempted, becauseStudentCompetencyStatusholds onlyPartiallyAttemptedandDemonstrated.Demonstratedwhen every one is,PartiallyAttemptedwhen any has a status). A sub-competency with no root criteria group anywhere beneath it can never be demonstrated, so it is logged at warning level with its parent and never raises.Manual Testing
Manual testing via the Django shell in the devstack,
docker exec -it -e PYTHONPATH=/mnt/openedx-core/src tutor_dev-cms-1 ./manage.py cms shell:Related to #643
🤖 Generated with Claude Code