diff --git a/cms/djangoapps/contentstore/rest_api/v2/urls.py b/cms/djangoapps/contentstore/rest_api/v2/urls.py index 893069f89aec..044e68432ade 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v2/urls.py @@ -2,12 +2,21 @@ from django.conf import settings from django.urls import path, re_path +from rest_framework.routers import DefaultRouter from cms.djangoapps.contentstore.rest_api.v2.views import downstreams, home, utils app_name = "v2" -urlpatterns = [ +# ADR 0028: HomeCoursesViewSetV2 registered via DefaultRouter. +# Generates: GET home/courses/ → home-courses-list +router = DefaultRouter() +router.register(r'home/courses', home.HomeCoursesViewSetV2, basename='home-courses') + +urlpatterns = router.urls + [ + # DEPRECATED (ADR 0028): kept for backward compatibility. + # Will be removed after one named release. + # Use GET home/courses/ (router URL name: home-courses-list) instead. path( "home/courses", home.HomePageCoursesViewV2.as_view(), diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/home.py b/cms/djangoapps/contentstore/rest_api/v2/views/home.py index 9d37684bfd81..a19fda86ae3b 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/home.py @@ -1,31 +1,89 @@ """HomePageCoursesViewV2 APIView for getting content available to the logged in user.""" -import edx_api_doc_tools as apidocs from collections import OrderedDict -from rest_framework.response import Response + +from drf_spectacular.utils import ( + extend_schema, + OpenApiParameter, + OpenApiResponse, +) +from rest_framework import viewsets +from rest_framework.permissions import IsAuthenticated from rest_framework.request import Request +from rest_framework.response import Response from rest_framework.views import APIView -from rest_framework.pagination import PageNumberPagination - -from openedx.core.lib.api.view_utils import view_auth_classes +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.paginators import DefaultPagination from cms.djangoapps.contentstore.utils import get_course_context_v2 from cms.djangoapps.contentstore.rest_api.v2.serializers import CourseHomeTabSerializerV2 -class HomePageCoursesPaginator(PageNumberPagination): - """Custom paginator for the home page courses view version 2.""" - page_size_query_param = 'page_size' +def _query_param(name: str, description: str, deprecated: bool = False) -> OpenApiParameter: + """Build a string-typed, optional query parameter (preserves api-doc-tools behavior).""" + return OpenApiParameter( + name=name, + description=description, + required=False, + type=str, + location=OpenApiParameter.QUERY, + deprecated=deprecated, + ) + + +# ADR 0033 – sorting standardization: +# ``ordering`` is the DRF-standard parameter name and is the preferred form. +# ``order`` is kept as a deprecated alias; requests using it receive a +# ``Deprecation`` HTTP header. Removal is scheduled for the named release +# tracked in ADR 0033's deprecation window (see _LEGACY_ORDER_DEPRECATION_HEADER). +_HOME_COURSES_QUERY_PARAMETERS = [ + _query_param("org", "Query param to filter by course org"), + _query_param("search", "Query param to filter by course name, org, or number"), + _query_param("ordering", "Query param to order by course name, org, or number (DRF standard, ADR 0033)"), + _query_param( + "order", + "Deprecated alias for 'ordering' (ADR 0033). Use 'ordering' instead.", + deprecated=True, + ), + _query_param("active_only", "Query param to filter by active courses only"), + _query_param("archived_only", "Query param to filter by archived courses only"), + _query_param("page", "Query param to paginate the courses"), + _query_param("page_size", "Query param to set page size"), +] +_UNAUTHENTICATED_RESPONSE = OpenApiResponse(description="The requester is not authenticated.") + +# ADR 0033 BC strategy §2: deprecation warning header emitted when the legacy +# ``order`` query parameter is used in place of the standardized ``ordering``. +_LEGACY_ORDER_DEPRECATION_HEADER = ( + "Parameter 'order' is deprecated. Use 'ordering' instead. " + "Support will be removed in release ''." +) + + +def _maybe_set_legacy_order_deprecation_header(request: Request, response: Response) -> Response: + """ + Set the ADR 0033 ``Deprecation`` header on ``response`` when the request + used the legacy ``order`` query parameter. + + The header is emitted whenever ``order`` appears in the query string, even + if ``ordering`` was also supplied (in which case ``ordering`` wins, but the + caller should still be told that ``order`` is deprecated). + """ + if 'order' in request.query_params: + response['Deprecation'] = _LEGACY_ORDER_DEPRECATION_HEADER + return response + - def get_paginated_response(self, data): - """Return a paginated style `Response` object for the given output data.""" - return Response(OrderedDict([ - ('count', self.page.paginator.count), - ('num_pages', self.page.paginator.num_pages), - ('next', self.get_next_link()), - ('previous', self.get_previous_link()), - ('results', data), - ])) +class HomePageCoursesPaginator(DefaultPagination): + """ + ADR 0032 – standard pagination for the Studio home courses list (v2). + + Extends DefaultPagination with the full 7-field response envelope: + count, num_pages, current_page, start, next, previous, results. + Handles Python ``filter`` objects returned by get_course_context_v2. + """ + page_size_query_param = 'page_size' def paginate_queryset(self, queryset, request, view=None): """ @@ -42,52 +100,103 @@ def paginate_queryset(self, queryset, request, view=None): return super().paginate_queryset(queryset, request, view) -@view_auth_classes(is_authenticated=True) +class HomeCoursesViewSetV2(viewsets.ViewSet): + """ + ViewSet for course listing (v2). Registered via DefaultRouter (basename ``home-courses``). + + Router-generated URLs: + GET /api/contentstore/v2/home/courses/ → list + + ADR 0033 compliance notes: + - Sorting uses the DRF-standard ``ordering`` parameter; ``order`` is + retained as a deprecated alias (Phase 1 of the BC strategy) and + requests using it receive a ``Deprecation`` HTTP header. + - Full migration to ``django-filter``/``DjangoFilterBackend`` is tracked + as a follow-up: the underlying data source returned by + ``get_courses_accessible_to_user`` is a Python ``filter()`` wrapper + around a hybrid queryset, so attaching a ``FilterSet`` requires + reshaping ``_accessible_courses_summary_iter`` / + ``_accessible_courses_list_from_groups`` first. + """ + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated,) + serializer_class = CourseHomeTabSerializerV2 + + def get_serializer(self, *args, **kwargs): + """Instantiate and return the configured serializer class.""" + return self.serializer_class(*args, **kwargs) + + @extend_schema( + summary="List courses for the Studio home page (paginated)", + description=( + "Returns a paginated list of all courses available to the logged-in user, " + "with optional filtering and ordering." + ), + parameters=_HOME_COURSES_QUERY_PARAMETERS, + responses={ + 200: OpenApiResponse( + response=CourseHomeTabSerializerV2, + description="Paginated course list retrieved successfully.", + ), + 401: _UNAUTHENTICATED_RESPONSE, + }, + ) + def list(self, request: Request): + """ + Get a paginated list of all courses available to the logged-in user. + + **Example Request** + + GET /api/contentstore/v2/home/courses/ + GET /api/contentstore/v2/home/courses/?org=edX + GET /api/contentstore/v2/home/courses/?search=E2E + GET /api/contentstore/v2/home/courses/?ordering=-org + GET /api/contentstore/v2/home/courses/?order=-org # deprecated, use ?ordering= + GET /api/contentstore/v2/home/courses/?active_only=true + GET /api/contentstore/v2/home/courses/?archived_only=true + GET /api/contentstore/v2/home/courses/?page=2 + GET /api/contentstore/v2/home/courses/?page_size=20 + + **Response Values** + + If the request is successful, an HTTP 200 \"OK\" response is returned. + + The HTTP 200 response is paginated and contains ``count``, ``num_pages``, + ``next``, ``previous`` and ``results`` keys. ``results`` contains the + serialized course data. + """ + courses, in_process_course_actions = get_course_context_v2(request) + paginator = HomePageCoursesPaginator() + courses_page = paginator.paginate_queryset(courses, request, view=self) + serializer = self.get_serializer({ + 'courses': courses_page, + 'in_process_course_actions': in_process_course_actions, + }) + response = paginator.get_paginated_response(serializer.data) + return _maybe_set_legacy_order_deprecation_header(request, response) + + class HomePageCoursesViewV2(APIView): """View for getting all courses available to the logged in user.""" + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated,) + serializer_class = CourseHomeTabSerializerV2 - @apidocs.schema( - parameters=[ - apidocs.string_parameter( - "org", - apidocs.ParameterLocation.QUERY, - description="Query param to filter by course org", - ), - apidocs.string_parameter( - "search", - apidocs.ParameterLocation.QUERY, - description="Query param to filter by course name, org, or number", - ), - apidocs.string_parameter( - "order", - apidocs.ParameterLocation.QUERY, - description="Query param to order by course name, org, or number", - ), - apidocs.string_parameter( - "active_only", - apidocs.ParameterLocation.QUERY, - description="Query param to filter by active courses only", - ), - apidocs.string_parameter( - "archived_only", - apidocs.ParameterLocation.QUERY, - description="Query param to filter by archived courses only", - ), - apidocs.string_parameter( - "page", - apidocs.ParameterLocation.QUERY, - description="Query param to paginate the courses", - ), - apidocs.string_parameter( - "page_size", - apidocs.ParameterLocation.QUERY, - description="Query param to set page size", - ), - ], + @extend_schema( + operation_id="v2_home_courses_retrieve_deprecated", + summary="List courses for the Studio home page (deprecated)", + description=( + "Deprecated. Use GET /api/contentstore/v2/home/courses/ instead." + ), + parameters=_HOME_COURSES_QUERY_PARAMETERS, responses={ - 200: CourseHomeTabSerializerV2, - 401: "The requester is not authenticated.", + 200: OpenApiResponse( + response=CourseHomeTabSerializerV2, + description="Paginated course list retrieved successfully.", + ), + 401: _UNAUTHENTICATED_RESPONSE, }, + deprecated=True, ) def get(self, request: Request): """ @@ -98,38 +207,61 @@ def get(self, request: Request): GET /api/contentstore/v2/home/courses GET /api/contentstore/v2/home/courses?org=edX GET /api/contentstore/v2/home/courses?search=E2E - GET /api/contentstore/v2/home/courses?order=-org + GET /api/contentstore/v2/home/courses?ordering=-org + GET /api/contentstore/v2/home/courses?order=-org # deprecated, use ?ordering= GET /api/contentstore/v2/home/courses?active_only=true GET /api/contentstore/v2/home/courses?archived_only=true GET /api/contentstore/v2/home/courses?page=2 GET /api/contentstore/v2/home/courses?page_size=20 + **Pagination Parameters** + + - ``page`` (int): Page number to retrieve. Default is 1. + - ``page_size`` (int): Items per page. Default is 10, max is 100. + **Response Values** If the request is successful, an HTTP 200 "OK" response is returned. - The HTTP 200 response contains a single dict that contains keys that - are the course's home. + The HTTP 200 response contains the ADR 0032 standard pagination envelope. + + **Response Envelope (ADR 0032)** + + - ``count`` (int): Total number of courses matching the filters. + - ``num_pages`` (int): Total number of pages. + - ``current_page`` (int): The current page number. + - ``start`` (int): The 0-based index of the first course on this page. + - ``next`` (str|null): URL for the next page, or null if this is the last page. + - ``previous`` (str|null): URL for the previous page, or null if this is the first page. + - ``results`` (dict): Course data for the current page. **Example Response** ```json { - "courses": [ - { - "course_key": "course-v1:edX+E2E-101+course", - "display_name": "E2E Test Course", - "lms_link": "//localhost:18000/courses/course-v1:edX+E2E-101+course", - "cms_link": "//localhost:18010/course/course-v1:edX+E2E-101+course", - "number": "E2E-101", - "org": "edX", - "rerun_link": "/course_rerun/course-v1:edX+E2E-101+course", - "run": "course", - "url": "/course/course-v1:edX+E2E-101+course", - "is_active": true - }, - ], - "in_process_course_actions": [], + "count": 2, + "num_pages": 1, + "current_page": 1, + "start": 0, + "next": null, + "previous": null, + "results": { + "courses": [ + { + "course_key": "course-v1:edX+E2E-101+course", + "display_name": "E2E Test Course", + "lms_link": "//localhost:18000/courses/course-v1:edX+E2E-101+course", + "cms_link": "//localhost:18010/course/course-v1:edX+E2E-101+course", + "number": "E2E-101", + "org": "edX", + "rerun_link": "/course_rerun/course-v1:edX+E2E-101+course", + "run": "course", + "url": "/course/course-v1:edX+E2E-101+course", + "is_active": true + } + ], + "in_process_course_actions": [] + } } ``` """ @@ -140,8 +272,9 @@ def get(self, request: Request): self.request, view=self ) - serializer = CourseHomeTabSerializerV2({ + serializer = self.serializer_class({ 'courses': courses_page, 'in_process_course_actions': in_process_course_actions, }) - return paginator.get_paginated_response(serializer.data) + response = paginator.get_paginated_response(serializer.data) + return _maybe_set_legacy_order_deprecation_header(request, response) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py index 6a51610ac9f2..08c04c8dcc74 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py @@ -8,11 +8,15 @@ import ddt import pytz from django.conf import settings +from django.test import TestCase from django.urls import reverse from rest_framework import status +from rest_framework.test import APIClient +from rest_framework.test import APITestCase as DRFAPITestCase # noqa: E402 from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.contentstore.utils import reverse_course_url +from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory @@ -86,13 +90,15 @@ def test_home_page_response(self): ], "in_process_course_actions": [], } - expected_response = OrderedDict([ - ('count', 2), - ('num_pages', 1), - ('next', None), - ('previous', None), - ('results', expected_data), - ]) + expected_response = { + 'count': 2, + 'num_pages': 1, + 'current_page': 1, + 'start': 0, + 'next': None, + 'previous': None, + 'results': expected_data, + } self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertDictEqual(expected_response, response.data) @@ -298,3 +304,117 @@ def test_if_empty_list_of_courses_non_staff(self, query_param, value): self.assertEqual(len(response.data["results"]["courses"]), 0) self.assertEqual(response.status_code, status.HTTP_200_OK) + + +class TestHomePageCoursesPaginatorStructure(TestCase): + """ + ADR 0032 – structural checks for HomePageCoursesPaginator. + + Pure import-level check: no course data or MongoDB required. + Verifies the paginator inherits from DefaultPagination, satisfying the + ADR 0032 requirement that all list endpoints use the standard paginator. + """ + + def test_paginator_is_defaultpagination_subclass(self): + """HomePageCoursesPaginator must subclass DefaultPagination (not PageNumberPagination directly).""" + from edx_rest_framework_extensions.paginators import DefaultPagination + from cms.djangoapps.contentstore.rest_api.v2.views.home import HomePageCoursesPaginator + self.assertTrue( + issubclass(HomePageCoursesPaginator, DefaultPagination), + "ADR 0032: HomePageCoursesPaginator must subclass DefaultPagination", + ) + + +class TestHomePageCoursesViewV2PaginationEnvelope(DRFAPITestCase): + """ + ADR 0032 – pagination envelope regression tests for HomePageCoursesViewV2. + + Uses a plain staff user (no CourseFactory / MongoDB) so that the test can + run in any environment. The endpoint returns an empty course list for a + user with no courses, which is sufficient to verify the 7-field envelope. + + Verifies that the GET /api/contentstore/v2/home/courses endpoint returns + the full 7-field ADR 0032 response envelope: count, num_pages, current_page, + start, next, previous, results. + """ + + def setUp(self): + super().setUp() + self.user = UserFactory.create(is_staff=True) + self.client.force_authenticate(user=self.user) + self.url = reverse("cms.djangoapps.contentstore:v2:courses") + + def test_response_includes_full_envelope(self): + """All 7 ADR 0032 envelope fields must be present in every paginated response.""" + response = self.client.get(self.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, f"ADR 0032: missing envelope field '{field}'") + + def test_current_page_is_one_on_first_page(self): + """current_page must equal 1 when requesting the first page.""" + response = self.client.get(self.url, {'page': 1}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data['current_page'], 1) + + def test_start_is_zero_on_first_page(self): + """start must be 0 on the first page (0-based index of the first item).""" + response = self.client.get(self.url, {'page': 1}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data['start'], 0) + + def test_results_contains_courses_key(self): + """results must be a dict containing the 'courses' key.""" + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertIsInstance(response.data['results'], dict) + self.assertIn('courses', response.data['results']) + + +class HomePageCoursesViewV2PermissionsTest(TestCase): + """ + ADR 0026 – permission regression tests for HomePageCoursesViewV2. + + Verifies that the explicit permission_classes = (IsAuthenticated,) enforces + the same access rules previously set by the @view_auth_classes(is_authenticated=True) + decorator. + """ + + def setUp(self): + super().setUp() + self.client = APIClient() + self.url = reverse("cms.djangoapps.contentstore:v2:courses") + self.user = UserFactory.create() + self.staff_user = UserFactory.create(is_staff=True) + + def test_unauthenticated_request_returns_401(self): + """ + Unauthenticated request (no credentials) must be rejected with 401. + + Before ADR 0026: enforced by @view_auth_classes(is_authenticated=True). + After ADR 0026: enforced by permission_classes = (IsAuthenticated,). + """ + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) + + def test_authenticated_user_gets_200(self): + """ + Any authenticated user (not necessarily staff) must receive 200. + + HomePageCoursesViewV2 only requires authentication — no staff role needed. + The view returns an empty course list for users with no assigned courses. + """ + self.client.force_authenticate(user=self.user) + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_staff_user_gets_200(self): + """Staff user must also receive 200 (staff is a superset of authenticated).""" + self.client.force_authenticate(user=self.staff_user) + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_post_by_unauthenticated_returns_401(self): + """Non-GET methods also enforce authentication — POST without credentials is 401.""" + response = self.client.post(self.url, data={}) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home_viewset.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home_viewset.py new file mode 100644 index 000000000000..3a94be855c55 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home_viewset.py @@ -0,0 +1,166 @@ +""" +Unit tests for HomeCoursesViewSetV2 (ADR 0028 ViewSet migration). + +All service-layer calls are mocked so these tests run without MongoDB. +""" +from unittest.mock import MagicMock, patch + +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient, APITestCase + +from cms.djangoapps.contentstore.rest_api.v2.views.home import HomeCoursesViewSetV2 +from common.djangoapps.student.tests.factories import UserFactory + +MOCK_GET_COURSE_CONTEXT_V2 = ( + 'cms.djangoapps.contentstore.rest_api.v2.views.home.get_course_context_v2' +) + + +class TestHomeCoursesViewSetV2Permissions(APITestCase): + """ + ADR 0028 – permission regression tests for HomeCoursesViewSetV2. + + URL: GET /api/contentstore/v2/home/courses/ (router name: home-courses-list) + """ + + def setUp(self): + super().setUp() + self.client = APIClient() + self.url = reverse("cms.djangoapps.contentstore:v2:home-courses-list") + + def test_unauthenticated_list_returns_401(self): + """Unauthenticated request must be rejected with 401.""" + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) + + def test_authenticated_user_gets_200(self): + """Any authenticated user gets 200 — only IsAuthenticated is required.""" + user = UserFactory.create() + self.client.force_authenticate(user=user) + with patch(MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_staff_user_gets_200(self): + """Staff user also gets 200 (superset of authenticated).""" + user = UserFactory.create(is_staff=True) + self.client.force_authenticate(user=user) + with patch(MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + +class TestHomeCoursesViewSetV2Actions(APITestCase): + """ + ADR 0028 – action tests for HomeCoursesViewSetV2.list. + + Service layer (get_course_context_v2) and serializer are mocked to keep + these tests free of MongoDB and CourseOverview DB queries. + """ + + def setUp(self): + super().setUp() + self.client = APIClient() + self.user = UserFactory.create(is_staff=True) + self.client.force_authenticate(user=self.user) + self.url = reverse("cms.djangoapps.contentstore:v2:home-courses-list") + + @patch.object(HomeCoursesViewSetV2, 'get_serializer') + @patch(MOCK_GET_COURSE_CONTEXT_V2) + def test_list_calls_get_course_context_v2(self, mock_context, mock_get_ser): + """GET /home/courses/ calls get_course_context_v2 exactly once and returns 200.""" + mock_context.return_value = ([], []) + mock_get_ser.return_value.data = { + 'courses': [], + 'in_process_course_actions': [], + } + + response = self.client.get(self.url) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + mock_context.assert_called_once() + # Response must be paginated (count / num_pages / next / previous / results) + self.assertIn('count', response.data) + self.assertIn('results', response.data) + + +class TestHomeCoursesViewSetV2OrderingDeprecation(APITestCase): + """ + ADR 0033 – sorting standardization tests. + + Verify that: + * The new ``ordering`` parameter works and does NOT trigger the + ``Deprecation`` header. + * The legacy ``order`` parameter is still accepted (backward compat, + ADR 0033 BC strategy §1) but DOES trigger the ``Deprecation`` header + (BC strategy §2). + * When both are sent, ``ordering`` wins (its value is forwarded into + ``get_query_params_if_present``) and the ``Deprecation`` header is + still emitted because ``order`` was present in the query string. + * When neither is sent, no ``Deprecation`` header is emitted. + """ + + EXPECTED_DEPRECATION_HEADER = ( + "Parameter 'order' is deprecated. Use 'ordering' instead. " + "Support will be removed in release ''." + ) + + def setUp(self): + super().setUp() + self.client = APIClient() + self.user = UserFactory.create(is_staff=True) + self.client.force_authenticate(user=self.user) + self.url = reverse("cms.djangoapps.contentstore:v2:home-courses-list") + + def _patched_get_course_context(self, captured): + """ + Returns a MOCK_GET_COURSE_CONTEXT_V2 patch whose side_effect captures + the request so the test can assert which ordering value reached the + service layer via ``get_query_params_if_present``. + """ + def _capture(request): + captured['request'] = request + return ([], []) + return patch(MOCK_GET_COURSE_CONTEXT_V2, side_effect=_capture) + + def test_new_ordering_param_does_not_emit_deprecation_header(self): + """``?ordering=display_name`` returns 200 and no ``Deprecation`` header.""" + with patch(MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get(self.url, {'ordering': 'display_name'}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertNotIn('Deprecation', response.headers) + + def test_legacy_order_param_emits_deprecation_header(self): + """``?order=display_name`` returns 200 AND emits the ADR 0033 header.""" + with patch(MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get(self.url, {'order': 'display_name'}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.headers.get('Deprecation'), self.EXPECTED_DEPRECATION_HEADER) + + def test_ordering_wins_when_both_sent_but_header_still_emitted(self): + """ + When both params are present, ``get_query_params_if_present`` must + forward the value of ``ordering`` (not ``order``) to the service + layer, but the ``Deprecation`` header must still be emitted because + the request *contained* the deprecated param. + """ + captured = {} + with self._patched_get_course_context(captured): + response = self.client.get( + self.url, + {'ordering': 'display_name', 'order': 'org'}, + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.headers.get('Deprecation'), self.EXPECTED_DEPRECATION_HEADER) + # Confirm ordering wins inside get_query_params_if_present + from cms.djangoapps.contentstore.views.course import get_query_params_if_present + _search, order_resolved, _active, _archived = get_query_params_if_present(captured['request']) + self.assertEqual(order_resolved, 'display_name') + + def test_no_ordering_param_no_deprecation_header(self): + """Plain ``GET /home/courses/`` does not emit the ``Deprecation`` header.""" + with patch(MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertNotIn('Deprecation', response.headers) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index bf2cd9cb83d6..a8f1ae758231 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -426,22 +426,27 @@ def get_query_params_if_present(request): """ Returns the query params from request if present. + ADR 0033 – sorting standardization: prefer the DRF-standard ``ordering`` + parameter; ``order`` is accepted as a deprecated alias for backward + compatibility. When both are present, ``ordering`` wins. + Arguments: request: the request object Returns: search_query (str): any string used to filter Course Overviews based on visible fields. - order (str): any string used to order Course Overviews. + order (str): any string used to order Course Overviews. Sourced from + ``ordering`` (preferred) or ``order`` (deprecated alias). active_only (str): if not None, this value will limit the courses returned to active courses. The default value is None. archived_only (str): if not None, this value will limit the courses returned to archived courses. The default value is None. """ - allowed_query_params = ['search', 'order', 'active_only', 'archived_only'] + allowed_query_params = ['search', 'ordering', 'order', 'active_only', 'archived_only'] if not any(param in request.GET for param in allowed_query_params): return None, None, None, None search_query = request.GET.get('search') - order = request.GET.get('order') + order = request.GET.get('ordering') or request.GET.get('order') active_only = get_bool_param(request, 'active_only', None) archived_only = get_bool_param(request, 'archived_only', None) return search_query, order, active_only, archived_only diff --git a/docs/decisions/0025-standardize-serializer-usage.rst b/docs/decisions/0025-standardize-serializer-usage.rst new file mode 100644 index 000000000000..780e7ed22ecc --- /dev/null +++ b/docs/decisions/0025-standardize-serializer-usage.rst @@ -0,0 +1,113 @@ +Standardize Serializer Usage Across APIs +======================================== + +:Status: Proposed +:Date: 2026-03-09 +:Deciders: API Working Group +:Technical Story: Open edX REST API Standards - Serializer standardization for consistency + +Context +------- + +Many Open edX platform API endpoints manually construct JSON responses using Python dictionaries instead of Django REST Framework (DRF) serializers. This leads to inconsistent schema responses, makes validation errors harder to manage, and creates unpredictable formats that AI and third-party systems struggle with. + +Decision +-------- + +We will standardize all Open edX REST APIs to use **DRF serializers** for request and response handling. + +Implementation requirements: + +* All API views MUST define explicit serializers for request and response handling. +* Replace manual JSON construction with serializer-based responses. +* Use serializers for both input validation and output formatting. +* Ensure serializers are properly documented with field descriptions and validation rules. +* Maintain backward compatibility for all APIs during migration. While the goal is fully compatible DRF serializers, if that is not possible and we must make a backwards incompatible change, that change MUST be handled by creating a new version of the API and transitioning to that API using the deprecation process. + +Relevance in edx-platform +------------------------- + +Current patterns that should be migrated: + +* **Certificates API** (``/api/certificates/v0/``) constructs JSON manually with nested dictionaries. +* **Enrollment API** endpoints manually build response objects without serializers. +* **Course API** views use hand-coded JSON responses instead of structured serializers. + +Code example (target serializer usage) +-------------------------------------- + +**Example serializer and APIView using DRF best practices:** + +.. code-block:: python + + # serializers.py + from rest_framework import serializers + + class CertificateSerializer(serializers.Serializer): + username = serializers.CharField( + help_text="The username of the certificate holder" + ) + course_id = serializers.CharField( + help_text="The course identifier" + ) + status = serializers.CharField( + help_text="The certificate status (e.g., downloadable, generating)" + ) + grade = serializers.FloatField( + help_text="The final grade achieved" + ) + + # views.py + from rest_framework.views import APIView + from rest_framework.response import Response + from rest_framework import status + + class CertificateAPIView(APIView): + def get(self, request): + data = { + "username": "john_doe", + "course_id": "course-v1:edX+DemoX+1T2024", + "status": "downloadable", + "grade": 0.95, + } + serializer = CertificateSerializer(data) + return Response(serializer.data, status=status.HTTP_200_OK) + +Consequences +------------ + +Positive +~~~~~~~~ + +* Simplifies validation and ensures consistent response contracts. +* Improves AI compatibility through predictable data structures. +* Enables automatic schema generation and documentation. +* Reduces code duplication and maintenance overhead. + +Negative / Trade-offs +~~~~~~~~~~~~~~~~~~~~~ + +* Requires refactoring existing endpoints that manually construct JSON. +* Initial development overhead for creating comprehensive serializers. +* May require updates to existing client code that expects legacy formats. + +Alternatives Considered +----------------------- + +* **Keep manual JSON construction**: rejected due to inconsistency and maintenance burden. +* **Use DRF defaults only**: rejected because explicit serializers provide better validation and documentation. +* **Use newer ways of managing API responses such as dataclasses or pydantic**: rejected due to complexity and unknowns in transitioning from two existing patterns (manual JSON and DRF serializers) to a third approach. While these python libraries offer better ergonomics, migration would require checking nested serializers, complex validation, and ModelSerializer-heavy endpoints. To move to some new format, we would want to prevent using the basic DRF Serializers any more than we do right now, but preventing new DRF serializers via linting is more complex than anticipated. This work can be revisited in the future once the platform is a bit more consistent. + +Rollout Plan +------------ + +1. Audit existing endpoints to identify those using manual JSON construction. +2. Create a library of common serializers for shared data structures. +3. Migrate high-impact endpoints first (certificates, enrollment, courses). +4. Update tests to validate serializer-based responses. +5. Update API documentation to reflect new serializer-based contracts. + +References +---------- + +* Open edX REST API Standards: "Serializer Usage" recommendations for API consistency.