Skip to content

feat: roll up learner competency statuses above a graded criterion - #870

Open
alezconsultant wants to merge 2 commits into
openedx:mainfrom
alezconsultant:alezconsultant/643-rollup-competency-statuses
Open

alezconsultant wants to merge 2 commits into
openedx:mainfrom
alezconsultant:alezconsultant/643-rollup-competency-statuses

Conversation

@alezconsultant

@alezconsultant alezconsultant commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

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 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.

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.
  • 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 best root-group status) and its direct sub-competencies (Demonstrated when every one is, PartiallyAttempted when 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.
  • Queries. Both walks read once per level, so a level costs a fixed number of queries however many groups or parents it contains.

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

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>
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 13, 2026
@openedx-webhooks

Copy link
Copy Markdown

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:

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 tbain left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

This feels like something that needs to be addressed for performance concerns

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.

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
alezconsultant force-pushed the alezconsultant/643-rollup-competency-statuses branch from c1b1788 to 6752464 Compare October 16, 2026 01:24
@alezconsultant

Copy link
Copy Markdown
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Status: Needs Triage

Development

Successfully merging this pull request may close these issues.

3 participants