diff --git a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py new file mode 100644 index 000000000000..2b085422cc06 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -0,0 +1,28 @@ +"""Authoring API v1 URLs.""" + +from django.urls import path + +from cms.djangoapps.contentstore.rest_api.v1.views import XblockViewSet + +app_name = "authoring_v1" + +urlpatterns = [ + # The viewset has no ``list`` action, so the collection accepts POST only. + path( + "xblocks/", + XblockViewSet.as_view({"post": "create"}), + name="xblock_list", + ), + path( + "xblocks//", + XblockViewSet.as_view( + { + "get": "retrieve", + "put": "update", + "patch": "partial_update", + "delete": "destroy", + } + ), + name="xblock_detail", + ), +] 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 638b0ce2eb35..2f69d29b49d7 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 @@ -10,7 +10,7 @@ from unittest.mock import patch from django.http import JsonResponse -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APITestCase @@ -216,3 +216,66 @@ def test_minimal_view_is_noop_for_non_json_payload(self, mock_retrieve): response = self.client.get(_detail_url(), {"view": "minimal", "fields": "graderType"}) assert response.status_code == status.HTTP_200_OK assert response.json() == "notgraded" + + +# --------------------------------------------------------------------------- +# URL-structure tests +# --------------------------------------------------------------------------- + + +def _authoring_list_url(): + return reverse("authoring_v1:xblock_list") + + +def _authoring_detail_url(): + return reverse( + "authoring_v1:xblock_detail", + kwargs={"usage_key_string": TEST_LOCATOR}, + ) + + +class XblockViewSetUrlStructureTest(ModuleStoreTestCase, APITestCase): + """The conforming /api/authoring/v1/xblocks/ routes serve the same view as the legacy ones.""" + + def setUp(self): + super().setUp() + self.staff = GlobalStaffFactory(password='password') + self.client.force_authenticate(user=self.staff) + + def test_conforming_urls_reverse_to_expected_paths(self): + assert _authoring_list_url() == "/api/authoring/v1/xblocks/" + assert _authoring_detail_url() == f"/api/authoring/v1/xblocks/{TEST_LOCATOR}/" + + def test_conforming_and_legacy_routes_share_view(self): + legacy_cls = resolve(_detail_url()).func.cls + conforming_cls = resolve(_authoring_detail_url()).func.cls + assert conforming_cls is legacy_cls + + def test_invalid_usage_key_is_404_on_conforming_route(self): + # The shared usage_key converter rejects unparseable keys with a + # routing-level 404. + response = self.client.get("/api/authoring/v1/xblocks/not-a-usage-key/") + assert response.status_code == status.HTTP_404_NOT_FOUND + + @patch(f"{_VIEW_MODULE}.retrieve_xblock_response", return_value=_MOCK_RESPONSE) + def test_get_on_conforming_route_calls_retrieve(self, mock_fn): + # Also exercises the UsageKey→str coercion in XblockViewSet.initial(). + response = self.client.get(_authoring_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}.create_xblock_response", return_value=_MOCK_RESPONSE) + def test_post_on_conforming_route_calls_create(self, mock_fn): + data = {"parent_locator": PARENT_LOCATOR, "category": "html"} + response = self.client.post(_authoring_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}.delete_xblock_response", return_value=_MOCK_RESPONSE) + def test_delete_on_conforming_route_calls_destroy(self, mock_fn): + response = self.client.delete(_authoring_detail_url()) + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "DELETE" diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py index 2d8d94ceb1de..02c0434ced2a 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py @@ -198,7 +198,12 @@ def initial(self, request, *args, **kwargs): 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") + # The conforming route passes a parsed UsageKey; the legacy route + # passes the raw string. Coerce to the string the actions expect — + # ``self.kwargs`` is the dict ``dispatch()`` unpacks into the handler. + if isinstance(self.kwargs.get("usage_key_string"), UsageKey): + self.kwargs["usage_key_string"] = str(self.kwargs["usage_key_string"]) + usage_key_string = self.kwargs.get("usage_key_string") if usage_key_string: try: self.course_key = UsageKey.from_string(usage_key_string).course_key diff --git a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py new file mode 100644 index 000000000000..2d1db8f88434 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py @@ -0,0 +1,35 @@ +"""Authoring API v3 URLs.""" + +from django.urls import path + +from cms.djangoapps.contentstore.rest_api.v3.views import AuthoringGradingViewSet, CourseDetailsViewSet, HomeViewSet + +app_name = "authoring_v3" + +urlpatterns = [ + path( + "home/", + HomeViewSet.as_view({"get": "list"}), + name="home", + ), + path( + "home/courses/", + HomeViewSet.as_view({"get": "courses"}), + name="home_courses", + ), + path( + "home/libraries/", + HomeViewSet.as_view({"get": "libraries"}), + name="home_libraries", + ), + path( + "courses//details/", + CourseDetailsViewSet.as_view({"get": "retrieve", "put": "update"}), + name="course_details", + ), + path( + "courses//grading/", + AuthoringGradingViewSet.as_view({"patch": "partial_update"}), + name="course_grading", + ), +] 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 f2b4a4744d65..4e6c83e5a0c5 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py @@ -10,7 +10,7 @@ ``EXCEPTION_HANDLER`` setting is unchanged, so v0/v1/v2 endpoints continue to return the legacy error shape. """ -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APIClient, APITestCase @@ -84,3 +84,30 @@ def test_v1_endpoint_unaffected_by_v3_envelope(self): # v1 still uses the project-default handler → ADR 0029 fields absent. assert "type" not in response.data assert "instance" not in response.data + + +class TestHomeViewSetUrlStructure(APITestCase): + """The conforming /api/authoring/v3/home/ routes serve the same view as the legacy ones.""" + + def test_conforming_urls_reverse_to_expected_paths(self): + assert reverse("authoring_v3:home") == "/api/authoring/v3/home/" + assert reverse("authoring_v3:home_courses") == "/api/authoring/v3/home/courses/" + assert reverse("authoring_v3:home_libraries") == "/api/authoring/v3/home/libraries/" + + def test_conforming_and_legacy_routes_share_view(self): + pairs = ( + ("cms.djangoapps.contentstore:v3:home-list", "authoring_v3:home"), + ("cms.djangoapps.contentstore:v3:home-courses", "authoring_v3:home_courses"), + ("cms.djangoapps.contentstore:v3:home-libraries", "authoring_v3:home_libraries"), + ) + for legacy_name, conforming_name in pairs: + legacy_cls = resolve(reverse(legacy_name)).func.cls + conforming_cls = resolve(reverse(conforming_name)).func.cls + assert conforming_cls is legacy_cls, f"{conforming_name} must serve the same view as {legacy_name}" + + def test_unauthenticated_conforming_route_returns_standardized_401(self): + """The conforming mount carries the same contract, error envelope included.""" + response = APIClient().get(reverse("authoring_v3:home")) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"missing error-envelope field '{field}'" diff --git a/cms/djangoapps/contentstore/rest_api/v3/utils.py b/cms/djangoapps/contentstore/rest_api/v3/utils.py index 79524acb8c53..bbc3ef235e64 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/utils.py +++ b/cms/djangoapps/contentstore/rest_api/v3/utils.py @@ -27,10 +27,15 @@ from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -def resolve_course_key(course_key: str) -> CourseKey: +def resolve_course_key(course_key: str | CourseKey) -> CourseKey: """ - Parse ``course_key`` (string) into a :class:`CourseKey` and verify the - course exists. + Parse ``course_key`` into a :class:`CourseKey` and verify the course + exists. + + Accepts either the raw string (the legacy ``/api/contentstore/v3/`` + routes) or an already-parsed :class:`CourseKey` (the conforming + ``/api/authoring/v3/`` routes, whose ``course_key`` path converter + hands views a parsed key). Raises: rest_framework.exceptions.NotFound: if the string is unparseable @@ -44,7 +49,7 @@ def resolve_course_key(course_key: str) -> CourseKey: positional argument. """ try: - parsed = CourseKey.from_string(course_key) + parsed = course_key if isinstance(course_key, CourseKey) else 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): 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 index 33f9efd69ff4..c81cd2641eb3 100644 --- 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 @@ -18,7 +18,7 @@ from unittest.mock import patch from django.test import TestCase -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APIClient, APITestCase @@ -291,3 +291,58 @@ def test_v0_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 + + +class TestAuthoringGradingViewSetUrlStructure(APITestCase): + """The conforming courses/{course_key}/grading/ route serves the same view as the legacy one.""" + + def setUp(self): + super().setUp() + self.client = APIClient() + self.conforming_url = reverse( + "authoring_v3:course_grading", + kwargs={"course_key": COURSE_ID}, + ) + self.legacy_url = reverse( + "cms.djangoapps.contentstore:v3:authoring_grading-detail", + kwargs={"course_key": COURSE_ID}, + ) + + def test_conforming_url_reverses_to_expected_path(self): + assert self.conforming_url == f"/api/authoring/v3/courses/{COURSE_ID}/grading/" + + def test_conforming_and_legacy_routes_share_view(self): + legacy_cls = resolve(self.legacy_url).func.cls + conforming_cls = resolve(self.conforming_url).func.cls + assert conforming_cls is legacy_cls + + def test_invalid_course_key_is_404_on_conforming_route(self): + # The shared course_key converter rejects unparseable keys with a + # routing-level 404. + response = self.client.patch( + "/api/authoring/v3/courses/not-a-course-key/grading/", + data={}, format="json", + ) + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_unauthenticated_patch_returns_401(self): + response = self.client.patch(self.conforming_url, data={}, format="json") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + @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_on_conforming_route_updates_grading( + self, mock_exists, mock_perm, mock_update, mock_credit, # noqa: ARG002 + ): + """Same contract on the conforming mount as on the legacy one.""" + user = UserFactory.create() + self.client.force_authenticate(user=user) + response = self.client.patch( + self.conforming_url, + data={"graders": _GRADERS_PAYLOAD}, + format="json", + ) + assert response.status_code == status.HTTP_200_OK + mock_update.assert_called_once() 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 e25df0188d18..b97a4bf505fa 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 @@ -19,7 +19,7 @@ """ from unittest.mock import MagicMock, patch -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APIClient, APITestCase @@ -378,3 +378,51 @@ def test_fields_csv_restricts_top_level_keys( assert response.status_code == status.HTTP_200_OK assert set(response.data.keys()) == {"course_id", "title"} + + +# --------------------------------------------------------------------------- +# URL-structure tests +# --------------------------------------------------------------------------- +class TestCourseDetailsViewSetUrlStructure(APITestCase): + """The conforming courses/{course_key}/details/ route serves the same view as the legacy one.""" + + def _conforming_url(self): + return reverse( + "authoring_v3:course_details", + kwargs={"course_id": TEST_COURSE_ID}, + ) + + def _legacy_url(self): + return reverse( + "cms.djangoapps.contentstore:v3:course_details-detail", + kwargs={"course_id": TEST_COURSE_ID}, + ) + + def test_conforming_url_reverses_to_expected_path(self): + assert self._conforming_url() == ( + f"/api/authoring/v3/courses/{TEST_COURSE_ID}/details/" + ) + + def test_conforming_and_legacy_routes_share_view(self): + legacy_cls = resolve(self._legacy_url()).func.cls + conforming_cls = resolve(self._conforming_url()).func.cls + assert conforming_cls is legacy_cls + + def test_invalid_course_key_is_404_on_conforming_route(self): + # The shared course_key converter rejects unparseable keys with a + # routing-level 404. + response = self.client.get("/api/authoring/v3/courses/not-a-course-key/details/") + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_unauthenticated_get_returns_401(self): + response = self.client.get(self._conforming_url()) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + @patch(MOCK_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 + """The conforming mount enforces the same authorization as the legacy one.""" + user = UserFactory.create() + self.client.force_authenticate(user=user) + response = self.client.get(self._conforming_url()) + assert response.status_code == status.HTTP_403_FORBIDDEN diff --git a/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py new file mode 100644 index 000000000000..0f8c8b3641b8 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py @@ -0,0 +1,15 @@ +"""Authoring API v4 URLs.""" + +from django.urls import path + +from cms.djangoapps.contentstore.rest_api.v4.views import home + +app_name = "authoring_v4" + +urlpatterns = [ + path( + "courses/", + home.HomeCoursesViewSet.as_view({"get": "list"}), + name="course_list", + ), +] 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 index 4b6b89c92d6c..f9868c2788d5 100644 --- a/cms/djangoapps/contentstore/rest_api/v4/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v4/views/tests/test_home.py @@ -8,7 +8,7 @@ import ddt from django.conf import settings -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APIClient, APITestCase @@ -277,3 +277,34 @@ def test_no_ordering_param_no_deprecation_header(self): response = self.client.get(self.list_url) self.assertNotIn("Deprecation", response) # noqa: PT009 + + +class TestHomeCoursesViewSetUrlStructure(APITestCase): + """The conforming /api/authoring/v4/courses/ route serves the same view as the legacy one.""" + + def test_conforming_url_reverses_to_expected_path(self): + assert reverse("authoring_v4:course_list") == "/api/authoring/v4/courses/" + + def test_conforming_and_legacy_routes_share_view(self): + legacy_cls = resolve( + reverse("cms.djangoapps.contentstore:v4:home-courses-list") + ).func.cls + conforming_cls = resolve(reverse("authoring_v4:course_list")).func.cls + assert conforming_cls is legacy_cls + + def test_unauthenticated_returns_401(self): + response = APIClient().get(reverse("authoring_v4:course_list")) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) # noqa: PT009 + + def test_authenticated_staff_gets_200(self): + """Same contract on the conforming mount as on the legacy one.""" + from django.contrib.auth import get_user_model + + User = get_user_model() + user = User.objects.create_user( + username="teststaff-authoring", password="pass", is_staff=True + ) + self.client.force_authenticate(user=user) + with patch(_MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get(reverse("authoring_v4:course_list")) + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index 0576a35272af..6db9599d0c79 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -356,12 +356,18 @@ def should_show_debug_toolbar(request): # pylint: disable=missing-function-docs 'SERVE_INCLUDE_SCHEMA': False, # restrict spectacular to CMS API endpoints (cms/lib/spectacular.py): 'PREPROCESSING_HOOKS': ['cms.lib.spectacular.cms_api_filter'], - # remove the default schema path prefix to replace it with server-specific base paths: - 'SCHEMA_PATH_PREFIX': '/api/contentstore', - 'SCHEMA_PATH_PREFIX_TRIM': '/api/contentstore', + # Mark migrated legacy addresses deprecated and BFF surfaces x-internal. + # The enum hook is drf-spectacular's default, restated because setting + # this key replaces the default list. + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'cms.lib.spectacular.cms_mark_migrated_paths', + ], + # Used for tag extraction only. Paths are emitted in full so they resolve + # against the service-root SERVERS below. + 'SCHEMA_PATH_PREFIX': r'/api/(contentstore|authoring)', 'SERVERS': [ {'url': AUTHORING_API_URL, 'description': 'Public'}, # noqa: F405 {'url': f'http://{CMS_BASE}', 'description': 'Local'}, - {'url': f'http://{CMS_BASE}/api/contentstore', 'description': 'CMS-contentstore'} ], } diff --git a/cms/envs/production.py b/cms/envs/production.py index 604d2753bccd..a3e9edd27106 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -416,13 +416,19 @@ def get_env_setting(setting): 'SERVE_INCLUDE_SCHEMA': False, # restrict spectacular to CMS API endpoints (cms/lib/spectacular.py): 'PREPROCESSING_HOOKS': ['cms.lib.spectacular.cms_api_filter'], - # remove the default schema path prefix to replace it with server-specific base paths: - 'SCHEMA_PATH_PREFIX': '/api/contentstore', - 'SCHEMA_PATH_PREFIX_TRIM': '/api/contentstore', + # Mark migrated legacy addresses deprecated and BFF surfaces x-internal. + # The enum hook is drf-spectacular's default, restated because setting + # this key replaces the default list. + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'cms.lib.spectacular.cms_mark_migrated_paths', + ], + # Used for tag extraction only. Paths are emitted in full so they resolve + # against the service-root SERVERS below. + 'SCHEMA_PATH_PREFIX': r'/api/(contentstore|authoring)', 'SERVERS': [ {'url': AUTHORING_API_URL, 'description': 'Public'}, # noqa: F405 {'url': f'https://{CMS_BASE}', 'description': 'Local'}, # noqa: F405 - {'url': f'https://{CMS_BASE}/api/contentstore', 'description': 'CMS-contentstore'} # noqa: F405 ], } diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 90bce5668fec..20103add181c 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -2,14 +2,31 @@ import re +# Legacy addresses of APIs migrated to /api/authoring/, marked deprecated for +# their deprecation window. +LEGACY_MIGRATED_PATH_PREFIXES = ( + "/api/contentstore/v1/xblock/", # → /api/authoring/v1/xblocks/ + "/api/contentstore/v3/home/", # → /api/authoring/v3/home/ + "/api/contentstore/v3/course_details/", # → /api/authoring/v3/courses/{course_key}/details/ + "/api/contentstore/v3/authoring_grading/", # → /api/authoring/v3/courses/{course_key}/grading/ + "/api/contentstore/v4/home/courses/", # → /api/authoring/v4/courses/ +) + +# BFF surfaces, marked x-internal so clients can tell them from a stable +# resource contract. Both the legacy and conforming mounts. +INTERNAL_BFF_PATH_PREFIXES = ( + "/api/contentstore/v3/home/", + "/api/authoring/v3/home/", +) + def cms_api_filter(endpoints): """ - Pre-processing hook: keep only contentstore versioned endpoints and select - course-level endpoints. + Pre-processing hook: keep only contentstore + authoring versioned + endpoints and select course-level endpoints. """ filtered = [] - CMS_PATH_PATTERN = re.compile(r"^/api/contentstore/v\d+/") + CMS_PATH_PATTERN = re.compile(r"^/api/(contentstore|authoring)/v\d+/") for path, path_regex, method, callback in endpoints: if ( @@ -22,3 +39,23 @@ def cms_api_filter(endpoints): filtered.append((path, path_regex, method, callback)) return filtered + + +def cms_mark_migrated_paths(result, generator, request, public): # pylint: disable=unused-argument + """ + Post-processing hook: mark the legacy addresses of + migrated APIs ``deprecated: true`` and BFF surfaces ``x-internal``. + """ + for path, path_item in result.get("paths", {}).items(): + legacy = path.startswith(LEGACY_MIGRATED_PATH_PREFIXES) + internal = path.startswith(INTERNAL_BFF_PATH_PREFIXES) + if not (legacy or internal): + continue + for operation in path_item.values(): + if not isinstance(operation, dict): + continue + if legacy: + operation["deprecated"] = True + if internal: + operation["x-internal"] = True + return result diff --git a/cms/lib/tests/__init__.py b/cms/lib/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/lib/tests/test_spectacular.py b/cms/lib/tests/test_spectacular.py new file mode 100644 index 000000000000..6980de3a5704 --- /dev/null +++ b/cms/lib/tests/test_spectacular.py @@ -0,0 +1,66 @@ +"""Tests for the CMS drf-spectacular hooks.""" + +from django.test import SimpleTestCase + +from cms.lib.spectacular import cms_api_filter, cms_mark_migrated_paths + + +def _endpoint(path): + return (path, path, "GET", None) + + +def _schema(*paths): + return {"paths": {path: {"get": {"operationId": path}} for path in paths}} + + +class CmsApiFilterTest(SimpleTestCase): + """The pre-processing hook keeps both the legacy and the conforming prefixes.""" + + def test_keeps_contentstore_and_authoring_drops_the_rest(self): + kept = cms_api_filter([ + _endpoint("/api/contentstore/v1/xblock/{usage_key_string}/"), + _endpoint("/api/authoring/v1/xblocks/{usage_key_string}/"), + _endpoint("/api/courses/v1/course-key/bulk_enable_disable_discussions"), + _endpoint("/api/user/v1/accounts/"), + _endpoint("/heartbeat"), + ]) + assert [path for path, *_ in kept] == [ + "/api/contentstore/v1/xblock/{usage_key_string}/", + "/api/authoring/v1/xblocks/{usage_key_string}/", + "/api/courses/v1/course-key/bulk_enable_disable_discussions", + ] + + +class CmsMarkMigratedPathsTest(SimpleTestCase): + """The post-processing hook works on full paths, so both prefixes coexist in one schema.""" + + def test_legacy_paths_deprecated_conforming_paths_not(self): + result = cms_mark_migrated_paths(_schema( + "/api/contentstore/v1/xblock/{usage_key_string}/", + "/api/authoring/v1/xblocks/{usage_key_string}/", + "/api/contentstore/v3/course_details/{course_id}/", + "/api/authoring/v3/courses/{course_key}/details/", + ), None, None, False) + paths = result["paths"] + assert paths["/api/contentstore/v1/xblock/{usage_key_string}/"]["get"]["deprecated"] is True + assert paths["/api/contentstore/v3/course_details/{course_id}/"]["get"]["deprecated"] is True + assert "deprecated" not in paths["/api/authoring/v1/xblocks/{usage_key_string}/"]["get"] + assert "deprecated" not in paths["/api/authoring/v3/courses/{course_key}/details/"]["get"] + + def test_bff_surfaces_internal_on_both_mounts(self): + result = cms_mark_migrated_paths(_schema( + "/api/contentstore/v3/home/", + "/api/authoring/v3/home/", + "/api/authoring/v4/courses/", + ), None, None, False) + paths = result["paths"] + assert paths["/api/contentstore/v3/home/"]["get"]["x-internal"] is True + assert paths["/api/contentstore/v3/home/"]["get"]["deprecated"] is True + assert paths["/api/authoring/v3/home/"]["get"]["x-internal"] is True + assert "deprecated" not in paths["/api/authoring/v3/home/"]["get"] + assert "x-internal" not in paths["/api/authoring/v4/courses/"]["get"] + + def test_paths_are_left_as_full_urls(self): + """Paths are not trimmed, so every key resolves against a service-root server.""" + result = cms_mark_migrated_paths(_schema("/api/contentstore/v1/xblock/"), None, None, False) + assert list(result["paths"]) == ["/api/contentstore/v1/xblock/"] diff --git a/cms/urls.py b/cms/urls.py index c0f96f489bb8..9fa980c977e2 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -13,6 +13,7 @@ from django.views.generic import RedirectView from drf_spectacular.views import SpectacularAPIView, SpectacularSwaggerView from edx_api_doc_tools import make_docs_urls +from edx_rest_framework_extensions.url_converters import register_url_converters import openedx.core.djangoapps.common_views.xblock import openedx.core.djangoapps.debug.views @@ -26,6 +27,9 @@ from openedx.core.djangoapps.password_policy import compliance as password_policy_compliance from openedx.core.djangoapps.password_policy.forms import PasswordPolicyAwareAdminAuthForm +# Shared opaque-key path converters, registered before any pattern using them. +register_url_converters() + django_autodiscover() admin.site.site_header = _('Studio Administration') admin.site.site_title = admin.site.site_header @@ -356,6 +360,14 @@ path('api/contentstore/', include('cms.djangoapps.contentstore.rest_api.urls')) ] +# Authoring REST APIs — conforming addresses, dual-mounted beside their +# legacy /api/contentstore/ routes for the deprecation window. +urlpatterns += [ + path('api/authoring/v1/', include('cms.djangoapps.contentstore.rest_api.v1.authoring_urls')), + path('api/authoring/v3/', include('cms.djangoapps.contentstore.rest_api.v3.authoring_urls')), + path('api/authoring/v4/', include('cms.djangoapps.contentstore.rest_api.v4.authoring_urls')), +] + # Content tagging urlpatterns += [ path('api/content_tagging/', include(('openedx.core.djangoapps.content_tagging.urls', 'content_tagging'))), diff --git a/lms/envs/common.py b/lms/envs/common.py index f7a6f15558cb..69411c261372 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2161,8 +2161,15 @@ 'VERSION': '0.1.0', 'SERVE_INCLUDE_SCHEMA': False, 'PREPROCESSING_HOOKS': ['lms.lib.spectacular.lms_api_filter'], + # Mark legacy slashless enrollment addresses deprecated. The enum hook is + # drf-spectacular's default, restated because setting this key replaces + # the default list. + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'lms.lib.spectacular.lms_mark_legacy_paths_deprecated', + ], + # Used for tag extraction only; paths are emitted in full. 'SCHEMA_PATH_PREFIX': '/api/enrollment', - 'SCHEMA_PATH_PREFIX_TRIM': '/api/enrollment', # SERVERS is environment-specific (LMS_ROOT_URL differs per env) and is # set in devstack.py / production.py. } diff --git a/lms/lib/spectacular.py b/lms/lib/spectacular.py index 433b05f6db11..f228d54374d9 100644 --- a/lms/lib/spectacular.py +++ b/lms/lib/spectacular.py @@ -15,3 +15,19 @@ def lms_api_filter(endpoints): filtered.append((path, path_regex, method, callback)) return filtered + + +def lms_mark_legacy_paths_deprecated(result, generator, request, public): # pylint: disable=unused-argument + """ + Mark the legacy slashless Enrollment v2 addresses ``deprecated: true``. + + Conforming routes always end in a slash, so a slashless v2 path is by + construction a legacy address. + """ + for path, path_item in result.get("paths", {}).items(): + if not path.startswith("/api/enrollment/v2/") or path.endswith("/"): + continue + for operation in path_item.values(): + if isinstance(operation, dict): + operation["deprecated"] = True + return result diff --git a/lms/lib/tests/test_spectacular.py b/lms/lib/tests/test_spectacular.py new file mode 100644 index 000000000000..af42446fb5bc --- /dev/null +++ b/lms/lib/tests/test_spectacular.py @@ -0,0 +1,52 @@ +"""Tests for the LMS drf-spectacular hooks.""" + +from django.test import SimpleTestCase + +from lms.lib.spectacular import lms_api_filter, lms_mark_legacy_paths_deprecated + + +def _endpoint(path): + return (path, path, "GET", None) + + +def _schema(*paths): + return {"paths": {path: {"get": {"operationId": path}} for path in paths}} + + +class LmsApiFilterTest(SimpleTestCase): + """The pre-processing hook keeps only enrollment endpoints.""" + + def test_keeps_enrollment_drops_the_rest(self): + kept = lms_api_filter([ + _endpoint("/api/enrollment/v2/enrollments/"), + _endpoint("/api/enrollment/v1/enrollment"), + _endpoint("/api/user/v1/accounts/"), + ]) + assert [path for path, *_ in kept] == [ + "/api/enrollment/v2/enrollments/", + "/api/enrollment/v1/enrollment", + ] + + +class LmsMarkLegacyPathsDeprecatedTest(SimpleTestCase): + """Slashless v2 paths are legacy and deprecated; slashed ones are conforming.""" + + def test_only_slashless_v2_paths_are_deprecated(self): + result = lms_mark_legacy_paths_deprecated(_schema( + "/api/enrollment/v2/enrollments", + "/api/enrollment/v2/enrollments/", + "/api/enrollment/v2/course/{course_key}", + "/api/enrollment/v2/courses/{course_key}/", + "/api/enrollment/v1/enrollment", + ), None, None, False) + paths = result["paths"] + assert paths["/api/enrollment/v2/enrollments"]["get"]["deprecated"] is True + assert paths["/api/enrollment/v2/course/{course_key}"]["get"]["deprecated"] is True + assert "deprecated" not in paths["/api/enrollment/v2/enrollments/"]["get"] + assert "deprecated" not in paths["/api/enrollment/v2/courses/{course_key}/"]["get"] + assert "deprecated" not in paths["/api/enrollment/v1/enrollment"]["get"] + + def test_paths_are_left_as_full_urls(self): + """Paths are not trimmed, so every key resolves against LMS_ROOT_URL.""" + result = lms_mark_legacy_paths_deprecated(_schema("/api/enrollment/v2/enrollments/"), None, None, False) + assert list(result["paths"]) == ["/api/enrollment/v2/enrollments/"] diff --git a/lms/urls.py b/lms/urls.py index 0765504d4080..5b6fdefa47b1 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -13,6 +13,7 @@ from drf_spectacular.views import SpectacularAPIView from edx_api_doc_tools import make_docs_urls from edx_django_utils.plugins import get_plugin_url_patterns +from edx_rest_framework_extensions.url_converters import register_url_converters from submissions import urls as submissions_urls from common.djangoapps.student import views as student_views @@ -53,6 +54,9 @@ from openedx.core.djangoapps.user_authn.views.login import redirect_to_lms_login from openedx.features.enterprise_support.api import enterprise_enabled +# Shared opaque-key path converters, registered before any pattern using them. +register_url_converters() + RESET_COURSE_DEADLINES_NAME = 'reset_course_deadlines' RENDER_XBLOCK_NAME = 'render_xblock' RENDER_VIDEO_XBLOCK_NAME = 'render_public_video_xblock' diff --git a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py index 414aa8075926..684f59ed991a 100644 --- a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py +++ b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py @@ -13,7 +13,7 @@ from unittest.mock import patch from django.test import override_settings -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APITestCase @@ -213,7 +213,9 @@ class TestUserRolesViewAliases(APITestCase): def setUp(self): super().setUp() self.user = UserFactory.create(password="test") - self.url = reverse("v2:enrollment-v2-roles") + # Renamed from the versioned kebab-case ``enrollment-v2-roles`` + # (the path is unchanged). + self.url = reverse("v2:user_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 @@ -287,3 +289,81 @@ def test_minimal_view_collapses_course_details_to_course_id(self, mock_list, moc assert {r["course_id"] for r in response.data["results"]} == { "course-v1:org+a+r", "course-v1:org+b+r", } + + +# --------------------------------------------------------------------------- +# URL-structure tests +# --------------------------------------------------------------------------- + +@skip_unless_lms +class TestEnrollmentUrlStructure(APITestCase): + """Conforming trailing-slash routes with snake_case names, beside the legacy slashless ones.""" + + USERNAME = "someone" + COURSE_ID = "course-v1:org+course+run" + + def test_conforming_urls_reverse_to_expected_paths(self): + assert reverse("v2:enrollment_admin_list") == "/api/enrollment/v2/enrollments/" + assert reverse( + "v2:enrollment_detail", + kwargs={"username": self.USERNAME, "course_id": self.COURSE_ID}, + ) == f"/api/enrollment/v2/enrollments/{self.USERNAME},{self.COURSE_ID}/" + assert reverse( + "v2:course_enrollment_detail", kwargs={"course_id": self.COURSE_ID}, + ) == f"/api/enrollment/v2/courses/{self.COURSE_ID}/" + assert reverse("v2:user_roles") == "/api/enrollment/v2/roles/" + + def test_conforming_and_legacy_routes_share_views(self): + pairs = ( + # (conforming path, legacy path) + ("/api/enrollment/v2/enrollments/", "/api/enrollment/v2/enrollments"), + ( + f"/api/enrollment/v2/enrollments/{self.USERNAME},{self.COURSE_ID}/", + f"/api/enrollment/v2/enrollment/{self.USERNAME},{self.COURSE_ID}", + ), + ( + f"/api/enrollment/v2/courses/{self.COURSE_ID}/", + f"/api/enrollment/v2/course/{self.COURSE_ID}", + ), + ) + for conforming, legacy in pairs: + assert resolve(conforming).func.cls is resolve(legacy).func.cls, ( + f"{conforming} must serve the same view as {legacy}" + ) + + def test_legacy_admin_list_optional_slash_coverage_is_preserved(self): + """ + The legacy ``^enrollments/?$`` optional-slash pattern is split into a + conforming slashed route plus a slashless legacy route: both + addresses still resolve, one route each. + """ + slashless = resolve("/api/enrollment/v2/enrollments") + slashed = resolve("/api/enrollment/v2/enrollments/") + assert slashless.func.cls is slashed.func.cls + assert slashless.url_name == "enrollment-v2-admin-list" + assert slashed.url_name == "enrollment_admin_list" + + def test_legacy_retrieve_routes_have_unique_names(self): + """ + The two legacy retrieve forms no longer share one URL name (Django + disambiguated them only by argument signature). + """ + composite = resolve( + f"/api/enrollment/v2/enrollment/{self.USERNAME},{self.COURSE_ID}" + ) + course_only = resolve(f"/api/enrollment/v2/enrollment/{self.COURSE_ID}") + assert composite.func.cls is course_only.func.cls + assert composite.url_name == "enrollment-v2-retrieve" + assert course_only.url_name == "enrollment-v2-retrieve-own" + + def test_invalid_course_key_is_404_on_conforming_route(self): + # The shared course_key converter rejects unparseable keys with a + # routing-level 404. + response = self.client.get("/api/enrollment/v2/courses/not-a-course-key/") + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_admin_list_contract_is_identical_on_both_addresses(self): + """Unauthenticated callers get the same 401 on legacy and conforming.""" + legacy = self.client.get("/api/enrollment/v2/enrollments") + conforming = self.client.get("/api/enrollment/v2/enrollments/") + assert legacy.status_code == conforming.status_code == status.HTTP_401_UNAUTHORIZED diff --git a/openedx/core/djangoapps/enrollments/v2/urls.py b/openedx/core/djangoapps/enrollments/v2/urls.py index cda839fd4319..1706082633f2 100644 --- a/openedx/core/djangoapps/enrollments/v2/urls.py +++ b/openedx/core/djangoapps/enrollments/v2/urls.py @@ -3,12 +3,11 @@ 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()``. +Conforming routes are dual-mounted beside the legacy slashless +routes, which keep their original names and are marked ``deprecated: true`` +in the OpenAPI schema (``lms/lib/spectacular.py``). Collapsing ``enrollment/`` +into ``enrollments/``, replacing ``unenroll`` with ``DELETE``, and addressing +the caller as ``me`` are contract changes deferred to a future version. URL surface ----------- @@ -21,12 +20,17 @@ POST /enrollment/enrollment_allowed/ DELETE /enrollment/enrollment_allowed/ -Explicit paths: +Conforming explicit paths: + GET /enrollments/ (name: enrollment_admin_list) + GET /enrollments/{username},{course_key}/ (name: enrollment_detail) + GET /courses/{course_key}/ (name: course_enrollment_detail) + GET /roles/ (name: user_roles) + +Legacy paths (deprecated, kept for their deprecation window): 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 /enrollment/{course_key} (name: enrollment-v2-retrieve-own) + 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 @@ -46,7 +50,33 @@ router = DefaultRouter() router.register(r"enrollment", EnrollmentViewSet, basename="enrollment") -urlpatterns = router.urls + [ +urlpatterns = [ + *router.urls, + # Conforming routes. + path( + "enrollments/", + EnrollmentsAdminListView.as_view(), + name="enrollment_admin_list", + ), + path( + "enrollments/,/", + EnrollmentRetrieveView.as_view(), + name="enrollment_detail", + ), + path( + "courses//", + CourseEnrollmentDetailView.as_view(), + name="course_enrollment_detail", + ), + path("roles/", UserRolesView.as_view(), name="user_roles"), + # Legacy routes, kept for their deprecation window. The admin list's + # optional-slash pattern is narrowed to slashless only, since the slashed + # address is now served by the conforming route above. + re_path( + r"^enrollments$", + EnrollmentsAdminListView.as_view(), + name="enrollment-v2-admin-list", + ), re_path( r"^enrollment/{username},{course_key}$".format( # noqa: UP032 username=settings.USERNAME_PATTERN, course_key=settings.COURSE_ID_PATTERN, @@ -57,17 +87,13 @@ 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", + # Was sharing ``enrollment-v2-retrieve`` with the composite-key form + # above; nothing reverses it, so it gets its own name. + name="enrollment-v2-retrieve-own", ), 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/views.py b/openedx/core/djangoapps/enrollments/v2/views.py index 5979b0aa92f7..180089e89bee 100644 --- a/openedx/core/djangoapps/enrollments/v2/views.py +++ b/openedx/core/djangoapps/enrollments/v2/views.py @@ -468,6 +468,11 @@ def get(self, request, course_id=None, username=None): ``has_api_key`` or staff privileges raises ``NotFound`` (so the caller cannot probe for the existence of other users' enrollments). """ + # The conforming route passes a parsed CourseKey; the legacy route + # passes the raw string. Coerce to the string form used below. + if course_id is not None and not isinstance(course_id, str): + course_id = str(course_id) + if username is None: username = request.user.username @@ -610,6 +615,10 @@ def get(self, request, course_id=None): course schedule and supported enrollment modes; pass ``?include_expired=1`` to include expired enrollment modes. """ + # The conforming route passes a parsed CourseKey; the legacy route + # passes the raw string. + if course_id is not None and not isinstance(course_id, str): + course_id = str(course_id) try: course_key = CourseKey.from_string(course_id) except InvalidKeyError as exc: diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 74dcae4d3d8c..8fd1a45ca988 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -467,7 +467,7 @@ edx-django-utils==8.0.2 # ora2 # super-csv # xblocks-contrib -edx-drf-extensions==10.7.0 +edx-drf-extensions==10.8.0 # via # edx-completion # edx-enterprise diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 37e0d79208db..352848bce529 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -520,7 +520,7 @@ edx-django-utils==8.0.2 # ora2 # super-csv # xblocks-contrib -edx-drf-extensions==10.7.0 +edx-drf-extensions==10.8.0 # via # edx-completion # edx-enterprise diff --git a/uv.lock b/uv.lock index 310e9c56fb20..7bdde241397c 100644 --- a/uv.lock +++ b/uv.lock @@ -2023,7 +2023,7 @@ wheels = [ [[package]] name = "edx-drf-extensions" -version = "10.7.0" +version = "10.8.0" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "django", version = "4.2.30", source = { registry = "https://pypi.org/simple" }, marker = "extra == 'group-16-openedx-platform-django42'" }, @@ -2037,9 +2037,9 @@ dependencies = [ { name = "requests" }, { name = "semantic-version" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/50/06/b32d6d48415d9278c188a70da786796715ce2c4e73178f114fd498108052/edx_drf_extensions-10.7.0.tar.gz", hash = "sha256:784710bf9dc77e4234d201295963c20fd15b4e27595f1c1587b180a79e0914d4", size = 80429, upload-time = "2026-08-18T15:32:48.976Z" } +sdist = { url = "https://files.pythonhosted.org/packages/03/ce/7b348f25bb9a975171166904abe740a026ce7c6dca10e3827c927aab36cb/edx_drf_extensions-10.8.0.tar.gz", hash = "sha256:f7a6d1d0a4cfdec7c95635b0f3eb427cc473f28428bfabe3c4e3db0095feb6da", size = 99711, upload-time = "2026-09-02T20:47:28.28Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/e8/e6/03aa9fc1de473702887657c0e165798c99dee32c77a6eca5a4b2f4d8809d/edx_drf_extensions-10.7.0-py2.py3-none-any.whl", hash = "sha256:c1931816a88ac60908051e28ecb6fd18ba97cc3d6f18d61360d8eb22d3203886", size = 79474, upload-time = "2026-08-18T15:32:47.815Z" }, + { url = "https://files.pythonhosted.org/packages/ff/cd/7db09d26bb1c01ecef32d0762207930c900caf689d9bdc635c31406d4488/edx_drf_extensions-10.8.0-py2.py3-none-any.whl", hash = "sha256:853892aaba931315e82a3d3cf3cab57c5fe105f8d1760d70a45455b6e995e33b", size = 102718, upload-time = "2026-09-02T20:47:26.99Z" }, ] [[package]]