diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/__init__.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/__init__.py index 26403f734775..ca07d9461aea 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/serializers/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/__init__.py @@ -26,3 +26,9 @@ VideoUploadSerializer, # noqa: F401 VideoUsageSerializer, # noqa: F401 ) +from .youtube_transcripts import ( # noqa: F401 + YoutubeTranscriptCheckRequestSerializer, + YoutubeTranscriptCheckSerializer, + YoutubeTranscriptUploadRequestSerializer, + YoutubeTranscriptUploadSerializer, +) diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/youtube_transcripts.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/youtube_transcripts.py new file mode 100644 index 000000000000..e10b12fb9e82 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/youtube_transcripts.py @@ -0,0 +1,93 @@ +""" +Serializers for the YouTube transcripts resource (v1 — ADR 0025). + +These mirror the v0 ``YoutubeTranscriptCheckSerializer`` / +``YoutubeTranscriptUploadSerializer`` field shapes exactly (response parity is +required — see the survey), but are actually used to serialize the DRF +``Response`` body in v1, instead of being declared-but-unused as they are in +v0. +""" +from rest_framework import serializers + +from cms.djangoapps.contentstore.rest_api.serializers.common import StrictSerializer + + +class YoutubeTranscriptCheckRequestSerializer(serializers.Serializer): + """ + Validates the ``data`` query-parameter JSON payload the legacy + ``check_transcripts()`` function expects (see + ``transcripts_ajax.py:518`` — read from ``request.GET['data']`` + regardless of HTTP verb). The view parses and validates this payload + through this serializer before calling the legacy function, rather than + leaving it as an opaque, unvalidated query string. + """ + locator = serializers.CharField(help_text="Usage key string identifying the video xblock.") + videos = serializers.ListField( + child=serializers.DictField(), + help_text=( + "List of video descriptors, e.g. " + "[{'type': 'youtube', 'video': 'abc123', 'mode': 'youtube'}, " + "{'type': 'html5', 'video': 'vid1', 'mode': 'mp4'}]." + ), + ) + + +class YoutubeTranscriptUploadRequestSerializer(serializers.Serializer): + """ + Validates the ``data`` query-parameter JSON payload the legacy + ``replace_transcripts()`` function expects. Same shape as the check + request; ``videos`` must include a ``youtube`` entry for the + upload/replace operation to succeed (enforced by the underlying legacy + function). + """ + locator = serializers.CharField(help_text="Usage key string identifying the video xblock.") + videos = serializers.ListField( + child=serializers.DictField(), + help_text=( + "List of video descriptors; must include a " + "{'type': 'youtube', 'video': '', 'mode': 'youtube'} entry." + ), + ) + + +class YoutubeTranscriptCheckSerializer(StrictSerializer): + """ + Strict serializer for the YouTube transcripts check response (v1). + + Field-for-field identical to the v0 declaration (response parity per the + survey) — this is now actually used to serialize the view's output + instead of being declared but unused. + """ + html5_local = serializers.ListField(child=serializers.CharField()) + html5_equal = serializers.BooleanField() + is_youtube_mode = serializers.BooleanField() + youtube_local = serializers.BooleanField() + youtube_server = serializers.BooleanField() + youtube_diff = serializers.BooleanField() + # `transcripts_ajax.py:387` sets this to `item.sub`, a plain string field + # on the video block (not a list) — the v0 declaration as `ListField` was + # wrong and would 400 any real response where `sub` is set. Corrected + # here to match actual legacy behavior (response parity), not a + # business-logic change. + current_item_subs = serializers.CharField(required=False, allow_null=True) + status = serializers.CharField() + command = serializers.CharField() + + +class YoutubeTranscriptUploadSerializer(StrictSerializer): + """ + Strict serializer for the YouTube transcripts upload response (v1). + + Field-for-field identical to the v0 declaration (response parity per the + survey), except ``edx_video_id`` is corrected to ``allow_null=True``: + ``transcripts_ajax.py:706-721,748`` shows ``replace_transcripts`` returns + ``{'edx_video_id': None, 'status': 'Success'}`` for a video hosted in a + V2 content library (``LibraryLocatorV2``) — the YouTube download, + transcript save, and ``video.save()`` have already completed + successfully by the time that response is built. A non-nullable + ``CharField`` would reject that already-committed success as a 400. This + is a serializer correction to match actual legacy behavior, not a + business-logic change. + """ + edx_video_id = serializers.CharField(allow_null=True) + status = serializers.CharField() diff --git a/cms/djangoapps/contentstore/rest_api/v1/tests/__init__.py b/cms/djangoapps/contentstore/rest_api/v1/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/rest_api/v1/tests/test_youtube_transcripts.py b/cms/djangoapps/contentstore/rest_api/v1/tests/test_youtube_transcripts.py new file mode 100644 index 000000000000..50668bb58584 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/tests/test_youtube_transcripts.py @@ -0,0 +1,281 @@ +""" +REST-level tests for the v1 YouTube transcripts endpoints +(``YoutubeTranscriptsViewSet``). + +These test the *view* — routing, auth/permission enforcement, and the +legacy-``JsonResponse``-to-DRF-``Response`` wrapping — by mocking the +``check_transcripts`` / ``replace_transcripts`` legacy functions at their +import location in the view module (the same pattern ``test_xblock.py`` uses +for ``handle_xblock``). The underlying business logic (YouTube API calls, +VAL, modulestore) is already covered by +``cms/djangoapps/contentstore/views/tests/test_transcripts.py`` and is not +re-tested here. + +``AuthorizeStaffTestCase`` contributes four inherited tests, not two: +``test_student`` / ``test_instructor_in_another_course`` (expect 403, and run +against the real unmocked view since the permission decorator rejects before +the legacy function is ever reached) plus ``test_global_staff`` / +``test_course_instructor`` (expect 200). The latter two call +``self.make_request()`` with no arguments, so — following the precedent in +``rest_api/v0/tests/test_xblock.py`` (``XBlockViewTestCase.make_request`` is +itself decorated with ``@patch(..., return_value=...)``) — ``make_request`` +here is decorated with a default-success ``@patch`` on the legacy function so +those inherited 200-expecting tests pass without a real, unmocked round-trip +through modulestore/VAL/YouTube-API code (impractical at this view-level +layer, and already out of scope per the module docstring above). Tests that +need a different mocked return value use their own ``@patch`` on a +differently-named test method instead of relying on the shared default. +""" +import json +from unittest.mock import patch +from urllib.parse import urlencode + +from django.http import Http404 +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APITestCase + +from cms.djangoapps.contentstore.tests.test_utils import AuthorizeStaffTestCase +from common.djangoapps.util.json_request import JsonResponse +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + +VERSION = "v1" + +_SUCCESS_CHECK_BODY = { + "html5_local": [], + "html5_equal": False, + "is_youtube_mode": True, + "youtube_local": True, + "youtube_server": False, + "youtube_diff": True, + "current_item_subs": None, + "status": "Success", + "command": "found", +} + +_SUCCESS_UPLOAD_BODY = { + "edx_video_id": "test-edx-video-id", + "status": "Success", +} + +_REQUEST_DATA = { + "locator": "block-v1:edX+DemoX+Demo_Course+type@video+block@abcd", + "videos": [{"type": "youtube", "video": "JMD_ifUUfsU", "mode": "youtube"}], +} + + +class YoutubeTranscriptsViewSetTestBase(AuthorizeStaffTestCase, ModuleStoreTestCase, APITestCase): + """Shared setup for the check/upload view tests.""" + + def get_url(self, url_name, course_id=None): + return reverse( + f"cms.djangoapps.contentstore:{VERSION}:{url_name}", + kwargs={"course_id": course_id or self.get_course_key_string()}, + ) + + def login_as_instructor(self): + self.client.login(username=self.course_instructor.username, password=self.password) + + +class YoutubeTranscriptCheckViewTest(YoutubeTranscriptsViewSetTestBase): + """Tests for GET .../youtube_transcripts/{course_id}/check/""" + + @patch( + "cms.djangoapps.contentstore.rest_api.v1.views.youtube_transcripts.check_transcripts", + return_value=JsonResponse(_SUCCESS_CHECK_BODY, 200), + ) + def make_request(self, mock_check_transcripts=None, course_id=None): # pylint: disable=arguments-differ + """ + Issue the GET request with the legacy ``check_transcripts`` mocked to + a default 200/success response. + + Decorating ``make_request`` itself (rather than each test method) + mirrors ``XBlockViewTestCase.make_request`` in + ``rest_api/v0/tests/test_xblock.py`` — it is what lets the + ``AuthorizeStaffTestCase``-inherited ``test_global_staff`` / + ``test_course_instructor`` (which call ``self.make_request()`` with + no arguments and expect 200) pass without a real, unmocked + modulestore/VAL/YouTube-API round trip. + """ + url = self.get_url("youtube_transcripts_check", course_id=course_id) + return self.client.get(url, {"data": json.dumps(_REQUEST_DATA)}) + + def test_check_success(self): + """Authenticated course author gets a 200 with the expected response shape.""" + self.login_as_instructor() + response = self.make_request() # pylint: disable=no-value-for-parameter + + assert response.status_code == status.HTTP_200_OK + assert response.json() == _SUCCESS_CHECK_BODY + + @patch( + "cms.djangoapps.contentstore.rest_api.v1.views.youtube_transcripts.check_transcripts", + return_value=JsonResponse({"status": "Incoming video data is empty."}, 400), + ) + def test_check_validation_error(self, mock_check_transcripts): + """A 400 from the legacy function is surfaced as a standardized 400.""" + self.login_as_instructor() + url = self.get_url("youtube_transcripts_check") + response = self.client.get(url, {"data": json.dumps(_REQUEST_DATA)}) + + mock_check_transcripts.assert_called_once() + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_check_rejects_malformed_data_param(self): + """ + Malformed ``data`` (not JSON, or JSON that fails the request + serializer) is rejected with a 400 before the legacy function is + ever called — this is the real ADR 0025 input-validation layer added + in front of ``check_transcripts``. + """ + self.login_as_instructor() + url = self.get_url("youtube_transcripts_check") + + response = self.client.get(url, {"data": "not-json"}) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + response = self.client.get(url, {"data": json.dumps({"locator": "abc"})}) # missing `videos` + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_check_requires_authentication(self): + """An unauthenticated request is rejected.""" + response = self.client.get( + self.get_url("youtube_transcripts_check"), {"data": json.dumps(_REQUEST_DATA)} + ) + assert response.status_code in (status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN) + + # test_student and test_instructor_in_another_course (both expecting 403) + # are inherited from AuthorizeStaffTestCase and exercise `make_request()` + # above directly against the real (unmocked) permission decorator — the + # `@course_author_access_required` permission check runs and rejects + # before the legacy function is ever called, so the class-level + # `check_transcripts` mock on `make_request` is never reached for those + # two. test_global_staff and test_course_instructor (both expecting 200) + # are also inherited and rely on that same mock to succeed. + + @patch( + "cms.djangoapps.contentstore.rest_api.v1.views.youtube_transcripts.check_transcripts", + side_effect=Http404("No item found for the given locator."), + ) + def test_check_not_found_for_missing_item(self, mock_check_transcripts): + """ + A course author hits a real 404 when the referenced video item does + not exist. ``check_transcripts`` itself never raises ``Http404`` today + (validation failures become a 400 ``status`` field instead — see the + module docstring's ADR 0029 note), so this exercises the view's + ``StandardizedErrorMixin`` ``Http404`` -> DRF 404 path in case a + future revision of the legacy function (or a library-content path + through ``_get_item``) raises it directly. This test documents a + path that cannot currently occur through the real legacy function; + it is kept as forward-looking coverage of the view's own exception + handling rather than removed. + """ + self.login_as_instructor() + url = self.get_url("youtube_transcripts_check") + response = self.client.get(url, {"data": json.dumps(_REQUEST_DATA)}) + + mock_check_transcripts.assert_called_once() + assert response.status_code == status.HTTP_404_NOT_FOUND + + +class YoutubeTranscriptUploadViewTest(YoutubeTranscriptsViewSetTestBase): + """Tests for POST .../youtube_transcripts/{course_id}/upload/""" + + @patch( + "cms.djangoapps.contentstore.rest_api.v1.views.youtube_transcripts.replace_transcripts", + return_value=JsonResponse(_SUCCESS_UPLOAD_BODY, 200), + ) + def make_request(self, mock_replace_transcripts=None, course_id=None): # pylint: disable=arguments-differ + """ + Issue the POST request with the legacy ``replace_transcripts`` + mocked to a default 200/success response. See + ``YoutubeTranscriptCheckViewTest.make_request`` above for why + ``make_request`` itself carries the ``@patch``. + + ``data`` is sent as a query string parameter (``?data=...``), not a + POST body field: the legacy function only ever reads + ``request.GET['data']`` regardless of HTTP verb, and the view's + ``@extend_schema`` now accurately documents ``data`` as a query + parameter rather than a JSON request body (see the view's ADR 0025 + finding-3 fix). Sending it as a POST body field instead (as an + earlier version of this test did) would put it in + ``request.POST``/``request.data``, which ``request.GET`` never sees + — that earlier test only passed because the mocked + ``replace_transcripts`` never actually looked at the request body. + """ + url = self.get_url("youtube_transcripts_upload", course_id=course_id) + return self.client.post(f"{url}?{urlencode({'data': json.dumps(_REQUEST_DATA)})}") + + def test_upload_success(self): + """Authenticated course author gets a 200 with the expected response shape.""" + self.login_as_instructor() + response = self.make_request() # pylint: disable=no-value-for-parameter + + assert response.status_code == status.HTTP_200_OK + assert response.json() == _SUCCESS_UPLOAD_BODY + + @patch( + "cms.djangoapps.contentstore.rest_api.v1.views.youtube_transcripts.replace_transcripts", + return_value=JsonResponse({"status": "YouTube ID is required."}, 400), + ) + def test_upload_validation_error(self, mock_replace_transcripts): + """A 400 from the legacy function is surfaced as a standardized 400.""" + self.login_as_instructor() + url = self.get_url("youtube_transcripts_upload") + response = self.client.post(f"{url}?{urlencode({'data': json.dumps(_REQUEST_DATA)})}") + + mock_replace_transcripts.assert_called_once() + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_upload_rejects_malformed_data_param(self): + """ + Malformed ``data`` is rejected with a 400 before + ``replace_transcripts`` (and its YouTube download / VAL write / + modulestore write) is ever called. + """ + self.login_as_instructor() + url = self.get_url("youtube_transcripts_upload") + + response = self.client.post(f"{url}?{urlencode({'data': 'not-json'})}") + assert response.status_code == status.HTTP_400_BAD_REQUEST + + response = self.client.post(f"{url}?{urlencode({'data': json.dumps({'locator': 'abc'})})}") # missing `videos` + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_upload_requires_authentication(self): + """An unauthenticated request is rejected.""" + url = self.get_url("youtube_transcripts_upload") + response = self.client.post(f"{url}?{urlencode({'data': json.dumps(_REQUEST_DATA)})}") + assert response.status_code in (status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN) + + # test_student and test_instructor_in_another_course (both expecting 403) + # and test_global_staff / test_course_instructor (both expecting 200) are + # inherited from AuthorizeStaffTestCase — see the note in + # YoutubeTranscriptCheckViewTest above; the same reasoning applies here + # against the class-level `replace_transcripts` mock on `make_request`. + + @patch( + "cms.djangoapps.contentstore.rest_api.v1.views.youtube_transcripts.replace_transcripts", + side_effect=Http404("No item found for the given locator."), + ) + def test_upload_not_found_for_missing_item(self, mock_replace_transcripts): + """ + A course author hits a real 404 when the referenced video item does + not exist. As with ``check``, ``replace_transcripts`` never actually + raises ``Http404`` today — this is forward-looking coverage of the + view's own exception handling, kept intentionally (see the note on + ``test_check_not_found_for_missing_item`` above). + """ + self.login_as_instructor() + url = self.get_url("youtube_transcripts_upload") + response = self.client.post(f"{url}?{urlencode({'data': json.dumps(_REQUEST_DATA)})}") + + mock_replace_transcripts.assert_called_once() + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_upload_rejects_get(self): + """GET is no longer accepted on the upload endpoint (ADR 0030 fix).""" + self.login_as_instructor() + url = self.get_url("youtube_transcripts_upload") + response = self.client.get(url, {"data": json.dumps(_REQUEST_DATA)}) + assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED diff --git a/cms/djangoapps/contentstore/rest_api/v1/urls.py b/cms/djangoapps/contentstore/rest_api/v1/urls.py index 7929b7f69a98..f8541a742b80 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/urls.py @@ -28,6 +28,7 @@ VideoDownloadView, VideoUsageView, XblockViewSet, + YoutubeTranscriptsViewSet, vertical_container_children_redirect_view, ) @@ -145,6 +146,22 @@ name="course_waffle_flags" ), + # YouTube transcripts (ADR 0038-conformant: mandatory trailing slash, no + # optional-`?` regex anti-pattern like the v0 equivalent). Registered as + # explicit routes (rather than DefaultRouter dynamic `@action` discovery) + # to keep the URL shape unambiguous and independently reviewable; the + # view itself remains a standard DRF ViewSet per ADR 0028. + re_path( + fr'^youtube_transcripts/{COURSE_ID_PATTERN}/check/$', + YoutubeTranscriptsViewSet.as_view({'get': 'check'}), + name="youtube_transcripts_check" + ), + re_path( + fr'^youtube_transcripts/{COURSE_ID_PATTERN}/upload/$', + YoutubeTranscriptsViewSet.as_view({'post': 'upload'}), + name="youtube_transcripts_upload" + ), + # Authoring API # Do not use under v1 yet (Nov. 23). The Authoring API is still experimental and the v0 versions should be used ] diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py index b413f6705c64..d9f47e63dca5 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py @@ -17,3 +17,4 @@ from .vertical_block import ContainerHandlerView, vertical_container_children_redirect_view # noqa: F401 from .videos import CourseVideosView, VideoDownloadView, VideoUsageView # noqa: F401 from .xblock import XblockViewSet # noqa: F401 +from .youtube_transcripts import YoutubeTranscriptsViewSet # noqa: F401 diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/youtube_transcripts.py b/cms/djangoapps/contentstore/rest_api/v1/views/youtube_transcripts.py new file mode 100644 index 000000000000..84136258e2d4 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/youtube_transcripts.py @@ -0,0 +1,436 @@ +""" +API Views for YouTube transcript check/upload — v1. + +Standardizes the v0 ``YoutubeTranscriptCheckView`` + ``YoutubeTranscriptUploadView`` +pair (``cms/djangoapps/contentstore/rest_api/v0/views/transcripts.py``) into a +single ``YoutubeTranscriptsViewSet`` applying the FC-0118 ADRs. The v0 views, +serializers, and URLs are untouched (ADR 0037) — this is a new, additive v1 +surface for the same resource. Only the two YouTube transcript endpoints are +migrated here; the sibling ``TranscriptView`` (``/video_transcripts/...``) is +a different resource and is out of scope for this change. + +ADR compliance: + * ADR 0025 - ``serializer_class`` is declared as a class attribute (the + ADR 0025 checklist requirement — schema generation and any + ``getattr(view, 'serializer_class')`` caller depend on it existing even + when a view overrides per-action selection), and additionally + per-action via ``get_serializer_class`` (the two actions have different + response shapes), plus a ``get_serializer`` helper since plain + ``viewsets.ViewSet`` has none. Response bodies are now actually built + through ``YoutubeTranscriptCheckSerializer`` / ``YoutubeTranscriptUploadSerializer`` + instead of being declared-but-unused as in v0. Request bodies are also now + actually validated: both actions parse the ``data`` query parameter + themselves and run it through ``YoutubeTranscriptCheckRequestSerializer`` / + ``YoutubeTranscriptUploadRequestSerializer`` via ``is_valid(raise_exception=True)`` + *before* calling the legacy function, so a malformed ``data`` payload now + gets a real structured 400 instead of only ever validating a response + body. (A prior version of this view declared the request serializers only + inside ``@extend_schema`` and never instantiated them — that was a real + ADR 0025 violation, caught in review; see the ``check``/``upload`` + docstrings for the fix and reasoning.) + * ADR 0026 - explicit ``authentication_classes`` + ``permission_classes`` + declared on the viewset (no reliance on project defaults). + * ADR 0027 - ``drf_spectacular`` ``@extend_schema`` on both actions + (v0 had no schema annotation at all). + * ADR 0028 - both endpoints act on the same resource (a course's YouTube + transcript state) and neither is a real ORM-backed model, so this is a + plain ``viewsets.ViewSet`` (not ``ModelViewSet``), with ``check`` and + ``upload`` as its two action methods. Routing is wired via explicit + ``re_path`` entries calling ``YoutubeTranscriptsViewSet.as_view({'get': + 'check'})`` / ``as_view({'post': 'upload'})`` in ``v1/urls.py`` rather + than ``DefaultRouter`` dynamic ``@action`` discovery — the course_id path + parameter here is not a router "detail" lookup on this resource's own + identity (the viewset has no ``list``/``retrieve``/collection of its + own), and explicit registration keeps the URL shape unambiguous and + independently reviewable while the view class itself remains a standard + DRF ``ViewSet``, satisfying the ADR's "migrate away from ad-hoc + ``APIView``/legacy dispatch" intent. Query-count discipline: both actions + delegate to the existing ``check_transcripts`` / ``replace_transcripts`` + legacy functions unchanged, so this migration introduces no new N+1s. No + ``select_related``/``prefetch_related`` opportunity exists here — the + data path is modulestore/contentstore/VAL/YouTube-API calls, not a + Django ORM queryset, so that MUST doesn't apply to this resource (see + Enrollment v2's ``viewsets.ViewSet`` precedent for a non-ORM data path). + * ADR 0029 - ``StandardizedErrorMixin`` provides the standardized error + envelope. The legacy functions return a raw ``JsonResponse`` with a + ``status`` field carrying the error message on failure (not the DRF + standard ``developer_message`` shape) — this view parses that JsonResponse + and re-raises as a DRF ``ValidationError`` so error responses go through + the standardized envelope. This is a deliberate shape change on the error + path only; the success-path response body is unchanged (see docstring on + each action). The ``ValidationError`` is raised with the *full* legacy + error body merged with an explicit ``error_code`` key (e.g. + ``youtube_transcript_check_failed`` / ``youtube_transcript_upload_failed``) + — an earlier version of this view raised ``ValidationError`` with only + the truncated ``status`` string, silently discarding the rest of the + legacy ``transcripts_presence`` dict (``html5_local``, ``youtube_diff``, + etc., which are also present on the error path since ``error_response()`` + only overwrites the ``status`` key on the full dict) and had no + ``error_code`` at all, inconsistent with the 403 path's + ``error_code='user_permissions'``. Fixed here as a low-risk change (it + only affects what is included in an already-thrown exception's detail). + * ADR 0030 - ``check`` remains ``GET`` (already idempotent - read-only + status probe, no writes). ``upload`` is changed from ``GET`` to ``POST``: + the v0 endpoint used GET for an operation that downloads transcripts from + YouTube and writes to VAL + modulestore, which is exactly the GET-mutates + violation this ADR targets. v1 fixes it: the operation is now a POST. The + ``/upload`` URL segment is kept (arguably a verb - see ADR 0038 note + below) because this is a real, non-resource-shaped operation ("perform an + upload/replace"), not a CRUD create of an addressable sub-resource; POST + to a stable noun path is the ADR 0038 rule 10 escape hatch for exactly + this case, and it must be flagged as such in the OpenAPI description, + which is done below. + * ADR 0031 - considered merging check (read) and upload (write) into one + action selected by a ``mode``/``action`` field, per the ADR's merge test: + "share the same resource domain and differ only in the operation + applied". Decision: kept as **two separate actions** on one viewset + rather than fused into a single endpoint. Reasoning: the ADR's merge + target is endpoints that differ only in *operation* on an otherwise + identical request/response contract (e.g. generate/regenerate/toggle a + certificate, all POST, all returning a task handle). Check and upload + differ in HTTP semantics (GET vs POST), side effects (none vs + YouTube-download + VAL-write + modulestore-write), and response shape + (``html5_local``/``youtube_diff``/... vs ``edx_video_id``/``status``) - + merging them behind one ``mode`` field would force a GET-shaped read and + a POST-shaped write through one verb-agnostic entry point, which is a + worse fit than the ADR's own certificate-task example (three POSTs that + already shared one shape). They *do* still get the boilerplate-sharing + benefit of ADR 0031 by living on the same ``YoutubeTranscriptsViewSet`` + with one class-level ``authentication_classes``/``permission_classes`` + declaration, without forcing an artificial shared contract. Both actions + keep their own coarse (``@course_author_access_required`` on the URL's + ``course_id``) plus specific (``has_course_author_access`` inside the + legacy ``_get_item`` against the *item's actual* course_key - relevant + for library content) permission layers, per the ADR's "do not flatten + authorization" requirement. + * ADR 0032 - out of scope. Neither action returns a list/collection. + * ADR 0033 - out of scope. Neither action takes filter/sort parameters. + * ADR 0034 - already compliant. ``authentication_classes`` is + ``(JwtAuthentication, SessionAuthenticationAllowInactiveUser)`` - no + ``BearerAuthentication``/``BearerAuthenticationAllowInactiveUser`` to + remove (v0 carried ``BearerAuthenticationAllowInactiveUser`` via + ``@view_auth_classes()``; that is dropped here per the deprecation + policy). ``SessionAuthenticationAllowInactiveUser`` is kept explicitly so + inactive Studio authors can still reach the endpoint. + * ADR 0035 - out of scope. Not an MFE configuration endpoint. + * ADR 0036 - out of scope. Both response bodies are flat, small, fixed-key + objects (9 and 2 top-level fields respectively) with no nested + sub-objects or tree shape to collapse. + * ADR 0037 - this is a new v1 surface. The v0 + ``YoutubeTranscriptCheckView``/``YoutubeTranscriptUploadView``, their + serializers, and their URL entries are untouched and continue to serve + ``GET`` on the old paths exactly as before. + * ADR 0038 - URLs are + ``/api/contentstore/v1/youtube_transcripts/{course_id}/check/`` (GET) and + ``/api/contentstore/v1/youtube_transcripts/{course_id}/upload/`` (POST), + replacing v0's ``.../youtube_transcripts/{course_id}/check?`` (optional + trailing ``?`` regex anti-pattern flagged by the survey). One level of + nesting (course -> check|upload) is used because neither ``check`` nor + ``upload`` is an independently addressable resource with its own opaque + key - both only make sense scoped to a course, satisfying rule 8. Trailing + slash is now mandatory (rule 6). ``check``/``upload`` are technically verb + segments (rule 10); they are kept because this is the ADR-0031-considered + "genuine non-resource operation" case the rule explicitly carves out, and + each is marked as such via its ``@extend_schema`` description. ``api_name`` + stays ``contentstore`` here (not renamed to ``authoring``) to stay + consistent with the rest of this same v1 mount (``xblock``, etc.) - a + platform-wide ``contentstore`` -> ``authoring`` rename is a bigger, + cross-viewset migration out of scope for this issue. +""" +import json +import logging + +from django.http import JsonResponse +from drf_spectacular.utils import OpenApiParameter, OpenApiResponse, extend_schema +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from edx_rest_framework_extensions.mixins import StandardizedErrorMixin +from rest_framework import viewsets +from rest_framework.exceptions import ValidationError +from rest_framework.permissions import IsAuthenticated +from rest_framework.request import Request +from rest_framework.response import Response + +from cms.djangoapps.contentstore.api.views.utils import course_author_access_required +from cms.djangoapps.contentstore.rest_api.v1.serializers.youtube_transcripts import ( + YoutubeTranscriptCheckRequestSerializer, + YoutubeTranscriptCheckSerializer, + YoutubeTranscriptUploadRequestSerializer, + YoutubeTranscriptUploadSerializer, +) +from cms.djangoapps.contentstore.views.transcripts_ajax import check_transcripts, replace_transcripts + +log = logging.getLogger(__name__) + + +_COURSE_ID_PARAMETER = OpenApiParameter( + name="course_id", + description="Course key string (e.g. course-v1:org+course+run) that owns the video.", + required=True, + type=str, + location=OpenApiParameter.PATH, +) + +_COMMON_ERROR_RESPONSES = { + 401: OpenApiResponse(description="The requester is not authenticated."), + 403: OpenApiResponse(description="The requester does not have course author permissions."), + 404: OpenApiResponse(description="The referenced video item does not exist."), +} + +_DATA_QUERY_PARAMETER_DESCRIPTION = ( + "JSON-encoded object (as a query string value, not a request body — the " + "underlying legacy implementation reads this from the query string " + "regardless of HTTP verb) with keys `locator` (usage key string of the " + "video xblock) and `videos` (list of video descriptors, e.g. " + "[{'type': 'youtube', 'video': 'abc123', 'mode': 'youtube'}])." +) + +_DATA_QUERY_PARAMETER = OpenApiParameter( + name="data", + description=_DATA_QUERY_PARAMETER_DESCRIPTION, + required=True, + type=str, + location=OpenApiParameter.QUERY, +) + + +def _parse_legacy_json_response(response: JsonResponse) -> tuple[dict, int]: + """ + Parse a legacy ``JsonResponse`` (returned by ``check_transcripts`` / + ``replace_transcripts``) into a ``(body_dict, status_code)`` pair. + + Both legacy functions return a raw ``JsonResponse`` rather than a DRF + ``Response`` — this is the seam where that gets bridged into the + standardized DRF response path. + """ + body = json.loads(response.content.decode("utf-8") or "{}") + return body, response.status_code + + +def _validate_incoming_data(request: Request, request_serializer_class) -> None: + """ + Parse and validate the ``data`` query parameter both legacy functions + read (``check_transcripts`` / ``replace_transcripts`` both call + ``request.GET.get('data', '{}')`` internally, regardless of HTTP verb — + see ``transcripts_ajax.py:518,559``). + + This is the real ADR 0025 input-validation layer: it runs *before* the + legacy function so a malformed ``data`` payload gets a structured 400 + (with field-level errors) before any legacy side effects (modulestore + reads, YouTube API calls, VAL/modulestore writes) run. It is + intentionally a validation layer bolted on in front of the legacy + function, not a replacement for it — the legacy function still parses + and validates the same query param again internally. That duplication + is accepted here: it is not a behavior change, and reimplementing + ``check_transcripts``/``replace_transcripts``'s own parsing to avoid it + is out of scope (touching legacy business logic is explicitly what this + migration avoids). + + Malformed JSON in ``data`` itself (not just a schema mismatch once + parsed) is also surfaced as a DRF ``ValidationError`` here, rather than + falling through to the legacy function's own "Incoming video data is + empty."/500 handling of bad JSON. + """ + raw_data = request.GET.get("data", "{}") + try: + parsed = json.loads(raw_data) + except (TypeError, ValueError) as exc: + raise ValidationError({"data": ["Must be a JSON object string."]}) from exc + if not isinstance(parsed, dict): + raise ValidationError({"data": ["Must be a JSON object."]}) + serializer = request_serializer_class(data=parsed) + serializer.is_valid(raise_exception=True) + + +@extend_schema(tags=["openedx-platform-sdk"]) +class YoutubeTranscriptsViewSet(StandardizedErrorMixin, viewsets.ViewSet): + """ + ViewSet for YouTube transcript check/upload operations (v1 — ADR 0028). + + URLs (explicitly registered in ``v1/urls.py``, see module docstring):: + + GET /api/contentstore/v1/youtube_transcripts/{course_id}/check/ → check + POST /api/contentstore/v1/youtube_transcripts/{course_id}/upload/ → upload + + Supersedes the v0 ``YoutubeTranscriptCheckView`` (GET .../check) and + ``YoutubeTranscriptUploadView`` (GET .../upload) — both left untouched at + ``/api/contentstore/v0/youtube_transcripts/{course_id}/...`` (ADR 0037). + + See the module docstring for the full per-ADR compliance summary, + including the ADR 0031 merge analysis (kept as two actions, not merged) + and the ADR 0030 GET→POST fix for ``upload``. + """ + + authentication_classes = ( + JwtAuthentication, + SessionAuthenticationAllowInactiveUser, + ) + permission_classes = (IsAuthenticated,) + + # ADR 0025: a plain ``viewsets.ViewSet`` has no ``serializer_class`` + # machinery of its own. This class attribute is the declared default (the + # ADR 0025 checklist item — schema generation and any + # ``getattr(view, 'serializer_class')`` caller depend on it existing), + # while ``get_serializer_class`` below overrides it per-action since + # ``check`` and ``upload`` have different response shapes. + serializer_class = YoutubeTranscriptCheckSerializer + + def get_serializer_class(self): + """Return the response serializer for the current action.""" + if self.action == "upload": + return YoutubeTranscriptUploadSerializer + return YoutubeTranscriptCheckSerializer + + def get_serializer(self, *args, **kwargs): + """Instantiate and return the action-appropriate serializer class.""" + return self.get_serializer_class()(*args, **kwargs) + + @extend_schema( + summary="Check YouTube transcript availability", + description=( + "Read-only status check: reports whether local/HTML5 and YouTube " + "transcripts exist and whether they differ, for the video " + "identified in the `data` query parameter. Performs no writes. " + "`check` is a verb segment, not a resource CRUD noun — it is the " + "ADR 0038 rule 10 'genuine non-resource operation' exception, " + "same as `upload` below." + ), + parameters=[_COURSE_ID_PARAMETER, _DATA_QUERY_PARAMETER], + responses={ + 200: OpenApiResponse( + response=YoutubeTranscriptCheckSerializer, + description="Transcript presence/status report.", + ), + 400: OpenApiResponse(description="The `data` query parameter is missing or invalid."), + **_COMMON_ERROR_RESPONSES, + }, + ) + @course_author_access_required + def check(self, request: Request, course_key): + """ + Get the status of YouTube transcripts for a given video. + + **Example Request** + + GET /api/contentstore/v1/youtube_transcripts/{course_id}/check/?data=%7B...%7D + + Coarse permission check: ``@course_author_access_required`` verifies + the caller has author access to ``course_id`` (the URL-level course). + A specific permission check also runs inside the legacy + ``check_transcripts`` → ``_get_item`` call, against the *actual* + course_key of the referenced item (relevant when the item lives in a + content library rather than the URL's course) — both checks are + preserved per ADR 0031. + + Input validation (ADR 0025): the ``data`` query parameter (the only + way the legacy function accepts input — see + ``transcripts_ajax.py:518``) is parsed as JSON and validated through + ``YoutubeTranscriptCheckRequestSerializer`` *before* + ``check_transcripts`` is called, so malformed input gets a real 400 + with field errors instead of the legacy "Incoming video data is + empty." string or a 500. This replaces an earlier version of this + view where the request serializer was declared only for + ``@extend_schema`` and never actually run — caught in review as an + ADR 0025 violation, since a client following the generated schema + (which previously advertised a JSON POST body) would get a spurious + 400 either way. The schema above now accurately documents ``data`` as + a query parameter, matching what the legacy function actually reads, + rather than describing an unenforced request body shape. + + Delegates to the existing ``check_transcripts()`` legacy function + unchanged (same modulestore/contentstore/VAL/YouTube-API calls as + v0 — no new queries introduced). Its ``JsonResponse`` is parsed and + re-wrapped: a 200 body is validated and re-serialized through + ``YoutubeTranscriptCheckSerializer`` for a standardized response type + parity with the documented v0 response shape (``html5_local``, + ``html5_equal``, ``is_youtube_mode``, ``youtube_local``, + ``youtube_server``, ``youtube_diff``, ``current_item_subs``, + ``status``, ``command``); a non-2xx body is raised as a DRF + ``ValidationError`` carrying the *full* legacy body (not just the + truncated ``status`` message — the legacy body's other keys, e.g. + ``html5_local``/``youtube_diff``, are also present and were + previously discarded) plus an explicit ``error_code`` key, so it + flows through ``StandardizedErrorMixin`` instead of the legacy + ad-hoc envelope without losing information (ADR 0029). + """ + _validate_incoming_data(request, YoutubeTranscriptCheckRequestSerializer) + legacy_response = check_transcripts(request) + body, status_code = _parse_legacy_json_response(legacy_response) + if status_code >= 400: + raise ValidationError({**body, "error_code": "youtube_transcript_check_failed"}) + serializer = self.get_serializer(data=body) + serializer.is_valid(raise_exception=True) + return Response(serializer.validated_data, status=status_code) + + @extend_schema( + summary="Upload/replace YouTube transcripts", + description=( + "Downloads the transcript from YouTube for the video identified " + "in the `data` query parameter and replaces the existing edX " + "transcript(s) in VAL / modulestore with it. This is a write " + "operation — changed from GET (v0) to POST in v1 per ADR 0030 " + "(GET must be idempotent). The ``upload`` path segment names a " + "genuine non-resource action rather than an addressable CRUD " + "sub-resource (ADR 0038 rule 10 exception)." + ), + parameters=[_COURSE_ID_PARAMETER, _DATA_QUERY_PARAMETER], + responses={ + 200: OpenApiResponse( + response=YoutubeTranscriptUploadSerializer, + description="The transcript was downloaded from YouTube and saved.", + ), + 400: OpenApiResponse(description="The `data` query parameter is missing or invalid."), + **_COMMON_ERROR_RESPONSES, + }, + ) + @course_author_access_required + def upload(self, request: Request, course_key): + """ + Download the YouTube transcript for a video and replace the existing + edX transcript(s) with it. + + **Example Request** + + POST /api/contentstore/v1/youtube_transcripts/{course_id}/upload/?data=%7B...%7D + + Coarse permission check: ``@course_author_access_required`` verifies + the caller has author access to ``course_id``. A specific permission + check also runs inside the legacy ``replace_transcripts`` → + ``_get_item`` call against the item's actual course_key — both + checks are preserved per ADR 0031. + + Input validation (ADR 0025): same as ``check`` above — the ``data`` + query parameter is parsed as JSON and validated through + ``YoutubeTranscriptUploadRequestSerializer`` *before* + ``replace_transcripts`` is called (and before any YouTube + download / VAL write / modulestore write can happen), so a malformed + payload never reaches the legacy function's side effects. + + Delegates to the existing ``replace_transcripts()`` legacy function + unchanged (same YouTube download + VAL write + modulestore save as + v0). Its ``JsonResponse`` is parsed and re-wrapped the same way as + ``check`` above, preserving the documented v0 response shape + (``edx_video_id``, ``status``) on success. + + Note: the legacy function reads its payload from + ``request.GET['data']`` (a query parameter), not from the POST body, + regardless of HTTP verb. This is unchanged here to avoid touching + ``replace_transcripts``'s parsing/business logic (ADR 0037-style + caution against modifying legacy behavior mid-migration) — the fix + for the "schema says POST body, only query param actually works" + defect flagged in review is on the *validation* side (see above): + the ``@extend_schema`` `request=` body declaration was removed in + favor of an honest ``data`` ``OpenApiParameter`` (query), and that + same ``data`` param is now what's actually validated up front. A + client must still pass ``data`` as a query parameter (e.g. + ``POST .../upload/?data=%7B...%7D``) — that is not a workaround, it + is what is now accurately documented. + """ + _validate_incoming_data(request, YoutubeTranscriptUploadRequestSerializer) + legacy_response = replace_transcripts(request) + body, status_code = _parse_legacy_json_response(legacy_response) + if status_code >= 400: + raise ValidationError({**body, "error_code": "youtube_transcript_upload_failed"}) + serializer = self.get_serializer(data=body) + serializer.is_valid(raise_exception=True) + return Response(serializer.validated_data, status=status_code)