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