feat: add endpoint to delete or archive a CompetencyCriteriaGroup (#675)

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

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/<tag_id>/criteria-groups/<group_id>/
         ``{"logic_operator": "AND"}``
 
-    The response is the group as stored, in the criteria-tree endpoint's shape. ``logic_operator``
+        DELETE api/cbe/v1/competencies/<tag_id>/criteria-groups/<group_id>/
+
+    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):
     """

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,

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:

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
 # =================================================================================================