From c91b6faeb22e4fe9394c6566ce171946a5c1a199 Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Thu, 15 Oct 2026 21:19:30 +0400 Subject: [PATCH] feat: add endpoint for creating competency criteria Add POST /api/cbe/v1/competencies//criteria/, associating a gradeable subsection with a competency. When group_id is supplied, the CompetencyCriterion is attached to that existing leaf CompetencyCriteriaGroup. When it is omitted, the root and course-level groups for this competency and course are reused or created, and a new leaf is always created under them. Add to applets/cbe/api.py, all marked UNSTABLE: resolve_competency_tag(), resolve_supplied_leaf_group(), create_competency_criterion(), and associate_competency_criterion() as the single entry point the view calls. Group creation is the private _create_leaf_group(), so groups are only ever created behind the duplicate-association check. The API raises Django ValidationError and model DoesNotExist exceptions; the view translates them into 400 and 404 responses, and a 404 carries the exception's message, so a missing tag, a tag outside a competency taxonomy, and a missing group can be told apart. Group creation and the duplicate-association check run inside one transaction that first locks the competency's Tag row with select_for_update(), so concurrent requests for the same competency are serialized. MySQL has no partial unique indexes, so the one-root-per-tag rule cannot be backed by a constraint and relies on that lock. Add migration 0015: a UniqueConstraint on CompetencyCriteriaGroup (tag, course, parent) that backstops course-level group creation. The root and course-level lookups match archived rows deliberately, so an archived container is never replaced by a second row in its slot. When a request supplies no rule fields, the criterion is assigned the live rule profile scoped to the competency's taxonomy if one exists, and the seeded system-default profile otherwise. It is never left with all three rule fields null, because CompetencyCriterion's profile_xor_override_check constraint forbids that. A request that supplies a profile together with an override, only one of the two overrides, or an archived or unknown profile is rejected with a 400 before anything is written. Related to openedx/openedx-core#665 Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 272 ++++++++- .../applets/cbe/models/criteria.py | 15 + .../applets/cbe/rest_api/v1/serializers.py | 47 +- .../applets/cbe/rest_api/v1/urls.py | 5 + .../applets/cbe/rest_api/v1/views.py | 68 ++- .../0015_criteria_group_unique_constraints.py | 26 + .../openedx_learning/applets/cbe/test_api.py | 356 ++++++++++- .../applets/cbe/test_views.py | 570 +++++++++++++++++- tests/openedx_tagging/test_api.py | 4 +- 9 files changed, 1350 insertions(+), 13 deletions(-) create mode 100644 src/openedx_learning/migrations/0015_criteria_group_unique_constraints.py diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 4aed9cba8..e2268c297 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -3,17 +3,34 @@ """ from __future__ import annotations +from django.core.exceptions import ValidationError +from django.db import transaction from django.db.models import QuerySet +from django.utils.translation import gettext_lazy as _ +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import UsageKey -from openedx_tagging.api import create_taxonomy -from openedx_tagging.models import Taxonomy +from openedx_catalog.api import get_course_run +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 .models import CompetencyRuleProfile, CompetencyTaxonomy +from .models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + LogicOperator, +) __all__ = [ + "associate_competency_criterion", + "create_competency_criterion", "create_competency_taxonomy", "get_competency_rule_profiles", "is_competency_taxonomy", + "resolve_competency_tag", + "resolve_supplied_leaf_group", "select_competency_taxonomies", ] @@ -79,3 +96,252 @@ def select_competency_taxonomies(taxonomies: QuerySet[Taxonomy]) -> QuerySet[Tax so the check costs no additional query per row. """ return taxonomies.select_related("competencytaxonomy") + + +def resolve_competency_tag(tag_id: int) -> Tag: + """ + Return the competency Tag identified by ``tag_id``. + + 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. + + Raises Tag.DoesNotExist if no such Tag exists, or if it isn't on a CompetencyTaxonomy. + Shared by the view and :func:`associate_competency_criterion` so both agree on what counts + as valid. + """ + tag = Tag.objects.get(pk=tag_id) + # Explicit None check narrows the type for mypy. + if tag.taxonomy is None or not is_competency_taxonomy(tag.taxonomy): + raise Tag.DoesNotExist("Tag is not on a CompetencyTaxonomy.") + return tag + + +def _create_leaf_group( + tag: Tag, + course_run: CourseRun, + logic_operator: str | None = None, +) -> CompetencyCriteriaGroup: + """ + Return a fresh leaf CompetencyCriteriaGroup under ``tag``'s root/course-level groups. + + Gets or creates the root and course-level groups, then always creates a new leaf: a + tag/course pair can hold more than one leaf, one per criterion. The root and course-level + group names are truncated to fit the name column; they are never shown in the UI. + + Must run inside a transaction holding the tag row lock. + """ + name_max_length = CompetencyCriteriaGroup._meta.get_field("name").max_length + # Both get_or_create() lookups match archived rows deliberately, so an archived container is + # never replaced; rejecting or reactivating it is the archived-group checks' decision. + root, _ = CompetencyCriteriaGroup.objects.get_or_create( + tag=tag, + parent=None, + defaults={"name": f"{tag.value} (root)"[:name_max_length]}, + ) + course_level_group, _ = CompetencyCriteriaGroup.objects.get_or_create( + tag=tag, + course=course_run, + parent=root, + defaults={"name": f"{tag.value} — {course_run.title}"[:name_max_length]}, + ) + return CompetencyCriteriaGroup.objects.create( + tag=tag, + course=None, + parent=course_level_group, + logic_operator=logic_operator or LogicOperator.OR, + ) + + +def resolve_supplied_leaf_group(group_id: int, tag: Tag, course_run: CourseRun) -> CompetencyCriteriaGroup: + """ + Return the CompetencyCriteriaGroup identified by ``group_id``, validated as a usable leaf. + + 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. + + Raises CompetencyCriteriaGroup.DoesNotExist if it doesn't exist, or ValidationError (keyed + "group_id") if it isn't a leaf, doesn't belong to ``tag``, or its course doesn't match + ``course_run``. + """ + group = CompetencyCriteriaGroup.objects.get(pk=group_id) + if group.parent_id is None or group.course_id is not None: + raise ValidationError({"group_id": _("group_id must reference a leaf CompetencyCriteriaGroup.")}) + if group.tag_id != tag.id: + raise ValidationError({"group_id": _("group_id must belong to the given competency tag.")}) + if group.parent.course_id != course_run.id: + raise ValidationError({"group_id": _("group_id must belong to the given course.")}) + return group + + +def create_competency_criterion( + group: CompetencyCriteriaGroup, + object_id: str, + rule_profile_id: int | None = None, + rule_type_override: str | None = None, + rule_payload_override: dict | None = None, +) -> CompetencyCriterion: + """ + Create and return a CompetencyCriterion under ``group``, tagging ``object_id`` along the way. + + UNSTABLE: the criteria-authoring family is incomplete, so its validation and group update + and archive behavior may change this function's shape without a deprecation cycle. + + ``tag_object()`` replaces an object's full tag list for a taxonomy rather than appending, so + this reads the object's existing tags first and unions in the new one, rather than silently + dropping a sibling competency's tag under the same taxonomy. Raises ValidationError (keyed + "object_id") when the taxonomy refuses the tag, e.g. it allows only one tag per object. + + 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. + """ + tag = group.tag + # tag.taxonomy_id is nullable at the model level; already guaranteed set by the caller. + assert tag.taxonomy_id is not None + existing_values = [existing_tag.value for existing_tag in get_object_tags(object_id, taxonomy_id=tag.taxonomy_id)] + if tag.value not in existing_values: + existing_values.append(tag.value) + try: + tag_object(object_id, tag.taxonomy, existing_values) + except ValueError as exc: + raise ValidationError({"object_id": str(exc)}) from exc + object_tag = ObjectTag.objects.get(object_id=object_id, taxonomy_id=tag.taxonomy_id, tag_id=tag.id) + + if rule_profile_id is None and rule_type_override is None and rule_payload_override is None: + 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 + ) + rule_profile_id = profile.id + + # Parent-competency dominance/containment validation belongs here, before creation. + + return CompetencyCriterion.objects.create( + group=group, + object_tag=object_tag, + rule_profile_id=rule_profile_id, + rule_type_override=rule_type_override, + rule_payload_override=rule_payload_override, + ) + + +def _reject_duplicate_criterion(tag: Tag, object_id: str, group_id: int | None) -> None: + """ + Raise ValidationError if ``object_id`` is already associated with ``tag`` (in ``group_id``, when supplied). + + Must run inside a transaction holding the tag row lock. + """ + assert tag.taxonomy_id is not None # narrowing for mypy; callers resolve competency tags only + existing_object_tag = ObjectTag.objects.filter( + object_id=object_id, taxonomy_id=tag.taxonomy_id, tag_id=tag.id, + ).first() + if existing_object_tag is None: + return + duplicate_criteria = CompetencyCriterion.objects.filter(object_tag=existing_object_tag) + if group_id is None: + # Can't know which group slot is intended, so any existing criterion is a duplicate. + if duplicate_criteria.exists(): + raise ValidationError( + {"object_id": _("A CompetencyCriterion already associates this tag with this object_id.")} + ) + elif duplicate_criteria.filter(group_id=group_id).exists(): + # A different explicit group is a deliberate second association (ADR-0002), not a duplicate. + raise ValidationError( + {"group_id": _("A CompetencyCriterion already associates this tag with this object_id in this group.")} + ) + + +def _validate_rule_fields( + rule_profile_id: int | None, + rule_type_override: str | None, + rule_payload_override: dict | None, +) -> None: + """ + Raise ValidationError, keyed by the offending field, unless the rule fields form a valid combination. + + Valid: a live ``rule_profile_id`` alone, both overrides without a profile, or none of the three. + """ + if rule_profile_id is not None and (rule_type_override is not None or rule_payload_override is not None): + raise ValidationError({"rule_profile_id": _( + "rule_profile_id cannot be combined with rule_type_override or rule_payload_override." + )}) + if (rule_type_override is None) != (rule_payload_override is None): + supplied_field = "rule_type_override" if rule_type_override is not None else "rule_payload_override" + raise ValidationError({supplied_field: _( + "rule_type_override and rule_payload_override must be supplied together." + )}) + if rule_profile_id is None: + return + archived = CompetencyRuleProfile.objects.filter(pk=rule_profile_id).values_list("archived", flat=True).first() + if archived is None: + raise ValidationError({"rule_profile_id": _("No CompetencyRuleProfile matches rule_profile_id.")}) + if archived: + # Archived profiles stay resolvable for existing criteria but are hidden from new associations. + raise ValidationError({"rule_profile_id": _("rule_profile_id references an archived CompetencyRuleProfile.")}) + + +def associate_competency_criterion( + tag_id: int, + object_id: str, + *, + group_id: int | None = None, + logic_operator: str | None = None, + rule_profile_id: int | None = None, + rule_type_override: str | None = None, + rule_payload_override: dict | None = None, +) -> CompetencyCriterion: + """ + Associate ``object_id`` with the competency ``tag_id`` names, creating a criterion for it. + + 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. + + The public entry point for the create-criterion endpoint; the caller is expected to have + already checked ``oel_tagging.can_tag_object``. Rejects re-targeting the same group a + (tag_id, object_id) pair already uses, and rejects a duplicate via the derive-or-create + path; a *different* explicit group is a deliberate second association per ADR-0002, not a + duplicate. Branches on ``group_id`` to resolve or derive/create a leaf, then creates the + criterion, all inside one transaction so a downstream failure rolls back any group just + created. + + Raises ValidationError (never DRF's) on rejected input, including an invalid combination of + rule fields or a missing or archived ``rule_profile_id``; Tag.DoesNotExist if ``tag_id`` + doesn't resolve to a competency tag; CompetencyCriteriaGroup.DoesNotExist if ``group_id`` + doesn't resolve. + """ + tag = resolve_competency_tag(tag_id) + # tag.taxonomy is already confirmed set; re-asserted since that doesn't cross function boundaries for mypy. + assert tag.taxonomy_id is not None + + try: + usage_key = UsageKey.from_string(object_id) + except InvalidKeyError as exc: + raise ValidationError({"object_id": _("object_id is not a valid usage key.")}) from exc + + try: + course_run = get_course_run(usage_key.course_key) + except CourseRun.DoesNotExist as exc: + raise ValidationError({"object_id": _("No course run matches object_id's course.")}) from exc + + if group_id is not None and logic_operator is not None: + raise ValidationError({"logic_operator": _("group_id and logic_operator cannot both be supplied.")}) + _validate_rule_fields(rule_profile_id, rule_type_override, rule_payload_override) + + with transaction.atomic(): + # Serializes the duplicate check and root creation; MySQL has no partial unique index for one root per tag. + Tag.objects.select_for_update().get(pk=tag.pk) + _reject_duplicate_criterion(tag, object_id, group_id) + if group_id is not None: + group = resolve_supplied_leaf_group(group_id, tag, course_run) + else: + group = _create_leaf_group(tag, course_run, logic_operator) + return create_competency_criterion( + group=group, + object_id=object_id, + rule_profile_id=rule_profile_id, + rule_type_override=rule_type_override, + rule_payload_override=rule_payload_override, + ) diff --git a/src/openedx_learning/applets/cbe/models/criteria.py b/src/openedx_learning/applets/cbe/models/criteria.py index d8086496e..9fb7d0119 100644 --- a/src/openedx_learning/applets/cbe/models/criteria.py +++ b/src/openedx_learning/applets/cbe/models/criteria.py @@ -115,6 +115,21 @@ class Meta: # indexes every ForeignKey column by default, so a second explicit one here would only # cost write throughput without adding any read benefit. ] + # No constraint tying `logic_operator` to child count, and no UniqueConstraint on (parent, + # ordering): a child group cannot be saved until its parent's primary key exists, so + # neither has a single-row state to check at save time. See ADR-0002 Decision 2. + constraints = [ + # Backstops _create_leaf_group()'s get_or_create() for course-level groups. It has no + # `condition` because MySQL has no partial indexes: a unique index allows repeated + # NULLs, and `course` is NULL for roots and leaves. + models.UniqueConstraint( + fields=["tag", "course", "parent"], + name="oel_cbe_criteria_group_one_course_group_per_tag_course", + violation_error_message=_( + "A competency tag may have at most one course-level CompetencyCriteriaGroup per course." + ), + ), + ] class CompetencyRuleProfile(models.Model): 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 02ee97927..97b179541 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py @@ -3,9 +3,12 @@ """ from __future__ import annotations +from django.utils.translation import gettext_lazy as _ +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import UsageKey from rest_framework import serializers -from ...models import CompetencyRuleProfile +from ...models import CompetencyCriterion, CompetencyRuleProfile, LogicOperator class CompetencyRuleProfileSerializer(serializers.ModelSerializer): @@ -54,3 +57,45 @@ def get_scope_type(self, profile: CompetencyRuleProfile) -> str: if profile.organization_id is not None: return "organization" return "system_default" + + +class CompetencyCriterionSerializer(serializers.ModelSerializer): + """ + Doubles as the request-body parser and the response representation for a criterion. + + ``object_id`` and ``logic_operator`` are not CompetencyCriterion fields at all (``object_id`` + isn't stored anywhere on this model; ``logic_operator`` belongs to CompetencyCriteriaGroup), + so they're declared as plain write_only fields the view reads out of ``validated_data``, not + model-bound fields. Every other field's JSON name matches its model attribute exactly + (``group_id``, ``rule_profile_id``, ``object_tag_id``), so none of them need a ``source=``. + """ + + object_id = serializers.CharField(write_only=True) + group_id = serializers.IntegerField(required=False, allow_null=True) + # Only ever applies on the derive-or-create path (group_id omitted): it sets the AND/OR + # operator on the brand-new leaf group this request creates. Rejected alongside an explicit + # group_id because changing an existing leaf's operator is a future group-update endpoint's + # job, not this one's -- a caller can't use this field to silently change an existing + # group's behavior. + logic_operator = serializers.ChoiceField( + choices=LogicOperator.choices, write_only=True, required=False, allow_null=True, + ) + rule_profile_id = serializers.IntegerField(required=False, allow_null=True) + object_tag_id = serializers.IntegerField(read_only=True) + + class Meta: + model = CompetencyCriterion + fields = [ + "id", "object_id", "group_id", "logic_operator", + "rule_profile_id", "rule_type_override", "rule_payload_override", "object_tag_id", + ] + read_only_fields = ["id", "object_tag_id"] + + def validate_object_id(self, value: str) -> str: + """Reject an object_id that isn't a parseable usage key.""" + # Checked before the view's permission check, which denies an unparseable key as a 403 rather than a 400. + try: + UsageKey.from_string(value) + except InvalidKeyError as exc: + raise serializers.ValidationError(_("object_id is not a valid usage key.")) from exc + return value 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 32b9fd778..f52abfa78 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py @@ -12,4 +12,9 @@ urlpatterns = [ path("", include(router.urls)), + path( + "competencies//criteria/", + views.CompetencyCriterionCreateView.as_view(), + name="criterion-create", + ), ] 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 574ec522a..4be3149cf 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -3,19 +3,27 @@ """ from __future__ import annotations +from django.core.exceptions import ValidationError as DjangoValidationError from django.db.models import QuerySet 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 mixins +from rest_framework import generics, mixins, status +from rest_framework.exceptions import NotFound, PermissionDenied +from rest_framework.exceptions import ValidationError as DRFValidationError +from rest_framework.permissions import IsAuthenticated +from rest_framework.response import Response from rest_framework.viewsets import GenericViewSet -from ...api import get_competency_rule_profiles -from ...models import CompetencyRuleProfile +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 ..paginators import CompetencyRuleProfilePagination from .permissions import CompetencyRuleProfilePermissions -from .serializers import CompetencyRuleProfileSerializer +from .serializers import CompetencyCriterionSerializer, CompetencyRuleProfileSerializer class CompetencyRuleProfileView(mixins.ListModelMixin, GenericViewSet): @@ -44,3 +52,55 @@ class CompetencyRuleProfileView(mixins.ListModelMixin, GenericViewSet): def get_queryset(self) -> QuerySet[CompetencyRuleProfile]: """Return the live rule profiles, filtered and ordered by the applet's public API.""" return get_competency_rule_profiles() + + +class CompetencyCriterionCreateView(generics.CreateAPIView): + """ + POST-only. Create a CompetencyCriterion associating an object_id with a competency tag. + + Overrides ``create()`` rather than relying on ``ModelSerializer.save()``, because creation + is a multi-step domain operation (tag/course resolution, permission check, duplicate check, + group derivation, criterion creation) that :func:`associate_competency_criterion` already + owns end-to-end -- the serializer's job here is validating/shaping the request body and + representing the result, not constructing the instance itself. + + ``permission_classes`` is ``IsAuthenticated`` only, not a ``DjangoObjectPermissions`` + subclass: every existing such class in this codebase only ever contributes the + authentication gate, since the ``rules`` predicate behind it returns True whenever called + with no object (as every class-level check does). Copying that shape here would mean + registering a new, unused ``openedx_learning.add_competencycriterion`` permission for no + behavioral gain, so the real, object-scoped authorization is a manual ``has_perm()`` call + below instead. + """ + + serializer_class = CompetencyCriterionSerializer + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = [IsAuthenticated] + + def create(self, request, *args, **kwargs): + """Validate the request body, check permission, then delegate to the public API.""" + serializer = self.get_serializer(data=request.data) + serializer.is_valid(raise_exception=True) + data = serializer.validated_data + try: + tag = resolve_competency_tag(self.kwargs["tag_id"]) + except Tag.DoesNotExist as exc: + raise NotFound(str(exc)) from exc + perm_obj = ObjectTagPermissionItem(taxonomy=tag.taxonomy, object_id=data["object_id"]) + if not request.user.has_perm("oel_tagging.can_tag_object", perm_obj): + raise PermissionDenied() + try: + criterion = associate_competency_criterion( + tag_id=tag.id, + object_id=data["object_id"], + group_id=data.get("group_id"), + logic_operator=data.get("logic_operator"), + rule_profile_id=data.get("rule_profile_id"), + rule_type_override=data.get("rule_type_override"), + rule_payload_override=data.get("rule_payload_override"), + ) + except (Tag.DoesNotExist, 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 + return Response(self.get_serializer(criterion).data, status=status.HTTP_201_CREATED) diff --git a/src/openedx_learning/migrations/0015_criteria_group_unique_constraints.py b/src/openedx_learning/migrations/0015_criteria_group_unique_constraints.py new file mode 100644 index 000000000..2de9caffe --- /dev/null +++ b/src/openedx_learning/migrations/0015_criteria_group_unique_constraints.py @@ -0,0 +1,26 @@ +""" +Add the UniqueConstraint on CompetencyCriteriaGroup that backstops course-level group creation. +""" +# Generated by Django 5.2.16 on 2026-09-11 00:00 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_learning', '0014_add_archived_to_criteria_group_and_criterion'), + ] + + operations = [ + migrations.AddConstraint( + model_name='competencycriteriagroup', + constraint=models.UniqueConstraint( + fields=('tag', 'course', 'parent'), + name='oel_cbe_criteria_group_one_course_group_per_tag_course', + violation_error_message=( + 'A competency tag may have at most one course-level CompetencyCriteriaGroup per course.' + ), + ), + ), + ] diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index b37d1b15a..0cfeafe62 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -1,17 +1,32 @@ """ Tests for the CBE public API surface (openedx_learning.api). """ + import pytest +from django.core.exceptions import ValidationError +from django.db.utils import IntegrityError from organizations.models import Organization +from openedx_catalog.models import CatalogCourse, CourseRun from openedx_learning.api import ( + associate_competency_criterion, create_competency_taxonomy, get_competency_rule_profiles, is_competency_taxonomy, + resolve_supplied_leaf_group, select_competency_taxonomies, ) -from openedx_learning.models import CompetencyRuleProfile, CompetencyTaxonomy, RuleType -from openedx_tagging.models import Taxonomy +from openedx_learning.applets.cbe import api as cbe_api +from openedx_learning.applets.cbe.api import _create_leaf_group +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + LogicOperator, + RuleType, +) +from openedx_tagging.models import Tag, Taxonomy pytestmark = pytest.mark.django_db @@ -63,6 +78,19 @@ def test_create_competency_taxonomy_defaults() -> None: assert result.export_id +def make_course_run(organization: Organization, course_code: str, run_code: str) -> CourseRun: + """Create a CourseRun distinct from the `course_run` fixture, for the same-tag-different-course tests.""" + catalog_course = CatalogCourse.objects.create(org=organization, course_code=course_code) + return CourseRun.objects.create(catalog_course=catalog_course, run_code=run_code) + + +def usage_key(course_run: CourseRun, block_id: str) -> str: + """Build a gradeable-subsection-shaped usage key string under `course_run`.""" + key = course_run.course_key + assert key is not None + return f"block-v1:{key.org}+{key.course}+{key.run}+type@sequential+block@{block_id}" + + def test_is_competency_taxonomy() -> None: """ is_competency_taxonomy() is True for a competency taxonomy, False for a plain one. @@ -158,3 +186,327 @@ def test_get_competency_rule_profiles_is_ordered_by_id( expected = [default_rule_profile, taxonomy_scoped, organization_scoped] assert list(get_competency_rule_profiles()) == expected + + +# ============================================================================================== +# _create_leaf_group +# ============================================================================================== + + +def test_create_leaf_group_builds_the_full_hierarchy_on_first_use( + tag: Tag, course_run: CourseRun +) -> None: + """A first call with no existing groups creates a root, a course-level group, and a leaf.""" + leaf = _create_leaf_group(tag, course_run) + + course_level = leaf.parent + assert course_level is not None + root = course_level.parent + assert root is not None + assert root.parent is None + assert root.tag_id == tag.id + assert root.course_id is None + assert root.name == f"{tag.value} (root)" + assert course_level.tag_id == tag.id + assert course_level.course_id == course_run.id + assert course_level.name == f"{tag.value} — {course_run.title}" + assert leaf.tag_id == tag.id + assert leaf.course_id is None + assert leaf.logic_operator == LogicOperator.OR + + +def test_create_leaf_group_passes_through_an_explicit_logic_operator( + tag: Tag, course_run: CourseRun +) -> None: + """A supplied logic_operator is stored on the leaf as-is, not defaulted to OR.""" + leaf = _create_leaf_group(tag, course_run, logic_operator=LogicOperator.AND) + assert leaf.logic_operator == LogicOperator.AND + + +def test_create_leaf_group_reuses_the_root_and_course_level_group_on_a_second_call( + tag: Tag, course_run: CourseRun +) -> None: + """A second call for the same tag and course reuses the root and course-level group.""" + first_leaf = _create_leaf_group(tag, course_run) + second_leaf = _create_leaf_group(tag, course_run) + + assert first_leaf.id != second_leaf.id + assert first_leaf.parent_id == second_leaf.parent_id + assert first_leaf.parent is not None + assert second_leaf.parent is not None + assert first_leaf.parent.parent_id == second_leaf.parent.parent_id + assert CompetencyCriteriaGroup.objects.filter(tag=tag, parent__isnull=True).count() == 1 + assert CompetencyCriteriaGroup.objects.filter(tag=tag, course=course_run).count() == 1 + + +def test_create_leaf_group_creates_a_new_course_level_group_for_a_different_course( + tag: Tag, course_run: CourseRun, organization: Organization +) -> None: + """A second course under the same tag gets its own course-level group, but shares the root.""" + other_course_run = make_course_run(organization, "Python200", "Fall2026") + + first_leaf = _create_leaf_group(tag, course_run) + second_leaf = _create_leaf_group(tag, other_course_run) + + assert first_leaf.parent is not None + assert second_leaf.parent is not None + assert first_leaf.parent.parent_id == second_leaf.parent.parent_id + assert first_leaf.parent_id != second_leaf.parent_id + assert CompetencyCriteriaGroup.objects.filter(tag=tag, parent__isnull=True).count() == 1 + assert CompetencyCriteriaGroup.objects.filter(tag=tag, course__isnull=False).count() == 2 + + +def test_create_leaf_group_reuses_a_root_a_concurrent_request_already_committed( + tag: Tag, course_run: CourseRun +) -> None: + """ + A root created out-of-band (standing in for a concurrent request's winning commit) is reused. + + Callers are serialized on the tag row, so a request that waited on the lock finds the + winner's committed root and ``get_or_create()`` fetches it instead of creating a second one. + """ + already_committed_root = CompetencyCriteriaGroup.objects.create(tag=tag, parent=None, name="pre-existing root") + + leaf = _create_leaf_group(tag, course_run) + + assert leaf.parent is not None + assert leaf.parent.parent_id == already_committed_root.id + assert CompetencyCriteriaGroup.objects.filter(tag=tag, parent__isnull=True).count() == 1 + + +def test_create_leaf_group_reuses_a_course_level_group_a_concurrent_request_already_committed( + tag: Tag, course_run: CourseRun +) -> None: + """The course-level group half of the same guarantee, isolated from the root.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag, parent=None) + already_committed_course_level = CompetencyCriteriaGroup.objects.create( + tag=tag, course=course_run, parent=root, name="pre-existing course-level group", + ) + + leaf = _create_leaf_group(tag, course_run) + + assert leaf.parent_id == already_committed_course_level.id + assert CompetencyCriteriaGroup.objects.filter(tag=tag, course=course_run).count() == 1 + + +def test_create_leaf_group_truncates_derived_names_to_fit_the_name_column( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun +) -> None: + """A maximum-length tag value (500) and course title (255) still produce storable root and course-level names.""" + long_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="t" * 500) + course_run.title = "c" * 255 + course_run.save() + + leaf = _create_leaf_group(long_tag, course_run) + + course_level = leaf.parent + assert course_level is not None + root = course_level.parent + assert root is not None + name_max_length = CompetencyCriteriaGroup._meta.get_field("name").max_length + assert len(CompetencyCriteriaGroup.objects.get(pk=root.pk).name) == name_max_length + assert len(CompetencyCriteriaGroup.objects.get(pk=course_level.pk).name) == name_max_length + + +def test_course_level_group_unique_constraint_rejects_a_second_group_for_the_same_tag_and_course( + tag: Tag, course_run: CourseRun +) -> None: + """ + The DB constraint backstopping _create_leaf_group()'s course-level get_or_create() exists. + + Proven directly (bypassing get_or_create) so a silently-dropped migration can't go unnoticed. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag, parent=None) + CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + with pytest.raises(IntegrityError): + CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + + +# ============================================================================================== +# resolve_supplied_leaf_group +# ============================================================================================== + + +def test_resolve_supplied_leaf_group_returns_the_leaf_when_it_is_valid(tag: Tag, course_run: CourseRun) -> None: + """A group_id that names a genuine, matching leaf is returned unchanged.""" + leaf = _create_leaf_group(tag, course_run) + assert resolve_supplied_leaf_group(leaf.id, tag, course_run) == leaf + + +def test_resolve_supplied_leaf_group_raises_does_not_exist_for_an_unknown_group( + tag: Tag, course_run: CourseRun +) -> None: + """An unknown group_id raises CompetencyCriteriaGroup.DoesNotExist.""" + with pytest.raises(CompetencyCriteriaGroup.DoesNotExist): + resolve_supplied_leaf_group(999999, tag, course_run) + + +def test_resolve_supplied_leaf_group_rejects_a_root_group_as_not_a_leaf(tag: Tag, course_run: CourseRun) -> None: + """A root group (no parent) is not a usable leaf.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag, parent=None) + with pytest.raises(ValidationError, match="group_id"): + resolve_supplied_leaf_group(root.id, tag, course_run) + + +def test_resolve_supplied_leaf_group_rejects_a_course_level_group_as_not_a_leaf( + tag: Tag, course_run: CourseRun +) -> None: + """A course-level group (has its own course) is not a usable leaf either.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag, parent=None) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, course=course_run, parent=root) + with pytest.raises(ValidationError, match="group_id"): + resolve_supplied_leaf_group(course_level.id, tag, course_run) + + +def test_resolve_supplied_leaf_group_rejects_a_group_for_a_different_tag( + tag: Tag, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy +) -> None: + """A leaf that belongs to a different competency tag is rejected.""" + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Other Competency") + leaf = _create_leaf_group(other_tag, course_run) + with pytest.raises(ValidationError, match="group_id"): + resolve_supplied_leaf_group(leaf.id, tag, course_run) + + +def test_resolve_supplied_leaf_group_rejects_a_group_for_a_different_course( + tag: Tag, course_run: CourseRun, organization: Organization +) -> None: + """A leaf whose course-level parent belongs to a different course is rejected.""" + other_course_run = make_course_run(organization, "Python200", "Fall2026") + leaf = _create_leaf_group(tag, other_course_run) + with pytest.raises(ValidationError, match="group_id"): + resolve_supplied_leaf_group(leaf.id, tag, course_run) + + +# ============================================================================================== +# associate_competency_criterion +# ============================================================================================== + + +def test_associate_competency_criterion_creates_the_hierarchy_and_criterion_when_nothing_exists( + tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile +) -> None: + """The happy path: no group_id, no existing groups, no rule fields supplied.""" + object_id = usage_key(course_run, "p1") + + criterion = associate_competency_criterion(tag_id=tag.id, object_id=object_id) + + assert criterion.group.tag_id == tag.id + assert criterion.group.parent is not None + assert criterion.group.parent.course_id == course_run.id + root = criterion.group.parent.parent + assert root is not None + assert root.parent is None + assert root.tag_id == tag.id + assert criterion.object_tag.object_id == object_id + assert criterion.object_tag.tag_id == tag.id + assert criterion.rule_profile_id == default_rule_profile.id + assert criterion.rule_type_override is None + assert criterion.rule_payload_override is None + + +def test_associate_competency_criterion_uses_a_supplied_group_id(tag: Tag, course_run: CourseRun) -> None: + """A caller-supplied group_id is used as-is, rather than deriving or creating a new leaf.""" + leaf = _create_leaf_group(tag, course_run) + object_id = usage_key(course_run, "p1") + + criterion = associate_competency_criterion(tag_id=tag.id, object_id=object_id, group_id=leaf.id) + + assert criterion.group_id == leaf.id + + +def test_associate_competency_criterion_rejects_group_id_and_logic_operator_together( + tag: Tag, course_run: CourseRun +) -> None: + """Supplying both group_id and logic_operator is rejected before anything is created.""" + leaf = _create_leaf_group(tag, course_run) + object_id = usage_key(course_run, "p1") + + with pytest.raises(ValidationError, match="logic_operator"): + associate_competency_criterion( + tag_id=tag.id, object_id=object_id, group_id=leaf.id, logic_operator=LogicOperator.AND, + ) + + +def test_associate_competency_criterion_rejects_a_duplicate_tag_object_association( + tag: Tag, course_run: CourseRun +) -> None: + """Via the derive-or-create path (no group_id), a second criterion for the same pair is rejected.""" + object_id = usage_key(course_run, "p1") + associate_competency_criterion(tag_id=tag.id, object_id=object_id) + + with pytest.raises(ValidationError, match="object_id"): + associate_competency_criterion(tag_id=tag.id, object_id=object_id) + + +def test_associate_competency_criterion_rejects_re_targeting_the_same_group( + tag: Tag, course_run: CourseRun +) -> None: + """Re-supplying the exact group a (tag_id, object_id) pair is already associated with is rejected.""" + object_id = usage_key(course_run, "p1") + first = associate_competency_criterion(tag_id=tag.id, object_id=object_id) + + with pytest.raises(ValidationError, match="group_id"): + associate_competency_criterion(tag_id=tag.id, object_id=object_id, group_id=first.group_id) + + +def test_associate_competency_criterion_allows_a_different_explicit_group_for_the_same_pair( + tag: Tag, course_run: CourseRun +) -> None: + """ + A different, explicitly supplied existing group creates a second, deliberate association -- + not a duplicate. ADR-0002's own worked example requires the same tag/object association to + be able to participate in more than one CompetencyCriteriaGroup. + """ + object_id = usage_key(course_run, "p1") + first = associate_competency_criterion(tag_id=tag.id, object_id=object_id) + other_leaf = _create_leaf_group(tag, course_run) + + second = associate_competency_criterion(tag_id=tag.id, object_id=object_id, group_id=other_leaf.id) + + assert second.object_tag_id == first.object_tag_id + assert second.group_id == other_leaf.id + assert CompetencyCriterion.objects.filter(object_tag_id=first.object_tag_id).count() == 2 + + +def test_associate_competency_criterion_raises_does_not_exist_for_an_unknown_tag_id(course_run: CourseRun) -> None: + """An unresolvable tag_id raises Tag.DoesNotExist before anything else is validated.""" + object_id = usage_key(course_run, "p1") + with pytest.raises(Tag.DoesNotExist): + associate_competency_criterion(tag_id=999999, object_id=object_id) + + +def test_associate_competency_criterion_rejects_a_malformed_object_id(tag: Tag) -> None: + """An object_id that isn't a parseable usage key is a 400, keyed by object_id.""" + with pytest.raises(ValidationError, match="object_id"): + associate_competency_criterion(tag_id=tag.id, object_id="not-a-usage-key") + + +def test_associate_competency_criterion_rejects_an_unresolvable_course(tag: Tag) -> None: + """A well-formed usage key whose course has no matching CourseRun is a 400, keyed by object_id.""" + object_id = "block-v1:NoOrg+NoCourse+NoRun+type@sequential+block@p1" + with pytest.raises(ValidationError, match="object_id"): + associate_competency_criterion(tag_id=tag.id, object_id=object_id) + + +def test_associate_competency_criterion_rolls_back_newly_created_groups_when_criterion_creation_fails( + monkeypatch: pytest.MonkeyPatch, tag: Tag, course_run: CourseRun +) -> None: + """ + A downstream failure during criterion creation rolls back any group this call created. + + ADR-0002 forbids persisting empty groups, so a failed attempt via the derive-or-create path + must not leave a root, course-level, or leaf group behind. Simulated here by making the final + CompetencyCriterion.objects.create() call itself fail -- standing in for any failure at that + point, including the containment check #666 will add right before it. + """ + def _reject(**_kwargs) -> None: + raise ValidationError({"object_id": "rejected for this test"}) + + monkeypatch.setattr(cbe_api.CompetencyCriterion.objects, "create", _reject) + object_id = usage_key(course_run, "p1") + + with pytest.raises(ValidationError): + associate_competency_criterion(tag_id=tag.id, object_id=object_id) + + assert not CompetencyCriteriaGroup.objects.filter(tag=tag).exists() diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index b6524edbd..f5c031f54 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -4,17 +4,83 @@ 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 + import pytest +import rules from django.urls import reverse +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import UsageKey from organizations.models import Organization from rest_framework import status from rest_framework.test import APIClient -from openedx_catalog.models import CourseRun -from openedx_learning.models import CompetencyRuleProfile, CompetencyTaxonomy, RuleType +from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_learning.applets.cbe.api import _create_leaf_group +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + LogicOperator, + RuleType, +) +from openedx_tagging.models import ObjectTag, Tag, Taxonomy pytestmark = pytest.mark.django_db +# A marker embedded in an object_id to signal "this caller has no object-level tagging +# permission", for the one test that needs a permission failure. oel_tagging.rules hardcodes +# change_objecttag_objectid to False ("should be defined in other apps for proper permission +# 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. +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.""" + def _predicate(_user, object_id: str) -> bool: + try: + UsageKey.from_string(object_id) + except InvalidKeyError: + return False + return UNAUTHORIZED_MARKER not in object_id + + perm = "oel_tagging.change_objecttag_objectid" + original = rules.permissions.permissions[perm] + rules.set_perm(perm, _predicate) + yield + rules.set_perm(perm, original) + + +@pytest.fixture(name="user_client") +def _user_client(api_client: APIClient, user) -> APIClient: + """A REST client acting as `user`, an ordinary, non-staff, authenticated caller.""" + api_client.force_authenticate(user=user) + return api_client + + +def criterion_create_url(tag_id: int) -> str: + """Return the create-criterion endpoint's path for `tag_id`, resolved through the URL name.""" + return reverse("cbe:criterion-create", kwargs={"tag_id": tag_id}) + + +def usage_key(course_run: CourseRun, block_id: str) -> str: + """Build a gradeable-subsection-shaped usage key string under `course_run`.""" + key = course_run.course_key + assert key is not None + return f"block-v1:{key.org}+{key.course}+{key.run}+type@sequential+block@{block_id}" + + +def make_course_run(organization: Organization, course_code: str, run_code: str) -> CourseRun: + """Create a CourseRun distinct from the `course_run` fixture, for the different-course tests.""" + catalog_course = CatalogCourse.objects.create(org=organization, course_code=course_code) + return CourseRun.objects.create(catalog_course=catalog_course, run_code=run_code) + + # What migration 0005 seeds the system default with. Asserted verbatim rather than imported, so # that a change to the seed surfaces here as a failing contract instead of passing silently. SEEDED_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} @@ -238,3 +304,503 @@ def test_any_signed_in_caller_may_read_the_collection( assert [row["id"] for row in response.data["results"]] == [default_rule_profile.id] else: assert "results" not in response.data + + +# ============================================================================================== +# CompetencyCriterionCreateView (#665) +# ============================================================================================== + + +def test_no_group_no_existing_groups_creates_the_full_hierarchy_and_201s( + user_client: APIClient, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """The happy path: no group_id, no groups yet, no rule fields supplied.""" + object_id = usage_key(course_run, "p1") + + response = user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + + assert response.status_code == status.HTTP_201_CREATED + criterion = CompetencyCriterion.objects.get(pk=response.data["id"]) + assert criterion.group.tag_id == tag.id + course_level = criterion.group.parent + assert course_level is not None + assert course_level.course_id == course_run.id + root = course_level.parent + assert root is not None + assert root.parent is None + assert response.data["object_tag_id"] == criterion.object_tag_id + assert response.data["group_id"] == criterion.group_id + assert response.data["rule_profile_id"] == default_rule_profile.id + + +def test_logic_operator_provided_is_stored_on_the_new_leaf( + user_client: APIClient, tag: Tag, course_run: CourseRun, +) -> None: + """A supplied logic_operator is stored on the newly created leaf, not defaulted.""" + object_id = usage_key(course_run, "p1") + + response = user_client.post( + criterion_create_url(tag.id), {"object_id": object_id, "logic_operator": "AND"}, format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + criterion = CompetencyCriterion.objects.get(pk=response.data["id"]) + assert criterion.group.logic_operator == LogicOperator.AND + + +def test_logic_operator_omitted_defaults_to_or(user_client: APIClient, tag: Tag, course_run: CourseRun) -> None: + """Omitting logic_operator on the derive-or-create path stores "OR" literally.""" + object_id = usage_key(course_run, "p1") + + response = user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + + assert response.status_code == status.HTTP_201_CREATED + criterion = CompetencyCriterion.objects.get(pk=response.data["id"]) + assert criterion.group.logic_operator == LogicOperator.OR + + +def test_group_id_and_logic_operator_together_is_rejected( + user_client: APIClient, tag: Tag, course_run: CourseRun, +) -> None: + """Supplying both group_id and logic_operator is a 400, before anything is created.""" + leaf = _create_leaf_group(tag, course_run) + object_id = usage_key(course_run, "p1") + + response = user_client.post( + criterion_create_url(tag.id), + {"object_id": object_id, "group_id": leaf.id, "logic_operator": "AND"}, + format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "logic_operator" in response.data + + +def test_course_level_group_is_reused_within_the_same_course( + user_client: APIClient, tag: Tag, course_run: CourseRun, +) -> None: + """Two criteria created for the same tag and course share one course-level group.""" + first = user_client.post( + criterion_create_url(tag.id), {"object_id": usage_key(course_run, "p1")}, format="json", + ) + second = user_client.post( + criterion_create_url(tag.id), {"object_id": usage_key(course_run, "p2")}, format="json", + ) + + assert first.status_code == status.HTTP_201_CREATED + assert second.status_code == status.HTTP_201_CREATED + first_group = CompetencyCriterion.objects.get(pk=first.data["id"]).group + second_group = CompetencyCriterion.objects.get(pk=second.data["id"]).group + assert first_group.id != second_group.id + assert first_group.parent_id == second_group.parent_id + + +def test_new_course_level_group_for_a_different_course( + user_client: APIClient, tag: Tag, course_run: CourseRun, organization: Organization, +) -> None: + """A second course under the same tag gets its own course-level group, but shares the root.""" + other_course_run = make_course_run(organization, "Python200", "Fall2026") + + first = user_client.post( + criterion_create_url(tag.id), {"object_id": usage_key(course_run, "p1")}, format="json", + ) + second = user_client.post( + criterion_create_url(tag.id), {"object_id": usage_key(other_course_run, "p1")}, format="json", + ) + + assert first.status_code == status.HTTP_201_CREATED + assert second.status_code == status.HTTP_201_CREATED + first_group = CompetencyCriterion.objects.get(pk=first.data["id"]).group + second_group = CompetencyCriterion.objects.get(pk=second.data["id"]).group + assert first_group.parent_id != second_group.parent_id + assert first_group.parent is not None + assert second_group.parent is not None + assert first_group.parent.parent_id == second_group.parent.parent_id + + +def test_supplied_leaf_group_happy_path(user_client: APIClient, tag: Tag, course_run: CourseRun) -> None: + """Supplying an existing, valid leaf group_id uses it directly.""" + leaf = _create_leaf_group(tag, course_run) + object_id = usage_key(course_run, "p1") + + response = user_client.post( + criterion_create_url(tag.id), {"object_id": object_id, "group_id": leaf.id}, format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + assert response.data["group_id"] == leaf.id + + +def test_supplied_group_non_leaf_is_rejected(user_client: APIClient, tag: Tag, course_run: CourseRun) -> None: + """A supplied group_id that names a root (non-leaf) group is a 400.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag, parent=None) + object_id = usage_key(course_run, "p1") + + response = user_client.post( + criterion_create_url(tag.id), {"object_id": object_id, "group_id": root.id}, format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "group_id" in response.data + + +def test_supplied_group_for_a_different_competency_is_rejected( + user_client: APIClient, tag: Tag, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """A supplied group_id that belongs to a different competency tag is a 400.""" + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Other Competency") + other_leaf = _create_leaf_group(other_tag, course_run) + object_id = usage_key(course_run, "p1") + + response = user_client.post( + criterion_create_url(tag.id), {"object_id": object_id, "group_id": other_leaf.id}, format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "group_id" in response.data + + +def test_supplied_group_for_a_different_course_is_rejected( + user_client: APIClient, tag: Tag, course_run: CourseRun, organization: Organization, +) -> None: + """A supplied group_id whose course-level parent belongs to a different course is a 400.""" + other_course_run = make_course_run(organization, "Python200", "Fall2026") + other_leaf = _create_leaf_group(tag, other_course_run) + object_id = usage_key(course_run, "p1") + + response = user_client.post( + criterion_create_url(tag.id), {"object_id": object_id, "group_id": other_leaf.id}, format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "group_id" in response.data + + +def test_duplicate_association_supplying_the_same_group_is_rejected( + user_client: APIClient, tag: Tag, course_run: CourseRun, +) -> None: + """A second criterion for the same (tag, object_id), re-supplying the group it just created, is a 400.""" + object_id = usage_key(course_run, "p1") + first = user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + first_group_id = first.data["group_id"] + + response = user_client.post( + criterion_create_url(tag.id), {"object_id": object_id, "group_id": first_group_id}, format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "group_id" in response.data + + +def test_duplicate_association_supplying_a_different_existing_leaf_creates_a_second_criterion( + user_client: APIClient, tag: Tag, course_run: CourseRun, +) -> None: + """ + A second criterion for the same (tag, object_id), explicitly aimed at a *different* existing + leaf, is a deliberate second association, not a duplicate -- per ADR-0002's worked example, + the same tag/object association may participate in more than one CompetencyCriteriaGroup. + Only re-targeting the exact same group already used is rejected (see the sibling test). + """ + object_id = usage_key(course_run, "p1") + first = user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + first_group_id = first.data["group_id"] + other_leaf = _create_leaf_group(tag, course_run) + groups_before = CompetencyCriteriaGroup.objects.count() + + response = user_client.post( + criterion_create_url(tag.id), {"object_id": object_id, "group_id": other_leaf.id}, format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + assert response.data["group_id"] == other_leaf.id + # No new groups: other_leaf already existed and was supplied explicitly. + assert CompetencyCriteriaGroup.objects.count() == groups_before + criterion_group_ids = set( + CompetencyCriterion.objects.filter(object_tag_id=response.data["object_tag_id"]) + .values_list("group_id", flat=True) + ) + assert criterion_group_ids == {first_group_id, other_leaf.id} + + +def test_duplicate_association_via_the_derive_path_is_rejected_before_creating_a_group( + user_client: APIClient, tag: Tag, course_run: CourseRun, +) -> None: + """ + A second criterion for the same (tag, object_id), with no group_id, is rejected before any new group. + + Rejected regardless of which group the existing criterion is actually in: with no group_id + supplied, there's no way to know which group slot this request intends, so any existing + criterion for the pair counts as a duplicate. + """ + object_id = usage_key(course_run, "p1") + user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + groups_before = CompetencyCriteriaGroup.objects.count() + + response = user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "object_id" in response.data + assert CompetencyCriteriaGroup.objects.count() == groups_before + + +def test_all_null_rule_fields_resolve_to_the_system_default_profile_id( + user_client: APIClient, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Omitting rule_profile_id, rule_type_override, and rule_payload_override together resolves + rule_profile_id to the seeded system-default profile's id, not null. + + CompetencyCriterion's own oel_cbe_criterion_profile_xor_override_check constraint never + allows all three fields null. + """ + object_id = usage_key(course_run, "p1") + + response = user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + + assert response.status_code == status.HTTP_201_CREATED + assert response.data["rule_profile_id"] == default_rule_profile.id + assert response.data["rule_type_override"] is None + assert response.data["rule_payload_override"] is None + + +def test_reusing_a_subsection_already_tagged_with_a_different_competency_preserves_both_tags( + user_client: APIClient, tag: Tag, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """ + Creating a criterion for a second competency on an already-tagged object merges, not overwrites. + + Both tags here come from the SAME CompetencyTaxonomy, per the plan's explicit note: only the + same-taxonomy case exercises tag_object()'s replace-the-whole-list behavior, since two + different taxonomies' ObjectTag rows would never collide even against a naive + overwrite-instead-of-merge implementation. + """ + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Other Competency") + object_id = usage_key(course_run, "p1") + + first = user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + second = user_client.post(criterion_create_url(other_tag.id), {"object_id": object_id}, format="json") + + assert first.status_code == status.HTTP_201_CREATED + assert second.status_code == status.HTTP_201_CREATED + applied_tag_ids = set( + ObjectTag.objects.filter(object_id=object_id, taxonomy=competency_taxonomy).values_list("tag_id", flat=True) + ) + assert applied_tag_ids == {tag.id, other_tag.id} + + +def test_competency_not_found_404s(user_client: APIClient, course_run: CourseRun) -> None: + """An unresolvable tag_id (URL path parameter) 404s.""" + object_id = usage_key(course_run, "p1") + + response = user_client.post(criterion_create_url(999999), {"object_id": object_id}, format="json") + + assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.data["detail"] == "Tag matching query does not exist." + + +def test_group_not_found_404s(user_client: APIClient, tag: Tag, course_run: CourseRun) -> None: + """An unresolvable group_id 404s.""" + object_id = usage_key(course_run, "p1") + + response = user_client.post( + criterion_create_url(tag.id), {"object_id": object_id, "group_id": 999999}, format="json", + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.data["detail"] == "CompetencyCriteriaGroup matching query does not exist." + + +def test_missing_object_id_is_400(user_client: APIClient, tag: Tag) -> None: + """A request with no object_id at all is a 400.""" + response = user_client.post(criterion_create_url(tag.id), {}, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "object_id" in response.data + + +def test_malformed_object_id_is_400(user_client: APIClient, tag: Tag) -> None: + """ + An object_id that isn't a parseable usage key is a 400, keyed by object_id. + + A 400 even though the permission predicate denies an unparseable key, so the body is + validated before permission is checked. + """ + response = user_client.post(criterion_create_url(tag.id), {"object_id": "not-a-usage-key"}, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "object_id" in response.data + + +def test_unresolvable_course_is_400(user_client: APIClient, tag: Tag) -> None: + """A well-formed usage key whose course has no matching CourseRun is a 400.""" + object_id = "block-v1:NoOrg+NoCourse+NoRun+type@sequential+block@p1" + + response = user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "object_id" in response.data + + +def test_no_permission_is_403(user_client: APIClient, tag: Tag, course_run: CourseRun) -> None: + """A caller without object-level tagging permission on this object_id is refused with a 403.""" + object_id = usage_key(course_run, UNAUTHORIZED_MARKER) + + response = user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_tag_on_a_plain_taxonomy_404s(user_client: APIClient, course_run: CourseRun) -> None: + """A tag_id naming a tag outside any CompetencyTaxonomy 404s.""" + plain_tag = Tag.objects.create(taxonomy=Taxonomy.objects.create(name="Plain Tags"), value="Plain") + + response = user_client.post( + criterion_create_url(plain_tag.id), {"object_id": usage_key(course_run, "p1")}, format="json", + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.data["detail"] == "Tag is not on a CompetencyTaxonomy." + + +def test_all_null_rule_fields_resolve_to_the_live_profile_for_the_tags_taxonomy( + user_client: APIClient, tag: Tag, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """A live profile scoped to the tag's own taxonomy wins over the system default.""" + taxonomy_scoped = make_profile(competency_taxonomy=competency_taxonomy) + + response = user_client.post( + criterion_create_url(tag.id), {"object_id": usage_key(course_run, "p1")}, format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + assert response.data["rule_profile_id"] == taxonomy_scoped.id + assert response.data["rule_type_override"] is None + assert response.data["rule_payload_override"] is None + + +def test_all_null_rule_fields_skip_an_archived_profile_for_the_tags_taxonomy( + user_client: APIClient, + tag: Tag, + course_run: CourseRun, + competency_taxonomy: CompetencyTaxonomy, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """An archived profile scoped to the tag's taxonomy is passed over for the system default.""" + make_profile(competency_taxonomy=competency_taxonomy, archived=True) + + response = user_client.post( + criterion_create_url(tag.id), {"object_id": usage_key(course_run, "p1")}, format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + assert response.data["rule_profile_id"] == default_rule_profile.id + + +def test_all_null_rule_fields_ignore_a_profile_for_a_different_taxonomy( + user_client: APIClient, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A live profile scoped to some other competency taxonomy does not apply to this tag.""" + make_profile(competency_taxonomy=CompetencyTaxonomy.objects.create(name="Welding", export_id="welding-v1")) + + response = user_client.post( + criterion_create_url(tag.id), {"object_id": usage_key(course_run, "p1")}, format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + assert response.data["rule_profile_id"] == default_rule_profile.id + + +@pytest.mark.parametrize( + "rule_fields, rejected_field", + [ + ({"rule_type_override": RuleType.GRADE}, "rule_type_override"), + ({"rule_payload_override": FIXTURE_GRADE_PAYLOAD}, "rule_payload_override"), + ({"use_live_profile": True, "rule_type_override": RuleType.GRADE}, "rule_profile_id"), + ({"use_live_profile": True, "rule_payload_override": FIXTURE_GRADE_PAYLOAD}, "rule_profile_id"), + ( + { + "use_live_profile": True, + "rule_type_override": RuleType.GRADE, + "rule_payload_override": FIXTURE_GRADE_PAYLOAD, + }, + "rule_profile_id", + ), + ], +) +def test_invalid_rule_field_combination_is_400_and_creates_nothing( + user_client: APIClient, + tag: Tag, + course_run: CourseRun, + rule_fields: dict, + rejected_field: str, +) -> None: + """A profile alongside any override, or one override without the other, is a 400 keyed by the offending field.""" + body = {"object_id": usage_key(course_run, "p1"), **rule_fields} + if body.pop("use_live_profile", False): + body["rule_profile_id"] = make_profile(competency_taxonomy_id=tag.taxonomy_id).id + + response = user_client.post(criterion_create_url(tag.id), body, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert rejected_field in response.data + assert not CompetencyCriterion.objects.exists() + assert not CompetencyCriteriaGroup.objects.exists() + + +@pytest.mark.parametrize("archived", [True, None], ids=["archived", "unknown"]) +def test_unusable_rule_profile_id_is_400_and_creates_nothing( + user_client: APIClient, + tag: Tag, + course_run: CourseRun, + archived: bool | None, +) -> None: + """A rule_profile_id naming an archived or nonexistent profile is a 400 keyed by rule_profile_id.""" + rule_profile_id = make_profile(competency_taxonomy_id=tag.taxonomy_id, archived=True).id if archived else 999999 + + response = user_client.post( + criterion_create_url(tag.id), + {"object_id": usage_key(course_run, "p1"), "rule_profile_id": rule_profile_id}, + format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "rule_profile_id" in response.data + assert not CompetencyCriterion.objects.exists() + assert not CompetencyCriteriaGroup.objects.exists() + + +def test_tag_refused_by_a_single_tag_taxonomy_is_400_and_creates_nothing( + user_client: APIClient, tag: Tag, course_run: CourseRun, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """A second competency on an object in a taxonomy allowing one tag per object is a 400 keyed by object_id.""" + competency_taxonomy.allow_multiple = False + competency_taxonomy.save() + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Other Competency") + object_id = usage_key(course_run, "p1") + ObjectTag.objects.create(object_id=object_id, taxonomy=competency_taxonomy, tag=other_tag) + + response = user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "object_id" in response.data + assert not CompetencyCriterion.objects.exists() + assert not CompetencyCriteriaGroup.objects.exists() + + +def test_tag_refused_at_the_object_tag_limit_is_400_and_creates_nothing( + monkeypatch: pytest.MonkeyPatch, user_client: APIClient, tag: Tag, course_run: CourseRun, +) -> None: + """An object already at the tag limit is a 400 keyed by object_id.""" + # Lowered so one existing tag reaches the limit, instead of creating 100 rows. + monkeypatch.setattr("openedx_tagging.api.OBJECT_MAX_TAGS", 1) + other_taxonomy = Taxonomy.objects.create(name="Other Taxonomy") + other_tag = Tag.objects.create(taxonomy=other_taxonomy, value="Other Tag") + object_id = usage_key(course_run, "p1") + ObjectTag.objects.create(object_id=object_id, taxonomy=other_taxonomy, tag=other_tag) + + response = user_client.post(criterion_create_url(tag.id), {"object_id": object_id}, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "Cannot add more than 1 tags" in str(response.data["object_id"]) + assert not CompetencyCriterion.objects.exists() + assert not CompetencyCriteriaGroup.objects.exists() diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index a2a8a8e2a..7c9931925 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -67,7 +67,9 @@ def test_create_taxonomy_without_export_id(self) -> None: def test_get_taxonomy(self) -> None: tax1 = tagging_api.get_taxonomy(1) assert tax1 == self.taxonomy - no_tax = tagging_api.get_taxonomy(200) + # Not a small id like 200: MySQL doesn't reset auto-increment between tests, so taxonomies + # created earlier (e.g. self.free_text_taxonomy in TestTagTaxonomyMixin.setUp()) can take it. + no_tax = tagging_api.get_taxonomy(2**31 - 1) assert no_tax is None def test_get_taxonomy_by_export_id(self) -> None: