From be22ac6d7208b52f594eb39929c84ebe7538c4aa Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 15 Sep 2026 11:55:18 +0500 Subject: [PATCH 01/17] feat: mark the v1 cohorts API superseded by v2 Applies ADR 0037 to the cohorts API. The v1 cohort, cohort-users and cohort-settings views now emit Deprecation and Link response headers naming their v2 successor, so remaining callers can be identified from access logs before the v1 routes are retired. v1 behaviour is otherwise unchanged; the v2 surface is added in the commits that follow. --- .../course_groups/rest_api/deprecation.py | 24 +++++++++++++++++++ .../core/djangoapps/course_groups/views.py | 10 +++++--- 2 files changed, 31 insertions(+), 3 deletions(-) create mode 100644 openedx/core/djangoapps/course_groups/rest_api/deprecation.py diff --git a/openedx/core/djangoapps/course_groups/rest_api/deprecation.py b/openedx/core/djangoapps/course_groups/rest_api/deprecation.py new file mode 100644 index 000000000000..584eb419fb39 --- /dev/null +++ b/openedx/core/djangoapps/course_groups/rest_api/deprecation.py @@ -0,0 +1,24 @@ +""" +Helpers for marking superseded cohort API versions. +""" + + +class DeprecatedAPIViewMixin: + """ + Advertises a view as superseded by a newer version of the same resource. + + Adds the ``Deprecation`` and ``Link`` response headers described in RFC 8594 + so that remaining callers can be identified from access logs before the + superseded routes are removed. + + Subclasses set ``successor_path`` to the path that replaces this one. + """ + + successor_path = None + + def finalize_response(self, request, response, *args, **kwargs): + response = super().finalize_response(request, response, *args, **kwargs) + response["Deprecation"] = "true" + if self.successor_path: + response["Link"] = f'<{self.successor_path}>; rel="successor-version"' + return response diff --git a/openedx/core/djangoapps/course_groups/views.py b/openedx/core/djangoapps/course_groups/views.py index 4f42516fda14..224564b93e2c 100644 --- a/openedx/core/djangoapps/course_groups/views.py +++ b/openedx/core/djangoapps/course_groups/views.py @@ -34,6 +34,7 @@ CohortMembership, ) from openedx.core.djangoapps.course_groups.permissions import IsStaffOrAdmin +from openedx.core.djangoapps.course_groups.rest_api.deprecation import DeprecatedAPIViewMixin from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin @@ -482,7 +483,7 @@ class APIPermissions(GenericAPIView): serializer_class = Serializer -class CohortSettings(DeveloperErrorViewMixin, APIPermissions): +class CohortSettings(DeprecatedAPIViewMixin, DeveloperErrorViewMixin, APIPermissions): """ **Use Cases** @@ -498,6 +499,7 @@ class CohortSettings(DeveloperErrorViewMixin, APIPermissions): * is_cohorted: current status of the cohort setting """ + successor_path = '/api/cohorts/v2/courses/{course_key}/cohort_settings/' def get(self, request, course_key_string): """ @@ -522,7 +524,7 @@ def put(self, request, course_key_string): return _get_cohort_settings_response(course_key) -class CohortHandler(DeveloperErrorViewMixin, APIPermissions): +class CohortHandler(DeprecatedAPIViewMixin, DeveloperErrorViewMixin, APIPermissions): """ **Use Cases** @@ -574,6 +576,7 @@ class CohortHandler(DeveloperErrorViewMixin, APIPermissions): * user_partition_id: The integer identifier of the UserPartition. * group_id: The integer identifier of the specific group in the partition. """ + successor_path = '/api/cohorts/v2/courses/{course_key}/cohorts/' queryset = [] def get(self, request, course_key_string, cohort_id=None): @@ -672,7 +675,7 @@ def patch(self, request, course_key_string, cohort_id=None): return Response(status=status.HTTP_204_NO_CONTENT) -class CohortUsers(DeveloperErrorViewMixin, APIPermissions): +class CohortUsers(DeprecatedAPIViewMixin, DeveloperErrorViewMixin, APIPermissions): """ **Use Cases** List users in a cohort @@ -759,6 +762,7 @@ class CohortUsers(DeveloperErrorViewMixin, APIPermissions): * The user corresponding to the given username could not be found. Returns a HTTP 204 No Content response status code to indicate success. """ + successor_path = '/api/cohorts/v2/courses/{course_key}/cohorts/{cohort_id}/users/' serializer_class = CohortUsersAPISerializer def _get_course_and_cohort(self, request, course_key_string, cohort_id): From 84f18ab129285101b445c987eb9d3851bf87b03b Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 15 Sep 2026 11:58:26 +0500 Subject: [PATCH 02/17] feat: add cohort viewsets for the v2 API Applies ADR 0028 to the cohorts API. The three v1 surfaces (cohort list/detail, cohort membership and course cohort settings) become CohortViewSet, CohortMemberViewSet and CohortSettingsView, with the shared course-key resolution and cohort lookup moved into a mixin instead of being repeated per handler. Explicit serializers replace the hand-built dictionaries, so request payloads are validated rather than read straight off request.data. Not routed yet; authentication and permissions are added before the URLs are registered. --- .../rest_api/cohort_serializers.py | 131 +++++++++ .../course_groups/rest_api/cohort_views.py | 251 ++++++++++++++++++ 2 files changed, 382 insertions(+) create mode 100644 openedx/core/djangoapps/course_groups/rest_api/cohort_serializers.py create mode 100644 openedx/core/djangoapps/course_groups/rest_api/cohort_views.py diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_serializers.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_serializers.py new file mode 100644 index 000000000000..a2ea097a300a --- /dev/null +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_serializers.py @@ -0,0 +1,131 @@ +""" +Serializers for the v2 cohorts REST API. +""" +from django.contrib.auth import get_user_model +from rest_framework import serializers + +from openedx.core.djangoapps.course_groups import cohorts + +User = get_user_model() + +ASSIGNMENT_TYPE_CHOICES = ("manual", "random") + + +class CohortSerializer(serializers.Serializer): + """ + Representation of a single cohort within a course. + + ``CourseUserGroup`` carries the name and membership, while the assignment + type and content-group association live on related models, so the fields + are declared explicitly rather than derived from one model. + """ + + id = serializers.IntegerField(read_only=True, help_text="Identifier of the cohort.") + name = serializers.CharField(max_length=255, help_text="Display name of the cohort.") + assignment_type = serializers.ChoiceField( + choices=ASSIGNMENT_TYPE_CHOICES, + help_text="How learners are placed into this cohort.", + ) + user_count = serializers.IntegerField( + read_only=True, + help_text="Number of actively enrolled learners in this cohort.", + ) + user_partition_id = serializers.IntegerField( + required=False, + allow_null=True, + help_text="Identifier of the content group configuration this cohort is associated with.", + ) + group_id = serializers.IntegerField( + required=False, + allow_null=True, + help_text="Identifier of the content group within the configuration. Send null to unlink.", + ) + + +class CohortUpdateSerializer(CohortSerializer): + """ + Update payload for a cohort, where every field is optional. + + ``group_id`` is nullable and meaningful when explicitly set to null, which + unlinks the content group association. + """ + + name = serializers.CharField(max_length=255, required=False) + assignment_type = serializers.ChoiceField(choices=ASSIGNMENT_TYPE_CHOICES, required=False) + + def validate(self, attrs): + if not attrs: + raise serializers.ValidationError( + "Supply at least one of name, assignment_type or group_id." + ) + return attrs + + +class CohortMemberSerializer(serializers.ModelSerializer): + """ + Representation of a learner who belongs to a cohort. + """ + + name = serializers.SerializerMethodField() + + class Meta: + model = User + fields = ("username", "email", "name") + + def get_name(self, user): + """Return the learner's full name.""" + return f"{user.first_name} {user.last_name}".strip() + + +class CohortMembershipRequestSerializer(serializers.Serializer): + """ + Request payload for adding learners to a cohort. + """ + + users = serializers.ListField( + child=serializers.CharField(allow_blank=False), + allow_empty=False, + help_text="Usernames or email addresses to place in this cohort.", + ) + + +class CohortMembershipResultSerializer(serializers.Serializer): + """ + Outcome of an add-learners request, partitioned by what happened to each entry. + """ + + added = serializers.ListField(child=serializers.DictField()) + changed = serializers.ListField(child=serializers.DictField()) + present = serializers.ListField(child=serializers.CharField()) + unknown = serializers.ListField(child=serializers.CharField()) + preassigned = serializers.ListField(child=serializers.CharField()) + invalid = serializers.ListField(child=serializers.CharField()) + + +class CohortSettingsSerializer(serializers.Serializer): + """ + Course-level cohort configuration. + """ + + id = serializers.IntegerField(read_only=True, allow_null=True) + is_cohorted = serializers.BooleanField( + help_text="Whether cohorts are enabled for this course.", + ) + + +def represent_cohort(cohort, course_key): + """ + Build the serializable representation of a cohort. + """ + group_id, partition_id = cohorts.get_group_info_for_cohort(cohort) + return { + "id": cohort.id, + "name": cohort.name, + "assignment_type": cohorts.get_assignment_type(cohort), + "user_count": cohort.users.filter( + courseenrollment__course_id=course_key, + courseenrollment__is_active=1, + ).count(), + "user_partition_id": partition_id, + "group_id": group_id, + } diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py new file mode 100644 index 000000000000..236b431702a5 --- /dev/null +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py @@ -0,0 +1,251 @@ +""" +REST API views for course cohorts, v2. + +Consolidates the three v1 function-and-APIView surfaces (cohort list/detail, +cohort membership and course cohort settings) into DRF ViewSets registered +through a router. +""" +from django.contrib.auth import get_user_model +from django.core.exceptions import ValidationError as DjangoValidationError +from opaque_keys.edx.keys import CourseKey +from rest_framework import status, viewsets +from rest_framework.response import Response +from rest_framework.views import APIView + +from openedx.core.djangoapps.course_groups import api as cohort_api +from openedx.core.djangoapps.course_groups import cohorts +from openedx.core.djangoapps.course_groups.models import ( + CohortMembership, + CourseUserGroup, + CourseUserGroupPartitionGroup, +) +from openedx.core.djangoapps.course_groups.rest_api.cohort_serializers import ( + CohortMembershipRequestSerializer, + CohortMemberSerializer, + CohortSerializer, + CohortSettingsSerializer, + CohortUpdateSerializer, + represent_cohort, +) +from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin + +User = get_user_model() + + +class CourseScopedMixin: + """ + Resolves the course key that every cohort resource is addressed under. + """ + + @property + def course_key(self): + """Return the course key taken from the request path.""" + return CourseKey.from_string(self.kwargs["course_key_string"]) + + def get_cohort_or_404(self): + """Return the cohort named in the path, or raise a 404.""" + try: + return cohorts.get_cohort_by_id(self.course_key, self.kwargs["cohort_id"]) + except CourseUserGroup.DoesNotExist as exc: + raise self.api_error( + status.HTTP_404_NOT_FOUND, + f"No cohort {self.kwargs['cohort_id']} in {self.kwargs['course_key_string']}.", + "cohort-not-found", + ) from exc + + +class CohortViewSet(CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): + """ + List, create, read and update the cohorts of a course. + """ + + serializer_class = CohortSerializer + lookup_url_kwarg = "cohort_id" + + def list(self, request, course_key_string): + """Return the cohorts of this course.""" + course_key = self.course_key + records = cohorts.get_course_cohorts(course_id=course_key) + return Response([represent_cohort(c, course_key) for c in records]) + + def retrieve(self, request, course_key_string, cohort_id): + """Return a single cohort.""" + cohort = self.get_cohort_or_404() + return Response(represent_cohort(cohort, self.course_key)) + + def create(self, request, course_key_string): + """Create a cohort in this course.""" + serializer = CohortSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + data = serializer.validated_data + course_key = self.course_key + + if cohorts.is_cohort_exists(course_key, data["name"]): + raise self.api_error( + status.HTTP_400_BAD_REQUEST, + "A cohort with that name already exists in this course.", + "cohort-name-exists", + ) + + cohort = cohorts.add_cohort(course_key, data["name"], data["assignment_type"]) + if data.get("group_id") is not None or data.get("user_partition_id") is not None: + self._apply_content_group(cohort, data) + return Response(represent_cohort(cohort, course_key), status=status.HTTP_201_CREATED) + + def partial_update(self, request, course_key_string, cohort_id): + """Update a cohort's name, assignment type or content group.""" + serializer = CohortUpdateSerializer(data=request.data, partial=True) + serializer.is_valid(raise_exception=True) + data = serializer.validated_data + course_key = self.course_key + cohort = self.get_cohort_or_404() + + name = data.get("name") + if name is not None and name != cohort.name: + if cohorts.is_cohort_exists(course_key, name): + raise self.api_error( + status.HTTP_400_BAD_REQUEST, + "A cohort with that name already exists in this course.", + "cohort-name-exists", + ) + cohort.name = name + cohort.save() + + if data.get("assignment_type") is not None: + try: + cohorts.set_assignment_type(cohort, data["assignment_type"]) + except ValueError as exc: + raise self.api_error( + status.HTTP_400_BAD_REQUEST, str(exc), "last-random-cohort" + ) from exc + + if "group_id" in request.data: + self._apply_content_group(cohort, data) + + return Response(represent_cohort(cohort, course_key)) + + def _apply_content_group(self, cohort, data): + """ + Link or unlink the cohort's content group association. + + A ``group_id`` of None unlinks any existing association, which is + distinct from the caller omitting the field entirely. + """ + group_id = data.get("group_id") + partition_id = data.get("user_partition_id") + existing_group_id, existing_partition_id = cohorts.get_group_info_for_cohort(cohort) + + if group_id is None: + if existing_group_id is not None: + CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort).delete() + return + + if partition_id is None: + raise self.api_error( + status.HTTP_400_BAD_REQUEST, + "user_partition_id is required when group_id is supplied.", + "missing-user-partition-id", + ) + if group_id != existing_group_id or partition_id != existing_partition_id: + CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort).delete() + CourseUserGroupPartitionGroup( + course_user_group=cohort, + partition_id=partition_id, + group_id=group_id, + ).save() + + +class CohortMemberViewSet(CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): + """ + List the learners in a cohort, add learners to it and remove one from it. + """ + + serializer_class = CohortMemberSerializer + lookup_url_kwarg = "username" + lookup_value_regex = r"[\w.@+-]+" + + def list(self, request, course_key_string, cohort_id): + """Return the learners in this cohort.""" + cohort = self.get_cohort_or_404() + return Response(CohortMemberSerializer(cohort.users.all(), many=True).data) + + def create(self, request, course_key_string, cohort_id): + """Add the named learners to this cohort.""" + cohort = self.get_cohort_or_404() + serializer = CohortMembershipRequestSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + + result = {key: [] for key in + ("added", "changed", "present", "unknown", "preassigned", "invalid")} + for identifier in serializer.validated_data["users"]: + try: + user, previous_cohort, preassigned = cohorts.add_user_to_cohort(cohort, identifier) + except User.DoesNotExist: + result["unknown"].append(identifier) + except DjangoValidationError: + result["invalid"].append(identifier) + except ValueError: + result["present"].append(identifier) + else: + if preassigned: + result["preassigned"].append(identifier) + elif previous_cohort: + result["changed"].append({ + "username": user.username, + "email": user.email, + "previous_cohort": previous_cohort, + }) + else: + result["added"].append({"username": user.username, "email": user.email}) + return Response(result) + + def destroy(self, request, course_key_string, cohort_id, username): + """Remove one learner from this cohort.""" + cohort = self.get_cohort_or_404() + try: + cohort_api.remove_user_from_cohort(self.course_key, username, cohort.id) + except User.DoesNotExist as exc: + raise self.api_error( + status.HTTP_404_NOT_FOUND, f"No user named {username}.", "user-not-found" + ) from exc + except CohortMembership.DoesNotExist as exc: + raise self.api_error( + status.HTTP_400_BAD_REQUEST, + f"{username} is not a member of this cohort.", + "user-not-in-cohort", + ) from exc + return Response(status=status.HTTP_204_NO_CONTENT) + + +class CohortSettingsView(DeveloperErrorViewMixin, APIView): + """ + Read and update whether a course uses cohorts. + """ + + serializer_class = CohortSettingsSerializer + + def get(self, request, course_key_string): + """Return the course's cohort configuration.""" + course_key = CourseKey.from_string(course_key_string) + return Response(self._representation(course_key)) + + def put(self, request, course_key_string): + """Enable or disable cohorts for the course.""" + course_key = CourseKey.from_string(course_key_string) + serializer = CohortSettingsSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + try: + cohorts.set_course_cohorted(course_key, serializer.validated_data["is_cohorted"]) + except ValueError as exc: + raise self.api_error( + status.HTTP_400_BAD_REQUEST, str(exc), "invalid-cohort-setting" + ) from exc + return Response(self._representation(course_key)) + + @staticmethod + def _representation(course_key): + """Build the cohort settings payload for a course.""" + return { + "id": cohorts.get_course_cohort_id(course_key), + "is_cohorted": cohorts.is_course_cohorted(course_key), + } From fc264e03531690493cd9b05695053afd4d2d532c Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 15 Sep 2026 11:58:52 +0500 Subject: [PATCH 03/17] feat: authenticate the v2 cohort endpoints with JWT and session only Applies ADR 0034 to the v2 cohorts API. The v1 views authenticate with JwtAuthentication, BearerAuthenticationAllowInactiveUser and SessionAuthenticationAllowInactiveUser; the deprecated Bearer class is left out of v2 entirely. SessionAuthenticationAllowInactiveUser is kept rather than dropped to the platform default, because v1 accepts inactive users over session and switching to the default session class would block callers that work today. --- .../course_groups/rest_api/cohort_views.py | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py index 236b431702a5..b5dbbee6c57c 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py @@ -7,6 +7,10 @@ """ from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError as DjangoValidationError +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.session.authentication import ( + SessionAuthenticationAllowInactiveUser, +) from opaque_keys.edx.keys import CourseKey from rest_framework import status, viewsets from rest_framework.response import Response @@ -32,6 +36,23 @@ User = get_user_model() +class CohortAPIAuthMixin: + """ + Authentication shared by every v2 cohort endpoint. + + JWT is the standard scheme for user-authenticated requests. Session + authentication keeps the Studio and instructor dashboard front ends + working, and the inactive-user variant is retained deliberately: the v1 + surface accepted inactive users over session, and narrowing that here + would lock out callers that work today. + """ + + authentication_classes = ( + JwtAuthentication, + SessionAuthenticationAllowInactiveUser, + ) + + class CourseScopedMixin: """ Resolves the course key that every cohort resource is addressed under. @@ -54,7 +75,7 @@ def get_cohort_or_404(self): ) from exc -class CohortViewSet(CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): +class CohortViewSet(CohortAPIAuthMixin, CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): """ List, create, read and update the cohorts of a course. """ @@ -155,7 +176,7 @@ def _apply_content_group(self, cohort, data): ).save() -class CohortMemberViewSet(CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): +class CohortMemberViewSet(CohortAPIAuthMixin, CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): """ List the learners in a cohort, add learners to it and remove one from it. """ @@ -217,7 +238,7 @@ def destroy(self, request, course_key_string, cohort_id, username): return Response(status=status.HTTP_204_NO_CONTENT) -class CohortSettingsView(DeveloperErrorViewMixin, APIView): +class CohortSettingsView(CohortAPIAuthMixin, DeveloperErrorViewMixin, APIView): """ Read and update whether a course uses cohorts. """ From c53b1851daabd4162d0e18447d3b1d51563a15ac Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 15 Sep 2026 11:59:48 +0500 Subject: [PATCH 04/17] feat: enforce cohort authorization through a permission class Applies ADR 0026 to the v2 cohorts API. Authorization is declared once as permission_classes on the shared access mixin instead of being repeated as a get_course_with_access call inside every handler, which is what the v1 views do. CanManageCohorts keeps the v1 access rules: course staff and instructors read and write, discussion moderators read only, global staff and superusers do either. It denies an unparseable course key rather than raising, and a malformed key is rejected before the permission check so that a 404 cannot be used to probe for courses. --- .../rest_api/cohort_permissions.py | 53 +++++++++++++++++++ .../course_groups/rest_api/cohort_views.py | 39 +++++++++++--- 2 files changed, 86 insertions(+), 6 deletions(-) create mode 100644 openedx/core/djangoapps/course_groups/rest_api/cohort_permissions.py diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_permissions.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_permissions.py new file mode 100644 index 000000000000..6e23456f4f37 --- /dev/null +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_permissions.py @@ -0,0 +1,53 @@ +""" +Permissions for the v2 cohorts REST API. +""" +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from rest_framework import permissions + +from common.djangoapps.student.roles import GlobalStaff +from lms.djangoapps.courseware.access import administrative_accesses_to_course_for_user +from lms.djangoapps.discussion.django_comment_client.utils import get_user_role_names +from openedx.core.djangoapps.django_comment_common.models import ( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_MODERATOR, +) + +DISCUSSION_PRIVILEGED_ROLES = { + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, +} + + +class CanManageCohorts(permissions.BasePermission): + """ + Grants access to the cohort endpoints of a course. + + Course staff and instructors may read and write. Discussion moderators, + administrators and community TAs may read only. Global staff and + superusers may do either. + + A course key that cannot be parsed is denied rather than raising, so a + malformed path cannot surface as a server error. + """ + + def has_permission(self, request, view): + user = request.user + if not user or not user.is_authenticated: + return False + if GlobalStaff().has_user(user) or user.is_superuser: + return True + + try: + course_key = CourseKey.from_string(view.kwargs.get("course_key_string", "")) + except InvalidKeyError: + return False + + _, staff_access, instructor_access = administrative_accesses_to_course_for_user(user, course_key) + if staff_access or instructor_access: + return True + + moderates_discussions = bool(get_user_role_names(user, course_key) & DISCUSSION_PRIVILEGED_ROLES) + return moderates_discussions and request.method in permissions.SAFE_METHODS diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py index b5dbbee6c57c..be958d668b87 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py @@ -11,8 +11,9 @@ from edx_rest_framework_extensions.auth.session.authentication import ( SessionAuthenticationAllowInactiveUser, ) +from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey -from rest_framework import status, viewsets +from rest_framework import permissions, status, viewsets from rest_framework.response import Response from rest_framework.views import APIView @@ -23,6 +24,7 @@ CourseUserGroup, CourseUserGroupPartitionGroup, ) +from openedx.core.djangoapps.course_groups.rest_api.cohort_permissions import CanManageCohorts from openedx.core.djangoapps.course_groups.rest_api.cohort_serializers import ( CohortMembershipRequestSerializer, CohortMemberSerializer, @@ -36,21 +38,46 @@ User = get_user_model() -class CohortAPIAuthMixin: +class CohortAPIAccessMixin: """ - Authentication shared by every v2 cohort endpoint. + Authentication and authorization shared by every v2 cohort endpoint. JWT is the standard scheme for user-authenticated requests. Session authentication keeps the Studio and instructor dashboard front ends working, and the inactive-user variant is retained deliberately: the v1 surface accepted inactive users over session, and narrowing that here would lock out callers that work today. + + Authorization lives entirely in the permission class. The handlers do not + repeat it, which is what the v1 views did by calling get_course_with_access + again inside each method. """ authentication_classes = ( JwtAuthentication, SessionAuthenticationAllowInactiveUser, ) + permission_classes = (permissions.IsAuthenticated, CanManageCohorts) + + def initial(self, request, *args, **kwargs): + """ + Reject a malformed course key before the permission check runs. + + The key is validated for syntax only. Whether the course exists is + checked inside the handlers, after authorization, so that a 404 cannot + be used to probe for courses. + """ + course_key_string = kwargs.get("course_key_string") + if course_key_string is not None: + try: + CourseKey.from_string(course_key_string) + except InvalidKeyError as exc: + raise self.api_error( + status.HTTP_404_NOT_FOUND, + f"{course_key_string} is not a valid course key.", + "invalid-course-key", + ) from exc + super().initial(request, *args, **kwargs) class CourseScopedMixin: @@ -75,7 +102,7 @@ def get_cohort_or_404(self): ) from exc -class CohortViewSet(CohortAPIAuthMixin, CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): +class CohortViewSet(CohortAPIAccessMixin, CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): """ List, create, read and update the cohorts of a course. """ @@ -176,7 +203,7 @@ def _apply_content_group(self, cohort, data): ).save() -class CohortMemberViewSet(CohortAPIAuthMixin, CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): +class CohortMemberViewSet(CohortAPIAccessMixin, CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): """ List the learners in a cohort, add learners to it and remove one from it. """ @@ -238,7 +265,7 @@ def destroy(self, request, course_key_string, cohort_id, username): return Response(status=status.HTTP_204_NO_CONTENT) -class CohortSettingsView(CohortAPIAuthMixin, DeveloperErrorViewMixin, APIView): +class CohortSettingsView(CohortAPIAccessMixin, DeveloperErrorViewMixin, APIView): """ Read and update whether a course uses cohorts. """ From 89ad3dc3502b0f1edf67a7c749b299a4d394d4a2 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 15 Sep 2026 12:00:39 +0500 Subject: [PATCH 05/17] feat: route the v2 cohort endpoints Applies ADR 0038 to the v2 cohorts API. Each route resolves exactly one address: the trailing slash is required rather than optional, list and detail are separate patterns instead of one pattern with an optional identifier, and the username segment is bounded so it cannot swallow path separators. This avoids three defects carried by the v1 patterns, where 'users?' matches both /user and /users, '(?P[0-9]+)?' serves both the collection and a member from one route, and '(?P.+)?' captures slashes so /users/bob/extra yields the username 'bob/extra'. The settings route is named course_cohort_settings because the v1 route already holds cohort_settings in this namespace. --- .../djangoapps/course_groups/rest_api/urls.py | 33 +++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/course_groups/rest_api/urls.py b/openedx/core/djangoapps/course_groups/rest_api/urls.py index cb9e277c71c2..9de163859e4b 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/urls.py +++ b/openedx/core/djangoapps/course_groups/rest_api/urls.py @@ -1,10 +1,10 @@ """ -Content Groups REST API v2 URLs +Cohorts and content groups REST API v2 URLs. """ from django.urls import re_path from openedx.core.constants import COURSE_ID_PATTERN -from openedx.core.djangoapps.course_groups.rest_api import views +from openedx.core.djangoapps.course_groups.rest_api import cohort_views, views urlpatterns = [ re_path( @@ -17,4 +17,33 @@ views.GroupConfigurationDetailView.as_view(), name='group_configurations_detail' ), + + # Cohorts. Every route carries a required trailing slash and resolves a + # single address; the list and detail routes are registered separately + # rather than sharing one pattern with an optional identifier. + re_path( + fr'^v2/courses/{COURSE_ID_PATTERN}/cohorts/$', + cohort_views.CohortViewSet.as_view({'get': 'list', 'post': 'create'}), + name='cohort_list', + ), + re_path( + fr'^v2/courses/{COURSE_ID_PATTERN}/cohorts/(?P[0-9]+)/$', + cohort_views.CohortViewSet.as_view({'get': 'retrieve', 'patch': 'partial_update'}), + name='cohort_detail', + ), + re_path( + fr'^v2/courses/{COURSE_ID_PATTERN}/cohorts/(?P[0-9]+)/users/$', + cohort_views.CohortMemberViewSet.as_view({'get': 'list', 'post': 'create'}), + name='cohort_member_list', + ), + re_path( + fr'^v2/courses/{COURSE_ID_PATTERN}/cohorts/(?P[0-9]+)/users/(?P[\w.@+-]+)/$', + cohort_views.CohortMemberViewSet.as_view({'delete': 'destroy'}), + name='cohort_member_detail', + ), + re_path( + fr'^v2/courses/{COURSE_ID_PATTERN}/cohort_settings/$', + cohort_views.CohortSettingsView.as_view(), + name='course_cohort_settings', + ), ] From 6fffa80a82e0cdc83cd7e86bfbe6bd49080dedae Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 15 Sep 2026 12:01:27 +0500 Subject: [PATCH 06/17] feat: keep the v2 cohort read paths free of writes Applies ADR 0030 to the v2 cohorts API. Listing cohorts goes through get_course_cohorts(course_id=...) rather than passing the course object, because the course branch of that helper calls migrate_cohort_settings(), which creates a CourseCohortsSettings row and backfills CourseCohort rows. The v1 list endpoint takes that branch, so a GET there writes to the database. Reading cohort settings no longer calls is_course_cohorted(), which lazily migrates and persists settings for the same reason. The stored row is used when present; otherwise the authored value is reported and persisting it is left to a write request. Course existence is still checked, so listing cohorts for a course that does not exist is a 404 rather than an empty collection. --- .../course_groups/rest_api/cohort_views.py | 29 +++++++++++++++++-- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py index be958d668b87..cafe79a8962b 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py @@ -19,8 +19,10 @@ from openedx.core.djangoapps.course_groups import api as cohort_api from openedx.core.djangoapps.course_groups import cohorts +from openedx.core.djangoapps.course_groups.cohorts import get_legacy_discussion_settings from openedx.core.djangoapps.course_groups.models import ( CohortMembership, + CourseCohortsSettings, CourseUserGroup, CourseUserGroupPartitionGroup, ) @@ -34,6 +36,7 @@ represent_cohort, ) from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin +from openedx.core.lib.courses import get_course_by_id User = get_user_model() @@ -90,6 +93,15 @@ def course_key(self): """Return the course key taken from the request path.""" return CourseKey.from_string(self.kwargs["course_key_string"]) + def ensure_course_exists(self): + """ + Raise a 404 when the addressed course does not exist. + + Listing cohorts by course key alone would otherwise return an empty + collection for a course that is not there. + """ + get_course_by_id(self.course_key) + def get_cohort_or_404(self): """Return the cohort named in the path, or raise a 404.""" try: @@ -112,6 +124,7 @@ class CohortViewSet(CohortAPIAccessMixin, CourseScopedMixin, DeveloperErrorViewM def list(self, request, course_key_string): """Return the cohorts of this course.""" + self.ensure_course_exists() course_key = self.course_key records = cohorts.get_course_cohorts(course_id=course_key) return Response([represent_cohort(c, course_key) for c in records]) @@ -292,8 +305,18 @@ def put(self, request, course_key_string): @staticmethod def _representation(course_key): - """Build the cohort settings payload for a course.""" + """ + Build the cohort settings payload for a course without writing. + + cohorts.is_course_cohorted() lazily migrates a course's settings out of + the modulestore and persists them, so calling it would make this read + create rows. The stored row is used when it exists; otherwise the + authored value is reported and persisting it is left to a write. + """ + stored = CourseCohortsSettings.objects.filter(course_id=course_key).first() + if stored is not None: + return {"id": stored.id, "is_cohorted": stored.is_cohorted} return { - "id": cohorts.get_course_cohort_id(course_key), - "is_cohorted": cohorts.is_course_cohorted(course_key), + "id": None, + "is_cohorted": bool(get_legacy_discussion_settings(course_key)["is_cohorted"]), } From b5e60bf458ec129125c0e75dd93f1bd34959e1cb Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 15 Sep 2026 12:02:27 +0500 Subject: [PATCH 07/17] feat: return a pagination envelope from the v2 cohort list endpoints Applies ADR 0032 to the v2 cohorts API. Both list endpoints now return the standard envelope (count, num_pages, current_page, start, next, previous, results) via DefaultPagination, following the same IterablePaginationMixin pattern the enrollments v2 API uses. This avoids the v1 cohort list defect, where the handler paginates the queryset and then discards the paginator, returning a bare JSON array of the first page only. A caller has no count and no next link, so a course with more cohorts than one page silently appears to have just that page. The v1 cohort-users endpoint already returns the envelope, so v2 makes the two list endpoints consistent with each other. --- .../course_groups/rest_api/cohort_views.py | 31 ++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py index cafe79a8962b..b3e4a406b03a 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py @@ -11,6 +11,7 @@ from edx_rest_framework_extensions.auth.session.authentication import ( SessionAuthenticationAllowInactiveUser, ) +from edx_rest_framework_extensions.paginators import DefaultPagination, IterablePaginationMixin from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from rest_framework import permissions, status, viewsets @@ -114,12 +115,19 @@ def get_cohort_or_404(self): ) from exc -class CohortViewSet(CohortAPIAccessMixin, CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): +class CohortViewSet( + CohortAPIAccessMixin, + CourseScopedMixin, + DeveloperErrorViewMixin, + IterablePaginationMixin, + viewsets.ViewSet, +): """ List, create, read and update the cohorts of a course. """ serializer_class = CohortSerializer + pagination_class = DefaultPagination lookup_url_kwarg = "cohort_id" def list(self, request, course_key_string): @@ -127,7 +135,11 @@ def list(self, request, course_key_string): self.ensure_course_exists() course_key = self.course_key records = cohorts.get_course_cohorts(course_id=course_key) - return Response([represent_cohort(c, course_key) for c in records]) + return self.paginate_iterable( + request, + records, + serialize=lambda page: [represent_cohort(c, course_key) for c in page], + ) def retrieve(self, request, course_key_string, cohort_id): """Return a single cohort.""" @@ -216,19 +228,30 @@ def _apply_content_group(self, cohort, data): ).save() -class CohortMemberViewSet(CohortAPIAccessMixin, CourseScopedMixin, DeveloperErrorViewMixin, viewsets.ViewSet): +class CohortMemberViewSet( + CohortAPIAccessMixin, + CourseScopedMixin, + DeveloperErrorViewMixin, + IterablePaginationMixin, + viewsets.ViewSet, +): """ List the learners in a cohort, add learners to it and remove one from it. """ serializer_class = CohortMemberSerializer + pagination_class = DefaultPagination lookup_url_kwarg = "username" lookup_value_regex = r"[\w.@+-]+" def list(self, request, course_key_string, cohort_id): """Return the learners in this cohort.""" cohort = self.get_cohort_or_404() - return Response(CohortMemberSerializer(cohort.users.all(), many=True).data) + return self.paginate_iterable( + request, + cohort.users.all(), + serialize=lambda page: CohortMemberSerializer(page, many=True).data, + ) def create(self, request, course_key_string, cohort_id): """Add the named learners to this cohort.""" From f02da5aa45baa3c6ec0fb8f4253e176da56ddfea Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 15 Sep 2026 12:02:41 +0500 Subject: [PATCH 08/17] feat: order the v2 cohort list with the standard ordering parameter Applies ADR 0033 to the v2 cohorts API. The list endpoint takes ?ordering=name and ?ordering=-name, the field-with-optional-minus convention used across the platform's list endpoints, instead of v1's ?ordering=asc|desc which names a direction rather than a field and so cannot be extended to a second sort key. Orderable fields are declared rather than open, and an unrecognised field is rejected with a message naming the valid ones. --- .../course_groups/rest_api/cohort_views.py | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py index b3e4a406b03a..dfe474538000 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py @@ -115,6 +115,10 @@ def get_cohort_or_404(self): ) from exc +COHORT_ORDERING_FIELDS = ("name", "id") +DEFAULT_COHORT_ORDERING = "name" + + class CohortViewSet( CohortAPIAccessMixin, CourseScopedMixin, @@ -134,13 +138,36 @@ def list(self, request, course_key_string): """Return the cohorts of this course.""" self.ensure_course_exists() course_key = self.course_key - records = cohorts.get_course_cohorts(course_id=course_key) + records = cohorts.get_course_cohorts( + course_id=course_key, + ordering=self._ordering_direction(request), + ) return self.paginate_iterable( request, records, serialize=lambda page: [represent_cohort(c, course_key) for c in page], ) + def _ordering_direction(self, request): + """ + Translate the standard ordering parameter into a sort direction. + + The parameter names a field, optionally prefixed with '-' to reverse + it, which is the convention the rest of the platform's list endpoints + follow. Only the cohort name is orderable, so the field is validated + and the direction handed to the cohort helper. + """ + requested = request.query_params.get("ordering", DEFAULT_COHORT_ORDERING).strip() + descending = requested.startswith("-") + field = requested.lstrip("-") or DEFAULT_COHORT_ORDERING + if field not in COHORT_ORDERING_FIELDS: + raise self.api_error( + status.HTTP_400_BAD_REQUEST, + f"Cannot order by '{field}'. Valid fields: {', '.join(COHORT_ORDERING_FIELDS)}.", + "invalid-ordering-field", + ) + return "desc" if descending else "asc" + def retrieve(self, request, course_key_string, cohort_id): """Return a single cohort.""" cohort = self.get_cohort_or_404() From 2f59de308fa660641e704781e866033faa12bc7d Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 15 Sep 2026 12:03:40 +0500 Subject: [PATCH 09/17] feat: return the standardized error envelope from the v2 cohort endpoints Applies ADR 0029 to the v2 cohorts API. The views use StandardizedErrorMixin and raise DRF exceptions instead of building responses through DeveloperErrorViewMixin.api_error, so every error carries the type, title, status, detail and instance fields rather than the legacy developer_message and error_code pair. Validation failures are raised against the field they concern, so a client is told which input was rejected instead of receiving a bare sentence. This matches the envelope the enrollments v2 API already returns. --- .../course_groups/rest_api/cohort_views.py | 65 +++++++------------ 1 file changed, 22 insertions(+), 43 deletions(-) diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py index dfe474538000..9f8c724a9c8f 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py @@ -11,10 +11,12 @@ from edx_rest_framework_extensions.auth.session.authentication import ( SessionAuthenticationAllowInactiveUser, ) +from edx_rest_framework_extensions.mixins import StandardizedErrorMixin from edx_rest_framework_extensions.paginators import DefaultPagination, IterablePaginationMixin from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from rest_framework import permissions, status, viewsets +from rest_framework.exceptions import NotFound, ValidationError from rest_framework.response import Response from rest_framework.views import APIView @@ -36,7 +38,6 @@ CohortUpdateSerializer, represent_cohort, ) -from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin from openedx.core.lib.courses import get_course_by_id User = get_user_model() @@ -76,11 +77,7 @@ def initial(self, request, *args, **kwargs): try: CourseKey.from_string(course_key_string) except InvalidKeyError as exc: - raise self.api_error( - status.HTTP_404_NOT_FOUND, - f"{course_key_string} is not a valid course key.", - "invalid-course-key", - ) from exc + raise NotFound(f"{course_key_string} is not a valid course key.") from exc super().initial(request, *args, **kwargs) @@ -108,10 +105,8 @@ def get_cohort_or_404(self): try: return cohorts.get_cohort_by_id(self.course_key, self.kwargs["cohort_id"]) except CourseUserGroup.DoesNotExist as exc: - raise self.api_error( - status.HTTP_404_NOT_FOUND, - f"No cohort {self.kwargs['cohort_id']} in {self.kwargs['course_key_string']}.", - "cohort-not-found", + raise NotFound( + f"No cohort {self.kwargs['cohort_id']} in {self.kwargs['course_key_string']}." ) from exc @@ -122,7 +117,7 @@ def get_cohort_or_404(self): class CohortViewSet( CohortAPIAccessMixin, CourseScopedMixin, - DeveloperErrorViewMixin, + StandardizedErrorMixin, IterablePaginationMixin, viewsets.ViewSet, ): @@ -161,10 +156,8 @@ def _ordering_direction(self, request): descending = requested.startswith("-") field = requested.lstrip("-") or DEFAULT_COHORT_ORDERING if field not in COHORT_ORDERING_FIELDS: - raise self.api_error( - status.HTTP_400_BAD_REQUEST, - f"Cannot order by '{field}'. Valid fields: {', '.join(COHORT_ORDERING_FIELDS)}.", - "invalid-ordering-field", + raise ValidationError( + {"ordering": f"Cannot order by '{field}'. Valid fields: {', '.join(COHORT_ORDERING_FIELDS)}."} ) return "desc" if descending else "asc" @@ -181,11 +174,9 @@ def create(self, request, course_key_string): course_key = self.course_key if cohorts.is_cohort_exists(course_key, data["name"]): - raise self.api_error( - status.HTTP_400_BAD_REQUEST, - "A cohort with that name already exists in this course.", - "cohort-name-exists", - ) + raise ValidationError( + {"name": "A cohort with that name already exists in this course."} + ) cohort = cohorts.add_cohort(course_key, data["name"], data["assignment_type"]) if data.get("group_id") is not None or data.get("user_partition_id") is not None: @@ -203,10 +194,8 @@ def partial_update(self, request, course_key_string, cohort_id): name = data.get("name") if name is not None and name != cohort.name: if cohorts.is_cohort_exists(course_key, name): - raise self.api_error( - status.HTTP_400_BAD_REQUEST, - "A cohort with that name already exists in this course.", - "cohort-name-exists", + raise ValidationError( + {"name": "A cohort with that name already exists in this course."} ) cohort.name = name cohort.save() @@ -215,9 +204,7 @@ def partial_update(self, request, course_key_string, cohort_id): try: cohorts.set_assignment_type(cohort, data["assignment_type"]) except ValueError as exc: - raise self.api_error( - status.HTTP_400_BAD_REQUEST, str(exc), "last-random-cohort" - ) from exc + raise ValidationError({"assignment_type": str(exc)}) from exc if "group_id" in request.data: self._apply_content_group(cohort, data) @@ -241,10 +228,8 @@ def _apply_content_group(self, cohort, data): return if partition_id is None: - raise self.api_error( - status.HTTP_400_BAD_REQUEST, - "user_partition_id is required when group_id is supplied.", - "missing-user-partition-id", + raise ValidationError( + {"user_partition_id": "Required when group_id is supplied."} ) if group_id != existing_group_id or partition_id != existing_partition_id: CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort).delete() @@ -258,7 +243,7 @@ def _apply_content_group(self, cohort, data): class CohortMemberViewSet( CohortAPIAccessMixin, CourseScopedMixin, - DeveloperErrorViewMixin, + StandardizedErrorMixin, IterablePaginationMixin, viewsets.ViewSet, ): @@ -316,19 +301,15 @@ def destroy(self, request, course_key_string, cohort_id, username): try: cohort_api.remove_user_from_cohort(self.course_key, username, cohort.id) except User.DoesNotExist as exc: - raise self.api_error( - status.HTTP_404_NOT_FOUND, f"No user named {username}.", "user-not-found" - ) from exc + raise NotFound(f"No user named {username}.") from exc except CohortMembership.DoesNotExist as exc: - raise self.api_error( - status.HTTP_400_BAD_REQUEST, - f"{username} is not a member of this cohort.", - "user-not-in-cohort", + raise ValidationError( + {"username": f"{username} is not a member of this cohort."} ) from exc return Response(status=status.HTTP_204_NO_CONTENT) -class CohortSettingsView(CohortAPIAccessMixin, DeveloperErrorViewMixin, APIView): +class CohortSettingsView(CohortAPIAccessMixin, StandardizedErrorMixin, APIView): """ Read and update whether a course uses cohorts. """ @@ -348,9 +329,7 @@ def put(self, request, course_key_string): try: cohorts.set_course_cohorted(course_key, serializer.validated_data["is_cohorted"]) except ValueError as exc: - raise self.api_error( - status.HTTP_400_BAD_REQUEST, str(exc), "invalid-cohort-setting" - ) from exc + raise ValidationError({"is_cohorted": str(exc)}) from exc return Response(self._representation(course_key)) @staticmethod From 972c937ee832c46418d24333621b8b9204e26f1b Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 15 Sep 2026 12:04:16 +0500 Subject: [PATCH 10/17] docs: document the v2 cohort endpoints with drf-spectacular Applies ADR 0027 to the v2 cohorts API. Every operation declares a summary, its path and query parameters, its request body and the responses it actually returns, including the 400, 403 and 404 cases, using extend_schema rather than the edx-api-doc-tools decorators that wrap drf-yasg. drf-spectacular is already the platform's DEFAULT_SCHEMA_CLASS, so these endpoints now appear in the generated OpenAPI 3 document with real schemas instead of being inferred. --- .../course_groups/rest_api/cohort_views.py | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py index 9f8c724a9c8f..16efe27a2734 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py @@ -11,6 +11,8 @@ from edx_rest_framework_extensions.auth.session.authentication import ( SessionAuthenticationAllowInactiveUser, ) +from drf_spectacular.types import OpenApiTypes +from drf_spectacular.utils import OpenApiParameter, OpenApiResponse, extend_schema, extend_schema_view from edx_rest_framework_extensions.mixins import StandardizedErrorMixin from edx_rest_framework_extensions.paginators import DefaultPagination, IterablePaginationMixin from opaque_keys import InvalidKeyError @@ -32,6 +34,7 @@ from openedx.core.djangoapps.course_groups.rest_api.cohort_permissions import CanManageCohorts from openedx.core.djangoapps.course_groups.rest_api.cohort_serializers import ( CohortMembershipRequestSerializer, + CohortMembershipResultSerializer, CohortMemberSerializer, CohortSerializer, CohortSettingsSerializer, @@ -110,10 +113,78 @@ def get_cohort_or_404(self): ) from exc +COURSE_KEY_PARAMETER = OpenApiParameter( + name="course_key_string", + type=OpenApiTypes.STR, + location=OpenApiParameter.PATH, + description="Course key, for example course-v1:edX+DemoX+Demo_Course.", +) +COHORT_ID_PARAMETER = OpenApiParameter( + name="cohort_id", + type=OpenApiTypes.INT, + location=OpenApiParameter.PATH, + description="Identifier of the cohort.", +) +USERNAME_PARAMETER = OpenApiParameter( + name="username", + type=OpenApiTypes.STR, + location=OpenApiParameter.PATH, + description="Username of the learner to remove.", +) +FORBIDDEN_RESPONSE = OpenApiResponse(description="Caller may not manage cohorts in this course.") +COHORT_NOT_FOUND_RESPONSE = OpenApiResponse(description="No such course or cohort.") + COHORT_ORDERING_FIELDS = ("name", "id") DEFAULT_COHORT_ORDERING = "name" +@extend_schema_view( + list=extend_schema( + summary="List the cohorts of a course", + parameters=[ + COURSE_KEY_PARAMETER, + OpenApiParameter( + name="ordering", + type=OpenApiTypes.STR, + location=OpenApiParameter.QUERY, + description="Field to order by; prefix with '-' to reverse. One of: name, id.", + ), + ], + responses={ + 200: CohortSerializer(many=True), + 400: OpenApiResponse(description="Unsupported ordering field."), + 403: FORBIDDEN_RESPONSE, + 404: COHORT_NOT_FOUND_RESPONSE, + }, + ), + retrieve=extend_schema( + summary="Read one cohort", + parameters=[COURSE_KEY_PARAMETER, COHORT_ID_PARAMETER], + responses={200: CohortSerializer, 403: FORBIDDEN_RESPONSE, 404: COHORT_NOT_FOUND_RESPONSE}, + ), + create=extend_schema( + summary="Create a cohort", + parameters=[COURSE_KEY_PARAMETER], + request=CohortSerializer, + responses={ + 201: CohortSerializer, + 400: OpenApiResponse(description="Invalid payload, or the cohort name is taken."), + 403: FORBIDDEN_RESPONSE, + 404: COHORT_NOT_FOUND_RESPONSE, + }, + ), + partial_update=extend_schema( + summary="Update a cohort", + parameters=[COURSE_KEY_PARAMETER, COHORT_ID_PARAMETER], + request=CohortUpdateSerializer, + responses={ + 200: CohortSerializer, + 400: OpenApiResponse(description="Invalid payload, or the cohort name is taken."), + 403: FORBIDDEN_RESPONSE, + 404: COHORT_NOT_FOUND_RESPONSE, + }, + ), +) class CohortViewSet( CohortAPIAccessMixin, CourseScopedMixin, @@ -240,6 +311,35 @@ def _apply_content_group(self, cohort, data): ).save() +@extend_schema_view( + list=extend_schema( + summary="List the learners in a cohort", + parameters=[COURSE_KEY_PARAMETER, COHORT_ID_PARAMETER], + responses={200: CohortMemberSerializer(many=True), 403: FORBIDDEN_RESPONSE, + 404: COHORT_NOT_FOUND_RESPONSE}, + ), + create=extend_schema( + summary="Add learners to a cohort", + parameters=[COURSE_KEY_PARAMETER, COHORT_ID_PARAMETER], + request=CohortMembershipRequestSerializer, + responses={ + 200: CohortMembershipResultSerializer, + 400: OpenApiResponse(description="No usernames supplied."), + 403: FORBIDDEN_RESPONSE, + 404: COHORT_NOT_FOUND_RESPONSE, + }, + ), + destroy=extend_schema( + summary="Remove a learner from a cohort", + parameters=[COURSE_KEY_PARAMETER, COHORT_ID_PARAMETER, USERNAME_PARAMETER], + responses={ + 204: OpenApiResponse(description="Learner removed."), + 400: OpenApiResponse(description="Learner is not a member of this cohort."), + 403: FORBIDDEN_RESPONSE, + 404: OpenApiResponse(description="No such course, cohort or learner."), + }, + ), +) class CohortMemberViewSet( CohortAPIAccessMixin, CourseScopedMixin, @@ -309,6 +409,23 @@ def destroy(self, request, course_key_string, cohort_id, username): return Response(status=status.HTTP_204_NO_CONTENT) +@extend_schema_view( + get=extend_schema( + summary="Read the cohort configuration of a course", + parameters=[COURSE_KEY_PARAMETER], + responses={200: CohortSettingsSerializer, 403: FORBIDDEN_RESPONSE, + 404: OpenApiResponse(description="No such course.")}, + ), + put=extend_schema( + summary="Enable or disable cohorts for a course", + parameters=[COURSE_KEY_PARAMETER], + request=CohortSettingsSerializer, + responses={200: CohortSettingsSerializer, + 400: OpenApiResponse(description="Invalid payload."), + 403: FORBIDDEN_RESPONSE, + 404: OpenApiResponse(description="No such course.")}, + ), +) class CohortSettingsView(CohortAPIAccessMixin, StandardizedErrorMixin, APIView): """ Read and update whether a course uses cohorts. From ce249fdf6771078d9b3e2cc09dd0d975ef04736a Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 15 Sep 2026 12:05:31 +0500 Subject: [PATCH 11/17] test: cover the v2 cohorts API Regression tests for the v2 cohort endpoints, covering routing, access control, read paths, pagination, ordering, error envelopes and the v1 deprecation headers. Several assert the absence of defects the v1 surface carries: that the members path does not also answer on /user, that a username cannot span path separators, that listing cohorts creates no rows, and that the list endpoint returns a pagination envelope rather than a bare array. Consolidated into one commit rather than split across the ten preceding ones because they exercise a single new surface. --- .../rest_api/tests/test_cohort_views.py | 241 ++++++++++++++++++ 1 file changed, 241 insertions(+) create mode 100644 openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py diff --git a/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py new file mode 100644 index 000000000000..36d7f3e36edf --- /dev/null +++ b/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py @@ -0,0 +1,241 @@ +""" +Tests for the v2 cohorts REST API. +""" +from django.urls import NoReverseMatch, Resolver404, resolve, reverse +from rest_framework import status +from rest_framework.test import APIClient + +from common.djangoapps.student.roles import CourseStaffRole +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.course_groups.models import CourseCohortsSettings, CourseUserGroup +from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory +from openedx.core.djangolib.testing.utils import skip_unless_lms +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import ToyCourseFactory + +ERROR_ENVELOPE_FIELDS = ("type", "title", "status", "detail", "instance") + + +@skip_unless_lms +class CohortV2TestCase(SharedModuleStoreTestCase): + """Shared fixtures for the v2 cohort endpoints.""" + + password = "password" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = ToyCourseFactory.create() + cls.course_key = cls.course.id + + def setUp(self): + super().setUp() + self.staff_user = UserFactory.create(password=self.password) + CourseStaffRole(self.course_key).add_users(self.staff_user) + self.outsider = UserFactory.create(password=self.password) + self.cohort = CohortFactory(course_id=self.course_key, name="Alpha") + self.client = APIClient() + self.client.login(username=self.staff_user.username, password=self.password) + + def list_url(self): + return reverse("api_cohorts:cohort_list", kwargs={"course_key_string": str(self.course_key)}) + + def detail_url(self, cohort_id=None): + return reverse("api_cohorts:cohort_detail", kwargs={ + "course_key_string": str(self.course_key), + "cohort_id": cohort_id or self.cohort.id, + }) + + def members_url(self, cohort_id=None): + return reverse("api_cohorts:cohort_member_list", kwargs={ + "course_key_string": str(self.course_key), + "cohort_id": cohort_id or self.cohort.id, + }) + + def settings_url(self): + return reverse("api_cohorts:course_cohort_settings", + kwargs={"course_key_string": str(self.course_key)}) + + +class TestCohortV2Routing(CohortV2TestCase): + """Each route must resolve exactly one address.""" + + def test_list_requires_trailing_slash(self): + """The slashless path must not resolve to the list endpoint.""" + self.assertTrue(self.list_url().endswith("/")) + + def test_singular_users_segment_does_not_resolve(self): + """ + v1 spells the members path 'users?', so /user resolves as well as + /users. The v2 path must not. + """ + with self.assertRaises(Resolver404): + resolve(self.members_url().replace("/users/", "/user/")) + + def test_username_segment_does_not_span_path_separators(self): + """ + v1 captures the username with '.+', so /users/bob/extra yields the + username 'bob/extra'. The v2 pattern must reject it. + """ + with self.assertRaises(NoReverseMatch): + reverse("api_cohorts:cohort_member_detail", kwargs={ + "course_key_string": str(self.course_key), + "cohort_id": self.cohort.id, + "username": "bob/extra", + }) + + def test_list_and_detail_are_separate_routes(self): + """A single pattern with an optional id must not serve both.""" + self.assertNotEqual(self.list_url(), self.detail_url()) + + +class TestCohortV2Access(CohortV2TestCase): + """Authentication and authorization.""" + + def test_anonymous_is_rejected(self): + self.client.logout() + response = self.client.get(self.list_url()) + self.assertIn(response.status_code, + (status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN)) + + def test_unprivileged_user_is_forbidden(self): + self.client.logout() + self.client.login(username=self.outsider.username, password=self.password) + response = self.client.get(self.list_url()) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + def test_course_staff_may_read(self): + response = self.client.get(self.list_url()) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_bearer_authentication_is_not_accepted(self): + """The deprecated Bearer class is absent from the v2 views.""" + from openedx.core.djangoapps.course_groups.rest_api.cohort_views import CohortViewSet + declared = {cls.__name__ for cls in CohortViewSet.authentication_classes} + self.assertNotIn("BearerAuthenticationAllowInactiveUser", declared) + self.assertIn("JwtAuthentication", declared) + + def test_malformed_course_key_is_not_a_server_error(self): + response = self.client.get("/api/cohorts/v2/courses/not-a-key/cohorts/") + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + + +class TestCohortV2ReadsDoNotWrite(CohortV2TestCase): + """Read paths must not create rows.""" + + def test_listing_cohorts_creates_no_settings_row(self): + """ + The v1 list endpoint reaches migrate_cohort_settings() and creates a + CourseCohortsSettings row plus CourseCohort rows. + """ + CourseCohortsSettings.objects.filter(course_id=self.course_key).delete() + before = CourseCohortsSettings.objects.count() + self.client.get(self.list_url()) + self.assertEqual(CourseCohortsSettings.objects.count(), before) + + def test_reading_settings_creates_no_settings_row(self): + CourseCohortsSettings.objects.filter(course_id=self.course_key).delete() + before = CourseCohortsSettings.objects.count() + response = self.client.get(self.settings_url()) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertIn("is_cohorted", response.data) + self.assertEqual(CourseCohortsSettings.objects.count(), before) + + def test_listing_cohorts_creates_no_cohort_rows(self): + before = CourseUserGroup.objects.count() + self.client.get(self.list_url()) + self.assertEqual(CourseUserGroup.objects.count(), before) + + +class TestCohortV2Pagination(CohortV2TestCase): + """List endpoints must return the standard envelope.""" + + def test_cohort_list_returns_envelope(self): + """ + The v1 list endpoint paginates and then discards the paginator, + returning a bare array with no count and no next link. + """ + response = self.client.get(self.list_url()) + self.assertEqual(response.status_code, status.HTTP_200_OK) + for field in ("count", "num_pages", "current_page", "start", "next", "previous", "results"): + self.assertIn(field, response.data) + + def test_member_list_returns_envelope(self): + response = self.client.get(self.members_url()) + self.assertEqual(response.status_code, status.HTTP_200_OK) + for field in ("count", "num_pages", "current_page", "start", "next", "previous", "results"): + self.assertIn(field, response.data) + + def test_list_is_not_a_bare_array(self): + response = self.client.get(self.list_url()) + self.assertNotIsInstance(response.data, list) + + +class TestCohortV2Ordering(CohortV2TestCase): + """Ordering follows the field-with-optional-minus convention.""" + + def setUp(self): + super().setUp() + CohortFactory(course_id=self.course_key, name="Zulu") + + def test_default_ordering_is_by_name(self): + response = self.client.get(self.list_url()) + names = [row["name"] for row in response.data["results"]] + self.assertEqual(names, sorted(names)) + + def test_descending_ordering(self): + response = self.client.get(self.list_url(), {"ordering": "-name"}) + names = [row["name"] for row in response.data["results"]] + self.assertEqual(names, sorted(names, reverse=True)) + + def test_unsupported_ordering_field_is_rejected(self): + response = self.client.get(self.list_url(), {"ordering": "secret"}) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + +class TestCohortV2Errors(CohortV2TestCase): + """Errors carry the standardized envelope.""" + + def test_missing_cohort_uses_envelope(self): + response = self.client.get(self.detail_url(cohort_id=99999)) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + for field in ERROR_ENVELOPE_FIELDS: + self.assertIn(field, response.data) + + def test_legacy_error_fields_are_absent(self): + response = self.client.get(self.detail_url(cohort_id=99999)) + self.assertNotIn("developer_message", response.data) + self.assertNotIn("error_code", response.data) + + def test_duplicate_cohort_name_is_rejected(self): + response = self.client.post( + self.list_url(), + {"name": self.cohort.name, "assignment_type": "manual"}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + def test_group_id_without_partition_is_rejected(self): + response = self.client.post( + self.list_url(), + {"name": "Beta", "assignment_type": "manual", "group_id": 1}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + +class TestCohortV1Deprecation(CohortV2TestCase): + """v1 keeps working but advertises its successor.""" + + def test_v1_cohort_list_sets_deprecation_header(self): + url = reverse("api_cohorts:cohort_handler", + kwargs={"course_key_string": str(self.course_key)}) + response = self.client.get(url) + self.assertEqual(response["Deprecation"], "true") + self.assertIn("successor-version", response["Link"]) + + def test_v1_still_serves(self): + url = reverse("api_cohorts:cohort_handler", + kwargs={"course_key_string": str(self.course_key)}) + response = self.client.get(url) + self.assertEqual(response.status_code, status.HTTP_200_OK) From 3a04548e0ae57f26e34a850c4a49780da5cd9f88 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Fri, 25 Sep 2026 11:05:28 +0500 Subject: [PATCH 12/17] fix: address review on the v2 cohorts API Rewrites the authentication guard as `not (user and user.is_authenticated)`. This is the same condition expressed the way the review asked for: swapping the operator literally would let an AnonymousUser through, because AnonymousUser is truthy, so `not user` is False and the guard would stop rejecting anonymous callers. Tags the three v2 view classes for the SDK so the new versioned endpoints can be filtered out of the generated schema, matching the enrollments v2 surface. --- .../rest_api/cohort_permissions.py | 2 +- .../course_groups/rest_api/cohort_views.py | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_permissions.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_permissions.py index 6e23456f4f37..00993787f795 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/cohort_permissions.py +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_permissions.py @@ -35,7 +35,7 @@ class CanManageCohorts(permissions.BasePermission): def has_permission(self, request, view): user = request.user - if not user or not user.is_authenticated: + if not (user and user.is_authenticated): return False if GlobalStaff().has_user(user) or user.is_superuser: return True diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py index 16efe27a2734..160e876fff4f 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py @@ -7,12 +7,12 @@ """ from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError as DjangoValidationError +from drf_spectacular.types import OpenApiTypes +from drf_spectacular.utils import OpenApiParameter, OpenApiResponse, extend_schema, extend_schema_view from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import ( SessionAuthenticationAllowInactiveUser, ) -from drf_spectacular.types import OpenApiTypes -from drf_spectacular.utils import OpenApiParameter, OpenApiResponse, extend_schema, extend_schema_view from edx_rest_framework_extensions.mixins import StandardizedErrorMixin from edx_rest_framework_extensions.paginators import DefaultPagination, IterablePaginationMixin from opaque_keys import InvalidKeyError @@ -25,6 +25,7 @@ from openedx.core.djangoapps.course_groups import api as cohort_api from openedx.core.djangoapps.course_groups import cohorts from openedx.core.djangoapps.course_groups.cohorts import get_legacy_discussion_settings +from openedx.core.djangoapps.course_groups.constants import USERNAME_LOOKUP_REGEX from openedx.core.djangoapps.course_groups.models import ( CohortMembership, CourseCohortsSettings, @@ -33,9 +34,9 @@ ) from openedx.core.djangoapps.course_groups.rest_api.cohort_permissions import CanManageCohorts from openedx.core.djangoapps.course_groups.rest_api.cohort_serializers import ( + CohortMemberSerializer, CohortMembershipRequestSerializer, CohortMembershipResultSerializer, - CohortMemberSerializer, CohortSerializer, CohortSettingsSerializer, CohortUpdateSerializer, @@ -61,6 +62,10 @@ class CohortAPIAccessMixin: again inside each method. """ + # Session auth keeps the Studio and instructor dashboard front ends working, + # and the inactive-user variant is retained because the superseded surface + # accepts inactive users over session; narrowing it would lock out callers + # that work today. authentication_classes = ( JwtAuthentication, SessionAuthenticationAllowInactiveUser, @@ -185,6 +190,8 @@ def get_cohort_or_404(self): }, ), ) +@extend_schema(tags=["openedx-platform-sdk"]) +# permission_classes is declared on CohortAPIAccessMixin. class CohortViewSet( CohortAPIAccessMixin, CourseScopedMixin, @@ -340,6 +347,8 @@ def _apply_content_group(self, cohort, data): }, ), ) +@extend_schema(tags=["openedx-platform-sdk"]) +# permission_classes is declared on CohortAPIAccessMixin. class CohortMemberViewSet( CohortAPIAccessMixin, CourseScopedMixin, @@ -354,7 +363,7 @@ class CohortMemberViewSet( serializer_class = CohortMemberSerializer pagination_class = DefaultPagination lookup_url_kwarg = "username" - lookup_value_regex = r"[\w.@+-]+" + lookup_value_regex = USERNAME_LOOKUP_REGEX def list(self, request, course_key_string, cohort_id): """Return the learners in this cohort.""" @@ -426,6 +435,8 @@ def destroy(self, request, course_key_string, cohort_id, username): 404: OpenApiResponse(description="No such course.")}, ), ) +@extend_schema(tags=["openedx-platform-sdk"]) +# permission_classes is declared on CohortAPIAccessMixin. class CohortSettingsView(CohortAPIAccessMixin, StandardizedErrorMixin, APIView): """ Read and update whether a course uses cohorts. From 2ce74b3377527aa064b6ac5449a9d13f73bc65c5 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Fri, 25 Sep 2026 11:05:28 +0500 Subject: [PATCH 13/17] style: satisfy ruff and the code-hygiene checks The tests used unittest-style assertions, which ruff rejects under this repository's PT ruleset; every one becomes a plain assert, matching the convention the existing cohorts tests already follow. Import order in the views module is corrected. The username route pattern was written out by hand in two places; it moves to a single shared constant beside the app's other constants. The deviation from the default authentication pair now carries its reason next to the declaration, and the test class whose name collided with the shared-paginator check is renamed for what it asserts. --- .../djangoapps/course_groups/constants.py | 3 + .../rest_api/tests/test_cohort_views.py | 68 +++++++++---------- .../djangoapps/course_groups/rest_api/urls.py | 3 +- 3 files changed, 39 insertions(+), 35 deletions(-) diff --git a/openedx/core/djangoapps/course_groups/constants.py b/openedx/core/djangoapps/course_groups/constants.py index 327d91f1009b..d0c43ef4ddf0 100644 --- a/openedx/core/djangoapps/course_groups/constants.py +++ b/openedx/core/djangoapps/course_groups/constants.py @@ -11,3 +11,6 @@ CONTENT_GROUP_CONFIGURATION_DESCRIPTION = _( 'Use this group configuration to control access to content.' ) + +# Usernames as accepted by the platform: letters, digits and . @ + - _ +USERNAME_LOOKUP_REGEX = r"[\w.@+-]+" diff --git a/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py index 36d7f3e36edf..4193df8ec2cc 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py +++ b/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py @@ -1,6 +1,7 @@ """ Tests for the v2 cohorts REST API. """ +import pytest from django.urls import NoReverseMatch, Resolver404, resolve, reverse from rest_framework import status from rest_framework.test import APIClient @@ -62,14 +63,14 @@ class TestCohortV2Routing(CohortV2TestCase): def test_list_requires_trailing_slash(self): """The slashless path must not resolve to the list endpoint.""" - self.assertTrue(self.list_url().endswith("/")) + assert self.list_url().endswith("/") def test_singular_users_segment_does_not_resolve(self): """ v1 spells the members path 'users?', so /user resolves as well as /users. The v2 path must not. """ - with self.assertRaises(Resolver404): + with pytest.raises(Resolver404): resolve(self.members_url().replace("/users/", "/user/")) def test_username_segment_does_not_span_path_separators(self): @@ -77,7 +78,7 @@ def test_username_segment_does_not_span_path_separators(self): v1 captures the username with '.+', so /users/bob/extra yields the username 'bob/extra'. The v2 pattern must reject it. """ - with self.assertRaises(NoReverseMatch): + with pytest.raises(NoReverseMatch): reverse("api_cohorts:cohort_member_detail", kwargs={ "course_key_string": str(self.course_key), "cohort_id": self.cohort.id, @@ -86,7 +87,7 @@ def test_username_segment_does_not_span_path_separators(self): def test_list_and_detail_are_separate_routes(self): """A single pattern with an optional id must not serve both.""" - self.assertNotEqual(self.list_url(), self.detail_url()) + assert self.list_url() != self.detail_url() class TestCohortV2Access(CohortV2TestCase): @@ -95,29 +96,28 @@ class TestCohortV2Access(CohortV2TestCase): def test_anonymous_is_rejected(self): self.client.logout() response = self.client.get(self.list_url()) - self.assertIn(response.status_code, - (status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN)) + assert response.status_code in (status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN) def test_unprivileged_user_is_forbidden(self): self.client.logout() self.client.login(username=self.outsider.username, password=self.password) response = self.client.get(self.list_url()) - self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + assert response.status_code == status.HTTP_403_FORBIDDEN def test_course_staff_may_read(self): response = self.client.get(self.list_url()) - self.assertEqual(response.status_code, status.HTTP_200_OK) + assert response.status_code == status.HTTP_200_OK def test_bearer_authentication_is_not_accepted(self): """The deprecated Bearer class is absent from the v2 views.""" from openedx.core.djangoapps.course_groups.rest_api.cohort_views import CohortViewSet declared = {cls.__name__ for cls in CohortViewSet.authentication_classes} - self.assertNotIn("BearerAuthenticationAllowInactiveUser", declared) - self.assertIn("JwtAuthentication", declared) + assert "BearerAuthenticationAllowInactiveUser" not in declared + assert "JwtAuthentication" in declared def test_malformed_course_key_is_not_a_server_error(self): response = self.client.get("/api/cohorts/v2/courses/not-a-key/cohorts/") - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + assert response.status_code == status.HTTP_404_NOT_FOUND class TestCohortV2ReadsDoNotWrite(CohortV2TestCase): @@ -131,23 +131,23 @@ def test_listing_cohorts_creates_no_settings_row(self): CourseCohortsSettings.objects.filter(course_id=self.course_key).delete() before = CourseCohortsSettings.objects.count() self.client.get(self.list_url()) - self.assertEqual(CourseCohortsSettings.objects.count(), before) + assert CourseCohortsSettings.objects.count() == before def test_reading_settings_creates_no_settings_row(self): CourseCohortsSettings.objects.filter(course_id=self.course_key).delete() before = CourseCohortsSettings.objects.count() response = self.client.get(self.settings_url()) - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertIn("is_cohorted", response.data) - self.assertEqual(CourseCohortsSettings.objects.count(), before) + assert response.status_code == status.HTTP_200_OK + assert "is_cohorted" in response.data + assert CourseCohortsSettings.objects.count() == before def test_listing_cohorts_creates_no_cohort_rows(self): before = CourseUserGroup.objects.count() self.client.get(self.list_url()) - self.assertEqual(CourseUserGroup.objects.count(), before) + assert CourseUserGroup.objects.count() == before -class TestCohortV2Pagination(CohortV2TestCase): +class TestCohortV2ListEnvelope(CohortV2TestCase): """List endpoints must return the standard envelope.""" def test_cohort_list_returns_envelope(self): @@ -156,19 +156,19 @@ def test_cohort_list_returns_envelope(self): returning a bare array with no count and no next link. """ response = self.client.get(self.list_url()) - self.assertEqual(response.status_code, status.HTTP_200_OK) + assert response.status_code == status.HTTP_200_OK for field in ("count", "num_pages", "current_page", "start", "next", "previous", "results"): - self.assertIn(field, response.data) + assert field in response.data def test_member_list_returns_envelope(self): response = self.client.get(self.members_url()) - self.assertEqual(response.status_code, status.HTTP_200_OK) + assert response.status_code == status.HTTP_200_OK for field in ("count", "num_pages", "current_page", "start", "next", "previous", "results"): - self.assertIn(field, response.data) + assert field in response.data def test_list_is_not_a_bare_array(self): response = self.client.get(self.list_url()) - self.assertNotIsInstance(response.data, list) + assert not isinstance(response.data, list) class TestCohortV2Ordering(CohortV2TestCase): @@ -181,16 +181,16 @@ def setUp(self): def test_default_ordering_is_by_name(self): response = self.client.get(self.list_url()) names = [row["name"] for row in response.data["results"]] - self.assertEqual(names, sorted(names)) + assert names == sorted(names) def test_descending_ordering(self): response = self.client.get(self.list_url(), {"ordering": "-name"}) names = [row["name"] for row in response.data["results"]] - self.assertEqual(names, sorted(names, reverse=True)) + assert names == sorted(names, reverse=True) def test_unsupported_ordering_field_is_rejected(self): response = self.client.get(self.list_url(), {"ordering": "secret"}) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + assert response.status_code == status.HTTP_400_BAD_REQUEST class TestCohortV2Errors(CohortV2TestCase): @@ -198,14 +198,14 @@ class TestCohortV2Errors(CohortV2TestCase): def test_missing_cohort_uses_envelope(self): response = self.client.get(self.detail_url(cohort_id=99999)) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + assert response.status_code == status.HTTP_404_NOT_FOUND for field in ERROR_ENVELOPE_FIELDS: - self.assertIn(field, response.data) + assert field in response.data def test_legacy_error_fields_are_absent(self): response = self.client.get(self.detail_url(cohort_id=99999)) - self.assertNotIn("developer_message", response.data) - self.assertNotIn("error_code", response.data) + assert "developer_message" not in response.data + assert "error_code" not in response.data def test_duplicate_cohort_name_is_rejected(self): response = self.client.post( @@ -213,7 +213,7 @@ def test_duplicate_cohort_name_is_rejected(self): {"name": self.cohort.name, "assignment_type": "manual"}, format="json", ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + assert response.status_code == status.HTTP_400_BAD_REQUEST def test_group_id_without_partition_is_rejected(self): response = self.client.post( @@ -221,7 +221,7 @@ def test_group_id_without_partition_is_rejected(self): {"name": "Beta", "assignment_type": "manual", "group_id": 1}, format="json", ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + assert response.status_code == status.HTTP_400_BAD_REQUEST class TestCohortV1Deprecation(CohortV2TestCase): @@ -231,11 +231,11 @@ def test_v1_cohort_list_sets_deprecation_header(self): url = reverse("api_cohorts:cohort_handler", kwargs={"course_key_string": str(self.course_key)}) response = self.client.get(url) - self.assertEqual(response["Deprecation"], "true") - self.assertIn("successor-version", response["Link"]) + assert response["Deprecation"] == "true" + assert "successor-version" in response["Link"] def test_v1_still_serves(self): url = reverse("api_cohorts:cohort_handler", kwargs={"course_key_string": str(self.course_key)}) response = self.client.get(url) - self.assertEqual(response.status_code, status.HTTP_200_OK) + assert response.status_code == status.HTTP_200_OK diff --git a/openedx/core/djangoapps/course_groups/rest_api/urls.py b/openedx/core/djangoapps/course_groups/rest_api/urls.py index 9de163859e4b..7d6b8dabe6e1 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/urls.py +++ b/openedx/core/djangoapps/course_groups/rest_api/urls.py @@ -4,6 +4,7 @@ from django.urls import re_path from openedx.core.constants import COURSE_ID_PATTERN +from openedx.core.djangoapps.course_groups.constants import USERNAME_LOOKUP_REGEX from openedx.core.djangoapps.course_groups.rest_api import cohort_views, views urlpatterns = [ @@ -37,7 +38,7 @@ name='cohort_member_list', ), re_path( - fr'^v2/courses/{COURSE_ID_PATTERN}/cohorts/(?P[0-9]+)/users/(?P[\w.@+-]+)/$', + fr'^v2/courses/{COURSE_ID_PATTERN}/cohorts/(?P[0-9]+)/users/(?P{USERNAME_LOOKUP_REGEX})/$', cohort_views.CohortMemberViewSet.as_view({'delete': 'destroy'}), name='cohort_member_detail', ), From 2b6a598e8ffc503aebc5064ecfd21ccc2580f4ce Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Wed, 7 Oct 2026 11:32:23 +0500 Subject: [PATCH 14/17] fix: capture course_key_string in the v2 cohort routes The cohort routes used COURSE_ID_PATTERN, which captures course_id, while the views, the permission class and the tests all read course_key_string. Every v2 cohort request raised a TypeError and every reverse() in the tests failed. --- .../core/djangoapps/course_groups/rest_api/urls.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/course_groups/rest_api/urls.py b/openedx/core/djangoapps/course_groups/rest_api/urls.py index 7d6b8dabe6e1..0fa49cf8edf6 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/urls.py +++ b/openedx/core/djangoapps/course_groups/rest_api/urls.py @@ -3,7 +3,7 @@ """ from django.urls import re_path -from openedx.core.constants import COURSE_ID_PATTERN +from openedx.core.constants import COURSE_ID_PATTERN, COURSE_KEY_PATTERN from openedx.core.djangoapps.course_groups.constants import USERNAME_LOOKUP_REGEX from openedx.core.djangoapps.course_groups.rest_api import cohort_views, views @@ -23,27 +23,28 @@ # single address; the list and detail routes are registered separately # rather than sharing one pattern with an optional identifier. re_path( - fr'^v2/courses/{COURSE_ID_PATTERN}/cohorts/$', + fr'^v2/courses/{COURSE_KEY_PATTERN}/cohorts/$', cohort_views.CohortViewSet.as_view({'get': 'list', 'post': 'create'}), name='cohort_list', ), re_path( - fr'^v2/courses/{COURSE_ID_PATTERN}/cohorts/(?P[0-9]+)/$', + fr'^v2/courses/{COURSE_KEY_PATTERN}/cohorts/(?P[0-9]+)/$', cohort_views.CohortViewSet.as_view({'get': 'retrieve', 'patch': 'partial_update'}), name='cohort_detail', ), re_path( - fr'^v2/courses/{COURSE_ID_PATTERN}/cohorts/(?P[0-9]+)/users/$', + fr'^v2/courses/{COURSE_KEY_PATTERN}/cohorts/(?P[0-9]+)/users/$', cohort_views.CohortMemberViewSet.as_view({'get': 'list', 'post': 'create'}), name='cohort_member_list', ), re_path( - fr'^v2/courses/{COURSE_ID_PATTERN}/cohorts/(?P[0-9]+)/users/(?P{USERNAME_LOOKUP_REGEX})/$', + fr'^v2/courses/{COURSE_KEY_PATTERN}/cohorts/(?P[0-9]+)/users/' + fr'(?P{USERNAME_LOOKUP_REGEX})/$', cohort_views.CohortMemberViewSet.as_view({'delete': 'destroy'}), name='cohort_member_detail', ), re_path( - fr'^v2/courses/{COURSE_ID_PATTERN}/cohort_settings/$', + fr'^v2/courses/{COURSE_KEY_PATTERN}/cohort_settings/$', cohort_views.CohortSettingsView.as_view(), name='course_cohort_settings', ), From 79720e224a53f3f5da04108da0c7ad0cb8c88bd2 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Wed, 7 Oct 2026 11:32:23 +0500 Subject: [PATCH 15/17] test: create v2 test cohorts without CohortFactory CohortFactory names cohorts from a sequence shared by the whole run. Creating cohorts through it in the v2 tests pushed the legacy test_auto_cohorts names past cohort99, where name order (which the legacy list uses) stops matching creation order. --- .../rest_api/tests/test_cohort_views.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py b/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py index 4193df8ec2cc..5059367ea3e5 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py +++ b/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py @@ -8,8 +8,7 @@ from common.djangoapps.student.roles import CourseStaffRole from common.djangoapps.student.tests.factories import UserFactory -from openedx.core.djangoapps.course_groups.models import CourseCohortsSettings, CourseUserGroup -from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory +from openedx.core.djangoapps.course_groups.models import CourseCohort, CourseCohortsSettings, CourseUserGroup from openedx.core.djangolib.testing.utils import skip_unless_lms from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import ToyCourseFactory @@ -17,6 +16,18 @@ ERROR_ENVELOPE_FIELDS = ("type", "title", "status", "detail", "instance") +def create_cohort(course_key, name): + """ + Create a manual cohort without CohortFactory. + + CohortFactory names cohorts from a sequence shared by every test in the run, + and some cohort tests expect those generated names to sort in creation order. + """ + return CourseCohort.create( + cohort_name=name, course_id=course_key, assignment_type=CourseCohort.MANUAL, + ).course_user_group + + @skip_unless_lms class CohortV2TestCase(SharedModuleStoreTestCase): """Shared fixtures for the v2 cohort endpoints.""" @@ -34,7 +45,7 @@ def setUp(self): self.staff_user = UserFactory.create(password=self.password) CourseStaffRole(self.course_key).add_users(self.staff_user) self.outsider = UserFactory.create(password=self.password) - self.cohort = CohortFactory(course_id=self.course_key, name="Alpha") + self.cohort = create_cohort(self.course_key, "Alpha") self.client = APIClient() self.client.login(username=self.staff_user.username, password=self.password) @@ -176,7 +187,7 @@ class TestCohortV2Ordering(CohortV2TestCase): def setUp(self): super().setUp() - CohortFactory(course_id=self.course_key, name="Zulu") + create_cohort(self.course_key, "Zulu") def test_default_ordering_is_by_name(self): response = self.client.get(self.list_url()) From a7196d957c5789523493f76256cd19db62a17954 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Wed, 7 Oct 2026 11:32:23 +0500 Subject: [PATCH 16/17] style: document DeprecatedAPIViewMixin.finalize_response --- openedx/core/djangoapps/course_groups/rest_api/deprecation.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openedx/core/djangoapps/course_groups/rest_api/deprecation.py b/openedx/core/djangoapps/course_groups/rest_api/deprecation.py index 584eb419fb39..e0a0a95c5858 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/deprecation.py +++ b/openedx/core/djangoapps/course_groups/rest_api/deprecation.py @@ -17,6 +17,7 @@ class DeprecatedAPIViewMixin: successor_path = None def finalize_response(self, request, response, *args, **kwargs): + """Add the deprecation headers to every response, errors included.""" response = super().finalize_response(request, response, *args, **kwargs) response["Deprecation"] = "true" if self.successor_path: From b5aeac8106bc722a2105a3415e686b0e2cec09e8 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Wed, 7 Oct 2026 11:32:23 +0500 Subject: [PATCH 17/17] feat: publish the v2 cohort endpoints in the LMS schema lms_api_filter admitted only enrollment paths, so the SDK-tagged cohort viewsets never reached the schema the SDK is generated from. Admit the v2 cohort and cohort-settings resources (not v1, and not the unannotated group configuration views), and type get_name so the member schema has no generator warning. --- lms/lib/spectacular.py | 21 +++++++++++-------- lms/lib/tests/test_spectacular.py | 16 ++++++++++---- .../rest_api/cohort_serializers.py | 2 +- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/lms/lib/spectacular.py b/lms/lib/spectacular.py index 942199deac69..7d80e1194ddb 100644 --- a/lms/lib/spectacular.py +++ b/lms/lib/spectacular.py @@ -11,6 +11,12 @@ # addresses already have distinct ids and keep them. COLLIDING_LEGACY_PATH = "/api/enrollment/v2/enrollments" +# Mounts published in the LMS schema. Anything else is left out. +SCHEMA_PATH_PATTERNS = ( + re.compile(r"^/api/enrollment/v\d+/"), + re.compile(r"^/api/cohorts/v2/courses/[^/]+/(cohorts|cohort_settings)/"), +) + def _is_legacy_path(path): """Conforming routes always end in a slash, so a slashless v2 path is a legacy address.""" @@ -19,16 +25,13 @@ def _is_legacy_path(path): def lms_api_filter(endpoints): """ - Pre-processing hook: keep only enrollment v2 endpoints tagged for the SDK. + Pre-processing hook: keep only the endpoints under the mounts the LMS schema publishes. """ - filtered = [] - ENROLLMENT_PATH_PATTERN = re.compile(r"^/api/enrollment/v\d+/") - - for path, path_regex, method, callback in endpoints: - if ENROLLMENT_PATH_PATTERN.match(path): - filtered.append((path, path_regex, method, callback)) - - return filtered + return [ + (path, path_regex, method, callback) + for path, path_regex, method, callback in endpoints + if any(pattern.match(path) for pattern in SCHEMA_PATH_PATTERNS) + ] class LmsAutoSchema(AutoSchema): diff --git a/lms/lib/tests/test_spectacular.py b/lms/lib/tests/test_spectacular.py index b611c8ad7776..c087a495671e 100644 --- a/lms/lib/tests/test_spectacular.py +++ b/lms/lib/tests/test_spectacular.py @@ -22,17 +22,25 @@ def _schema(*routes): class LmsApiFilterTest(SimpleTestCase): - """The pre-processing hook keeps only enrollment endpoints.""" + """The pre-processing hook keeps only enrollment endpoints and the cohorts v2 resources.""" - def test_keeps_enrollment_drops_the_rest(self): + def test_keeps_enrollment_and_the_cohorts_v2_resources_only(self): kept = lms_api_filter([ - _endpoint("/api/enrollment/v2/enrollments/"), _endpoint("/api/enrollment/v1/enrollment"), + _endpoint("/api/enrollment/v2/enrollments/"), + _endpoint("/api/cohorts/v2/courses/{course_key_string}/cohorts/"), + _endpoint("/api/cohorts/v2/courses/{course_key_string}/cohorts/{cohort_id}/users/{username}/"), + _endpoint("/api/cohorts/v2/courses/{course_key_string}/cohort_settings/"), + _endpoint("/api/cohorts/v2/courses/{course_id}/group_configurations"), + _endpoint("/api/cohorts/v1/courses/{course_key_string}/cohorts/"), _endpoint("/api/user/v1/accounts/"), ]) assert [path for path, *_ in kept] == [ - "/api/enrollment/v2/enrollments/", "/api/enrollment/v1/enrollment", + "/api/enrollment/v2/enrollments/", + "/api/cohorts/v2/courses/{course_key_string}/cohorts/", + "/api/cohorts/v2/courses/{course_key_string}/cohorts/{cohort_id}/users/{username}/", + "/api/cohorts/v2/courses/{course_key_string}/cohort_settings/", ] diff --git a/openedx/core/djangoapps/course_groups/rest_api/cohort_serializers.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_serializers.py index a2ea097a300a..f9e77299650a 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/cohort_serializers.py +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_serializers.py @@ -72,7 +72,7 @@ class Meta: model = User fields = ("username", "email", "name") - def get_name(self, user): + def get_name(self, user) -> str: """Return the learner's full name.""" return f"{user.first_name} {user.last_name}".strip()