Repository navigation
feat: record competency status in the same save as a subsection grade - #39179
alezconsultant wants to merge 1 commit into
Conversation
When a learner's subsection grade is persisted, the LMS now also updates the learner's competency criterion status in the same database transaction, using record_graded_object_statuses from openedx-core. The grade and the status commit or roll back together, so a learner is never shown a grade without the mastery it earned. The behavior is gated by the new ENABLE_COMPETENCY_MASTERY_TRACKING setting, which defaults to False. Related to openedx/openedx-core#701 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. 🔘 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. |
tbain
left a comment
There was a problem hiding this comment.
Looks good to me, and Codex review didn't raise any actual concerns.
Raw Codex output:
Details
Verdict: request changes on PR [[#39179](https://github.com//pull/39179)](https://github.com//pull/39179), reviewed against [[ticket #701](https://github.com/openedx/openedx-core/issues/701)](https://github.com/openedx/openedx-core/issues/701).-
Bug.
[subsection_grade.py:15](https://github.com/openedx/openedx-platform/blob/9e0b6d447fbc496d8ae927918f079621ee8ba299/lms/djangoapps/grades/subsection_grade.py#L15)and[tasks.py:18](https://github.com/openedx/openedx-platform/blob/9e0b6d447fbc496d8ae927918f079621ee8ba299/lms/djangoapps/grades/tasks.py#L18)unconditionally import APIs absent from the pinnedopenedx-core. The feature flag cannot protect these imports, so LMS/Studio imports fail until the compatible core release and dependency pin land together. -
Bug.
[subsection_grade.py:334](https://github.com/openedx/openedx-platform/blob/9e0b6d447fbc496d8ae927918f079621ee8ba299/lms/djangoapps/grades/subsection_grade.py#L334)and[:361](https://github.com/openedx/openedx-platform/blob/9e0b6d447fbc496d8ae927918f079621ee8ba299/lms/djangoapps/grades/subsection_grade.py#L361)usetransaction.on_commit(..., robust=True). If publishing the roll-up task fails after the grade/status commit, the exception is swallowed and no retry or durable recovery occurs; higher-level competency statuses can remain stale indefinitely.
Additional review notes: AI attribution is present. The PR provides automated test commands but no manual testing steps. I did not rerun tests because the workspace is not checked out at the PR commit and requires the unreleased openedx-core APIs.
I think we can disregard the first bug, this ticket depends on upstream tickets being merged in so that's a known issue. Can you expand on the second bug, though, and provide a simple suggestion for how to address it?
Yes—setting aside the upstream dependency issue, the second concern is about losing the roll-up task.
The grade and criterion status are committed first. Then the on_commit callback calls apply_async():
transaction.on_commit(
lambda: _enqueue_competency_rollup(student.id, object_ids),
robust=True,
)If the broker is unavailable:
- The grade and criterion status commit successfully.
- Task publication fails.
robust=Truecauses Django to log and suppress the exception.- The roll-up task is discarded with no retry.
The learner may therefore have an updated criterion status but stale higher-level competency statuses. The test test_single_path_enqueue_failure_does_not_skip_event_or_raise currently codifies this behavior.
Simple suggestion: record a pending roll-up request in the same database transaction, then have a retryable worker dispatch those requests to Celery. That preserves both requirements: the grade write is not rolled back because of a broker outage, and the roll-up cannot be silently lost.
Simply removing robust=True would expose the failure, but would not reliably recover the task and could make callers see an error after the grade already committed.
If we have a test named test_single_path_enqueue_failure_does_not_skip_event_or_raise the codifies this behavior, does that mean it should be expected? What AC are you looking at that makes you think this is a bug?
Yes. Given that test, enqueue failure is an intentional behavior: the grade/status save succeeds, and a broker failure does not propagate to the caller.
The AC I was relying on was:
“competency status storage failure rolls back the grade and tells the caller failure.”
But that concerns database/storage failure, not failure to enqueue the post-commit task. The other ACs require criterion statuses to be updated in the same grade operation; they do not require reliable delivery of the follow-up roll-up task.
So I’d withdraw this as a PR bug. At most, it is a separate operational concern: a broker outage could leave higher-level aggregates stale. Addressing that would require a new reliability requirement and likely an outbox/retry mechanism.
Description
Recording a subsection grade used to save the grade alone. This PR makes the learner's competency criterion status part of the same save, using
record_graded_object_statusesfromopenedx-core. The grade and the status are written in one transaction, so they commit or roll back together.It covers both paths that persist a subsection grade:
CreateSubsectionGrade.update_or_create_model(one subsection) andCreateSubsectionGrade.bulk_create_models(many subsections for one learner). Computing the levels above the criterion belongs toroll_up_competency_statusesinopenedx-core; this PR only queues the task that calls it.What changes:
transaction.atomic(). The competency call is not wrapped intry/except, so a storage failure fails the grade write and the caller sees the failure.openedx-coreisDecimal(str(percent_graded))clamped to[0, 1]. On the single path it is read after the staff override has been applied, so a learner whose instructor raised their grade is credited. Subsections with no graded points available, or with no graded attempt, are not passed.transaction.on_commit(..., robust=True)queues the new taskroll_up_competency_statuses_for_user. It is queued only after the grade has committed. The bulk path queues one task for the whole batch. The task follows the shape ofrecalculate_subsection_grade_v3and is routed to the same queue.PersistentSubsectionGrade.update_or_create_gradeandbulk_create_grades.CreateSubsectionGradeemits it after the atomic block, so it is never sent for a grade that was rolled back. Those two model methods have no other production callers.ENABLE_COMPETENCY_MASTERY_TRACKING(defaultFalse) gates the competency call and the enqueue. With it off, no competency code runs and no task is queued.Behavior to be aware of
openedx-core.subsection_grade.pyimportsGradedObjectScoreandrecord_graded_object_statusesfromopenedx_learning.apiat module level. The pinnedopenedx-core==1.4.0does not contain them, so the grades module fails to import until the pin is bumped to a release that does. The pin bump is not part of this PR, and this PR stays a draft until it is. The roll-up functionroll_up_competency_statusesmust also exist in that release.ATOMIC_REQUESTSthat block is a savepoint, so on request paths the event fires before the outer commit, exactly as it did before this change.Supporting information
Related to openedx/openedx-core#701
Testing instructions
Automated (run in the Tutor devstack LMS container with
openedx-coremounted from the branch that contains the competency function):pytest lms/djangoapps/grades(the whole app).ruff check lms/envs/common.py lms/envs/production.py lms/djangoapps/grades.Additional testing can be done when openedx/openedx-core#603 will be merged.
🤖 Generated with Claude Code