From 05360ae1d913aa9714242c6cd94acf4857a27293 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Mon, 14 Sep 2026 17:36:53 +0500 Subject: [PATCH 01/15] feat: standardize Course Videos API into authoring v1 Adds a conforming REST surface for a course's video assets at /api/authoring/v1/courses/{course_key}/videos/ and .../{edx_video_id}/, as one ViewSet with list, retrieve, create and destroy. The legacy GET at /api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id} declares a video id and ignores it, returning the whole course listing. The new version splits that in two: the collection endpoint succeeds the behaviour the legacy route really has, and the member endpoint finally honours the id. Three defects are fixed on the new addresses only, because fixing them in place would break anyone parsing the current responses: - The legacy GET and HEAD write. They flip every video stuck in 'upload' for more than 24 hours to 'upload_failed'. The new read path computes the same display status without the write; the legacy route still reconciles. - Three incompatible error shapes, including a 400 with an empty body whose payload rides in the HTTP reason phrase, become one error envelope. - A non-JSON Accept header returns a redirect to the authoring MFE, or a 500 when COURSE_AUTHORING_MICROFRONTEND_URL is unset, which is the platform default. The new addresses are JSON only. Business logic is reused by importing the existing video_storage_handlers and edxval.api functions; none of it is duplicated or edited. Authentication drops to the platform default, which refuses inactive users on the new addresses while the legacy addresses are unchanged. Addresses under a course that no endpoint serves are answered with the error envelope rather than the site's HTML error page, so a client can read them. Part of #39060. --- .../rest_api/v1/authoring_urls.py | 47 + .../contentstore/rest_api/v1/error_types.py | 30 + .../rest_api/v1/serializers/video_uploads.py | 204 ++ .../rest_api/v1/video_uploads_service.py | 292 +++ .../v1/views/tests/test_video_uploads.py | 1738 +++++++++++++++++ .../rest_api/v1/views/unknown_route.py | 26 + .../rest_api/v1/views/video_uploads.py | 399 ++++ cms/urls.py | 7 + 8 files changed, 2743 insertions(+) create mode 100644 cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/error_types.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/serializers/video_uploads.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/video_uploads.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..a3cb45753f7c --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -0,0 +1,47 @@ +"""Authoring API v1 URLs.""" + +from django.urls import path + +from cms.djangoapps.contentstore.rest_api.v1.error_types import register_request_error_types +from cms.djangoapps.contentstore.rest_api.v1.views.unknown_route import UnknownRouteView +from cms.djangoapps.contentstore.rest_api.v1.views.video_uploads import CourseVideoUploadsViewSet + +app_name = "authoring_v1" + +register_request_error_types() + +urlpatterns = [ + path( + "courses//videos/", + CourseVideoUploadsViewSet.as_view({"get": "list", "post": "create"}), + name="course_video_list", + ), + path( + "courses//videos//", + CourseVideoUploadsViewSet.as_view({"get": "retrieve", "delete": "destroy"}), + name="course_video_detail", + ), + # The three routes below stand behind the two above and are reached only by a + # request the ones above turned down: a course key the converter refuses + # because it is malformed or in the deprecated slash-separated form, or an + # address under a course that this API does not serve. They answer with the + # API error body, which a client can read, instead of the site's HTML error + # page. Order is what keeps them out of the way of real requests, and each + # accepts only addresses ending in a slash so that the redirect to the + # slash-terminated form keeps working. + path( + "courses//videos/", + UnknownRouteView.as_view(), + name="course_video_list_unmatched", + ), + path( + "courses//videos//", + UnknownRouteView.as_view(), + name="course_video_detail_unmatched", + ), + path( + "courses//", + UnknownRouteView.as_view(), + name="course_unmatched", + ), +] diff --git a/cms/djangoapps/contentstore/rest_api/v1/error_types.py b/cms/djangoapps/contentstore/rest_api/v1/error_types.py new file mode 100644 index 000000000000..91e5d0b24d39 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/error_types.py @@ -0,0 +1,30 @@ +"""Error-type catalog entries for request-level refusals.""" + +from edx_rest_framework_extensions.errors import register_error_type +from rest_framework.exceptions import ( + MethodNotAllowed, + NotAcceptable, + ParseError, + UnsupportedMediaType, +) + +#: Refusals of the request itself - its body, its method, its media type - with +#: the error type and title each is answered with. +_REQUEST_ERROR_TYPES = ( + (ParseError, "validation", "Malformed Request"), + (MethodNotAllowed, "method-not-allowed", "Method Not Allowed"), + (NotAcceptable, "not-acceptable", "Not Acceptable"), + (UnsupportedMediaType, "unsupported-media-type", "Unsupported Media Type"), +) + + +def register_request_error_types(): + """ + Give each request-level refusal its own error type. + + Uncataloged errors are answered with the type and title of an internal + server error while keeping their 4xx status, which tells a client that its + own unparseable body or unsupported method was a fault of the server. + """ + for exception_class, slug, title in _REQUEST_ERROR_TYPES: + register_error_type(exception_class, slug, title) diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/video_uploads.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/video_uploads.py new file mode 100644 index 000000000000..81f6fbd72348 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/video_uploads.py @@ -0,0 +1,204 @@ +"""API Serializers for course video uploads.""" + +from edx_rest_framework_extensions.shaping import MinimalViewMixin +from rest_framework import serializers +from rest_framework.settings import api_settings + +from cms.djangoapps.contentstore.rest_api.serializers.common import StrictSerializer +from cms.djangoapps.contentstore.rest_api.v1.video_uploads_service import VIDEO_ORDERING_CHOICES +from cms.djangoapps.contentstore.video_storage_handlers import VIDEO_SUPPORTED_FILE_FORMATS + +SUPPORTED_UPLOAD_CONTENT_TYPES = sorted(set(VIDEO_SUPPORTED_FILE_FORMATS.values())) + +NON_FIELD_ERRORS_KEY = api_settings.NON_FIELD_ERRORS_KEY + +#: The fields one video is reduced to under the minimal response preset. +MINIMAL_VIDEO_FIELDS = ("edx_video_id", "client_video_id", "status", "created", "duration") + + +def _flat_messages(detail, path=""): + """ + Return validation ``detail`` as a map of field path to messages. + + Errors reported for the entries of a list of objects are nested one or two + levels below the field they belong to. Flattening them to + ``{"files[0].content_type": ["..."]}`` gives a client the message of every + invalid field without walking the containers it is reported inside. + """ + if isinstance(detail, dict): + flat = {} + for field, value in detail.items(): + nested = path if field == NON_FIELD_ERRORS_KEY and path else _join(path, field) + flat.update(_flat_messages(value, nested)) + return flat + if isinstance(detail, list) and any(isinstance(entry, (dict, list)) for entry in detail): + flat = {} + for index, entry in enumerate(detail): + if entry: + flat.update(_flat_messages(entry, f"{path}[{index}]")) + return flat + messages = detail if isinstance(detail, list) else [detail] + return {path or NON_FIELD_ERRORS_KEY: [str(message) for message in messages]} + + +def _join(path, field): + """Return the path of ``field`` inside ``path``.""" + return f"{path}.{field}" if path else str(field) + + +class VideoUploadFileSerializer(StrictSerializer): + """One requested upload slot: the file the caller intends to PUT.""" + + file_name = serializers.CharField( + help_text="Name of the video file being uploaded. ASCII characters only.", + ) + content_type = serializers.ChoiceField( + choices=SUPPORTED_UPLOAD_CONTENT_TYPES, + help_text="MIME type of the video file. Only these types can be stored.", + ) + + def validate_file_name(self, value): + """Reject names the storage backend cannot carry in its object metadata.""" + try: + value.encode("ascii") + except UnicodeEncodeError as error: + raise serializers.ValidationError( + "The file name must contain only ASCII characters." + ) from error + return value + + +class VideoUploadRequestSerializer(StrictSerializer): + """Request body for creating upload slots.""" + + files = VideoUploadFileSerializer( + many=True, + allow_empty=False, + help_text="Files to create upload slots for. One slot is returned per entry, in order.", + ) + + def run_validation(self, data=serializers.empty): + """Validate the body, reporting every invalid field as a list of messages.""" + try: + return super().run_validation(data) + except serializers.ValidationError as error: + raise serializers.ValidationError(_flat_messages(error.detail)) from error + + +class VideoUploadLinkSerializer(serializers.Serializer): # pylint: disable=abstract-method + """One created upload slot.""" + + file_name = serializers.CharField( + help_text="Name of the video file, echoed from the request.", + ) + upload_url = serializers.CharField( + help_text="Short-lived pre-signed URL to PUT the video file to. Expires after 24 hours.", + ) + edx_video_id = serializers.CharField( + help_text="Identifier assigned to the video. Use it to address the video afterwards.", + ) + + +class VideoUploadResponseSerializer(serializers.Serializer): # pylint: disable=abstract-method + """Response body for creating upload slots.""" + + files = VideoUploadLinkSerializer( + many=True, + help_text="Created upload slots, one per requested file, in request order.", + ) + + +class CourseVideoSerializer(serializers.Serializer): # pylint: disable=abstract-method + """One video asset attached to a course.""" + + edx_video_id = serializers.CharField( + help_text="Identifier of the video.", + ) + client_video_id = serializers.CharField( + allow_blank=True, + help_text="Original file name the video was uploaded under.", + ) + created = serializers.DateTimeField( + allow_null=True, + help_text="Time the video record was created.", + ) + duration = serializers.FloatField( + allow_null=True, + help_text="Length of the video in seconds; 0 until the video has been processed.", + ) + status = serializers.CharField( + help_text=( + "Processing state of the video, in stable English: for example Uploading, " + "In Progress, Ready, Failed, YouTube Duplicate." + ), + ) + error_description = serializers.CharField( + allow_null=True, + allow_blank=True, + help_text="Details of the processing failure, when the video failed to process.", + ) + course_video_image_url = serializers.CharField( + allow_null=True, + help_text="Thumbnail image URL for this video in this course, or null when none is set.", + ) + download_link = serializers.CharField( + allow_blank=True, + help_text="URL of the desktop MP4 encoding, or an empty string when it is not ready.", + ) + file_size = serializers.IntegerField( + help_text="Size of the desktop MP4 encoding in bytes; 0 when it is not ready.", + ) + transcripts = serializers.ListField( + child=serializers.CharField(), + help_text="Language codes the video has transcripts for.", + ) + transcription_status = serializers.CharField( + allow_blank=True, + help_text=( + "Transcription state for courses on the current video workflow, in stable English; " + "empty for courses still using a course video upload token." + ), + ) + transcript_urls = serializers.DictField( + child=serializers.CharField(), + help_text="Transcript download URL keyed by language code.", + ) + + +class CourseVideoMinimalSerializer(CourseVideoSerializer): + """One video asset, reduced to the fields that identify it.""" + + def get_fields(self): + """Return only the fields the minimal preset keeps.""" + fields = super().get_fields() + return {name: field for name, field in fields.items() if name in MINIMAL_VIDEO_FIELDS} + + +class CourseVideoQuerySerializer(serializers.Serializer): # pylint: disable=abstract-method + """Query parameters accepted by every course video address.""" + + view = serializers.ChoiceField( + choices=[MinimalViewMixin.minimal_view_value], + required=False, + help_text="Response preset to return. Omit it for the full representation.", + ) + + +class CourseVideoListQuerySerializer(CourseVideoQuerySerializer): + """Query parameters accepted by the course video collection.""" + + ordering = serializers.ChoiceField( + choices=VIDEO_ORDERING_CHOICES, + required=False, + help_text="Field to sort by. Prefix the field name with '-' to sort descending.", + ) + page = serializers.IntegerField( + required=False, + min_value=1, + help_text="Number of the page to return.", + ) + page_size = serializers.IntegerField( + required=False, + min_value=1, + help_text="Number of videos per page; values above the maximum are reduced to it.", + ) diff --git a/cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py b/cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py new file mode 100644 index 000000000000..7001a435cdef --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py @@ -0,0 +1,292 @@ +"""Service layer for the course video uploads resource.""" + +import logging +from datetime import datetime, timedelta + +from django.core.exceptions import ObjectDoesNotExist +from django.core.exceptions import PermissionDenied as DjangoPermissionDenied +from django.http import Http404 +from edx_rest_framework_extensions.errors import register_error_type +from edxval.api import ( + SortDirection, + VideoSortField, + get_available_transcript_languages, + get_course_videos_qset, + get_video_info, + get_video_transcript_url, + get_videos_for_course, + remove_video_for_course, +) +from edxval.exceptions import ValVideoNotFoundError +from pytz import UTC +from rest_framework.exceptions import ( + APIException, + NotFound, + PermissionDenied, + ValidationError, +) +from rest_framework.status import ( + HTTP_200_OK, + HTTP_400_BAD_REQUEST, + HTTP_403_FORBIDDEN, + HTTP_404_NOT_FOUND, +) + +from cms.djangoapps.contentstore.toggles import use_mock_video_uploads +from cms.djangoapps.contentstore.video_storage_handlers import ( + MAX_UPLOAD_HOURS, + StatusDisplayStrings, + _get_and_validate_course, + videos_post, +) + +log = logging.getLogger(__name__) + +#: Error type of a course whose video uploads are switched off or unconfigured. +VIDEO_UPLOADS_NOT_CONFIGURED_SLUG = "videos/uploads-not-configured" + +#: Sort fields the video store can order by, ascending and descending. +VIDEO_ORDERING_CHOICES = tuple( + sorted( + [field.value for field in VideoSortField] + + [f"-{field.value}" for field in VideoSortField] + ) +) +DEFAULT_ORDERING = "-created" + +#: VAL statuses that mean transcription has started, which only happens once +#: every encoding is complete on the current video workflow. +_TRANSCRIPTION_STATUSES = frozenset({ + "transcription_in_progress", + "transcript_ready", + "partial_failure", + "transcript_failed", +}) + + +class VideoUploadsNotConfigured(NotFound): + """The course cannot serve video uploads because the upload pipeline is off or unconfigured.""" + + default_detail = "Video uploads are not configured for this course." + default_code = "video_uploads_not_configured" + + +register_error_type( + VideoUploadsNotConfigured, + VIDEO_UPLOADS_NOT_CONFIGURED_SLUG, + "Video Uploads Not Configured", +) + + +class _JsonBody: + """ + Carrier for a parsed JSON body. + + ``videos_post`` reads the request's already-parsed JSON and nothing else, so + it is handed this instead of the live request, which keeps the request + object out of the storage layer. + """ + + def __init__(self, json_body): + self.json = json_body + + +def get_course_for_uploads(course_key, user): + """ + Return the course a video upload operation applies to. + + Enforces the caller's Studio access to the course and the course's upload + pipeline configuration; a course the caller may not read is refused, and a + course that does not accept uploads is reported as missing. Returns + ``None`` only when uploads are mocked for local development, in which case + no real course is needed. + """ + try: + course = _get_and_validate_course(str(course_key), user) + except DjangoPermissionDenied as error: + log.info("Course [%s] is not readable in Studio by user [%s]", course_key, user.id) + raise PermissionDenied() from error + except Http404 as error: + log.info("No course [%s] to serve video uploads for", course_key) + raise NotFound() from error + if course is None and not use_mock_video_uploads(): + raise VideoUploadsNotConfigured() + return course + + +def _require_course(course): + """Return ``course``, or refuse a read that has no course behind it.""" + if course is None: + raise VideoUploadsNotConfigured() + return course + + +def _parse_ordering(ordering): + """Split an ordering value into the sort field and direction the video store expects.""" + ordering = ordering or DEFAULT_ORDERING + descending = ordering.startswith("-") + return ( + VideoSortField(ordering.lstrip("-")), + SortDirection.desc if descending else SortDirection.asc, + ) + + +def list_course_videos(course, ordering=DEFAULT_ORDERING): + """ + Return every non-hidden video attached to ``course``, in the requested order. + + Rows are returned unenriched: the per-video transcript lookups happen in + :func:`enrich_course_videos`, so a caller that only shows one page pays for + one page. + """ + sort_field, sort_direction = _parse_ordering(ordering) + videos, __ = get_videos_for_course(str(_require_course(course).id), sort_field, sort_direction) + return list(videos) + + +def get_course_video(course, edx_video_id): + """Return the one video attached to ``course`` under ``edx_video_id``, unenriched.""" + course = _require_course(course) + attached = get_course_videos_qset(course.id).filter(video__edx_video_id=edx_video_id).exists() + if not attached: + raise NotFound() + try: + return get_video_info(edx_video_id) + except ValVideoNotFoundError as error: + log.info("No video record for edx_video_id [%s] in course [%s]", edx_video_id, course.id) + raise NotFound() from error + + +def enrich_course_videos(course, videos): + """ + Return the API representation of ``videos`` for ``course``. + + Each row gains its display status, its course-specific thumbnail, its + downloadable encoding and its transcript languages and URLs. + """ + course = _require_course(course) + course_id = str(course.id) + upload_token = course.video_upload_pipeline.get("course_video_upload_token") + return [_enrich_video(video, course_id, upload_token) for video in videos] + + +def _enrich_video(video, course_id, upload_token): + """Return the API representation of one video row.""" + encodes_ready = not upload_token and video["status"] in _TRANSCRIPTION_STATUSES + status = _display_status(video, encodes_ready) + transcripts = get_available_transcript_languages(video_id=video["edx_video_id"]) + download_link, file_size = _desktop_encoding(video) + return { + "edx_video_id": video["edx_video_id"], + "client_video_id": video["client_video_id"], + "created": video["created"], + "duration": video["duration"], + "status": status, + "error_description": video["error_description"], + "course_video_image_url": _course_video_image_url(video, course_id), + "download_link": download_link, + "file_size": file_size, + "transcripts": transcripts, + "transcription_status": _transcription_status(video, encodes_ready), + "transcript_urls": { + language_code: get_video_transcript_url( + video_id=video["edx_video_id"], + language_code=language_code, + ) + for language_code in transcripts + }, + } + + +def _transcription_status(video, encodes_ready): + """ + Return how far transcription has got for one video. + + Only courses on the current video workflow report it: elsewhere the stored + status never reaches a transcription state. It names the transcription + state itself, which the display status no longer shows once the encodes are + complete. + """ + if not encodes_ready: + return "" + return StatusDisplayStrings.get(video["status"]) + + +def _display_status(video, encodes_ready): + """ + Return the display status of one video. + + A video left in ``upload`` for longer than the upload window is reported as + failed. Reporting it does not reconcile the stored record. + """ + created = video.get("created") + now = datetime.now(created.tzinfo if created else UTC) + if video["status"] == "upload" and created and (now - created) > timedelta(hours=MAX_UPLOAD_HOURS): + return StatusDisplayStrings.get("upload_failed") + if video["status"] == "invalid_token": + return StatusDisplayStrings.get("youtube_duplicate") + if encodes_ready: + return StatusDisplayStrings.get("file_complete") + return StatusDisplayStrings.get(video["status"]) + + +def _course_video_image_url(video, course_id): + """Return the thumbnail URL recorded for this video in this course, or None.""" + for course in video["courses"]: + if course_id in course: + return course[course_id] + return None + + +def _desktop_encoding(video): + """Return the desktop MP4 download URL and file size, or empty values when absent.""" + for encoding in video["encoded_videos"]: + if encoding["profile"] == "desktop_mp4": + return encoding["url"], encoding["file_size"] + return "", 0 + + +#: The message every refused upload request is answered with. The video store's +#: own wording can name internal storage detail, so it is logged instead. +_UPLOAD_REFUSED = "The requested files cannot be uploaded." + +#: How a refusal from the video store is reported, by the status it refused with. +_UPLOAD_REFUSAL_ERRORS = { + HTTP_403_FORBIDDEN: PermissionDenied, + HTTP_404_NOT_FOUND: NotFound, +} + + +def _upload_refusal(status_code): + """Return the error answering a refusal the video store reported with ``status_code``.""" + if status_code == HTTP_400_BAD_REQUEST: + return ValidationError({"files": [_UPLOAD_REFUSED]}) + return _UPLOAD_REFUSAL_ERRORS.get(status_code, APIException)(_UPLOAD_REFUSED) + + +def create_video_uploads(course, files): + """ + Create one upload slot per entry in ``files`` and return them in request order. + + Each slot carries a newly assigned video identifier and a short-lived + pre-signed URL the caller PUTs the file to. + """ + data, status_code = videos_post(course, _JsonBody({"files": files})) + if status_code == HTTP_200_OK: + return data + log.warning( + "Video upload slots refused with status [%s] for course [%s]: %s", + status_code, + getattr(course, "id", None), + data.get("error"), + ) + raise _upload_refusal(status_code) + + +def delete_course_video(course_key, edx_video_id): + """Detach ``edx_video_id`` from the course; the video itself is kept for other courses.""" + try: + remove_video_for_course(str(course_key), edx_video_id) + except ObjectDoesNotExist as error: + log.info("No video [%s] attached to course [%s] to remove", edx_video_id, course_key) + raise NotFound() from error diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py new file mode 100644 index 000000000000..89e654226fbe --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py @@ -0,0 +1,1738 @@ +""" +Unit tests for the course video uploads API. +""" +import json +import os +import subprocess +import sys +import tempfile +from datetime import datetime, timedelta +from pathlib import Path +from unittest.mock import Mock, patch + +import ddt +import pytest +import pytz +from django.core.exceptions import PermissionDenied as DjangoPermissionDenied +from django.http import Http404 +from django.test import TestCase, override_settings +from django.urls import resolve, reverse +from drf_spectacular.generators import SchemaGenerator +from drf_spectacular.settings import patched_settings +from edx_django_utils.cache import TieredCache +from edx_rest_framework_extensions.auth.jwt.tests.utils import generate_jwt +from edx_rest_framework_extensions.testing import assert_error_envelope +from edxval.api import create_profile, create_video +from edxval.models import CourseVideo, EncodedVideo, Video +from opaque_keys.edx.keys import CourseKey +from rest_framework import status +from rest_framework.test import APIClient + +import cms.envs +from cms.djangoapps.contentstore.rest_api.v0.views.authoring_videos import ( + VideosCreateUploadView, + VideosUploadsView, +) +from cms.djangoapps.contentstore.rest_api.v1.serializers.video_uploads import CourseVideoSerializer +from cms.djangoapps.contentstore.rest_api.v1.views.unknown_route import UnknownRouteView +from cms.djangoapps.contentstore.rest_api.v1.views.video_uploads import CourseVideoUploadsViewSet +from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from cms.lib.spectacular import cms_api_filter, cms_mark_superseded_paths +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseLimitedStaffRole, + CourseStaffRole, +) +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.video_pipeline.models import VideoUploadsEnabledByDefault +from xmodule.modulestore.tests.factories import CourseFactory + +LIST_URL_NAME = "authoring_v1:course_video_list" +DETAIL_URL_NAME = "authoring_v1:course_video_detail" +LEGACY_LIST_URL_NAME = "cms.djangoapps.contentstore:v0:cms_api_create_videos_upload" +LEGACY_DETAIL_URL_NAME = "cms.djangoapps.contentstore:v0:cms_api_videos_uploads" + +ENUM_POSTPROCESSING_HOOK = "drf_spectacular.hooks.postprocess_schema_enums" +SUPERSEDED_PATHS_HOOK = "cms.lib.spectacular.cms_mark_superseded_paths" + +# The post-processing hooks the CMS registers. Registering any hook replaces +# drf-spectacular's own list, so the enum hook has to be named again alongside. +REGISTERED_POSTPROCESSING_HOOKS = [ENUM_POSTPROCESSING_HOOK, SUPERSEDED_PATHS_HOOK] + +PRODUCTION_SETTINGS = "cms.envs.production" +DEVSTACK_SETTINGS = "cms.envs.devstack" +SETTINGS_DIRECTORY = Path(cms.envs.__file__).resolve().parent +REPO_ROOT = SETTINGS_DIRECTORY.parents[1] +MOCK_CONFIG = SETTINGS_DIRECTORY / "mock.yml" + +# The schema settings that decide the addresses the document publishes, and the +# subset of them a document generated in-process has to be given. +SCHEMA_SETTING_KEYS = ( + "PREPROCESSING_HOOKS", + "POSTPROCESSING_HOOKS", + "SCHEMA_PATH_PREFIX", + "SCHEMA_PATH_PREFIX_TRIM", + "SERVERS", +) +GENERATION_SETTING_KEYS = SCHEMA_SETTING_KEYS[:-1] +SETTINGS_MARKER = "schema settings: " + + +def schema_settings(module, config_file=MOCK_CONFIG): + """ + Return the schema settings of the deployment settings module ``module``. + + Deployment settings share their mutable defaults with the settings the test + process runs under and would alter them on import, so they are read in a + separate process. + """ + script = ( + "import importlib, json, sys\n" + "values = importlib.import_module(sys.argv[1]).SPECTACULAR_SETTINGS\n" + "print(sys.argv[2] + json.dumps({key: values[key] for key in sys.argv[3:]}))\n" + ) + completed = subprocess.run( + [sys.executable, "-c", script, module, SETTINGS_MARKER, *SCHEMA_SETTING_KEYS], + capture_output=True, + check=True, + cwd=REPO_ROOT, + env={**os.environ, "CMS_CFG": str(config_file), "SERVICE_VARIANT": "cms"}, + text=True, + ) + reported = next( + line for line in completed.stdout.splitlines() if line.startswith(SETTINGS_MARKER) + ) + return json.loads(reported[len(SETTINGS_MARKER):]) + + +VIDEO_UPLOAD_PIPELINE = { + "BUCKET": "test_bucket", + "ROOT_PATH": "test_root", + "CONCURRENT_UPLOAD_LIMIT": 4, + "VEM_S3_BUCKET": "vem_test_bucket", +} + +# Every way one video row of this version may differ from the same row of the +# legacy listing, as the JSON path it shows up at and why it is accepted. The +# row parity tests suppress exactly these paths and assert that each of them +# genuinely occurs, so a row that diverges anywhere else, or that stops +# diverging here, fails. Differences that belong to a whole response rather +# than to a row - the pagination envelope, the created status code, the error +# bodies, the Accept-header redirect and the accepted course key forms - are +# each proven by their own test. +ROW_DIFFERENCES = [ + ( + "status_nontranslated", + "the untranslated companion field is gone; 'status' carries that value", + ), + ( + "created", + "timestamps keep the microsecond precision the video store records, which the legacy " + "encoder truncated to milliseconds", + ), +] + +# Fields that legitimately differ per request and are ignored when diffing. +VOLATILE_FIELDS = {"instance", "next", "previous"} + + +def _normalize(obj, path=""): + """Flatten ``obj`` to a {json_path: value} map, dropping volatile fields.""" + flat = {} + if isinstance(obj, dict): + for key in sorted(obj): + if key in VOLATILE_FIELDS: + continue + flat.update(_normalize(obj[key], f"{path}.{key}" if path else key)) + elif isinstance(obj, list): + for index, item in enumerate(obj): + flat.update(_normalize(item, f"{path}[{index}]")) + else: + flat[path] = obj + return flat + + +def _matches(json_path, declared): + return json_path == declared or json_path.startswith(declared + ".") or json_path.startswith(declared + "[") + + +def _diff(legacy, new, declared=()): + """Return (changed paths, paths not covered by a declared difference).""" + left, right = _normalize(legacy), _normalize(new) + changed = {key for key in set(left) | set(right) if left.get(key, "") != right.get(key, "")} + unexpected = {key for key in changed if not any(_matches(key, path) for path in declared)} + return changed, unexpected + + +@override_settings(ENABLE_VIDEO_UPLOAD_PIPELINE=True, VIDEO_UPLOAD_PIPELINE=VIDEO_UPLOAD_PIPELINE) +class CourseVideoUploadsTestBase(CourseTestCase): + """Fixtures shared by every course video uploads test.""" + + def setUp(self): + super().setUp() + self.course.video_upload_pipeline = {"course_video_upload_token": "test_token"} + self.save_course() + + self.api_client = APIClient() + self.api_client.force_authenticate(user=self.user) + + self.list_url = reverse(LIST_URL_NAME, kwargs={"course_key": self.course.id}) + # Microsecond-bearing, as every stored creation time is, so that the + # precision the two versions publish is compared rather than assumed. + self.created = datetime(2026, 1, 2, 3, 4, 5, 123456, tzinfo=pytz.utc) + + def detail_url(self, edx_video_id, course_key=None): + """Return the member address of one video.""" + return reverse( + DETAIL_URL_NAME, + kwargs={"course_key": course_key or self.course.id, "edx_video_id": edx_video_id}, + ) + + def create_videos(self, count=2, status_value="file_complete", course=None): + """Create ``count`` videos attached to the course and return their ids.""" + course = course or self.course + video_ids = [] + for index in range(count): + edx_video_id = f"video-{status_value}-{index}" + create_video({ + "edx_video_id": edx_video_id, + "client_video_id": f"{edx_video_id}.mp4", + "duration": 42.0 + index, + "status": status_value, + "courses": [str(course.id)], + "encoded_videos": [], + }) + video_ids.append(edx_video_id) + self.set_created(video_ids) + return video_ids + + def set_created(self, video_ids, base=None, age=None): + """ + Pin the creation time of ``video_ids``. + + ``created`` is assigned by the database on insert, so it is rewritten + here to make ordering deterministic and, with ``age``, to age a video + past the upload window. + """ + base = base or self.created + for offset, edx_video_id in enumerate(video_ids): + created = base + timedelta(seconds=offset) + if age: + created = datetime.now(pytz.utc) - age + Video.objects.filter(edx_video_id=edx_video_id).update(created=created) + + def create_course_author(self, role_class, course=None): + """Create a user holding ``role_class`` on the course and return them.""" + user = UserFactory.create(is_staff=False) + role_class((course or self.course).id).add_users(user) + return user + + def client_for(self, user): + """Return an API client authenticated as ``user``.""" + client = APIClient() + if user is not None: + client.force_authenticate(user=user) + return client + + def legacy_detail_url(self, edx_video_id, course=None): + """Return the legacy v0 member address.""" + return reverse( + LEGACY_DETAIL_URL_NAME, + kwargs={"course_id": str((course or self.course).id), "edx_video_id": edx_video_id}, + ) + + def legacy_list_url(self, course=None): + """Return the legacy v0 create address.""" + return reverse(LEGACY_LIST_URL_NAME, kwargs={"course_id": str((course or self.course).id)}) + + +class CurrentWorkflowTestBase(CourseVideoUploadsTestBase): + """ + Fixtures for a course on the current video workflow. + + Such a course holds no course video upload token; video uploads are enabled + for the whole platform instead. This is the only configuration in which a + video reaches a transcription status, so it is the only one in which the + encodes-ready branch of the display status and the transcription status are + reachable at all. + """ + + def setUp(self): + super().setUp() + self.course.video_upload_pipeline = {} + self.save_course() + VideoUploadsEnabledByDefault.objects.create(enabled=True, enabled_for_all_courses=True) + self.addCleanup(TieredCache.delete_all_tiers, VideoUploadsEnabledByDefault.cache_key_name()) + + +def _mock_bucket(mock_boto3_resource, upload_url="http://example.com/put_video"): + """Point ``boto3.resource`` at a bucket whose pre-signed URLs are predictable.""" + mock_s3_client = Mock() + mock_s3_client.generate_presigned_url.return_value = upload_url + mock_bucket = Mock() + mock_bucket.name = "vem_test_bucket" + mock_bucket.meta.client = mock_s3_client + mock_resource = Mock() + mock_resource.Bucket.return_value = mock_bucket + mock_boto3_resource.return_value = mock_resource + return mock_s3_client + + +@ddt.ddt +class CourseVideoListTest(CourseVideoUploadsTestBase): + """Tests for listing a course's videos.""" + + def test_list_returns_pagination_envelope(self): + self.create_videos(count=2) + response = self.api_client.get(self.list_url) + assert response.status_code == status.HTTP_200_OK + assert set(response.data) == { + "count", "num_pages", "current_page", "start", "next", "previous", "results", + } + assert response.data["count"] == 2 + + def test_list_row_shape(self): + self.create_videos(count=1) + response = self.api_client.get(self.list_url) + assert set(response.data["results"][0]) == { + "edx_video_id", + "client_video_id", + "created", + "duration", + "status", + "error_description", + "course_video_image_url", + "download_link", + "file_size", + "transcripts", + "transcription_status", + "transcript_urls", + } + + def test_list_excludes_other_courses(self): + other_course = CourseFactory.create() + other_course.video_upload_pipeline = {"course_video_upload_token": "test_token"} + other_course.save() + self.store.update_item(other_course, self.user.id) + self.create_videos(count=1, status_value="elsewhere", course=other_course) + self.create_videos(count=1) + + response = self.api_client.get(self.list_url) + assert [row["edx_video_id"] for row in response.data["results"]] == ["video-file_complete-0"] + + def test_list_excludes_removed_videos(self): + video_ids = self.create_videos(count=2) + CourseVideo.objects.filter( + course_id=str(self.course.id), video__edx_video_id=video_ids[0] + ).update(is_hidden=True) + + response = self.api_client.get(self.list_url) + assert [row["edx_video_id"] for row in response.data["results"]] == [video_ids[1]] + + def test_list_defaults_to_newest_first(self): + video_ids = self.create_videos(count=3) + response = self.api_client.get(self.list_url) + assert [row["edx_video_id"] for row in response.data["results"]] == list(reversed(video_ids)) + + def test_list_honours_ordering(self): + video_ids = self.create_videos(count=3) + response = self.api_client.get(self.list_url, {"ordering": "created"}) + assert [row["edx_video_id"] for row in response.data["results"]] == video_ids + + def test_list_rejects_unknown_ordering(self): + response = self.api_client.get(self.list_url, {"ordering": "status"}) + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert "ordering" in response.data["errors"] + + def test_list_reduces_an_oversized_page_size_to_the_maximum(self): + self.create_videos(count=101) + response = self.api_client.get(self.list_url, {"page_size": 500}) + assert response.status_code == status.HTTP_200_OK + assert len(response.data["results"]) == 100 + assert response.data["count"] == 101 + assert response.data["num_pages"] == 2 + + def test_list_accepts_the_maximum_page_size(self): + self.create_videos(count=11) + response = self.api_client.get(self.list_url, {"page_size": 100}) + assert response.status_code == status.HTTP_200_OK + assert len(response.data["results"]) == 11 + assert response.data["num_pages"] == 1 + + def test_list_returns_ten_videos_a_page_by_default(self): + self.create_videos(count=11) + response = self.api_client.get(self.list_url) + assert response.status_code == status.HTTP_200_OK + assert len(response.data["results"]) == 10 + assert response.data["count"] == 11 + assert response.data["num_pages"] == 2 + + @ddt.data("abc", "0", "-1") + def test_list_rejects_an_unusable_page(self, page): + response = self.api_client.get(self.list_url, {"page": page}) + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert "page" in response.data["errors"] + + @ddt.data("0", "-1", "abc") + def test_list_rejects_an_unusable_page_size(self, page_size): + response = self.api_client.get(self.list_url, {"page_size": page_size}) + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert "page_size" in response.data["errors"] + + @ddt.data("full", "miniml", "MINIMAL") + def test_list_rejects_an_unknown_view(self, view): + response = self.api_client.get(self.list_url, {"view": view}) + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert "view" in response.data["errors"] + + def test_list_paginates(self): + self.create_videos(count=3) + response = self.api_client.get(self.list_url, {"page": 2, "page_size": 1}) + assert response.status_code == status.HTTP_200_OK + assert response.data["count"] == 3 + assert response.data["num_pages"] == 3 + assert response.data["current_page"] == 2 + assert response.data["start"] == 1 + assert len(response.data["results"]) == 1 + + def test_list_minimal_view_is_opt_in(self): + self.create_videos(count=1) + full = self.api_client.get(self.list_url) + minimal = self.api_client.get(self.list_url, {"view": "minimal"}) + assert set(minimal.data["results"][0]) == { + "edx_video_id", "client_video_id", "status", "created", "duration", + } + assert set(minimal.data["results"][0]) < set(full.data["results"][0]) + + def test_the_serializer_is_built_with_the_request_context(self): + captured = {} + + class ContextCapturingSerializer(CourseVideoSerializer): + """Record the context the view builds its serializers with.""" + + def __init__(self, *args, **kwargs): + captured.update(kwargs.get("context") or {}) + super().__init__(*args, **kwargs) + + self.create_videos(count=1) + with patch.object( + CourseVideoUploadsViewSet, "serializer_class", ContextCapturingSerializer + ): + response = self.api_client.get(self.list_url) + + assert response.status_code == status.HTTP_200_OK + assert captured["request"].path == self.list_url + assert isinstance(captured["view"], CourseVideoUploadsViewSet) + assert "format" in captured + + def test_list_404_when_pipeline_disabled(self): + with override_settings(ENABLE_VIDEO_UPLOAD_PIPELINE=False): + response = self.api_client.get(self.list_url) + assert_error_envelope(response, expected_status=404, expected_type_slug="videos/uploads-not-configured") + + def test_list_404_when_course_not_configured(self): + self.course.video_upload_pipeline = {} + self.save_course() + response = self.api_client.get(self.list_url) + assert_error_envelope(response, expected_status=404, expected_type_slug="videos/uploads-not-configured") + + def test_list_404_for_a_page_past_the_end(self): + self.create_videos(count=1) + response = self.api_client.get(self.list_url, {"page": 5}) + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + def test_list_404_for_unknown_course(self): + unknown = CourseKey.from_string("course-v1:no+such+course") + response = self.api_client.get(reverse(LIST_URL_NAME, kwargs={"course_key": unknown})) + assert_error_envelope(response, expected_status=404) + + +class CourseVideoRetrieveTest(CourseVideoUploadsTestBase): + """Tests for retrieving one of a course's videos.""" + + def test_retrieve_returns_the_addressed_video(self): + video_ids = self.create_videos(count=2) + response = self.api_client.get(self.detail_url(video_ids[0])) + assert response.status_code == status.HTTP_200_OK + assert response.data["edx_video_id"] == video_ids[0] + + def test_retrieve_reports_encoding_and_transcripts(self): + create_profile("desktop_mp4") + create_video({ + "edx_video_id": "encoded", + "client_video_id": "encoded.mp4", + "duration": 12.0, + "status": "file_complete", + "courses": [str(self.course.id)], + "encoded_videos": [{ + "profile": "desktop_mp4", + "url": "http://example.com/encoded.mp4", + "file_size": 1600, + "bitrate": 100, + }], + }) + response = self.api_client.get(self.detail_url("encoded")) + assert response.data["download_link"] == "http://example.com/encoded.mp4" + assert response.data["file_size"] == 1600 + assert response.data["transcripts"] == [] + assert response.data["transcript_urls"] == {} + + def test_retrieve_minimal_view(self): + video_ids = self.create_videos(count=1) + response = self.api_client.get(self.detail_url(video_ids[0]), {"view": "minimal"}) + assert set(response.data) == { + "edx_video_id", "client_video_id", "status", "created", "duration", + } + + def test_retrieve_rejects_an_unknown_view(self): + video_ids = self.create_videos(count=1) + response = self.api_client.get(self.detail_url(video_ids[0]), {"view": "full"}) + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert "view" in response.data["errors"] + + def test_retrieve_404_for_unknown_video(self): + response = self.api_client.get(self.detail_url("no-such-video")) + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + def test_retrieve_404_when_pipeline_disabled(self): + video_ids = self.create_videos(count=1) + with override_settings(ENABLE_VIDEO_UPLOAD_PIPELINE=False): + response = self.api_client.get(self.detail_url(video_ids[0])) + assert_error_envelope(response, expected_status=404, expected_type_slug="videos/uploads-not-configured") + + def test_retrieve_404_for_video_of_another_course(self): + other_course = CourseFactory.create() + other_course.video_upload_pipeline = {"course_video_upload_token": "test_token"} + other_course.save() + self.store.update_item(other_course, self.user.id) + video_ids = self.create_videos(count=1, status_value="elsewhere", course=other_course) + + response = self.api_client.get(self.detail_url(video_ids[0])) + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + def test_retrieve_404_for_removed_video(self): + video_ids = self.create_videos(count=1) + CourseVideo.objects.filter( + course_id=str(self.course.id), video__edx_video_id=video_ids[0] + ).update(is_hidden=True) + response = self.api_client.get(self.detail_url(video_ids[0])) + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + +@ddt.ddt +class CurrentWorkflowStatusTest(CurrentWorkflowTestBase): + """What a course on the current video workflow reports for each stored status.""" + + def row(self, status_value): + """Create one video in ``status_value`` and return its row from the listing.""" + self.create_videos(count=1, status_value=status_value) + response = self.api_client.get(self.list_url) + assert response.status_code == status.HTTP_200_OK + result, = response.data["results"] + return result + + @ddt.data( + ("transcription_in_progress", "Transcription in Progress"), + ("transcript_ready", "Transcript Ready"), + ("partial_failure", "Partial Failure"), + ("transcript_failed", "Transcript Failed"), + ) + @ddt.unpack + def test_transcription_status_names_the_transcription_state(self, status_value, expected): + row = self.row(status_value) + assert row["transcription_status"] == expected + assert row["status"] == "Ready" + + @ddt.data( + ("ingest", "In Progress"), + ("file_complete", "Ready"), + ("upload_completed", "Uploaded"), + ("pipeline_error", "Failed"), + ) + @ddt.unpack + def test_transcription_status_is_empty_before_transcription_starts(self, status_value, expected): + row = self.row(status_value) + assert row["transcription_status"] == "" + assert row["status"] == expected + + def test_an_invalid_token_is_reported_as_a_youtube_duplicate(self): + row = self.row("invalid_token") + assert row["status"] == "YouTube Duplicate" + assert row["transcription_status"] == "" + + def test_the_member_reports_the_same_transcription_state(self): + video_ids = self.create_videos(count=1, status_value="transcript_failed") + response = self.api_client.get(self.detail_url(video_ids[0])) + assert response.data["transcription_status"] == "Transcript Failed" + assert response.data["status"] == "Ready" + + +class UploadTokenStatusTest(CourseVideoUploadsTestBase): + """A course still holding an upload token reports no transcription state.""" + + def test_a_transcription_status_is_reported_as_the_display_status_only(self): + self.create_videos(count=1, status_value="transcription_in_progress") + response = self.api_client.get(self.list_url) + result, = response.data["results"] + assert result["status"] == "Transcription in Progress" + assert result["transcription_status"] == "" + + +@ddt.ddt +class CourseVideoCreateTest(CourseVideoUploadsTestBase): + """Tests for creating upload slots.""" + + @patch("cms.djangoapps.contentstore.video_storage_handlers.boto3.resource") + def test_create_returns_one_slot_per_file(self, mock_boto3_resource): + _mock_bucket(mock_boto3_resource) + response = self.api_client.post( + self.list_url, + {"files": [ + {"file_name": "first.mp4", "content_type": "video/mp4"}, + {"file_name": "second.mov", "content_type": "video/quicktime"}, + ]}, + format="json", + ) + assert response.status_code == status.HTTP_201_CREATED + assert [entry["file_name"] for entry in response.data["files"]] == ["first.mp4", "second.mov"] + for entry in response.data["files"]: + assert set(entry) == {"file_name", "upload_url", "edx_video_id"} + assert entry["upload_url"] == "http://example.com/put_video" + assert Video.objects.filter(edx_video_id=entry["edx_video_id"], status="upload").exists() + assert CourseVideo.objects.filter( + course_id=str(self.course.id), video__edx_video_id=entry["edx_video_id"] + ).exists() + + @patch("cms.djangoapps.contentstore.video_storage_handlers.boto3.resource") + def test_create_presigns_with_the_requested_content_type(self, mock_boto3_resource): + mock_s3_client = _mock_bucket(mock_boto3_resource) + self.api_client.post( + self.list_url, + {"files": [{"file_name": "first.mov", "content_type": "video/quicktime"}]}, + format="json", + ) + params = mock_s3_client.generate_presigned_url.call_args[1]["Params"] + assert params["ContentType"] == "video/quicktime" + assert params["Metadata"]["client_video_id"] == "first.mov" + assert params["Metadata"]["course_key"] == str(self.course.id) + + def test_create_rejects_unsupported_content_type(self): + response = self.api_client.post( + self.list_url, + {"files": [{"file_name": "first.webm", "content_type": "video/webm"}]}, + format="json", + ) + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert response.data["errors"]["files[0].content_type"] == [ + '"video/webm" is not a valid choice.' + ] + + def test_create_names_the_entry_each_message_belongs_to(self): + response = self.api_client.post( + self.list_url, + {"files": [ + {"file_name": "first.mp4", "content_type": "video/mp4"}, + {"content_type": "video/avi"}, + ]}, + format="json", + ) + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert response.data["errors"] == { + "files[1].file_name": ["This field is required."], + "files[1].content_type": ['"video/avi" is not a valid choice.'], + } + + def test_create_rejects_non_ascii_file_name(self): + response = self.api_client.post( + self.list_url, + {"files": [{"file_name": "nón-ascii-näme.mp4", "content_type": "video/mp4"}]}, + format="json", + ) + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert response.data["errors"]["files[0].file_name"] == [ + "The file name must contain only ASCII characters." + ] + + def test_create_rejects_missing_files(self): + response = self.api_client.post(self.list_url, {}, format="json") + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert response.data["errors"]["files"] == ["This field is required."] + + def test_create_rejects_empty_files(self): + response = self.api_client.post(self.list_url, {"files": []}, format="json") + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert response.data["errors"]["files"] == ["This list may not be empty."] + + def test_create_reports_every_message_as_a_string(self): + response = self.api_client.post(self.list_url, {"files": []}, format="json") + errors = response.data["errors"] + assert errors + for messages in errors.values(): + assert all(isinstance(message, str) for message in messages) + assert "ErrorDetail" not in json.dumps(errors) + + def test_create_rejects_unexpected_fields(self): + response = self.api_client.post( + self.list_url, + {"files": [{"file_name": "first.mp4", "content_type": "video/mp4"}], "extra": 1}, + format="json", + ) + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert "extra" in response.data["errors"] + + def post_with_store_refusal(self, refusal_status): + """Create an upload slot against a video store that refuses with ``refusal_status``.""" + with patch( + "cms.djangoapps.contentstore.rest_api.v1.video_uploads_service.videos_post", + return_value=({"error": "internal bucket detail"}, refusal_status), + ): + return self.api_client.post( + self.list_url, + {"files": [{"file_name": "first.mp4", "content_type": "video/mp4"}]}, + format="json", + ) + + def test_create_reports_a_stable_message_when_the_store_refuses(self): + response = self.post_with_store_refusal(400) + assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert "internal bucket detail" not in json.dumps(response.data) + + @ddt.data( + (403, "authz"), + (404, "not-found"), + ) + @ddt.unpack + def test_create_keeps_the_status_the_store_refused_with(self, refusal_status, expected_slug): + response = self.post_with_store_refusal(refusal_status) + assert_error_envelope( + response, expected_status=refusal_status, expected_type_slug=expected_slug + ) + assert "internal bucket detail" not in json.dumps(response.data) + + def test_create_reports_an_unexpected_refusal_as_a_server_error(self): + response = self.post_with_store_refusal(418) + assert_error_envelope(response, expected_status=500, expected_type_slug="internal") + + def test_create_404_when_pipeline_disabled(self): + with override_settings(ENABLE_VIDEO_UPLOAD_PIPELINE=False): + response = self.api_client.post( + self.list_url, + {"files": [{"file_name": "first.mp4", "content_type": "video/mp4"}]}, + format="json", + ) + assert_error_envelope(response, expected_status=404, expected_type_slug="videos/uploads-not-configured") + + +class CourseVideoDestroyTest(CourseVideoUploadsTestBase): + """Tests for removing a video from a course.""" + + def test_destroy_detaches_the_video(self): + video_ids = self.create_videos(count=1) + response = self.api_client.delete(self.detail_url(video_ids[0])) + assert response.status_code == status.HTTP_204_NO_CONTENT + assert not response.data + course_video = CourseVideo.objects.get( + course_id=str(self.course.id), video__edx_video_id=video_ids[0] + ) + assert course_video.is_hidden is True + assert Video.objects.filter(edx_video_id=video_ids[0]).exists() + + def test_destroy_leaves_other_courses_alone(self): + other_course = CourseFactory.create() + create_video({ + "edx_video_id": "shared", + "client_video_id": "shared.mp4", + "duration": 1.0, + "status": "file_complete", + "courses": [str(self.course.id), str(other_course.id)], + "encoded_videos": [], + }) + self.api_client.delete(self.detail_url("shared")) + assert CourseVideo.objects.get( + course_id=str(other_course.id), video__edx_video_id="shared" + ).is_hidden is False + + def test_destroy_accepts_a_video_already_detached_from_the_course(self): + # A detached video is invisible to the reads but still removable, as it + # is on the legacy endpoints. + video_ids = self.create_videos(count=1) + self.api_client.delete(self.detail_url(video_ids[0])) + + assert self.api_client.get(self.detail_url(video_ids[0])).status_code == ( + status.HTTP_404_NOT_FOUND + ) + again = self.api_client.delete(self.detail_url(video_ids[0])) + assert again.status_code == status.HTTP_204_NO_CONTENT + assert CourseVideo.objects.get( + course_id=str(self.course.id), video__edx_video_id=video_ids[0] + ).is_hidden is True + + def test_destroy_404_for_unknown_video(self): + response = self.api_client.delete(self.detail_url("no-such-video")) + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + def test_destroy_404_when_pipeline_disabled(self): + video_ids = self.create_videos(count=1) + with override_settings(ENABLE_VIDEO_UPLOAD_PIPELINE=False): + response = self.api_client.delete(self.detail_url(video_ids[0])) + assert_error_envelope(response, expected_status=404, expected_type_slug="videos/uploads-not-configured") + assert CourseVideo.objects.get( + course_id=str(self.course.id), video__edx_video_id=video_ids[0] + ).is_hidden is False + + +class CourseVideoRequestErrorTest(CourseVideoUploadsTestBase): + """A request the API refuses to read is reported as the caller's error, not the server's.""" + + def test_a_malformed_body_is_reported_as_a_client_error(self): + response = self.api_client.post( + self.list_url, data="{not json", content_type="application/json" + ) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["title"] == "Malformed Request" + + def test_an_unsupported_method_is_reported_as_such(self): + response = self.api_client.put(self.list_url, {}, format="json") + envelope = assert_error_envelope( + response, expected_status=405, expected_type_slug="method-not-allowed" + ) + assert envelope["title"] == "Method Not Allowed" + + def test_an_unsupported_media_type_is_reported_as_such(self): + response = self.api_client.post( + self.list_url, data="file_name,content_type", content_type="text/csv" + ) + envelope = assert_error_envelope( + response, expected_status=415, expected_type_slug="unsupported-media-type" + ) + assert envelope["title"] == "Unsupported Media Type" + + def test_an_unsatisfiable_accept_header_is_reported_as_such(self): + response = self.api_client.get(self.list_url, HTTP_ACCEPT="application/xml") + envelope = assert_error_envelope( + response, expected_status=406, expected_type_slug="not-acceptable" + ) + assert envelope["title"] == "Not Acceptable" + + +@ddt.ddt +class CourseVideoAuthorizationTest(CourseVideoUploadsTestBase): + """Who may reach each operation, and who may not.""" + + def setUp(self): + super().setUp() + self.video_id = self.create_videos(count=1)[0] + + def call(self, client, operation, course_key=None): + """Issue ``operation`` against the course video endpoints as ``client``.""" + course_key = course_key or self.course.id + list_url = reverse(LIST_URL_NAME, kwargs={"course_key": course_key}) + detail_url = reverse( + DETAIL_URL_NAME, kwargs={"course_key": course_key, "edx_video_id": self.video_id} + ) + if operation == "list": + return client.get(list_url) + if operation == "retrieve": + return client.get(detail_url) + if operation == "create": + with patch("cms.djangoapps.contentstore.video_storage_handlers.boto3.resource") as resource: + _mock_bucket(resource) + return client.post( + list_url, + {"files": [{"file_name": "first.mp4", "content_type": "video/mp4"}]}, + format="json", + ) + return client.delete(detail_url) + + @ddt.data( + ("list", status.HTTP_200_OK), + ("retrieve", status.HTTP_200_OK), + ("create", status.HTTP_201_CREATED), + ("destroy", status.HTTP_204_NO_CONTENT), + ) + @ddt.unpack + def test_global_staff_is_allowed(self, operation, expected_status): + assert self.call(self.client_for(self.user), operation).status_code == expected_status + + @ddt.data( + ("list", status.HTTP_200_OK), + ("retrieve", status.HTTP_200_OK), + ("create", status.HTTP_201_CREATED), + ("destroy", status.HTTP_204_NO_CONTENT), + ) + @ddt.unpack + def test_course_staff_is_allowed(self, operation, expected_status): + author = self.create_course_author(CourseStaffRole) + assert self.call(self.client_for(author), operation).status_code == expected_status + + @ddt.data( + ("list", status.HTTP_200_OK), + ("retrieve", status.HTTP_200_OK), + ("create", status.HTTP_201_CREATED), + ("destroy", status.HTTP_204_NO_CONTENT), + ) + @ddt.unpack + def test_course_instructor_is_allowed(self, operation, expected_status): + author = self.create_course_author(CourseInstructorRole) + assert self.call(self.client_for(author), operation).status_code == expected_status + + @ddt.data("list", "retrieve", "create", "destroy") + def test_anonymous_is_refused(self, operation): + response = self.call(self.client_for(None), operation) + assert_error_envelope(response, expected_status=401, expected_type_slug="authn") + + @ddt.data("list", "retrieve", "create", "destroy") + def test_user_without_a_role_is_refused(self, operation): + stranger = UserFactory.create(is_staff=False) + response = self.call(self.client_for(stranger), operation) + assert_error_envelope(response, expected_status=403, expected_type_slug="authz") + + @ddt.data("list", "retrieve", "create", "destroy") + def test_limited_staff_is_refused(self, operation): + limited = self.create_course_author(CourseLimitedStaffRole) + response = self.call(self.client_for(limited), operation) + assert_error_envelope(response, expected_status=403, expected_type_slug="authz") + + @ddt.data("list", "retrieve", "create", "destroy") + def test_author_of_another_course_is_refused(self, operation): + other_course = CourseFactory.create() + author = self.create_course_author(CourseStaffRole, course=other_course) + response = self.call(self.client_for(author), operation) + assert_error_envelope(response, expected_status=403, expected_type_slug="authz") + + @ddt.data("list", "retrieve", "create", "destroy") + def test_ccx_course_key_is_refused(self, operation): + course_id = self.course.id + ccx_key = CourseKey.from_string( + f"ccx-v1:{course_id.org}+{course_id.course}+{course_id.run}+ccx@1" + ) + response = self.call(self.client_for(self.user), operation, course_key=ccx_key) + assert_error_envelope(response, expected_status=403, expected_type_slug="authz") + + @ddt.data("list", "retrieve", "create", "destroy") + def test_a_course_the_caller_cannot_read_in_studio_is_refused(self, operation): + # Studio's own read check refuses with its framework-level error, which + # carries no status of its own; the operation must still answer 403. + with patch( + "cms.djangoapps.contentstore.rest_api.v1.video_uploads_service._get_and_validate_course", + side_effect=DjangoPermissionDenied(), + ): + response = self.call(self.client_for(self.user), operation) + assert_error_envelope(response, expected_status=403, expected_type_slug="authz") + + @ddt.data("list", "retrieve", "create", "destroy") + def test_a_course_that_cannot_be_loaded_is_reported_as_missing(self, operation): + with patch( + "cms.djangoapps.contentstore.rest_api.v1.video_uploads_service._get_and_validate_course", + side_effect=Http404(), + ): + response = self.call(self.client_for(self.user), operation) + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + @ddt.data("list", "retrieve", "create", "destroy") + def test_no_domain_record_is_touched_when_refused(self, operation): + stranger = UserFactory.create(is_staff=False) + before = (Video.objects.count(), CourseVideo.objects.count()) + self.call(self.client_for(stranger), operation) + assert (Video.objects.count(), CourseVideo.objects.count()) == before + + +class CourseVideoReadOnlyTest(CourseVideoUploadsTestBase): + """Reading the course's videos must not change any of them.""" + + def setUp(self): + super().setUp() + self.create_videos(count=1, status_value="upload") + self.stuck_id = "video-upload-0" + self.set_created([self.stuck_id], age=timedelta(hours=48)) + + def row_counts(self): + return (Video.objects.count(), CourseVideo.objects.count(), EncodedVideo.objects.count()) + + def test_list_reports_a_stuck_upload_as_failed_without_changing_it(self): + before = Video.objects.get(edx_video_id=self.stuck_id).status + counts = self.row_counts() + + response = self.api_client.get(self.list_url) + + assert response.data["results"][0]["status"] == "Failed" + assert Video.objects.get(edx_video_id=self.stuck_id).status == before + assert self.row_counts() == counts + + def test_head_on_the_list_changes_nothing(self): + before = Video.objects.get(edx_video_id=self.stuck_id).status + counts = self.row_counts() + + response = self.api_client.head(self.list_url) + + assert response.status_code == status.HTTP_200_OK + assert Video.objects.get(edx_video_id=self.stuck_id).status == before + assert self.row_counts() == counts + + def test_head_on_the_member_changes_nothing(self): + before = Video.objects.get(edx_video_id=self.stuck_id).status + counts = self.row_counts() + + response = self.api_client.head(self.detail_url(self.stuck_id)) + + assert response.status_code == status.HTTP_200_OK + assert Video.objects.get(edx_video_id=self.stuck_id).status == before + assert self.row_counts() == counts + + def test_retrieve_reports_a_stuck_upload_as_failed_without_changing_it(self): + before = Video.objects.get(edx_video_id=self.stuck_id).status + counts = self.row_counts() + + response = self.api_client.get(self.detail_url(self.stuck_id)) + + assert response.data["status"] == "Failed" + assert Video.objects.get(edx_video_id=self.stuck_id).status == before + assert self.row_counts() == counts + + def test_the_legacy_route_still_reconciles_a_stuck_upload(self): + from cms.djangoapps.contentstore.utils import reverse_course_url + + assert Video.objects.get(edx_video_id=self.stuck_id).status == "upload" + response = self.client.get_json(reverse_course_url("videos_handler", str(self.course.id))) + assert response.status_code == status.HTTP_200_OK + assert Video.objects.get(edx_video_id=self.stuck_id).status == "upload_failed" + + +class CourseVideoAuthenticationTest(CourseVideoUploadsTestBase): + """The endpoints use the platform's default authentication.""" + + def test_session_caller_succeeds(self): + client = APIClient() + client.login(username=self.user.username, password=self.user_password) + assert client.get(self.list_url).status_code == status.HTTP_200_OK + + def test_jwt_caller_succeeds(self): + client = APIClient() + token = generate_jwt(self.user) + assert client.get( + self.list_url, HTTP_AUTHORIZATION=f"JWT {token}" + ).status_code == status.HTTP_200_OK + + def deactivated_session_client(self, is_staff=False): + """Log a course author in, then deactivate the account behind the live session.""" + password = "password-12345" + author = UserFactory.create(is_staff=is_staff, password=password) + CourseStaffRole(self.course.id).add_users(author) + client = APIClient() + assert client.login(username=author.username, password=password) + author.is_active = False + author.save() + return client + + def test_inactive_author_is_refused(self): + response = self.deactivated_session_client().get(self.list_url) + + assert_error_envelope(response, expected_status=401, expected_type_slug="authn") + + def test_inactive_author_is_refused_by_the_legacy_address_too(self): + video_ids = self.create_videos(count=1) + response = self.deactivated_session_client().get( + self.legacy_detail_url(video_ids[0]), HTTP_ACCEPT="application/json" + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_inactive_global_staff_is_refused(self): + response = self.deactivated_session_client(is_staff=True).get(self.list_url) + + assert_error_envelope(response, expected_status=401, expected_type_slug="authn") + + def test_inactive_global_staff_still_reaches_the_legacy_address(self): + video_ids = self.create_videos(count=1) + response = self.deactivated_session_client(is_staff=True).get( + self.legacy_detail_url(video_ids[0]), HTTP_ACCEPT="application/json" + ) + + assert response.status_code == status.HTTP_200_OK + + def test_the_viewset_does_not_declare_authentication_classes(self): + assert "authentication_classes" not in CourseVideoUploadsViewSet.__dict__ + assert not any( + "Bearer" in klass.__name__ or "OAuth2" in klass.__name__ + for klass in CourseVideoUploadsViewSet.authentication_classes + ) + + +class CourseVideoUrlContractTest(CourseVideoUploadsTestBase): + """The addresses, their names, and what happens to keys they do not accept.""" + + def test_conforming_addresses(self): + course_key = "course-v1:edX+DemoX+Demo_Course" + assert reverse(LIST_URL_NAME, kwargs={"course_key": course_key}) == ( + f"/api/authoring/v1/courses/{course_key}/videos/" + ) + assert reverse( + DETAIL_URL_NAME, kwargs={"course_key": course_key, "edx_video_id": "abc-123"} + ) == f"/api/authoring/v1/courses/{course_key}/videos/abc-123/" + + def test_conforming_addresses_resolve_to_the_viewset(self): + assert resolve(self.list_url).func.cls is CourseVideoUploadsViewSet + assert resolve(self.detail_url("abc-123")).func.cls is CourseVideoUploadsViewSet + + def test_a_usable_course_key_reaches_the_viewset_and_not_the_standby_routes(self): + """A key the converter accepts must be served by the endpoints, whatever stands behind them.""" + list_match, detail_match = resolve(self.list_url), resolve(self.detail_url("abc-123")) + + assert (list_match.url_name, detail_match.url_name) == ("course_video_list", "course_video_detail") + assert list_match.kwargs["course_key"] == self.course.id + assert detail_match.kwargs["course_key"] == self.course.id + assert isinstance(detail_match.kwargs["course_key"], CourseKey) + + def test_legacy_addresses_are_unchanged(self): + course_id = str(self.course.id) + assert self.legacy_list_url() == f"/api/contentstore/v0/videos/uploads/{course_id}" + assert self.legacy_detail_url("abc-123") == ( + f"/api/contentstore/v0/videos/uploads/{course_id}/abc-123" + ) + assert resolve(self.legacy_list_url()).func.cls is VideosCreateUploadView + assert resolve(self.legacy_detail_url("abc-123")).func.cls is VideosUploadsView + + def assert_json_not_found(self, url): + """Assert that ``url`` answers with the API error body rather than an error page.""" + response = self.api_client.get(url) + assert response["Content-Type"].startswith("application/json") + return assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + def test_deprecated_course_key_is_not_routed(self): + self.assert_json_not_found("/api/authoring/v1/courses/Org/Course/Run/videos/") + + def test_deprecated_course_key_is_not_routed_on_the_member_address(self): + self.assert_json_not_found("/api/authoring/v1/courses/Org/Course/Run/videos/abc-123/") + + def test_the_legacy_addresses_still_accept_a_deprecated_course_key(self): + legacy_match = resolve("/api/contentstore/v0/videos/uploads/Org/Course/Run/abc-123") + assert legacy_match.func.cls is VideosUploadsView + assert legacy_match.kwargs["course_id"] == "Org/Course/Run" + + def test_the_legacy_list_address_still_accepts_a_deprecated_course_key(self): + legacy_match = resolve("/api/contentstore/v0/videos/uploads/Org/Course/Run") + assert legacy_match.func.cls is VideosCreateUploadView + assert legacy_match.kwargs["course_id"] == "Org/Course/Run" + + def test_malformed_course_key_is_not_routed(self): + self.assert_json_not_found("/api/authoring/v1/courses/a+b+c/videos/") + + def test_an_unknown_address_under_a_course_is_not_routed(self): + self.assert_json_not_found("/api/authoring/v1/courses/course-v1:edX+DemoX+Demo_Course/no_such_thing/") + + def test_an_unknown_member_of_a_course_is_not_routed(self): + self.assert_json_not_found("/api/authoring/v1/courses/a+b+c/videos/abc-123/") + + def test_an_address_outside_a_course_is_left_to_the_site(self): + """ + Only addresses under a course carry the API error body. + + An address that names no course at all is beyond what this API claims, and + is answered by the site the same way as any other address it does not serve. + """ + from django.urls import Resolver404 + + with pytest.raises(Resolver404): + resolve("/api/authoring/v1/no/such/thing/") + assert self.api_client.get("/api/authoring/v1/no/such/thing/").status_code == 404 + + def test_unmatched_addresses_are_claimed_by_routes_that_can_be_reversed(self): + """Each address the endpoints turn down is claimed by a named route of its own.""" + unusable_key = "a+b+c" + addresses = { + "authoring_v1:course_video_list_unmatched": + f"/api/authoring/v1/courses/{unusable_key}/videos/", + "authoring_v1:course_video_detail_unmatched": + f"/api/authoring/v1/courses/{unusable_key}/videos/abc-123/", + "authoring_v1:course_unmatched": + f"/api/authoring/v1/courses/{unusable_key}/", + } + kwargs = { + "authoring_v1:course_video_detail_unmatched": {"edx_video_id": "abc-123"}, + } + for name, address in addresses.items(): + reversed_address = reverse(name, kwargs={"course_key": unusable_key, **kwargs.get(name, {})}) + assert reversed_address == address, name + assert resolve(address).func.cls is UnknownRouteView, name + + def test_an_unclaimed_address_needs_no_authentication(self): + response = self.client_for(None).get("/api/authoring/v1/courses/a+b+c/videos/") + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + def test_addresses_require_the_trailing_slash(self): + from django.urls import Resolver404 + + with pytest.raises(Resolver404): + resolve(self.list_url.rstrip("/")) + with pytest.raises(Resolver404): + resolve(self.detail_url("abc-123").rstrip("/")) + + def test_the_trailing_slash_is_still_appended(self): + response = self.api_client.get(self.list_url.rstrip("/")) + assert response.status_code == status.HTTP_301_MOVED_PERMANENTLY + assert response["Location"] == self.list_url + + def test_url_names_are_unique_to_the_authoring_namespace(self): + from django.urls import NoReverseMatch + + for name in ("course_video_list", "course_video_detail"): + with pytest.raises(NoReverseMatch): + reverse(f"cms.djangoapps.contentstore:v1:{name}", kwargs={"course_key": self.course.id}) + + +class CourseVideoQueryCountTest(CourseVideoUploadsTestBase): + """The cost of a page must not grow with the size of the course.""" + + #: Set from the pytest fixture below, which class-based tests cannot request directly. + django_assert_num_queries = None + + @pytest.fixture(autouse=True) + def _assert_num_queries(self, django_assert_num_queries): + self.django_assert_num_queries = django_assert_num_queries + + def warm_caches(self): + """Issue requests first so per-process caches do not skew the measured count.""" + for __ in range(2): + self.api_client.get(self.list_url, {"page_size": 1}) + + def test_list_query_count(self): + self.create_videos(count=10) + self.warm_caches() + with self.django_assert_num_queries(21): + self.api_client.get(self.list_url, {"page_size": 10}) + + def test_list_query_count_is_independent_of_course_size(self): + self.create_videos(count=100) + self.warm_caches() + with self.django_assert_num_queries(21): + self.api_client.get(self.list_url, {"page_size": 10}) + + def test_list_query_count_grows_only_with_the_page(self): + self.create_videos(count=10) + self.warm_caches() + with self.django_assert_num_queries(13): + self.api_client.get(self.list_url, {"page_size": 2}) + + def test_retrieve_query_count(self): + video_ids = self.create_videos(count=10) + self.warm_caches() + with self.django_assert_num_queries(13): + self.api_client.get(self.detail_url(video_ids[0])) + + @patch("cms.djangoapps.contentstore.video_storage_handlers.boto3.resource") + def test_create_query_count(self, mock_boto3_resource): + _mock_bucket(mock_boto3_resource) + body = {"files": [ + {"file_name": "first.mp4", "content_type": "video/mp4"}, + {"file_name": "second.mp4", "content_type": "video/mp4"}, + ]} + self.warm_caches() + self.api_client.post(self.list_url, body, format="json") + with self.django_assert_num_queries(16): + self.api_client.post(self.list_url, body, format="json") + + +class RowParityMixin: + """Comparing one row of the legacy listing against the same row of this version.""" + + def legacy_and_new_row(self, edx_video_id): + """Fetch both listings and return the (legacy, new) row for one video.""" + legacy = self.api_client.get( + self.legacy_detail_url(edx_video_id), HTTP_ACCEPT="application/json" + ) + new = self.api_client.get(self.list_url, {"page_size": 100}) + + assert legacy.status_code == status.HTTP_200_OK + assert new.status_code == status.HTTP_200_OK + legacy_rows = {row["edx_video_id"]: row for row in json.loads(legacy.content)["videos"]} + new_rows = {row["edx_video_id"]: row for row in json.loads(new.content)["results"]} + assert set(legacy_rows) == set(new_rows) + return legacy_rows[edx_video_id], new_rows[edx_video_id] + + def assert_row_parity(self, legacy_row, new_row): + """Assert the two rows differ in every declared way, and in no other way.""" + declared = [path for path, __ in ROW_DIFFERENCES] + changed, unexpected = _diff(legacy_row, new_row, declared) + assert not unexpected, f"undeclared differences: {sorted(unexpected)}" + for path in declared: + assert any(_matches(key, path) for key in changed), ( + f"declared difference did not occur: {path}" + ) + assert new_row["status"] == legacy_row["status_nontranslated"] + + +class CourseVideoParityTest(RowParityMixin, CourseVideoUploadsTestBase): + """The new addresses return what the legacy ones do, apart from the declared differences.""" + + def test_list_rows_match_the_legacy_listing(self): + create_profile("desktop_mp4") + create_video({ + "edx_video_id": "parity", + "client_video_id": "parity.mp4", + "duration": 42.0, + "status": "file_complete", + "courses": [str(self.course.id)], + "encoded_videos": [{ + "profile": "desktop_mp4", + "url": "http://example.com/parity.mp4", + "file_size": 1600, + "bitrate": 100, + }], + }) + self.set_created(["parity"]) + + self.assert_row_parity(*self.legacy_and_new_row("parity")) + + def test_created_keeps_the_precision_the_legacy_encoder_dropped(self): + video_ids = self.create_videos(count=1) + legacy_row, new_row = self.legacy_and_new_row(video_ids[0]) + + assert legacy_row["created"] == "2026-01-02T03:04:05.123Z" + assert new_row["created"] == "2026-01-02T03:04:05.123456Z" + + def test_the_status_is_never_locale_translated(self): + video_ids = self.create_videos(count=1) + with patch( + "cms.djangoapps.contentstore.video_storage_handlers._", side_effect="<{}>".format + ): + legacy_row, new_row = self.legacy_and_new_row(video_ids[0]) + + assert legacy_row["status"] == "" + assert new_row["status"] == "Ready" + + @patch("cms.djangoapps.contentstore.video_storage_handlers.boto3.resource") + def test_create_matches_the_legacy_create(self, mock_boto3_resource): + mock_s3_client = _mock_bucket(mock_boto3_resource) + body = {"files": [{"file_name": "parity.mp4", "content_type": "video/mp4"}]} + + legacy = self.api_client.post(self.legacy_list_url(), body, format="json") + legacy_params = mock_s3_client.generate_presigned_url.call_args[1] + new = self.api_client.post(self.list_url, body, format="json") + new_params = mock_s3_client.generate_presigned_url.call_args[1] + + assert legacy.status_code == status.HTTP_200_OK + assert new.status_code == status.HTTP_201_CREATED + + legacy_entry, = json.loads(legacy.content)["files"] + new_entry, = json.loads(new.content)["files"] + assert set(legacy_entry) == set(new_entry) + assert legacy_entry["file_name"] == new_entry["file_name"] + assert legacy_entry["upload_url"] == new_entry["upload_url"] + assert legacy_entry["edx_video_id"] != new_entry["edx_video_id"] + + assert legacy_params["ExpiresIn"] == new_params["ExpiresIn"] + assert legacy_params["Params"]["Bucket"] == new_params["Params"]["Bucket"] + assert legacy_params["Params"]["ContentType"] == new_params["Params"]["ContentType"] + assert legacy_params["Params"]["Metadata"] == new_params["Params"]["Metadata"] + + for entry in (legacy_entry, new_entry): + video = Video.objects.get(edx_video_id=entry["edx_video_id"]) + assert video.status == "upload" + assert video.client_video_id == "parity.mp4" + assert CourseVideo.objects.filter( + course_id=str(self.course.id), video=video + ).exists() + + def test_delete_matches_the_legacy_delete(self): + video_ids = self.create_videos(count=2) + + legacy = self.api_client.delete(self.legacy_detail_url(video_ids[0])) + new = self.api_client.delete(self.detail_url(video_ids[1])) + + assert legacy.status_code == new.status_code == status.HTTP_204_NO_CONTENT + for edx_video_id in video_ids: + assert CourseVideo.objects.get( + course_id=str(self.course.id), video__edx_video_id=edx_video_id + ).is_hidden is True + + def test_unauthenticated_error_shapes(self): + client = self.client_for(None) + legacy = client.get(self.legacy_detail_url("any"), HTTP_ACCEPT="application/json") + new = client.get(self.list_url) + + assert legacy.status_code == new.status_code == status.HTTP_401_UNAUTHORIZED + assert set(json.loads(legacy.content)) == {"developer_message"} + assert_error_envelope(new, expected_status=401, expected_type_slug="authn") + + def test_forbidden_error_shapes(self): + client = self.client_for(UserFactory.create(is_staff=False)) + legacy = client.get(self.legacy_detail_url("any"), HTTP_ACCEPT="application/json") + new = client.get(self.list_url) + + assert legacy.status_code == new.status_code == status.HTTP_403_FORBIDDEN + assert json.loads(legacy.content)["error_code"] == "user_permissions" + assert_error_envelope(new, expected_status=403, expected_type_slug="authz") + + def test_pipeline_disabled_error_shapes(self): + with override_settings(ENABLE_VIDEO_UPLOAD_PIPELINE=False): + legacy = self.api_client.get(self.legacy_detail_url("any"), HTTP_ACCEPT="application/json") + new = self.api_client.get(self.list_url) + + assert legacy.status_code == new.status_code == status.HTTP_404_NOT_FOUND + assert legacy.content == b"" + assert_error_envelope(new, expected_status=404, expected_type_slug="videos/uploads-not-configured") + + def test_invalid_request_body_error_shapes(self): + body = {"files": [{"file_name": "first.mp4"}]} + legacy = self.api_client.post(self.legacy_list_url(), body, format="json") + new = self.api_client.post(self.list_url, body, format="json") + + assert legacy.status_code == new.status_code == status.HTTP_400_BAD_REQUEST + assert legacy.content == b"" + assert_error_envelope(new, expected_status=400, expected_type_slug="validation") + + def test_missing_video_error_shapes(self): + legacy = self.api_client.delete(self.legacy_detail_url("no-such-video")) + new = self.api_client.delete(self.detail_url("no-such-video")) + + assert legacy.status_code == new.status_code == status.HTTP_404_NOT_FOUND + assert "developer_message" in json.loads(legacy.content) + assert_error_envelope(new, expected_status=404, expected_type_slug="not-found") + + def test_the_legacy_accept_header_branch_is_not_carried_over(self): + video_ids = self.create_videos(count=1) + + legacy = self.api_client.get(self.legacy_detail_url(video_ids[0]), HTTP_ACCEPT="*/*") + new = self.api_client.get(self.list_url, HTTP_ACCEPT="*/*") + + assert legacy.status_code == status.HTTP_302_FOUND + assert legacy["Location"] + assert new.status_code == status.HTTP_200_OK + assert "Location" not in new + assert set(json.loads(new.content)) == { + "count", "num_pages", "current_page", "start", "next", "previous", "results", + } + + def test_the_listing_is_wrapped_in_the_page_envelope(self): + self.create_videos(count=3) + + legacy = self.api_client.get( + self.legacy_detail_url("video-file_complete-0"), HTTP_ACCEPT="application/json" + ) + new = self.api_client.get(self.list_url, {"page": 2, "page_size": 1}) + + assert set(json.loads(legacy.content)) == {"videos"} + assert set(json.loads(new.content)) == { + "count", "num_pages", "current_page", "start", "next", "previous", "results", + } + + def test_minimal_view_is_a_subset_of_the_default(self): + video_ids = self.create_videos(count=1) + default = self.api_client.get(self.detail_url(video_ids[0])) + minimal = self.api_client.get(self.detail_url(video_ids[0]), {"view": "minimal"}) + assert set(minimal.data) < set(default.data) + for field in minimal.data: + assert minimal.data[field] == default.data[field] + + +@ddt.ddt +class CurrentWorkflowParityTest(RowParityMixin, CurrentWorkflowTestBase): + """Row parity for a course whose videos reach the encodes-ready and transcription states.""" + + @ddt.data( + "ingest", + "file_complete", + "upload_completed", + "invalid_token", + "transcription_in_progress", + "transcript_ready", + "partial_failure", + "transcript_failed", + ) + def test_rows_match_the_legacy_listing(self, status_value): + video_ids = self.create_videos(count=1, status_value=status_value) + + self.assert_row_parity(*self.legacy_and_new_row(video_ids[0])) + + def test_transcription_status_matches_the_legacy_listing(self): + video_ids = self.create_videos(count=1, status_value="partial_failure") + + legacy_row, new_row = self.legacy_and_new_row(video_ids[0]) + + assert legacy_row["transcription_status"] == "Partial Failure" + assert new_row["transcription_status"] == legacy_row["transcription_status"] + + +class CourseVideoSchemaTest(CourseVideoUploadsTestBase): + """What the generated schema promises, measured against what the endpoints return.""" + + def generated_schema(self): + """Generate the OpenAPI document for the course video addresses.""" + from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls + + return SchemaGenerator(patterns=authoring_urls.urlpatterns).get_schema(request=None, public=True) + + def resolve(self, schema, body): + """Return the component ``body`` refers to, or ``body`` itself.""" + if "$ref" in body: + return schema["components"]["schemas"][body["$ref"].rsplit("/", 1)[-1]] + return body + + def response_schema(self, schema, path_suffix, method="get", code="200"): + """Return the declared response body for one operation, following any reference.""" + path = next(path for path in schema["paths"] if path.endswith(path_suffix)) + body = schema["paths"][path][method]["responses"][code]["content"]["application/json"]["schema"] + return self.resolve(schema, body) + + def representations(self, schema, declared): + """Return the video representations ``declared`` allows, by component name.""" + variant = self.resolve(schema, declared) + return { + member["$ref"].rsplit("/", 1)[-1]: self.resolve(schema, member) + for member in variant["oneOf"] + } + + def test_only_the_two_video_addresses_are_documented(self): + """The routes that stand behind the endpoints serve no operation and must stay unpublished.""" + assert set(self.generated_schema()["paths"]) == { + "/courses/{course_key}/videos/", + "/courses/{course_key}/videos/{edx_video_id}/", + } + + def test_the_list_response_is_declared_as_the_page_envelope(self): + envelope = self.response_schema(self.generated_schema(), "/videos/") + assert envelope["type"] == "object" + assert set(envelope["properties"]) == { + "count", "num_pages", "current_page", "start", "next", "previous", "results", + } + assert set(envelope["required"]) == {"count", "num_pages", "current_page", "start", "results"} + assert envelope["properties"]["results"]["type"] == "array" + + def test_the_declared_list_envelope_is_the_one_returned(self): + self.create_videos(count=1) + envelope = self.response_schema(self.generated_schema(), "/videos/") + response = self.api_client.get(self.list_url) + assert set(response.data) == set(envelope["properties"]) + + def test_both_video_representations_are_declared(self): + schema = self.generated_schema() + envelope = self.response_schema(schema, "/videos/") + declared = self.representations(schema, envelope["properties"]["results"]["items"]) + assert set(declared) == {"CourseVideo", "CourseVideoMinimal"} + + def test_the_declared_full_representation_is_the_one_returned(self): + self.create_videos(count=1) + schema = self.generated_schema() + envelope = self.response_schema(schema, "/videos/") + declared = self.representations(schema, envelope["properties"]["results"]["items"])["CourseVideo"] + row = self.api_client.get(self.list_url).data["results"][0] + assert set(row) == set(declared["properties"]) == set(declared["required"]) + + def test_the_declared_minimal_representation_is_the_one_returned(self): + self.create_videos(count=1) + schema = self.generated_schema() + envelope = self.response_schema(schema, "/videos/") + declared = self.representations( + schema, envelope["properties"]["results"]["items"] + )["CourseVideoMinimal"] + row = self.api_client.get(self.list_url, {"view": "minimal"}).data["results"][0] + assert set(row) == set(declared["properties"]) == set(declared["required"]) + + def test_the_member_response_declares_both_representations(self): + schema = self.generated_schema() + video_ids = self.create_videos(count=1) + declared = self.representations( + schema, schema["paths"]["/courses/{course_key}/videos/{edx_video_id}/"]["get"][ + "responses"]["200"]["content"]["application/json"]["schema"] + ) + assert set(declared) == {"CourseVideo", "CourseVideoMinimal"} + full = self.api_client.get(self.detail_url(video_ids[0])).data + minimal = self.api_client.get(self.detail_url(video_ids[0]), {"view": "minimal"}).data + assert set(full) == set(declared["CourseVideo"]["properties"]) + assert set(minimal) == set(declared["CourseVideoMinimal"]["properties"]) + + def test_the_minimal_representation_is_named_by_the_view_parameter(self): + schema = self.generated_schema() + parameters = schema["paths"]["/courses/{course_key}/videos/"]["get"]["parameters"] + view_parameter = next(item for item in parameters if item["name"] == "view") + assert view_parameter["schema"]["enum"] == ["minimal"] + declared = self.representations( + schema, + self.response_schema(schema, "/videos/")["properties"]["results"]["items"], + )["CourseVideoMinimal"] + for field in declared["properties"]: + assert field in view_parameter["description"] + + def declared_examples(self, schema, path, method, code): + """Return the declared response examples of one operation, by title.""" + content = schema["paths"][path][method]["responses"][code]["content"]["application/json"] + return {name: example["value"] for name, example in content["examples"].items()} + + def test_the_member_404_names_both_causes(self): + schema = self.generated_schema() + declared = self.declared_examples( + schema, "/courses/{course_key}/videos/{edx_video_id}/", "get", "404" + ) + assert {example["type"] for example in declared.values()} == { + "https://docs.openedx.org/errors/videos/uploads-not-configured", + "https://docs.openedx.org/errors/not-found", + } + + def test_the_declared_404_types_are_the_ones_returned(self): + schema = self.generated_schema() + declared = { + example["type"] + for example in self.declared_examples( + schema, "/courses/{course_key}/videos/{edx_video_id}/", "get", "404" + ).values() + } + video_ids = self.create_videos(count=1) + unknown = self.api_client.get(self.detail_url("no-such-video")) + with override_settings(ENABLE_VIDEO_UPLOAD_PIPELINE=False): + unconfigured = self.api_client.get(self.detail_url(video_ids[0])) + assert {unknown.data["type"], unconfigured.data["type"]} == declared + + def test_the_create_404_names_only_the_cause_it_can_have(self): + schema = self.generated_schema() + declared = self.declared_examples(schema, "/courses/{course_key}/videos/", "post", "404") + assert {example["type"] for example in declared.values()} == { + "https://docs.openedx.org/errors/videos/uploads-not-configured", + } + + def test_the_create_response_is_declared_as_the_upload_slots(self): + slots = self.response_schema(self.generated_schema(), "/videos/", method="post", code="201") + assert set(slots["properties"]) == {"files"} + + +class CmsSchemaHookTest(CourseVideoUploadsTestBase): + """The schema hooks admit the new paths and flag the superseded ones.""" + + def endpoint(self, path): + return (path, path, "get", None) + + def test_filter_admits_the_authoring_prefix(self): + admitted = cms_api_filter([ + self.endpoint("/api/authoring/v1/courses/course-v1:a+b+c/videos/"), + self.endpoint("/api/contentstore/v0/videos/uploads/course-v1:a+b+c"), + self.endpoint("/login"), + ]) + assert [entry[0] for entry in admitted] == [ + "/api/authoring/v1/courses/course-v1:a+b+c/videos/", + "/api/contentstore/v0/videos/uploads/course-v1:a+b+c", + ] + + def test_post_processing_marks_only_the_superseded_operations(self): + schema = { + "paths": { + "/v0/videos/uploads/{course_id}": {"post": {}}, + "/v0/videos/uploads/{course_id}/{edx_video_id}": {"get": {}, "delete": {}}, + "/v0/videos/images/{course_id}/{edx_video_id}": {"post": {}}, + "/api/authoring/v1/courses/{course_key}/videos/": {"get": {}, "post": {}}, + } + } + result = cms_mark_superseded_paths(schema, None, None, False) + + deprecated = { + (path, method) + for path, item in result["paths"].items() + for method, operation in item.items() + if operation.get("deprecated") + } + assert deprecated == { + ("/v0/videos/uploads/{course_id}", "post"), + ("/v0/videos/uploads/{course_id}/{edx_video_id}", "get"), + ("/v0/videos/uploads/{course_id}/{edx_video_id}", "delete"), + } + + def test_post_processing_also_matches_untrimmed_paths(self): + schema = {"paths": {"/api/contentstore/v0/videos/uploads/{course_id}": {"post": {}}}} + result = cms_mark_superseded_paths(schema, None, None, False) + assert result["paths"]["/api/contentstore/v0/videos/uploads/{course_id}"]["post"]["deprecated"] is True + + def authoring_schema(self, hooks): + """Generate the authoring document with ``hooks`` as the post-processing list.""" + from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls + + with patched_settings({"POSTPROCESSING_HOOKS": hooks}): + return SchemaGenerator(patterns=authoring_urls.urlpatterns).get_schema( + request=None, public=True + ) + + def test_enum_components_survive_the_registered_post_processing(self): + deprecation_only = self.authoring_schema([SUPERSEDED_PATHS_HOOK]) + registered = self.authoring_schema(REGISTERED_POSTPROCESSING_HOOKS) + + assert not [ + name for name in deprecation_only["components"]["schemas"] if name.endswith("Enum") + ] + assert "ContentTypeEnum" in registered["components"]["schemas"] + + def test_post_processing_leaves_other_schema_members_alone(self): + schema = {"paths": {"/v0/videos/uploads/{course_id}": {"post": {}, "parameters": []}}} + result = cms_mark_superseded_paths(schema, None, None, False) + assert not result["paths"]["/v0/videos/uploads/{course_id}"]["parameters"] + + +@ddt.ddt +class CmsSchemaAddressTest(TestCase): + """The published document addresses every operation the way it is mounted.""" + + # One operation of every Studio API version the document covers, with the + # address a client reaches by joining the published path to a server, plus + # both new addresses. + PUBLISHED_PATHS = { + "/api/contentstore/v0/videos/uploads/{course_id}": + "/api/contentstore/v0/videos/uploads/course-v1:a+b+c", + "/api/contentstore/v1/videos/{course_id}": + "/api/contentstore/v1/videos/course-v1:a+b+c", + "/api/contentstore/v2/home/courses": + "/api/contentstore/v2/home/courses", + "/api/contentstore/v3/course_details/{course_id}/": + "/api/contentstore/v3/course_details/course-v1:a+b+c/", + "/api/contentstore/v4/home/courses/": + "/api/contentstore/v4/home/courses/", + "/api/authoring/v1/courses/{course_key}/videos/": + "/api/authoring/v1/courses/course-v1:a+b+c/videos/", + "/api/authoring/v1/courses/{course_key}/videos/{edx_video_id}/": + "/api/authoring/v1/courses/course-v1:a+b+c/videos/video-1/", + } + + def cms_schema(self, module=PRODUCTION_SETTINGS): + """Generate the Studio document with the schema settings of ``module``.""" + read = schema_settings(module) + with patched_settings({key: read[key] for key in GENERATION_SETTING_KEYS}): + return SchemaGenerator().get_schema(request=None, public=True) + + def config_without_public_host(self): + """Write a deployment configuration that leaves the public host unset.""" + directory = tempfile.TemporaryDirectory() + self.addCleanup(directory.cleanup) + config = Path(directory.name) / "cms.yml" + config.write_text("".join( + line for line in MOCK_CONFIG.read_text().splitlines(keepends=True) + if not line.startswith("AUTHORING_API_URL:") + )) + return config + + def test_the_document_publishes_the_mounted_addresses(self): + assert set(self.PUBLISHED_PATHS) <= set(self.cms_schema()["paths"]) + + def test_the_document_publishes_no_other_authoring_address(self): + published = self.cms_schema()["paths"] + authoring = {path for path in published if path.startswith("/api/authoring/")} + assert authoring == {path for path in self.PUBLISHED_PATHS if path.startswith("/api/authoring/")} + + def test_no_published_path_is_shortened(self): + published = sorted(self.cms_schema()["paths"]) + assert published, "the pre-processing hook admitted no path" + assert [path for path in published if not path.startswith("/api/")] == [] + + def test_every_published_path_is_an_address_that_resolves(self): + for published, address in self.PUBLISHED_PATHS.items(): + assert resolve(address).func, published + + @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) + def test_the_settings_publish_paths_in_full(self, module): + assert schema_settings(module)["SCHEMA_PATH_PREFIX_TRIM"] is False + + @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) + def test_the_servers_serve_every_published_path(self, module): + servers = schema_settings(module)["SERVERS"] + assert [server["description"] for server in servers] == ["Public", "Local"] + assert [server for server in servers if "/api/contentstore" in server["url"]] == [] + + @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) + def test_an_unset_public_host_is_left_out_of_the_servers(self, module): + servers = schema_settings(module, config_file=self.config_without_public_host())["SERVERS"] + assert [server["description"] for server in servers] == ["Local"] + assert all(server["url"] for server in servers) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py b/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py new file mode 100644 index 000000000000..dd089c0163fe --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py @@ -0,0 +1,26 @@ +"""API view answering addresses that no endpoint serves.""" + +from drf_spectacular.utils import extend_schema +from edx_rest_framework_extensions.mixins import StandardizedErrorMixin +from rest_framework.exceptions import NotFound +from rest_framework.permissions import AllowAny +from rest_framework.views import APIView + + +@extend_schema(exclude=True) +class UnknownRouteView(StandardizedErrorMixin, APIView): + """ + Answer an address under a course that no endpoint of this API serves. + + A course key that is malformed or in the deprecated slash-separated form, or + a misspelt address beneath a course, would otherwise be answered by the + site's HTML error page, which an API client cannot read. Nothing here needs + the caller's identity, so the answer is the same for everyone: the address + holds no resource. + """ + + permission_classes = (AllowAny,) + + def initial(self, request, *args, **kwargs): + """Refuse the request before any handler runs; the address holds nothing.""" + raise NotFound() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/video_uploads.py b/cms/djangoapps/contentstore/rest_api/v1/views/video_uploads.py new file mode 100644 index 000000000000..25e308b7e16a --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/video_uploads.py @@ -0,0 +1,399 @@ +"""API Views for the video assets of a course.""" + +import logging + +from drf_spectacular.utils import ( + OpenApiExample, + OpenApiParameter, + OpenApiRequest, + OpenApiResponse, + PolymorphicProxySerializer, + extend_schema, +) +from edx_rest_framework_extensions.errors import ErrorResponseSerializer, error_type_uri +from edx_rest_framework_extensions.mixins import StandardizedErrorMixin +from edx_rest_framework_extensions.paginators import DefaultPagination, IterablePaginationMixin +from edx_rest_framework_extensions.shaping import MinimalViewMixin +from rest_framework import status, viewsets +from rest_framework.permissions import IsAuthenticated +from rest_framework.response import Response + +from cms.djangoapps.contentstore.rest_api.v1.serializers.video_uploads import ( + MINIMAL_VIDEO_FIELDS, + CourseVideoListQuerySerializer, + CourseVideoMinimalSerializer, + CourseVideoQuerySerializer, + CourseVideoSerializer, + VideoUploadRequestSerializer, + VideoUploadResponseSerializer, +) +from cms.djangoapps.contentstore.rest_api.v1.video_uploads_service import ( + DEFAULT_ORDERING, + VIDEO_ORDERING_CHOICES, + VIDEO_UPLOADS_NOT_CONFIGURED_SLUG, + create_video_uploads, + delete_course_video, + enrich_course_videos, + get_course_for_uploads, + get_course_video, + list_course_videos, +) +from cms.djangoapps.contentstore.rest_api.v1.views.permissions import HasCourseAuthorAccess + +log = logging.getLogger(__name__) + + +class CourseVideoPagination(DefaultPagination): + """ + The platform's page envelope, described in full to schema consumers. + + ``DefaultPagination`` answers with seven members, but the response schema it + inherits names only four of them, so a generated client would not know that + ``num_pages``, ``current_page`` and ``start`` are there. + """ + + def get_paginated_response_schema(self, schema): + """Describe every member of the page envelope this paginator returns.""" + inherited = super().get_paginated_response_schema(schema)["properties"] + return { + "type": "object", + "required": ["count", "num_pages", "current_page", "start", "results"], + "properties": { + "count": inherited["count"], + "num_pages": { + "type": "integer", + "description": "Number of pages the results are divided into.", + "example": 13, + }, + "current_page": { + "type": "integer", + "description": "Number of the page returned.", + "example": 4, + }, + "start": { + "type": "integer", + "description": "Position of this page's first result within the whole list.", + "example": 30, + }, + "next": inherited["next"], + "previous": inherited["previous"], + "results": inherited["results"], + }, + } + + +_COURSE_KEY_PARAMETER = OpenApiParameter( + name="course_key", + description="Key of the course the videos belong to, for example course-v1:edX+DemoX+Demo_Course.", + required=True, + type=str, + location=OpenApiParameter.PATH, +) + +_EDX_VIDEO_ID_PARAMETER = OpenApiParameter( + name="edx_video_id", + description="Identifier of the video, as returned when its upload slot was created.", + required=True, + type=str, + location=OpenApiParameter.PATH, +) + +_VIEW_PARAMETER = OpenApiParameter( + name="view", + description=( + "Response preset. 'minimal' reduces each video to " + f"{', '.join(MINIMAL_VIDEO_FIELDS)}, omitting every other field. " + "Omit the parameter to receive the full representation." + ), + required=False, + type=str, + location=OpenApiParameter.QUERY, + enum=[MinimalViewMixin.minimal_view_value], +) + +_ORDERING_PARAMETER = OpenApiParameter( + name="ordering", + description=( + "Field to sort the videos by. Prefix the field name with '-' to sort descending. " + f"Defaults to {DEFAULT_ORDERING}." + ), + required=False, + type=str, + location=OpenApiParameter.QUERY, + enum=list(VIDEO_ORDERING_CHOICES), +) + +_PAGE_PARAMETER = OpenApiParameter( + name="page", + description="Number of the page to return. Defaults to the first page.", + required=False, + type=int, + location=OpenApiParameter.QUERY, +) + +_PAGE_SIZE_PARAMETER = OpenApiParameter( + name="page_size", + description=( + f"Number of videos per page. Defaults to {CourseVideoPagination.page_size}; a larger " + f"value is reduced to the maximum of {CourseVideoPagination.max_page_size}." + ), + required=False, + type=int, + location=OpenApiParameter.QUERY, +) + +_RESPONSE_BAD_REQUEST = OpenApiResponse( + response=ErrorResponseSerializer, + description="The request body or a query parameter is invalid.", +) +_RESPONSE_UNAUTHENTICATED = OpenApiResponse( + response=ErrorResponseSerializer, + description="The requester is not authenticated.", +) +_RESPONSE_FORBIDDEN = OpenApiResponse( + response=ErrorResponseSerializer, + description="The requester does not have authoring access to the course.", +) +_EXAMPLE_UPLOADS_NOT_CONFIGURED = OpenApiExample( + "The course does not accept video uploads", + value={ + "type": error_type_uri(VIDEO_UPLOADS_NOT_CONFIGURED_SLUG), + "title": "Video Uploads Not Configured", + "status": 404, + "detail": "Video uploads are not configured for this course.", + "instance": "/api/authoring/v1/courses/course-v1:edX+DemoX+Demo_Course/videos/", + }, + response_only=True, + status_codes=["404"], +) + +_EXAMPLE_VIDEO_NOT_FOUND = OpenApiExample( + "No such video in this course", + value={ + "type": error_type_uri("not-found"), + "title": "Not Found", + "status": 404, + "detail": "Not found.", + "instance": ( + "/api/authoring/v1/courses/course-v1:edX+DemoX+Demo_Course/videos/" + "8f2a6d9c-2f2e-4a7e-9d6b-1f0c9e2a4b31/" + ), + }, + response_only=True, + status_codes=["404"], +) + +_RESPONSE_NOT_FOUND = OpenApiResponse( + response=ErrorResponseSerializer, + description="The course does not accept video uploads, or the video is not attached to it.", + examples=[_EXAMPLE_UPLOADS_NOT_CONFIGURED, _EXAMPLE_VIDEO_NOT_FOUND], +) +_RESPONSE_CREATE_NOT_FOUND = OpenApiResponse( + response=ErrorResponseSerializer, + description="The course does not accept video uploads.", + examples=[_EXAMPLE_UPLOADS_NOT_CONFIGURED], +) +_RESPONSE_LIST_NOT_FOUND = OpenApiResponse( + response=ErrorResponseSerializer, + description=( + "The course does not accept video uploads, or the requested page is past the end of " + "the list." + ), + examples=[_EXAMPLE_UPLOADS_NOT_CONFIGURED], +) + +_CREATE_EXAMPLE_REQUEST = OpenApiExample( + "Two files", + value={ + "files": [ + {"file_name": "lecture-01.mp4", "content_type": "video/mp4"}, + {"file_name": "lecture-02.mov", "content_type": "video/quicktime"}, + ] + }, + request_only=True, +) + +_CREATE_EXAMPLE_RESPONSE = OpenApiExample( + "Upload slots to PUT the files to", + value={ + "files": [ + { + "file_name": "lecture-01.mp4", + "upload_url": "https://video-uploads.example.com/videos/8f2a...?X-Amz-Signature=...", + "edx_video_id": "8f2a6d9c-2f2e-4a7e-9d6b-1f0c9e2a4b31", + }, + { + "file_name": "lecture-02.mov", + "upload_url": "https://video-uploads.example.com/videos/1c7b...?X-Amz-Signature=...", + "edx_video_id": "1c7b40f5-6b62-4a51-8f10-0b4d2a9f7c58", + }, + ] + }, + response_only=True, +) + + +def _video_response(description, many=False): + """Describe a response carrying videos in whichever representation was asked for.""" + return OpenApiResponse( + response=PolymorphicProxySerializer( + component_name="CourseVideoRepresentation", + serializers=[CourseVideoSerializer, CourseVideoMinimalSerializer], + resource_type_field_name=None, + many=many, + ), + description=description, + ) + + +@extend_schema(tags=["openedx-platform-sdk"]) +class CourseVideoUploadsViewSet( + StandardizedErrorMixin, MinimalViewMixin, IterablePaginationMixin, viewsets.ViewSet +): + """ + The video assets of a single course. + + Lists the videos attached to a course, returns one of them, creates upload + slots for new ones, and detaches a video from the course. Reads and writes + go through the video store rather than the database directly, so this is a + plain ViewSet with no queryset of its own. + + Listing a video reports an upload that has been stuck past the upload window + as failed without changing the stored record; a stuck upload is reconciled + by the upload pipeline, not by reading the list. + """ + + permission_classes = (IsAuthenticated, HasCourseAuthorAccess) + serializer_class = CourseVideoSerializer + pagination_class = CourseVideoPagination + minimal_fields = MINIMAL_VIDEO_FIELDS + + def __init__(self, **kwargs): + super().__init__(**kwargs) + self.course_key = None + + def initial(self, request, *args, **kwargs): + """Expose the course key for the permission check that runs next.""" + self.course_key = kwargs.get("course_key") + super().initial(request, *args, **kwargs) + + def get_serializer_context(self): + """Return the context every serializer of this view is built with.""" + return {"request": self.request, "view": self, "format": self.format_kwarg} + + def get_serializer(self, *args, **kwargs): + """Instantiate and return the configured serializer class.""" + kwargs.setdefault("context", self.get_serializer_context()) + return self.serializer_class(*args, **kwargs) + + @extend_schema( + summary="List the videos of a course", + description=( + "Returns a paginated list of the videos attached to the course, newest first by " + "default. Transcript details are resolved only for the videos on the requested page." + ), + parameters=[ + _COURSE_KEY_PARAMETER, + _ORDERING_PARAMETER, + _PAGE_PARAMETER, + _PAGE_SIZE_PARAMETER, + _VIEW_PARAMETER, + ], + responses={ + 200: _video_response( + "A page of videos, in the representation the view parameter selected.", + many=True, + ), + 400: _RESPONSE_BAD_REQUEST, + 401: _RESPONSE_UNAUTHENTICATED, + 403: _RESPONSE_FORBIDDEN, + 404: _RESPONSE_LIST_NOT_FOUND, + }, + ) + def list(self, request, course_key): + """Return a page of the course's videos.""" + query = CourseVideoListQuerySerializer(data=request.query_params) + query.is_valid(raise_exception=True) + course = get_course_for_uploads(course_key, request.user) + videos = list_course_videos(course, query.validated_data.get("ordering", DEFAULT_ORDERING)) + return self.paginate_iterable( + request, + videos, + serialize=lambda page: self.shape_minimal( + self.get_serializer(enrich_course_videos(course, page), many=True).data, + request, + ), + ) + + @extend_schema( + summary="Get one video of a course", + description="Returns the video attached to the course under the given identifier.", + parameters=[_COURSE_KEY_PARAMETER, _EDX_VIDEO_ID_PARAMETER, _VIEW_PARAMETER], + responses={ + 200: _video_response("The video, in the representation the view parameter selected."), + 400: _RESPONSE_BAD_REQUEST, + 401: _RESPONSE_UNAUTHENTICATED, + 403: _RESPONSE_FORBIDDEN, + 404: _RESPONSE_NOT_FOUND, + }, + ) + def retrieve(self, request, course_key, edx_video_id): + """Return one of the course's videos.""" + CourseVideoQuerySerializer(data=request.query_params).is_valid(raise_exception=True) + course = get_course_for_uploads(course_key, request.user) + video = get_course_video(course, edx_video_id) + enriched, = enrich_course_videos(course, [video]) + data = self.get_serializer(enriched).data + return Response(self.shape_minimal(data, request)) + + @extend_schema( + summary="Create upload slots for new course videos", + description=( + "Registers one video per requested file and returns a short-lived pre-signed URL for " + "each. Upload the file itself with a PUT to that URL; this endpoint does not receive " + "file content. The returned identifiers address the videos from then on." + ), + parameters=[_COURSE_KEY_PARAMETER], + request=OpenApiRequest(request=VideoUploadRequestSerializer), + responses={ + 201: OpenApiResponse( + response=VideoUploadResponseSerializer, + description="Upload slots created, one per requested file, in request order.", + ), + 400: _RESPONSE_BAD_REQUEST, + 401: _RESPONSE_UNAUTHENTICATED, + 403: _RESPONSE_FORBIDDEN, + 404: _RESPONSE_CREATE_NOT_FOUND, + }, + examples=[_CREATE_EXAMPLE_REQUEST, _CREATE_EXAMPLE_RESPONSE], + ) + def create(self, request, course_key): + """Create an upload slot for each requested file.""" + request_serializer = VideoUploadRequestSerializer(data=request.data) + request_serializer.is_valid(raise_exception=True) + course = get_course_for_uploads(course_key, request.user) + created = create_video_uploads(course, request_serializer.validated_data["files"]) + return Response( + VideoUploadResponseSerializer(created).data, + status=status.HTTP_201_CREATED, + ) + + @extend_schema( + summary="Remove a video from a course", + description=( + "Detaches the video from the course. The video itself is kept, so it stays available " + "in any other course it is attached to." + ), + parameters=[_COURSE_KEY_PARAMETER, _EDX_VIDEO_ID_PARAMETER], + responses={ + 204: OpenApiResponse(description="The video is no longer attached to the course."), + 401: _RESPONSE_UNAUTHENTICATED, + 403: _RESPONSE_FORBIDDEN, + 404: _RESPONSE_NOT_FOUND, + }, + ) + def destroy(self, request, course_key, edx_video_id): + """Detach one video from the course.""" + get_course_for_uploads(course_key, request.user) + delete_course_video(course_key, edx_video_id) + return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/cms/urls.py b/cms/urls.py index c0f96f489bb8..9bf84dc7ea86 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 @@ -27,6 +28,7 @@ from openedx.core.djangoapps.password_policy.forms import PasswordPolicyAwareAdminAuthForm django_autodiscover() +register_url_converters() admin.site.site_header = _('Studio Administration') admin.site.site_title = admin.site.site_header @@ -356,6 +358,11 @@ path('api/contentstore/', include('cms.djangoapps.contentstore.rest_api.urls')) ] +# Authoring REST APIs +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 a973122de4f6662a2afbd3d29abe1c0044cf0f80 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Mon, 14 Sep 2026 17:37:04 +0500 Subject: [PATCH 02/15] fix: publish CMS schema paths in full so every operation is reachable The CMS schema trimmed '/api/contentstore' from every path and advertised a matching server entry. Once the document also carries paths under another prefix, that arrangement has no server that serves both: a client generated from the advertised server builds https://{CMS_BASE}/api/contentstore/api/authoring/v1/courses/{key}/videos/ and gets a 404. Trimming is therefore off and the server list is reduced to the public host and the Studio host, so one server URL serves every operation in the document. Path strings change from /v0/... to /api/contentstore/v0/..., but no effective URL moves and no operation is added or removed by this commit. Two consequences worth knowing. The schema comparison gate reads a path-key rename as a removal, so it reports a breaking change for each renamed path; the compensating addition is invisible to it. And AUTHORING_API_URL now joins full paths rather than shortened ones, so it must be the Studio service root rather than a gateway base. These settings predate this work; the defect is latent on master and is surfaced, not introduced, by adding a second prefix to the document. --- cms/envs/devstack.py | 22 +++++++++++++++++----- cms/envs/production.py | 22 +++++++++++++++++----- 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index 0576a35272af..a2e5282309ea 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -356,12 +356,24 @@ def should_show_debug_toolbar(request): # pylint: disable=missing-function-docs 'SERVE_INCLUDE_SCHEMA': False, # restrict spectacular to CMS API endpoints (cms/lib/spectacular.py): 'PREPROCESSING_HOOKS': ['cms.lib.spectacular.cms_api_filter'], - # remove the default schema path prefix to replace it with server-specific base paths: + # Setting this replaces drf-spectacular's default hook list, so the default + # enum post-processing has to be listed again to keep running: + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'cms.lib.spectacular.cms_mark_superseded_paths', + ], + # Paths are published in full, so one server URL serves every operation of the + # document. The prefix below is not removed from the paths; it only keeps the + # service prefix out of generated operation ids and tags: 'SCHEMA_PATH_PREFIX': '/api/contentstore', - 'SCHEMA_PATH_PREFIX_TRIM': '/api/contentstore', + 'SCHEMA_PATH_PREFIX_TRIM': False, + # The public host is optional, so an unset URL is left out of the list: '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'} + {'url': url, 'description': description} + for url, description in [ + (AUTHORING_API_URL, 'Public'), # noqa: F405 + (f'http://{CMS_BASE}', 'Local'), + ] + if url ], } diff --git a/cms/envs/production.py b/cms/envs/production.py index b15caa2d1c14..bbe4930e49e9 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -415,13 +415,25 @@ def get_env_setting(setting): 'SERVE_INCLUDE_SCHEMA': False, # restrict spectacular to CMS API endpoints (cms/lib/spectacular.py): 'PREPROCESSING_HOOKS': ['cms.lib.spectacular.cms_api_filter'], - # remove the default schema path prefix to replace it with server-specific base paths: + # Setting this replaces drf-spectacular's default hook list, so the default + # enum post-processing has to be listed again to keep running: + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'cms.lib.spectacular.cms_mark_superseded_paths', + ], + # Paths are published in full, so one server URL serves every operation of the + # document. The prefix below is not removed from the paths; it only keeps the + # service prefix out of generated operation ids and tags: 'SCHEMA_PATH_PREFIX': '/api/contentstore', - 'SCHEMA_PATH_PREFIX_TRIM': '/api/contentstore', + 'SCHEMA_PATH_PREFIX_TRIM': False, + # The public host is optional, so an unset URL is left out of the list: '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 + {'url': url, 'description': description} + for url, description in [ + (AUTHORING_API_URL, 'Public'), # noqa: F405 + (f'https://{CMS_BASE}', 'Local'), # noqa: F405 + ] + if url ], } From e5971c512624fbf514d6f7bcec77c67e321b31f8 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Mon, 14 Sep 2026 17:37:15 +0500 Subject: [PATCH 03/15] docs: mark the v0 video upload endpoints deprecated The three video upload operations under /api/contentstore/v0/videos/uploads/ now have conforming successors, so they are marked deprecated in the published schema by a post-processing hook, and their docstrings name the successor address for each operation. The hook lives outside the frozen version directory, and the change inside it is docstring text only: no import, no statement, no decorator, so the v0 endpoints behave exactly as before. No runtime Deprecation header is emitted, since that would be a response change on a live endpoint. The pre-processing filter is widened at the same time so the authoring paths reach the document at all; without it they are dropped silently. Still owed before the deprecation window can formally open: a DEPR issue in openedx/public-engineering, and a named removal release. Both need someone who knows the current release train. Part of #39060. --- .../rest_api/v0/views/authoring_videos.py | 28 ++++++++++++++++ cms/lib/spectacular.py | 32 +++++++++++++++++-- 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py b/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py index 5a7563c2372e..e5618a05b6cc 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py @@ -1,5 +1,12 @@ """ Public rest API endpoints for the Authoring API video assets. + +.. deprecated:: + The video upload endpoints in this module are superseded by + ``CourseVideoUploadsViewSet`` in + ``cms.djangoapps.contentstore.rest_api.v1.views.video_uploads``. + Use ``/api/authoring/v1/courses/{course_key}/videos/`` going forward. + These v0 endpoints will be removed in a future release. """ import logging @@ -27,6 +34,10 @@ @view_auth_classes() class VideosUploadsView(DeveloperErrorViewMixin, RetrieveAPIView, DestroyAPIView): """ + **DEPRECATED** — use ``GET /api/authoring/v1/courses/{course_key}/videos/`` for the listing + this GET returns, and ``DELETE /api/authoring/v1/courses/{course_key}/videos/{edx_video_id}/`` + to remove one video from the course. These v0 endpoints will be removed in a future release. + public rest API endpoints for the CMS API video assets. course_key: required argument, needed to authorize course authors and identify the video. video_id: required argument, needed to identify the video. @@ -35,17 +46,34 @@ class VideosUploadsView(DeveloperErrorViewMixin, RetrieveAPIView, DestroyAPIView @course_author_access_required def retrieve(self, request, course_key, edx_video_id=None): # pylint: disable=arguments-differ + """ + **DEPRECATED** — use ``GET /api/authoring/v1/courses/{course_key}/videos/`` instead. + + This GET ignores ``edx_video_id`` and returns the whole course listing; + ``/api/authoring/v1/courses/{course_key}/videos/`` is the successor for that listing. + It will be removed in a future release. + """ return handle_videos(request, course_key.html_id(), edx_video_id) @course_author_access_required @expect_json_in_class_view def destroy(self, request, course_key, edx_video_id): # pylint: disable=arguments-differ + """ + **DEPRECATED** — use + ``DELETE /api/authoring/v1/courses/{course_key}/videos/{edx_video_id}/`` instead. + + It will be removed in a future release. + """ return handle_videos(request, course_key.html_id(), edx_video_id) @view_auth_classes() class VideosCreateUploadView(DeveloperErrorViewMixin, CreateAPIView): """ + **DEPRECATED** — use ``POST /api/authoring/v1/courses/{course_key}/videos/`` instead. + + These v0 endpoints will be removed in a future release. + public rest API endpoints for the CMS API video assets. course_key: required argument, needed to authorize course authors and identify the video. """ diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 90bce5668fec..af60402915b2 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -2,14 +2,26 @@ import re +CMS_PATH_PATTERN = re.compile(r"^/api/(contentstore|authoring)/v\d+/") + +# Path prefixes of operations that have a conforming successor and are kept only +# for their deprecation window. The mounted prefix is listed next to the +# shortened form, so the hook still matches where a document is published +# without the service prefix. +SUPERSEDED_PATH_PREFIXES = ( + "/api/contentstore/v0/videos/uploads/", + "/v0/videos/uploads/", +) + +_OPERATION_KEYS = frozenset({"get", "put", "post", "delete", "options", "head", "patch", "trace"}) + def cms_api_filter(endpoints): """ - Pre-processing hook: keep only contentstore versioned endpoints and select - course-level endpoints. + Pre-processing hook: keep the versioned authoring and contentstore + endpoints, and the course-level endpoints named below. """ filtered = [] - CMS_PATH_PATTERN = re.compile(r"^/api/contentstore/v\d+/") for path, path_regex, method, callback in endpoints: if ( @@ -22,3 +34,17 @@ def cms_api_filter(endpoints): filtered.append((path, path_regex, method, callback)) return filtered + + +def cms_mark_superseded_paths(result, generator, request, public): # pylint: disable=unused-argument + """ + Post-processing hook: mark every operation of a superseded path deprecated. + """ + for path, path_item in (result.get("paths") or {}).items(): + if not path.startswith(SUPERSEDED_PATH_PREFIXES): + continue + for key, operation in path_item.items(): + if key in _OPERATION_KEYS and isinstance(operation, dict): + operation["deprecated"] = True + + return result From a1c46d5ba9988c02793295c919983a5278ecb901 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Fri, 18 Sep 2026 14:50:25 +0500 Subject: [PATCH 04/15] refactor: align the authoring v1 schema plumbing with the ADR 0038 migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The open URL-structure work (#39078) reaches the same schema conclusion this branch did — paths published in full, the service-prefixed server entry dropped — but expresses it differently. Adopt its shapes so the two merge mechanically instead of conflicting: - cms/lib/spectacular.py becomes its module, with one added prefix for the v0 video upload addresses. Our separately named hook, its operation-key set and the shortened path form are gone; nothing trims any more, so the short form was dead. - SPECTACULAR_SETTINGS matches it byte for byte: SCHEMA_PATH_PREFIX_TRIM removed rather than set false, the prefix reduced to a tag-extraction regex covering both services, and SERVERS restored to the plain two-entry list. - The opaque-key converters register before autodiscovery, as they do there. - The authoring URLConf follows its layout. Its xblock routes are deliberately not taken; they belong to that change. Two consequences worth naming. Operation ids lose the service prefix, so the new endpoints become v1_courses_videos_* — they are new here, so nothing depends on the old spelling, and contentstore ids are untouched. And an unset public host is published as an empty server URL again, which is how the setting behaves on master; the guard against it belongs in its own change rather than hidden in this one. --- .../rest_api/v1/authoring_urls.py | 13 ++- .../v1/views/tests/test_video_uploads.py | 88 ++++++++----------- cms/envs/devstack.py | 24 ++--- cms/envs/production.py | 24 ++--- cms/lib/spectacular.py | 50 +++++++---- cms/urls.py | 6 +- 6 files changed, 95 insertions(+), 110 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py index a3cb45753f7c..ba361e43c5a9 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -21,14 +21,11 @@ CourseVideoUploadsViewSet.as_view({"get": "retrieve", "delete": "destroy"}), name="course_video_detail", ), - # The three routes below stand behind the two above and are reached only by a - # request the ones above turned down: a course key the converter refuses - # because it is malformed or in the deprecated slash-separated form, or an - # address under a course that this API does not serve. They answer with the - # API error body, which a client can read, instead of the site's HTML error - # page. Order is what keeps them out of the way of real requests, and each - # accepts only addresses ending in a slash so that the redirect to the - # slash-terminated form keeps working. + # Reached only by a request the routes above turned down: a course key the + # converter refuses, or an address beneath a course that this API does not + # serve. They answer with the API error body rather than the site's HTML + # error page. Each matches only slash-terminated addresses, so the redirect + # to the slash-terminated form still happens first. path( "courses//videos/", UnknownRouteView.as_view(), diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py index 89e654226fbe..4ec457e44b74 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py @@ -5,7 +5,6 @@ import os import subprocess import sys -import tempfile from datetime import datetime, timedelta from pathlib import Path from unittest.mock import Mock, patch @@ -37,7 +36,7 @@ from cms.djangoapps.contentstore.rest_api.v1.views.unknown_route import UnknownRouteView from cms.djangoapps.contentstore.rest_api.v1.views.video_uploads import CourseVideoUploadsViewSet from cms.djangoapps.contentstore.tests.utils import CourseTestCase -from cms.lib.spectacular import cms_api_filter, cms_mark_superseded_paths +from cms.lib.spectacular import cms_api_filter, cms_mark_migrated_paths from common.djangoapps.student.roles import ( CourseInstructorRole, CourseLimitedStaffRole, @@ -53,11 +52,11 @@ LEGACY_DETAIL_URL_NAME = "cms.djangoapps.contentstore:v0:cms_api_videos_uploads" ENUM_POSTPROCESSING_HOOK = "drf_spectacular.hooks.postprocess_schema_enums" -SUPERSEDED_PATHS_HOOK = "cms.lib.spectacular.cms_mark_superseded_paths" +MIGRATED_PATHS_HOOK = "cms.lib.spectacular.cms_mark_migrated_paths" # The post-processing hooks the CMS registers. Registering any hook replaces # drf-spectacular's own list, so the enum hook has to be named again alongside. -REGISTERED_POSTPROCESSING_HOOKS = [ENUM_POSTPROCESSING_HOOK, SUPERSEDED_PATHS_HOOK] +REGISTERED_POSTPROCESSING_HOOKS = [ENUM_POSTPROCESSING_HOOK, MIGRATED_PATHS_HOOK] PRODUCTION_SETTINGS = "cms.envs.production" DEVSTACK_SETTINGS = "cms.envs.devstack" @@ -65,22 +64,19 @@ REPO_ROOT = SETTINGS_DIRECTORY.parents[1] MOCK_CONFIG = SETTINGS_DIRECTORY / "mock.yml" -# The schema settings that decide the addresses the document publishes, and the -# subset of them a document generated in-process has to be given. -SCHEMA_SETTING_KEYS = ( +# The schema settings a document generated in this process has to be given for +# its addresses to come out the way the deployment publishes them. +GENERATION_SETTING_KEYS = ( "PREPROCESSING_HOOKS", "POSTPROCESSING_HOOKS", "SCHEMA_PATH_PREFIX", - "SCHEMA_PATH_PREFIX_TRIM", - "SERVERS", ) -GENERATION_SETTING_KEYS = SCHEMA_SETTING_KEYS[:-1] SETTINGS_MARKER = "schema settings: " -def schema_settings(module, config_file=MOCK_CONFIG): +def schema_settings(module): """ - Return the schema settings of the deployment settings module ``module``. + Return ``SPECTACULAR_SETTINGS`` of the deployment settings module ``module``. Deployment settings share their mutable defaults with the settings the test process runs under and would alter them on import, so they are read in a @@ -89,14 +85,14 @@ def schema_settings(module, config_file=MOCK_CONFIG): script = ( "import importlib, json, sys\n" "values = importlib.import_module(sys.argv[1]).SPECTACULAR_SETTINGS\n" - "print(sys.argv[2] + json.dumps({key: values[key] for key in sys.argv[3:]}))\n" + "print(sys.argv[2] + json.dumps(values))\n" ) completed = subprocess.run( - [sys.executable, "-c", script, module, SETTINGS_MARKER, *SCHEMA_SETTING_KEYS], + [sys.executable, "-c", script, module, SETTINGS_MARKER], capture_output=True, check=True, cwd=REPO_ROOT, - env={**os.environ, "CMS_CFG": str(config_file), "SERVICE_VARIANT": "cms"}, + env={**os.environ, "CMS_CFG": str(MOCK_CONFIG), "SERVICE_VARIANT": "cms"}, text=True, ) reported = next( @@ -1595,7 +1591,7 @@ def test_the_create_response_is_declared_as_the_upload_slots(self): class CmsSchemaHookTest(CourseVideoUploadsTestBase): - """The schema hooks admit the new paths and flag the superseded ones.""" + """The schema hooks admit the new paths and deprecate the legacy ones.""" def endpoint(self, path): return (path, path, "get", None) @@ -1611,16 +1607,18 @@ def test_filter_admits_the_authoring_prefix(self): "/api/contentstore/v0/videos/uploads/course-v1:a+b+c", ] - def test_post_processing_marks_only_the_superseded_operations(self): + def test_post_processing_marks_only_the_replaced_upload_operations(self): schema = { "paths": { - "/v0/videos/uploads/{course_id}": {"post": {}}, - "/v0/videos/uploads/{course_id}/{edx_video_id}": {"get": {}, "delete": {}}, - "/v0/videos/images/{course_id}/{edx_video_id}": {"post": {}}, + "/api/contentstore/v0/videos/uploads/{course_id}": {"post": {}}, + "/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}": { + "get": {}, "delete": {}, + }, + "/api/contentstore/v0/videos/images/{course_id}/{edx_video_id}": {"post": {}}, "/api/authoring/v1/courses/{course_key}/videos/": {"get": {}, "post": {}}, } } - result = cms_mark_superseded_paths(schema, None, None, False) + result = cms_mark_migrated_paths(schema, None, None, False) deprecated = { (path, method) @@ -1629,16 +1627,11 @@ def test_post_processing_marks_only_the_superseded_operations(self): if operation.get("deprecated") } assert deprecated == { - ("/v0/videos/uploads/{course_id}", "post"), - ("/v0/videos/uploads/{course_id}/{edx_video_id}", "get"), - ("/v0/videos/uploads/{course_id}/{edx_video_id}", "delete"), + ("/api/contentstore/v0/videos/uploads/{course_id}", "post"), + ("/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}", "get"), + ("/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}", "delete"), } - def test_post_processing_also_matches_untrimmed_paths(self): - schema = {"paths": {"/api/contentstore/v0/videos/uploads/{course_id}": {"post": {}}}} - result = cms_mark_superseded_paths(schema, None, None, False) - assert result["paths"]["/api/contentstore/v0/videos/uploads/{course_id}"]["post"]["deprecated"] is True - def authoring_schema(self, hooks): """Generate the authoring document with ``hooks`` as the post-processing list.""" from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls @@ -1649,7 +1642,7 @@ def authoring_schema(self, hooks): ) def test_enum_components_survive_the_registered_post_processing(self): - deprecation_only = self.authoring_schema([SUPERSEDED_PATHS_HOOK]) + deprecation_only = self.authoring_schema([MIGRATED_PATHS_HOOK]) registered = self.authoring_schema(REGISTERED_POSTPROCESSING_HOOKS) assert not [ @@ -1658,9 +1651,9 @@ def test_enum_components_survive_the_registered_post_processing(self): assert "ContentTypeEnum" in registered["components"]["schemas"] def test_post_processing_leaves_other_schema_members_alone(self): - schema = {"paths": {"/v0/videos/uploads/{course_id}": {"post": {}, "parameters": []}}} - result = cms_mark_superseded_paths(schema, None, None, False) - assert not result["paths"]["/v0/videos/uploads/{course_id}"]["parameters"] + path = "/api/contentstore/v0/videos/uploads/{course_id}" + result = cms_mark_migrated_paths({"paths": {path: {"post": {}, "parameters": []}}}, None, None, False) + assert not result["paths"][path]["parameters"] @ddt.ddt @@ -1693,17 +1686,6 @@ def cms_schema(self, module=PRODUCTION_SETTINGS): with patched_settings({key: read[key] for key in GENERATION_SETTING_KEYS}): return SchemaGenerator().get_schema(request=None, public=True) - def config_without_public_host(self): - """Write a deployment configuration that leaves the public host unset.""" - directory = tempfile.TemporaryDirectory() - self.addCleanup(directory.cleanup) - config = Path(directory.name) / "cms.yml" - config.write_text("".join( - line for line in MOCK_CONFIG.read_text().splitlines(keepends=True) - if not line.startswith("AUTHORING_API_URL:") - )) - return config - def test_the_document_publishes_the_mounted_addresses(self): assert set(self.PUBLISHED_PATHS) <= set(self.cms_schema()["paths"]) @@ -1717,22 +1699,26 @@ def test_no_published_path_is_shortened(self): assert published, "the pre-processing hook admitted no path" assert [path for path in published if not path.startswith("/api/")] == [] + def test_the_published_operation_ids_carry_the_version_not_the_service_prefix(self): + operations = self.cms_schema()["paths"]["/api/authoring/v1/courses/{course_key}/videos/"] + assert { + method: operation["operationId"] + for method, operation in operations.items() + if isinstance(operation, dict) + } == {"get": "v1_courses_videos_list", "post": "v1_courses_videos_create"} + def test_every_published_path_is_an_address_that_resolves(self): for published, address in self.PUBLISHED_PATHS.items(): assert resolve(address).func, published @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) def test_the_settings_publish_paths_in_full(self, module): - assert schema_settings(module)["SCHEMA_PATH_PREFIX_TRIM"] is False + settings_values = schema_settings(module) + assert "SCHEMA_PATH_PREFIX_TRIM" not in settings_values + assert settings_values["SCHEMA_PATH_PREFIX"] == r"/api/(contentstore|authoring)" @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) def test_the_servers_serve_every_published_path(self, module): servers = schema_settings(module)["SERVERS"] assert [server["description"] for server in servers] == ["Public", "Local"] assert [server for server in servers if "/api/contentstore" in server["url"]] == [] - - @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) - def test_an_unset_public_host_is_left_out_of_the_servers(self, module): - servers = schema_settings(module, config_file=self.config_without_public_host())["SERVERS"] - assert [server["description"] for server in servers] == ["Local"] - assert all(server["url"] for server in servers) diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index a2e5282309ea..6db9599d0c79 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -356,24 +356,18 @@ def should_show_debug_toolbar(request): # pylint: disable=missing-function-docs 'SERVE_INCLUDE_SCHEMA': False, # restrict spectacular to CMS API endpoints (cms/lib/spectacular.py): 'PREPROCESSING_HOOKS': ['cms.lib.spectacular.cms_api_filter'], - # Setting this replaces drf-spectacular's default hook list, so the default - # enum post-processing has to be listed again to keep running: + # 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_superseded_paths', + 'cms.lib.spectacular.cms_mark_migrated_paths', ], - # Paths are published in full, so one server URL serves every operation of the - # document. The prefix below is not removed from the paths; it only keeps the - # service prefix out of generated operation ids and tags: - 'SCHEMA_PATH_PREFIX': '/api/contentstore', - 'SCHEMA_PATH_PREFIX_TRIM': False, - # The public host is optional, so an unset URL is left out of the list: + # 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': url, 'description': description} - for url, description in [ - (AUTHORING_API_URL, 'Public'), # noqa: F405 - (f'http://{CMS_BASE}', 'Local'), - ] - if url + {'url': AUTHORING_API_URL, 'description': 'Public'}, # noqa: F405 + {'url': f'http://{CMS_BASE}', 'description': 'Local'}, ], } diff --git a/cms/envs/production.py b/cms/envs/production.py index bbe4930e49e9..452da2383e2c 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -415,25 +415,19 @@ def get_env_setting(setting): 'SERVE_INCLUDE_SCHEMA': False, # restrict spectacular to CMS API endpoints (cms/lib/spectacular.py): 'PREPROCESSING_HOOKS': ['cms.lib.spectacular.cms_api_filter'], - # Setting this replaces drf-spectacular's default hook list, so the default - # enum post-processing has to be listed again to keep running: + # 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_superseded_paths', + 'cms.lib.spectacular.cms_mark_migrated_paths', ], - # Paths are published in full, so one server URL serves every operation of the - # document. The prefix below is not removed from the paths; it only keeps the - # service prefix out of generated operation ids and tags: - 'SCHEMA_PATH_PREFIX': '/api/contentstore', - 'SCHEMA_PATH_PREFIX_TRIM': False, - # The public host is optional, so an unset URL is left out of the list: + # 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': url, 'description': description} - for url, description in [ - (AUTHORING_API_URL, 'Public'), # noqa: F405 - (f'https://{CMS_BASE}', 'Local'), # noqa: F405 - ] - if url + {'url': AUTHORING_API_URL, 'description': 'Public'}, # noqa: F405 + {'url': f'https://{CMS_BASE}', 'description': 'Local'}, # noqa: F405 ], } diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index af60402915b2..13d41c328fb6 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -2,26 +2,32 @@ import re -CMS_PATH_PATTERN = re.compile(r"^/api/(contentstore|authoring)/v\d+/") - -# Path prefixes of operations that have a conforming successor and are kept only -# for their deprecation window. The mounted prefix is listed next to the -# shortened form, so the hook still matches where a document is published -# without the service prefix. -SUPERSEDED_PATH_PREFIXES = ( - "/api/contentstore/v0/videos/uploads/", - "/v0/videos/uploads/", +# Legacy addresses of APIs migrated to /api/authoring/, marked deprecated for +# their deprecation window. +LEGACY_MIGRATED_PATH_PREFIXES = ( + "/api/contentstore/v1/xblock/", # → /api/authoring/v1/xblocks/ + "/api/contentstore/v3/home/", # → /api/authoring/v3/home/ + "/api/contentstore/v3/course_details/", # → /api/authoring/v3/courses/{course_key}/details/ + "/api/contentstore/v3/authoring_grading/", # → /api/authoring/v3/courses/{course_key}/grading/ + "/api/contentstore/v4/home/courses/", # → /api/authoring/v4/courses/ + "/api/contentstore/v0/videos/uploads/", # → /api/authoring/v1/courses/{course_key}/videos/ ) -_OPERATION_KEYS = frozenset({"get", "put", "post", "delete", "options", "head", "patch", "trace"}) +# BFF surfaces, marked x-internal so clients can tell them from a stable +# resource contract. Both the legacy and conforming mounts. +INTERNAL_BFF_PATH_PREFIXES = ( + "/api/contentstore/v3/home/", + "/api/authoring/v3/home/", +) def cms_api_filter(endpoints): """ - Pre-processing hook: keep the versioned authoring and contentstore - endpoints, and the course-level endpoints named below. + Pre-processing hook: keep only contentstore + authoring versioned + endpoints and select course-level endpoints. """ filtered = [] + CMS_PATH_PATTERN = re.compile(r"^/api/(contentstore|authoring)/v\d+/") for path, path_regex, method, callback in endpoints: if ( @@ -36,15 +42,21 @@ def cms_api_filter(endpoints): return filtered -def cms_mark_superseded_paths(result, generator, request, public): # pylint: disable=unused-argument +def cms_mark_migrated_paths(result, generator, request, public): # pylint: disable=unused-argument """ - Post-processing hook: mark every operation of a superseded path deprecated. + 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") or {}).items(): - if not path.startswith(SUPERSEDED_PATH_PREFIXES): + 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 key, operation in path_item.items(): - if key in _OPERATION_KEYS and isinstance(operation, dict): + 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 9bf84dc7ea86..68536a9ca817 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -27,8 +27,10 @@ from openedx.core.djangoapps.password_policy import compliance as password_policy_compliance from openedx.core.djangoapps.password_policy.forms import PasswordPolicyAwareAdminAuthForm -django_autodiscover() +# Shared opaque-key path converters, registered before any pattern using them. register_url_converters() + +django_autodiscover() admin.site.site_header = _('Studio Administration') admin.site.site_title = admin.site.site_header @@ -358,7 +360,7 @@ path('api/contentstore/', include('cms.djangoapps.contentstore.rest_api.urls')) ] -# Authoring REST APIs +# Authoring REST APIs — conforming addresses for resources standardized here. urlpatterns += [ path('api/authoring/v1/', include('cms.djangoapps.contentstore.rest_api.v1.authoring_urls')), ] From 2155ce938aefdef97091c13d471b95cc674c4a44 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Mon, 28 Sep 2026 01:20:20 +0500 Subject: [PATCH 05/15] fix: keep the v0 video upload deprecation out of the superseded code Review asked for the deprecation notices on the v0 video upload view to go (r4091236449, r4091238144), and made the same request on the sibling youtube transcripts change. The view goes back to exactly what master has. The deprecation is not lost. The three v0 operations stay marked deprecated in the published schema by the CMS post-processing hook, which lives outside the frozen view, and the deprecation is tracked on the FC-0118 umbrella DEPR ticket #39128. OEP-21 marks code only once a DEPR ticket is accepted, and then as a single line linking it; the multi-line notices were not that form. With nothing left in the superseded module, the versions gate passes without allowing any pre-existing version file to change. --- .../rest_api/v0/views/authoring_videos.py | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py b/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py index e5618a05b6cc..5a7563c2372e 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py @@ -1,12 +1,5 @@ """ Public rest API endpoints for the Authoring API video assets. - -.. deprecated:: - The video upload endpoints in this module are superseded by - ``CourseVideoUploadsViewSet`` in - ``cms.djangoapps.contentstore.rest_api.v1.views.video_uploads``. - Use ``/api/authoring/v1/courses/{course_key}/videos/`` going forward. - These v0 endpoints will be removed in a future release. """ import logging @@ -34,10 +27,6 @@ @view_auth_classes() class VideosUploadsView(DeveloperErrorViewMixin, RetrieveAPIView, DestroyAPIView): """ - **DEPRECATED** — use ``GET /api/authoring/v1/courses/{course_key}/videos/`` for the listing - this GET returns, and ``DELETE /api/authoring/v1/courses/{course_key}/videos/{edx_video_id}/`` - to remove one video from the course. These v0 endpoints will be removed in a future release. - public rest API endpoints for the CMS API video assets. course_key: required argument, needed to authorize course authors and identify the video. video_id: required argument, needed to identify the video. @@ -46,34 +35,17 @@ class VideosUploadsView(DeveloperErrorViewMixin, RetrieveAPIView, DestroyAPIView @course_author_access_required def retrieve(self, request, course_key, edx_video_id=None): # pylint: disable=arguments-differ - """ - **DEPRECATED** — use ``GET /api/authoring/v1/courses/{course_key}/videos/`` instead. - - This GET ignores ``edx_video_id`` and returns the whole course listing; - ``/api/authoring/v1/courses/{course_key}/videos/`` is the successor for that listing. - It will be removed in a future release. - """ return handle_videos(request, course_key.html_id(), edx_video_id) @course_author_access_required @expect_json_in_class_view def destroy(self, request, course_key, edx_video_id): # pylint: disable=arguments-differ - """ - **DEPRECATED** — use - ``DELETE /api/authoring/v1/courses/{course_key}/videos/{edx_video_id}/`` instead. - - It will be removed in a future release. - """ return handle_videos(request, course_key.html_id(), edx_video_id) @view_auth_classes() class VideosCreateUploadView(DeveloperErrorViewMixin, CreateAPIView): """ - **DEPRECATED** — use ``POST /api/authoring/v1/courses/{course_key}/videos/`` instead. - - These v0 endpoints will be removed in a future release. - public rest API endpoints for the CMS API video assets. course_key: required argument, needed to authorize course authors and identify the video. """ From f74beacfa5f7f091d1cd61e5f7d4baac2bf55727 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Mon, 28 Sep 2026 01:20:31 +0500 Subject: [PATCH 06/15] fix: scope this change's schema, errors and routes to its own API Three pieces of this change reached beyond the video uploads API. The schema hook, copied from the open URL-structure change so the two would merge cleanly, carried that change's data as well as its shape: five legacy prefixes and an x-internal stamp for resources whose new addresses only that change mounts. Here they would have marked the current version of v1 xblock, v3 home, course_details, authoring_grading and v4 home deprecated with nowhere to move, and hidden v3 home from generated clients. The hook now lists only the v0 video upload prefix, in a tuple of its own: that change suffixes the operationIds of every path in its migrated-path list, and a superseded version must not inherit that. The hook's test now includes an operation of every other current Studio version and asserts they are left alone; its old fixture held only video paths, so it could not fail. Registering request-level error types wrote to the error library's process-wide registry, so a malformed body, 405, 406 or 415 would have changed error type on every standardized Studio API at once. The registration is removed. Those four still come back in the error envelope with the right status; cataloguing them belongs in the library. The broadest fallback route claimed every address under a course, so any route a later change appends to this shared URLconf under courses/ would have become unreachable, the sibling youtube transcripts routes included. The fallbacks now claim video addresses only, and a test pins that a sibling address is left for its own route. --- .../rest_api/v1/authoring_urls.py | 19 ++-- .../contentstore/rest_api/v1/error_types.py | 30 ------- .../v1/views/tests/test_video_uploads.py | 88 ++++++++++++------- .../rest_api/v1/views/unknown_route.py | 11 ++- cms/lib/spectacular.py | 34 ++----- 5 files changed, 73 insertions(+), 109 deletions(-) delete mode 100644 cms/djangoapps/contentstore/rest_api/v1/error_types.py diff --git a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py index ba361e43c5a9..74f47e42fccd 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -2,14 +2,11 @@ from django.urls import path -from cms.djangoapps.contentstore.rest_api.v1.error_types import register_request_error_types from cms.djangoapps.contentstore.rest_api.v1.views.unknown_route import UnknownRouteView from cms.djangoapps.contentstore.rest_api.v1.views.video_uploads import CourseVideoUploadsViewSet app_name = "authoring_v1" -register_request_error_types() - urlpatterns = [ path( "courses//videos/", @@ -21,11 +18,12 @@ CourseVideoUploadsViewSet.as_view({"get": "retrieve", "delete": "destroy"}), name="course_video_detail", ), - # Reached only by a request the routes above turned down: a course key the - # converter refuses, or an address beneath a course that this API does not - # serve. They answer with the API error body rather than the site's HTML - # error page. Each matches only slash-terminated addresses, so the redirect - # to the slash-terminated form still happens first. + # Reached only by a video address whose course key the routes above refuse, + # malformed or in the deprecated slash-separated form. They answer with the + # API error body rather than the site's HTML error page. They match video + # addresses and nothing else, so a resource added under courses/ is never + # shadowed, and only slash-terminated ones, so the redirect to the + # slash-terminated form still happens first. path( "courses//videos/", UnknownRouteView.as_view(), @@ -36,9 +34,4 @@ UnknownRouteView.as_view(), name="course_video_detail_unmatched", ), - path( - "courses//", - UnknownRouteView.as_view(), - name="course_unmatched", - ), ] diff --git a/cms/djangoapps/contentstore/rest_api/v1/error_types.py b/cms/djangoapps/contentstore/rest_api/v1/error_types.py deleted file mode 100644 index 91e5d0b24d39..000000000000 --- a/cms/djangoapps/contentstore/rest_api/v1/error_types.py +++ /dev/null @@ -1,30 +0,0 @@ -"""Error-type catalog entries for request-level refusals.""" - -from edx_rest_framework_extensions.errors import register_error_type -from rest_framework.exceptions import ( - MethodNotAllowed, - NotAcceptable, - ParseError, - UnsupportedMediaType, -) - -#: Refusals of the request itself - its body, its method, its media type - with -#: the error type and title each is answered with. -_REQUEST_ERROR_TYPES = ( - (ParseError, "validation", "Malformed Request"), - (MethodNotAllowed, "method-not-allowed", "Method Not Allowed"), - (NotAcceptable, "not-acceptable", "Not Acceptable"), - (UnsupportedMediaType, "unsupported-media-type", "Unsupported Media Type"), -) - - -def register_request_error_types(): - """ - Give each request-level refusal its own error type. - - Uncataloged errors are answered with the type and title of an internal - server error while keeping their 4xx status, which tells a client that its - own unparseable body or unsupported method was a fault of the server. - """ - for exception_class, slug, title in _REQUEST_ERROR_TYPES: - register_error_type(exception_class, slug, title) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py index 4ec457e44b74..880dcf93cfa5 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py @@ -778,37 +778,27 @@ def test_destroy_404_when_pipeline_disabled(self): class CourseVideoRequestErrorTest(CourseVideoUploadsTestBase): - """A request the API refuses to read is reported as the caller's error, not the server's.""" + """A request the API refuses to read gets its status in the standard error body.""" - def test_a_malformed_body_is_reported_as_a_client_error(self): + def test_a_malformed_body_is_refused(self): response = self.api_client.post( self.list_url, data="{not json", content_type="application/json" ) - envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") - assert envelope["title"] == "Malformed Request" + assert_error_envelope(response, expected_status=400) - def test_an_unsupported_method_is_reported_as_such(self): + def test_an_unsupported_method_is_refused(self): response = self.api_client.put(self.list_url, {}, format="json") - envelope = assert_error_envelope( - response, expected_status=405, expected_type_slug="method-not-allowed" - ) - assert envelope["title"] == "Method Not Allowed" + assert_error_envelope(response, expected_status=405) - def test_an_unsupported_media_type_is_reported_as_such(self): + def test_an_unsupported_media_type_is_refused(self): response = self.api_client.post( self.list_url, data="file_name,content_type", content_type="text/csv" ) - envelope = assert_error_envelope( - response, expected_status=415, expected_type_slug="unsupported-media-type" - ) - assert envelope["title"] == "Unsupported Media Type" + assert_error_envelope(response, expected_status=415) - def test_an_unsatisfiable_accept_header_is_reported_as_such(self): + def test_an_unsatisfiable_accept_header_is_refused(self): response = self.api_client.get(self.list_url, HTTP_ACCEPT="application/xml") - envelope = assert_error_envelope( - response, expected_status=406, expected_type_slug="not-acceptable" - ) - assert envelope["title"] == "Not Acceptable" + assert_error_envelope(response, expected_status=406) @ddt.ddt @@ -1113,8 +1103,17 @@ def test_the_legacy_list_address_still_accepts_a_deprecated_course_key(self): def test_malformed_course_key_is_not_routed(self): self.assert_json_not_found("/api/authoring/v1/courses/a+b+c/videos/") - def test_an_unknown_address_under_a_course_is_not_routed(self): - self.assert_json_not_found("/api/authoring/v1/courses/course-v1:edX+DemoX+Demo_Course/no_such_thing/") + def test_a_sibling_resource_under_a_course_is_left_for_its_own_route(self): + """ + The fallbacks claim video addresses only. + + Other resources share this URLconf under courses/, so a fallback that + claimed every course address would shadow any route added after it. + """ + from django.urls import Resolver404 + + with pytest.raises(Resolver404): + resolve("/api/authoring/v1/courses/course-v1:edX+DemoX+Demo_Course/youtube_transcript_checks/") def test_an_unknown_member_of_a_course_is_not_routed(self): self.assert_json_not_found("/api/authoring/v1/courses/a+b+c/videos/abc-123/") @@ -1140,8 +1139,6 @@ def test_unmatched_addresses_are_claimed_by_routes_that_can_be_reversed(self): f"/api/authoring/v1/courses/{unusable_key}/videos/", "authoring_v1:course_video_detail_unmatched": f"/api/authoring/v1/courses/{unusable_key}/videos/abc-123/", - "authoring_v1:course_unmatched": - f"/api/authoring/v1/courses/{unusable_key}/", } kwargs = { "authoring_v1:course_video_detail_unmatched": {"edx_video_id": "abc-123"}, @@ -1607,18 +1604,31 @@ def test_filter_admits_the_authoring_prefix(self): "/api/contentstore/v0/videos/uploads/course-v1:a+b+c", ] - def test_post_processing_marks_only_the_replaced_upload_operations(self): - schema = { - "paths": { - "/api/contentstore/v0/videos/uploads/{course_id}": {"post": {}}, - "/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}": { - "get": {}, "delete": {}, - }, - "/api/contentstore/v0/videos/images/{course_id}/{edx_video_id}": {"post": {}}, - "/api/authoring/v1/courses/{course_key}/videos/": {"get": {}, "post": {}}, - } + # One operation of every other current Studio API version published under + # /api/contentstore/. None of them has a successor mounted by this change, so + # marking any of them would tell clients to leave the version they should use. + OTHER_CURRENT_VERSION_PATHS = ( + "/api/contentstore/v1/xblock/{usage_key_string}/", + "/api/contentstore/v3/home/", + "/api/contentstore/v3/course_details/{course_key}/", + "/api/contentstore/v3/authoring_grading/{course_key}/", + "/api/contentstore/v4/home/courses/", + ) + + def superseded_schema(self): + paths = { + "/api/contentstore/v0/videos/uploads/{course_id}": {"post": {}}, + "/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}": { + "get": {}, "delete": {}, + }, + "/api/contentstore/v0/videos/images/{course_id}/{edx_video_id}": {"post": {}}, + "/api/authoring/v1/courses/{course_key}/videos/": {"get": {}, "post": {}}, } - result = cms_mark_migrated_paths(schema, None, None, False) + paths.update({path: {"get": {}} for path in self.OTHER_CURRENT_VERSION_PATHS}) + return cms_mark_migrated_paths({"paths": paths}, None, None, False) + + def test_post_processing_marks_only_the_replaced_upload_operations(self): + result = self.superseded_schema() deprecated = { (path, method) @@ -1632,6 +1642,16 @@ def test_post_processing_marks_only_the_replaced_upload_operations(self): ("/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}", "delete"), } + def test_post_processing_marks_no_operation_internal(self): + result = self.superseded_schema() + + assert not [ + (path, method) + for path, item in result["paths"].items() + for method, operation in item.items() + if "x-internal" in operation + ] + def authoring_schema(self, hooks): """Generate the authoring document with ``hooks`` as the post-processing list.""" from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py b/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py index dd089c0163fe..e551794a8507 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py @@ -10,13 +10,12 @@ @extend_schema(exclude=True) class UnknownRouteView(StandardizedErrorMixin, APIView): """ - Answer an address under a course that no endpoint of this API serves. + Answer a video address whose course key the conforming routes refuse. - A course key that is malformed or in the deprecated slash-separated form, or - a misspelt address beneath a course, would otherwise be answered by the - site's HTML error page, which an API client cannot read. Nothing here needs - the caller's identity, so the answer is the same for everyone: the address - holds no resource. + A key that is malformed or in the deprecated slash-separated form fails to + resolve, and would otherwise be answered by the site's HTML error page, which + an API client cannot read. Nothing here needs the caller's identity, so the + answer is the same for everyone: the address holds no resource. """ permission_classes = (AllowAny,) diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 13d41c328fb6..d6681e4006b2 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -2,22 +2,10 @@ import re -# Legacy addresses of APIs migrated to /api/authoring/, marked deprecated for -# their deprecation window. -LEGACY_MIGRATED_PATH_PREFIXES = ( - "/api/contentstore/v1/xblock/", # → /api/authoring/v1/xblocks/ - "/api/contentstore/v3/home/", # → /api/authoring/v3/home/ - "/api/contentstore/v3/course_details/", # → /api/authoring/v3/courses/{course_key}/details/ - "/api/contentstore/v3/authoring_grading/", # → /api/authoring/v3/courses/{course_key}/grading/ - "/api/contentstore/v4/home/courses/", # → /api/authoring/v4/courses/ - "/api/contentstore/v0/videos/uploads/", # → /api/authoring/v1/courses/{course_key}/videos/ -) - -# BFF surfaces, marked x-internal so clients can tell them from a stable -# resource contract. Both the legacy and conforming mounts. -INTERNAL_BFF_PATH_PREFIXES = ( - "/api/contentstore/v3/home/", - "/api/authoring/v3/home/", +# Path prefixes of operations superseded by a newer version, kept for their +# deprecation window. +SUPERSEDED_PATH_PREFIXES = ( + "/api/contentstore/v0/videos/uploads/", # → /api/authoring/v1/courses/{course_key}/videos/ ) @@ -44,19 +32,13 @@ def cms_api_filter(endpoints): def cms_mark_migrated_paths(result, generator, request, public): # pylint: disable=unused-argument """ - Post-processing hook: mark the legacy addresses of - migrated APIs ``deprecated: true`` and BFF surfaces ``x-internal``. + Post-processing hook: mark the operations that have a conforming successor + ``deprecated: true``. """ 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): + if not path.startswith(SUPERSEDED_PATH_PREFIXES): continue for operation in path_item.values(): - if not isinstance(operation, dict): - continue - if legacy: + if isinstance(operation, dict): operation["deprecated"] = True - if internal: - operation["x-internal"] = True return result From c55fdec59f1be3ba1888f894a978baa7c3c0cda0 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Mon, 28 Sep 2026 01:44:17 +0500 Subject: [PATCH 07/15] test: document the hook fixture helper The helper that builds the post-processing hook's fixture is long enough that the repository's pylint configuration requires a docstring on it, and CI's pylint check failed without one. --- .../contentstore/rest_api/v1/views/tests/test_video_uploads.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py index 880dcf93cfa5..e3c074f5e842 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py @@ -1616,6 +1616,7 @@ def test_filter_admits_the_authoring_prefix(self): ) def superseded_schema(self): + """Run the post-processing hook over the upload paths and one of every other version.""" paths = { "/api/contentstore/v0/videos/uploads/{course_id}": {"post": {}}, "/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}": { From 1d945f36788dc93bba7490e687f9458f36be2112 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Wed, 14 Oct 2026 12:20:32 +0500 Subject: [PATCH 08/15] fix: answer 404 for version-only course keys, and realign the schema tests A course key that names only a version parses and carries no slash, so the course_key path converter accepts it and hands it to the view. It names no course run, and the role lookup the permission check runs raises InvalidKeyError for such a key rather than refusing it, so every operation behind this permission class answered 500. Refuse the key as a missing course before the role lookup; the two routes already on master that use this permission class are repaired with it. The schema tests were written against three premises that no longer hold. The service settings now register cms_mark_superseded_paths alongside the other two hooks and define the schema settings at the common level, so a document generated from the authoring URLconf alone is filtered away by the pre-processing hook unless it is generated under the prefix the service mounts it at; generate it there and name the published paths in full. Every legacy address the migrated-paths hook knows about now has a conforming successor, so the assertions that no other version is deprecated and that nothing is x-internal described the old single-successor list; narrow them to the video addresses this change migrates and the sibling video addresses it leaves alone. A sibling course resource now has a route of its own, so the fallbacks are shown not to shadow it rather than shown to leave the address unrouted. The settings and hook-mechanics assertions that cms/lib/tests/test_spectacular.py now owns are dropped rather than kept in two places. Co-Authored-By: Claude Opus 5 --- .../rest_api/v1/views/permissions.py | 9 +- .../v1/views/tests/test_video_uploads.py | 152 ++++++++++-------- 2 files changed, 96 insertions(+), 65 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py b/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py index 3b60607eebec..a1b52f495234 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py @@ -3,6 +3,7 @@ """ import logging +from rest_framework.exceptions import NotFound from rest_framework.permissions import BasePermission from common.djangoapps.student.auth import has_course_author_access @@ -17,11 +18,17 @@ class HasCourseAuthorAccess(BasePermission): Reads ``view.kwargs["course_key"]`` (a CourseKey instance) that is injected by XblockViewSet.initial() before DRF runs permission checks. Returns 403 if the authenticated user lacks authoring rights on that - course, or if no course key could be derived. + course, or if no course key could be derived, and 404 for a key that + names only a version, which has no course run to grant access to. """ def has_permission(self, request, view): course_key = getattr(view, "course_key", None) if not course_key: return False + # A key that names only a version has no course run behind it, so the + # role lookup raises InvalidKeyError instead of refusing it. There is + # nothing to grant access to, so the address holds no course. + if course_key.org is None: + raise NotFound(f"Course {course_key} not found.") return has_course_author_access(request.user, course_key) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py index e3c074f5e842..7fa95e232e98 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py @@ -51,15 +51,32 @@ LEGACY_LIST_URL_NAME = "cms.djangoapps.contentstore:v0:cms_api_create_videos_upload" LEGACY_DETAIL_URL_NAME = "cms.djangoapps.contentstore:v0:cms_api_videos_uploads" +# A key that names a version and no course run. It parses, and the path +# converter therefore accepts it, but no course stands behind it. +VERSION_ONLY_COURSE_KEY = "course-v1:version@5f8d0d55b54eba0b97d3dba5" + ENUM_POSTPROCESSING_HOOK = "drf_spectacular.hooks.postprocess_schema_enums" +SUPERSEDED_PATHS_HOOK = "cms.lib.spectacular.cms_mark_superseded_paths" MIGRATED_PATHS_HOOK = "cms.lib.spectacular.cms_mark_migrated_paths" # The post-processing hooks the CMS registers. Registering any hook replaces # drf-spectacular's own list, so the enum hook has to be named again alongside. -REGISTERED_POSTPROCESSING_HOOKS = [ENUM_POSTPROCESSING_HOOK, MIGRATED_PATHS_HOOK] +REGISTERED_POSTPROCESSING_HOOKS = [ + ENUM_POSTPROCESSING_HOOK, + SUPERSEDED_PATHS_HOOK, + MIGRATED_PATHS_HOOK, +] + +# Where the authoring URLconf under test is mounted. The pre-processing hook +# admits paths by this prefix, so a document generated from the URLconf alone +# has to be generated under it. +AUTHORING_V1_PREFIX = "api/authoring/v1/" + +# The two addresses this change publishes, spelled as the document spells them. +VIDEO_LIST_PATH = "/api/authoring/v1/courses/{course_key}/videos/" +VIDEO_DETAIL_PATH = "/api/authoring/v1/courses/{course_key}/videos/{edx_video_id}/" PRODUCTION_SETTINGS = "cms.envs.production" -DEVSTACK_SETTINGS = "cms.envs.devstack" SETTINGS_DIRECTORY = Path(cms.envs.__file__).resolve().parent REPO_ROOT = SETTINGS_DIRECTORY.parents[1] MOCK_CONFIG = SETTINGS_DIRECTORY / "mock.yml" @@ -74,6 +91,15 @@ SETTINGS_MARKER = "schema settings: " +def authoring_v1_patterns(): + """Return the authoring v1 URLconf mounted where the service mounts it.""" + from django.urls import include, path + + from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls + + return [path(AUTHORING_V1_PREFIX, include(authoring_urls))] + + def schema_settings(module): """ Return ``SPECTACULAR_SETTINGS`` of the deployment settings module ``module``. @@ -1103,17 +1129,41 @@ def test_the_legacy_list_address_still_accepts_a_deprecated_course_key(self): def test_malformed_course_key_is_not_routed(self): self.assert_json_not_found("/api/authoring/v1/courses/a+b+c/videos/") + def test_a_version_only_course_key_is_not_found(self): + """ + A key naming only a version parses, so the routes accept it. + + It names no course run, so the role lookup behind the permission check + cannot take it; the address holds no course and every operation says so + rather than failing. + """ + version_only = CourseKey.from_string(VERSION_ONLY_COURSE_KEY) + collection = reverse(LIST_URL_NAME, kwargs={"course_key": version_only}) + member = self.detail_url("abc-123", course_key=version_only) + requests = [ + self.api_client.get(collection), + self.api_client.post(collection, {"files": []}, format="json"), + self.api_client.get(member), + self.api_client.delete(member), + ] + for response in requests: + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + def test_a_sibling_resource_under_a_course_is_left_for_its_own_route(self): """ The fallbacks claim video addresses only. Other resources share this URLconf under courses/, so a fallback that - claimed every course address would shadow any route added after it. + claimed every course address would shadow them. """ from django.urls import Resolver404 + sibling = resolve( + "/api/authoring/v1/courses/course-v1:edX+DemoX+Demo_Course/youtube_transcript_checks/" + ) + assert sibling.url_name == "youtube_transcript_check_list" with pytest.raises(Resolver404): - resolve("/api/authoring/v1/courses/course-v1:edX+DemoX+Demo_Course/youtube_transcript_checks/") + resolve("/api/authoring/v1/courses/course-v1:edX+DemoX+Demo_Course/no_such_resource/") def test_an_unknown_member_of_a_course_is_not_routed(self): self.assert_json_not_found("/api/authoring/v1/courses/a+b+c/videos/abc-123/") @@ -1450,10 +1500,8 @@ class CourseVideoSchemaTest(CourseVideoUploadsTestBase): """What the generated schema promises, measured against what the endpoints return.""" def generated_schema(self): - """Generate the OpenAPI document for the course video addresses.""" - from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls - - return SchemaGenerator(patterns=authoring_urls.urlpatterns).get_schema(request=None, public=True) + """Generate the OpenAPI document for the authoring v1 addresses.""" + return SchemaGenerator(patterns=authoring_v1_patterns()).get_schema(request=None, public=True) def resolve(self, schema, body): """Return the component ``body`` refers to, or ``body`` itself.""" @@ -1477,9 +1525,10 @@ def representations(self, schema, declared): def test_only_the_two_video_addresses_are_documented(self): """The routes that stand behind the endpoints serve no operation and must stay unpublished.""" - assert set(self.generated_schema()["paths"]) == { - "/courses/{course_key}/videos/", - "/courses/{course_key}/videos/{edx_video_id}/", + published = set(self.generated_schema()["paths"]) + assert {path for path in published if "/videos/" in path} == { + VIDEO_LIST_PATH, + VIDEO_DETAIL_PATH, } def test_the_list_response_is_declared_as_the_page_envelope(self): @@ -1525,7 +1574,7 @@ def test_the_member_response_declares_both_representations(self): schema = self.generated_schema() video_ids = self.create_videos(count=1) declared = self.representations( - schema, schema["paths"]["/courses/{course_key}/videos/{edx_video_id}/"]["get"][ + schema, schema["paths"][VIDEO_DETAIL_PATH]["get"][ "responses"]["200"]["content"]["application/json"]["schema"] ) assert set(declared) == {"CourseVideo", "CourseVideoMinimal"} @@ -1536,7 +1585,7 @@ def test_the_member_response_declares_both_representations(self): def test_the_minimal_representation_is_named_by_the_view_parameter(self): schema = self.generated_schema() - parameters = schema["paths"]["/courses/{course_key}/videos/"]["get"]["parameters"] + parameters = schema["paths"][VIDEO_LIST_PATH]["get"]["parameters"] view_parameter = next(item for item in parameters if item["name"] == "view") assert view_parameter["schema"]["enum"] == ["minimal"] declared = self.representations( @@ -1554,7 +1603,7 @@ def declared_examples(self, schema, path, method, code): def test_the_member_404_names_both_causes(self): schema = self.generated_schema() declared = self.declared_examples( - schema, "/courses/{course_key}/videos/{edx_video_id}/", "get", "404" + schema, VIDEO_DETAIL_PATH, "get", "404" ) assert {example["type"] for example in declared.values()} == { "https://docs.openedx.org/errors/videos/uploads-not-configured", @@ -1566,7 +1615,7 @@ def test_the_declared_404_types_are_the_ones_returned(self): declared = { example["type"] for example in self.declared_examples( - schema, "/courses/{course_key}/videos/{edx_video_id}/", "get", "404" + schema, VIDEO_DETAIL_PATH, "get", "404" ).values() } video_ids = self.create_videos(count=1) @@ -1577,7 +1626,7 @@ def test_the_declared_404_types_are_the_ones_returned(self): def test_the_create_404_names_only_the_cause_it_can_have(self): schema = self.generated_schema() - declared = self.declared_examples(schema, "/courses/{course_key}/videos/", "post", "404") + declared = self.declared_examples(schema, VIDEO_LIST_PATH, "post", "404") assert {example["type"] for example in declared.values()} == { "https://docs.openedx.org/errors/videos/uploads-not-configured", } @@ -1604,28 +1653,20 @@ def test_filter_admits_the_authoring_prefix(self): "/api/contentstore/v0/videos/uploads/course-v1:a+b+c", ] - # One operation of every other current Studio API version published under - # /api/contentstore/. None of them has a successor mounted by this change, so - # marking any of them would tell clients to leave the version they should use. - OTHER_CURRENT_VERSION_PATHS = ( - "/api/contentstore/v1/xblock/{usage_key_string}/", - "/api/contentstore/v3/home/", - "/api/contentstore/v3/course_details/{course_key}/", - "/api/contentstore/v3/authoring_grading/{course_key}/", - "/api/contentstore/v4/home/courses/", - ) - def superseded_schema(self): - """Run the post-processing hook over the upload paths and one of every other version.""" + """Run the post-processing hook over the upload paths and their unmigrated siblings.""" paths = { "/api/contentstore/v0/videos/uploads/{course_id}": {"post": {}}, "/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}": { "get": {}, "delete": {}, }, + # Video addresses this change does not migrate, so the prefix must + # not reach them. "/api/contentstore/v0/videos/images/{course_id}/{edx_video_id}": {"post": {}}, + "/api/contentstore/v0/videos/encodings/{course_id}": {"get": {}}, + "/api/contentstore/v0/videos/features": {"get": {}}, "/api/authoring/v1/courses/{course_key}/videos/": {"get": {}, "post": {}}, } - paths.update({path: {"get": {}} for path in self.OTHER_CURRENT_VERSION_PATHS}) return cms_mark_migrated_paths({"paths": paths}, None, None, False) def test_post_processing_marks_only_the_replaced_upload_operations(self): @@ -1643,22 +1684,10 @@ def test_post_processing_marks_only_the_replaced_upload_operations(self): ("/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}", "delete"), } - def test_post_processing_marks_no_operation_internal(self): - result = self.superseded_schema() - - assert not [ - (path, method) - for path, item in result["paths"].items() - for method, operation in item.items() - if "x-internal" in operation - ] - def authoring_schema(self, hooks): """Generate the authoring document with ``hooks`` as the post-processing list.""" - from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls - with patched_settings({"POSTPROCESSING_HOOKS": hooks}): - return SchemaGenerator(patterns=authoring_urls.urlpatterns).get_schema( + return SchemaGenerator(patterns=authoring_v1_patterns()).get_schema( request=None, public=True ) @@ -1671,13 +1700,7 @@ def test_enum_components_survive_the_registered_post_processing(self): ] assert "ContentTypeEnum" in registered["components"]["schemas"] - def test_post_processing_leaves_other_schema_members_alone(self): - path = "/api/contentstore/v0/videos/uploads/{course_id}" - result = cms_mark_migrated_paths({"paths": {path: {"post": {}, "parameters": []}}}, None, None, False) - assert not result["paths"][path]["parameters"] - -@ddt.ddt class CmsSchemaAddressTest(TestCase): """The published document addresses every operation the way it is mounted.""" @@ -1695,9 +1718,9 @@ class CmsSchemaAddressTest(TestCase): "/api/contentstore/v3/course_details/course-v1:a+b+c/", "/api/contentstore/v4/home/courses/": "/api/contentstore/v4/home/courses/", - "/api/authoring/v1/courses/{course_key}/videos/": + VIDEO_LIST_PATH: "/api/authoring/v1/courses/course-v1:a+b+c/videos/", - "/api/authoring/v1/courses/{course_key}/videos/{edx_video_id}/": + VIDEO_DETAIL_PATH: "/api/authoring/v1/courses/course-v1:a+b+c/videos/video-1/", } @@ -1710,10 +1733,17 @@ def cms_schema(self, module=PRODUCTION_SETTINGS): def test_the_document_publishes_the_mounted_addresses(self): assert set(self.PUBLISHED_PATHS) <= set(self.cms_schema()["paths"]) - def test_the_document_publishes_no_other_authoring_address(self): + def test_the_document_publishes_no_other_video_address(self): + """The routes standing behind the endpoints serve no operation and stay unpublished.""" published = self.cms_schema()["paths"] - authoring = {path for path in published if path.startswith("/api/authoring/")} - assert authoring == {path for path in self.PUBLISHED_PATHS if path.startswith("/api/authoring/")} + authoring_videos = { + path for path in published + if path.startswith("/api/authoring/") and "/videos/" in path + } + assert authoring_videos == { + path for path in self.PUBLISHED_PATHS + if path.startswith("/api/authoring/") and "/videos/" in path + } def test_no_published_path_is_shortened(self): published = sorted(self.cms_schema()["paths"]) @@ -1732,14 +1762,8 @@ def test_every_published_path_is_an_address_that_resolves(self): for published, address in self.PUBLISHED_PATHS.items(): assert resolve(address).func, published - @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) - def test_the_settings_publish_paths_in_full(self, module): - settings_values = schema_settings(module) - assert "SCHEMA_PATH_PREFIX_TRIM" not in settings_values - assert settings_values["SCHEMA_PATH_PREFIX"] == r"/api/(contentstore|authoring)" - - @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) - def test_the_servers_serve_every_published_path(self, module): - servers = schema_settings(module)["SERVERS"] - assert [server["description"] for server in servers] == ["Public", "Local"] - assert [server for server in servers if "/api/contentstore" in server["url"]] == [] + def test_the_registered_hooks_are_the_ones_generated_with(self): + """The document under test is generated with the hooks the deployment registers.""" + assert schema_settings(PRODUCTION_SETTINGS)["POSTPROCESSING_HOOKS"] == ( + REGISTERED_POSTPROCESSING_HOOKS + ) From 800d10b6c0c44a7ae89c7b9ea131352a59cd4970 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Wed, 14 Oct 2026 12:23:51 +0500 Subject: [PATCH 09/15] refactor: hand the video upload course lookup the parsed course key The conforming route's path converter gives the view an already-parsed CourseKey, which the service turned back into a string so the course lookup could parse it again. Let the lookup take either form, as the legacy routes still carry the raw string, and pass the parsed key straight through. Co-Authored-By: Claude Opus 5 --- .../contentstore/rest_api/v1/video_uploads_service.py | 2 +- cms/djangoapps/contentstore/video_storage_handlers.py | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py b/cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py index 7001a435cdef..5bb7911723a5 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py +++ b/cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py @@ -102,7 +102,7 @@ def get_course_for_uploads(course_key, user): no real course is needed. """ try: - course = _get_and_validate_course(str(course_key), user) + course = _get_and_validate_course(course_key, user) except DjangoPermissionDenied as error: log.info("Course [%s] is not readable in Studio by user [%s]", course_key, user.id) raise PermissionDenied() from error diff --git a/cms/djangoapps/contentstore/video_storage_handlers.py b/cms/djangoapps/contentstore/video_storage_handlers.py index d7e40dc1af8c..9b7195dcff36 100644 --- a/cms/djangoapps/contentstore/video_storage_handlers.py +++ b/cms/djangoapps/contentstore/video_storage_handlers.py @@ -631,12 +631,16 @@ def make_csv_dict(video): return FileResponse(buffer, as_attachment=True, filename=filename, content_type="text/csv") -def _get_and_validate_course(course_key_string, user): +def _get_and_validate_course(course_key, user): """ Given a course key, return the course if it exists, the given user has access to it, and it is properly configured for video uploads + + Accepts the raw string the legacy routes carry or the parsed key the + conforming routes' path converter hands their views. """ - course_key = CourseKey.from_string(course_key_string) + if not isinstance(course_key, CourseKey): + course_key = CourseKey.from_string(course_key) # For now, assume all studio users that have access to the course can upload videos. # In the future, we plan to add a new org-level role for video uploaders. From a6393f741182f347d5d6060d948e8add6dc380cb Mon Sep 17 00:00:00 2001 From: Claude Opus 5 Date: Wed, 14 Oct 2026 12:41:26 +0500 Subject: [PATCH 10/15] fix: keep the existing content_type enum on its published name Adding a second ``content_type`` ChoiceField makes drf-spectacular stop using the unprefixed enum name for either field and rename both after their serializers, so ``ContentTypeEnum`` -- which the generated SDK already has a model for -- would disappear and come back as ``FileSpecContentTypeEnum``. Pinning both names keeps the existing component where it is and gives the new one a name of its own. Co-Authored-By: Claude Opus 5 --- cms/envs/common.py | 13 +++++++++++++ cms/envs/devstack.py | 13 +++++++++++++ cms/envs/production.py | 13 +++++++++++++ 3 files changed, 39 insertions(+) diff --git a/cms/envs/common.py b/cms/envs/common.py index 87ee13f3e0a6..699769ca8331 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1342,6 +1342,19 @@ def _should_send_xblock_events(settings): # service root, so /api/contentstore and /api/authoring resolve against the # same base URL. 'SCHEMA_PATH_PREFIX': r'/api/(contentstore|authoring)', + # ``content_type`` appears on FileSpecSerializer as well, and a bare enum + # name is only used while one serializer claims it: a second one makes + # drf-spectacular rename both after their serializers, moving the existing + # ContentTypeEnum off the name the generated SDK already uses. Naming this + # one keeps the other where it is. + 'ENUM_NAME_OVERRIDES': { + # FileSpecSerializer's content_type held the unprefixed name while it was + # the only such field. Both names are pinned here so adding the second + # one does not move the first. + 'ContentTypeEnum': ['video/mp4', 'video/webm', 'video/ogg'], + 'VideoUploadContentTypeEnum': + 'cms.djangoapps.contentstore.rest_api.v1.serializers.video_uploads.SUPPORTED_UPLOAD_CONTENT_TYPES', + }, } ################### Studio Search (beta), using Meilisearch ################### diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index e3fe5f22a3d6..2dffb60d7206 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -368,6 +368,19 @@ def should_show_debug_toolbar(request): # pylint: disable=missing-function-docs # it renames every operationId. Paths themselves are emitted in full so # they resolve against the service-root SERVERS below. 'SCHEMA_PATH_PREFIX': r'/api/(contentstore|authoring)', + # ``content_type`` appears on FileSpecSerializer as well, and a bare enum + # name is only used while one serializer claims it: a second one makes + # drf-spectacular rename both after their serializers, moving the existing + # ContentTypeEnum off the name the generated SDK already uses. Naming this + # one keeps the other where it is. + 'ENUM_NAME_OVERRIDES': { + # FileSpecSerializer's content_type held the unprefixed name while it was + # the only such field. Both names are pinned here so adding the second + # one does not move the first. + 'ContentTypeEnum': ['video/mp4', 'video/webm', 'video/ogg'], + 'VideoUploadContentTypeEnum': + 'cms.djangoapps.contentstore.rest_api.v1.serializers.video_uploads.SUPPORTED_UPLOAD_CONTENT_TYPES', + }, # Serve the Swagger UI and ReDoc assets from drf-spectacular-sidecar rather # than drf-spectacular's default unpinned jsdelivr CDN URLs. 'SWAGGER_UI_DIST': 'SIDECAR', diff --git a/cms/envs/production.py b/cms/envs/production.py index 596d70c9f9cc..827c56825bb1 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -427,6 +427,19 @@ def get_env_setting(setting): # it renames every operationId. Paths themselves are emitted in full so # they resolve against the service-root SERVERS below. 'SCHEMA_PATH_PREFIX': r'/api/(contentstore|authoring)', + # ``content_type`` appears on FileSpecSerializer as well, and a bare enum + # name is only used while one serializer claims it: a second one makes + # drf-spectacular rename both after their serializers, moving the existing + # ContentTypeEnum off the name the generated SDK already uses. Naming this + # one keeps the other where it is. + 'ENUM_NAME_OVERRIDES': { + # FileSpecSerializer's content_type held the unprefixed name while it was + # the only such field. Both names are pinned here so adding the second + # one does not move the first. + 'ContentTypeEnum': ['video/mp4', 'video/webm', 'video/ogg'], + 'VideoUploadContentTypeEnum': + 'cms.djangoapps.contentstore.rest_api.v1.serializers.video_uploads.SUPPORTED_UPLOAD_CONTENT_TYPES', + }, # Serve the Swagger UI and ReDoc assets from drf-spectacular-sidecar rather # than drf-spectacular's default unpinned jsdelivr CDN URLs. 'SWAGGER_UI_DIST': 'SIDECAR', From b72ca7ae81e968ec9fd320c51b768a3c327a5997 Mon Sep 17 00:00:00 2001 From: Claude Opus 5 Date: Wed, 14 Oct 2026 12:47:28 +0500 Subject: [PATCH 11/15] test: assert the pinned name of this API's content_type enum The assertion named the unprefixed enum, which this API only held while no other serializer had a content_type field. It now reads the pinned name, so a sibling API growing that field cannot break it. Co-Authored-By: Claude Opus 5 --- .../rest_api/v1/views/tests/test_video_uploads.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py index 7fa95e232e98..70d33ecfc4bc 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py @@ -1698,7 +1698,9 @@ def test_enum_components_survive_the_registered_post_processing(self): assert not [ name for name in deprecation_only["components"]["schemas"] if name.endswith("Enum") ] - assert "ContentTypeEnum" in registered["components"]["schemas"] + # The name is pinned in ENUM_NAME_OVERRIDES, so it does not move when + # another serializer grows a content_type field. + assert "VideoUploadContentTypeEnum" in registered["components"]["schemas"] class CmsSchemaAddressTest(TestCase): From e07db66c13455658b5e4f99648a1a70e96f5f25b Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Wed, 21 Oct 2026 12:07:34 +0500 Subject: [PATCH 12/15] refactor: reuse the legacy video status logic in the v1 service The v1 service re-implemented convert_video_status, the transcription rule and the _get_index_videos row extraction so it could skip the status write. It now calls the handlers' own code: - convert_video_status takes update_status=True; with False it reports a stuck upload as failed without calling send_video_status_update. - _get_videos' per-video loop is convert_videos(course, videos, update_status), and _get_index_videos' inner _get_values is get_index_video_values(video, course_id), so the service can convert one page rather than the whole course. - _get_and_validate_course is back to taking the key string, and is renamed get_and_validate_course since the service imports it. Legacy callers keep the defaults, so their behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 --- .../rest_api/v1/video_uploads_service.py | 109 +++--------------- .../contentstore/video_storage_handlers.py | 102 ++++++++-------- 2 files changed, 70 insertions(+), 141 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py b/cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py index 5bb7911723a5..7514c3fbac9f 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py +++ b/cms/djangoapps/contentstore/rest_api/v1/video_uploads_service.py @@ -1,7 +1,6 @@ """Service layer for the course video uploads resource.""" import logging -from datetime import datetime, timedelta from django.core.exceptions import ObjectDoesNotExist from django.core.exceptions import PermissionDenied as DjangoPermissionDenied @@ -10,15 +9,12 @@ from edxval.api import ( SortDirection, VideoSortField, - get_available_transcript_languages, get_course_videos_qset, get_video_info, - get_video_transcript_url, get_videos_for_course, remove_video_for_course, ) from edxval.exceptions import ValVideoNotFoundError -from pytz import UTC from rest_framework.exceptions import ( APIException, NotFound, @@ -34,9 +30,9 @@ from cms.djangoapps.contentstore.toggles import use_mock_video_uploads from cms.djangoapps.contentstore.video_storage_handlers import ( - MAX_UPLOAD_HOURS, - StatusDisplayStrings, - _get_and_validate_course, + convert_videos, + get_and_validate_course, + get_index_video_values, videos_post, ) @@ -54,15 +50,6 @@ ) DEFAULT_ORDERING = "-created" -#: VAL statuses that mean transcription has started, which only happens once -#: every encoding is complete on the current video workflow. -_TRANSCRIPTION_STATUSES = frozenset({ - "transcription_in_progress", - "transcript_ready", - "partial_failure", - "transcript_failed", -}) - class VideoUploadsNotConfigured(NotFound): """The course cannot serve video uploads because the upload pipeline is off or unconfigured.""" @@ -102,7 +89,7 @@ def get_course_for_uploads(course_key, user): no real course is needed. """ try: - course = _get_and_validate_course(course_key, user) + course = get_and_validate_course(str(course_key), user) except DjangoPermissionDenied as error: log.info("Course [%s] is not readable in Studio by user [%s]", course_key, user.id) raise PermissionDenied() from error @@ -162,88 +149,22 @@ def enrich_course_videos(course, videos): Return the API representation of ``videos`` for ``course``. Each row gains its display status, its course-specific thumbnail, its - downloadable encoding and its transcript languages and URLs. + downloadable encoding and its transcript languages and URLs. An upload + stuck past the upload window is reported as failed without the stored + record being changed. """ course = _require_course(course) course_id = str(course.id) - upload_token = course.video_upload_pipeline.get("course_video_upload_token") - return [_enrich_video(video, course_id, upload_token) for video in videos] - - -def _enrich_video(video, course_id, upload_token): - """Return the API representation of one video row.""" - encodes_ready = not upload_token and video["status"] in _TRANSCRIPTION_STATUSES - status = _display_status(video, encodes_ready) - transcripts = get_available_transcript_languages(video_id=video["edx_video_id"]) - download_link, file_size = _desktop_encoding(video) - return { - "edx_video_id": video["edx_video_id"], - "client_video_id": video["client_video_id"], - "created": video["created"], - "duration": video["duration"], - "status": status, - "error_description": video["error_description"], - "course_video_image_url": _course_video_image_url(video, course_id), - "download_link": download_link, - "file_size": file_size, - "transcripts": transcripts, - "transcription_status": _transcription_status(video, encodes_ready), - "transcript_urls": { - language_code: get_video_transcript_url( - video_id=video["edx_video_id"], - language_code=language_code, - ) - for language_code in transcripts - }, - } - - -def _transcription_status(video, encodes_ready): - """ - Return how far transcription has got for one video. - - Only courses on the current video workflow report it: elsewhere the stored - status never reaches a transcription state. It names the transcription - state itself, which the display status no longer shows once the encodes are - complete. - """ - if not encodes_ready: - return "" - return StatusDisplayStrings.get(video["status"]) + return [ + _untranslated(get_index_video_values(video, course_id)) + for video in convert_videos(course, videos, update_status=False) + ] -def _display_status(video, encodes_ready): - """ - Return the display status of one video. - - A video left in ``upload`` for longer than the upload window is reported as - failed. Reporting it does not reconcile the stored record. - """ - created = video.get("created") - now = datetime.now(created.tzinfo if created else UTC) - if video["status"] == "upload" and created and (now - created) > timedelta(hours=MAX_UPLOAD_HOURS): - return StatusDisplayStrings.get("upload_failed") - if video["status"] == "invalid_token": - return StatusDisplayStrings.get("youtube_duplicate") - if encodes_ready: - return StatusDisplayStrings.get("file_complete") - return StatusDisplayStrings.get(video["status"]) - - -def _course_video_image_url(video, course_id): - """Return the thumbnail URL recorded for this video in this course, or None.""" - for course in video["courses"]: - if course_id in course: - return course[course_id] - return None - - -def _desktop_encoding(video): - """Return the desktop MP4 download URL and file size, or empty values when absent.""" - for encoding in video["encoded_videos"]: - if encoding["profile"] == "desktop_mp4": - return encoding["url"], encoding["file_size"] - return "", 0 +def _untranslated(values): + """Return a video row whose ``status`` is the display status before translation.""" + values["status"] = values.pop("status_nontranslated") + return values #: The message every refused upload request is answered with. The video store's diff --git a/cms/djangoapps/contentstore/video_storage_handlers.py b/cms/djangoapps/contentstore/video_storage_handlers.py index 9b7195dcff36..4a70640da3f2 100644 --- a/cms/djangoapps/contentstore/video_storage_handlers.py +++ b/cms/djangoapps/contentstore/video_storage_handlers.py @@ -198,7 +198,7 @@ def handle_videos(request, course_key_string, edx_video_id=None): DELETE soft deletes a video for particular course """ - course = _get_and_validate_course(course_key_string, request.user) + course = get_and_validate_course(course_key_string, request.user) if (not course and not use_mock_video_uploads()): return HttpResponseNotFound() @@ -383,7 +383,7 @@ def handle_generate_video_upload_link(request, course_key_string): API for creating a video upload. Returns an edx_video_id and a presigned URL that can be used to upload the video to AWS S3. """ - course = _get_and_validate_course(course_key_string, request.user) + course = get_and_validate_course(course_key_string, request.user) if not course: return Response(data='Course Not Found', status=rest_status.HTTP_400_BAD_REQUEST) @@ -562,7 +562,7 @@ def get_video_encodings_download(request, course_key_string): Video ID,Name,Status,Profile1 URL,Profile2 URL aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa,video.mp4,Complete,http://example.com/prof1.mp4,http://example.com/prof2.mp4 """ - course = _get_and_validate_course(course_key_string, request.user) + course = get_and_validate_course(course_key_string, request.user) if not course: return HttpResponseNotFound() @@ -631,16 +631,12 @@ def make_csv_dict(video): return FileResponse(buffer, as_attachment=True, filename=filename, content_type="text/csv") -def _get_and_validate_course(course_key, user): +def get_and_validate_course(course_key_string, user): """ Given a course key, return the course if it exists, the given user has access to it, and it is properly configured for video uploads - - Accepts the raw string the legacy routes carry or the parsed key the - conforming routes' path converter hands their views. """ - if not isinstance(course_key, CourseKey): - course_key = CourseKey.from_string(course_key) + course_key = CourseKey.from_string(course_key_string) # For now, assume all studio users that have access to the course can upload videos. # In the future, we plan to add a new org-level role for video uploaders. @@ -657,29 +653,32 @@ def _get_and_validate_course(course_key, user): return None -def convert_video_status(video, is_video_encodes_ready=False): +def convert_video_status(video, is_video_encodes_ready=False, update_status=True): """ Convert status of a video. Status can be converted to one of the following: * FAILED if video is in `upload` state for more than 24 hours * `YouTube Duplicate` if status is `invalid_token` * user-friendly video status + + A video stuck in `upload` is also marked failed in VAL, unless `update_status` is False. """ now = datetime.now(video.get('created', datetime.now().replace(tzinfo=UTC)).tzinfo) if video['status'] == 'upload' and (now - video['created']) > timedelta(hours=MAX_UPLOAD_HOURS): new_status = 'upload_failed' status = StatusDisplayStrings.get(new_status) - message = 'Video with id [{}] is still in upload after [{}] hours, setting status to [{}]'.format( - video['edx_video_id'], MAX_UPLOAD_HOURS, new_status - ) - send_video_status_update([ - { - 'edxVideoId': video['edx_video_id'], - 'status': new_status, - 'message': message - } - ]) + if update_status: + message = 'Video with id [{}] is still in upload after [{}] hours, setting status to [{}]'.format( + video['edx_video_id'], MAX_UPLOAD_HOURS, new_status + ) + send_video_status_update([ + { + 'edxVideoId': video['edx_video_id'], + 'status': new_status, + 'message': message + } + ]) elif video['status'] == 'invalid_token': status = StatusDisplayStrings.get('youtube_duplicate') elif is_video_encodes_ready: @@ -700,6 +699,15 @@ def _get_videos(course, pagination_conf=None): SortDirection.desc, pagination_conf ) + return convert_videos(course, videos), pagination_context + + +def convert_videos(course, videos, update_status=True): + """ + Add Studio's display status, transcription status and transcript details to VAL videos of `course`. + + `update_status` is passed to `convert_video_status`. + """ videos = list(videos) # This is required to see if edx video pipeline is enabled while converting the video status. @@ -726,11 +734,11 @@ def _get_videos(course, pagination_conf=None): ) # Convert the video status. # Legacy frontend expects the status to be translated unlike MFEs which handle translation themselves. - video['status_nontranslated'] = convert_video_status(video, is_video_encodes_ready) + video['status_nontranslated'] = convert_video_status(video, is_video_encodes_ready, update_status) # pylint: disable=translation-of-non-string video['status'] = _(video['status_nontranslated']) - return videos, pagination_context + return videos def _get_default_video_image_url(): @@ -745,37 +753,37 @@ def _get_index_videos(course, pagination_conf=None): Returns the information about each video upload required for the video list """ course_id = str(course.id) + videos, pagination_context = _get_videos(course, pagination_conf) + return [get_index_video_values(video, course_id) for video in videos], pagination_context + + +def get_index_video_values(video, course_id): + """ + Get the video list attributes of one video converted by `convert_videos`. + """ attrs = [ 'edx_video_id', 'client_video_id', 'created', 'duration', 'status_nontranslated', 'status', 'courses', 'encoded_videos', 'transcripts', 'transcription_status', 'transcript_urls', 'error_description' ] - - def _get_values(video, course): - """ - Get data for predefined video attributes. - """ - values = {} - for attr in attrs: - if attr == 'courses': - current_course = [c for c in video['courses'] if course_id in c] - if current_course: - values['course_video_image_url'] = current_course[0][course_id] - else: - values['course_video_image_url'] = None - elif attr == 'encoded_videos': - values['download_link'] = '' - values['file_size'] = 0 - for encoding in video['encoded_videos']: - if encoding['profile'] == 'desktop_mp4': - values['download_link'] = encoding['url'] - values['file_size'] = encoding['file_size'] + values = {} + for attr in attrs: + if attr == 'courses': + current_course = [c for c in video['courses'] if course_id in c] + if current_course: + values['course_video_image_url'] = current_course[0][course_id] else: - values[attr] = video[attr] - return values - - videos, pagination_context = _get_videos(course, pagination_conf) - return [_get_values(video, course) for video in videos], pagination_context + values['course_video_image_url'] = None + elif attr == 'encoded_videos': + values['download_link'] = '' + values['file_size'] = 0 + for encoding in video['encoded_videos']: + if encoding['profile'] == 'desktop_mp4': + values['download_link'] = encoding['url'] + values['file_size'] = encoding['file_size'] + else: + values[attr] = video[attr] + return values def get_all_transcript_languages(): From e2c27d96b1bc60d38ad901bfd5ef39f0b94f723f Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Wed, 21 Oct 2026 12:07:35 +0500 Subject: [PATCH 13/15] feat: add a command that marks stuck video uploads as failed The legacy video list GET was the only thing that moved a video stuck in upload for more than MAX_UPLOAD_HOURS to upload_failed. The v1 GET deliberately does not write, so once the legacy GETs go those rows would stay at upload. fail_stuck_video_uploads does the same check for every video, through convert_video_status, and has a --dry-run. The v1 list schema now also requires next and previous, which DefaultPagination always returns. Co-Authored-By: Claude Opus 5.5 --- .../commands/fail_stuck_video_uploads.py | 52 ++++++++++++++++++ .../tests/test_fail_stuck_video_uploads.py | 53 +++++++++++++++++++ .../rest_api/v1/views/video_uploads.py | 10 ++-- 3 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 cms/djangoapps/contentstore/management/commands/fail_stuck_video_uploads.py create mode 100644 cms/djangoapps/contentstore/management/commands/tests/test_fail_stuck_video_uploads.py diff --git a/cms/djangoapps/contentstore/management/commands/fail_stuck_video_uploads.py b/cms/djangoapps/contentstore/management/commands/fail_stuck_video_uploads.py new file mode 100644 index 000000000000..7b55c69b1552 --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/fail_stuck_video_uploads.py @@ -0,0 +1,52 @@ +""" +Mark video uploads that never finished as failed. +""" +import logging +from datetime import datetime, timedelta + +from django.core.management.base import BaseCommand +from edxval.models import Video +from pytz import UTC + +from cms.djangoapps.contentstore.video_storage_handlers import MAX_UPLOAD_HOURS, convert_video_status + +log = logging.getLogger(__name__) + + +class Command(BaseCommand): + """ + Mark every video still in the `upload` status after MAX_UPLOAD_HOURS as `upload_failed`. + + Studio's video list does the same for the videos of the course it shows; this command does it + for all of them, without anyone having to open that list. + + Example: + ./manage.py cms fail_stuck_video_uploads --dry-run + ./manage.py cms fail_stuck_video_uploads + """ + help = f'Mark videos still uploading after {MAX_UPLOAD_HOURS} hours as failed.' + + def add_arguments(self, parser): + parser.add_argument( + '--dry-run', + action='store_true', + help='Report the stuck uploads without changing them.', + ) + + def handle(self, *args, **options): + cutoff = datetime.now(UTC) - timedelta(hours=MAX_UPLOAD_HOURS) + stuck = list( + Video.objects.filter(status='upload', created__lt=cutoff) + .values('edx_video_id', 'status', 'created') + ) + for video in stuck: + if options['dry_run']: + log.info('Video [%s] is still in upload since [%s]', video['edx_video_id'], video['created']) + else: + convert_video_status(video) + log.info( + '%s %d video upload(s) stuck for more than %d hours', + 'Found' if options['dry_run'] else 'Marked as failed', + len(stuck), + MAX_UPLOAD_HOURS, + ) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_fail_stuck_video_uploads.py b/cms/djangoapps/contentstore/management/commands/tests/test_fail_stuck_video_uploads.py new file mode 100644 index 000000000000..23739e6972aa --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/tests/test_fail_stuck_video_uploads.py @@ -0,0 +1,53 @@ +""" +Tests for the `fail_stuck_video_uploads` management command. +""" +from datetime import datetime, timedelta + +import pytz +from django.core.management import call_command +from django.test import TestCase +from edxval.api import create_video +from edxval.models import Video + + +class FailStuckVideoUploadsTest(TestCase): + """ + Tests for the `fail_stuck_video_uploads` management command. + """ + + def create_video(self, edx_video_id, status, age): + """Create a video in `status` whose upload started `age` ago.""" + create_video({ + 'edx_video_id': edx_video_id, + 'client_video_id': f'{edx_video_id}.mp4', + 'duration': 42.0, + 'status': status, + 'courses': ['course-v1:edX+DemoX+Demo_Course'], + 'encoded_videos': [], + }) + Video.objects.filter(edx_video_id=edx_video_id).update(created=datetime.now(pytz.utc) - age) + + def statuses(self): + return dict(Video.objects.values_list('edx_video_id', 'status')) + + def setUp(self): + super().setUp() + self.create_video('stuck', 'upload', timedelta(hours=25)) + self.create_video('uploading', 'upload', timedelta(hours=23)) + self.create_video('old-and-ready', 'file_complete', timedelta(hours=48)) + + def test_marks_only_uploads_older_than_the_upload_window_as_failed(self): + call_command('fail_stuck_video_uploads') + + assert self.statuses() == { + 'stuck': 'upload_failed', + 'uploading': 'upload', + 'old-and-ready': 'file_complete', + } + + def test_dry_run_changes_nothing(self): + before = self.statuses() + + call_command('fail_stuck_video_uploads', '--dry-run') + + assert self.statuses() == before diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/video_uploads.py b/cms/djangoapps/contentstore/rest_api/v1/views/video_uploads.py index 25e308b7e16a..2e406cc88a07 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/video_uploads.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/video_uploads.py @@ -50,6 +50,10 @@ class CourseVideoPagination(DefaultPagination): ``DefaultPagination`` answers with seven members, but the response schema it inherits names only four of them, so a generated client would not know that ``num_pages``, ``current_page`` and ``start`` are there. + + Delete this subclass and use ``DefaultPagination`` directly once + edx-drf-extensions documents all seven keys in + ``DefaultPagination.get_paginated_response_schema``. """ def get_paginated_response_schema(self, schema): @@ -57,7 +61,7 @@ def get_paginated_response_schema(self, schema): inherited = super().get_paginated_response_schema(schema)["properties"] return { "type": "object", - "required": ["count", "num_pages", "current_page", "start", "results"], + "required": ["count", "num_pages", "current_page", "start", "next", "previous", "results"], "properties": { "count": inherited["count"], "num_pages": { @@ -259,8 +263,8 @@ class CourseVideoUploadsViewSet( plain ViewSet with no queryset of its own. Listing a video reports an upload that has been stuck past the upload window - as failed without changing the stored record; a stuck upload is reconciled - by the upload pipeline, not by reading the list. + as failed without changing the stored record; the + ``fail_stuck_video_uploads`` management command marks it failed. """ permission_classes = (IsAuthenticated, HasCourseAuthorAccess) From be52ded3d3e773c6cee628bec8e71ac33a69b6f2 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Wed, 21 Oct 2026 12:07:35 +0500 Subject: [PATCH 14/15] fix: answer unroutable video course keys like the sibling resources A malformed or slash-separated course key on the video addresses got a JSON 404 from UnknownRouteView, while xblocks, youtube transcripts, textbooks and video settings in the same URLconf answer the same input with Studio's 404 page. The fallback view and its two routes are gone. The whole-CMS schema tests are dropped as well: cms/lib/tests covers the hooks and published addresses. The checks specific to this API (which v0 operations are deprecated, the list operationIds, and that both content_type enums keep their names) now run on the URLconfs involved. Co-Authored-By: Claude Opus 5.5 --- .../rest_api/v1/authoring_urls.py | 17 -- .../v1/views/tests/test_video_uploads.py | 272 +++--------------- .../rest_api/v1/views/unknown_route.py | 25 -- 3 files changed, 39 insertions(+), 275 deletions(-) delete mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py diff --git a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py index f7b51a701749..3630ff4e2918 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -7,7 +7,6 @@ YoutubeTranscriptChecksViewSet, YoutubeTranscriptImportsViewSet, ) -from cms.djangoapps.contentstore.rest_api.v1.views.unknown_route import UnknownRouteView from cms.djangoapps.contentstore.rest_api.v1.views.video_uploads import CourseVideoUploadsViewSet app_name = "authoring_v1" @@ -51,20 +50,4 @@ ), name="xblock_detail", ), - # Reached only by a video address whose course key the routes above refuse, - # malformed or in the deprecated slash-separated form. They answer with the - # API error body rather than the site's HTML error page. They match video - # addresses and nothing else, so a resource added under courses/ is never - # shadowed, and only slash-terminated ones, so the redirect to the - # slash-terminated form still happens first. - path( - "courses//videos/", - UnknownRouteView.as_view(), - name="course_video_list_unmatched", - ), - path( - "courses//videos//", - UnknownRouteView.as_view(), - name="course_video_detail_unmatched", - ), ] diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py index 70d33ecfc4bc..8ef93a0756c1 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_video_uploads.py @@ -2,11 +2,7 @@ Unit tests for the course video uploads API. """ import json -import os -import subprocess -import sys from datetime import datetime, timedelta -from pathlib import Path from unittest.mock import Mock, patch import ddt @@ -14,10 +10,9 @@ import pytz from django.core.exceptions import PermissionDenied as DjangoPermissionDenied from django.http import Http404 -from django.test import TestCase, override_settings +from django.test import override_settings from django.urls import resolve, reverse from drf_spectacular.generators import SchemaGenerator -from drf_spectacular.settings import patched_settings from edx_django_utils.cache import TieredCache from edx_rest_framework_extensions.auth.jwt.tests.utils import generate_jwt from edx_rest_framework_extensions.testing import assert_error_envelope @@ -27,16 +22,14 @@ from rest_framework import status from rest_framework.test import APIClient -import cms.envs from cms.djangoapps.contentstore.rest_api.v0.views.authoring_videos import ( VideosCreateUploadView, VideosUploadsView, ) from cms.djangoapps.contentstore.rest_api.v1.serializers.video_uploads import CourseVideoSerializer -from cms.djangoapps.contentstore.rest_api.v1.views.unknown_route import UnknownRouteView from cms.djangoapps.contentstore.rest_api.v1.views.video_uploads import CourseVideoUploadsViewSet from cms.djangoapps.contentstore.tests.utils import CourseTestCase -from cms.lib.spectacular import cms_api_filter, cms_mark_migrated_paths +from cms.lib.spectacular import cms_mark_migrated_paths from common.djangoapps.student.roles import ( CourseInstructorRole, CourseLimitedStaffRole, @@ -55,18 +48,6 @@ # converter therefore accepts it, but no course stands behind it. VERSION_ONLY_COURSE_KEY = "course-v1:version@5f8d0d55b54eba0b97d3dba5" -ENUM_POSTPROCESSING_HOOK = "drf_spectacular.hooks.postprocess_schema_enums" -SUPERSEDED_PATHS_HOOK = "cms.lib.spectacular.cms_mark_superseded_paths" -MIGRATED_PATHS_HOOK = "cms.lib.spectacular.cms_mark_migrated_paths" - -# The post-processing hooks the CMS registers. Registering any hook replaces -# drf-spectacular's own list, so the enum hook has to be named again alongside. -REGISTERED_POSTPROCESSING_HOOKS = [ - ENUM_POSTPROCESSING_HOOK, - SUPERSEDED_PATHS_HOOK, - MIGRATED_PATHS_HOOK, -] - # Where the authoring URLconf under test is mounted. The pre-processing hook # admits paths by this prefix, so a document generated from the URLconf alone # has to be generated under it. @@ -76,19 +57,6 @@ VIDEO_LIST_PATH = "/api/authoring/v1/courses/{course_key}/videos/" VIDEO_DETAIL_PATH = "/api/authoring/v1/courses/{course_key}/videos/{edx_video_id}/" -PRODUCTION_SETTINGS = "cms.envs.production" -SETTINGS_DIRECTORY = Path(cms.envs.__file__).resolve().parent -REPO_ROOT = SETTINGS_DIRECTORY.parents[1] -MOCK_CONFIG = SETTINGS_DIRECTORY / "mock.yml" - -# The schema settings a document generated in this process has to be given for -# its addresses to come out the way the deployment publishes them. -GENERATION_SETTING_KEYS = ( - "PREPROCESSING_HOOKS", - "POSTPROCESSING_HOOKS", - "SCHEMA_PATH_PREFIX", -) -SETTINGS_MARKER = "schema settings: " def authoring_v1_patterns(): @@ -100,33 +68,6 @@ def authoring_v1_patterns(): return [path(AUTHORING_V1_PREFIX, include(authoring_urls))] -def schema_settings(module): - """ - Return ``SPECTACULAR_SETTINGS`` of the deployment settings module ``module``. - - Deployment settings share their mutable defaults with the settings the test - process runs under and would alter them on import, so they are read in a - separate process. - """ - script = ( - "import importlib, json, sys\n" - "values = importlib.import_module(sys.argv[1]).SPECTACULAR_SETTINGS\n" - "print(sys.argv[2] + json.dumps(values))\n" - ) - completed = subprocess.run( - [sys.executable, "-c", script, module, SETTINGS_MARKER], - capture_output=True, - check=True, - cwd=REPO_ROOT, - env={**os.environ, "CMS_CFG": str(MOCK_CONFIG), "SERVICE_VARIANT": "cms"}, - text=True, - ) - reported = next( - line for line in completed.stdout.splitlines() if line.startswith(SETTINGS_MARKER) - ) - return json.loads(reported[len(SETTINGS_MARKER):]) - - VIDEO_UPLOAD_PIPELINE = { "BUCKET": "test_bucket", "ROOT_PATH": "test_root", @@ -926,7 +867,7 @@ def test_a_course_the_caller_cannot_read_in_studio_is_refused(self, operation): # Studio's own read check refuses with its framework-level error, which # carries no status of its own; the operation must still answer 403. with patch( - "cms.djangoapps.contentstore.rest_api.v1.video_uploads_service._get_and_validate_course", + "cms.djangoapps.contentstore.rest_api.v1.video_uploads_service.get_and_validate_course", side_effect=DjangoPermissionDenied(), ): response = self.call(self.client_for(self.user), operation) @@ -935,7 +876,7 @@ def test_a_course_the_caller_cannot_read_in_studio_is_refused(self, operation): @ddt.data("list", "retrieve", "create", "destroy") def test_a_course_that_cannot_be_loaded_is_reported_as_missing(self, operation): with patch( - "cms.djangoapps.contentstore.rest_api.v1.video_uploads_service._get_and_validate_course", + "cms.djangoapps.contentstore.rest_api.v1.video_uploads_service.get_and_validate_course", side_effect=Http404(), ): response = self.call(self.client_for(self.user), operation) @@ -1086,8 +1027,7 @@ def test_conforming_addresses_resolve_to_the_viewset(self): assert resolve(self.list_url).func.cls is CourseVideoUploadsViewSet assert resolve(self.detail_url("abc-123")).func.cls is CourseVideoUploadsViewSet - def test_a_usable_course_key_reaches_the_viewset_and_not_the_standby_routes(self): - """A key the converter accepts must be served by the endpoints, whatever stands behind them.""" + def test_a_usable_course_key_reaches_the_viewset_parsed(self): list_match, detail_match = resolve(self.list_url), resolve(self.detail_url("abc-123")) assert (list_match.url_name, detail_match.url_name) == ("course_video_list", "course_video_detail") @@ -1104,17 +1044,19 @@ def test_legacy_addresses_are_unchanged(self): assert resolve(self.legacy_list_url()).func.cls is VideosCreateUploadView assert resolve(self.legacy_detail_url("abc-123")).func.cls is VideosUploadsView - def assert_json_not_found(self, url): - """Assert that ``url`` answers with the API error body rather than an error page.""" - response = self.api_client.get(url) - assert response["Content-Type"].startswith("application/json") - return assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + def assert_not_routed(self, url): + """Assert that ``url`` matches no route and is answered 404, as on the sibling resources.""" + from django.urls import Resolver404 + + with pytest.raises(Resolver404): + resolve(url) + assert self.api_client.get(url).status_code == status.HTTP_404_NOT_FOUND def test_deprecated_course_key_is_not_routed(self): - self.assert_json_not_found("/api/authoring/v1/courses/Org/Course/Run/videos/") + self.assert_not_routed("/api/authoring/v1/courses/Org/Course/Run/videos/") def test_deprecated_course_key_is_not_routed_on_the_member_address(self): - self.assert_json_not_found("/api/authoring/v1/courses/Org/Course/Run/videos/abc-123/") + self.assert_not_routed("/api/authoring/v1/courses/Org/Course/Run/videos/abc-123/") def test_the_legacy_addresses_still_accept_a_deprecated_course_key(self): legacy_match = resolve("/api/contentstore/v0/videos/uploads/Org/Course/Run/abc-123") @@ -1127,7 +1069,7 @@ def test_the_legacy_list_address_still_accepts_a_deprecated_course_key(self): assert legacy_match.kwargs["course_id"] == "Org/Course/Run" def test_malformed_course_key_is_not_routed(self): - self.assert_json_not_found("/api/authoring/v1/courses/a+b+c/videos/") + self.assert_not_routed("/api/authoring/v1/courses/a+b+c/videos/") def test_a_version_only_course_key_is_not_found(self): """ @@ -1149,58 +1091,8 @@ def test_a_version_only_course_key_is_not_found(self): for response in requests: assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") - def test_a_sibling_resource_under_a_course_is_left_for_its_own_route(self): - """ - The fallbacks claim video addresses only. - - Other resources share this URLconf under courses/, so a fallback that - claimed every course address would shadow them. - """ - from django.urls import Resolver404 - - sibling = resolve( - "/api/authoring/v1/courses/course-v1:edX+DemoX+Demo_Course/youtube_transcript_checks/" - ) - assert sibling.url_name == "youtube_transcript_check_list" - with pytest.raises(Resolver404): - resolve("/api/authoring/v1/courses/course-v1:edX+DemoX+Demo_Course/no_such_resource/") - - def test_an_unknown_member_of_a_course_is_not_routed(self): - self.assert_json_not_found("/api/authoring/v1/courses/a+b+c/videos/abc-123/") - - def test_an_address_outside_a_course_is_left_to_the_site(self): - """ - Only addresses under a course carry the API error body. - - An address that names no course at all is beyond what this API claims, and - is answered by the site the same way as any other address it does not serve. - """ - from django.urls import Resolver404 - - with pytest.raises(Resolver404): - resolve("/api/authoring/v1/no/such/thing/") - assert self.api_client.get("/api/authoring/v1/no/such/thing/").status_code == 404 - - def test_unmatched_addresses_are_claimed_by_routes_that_can_be_reversed(self): - """Each address the endpoints turn down is claimed by a named route of its own.""" - unusable_key = "a+b+c" - addresses = { - "authoring_v1:course_video_list_unmatched": - f"/api/authoring/v1/courses/{unusable_key}/videos/", - "authoring_v1:course_video_detail_unmatched": - f"/api/authoring/v1/courses/{unusable_key}/videos/abc-123/", - } - kwargs = { - "authoring_v1:course_video_detail_unmatched": {"edx_video_id": "abc-123"}, - } - for name, address in addresses.items(): - reversed_address = reverse(name, kwargs={"course_key": unusable_key, **kwargs.get(name, {})}) - assert reversed_address == address, name - assert resolve(address).func.cls is UnknownRouteView, name - - def test_an_unclaimed_address_needs_no_authentication(self): - response = self.client_for(None).get("/api/authoring/v1/courses/a+b+c/videos/") - assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + def test_malformed_course_key_is_not_routed_on_the_member_address(self): + self.assert_not_routed("/api/authoring/v1/courses/a+b+c/videos/abc-123/") def test_addresses_require_the_trailing_slash(self): from django.urls import Resolver404 @@ -1524,7 +1416,6 @@ def representations(self, schema, declared): } def test_only_the_two_video_addresses_are_documented(self): - """The routes that stand behind the endpoints serve no operation and must stay unpublished.""" published = set(self.generated_schema()["paths"]) assert {path for path in published if "/videos/" in path} == { VIDEO_LIST_PATH, @@ -1537,7 +1428,7 @@ def test_the_list_response_is_declared_as_the_page_envelope(self): assert set(envelope["properties"]) == { "count", "num_pages", "current_page", "start", "next", "previous", "results", } - assert set(envelope["required"]) == {"count", "num_pages", "current_page", "start", "results"} + assert set(envelope["required"]) == set(envelope["properties"]) assert envelope["properties"]["results"]["type"] == "array" def test_the_declared_list_envelope_is_the_one_returned(self): @@ -1635,26 +1526,30 @@ def test_the_create_response_is_declared_as_the_upload_slots(self): slots = self.response_schema(self.generated_schema(), "/videos/", method="post", code="201") assert set(slots["properties"]) == {"files"} + def test_the_published_operation_ids_carry_the_version_not_the_service_prefix(self): + operations = self.generated_schema()["paths"][VIDEO_LIST_PATH] + assert { + method: operation["operationId"] + for method, operation in operations.items() + if isinstance(operation, dict) + } == {"get": "v1_courses_videos_list", "post": "v1_courses_videos_create"} -class CmsSchemaHookTest(CourseVideoUploadsTestBase): - """The schema hooks admit the new paths and deprecate the legacy ones.""" + def test_both_content_type_enums_keep_their_names(self): + """ + Adding a second ``content_type`` field must not rename the existing enum. - def endpoint(self, path): - return (path, path, "get", None) + Both are pinned in ENUM_NAME_OVERRIDES; without that, drf-spectacular + renames both after their serializers once two fields share the name. + """ + from django.urls import include, path - def test_filter_admits_the_authoring_prefix(self): - admitted = cms_api_filter([ - self.endpoint("/api/authoring/v1/courses/course-v1:a+b+c/videos/"), - self.endpoint("/api/contentstore/v0/videos/uploads/course-v1:a+b+c"), - self.endpoint("/login"), - ]) - assert [entry[0] for entry in admitted] == [ - "/api/authoring/v1/courses/course-v1:a+b+c/videos/", - "/api/contentstore/v0/videos/uploads/course-v1:a+b+c", + patterns = authoring_v1_patterns() + [ + path("api/contentstore/", include("cms.djangoapps.contentstore.rest_api.urls")), ] + schema = SchemaGenerator(patterns=patterns).get_schema(request=None, public=True) + assert {"ContentTypeEnum", "VideoUploadContentTypeEnum"} <= set(schema["components"]["schemas"]) - def superseded_schema(self): - """Run the post-processing hook over the upload paths and their unmigrated siblings.""" + def test_only_the_replaced_upload_operations_are_marked_deprecated(self): paths = { "/api/contentstore/v0/videos/uploads/{course_id}": {"post": {}}, "/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}": { @@ -1665,12 +1560,9 @@ def superseded_schema(self): "/api/contentstore/v0/videos/images/{course_id}/{edx_video_id}": {"post": {}}, "/api/contentstore/v0/videos/encodings/{course_id}": {"get": {}}, "/api/contentstore/v0/videos/features": {"get": {}}, - "/api/authoring/v1/courses/{course_key}/videos/": {"get": {}, "post": {}}, + VIDEO_LIST_PATH: {"get": {}, "post": {}}, } - return cms_mark_migrated_paths({"paths": paths}, None, None, False) - - def test_post_processing_marks_only_the_replaced_upload_operations(self): - result = self.superseded_schema() + result = cms_mark_migrated_paths({"paths": paths}, None, None, False) deprecated = { (path, method) @@ -1683,89 +1575,3 @@ def test_post_processing_marks_only_the_replaced_upload_operations(self): ("/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}", "get"), ("/api/contentstore/v0/videos/uploads/{course_id}/{edx_video_id}", "delete"), } - - def authoring_schema(self, hooks): - """Generate the authoring document with ``hooks`` as the post-processing list.""" - with patched_settings({"POSTPROCESSING_HOOKS": hooks}): - return SchemaGenerator(patterns=authoring_v1_patterns()).get_schema( - request=None, public=True - ) - - def test_enum_components_survive_the_registered_post_processing(self): - deprecation_only = self.authoring_schema([MIGRATED_PATHS_HOOK]) - registered = self.authoring_schema(REGISTERED_POSTPROCESSING_HOOKS) - - assert not [ - name for name in deprecation_only["components"]["schemas"] if name.endswith("Enum") - ] - # The name is pinned in ENUM_NAME_OVERRIDES, so it does not move when - # another serializer grows a content_type field. - assert "VideoUploadContentTypeEnum" in registered["components"]["schemas"] - - -class CmsSchemaAddressTest(TestCase): - """The published document addresses every operation the way it is mounted.""" - - # One operation of every Studio API version the document covers, with the - # address a client reaches by joining the published path to a server, plus - # both new addresses. - PUBLISHED_PATHS = { - "/api/contentstore/v0/videos/uploads/{course_id}": - "/api/contentstore/v0/videos/uploads/course-v1:a+b+c", - "/api/contentstore/v1/videos/{course_id}": - "/api/contentstore/v1/videos/course-v1:a+b+c", - "/api/contentstore/v2/home/courses": - "/api/contentstore/v2/home/courses", - "/api/contentstore/v3/course_details/{course_id}/": - "/api/contentstore/v3/course_details/course-v1:a+b+c/", - "/api/contentstore/v4/home/courses/": - "/api/contentstore/v4/home/courses/", - VIDEO_LIST_PATH: - "/api/authoring/v1/courses/course-v1:a+b+c/videos/", - VIDEO_DETAIL_PATH: - "/api/authoring/v1/courses/course-v1:a+b+c/videos/video-1/", - } - - def cms_schema(self, module=PRODUCTION_SETTINGS): - """Generate the Studio document with the schema settings of ``module``.""" - read = schema_settings(module) - with patched_settings({key: read[key] for key in GENERATION_SETTING_KEYS}): - return SchemaGenerator().get_schema(request=None, public=True) - - def test_the_document_publishes_the_mounted_addresses(self): - assert set(self.PUBLISHED_PATHS) <= set(self.cms_schema()["paths"]) - - def test_the_document_publishes_no_other_video_address(self): - """The routes standing behind the endpoints serve no operation and stay unpublished.""" - published = self.cms_schema()["paths"] - authoring_videos = { - path for path in published - if path.startswith("/api/authoring/") and "/videos/" in path - } - assert authoring_videos == { - path for path in self.PUBLISHED_PATHS - if path.startswith("/api/authoring/") and "/videos/" in path - } - - def test_no_published_path_is_shortened(self): - published = sorted(self.cms_schema()["paths"]) - assert published, "the pre-processing hook admitted no path" - assert [path for path in published if not path.startswith("/api/")] == [] - - def test_the_published_operation_ids_carry_the_version_not_the_service_prefix(self): - operations = self.cms_schema()["paths"]["/api/authoring/v1/courses/{course_key}/videos/"] - assert { - method: operation["operationId"] - for method, operation in operations.items() - if isinstance(operation, dict) - } == {"get": "v1_courses_videos_list", "post": "v1_courses_videos_create"} - - def test_every_published_path_is_an_address_that_resolves(self): - for published, address in self.PUBLISHED_PATHS.items(): - assert resolve(address).func, published - - def test_the_registered_hooks_are_the_ones_generated_with(self): - """The document under test is generated with the hooks the deployment registers.""" - assert schema_settings(PRODUCTION_SETTINGS)["POSTPROCESSING_HOOKS"] == ( - REGISTERED_POSTPROCESSING_HOOKS - ) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py b/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py deleted file mode 100644 index e551794a8507..000000000000 --- a/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py +++ /dev/null @@ -1,25 +0,0 @@ -"""API view answering addresses that no endpoint serves.""" - -from drf_spectacular.utils import extend_schema -from edx_rest_framework_extensions.mixins import StandardizedErrorMixin -from rest_framework.exceptions import NotFound -from rest_framework.permissions import AllowAny -from rest_framework.views import APIView - - -@extend_schema(exclude=True) -class UnknownRouteView(StandardizedErrorMixin, APIView): - """ - Answer a video address whose course key the conforming routes refuse. - - A key that is malformed or in the deprecated slash-separated form fails to - resolve, and would otherwise be answered by the site's HTML error page, which - an API client cannot read. Nothing here needs the caller's identity, so the - answer is the same for everyone: the address holds no resource. - """ - - permission_classes = (AllowAny,) - - def initial(self, request, *args, **kwargs): - """Refuse the request before any handler runs; the address holds nothing.""" - raise NotFound() From 1b54e407266823ffafc979e6d96b4db5462add4a Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Wed, 21 Oct 2026 12:07:35 +0500 Subject: [PATCH 15/15] test: pin the version-only course key 404 on the other v1 viewsets HasCourseAuthorAccess is shared with XblockViewSet and the youtube transcript viewsets, so a version-only course key now answers 404 rather than 500 there too. Those tests fail with 500 against master's permission class. The docstring no longer says the key is injected by XblockViewSet.initial(). Co-Authored-By: Claude Opus 5.5 --- .../contentstore/rest_api/v1/views/permissions.py | 4 ++-- .../rest_api/v1/views/tests/test_xblock_viewset.py | 9 +++++++++ .../v1/views/tests/test_youtube_transcripts.py | 10 ++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py b/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py index a1b52f495234..c88708edb30b 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py @@ -15,8 +15,8 @@ class HasCourseAuthorAccess(BasePermission): """ ADR 0026: replaces the @course_author_access_required decorator. - Reads ``view.kwargs["course_key"]`` (a CourseKey instance) that is - injected by XblockViewSet.initial() before DRF runs permission checks. + Reads ``view.course_key`` (a CourseKey instance), which the view sets in + ``initial()`` before DRF runs permission checks. Returns 403 if the authenticated user lacks authoring rights on that course, or if no course key could be derived, and 404 for a key that names only a version, which has no course run to grant access to. 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 52d6fe9c0e7c..be66e9a04a46 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 @@ -134,6 +134,15 @@ def test_non_author_403_type_uri(self): response = self.client.get(_detail_url()) assert response.json()["type"] == "https://docs.openedx.org/errors/authz" + def test_version_only_course_key_returns_404(self): + self.client.force_authenticate(user=self.non_author) + response = self.client.get(reverse( + "authoring_v1:xblock_detail", + kwargs={"usage_key_string": "block-v1:version@5f8d0d55b54eba0b97d3dba5+type@problem+block@abc"}, + )) + assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.json()["type"] == "https://docs.openedx.org/errors/not-found" + def test_error_body_has_no_developer_message(self): response = self.client.get(_detail_url()) data = response.json() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_youtube_transcripts.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_youtube_transcripts.py index bef2c4944729..33794d6491ea 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_youtube_transcripts.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_youtube_transcripts.py @@ -47,6 +47,8 @@ IMPORT_URL_NAME = "authoring_v1:youtube_transcript_import_list" LEGACY_CHECK_URL_NAME = "cms.djangoapps.contentstore:v0:cms_api_youtube_transcripts_check" LEGACY_IMPORT_URL_NAME = "cms.djangoapps.contentstore:v0:cms_api_youtube_transcripts_upload" +# A key that names a version and no course run. +VERSION_ONLY_COURSE_KEY = "course-v1:version@5f8d0d55b54eba0b97d3dba5" # A V1 content library block: neither a course nor a V2 library context. UNSUPPORTED_CONTEXT_LOCATOR = "lib-block-v1:edX+Lib+2024+type@video+block@abc" @@ -163,6 +165,10 @@ def test_global_staff_is_allowed(self): assert response.status_code == status.HTTP_200_OK assert response.json()["status"] == "Success" + def test_a_version_only_course_key_is_not_found(self): + response = self.get_check(user=self.course_instructor, course_key=VERSION_ONLY_COURSE_KEY) + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + @ddt.ddt @patch(DOWNLOAD_PATH, Mock(return_value=[["en", SJSON_TRANSCRIPT_CONTENT]])) @@ -186,6 +192,10 @@ def test_course_instructor_is_allowed(self): assert response.status_code == status.HTTP_200_OK assert response.json()["status"] == "Success" + def test_a_version_only_course_key_is_not_found(self): + response = self.post_import(user=self.course_instructor, course_key=VERSION_ONLY_COURSE_KEY) + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + def test_global_staff_is_allowed(self): response = self.post_import(user=self.global_staff) assert response.status_code == status.HTTP_200_OK