From 6ba0b58303111b7393087d6c9a7b007b651207b1 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Sun, 30 Aug 2026 21:44:59 +0500 Subject: [PATCH 01/10] feat: wire shared opaque-key URL path converters from edx-drf-extensions (ADR 0038) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Register the CourseKeyConverter / UsageKeyConverter path converters — added to edx-drf-extensions 10.8.0 (openedx/edx-drf-extensions#573) per ADR 0038's 'Code examples' section — once per service in lms/urls.py and cms/urls.py, as / . ADR 0038 rule 9: conforming routes resolve opaque keys in the URLconf, views receive parsed keys, and malformed or deprecated (Org/Course/Run, i4x://) keys become routing-level 404s. Bumps edx-drf-extensions 10.7.0 -> 10.8.0, the release that adds the converters (plus the ADR 0029/0032/0036 building blocks this API series already consumes). Converter unit tests live in the library; the per-API URL tests in the following commits cover resolve/reverse integration through the real routes. --- cms/urls.py | 5 +++++ lms/urls.py | 5 +++++ requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- uv.lock | 6 +++--- 5 files changed, 15 insertions(+), 5 deletions(-) diff --git a/cms/urls.py b/cms/urls.py index c0f96f489bb8..bd42c1f549d0 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,10 @@ 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 (ADR 0038): registered once per service, +# before any URL pattern that uses / . +register_url_converters() + django_autodiscover() admin.site.site_header = _('Studio Administration') admin.site.site_title = admin.site.site_header diff --git a/lms/urls.py b/lms/urls.py index 0765504d4080..173f1d409baa 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,10 @@ 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 (ADR 0038): registered once per service, +# before any URL pattern that uses / . +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/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]] From ccaa46bd314ba0f6dbe304d119d2fd5660d6741f Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Sun, 30 Aug 2026 21:45:32 +0500 Subject: [PATCH 02/10] feat: apply ADR 0038 to Xblock v1 (/api/authoring/v1/xblocks/) Mount the conforming routes beside the legacy /api/contentstore/v1/xblock/ ones (OEP-21), serving the same XblockViewSet: the collection becomes plural (rule 2), the API name describes the domain rather than the implementing Django app (rule 3), the usage key is resolved by the shared usage_key converter, which turns malformed and deprecated i4x:// keys into routing-level 404s (rule 9), and URL names are snake_case, version-free, and unique (rule 11). The viewset's initial() coerces a parsed UsageKey back to the string form the action methods expect, so both mounts share one contract. The legacy routes stay live for their deprecation window and are marked deprecated: true in the OpenAPI schema via the new cms_mark_migrated_paths post-processing hook; cms_api_filter now also admits /api/authoring/ paths. Tests pin reverse() literals, same-view resolution for both mounts, the routing-level 404, and handler parity on the conforming routes. ADR 0038 (implementation note 4) asks that /api/authoring/v1/xblocks/ be reconciled with the Learning Core /api/xblock/v2/xblocks/ rather than leaving two names for what looks like one API; that reconciliation is an API-owner decision tracked with the DEPR work, not part of this mechanical migration. --- .../rest_api/v1/authoring_urls.py | 52 ++++++++++++++ .../v1/views/tests/test_xblock_viewset.py | 69 ++++++++++++++++++- .../contentstore/rest_api/v1/views/xblock.py | 10 ++- cms/envs/devstack.py | 8 +++ cms/envs/production.py | 8 +++ cms/lib/spectacular.py | 40 ++++++++++- cms/urls.py | 8 +++ 7 files changed, 190 insertions(+), 5 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py 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..ab66ea1854f3 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -0,0 +1,52 @@ +""" +Conforming (ADR 0038) URLs for the authoring API, v1. + +Mounted at ``api/authoring/v1/`` from ``cms/urls.py``, beside the legacy +``/api/contentstore/v1/xblock/`` routes, which stay live for their OEP-21 +deprecation window and are marked ``deprecated: true`` in the OpenAPI schema +(see ``cms/lib/spectacular.py``). + +ADR 0038 conformance relative to the legacy mount: + +* rule 2 — the collection is plural (``xblocks/``), the API name singular; +* rule 3 — the API name describes the domain (``authoring``), not the + implementing Django app (``contentstore``); +* rule 9 — the identifier is resolved by the shared ``usage_key`` path + converter (``edx_rest_framework_extensions.url_converters``), which rejects + deprecated ``i4x://`` keys with a 404; +* rule 11 — URL names are ``snake_case``, version-free, and unique. + +Note: ADR 0038 (implementation note 4) asks that ``/api/authoring/v1/xblocks/`` +be reconciled with the existing Learning Core ``/api/xblock/v2/xblocks/`` +rather than leaving two names for what looks like one API. That reconciliation +is an API-owner decision tracked with the DEPR work, not part of this +mechanical migration. +""" + +from django.urls import path + +from cms.djangoapps.contentstore.rest_api.v1.views import XblockViewSet + +app_name = "authoring_v1" + +urlpatterns = [ + # No ``list`` action exists on the viewset, so the collection URL accepts + # POST only — the same surface the legacy router-generated route exposes. + 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..0edee2c4b992 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,70 @@ 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" + + +# --------------------------------------------------------------------------- +# ADR 0038 — 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): + """ + ADR 0038 — the conforming /api/authoring/v1/xblocks/ routes are mounted + beside the legacy /api/contentstore/v1/xblock/ routes and serve the same + view. + """ + + 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..dac3953c330c 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py @@ -198,7 +198,15 @@ 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") + # ADR 0038: the conforming /api/authoring/v1/xblocks// + # route passes a parsed UsageKey, while the legacy + # /api/contentstore/v1/xblock/ route passes the raw string. Coerce to + # the string form the action methods expect; ``self.kwargs`` is the + # same dict ``dispatch()`` unpacks into the handler, so the handler + # receives the coerced value as well. + 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/envs/devstack.py b/cms/envs/devstack.py index 0576a35272af..0562c3099a1c 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -356,6 +356,14 @@ 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'], + # ADR 0038 / OEP-21: mark legacy addresses of migrated APIs deprecated + # and BFF surfaces x-internal (cms/lib/spectacular.py). The enum hook is + # drf-spectacular's default, restated because setting this key overrides + # the default list. + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'cms.lib.spectacular.cms_mark_migrated_paths', + ], # 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', diff --git a/cms/envs/production.py b/cms/envs/production.py index 604d2753bccd..350aa52037b6 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -416,6 +416,14 @@ 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'], + # ADR 0038 / OEP-21: mark legacy addresses of migrated APIs deprecated + # and BFF surfaces x-internal (cms/lib/spectacular.py). The enum hook is + # drf-spectacular's default, restated because setting this key overrides + # the default list. + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'cms.lib.spectacular.cms_mark_migrated_paths', + ], # 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', diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 90bce5668fec..506da615e47f 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -2,14 +2,28 @@ import re +# Legacy schema paths of the APIs migrated to their ADR 0038-conforming +# /api/authoring/ addresses. The legacy routes stay live for their OEP-21 +# deprecation window and are marked ``deprecated: true`` in the schema so +# generated clients steer to the conforming address. Paths are as they appear +# in the schema, i.e. after SCHEMA_PATH_PREFIX_TRIM strips /api/contentstore. +LEGACY_MIGRATED_PATH_PREFIXES = ( + "/v1/xblock/", # → /api/authoring/v1/xblocks/ +) + +# BFF surfaces (ADR 0038): kept under /api/ with one canonical conforming +# mount, but marked ``x-internal`` so clients can tell them apart from a +# stable resource contract. Applies to both the legacy and conforming mounts. +INTERNAL_BFF_PATH_PREFIXES = () + 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 +36,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 (ADR 0038 / OEP-21): 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/urls.py b/cms/urls.py index bd42c1f549d0..55af7b5cf0e3 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -361,6 +361,14 @@ path('api/contentstore/', include('cms.djangoapps.contentstore.rest_api.urls')) ] +# Authoring REST APIs — the ADR 0038-conforming addresses of the APIs +# standardized under FC-0118, dual-mounted (OEP-21) beside their legacy +# /api/contentstore/ routes during the deprecation window. Per ADR 0038 +# rule 5, each mount declares its own full api/{api_name}/v{N}/ prefix. +urlpatterns += [ + path('api/authoring/v1/', include('cms.djangoapps.contentstore.rest_api.v1.authoring_urls')), +] + # Content tagging urlpatterns += [ path('api/content_tagging/', include(('openedx.core.djangoapps.content_tagging.urls', 'content_tagging'))), From 6b7f19d3491b5ba2f7b5242e218338cce73a362f Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Sun, 30 Aug 2026 21:45:53 +0500 Subject: [PATCH 03/10] feat: apply ADR 0038 to CourseHome v3 (/api/authoring/v3/home/, x-internal BFF) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mount the conforming home/, home/courses/, and home/libraries/ routes at /api/authoring/v3/ beside the legacy /api/contentstore/v3/home/ ones (OEP-21), serving the same HomeViewSet, with snake_case version-free URL names (rule 11) and the domain-named api_name (rule 3). home is a BFF aggregate for the Studio home screen. Rule 4 disfavors screen names as resources, but the ADR's BFF provision applies: the surface keeps the /api/ prefix and one canonical conforming mount, and is marked x-internal in the OpenAPI schema — on both mounts — so clients can tell it apart from a stable resource contract. The legacy routes are additionally marked deprecated: true. Tests pin reverse() literals, same-view resolution for all three action pairs, and the ADR 0029 envelope on the conforming mount. --- .../rest_api/v3/authoring_urls.py | 45 +++++++++++++++++++ .../rest_api/v3/tests/test_home.py | 36 ++++++++++++++- cms/lib/spectacular.py | 6 ++- cms/urls.py | 1 + 4 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py 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..b12da94cc9c2 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py @@ -0,0 +1,45 @@ +""" +Conforming (ADR 0038) URLs for the authoring API, v3. + +Mounted at ``api/authoring/v3/`` from ``cms/urls.py``, beside the legacy +``/api/contentstore/v3/`` routes, which stay live for their OEP-21 +deprecation window and are marked ``deprecated: true`` in the OpenAPI schema +(see ``cms/lib/spectacular.py``). + +ADR 0038 conformance relative to the legacy mount: + +* rule 3 — the API name describes the domain (``authoring``), not the + implementing Django app (``contentstore``); +* rule 11 — URL names are ``snake_case``, version-free, and unique. + +``home/`` is a BFF aggregate for the Studio home screen. Rule 4 disfavors +screen names, but ADR 0038's BFF provision applies: the surface keeps its +``/api/`` prefix and a single canonical conforming mount, and is marked +``x-internal`` in the OpenAPI schema (``cms/lib/spectacular.py``) so clients +can tell it apart from a stable resource contract. +""" + +from django.urls import path + +from cms.djangoapps.contentstore.rest_api.v3.views import HomeViewSet + +app_name = "authoring_v3" + +urlpatterns = [ + # Studio home BFF (x-internal — see module docstring). + 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", + ), +] 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..c882dc29278b 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,37 @@ 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 + + +# =========================================================================== +# ADR 0038 — URL-structure tests +# =========================================================================== +class TestHomeViewSetUrlStructure(APITestCase): + """ + ADR 0038 — the conforming /api/authoring/v3/home/ routes are mounted + beside the legacy /api/contentstore/v3/home/ routes and serve the same + view. + """ + + 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 — ADR 0029 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"ADR 0029: missing field '{field}'" diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 506da615e47f..6fbb2f7ed87e 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -9,12 +9,16 @@ # in the schema, i.e. after SCHEMA_PATH_PREFIX_TRIM strips /api/contentstore. LEGACY_MIGRATED_PATH_PREFIXES = ( "/v1/xblock/", # → /api/authoring/v1/xblocks/ + "/v3/home/", # → /api/authoring/v3/home/ ) # BFF surfaces (ADR 0038): kept under /api/ with one canonical conforming # mount, but marked ``x-internal`` so clients can tell them apart from a # stable resource contract. Applies to both the legacy and conforming mounts. -INTERNAL_BFF_PATH_PREFIXES = () +INTERNAL_BFF_PATH_PREFIXES = ( + "/v3/home/", + "/api/authoring/v3/home/", +) def cms_api_filter(endpoints): diff --git a/cms/urls.py b/cms/urls.py index 55af7b5cf0e3..624ef8bcd32d 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -367,6 +367,7 @@ # rule 5, each mount declares its own full api/{api_name}/v{N}/ prefix. 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')), ] # Content tagging From f9c63a1e997c7d2c9524b7c4b6fd38bb52243dfa Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Sun, 30 Aug 2026 21:46:11 +0500 Subject: [PATCH 04/10] feat: apply ADR 0038 to CourseHome v4 (/api/authoring/v4/courses/) Mount the conforming courses/ collection at /api/authoring/v4/ beside the legacy /api/contentstore/v4/home/courses/ route (OEP-21), serving the same HomeCoursesViewSet: the screen-shaped home/courses/ address becomes the concrete plural collection of authorable courses (rule 4), filtered, sorted, and paginated in the query string, under the domain-named api_name (rule 3) with a snake_case version-free URL name (rule 11). The legacy route stays live for its deprecation window and is marked deprecated: true in the OpenAPI schema. Tests pin the reverse() literal, same-view resolution for both mounts, and 401/200 contract parity on the conforming mount. --- .../rest_api/v4/authoring_urls.py | 31 ++++++++++++++ .../rest_api/v4/views/tests/test_home.py | 40 ++++++++++++++++++- cms/lib/spectacular.py | 1 + cms/urls.py | 1 + 4 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py 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..f97257f982e0 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py @@ -0,0 +1,31 @@ +""" +Conforming (ADR 0038) URLs for the authoring API, v4. + +Mounted at ``api/authoring/v4/`` from ``cms/urls.py``, beside the legacy +``/api/contentstore/v4/home/courses/`` route, which stays live for its OEP-21 +deprecation window and is marked ``deprecated: true`` in the OpenAPI schema +(see ``cms/lib/spectacular.py``). + +ADR 0038 conformance relative to the legacy mount: + +* rule 3 — the API name describes the domain (``authoring``), not the + implementing Django app (``contentstore``); +* rule 4 — the screen-shaped ``home/courses/`` address becomes the concrete + plural collection ``courses/`` (the authorable courses, filtered, sorted, + and paginated in the query string); +* rule 11 — the URL name is ``snake_case``, version-free, and unique. +""" + +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..b9bc646ed8b4 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,41 @@ def test_no_ordering_param_no_deprecation_header(self): response = self.client.get(self.list_url) self.assertNotIn("Deprecation", response) # noqa: PT009 + + +# =========================================================================== +# ADR 0038 — URL-structure tests +# =========================================================================== +class TestHomeCoursesViewSetUrlStructure(APITestCase): + """ + ADR 0038 — the conforming /api/authoring/v4/courses/ route is mounted + beside the legacy /api/contentstore/v4/home/courses/ route and serves + the same view. + """ + + 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/lib/spectacular.py b/cms/lib/spectacular.py index 6fbb2f7ed87e..ef520474802a 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -10,6 +10,7 @@ LEGACY_MIGRATED_PATH_PREFIXES = ( "/v1/xblock/", # → /api/authoring/v1/xblocks/ "/v3/home/", # → /api/authoring/v3/home/ + "/v4/home/courses/", # → /api/authoring/v4/courses/ ) # BFF surfaces (ADR 0038): kept under /api/ with one canonical conforming diff --git a/cms/urls.py b/cms/urls.py index 624ef8bcd32d..38c2a71c0812 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -368,6 +368,7 @@ 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 From 1cd3153258a527cbd1a7a9f55ff3bd3244a785db Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Sun, 30 Aug 2026 21:46:39 +0500 Subject: [PATCH 05/10] feat: apply ADR 0038 to Course Detail v3 (courses/{course_key}/details/) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mount the conforming /api/authoring/v3/courses/{course_key}/details/ route beside the legacy /api/contentstore/v3/course_details/{course_id}/ one (OEP-21), serving the same CourseDetailsViewSet: the screen-shaped collection becomes a sub-resource of the plural courses/ collection, one level deep — the ADR's own target for these endpoints (rules 4 and 8) — with the course key resolved by the shared course_key converter, which turns malformed and deprecated Org/Course/Run keys into routing-level 404s (rule 9). resolve_course_key() now also accepts an already-parsed CourseKey, so both mounts funnel through one code path and share one contract. The legacy route stays live for its deprecation window and is marked deprecated: true in the OpenAPI schema. Tests pin the reverse() literal, same-view resolution, the routing-level 404, and 401/403 parity on the conforming mount. --- .../rest_api/v3/authoring_urls.py | 16 +++++- .../contentstore/rest_api/v3/utils.py | 13 +++-- .../v3/views/tests/test_course_details.py | 55 ++++++++++++++++++- cms/lib/spectacular.py | 1 + 4 files changed, 79 insertions(+), 6 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py index b12da94cc9c2..2f0fbba76a33 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py @@ -10,6 +10,13 @@ * rule 3 — the API name describes the domain (``authoring``), not the implementing Django app (``contentstore``); +* rule 4 / 8 — the screen-shaped ``course_details`` collection becomes a + sub-resource of the plural ``courses/`` collection, one level deep, per the + ADR's own target for these endpoints + (``/api/authoring/…/courses/{course_key}/details/`` "and siblings"); +* rule 9 — course keys are resolved by the shared ``course_key`` path + converter (``edx_rest_framework_extensions.url_converters``), which rejects + deprecated ``Org/Course/Run`` keys with a 404; * rule 11 — URL names are ``snake_case``, version-free, and unique. ``home/`` is a BFF aggregate for the Studio home screen. Rule 4 disfavors @@ -21,7 +28,7 @@ from django.urls import path -from cms.djangoapps.contentstore.rest_api.v3.views import HomeViewSet +from cms.djangoapps.contentstore.rest_api.v3.views import CourseDetailsViewSet, HomeViewSet app_name = "authoring_v3" @@ -42,4 +49,11 @@ HomeViewSet.as_view({"get": "libraries"}), name="home_libraries", ), + # Course details — /api/contentstore/v3/course_details/{course_id}/ + # renamed per the ADR's target shape; same view, same contract. + path( + "courses//details/", + CourseDetailsViewSet.as_view({"get": "retrieve", "put": "update"}), + name="course_details", + ), ] diff --git a/cms/djangoapps/contentstore/rest_api/v3/utils.py b/cms/djangoapps/contentstore/rest_api/v3/utils.py index 79524acb8c53..4fed4e44f7d8 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 — + ADR 0038 rule 9 — 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_course_details.py b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py index e25df0188d18..6b838e00a167 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,56 @@ def test_fields_csv_restricts_top_level_keys( assert response.status_code == status.HTTP_200_OK assert set(response.data.keys()) == {"course_id", "title"} + + +# =========================================================================== +# ADR 0038 — URL-structure tests +# =========================================================================== +class TestCourseDetailsViewSetUrlStructure(APITestCase): + """ + ADR 0038 — the conforming /api/authoring/v3/courses/{course_key}/details/ + route is mounted beside the legacy + /api/contentstore/v3/course_details/{course_id}/ route and serves the + same view. + """ + + 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/lib/spectacular.py b/cms/lib/spectacular.py index ef520474802a..15d0410b03b4 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -10,6 +10,7 @@ LEGACY_MIGRATED_PATH_PREFIXES = ( "/v1/xblock/", # → /api/authoring/v1/xblocks/ "/v3/home/", # → /api/authoring/v3/home/ + "/v3/course_details/", # → /api/authoring/v3/courses/{course_key}/details/ "/v4/home/courses/", # → /api/authoring/v4/courses/ ) From 58c1a0d19b6204a1bab1b35410e7b09edb18367a Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Sun, 30 Aug 2026 21:47:18 +0500 Subject: [PATCH 06/10] feat: apply ADR 0038 to AuthorGrading v3 (courses/{course_key}/grading/) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mount the conforming /api/authoring/v3/courses/{course_key}/grading/ route beside the legacy /api/contentstore/v3/authoring_grading/{course_key}/ one (OEP-21), serving the same AuthoringGradingViewSet: the app-flavored authoring_grading collection becomes the grading sub-resource of the plural courses/ collection, one level deep (rules 3, 4 and 8) — the authoring_ prefix is dropped because the namespace already says it — with the course key resolved by the shared course_key converter (rule 9). Both mounts funnel through resolve_course_key(), which already accepts parsed keys, so they share one contract. The legacy route stays live for its deprecation window and is marked deprecated: true in the OpenAPI schema. Tests pin the reverse() literal, same-view resolution, the routing-level 404, and 401/200 PATCH parity on the conforming mount. --- .../rest_api/v3/authoring_urls.py | 16 +++-- .../v3/views/tests/test_authoring_grading.py | 65 ++++++++++++++++++- cms/lib/spectacular.py | 1 + 3 files changed, 77 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py index 2f0fbba76a33..c9393179a3d5 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py @@ -10,9 +10,9 @@ * rule 3 — the API name describes the domain (``authoring``), not the implementing Django app (``contentstore``); -* rule 4 / 8 — the screen-shaped ``course_details`` collection becomes a - sub-resource of the plural ``courses/`` collection, one level deep, per the - ADR's own target for these endpoints +* rule 4 / 8 — the screen-shaped ``course_details`` and ``authoring_grading`` + collections become sub-resources of the plural ``courses/`` collection, + one level deep, per the ADR's own target for these endpoints (``/api/authoring/…/courses/{course_key}/details/`` "and siblings"); * rule 9 — course keys are resolved by the shared ``course_key`` path converter (``edx_rest_framework_extensions.url_converters``), which rejects @@ -28,7 +28,7 @@ from django.urls import path -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 = "authoring_v3" @@ -56,4 +56,12 @@ CourseDetailsViewSet.as_view({"get": "retrieve", "put": "update"}), name="course_details", ), + # Course grading — /api/contentstore/v3/authoring_grading/{course_key}/ + # renamed per the ADR's target shape; same view, same contract. The + # ``authoring_`` prefix is dropped because the namespace already says it. + path( + "courses//grading/", + AuthoringGradingViewSet.as_view({"patch": "partial_update"}), + name="course_grading", + ), ] 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..932a5b3b25d5 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,66 @@ 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 + + +# =========================================================================== +# ADR 0038 — URL-structure tests +# =========================================================================== +class TestAuthoringGradingViewSetUrlStructure(APITestCase): + """ + ADR 0038 — the conforming /api/authoring/v3/courses/{course_key}/grading/ + route is mounted beside the legacy + /api/contentstore/v3/authoring_grading/{course_key}/ route and serves + the same view. + """ + + 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/lib/spectacular.py b/cms/lib/spectacular.py index 15d0410b03b4..27e8a03c033c 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -11,6 +11,7 @@ "/v1/xblock/", # → /api/authoring/v1/xblocks/ "/v3/home/", # → /api/authoring/v3/home/ "/v3/course_details/", # → /api/authoring/v3/courses/{course_key}/details/ + "/v3/authoring_grading/", # → /api/authoring/v3/courses/{course_key}/grading/ "/v4/home/courses/", # → /api/authoring/v4/courses/ ) From 1ad2f0ee3db3d488206ba53bce11f00ac64f1f3b Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Sun, 30 Aug 2026 21:47:35 +0500 Subject: [PATCH 07/10] feat: apply ADR 0038 to Enrollment v2 (required trailing slashes + conforming URL names) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /api/enrollment/v2/ already conforms in API name and version position; this fixes the remaining rule 6 and rule 11 violations. Conforming routes are dual-mounted (OEP-21) beside the legacy slashless ones, serving the same views: GET /enrollments/ (the admin list's optional-slash pattern — the ADR's own rule 6 example — is split into an exact slashed route plus a slashless legacy route, so every address that resolved before still resolves), GET /enrollments/{username},{course_key}/ under the plural collection (rule 2), GET /courses/{course_key}/ (plural, slashed), and roles/ renamed from the versioned kebab-case enrollment-v2-roles to user_roles (rule 11; path unchanged). Conforming member routes resolve course keys with the shared course_key converter (rule 9); the views coerce a parsed CourseKey back to the string form their bodies expect, so both mounts share one contract. The two legacy retrieve forms no longer share one URL name — Django resolved that only by argument signature, the fragility rule 11 calls out — and the slashless legacy addresses are marked deprecated: true in the OpenAPI schema via a post-processing hook scoped to /v2/ (deprecating v1 is its own DEPR decision). Deeper ADR 0038 targets — collapsing the singular enrollment/ collection into enrollments/, replacing the unenroll verb (rule 10) with DELETE on the member address, and addressing the requesting user as me (rule 9) — are contract changes and belong to a future v3 per ADR 0037. Tests pin the reverse() literals, same-view resolution for every legacy/conforming pair, the optional-slash coverage split, the unique legacy names, the routing-level 404, and 401 parity on both admin-list addresses. --- lms/envs/common.py | 7 ++ lms/lib/spectacular.py | 21 +++++ .../enrollments/v2/tests/test_views.py | 88 ++++++++++++++++++- .../core/djangoapps/enrollments/v2/urls.py | 70 ++++++++++++--- .../core/djangoapps/enrollments/v2/views.py | 12 +++ 5 files changed, 184 insertions(+), 14 deletions(-) diff --git a/lms/envs/common.py b/lms/envs/common.py index f7a6f15558cb..96b9ee411228 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2161,6 +2161,13 @@ 'VERSION': '0.1.0', 'SERVE_INCLUDE_SCHEMA': False, 'PREPROCESSING_HOOKS': ['lms.lib.spectacular.lms_api_filter'], + # ADR 0038 / OEP-21: mark legacy slashless enrollment addresses + # deprecated (lms/lib/spectacular.py). The enum hook is drf-spectacular's + # default, restated because setting this key overrides the default list. + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'lms.lib.spectacular.lms_mark_legacy_paths_deprecated', + ], 'SCHEMA_PATH_PREFIX': '/api/enrollment', 'SCHEMA_PATH_PREFIX_TRIM': '/api/enrollment', # SERVERS is environment-specific (LMS_ROOT_URL differs per env) and is diff --git a/lms/lib/spectacular.py b/lms/lib/spectacular.py index 433b05f6db11..a1a053b4190c 100644 --- a/lms/lib/spectacular.py +++ b/lms/lib/spectacular.py @@ -15,3 +15,24 @@ 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 + """ + Post-processing hook (ADR 0038 / OEP-21): mark the legacy slashless + Enrollment v2 addresses ``deprecated: true``. + + ADR 0038 rule 6 requires the trailing slash on every conforming route, so + within the migrated v2 surface a path without one is, by construction, a + legacy address whose slashed (or renamed) replacement is mounted beside + it. Scoped to ``/v2/`` so the marking tracks this migration — deprecating + v1 is its own DEPR decision. Paths appear here after + SCHEMA_PATH_PREFIX_TRIM strips /api/enrollment. + """ + for path, path_item in result.get("paths", {}).items(): + if not path.startswith("/v2/") or path.endswith("/"): + continue + for operation in path_item.values(): + if isinstance(operation, dict): + operation["deprecated"] = True + return result diff --git a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py index 414aa8075926..675553fc8c66 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`` + # (ADR 0038; 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,85 @@ 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", } + + +# --------------------------------------------------------------------------- +# ADR 0038 — URL-structure tests +# --------------------------------------------------------------------------- + +@skip_unless_lms +class TestEnrollmentUrlStructure(APITestCase): + """ + ADR 0038 — conforming trailing-slash routes with snake_case URL names, + mounted beside the legacy slashless routes, which keep their names and + serve the same views. + """ + + 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..174369c153c8 100644 --- a/openedx/core/djangoapps/enrollments/v2/urls.py +++ b/openedx/core/djangoapps/enrollments/v2/urls.py @@ -10,6 +10,20 @@ they remain as standalone ``APIView`` classes routed via ``path()`` / ``re_path()``. +ADR 0038 — the API name and version position already conform. The conforming +routes below fix the remaining rule 6 violations (a required trailing slash; +no optional-slash patterns) and rule 11 violations (``snake_case``, +version-free, unique URL names), and are dual-mounted (OEP-21) beside the +legacy slashless routes, which keep their original names and are marked +``deprecated: true`` in the OpenAPI schema (``lms/lib/spectacular.py``). +Conforming member routes live under the plural ``enrollments/`` and +``courses/`` collections (rule 2), with course keys resolved by the shared +``course_key`` converter (rule 9), which rejects deprecated ``Org/Course/Run`` +keys. Deeper ADR 0038 targets — collapsing the singular ``enrollment/`` +collection into ``enrollments/``, replacing ``unenroll`` (a verb, rule 10) +with ``DELETE`` on the member address, and addressing the requesting user as +``me`` — are contract changes and belong to a future v3 per ADR 0037. + URL surface ----------- @@ -21,12 +35,17 @@ POST /enrollment/enrollment_allowed/ DELETE /enrollment/enrollment_allowed/ -Explicit paths: +Conforming explicit paths (ADR 0038): + 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 OEP-21 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 +65,36 @@ router = DefaultRouter() router.register(r"enrollment", EnrollmentViewSet, basename="enrollment") -urlpatterns = router.urls + [ +urlpatterns = [ + *router.urls, + # -- Conforming routes (ADR 0038: required trailing slash, plural + # -- collections, snake_case version-free names, shared key converter). + 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 (OEP-21 deprecation window; ADR 0038 rule 6 + # -- violations frozen as-is, marked deprecated in the OpenAPI schema). + # -- The admin list's optional-slash pattern is narrowed to slashless + # -- only: the slashed address is now served by the conforming route + # -- above, so every address that resolved before still resolves. + 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 +105,15 @@ 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", + # Previously this route shared the name ``enrollment-v2-retrieve`` + # with the composite-key form above, resolving only because Django + # disambiguates by argument signature (the fragility ADR 0038 rule 11 + # calls out). 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..aa55fbfd6ef1 100644 --- a/openedx/core/djangoapps/enrollments/v2/views.py +++ b/openedx/core/djangoapps/enrollments/v2/views.py @@ -468,6 +468,13 @@ 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). """ + # ADR 0038: the conforming /enrollments/{username},{course_key}/ + # route passes a parsed CourseKey (shared ``course_key`` converter); + # the legacy slashless routes pass the raw string. Coerce to the + # string form the body below expects. + 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 +617,11 @@ def get(self, request, course_id=None): course schedule and supported enrollment modes; pass ``?include_expired=1`` to include expired enrollment modes. """ + # ADR 0038: the conforming /courses/{course_key}/ route passes a + # parsed CourseKey (shared ``course_key`` converter); the legacy + # slashless /course/{course_key} 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: From 1677cb50e4edabe616613d4add80128c4e8f59b8 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft <139064778+Abdul-Muqadim-Arbisoft@users.noreply.github.com> Date: Tue, 8 Sep 2026 18:13:14 +0500 Subject: [PATCH 08/10] chore: reduce redundant docstrings and comments across the ADR 0038 migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: the new authoring_urls.py module docstrings restated ADR 0038's rules rather than describing the module, and several comments repeated what the commits already say. The three urls.py modules now carry a one-line docstring matching their siblings in the same package, the per-rule conformance lists and the "same view, same contract" notes are gone, and the test-class docstrings, Enrollment v2 docstring, and spectacular helpers are trimmed; comments that prevent a mistake are kept but shortened (conforming routes pass a parsed CourseKey/UsageKey where legacy routes pass the raw string; POSTPROCESSING_HOOKS replaces rather than extends drf-spectacular's default list). Prose only — with docstrings stripped, all 19 files parse to ASTs identical to the previous revision, ruff passes, and no view or serializer docstring is touched. --- .../rest_api/v1/authoring_urls.py | 28 +----------- .../v1/views/tests/test_xblock_viewset.py | 6 +-- .../contentstore/rest_api/v1/views/xblock.py | 9 ++-- .../rest_api/v3/authoring_urls.py | 34 +------------- .../rest_api/v3/tests/test_home.py | 9 +--- .../v3/views/tests/test_authoring_grading.py | 10 +---- .../v3/views/tests/test_course_details.py | 11 ++--- .../rest_api/v4/authoring_urls.py | 18 +------- .../rest_api/v4/views/tests/test_home.py | 9 +--- cms/envs/devstack.py | 7 ++- cms/envs/production.py | 7 ++- cms/lib/spectacular.py | 12 ++--- cms/urls.py | 9 ++-- lms/envs/common.py | 6 +-- lms/lib/spectacular.py | 13 ++---- lms/urls.py | 3 +- .../enrollments/v2/tests/test_views.py | 6 +-- .../core/djangoapps/enrollments/v2/urls.py | 44 +++++-------------- .../core/djangoapps/enrollments/v2/views.py | 11 ++--- 19 files changed, 52 insertions(+), 200 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py index ab66ea1854f3..17be6d10a3ca 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -1,27 +1,4 @@ -""" -Conforming (ADR 0038) URLs for the authoring API, v1. - -Mounted at ``api/authoring/v1/`` from ``cms/urls.py``, beside the legacy -``/api/contentstore/v1/xblock/`` routes, which stay live for their OEP-21 -deprecation window and are marked ``deprecated: true`` in the OpenAPI schema -(see ``cms/lib/spectacular.py``). - -ADR 0038 conformance relative to the legacy mount: - -* rule 2 — the collection is plural (``xblocks/``), the API name singular; -* rule 3 — the API name describes the domain (``authoring``), not the - implementing Django app (``contentstore``); -* rule 9 — the identifier is resolved by the shared ``usage_key`` path - converter (``edx_rest_framework_extensions.url_converters``), which rejects - deprecated ``i4x://`` keys with a 404; -* rule 11 — URL names are ``snake_case``, version-free, and unique. - -Note: ADR 0038 (implementation note 4) asks that ``/api/authoring/v1/xblocks/`` -be reconciled with the existing Learning Core ``/api/xblock/v2/xblocks/`` -rather than leaving two names for what looks like one API. That reconciliation -is an API-owner decision tracked with the DEPR work, not part of this -mechanical migration. -""" +"""Authoring API v1 URLs (ADR 0038 conforming mount for Contentstore v1).""" from django.urls import path @@ -30,8 +7,7 @@ app_name = "authoring_v1" urlpatterns = [ - # No ``list`` action exists on the viewset, so the collection URL accepts - # POST only — the same surface the legacy router-generated route exposes. + # The viewset has no ``list`` action, so the collection accepts POST only. path( "xblocks/", XblockViewSet.as_view({"post": "create"}), 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 0edee2c4b992..e212bf016932 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 @@ -235,11 +235,7 @@ def _authoring_detail_url(): class XblockViewSetUrlStructureTest(ModuleStoreTestCase, APITestCase): - """ - ADR 0038 — the conforming /api/authoring/v1/xblocks/ routes are mounted - beside the legacy /api/contentstore/v1/xblock/ routes and serve the same - view. - """ + """The conforming /api/authoring/v1/xblocks/ routes serve the same view as the legacy ones.""" def setUp(self): super().setUp() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py index dac3953c330c..02c0434ced2a 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py @@ -198,12 +198,9 @@ def initial(self, request, *args, **kwargs): bytes) rather than request.data to avoid consuming the WSGI stream before @expect_json_in_class_view runs. """ - # ADR 0038: the conforming /api/authoring/v1/xblocks// - # route passes a parsed UsageKey, while the legacy - # /api/contentstore/v1/xblock/ route passes the raw string. Coerce to - # the string form the action methods expect; ``self.kwargs`` is the - # same dict ``dispatch()`` unpacks into the handler, so the handler - # receives the coerced value as well. + # 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") diff --git a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py index c9393179a3d5..56faf73c811c 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py @@ -1,30 +1,4 @@ -""" -Conforming (ADR 0038) URLs for the authoring API, v3. - -Mounted at ``api/authoring/v3/`` from ``cms/urls.py``, beside the legacy -``/api/contentstore/v3/`` routes, which stay live for their OEP-21 -deprecation window and are marked ``deprecated: true`` in the OpenAPI schema -(see ``cms/lib/spectacular.py``). - -ADR 0038 conformance relative to the legacy mount: - -* rule 3 — the API name describes the domain (``authoring``), not the - implementing Django app (``contentstore``); -* rule 4 / 8 — the screen-shaped ``course_details`` and ``authoring_grading`` - collections become sub-resources of the plural ``courses/`` collection, - one level deep, per the ADR's own target for these endpoints - (``/api/authoring/…/courses/{course_key}/details/`` "and siblings"); -* rule 9 — course keys are resolved by the shared ``course_key`` path - converter (``edx_rest_framework_extensions.url_converters``), which rejects - deprecated ``Org/Course/Run`` keys with a 404; -* rule 11 — URL names are ``snake_case``, version-free, and unique. - -``home/`` is a BFF aggregate for the Studio home screen. Rule 4 disfavors -screen names, but ADR 0038's BFF provision applies: the surface keeps its -``/api/`` prefix and a single canonical conforming mount, and is marked -``x-internal`` in the OpenAPI schema (``cms/lib/spectacular.py``) so clients -can tell it apart from a stable resource contract. -""" +"""Authoring API v3 URLs (ADR 0038 conforming mount for Contentstore v3).""" from django.urls import path @@ -33,7 +7,6 @@ app_name = "authoring_v3" urlpatterns = [ - # Studio home BFF (x-internal — see module docstring). path( "home/", HomeViewSet.as_view({"get": "list"}), @@ -49,16 +22,11 @@ HomeViewSet.as_view({"get": "libraries"}), name="home_libraries", ), - # Course details — /api/contentstore/v3/course_details/{course_id}/ - # renamed per the ADR's target shape; same view, same contract. path( "courses//details/", CourseDetailsViewSet.as_view({"get": "retrieve", "put": "update"}), name="course_details", ), - # Course grading — /api/contentstore/v3/authoring_grading/{course_key}/ - # renamed per the ADR's target shape; same view, same contract. The - # ``authoring_`` prefix is dropped because the namespace already says it. path( "courses//grading/", AuthoringGradingViewSet.as_view({"patch": "partial_update"}), 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 c882dc29278b..e48ae26ac47a 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py @@ -86,15 +86,8 @@ def test_v1_endpoint_unaffected_by_v3_envelope(self): assert "instance" not in response.data -# =========================================================================== -# ADR 0038 — URL-structure tests -# =========================================================================== class TestHomeViewSetUrlStructure(APITestCase): - """ - ADR 0038 — the conforming /api/authoring/v3/home/ routes are mounted - beside the legacy /api/contentstore/v3/home/ routes and serve the same - view. - """ + """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/" 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 932a5b3b25d5..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 @@ -293,16 +293,8 @@ def test_v0_endpoint_unaffected_by_v3_envelope(self): assert "instance" not in response.data -# =========================================================================== -# ADR 0038 — URL-structure tests -# =========================================================================== class TestAuthoringGradingViewSetUrlStructure(APITestCase): - """ - ADR 0038 — the conforming /api/authoring/v3/courses/{course_key}/grading/ - route is mounted beside the legacy - /api/contentstore/v3/authoring_grading/{course_key}/ route and serves - the same view. - """ + """The conforming courses/{course_key}/grading/ route serves the same view as the legacy one.""" def setUp(self): super().setUp() 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 6b838e00a167..c57b9123456b 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 @@ -380,16 +380,11 @@ def test_fields_csv_restricts_top_level_keys( assert set(response.data.keys()) == {"course_id", "title"} -# =========================================================================== +# --------------------------------------------------------------------------- # ADR 0038 — URL-structure tests -# =========================================================================== +# --------------------------------------------------------------------------- class TestCourseDetailsViewSetUrlStructure(APITestCase): - """ - ADR 0038 — the conforming /api/authoring/v3/courses/{course_key}/details/ - route is mounted beside the legacy - /api/contentstore/v3/course_details/{course_id}/ route and serves the - same view. - """ + """The conforming courses/{course_key}/details/ route serves the same view as the legacy one.""" def _conforming_url(self): return reverse( diff --git a/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py index f97257f982e0..27e3ec45533a 100644 --- a/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py @@ -1,20 +1,4 @@ -""" -Conforming (ADR 0038) URLs for the authoring API, v4. - -Mounted at ``api/authoring/v4/`` from ``cms/urls.py``, beside the legacy -``/api/contentstore/v4/home/courses/`` route, which stays live for its OEP-21 -deprecation window and is marked ``deprecated: true`` in the OpenAPI schema -(see ``cms/lib/spectacular.py``). - -ADR 0038 conformance relative to the legacy mount: - -* rule 3 — the API name describes the domain (``authoring``), not the - implementing Django app (``contentstore``); -* rule 4 — the screen-shaped ``home/courses/`` address becomes the concrete - plural collection ``courses/`` (the authorable courses, filtered, sorted, - and paginated in the query string); -* rule 11 — the URL name is ``snake_case``, version-free, and unique. -""" +"""Authoring API v4 URLs (ADR 0038 conforming mount for Contentstore v4).""" from django.urls import path 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 b9bc646ed8b4..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 @@ -279,15 +279,8 @@ def test_no_ordering_param_no_deprecation_header(self): self.assertNotIn("Deprecation", response) # noqa: PT009 -# =========================================================================== -# ADR 0038 — URL-structure tests -# =========================================================================== class TestHomeCoursesViewSetUrlStructure(APITestCase): - """ - ADR 0038 — the conforming /api/authoring/v4/courses/ route is mounted - beside the legacy /api/contentstore/v4/home/courses/ route and serves - the same view. - """ + """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/" diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index 0562c3099a1c..595e04884974 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -356,10 +356,9 @@ 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'], - # ADR 0038 / OEP-21: mark legacy addresses of migrated APIs deprecated - # and BFF surfaces x-internal (cms/lib/spectacular.py). The enum hook is - # drf-spectacular's default, restated because setting this key overrides - # the default list. + # 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', diff --git a/cms/envs/production.py b/cms/envs/production.py index 350aa52037b6..c3bd9d94b8d9 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -416,10 +416,9 @@ 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'], - # ADR 0038 / OEP-21: mark legacy addresses of migrated APIs deprecated - # and BFF surfaces x-internal (cms/lib/spectacular.py). The enum hook is - # drf-spectacular's default, restated because setting this key overrides - # the default list. + # 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', diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 27e8a03c033c..f8b04efac2d6 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -2,11 +2,8 @@ import re -# Legacy schema paths of the APIs migrated to their ADR 0038-conforming -# /api/authoring/ addresses. The legacy routes stay live for their OEP-21 -# deprecation window and are marked ``deprecated: true`` in the schema so -# generated clients steer to the conforming address. Paths are as they appear -# in the schema, i.e. after SCHEMA_PATH_PREFIX_TRIM strips /api/contentstore. +# Legacy addresses of APIs migrated to /api/authoring/, marked deprecated for +# their OEP-21 window. Paths are post-SCHEMA_PATH_PREFIX_TRIM. LEGACY_MIGRATED_PATH_PREFIXES = ( "/v1/xblock/", # → /api/authoring/v1/xblocks/ "/v3/home/", # → /api/authoring/v3/home/ @@ -15,9 +12,8 @@ "/v4/home/courses/", # → /api/authoring/v4/courses/ ) -# BFF surfaces (ADR 0038): kept under /api/ with one canonical conforming -# mount, but marked ``x-internal`` so clients can tell them apart from a -# stable resource contract. Applies to both the legacy and conforming mounts. +# 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 = ( "/v3/home/", "/api/authoring/v3/home/", diff --git a/cms/urls.py b/cms/urls.py index 38c2a71c0812..c52f52659155 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -27,8 +27,7 @@ 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 (ADR 0038): registered once per service, -# before any URL pattern that uses / . +# Shared opaque-key path converters, registered before any pattern using them. register_url_converters() django_autodiscover() @@ -361,10 +360,8 @@ path('api/contentstore/', include('cms.djangoapps.contentstore.rest_api.urls')) ] -# Authoring REST APIs — the ADR 0038-conforming addresses of the APIs -# standardized under FC-0118, dual-mounted (OEP-21) beside their legacy -# /api/contentstore/ routes during the deprecation window. Per ADR 0038 -# rule 5, each mount declares its own full api/{api_name}/v{N}/ prefix. +# Authoring REST APIs — conforming addresses (ADR 0038), dual-mounted beside +# their legacy /api/contentstore/ routes for the OEP-21 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')), diff --git a/lms/envs/common.py b/lms/envs/common.py index 96b9ee411228..8bc7a258be8a 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2161,9 +2161,9 @@ 'VERSION': '0.1.0', 'SERVE_INCLUDE_SCHEMA': False, 'PREPROCESSING_HOOKS': ['lms.lib.spectacular.lms_api_filter'], - # ADR 0038 / OEP-21: mark legacy slashless enrollment addresses - # deprecated (lms/lib/spectacular.py). The enum hook is drf-spectacular's - # default, restated because setting this key overrides the default list. + # 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', diff --git a/lms/lib/spectacular.py b/lms/lib/spectacular.py index a1a053b4190c..aea3f8c4dd0b 100644 --- a/lms/lib/spectacular.py +++ b/lms/lib/spectacular.py @@ -19,15 +19,10 @@ def lms_api_filter(endpoints): def lms_mark_legacy_paths_deprecated(result, generator, request, public): # pylint: disable=unused-argument """ - Post-processing hook (ADR 0038 / OEP-21): mark the legacy slashless - Enrollment v2 addresses ``deprecated: true``. - - ADR 0038 rule 6 requires the trailing slash on every conforming route, so - within the migrated v2 surface a path without one is, by construction, a - legacy address whose slashed (or renamed) replacement is mounted beside - it. Scoped to ``/v2/`` so the marking tracks this migration — deprecating - v1 is its own DEPR decision. Paths appear here after - SCHEMA_PATH_PREFIX_TRIM strips /api/enrollment. + 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. Paths are post-SCHEMA_PATH_PREFIX_TRIM. """ for path, path_item in result.get("paths", {}).items(): if not path.startswith("/v2/") or path.endswith("/"): diff --git a/lms/urls.py b/lms/urls.py index 173f1d409baa..5b6fdefa47b1 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -54,8 +54,7 @@ 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 (ADR 0038): registered once per service, -# before any URL pattern that uses / . +# Shared opaque-key path converters, registered before any pattern using them. register_url_converters() RESET_COURSE_DEADLINES_NAME = 'reset_course_deadlines' diff --git a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py index 675553fc8c66..fe54ce2dd69f 100644 --- a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py +++ b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py @@ -297,11 +297,7 @@ def test_minimal_view_collapses_course_details_to_course_id(self, mock_list, moc @skip_unless_lms class TestEnrollmentUrlStructure(APITestCase): - """ - ADR 0038 — conforming trailing-slash routes with snake_case URL names, - mounted beside the legacy slashless routes, which keep their names and - serve the same views. - """ + """Conforming trailing-slash routes with snake_case names, beside the legacy slashless ones.""" USERNAME = "someone" COURSE_ID = "course-v1:org+course+run" diff --git a/openedx/core/djangoapps/enrollments/v2/urls.py b/openedx/core/djangoapps/enrollments/v2/urls.py index 174369c153c8..24729b9e0b45 100644 --- a/openedx/core/djangoapps/enrollments/v2/urls.py +++ b/openedx/core/djangoapps/enrollments/v2/urls.py @@ -3,26 +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()``. - -ADR 0038 — the API name and version position already conform. The conforming -routes below fix the remaining rule 6 violations (a required trailing slash; -no optional-slash patterns) and rule 11 violations (``snake_case``, -version-free, unique URL names), and are dual-mounted (OEP-21) beside the -legacy slashless routes, which keep their original names and are marked -``deprecated: true`` in the OpenAPI schema (``lms/lib/spectacular.py``). -Conforming member routes live under the plural ``enrollments/`` and -``courses/`` collections (rule 2), with course keys resolved by the shared -``course_key`` converter (rule 9), which rejects deprecated ``Org/Course/Run`` -keys. Deeper ADR 0038 targets — collapsing the singular ``enrollment/`` -collection into ``enrollments/``, replacing ``unenroll`` (a verb, rule 10) -with ``DELETE`` on the member address, and addressing the requesting user as -``me`` — are contract changes and belong to a future v3 per ADR 0037. +Conforming routes (ADR 0038) 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 ----------- @@ -35,7 +20,7 @@ POST /enrollment/enrollment_allowed/ DELETE /enrollment/enrollment_allowed/ -Conforming explicit paths (ADR 0038): +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) @@ -67,8 +52,7 @@ urlpatterns = [ *router.urls, - # -- Conforming routes (ADR 0038: required trailing slash, plural - # -- collections, snake_case version-free names, shared key converter). + # Conforming routes (ADR 0038). path( "enrollments/", EnrollmentsAdminListView.as_view(), @@ -85,11 +69,9 @@ name="course_enrollment_detail", ), path("roles/", UserRolesView.as_view(), name="user_roles"), - # -- Legacy routes (OEP-21 deprecation window; ADR 0038 rule 6 - # -- violations frozen as-is, marked deprecated in the OpenAPI schema). - # -- The admin list's optional-slash pattern is narrowed to slashless - # -- only: the slashed address is now served by the conforming route - # -- above, so every address that resolved before still resolves. + # Legacy routes, kept for their OEP-21 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(), @@ -105,10 +87,8 @@ re_path( rf"^enrollment/{settings.COURSE_ID_PATTERN}$", EnrollmentRetrieveView.as_view(), - # Previously this route shared the name ``enrollment-v2-retrieve`` - # with the composite-key form above, resolving only because Django - # disambiguates by argument signature (the fragility ADR 0038 rule 11 - # calls out). Nothing reverses it, so it gets its own name. + # 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( diff --git a/openedx/core/djangoapps/enrollments/v2/views.py b/openedx/core/djangoapps/enrollments/v2/views.py index aa55fbfd6ef1..180089e89bee 100644 --- a/openedx/core/djangoapps/enrollments/v2/views.py +++ b/openedx/core/djangoapps/enrollments/v2/views.py @@ -468,10 +468,8 @@ 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). """ - # ADR 0038: the conforming /enrollments/{username},{course_key}/ - # route passes a parsed CourseKey (shared ``course_key`` converter); - # the legacy slashless routes pass the raw string. Coerce to the - # string form the body below expects. + # 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) @@ -617,9 +615,8 @@ def get(self, request, course_id=None): course schedule and supported enrollment modes; pass ``?include_expired=1`` to include expired enrollment modes. """ - # ADR 0038: the conforming /courses/{course_key}/ route passes a - # parsed CourseKey (shared ``course_key`` converter); the legacy - # slashless /course/{course_key} route passes the raw string. + # 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: From 230799b8b900282140cb86b720b784e40e6fcc9a Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft <139064778+Abdul-Muqadim-Arbisoft@users.noreply.github.com> Date: Mon, 14 Sep 2026 21:18:07 +0500 Subject: [PATCH 09/10] chore: drop ADR and OEP names from the code this branch adds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #39037: the code should follow the standards without naming the documents. The three authoring urls.py modules now read "Authoring API vN URLs.", the URL-structure test banners and the Enrollment v2 module docstring lose their rule references, and the remaining comments say what the code does instead — deprecation window rather than OEP-21 window, "whose course_key path converter hands views a parsed key" rather than a rule number. Only lines this branch adds are touched; the pre-existing ADR references elsewhere in these files are left alone, the one exception being the ADR 0028 line in the Enrollment v2 module docstring, which this branch was already rewriting. Prose only apart from one assert message in the new URL-structure test, which now reads "missing error-envelope field". ruff passes. --- cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py | 2 +- .../rest_api/v1/views/tests/test_xblock_viewset.py | 2 +- cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py | 2 +- .../contentstore/rest_api/v3/tests/test_home.py | 4 ++-- cms/djangoapps/contentstore/rest_api/v3/utils.py | 4 ++-- .../rest_api/v3/views/tests/test_course_details.py | 2 +- cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py | 2 +- cms/lib/spectacular.py | 4 ++-- cms/urls.py | 4 ++-- .../core/djangoapps/enrollments/v2/tests/test_views.py | 4 ++-- openedx/core/djangoapps/enrollments/v2/urls.py | 8 ++++---- 11 files changed, 19 insertions(+), 19 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py index 17be6d10a3ca..2b085422cc06 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -1,4 +1,4 @@ -"""Authoring API v1 URLs (ADR 0038 conforming mount for Contentstore v1).""" +"""Authoring API v1 URLs.""" from django.urls import path 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 e212bf016932..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 @@ -219,7 +219,7 @@ def test_minimal_view_is_noop_for_non_json_payload(self, mock_retrieve): # --------------------------------------------------------------------------- -# ADR 0038 — URL-structure tests +# URL-structure tests # --------------------------------------------------------------------------- diff --git a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py index 56faf73c811c..2d1db8f88434 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py @@ -1,4 +1,4 @@ -"""Authoring API v3 URLs (ADR 0038 conforming mount for Contentstore v3).""" +"""Authoring API v3 URLs.""" from django.urls import path 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 e48ae26ac47a..4e6c83e5a0c5 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py @@ -106,8 +106,8 @@ def test_conforming_and_legacy_routes_share_view(self): 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 — ADR 0029 envelope included.""" + """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"ADR 0029: missing field '{field}'" + 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 4fed4e44f7d8..bbc3ef235e64 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/utils.py +++ b/cms/djangoapps/contentstore/rest_api/v3/utils.py @@ -34,8 +34,8 @@ def resolve_course_key(course_key: str | CourseKey) -> CourseKey: 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 — - ADR 0038 rule 9 — hands views a parsed key). + ``/api/authoring/v3/`` routes, whose ``course_key`` path converter + hands views a parsed key). Raises: rest_framework.exceptions.NotFound: if the string is unparseable 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 c57b9123456b..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 @@ -381,7 +381,7 @@ def test_fields_csv_restricts_top_level_keys( # --------------------------------------------------------------------------- -# ADR 0038 — URL-structure tests +# URL-structure tests # --------------------------------------------------------------------------- class TestCourseDetailsViewSetUrlStructure(APITestCase): """The conforming courses/{course_key}/details/ route serves the same view as the legacy one.""" diff --git a/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py index 27e3ec45533a..0f8c8b3641b8 100644 --- a/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py @@ -1,4 +1,4 @@ -"""Authoring API v4 URLs (ADR 0038 conforming mount for Contentstore v4).""" +"""Authoring API v4 URLs.""" from django.urls import path diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index f8b04efac2d6..eb1175dd7adb 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -3,7 +3,7 @@ import re # Legacy addresses of APIs migrated to /api/authoring/, marked deprecated for -# their OEP-21 window. Paths are post-SCHEMA_PATH_PREFIX_TRIM. +# their deprecation window. Paths are post-SCHEMA_PATH_PREFIX_TRIM. LEGACY_MIGRATED_PATH_PREFIXES = ( "/v1/xblock/", # → /api/authoring/v1/xblocks/ "/v3/home/", # → /api/authoring/v3/home/ @@ -43,7 +43,7 @@ def cms_api_filter(endpoints): def cms_mark_migrated_paths(result, generator, request, public): # pylint: disable=unused-argument """ - Post-processing hook (ADR 0038 / OEP-21): mark the legacy addresses of + 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(): diff --git a/cms/urls.py b/cms/urls.py index c52f52659155..9fa980c977e2 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -360,8 +360,8 @@ path('api/contentstore/', include('cms.djangoapps.contentstore.rest_api.urls')) ] -# Authoring REST APIs — conforming addresses (ADR 0038), dual-mounted beside -# their legacy /api/contentstore/ routes for the OEP-21 deprecation window. +# 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')), diff --git a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py index fe54ce2dd69f..684f59ed991a 100644 --- a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py +++ b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py @@ -214,7 +214,7 @@ def setUp(self): super().setUp() self.user = UserFactory.create(password="test") # Renamed from the versioned kebab-case ``enrollment-v2-roles`` - # (ADR 0038; the path is unchanged). + # (the path is unchanged). self.url = reverse("v2:user_roles") @patch("openedx.core.djangoapps.enrollments.v2.views.api.get_user_roles", return_value=[]) @@ -292,7 +292,7 @@ def test_minimal_view_collapses_course_details_to_course_id(self, mock_list, moc # --------------------------------------------------------------------------- -# ADR 0038 — URL-structure tests +# URL-structure tests # --------------------------------------------------------------------------- @skip_unless_lms diff --git a/openedx/core/djangoapps/enrollments/v2/urls.py b/openedx/core/djangoapps/enrollments/v2/urls.py index 24729b9e0b45..1706082633f2 100644 --- a/openedx/core/djangoapps/enrollments/v2/urls.py +++ b/openedx/core/djangoapps/enrollments/v2/urls.py @@ -3,7 +3,7 @@ Mounted at ``/api/enrollment/v2/`` (see ``lms/urls.py``). -Conforming routes (ADR 0038) are dual-mounted beside the legacy slashless +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 @@ -26,7 +26,7 @@ GET /courses/{course_key}/ (name: course_enrollment_detail) GET /roles/ (name: user_roles) -Legacy paths (deprecated, kept for their OEP-21 window): +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-own) GET /enrollments (name: enrollment-v2-admin-list) @@ -52,7 +52,7 @@ urlpatterns = [ *router.urls, - # Conforming routes (ADR 0038). + # Conforming routes. path( "enrollments/", EnrollmentsAdminListView.as_view(), @@ -69,7 +69,7 @@ name="course_enrollment_detail", ), path("roles/", UserRolesView.as_view(), name="user_roles"), - # Legacy routes, kept for their OEP-21 window. The admin list's + # 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( From 608009797da92e8440b7bea0df36ea8317435cef Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft <139064778+Abdul-Muqadim-Arbisoft@users.noreply.github.com> Date: Fri, 18 Sep 2026 14:17:38 +0500 Subject: [PATCH 10/10] fix: emit full paths in the OpenAPI schemas instead of trimming the prefix SCHEMA_PATH_PREFIX_TRIM is a boolean that we had set to a string in three places; it only worked because the string was truthy. More importantly the trimming itself produced wrong URLs: the LMS spec advertised /v2/enrollments/ against a bare LMS_ROOT_URL server, and adding /api/authoring/ to the CMS spec left those paths untrimmed beside trimmed contentstore ones. Widening the prefix regex would collapse /api/contentstore/v3/home/ and /api/authoring/v3/home/ onto one key. Drop the trim, drop the CMS-contentstore server that existed only to compensate, keep SCHEMA_PATH_PREFIX for tag extraction, and update both post-processing hooks to full paths. Adds tests for the hooks, which had none. --- cms/envs/devstack.py | 7 ++-- cms/envs/production.py | 7 ++-- cms/lib/spectacular.py | 14 +++---- cms/lib/tests/__init__.py | 0 cms/lib/tests/test_spectacular.py | 66 +++++++++++++++++++++++++++++++ lms/envs/common.py | 2 +- lms/lib/spectacular.py | 6 +-- lms/lib/tests/test_spectacular.py | 52 ++++++++++++++++++++++++ 8 files changed, 135 insertions(+), 19 deletions(-) create mode 100644 cms/lib/tests/__init__.py create mode 100644 cms/lib/tests/test_spectacular.py create mode 100644 lms/lib/tests/test_spectacular.py diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index 595e04884974..6db9599d0c79 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -363,12 +363,11 @@ def should_show_debug_toolbar(request): # pylint: disable=missing-function-docs 'drf_spectacular.hooks.postprocess_schema_enums', 'cms.lib.spectacular.cms_mark_migrated_paths', ], - # 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', + # 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 c3bd9d94b8d9..a3e9edd27106 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -423,13 +423,12 @@ def get_env_setting(setting): 'drf_spectacular.hooks.postprocess_schema_enums', 'cms.lib.spectacular.cms_mark_migrated_paths', ], - # 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', + # 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 eb1175dd7adb..20103add181c 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -3,19 +3,19 @@ import re # Legacy addresses of APIs migrated to /api/authoring/, marked deprecated for -# their deprecation window. Paths are post-SCHEMA_PATH_PREFIX_TRIM. +# their deprecation window. LEGACY_MIGRATED_PATH_PREFIXES = ( - "/v1/xblock/", # → /api/authoring/v1/xblocks/ - "/v3/home/", # → /api/authoring/v3/home/ - "/v3/course_details/", # → /api/authoring/v3/courses/{course_key}/details/ - "/v3/authoring_grading/", # → /api/authoring/v3/courses/{course_key}/grading/ - "/v4/home/courses/", # → /api/authoring/v4/courses/ + "/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 = ( - "/v3/home/", + "/api/contentstore/v3/home/", "/api/authoring/v3/home/", ) 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/lms/envs/common.py b/lms/envs/common.py index 8bc7a258be8a..69411c261372 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2168,8 +2168,8 @@ '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 aea3f8c4dd0b..f228d54374d9 100644 --- a/lms/lib/spectacular.py +++ b/lms/lib/spectacular.py @@ -21,11 +21,11 @@ def lms_mark_legacy_paths_deprecated(result, generator, request, public): # pyl """ 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. Paths are post-SCHEMA_PATH_PREFIX_TRIM. + 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("/v2/") or path.endswith("/"): + if not path.startswith("/api/enrollment/v2/") or path.endswith("/"): continue for operation in path_item.values(): if isinstance(operation, dict): 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/"]