[alezconsultant]

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_statuses from openedx-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) and CreateSubsectionGrade.bulk_create_models (many subsections for one learner). Computing the levels above the criterion belongs to roll_up_competency_statuses in openedx-core; this PR only queues the task that calls it.

What changes:

• Transaction. Both methods wrap the grade write and the competency call in transaction.atomic(). The competency call is not wrapped in try/except, so a storage failure fails the grade write and the caller sees the failure.
• Fraction. The score passed to openedx-core is Decimal(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 are not passed.
• Staff-entered grades without an attempt. A grade that staff enter in the gradebook counts toward competency status even when the learner never attempted the subsection: a graded override (earned_graded_override set) is treated as an attempt, the same way the gradebook already treats attempted_graded or override. Subsections with neither an attempt nor a graded override are not passed.
• Roll-up task. When the competency call reports that at least one status changed, transaction.on_commit(..., robust=True) queues the new task roll_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 of recalculate_subsection_grade_v3 and is routed to the same queue.
• Failed enqueue. If the task can't be queued, for example because the broker is down, the failure is logged at error level with the learner's user id and the object ids, so support knows whose statuses to recompute. The grade and its event are unaffected. A lost roll-up leaves a status stale rather than wrong, because the roll-up only ever raises a status.
• Grade-calculated event. The event is no longer emitted from PersistentSubsectionGrade.update_or_create_grade and bulk_create_grades. CreateSubsectionGrade emits 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.
• Setting. ENABLE_COMPETENCY_MASTERY_TRACKING (default False) gates the competency call and the enqueue. With it off, no competency code runs and no task is queued.
• Dependency. openedx-core moves from 1.20.0 to 1.21.0 in uv.lock, requirements/edx/base.txt and requirements/edx/development.txt, regenerated with make upgrade-package package=openedx-core. No other package moves. 1.21.0 is the first release that contains both feat: evaluate and record a learner's leaf competency status for a grade openedx-core#862 (GradedObjectScore and record_graded_object_statuses, released in 1.13.0) and feat: roll up learner competency statuses above a graded criterion openedx-core#870 (roll_up_competency_statuses), which this PR imports at module level.

Behavior to be aware of

• Event timing. The event fires after the atomic block exits. Under ATOMIC_REQUESTS that block is a savepoint, so on request paths the event fires before the outer commit, exactly as it did before this change.
• Gradebook. The gradebook bulk update runs the recalculation task synchronously and the task retries on failure, so a competency storage failure there rolls back the grade recalculation while the endpoint still returns 202. This is how the endpoint already treats any recalculation failure.

Tests

• The unit tests in test_subsection_grade.py mock openedx-core and cover each behavior above, including a staff override for a learner who never attempted the subsection, and the error log, with its user and object ids, when the enqueue fails.
• SubsectionGradeCompetencyIntegrationTest runs against real openedx-core for the three scenarios that exercise the wiring between the two repositories: a grade that meets the threshold, a staff override that raises a failing grade, and a status write failure that rolls the grade back. It creates the competency and criterion through openedx-core's public API (create_competency_taxonomy, add_tag_to_taxonomy, associate_competency_criterion) and imports models only from openedx_learning.models_api, so a schema change inside openedx-core doesn't break this repository's tests. The test database is built without migrations, so it seeds the mastery-status rows itself.
• The gradebook test posts a staff-entered grade for a learner with no attempt and checks, through the real graded_object_score(), that a status is recorded.

Supporting information

Related to openedx/openedx-core#701. The openedx-core side is openedx/openedx-core#862 (recording a learner's leaf status) and openedx/openedx-core#870 (the roll-up), both merged and released in openedx-core 1.21.0.

Testing instructions

Automated, in the Tutor devstack LMS container built from this branch, so that the pinned openedx-core 1.21.0 is installed:

1. pytest --ds=lms.envs.test lms/djangoapps/grades (the whole app).
2. ruff check lms/envs/common.py lms/envs/production.py lms/djangoapps/grades.
3. pylint lms/djangoapps/grades/subsection_grade.py lms/djangoapps/grades/tasks.py.
4. lint-imports.

🤖 Generated with Claude Code

[openedx-webhooks]

Thanks for the pull request, @alezconsultant!

This repository is currently maintained by @openedx/wg-maintenance-openedx-platform-oncall.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

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:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

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:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[tbain]

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 pinned openedx-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) use transaction.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:

