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/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/cohort_permissions.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_permissions.py new file mode 100644 index 000000000000..00993787f795 --- /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 and 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_serializers.py b/openedx/core/djangoapps/course_groups/rest_api/cohort_serializers.py new file mode 100644 index 000000000000..f9e77299650a --- /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) -> str: + """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..160e876fff4f --- /dev/null +++ b/openedx/core/djangoapps/course_groups/rest_api/cohort_views.py @@ -0,0 +1,479 @@ +""" +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 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 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 + +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, + 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 ( + CohortMemberSerializer, + CohortMembershipRequestSerializer, + CohortMembershipResultSerializer, + CohortSerializer, + CohortSettingsSerializer, + CohortUpdateSerializer, + represent_cohort, +) +from openedx.core.lib.courses import get_course_by_id + +User = get_user_model() + + +class CohortAPIAccessMixin: + """ + 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. + """ + + # 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, + ) + 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 NotFound(f"{course_key_string} is not a valid course key.") from exc + super().initial(request, *args, **kwargs) + + +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 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: + return cohorts.get_cohort_by_id(self.course_key, self.kwargs["cohort_id"]) + except CourseUserGroup.DoesNotExist as exc: + raise NotFound( + f"No cohort {self.kwargs['cohort_id']} in {self.kwargs['course_key_string']}." + ) 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, + }, + ), +) +@extend_schema(tags=["openedx-platform-sdk"]) +# permission_classes is declared on CohortAPIAccessMixin. +class CohortViewSet( + CohortAPIAccessMixin, + CourseScopedMixin, + StandardizedErrorMixin, + 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): + """Return the cohorts of this course.""" + self.ensure_course_exists() + course_key = self.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 ValidationError( + {"ordering": f"Cannot order by '{field}'. Valid fields: {', '.join(COHORT_ORDERING_FIELDS)}."} + ) + 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() + 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 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: + 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 ValidationError( + {"name": "A cohort with that name already exists in this course."} + ) + 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 ValidationError({"assignment_type": str(exc)}) 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 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() + CourseUserGroupPartitionGroup( + course_user_group=cohort, + partition_id=partition_id, + group_id=group_id, + ).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."), + }, + ), +) +@extend_schema(tags=["openedx-platform-sdk"]) +# permission_classes is declared on CohortAPIAccessMixin. +class CohortMemberViewSet( + CohortAPIAccessMixin, + CourseScopedMixin, + StandardizedErrorMixin, + 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 = USERNAME_LOOKUP_REGEX + + def list(self, request, course_key_string, cohort_id): + """Return the learners in this cohort.""" + cohort = self.get_cohort_or_404() + 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.""" + 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 NotFound(f"No user named {username}.") from exc + except CohortMembership.DoesNotExist as exc: + raise ValidationError( + {"username": f"{username} is not a member of this cohort."} + ) from exc + 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.")}, + ), +) +@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. + """ + + 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 ValidationError({"is_cohorted": str(exc)}) from exc + return Response(self._representation(course_key)) + + @staticmethod + def _representation(course_key): + """ + 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": None, + "is_cohorted": bool(get_legacy_discussion_settings(course_key)["is_cohorted"]), + } 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..e0a0a95c5858 --- /dev/null +++ b/openedx/core/djangoapps/course_groups/rest_api/deprecation.py @@ -0,0 +1,25 @@ +""" +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): + """Add the deprecation headers to every response, errors included.""" + 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/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..5059367ea3e5 --- /dev/null +++ b/openedx/core/djangoapps/course_groups/rest_api/tests/test_cohort_views.py @@ -0,0 +1,252 @@ +""" +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 + +from common.djangoapps.student.roles import CourseStaffRole +from common.djangoapps.student.tests.factories import UserFactory +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 + +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.""" + + 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 = create_cohort(self.course_key, "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.""" + 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 pytest.raises(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 pytest.raises(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.""" + assert 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()) + 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()) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_course_staff_may_read(self): + response = self.client.get(self.list_url()) + 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} + 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/") + assert 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()) + 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()) + 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()) + assert CourseUserGroup.objects.count() == before + + +class TestCohortV2ListEnvelope(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()) + assert response.status_code == status.HTTP_200_OK + for field in ("count", "num_pages", "current_page", "start", "next", "previous", "results"): + assert field in response.data + + def test_member_list_returns_envelope(self): + response = self.client.get(self.members_url()) + assert response.status_code == status.HTTP_200_OK + for field in ("count", "num_pages", "current_page", "start", "next", "previous", "results"): + assert field in response.data + + def test_list_is_not_a_bare_array(self): + response = self.client.get(self.list_url()) + assert not isinstance(response.data, list) + + +class TestCohortV2Ordering(CohortV2TestCase): + """Ordering follows the field-with-optional-minus convention.""" + + def setUp(self): + super().setUp() + create_cohort(self.course_key, "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"]] + 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"]] + assert names == sorted(names, reverse=True) + + def test_unsupported_ordering_field_is_rejected(self): + response = self.client.get(self.list_url(), {"ordering": "secret"}) + assert 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)) + assert response.status_code == status.HTTP_404_NOT_FOUND + for field in ERROR_ENVELOPE_FIELDS: + assert field in response.data + + def test_legacy_error_fields_are_absent(self): + response = self.client.get(self.detail_url(cohort_id=99999)) + 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( + self.list_url(), + {"name": self.cohort.name, "assignment_type": "manual"}, + format="json", + ) + assert 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", + ) + assert 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) + 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) + 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 cb9e277c71c2..0fa49cf8edf6 100644 --- a/openedx/core/djangoapps/course_groups/rest_api/urls.py +++ b/openedx/core/djangoapps/course_groups/rest_api/urls.py @@ -1,10 +1,11 @@ """ -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.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 urlpatterns = [ re_path( @@ -17,4 +18,34 @@ 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_KEY_PATTERN}/cohorts/$', + cohort_views.CohortViewSet.as_view({'get': 'list', 'post': 'create'}), + name='cohort_list', + ), + re_path( + 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_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_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_KEY_PATTERN}/cohort_settings/$', + cohort_views.CohortSettingsView.as_view(), + name='course_cohort_settings', + ), ] 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):