From 5e6ccf5eb2f5c69331d7338c303b61f528a0d9cd Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Tue, 6 Oct 2026 17:00:56 +0300 Subject: [PATCH 01/24] feat: add endpoint to delete or archive a CompetencyCriterion (#674) DELETE competencies//criteria// hard-deletes a CompetencyCriterion when no StudentCompetencyCriterionStatus references it, or archives it in place otherwise. On the hard-delete path, the now-unreferenced ObjectTag is untagged via the same read-merge-write tag_object() creation already uses, but only once no other CompetencyCriterion still shares it. The archive path never touches the ObjectTag. An ancestor CompetencyCriteriaGroup left with no live children is cascaded in turn (hard-deleted or archived, per the same learner-status rule, stopping at the first ancestor that still has live content), so a removal never leaves an empty group behind per ADR-0002 Decision 2. The view checks oel_tagging.can_tag_object the same way the create view does, and delete_competency_criterion() leaves that check to its caller, like associate_competency_criterion(). The create endpoint's response now includes the criterion's archived flag. Co-Authored-By: Claude Sonnet 5 Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 82 +++++- .../applets/cbe/rest_api/v1/serializers.py | 5 +- .../applets/cbe/rest_api/v1/urls.py | 5 + .../applets/cbe/rest_api/v1/views.py | 35 +++ .../applets/cbe/test_criterion_removal.py | 252 ++++++++++++++++++ .../applets/cbe/test_views.py | 151 ++++++++++- 6 files changed, 526 insertions(+), 4 deletions(-) create mode 100644 tests/openedx_learning/applets/cbe/test_criterion_removal.py diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 29205bb0d..a14166636 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -7,7 +7,7 @@ from dataclasses import dataclass from datetime import datetime, timezone from decimal import Decimal -from typing import Any, Iterable, Sequence +from typing import Any, Iterable, Literal, Sequence from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError from django.db import connection, transaction @@ -32,6 +32,7 @@ CompetencyTaxonomy, LogicOperator, MasteryStatus, + StudentCompetencyCriteriaGroupStatus, StudentCompetencyCriterionStatus, ) from .rollup import roll_up_competency_statuses @@ -47,6 +48,7 @@ "bulk_update_competency_criteria", "create_competency_criterion", "create_competency_taxonomy", + "delete_competency_criterion", "get_competency_criteria_tree", "get_competency_rule_profiles", "get_courses_with_relative_criteria", @@ -917,3 +919,81 @@ def update_competency_criteria_group( # Never QuerySet.update(): it skips post_save, so simple_history would record nothing. group.save(update_fields=["logic_operator"]) return group + + +@dataclass(frozen=True) +class CriterionDeletionResult: + """The outcome of :func:`delete_competency_criterion`: whether the row was hard-deleted or archived.""" + + id: int + archived: bool + + +def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", "archive"]) -> None: + """ + Walk `group` and its ancestors, collapsing/archiving any left with no live children. + + `group.criteria` only has rows at the true-leaf level, so checking it at every level + unconditionally is harmless (an empty queryset above the leaf) and avoids a separate + leaf/non-leaf code path. + """ + has_live_child = ( + group.criteria.filter(archived=False).exists() or group.child_groups.filter(archived=False).exists() + ) + parent = group.parent + if has_live_child: + return + if event == "delete" and not group.criteria.exists() and not group.child_groups.exists(): + if StudentCompetencyCriteriaGroupStatus.objects.filter(group_id=group.id).exists(): + group.archived = True + group.save() + next_event: Literal["delete", "archive"] = "archive" + else: + group.delete() + next_event = "delete" + else: + # Only archived children remain. + group.archived = True + group.save() + next_event = "archive" + if parent is not None: + _cascade_from_group(parent, next_event) + + +def delete_competency_criterion(criterion_id: int) -> CriterionDeletionResult: + """ + Hard-delete `criterion_id`, or archive it if a learner status row already references it. + + UNSTABLE: the criteria-authoring family is incomplete, so its validation and group update + and archive behavior may change this function's shape without a deprecation cycle. + + The caller is expected to have already checked ``oel_tagging.can_tag_object`` for the + criterion's object, the same contract as :func:`associate_competency_criterion`. + + An ancestor :class:`CompetencyCriteriaGroup` left with no live children by this call is + cascaded in turn: hard-deleted if it carries no group-level status of its own, archived + otherwise. See ADR-0002 Decision 2. + + Raises CompetencyCriterion.DoesNotExist if no such criterion exists. + """ + criterion = CompetencyCriterion.objects.get(pk=criterion_id) + + has_status = StudentCompetencyCriterionStatus.objects.filter(criterion_id=criterion_id).exists() + if not has_status: + with transaction.atomic(): + group = criterion.group + object_tag = criterion.object_tag + criterion.delete() + if not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists(): + existing_values = [ + t.value for t in get_object_tags(object_tag.object_id, taxonomy_id=object_tag.taxonomy_id) + if t.value != group.tag.value + ] + tag_object(object_tag.object_id, group.tag.taxonomy, existing_values) + _cascade_from_group(group, "delete") + return CriterionDeletionResult(id=criterion_id, archived=False) + + criterion.archived = True + criterion.save() + _cascade_from_group(criterion.group, "archive") + return CriterionDeletionResult(id=criterion_id, archived=True) diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py index 57376ee8f..a42110a23 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py @@ -82,14 +82,15 @@ class CompetencyCriterionSerializer(serializers.ModelSerializer): ) rule_profile_id = serializers.IntegerField(required=False, allow_null=True) object_tag_id = serializers.IntegerField(read_only=True) + archived = serializers.BooleanField(read_only=True) class Meta: model = CompetencyCriterion fields = [ "id", "object_id", "group_id", "logic_operator", - "rule_profile_id", "rule_type_override", "rule_payload_override", "object_tag_id", + "rule_profile_id", "rule_type_override", "rule_payload_override", "object_tag_id", "archived", ] - read_only_fields = ["id", "object_tag_id"] + read_only_fields = ["id", "object_tag_id", "archived"] def validate_object_id(self, value: str) -> str: """Reject an object_id that isn't a parseable usage key.""" diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py index e96d754a1..19ee778ca 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py @@ -32,4 +32,9 @@ views.CompetencyCriteriaGroupDetailView.as_view(), name="criteria-group-detail", ), + path( + "competencies//criteria//", + views.CompetencyCriterionDeleteView.as_view(), + name="criterion-delete", + ), ] diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/views.py b/src/openedx_learning/applets/cbe/rest_api/v1/views.py index d7c3a4d8c..024dc0ba4 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -26,6 +26,7 @@ CompetencyCriterionArchivedError, associate_competency_criterion, bulk_update_competency_criteria, + delete_competency_criterion, get_competency_criteria_tree, get_competency_rule_profiles, resolve_competency_tag, @@ -273,3 +274,37 @@ def patch(self, request, tag_id: int, group_id: int): except CompetencyCriteriaGroupArchivedError as exc: raise CompetencyCriteriaConflict(str(exc)) from exc return Response(CompetencyCriteriaGroupSerializer(group).data, status=status.HTTP_200_OK) + + +class CompetencyCriterionDeleteView(generics.GenericAPIView): + """ + DELETE-only. Remove a CompetencyCriterion: hard-delete, or archive if learner status exists. + + Does not call self.check_object_permissions(): oel_tagging.can_tag_object is checked inline + with a manual ``has_perm()`` call, the same pattern CompetencyCriterionCreateView uses. + """ + + serializer_class = CompetencyCriterionSerializer + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = [IsAuthenticated] + + def delete(self, request, *args, **kwargs): + """ + Resolve the criterion scoped to both path segments, check permission, then delegate to the public API. + + Scoped by tag_id rather than a specific group_id, matching CompetencyCriterionCreateView's + own URL convention; a group-scoped delete is a separate ticket's concern (direct group + deletion/archival). The lookup deliberately does not filter `archived=False`, so a repeat + DELETE against an already-archived row still resolves (200), and a mismatched tag_id or a + nonexistent criterion_id both 404 the same way, via this one get_object_or_404 call. + """ + criterion = get_object_or_404( + CompetencyCriterion, pk=self.kwargs["criterion_id"], group__tag_id=self.kwargs["tag_id"], + ) + perm_obj = ObjectTagPermissionItem( + taxonomy=criterion.group.tag.taxonomy, object_id=criterion.object_tag.object_id, + ) + if not request.user.has_perm("oel_tagging.can_tag_object", perm_obj): + raise PermissionDenied() + result = delete_competency_criterion(criterion.id) + return Response({"id": result.id, "archived": result.archived}, status=status.HTTP_200_OK) diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py new file mode 100644 index 000000000..878403b37 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -0,0 +1,252 @@ +"""Tests for delete_competency_criterion() and its ancestor-cascade helper, _cascade_from_group().""" +from datetime import datetime + +import pytest + +from openedx_learning.api import delete_competency_criterion +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + MasteryStatus, + StudentCompetencyCriteriaGroupStatus, + StudentCompetencyCriterionStatus, +) +from openedx_tagging.models import ObjectTag, Tag + +pytestmark = pytest.mark.django_db + + +def _make_object_tag(competency_taxonomy: CompetencyTaxonomy, tag: Tag, object_id: str) -> ObjectTag: + """Create an ObjectTag for `object_id`, distinct from the conftest `object_tag` fixture's own row.""" + return ObjectTag.objects.create(object_id=object_id, taxonomy=competency_taxonomy, tag=tag) + + +def _make_criterion( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> CompetencyCriterion: + """Create a CompetencyCriterion under `group`, on the system-default rule profile.""" + return CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=default_rule_profile) + + +# ================================================================================================= +# Hard-delete vs. archive, on the criterion itself +# ================================================================================================= + + +def test_hard_delete_with_no_status_removes_the_row_and_its_object_tag( + tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Deleting a criterion with no learner status hard-deletes it and drops its now-unreferenced + object_tag, returning archived=False. + """ + group = CompetencyCriteriaGroup.objects.create(tag=tag) + criterion = _make_criterion(group, object_tag, default_rule_profile) + + result = delete_competency_criterion(criterion.id) + + assert result.id == criterion.id + assert result.archived is False + assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() + assert not ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_archive_path_leaves_the_row_and_its_object_tag_in_place( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + Deleting a criterion that a learner status row already references archives it instead of + hard-deleting it: the row and its object_tag both survive, and archived=True comes back. + """ + group = CompetencyCriteriaGroup.objects.create(tag=tag) + criterion = _make_criterion(group, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_competency_criterion(criterion.id) + + assert result.archived is True + criterion.refresh_from_db() + assert criterion.archived is True + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_repeat_call_on_an_already_archived_criterion_is_idempotent( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """A second DELETE against an already-archived criterion re-enters the archive branch without error.""" + group = CompetencyCriteriaGroup.objects.create(tag=tag) + criterion = _make_criterion(group, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + delete_competency_criterion(criterion.id) + + result = delete_competency_criterion(criterion.id) + + assert result.id == criterion.id + assert result.archived is True + criterion.refresh_from_db() + assert criterion.archived is True + + +def test_shared_object_tag_is_untouched_while_another_criterion_still_references_it( + tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Two criteria, in two different groups, referencing the same object_tag (a deliberate second + association per ADR-0002): hard-deleting one leaves the other, and the shared object_tag, intact. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + group_a = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + group_b = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion_a = _make_criterion(group_a, object_tag, default_rule_profile) + criterion_b = _make_criterion(group_b, object_tag, default_rule_profile) + + delete_competency_criterion(criterion_a.id) + + assert not CompetencyCriterion.objects.filter(id=criterion_a.id).exists() + assert CompetencyCriterion.objects.filter(id=criterion_b.id).exists() + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +# ================================================================================================= +# The ancestor cascade, on hard-delete +# ================================================================================================= + + +def test_single_child_leaf_group_cascade_deletes_while_a_sibling_keeps_the_root_alive( + competency_taxonomy: CompetencyTaxonomy, tag: Tag, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Hard-deleting a criterion that was its leaf group's only child also deletes that now-empty + group, but the cascade stops there: a sibling group under the same root, with its own live + criterion, keeps the root from being touched. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + sibling = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + sibling_object_tag = _make_object_tag(competency_taxonomy, tag, "obj-sibling") + _make_criterion(sibling, sibling_object_tag, default_rule_profile) + + delete_competency_criterion(criterion.id) + + assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=sibling.id).exists() + + +def test_multilevel_cascade_fully_collapses_the_root_to_leaf_chain( + tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Hard-deleting the only criterion in a root -> mid -> leaf chain, with nothing else anywhere in + the tree, collapses every group in the chain, one level at a time, all the way to the root. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + mid = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=mid) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + + delete_competency_criterion(criterion.id) + + assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + assert not CompetencyCriteriaGroup.objects.filter(id=mid.id).exists() + assert not CompetencyCriteriaGroup.objects.filter(id=root.id).exists() + + +def test_cascade_stops_at_an_ancestor_with_a_surviving_sibling_group( + competency_taxonomy: CompetencyTaxonomy, tag: Tag, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A deeper chain (root -> branch -> leaf) collapses branch and leaf when the leaf's only + criterion is hard-deleted, but stops at root: a second branch under root, with its own live + criterion, keeps root from being touched. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + branch = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=branch) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + other_branch = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + other_object_tag = _make_object_tag(competency_taxonomy, tag, "obj-other-branch") + other_criterion = _make_criterion(other_branch, other_object_tag, default_rule_profile) + + delete_competency_criterion(criterion.id) + + assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + assert not CompetencyCriteriaGroup.objects.filter(id=branch.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=other_branch.id).exists() + assert CompetencyCriterion.objects.filter(id=other_criterion.id).exists() + + +def test_group_with_its_own_status_is_archived_not_deleted_at_zero_remaining_children( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + A leaf group carrying its own StudentCompetencyCriteriaGroupStatus is archived, not + hard-deleted, even once its last criterion is gone -- while that criterion itself, with no + status of its own, is still hard-deleted. Archiving the leaf then cascades upward as + "archive", so an ancestor with no other content is archived too, not deleted. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=leaf, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_competency_criterion(criterion.id) + + assert result.archived is False + assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() + leaf.refresh_from_db() + assert leaf.archived is True + root.refresh_from_db() + assert root.archived is True + + +# ================================================================================================= +# The ancestor cascade, on archive +# ================================================================================================= + + +def test_archiving_a_groups_only_criterion_archives_the_group_and_cascades_upward( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + Archiving a criterion that was its leaf group's only live child archives that group too, and + the cascade continues upward through an ancestor with no other active content. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + delete_competency_criterion(criterion.id) + + leaf.refresh_from_db() + root.refresh_from_db() + assert leaf.archived is True + assert root.archived is True + assert CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() + + +# ================================================================================================= +# Not found +# ================================================================================================= + + +def test_does_not_exist_for_an_unknown_criterion_id() -> None: + """A criterion_id with no matching row raises CompetencyCriterion.DoesNotExist.""" + with pytest.raises(CompetencyCriterion.DoesNotExist): + delete_competency_criterion(999999999) diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index be957f857..6817e3d20 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -4,6 +4,7 @@ Fixtures live in this directory's conftest.py. Several scenarios here create rule profile rows directly rather than through a live call, because no create or archive endpoint exists yet. """ +from datetime import datetime from typing import Any, Generator from urllib.parse import urlencode @@ -24,7 +25,9 @@ CompetencyRuleProfile, CompetencyTaxonomy, LogicOperator, + MasteryStatus, RuleType, + StudentCompetencyCriterionStatus, ) from openedx_tagging.models import ObjectTag, Tag, Taxonomy @@ -73,6 +76,11 @@ def criterion_create_url(tag_id: int) -> str: return reverse("cbe:criterion-create", kwargs={"tag_id": tag_id}) +def criterion_delete_url(tag_id: int, criterion_id: int) -> str: + """Return the delete-criterion endpoint's path for `tag_id`/`criterion_id`, resolved through the URL name.""" + return reverse("cbe:criterion-delete", kwargs={"tag_id": tag_id, "criterion_id": criterion_id}) + + def usage_key(course_run: CourseRun, block_id: str) -> str: """Build a gradeable-subsection-shaped usage key string under `course_run`.""" key = course_run.course_key @@ -822,7 +830,9 @@ def test_tag_refused_at_the_object_tag_limit_is_400_and_creates_nothing( NEW_PAYLOAD = {"op": "gte", "value": 0.75, "scale": "percent"} NEW_RULE: dict[str, Any] = {"rule_type_override": "Grade", "rule_payload_override": NEW_PAYLOAD} -CRITERION_FIELDS = {"id", "group_id", "rule_profile_id", "rule_type_override", "rule_payload_override", "object_tag_id"} +CRITERION_FIELDS = { + "id", "group_id", "rule_profile_id", "rule_type_override", "rule_payload_override", "object_tag_id", "archived", +} def bulk_update_url(group_id: int) -> str: @@ -1993,3 +2003,142 @@ def test_group_detail_offers_only_patch( response = getattr(user_client, method)(group_detail_url(tag.id, leaf.id), {}, format="json") assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED + + +# ============================================================================================== +# CompetencyCriterionDeleteView (#674) +# ============================================================================================== + + +@pytest.fixture(name="criterion") +def _criterion( + group: CompetencyCriteriaGroup, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile +) -> CompetencyCriterion: + """A leaf-agnostic CompetencyCriterion directly under the root `group`, for the delete-view tests.""" + # The conftest object_tag's object_id isn't a parseable usage key, which _allow_object_level_tagging denies. + object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) + return CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=default_rule_profile) + + +def test_create_response_reports_archived_false(user_client: APIClient, tag: Tag, course_run: CourseRun) -> None: + """The create endpoint's response carries the read-only archived flag, false for a new criterion.""" + response = user_client.post(criterion_create_url(tag.id), {"object_id": usage_key(course_run, "p1")}, format="json") + + assert response.status_code == status.HTTP_201_CREATED + assert response.data["archived"] is False + + +def test_hard_delete_200s_with_archived_false(user_client: APIClient, criterion: CompetencyCriterion) -> None: + """Deleting a criterion with no learner status 200s, hard-deletes it, and reports archived=false.""" + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data == {"id": criterion.id, "archived": False} + assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() + + +def test_archive_200s_with_archived_true( + user_client: APIClient, user, criterion: CompetencyCriterion, now: datetime, +) -> None: + """Deleting a criterion a learner status row references 200s, archives it, and reports archived=true.""" + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data == {"id": criterion.id, "archived": True} + criterion.refresh_from_db() + assert criterion.archived is True + + +def test_repeat_delete_on_an_archived_criterion_is_idempotent_200( + user_client: APIClient, user, criterion: CompetencyCriterion, now: datetime, +) -> None: + """A second DELETE against an already-archived criterion still 200s with the same body.""" + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data == {"id": criterion.id, "archived": True} + + +def test_unknown_criterion_id_404s(user_client: APIClient, tag: Tag) -> None: + """A criterion_id with no matching row 404s.""" + response = user_client.delete(criterion_delete_url(tag.id, 999999)) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_repeat_delete_after_hard_delete_404s( + user_client: APIClient, criterion: CompetencyCriterion, +) -> None: + """A second DELETE against the same URL, after a hard-delete already removed the row, 404s.""" + url = criterion_delete_url(criterion.group.tag_id, criterion.id) + first = user_client.delete(url) + assert first.status_code == status.HTTP_200_OK + + second = user_client.delete(url) + + assert second.status_code == status.HTTP_404_NOT_FOUND + + +def test_cascade_deleted_group_404s_on_subsequent_create( + user_client: APIClient, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Hard-deleting a group's only criterion cascade-deletes that now-empty leaf group, and a later + create-criterion POST naming that same (now-gone) group_id 404s rather than resolving. + """ + leaf = _create_leaf_group(tag, course_run) + object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) + criterion = CompetencyCriterion.objects.create( + group=leaf, object_tag=object_tag, rule_profile=default_rule_profile, + ) + + delete_response = user_client.delete(criterion_delete_url(tag.id, criterion.id)) + assert delete_response.status_code == status.HTTP_200_OK + assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + + create_response = user_client.post( + criterion_create_url(tag.id), + {"object_id": usage_key(course_run, "p2"), "group_id": leaf.id}, + format="json", + ) + + assert create_response.status_code == status.HTTP_404_NOT_FOUND + + +def test_tag_id_criterion_id_mismatch_404s( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, criterion: CompetencyCriterion, +) -> None: + """A criterion that exists, but under a different competency tag than the URL names, 404s.""" + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Other Competency") + + response = user_client.delete(criterion_delete_url(other_tag.id, criterion.id)) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_delete_no_permission_is_403( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, tag: Tag, group: CompetencyCriteriaGroup, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A caller without object-level tagging permission on the criterion's object is refused with a 403.""" + object_tag = ObjectTag.objects.create( + object_id=f"block-v1:unused+{UNAUTHORIZED_MARKER}+unused+type@sequential+block@p1", + taxonomy=competency_taxonomy, + tag=tag, + ) + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile, + ) + + response = user_client.delete(criterion_delete_url(tag.id, criterion.id)) + + assert response.status_code == status.HTTP_403_FORBIDDEN From 226f9585d8e05143b11371ee202424068d60a28c Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Sat, 17 Oct 2026 00:54:58 +0300 Subject: [PATCH 02/24] fix: decide hard-delete or archive inside the transaction (#674) delete_competency_criterion() checked for a StudentCompetencyCriterionStatus row before opening its transaction. The status foreign keys are on_delete=PROTECT, so a status row committed between that check and criterion.delete() made the delete raise ProtectedError (or IntegrityError from the database constraint), and the endpoint returned a 500 instead of archiving. The criterion row is now locked with select_for_update() and the status check, the hard delete, the ObjectTag cleanup, and the ancestor cascade all run in one transaction. On MySQL and PostgreSQL, inserting a status row that references the locked criterion waits for this transaction, so the check stays valid until the delete. The archive path runs in the same transaction too. If the hard delete is still refused, for example because a group-level status appears for an ancestor that _cascade_from_group() deletes, the ProtectedError or IntegrityError is caught after the transaction has rolled back, and the criterion is archived in a new transaction. The result then reports archived=True. The tests cover the fallback by making the delete raise ProtectedError (a real status row created just before the real delete) and IntegrityError. SQLite cannot reproduce row locks, so the lock itself is not unit-tested. Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 39 ++++++++++----- .../applets/cbe/test_criterion_removal.py | 48 +++++++++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index a14166636..85eec126e 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -10,8 +10,8 @@ from typing import Any, Iterable, Literal, Sequence from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError -from django.db import connection, transaction -from django.db.models import F, Q, QuerySet +from django.db import IntegrityError, connection, transaction +from django.db.models import F, ProtectedError, Q, QuerySet from django.db.transaction import TransactionManagementError from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError @@ -960,6 +960,13 @@ def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", _cascade_from_group(parent, next_event) +def _archive_criterion(criterion: CompetencyCriterion) -> None: + """Archive ``criterion`` in place and cascade the archive to any ancestor left with no live children.""" + criterion.archived = True + criterion.save() + _cascade_from_group(criterion.group, "archive") + + def delete_competency_criterion(criterion_id: int) -> CriterionDeletionResult: """ Hard-delete `criterion_id`, or archive it if a learner status row already references it. @@ -974,13 +981,19 @@ def delete_competency_criterion(criterion_id: int) -> CriterionDeletionResult: cascaded in turn: hard-deleted if it carries no group-level status of its own, archived otherwise. See ADR-0002 Decision 2. + If the hard delete is refused because a learner status row appeared concurrently, the whole + attempt rolls back and the criterion is archived instead, so the result reports archived=True. + Raises CompetencyCriterion.DoesNotExist if no such criterion exists. """ - criterion = CompetencyCriterion.objects.get(pk=criterion_id) - - has_status = StudentCompetencyCriterionStatus.objects.filter(criterion_id=criterion_id).exists() - if not has_status: + try: with transaction.atomic(): + # The lock makes a concurrent status insert for this criterion wait for this transaction, + # since inserting a row that references a locked row blocks on MySQL and PostgreSQL. + criterion = CompetencyCriterion.objects.select_for_update().get(pk=criterion_id) + if StudentCompetencyCriterionStatus.objects.filter(criterion_id=criterion_id).exists(): + _archive_criterion(criterion) + return CriterionDeletionResult(id=criterion_id, archived=True) group = criterion.group object_tag = criterion.object_tag criterion.delete() @@ -991,9 +1004,11 @@ def delete_competency_criterion(criterion_id: int) -> CriterionDeletionResult: ] tag_object(object_tag.object_id, group.tag.taxonomy, existing_values) _cascade_from_group(group, "delete") - return CriterionDeletionResult(id=criterion_id, archived=False) - - criterion.archived = True - criterion.save() - _cascade_from_group(criterion.group, "archive") - return CriterionDeletionResult(id=criterion_id, archived=True) + return CriterionDeletionResult(id=criterion_id, archived=False) + except (ProtectedError, IntegrityError): + # The status foreign keys are on_delete=PROTECT: a status row for this criterion or one of + # its ancestor groups, committed after the check above, refuses the delete. The transaction + # has rolled back, so nothing was removed; archive instead. + with transaction.atomic(): + _archive_criterion(CompetencyCriterion.objects.select_for_update().get(pk=criterion_id)) + return CriterionDeletionResult(id=criterion_id, archived=True) diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index 878403b37..c973e55be 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -1,7 +1,9 @@ """Tests for delete_competency_criterion() and its ancestor-cascade helper, _cascade_from_group().""" from datetime import datetime +from unittest import mock import pytest +from django.db import IntegrityError from openedx_learning.api import delete_competency_criterion from openedx_learning.models import ( @@ -74,6 +76,52 @@ def test_archive_path_leaves_the_row_and_its_object_tag_in_place( assert ObjectTag.objects.filter(id=object_tag.id).exists() +def test_status_committed_after_the_check_falls_back_to_archive( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + A status row that appears between the status check and the delete makes the PROTECT foreign + key refuse the delete; the attempt rolls back and the criterion is archived instead, with + nothing removed along the way. + """ + group = CompetencyCriteriaGroup.objects.create(tag=tag) + criterion = _make_criterion(group, object_tag, default_rule_profile) + real_delete = CompetencyCriterion.delete + + def _delete_after_a_status_appears(self, *args, **kwargs): + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=self, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + return real_delete(self, *args, **kwargs) + + with mock.patch.object(CompetencyCriterion, "delete", autospec=True, side_effect=_delete_after_a_status_appears): + result = delete_competency_criterion(criterion.id) + + assert result.id == criterion.id + assert result.archived is True + criterion.refresh_from_db() + assert criterion.archived is True + group.refresh_from_db() + assert group.archived is True + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_integrity_error_on_the_delete_falls_back_to_archive( + tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A backend that reports the PROTECT violation as an IntegrityError gets the same archive fallback.""" + group = CompetencyCriteriaGroup.objects.create(tag=tag) + criterion = _make_criterion(group, object_tag, default_rule_profile) + + with mock.patch.object(CompetencyCriterion, "delete", autospec=True, side_effect=IntegrityError): + result = delete_competency_criterion(criterion.id) + + assert result.archived is True + criterion.refresh_from_db() + assert criterion.archived is True + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + def test_repeat_call_on_an_already_archived_criterion_is_idempotent( user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, ) -> None: From fb114a667ebcdb954e3bddf0f8460412cf298a98 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Thu, 22 Oct 2026 17:37:44 +0300 Subject: [PATCH 03/24] fix: archive a refused delete only when the criterion has a status (#674) The fallback after a refused hard delete archived the criterion on any ProtectedError or IntegrityError. The attempt also covers the ancestor cascade and the ObjectTag cleanup, so an ancestor that gained a status mid-cascade, or an unrelated integrity failure, archived a criterion that has no status and reported archived=True. The fallback now re-checks the criterion's own status and archives only if one exists. Otherwise the delete is retried once, so the cascade sees any ancestor status committed meanwhile and archives only that ancestor; a failure on the retry propagates. Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 73 ++++++++++++------- .../applets/cbe/test_criterion_removal.py | 70 +++++++++++++----- 2 files changed, 99 insertions(+), 44 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 85eec126e..fdc67248e 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -944,7 +944,7 @@ def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", if has_live_child: return if event == "delete" and not group.criteria.exists() and not group.child_groups.exists(): - if StudentCompetencyCriteriaGroupStatus.objects.filter(group_id=group.id).exists(): + if _group_has_status(group.id): group.archived = True group.save() next_event: Literal["delete", "archive"] = "archive" @@ -967,6 +967,38 @@ def _archive_criterion(criterion: CompetencyCriterion) -> None: _cascade_from_group(criterion.group, "archive") +def _criterion_has_status(criterion_id: int) -> bool: + """Return whether a learner status row references ``criterion_id``.""" + return StudentCompetencyCriterionStatus.objects.filter(criterion_id=criterion_id).exists() + + +def _group_has_status(group_id: int) -> bool: + """Return whether a learner group-level status row references ``group_id``.""" + return StudentCompetencyCriteriaGroupStatus.objects.filter(group_id=group_id).exists() + + +def _remove_competency_criterion(criterion_id: int) -> CriterionDeletionResult: + """Run one all-or-nothing attempt to hard-delete or archive ``criterion_id``.""" + with transaction.atomic(): + # The lock makes a concurrent status insert for this criterion wait for this transaction, + # since inserting a row that references a locked row blocks on MySQL and PostgreSQL. + criterion = CompetencyCriterion.objects.select_for_update().get(pk=criterion_id) + if _criterion_has_status(criterion_id): + _archive_criterion(criterion) + return CriterionDeletionResult(id=criterion_id, archived=True) + group = criterion.group + object_tag = criterion.object_tag + criterion.delete() + if not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists(): + existing_values = [ + t.value for t in get_object_tags(object_tag.object_id, taxonomy_id=object_tag.taxonomy_id) + if t.value != group.tag.value + ] + tag_object(object_tag.object_id, group.tag.taxonomy, existing_values) + _cascade_from_group(group, "delete") + return CriterionDeletionResult(id=criterion_id, archived=False) + + def delete_competency_criterion(criterion_id: int) -> CriterionDeletionResult: """ Hard-delete `criterion_id`, or archive it if a learner status row already references it. @@ -981,34 +1013,25 @@ def delete_competency_criterion(criterion_id: int) -> CriterionDeletionResult: cascaded in turn: hard-deleted if it carries no group-level status of its own, archived otherwise. See ADR-0002 Decision 2. - If the hard delete is refused because a learner status row appeared concurrently, the whole - attempt rolls back and the criterion is archived instead, so the result reports archived=True. + If the hard delete is refused, the attempt rolls back and the criterion is archived only if a + learner status row references the criterion itself, so the result reports archived=True. Otherwise + the refusal came from elsewhere, such as an ancestor group that gained a status after the cascade + checked it, so the delete is retried once. A failure on the retry propagates to the caller. - Raises CompetencyCriterion.DoesNotExist if no such criterion exists. + Raises CompetencyCriterion.DoesNotExist if no such criterion exists, and ProtectedError or + IntegrityError if the retry is refused too. """ try: + return _remove_competency_criterion(criterion_id) + except (ProtectedError, IntegrityError): + # The refusal may come from the criterion, an ancestor group, or the tag cleanup. Only a + # status on the criterion itself justifies archiving it. with transaction.atomic(): - # The lock makes a concurrent status insert for this criterion wait for this transaction, - # since inserting a row that references a locked row blocks on MySQL and PostgreSQL. criterion = CompetencyCriterion.objects.select_for_update().get(pk=criterion_id) - if StudentCompetencyCriterionStatus.objects.filter(criterion_id=criterion_id).exists(): + if _criterion_has_status(criterion_id): _archive_criterion(criterion) return CriterionDeletionResult(id=criterion_id, archived=True) - group = criterion.group - object_tag = criterion.object_tag - criterion.delete() - if not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists(): - existing_values = [ - t.value for t in get_object_tags(object_tag.object_id, taxonomy_id=object_tag.taxonomy_id) - if t.value != group.tag.value - ] - tag_object(object_tag.object_id, group.tag.taxonomy, existing_values) - _cascade_from_group(group, "delete") - return CriterionDeletionResult(id=criterion_id, archived=False) - except (ProtectedError, IntegrityError): - # The status foreign keys are on_delete=PROTECT: a status row for this criterion or one of - # its ancestor groups, committed after the check above, refuses the delete. The transaction - # has rolled back, so nothing was removed; archive instead. - with transaction.atomic(): - _archive_criterion(CompetencyCriterion.objects.select_for_update().get(pk=criterion_id)) - return CriterionDeletionResult(id=criterion_id, archived=True) + # No status on the criterion. If the refusal came from an ancestor status committed meanwhile, + # the retry's cascade now sees it and archives only that ancestor; any other failure repeats + # and propagates. + return _remove_competency_criterion(criterion_id) diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index c973e55be..c29512d8a 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -76,25 +76,22 @@ def test_archive_path_leaves_the_row_and_its_object_tag_in_place( assert ObjectTag.objects.filter(id=object_tag.id).exists() -def test_status_committed_after_the_check_falls_back_to_archive( +def test_status_on_the_criterion_missed_by_the_first_check_archives_it( user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, ) -> None: """ - A status row that appears between the status check and the delete makes the PROTECT foreign - key refuse the delete; the attempt rolls back and the criterion is archived instead, with - nothing removed along the way. + The criterion has a status row, but the first status check misses it. The PROTECT foreign key + refuses the delete, the attempt rolls back, and the fallback finds the status and archives the + criterion, cascading the archive to its group and leaving the object_tag in place. """ group = CompetencyCriteriaGroup.objects.create(tag=tag) criterion = _make_criterion(group, object_tag, default_rule_profile) - real_delete = CompetencyCriterion.delete - - def _delete_after_a_status_appears(self, *args, **kwargs): - StudentCompetencyCriterionStatus.objects.create( - user=user, criterion=self, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, - ) - return real_delete(self, *args, **kwargs) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) - with mock.patch.object(CompetencyCriterion, "delete", autospec=True, side_effect=_delete_after_a_status_appears): + # Call 1 is the check inside the transaction, call 2 is the fallback re-check. + with mock.patch("openedx_learning.applets.cbe.api._criterion_has_status", side_effect=[False, True]): result = delete_competency_criterion(criterion.id) assert result.id == criterion.id @@ -106,20 +103,55 @@ def _delete_after_a_status_appears(self, *args, **kwargs): assert ObjectTag.objects.filter(id=object_tag.id).exists() -def test_integrity_error_on_the_delete_falls_back_to_archive( +def test_status_on_an_ancestor_missed_once_archives_only_the_ancestor( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + The leaf group has a status row, but the cascade's first check misses it, so the group delete is + refused. The criterion itself has no status, so the fallback does not archive it; the retry sees + the status, archives the leaf group and its parent, and the criterion is still hard-deleted. + If the fallback archived unconditionally, the criterion row would survive and this would fail. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=leaf, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + # Call 1 is the first cascade at the leaf group, call 2 is the retry's cascade. + with mock.patch("openedx_learning.applets.cbe.api._group_has_status", side_effect=[False, True]): + result = delete_competency_criterion(criterion.id) + + assert result.archived is False + assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() + assert not ObjectTag.objects.filter(id=object_tag.id).exists() + leaf.refresh_from_db() + assert leaf.archived is True + root.refresh_from_db() + assert root.archived is True + + +def test_unrelated_integrity_error_propagates_without_archiving( tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, ) -> None: - """A backend that reports the PROTECT violation as an IntegrityError gets the same archive fallback.""" + """ + An IntegrityError from the object_tag cleanup, with no status anywhere, is retried once and then + propagates. The criterion is not archived and its group is unchanged. + """ group = CompetencyCriteriaGroup.objects.create(tag=tag) criterion = _make_criterion(group, object_tag, default_rule_profile) - with mock.patch.object(CompetencyCriterion, "delete", autospec=True, side_effect=IntegrityError): - result = delete_competency_criterion(criterion.id) + with mock.patch("openedx_learning.applets.cbe.api.tag_object", side_effect=IntegrityError) as tag_object: + with pytest.raises(IntegrityError): + delete_competency_criterion(criterion.id) + + assert tag_object.call_count == 2 - assert result.archived is True criterion.refresh_from_db() - assert criterion.archived is True - assert ObjectTag.objects.filter(id=object_tag.id).exists() + assert criterion.archived is False + group.refresh_from_db() + assert group.archived is False def test_repeat_call_on_an_already_archived_criterion_is_idempotent( From 777f175cc39c61591c98e84e51ababe903d3aeee Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Mon, 26 Oct 2026 18:26:40 +0300 Subject: [PATCH 04/24] refactor: check permission inside delete_or_archive_competency_criterion (#674) Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 35 ++++---- src/openedx_learning/applets/cbe/data.py | 13 +++ .../applets/cbe/rest_api/v1/views.py | 19 ++-- .../applets/cbe/test_criterion_removal.py | 88 ++++++++++++++----- .../applets/cbe/test_views.py | 39 ++++++-- 5 files changed, 137 insertions(+), 57 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index fdc67248e..8e511c78f 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -7,7 +7,7 @@ from dataclasses import dataclass from datetime import datetime, timezone from decimal import Decimal -from typing import Any, Iterable, Literal, Sequence +from typing import Any, Iterable, Literal, Sequence, cast from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError from django.db import IntegrityError, connection, transaction @@ -23,7 +23,7 @@ from openedx_tagging.models import ObjectTag, Tag, Taxonomy from openedx_tagging.rules import ObjectTagPermissionItem, UserType -from .data import GradedObjectScore +from .data import CriterionDeletionResult, GradedObjectScore from .models import ( CompetencyCourseRun, CompetencyCriteriaGroup, @@ -43,12 +43,13 @@ "CompetencyCriteriaGroupArchivedError", "CompetencyCriteriaTree", "CompetencyCriterionArchivedError", + "CriterionDeletionResult", "GradedObjectScore", "associate_competency_criterion", "bulk_update_competency_criteria", "create_competency_criterion", "create_competency_taxonomy", - "delete_competency_criterion", + "delete_or_archive_competency_criterion", "get_competency_criteria_tree", "get_competency_rule_profiles", "get_courses_with_relative_criteria", @@ -921,14 +922,6 @@ def update_competency_criteria_group( return group -@dataclass(frozen=True) -class CriterionDeletionResult: - """The outcome of :func:`delete_competency_criterion`: whether the row was hard-deleted or archived.""" - - id: int - archived: bool - - def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", "archive"]) -> None: """ Walk `group` and its ancestors, collapsing/archiving any left with no live children. @@ -999,15 +992,16 @@ def _remove_competency_criterion(criterion_id: int) -> CriterionDeletionResult: return CriterionDeletionResult(id=criterion_id, archived=False) -def delete_competency_criterion(criterion_id: int) -> CriterionDeletionResult: +def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> CriterionDeletionResult: """ Hard-delete `criterion_id`, or archive it if a learner status row already references it. UNSTABLE: the criteria-authoring family is incomplete, so its validation and group update and archive behavior may change this function's shape without a deprecation cycle. - The caller is expected to have already checked ``oel_tagging.can_tag_object`` for the - criterion's object, the same contract as :func:`associate_competency_criterion`. + ``user`` must hold ``oel_tagging.can_tag_object`` for the criterion's object, as + :func:`bulk_update_competency_criteria` requires; otherwise Django's ``PermissionDenied`` is + raised before anything is locked or changed. An ancestor :class:`CompetencyCriteriaGroup` left with no live children by this call is cascaded in turn: hard-deleted if it carries no group-level status of its own, archived @@ -1018,9 +1012,18 @@ def delete_competency_criterion(criterion_id: int) -> CriterionDeletionResult: the refusal came from elsewhere, such as an ancestor group that gained a status after the cascade checked it, so the delete is retried once. A failure on the retry propagates to the caller. - Raises CompetencyCriterion.DoesNotExist if no such criterion exists, and ProtectedError or - IntegrityError if the retry is refused too. + Raises CompetencyCriterion.DoesNotExist if no such criterion exists, PermissionDenied if + ``user`` may not tag the criterion's object, and ProtectedError or IntegrityError if the + retry is refused too. """ + criterion = CompetencyCriterion.objects.select_related("group__tag__taxonomy", "object_tag").get(pk=criterion_id) + # A group's tag is always a competency tag, so its taxonomy is always set. + assert criterion.group.tag.taxonomy is not None + perm_obj = ObjectTagPermissionItem(taxonomy=criterion.group.tag.taxonomy, object_id=criterion.object_tag.object_id) + # The rules-based permission takes an ObjectTagPermissionItem, but django-stubs types the + # has_perm object argument as a Model. + if not user.has_perm("oel_tagging.can_tag_object", cast(Any, perm_obj)): + raise PermissionDenied() try: return _remove_competency_criterion(criterion_id) except (ProtectedError, IntegrityError): diff --git a/src/openedx_learning/applets/cbe/data.py b/src/openedx_learning/applets/cbe/data.py index a985c6eba..cc01fc7c0 100644 --- a/src/openedx_learning/applets/cbe/data.py +++ b/src/openedx_learning/applets/cbe/data.py @@ -36,3 +36,16 @@ def __post_init__(self) -> None: # NaN and Infinity, actually raise the documented ValueError. if not self.fraction.is_finite() or not 0 <= self.fraction <= 1: raise ValueError(f"fraction must be finite and between 0 and 1 inclusive, not {self.fraction!r}") + + +@dataclass(frozen=True, kw_only=True, slots=True) +class CriterionDeletionResult: + """ + The outcome of removing a CompetencyCriterion: whether its row was hard-deleted or archived. + + UNSTABLE: the criteria-authoring family is incomplete, so this shape may change without a + deprecation cycle. + """ + + id: int + archived: bool diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/views.py b/src/openedx_learning/applets/cbe/rest_api/v1/views.py index 024dc0ba4..60e8852e0 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -26,7 +26,7 @@ CompetencyCriterionArchivedError, associate_competency_criterion, bulk_update_competency_criteria, - delete_competency_criterion, + delete_or_archive_competency_criterion, get_competency_criteria_tree, get_competency_rule_profiles, resolve_competency_tag, @@ -280,8 +280,8 @@ class CompetencyCriterionDeleteView(generics.GenericAPIView): """ DELETE-only. Remove a CompetencyCriterion: hard-delete, or archive if learner status exists. - Does not call self.check_object_permissions(): oel_tagging.can_tag_object is checked inline - with a manual ``has_perm()`` call, the same pattern CompetencyCriterionCreateView uses. + Does not call self.check_object_permissions(): the public API function checks + oel_tagging.can_tag_object itself and raises Django's PermissionDenied, which becomes a 403. """ serializer_class = CompetencyCriterionSerializer @@ -290,7 +290,7 @@ class CompetencyCriterionDeleteView(generics.GenericAPIView): def delete(self, request, *args, **kwargs): """ - Resolve the criterion scoped to both path segments, check permission, then delegate to the public API. + Resolve the criterion scoped to both path segments, then delegate to the public API, which checks permission. Scoped by tag_id rather than a specific group_id, matching CompetencyCriterionCreateView's own URL convention; a group-scoped delete is a separate ticket's concern (direct group @@ -301,10 +301,9 @@ def delete(self, request, *args, **kwargs): criterion = get_object_or_404( CompetencyCriterion, pk=self.kwargs["criterion_id"], group__tag_id=self.kwargs["tag_id"], ) - perm_obj = ObjectTagPermissionItem( - taxonomy=criterion.group.tag.taxonomy, object_id=criterion.object_tag.object_id, - ) - if not request.user.has_perm("oel_tagging.can_tag_object", perm_obj): - raise PermissionDenied() - result = delete_competency_criterion(criterion.id) + try: + result = delete_or_archive_competency_criterion(criterion.id, request.user) + except CompetencyCriterion.DoesNotExist as exc: + # Removed between the lookup above and the API call. + raise NotFound() from exc return Response({"id": result.id, "archived": result.archived}, status=status.HTTP_200_OK) diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index c29512d8a..5280880e9 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -1,11 +1,15 @@ -"""Tests for delete_competency_criterion() and its ancestor-cascade helper, _cascade_from_group().""" +"""Tests for delete_or_archive_competency_criterion() and its ancestor-cascade helper, _cascade_from_group().""" +from collections.abc import Iterator from datetime import datetime from unittest import mock import pytest +import rules +from django.core.exceptions import PermissionDenied from django.db import IntegrityError +from rules.permissions import permissions as rule_permissions -from openedx_learning.api import delete_competency_criterion +from openedx_learning.api import delete_or_archive_competency_criterion from openedx_learning.models import ( CompetencyCriteriaGroup, CompetencyCriterion, @@ -19,6 +23,22 @@ pytestmark = pytest.mark.django_db +CHANGE_OBJECTTAG_OBJECTID = "oel_tagging.change_objecttag_objectid" + + +@pytest.fixture(autouse=True) +def _allow_object_level_tagging() -> Iterator[None]: + """ + Let any user tag any object; restore the real rule afterwards. + + openedx_tagging denies change_objecttag_objectid to everyone and leaves the real Studio-role + check to openedx-platform, so these tests substitute a permissive one. + """ + original = rule_permissions[CHANGE_OBJECTTAG_OBJECTID] + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, lambda _user, _object_id: True) + yield + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, original) + def _make_object_tag(competency_taxonomy: CompetencyTaxonomy, tag: Tag, object_id: str) -> ObjectTag: """Create an ObjectTag for `object_id`, distinct from the conftest `object_tag` fixture's own row.""" @@ -38,7 +58,7 @@ def _make_criterion( def test_hard_delete_with_no_status_removes_the_row_and_its_object_tag( - tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, ) -> None: """ Deleting a criterion with no learner status hard-deletes it and drops its now-unreferenced @@ -47,7 +67,7 @@ def test_hard_delete_with_no_status_removes_the_row_and_its_object_tag( group = CompetencyCriteriaGroup.objects.create(tag=tag) criterion = _make_criterion(group, object_tag, default_rule_profile) - result = delete_competency_criterion(criterion.id) + result = delete_or_archive_competency_criterion(criterion.id, user) assert result.id == criterion.id assert result.archived is False @@ -68,7 +88,7 @@ def test_archive_path_leaves_the_row_and_its_object_tag_in_place( user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, ) - result = delete_competency_criterion(criterion.id) + result = delete_or_archive_competency_criterion(criterion.id, user) assert result.archived is True criterion.refresh_from_db() @@ -92,7 +112,7 @@ def test_status_on_the_criterion_missed_by_the_first_check_archives_it( # Call 1 is the check inside the transaction, call 2 is the fallback re-check. with mock.patch("openedx_learning.applets.cbe.api._criterion_has_status", side_effect=[False, True]): - result = delete_competency_criterion(criterion.id) + result = delete_or_archive_competency_criterion(criterion.id, user) assert result.id == criterion.id assert result.archived is True @@ -121,7 +141,7 @@ def test_status_on_an_ancestor_missed_once_archives_only_the_ancestor( # Call 1 is the first cascade at the leaf group, call 2 is the retry's cascade. with mock.patch("openedx_learning.applets.cbe.api._group_has_status", side_effect=[False, True]): - result = delete_competency_criterion(criterion.id) + result = delete_or_archive_competency_criterion(criterion.id, user) assert result.archived is False assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() @@ -133,7 +153,7 @@ def test_status_on_an_ancestor_missed_once_archives_only_the_ancestor( def test_unrelated_integrity_error_propagates_without_archiving( - tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, ) -> None: """ An IntegrityError from the object_tag cleanup, with no status anywhere, is retried once and then @@ -144,7 +164,7 @@ def test_unrelated_integrity_error_propagates_without_archiving( with mock.patch("openedx_learning.applets.cbe.api.tag_object", side_effect=IntegrityError) as tag_object: with pytest.raises(IntegrityError): - delete_competency_criterion(criterion.id) + delete_or_archive_competency_criterion(criterion.id, user) assert tag_object.call_count == 2 @@ -163,9 +183,9 @@ def test_repeat_call_on_an_already_archived_criterion_is_idempotent( StudentCompetencyCriterionStatus.objects.create( user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, ) - delete_competency_criterion(criterion.id) + delete_or_archive_competency_criterion(criterion.id, user) - result = delete_competency_criterion(criterion.id) + result = delete_or_archive_competency_criterion(criterion.id, user) assert result.id == criterion.id assert result.archived is True @@ -174,7 +194,7 @@ def test_repeat_call_on_an_already_archived_criterion_is_idempotent( def test_shared_object_tag_is_untouched_while_another_criterion_still_references_it( - tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, ) -> None: """ Two criteria, in two different groups, referencing the same object_tag (a deliberate second @@ -186,7 +206,7 @@ def test_shared_object_tag_is_untouched_while_another_criterion_still_references criterion_a = _make_criterion(group_a, object_tag, default_rule_profile) criterion_b = _make_criterion(group_b, object_tag, default_rule_profile) - delete_competency_criterion(criterion_a.id) + delete_or_archive_competency_criterion(criterion_a.id, user) assert not CompetencyCriterion.objects.filter(id=criterion_a.id).exists() assert CompetencyCriterion.objects.filter(id=criterion_b.id).exists() @@ -199,7 +219,7 @@ def test_shared_object_tag_is_untouched_while_another_criterion_still_references def test_single_child_leaf_group_cascade_deletes_while_a_sibling_keeps_the_root_alive( - competency_taxonomy: CompetencyTaxonomy, tag: Tag, object_tag: ObjectTag, + user, competency_taxonomy: CompetencyTaxonomy, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, ) -> None: """ @@ -214,7 +234,7 @@ def test_single_child_leaf_group_cascade_deletes_while_a_sibling_keeps_the_root_ sibling_object_tag = _make_object_tag(competency_taxonomy, tag, "obj-sibling") _make_criterion(sibling, sibling_object_tag, default_rule_profile) - delete_competency_criterion(criterion.id) + delete_or_archive_competency_criterion(criterion.id, user) assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() @@ -222,7 +242,7 @@ def test_single_child_leaf_group_cascade_deletes_while_a_sibling_keeps_the_root_ def test_multilevel_cascade_fully_collapses_the_root_to_leaf_chain( - tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, ) -> None: """ Hard-deleting the only criterion in a root -> mid -> leaf chain, with nothing else anywhere in @@ -233,7 +253,7 @@ def test_multilevel_cascade_fully_collapses_the_root_to_leaf_chain( leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=mid) criterion = _make_criterion(leaf, object_tag, default_rule_profile) - delete_competency_criterion(criterion.id) + delete_or_archive_competency_criterion(criterion.id, user) assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() assert not CompetencyCriteriaGroup.objects.filter(id=mid.id).exists() @@ -241,7 +261,7 @@ def test_multilevel_cascade_fully_collapses_the_root_to_leaf_chain( def test_cascade_stops_at_an_ancestor_with_a_surviving_sibling_group( - competency_taxonomy: CompetencyTaxonomy, tag: Tag, object_tag: ObjectTag, + user, competency_taxonomy: CompetencyTaxonomy, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, ) -> None: """ @@ -257,7 +277,7 @@ def test_cascade_stops_at_an_ancestor_with_a_surviving_sibling_group( other_object_tag = _make_object_tag(competency_taxonomy, tag, "obj-other-branch") other_criterion = _make_criterion(other_branch, other_object_tag, default_rule_profile) - delete_competency_criterion(criterion.id) + delete_or_archive_competency_criterion(criterion.id, user) assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() assert not CompetencyCriteriaGroup.objects.filter(id=branch.id).exists() @@ -282,7 +302,7 @@ def test_group_with_its_own_status_is_archived_not_deleted_at_zero_remaining_chi user=user, group=leaf, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, ) - result = delete_competency_criterion(criterion.id) + result = delete_or_archive_competency_criterion(criterion.id, user) assert result.archived is False assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() @@ -311,7 +331,7 @@ def test_archiving_a_groups_only_criterion_archives_the_group_and_cascades_upwar user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, ) - delete_competency_criterion(criterion.id) + delete_or_archive_competency_criterion(criterion.id, user) leaf.refresh_from_db() root.refresh_from_db() @@ -321,12 +341,34 @@ def test_archiving_a_groups_only_criterion_archives_the_group_and_cascades_upwar assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() +# ================================================================================================= +# Permission +# ================================================================================================= + + +def test_a_user_without_permission_is_refused_and_nothing_changes( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A user who may not tag the criterion's object is refused, and the criterion, group and ObjectTag survive.""" + group = CompetencyCriteriaGroup.objects.create(tag=tag) + criterion = _make_criterion(group, object_tag, default_rule_profile) + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, lambda _user, _object_id: False) + + with pytest.raises(PermissionDenied): + delete_or_archive_competency_criterion(criterion.id, user) + + criterion.refresh_from_db() + assert criterion.archived is False + assert CompetencyCriteriaGroup.objects.filter(id=group.id).exists() + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + # ================================================================================================= # Not found # ================================================================================================= -def test_does_not_exist_for_an_unknown_criterion_id() -> None: +def test_does_not_exist_for_an_unknown_criterion_id(user) -> None: """A criterion_id with no matching row raises CompetencyCriterion.DoesNotExist.""" with pytest.raises(CompetencyCriterion.DoesNotExist): - delete_competency_criterion(999999999) + delete_or_archive_competency_criterion(999999999, user) diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 6817e3d20..7e8b425e6 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -6,6 +6,7 @@ """ from datetime import datetime from typing import Any, Generator +from unittest import mock from urllib.parse import urlencode import pytest @@ -349,6 +350,7 @@ def test_no_group_no_existing_groups_creates_the_full_hierarchy_and_201s( assert response.data["object_tag_id"] == criterion.object_tag_id assert response.data["group_id"] == criterion.group_id assert response.data["rule_profile_id"] == default_rule_profile.id + assert response.data["archived"] is False def test_logic_operator_provided_is_stored_on_the_new_leaf( @@ -2020,14 +2022,6 @@ def _criterion( return CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=default_rule_profile) -def test_create_response_reports_archived_false(user_client: APIClient, tag: Tag, course_run: CourseRun) -> None: - """The create endpoint's response carries the read-only archived flag, false for a new criterion.""" - response = user_client.post(criterion_create_url(tag.id), {"object_id": usage_key(course_run, "p1")}, format="json") - - assert response.status_code == status.HTTP_201_CREATED - assert response.data["archived"] is False - - def test_hard_delete_200s_with_archived_false(user_client: APIClient, criterion: CompetencyCriterion) -> None: """Deleting a criterion with no learner status 200s, hard-deletes it, and reports archived=false.""" response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) @@ -2142,3 +2136,32 @@ def test_delete_no_permission_is_403( response = user_client.delete(criterion_delete_url(tag.id, criterion.id)) assert response.status_code == status.HTTP_403_FORBIDDEN + assert CompetencyCriterion.objects.filter(id=criterion.id).exists() + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_delete_without_view_access_to_the_taxonomy_is_403( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, criterion: CompetencyCriterion, +) -> None: + """A disabled taxonomy refuses an ordinary caller even when the object id itself is permitted.""" + competency_taxonomy.enabled = False + competency_taxonomy.save() + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert CompetencyCriterion.objects.filter(id=criterion.id).exists() + assert ObjectTag.objects.filter(id=criterion.object_tag_id).exists() + + +def test_delete_of_a_criterion_gone_by_the_time_of_the_call_is_404( + user_client: APIClient, criterion: CompetencyCriterion, +) -> None: + """A criterion removed between the view's lookup and the API call is a 404, not a 500.""" + with mock.patch( + "openedx_learning.applets.cbe.rest_api.v1.views.delete_or_archive_competency_criterion", + side_effect=CompetencyCriterion.DoesNotExist, + ): + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_404_NOT_FOUND From 237aae6924360ab90e0832f3284849941035f01b Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Mon, 26 Oct 2026 18:29:02 +0300 Subject: [PATCH 05/24] fix: lock tag, ancestors, and criterion instead of retrying (#674) Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 89 +++++++++---------- .../applets/cbe/test_criterion_removal.py | 80 ----------------- 2 files changed, 43 insertions(+), 126 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 8e511c78f..f762995d8 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -10,8 +10,8 @@ from typing import Any, Iterable, Literal, Sequence, cast from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError -from django.db import IntegrityError, connection, transaction -from django.db.models import F, ProtectedError, Q, QuerySet +from django.db import connection, transaction +from django.db.models import F, Q, QuerySet from django.db.transaction import TransactionManagementError from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError @@ -937,7 +937,7 @@ def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", if has_live_child: return if event == "delete" and not group.criteria.exists() and not group.child_groups.exists(): - if _group_has_status(group.id): + if _group_has_learner_progress(group.id): group.archived = True group.save() next_event: Literal["delete", "archive"] = "archive" @@ -960,36 +960,24 @@ def _archive_criterion(criterion: CompetencyCriterion) -> None: _cascade_from_group(criterion.group, "archive") -def _criterion_has_status(criterion_id: int) -> bool: +def _criterion_has_learner_progress(criterion_id: int) -> bool: """Return whether a learner status row references ``criterion_id``.""" return StudentCompetencyCriterionStatus.objects.filter(criterion_id=criterion_id).exists() -def _group_has_status(group_id: int) -> bool: +def _group_has_learner_progress(group_id: int) -> bool: """Return whether a learner group-level status row references ``group_id``.""" return StudentCompetencyCriteriaGroupStatus.objects.filter(group_id=group_id).exists() -def _remove_competency_criterion(criterion_id: int) -> CriterionDeletionResult: - """Run one all-or-nothing attempt to hard-delete or archive ``criterion_id``.""" - with transaction.atomic(): - # The lock makes a concurrent status insert for this criterion wait for this transaction, - # since inserting a row that references a locked row blocks on MySQL and PostgreSQL. - criterion = CompetencyCriterion.objects.select_for_update().get(pk=criterion_id) - if _criterion_has_status(criterion_id): - _archive_criterion(criterion) - return CriterionDeletionResult(id=criterion_id, archived=True) - group = criterion.group - object_tag = criterion.object_tag - criterion.delete() - if not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists(): - existing_values = [ - t.value for t in get_object_tags(object_tag.object_id, taxonomy_id=object_tag.taxonomy_id) - if t.value != group.tag.value - ] - tag_object(object_tag.object_id, group.tag.taxonomy, existing_values) - _cascade_from_group(group, "delete") - return CriterionDeletionResult(id=criterion_id, archived=False) +def _lineage_group_ids(group: CompetencyCriteriaGroup) -> list[int]: + """Return the ids of ``group`` and every ancestor up to the root, one query per level.""" + ids = [group.pk] + parent_id = group.parent_id + while parent_id is not None: + ids.append(parent_id) + parent_id = CompetencyCriteriaGroup.objects.values_list("parent_id", flat=True).get(pk=parent_id) + return ids def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> CriterionDeletionResult: @@ -1007,14 +995,14 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> cascaded in turn: hard-deleted if it carries no group-level status of its own, archived otherwise. See ADR-0002 Decision 2. - If the hard delete is refused, the attempt rolls back and the criterion is archived only if a - learner status row references the criterion itself, so the result reports archived=True. Otherwise - the refusal came from elsewhere, such as an ancestor group that gained a status after the cascade - checked it, so the delete is retried once. A failure on the retry propagates to the caller. + Raises CompetencyCriterion.DoesNotExist if no such criterion exists (including one deleted + concurrently) and PermissionDenied if ``user`` may not tag the criterion's object. - Raises CompetencyCriterion.DoesNotExist if no such criterion exists, PermissionDenied if - ``user`` may not tag the criterion's object, and ProtectedError or IntegrityError if the - retry is refused too. + The whole operation runs in one transaction that locks the competency's Tag, then the + criterion's group and every ancestor group, then the criterion. A concurrent create for the + same competency waits on the Tag lock. A learner-progress insert on the criterion or on any + ancestor group waits on that row's lock. The progress checks therefore cannot go stale, so + no fallback or retry is needed. """ criterion = CompetencyCriterion.objects.select_related("group__tag__taxonomy", "object_tag").get(pk=criterion_id) # A group's tag is always a competency tag, so its taxonomy is always set. @@ -1024,17 +1012,26 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> # has_perm object argument as a Model. if not user.has_perm("oel_tagging.can_tag_object", cast(Any, perm_obj)): raise PermissionDenied() - try: - return _remove_competency_criterion(criterion_id) - except (ProtectedError, IntegrityError): - # The refusal may come from the criterion, an ancestor group, or the tag cleanup. Only a - # status on the criterion itself justifies archiving it. - with transaction.atomic(): - criterion = CompetencyCriterion.objects.select_for_update().get(pk=criterion_id) - if _criterion_has_status(criterion_id): - _archive_criterion(criterion) - return CriterionDeletionResult(id=criterion_id, archived=True) - # No status on the criterion. If the refusal came from an ancestor status committed meanwhile, - # the retry's cascade now sees it and archives only that ancestor; any other failure repeats - # and propagates. - return _remove_competency_criterion(criterion_id) + tag_id = criterion.group.tag_id + # Tree changes are serialized by the Tag lock, so reading the lineage before taking it is safe. + lineage_ids = _lineage_group_ids(criterion.group) + with transaction.atomic(): + Tag.objects.select_for_update().get(pk=tag_id) + # Evaluated so the locks are taken now, in primary-key order to avoid deadlocks between writers. + list(CompetencyCriteriaGroup.objects.select_for_update().filter(pk__in=lineage_ids).order_by("pk")) + # A concurrent delete may have won; DoesNotExist propagates and the view maps it to 404. + criterion = CompetencyCriterion.objects.select_for_update().get(pk=criterion_id) + if _criterion_has_learner_progress(criterion_id): + _archive_criterion(criterion) + return CriterionDeletionResult(id=criterion_id, archived=True) + group = criterion.group + object_tag = criterion.object_tag + criterion.delete() + if not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists(): + existing_values = [ + t.value for t in get_object_tags(object_tag.object_id, taxonomy_id=object_tag.taxonomy_id) + if t.value != group.tag.value + ] + tag_object(object_tag.object_id, group.tag.taxonomy, existing_values) + _cascade_from_group(group, "delete") + return CriterionDeletionResult(id=criterion_id, archived=False) diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index 5280880e9..18f180a0e 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -1,12 +1,10 @@ """Tests for delete_or_archive_competency_criterion() and its ancestor-cascade helper, _cascade_from_group().""" from collections.abc import Iterator from datetime import datetime -from unittest import mock import pytest import rules from django.core.exceptions import PermissionDenied -from django.db import IntegrityError from rules.permissions import permissions as rule_permissions from openedx_learning.api import delete_or_archive_competency_criterion @@ -96,84 +94,6 @@ def test_archive_path_leaves_the_row_and_its_object_tag_in_place( assert ObjectTag.objects.filter(id=object_tag.id).exists() -def test_status_on_the_criterion_missed_by_the_first_check_archives_it( - user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, -) -> None: - """ - The criterion has a status row, but the first status check misses it. The PROTECT foreign key - refuses the delete, the attempt rolls back, and the fallback finds the status and archives the - criterion, cascading the archive to its group and leaving the object_tag in place. - """ - group = CompetencyCriteriaGroup.objects.create(tag=tag) - criterion = _make_criterion(group, object_tag, default_rule_profile) - StudentCompetencyCriterionStatus.objects.create( - user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, - ) - - # Call 1 is the check inside the transaction, call 2 is the fallback re-check. - with mock.patch("openedx_learning.applets.cbe.api._criterion_has_status", side_effect=[False, True]): - result = delete_or_archive_competency_criterion(criterion.id, user) - - assert result.id == criterion.id - assert result.archived is True - criterion.refresh_from_db() - assert criterion.archived is True - group.refresh_from_db() - assert group.archived is True - assert ObjectTag.objects.filter(id=object_tag.id).exists() - - -def test_status_on_an_ancestor_missed_once_archives_only_the_ancestor( - user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, -) -> None: - """ - The leaf group has a status row, but the cascade's first check misses it, so the group delete is - refused. The criterion itself has no status, so the fallback does not archive it; the retry sees - the status, archives the leaf group and its parent, and the criterion is still hard-deleted. - If the fallback archived unconditionally, the criterion row would survive and this would fail. - """ - root = CompetencyCriteriaGroup.objects.create(tag=tag) - leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) - criterion = _make_criterion(leaf, object_tag, default_rule_profile) - StudentCompetencyCriteriaGroupStatus.objects.create( - user=user, group=leaf, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, - ) - - # Call 1 is the first cascade at the leaf group, call 2 is the retry's cascade. - with mock.patch("openedx_learning.applets.cbe.api._group_has_status", side_effect=[False, True]): - result = delete_or_archive_competency_criterion(criterion.id, user) - - assert result.archived is False - assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() - assert not ObjectTag.objects.filter(id=object_tag.id).exists() - leaf.refresh_from_db() - assert leaf.archived is True - root.refresh_from_db() - assert root.archived is True - - -def test_unrelated_integrity_error_propagates_without_archiving( - user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, -) -> None: - """ - An IntegrityError from the object_tag cleanup, with no status anywhere, is retried once and then - propagates. The criterion is not archived and its group is unchanged. - """ - group = CompetencyCriteriaGroup.objects.create(tag=tag) - criterion = _make_criterion(group, object_tag, default_rule_profile) - - with mock.patch("openedx_learning.applets.cbe.api.tag_object", side_effect=IntegrityError) as tag_object: - with pytest.raises(IntegrityError): - delete_or_archive_competency_criterion(criterion.id, user) - - assert tag_object.call_count == 2 - - criterion.refresh_from_db() - assert criterion.archived is False - group.refresh_from_db() - assert group.archived is False - - def test_repeat_call_on_an_already_archived_criterion_is_idempotent( user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, ) -> None: From dd951d5d825124d1dd2d2b9d457a63815b2fc539 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Mon, 26 Oct 2026 18:33:11 +0300 Subject: [PATCH 06/24] fix: write only the archived flag, record the user, stop at archived levels (#674) Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 48 ++++-- .../applets/cbe/test_criterion_removal.py | 143 ++++++++++++++++-- 2 files changed, 165 insertions(+), 26 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index f762995d8..860da2886 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -922,14 +922,26 @@ def update_competency_criteria_group( return group -def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", "archive"]) -> None: +def _archive_group(group: CompetencyCriteriaGroup, user: UserType) -> None: + """Archive ``group``, writing only the flag and recording ``user`` in its history.""" + group.archived = True + # Set on the instance so attribution doesn't depend on simple_history's request middleware. + group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + group.save(update_fields=["archived"]) + + +def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", "archive"], user: UserType) -> None: """ Walk `group` and its ancestors, collapsing/archiving any left with no live children. - `group.criteria` only has rows at the true-leaf level, so checking it at every level - unconditionally is harmless (an empty queryset above the leaf) and avoids a separate - leaf/non-leaf code path. + Checking both relations (`criteria` and `child_groups`) at every level is harmless: the one that + does not apply is an empty queryset, and it avoids a separate leaf/non-leaf code path. + + The walk stops at a group that is already archived. Everything above it was handled when it was + archived, so writing it again would only add a redundant history row. """ + if group.archived: + return has_live_child = ( group.criteria.filter(archived=False).exists() or group.child_groups.filter(archived=False).exists() ) @@ -938,26 +950,32 @@ def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", return if event == "delete" and not group.criteria.exists() and not group.child_groups.exists(): if _group_has_learner_progress(group.id): - group.archived = True - group.save() + _archive_group(group, user) next_event: Literal["delete", "archive"] = "archive" else: group.delete() next_event = "delete" else: # Only archived children remain. - group.archived = True - group.save() + _archive_group(group, user) next_event = "archive" if parent is not None: - _cascade_from_group(parent, next_event) + _cascade_from_group(parent, next_event, user) -def _archive_criterion(criterion: CompetencyCriterion) -> None: - """Archive ``criterion`` in place and cascade the archive to any ancestor left with no live children.""" +def _archive_criterion(criterion: CompetencyCriterion, user: UserType) -> None: + """ + Archive ``criterion`` in place and cascade the archive to any ancestor left with no live children. + + Does nothing if ``criterion`` is already archived, so a repeat call writes no history. + """ + if criterion.archived: + return criterion.archived = True - criterion.save() - _cascade_from_group(criterion.group, "archive") + # Set on the instance so attribution doesn't depend on simple_history's request middleware. + criterion._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + criterion.save(update_fields=["archived"]) + _cascade_from_group(criterion.group, "archive", user) def _criterion_has_learner_progress(criterion_id: int) -> bool: @@ -1022,7 +1040,7 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> # A concurrent delete may have won; DoesNotExist propagates and the view maps it to 404. criterion = CompetencyCriterion.objects.select_for_update().get(pk=criterion_id) if _criterion_has_learner_progress(criterion_id): - _archive_criterion(criterion) + _archive_criterion(criterion, user) return CriterionDeletionResult(id=criterion_id, archived=True) group = criterion.group object_tag = criterion.object_tag @@ -1033,5 +1051,5 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> if t.value != group.tag.value ] tag_object(object_tag.object_id, group.tag.taxonomy, existing_values) - _cascade_from_group(group, "delete") + _cascade_from_group(group, "delete", user) return CriterionDeletionResult(id=criterion_id, archived=False) diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index 18f180a0e..e3321c287 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -4,6 +4,7 @@ import pytest import rules +from django.apps import apps from django.core.exceptions import PermissionDenied from rules.permissions import permissions as rule_permissions @@ -94,16 +95,36 @@ def test_archive_path_leaves_the_row_and_its_object_tag_in_place( assert ObjectTag.objects.filter(id=object_tag.id).exists() -def test_repeat_call_on_an_already_archived_criterion_is_idempotent( +def _latest_history_user(model_name: str, pk: int): + """Return the user recorded on the newest historical row of the ``model_name`` row ``pk``.""" + historical = apps.get_model("openedx_learning", model_name) + return historical.objects.filter(id=pk).order_by("-history_date", "-history_id").first().history_user + + +def _history_count(model_name: str, pk: int) -> int: + """Return how many historical rows exist for the ``model_name`` row ``pk``.""" + return apps.get_model("openedx_learning", model_name).objects.filter(id=pk).count() + + +def test_repeat_delete_on_an_archived_criterion_writes_no_history( user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, ) -> None: - """A second DELETE against an already-archived criterion re-enters the archive branch without error.""" - group = CompetencyCriteriaGroup.objects.create(tag=tag) - criterion = _make_criterion(group, object_tag, default_rule_profile) + """ + A second delete of an already-archived criterion returns archived=True and leaves the + criterion and its groups untouched, so no new historical row is written for any of them. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) StudentCompetencyCriterionStatus.objects.create( user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, ) delete_or_archive_competency_criterion(criterion.id, user) + counts = ( + _history_count("HistoricalCompetencyCriterion", criterion.id), + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) result = delete_or_archive_competency_criterion(criterion.id, user) @@ -111,6 +132,29 @@ def test_repeat_call_on_an_already_archived_criterion_is_idempotent( assert result.archived is True criterion.refresh_from_db() assert criterion.archived is True + assert counts == ( + _history_count("HistoricalCompetencyCriterion", criterion.id), + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) + + +def test_archive_records_the_acting_user_in_history( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """Archiving a criterion records the acting user on the newest historical row of it and of each archived group.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + delete_or_archive_competency_criterion(criterion.id, user) + + assert _latest_history_user("HistoricalCompetencyCriterion", criterion.id) == user + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", leaf.id) == user + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", root.id) == user def test_shared_object_tag_is_untouched_while_another_criterion_still_references_it( @@ -159,6 +203,10 @@ def test_single_child_leaf_group_cascade_deletes_while_a_sibling_keeps_the_root_ assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() assert CompetencyCriteriaGroup.objects.filter(id=sibling.id).exists() + root.refresh_from_db() + sibling.refresh_from_db() + assert root.archived is False + assert sibling.archived is False def test_multilevel_cascade_fully_collapses_the_root_to_leaf_chain( @@ -204,31 +252,72 @@ def test_cascade_stops_at_an_ancestor_with_a_surviving_sibling_group( assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() assert CompetencyCriteriaGroup.objects.filter(id=other_branch.id).exists() assert CompetencyCriterion.objects.filter(id=other_criterion.id).exists() + root.refresh_from_db() + other_branch.refresh_from_db() + assert root.archived is False + assert other_branch.archived is False +@pytest.mark.parametrize("progress_on", ["leaf group", "root group"]) def test_group_with_its_own_status_is_archived_not_deleted_at_zero_remaining_children( - user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, + progress_on: str, user, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, ) -> None: """ - A leaf group carrying its own StudentCompetencyCriteriaGroupStatus is archived, not - hard-deleted, even once its last criterion is gone -- while that criterion itself, with no - status of its own, is still hard-deleted. Archiving the leaf then cascades upward as - "archive", so an ancestor with no other content is archived too, not deleted. + A group carrying its own StudentCompetencyCriteriaGroupStatus is archived, not hard-deleted, + once its last child is gone, whether that group is the leaf or the root above it (depth 2). + The criterion itself, with no status of its own, is still hard-deleted, and an archive + cascades upward as "archive", so every ancestor of an archived group is archived too. """ + assert object_tag.tag is not None + tag = object_tag.tag root = CompetencyCriteriaGroup.objects.create(tag=tag) leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) criterion = _make_criterion(leaf, object_tag, default_rule_profile) StudentCompetencyCriteriaGroupStatus.objects.create( - user=user, group=leaf, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + user=user, group=leaf if progress_on == "leaf group" else root, + status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, ) result = delete_or_archive_competency_criterion(criterion.id, user) assert result.archived is False assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() + root.refresh_from_db() + assert root.archived is True + if progress_on == "leaf group": + leaf.refresh_from_db() + assert leaf.archived is True + else: + assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + + +def test_hard_delete_beside_an_archived_criterion_archives_the_group_and_its_parent( + user, competency_taxonomy: CompetencyTaxonomy, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + A leaf holding one archived criterion (it has learner progress) and one live criterion: hard-deleting + the live one removes it, and the leaf, left with only archived children, is archived with its parent. + """ + assert object_tag.tag is not None + tag = object_tag.tag + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + live = _make_criterion(leaf, object_tag, default_rule_profile) + archived_object_tag = _make_object_tag(competency_taxonomy, tag, "obj-archived") + archived = _make_criterion(leaf, archived_object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=archived, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + delete_or_archive_competency_criterion(archived.id, user) + + result = delete_or_archive_competency_criterion(live.id, user) + + assert result.archived is False + assert not CompetencyCriterion.objects.filter(id=live.id).exists() leaf.refresh_from_db() - assert leaf.archived is True root.refresh_from_db() + assert leaf.archived is True assert root.archived is True @@ -292,3 +381,35 @@ def test_does_not_exist_for_an_unknown_criterion_id(user) -> None: """A criterion_id with no matching row raises CompetencyCriterion.DoesNotExist.""" with pytest.raises(CompetencyCriterion.DoesNotExist): delete_or_archive_competency_criterion(999999999, user) + + +# ================================================================================================= +# The walk stops at archived groups +# ================================================================================================= + + +def test_cascade_stops_at_a_group_that_is_already_archived( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + Archiving a criterion whose group is already archived leaves that group and its parent alone, so + neither gets a new historical row: everything above an archived group was handled when it was archived. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag, archived=True) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, archived=True) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + counts = ( + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.archived is True + assert counts == ( + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) From 3ee7d880425723c03c392396992baf7888d53eb8 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Mon, 26 Oct 2026 18:36:05 +0300 Subject: [PATCH 07/24] fix: untag only the object tag the tagging API reports (#674) Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 22 ++++++++--- .../applets/cbe/test_criterion_removal.py | 38 ++++++++++++++++++- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 860da2886..ecbf3c2ea 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -1044,12 +1044,22 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> return CriterionDeletionResult(id=criterion_id, archived=True) group = criterion.group object_tag = criterion.object_tag + untag = not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exclude(pk=criterion_id).exists() + if untag: + tagged = get_object_tags(object_tag.object_id, taxonomy_id=object_tag.taxonomy_id) + # get_object_tags hides the tags of a disabled taxonomy, but tag_object deletes every + # ObjectTag missing from its list, so a keep list built from the filtered rows would + # delete tags it never saw. Refuse before writing anything. + if object_tag.id not in {t.id for t in tagged}: + raise ValidationError({"object_id": _( + "The tagging API does not report this criterion's object tag, so it cannot be untagged safely." + )}) criterion.delete() - if not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists(): - existing_values = [ - t.value for t in get_object_tags(object_tag.object_id, taxonomy_id=object_tag.taxonomy_id) - if t.value != group.tag.value - ] - tag_object(object_tag.object_id, group.tag.taxonomy, existing_values) + if untag: + tag_object( + object_tag.object_id, + group.tag.taxonomy, + [t.value for t in tagged if t.value != group.tag.value], + ) _cascade_from_group(group, "delete", user) return CriterionDeletionResult(id=criterion_id, archived=False) diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index e3321c287..5fcd49bbd 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -5,7 +5,7 @@ import pytest import rules from django.apps import apps -from django.core.exceptions import PermissionDenied +from django.core.exceptions import PermissionDenied, ValidationError from rules.permissions import permissions as rule_permissions from openedx_learning.api import delete_or_archive_competency_criterion @@ -23,6 +23,7 @@ pytestmark = pytest.mark.django_db CHANGE_OBJECTTAG_OBJECTID = "oel_tagging.change_objecttag_objectid" +CHANGE_OBJECTTAG_TAXONOMY = "oel_tagging.change_objecttag_taxonomy" @pytest.fixture(autouse=True) @@ -372,6 +373,41 @@ def test_a_user_without_permission_is_refused_and_nothing_changes( assert ObjectTag.objects.filter(id=object_tag.id).exists() +def test_untag_with_the_competency_taxonomy_disabled_raises_and_deletes_nothing( + user, + competency_taxonomy: CompetencyTaxonomy, + tag: Tag, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + With the taxonomy disabled the tagging API hides its tags, so the untag cannot name the right + ObjectTag: the call raises on object_id and neither criterion nor ObjectTag is touched. The + taxonomy-level permission rule denies every user on a disabled taxonomy, so the test substitutes + a permissive one to reach the untag step. + """ + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Reading Poetry") + other_object_tag = _make_object_tag(competency_taxonomy, other_tag, object_tag.object_id) + criterion = _make_criterion(CompetencyCriteriaGroup.objects.create(tag=tag), object_tag, default_rule_profile) + other_criterion = _make_criterion( + CompetencyCriteriaGroup.objects.create(tag=other_tag), other_object_tag, default_rule_profile + ) + competency_taxonomy.enabled = False + competency_taxonomy.save() + original = rule_permissions[CHANGE_OBJECTTAG_TAXONOMY] + rules.set_perm(CHANGE_OBJECTTAG_TAXONOMY, lambda _user, _taxonomy: True) + + try: + with pytest.raises(ValidationError) as exc_info: + delete_or_archive_competency_criterion(criterion.id, user) + finally: + rules.set_perm(CHANGE_OBJECTTAG_TAXONOMY, original) + + assert "object_id" in exc_info.value.message_dict + assert CompetencyCriterion.objects.filter(id__in=[criterion.id, other_criterion.id]).count() == 2 + assert ObjectTag.objects.filter(id__in=[object_tag.id, other_object_tag.id]).count() == 2 + + # ================================================================================================= # Not found # ================================================================================================= From 1b668cb46d9de012b2867679c988baac6ab21664 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Mon, 26 Oct 2026 18:38:12 +0300 Subject: [PATCH 08/24] docs: correct the delete view and cascade docstrings and tidy the tests (#674) Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 5 +++-- .../applets/cbe/rest_api/v1/views.py | 6 +++--- .../applets/cbe/test_criterion_removal.py | 4 +--- tests/openedx_learning/applets/cbe/test_views.py | 12 ++++++------ 4 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index ecbf3c2ea..94dc829f5 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -1037,7 +1037,7 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> Tag.objects.select_for_update().get(pk=tag_id) # Evaluated so the locks are taken now, in primary-key order to avoid deadlocks between writers. list(CompetencyCriteriaGroup.objects.select_for_update().filter(pk__in=lineage_ids).order_by("pk")) - # A concurrent delete may have won; DoesNotExist propagates and the view maps it to 404. + # A concurrent delete may have won; DoesNotExist propagates to the caller. criterion = CompetencyCriterion.objects.select_for_update().get(pk=criterion_id) if _criterion_has_learner_progress(criterion_id): _archive_criterion(criterion, user) @@ -1049,7 +1049,8 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> tagged = get_object_tags(object_tag.object_id, taxonomy_id=object_tag.taxonomy_id) # get_object_tags hides the tags of a disabled taxonomy, but tag_object deletes every # ObjectTag missing from its list, so a keep list built from the filtered rows would - # delete tags it never saw. Refuse before writing anything. + # delete tags it never saw. Refuse before writing anything. This is defense in depth: + # the permission check above already refuses a disabled taxonomy. if object_tag.id not in {t.id for t in tagged}: raise ValidationError({"object_id": _( "The tagging API does not report this criterion's object tag, so it cannot be untagged safely." diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/views.py b/src/openedx_learning/applets/cbe/rest_api/v1/views.py index 60e8852e0..33ca520b5 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -292,9 +292,9 @@ def delete(self, request, *args, **kwargs): """ Resolve the criterion scoped to both path segments, then delegate to the public API, which checks permission. - Scoped by tag_id rather than a specific group_id, matching CompetencyCriterionCreateView's - own URL convention; a group-scoped delete is a separate ticket's concern (direct group - deletion/archival). The lookup deliberately does not filter `archived=False`, so a repeat + The URL is scoped by tag_id, not group_id, because its caller (#710) sends exactly + ``DELETE .../competencies//criteria//`` and has no group id. The + lookup deliberately does not filter `archived=False`, so a repeat DELETE against an already-archived row still resolves (200), and a mismatched tag_id or a nonexistent criterion_id both 404 the same way, via this one get_object_or_404 call. """ diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index 5fcd49bbd..c4b638717 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -347,8 +347,6 @@ def test_archiving_a_groups_only_criterion_archives_the_group_and_cascades_upwar root.refresh_from_db() assert leaf.archived is True assert root.archived is True - assert CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() - assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() # ================================================================================================= @@ -414,7 +412,7 @@ def test_untag_with_the_competency_taxonomy_disabled_raises_and_deletes_nothing( def test_does_not_exist_for_an_unknown_criterion_id(user) -> None: - """A criterion_id with no matching row raises CompetencyCriterion.DoesNotExist.""" + """The API layer lets DoesNotExist propagate for an unknown criterion_id so each caller can map it itself.""" with pytest.raises(CompetencyCriterion.DoesNotExist): delete_or_archive_competency_criterion(999999999, user) diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 7e8b425e6..ec5775dd8 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -2016,14 +2016,14 @@ def test_group_detail_offers_only_patch( def _criterion( group: CompetencyCriteriaGroup, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile ) -> CompetencyCriterion: - """A leaf-agnostic CompetencyCriterion directly under the root `group`, for the delete-view tests.""" + """A CompetencyCriterion directly under the root `group`, for the delete-view tests.""" # The conftest object_tag's object_id isn't a parseable usage key, which _allow_object_level_tagging denies. object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) return CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=default_rule_profile) def test_hard_delete_200s_with_archived_false(user_client: APIClient, criterion: CompetencyCriterion) -> None: - """Deleting a criterion with no learner status 200s, hard-deletes it, and reports archived=false.""" + """With no learner status on the criterion, the DELETE removes the row and the body says archived=false.""" response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) assert response.status_code == status.HTTP_200_OK @@ -2034,7 +2034,7 @@ def test_hard_delete_200s_with_archived_false(user_client: APIClient, criterion: def test_archive_200s_with_archived_true( user_client: APIClient, user, criterion: CompetencyCriterion, now: datetime, ) -> None: - """Deleting a criterion a learner status row references 200s, archives it, and reports archived=true.""" + """With a learner status on the criterion, the DELETE keeps the row, flags it archived, and says archived=true.""" StudentCompetencyCriterionStatus.objects.create( user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, ) @@ -2063,7 +2063,7 @@ def test_repeat_delete_on_an_archived_criterion_is_idempotent_200( def test_unknown_criterion_id_404s(user_client: APIClient, tag: Tag) -> None: - """A criterion_id with no matching row 404s.""" + """A criterion_id that never existed under a real tag 404s from the view's own lookup.""" response = user_client.delete(criterion_delete_url(tag.id, 999999)) assert response.status_code == status.HTTP_404_NOT_FOUND @@ -2072,7 +2072,7 @@ def test_unknown_criterion_id_404s(user_client: APIClient, tag: Tag) -> None: def test_repeat_delete_after_hard_delete_404s( user_client: APIClient, criterion: CompetencyCriterion, ) -> None: - """A second DELETE against the same URL, after a hard-delete already removed the row, 404s.""" + """Repeating a DELETE after a hard-delete removed the row is not idempotent like the archived case: it 404s.""" url = criterion_delete_url(criterion.group.tag_id, criterion.id) first = user_client.delete(url) assert first.status_code == status.HTTP_200_OK @@ -2123,7 +2123,7 @@ def test_delete_no_permission_is_403( user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, tag: Tag, group: CompetencyCriteriaGroup, default_rule_profile: CompetencyRuleProfile, ) -> None: - """A caller without object-level tagging permission on the criterion's object is refused with a 403.""" + """A caller refused by the object-level tagging rule gets a 403, and the criterion and ObjectTag remain.""" object_tag = ObjectTag.objects.create( object_id=f"block-v1:unused+{UNAUTHORIZED_MARKER}+unused+type@sequential+block@p1", taxonomy=competency_taxonomy, From 6489d3d6e9f7b19349b25a6eb048e075ea01437c Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Mon, 26 Oct 2026 18:42:44 +0300 Subject: [PATCH 09/24] fix: map validation errors in the delete view and read the lineage under the tag lock (#674) Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 12 ++++++++---- .../applets/cbe/rest_api/v1/views.py | 2 ++ tests/openedx_learning/applets/cbe/test_views.py | 15 +++++++++++++++ 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 94dc829f5..d00d1eefa 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -725,7 +725,10 @@ def bulk_update_competency_criteria( # Locked for the same reason as the criteria below. lineage = [group.id, group.parent.id, group.parent.parent_id] archived_by_id = dict( - CompetencyCriteriaGroup.objects.select_for_update().filter(pk__in=lineage).values_list("pk", "archived") + CompetencyCriteriaGroup.objects.select_for_update() + .filter(pk__in=lineage) + .order_by("pk") + .values_list("pk", "archived") ) archived_group_id = next((pk for pk in lineage if archived_by_id.get(pk)), None) if archived_group_id is not None: @@ -1014,7 +1017,9 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> otherwise. See ADR-0002 Decision 2. Raises CompetencyCriterion.DoesNotExist if no such criterion exists (including one deleted - concurrently) and PermissionDenied if ``user`` may not tag the criterion's object. + concurrently) and PermissionDenied if ``user`` may not tag the criterion's object. Raises + ValidationError keyed ``object_id`` if the criterion's ObjectTag must be removed but the tagging + API does not report it, which the permission check normally prevents. The whole operation runs in one transaction that locks the competency's Tag, then the criterion's group and every ancestor group, then the criterion. A concurrent create for the @@ -1031,10 +1036,9 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> if not user.has_perm("oel_tagging.can_tag_object", cast(Any, perm_obj)): raise PermissionDenied() tag_id = criterion.group.tag_id - # Tree changes are serialized by the Tag lock, so reading the lineage before taking it is safe. - lineage_ids = _lineage_group_ids(criterion.group) with transaction.atomic(): Tag.objects.select_for_update().get(pk=tag_id) + lineage_ids = _lineage_group_ids(criterion.group) # Evaluated so the locks are taken now, in primary-key order to avoid deadlocks between writers. list(CompetencyCriteriaGroup.objects.select_for_update().filter(pk__in=lineage_ids).order_by("pk")) # A concurrent delete may have won; DoesNotExist propagates to the caller. diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/views.py b/src/openedx_learning/applets/cbe/rest_api/v1/views.py index 33ca520b5..b7afaa7e6 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -306,4 +306,6 @@ def delete(self, request, *args, **kwargs): except CompetencyCriterion.DoesNotExist as exc: # Removed between the lookup above and the API call. raise NotFound() from exc + except DjangoValidationError as exc: + raise DRFValidationError(exc.message_dict if hasattr(exc, "message_dict") else exc.messages) from exc return Response({"id": result.id, "archived": result.archived}, status=status.HTTP_200_OK) diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index ec5775dd8..a07576530 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -11,6 +11,7 @@ import pytest import rules +from django.core.exceptions import ValidationError from django.urls import reverse from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey @@ -2165,3 +2166,17 @@ def test_delete_of_a_criterion_gone_by_the_time_of_the_call_is_404( response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_delete_maps_a_validation_error_from_the_api_to_400( + user_client: APIClient, criterion: CompetencyCriterion, +) -> None: + """A Django ValidationError raised by the API function becomes a 400 keyed by its field.""" + with mock.patch( + "openedx_learning.applets.cbe.rest_api.v1.views.delete_or_archive_competency_criterion", + side_effect=ValidationError({"object_id": "x"}), + ): + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "object_id" in response.json() From 86ccf36d64d568d480aad864cf236f4e3d69089b Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 28 Oct 2026 01:57:42 +0300 Subject: [PATCH 10/24] fix: remove only the orphaned object tag when deleting a criterion (#674) Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 35 +++++-------- .../applets/cbe/test_criterion_removal.py | 52 +++++++++++++++---- 2 files changed, 54 insertions(+), 33 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index d00d1eefa..d8ac679b0 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -1017,13 +1017,14 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> otherwise. See ADR-0002 Decision 2. Raises CompetencyCriterion.DoesNotExist if no such criterion exists (including one deleted - concurrently) and PermissionDenied if ``user`` may not tag the criterion's object. Raises - ValidationError keyed ``object_id`` if the criterion's ObjectTag must be removed but the tagging - API does not report it, which the permission check normally prevents. + concurrently) and PermissionDenied if ``user`` may not tag the criterion's object. + + On a hard delete, the criterion's own ObjectTag is deleted too, and only when no other + criterion references it. No other ObjectTag on the object is touched. The whole operation runs in one transaction that locks the competency's Tag, then the - criterion's group and every ancestor group, then the criterion. A concurrent create for the - same competency waits on the Tag lock. A learner-progress insert on the criterion or on any + criterion's group and every ancestor group, then the criterion, then its ObjectTag. A concurrent + create for the same competency waits on the Tag lock. A learner-progress insert on the criterion or on any ancestor group waits on that row's lock. The progress checks therefore cannot go stale, so no fallback or retry is needed. """ @@ -1048,23 +1049,13 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> return CriterionDeletionResult(id=criterion_id, archived=True) group = criterion.group object_tag = criterion.object_tag - untag = not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exclude(pk=criterion_id).exists() - if untag: - tagged = get_object_tags(object_tag.object_id, taxonomy_id=object_tag.taxonomy_id) - # get_object_tags hides the tags of a disabled taxonomy, but tag_object deletes every - # ObjectTag missing from its list, so a keep list built from the filtered rows would - # delete tags it never saw. Refuse before writing anything. This is defense in depth: - # the permission check above already refuses a disabled taxonomy. - if object_tag.id not in {t.id for t in tagged}: - raise ValidationError({"object_id": _( - "The tagging API does not report this criterion's object tag, so it cannot be untagged safely." - )}) criterion.delete() - if untag: - tag_object( - object_tag.object_id, - group.tag.taxonomy, - [t.value for t in tagged if t.value != group.tag.value], - ) + # Only this criterion's own ObjectTag is a candidate, and its lock comes last. Inserting a + # criterion takes a foreign key lock on its ObjectTag, so the re-check under this lock sees + # any criterion committed for it in the meantime, and that criterion keeps its ObjectTag. + locked_object_tag = ObjectTag.objects.select_for_update().filter(pk=object_tag.id).first() + still_referenced = CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists() + if locked_object_tag is not None and not still_referenced: + locked_object_tag.delete() _cascade_from_group(group, "delete", user) return CriterionDeletionResult(id=criterion_id, archived=False) diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index c4b638717..6a8a57ff4 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -5,7 +5,7 @@ import pytest import rules from django.apps import apps -from django.core.exceptions import PermissionDenied, ValidationError +from django.core.exceptions import PermissionDenied from rules.permissions import permissions as rule_permissions from openedx_learning.api import delete_or_archive_competency_criterion @@ -371,7 +371,7 @@ def test_a_user_without_permission_is_refused_and_nothing_changes( assert ObjectTag.objects.filter(id=object_tag.id).exists() -def test_untag_with_the_competency_taxonomy_disabled_raises_and_deletes_nothing( +def test_untag_with_the_competency_taxonomy_disabled_removes_only_the_orphaned_object_tag( user, competency_taxonomy: CompetencyTaxonomy, tag: Tag, @@ -379,10 +379,10 @@ def test_untag_with_the_competency_taxonomy_disabled_raises_and_deletes_nothing( default_rule_profile: CompetencyRuleProfile, ) -> None: """ - With the taxonomy disabled the tagging API hides its tags, so the untag cannot name the right - ObjectTag: the call raises on object_id and neither criterion nor ObjectTag is touched. The - taxonomy-level permission rule denies every user on a disabled taxonomy, so the test substitutes - a permissive one to reach the untag step. + With the taxonomy disabled the tagging API hides its tags, but the delete removes only the + criterion's own ObjectTag, so it succeeds and the object's other ObjectTag and its criterion + stay. The taxonomy-level permission rule denies every user on a disabled taxonomy, so the test + substitutes a permissive one to reach the untag step. """ other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Reading Poetry") other_object_tag = _make_object_tag(competency_taxonomy, other_tag, object_tag.object_id) @@ -396,14 +396,44 @@ def test_untag_with_the_competency_taxonomy_disabled_raises_and_deletes_nothing( rules.set_perm(CHANGE_OBJECTTAG_TAXONOMY, lambda _user, _taxonomy: True) try: - with pytest.raises(ValidationError) as exc_info: - delete_or_archive_competency_criterion(criterion.id, user) + result = delete_or_archive_competency_criterion(criterion.id, user) finally: rules.set_perm(CHANGE_OBJECTTAG_TAXONOMY, original) - assert "object_id" in exc_info.value.message_dict - assert CompetencyCriterion.objects.filter(id__in=[criterion.id, other_criterion.id]).count() == 2 - assert ObjectTag.objects.filter(id__in=[object_tag.id, other_object_tag.id]).count() == 2 + assert result.archived is False + assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() + assert not ObjectTag.objects.filter(id=object_tag.id).exists() + assert CompetencyCriterion.objects.filter(id=other_criterion.id).exists() + assert ObjectTag.objects.filter(id=other_object_tag.id).exists() + + +def test_hard_delete_leaves_a_tag_of_another_competency_on_the_same_object( + user, + monkeypatch: pytest.MonkeyPatch, + tag: Tag, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + An ObjectTag for another competency tag on the same object, committed after a tag list was + read, survives the delete together with its criterion. The tag list is stubbed to omit it, as + a read taken before that commit would; the delete must not depend on reading it. + """ + competency_taxonomy = CompetencyTaxonomy.objects.get() + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Reading Poetry") + other_object_tag = _make_object_tag(competency_taxonomy, other_tag, object_tag.object_id) + criterion = _make_criterion(CompetencyCriteriaGroup.objects.create(tag=tag), object_tag, default_rule_profile) + other_criterion = _make_criterion( + CompetencyCriteriaGroup.objects.create(tag=other_tag), other_object_tag, default_rule_profile + ) + stale_list = [ObjectTag.objects.get(pk=object_tag.pk)] + monkeypatch.setattr("openedx_learning.applets.cbe.api.get_object_tags", lambda *_args, **_kwargs: stale_list) + + delete_or_archive_competency_criterion(criterion.id, user) + + assert not ObjectTag.objects.filter(id=object_tag.id).exists() + assert ObjectTag.objects.filter(id=other_object_tag.id).exists() + assert CompetencyCriterion.objects.filter(id=other_criterion.id).exists() # ================================================================================================= From 5f6ba29822b24125e3e0792467dd9dd69426ae56 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Thu, 29 Oct 2026 19:33:08 +0300 Subject: [PATCH 11/24] fix: lock only the tag when deleting a criterion (#674) Drop the ancestor-group, criterion and ObjectTag row locks from delete_or_archive_competency_criterion. The criterion is still re-read inside the transaction after the competency's Tag lock, the same lock a create for that competency takes first, so a concurrent create still waits for the delete. The criterion's ObjectTag is deleted only when no other criterion references it, as before. Also revert the pk ordering this PR added to the group lock in bulk_update_competency_criteria; it only kept a common lock order with the ancestor locks removed here. Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 39 +++++-------------------- 1 file changed, 8 insertions(+), 31 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index d8ac679b0..7345be859 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -725,10 +725,7 @@ def bulk_update_competency_criteria( # Locked for the same reason as the criteria below. lineage = [group.id, group.parent.id, group.parent.parent_id] archived_by_id = dict( - CompetencyCriteriaGroup.objects.select_for_update() - .filter(pk__in=lineage) - .order_by("pk") - .values_list("pk", "archived") + CompetencyCriteriaGroup.objects.select_for_update().filter(pk__in=lineage).values_list("pk", "archived") ) archived_group_id = next((pk for pk in lineage if archived_by_id.get(pk)), None) if archived_group_id is not None: @@ -991,16 +988,6 @@ def _group_has_learner_progress(group_id: int) -> bool: return StudentCompetencyCriteriaGroupStatus.objects.filter(group_id=group_id).exists() -def _lineage_group_ids(group: CompetencyCriteriaGroup) -> list[int]: - """Return the ids of ``group`` and every ancestor up to the root, one query per level.""" - ids = [group.pk] - parent_id = group.parent_id - while parent_id is not None: - ids.append(parent_id) - parent_id = CompetencyCriteriaGroup.objects.values_list("parent_id", flat=True).get(pk=parent_id) - return ids - - def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> CriterionDeletionResult: """ Hard-delete `criterion_id`, or archive it if a learner status row already references it. @@ -1022,11 +1009,8 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> On a hard delete, the criterion's own ObjectTag is deleted too, and only when no other criterion references it. No other ObjectTag on the object is touched. - The whole operation runs in one transaction that locks the competency's Tag, then the - criterion's group and every ancestor group, then the criterion, then its ObjectTag. A concurrent - create for the same competency waits on the Tag lock. A learner-progress insert on the criterion or on any - ancestor group waits on that row's lock. The progress checks therefore cannot go stale, so - no fallback or retry is needed. + The whole operation runs in one transaction that locks only the competency's Tag, the same lock + a create for that competency takes first, so a concurrent create waits until this call commits. """ criterion = CompetencyCriterion.objects.select_related("group__tag__taxonomy", "object_tag").get(pk=criterion_id) # A group's tag is always a competency tag, so its taxonomy is always set. @@ -1039,23 +1023,16 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> tag_id = criterion.group.tag_id with transaction.atomic(): Tag.objects.select_for_update().get(pk=tag_id) - lineage_ids = _lineage_group_ids(criterion.group) - # Evaluated so the locks are taken now, in primary-key order to avoid deadlocks between writers. - list(CompetencyCriteriaGroup.objects.select_for_update().filter(pk__in=lineage_ids).order_by("pk")) - # A concurrent delete may have won; DoesNotExist propagates to the caller. - criterion = CompetencyCriterion.objects.select_for_update().get(pk=criterion_id) + # Re-read under the Tag lock: a concurrent delete may have won; DoesNotExist propagates to the caller. + criterion = CompetencyCriterion.objects.get(pk=criterion_id) if _criterion_has_learner_progress(criterion_id): _archive_criterion(criterion, user) return CriterionDeletionResult(id=criterion_id, archived=True) group = criterion.group object_tag = criterion.object_tag criterion.delete() - # Only this criterion's own ObjectTag is a candidate, and its lock comes last. Inserting a - # criterion takes a foreign key lock on its ObjectTag, so the re-check under this lock sees - # any criterion committed for it in the meantime, and that criterion keeps its ObjectTag. - locked_object_tag = ObjectTag.objects.select_for_update().filter(pk=object_tag.id).first() - still_referenced = CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists() - if locked_object_tag is not None and not still_referenced: - locked_object_tag.delete() + # Only this criterion's own ObjectTag is a candidate, and only while no other criterion uses it. + if not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists(): + ObjectTag.objects.filter(pk=object_tag.id).delete() _cascade_from_group(group, "delete", user) return CriterionDeletionResult(id=criterion_id, archived=False) From b1e27af5520a1e0ef0c049487ef66afe18b5ae2a Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Thu, 29 Oct 2026 19:33:08 +0300 Subject: [PATCH 12/24] fix: delete an emptied archived group on a criterion delete (#674) _cascade_from_group stopped at any archived group. After a learner whose status archived a criterion is deleted, a repeat delete hard-deletes the criterion and left its archived groups behind with nothing in them, breaking ADR-0002 Decision 2. On a delete event, an archived group left with no criteria, no child groups and no group-level status is now deleted and the walk continues upward. Otherwise the walk still stops at an archived group without saving it. Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 18 +++-- .../applets/cbe/test_criterion_removal.py | 72 +++++++++++++++++++ 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 7345be859..e5146aa27 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -937,21 +937,31 @@ def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", Checking both relations (`criteria` and `child_groups`) at every level is harmless: the one that does not apply is an empty queryset, and it avoids a separate leaf/non-leaf code path. - The walk stops at a group that is already archived. Everything above it was handled when it was - archived, so writing it again would only add a redundant history row. + An archived group is never written again: everything above it was handled when it was archived, + so a second write would only add a redundant history row. A delete event can still remove it. If + the delete left it with no criteria, no child groups and no group-level status, it is deleted and + the walk continues upward, so no empty group stays persisted. Otherwise the walk stops there. """ + parent = group.parent + next_event: Literal["delete", "archive"] if group.archived: + if event == "archive" or group.criteria.exists() or group.child_groups.exists(): + return + if _group_has_learner_progress(group.id): + return + group.delete() + if parent is not None: + _cascade_from_group(parent, "delete", user) return has_live_child = ( group.criteria.filter(archived=False).exists() or group.child_groups.filter(archived=False).exists() ) - parent = group.parent if has_live_child: return if event == "delete" and not group.criteria.exists() and not group.child_groups.exists(): if _group_has_learner_progress(group.id): _archive_group(group, user) - next_event: Literal["delete", "archive"] = "archive" + next_event = "archive" else: group.delete() next_event = "delete" diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index 6a8a57ff4..4e3a242b3 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -5,6 +5,7 @@ import pytest import rules from django.apps import apps +from django.contrib.auth import get_user_model from django.core.exceptions import PermissionDenied from rules.permissions import permissions as rule_permissions @@ -477,3 +478,74 @@ def test_cascade_stops_at_a_group_that_is_already_archived( _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), _history_count("HistoricalCompetencyCriteriaGroup", root.id), ) + + +def test_delete_after_the_learner_is_gone_deletes_the_emptied_archived_groups( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + Once the learner whose status archived a criterion is deleted, a repeat delete hard-deletes the + criterion, and every archived group it leaves with nothing in it is deleted too: no empty group stays. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + learner = get_user_model().objects.create(username="departing-learner") + StudentCompetencyCriterionStatus.objects.create( + user=learner, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + assert delete_or_archive_competency_criterion(criterion.id, user).archived is True + learner.delete() + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.archived is False + assert not CompetencyCriterion.objects.filter(pk=criterion.id).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, root.id]).exists() + + +def test_delete_leaves_an_archived_group_that_still_holds_an_archived_criterion( + user, tag: Tag, competency_taxonomy: CompetencyTaxonomy, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A hard delete under an archived group that still holds another criterion leaves that group and its + parent as they are, archived and with no new historical row. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag, archived=True) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, archived=True) + kept = _make_criterion(leaf, object_tag, default_rule_profile) + kept.archived = True + kept.save() + other_object_tag = _make_object_tag(competency_taxonomy, tag, "block-v1:Org1+Python100+Fall2026+problem+p2") + deleted = _make_criterion(leaf, other_object_tag, default_rule_profile) + counts = ( + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) + + result = delete_or_archive_competency_criterion(deleted.id, user) + + assert result.archived is False + assert CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, root.id], archived=True).count() == 2 + assert counts == ( + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) + + +def test_delete_leaves_an_emptied_archived_group_that_has_its_own_status( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """An archived group left empty by a hard delete stays when a learner group status still references it.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag, archived=True) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, archived=True) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=leaf, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.archived is False + assert CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, root.id], archived=True).count() == 2 From 0a717a281d5b54993f5f8c983b2096587b1caaaa Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 18:52:31 +0300 Subject: [PATCH 13/24] fix: serialize criterion writers on the competency's Tag row (#674) Deleting a criterion and bulk-updating it could deadlock. The delete takes the competency's Tag lock, writes the criterion row, then writes the leaf group and its ancestors. Bulk update locked the group rows and then the criterion rows without taking the Tag lock. When the delete holds the criterion and waits for the leaf group while bulk update holds the leaf group and waits for the criterion, MySQL kills one request with an unhandled OperationalError (1213). Every criterion authoring writer now follows one rule: take the Tag row lock first, read mutable state only after it, take no other row lock, and save several criteria in ascending primary-key order. Bulk update drops its two select_for_update() calls in favor of the Tag lock and reads the archived lineage and its criteria after it. It saves in primary-key order because each save takes an exclusive row lock and the grading write locks criteria in ascending order, so a different order would form a new cycle. The response keeps request order. PATCH of a group's logic operator takes the same lock and re-reads the group under it. A delete that archived or removed the group first now yields a clean archived error or a 404, instead of a write onto an archived group or a DatabaseError. The bulk update and delete views map Tag.DoesNotExist to 404, for a competency tag deleted while the request runs. Two-connection MySQL tests cover the delete against bulk update and against PATCH, for both a hard delete and an archive. Lock-point tests that run on SQLite pin that each writer reads after taking the lock. Co-Authored-By: Claude Fable 5 --- src/openedx_learning/applets/cbe/api.py | 34 ++- .../applets/cbe/rest_api/v1/views.py | 6 +- .../openedx_learning/applets/cbe/test_api.py | 61 +++- .../applets/cbe/test_criterion_concurrency.py | 263 ++++++++++++++++++ .../applets/cbe/test_criterion_removal.py | 32 +++ 5 files changed, 383 insertions(+), 13 deletions(-) create mode 100644 tests/openedx_learning/applets/cbe/test_criterion_concurrency.py diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index e5146aa27..8c556b979 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -699,7 +699,10 @@ def bulk_update_competency_criteria( if any named id isn't a criterion of that group, and ValidationError, keyed by the REST field names, for a non-leaf group, an empty or repeated id list, or an invalid rule source. Raises CompetencyCriterionArchivedError if the group, either of its ancestors, or any named criterion - is archived. + is archived. Raises Tag.DoesNotExist if the group's competency tag was deleted while the request ran. + + Takes the competency's Tag row lock before reading anything that can change, like every writer of + its criteria, so it waits for a concurrent create or delete of the same competency. """ with transaction.atomic(): group = CompetencyCriteriaGroup.objects.select_related("tag__taxonomy", "parent__course").get( @@ -720,13 +723,16 @@ def bulk_update_competency_criteria( raise ValidationError({"criterion_ids": _("criterion_ids must not name the same criterion twice.")}) _validate_rule_source(rule_profile_id, rule_type_override, rule_payload_override) + # Every writer of a competency's criteria serializes on its Tag row, and none takes another row lock, + # so the writers can't deadlock. Everything below reads committed state, since a delete, an archive + # or a create for this competency can't commit while this transaction holds the lock. The reads + # above are safe without it because a group's tag, parent and course never change. + Tag.objects.select_for_update().get(pk=group.tag_id) + # Archiving a group doesn't archive its criteria, yet ADR 0002 Decision 2 closes them to authoring. # An archived ancestor counts too: it stands in for a delete that would have cascaded to this leaf. - # Locked for the same reason as the criteria below. lineage = [group.id, group.parent.id, group.parent.parent_id] - archived_by_id = dict( - CompetencyCriteriaGroup.objects.select_for_update().filter(pk__in=lineage).values_list("pk", "archived") - ) + archived_by_id = dict(CompetencyCriteriaGroup.objects.filter(pk__in=lineage).values_list("pk", "archived")) archived_group_id = next((pk for pk in lineage if archived_by_id.get(pk)), None) if archived_group_id is not None: raise CompetencyCriterionArchivedError( @@ -735,9 +741,11 @@ def bulk_update_competency_criteria( ) ) - # Locked so a concurrent archive can't slip in after the archived check, and so a concurrent - # identical request waits and then sees nothing to change instead of writing a second history row. - criteria_by_id = CompetencyCriterion.objects.select_for_update().filter(group=group).in_bulk(criterion_ids) + # No row lock here: the Tag lock already stops a concurrent archive from slipping in after the archived + # check, and makes a concurrent identical request wait and then see nothing to change instead of + # writing a second history row. Locking the lineage or the criteria as well would only add a second + # lock order for writers to deadlock on. + criteria_by_id = CompetencyCriterion.objects.filter(group=group).in_bulk(criterion_ids) missing_ids = [criterion_id for criterion_id in criterion_ids if criterion_id not in criteria_by_id] if missing_ids: raise CompetencyCriterion.DoesNotExist( @@ -759,7 +767,9 @@ def bulk_update_competency_criteria( # every criterion in a leaf shares the group's tag, so they all resolve to the same profile. applicable = _get_applicable_rule_profile(group.tag.taxonomy.id) if rule_profile_id is None else None criteria = [criteria_by_id[criterion_id] for criterion_id in criterion_ids] - for criterion in criteria: + # Saves run in primary-key order because each save() takes an exclusive row lock and the grading + # write locks criteria in ascending order; two different orders would deadlock. + for criterion in sorted(criteria, key=lambda item: item.pk): _apply_rule_source( criterion, rule_profile_id, @@ -874,6 +884,9 @@ def update_competency_criteria_group( PermissionDenied if ``user`` fails ``oel_tagging.can_tag_object`` for the group's taxonomy and course. Raises CompetencyCriteriaGroupArchivedError if the group is archived, even when nothing would change. + + Takes the competency's Tag row lock and re-reads the group under it, like every writer of the + competency's criteria, so a concurrent delete that archived or removed the group is seen. """ with transaction.atomic(): group = CompetencyCriteriaGroup.objects.select_related("course", "parent__course").get(pk=group_id) @@ -882,6 +895,9 @@ def update_competency_criteria_group( tag = resolve_competency_tag(group.tag_id) # tag.taxonomy is already confirmed set; re-asserted since that doesn't cross function boundaries for mypy. assert tag.taxonomy is not None + Tag.objects.select_for_update().get(pk=tag.pk) + # Re-read under the Tag lock: a concurrent delete or archive may have won. + group = CompetencyCriteriaGroup.objects.select_related("course", "parent__course").get(pk=group_id) # A root has neither a parent nor a course, and its subtree can span several courses, so # there's no one course to check permission against. if group.parent_id is None and group.course_id is None: diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/views.py b/src/openedx_learning/applets/cbe/rest_api/v1/views.py index b7afaa7e6..feedd7f0b 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -166,7 +166,7 @@ def patch(self, request, group_id: int): rule_payload_override=data.get("rule_payload_override"), user=request.user, ) - except (CompetencyCriteriaGroup.DoesNotExist, CompetencyCriterion.DoesNotExist) as exc: + except (CompetencyCriteriaGroup.DoesNotExist, CompetencyCriterion.DoesNotExist, Tag.DoesNotExist) as exc: raise NotFound(str(exc)) from exc except DjangoValidationError as exc: raise DRFValidationError(exc.message_dict if hasattr(exc, "message_dict") else exc.messages) from exc @@ -303,8 +303,8 @@ def delete(self, request, *args, **kwargs): ) try: result = delete_or_archive_competency_criterion(criterion.id, request.user) - except CompetencyCriterion.DoesNotExist as exc: - # Removed between the lookup above and the API call. + except (CompetencyCriterion.DoesNotExist, Tag.DoesNotExist) as exc: + # The criterion, or its competency tag, was removed between the lookup above and the API call. raise NotFound() from exc except DjangoValidationError as exc: raise DRFValidationError(exc.message_dict if hasattr(exc, "message_dict") else exc.messages) from exc diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index 52f844e07..18fe627e0 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -2,7 +2,7 @@ Tests for the CBE public API surface (openedx_learning.api). """ import re -from collections.abc import Iterator +from collections.abc import Callable, Iterator from typing import Any import pytest @@ -632,6 +632,32 @@ def snapshot(criteria) -> dict[int, tuple]: return {criterion.id: (rule_state(criterion), history_of(criterion).count()) for criterion in criteria} +def run_when_tag_lock_is_taken(monkeypatch: pytest.MonkeyPatch, side_effect: Callable[[], None]) -> None: + """ + Run ``side_effect`` once, as the API takes the Tag row lock and before it reads the Tag. + + It stands in for a transaction that commits while the API waits for the lock, so a read the API + makes after taking the lock sees the change and a read it made earlier doesn't. + """ + original_select_for_update = Tag.objects.select_for_update + fired: list[bool] = [] + + def _select_for_update(*args, **kwargs): + queryset = original_select_for_update(*args, **kwargs) + original_get = queryset.get + + def _get(*get_args, **get_kwargs): + if not fired: + fired.append(True) + side_effect() + return original_get(*get_args, **get_kwargs) + + queryset.get = _get + return queryset + + monkeypatch.setattr(Tag.objects, "select_for_update", _select_for_update) + + def test_bulk_update_gives_every_criterion_the_rule_values_whatever_its_starting_source( batch: tuple[CompetencyCriterion, CompetencyCriterion], leaf: CompetencyCriteriaGroup, user, ) -> None: @@ -1041,6 +1067,24 @@ def _fail_on_second_save(self, *args, **kwargs) -> None: assert snapshot(batch) == before +def test_bulk_update_checks_archived_state_under_the_tag_lock( + monkeypatch: pytest.MonkeyPatch, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, +) -> None: + """A group archived while the update waits for the Tag lock refuses the batch, so the check reads after the lock.""" + run_when_tag_lock_is_taken( + monkeypatch, lambda: CompetencyCriteriaGroup.objects.filter(pk=leaf.pk).update(archived=True), + ) + before = snapshot(batch) + + with pytest.raises(CompetencyCriterionArchivedError, match=f"group {leaf.id}"): + bulk_update_competency_criteria([c.id for c in batch], leaf.id, user=user, **NEW_RULE) + + assert snapshot(batch) == before + + # ============================================================================================== # get_competency_criteria_tree # ============================================================================================== @@ -2237,6 +2281,21 @@ def test_update_raises_does_not_exist_for_an_unknown_group(user) -> None: update_competency_criteria_group(999999, logic_operator=LogicOperator.AND, user=user) +def test_group_update_re_reads_the_group_under_the_tag_lock( + monkeypatch: pytest.MonkeyPatch, leaf: CompetencyCriteriaGroup, user, +) -> None: + """A group archived while the update waits for the Tag lock is refused, not written onto.""" + run_when_tag_lock_is_taken( + monkeypatch, lambda: CompetencyCriteriaGroup.objects.filter(pk=leaf.pk).update(archived=True), + ) + + with pytest.raises(CompetencyCriteriaGroupArchivedError): + update_competency_criteria_group(leaf.id, logic_operator=LogicOperator.AND, user=user) + + leaf.refresh_from_db() + assert leaf.logic_operator == LogicOperator.OR + + @pytest.mark.parametrize("level", ["course-level", "leaf"]) def test_update_checks_permission_against_the_groups_course( # pylint: disable=too-many-positional-arguments level: str, diff --git a/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py b/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py new file mode 100644 index 000000000..36d4596d2 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py @@ -0,0 +1,263 @@ +""" +Two-connection tests for the row locks that criterion authoring writers take. + +Plain SQLite has no row locks, so these run only where ``select_for_update()`` is real (MySQL in CI). +Each test runs the delete in one thread and pauses it inside its transaction, then lets a second +connection act while the first still holds its locks. +""" +import threading +from collections.abc import Callable, Iterator +from datetime import datetime, timezone +from typing import Any + +import pytest +import rules +from django.apps import apps +from django.db import connection +from django.db.utils import OperationalError +from rules.permissions import permissions as rule_permissions + +from openedx_catalog.models import CourseRun +from openedx_learning.api import ( + CompetencyCriteriaGroupArchivedError, + CompetencyCriterionArchivedError, + bulk_update_competency_criteria, + delete_or_archive_competency_criterion, + update_competency_criteria_group, +) +from openedx_learning.applets.cbe import api as cbe_api +from openedx_learning.applets.cbe.api import _cascade_from_group, _create_leaf_group +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + LogicOperator, + MasteryStatus, + RuleType, + StudentCompetencyCriterionStatus, +) +from openedx_tagging.models import ObjectTag, Tag + +pytestmark = [ + pytest.mark.django_db(transaction=True), + pytest.mark.skipif(not connection.features.has_select_for_update, reason="needs real row locks"), + pytest.mark.usefixtures("migration_seed_rows"), +] + +CHANGE_OBJECTTAG_OBJECTID = "oel_tagging.change_objecttag_objectid" +NEW_RULE: dict[str, Any] = { + "rule_type_override": RuleType.GRADE, + "rule_payload_override": {"op": "gte", "value": 0.9, "scale": "percent"}, +} +# How long a test waits for a thread to reach a pause point or finish; far above any healthy run. +WAIT_SECONDS = 30 +# A worker blocked on a lock fails after this many seconds instead of MySQL's default fifty. +LOCK_WAIT_SECONDS = 4 + + +@pytest.fixture(autouse=True) +def _allow_object_level_tagging() -> Iterator[None]: + """Let any user tag any object; restore the real rule afterwards.""" + original = rule_permissions[CHANGE_OBJECTTAG_OBJECTID] + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, lambda _user, _object_id: True) + yield + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, original) + + +class Worker: + """ + A function running in its own thread, and so on its own database connection. + + The worker's exception is kept and re-raised by ``result()``, so a failure in the thread fails + the test instead of vanishing. + """ + + def __init__(self, func: Callable[..., Any], *args: Any, **kwargs: Any) -> None: + self._func, self._args, self._kwargs = func, args, kwargs + self._value: Any = None + self._error: BaseException | None = None + self.thread = threading.Thread(target=self._run, daemon=True) + self.thread.start() + + def _run(self) -> None: + """Thread body: cap lock waits, run the function, keep its outcome, and close the connection.""" + try: + with connection.cursor() as cursor: + cursor.execute(f"SET SESSION innodb_lock_wait_timeout = {LOCK_WAIT_SECONDS}") + self._value = self._func(*self._args, **self._kwargs) + except BaseException as error: + self._error = error + finally: + # The table flush after the test waits on any transaction this connection left open. + connection.close() + + def is_blocked(self) -> bool: + """Return whether the worker is still running after a short wait, i.e. it is waiting on a lock.""" + self.thread.join(timeout=1.0) + return self.thread.is_alive() + + def result(self) -> Any: + """Wait for the worker, then return its value or re-raise its exception.""" + self.thread.join(timeout=WAIT_SECONDS) + assert not self.thread.is_alive(), "worker is still running" + if self._error is not None: + raise self._error + return self._value + + +@pytest.fixture(name="start") +def _start() -> Iterator[Callable[..., Worker]]: + """Return a function that starts a Worker, and wait for every Worker it started once the test ends.""" + workers: list[Worker] = [] + + def _start_worker(func: Callable[..., Any], *args: Any, **kwargs: Any) -> Worker: + workers.append(Worker(func, *args, **kwargs)) + return workers[-1] + + yield _start_worker + for worker in workers: + worker.thread.join(timeout=WAIT_SECONDS) + + +class PausePoint: + """Two events that stop the first call of a wrapped function until the test lets it go.""" + + def __init__(self) -> None: + self.reached = threading.Event() + self.release = threading.Event() + self._tripped = False + + def wrap(self, original: Callable[..., Any]) -> Callable[..., Any]: + """Return ``original`` wrapped to signal ``reached`` and wait for ``release`` on its first call only.""" + + def _wrapper(*args: Any, **kwargs: Any) -> Any: + if not self._tripped: + self._tripped = True + self.reached.set() + if not self.release.wait(WAIT_SECONDS): + raise RuntimeError("pause point was never released") + return original(*args, **kwargs) + + return _wrapper + + def wait_until_reached(self) -> None: + """Wait for the paused thread to arrive.""" + assert self.reached.wait(WAIT_SECONDS), "the delete never reached its pause point" + + +def pause_before_the_walk(monkeypatch: pytest.MonkeyPatch) -> PausePoint: + """ + Pause the delete after it writes the criterion and before it walks up the groups. + + The walk calls itself at every level, so only its first call pauses. + """ + pause = PausePoint() + monkeypatch.setattr(cbe_api, "_cascade_from_group", pause.wrap(_cascade_from_group)) + return pause + + +def pause_after_the_walk(monkeypatch: pytest.MonkeyPatch) -> PausePoint: + """Pause the delete after the walk, when it builds its result, which is the last step inside its transaction.""" + pause = PausePoint() + monkeypatch.setattr(cbe_api, "CriterionDeletionResult", pause.wrap(cbe_api.CriterionDeletionResult)) + return pause + + +@pytest.fixture(name="learner_progress", params=[False, True], ids=["hard delete", "archive"]) +def _learner_progress(request: pytest.FixtureRequest) -> bool: + """Whether the criterion has a learner status row, which makes a delete archive it instead of removing it.""" + return request.param + + +@pytest.fixture(name="leaf_and_criterion") +def _leaf_and_criterion( + learner_progress: bool, + tag: Tag, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, + user, +) -> tuple[CompetencyCriteriaGroup, CompetencyCriterion]: + """A leaf group holding one criterion, with a learner status row on it if ``learner_progress``.""" + leaf = _create_leaf_group(tag, course_run) + key = course_run.course_key + assert key is not None + object_tag = ObjectTag.objects.create( + object_id=f"block-v1:{key.org}+{key.course}+{key.run}+type@sequential+block@s1", + taxonomy=tag.taxonomy, + tag=tag, + ) + criterion = CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, rule_profile=default_rule_profile) + if learner_progress: + now = datetime.now(timezone.utc) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + return leaf, criterion + + +def test_delete_and_bulk_update_do_not_deadlock( + learner_progress: bool, + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + start: Callable[..., Worker], + user, +) -> None: + """ + A bulk update of a criterion that is being deleted waits for the delete, then fails cleanly. + + Neither side may be killed by the database as a deadlock victim. After a hard delete the update finds + nothing to edit; after an archive it finds the criterion archived. + """ + leaf, criterion = leaf_and_criterion + pause = pause_before_the_walk(monkeypatch) + try: + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + pause.wait_until_reached() + bulk_update = start(bulk_update_competency_criteria, [criterion.id], leaf.id, user=user, **NEW_RULE) + assert bulk_update.is_blocked() + finally: + pause.release.set() + + # An OperationalError (MySQL error 1213, deadlock) raised by either worker is re-raised here. + result = delete.result() + expected: tuple[type[Exception], ...] + if learner_progress: + expected = (CompetencyCriterionArchivedError,) + else: + expected = (CompetencyCriterion.DoesNotExist, CompetencyCriteriaGroup.DoesNotExist) + with pytest.raises(expected) as raised: + bulk_update.result() + assert not isinstance(raised.value, OperationalError) + assert result.archived is learner_progress + + +def test_patch_does_not_write_onto_an_archived_or_deleted_group( + learner_progress: bool, + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + start: Callable[..., Worker], + user, +) -> None: + """ + A PATCH of a leaf's logic operator that races the delete of its only criterion changes nothing. + + The delete archives or deletes the leaf. The PATCH waits for it, then reports the group as archived + or missing instead of writing onto it. + """ + leaf, criterion = leaf_and_criterion + assert leaf.logic_operator == LogicOperator.OR + pause = pause_after_the_walk(monkeypatch) + try: + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + pause.wait_until_reached() + patch = start(update_competency_criteria_group, leaf.id, logic_operator=LogicOperator.AND, user=user) + assert patch.is_blocked() + finally: + pause.release.set() + + assert delete.result().archived is learner_progress + expected = CompetencyCriteriaGroupArchivedError if learner_progress else CompetencyCriteriaGroup.DoesNotExist + with pytest.raises(expected): + patch.result() + historical_group = apps.get_model("openedx_learning", "HistoricalCompetencyCriteriaGroup") + assert not historical_group.objects.filter(id=leaf.id, logic_operator=LogicOperator.AND).exists() diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index 4e3a242b3..a198a7248 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -437,6 +437,38 @@ def test_hard_delete_leaves_a_tag_of_another_competency_on_the_same_object( assert CompetencyCriterion.objects.filter(id=other_criterion.id).exists() +def test_delete_re_reads_the_criterion_under_the_tag_lock( + monkeypatch: pytest.MonkeyPatch, + user, + tag: Tag, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A criterion removed while the delete waits for the Tag lock raises DoesNotExist, so the delete + reads the criterion again after the lock rather than trusting its first read. + """ + criterion = _make_criterion(CompetencyCriteriaGroup.objects.create(tag=tag), object_tag, default_rule_profile) + original_select_for_update = Tag.objects.select_for_update + + def _select_for_update(*args, **kwargs): + queryset = original_select_for_update(*args, **kwargs) + original_get = queryset.get + + def _get(*get_args, **get_kwargs): + # Stands in for a concurrent delete that commits while this one waits for the lock. + CompetencyCriterion.objects.filter(pk=criterion.pk).delete() + return original_get(*get_args, **get_kwargs) + + queryset.get = _get + return queryset + + monkeypatch.setattr(Tag.objects, "select_for_update", _select_for_update) + + with pytest.raises(CompetencyCriterion.DoesNotExist): + delete_or_archive_competency_criterion(criterion.id, user) + + # ================================================================================================= # Not found # ================================================================================================= From 8d66a52611a44cd6328c0cb365cf9bfcbbbff756 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 19:01:25 +0300 Subject: [PATCH 14/24] test: cover criterion delete races over two MySQL connections (#674) Add four two-connection tests for the criterion delete path. Each pauses the delete inside its transaction and acts from a second connection. - The Tag lock stays held from the delete's first write until it commits, for both hard delete and archive. - A create for the same competency waits for the delete, then builds its own leaf, course-level group and root while the original three are gone. - Two deletes of sibling criteria in one leaf leave no empty leaf, so the leaf, course-level group and root are all removed. - A grade write and a hard delete fail one side with IntegrityError in either order: the delete when the grade holds the criterion first (a second delete then archives it), the grade when the delete does. The file skips wherever select_for_update is not real, so it runs on MySQL and skips on SQLite. Co-Authored-By: Claude Fable 5 --- .../applets/cbe/test_criterion_concurrency.py | 206 +++++++++++++++++- 1 file changed, 205 insertions(+), 1 deletion(-) diff --git a/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py b/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py index 36d4596d2..6f05bab3c 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py @@ -8,12 +8,13 @@ import threading from collections.abc import Callable, Iterator from datetime import datetime, timezone +from decimal import Decimal from typing import Any import pytest import rules from django.apps import apps -from django.db import connection +from django.db import DatabaseError, IntegrityError, connection, transaction from django.db.utils import OperationalError from rules.permissions import permissions as rule_permissions @@ -21,8 +22,11 @@ from openedx_learning.api import ( CompetencyCriteriaGroupArchivedError, CompetencyCriterionArchivedError, + GradedObjectScore, + associate_competency_criterion, bulk_update_competency_criteria, delete_or_archive_competency_criterion, + record_graded_object_statuses, update_competency_criteria_group, ) from openedx_learning.applets.cbe import api as cbe_api @@ -163,6 +167,28 @@ def pause_after_the_walk(monkeypatch: pytest.MonkeyPatch) -> PausePoint: return pause +def tag_row_is_locked(tag: Tag) -> bool: + """ + Return whether another connection holds the lock on ``tag``'s row. + + ``nowait`` makes MySQL fail at once instead of waiting, so the answer involves no timing. + """ + try: + with transaction.atomic(): + Tag.objects.select_for_update(nowait=True).get(pk=tag.pk) + except DatabaseError: + return True + return False + + +@pytest.fixture(name="sibling_object_id") +def _sibling_object_id(course_run: CourseRun) -> str: + """The usage key of a second sequential in the same course as the criterion in ``leaf_and_criterion``.""" + key = course_run.course_key + assert key is not None + return f"block-v1:{key.org}+{key.course}+{key.run}+type@sequential+block@s2" + + @pytest.fixture(name="learner_progress", params=[False, True], ids=["hard delete", "archive"]) def _learner_progress(request: pytest.FixtureRequest) -> bool: """Whether the criterion has a learner status row, which makes a delete archive it instead of removing it.""" @@ -261,3 +287,181 @@ def test_patch_does_not_write_onto_an_archived_or_deleted_group( patch.result() historical_group = apps.get_model("openedx_learning", "HistoricalCompetencyCriteriaGroup") assert not historical_group.objects.filter(id=leaf.id, logic_operator=LogicOperator.AND).exists() + + +def test_tag_lock_is_held_for_the_whole_delete_request( + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + start: Callable[..., Worker], + tag: Tag, + user, +) -> None: + """The competency's Tag row stays locked from the delete's first write until its transaction commits.""" + _, criterion = leaf_and_criterion + pause = pause_before_the_walk(monkeypatch) + try: + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + pause.wait_until_reached() + assert tag_row_is_locked(tag) + finally: + pause.release.set() + + delete.result() + assert not tag_row_is_locked(tag) + + +@pytest.mark.parametrize("learner_progress", [False], ids=["hard delete"]) +def test_create_for_the_same_competency_waits_and_its_criterion_survives( + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + sibling_object_id: str, + start: Callable[..., Worker], + user, +) -> None: + """ + A create for the competency whose only criterion is being deleted waits, then builds a fresh tree. + + The delete removes the leaf, course-level group and root. The create, which would otherwise reuse + those containers while the delete is removing them, runs afterwards and makes its own. + """ + leaf, criterion = leaf_and_criterion + course_level_group_id = leaf.parent_id + assert course_level_group_id is not None + root_id = CompetencyCriteriaGroup.objects.get(pk=course_level_group_id).parent_id + pause = pause_before_the_walk(monkeypatch) + try: + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + pause.wait_until_reached() + create = start(associate_competency_criterion, criterion.object_tag.tag_id, sibling_object_id) + assert create.is_blocked() + finally: + pause.release.set() + + delete.result() + new_criterion = create.result() + new_leaf = new_criterion.group + assert new_leaf.id != leaf.id + new_course_level_group = new_leaf.parent + assert new_course_level_group is not None + assert new_course_level_group.id != course_level_group_id + new_root = new_course_level_group.parent + assert new_root is not None + assert new_root.id != root_id + assert new_root.parent_id is None + assert CompetencyCriterion.objects.filter(pk=new_criterion.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, course_level_group_id, root_id]).exists() + + +@pytest.mark.parametrize("learner_progress", [False], ids=["hard delete"]) +def test_two_deletes_of_sibling_criteria_leave_no_empty_leaf( + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + sibling_object_id: str, + start: Callable[..., Worker], + user, +) -> None: + """ + Two criteria in one leaf, deleted at the same time, leave neither the leaf nor anything above it. + + Without the Tag lock each delete would still see the other's criterion in the leaf and stop the walk + there, leaving an empty leaf behind. + """ + leaf, first = leaf_and_criterion + course_level_group_id = leaf.parent_id + assert course_level_group_id is not None + root_id = CompetencyCriteriaGroup.objects.get(pk=course_level_group_id).parent_id + object_tag = ObjectTag.objects.create( + object_id=sibling_object_id, taxonomy_id=first.object_tag.taxonomy_id, tag_id=first.object_tag.tag_id, + ) + second = CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, rule_profile=first.rule_profile) + pause = pause_before_the_walk(monkeypatch) + try: + first_delete = start(delete_or_archive_competency_criterion, first.id, user) + pause.wait_until_reached() + second_delete = start(delete_or_archive_competency_criterion, second.id, user) + assert second_delete.is_blocked() + finally: + pause.release.set() + + assert first_delete.result().archived is False + assert second_delete.result().archived is False + assert not CompetencyCriterion.objects.filter(pk__in=[first.id, second.id]).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, course_level_group_id, root_id]).exists() + + +def _grade_then_wait( + user_id: int, object_id: str, reached: threading.Event, release: threading.Event, +) -> None: + """Record a passing grade for ``object_id`` and hold the grade's transaction open until ``release`` is set.""" + with transaction.atomic(): + record_graded_object_statuses( + user_id=user_id, scores=[GradedObjectScore(object_id=object_id, fraction=Decimal("0.9"))], + ) + reached.set() + if not release.wait(WAIT_SECONDS): + raise RuntimeError("the grade transaction was never released") + + +@pytest.mark.parametrize("learner_progress", [False], ids=["hard delete"]) +def test_grade_write_and_hard_delete_fail_one_side_never_silently_grade_first( + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + start: Callable[..., Worker], + user, +) -> None: + """ + A hard delete that races a grade write already holding the criterion fails with ``IntegrityError``. + + The grade's status row holds a lock on the criterion, so the delete waits for the grade to commit and + then fails the foreign-key check. Nothing is lost: both rows survive, and a second delete archives the + criterion. + """ + _, criterion = leaf_and_criterion + grade_reached, grade_release = threading.Event(), threading.Event() + try: + grade = start(_grade_then_wait, user.id, criterion.object_tag.object_id, grade_reached, grade_release) + assert grade_reached.wait(WAIT_SECONDS), "the grade write never recorded its status" + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + assert delete.is_blocked() + finally: + grade_release.set() + + grade.result() + with pytest.raises(IntegrityError): + delete.result() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + assert StudentCompetencyCriterionStatus.objects.filter(criterion=criterion, user=user).exists() + assert delete_or_archive_competency_criterion(criterion.id, user).archived is True + + +@pytest.mark.parametrize("learner_progress", [False], ids=["hard delete"]) +def test_grade_write_and_hard_delete_fail_one_side_never_silently_delete_first( + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + start: Callable[..., Worker], + user, +) -> None: + """ + A grade write that races a hard delete already holding the criterion fails with ``IntegrityError``. + + The grade's status insert waits for the delete to commit and then fails the foreign-key check. The + criterion is gone and no status row was written. + """ + _, criterion = leaf_and_criterion + pause = pause_before_the_walk(monkeypatch) + grade_reached, grade_release = threading.Event(), threading.Event() + # Already released: the grade's transaction ends as soon as its insert returns, so only the delete holds locks. + grade_release.set() + try: + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + pause.wait_until_reached() + grade = start(_grade_then_wait, user.id, criterion.object_tag.object_id, grade_reached, grade_release) + assert grade.is_blocked() + finally: + pause.release.set() + + assert delete.result().archived is False + with pytest.raises(IntegrityError): + grade.result() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + assert not StudentCompetencyCriterionStatus.objects.filter(user=user).exists() From a44fb5b25a42ca0e01fbd0797b07d3cf33f76a9b Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 19:07:40 +0300 Subject: [PATCH 15/24] fix: record the acting user on deletion history rows (#674) A hard delete wrote its simple_history deletion rows with no history_user, because the criterion and each cascaded group are deleted without the user set on the instance. Shell, task, and plugin callers bypass simple_history's request middleware, so the row stayed unattributed. Set the user on the criterion and on both group deletes before they run, as the archive path already does. Also read the group and object tag with the criterion under the Tag lock, which saves two queries. Co-Authored-By: Claude Fable 5 --- src/openedx_learning/applets/cbe/api.py | 5 ++++- .../applets/cbe/test_criterion_removal.py | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 8c556b979..9a4af90ad 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -965,6 +965,7 @@ def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", return if _group_has_learner_progress(group.id): return + group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access group.delete() if parent is not None: _cascade_from_group(parent, "delete", user) @@ -979,6 +980,7 @@ def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", _archive_group(group, user) next_event = "archive" else: + group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access group.delete() next_event = "delete" else: @@ -1050,12 +1052,13 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> with transaction.atomic(): Tag.objects.select_for_update().get(pk=tag_id) # Re-read under the Tag lock: a concurrent delete may have won; DoesNotExist propagates to the caller. - criterion = CompetencyCriterion.objects.get(pk=criterion_id) + criterion = CompetencyCriterion.objects.select_related("group", "object_tag").get(pk=criterion_id) if _criterion_has_learner_progress(criterion_id): _archive_criterion(criterion, user) return CriterionDeletionResult(id=criterion_id, archived=True) group = criterion.group object_tag = criterion.object_tag + criterion._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access criterion.delete() # Only this criterion's own ObjectTag is a candidate, and only while no other criterion uses it. if not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists(): diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index a198a7248..6ce8d3453 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -159,6 +159,21 @@ def test_archive_records_the_acting_user_in_history( assert _latest_history_user("HistoricalCompetencyCriteriaGroup", root.id) == user +def test_hard_delete_records_the_acting_user_in_history( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """Hard-deleting a criterion records the acting user on the deletion row of it and of each cascaded group.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + + delete_or_archive_competency_criterion(criterion.id, user) + + assert _latest_history_user("HistoricalCompetencyCriterion", criterion.id) == user + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", leaf.id) == user + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", root.id) == user + + def test_shared_object_tag_is_untouched_while_another_criterion_still_references_it( user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, ) -> None: From d0a8f77cec002c09a9be5473348dd5b632e927c5 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 19:07:58 +0300 Subject: [PATCH 16/24] refactor: drop the cascade's event parameter, fix lock comments (#674) The two exists() checks in _cascade_from_group decide every branch: an archive call always has the archived criterion still in the group, so the checks stop the walk without the event. Passing "delete" at every level behaves the same as threading the parameter through. Forcing event to "archive" inside the function fails six tests, all of them delete cases, which confirms the checks and the delete event are the only inputs that matter. The function now takes the group and the user. The lock comments name every writer that depends on the Tag lock: the create path notes that the delete relies on it, the bulk update comment stays true of create's later course-run lock, and the group PATCH and delete docstrings name Tag.DoesNotExist. The delete docstring states what the lock prevents, the isolation level the code relies on, who waits on it, and the two outcomes of saving a grade during a hard delete. The history-attribution comment appears once in full, and the "always a competency tag" comments give the real reason. Co-Authored-By: Claude Fable 5 --- src/openedx_learning/applets/cbe/api.py | 102 +++++++++++------- .../applets/cbe/rest_api/v1/views.py | 4 +- 2 files changed, 63 insertions(+), 43 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 9a4af90ad..eb5462611 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -7,7 +7,7 @@ from dataclasses import dataclass from datetime import datetime, timezone from decimal import Decimal -from typing import Any, Iterable, Literal, Sequence, cast +from typing import Any, Iterable, Sequence, cast from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError from django.db import connection, transaction @@ -294,7 +294,8 @@ def _create_leaf_group( The root and course-level lookups need no lock of their own: the unique constraints hold one root per tag and one course-level group per tag and course, and ``get_or_create()`` recovers - from a concurrent insert. The caller's tag lock is for ``_reject_duplicate_criterion()``. + from a concurrent insert. The caller's tag lock is for ``_reject_duplicate_criterion()``. The delete path + relies on the same lock to keep this function's inserts out of its emptiness-check window. """ name_max_length = CompetencyCriteriaGroup._meta.get_field("name").max_length # Both get_or_create() lookups match archived rows deliberately, so an archived container is @@ -574,7 +575,8 @@ def associate_competency_criterion( # The database enforces one root per tag; the tag lock only serializes _reject_duplicate_criterion(). That # check is check-then-act on both paths: without group_id a request always creates a fresh leaf, and with # group_id CompetencyCriterion has no unique constraint on (object_tag, group), so no index catches a - # concurrent copy. + # concurrent copy. delete_or_archive_competency_criterion() depends on this lock too: it keeps this + # function's inserts out of the delete's emptiness-check window. Tag.objects.select_for_update().get(pk=tag.pk) # Serializes the per-course dominance check on CBE's own row, not the catalog's CourseRun. # Created first, since a locking read of a row that doesn't exist yet locks nothing; get_or_create() @@ -711,7 +713,7 @@ def bulk_update_competency_criteria( # Criteria hang only off leaf groups, and a leaf's course is on its course-level parent. if group.parent is None or group.course_id is not None or group.parent.course is None: raise ValidationError({"group_id": _("group_id must reference a leaf CompetencyCriteriaGroup.")}) - # A group's tag is always a competency tag, so its taxonomy is always set. + # The create path only accepts competency tags, and a group's tag never changes, so its taxonomy is set. assert group.tag.taxonomy is not None perm_obj = ObjectTagPermissionItem(taxonomy=group.tag.taxonomy, object_id=str(group.parent.course.course_key)) if not user.has_perm("oel_tagging.can_tag_object", perm_obj): @@ -723,8 +725,8 @@ def bulk_update_competency_criteria( raise ValidationError({"criterion_ids": _("criterion_ids must not name the same criterion twice.")}) _validate_rule_source(rule_profile_id, rule_type_override, rule_payload_override) - # Every writer of a competency's criteria serializes on its Tag row, and none takes another row lock, - # so the writers can't deadlock. Everything below reads committed state, since a delete, an archive + # Every writer of a competency's criteria takes its Tag row lock first, and none takes another row lock + # before it, so the writers can't deadlock. Everything below reads committed state, since a delete, an archive # or a create for this competency can't commit while this transaction holds the lock. The reads # above are safe without it because a group's tag, parent and course never change. Tag.objects.select_for_update().get(pk=group.tag_id) @@ -878,7 +880,7 @@ def update_competency_criteria_group( only for a real change. Learner status is never read or written. Raises CompetencyCriteriaGroup.DoesNotExist if no group has ``group_id``, or Tag.DoesNotExist - if its tag isn't on a CompetencyTaxonomy. + if its tag isn't on a CompetencyTaxonomy or was deleted while the request ran. Raises ValidationError, keyed by the REST field names, for a root group, a group with no course anywhere above it to check permission against, or a ``logic_operator`` other than AND or OR, and PermissionDenied if ``user`` fails ``oel_tagging.can_tag_object`` for the group's taxonomy and @@ -931,7 +933,7 @@ def update_competency_criteria_group( return group group.logic_operator = logic_operator - # Set on the instance so attribution doesn't depend on simple_history's request middleware. + # Set for history attribution; see _apply_rule_source. group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access # Never QuerySet.update(): it skips post_save, so simple_history would record nothing. group.save(update_fields=["logic_operator"]) @@ -941,54 +943,52 @@ def update_competency_criteria_group( def _archive_group(group: CompetencyCriteriaGroup, user: UserType) -> None: """Archive ``group``, writing only the flag and recording ``user`` in its history.""" group.archived = True - # Set on the instance so attribution doesn't depend on simple_history's request middleware. + # Set for history attribution; see _apply_rule_source. group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access group.save(update_fields=["archived"]) -def _cascade_from_group(group: CompetencyCriteriaGroup, event: Literal["delete", "archive"], user: UserType) -> None: +def _cascade_from_group(group: CompetencyCriteriaGroup, user: UserType) -> None: """ - Walk `group` and its ancestors, collapsing/archiving any left with no live children. + Walk ``group`` and its ancestors, collapsing/archiving any left with no live children. - Checking both relations (`criteria` and `child_groups`) at every level is harmless: the one that + Checking both relations (``criteria`` and ``child_groups``) at every level is harmless: the one that does not apply is an empty queryset, and it avoids a separate leaf/non-leaf code path. An archived group is never written again: everything above it was handled when it was archived, - so a second write would only add a redundant history row. A delete event can still remove it. If - the delete left it with no criteria, no child groups and no group-level status, it is deleted and - the walk continues upward, so no empty group stays persisted. Otherwise the walk stops there. + so a second write would only add a redundant history row. If it has no criteria, no child groups + and no group-level status, it is deleted and the walk continues upward, so no empty group stays + persisted. Otherwise the walk stops there. """ parent = group.parent - next_event: Literal["delete", "archive"] if group.archived: - if event == "archive" or group.criteria.exists() or group.child_groups.exists(): + if group.criteria.exists() or group.child_groups.exists(): return if _group_has_learner_progress(group.id): return + # Set for history attribution; see _apply_rule_source. group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access group.delete() if parent is not None: - _cascade_from_group(parent, "delete", user) + _cascade_from_group(parent, user) return has_live_child = ( group.criteria.filter(archived=False).exists() or group.child_groups.filter(archived=False).exists() ) if has_live_child: return - if event == "delete" and not group.criteria.exists() and not group.child_groups.exists(): + if not group.criteria.exists() and not group.child_groups.exists(): if _group_has_learner_progress(group.id): _archive_group(group, user) - next_event = "archive" else: + # Set for history attribution; see _apply_rule_source. group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access group.delete() - next_event = "delete" else: # Only archived children remain. _archive_group(group, user) - next_event = "archive" if parent is not None: - _cascade_from_group(parent, next_event, user) + _cascade_from_group(parent, user) def _archive_criterion(criterion: CompetencyCriterion, user: UserType) -> None: @@ -1000,10 +1000,10 @@ def _archive_criterion(criterion: CompetencyCriterion, user: UserType) -> None: if criterion.archived: return criterion.archived = True - # Set on the instance so attribution doesn't depend on simple_history's request middleware. + # Set for history attribution; see _apply_rule_source. criterion._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access criterion.save(update_fields=["archived"]) - _cascade_from_group(criterion.group, "archive", user) + _cascade_from_group(criterion.group, user) def _criterion_has_learner_progress(criterion_id: int) -> bool: @@ -1018,34 +1018,53 @@ def _group_has_learner_progress(group_id: int) -> bool: def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> CriterionDeletionResult: """ - Hard-delete `criterion_id`, or archive it if a learner status row already references it. + Hard-delete ``criterion_id``, or archive it if a learner status row already references it. UNSTABLE: the criteria-authoring family is incomplete, so its validation and group update and archive behavior may change this function's shape without a deprecation cycle. - ``user`` must hold ``oel_tagging.can_tag_object`` for the criterion's object, as - :func:`bulk_update_competency_criteria` requires; otherwise Django's ``PermissionDenied`` is - raised before anything is locked or changed. - - An ancestor :class:`CompetencyCriteriaGroup` left with no live children by this call is - cascaded in turn: hard-deleted if it carries no group-level status of its own, archived - otherwise. See ADR-0002 Decision 2. + ``user`` must hold ``oel_tagging.can_tag_object`` for the taxonomy of the criterion's group and for + the usage key its ObjectTag tags. The check runs before anything is locked or changed. - Raises CompetencyCriterion.DoesNotExist if no such criterion exists (including one deleted - concurrently) and PermissionDenied if ``user`` may not tag the criterion's object. + An ancestor :class:`CompetencyCriteriaGroup` left with no live children by this call is cascaded in + turn: hard-deleted if it has no children of any kind and no group-level status of its own, archived + otherwise, including when only archived children remain. See ADR-0002 Decision 2. On a hard delete, the criterion's own ObjectTag is deleted too, and only when no other criterion references it. No other ObjectTag on the object is touched. - The whole operation runs in one transaction that locks only the competency's Tag, the same lock - a create for that competency takes first, so a concurrent create waits until this call commits. + Raises ``CompetencyCriterion.DoesNotExist`` if no such criterion exists (including one deleted + concurrently), ``Tag.DoesNotExist`` if its competency tag was deleted concurrently, and Django's + ``PermissionDenied`` if ``user`` fails the permission check. + + The first read and the permission check run before the transaction. The rest runs in one + transaction that locks only the competency's Tag row, the lock every writer of its criteria takes + first, so a concurrent create waits until this call commits. The lock prevents three races that + follow from Django's Python-side cascade. A create that commits a new leaf and criterion between the + walk's emptiness check and the collector's SELECT inside ``group.delete()`` would have those rows + deleted silently, after its caller received a 201. Two deletes of sibling criteria in one leaf would + each still see the other's criterion and leave an empty leaf behind. A create that reuses the same + ObjectTag and commits between the "still referenced" check and the ObjectTag delete would have its + criterion cascaded away. The lock is held until the request commits, so other authoring writers for + this competency, and inserts that reference its Tag row, wait meanwhile. + + The code relies on the READ COMMITTED isolation level: the re-read after the lock and the walk's + ``exists()`` checks must see concurrent commits. Under REPEATABLE READ the re-read would see the + snapshot from before the lock, so a racing duplicate delete would return success and write a second + deletion history row, and two sibling deletes could leave an empty group, all without an error. + + A learner's grade saved at the same moment as a hard delete fails one side with an error. The delete + checks for a status row and then deletes the criterion, and a grade write that inserts a status row + in between is not waited for. If the status INSERT comes first, this delete waits for the grade + transaction and then fails the foreign-key check, and a second delete archives the criterion. If the + DELETE comes first, the grade's INSERT waits for it to commit and then fails inside the learner's + grade transaction. """ criterion = CompetencyCriterion.objects.select_related("group__tag__taxonomy", "object_tag").get(pk=criterion_id) - # A group's tag is always a competency tag, so its taxonomy is always set. + # The create path only accepts competency tags, and a group's tag never changes, so its taxonomy is set. assert criterion.group.tag.taxonomy is not None perm_obj = ObjectTagPermissionItem(taxonomy=criterion.group.tag.taxonomy, object_id=criterion.object_tag.object_id) - # The rules-based permission takes an ObjectTagPermissionItem, but django-stubs types the - # has_perm object argument as a Model. + # has_perm()'s stub expects a Model, but a rules predicate takes any object. if not user.has_perm("oel_tagging.can_tag_object", cast(Any, perm_obj)): raise PermissionDenied() tag_id = criterion.group.tag_id @@ -1058,10 +1077,11 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> return CriterionDeletionResult(id=criterion_id, archived=True) group = criterion.group object_tag = criterion.object_tag + # Set for history attribution; see _apply_rule_source. criterion._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access criterion.delete() # Only this criterion's own ObjectTag is a candidate, and only while no other criterion uses it. if not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists(): ObjectTag.objects.filter(pk=object_tag.id).delete() - _cascade_from_group(group, "delete", user) + _cascade_from_group(group, user) return CriterionDeletionResult(id=criterion_id, archived=False) diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/views.py b/src/openedx_learning/applets/cbe/rest_api/v1/views.py index feedd7f0b..f23959b81 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -293,8 +293,8 @@ def delete(self, request, *args, **kwargs): Resolve the criterion scoped to both path segments, then delegate to the public API, which checks permission. The URL is scoped by tag_id, not group_id, because its caller (#710) sends exactly - ``DELETE .../competencies//criteria//`` and has no group id. The - lookup deliberately does not filter `archived=False`, so a repeat + ``DELETE .../competencies//criteria//`` and has no group id. + The lookup deliberately does not filter `archived=False`, so a repeat DELETE against an already-archived row still resolves (200), and a mismatched tag_id or a nonexistent criterion_id both 404 the same way, via this one get_object_or_404 call. """ From cfa1710a3d80d7bfd459be1e851599eb3d64b059 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 19:17:14 +0300 Subject: [PATCH 17/24] test: assert real behavior instead of stubbed tagging reads (#674) The monkeypatched tagging read no longer exists in the delete path, so the two tests that depended on it or on a disabled taxonomy are one test that asserts the criterion's own ObjectTag is removed and another competency's ObjectTag on the same object survives. The 400 test for DELETE now archives a criterion whose stored payload went invalid, written past validation with update(), instead of mocking a ValidationError the endpoint cannot produce. The two 404 tests are one parametrized test, and the view tests keep at most one row-state assertion each. New tests cover a competency Tag deleted while a delete or bulk update waits for the lock, the primary-key save order of bulk update, and the history user on deleted archived groups. The tag-lock helper moves to the cbe conftest as a fixture shared by its three callers. Co-Authored-By: Claude Fable 5 --- .../openedx_learning/applets/cbe/conftest.py | 33 +++++ .../openedx_learning/applets/cbe/test_api.py | 66 +++++----- .../applets/cbe/test_criterion_removal.py | 115 +++++------------- .../applets/cbe/test_views.py | 83 ++++++++----- 4 files changed, 152 insertions(+), 145 deletions(-) diff --git a/tests/openedx_learning/applets/cbe/conftest.py b/tests/openedx_learning/applets/cbe/conftest.py index 90f222927..956ed31c9 100644 --- a/tests/openedx_learning/applets/cbe/conftest.py +++ b/tests/openedx_learning/applets/cbe/conftest.py @@ -1,4 +1,5 @@ """Shared fixtures for the CBE test modules: schema, deletion, tree-integration, status, and REST tests.""" +from collections.abc import Callable from datetime import datetime, timezone import pytest @@ -126,3 +127,35 @@ def _staff_client(staff_user) -> APIClient: client = APIClient() client.force_authenticate(user=staff_user) return client + + +@pytest.fixture(name="run_when_tag_lock_is_taken") +def _run_when_tag_lock_is_taken(monkeypatch: pytest.MonkeyPatch) -> Callable[[Callable[[], None]], None]: + """ + Return a function that makes the API run a callable as it takes the Tag row lock. + + The callable runs once, before the API reads the Tag. It stands in for a transaction that commits + while the API waits for the lock, so a read the API makes after taking the lock sees the change and + a read it made earlier doesn't. + """ + + def _install(side_effect: Callable[[], None]) -> None: + original_select_for_update = Tag.objects.select_for_update + fired: list[bool] = [] + + def _select_for_update(*args, **kwargs): + queryset = original_select_for_update(*args, **kwargs) + original_get = queryset.get + + def _get(*get_args, **get_kwargs): + if not fired: + fired.append(True) + side_effect() + return original_get(*get_args, **get_kwargs) + + queryset.get = _get + return queryset + + monkeypatch.setattr(Tag.objects, "select_for_update", _select_for_update) + + return _install diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index 18fe627e0..c27a95d88 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -2,7 +2,7 @@ Tests for the CBE public API surface (openedx_learning.api). """ import re -from collections.abc import Callable, Iterator +from collections.abc import Iterator from typing import Any import pytest @@ -632,32 +632,6 @@ def snapshot(criteria) -> dict[int, tuple]: return {criterion.id: (rule_state(criterion), history_of(criterion).count()) for criterion in criteria} -def run_when_tag_lock_is_taken(monkeypatch: pytest.MonkeyPatch, side_effect: Callable[[], None]) -> None: - """ - Run ``side_effect`` once, as the API takes the Tag row lock and before it reads the Tag. - - It stands in for a transaction that commits while the API waits for the lock, so a read the API - makes after taking the lock sees the change and a read it made earlier doesn't. - """ - original_select_for_update = Tag.objects.select_for_update - fired: list[bool] = [] - - def _select_for_update(*args, **kwargs): - queryset = original_select_for_update(*args, **kwargs) - original_get = queryset.get - - def _get(*get_args, **get_kwargs): - if not fired: - fired.append(True) - side_effect() - return original_get(*get_args, **get_kwargs) - - queryset.get = _get - return queryset - - monkeypatch.setattr(Tag.objects, "select_for_update", _select_for_update) - - def test_bulk_update_gives_every_criterion_the_rule_values_whatever_its_starting_source( batch: tuple[CompetencyCriterion, CompetencyCriterion], leaf: CompetencyCriteriaGroup, user, ) -> None: @@ -1067,16 +1041,40 @@ def _fail_on_second_save(self, *args, **kwargs) -> None: assert snapshot(batch) == before -def test_bulk_update_checks_archived_state_under_the_tag_lock( +def test_bulk_update_saves_in_primary_key_order_and_returns_request_order( monkeypatch: pytest.MonkeyPatch, batch: tuple[CompetencyCriterion, CompetencyCriterion], leaf: CompetencyCriteriaGroup, + default_rule_profile: CompetencyRuleProfile, + user, +) -> None: + """Each save takes a row lock, so the saves run in ascending pk order however the ids were requested.""" + c1, c2 = batch + c3 = make_criterion(leaf, "p3", rule_profile=default_rule_profile) + original_save = CompetencyCriterion.save + saves: list[int] = [] + + def _record_save(self, *args, **kwargs) -> None: + saves.append(self.pk) + original_save(self, *args, **kwargs) + + monkeypatch.setattr(CompetencyCriterion, "save", _record_save) + + result = bulk_update_competency_criteria([c3.id, c1.id, c2.id], leaf.id, user=user, **NEW_RULE) + + assert saves == sorted(saves) + assert set(saves) == {c1.id, c2.id, c3.id} + assert [criterion.id for criterion in result] == [c3.id, c1.id, c2.id] + + +def test_bulk_update_checks_archived_state_under_the_tag_lock( + run_when_tag_lock_is_taken, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, user, ) -> None: """A group archived while the update waits for the Tag lock refuses the batch, so the check reads after the lock.""" - run_when_tag_lock_is_taken( - monkeypatch, lambda: CompetencyCriteriaGroup.objects.filter(pk=leaf.pk).update(archived=True), - ) + run_when_tag_lock_is_taken(lambda: CompetencyCriteriaGroup.objects.filter(pk=leaf.pk).update(archived=True)) before = snapshot(batch) with pytest.raises(CompetencyCriterionArchivedError, match=f"group {leaf.id}"): @@ -2282,12 +2280,10 @@ def test_update_raises_does_not_exist_for_an_unknown_group(user) -> None: def test_group_update_re_reads_the_group_under_the_tag_lock( - monkeypatch: pytest.MonkeyPatch, leaf: CompetencyCriteriaGroup, user, + run_when_tag_lock_is_taken, leaf: CompetencyCriteriaGroup, user, ) -> None: """A group archived while the update waits for the Tag lock is refused, not written onto.""" - run_when_tag_lock_is_taken( - monkeypatch, lambda: CompetencyCriteriaGroup.objects.filter(pk=leaf.pk).update(archived=True), - ) + run_when_tag_lock_is_taken(lambda: CompetencyCriteriaGroup.objects.filter(pk=leaf.pk).update(archived=True)) with pytest.raises(CompetencyCriteriaGroupArchivedError): update_competency_criteria_group(leaf.id, logic_operator=LogicOperator.AND, user=user) diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index 6ce8d3453..e203ce917 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -24,7 +24,6 @@ pytestmark = pytest.mark.django_db CHANGE_OBJECTTAG_OBJECTID = "oel_tagging.change_objecttag_objectid" -CHANGE_OBJECTTAG_TAXONOMY = "oel_tagging.change_objecttag_taxonomy" @pytest.fixture(autouse=True) @@ -194,6 +193,34 @@ def test_shared_object_tag_is_untouched_while_another_criterion_still_references assert ObjectTag.objects.filter(id=object_tag.id).exists() +def test_hard_delete_removes_only_the_criterions_own_object_tag( + user, + competency_taxonomy: CompetencyTaxonomy, + tag: Tag, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A hard delete removes the criterion's own ObjectTag, now referenced by nothing. An ObjectTag for + another competency on the same object stays with its criterion, so the delete picks the ObjectTag + by its primary key and not by the object it is on. + """ + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Reading Poetry") + other_object_tag = _make_object_tag(competency_taxonomy, other_tag, object_tag.object_id) + criterion = _make_criterion(CompetencyCriteriaGroup.objects.create(tag=tag), object_tag, default_rule_profile) + other_criterion = _make_criterion( + CompetencyCriteriaGroup.objects.create(tag=other_tag), other_object_tag, default_rule_profile + ) + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.archived is False + assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() + assert not ObjectTag.objects.filter(id=object_tag.id).exists() + assert CompetencyCriterion.objects.filter(id=other_criterion.id).exists() + assert ObjectTag.objects.filter(id=other_object_tag.id).exists() + + # ================================================================================================= # The ancestor cascade, on hard-delete # ================================================================================================= @@ -387,73 +414,8 @@ def test_a_user_without_permission_is_refused_and_nothing_changes( assert ObjectTag.objects.filter(id=object_tag.id).exists() -def test_untag_with_the_competency_taxonomy_disabled_removes_only_the_orphaned_object_tag( - user, - competency_taxonomy: CompetencyTaxonomy, - tag: Tag, - object_tag: ObjectTag, - default_rule_profile: CompetencyRuleProfile, -) -> None: - """ - With the taxonomy disabled the tagging API hides its tags, but the delete removes only the - criterion's own ObjectTag, so it succeeds and the object's other ObjectTag and its criterion - stay. The taxonomy-level permission rule denies every user on a disabled taxonomy, so the test - substitutes a permissive one to reach the untag step. - """ - other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Reading Poetry") - other_object_tag = _make_object_tag(competency_taxonomy, other_tag, object_tag.object_id) - criterion = _make_criterion(CompetencyCriteriaGroup.objects.create(tag=tag), object_tag, default_rule_profile) - other_criterion = _make_criterion( - CompetencyCriteriaGroup.objects.create(tag=other_tag), other_object_tag, default_rule_profile - ) - competency_taxonomy.enabled = False - competency_taxonomy.save() - original = rule_permissions[CHANGE_OBJECTTAG_TAXONOMY] - rules.set_perm(CHANGE_OBJECTTAG_TAXONOMY, lambda _user, _taxonomy: True) - - try: - result = delete_or_archive_competency_criterion(criterion.id, user) - finally: - rules.set_perm(CHANGE_OBJECTTAG_TAXONOMY, original) - - assert result.archived is False - assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() - assert not ObjectTag.objects.filter(id=object_tag.id).exists() - assert CompetencyCriterion.objects.filter(id=other_criterion.id).exists() - assert ObjectTag.objects.filter(id=other_object_tag.id).exists() - - -def test_hard_delete_leaves_a_tag_of_another_competency_on_the_same_object( - user, - monkeypatch: pytest.MonkeyPatch, - tag: Tag, - object_tag: ObjectTag, - default_rule_profile: CompetencyRuleProfile, -) -> None: - """ - An ObjectTag for another competency tag on the same object, committed after a tag list was - read, survives the delete together with its criterion. The tag list is stubbed to omit it, as - a read taken before that commit would; the delete must not depend on reading it. - """ - competency_taxonomy = CompetencyTaxonomy.objects.get() - other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Reading Poetry") - other_object_tag = _make_object_tag(competency_taxonomy, other_tag, object_tag.object_id) - criterion = _make_criterion(CompetencyCriteriaGroup.objects.create(tag=tag), object_tag, default_rule_profile) - other_criterion = _make_criterion( - CompetencyCriteriaGroup.objects.create(tag=other_tag), other_object_tag, default_rule_profile - ) - stale_list = [ObjectTag.objects.get(pk=object_tag.pk)] - monkeypatch.setattr("openedx_learning.applets.cbe.api.get_object_tags", lambda *_args, **_kwargs: stale_list) - - delete_or_archive_competency_criterion(criterion.id, user) - - assert not ObjectTag.objects.filter(id=object_tag.id).exists() - assert ObjectTag.objects.filter(id=other_object_tag.id).exists() - assert CompetencyCriterion.objects.filter(id=other_criterion.id).exists() - - def test_delete_re_reads_the_criterion_under_the_tag_lock( - monkeypatch: pytest.MonkeyPatch, + run_when_tag_lock_is_taken, user, tag: Tag, object_tag: ObjectTag, @@ -464,21 +426,8 @@ def test_delete_re_reads_the_criterion_under_the_tag_lock( reads the criterion again after the lock rather than trusting its first read. """ criterion = _make_criterion(CompetencyCriteriaGroup.objects.create(tag=tag), object_tag, default_rule_profile) - original_select_for_update = Tag.objects.select_for_update - - def _select_for_update(*args, **kwargs): - queryset = original_select_for_update(*args, **kwargs) - original_get = queryset.get - - def _get(*get_args, **get_kwargs): - # Stands in for a concurrent delete that commits while this one waits for the lock. - CompetencyCriterion.objects.filter(pk=criterion.pk).delete() - return original_get(*get_args, **get_kwargs) - - queryset.get = _get - return queryset - - monkeypatch.setattr(Tag.objects, "select_for_update", _select_for_update) + # Stands in for a concurrent delete that commits while this one waits for the lock. + run_when_tag_lock_is_taken(lambda: CompetencyCriterion.objects.filter(pk=criterion.pk).delete()) with pytest.raises(CompetencyCriterion.DoesNotExist): delete_or_archive_competency_criterion(criterion.id, user) @@ -549,6 +498,8 @@ def test_delete_after_the_learner_is_gone_deletes_the_emptied_archived_groups( assert result.archived is False assert not CompetencyCriterion.objects.filter(pk=criterion.id).exists() assert not CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, root.id]).exists() + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", leaf.id) == user + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", root.id) == user def test_delete_leaves_an_archived_group_that_still_holds_an_archived_criterion( diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index a07576530..287431ebb 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -11,7 +11,6 @@ import pytest import rules -from django.core.exceptions import ValidationError from django.urls import reverse from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey @@ -2063,24 +2062,22 @@ def test_repeat_delete_on_an_archived_criterion_is_idempotent_200( assert response.data == {"id": criterion.id, "archived": True} -def test_unknown_criterion_id_404s(user_client: APIClient, tag: Tag) -> None: - """A criterion_id that never existed under a real tag 404s from the view's own lookup.""" - response = user_client.delete(criterion_delete_url(tag.id, 999999)) - - assert response.status_code == status.HTTP_404_NOT_FOUND - - -def test_repeat_delete_after_hard_delete_404s( - user_client: APIClient, criterion: CompetencyCriterion, +@pytest.mark.parametrize("hard_delete_first", [False, True], ids=["never-existed", "already-hard-deleted"]) +def test_delete_of_a_criterion_that_is_not_there_404s( + hard_delete_first: bool, user_client: APIClient, criterion: CompetencyCriterion, ) -> None: - """Repeating a DELETE after a hard-delete removed the row is not idempotent like the archived case: it 404s.""" - url = criterion_delete_url(criterion.group.tag_id, criterion.id) - first = user_client.delete(url) - assert first.status_code == status.HTTP_200_OK + """ + A criterion_id that never existed, or whose row an earlier hard delete removed, 404s from the view's + own lookup. Repeating a hard delete is not idempotent like repeating an archive. + """ + criterion_id = 999999 + if hard_delete_first: + criterion_id = criterion.id + assert user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion_id)).status_code == 200 - second = user_client.delete(url) + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion_id)) - assert second.status_code == status.HTTP_404_NOT_FOUND + assert response.status_code == status.HTTP_404_NOT_FOUND def test_cascade_deleted_group_404s_on_subsequent_create( @@ -2124,7 +2121,7 @@ def test_delete_no_permission_is_403( user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, tag: Tag, group: CompetencyCriteriaGroup, default_rule_profile: CompetencyRuleProfile, ) -> None: - """A caller refused by the object-level tagging rule gets a 403, and the criterion and ObjectTag remain.""" + """A caller refused by the object-level tagging rule gets a 403, and the criterion remains.""" object_tag = ObjectTag.objects.create( object_id=f"block-v1:unused+{UNAUTHORIZED_MARKER}+unused+type@sequential+block@p1", taxonomy=competency_taxonomy, @@ -2138,7 +2135,6 @@ def test_delete_no_permission_is_403( assert response.status_code == status.HTTP_403_FORBIDDEN assert CompetencyCriterion.objects.filter(id=criterion.id).exists() - assert ObjectTag.objects.filter(id=object_tag.id).exists() def test_delete_without_view_access_to_the_taxonomy_is_403( @@ -2152,7 +2148,6 @@ def test_delete_without_view_access_to_the_taxonomy_is_403( assert response.status_code == status.HTTP_403_FORBIDDEN assert CompetencyCriterion.objects.filter(id=criterion.id).exists() - assert ObjectTag.objects.filter(id=criterion.object_tag_id).exists() def test_delete_of_a_criterion_gone_by_the_time_of_the_call_is_404( @@ -2168,15 +2163,47 @@ def test_delete_of_a_criterion_gone_by_the_time_of_the_call_is_404( assert response.status_code == status.HTTP_404_NOT_FOUND -def test_delete_maps_a_validation_error_from_the_api_to_400( - user_client: APIClient, criterion: CompetencyCriterion, +def test_archiving_a_criterion_whose_stored_payload_went_invalid_is_400_and_changes_nothing( + user_client: APIClient, user, batch: tuple[CompetencyCriterion, CompetencyCriterion], now: datetime, ) -> None: - """A Django ValidationError raised by the API function becomes a 400 keyed by its field.""" - with mock.patch( - "openedx_learning.applets.cbe.rest_api.v1.views.delete_or_archive_competency_criterion", - side_effect=ValidationError({"object_id": "x"}), - ): - response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + """ + Archiving saves the criterion, which re-validates its rule payload. A payload that stopped being + valid after it was stored, here written past validation with update(), makes the DELETE a 400 + keyed by the model-level error, and the criterion is neither archived nor deleted. + """ + criterion = batch[1] + CompetencyCriterion.objects.filter(pk=criterion.pk).update(rule_payload_override={"op": "gt", "value": 75}) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "object_id" in response.json() + assert list(response.json()) == ["__all__"] + criterion.refresh_from_db() + assert criterion.archived is False + + +@pytest.mark.parametrize("endpoint", ["delete a criterion", "bulk update criteria"]) +def test_a_competency_tag_deleted_while_the_request_waits_for_its_lock_is_404( + endpoint: str, + run_when_tag_lock_is_taken, + user_client: APIClient, + leaf: CompetencyCriteriaGroup, + batch: tuple[CompetencyCriterion, CompetencyCriterion], +) -> None: + """ + A competency tag removed by an admin while the request waits for the Tag lock is a 404, not a 500. + The group's foreign key to the tag cascades, so the row can't go before the request starts without + taking the criterion with it; the delete runs as the lock is taken, as a commit during the wait would. + """ + run_when_tag_lock_is_taken(lambda: Tag.objects.filter(pk=leaf.tag_id).delete()) + + if endpoint == "delete a criterion": + response = user_client.delete(criterion_delete_url(leaf.tag_id, batch[0].id)) + else: + body = {"criterion_ids": [criterion.id for criterion in batch], **NEW_RULE} + response = user_client.patch(bulk_update_url(leaf.id), body, format="json") + + assert response.status_code == status.HTTP_404_NOT_FOUND From 5c767e9298194960ff1ca6a44d85d03b8871425a Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 19:21:06 +0300 Subject: [PATCH 18/24] docs: record #674's deviations in ADRs 0002, 0003 and 0004 (#674) 0002 Decision 2: the delete cascade keeps empty groups out of the DB. 0002 Decision 7: the cascade checks group status before a group delete. 0003 Decision 3: an archived criterion's ObjectTag stays live for MVP. 0004 Decision 6: only learner-status writers are lock-free; authoring writers lock the competency's Tag row first. Co-Authored-By: Claude Fable 5 --- .../decisions/0002-competency-criteria-model.rst | 11 +++++++++-- .../0003-competency-criteria-versioning.rst | 10 +++++++++- .../0004-competency-mastery-concurrency.rst | 12 +++++++++++- 3 files changed, 29 insertions(+), 4 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..b1e831c3c 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -108,7 +108,7 @@ Decision Boundaries and intended behavior: - - Empty groups: Persisted criteria definitions should not contain empty groups. Authoring flows may temporarily create empty groups while editing, but backend validation must reject them. + - Empty groups: Persisted criteria definitions should not contain empty groups. Authoring flows may temporarily create empty groups while editing. The delete cascade enforces this rule: when deleting a criterion leaves its group with no children, that group is deleted, or archived when learner progress references it, in the same transaction, so no empty group persists. - Mixed tree depths: Backend supports deeply nested groups. Current frontend authoring constraint is a maximum depth of 3 layers total, using zero-indexed depth (``0=root``, ``1=course-scope group``, ``2=leaf criteria/group``). - Retrieval scope: A read always includes the root group, plus any selected subset of course-scoped groups, each returned as a complete subtree, never truncated. - Practical size and growth: Total rows in ``CompetencyCriteriaGroup`` are expected to grow over time as course runs are added; this ADR sets no global DB row cap. No max total node-count cap is required per root group. Each course-scoped subtree is expected to stay small (realistically <=500 nodes). @@ -311,7 +311,7 @@ Decision - If no related row exists in ``StudentCompetencyCriteriaStatus`` (the leaf-level learner status table, Decision 6) for a competency definition, delete behaves as it does today: a hard delete that cascades through competency metadata tables. - Once a related row exists in ``StudentCompetencyCriteriaStatus``, deletion of the associated competency definition row still succeeds, but as an archive (soft delete) instead of a hard delete: the row is hidden from authoring and new associations but remains queryable, so existing learner status rows stay resolvable. This archive-vs-hard-delete rule applies to ``oel_tagging_tag``, ``oel_tagging_taxonomy``, ``CompetencyTaxonomy``, ``oel_tagging_objecttag``, ``CompetencyCriteriaGroup``, and ``CompetencyCriteria``; see :ref:`openedx-learning-adr-0003` Decision 3 for ``oel_tagging_objecttag``'s own archive rule and traceability exception. - - ``StudentCompetencyCriteriaStatus`` is what determines whether a record is protected. ``StudentCompetencyCriteriaGroupStatus`` and ``StudentCompetencyStatus`` are roll-up tables derived from it (Decision 6) and are not independently checked for this purpose: :ref:`openedx-learning-adr-0004` writes the leaf table synchronously with the grade but rolls the two roll-up tables up later via an asynchronous task, which can lag behind the leaf or, per that ADR's Decision 5, need manual recovery. Checking only the roll-up tables could therefore miss real learner progress that has not rolled up yet. + - ``StudentCompetencyCriteriaStatus`` is what determines whether a record is protected. ``StudentCompetencyCriteriaGroupStatus`` and ``StudentCompetencyStatus`` are roll-up tables derived from it (Decision 6) and are not independently checked for this purpose, with one exception: the criterion delete cascade checks ``StudentCompetencyCriteriaGroupStatus`` directly before hard-deleting a group, because a group row about to be deleted cannot wait for the roll-up to catch up. :ref:`openedx-learning-adr-0004` writes the leaf table synchronously with the grade but rolls the two roll-up tables up later via an asynchronous task, which can lag behind the leaf or, per that ADR's Decision 5, need manual recovery. Checking only the roll-up tables could therefore miss real learner progress that has not rolled up yet. - Direct deletion of a ``CompetencyRuleProfile`` is never a hard delete; retirement is always archive-only, via a normal update to its ``archived`` column (Decision 3). However, if a taxonomy or course that is associated with a taxonomy- or course-scoped profile is deleted, then this profile will be deleted along with it. - For MVP, archiving a ``CompetencyCriteriaGroup`` or a ``CompetencyCriteria`` row is permanent, with one exception: an archived root ``CompetencyCriteriaGroup`` is reactivated when a new criterion is added for its competency (Decision 2). ``oel_tagging_objecttag`` follows :ref:`openedx-learning-adr-0003` Decision 3 instead. @@ -490,3 +490,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-04: + +* Amended Decisions 2 and 7 for issue #674: the criterion delete cascade, not a backend validation + rejection, keeps empty groups out of the database, and it checks + ``StudentCompetencyCriteriaGroupStatus`` directly before hard-deleting a group, as #674's + acceptance criteria require. diff --git a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst index 0c839b74a..43def79a4 100644 --- a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst +++ b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst @@ -35,7 +35,7 @@ For the initial implementation, versioning and traceability of competency achiev - Before any related learner status exists, edits and deletes are allowed. - After any related learner status exists, disassociation/deletion is archive-only (soft delete), not hard delete. - - Ordinary reads (tag lists, object-tag-for-an-object lookups used by authoring UIs, and similar) exclude archived rows, so an archived association behaves as gone from those paths. + - Ordinary reads (tag lists, object-tag-for-an-object lookups used by authoring UIs, and similar) exclude archived rows, so an archived association behaves as gone from those paths. For MVP, one exception applies: the ``ObjectTag`` of an archived ``CompetencyCriterion`` stays live, so the competency tag remains visible on the tagged object while the criterion itself is closed to authoring. - The read path that resolves a learner status row back to the ``ObjectTag`` it references is exempt from that exclusion: archived rows remain queryable on that path, so learner status records can continue to be traced back to their source association. 4. Authoring guardrails must warn on potentially impactful edits: @@ -128,3 +128,11 @@ Rejected Alternatives 11. Drop the database-level foreign key constraint on the learner column. - Cons: - This repo's convention is a real foreign key to ``settings.AUTH_USER_MODEL``. Reports of contention on the user row exist elsewhere in the ecosystem, but are not understood well enough here to design around. + +Changelog +--------- + +2026-11-04: + +* Amended Decision 3 for issue #674: the ``ObjectTag`` of an archived criterion stays live for MVP, + so the competency tag remains visible on the object. diff --git a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst index a018cd45b..aa51c97c1 100644 --- a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -68,7 +68,9 @@ Decision rows above it are recalculated and overwritten rather than merged. A later grade change can raise what an edit lowered, but never lower what an edit raised. The staff correction takes a row lock on the ``StudentCompetencyCriteriaGroupStatus`` row for that learner and the competency's root - criteria group, the group with no parent; no other path takes a lock. + criteria group, the group with no parent; no other learner-status writer takes a lock. Criterion + authoring writers (create, delete, bulk update, and group update) serialize per competency on + the competency's ``Tag`` row, each taking that lock first and no other group or criterion row lock. @@ -162,3 +164,11 @@ Rejected Alternatives before the write completes. - It also only checks for a race at the moment each parent is read. If the worker crashes mid-cascade before reaching the next read, nothing notices the rollup was left unfinished. Decision 5's manual recovery mechanism exists to catch that case. + +Changelog +--------- + +2026-11-04: + +* Amended Decision 6 for issue #674: the rule that no other path takes a lock covers learner-status + writers only. Criterion authoring writers serialize per competency on the ``Tag`` row. From 255f8911abc5cc9265a8c083c2a3d8a053cae4d3 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 19:27:44 +0300 Subject: [PATCH 19/24] docs: tighten the new lock docstrings and comments (#674) An editorial pass over the prose this branch added. The delete docstring names the ancestor walk instead of an undefined "walk" and "collector", drops HTTP vocabulary from the HTTP-free API layer, and marks ObjectTag as a code literal. The cascade docstring says an archived group is never saved again, since a delete can still remove it. The two new Raises sentences both say "deleted concurrently". ADR 0002 Decision 2 records that the cascade also removes emptied ancestors, not only the deleted criterion's own group. Co-Authored-By: Claude Fable 5 --- .../0002-competency-criteria-model.rst | 2 +- src/openedx_learning/applets/cbe/api.py | 36 ++++++++++--------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index b1e831c3c..640045b96 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -108,7 +108,7 @@ Decision Boundaries and intended behavior: - - Empty groups: Persisted criteria definitions should not contain empty groups. Authoring flows may temporarily create empty groups while editing. The delete cascade enforces this rule: when deleting a criterion leaves its group with no children, that group is deleted, or archived when learner progress references it, in the same transaction, so no empty group persists. + - Empty groups: Persisted criteria definitions should not contain empty groups. Authoring flows may temporarily create empty groups while editing. The delete cascade enforces this rule: when a criterion delete leaves its group, or an ancestor above it, with no children, that group is deleted, or archived when learner progress references it, in the same transaction, so no empty group persists. - Mixed tree depths: Backend supports deeply nested groups. Current frontend authoring constraint is a maximum depth of 3 layers total, using zero-indexed depth (``0=root``, ``1=course-scope group``, ``2=leaf criteria/group``). - Retrieval scope: A read always includes the root group, plus any selected subset of course-scoped groups, each returned as a complete subtree, never truncated. - Practical size and growth: Total rows in ``CompetencyCriteriaGroup`` are expected to grow over time as course runs are added; this ADR sets no global DB row cap. No max total node-count cap is required per root group. Each course-scoped subtree is expected to stay small (realistically <=500 nodes). diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index eb5462611..564970ca1 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -701,7 +701,7 @@ def bulk_update_competency_criteria( if any named id isn't a criterion of that group, and ValidationError, keyed by the REST field names, for a non-leaf group, an empty or repeated id list, or an invalid rule source. Raises CompetencyCriterionArchivedError if the group, either of its ancestors, or any named criterion - is archived. Raises Tag.DoesNotExist if the group's competency tag was deleted while the request ran. + is archived. Raises Tag.DoesNotExist if the group's competency tag was deleted concurrently. Takes the competency's Tag row lock before reading anything that can change, like every writer of its criteria, so it waits for a concurrent create or delete of the same competency. @@ -880,7 +880,7 @@ def update_competency_criteria_group( only for a real change. Learner status is never read or written. Raises CompetencyCriteriaGroup.DoesNotExist if no group has ``group_id``, or Tag.DoesNotExist - if its tag isn't on a CompetencyTaxonomy or was deleted while the request ran. + if its tag isn't on a CompetencyTaxonomy or was deleted concurrently. Raises ValidationError, keyed by the REST field names, for a root group, a group with no course anywhere above it to check permission against, or a ``logic_operator`` other than AND or OR, and PermissionDenied if ``user`` fails ``oel_tagging.can_tag_object`` for the group's taxonomy and @@ -955,8 +955,8 @@ def _cascade_from_group(group: CompetencyCriteriaGroup, user: UserType) -> None: Checking both relations (``criteria`` and ``child_groups``) at every level is harmless: the one that does not apply is an empty queryset, and it avoids a separate leaf/non-leaf code path. - An archived group is never written again: everything above it was handled when it was archived, - so a second write would only add a redundant history row. If it has no criteria, no child groups + An archived group is never saved again: everything above it was handled when it was archived, + so a second save would only add a redundant history row. If it has no criteria, no child groups and no group-level status, it is deleted and the walk continues upward, so no empty group stays persisted. Otherwise the walk stops there. """ @@ -1024,7 +1024,7 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> and archive behavior may change this function's shape without a deprecation cycle. ``user`` must hold ``oel_tagging.can_tag_object`` for the taxonomy of the criterion's group and for - the usage key its ObjectTag tags. The check runs before anything is locked or changed. + the usage key its ``ObjectTag`` tags. The check runs before anything is locked or changed. An ancestor :class:`CompetencyCriteriaGroup` left with no live children by this call is cascaded in turn: hard-deleted if it has no children of any kind and no group-level status of its own, archived @@ -1040,18 +1040,20 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> The first read and the permission check run before the transaction. The rest runs in one transaction that locks only the competency's Tag row, the lock every writer of its criteria takes first, so a concurrent create waits until this call commits. The lock prevents three races that - follow from Django's Python-side cascade. A create that commits a new leaf and criterion between the - walk's emptiness check and the collector's SELECT inside ``group.delete()`` would have those rows - deleted silently, after its caller received a 201. Two deletes of sibling criteria in one leaf would - each still see the other's criterion and leave an empty leaf behind. A create that reuses the same - ObjectTag and commits between the "still referenced" check and the ObjectTag delete would have its - criterion cascaded away. The lock is held until the request commits, so other authoring writers for - this competency, and inserts that reference its Tag row, wait meanwhile. - - The code relies on the READ COMMITTED isolation level: the re-read after the lock and the walk's - ``exists()`` checks must see concurrent commits. Under REPEATABLE READ the re-read would see the - snapshot from before the lock, so a racing duplicate delete would return success and write a second - deletion history row, and two sibling deletes could leave an empty group, all without an error. + follow from Django's Python-side cascade. A create that commits a new leaf and criterion between + the ancestor walk's emptiness check and the row-collecting SELECT inside ``group.delete()`` would + have those rows deleted silently, after the create call already returned success. Two deletes of + sibling criteria in one leaf would each still see the other's criterion and leave an empty leaf + behind. A create that reuses the same ``ObjectTag`` and commits between the "still referenced" + check and the ``ObjectTag`` delete would have its criterion cascaded away. The lock is held until + the request commits, so other authoring writers for this competency, and inserts that reference + its Tag row, wait meanwhile. + + The code relies on the READ COMMITTED isolation level: the re-read after the lock and the + ancestor walk's ``exists()`` checks must see concurrent commits. Under REPEATABLE READ the re-read + would see the snapshot from before the lock, so a racing duplicate delete would return success and + write a second deletion history row, and two sibling deletes could leave an empty group, all + without an error. A learner's grade saved at the same moment as a hard delete fails one side with an error. The delete checks for a status row and then deletes the criterion, and a grade write that inserts a status row From c620f1d6117fd9dd926dc7da9f9ca3394afec5fe Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 22:06:15 +0300 Subject: [PATCH 20/24] fix: lock in create call, map deleted tag in group view create_competency_criterion() now takes the competency's Tag row lock itself, so a caller outside associate_competency_criterion() is serialized with a criterion delete instead of having its new rows cascaded away. Re-acquiring the lock inside one transaction is harmless. The group PATCH view maps Tag.DoesNotExist, raised when the tag is deleted while the request waits for the lock, to 404 instead of 500. Co-Authored-By: Claude Fable 5 --- src/openedx_learning/applets/cbe/api.py | 11 +++++++---- .../applets/cbe/rest_api/v1/views.py | 2 +- tests/openedx_learning/applets/cbe/test_api.py | 14 ++++++++++++++ tests/openedx_learning/applets/cbe/test_views.py | 6 +++++- 4 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 564970ca1..24953a6ac 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -435,10 +435,13 @@ def create_competency_criterion( resolves to a concrete profile, and the model's own xor-override constraint forbids leaving all three fields null. - ``course_run`` is ``group``'s course. Must run inside a transaction holding the tag and - course run locks that associate_competency_criterion() takes, so the dominance check below - can't race another create. + ``course_run`` is ``group``'s course. Must run inside a transaction. This function takes the + competency's Tag row lock itself, so a concurrent delete of a sibling criterion can't cascade + this call's new rows away. The caller must also hold the course run lock that + associate_competency_criterion() takes, so the dominance check below can't race another create. """ + # Re-acquiring the lock associate_competency_criterion() already holds is harmless within one transaction. + Tag.objects.select_for_update().get(pk=group.tag_id) tag = group.tag # tag.taxonomy_id is nullable at the model level; already guaranteed set by the caller. assert tag.taxonomy_id is not None @@ -572,7 +575,7 @@ def associate_competency_criterion( with transaction.atomic(): # Lock order is always the tag, then the course run, so two creates can't deadlock. - # The database enforces one root per tag; the tag lock only serializes _reject_duplicate_criterion(). That + # The database enforces one root per tag; the tag lock serializes _reject_duplicate_criterion(). That # check is check-then-act on both paths: without group_id a request always creates a fresh leaf, and with # group_id CompetencyCriterion has no unique constraint on (object_tag, group), so no index catches a # concurrent copy. delete_or_archive_competency_criterion() depends on this lock too: it keeps this diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/views.py b/src/openedx_learning/applets/cbe/rest_api/v1/views.py index f23959b81..50fc19c93 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -267,7 +267,7 @@ def patch(self, request, tag_id: int, group_id: int): group = update_competency_criteria_group( group.id, logic_operator=serializer.validated_data["logic_operator"], user=request.user, ) - except CompetencyCriteriaGroup.DoesNotExist as exc: + except (CompetencyCriteriaGroup.DoesNotExist, Tag.DoesNotExist) as exc: raise NotFound(str(exc)) from exc except DjangoValidationError as exc: raise DRFValidationError(exc.message_dict if hasattr(exc, "message_dict") else exc.messages) from exc diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index c27a95d88..7dc1ba3d6 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -1994,6 +1994,20 @@ def _fail(*args, **kwargs): assert criterion.group_id == group.id +def test_create_competency_criterion_takes_the_tag_lock_itself( + run_when_tag_lock_is_taken, tag: Tag, course_run: CourseRun, +) -> None: + """ + A caller outside associate_competency_criterion() is still serialized with a delete of the competency's + criteria: the lock is taken before anything else, so a tag deleted while the call waits for it is seen. + """ + group = _create_leaf_group(tag, course_run) + run_when_tag_lock_is_taken(lambda: Tag.objects.filter(pk=tag.pk).delete()) + + with pytest.raises(Tag.DoesNotExist): + cbe_api.create_competency_criterion(group, usage_key(course_run, "p1"), course_run=course_run) + + def test_associate_competency_criterion_creates_the_course_runs_cbe_row( tag: Tag, course_run: CourseRun, ) -> None: diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 287431ebb..10de94393 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -2202,8 +2202,12 @@ def test_a_competency_tag_deleted_while_the_request_waits_for_its_lock_is_404( if endpoint == "delete a criterion": response = user_client.delete(criterion_delete_url(leaf.tag_id, batch[0].id)) - else: + elif endpoint == "bulk update criteria": body = {"criterion_ids": [criterion.id for criterion in batch], **NEW_RULE} response = user_client.patch(bulk_update_url(leaf.id), body, format="json") + else: + response = user_client.patch( + group_detail_url(leaf.tag_id, leaf.id), {"logic_operator": LogicOperator.AND}, format="json", + ) assert response.status_code == status.HTTP_404_NOT_FOUND From 3e9a2328ae32a59a0d563e993901da3d53c686c4 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 22:06:23 +0300 Subject: [PATCH 21/24] fix: check permission before locking in group update The checks read only a group's tag, parent and course, which never change, so they run before the Tag lock. A caller who fails them no longer holds the lock. The re-read, archived and logic_operator checks stay after it, and the re-read keeps select_related("course") because the view serializes the returned group. Co-Authored-By: Claude Fable 5 --- src/openedx_learning/applets/cbe/api.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 24953a6ac..dd9961218 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -900,9 +900,6 @@ def update_competency_criteria_group( tag = resolve_competency_tag(group.tag_id) # tag.taxonomy is already confirmed set; re-asserted since that doesn't cross function boundaries for mypy. assert tag.taxonomy is not None - Tag.objects.select_for_update().get(pk=tag.pk) - # Re-read under the Tag lock: a concurrent delete or archive may have won. - group = CompetencyCriteriaGroup.objects.select_related("course", "parent__course").get(pk=group_id) # A root has neither a parent nor a course, and its subtree can span several courses, so # there's no one course to check permission against. if group.parent_id is None and group.course_id is None: @@ -922,6 +919,12 @@ def update_competency_criteria_group( if not user.has_perm("oel_tagging.can_tag_object", perm_obj): # type: ignore[arg-type] raise PermissionDenied + # The checks above read only a group's tag, parent and course, which never change, so they run before + # the lock and a caller who fails them never holds it. + Tag.objects.select_for_update().get(pk=tag.pk) + # Re-read under the Tag lock: a concurrent delete or archive may have won. + group = CompetencyCriteriaGroup.objects.select_related("course").get(pk=group_id) + # Null is a stored state (a single-child group), but never one a caller may ask for. if logic_operator not in LogicOperator.values: raise ValidationError( From be29b8fb323fedc604a47bd068a152b5fe78dd7b Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 22:06:23 +0300 Subject: [PATCH 22/24] refactor: set history users through one helper Replace the seven _history_user assignments, each carrying a type and pylint suppression, with _set_history_user(), which carries the one suppression pair and the reason in its docstring. Co-Authored-By: Claude Fable 5 --- src/openedx_learning/applets/cbe/api.py | 26 ++++++++++++------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index dd9961218..2b486ee64 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -638,6 +638,11 @@ def _validate_rule_source( raise ValidationError(errors) from exc +def _set_history_user(instance: Any, user: UserType) -> None: + """Record ``user`` as the author of ``instance``'s next history row; API callers have no request middleware.""" + instance._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + + def _apply_rule_source( criterion: CompetencyCriterion, rule_profile_id: int | None, @@ -672,8 +677,7 @@ def _apply_rule_source( return criterion.rule_profile_id, criterion.rule_type_override, criterion.rule_payload_override = target - # Set on the instance so attribution doesn't depend on simple_history's request middleware. - criterion._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + _set_history_user(criterion, user) criterion._change_reason = change_reason # type: ignore[attr-defined] # pylint: disable=protected-access # Never QuerySet.update(): it skips post_save, so simple_history would record nothing. criterion.save(update_fields=["rule_profile", "rule_type_override", "rule_payload_override"]) @@ -939,8 +943,7 @@ def update_competency_criteria_group( return group group.logic_operator = logic_operator - # Set for history attribution; see _apply_rule_source. - group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + _set_history_user(group, user) # Never QuerySet.update(): it skips post_save, so simple_history would record nothing. group.save(update_fields=["logic_operator"]) return group @@ -949,8 +952,7 @@ def update_competency_criteria_group( def _archive_group(group: CompetencyCriteriaGroup, user: UserType) -> None: """Archive ``group``, writing only the flag and recording ``user`` in its history.""" group.archived = True - # Set for history attribution; see _apply_rule_source. - group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + _set_history_user(group, user) group.save(update_fields=["archived"]) @@ -972,8 +974,7 @@ def _cascade_from_group(group: CompetencyCriteriaGroup, user: UserType) -> None: return if _group_has_learner_progress(group.id): return - # Set for history attribution; see _apply_rule_source. - group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + _set_history_user(group, user) group.delete() if parent is not None: _cascade_from_group(parent, user) @@ -987,8 +988,7 @@ def _cascade_from_group(group: CompetencyCriteriaGroup, user: UserType) -> None: if _group_has_learner_progress(group.id): _archive_group(group, user) else: - # Set for history attribution; see _apply_rule_source. - group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + _set_history_user(group, user) group.delete() else: # Only archived children remain. @@ -1006,8 +1006,7 @@ def _archive_criterion(criterion: CompetencyCriterion, user: UserType) -> None: if criterion.archived: return criterion.archived = True - # Set for history attribution; see _apply_rule_source. - criterion._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + _set_history_user(criterion, user) criterion.save(update_fields=["archived"]) _cascade_from_group(criterion.group, user) @@ -1085,8 +1084,7 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> return CriterionDeletionResult(id=criterion_id, archived=True) group = criterion.group object_tag = criterion.object_tag - # Set for history attribution; see _apply_rule_source. - criterion._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + _set_history_user(criterion, user) criterion.delete() # Only this criterion's own ObjectTag is a candidate, and only while no other criterion uses it. if not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists(): From 2dedf27cbb220cbad7142cbff9dea30d76c4ea7f Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 22:06:23 +0300 Subject: [PATCH 23/24] docs: tighten docstrings, comments and ADR wording for criterion delete Replace the REPEATABLE READ paragraph in the delete docstring with a statement of the READ COMMITTED reliance, list ValidationError in its Raises sentence, reword the bulk-update save-order comment, and add the one-call-per-transaction rule to record_graded_object_statuses(). Scope the ADR-0002, ADR-0003 and ADR-0004 amendments to what the delete path guarantees: the empty-group rule covers the groups the delete path empties, the live ObjectTag covers criteria archived by the delete endpoint, and the lock rule excludes only explicit locking reads on groups and criteria. Co-Authored-By: Claude Fable 5 --- .../0002-competency-criteria-model.rst | 4 ++-- .../0003-competency-criteria-versioning.rst | 4 ++-- .../0004-competency-mastery-concurrency.rst | 2 +- src/openedx_learning/applets/cbe/api.py | 23 +++++++++++-------- .../applets/cbe/rest_api/v1/views.py | 5 ++-- 5 files changed, 21 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 640045b96..9eee316d3 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -108,7 +108,7 @@ Decision Boundaries and intended behavior: - - Empty groups: Persisted criteria definitions should not contain empty groups. Authoring flows may temporarily create empty groups while editing. The delete cascade enforces this rule: when a criterion delete leaves its group, or an ancestor above it, with no children, that group is deleted, or archived when learner progress references it, in the same transaction, so no empty group persists. + - Empty groups: Persisted criteria definitions should not contain empty groups. Authoring flows may temporarily create empty groups while editing. The criterion delete path enforces this rule for the groups it empties: such a group is deleted, or archived when learner progress references it or when only archived children remain, in the same transaction, so that path persists no empty group. Writers outside this path are not covered by it. - Mixed tree depths: Backend supports deeply nested groups. Current frontend authoring constraint is a maximum depth of 3 layers total, using zero-indexed depth (``0=root``, ``1=course-scope group``, ``2=leaf criteria/group``). - Retrieval scope: A read always includes the root group, plus any selected subset of course-scoped groups, each returned as a complete subtree, never truncated. - Practical size and growth: Total rows in ``CompetencyCriteriaGroup`` are expected to grow over time as course runs are added; this ADR sets no global DB row cap. No max total node-count cap is required per root group. Each course-scoped subtree is expected to stay small (realistically <=500 nodes). @@ -494,6 +494,6 @@ Changelog 2026-11-04: * Amended Decisions 2 and 7 for issue #674: the criterion delete cascade, not a backend validation - rejection, keeps empty groups out of the database, and it checks + rejection, keeps the groups it empties out of the database, and it checks ``StudentCompetencyCriteriaGroupStatus`` directly before hard-deleting a group, as #674's acceptance criteria require. diff --git a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst index 43def79a4..5b46163d0 100644 --- a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst +++ b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst @@ -35,7 +35,7 @@ For the initial implementation, versioning and traceability of competency achiev - Before any related learner status exists, edits and deletes are allowed. - After any related learner status exists, disassociation/deletion is archive-only (soft delete), not hard delete. - - Ordinary reads (tag lists, object-tag-for-an-object lookups used by authoring UIs, and similar) exclude archived rows, so an archived association behaves as gone from those paths. For MVP, one exception applies: the ``ObjectTag`` of an archived ``CompetencyCriterion`` stays live, so the competency tag remains visible on the tagged object while the criterion itself is closed to authoring. + - Ordinary reads (tag lists, object-tag-for-an-object lookups used by authoring UIs, and similar) exclude archived rows, so an archived association behaves as gone from those paths. For MVP, one exception applies: the ``ObjectTag`` of a criterion archived by the delete endpoint stays live, so the competency tag remains visible on the tagged object while the criterion itself is closed to authoring. - The read path that resolves a learner status row back to the ``ObjectTag`` it references is exempt from that exclusion: archived rows remain queryable on that path, so learner status records can continue to be traced back to their source association. 4. Authoring guardrails must warn on potentially impactful edits: @@ -134,5 +134,5 @@ Changelog 2026-11-04: -* Amended Decision 3 for issue #674: the ``ObjectTag`` of an archived criterion stays live for MVP, +* Amended Decision 3 for issue #674: the ``ObjectTag`` of a criterion archived by the delete endpoint stays live for MVP, so the competency tag remains visible on the object. diff --git a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst index aa51c97c1..1d20a08a6 100644 --- a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -70,7 +70,7 @@ Decision on the ``StudentCompetencyCriteriaGroupStatus`` row for that learner and the competency's root criteria group, the group with no parent; no other learner-status writer takes a lock. Criterion authoring writers (create, delete, bulk update, and group update) serialize per competency on - the competency's ``Tag`` row, each taking that lock first and no other group or criterion row lock. + the competency's ``Tag`` row, each taking that lock first and no other explicit locking read on groups or criteria. diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 2b486ee64..3bdd080a7 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -776,8 +776,9 @@ def bulk_update_competency_criteria( # every criterion in a leaf shares the group's tag, so they all resolve to the same profile. applicable = _get_applicable_rule_profile(group.tag.taxonomy.id) if rule_profile_id is None else None criteria = [criteria_by_id[criterion_id] for criterion_id in criterion_ids] - # Saves run in primary-key order because each save() takes an exclusive row lock and the grading - # write locks criteria in ascending order; two different orders would deadlock. + # Saves run in ascending primary-key order because the grading write inserts status rows in ascending + # criterion-pk order, and each insert takes an implicit shared foreign-key lock on its criterion row; + # a different order here could deadlock with it. for criterion in sorted(criteria, key=lambda item: item.pk): _apply_rule_source( criterion, @@ -821,6 +822,10 @@ def record_graded_object_statuses(*, user_id: int, scores: Sequence[GradedObject raised, where ``0`` means nothing changed. This function performs no authorization check of its own; as with ``openedx_tagging.api``, the caller is responsible for enforcing permissions. + Call this at most once per transaction. Its implicit criterion-row locks are taken in ascending + primary-key order within one call, and two calls in one transaction would form two ascending runs + that can deadlock with a concurrent bulk update. + :raises TransactionManagementError: if called outside an atomic block. """ # In autocommit mode get_or_create() would commit a leaf row on its own, apart from the grade. @@ -1039,10 +1044,11 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> criterion references it. No other ObjectTag on the object is touched. Raises ``CompetencyCriterion.DoesNotExist`` if no such criterion exists (including one deleted - concurrently), ``Tag.DoesNotExist`` if its competency tag was deleted concurrently, and Django's - ``PermissionDenied`` if ``user`` fails the permission check. + concurrently), ``Tag.DoesNotExist`` if its competency tag was deleted concurrently, Django's + ``PermissionDenied`` if ``user`` fails the permission check, and Django's ``ValidationError`` if + the archive path's ``full_clean()`` rejects an invalid stored rule payload. - The first read and the permission check run before the transaction. The rest runs in one + The first read and the permission check run before the lock. The rest runs in one transaction that locks only the competency's Tag row, the lock every writer of its criteria takes first, so a concurrent create waits until this call commits. The lock prevents three races that follow from Django's Python-side cascade. A create that commits a new leaf and criterion between @@ -1054,11 +1060,8 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> the request commits, so other authoring writers for this competency, and inserts that reference its Tag row, wait meanwhile. - The code relies on the READ COMMITTED isolation level: the re-read after the lock and the - ancestor walk's ``exists()`` checks must see concurrent commits. Under REPEATABLE READ the re-read - would see the snapshot from before the lock, so a racing duplicate delete would return success and - write a second deletion history row, and two sibling deletes could leave an empty group, all - without an error. + The re-read under the lock and the ancestor walk's ``exists()`` checks rely on READ COMMITTED, the + isolation level this repo always runs (see ``AGENTS.md``): they must see concurrent commits. A learner's grade saved at the same moment as a hard delete fails one side with an error. The delete checks for a status row and then deletes the criterion, and a grade write that inserts a status row diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/views.py b/src/openedx_learning/applets/cbe/rest_api/v1/views.py index 50fc19c93..c83d8bbd2 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -292,8 +292,9 @@ def delete(self, request, *args, **kwargs): """ Resolve the criterion scoped to both path segments, then delegate to the public API, which checks permission. - The URL is scoped by tag_id, not group_id, because its caller (#710) sends exactly - ``DELETE .../competencies//criteria//`` and has no group id. + The URL is scoped by tag_id, not group_id, because the route follows the + ``competencies//criteria/`` family #665 created, and the Studio flow addresses a criterion by + tag and criterion id. The lookup deliberately does not filter `archived=False`, so a repeat DELETE against an already-archived row still resolves (200), and a mismatched tag_id or a nonexistent criterion_id both 404 the same way, via this one get_object_or_404 call. From 3bb7e6d0b7c4238c5870037c5f8ed85793f65cec Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 4 Nov 2026 22:06:23 +0300 Subject: [PATCH 24/24] test: pin lock points and tighten assertions for criterion removal Add a bulk-update test that archives a criterion under the Tag lock, so reading the criteria before the lock fails. Replace the mocked API call in the criterion-gone view test with a real hard delete at lock time, assert the exact save order, merge the two history-attribution tests into one parametrized test, and drop assertions that cannot fail. Co-Authored-By: Claude Fable 5 --- .../openedx_learning/applets/cbe/test_api.py | 20 ++++++++- .../applets/cbe/test_criterion_concurrency.py | 4 +- .../applets/cbe/test_criterion_removal.py | 42 ++++++++----------- .../applets/cbe/test_views.py | 15 +++---- 4 files changed, 42 insertions(+), 39 deletions(-) diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index 7dc1ba3d6..ad695bb35 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -1062,8 +1062,7 @@ def _record_save(self, *args, **kwargs) -> None: result = bulk_update_competency_criteria([c3.id, c1.id, c2.id], leaf.id, user=user, **NEW_RULE) - assert saves == sorted(saves) - assert set(saves) == {c1.id, c2.id, c3.id} + assert saves == [c1.id, c2.id, c3.id] assert [criterion.id for criterion in result] == [c3.id, c1.id, c2.id] @@ -1083,6 +1082,23 @@ def test_bulk_update_checks_archived_state_under_the_tag_lock( assert snapshot(batch) == before +def test_bulk_update_reads_the_criteria_under_the_tag_lock( + run_when_tag_lock_is_taken, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, +) -> None: + """A criterion archived while the update waits for the Tag lock refuses the batch, so the criteria read after it.""" + archived = batch[1] + run_when_tag_lock_is_taken(lambda: CompetencyCriterion.objects.filter(pk=archived.pk).update(archived=True)) + before = snapshot(batch) + + with pytest.raises(CompetencyCriterionArchivedError, match=rf"criteria cannot be edited: {archived.id}\."): + bulk_update_competency_criteria([c.id for c in batch], leaf.id, user=user, **NEW_RULE) + + assert snapshot(batch) == before + + # ============================================================================================== # get_competency_criteria_tree # ============================================================================================== diff --git a/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py b/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py index 6f05bab3c..d2437ddde 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py @@ -15,7 +15,6 @@ import rules from django.apps import apps from django.db import DatabaseError, IntegrityError, connection, transaction -from django.db.utils import OperationalError from rules.permissions import permissions as rule_permissions from openedx_catalog.models import CourseRun @@ -251,9 +250,8 @@ def test_delete_and_bulk_update_do_not_deadlock( expected = (CompetencyCriterionArchivedError,) else: expected = (CompetencyCriterion.DoesNotExist, CompetencyCriteriaGroup.DoesNotExist) - with pytest.raises(expected) as raised: + with pytest.raises(expected): bulk_update.result() - assert not isinstance(raised.value, OperationalError) assert result.archived is learner_progress diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py index e203ce917..bdfd93a33 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -140,31 +140,27 @@ def test_repeat_delete_on_an_archived_criterion_writes_no_history( ) -def test_archive_records_the_acting_user_in_history( - user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, -) -> None: - """Archiving a criterion records the acting user on the newest historical row of it and of each archived group.""" - root = CompetencyCriteriaGroup.objects.create(tag=tag) - leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) - criterion = _make_criterion(leaf, object_tag, default_rule_profile) - StudentCompetencyCriterionStatus.objects.create( - user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, - ) - - delete_or_archive_competency_criterion(criterion.id, user) - - assert _latest_history_user("HistoricalCompetencyCriterion", criterion.id) == user - assert _latest_history_user("HistoricalCompetencyCriteriaGroup", leaf.id) == user - assert _latest_history_user("HistoricalCompetencyCriteriaGroup", root.id) == user - - -def test_hard_delete_records_the_acting_user_in_history( - user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +@pytest.mark.parametrize("has_status_row", [True, False], ids=["archive", "hard delete"]) +def test_removal_records_the_acting_user_in_history( + has_status_row: bool, + *, + user, + tag: Tag, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, + now: datetime, ) -> None: - """Hard-deleting a criterion records the acting user on the deletion row of it and of each cascaded group.""" + """ + Archiving a criterion, or hard-deleting one with no status row, records the acting user on the + newest historical row of it and of each group the cascade archived or deleted. + """ root = CompetencyCriteriaGroup.objects.create(tag=tag) leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) criterion = _make_criterion(leaf, object_tag, default_rule_profile) + if has_status_row: + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) delete_or_archive_competency_criterion(criterion.id, user) @@ -248,9 +244,7 @@ def test_single_child_leaf_group_cascade_deletes_while_a_sibling_keeps_the_root_ assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() assert CompetencyCriteriaGroup.objects.filter(id=sibling.id).exists() root.refresh_from_db() - sibling.refresh_from_db() assert root.archived is False - assert sibling.archived is False def test_multilevel_cascade_fully_collapses_the_root_to_leaf_chain( @@ -297,9 +291,7 @@ def test_cascade_stops_at_an_ancestor_with_a_surviving_sibling_group( assert CompetencyCriteriaGroup.objects.filter(id=other_branch.id).exists() assert CompetencyCriterion.objects.filter(id=other_criterion.id).exists() root.refresh_from_db() - other_branch.refresh_from_db() assert root.archived is False - assert other_branch.archived is False @pytest.mark.parametrize("progress_on", ["leaf group", "root group"]) diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 10de94393..8b77f5268 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -6,7 +6,6 @@ """ from datetime import datetime from typing import Any, Generator -from unittest import mock from urllib.parse import urlencode import pytest @@ -2151,14 +2150,12 @@ def test_delete_without_view_access_to_the_taxonomy_is_403( def test_delete_of_a_criterion_gone_by_the_time_of_the_call_is_404( - user_client: APIClient, criterion: CompetencyCriterion, + run_when_tag_lock_is_taken, user_client: APIClient, criterion: CompetencyCriterion, ) -> None: - """A criterion removed between the view's lookup and the API call is a 404, not a 500.""" - with mock.patch( - "openedx_learning.applets.cbe.rest_api.v1.views.delete_or_archive_competency_criterion", - side_effect=CompetencyCriterion.DoesNotExist, - ): - response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + """A criterion removed between the view's lookup and the API's locked re-read is a 404, not a 500.""" + run_when_tag_lock_is_taken(lambda: CompetencyCriterion.objects.filter(pk=criterion.pk).delete()) + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) assert response.status_code == status.HTTP_404_NOT_FOUND @@ -2185,7 +2182,7 @@ def test_archiving_a_criterion_whose_stored_payload_went_invalid_is_400_and_chan assert criterion.archived is False -@pytest.mark.parametrize("endpoint", ["delete a criterion", "bulk update criteria"]) +@pytest.mark.parametrize("endpoint", ["delete a criterion", "bulk update criteria", "update a group"]) def test_a_competency_tag_deleted_while_the_request_waits_for_its_lock_is_404( endpoint: str, run_when_tag_lock_is_taken,