From 2ff8aab5b96cf6d1656c14f4773122f18a4897f3 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed <68893403+taimoor-ahmed-1@users.noreply.github.com> Date: Tue, 2 Jun 2026 11:26:32 +0500 Subject: [PATCH 01/13] feat: Standardize HomePageCoursesView (#38366) Standardize HomePageCoursesView Co-authored-by: Taimoor Ahmed Co-authored-by: Claude Sonnet 4.6 --- .../contentstore/rest_api/v1/urls.py | 10 +- .../rest_api/v1/views/__init__.py | 3 +- .../contentstore/rest_api/v1/views/home.py | 150 +++++++++++++++++- .../rest_api/v1/views/tests/test_home.py | 57 ++++++- .../v1/views/tests/test_home_viewset.py | 128 +++++++++++++++ 5 files changed, 341 insertions(+), 7 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home_viewset.py diff --git a/cms/djangoapps/contentstore/rest_api/v1/urls.py b/cms/djangoapps/contentstore/rest_api/v1/urls.py index 685a81d778ce..da97bcab6a7a 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/urls.py @@ -2,6 +2,7 @@ from django.conf import settings from django.urls import path, re_path +from rest_framework.routers import DefaultRouter from openedx.core.constants import COURSE_ID_PATTERN @@ -23,6 +24,7 @@ HomePageCoursesView, HomePageLibrariesView, HomePageView, + HomeViewSet, ProctoredExamSettingsView, ProctoringErrorsView, VideoDownloadView, @@ -34,7 +36,13 @@ VIDEO_ID_PATTERN = r'(?P[-\w]+)' -urlpatterns = [ +# ADR 0028: ViewSets registered via DefaultRouter. +router = DefaultRouter() +router.register(r'home', HomeViewSet, basename='home') + +urlpatterns = router.urls + [ + # DEPRECATED (ADR 0028): Use HomeViewSet instead (GET home/, home/courses/, home/libraries/). + # Kept as backward-compatible aliases. Remove after one named release. path( 'home', HomePageView.as_view(), diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py index 7654c9e0befc..4bc061c1f6a4 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py @@ -1,6 +1,7 @@ """ Views for v1 contentstore API. """ + from .certificates import CourseCertificatesView # noqa: F401 from .course_details import CourseDetailsView # noqa: F401 from .course_index import ContainerChildrenView, CourseIndexView # noqa: F401 @@ -10,7 +11,7 @@ from .grading import CourseGradingView # noqa: F401 from .group_configurations import CourseGroupConfigurationsView # noqa: F401 from .help_urls import HelpUrlsView # noqa: F401 -from .home import HomePageCoursesView, HomePageLibrariesView, HomePageView # noqa: F401 +from .home import HomePageCoursesView, HomePageLibrariesView, HomePageView, HomeViewSet # noqa: F401 from .proctoring import ProctoredExamSettingsView, ProctoringErrorsView # noqa: F401 from .settings import CourseSettingsView # noqa: F401 from .textbooks import CourseTextbooksView # noqa: F401 diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/home.py b/cms/djangoapps/contentstore/rest_api/v1/views/home.py index 95723020c11f..9dc34744a7fa 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/home.py @@ -2,7 +2,12 @@ import edx_api_doc_tools as apidocs from django.conf import settings +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from organizations import api as org_api +from rest_framework import viewsets +from rest_framework.decorators import action +from rest_framework.permissions import IsAuthenticated from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView @@ -13,6 +18,145 @@ from ..serializers import CourseHomeTabSerializer, LibraryTabSerializer, StudioHomeSerializer +# ADR 0028 – consolidated from HomePageView, HomePageCoursesView, HomePageLibrariesView +class HomeViewSet(viewsets.ViewSet): + """ + ViewSet for the Studio home page. Registered via DefaultRouter (basename ``home``). + + Router-generated URLs: + GET /api/contentstore/v1/home/ → list (aggregated home context) + GET /api/contentstore/v1/home/courses/ → courses (course list only) + GET /api/contentstore/v1/home/libraries/→ libraries (library list only) + + ADR 0025 compliance notes: + - Three different serializers are returned by ``get_serializer_class()`` depending + on the action. ``serializer_class`` is set to the default (``StudioHomeSerializer``). + - All response formatting is handled by the respective serializer class. + """ + + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated,) # ADR 0026 + serializer_class = StudioHomeSerializer # default; overridden by get_serializer_class() + + def get_serializer_class(self): + """Return the appropriate serializer class for the current action.""" + if self.action == 'courses': + return CourseHomeTabSerializer + if self.action == 'libraries': + return LibraryTabSerializer + return StudioHomeSerializer + + def get_serializer(self, *args, **kwargs): + """Return a serializer instance using the action-appropriate class.""" + return self.get_serializer_class()(*args, **kwargs) + + @apidocs.schema( + parameters=[ + apidocs.string_parameter( + "org", + apidocs.ParameterLocation.QUERY, + description="Query param to filter by course org", + )], + responses={ + 200: StudioHomeSerializer, + 401: "The requester is not authenticated.", + }, + ) + def list(self, request: Request): + """ + Get an object containing all courses and libraries on home page. + + **Example Request** + + GET /api/contentstore/v1/home/ + """ + home_context = get_home_context(request, True) + home_context.update({ + # 'allow_to_create_new_org' is actually about auto-creating organizations + # (e.g. when creating a course or library), so we add an additional test. + 'allow_to_create_new_org': ( + home_context['can_create_organizations'] and + org_api.is_autocreate_enabled() + ), + 'studio_name': settings.STUDIO_NAME, + 'studio_short_name': settings.STUDIO_SHORT_NAME, + 'studio_request_email': settings.FEATURES.get('STUDIO_REQUEST_EMAIL', ''), + 'tech_support_email': settings.TECH_SUPPORT_EMAIL, + 'platform_name': settings.PLATFORM_NAME, + 'user_is_active': request.user.is_active, + }) + serializer = self.get_serializer(home_context) + return Response(serializer.data) + + @apidocs.schema( + parameters=[ + apidocs.string_parameter( + "org", + apidocs.ParameterLocation.QUERY, + description="Query param to filter by course org", + )], + responses={ + 200: CourseHomeTabSerializer, + 401: "The requester is not authenticated.", + }, + ) + @action(detail=False, methods=['get'], url_path='courses', url_name='courses') + def courses(self, request: Request): + """ + Get an object containing all courses. + + **Example Request** + + GET /api/contentstore/v1/home/courses/ + """ + active_courses, archived_courses, in_process_course_actions = get_course_context(request) + courses_context = { + "courses": active_courses, + "archived_courses": archived_courses, + "in_process_course_actions": in_process_course_actions, + } + serializer = self.get_serializer(courses_context) + return Response(serializer.data) + + @apidocs.schema( + parameters=[ + apidocs.string_parameter( + "org", + apidocs.ParameterLocation.QUERY, + description="Query param to filter by course org", + ), + apidocs.query_parameter( + "is_migrated", + bool, + description=( + "Query param to filter by migrated status of library." + " If present (true or false), it will filter by migration status" + " else it will return all legacy libraries." + ), + ) + ], + responses={ + 200: LibraryTabSerializer, + 401: "The requester is not authenticated.", + }, + ) + @action(detail=False, methods=['get'], url_path='libraries', url_name='libraries') + def libraries(self, request: Request): + """ + Get an object containing all libraries on home page. + + **Example Request** + + GET /api/contentstore/v1/home/libraries/ + """ + library_context = get_library_context(request) + serializer = self.get_serializer(library_context) + return Response(serializer.data) + + +# DEPRECATED (ADR 0028): Use HomeViewSet instead. +# Will be removed after one named release. +# Use GET home/, home/courses/, home/libraries/ instead. @view_auth_classes(is_authenticated=True) class HomePageView(APIView): """ @@ -99,11 +243,13 @@ def get(self, request: Request): return Response(serializer.data) -@view_auth_classes(is_authenticated=True) class HomePageCoursesView(APIView): """ View for getting all courses and libraries available to the logged in user. """ + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated,) + serializer_class = CourseHomeTabSerializer @apidocs.schema( parameters=[ apidocs.string_parameter( @@ -170,7 +316,7 @@ def get(self, request: Request): "archived_courses": archived_courses, "in_process_course_actions": in_process_course_actions, } - serializer = CourseHomeTabSerializer(courses_context) + serializer = self.serializer_class(courses_context) return Response(serializer.data) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py index ccdd906609a4..d0a16c095eb2 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py @@ -7,17 +7,19 @@ import ddt import pytz from django.conf import settings -from django.test import override_settings +from django.test import TestCase, override_settings from django.urls import reverse from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_content import api as content_api from organizations.tests.factories import OrganizationFactory from rest_framework import status +from rest_framework.test import APIClient from cms.djangoapps.contentstore.tests.test_libraries import LibraryTestCase from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.modulestore_migrator import api as migrator_api from cms.djangoapps.modulestore_migrator.data import CompositionLevel, RepeatHandlingStrategy +from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.content_libraries import api as lib_api @@ -400,5 +402,54 @@ def test_home_page_libraries_response(self): ], } - self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 - self.assertDictEqual(expected_response, response.json()) # noqa: PT009 + assert response.status_code == status.HTTP_200_OK + assert response.json() == expected_response + + +class HomePageCoursesViewPermissionsTest(TestCase): + """ + ADR 0026 – permission regression tests for HomePageCoursesView. + + 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:v1: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) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_authenticated_user_gets_200(self): + """ + Any authenticated user (not necessarily staff) must receive 200. + + HomePageCoursesView 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) + assert 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) + assert 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={}) + assert response.status_code == status.HTTP_401_UNAUTHORIZED diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home_viewset.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home_viewset.py new file mode 100644 index 000000000000..d0e309f5377a --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home_viewset.py @@ -0,0 +1,128 @@ +""" +Unit tests for HomeViewSet (ADR 0028). + +MongoDB-free: all service-layer calls are mocked. + +patch.object is used for the ViewSet's get_serializer() method because: + - get_serializer_class() returns a *different* serializer per action. + - Each serializer (StudioHomeSerializer, CourseHomeTabSerializer, + LibraryTabSerializer) has many required fields that would be painful to + satisfy with synthetic data. + - Patching get_serializer() lets us focus on routing + service-call assertions + without re-testing serializer logic (covered in serializer unit tests). +""" +from unittest.mock import patch + +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APITestCase + +from cms.djangoapps.contentstore.rest_api.v1.views.home import HomeViewSet +from common.djangoapps.student.tests.factories import UserFactory + +# ------------------------------------------------------------------ +# Mock target paths +# ------------------------------------------------------------------ +MOCK_GET_HOME_CONTEXT = ( + 'cms.djangoapps.contentstore.rest_api.v1.views.home.get_home_context' +) +MOCK_GET_COURSE_CONTEXT = ( + 'cms.djangoapps.contentstore.rest_api.v1.views.home.get_course_context' +) +MOCK_GET_LIBRARY_CONTEXT = ( + 'cms.djangoapps.contentstore.rest_api.v1.views.home.get_library_context' +) +MOCK_ORG_API = ( + 'cms.djangoapps.contentstore.rest_api.v1.views.home.org_api' +) + + +class TestHomeViewSetPermissions(APITestCase): + """ + ADR 0028 – permission regression tests for HomeViewSet. + + Verifies that IsAuthenticated enforces the same access rules previously + provided by @view_auth_classes(is_authenticated=True) on the deprecated views. + """ + + def test_unauthenticated_list_returns_401(self): + """Unauthenticated GET /home/ must return 401.""" + url = reverse('cms.djangoapps.contentstore:v1:home-list') + response = self.client.get(url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_unauthenticated_courses_returns_401(self): + """Unauthenticated GET /home/courses/ must return 401.""" + url = reverse('cms.djangoapps.contentstore:v1:home-courses') + response = self.client.get(url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_unauthenticated_libraries_returns_401(self): + """Unauthenticated GET /home/libraries/ must return 401.""" + url = reverse('cms.djangoapps.contentstore:v1:home-libraries') + response = self.client.get(url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + +class TestHomeViewSetActions(APITestCase): + """ + Action tests for HomeViewSet (list, courses, libraries). + + Any authenticated user can access these endpoints — no course-staff role + is required — so a plain (non-staff) factory user is sufficient. + """ + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.client.force_authenticate(user=self.user) + + # ------------------------------------------------------------------ + # list → GET /home/ + # ------------------------------------------------------------------ + + @patch.object(HomeViewSet, 'get_serializer') + @patch(MOCK_ORG_API) + @patch(MOCK_GET_HOME_CONTEXT) + def test_list_calls_get_home_context(self, mock_home, mock_org, mock_get_ser): + """GET /home/ calls get_home_context() and returns 200.""" + mock_home.return_value = {'can_create_organizations': True} + mock_org.is_autocreate_enabled.return_value = True + mock_get_ser.return_value.data = {'studio_name': 'Studio'} + + response = self.client.get(reverse('cms.djangoapps.contentstore:v1:home-list')) + + assert response.status_code == status.HTTP_200_OK + mock_home.assert_called_once() + + # ------------------------------------------------------------------ + # courses → GET /home/courses/ + # ------------------------------------------------------------------ + + @patch.object(HomeViewSet, 'get_serializer') + @patch(MOCK_GET_COURSE_CONTEXT) + def test_courses_calls_get_course_context(self, mock_courses, mock_get_ser): + """GET /home/courses/ calls get_course_context() and returns 200.""" + mock_courses.return_value = ([], [], []) + mock_get_ser.return_value.data = {'courses': [], 'archived_courses': [], 'in_process_course_actions': []} + + response = self.client.get(reverse('cms.djangoapps.contentstore:v1:home-courses')) + + assert response.status_code == status.HTTP_200_OK + mock_courses.assert_called_once() + + # ------------------------------------------------------------------ + # libraries → GET /home/libraries/ + # ------------------------------------------------------------------ + + @patch.object(HomeViewSet, 'get_serializer') + @patch(MOCK_GET_LIBRARY_CONTEXT) + def test_libraries_calls_get_library_context(self, mock_libs, mock_get_ser): + """GET /home/libraries/ calls get_library_context() and returns 200.""" + mock_libs.return_value = {'libraries': []} + mock_get_ser.return_value.data = {'libraries': []} + + response = self.client.get(reverse('cms.djangoapps.contentstore:v1:home-libraries')) + + assert response.status_code == status.HTTP_200_OK + mock_libs.assert_called_once() From a3b316d1fb79431bff1affcbd72a3f4696b2bcf1 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed <68893403+taimoor-ahmed-1@users.noreply.github.com> Date: Tue, 2 Jun 2026 15:28:00 +0500 Subject: [PATCH 02/13] revert: Revert "[FC-0018] feat: Standardize HomePageCoursesView (#38366)" (#38682) This reverts commit 1a60c23d9b1eaf3741d5be59129c07e0f821241e. --- .../contentstore/rest_api/v1/urls.py | 10 +- .../rest_api/v1/views/__init__.py | 3 +- .../contentstore/rest_api/v1/views/home.py | 150 +----------------- .../rest_api/v1/views/tests/test_home.py | 57 +------ .../v1/views/tests/test_home_viewset.py | 128 --------------- 5 files changed, 7 insertions(+), 341 deletions(-) delete mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home_viewset.py diff --git a/cms/djangoapps/contentstore/rest_api/v1/urls.py b/cms/djangoapps/contentstore/rest_api/v1/urls.py index da97bcab6a7a..685a81d778ce 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/urls.py @@ -2,7 +2,6 @@ from django.conf import settings from django.urls import path, re_path -from rest_framework.routers import DefaultRouter from openedx.core.constants import COURSE_ID_PATTERN @@ -24,7 +23,6 @@ HomePageCoursesView, HomePageLibrariesView, HomePageView, - HomeViewSet, ProctoredExamSettingsView, ProctoringErrorsView, VideoDownloadView, @@ -36,13 +34,7 @@ VIDEO_ID_PATTERN = r'(?P[-\w]+)' -# ADR 0028: ViewSets registered via DefaultRouter. -router = DefaultRouter() -router.register(r'home', HomeViewSet, basename='home') - -urlpatterns = router.urls + [ - # DEPRECATED (ADR 0028): Use HomeViewSet instead (GET home/, home/courses/, home/libraries/). - # Kept as backward-compatible aliases. Remove after one named release. +urlpatterns = [ path( 'home', HomePageView.as_view(), diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py index 4bc061c1f6a4..7654c9e0befc 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py @@ -1,7 +1,6 @@ """ Views for v1 contentstore API. """ - from .certificates import CourseCertificatesView # noqa: F401 from .course_details import CourseDetailsView # noqa: F401 from .course_index import ContainerChildrenView, CourseIndexView # noqa: F401 @@ -11,7 +10,7 @@ from .grading import CourseGradingView # noqa: F401 from .group_configurations import CourseGroupConfigurationsView # noqa: F401 from .help_urls import HelpUrlsView # noqa: F401 -from .home import HomePageCoursesView, HomePageLibrariesView, HomePageView, HomeViewSet # noqa: F401 +from .home import HomePageCoursesView, HomePageLibrariesView, HomePageView # noqa: F401 from .proctoring import ProctoredExamSettingsView, ProctoringErrorsView # noqa: F401 from .settings import CourseSettingsView # noqa: F401 from .textbooks import CourseTextbooksView # noqa: F401 diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/home.py b/cms/djangoapps/contentstore/rest_api/v1/views/home.py index 9dc34744a7fa..95723020c11f 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/home.py @@ -2,12 +2,7 @@ import edx_api_doc_tools as apidocs from django.conf import settings -from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication -from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from organizations import api as org_api -from rest_framework import viewsets -from rest_framework.decorators import action -from rest_framework.permissions import IsAuthenticated from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView @@ -18,145 +13,6 @@ from ..serializers import CourseHomeTabSerializer, LibraryTabSerializer, StudioHomeSerializer -# ADR 0028 – consolidated from HomePageView, HomePageCoursesView, HomePageLibrariesView -class HomeViewSet(viewsets.ViewSet): - """ - ViewSet for the Studio home page. Registered via DefaultRouter (basename ``home``). - - Router-generated URLs: - GET /api/contentstore/v1/home/ → list (aggregated home context) - GET /api/contentstore/v1/home/courses/ → courses (course list only) - GET /api/contentstore/v1/home/libraries/→ libraries (library list only) - - ADR 0025 compliance notes: - - Three different serializers are returned by ``get_serializer_class()`` depending - on the action. ``serializer_class`` is set to the default (``StudioHomeSerializer``). - - All response formatting is handled by the respective serializer class. - """ - - authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) - permission_classes = (IsAuthenticated,) # ADR 0026 - serializer_class = StudioHomeSerializer # default; overridden by get_serializer_class() - - def get_serializer_class(self): - """Return the appropriate serializer class for the current action.""" - if self.action == 'courses': - return CourseHomeTabSerializer - if self.action == 'libraries': - return LibraryTabSerializer - return StudioHomeSerializer - - def get_serializer(self, *args, **kwargs): - """Return a serializer instance using the action-appropriate class.""" - return self.get_serializer_class()(*args, **kwargs) - - @apidocs.schema( - parameters=[ - apidocs.string_parameter( - "org", - apidocs.ParameterLocation.QUERY, - description="Query param to filter by course org", - )], - responses={ - 200: StudioHomeSerializer, - 401: "The requester is not authenticated.", - }, - ) - def list(self, request: Request): - """ - Get an object containing all courses and libraries on home page. - - **Example Request** - - GET /api/contentstore/v1/home/ - """ - home_context = get_home_context(request, True) - home_context.update({ - # 'allow_to_create_new_org' is actually about auto-creating organizations - # (e.g. when creating a course or library), so we add an additional test. - 'allow_to_create_new_org': ( - home_context['can_create_organizations'] and - org_api.is_autocreate_enabled() - ), - 'studio_name': settings.STUDIO_NAME, - 'studio_short_name': settings.STUDIO_SHORT_NAME, - 'studio_request_email': settings.FEATURES.get('STUDIO_REQUEST_EMAIL', ''), - 'tech_support_email': settings.TECH_SUPPORT_EMAIL, - 'platform_name': settings.PLATFORM_NAME, - 'user_is_active': request.user.is_active, - }) - serializer = self.get_serializer(home_context) - return Response(serializer.data) - - @apidocs.schema( - parameters=[ - apidocs.string_parameter( - "org", - apidocs.ParameterLocation.QUERY, - description="Query param to filter by course org", - )], - responses={ - 200: CourseHomeTabSerializer, - 401: "The requester is not authenticated.", - }, - ) - @action(detail=False, methods=['get'], url_path='courses', url_name='courses') - def courses(self, request: Request): - """ - Get an object containing all courses. - - **Example Request** - - GET /api/contentstore/v1/home/courses/ - """ - active_courses, archived_courses, in_process_course_actions = get_course_context(request) - courses_context = { - "courses": active_courses, - "archived_courses": archived_courses, - "in_process_course_actions": in_process_course_actions, - } - serializer = self.get_serializer(courses_context) - return Response(serializer.data) - - @apidocs.schema( - parameters=[ - apidocs.string_parameter( - "org", - apidocs.ParameterLocation.QUERY, - description="Query param to filter by course org", - ), - apidocs.query_parameter( - "is_migrated", - bool, - description=( - "Query param to filter by migrated status of library." - " If present (true or false), it will filter by migration status" - " else it will return all legacy libraries." - ), - ) - ], - responses={ - 200: LibraryTabSerializer, - 401: "The requester is not authenticated.", - }, - ) - @action(detail=False, methods=['get'], url_path='libraries', url_name='libraries') - def libraries(self, request: Request): - """ - Get an object containing all libraries on home page. - - **Example Request** - - GET /api/contentstore/v1/home/libraries/ - """ - library_context = get_library_context(request) - serializer = self.get_serializer(library_context) - return Response(serializer.data) - - -# DEPRECATED (ADR 0028): Use HomeViewSet instead. -# Will be removed after one named release. -# Use GET home/, home/courses/, home/libraries/ instead. @view_auth_classes(is_authenticated=True) class HomePageView(APIView): """ @@ -243,13 +99,11 @@ def get(self, request: Request): return Response(serializer.data) +@view_auth_classes(is_authenticated=True) class HomePageCoursesView(APIView): """ View for getting all courses and libraries available to the logged in user. """ - authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) - permission_classes = (IsAuthenticated,) - serializer_class = CourseHomeTabSerializer @apidocs.schema( parameters=[ apidocs.string_parameter( @@ -316,7 +170,7 @@ def get(self, request: Request): "archived_courses": archived_courses, "in_process_course_actions": in_process_course_actions, } - serializer = self.serializer_class(courses_context) + serializer = CourseHomeTabSerializer(courses_context) return Response(serializer.data) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py index d0a16c095eb2..ccdd906609a4 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py @@ -7,19 +7,17 @@ import ddt import pytz from django.conf import settings -from django.test import TestCase, override_settings +from django.test import override_settings from django.urls import reverse from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_content import api as content_api from organizations.tests.factories import OrganizationFactory from rest_framework import status -from rest_framework.test import APIClient from cms.djangoapps.contentstore.tests.test_libraries import LibraryTestCase from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.modulestore_migrator import api as migrator_api from cms.djangoapps.modulestore_migrator.data import CompositionLevel, RepeatHandlingStrategy -from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.content_libraries import api as lib_api @@ -402,54 +400,5 @@ def test_home_page_libraries_response(self): ], } - assert response.status_code == status.HTTP_200_OK - assert response.json() == expected_response - - -class HomePageCoursesViewPermissionsTest(TestCase): - """ - ADR 0026 – permission regression tests for HomePageCoursesView. - - 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:v1: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) - assert response.status_code == status.HTTP_401_UNAUTHORIZED - - def test_authenticated_user_gets_200(self): - """ - Any authenticated user (not necessarily staff) must receive 200. - - HomePageCoursesView 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) - assert 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) - assert 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={}) - assert response.status_code == status.HTTP_401_UNAUTHORIZED + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertDictEqual(expected_response, response.json()) # noqa: PT009 diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home_viewset.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home_viewset.py deleted file mode 100644 index d0e309f5377a..000000000000 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home_viewset.py +++ /dev/null @@ -1,128 +0,0 @@ -""" -Unit tests for HomeViewSet (ADR 0028). - -MongoDB-free: all service-layer calls are mocked. - -patch.object is used for the ViewSet's get_serializer() method because: - - get_serializer_class() returns a *different* serializer per action. - - Each serializer (StudioHomeSerializer, CourseHomeTabSerializer, - LibraryTabSerializer) has many required fields that would be painful to - satisfy with synthetic data. - - Patching get_serializer() lets us focus on routing + service-call assertions - without re-testing serializer logic (covered in serializer unit tests). -""" -from unittest.mock import patch - -from django.urls import reverse -from rest_framework import status -from rest_framework.test import APITestCase - -from cms.djangoapps.contentstore.rest_api.v1.views.home import HomeViewSet -from common.djangoapps.student.tests.factories import UserFactory - -# ------------------------------------------------------------------ -# Mock target paths -# ------------------------------------------------------------------ -MOCK_GET_HOME_CONTEXT = ( - 'cms.djangoapps.contentstore.rest_api.v1.views.home.get_home_context' -) -MOCK_GET_COURSE_CONTEXT = ( - 'cms.djangoapps.contentstore.rest_api.v1.views.home.get_course_context' -) -MOCK_GET_LIBRARY_CONTEXT = ( - 'cms.djangoapps.contentstore.rest_api.v1.views.home.get_library_context' -) -MOCK_ORG_API = ( - 'cms.djangoapps.contentstore.rest_api.v1.views.home.org_api' -) - - -class TestHomeViewSetPermissions(APITestCase): - """ - ADR 0028 – permission regression tests for HomeViewSet. - - Verifies that IsAuthenticated enforces the same access rules previously - provided by @view_auth_classes(is_authenticated=True) on the deprecated views. - """ - - def test_unauthenticated_list_returns_401(self): - """Unauthenticated GET /home/ must return 401.""" - url = reverse('cms.djangoapps.contentstore:v1:home-list') - response = self.client.get(url) - assert response.status_code == status.HTTP_401_UNAUTHORIZED - - def test_unauthenticated_courses_returns_401(self): - """Unauthenticated GET /home/courses/ must return 401.""" - url = reverse('cms.djangoapps.contentstore:v1:home-courses') - response = self.client.get(url) - assert response.status_code == status.HTTP_401_UNAUTHORIZED - - def test_unauthenticated_libraries_returns_401(self): - """Unauthenticated GET /home/libraries/ must return 401.""" - url = reverse('cms.djangoapps.contentstore:v1:home-libraries') - response = self.client.get(url) - assert response.status_code == status.HTTP_401_UNAUTHORIZED - - -class TestHomeViewSetActions(APITestCase): - """ - Action tests for HomeViewSet (list, courses, libraries). - - Any authenticated user can access these endpoints — no course-staff role - is required — so a plain (non-staff) factory user is sufficient. - """ - - def setUp(self): - super().setUp() - self.user = UserFactory.create() - self.client.force_authenticate(user=self.user) - - # ------------------------------------------------------------------ - # list → GET /home/ - # ------------------------------------------------------------------ - - @patch.object(HomeViewSet, 'get_serializer') - @patch(MOCK_ORG_API) - @patch(MOCK_GET_HOME_CONTEXT) - def test_list_calls_get_home_context(self, mock_home, mock_org, mock_get_ser): - """GET /home/ calls get_home_context() and returns 200.""" - mock_home.return_value = {'can_create_organizations': True} - mock_org.is_autocreate_enabled.return_value = True - mock_get_ser.return_value.data = {'studio_name': 'Studio'} - - response = self.client.get(reverse('cms.djangoapps.contentstore:v1:home-list')) - - assert response.status_code == status.HTTP_200_OK - mock_home.assert_called_once() - - # ------------------------------------------------------------------ - # courses → GET /home/courses/ - # ------------------------------------------------------------------ - - @patch.object(HomeViewSet, 'get_serializer') - @patch(MOCK_GET_COURSE_CONTEXT) - def test_courses_calls_get_course_context(self, mock_courses, mock_get_ser): - """GET /home/courses/ calls get_course_context() and returns 200.""" - mock_courses.return_value = ([], [], []) - mock_get_ser.return_value.data = {'courses': [], 'archived_courses': [], 'in_process_course_actions': []} - - response = self.client.get(reverse('cms.djangoapps.contentstore:v1:home-courses')) - - assert response.status_code == status.HTTP_200_OK - mock_courses.assert_called_once() - - # ------------------------------------------------------------------ - # libraries → GET /home/libraries/ - # ------------------------------------------------------------------ - - @patch.object(HomeViewSet, 'get_serializer') - @patch(MOCK_GET_LIBRARY_CONTEXT) - def test_libraries_calls_get_library_context(self, mock_libs, mock_get_ser): - """GET /home/libraries/ calls get_library_context() and returns 200.""" - mock_libs.return_value = {'libraries': []} - mock_get_ser.return_value.data = {'libraries': []} - - response = self.client.get(reverse('cms.djangoapps.contentstore:v1:home-libraries')) - - assert response.status_code == status.HTTP_200_OK - mock_libs.assert_called_once() From 0ff8d422d584098ff294e38369b0dbee33a2d764 Mon Sep 17 00:00:00 2001 From: Muhammad Faraz Maqsood Date: Wed, 3 Jun 2026 12:57:55 +0500 Subject: [PATCH 03/13] feat: standardize home v2 API into v4 (#38684) --- cms/djangoapps/contentstore/rest_api/urls.py | 4 +- .../contentstore/rest_api/v4/__init__.py | 0 .../rest_api/v4/serializers/__init__.py | 0 .../rest_api/v4/serializers/home.py | 16 + .../rest_api/v4/tests/__init__.py | 0 .../rest_api/v4/tests/test_home.py | 55 ++++ .../contentstore/rest_api/v4/urls.py | 14 + .../rest_api/v4/views/__init__.py | 0 .../contentstore/rest_api/v4/views/home.py | 220 ++++++++++++++ .../rest_api/v4/views/tests/__init__.py | 0 .../rest_api/v4/views/tests/test_home.py | 275 ++++++++++++++++++ cms/djangoapps/contentstore/views/course.py | 10 +- openedx/core/lib/api/exceptions.py | 135 +++++++++ 13 files changed, 725 insertions(+), 4 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v4/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v4/serializers/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v4/serializers/home.py create mode 100644 cms/djangoapps/contentstore/rest_api/v4/tests/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v4/tests/test_home.py create mode 100644 cms/djangoapps/contentstore/rest_api/v4/urls.py create mode 100644 cms/djangoapps/contentstore/rest_api/v4/views/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v4/views/home.py create mode 100644 cms/djangoapps/contentstore/rest_api/v4/views/tests/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v4/views/tests/test_home.py create mode 100644 openedx/core/lib/api/exceptions.py diff --git a/cms/djangoapps/contentstore/rest_api/urls.py b/cms/djangoapps/contentstore/rest_api/urls.py index af2694bfbbc5..e6337cb11b7c 100644 --- a/cms/djangoapps/contentstore/rest_api/urls.py +++ b/cms/djangoapps/contentstore/rest_api/urls.py @@ -7,11 +7,13 @@ from .v0 import urls as v0_urls from .v1 import urls as v1_urls from .v2 import urls as v2_urls +from .v4 import urls as v4_urls app_name = 'cms.djangoapps.contentstore' urlpatterns = [ path('v0/', include(v0_urls)), path('v1/', include(v1_urls)), - path('v2/', include(v2_urls)) + path('v2/', include(v2_urls)), + path('v4/', include(v4_urls)), ] diff --git a/cms/djangoapps/contentstore/rest_api/v4/__init__.py b/cms/djangoapps/contentstore/rest_api/v4/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/rest_api/v4/serializers/__init__.py b/cms/djangoapps/contentstore/rest_api/v4/serializers/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/rest_api/v4/serializers/home.py b/cms/djangoapps/contentstore/rest_api/v4/serializers/home.py new file mode 100644 index 000000000000..f3b6bf0f4ee3 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v4/serializers/home.py @@ -0,0 +1,16 @@ +"""API serializers for course home V4. Re-exports V2 serializers under V4 names.""" +from cms.djangoapps.contentstore.rest_api.v2.serializers.home import ( + CourseCommonSerializerV2, + CourseHomeTabSerializerV2, + UnsucceededCourseSerializerV2, +) + +CourseCommonSerializerV4 = CourseCommonSerializerV2 +CourseHomeTabSerializerV4 = CourseHomeTabSerializerV2 +UnsucceededCourseSerializerV4 = UnsucceededCourseSerializerV2 + +__all__ = [ + "CourseCommonSerializerV4", + "CourseHomeTabSerializerV4", + "UnsucceededCourseSerializerV4", +] diff --git a/cms/djangoapps/contentstore/rest_api/v4/tests/__init__.py b/cms/djangoapps/contentstore/rest_api/v4/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/rest_api/v4/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v4/tests/test_home.py new file mode 100644 index 000000000000..aa58305a605f --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v4/tests/test_home.py @@ -0,0 +1,55 @@ +""" +ADR 0029 - Standardized error-response tests for HomeCoursesViewSet (v4). + +Verifies that the central exception handler produces the correct ADR 0029 +envelope for auth errors on the v4 home courses endpoint. +""" + +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient, APITestCase + +_REQUIRED_ERROR_FIELDS = ("type", "title", "status", "detail", "instance") + + +class TestHomeCoursesViewSetErrorShape(APITestCase): + """ + ADR 0029 - error response shape regression tests for HomeCoursesViewSet (v4). + """ + + def setUp(self): + super().setUp() + self.client = APIClient() + self.list_url = reverse("cms.djangoapps.contentstore:v4:home-courses-list") + + def test_unauthenticated_returns_standardized_401(self): + """Unauthenticated GET must return 401 with the ADR 0029 envelope.""" + response = self.client.get(self.list_url) + + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) # noqa: PT009 + for field in _REQUIRED_ERROR_FIELDS: + self.assertIn( # noqa: PT009 + field, response.data, f"ADR 0029: missing field '{field}'" + ) + + def test_unauthenticated_401_type_uri(self): + """The ``type`` field for 401 must be the ADR 0029 authn URI.""" + response = self.client.get(self.list_url) + + self.assertEqual( # noqa: PT009 + response.data.get("type"), + "https://docs.openedx.org/errors/authn", + ) + + def test_error_body_has_no_legacy_fields(self): + """Error responses must NOT contain old DeveloperErrorViewMixin fields.""" + response = self.client.get(self.list_url) + + self.assertNotIn("developer_message", response.data) # noqa: PT009 + self.assertNotIn("error_code", response.data) # noqa: PT009 + + def test_instance_field_is_request_path(self): + """The ``instance`` field must equal the request path.""" + response = self.client.get(self.list_url) + + self.assertEqual(response.data.get("instance"), self.list_url) # noqa: PT009 diff --git a/cms/djangoapps/contentstore/rest_api/v4/urls.py b/cms/djangoapps/contentstore/rest_api/v4/urls.py new file mode 100644 index 000000000000..c75a113ef53f --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v4/urls.py @@ -0,0 +1,14 @@ +"""Contentstore API v4 URLs.""" + +from rest_framework.routers import DefaultRouter + +from cms.djangoapps.contentstore.rest_api.v4.views import home + +app_name = "v4" + +# ADR 0028: HomeCoursesViewSet registered via DefaultRouter. +# Generates: GET home/courses/ → name: home-courses-list +router = DefaultRouter() +router.register(r'home/courses', home.HomeCoursesViewSet, basename='home-courses') + +urlpatterns = router.urls diff --git a/cms/djangoapps/contentstore/rest_api/v4/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v4/views/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/rest_api/v4/views/home.py b/cms/djangoapps/contentstore/rest_api/v4/views/home.py new file mode 100644 index 000000000000..d6c927b328c7 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v4/views/home.py @@ -0,0 +1,220 @@ +"""HomeCoursesViewSet for getting courses available to the logged-in user (v4).""" + +from drf_spectacular.utils import OpenApiParameter, OpenApiResponse, extend_schema +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 rest_framework import viewsets +from rest_framework.permissions import IsAuthenticated +from rest_framework.request import Request +from rest_framework.response import Response + +from cms.djangoapps.contentstore.rest_api.v4.serializers.home import ( + CourseHomeTabSerializerV4, +) +from cms.djangoapps.contentstore.utils import get_course_context_v2 + + +class HomePageCoursesPaginator(DefaultPagination): + """ + ADR 0032 - standard pagination for the Studio home courses list (v4). + + Extends ``DefaultPagination`` (edx-rest-framework-extensions) which + provides the 7-field response envelope: + ``count``, ``num_pages``, ``current_page``, ``start``, + ``next``, ``previous``, ``results``. + + Overrides ``paginate_queryset`` to handle ``filter`` objects returned + by ``get_course_context_v2``. + """ + + page_size_query_param = "page_size" + + def paginate_queryset(self, queryset, request, view=None): + """ + Paginate a queryset, converting ``filter`` objects to lists first. + + ``get_course_context_v2`` may return a ``filter`` object; the base + ``PageNumberPagination`` cannot measure its length without materialising + it first, so we do that here. + """ + if isinstance(queryset, filter): + queryset = list(queryset) + return super().paginate_queryset(queryset, request, view) + + +def _query_param( + name: str, description: str, deprecated: bool = False +) -> OpenApiParameter: + """Build a string-typed, optional query parameter for OpenAPI docs.""" + return OpenApiParameter( + name=name, + description=description, + required=False, + type=str, + location=OpenApiParameter.QUERY, + deprecated=deprecated, + ) + + +_HOME_COURSES_QUERY_PARAMETERS = [ + _query_param("org", "Filter by course org"), + _query_param("search", "Filter by course name, org, or number"), + _query_param( + "ordering", + "Order by course field: display_name, org, number, or run (ADR 0033 standard parameter).", + ), + _query_param( + "order", + "Deprecated alias for 'ordering' (ADR 0033). Use 'ordering' instead.", + deprecated=True, + ), + _query_param("active_only", "Filter to active courses only"), + _query_param("archived_only", "Filter to archived courses only"), + _query_param("page", "Page number for pagination"), + _query_param("page_size", "Number of courses per page (default 10, max 100)"), +] + +_UNAUTHENTICATED_RESPONSE = OpenApiResponse( + description="The requester is not authenticated." +) + +# ADR 0033: emitted as an HTTP ``Deprecation`` header when the legacy ``order`` +# parameter is used instead of the DRF-standard ``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 when the legacy ``order`` parameter is used.""" + if "order" in request.query_params: + response["Deprecation"] = _LEGACY_ORDER_DEPRECATION_HEADER + return response + + +class HomeCoursesViewSet(viewsets.ViewSet): + """ + ViewSet for course listing (v4). Registered via DefaultRouter (basename ``home-courses``). + + Router-generated URLs:: + + GET /api/contentstore/v4/home/courses/ → list + + Supersedes ``HomePageCoursesViewV2`` at ``/api/contentstore/v2/home/courses``. + + ADR compliance: + - 0025: ``serializer_class`` attribute for schema generation + - 0026: explicit ``authentication_classes`` and ``permission_classes`` + - 0027: ``drf_spectacular`` for OpenAPI documentation + - 0028: ViewSet with DefaultRouter registration + - 0032: 7-field pagination envelope via ``DefaultPagination`` + - 0033: ``ordering`` parameter; ``order`` kept as deprecated alias + """ + + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated,) + serializer_class = CourseHomeTabSerializerV4 + + def get_exception_handler(self): + """Return the ADR 0029 standardized error handler for this viewset.""" + from openedx.core.lib.api.exceptions import standardized_error_exception_handler + return standardized_error_exception_handler + + 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. " + "Supersedes ``GET /api/contentstore/v2/home/courses``." + ), + parameters=_HOME_COURSES_QUERY_PARAMETERS, + responses={ + 200: OpenApiResponse( + response=CourseHomeTabSerializerV4, + 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/v4/home/courses/ + GET /api/contentstore/v4/home/courses/?org=edX + GET /api/contentstore/v4/home/courses/?search=E2E + GET /api/contentstore/v4/home/courses/?ordering=-org + GET /api/contentstore/v4/home/courses/?order=-org + GET /api/contentstore/v4/home/courses/?active_only=true + GET /api/contentstore/v4/home/courses/?archived_only=true + GET /api/contentstore/v4/home/courses/?page=2 + GET /api/contentstore/v4/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 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 on the last page. + - ``previous`` (str|null): URL for the previous page, or null on the first page. + - ``results`` (dict): Course data for the current page. + + **Example Response** + + ```json + { + "count": 1, + "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": [] + } + } + ``` + """ + 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) diff --git a/cms/djangoapps/contentstore/rest_api/v4/views/tests/__init__.py b/cms/djangoapps/contentstore/rest_api/v4/views/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/rest_api/v4/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v4/views/tests/test_home.py new file mode 100644 index 000000000000..45b6ab472709 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v4/views/tests/test_home.py @@ -0,0 +1,275 @@ +""" +Unit tests for HomeCoursesViewSet (v4). +""" + +from collections import OrderedDict +from datetime import datetime, timedelta, timezone +from unittest.mock import patch + +import ddt +from django.conf import settings +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient, APITestCase + +from cms.djangoapps.contentstore.rest_api.v4.views.home import ( + _LEGACY_ORDER_DEPRECATION_HEADER, +) +from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from cms.djangoapps.contentstore.utils import reverse_course_url +from openedx.core.djangoapps.content.course_overviews.tests.factories import ( + CourseOverviewFactory, +) + +_MOCK_GET_COURSE_CONTEXT_V2 = ( + "cms.djangoapps.contentstore.rest_api.v4.views.home.get_course_context_v2" +) + + +class TestHomeCoursesViewSetPermissions(APITestCase): + """ADR 0026 - permission regression tests for HomeCoursesViewSet.""" + + def setUp(self): + super().setUp() + self.list_url = reverse("cms.djangoapps.contentstore:v4:home-courses-list") + + def test_unauthenticated_returns_401(self): + """Unauthenticated GET /v4/home/courses/ must return 401.""" + client = APIClient() + response = client.get(self.list_url) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) # noqa: PT009 + + def test_authenticated_staff_gets_200(self): + """Authenticated staff user must receive 200.""" + from django.contrib.auth import get_user_model + + User = get_user_model() + user = User.objects.create_user( + username="teststaff", password="pass", is_staff=True + ) + self.client.force_authenticate(user=user) + with patch(_MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get(self.list_url) + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + + +@ddt.ddt +class TestHomeCoursesViewSet(CourseTestCase): + """Functional tests for HomeCoursesViewSet list action.""" + + def setUp(self): + super().setUp() + self.api_v4_url = reverse("cms.djangoapps.contentstore:v4:home-courses-list") + self.active_course = CourseOverviewFactory.create( + id=self.course.id, + org=self.course.org, + display_name=self.course.display_name, + ) + archived_course_key = self.store.make_course_key( + "demo-org", "demo-number", "demo-run" + ) + self.archived_course = CourseOverviewFactory.create( + display_name="Demo Course (Sample)", + id=archived_course_key, + org=archived_course_key.org, + end=(datetime.now() - timedelta(days=365)).replace( + tzinfo=timezone.utc # noqa: UP017 + ), + ) + self.non_staff_client, _ = self.create_non_staff_authed_user_client() + + def test_home_page_response(self): + """GET /v4/home/courses/ must return the 7-field ADR 0032 pagination envelope.""" + response = self.client.get(self.api_v4_url) + course_id = str(self.course.id) + archived_course_id = str(self.archived_course.id) + + expected_data = { + "courses": [ + OrderedDict( + [ + ("course_key", course_id), + ("display_name", self.course.display_name), + ( + "lms_link", + f"{settings.LMS_ROOT_URL}/courses/{course_id}/jump_to/{self.course.location}", + ), + ( + "cms_link", + f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.course.id)}', + ), + ("number", self.course.number), + ("org", self.course.org), + ("rerun_link", f"/course_rerun/{course_id}"), + ("run", self.course.id.run), + ("url", f"/course/{course_id}"), + ("is_active", True), + ] + ), + OrderedDict( + [ + ("course_key", str(self.archived_course.id)), + ("display_name", self.archived_course.display_name), + ( + "lms_link", + f"{settings.LMS_ROOT_URL}/courses/{archived_course_id}" + f"/jump_to/{self.archived_course.location}", + ), + ( + "cms_link", + f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}', + ), + ("number", self.archived_course.number), + ("org", self.archived_course.org), + ("rerun_link", f"/course_rerun/{str(self.archived_course.id)}"), + ("run", self.archived_course.id.run), + ("url", f"/course/{str(self.archived_course.id)}"), + ("is_active", False), + ] + ), + ], + "in_process_course_actions": [], + } + 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) # noqa: PT009 + self.assertDictEqual(expected_response, response.data) # noqa: PT009 + + def test_active_only_query_if_passed(self): + """?active_only=true must return only active courses.""" + response = self.client.get(self.api_v4_url, {"active_only": "true"}) + + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertEqual(len(response.data["results"]["courses"]), 1) # noqa: PT009 + self.assertTrue( # noqa: PT009 + response.data["results"]["courses"][0]["is_active"] + ) + + def test_archived_only_query_if_passed(self): + """?archived_only=true must return only archived courses.""" + response = self.client.get(self.api_v4_url, {"archived_only": "true"}) + + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertEqual(len(response.data["results"]["courses"]), 1) # noqa: PT009 + self.assertFalse( # noqa: PT009 + response.data["results"]["courses"][0]["is_active"] + ) + + def test_search_query_if_passed(self): + """?search=sample must filter courses by name.""" + response = self.client.get(self.api_v4_url, {"search": "sample"}) + + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertEqual(len(response.data["results"]["courses"]), 1) # noqa: PT009 + + def test_ordering_query_if_passed(self): + """?ordering=org must order courses by org (ADR 0033 standard parameter).""" + response = self.client.get(self.api_v4_url, {"ordering": "org"}) + + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertEqual(len(response.data["results"]["courses"]), 2) # noqa: PT009 + self.assertEqual( # noqa: PT009 + response.data["results"]["courses"][0]["org"], "demo-org" + ) + + def test_legacy_order_query_still_works(self): + """?order=org must still work (deprecated alias, ADR 0033).""" + response = self.client.get(self.api_v4_url, {"order": "org"}) + + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertEqual(len(response.data["results"]["courses"]), 2) # noqa: PT009 + + def test_page_query_if_passed(self): + """?page=1 must return paginated result with count.""" + response = self.client.get(self.api_v4_url, {"page": 1}) + + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertEqual(response.data["count"], 2) # noqa: PT009 + + @ddt.data( + ("active_only", "true"), + ("archived_only", "true"), + ("search", "sample"), + ("ordering", "org"), + ("page", 1), + ) + @ddt.unpack + def test_if_empty_list_of_courses(self, query_param, value): + """Empty course list returns empty results, not an error.""" + self.active_course.delete() + self.archived_course.delete() + + response = self.client.get(self.api_v4_url, {query_param: value}) + + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertEqual(len(response.data["results"]["courses"]), 0) # noqa: PT009 + + @ddt.data( + ("active_only", "true"), + ("archived_only", "true"), + ("search", "sample"), + ("ordering", "org"), + ("page", 1), + ) + @ddt.unpack + def test_if_empty_list_of_courses_non_staff(self, query_param, value): + """Non-staff users with no courses get an empty result.""" + self.active_course.delete() + self.archived_course.delete() + + response = self.non_staff_client.get(self.api_v4_url, {query_param: value}) + + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertEqual(len(response.data["results"]["courses"]), 0) # noqa: PT009 + + +class TestHomeCoursesViewSetOrderingDeprecation(CourseTestCase): + """ADR 0033 – Deprecation header tests for the legacy ``order`` parameter.""" + + def setUp(self): + super().setUp() + self.list_url = reverse("cms.djangoapps.contentstore:v4:home-courses-list") + + def test_ordering_param_no_deprecation_header(self): + """``?ordering=display_name`` must not emit a Deprecation header.""" + with patch(_MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get(self.list_url, {"ordering": "display_name"}) + + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertNotIn("Deprecation", response) # noqa: PT009 + + def test_legacy_order_param_emits_deprecation_header(self): + """``?order=display_name`` must emit the ADR 0033 Deprecation header.""" + with patch(_MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get(self.list_url, {"order": "display_name"}) + + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertIn("Deprecation", response) # noqa: PT009 + self.assertEqual( # noqa: PT009 + response["Deprecation"], _LEGACY_ORDER_DEPRECATION_HEADER + ) + + def test_ordering_wins_when_both_present(self): + """When both params sent, ``ordering`` wins and Deprecation header is still emitted.""" + with patch(_MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get( + self.list_url, {"ordering": "org", "order": "display_name"} + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + self.assertIn("Deprecation", response) # noqa: PT009 + + def test_no_ordering_param_no_deprecation_header(self): + """Plain GET /v4/home/courses/ must not emit a Deprecation header.""" + with patch(_MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get(self.list_url) + + self.assertNotIn("Deprecation", response) # noqa: PT009 diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index d3da7b9ec064..e5e54e2ebd69 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -465,19 +465,23 @@ def get_query_params_if_present(request): Arguments: request: the request object + ADR 0033 - ``ordering`` is the preferred parameter (DRF standard); ``order`` + is kept as a deprecated alias. When both are present, ``ordering`` wins. + 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/openedx/core/lib/api/exceptions.py b/openedx/core/lib/api/exceptions.py new file mode 100644 index 000000000000..13f6cb744c5c --- /dev/null +++ b/openedx/core/lib/api/exceptions.py @@ -0,0 +1,135 @@ +"""ADR 0029 - Standardized error-response exception handler and helpers.""" + +from rest_framework.exceptions import APIException, ValidationError +from rest_framework.response import Response + + +class Conflict(APIException): + """HTTP 409 Conflict — ADR 0029.""" + + status_code = 409 + default_detail = "A conflict occurred." + default_code = "conflict" + + +def standardized_error_exception_handler(exc, context): + """ + ADR 0029 - platform-level DRF exception handler. + + Wraps the existing ``ignored_error_exception_handler`` and reformats its + response into the standardized JSON error envelope:: + + { + "type": "https://docs.openedx.org/errors/{category}", + "title": "", + "status": , + "detail": "", + "instance": "" + } + + For ``ValidationError``, an additional ``errors`` key is included with + per-field error details. + """ + from openedx.core.lib.request_utils import ( + ignored_error_exception_handler, + ) # avoid circular import + + response = ignored_error_exception_handler(exc, context) + + if response is None: + return Response( + { + "type": "https://docs.openedx.org/errors/internal", + "title": "Internal Server Error", + "status": 500, + "detail": "An unexpected error occurred. Please try again later.", + }, + status=500, + ) + + request = context.get("request") + body = { + "type": f"https://docs.openedx.org/errors/{_error_type(exc)}", + "title": _error_title(exc), + "status": response.status_code, + "detail": _flatten_detail(response.data), + } + if request: + body["instance"] = request.path + if hasattr(exc, "user_message") and exc.user_message: + body["user_message"] = exc.user_message + if isinstance(exc, ValidationError) and hasattr(exc, "detail"): + body["errors"] = _normalize_validation_errors(exc.detail) + + response.data = body + response["Content-Type"] = "application/json" + return response + + +def _error_type(exc): + """Map a DRF exception to an ADR 0029 error category slug.""" + from rest_framework.exceptions import ( # avoid circular import at module level + AuthenticationFailed, + NotAuthenticated, + NotFound, + PermissionDenied, + Throttled, + ) + + if isinstance(exc, (NotAuthenticated, AuthenticationFailed)): + return "authn" + if isinstance(exc, PermissionDenied): + return "authz" + if isinstance(exc, NotFound): + return "not-found" + if isinstance(exc, ValidationError): + return "validation" + if isinstance(exc, Throttled): + return "rate-limited" + if isinstance(exc, Conflict): + return "conflict" + return "internal" + + +def _error_title(exc): + """Return a human-readable title for the given DRF exception.""" + from rest_framework.exceptions import ( # avoid circular import at module level + AuthenticationFailed, + NotAuthenticated, + NotFound, + PermissionDenied, + Throttled, + ) + + return { + NotAuthenticated: "Authentication Required", + AuthenticationFailed: "Authentication Failed", + PermissionDenied: "Permission Denied", + NotFound: "Not Found", + ValidationError: "Validation Error", + Throttled: "Too Many Requests", + Conflict: "Conflict", + }.get(type(exc), "Internal Server Error") + + +def _flatten_detail(data): + """Extract a single string detail message from a DRF response data payload.""" + if isinstance(data, str): + return data + if isinstance(data, dict) and "detail" in data: + return str(data["detail"]) + if isinstance(data, list) and data: + return str(data[0]) + return str(data) + + +def _normalize_validation_errors(detail): + """Convert DRF validation error detail into a consistent per-field dict.""" + if isinstance(detail, dict): + return { + field: [str(e) for e in (errs if isinstance(errs, list) else [errs])] + for field, errs in detail.items() + } + if isinstance(detail, list): + return {"non_field_errors": [str(e) for e in detail]} + return {"non_field_errors": [str(detail)]} From 841e76a0e304b29c424b88d1c69c35cf26f59745 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed <68893403+taimoor-ahmed-1@users.noreply.github.com> Date: Wed, 3 Jun 2026 16:06:15 +0500 Subject: [PATCH 04/13] feat: apply ADRs to home v1 apis & standardize them to v3 (#38694) * feat: apply ADRs to home v1 apis * feat: apply ADR 0029 error envelope to home v3 viewset Wires HomeViewSet (v3) into openedx/core/lib/api/exceptions's standardized_error_exception_handler via a v3-local StandardizedErrorMixin that overrides DRF's per-view get_exception_handler. Project-wide EXCEPTION_HANDLER setting is intentionally left unchanged so v0/v1/v2 endpoints are unaffected. * feat: refactor mixin so that both v3 and v4 can use it --- .../contentstore/rest_api/mixins.py | 29 ++++ cms/djangoapps/contentstore/rest_api/urls.py | 2 + .../contentstore/rest_api/v3/__init__.py | 0 .../rest_api/v3/tests/__init__.py | 0 .../rest_api/v3/tests/test_home.py | 86 +++++++++ .../contentstore/rest_api/v3/urls.py | 12 ++ .../rest_api/v3/views/__init__.py | 3 + .../contentstore/rest_api/v3/views/home.py | 164 ++++++++++++++++++ .../rest_api/v3/views/tests/__init__.py | 0 .../rest_api/v3/views/tests/test_home.py | 118 +++++++++++++ .../contentstore/rest_api/v4/views/home.py | 9 +- 11 files changed, 417 insertions(+), 6 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/mixins.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/tests/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/urls.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/views/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/views/home.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/views/tests/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/views/tests/test_home.py diff --git a/cms/djangoapps/contentstore/rest_api/mixins.py b/cms/djangoapps/contentstore/rest_api/mixins.py new file mode 100644 index 000000000000..61f1ff82b4b6 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/mixins.py @@ -0,0 +1,29 @@ +""" +Shared mixins for the contentstore REST API (used across versions). + +Currently provides :class:`StandardizedErrorMixin`, which opts a single +view/viewset into the ADR 0029 error envelope without changing the +project-wide DRF ``EXCEPTION_HANDLER`` setting. +""" +from openedx.core.lib.api.exceptions import standardized_error_exception_handler + + +class StandardizedErrorMixin: + """ + Opt-in mixin that routes DRF exceptions on this view through the ADR 0029 + standardized error-response handler (see + ``openedx.core.lib.api.exceptions.standardized_error_exception_handler``). + + DRF's :class:`rest_framework.views.APIView` calls ``self.get_exception_handler`` + inside ``handle_exception``; overriding that method here lets the view + return the standardized envelope while other endpoints continue to use + whichever handler the project-wide ``EXCEPTION_HANDLER`` setting points at. + + Usage:: + + class MyViewSet(StandardizedErrorMixin, viewsets.ViewSet): + ... + """ + + def get_exception_handler(self): + return standardized_error_exception_handler diff --git a/cms/djangoapps/contentstore/rest_api/urls.py b/cms/djangoapps/contentstore/rest_api/urls.py index e6337cb11b7c..9696cdf9b73c 100644 --- a/cms/djangoapps/contentstore/rest_api/urls.py +++ b/cms/djangoapps/contentstore/rest_api/urls.py @@ -7,6 +7,7 @@ from .v0 import urls as v0_urls from .v1 import urls as v1_urls from .v2 import urls as v2_urls +from .v3 import urls as v3_urls from .v4 import urls as v4_urls app_name = 'cms.djangoapps.contentstore' @@ -15,5 +16,6 @@ path('v0/', include(v0_urls)), path('v1/', include(v1_urls)), path('v2/', include(v2_urls)), + path('v3/', include(v3_urls)), path('v4/', include(v4_urls)), ] diff --git a/cms/djangoapps/contentstore/rest_api/v3/__init__.py b/cms/djangoapps/contentstore/rest_api/v3/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/rest_api/v3/tests/__init__.py b/cms/djangoapps/contentstore/rest_api/v3/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py new file mode 100644 index 000000000000..50f14bc361f6 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py @@ -0,0 +1,86 @@ +""" +ADR 0029 – Standardized error-response regression tests for HomeViewSet (v3). + +The ADR 0029 envelope is wired into the v3 viewset via +:class:`cms.djangoapps.contentstore.rest_api.mixins.StandardizedErrorMixin`, +which overrides DRF's per-view ``get_exception_handler`` to point at +``openedx.core.lib.api.exceptions.standardized_error_exception_handler``. + +This is intentionally *scoped to v3* — the project-wide DRF +``EXCEPTION_HANDLER`` setting is unchanged, so v0/v1/v2 endpoints continue +to return the legacy error shape. +""" +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient, APITestCase + +_REQUIRED_ERROR_FIELDS = ("type", "title", "status", "detail", "instance") + + +class TestHomeViewSetErrorShape(APITestCase): + """ + ADR 0029 – error response shape regression tests for HomeViewSet (v3). + + Verifies that 401 responses on all three actions conform to the + standardized JSON envelope. + """ + + def setUp(self): + super().setUp() + self.client = APIClient() + self.list_url = reverse("cms.djangoapps.contentstore:v3:home-list") + self.courses_url = reverse("cms.djangoapps.contentstore:v3:home-courses") + self.libraries_url = reverse("cms.djangoapps.contentstore:v3:home-libraries") + + def test_unauthenticated_list_returns_standardized_401(self): + """Unauthenticated GET /home/ must return 401 with the ADR 0029 envelope.""" + response = self.client.get(self.list_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + def test_unauthenticated_list_401_type_uri(self): + """The ``type`` field for 401 must be the ADR 0029 authn URI.""" + response = self.client.get(self.list_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.data.get("type") == "https://docs.openedx.org/errors/authn" + + def test_unauthenticated_courses_returns_standardized_401(self): + """Unauthenticated GET /home/courses/ must return 401 with the ADR 0029 envelope.""" + response = self.client.get(self.courses_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + def test_unauthenticated_libraries_returns_standardized_401(self): + """Unauthenticated GET /home/libraries/ must return 401 with the ADR 0029 envelope.""" + response = self.client.get(self.libraries_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + def test_error_body_has_no_developer_message(self): + """Error responses must NOT contain old DeveloperErrorViewMixin fields.""" + response = self.client.get(self.list_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert "developer_message" not in response.data + assert "error_code" not in response.data + + def test_instance_field_is_request_path(self): + """The ``instance`` field must equal the request path.""" + response = self.client.get(self.list_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.data.get("instance") == self.list_url + + def test_v1_endpoint_unaffected_by_v3_envelope(self): + """ + The ADR 0029 envelope must be scoped to v3 — hitting the legacy v1 + ``home/courses`` endpoint unauthenticated must NOT return the v3 envelope + (it has no ``type`` / ``instance`` keys). + """ + v1_url = reverse("cms.djangoapps.contentstore:v1:courses") + response = self.client.get(v1_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + # v1 still uses the project-default handler → ADR 0029 fields absent. + assert "type" not in response.data + assert "instance" not in response.data diff --git a/cms/djangoapps/contentstore/rest_api/v3/urls.py b/cms/djangoapps/contentstore/rest_api/v3/urls.py new file mode 100644 index 000000000000..9d8f93a7ee7c --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/urls.py @@ -0,0 +1,12 @@ +"""Contentstore API v3 URLs.""" + +from rest_framework.routers import DefaultRouter + +from cms.djangoapps.contentstore.rest_api.v3.views import HomeViewSet + +app_name = "v3" + +router = DefaultRouter() +router.register(r'home', HomeViewSet, basename='home') + +urlpatterns = router.urls diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v3/views/__init__.py new file mode 100644 index 000000000000..573556d80b39 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/views/__init__.py @@ -0,0 +1,3 @@ +"""Views for v3 contentstore API.""" + +from .home import HomeViewSet # noqa: F401 diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/home.py b/cms/djangoapps/contentstore/rest_api/v3/views/home.py new file mode 100644 index 000000000000..6f2bcfe63acb --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/views/home.py @@ -0,0 +1,164 @@ +""" +API Views for Studio course home — v3. + +This module is the v3 incarnation of the v1 ``home`` endpoints, restructured +to apply the FC-0118 ADRs: + + * ADR 0025 – ``serializer_class`` (with per-action ``get_serializer_class``) + * ADR 0026 – explicit ``authentication_classes`` + ``permission_classes`` + * ADR 0028 – consolidated into a single DRF ``ViewSet`` registered via + ``DefaultRouter`` (replaces the three legacy ``APIView`` classes + ``HomePageView`` / ``HomePageCoursesView`` / ``HomePageLibrariesView``) + * ADR 0029 – standardized error envelope, opted in via + :class:`StandardizedErrorMixin` (v3-scoped — does not change the + project-wide DRF ``EXCEPTION_HANDLER`` setting) +""" + +import edx_api_doc_tools as apidocs +from django.conf import settings +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from organizations import api as org_api +from rest_framework import viewsets +from rest_framework.decorators import action +from rest_framework.permissions import IsAuthenticated +from rest_framework.request import Request +from rest_framework.response import Response + +from cms.djangoapps.contentstore.rest_api.mixins import StandardizedErrorMixin +from cms.djangoapps.contentstore.rest_api.v1.serializers import ( + CourseHomeTabSerializer, + LibraryTabSerializer, + StudioHomeSerializer, +) +from cms.djangoapps.contentstore.utils import get_course_context, get_home_context, get_library_context + + +class HomeViewSet(StandardizedErrorMixin, viewsets.ViewSet): + """ + ViewSet for the Studio home page. Registered via DefaultRouter (basename ``home``). + + Router-generated URLs: + GET /api/contentstore/v3/home/ → list (aggregated home context) + GET /api/contentstore/v3/home/courses/ → courses (course list only) + GET /api/contentstore/v3/home/libraries/ → libraries (library list only) + """ + + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated,) + serializer_class = StudioHomeSerializer + + def get_serializer_class(self): + """Return the appropriate serializer class for the current action.""" + if self.action == 'courses': + return CourseHomeTabSerializer + if self.action == 'libraries': + return LibraryTabSerializer + return StudioHomeSerializer + + def get_serializer(self, *args, **kwargs): + """Return a serializer instance using the action-appropriate class.""" + return self.get_serializer_class()(*args, **kwargs) + + @apidocs.schema( + parameters=[ + apidocs.string_parameter( + "org", + apidocs.ParameterLocation.QUERY, + description="Query param to filter by course org", + )], + responses={ + 200: StudioHomeSerializer, + 401: "The requester is not authenticated.", + }, + ) + def list(self, request: Request): + """ + Get an object containing all courses and libraries on home page. + + **Example Request** + + GET /api/contentstore/v3/home/ + """ + home_context = get_home_context(request, True) + home_context.update({ + # 'allow_to_create_new_org' is actually about auto-creating organizations + # (e.g. when creating a course or library), so we add an additional test. + 'allow_to_create_new_org': ( + home_context['can_create_organizations'] and + org_api.is_autocreate_enabled() + ), + 'studio_name': settings.STUDIO_NAME, + 'studio_short_name': settings.STUDIO_SHORT_NAME, + 'studio_request_email': settings.FEATURES.get('STUDIO_REQUEST_EMAIL', ''), + 'tech_support_email': settings.TECH_SUPPORT_EMAIL, + 'platform_name': settings.PLATFORM_NAME, + 'user_is_active': request.user.is_active, + }) + serializer = self.get_serializer(home_context) + return Response(serializer.data) + + @apidocs.schema( + parameters=[ + apidocs.string_parameter( + "org", + apidocs.ParameterLocation.QUERY, + description="Query param to filter by course org", + )], + responses={ + 200: CourseHomeTabSerializer, + 401: "The requester is not authenticated.", + }, + ) + @action(detail=False, methods=['get'], url_path='courses', url_name='courses') + def courses(self, request: Request): + """ + Get an object containing all courses. + + **Example Request** + + GET /api/contentstore/v3/home/courses/ + """ + active_courses, archived_courses, in_process_course_actions = get_course_context(request) + courses_context = { + "courses": active_courses, + "archived_courses": archived_courses, + "in_process_course_actions": in_process_course_actions, + } + serializer = self.get_serializer(courses_context) + return Response(serializer.data) + + @apidocs.schema( + parameters=[ + apidocs.string_parameter( + "org", + apidocs.ParameterLocation.QUERY, + description="Query param to filter by course org", + ), + apidocs.query_parameter( + "is_migrated", + bool, + description=( + "Query param to filter by migrated status of library." + " If present (true or false), it will filter by migration status" + " else it will return all legacy libraries." + ), + ) + ], + responses={ + 200: LibraryTabSerializer, + 401: "The requester is not authenticated.", + }, + ) + @action(detail=False, methods=['get'], url_path='libraries', url_name='libraries') + def libraries(self, request: Request): + """ + Get an object containing all libraries on home page. + + **Example Request** + + GET /api/contentstore/v3/home/libraries/ + """ + library_context = get_library_context(request) + serializer = self.get_serializer(library_context) + return Response(serializer.data) diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/tests/__init__.py b/cms/djangoapps/contentstore/rest_api/v3/views/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_home.py new file mode 100644 index 000000000000..a688a488e63c --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_home.py @@ -0,0 +1,118 @@ +""" +Unit tests for HomeViewSet — v3 (ADR 0025 / 0026 / 0028). + +MongoDB-free: all service-layer calls are mocked. + +patch.object is used for the ViewSet's get_serializer() method because: + - get_serializer_class() returns a *different* serializer per action. + - Each serializer (StudioHomeSerializer, CourseHomeTabSerializer, + LibraryTabSerializer) has many required fields that would be painful to + satisfy with synthetic data. + - Patching get_serializer() lets us focus on routing + service-call + assertions without re-testing serializer logic (covered in serializer + unit tests). +""" +from unittest.mock import patch + +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APITestCase + +from cms.djangoapps.contentstore.rest_api.v3.views.home import HomeViewSet +from common.djangoapps.student.tests.factories import UserFactory + +MOCK_GET_HOME_CONTEXT = ( + 'cms.djangoapps.contentstore.rest_api.v3.views.home.get_home_context' +) +MOCK_GET_COURSE_CONTEXT = ( + 'cms.djangoapps.contentstore.rest_api.v3.views.home.get_course_context' +) +MOCK_GET_LIBRARY_CONTEXT = ( + 'cms.djangoapps.contentstore.rest_api.v3.views.home.get_library_context' +) +MOCK_ORG_API = ( + 'cms.djangoapps.contentstore.rest_api.v3.views.home.org_api' +) + + +class TestHomeViewSetPermissions(APITestCase): + """ + ADR 0026 – permission regression tests for HomeViewSet (v3). + + Verifies that ``permission_classes = (IsAuthenticated,)`` enforces the + access rules expected of the consolidated viewset. + """ + + def test_unauthenticated_list_returns_401(self): + """Unauthenticated GET /home/ must return 401.""" + url = reverse('cms.djangoapps.contentstore:v3:home-list') + response = self.client.get(url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_unauthenticated_courses_returns_401(self): + """Unauthenticated GET /home/courses/ must return 401.""" + url = reverse('cms.djangoapps.contentstore:v3:home-courses') + response = self.client.get(url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_unauthenticated_libraries_returns_401(self): + """Unauthenticated GET /home/libraries/ must return 401.""" + url = reverse('cms.djangoapps.contentstore:v3:home-libraries') + response = self.client.get(url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + +class TestHomeViewSetActions(APITestCase): + """ + Action tests for HomeViewSet (list, courses, libraries). + + Any authenticated user can access these endpoints — no course-staff role + is required — so a plain (non-staff) factory user is sufficient. + """ + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.client.force_authenticate(user=self.user) + + @patch.object(HomeViewSet, 'get_serializer') + @patch(MOCK_ORG_API) + @patch(MOCK_GET_HOME_CONTEXT) + def test_list_calls_get_home_context(self, mock_home, mock_org, mock_get_ser): + """GET /home/ calls get_home_context() and returns 200.""" + mock_home.return_value = {'can_create_organizations': True} + mock_org.is_autocreate_enabled.return_value = True + mock_get_ser.return_value.data = {'studio_name': 'Studio'} + + response = self.client.get(reverse('cms.djangoapps.contentstore:v3:home-list')) + + assert response.status_code == status.HTTP_200_OK + mock_home.assert_called_once() + + @patch.object(HomeViewSet, 'get_serializer') + @patch(MOCK_GET_COURSE_CONTEXT) + def test_courses_calls_get_course_context(self, mock_courses, mock_get_ser): + """GET /home/courses/ calls get_course_context() and returns 200.""" + mock_courses.return_value = ([], [], []) + mock_get_ser.return_value.data = { + 'courses': [], + 'archived_courses': [], + 'in_process_course_actions': [], + } + + response = self.client.get(reverse('cms.djangoapps.contentstore:v3:home-courses')) + + assert response.status_code == status.HTTP_200_OK + mock_courses.assert_called_once() + + @patch.object(HomeViewSet, 'get_serializer') + @patch(MOCK_GET_LIBRARY_CONTEXT) + def test_libraries_calls_get_library_context(self, mock_libs, mock_get_ser): + """GET /home/libraries/ calls get_library_context() and returns 200.""" + mock_libs.return_value = {'libraries': []} + mock_get_ser.return_value.data = {'libraries': []} + + response = self.client.get(reverse('cms.djangoapps.contentstore:v3:home-libraries')) + + assert response.status_code == status.HTTP_200_OK + mock_libs.assert_called_once() diff --git a/cms/djangoapps/contentstore/rest_api/v4/views/home.py b/cms/djangoapps/contentstore/rest_api/v4/views/home.py index d6c927b328c7..3013f05e9b28 100644 --- a/cms/djangoapps/contentstore/rest_api/v4/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v4/views/home.py @@ -11,6 +11,7 @@ from rest_framework.request import Request from rest_framework.response import Response +from cms.djangoapps.contentstore.rest_api.mixins import StandardizedErrorMixin from cms.djangoapps.contentstore.rest_api.v4.serializers.home import ( CourseHomeTabSerializerV4, ) @@ -98,7 +99,7 @@ def _maybe_set_legacy_order_deprecation_header( return response -class HomeCoursesViewSet(viewsets.ViewSet): +class HomeCoursesViewSet(StandardizedErrorMixin, viewsets.ViewSet): """ ViewSet for course listing (v4). Registered via DefaultRouter (basename ``home-courses``). @@ -113,6 +114,7 @@ class HomeCoursesViewSet(viewsets.ViewSet): - 0026: explicit ``authentication_classes`` and ``permission_classes`` - 0027: ``drf_spectacular`` for OpenAPI documentation - 0028: ViewSet with DefaultRouter registration + - 0029: standardized error envelope via ``StandardizedErrorMixin`` - 0032: 7-field pagination envelope via ``DefaultPagination`` - 0033: ``ordering`` parameter; ``order`` kept as deprecated alias """ @@ -121,11 +123,6 @@ class HomeCoursesViewSet(viewsets.ViewSet): permission_classes = (IsAuthenticated,) serializer_class = CourseHomeTabSerializerV4 - def get_exception_handler(self): - """Return the ADR 0029 standardized error handler for this viewset.""" - from openedx.core.lib.api.exceptions import standardized_error_exception_handler - return standardized_error_exception_handler - def get_serializer(self, *args, **kwargs): """Instantiate and return the configured serializer class.""" return self.serializer_class(*args, **kwargs) From 21097e18cc23eaa2dd6fe105286313b98e4327dd Mon Sep 17 00:00:00 2001 From: Muhammad Faraz Maqsood Date: Wed, 10 Jun 2026 12:48:32 +0500 Subject: [PATCH 05/13] feat: standardize xblock API, add v1 (#38723) --- .../contentstore/rest_api/v0/views/xblock.py | 23 ++- .../contentstore/rest_api/v1/urls.py | 7 +- .../rest_api/v1/views/__init__.py | 1 + .../rest_api/v1/views/permissions.py | 27 +++ .../v1/views/tests/test_xblock_viewset.py | 144 +++++++++++++++ .../contentstore/rest_api/v1/views/xblock.py | 119 ++++++++++++ .../xblock_storage_handlers/view_handlers.py | 174 +++++++++++++----- 7 files changed, 449 insertions(+), 46 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/permissions.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/xblock.py diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py index 79217971bb67..1cab4a390570 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py @@ -1,7 +1,14 @@ """ -Public rest API endpoints for the CMS API. +Public rest API endpoints for the CMS API — v0 xblock (DEPRECATED). + +.. deprecated:: + These views are superseded by ``XblockViewSet`` in + ``cms.djangoapps.contentstore.rest_api.v1.views.xblock``. + Use ``/api/contentstore/v1/xblock/`` going forward. + These v0 endpoints will be removed in a future release. """ import logging +import warnings from django.views.decorators.csrf import csrf_exempt from rest_framework.generics import CreateAPIView, RetrieveUpdateDestroyAPIView @@ -17,10 +24,17 @@ log = logging.getLogger(__name__) handle_xblock = view_handlers.handle_xblock +_DEPRECATION_MSG = ( + "The v0 xblock API (/api/contentstore/v0/xblock/) is deprecated. " + "Use /api/contentstore/v1/xblock/ instead." +) + @view_auth_classes() class XblockView(DeveloperErrorViewMixin, RetrieveUpdateDestroyAPIView): """ + **DEPRECATED** — use ``/api/contentstore/v1/xblock/{usage_key_string}/`` instead. + Public rest API endpoints for the CMS API. course_key: required argument, needed to authorize course authors. usage_key_string (optional): @@ -32,29 +46,35 @@ class XblockView(DeveloperErrorViewMixin, RetrieveUpdateDestroyAPIView): @course_author_access_required @expect_json_in_class_view def retrieve(self, request, course_key, usage_key_string=None): + warnings.warn(_DEPRECATION_MSG, DeprecationWarning, stacklevel=2) return handle_xblock(request, usage_key_string) @course_author_access_required @expect_json_in_class_view @validate_request_with_serializer def update(self, request, course_key, usage_key_string=None): + warnings.warn(_DEPRECATION_MSG, DeprecationWarning, stacklevel=2) return handle_xblock(request, usage_key_string) @course_author_access_required @expect_json_in_class_view @validate_request_with_serializer def partial_update(self, request, course_key, usage_key_string=None): + warnings.warn(_DEPRECATION_MSG, DeprecationWarning, stacklevel=2) return handle_xblock(request, usage_key_string) @course_author_access_required @expect_json_in_class_view def destroy(self, request, course_key, usage_key_string=None): + warnings.warn(_DEPRECATION_MSG, DeprecationWarning, stacklevel=2) return handle_xblock(request, usage_key_string) @view_auth_classes() class XblockCreateView(DeveloperErrorViewMixin, CreateAPIView): """ + **DEPRECATED** — use ``POST /api/contentstore/v1/xblock/`` instead. + Public rest API endpoints for the CMS API. course_key: required argument, needed to authorize course authors. usage_key_string (optional): @@ -68,4 +88,5 @@ class XblockCreateView(DeveloperErrorViewMixin, CreateAPIView): @expect_json_in_class_view @validate_request_with_serializer def create(self, request, course_key, usage_key_string=None): + warnings.warn(_DEPRECATION_MSG, DeprecationWarning, stacklevel=2) return handle_xblock(request, usage_key_string) diff --git a/cms/djangoapps/contentstore/rest_api/v1/urls.py b/cms/djangoapps/contentstore/rest_api/v1/urls.py index 685a81d778ce..48cd5118cc40 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/urls.py @@ -2,6 +2,7 @@ from django.conf import settings from django.urls import path, re_path +from rest_framework.routers import DefaultRouter from openedx.core.constants import COURSE_ID_PATTERN @@ -27,6 +28,7 @@ ProctoringErrorsView, VideoDownloadView, VideoUsageView, + XblockViewSet, vertical_container_children_redirect_view, ) @@ -34,7 +36,10 @@ VIDEO_ID_PATTERN = r'(?P[-\w]+)' -urlpatterns = [ +_router = DefaultRouter() +_router.register(r'xblock', XblockViewSet, basename='xblock') + +urlpatterns = _router.urls + [ path( 'home', HomePageView.as_view(), diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py index 7654c9e0befc..f60e186f9f5c 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py @@ -16,3 +16,4 @@ from .textbooks import CourseTextbooksView # noqa: F401 from .vertical_block import ContainerHandlerView, vertical_container_children_redirect_view # noqa: F401 from .videos import CourseVideosView, VideoDownloadView, VideoUsageView # noqa: F401 +from .xblock import XblockViewSet # noqa: F401 diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py b/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py new file mode 100644 index 000000000000..3b60607eebec --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py @@ -0,0 +1,27 @@ +""" +Permission classes for v1 contentstore API views (ADR 0026). +""" +import logging + +from rest_framework.permissions import BasePermission + +from common.djangoapps.student.auth import has_course_author_access + +log = logging.getLogger(__name__) + + +class HasCourseAuthorAccess(BasePermission): + """ + ADR 0026: replaces the @course_author_access_required decorator. + + Reads ``view.kwargs["course_key"]`` (a CourseKey instance) that is + injected by XblockViewSet.initial() before DRF runs permission checks. + Returns 403 if the authenticated user lacks authoring rights on that + course, or if no course key could be derived. + """ + + def has_permission(self, request, view): + course_key = getattr(view, "course_key", None) + if not course_key: + return False + return has_course_author_access(request.user, course_key) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py new file mode 100644 index 000000000000..8694f7023683 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py @@ -0,0 +1,144 @@ +""" +Tests for XblockViewSet (v1 — ADR 0028, 0026, 0029). + +Verifies: + * Each HTTP method routes to the correct per-verb handler (ADR 0028) + * Unauthenticated requests return standardized 401 (ADR 0029) + * Authenticated non-authors return standardized 403 (ADR 0029) + * ADR 0029 error envelope fields are present and correctly typed +""" +from unittest.mock import patch + +from django.http import JsonResponse +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APITestCase + +from common.djangoapps.student.tests.factories import GlobalStaffFactory, UserFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + +TEST_LOCATOR = "block-v1:edX+ToyX+Toy_Course+type@problem+block@ba6327f840da49289fb27a9243913478" +PARENT_LOCATOR = "block-v1:edX+ToyX+Toy_Course+type@vertical+block@vert1" + +_REQUIRED_ERROR_FIELDS = ("type", "title", "status", "detail", "instance") + +_MOCK_RESPONSE = JsonResponse({"locator": TEST_LOCATOR}) + +_VIEW_MODULE = "cms.djangoapps.contentstore.rest_api.v1.views.xblock" + + +def _list_url(): + return reverse("cms.djangoapps.contentstore:v1:xblock-list") + + +def _detail_url(): + return reverse( + "cms.djangoapps.contentstore:v1:xblock-detail", + kwargs={"usage_key_string": TEST_LOCATOR}, + ) + + +# --------------------------------------------------------------------------- +# Routing tests +# --------------------------------------------------------------------------- + + +class XblockViewSetRoutingTest(ModuleStoreTestCase, APITestCase): + """Verify each HTTP method routes to the correct per-verb handler (ADR 0028).""" + + def setUp(self): + super().setUp() + self.staff = GlobalStaffFactory(password='password') + self.client.force_authenticate(user=self.staff) + + @patch(f"{_VIEW_MODULE}.create_xblock_response", return_value=_MOCK_RESPONSE) + def test_post_calls_create_xblock_response(self, mock_fn): + data = {"parent_locator": PARENT_LOCATOR, "category": "html"} + response = self.client.post(_list_url(), data=data, format="json") + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "POST" + + @patch(f"{_VIEW_MODULE}.retrieve_xblock_response", return_value=_MOCK_RESPONSE) + def test_get_calls_retrieve_xblock_response(self, mock_fn): + response = self.client.get(_detail_url()) + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "GET" + + @patch(f"{_VIEW_MODULE}.update_xblock_response", return_value=_MOCK_RESPONSE) + def test_put_calls_update_xblock_response(self, mock_fn): + data = {"id": TEST_LOCATOR, "data": "

Updated

"} + response = self.client.put(_detail_url(), data=data, format="json") + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "PUT" + + @patch(f"{_VIEW_MODULE}.update_xblock_response", return_value=_MOCK_RESPONSE) + def test_patch_calls_update_xblock_response(self, mock_fn): + data = {"id": TEST_LOCATOR, "display_name": "New Name"} + response = self.client.patch(_detail_url(), data=data, format="json") + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "PATCH" + + @patch(f"{_VIEW_MODULE}.delete_xblock_response", return_value=_MOCK_RESPONSE) + def test_delete_calls_delete_xblock_response(self, mock_fn): + response = self.client.delete(_detail_url()) + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "DELETE" + + +# --------------------------------------------------------------------------- +# ADR 0029 error-shape tests +# --------------------------------------------------------------------------- + + +class XblockViewSetErrorShapeTest(ModuleStoreTestCase, APITestCase): + """Verify ADR 0029 standardized error envelope for auth failures.""" + + def setUp(self): + super().setUp() + self.non_author = UserFactory.create(password='password') + + def test_unauthenticated_returns_401(self): + response = self.client.get(_detail_url()) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_unauthenticated_401_has_required_fields(self): + response = self.client.get(_detail_url()) + data = response.json() + for field in _REQUIRED_ERROR_FIELDS: + assert field in data, f"Missing ADR 0029 field: {field}" + + def test_unauthenticated_401_type_uri(self): + response = self.client.get(_detail_url()) + assert response.json()["type"] == "https://docs.openedx.org/errors/authn" + + def test_non_author_returns_403(self): + self.client.force_authenticate(user=self.non_author) + response = self.client.get(_detail_url()) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_non_author_403_has_required_fields(self): + self.client.force_authenticate(user=self.non_author) + response = self.client.get(_detail_url()) + data = response.json() + for field in _REQUIRED_ERROR_FIELDS: + assert field in data, f"Missing ADR 0029 field: {field}" + + def test_non_author_403_type_uri(self): + self.client.force_authenticate(user=self.non_author) + response = self.client.get(_detail_url()) + assert response.json()["type"] == "https://docs.openedx.org/errors/authz" + + def test_error_body_has_no_developer_message(self): + response = self.client.get(_detail_url()) + data = response.json() + assert "developer_message" not in data + assert "error_code" not in data + + def test_instance_field_is_request_path(self): + response = self.client.get(_detail_url()) + assert response.json()["instance"] == _detail_url() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py new file mode 100644 index 000000000000..a3d2e38a3ba7 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py @@ -0,0 +1,119 @@ +""" +API Views for Studio xblock CRUD — v1. + +Standardizes the v0 XblockView + XblockCreateView pair into a single +XblockViewSet applying the FC-0118 ADRs: + + * ADR 0025 - serializer_class + * ADR 0026 - explicit authentication_classes + permission_classes + * ADR 0028 - consolidated into XblockViewSet via DefaultRouter + * ADR 0029 - standardized error envelope via StandardizedErrorMixin +""" +import json +import logging + +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import UsageKey +from rest_framework import viewsets +from rest_framework.permissions import IsAuthenticated + +from cms.djangoapps.contentstore.rest_api.mixins import StandardizedErrorMixin +from cms.djangoapps.contentstore.rest_api.v0.serializers import XblockSerializer +from cms.djangoapps.contentstore.rest_api.v0.views.utils import validate_request_with_serializer +from cms.djangoapps.contentstore.rest_api.v1.views.permissions import HasCourseAuthorAccess +from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import ( + create_xblock_response, + delete_xblock_response, + retrieve_xblock_response, + update_xblock_response, +) +from common.djangoapps.util.json_request import expect_json_in_class_view + +log = logging.getLogger(__name__) + + +class XblockViewSet(StandardizedErrorMixin, viewsets.ViewSet): + """ + ViewSet for xblock CRUD operations (v1 — ADR 0028). + + Router-generated URLs: + POST /api/contentstore/v1/xblock/ → create + GET /api/contentstore/v1/xblock/{usage_key_string}/ → retrieve + PUT /api/contentstore/v1/xblock/{usage_key_string}/ → update + PATCH /api/contentstore/v1/xblock/{usage_key_string}/ → partial_update + DELETE /api/contentstore/v1/xblock/{usage_key_string}/ → destroy + """ + + authentication_classes = ( + JwtAuthentication, + SessionAuthenticationAllowInactiveUser, + ) + permission_classes = (IsAuthenticated, HasCourseAuthorAccess) + serializer_class = XblockSerializer + lookup_field = "usage_key_string" + lookup_value_regex = r'(?:i4x://?[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+)' + + def __init__(self, **kwargs): + super().__init__(**kwargs) + self.course_key = None + + def initial(self, request, *args, **kwargs): + """ + Derive course_key and store it as self.course_key before DRF runs + permission checks. + + Detail actions (GET/PUT/PATCH/DELETE): course_key is extracted from + the usage key embedded in the URL. + Create action (POST): course_key is extracted from parent_locator in + the raw request body. We read request._request.body (Django's cached + bytes) rather than request.data to avoid consuming the WSGI stream + before @expect_json_in_class_view runs. + """ + usage_key_string = kwargs.get("usage_key_string") + if usage_key_string: + try: + self.course_key = UsageKey.from_string(usage_key_string).course_key + except InvalidKeyError: + self.course_key = None + else: + try: + # pylint: disable=protected-access + body = json.loads(request._request.body or b'{}') + parent_locator = body.get("parent_locator", "") + self.course_key = ( + UsageKey.from_string(parent_locator).course_key + if parent_locator else None + ) + except (ValueError, InvalidKeyError): + self.course_key = None + super().initial(request, *args, **kwargs) + + @expect_json_in_class_view + @validate_request_with_serializer + def create(self, request): + """Create a new xblock under the given parent.""" + return create_xblock_response(request) + + @expect_json_in_class_view + def retrieve(self, request, usage_key_string=None): + """Retrieve an xblock by its usage key.""" + return retrieve_xblock_response(request, usage_key_string) + + @expect_json_in_class_view + @validate_request_with_serializer + def update(self, request, usage_key_string=None): + """Fully update an xblock.""" + return update_xblock_response(request, usage_key_string) + + @expect_json_in_class_view + @validate_request_with_serializer + def partial_update(self, request, usage_key_string=None): + """Partially update an xblock.""" + return update_xblock_response(request, usage_key_string) + + @expect_json_in_class_view + def destroy(self, request, usage_key_string=None): + """Delete an xblock.""" + return delete_xblock_response(request, usage_key_string) diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 4f5e1ccb4244..e558c42735d1 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -305,46 +305,7 @@ def handle_xblock(request, usage_key_string=None): elif request.method in ("PUT", "POST"): if "duplicate_source_locator" in request.json: - parent_usage_key = usage_key_with_run(request.json["parent_locator"]) - duplicate_source_usage_key = usage_key_with_run( - request.json["duplicate_source_locator"] - ) - source_course = duplicate_source_usage_key.course_key - dest_course = parent_usage_key.course_key # noqa: F841 - - # Check authz permission for destination - permission = _check_xblock_permission(request, parent_usage_key) - # Legacy path also requires read access on the source course - if permission is None: - if not has_studio_read_access(request.user, source_course): - raise PermissionDenied() - - # Libraries have a maximum component limit enforced on them - if isinstance( - parent_usage_key, LibraryUsageLocator - ) and _is_library_component_limit_reached(parent_usage_key): - return JsonResponse( - { - "error": _( - "Libraries cannot have more than {limit} components" - ).format(limit=settings.MAX_BLOCKS_PER_CONTENT_LIBRARY) - }, - status=400, - ) - - dest_usage_key = duplicate_block( - parent_usage_key, - duplicate_source_usage_key, - request.user, - display_name=request.json.get('display_name'), - ) - - return JsonResponse( - { - "locator": str(dest_usage_key), - "courseKey": str(dest_usage_key.course_key), - } - ) + return _duplicate_xblock(request) else: return _create_block(request) elif request.method == "PATCH": @@ -378,6 +339,122 @@ def handle_xblock(request, usage_key_string=None): ) +# --------------------------------------------------------------------------- +# Public per-verb helpers for XblockViewSet (v1 — ADR 0028) +# These replace handle_xblock's internal method-dispatch so each ViewSet action +# calls the correct logic directly, satisfying REST semantics. +# --------------------------------------------------------------------------- + +def _duplicate_xblock(request): + """ + Shared duplicate-block logic used by both handle_xblock and create_xblock_response. + + Expects request.json to contain ``parent_locator`` and ``duplicate_source_locator``. + """ + parent_usage_key = usage_key_with_run(request.json["parent_locator"]) + duplicate_source_usage_key = usage_key_with_run( + request.json["duplicate_source_locator"] + ) + source_course = duplicate_source_usage_key.course_key + + # Check authz permission for destination + permission = _check_xblock_permission(request, parent_usage_key) + # Legacy path also requires read access on the source course + if permission is None: + if not has_studio_read_access(request.user, source_course): + raise PermissionDenied() + + # Libraries have a maximum component limit enforced on them + if isinstance( + parent_usage_key, LibraryUsageLocator + ) and _is_library_component_limit_reached(parent_usage_key): + return JsonResponse( + { + "error": _( + "Libraries cannot have more than {limit} components" + ).format(limit=settings.MAX_BLOCKS_PER_CONTENT_LIBRARY) + }, + status=400, + ) + + dest_usage_key = duplicate_block( + parent_usage_key, + duplicate_source_usage_key, + request.user, + display_name=request.json.get('display_name'), + ) + return JsonResponse( + { + "locator": str(dest_usage_key), + "courseKey": str(dest_usage_key.course_key), + } + ) + + +def create_xblock_response(request): + """ + Public entry point for POST (create). Called by XblockViewSet.create. + + Handles both duplication (duplicate_source_locator present) and normal creation. + """ + if "duplicate_source_locator" in request.json: + return _duplicate_xblock(request) + return _create_block_core(request) + + +def retrieve_xblock_response(request, usage_key_string): + """ + Public entry point for GET on a specific xblock. Called by XblockViewSet.retrieve. + """ + usage_key = usage_key_with_run(usage_key_string) + _check_xblock_permission(request, usage_key) + + accept_header = request.META.get("HTTP_ACCEPT", "application/json") + if "application/json" not in accept_header: + return HttpResponse(status=406) + + fields = request.GET.get("fields", "").split(",") + if "graderType" in fields: + return JsonResponse(CourseGradingModel.get_section_grader_type(usage_key)) + if "ancestorInfo" in fields: + xblock = get_xblock(usage_key, request.user) + return JsonResponse(_create_xblock_ancestor_info(xblock, is_concise=True)) + with modulestore().bulk_operations(usage_key.course_key): + response = get_block_info(get_xblock(usage_key, request.user)) + if "customReadToken" in fields: + parent_children = _get_block_parent_children(get_xblock(usage_key, request.user)) + response.update(parent_children) + return JsonResponse(response) + + +def update_xblock_response(request, usage_key_string): + """ + Public entry point for PUT/PATCH on a specific xblock. Called by XblockViewSet.update/partial_update. + + For PATCH with ``move_source_locator``, routes to the move-item path (permission check + is against the target parent, not the URL's usage key). + """ + if request.method == "PATCH" and "move_source_locator" in request.json: + move_source_usage_key = usage_key_with_run(request.json.get("move_source_locator")) + target_parent_usage_key = usage_key_with_run(request.json.get("parent_locator")) + target_index = request.json.get("target_index") + _check_xblock_permission(request, target_parent_usage_key) + return _move_item(move_source_usage_key, target_parent_usage_key, request.user, target_index) + usage_key = usage_key_with_run(usage_key_string) + _check_xblock_permission(request, usage_key) + return modify_xblock(usage_key, request) + + +def delete_xblock_response(request, usage_key_string): + """ + Public entry point for DELETE on a specific xblock. Called by XblockViewSet.destroy. + """ + usage_key = usage_key_with_run(usage_key_string) + _check_xblock_permission(request, usage_key) + _delete_item(usage_key, request.user) + return JsonResponse() + + def modify_xblock(usage_key, request): request_data = request.json return _save_xblock( @@ -746,10 +823,12 @@ def sync_library_content( return static_file_notices -@login_required -@expect_json -def _create_block(request): - """View for create blocks.""" +def _create_block_core(request): + """ + Core xblock creation logic, usable without @login_required / @expect_json decorators. + + Called by both _create_block (legacy view) and create_xblock_response (v1 ViewSet). + """ parent_locator = request.json["parent_locator"] usage_key = usage_key_with_run(parent_locator) category = request.json.get("category") @@ -836,6 +915,13 @@ def _create_block(request): return JsonResponse(response) +@login_required +@expect_json +def _create_block(request): + """View for create blocks.""" + return _create_block_core(request) + + def _get_source_index(source_usage_key, source_parent): """ Get source index position of the XBlock. From ad74c564d19ecb1d9929d541fdc0a7908891f1c4 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed <68893403+taimoor-ahmed-1@users.noreply.github.com> Date: Wed, 10 Jun 2026 17:06:21 +0500 Subject: [PATCH 06/13] feat: apply ADRs standardization to enrollment apis (#38724) --- .../contentstore/rest_api/mixins.py | 29 - .../contentstore/rest_api/v1/views/xblock.py | 2 +- .../rest_api/v3/tests/test_home.py | 2 +- .../contentstore/rest_api/v3/views/home.py | 2 +- .../contentstore/rest_api/v4/views/home.py | 2 +- lms/urls.py | 1 + .../djangoapps/enrollments/v2/__init__.py | 0 .../core/djangoapps/enrollments/v2/forms.py | 128 ++++ .../djangoapps/enrollments/v2/paginators.py | 29 + .../djangoapps/enrollments/v2/serializers.py | 34 + .../enrollments/v2/tests/__init__.py | 0 .../enrollments/v2/tests/test_envelope.py | 129 ++++ .../v2/tests/test_view_services.py | 87 +++ .../enrollments/v2/tests/test_views.py | 236 +++++++ .../core/djangoapps/enrollments/v2/urls.py | 73 ++ .../enrollments/v2/view_services.py | 376 +++++++++++ .../core/djangoapps/enrollments/v2/views.py | 626 ++++++++++++++++++ openedx/core/lib/api/mixins.py | 23 + 18 files changed, 1746 insertions(+), 33 deletions(-) delete mode 100644 cms/djangoapps/contentstore/rest_api/mixins.py create mode 100644 openedx/core/djangoapps/enrollments/v2/__init__.py create mode 100644 openedx/core/djangoapps/enrollments/v2/forms.py create mode 100644 openedx/core/djangoapps/enrollments/v2/paginators.py create mode 100644 openedx/core/djangoapps/enrollments/v2/serializers.py create mode 100644 openedx/core/djangoapps/enrollments/v2/tests/__init__.py create mode 100644 openedx/core/djangoapps/enrollments/v2/tests/test_envelope.py create mode 100644 openedx/core/djangoapps/enrollments/v2/tests/test_view_services.py create mode 100644 openedx/core/djangoapps/enrollments/v2/tests/test_views.py create mode 100644 openedx/core/djangoapps/enrollments/v2/urls.py create mode 100644 openedx/core/djangoapps/enrollments/v2/view_services.py create mode 100644 openedx/core/djangoapps/enrollments/v2/views.py diff --git a/cms/djangoapps/contentstore/rest_api/mixins.py b/cms/djangoapps/contentstore/rest_api/mixins.py deleted file mode 100644 index 61f1ff82b4b6..000000000000 --- a/cms/djangoapps/contentstore/rest_api/mixins.py +++ /dev/null @@ -1,29 +0,0 @@ -""" -Shared mixins for the contentstore REST API (used across versions). - -Currently provides :class:`StandardizedErrorMixin`, which opts a single -view/viewset into the ADR 0029 error envelope without changing the -project-wide DRF ``EXCEPTION_HANDLER`` setting. -""" -from openedx.core.lib.api.exceptions import standardized_error_exception_handler - - -class StandardizedErrorMixin: - """ - Opt-in mixin that routes DRF exceptions on this view through the ADR 0029 - standardized error-response handler (see - ``openedx.core.lib.api.exceptions.standardized_error_exception_handler``). - - DRF's :class:`rest_framework.views.APIView` calls ``self.get_exception_handler`` - inside ``handle_exception``; overriding that method here lets the view - return the standardized envelope while other endpoints continue to use - whichever handler the project-wide ``EXCEPTION_HANDLER`` setting points at. - - Usage:: - - class MyViewSet(StandardizedErrorMixin, viewsets.ViewSet): - ... - """ - - def get_exception_handler(self): - return standardized_error_exception_handler diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py index a3d2e38a3ba7..acd4a8192ad9 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py @@ -19,7 +19,6 @@ from rest_framework import viewsets from rest_framework.permissions import IsAuthenticated -from cms.djangoapps.contentstore.rest_api.mixins import StandardizedErrorMixin from cms.djangoapps.contentstore.rest_api.v0.serializers import XblockSerializer from cms.djangoapps.contentstore.rest_api.v0.views.utils import validate_request_with_serializer from cms.djangoapps.contentstore.rest_api.v1.views.permissions import HasCourseAuthorAccess @@ -30,6 +29,7 @@ update_xblock_response, ) from common.djangoapps.util.json_request import expect_json_in_class_view +from openedx.core.lib.api.mixins import StandardizedErrorMixin log = logging.getLogger(__name__) diff --git a/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py index 50f14bc361f6..54c08b0a3894 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py @@ -2,7 +2,7 @@ ADR 0029 – Standardized error-response regression tests for HomeViewSet (v3). The ADR 0029 envelope is wired into the v3 viewset via -:class:`cms.djangoapps.contentstore.rest_api.mixins.StandardizedErrorMixin`, +:class:`openedx.core.lib.api.mixins.StandardizedErrorMixin`, which overrides DRF's per-view ``get_exception_handler`` to point at ``openedx.core.lib.api.exceptions.standardized_error_exception_handler``. diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/home.py b/cms/djangoapps/contentstore/rest_api/v3/views/home.py index 6f2bcfe63acb..2e11b191806e 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/home.py @@ -25,13 +25,13 @@ from rest_framework.request import Request from rest_framework.response import Response -from cms.djangoapps.contentstore.rest_api.mixins import StandardizedErrorMixin from cms.djangoapps.contentstore.rest_api.v1.serializers import ( CourseHomeTabSerializer, LibraryTabSerializer, StudioHomeSerializer, ) from cms.djangoapps.contentstore.utils import get_course_context, get_home_context, get_library_context +from openedx.core.lib.api.mixins import StandardizedErrorMixin class HomeViewSet(StandardizedErrorMixin, viewsets.ViewSet): diff --git a/cms/djangoapps/contentstore/rest_api/v4/views/home.py b/cms/djangoapps/contentstore/rest_api/v4/views/home.py index 3013f05e9b28..8f0fedc1cfed 100644 --- a/cms/djangoapps/contentstore/rest_api/v4/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v4/views/home.py @@ -11,11 +11,11 @@ from rest_framework.request import Request from rest_framework.response import Response -from cms.djangoapps.contentstore.rest_api.mixins import StandardizedErrorMixin from cms.djangoapps.contentstore.rest_api.v4.serializers.home import ( CourseHomeTabSerializerV4, ) from cms.djangoapps.contentstore.utils import get_course_context_v2 +from openedx.core.lib.api.mixins import StandardizedErrorMixin class HomePageCoursesPaginator(DefaultPagination): diff --git a/lms/urls.py b/lms/urls.py index caa95569f3e0..280c739c9bce 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -117,6 +117,7 @@ # Enrollment API RESTful endpoints path('api/enrollment/v1/', include('openedx.core.djangoapps.enrollments.urls')), + path('api/enrollment/v2/', include('openedx.core.djangoapps.enrollments.v2.urls')), # Agreements API RESTful endpoints path('api/agreements/v1/', include('openedx.core.djangoapps.agreements.urls')), diff --git a/openedx/core/djangoapps/enrollments/v2/__init__.py b/openedx/core/djangoapps/enrollments/v2/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/enrollments/v2/forms.py b/openedx/core/djangoapps/enrollments/v2/forms.py new file mode 100644 index 000000000000..413bb870bb87 --- /dev/null +++ b/openedx/core/djangoapps/enrollments/v2/forms.py @@ -0,0 +1,128 @@ +""" +Forms for validating user input to the Course Enrollment v2 views. + +ADR 0033 (OEP-68 parameter naming standardization) — accepts both the +preferred parameter names (``course_key``, ``course_keys``) and the legacy +aliases (``course_id``, ``course_ids``). When both are present, the +preferred name wins. Use :meth:`legacy_param_aliases_used` from the view +layer to emit the ADR 0033 ``Deprecation`` HTTP header when a legacy alias +was sent. + +Internally the cleaned_data continues to expose ``course_id`` / +``course_ids`` (the names the queryset code reads) — the form coalesces +the preferred values onto those fields before the rest of validation runs. +""" + +from django.core.exceptions import ValidationError +from django.forms import CharField, Form +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey + +from openedx.core.djangoapps.user_authn.views.registration_form import validate_username + + +class EnrollmentsAdminListForm(Form): + """ + Validates the query string parameters for the v2 admin enrollments list + endpoint (``GET /api/enrollment/v2/enrollments/``). + """ + + MAX_INPUT_COUNT = 100 + # Legacy / OEP-68 alias pairs: (legacy, preferred). + _LEGACY_PARAM_ALIASES = ( + ("course_id", "course_key"), + ("course_ids", "course_keys"), + ) + + username = CharField(required=False) + course_id = CharField(required=False) + course_key = CharField(required=False) + course_ids = CharField(required=False) + course_keys = CharField(required=False) + email = CharField(required=False) + + def __init__(self, query_params, *args, **kwargs): + # Capture the raw param names supplied on the wire (before Django's + # form layer resolves aliases) so :meth:`legacy_param_aliases_used` + # can later report exactly which legacy names were used. + try: + raw_keys = set(query_params.keys()) + except AttributeError: + raw_keys = set() + self._raw_param_names = raw_keys + + # Coalesce OEP-68 preferred names onto the legacy field names so the + # downstream queryset code keeps reading ``course_id`` / ``course_ids`` + # without changes. The preferred name wins when both are sent. + if hasattr(query_params, "copy"): + data = query_params.copy() + else: + data = dict(query_params) + for legacy_name, preferred_name in self._LEGACY_PARAM_ALIASES: + preferred_value = data.get(preferred_name) + if preferred_value: + data[legacy_name] = preferred_value + + super().__init__(data, *args, **kwargs) + + def legacy_param_aliases_used(self): + """ + Return the list of legacy parameter names that were actually present + in the request, in declaration order. The view layer uses this to + emit the ADR 0033 ``Deprecation`` header. + """ + return [ + legacy for legacy, _preferred in self._LEGACY_PARAM_ALIASES + if legacy in self._raw_param_names + ] + + def clean_course_id(self): + """Parse and validate the ``course_id`` (or aliased ``course_key``) parameter.""" + course_id = self.cleaned_data.get("course_id") + if course_id: + try: + return CourseKey.from_string(course_id) + except InvalidKeyError as exc: + raise ValidationError(f"'{course_id}' is not a valid course id.") from exc + return course_id + + def clean_course_ids(self): + """Split the ``course_ids`` CSV (or aliased ``course_keys``) and enforce MAX_INPUT_COUNT.""" + course_ids_csv = self.cleaned_data.get("course_ids") + if course_ids_csv: + course_ids = course_ids_csv.split(",") + if len(course_ids) > self.MAX_INPUT_COUNT: + raise ValidationError( + f"Too many course_ids in a single request - {len(course_ids)}. " + f"A maximum of {self.MAX_INPUT_COUNT} is allowed" + ) + return course_ids + return course_ids_csv + + def clean_username(self): + """Split the ``username`` CSV, validate each entry, and enforce MAX_INPUT_COUNT.""" + usernames_csv = self.cleaned_data.get("username") + if usernames_csv: + usernames = usernames_csv.split(",") + if len(usernames) > self.MAX_INPUT_COUNT: + raise ValidationError( + f"Too many usernames in a single request - {len(usernames)}. " + f"A maximum of {self.MAX_INPUT_COUNT} is allowed" + ) + for username in usernames: + validate_username(username) + return usernames + return usernames_csv + + def clean_email(self): + """Split the ``email`` CSV and enforce MAX_INPUT_COUNT.""" + emails_csv = self.cleaned_data.get("email") + if emails_csv: + emails = emails_csv.split(",") + if len(emails) > self.MAX_INPUT_COUNT: + raise ValidationError( + f"Too many emails in a single request - {len(emails)}. " + f"A maximum of {self.MAX_INPUT_COUNT} is allowed" + ) + return emails + return emails_csv diff --git a/openedx/core/djangoapps/enrollments/v2/paginators.py b/openedx/core/djangoapps/enrollments/v2/paginators.py new file mode 100644 index 000000000000..d8d9eed96c77 --- /dev/null +++ b/openedx/core/djangoapps/enrollments/v2/paginators.py @@ -0,0 +1,29 @@ +""" +Pagination for the Enrollment API — v2. + +ADR 0032 — uses :class:`DefaultPagination` from +``edx-rest-framework-extensions``, which provides the standard 7-field +envelope: ``count``, ``num_pages``, ``current_page``, ``start``, ``next``, +``previous``, ``results``. + +Distinct from v1's :class:`openedx.core.djangoapps.enrollments.paginators.CourseEnrollmentsApiListPagination` +(which is a :class:`CursorPagination` subclass with a 3-field envelope). +v2 introduces the new shape — clients that need the legacy shape stay on +``/api/enrollment/v1/`` until they migrate. +""" + +from edx_rest_framework_extensions.paginators import DefaultPagination + + +class EnrollmentsAdminListPagination(DefaultPagination): + """ + ADR 0032 — standard pagination for the admin enrollments list API + (GET /api/enrollment/v2/enrollments/). + + Defaults sized for an admin-facing bulk-query endpoint: + page_size 100, max 100. + """ + + page_size = 100 + page_size_query_param = "page_size" + max_page_size = 100 diff --git a/openedx/core/djangoapps/enrollments/v2/serializers.py b/openedx/core/djangoapps/enrollments/v2/serializers.py new file mode 100644 index 000000000000..2c7220b398f1 --- /dev/null +++ b/openedx/core/djangoapps/enrollments/v2/serializers.py @@ -0,0 +1,34 @@ +""" +Serializers for the Enrollment API — v2. + +Only contains the serializers introduced by ADR 0025 (replacing inline +dict construction in role-listing endpoints). The other v1 serializers +(:class:`CourseEnrollmentSerializer`, :class:`CourseSerializer`, +:class:`CourseEnrollmentAllowedSerializer`, :class:`CourseEnrollmentsApiListSerializer`) +are unchanged in shape between v1 and v2 — v2 view code imports them +directly from :mod:`openedx.core.djangoapps.enrollments.serializers`. + +If a future v3 needs to break any of those response shapes, fork them +into a new v3/serializers.py at that time. +""" + +from rest_framework import serializers + + +class UserRoleSerializer(serializers.Serializer): # pylint: disable=abstract-method + """Serializes a single course-level role entry for a user (ADR 0025).""" + + org = serializers.CharField() + course_id = serializers.SerializerMethodField() + role = serializers.CharField() + + def get_course_id(self, obj): + """Return course_id as a string.""" + return str(obj.course_id) + + +class UserRolesResponseSerializer(serializers.Serializer): # pylint: disable=abstract-method + """Serializes the full response payload for UserRolesViewSet (ADR 0025).""" + + roles = UserRoleSerializer(many=True) + is_staff = serializers.BooleanField() diff --git a/openedx/core/djangoapps/enrollments/v2/tests/__init__.py b/openedx/core/djangoapps/enrollments/v2/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/enrollments/v2/tests/test_envelope.py b/openedx/core/djangoapps/enrollments/v2/tests/test_envelope.py new file mode 100644 index 000000000000..3c87e8c8008d --- /dev/null +++ b/openedx/core/djangoapps/enrollments/v2/tests/test_envelope.py @@ -0,0 +1,129 @@ +""" +ADR 0029 — Standardized error-response envelope regression tests for the +v2 Enrollment API. + +The envelope is wired into every v2 viewset via +:class:`openedx.core.lib.api.mixins.StandardizedErrorMixin`, which overrides +DRF's per-view ``get_exception_handler`` to point at the project-wide +``standardized_error_exception_handler``. + +The envelope shape is:: + + { + "type": "https://docs.openedx.org/errors/", + "title": "", + "status": , + "detail": "", + "instance": "", + } + +These tests confirm the envelope reaches every v2 endpoint that can produce +a 401 / 403 / 404 / 400. The last test (``test_v1_endpoint_unaffected``) +locks in the scoping — v1 must NOT carry the envelope. +""" +from unittest.mock import patch + +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient, APITestCase + +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangolib.testing.utils import skip_unless_lms + +_REQUIRED_ERROR_FIELDS = ("type", "title", "status", "detail", "instance") + + +@skip_unless_lms +class TestEnrollmentViewSetEnvelope(APITestCase): + """ADR 0029 — envelope on the EnrollmentViewSet 401s.""" + + def setUp(self): + super().setUp() + self.client = APIClient() + self.list_url = reverse("v2:enrollment-list") + self.unenroll_url = reverse("v2:enrollment-unenroll") + self.allowed_url = reverse("v2:enrollment-allowed") + + def test_list_unauthenticated_envelope(self): + response = self.client.get(self.list_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + def test_list_unauthenticated_type_uri(self): + response = self.client.get(self.list_url) + assert response.data.get("type") == "https://docs.openedx.org/errors/authn" + + def test_unenroll_unauthenticated_envelope(self): + response = self.client.post(self.unenroll_url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + def test_allowed_unauthenticated_envelope(self): + response = self.client.get(self.allowed_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + def test_non_admin_get_allowed_envelope(self): + """ADR 0029 — 403 also carries the envelope.""" + self.client.force_authenticate(user=UserFactory.create()) + response = self.client.get(self.allowed_url) + assert response.status_code == status.HTTP_403_FORBIDDEN + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + assert response.data.get("type") == "https://docs.openedx.org/errors/authz" + + def test_create_missing_course_id_envelope(self): + """ADR 0029 — inline ValidationError surfaces with the envelope.""" + self.client.force_authenticate(user=UserFactory.create()) + response = self.client.post(self.list_url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_400_BAD_REQUEST + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + assert response.data.get("type") == "https://docs.openedx.org/errors/validation" + + def test_instance_field_is_request_path(self): + response = self.client.get(self.list_url) + assert response.data.get("instance") == self.list_url + + def test_error_body_has_no_developer_message(self): + """Legacy DeveloperErrorViewMixin fields must not leak through.""" + response = self.client.get(self.list_url) + assert "developer_message" not in response.data + assert "error_code" not in response.data + + +@skip_unless_lms +class TestCourseEnrollmentDetailViewEnvelope(APITestCase): + """ADR 0029 — envelope on the public course-detail endpoint.""" + + def test_invalid_course_key_envelope(self): + url = reverse("v2:enrollment-v2-course-detail", kwargs={"course_id": "course-v1:org+course+run"}) + with patch( + "openedx.core.djangoapps.enrollments.v2.views.CourseOverview.get_from_id", + ) as mock_get: + from openedx.core.djangoapps.content.course_overviews.models import CourseOverview + mock_get.side_effect = CourseOverview.DoesNotExist() + response = self.client.get(url) + assert response.status_code == status.HTTP_404_NOT_FOUND + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + assert response.data.get("type") == "https://docs.openedx.org/errors/not-found" + + +@skip_unless_lms +class TestV1EndpointUnaffected(APITestCase): + """ + The ADR 0029 envelope must be scoped to v2 — v1 endpoints continue to + use whichever handler the project-wide ``EXCEPTION_HANDLER`` setting + points at. Hitting v1 unauthenticated must NOT return the v2 envelope. + """ + + def test_v1_enrollment_list_does_not_carry_envelope(self): + response = self.client.get(reverse("courseenrollments")) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + # v1 still uses the project-default handler; ADR 0029 fields absent. + assert "type" not in response.data + assert "instance" not in response.data diff --git a/openedx/core/djangoapps/enrollments/v2/tests/test_view_services.py b/openedx/core/djangoapps/enrollments/v2/tests/test_view_services.py new file mode 100644 index 000000000000..0bb8fd1ecbfe --- /dev/null +++ b/openedx/core/djangoapps/enrollments/v2/tests/test_view_services.py @@ -0,0 +1,87 @@ +""" +Unit tests for EnrollmentOperationsService (v2). + +Exercises the service methods directly — no HTTP layer involved. Tests the +two-layer authorization model (ADR 0031) and the modern ADR 0029 raise-DRF- +exceptions pattern. +""" +from unittest.mock import MagicMock, patch + +import pytest +from django.test import TestCase +from rest_framework.exceptions import NotFound, ValidationError + +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.enrollments.v2.view_services import EnrollmentOperationsService + + +class TestUnenrollUserForRetirement(TestCase): + """ADR 0029 — error handling for the retirement unenroll flow.""" + + def setUp(self): + super().setUp() + self.service = EnrollmentOperationsService() + + def test_missing_username_raises_validation_error(self): + with pytest.raises(ValidationError): + self.service.unenroll_user_for_retirement(None) + + def test_blank_username_raises_validation_error(self): + with pytest.raises(ValidationError): + self.service.unenroll_user_for_retirement("") + + @patch( + "openedx.core.djangoapps.enrollments.v2.view_services.UserRetirementStatus.get_retirement_for_retirement_action" + ) + def test_unknown_retirement_status_raises_not_found(self, mock_get): + from openedx.core.djangoapps.user_api.models import UserRetirementStatus + mock_get.side_effect = UserRetirementStatus.DoesNotExist() + with pytest.raises(NotFound): + self.service.unenroll_user_for_retirement("ghost-user") + + +class TestListEnrollmentsForUser(TestCase): + """ADR 0031 — per-operation permission filter in the listing helper.""" + + def setUp(self): + super().setUp() + self.service = EnrollmentOperationsService() + self.user = UserFactory.create() + self.other = UserFactory.create() + + @patch("openedx.core.djangoapps.enrollments.v2.view_services.CourseEnrollment.objects") + def test_self_lookup_returns_full_list_unfiltered(self, mock_objects): + """Requesting your own enrollments bypasses the course-staff filter.""" + mock_qs = MagicMock() + mock_qs.__iter__ = lambda self: iter([]) + mock_objects.filter.return_value.select_related.return_value = mock_qs + result = self.service.list_enrollments_for_user( + request_user=self.user, target_username=self.user.username, has_api_key=False, + ) + assert isinstance(result, list) + + @patch("openedx.core.djangoapps.enrollments.v2.view_services.CourseEnrollment.objects") + def test_api_key_bypasses_per_course_filter(self, mock_objects): + """has_api_key=True returns the full list even across user boundaries.""" + mock_qs = MagicMock() + mock_qs.__iter__ = lambda self: iter([]) + mock_objects.filter.return_value.select_related.return_value = mock_qs + result = self.service.list_enrollments_for_user( + request_user=self.user, target_username=self.other.username, has_api_key=True, + ) + assert isinstance(result, list) + + +class TestDeleteAllowedEnrollment(TestCase): + """ADR 0029 — delete raises NotFound when the row is missing.""" + + def setUp(self): + super().setUp() + self.service = EnrollmentOperationsService() + + @patch("openedx.core.djangoapps.enrollments.v2.view_services.CourseEnrollmentAllowed.objects") + def test_delete_missing_row_raises_not_found(self, mock_objects): + from django.core.exceptions import ObjectDoesNotExist + mock_objects.get.side_effect = ObjectDoesNotExist() + with pytest.raises(NotFound): + self.service.delete_allowed_enrollment("ghost@example.com", "course-v1:org+course+run") diff --git a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py new file mode 100644 index 000000000000..ce8f958a1d79 --- /dev/null +++ b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py @@ -0,0 +1,236 @@ +""" +Action + permission regression tests for the v2 Enrollment ViewSet. + +MongoDB-free: every service-layer call is mocked, so these tests run +without a live modulestore or course-overview row. + +Covers: + - ADR 0026: permission enforcement on every action (list/create/unenroll/allowed) + - ADR 0028: router-generated URL reverse names work + - ADR 0032: list action returns the 7-field DefaultPagination envelope + - ADR 0033: ordering whitelist + Deprecation header on the admin list +""" +from unittest.mock import patch + +from django.test import override_settings +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APITestCase + +from common.djangoapps.student.tests.factories import AdminFactory, UserFactory +from openedx.core.djangolib.testing.utils import skip_unless_lms + +API_KEY = "test-enrollment-v2-api-key" + +# Mock targets — all keyed off the v2 module to avoid leaking into v1. +MOCK_OPS_LIST = "openedx.core.djangoapps.enrollments.v2.views._OPS.list_enrollments_for_user" +MOCK_OPS_CREATE = "openedx.core.djangoapps.enrollments.v2.views._OPS.create_or_update_enrollment" +MOCK_OPS_UNENROLL = "openedx.core.djangoapps.enrollments.v2.views._OPS.unenroll_user_for_retirement" +MOCK_OPS_LIST_ALLOWED = "openedx.core.djangoapps.enrollments.v2.views._OPS.list_allowed_for_email" +MOCK_OPS_CREATE_ALLOWED = "openedx.core.djangoapps.enrollments.v2.views._OPS.create_allowed_enrollment" +MOCK_OPS_DELETE_ALLOWED = "openedx.core.djangoapps.enrollments.v2.views._OPS.delete_allowed_enrollment" + + +# --------------------------------------------------------------------------- +# EnrollmentViewSet.list (GET /enrollment/) +# --------------------------------------------------------------------------- + +@skip_unless_lms +class TestEnrollmentViewSetList(APITestCase): + """ADR 0026 + 0028 — permission + reverse-name tests for the list action.""" + + def setUp(self): + super().setUp() + self.user = UserFactory.create(password="test") + self.url = reverse("v2:enrollment-list") + + def test_unauthenticated_gets_401(self): + response = self.client.get(self.url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + @patch(MOCK_OPS_LIST, return_value=[]) + def test_authenticated_user_gets_200(self, mock_list): # noqa: ARG002 + self.client.force_authenticate(user=self.user) + response = self.client.get(self.url) + assert response.status_code == status.HTTP_200_OK + + @patch(MOCK_OPS_LIST, return_value=[]) + def test_valid_api_key_gets_200(self, mock_list): # noqa: ARG002 + with override_settings(EDX_API_KEY=API_KEY): + response = self.client.get(self.url, HTTP_X_EDX_API_KEY=API_KEY) + assert response.status_code == status.HTTP_200_OK + + def test_invalid_api_key_without_session_gets_401(self): + response = self.client.get(self.url, HTTP_X_EDX_API_KEY="wrong-key") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + @patch(MOCK_OPS_LIST, return_value=[]) + def test_list_returns_pagination_envelope(self, mock_list): # noqa: ARG002 + """ADR 0032 — every response carries the 7-field DefaultPagination envelope.""" + self.client.force_authenticate(user=self.user) + response = self.client.get(self.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, f"ADR 0032: missing envelope field '{field}'" + + +# --------------------------------------------------------------------------- +# EnrollmentViewSet.create (POST /enrollment/) +# --------------------------------------------------------------------------- + +@skip_unless_lms +class TestEnrollmentViewSetCreate(APITestCase): + """ADR 0026 + 0028 — permission tests for the create action.""" + + def setUp(self): + super().setUp() + self.user = UserFactory.create(password="test") + self.url = reverse("v2:enrollment-list") + + def test_unauthenticated_post_gets_401(self): + response = self.client.post(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_authenticated_post_missing_course_id_gets_400(self): + """ADR 0029 — missing course_id raises ValidationError → 400.""" + self.client.force_authenticate(user=self.user) + response = self.client.post(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_authenticated_post_invalid_course_id_gets_400(self): + """ADR 0029 — unparseable course_id raises ValidationError → 400.""" + self.client.force_authenticate(user=self.user) + response = self.client.post( + self.url, + data={"course_details": {"course_id": "not-a-course-key"}}, + content_type="application/json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + @patch(MOCK_OPS_CREATE, return_value={"mode": "audit", "is_active": True}) + def test_authenticated_post_valid_returns_200(self, mock_create): # noqa: ARG002 + self.client.force_authenticate(user=self.user) + response = self.client.post( + self.url, + data={"course_details": {"course_id": "course-v1:org+course+run"}}, + content_type="application/json", + ) + assert response.status_code == status.HTTP_200_OK + + +# --------------------------------------------------------------------------- +# EnrollmentViewSet.unenroll (POST /enrollment/unenroll/) +# --------------------------------------------------------------------------- + +@skip_unless_lms +class TestEnrollmentViewSetUnenroll(APITestCase): + """ADR 0026 — IsAuthenticated + CanRetireUser permission.""" + + def setUp(self): + super().setUp() + self.user = UserFactory.create(password="test") + self.url = reverse("v2:enrollment-unenroll") + + def test_unauthenticated_gets_401(self): + response = self.client.post( + self.url, data={"username": self.user.username}, content_type="application/json", + ) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_authenticated_non_retirement_user_gets_403(self): + """A plain authenticated user lacks CanRetireUser → 403.""" + self.client.force_authenticate(user=self.user) + response = self.client.post( + self.url, data={"username": self.user.username}, content_type="application/json", + ) + assert response.status_code == status.HTTP_403_FORBIDDEN + + +# --------------------------------------------------------------------------- +# EnrollmentViewSet.allowed (GET/POST/DELETE /enrollment/enrollment_allowed/) +# --------------------------------------------------------------------------- + +@skip_unless_lms +class TestEnrollmentViewSetAllowed(APITestCase): + """ADR 0026 — IsAdminUser permission on the allowed action.""" + + def setUp(self): + super().setUp() + self.user = UserFactory.create(password="test") + self.admin = AdminFactory.create(password="test") + self.url = reverse("v2:enrollment-allowed") + + def test_unauthenticated_get_gets_401(self): + response = self.client.get(self.url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_non_admin_get_gets_403(self): + self.client.force_authenticate(user=self.user) + response = self.client.get(self.url) + assert response.status_code == status.HTTP_403_FORBIDDEN + + @patch(MOCK_OPS_LIST_ALLOWED, return_value=[]) + def test_admin_get_gets_200(self, mock_list): # noqa: ARG002 + self.client.force_authenticate(user=self.admin) + response = self.client.get(self.url) + assert response.status_code == status.HTTP_200_OK + assert response.data == [] + + def test_non_admin_post_gets_403(self): + self.client.force_authenticate(user=self.user) + response = self.client.post( + self.url, + data={"email": "test@example.com", "course_id": "course-v1:edX+DemoX+Demo_Course"}, + content_type="application/json", + ) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_non_admin_delete_gets_403(self): + self.client.force_authenticate(user=self.user) + response = self.client.delete( + self.url, + data={"email": "test@example.com", "course_id": "course-v1:edX+DemoX+Demo_Course"}, + content_type="application/json", + ) + assert response.status_code == status.HTTP_403_FORBIDDEN + + +# --------------------------------------------------------------------------- +# UserRolesView (GET /roles/) — ADR 0033 OEP-68 aliasing +# --------------------------------------------------------------------------- + +_ADR_0033_HEADER_COURSE_ID = ( + "Parameter 'course_id' is deprecated. Use 'course_key' instead. " + "Support will be removed in release ''." +) + + +@skip_unless_lms +class TestUserRolesViewAliases(APITestCase): + """ADR 0033 — OEP-68 parameter alias + Deprecation header tests.""" + + def setUp(self): + super().setUp() + self.user = UserFactory.create(password="test") + self.url = reverse("v2:enrollment-v2-roles") + + @patch("openedx.core.djangoapps.enrollments.v2.views.api.get_user_roles", return_value=[]) + def test_new_course_key_param_no_header(self, mock_get): # noqa: ARG002 + self.client.force_authenticate(user=self.user) + response = self.client.get(self.url, {"course_key": "course-v1:org+course+run"}) + assert response.status_code == status.HTTP_200_OK + assert "Deprecation" not in response.headers + + @patch("openedx.core.djangoapps.enrollments.v2.views.api.get_user_roles", return_value=[]) + def test_legacy_course_id_param_emits_header(self, mock_get): # noqa: ARG002 + self.client.force_authenticate(user=self.user) + response = self.client.get(self.url, {"course_id": "course-v1:org+course+run"}) + assert response.status_code == status.HTTP_200_OK + assert response.headers.get("Deprecation") == _ADR_0033_HEADER_COURSE_ID + + @patch("openedx.core.djangoapps.enrollments.v2.views.api.get_user_roles", return_value=[]) + def test_no_filter_no_header(self, mock_get): # noqa: ARG002 + self.client.force_authenticate(user=self.user) + response = self.client.get(self.url) + assert response.status_code == status.HTTP_200_OK + assert "Deprecation" not in response.headers diff --git a/openedx/core/djangoapps/enrollments/v2/urls.py b/openedx/core/djangoapps/enrollments/v2/urls.py new file mode 100644 index 000000000000..cda839fd4319 --- /dev/null +++ b/openedx/core/djangoapps/enrollments/v2/urls.py @@ -0,0 +1,73 @@ +""" +URLs for the Enrollment API — v2. + +Mounted at ``/api/enrollment/v2/`` (see ``lms/urls.py``). + +ADR 0028 — :class:`EnrollmentViewSet` is registered via ``DefaultRouter`` +(actions: ``list``, ``create``, ``unenroll``, ``allowed``). The other v2 +endpoints (singleton retrieve by URL form, roles, course-detail-by-id, +admin enrollments list) cannot be expressed as router-generated URLs, so +they remain as standalone ``APIView`` classes routed via ``path()`` / +``re_path()``. + +URL surface +----------- + +Router-generated (basename ``enrollment``): + GET /enrollment/ + POST /enrollment/ + POST /enrollment/unenroll/ + GET /enrollment/enrollment_allowed/ + POST /enrollment/enrollment_allowed/ + DELETE /enrollment/enrollment_allowed/ + +Explicit paths: + GET /enrollment/{username},{course_key} (name: enrollment-v2-retrieve) + GET /enrollment/{course_key} (name: enrollment-v2-retrieve) + GET /enrollments/ (name: enrollment-v2-admin-list) + GET /course/{course_key} (name: enrollment-v2-course-detail) + GET /roles/ (name: enrollment-v2-roles) +""" + +from django.conf import settings +from django.urls import path, re_path +from rest_framework.routers import DefaultRouter + +from .views import ( + CourseEnrollmentDetailView, + EnrollmentRetrieveView, + EnrollmentsAdminListView, + EnrollmentViewSet, + UserRolesView, +) + +app_name = "v2" + +router = DefaultRouter() +router.register(r"enrollment", EnrollmentViewSet, basename="enrollment") + +urlpatterns = router.urls + [ + re_path( + r"^enrollment/{username},{course_key}$".format( # noqa: UP032 + username=settings.USERNAME_PATTERN, course_key=settings.COURSE_ID_PATTERN, + ), + EnrollmentRetrieveView.as_view(), + name="enrollment-v2-retrieve", + ), + re_path( + rf"^enrollment/{settings.COURSE_ID_PATTERN}$", + EnrollmentRetrieveView.as_view(), + name="enrollment-v2-retrieve", + ), + re_path( + r"^enrollments/?$", + EnrollmentsAdminListView.as_view(), + name="enrollment-v2-admin-list", + ), + re_path( + rf"^course/{settings.COURSE_ID_PATTERN}$", + CourseEnrollmentDetailView.as_view(), + name="enrollment-v2-course-detail", + ), + path("roles/", UserRolesView.as_view(), name="enrollment-v2-roles"), +] diff --git a/openedx/core/djangoapps/enrollments/v2/view_services.py b/openedx/core/djangoapps/enrollments/v2/view_services.py new file mode 100644 index 000000000000..75c7de0dcec3 --- /dev/null +++ b/openedx/core/djangoapps/enrollments/v2/view_services.py @@ -0,0 +1,376 @@ +""" +Shared service layer for enrollment v2 HTTP operations. + +ADR 0031 (Merge Similar Endpoints) — consolidates the business logic +behind the three v2 viewset actions that previously had partially duplicated +implementations in v1's ``EnrollmentListView`` / ``UnenrollmentView`` / +``EnrollmentAllowedView``. + +Authorization model +------------------- +Each operation is enforced in two layers: + +1. The viewset declares a coarse permission class (``IsAuthenticated``, + ``IsAdminUser``, ``CanRetireUser``, ``ApiKeyHeaderPermissionIsAuthenticated``) + on the action. +2. The service method enforces the per-operation rules — e.g. only API-key + callers or global staff may deactivate enrollments, downgrade modes, or + force-enroll a user. + +ADR 0029 — service methods raise DRF exceptions (``NotFound``, +``ValidationError``, ``PermissionDenied``, ``Conflict``) instead of returning +``Response`` objects with non-2xx status. The exceptions flow through the +viewset's :class:`StandardizedErrorMixin` to produce the standardized +envelope. +""" + +import logging + +from django.contrib.auth import get_user_model +from django.core.exceptions import ObjectDoesNotExist +from rest_framework.exceptions import ( + APIException, + NotFound, + PermissionDenied, + ValidationError, +) + +from common.djangoapps.course_modes.models import CourseMode +from common.djangoapps.student.auth import user_has_role +from common.djangoapps.student.models import CourseEnrollment, CourseEnrollmentAllowed, EnrollmentNotAllowed +from common.djangoapps.student.roles import CourseStaffRole, GlobalStaff +from openedx.core.djangoapps.course_groups.cohorts import CourseUserGroup, add_user_to_cohort, get_cohort_by_name +from openedx.core.djangoapps.embargo import api as embargo_api +from openedx.core.djangoapps.enrollments import api +from openedx.core.djangoapps.enrollments.errors import ( + CourseEnrollmentError, + CourseEnrollmentExistsError, + CourseModeNotFoundError, + InvalidEnrollmentAttribute, +) +from openedx.core.djangoapps.user_api.models import UserRetirementStatus +from openedx.core.djangoapps.user_api.preferences.api import update_email_opt_in +from openedx.core.lib.api.exceptions import Conflict +from openedx.core.lib.exceptions import CourseNotFoundError +from openedx.core.lib.log_utils import audit_log +from openedx.features.enterprise_support.api import ( + ConsentApiServiceClient, + EnterpriseApiException, + EnterpriseApiServiceClient, + enterprise_enabled, +) + +log = logging.getLogger(__name__) + +User = get_user_model() + +REQUIRED_ATTRIBUTES = { + "credit": ["credit:provider_id"], +} + + +class EnrollmentOperationsService: + """ + Operation handlers for the v2 EnrollmentViewSet. + + All methods raise DRF exceptions on error paths so the viewset's + :class:`StandardizedErrorMixin` can produce the ADR 0029 envelope. + """ + + # ------------------------------------------------------------------ + # Listing + # ------------------------------------------------------------------ + def list_enrollments_for_user(self, request_user, target_username, has_api_key): + """ + Return enrollments visible to ``request_user`` for ``target_username``. + + - Self / global staff / api-key requests → full list. + - Otherwise filtered to courses ``request_user`` staffs. + """ + enrollments = CourseEnrollment.objects.filter( + user__username=target_username + ).select_related("user", "course") + if ( + target_username == request_user.username + or GlobalStaff().has_user(request_user) + or has_api_key + ): + return list(enrollments) + return [ + enrollment for enrollment in enrollments + if user_has_role(request_user, CourseStaffRole(enrollment.course_id)) + ] + + # ------------------------------------------------------------------ + # Create / update + # ------------------------------------------------------------------ + def create_or_update_enrollment(self, request, has_api_key, course_id): + """ + Handle the POST /enrollment/ create-or-update flow. + + ``course_id`` is a parsed :class:`CourseKey`. The viewset is + responsible for the up-front ``InvalidKeyError → ValidationError`` + translation before calling this method. + + Returns the enrollment dict on success. Raises DRF exceptions on + any error path. + """ + # pylint: disable=too-many-statements,too-many-branches + username = request.data.get("user") + mode = request.data.get("mode") + is_active = None + user = None + cohort_name = None + + # Per-operation authz layer 1: only admin/api-key callers may enroll + # other users. Non-staff callers can only enroll themselves. + if ( + username + and username != request.user.username + and not has_api_key + and not GlobalStaff().has_user(request.user) + ): + raise NotFound() + + if not username: + email = request.data.get("email") + if email: + if not has_api_key and not GlobalStaff().has_user(request.user): + raise NotFound() + try: + username = User.objects.get(email=email).username + except ObjectDoesNotExist as exc: + raise NotFound( + f"The user with the email address {email} does not exist." + ) from exc + else: + username = request.user.username + + # Per-operation authz layer 2: non-default modes require api-key or + # global-staff privileges. + if ( + mode not in (CourseMode.AUDIT, CourseMode.HONOR, None) + and not has_api_key + and not GlobalStaff().has_user(request.user) + ): + raise PermissionDenied( + f"User does not have permission to create enrollment with mode [{mode}]." + ) + + try: + user = User.objects.get(username=username) + except ObjectDoesNotExist as exc: + raise NotFound(f"The user {username} does not exist.") from exc + + embargo_response = embargo_api.get_embargo_response(request, course_id, user) + if embargo_response: + # Embargo returns a fully-formed Response; surface its body as a + # PermissionDenied so the standardized envelope wraps it. + raise PermissionDenied(detail=getattr(embargo_response, "data", "Embargoed.")) + + try: + is_active = request.data.get("is_active") + if is_active is not None and not isinstance(is_active, bool): + raise ValidationError(f"'{is_active}' is an invalid enrollment activation status.") + + explicit_linked_enterprise = request.data.get("linked_enterprise_customer") + if explicit_linked_enterprise and has_api_key and enterprise_enabled(): + enterprise_api_client = EnterpriseApiServiceClient() + consent_client = ConsentApiServiceClient() + try: + enterprise_api_client.post_enterprise_course_enrollment(username, str(course_id)) + except EnterpriseApiException as error: + log.exception( + "An unexpected error occurred while creating the new EnterpriseCourseEnrollment " + "for user [%s] in course run [%s]", username, course_id, + ) + raise CourseEnrollmentError(str(error)) from error + consent_client.provide_consent( + username=username, + course_id=str(course_id), + enterprise_customer_uuid=explicit_linked_enterprise, + ) + + enrollment_attributes = request.data.get("enrollment_attributes") + force_enrollment = request.data.get("force_enrollment") + if force_enrollment is not None and not isinstance(force_enrollment, bool): + raise ValidationError(f"'{force_enrollment}' is an invalid force enrollment status.") + force_enrollment = force_enrollment and GlobalStaff().has_user(request.user) + + enrollment = api.get_enrollment(username, str(course_id)) + mode_changed = enrollment and mode is not None and enrollment["mode"] != mode + active_changed = enrollment and is_active is not None and enrollment["is_active"] != is_active + missing_attrs = [] + if enrollment_attributes: + actual_attrs = ["{namespace}:{name}".format(**attr) for attr in enrollment_attributes] + missing_attrs = set(REQUIRED_ATTRIBUTES.get(mode, [])) - set(actual_attrs) + + if (GlobalStaff().has_user(request.user) or has_api_key) and (mode_changed or active_changed): + if mode_changed and active_changed and not is_active: + msg = ( + f"Enrollment mode mismatch: active mode={enrollment['mode']}, " + f"requested mode={mode}. Won't deactivate." + ) + log.warning(msg) + raise ValidationError(msg) + + if missing_attrs: + msg = ( + f"Missing enrollment attributes: requested mode={mode} " + f"required attributes={REQUIRED_ATTRIBUTES.get(mode)}" + ) + log.warning(msg) + raise ValidationError(msg) + + response_data = api.update_enrollment( + username, + str(course_id), + mode=mode, + is_active=is_active, + enrollment_attributes=enrollment_attributes, + include_expired=has_api_key, + ) + else: + response_data = api.add_enrollment( + username, + str(course_id), + mode=mode, + is_active=is_active, + enrollment_attributes=enrollment_attributes, + enterprise_uuid=request.data.get("enterprise_uuid"), + force_enrollment=force_enrollment, + include_expired=force_enrollment, + ) + + cohort_name = request.data.get("cohort") + if cohort_name is not None: + cohort = get_cohort_by_name(course_id, cohort_name) + try: + add_user_to_cohort(cohort, user) + except ValueError: + log.exception("Cohort re-addition") + + email_opt_in = request.data.get("email_opt_in", None) + if email_opt_in is not None: + update_email_opt_in(request.user, course_id.org, email_opt_in) + + log.info("The user [%s] has already been enrolled in course run [%s].", username, course_id) + return response_data + + except InvalidEnrollmentAttribute as error: + raise ValidationError(str(error)) from error + except EnrollmentNotAllowed as error: + raise PermissionDenied(str(error)) from error + except CourseModeNotFoundError as error: + raise ValidationError( + f"The [{mode}] course mode is expired or otherwise unavailable for course run [{course_id}]." + ) from error + except CourseNotFoundError as error: + raise ValidationError(f"No course '{course_id}' found for enrollment") from error + except CourseEnrollmentExistsError as error: + log.warning("An enrollment already exists for user [%s] in course run [%s].", username, course_id) + # Caller-visible signal that the enrollment already exists. Use 200 + existing enrollment body + # (matches v1 semantics) — surface as a successful return, not an exception. + return error.enrollment + except CourseEnrollmentError as error: + log.exception( + "An error occurred while creating the new course enrollment for user [%s] in course run [%s]", + username, course_id, + ) + raise ValidationError( + f"An error occurred while creating the new course enrollment " + f"for user '{username}' in course '{course_id}'" + ) from error + except CourseUserGroup.DoesNotExist as error: + log.exception("Missing cohort [%s] in course run [%s]", cohort_name, course_id) + raise ValidationError( + f"An error occured while adding to cohort [{cohort_name}]" + ) from error + finally: + # Audit-log every API-key-driven enrollment change. + if has_api_key and user is not None: + try: + current = CourseEnrollment.objects.get(user__username=username, course_id=course_id) + actual_mode = current.mode + actual_activation = current.is_active + except CourseEnrollment.DoesNotExist: + actual_mode = None + actual_activation = None + audit_log( + "enrollment_change_requested", + course_id=str(course_id), + requested_mode=mode, + actual_mode=actual_mode, + requested_activation=is_active, + actual_activation=actual_activation, + user_id=user.id, + ) + + # ------------------------------------------------------------------ + # Unenroll (retirement pipeline) + # ------------------------------------------------------------------ + def unenroll_user_for_retirement(self, username): + """ + Handle the retirement-pipeline /enrollment/unenroll/ flow. + + Returns: + - ``None`` if the user has no active enrollments (caller should + return 204 No Content). + - A dict (the unenroll-result payload) on success (caller returns 200). + + Raises: + ValidationError: if ``username`` is missing. + NotFound: if no retirement-status row exists for the user. + APIException: on any other unexpected error (mapped to 500). + """ + if not username: + raise ValidationError("Username not specified.") + try: + UserRetirementStatus.get_retirement_for_retirement_action(username) + except UserRetirementStatus.DoesNotExist as exc: + raise NotFound("No retirement request status for username.") from exc + try: + if not CourseEnrollment.objects.filter(user__username=username, is_active=True).exists(): + return None + return api.unenroll_user_from_all_courses(username) + except Exception as exc: # pylint: disable=broad-except + log.exception("Unexpected error during unenrollment for user %s", username) + raise APIException("An unexpected error occurred during unenrollment.") from exc + + # ------------------------------------------------------------------ + # Allowed enrollments + # ------------------------------------------------------------------ + def list_allowed_for_email(self, email): + """Return the ``CourseEnrollmentAllowed`` queryset for ``email``.""" + return CourseEnrollmentAllowed.objects.filter(email=email) + + def create_allowed_enrollment(self, serializer): + """ + Persist the allowed-enrollment described by ``serializer``. + + Raises: + Conflict: if a row already exists for the (email, course_id) pair. + """ + from django.db import IntegrityError # local import — avoid heavy startup cost + try: + return serializer.save() + except IntegrityError as exc: + email = serializer.validated_data.get("email") + course_id = serializer.validated_data.get("course_id") + raise Conflict( + f"An enrollment allowed with email {email} and course {course_id} already exists." + ) from exc + + def delete_allowed_enrollment(self, email, course_id): + """ + Delete the allowed-enrollment row identified by (email, course_id). + + Raises: + NotFound: if no such row exists. + """ + try: + CourseEnrollmentAllowed.objects.get(email=email, course_id=course_id).delete() + except ObjectDoesNotExist as exc: + raise NotFound( + f"An enrollment allowed with email {email} and course {course_id} doesn't exists." + ) from exc diff --git a/openedx/core/djangoapps/enrollments/v2/views.py b/openedx/core/djangoapps/enrollments/v2/views.py new file mode 100644 index 000000000000..cb940244c401 --- /dev/null +++ b/openedx/core/djangoapps/enrollments/v2/views.py @@ -0,0 +1,626 @@ +""" +API Views for the Enrollment API — v2. + +This module is the v2 incarnation of the v1 enrollment views, restructured +to apply the FC-0118 ADRs from the start: + + * ADR 0025 – ``serializer_class`` on every viewset/view + * ADR 0026 – explicit ``authentication_classes`` + ``permission_classes`` + * ADR 0027 – ``drf_spectacular`` for OpenAPI schema generation + * ADR 0028 – consolidated into ``ViewSet`` classes registered via + ``DefaultRouter`` where the URL shape allows it + * ADR 0029 – standardized error envelope via :class:`StandardizedErrorMixin` + * ADR 0031 – business logic centralized in + :class:`EnrollmentOperationsService` (``v2.view_services``) + * ADR 0032 – ``DefaultPagination`` 7-field envelope on list endpoints + * ADR 0033 – OEP-68 parameter naming (``course_key`` preferred, + ``course_id`` as deprecated alias) plus standard ``ordering`` whitelist + +Existing v1 endpoints at ``/api/enrollment/v1/`` are unchanged — v2 is a +parallel new version mounted at ``/api/enrollment/v2/``. +""" + +import logging + +from django.contrib.auth import get_user_model +from django.utils.decorators import method_decorator +from drf_spectacular.utils import ( + OpenApiParameter, + OpenApiRequest, + OpenApiResponse, + extend_schema, +) +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.paginators import DefaultPagination +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from rest_framework import permissions, status, viewsets +from rest_framework.decorators import action +from rest_framework.exceptions import NotFound, ValidationError +from rest_framework.generics import ListAPIView +from rest_framework.response import Response +from rest_framework.views import APIView + +from common.djangoapps.student.models import CourseEnrollment +from common.djangoapps.util.disable_rate_limit import can_disable_rate_limit +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.cors_csrf.decorators import ensure_csrf_cookie_cross_domain +from openedx.core.djangoapps.enrollments import api +from openedx.core.djangoapps.enrollments.errors import CourseEnrollmentError +from openedx.core.djangoapps.enrollments.serializers import ( + CourseEnrollmentAllowedSerializer, + CourseEnrollmentsApiListSerializer, + CourseEnrollmentSerializer, + CourseSerializer, +) +from openedx.core.djangoapps.enrollments.v2.forms import EnrollmentsAdminListForm +from openedx.core.djangoapps.enrollments.v2.paginators import EnrollmentsAdminListPagination +from openedx.core.djangoapps.enrollments.v2.serializers import UserRolesResponseSerializer +from openedx.core.djangoapps.enrollments.v2.view_services import EnrollmentOperationsService +from openedx.core.djangoapps.enrollments.views import ( + ApiKeyPermissionMixIn, + EnrollmentCrossDomainSessionAuth, + EnrollmentUserThrottle, +) +from openedx.core.djangoapps.user_api.accounts.permissions import CanRetireUser +from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser +from openedx.core.lib.api.mixins import StandardizedErrorMixin +from openedx.core.lib.api.permissions import ApiKeyHeaderPermissionIsAuthenticated + +log = logging.getLogger(__name__) +User = get_user_model() + +# ADR 0031 — single shared service instance for the v2 enrollment operations. +_OPS = EnrollmentOperationsService() + + +# --------------------------------------------------------------------------- +# ADR 0027 — shared OpenAPI parameter and response building blocks +# --------------------------------------------------------------------------- +def _path_param(name: str, description: str) -> OpenApiParameter: + return OpenApiParameter( + name=name, description=description, required=True, type=str, location=OpenApiParameter.PATH, + ) + + +def _query_param(name: str, description: str, *, required: bool = False, type_=str, + deprecated: bool = False) -> OpenApiParameter: + return OpenApiParameter( + name=name, description=description, required=required, type=type_, + location=OpenApiParameter.QUERY, deprecated=deprecated, + ) + + +_COURSE_ID_PATH_PARAM = _path_param("course_id", "Course ID (e.g. course-v1:org+course+run).") +_USERNAME_PATH_PARAM = _path_param("username", "Username of the user.") +_USER_QUERY_PARAM = _query_param("user", "Username of the user whose enrollments to list.") +_INCLUDE_EXPIRED_QUERY_PARAM = _query_param( + "include_expired", "If '1', include expired enrollment modes in the response.", +) +_PAGE_QUERY_PARAM = _query_param("page", "Page number to retrieve. Default 1.") +_PAGE_SIZE_QUERY_PARAM = _query_param("page_size", "Items per page (default 10, max 100).") + +_RESP_UNAUTHENTICATED = OpenApiResponse(description="The requester is not authenticated.") +_RESP_FORBIDDEN = OpenApiResponse(description="The requester does not have permission for this operation.") +_RESP_NOT_FOUND = OpenApiResponse(description="The requested resource does not exist.") +_RESP_BAD_REQUEST = OpenApiResponse(description="Invalid request data or parameters.") + + +# --------------------------------------------------------------------------- +# ADR 0033 — Deprecation-header helpers (OEP-68 parameter naming) +# --------------------------------------------------------------------------- +def _build_legacy_param_deprecation_header(legacy_to_preferred): + """ + Build the ADR 0033 ``Deprecation`` HTTP header value for one or more + legacy parameter names, each paired with its OEP-68-compliant + replacement. + + Example: ``[('course_id', 'course_key')]`` → + ``"Parameter 'course_id' is deprecated. Use 'course_key' instead. ..."`` + """ + parts = [ + f"Parameter '{legacy}' is deprecated. Use '{preferred}' instead." + for legacy, preferred in legacy_to_preferred + ] + parts.append("Support will be removed in release ''.") + return " ".join(parts) + + +def _maybe_set_legacy_param_deprecation_header(request, response, alias_pairs): + """Set the ADR 0033 ``Deprecation`` HTTP header on the response when any + legacy parameter name from ``alias_pairs`` is present in the request.""" + used = [(legacy, preferred) for legacy, preferred in alias_pairs if legacy in request.query_params] + if used: + response["Deprecation"] = _build_legacy_param_deprecation_header(used) + return response + + +# =========================================================================== +# EnrollmentViewSet — consolidates list / create / unenroll / allowed +# =========================================================================== +@can_disable_rate_limit +class EnrollmentViewSet(StandardizedErrorMixin, viewsets.ViewSet, ApiKeyPermissionMixIn): + """ + Canonical ViewSet for the v2 Enrollment API. + + Consolidates the v1 ``EnrollmentListView`` + ``UnenrollmentView`` + + ``EnrollmentAllowedView`` into a single router-registered ViewSet + (ADR 0028). Per-action permissions are declared via the ``@action`` + decorator's ``permission_classes`` kwarg. + + Router URLs (registered at ``basename="enrollment"``):: + + GET /api/enrollment/v2/enrollment/ → list + POST /api/enrollment/v2/enrollment/ → create + POST /api/enrollment/v2/enrollment/unenroll/ → unenroll + GET /api/enrollment/v2/enrollment/enrollment_allowed/ → allowed (GET) + POST /api/enrollment/v2/enrollment/enrollment_allowed/ → allowed (POST) + DELETE /api/enrollment/v2/enrollment/enrollment_allowed/ → allowed (DELETE) + """ + + authentication_classes = ( + JwtAuthentication, + BearerAuthenticationAllowInactiveUser, + EnrollmentCrossDomainSessionAuth, + ) + permission_classes = (ApiKeyHeaderPermissionIsAuthenticated,) + throttle_classes = (EnrollmentUserThrottle,) + serializer_class = CourseEnrollmentSerializer + pagination_class = DefaultPagination # ADR 0032 + + def get_serializer_class(self): + if self.action == "allowed": + return CourseEnrollmentAllowedSerializer + return self.serializer_class + + def get_serializer(self, *args, **kwargs): + return self.get_serializer_class()(*args, **kwargs) + + # ------------------------------------------------------------------ + # list — GET /enrollment/ + # ------------------------------------------------------------------ + @extend_schema( + summary="List enrollments for a user (paginated)", + description=( + "Returns a paginated list of enrollments for the currently logged-in user, or for " + "the user named by the 'user' query parameter. Staff/admin/api-key access is required " + "to view another user's enrollments — otherwise the list is filtered to courses the " + "requester staffs." + ), + parameters=[_USER_QUERY_PARAM, _PAGE_QUERY_PARAM, _PAGE_SIZE_QUERY_PARAM], + responses={ + 200: OpenApiResponse( + response=CourseEnrollmentSerializer(many=True), + description="Paginated enrollment list.", + ), + 401: _RESP_UNAUTHENTICATED, + }, + ) + @method_decorator(ensure_csrf_cookie_cross_domain) + def list(self, request): + """List enrollments for the currently logged-in user (paginated).""" + username = request.GET.get("user", request.user.username) + enrollments = _OPS.list_enrollments_for_user( + request_user=request.user, + target_username=username, + has_api_key=self.has_api_key_permissions(request), + ) + paginator = self.pagination_class() + page = paginator.paginate_queryset(enrollments, request, view=self) + return paginator.get_paginated_response(self.get_serializer(page, many=True).data) + + # ------------------------------------------------------------------ + # create — POST /enrollment/ + # ------------------------------------------------------------------ + @extend_schema( + summary="Create or update an enrollment", + description=( + "Enrolls a user in a course. Server-to-server calls may deactivate or modify the " + "mode of existing enrollments; all other requests create or reactivate enrollments. " + "The request body must include course_details.course_id." + ), + request=OpenApiRequest(request=CourseEnrollmentSerializer), + responses={ + 200: OpenApiResponse( + response=CourseEnrollmentSerializer, + description="Enrollment created, reactivated, or updated successfully.", + ), + 400: _RESP_BAD_REQUEST, + 403: _RESP_FORBIDDEN, + 404: _RESP_NOT_FOUND, + }, + ) + @method_decorator(ensure_csrf_cookie_cross_domain) + def create(self, request): + """Enroll a user in a course (or update an existing enrollment).""" + course_id = request.data.get("course_details", {}).get("course_id") + if not course_id: + raise ValidationError("Course ID must be specified to create a new enrollment.") + try: + course_key = CourseKey.from_string(course_id) + except InvalidKeyError as exc: + raise ValidationError(f"No course '{course_id}' found for enrollment") from exc + return Response(_OPS.create_or_update_enrollment( + request=request, + has_api_key=self.has_api_key_permissions(request), + course_id=course_key, + )) + + # ------------------------------------------------------------------ + # unenroll — @action POST /enrollment/unenroll/ + # ------------------------------------------------------------------ + @extend_schema( + summary="Unenroll a user from all courses (retirement)", + description=( + "Privileged retirement-pipeline use only. Unenrolls the named user from every active " + "enrollment. The request must be made by a service user with CanRetireUser permission, " + "not the user being unenrolled." + ), + request=OpenApiRequest( + request={"type": "object", "properties": {"username": {"type": "string"}}, "required": ["username"]}, + ), + responses={ + 200: OpenApiResponse(description="List of courses from which the user was unenrolled."), + 204: OpenApiResponse(description="User has no active enrollments."), + 400: _RESP_BAD_REQUEST, + 404: _RESP_NOT_FOUND, + }, + ) + @action( + detail=False, + methods=["post"], + url_path="unenroll", + permission_classes=[permissions.IsAuthenticated, CanRetireUser], + ) + def unenroll(self, request): + """Unenroll the specified user from all courses (retirement pipeline).""" + result = _OPS.unenroll_user_for_retirement(request.data.get("username")) + if result is None: + return Response(status=status.HTTP_204_NO_CONTENT) + return Response(result) + + # ------------------------------------------------------------------ + # allowed — @action GET/POST/DELETE /enrollment/enrollment_allowed/ + # ------------------------------------------------------------------ + @extend_schema( + summary="Manage CourseEnrollmentAllowed records (admin-only)", + description=( + "GET lists allowed enrollments for an email; POST creates a new one; DELETE removes " + "an existing one by email + course_id. Admin-only." + ), + request=OpenApiRequest(request=CourseEnrollmentAllowedSerializer), + parameters=[_query_param("email", "Email to query (GET only). Defaults to the requester's email.")], + responses={ + 200: OpenApiResponse( + response=CourseEnrollmentAllowedSerializer(many=True), + description="GET success — list of allowed enrollments for the email.", + ), + 201: OpenApiResponse( + response=CourseEnrollmentAllowedSerializer, + description="POST success — allowed enrollment created.", + ), + 204: OpenApiResponse(description="DELETE success — allowed enrollment deleted."), + 400: _RESP_BAD_REQUEST, + 403: _RESP_FORBIDDEN, + 404: OpenApiResponse(description="DELETE: allowed enrollment not found."), + 409: OpenApiResponse(description="POST: allowed enrollment already exists."), + }, + ) + @action( + detail=False, + methods=["get", "post", "delete"], + url_path="enrollment_allowed", + permission_classes=[permissions.IsAdminUser], + throttle_classes=[EnrollmentUserThrottle], + ) + def allowed(self, request): + """Retrieve, create, or delete CourseEnrollmentAllowed records. Admin-only.""" + if request.method == "GET": + user_email = request.query_params.get("email") or request.user.email + enrollments_allowed = _OPS.list_allowed_for_email(user_email) + return Response( + status=status.HTTP_200_OK, + data=self.get_serializer(enrollments_allowed, many=True).data, + ) + + serializer = self.get_serializer(data=request.data) + if not serializer.is_valid(): + raise ValidationError(serializer.errors) + + if request.method == "POST": + enrollment_allowed = _OPS.create_allowed_enrollment(serializer) + return Response( + status=status.HTTP_201_CREATED, + data=self.get_serializer(enrollment_allowed).data, + ) + + # DELETE + _OPS.delete_allowed_enrollment( + email=serializer.validated_data.get("email"), + course_id=serializer.validated_data.get("course_id"), + ) + return Response(status=status.HTTP_204_NO_CONTENT) + + +# =========================================================================== +# EnrollmentRetrieveView — singleton GET /enrollment/{course_id} +# =========================================================================== +# Kept as a standalone APIView because the {username},{course_id} URL form +# (comma-separated, both optional) is not expressible via DefaultRouter. +class EnrollmentRetrieveView(StandardizedErrorMixin, ApiKeyPermissionMixIn, APIView): + """GET enrollment for a course (and optionally a named user).""" + + authentication_classes = ( + JwtAuthentication, + BearerAuthenticationAllowInactiveUser, + EnrollmentCrossDomainSessionAuth, + ) + permission_classes = (ApiKeyHeaderPermissionIsAuthenticated,) + throttle_classes = (EnrollmentUserThrottle,) + serializer_class = CourseEnrollmentSerializer + + @extend_schema( + summary="Retrieve a user's enrollment in a course", + description=( + "Returns the current user's enrollment for the specified course, or the named user's " + "enrollment when invoked with the {username},{course_id} URL form (server-to-server or " + "staff only)." + ), + parameters=[_USERNAME_PATH_PARAM, _COURSE_ID_PATH_PARAM], + responses={ + 200: OpenApiResponse( + response=CourseEnrollmentSerializer, + description="Enrollment retrieved successfully (or empty body if no enrollment).", + ), + 400: _RESP_BAD_REQUEST, + 404: _RESP_NOT_FOUND, + }, + ) + @method_decorator(ensure_csrf_cookie_cross_domain) + def get(self, request, course_id=None, username=None): + """ + Return the enrollment for ``(username, course_id)``. + + When ``username`` is omitted (the ``GET /enrollment/{course_id}`` + URL form), the request user is used. Non-staff callers may only + look up their own enrollment; any cross-user lookup without + ``has_api_key`` or staff privileges raises ``NotFound`` (so the + caller cannot probe for the existence of other users' enrollments). + """ + if username is None: + username = request.user.username + + if ( + username != request.user.username + and not self.has_api_key_permissions(request) + and not request.user.is_staff + ): + # Hide existence of other users' enrollments. + raise NotFound() + + try: + course_key = CourseKey.from_string(course_id) + except InvalidKeyError as exc: + raise ValidationError(f"No course '{course_id}' found for enrollment") from exc + + try: + enrollment = CourseEnrollment.objects.get(user__username=username, course_id=course_key) + except CourseEnrollment.DoesNotExist: + return Response(None) + except CourseEnrollmentError as exc: + raise ValidationError( + f"An error occurred while retrieving enrollments for user " + f"'{username}' in course '{course_id}'" + ) from exc + + return Response(self.serializer_class(enrollment).data) + + +# =========================================================================== +# UserRolesView — GET /roles/ (singleton list endpoint for the current user) +# =========================================================================== +class UserRolesView(StandardizedErrorMixin, APIView): + """List the current user's course-level roles.""" + + authentication_classes = ( + JwtAuthentication, + BearerAuthenticationAllowInactiveUser, + EnrollmentCrossDomainSessionAuth, + ) + permission_classes = (ApiKeyHeaderPermissionIsAuthenticated,) + throttle_classes = (EnrollmentUserThrottle,) + serializer_class = UserRolesResponseSerializer + + # ADR 0033: ``course_key`` is the preferred filter name (OEP-68); + # ``course_id`` is retained as a deprecated alias. + _LEGACY_PARAM_ALIASES = (("course_id", "course_key"),) + + @extend_schema( + summary="List the current user's course roles", + description=( + "Returns the list of course-level roles held by the currently logged-in user, plus " + "an is_staff flag. Optionally filters by course_key (or course_id, deprecated)." + ), + parameters=[ + _query_param("course_key", "If provided, only roles for this course are returned (OEP-68)."), + _query_param( + "course_id", "Deprecated alias for 'course_key' (ADR 0033). Use 'course_key' instead.", + deprecated=True, + ), + ], + responses={ + 200: OpenApiResponse( + response=UserRolesResponseSerializer, + description="Roles retrieved successfully.", + ), + 400: _RESP_BAD_REQUEST, + }, + ) + @method_decorator(ensure_csrf_cookie_cross_domain) + def get(self, request): + """ + List the current user's course-level roles. + + Optionally filtered by ``course_key`` (preferred, OEP-68) or + ``course_id`` (deprecated alias). When both are present, + ``course_key`` wins and the response carries the ADR 0033 + ``Deprecation`` HTTP header. + """ + try: + course_key = request.GET.get("course_key") or request.GET.get("course_id") + roles_data = api.get_user_roles(request.user.username) + if course_key: + roles_data = [role for role in roles_data if str(role.course_id) == course_key] + except Exception as exc: # pylint: disable=broad-except + raise ValidationError( + f"An error occurred while retrieving roles for user '{request.user.username}'" + ) from exc + + serializer = self.serializer_class({ + "roles": list(roles_data), + "is_staff": request.user.is_staff, + }) + response = Response(serializer.data) + return _maybe_set_legacy_param_deprecation_header( + request, response, self._LEGACY_PARAM_ALIASES, + ) + + +# =========================================================================== +# CourseEnrollmentDetailView — GET /course/{course_id} (public, no auth) +# =========================================================================== +class CourseEnrollmentDetailView(StandardizedErrorMixin, APIView): + """Get enrollment information about a particular course.""" + + authentication_classes = () + permission_classes = () + throttle_classes = (EnrollmentUserThrottle,) + serializer_class = CourseSerializer + + @extend_schema( + summary="Get enrollment details for a course", + description=( + "Returns the course schedule and supported enrollment modes. No authentication " + "required. Use ?include_expired=1 to include expired enrollment modes." + ), + parameters=[_COURSE_ID_PATH_PARAM, _INCLUDE_EXPIRED_QUERY_PARAM], + responses={ + 200: OpenApiResponse( + response=CourseSerializer, + description="Course enrollment details retrieved successfully.", + ), + 400: _RESP_BAD_REQUEST, + 404: _RESP_NOT_FOUND, + }, + ) + def get(self, request, course_id=None): + """ + Return enrollment-related details for the specified course. + + Public (no authentication required). The response includes the + course schedule and supported enrollment modes; pass + ``?include_expired=1`` to include expired enrollment modes. + """ + try: + course_key = CourseKey.from_string(course_id) + except InvalidKeyError as exc: + raise ValidationError(f"No course found for course ID '{course_id}'") from exc + try: + course_overview = CourseOverview.get_from_id(course_key) + except CourseOverview.DoesNotExist as exc: + raise NotFound(f"No course found for course ID '{course_id}'") from exc + + include_expired = bool(request.GET.get("include_expired", "")) + serializer = self.serializer_class(course_overview, include_expired=include_expired) + return Response(serializer.data) + + +# =========================================================================== +# EnrollmentsAdminListView — GET /enrollments/ (admin paginated list) +# =========================================================================== +@extend_schema( + summary="List all course enrollments (admin-only, paginated)", + description=( + "Admin-only paginated list of CourseEnrollment records, optionally filtered by " + "course_key, course_keys, username, or email, and optionally ordered." + ), + parameters=[ + _query_param("course_key", "Filter to enrollments for this course (OEP-68)."), + _query_param("course_keys", "Comma-separated list of course keys (OEP-68)."), + _query_param( + "course_id", "Deprecated alias for 'course_key' (ADR 0033). Use 'course_key' instead.", + deprecated=True, + ), + _query_param( + "course_ids", "Deprecated alias for 'course_keys' (ADR 0033). Use 'course_keys' instead.", + deprecated=True, + ), + _query_param("username", "Comma-separated list of usernames."), + _query_param("email", "Comma-separated list of emails."), + _query_param("ordering", "Order results by one of: created, -created, id, -id (ADR 0033 §3)."), + _PAGE_QUERY_PARAM, + _PAGE_SIZE_QUERY_PARAM, + ], + responses={ + 200: OpenApiResponse( + response=CourseEnrollmentsApiListSerializer(many=True), + description="Paginated list of course enrollments.", + ), + 400: _RESP_BAD_REQUEST, + 401: _RESP_UNAUTHENTICATED, + 403: _RESP_FORBIDDEN, + }, +) +class EnrollmentsAdminListView(StandardizedErrorMixin, ListAPIView): + """Admin-only paginated enrollment list with OEP-68 filter aliases.""" + + authentication_classes = ( + JwtAuthentication, + BearerAuthenticationAllowInactiveUser, + EnrollmentCrossDomainSessionAuth, + ) + permission_classes = (permissions.IsAdminUser,) + throttle_classes = (EnrollmentUserThrottle,) + serializer_class = CourseEnrollmentsApiListSerializer + pagination_class = EnrollmentsAdminListPagination + + # ADR 0033 §3 — whitelist of allowed values for the ``ordering`` param. + ALLOWED_ORDERING_FIELDS = frozenset({"created", "-created", "id", "-id"}) + + # ADR 0033 §2 / OEP-68 alias pairs accepted by this endpoint. + _LEGACY_PARAM_ALIASES = ( + ("course_id", "course_key"), + ("course_ids", "course_keys"), + ) + + def get_queryset(self): + form = EnrollmentsAdminListForm(self.request.query_params) + if not form.is_valid(): + raise ValidationError(form.errors) + + queryset = CourseEnrollment.objects.all().select_related("user", "course") + course_id = form.cleaned_data.get("course_id") + course_ids = form.cleaned_data.get("course_ids") + usernames = form.cleaned_data.get("username") + emails = form.cleaned_data.get("email") + + if course_id: + queryset = queryset.filter(course_id=course_id) + if course_ids: + queryset = queryset.filter(course_id__in=course_ids) + if usernames: + queryset = queryset.filter(user__username__in=usernames) + if emails: + queryset = queryset.filter(user__email__in=emails) + + ordering = self.request.query_params.get("ordering") + if ordering in self.ALLOWED_ORDERING_FIELDS: + queryset = queryset.order_by(ordering) + return queryset + + def list(self, request, *args, **kwargs): + """Override to emit the ADR 0033 Deprecation header when legacy params used.""" + response = super().list(request, *args, **kwargs) + return _maybe_set_legacy_param_deprecation_header( + request, response, self._LEGACY_PARAM_ALIASES, + ) diff --git a/openedx/core/lib/api/mixins.py b/openedx/core/lib/api/mixins.py index f0af9072e101..693a02ecf155 100644 --- a/openedx/core/lib/api/mixins.py +++ b/openedx/core/lib/api/mixins.py @@ -8,6 +8,29 @@ from rest_framework.mixins import CreateModelMixin from rest_framework.response import Response +from openedx.core.lib.api.exceptions import standardized_error_exception_handler + + +class StandardizedErrorMixin: + """ + Opt-in mixin that routes DRF exceptions on this view through the ADR 0029 + standardized error-response handler (see + ``openedx.core.lib.api.exceptions.standardized_error_exception_handler``). + + DRF's :class:`rest_framework.views.APIView` calls ``self.get_exception_handler`` + inside ``handle_exception``; overriding that method here lets the view + return the standardized envelope while other endpoints continue to use + whichever handler the project-wide ``EXCEPTION_HANDLER`` setting points at. + + Usage:: + + class MyViewSet(StandardizedErrorMixin, viewsets.ViewSet): + ... + """ + + def get_exception_handler(self): + return standardized_error_exception_handler + class PutAsCreateMixin(CreateModelMixin): """ From 4fc896b828872bfa1e16e8a6f195dd93839ee50f Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed <68893403+taimoor-ahmed-1@users.noreply.github.com> Date: Wed, 10 Jun 2026 17:42:18 +0500 Subject: [PATCH 07/13] feat: Update course detail api version with ADR standardization (#38708) --- .../contentstore/rest_api/v3/urls.py | 3 +- .../rest_api/v3/views/__init__.py | 1 + .../rest_api/v3/views/course_details.py | 204 +++++++++++++ .../v3/views/tests/test_course_details.py | 269 ++++++++++++++++++ 4 files changed, 476 insertions(+), 1 deletion(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v3/views/course_details.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py diff --git a/cms/djangoapps/contentstore/rest_api/v3/urls.py b/cms/djangoapps/contentstore/rest_api/v3/urls.py index 9d8f93a7ee7c..b0ba53997bae 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v3/urls.py @@ -2,11 +2,12 @@ from rest_framework.routers import DefaultRouter -from cms.djangoapps.contentstore.rest_api.v3.views import HomeViewSet +from cms.djangoapps.contentstore.rest_api.v3.views import CourseDetailsViewSet, HomeViewSet app_name = "v3" router = DefaultRouter() router.register(r'home', HomeViewSet, basename='home') +router.register(r'course_details', CourseDetailsViewSet, basename='course_details') urlpatterns = router.urls diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v3/views/__init__.py index 573556d80b39..3fd8759e2318 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/__init__.py @@ -1,3 +1,4 @@ """Views for v3 contentstore API.""" +from .course_details import CourseDetailsViewSet # noqa: F401 from .home import HomeViewSet # noqa: F401 diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py b/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py new file mode 100644 index 000000000000..85bfa6d691bc --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py @@ -0,0 +1,204 @@ +""" +API Views for course details — v3. + +This module is the v3 incarnation of the v1 ``course_details`` endpoint, +restructured to apply the FC-0118 ADRs: + + * ADR 0025 – ``serializer_class`` on the viewset + * ADR 0026 – explicit ``authentication_classes`` + ``permission_classes`` + * ADR 0027 – ``drf_spectacular`` for OpenAPI schema generation + * ADR 0028 – consolidated into a single DRF ``ViewSet`` registered via + ``DefaultRouter`` (replaces ``CourseDetailsView`` ``APIView``) + * ADR 0029 – standardized error envelope via :class:`StandardizedErrorMixin` + (v3-scoped — does not change the project-wide DRF ``EXCEPTION_HANDLER`` + setting) + +Permission model note: + PR #38365 proposed a class-level ``HasStudioReadAccess`` permission. The + current v1 view has since evolved to use the ``openedx_authz`` permission + framework with a schedule-vs-details classification that gates updates on + *different* permissions depending on the payload. That granularity cannot + be hoisted to a single class-level permission, so the per-action checks + remain inline (gated by ``IsAuthenticated`` at the class level). +""" + +from django.core.exceptions import ValidationError as DjangoValidationError +from drf_spectacular.utils import OpenApiParameter, OpenApiRequest, OpenApiResponse, extend_schema +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from openedx_authz.constants.permissions import ( + COURSES_EDIT_DETAILS, + COURSES_EDIT_SCHEDULE, + COURSES_VIEW_SCHEDULE_AND_DETAILS, +) +from rest_framework import viewsets +from rest_framework.exceptions import NotFound +from rest_framework.exceptions import ValidationError as DRFValidationError +from rest_framework.permissions import IsAuthenticated +from rest_framework.request import Request +from rest_framework.response import Response + +from cms.djangoapps.contentstore.rest_api.v1.serializers import CourseDetailsSerializer +from cms.djangoapps.contentstore.rest_api.v1.views.course_details import _classify_update +from cms.djangoapps.contentstore.utils import update_course_details +from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission +from openedx.core.djangoapps.authz.decorators import user_has_course_permission +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.models.course_details import CourseDetails +from openedx.core.lib.api.mixins import StandardizedErrorMixin +from xmodule.modulestore.django import modulestore + +_COURSE_ID_PARAMETER = OpenApiParameter( + name="course_id", + description="Course ID", + required=True, + type=str, + location=OpenApiParameter.PATH, +) +_COMMON_ERROR_RESPONSES = { + 401: OpenApiResponse(description="The requester is not authenticated."), + 403: OpenApiResponse(description="The requester cannot access the specified course."), + 404: OpenApiResponse(description="The requested course does not exist."), +} + + +def _resolve_course_key(course_id: str) -> CourseKey: + """ + Parse ``course_id`` into a ``CourseKey`` and verify the course exists. + + Raises ``NotFound`` for both unparseable keys and missing courses, which + the ADR 0029 envelope renders as a structured 404 response. This replaces + the legacy ``@verify_course_exists()`` decorator from v1 and avoids + relying on ``DeveloperErrorViewMixin``. + """ + try: + course_key = CourseKey.from_string(course_id) + except InvalidKeyError as exc: + raise NotFound("The provided course key cannot be parsed.") from exc + if not CourseOverview.course_exists(course_key): + raise NotFound(f"Course {course_id} not found.") + return course_key + + +class CourseDetailsViewSet(StandardizedErrorMixin, viewsets.ViewSet): + """ + ViewSet for course details (v3). Registered via DefaultRouter (basename ``course_details``). + + Router-generated URLs:: + + GET /api/contentstore/v3/course_details/{course_id}/ → retrieve + PUT /api/contentstore/v3/course_details/{course_id}/ → update + + Supersedes ``CourseDetailsView`` at ``/api/contentstore/v1/course_details/{course_id}``. + """ + + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated,) + serializer_class = CourseDetailsSerializer + + # Matches both slash-separated (org/course/run) and plus-separated (course-v1:org+course+run) IDs + lookup_field = "course_id" + lookup_value_regex = r"[^/+]+(?:/|\+)[^/+]+(?:/|\+)[^/?]+" + + @extend_schema( + summary="Retrieve a course's details", + description="Get an object containing all the course details for the specified course.", + parameters=[_COURSE_ID_PARAMETER], + responses={ + 200: OpenApiResponse( + response=CourseDetailsSerializer, + description="Course details retrieved successfully.", + ), + **_COMMON_ERROR_RESPONSES, + }, + ) + def retrieve(self, request: Request, course_id: str): + """ + Get an object containing all the course details. + + **Example Request** + + GET /api/contentstore/v3/course_details/{course_id}/ + """ + course_key = _resolve_course_key(course_id) + if not user_has_course_permission( + request.user, + COURSES_VIEW_SCHEDULE_AND_DETAILS.identifier, + course_key, + LegacyAuthoringPermission.READ, + ): + self.permission_denied(request) + + course_details = CourseDetails.fetch(course_key) + serializer = self.serializer_class(course_details) + return Response(serializer.data) + + @extend_schema( + summary="Update a course's details", + description="Update the details for the specified course.", + request=OpenApiRequest(request=CourseDetailsSerializer), + parameters=[_COURSE_ID_PARAMETER], + responses={ + 200: OpenApiResponse( + response=CourseDetailsSerializer, + description="Course details updated successfully.", + ), + 400: OpenApiResponse(description="Bad request — invalid data."), + **_COMMON_ERROR_RESPONSES, + }, + ) + def update(self, request: Request, course_id: str): + """ + Update a course's details. + + **Example Request** + + PUT /api/contentstore/v3/course_details/{course_id}/ + + **PUT Parameters** + + The data sent for a put request should follow a similar format as + is returned by a ``GET`` request. Multiple details can be updated in + a single request, however only the ``value`` field can be updated; + any other fields, if included, will be ignored. + + **Response Values** + + If the request is successful, an HTTP 200 "OK" response is returned, + along with all the course's details similar to a ``GET`` request. + """ + course_key = _resolve_course_key(course_id) + is_schedule_update, is_details_update = _classify_update(request.data, course_key) + + if not is_schedule_update and not is_details_update: + # No updatable fields provided — fall through to a details-permission check + # so the caller gets 403 if they lack edit-details rights. + is_details_update = True + + if is_schedule_update and not user_has_course_permission( + request.user, + COURSES_EDIT_SCHEDULE.identifier, + course_key, + LegacyAuthoringPermission.READ, + ): + self.permission_denied(request) + + if is_details_update and not user_has_course_permission( + request.user, + COURSES_EDIT_DETAILS.identifier, + course_key, + LegacyAuthoringPermission.READ, + ): + self.permission_denied(request) + + course_block = modulestore().get_course(course_key) + + try: + updated_data = update_course_details(request, course_key, request.data, course_block) + except DjangoValidationError as err: + raise DRFValidationError(err.message) from err + + serializer = self.serializer_class(updated_data) + return Response(serializer.data) diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py new file mode 100644 index 000000000000..3fe6854d1f35 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py @@ -0,0 +1,269 @@ +""" +Unit tests for CourseDetailsViewSet (v3). + +Single test module covering every ADR applied to the viewset: + * ADR 0025 / 0026 / 0027 / 0028 — action + permission + routing tests + (``TestCourseDetailsViewSetPermissions``, ``TestCourseDetailsViewSetActions``) + * ADR 0029 — standardized error envelope shape tests + (``TestCourseDetailsViewSetErrorShape``) + +MongoDB-free: every service-layer call (``CourseDetails.fetch``, +``modulestore``, ``update_course_details``, +``openedx_authz.user_has_course_permission``, and +``CourseOverview.course_exists``) is mocked so the suite runs without a live +modulestore or course-overview row. + +``patch.object`` is used for ``serializer_class`` because the attribute is +resolved at class-definition time — string-based ``patch()`` of the module +attribute does not replace the live ViewSet attribute. +""" +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.v3.views.course_details import CourseDetailsViewSet +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview + +# --------------------------------------------------------------------------- +# Mock target paths +# --------------------------------------------------------------------------- +MOCK_FETCH = "openedx.core.djangoapps.models.course_details.CourseDetails.fetch" +MOCK_MODULESTORE = "cms.djangoapps.contentstore.rest_api.v3.views.course_details.modulestore" +MOCK_UPDATE = "cms.djangoapps.contentstore.rest_api.v3.views.course_details.update_course_details" +MOCK_HAS_PERMISSION = ( + "cms.djangoapps.contentstore.rest_api.v3.views.course_details.user_has_course_permission" +) +MOCK_CLASSIFY = "cms.djangoapps.contentstore.rest_api.v3.views.course_details._classify_update" +MOCK_COURSE_EXISTS = ( + "cms.djangoapps.contentstore.rest_api.v3.views.course_details.CourseOverview.course_exists" +) + +# Syntactically valid course key reused across action / permission / envelope tests. +TEST_COURSE_ID = "course-v1:org+course+run" + +_REQUIRED_ERROR_FIELDS = ("type", "title", "status", "detail", "instance") + + +# =========================================================================== +# ADR 0026 — permission boundary tests +# =========================================================================== +class TestCourseDetailsViewSetPermissions(APITestCase): + """ + ADR 0026 – permission regression tests for CourseDetailsViewSet (v3). + + The v3 viewset enforces ``IsAuthenticated`` at the class level and uses + inline ``user_has_course_permission`` checks for course-level authorization + (necessary because the schedule-vs-details split depends on the payload). + """ + + def setUp(self): + super().setUp() + self.url = reverse( + "cms.djangoapps.contentstore:v3:course_details-detail", + kwargs={"course_id": TEST_COURSE_ID}, + ) + + # --- Unauthenticated --- + + def test_unauthenticated_get_returns_401(self): + """Unauthenticated GET must return 401 (IsAuthenticated).""" + response = self.client.get(self.url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_unauthenticated_put_returns_401(self): + """Unauthenticated PUT must return 401 (IsAuthenticated).""" + response = self.client.put(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + # --- Authenticated but no course access --- + + @patch.object(CourseOverview, "course_exists", return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=False) + def test_non_author_get_returns_403(self, mock_perm, mock_exists): # noqa: ARG002 + """Authenticated user without view permission must receive 403 on GET.""" + user = UserFactory.create() + self.client.force_authenticate(user=user) + response = self.client.get(self.url) + assert response.status_code == status.HTTP_403_FORBIDDEN + + @patch.object(CourseOverview, "course_exists", return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=False) + @patch(MOCK_CLASSIFY, return_value=(False, True)) # details-only update + def test_non_author_put_returns_403(self, mock_classify, mock_perm, mock_exists): # noqa: ARG002 + """Authenticated user without edit permission must receive 403 on PUT.""" + user = UserFactory.create() + self.client.force_authenticate(user=user) + response = self.client.put(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_403_FORBIDDEN + + +# =========================================================================== +# ADR 0025 / 0028 — action body tests +# =========================================================================== +class TestCourseDetailsViewSetActions(APITestCase): + """ + Action tests for CourseDetailsViewSet (retrieve and update). + + Service-layer calls are mocked, and ``user_has_course_permission`` returns + True so authorization passes through to the action body. + """ + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.client.force_authenticate(user=self.user) + self.url = reverse( + "cms.djangoapps.contentstore:v3:course_details-detail", + kwargs={"course_id": TEST_COURSE_ID}, + ) + + @patch.object(CourseDetailsViewSet, "serializer_class") + @patch.object(CourseOverview, "course_exists", return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=True) + @patch(MOCK_FETCH) + def test_retrieve_calls_course_details_fetch( + self, mock_fetch, mock_perm, mock_exists, mock_ser_cls, # noqa: ARG002 + ): + """GET calls CourseDetails.fetch() and returns 200.""" + mock_fetch.return_value = MagicMock() + mock_ser_cls.return_value.data = {"course_id": "run"} + + response = self.client.get(self.url) + + assert response.status_code == status.HTTP_200_OK + mock_fetch.assert_called_once() + + @patch.object(CourseDetailsViewSet, "serializer_class") + @patch.object(CourseOverview, "course_exists", return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=True) + @patch(MOCK_UPDATE) + @patch(MOCK_MODULESTORE) + @patch(MOCK_CLASSIFY, return_value=(False, True)) + def test_update_calls_update_course_details( # noqa: PLR0913 + self, + mock_classify, # noqa: ARG002 + mock_store, + mock_update, + mock_perm, # noqa: ARG002 + mock_exists, # noqa: ARG002 + mock_ser_cls, + ): + """PUT calls update_course_details() and returns 200.""" + mock_store.return_value.get_course.return_value = MagicMock() + mock_update.return_value = MagicMock() + mock_ser_cls.return_value.data = {"course_id": "run"} + + response = self.client.put(self.url, data={}, content_type="application/json") + + assert response.status_code == status.HTTP_200_OK + mock_update.assert_called_once() + + +# =========================================================================== +# ADR 0029 — standardized error-response envelope tests +# =========================================================================== +class TestCourseDetailsViewSetErrorShape(APITestCase): + """ + ADR 0029 – error response shape regression tests for CourseDetailsViewSet (v3). + + The envelope is wired in via + :class:`openedx.core.lib.api.mixins.StandardizedErrorMixin`, which overrides + DRF's per-view ``get_exception_handler`` to point at + ``openedx.core.lib.api.exceptions.standardized_error_exception_handler``. + + Scoped to v3 — the project-wide DRF ``EXCEPTION_HANDLER`` setting is + unchanged, so v0 / v1 / v2 / v4 endpoints continue to return the legacy + error shape (locked in by ``test_v1_endpoint_unaffected_by_v3_envelope``). + """ + + def setUp(self): + super().setUp() + self.client = APIClient() + self.detail_url = reverse( + "cms.djangoapps.contentstore:v3:course_details-detail", + kwargs={"course_id": TEST_COURSE_ID}, + ) + + def test_unauthenticated_get_returns_standardized_401(self): + """Unauthenticated GET must return 401 with the ADR 0029 envelope.""" + response = self.client.get(self.detail_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + def test_unauthenticated_401_type_uri(self): + """The ``type`` field for 401 must be the ADR 0029 authn URI.""" + response = self.client.get(self.detail_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.data.get("type") == "https://docs.openedx.org/errors/authn" + + @patch(MOCK_COURSE_EXISTS, return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=False) + def test_non_author_get_returns_standardized_403(self, mock_perm, mock_exists): # noqa: ARG002 + """Authenticated non-author GET must return 403 with the ADR 0029 envelope.""" + non_author = UserFactory.create() + self.client.force_authenticate(user=non_author) + response = self.client.get(self.detail_url) + assert response.status_code == status.HTTP_403_FORBIDDEN + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + @patch(MOCK_COURSE_EXISTS, return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=False) + def test_non_author_403_type_uri(self, mock_perm, mock_exists): # noqa: ARG002 + """The ``type`` field for 403 must be the ADR 0029 authz URI.""" + non_author = UserFactory.create() + self.client.force_authenticate(user=non_author) + response = self.client.get(self.detail_url) + assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.data.get("type") == "https://docs.openedx.org/errors/authz" + + @patch(MOCK_COURSE_EXISTS, return_value=False) + def test_nonexistent_course_returns_standardized_404(self, mock_exists): # noqa: ARG002 + """GET for a non-existent course must return 404 with the ADR 0029 envelope.""" + staff = UserFactory.create(is_staff=True) + self.client.force_authenticate(user=staff) + response = self.client.get(self.detail_url) + assert response.status_code == status.HTTP_404_NOT_FOUND + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + @patch(MOCK_COURSE_EXISTS, return_value=False) + def test_not_found_type_uri(self, mock_exists): # noqa: ARG002 + """The ``type`` field for 404 must be the ADR 0029 not-found URI.""" + staff = UserFactory.create(is_staff=True) + self.client.force_authenticate(user=staff) + response = self.client.get(self.detail_url) + assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.data.get("type") == "https://docs.openedx.org/errors/not-found" + + def test_error_body_has_no_developer_message(self): + """Error responses must NOT contain old DeveloperErrorViewMixin fields.""" + response = self.client.get(self.detail_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert "developer_message" not in response.data + assert "error_code" not in response.data + + def test_instance_field_is_request_path(self): + """The ``instance`` field must equal the request path.""" + response = self.client.get(self.detail_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.data.get("instance") == self.detail_url + + def test_v1_endpoint_unaffected_by_v3_envelope(self): + """ + The ADR 0029 envelope must be scoped to v3 — hitting the legacy v1 + ``course_details`` endpoint unauthenticated must NOT return the v3 + envelope (no ``type`` / ``instance`` keys). + """ + v1_url = reverse( + "cms.djangoapps.contentstore:v1:course_details", + kwargs={"course_id": TEST_COURSE_ID}, + ) + response = self.client.get(v1_url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert "type" not in response.data + assert "instance" not in response.data From b8df48f98a11b106faa9737aea7756c44118c006 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed <68893403+taimoor-ahmed-1@users.noreply.github.com> Date: Mon, 15 Jun 2026 13:06:33 +0500 Subject: [PATCH 08/13] feat: apply ADR standardization to AuthorGrading apis (#38726) --- .../contentstore/rest_api/v3/urls.py | 3 +- .../contentstore/rest_api/v3/utils.py | 55 ++++ .../rest_api/v3/views/__init__.py | 1 + .../rest_api/v3/views/authoring_grading.py | 163 ++++++++++ .../rest_api/v3/views/course_details.py | 36 +-- .../v3/views/tests/test_authoring_grading.py | 293 ++++++++++++++++++ .../v3/views/tests/test_course_details.py | 5 +- 7 files changed, 523 insertions(+), 33 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v3/utils.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py diff --git a/cms/djangoapps/contentstore/rest_api/v3/urls.py b/cms/djangoapps/contentstore/rest_api/v3/urls.py index b0ba53997bae..36eb245396b9 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v3/urls.py @@ -2,12 +2,13 @@ from rest_framework.routers import DefaultRouter -from cms.djangoapps.contentstore.rest_api.v3.views import CourseDetailsViewSet, HomeViewSet +from cms.djangoapps.contentstore.rest_api.v3.views import AuthoringGradingViewSet, CourseDetailsViewSet, HomeViewSet app_name = "v3" router = DefaultRouter() router.register(r'home', HomeViewSet, basename='home') router.register(r'course_details', CourseDetailsViewSet, basename='course_details') +router.register(r'authoring_grading', AuthoringGradingViewSet, basename='authoring_grading') urlpatterns = router.urls diff --git a/cms/djangoapps/contentstore/rest_api/v3/utils.py b/cms/djangoapps/contentstore/rest_api/v3/utils.py new file mode 100644 index 000000000000..3db96963b764 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/utils.py @@ -0,0 +1,55 @@ +""" +Shared utilities for v3 contentstore API viewsets. + +Houses the small helpers and OpenAPI constants that more than one v3 viewset +needs, so the per-viewset modules stay focused on action bodies and don't +drift apart over time. + +Currently provides: + * :func:`resolve_course_key` – parse-and-verify a course key string, + raising ``NotFound`` for unparseable keys or missing courses (replaces + the legacy ``@verify_course_exists()`` decorator from v1 and avoids + relying on ``DeveloperErrorViewMixin``). + * :data:`COMMON_ERROR_RESPONSES` – the shared ``@extend_schema(responses=...)`` + fragment for the 401 / 403 / 404 cases every v3 course-scoped viewset + can raise. +""" + +from drf_spectacular.utils import OpenApiResponse +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from rest_framework.exceptions import NotFound + +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview + + +def resolve_course_key(course_key: str) -> CourseKey: + """ + Parse ``course_key`` (string) into a :class:`CourseKey` and verify the + course exists. + + Raises: + rest_framework.exceptions.NotFound: if the string is unparseable + *or* the course does not exist. The ADR 0029 envelope (wired in + by :class:`openedx.core.lib.api.mixins.StandardizedErrorMixin`) + renders both as a structured 404. + + OEP-68: the parameter name is ``course_key`` rather than the legacy + ``course_id``. The function is intentionally agnostic to which URL kwarg + name the caller used — callers may pass the value of either kwarg as a + positional argument. + """ + try: + parsed = CourseKey.from_string(course_key) + except InvalidKeyError as exc: + raise NotFound("The provided course key cannot be parsed.") from exc + if not CourseOverview.course_exists(parsed): + raise NotFound(f"Course {course_key} not found.") + return parsed + + +COMMON_ERROR_RESPONSES = { + 401: OpenApiResponse(description="The requester is not authenticated."), + 403: OpenApiResponse(description="The requester cannot access the specified course."), + 404: OpenApiResponse(description="The requested course does not exist."), +} diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v3/views/__init__.py index 3fd8759e2318..d3d8670950a3 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/__init__.py @@ -1,4 +1,5 @@ """Views for v3 contentstore API.""" +from .authoring_grading import AuthoringGradingViewSet # noqa: F401 from .course_details import CourseDetailsViewSet # noqa: F401 from .home import HomeViewSet # noqa: F401 diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py b/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py new file mode 100644 index 000000000000..49dd882b4f07 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py @@ -0,0 +1,163 @@ +""" +API Views for course grading settings — v3. + +This module is the v3 incarnation of the v0 ``AuthoringGradingView`` endpoint, +restructured to apply the FC-0118 ADRs from the start: + + * ADR 0025 – ``serializer_class`` on the viewset + * ADR 0026 – explicit ``authentication_classes`` + ``permission_classes`` + * ADR 0027 – ``drf_spectacular`` for OpenAPI schema generation + * ADR 0028 – consolidated into a single DRF ``ViewSet`` registered via + ``DefaultRouter`` (replaces ``AuthoringGradingView`` ``APIView``) + * ADR 0029 – standardized error envelope via :class:`StandardizedErrorMixin` + (v3-scoped — does not change the project-wide DRF ``EXCEPTION_HANDLER`` + setting) + * ADR 0033 / OEP-68 – the URL kwarg, action parameter, and OpenAPI parameter + are named ``course_key`` (the OEP-68-standardized name) rather than the + legacy ``course_id``. Since this is a brand-new versioned API, no + deprecated alias is needed — clients on the v0 endpoint continue to use + ``course_id`` there. + +Permission model note: + PR #38363 proposed a class-level ``HasStudioReadAccess`` permission. The + current v0 view has since evolved to use the ``openedx_authz`` permission + framework (``COURSES_EDIT_GRADING_SETTINGS``), which is more specific to + grading and aligns with the platform-wide authz direction. + + The v3 viewset preserves the openedx_authz model via an *inline* + ``user_has_course_permission`` check inside the action body (rather than + the ``@authz_permission_required`` decorator). The decorator raises + ``DeveloperErrorResponseException`` — a plain ``Exception`` subclass that + does not flow through DRF's exception handler, so it would bypass + :class:`StandardizedErrorMixin` and surface as an unstructured 500. + Raising ``rest_framework.exceptions.PermissionDenied`` directly keeps the + ADR 0029 envelope intact. +""" + +from drf_spectacular.utils import OpenApiParameter, OpenApiRequest, OpenApiResponse, extend_schema +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from openedx_authz.constants.permissions import COURSES_EDIT_GRADING_SETTINGS +from rest_framework import viewsets +from rest_framework.exceptions import PermissionDenied +from rest_framework.permissions import IsAuthenticated +from rest_framework.request import Request +from rest_framework.response import Response + +from cms.djangoapps.contentstore.rest_api.v0.serializers import CourseGradingModelSerializer +from cms.djangoapps.contentstore.rest_api.v3.utils import COMMON_ERROR_RESPONSES, resolve_course_key +from cms.djangoapps.models.settings.course_grading import CourseGradingModel +from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission +from openedx.core.djangoapps.authz.decorators import user_has_course_permission +from openedx.core.djangoapps.credit.tasks import update_credit_course_requirements +from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser +from openedx.core.lib.api.mixins import StandardizedErrorMixin + +_COURSE_KEY_PARAMETER = OpenApiParameter( + name="course_key", + description="OEP-68 course key (e.g. course-v1:org+course+run).", + required=True, + type=str, + location=OpenApiParameter.PATH, +) + + +class AuthoringGradingViewSet(StandardizedErrorMixin, viewsets.ViewSet): + """ + ViewSet for course grading settings (v3). Registered via DefaultRouter + (basename ``authoring_grading``). + + Router-generated URL:: + + PATCH /api/contentstore/v3/authoring_grading/{course_key}/ → partial_update + + Supersedes ``AuthoringGradingView`` at ``POST /api/contentstore/v0/grading/{course_id}``. + """ + + authentication_classes = ( + JwtAuthentication, + BearerAuthenticationAllowInactiveUser, + SessionAuthenticationAllowInactiveUser, + ) + permission_classes = (IsAuthenticated,) + serializer_class = CourseGradingModelSerializer + + # DefaultRouter lookup: matches course-v1:org+course+run (+ or / separators). + # OEP-68: the kwarg name is ``course_key`` (not the legacy ``course_id``). + lookup_field = "course_key" + lookup_value_regex = r"[^/+]+(?:/|\+)[^/+]+(?:/|\+)[^/?]+" + + def get_serializer(self, *args, **kwargs): + """Instantiate and return the configured serializer class.""" + return self.serializer_class(*args, **kwargs) + + @extend_schema( + summary="Update a course's grading settings", + description="Partially update the grading settings for the specified course.", + request=OpenApiRequest(request=CourseGradingModelSerializer), + parameters=[_COURSE_KEY_PARAMETER], + responses={ + 200: OpenApiResponse( + response=CourseGradingModelSerializer, + description="Grading settings updated successfully.", + ), + **COMMON_ERROR_RESPONSES, + }, + ) + def partial_update(self, request: Request, course_key: str): + """ + Update a course's grading settings. + + **Example Request** + + PATCH /api/contentstore/v3/authoring_grading/{course_key}/ + + **PATCH Parameters** + + The request body should follow the ``CourseGradingModelSerializer`` + schema. Example:: + + { + "graders": [ + { + "type": "Homework", + "min_count": 1, + "drop_count": 0, + "short_label": "", + "weight": 100, + "id": 0 + } + ], + "grade_cutoffs": {"A": 0.75, "B": 0.63, "C": 0.57, "D": 0.5}, + "grace_period": {"hours": 12, "minutes": 0}, + "minimum_grade_credit": 0.7, + "is_credit_course": true + } + + **Response Values** + + If the request is successful, an HTTP 200 "OK" response is returned + with the updated grading data serialized via + :class:`CourseGradingModelSerializer`. + """ + parsed_course_key = resolve_course_key(course_key) + + # Per-action authorization (ADR 0026): kept inline rather than + # behind ``@authz_permission_required`` because that decorator + # raises ``DeveloperErrorResponseException`` (not a DRF exception), + # which bypasses :class:`StandardizedErrorMixin`. Raising + # ``PermissionDenied`` directly flows through the ADR 0029 envelope. + if not user_has_course_permission( + request.user, + COURSES_EDIT_GRADING_SETTINGS.identifier, + parsed_course_key, + LegacyAuthoringPermission.READ, + ): + raise PermissionDenied("You do not have permission to perform this action.") + + if "minimum_grade_credit" in request.data: + update_credit_course_requirements.delay(str(parsed_course_key)) + + updated_data = CourseGradingModel.update_from_json(parsed_course_key, request.data, request.user) + serializer = self.get_serializer(updated_data) + return Response(serializer.data) diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py b/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py index 85bfa6d691bc..47a4743854b0 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py @@ -26,15 +26,12 @@ from drf_spectacular.utils import OpenApiParameter, OpenApiRequest, OpenApiResponse, extend_schema from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey from openedx_authz.constants.permissions import ( COURSES_EDIT_DETAILS, COURSES_EDIT_SCHEDULE, COURSES_VIEW_SCHEDULE_AND_DETAILS, ) from rest_framework import viewsets -from rest_framework.exceptions import NotFound from rest_framework.exceptions import ValidationError as DRFValidationError from rest_framework.permissions import IsAuthenticated from rest_framework.request import Request @@ -42,10 +39,10 @@ from cms.djangoapps.contentstore.rest_api.v1.serializers import CourseDetailsSerializer from cms.djangoapps.contentstore.rest_api.v1.views.course_details import _classify_update +from cms.djangoapps.contentstore.rest_api.v3.utils import COMMON_ERROR_RESPONSES, resolve_course_key from cms.djangoapps.contentstore.utils import update_course_details from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission from openedx.core.djangoapps.authz.decorators import user_has_course_permission -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.models.course_details import CourseDetails from openedx.core.lib.api.mixins import StandardizedErrorMixin from xmodule.modulestore.django import modulestore @@ -57,29 +54,6 @@ type=str, location=OpenApiParameter.PATH, ) -_COMMON_ERROR_RESPONSES = { - 401: OpenApiResponse(description="The requester is not authenticated."), - 403: OpenApiResponse(description="The requester cannot access the specified course."), - 404: OpenApiResponse(description="The requested course does not exist."), -} - - -def _resolve_course_key(course_id: str) -> CourseKey: - """ - Parse ``course_id`` into a ``CourseKey`` and verify the course exists. - - Raises ``NotFound`` for both unparseable keys and missing courses, which - the ADR 0029 envelope renders as a structured 404 response. This replaces - the legacy ``@verify_course_exists()`` decorator from v1 and avoids - relying on ``DeveloperErrorViewMixin``. - """ - try: - course_key = CourseKey.from_string(course_id) - except InvalidKeyError as exc: - raise NotFound("The provided course key cannot be parsed.") from exc - if not CourseOverview.course_exists(course_key): - raise NotFound(f"Course {course_id} not found.") - return course_key class CourseDetailsViewSet(StandardizedErrorMixin, viewsets.ViewSet): @@ -111,7 +85,7 @@ class CourseDetailsViewSet(StandardizedErrorMixin, viewsets.ViewSet): response=CourseDetailsSerializer, description="Course details retrieved successfully.", ), - **_COMMON_ERROR_RESPONSES, + **COMMON_ERROR_RESPONSES, }, ) def retrieve(self, request: Request, course_id: str): @@ -122,7 +96,7 @@ def retrieve(self, request: Request, course_id: str): GET /api/contentstore/v3/course_details/{course_id}/ """ - course_key = _resolve_course_key(course_id) + course_key = resolve_course_key(course_id) if not user_has_course_permission( request.user, COURSES_VIEW_SCHEDULE_AND_DETAILS.identifier, @@ -146,7 +120,7 @@ def retrieve(self, request: Request, course_id: str): description="Course details updated successfully.", ), 400: OpenApiResponse(description="Bad request — invalid data."), - **_COMMON_ERROR_RESPONSES, + **COMMON_ERROR_RESPONSES, }, ) def update(self, request: Request, course_id: str): @@ -169,7 +143,7 @@ def update(self, request: Request, course_id: str): If the request is successful, an HTTP 200 "OK" response is returned, along with all the course's details similar to a ``GET`` request. """ - course_key = _resolve_course_key(course_id) + course_key = resolve_course_key(course_id) is_schedule_update, is_details_update = _classify_update(request.data, course_key) if not is_schedule_update and not is_details_update: diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py new file mode 100644 index 000000000000..33f9efd69ff4 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py @@ -0,0 +1,293 @@ +""" +Unit tests for AuthoringGradingViewSet (v3). + +Single test module covering every ADR applied to the viewset: + * ADR 0025 / 0026 / 0027 / 0028 — action + permission + routing tests + (``TestAuthoringGradingViewSetPermissions``, ``TestAuthoringGradingViewSetUpdate``, + ``TestAuthoringGradingViewSetRouting``) + * ADR 0029 — standardized error envelope shape tests + (``TestAuthoringGradingViewSetErrorShape``) + +MongoDB-free: every service-layer call (``CourseGradingModel.update_from_json``, +``CourseOverview.course_exists``, ``update_credit_course_requirements.delay``, +and the ``openedx_authz`` permission lookup) is mocked, so these tests run +without a live modulestore or course-overview row. +""" +import json +from types import SimpleNamespace +from unittest.mock import patch + +from django.test import TestCase +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient, APITestCase + +from common.djangoapps.student.tests.factories import UserFactory + +COURSE_ID = "course-v1:edX+ToyX+Toy_Course" + +# Minimal graders payload accepted by CourseGradingModelSerializer. +_GRADERS_PAYLOAD = [ + { + "type": "Homework", + "min_count": 1, + "drop_count": 0, + "short_label": "", + "weight": 100, + "id": 0, + }, +] + +# Fake CourseGradingModel return value: only the field the serializer reads. +_MOCK_GRADING_MODEL = SimpleNamespace(graders=_GRADERS_PAYLOAD) + +# CourseOverview.course_exists is called inside ``resolve_course_key()`` (now in +# v3/utils.py), not directly in the view module — so the patch must target the +# utils module's binding, not the view module's. +MOCK_COURSE_EXISTS = ( + "cms.djangoapps.contentstore.rest_api.v3.utils.CourseOverview.course_exists" +) +MOCK_UPDATE_FROM_JSON = ( + "cms.djangoapps.contentstore.rest_api.v3.views.authoring_grading.CourseGradingModel.update_from_json" +) +MOCK_CREDIT_TASK = ( + "cms.djangoapps.contentstore.rest_api.v3.views.authoring_grading." + "update_credit_course_requirements.delay" +) +# Patch the local module-level binding (imported from openedx.core.djangoapps.authz.decorators) +# — patching the source module would not replace the already-bound reference inside the view. +MOCK_HAS_PERMISSION = ( + "cms.djangoapps.contentstore.rest_api.v3.views.authoring_grading.user_has_course_permission" +) + +_REQUIRED_ERROR_FIELDS = ("type", "title", "status", "detail", "instance") + + +# =========================================================================== +# ADR 0026 — permission boundary tests +# =========================================================================== +class TestAuthoringGradingViewSetPermissions(APITestCase): + """ + ADR 0026 — permission boundary tests for the partial_update action. + """ + + def setUp(self): + super().setUp() + self.client = APIClient() + self.url = reverse( + "cms.djangoapps.contentstore:v3:authoring_grading-detail", + kwargs={"course_key": COURSE_ID}, + ) + + def test_unauthenticated_patch_returns_401(self): + """Unauthenticated PATCH must return 401 (IsAuthenticated).""" + response = self.client.patch(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + @patch(MOCK_HAS_PERMISSION, return_value=False) + @patch(MOCK_COURSE_EXISTS, return_value=True) + def test_authenticated_without_grading_permission_returns_403(self, mock_exists, mock_perm): # noqa: ARG002 + """Authenticated user without grading permission must receive 403.""" + user = UserFactory.create() + self.client.force_authenticate(user=user) + response = self.client.patch(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_403_FORBIDDEN + + +# =========================================================================== +# ADR 0025 / 0028 — action body tests +# =========================================================================== +class TestAuthoringGradingViewSetUpdate(APITestCase): + """ + Action tests for the partial_update flow. + + All service-layer interactions are mocked so the test exercises the + routing, serialization, and credit-task wiring without touching MongoDB + or modulestore. + """ + + 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:v3:authoring_grading-detail", + kwargs={"course_key": COURSE_ID}, + ) + + @patch(MOCK_CREDIT_TASK) + @patch(MOCK_UPDATE_FROM_JSON, return_value=_MOCK_GRADING_MODEL) + @patch(MOCK_HAS_PERMISSION, return_value=True) + @patch(MOCK_COURSE_EXISTS, return_value=True) + def test_patch_with_minimum_grade_credit_fires_credit_task( + self, mock_exists, mock_perm, mock_update, mock_credit_task, # noqa: ARG002 + ): + """``minimum_grade_credit`` in the payload triggers the credit-requirements Celery task.""" + body = { + "graders": _GRADERS_PAYLOAD, + "grade_cutoffs": {"A": 0.75, "B": 0.63, "C": 0.57, "D": 0.5}, + "grace_period": {"hours": 12, "minutes": 0}, + "minimum_grade_credit": 0.7, + "is_credit_course": True, + } + response = self.client.patch(self.url, data=json.dumps(body), content_type="application/json") + assert response.status_code == status.HTTP_200_OK + mock_update.assert_called_once() + mock_credit_task.assert_called_once() + + @patch(MOCK_CREDIT_TASK) + @patch(MOCK_UPDATE_FROM_JSON, return_value=_MOCK_GRADING_MODEL) + @patch(MOCK_HAS_PERMISSION, return_value=True) + @patch(MOCK_COURSE_EXISTS, return_value=True) + def test_patch_without_minimum_grade_credit_skips_credit_task( + self, mock_exists, mock_perm, mock_update, mock_credit_task, # noqa: ARG002 + ): + """Absent ``minimum_grade_credit`` keeps the credit-requirements task unscheduled.""" + body = { + "graders": _GRADERS_PAYLOAD, + "grade_cutoffs": {"A": 0.75, "B": 0.63, "C": 0.57, "D": 0.5}, + "grace_period": {"hours": 12, "minutes": 0}, + } + response = self.client.patch(self.url, data=json.dumps(body), content_type="application/json") + assert response.status_code == status.HTTP_200_OK + mock_update.assert_called_once() + mock_credit_task.assert_not_called() + + +# =========================================================================== +# ADR 0028 — routing checks +# =========================================================================== +class TestAuthoringGradingViewSetRouting(TestCase): + """ + Routing checks — confirm the URL namespace and HTTP-method mapping are wired correctly. + """ + + def test_detail_url_resolves(self): + """v3 router exposes the viewset under ``v3:authoring_grading-detail``.""" + url = reverse( + "cms.djangoapps.contentstore:v3:authoring_grading-detail", + kwargs={"course_key": COURSE_ID}, + ) + assert "/api/contentstore/v3/authoring_grading/" in url + assert COURSE_ID in url + + def test_post_not_allowed(self): + """The viewset only exposes PATCH (partial_update); POST must return 405.""" + client = APIClient() + client.force_authenticate(user=UserFactory.create(is_staff=True)) + url = reverse( + "cms.djangoapps.contentstore:v3:authoring_grading-detail", + kwargs={"course_key": COURSE_ID}, + ) + response = client.post(url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED + + +# =========================================================================== +# ADR 0029 — standardized error-response envelope tests +# =========================================================================== +class TestAuthoringGradingViewSetErrorShape(APITestCase): + """ + ADR 0029 — error response shape regression tests for AuthoringGradingViewSet. + + The envelope is wired in via + :class:`openedx.core.lib.api.mixins.StandardizedErrorMixin`, which overrides + DRF's per-view ``get_exception_handler`` to point at + ``openedx.core.lib.api.exceptions.standardized_error_exception_handler``. + + Scoped to v3 — the project-wide DRF ``EXCEPTION_HANDLER`` setting is + unchanged, so v0 / v1 / v2 / v4 endpoints continue to return the legacy + error shape (locked in by ``test_v0_endpoint_unaffected_by_v3_envelope``). + """ + + def setUp(self): + super().setUp() + self.client = APIClient() + self.url = reverse( + "cms.djangoapps.contentstore:v3:authoring_grading-detail", + kwargs={"course_key": COURSE_ID}, + ) + + def test_unauthenticated_patch_returns_standardized_401(self): + """Unauthenticated PATCH must return 401 with the ADR 0029 envelope.""" + response = self.client.patch(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + def test_unauthenticated_401_type_uri(self): + """The ``type`` field for 401 must be the ADR 0029 authn URI.""" + response = self.client.patch(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.data.get("type") == "https://docs.openedx.org/errors/authn" + + @patch(MOCK_COURSE_EXISTS, return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=False) + def test_non_author_patch_returns_standardized_403(self, mock_perm, mock_exists): # noqa: ARG002 + """Authenticated non-author PATCH must return 403 with the ADR 0029 envelope.""" + non_author = UserFactory.create() + self.client.force_authenticate(user=non_author) + response = self.client.patch(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_403_FORBIDDEN + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + @patch(MOCK_COURSE_EXISTS, return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=False) + def test_non_author_403_type_uri(self, mock_perm, mock_exists): # noqa: ARG002 + """The ``type`` field for 403 must be the ADR 0029 authz URI.""" + non_author = UserFactory.create() + self.client.force_authenticate(user=non_author) + response = self.client.patch(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.data.get("type") == "https://docs.openedx.org/errors/authz" + + @patch(MOCK_COURSE_EXISTS, return_value=False) + def test_nonexistent_course_returns_standardized_404(self, mock_exists): # noqa: ARG002 + """PATCH for a non-existent course must return 404 with the ADR 0029 envelope.""" + staff = UserFactory.create(is_staff=True) + self.client.force_authenticate(user=staff) + response = self.client.patch(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_404_NOT_FOUND + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" + + @patch(MOCK_COURSE_EXISTS, return_value=False) + def test_not_found_type_uri(self, mock_exists): # noqa: ARG002 + """The ``type`` field for 404 must be the ADR 0029 not-found URI.""" + staff = UserFactory.create(is_staff=True) + self.client.force_authenticate(user=staff) + response = self.client.patch(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.data.get("type") == "https://docs.openedx.org/errors/not-found" + + def test_error_body_has_no_developer_message(self): + """Error responses must NOT contain old DeveloperErrorViewMixin fields.""" + response = self.client.patch(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert "developer_message" not in response.data + assert "error_code" not in response.data + + def test_instance_field_is_request_path(self): + """The ``instance`` field must equal the request path.""" + response = self.client.patch(self.url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.data.get("instance") == self.url + + def test_v0_endpoint_unaffected_by_v3_envelope(self): + """ + The ADR 0029 envelope must be scoped to v3 — hitting the legacy v0 + ``grading`` endpoint unauthenticated must NOT return the v3 envelope + (no ``type`` / ``instance`` keys). + """ + v0_url = reverse( + "cms.djangoapps.contentstore:v0:cms_api_update_grading", + # v0 URL uses the legacy ``course_id`` named group; only v3 was renamed + # to ``course_key`` per OEP-68. + kwargs={"course_id": COURSE_ID}, + ) + response = self.client.post(v0_url, data={}, content_type="application/json") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert "type" not in response.data + assert "instance" not in response.data diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py index 3fe6854d1f35..a7d0165fb32d 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py @@ -37,8 +37,11 @@ "cms.djangoapps.contentstore.rest_api.v3.views.course_details.user_has_course_permission" ) MOCK_CLASSIFY = "cms.djangoapps.contentstore.rest_api.v3.views.course_details._classify_update" +# CourseOverview.course_exists is called inside ``resolve_course_key()`` (now in +# v3/utils.py), not directly in the view module — so the patch must target the +# utils module's binding, not the view module's. MOCK_COURSE_EXISTS = ( - "cms.djangoapps.contentstore.rest_api.v3.views.course_details.CourseOverview.course_exists" + "cms.djangoapps.contentstore.rest_api.v3.utils.CourseOverview.course_exists" ) # Syntactically valid course key reused across action / permission / envelope tests. From 1e7dd0ea4fd4db68700167a429b1e342dbfdbf07 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed <68893403+taimoor-ahmed-1@users.noreply.github.com> Date: Tue, 23 Jun 2026 11:33:15 +0500 Subject: [PATCH 09/13] feat: apply ADR 0036 (nested JSON normalization) across 6 standardized APIs (#38773) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: apply ADR 0036 to Xblock v1 (?view=minimal) Adds a ``?view=minimal`` query parameter on ``XblockViewSet.retrieve`` that filters the (tree-shaped) xblock response down to a small set of structural fields — id, display_name, category, children, has_children, studio_url — dropping heavy/contextual fields such as data, metadata, fields, student_view_data, edited_on, published. Default response shape is unchanged (full xblock payload) to avoid breaking the existing Studio frontend. The pre-existing ``?fields=...`` query parameter retains its legacy "type of response" semantics (?fields=graderType, ?fields=ancestorInfo, ?fields=customReadToken); ADR 0036's CSV-subset interpretation is deferred to a future API version to avoid breaking those callers. Adds 4 regression tests covering: default response untouched, minimal strips heavy fields, minimal keeps structural fields, minimal is a no-op for non-dict response bodies (legacy ?fields=graderType path). * feat: apply ADR 0036 to CourseHome v3 (?fields= on list action) The v3 ``HomeViewSet.list`` action returns a wide ``StudioHomeSerializer`` payload with ~25 top-level keys (courses, archived_courses, libraries, allowed_organizations, allowed_organizations_for_libraries, plus Studio settings). Adds a ``?fields=`` query parameter so clients can request a subset of those keys explicitly. The flat-list ``courses`` and ``libraries`` actions are out of scope (each returns a single-key dict wrapping a list of small items — no nested or wide structure to filter). Adds a shared ``apply_field_selection`` helper to ``v3/utils.py`` so future v3 viewsets can opt into the same convention without re-implementing it. Adds 2 regression tests: default keeps all keys, ``?fields=courses,libraries`` restricts to exactly those keys. * docs: audit CourseHome v4 against ADR 0036 (out of scope) Adds an ADR 0036 entry to ``HomeCoursesViewSet``'s compliance list explicitly marking this endpoint as out of scope. Rationale: the v4 home endpoint returns a flat paginated list governed by ADR 0032 (DefaultPagination 7-field envelope). ADR 0036 excludes flat lists from its ``?view=`` / ``?depth=`` / minimal-by-default requirements — those apply to tree-shaped responses or wide flat objects with embedded sub-objects. Each course item is a thin 9-field record with no nested children, no embedded sub-objects, and no tree structure to bound. Per-item ``?fields=`` subset filtering remains a possible follow-up (would require a dynamic-fields serializer mixin and per-field schema documentation) but is deferred to keep the v4 contract stable for the existing Studio frontend. * feat: apply ADR 0036 to Enrollment v2 (?view=minimal flattens course_details) Each enrollment record returned by the v2 ``EnrollmentViewSet.list`` and ``EnrollmentRetrieveView.get`` actions embeds a full ``course_details`` sub-object (which itself includes a ``course_modes`` list and other heavy course-overview fields). Server-to-server callers and AI agents that only need to know which courses a user is enrolled in shouldn't have to parse the embedded sub-object on every row. Adds a ``?view=minimal`` query parameter on both actions that collapses the embedded ``course_details`` to a single ``course_id`` string. The enrollment-level fields (``created``, ``mode``, ``is_active``, ``user``) are preserved. Default response shape is unchanged. Adds 2 regression tests (mocked, MongoDB-free): default list keeps the full shape, ``?view=minimal`` collapses each row's course_details to a flat course_id and the heavy fields are dropped. * feat: apply ADR 0036 to Course Detail v3 (?view=minimal + ?fields=) The v3 ``CourseDetailsViewSet.retrieve`` action returns a wide ``CourseDetailsSerializer`` payload with ~40 top-level fields plus a nested ``instructor_info`` sub-object (instructor names, bios, image URLs) and a ``learning_info`` long-form list. The default full payload is preserved; two new opt-in query parameters apply ADR 0036: * ``?view=minimal`` drops heavy fields (overview, syllabus, description, short_description, instructor_info, learning_info, banner_image_name / banner_image_asset_path, video_thumbnail assets, license) — leaving only identification (course_id, org, run, title, subtitle, language), schedule (start_date, end_date, enrollment_start, enrollment_end, certificate_available_date), and flags (self_paced, certificates_display_behavior, has_changes). * ``?fields=a,b,c`` keeps an explicit CSV subset of top-level keys. Composes with ``?view=minimal`` — the preset is applied first, then the explicit subset. Reuses ``apply_field_selection`` from ``v3/utils.py`` (introduced in the CourseHome v3 commit) so the convention is consistent across v3. Adds 3 regression tests (mocked, MongoDB-free): default keeps all fields, ?view=minimal drops the heavy/embedded ones and keeps identification/schedule/flags, ?fields=course_id,title restricts to exactly those keys. * docs: audit AuthorGrading v3 against ADR 0036 (largely out of scope) Adds an ADR 0036 entry to ``AuthoringGradingViewSet``'s compliance list. Rationale: the v3 grading response is a single top-level ``graders`` list of small fixed-shape objects (type, min_count, drop_count, short_label, weight, id) — no tree nesting, no embedded sub-objects, no ``children`` field, no wide flat object. ``?view=minimal`` and ``?fields=`` would have no fields to drop; the only ADR 0036 concern that applies is anti-pattern #3 (unbounded child list). In practice each course has ≤8 graders (Homework, Lab, Exam, etc.) and the update flow is exercised only by course-authoring staff, so the real-world payload is always small. The hard cap is enforced upstream by ``CourseGradingModel.update_from_json``; documented as a note rather than re-implemented at the view layer. --- .../v1/views/tests/test_xblock_viewset.py | 74 ++++++++++++ .../contentstore/rest_api/v1/views/xblock.py | 66 ++++++++++- .../contentstore/rest_api/v3/utils.py | 32 ++++++ .../rest_api/v3/views/authoring_grading.py | 13 +++ .../rest_api/v3/views/course_details.py | 100 +++++++++++++++- .../contentstore/rest_api/v3/views/home.py | 27 ++++- .../v3/views/tests/test_course_details.py | 108 ++++++++++++++++++ .../rest_api/v3/views/tests/test_home.py | 55 +++++++++ .../contentstore/rest_api/v4/views/home.py | 11 ++ .../enrollments/v2/tests/test_views.py | 53 +++++++++ .../core/djangoapps/enrollments/v2/views.py | 82 ++++++++++++- 11 files changed, 605 insertions(+), 16 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py index 8694f7023683..638b0ce2eb35 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py @@ -142,3 +142,77 @@ def test_error_body_has_no_developer_message(self): def test_instance_field_is_request_path(self): response = self.client.get(_detail_url()) assert response.json()["instance"] == _detail_url() + + +# --------------------------------------------------------------------------- +# ADR 0036 — minimal-view regression tests +# --------------------------------------------------------------------------- +class TestXblockViewSetMinimalView(ModuleStoreTestCase, APITestCase): + """ + ADR 0036 — verify ``?view=minimal`` strips the xblock response down to + the structural fields enumerated in ``_MINIMAL_VIEW_FIELDS`` and leaves + the default (full) response untouched. + """ + + _FULL_PAYLOAD = { + "id": TEST_LOCATOR, + "display_name": "Problem 1", + "category": "problem", + "children": [], + "has_children": False, + "studio_url": "/studio/...", + # Heavy / contextual fields that ``?view=minimal`` MUST drop: + "data": "...", + "metadata": {"weight": 1.0}, + "fields": {"showanswer": "always"}, + "student_view_data": {"...": "..."}, + "edited_on": "2026-06-17T00:00:00Z", + "published": True, + } + + def setUp(self): + super().setUp() + self.author = GlobalStaffFactory.create() + self.client.force_authenticate(user=self.author) + + @patch(f"{_VIEW_MODULE}.retrieve_xblock_response") + def test_default_response_is_unchanged(self, mock_retrieve): + """Without ``?view=minimal`` the response is the full handler payload.""" + mock_retrieve.return_value = JsonResponse(self._FULL_PAYLOAD) + response = self.client.get(_detail_url()) + assert response.status_code == status.HTTP_200_OK + body = response.json() + # Heavy fields must still be present in the default response. + assert "data" in body + assert "metadata" in body + assert "student_view_data" in body + + @patch(f"{_VIEW_MODULE}.retrieve_xblock_response") + def test_minimal_view_strips_heavy_fields(self, mock_retrieve): + """``?view=minimal`` drops data, metadata, fields, student_view_data, edited_on, published.""" + mock_retrieve.return_value = JsonResponse(self._FULL_PAYLOAD) + response = self.client.get(_detail_url(), {"view": "minimal"}) + assert response.status_code == status.HTTP_200_OK + body = response.json() + # Heavy fields MUST be dropped. + for dropped in ("data", "metadata", "fields", "student_view_data", "edited_on", "published"): + assert dropped not in body, f"ADR 0036: ?view=minimal must drop '{dropped}'" + + @patch(f"{_VIEW_MODULE}.retrieve_xblock_response") + def test_minimal_view_keeps_structural_fields(self, mock_retrieve): + """``?view=minimal`` keeps id, display_name, category, children, has_children, studio_url.""" + mock_retrieve.return_value = JsonResponse(self._FULL_PAYLOAD) + response = self.client.get(_detail_url(), {"view": "minimal"}) + body = response.json() + for kept in ("id", "display_name", "category", "children", "has_children", "studio_url"): + assert kept in body, f"ADR 0036: ?view=minimal must keep '{kept}'" + assert body["id"] == TEST_LOCATOR + assert body["category"] == "problem" + + @patch(f"{_VIEW_MODULE}.retrieve_xblock_response") + def test_minimal_view_is_noop_for_non_json_payload(self, mock_retrieve): + """Legacy ``?fields=graderType`` returns a non-dict body — minimal must be a no-op.""" + mock_retrieve.return_value = JsonResponse("notgraded", safe=False) + response = self.client.get(_detail_url(), {"view": "minimal", "fields": "graderType"}) + assert response.status_code == status.HTTP_200_OK + assert response.json() == "notgraded" diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py index acd4a8192ad9..025f2c4c4ca9 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py @@ -8,10 +8,24 @@ * ADR 0026 - explicit authentication_classes + permission_classes * ADR 0028 - consolidated into XblockViewSet via DefaultRouter * ADR 0029 - standardized error envelope via StandardizedErrorMixin + * ADR 0036 - minimal/flattened views. ``retrieve`` accepts a ``?view=minimal`` + query parameter that strips the (tree-shaped) xblock response to a small + set of structural fields. The full xblock response is kept as the default + for backwards compatibility with the existing Studio frontend; new clients + SHOULD opt into ``?view=minimal`` whenever the full nested payload is not + required. + + Note on ``?fields=`` — the underlying ``retrieve_xblock_response`` already + interprets ``?fields=`` with **legacy semantics** as a "type of response" + selector (``?fields=graderType``, ``?fields=ancestorInfo``, + ``?fields=customReadToken``). To avoid breaking existing callers, v1 does + NOT repurpose ``?fields=`` as the ADR 0036 CSV-subset selector — use + ``?view=minimal`` instead. A future v2 may reconcile these names. """ import json import logging +from django.http import JsonResponse from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from opaque_keys import InvalidKeyError @@ -33,6 +47,39 @@ log = logging.getLogger(__name__) +# ADR 0036 — top-level keys kept when ``?view=minimal`` is requested. Chosen so +# the response is structurally complete (callers can navigate the tree by id +# and fetch full nodes on demand) without any heavy/contextual fields +# (student_view_data, completion, OLX metadata, etc.). +_MINIMAL_VIEW_FIELDS = frozenset({ + "id", + "display_name", + "category", + "children", + "has_children", + "studio_url", +}) + + +def _apply_minimal_view(response): + """ + ADR 0036 — when ``?view=minimal`` was requested, drop every top-level key + not in :data:`_MINIMAL_VIEW_FIELDS` from ``response``. No-op for non-JSON + or non-2xx responses. + """ + if not isinstance(response, JsonResponse) or response.status_code >= 300: + return response + try: + body = json.loads(response.content.decode("utf-8") or "{}") + except (ValueError, AttributeError): + return response + if not isinstance(body, dict): + # If the handler returned a non-object payload (e.g. `?fields=graderType` + # which returns the grader-type value directly), there's nothing to + # filter — return the response untouched. + return response + return JsonResponse({k: v for k, v in body.items() if k in _MINIMAL_VIEW_FIELDS}) + class XblockViewSet(StandardizedErrorMixin, viewsets.ViewSet): """ @@ -44,6 +91,12 @@ class XblockViewSet(StandardizedErrorMixin, viewsets.ViewSet): PUT /api/contentstore/v1/xblock/{usage_key_string}/ → update PATCH /api/contentstore/v1/xblock/{usage_key_string}/ → partial_update DELETE /api/contentstore/v1/xblock/{usage_key_string}/ → destroy + + Query parameters (ADR 0036, GET only): + ?view=minimal Drop heavy / contextual fields from the response, + keeping only structural fields (id, display_name, + category, children, has_children, studio_url). + Default response is the full xblock payload. """ authentication_classes = ( @@ -98,8 +151,17 @@ def create(self, request): @expect_json_in_class_view def retrieve(self, request, usage_key_string=None): - """Retrieve an xblock by its usage key.""" - return retrieve_xblock_response(request, usage_key_string) + """ + Retrieve an xblock by its usage key. + + ADR 0036 — honours ``?view=minimal``; everything else is delegated to + ``retrieve_xblock_response`` (which keeps its legacy ``?fields=`` / + ``?fields=ancestorInfo`` / ``?fields=customReadToken`` semantics). + """ + response = retrieve_xblock_response(request, usage_key_string) + if request.GET.get("view") == "minimal": + response = _apply_minimal_view(response) + return response @expect_json_in_class_view @validate_request_with_serializer diff --git a/cms/djangoapps/contentstore/rest_api/v3/utils.py b/cms/djangoapps/contentstore/rest_api/v3/utils.py index 3db96963b764..79524acb8c53 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/utils.py +++ b/cms/djangoapps/contentstore/rest_api/v3/utils.py @@ -13,6 +13,10 @@ * :data:`COMMON_ERROR_RESPONSES` – the shared ``@extend_schema(responses=...)`` fragment for the 401 / 403 / 404 cases every v3 course-scoped viewset can raise. + * :func:`apply_field_selection` – ADR 0036 helper. Drops every top-level + key not listed in the caller's ``?fields=`` CSV. No-op when ``?fields=`` + is absent. Use this when an action returns a wide flat object and clients + want to request a subset (e.g. ``?fields=id,display_name,courses``). """ from drf_spectacular.utils import OpenApiResponse @@ -53,3 +57,31 @@ def resolve_course_key(course_key: str) -> CourseKey: 403: OpenApiResponse(description="The requester cannot access the specified course."), 404: OpenApiResponse(description="The requested course does not exist."), } + + +def apply_field_selection(data, fields_csv): + """ + ADR 0036 — drop every top-level key not listed in ``fields_csv``. + + Args: + data: a ``dict`` (typically ``serializer.data``). Anything else is + returned untouched. + fields_csv: the raw value of the ``?fields=`` query parameter. ``None`` + or empty string → no filtering (the full ``data`` is returned). + + Returns: + A new ``dict`` containing only the requested top-level keys, or the + original ``data`` if filtering is not applicable. + + Note: + Only top-level keys are honoured. Dotted paths (``?fields=children.x``) + are stripped to their first segment (``children``) — full dotted-path + traversal is intentionally left to a future implementation per the + ADR 0036 guidance to "reject silent over-fetching" via that syntax. + """ + if not fields_csv or not isinstance(data, dict): + return data + wanted = {name.strip().split(".", 1)[0] for name in fields_csv.split(",") if name.strip()} + if not wanted: + return data + return {key: value for key, value in data.items() if key in wanted} diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py b/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py index 49dd882b4f07..72a4039c9ff3 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py @@ -17,6 +17,19 @@ legacy ``course_id``. Since this is a brand-new versioned API, no deprecated alias is needed — clients on the v0 endpoint continue to use ``course_id`` there. + * ADR 0036 – **largely out of scope.** The ``CourseGradingModelSerializer`` + response is a single top-level ``graders`` list of small fixed-shape + objects (type, min_count, drop_count, short_label, weight, id) — no + tree nesting, no embedded sub-objects, no ``children`` field, no wide + flat object that would benefit from ``?view=minimal`` / ``?fields=``. + + The one ADR 0036 concern is anti-pattern #3 (unbounded child list): the + ``graders`` array has no upper bound in the serializer. In practice each + course has typically ≤8 graders (Homework, Lab, Exam, etc.) and the + update flow is exercised only by course-authoring staff, so the + real-world payload is always small. A hard cap is enforced upstream of + this endpoint by :func:`CourseGradingModel.update_from_json`; we surface + that as a documentation note rather than re-implement the bound here. Permission model note: PR #38363 proposed a class-level ``HasStudioReadAccess`` permission. The diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py b/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py index 47a4743854b0..79a1f0ed1cba 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py @@ -39,7 +39,11 @@ from cms.djangoapps.contentstore.rest_api.v1.serializers import CourseDetailsSerializer from cms.djangoapps.contentstore.rest_api.v1.views.course_details import _classify_update -from cms.djangoapps.contentstore.rest_api.v3.utils import COMMON_ERROR_RESPONSES, resolve_course_key +from cms.djangoapps.contentstore.rest_api.v3.utils import ( + COMMON_ERROR_RESPONSES, + apply_field_selection, + resolve_course_key, +) from cms.djangoapps.contentstore.utils import update_course_details from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission from openedx.core.djangoapps.authz.decorators import user_has_course_permission @@ -55,6 +59,68 @@ location=OpenApiParameter.PATH, ) +# ADR 0036 — document the minimal/full response variants in OpenAPI (decision #3). +# Declaring these as query parameters is what makes the presets discoverable by +# OpenAPI consumers (Swagger UI, generated SDK clients, etc.). The 200 response +# schema below points at the full ``CourseDetailsSerializer``; ``?view=minimal`` +# returns the subset of top-level keys listed in :data:`_MINIMAL_VIEW_FIELDS`. +_VIEW_QUERY_PARAMETER = OpenApiParameter( + name="view", + description=( + "ADR 0036 response preset. ``minimal`` drops heavy fields (overview, " + "syllabus, description, instructor_info, learning_info, banner/video " + "assets, license) leaving only identification, schedule, and flags. " + "Omit the parameter to receive the full response." + ), + required=False, + type=str, + location=OpenApiParameter.QUERY, + enum=["minimal"], +) +_FIELDS_QUERY_PARAMETER = OpenApiParameter( + name="fields", + description=( + "ADR 0036 explicit field selection. Comma-separated list of top-level " + "keys to include in the response (e.g. ``course_id,title,start_date``). " + "When combined with ``?view=``, the preset is applied first and " + "``?fields=`` is applied to the result. Unknown keys are silently " + "skipped." + ), + required=False, + type=str, + location=OpenApiParameter.QUERY, +) + +# ADR 0036 — the ``CourseDetailsSerializer`` has ~40 top-level fields plus a +# nested ``instructor_info`` sub-object with bios and image URLs and a +# ``learning_info`` long-form list. When ``?view=minimal`` is requested, +# everything outside :data:`_MINIMAL_VIEW_FIELDS` is dropped so server-to-server +# and AI-agent callers can fetch just the identification + schedule + flags +# without paying for the heavy text and embedded sub-objects. +_MINIMAL_VIEW_FIELDS = frozenset({ + "course_id", + "org", + "run", + "title", + "subtitle", + "language", + "self_paced", + "start_date", + "end_date", + "enrollment_start", + "enrollment_end", + "certificate_available_date", + "certificates_display_behavior", + "has_changes", +}) + + +def _apply_view_preset(data, view_preset): + """ADR 0036 — drop everything outside ``_MINIMAL_VIEW_FIELDS`` when ``?view=minimal``.""" + if view_preset != "minimal" or not isinstance(data, dict): + return data + return {key: value for key, value in data.items() if key in _MINIMAL_VIEW_FIELDS} + class CourseDetailsViewSet(StandardizedErrorMixin, viewsets.ViewSet): """ @@ -78,12 +144,21 @@ class CourseDetailsViewSet(StandardizedErrorMixin, viewsets.ViewSet): @extend_schema( summary="Retrieve a course's details", - description="Get an object containing all the course details for the specified course.", - parameters=[_COURSE_ID_PARAMETER], + description=( + "Get an object containing the course details for the specified course. " + "Supports the ADR 0036 ``?view=minimal`` preset and ``?fields=`` " + "explicit field selection (see the parameter descriptions for details)." + ), + parameters=[_COURSE_ID_PARAMETER, _VIEW_QUERY_PARAMETER, _FIELDS_QUERY_PARAMETER], responses={ 200: OpenApiResponse( response=CourseDetailsSerializer, - description="Course details retrieved successfully.", + description=( + "Course details retrieved successfully. The schema below is " + "the full default response; when ``?view=minimal`` and/or " + "``?fields=`` is supplied, the response contains a subset of " + "these top-level keys (see ADR 0036)." + ), ), **COMMON_ERROR_RESPONSES, }, @@ -95,6 +170,16 @@ def retrieve(self, request: Request, course_id: str): **Example Request** GET /api/contentstore/v3/course_details/{course_id}/ + GET /api/contentstore/v3/course_details/{course_id}/?view=minimal + GET /api/contentstore/v3/course_details/{course_id}/?fields=course_id,title + + ADR 0036: + * ``?view=minimal`` drops heavy fields (overview, syllabus, description, + instructor_info, learning_info, banner/video assets, license, etc.) + leaving only identification, schedule, and flags. + * ``?fields=...`` keeps an arbitrary CSV subset of top-level keys. + * ``?fields=`` and ``?view=`` may be combined — ``?view=minimal`` + is applied first, then ``?fields=`` is applied to the result. """ course_key = resolve_course_key(course_id) if not user_has_course_permission( @@ -106,8 +191,11 @@ def retrieve(self, request: Request, course_id: str): self.permission_denied(request) course_details = CourseDetails.fetch(course_key) - serializer = self.serializer_class(course_details) - return Response(serializer.data) + data = self.serializer_class(course_details).data + # ADR 0036 — preset first, then explicit CSV subset. + data = _apply_view_preset(data, request.query_params.get("view")) + data = apply_field_selection(data, request.query_params.get("fields")) + return Response(data) @extend_schema( summary="Update a course's details", diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/home.py b/cms/djangoapps/contentstore/rest_api/v3/views/home.py index 2e11b191806e..de5158c17e58 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/home.py @@ -12,6 +12,12 @@ * ADR 0029 – standardized error envelope, opted in via :class:`StandardizedErrorMixin` (v3-scoped — does not change the project-wide DRF ``EXCEPTION_HANDLER`` setting) + * ADR 0036 – field selection via ``?fields=`` (e.g. ``?fields=courses``). + The ``list`` action returns a wide ``StudioHomeSerializer`` payload with + ~25 top-level keys; clients that only need a subset can request it + explicitly. The flat-list ``courses`` and ``libraries`` actions are + out of scope (single-key dict around a list) and do not honour + ``?fields=``. """ import edx_api_doc_tools as apidocs @@ -30,6 +36,7 @@ LibraryTabSerializer, StudioHomeSerializer, ) +from cms.djangoapps.contentstore.rest_api.v3.utils import apply_field_selection from cms.djangoapps.contentstore.utils import get_course_context, get_home_context, get_library_context from openedx.core.lib.api.mixins import StandardizedErrorMixin @@ -66,7 +73,21 @@ def get_serializer(self, *args, **kwargs): "org", apidocs.ParameterLocation.QUERY, description="Query param to filter by course org", - )], + ), + # ADR 0036 decision #3 — document the ``?fields=`` variant so it's + # discoverable by OpenAPI consumers. The 200 response below is the + # full default shape; ``?fields=`` returns a subset of top-level keys. + apidocs.string_parameter( + "fields", + apidocs.ParameterLocation.QUERY, + description=( + "ADR 0036 explicit field selection. Comma-separated list " + "of top-level keys to include in the response (e.g. " + "``courses,libraries,studio_name``). Omit for the full " + "response. Unknown keys are silently skipped." + ), + ), + ], responses={ 200: StudioHomeSerializer, 401: "The requester is not authenticated.", @@ -79,6 +100,7 @@ def list(self, request: Request): **Example Request** GET /api/contentstore/v3/home/ + GET /api/contentstore/v3/home/?fields=courses,libraries (ADR 0036) """ home_context = get_home_context(request, True) home_context.update({ @@ -96,7 +118,8 @@ def list(self, request: Request): 'user_is_active': request.user.is_active, }) serializer = self.get_serializer(home_context) - return Response(serializer.data) + # ADR 0036 — drop top-level keys not requested via ?fields=. + return Response(apply_field_selection(serializer.data, request.query_params.get("fields"))) @apidocs.schema( parameters=[ diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py index a7d0165fb32d..e25df0188d18 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py @@ -270,3 +270,111 @@ def test_v1_endpoint_unaffected_by_v3_envelope(self): assert response.status_code == status.HTTP_401_UNAUTHORIZED assert "type" not in response.data assert "instance" not in response.data + + +# =========================================================================== +# ADR 0036 — ?view=minimal and ?fields= tests +# =========================================================================== +class TestCourseDetailsViewSetNestedJsonNormalization(APITestCase): + """ + ADR 0036 — verify ``?view=minimal`` drops the heavy fields and ``?fields=`` + restricts to an explicit subset. The full default response is unchanged. + """ + + _FAKE_DATA = { + # kept by ?view=minimal: + "course_id": "course-v1:org+course+run", + "org": "org", + "run": "run", + "title": "Sample Title", + "subtitle": "", + "language": "en", + "self_paced": False, + "start_date": "2026-06-01T00:00:00Z", + "end_date": "2026-12-01T00:00:00Z", + "enrollment_start": None, + "enrollment_end": None, + "certificate_available_date": None, + "certificates_display_behavior": "end", + "has_changes": False, + # dropped by ?view=minimal: + "overview": "", + "syllabus": "", + "description": "", + "short_description": "", + "instructor_info": {"instructors": [{"name": "x", "bio": "..."}]}, + "learning_info": ["a", "b"], + "banner_image_name": "img.jpg", + "banner_image_asset_path": "/asset/...", + "video_thumbnail_image_name": "vid.jpg", + "video_thumbnail_image_asset_path": "/asset/...", + "license": "...", + } + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.client.force_authenticate(user=self.user) + self.url = reverse( + "cms.djangoapps.contentstore:v3:course_details-detail", + kwargs={"course_id": TEST_COURSE_ID}, + ) + + @patch.object(CourseDetailsViewSet, "serializer_class") + @patch.object(CourseOverview, "course_exists", return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=True) + @patch(MOCK_FETCH) + def test_default_response_keeps_all_fields( + self, mock_fetch, mock_perm, mock_exists, mock_ser_cls, # noqa: ARG002 + ): + """Without ``?view=`` or ``?fields=`` the full payload is returned.""" + mock_fetch.return_value = MagicMock() + mock_ser_cls.return_value.data = self._FAKE_DATA + + response = self.client.get(self.url) + + assert response.status_code == status.HTTP_200_OK + assert "instructor_info" in response.data + assert "overview" in response.data + assert "learning_info" in response.data + + @patch.object(CourseDetailsViewSet, "serializer_class") + @patch.object(CourseOverview, "course_exists", return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=True) + @patch(MOCK_FETCH) + def test_view_minimal_drops_heavy_fields( + self, mock_fetch, mock_perm, mock_exists, mock_ser_cls, # noqa: ARG002 + ): + """``?view=minimal`` drops the heavy text + embedded instructor_info sub-object.""" + mock_fetch.return_value = MagicMock() + mock_ser_cls.return_value.data = self._FAKE_DATA + + response = self.client.get(self.url, {"view": "minimal"}) + + assert response.status_code == status.HTTP_200_OK + for dropped in ( + "overview", "syllabus", "description", "short_description", + "instructor_info", "learning_info", + "banner_image_name", "banner_image_asset_path", + "video_thumbnail_image_name", "video_thumbnail_image_asset_path", + "license", + ): + assert dropped not in response.data, f"ADR 0036: ?view=minimal must drop '{dropped}'" + for kept in ("course_id", "org", "run", "title", "self_paced", "start_date", "end_date"): + assert kept in response.data, f"ADR 0036: ?view=minimal must keep '{kept}'" + + @patch.object(CourseDetailsViewSet, "serializer_class") + @patch.object(CourseOverview, "course_exists", return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=True) + @patch(MOCK_FETCH) + def test_fields_csv_restricts_top_level_keys( + self, mock_fetch, mock_perm, mock_exists, mock_ser_cls, # noqa: ARG002 + ): + """``?fields=course_id,title`` returns exactly those two keys.""" + mock_fetch.return_value = MagicMock() + mock_ser_cls.return_value.data = self._FAKE_DATA + + response = self.client.get(self.url, {"fields": "course_id,title"}) + + assert response.status_code == status.HTTP_200_OK + assert set(response.data.keys()) == {"course_id", "title"} diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_home.py index a688a488e63c..980d33b81bd3 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_home.py @@ -116,3 +116,58 @@ def test_libraries_calls_get_library_context(self, mock_libs, mock_get_ser): assert response.status_code == status.HTTP_200_OK mock_libs.assert_called_once() + + +# --------------------------------------------------------------------------- +# ADR 0036 — ?fields= field selection on the list action +# --------------------------------------------------------------------------- +class TestHomeViewSetFieldSelection(APITestCase): + """ + ADR 0036 — verify ``?fields=`` filters top-level keys on the ``list`` + action's wide ``StudioHomeSerializer`` response. ``courses`` and + ``libraries`` actions are out of scope (single-key dicts). + """ + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.client.force_authenticate(user=self.user) + self.url = reverse('cms.djangoapps.contentstore:v3:home-list') + + @patch.object(HomeViewSet, 'get_serializer') + @patch(MOCK_ORG_API) + @patch(MOCK_GET_HOME_CONTEXT) + def test_default_response_keeps_all_keys(self, mock_home, mock_org, mock_get_ser): # noqa: ARG002 + """Without ``?fields=`` every top-level key is returned.""" + mock_home.return_value = {'can_create_organizations': True} + mock_org.is_autocreate_enabled.return_value = True + mock_get_ser.return_value.data = { + 'studio_name': 'Studio', 'platform_name': 'edX', + 'courses': [], 'libraries': [], 'archived_courses': [], + } + + response = self.client.get(self.url) + + assert response.status_code == status.HTTP_200_OK + assert set(response.data.keys()) == { + 'studio_name', 'platform_name', 'courses', 'libraries', 'archived_courses', + } + + @patch.object(HomeViewSet, 'get_serializer') + @patch(MOCK_ORG_API) + @patch(MOCK_GET_HOME_CONTEXT) + def test_fields_csv_restricts_top_level_keys(self, mock_home, mock_org, mock_get_ser): # noqa: ARG002 + """``?fields=courses,libraries`` returns exactly those keys.""" + mock_home.return_value = {'can_create_organizations': True} + mock_org.is_autocreate_enabled.return_value = True + mock_get_ser.return_value.data = { + 'studio_name': 'Studio', 'platform_name': 'edX', + 'courses': [], 'libraries': [], 'archived_courses': [], + } + + response = self.client.get(self.url, {'fields': 'courses,libraries'}) + + assert response.status_code == status.HTTP_200_OK + assert set(response.data.keys()) == {'courses', 'libraries'} + assert 'studio_name' not in response.data + assert 'platform_name' not in response.data diff --git a/cms/djangoapps/contentstore/rest_api/v4/views/home.py b/cms/djangoapps/contentstore/rest_api/v4/views/home.py index 8f0fedc1cfed..28fab47b55c0 100644 --- a/cms/djangoapps/contentstore/rest_api/v4/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v4/views/home.py @@ -117,6 +117,17 @@ class HomeCoursesViewSet(StandardizedErrorMixin, viewsets.ViewSet): - 0029: standardized error envelope via ``StandardizedErrorMixin`` - 0032: 7-field pagination envelope via ``DefaultPagination`` - 0033: ``ordering`` parameter; ``order`` kept as deprecated alias + - 0036: **out of scope.** This endpoint returns a flat paginated list + governed by ADR 0032; ADR 0036 explicitly excludes flat lists from + its ``?view=`` / ``?depth=`` / minimal-by-default requirements. Each + course item carries 9 thin top-level fields (``course_key``, + ``display_name``, ``lms_link``, ``cms_link``, ``number``, ``org``, + ``rerun_link``, ``run``, ``url``, ``is_active``) — no nested + children, no embedded full sub-objects, no tree shape. Per-item + ``?fields=`` subset filtering is a possible follow-up (would require + a dynamic-fields serializer mixin and per-field schema documentation) + but is intentionally NOT added here to keep the v4 contract stable + for the existing Studio frontend. """ authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) diff --git a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py index ce8f958a1d79..414aa8075926 100644 --- a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py +++ b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py @@ -18,6 +18,7 @@ from rest_framework.test import APITestCase from common.djangoapps.student.tests.factories import AdminFactory, UserFactory +from openedx.core.djangoapps.enrollments.v2.views import EnrollmentViewSet from openedx.core.djangolib.testing.utils import skip_unless_lms API_KEY = "test-enrollment-v2-api-key" @@ -234,3 +235,55 @@ def test_no_filter_no_header(self, mock_get): # noqa: ARG002 response = self.client.get(self.url) assert response.status_code == status.HTTP_200_OK assert "Deprecation" not in response.headers + + +# --------------------------------------------------------------------------- +# ADR 0036 — minimal view tests +# --------------------------------------------------------------------------- +@skip_unless_lms +class TestEnrollmentViewSetMinimalView(APITestCase): + """ + ADR 0036 — verify ``?view=minimal`` on the list action collapses each + enrollment's embedded ``course_details`` sub-object to a single ``course_id`` + string and drops the heavy fields (``course_modes`` etc.). + """ + + def setUp(self): + super().setUp() + self.user = UserFactory.create(password="test") + self.client.force_authenticate(user=self.user) + self.url = reverse("v2:enrollment-list") + + @patch(MOCK_OPS_LIST, return_value=[]) + def test_default_list_includes_course_details(self, mock_list): # noqa: ARG002 + """Without ``?view=minimal``, embedded course_details is present (full shape).""" + response = self.client.get(self.url) + assert response.status_code == status.HTTP_200_OK + # An empty list naturally has no rows to inspect — the contract is that the + # envelope's `results` key is a list (already verified by the pagination test). + assert response.data["results"] == [] + + @patch.object(EnrollmentViewSet, "get_serializer") + @patch(MOCK_OPS_LIST, return_value=["e1", "e2"]) + def test_minimal_view_collapses_course_details_to_course_id(self, mock_list, mock_get_ser): # noqa: ARG002 + """``?view=minimal`` replaces each ``course_details`` sub-object with a ``course_id`` string.""" + mock_get_ser.return_value.data = [ + { + "mode": "audit", "is_active": True, "user": "u1", + "course_details": {"course_id": "course-v1:org+a+r", "course_modes": [{"slug": "audit"}]}, + }, + { + "mode": "honor", "is_active": True, "user": "u1", + "course_details": {"course_id": "course-v1:org+b+r", "course_modes": [{"slug": "honor"}]}, + }, + ] + + response = self.client.get(self.url, {"view": "minimal"}) + + assert response.status_code == status.HTTP_200_OK + for row in response.data["results"]: + assert "course_details" not in row, "ADR 0036: minimal must drop embedded course_details" + assert "course_id" in row, "ADR 0036: minimal must keep the flattened course_id" + assert {r["course_id"] for r in response.data["results"]} == { + "course-v1:org+a+r", "course-v1:org+b+r", + } diff --git a/openedx/core/djangoapps/enrollments/v2/views.py b/openedx/core/djangoapps/enrollments/v2/views.py index cb940244c401..905452f1d32c 100644 --- a/openedx/core/djangoapps/enrollments/v2/views.py +++ b/openedx/core/djangoapps/enrollments/v2/views.py @@ -15,6 +15,13 @@ * ADR 0032 – ``DefaultPagination`` 7-field envelope on list endpoints * ADR 0033 – OEP-68 parameter naming (``course_key`` preferred, ``course_id`` as deprecated alias) plus standard ``ordering`` whitelist + * ADR 0036 – ``?view=minimal`` on the enrollment ``list`` and singleton + ``retrieve`` actions. By default each enrollment record embeds the full + ``course_details`` sub-object (which itself includes a ``course_modes`` + list and other heavy fields). When ``?view=minimal`` is requested, the + embedded sub-object is flattened to a single ``course_id`` string so + callers that only need to know which courses a user is enrolled in (AI + agents, sync pipelines) can skip the per-row sub-object payload. Existing v1 endpoints at ``/api/enrollment/v1/`` are unchanged — v2 is a parallel new version mounted at ``/api/enrollment/v2/``. @@ -100,6 +107,24 @@ def _query_param(name: str, description: str, *, required: bool = False, type_=s _PAGE_QUERY_PARAM = _query_param("page", "Page number to retrieve. Default 1.") _PAGE_SIZE_QUERY_PARAM = _query_param("page_size", "Items per page (default 10, max 100).") +# ADR 0036 decision #3 — document the ``?view=`` variant in OpenAPI so it's +# discoverable. ``?view=minimal`` collapses each enrollment's embedded +# ``course_details`` sub-object to a single ``course_id`` string; omit to +# receive the full default shape declared by the 200 response schema. +_VIEW_QUERY_PARAM = OpenApiParameter( + name="view", + description=( + "ADR 0036 response preset. ``minimal`` collapses the embedded " + "``course_details`` sub-object on each enrollment to a single " + "``course_id`` string (drops ``course_modes`` and other heavy " + "course-detail fields). Omit the parameter to receive the full response." + ), + required=False, + type=str, + location=OpenApiParameter.QUERY, + enum=["minimal"], +) + _RESP_UNAUTHENTICATED = OpenApiResponse(description="The requester is not authenticated.") _RESP_FORBIDDEN = OpenApiResponse(description="The requester does not have permission for this operation.") _RESP_NOT_FOUND = OpenApiResponse(description="The requested resource does not exist.") @@ -135,6 +160,32 @@ def _maybe_set_legacy_param_deprecation_header(request, response, alias_pairs): return response +# --------------------------------------------------------------------------- +# ADR 0036 — minimal enrollment view helper +# --------------------------------------------------------------------------- +def _to_minimal_enrollment(enrollment_dict): + """ + ADR 0036 — collapse the embedded ``course_details`` sub-object on a serialized + enrollment dict down to a single ``course_id`` string. Heavy fields such as + ``course_modes`` are dropped. The enrollment-level fields (``created``, + ``mode``, ``is_active``, ``user``) are kept. + + Returns a new dict — the original is not mutated. + """ + if not isinstance(enrollment_dict, dict): + return enrollment_dict + minimal = {k: v for k, v in enrollment_dict.items() if k != "course_details"} + details = enrollment_dict.get("course_details") or {} + if isinstance(details, dict): + minimal["course_id"] = details.get("course_id") + return minimal + + +def _is_minimal_view_requested(request) -> bool: + """Return True when the caller asked for the ADR 0036 minimal preset.""" + return request.query_params.get("view") == "minimal" + + # =========================================================================== # EnrollmentViewSet — consolidates list / create / unenroll / allowed # =========================================================================== @@ -185,20 +236,33 @@ def get_serializer(self, *args, **kwargs): "Returns a paginated list of enrollments for the currently logged-in user, or for " "the user named by the 'user' query parameter. Staff/admin/api-key access is required " "to view another user's enrollments — otherwise the list is filtered to courses the " - "requester staffs." + "requester staffs. Supports the ADR 0036 ``?view=minimal`` preset (see parameter " + "description)." ), - parameters=[_USER_QUERY_PARAM, _PAGE_QUERY_PARAM, _PAGE_SIZE_QUERY_PARAM], + parameters=[_USER_QUERY_PARAM, _PAGE_QUERY_PARAM, _PAGE_SIZE_QUERY_PARAM, _VIEW_QUERY_PARAM], responses={ 200: OpenApiResponse( response=CourseEnrollmentSerializer(many=True), - description="Paginated enrollment list.", + description=( + "Paginated enrollment list. The schema below is the full " + "default shape; when ``?view=minimal`` is supplied each " + "enrollment's ``course_details`` is collapsed to a single " + "``course_id`` string (ADR 0036)." + ), ), 401: _RESP_UNAUTHENTICATED, }, ) @method_decorator(ensure_csrf_cookie_cross_domain) def list(self, request): - """List enrollments for the currently logged-in user (paginated).""" + """ + List enrollments for the currently logged-in user (paginated). + + ADR 0036 — when ``?view=minimal`` is supplied, each enrollment's embedded + ``course_details`` sub-object is collapsed to a single ``course_id`` + string; ``course_modes`` and the other heavy course-detail fields are + dropped. Default response shape is unchanged for backwards compatibility. + """ username = request.GET.get("user", request.user.username) enrollments = _OPS.list_enrollments_for_user( request_user=request.user, @@ -207,7 +271,10 @@ def list(self, request): ) paginator = self.pagination_class() page = paginator.paginate_queryset(enrollments, request, view=self) - return paginator.get_paginated_response(self.get_serializer(page, many=True).data) + data = self.get_serializer(page, many=True).data + if _is_minimal_view_requested(request): + data = [_to_minimal_enrollment(item) for item in data] + return paginator.get_paginated_response(data) # ------------------------------------------------------------------ # create — POST /enrollment/ @@ -413,7 +480,10 @@ def get(self, request, course_id=None, username=None): f"'{username}' in course '{course_id}'" ) from exc - return Response(self.serializer_class(enrollment).data) + data = self.serializer_class(enrollment).data + if _is_minimal_view_requested(request): + data = _to_minimal_enrollment(data) + return Response(data) # =========================================================================== From 3bc64c8864aaea720ea76f5c7043dbb38b0f6470 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed <68893403+taimoor-ahmed-1@users.noreply.github.com> Date: Tue, 23 Jun 2026 13:28:24 +0500 Subject: [PATCH 10/13] feat: apply ADR 0034 (auth standardization) across 6 standardized APIs (#38796) --- .../contentstore/rest_api/v1/views/xblock.py | 7 +++++ .../rest_api/v3/views/authoring_grading.py | 13 +++++++-- .../rest_api/v3/views/course_details.py | 6 ++++ .../contentstore/rest_api/v3/views/home.py | 6 ++++ .../contentstore/rest_api/v4/views/home.py | 5 ++++ .../core/djangoapps/enrollments/v2/views.py | 28 +++++++++++++++---- 6 files changed, 58 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py index 025f2c4c4ca9..d00b37c10a2d 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py @@ -8,6 +8,13 @@ * ADR 0026 - explicit authentication_classes + permission_classes * ADR 0028 - consolidated into XblockViewSet via DefaultRouter * ADR 0029 - standardized error envelope via StandardizedErrorMixin + * ADR 0034 - already compliant. ``authentication_classes`` is + ``(JwtAuthentication, SessionAuthenticationAllowInactiveUser)`` — no + ``BearerAuthentication`` / ``OAuth2Authentication`` to remove. This view + is set explicitly (rather than relying on platform defaults) because it + needs ``SessionAuthenticationAllowInactiveUser`` instead of the default + ``SessionAuthentication`` so inactive Studio authors can still hit the + endpoint while their session is being verified. * ADR 0036 - minimal/flattened views. ``retrieve`` accepts a ``?view=minimal`` query parameter that strips the (tree-shaped) xblock response to a small set of structural fields. The full xblock response is kept as the default diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py b/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py index 72a4039c9ff3..e92272e6f8bd 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py @@ -30,6 +30,13 @@ real-world payload is always small. A hard cap is enforced upstream of this endpoint by :func:`CourseGradingModel.update_from_json`; we surface that as a documentation note rather than re-implement the bound here. + * ADR 0034 – auth standardization (OEP-0042). + ``authentication_classes`` is ``(JwtAuthentication, SessionAuthenticationAllowInactiveUser)``; + ``BearerAuthenticationAllowInactiveUser`` has been removed per the + deprecation policy. ``SessionAuthenticationAllowInactiveUser`` is + retained (rather than relying on the platform-default + ``SessionAuthentication``) so Studio authors whose accounts are + temporarily inactive can still update grading. Permission model note: PR #38363 proposed a class-level ``HasStudioReadAccess`` permission. The @@ -63,7 +70,6 @@ from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission from openedx.core.djangoapps.authz.decorators import user_has_course_permission from openedx.core.djangoapps.credit.tasks import update_credit_course_requirements -from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.mixins import StandardizedErrorMixin _COURSE_KEY_PARAMETER = OpenApiParameter( @@ -87,9 +93,12 @@ class AuthoringGradingViewSet(StandardizedErrorMixin, viewsets.ViewSet): Supersedes ``AuthoringGradingView`` at ``POST /api/contentstore/v0/grading/{course_id}``. """ + # ADR 0034 — JWT + session-with-inactive-user (BearerAuthenticationAllowInactiveUser + # removed per OEP-0042). SessionAuthenticationAllowInactiveUser is retained + # (instead of relying on the platform-default SessionAuthentication) so Studio + # authors whose accounts are temporarily inactive can still update grading. authentication_classes = ( JwtAuthentication, - BearerAuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser, ) permission_classes = (IsAuthenticated,) diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py b/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py index 79a1f0ed1cba..ae31a1f7d0e1 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/course_details.py @@ -12,6 +12,12 @@ * ADR 0029 – standardized error envelope via :class:`StandardizedErrorMixin` (v3-scoped — does not change the project-wide DRF ``EXCEPTION_HANDLER`` setting) + * ADR 0034 – already compliant. ``authentication_classes`` is + ``(JwtAuthentication, SessionAuthenticationAllowInactiveUser)`` — no + ``BearerAuthentication`` / ``OAuth2Authentication`` to remove. + Explicit declaration kept (rather than relying on platform defaults) + so that ``SessionAuthenticationAllowInactiveUser`` is used instead of + the default ``SessionAuthentication``. Permission model note: PR #38365 proposed a class-level ``HasStudioReadAccess`` permission. The diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/home.py b/cms/djangoapps/contentstore/rest_api/v3/views/home.py index de5158c17e58..fcf09a0e617f 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/home.py @@ -18,6 +18,12 @@ explicitly. The flat-list ``courses`` and ``libraries`` actions are out of scope (single-key dict around a list) and do not honour ``?fields=``. + * ADR 0034 – already compliant. ``authentication_classes`` is + ``(JwtAuthentication, SessionAuthenticationAllowInactiveUser)`` — + no ``BearerAuthentication`` / ``OAuth2Authentication`` to remove. + The explicit declaration is kept (rather than relying on platform + defaults) so that ``SessionAuthenticationAllowInactiveUser`` is used + instead of the default ``SessionAuthentication``. """ import edx_api_doc_tools as apidocs diff --git a/cms/djangoapps/contentstore/rest_api/v4/views/home.py b/cms/djangoapps/contentstore/rest_api/v4/views/home.py index 28fab47b55c0..7617cd6bd850 100644 --- a/cms/djangoapps/contentstore/rest_api/v4/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v4/views/home.py @@ -128,6 +128,11 @@ class HomeCoursesViewSet(StandardizedErrorMixin, viewsets.ViewSet): a dynamic-fields serializer mixin and per-field schema documentation) but is intentionally NOT added here to keep the v4 contract stable for the existing Studio frontend. + - 0034: already compliant. ``authentication_classes`` is + ``(JwtAuthentication, SessionAuthenticationAllowInactiveUser)`` — no + ``BearerAuthentication`` / ``OAuth2Authentication`` to remove. + Explicit declaration kept so ``SessionAuthenticationAllowInactiveUser`` + is used instead of the default ``SessionAuthentication``. """ authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) diff --git a/openedx/core/djangoapps/enrollments/v2/views.py b/openedx/core/djangoapps/enrollments/v2/views.py index 905452f1d32c..67af3006a025 100644 --- a/openedx/core/djangoapps/enrollments/v2/views.py +++ b/openedx/core/djangoapps/enrollments/v2/views.py @@ -6,6 +6,13 @@ * ADR 0025 – ``serializer_class`` on every viewset/view * ADR 0026 – explicit ``authentication_classes`` + ``permission_classes`` + * ADR 0034 – auth standardization (OEP-0042). All four v2 viewsets/views use + ``(JwtAuthentication, EnrollmentCrossDomainSessionAuth)``; + ``BearerAuthenticationAllowInactiveUser`` has been removed per the + deprecation policy. ``EnrollmentCrossDomainSessionAuth`` is retained + (rather than relying on the platform-default ``SessionAuthentication``) + because these endpoints must accept cross-domain Studio/LMS CSRF-validated + session cookies. * ADR 0027 – ``drf_spectacular`` for OpenAPI schema generation * ADR 0028 – consolidated into ``ViewSet`` classes registered via ``DefaultRouter`` where the URL shape allows it @@ -70,7 +77,6 @@ EnrollmentUserThrottle, ) from openedx.core.djangoapps.user_api.accounts.permissions import CanRetireUser -from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.mixins import StandardizedErrorMixin from openedx.core.lib.api.permissions import ApiKeyHeaderPermissionIsAuthenticated @@ -209,9 +215,12 @@ class EnrollmentViewSet(StandardizedErrorMixin, viewsets.ViewSet, ApiKeyPermissi DELETE /api/enrollment/v2/enrollment/enrollment_allowed/ → allowed (DELETE) """ + # ADR 0034 — JWT + cross-domain session (BearerAuthenticationAllowInactiveUser + # removed per OEP-0042). EnrollmentCrossDomainSessionAuth retained because the + # endpoint must accept cross-domain Studio/LMS CSRF-validated session cookies; + # the platform-default SessionAuthentication would reject those. authentication_classes = ( JwtAuthentication, - BearerAuthenticationAllowInactiveUser, EnrollmentCrossDomainSessionAuth, ) permission_classes = (ApiKeyHeaderPermissionIsAuthenticated,) @@ -417,9 +426,12 @@ def allowed(self, request): class EnrollmentRetrieveView(StandardizedErrorMixin, ApiKeyPermissionMixIn, APIView): """GET enrollment for a course (and optionally a named user).""" + # ADR 0034 — JWT + cross-domain session (BearerAuthenticationAllowInactiveUser + # removed per OEP-0042). EnrollmentCrossDomainSessionAuth retained because the + # endpoint must accept cross-domain Studio/LMS CSRF-validated session cookies; + # the platform-default SessionAuthentication would reject those. authentication_classes = ( JwtAuthentication, - BearerAuthenticationAllowInactiveUser, EnrollmentCrossDomainSessionAuth, ) permission_classes = (ApiKeyHeaderPermissionIsAuthenticated,) @@ -492,9 +504,12 @@ def get(self, request, course_id=None, username=None): class UserRolesView(StandardizedErrorMixin, APIView): """List the current user's course-level roles.""" + # ADR 0034 — JWT + cross-domain session (BearerAuthenticationAllowInactiveUser + # removed per OEP-0042). EnrollmentCrossDomainSessionAuth retained because the + # endpoint must accept cross-domain Studio/LMS CSRF-validated session cookies; + # the platform-default SessionAuthentication would reject those. authentication_classes = ( JwtAuthentication, - BearerAuthenticationAllowInactiveUser, EnrollmentCrossDomainSessionAuth, ) permission_classes = (ApiKeyHeaderPermissionIsAuthenticated,) @@ -644,9 +659,12 @@ def get(self, request, course_id=None): class EnrollmentsAdminListView(StandardizedErrorMixin, ListAPIView): """Admin-only paginated enrollment list with OEP-68 filter aliases.""" + # ADR 0034 — JWT + cross-domain session (BearerAuthenticationAllowInactiveUser + # removed per OEP-0042). EnrollmentCrossDomainSessionAuth retained because the + # endpoint must accept cross-domain Studio/LMS CSRF-validated session cookies; + # the platform-default SessionAuthentication would reject those. authentication_classes = ( JwtAuthentication, - BearerAuthenticationAllowInactiveUser, EnrollmentCrossDomainSessionAuth, ) permission_classes = (permissions.IsAdminUser,) From c4d0b7a44783089250b09bd8f694fdc83ff6ed5c Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 23 Jun 2026 16:12:59 +0500 Subject: [PATCH 11/13] feat(adr-0040): designate FrontendSiteConfigView as canonical MFE config endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per ADR 0040 (Canonical MFE Configuration Endpoint), ``/api/frontend_site_config/v1/`` (``FrontendSiteConfigView``) is the canonical endpoint for MFE / front-end runtime configuration, aligned with frontend-base's ``SiteConfig`` format under OEP-65. New consumers MUST target it; the sibling ``/api/mfe_config/v1`` (``MFEConfigView``, ADR 0001) is legacy and on the OEP-21 DEPR path (#37255, targeting Verawood 2026-04). Docstring updates: * Restated as "Canonical MFE / front-end runtime configuration endpoint" in the class summary. * Added an explicit ADR 0040 section with three decisions: canonical designation, no-per-user-data rule, no-new-config-endpoints rule. * Linked the ADR file path so the relationship is discoverable from the source. The no-per-user-data rule is the most actionable piece going forward: this view declares ``permission_classes = [AllowAny]`` and is wrapped in ``cache_page``, so any per-user field added to this payload would leak across users via the shared cache. User roles, enrollments, and course-specific permissions are first-class resources, not configuration. No code-behaviour change — docstring annotations only. --- lms/djangoapps/mfe_config_api/views.py | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/mfe_config_api/views.py b/lms/djangoapps/mfe_config_api/views.py index 836cac4a09f8..e6e8e6558316 100644 --- a/lms/djangoapps/mfe_config_api/views.py +++ b/lms/djangoapps/mfe_config_api/views.py @@ -402,7 +402,8 @@ def translate_legacy_mfe_config() -> dict: class FrontendSiteConfigView(APIView): """ - Provides the frontend site configuration endpoint. + Canonical MFE / front-end runtime configuration endpoint + (``GET /api/frontend_site_config/v1/``). Returns the contents of ``FRONTEND_SITE_CONFIG`` merged on top of a compatibility translation of the legacy ``MFE_CONFIG`` / @@ -412,6 +413,25 @@ class FrontendSiteConfigView(APIView): See `frontend-base SiteConfig `_. + + ADR 0040 (Canonical MFE Configuration Endpoint): + * This is **the** canonical endpoint for MFE / front-end runtime + configuration, aligned with frontend-base's ``SiteConfig`` format + under OEP-65. New consumers MUST target it. The sibling + :class:`MFEConfigView` (``/api/mfe_config/v1``) is legacy and on + the OEP-21 DEPR path (#37255, targeting Verawood 2026-04). + * **User-contextual data MUST NOT be served from this endpoint.** + User roles, enrollments, and course-specific permissions are + first-class resources, not configuration. This view is + ``AllowAny`` + ``cache_page``-decorated — i.e. unauthenticated and + shared-cached — so per-user data on this surface would be both + unsafe (leaks across users) and semantically wrong. Per-user data + belongs at resource-oriented endpoints. + * **No new configuration endpoints.** If configuration needs grow, + extend this endpoint via URL-path versioning (e.g. ``/v2/``) + rather than adding parallel surfaces. + + See ``docs/decisions/0040-canonical-mfe-configuration-endpoint.rst``. """ authentication_classes = [] From 1707060c918221e523796fa79895cc94f45e60a6 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 23 Jun 2026 16:14:46 +0500 Subject: [PATCH 12/13] feat(adr-0040): mark MFEConfigView legacy + point at canonical successor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0040 designates ``/api/frontend_site_config/v1/`` (``FrontendSiteConfigView``) as the canonical MFE configuration endpoint and partially supersedes ADR 0001 for that role. Updated ``MFEConfigView``'s docstring to: * Restate it as the "Legacy" endpoint in the class summary (was "Provides an API endpoint to get the MFE configuration ...", which didn't carry any deprecation signal at the summary level). * Add an explicit ``.. deprecated::`` block pointing at ``FrontendSiteConfigView`` as the replacement. * Reference the primary DEPR ticket #37255 (the current docstring mentioned only the cross-linked #37210) and the Verawood (2026-04) target release. * Carry the same no-per-user-data note as the canonical endpoint: ``AllowAny`` + ``cache_page`` makes per-user fields unsafe via the shared cache. * Link to the ADR 0040 file path so the relationship is discoverable from the source. No code-behaviour change — docstring annotations only. The DEPR ticket (#37255) remains the authoritative removal tracker; this commit just makes the relationship visible from the legacy view itself. --- lms/djangoapps/mfe_config_api/views.py | 30 +++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/mfe_config_api/views.py b/lms/djangoapps/mfe_config_api/views.py index e6e8e6558316..e154efb15e4c 100644 --- a/lms/djangoapps/mfe_config_api/views.py +++ b/lms/djangoapps/mfe_config_api/views.py @@ -245,7 +245,35 @@ def get_frontend_site_config() -> dict: class MFEConfigView(APIView): """ - Provides an API endpoint to get the MFE configuration from settings (or site configuration). + Legacy MFE configuration endpoint (``GET /api/mfe_config/v1``). + + Returns the legacy ``SCREAMING_SNAKE_CASE`` ``MFE_CONFIG`` shape from + settings (or site configuration). + + .. deprecated:: + Use :class:`FrontendSiteConfigView` (``/api/frontend_site_config/v1/``) + instead — that endpoint is canonical per ADR 0040 and returns + ``FRONTEND_SITE_CONFIG`` in frontend-base's camelCase ``SiteConfig`` + format (OEP-65). + + ADR 0040 (Canonical MFE Configuration Endpoint) **partially supersedes + ADR 0001** specifically for the role of "the configuration endpoint + of record" — the runtime-vs-build rationale, no-auth posture, and + cache behaviour from ADR 0001 still apply to the canonical endpoint. + + Removal is tracked under OEP-21 DEPR in + `#37255 `_ + (primary) and + `#37210 `_ + (referenced), targeting the **Verawood (2026-04)** named release. + + Per ADR 0040, user-contextual data (roles, enrollments, permissions) + MUST NOT be served from this endpoint either: this view is ``AllowAny`` + + ``cache_page``-decorated, so any per-user field on the payload would leak + across users via the shared cache. Such data belongs at resource-oriented + endpoints. + + See ``docs/decisions/0040-canonical-mfe-configuration-endpoint.rst``. """ @method_decorator(cache_page(settings.MFE_CONFIG_API_CACHE_TIMEOUT)) From f53b4d43db1da1aa0c0d07ddf75d6a791832ac23 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 23 Jun 2026 16:16:01 +0500 Subject: [PATCH 13/13] docs(adr-0040): mark ADR 0001 as partially superseded by ADR 0040 ADR 0040 (Canonical MFE Configuration Endpoint) explicitly instructs: > Per ADR convention, 0001 carries a one-line "Superseded (partially) > by ADR 0040" status note pointing here, so the relationship is > discoverable from either direction. Updated ADR 0001's Status section accordingly with a relative link to ADR 0040 and a short summary of which parts of 0001 are superseded (the endpoint-of-record role) vs. which remain in effect (runtime-vs- build rationale, no-auth posture, cache behaviour). Also referenced the OEP-21 DEPR ticket #37255 and the Verawood (2026-04) target release for ``/api/mfe_config/v1`` removal. No code change. Pure documentation cross-linking so a reader of ADR 0001 lands on ADR 0040 (the canonical reference for the configuration-endpoint topology going forward). --- .../docs/decisions/0001-mfe-config-api.rst | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/mfe_config_api/docs/decisions/0001-mfe-config-api.rst b/lms/djangoapps/mfe_config_api/docs/decisions/0001-mfe-config-api.rst index 8e298991186d..3da384e2a0f4 100644 --- a/lms/djangoapps/mfe_config_api/docs/decisions/0001-mfe-config-api.rst +++ b/lms/djangoapps/mfe_config_api/docs/decisions/0001-mfe-config-api.rst @@ -4,7 +4,14 @@ Status ****** -Accepted +Accepted — Superseded (partially) by `ADR 0040 — Canonical MFE Configuration +Endpoint <../../../../../docs/decisions/0040-canonical-mfe-configuration-endpoint.rst>`_ +for the role of "the configuration endpoint of record". The +runtime-vs-build-time rationale, no-authentication posture, and response-cache +behaviour from this ADR still apply to the canonical endpoint +(``/api/frontend_site_config/v1/``). Only ``/api/mfe_config/v1`` itself +(the endpoint URL / payload shape established here) is deprecated, tracked +under OEP-21 DEPR #37255 (Verawood 2026-04). Context *******