From b7d66cfd3fd500b09c03e8833b885b5b408a0cce Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Wed, 28 Oct 2026 19:11:36 +0400 Subject: [PATCH 1/4] feat: exclude archived criteria and groups from the status rollup Filter archived rows out of the two queries that fetch a group's children, so an archived criterion or criteria group does not count when a learner's group and competency statuses are combined. A group whose children are all archived has no status to report, so the rollup writes nothing for it. An archived group that the walk reaches is passed through, so its parent is still recomputed from its non-archived children, but its own status is not recomputed. An archived root's competency row is raised from the root's stored status, so a rollup that stopped between the root write and the competency write is repaired even after the root is archived. ADR-0002 Decision 6 records the evaluation rule. Statuses a learner already holds are never lowered or removed, so mastery earned through a requirement that was archived later stays earned. A parent competency's own archived root group counts through its stored status. Archiving does not trigger a rollup; a learner's statuses reflect it the next time a grade raises a non-archived criterion's status in that group's subtree, or when an operator recomputes them. Leaf evaluation skips archived criteria when a grade is recorded. The rollup's entry query does not, so a rollup call naming an archived criterion's content still recomputes that group from its non-archived children. Related to https://github.com/openedx/openedx-core/issues/815 Co-Authored-By: Claude Fable 5.1 --- .../0002-competency-criteria-model.rst | 12 +- src/openedx_learning/applets/cbe/rollup.py | 33 ++- .../applets/cbe/test_rollup_api.py | 192 +++++++++++++++++- .../applets/cbe/test_tag_rollup.py | 76 ++++++- 4 files changed, 296 insertions(+), 17 deletions(-) diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index 5869b2885..451d8e817 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -87,7 +87,7 @@ Decision 5. ``name``: string 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. 7. ``logic_operator``: Either “AND” or “OR” or null. This determines how children are combined at a group node ("AND" or "OR"). - 8. ``archived``: Boolean, defaults to false. Set instead of deleting a group once a student status exists for it. Archived groups are hidden from authoring and new associations but remain queryable, so existing ``CompetencyCriteria`` rows and learner status rows stay resolvable. For MVP, archiving a group or a criterion is permanent, with the root as the one reversible exception: a new criterion for an archived root's competency reactivates that root instead of creating a second one, because archiving it for good would remove the ability to ever associate criteria with that competency again. Archiving a course-level group blocks that course for that competency; archiving a leaf closes only that leaf to new criteria. More unarchival capabilities may be added after MVP. + 8. ``archived``: Boolean, defaults to false. Set instead of deleting a group once a student status exists for it. Archived groups are hidden from authoring and new associations but remain queryable, so existing ``CompetencyCriteria`` rows and learner status rows stay resolvable. For MVP, archiving a group or a criterion is permanent, with the root as the one reversible exception: a new criterion for an archived root's competency reactivates that root instead of creating a second one, because archiving it for good would remove the ability to ever associate criteria with that competency again. Archiving a course-level group blocks that course for that competency; archiving a leaf closes only that leaf to new criteria. More unarchival capabilities may be added after MVP. Archived groups do not count toward learner statuses (Decision 6). 9. ``scope_code``: Derived from ``oel_tagging_tag_id``, set on the root group and null on every other group, whether or not the root is archived. It exists so the database can hold at most one root per competency, the same way Decision 3's derived ``scope_code`` holds at most one live profile per scope. Example: A root group uses "OR" with two child groups. @@ -166,7 +166,7 @@ Decision 1. ``Grade``: ``{"op": "gte", "value": 0.75, "scale": "percent"}``. Allowed ``op`` values: ``gte``, ``lte``, ``eq``. ``value`` must be a fraction between 0.0 and 1.0 inclusive, matching the platform's existing fractional grade representation, not a 0-100 scale. - 7. ``archived``: Boolean, defaults to false. Set instead of deleting a criterion once a student status exists for it. Archived criteria are hidden from authoring and new associations but remain queryable, so existing learner status rows stay resolvable. + 7. ``archived``: Boolean, defaults to false. Set instead of deleting a criterion once a student status exists for it. Archived criteria are hidden from authoring and new associations but remain queryable, so existing learner status rows stay resolvable. Archived criteria do not count toward learner statuses (Decision 6). Exactly one of the following holds for a given criterion, never both, never neither: @@ -270,6 +270,7 @@ Decision - When a root group is recomputed, write the competency's ``StudentCompetencyStatus`` from it, mapping ``AttemptedNotDemonstrated`` to ``PartiallyAttempted`` because that table holds only ``Demonstrated`` and ``PartiallyAttempted``. - Then recompute each parent competency up the taxonomy, one level at a time. A parent's status is the better of two inputs: its own criteria result, which is its root group's status mapped the same way, and its sub-competency result, which is ``Demonstrated`` when every direct sub-competency is ``Demonstrated`` and ``PartiallyAttempted`` when at least one has a status. A competency with no criteria tree of its own gets a status row only through its sub-competencies; a competency with neither a root group nor a sub-competency with a status gets no row. - Every write only ever raises a stored status, and each level commits before the next is read. :ref:`openedx-learning-adr-0004` records why. + - An archived criterion is not evaluated when a grade is recorded, and a group is recomputed from its non-archived children only. Neither archiving nor the rollup lowers a stored status, including a status that a non-archived group or a competency earned through a child that is archived. Rows of archived criteria and groups are left unchanged. A status stored against an archived root group raises its competency's row and counts toward its parent competency. Archiving recomputes nothing itself: a learner's statuses reflect it the next time a grade raises a non-archived criterion's status in that group's subtree, or when an operator recomputes them. 1. Add new database table for ``CompetencyMasteryStatuses`` with these columns: @@ -490,3 +491,10 @@ Changelog supersedes the ordered, short-circuited evaluation the earlier text described. Decision 6's update flow now continues past the root group to the competency's own status row and up through parent competencies, and states what a competency with no criteria tree of its own receives. + +2026-11-06: + +* Amended Decision 6 for issue #815: a group is recomputed from its non-archived children only, + so an archived requirement or branch does not have to be met. A status recorded against an + archived criterion or group is kept. An archived root's stored status raises its competency's + row and counts toward its parent competency. Archiving by itself recomputes nothing. diff --git a/src/openedx_learning/applets/cbe/rollup.py b/src/openedx_learning/applets/cbe/rollup.py index 0ef6c2678..e1005862d 100644 --- a/src/openedx_learning/applets/cbe/rollup.py +++ b/src/openedx_learning/applets/cbe/rollup.py @@ -69,21 +69,34 @@ def combine_parent_competency_status( def _child_groups(group_ids: set[int]) -> list[tuple[int, int]]: - """Return (child group id, parent group id) for every child group of ``group_ids``.""" - return list(CompetencyCriteriaGroup.objects.filter(parent_id__in=group_ids).values_list("pk", "parent_id")) + """Return (child group id, parent group id) for every non-archived child group of ``group_ids``.""" + return list( + CompetencyCriteriaGroup.objects.filter(parent_id__in=group_ids, archived=False).values_list("pk", "parent_id") + ) def _child_criteria(group_ids: set[int]) -> list[tuple[int, int]]: - """Return (criterion id, group id) for every criterion directly under ``group_ids``.""" - return list(CompetencyCriterion.objects.filter(group_id__in=group_ids).values_list("pk", "group_id")) + """Return (criterion id, group id) for every non-archived criterion directly under ``group_ids``.""" + return list( + CompetencyCriterion.objects.filter(group_id__in=group_ids, archived=False).values_list("pk", "group_id") + ) def _to_status(value: int | None) -> MasteryStatus | None: return None if value is None else MasteryStatus(value) +def _stored_group_status(user_id: int, group_id: int) -> MasteryStatus | None: + """Return the status stored for ``group_id``, or None when the learner has no row for it.""" + return _to_status( + StudentCompetencyCriteriaGroupStatus.objects.filter(user_id=user_id, group_id=group_id) + .values_list("status_id", flat=True) + .first() + ) + + def _children_statuses(user_id: int, group_ids: set[int]) -> dict[int, list[MasteryStatus | None]]: - """Return, per group, one status (or None for no row) for each of its child groups and criteria.""" + """Return, per group, one status (or None for no row) for each of its non-archived child groups and criteria.""" child_groups = _child_groups(group_ids) child_criteria = _child_criteria(group_ids) group_status = dict( @@ -115,7 +128,8 @@ def _roll_up_groups(user_id: int, group_ids: set[int], now: datetime) -> set[int """ Recompute each group in ``group_ids`` and then its parent, one level at a time, up to the roots. - Returns the ids of the competency tags whose root group was reached. + An archived group is passed through without recomputing its status, and an archived root raises its competency from + the status it already holds. Returns the ids of the competency tags whose root group was reached. """ competency_tag_ids: set[int] = set() frontier = set(group_ids) @@ -123,7 +137,7 @@ def _roll_up_groups(user_id: int, group_ids: set[int], now: datetime) -> set[int statuses = _children_statuses(user_id, frontier) next_frontier: set[int] = set() for group in CompetencyCriteriaGroup.objects.filter(pk__in=frontier): - status = combine_children(group.logic_operator, statuses[group.pk]) + status = None if group.archived else combine_children(group.logic_operator, statuses[group.pk]) if status is not None: raise_status( StudentCompetencyCriteriaGroupStatus.objects, @@ -135,6 +149,8 @@ def _roll_up_groups(user_id: int, group_ids: set[int], now: datetime) -> set[int next_frontier.add(group.parent_id) continue competency_tag_ids.add(group.tag_id) + if group.archived: + status = _stored_group_status(user_id, group.pk) if status is not None: raise_status( StudentCompetencyStatus.objects, @@ -148,6 +164,7 @@ def _roll_up_groups(user_id: int, group_ids: set[int], now: datetime) -> set[int def _own_criteria_statuses(user_id: int, tag_ids: set[int]) -> dict[int, MasteryStatus]: """Return each tag's own criteria result: its best root-group status, for tags that have an attempted root.""" + # Archived roots are counted because the walk raises a competency's row from an archived root's stored status. rows = ( StudentCompetencyCriteriaGroupStatus.objects.filter( user_id=user_id, group__tag_id__in=tag_ids, group__parent__isnull=True, @@ -221,6 +238,7 @@ def roll_up_competency_statuses(*, user_id: int, object_ids: Sequence[str]) -> N The caller must not be inside a transaction, because each level has to commit before the next is read. Repeated calls with the same arguments are safe, since every write only ever raises a stored status. + Archived groups and criteria are left out of the combination, and a status already stored against them is kept. :raises TransactionManagementError: if called inside an atomic block. """ @@ -231,6 +249,7 @@ def roll_up_competency_statuses(*, user_id: int, object_ids: Sequence[str]) -> N ) if not object_ids: return + # Not filtered on archived: naming an archived criterion's object recomputes its group from non-archived children. group_ids = set( CompetencyCriterion.objects.filter( object_tag__object_id__in=list(set(object_ids)), diff --git a/tests/openedx_learning/applets/cbe/test_rollup_api.py b/tests/openedx_learning/applets/cbe/test_rollup_api.py index 1a913876c..5885bfb9e 100644 --- a/tests/openedx_learning/applets/cbe/test_rollup_api.py +++ b/tests/openedx_learning/applets/cbe/test_rollup_api.py @@ -39,17 +39,26 @@ ASSIGNMENT_2 = "block-v1:Org1+Python100+Fall2026+problem+a2" -def _make_group(tag: Tag, *, parent=None, logic_operator=None) -> CompetencyCriteriaGroup: +def _make_group(tag: Tag, *, parent=None, logic_operator=None, archived: bool = False) -> CompetencyCriteriaGroup: """Create a criteria group; a null operator is how real root and course-level groups are stored.""" - return CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent, logic_operator=logic_operator) + return CompetencyCriteriaGroup.objects.create( + tag=tag, parent=parent, logic_operator=logic_operator, archived=archived, + ) def _make_criterion( - group: CompetencyCriteriaGroup, object_id: str, profile: CompetencyRuleProfile, + group: CompetencyCriteriaGroup, object_id: str, profile: CompetencyRuleProfile, *, archived: bool = False, ) -> CompetencyCriterion: """Create a criterion on ``group`` for ``object_id``, tagging the object with the group's competency.""" object_tag = ObjectTag.objects.create(object_id=object_id, taxonomy=group.tag.taxonomy, tag=group.tag) - return CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=profile) + return CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=profile, archived=archived, + ) + + +def _archive(row: CompetencyCriteriaGroup | CompetencyCriterion) -> None: + row.archived = True + row.save(update_fields=["archived"]) def _record(user, criterion: CompetencyCriterion, status: MasteryStatus) -> None: @@ -425,3 +434,178 @@ def test_a_call_inside_a_transaction_raises(user) -> None: """Each level must commit before the next is read, so the walk refuses to run inside a transaction.""" with pytest.raises(TransactionManagementError), transaction.atomic(): _roll_up(user, ASSIGNMENT_1) + + +def test_an_archived_requirement_no_longer_has_to_be_met(user, tag, and_tree) -> None: + _archive(and_tree.first) + _record(user, and_tree.second, DEMONSTRATED) + + _roll_up(user, ASSIGNMENT_2) + + assert _group_status(user, and_tree.leaf) == DEMONSTRATED + assert _group_status(user, and_tree.root) == DEMONSTRATED + assert _competency_status(user, tag) == DEMONSTRATED + + +def test_a_requirement_failed_before_it_was_archived_no_longer_holds_the_learner_back(user, tag, and_tree) -> None: + _record(user, and_tree.first, FAILED) + _roll_up(user, ASSIGNMENT_1) + assert _group_status(user, and_tree.leaf) == FAILED + _archive(and_tree.first) + _record(user, and_tree.second, DEMONSTRATED) + + _roll_up(user, ASSIGNMENT_2) + + assert _group_status(user, and_tree.leaf) == DEMONSTRATED + assert _group_status(user, and_tree.root) == DEMONSTRATED + assert _competency_status(user, tag) == DEMONSTRATED + + +def test_an_archived_branch_no_longer_has_to_be_completed(user, tag, default_rule_profile) -> None: + root = _make_group(tag, logic_operator=LogicOperator.AND) + archived_leaf = _make_group(tag, parent=root, logic_operator=LogicOperator.AND, archived=True) + live_leaf = _make_group(tag, parent=root, logic_operator=LogicOperator.AND) + in_archived = _make_criterion(archived_leaf, ASSIGNMENT_1, default_rule_profile) + in_live = _make_criterion(live_leaf, ASSIGNMENT_2, default_rule_profile) + _record(user, in_archived, DEMONSTRATED) + _record(user, in_live, DEMONSTRATED) + + _roll_up(user, ASSIGNMENT_1, ASSIGNMENT_2) + + assert _group_status(user, archived_leaf) is None + assert _group_status(user, live_leaf) == DEMONSTRATED + assert _group_status(user, root) == DEMONSTRATED + assert _competency_status(user, tag) == DEMONSTRATED + + +def test_an_or_group_whose_only_demonstrated_route_is_archived_is_not_demonstrated(user, tag, or_tree) -> None: + _record(user, or_tree.first, DEMONSTRATED) + _archive(or_tree.route_1) + _archive(or_tree.first) + _record(user, or_tree.second, FAILED) + + _roll_up(user, ASSIGNMENT_1, ASSIGNMENT_2) + + assert _group_status(user, or_tree.route_1) is None + assert _group_status(user, or_tree.route_2) == FAILED + assert _group_status(user, or_tree.root) == FAILED + assert _competency_status(user, tag) == PARTIAL + + +def test_a_competency_whose_requirements_are_all_archived_reports_no_status(user, and_tree) -> None: + _archive(and_tree.first) + _archive(and_tree.second) + + _roll_up(user, ASSIGNMENT_1) + + assert not StudentCompetencyCriteriaGroupStatus.objects.filter(user=user).exists() + assert not StudentCompetencyStatus.objects.filter(user=user).exists() + + +def test_mastery_already_earned_through_a_requirement_later_archived_is_kept(user, tag, or_tree) -> None: + """The recomputed root is FAILED, below the stored DEMONSTRATED, so only a raise-only write keeps the stored row.""" + _record(user, or_tree.first, DEMONSTRATED) + with freeze_time("2026-02-01"): + _roll_up(user, ASSIGNMENT_1) + _archive(or_tree.route_1) + _archive(or_tree.first) + _record(user, or_tree.second, FAILED) + + with freeze_time("2026-03-01"): + _roll_up(user, ASSIGNMENT_2) + + assert _group_status(user, or_tree.route_2) == FAILED + assert _group_status(user, or_tree.root) == DEMONSTRATED + assert _competency_status(user, tag) == DEMONSTRATED + assert StudentCompetencyStatus.objects.get(tag=tag).modified == datetime(2026, 2, 1, tzinfo=timezone.utc) + + +def test_a_recorded_unmet_group_status_is_kept_when_its_only_attempted_requirement_is_archived(user, and_tree) -> None: + _record(user, and_tree.first, FAILED) + _roll_up(user, ASSIGNMENT_1) + _archive(and_tree.first) + + _roll_up(user, ASSIGNMENT_1) + + assert _group_status(user, and_tree.leaf) == FAILED + + +def test_a_previously_demonstrated_requirement_changes_nothing_until_the_rollup_runs(user, tag, and_tree) -> None: + _record(user, and_tree.first, DEMONSTRATED) + _record(user, and_tree.second, FAILED) + _roll_up(user, ASSIGNMENT_1, ASSIGNMENT_2) + assert _group_status(user, and_tree.leaf) == FAILED + + _archive(and_tree.second) + + _roll_up(user, ASSIGNMENT_2) + assert _group_status(user, and_tree.leaf) == DEMONSTRATED + assert _competency_status(user, tag) == DEMONSTRATED + + +def test_an_archived_root_group_writes_neither_its_own_nor_the_competency_status( + user, tag, default_rule_profile, +) -> None: + root = _make_group(tag, archived=True) + leaf = _make_group(tag, parent=root, logic_operator=LogicOperator.AND) + criterion = _make_criterion(leaf, ASSIGNMENT_1, default_rule_profile) + _record(user, criterion, DEMONSTRATED) + + _roll_up(user, ASSIGNMENT_1) + + assert _group_status(user, leaf) == DEMONSTRATED + assert _group_status(user, root) is None + assert _competency_status(user, tag) is None + + +def test_an_archived_root_raises_its_competency_from_the_stored_root_status(user, tag, and_tree) -> None: + """A rollup that stopped between the root write and the competency write is repaired after the root is archived.""" + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=and_tree.root, status_id=DEMONSTRATED, created=NOW, modified=NOW, + ) + _archive(and_tree.first) + _archive(and_tree.second) + _archive(and_tree.root) + + _roll_up(user, ASSIGNMENT_1) + + assert _group_status(user, and_tree.root) == DEMONSTRATED + assert _competency_status(user, tag) == DEMONSTRATED + + +def test_the_walk_continues_past_an_archived_group_to_its_parent(user, tag, default_rule_profile) -> None: + root = _make_group(tag) + archived_leaf = _make_group(tag, parent=root, logic_operator=LogicOperator.AND, archived=True) + live_leaf = _make_group(tag, parent=root, logic_operator=LogicOperator.AND) + in_archived = _make_criterion(archived_leaf, ASSIGNMENT_1, default_rule_profile) + _make_criterion(live_leaf, ASSIGNMENT_2, default_rule_profile) + _record(user, in_archived, DEMONSTRATED) + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=live_leaf, status_id=DEMONSTRATED, created=NOW, modified=NOW, + ) + + _roll_up(user, ASSIGNMENT_1) + + assert _group_status(user, archived_leaf) is None + assert _group_status(user, root) == DEMONSTRATED + assert _competency_status(user, tag) == DEMONSTRATED + + +def test_an_archived_intermediate_group_cuts_its_live_subtree_off_from_the_root( + user, tag, default_rule_profile, +) -> None: + """ + The archived middle group is dropped from the root's children, so the live leaf's result does not reach the root. + """ + root = _make_group(tag) + middle = _make_group(tag, parent=root, logic_operator=LogicOperator.AND, archived=True) + leaf = _make_group(tag, parent=middle, logic_operator=LogicOperator.AND) + criterion = _make_criterion(leaf, ASSIGNMENT_1, default_rule_profile) + _record(user, criterion, DEMONSTRATED) + + _roll_up(user, ASSIGNMENT_1) + + assert _group_status(user, leaf) == DEMONSTRATED + assert _group_status(user, middle) is None + assert _group_status(user, root) is None + assert _competency_status(user, tag) is None diff --git a/tests/openedx_learning/applets/cbe/test_tag_rollup.py b/tests/openedx_learning/applets/cbe/test_tag_rollup.py index 5d9cf2646..7f1362e52 100644 --- a/tests/openedx_learning/applets/cbe/test_tag_rollup.py +++ b/tests/openedx_learning/applets/cbe/test_tag_rollup.py @@ -42,11 +42,13 @@ def _object_id(name: str) -> str: return f"block-v1:Org1+Python100+Fall2026+problem+{name}" -def _earnable(tag: Tag, object_id: str, profile: CompetencyRuleProfile) -> SimpleNamespace: - """Give ``tag`` a root group with one criterion on ``object_id``.""" - root = CompetencyCriteriaGroup.objects.create(tag=tag) +def _earnable(tag: Tag, object_id: str, profile: CompetencyRuleProfile, *, archived: bool = False) -> SimpleNamespace: + """Give ``tag`` a root group with one criterion on ``object_id``, both archived when ``archived`` is set.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag, archived=archived) object_tag = ObjectTag.objects.create(object_id=object_id, taxonomy=tag.taxonomy, tag=tag) - criterion = CompetencyCriterion.objects.create(group=root, object_tag=object_tag, rule_profile=profile) + criterion = CompetencyCriterion.objects.create( + group=root, object_tag=object_tag, rule_profile=profile, archived=archived, + ) return SimpleNamespace(root=root, criterion=criterion, object_id=object_id) @@ -63,6 +65,11 @@ def _set_group_status(user, group: CompetencyCriteriaGroup, status: MasteryStatu ) +def _set_competency_status(user, tag: Tag, status: MasteryStatus) -> None: + """Store a competency status as an earlier rollup would have.""" + StudentCompetencyStatus.objects.create(user=user, tag=tag, status_id=status, created=NOW, modified=NOW) + + def _competency_status(user, tag: Tag) -> int | None: row = StudentCompetencyStatus.objects.filter(user=user, tag=tag).first() return None if row is None else row.status_id @@ -316,3 +323,64 @@ def test_competencies_graded_at_different_depths_converge_on_the_shared_ancestor assert _competency_status(user, middle) == DEMONSTRATED assert _competency_status(user, shallow) == DEMONSTRATED assert _competency_status(user, top) == DEMONSTRATED + + +def test_a_parent_with_its_own_requirements_is_demonstrated_when_a_sub_competency_is_fully_archived( + user, family, default_rule_profile, +) -> None: + own = _earnable(family.parent, _object_id("p"), default_rule_profile) + _set_group_status(user, own.root, DEMONSTRATED) + _earnable(family.children[0], _object_id("c1"), default_rule_profile, archived=True) + earnable = _earnable(family.children[1], _object_id("c2"), default_rule_profile) + _record(user, earnable, DEMONSTRATED) + + _roll_up(user, earnable.object_id) + + assert _competency_status(user, family.parent) == DEMONSTRATED + assert _competency_status(user, family.children[1]) == DEMONSTRATED + + +def test_a_parent_with_no_requirements_of_its_own_stays_short_when_a_sub_competency_is_fully_archived( + user, competency_taxonomy, default_rule_profile, +) -> None: + parent = _make_tag(competency_taxonomy, "Parent") + archived_child = _make_tag(competency_taxonomy, "Archived", parent) + live_child = _make_tag(competency_taxonomy, "Live", parent) + _earnable(archived_child, _object_id("a"), default_rule_profile, archived=True) + earnable = _earnable(live_child, _object_id("l"), default_rule_profile) + _record(user, earnable, DEMONSTRATED) + + _roll_up(user, earnable.object_id) + + assert _competency_status(user, live_child) == DEMONSTRATED + assert _competency_status(user, parent) == PARTIAL + + +def test_an_archived_root_group_still_starts_the_parent_competency_walk( + user, competency_taxonomy, default_rule_profile, +) -> None: + parent = _make_tag(competency_taxonomy, "Parent") + archived_child = _make_tag(competency_taxonomy, "Archived", parent) + live_child = _make_tag(competency_taxonomy, "Live", parent) + earnable = _earnable(archived_child, _object_id("a"), default_rule_profile, archived=True) + _set_competency_status(user, live_child, DEMONSTRATED) + _record(user, earnable, DEMONSTRATED) + + _roll_up(user, earnable.object_id) + + assert _competency_status(user, archived_child) is None + assert _competency_status(user, parent) == PARTIAL + + +def test_a_parents_own_archived_root_still_counts_through_its_stored_status( + user, family, default_rule_profile, +) -> None: + own = _earnable(family.parent, _object_id("p"), default_rule_profile, archived=True) + _set_group_status(user, own.root, DEMONSTRATED) + earnable = _earnable(family.children[0], _object_id("c1"), default_rule_profile) + _record(user, earnable, FAILED) + + _roll_up(user, earnable.object_id) + + assert _competency_status(user, family.children[0]) == PARTIAL + assert _competency_status(user, family.parent) == DEMONSTRATED From 365fa1c486f11c7244e922082b81d7aa03af8e23 Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Thu, 12 Nov 2026 20:21:59 +0400 Subject: [PATCH 2/4] refactor: split the competency rollup walks into one function per level Each of the two walks in rollup.py is now a short loop over levels that delegates the work for one level. _roll_up_groups calls _roll_up_group_level, which calls _roll_up_group for each group, and _roll_up_parent_tags calls _roll_up_parent_level. combine_children keeps its signature and delegates to _combine_and and _combine_or, one function per operator. The parent-competency walk no longer queries the parent tags at each level to read back the primary keys it already holds, so each level of that walk makes one query fewer, and the parents of a level are processed in ascending id order. In the rollup tests, the _earnable helper is named _add_requirement, the FAILED alias is named NOT_DEMONSTRATED, and the tests for archived requirements name each competency and requirement after its role. A new test covers a parent competency that has no requirement of its own and no sub-competency with a status, which takes branch coverage of rollup.py to 100%. Related to https://github.com/openedx/openedx-core/issues/815 Co-Authored-By: Claude Fable 5.2 --- src/openedx_learning/applets/cbe/rollup.py | 202 +++++++++++------ .../applets/cbe/test_rollup_api.py | 50 ++--- .../applets/cbe/test_rollup_rules.py | 27 ++- .../applets/cbe/test_tag_rollup.py | 210 ++++++++++-------- 4 files changed, 290 insertions(+), 199 deletions(-) diff --git a/src/openedx_learning/applets/cbe/rollup.py b/src/openedx_learning/applets/cbe/rollup.py index e1005862d..e8145de2e 100644 --- a/src/openedx_learning/applets/cbe/rollup.py +++ b/src/openedx_learning/applets/cbe/rollup.py @@ -30,6 +30,24 @@ __all__ = ["combine_children", "combine_parent_competency_status", "roll_up_competency_statuses"] +def _combine_and(child_statuses: Sequence[MasteryStatus | None]) -> MasteryStatus: + """Return an AND group's status: demonstrated when every child is, not demonstrated when any child is.""" + if all(status == MasteryStatus.DEMONSTRATED for status in child_statuses): + return MasteryStatus.DEMONSTRATED + if MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED in child_statuses: + return MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED + return MasteryStatus.PARTIALLY_ATTEMPTED + + +def _combine_or(child_statuses: Sequence[MasteryStatus | None]) -> MasteryStatus: + """Return an OR group's status: demonstrated when any child is, not demonstrated when every child is.""" + if MasteryStatus.DEMONSTRATED in child_statuses: + return MasteryStatus.DEMONSTRATED + if all(status == MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED for status in child_statuses): + return MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED + return MasteryStatus.PARTIALLY_ATTEMPTED + + def combine_children( logic_operator: str | None, child_statuses: Sequence[MasteryStatus | None], ) -> MasteryStatus | None: @@ -38,22 +56,11 @@ def combine_children( ``None`` entries are children with no row. A null ``logic_operator`` is treated as OR. """ - attempted = {status for status in child_statuses if status is not None} - if not attempted: + if all(status is None for status in child_statuses): return None - every_child_attempted = None not in child_statuses if logic_operator == LogicOperator.AND: - if every_child_attempted and attempted == {MasteryStatus.DEMONSTRATED}: - return MasteryStatus.DEMONSTRATED - if MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED in attempted: - return MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED - else: # OR, or a null operator, which behaves as OR. - if MasteryStatus.DEMONSTRATED in attempted: - return MasteryStatus.DEMONSTRATED - if every_child_attempted and attempted == {MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED}: - return MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED - # Some child has a status, but neither rule above is settled: the learner is in progress. - return MasteryStatus.PARTIALLY_ATTEMPTED + return _combine_and(child_statuses) + return _combine_or(child_statuses) def combine_parent_competency_status( @@ -124,41 +131,89 @@ def _competency_level(status: MasteryStatus) -> MasteryStatus: return status +def _raise_group_status(user_id: int, group_id: int, status: MasteryStatus | None, now: datetime) -> None: + """Raise the learner's stored status for the group to ``status``; a None status writes nothing.""" + if status is None: + return + raise_status( + StudentCompetencyCriteriaGroupStatus.objects, + lookup={"user_id": user_id, "group_id": group_id}, + status=status, + now=now, + ) + + +def _raise_competency_status(user_id: int, tag_id: int, status: MasteryStatus | None, now: datetime) -> None: + """Raise the learner's stored status for the competency to ``status``; a None status writes nothing.""" + if status is None: + return + raise_status( + StudentCompetencyStatus.objects, + lookup={"user_id": user_id, "tag_id": tag_id}, + status=status, + now=now, + ) + + +def _recompute_group( + user_id: int, group: CompetencyCriteriaGroup, child_statuses: list[MasteryStatus | None], now: datetime, +) -> MasteryStatus | None: + """Raise the group's stored status to its children's combined result and return that result.""" + status = combine_children(group.logic_operator, child_statuses) + _raise_group_status(user_id, group.pk, status, now) + return status + + +def _roll_up_group( + user_id: int, group: CompetencyCriteriaGroup, child_statuses: list[MasteryStatus | None], now: datetime, +) -> None: + """ + Recompute one group, and for a root group raise its competency's status from the root's status. + + An archived group is not recomputed, and an archived root passes on the status it already holds. + """ + is_root = group.parent_id is None + if not group.archived: + status = _recompute_group(user_id, group, child_statuses, now) + elif is_root: + status = _stored_group_status(user_id, group.pk) + else: + status = None + if is_root and status is not None: + _raise_competency_status(user_id, group.tag_id, _competency_level(status), now) + + +def _roll_up_group_level(user_id: int, group_ids: set[int], now: datetime) -> list[CompetencyCriteriaGroup]: + """Roll up each group in ``group_ids`` from its children's statuses, and return the groups.""" + child_statuses = _children_statuses(user_id, group_ids) + groups = list(CompetencyCriteriaGroup.objects.filter(pk__in=group_ids)) + for group in groups: + _roll_up_group(user_id, group, child_statuses[group.pk], now) + return groups + + +def _competency_tag_ids(groups: list[CompetencyCriteriaGroup]) -> set[int]: + """Return the ids of the competency tags that the root groups in ``groups`` belong to.""" + return {group.tag_id for group in groups if group.parent_id is None} + + +def _parent_group_ids(groups: list[CompetencyCriteriaGroup]) -> set[int]: + """Return the ids of the parent groups of the non-root groups in ``groups``.""" + return {group.parent_id for group in groups if group.parent_id is not None} + + def _roll_up_groups(user_id: int, group_ids: set[int], now: datetime) -> set[int]: """ Recompute each group in ``group_ids`` and then its parent, one level at a time, up to the roots. - An archived group is passed through without recomputing its status, and an archived root raises its competency from - the status it already holds. Returns the ids of the competency tags whose root group was reached. + Returns the ids of the competency tags whose root group was reached. """ competency_tag_ids: set[int] = set() - frontier = set(group_ids) - while frontier: - statuses = _children_statuses(user_id, frontier) - next_frontier: set[int] = set() - for group in CompetencyCriteriaGroup.objects.filter(pk__in=frontier): - status = None if group.archived else combine_children(group.logic_operator, statuses[group.pk]) - if status is not None: - raise_status( - StudentCompetencyCriteriaGroupStatus.objects, - lookup={"user_id": user_id, "group_id": group.pk}, - status=status, - now=now, - ) - if group.parent_id is not None: - next_frontier.add(group.parent_id) - continue - competency_tag_ids.add(group.tag_id) - if group.archived: - status = _stored_group_status(user_id, group.pk) - if status is not None: - raise_status( - StudentCompetencyStatus.objects, - lookup={"user_id": user_id, "tag_id": group.tag_id}, - status=_competency_level(status), - now=now, - ) - frontier = next_frontier + level = set(group_ids) + while level: + groups = _roll_up_group_level(user_id, level, now) + competency_tag_ids |= _competency_tag_ids(groups) + level = _parent_group_ids(groups) return competency_tag_ids @@ -191,39 +246,46 @@ def _parent_ids(tag_ids: set[int]) -> set[int]: return set(Tag.objects.filter(pk__in=tag_ids, parent_id__isnull=False).values_list("parent_id", flat=True)) +def _child_ids_by_parent(parent_ids: set[int]) -> dict[int, list[int]]: + """Return, per parent tag, the ids of its direct child tags.""" + children: dict[int, list[int]] = {} + for child_id, parent_id in Tag.objects.filter(parent_id__in=parent_ids).values_list("pk", "parent_id"): + children.setdefault(parent_id, []).append(child_id) + return children + + +def _child_competency_statuses(user_id: int, parent_ids: set[int]) -> dict[int, int]: + """Return the learner's stored competency status for each child tag of ``parent_ids`` that has one.""" + return dict( + StudentCompetencyStatus.objects.filter(user_id=user_id, tag__parent_id__in=parent_ids).values_list( + "tag_id", "status_id", + ) + ) + + +def _roll_up_parent_level(user_id: int, parent_ids: set[int], now: datetime) -> None: + """Recompute each competency in ``parent_ids`` from its own criteria and its direct sub-competencies.""" + child_ids = _child_ids_by_parent(parent_ids) + child_statuses = _child_competency_statuses(user_id, parent_ids) + own_statuses = _own_criteria_statuses(user_id, parent_ids) + for parent_id in sorted(parent_ids): + status = combine_parent_competency_status( + own_statuses.get(parent_id), _sub_competency_status(child_ids.get(parent_id, []), child_statuses), + ) + _raise_competency_status(user_id, parent_id, status, now) + + def _roll_up_parent_tags(user_id: int, tag_ids: set[int], now: datetime) -> None: """Recompute the parent of each tag in ``tag_ids``, then its parent, one level at a time, up to the top.""" - frontier = set(tag_ids) + level = set(tag_ids) # The depth cap bounds the walk to a few iterations, and a tag's parents stay in its own taxonomy. for _ in range(TAXONOMY_MAX_DEPTH): - parent_ids = _parent_ids(frontier) - if not parent_ids: + level = _parent_ids(level) + if not level: return - parents = list(Tag.objects.filter(pk__in=parent_ids)) - children_by_parent: dict[int, list[int]] = {} - for child_id, parent_id in Tag.objects.filter(parent_id__in=parent_ids).values_list("pk", "parent_id"): - children_by_parent.setdefault(parent_id, []).append(child_id) - competency_status = dict( - StudentCompetencyStatus.objects.filter(user_id=user_id, tag__parent_id__in=parent_ids).values_list( - "tag_id", "status_id", - ) - ) - own = _own_criteria_statuses(user_id, parent_ids) - for parent in parents: - child_ids = children_by_parent.get(parent.pk, []) - status = combine_parent_competency_status( - own.get(parent.pk), _sub_competency_status(child_ids, competency_status), - ) - if status is not None: - raise_status( - StudentCompetencyStatus.objects, - lookup={"user_id": user_id, "tag_id": parent.pk}, - status=status, - now=now, - ) - frontier = parent_ids + _roll_up_parent_level(user_id, level, now) # Tag.save() enforces the cap, so only a tree built around it can still have parents here. - pending = _parent_ids(frontier) + pending = _parent_ids(level) if pending: log.error( "Competency rollup for user %s stopped at the taxonomy depth cap; parent tags %s were not recomputed", diff --git a/tests/openedx_learning/applets/cbe/test_rollup_api.py b/tests/openedx_learning/applets/cbe/test_rollup_api.py index 5885bfb9e..e6d951681 100644 --- a/tests/openedx_learning/applets/cbe/test_rollup_api.py +++ b/tests/openedx_learning/applets/cbe/test_rollup_api.py @@ -32,7 +32,7 @@ DEMONSTRATED = MasteryStatus.DEMONSTRATED PARTIAL = MasteryStatus.PARTIALLY_ATTEMPTED -FAILED = MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED +NOT_DEMONSTRATED = MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED NOW = datetime(2026, 1, 1, tzinfo=timezone.utc) ASSIGNMENT_1 = "block-v1:Org1+Python100+Fall2026+problem+a1" @@ -133,11 +133,11 @@ def test_completing_one_of_several_requirements_shows_progress(user, tag, and_tr def test_failing_one_of_two_attempted_requirements_shows_the_failure_on_the_group(user, tag, and_tree) -> None: """Holds when both grades are recorded before the first rollup; see the next test for the other order.""" _record(user, and_tree.first, DEMONSTRATED) - _record(user, and_tree.second, FAILED) + _record(user, and_tree.second, NOT_DEMONSTRATED) _roll_up(user, ASSIGNMENT_1, ASSIGNMENT_2) - assert _group_status(user, and_tree.leaf) == FAILED + assert _group_status(user, and_tree.leaf) == NOT_DEMONSTRATED assert _competency_status(user, tag) == PARTIAL @@ -147,7 +147,7 @@ def test_a_group_stays_partially_attempted_once_stored_even_when_the_last_requir """Raise-only writes discard the failure computed on the second rollup, because it ranks below what is stored.""" _record(user, and_tree.first, DEMONSTRATED) _roll_up(user, ASSIGNMENT_1) - _record(user, and_tree.second, FAILED) + _record(user, and_tree.second, NOT_DEMONSTRATED) _roll_up(user, ASSIGNMENT_2) @@ -166,7 +166,7 @@ def test_either_alternative_route_demonstrates_the_competency(user, tag, or_tree def test_attempting_some_routes_without_succeeding_shows_progress(user, tag, or_tree) -> None: - _record(user, or_tree.first, FAILED) + _record(user, or_tree.first, NOT_DEMONSTRATED) _roll_up(user, ASSIGNMENT_1) @@ -176,24 +176,24 @@ def test_attempting_some_routes_without_succeeding_shows_progress(user, tag, or_ def test_failing_every_route_shows_the_failure_on_the_root_but_not_on_the_competency(user, tag, or_tree) -> None: """Holds when both grades are recorded before the first rollup, as in the AND-group test above.""" - _record(user, or_tree.first, FAILED) - _record(user, or_tree.second, FAILED) + _record(user, or_tree.first, NOT_DEMONSTRATED) + _record(user, or_tree.second, NOT_DEMONSTRATED) _roll_up(user, ASSIGNMENT_1, ASSIGNMENT_2) - assert _group_status(user, or_tree.root) == FAILED + assert _group_status(user, or_tree.root) == NOT_DEMONSTRATED assert _competency_status(user, tag) == PARTIAL def test_failing_every_route_one_rollup_at_a_time_leaves_the_root_partially_attempted(user, tag, or_tree) -> None: - """Pins the raise-only behavior for the other grading order: the stored PARTIAL is never replaced by FAILED.""" - _record(user, or_tree.first, FAILED) + """Pins raise-only writes for the other grading order: a stored PARTIAL is never replaced by NOT_DEMONSTRATED.""" + _record(user, or_tree.first, NOT_DEMONSTRATED) _roll_up(user, ASSIGNMENT_1) - _record(user, or_tree.second, FAILED) + _record(user, or_tree.second, NOT_DEMONSTRATED) _roll_up(user, ASSIGNMENT_2) - assert _group_status(user, or_tree.route_1) == FAILED - assert _group_status(user, or_tree.route_2) == FAILED + assert _group_status(user, or_tree.route_1) == NOT_DEMONSTRATED + assert _group_status(user, or_tree.route_2) == NOT_DEMONSTRATED assert _group_status(user, or_tree.root) == PARTIAL assert _competency_status(user, tag) == PARTIAL @@ -448,9 +448,9 @@ def test_an_archived_requirement_no_longer_has_to_be_met(user, tag, and_tree) -> def test_a_requirement_failed_before_it_was_archived_no_longer_holds_the_learner_back(user, tag, and_tree) -> None: - _record(user, and_tree.first, FAILED) + _record(user, and_tree.first, NOT_DEMONSTRATED) _roll_up(user, ASSIGNMENT_1) - assert _group_status(user, and_tree.leaf) == FAILED + assert _group_status(user, and_tree.leaf) == NOT_DEMONSTRATED _archive(and_tree.first) _record(user, and_tree.second, DEMONSTRATED) @@ -482,13 +482,13 @@ def test_an_or_group_whose_only_demonstrated_route_is_archived_is_not_demonstrat _record(user, or_tree.first, DEMONSTRATED) _archive(or_tree.route_1) _archive(or_tree.first) - _record(user, or_tree.second, FAILED) + _record(user, or_tree.second, NOT_DEMONSTRATED) _roll_up(user, ASSIGNMENT_1, ASSIGNMENT_2) assert _group_status(user, or_tree.route_1) is None - assert _group_status(user, or_tree.route_2) == FAILED - assert _group_status(user, or_tree.root) == FAILED + assert _group_status(user, or_tree.route_2) == NOT_DEMONSTRATED + assert _group_status(user, or_tree.root) == NOT_DEMONSTRATED assert _competency_status(user, tag) == PARTIAL @@ -503,38 +503,38 @@ def test_a_competency_whose_requirements_are_all_archived_reports_no_status(user def test_mastery_already_earned_through_a_requirement_later_archived_is_kept(user, tag, or_tree) -> None: - """The recomputed root is FAILED, below the stored DEMONSTRATED, so only a raise-only write keeps the stored row.""" + """The recomputed root is NOT_DEMONSTRATED, below the stored DEMONSTRATED, so a raise-only write keeps the row.""" _record(user, or_tree.first, DEMONSTRATED) with freeze_time("2026-02-01"): _roll_up(user, ASSIGNMENT_1) _archive(or_tree.route_1) _archive(or_tree.first) - _record(user, or_tree.second, FAILED) + _record(user, or_tree.second, NOT_DEMONSTRATED) with freeze_time("2026-03-01"): _roll_up(user, ASSIGNMENT_2) - assert _group_status(user, or_tree.route_2) == FAILED + assert _group_status(user, or_tree.route_2) == NOT_DEMONSTRATED assert _group_status(user, or_tree.root) == DEMONSTRATED assert _competency_status(user, tag) == DEMONSTRATED assert StudentCompetencyStatus.objects.get(tag=tag).modified == datetime(2026, 2, 1, tzinfo=timezone.utc) def test_a_recorded_unmet_group_status_is_kept_when_its_only_attempted_requirement_is_archived(user, and_tree) -> None: - _record(user, and_tree.first, FAILED) + _record(user, and_tree.first, NOT_DEMONSTRATED) _roll_up(user, ASSIGNMENT_1) _archive(and_tree.first) _roll_up(user, ASSIGNMENT_1) - assert _group_status(user, and_tree.leaf) == FAILED + assert _group_status(user, and_tree.leaf) == NOT_DEMONSTRATED def test_a_previously_demonstrated_requirement_changes_nothing_until_the_rollup_runs(user, tag, and_tree) -> None: _record(user, and_tree.first, DEMONSTRATED) - _record(user, and_tree.second, FAILED) + _record(user, and_tree.second, NOT_DEMONSTRATED) _roll_up(user, ASSIGNMENT_1, ASSIGNMENT_2) - assert _group_status(user, and_tree.leaf) == FAILED + assert _group_status(user, and_tree.leaf) == NOT_DEMONSTRATED _archive(and_tree.second) diff --git a/tests/openedx_learning/applets/cbe/test_rollup_rules.py b/tests/openedx_learning/applets/cbe/test_rollup_rules.py index 108aecf92..273a76743 100644 --- a/tests/openedx_learning/applets/cbe/test_rollup_rules.py +++ b/tests/openedx_learning/applets/cbe/test_rollup_rules.py @@ -10,7 +10,7 @@ DEMONSTRATED = MasteryStatus.DEMONSTRATED PARTIAL = MasteryStatus.PARTIALLY_ATTEMPTED -FAILED = MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED +NOT_DEMONSTRATED = MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED AND = LogicOperator.AND OR = LogicOperator.OR @@ -22,18 +22,18 @@ (AND, [DEMONSTRATED, DEMONSTRATED], DEMONSTRATED), (AND, [DEMONSTRATED, None], PARTIAL), (AND, [None, None], None), - (AND, [DEMONSTRATED, FAILED], FAILED), - (AND, [None, FAILED], FAILED), + (AND, [DEMONSTRATED, NOT_DEMONSTRATED], NOT_DEMONSTRATED), + (AND, [None, NOT_DEMONSTRATED], NOT_DEMONSTRATED), (AND, [PARTIAL, DEMONSTRATED], PARTIAL), - (AND, [FAILED, FAILED], FAILED), + (AND, [NOT_DEMONSTRATED, NOT_DEMONSTRATED], NOT_DEMONSTRATED), (OR, [DEMONSTRATED, None], DEMONSTRATED), - (OR, [FAILED, DEMONSTRATED], DEMONSTRATED), - (OR, [FAILED, FAILED], FAILED), - (OR, [FAILED, None], PARTIAL), - (OR, [PARTIAL, FAILED], PARTIAL), + (OR, [NOT_DEMONSTRATED, DEMONSTRATED], DEMONSTRATED), + (OR, [NOT_DEMONSTRATED, NOT_DEMONSTRATED], NOT_DEMONSTRATED), + (OR, [NOT_DEMONSTRATED, None], PARTIAL), + (OR, [PARTIAL, NOT_DEMONSTRATED], PARTIAL), (OR, [None, None], None), - (None, [FAILED, DEMONSTRATED], DEMONSTRATED), - (None, [FAILED], FAILED), + (None, [NOT_DEMONSTRATED, DEMONSTRATED], DEMONSTRATED), + (None, [NOT_DEMONSTRATED], NOT_DEMONSTRATED), (AND, [], None), (OR, [], None), ], @@ -45,7 +45,12 @@ def test_combine_children(operator, children, expected) -> None: @pytest.mark.parametrize("operator", [AND, OR, None]) @pytest.mark.parametrize( - "children", [[DEMONSTRATED, FAILED, None], [None, FAILED, PARTIAL], [DEMONSTRATED, DEMONSTRATED, PARTIAL]], + "children", + [ + [DEMONSTRATED, NOT_DEMONSTRATED, None], + [None, NOT_DEMONSTRATED, PARTIAL], + [DEMONSTRATED, DEMONSTRATED, PARTIAL], + ], ) def test_combine_children_ignores_child_order(operator, children) -> None: """The result depends only on the set of child values, never on the order they are read in.""" diff --git a/tests/openedx_learning/applets/cbe/test_tag_rollup.py b/tests/openedx_learning/applets/cbe/test_tag_rollup.py index 7f1362e52..64cf7c290 100644 --- a/tests/openedx_learning/applets/cbe/test_tag_rollup.py +++ b/tests/openedx_learning/applets/cbe/test_tag_rollup.py @@ -28,7 +28,7 @@ DEMONSTRATED = MasteryStatus.DEMONSTRATED PARTIAL = MasteryStatus.PARTIALLY_ATTEMPTED -FAILED = MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED +NOT_DEMONSTRATED = MasteryStatus.ATTEMPTED_NOT_DEMONSTRATED NOW = datetime(2026, 1, 1, tzinfo=timezone.utc) @@ -42,7 +42,9 @@ def _object_id(name: str) -> str: return f"block-v1:Org1+Python100+Fall2026+problem+{name}" -def _earnable(tag: Tag, object_id: str, profile: CompetencyRuleProfile, *, archived: bool = False) -> SimpleNamespace: +def _add_requirement( + tag: Tag, object_id: str, profile: CompetencyRuleProfile, *, archived: bool = False, +) -> SimpleNamespace: """Give ``tag`` a root group with one criterion on ``object_id``, both archived when ``archived`` is set.""" root = CompetencyCriteriaGroup.objects.create(tag=tag, archived=archived) object_tag = ObjectTag.objects.create(object_id=object_id, taxonomy=tag.taxonomy, tag=tag) @@ -52,9 +54,10 @@ def _earnable(tag: Tag, object_id: str, profile: CompetencyRuleProfile, *, archi return SimpleNamespace(root=root, criterion=criterion, object_id=object_id) -def _record(user, earnable: SimpleNamespace, status: MasteryStatus) -> None: +def _record(user, requirement: SimpleNamespace, status: MasteryStatus) -> None: + """Store the learner's status on the requirement's criterion as the grade transaction would have.""" StudentCompetencyCriterionStatus.objects.create( - user=user, criterion=earnable.criterion, status_id=status, created=NOW, modified=NOW, + user=user, criterion=requirement.criterion, status_id=status, created=NOW, modified=NOW, ) @@ -94,10 +97,10 @@ def test_demonstrating_the_only_sub_competency_raises_the_parent_and_grandparent grandparent = _make_tag(competency_taxonomy, "Grandparent") parent = _make_tag(competency_taxonomy, "Parent", grandparent) child = _make_tag(competency_taxonomy, "Child", parent) - earnable = _earnable(child, _object_id("c"), default_rule_profile) - _record(user, earnable, DEMONSTRATED) + requirement = _add_requirement(child, _object_id("c"), default_rule_profile) + _record(user, requirement, DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, requirement.object_id) assert [_competency_status(user, tag) for tag in (child, parent, grandparent)] == [DEMONSTRATED] * 3 @@ -105,50 +108,50 @@ def test_demonstrating_the_only_sub_competency_raises_the_parent_and_grandparent def test_a_parent_whose_own_requirements_are_met_is_demonstrated_with_a_sub_competency_outstanding( user, family, default_rule_profile, ) -> None: - earnable = _earnable(family.children[0], _object_id("c1"), default_rule_profile) - _record(user, earnable, FAILED) - own = _earnable(family.parent, _object_id("p"), default_rule_profile) - _set_group_status(user, own.root, DEMONSTRATED) + requirement = _add_requirement(family.children[0], _object_id("c1"), default_rule_profile) + _record(user, requirement, NOT_DEMONSTRATED) + parent_requirement = _add_requirement(family.parent, _object_id("p"), default_rule_profile) + _set_group_status(user, parent_requirement.root, DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, requirement.object_id) assert _competency_status(user, family.parent) == DEMONSTRATED def test_a_parent_is_demonstrated_when_every_sub_competency_is(user, family, default_rule_profile) -> None: """The parent has its own requirements, which this learner has not attempted.""" - _earnable(family.parent, _object_id("p"), default_rule_profile) - earnables = [ - _earnable(child, _object_id(f"c{index}"), default_rule_profile) + _add_requirement(family.parent, _object_id("p"), default_rule_profile) + requirements = [ + _add_requirement(child, _object_id(f"c{index}"), default_rule_profile) for index, child in enumerate(family.children) ] - for earnable in earnables: - _record(user, earnable, DEMONSTRATED) + for requirement in requirements: + _record(user, requirement, DEMONSTRATED) - _roll_up(user, *(earnable.object_id for earnable in earnables)) + _roll_up(user, *(requirement.object_id for requirement in requirements)) assert _competency_status(user, family.parent) == DEMONSTRATED def test_a_parent_with_progress_from_either_source_is_partially_attempted(user, family, default_rule_profile) -> None: """The parent has its own requirements, which this learner has not attempted.""" - _earnable(family.parent, _object_id("p"), default_rule_profile) - earnable = _earnable(family.children[0], _object_id("c1"), default_rule_profile) - _record(user, earnable, DEMONSTRATED) - _earnable(family.children[1], _object_id("c2"), default_rule_profile) + _add_requirement(family.parent, _object_id("p"), default_rule_profile) + requirement = _add_requirement(family.children[0], _object_id("c1"), default_rule_profile) + _record(user, requirement, DEMONSTRATED) + _add_requirement(family.children[1], _object_id("c2"), default_rule_profile) - _roll_up(user, earnable.object_id) + _roll_up(user, requirement.object_id) assert _competency_status(user, family.parent) == PARTIAL def test_a_failed_own_root_group_shows_progress_on_the_parent(user, family, default_rule_profile) -> None: - earnable = _earnable(family.children[0], _object_id("c1"), default_rule_profile) - _record(user, earnable, FAILED) - own = _earnable(family.parent, _object_id("p"), default_rule_profile) - _set_group_status(user, own.root, FAILED) + requirement = _add_requirement(family.children[0], _object_id("c1"), default_rule_profile) + _record(user, requirement, NOT_DEMONSTRATED) + parent_requirement = _add_requirement(family.parent, _object_id("p"), default_rule_profile) + _set_group_status(user, parent_requirement.root, NOT_DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, requirement.object_id) assert _competency_status(user, family.parent) == PARTIAL @@ -156,12 +159,12 @@ def test_a_failed_own_root_group_shows_progress_on_the_parent(user, family, defa def test_sub_competencies_without_criteria_do_not_block_a_parent_with_its_own_requirements( user, family, default_rule_profile, ) -> None: - earnable = _earnable(family.children[0], _object_id("c1"), default_rule_profile) - _record(user, earnable, DEMONSTRATED) - own = _earnable(family.parent, _object_id("p"), default_rule_profile) - _set_group_status(user, own.root, DEMONSTRATED) + requirement = _add_requirement(family.children[0], _object_id("c1"), default_rule_profile) + _record(user, requirement, DEMONSTRATED) + parent_requirement = _add_requirement(family.parent, _object_id("p"), default_rule_profile) + _set_group_status(user, parent_requirement.root, DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, requirement.object_id) assert _competency_status(user, family.parent) == DEMONSTRATED @@ -170,12 +173,12 @@ def test_a_sub_competency_without_criteria_caps_a_parent_that_has_no_requirement user, competency_taxonomy, default_rule_profile, ) -> None: parent = _make_tag(competency_taxonomy, "Parent") - earnable_child = _make_tag(competency_taxonomy, "Earnable", parent) - _make_tag(competency_taxonomy, "Unearnable", parent) - earnable = _earnable(earnable_child, _object_id("c"), default_rule_profile) - _record(user, earnable, DEMONSTRATED) + child_with_requirement = _make_tag(competency_taxonomy, "With requirement", parent) + _make_tag(competency_taxonomy, "Without requirement", parent) + requirement = _add_requirement(child_with_requirement, _object_id("c"), default_rule_profile) + _record(user, requirement, DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, requirement.object_id) assert _competency_status(user, parent) == PARTIAL @@ -186,10 +189,10 @@ def test_a_sub_competency_with_criteria_only_further_down_still_participates( parent = _make_tag(competency_taxonomy, "Parent") middle = _make_tag(competency_taxonomy, "Middle", parent) leaf = _make_tag(competency_taxonomy, "Leaf", middle) - earnable = _earnable(leaf, _object_id("c"), default_rule_profile) - _record(user, earnable, DEMONSTRATED) + requirement = _add_requirement(leaf, _object_id("c"), default_rule_profile) + _record(user, requirement, DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, requirement.object_id) assert _competency_status(user, leaf) == DEMONSTRATED assert _competency_status(user, middle) == DEMONSTRATED @@ -202,20 +205,20 @@ def test_a_stored_demonstrated_parent_is_not_lowered_when_only_some_sub_competen StudentCompetencyStatus.objects.create( user=user, tag=family.parent, status_id=DEMONSTRATED, created=NOW, modified=NOW, ) - earnable = _earnable(family.children[0], _object_id("c1"), default_rule_profile) - _record(user, earnable, DEMONSTRATED) + requirement = _add_requirement(family.children[0], _object_id("c1"), default_rule_profile) + _record(user, requirement, DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, requirement.object_id) assert _competency_status(user, family.parent) == DEMONSTRATED def test_a_tag_at_the_taxonomy_root_ends_the_walk(user, competency_taxonomy, default_rule_profile) -> None: root_tag = _make_tag(competency_taxonomy, "Root") - earnable = _earnable(root_tag, _object_id("r"), default_rule_profile) - _record(user, earnable, DEMONSTRATED) + requirement = _add_requirement(root_tag, _object_id("r"), default_rule_profile) + _record(user, requirement, DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, requirement.object_id) assert list(StudentCompetencyStatus.objects.values_list("tag_id", flat=True)) == [root_tag.pk] @@ -224,10 +227,10 @@ def test_the_deepest_allowed_taxonomy_is_walked_to_the_top(user, competency_taxo tags = [_make_tag(competency_taxonomy, "Level 0")] for depth in range(1, 6): tags.append(_make_tag(competency_taxonomy, f"Level {depth}", tags[-1])) - earnable = _earnable(tags[-1], _object_id("deep"), default_rule_profile) - _record(user, earnable, DEMONSTRATED) + requirement = _add_requirement(tags[-1], _object_id("deep"), default_rule_profile) + _record(user, requirement, DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, requirement.object_id) assert _competency_status(user, tags[0]) == DEMONSTRATED assert _competency_status(user, tags[1]) == DEMONSTRATED @@ -249,11 +252,11 @@ def test_a_taxonomy_deeper_than_the_cap_logs_the_parents_left_unrecomputed( parent=tags[-2], depth=depth, lineage=tags[-2].lineage + tags[-1].value + "\t", ) tags[-1].refresh_from_db() - earnable = _earnable(tags[-1], _object_id("too-deep"), default_rule_profile) - _record(user, earnable, DEMONSTRATED) + requirement = _add_requirement(tags[-1], _object_id("too-deep"), default_rule_profile) + _record(user, requirement, DEMONSTRATED) with caplog.at_level(logging.ERROR, logger=rollup.__name__): - _roll_up(user, earnable.object_id) + _roll_up(user, requirement.object_id) assert _competency_status(user, tags[1]) == DEMONSTRATED assert _competency_status(user, tags[0]) is None @@ -266,17 +269,17 @@ def test_a_taxonomy_deeper_than_the_cap_logs_the_parents_left_unrecomputed( def test_two_graded_competencies_sharing_a_parent_recompute_that_parent_once( user, family, default_rule_profile, ) -> None: - earnables = [ - _earnable(child, _object_id(f"c{index}"), default_rule_profile) + requirements = [ + _add_requirement(child, _object_id(f"c{index}"), default_rule_profile) for index, child in enumerate(family.children[:2]) ] - for earnable in earnables: - _record(user, earnable, DEMONSTRATED) + for requirement in requirements: + _record(user, requirement, DEMONSTRATED) with mock.patch.object( rollup, "combine_parent_competency_status", wraps=rollup.combine_parent_competency_status, ) as combine: - _roll_up(user, *(earnable.object_id for earnable in earnables)) + _roll_up(user, *(requirement.object_id for requirement in requirements)) assert combine.call_count == 2 # the parent once, then the grandparent once @@ -290,14 +293,14 @@ def test_parents_at_the_same_level_are_rolled_up_independently( first = _make_tag(competency_taxonomy, "First", parent_two) second = _make_tag(competency_taxonomy, "Second", parent_two) graded = [ - _earnable(only_child, _object_id("only"), default_rule_profile), - _earnable(first, _object_id("first"), default_rule_profile), + _add_requirement(only_child, _object_id("only"), default_rule_profile), + _add_requirement(first, _object_id("first"), default_rule_profile), ] - _earnable(second, _object_id("second"), default_rule_profile) - for earnable in graded: - _record(user, earnable, DEMONSTRATED) + _add_requirement(second, _object_id("second"), default_rule_profile) + for requirement in graded: + _record(user, requirement, DEMONSTRATED) - _roll_up(user, *(earnable.object_id for earnable in graded)) + _roll_up(user, *(requirement.object_id for requirement in graded)) assert _competency_status(user, parent_one) == DEMONSTRATED assert _competency_status(user, parent_two) == PARTIAL @@ -311,13 +314,13 @@ def test_competencies_graded_at_different_depths_converge_on_the_shared_ancestor deep = _make_tag(competency_taxonomy, "Deep", middle) shallow = _make_tag(competency_taxonomy, "Shallow", top) graded = [ - _earnable(deep, _object_id("deep"), default_rule_profile), - _earnable(shallow, _object_id("shallow"), default_rule_profile), + _add_requirement(deep, _object_id("deep"), default_rule_profile), + _add_requirement(shallow, _object_id("shallow"), default_rule_profile), ] - for earnable in graded: - _record(user, earnable, DEMONSTRATED) + for requirement in graded: + _record(user, requirement, DEMONSTRATED) - _roll_up(user, *(earnable.object_id for earnable in graded)) + _roll_up(user, *(requirement.object_id for requirement in graded)) assert _competency_status(user, deep) == DEMONSTRATED assert _competency_status(user, middle) == DEMONSTRATED @@ -328,45 +331,49 @@ def test_competencies_graded_at_different_depths_converge_on_the_shared_ancestor def test_a_parent_with_its_own_requirements_is_demonstrated_when_a_sub_competency_is_fully_archived( user, family, default_rule_profile, ) -> None: - own = _earnable(family.parent, _object_id("p"), default_rule_profile) - _set_group_status(user, own.root, DEMONSTRATED) - _earnable(family.children[0], _object_id("c1"), default_rule_profile, archived=True) - earnable = _earnable(family.children[1], _object_id("c2"), default_rule_profile) - _record(user, earnable, DEMONSTRATED) + """The parent is demonstrated through its own requirement, whatever state its sub-competencies are in.""" + archived_child, graded_child, _child_without_requirements = family.children + parent_requirement = _add_requirement(family.parent, _object_id("p"), default_rule_profile) + _set_group_status(user, parent_requirement.root, DEMONSTRATED) + _add_requirement(archived_child, _object_id("c1"), default_rule_profile, archived=True) + graded_requirement = _add_requirement(graded_child, _object_id("c2"), default_rule_profile) + _record(user, graded_requirement, DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, graded_requirement.object_id) + assert _competency_status(user, graded_child) == DEMONSTRATED assert _competency_status(user, family.parent) == DEMONSTRATED - assert _competency_status(user, family.children[1]) == DEMONSTRATED def test_a_parent_with_no_requirements_of_its_own_stays_short_when_a_sub_competency_is_fully_archived( user, competency_taxonomy, default_rule_profile, ) -> None: + """The archived sub-competency can never be demonstrated, so the parent cannot have every sub-competency.""" parent = _make_tag(competency_taxonomy, "Parent") archived_child = _make_tag(competency_taxonomy, "Archived", parent) - live_child = _make_tag(competency_taxonomy, "Live", parent) - _earnable(archived_child, _object_id("a"), default_rule_profile, archived=True) - earnable = _earnable(live_child, _object_id("l"), default_rule_profile) - _record(user, earnable, DEMONSTRATED) + graded_child = _make_tag(competency_taxonomy, "Graded", parent) + _add_requirement(archived_child, _object_id("a"), default_rule_profile, archived=True) + graded_requirement = _add_requirement(graded_child, _object_id("g"), default_rule_profile) + _record(user, graded_requirement, DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, graded_requirement.object_id) - assert _competency_status(user, live_child) == DEMONSTRATED + assert _competency_status(user, graded_child) == DEMONSTRATED assert _competency_status(user, parent) == PARTIAL def test_an_archived_root_group_still_starts_the_parent_competency_walk( user, competency_taxonomy, default_rule_profile, ) -> None: + """A rollup for an archived requirement writes nothing for its own competency but still recomputes the parent.""" parent = _make_tag(competency_taxonomy, "Parent") archived_child = _make_tag(competency_taxonomy, "Archived", parent) - live_child = _make_tag(competency_taxonomy, "Live", parent) - earnable = _earnable(archived_child, _object_id("a"), default_rule_profile, archived=True) - _set_competency_status(user, live_child, DEMONSTRATED) - _record(user, earnable, DEMONSTRATED) + demonstrated_child = _make_tag(competency_taxonomy, "Demonstrated earlier", parent) + _set_competency_status(user, demonstrated_child, DEMONSTRATED) + archived_requirement = _add_requirement(archived_child, _object_id("a"), default_rule_profile, archived=True) + _record(user, archived_requirement, DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, archived_requirement.object_id) assert _competency_status(user, archived_child) is None assert _competency_status(user, parent) == PARTIAL @@ -375,12 +382,29 @@ def test_an_archived_root_group_still_starts_the_parent_competency_walk( def test_a_parents_own_archived_root_still_counts_through_its_stored_status( user, family, default_rule_profile, ) -> None: - own = _earnable(family.parent, _object_id("p"), default_rule_profile, archived=True) - _set_group_status(user, own.root, DEMONSTRATED) - earnable = _earnable(family.children[0], _object_id("c1"), default_rule_profile) - _record(user, earnable, FAILED) + """The parent demonstrated its own requirement before that requirement was archived, and keeps the result.""" + graded_child = family.children[0] + parent_requirement = _add_requirement(family.parent, _object_id("p"), default_rule_profile, archived=True) + _set_group_status(user, parent_requirement.root, DEMONSTRATED) + graded_requirement = _add_requirement(graded_child, _object_id("c1"), default_rule_profile) + _record(user, graded_requirement, NOT_DEMONSTRATED) - _roll_up(user, earnable.object_id) + _roll_up(user, graded_requirement.object_id) - assert _competency_status(user, family.children[0]) == PARTIAL + assert _competency_status(user, graded_child) == PARTIAL assert _competency_status(user, family.parent) == DEMONSTRATED + + +def test_a_parent_whose_only_sub_competency_has_no_status_gets_no_status( + user, competency_taxonomy, default_rule_profile, +) -> None: + """The parent has no requirement of its own and no sub-competency with a status, so there is nothing to write.""" + parent = _make_tag(competency_taxonomy, "Parent") + archived_child = _make_tag(competency_taxonomy, "Archived", parent) + archived_requirement = _add_requirement(archived_child, _object_id("a"), default_rule_profile, archived=True) + _record(user, archived_requirement, DEMONSTRATED) + + _roll_up(user, archived_requirement.object_id) + + assert _competency_status(user, archived_child) is None + assert _competency_status(user, parent) is None From 493dcb900537df9dc9b2176562252dd93df1c5f8 Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Thu, 12 Nov 2026 23:02:28 +0400 Subject: [PATCH 3/4] test: cover rollup edge cases for depth, learners, and query count Add four tests for the competency rollup. Root groups reached at different depths of the group walk each start their own parent-competency walk. Another learner's criterion, root group, and competency statuses do not count toward the learner being rolled up. A repeated rollup of nested groups under a parent competency runs a fixed number of queries. A status on a nested group in a parent competency's own tree does not count as the parent's own result. Related to https://github.com/openedx/openedx-core/issues/815 Co-Authored-By: Claude Fable 5.2 --- .../applets/cbe/test_tag_rollup.py | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/tests/openedx_learning/applets/cbe/test_tag_rollup.py b/tests/openedx_learning/applets/cbe/test_tag_rollup.py index 64cf7c290..645ccca76 100644 --- a/tests/openedx_learning/applets/cbe/test_tag_rollup.py +++ b/tests/openedx_learning/applets/cbe/test_tag_rollup.py @@ -7,6 +7,7 @@ from unittest import mock import pytest +from django.contrib.auth import get_user_model from openedx_learning.api import roll_up_competency_statuses from openedx_learning.applets.cbe import rollup @@ -54,6 +55,19 @@ def _add_requirement( return SimpleNamespace(root=root, criterion=criterion, object_id=object_id) +def _add_nested_requirement( + tag: Tag, object_id: str, profile: CompetencyRuleProfile, *, levels: int, +) -> SimpleNamespace: + """Give ``tag`` a root group and one criterion on ``object_id`` that sits ``levels`` groups below the root.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + group = root + for _ in range(levels): + group = CompetencyCriteriaGroup.objects.create(tag=tag, parent=group) + object_tag = ObjectTag.objects.create(object_id=object_id, taxonomy=tag.taxonomy, tag=tag) + criterion = CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=profile) + return SimpleNamespace(root=root, group=group, criterion=criterion, object_id=object_id) + + def _record(user, requirement: SimpleNamespace, status: MasteryStatus) -> None: """Store the learner's status on the requirement's criterion as the grade transaction would have.""" StudentCompetencyCriterionStatus.objects.create( @@ -408,3 +422,79 @@ def test_a_parent_whose_only_sub_competency_has_no_status_gets_no_status( assert _competency_status(user, archived_child) is None assert _competency_status(user, parent) is None + + +def test_root_groups_reached_at_different_depths_each_start_their_own_parent_walk( + user, competency_taxonomy, default_rule_profile, +) -> None: + """The shallow root is reached on the first level of the group walk and the deep root two levels later.""" + shallow_parent = _make_tag(competency_taxonomy, "Shallow parent") + shallow = _make_tag(competency_taxonomy, "Shallow", shallow_parent) + deep_parent = _make_tag(competency_taxonomy, "Deep parent") + deep = _make_tag(competency_taxonomy, "Deep", deep_parent) + graded = [ + _add_requirement(shallow, _object_id("shallow"), default_rule_profile), + _add_nested_requirement(deep, _object_id("deep"), default_rule_profile, levels=2), + ] + for requirement in graded: + _record(user, requirement, DEMONSTRATED) + + _roll_up(user, *(requirement.object_id for requirement in graded)) + + assert _competency_status(user, shallow_parent) == DEMONSTRATED + assert _competency_status(user, deep_parent) == DEMONSTRATED + + +def test_another_learners_statuses_do_not_count_toward_the_rolled_up_learner( + user, competency_taxonomy, default_rule_profile, +) -> None: + """The other learner holds a status at every level that the rollup reads: criterion, root group, competency.""" + other_learner = get_user_model().objects.create(username="other_learner") + parent = _make_tag(competency_taxonomy, "Parent") + graded_child = _make_tag(competency_taxonomy, "Graded", parent) + other_child = _make_tag(competency_taxonomy, "Other", parent) + parent_requirement = _add_requirement(parent, _object_id("p"), default_rule_profile) + graded_requirement = _add_requirement(graded_child, _object_id("graded"), default_rule_profile) + other_requirement = _add_requirement(other_child, _object_id("other"), default_rule_profile) + _record(user, graded_requirement, DEMONSTRATED) + _record(other_learner, other_requirement, DEMONSTRATED) + _set_group_status(other_learner, parent_requirement.root, DEMONSTRATED) + _set_competency_status(other_learner, other_child, DEMONSTRATED) + + _roll_up(user, graded_requirement.object_id, other_requirement.object_id) + + assert _competency_status(user, graded_child) == DEMONSTRATED + assert _competency_status(user, other_child) is None + assert _competency_status(user, parent) == PARTIAL + assert _competency_status(other_learner, parent) is None + + +def test_a_repeated_rollup_of_nested_groups_under_a_parent_competency_runs_a_fixed_number_of_queries( + user, competency_taxonomy, default_rule_profile, django_assert_num_queries, +) -> None: + """The first rollup creates every status row, so the repeated one only reads and compares.""" + parent = _make_tag(competency_taxonomy, "Parent") + child = _make_tag(competency_taxonomy, "Child", parent) + requirement = _add_nested_requirement(child, _object_id("c"), default_rule_profile, levels=1) + _record(user, requirement, DEMONSTRATED) + _roll_up(user, requirement.object_id) + + with django_assert_num_queries(22): + _roll_up(user, requirement.object_id) + + +def test_a_nested_group_in_a_parents_own_tree_does_not_count_as_the_parents_own_result( + user, competency_taxonomy, default_rule_profile, +) -> None: + """Only a root group's status is the parent's own result, and this learner has none on the parent's root.""" + parent = _make_tag(competency_taxonomy, "Parent") + child = _make_tag(competency_taxonomy, "Child", parent) + parent_requirement = _add_nested_requirement(parent, _object_id("p"), default_rule_profile, levels=1) + _set_group_status(user, parent_requirement.group, DEMONSTRATED) + child_requirement = _add_requirement(child, _object_id("c"), default_rule_profile) + _record(user, child_requirement, NOT_DEMONSTRATED) + + _roll_up(user, child_requirement.object_id) + + assert _competency_status(user, child) == PARTIAL + assert _competency_status(user, parent) == PARTIAL From 5043c6d4cf6b04cea85c4693333dd583eb2eff81 Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Thu, 12 Nov 2026 23:28:43 +0400 Subject: [PATCH 4/4] fix: raise a competency from its root group's stored status A rollup raises a competency's status from the status stored for its root group, whether or not the root is archived. A stored root status can be higher than the value recomputed from the root's children: a rollup can stop between the root write and the competency write, and the child that earned the root's status can be archived before the retry. Reading the stored status makes the retry bring the competency up to the root's status. The read is one more query per root group in a rollup. ADR-0002 states the rule in Decision 6, and the description of the group's archived column names the archived root as the case where an archived group's stored status still counts. Related to https://github.com/openedx/openedx-core/issues/815 Co-Authored-By: Claude Fable 5.2 --- .../0002-competency-criteria-model.rst | 8 +++--- src/openedx_learning/applets/cbe/rollup.py | 28 ++++++++----------- .../applets/cbe/test_rollup_api.py | 17 +++++++++++ .../applets/cbe/test_tag_rollup.py | 2 +- 4 files changed, 33 insertions(+), 22 deletions(-) diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index 451d8e817..a3acf3dd1 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -87,7 +87,7 @@ Decision 5. ``name``: string 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. 7. ``logic_operator``: Either “AND” or “OR” or null. This determines how children are combined at a group node ("AND" or "OR"). - 8. ``archived``: Boolean, defaults to false. Set instead of deleting a group once a student status exists for it. Archived groups are hidden from authoring and new associations but remain queryable, so existing ``CompetencyCriteria`` rows and learner status rows stay resolvable. For MVP, archiving a group or a criterion is permanent, with the root as the one reversible exception: a new criterion for an archived root's competency reactivates that root instead of creating a second one, because archiving it for good would remove the ability to ever associate criteria with that competency again. Archiving a course-level group blocks that course for that competency; archiving a leaf closes only that leaf to new criteria. More unarchival capabilities may be added after MVP. Archived groups do not count toward learner statuses (Decision 6). + 8. ``archived``: Boolean, defaults to false. Set instead of deleting a group once a student status exists for it. Archived groups are hidden from authoring and new associations but remain queryable, so existing ``CompetencyCriteria`` rows and learner status rows stay resolvable. For MVP, archiving a group or a criterion is permanent, with the root as the one reversible exception: a new criterion for an archived root's competency reactivates that root instead of creating a second one, because archiving it for good would remove the ability to ever associate criteria with that competency again. Archiving a course-level group blocks that course for that competency; archiving a leaf closes only that leaf to new criteria. More unarchival capabilities may be added after MVP. Archived groups do not count when a group's status is combined from its children, and an archived root's stored status still raises its competency's row and counts toward its parent competency (Decision 6). 9. ``scope_code``: Derived from ``oel_tagging_tag_id``, set on the root group and null on every other group, whether or not the root is archived. It exists so the database can hold at most one root per competency, the same way Decision 3's derived ``scope_code`` holds at most one live profile per scope. Example: A root group uses "OR" with two child groups. @@ -270,7 +270,7 @@ Decision - When a root group is recomputed, write the competency's ``StudentCompetencyStatus`` from it, mapping ``AttemptedNotDemonstrated`` to ``PartiallyAttempted`` because that table holds only ``Demonstrated`` and ``PartiallyAttempted``. - Then recompute each parent competency up the taxonomy, one level at a time. A parent's status is the better of two inputs: its own criteria result, which is its root group's status mapped the same way, and its sub-competency result, which is ``Demonstrated`` when every direct sub-competency is ``Demonstrated`` and ``PartiallyAttempted`` when at least one has a status. A competency with no criteria tree of its own gets a status row only through its sub-competencies; a competency with neither a root group nor a sub-competency with a status gets no row. - Every write only ever raises a stored status, and each level commits before the next is read. :ref:`openedx-learning-adr-0004` records why. - - An archived criterion is not evaluated when a grade is recorded, and a group is recomputed from its non-archived children only. Neither archiving nor the rollup lowers a stored status, including a status that a non-archived group or a competency earned through a child that is archived. Rows of archived criteria and groups are left unchanged. A status stored against an archived root group raises its competency's row and counts toward its parent competency. Archiving recomputes nothing itself: a learner's statuses reflect it the next time a grade raises a non-archived criterion's status in that group's subtree, or when an operator recomputes them. + - An archived criterion is not evaluated when a grade is recorded, and a group is recomputed from its non-archived children only. Neither archiving nor the rollup lowers a stored status, including a status that a non-archived group or a competency earned through a child that is archived. Rows of archived criteria and groups are left unchanged. A competency's row is raised from the status stored for its root group, whether or not the root is archived, and that stored status counts toward its parent competency. Archiving recomputes nothing itself: a learner's statuses reflect it the next time a grade raises a non-archived criterion's status in that group's subtree, or when an operator recomputes them. 1. Add new database table for ``CompetencyMasteryStatuses`` with these columns: @@ -496,5 +496,5 @@ Changelog * Amended Decision 6 for issue #815: a group is recomputed from its non-archived children only, so an archived requirement or branch does not have to be met. A status recorded against an - archived criterion or group is kept. An archived root's stored status raises its competency's - row and counts toward its parent competency. Archiving by itself recomputes nothing. + archived criterion or group is kept. A root's stored status, archived or not, raises its + competency's row and counts toward its parent competency. Archiving by itself recomputes nothing. diff --git a/src/openedx_learning/applets/cbe/rollup.py b/src/openedx_learning/applets/cbe/rollup.py index e8145de2e..ab299dfde 100644 --- a/src/openedx_learning/applets/cbe/rollup.py +++ b/src/openedx_learning/applets/cbe/rollup.py @@ -155,32 +155,26 @@ def _raise_competency_status(user_id: int, tag_id: int, status: MasteryStatus | ) -def _recompute_group( - user_id: int, group: CompetencyCriteriaGroup, child_statuses: list[MasteryStatus | None], now: datetime, -) -> MasteryStatus | None: - """Raise the group's stored status to its children's combined result and return that result.""" - status = combine_children(group.logic_operator, child_statuses) - _raise_group_status(user_id, group.pk, status, now) - return status +def _raise_competency_from_root(user_id: int, root: CompetencyCriteriaGroup, now: datetime) -> None: + """Raise the competency's status from the status stored for its root group; no stored status writes nothing.""" + status = _stored_group_status(user_id, root.pk) + if status is not None: + _raise_competency_status(user_id, root.tag_id, _competency_level(status), now) def _roll_up_group( user_id: int, group: CompetencyCriteriaGroup, child_statuses: list[MasteryStatus | None], now: datetime, ) -> None: """ - Recompute one group, and for a root group raise its competency's status from the root's status. + Recompute one group, and for a root group raise its competency's status from the root's stored status. - An archived group is not recomputed, and an archived root passes on the status it already holds. + An archived group is not recomputed. """ - is_root = group.parent_id is None if not group.archived: - status = _recompute_group(user_id, group, child_statuses, now) - elif is_root: - status = _stored_group_status(user_id, group.pk) - else: - status = None - if is_root and status is not None: - _raise_competency_status(user_id, group.tag_id, _competency_level(status), now) + _raise_group_status(user_id, group.pk, combine_children(group.logic_operator, child_statuses), now) + # The stored status can be higher than this recomputation, and the competency has to match it. + if group.parent_id is None: + _raise_competency_from_root(user_id, group, now) def _roll_up_group_level(user_id: int, group_ids: set[int], now: datetime) -> list[CompetencyCriteriaGroup]: diff --git a/tests/openedx_learning/applets/cbe/test_rollup_api.py b/tests/openedx_learning/applets/cbe/test_rollup_api.py index e6d951681..9375a1feb 100644 --- a/tests/openedx_learning/applets/cbe/test_rollup_api.py +++ b/tests/openedx_learning/applets/cbe/test_rollup_api.py @@ -573,6 +573,23 @@ def test_an_archived_root_raises_its_competency_from_the_stored_root_status(user assert _competency_status(user, tag) == DEMONSTRATED +def test_a_root_raises_its_competency_from_the_stored_root_status_after_the_earning_route_is_archived( + user, tag, or_tree, +) -> None: + """A rollup that stopped between the root write and the competency write is repaired from the stored root status.""" + _record(user, or_tree.first, DEMONSTRATED) + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=or_tree.root, status_id=DEMONSTRATED, created=NOW, modified=NOW, + ) + _archive(or_tree.route_1) + _record(user, or_tree.second, NOT_DEMONSTRATED) + + _roll_up(user, ASSIGNMENT_2) + + assert _group_status(user, or_tree.root) == DEMONSTRATED + assert _competency_status(user, tag) == DEMONSTRATED + + def test_the_walk_continues_past_an_archived_group_to_its_parent(user, tag, default_rule_profile) -> None: root = _make_group(tag) archived_leaf = _make_group(tag, parent=root, logic_operator=LogicOperator.AND, archived=True) diff --git a/tests/openedx_learning/applets/cbe/test_tag_rollup.py b/tests/openedx_learning/applets/cbe/test_tag_rollup.py index 645ccca76..20a99e704 100644 --- a/tests/openedx_learning/applets/cbe/test_tag_rollup.py +++ b/tests/openedx_learning/applets/cbe/test_tag_rollup.py @@ -479,7 +479,7 @@ def test_a_repeated_rollup_of_nested_groups_under_a_parent_competency_runs_a_fix _record(user, requirement, DEMONSTRATED) _roll_up(user, requirement.object_id) - with django_assert_num_queries(22): + with django_assert_num_queries(23): _roll_up(user, requirement.object_id)