From 03044fe39208a573821ba7b1158d3ece79e69031 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Thu, 4 Jun 2026 12:54:36 +0500 Subject: [PATCH] feat: Update course detail api version with ADR standardization --- .../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