diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index ef9f69562..2d9b65c63 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 _ @@ -19,11 +19,12 @@ 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 -from .data import CriterionDeletionResult, GradedObjectScore +from .data import CriterionDeletionResult, GradedObjectScore, GroupDeletionResult from .models import ( CompetencyCourseRun, CompetencyCriteriaGroup, @@ -46,10 +47,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", @@ -483,10 +486,12 @@ 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 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. 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 @@ -506,16 +511,14 @@ 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) + 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 - object_tag = ObjectTag.objects.get(object_id=object_id, taxonomy_id=tag.taxonomy_id, tag_id=tag.id) 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) @@ -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: @@ -1158,3 +1171,186 @@ 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``. + + 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. + + 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 _competency_criteria_subtree(target: CompetencyCriteriaGroup) -> tuple[set[int], list[CompetencyCriterion]]: + """ + Return the ids of ``target`` and its descendant groups (found level by level), and the subtree's criteria. + + 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] + while level_ids: + level_ids = list( + CompetencyCriteriaGroup.objects.filter(parent_id__in=level_ids).values_list("pk", flat=True) + ) + subtree_ids.update(level_ids) + criteria = list(CompetencyCriterion.objects.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_objecttag_ids: set[int]) -> None: + """ + Delete each ObjectTag in ``candidate_objecttag_ids`` that no CompetencyCriterion references, and nothing else. + + 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_objecttag_ids, + ).values_list("object_tag_id", flat=True) + ) + ObjectTag.objects.filter(pk__in=candidate_objecttag_ids - still_referenced).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 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, + 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). + + 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 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 + 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. + + 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) + # 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(): + # 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] + 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. + 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: + _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 + 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 should_archive: + _archive_competency_criteria_subtree(target_group, subtree_ids, criteria, user) + return GroupDeletionResult( + id=group_id, + 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 cc01fc7c0..0c939c291 100644 --- a/src/openedx_learning/applets/cbe/data.py +++ b/src/openedx_learning/applets/cbe/data.py @@ -49,3 +49,22 @@ 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`` 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. + """ + + id: int + archived: bool + cascaded_group_count: int + cascaded_criteria_count: int 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..7b5a77992 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, @@ -230,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] @@ -279,6 +288,39 @@ 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 by the `group_id` and `tag_id` in the URL, then delegate to the public API. + + 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"], + ) + 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 + except IntegrityError as exc: + # 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 + 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): """ 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_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_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..869ba8095 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_group_removal.py @@ -0,0 +1,800 @@ +"""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 +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 +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, + 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) + + +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 +# ================================================================================================= + + +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_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, +) -> 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_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, *, 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 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) + 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 + ) + + 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_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 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) + 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 + 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_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_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: + """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, +) -> 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 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 + object_tag.refresh_from_db() + assert (object_tag.object_id, object_tag.tag_id) == before + + +# ================================================================================================= +# 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_remove_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 + + +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 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 + + +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 +# ================================================================================================= + + +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 + + +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..7d564cec1 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 @@ -30,6 +32,7 @@ LogicOperator, MasteryStatus, RuleType, + StudentCompetencyCriteriaGroupStatus, StudentCompetencyCriterionStatus, ) from openedx_tagging.models import ObjectTag, Tag, Taxonomy @@ -86,6 +89,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 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 # 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"} @@ -2027,11 +2035,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 @@ -2573,7 +2581,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, @@ -2593,9 +2601,363 @@ 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 + + +# ============================================================================================== +# CompetencyCriteriaGroupDetailView DELETE (#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_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_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: + """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() + + +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.""" + 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 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 + 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 [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) + ) == {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} 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