From 27dfbac50c61bc5ec9ee6ba7ac6226f4312d7d99 Mon Sep 17 00:00:00 2001 From: javoconsultant Date: Thu, 29 Oct 2026 15:48:36 +0500 Subject: [PATCH] feat: add PATCH endpoint for a competency criteria group's logic_operator (#760) Add PATCH /api/cbe/v1/competencies//criteria-groups// and update_competency_criteria_group(), which let an author change how one CompetencyCriteriaGroup combines the criteria and groups beneath it by setting its logic_operator to AND or OR. Nothing else about the group or its subtree changes, and no StudentCompetency*Status row is read or written. - The view is a thin adapter. It resolves the competency from the URL and checks oel_tagging.view_taxonomy, as the criteria-tree endpoint (#681) does, before it looks up the group or compares the body, since both reveal what is stored. update_competency_criteria_group() makes every other refusal, including the oel_tagging.can_tag_object edit check, so an in-process caller such as Studio is refused in the same cases as an HTTP caller. - logic_operator is required and is the only field that can change. The body may also restate the other fields the criteria-tree endpoint returns for a group (id, parent_id, tag_id, course_key, name, ordering), so a client can send back the object it read. Each restated field must equal the stored value exactly (a name is compared without trimming). A field that differs, or a key outside that representation, is a 400 that names it. The response is the stored group in the criteria-tree shape. - A missing, null, or non-AND/OR logic_operator is a 400, as is a root group or a group with no course at or above it. A tag that is not a competency tag, an unknown group, or a group under a different competency is a 404. An archived group is a 409, through the new public CompetencyCriteriaGroupArchivedError and the existing CompetencyCriteriaConflict, even when the request would change nothing. - The permission course is a course-level group's own course, and for any group below one, the course of its nearest ancestor that has one, however deeply it is nested. - The update is one save(update_fields=["logic_operator"]) inside transaction.atomic(), with _history_user set to the caller, so django-simple-history records exactly one row per real change, attributed to that user (ADR 0003 Decision 1). Asking for the operator the group already has returns 200 without saving, so it adds no history row. - update_competency_criteria_group() checks for a root group, resolves the course, checks permission, validates the operator value, refuses an archived group, and then skips the save if nothing changed. The archived check runs before the no-op check so an archived group is never reported as successfully edited. Deviations from #760: - The ticket lists course_id, oel_tagging_tag_id, and archived among the fields a client may restate. The endpoint instead accepts the fields the criteria-tree endpoint returns, under their REST names (course_key, tag_id), plus id. archived is not part of that representation, so a body that includes it is rejected as an unrecognized key. - The ticket resolves the permission course from either a course-level group or a leaf. The endpoint walks up to the nearest ancestor with a course, so a group nested below a leaf can also be edited. A non-root group with no course anywhere above it is rejected with 400 before the permission check. - The ticket orders the permission check before request validation. Here it is split in two: the view checks read access first, then the body's shape (missing or null operator, unrecognized keys, changed fixed fields), which stays in the view because only a REST body carries the fixed fields. The edit check runs after that, inside the API function, followed by the operator value check. A caller who can read the tree but cannot edit the group's course therefore gets 400, not 403, for a malformed body. Related: #675 removes a group and its subtree at the same path. Django dispatches a path to the first pattern that matches it, so whichever PR lands second should add its method to the other PR's view class rather than register a second route. If this lands first, #675 adds delete() to CompetencyCriteriaGroupDetailView and "delete" to its http_method_names. #723 (Epic 10) owns the learner-status warning that will sit in front of this endpoint. --- src/openedx_learning/applets/cbe/api.py | 83 +++++ .../applets/cbe/rest_api/v1/serializers.py | 32 ++ .../applets/cbe/rest_api/v1/urls.py | 5 + .../applets/cbe/rest_api/v1/views.py | 56 +++ .../openedx_learning/applets/cbe/test_api.py | 314 ++++++++++++++++ .../applets/cbe/test_views.py | 344 +++++++++++++++++- 6 files changed, 833 insertions(+), 1 deletion(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index abe29ebf8..09503ff6d 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -37,6 +37,7 @@ from .rule_payloads import evaluate_rule __all__ = [ + "CompetencyCriteriaGroupArchivedError", "CompetencyCriteriaTree", "CompetencyCriterionArchivedError", "GradedObjectScore", @@ -52,6 +53,7 @@ "resolve_competency_tag", "resolve_supplied_leaf_group", "select_competency_taxonomies", + "update_competency_criteria_group", ] log = logging.getLogger(__name__) @@ -67,6 +69,15 @@ class CompetencyCriterionArchivedError(Exception): """ +class CompetencyCriteriaGroupArchivedError(Exception): + """ + Raised when a request would edit a CompetencyCriteriaGroup that has been archived. + + UNSTABLE: the criteria-authoring family is incomplete, so its archive behavior may change this + exception, or when it is raised, without a deprecation cycle. + """ + + def create_competency_taxonomy( # pylint: disable=too-many-positional-arguments name: str, description: str | None = None, @@ -781,3 +792,75 @@ def record_graded_object_statuses(*, user_id: int, scores: Sequence[GradedObject changed += _raise_leaf_status(user_id, criterion.pk, status, now) return changed + + +def update_competency_criteria_group( + group_id: int, + *, + logic_operator: str, + user: UserType, +) -> CompetencyCriteriaGroup: + """ + Set how the CompetencyCriteriaGroup ``group_id`` names combines its children, and return it. + + UNSTABLE: the criteria-authoring family is incomplete, so its validation and archive behavior + may change this function's shape without a deprecation cycle. + + ``logic_operator`` is the only thing this changes. The group's name, ordering, competency, + course, and parent stay as they were created, and so does everything beneath it. A request + that leaves ``logic_operator`` as stored saves nothing, so the group's history gains a row + only for a real change. Learner status is never read or written. + + Raises CompetencyCriteriaGroup.DoesNotExist if no group has ``group_id``, or Tag.DoesNotExist + if its tag isn't on a CompetencyTaxonomy. + Raises ValidationError, keyed by the REST field names, for a root group, a group with no course + anywhere above it to check permission against, or a ``logic_operator`` other than AND or OR, and + PermissionDenied if ``user`` fails ``oel_tagging.can_tag_object`` for the group's taxonomy and + course. Raises CompetencyCriteriaGroupArchivedError if the group is archived, even when nothing + would change. + """ + with transaction.atomic(): + group = CompetencyCriteriaGroup.objects.select_related("course", "parent__course").get(pk=group_id) + # Nothing in the model stops a group's tag being on another kind of taxonomy, so it's checked + # the way the create and tree endpoints check theirs. + tag = resolve_competency_tag(group.tag_id) + # tag.taxonomy is already confirmed set; re-asserted since that doesn't cross function boundaries for mypy. + assert tag.taxonomy is not None + # A root has neither a parent nor a course, and its subtree can span several courses, so + # there's no one course to check permission against. + if group.parent_id is None and group.course_id is None: + raise ValidationError({"group_id": _("A root CompetencyCriteriaGroup cannot be targeted directly.")}) + # A course-level group carries its own course, and a group beneath one takes the course of + # its nearest ancestor that has one, however deeply it's nested. + ancestor = group + while ancestor.course is None and ancestor.parent is not None: + ancestor = ancestor.parent + course_run = ancestor.course + if course_run is None: + raise ValidationError( + {"group_id": _("This CompetencyCriteriaGroup has no course above it to check permission against.")} + ) + perm_obj = ObjectTagPermissionItem(taxonomy=tag.taxonomy, object_id=str(course_run.course_key)) + # has_perm()'s stub expects a Model, but a rules predicate takes any object. + if not user.has_perm("oel_tagging.can_tag_object", perm_obj): # type: ignore[arg-type] + raise PermissionDenied + + # Null is a stored state (a single-child group), but never one a caller may ask for. + if logic_operator not in LogicOperator.values: + raise ValidationError( + {"logic_operator": _("logic_operator must be one of: {choices}.").format( + choices=", ".join(LogicOperator.values), + )} + ) + # ADR-0002 Decision 2 hides archived groups from authoring; nothing reopens one here. + if group.archived: + raise CompetencyCriteriaGroupArchivedError(_("An archived CompetencyCriteriaGroup cannot be edited.")) + if group.logic_operator == logic_operator: + return group + + group.logic_operator = logic_operator + # Set on the instance so attribution doesn't depend on simple_history's request middleware. + group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + # Never QuerySet.update(): it skips post_save, so simple_history would record nothing. + group.save(update_fields=["logic_operator"]) + return group diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py index 0c700c1fa..347020326 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py @@ -141,6 +141,38 @@ class Meta: fields = ["id", "parent_id", "tag_id", "course_key", "name", "ordering", "logic_operator"] +class CompetencyCriteriaGroupUpdateSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + Parse the group-update request body against the stored group, which is this serializer's instance. + + ``logic_operator`` is required and is the only field that can change. Its type is checked here, + and its value is checked by :func:`update_competency_criteria_group`. Every other field that + CompetencyCriteriaGroupSerializer emits is accepted too, so a client can send back the group it + read. Each one must match what is stored, and the error names any that don't. A mismatch is a + rejected edit, not something to ignore silently. + """ + + logic_operator = serializers.CharField() + id = serializers.IntegerField(required=False) + parent_id = serializers.IntegerField(required=False, allow_null=True) + tag_id = serializers.IntegerField(required=False) + course_key = serializers.CharField(required=False, allow_null=True) + # Compared as sent: trimming would turn an exact echo of a padded name into an apparent rename. + name = serializers.CharField(required=False, allow_blank=True, trim_whitespace=False) + ordering = serializers.IntegerField(required=False) + + def validate(self, attrs: dict) -> dict: + """Reject any key this serializer does not declare, and any fixed field that differs from the stored one.""" + unrecognized = sorted(set(self.initial_data) - set(self.fields)) + if unrecognized: + raise serializers.ValidationError({key: "This field is not recognized." for key in unrecognized}) + stored = CompetencyCriteriaGroupSerializer(self.instance).data + changed = sorted(key for key, value in attrs.items() if key != "logic_operator" and value != stored[key]) + if changed: + raise serializers.ValidationError({key: "This field cannot be changed." for key in changed}) + return attrs + + class CompetencyCriterionReadSerializer(serializers.ModelSerializer): """ Read-only representation of a CompetencyCriterion, for the criteria-tree read endpoint. diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py index 658cab5e0..e96d754a1 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py @@ -27,4 +27,9 @@ views.CompetencyCriteriaTreeView.as_view(), name="criteria-tree", ), + path( + "competencies//criteria-groups//", + views.CompetencyCriteriaGroupDetailView.as_view(), + name="criteria-group-detail", + ), ] 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 a232cfd83..715009016 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -5,6 +5,7 @@ from django.core.exceptions import ValidationError as DjangoValidationError from django.db.models import QuerySet +from django.shortcuts import get_object_or_404 from django.utils.translation import gettext_lazy as _ from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication # type: ignore[import] from edx_rest_framework_extensions.auth.session.authentication import ( # type: ignore[import] @@ -21,18 +22,21 @@ from openedx_tagging.rules import ObjectTagPermissionItem from ...api import ( + CompetencyCriteriaGroupArchivedError, CompetencyCriterionArchivedError, associate_competency_criterion, bulk_update_competency_criteria, get_competency_criteria_tree, get_competency_rule_profiles, resolve_competency_tag, + update_competency_criteria_group, ) from ...models import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile from ..paginators import CompetencyRuleProfilePagination from .permissions import CompetencyReadPermission, CompetencyRuleProfilePermissions from .serializers import ( CompetencyCriteriaGroupSerializer, + CompetencyCriteriaGroupUpdateSerializer, CompetencyCriterionBulkUpdateSerializer, CompetencyCriterionReadSerializer, CompetencyCriterionSerializer, @@ -198,3 +202,55 @@ def get(self, request, tag_id): "groups": CompetencyCriteriaGroupSerializer(tree.groups, many=True).data, "criteria": CompetencyCriterionReadSerializer(tree.criteria, many=True).data, }) + + +class CompetencyCriteriaGroupDetailView(generics.GenericAPIView): + """ + PATCH-only. Change how one CompetencyCriteriaGroup combines the criteria and groups beneath it. + + **Example Request** + 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`` + 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. + """ + + serializer_class = CompetencyCriteriaGroupUpdateSerializer + http_method_names = ["patch"] + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = [IsAuthenticated, CompetencyReadPermission] + + def patch(self, request, tag_id: int, group_id: int): + """Check the body against the stored group, then delegate to the public API.""" + # Same 404 the create and tree endpoints give a tag that isn't a competency, ahead of the body checks. + try: + tag = resolve_competency_tag(tag_id) + except Tag.DoesNotExist as exc: + raise NotFound(str(exc)) from exc + # The group lookup and body check below reveal what's stored, so only a tree reader may reach them. + self.check_object_permissions(request, tag) + # Scoped by tag_id, so addressing a group under a competency it doesn't belong to is a 404. + group = get_object_or_404( + CompetencyCriteriaGroup.objects.select_related("course"), pk=group_id, tag_id=tag_id, + ) + serializer = self.get_serializer(group, data=request.data) + serializer.is_valid(raise_exception=True) + try: + group = update_competency_criteria_group( + group.id, logic_operator=serializer.validated_data["logic_operator"], user=request.user, + ) + except CompetencyCriteriaGroup.DoesNotExist as exc: + raise NotFound(str(exc)) from exc + except DjangoValidationError as exc: + raise DRFValidationError(exc.message_dict if hasattr(exc, "message_dict") else exc.messages) from exc + except CompetencyCriteriaGroupArchivedError as exc: + raise CompetencyCriteriaConflict(str(exc)) from exc + return Response(CompetencyCriteriaGroupSerializer(group).data, status=status.HTTP_200_OK) diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index f4dbcbd68..1b934087b 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -10,12 +10,14 @@ from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError from django.db import connection from django.db.utils import IntegrityError +from django.forms.models import model_to_dict from django.test.utils import CaptureQueriesContext from organizations.models import Organization from rules.permissions import permissions as rule_permissions from openedx_catalog.models import CatalogCourse, CourseRun from openedx_learning.api import ( + CompetencyCriteriaGroupArchivedError, CompetencyCriterionArchivedError, associate_competency_criterion, bulk_update_competency_criteria, @@ -27,6 +29,7 @@ is_competency_taxonomy, resolve_supplied_leaf_group, select_competency_taxonomies, + update_competency_criteria_group, ) from openedx_learning.applets.cbe import api as cbe_api from openedx_learning.applets.cbe.api import _create_leaf_group @@ -1557,3 +1560,314 @@ def test_competency_course_run_is_deleted_with_its_course_run( course_run.delete() assert not CompetencyCourseRun.objects.exists() + + +# ============================================================================================== +# update_competency_criteria_group (#760) +# ============================================================================================== + +def course_level_of(leaf: CompetencyCriteriaGroup) -> CompetencyCriteriaGroup: + """Return the course-level group `leaf` sits under.""" + assert leaf.parent is not None + return leaf.parent + + +def root_of(leaf: CompetencyCriteriaGroup) -> CompetencyCriteriaGroup: + """Return the root group of `leaf`'s tree.""" + root = course_level_of(leaf).parent + assert root is not None + return root + + +def with_operator(group: CompetencyCriteriaGroup, logic_operator: str | None) -> CompetencyCriteriaGroup: + """Store `logic_operator` on `group` directly, as the starting state for a test.""" + group.logic_operator = logic_operator + group.save() + return group + + +def group_history(group: CompetencyCriteriaGroup): + """ + Return `group`'s history rows, newest first. + + Looked up through the app registry, as the model tests do, because simple_history's `.history` + descriptor has no type stubs. + """ + historical_group = apps.get_model("openedx_learning", "HistoricalCompetencyCriteriaGroup") + return historical_group.objects.filter(id=group.pk).order_by("-history_date", "-history_id") + + +def stored(group: CompetencyCriteriaGroup) -> tuple[dict, int]: + """Return `group`'s columns as stored and its history length, to show what a call changed.""" + return model_to_dict(CompetencyCriteriaGroup.objects.get(pk=group.pk)), group_history(group).count() + + +@pytest.mark.parametrize("level", ["course-level", "leaf"]) +@pytest.mark.parametrize( + "before, after", [(LogicOperator.AND, LogicOperator.OR), (LogicOperator.OR, LogicOperator.AND)], +) +def test_update_switches_how_a_group_combines_its_children( + level: str, before: str, after: str, leaf: CompetencyCriteriaGroup, user, +) -> None: + """Either operator replaces the other, on a course-level group and on a leaf alike.""" + group = with_operator(course_level_of(leaf) if level == "course-level" else leaf, before) + + result = update_competency_criteria_group(group.id, logic_operator=after, user=user) + + assert result.logic_operator == after + group.refresh_from_db() + assert group.logic_operator == after + + +def test_update_writes_one_history_row_attributed_to_the_caller( + leaf: CompetencyCriteriaGroup, user, +) -> None: + """A real change is one save, so one history row, attributed without any request middleware.""" + rows_before = group_history(leaf).count() + + update_competency_criteria_group(leaf.id, logic_operator=LogicOperator.AND, user=user) + + assert group_history(leaf).count() == rows_before + 1 + latest = group_history(leaf).first() + assert latest.history_user == user + assert latest.logic_operator == LogicOperator.AND + + +def test_update_that_changes_nothing_saves_nothing(leaf: CompetencyCriteriaGroup, user) -> None: + """Asking for the operator a group already has succeeds without adding a history row.""" + assert leaf.logic_operator == LogicOperator.OR + before = stored(leaf) + + result = update_competency_criteria_group(leaf.id, logic_operator=LogicOperator.OR, user=user) + + assert result.logic_operator == LogicOperator.OR + assert stored(leaf) == before + + +def test_update_sets_an_unset_operator_even_to_or(leaf: CompetencyCriteriaGroup, user) -> None: + """ + A null operator given OR is a real change. + + Null is evaluated like OR, but it is stored differently, and the caller asked for an explicit value. + """ + course_level = course_level_of(leaf) + assert course_level.logic_operator is None + rows_before = group_history(course_level).count() + + update_competency_criteria_group(course_level.id, logic_operator=LogicOperator.OR, user=user) + + course_level.refresh_from_db() + assert course_level.logic_operator == LogicOperator.OR + assert group_history(course_level).count() == rows_before + 1 + + +def test_update_leaves_everything_else_about_the_group_and_its_branch_unchanged( + leaf: CompetencyCriteriaGroup, user, default_rule_profile: CompetencyRuleProfile, +) -> None: + """Only the operator changes: not the group's name, ordering, competency, course, or parent, nor anything below.""" + course_level = with_operator(course_level_of(leaf), LogicOperator.AND) + # A non-default ordering, so that a reset to the default would show. + course_level.ordering = 3 + course_level.save() + criterion = make_criterion(leaf, "p1", rule_profile=default_rule_profile) + group_before, _ = stored(course_level) + leaf_before = stored(leaf) + criterion_before = model_to_dict(criterion) + + update_competency_criteria_group(course_level.id, logic_operator=LogicOperator.OR, user=user) + + group_after, _ = stored(course_level) + assert group_after == {**group_before, "logic_operator": LogicOperator.OR} + assert stored(leaf) == leaf_before + assert model_to_dict(CompetencyCriterion.objects.get(pk=criterion.pk)) == criterion_before + + +@pytest.mark.parametrize("children", [0, 1]) +def test_update_accepts_a_group_with_fewer_than_two_children( + children: int, leaf: CompetencyCriteriaGroup, user, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A group's child count is creation's and deletion's concern, so it never blocks this edit.""" + for n in range(children): + make_criterion(leaf, f"p{n}", rule_profile=default_rule_profile) + + result = update_competency_criteria_group(leaf.id, logic_operator=LogicOperator.AND, user=user) + + assert result.logic_operator == LogicOperator.AND + + +@pytest.mark.parametrize("logic_operator", [None, "", "XOR", "and", 1]) +def test_update_rejects_an_operator_the_group_cannot_store( + logic_operator, leaf: CompetencyCriteriaGroup, user, +) -> None: + """Anything but AND or OR is refused, keyed by the field, and the group keeps its operator.""" + before = stored(leaf) + + with pytest.raises(ValidationError) as exc_info: + update_competency_criteria_group(leaf.id, logic_operator=logic_operator, user=user) + + assert "logic_operator" in exc_info.value.message_dict + assert stored(leaf) == before + + +@pytest.mark.parametrize("logic_operator", [LogicOperator.OR, LogicOperator.AND]) +def test_update_refuses_an_archived_group_even_when_nothing_would_change( + logic_operator: str, leaf: CompetencyCriteriaGroup, user, +) -> None: + """ + An archived group is refused outright, whatever the request. + + Asking for the operator it already has is refused too. That pins the archived check ahead of the + no-op check, since a success would suggest the edit was accepted. + """ + leaf.archived = True + leaf.save() + before = stored(leaf) + + with pytest.raises(CompetencyCriteriaGroupArchivedError): + update_competency_criteria_group(leaf.id, logic_operator=logic_operator, user=user) + + assert stored(leaf) == before + + +def test_update_rejects_a_root_group_before_checking_permission( + checked_object_ids: list[str], leaf: CompetencyCriteriaGroup, user, +) -> None: + """A root spans every course beneath it, so it is refused before any permission check or write.""" + root = with_operator(root_of(leaf), LogicOperator.AND) + before = stored(root) + + with pytest.raises(ValidationError, match="root") as exc_info: + update_competency_criteria_group(root.id, logic_operator=LogicOperator.OR, user=user) + + assert "group_id" in exc_info.value.message_dict + assert not checked_object_ids + assert stored(root) == before + + +def test_update_does_not_mistake_a_course_less_child_of_the_root_for_a_root( + checked_object_ids: list[str], tag: Tag, leaf: CompetencyCriteriaGroup, user, +) -> None: + """ + A group with a parent is no root, even with no course of its own and none above it. + + The model and ADR-0002 allow such a group, but with no course anywhere above it there's nothing to check + permission against. So it's refused, for that reason and not as a root, before any permission check or write. + """ + course_less = CompetencyCriteriaGroup.objects.create( + tag=tag, parent=root_of(leaf), logic_operator=LogicOperator.AND, + ) + before = stored(course_less) + + with pytest.raises(ValidationError, match="no course") as exc_info: + update_competency_criteria_group(course_less.id, logic_operator=LogicOperator.OR, user=user) + + assert "root" not in str(exc_info.value) + assert "group_id" in exc_info.value.message_dict + assert not checked_object_ids + assert stored(course_less) == before + + +def test_update_resolves_the_course_of_a_group_nested_below_a_leaf( + checked_object_ids: list[str], leaf: CompetencyCriteriaGroup, user, course_run: CourseRun, +) -> None: + """ADR-0002 supports deeply nested groups, so one below a leaf takes the course of its nearest ancestor with one.""" + nested = CompetencyCriteriaGroup.objects.create(tag=leaf.tag, parent=leaf, logic_operator=LogicOperator.OR) + + result = update_competency_criteria_group(nested.id, logic_operator=LogicOperator.AND, user=user) + + assert result.logic_operator == LogicOperator.AND + nested.refresh_from_db() + assert nested.logic_operator == LogicOperator.AND + assert checked_object_ids == [str(course_run.course_key)] + + +def test_update_404s_for_a_group_whose_tag_is_not_on_a_competency_taxonomy( + checked_object_ids: list[str], course_run: CourseRun, user, +) -> None: + """A group under any other kind of taxonomy isn't a competency's group, so it's missing, and nothing changes.""" + plain_tag = Tag.objects.create(taxonomy=Taxonomy.objects.create(name="Plain Tags", export_id="plain-v1"), value="x") + root = CompetencyCriteriaGroup.objects.create(tag=plain_tag, parent=None) + # Shaped like any editable group, so only its taxonomy sets it apart. + course_level = CompetencyCriteriaGroup.objects.create( + tag=plain_tag, course=course_run, parent=root, logic_operator=LogicOperator.AND, + ) + before = stored(course_level) + + with pytest.raises(Tag.DoesNotExist): + update_competency_criteria_group(course_level.id, logic_operator=LogicOperator.OR, user=user) + + assert not checked_object_ids + assert stored(course_level) == before + + +def test_update_raises_does_not_exist_for_an_unknown_group(user) -> None: + """An unknown group id raises CompetencyCriteriaGroup.DoesNotExist.""" + with pytest.raises(CompetencyCriteriaGroup.DoesNotExist): + update_competency_criteria_group(999999, logic_operator=LogicOperator.AND, user=user) + + +@pytest.mark.parametrize("level", ["course-level", "leaf"]) +def test_update_checks_permission_against_the_groups_course( # pylint: disable=too-many-positional-arguments + level: str, + checked_object_ids: list[str], + leaf: CompetencyCriteriaGroup, + user, + course_run: CourseRun, + organization: Organization, +) -> None: + """A course-level group's own course is checked, and a leaf's is resolved through its course-level parent.""" + # Another course under the same root, so checking any course but the group's own would show. + _create_leaf_group(leaf.tag, make_course_run(organization, "Python200", "Fall2026")) + group = course_level_of(leaf) if level == "course-level" else leaf + + update_competency_criteria_group(group.id, logic_operator=LogicOperator.AND, user=user) + + assert checked_object_ids == [str(course_run.course_key)] + + +@pytest.mark.parametrize("lacks", ["course write access", "taxonomy view access"]) +def test_update_refuses_a_user_without_permission_changing_nothing( + lacks: str, leaf: CompetencyCriteriaGroup, user, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """A user failing oel_tagging.can_tag_object's taxonomy check or object_id check is refused, and nothing changes.""" + if lacks == "course write access": + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, lambda _user, _object_id: False) + else: + # can_tag_object's taxonomy check fails on a disabled taxonomy for anyone but a superuser. + competency_taxonomy.enabled = False + competency_taxonomy.save() + before = stored(leaf) + + with pytest.raises(PermissionDenied): + update_competency_criteria_group(leaf.id, logic_operator=LogicOperator.AND, user=user) + + assert stored(leaf) == before + + +@pytest.mark.parametrize("path", ["changed", "no-op", "archived", "root", "unsupported operator"]) +def test_update_never_reads_or_writes_learner_status( + path: str, leaf: CompetencyCriteriaGroup, user, +) -> None: + """ + No path issues a query against a StudentCompetency*Status table. + + Whether learners have progress under a group plays no part in this edit; warning about that is #723's job. + """ + group, logic_operator = leaf, str(LogicOperator.AND) + if path == "no-op": + logic_operator = LogicOperator.OR + elif path == "archived": + leaf.archived = True + leaf.save() + elif path == "root": + group = root_of(leaf) + elif path == "unsupported operator": + logic_operator = "XOR" + + with CaptureQueriesContext(connection) as queries: + try: + update_competency_criteria_group(group.id, logic_operator=logic_operator, user=user) + except (ValidationError, CompetencyCriteriaGroupArchivedError): + pass + + assert not [q for q in queries.captured_queries if "studentcompetency" in q["sql"].lower()] diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 983d252ae..caf312d02 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -47,7 +47,7 @@ def _predicate(_user, object_id: str) -> bool: UsageKey.from_string(object_id) except InvalidKeyError: try: - # The bulk-update endpoint checks permission against the group's course key. + # The bulk-update and group-update endpoints check permission against the group's course key. CourseKey.from_string(object_id) except InvalidKeyError: return False @@ -1480,3 +1480,345 @@ def test_the_same_tag_with_two_different_criteria_in_the_same_course_is_allowed( response = user_client.post(criterion_create_url(tag.id), {"object_id": usage_key(course_run, "p2")}, format="json") assert response.status_code == status.HTTP_201_CREATED + + +# ============================================================================================== +# CompetencyCriteriaGroupDetailView (#760) +# ============================================================================================== + +GROUP_FIELDS = {"id", "parent_id", "tag_id", "course_key", "name", "ordering", "logic_operator"} + + +def group_detail_url(tag_id: int, group_id: int) -> str: + """Return the group detail endpoint's path, resolved through the URL name.""" + return reverse("cbe:criteria-group-detail", kwargs={"tag_id": tag_id, "group_id": group_id}) + + +@pytest.fixture(name="course_level") +def _course_level(leaf: CompetencyCriteriaGroup) -> CompetencyCriteriaGroup: + """The course-level group `leaf` sits under, combining its children with AND.""" + course_level = leaf.parent + assert course_level is not None + course_level.logic_operator = LogicOperator.AND + course_level.save() + return course_level + + +def stored_group(group: CompetencyCriteriaGroup) -> tuple: + """Return `group`'s columns as stored, to prove a refused request changed nothing.""" + group.refresh_from_db() + return ( + group.logic_operator, group.name, group.ordering, group.tag_id, group.course_id, group.parent_id, + group.archived, + ) + + +def test_group_detail_resolves_to_the_documented_path() -> None: + """The group's own route sits under the competency's criteria-groups collection.""" + assert group_detail_url(7, 9) == "/api/cbe/v1/competencies/7/criteria-groups/9/" + + +@pytest.mark.parametrize( + "before, after", [(LogicOperator.AND, LogicOperator.OR), (LogicOperator.OR, LogicOperator.AND)], +) +def test_update_group_switches_its_operator_and_echoes_the_rest( # pylint: disable=too-many-positional-arguments + before: str, + after: str, + user_client: APIClient, + tag: Tag, + course_run: CourseRun, + course_level: CompetencyCriteriaGroup, +) -> None: + """The response is the group as stored: the new operator, everything else as before.""" + course_level.logic_operator = before + course_level.save() + + response = user_client.patch(group_detail_url(tag.id, course_level.id), {"logic_operator": after}, format="json") + + assert response.status_code == status.HTTP_200_OK + assert set(response.data.keys()) == GROUP_FIELDS + assert response.data["logic_operator"] == after + assert response.data["id"] == course_level.id + assert response.data["name"] == course_level.name + assert response.data["ordering"] == course_level.ordering + assert response.data["tag_id"] == tag.id + assert response.data["course_key"] == str(course_run.course_key) + assert response.data["parent_id"] == course_level.parent_id + + +def test_update_group_accepts_back_the_representation_the_tree_endpoint_gave( + user_client: APIClient, tag: Tag, course_level: CompetencyCriteriaGroup, +) -> None: + """A client can read the group, change only its operator, and send the whole object back.""" + tree = user_client.get(criteria_tree_url(tag.id)) + read = next(group for group in tree.data["groups"] if group["id"] == course_level.id) + + response = user_client.patch( + group_detail_url(tag.id, course_level.id), {**read, "logic_operator": LogicOperator.OR}, format="json", + ) + + assert response.status_code == status.HTTP_200_OK + assert response.data == {**read, "logic_operator": LogicOperator.OR} + + +@pytest.mark.parametrize("sent, expected_status", [ + (" Padded name ", status.HTTP_200_OK), + ("Padded name", status.HTTP_400_BAD_REQUEST), + (" padded name ", status.HTTP_400_BAD_REQUEST), +]) +def test_update_group_compares_a_restated_name_exactly( + sent: str, + expected_status: int, + user_client: APIClient, + tag: Tag, + course_level: CompetencyCriteriaGroup, +) -> None: + """A restated name matches only if it is identical, so neither trimming nor a change of case gets through.""" + course_level.name = " Padded name " + course_level.save() + + response = user_client.patch( + group_detail_url(tag.id, course_level.id), {"logic_operator": LogicOperator.OR, "name": sent}, format="json", + ) + + assert response.status_code == expected_status + + +def test_update_group_that_changes_nothing_is_200( + user_client: APIClient, tag: Tag, course_level: CompetencyCriteriaGroup, +) -> None: + """Sending the operator the group already has succeeds and reports it.""" + response = user_client.patch( + group_detail_url(tag.id, course_level.id), {"logic_operator": LogicOperator.AND}, format="json", + ) + + assert response.status_code == status.HTTP_200_OK + assert response.data["logic_operator"] == LogicOperator.AND + + +@pytest.mark.parametrize( + "case, error_key", + [ + ("missing operator", "logic_operator"), + ("null operator", "logic_operator"), + ("unsupported operator", "logic_operator"), + ("unrecognized key", "children"), + ("rename", "name"), + ("reorder", "ordering"), + ("re-parent", "parent_id"), + ("change course", "course_key"), + ("change competency", "tag_id"), + ("archive", "archived"), + ("change id", "id"), + ], +) +def test_update_group_rejects_a_malformed_body_with_400( # pylint: disable=too-many-positional-arguments + case: str, + error_key: str, + user_client: APIClient, + tag: Tag, + leaf: CompetencyCriteriaGroup, + course_level: CompetencyCriteriaGroup, +) -> None: + """Each malformed body is a 400 naming the offending field, and the group is unchanged.""" + change = {"logic_operator": LogicOperator.OR} + bodies: dict[str, dict[str, Any]] = { + "missing operator": {}, + "null operator": {"logic_operator": None}, + "unsupported operator": {"logic_operator": "XOR"}, + # A group's membership is not this endpoint's to change. + "unrecognized key": {**change, "children": [leaf.id]}, + "rename": {**change, "name": "Renamed"}, + "reorder": {**change, "ordering": course_level.ordering + 1}, + "re-parent": {**change, "parent_id": None}, + "change course": {**change, "course_key": "course-v1:Org1+Other+Run"}, + "change competency": {**change, "tag_id": tag.id + 1}, + "archive": {**change, "archived": True}, + "change id": {**change, "id": leaf.id}, + } + before = stored_group(course_level) + + response = user_client.patch(group_detail_url(tag.id, course_level.id), bodies[case], format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert error_key in response.data + assert stored_group(course_level) == before + + +def test_update_root_group_is_400(user_client: APIClient, tag: Tag, course_level: CompetencyCriteriaGroup) -> None: + """A root group is refused with a 400 saying roots cannot be targeted, and it is unchanged.""" + root = course_level.parent + assert root is not None + before = stored_group(root) + + response = user_client.patch( + group_detail_url(tag.id, root.id), {"logic_operator": LogicOperator.AND}, format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "root" in str(response.data["group_id"]) + assert stored_group(root) == before + + +def test_update_course_less_child_of_the_root_is_400_but_not_as_a_root( + user_client: APIClient, tag: Tag, course_level: CompetencyCriteriaGroup, +) -> None: + """A group with a parent but no course anywhere above it is refused for having no course, not as a root.""" + root = course_level.parent + assert root is not None + course_less = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, logic_operator=LogicOperator.AND) + before = stored_group(course_less) + + response = user_client.patch( + group_detail_url(tag.id, course_less.id), {"logic_operator": LogicOperator.OR}, format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "no course" in str(response.data["group_id"]) + assert "root" not in str(response.data["group_id"]) + assert stored_group(course_less) == before + + +def test_update_group_nested_below_a_leaf_is_200( + user_client: APIClient, tag: Tag, leaf: CompetencyCriteriaGroup, +) -> None: + """A group nested below a leaf is editable, its course coming from the nearest ancestor that has one.""" + nested = CompetencyCriteriaGroup.objects.create(tag=tag, parent=leaf, logic_operator=LogicOperator.OR) + + response = user_client.patch( + group_detail_url(tag.id, nested.id), {"logic_operator": LogicOperator.AND}, format="json", + ) + + assert response.status_code == status.HTTP_200_OK + assert response.data["logic_operator"] == LogicOperator.AND + + +def test_update_group_under_a_tag_that_is_not_a_competency_is_404( + user_client: APIClient, course_run: CourseRun, +) -> None: + """A group under a non-competency taxonomy is a missing resource, and it is unchanged.""" + plain_tag = Tag.objects.create(taxonomy=Taxonomy.objects.create(name="Plain Tags", export_id="plain-v1"), value="x") + root = CompetencyCriteriaGroup.objects.create(tag=plain_tag, parent=None) + # Shaped like any editable group, so only its taxonomy sets it apart. + course_level = CompetencyCriteriaGroup.objects.create( + tag=plain_tag, course=course_run, parent=root, logic_operator=LogicOperator.AND, + ) + before = stored_group(course_level) + + # The mismatched name would be a 400 if the body were checked before the tag, as it isn't on the other endpoints. + response = user_client.patch( + group_detail_url(plain_tag.id, course_level.id), + {"logic_operator": LogicOperator.OR, "name": "Renamed"}, + format="json", + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + assert stored_group(course_level) == before + + +def test_update_group_addressed_under_another_competency_is_404( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_level: CompetencyCriteriaGroup, +) -> None: + """A group addressed through a competency it doesn't belong to doesn't exist there, and is unchanged.""" + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Reading Poetry") + before = stored_group(course_level) + + response = user_client.patch( + group_detail_url(other_tag.id, course_level.id), {"logic_operator": LogicOperator.OR}, format="json", + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + assert stored_group(course_level) == before + + +def test_update_group_that_does_not_exist_is_404(user_client: APIClient, tag: Tag) -> None: + """An unknown group id in the URL is a missing resource.""" + response = user_client.patch(group_detail_url(tag.id, 999999), {"logic_operator": LogicOperator.OR}, format="json") + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_update_archived_group_is_409(user_client: APIClient, tag: Tag, leaf: CompetencyCriteriaGroup) -> None: + """An archived group conflicts with any edit, and stays archived and otherwise unchanged.""" + leaf.archived = True + leaf.save() + before = stored_group(leaf) + + response = user_client.patch( + group_detail_url(tag.id, leaf.id), {"logic_operator": LogicOperator.AND}, format="json", + ) + + assert response.status_code == status.HTTP_409_CONFLICT + assert "archived" in str(response.data["detail"]) + assert stored_group(leaf) == before + + +@pytest.mark.parametrize("lacks", ["course write access", "taxonomy view access"]) +def test_update_group_without_permission_is_403( + lacks: str, user_client: APIClient, tag: Tag, organization: Organization, +) -> None: + """A caller who may not tag objects in the group's course, or may not view its taxonomy, is refused.""" + course_code = UNAUTHORIZED_MARKER if lacks == "course write access" else "Python200" + leaf = _create_leaf_group(tag, make_course_run(organization, course_code, "Fall2026")) + if lacks == "taxonomy view access": + taxonomy = tag.taxonomy + assert taxonomy is not None + taxonomy.enabled = False + taxonomy.save() + before = stored_group(leaf) + + response = user_client.patch( + group_detail_url(tag.id, leaf.id), {"logic_operator": LogicOperator.AND}, format="json", + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert stored_group(leaf) == before + + +@pytest.mark.parametrize("guess", ["wrong name", "right name", "wrong course_key", "wrong parent_id", "unknown group"]) +def test_update_group_tells_a_caller_who_cannot_read_the_tree_nothing_about_what_is_stored( + guess: str, user_client: APIClient, tag: Tag, course_level: CompetencyCriteriaGroup, +) -> None: + """ + Without view access to the taxonomy, every guess is the same 403, and nothing changes. + + Were the body compared or the group looked up first, a wrong guess would be a 400 or a 404 and a right one a 403. + """ + taxonomy = tag.taxonomy + assert taxonomy is not None + taxonomy.enabled = False + taxonomy.save() + change = {"logic_operator": LogicOperator.OR} + bodies: dict[str, dict[str, Any]] = { + "wrong name": {**change, "name": "Guessed"}, + "right name": {**change, "name": course_level.name}, + "wrong course_key": {**change, "course_key": "course-v1:Org1+Other+Run"}, + "wrong parent_id": {**change, "parent_id": None}, + "unknown group": change, + } + group_id = 999999 if guess == "unknown group" else course_level.id + before = stored_group(course_level) + + response = user_client.patch(group_detail_url(tag.id, group_id), bodies[guess], format="json") + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert stored_group(course_level) == before + + +def test_update_group_from_an_unidentified_caller_is_401( + api_client: APIClient, tag: Tag, leaf: CompetencyCriteriaGroup, +) -> None: + """A caller the system cannot identify is refused before anything is read.""" + response = api_client.patch(group_detail_url(tag.id, leaf.id), {"logic_operator": LogicOperator.AND}, format="json") + + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + +@pytest.mark.parametrize("method", ["get", "put", "post", "delete"]) +def test_group_detail_offers_only_patch( + method: str, user_client: APIClient, tag: Tag, leaf: CompetencyCriteriaGroup, +) -> None: + """Every method other than PATCH, 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