diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index aa52d33f7..c4df8d3db 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -8,9 +8,9 @@ from decimal import Decimal from typing import Any, Sequence -from django.core.exceptions import ValidationError +from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError from django.db import connection, transaction -from django.db.models import QuerySet +from django.db.models import F, Q, QuerySet from django.db.transaction import TransactionManagementError from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError @@ -20,6 +20,7 @@ from openedx_catalog.models_api import CourseRun from openedx_tagging.api import create_taxonomy, get_object_tags, tag_object from openedx_tagging.models import ObjectTag, Tag, Taxonomy +from openedx_tagging.rules import ObjectTagPermissionItem, UserType from .data import GradedObjectScore from .models import ( @@ -34,8 +35,10 @@ from .rule_payloads import evaluate_rule __all__ = [ + "CompetencyCriterionArchivedError", "GradedObjectScore", "associate_competency_criterion", + "bulk_update_competency_criteria", "create_competency_criterion", "create_competency_taxonomy", "get_competency_rule_profiles", @@ -49,6 +52,16 @@ log = logging.getLogger(__name__) +class CompetencyCriterionArchivedError(Exception): + """ + Raised when a request would edit an archived CompetencyCriterion, or one under an archived group. + + UNSTABLE: the criteria-authoring family is incomplete, so its validation and group update + and 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, @@ -187,6 +200,24 @@ def resolve_supplied_leaf_group(group_id: int, tag: Tag, course_run: CourseRun) return group +def _get_applicable_rule_profile(taxonomy_id: int) -> CompetencyRuleProfile | None: + """ + Return the live profile ADR-0002 Decision 4 assigns a criterion in ``taxonomy_id``, or None if there is none. + + The profile scoped to the taxonomy wins, else the system default. Course- and + organization-scoped profiles are not resolved here, since a criterion is authored at taxonomy + scope. + """ + live = CompetencyRuleProfile.objects.filter(archived=False, organization__isnull=True, course__isnull=True) + # CompetencyTaxonomy shares its pk with its parent Taxonomy row. Of the at most two rows that + # match, nulls_last sorts the taxonomy's own profile ahead of the system default. + return ( + live.filter(Q(competency_taxonomy_id=taxonomy_id) | Q(competency_taxonomy__isnull=True)) + .order_by(F("competency_taxonomy_id").desc(nulls_last=True)) + .first() + ) + + def create_competency_criterion( group: CompetencyCriteriaGroup, object_id: str, @@ -208,8 +239,7 @@ def create_competency_criterion( When no rule fields are supplied, resolves to the live profile scoped to the tag's taxonomy, else the live system default, rather than persisting null: ADR-0002 Decision 4 always resolves to a concrete profile, and the model's own xor-override constraint forbids leaving - all three fields null. Course- and organization-scoped profiles are not resolved here, since - a criterion is authored at taxonomy scope. + all three fields null. """ tag = group.tag # tag.taxonomy_id is nullable at the model level; already guaranteed set by the caller. @@ -224,11 +254,9 @@ def create_competency_criterion( object_tag = ObjectTag.objects.get(object_id=object_id, taxonomy_id=tag.taxonomy_id, tag_id=tag.id) if rule_profile_id is None and rule_type_override is None and rule_payload_override is None: - live = CompetencyRuleProfile.objects.filter(archived=False, organization__isnull=True, course__isnull=True) - # CompetencyTaxonomy shares its pk with its parent Taxonomy row. - profile = live.filter(competency_taxonomy_id=tag.taxonomy_id).first() or live.get( - competency_taxonomy__isnull=True - ) + profile = _get_applicable_rule_profile(tag.taxonomy_id) + if profile is None: + raise CompetencyRuleProfile.DoesNotExist("No live rule profile applies to this criterion.") rule_profile_id = profile.id # Parent-competency dominance/containment validation belongs here, before creation. @@ -361,6 +389,181 @@ def associate_competency_criterion( ) +def _validate_rule_source( + rule_profile_id: int | None, + rule_type_override: str | None, + rule_payload_override: dict | None, +) -> None: + """ + Validate the one rule source a bulk update applies. + + Runs the create path's rule-field checks, then also refuses a request naming no rule source: a + bulk update has no applicable-profile fallback, and ADR-0002 Decision 4 allows a criterion no + other state. Override values are validated once for the whole batch through the model's own + ``full_clean()``, so the payload contract has no second copy here. Raises ValidationError keyed + by the REST field names. + """ + _validate_rule_fields(rule_profile_id, rule_type_override, rule_payload_override) + if rule_profile_id is not None: + return + # _validate_rule_fields has already refused half a pair, so one override being None means both are. + if rule_type_override is None: + raise ValidationError( + {NON_FIELD_ERRORS: _("Supply rule_profile_id, or both rule_type_override and rule_payload_override.")} + ) + try: + CompetencyCriterion( + rule_type_override=rule_type_override, + rule_payload_override=rule_payload_override, + ).full_clean(exclude=["group", "object_tag", "rule_profile"], validate_unique=False, validate_constraints=False) + except ValidationError as exc: + errors = dict(exc.error_dict) + # The model's clean() reports the payload's shape as a non-field error. A payload is only judged + # against a valid rule type, so once rule_type_override is itself refused, that error just restates it. + payload_errors = errors.pop(NON_FIELD_ERRORS, []) + if payload_errors and "rule_type_override" not in errors: + errors["rule_payload_override"] = [*errors.get("rule_payload_override", []), *payload_errors] + raise ValidationError(errors) from exc + + +def _apply_rule_source( + criterion: CompetencyCriterion, + rule_profile_id: int | None, + rule_type_override: str | None, + rule_payload_override: dict | None, + user: UserType, + *, + applicable: CompetencyRuleProfile | None, +) -> None: + """ + Point ``criterion`` at ``rule_profile_id``, or give it the override values, saving only if that changes it. + + Override values that exactly match ``applicable``, the profile ADR-0002 Decision 4 assigns the + criterion, reassign it to that profile instead. All three rule columns are written in one save, + so the criterion's new history row never records both a profile and overrides, or neither. + """ + # Compares the parsed rule, never stored JSON text, so key order and formatting don't matter. + if ( + rule_profile_id is None + and applicable is not None + and applicable.rule_type == rule_type_override + and applicable.rule_payload == rule_payload_override + ): + rule_profile_id = applicable.id + + target: tuple[int | None, str | None, dict | None] + if rule_profile_id is not None: + target, change_reason = (rule_profile_id, None, None), "Reassigned to rule profile" + else: + target, change_reason = (None, rule_type_override, rule_payload_override), "Rule values set" + if (criterion.rule_profile_id, criterion.rule_type_override, criterion.rule_payload_override) == target: + return + + criterion.rule_profile_id, criterion.rule_type_override, criterion.rule_payload_override = target + # Set on the instance so attribution doesn't depend on simple_history's request middleware. + criterion._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + criterion._change_reason = change_reason # type: ignore[attr-defined] # pylint: disable=protected-access + # Never QuerySet.update(): it skips post_save, so simple_history would record nothing. + criterion.save(update_fields=["rule_profile", "rule_type_override", "rule_payload_override"]) + + +def bulk_update_competency_criteria( + criterion_ids: list[int], + group_id: int, + *, + rule_profile_id: int | None = None, + rule_type_override: str | None = None, + rule_payload_override: dict | None = None, + user: UserType, +) -> list[CompetencyCriterion]: + """ + Apply one rule source to every criterion ``criterion_ids`` names in one leaf group, atomically. + + UNSTABLE: the criteria-authoring family is incomplete, so its validation and group update + and archive behavior may change this function's shape without a deprecation cycle. + + Supply either ``rule_profile_id`` or both override values; each criterion's three rule columns + are then rewritten together, so a profile clears any overrides and overrides clear any profile. + Learner status is never touched. + + Returns the named criteria in request order. Raises PermissionDenied if ``user`` fails + ``oel_tagging.can_tag_object`` for the group's taxonomy and course, + CompetencyCriteriaGroup.DoesNotExist if the group doesn't exist, CompetencyCriterion.DoesNotExist + if any named id isn't a criterion of that group, and ValidationError, keyed by the REST field + names, for a non-leaf group, an empty or repeated id list, or an invalid rule source. Raises + CompetencyCriterionArchivedError if the group, either of its ancestors, or any named criterion + is archived. + """ + with transaction.atomic(): + group = CompetencyCriteriaGroup.objects.select_related("tag__taxonomy", "parent__course").get( + pk=group_id + ) + # Criteria hang only off leaf groups, and a leaf's course is on its course-level parent. + if group.parent is None or group.course_id is not None or group.parent.course is None: + raise ValidationError({"group_id": _("group_id must reference a leaf CompetencyCriteriaGroup.")}) + # A group's tag is always a competency tag, so its taxonomy is always set. + assert group.tag.taxonomy is not None + perm_obj = ObjectTagPermissionItem(taxonomy=group.tag.taxonomy, object_id=str(group.parent.course.course_key)) + if not user.has_perm("oel_tagging.can_tag_object", perm_obj): + raise PermissionDenied + + if not criterion_ids: + raise ValidationError({"criterion_ids": _("criterion_ids must name at least one criterion.")}) + if len(set(criterion_ids)) != len(criterion_ids): + raise ValidationError({"criterion_ids": _("criterion_ids must not name the same criterion twice.")}) + _validate_rule_source(rule_profile_id, rule_type_override, rule_payload_override) + + # Archiving a group doesn't archive its criteria, yet ADR 0002 Decision 2 closes them to authoring. + # An archived ancestor counts too: it stands in for a delete that would have cascaded to this leaf. + # Locked for the same reason as the criteria below. + lineage = [group.id, group.parent.id, group.parent.parent_id] + archived_by_id = dict( + CompetencyCriteriaGroup.objects.select_for_update().filter(pk__in=lineage).values_list("pk", "archived") + ) + archived_group_id = next((pk for pk in lineage if archived_by_id.get(pk)), None) + if archived_group_id is not None: + raise CompetencyCriterionArchivedError( + _("Criteria under an archived group cannot be edited: group {group_id}.").format( + group_id=archived_group_id + ) + ) + + # Locked so a concurrent archive can't slip in after the archived check, and so a concurrent + # identical request waits and then sees nothing to change instead of writing a second history row. + criteria_by_id = CompetencyCriterion.objects.select_for_update().filter(group=group).in_bulk(criterion_ids) + missing_ids = [criterion_id for criterion_id in criterion_ids if criterion_id not in criteria_by_id] + if missing_ids: + raise CompetencyCriterion.DoesNotExist( + _("Not criteria of this group: {criterion_ids}.").format( + criterion_ids=", ".join(str(criterion_id) for criterion_id in missing_ids) + ) + ) + + # One archived criterion refuses the whole batch: ADR 0002 Decision 4 closes archived criteria to authoring. + archived_ids = [criterion_id for criterion_id in criterion_ids if criteria_by_id[criterion_id].archived] + if archived_ids: + raise CompetencyCriterionArchivedError( + _("Archived criteria cannot be edited: {criterion_ids}.").format( + criterion_ids=", ".join(str(criterion_id) for criterion_id in archived_ids) + ) + ) + + # Looked up once for the whole batch, and only when override values need comparing against it: + # every criterion in a leaf shares the group's tag, so they all resolve to the same profile. + applicable = _get_applicable_rule_profile(group.tag.taxonomy.id) if rule_profile_id is None else None + criteria = [criteria_by_id[criterion_id] for criterion_id in criterion_ids] + for criterion in criteria: + _apply_rule_source( + criterion, + rule_profile_id, + rule_type_override, + rule_payload_override, + user, + applicable=applicable, + ) + return criteria + + def _effective_rule(criterion: CompetencyCriterion) -> tuple[str, Any]: """Return (rule_type, payload) for whichever of criterion's profile or override is set.""" if criterion.rule_profile_id is not None: 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 97b179541..22e029e9c 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py @@ -99,3 +99,23 @@ def validate_object_id(self, value: str) -> str: except InvalidKeyError as exc: raise serializers.ValidationError(_("object_id is not a valid usage key.")) from exc return value + + +class CompetencyCriterionBulkUpdateSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + Parse the bulk-update request body, checking only each field's shape and rejecting unknown keys. + """ + + criterion_ids = serializers.ListField(child=serializers.IntegerField()) + rule_profile_id = serializers.IntegerField(required=False, allow_null=True) + rule_type_override = serializers.CharField(required=False, allow_null=True) + rule_payload_override = serializers.JSONField(required=False, allow_null=True) + + def validate(self, attrs: dict) -> dict: + """ + Reject any top-level key this serializer does not declare. + """ + unrecognized = sorted(set(self.initial_data) - set(self.fields)) + if unrecognized: + raise serializers.ValidationError({key: "This field is not recognized." for key in unrecognized}) + return attrs 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 f52abfa78..0862262d5 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py @@ -17,4 +17,9 @@ views.CompetencyCriterionCreateView.as_view(), name="criterion-create", ), + path( + "criteria-groups//criteria/bulk-update/", + views.CompetencyCriterionBulkUpdateView.as_view(), + name="criterion-bulk-update", + ), ] 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 4be3149cf..e576108b9 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -5,12 +5,13 @@ from django.core.exceptions import ValidationError as DjangoValidationError from django.db.models import QuerySet +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] SessionAuthenticationAllowInactiveUser, ) from rest_framework import generics, mixins, status -from rest_framework.exceptions import NotFound, PermissionDenied +from rest_framework.exceptions import APIException, NotFound, PermissionDenied from rest_framework.exceptions import ValidationError as DRFValidationError from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response @@ -19,11 +20,21 @@ from openedx_tagging.models import Tag from openedx_tagging.rules import ObjectTagPermissionItem -from ...api import associate_competency_criterion, get_competency_rule_profiles, resolve_competency_tag -from ...models import CompetencyCriteriaGroup, CompetencyRuleProfile +from ...api import ( + CompetencyCriterionArchivedError, + associate_competency_criterion, + bulk_update_competency_criteria, + get_competency_rule_profiles, + resolve_competency_tag, +) +from ...models import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile from ..paginators import CompetencyRuleProfilePagination from .permissions import CompetencyRuleProfilePermissions -from .serializers import CompetencyCriterionSerializer, CompetencyRuleProfileSerializer +from .serializers import ( + CompetencyCriterionBulkUpdateSerializer, + CompetencyCriterionSerializer, + CompetencyRuleProfileSerializer, +) class CompetencyRuleProfileView(mixins.ListModelMixin, GenericViewSet): @@ -104,3 +115,52 @@ def create(self, request, *args, **kwargs): except DjangoValidationError as exc: raise DRFValidationError(exc.message_dict if hasattr(exc, "message_dict") else exc.messages) from exc return Response(self.get_serializer(criterion).data, status=status.HTTP_201_CREATED) + + +class CompetencyCriteriaConflict(APIException): + status_code = status.HTTP_409_CONFLICT + default_detail = _("The request conflicts with the current state of the named criteria.") + default_code = "conflict" + + +class CompetencyCriterionBulkUpdateView(generics.GenericAPIView): + """ + PATCH-only. Apply one rule to a named batch of CompetencyCriteria in one leaf group, atomically. + + The body names the criteria to update, all of which must belong to the group in the URL, and + exactly one rule source: either a live ``rule_profile_id`` to point them at, or both + ``rule_type_override`` and ``rule_payload_override``. The response is every named criterion as + stored, in request order, including any the update left unchanged. + + A thin adapter: :func:`bulk_update_competency_criteria` owns every semantic refusal, the + permission check included, so an in-process caller gets the same refusals. A batch naming an + archived criterion, or addressed to a group that is archived or sits under an archived group, + is refused whole with 409. + """ + + serializer_class = CompetencyCriterionBulkUpdateSerializer + http_method_names = ["patch"] + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = [IsAuthenticated] + + def patch(self, request, group_id: int): + """Parse the request body, then delegate to the public API.""" + serializer = self.get_serializer(data=request.data) + serializer.is_valid(raise_exception=True) + data = serializer.validated_data + try: + criteria = bulk_update_competency_criteria( + data["criterion_ids"], + group_id, + rule_profile_id=data.get("rule_profile_id"), + rule_type_override=data.get("rule_type_override"), + rule_payload_override=data.get("rule_payload_override"), + user=request.user, + ) + except (CompetencyCriteriaGroup.DoesNotExist, CompetencyCriterion.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 CompetencyCriterionArchivedError as exc: + raise CompetencyCriteriaConflict(str(exc)) from exc + return Response(CompetencyCriterionSerializer(criteria, many=True).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 0cfeafe62..8f014fa9d 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -1,15 +1,25 @@ """ Tests for the CBE public API surface (openedx_learning.api). """ +from collections.abc import Iterator +from typing import Any import pytest -from django.core.exceptions import ValidationError +import rules +from django.apps import apps +from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError +from django.db import connection from django.db.utils import IntegrityError +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 ( + CompetencyCriterionArchivedError, associate_competency_criterion, + bulk_update_competency_criteria, + create_competency_criterion, create_competency_taxonomy, get_competency_rule_profiles, is_competency_taxonomy, @@ -25,8 +35,11 @@ CompetencyTaxonomy, LogicOperator, RuleType, + StudentCompetencyCriteriaGroupStatus, + StudentCompetencyCriterionStatus, + StudentCompetencyStatus, ) -from openedx_tagging.models import Tag, Taxonomy +from openedx_tagging.models import ObjectTag, Tag, Taxonomy pytestmark = pytest.mark.django_db @@ -510,3 +523,495 @@ def _reject(**_kwargs) -> None: associate_competency_criterion(tag_id=tag.id, object_id=object_id) assert not CompetencyCriteriaGroup.objects.filter(tag=tag).exists() + + +# ============================================================================================== +# bulk_update_competency_criteria (#759) +# ============================================================================================== + +# Migration 0005's seeded system-default rule, written as the override values a caller would submit. +SEEDED_RULE: dict[str, Any] = {"rule_type_override": RuleType.GRADE, "rule_payload_override": GRADE_PAYLOAD} +# The threshold most tests apply, distinct from the seeded default so it stays an override. +NEW_PAYLOAD = {"op": "gte", "value": 0.75, "scale": "percent"} +NEW_RULE: dict[str, Any] = {"rule_type_override": RuleType.GRADE, "rule_payload_override": NEW_PAYLOAD} +# A starting override that neither of the above matches. +OLD_PAYLOAD = {"op": "lte", "value": 0.5, "scale": "percent"} + +CHANGE_OBJECTTAG_OBJECTID = "oel_tagging.change_objecttag_objectid" + + +@pytest.fixture(name="checked_object_ids", autouse=True) +def _checked_object_ids() -> Iterator[list[str]]: + """ + Let any user tag any object, recording each object_id checked; restore the real rule afterwards. + + openedx_tagging denies change_objecttag_objectid to everyone and leaves the real Studio-role + check to openedx-platform, so these tests substitute a permissive one. + """ + original = rule_permissions[CHANGE_OBJECTTAG_OBJECTID] + checked: list[str] = [] + + def _predicate(_user, object_id: str) -> bool: + checked.append(object_id) + return True + + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, _predicate) + yield checked + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, original) + + +@pytest.fixture(name="leaf") +def _leaf(tag: Tag, course_run: CourseRun) -> CompetencyCriteriaGroup: + """A leaf group under `tag` and `course_run`. Only a leaf group holds criteria.""" + return _create_leaf_group(tag, course_run) + + +def make_criterion(leaf: CompetencyCriteriaGroup, block_id: str, **rule) -> CompetencyCriterion: + """Create a criterion on `leaf` for a fresh object in its course, with `rule` as its rule columns.""" + assert leaf.parent is not None and leaf.parent.course is not None + object_tag = ObjectTag.objects.create( + object_id=usage_key(leaf.parent.course, block_id), taxonomy=leaf.tag.taxonomy, tag=leaf.tag, + ) + return CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, **rule) + + +@pytest.fixture(name="batch") +def _batch( + leaf: CompetencyCriteriaGroup, default_rule_profile: CompetencyRuleProfile +) -> tuple[CompetencyCriterion, CompetencyCriterion]: + """Two criteria on `leaf` with different rule sources: one follows the default, one has an override.""" + return ( + make_criterion(leaf, "p1", rule_profile=default_rule_profile), + make_criterion(leaf, "p2", rule_type_override=RuleType.GRADE, rule_payload_override=OLD_PAYLOAD), + ) + + +def rule_state(criterion: CompetencyCriterion) -> tuple: + """Return `criterion`'s three rule columns as stored.""" + criterion.refresh_from_db() + return (criterion.rule_profile_id, criterion.rule_type_override, criterion.rule_payload_override) + + +def history_of(criterion: CompetencyCriterion): + """ + Return `criterion`'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_criterion = apps.get_model("openedx_learning", "HistoricalCompetencyCriterion") + return historical_criterion.objects.filter(id=criterion.pk).order_by("-history_date", "-history_id") + + +def snapshot(criteria) -> dict[int, tuple]: + """Record each criterion's stored rule and history length, to prove a refused call changed nothing.""" + return {criterion.id: (rule_state(criterion), history_of(criterion).count()) for criterion in criteria} + + +def test_bulk_update_gives_every_criterion_the_rule_values_whatever_its_starting_source( + batch: tuple[CompetencyCriterion, CompetencyCriterion], leaf: CompetencyCriteriaGroup, user, +) -> None: + """Rule values replace a profile link and an existing override alike, and the result keeps request order.""" + on_profile, on_override = batch + + result = bulk_update_competency_criteria([on_override.id, on_profile.id], leaf.id, user=user, **NEW_RULE) + + assert [criterion.id for criterion in result] == [on_override.id, on_profile.id] + for criterion in batch: + assert rule_state(criterion) == (None, RuleType.GRADE, NEW_PAYLOAD) + + +@pytest.mark.parametrize("named", ["system default", "taxonomy-scoped"]) +def test_bulk_update_assigns_a_named_profile_clearing_values( # pylint: disable=too-many-positional-arguments + named: str, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, + default_rule_profile: CompetencyRuleProfile, + competency_taxonomy: CompetencyTaxonomy, +) -> None: + """Naming a profile clears any override values, and the history row says why it changed.""" + if named == "system default": + profile = default_rule_profile + else: + profile = CompetencyRuleProfile.objects.create( + rule_type=RuleType.GRADE, rule_payload=NEW_PAYLOAD, competency_taxonomy=competency_taxonomy, + ) + on_override = batch[1] + + bulk_update_competency_criteria([c.id for c in batch], leaf.id, rule_profile_id=profile.id, user=user) + + for criterion in batch: + assert rule_state(criterion) == (profile.id, None, None) + assert history_of(on_override).first().history_change_reason == "Reassigned to rule profile" + + +def test_bulk_update_looks_up_the_applicable_profile_once_per_batch( + leaf: CompetencyCriteriaGroup, user, default_rule_profile: CompetencyRuleProfile, +) -> None: + """However many criteria a batch names, comparing override values against the applicable profile reads it once.""" + criteria = [make_criterion(leaf, f"p{n}", rule_profile=default_rule_profile) for n in range(5)] + profile_table = CompetencyRuleProfile._meta.db_table + + with CaptureQueriesContext(connection) as queries: + bulk_update_competency_criteria([c.id for c in criteria], leaf.id, user=user, **NEW_RULE) + + profile_reads = [q for q in queries.captured_queries if q["sql"].startswith("SELECT") and profile_table in q["sql"]] + assert len(profile_reads) == 1 + + +def test_bulk_update_values_that_match_the_default_return_the_criterion_to_it( + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Values equal to the applicable profile's rule store the profile link, not a redundant override. + + The payload's keys arrive in a different order from the seeded row's, to show the comparison is + on the parsed rule and never on JSON text. + """ + reordered_payload = {"scale": "percent", "value": 0.8, "op": "gte"} + + bulk_update_competency_criteria( + [c.id for c in batch], leaf.id, rule_type_override=RuleType.GRADE, rule_payload_override=reordered_payload, + user=user, + ) + + for criterion in batch: + assert rule_state(criterion) == (default_rule_profile.id, None, None) + + +def test_bulk_update_values_return_a_criterion_to_the_taxonomy_profile_not_the_default( + leaf: CompetencyCriteriaGroup, user, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """ + With a live profile scoped to the leaf's taxonomy, values are compared against it, as creation resolves it. + + ADR-0002 Decision 4 assigns the taxonomy profile over the system default, so values equal to the + system default's rule are an override, and only values equal to the taxonomy profile's return + the criterion to it. + """ + taxonomy_profile = CompetencyRuleProfile.objects.create( + rule_type=RuleType.GRADE, rule_payload=NEW_PAYLOAD, competency_taxonomy=competency_taxonomy, + ) + assert leaf.parent is not None and leaf.parent.course is not None + criterion = create_competency_criterion(leaf, usage_key(leaf.parent.course, "p1")) + assert rule_state(criterion) == (taxonomy_profile.id, None, None) + + bulk_update_competency_criteria([criterion.id], leaf.id, user=user, **SEEDED_RULE) + assert rule_state(criterion) == (None, RuleType.GRADE, GRADE_PAYLOAD) + + bulk_update_competency_criteria([criterion.id], leaf.id, user=user, **NEW_RULE) + assert rule_state(criterion) == (taxonomy_profile.id, None, None) + + +def test_bulk_update_writes_history_only_for_criteria_it_changes( + leaf: CompetencyCriteriaGroup, user, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A criterion already carrying the rule gets no new history row; one that changes gets exactly one.""" + unchanged = make_criterion(leaf, "p1", **NEW_RULE) + changed = make_criterion(leaf, "p2", rule_profile=default_rule_profile) + unchanged_before, changed_before = history_of(unchanged).count(), history_of(changed).count() + + result = bulk_update_competency_criteria([unchanged.id, changed.id], leaf.id, user=user, **NEW_RULE) + + assert [criterion.id for criterion in result] == [unchanged.id, changed.id] + assert history_of(unchanged).count() == unchanged_before + assert history_of(changed).count() == changed_before + 1 + latest = history_of(changed).first() + assert latest.history_user == user + assert latest.history_change_reason == "Rule values set" + assert (latest.rule_profile_id, latest.rule_type_override, latest.rule_payload_override) == ( + None, RuleType.GRADE, NEW_PAYLOAD, + ) + + +@pytest.mark.parametrize( + "case", + [ + "rule values", + "named profile", + "values matching the default", + "nothing to change", + "malformed request", + "id outside the group", + "archived criterion", + "no permission", + ], +) +def test_bulk_update_never_reads_or_writes_learner_status( + case: str, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + No path, accepted or refused, sends a query to a learner status table. + + Whether a learner has a recorded status plays no part in accepting a batch: #723 owns that check, + in front of this endpoint. A create, update, delete or read would each be a query, so checking + the SQL covers all four. + """ + first, second = batch + ids = [first.id, second.id] + unchanged = make_criterion(leaf, "p3", **NEW_RULE) + archived = make_criterion(leaf, "p4", rule_profile=default_rule_profile, archived=True) + requests: dict[str, tuple[list[int], dict[str, Any], type[Exception] | None]] = { + "rule values": (ids, NEW_RULE, None), + "named profile": (ids, {"rule_profile_id": default_rule_profile.id}, None), + "values matching the default": (ids, SEEDED_RULE, None), + "nothing to change": ([unchanged.id], NEW_RULE, None), + "malformed request": ([first.id, first.id], NEW_RULE, ValidationError), + "id outside the group": ([first.id, 999999], NEW_RULE, CompetencyCriterion.DoesNotExist), + "archived criterion": ([first.id, archived.id], NEW_RULE, CompetencyCriterionArchivedError), + "no permission": (ids, NEW_RULE, PermissionDenied), + } + criterion_ids, rule, error = requests[case] + if case == "no permission": + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, lambda _user, _object_id: False) + status_tables = [ + model._meta.db_table + for model in (StudentCompetencyStatus, StudentCompetencyCriteriaGroupStatus, StudentCompetencyCriterionStatus) + ] + + with CaptureQueriesContext(connection) as queries: + if error is None: + bulk_update_competency_criteria(criterion_ids, leaf.id, user=user, **rule) + else: + with pytest.raises(error): + bulk_update_competency_criteria(criterion_ids, leaf.id, user=user, **rule) + + assert queries.captured_queries + assert not [q["sql"] for q in queries.captured_queries if any(table in q["sql"] for table in status_tables)] + + +@pytest.mark.parametrize( + "case", + [ + "empty id list", + "duplicate id", + "both rule forms", + "neither rule form", + "rule type without payload", + "payload without rule type", + "unknown profile", + "archived profile", + "unsupported comparison", + "threshold above 1.0", + "threshold below 0.0", + "unimplemented rule type", + ], +) +def test_bulk_update_rejects_a_malformed_request_changing_nothing( # pylint: disable=too-many-positional-arguments + case: str, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, + default_rule_profile: CompetencyRuleProfile, + competency_taxonomy: CompetencyTaxonomy, +) -> None: + """Each malformed request is refused with an error naming what is wrong, and no criterion changes.""" + first, second = batch + ids = [first.id, second.id] + archived = CompetencyRuleProfile.objects.create( + rule_type=RuleType.GRADE, rule_payload=NEW_PAYLOAD, competency_taxonomy=competency_taxonomy, archived=True, + ) + requests: dict[str, tuple[list[int], dict[str, Any], str]] = { + "empty id list": ([], NEW_RULE, "criterion_ids"), + "duplicate id": ([first.id, first.id, second.id], NEW_RULE, "criterion_ids"), + "both rule forms": ( + ids, {"rule_profile_id": default_rule_profile.id, **NEW_RULE}, "rule_profile_id", + ), + "neither rule form": (ids, {}, NON_FIELD_ERRORS), + "rule type without payload": (ids, {"rule_type_override": RuleType.GRADE}, "rule_type_override"), + "payload without rule type": (ids, {"rule_payload_override": NEW_PAYLOAD}, "rule_payload_override"), + "unknown profile": (ids, {"rule_profile_id": 999999}, "rule_profile_id"), + "archived profile": (ids, {"rule_profile_id": archived.id}, "rule_profile_id"), + "unsupported comparison": ( + ids, {**NEW_RULE, "rule_payload_override": {**NEW_PAYLOAD, "op": "gt"}}, "rule_payload_override", + ), + "threshold above 1.0": ( + ids, {**NEW_RULE, "rule_payload_override": {**NEW_PAYLOAD, "value": 1.5}}, "rule_payload_override", + ), + "threshold below 0.0": ( + ids, {**NEW_RULE, "rule_payload_override": {**NEW_PAYLOAD, "value": -0.1}}, "rule_payload_override", + ), + "unimplemented rule type": (ids, {**NEW_RULE, "rule_type_override": "Completion"}, "rule_type_override"), + } + criterion_ids, rule, error_key = requests[case] + before = snapshot(batch) + + with pytest.raises(ValidationError) as exc_info: + bulk_update_competency_criteria(criterion_ids, leaf.id, user=user, **rule) + + assert list(exc_info.value.message_dict) == [error_key] + assert snapshot(batch) == before + + +def test_bulk_update_names_both_rule_forms_as_not_combinable( + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """The refusal of a profile plus values says the two cannot be combined, not merely that the request failed.""" + with pytest.raises(ValidationError, match="cannot be combined"): + bulk_update_competency_criteria( + [c.id for c in batch], leaf.id, rule_profile_id=default_rule_profile.id, user=user, **NEW_RULE, + ) + + +def test_bulk_update_names_half_a_rule_as_needing_its_pair( + batch: tuple[CompetencyCriterion, CompetencyCriterion], leaf: CompetencyCriteriaGroup, user, +) -> None: + """A rule type without its payload is refused with a message saying the two must be supplied together.""" + with pytest.raises(ValidationError, match="must be supplied together"): + bulk_update_competency_criteria([c.id for c in batch], leaf.id, rule_type_override=RuleType.GRADE, user=user) + + +@pytest.mark.parametrize("stray", ["from another group", "nonexistent"]) +def test_bulk_update_raises_does_not_exist_for_ids_outside_the_group( # pylint: disable=too-many-positional-arguments + stray: str, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, + tag: Tag, + course_run: CourseRun, +) -> None: + """One id outside the group raises CompetencyCriterion.DoesNotExist naming it, and nothing changes.""" + if stray == "from another group": + stray_id = make_criterion(_create_leaf_group(tag, course_run), "p3", **NEW_RULE).id + else: + stray_id = 999999 + before = snapshot(batch) + + with pytest.raises(CompetencyCriterion.DoesNotExist, match=str(stray_id)): + bulk_update_competency_criteria([batch[0].id, stray_id, batch[1].id], leaf.id, user=user, **SEEDED_RULE) + + assert snapshot(batch) == before + + +def test_bulk_update_raises_does_not_exist_for_an_unknown_group( + batch: tuple[CompetencyCriterion, CompetencyCriterion], user, +) -> None: + """An unknown group id raises CompetencyCriteriaGroup.DoesNotExist.""" + with pytest.raises(CompetencyCriteriaGroup.DoesNotExist): + bulk_update_competency_criteria([c.id for c in batch], 999999, user=user, **NEW_RULE) + + +def test_bulk_update_refuses_the_whole_batch_if_any_criterion_is_archived( + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """One archived criterion refuses the whole batch, and nothing changes, not even the valid criteria.""" + archived = make_criterion(leaf, "p3", rule_profile=default_rule_profile, archived=True) + criteria = [batch[0], archived, batch[1]] + before = snapshot(criteria) + + with pytest.raises(CompetencyCriterionArchivedError, match=str(archived.id)): + bulk_update_competency_criteria([c.id for c in criteria], leaf.id, user=user, **NEW_RULE) + + assert snapshot(criteria) == before + + +@pytest.mark.parametrize("level", ["leaf", "course-level", "root"]) +def test_bulk_update_refuses_the_whole_batch_under_an_archived_group( + level: str, batch: tuple[CompetencyCriterion, CompetencyCriterion], leaf: CompetencyCriteriaGroup, user, +) -> None: + """ + An archived leaf, or an archived ancestor of it, refuses the batch, and nothing changes. + + Archiving a group leaves its criteria unarchived, so the criterion check alone would let this through. + """ + course_level = leaf.parent + assert course_level is not None and course_level.parent is not None + archived = {"leaf": leaf, "course-level": course_level, "root": course_level.parent}[level] + CompetencyCriteriaGroup.objects.filter(pk=archived.pk).update(archived=True) + assert not any(c.archived for c in batch) + before = snapshot(batch) + + with pytest.raises(CompetencyCriterionArchivedError, match=f"group {archived.id}"): + bulk_update_competency_criteria([c.id for c in batch], leaf.id, user=user, **NEW_RULE) + + assert snapshot(batch) == before + + +@pytest.mark.parametrize("level", ["root", "course-level"]) +def test_bulk_update_rejects_a_group_that_is_not_a_leaf( + level: str, leaf: CompetencyCriteriaGroup, user, +) -> None: + """Only a leaf group holds criteria, so the root and course-level groups are refused.""" + course_level = leaf.parent + assert course_level is not None and course_level.parent is not None + group = course_level.parent if level == "root" else course_level + + with pytest.raises(ValidationError, match="group_id"): + bulk_update_competency_criteria([1], group.id, user=user, **NEW_RULE) + + +def test_bulk_update_checks_permission_against_the_leafs_course( + checked_object_ids: list[str], + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, + course_run: CourseRun, +) -> None: + """The object-level check asks about the leaf's course, resolved through its course-level parent.""" + bulk_update_competency_criteria([c.id for c in batch], leaf.id, user=user, **NEW_RULE) + + assert checked_object_ids == [str(course_run.course_key)] + + +@pytest.mark.parametrize("lacks", ["course write access", "taxonomy view access"]) +def test_bulk_update_refuses_a_user_without_permission_changing_nothing( + lacks: str, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + 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 = snapshot(batch) + + with pytest.raises(PermissionDenied): + bulk_update_competency_criteria([c.id for c in batch], leaf.id, user=user, **NEW_RULE) + + assert snapshot(batch) == before + + +def test_bulk_update_rolls_back_every_criterion_if_one_save_fails( + monkeypatch: pytest.MonkeyPatch, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, +) -> None: + """A failure partway through the batch undoes the saves already made, history included.""" + before = snapshot(batch) + original_save = CompetencyCriterion.save + saves: list[int] = [] + + def _fail_on_second_save(self, *args, **kwargs) -> None: + saves.append(self.pk) + if len(saves) == 2: + raise IntegrityError("simulated failure") + original_save(self, *args, **kwargs) + + monkeypatch.setattr(CompetencyCriterion, "save", _fail_on_second_save) + + with pytest.raises(IntegrityError): + bulk_update_competency_criteria([c.id for c in batch], leaf.id, user=user, **NEW_RULE) + + monkeypatch.undo() + assert len(saves) == 2 + assert snapshot(batch) == before diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index f5c031f54..bc1798e35 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -4,13 +4,13 @@ Fixtures live in this directory's conftest.py. Several scenarios here create rule profile rows directly rather than through a live call, because no create or archive endpoint exists yet. """ -from typing import Generator +from typing import Any, Generator import pytest import rules from django.urls import reverse from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.keys import CourseKey, UsageKey from organizations.models import Organization from rest_framework import status from rest_framework.test import APIClient @@ -35,18 +35,22 @@ # checking"): real Studio-role logic lives only in openedx-platform, never in this repo. Following # tests/openedx_tagging/test_views.py's TestObjectTagViewSet.setUp() precedent, this override # simulates a permitted caller for every object_id except ones carrying this marker. Like the -# platform's rule, it also denies an object_id that isn't a parseable usage key. +# platform's rule, it also denies an object_id that is neither a parseable usage key nor a course key. UNAUTHORIZED_MARKER = "unauthorized" @pytest.fixture(autouse=True) def _allow_object_level_tagging() -> Generator[None, None, None]: - """Let an ordinary authenticated user tag any usage key except one carrying UNAUTHORIZED_MARKER.""" + """Let an ordinary authenticated user tag any usage or course key except one carrying UNAUTHORIZED_MARKER.""" def _predicate(_user, object_id: str) -> bool: try: UsageKey.from_string(object_id) except InvalidKeyError: - return False + try: + # The bulk-update endpoint checks permission against the group's course key. + CourseKey.from_string(object_id) + except InvalidKeyError: + return False return UNAUTHORIZED_MARKER not in object_id perm = "oel_tagging.change_objecttag_objectid" @@ -804,3 +808,293 @@ def test_tag_refused_at_the_object_tag_limit_is_400_and_creates_nothing( assert "Cannot add more than 1 tags" in str(response.data["object_id"]) assert not CompetencyCriterion.objects.exists() assert not CompetencyCriteriaGroup.objects.exists() + + +# ============================================================================================== +# CompetencyCriterionBulkUpdateView (#759) +# ============================================================================================== + +NEW_PAYLOAD = {"op": "gte", "value": 0.75, "scale": "percent"} +NEW_RULE: dict[str, Any] = {"rule_type_override": "Grade", "rule_payload_override": NEW_PAYLOAD} +CRITERION_FIELDS = {"id", "group_id", "rule_profile_id", "rule_type_override", "rule_payload_override", "object_tag_id"} + + +def bulk_update_url(group_id: int) -> str: + """Return the bulk-update endpoint's path for `group_id`, resolved through the URL name.""" + return reverse("cbe:criterion-bulk-update", kwargs={"group_id": group_id}) + + +def make_criterion(leaf: CompetencyCriteriaGroup, block_id: str, **rule) -> CompetencyCriterion: + """Create a criterion on `leaf` for a fresh object in its course, with `rule` as its rule columns.""" + assert leaf.parent is not None and leaf.parent.course is not None + object_tag = ObjectTag.objects.create( + object_id=usage_key(leaf.parent.course, block_id), taxonomy=leaf.tag.taxonomy, tag=leaf.tag, + ) + return CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, **rule) + + +@pytest.fixture(name="leaf") +def _leaf(tag: Tag, course_run: CourseRun) -> CompetencyCriteriaGroup: + """A leaf group under `tag` and `course_run`. Only a leaf group holds criteria.""" + return _create_leaf_group(tag, course_run) + + +@pytest.fixture(name="batch") +def _batch( + leaf: CompetencyCriteriaGroup, default_rule_profile: CompetencyRuleProfile +) -> tuple[CompetencyCriterion, CompetencyCriterion]: + """Two criteria on `leaf` with different rule sources: one follows the default, one has an override.""" + return ( + make_criterion(leaf, "p1", rule_profile=default_rule_profile), + make_criterion( + leaf, "p2", rule_type_override=RuleType.GRADE, rule_payload_override=dict(FIXTURE_GRADE_PAYLOAD), + ), + ) + + +def stored_rules(criteria) -> list[tuple]: + """Return each criterion's three rule columns as stored, to prove a refused request changed nothing.""" + rows = [] + for criterion in criteria: + criterion.refresh_from_db() + rows.append((criterion.rule_profile_id, criterion.rule_type_override, criterion.rule_payload_override)) + return rows + + +def test_bulk_update_resolves_to_the_documented_path() -> None: + """The bulk-update route sits under the group whose criteria it edits.""" + assert bulk_update_url(7) == "/api/cbe/v1/criteria-groups/7/criteria/bulk-update/" + + +def test_bulk_update_with_rule_values_echoes_every_criterion_as_stored( + user_client: APIClient, batch: tuple[CompetencyCriterion, CompetencyCriterion], leaf: CompetencyCriteriaGroup, +) -> None: + """Every named criterion comes back, in request order, carrying the values and no profile.""" + ids = [batch[1].id, batch[0].id] + + response = user_client.patch(bulk_update_url(leaf.id), {"criterion_ids": ids, **NEW_RULE}, format="json") + + assert response.status_code == status.HTTP_200_OK + assert [row["id"] for row in response.data] == ids + for row in response.data: + assert set(row.keys()) == CRITERION_FIELDS + assert row["group_id"] == leaf.id + assert (row["rule_profile_id"], row["rule_type_override"], row["rule_payload_override"]) == ( + None, "Grade", NEW_PAYLOAD, + ) + + +def test_bulk_update_with_a_named_profile_echoes_every_criterion_following_it( + user_client: APIClient, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """Every named criterion comes back following the profile, with no values of its own.""" + ids = [criterion.id for criterion in batch] + + response = user_client.patch( + bulk_update_url(leaf.id), {"criterion_ids": ids, "rule_profile_id": default_rule_profile.id}, format="json", + ) + + assert response.status_code == status.HTTP_200_OK + assert [row["id"] for row in response.data] == ids + for row in response.data: + assert (row["rule_profile_id"], row["rule_type_override"], row["rule_payload_override"]) == ( + default_rule_profile.id, None, None, + ) + + +def test_bulk_update_with_values_matching_the_default_reports_the_default( + user_client: APIClient, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """Values equal to the applicable default are reported as following it, not as an override.""" + body = { + "criterion_ids": [criterion.id for criterion in batch], + "rule_type_override": "Grade", + "rule_payload_override": SEEDED_GRADE_PAYLOAD, + } + + response = user_client.patch(bulk_update_url(leaf.id), body, format="json") + + assert response.status_code == status.HTTP_200_OK + assert {row["rule_profile_id"] for row in response.data} == {default_rule_profile.id} + assert {row["rule_payload_override"] for row in response.data} == {None} + + +@pytest.mark.parametrize( + "case, error_key", + [ + ("empty id list", "criterion_ids"), + ("duplicate id", "criterion_ids"), + ("ids not a list", "criterion_ids"), + ("both rule forms", "rule_profile_id"), + ("neither rule form", "__all__"), + ("rule type without payload", "rule_type_override"), + ("payload without rule type", "rule_payload_override"), + ("unknown profile", "rule_profile_id"), + ("archived profile", "rule_profile_id"), + ("unsupported comparison", "rule_payload_override"), + ("threshold out of range", "rule_payload_override"), + ("unimplemented rule type", "rule_type_override"), + ("unrecognized key", "group_id"), + ], +) +def test_bulk_update_rejects_a_malformed_body_with_400( # pylint: disable=too-many-positional-arguments + case: str, + error_key: str, + user_client: APIClient, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + default_rule_profile: CompetencyRuleProfile, + competency_taxonomy: CompetencyTaxonomy, +) -> None: + """Each malformed body is a 400 naming the offending field, and no criterion changes.""" + first, second = batch + ids = [first.id, second.id] + archived = make_profile(competency_taxonomy=competency_taxonomy, archived=True) + bodies: dict[str, dict[str, Any]] = { + "empty id list": {"criterion_ids": [], **NEW_RULE}, + "duplicate id": {"criterion_ids": [first.id, first.id], **NEW_RULE}, + "ids not a list": {"criterion_ids": first.id, **NEW_RULE}, + "both rule forms": {"criterion_ids": ids, "rule_profile_id": default_rule_profile.id, **NEW_RULE}, + "neither rule form": {"criterion_ids": ids}, + "rule type without payload": {"criterion_ids": ids, "rule_type_override": "Grade"}, + "payload without rule type": {"criterion_ids": ids, "rule_payload_override": NEW_PAYLOAD}, + "unknown profile": {"criterion_ids": ids, "rule_profile_id": 999999}, + "archived profile": {"criterion_ids": ids, "rule_profile_id": archived.id}, + "unsupported comparison": { + "criterion_ids": ids, **NEW_RULE, "rule_payload_override": {**NEW_PAYLOAD, "op": "gt"}, + }, + "threshold out of range": { + "criterion_ids": ids, **NEW_RULE, "rule_payload_override": {**NEW_PAYLOAD, "value": 75}, + }, + "unimplemented rule type": {"criterion_ids": ids, **NEW_RULE, "rule_type_override": "Completion"}, + # A criterion's group is fixed; this endpoint has no field that could move one. + "unrecognized key": {"criterion_ids": ids, **NEW_RULE, "group_id": leaf.id}, + } + before = stored_rules(batch) + + response = user_client.patch(bulk_update_url(leaf.id), bodies[case], format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert list(response.data) == [error_key] + assert stored_rules(batch) == before + + +def test_bulk_update_on_a_group_that_is_not_a_leaf_is_400( + user_client: APIClient, batch: tuple[CompetencyCriterion, CompetencyCriterion], leaf: CompetencyCriteriaGroup, +) -> None: + """A course-level group holds no criteria of its own, so it cannot be addressed here.""" + assert leaf.parent is not None + + response = user_client.patch( + bulk_update_url(leaf.parent.id), {"criterion_ids": [batch[0].id], **NEW_RULE}, format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "group_id" in response.data + + +@pytest.mark.parametrize("stray", ["from another group", "nonexistent"]) +def test_bulk_update_naming_a_criterion_outside_the_group_is_404( # pylint: disable=too-many-positional-arguments + stray: str, + user_client: APIClient, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + tag: Tag, + course_run: CourseRun, +) -> None: + """One id outside the group fails the whole batch with a 404 naming it, and nothing changes.""" + if stray == "from another group": + stray_id = make_criterion(_create_leaf_group(tag, course_run), "p3", **NEW_RULE).id + else: + stray_id = 999999 + before = stored_rules(batch) + + response = user_client.patch( + bulk_update_url(leaf.id), {"criterion_ids": [batch[0].id, stray_id], **NEW_RULE}, format="json", + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + assert str(stray_id) in response.data["detail"] + assert stored_rules(batch) == before + + +def test_bulk_update_on_a_group_that_does_not_exist_is_404( + user_client: APIClient, batch: tuple[CompetencyCriterion, CompetencyCriterion], +) -> None: + """An unknown group id in the URL is a missing resource.""" + response = user_client.patch(bulk_update_url(999999), {"criterion_ids": [batch[0].id], **NEW_RULE}, format="json") + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_bulk_update_naming_an_archived_criterion_is_409( + user_client: APIClient, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """One archived criterion refuses the whole batch with a 409 naming it, and nothing changes.""" + archived = make_criterion(leaf, "p3", rule_profile=default_rule_profile, archived=True) + criteria = [batch[0], archived, batch[1]] + before = stored_rules(criteria) + + response = user_client.patch( + bulk_update_url(leaf.id), {"criterion_ids": [c.id for c in criteria], **NEW_RULE}, format="json", + ) + + assert response.status_code == status.HTTP_409_CONFLICT + assert str(archived.id) in response.data["detail"] + assert stored_rules(criteria) == before + + +def test_bulk_update_on_an_archived_group_is_409( + user_client: APIClient, batch: tuple[CompetencyCriterion, CompetencyCriterion], leaf: CompetencyCriteriaGroup, +) -> None: + """An archived leaf refuses the whole batch with a 409 naming it, though its criteria aren't archived.""" + CompetencyCriteriaGroup.objects.filter(pk=leaf.pk).update(archived=True) + before = stored_rules(batch) + + response = user_client.patch( + bulk_update_url(leaf.id), {"criterion_ids": [c.id for c in batch], **NEW_RULE}, format="json", + ) + + assert response.status_code == status.HTTP_409_CONFLICT + assert f"group {leaf.id}" in response.data["detail"] + assert stored_rules(batch) == before + + +def test_bulk_update_without_permission_on_the_groups_course_is_403( + user_client: APIClient, tag: Tag, organization: Organization, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A caller who may not tag objects in the group's course is refused, and nothing changes.""" + unauthorized_course = make_course_run(organization, UNAUTHORIZED_MARKER, "Fall2026") + leaf = _create_leaf_group(tag, unauthorized_course) + criterion = make_criterion(leaf, "p1", rule_profile=default_rule_profile) + + response = user_client.patch(bulk_update_url(leaf.id), {"criterion_ids": [criterion.id], **NEW_RULE}, format="json") + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert stored_rules([criterion]) == [(default_rule_profile.id, None, None)] + + +def test_bulk_update_from_an_unidentified_caller_is_401( + api_client: APIClient, batch: tuple[CompetencyCriterion, CompetencyCriterion], leaf: CompetencyCriteriaGroup, +) -> None: + """A caller the system cannot identify is refused before anything is read.""" + response = api_client.patch(bulk_update_url(leaf.id), {"criterion_ids": [batch[0].id], **NEW_RULE}, format="json") + + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + +@pytest.mark.parametrize("method", ["get", "put", "post", "delete"]) +def test_bulk_update_offers_only_patch(method: str, user_client: APIClient, leaf: CompetencyCriteriaGroup) -> None: + """Every method other than PATCH is refused as not allowed.""" + response = getattr(user_client, method)(bulk_update_url(leaf.id), {}, format="json") + + assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED