[alezconsultant]

Summary

Adds roll_up_competency_statuses(*, user_id, object_ids) to the CBE applet and exports it from openedx_learning.api. After a grade has committed, it recomputes every learner status above the criteria attached to object_ids:

1. Each criteria group, one level at a time, up to the root group.
2. The competency's StudentCompetencyStatus at the root group.
3. Every parent competency, one level at a time, up to the top of the taxonomy.

The function is meant to be called from the Celery task that runs after the grade transaction commits. ADR 0002 (Decisions 2 and 6) is amended to describe this flow and the set-based evaluation below.

How it works

• Groups. A group's children are its child groups and its criteria, read as a set. AND is Demonstrated when every child is, AttemptedNotDemonstrated as soon as any child is, and otherwise PartiallyAttempted if any child has a status. OR is Demonstrated as soon as any child is, AttemptedNotDemonstrated only when every child is, and otherwise PartiallyAttempted. A null operator behaves as OR. A group with nothing attempted beneath it gets no row. ordering plays no part in evaluation; it stays the sort key the read API uses for a stable sibling order, and its help text is left as is until the field's future is decided.
• Competency level. A root group's AttemptedNotDemonstrated is written as PartiallyAttempted, because StudentCompetencyStatus holds only PartiallyAttempted and Demonstrated.
• Parent competencies. A parent is the higher of two inputs: its own criteria result (its root group's status) and its direct sub-competencies (Demonstrated when every one is, PartiallyAttempted when any has a status). A sub-competency with no criteria tree never gets a row, so it holds its parent at PartiallyAttempted unless the parent's own criteria raise it. Reporting such tags belongs to the recompute command in [BE] Recompute learner competency statuses on operator command #774.
• Queries. Both walks read once per level, so a level costs a fixed number of queries however many groups or parents it contains.
• Transactions. Each level commits before the next level is read, so the function raises TransactionManagementError when called inside an atomic block. The platform task calls it outside any transaction. The two rollup test modules therefore run with transaction=True, and a fixture recreates the rows that migrations 0005 and 0009 seed, since the transactional flush removes them.
• One write path. raise_status() in statuses.py writes all three status tables, including the leaf write in record_graded_object_statuses(). It logs at info when it creates or raises a row. If the parent-competency walk reaches the taxonomy depth cap with parents still pending, it logs an error with the user id and the parent ids left out.

Archived groups, criteria, and tags are handled in #815, which builds on this PR.

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:

from datetime import datetime, timezone
from django.contrib.auth import get_user_model
from openedx_learning.api import roll_up_competency_statuses
from openedx_learning.models import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, CompetencyTaxonomy, LogicOperator, MasteryStatus, StudentCompetencyCriteriaGroupStatus, StudentCompetencyCriterionStatus, StudentCompetencyStatus
from openedx_tagging.models import ObjectTag, Tag

taxonomy = CompetencyTaxonomy.objects.create(name="Test643", export_id="test643")
profile = CompetencyRuleProfile.objects.get(organization__isnull=True, course__isnull=True, competency_taxonomy__isnull=True)
user, _ = get_user_model().objects.get_or_create(username="test643-learner")
other, _ = get_user_model().objects.get_or_create(username="test643-other")
now = datetime.now(tz=timezone.utc)

# Algebra > Linear equations, which needs both the quiz and the exam (AND group).
parent = Tag.objects.create(taxonomy=taxonomy, value="Algebra")
child = Tag.objects.create(taxonomy=taxonomy, value="Linear equations", parent=parent)
root = CompetencyCriteriaGroup.objects.create(tag=child)
both = CompetencyCriteriaGroup.objects.create(tag=child, parent=root, logic_operator=LogicOperator.AND)
otag_quiz = ObjectTag.objects.create(object_id="block-v1:Test643+C1+2026+problem+quiz", taxonomy=taxonomy, tag=child)
otag_exam = ObjectTag.objects.create(object_id="block-v1:Test643+C1+2026+problem+exam", taxonomy=taxonomy, tag=child)
crit_quiz = CompetencyCriterion.objects.create(group=both, object_tag=otag_quiz, rule_profile=profile)
crit_exam = CompetencyCriterion.objects.create(group=both, object_tag=otag_exam, rule_profile=profile)

# The learner passes the quiz: the grade path stores the criterion status, then the rollup runs.
StudentCompetencyCriterionStatus.objects.create(user=user, criterion=crit_quiz, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now)
roll_up_competency_statuses(user_id=user.id, object_ids=[otag_quiz.object_id])
StudentCompetencyCriteriaGroupStatus.objects.get(user=user, group=both).status_id  # 20 -> PartiallyAttempted
StudentCompetencyStatus.objects.get(user=user, tag=parent).status_id  # 20 -> PartiallyAttempted

# The learner passes the exam too.
StudentCompetencyCriterionStatus.objects.create(user=user, criterion=crit_exam, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now)
roll_up_competency_statuses(user_id=user.id, object_ids=[otag_exam.object_id])
StudentCompetencyCriteriaGroupStatus.objects.get(user=user, group=both).status_id  # 30 -> Demonstrated
StudentCompetencyStatus.objects.get(user=user, tag=parent).status_id  # 30 -> Demonstrated

# Another learner passes the quiz and fails the exam.
StudentCompetencyCriterionStatus.objects.create(user=other, criterion=crit_quiz, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now)
StudentCompetencyCriterionStatus.objects.create(user=other, criterion=crit_exam, status_id=MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED, created=now, modified=now)
roll_up_competency_statuses(user_id=other.id, object_ids=[otag_quiz.object_id, otag_exam.object_id])
StudentCompetencyCriteriaGroupStatus.objects.get(user=other, group=both).status_id  # 10 -> AttemptedNotDemonstrated
StudentCompetencyStatus.objects.get(user=other, tag=parent).status_id  # 20 -> PartiallyAttempted, competencies never store 10

Related to #643

🤖 Generated with Claude Code

[openedx-webhooks]

Thanks for the pull request, @alezconsultant!

This repository is currently maintained by @axim-engineering.

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]

