From 5ee3836dddcb2a93c659ba978fb80cb34e27ef1e Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Thu, 29 Oct 2026 20:31:29 +0300 Subject: [PATCH 01/10] feat: add endpoint to delete or archive a CompetencyCriteriaGroup subtree (#675) Adds DELETE /api/cbe/v1/competencies//criteria-groups//, which hard-deletes a CompetencyCriteriaGroup subtree when no learner status exists anywhere in it, or archives the whole subtree when one does, then cascades above the target's parent. The course that scopes the permission check is resolved at any depth. The permission check runs inside delete_or_archive_competency_criteria_group. An archive writes only the archived flag and records the acting user. A hard delete removes only the ObjectTags that no remaining criterion references, and leaves every other tag on the object untouched. Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 202 +++++- src/openedx_learning/applets/cbe/data.py | 18 + .../applets/cbe/rest_api/v1/urls.py | 5 + .../applets/cbe/rest_api/v1/views.py | 37 ++ .../applets/cbe/test_criterion_removal.py | 33 + .../applets/cbe/test_group_removal.py | 622 ++++++++++++++++++ .../applets/cbe/test_views.py | 297 +++++++++ 7 files changed, 1208 insertions(+), 6 deletions(-) create mode 100644 tests/openedx_learning/applets/cbe/test_group_removal.py diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index ef9f69562..01aad5a27 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -23,7 +23,7 @@ from openedx_tagging.models import ObjectTag, Tag, Taxonomy from openedx_tagging.rules import ObjectTagPermissionItem, UserType -from .data import CriterionDeletionResult, GradedObjectScore +from .data import CriterionDeletionResult, GradedObjectScore, GroupDeletionResult from .models import ( CompetencyCourseRun, CompetencyCriteriaGroup, @@ -46,10 +46,12 @@ "CompetencyCriterionArchivedError", "CriterionDeletionResult", "GradedObjectScore", + "GroupDeletionResult", "associate_competency_criterion", "bulk_update_competency_criteria", "create_competency_criterion", "create_competency_taxonomy", + "delete_or_archive_competency_criteria_group", "delete_or_archive_competency_criterion", "get_blocked_courses", "get_competency_criteria_tree", @@ -57,6 +59,7 @@ "get_courses_with_live_criteria", "is_competency_taxonomy", "record_graded_object_statuses", + "resolve_competency_criteria_group_course", "resolve_competency_tag", "resolve_supplied_leaf_group", "roll_up_competency_statuses", @@ -1056,18 +1059,28 @@ def _cascade_remove_from_group(group: CompetencyCriteriaGroup, user: UserType) - _cascade_remove_from_group(parent, user) -def _archive_criterion(criterion: CompetencyCriterion, user: UserType) -> None: +def _write_criterion_archived(criterion: CompetencyCriterion, user: UserType) -> bool: """ - Archive ``criterion`` in place and cascade the archive to any ancestor left with no live children. + Archive ``criterion`` in place, recording ``user`` in its history, without cascading to its group. - Does nothing if ``criterion`` is already archived, so a repeat call writes no history. + Returns False and writes nothing if ``criterion`` is already archived, so a repeat call adds no history row. """ if criterion.archived: - return + return False criterion.archived = True _set_history_user(criterion, user) criterion.save(update_fields=["archived"]) - _cascade_remove_from_group(criterion.group, user) + return True + + +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 _write_criterion_archived(criterion, user): + _cascade_remove_from_group(criterion.group, user) def _criterion_has_learner_progress(criterion_id: int) -> bool: @@ -1080,6 +1093,16 @@ 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. @@ -1158,3 +1181,170 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> ObjectTag.objects.filter(pk=object_tag.id).delete() _cascade_remove_from_group(group, user) return CriterionDeletionResult(id=criterion_id, archived=False) + + +def resolve_competency_criteria_group_course(group: CompetencyCriteriaGroup) -> CourseRun: + """ + Return the CourseRun that scopes the subtree rooted at ``group``. + + 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. + + That is the course of the nearest group with one, starting at ``group`` itself and walking + up through its ancestors, so a group may sit at any depth below its course-level group. Costs + one query per ancestor walked. A caller checks ``oel_tagging.can_tag_object`` against this + course before removing the subtree. + + Raises ValidationError (keyed "group_id") if ``group`` is a root CompetencyCriteriaGroup, + whose subtree can span several courses, or if no group in its lineage has a course. + """ + if group.parent_id is None: + raise ValidationError({"group_id": _("group_id must not reference a root CompetencyCriteriaGroup.")}) + + node: CompetencyCriteriaGroup | None = group + while node is not None: + if node.course is not None: + return node.course + node = node.parent + raise ValidationError( + {"group_id": _("group_id must reference a group under a course-level CompetencyCriteriaGroup.")} + ) + + +def _lock_competency_criteria_subtree( + target: CompetencyCriteriaGroup, lineage_ids: list[int], +) -> tuple[set[int], list[CompetencyCriterion]]: + """ + Lock the groups involved in removing ``target``, then its subtree's criteria, and return both. + + Takes one ``select_for_update()`` over the target's ancestors, the target, and its descendants + (found level by level), ordered by primary key so concurrent writers cannot deadlock. Must run + inside a transaction that already holds the competency's Tag lock. + """ + subtree_ids = {target.pk} + level_ids = [target.pk] + while level_ids: + level_ids = list( + CompetencyCriteriaGroup.objects.filter(parent_id__in=level_ids).values_list("pk", flat=True) + ) + subtree_ids.update(level_ids) + # Evaluated so the locks are taken now. + list( + CompetencyCriteriaGroup.objects.select_for_update() + .filter(pk__in=subtree_ids.union(lineage_ids)) + .order_by("pk") + ) + criteria = list( + CompetencyCriterion.objects.select_for_update().filter(group_id__in=subtree_ids).order_by("pk") + ) + return subtree_ids, criteria + + +def _archive_competency_criteria_subtree( + target_group: CompetencyCriteriaGroup, + all_group_ids: set[int], + criteria: list[CompetencyCriterion], + user: UserType, +) -> None: + """Archive every group and criterion in the subtree, then cascade the archive above the target.""" + # The criteria are written without #674's per-criterion cascade: every group in the subtree is archived + # here, and the one upward walk below covers the target's ancestors. Never touches any ObjectTag. + for group in CompetencyCriteriaGroup.objects.filter(id__in=all_group_ids, archived=False): + _archive_group(group, user) + for criterion in criteria: + _write_criterion_archived(criterion, user) + if target_group.parent is not None: + _cascade_remove_from_group(target_group.parent, user) + + +def _subtree_has_learner_progress(group_ids: set[int], criterion_ids: list[int]) -> bool: + """Return whether a learner status row references any group in ``group_ids`` or any of ``criterion_ids``.""" + return ( + StudentCompetencyCriteriaGroupStatus.objects.filter(group_id__in=group_ids).exists() + or StudentCompetencyCriterionStatus.objects.filter(criterion_id__in=criterion_ids).exists() + ) + + +def _delete_orphaned_object_tags(candidate_ids: set[int]) -> None: + """ + Delete each ObjectTag in ``candidate_ids`` that no CompetencyCriterion references, and nothing else. + + ``candidate_ids`` are the ObjectTag ids of the criteria in a deleted group subtree. Must run + after those criteria are deleted, inside the transaction that holds the competency's Tag lock. + The candidate rows are locked last, in primary-key order. Inserting a criterion takes a foreign + key lock on its ObjectTag, so the reference check under this lock sees any criterion committed + for a candidate in the meantime, and that criterion keeps its ObjectTag. No other ObjectTag on + the object, in any taxonomy, is read or written. + """ + # Evaluated so the locks are taken now. + locked = list(ObjectTag.objects.select_for_update().filter(pk__in=candidate_ids).order_by("pk")) + still_referenced = set( + CompetencyCriterion.objects.filter(object_tag_id__in=candidate_ids).values_list("object_tag_id", flat=True) + ) + for object_tag in locked: + if object_tag.pk not in still_referenced: + object_tag.delete() + + +def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) -> GroupDeletionResult: + """ + Hard-delete the CompetencyCriteriaGroup subtree rooted at `group_id`, or archive it whole. + + 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 course + :func:`resolve_competency_criteria_group_course` returns for the target group; otherwise + Django's ``PermissionDenied`` is raised before anything is locked or changed. + + The subtree is the target group plus every descendant group and leaf CompetencyCriterion. + Hard-deleted if no learner status exists anywhere in it; archived in place, all-or-nothing, + if any does. Either way, each ancestor left with no live children is then hard-deleted or + archived, as for a criterion delete (ADR-0002 Decision 2). + + The whole operation runs in one transaction that locks the competency's Tag, then the target's + ancestors, the target and its descendants, then the subtree's criteria, then, on a hard delete, + the ObjectTags of those criteria that no other criterion references. Only those ObjectTags are + removed; no other tag on the object is touched. A concurrent create or criterion delete for the + same competency waits on the Tag lock. A learner-progress insert on any locked group or criterion + waits on that row's lock. At the READ COMMITTED isolation level, Django's default for MySQL and + PostgreSQL, the progress checks therefore cannot go stale, so no fallback or retry is needed. + + Raises CompetencyCriteriaGroup.DoesNotExist if no such group exists (including one deleted + concurrently), PermissionDenied if ``user`` may not tag the course's object, and + ValidationError (keyed "group_id") if `group_id` names a root CompetencyCriteriaGroup or a + group with no course-level group in its lineage. + """ + group = CompetencyCriteriaGroup.objects.select_related("tag__taxonomy").get(pk=group_id) + course = resolve_competency_criteria_group_course(group) + # A group's tag is always a competency tag, so its taxonomy is always set. + assert group.tag.taxonomy is not None + perm_obj = ObjectTagPermissionItem(taxonomy=group.tag.taxonomy, object_id=str(course.course_key)) + # 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() + tag_id = group.tag_id + with transaction.atomic(): + Tag.objects.select_for_update().get(pk=tag_id) + # Read under the Tag lock: a concurrent delete may have won, and DoesNotExist propagates. + target_group = CompetencyCriteriaGroup.objects.get(pk=group_id) + # Reading the lineage and descendants before the group locks is safe: a group's parent never + # changes, and every tree change for this competency takes the Tag lock held here. + subtree_ids, criteria = _lock_competency_criteria_subtree(target_group, _lineage_group_ids(target_group)) + criterion_ids = [criterion.pk for criterion in criteria] + has_progress = _subtree_has_learner_progress(subtree_ids, criterion_ids) + if has_progress: + _archive_competency_criteria_subtree(target_group, subtree_ids, criteria, user) + else: + parent_group = target_group.parent + target_group.delete() + _delete_orphaned_object_tags({criterion.object_tag_id for criterion in criteria}) + if parent_group is not None: + _cascade_remove_from_group(parent_group, user) + return GroupDeletionResult( + id=group_id, + archived=has_progress, + cascaded_group_count=len(subtree_ids) - 1, + cascaded_criteria_count=len(criteria), + ) diff --git a/src/openedx_learning/applets/cbe/data.py b/src/openedx_learning/applets/cbe/data.py index cc01fc7c0..ad72c3f2c 100644 --- a/src/openedx_learning/applets/cbe/data.py +++ b/src/openedx_learning/applets/cbe/data.py @@ -49,3 +49,21 @@ class CriterionDeletionResult: id: int archived: bool + + +@dataclass(frozen=True, kw_only=True, slots=True) +class GroupDeletionResult: + """ + The outcome of removing a CompetencyCriteriaGroup subtree: hard-deleted or archived, with counts. + + The counts cover the target subtree only: ``cascaded_group_count`` is the number of descendant + groups beneath the target and ``cascaded_criteria_count`` the number of criteria in the subtree. + + UNSTABLE: the criteria-authoring family is incomplete, so this shape may change without a + deprecation cycle. + """ + + id: int + archived: bool + cascaded_group_count: int + cascaded_criteria_count: int 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 f4acf99cf..ec8759cfb 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py @@ -42,4 +42,9 @@ views.CompetencyBlockedCoursesView.as_view(), name="blocked-courses", ), + path( + "competencies//criteria-groups//", + views.CompetencyCriteriaGroupDeleteView.as_view(), + name="group-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 6430077b3..8adcf28f7 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -27,6 +27,7 @@ CompetencyCriterionArchivedError, associate_competency_criterion, bulk_update_competency_criteria, + delete_or_archive_competency_criteria_group, delete_or_archive_competency_criterion, get_blocked_courses, get_competency_criteria_tree, @@ -351,3 +352,39 @@ def get(self, request, tag_id): rows = get_blocked_courses(tag, course_keys=query_params.validated_data.get("course_keys")) page = self.paginate_queryset(rows) return self.get_paginated_response(BlockedCourseSerializer(page, many=True).data) +class CompetencyCriteriaGroupDeleteView(generics.GenericAPIView): + """ + DELETE-only. Hard-delete a CompetencyCriteriaGroup subtree, or archive it if learner status exists anywhere in it. + + The view only resolves the group from the URL. Course resolution and the permission check happen in + :func:`delete_or_archive_competency_criteria_group`, so a refused user gets 403 and a root group gets 400. + """ + + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = [IsAuthenticated] + + def delete(self, request, *args, **kwargs): + """ + Resolve the group scoped to both path segments, then delegate to the public API, which checks permission. + + The lookup keeps archived groups, so a repeat DELETE on an archived subtree still returns 200. + """ + group = get_object_or_404( + CompetencyCriteriaGroup, pk=self.kwargs["group_id"], tag_id=self.kwargs["tag_id"], + ) + try: + result = delete_or_archive_competency_criteria_group(group.id, request.user) + except CompetencyCriteriaGroup.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, + "cascaded_group_count": result.cascaded_group_count, + "cascaded_criteria_count": result.cascaded_criteria_count, + }, + 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 586dc77e1..17a9f274e 100644 --- a/tests/openedx_learning/applets/cbe/test_criterion_removal.py +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -1,15 +1,19 @@ """Tests for delete_or_archive_competency_criterion() and its ancestor-cascade helper, _cascade_remove_from_group().""" from collections.abc import Iterator +from contextlib import contextmanager from datetime import datetime +from types import SimpleNamespace import pytest import rules from django.apps import apps from django.contrib.auth import get_user_model from django.core.exceptions import PermissionDenied +from django.db import transaction from rules.permissions import permissions as rule_permissions from openedx_learning.api import delete_or_archive_competency_criterion +from openedx_learning.applets.cbe import api as cbe_api from openedx_learning.models import ( CompetencyCriteriaGroup, CompetencyCriterion, @@ -450,6 +454,35 @@ def test_does_not_exist_for_an_unknown_criterion_id(user) -> None: delete_or_archive_competency_criterion(999999999, user) +def test_a_criterion_removed_with_its_group_before_the_lock_raises_does_not_exist( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, monkeypatch, +) -> None: + """ + A concurrent group delete that removes the criterion and its ancestors after the function's + pre-lock read, but before its Tag lock, surfaces as CompetencyCriterion.DoesNotExist. + + The race is simulated without threads: the ``transaction`` name the api module uses is swapped + for one whose ``atomic`` (the function's only use of it) first hard-deletes the group chain, + cascading to the criterion, and then opens the real transaction. That is the point between the + pre-lock read and the Tag lock. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + group = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(group, object_tag, default_rule_profile) + real_atomic = transaction.atomic + + @contextmanager + def delete_chain_then_atomic() -> Iterator[None]: + CompetencyCriteriaGroup.objects.filter(pk=root.pk).delete() + with real_atomic(): + yield + + monkeypatch.setattr(cbe_api, "transaction", SimpleNamespace(atomic=delete_chain_then_atomic)) + + with pytest.raises(CompetencyCriterion.DoesNotExist): + delete_or_archive_competency_criterion(criterion.id, user) + + # ================================================================================================= # The walk stops at archived groups # ================================================================================================= diff --git a/tests/openedx_learning/applets/cbe/test_group_removal.py b/tests/openedx_learning/applets/cbe/test_group_removal.py new file mode 100644 index 000000000..b1776a127 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_group_removal.py @@ -0,0 +1,622 @@ +"""Tests for delete_or_archive_competency_criteria_group(), which reuses _cascade_from_group() unchanged.""" +from collections.abc import Iterator +from datetime import datetime +from typing import Any + +import pytest +import rules +from django.apps import apps +from django.core.exceptions import PermissionDenied, ValidationError +from rules.permissions import permissions as rule_permissions + +from openedx_catalog.models import CourseRun +from openedx_learning.api import delete_or_archive_competency_criteria_group, resolve_competency_criteria_group_course +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + MasteryStatus, + StudentCompetencyCriteriaGroupStatus, + StudentCompetencyCriterionStatus, +) +from openedx_tagging.models import ObjectTag, Tag + +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) +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.""" + 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, and the reported counts +# ================================================================================================= + + +def test_leaf_group_with_no_descendants_or_criteria_hard_deletes_with_zero_counts( + user, tag: Tag, course_run: CourseRun, +) -> None: + """Deleting an empty, childless target group reports both cascade counts as zero.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.id == target.id + assert result.archived is False + assert result.cascaded_group_count == 0 + assert result.cascaded_criteria_count == 0 + assert not CompetencyCriteriaGroup.objects.filter(id=target.id).exists() + + +def test_multilevel_cascade_hard_delete_counts_the_descendant_group_and_both_criteria( + user, competency_taxonomy: CompetencyTaxonomy, tag: Tag, course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A target group with one child group holding two criteria reports exactly those counts.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=target) + object_tag_a = _make_object_tag(competency_taxonomy, tag, "obj-a") + object_tag_b = _make_object_tag(competency_taxonomy, tag, "obj-b") + criterion_a = _make_criterion(child, object_tag_a, default_rule_profile) + criterion_b = _make_criterion(child, object_tag_b, default_rule_profile) + + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.archived is False + assert result.cascaded_group_count == 1 + assert result.cascaded_criteria_count == 2 + assert not CompetencyCriteriaGroup.objects.filter(id=target.id).exists() + assert not CompetencyCriteriaGroup.objects.filter(id=child.id).exists() + assert not CompetencyCriterion.objects.filter(id__in=[criterion_a.id, criterion_b.id]).exists() + + +def test_archive_when_status_exists_at_a_buried_grandchild_criterion( + user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + A learner status on a criterion several levels below the target archives the whole subtree + -- every group and criterion in it -- with the cascade counts still reported accurately. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + mid = CompetencyCriteriaGroup.objects.create(tag=tag, parent=target) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=mid) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.archived is True + assert result.cascaded_group_count == 2 + assert result.cascaded_criteria_count == 1 + target.refresh_from_db() + mid.refresh_from_db() + leaf.refresh_from_db() + criterion.refresh_from_db() + assert target.archived is True + assert mid.archived is True + assert leaf.archived is True + assert criterion.archived is True + + +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 _latest_history_user(model_name: str, pk: int) -> Any: + """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 test_subtree_archive_records_the_acting_user_in_history( + user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """Archiving a subtree records the acting user on the newest historical row of each archived group and criterion.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=target) + criterion = _make_criterion(child, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + delete_or_archive_competency_criteria_group(target.id, user) + + for group in (target, child, root): + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", group.id) == user + assert _latest_history_user("HistoricalCompetencyCriterion", criterion.id) == user + + +def test_repeat_delete_on_an_archived_subtree_writes_no_history( + user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + A second delete of an already-archived subtree returns archived=True and writes no new historical + row for the target, its descendants, its criteria, or its ancestors. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=target) + criterion = _make_criterion(child, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + delete_or_archive_competency_criteria_group(target.id, user) + + def counts() -> tuple[int, ...]: + return ( + *(_history_count("HistoricalCompetencyCriteriaGroup", g.id) for g in (target, child, root)), + _history_count("HistoricalCompetencyCriterion", criterion.id), + ) + + before = counts() + + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.id == target.id + assert result.archived is True + target.refresh_from_db() + assert target.archived is True + assert counts() == before + + +# ================================================================================================= +# ObjectTag cleanup +# ================================================================================================= + + +def test_orphaned_object_tag_is_removed_on_hard_delete( + user, competency_taxonomy: CompetencyTaxonomy, tag: Tag, course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """An object_tag referenced only by a criterion inside the deleted subtree is untagged too.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + object_tag = _make_object_tag(competency_taxonomy, tag, "obj-only-in-subtree") + _make_criterion(target, object_tag, default_rule_profile) + + delete_or_archive_competency_criteria_group(target.id, user) + + assert not ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_shared_object_tag_survives_when_a_criterion_outside_the_subtree_still_references_it( + user, tag: Tag, course_run: CourseRun, 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 group's subtree leaves the other group, its + criterion, and the shared object_tag, all intact. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + sibling = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + _make_criterion(target, object_tag, default_rule_profile) + sibling_criterion = _make_criterion(sibling, object_tag, default_rule_profile) + + delete_or_archive_competency_criteria_group(target.id, user) + + assert ObjectTag.objects.filter(id=object_tag.id).exists() + assert CompetencyCriterion.objects.filter(id=sibling_criterion.id).exists() + + +def test_hard_delete_keeps_an_object_tag_still_referenced_outside_the_subtree( + user, competency_taxonomy: CompetencyTaxonomy, tag: Tag, course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + One object carries two competency tags of the same taxonomy. The tag only the deleted subtree + references is removed, and the tag a criterion outside the subtree references stays, with that criterion. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + sibling = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + tag_a = Tag.objects.create(taxonomy=competency_taxonomy, value="Tag A") + tag_b = Tag.objects.create(taxonomy=competency_taxonomy, value="Tag B") + object_tag_a = _make_object_tag(competency_taxonomy, tag_a, "obj-two-tags") + object_tag_b = _make_object_tag(competency_taxonomy, tag_b, "obj-two-tags") + _make_criterion(target, object_tag_a, default_rule_profile) + sibling_criterion = _make_criterion(sibling, object_tag_b, default_rule_profile) + + delete_or_archive_competency_criteria_group(target.id, user) + + assert not ObjectTag.objects.filter(id=object_tag_a.id).exists() + assert ObjectTag.objects.filter(id=object_tag_b.id).exists() + assert CompetencyCriterion.objects.filter(id=sibling_criterion.id).exists() + + +def test_untag_with_the_competency_taxonomy_disabled_removes_only_the_orphaned_object_tag( + user, competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + With the taxonomy disabled the tagging API hides its tags, but the delete removes only the + subtree's orphaned 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. + """ + assert object_tag.tag is not None + tag = object_tag.tag + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + criterion = _make_criterion(target, object_tag, default_rule_profile) + 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) + 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_criteria_group(target.id, user) + finally: + rules.set_perm(CHANGE_OBJECTTAG_TAXONOMY, original) + + assert result.archived is False + assert not CompetencyCriteriaGroup.objects.filter(id=target.id).exists() + 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, competency_taxonomy: CompetencyTaxonomy, tag: Tag, course_run: CourseRun, + 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. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + _make_criterion(target, object_tag, default_rule_profile) + 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) + 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_criteria_group(target.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_hard_delete_removes_an_object_tag_shared_by_two_criteria_in_the_subtree_once( + user, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """Two criteria in the deleted subtree share one orphaned ObjectTag: the delete succeeds and the row is gone.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=target) + _make_criterion(target, object_tag, default_rule_profile) + _make_criterion(child, object_tag, default_rule_profile) + + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.archived is False + assert not CompetencyCriterion.objects.filter(group_id__in=[target.id, child.id]).exists() + assert not ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_archive_when_status_exists_only_on_the_target_group( + user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """A group-level status on the target alone, with no criterion status, archives the whole subtree.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=target) + criterion = _make_criterion(child, object_tag, default_rule_profile) + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=target, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.archived is True + for node in (target, child, criterion): + node.refresh_from_db() + assert node.archived is True + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_archive_when_status_exists_only_on_a_descendant_group( + user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """A group-level status on a descendant alone, with no criterion status, archives the whole subtree.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=target) + criterion = _make_criterion(target, object_tag, default_rule_profile) + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=child, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.archived is True + for node in (target, child, criterion): + node.refresh_from_db() + assert node.archived is True + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_object_tag_is_untouched_on_archive( + user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """Archiving a subtree never calls tag_object(): the object_tag row is left exactly as it was.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + criterion = _make_criterion(target, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.archived is True + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +# ================================================================================================= +# The ancestor cascade, above the target +# ================================================================================================= + + +def test_ancestor_cascade_collapses_the_chain_on_hard_delete( + user, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Hard-deleting a subtree that was its course-level ancestor's only child collapses that + ancestor and the root above it too, via _cascade_from_group(). + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + _make_criterion(target, object_tag, default_rule_profile) + + delete_or_archive_competency_criteria_group(target.id, user) + + assert not CompetencyCriteriaGroup.objects.filter(id=target.id).exists() + assert not CompetencyCriteriaGroup.objects.filter(id=course_level.id).exists() + assert not CompetencyCriteriaGroup.objects.filter(id=root.id).exists() + + +def test_ancestor_cascade_archives_the_chain_when_the_target_carries_status( + user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """Archiving a subtree that was its ancestor's only live child archives that ancestor too, up to the root.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + criterion = _make_criterion(target, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + delete_or_archive_competency_criteria_group(target.id, user) + + course_level.refresh_from_db() + root.refresh_from_db() + assert course_level.archived is True + assert root.archived is True + assert CompetencyCriteriaGroup.objects.filter(id=course_level.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() + + +def test_cascade_stops_at_an_ancestor_with_a_surviving_sibling_group( + user, *, competency_taxonomy: CompetencyTaxonomy, tag: Tag, course_run: CourseRun, + object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A sibling group under the same course-level ancestor, with its own live criterion, keeps that + ancestor from being touched, even though the target's own subtree is fully removed. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + _make_criterion(target, object_tag, default_rule_profile) + sibling = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + sibling_object_tag = _make_object_tag(competency_taxonomy, tag, "obj-sibling") + sibling_criterion = _make_criterion(sibling, sibling_object_tag, default_rule_profile) + + delete_or_archive_competency_criteria_group(target.id, user) + + assert not CompetencyCriteriaGroup.objects.filter(id=target.id).exists() + course_level.refresh_from_db() + root.refresh_from_db() + sibling.refresh_from_db() + assert course_level.archived is False + assert root.archived is False + assert sibling.archived is False + assert CompetencyCriteriaGroup.objects.filter(id=course_level.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=sibling.id).exists() + assert CompetencyCriterion.objects.filter(id=sibling_criterion.id).exists() + + +def test_archive_cascade_stops_at_an_ancestor_with_a_surviving_sibling_group( + user, *, competency_taxonomy: CompetencyTaxonomy, tag: Tag, course_run: CourseRun, + object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + When the target subtree is archived because of learner progress, a live sibling group under the + same parent keeps the parent and the root from being archived. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + criterion = _make_criterion(target, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + sibling = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + sibling_object_tag = _make_object_tag(competency_taxonomy, tag, "obj-sibling") + _make_criterion(sibling, sibling_object_tag, default_rule_profile) + + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.archived is True + for group in (target, course_level, root, sibling): + group.refresh_from_db() + assert target.archived is True + assert course_level.archived is False + assert root.archived is False + assert sibling.archived is False + + +# ================================================================================================= +# Root-group rejection, permission-course resolution, and not-found +# ================================================================================================= + + +def test_root_group_rejection_raises_before_any_write( + user, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A root target_group is rejected with ValidationError before any write: the root, its child, + and its criterion are all untouched afterward. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + child = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + criterion = _make_criterion(child, object_tag, default_rule_profile) + + with pytest.raises(ValidationError): + delete_or_archive_competency_criteria_group(root.id, user) + + root.refresh_from_db() + child.refresh_from_db() + criterion.refresh_from_db() + assert root.archived is False + assert child.archived is False + assert criterion.archived is False + assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=child.id).exists() + assert CompetencyCriterion.objects.filter(id=criterion.id).exists() + + +def test_permission_course_rejects_a_root_group(tag: Tag) -> None: + """A root group has no single course to check permission against, so it is rejected.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + + with pytest.raises(ValidationError) as exc_info: + resolve_competency_criteria_group_course(root) + + assert "group_id" in exc_info.value.message_dict + + +def test_permission_course_is_the_targets_own_course_for_a_course_level_group(tag: Tag, course_run: CourseRun) -> None: + """A course-level target group (its own course_id set) resolves to that course.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + + assert resolve_competency_criteria_group_course(course_level) == course_run + + +def test_permission_course_is_the_parents_course_for_a_non_course_level_target(tag: Tag, course_run: CourseRun) -> None: + """A target group with no course_id of its own (one level under a course-level group) resolves to its parent's.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + + assert resolve_competency_criteria_group_course(leaf) == course_run + + +@pytest.mark.parametrize("depth", [2, 3]) +def test_permission_course_is_the_nearest_course_level_ancestors_course_at_any_depth( + tag: Tag, course_run: CourseRun, depth: int, +) -> None: + """A target nested `depth` levels below its course-level group resolves to that group's course.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + group = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + for _ in range(depth): + group = CompetencyCriteriaGroup.objects.create(tag=tag, parent=group) + + assert resolve_competency_criteria_group_course(group) == course_run + + +def test_permission_course_rejects_a_group_with_no_course_level_group_in_its_lineage(tag: Tag) -> None: + """A non-root group with no course anywhere above it is invalid data, rejected as a group_id error.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + middle = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=middle) + + with pytest.raises(ValidationError) as exc_info: + resolve_competency_criteria_group_course(target) + + assert "group_id" in exc_info.value.message_dict + + +def test_does_not_exist_for_an_unknown_group_id(user) -> None: + """A group_id with no matching row raises CompetencyCriteriaGroup.DoesNotExist.""" + with pytest.raises(CompetencyCriteriaGroup.DoesNotExist): + delete_or_archive_competency_criteria_group(999999999, user) + + +def test_a_user_without_permission_is_refused_and_nothing_changes( + user, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A user who may not tag the course is refused, and the subtree, its criteria and ObjectTags survive.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + child = CompetencyCriteriaGroup.objects.create(tag=tag, parent=target) + criterion = _make_criterion(child, object_tag, default_rule_profile) + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, lambda _user, _object_id: False) + + with pytest.raises(PermissionDenied): + delete_or_archive_competency_criteria_group(target.id, user) + + for group in (root, target, child): + group.refresh_from_db() + assert group.archived is False + criterion.refresh_from_db() + assert criterion.archived is False + assert ObjectTag.objects.filter(id=object_tag.id).exists() diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index c71529e64..d5b7adaba 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -6,10 +6,12 @@ """ from datetime import datetime from typing import Any, Generator +from unittest import mock from urllib.parse import urlencode 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 @@ -86,6 +88,11 @@ def criterion_delete_url(tag_id: int, criterion_id: int) -> str: return reverse("cbe:criterion-delete", kwargs={"tag_id": tag_id, "criterion_id": criterion_id}) +def group_delete_url(tag_id: int, group_id: int) -> str: + """Return the delete-group endpoint's path for `tag_id`/`group_id`, resolved through the URL name.""" + return reverse("cbe:group-delete", kwargs={"tag_id": tag_id, "group_id": group_id}) + + # What migration 0005 seeds the system default with. Asserted verbatim rather than imported, so # that a change to the seed surfaces here as a failing contract instead of passing silently. SEEDED_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} @@ -2599,3 +2606,293 @@ def test_a_competency_tag_deleted_while_the_request_waits_for_its_lock_is_404( ) assert response.status_code == status.HTTP_404_NOT_FOUND + + +# ============================================================================================== +# CompetencyCriteriaGroupDeleteView (#675) +# ============================================================================================== + + +def test_group_hard_delete_200s_then_repeat_delete_404s( + user_client: APIClient, tag: Tag, course_run: CourseRun, +) -> None: + """ + Deleting an empty leaf group 200s, hard-deletes it, and reports zero cascade counts; a + repeat DELETE then 404s. + """ + leaf = _create_leaf_group(tag, course_run) + url = group_delete_url(tag.id, leaf.id) + + response = user_client.delete(url) + + assert response.status_code == status.HTTP_200_OK + assert response.data == { + "id": leaf.id, "archived": False, "cascaded_group_count": 0, "cascaded_criteria_count": 0, + } + assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + + repeat = user_client.delete(url) + + assert repeat.status_code == status.HTTP_404_NOT_FOUND + + +def test_group_cascade_hard_delete_200s_then_child_group_id_404s_on_subsequent_create( + user_client: APIClient, tag: Tag, course_run: CourseRun, +) -> None: + """ + Hard-deleting a course-level group cascade-deletes its child leaf group too, and a later + create-criterion POST naming that now-gone child group_id 404s rather than resolving. + """ + leaf = _create_leaf_group(tag, course_run) + course_level = leaf.parent + assert course_level is not None + + response = user_client.delete(group_delete_url(tag.id, course_level.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data["archived"] is False + assert response.data["cascaded_group_count"] == 1 + assert response.data["cascaded_criteria_count"] == 0 + 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, "p1"), "group_id": leaf.id}, + format="json", + ) + + assert create_response.status_code == status.HTTP_404_NOT_FOUND + + +def test_group_archive_200s_with_archived_true( + user_client: APIClient, *, user, tag: Tag, course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + Deleting a group whose criterion a learner status row references 200s, archives the + subtree, and reports archived=true. + """ + 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, + ) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + response = user_client.delete(group_delete_url(tag.id, leaf.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data == { + "id": leaf.id, "archived": True, "cascaded_group_count": 0, "cascaded_criteria_count": 1, + } + leaf.refresh_from_db() + assert leaf.archived is True + + +def test_group_repeat_delete_on_an_archived_group_is_idempotent_200( + user_client: APIClient, *, user, tag: Tag, course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """A second DELETE against an already-archived group subtree still 200s with the same body.""" + 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, + ) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + url = group_delete_url(tag.id, leaf.id) + user_client.delete(url) + + response = user_client.delete(url) + + assert response.status_code == status.HTTP_200_OK + assert response.data == { + "id": leaf.id, "archived": True, "cascaded_group_count": 0, "cascaded_criteria_count": 1, + } + + +def test_group_unknown_group_id_404s(user_client: APIClient, tag: Tag) -> None: + """A group_id with no matching row 404s.""" + response = user_client.delete(group_delete_url(tag.id, 999999)) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_group_tag_id_mismatch_404s( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, tag: Tag, course_run: CourseRun, +) -> None: + """A group that exists, but under a different competency tag than the URL names, 404s.""" + leaf = _create_leaf_group(tag, course_run) + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Other Competency") + + response = user_client.delete(group_delete_url(other_tag.id, leaf.id)) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_group_root_target_is_400(user_client: APIClient, tag: Tag, course_run: CourseRun) -> None: + """A root group_id is rejected with a 400, not attempted as a deletable subtree.""" + leaf = _create_leaf_group(tag, course_run) + course_level = leaf.parent + assert course_level is not None + root = course_level.parent + assert root is not None + + response = user_client.delete(group_delete_url(tag.id, root.id)) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "group_id" in response.data + + +def test_group_delete_no_permission_is_403( + user_client: APIClient, tag: Tag, organization: Organization, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A caller without tagging permission on the subtree's course gets a 403, and the subtree survives.""" + unauthorized_course_run = make_course_run(organization, f"{UNAUTHORIZED_MARKER}100", "Fall2026") + leaf = _create_leaf_group(tag, unauthorized_course_run) + course_level = leaf.parent + assert course_level is not None + criterion = CompetencyCriterion.objects.create( + group=leaf, object_tag=object_tag, rule_profile=default_rule_profile, + ) + + response = user_client.delete(group_delete_url(tag.id, leaf.id)) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert CompetencyCriteriaGroup.objects.filter(id__in=[leaf.id, course_level.id], archived=False).count() == 2 + assert CompetencyCriterion.objects.filter(id=criterion.id, archived=False).exists() + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_group_delete_of_a_group_gone_by_the_time_of_the_call_is_404( + user_client: APIClient, tag: Tag, course_run: CourseRun, +) -> None: + """A group removed between the view's lookup and the API call is a 404, not a 500.""" + leaf = _create_leaf_group(tag, course_run) + + with mock.patch( + "openedx_learning.applets.cbe.rest_api.v1.views.delete_or_archive_competency_criteria_group", + side_effect=CompetencyCriteriaGroup.DoesNotExist, + ): + response = user_client.delete(group_delete_url(tag.id, leaf.id)) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_group_delete_maps_a_validation_error_from_the_api_to_400( + user_client: APIClient, tag: Tag, course_run: CourseRun, +) -> None: + """A Django ValidationError raised by the API function becomes a 400 keyed by its field.""" + leaf = _create_leaf_group(tag, course_run) + + with mock.patch( + "openedx_learning.applets.cbe.rest_api.v1.views.delete_or_archive_competency_criteria_group", + side_effect=ValidationError({"group_id": "x"}), + ): + response = user_client.delete(group_delete_url(tag.id, leaf.id)) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "group_id" in response.json() + + +@pytest.fixture(name="checked_object_ids") +def _checked_object_ids(_allow_object_level_tagging: None) -> Generator[list[str], None, None]: + """Record every object_id the object-level tagging predicate is asked about, then defer to it.""" + perm = "oel_tagging.change_objecttag_objectid" + allowing = rules.permissions.permissions[perm] + seen: list[str] = [] + + def _recording_predicate(user, object_id: str) -> bool: + seen.append(object_id) + return allowing.test(user, object_id) + + rules.set_perm(perm, _recording_predicate) + yield seen + rules.set_perm(perm, allowing) + + +def test_group_delete_checks_permission_against_a_course_level_groups_own_course( + user_client: APIClient, tag: Tag, course_run: CourseRun, checked_object_ids: list[str], +) -> None: + """A course-level target group is checked against its own course's key.""" + leaf = _create_leaf_group(tag, course_run) + course_level = leaf.parent + assert course_level is not None + + response = user_client.delete(group_delete_url(tag.id, course_level.id)) + + assert response.status_code == status.HTTP_200_OK + assert checked_object_ids == [str(course_run.course_key)] + + +def test_group_delete_checks_permission_against_the_parents_course_for_a_leaf_group( + user_client: APIClient, tag: Tag, course_run: CourseRun, checked_object_ids: list[str], +) -> None: + """A leaf target group, which has no course of its own, is checked against its parent's course key.""" + leaf = _create_leaf_group(tag, course_run) + + response = user_client.delete(group_delete_url(tag.id, leaf.id)) + + assert response.status_code == status.HTTP_200_OK + assert checked_object_ids == [str(course_run.course_key)] + + +def test_group_root_target_is_400_before_any_permission_check_or_write( + user_client: APIClient, tag: Tag, course_run: CourseRun, checked_object_ids: list[str], +) -> None: + """A root group_id is rejected before the permission predicate runs, and nothing under it changes.""" + leaf = _create_leaf_group(tag, course_run) + course_level = leaf.parent + assert course_level is not None + root = course_level.parent + assert root is not None + + response = user_client.delete(group_delete_url(tag.id, root.id)) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not checked_object_ids + assert set( + CompetencyCriteriaGroup.objects.filter(archived=False).values_list("id", flat=True) + ) == {root.id, course_level.id, leaf.id} + + +@pytest.mark.parametrize("depth", [2, 3]) +def test_group_delete_checks_permission_against_the_course_level_ancestor_at_any_depth( + user_client: APIClient, tag: Tag, course_run: CourseRun, checked_object_ids: list[str], depth: int, +) -> None: + """A target nested `depth` levels below its course-level group is checked against that course and deleted.""" + course_level = _create_leaf_group(tag, course_run).parent + assert course_level is not None + target = course_level + for _ in range(depth): + target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=target) + + response = user_client.delete(group_delete_url(tag.id, target.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data["archived"] is False + assert checked_object_ids == [str(course_run.course_key)] + assert not CompetencyCriteriaGroup.objects.filter(id=target.id).exists() + + +def test_group_with_no_course_level_group_in_its_lineage_is_400_before_any_permission_check_or_write( + user_client: APIClient, tag: Tag, checked_object_ids: list[str], +) -> None: + """A non-root group with no course above it is invalid data: a 400, with no permission check and no write.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + middle = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=middle) + + response = user_client.delete(group_delete_url(tag.id, target.id)) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "group_id" in response.data + assert not checked_object_ids + assert set( + CompetencyCriteriaGroup.objects.filter(archived=False).values_list("id", flat=True) + ) == {root.id, middle.id, target.id} From 086062d1f5ebbbcbe9c06e6c9546183065ec9dbe Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Thu, 29 Oct 2026 20:32:47 +0300 Subject: [PATCH 02/10] fix: serve the group DELETE from the group detail view (#675) The group detail path already belongs to CompetencyCriteriaGroupDetailView (PATCH, #760), so a second view registered on the same path never received a request and every DELETE was a 405. DELETE is now a method of that view, and the separate view and its URL name are gone. DELETE does not call check_object_permissions(), so the view's CompetencyReadPermission still applies to PATCH only, and the delete_or_archive_competency_criteria_group permission check is unchanged. Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 6 +- .../applets/cbe/rest_api/v1/urls.py | 5 -- .../applets/cbe/rest_api/v1/views.py | 88 +++++++++---------- .../applets/cbe/test_views.py | 18 ++-- 4 files changed, 56 insertions(+), 61 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 01aad5a27..c65242fb4 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -1311,9 +1311,9 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - PostgreSQL, the progress checks therefore cannot go stale, so no fallback or retry is needed. Raises CompetencyCriteriaGroup.DoesNotExist if no such group exists (including one deleted - concurrently), PermissionDenied if ``user`` may not tag the course's object, and - ValidationError (keyed "group_id") if `group_id` names a root CompetencyCriteriaGroup or a - group with no course-level group in its lineage. + concurrently), Tag.DoesNotExist if its competency tag was deleted concurrently, PermissionDenied + if ``user`` may not tag the course's object, and ValidationError (keyed "group_id") if `group_id` + names a root CompetencyCriteriaGroup or a group with no course-level group in its lineage. """ group = CompetencyCriteriaGroup.objects.select_related("tag__taxonomy").get(pk=group_id) course = resolve_competency_criteria_group_course(group) 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 ec8759cfb..f4acf99cf 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py @@ -42,9 +42,4 @@ views.CompetencyBlockedCoursesView.as_view(), name="blocked-courses", ), - path( - "competencies//criteria-groups//", - views.CompetencyCriteriaGroupDeleteView.as_view(), - name="group-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 8adcf28f7..0167c9fdd 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -231,25 +231,33 @@ def get(self, request, tag_id): class CompetencyCriteriaGroupDetailView(generics.GenericAPIView): """ - PATCH-only. Change how one CompetencyCriteriaGroup combines the criteria and groups beneath it. + PATCH or DELETE one CompetencyCriteriaGroup. - **Example Request** + PATCH changes how the group combines the criteria and groups beneath it. DELETE hard-deletes + the group's subtree, or archives it if learner status exists anywhere in it. + + **Example Requests** PATCH api/cbe/v1/competencies//criteria-groups// ``{"logic_operator": "AND"}`` - The response is the group as stored, in the criteria-tree endpoint's shape. ``logic_operator`` + DELETE api/cbe/v1/competencies//criteria-groups// + + PATCH responds with the group as stored, in the criteria-tree endpoint's shape. ``logic_operator`` is the only editable field. The body may also repeat any other field the group's representation carries, so a client can send back what it read, as long as each one matches what is stored. - A thin adapter: :func:`update_competency_criteria_group` makes every other refusal, including - the edit permission check, so an in-process caller is refused in the same cases. The body's - fixed fields are compared here, before that function runs, because only a REST body carries - them. Comparing them reveals what's stored, so a caller who can't read the competency's tree is - refused with 403 before the group is looked up. + For PATCH, the view is a thin adapter: :func:`update_competency_criteria_group` makes every other + refusal, including the edit permission check, so an in-process caller is refused in the same + cases. The body's fixed fields are compared here, before that function runs, because only a REST + body carries them. Comparing them reveals what's stored, so a caller who can't read the + competency's tree is refused with 403 before the group is looked up. + + DELETE only resolves the group from the URL. Course resolution and the permission check happen in + :func:`delete_or_archive_competency_criteria_group`, so a refused user gets 403 and a root group gets 400. """ serializer_class = CompetencyCriteriaGroupUpdateSerializer - http_method_names = ["patch"] + http_method_names = ["patch", "delete"] authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) permission_classes = [IsAuthenticated, CompetencyReadPermission] @@ -280,6 +288,32 @@ def patch(self, request, tag_id: int, group_id: int): raise CompetencyCriteriaConflict(str(exc)) from exc return Response(CompetencyCriteriaGroupSerializer(group).data, status=status.HTTP_200_OK) + def delete(self, request, *args, **kwargs): + """ + Resolve the group scoped to both path segments, then delegate to the public API, which checks permission. + + The lookup keeps archived groups, so a repeat DELETE on an archived subtree still returns 200. + """ + group = get_object_or_404( + CompetencyCriteriaGroup, pk=self.kwargs["group_id"], tag_id=self.kwargs["tag_id"], + ) + try: + result = delete_or_archive_competency_criteria_group(group.id, request.user) + except (CompetencyCriteriaGroup.DoesNotExist, Tag.DoesNotExist) as exc: + # Removed between the lookup above and the API call, or while the call waited for the Tag lock. + 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, + "cascaded_group_count": result.cascaded_group_count, + "cascaded_criteria_count": result.cascaded_criteria_count, + }, + status=status.HTTP_200_OK, + ) + class CompetencyCriterionDeleteView(generics.GenericAPIView): """ @@ -352,39 +386,3 @@ def get(self, request, tag_id): rows = get_blocked_courses(tag, course_keys=query_params.validated_data.get("course_keys")) page = self.paginate_queryset(rows) return self.get_paginated_response(BlockedCourseSerializer(page, many=True).data) -class CompetencyCriteriaGroupDeleteView(generics.GenericAPIView): - """ - DELETE-only. Hard-delete a CompetencyCriteriaGroup subtree, or archive it if learner status exists anywhere in it. - - The view only resolves the group from the URL. Course resolution and the permission check happen in - :func:`delete_or_archive_competency_criteria_group`, so a refused user gets 403 and a root group gets 400. - """ - - authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) - permission_classes = [IsAuthenticated] - - def delete(self, request, *args, **kwargs): - """ - Resolve the group scoped to both path segments, then delegate to the public API, which checks permission. - - The lookup keeps archived groups, so a repeat DELETE on an archived subtree still returns 200. - """ - group = get_object_or_404( - CompetencyCriteriaGroup, pk=self.kwargs["group_id"], tag_id=self.kwargs["tag_id"], - ) - try: - result = delete_or_archive_competency_criteria_group(group.id, request.user) - except CompetencyCriteriaGroup.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, - "cascaded_group_count": result.cascaded_group_count, - "cascaded_criteria_count": result.cascaded_criteria_count, - }, - 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 d5b7adaba..f59b81517 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -89,8 +89,8 @@ def criterion_delete_url(tag_id: int, criterion_id: int) -> str: def group_delete_url(tag_id: int, group_id: int) -> str: - """Return the delete-group endpoint's path for `tag_id`/`group_id`, resolved through the URL name.""" - return reverse("cbe:group-delete", kwargs={"tag_id": tag_id, "group_id": group_id}) + """Return the group detail endpoint's path for `tag_id`/`group_id`, which DELETE is sent to.""" + return reverse("cbe:criteria-group-detail", kwargs={"tag_id": tag_id, "group_id": group_id}) # What migration 0005 seeds the system default with. Asserted verbatim rather than imported, so @@ -2034,11 +2034,11 @@ def test_update_group_from_an_unidentified_caller_is_401( assert response.status_code == status.HTTP_401_UNAUTHORIZED -@pytest.mark.parametrize("method", ["get", "put", "post", "delete"]) -def test_group_detail_offers_only_patch( +@pytest.mark.parametrize("method", ["get", "put", "post"]) +def test_group_detail_offers_only_patch_and_delete( method: str, user_client: APIClient, tag: Tag, leaf: CompetencyCriteriaGroup, ) -> None: - """Every method other than PATCH, PUT included, is refused as not allowed.""" + """Every method other than PATCH and DELETE, PUT included, is refused as not allowed.""" response = getattr(user_client, method)(group_detail_url(tag.id, leaf.id), {}, format="json") assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED @@ -2580,7 +2580,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", "update a group"]) +@pytest.mark.parametrize("endpoint", ["delete a criterion", "bulk update criteria", "update a group", "delete 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, @@ -2600,16 +2600,18 @@ def test_a_competency_tag_deleted_while_the_request_waits_for_its_lock_is_404( 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: + elif endpoint == "update a group": response = user_client.patch( group_detail_url(leaf.tag_id, leaf.id), {"logic_operator": LogicOperator.AND}, format="json", ) + else: + response = user_client.delete(group_delete_url(leaf.tag_id, leaf.id)) assert response.status_code == status.HTTP_404_NOT_FOUND # ============================================================================================== -# CompetencyCriteriaGroupDeleteView (#675) +# CompetencyCriteriaGroupDetailView DELETE (#675) # ============================================================================================== From 99ce8209d868203dcea2b005032c5e82228d7785 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Thu, 29 Oct 2026 20:33:52 +0300 Subject: [PATCH 03/10] fix: lock only the tag when deleting a group subtree (#675) Drop the ancestor-group, subtree, criterion and ObjectTag row locks from delete_or_archive_competency_criteria_group, matching the criterion delete. The subtree is still read inside the transaction after the competency's Tag lock, the same lock a create or a criterion delete for that competency takes first. Only the ObjectTags no remaining criterion references are deleted, as before. Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 59 ++++++------------------- 1 file changed, 14 insertions(+), 45 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index c65242fb4..de6f21c64 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -1093,16 +1093,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. @@ -1211,15 +1201,12 @@ def resolve_competency_criteria_group_course(group: CompetencyCriteriaGroup) -> ) -def _lock_competency_criteria_subtree( - target: CompetencyCriteriaGroup, lineage_ids: list[int], -) -> tuple[set[int], list[CompetencyCriterion]]: +def _competency_criteria_subtree(target: CompetencyCriteriaGroup) -> tuple[set[int], list[CompetencyCriterion]]: """ - Lock the groups involved in removing ``target``, then its subtree's criteria, and return both. + Return the ids of ``target`` and its descendant groups (found level by level), and the subtree's criteria. - Takes one ``select_for_update()`` over the target's ancestors, the target, and its descendants - (found level by level), ordered by primary key so concurrent writers cannot deadlock. Must run - inside a transaction that already holds the competency's Tag lock. + Must run inside a transaction that already holds the competency's Tag lock, so no tree change + for that competency can land between this read and the delete or archive that follows it. """ subtree_ids = {target.pk} level_ids = [target.pk] @@ -1228,15 +1215,7 @@ def _lock_competency_criteria_subtree( CompetencyCriteriaGroup.objects.filter(parent_id__in=level_ids).values_list("pk", flat=True) ) subtree_ids.update(level_ids) - # Evaluated so the locks are taken now. - list( - CompetencyCriteriaGroup.objects.select_for_update() - .filter(pk__in=subtree_ids.union(lineage_ids)) - .order_by("pk") - ) - criteria = list( - CompetencyCriterion.objects.select_for_update().filter(group_id__in=subtree_ids).order_by("pk") - ) + criteria = list(CompetencyCriterion.objects.filter(group_id__in=subtree_ids).order_by("pk")) return subtree_ids, criteria @@ -1271,19 +1250,12 @@ def _delete_orphaned_object_tags(candidate_ids: set[int]) -> None: ``candidate_ids`` are the ObjectTag ids of the criteria in a deleted group subtree. Must run after those criteria are deleted, inside the transaction that holds the competency's Tag lock. - The candidate rows are locked last, in primary-key order. Inserting a criterion takes a foreign - key lock on its ObjectTag, so the reference check under this lock sees any criterion committed - for a candidate in the meantime, and that criterion keeps its ObjectTag. No other ObjectTag on - the object, in any taxonomy, is read or written. + No other ObjectTag on the object, in any taxonomy, is read or written. """ - # Evaluated so the locks are taken now. - locked = list(ObjectTag.objects.select_for_update().filter(pk__in=candidate_ids).order_by("pk")) still_referenced = set( CompetencyCriterion.objects.filter(object_tag_id__in=candidate_ids).values_list("object_tag_id", flat=True) ) - for object_tag in locked: - if object_tag.pk not in still_referenced: - object_tag.delete() + ObjectTag.objects.filter(pk__in=candidate_ids - still_referenced).delete() def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) -> GroupDeletionResult: @@ -1302,13 +1274,12 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - if any does. Either way, each ancestor left with no live children is then hard-deleted or archived, as for a criterion delete (ADR-0002 Decision 2). - The whole operation runs in one transaction that locks the competency's Tag, then the target's - ancestors, the target and its descendants, then the subtree's criteria, then, on a hard delete, - the ObjectTags of those criteria that no other criterion references. Only those ObjectTags are - removed; no other tag on the object is touched. A concurrent create or criterion delete for the - same competency waits on the Tag lock. A learner-progress insert on any locked group or criterion - waits on that row's lock. At the READ COMMITTED isolation level, Django's default for MySQL and - PostgreSQL, the progress checks therefore cannot go stale, so no fallback or retry is needed. + On a hard delete, only the ObjectTags of the subtree's criteria that no other criterion + references are removed; no other tag on the object is touched. + + The whole operation runs in one transaction that locks only the competency's Tag, the same lock + a create or a criterion delete for that competency takes first, so those wait until this call + commits. Raises CompetencyCriteriaGroup.DoesNotExist if no such group exists (including one deleted concurrently), Tag.DoesNotExist if its competency tag was deleted concurrently, PermissionDenied @@ -1329,9 +1300,7 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - Tag.objects.select_for_update().get(pk=tag_id) # Read under the Tag lock: a concurrent delete may have won, and DoesNotExist propagates. target_group = CompetencyCriteriaGroup.objects.get(pk=group_id) - # Reading the lineage and descendants before the group locks is safe: a group's parent never - # changes, and every tree change for this competency takes the Tag lock held here. - subtree_ids, criteria = _lock_competency_criteria_subtree(target_group, _lineage_group_ids(target_group)) + subtree_ids, criteria = _competency_criteria_subtree(target_group) criterion_ids = [criterion.pk for criterion in criteria] has_progress = _subtree_has_learner_progress(subtree_ids, criterion_ids) if has_progress: From 21dded48f9cfc9b2e6f405754e6e1febd51a4c00 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Sat, 7 Nov 2026 20:19:04 +0300 Subject: [PATCH 04/10] fix: archive instead of delete when a learner status lands mid-delete (#675) A learner status written after the progress check, but before the hard delete, makes the delete fail on the status table's PROTECT foreign key. delete_or_archive_competency_criteria_group() now runs the hard delete in a savepoint. On IntegrityError the savepoint rolls back and the progress check runs again. If it finds a status in the target subtree, the call archives the subtree instead, which is what it would have done had the first check seen the status. If the second check finds none, the failure came from outside the subtree, for example a status that reached an ancestor group during the cascade, or an unrelated integrity error. Archiving would not fix that, so the original exception is re-raised with nothing changed. The group DELETE view answers it with a 409 that tells the caller to retry. Co-Authored-By: Claude Opus 5.5 Co-Authored-By: Claude Fable 5 --- src/openedx_learning/applets/cbe/api.py | 44 ++++-- .../applets/cbe/rest_api/v1/views.py | 7 + .../applets/cbe/test_group_removal.py | 145 ++++++++++++++++++ .../applets/cbe/test_views.py | 23 +++ 4 files changed, 207 insertions(+), 12 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index de6f21c64..25c83237b 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -10,7 +10,7 @@ from typing import Any, Iterable, Sequence, cast from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError -from django.db import connection, transaction +from django.db import IntegrityError, connection, transaction from django.db.models import F, Q, QuerySet from django.db.transaction import TransactionManagementError from django.utils.translation import gettext_lazy as _ @@ -1281,10 +1281,17 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - a create or a criterion delete for that competency takes first, so those wait until this call commits. + A learner status written concurrently, after this call's progress check, makes the call + archive the subtree instead of deleting it, provided a second progress check finds a status on + the subtree. If it finds none, the failure came from outside the subtree, such as a status that + reached an ancestor group during the cascade, and the original ``IntegrityError`` is re-raised + with nothing changed. A caller may retry: the retry re-checks, then deletes or archives. + Raises CompetencyCriteriaGroup.DoesNotExist if no such group exists (including one deleted concurrently), Tag.DoesNotExist if its competency tag was deleted concurrently, PermissionDenied - if ``user`` may not tag the course's object, and ValidationError (keyed "group_id") if `group_id` - names a root CompetencyCriteriaGroup or a group with no course-level group in its lineage. + if ``user`` may not tag the course's object, ValidationError (keyed "group_id") if `group_id` + names a root CompetencyCriteriaGroup or a group with no course-level group in its lineage, and + ``IntegrityError`` when the delete races with a write outside the target subtree. """ group = CompetencyCriteriaGroup.objects.select_related("tag__taxonomy").get(pk=group_id) course = resolve_competency_criteria_group_course(group) @@ -1302,18 +1309,31 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - target_group = CompetencyCriteriaGroup.objects.get(pk=group_id) subtree_ids, criteria = _competency_criteria_subtree(target_group) criterion_ids = [criterion.pk for criterion in criteria] - has_progress = _subtree_has_learner_progress(subtree_ids, criterion_ids) - if has_progress: + archived = _subtree_has_learner_progress(subtree_ids, criterion_ids) + if not archived: + try: + # Savepoint: a learner status committed after the progress check above makes the delete + # fail on its PROTECT foreign key, and only this block must roll back. + with transaction.atomic(): + parent_group = target_group.parent + target_group.delete() + _delete_orphaned_object_tags({criterion.object_tag_id for criterion in criteria}) + if parent_group is not None: + _cascade_remove_from_group(parent_group, user) + except IntegrityError: + # Archive only if the failure traces to the subtree's own progress. A status on an ancestor + # group, or any other integrity failure, would not be fixed by archiving the subtree. The + # re-query is needed because the status may have landed after the pre-check above. + if not _subtree_has_learner_progress(subtree_ids, criterion_ids): + raise + archived = True + # The rolled-back block may have cleared the pk of in-memory instances it had already deleted. + target_group = CompetencyCriteriaGroup.objects.get(pk=group_id) + if archived: _archive_competency_criteria_subtree(target_group, subtree_ids, criteria, user) - else: - parent_group = target_group.parent - target_group.delete() - _delete_orphaned_object_tags({criterion.object_tag_id for criterion in criteria}) - if parent_group is not None: - _cascade_remove_from_group(parent_group, user) return GroupDeletionResult( id=group_id, - archived=has_progress, + archived=archived, cascaded_group_count=len(subtree_ids) - 1, cascaded_criteria_count=len(criteria), ) 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 0167c9fdd..976842ac6 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -304,6 +304,13 @@ def delete(self, request, *args, **kwargs): raise NotFound() from exc except DjangoValidationError as exc: raise DRFValidationError(exc.message_dict if hasattr(exc, "message_dict") else exc.messages) from exc + except IntegrityError as exc: + # ProtectedError is a subclass. The API archives a race on the target subtree's own progress itself, so + # this is a conflict it could not attribute to that subtree, such as a status that reached an ancestor + # group. A retry re-checks and then deletes or archives accordingly. + raise CompetencyCriteriaConflict( + _("The delete conflicted with a concurrent change. Retry the DELETE.") + ) from exc return Response( { "id": result.id, diff --git a/tests/openedx_learning/applets/cbe/test_group_removal.py b/tests/openedx_learning/applets/cbe/test_group_removal.py index b1776a127..223b23e4f 100644 --- a/tests/openedx_learning/applets/cbe/test_group_removal.py +++ b/tests/openedx_learning/applets/cbe/test_group_removal.py @@ -2,15 +2,19 @@ from collections.abc import Iterator from datetime import datetime from typing import Any +from unittest.mock import patch import pytest import rules from django.apps import apps from django.core.exceptions import PermissionDenied, ValidationError +from django.db import IntegrityError from rules.permissions import permissions as rule_permissions from openedx_catalog.models import CourseRun from openedx_learning.api import delete_or_archive_competency_criteria_group, resolve_competency_criteria_group_course +from openedx_learning.applets.cbe import api as cbe_api +from openedx_learning.applets.cbe.api import _subtree_has_learner_progress as subtree_has_learner_progress from openedx_learning.models import ( CompetencyCriteriaGroup, CompetencyCriterion, @@ -54,6 +58,23 @@ def _make_criterion( return CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=default_rule_profile) +def _progress_check_misses_the_status_once() -> Any: + """ + Stand in for ``_subtree_has_learner_progress`` so its first call, the pre-check, reports no progress. + + Every later call runs the real query, as the fallback's re-check does, so a status row that exists + by then is found. + """ + calls = 0 + + def check(group_ids: set[int], criterion_ids: list[int]) -> bool: + nonlocal calls + calls += 1 + return False if calls == 1 else subtree_has_learner_progress(group_ids, criterion_ids) + + return patch.object(cbe_api, "_subtree_has_learner_progress", side_effect=check) + + # ================================================================================================= # Hard-delete vs. archive, and the reported counts # ================================================================================================= @@ -344,6 +365,130 @@ def test_hard_delete_removes_an_object_tag_shared_by_two_criteria_in_the_subtree assert not ObjectTag.objects.filter(id=object_tag.id).exists() +def test_learner_status_committed_after_the_progress_check_archives_instead_of_deleting( + user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + A status row that lands after the progress check makes the hard delete fail on the PROTECT + foreign key; the call archives the subtree instead of raising, and keeps every ObjectTag. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + criterion = _make_criterion(target, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + with _progress_check_misses_the_status_once(): + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.archived is True + assert result.cascaded_criteria_count == 1 + target.refresh_from_db() + criterion.refresh_from_db() + assert target.archived is True + assert criterion.archived is True + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_group_status_committed_after_the_progress_check_archives_instead_of_deleting( + user, *, tag: Tag, course_run: CourseRun, now: datetime, +) -> None: + """A group-level status row that lands after the progress check also falls back to archiving.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=target, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + with _progress_check_misses_the_status_once(): + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.archived is True + target.refresh_from_db() + assert target.archived is True + + +def test_status_inserted_right_after_the_progress_check_archives_instead_of_deleting( + user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + A status row written between the progress check and the delete is found by the fallback's + re-check, so the subtree is archived rather than the error being raised. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + criterion = _make_criterion(target, object_tag, default_rule_profile) + calls = 0 + + def graded_right_after_the_first_check(group_ids: set[int], criterion_ids: list[int]) -> bool: + nonlocal calls + calls += 1 + if calls > 1: + return subtree_has_learner_progress(group_ids, criterion_ids) + found = subtree_has_learner_progress(group_ids, criterion_ids) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + return found + + with patch.object(cbe_api, "_subtree_has_learner_progress", side_effect=graded_right_after_the_first_check): + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.archived is True + target.refresh_from_db() + criterion.refresh_from_db() + assert target.archived is True + assert criterion.archived is True + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_status_on_an_ancestor_group_outside_the_subtree_raises_instead_of_archiving( + user, *, tag: Tag, course_run: CourseRun, now: datetime, +) -> None: + """ + A status that reaches an ancestor group after the cascade's own check makes the ancestor's delete + fail. The target subtree has no progress, so nothing is archived: the error propagates and the + rolled-back subtree and ancestor stay as they were. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=course_level, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + with patch.object(cbe_api, "_group_has_learner_progress", return_value=False): + with pytest.raises(IntegrityError): + delete_or_archive_competency_criteria_group(target.id, user) + + target.refresh_from_db() + course_level.refresh_from_db() + assert target.archived is False + assert course_level.archived is False + + +def test_integrity_error_unrelated_to_learner_progress_is_raised_instead_of_archiving( + user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """An IntegrityError with no learner status anywhere in the subtree propagates and leaves the subtree untouched.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + criterion = _make_criterion(target, object_tag, default_rule_profile) + + with patch.object(cbe_api, "_delete_orphaned_object_tags", side_effect=IntegrityError): + with pytest.raises(IntegrityError): + delete_or_archive_competency_criteria_group(target.id, user) + + target.refresh_from_db() + criterion.refresh_from_db() + assert target.archived is False + assert criterion.archived is False + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + def test_archive_when_status_exists_only_on_the_target_group( user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index f59b81517..6c0d5fb2b 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -32,6 +32,7 @@ LogicOperator, MasteryStatus, RuleType, + StudentCompetencyCriteriaGroupStatus, StudentCompetencyCriterionStatus, ) from openedx_tagging.models import ObjectTag, Tag, Taxonomy @@ -2802,6 +2803,28 @@ def test_group_delete_maps_a_validation_error_from_the_api_to_400( assert "group_id" in response.json() +def test_group_delete_whose_error_is_not_a_learner_race_on_the_subtree_is_a_retryable_409( + user_client: APIClient, user, tag: Tag, course_run: CourseRun, now: datetime, +) -> None: + """ + A status that reaches an ancestor group during the delete makes the DELETE fail outside the target + subtree. The view answers a 409 telling the caller to retry, and nothing is deleted or archived. + """ + leaf = _create_leaf_group(tag, course_run) + course_level = leaf.parent + assert course_level is not None + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=course_level, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + with mock.patch.object(cbe_api, "_group_has_learner_progress", return_value=False): + response = user_client.delete(group_delete_url(tag.id, leaf.id)) + + assert response.status_code == status.HTTP_409_CONFLICT + assert str(response.data["detail"]) == "The delete conflicted with a concurrent change. Retry the DELETE." + assert CompetencyCriteriaGroup.objects.filter(id__in=[leaf.id, course_level.id], archived=False).count() == 2 + + @pytest.fixture(name="checked_object_ids") def _checked_object_ids(_allow_object_level_tagging: None) -> Generator[list[str], None, None]: """Record every object_id the object-level tagging predicate is asked about, then defer to it.""" From b061f607b821e998d08a5331d459401280f86d96 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Fri, 30 Oct 2026 16:39:26 +0300 Subject: [PATCH 05/10] fix: add only the competency's own tag when creating a criterion (#675) create_competency_criterion() read the object's tags in the taxonomy, added its own value, and passed the full list to tag_object(), which replaces the object's tags in that taxonomy. A concurrent create or delete for another competency holds a different Tag lock, so the two could interleave: the create could restore an ObjectTag the delete had just removed, or delete an ObjectTag the other create had just added, taking that criterion with it through the cascade. The create now reuses this competency's ObjectTag if it exists, or inserts only that one row after the checks tag_object() applies when it adds one tag: allow_multiple and OBJECT_MAX_TAGS, with the same error messages. It never reads or writes any other tag on the object. Co-Authored-By: Claude Opus 5.5 Co-Authored-By: Claude Fable 5 --- src/openedx_learning/applets/cbe/api.py | 40 +++++++---- .../openedx_learning/applets/cbe/test_api.py | 69 +++++++++++++++++++ .../applets/cbe/test_group_removal.py | 10 +-- 3 files changed, 98 insertions(+), 21 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 25c83237b..94e5e25ef 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -19,7 +19,8 @@ from openedx_catalog.api import get_course_run, get_course_run_ids from openedx_catalog.models_api import CourseRun -from openedx_tagging.api import create_taxonomy, get_ancestor_tags, get_descendant_tags, get_object_tags, tag_object +from openedx_tagging import api as tagging_api +from openedx_tagging.api import create_taxonomy, get_ancestor_tags, get_descendant_tags from openedx_tagging.models import ObjectTag, Tag, Taxonomy from openedx_tagging.rules import ObjectTagPermissionItem, UserType @@ -486,10 +487,11 @@ def create_competency_criterion( 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. - ``tag_object()`` replaces an object's full tag list for a taxonomy rather than appending, so - this reads the object's existing tags first and unions in the new one, rather than silently - dropping a sibling competency's tag under the same taxonomy. Raises ValidationError (keyed - "object_id") when the taxonomy refuses the tag, e.g. it allows only one tag per object. + Adds only this competency's ObjectTag to the object, reusing it if it already exists. Other tags + on the object, including other competencies' tags in the same taxonomy, are never read or + changed, so a concurrent create or delete for another competency cannot be undone by this one. + Raises ValidationError (keyed "object_id") when the taxonomy allows one tag per object and the + object already has a tag in it, or when the object is at the tag limit. When no rule fields are supplied, resolves to the live profile scoped to the tag's taxonomy, else the live system default, rather than persisting null: ADR-0002 Decision 4 always @@ -509,16 +511,28 @@ def create_competency_criterion( 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 + assert tag.taxonomy is not None _validate_containment(group, course_run) - existing_values = [existing_tag.value for existing_tag in get_object_tags(object_id, taxonomy_id=tag.taxonomy_id)] - if tag.value not in existing_values: - existing_values.append(tag.value) - try: - tag_object(object_id, tag.taxonomy, existing_values) - except ValueError as exc: - raise ValidationError({"object_id": str(exc)}) from exc - object_tag = ObjectTag.objects.get(object_id=object_id, taxonomy_id=tag.taxonomy_id, tag_id=tag.id) + object_tag = ObjectTag.objects.filter(object_id=object_id, taxonomy_id=tag.taxonomy_id, tag_id=tag.id).first() + if object_tag is None: + # These are the checks tag_object() applies when adding one tag, without its replace-the-list write. + if not tag.taxonomy.allow_multiple and ObjectTag.objects.filter( + object_id=object_id, taxonomy_id=tag.taxonomy_id, + ).exists(): + raise ValidationError({ + "object_id": _("Taxonomy ({name}) only allows one tag per object.").format(name=tag.taxonomy.name), + }) + limit = tagging_api.OBJECT_MAX_TAGS + if ObjectTag.objects.filter(object_id=object_id).count() + 1 > limit: + raise ValidationError({ + "object_id": _("Cannot add more than {limit} tags to ({object_id}).").format( + object_id=object_id, limit=limit, + ), + }) + object_tag = ObjectTag(taxonomy=tag.taxonomy, object_id=object_id, tag=tag) + object_tag.full_clean() + object_tag.save() if rule_profile_id is None and rule_type_override is None and rule_payload_override is None: profile = _get_applicable_rule_profile(tag.taxonomy_id) diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index 78671a64f..ca8e9f8d8 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -2029,6 +2029,75 @@ def test_create_competency_criterion_takes_the_tag_lock_itself( cbe_api.create_competency_criterion(group, usage_key(course_run, "p1"), course_run=course_run) +def test_create_competency_criterion_writes_only_its_own_object_tag( + tag: Tag, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """ + Creating a criterion for one competency inserts that competency's ObjectTag and nothing else, so + it cannot undo a concurrent create or delete of a sibling competency's ObjectTag on the same object. + """ + sibling_tag = make_tag(competency_taxonomy, "Sibling Skill") + object_id = usage_key(course_run, "p1") + sibling = ObjectTag.objects.create(object_id=object_id, taxonomy=competency_taxonomy, tag=sibling_tag) + group = _create_leaf_group(tag, course_run) + + with CaptureQueriesContext(connection) as queries: + criterion = cbe_api.create_competency_criterion(group, object_id, course_run=course_run) + + quoted_table = re.escape(connection.ops.quote_name("oel_tagging_objecttag")) + object_tag_writes = [ + query["sql"] for query in queries.captured_queries + if re.match(rf"(INSERT INTO|UPDATE|DELETE FROM) {quoted_table}", query["sql"]) + ] + assert len(object_tag_writes) == 1 + assert object_tag_writes[0].startswith("INSERT") + assert criterion.object_tag.tag_id == tag.id + assert ObjectTag.objects.get(id=sibling.id).tag_id == sibling_tag.id + + +def test_create_competency_criterion_does_not_resurrect_a_sibling_object_tag_deleted_mid_create( + tag: Tag, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """ + A sibling competency's ObjectTag deleted by a concurrent request, right after this create first + reads ObjectTags, stays deleted; the create adds only its own competency's tag. + """ + sibling_tag = make_tag(competency_taxonomy, "Sibling Skill") + object_id = usage_key(course_run, "p1") + sibling = ObjectTag.objects.create(object_id=object_id, taxonomy=competency_taxonomy, tag=sibling_tag) + group = _create_leaf_group(tag, course_run) + raced = False + quoted_table = connection.ops.quote_name("oel_tagging_objecttag") + + def _delete_sibling_after_first_object_tag_query(execute, sql, params, many, context): + nonlocal raced + result = execute(sql, params, many, context) + if not raced and quoted_table in sql: + raced = True + ObjectTag.objects.filter(pk=sibling.pk).delete() + return result + + with connection.execute_wrapper(_delete_sibling_after_first_object_tag_query): + cbe_api.create_competency_criterion(group, object_id, course_run=course_run) + + assert raced + assert list(ObjectTag.objects.filter(object_id=object_id).values_list("tag_id", flat=True)) == [tag.id] + + +def test_create_competency_criterion_reuses_an_existing_object_tag_for_the_same_competency( + tag: Tag, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """An ObjectTag already tying the object to this competency is reused rather than duplicated.""" + object_id = usage_key(course_run, "p1") + existing = ObjectTag.objects.create(object_id=object_id, taxonomy=competency_taxonomy, tag=tag) + group = _create_leaf_group(tag, course_run) + + criterion = cbe_api.create_competency_criterion(group, object_id, course_run=course_run) + + assert criterion.object_tag_id == existing.id + assert ObjectTag.objects.filter(object_id=object_id).count() == 1 + + 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_group_removal.py b/tests/openedx_learning/applets/cbe/test_group_removal.py index 223b23e4f..e690943a4 100644 --- a/tests/openedx_learning/applets/cbe/test_group_removal.py +++ b/tests/openedx_learning/applets/cbe/test_group_removal.py @@ -322,14 +322,10 @@ def test_untag_with_the_competency_taxonomy_disabled_removes_only_the_orphaned_o def test_hard_delete_leaves_a_tag_of_another_competency_on_the_same_object( - user, *, monkeypatch: pytest.MonkeyPatch, competency_taxonomy: CompetencyTaxonomy, tag: Tag, course_run: CourseRun, + user, *, competency_taxonomy: CompetencyTaxonomy, tag: Tag, course_run: CourseRun, 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. - """ + """An ObjectTag for another competency tag on the same object survives the delete together with its criterion.""" root = CompetencyCriteriaGroup.objects.create(tag=tag) target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) _make_criterion(target, object_tag, default_rule_profile) @@ -338,8 +334,6 @@ def test_hard_delete_leaves_a_tag_of_another_competency_on_the_same_object( 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_criteria_group(target.id, user) From 426f9a578c3ffd1a95aa41563d317ea60739cadc Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Fri, 30 Oct 2026 16:39:43 +0300 Subject: [PATCH 06/10] docs: explain why the delete paths keep the competency's tag lock (#675) AGENTS.md asks every select_for_update() to say why a constraint cannot do the job. The delete paths take the same Tag lock that associate_competency_criterion() takes first, so a create and a delete for one competency run one after the other instead of failing on a foreign key. Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 94e5e25ef..b423f4b7f 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -1318,6 +1318,9 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - raise PermissionDenied() tag_id = group.tag_id with transaction.atomic(): + # The same lock associate_competency_criterion() takes first. Without it, a create could add a criterion + # to this subtree after it is read, or reuse an ObjectTag this call deletes as orphaned. The foreign keys + # would only turn either into an IntegrityError on one side, not order the two. Tag.objects.select_for_update().get(pk=tag_id) # Read under the Tag lock: a concurrent delete may have won, and DoesNotExist propagates. target_group = CompetencyCriteriaGroup.objects.get(pk=group_id) From ede2c2b7dcec3777718829ce0b9492917b1c4fa2 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Sat, 7 Nov 2026 20:34:59 +0300 Subject: [PATCH 07/10] fix: record the acting user when a group subtree is deleted (#675) The target group's delete ran without _set_history_user(), so its deletion history row had no user, unlike every other delete in this module. Descendant groups and criteria are removed by Django's delete collector, which gives no per-instance hook for the user, so their deletion rows are left as they were. Co-Authored-By: Claude Fable 5 --- src/openedx_learning/applets/cbe/api.py | 1 + .../applets/cbe/test_group_removal.py | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index b423f4b7f..e57cc4d7a 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -1333,6 +1333,7 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - # fail on its PROTECT foreign key, and only this block must roll back. with transaction.atomic(): parent_group = target_group.parent + _set_history_user(target_group, user) target_group.delete() _delete_orphaned_object_tags({criterion.object_tag_id for criterion in criteria}) if parent_group is not None: diff --git a/tests/openedx_learning/applets/cbe/test_group_removal.py b/tests/openedx_learning/applets/cbe/test_group_removal.py index e690943a4..bea91ae3c 100644 --- a/tests/openedx_learning/applets/cbe/test_group_removal.py +++ b/tests/openedx_learning/applets/cbe/test_group_removal.py @@ -182,6 +182,19 @@ def test_subtree_archive_records_the_acting_user_in_history( assert _latest_history_user("HistoricalCompetencyCriterion", criterion.id) == user +def test_hard_delete_records_the_acting_user_on_the_targets_deletion_history( + user, *, tag: Tag, course_run: CourseRun, +) -> None: + """The history row written when the target group is deleted carries the acting user.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag) + target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + target_id = target.id + + delete_or_archive_competency_criteria_group(target_id, user) + + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", target_id) == user + + def test_repeat_delete_on_an_archived_subtree_writes_no_history( user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, From 9959d8c42d21c2cf9e7e6aaaba842e9c8932eb9f Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Fri, 13 Nov 2026 21:21:56 +0300 Subject: [PATCH 08/10] fix: add one tag to an object without replacing its tags (#675) create_competency_criterion() wrote the ObjectTag row itself and copied two of tag_object()'s rules, so the single-valued taxonomy rule and the per-object tag limit lived in two apps. The tagging API gains add_tag_to_object(), which adds one tag to an object, reuses the ObjectTag if the tag is already applied, and leaves every other tag of the object untouched. tag_object() and the new function share the same two checks and error messages through _check_object_tag_limit() and _check_taxonomy_allows_tag_count(). create_competency_criterion() calls the new function and maps its ValueError to a ValidationError keyed to object_id. Co-Authored-By: Claude Fable 5 --- src/openedx_learning/applets/cbe/api.py | 31 ++++-------- src/openedx_tagging/api.py | 58 +++++++++++++++++++++- tests/openedx_tagging/test_api.py | 64 +++++++++++++++++++++++++ 3 files changed, 129 insertions(+), 24 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index e57cc4d7a..b1c1584af 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -487,9 +487,10 @@ def create_competency_criterion( 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. - Adds only this competency's ObjectTag to the object, reusing it if it already exists. Other tags - on the object, including other competencies' tags in the same taxonomy, are never read or - changed, so a concurrent create or delete for another competency cannot be undone by this one. + Adds only this competency's ObjectTag to the object through ``tagging_api.add_tag_to_object()``, + reusing it if it already exists. Other tags on the object, including other competencies' tags in + the same taxonomy, are never changed, so a concurrent create or delete for another competency + cannot be undone by this one. Raises ValidationError (keyed "object_id") when the taxonomy allows one tag per object and the object already has a tag in it, or when the object is at the tag limit. @@ -514,25 +515,11 @@ def create_competency_criterion( assert tag.taxonomy is not None _validate_containment(group, course_run) - object_tag = ObjectTag.objects.filter(object_id=object_id, taxonomy_id=tag.taxonomy_id, tag_id=tag.id).first() - if object_tag is None: - # These are the checks tag_object() applies when adding one tag, without its replace-the-list write. - if not tag.taxonomy.allow_multiple and ObjectTag.objects.filter( - object_id=object_id, taxonomy_id=tag.taxonomy_id, - ).exists(): - raise ValidationError({ - "object_id": _("Taxonomy ({name}) only allows one tag per object.").format(name=tag.taxonomy.name), - }) - limit = tagging_api.OBJECT_MAX_TAGS - if ObjectTag.objects.filter(object_id=object_id).count() + 1 > limit: - raise ValidationError({ - "object_id": _("Cannot add more than {limit} tags to ({object_id}).").format( - object_id=object_id, limit=limit, - ), - }) - object_tag = ObjectTag(taxonomy=tag.taxonomy, object_id=object_id, tag=tag) - object_tag.full_clean() - object_tag.save() + try: + object_tag = tagging_api.add_tag_to_object(object_id, tag) + except ValueError as exc: + # The tagging API reports its tag-count rules as ValueError; this API reports them keyed to the field. + raise ValidationError({"object_id": str(exc)}) from exc if rule_profile_id is None and rule_type_override is None and rule_payload_override is None: profile = _get_applicable_rule_profile(tag.taxonomy_id) diff --git a/src/openedx_tagging/api.py b/src/openedx_tagging/api.py index 8d2f90353..25c9f8ae6 100644 --- a/src/openedx_tagging/api.py +++ b/src/openedx_tagging/api.py @@ -330,12 +330,27 @@ def _check_new_tag_count( else: current_count = ObjectTag.objects.filter(object_id=object_id).exclude(_export_id=taxonomy_export_id).count() + _check_object_tag_limit(current_count, new_tag_count, object_id) + + +def _check_object_tag_limit(current_count: int, new_tag_count: int, object_id: str) -> None: + """ + Raises ValueError if ``new_tag_count`` tags on top of ``current_count`` would exceed OBJECT_MAX_TAGS. + """ if current_count + new_tag_count > OBJECT_MAX_TAGS: raise ValueError( _("Cannot add more than {limit} tags to ({object_id}).").format(object_id=object_id, limit=OBJECT_MAX_TAGS) ) +def _check_taxonomy_allows_tag_count(taxonomy: Taxonomy, tag_count: int) -> None: + """ + Raises ValueError if ``tag_count`` tags on one object is more than the taxonomy allows. + """ + if not taxonomy.allow_multiple and tag_count > 1: + raise ValueError(_("Taxonomy ({name}) only allows one tag per object.").format(name=taxonomy.name)) + + def _get_current_tags( taxonomy: Taxonomy | None, tags: list[str], @@ -346,8 +361,7 @@ def _get_current_tags( Returns the current object tags of the related object_id with taxonomy """ if taxonomy: - if not taxonomy.allow_multiple and len(tags) > 1: - raise ValueError(_("Taxonomy ({name}) only allows one tag per object.").format(name=taxonomy.name)) + _check_taxonomy_allows_tag_count(taxonomy, len(tags)) current_tags = list( ObjectTag.objects.filter(taxonomy=taxonomy, object_id=object_id) ) @@ -461,6 +475,46 @@ def tag_object( object_tag.save() +def add_tag_to_object(object_id: str, tag: Tag) -> ObjectTag: + """ + Adds one existing ``tag`` to ``object_id``, and returns its ObjectTag. + + UNSTABLE: this function is new, so its shape may change without a deprecation cycle. + + Unlike :func:`tag_object`, no other ObjectTag of the object is read for replacement or changed, + so a concurrent write of another tag on the same object, even one in the same taxonomy, is not + undone by this call. If ``tag`` is already applied to the object, its existing ObjectTag is + returned and nothing is written. + + Raises ValueError if the taxonomy of ``tag`` allows only one tag per object and the object + already has a different tag in it, or if the object already has the maximum number of tags + (``OBJECT_MAX_TAGS``). Both use the same messages as :func:`tag_object`. Raises ValueError if + ``tag`` does not belong to a taxonomy. Raises Django's ``ValidationError`` if the new ObjectTag + fails ``full_clean()``. + + The checks and the insert are not atomic, so a caller that needs them to hold against concurrent + writers must serialize those writers itself. + """ + taxonomy = tag.taxonomy + if taxonomy is None: + raise ValueError(_("Tag ({value}) does not belong to a taxonomy.").format(value=tag.value)) + + object_tag = ObjectTag.objects.filter(object_id=object_id, taxonomy_id=taxonomy.id, tag_id=tag.id).first() + if object_tag is not None: + return object_tag + + if not taxonomy.allow_multiple: + # The new tag would be the one after the ones the object already has in this taxonomy. + existing_count = ObjectTag.objects.filter(object_id=object_id, taxonomy_id=taxonomy.id).count() + _check_taxonomy_allows_tag_count(taxonomy, existing_count + 1) + _check_object_tag_limit(ObjectTag.objects.filter(object_id=object_id).count(), 1, object_id) + + object_tag = ObjectTag(taxonomy=taxonomy, object_id=object_id, tag=tag) + object_tag.full_clean() + object_tag.save() + return object_tag + + def add_tag_to_taxonomy( taxonomy: Taxonomy, tag: str, diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index e047c6a07..e06630042 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -663,6 +663,70 @@ def test_tag_object_limit(self) -> None: assert exc.exception assert "Cannot add more than 100 tags to" in str(exc.exception) + def test_add_tag_to_object(self) -> None: + """Adds the tag as a new ObjectTag and leaves the object's other tags alone.""" + self.taxonomy.allow_multiple = True + self.taxonomy.save() + existing = ObjectTag.objects.create(object_id="biology101", taxonomy=self.taxonomy, tag=self.archaea) + + object_tag = tagging_api.add_tag_to_object("biology101", self.bacteria) + + assert object_tag.pk != existing.pk + assert object_tag.tag_id == self.bacteria.id + assert object_tag.taxonomy_id == self.taxonomy.id + assert object_tag.object_id == "biology101" + assert sorted(ObjectTag.objects.filter(object_id="biology101").values_list("tag_id", flat=True)) == sorted( + [self.archaea.id, self.bacteria.id] + ) + + def test_add_tag_to_object_reuses_an_existing_object_tag_for_the_tag(self) -> None: + existing = ObjectTag.objects.create(object_id="biology101", taxonomy=self.taxonomy, tag=self.archaea) + + object_tag = tagging_api.add_tag_to_object("biology101", self.archaea) + + assert object_tag.pk == existing.pk + assert ObjectTag.objects.filter(object_id="biology101").count() == 1 + + def test_add_tag_to_object_requires_a_taxonomy(self) -> None: + tag = Tag(taxonomy=None, value="Orphan") + + with pytest.raises(ValueError) as excinfo: + tagging_api.add_tag_to_object("biology101", tag) + + assert str(excinfo.value) == "Tag (Orphan) does not belong to a taxonomy." + assert not ObjectTag.objects.filter(object_id="biology101").exists() + + def test_add_tag_to_object_no_multiple(self) -> None: + """A single-valued taxonomy rejects a second tag, with the same message tag_object() gives.""" + assert not self.taxonomy.allow_multiple + ObjectTag.objects.create(object_id="biology101", taxonomy=self.taxonomy, tag=self.archaea) + + with pytest.raises(ValueError) as add_excinfo: + tagging_api.add_tag_to_object("biology101", self.bacteria) + with pytest.raises(ValueError) as tag_object_excinfo: + tagging_api.tag_object("biology101", self.taxonomy, [self.archaea.value, self.bacteria.value]) + + assert "only allows one tag per object" in str(add_excinfo.value) + assert str(add_excinfo.value) == str(tag_object_excinfo.value) + assert list(ObjectTag.objects.filter(object_id="biology101").values_list("tag_id", flat=True)) == [ + self.archaea.id + ] + + def test_add_tag_to_object_limit(self) -> None: + """An object at the tag limit rejects another tag, with the same message tag_object() gives.""" + self.taxonomy.allow_multiple = True + self.taxonomy.save() + self.create_100_taxonomies() + + with pytest.raises(ValueError) as add_excinfo: + tagging_api.add_tag_to_object("limit_tag_count", self.archaea) + with pytest.raises(ValueError) as tag_object_excinfo: + tagging_api.tag_object("limit_tag_count", self.taxonomy, [self.archaea.value]) + + assert "Cannot add more than 100 tags to (limit_tag_count)" in str(add_excinfo.value) + assert str(add_excinfo.value) == str(tag_object_excinfo.value) + assert not ObjectTag.objects.filter(object_id="limit_tag_count", taxonomy=self.taxonomy).exists() + def test_get_object_tags_deleted_disabled(self) -> None: """ Test that get_object_tags doesn't return tags from disabled taxonomies From 872b2bb0f99d4f6c67420fd2fc4bc7da78ef89bb Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Fri, 13 Nov 2026 21:23:53 +0300 Subject: [PATCH 09/10] fix: correct the group delete's lock comment and docstring (#675) The Tag-lock comment in delete_or_archive_competency_criteria_group() claimed the foreign keys turn every race into an IntegrityError. A create that commits into the subtree just before the delete reads its rows is deleted silently, because CompetencyCriterion.group cascades. The comment now gives that reasoning, as the criterion delete's lock comment does. The docstring also gains the outcomes it left out: the archive path re-validates each stored rule payload and can fail the whole call, a hard delete that commits first fails a learner's later grade write, and a repeat call on a group archived only because of a sibling hard-deletes it. Wording fixes in the view, the course resolution function and GroupDeletionResult, and two renames (candidate_objecttag_ids, should_archive). resolve_competency_criteria_group_course() becomes private, since only the delete function calls it. Co-Authored-By: Claude Fable 5 --- src/openedx_learning/applets/cbe/api.py | 84 +++++++++++-------- src/openedx_learning/applets/cbe/data.py | 5 +- .../applets/cbe/rest_api/v1/views.py | 10 +-- .../applets/cbe/test_group_removal.py | 13 +-- 4 files changed, 63 insertions(+), 49 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index b1c1584af..2d9b65c63 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -60,7 +60,6 @@ "get_courses_with_live_criteria", "is_competency_taxonomy", "record_graded_object_statuses", - "resolve_competency_criteria_group_course", "resolve_competency_tag", "resolve_supplied_leaf_group", "roll_up_competency_statuses", @@ -1174,17 +1173,12 @@ def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> return CriterionDeletionResult(id=criterion_id, archived=False) -def resolve_competency_criteria_group_course(group: CompetencyCriteriaGroup) -> CourseRun: +def _resolve_competency_criteria_group_course(group: CompetencyCriteriaGroup) -> CourseRun: """ Return the CourseRun that scopes the subtree rooted at ``group``. - 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. - That is the course of the nearest group with one, starting at ``group`` itself and walking - up through its ancestors, so a group may sit at any depth below its course-level group. Costs - one query per ancestor walked. A caller checks ``oel_tagging.can_tag_object`` against this - course before removing the subtree. + up through its ancestors, so a group may sit at any depth below its course-level group. Raises ValidationError (keyed "group_id") if ``group`` is a root CompetencyCriteriaGroup, whose subtree can span several courses, or if no group in its lineage has a course. @@ -1245,18 +1239,19 @@ def _subtree_has_learner_progress(group_ids: set[int], criterion_ids: list[int]) ) -def _delete_orphaned_object_tags(candidate_ids: set[int]) -> None: +def _delete_orphaned_object_tags(candidate_objecttag_ids: set[int]) -> None: """ - Delete each ObjectTag in ``candidate_ids`` that no CompetencyCriterion references, and nothing else. + Delete each ObjectTag in ``candidate_objecttag_ids`` that no CompetencyCriterion references, and nothing else. - ``candidate_ids`` are the ObjectTag ids of the criteria in a deleted group subtree. Must run - after those criteria are deleted, inside the transaction that holds the competency's Tag lock. - No other ObjectTag on the object, in any taxonomy, is read or written. + Must run after the criteria that used these ObjectTags are deleted, inside the transaction that + holds the competency's Tag lock. No other ObjectTag on the object, in any taxonomy, is read or written. """ still_referenced = set( - CompetencyCriterion.objects.filter(object_tag_id__in=candidate_ids).values_list("object_tag_id", flat=True) + CompetencyCriterion.objects.filter( + object_tag_id__in=candidate_objecttag_ids, + ).values_list("object_tag_id", flat=True) ) - ObjectTag.objects.filter(pk__in=candidate_ids - still_referenced).delete() + ObjectTag.objects.filter(pk__in=candidate_objecttag_ids - still_referenced).delete() def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) -> GroupDeletionResult: @@ -1266,9 +1261,8 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - 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 course - :func:`resolve_competency_criteria_group_course` returns for the target group; otherwise - Django's ``PermissionDenied`` is raised before anything is locked or changed. + ``user`` must hold ``oel_tagging.can_tag_object`` for the course of the nearest group with one, + starting at the target. The check runs before anything is locked or changed. The subtree is the target group plus every descendant group and leaf CompetencyCriterion. Hard-deleted if no learner status exists anywhere in it; archived in place, all-or-nothing, @@ -1278,9 +1272,9 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - On a hard delete, only the ObjectTags of the subtree's criteria that no other criterion references are removed; no other tag on the object is touched. - The whole operation runs in one transaction that locks only the competency's Tag, the same lock - a create or a criterion delete for that competency takes first, so those wait until this call - commits. + The first read and the permission check run before the lock. The rest runs in one transaction that + takes only one explicit lock, on the competency's Tag row, the same lock a create or a criterion + delete for that competency takes first, so those wait until this call commits. A learner status written concurrently, after this call's progress check, makes the call archive the subtree instead of deleting it, provided a second progress check finds a status on @@ -1288,14 +1282,27 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - reached an ancestor group during the cascade, and the original ``IntegrityError`` is re-raised with nothing changed. A caller may retry: the retry re-checks, then deletes or archives. - Raises CompetencyCriteriaGroup.DoesNotExist if no such group exists (including one deleted - concurrently), Tag.DoesNotExist if its competency tag was deleted concurrently, PermissionDenied - if ``user`` may not tag the course's object, ValidationError (keyed "group_id") if `group_id` - names a root CompetencyCriteriaGroup or a group with no course-level group in its lineage, and - ``IntegrityError`` when the delete races with a write outside the target subtree. + The archive path saves each criterion through ``CompetencyCriterion.save()``, which re-validates + the stored rule payload. One criterion with an invalid stored payload therefore fails the whole + call with ``ValidationError`` and nothing changes. + + When a hard delete commits first, a learner's grade write for any criterion in the subtree fails + inside the learner's grade transaction, as it does for a single criterion (see + :func:`delete_or_archive_competency_criterion`). + + A repeat call on an archived group whose own subtree has no learner status hard-deletes it and + returns ``archived=False``. This happens to a child group that was archived only because a + sibling under the same parent had learner status. + + Raises ``CompetencyCriteriaGroup.DoesNotExist`` if the group is gone (including a concurrent + delete), ``Tag.DoesNotExist`` if its competency tag was deleted concurrently, ``PermissionDenied`` + if ``user`` fails the check, ``ValidationError`` (keyed "group_id") for a root group or a group + with no course-level group in its lineage, ``ValidationError`` for an invalid stored rule payload + on the archive path, and ``IntegrityError`` when the delete races with a write outside the target + subtree. """ group = CompetencyCriteriaGroup.objects.select_related("tag__taxonomy").get(pk=group_id) - course = resolve_competency_criteria_group_course(group) + course = _resolve_competency_criteria_group_course(group) # A group's tag is always a competency tag, so its taxonomy is always set. assert group.tag.taxonomy is not None perm_obj = ObjectTagPermissionItem(taxonomy=group.tag.taxonomy, object_id=str(course.course_key)) @@ -1305,16 +1312,20 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - raise PermissionDenied() tag_id = group.tag_id with transaction.atomic(): - # The same lock associate_competency_criterion() takes first. Without it, a create could add a criterion - # to this subtree after it is read, or reuse an ObjectTag this call deletes as orphaned. The foreign keys - # would only turn either into an IntegrityError on one side, not order the two. + # A lock rather than a constraint, because Django cascades deletes in Python and the database can't + # catch the two races that follow. A create that commits a new criterion or leaf group into this + # subtree between the subtree read below and the row-collecting SELECT inside ``target_group.delete()`` + # would have those rows deleted silently, since ``CompetencyCriterion.group`` cascades, after the create + # call already returned success. A create that reuses an ObjectTag this call deletes as orphaned, and + # commits between the "still referenced" check and the ObjectTag delete, would have its criterion + # cascaded away. Same lock associate_competency_criterion() takes first. Tag.objects.select_for_update().get(pk=tag_id) # Read under the Tag lock: a concurrent delete may have won, and DoesNotExist propagates. target_group = CompetencyCriteriaGroup.objects.get(pk=group_id) subtree_ids, criteria = _competency_criteria_subtree(target_group) criterion_ids = [criterion.pk for criterion in criteria] - archived = _subtree_has_learner_progress(subtree_ids, criterion_ids) - if not archived: + should_archive = _subtree_has_learner_progress(subtree_ids, criterion_ids) + if not should_archive: try: # Savepoint: a learner status committed after the progress check above makes the delete # fail on its PROTECT foreign key, and only this block must roll back. @@ -1331,14 +1342,15 @@ def delete_or_archive_competency_criteria_group(group_id: int, user: UserType) - # re-query is needed because the status may have landed after the pre-check above. if not _subtree_has_learner_progress(subtree_ids, criterion_ids): raise - archived = True - # The rolled-back block may have cleared the pk of in-memory instances it had already deleted. + should_archive = True + # target_group.delete() sets the instance's pk to None even though the savepoint rolled the row + # back, so re-read it. target_group = CompetencyCriteriaGroup.objects.get(pk=group_id) - if archived: + if should_archive: _archive_competency_criteria_subtree(target_group, subtree_ids, criteria, user) return GroupDeletionResult( id=group_id, - archived=archived, + archived=should_archive, cascaded_group_count=len(subtree_ids) - 1, cascaded_criteria_count=len(criteria), ) diff --git a/src/openedx_learning/applets/cbe/data.py b/src/openedx_learning/applets/cbe/data.py index ad72c3f2c..0c939c291 100644 --- a/src/openedx_learning/applets/cbe/data.py +++ b/src/openedx_learning/applets/cbe/data.py @@ -56,8 +56,9 @@ class GroupDeletionResult: """ The outcome of removing a CompetencyCriteriaGroup subtree: hard-deleted or archived, with counts. - The counts cover the target subtree only: ``cascaded_group_count`` is the number of descendant - groups beneath the target and ``cascaded_criteria_count`` the number of criteria in the subtree. + The counts cover the target subtree only: ``cascaded_group_count`` counts the target's descendant + groups, not the target itself and not the ancestors the removal cascaded to, and + ``cascaded_criteria_count`` is the number of criteria in the subtree. UNSTABLE: the criteria-authoring family is incomplete, so this shape may change without a deprecation cycle. 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 976842ac6..7b5a77992 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -290,9 +290,10 @@ def patch(self, request, tag_id: int, group_id: int): def delete(self, request, *args, **kwargs): """ - Resolve the group scoped to both path segments, then delegate to the public API, which checks permission. + Resolve the group by the `group_id` and `tag_id` in the URL, then delegate to the public API. - The lookup keeps archived groups, so a repeat DELETE on an archived subtree still returns 200. + The public API checks permission. The lookup keeps archived groups, so a repeat DELETE on an + archived subtree still returns 200. """ group = get_object_or_404( CompetencyCriteriaGroup, pk=self.kwargs["group_id"], tag_id=self.kwargs["tag_id"], @@ -305,9 +306,8 @@ def delete(self, request, *args, **kwargs): except DjangoValidationError as exc: raise DRFValidationError(exc.message_dict if hasattr(exc, "message_dict") else exc.messages) from exc except IntegrityError as exc: - # ProtectedError is a subclass. The API archives a race on the target subtree's own progress itself, so - # this is a conflict it could not attribute to that subtree, such as a status that reached an ancestor - # group. A retry re-checks and then deletes or archives accordingly. + # ProtectedError is a subclass. The API already archives when the target subtree itself gained a + # status, so this is a conflict outside it, such as a status on an ancestor. A retry re-checks. raise CompetencyCriteriaConflict( _("The delete conflicted with a concurrent change. Retry the DELETE.") ) from exc diff --git a/tests/openedx_learning/applets/cbe/test_group_removal.py b/tests/openedx_learning/applets/cbe/test_group_removal.py index bea91ae3c..34d0441c9 100644 --- a/tests/openedx_learning/applets/cbe/test_group_removal.py +++ b/tests/openedx_learning/applets/cbe/test_group_removal.py @@ -12,8 +12,9 @@ from rules.permissions import permissions as rule_permissions from openedx_catalog.models import CourseRun -from openedx_learning.api import delete_or_archive_competency_criteria_group, resolve_competency_criteria_group_course +from openedx_learning.api import delete_or_archive_competency_criteria_group from openedx_learning.applets.cbe import api as cbe_api +from openedx_learning.applets.cbe.api import _resolve_competency_criteria_group_course from openedx_learning.applets.cbe.api import _subtree_has_learner_progress as subtree_has_learner_progress from openedx_learning.models import ( CompetencyCriteriaGroup, @@ -700,7 +701,7 @@ def test_permission_course_rejects_a_root_group(tag: Tag) -> None: root = CompetencyCriteriaGroup.objects.create(tag=tag) with pytest.raises(ValidationError) as exc_info: - resolve_competency_criteria_group_course(root) + _resolve_competency_criteria_group_course(root) assert "group_id" in exc_info.value.message_dict @@ -710,7 +711,7 @@ def test_permission_course_is_the_targets_own_course_for_a_course_level_group(ta root = CompetencyCriteriaGroup.objects.create(tag=tag) course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) - assert resolve_competency_criteria_group_course(course_level) == course_run + assert _resolve_competency_criteria_group_course(course_level) == course_run def test_permission_course_is_the_parents_course_for_a_non_course_level_target(tag: Tag, course_run: CourseRun) -> None: @@ -719,7 +720,7 @@ def test_permission_course_is_the_parents_course_for_a_non_course_level_target(t course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) - assert resolve_competency_criteria_group_course(leaf) == course_run + assert _resolve_competency_criteria_group_course(leaf) == course_run @pytest.mark.parametrize("depth", [2, 3]) @@ -732,7 +733,7 @@ def test_permission_course_is_the_nearest_course_level_ancestors_course_at_any_d for _ in range(depth): group = CompetencyCriteriaGroup.objects.create(tag=tag, parent=group) - assert resolve_competency_criteria_group_course(group) == course_run + assert _resolve_competency_criteria_group_course(group) == course_run def test_permission_course_rejects_a_group_with_no_course_level_group_in_its_lineage(tag: Tag) -> None: @@ -742,7 +743,7 @@ def test_permission_course_rejects_a_group_with_no_course_level_group_in_its_lin target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=middle) with pytest.raises(ValidationError) as exc_info: - resolve_competency_criteria_group_course(target) + _resolve_competency_criteria_group_course(target) assert "group_id" in exc_info.value.message_dict From c9bb643180e7e244fc9627956de4609a1c8173ff Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Fri, 13 Nov 2026 21:27:57 +0300 Subject: [PATCH 10/10] test: pin root message, taxonomy 403, savepoint, sibling (#675) Four behaviors of the group delete had no test that fails when they break. The root-group 400 is now checked for its message, not just its key, in one merged test. A caller who lacks taxonomy access gets a 403 and the subtree stays unchanged. An ancestor cascade that fails after the subtree and its orphaned ObjectTags are deleted rolls back through the savepoint and archives instead. Deleting one group leaves a sibling group with its own learner status untouched. A view test covers the archive path failing the whole DELETE with a 400 when one criterion's stored rule payload has gone invalid. Test cleanup: drop a test whose setup product code cannot create, fold two duplicate fallback tests into one, make the ObjectTag untouched check compare values, remove existence checks that follow refresh_from_db(), and fix the stale helper name in two docstrings. Co-Authored-By: Claude Fable 5 --- .../applets/cbe/test_group_removal.py | 157 ++++++++++-------- .../applets/cbe/test_views.py | 70 ++++++-- 2 files changed, 146 insertions(+), 81 deletions(-) diff --git a/tests/openedx_learning/applets/cbe/test_group_removal.py b/tests/openedx_learning/applets/cbe/test_group_removal.py index 34d0441c9..869ba8095 100644 --- a/tests/openedx_learning/applets/cbe/test_group_removal.py +++ b/tests/openedx_learning/applets/cbe/test_group_removal.py @@ -1,4 +1,4 @@ -"""Tests for delete_or_archive_competency_criteria_group(), which reuses _cascade_from_group() unchanged.""" +"""Tests for delete_or_archive_competency_criteria_group(), which reuses _cascade_remove_from_group() unchanged.""" from collections.abc import Iterator from datetime import datetime from typing import Any @@ -271,32 +271,6 @@ def test_shared_object_tag_survives_when_a_criterion_outside_the_subtree_still_r assert CompetencyCriterion.objects.filter(id=sibling_criterion.id).exists() -def test_hard_delete_keeps_an_object_tag_still_referenced_outside_the_subtree( - user, competency_taxonomy: CompetencyTaxonomy, tag: Tag, course_run: CourseRun, - default_rule_profile: CompetencyRuleProfile, -) -> None: - """ - One object carries two competency tags of the same taxonomy. The tag only the deleted subtree - references is removed, and the tag a criterion outside the subtree references stays, with that criterion. - """ - root = CompetencyCriteriaGroup.objects.create(tag=tag) - course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) - target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) - sibling = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) - tag_a = Tag.objects.create(taxonomy=competency_taxonomy, value="Tag A") - tag_b = Tag.objects.create(taxonomy=competency_taxonomy, value="Tag B") - object_tag_a = _make_object_tag(competency_taxonomy, tag_a, "obj-two-tags") - object_tag_b = _make_object_tag(competency_taxonomy, tag_b, "obj-two-tags") - _make_criterion(target, object_tag_a, default_rule_profile) - sibling_criterion = _make_criterion(sibling, object_tag_b, default_rule_profile) - - delete_or_archive_competency_criteria_group(target.id, user) - - assert not ObjectTag.objects.filter(id=object_tag_a.id).exists() - assert ObjectTag.objects.filter(id=object_tag_b.id).exists() - assert CompetencyCriterion.objects.filter(id=sibling_criterion.id).exists() - - def test_untag_with_the_competency_taxonomy_disabled_removes_only_the_orphaned_object_tag( user, competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, @@ -373,33 +347,6 @@ def test_hard_delete_removes_an_object_tag_shared_by_two_criteria_in_the_subtree assert not ObjectTag.objects.filter(id=object_tag.id).exists() -def test_learner_status_committed_after_the_progress_check_archives_instead_of_deleting( - user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, - default_rule_profile: CompetencyRuleProfile, now: datetime, -) -> None: - """ - A status row that lands after the progress check makes the hard delete fail on the PROTECT - foreign key; the call archives the subtree instead of raising, and keeps every ObjectTag. - """ - root = CompetencyCriteriaGroup.objects.create(tag=tag) - target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) - criterion = _make_criterion(target, object_tag, default_rule_profile) - StudentCompetencyCriterionStatus.objects.create( - user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, - ) - - with _progress_check_misses_the_status_once(): - result = delete_or_archive_competency_criteria_group(target.id, user) - - assert result.archived is True - assert result.cascaded_criteria_count == 1 - target.refresh_from_db() - criterion.refresh_from_db() - assert target.archived is True - assert criterion.archived is True - assert ObjectTag.objects.filter(id=object_tag.id).exists() - - def test_group_status_committed_after_the_progress_check_archives_instead_of_deleting( user, *, tag: Tag, course_run: CourseRun, now: datetime, ) -> None: @@ -423,8 +370,9 @@ def test_status_inserted_right_after_the_progress_check_archives_instead_of_dele default_rule_profile: CompetencyRuleProfile, now: datetime, ) -> None: """ - A status row written between the progress check and the delete is found by the fallback's - re-check, so the subtree is archived rather than the error being raised. + A status row written between the progress check and the delete makes the delete fail on the PROTECT + foreign key. The fallback's re-check finds the row, so the subtree is archived rather than the error + being raised, with the counts reported and every ObjectTag kept. """ root = CompetencyCriteriaGroup.objects.create(tag=tag) target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) @@ -446,6 +394,7 @@ def graded_right_after_the_first_check(group_ids: set[int], criterion_ids: list[ result = delete_or_archive_competency_criteria_group(target.id, user) assert result.archived is True + assert result.cascaded_criteria_count == 1 target.refresh_from_db() criterion.refresh_from_db() assert target.archived is True @@ -478,6 +427,51 @@ def test_status_on_an_ancestor_group_outside_the_subtree_raises_instead_of_archi assert course_level.archived is False +def test_ancestor_cascade_failing_after_the_subtree_delete_rolls_it_back_and_archives_instead( + user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + The ancestor cascade raises IntegrityError after the target subtree and its orphaned ObjectTag are + already deleted, and the second progress check then finds a status on the subtree. The savepoint + around the delete rolls those deletions back, so the subtree is archived and the ObjectTag is kept. + Without the savepoint the deletions would stand and the re-read of the target would find nothing. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + target = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + criterion = _make_criterion(target, object_tag, default_rule_profile) + # The ancestor's own status makes its delete fail on the PROTECT foreign key; hiding it from the + # cascade's check lets the cascade attempt that delete after the subtree's rows are gone. + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=course_level, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + calls = 0 + + def status_arrives_before_the_second_check(group_ids: set[int], criterion_ids: list[int]) -> bool: + nonlocal calls + calls += 1 + if calls == 2: + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + return subtree_has_learner_progress(group_ids, criterion_ids) + + with ( + patch.object(cbe_api, "_group_has_learner_progress", return_value=False), + patch.object(cbe_api, "_subtree_has_learner_progress", side_effect=status_arrives_before_the_second_check), + ): + result = delete_or_archive_competency_criteria_group(target.id, user) + + assert result.archived is True + assert calls == 2 + target.refresh_from_db() + criterion.refresh_from_db() + assert target.archived is True + assert criterion.archived is True + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + def test_integrity_error_unrelated_to_learner_progress_is_raised_instead_of_archiving( user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, ) -> None: @@ -545,18 +539,20 @@ def test_object_tag_is_untouched_on_archive( user, *, tag: Tag, course_run: CourseRun, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, ) -> None: - """Archiving a subtree never calls tag_object(): the object_tag row is left exactly as it was.""" + """Archiving a subtree leaves the criterion's ObjectTag row exactly as it was.""" root = CompetencyCriteriaGroup.objects.create(tag=tag) target = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) criterion = _make_criterion(target, object_tag, default_rule_profile) StudentCompetencyCriterionStatus.objects.create( user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, ) + before = (object_tag.object_id, object_tag.tag_id) result = delete_or_archive_competency_criteria_group(target.id, user) assert result.archived is True - assert ObjectTag.objects.filter(id=object_tag.id).exists() + object_tag.refresh_from_db() + assert (object_tag.object_id, object_tag.tag_id) == before # ================================================================================================= @@ -569,7 +565,7 @@ def test_ancestor_cascade_collapses_the_chain_on_hard_delete( ) -> None: """ Hard-deleting a subtree that was its course-level ancestor's only child collapses that - ancestor and the root above it too, via _cascade_from_group(). + ancestor and the root above it too, via _cascade_remove_from_group(). """ root = CompetencyCriteriaGroup.objects.create(tag=tag) course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) @@ -602,8 +598,6 @@ def test_ancestor_cascade_archives_the_chain_when_the_target_carries_status( root.refresh_from_db() assert course_level.archived is True assert root.archived is True - assert CompetencyCriteriaGroup.objects.filter(id=course_level.id).exists() - assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() def test_cascade_stops_at_an_ancestor_with_a_surviving_sibling_group( @@ -631,8 +625,6 @@ def test_cascade_stops_at_an_ancestor_with_a_surviving_sibling_group( assert course_level.archived is False assert root.archived is False assert sibling.archived is False - assert CompetencyCriteriaGroup.objects.filter(id=course_level.id).exists() - assert CompetencyCriteriaGroup.objects.filter(id=sibling.id).exists() assert CompetencyCriterion.objects.filter(id=sibling_criterion.id).exists() @@ -666,6 +658,42 @@ def test_archive_cascade_stops_at_an_ancestor_with_a_surviving_sibling_group( assert sibling.archived is False +def test_archiving_one_group_leaves_a_sibling_group_with_its_own_status_untouched( + user, *, competency_taxonomy: CompetencyTaxonomy, tag: Tag, course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + Groups A and B sit under one course-level group. A holds one criterion with learner status and one + without, and B holds one criterion with status. Deleting A archives A and both its criteria, and + leaves B, its criterion, the course-level group, the root, and all three ObjectTags untouched. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + group_a = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + group_b = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + object_tags = [_make_object_tag(competency_taxonomy, tag, f"obj-{name}") for name in ("a1", "a2", "b1")] + a_with_status = _make_criterion(group_a, object_tags[0], default_rule_profile) + a_without_status = _make_criterion(group_a, object_tags[1], default_rule_profile) + b_criterion = _make_criterion(group_b, object_tags[2], default_rule_profile) + for graded in (a_with_status, b_criterion): + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=graded, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_or_archive_competency_criteria_group(group_a.id, user) + + assert result.archived is True + assert result.cascaded_group_count == 0 + assert result.cascaded_criteria_count == 2 + for archived_node in (group_a, a_with_status, a_without_status): + archived_node.refresh_from_db() + assert archived_node.archived is True + for untouched_node in (group_b, b_criterion, course_level, root): + untouched_node.refresh_from_db() + assert untouched_node.archived is False + assert ObjectTag.objects.filter(id__in=[object_tag.id for object_tag in object_tags]).count() == 3 + + # ================================================================================================= # Root-group rejection, permission-course resolution, and not-found # ================================================================================================= @@ -691,9 +719,6 @@ def test_root_group_rejection_raises_before_any_write( assert root.archived is False assert child.archived is False assert criterion.archived is False - assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() - assert CompetencyCriteriaGroup.objects.filter(id=child.id).exists() - assert CompetencyCriterion.objects.filter(id=criterion.id).exists() def test_permission_course_rejects_a_root_group(tag: Tag) -> None: diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 6c0d5fb2b..7d564cec1 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -2737,20 +2737,6 @@ def test_group_tag_id_mismatch_404s( assert response.status_code == status.HTTP_404_NOT_FOUND -def test_group_root_target_is_400(user_client: APIClient, tag: Tag, course_run: CourseRun) -> None: - """A root group_id is rejected with a 400, not attempted as a deletable subtree.""" - leaf = _create_leaf_group(tag, course_run) - course_level = leaf.parent - assert course_level is not None - root = course_level.parent - assert root is not None - - response = user_client.delete(group_delete_url(tag.id, root.id)) - - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "group_id" in response.data - - def test_group_delete_no_permission_is_403( user_client: APIClient, tag: Tag, organization: Organization, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, @@ -2772,6 +2758,54 @@ def test_group_delete_no_permission_is_403( assert ObjectTag.objects.filter(id=object_tag.id).exists() +def test_group_delete_without_taxonomy_access_is_403_and_changes_nothing( + user_client: APIClient, leaf: CompetencyCriteriaGroup, batch: tuple[CompetencyCriterion, CompetencyCriterion], +) -> None: + """ + A caller who may tag the course but lacks ``oel_tagging.change_objecttag_taxonomy`` for the + competency's taxonomy gets a 403, and the subtree is untouched. This check is the only one that can + refuse here, because CompetencyReadPermission does not run on DELETE. + """ + course_level = leaf.parent + assert course_level is not None + perm = "oel_tagging.change_objecttag_taxonomy" + original = rules.permissions.permissions[perm] + rules.set_perm(perm, rules.always_deny) + try: + response = user_client.delete(group_delete_url(leaf.tag_id, leaf.id)) + finally: + rules.set_perm(perm, original) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert CompetencyCriteriaGroup.objects.filter(id__in=[leaf.id, course_level.id], archived=False).count() == 2 + assert CompetencyCriterion.objects.filter(id__in=[c.id for c in batch], archived=False).count() == 2 + assert ObjectTag.objects.filter(id__in=[c.object_tag_id for c in batch]).count() == 2 + + +def test_archiving_a_group_with_a_criterion_whose_stored_payload_went_invalid_is_400_and_changes_nothing( + user_client: APIClient, user, leaf: CompetencyCriteriaGroup, + batch: tuple[CompetencyCriterion, CompetencyCriterion], now: datetime, +) -> None: + """ + The archive path saves each 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, + and nothing in the subtree is archived, including the criterion that holds the learner status. + """ + course_level = leaf.parent + assert course_level is not None + CompetencyCriterion.objects.filter(pk=batch[1].pk).update(rule_payload_override={"op": "gt", "value": 75}) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=batch[0], status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + response = user_client.delete(group_delete_url(leaf.tag_id, leaf.id)) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert list(response.json()) == ["__all__"] + assert not CompetencyCriteriaGroup.objects.filter(id__in=[leaf.id, course_level.id], archived=True).exists() + assert not CompetencyCriterion.objects.filter(id__in=[c.id for c in batch], archived=True).exists() + + def test_group_delete_of_a_group_gone_by_the_time_of_the_call_is_404( user_client: APIClient, tag: Tag, course_run: CourseRun, ) -> None: @@ -2870,7 +2904,10 @@ def test_group_delete_checks_permission_against_the_parents_course_for_a_leaf_gr def test_group_root_target_is_400_before_any_permission_check_or_write( user_client: APIClient, tag: Tag, course_run: CourseRun, checked_object_ids: list[str], ) -> None: - """A root group_id is rejected before the permission predicate runs, and nothing under it changes.""" + """ + A root group_id is rejected with a 400 that says root groups cannot be targeted, before the + permission predicate runs, and nothing under it changes. + """ leaf = _create_leaf_group(tag, course_run) course_level = leaf.parent assert course_level is not None @@ -2880,6 +2917,9 @@ def test_group_root_target_is_400_before_any_permission_check_or_write( response = user_client.delete(group_delete_url(tag.id, root.id)) assert response.status_code == status.HTTP_400_BAD_REQUEST + assert [str(message) for message in response.data["group_id"]] == [ + "group_id must not reference a root CompetencyCriteriaGroup." + ] assert not checked_object_ids assert set( CompetencyCriteriaGroup.objects.filter(archived=False).values_list("id", flat=True)