1. The grade and criterion status commit successfully.
2. Task publication fails.
3. robust=True causes Django to log and suppress the exception.
4. 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.

[ormsbee]

I know @tbain already went through this. Most of what I have comes from reading it next to the openedx-core side (openedx/openedx-core#862 for the status write, and now openedx/openedx-core#870 for the rollup), so it's mostly about the seams between the two repos. The grades-side changes themselves look right to me: the override-adjusted fraction, moving the event out from under the transaction, and the on-commit enqueue are all what openedx/openedx-core#701 asked for, and the tests cover the acceptance criteria well.

Things I'd like addressed before this comes out of draft:

1. Please fix the model name in test_subsection_grade.py (StudentCompetencyCriterionStatus is what merged onto main), and switch that import to openedx_learning.models_api once openedx/openedx-core#862 exposes the models there, which I've asked for in my review on openedx/openedx-core#862. Details inline.
2. Please log the learner and object IDs when the rollup enqueue fails, since robust=True otherwise leaves support with nothing to recompute from (inline on _enqueue_competency_rollup).
3. Please fix the openedx-core link in the PR description: openedx/openedx-core#603 is a dependabot PR, and I think you mean openedx/openedx-core#862 and openedx/openedx-core#870. Once we know which openedx-core release this needs, that should go in the commit body too.

There's also a question about staff-entered grades for learners who never attempted the subsection (inline on graded_object_score()). Code changes for this are not blocking, but I'd like an answer before this goes out.

On dependencies: openedx/openedx-core#870 doesn't build on openedx/openedx-core#862, so the pin bump will need a release that contains both. Claude checked that the roll_up_competency_statuses(*, user_id, object_ids) signature in openedx/openedx-core#870 matches what tasks.py calls, and that its "caller must not be inside a transaction" rule is honored by the task here. Now that openedx/openedx-core#870 is up, I'm fine with this PR waiting on it rather than splitting the rollup half out; if openedx/openedx-core#870 turns into a long review, we can revisit.

FWIW, CI hasn't actually exercised any of the new tests yet: every unit-test shard was cancelled or failed at collection on the openedx_learning.api import, and the import-linter failure is a real contract violation that will outlive the pin bump. So we'll want a green run against a real openedx-core release before this merges, and I'd like to see it after (1), since the end-to-end tests here can't have run against what's on main.

Thank you.

---

Claude drafted the first pass of this review for me and did the cross-checking against openedx-core main and the openedx/openedx-core#862 and openedx/openedx-core#870 branches, plus the CI logs. It couldn't run the platform test suite locally, since that needs the unreleased openedx-core and a Mongo instance.

[ormsbee]

Two problems with this import, the first of which is what's failing the "Lint Python Imports" check:

1. openedx_learning.models isn't public from this repo's point of view. The import-linter contract in pyproject.toml lists openedx_learning as an isolated app, which only allows api, models_api, data, and tests, and the CI job names this line. That's independent of the version pin, so it will still fail after the bump. The sanctioned path is openedx_learning.models_api, which today only exports CompetencyTaxonomy. I've asked on feat: evaluate and record a learner's leaf competency status for a grade openedx-core#862 for the models this test needs to be exported from there, so please switch this import over once that lands.
2. StudentCompetencyCriteriaStatus is the old name. I asked for it to become StudentCompetencyCriterionStatus during review of feat: add student status models for competency criteria and groups openedx-core#854, and that's what's on main.

For (1), one more thing: I do want at least one end-to-end test in this repo, since the mocked tests can't catch the two sides drifting apart. That being said, I'd rather not have platform tests assembling a criteria tree out of five raw models plus a hand-seeded CompetencyMasteryStatus table, because every schema change in openedx-core will then break this repo's build (which is more or less what happened with the rename). So please keep these to the few scenarios that exercise the wiring (threshold met, the override one, the rollback one), and once there's a real API for creating criteria (openedx/openedx-core#847 is headed there), switch the raw model setup to it.

[ormsbee]

Please catch the publish failure here and log the user ID and object IDs at error level, so that the log line is something support can act on. Django's own robust handling logs "Error calling CreateSubsectionGrade.update_or_create_model.. in on_commit() during transaction" plus the traceback (docs; Claude pulled the message text from the Django 5.2.17 source), which says that a rollup was lost, but not whose. A task that fails after it's been picked up gets persisted in FailedTask by LoggedPersistOnFailureTask, kwargs and all (edx-celeryutils), but a task that never made it to the broker leaves nothing, and the only recovery is the operator recompute in openedx/openedx-core#774, which needs to know which learner to recompute. I'm fine with "log and move on" as the policy (the rollup only ever raises a status, so a lost one is stale rather than wrong); I just want the message to carry the IDs. Maybe something like:

    try:
        roll_up_competency_statuses_for_user.apply_async(kwargs={"user_id": user_id, "object_ids": object_ids})
    except Exception:  # pylint: disable=broad-except
        log.exception(
            "Could not queue competency rollup for user %s and objects %s; recompute manually.",
            user_id, object_ids,
        )

With that in place robust=True becomes a belt-and-suspenders thing, which is fine to leave.

[ormsbee]

Question: what should happen for a grade that staff enter through the gradebook for a learner who never attempted the subsection? Reading the code, first_attempted on the row stays None in that case (update_or_create_override() doesn't touch it, and _aggregated_score_from_model() copies it from the row), so attempted_graded is False here and no status gets recorded, even though the override does count toward the learner's course grade. openedx/openedx-core#701 only says to skip unattempted objects, and its staff-correction scenario starts from a learner who scored below the threshold, so I'm not sure whether "staff entered a score" was meant to count as attempted. It seems like exactly the case CBE folks will hit early (an instructor recording an observed demonstration), so I'd like to know what the intended behavior is before this goes out. If it should count, I think the change is small (treat an override with earned_graded_override set as attempted), but I'm not sure that would reflect the product intent. @mgwozdz-unicon, do you know?

[mgwozdz-unicon]

Yes, we want scores manually entered by instructors to count even if the LMS has no record of the learner having attempted the subsection.

[ormsbee]

Related to my question on graded_object_score(): patching this hides the fact that the real gradebook path for a never-attempted learner records nothing. Once we've settled what the behavior should be, please have this test go through the real method (a scorable child in the fixture, or a mocked attempted score), so that it asserts the answer rather than bypassing the question.

[alezconsultant]

@ormsbee
Thanks for feedback. PR's branch has been rebased to master.

1. The test now imports only from models_api, and the end-to-end tests are down to three scenarios.
2. A failed rollup enqueue is now logged with the user and object IDs (as you proposed).
3. The model names are fixed.
4. The PR description now points to feat: evaluate and record a learner's leaf competency status for a grade openedx-core#862 and feat: roll up learner competency statuses above a graded criterion openedx-core#870.
5. A staff-entered grade now counts even without an attempt.
6. The pin bump is waiting on an openedx-core release that includes feat: roll up learner competency statuses above a graded criterion openedx-core#870. I'll bump it and add the release to the commit body then.

[alezconsultant]

@ormsbee core version bumped, so now we can finally fully done with review with this pr.

[ormsbee]

@alezconsultant, @mgwozdz-unicon: FWIW, I would expect ENABLE_COMPETENCY_MASTERY_TRACKING to be a short-lived thing that we keep around just long enough to make sure it works properly in a prod-like environment and to give close-to-master folks like MIT a way to opt out if we discover issues with it. I would expect this setting to be gone before the Willow cut, and that everyone will be running with competencies enabled, even if that's a null operation for most folks because they're not using competencies. Does that sound correct to you?

[mgwozdz-unicon]

@ormsbee Yes, that sounds right to me. When I originally envisioned this setting in openedx/openedx-core#701, I had two reasons in mind: First, a deployment not using competencies should not pay for a new query on the subsection grade write, and an operator who hits a problem should be able to stop competency code from blocking grade writes without a code deploy, since the status write shares the grade's transaction and a storage failure fails the grade. The second reason is exactly the short-lived safety valve you describe. The first one is weaker than I thought at the time: with no criteria in the deployment, record_graded_object_statuses runs one indexed SELECT that returns no rows and queues nothing, so the cost of running with it on everywhere is small. I'll file a follow-up ticket to remove the setting before the Willow cut so that the competency call runs unconditionally.