Human questions (Some of these are from Mary):

1. (Comment cleanup - please clarify) For line 2 of rollup.py, what is meant by "changed criteria"?
2. (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.
3. (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").
4. (Comment cleanup - please clarify) I don't understand the comment on lines 54-65 in rollup.py.
5. (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)?
6. (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?
7. (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?
8. (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?
9. (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?
10. (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’s ordering and always reads/materializes every sibling status. This does not implement the ADR/ticket’s required ordered short-circuit evaluation. For large groups, an already-determined AND/OR result 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’s ordering and always reads/materializes every sibling status. This does not implement the ADR/ticket’s required ordered short-circuit evaluation. For large groups, an already-determined AND/OR result 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 targeted mypy passed.
• No migrations were added.
• The PR includes AI attribution and manual testing steps.

[alezconsultant]

@tbain

1. Fixed.
2. Yes, it's reachable. The AC requires it: "Completing one of several requirements shows progress" is handled there (the AND group's children are [DEMONSTRATED, None]). I also changed combine_children to use more explicit if-else branching.
3. Added a comment that the else branch covers both OR and a null operator (ADR 0002 treats null as OR)
4. Rewrote it plainly.
5. Yes, if I understood your question correctly. MasteryStatus is a models.IntegerChoices ordered lowest to highest, and statuses can only ever be raised (ADR 0004, Decision 4). The competency level maps 10 to 20, and the StudentCompetencyStatus check constraint allows only 20 and 30. So i think max is good here.
6. It can't. Each row is one parent tag at the current level with its best own-criteria status. Breaking on one tag's demonstrated status would drop the rows for the other tags. I also refactor and replace the loop with a GROUP BY and Max("status_id") query, so the database returns one row per tag and there's no loop left.
7. I removed it. Tag.lineage stores each ancestor's value followed by a \t, and i think it's clear enough without it since in my code we don't have any magic constants.

8-10. Tests fixed.

As for the bug and the suggestion. I ran into two issues with ordering. First, only CompetencyCriteriaGroup has an ordering field; CompetencyCriterion doesn't. Second, nothing sets ordering on CompetencyCriteriaGroup today, so every group has 0. So I switched to an unordered approach, which @mgwozdz-unicon approved. A short-circuit wouldn't save any queries either, because each level reads all child statuses in one batch. I could order by PK, if we short-circuit approach is mandatory.

[tbain]

Approved, thank you for addressing all those comments.

For the Codex bug it doesn't sound like it's feasible, I don't know if we need to update the ticket to amend the tickets requirements? But it sounds like you and Mary overrode it for good reasons.

[ormsbee]

Thank you for your patience. Two things I'd like settled before merging, and some smaller requests inline:

1. Archived groups and criteria. record_graded_object_statuses() skips archived criteria, but this walk counts an archived criterion or an archived child group as a child with no status, so once #674/#675 start archiving things, an AND group with an archived child can never reach Demonstrated for anyone who didn't attempt it first (inline on rollup.py, with the probe).
2. The ADR 0002 amendment the ticket asked for (and that @tbain's first review flagged) isn't in the PR. Decision 6's update flow still stops at the root group, says nothing about parent competencies or about status rows for tags that own no criteria tree, and both Decision 2 and Decision 6 still promise ordered, short-circuited evaluation, which this PR doesn't do (rightly, I think; inline on combine_children()). Please amend it in this PR, so that the next person doesn't implement ordering because the ADR told them to.

I'd also like the unearnable-sub-competency warning out of the per-learner path (inline on the log.warning). I know that one is in the ticket's acceptance criteria, so if you folks feel strongly that it belongs here I won't hold the merge on it, but please read the reasoning first.

The rest of the inline comments are smaller requests and one question.

Thank you.

---

Claude drafted the first pass of this review and ran the CBE suite on 446228f for me (413 tests, all passing on SQLite), along with the archived-row probe quoted inline and a query count for a single rollup through three group levels and a six-level taxonomy (79 queries on the first pass, 61 when run again with nothing left to write).

[ormsbee]

Should this and _child_groups() above (and the entry query in roll_up_competency_statuses()) filter archived=False? record_graded_object_statuses() skips archived criteria, so an archived criterion never gets a new leaf row, but this walk still counts it as a child with no status. Claude ran this on the branch with the user/tag/default_rule_profile fixtures and test_rollup_api.py's helpers:

root = CompetencyCriteriaGroup.objects.create(tag=tag)
leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, logic_operator=LogicOperator.AND)
live = _make_criterion(leaf, ASSIGNMENT_1, default_rule_profile)
old = _make_criterion(leaf, ASSIGNMENT_2, default_rule_profile)
old.archived = True
old.save()
with transaction.atomic():
    record_graded_object_statuses(user_id=user.id, scores=[
        GradedObjectScore(object_id=ASSIGNMENT_1, fraction=Decimal("1")),
        GradedObjectScore(object_id=ASSIGNMENT_2, fraction=Decimal("1")),
    ])  # returns 1: the archived criterion is skipped
roll_up_competency_statuses(user_id=user.id, object_ids=[ASSIGNMENT_1, ASSIGNMENT_2])

That leaves the leaf group, the root, and the competency at PartiallyAttempted, and the same thing happens with an archived child group under an AND root. So once #674/#675 start archiving, a requirement that was removed from a competency keeps every learner who didn't attempt it beforehand from ever reaching Demonstrated, and nothing un-blocks it later.

I think archived rows should be invisible here, the same way they are in record_graded_object_statuses(). The status rows already stored under them can stay put, since raise-only means they can't lower anything. (@mgwozdz-unicon: Does that make sense to you?)

Not an immediate action item, but I do wonder if we should be using custom managers or Proxy models to make it extremely obvious when certain bits of code are supposed to be reading archived rows vs. not.

[mgwozdz-unicon]

Yes, Anton is also working on this ticket to filter on archived=False: #815

[alezconsultant]

Yes, i add it in Unicon#3 ( need to go via unicon review first)

[ormsbee]

I'd like this out of the per-learner path. Whether a sub-competency has any criteria beneath it is a property of the taxonomy, not of the learner, but this check runs once per tag level on every rollup (five of the 79 queries in Claude's count were this one), and it logs the same pair again for every grade event of every learner under that parent. While a framework is only partly covered by courses, which I'd expect to be the normal state of things for a long time, that's a warning per not-yet-covered sibling per graded subsection per learner, and the support team will learn to ignore the whole message. I know the ticket asked for this, but I think it belongs in #774's recompute command (or an authoring-time check), where it runs once and someone is actually looking at the output.

[alezconsultant]

I agree, that message that nobody would read is bad practice, so if @mgwozdz-unicon approve, than we can move it to #774

[mgwozdz-unicon]

I agree with dropping this warning from the rollup.

For the record, what I had in mind when I wrote that AC was narrower than what the code does. I was envisioning a warning only when a missing criteria group actually blocks the parent in the rollup being computed: the learner has demonstrated every sub-competency that has criteria, and the parent still can't reach DEMONSTRATED because a sibling has no criteria anywhere beneath it. For the institutions I've talked with, a parent competency's sub-competencies live in one course, so once that course is built out they all have criteria and this would be a rare misconfiguration rather than a steady state. Sub-competencies split across courses could happen, but I expect it to be uncommon.

The check as written doesn't gate on that, so it fires for every uncovered sibling on every rollup, which is the noise you describe. However, I don't think this warning provides a ton of value anyways because even the narrow version reaches the wrong person: the operator reading logs wouldn't be the person we'd want to inform, it would be the course author.

So, since the AC asked for this, @alezconsultant please remove the warning and call out the removal in the PR description. I'll work with Tammie to get this AC removed and add a note on #774 that the recompute command should report sub-competencies with no criteria anywhere beneath them, as a cheap interim. Longer term this should probably become an authoring-time check on the competency management page, which is outside MVP scope.

[ormsbee]

FWIW, I think reading each level as a set makes total sense. That being said, ADR 0002 still talks about ordered short-circuiting in Decision 2 and in the Decision 6 update flow, and so does CompetencyCriteriaGroup.ordering's help text. So when you amend the ADR, please say that evaluation is set-based and what ordering is still for, if anything (just authoring display?).

[mgwozdz-unicon]

Reading each level as a set is correct, and ordering should play no part in evaluation. Every group combines its children with a single operator, all AND or all OR, and any mixing of the two happens through nesting rather than within one group. So the order of siblings under a group cannot change the result; the tree already settled what binds to what. Short-circuiting doesn't buy anything either: the rollup reads every child's stored status for a level in bulk before it decides anything, so stopping early would only skip a few in-memory comparisons.

Some history on why the field exists, so the ADR amendment doesn't invent a purpose for it. When I first designed this, the UX design did not show the hierarchical groupings; criteria were presented as a flat expression, and in a flat boolean expression operand order looks load-bearing. Once the UI started rendering the tree, that reason went away. It was never added for display.

Today the only thing reading it is the read API, which sorts siblings by (parent_id, ordering, id) so the tree comes back in a stable order. Whether the Rule Box UI will ever let authors reorder sibling groups is an open question, and if it doesn't, the field is a relic and should possibly just be removed. For the ADR, I'd suggest: evaluation is set-based and ignores ordering; the field currently only gives the read API a deterministic sibling order; and it is a candidate for removal unless the authoring UI turns out to need author-controlled ordering. Removal would be a follow-up, not part of this PR.

[ormsbee]

Please have _raise_leaf_status() in api.py call this (or just replace it with this), so there's consistency for all three tables. Right now it's the same get_or_create() plus conditional UPDATE with the table baked in, and the next person who fixes a bug in one of them will miss the other.

[ormsbee]

Please log at info when this creates or raises a row (the model, the lookup, and the status it went from and to), since "why does this learner show Demonstrated?" is the support question these tables exist to answer, and right now a rollup leaves no trace beyond the modified timestamps.

[alezconsultant]

@ormsbee
Rebase, update pr description, and apply fixes.

1. Handled in [BE] Exclude archived criteria and groups when rolling up a learner's competency status #815, which builds on this PR.
2. Decisions 2 and 6 now describe set-based evaluation with no ordering or short-circuit, and the update flow continues past the root group through parent competencies. The ordering help text is left alone, since changing it needs a migration; it can go with whatever we decide about the field.
3. Removed from the rollup; it moves to [BE] Recompute learner competency statuses on operator command #774's recompute command.
4. Running out of depth with parents still pending logs an error with the user id and the parent ids left out.
5. roll_up_competency_statuses() raises TransactionManagementError inside an atomic block, since each level commits before the next is read. The rollup tests run with transaction=True as a result.
6. The leaf write in record_graded_object_statuses() goes through it too, so all three status tables share one write path.
7. raise_status() logs at info when it creates or raises a row, with the model, the lookup, and the old and new status.

Also during the rebase process #872's one-root-per-competency constraint added, so the test that built two roots on purpose is gone.

[ormsbee]

Thank you for the quick turnaround. Everything from my last pass is in, or in the case of archived rows, deferred to #815, which works for me. Two small requests inline, neither of which need another pass from me: please make them and squash, and I'll merge.

Thank you.

---

Claude assistance

Claude drafted this pass and re-ran the CBE suite on caafb39 for me (578 tests, all passing on SQLite), re-counted last round's single-rollup probe (75 queries on the first pass and 57 on a repeat, down from 79 and 61 now that the unearnable check is gone), checked that openedx-platform#39179's task tests mock `roll_up_competency_statuses()` so the new transaction guard won't trip them, and confirmed the amended ADR renders on the Read the Docs build for this PR.

[ormsbee]

Please also capture the last part of @mgwozdz-unicon's note, that the field is a removal candidate unless the authoring UI turns out to need author-controlled order, so that the next person reading this knows it isn't settled. Something like:

Suggested change

6. ``ordering``: A sibling sort key. The read API returns a group's children ordered by ``(ordering, id)`` so a tree comes back in a stable order. Evaluation ignores it, because the result of ``AND`` or ``OR`` over a group's children cannot depend on the order they are read in.

6. ``ordering``: A sibling sort key. The read API returns a group's children ordered by ``(ordering, id)`` so a tree comes back in a stable order. Evaluation ignores it, because the result of ``AND`` or ``OR`` over a group's children cannot depend on the order they are read in. Whether authors will ever need to control that order is an open question; if the authoring UI turns out not to need it, the field should be removed in a follow-up.

[ormsbee]

Likewise, the CompetencyCriteriaGroup class docstring still says that evaluation relies on ordering for "deterministic, short-circuiting evaluation order". Unlike the help_text, that doesn't need a migration, so please fix it in this PR.