From 15a98864c490ef2d73038d005575bf9324a1f169 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Sun, 25 Oct 2026 15:09:22 +0500 Subject: [PATCH 1/9] feat: add course existence and advanced settings permission classes The v0 advanced settings view checks course existence with the @verify_course_exists decorator and access with an inline check_course_advanced_settings_access call in each handler. The conforming views need these as permission classes. IsExistingCourse answers 404 for a course key that names no course run: an unknown course, a key that names only a version, or a library key. CanViewAdvancedSettings and CanManageAdvancedSettings make the same check_course_advanced_settings_access(read|write) call the v0 view makes, so the policy-permission and legacy Studio-access branches are unchanged. Existing classes in the module are untouched. --- .../contentstore/views/permissions.py | 67 ++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/permissions.py b/cms/djangoapps/contentstore/views/permissions.py index 6450a3bfcc3e..3701d7149481 100644 --- a/cms/djangoapps/contentstore/views/permissions.py +++ b/cms/djangoapps/contentstore/views/permissions.py @@ -3,13 +3,21 @@ """ from openedx_authz.constants.permissions import COURSES_VIEW_PAGES_AND_RESOURCES +from rest_framework.exceptions import NotFound from rest_framework.permissions import BasePermission -from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access +from common.djangoapps.student.auth import ( + check_course_advanced_settings_access, + has_studio_read_access, + has_studio_write_access, +) from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission from openedx.core.djangoapps.authz.decorators import user_has_course_permission +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.lib.api.view_utils import validate_course_key +COURSE_NOT_FOUND_MESSAGE = "The requested course does not exist." + class HasStudioWriteAccess(BasePermission): """ @@ -58,3 +66,60 @@ def has_permission(self, request, view): if course_key is None: return False return has_studio_read_access(request.user, course_key) + + +class IsExistingCourse(BasePermission): + """ + Answer 404 when the course in the path does not exist. + + Reads the parsed ``course_key`` URL argument. A key that names only a + content version parses, but no course run is behind it, so it is answered + as an unknown course. List this class after the authentication check, so + an anonymous caller learns nothing about which courses exist, and before + any class that checks roles on the course, so a signed-in caller is told + that a course is missing before being refused. + """ + + def has_permission(self, request, view): + course_key = view.kwargs.get("course_key") + if course_key is None: + return False + if course_key.org is None or not CourseOverview.course_exists(course_key): + raise NotFound(COURSE_NOT_FOUND_MESSAGE) + return True + + +class CanViewAdvancedSettings(BasePermission): + """ + Allow callers who may view the advanced settings of the course in the path. + + Reads the parsed ``course_key`` URL argument. On courses where + policy-based authoring permissions are enabled, only that policy is + consulted; elsewhere the caller needs Studio read access to the course. + Advanced settings being restricted to staff site-wide does not apply here: + course teams also change page and resource toggles through these settings. + """ + + def has_permission(self, request, view): + course_key = view.kwargs.get("course_key") + if course_key is None: + return False + return check_course_advanced_settings_access(request.user, course_key, access_type="read") + + +class CanManageAdvancedSettings(BasePermission): + """ + Allow callers who may change the advanced settings of the course in the path. + + Reads the parsed ``course_key`` URL argument. On courses where + policy-based authoring permissions are enabled, only that policy is + consulted; elsewhere the caller needs Studio write access to the course. + Advanced settings being restricted to staff site-wide does not apply here, + as for ``CanViewAdvancedSettings``. + """ + + def has_permission(self, request, view): + course_key = view.kwargs.get("course_key") + if course_key is None: + return False + return check_course_advanced_settings_access(request.user, course_key, access_type="write") From 4cf3dec0381c18df70645a073ec112c4f66fa2a7 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Sun, 25 Oct 2026 15:09:22 +0500 Subject: [PATCH 2/9] feat: serve course advanced settings from authoring v1 GET/PATCH /api/contentstore/v0/advanced_settings/{course_id} answers errors with developer_message bodies and some failures with a 500, accepts Bearer tokens, returns every settings field by default, and reports a refused value with the field parser's own text, which can be Python exception text. GET/PATCH /api/authoring/v1/courses/{course_key}/advanced_settings/ is its successor. Errors use the standard envelope; a refused value is reported under its setting name with one fixed message and the original text is logged. The default read is the set of settings course authors edit (v0 with fetch_all=0); view=full returns every field and fields narrows either view. Authentication keeps the session class that admits inactive users, as the legacy endpoint did; Bearer is dropped. Access is IsAuthenticated, IsExistingCourse and a per-action class making v0's read or write check, and an action without a declared permission is never served. Parity tests compare v1 with v0 through the test client; the declared differences are the error envelope and the fixed message. --- .../rest_api/v1/advanced_settings_service.py | 90 + .../rest_api/v1/authoring_urls.py | 6 + .../v1/serializers/advanced_settings.py | 144 ++ .../rest_api/v1/views/advanced_settings.py | 203 +++ .../v1/views/tests/test_advanced_settings.py | 1577 +++++++++++++++++ 5 files changed, 2020 insertions(+) create mode 100644 cms/djangoapps/contentstore/rest_api/v1/advanced_settings_service.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/serializers/advanced_settings.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/advanced_settings.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py diff --git a/cms/djangoapps/contentstore/rest_api/v1/advanced_settings_service.py b/cms/djangoapps/contentstore/rest_api/v1/advanced_settings_service.py new file mode 100644 index 000000000000..3a641e488000 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/advanced_settings_service.py @@ -0,0 +1,90 @@ +"""Reads and updates of a course's advanced settings.""" + +import logging + +from rest_framework.exceptions import NotFound, ValidationError +from rest_framework.settings import api_settings + +from cms.djangoapps.contentstore.views.course import update_course_advanced_settings +from cms.djangoapps.contentstore.views.permissions import COURSE_NOT_FOUND_MESSAGE +from cms.djangoapps.models.settings.course_metadata import CourseMetadata +from xmodule.modulestore.django import modulestore + +log = logging.getLogger(__name__) + +# Reported under the name of each setting whose value the course refused. The +# validator's own explanation is logged, not returned. +INVALID_SETTING_MESSAGE = "The value is not valid for this setting." +# Reported when the settings could not be saved for a reason tied to no setting. +SETTINGS_NOT_SAVED_MESSAGE = "The settings could not be saved. No setting was changed." + + +def _course_block(course_key): + """Return the course block of ``course_key``, or raise ``NotFound`` when the modulestore has none.""" + course_block = modulestore().get_course(course_key) + if course_block is None: + raise NotFound(COURSE_NOT_FOUND_MESSAGE) + return course_block + + +def settings_authors_do_not_edit(course_key): + """Return the names of the course's settings that are not edited as advanced settings.""" + return frozenset(CourseMetadata.get_exclude_list_of_fields(course_key)) + + +def get_advanced_settings(course_key, full=False): + """ + Return the advanced settings of the course, keyed by setting name. + + By default only the settings course authors edit in Studio are returned; + with ``full`` every setting of the course is. + """ + course_block = _course_block(course_key) + if full: + return CourseMetadata.fetch_all(course_block) + return CourseMetadata.fetch(course_block) + + +def update_advanced_settings(course_key, changes, user): + """ + Apply ``changes`` (``{name: {"value": value}}``) to the course as ``user``. + + Every value is validated before any is stored; when one is refused, + nothing is stored and ``ValidationError`` maps each refused setting to + ``INVALID_SETTING_MESSAGE``. + Returns the settings course authors edit in Studio, as stored. + """ + course_block = _course_block(course_key) + try: + return update_course_advanced_settings(course_block, changes, user) + except ValidationError as error: + raise _errors_by_setting(course_key, error) from error + + +def _errors_by_setting(course_key, error): + """ + Return a ``ValidationError`` mapping each refused setting to a fixed message. + + The settings validator reports one entry per refused setting, carrying the + setting's name and its own explanation, which can be the text of a parsing + exception. The explanation is logged with the course and setting; the + caller gets ``INVALID_SETTING_MESSAGE`` under the setting's name. Any + failure not tied to a setting is logged and reported with + ``SETTINGS_NOT_SAVED_MESSAGE``. + """ + detail = error.detail if isinstance(error.detail, list) else [error.detail] + by_setting = {} + unattributed = [] + for entry in detail: + name = entry.get("key") if isinstance(entry, dict) else None + if name: + log.warning( + "Advanced setting %s of course %s was refused: %s", name, course_key, entry.get("message", ""), + ) + by_setting[str(name)] = [INVALID_SETTING_MESSAGE] + else: + unattributed.append(entry) + if unattributed: + log.warning("Advanced settings of course %s were not saved: %s", course_key, unattributed) + by_setting.setdefault(api_settings.NON_FIELD_ERRORS_KEY, []).append(SETTINGS_NOT_SAVED_MESSAGE) + return ValidationError(by_setting) diff --git a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py index 5a53ae650413..afea706611ba 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -7,6 +7,7 @@ YoutubeTranscriptChecksViewSet, YoutubeTranscriptImportsViewSet, ) +from cms.djangoapps.contentstore.rest_api.v1.views.advanced_settings import CourseAdvancedSettingsViewSet app_name = "authoring_v1" @@ -39,4 +40,9 @@ ), name="xblock_detail", ), + path( + "courses//advanced_settings/", + CourseAdvancedSettingsViewSet.as_view({"get": "retrieve", "patch": "partial_update"}), + name="course_advanced_settings", + ), ] diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/advanced_settings.py new file mode 100644 index 000000000000..7090efd864b1 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/advanced_settings.py @@ -0,0 +1,144 @@ +"""Serializers for a course's advanced settings.""" + +from collections.abc import Mapping + +from drf_spectacular.extensions import OpenApiSerializerExtension +from drf_spectacular.plumbing import build_object_type, get_doc +from rest_framework import serializers +from rest_framework.exceptions import ValidationError +from rest_framework.settings import api_settings + +MINIMAL_VIEW = "minimal" +FULL_VIEW = "full" + + +class AdvancedSettingSerializer(serializers.Serializer): # pylint: disable=abstract-method + """One advanced setting of a course, as Studio presents it for editing.""" + + value = serializers.JSONField( + allow_null=True, + help_text="Current value of the setting. Its type depends on the setting.", + ) + display_name = serializers.CharField(help_text="Name of the setting as Studio shows it.") + help = serializers.CharField( + allow_null=True, + help_text="Text explaining what the setting does, or null when the setting has none.", + ) + deprecated = serializers.BooleanField(help_text="Whether the setting is deprecated.") + hide_on_enabled_publisher = serializers.BooleanField( + help_text="Whether Studio hides the setting when the course is managed by a publishing service.", + ) + + class Meta: + ref_name = "AdvancedSetting.v1" + + +class AdvancedSettingUpdateSerializer(serializers.Serializer): # pylint: disable=abstract-method + """The new value of one advanced setting.""" + + value = serializers.JSONField( + allow_null=True, + help_text="New value of the setting. Its type depends on the setting.", + ) + + class Meta: + ref_name = "AdvancedSettingUpdate.v1" + + +class _SettingsByNameSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + An object whose members are settings keyed by setting name. + + The set of names depends on the course, so the members cannot be declared + as fields; each one is handled by ``setting_serializer_class``. Names given + in the ``ignored_settings`` context entry are dropped before any member is + validated. + """ + + setting_serializer_class = None + + def to_representation(self, instance): + return { + name: self.setting_serializer_class(setting).data + for name, setting in instance.items() + } + + def to_internal_value(self, data): + if not isinstance(data, Mapping): + message = self.error_messages["invalid"].format(datatype=type(data).__name__) + raise ValidationError({api_settings.NON_FIELD_ERRORS_KEY: [message]}, code="invalid") + ignored = self.context.get("ignored_settings", frozenset()) + validated, errors = {}, {} + for name, item in data.items(): + if name in ignored: + continue + setting = self.setting_serializer_class(data=item) + if setting.is_valid(): + validated[name] = dict(setting.validated_data) + else: + errors[name] = [str(message) for messages in setting.errors.values() for message in messages] + if errors: + raise ValidationError(errors) + return validated + + +class CourseAdvancedSettingsSerializer(_SettingsByNameSerializer): # pylint: disable=abstract-method + """A course's advanced settings, keyed by setting name in the order the course declares them.""" + + setting_serializer_class = AdvancedSettingSerializer + + class Meta: + ref_name = "CourseAdvancedSettings.v1" + + +class CourseAdvancedSettingsUpdateSerializer(_SettingsByNameSerializer): # pylint: disable=abstract-method + """ + New values for some of a course's advanced settings, keyed by setting name. + + Settings that authors do not edit are ignored, whatever their entry holds; + settings the course does not have are ignored once their entry is valid. + """ + + setting_serializer_class = AdvancedSettingUpdateSerializer + + class Meta: + ref_name = "CourseAdvancedSettingsUpdate.v1" + + +class AdvancedSettingsQuerySerializer(serializers.Serializer): # pylint: disable=abstract-method + """Query parameters of a read of a course's advanced settings.""" + + view = serializers.ChoiceField( + choices=[MINIMAL_VIEW, FULL_VIEW], + default=MINIMAL_VIEW, + help_text=( + "Which settings to return. 'minimal' returns the settings course authors edit in Studio; " + "'full' returns every setting of the course, including those Studio manages on other pages " + "and those of features that are turned off." + ), + ) + fields = serializers.CharField( + required=False, + allow_blank=True, + help_text=( + "Comma-separated names of the settings to return, out of those the view returns. " + "Names the view does not return are ignored." + ), + ) + + class Meta: + ref_name = "AdvancedSettingsQuery.v1" + + +class SettingsByNameSerializerExtension(OpenApiSerializerExtension): + """Publish a settings-by-name serializer as an object whose members are its setting component.""" + + target_class = _SettingsByNameSerializer + match_subclasses = True + + def map_serializer(self, auto_schema, direction): + setting = auto_schema.resolve_serializer(self.target.setting_serializer_class, direction) + return build_object_type( + description=get_doc(self.target.__class__), + additionalProperties=setting.ref, + ) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v1/views/advanced_settings.py new file mode 100644 index 000000000000..3fba8c7740bd --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/advanced_settings.py @@ -0,0 +1,203 @@ +"""API views for a course's advanced settings.""" + +from django.core.exceptions import ImproperlyConfigured +from drf_spectacular.utils import OpenApiExample, OpenApiParameter, OpenApiResponse, extend_schema +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from edx_rest_framework_extensions.errors import ErrorResponseSerializer +from edx_rest_framework_extensions.mixins import StandardizedErrorMixin +from edx_rest_framework_extensions.shaping import project +from edx_rest_framework_extensions.url_converters import CourseKeyConverter +from rest_framework import viewsets +from rest_framework.permissions import IsAuthenticated +from rest_framework.response import Response + +from cms.djangoapps.contentstore.rest_api.v1.advanced_settings_service import ( + INVALID_SETTING_MESSAGE, + get_advanced_settings, + settings_authors_do_not_edit, + update_advanced_settings, +) +from cms.djangoapps.contentstore.rest_api.v1.serializers.advanced_settings import ( + FULL_VIEW, + MINIMAL_VIEW, + AdvancedSettingsQuerySerializer, + CourseAdvancedSettingsSerializer, + CourseAdvancedSettingsUpdateSerializer, +) +from cms.djangoapps.contentstore.views.permissions import ( + CanManageAdvancedSettings, + CanViewAdvancedSettings, + IsExistingCourse, +) + +_COURSE_KEY_PARAMETER = OpenApiParameter( + name="course_key", + description="Key of the course, such as course-v1:edX+DemoX+2026.", + location=OpenApiParameter.PATH, + type=str, + pattern=f"^{CourseKeyConverter.regex}$", +) + + +def _query_parameter(name, **kwargs): + return OpenApiParameter( + name=name, + description=AdvancedSettingsQuerySerializer().fields[name].help_text, + required=False, + location=OpenApiParameter.QUERY, + **kwargs, + ) + + +def _error(description): + return OpenApiResponse(response=ErrorResponseSerializer, description=description) + + +_SETTINGS_EXAMPLE = OpenApiExample( + "Advanced settings", + summary="Two of a course's settings", + response_only=True, + status_codes=["200"], + value={ + "display_name": { + "value": "Demonstration Course", + "display_name": "Course Display Name", + "help": "Enter the name of the course as it should appear in the course list.", + "deprecated": False, + "hide_on_enabled_publisher": False, + }, + "days_early_for_beta": { + "value": None, + "display_name": "Days Early for Beta Users", + "help": "Enter the number of days before the start date that beta users can access the course.", + "deprecated": False, + "hide_on_enabled_publisher": False, + }, + }, +) + +_UPDATE_EXAMPLE = OpenApiExample( + "New values", + summary="Turn on the calculator and set the advanced modules", + request_only=True, + value={ + "show_calculator": {"value": True}, + "advanced_modules": {"value": ["poll", "survey"]}, + }, +) + + +@extend_schema(tags=["openedx-platform-sdk"]) +class CourseAdvancedSettingsViewSet(StandardizedErrorMixin, viewsets.ViewSet): + """ + The advanced settings of one course: its policy settings keyed by name. + + By default (``view=minimal``) the settings course authors edit in Studio + are returned; with ``view=full`` every setting of the course is. A partial + update changes the values of the settings it names, all of them or none. + + Anyone who may view the course's advanced settings may read them; changing + them needs the permission to manage them. The course is looked up first, + so a signed-in caller learns that a course does not exist (404) before + being refused (403). + + A plain ``ViewSet``: the settings are fields of the course block in the + modulestore, not database rows. + """ + + # Kept from the legacy endpoint: an inactive account's browser session stays authenticated and + # the course permission decides (inactive course staff are refused, inactive global staff are not). + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated, IsExistingCourse) + action_permission_classes = { + "retrieve": (CanViewAdvancedSettings,), + "partial_update": (CanManageAdvancedSettings,), + } + # A method with no route (answered 405) and OPTIONS serve no settings, so + # only the course is checked for them. + unrouted_actions = (None, "metadata") + serializer_class = CourseAdvancedSettingsSerializer + + def get_permissions(self): + """ + Check the course, then the permission the requested action needs. + + An action with no permission declared in ``action_permission_classes`` + is refused with a server error instead of being served unchecked. + """ + if self.action in self.unrouted_actions: + return super().get_permissions() + if self.action not in self.action_permission_classes: + raise ImproperlyConfigured(f"No permission is declared for the {self.action!r} action.") + specific = self.action_permission_classes[self.action] + return [*super().get_permissions(), *(permission() for permission in specific)] + + def get_serializer(self, *args, **kwargs): + """Instantiate the settings serializer declared on this viewset.""" + return self.serializer_class(*args, **kwargs) + + @extend_schema( + summary="Get a course's advanced settings", + description=( + "Returns the course's advanced settings keyed by setting name, each with its value, its name " + "as Studio shows it, its help text and whether it is deprecated. By default (view=minimal) the " + "settings course authors edit in Studio are returned; view=full returns every setting of the " + "course. fields narrows the response to the named settings." + ), + parameters=[ + _COURSE_KEY_PARAMETER, + _query_parameter("view", type=str, enum=[MINIMAL_VIEW, FULL_VIEW], default=MINIMAL_VIEW), + _query_parameter("fields", type=str), + ], + responses={ + 200: OpenApiResponse( + response=CourseAdvancedSettingsSerializer, + description="The course's advanced settings.", + ), + 400: _error("A query parameter has an invalid value."), + 401: _error("The requester is not authenticated."), + 403: _error("The requester may not view this course's advanced settings."), + 404: _error("The course does not exist."), + }, + examples=[_SETTINGS_EXAMPLE], + ) + def retrieve(self, request, course_key): + """Return the course's advanced settings, narrowed to ``fields`` when given.""" + query = AdvancedSettingsQuerySerializer(data=request.query_params) + query.is_valid(raise_exception=True) + params = query.validated_data + current = get_advanced_settings(course_key, full=params["view"] == FULL_VIEW) + return Response(project(self.get_serializer(current).data, params.get("fields"))) + + @extend_schema( + summary="Change some of a course's advanced settings", + description=( + "Sets the value of each setting named in the body. Every value is validated before any is " + "stored: when one is refused nothing is changed, and the error's errors member maps the name " + f"of each refused setting to the message '{INVALID_SETTING_MESSAGE}'. Settings that authors do " + "not edit are ignored, and so are settings the course does not have. Returns the settings " + "course authors edit in Studio, as stored." + ), + parameters=[_COURSE_KEY_PARAMETER], + request={"application/json": CourseAdvancedSettingsUpdateSerializer}, + responses={ + 200: OpenApiResponse( + response=CourseAdvancedSettingsSerializer, + description="The settings course authors edit in Studio, after the change.", + ), + 400: _error("The body is not an object of settings, or a value was refused."), + 401: _error("The requester is not authenticated."), + 403: _error("The requester may not change this course's advanced settings."), + 404: _error("The course does not exist."), + }, + examples=[_SETTINGS_EXAMPLE, _UPDATE_EXAMPLE], + ) + def partial_update(self, request, course_key): + """Store the new values and return the editable settings as stored.""" + changes = CourseAdvancedSettingsUpdateSerializer( + data=request.data, context={"ignored_settings": settings_authors_do_not_edit(course_key)}, + ) + changes.is_valid(raise_exception=True) + stored = update_advanced_settings(course_key, changes.validated_data, request.user) + return Response(self.get_serializer(stored).data) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py new file mode 100644 index 000000000000..38973d752674 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py @@ -0,0 +1,1577 @@ +"""Tests for the v1 course advanced settings API.""" + +import json +import uuid +from contextlib import contextmanager +from datetime import UTC, datetime, timedelta +from functools import cache +from types import SimpleNamespace +from unittest.mock import patch + +import ddt +import jsonschema +import pymongo +import pytest +from crum import signals as crum_signals +from django.core import signals as core_signals +from django.core.exceptions import ImproperlyConfigured +from django.db import connection, transaction +from django.db.models import signals as model_signals +from django.dispatch import Signal +from django.test import SimpleTestCase, TestCase +from django.test.utils import CaptureQueriesContext, override_settings +from django.urls import Resolver404, resolve, reverse +from edx_rest_framework_extensions.testing import assert_error_envelope +from opaque_keys.edx.keys import CourseKey +from openedx_authz.constants.roles import ( + COURSE_ADMIN, + COURSE_AUDITOR, + COURSE_BETA_TESTER, + COURSE_DATA_RESEARCHER, + COURSE_EDITOR, + COURSE_LIMITED_STAFF, + COURSE_STAFF, +) +from openedx_authz.models.engine import PolicyCacheControl +from openedx_events.tooling import OpenEdxPublicSignal +from rest_framework.permissions import IsAuthenticated +from rest_framework.test import APIClient, APIRequestFactory, force_authenticate +from rest_framework.utils.encoders import JSONEncoder + +from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls +from cms.djangoapps.contentstore.rest_api.v1.advanced_settings_service import ( + INVALID_SETTING_MESSAGE, + SETTINGS_NOT_SAVED_MESSAGE, +) +from cms.djangoapps.contentstore.rest_api.v1.views.advanced_settings import CourseAdvancedSettingsViewSet +from cms.djangoapps.contentstore.rest_api.v2.views.tests.helpers import generate_cms_schema +from cms.djangoapps.contentstore.tests.utils import routed_actions +from cms.djangoapps.contentstore.views.permissions import ( + CanManageAdvancedSettings, + CanViewAdvancedSettings, + IsExistingCourse, +) +from cms.djangoapps.models.settings.course_metadata import CourseMetadata +from cms.lib.spectacular import LEGACY_MIGRATED_PATH_PREFIXES, SUPERSEDED_PATHS +from common.djangoapps.student.auth import check_course_advanced_settings_access, has_studio_advanced_settings_access +from common.djangoapps.student.models import CourseAccessRole +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseLimitedStaffRole, + CourseStaffRole, + OrgInstructorRole, + OrgStaffRole, +) +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin +from openedx.core.djangoapps.oauth_dispatch.jwt import create_jwt_for_user +from openedx.core.djangoapps.oauth_dispatch.tests.factories import AccessTokenFactory, ApplicationFactory +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.mixed import MixedModuleStore +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, LibraryFactory +from xmodule.tabs import InvalidTabsException + +URL_NAME = "authoring_v1:course_advanced_settings" +LEGACY_URL_NAME = "cms.djangoapps.contentstore:v0:course_advanced_settings" +LEGACY_VIEW = ("cms.djangoapps.contentstore.rest_api.v0.views.advanced_settings", "AdvancedCourseSettingsView") +SERVICE_LOGGER = "cms.djangoapps.contentstore.rest_api.v1.advanced_settings_service" +UNKNOWN_COURSE_KEY = "course-v1:NoSuchOrg+NoSuchCourse+NoSuchRun" +VERSION_ONLY_KEY = "course-v1:version@0123456789abcdef01234567" +COURSE_NOT_FOUND = "The requested course does not exist." +FULL = {"view": "full"} +LEGACY_DEFAULT = {"fetch_all": "0"} +# An integer setting authors edit, so a non-numeric value is refused by its field. +INTEGER_SETTING = "max_student_enrollments_allowed" +SETTING_MEMBERS = {"value", "display_name", "help", "deprecated", "hide_on_enabled_publisher"} +ACTIONS = ("read", "update") + + +def url(course_key): + """Return the address of the advanced settings of ``course_key``.""" + return reverse(URL_NAME, kwargs={"course_key": str(course_key)}) + + +def legacy_url(course_key): + """Return the deprecated advanced settings address of ``course_key``.""" + return reverse(LEGACY_URL_NAME, kwargs={"course_id": str(course_key)}) + + +def _client(user=None): + """An API client that returns server errors as responses, authenticated as ``user`` if given.""" + client = APIClient() + client.raise_request_exception = False + if user is not None: + client.force_authenticate(user=user) + return client + + +def _session_client(user): + """A client signed in to a browser session as ``user``.""" + user.set_password("password") + user.save() + client = APIClient() + assert client.login(username=user.username, password="password") + return client + + +def stored_course(course_key): + """The stored course block of ``course_key`` (a key or its string form), or None when there is none.""" + return modulestore().get_course(CourseKey.from_string(str(course_key))) + + +def stored_display_name(course_key): + """The display name stored for the course.""" + return stored_course(course_key).display_name + + +def editable_settings(course_key): + """The settings authors edit, as the settings model returns them for the stored course.""" + return CourseMetadata.fetch(stored_course(course_key)) + + +def every_setting(course_key): + """Every setting of the stored course, as the settings model returns them.""" + return CourseMetadata.fetch_all(stored_course(course_key)) + + +def _assert_refused(response, status): + """Assert ``response`` is the error envelope for ``status``; a 404 names the missing course.""" + slug = {401: "authn", 403: "authz", 404: "not-found"}[status] + envelope = assert_error_envelope(response, expected_status=status, expected_type_slug=slug) + if status == 404: + assert envelope["detail"] == COURSE_NOT_FOUND + return envelope + + +# --------------------------------------------------------------------------- +# The published schema +# --------------------------------------------------------------------------- + +@cache +def _published_schema(): + """The CMS authoring schema, generated once per test process.""" + return generate_cms_schema() + + +def _as_json_schema(node): + """An OpenAPI 3.0 schema as JSON Schema: ``nullable`` becomes a null alternative.""" + if isinstance(node, list): + return [_as_json_schema(item) for item in node] + if not isinstance(node, dict): + return node + converted = {key: _as_json_schema(value) for key, value in node.items() if key != "nullable"} + if node.get("nullable"): + return {"anyOf": [converted, {"type": "null"}]} + return converted + + +def assert_matches_published_schema(body, component): + """Assert ``body`` is valid against the published schema component ``component``.""" + components = _as_json_schema(_published_schema()["components"]["schemas"]) + jsonschema.validate(body, {"$ref": f"#/components/schemas/{component}", "components": {"schemas": components}}) + + +# --------------------------------------------------------------------------- +# Who may call the endpoint +# --------------------------------------------------------------------------- + +class _AccessMixin: + """ + Checks a caller's status on a read or an update against the expected one, + and that v0 decides the same for the same caller. + """ + + def assert_read(self, user, expected, course_key=None): + """Assert the status of a read by ``user``, its body or envelope, and agreement with v0.""" + course_key = course_key or self.course.id + response = _client(user).get(url(course_key)) + legacy = _client(user).get(legacy_url(course_key), LEGACY_DEFAULT) + assert response.status_code == expected, response.content + if expected == 200: + assert list(response.json()) == list(editable_settings(course_key)) + else: + _assert_refused(response, expected) + assert legacy.status_code == expected, f"v0 decided {legacy.status_code} for the same caller" + + def assert_update(self, user, expected, course_key=None): + """Assert the status of an update by ``user``, what it stored, and agreement with v0.""" + course_key = course_key or self.course.id + course = stored_course(course_key) + before = course.display_name if course is not None else None + new_name = f"Renamed {uuid.uuid4().hex[:8]}" + response = _client(user).patch(url(course_key), {"display_name": {"value": new_name}}, format="json") + assert response.status_code == expected, response.content + if expected == 200: + assert response.json()["display_name"]["value"] == new_name + assert stored_display_name(course_key) == new_name + else: + _assert_refused(response, expected) + if course is not None: + assert stored_display_name(course_key) == before + legacy_name = f"Legacy {uuid.uuid4().hex[:8]}" + legacy = _client(user).patch(legacy_url(course_key), {"display_name": {"value": legacy_name}}, format="json") + assert legacy.status_code == expected, f"v0 decided {legacy.status_code} for the same caller" + + def assert_access(self, user, action, expected, course_key=None): + """Assert ``expected`` for ``action`` (``read`` or ``update``) by ``user``.""" + getattr(self, f"assert_{action}")(user, expected, course_key) + + +@ddt.ddt +class AdvancedSettingsAccessLegacyPermissionsTest(_AccessMixin, SharedModuleStoreTestCase): + """Who may read and change the settings when policy-based authoring permissions are off.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + cls.other_course = CourseFactory.create() + + def _user_with(self, role): + user = UserFactory() + role.add_users(user) + return user + + @ddt.data(*ACTIONS) + def test_anonymous_caller_is_refused_with_401(self, action): + self.assert_access(None, action, 401) + + @ddt.data(*ACTIONS) + def test_anonymous_caller_of_an_unknown_course_is_refused_with_401(self, action): + self.assert_access(None, action, 401, course_key=UNKNOWN_COURSE_KEY) + + @ddt.data(*ACTIONS) + def test_user_without_a_role_is_refused_with_403(self, action): + self.assert_access(UserFactory(), action, 403) + + @ddt.data(*ACTIONS) + def test_unknown_course_is_404_before_permission_is_checked(self, action): + self.assert_access(UserFactory(), action, 404, course_key=UNKNOWN_COURSE_KEY) + + @ddt.data(*ACTIONS) + def test_limited_staff_is_refused(self, action): + self.assert_access(self._user_with(CourseLimitedStaffRole(self.course.id)), action, 403) + + @ddt.data(*ACTIONS) + def test_staff_and_instructors_of_another_course_are_refused(self, action): + self.assert_access(self._user_with(CourseStaffRole(self.other_course.id)), action, 403) + self.assert_access(self._user_with(CourseInstructorRole(self.other_course.id)), action, 403) + + @ddt.data(*ACTIONS) + def test_course_staff_is_allowed(self, action): + self.assert_access(self._user_with(CourseStaffRole(self.course.id)), action, 200) + + @ddt.data(*ACTIONS) + def test_course_instructor_is_allowed(self, action): + self.assert_access(self._user_with(CourseInstructorRole(self.course.id)), action, 200) + + @ddt.data(*ACTIONS) + def test_org_staff_and_org_instructor_are_allowed(self, action): + self.assert_access(self._user_with(OrgStaffRole(self.course.id.org)), action, 200) + self.assert_access(self._user_with(OrgInstructorRole(self.course.id.org)), action, 200) + + @ddt.data(*ACTIONS) + def test_global_staff_is_allowed(self, action): + self.assert_access(UserFactory(is_staff=True), action, 200) + + @ddt.data(*ACTIONS) + def test_a_superuser_who_is_not_global_staff_is_refused(self, action): + """Without policy-based permissions, Studio grants global staff and course roles, not the superuser flag.""" + self.assert_access(UserFactory(is_superuser=True, is_staff=False), action, 403) + + def test_a_version_only_course_key_is_404(self): + """The key parses but names no course run; v0 has no address that takes it.""" + user = UserFactory(is_staff=True) + _assert_refused(_client(user).get(url(VERSION_ONLY_KEY)), 404) + _assert_refused( + _client(user).patch(url(VERSION_ONLY_KEY), {"display_name": {"value": "x"}}, format="json"), 404, + ) + + def test_a_library_is_not_a_course(self): + library_key = LibraryFactory.create().location.library_key + user = UserFactory(is_staff=True) + _assert_refused(_client(user).get(url(library_key)), 404) + _assert_refused(_client(user).patch(url(library_key), {}, format="json"), 404) + + +@ddt.ddt +class AdvancedSettingsAccessAuthzTest(CourseAuthoringAuthzTestMixin, _AccessMixin, SharedModuleStoreTestCase): + """ + Who may read and change the settings when policy-based authoring + permissions are on: viewing needs the permission to view advanced + settings, changing them the permission to manage them. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + cls.other_course = CourseFactory.create() + + def _user_with(self, role, course_key=None): + user = UserFactory() + self.add_user_to_role_in_course(user, role, course_key or self.course.id) + return user + + @ddt.data(*ACTIONS) + def test_anonymous_caller_is_refused_with_401(self, action): + self.assert_access(None, action, 401) + + @ddt.data(*ACTIONS) + def test_user_without_a_role_is_refused_with_403(self, action): + self.assert_access(self.unauthorized_user, action, 403) + + @ddt.data(*ACTIONS) + def test_unknown_course_is_404_before_permission_is_checked(self, action): + self.assert_access(self.unauthorized_user, action, 404, course_key=UNKNOWN_COURSE_KEY) + + @ddt.data(COURSE_AUDITOR.external_key, COURSE_EDITOR.external_key) + def test_roles_that_may_only_view_may_read_but_not_update(self, role): + user = self._user_with(role) + self.assert_read(user, 200) + self.assert_update(user, 403) + + @ddt.data(*[(action, role) for action in ACTIONS for role in ( + COURSE_STAFF.external_key, COURSE_ADMIN.external_key, + )]) + @ddt.unpack + def test_roles_that_may_manage_may_read_and_update(self, action, role): + self.assert_access(self._user_with(role), action, 200) + + @ddt.data(*[(action, role) for action in ACTIONS for role in ( + COURSE_LIMITED_STAFF.external_key, COURSE_DATA_RESEARCHER.external_key, COURSE_BETA_TESTER.external_key, + )]) + @ddt.unpack + def test_other_roles_are_refused(self, action, role): + self.assert_access(self._user_with(role), action, 403) + + @ddt.data(*ACTIONS) + def test_a_role_on_another_course_is_refused(self, action): + user = self._user_with(COURSE_ADMIN.external_key, self.other_course.id) + self.assert_access(user, action, 200, course_key=self.other_course.id) + self.assert_access(user, action, 403) + + @ddt.data(*ACTIONS) + def test_legacy_course_staff_without_a_policy_role_is_refused(self, action): + user = UserFactory() + CourseAccessRole.objects.create(user=user, role="staff", course_id=self.course.id, org=self.course.id.org) + self.assert_access(user, action, 403) + + @ddt.data(*ACTIONS) + def test_staff_and_superuser_are_allowed_without_a_role(self, action): + self.assert_access(self.staff_user, action, 200) + self.assert_access(self.super_user, action, 200) + + def test_an_inactive_session_with_a_role_is_accepted(self): + """The policy does not look at whether the account is activated, on v1 as on v0.""" + user = self._user_with(COURSE_AUDITOR.external_key) + client = _session_client(user) + user.is_active = False + user.save() + assert client.get(url(self.course.id)).status_code == 200 + assert client.get(legacy_url(self.course.id)).status_code == 200 + + +@override_settings(DISABLE_ADVANCED_SETTINGS=True) +class AdvancedSettingsSiteRestrictionLegacyPermissionsTest(_AccessMixin, SharedModuleStoreTestCase): + """ + With advanced settings restricted to staff site-wide and policy-based + permissions off, course staff still read and change them here, as on v0. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def setUp(self): + super().setUp() + self.course_staff = UserFactory() + CourseStaffRole(self.course.id).add_users(self.course_staff) + + def test_the_site_restriction_is_on(self): + """The Studio advanced settings page would refuse this caller.""" + assert has_studio_advanced_settings_access(self.course_staff) is False + assert check_course_advanced_settings_access(self.course_staff, self.course.id, "feature_restricted") is False + + def test_course_staff_may_still_read_and_update(self): + self.assert_read(self.course_staff, 200) + self.assert_update(self.course_staff, 200) + + def test_a_caller_without_a_role_is_still_refused(self): + self.assert_read(UserFactory(), 403) + self.assert_update(UserFactory(), 403) + + +@override_settings(DISABLE_ADVANCED_SETTINGS=True) +class AdvancedSettingsSiteRestrictionAuthzTest(CourseAuthoringAuthzTestMixin, _AccessMixin, SharedModuleStoreTestCase): + """ + With advanced settings restricted to staff site-wide and policy-based + permissions on, the policy alone still decides here, as on v0. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def _user_with(self, role): + user = UserFactory() + self.add_user_to_role_in_course(user, role, self.course.id) + return user + + def test_the_site_restriction_is_on(self): + """The restricted check that Studio pages use would refuse a course staff role.""" + user = self._user_with(COURSE_STAFF.external_key) + assert check_course_advanced_settings_access(user, self.course.id, "feature_restricted") is False + + def test_a_role_that_may_manage_may_still_read_and_update(self): + user = self._user_with(COURSE_STAFF.external_key) + self.assert_read(user, 200) + self.assert_update(user, 200) + + def test_a_role_that_may_only_view_still_may_not_update(self): + user = self._user_with(COURSE_EDITOR.external_key) + self.assert_read(user, 200) + self.assert_update(user, 403) + + +class AdvancedSettingsPermissionWiringTest(SimpleTestCase): + """Which permission each action checks, and which access each settings permission asks for.""" + + def test_each_action_checks_the_course_then_its_own_permission(self): + view = CourseAdvancedSettingsViewSet() + for action, expected in ( + ("retrieve", [IsAuthenticated, IsExistingCourse, CanViewAdvancedSettings]), + ("partial_update", [IsAuthenticated, IsExistingCourse, CanManageAdvancedSettings]), + (None, [IsAuthenticated, IsExistingCourse]), + ("metadata", [IsAuthenticated, IsExistingCourse]), + ): + view.action = action + assert [type(permission) for permission in view.get_permissions()] == expected, action + + def test_an_action_without_a_declared_permission_is_refused(self): + view = CourseAdvancedSettingsViewSet() + for action in ("destroy", "update", "list", "create"): + view.action = action + with pytest.raises(ImproperlyConfigured): + view.get_permissions() + + def test_a_routed_action_without_a_declared_permission_is_never_served(self): + """A request reaching such an action is answered with a server error, not the handler's body.""" + + class Unchecked(CourseAdvancedSettingsViewSet): + permission_classes = CourseAdvancedSettingsViewSet.permission_classes + + def destroy(self, request, course_key): # pylint: disable=unused-argument + raise AssertionError("the handler ran without a permission check") + + view = Unchecked.as_view({"delete": "destroy"}) + request = APIRequestFactory().delete(url(COURSE_KEY)) + force_authenticate(request, user=SimpleNamespace(is_authenticated=True, is_active=True)) + response = view(request, course_key=CourseKey.from_string(COURSE_KEY)) + assert_error_envelope(response, expected_status=500, expected_type_slug="internal") + + def test_the_settings_permissions_ask_for_read_and_write_access(self): + course_key = "course-v1:edX+Wiring+Run" + request = SimpleNamespace(user=object()) + view = SimpleNamespace(kwargs={"course_key": course_key}) + target = "cms.djangoapps.contentstore.views.permissions.check_course_advanced_settings_access" + for permission, access_type in ((CanViewAdvancedSettings, "read"), (CanManageAdvancedSettings, "write")): + for decision in (True, False): + with patch(target, return_value=decision) as check: + assert permission().has_permission(request, view) is decision + check.assert_called_once_with(request.user, course_key, access_type=access_type) + + def test_the_settings_permissions_refuse_a_view_without_a_course(self): + request = SimpleNamespace(user=object()) + view = SimpleNamespace(kwargs={}) + for permission in (CanViewAdvancedSettings, CanManageAdvancedSettings, IsExistingCourse): + assert permission().has_permission(request, view) is False + + +class AdvancedSettingsAuthenticationTest(SharedModuleStoreTestCase): + """How callers authenticate, with policy-based authoring permissions off.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def _course_staff(self): + user = UserFactory() + CourseStaffRole(self.course.id).add_users(user) + return user + + def test_a_jwt_caller_may_read_and_update(self): + header = {"HTTP_AUTHORIZATION": f"JWT {create_jwt_for_user(self._course_staff())}"} + assert APIClient().get(url(self.course.id), **header).status_code == 200 + response = APIClient().patch( + url(self.course.id), {"display_name": {"value": "By JWT"}}, format="json", **header, + ) + assert response.status_code == 200 + assert stored_display_name(self.course.id) == "By JWT" + + def test_a_session_caller_may_read_and_update(self): + client = _session_client(self._course_staff()) + assert client.get(url(self.course.id)).status_code == 200 + response = client.patch(url(self.course.id), {"display_name": {"value": "By session"}}, format="json") + assert response.status_code == 200 + assert stored_display_name(self.course.id) == "By session" + + def test_an_inactive_session_is_authenticated(self): + """ + A session whose account is not activated is accepted, as on v0. Global + staff then pass; course staff are refused (403, not 401) because a + course role does not count while the account is inactive. + """ + for user, expected in ((UserFactory(is_staff=True), 200), (self._course_staff(), 403)): + client = _session_client(user) + user.is_active = False + user.save() + response = client.get(url(self.course.id)) + assert response.status_code == expected, response.content + if expected == 403: + _assert_refused(response, 403) + assert client.get(legacy_url(self.course.id)).status_code == expected + + def test_a_bearer_token_is_refused(self): + """Bearer tokens are not accepted here, though v0 takes them.""" + user = UserFactory(is_staff=True) + token = AccessTokenFactory(user=user, application=ApplicationFactory(user=user)).token + header = {"HTTP_AUTHORIZATION": f"Bearer {token}"} + _assert_refused(APIClient().get(url(self.course.id), **header), 401) + assert APIClient().get(legacy_url(self.course.id), **header).status_code == 200 + + +# --------------------------------------------------------------------------- +# Reads +# --------------------------------------------------------------------------- + +@ddt.ddt +class AdvancedSettingsReadTest(SharedModuleStoreTestCase): + """What a read returns, by view and by named settings.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def setUp(self): + super().setUp() + reader = UserFactory() + CourseStaffRole(self.course.id).add_users(reader) + self.client = _client(reader) + + def get(self, **params): + response = self.client.get(url(self.course.id), params) + assert response.status_code == 200, response.content + return response.json() + + def test_the_default_view_is_the_settings_authors_edit_in_course_order(self): + body = self.get() + assert list(body) == list(editable_settings(self.course.id)) + assert "display_name" in body + assert "tabs" not in body + + def test_the_full_view_is_every_setting_in_course_order(self): + body = self.get(**FULL) + assert list(body) == list(every_setting(self.course.id)) + assert "tabs" in body + + def test_the_default_view_leaves_out_exactly_the_excluded_settings(self): + default, full = self.get(), self.get(**FULL) + excluded = set(CourseMetadata.get_exclude_list_of_fields(self.course.id)) + assert set(full) - set(default) == set(full) & excluded + assert set(full) - set(default) != set() + + @ddt.data({}, FULL) + def test_every_setting_carries_the_five_members(self, params): + body = self.get(**params) + assert {name: set(setting) for name, setting in body.items()} == {name: SETTING_MEMBERS for name in body} + + @ddt.data({}, FULL) + def test_the_body_matches_the_published_schema(self, params): + assert_matches_published_schema(self.get(**params), "CourseAdvancedSettings.v1") + + def test_the_values_are_those_the_settings_model_reads(self): + body = self.get(**FULL) + assert {name: setting["value"] for name, setting in body.items()} == { + name: json.loads(json.dumps(setting["value"], cls=JSONEncoder)) + for name, setting in every_setting(self.course.id).items() + } + + def test_fields_narrows_to_the_named_settings_in_course_order(self): + named = {"days_early_for_beta", "display_name"} + body = self.get(fields="days_early_for_beta,display_name") + assert list(body) == [name for name in editable_settings(self.course.id) if name in named] + + def test_fields_ignores_names_outside_the_view(self): + assert self.get(fields="tabs,no_such_setting") == {} + assert list(self.get(fields="tabs,no_such_setting", **FULL)) == ["tabs"] + + def test_fields_ignores_spaces_around_names(self): + assert set(self.get(fields=" display_name , days_early_for_beta ")) == {"display_name", "days_early_for_beta"} + + def test_empty_fields_returns_the_whole_view(self): + assert list(self.get(fields="")) == list(editable_settings(self.course.id)) + + def test_the_minimal_view_is_the_default(self): + assert self.get(view="minimal") == self.get() + assert list(self.get(view="minimal")) == list(editable_settings(self.course.id)) + + def test_an_unknown_view_is_refused(self): + response = self.client.get(url(self.course.id), {"view": "default"}) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"view": ['"default" is not a valid choice.']} + + def test_the_v0_query_parameters_are_ignored(self): + """fetch_all and filter_fields have no meaning here; view and fields replace them.""" + for params in ({}, FULL): + expected = self.get(**params) + assert self.get(**params, fetch_all="0", filter_fields="display_name") == expected + assert self.get(**params, fetch_all="1") == expected + + def test_head_answers_without_a_body(self): + for params in ({}, FULL): + response = self.client.head(url(self.course.id), params) + assert response.status_code == 200 + assert response.content == b"" + + + +# --------------------------------------------------------------------------- +# Updates +# --------------------------------------------------------------------------- + +class AdvancedSettingsUpdateTest(ModuleStoreTestCase): + """What an update stores, returns and refuses.""" + + def setUp(self): + super().setUp() + self.course = CourseFactory.create(display_name="Original") + writer = UserFactory() + CourseStaffRole(self.course.id).add_users(writer) + self.writer = writer + self.client = _client(writer) + + def patch(self, body): + return self.client.patch(url(self.course.id), body, format="json") + + def test_named_settings_are_stored_and_the_editable_settings_returned(self): + response = self.patch({"display_name": {"value": "Renamed"}, "days_early_for_beta": {"value": 3.0}}) + assert response.status_code == 200, response.content + course = modulestore().get_course(self.course.id) + assert (course.display_name, course.days_early_for_beta) == ("Renamed", 3.0) + body = response.json() + assert list(body) == list(editable_settings(self.course.id)) + assert body["display_name"]["value"] == "Renamed" + assert body["days_early_for_beta"]["value"] == 3.0 + assert_matches_published_schema(body, "CourseAdvancedSettings.v1") + + def test_members_other_than_value_are_ignored(self): + response = self.patch({"display_name": {"value": "Renamed", "display_name": "Not stored", "deprecated": True}}) + assert response.status_code == 200, response.content + setting = response.json()["display_name"] + assert (setting["value"], setting["deprecated"]) == ("Renamed", False) + assert setting["display_name"] == str(editable_settings(self.course.id)["display_name"]["display_name"]) + + def test_unknown_settings_and_settings_authors_do_not_edit_are_ignored(self): + start = modulestore().get_course(self.course.id).start + response = self.patch({"no_such_setting": {"value": 1}, "start": {"value": "2030-01-01T00:00:00Z"}}) + assert response.status_code == 200, response.content + assert "no_such_setting" not in response.json() + assert "start" not in response.json() + assert modulestore().get_course(self.course.id).start == start + + def test_a_null_value_is_stored(self): + self.patch({"days_early_for_beta": {"value": 3.0}}) + response = self.patch({"days_early_for_beta": {"value": None}}) + assert response.status_code == 200, response.content + assert modulestore().get_course(self.course.id).days_early_for_beta is None + + def test_a_refused_value_is_reported_with_a_fixed_message_and_nothing_is_stored(self): + body = {INTEGER_SETTING: {"value": "lots"}, "display_name": {"value": "Not stored"}} + with self.assertLogs(SERVICE_LOGGER, "WARNING") as logs: + response = self.patch(body) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {INTEGER_SETTING: [INVALID_SETTING_MESSAGE]} + assert stored_display_name(self.course.id) == "Original" + legacy = self.client.patch(legacy_url(self.course.id), body, format="json") + assert legacy.status_code == 400 + (validator_text,) = [ + entry["message"] for entry in legacy.json()["developer_message"] if entry["key"] == INTEGER_SETTING + ] + assert validator_text != INVALID_SETTING_MESSAGE + assert validator_text not in response.content.decode() + (record,) = logs.output + assert validator_text in record + assert INTEGER_SETTING in record + assert str(self.course.id) in record + + def test_several_refused_settings_are_each_named(self): + body = {INTEGER_SETTING: {"value": "lots"}, "days_early_for_beta": {"value": "soon"}} + with self.assertLogs(SERVICE_LOGGER, "WARNING") as logs: + response = self.patch(body) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == { + INTEGER_SETTING: [INVALID_SETTING_MESSAGE], "days_early_for_beta": [INVALID_SETTING_MESSAGE], + } + assert len(logs.output) == 2 + + def test_a_body_that_is_not_an_object_is_refused(self): + envelope = assert_error_envelope(self.patch([1, 2]), expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"non_field_errors": ["Invalid data. Expected a dictionary, but got list."]} + + def test_each_entry_must_be_an_object_with_a_value(self): + response = self.patch({"display_name": "Renamed", "days_early_for_beta": {}}) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == { + "display_name": ["Invalid data. Expected a dictionary, but got str."], + "days_early_for_beta": ["This field is required."], + } + assert stored_display_name(self.course.id) == "Original" + + def test_an_entry_that_is_not_an_object_is_refused_by_v0_too(self): + body = {"display_name": "Renamed"} + assert self.patch(body).status_code == 400 + assert self.client.patch(legacy_url(self.course.id), body, format="json").status_code == 400 + assert stored_display_name(self.course.id) == "Original" + + def test_a_malformed_entry_of_a_setting_authors_do_not_edit_is_ignored(self): + start = stored_course(self.course.id).start + response = self.patch({"tabs": "not tabs", "start": {}, "display_name": {"value": "Renamed"}}) + assert response.status_code == 200, response.content + assert stored_display_name(self.course.id) == "Renamed" + assert stored_course(self.course.id).start == start + + def test_a_failure_saving_the_tabs_is_reported_without_its_text(self): + target = "cms.djangoapps.contentstore.views.course._refresh_course_tabs" + with patch(target, side_effect=InvalidTabsException("tab internals")), self.assertLogs(SERVICE_LOGGER, "WARNING"): + response = self.patch({"display_name": {"value": "Not stored"}}) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"non_field_errors": [SETTINGS_NOT_SAVED_MESSAGE]} + assert b"tab internals" not in response.content + assert b"Tabs Exception" not in response.content + assert stored_display_name(self.course.id) == "Original" + with patch(target, side_effect=InvalidTabsException("tab internals")): + legacy = self.client.patch(legacy_url(self.course.id), {"display_name": {"value": "x"}}, format="json") + assert legacy.status_code == 400 + + def test_an_unexpected_validation_failure_is_reported_without_its_text(self): + target = "cms.djangoapps.models.settings.course_metadata.CourseMetadata.validate_and_update_from_json" + with patch(target, side_effect=TypeError("validator internals")), self.assertLogs(SERVICE_LOGGER, "WARNING"): + response = self.patch({"display_name": {"value": "Not stored"}}) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"non_field_errors": [SETTINGS_NOT_SAVED_MESSAGE]} + assert b"validator internals" not in response.content + assert stored_display_name(self.course.id) == "Original" + with patch(target, side_effect=TypeError("validator internals")): + legacy = self.client.patch(legacy_url(self.course.id), {"display_name": {"value": "x"}}, format="json") + assert legacy.status_code == 400 + assert b"validator internals" in legacy.content + + +# Requests v0 and v1 answer differently beyond the error body, as +# (request, v0 answer, v1 answer, test in AdvancedSettingsResponseDifferencesTest). +RESPONSE_DIFFERENCES = [ + ("read or update of a course the modulestore returns nothing for", "500", "404", + "test_a_course_the_modulestore_returns_nothing_for"), + ("update whose body is not an object", "500", "400", "test_an_update_whose_body_is_not_an_object"), + ("update entry without a value", "500", "400", "test_an_update_entry_without_a_value"), + ("read with view=default", "200", "400", "test_an_unknown_view"), + ("read or update with a Bearer token", "200", "401", "test_a_bearer_token"), + ("slash-separated course key", "404 JSON", "404 HTML", "test_a_slash_separated_course_key"), +] + + +class AdvancedSettingsResponseDifferencesTest(ModuleStoreTestCase): + """Each request v0 and v1 answer differently, both answers observed.""" + + def setUp(self): + super().setUp() + self.course = CourseFactory.create(display_name="Original") + writer = UserFactory() + CourseStaffRole(self.course.id).add_users(writer) + self.client = _client(writer) + + def both(self, method, body=None, **params): + """The v0 and v1 responses to the same request.""" + call = getattr(self.client, method) + kwargs = {"format": "json"} if body is not None else {} + data = body if body is not None else params + return call(legacy_url(self.course.id), data, **kwargs), call(url(self.course.id), data, **kwargs) + + def test_every_listed_difference_has_its_test(self): + assert [name for *_, name in RESPONSE_DIFFERENCES if not hasattr(self, name)] == [] + + def test_a_course_the_modulestore_returns_nothing_for(self): + with patch.object(MixedModuleStore, "get_course", return_value=None): + for method, body in (("get", None), ("patch", {"display_name": {"value": "x"}})): + legacy, new = self.both(method, body) + assert legacy.status_code == 500, method + _assert_refused(new, 404) + + def test_an_update_whose_body_is_not_an_object(self): + legacy, new = self.both("patch", [1, 2]) + assert legacy.status_code == 500 + envelope = assert_error_envelope(new, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"non_field_errors": ["Invalid data. Expected a dictionary, but got list."]} + + def test_an_update_entry_without_a_value(self): + legacy, new = self.both("patch", {"display_name": {}}) + assert legacy.status_code == 500 + envelope = assert_error_envelope(new, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"display_name": ["This field is required."]} + assert stored_display_name(self.course.id) == "Original" + + def test_an_unknown_view(self): + legacy, new = self.both("get", view="default") + assert legacy.status_code == 200 + assert_error_envelope(new, expected_status=400, expected_type_slug="validation") + + def test_a_bearer_token(self): + user = UserFactory(is_staff=True) + token = AccessTokenFactory(user=user, application=ApplicationFactory(user=user)).token + header = {"HTTP_AUTHORIZATION": f"Bearer {token}"} + assert APIClient().get(legacy_url(self.course.id), **header).status_code == 200 + _assert_refused(APIClient().get(url(self.course.id), **header), 401) + body = {"display_name": {"value": "By Bearer"}} + assert APIClient().patch(legacy_url(self.course.id), body, format="json", **header).status_code == 200 + _assert_refused(APIClient().patch(url(self.course.id), body, format="json", **header), 401) + + def test_a_slash_separated_course_key(self): + """v0 routes the slash form and reports the course unknown in JSON; v1 has no route for it.""" + legacy = self.client.get("/api/contentstore/v0/advanced_settings/edX/NoSuch/Course") + new = self.client.get("/api/authoring/v1/courses/edX/NoSuch/Course/advanced_settings/") + assert legacy.status_code == new.status_code == 404 + assert legacy.json()["error_code"] == "course_does_not_exist" + assert new["Content-Type"].startswith("text/html") + + +@ddt.ddt +@override_settings( + PROCTORING_BACKENDS={"DEFAULT": "test_proctoring_provider", "valid_provider": {}}, + PARTNER_SUPPORT_EMAIL="support@example.com", +) +class AdvancedSettingsProctoringProviderTest(ModuleStoreTestCase): + """ + Once a course has started, only global staff may change its proctoring + provider, on v1 as on v0. Before it starts, course staff may. + """ + + CHANGE = {"proctoring_provider": {"value": "valid_provider"}} + + def _course(self, started): + offset = timedelta(days=-1 if started else 30) + return CourseFactory.create(start=datetime.now(UTC) + offset) + + def _course_staff(self, course): + user = UserFactory() + CourseStaffRole(course.id).add_users(user) + return user + + def _patch(self, version, user, course): + address = url(course.id) if version == "v1" else legacy_url(course.id) + return _client(user).patch(address, self.CHANGE, format="json") + + @ddt.data("v1", "v0") + def test_course_staff_may_not_change_it_after_the_course_starts(self, version): + course = self._course(started=True) + before = stored_course(course.id).proctoring_provider + assert before != "valid_provider" + response = self._patch(version, self._course_staff(course), course) + assert response.status_code == 400, response.content + if version == "v1": + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert list(envelope["errors"]) == ["proctoring_provider"] + else: + assert [entry["key"] for entry in response.json()["developer_message"]] == ["proctoring_provider"] + assert stored_course(course.id).proctoring_provider == before + + @ddt.data("v1", "v0") + def test_global_staff_may_change_it_after_the_course_starts(self, version): + course = self._course(started=True) + response = self._patch(version, UserFactory(is_staff=True), course) + assert response.status_code == 200, response.content + assert response.json()["proctoring_provider"]["value"] == "valid_provider" + assert stored_course(course.id).proctoring_provider == "valid_provider" + + @ddt.data("v1", "v0") + def test_course_staff_may_change_it_before_the_course_starts(self, version): + course = self._course(started=False) + response = self._patch(version, self._course_staff(course), course) + assert response.status_code == 200, response.content + assert stored_course(course.id).proctoring_provider == "valid_provider" + + +# --------------------------------------------------------------------------- +# Parity with v0 +# --------------------------------------------------------------------------- + +# Members of a response body that differ between v0 and v1 for the same +# request. The comparison asserts nothing else differs, and that each of these +# occurs in the error cases below. +INTENTIONAL_DIFFERENCES = [ + ("type", "errors use the standardized envelope"), + ("title", "errors use the standardized envelope"), + ("status", "errors use the standardized envelope"), + ("detail", "errors use the standardized envelope"), + ("errors", "a refused update maps the name of each refused setting to INVALID_SETTING_MESSAGE, where v0 " + "lists the validator's own text under developer_message"), + ("developer_message", "the v0 error body carries developer_message; the envelope does not"), + ("error_code", "the v0 not-found body carries error_code; the envelope does not"), +] +VOLATILE_FIELDS = {"instance"} + +# Reads of the two addresses that return the same settings. Their parameters +# differ by design: the default view is v0's fetch_all=0 response, view=full +# is v0's default, and fields replaces filter_fields. +EQUIVALENT_READS = [ + ({}, LEGACY_DEFAULT), + (FULL, {}), + ( + {"fields": "display_name,days_early_for_beta"}, + {**LEGACY_DEFAULT, "filter_fields": "display_name,days_early_for_beta"}, + ), + ({**FULL, "fields": "tabs,display_name"}, {"filter_fields": "tabs,display_name"}), +] + +_ABSENT = object() + + +def _flatten(obj, prefix=""): + """ + A ``{json_path: value}`` map of every leaf of ``obj``, without volatile + members. An empty object or array is a leaf, so a container that empties + or changes kind is seen. + """ + flat = {} + if isinstance(obj, dict) and obj: + for key in sorted(obj): + if key in VOLATILE_FIELDS: + continue + flat.update(_flatten(obj[key], f"{prefix}.{key}" if prefix else key)) + elif isinstance(obj, list) and obj: + for index, item in enumerate(obj): + flat.update(_flatten(item, f"{prefix}[{index}]")) + else: + flat[prefix] = obj + return flat + + +def _same(a, b): + """Whether two leaves are equal and of the same JSON type: true is not 1, 1.0 is not 1, {} is not [].""" + return type(a) is type(b) and a == b + + +def _changed(legacy, new): + """The json paths whose leaf differs between the two bodies.""" + a, b = _flatten(legacy), _flatten(new) + return {key for key in set(a) | set(b) if not _same(a.get(key, _ABSENT), b.get(key, _ABSENT))} + + +def _undeclared(changed): + """The changed paths no declared intentional difference covers.""" + declared = [path for path, _ in INTENTIONAL_DIFFERENCES] + return { + key for key in changed + if not any(key == path or key.startswith(f"{path}.") or key.startswith(f"{path}[") for path in declared) + } + + +def test_the_comparison_sees_types_order_free_members_and_empty_containers(): + assert _changed({"a": {"b": []}}, {"a": {}}) == {"a", "a.b"} + assert _changed({"x": True}, {"x": 1}) == {"x"} + assert _changed({"x": 1.0}, {"x": 1}) == {"x"} + assert _changed({"x": {}}, {"x": []}) == {"x"} + assert _changed({"x": None, "y": 1}, {"y": 1}) == {"x"} + assert _changed({"instance": "/a"}, {"instance": "/b"}) == set() + assert _changed({"x": [1, {"y": False}]}, {"x": [1, {"y": False}]}) == set() + + +def test_only_declared_differences_are_tolerated(): + assert _undeclared({"type", "errors.display_name[0]", "developer_message[0].key"}) == set() + assert _undeclared({"display_name.value", "typed"}) == {"display_name.value", "typed"} + + +class _ParityMixin: + """ + v1 against v0 for the same course and caller: reads and updates return + identical bodies, and errors differ only by the declared differences. + """ + + def reader_for(self, course): + """Return a user who may read and change the course's settings.""" + raise NotImplementedError + + def outsider(self): + """Return a signed-in user with no access to the course.""" + raise NotImplementedError + + def setUp(self): + super().setUp() + self.course = CourseFactory.create(display_name="Parity") + self.reader = self.reader_for(self.course) + self.observed = set() + + def compare(self, legacy, new, expected_status): + """Assert both statuses and that the bodies differ only as declared; return both bodies.""" + assert legacy.status_code == expected_status, legacy.content + assert new.status_code == expected_status, new.content + legacy_body, new_body = json.loads(legacy.content or b"null"), json.loads(new.content or b"null") + changed = _changed(legacy_body, new_body) + assert _undeclared(changed) == set(), f"undeclared differences: legacy={legacy_body} new={new_body}" + self.observed |= changed + return legacy_body, new_body + + def test_equivalent_reads_return_identical_bodies(self): + client = _client(self.reader) + for new_params, legacy_params in EQUIVALENT_READS: + legacy_body, new_body = self.compare( + client.get(legacy_url(self.course.id), legacy_params), + client.get(url(self.course.id), new_params), + 200, + ) + assert _changed(legacy_body, new_body) == set(), (new_params, legacy_params) + assert list(new_body) == list(legacy_body) + assert new_body != {} + + def test_the_same_parameters_return_different_settings(self): + """The default view is not v0's default: the difference is exactly the excluded settings.""" + client = _client(self.reader) + legacy_body = client.get(legacy_url(self.course.id)).json() + new_body = client.get(url(self.course.id)).json() + assert set(legacy_body) - set(new_body) == set(every_setting(self.course.id)) - set( + editable_settings(self.course.id) + ) + assert set(new_body) < set(legacy_body) + assert {name: legacy_body[name] for name in new_body} == new_body + + def test_an_update_returns_an_identical_body(self): + body = {"display_name": {"value": "Changed by both"}, "days_early_for_beta": {"value": 2.0}} + client = _client(self.reader) + new = client.patch(url(self.course.id), body, format="json") + legacy = client.patch(legacy_url(self.course.id), body, format="json") + legacy_body, new_body = self.compare(legacy, new, 200) + assert _changed(legacy_body, new_body) == set() + assert new_body["display_name"]["value"] == "Changed by both" + + def _error_pairs(self): + """``(legacy, new, status)`` for each error both addresses produce.""" + refused = {INTEGER_SETTING: {"value": "lots"}} + cases = [ + (_client(), self.course.id, "get", None, 401), + (_client(self.outsider()), self.course.id, "get", None, 403), + (_client(self.outsider()), UNKNOWN_COURSE_KEY, "get", None, 404), + (_client(self.reader), self.course.id, "patch", refused, 400), + ] + for client, course_key, method, body, status in cases: + call = getattr(client, method) + kwargs = {"format": "json"} if body is not None else {} + legacy = call(legacy_url(course_key), body, **kwargs) + new = call(url(course_key), body, **kwargs) + yield legacy, new, status + + def test_errors_differ_only_by_the_envelope(self): + for legacy, new, status in self._error_pairs(): + self.compare(legacy, new, status) + + def test_a_refused_update_names_the_settings_v0_names_with_a_fixed_message(self): + *_, (legacy, new, status) = self._error_pairs() + legacy_body, new_body = self.compare(legacy, new, status) + validator_texts = {entry["key"]: entry["message"] for entry in legacy_body["developer_message"]} + assert new_body["errors"] == {name: [INVALID_SETTING_MESSAGE] for name in validator_texts} + assert INVALID_SETTING_MESSAGE not in validator_texts.values() + assert [text for text in validator_texts.values() if text in new.content.decode()] == [] + + def test_a_malformed_entry_of_a_setting_authors_do_not_edit_is_ignored_by_both(self): + client = _client(self.reader) + for body in ({"tabs": "not tabs"}, {"start": {}}, {"tabs": "not tabs", "display_name": {"value": "Both"}}): + new = client.patch(url(self.course.id), body, format="json") + legacy = client.patch(legacy_url(self.course.id), body, format="json") + legacy_body, new_body = self.compare(legacy, new, 200) + assert _changed(legacy_body, new_body) == set(), body + + def test_every_declared_difference_occurs(self): + for legacy, new, status in self._error_pairs(): + self.compare(legacy, new, status) + missing = [ + path for path, _ in INTENTIONAL_DIFFERENCES + if not any(key == path or key.startswith(f"{path}.") or key.startswith(f"{path}[") for key in self.observed) + ] + assert missing == [] + + +class AdvancedSettingsParityLegacyPermissionsTest(_ParityMixin, ModuleStoreTestCase): + """Parity read and written by course staff, with policy-based authoring permissions off.""" + + def reader_for(self, course): + reader = UserFactory() + CourseStaffRole(course.id).add_users(reader) + return reader + + def outsider(self): + return UserFactory() + + +class AdvancedSettingsParityAuthzTest(_ParityMixin, CourseAuthoringAuthzTestMixin, ModuleStoreTestCase): + """Parity read and written by a course staff role, with policy-based authoring permissions on.""" + + def reader_for(self, course): + reader = UserFactory() + self.add_user_to_role_in_course(reader, COURSE_STAFF.external_key, course.id) + return reader + + def outsider(self): + return self.unauthorized_user + + +# --------------------------------------------------------------------------- +# Side effects of reads +# --------------------------------------------------------------------------- + +# The one write the permission library makes on a database's first role or +# permission lookup, and the model signals its save sends. +FIRST_LOOKUP_WRITES = [("INSERT", PolicyCacheControl._meta.db_table)] # pylint: disable=protected-access +FIRST_LOOKUP_SIGNALS = [("pre_save", "PolicyCacheControl"), ("post_save", "PolicyCacheControl")] + +# Signals every request sends without writing anything. +_NEUTRAL_SIGNALS = ( + core_signals.request_started, + core_signals.request_finished, + model_signals.pre_init, + model_signals.post_init, + crum_signals.current_user_getter, + crum_signals.current_user_setter, +) +_MODEL_SIGNAL_NAMES = { + model_signals.pre_save: "pre_save", + model_signals.post_save: "post_save", + model_signals.pre_delete: "pre_delete", + model_signals.post_delete: "post_delete", + model_signals.m2m_changed: "m2m_changed", +} + +READS = [("get", {}), ("get", FULL), ("get", {"fields": "display_name"}), ("head", {}), ("head", FULL)] + + +@contextmanager +def count_mongo_calls(): + """Count Mongo reads and writes made inside the block.""" + reads = ("find", "find_one", "count_documents", "aggregate") + writes = ("insert_one", "insert_many", "replace_one", "update_one", "update_many", "bulk_write", + "delete_one", "delete_many", "find_one_and_update", "find_one_and_replace", "find_one_and_delete") + counts = {"reads": 0, "writes": 0} + + def counting(kind, original): + def wrapper(self, *args, **kwargs): + counts[kind] += 1 + return original(self, *args, **kwargs) + return wrapper + + collection = pymongo.collection.Collection + with patch.multiple(collection, **{ + name: counting("reads" if name in reads else "writes", getattr(collection, name)) for name in reads + writes + }): + yield counts + + +def sql_writes(queries): + """``(statement, table)`` for each INSERT, UPDATE and DELETE among the captured ``queries``.""" + found = [] + for query in queries.captured_queries: + words = query["sql"].split() + statement = words[0].upper() + if statement in ("INSERT", "DELETE"): + found.append((statement, words[2].strip('"`'))) + elif statement == "UPDATE": + found.append((statement, words[1].strip('"`'))) + return found + + +@contextmanager +def record_effects(test_case): + """ + Record, inside the block, SQL writes, Mongo writes, modulestore updates, + every Django signal other than the neutral ones (as ``(name, sender)``) and + every openedx-event. Commit callbacks registered inside the block are run, + so anything sent on commit is seen. + """ + effects = {"signals": [], "events": [], "update_item": []} + send, send_robust, send_event = Signal.send, Signal.send_robust, OpenEdxPublicSignal.send_event + update_item = MixedModuleStore.update_item + + def record(signal, sender): + if any(signal is neutral for neutral in _NEUTRAL_SIGNALS): + return + name = _MODEL_SIGNAL_NAMES.get(signal, repr(signal)) + effects["signals"].append((name, getattr(sender, "__name__", repr(sender)))) + + def spy_send(signal, sender, **named): + record(signal, sender) + return send(signal, sender, **named) + + def spy_send_robust(signal, sender, **named): + record(signal, sender) + return send_robust(signal, sender, **named) + + def spy_send_event(signal, *args, **kwargs): + effects["events"].append(signal.event_type) + return send_event(signal, *args, **kwargs) + + def spy_update_item(store, xblock, *args, **kwargs): + effects["update_item"].append(str(xblock.location)) + return update_item(store, xblock, *args, **kwargs) + + with CaptureQueriesContext(connection) as queries, \ + count_mongo_calls() as mongo, \ + patch.object(Signal, "send", spy_send), \ + patch.object(Signal, "send_robust", spy_send_robust), \ + patch.object(OpenEdxPublicSignal, "send_event", spy_send_event), \ + patch.object(MixedModuleStore, "update_item", spy_update_item): + # Innermost, so the callbacks run on its exit while every spy above is installed. + with test_case.captureOnCommitCallbacks(execute=True): + yield effects + effects["sql_writes"] = sql_writes(queries) + effects["mongo_writes"] = mongo["writes"] + + +class _ReadsAreSideEffectFreeMixin: + """ + Every read (default, full, named settings, and HEAD on both views) writes + no row and no Mongo document, stores nothing through the modulestore and + sends no signal or event. The reads start on a database that has never + served a role or permission lookup, so the first read may insert the + permission library's one policy version row and nothing else; v0 inserts + the same row. + """ + + def reader_for(self, course): + """Return a user who may read ``course``, leaving the database as no lookup had touched it.""" + raise NotImplementedError + + def setUp(self): + super().setUp() + self.course = CourseFactory.create() + + def test_reads_have_no_side_effects(self): + client = _client(self.reader_for(self.course)) + assert not PolicyCacheControl.objects.exists() + for index, (method, params) in enumerate(READS): + with record_effects(self) as effects: + response = getattr(client, method)(url(self.course.id), params) + read = (index, method, params) + first = index == 0 + assert response.status_code == 200, (read, response.content) + assert effects["sql_writes"] == (FIRST_LOOKUP_WRITES if first else []), read + assert effects["signals"] == (FIRST_LOOKUP_SIGNALS if first else []), read + assert effects["events"] == [], read + assert effects["mongo_writes"] == 0, read + assert effects["update_item"] == [], read + + def test_the_v0_read_makes_the_same_first_lookup_write(self): + client = _client(self.reader_for(self.course)) + with record_effects(self) as effects: + response = client.get(legacy_url(self.course.id), LEGACY_DEFAULT) + assert response.status_code == 200, response.content + assert effects["sql_writes"] == FIRST_LOOKUP_WRITES + assert effects["signals"] == FIRST_LOOKUP_SIGNALS + assert effects["mongo_writes"] == 0 + + def test_the_recorder_sees_writes_signals_and_modulestore_updates(self): + """The checks above would notice a write: one made inside the recorder is seen.""" + with record_effects(self) as effects: + UserFactory() + course = modulestore().get_course(self.course.id) + modulestore().update_item(course, self.user.id) + assert ("INSERT", "auth_user") in effects["sql_writes"] + assert ("post_save", "User") in effects["signals"] + assert effects["mongo_writes"] > 0 + assert effects["update_item"] == [str(course.location)] + + def test_the_recorder_sees_what_commit_callbacks_do(self): + """Work deferred to the end of the transaction is recorded too.""" + probe = Signal() + + class Sender: + pass + + with record_effects(self) as effects: + transaction.on_commit(lambda: probe.send(sender=Sender)) + transaction.on_commit(lambda: UserFactory(username="made_on_commit")) + assert (repr(probe), "Sender") in effects["signals"] + assert ("INSERT", "auth_user") in effects["sql_writes"] + assert ("post_save", "User") in effects["signals"] + + +class AdvancedSettingsReadSideEffectsLegacyPermissionsTest(_ReadsAreSideEffectFreeMixin, ModuleStoreTestCase): + """Reads by course staff with policy-based authoring permissions off.""" + + def reader_for(self, course): + reader = UserFactory() + CourseStaffRole(course.id).add_users(reader) + PolicyCacheControl.objects.all().delete() + return reader + + +class AdvancedSettingsReadSideEffectsAuthzTest( + _ReadsAreSideEffectFreeMixin, CourseAuthoringAuthzTestMixin, ModuleStoreTestCase, +): + """ + Reads by a course auditor with policy-based authoring permissions on. The + role is assigned through the permission library, which creates the policy + version row; it is deleted before the reads. + """ + + def reader_for(self, course): + reader = UserFactory() + self.add_user_to_role_in_course(reader, COURSE_AUDITOR.external_key, course.id) + PolicyCacheControl.objects.all().delete() + return reader + + +# --------------------------------------------------------------------------- +# Query counts +# --------------------------------------------------------------------------- + +def _measure(request): + """SQL queries, Mongo reads and Mongo writes of the second of two identical requests.""" + request() + with CaptureQueriesContext(connection) as queries, count_mongo_calls() as mongo: + response = request() + assert response.status_code == 200, response.content + return len(queries.captured_queries), mongo["reads"], mongo["writes"] + + +class _QueryCountMixin: + """Each read costs no more SQL queries or Mongo reads than the v0 read returning the same settings.""" + + def reader_for(self, course): + raise NotImplementedError + + def test_reads_cost_no_more_than_the_equivalent_v0_reads(self): + course = CourseFactory.create() + client = _client(self.reader_for(course)) + for new_params, legacy_params in EQUIVALENT_READS[:2]: + legacy = _measure(lambda params=legacy_params: client.get(legacy_url(course.id), params)) + new = _measure(lambda params=new_params: client.get(url(course.id), params)) + assert new[0] <= legacy[0], (new_params, new, legacy) + assert new[1] <= legacy[1], (new_params, new, legacy) + assert new[2] == legacy[2] == 0, (new_params, new, legacy) + + +class AdvancedSettingsQueryCountLegacyPermissionsTest(_QueryCountMixin, ModuleStoreTestCase): + """Costs read by course staff, with policy-based authoring permissions off.""" + + def reader_for(self, course): + reader = UserFactory() + CourseStaffRole(course.id).add_users(reader) + return reader + + +class AdvancedSettingsQueryCountAuthzTest(CourseAuthoringAuthzTestMixin, _QueryCountMixin, ModuleStoreTestCase): + """Costs read by a course auditor, with policy-based authoring permissions on.""" + + def reader_for(self, course): + reader = UserFactory() + self.add_user_to_role_in_course(reader, COURSE_AUDITOR.external_key, course.id) + return reader + + +# --------------------------------------------------------------------------- +# Addresses and methods +# --------------------------------------------------------------------------- + +COURSE_KEY = "course-v1:edX+DemoX+Demo_Course" + + +@ddt.ddt +class AdvancedSettingsRoutingTest(SharedModuleStoreTestCase): + """The address, the v0 address, and requests no route or method serves.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def test_the_address(self): + assert url(COURSE_KEY) == f"/api/authoring/v1/courses/{COURSE_KEY}/advanced_settings/" + match = resolve(url(COURSE_KEY)) + assert match.func.cls is CourseAdvancedSettingsViewSet + assert routed_actions(match.func) == {"get": "retrieve", "patch": "partial_update"} + + def test_the_v0_address_is_unchanged(self): + assert legacy_url(COURSE_KEY) == f"/api/contentstore/v0/advanced_settings/{COURSE_KEY}" + view = resolve(legacy_url(COURSE_KEY)).func.cls + assert (view.__module__, view.__name__) == LEGACY_VIEW + + def test_v0_still_takes_a_slash_separated_key_that_v1_does_not(self): + view = resolve("/api/contentstore/v0/advanced_settings/edX/DemoX/Demo_Course").func.cls + assert (view.__module__, view.__name__) == LEGACY_VIEW + with pytest.raises(Resolver404): + resolve("/api/authoring/v1/courses/edX/DemoX/Demo_Course/advanced_settings/") + + def test_the_url_names_are_unique_in_the_namespace(self): + names = [pattern.name for pattern in authoring_urls.urlpatterns] + assert len(names) == len(set(names)) + assert "course_advanced_settings" in names + + # The error library does not catalogue MethodNotAllowed or NotAcceptable, so + # these answer with the envelope typed internal. Change the expected type + # here once it does. + + @ddt.data("post", "put", "delete") + def test_other_methods_are_not_allowed(self, method): + response = getattr(_client(UserFactory(is_staff=True)), method)(url(self.course.id), {}, format="json") + assert_error_envelope(response, expected_status=405, expected_type_slug="internal") + + def test_unsupported_accept_is_refused_with_406(self): + response = _client(UserFactory(is_staff=True)).get(url(self.course.id), HTTP_ACCEPT="application/xml") + assert_error_envelope(response, expected_status=406, expected_type_slug="internal") + + # Nor ParseError or UnsupportedMediaType: a malformed or unsupported body is + # answered with its own status but typed internal. Change the expected type + # here once the library catalogues them. + + def test_a_malformed_json_body_is_a_400_typed_internal(self): + response = _client(UserFactory(is_staff=True)).patch( + url(self.course.id), data="{", content_type="application/json", + ) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="internal") + assert envelope["title"] == "Internal Server Error" + + def test_an_unsupported_media_type_is_a_415_typed_internal(self): + response = _client(UserFactory(is_staff=True)).patch( + url(self.course.id), data="display_name=Renamed", content_type="text/plain", + ) + envelope = assert_error_envelope(response, expected_status=415, expected_type_slug="internal") + assert envelope["title"] == "Internal Server Error" + assert stored_display_name(self.course.id) != "Renamed" + + # These addresses match no route, so the service's HTML 404 page answers + # them. Change these to the error envelope once a catch-all route under the + # mount answers unknown addresses. + + @ddt.data("not-a-course-key", "edX/DemoX/Demo_Course", "edX+DemoX+Demo_Course") + def test_an_unparsable_or_slash_form_key_is_an_html_404(self, key): + response = _client(UserFactory(is_staff=True)).get(f"/api/authoring/v1/courses/{key}/advanced_settings/") + assert response.status_code == 404 + assert response["Content-Type"].startswith("text/html") + + def test_the_address_without_its_trailing_slash_redirects_to_it(self): + """No route answers the address without its slash; the service redirects a read to the slashed one.""" + address = f"/api/authoring/v1/courses/{self.course.id}/advanced_settings" + response = _client(UserFactory(is_staff=True)).get(address) + assert response.status_code == 301 + assert response["Location"] == url(self.course.id) + with pytest.raises(Resolver404): + resolve(address) + + +# --------------------------------------------------------------------------- +# The published schema +# --------------------------------------------------------------------------- + +class AdvancedSettingsSchemaTest(TestCase): + """The read and the update as published in the CMS authoring schema.""" + + PATH = "/api/authoring/v1/courses/{course_key}/advanced_settings/" + LEGACY = "/api/contentstore/v0/advanced_settings/{course_id}" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.schema = _published_schema() + cls.components = cls.schema["components"]["schemas"] + + @staticmethod + def _ref(node): + return node["content"]["application/json"]["schema"]["$ref"].rsplit("/", 1)[-1] + + def test_the_read_and_the_update_are_published_for_the_sdk(self): + operations = self.schema["paths"][self.PATH] + assert sorted(operations) == ["get", "patch"] + assert operations["get"]["operationId"] == "v1_courses_advanced_settings_retrieve" + assert operations["patch"]["operationId"] == "v1_courses_advanced_settings_partial_update" + for operation in operations.values(): + assert operation["tags"] == ["openedx-platform-sdk"] + assert "deprecated" not in operation + assert self._ref(operation["responses"]["200"]) == "CourseAdvancedSettings.v1" + for status in ("400", "401", "403", "404"): + assert self._ref(operation["responses"][status]) == "ErrorResponse", status + + def test_the_read_parameters(self): + parameters = {p["name"]: p for p in self.schema["paths"][self.PATH]["get"]["parameters"]} + assert set(parameters) == {"course_key", "view", "fields"} + assert parameters["course_key"]["in"] == "path" + assert parameters["course_key"]["schema"] == {"type": "string", "pattern": "^[^/]+$"} + assert parameters["view"]["in"] == parameters["fields"]["in"] == "query" + assert not parameters["view"].get("required") + assert not parameters["fields"].get("required") + assert parameters["view"]["schema"] == {"type": "string", "enum": ["full", "minimal"], "default": "minimal"} + assert parameters["fields"]["schema"] == {"type": "string"} + + def test_the_settings_are_an_object_keyed_by_setting_name(self): + settings = self.components["CourseAdvancedSettings.v1"] + assert settings["type"] == "object" + assert settings["additionalProperties"] == {"$ref": "#/components/schemas/AdvancedSetting.v1"} + assert "properties" not in settings + setting = self.components["AdvancedSetting.v1"] + assert set(setting["properties"]) == SETTING_MEMBERS + assert set(setting["required"]) == SETTING_MEMBERS + nullable = {name for name, field in setting["properties"].items() if field.get("nullable")} + assert nullable == {"value", "help"} + + def test_the_update_body_is_json_settings_each_with_a_value(self): + body = self.schema["paths"][self.PATH]["patch"]["requestBody"] + assert list(body["content"]) == ["application/json"] + assert self._ref(body) == "PatchedCourseAdvancedSettingsUpdate.v1" + update = self.components[self._ref(body)] + assert update["type"] == "object" + assert update["additionalProperties"] == {"$ref": "#/components/schemas/AdvancedSettingUpdate.v1"} + entry = self.components["AdvancedSettingUpdate.v1"] + assert set(entry["properties"]) == {"value"} + assert entry["required"] == ["value"] + assert entry["properties"]["value"]["nullable"] is True + + def test_every_member_is_described(self): + for name in ("CourseAdvancedSettings.v1", "AdvancedSetting.v1", "AdvancedSettingUpdate.v1"): + component = self.components[name] + assert component.get("description"), name + properties = component.get("properties", {}) + assert [field for field, spec in properties.items() if not spec.get("description")] == [], name + + def test_the_examples_are_valid_bodies(self): + get = self.schema["paths"][self.PATH]["get"] + patch_ = self.schema["paths"][self.PATH]["patch"] + response_example = get["responses"]["200"]["content"]["application/json"]["examples"]["AdvancedSettings"] + assert_matches_published_schema(response_example["value"], "CourseAdvancedSettings.v1") + request_example = patch_["requestBody"]["content"]["application/json"]["examples"]["NewValues"] + assert_matches_published_schema(request_example["value"], self._ref(patch_["requestBody"])) + + def test_the_v0_operations_are_deprecated_and_left_out_of_the_sdk(self): + operations = self.schema["paths"][self.LEGACY] + assert sorted(operations) == ["get", "patch"] + assert operations["get"]["operationId"] == "v0_advanced_settings_retrieve" + assert operations["patch"]["operationId"] == "v0_advanced_settings_partial_update" + for operation in operations.values(): + assert operation["deprecated"] is True + assert "openedx-platform-sdk" not in operation.get("tags", []) + + def test_only_superseded_or_migrated_addresses_are_deprecated(self): + """No operation is marked deprecated except by the legacy-address lists, so a stray mark is caught.""" + deprecated = { + (path, method) + for path, operations in self.schema["paths"].items() + for method, operation in operations.items() + if isinstance(operation, dict) and operation.get("deprecated") + } + assert {(self.LEGACY, "get"), (self.LEGACY, "patch")} <= deprecated + assert {(self.PATH, "get"), (self.PATH, "patch")} & deprecated == set() + stray = sorted( + (path, method) for path, method in deprecated + if path not in SUPERSEDED_PATHS and not path.startswith(LEGACY_MIGRATED_PATH_PREFIXES) + ) + assert stray == [] From ac6fe16116d073555aa42a1d61884a5a56e7bff5 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Sun, 25 Oct 2026 15:09:23 +0500 Subject: [PATCH 3/9] docs: mark the v0 advanced settings deprecated in favour of authoring v1 GET/PATCH /api/contentstore/v0/advanced_settings/{course_id} now has a conforming successor at /api/authoring/v1/courses/{course_key}/advanced_settings/. Both v0 operations are marked deprecated in the CMS schema through an exact SUPERSEDED_PATHS entry, and the view's docstrings name the successor, how each v0 read maps to a v1 read, and the deprecation ticket. The view itself is unchanged. The schema hook test that used this path as its example of an unrelated contentstore path now uses the heartbeat, which is not deprecated. --- .../rest_api/v0/views/advanced_settings.py | 12 ++++++++++++ cms/lib/spectacular.py | 10 ++++++---- cms/lib/tests/test_spectacular.py | 13 ++++++++++++- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py index c66663cd208d..ea119ea895e0 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py @@ -23,6 +23,9 @@ class AdvancedCourseSettingsView(DeveloperErrorViewMixin, APIView): """ View for getting and setting the advanced settings for a course. + + Deprecated: use ``GET`` and ``PATCH /api/authoring/v1/courses/{course_key}/advanced_settings/`` + instead. See https://github.com/openedx/openedx-platform/issues/39128. """ class FilterQuery(forms.Form): @@ -65,6 +68,12 @@ def get(self, request: Request, course_id: str): """ Get an object containing all the advanced settings in a course. + Deprecated: use ``GET /api/authoring/v1/courses/{course_key}/advanced_settings/``. + Equivalent v1 reads: without ``view`` = this endpoint with ``fetch_all=0``; + ``view=full`` = this endpoint's default; ``fields`` replaces ``filter_fields`` + and selects within the chosen view. + See https://github.com/openedx/openedx-platform/issues/39128. + **Example Request** GET /api/contentstore/v0/advanced_settings/{course_id} @@ -148,6 +157,9 @@ def patch(self, request: Request, course_id: str): """ Update a course's advanced settings. + Deprecated: use ``PATCH /api/authoring/v1/courses/{course_key}/advanced_settings/``, + which accepts this body unchanged. See https://github.com/openedx/openedx-platform/issues/39128. + **Example Request** PATCH /api/contentstore/v0/advanced_settings/{course_id} { diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 662cf80c65f0..b4292d975e96 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -4,15 +4,17 @@ from drf_spectacular.openapi import AutoSchema -# Legacy operations superseded by a conforming address under /api/authoring/. -# Each is listed in the truncated spelling as well as the full one: the routes -# make the final character optional, so both addresses resolve, and which one -# the generator emits depends on how it renders that regex. +# Legacy operations superseded by a conforming address under /api/authoring/, +# matched on the exact path. A route that makes its final character optional +# resolves with and without it, and the generator may emit either spelling, so +# such a route is listed in both. SUPERSEDED_PATHS = ( "/api/contentstore/v0/youtube_transcripts/{course_id}/chec", "/api/contentstore/v0/youtube_transcripts/{course_id}/check", "/api/contentstore/v0/youtube_transcripts/{course_id}/uploa", "/api/contentstore/v0/youtube_transcripts/{course_id}/upload", + # → /api/authoring/v1/courses/{course_key}/advanced_settings/ + "/api/contentstore/v0/advanced_settings/{course_id}", ) # Legacy addresses of APIs migrated to /api/authoring/, marked deprecated for diff --git a/cms/lib/tests/test_spectacular.py b/cms/lib/tests/test_spectacular.py index 4581f23a6148..ba7732a260e6 100644 --- a/cms/lib/tests/test_spectacular.py +++ b/cms/lib/tests/test_spectacular.py @@ -76,6 +76,7 @@ def test_both_spellings_of_each_superseded_path_are_listed(self): "/api/contentstore/v0/youtube_transcripts/{course_id}/check", "/api/contentstore/v0/youtube_transcripts/{course_id}/uploa", "/api/contentstore/v0/youtube_transcripts/{course_id}/upload", + "/api/contentstore/v0/advanced_settings/{course_id}", } def test_the_conforming_paths_are_left_unmarked(self): @@ -83,8 +84,18 @@ def test_the_conforming_paths_are_left_unmarked(self): result = self._run({"paths": {conforming: {"get": {"operationId": "x"}}}}) assert result["paths"][conforming]["get"] == {"operationId": "x"} + def test_the_advanced_settings_operations_are_marked_and_their_successor_is_not(self): + legacy = "/api/contentstore/v0/advanced_settings/{course_id}" + conforming = "/api/authoring/v1/courses/{course_key}/advanced_settings/" + result = self._run({"paths": { + legacy: {"get": {}, "patch": {}}, + conforming: {"get": {}, "patch": {}}, + }}) + assert result["paths"][legacy] == {"get": {"deprecated": True}, "patch": {"deprecated": True}} + assert result["paths"][conforming] == {"get": {}, "patch": {}} + def test_an_unrelated_contentstore_path_is_left_unmarked(self): - other = "/api/contentstore/v0/advanced_settings/{course_id}" + other = "/api/contentstore/v0/heartbeat" result = self._run({"paths": {other: {"get": {}}}}) assert "deprecated" not in result["paths"][other]["get"] From 2376c3811de25e879f29e0a393c5f44f8a62c652 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Sun, 25 Oct 2026 17:55:19 +0500 Subject: [PATCH 4/9] test: import the authoring URLconf inside the test that needs it Importing cms.djangoapps.contentstore.rest_api.v1.authoring_urls at module level builds its routes before cms/urls.py registers the course_key path converter, so collecting the module failed with an ImproperlyConfigured error. The URL-name test now imports it when it runs, as the youtube transcript tests do. Also wraps a line pylint reported as too long. --- .../rest_api/v1/views/tests/test_advanced_settings.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py index 38973d752674..e2ff158b0059 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py @@ -38,7 +38,6 @@ from rest_framework.test import APIClient, APIRequestFactory, force_authenticate from rest_framework.utils.encoders import JSONEncoder -from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls from cms.djangoapps.contentstore.rest_api.v1.advanced_settings_service import ( INVALID_SETTING_MESSAGE, SETTINGS_NOT_SAVED_MESSAGE, @@ -748,7 +747,10 @@ def test_a_malformed_entry_of_a_setting_authors_do_not_edit_is_ignored(self): def test_a_failure_saving_the_tabs_is_reported_without_its_text(self): target = "cms.djangoapps.contentstore.views.course._refresh_course_tabs" - with patch(target, side_effect=InvalidTabsException("tab internals")), self.assertLogs(SERVICE_LOGGER, "WARNING"): + with ( + patch(target, side_effect=InvalidTabsException("tab internals")), + self.assertLogs(SERVICE_LOGGER, "WARNING"), + ): response = self.patch({"display_name": {"value": "Not stored"}}) envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") assert envelope["errors"] == {"non_field_errors": [SETTINGS_NOT_SAVED_MESSAGE]} @@ -1414,6 +1416,8 @@ def test_v0_still_takes_a_slash_separated_key_that_v1_does_not(self): resolve("/api/authoring/v1/courses/edX/DemoX/Demo_Course/advanced_settings/") def test_the_url_names_are_unique_in_the_namespace(self): + # Imported here: at module import the URL converters are not registered yet. + from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls # noqa: PLC0415 names = [pattern.name for pattern in authoring_urls.urlpatterns] assert len(names) == len(set(names)) assert "course_advanced_settings" in names From 0131b820ceba61217a2da56f603bb0124d818463 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Sun, 25 Oct 2026 18:33:41 +0500 Subject: [PATCH 5/9] test: align the advanced settings tests with the platform's behaviour The first CI run of the module showed three expectations that did not match the platform: - The legacy role checks consult the policy engine, which admits superusers, so a superuser who is not global staff is served by v0 and v1 alike. The test now expects that. - The proctoring provider field keeps its default unless proctored exams are enabled, so the proctoring tests enable them. - The JWT middleware appends NotJwtRestrictedApplication to the permissions of views that accept JWTs, so the wiring test compares only the classes the view declares. --- .../v1/views/tests/test_advanced_settings.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py index e2ff158b0059..1d95689e94f4 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py @@ -21,6 +21,7 @@ from django.test import SimpleTestCase, TestCase from django.test.utils import CaptureQueriesContext, override_settings from django.urls import Resolver404, resolve, reverse +from edx_rest_framework_extensions.permissions import NotJwtRestrictedApplication from edx_rest_framework_extensions.testing import assert_error_envelope from opaque_keys.edx.keys import CourseKey from openedx_authz.constants.roles import ( @@ -275,9 +276,9 @@ def test_global_staff_is_allowed(self, action): self.assert_access(UserFactory(is_staff=True), action, 200) @ddt.data(*ACTIONS) - def test_a_superuser_who_is_not_global_staff_is_refused(self, action): - """Without policy-based permissions, Studio grants global staff and course roles, not the superuser flag.""" - self.assert_access(UserFactory(is_superuser=True, is_staff=False), action, 403) + def test_a_superuser_who_is_not_global_staff_is_allowed(self, action): + """The role checks consult the policy engine even here, and it admits superusers, as on v0.""" + self.assert_access(UserFactory(is_superuser=True, is_staff=False), action, 200) def test_a_version_only_course_key_is_404(self): """The key parses but names no course run; v0 has no address that takes it.""" @@ -448,7 +449,12 @@ def test_each_action_checks_the_course_then_its_own_permission(self): ("metadata", [IsAuthenticated, IsExistingCourse]), ): view.action = action - assert [type(permission) for permission in view.get_permissions()] == expected, action + # The JWT middleware appends NotJwtRestrictedApplication to views that take JWTs. + declared = [ + type(permission) for permission in view.get_permissions() + if not isinstance(permission, NotJwtRestrictedApplication) + ] + assert declared == expected, action def test_an_action_without_a_declared_permission_is_refused(self): view = CourseAdvancedSettingsViewSet() @@ -854,6 +860,7 @@ def test_a_slash_separated_course_key(self): @ddt.ddt @override_settings( + ENABLE_PROCTORED_EXAMS=True, PROCTORING_BACKENDS={"DEFAULT": "test_proctoring_provider", "valid_provider": {}}, PARTNER_SUPPORT_EMAIL="support@example.com", ) From 9b4d60ea0d40e8b5ddbba8bd782980662bd72a5b Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Sun, 25 Oct 2026 21:46:35 +0500 Subject: [PATCH 6/9] test: share the authoring v1 test helpers The side-effect recorder, the body comparison helpers, the schema helpers and the client helpers in the advanced settings tests are not specific to advanced settings. They move unchanged, under public names, into v1/views/tests/helpers.py so the other authoring v1 endpoint tests reuse one copy instead of each carrying its own. --- .../rest_api/v1/views/tests/helpers.py | 254 +++++++++++++ .../v1/views/tests/test_advanced_settings.py | 340 ++++-------------- 2 files changed, 326 insertions(+), 268 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/tests/helpers.py diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/helpers.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/helpers.py new file mode 100644 index 000000000000..08e7c5748e43 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/helpers.py @@ -0,0 +1,254 @@ +"""Shared helpers for the authoring v1 endpoint tests.""" + +from contextlib import contextmanager +from functools import cache +from unittest.mock import patch + +import jsonschema +import pymongo +from crum import signals as crum_signals +from django.core import signals as core_signals +from django.db import connection +from django.db.models import signals as model_signals +from django.dispatch import Signal +from django.test.utils import CaptureQueriesContext +from edx_rest_framework_extensions.testing import assert_error_envelope +from openedx_authz.models.engine import PolicyCacheControl +from openedx_events.tooling import OpenEdxPublicSignal +from rest_framework.test import APIClient + +from cms.djangoapps.contentstore.rest_api.v2.views.tests.helpers import generate_cms_schema +from xmodule.modulestore.mixed import MixedModuleStore + +COURSE_NOT_FOUND = "The requested course does not exist." +UNKNOWN_COURSE_KEY = "course-v1:NoSuchOrg+NoSuchCourse+NoSuchRun" +VERSION_ONLY_KEY = "course-v1:version@0123456789abcdef01234567" + + +# --------------------------------------------------------------------------- +# Clients and refusals +# --------------------------------------------------------------------------- + +def api_client(user=None): + """An API client that returns server errors as responses, authenticated as ``user`` if given.""" + client = APIClient() + client.raise_request_exception = False + if user is not None: + client.force_authenticate(user=user) + return client + + +def session_client(user): + """A client signed in to a browser session as ``user``.""" + user.set_password("password") + user.save() + client = APIClient() + assert client.login(username=user.username, password="password") + return client + + +def assert_refused(response, status): + """Assert ``response`` is the error envelope for ``status``; a 404 names the missing course.""" + slug = {401: "authn", 403: "authz", 404: "not-found"}[status] + envelope = assert_error_envelope(response, expected_status=status, expected_type_slug=slug) + if status == 404: + assert envelope["detail"] == COURSE_NOT_FOUND + return envelope + + +# --------------------------------------------------------------------------- +# The published schema +# --------------------------------------------------------------------------- + +@cache +def published_schema(): + """The CMS authoring schema, generated once per test process.""" + return generate_cms_schema() + + +def as_json_schema(node): + """An OpenAPI 3.0 schema as JSON Schema: ``nullable`` becomes a null alternative.""" + if isinstance(node, list): + return [as_json_schema(item) for item in node] + if not isinstance(node, dict): + return node + converted = {key: as_json_schema(value) for key, value in node.items() if key != "nullable"} + if node.get("nullable"): + return {"anyOf": [converted, {"type": "null"}]} + return converted + + +def assert_matches_published_schema(body, component): + """Assert ``body`` is valid against the published schema component ``component``.""" + components = as_json_schema(published_schema()["components"]["schemas"]) + jsonschema.validate(body, {"$ref": f"#/components/schemas/{component}", "components": {"schemas": components}}) + + +# --------------------------------------------------------------------------- +# Comparing two response bodies +# --------------------------------------------------------------------------- + +VOLATILE_FIELDS = {"instance"} +_ABSENT = object() + + +def flatten_json(obj, prefix=""): + """ + A ``{json_path: value}`` map of every leaf of ``obj``, without volatile + members. An empty object or array is a leaf, so a container that empties + or changes kind is seen. + """ + flat = {} + if isinstance(obj, dict) and obj: + for key in sorted(obj): + if key in VOLATILE_FIELDS: + continue + flat.update(flatten_json(obj[key], f"{prefix}.{key}" if prefix else key)) + elif isinstance(obj, list) and obj: + for index, item in enumerate(obj): + flat.update(flatten_json(item, f"{prefix}[{index}]")) + else: + flat[prefix] = obj + return flat + + +def same_json(a, b): + """Whether two leaves are equal and of the same JSON type: true is not 1, 1.0 is not 1, {} is not [].""" + return type(a) is type(b) and a == b + + +def changed_paths(legacy, new): + """The json paths whose leaf differs between the two bodies.""" + a, b = flatten_json(legacy), flatten_json(new) + return {key for key in set(a) | set(b) if not same_json(a.get(key, _ABSENT), b.get(key, _ABSENT))} + + +def undeclared_paths(changed, intentional_differences): + """The ``changed`` paths that no ``(path, reason)`` entry of ``intentional_differences`` covers.""" + declared = [path for path, _ in intentional_differences] + return { + key for key in changed + if not any(key == path or key.startswith(f"{path}.") or key.startswith(f"{path}[") for path in declared) + } + + +# --------------------------------------------------------------------------- +# Side effects and costs +# --------------------------------------------------------------------------- + +# The one write the permission library makes on a database's first role or +# permission lookup, and the model signals its save sends. +FIRST_LOOKUP_WRITES = [("INSERT", PolicyCacheControl._meta.db_table)] # pylint: disable=protected-access + + +FIRST_LOOKUP_SIGNALS = [("pre_save", "PolicyCacheControl"), ("post_save", "PolicyCacheControl")] + + +# Signals every request sends without writing anything. +NEUTRAL_SIGNALS = ( + core_signals.request_started, + core_signals.request_finished, + model_signals.pre_init, + model_signals.post_init, + crum_signals.current_user_getter, + crum_signals.current_user_setter, +) + + +MODEL_SIGNAL_NAMES = { + model_signals.pre_save: "pre_save", + model_signals.post_save: "post_save", + model_signals.pre_delete: "pre_delete", + model_signals.post_delete: "post_delete", + model_signals.m2m_changed: "m2m_changed", +} + + +@contextmanager +def count_mongo_calls(): + """Count Mongo reads and writes made inside the block.""" + reads = ("find", "find_one", "count_documents", "aggregate") + writes = ("insert_one", "insert_many", "replace_one", "update_one", "update_many", "bulk_write", + "delete_one", "delete_many", "find_one_and_update", "find_one_and_replace", "find_one_and_delete") + counts = {"reads": 0, "writes": 0} + + def counting(kind, original): + def wrapper(self, *args, **kwargs): + counts[kind] += 1 + return original(self, *args, **kwargs) + return wrapper + + collection = pymongo.collection.Collection + with patch.multiple(collection, **{ + name: counting("reads" if name in reads else "writes", getattr(collection, name)) for name in reads + writes + }): + yield counts + + +def sql_writes(queries): + """``(statement, table)`` for each INSERT, UPDATE and DELETE among the captured ``queries``.""" + found = [] + for query in queries.captured_queries: + words = query["sql"].split() + statement = words[0].upper() + if statement in ("INSERT", "DELETE"): + found.append((statement, words[2].strip('"`'))) + elif statement == "UPDATE": + found.append((statement, words[1].strip('"`'))) + return found + + +@contextmanager +def record_effects(test_case): + """ + Record, inside the block, SQL writes, Mongo writes, modulestore updates, + every Django signal other than the neutral ones (as ``(name, sender)``) and + every openedx-event. Commit callbacks registered inside the block are run, + so anything sent on commit is seen. + """ + effects = {"signals": [], "events": [], "update_item": []} + send, send_robust, send_event = Signal.send, Signal.send_robust, OpenEdxPublicSignal.send_event + update_item = MixedModuleStore.update_item + + def record(signal, sender): + if any(signal is neutral for neutral in NEUTRAL_SIGNALS): + return + name = MODEL_SIGNAL_NAMES.get(signal, repr(signal)) + effects["signals"].append((name, getattr(sender, "__name__", repr(sender)))) + + def spy_send(signal, sender, **named): + record(signal, sender) + return send(signal, sender, **named) + + def spy_send_robust(signal, sender, **named): + record(signal, sender) + return send_robust(signal, sender, **named) + + def spy_send_event(signal, *args, **kwargs): + effects["events"].append(signal.event_type) + return send_event(signal, *args, **kwargs) + + def spy_update_item(store, xblock, *args, **kwargs): + effects["update_item"].append(str(xblock.location)) + return update_item(store, xblock, *args, **kwargs) + + with CaptureQueriesContext(connection) as queries, \ + count_mongo_calls() as mongo, \ + patch.object(Signal, "send", spy_send), \ + patch.object(Signal, "send_robust", spy_send_robust), \ + patch.object(OpenEdxPublicSignal, "send_event", spy_send_event), \ + patch.object(MixedModuleStore, "update_item", spy_update_item): + # Innermost, so the callbacks run on its exit while every spy above is installed. + with test_case.captureOnCommitCallbacks(execute=True): + yield effects + effects["sql_writes"] = sql_writes(queries) + effects["mongo_writes"] = mongo["writes"] + + +def measure(request): + """SQL queries, Mongo reads and Mongo writes of the second of two identical requests.""" + request() + with CaptureQueriesContext(connection) as queries, count_mongo_calls() as mongo: + response = request() + assert response.status_code == 200, response.content + return len(queries.captured_queries), mongo["reads"], mongo["writes"] diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py index 1d95689e94f4..27dded1e7e28 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py @@ -2,24 +2,17 @@ import json import uuid -from contextlib import contextmanager from datetime import UTC, datetime, timedelta -from functools import cache from types import SimpleNamespace from unittest.mock import patch import ddt -import jsonschema -import pymongo import pytest -from crum import signals as crum_signals -from django.core import signals as core_signals from django.core.exceptions import ImproperlyConfigured -from django.db import connection, transaction -from django.db.models import signals as model_signals +from django.db import transaction from django.dispatch import Signal from django.test import SimpleTestCase, TestCase -from django.test.utils import CaptureQueriesContext, override_settings +from django.test.utils import override_settings from django.urls import Resolver404, resolve, reverse from edx_rest_framework_extensions.permissions import NotJwtRestrictedApplication from edx_rest_framework_extensions.testing import assert_error_envelope @@ -34,7 +27,6 @@ COURSE_STAFF, ) from openedx_authz.models.engine import PolicyCacheControl -from openedx_events.tooling import OpenEdxPublicSignal from rest_framework.permissions import IsAuthenticated from rest_framework.test import APIClient, APIRequestFactory, force_authenticate from rest_framework.utils.encoders import JSONEncoder @@ -44,7 +36,21 @@ SETTINGS_NOT_SAVED_MESSAGE, ) from cms.djangoapps.contentstore.rest_api.v1.views.advanced_settings import CourseAdvancedSettingsViewSet -from cms.djangoapps.contentstore.rest_api.v2.views.tests.helpers import generate_cms_schema +from cms.djangoapps.contentstore.rest_api.v1.views.tests.helpers import ( + FIRST_LOOKUP_SIGNALS, + FIRST_LOOKUP_WRITES, + UNKNOWN_COURSE_KEY, + VERSION_ONLY_KEY, + api_client, + assert_matches_published_schema, + assert_refused, + changed_paths, + measure, + published_schema, + record_effects, + session_client, + undeclared_paths, +) from cms.djangoapps.contentstore.tests.utils import routed_actions from cms.djangoapps.contentstore.views.permissions import ( CanManageAdvancedSettings, @@ -76,9 +82,6 @@ LEGACY_URL_NAME = "cms.djangoapps.contentstore:v0:course_advanced_settings" LEGACY_VIEW = ("cms.djangoapps.contentstore.rest_api.v0.views.advanced_settings", "AdvancedCourseSettingsView") SERVICE_LOGGER = "cms.djangoapps.contentstore.rest_api.v1.advanced_settings_service" -UNKNOWN_COURSE_KEY = "course-v1:NoSuchOrg+NoSuchCourse+NoSuchRun" -VERSION_ONLY_KEY = "course-v1:version@0123456789abcdef01234567" -COURSE_NOT_FOUND = "The requested course does not exist." FULL = {"view": "full"} LEGACY_DEFAULT = {"fetch_all": "0"} # An integer setting authors edit, so a non-numeric value is refused by its field. @@ -97,24 +100,6 @@ def legacy_url(course_key): return reverse(LEGACY_URL_NAME, kwargs={"course_id": str(course_key)}) -def _client(user=None): - """An API client that returns server errors as responses, authenticated as ``user`` if given.""" - client = APIClient() - client.raise_request_exception = False - if user is not None: - client.force_authenticate(user=user) - return client - - -def _session_client(user): - """A client signed in to a browser session as ``user``.""" - user.set_password("password") - user.save() - client = APIClient() - assert client.login(username=user.username, password="password") - return client - - def stored_course(course_key): """The stored course block of ``course_key`` (a key or its string form), or None when there is none.""" return modulestore().get_course(CourseKey.from_string(str(course_key))) @@ -135,42 +120,10 @@ def every_setting(course_key): return CourseMetadata.fetch_all(stored_course(course_key)) -def _assert_refused(response, status): - """Assert ``response`` is the error envelope for ``status``; a 404 names the missing course.""" - slug = {401: "authn", 403: "authz", 404: "not-found"}[status] - envelope = assert_error_envelope(response, expected_status=status, expected_type_slug=slug) - if status == 404: - assert envelope["detail"] == COURSE_NOT_FOUND - return envelope - - # --------------------------------------------------------------------------- # The published schema # --------------------------------------------------------------------------- -@cache -def _published_schema(): - """The CMS authoring schema, generated once per test process.""" - return generate_cms_schema() - - -def _as_json_schema(node): - """An OpenAPI 3.0 schema as JSON Schema: ``nullable`` becomes a null alternative.""" - if isinstance(node, list): - return [_as_json_schema(item) for item in node] - if not isinstance(node, dict): - return node - converted = {key: _as_json_schema(value) for key, value in node.items() if key != "nullable"} - if node.get("nullable"): - return {"anyOf": [converted, {"type": "null"}]} - return converted - - -def assert_matches_published_schema(body, component): - """Assert ``body`` is valid against the published schema component ``component``.""" - components = _as_json_schema(_published_schema()["components"]["schemas"]) - jsonschema.validate(body, {"$ref": f"#/components/schemas/{component}", "components": {"schemas": components}}) - # --------------------------------------------------------------------------- # Who may call the endpoint @@ -185,13 +138,13 @@ class _AccessMixin: def assert_read(self, user, expected, course_key=None): """Assert the status of a read by ``user``, its body or envelope, and agreement with v0.""" course_key = course_key or self.course.id - response = _client(user).get(url(course_key)) - legacy = _client(user).get(legacy_url(course_key), LEGACY_DEFAULT) + response = api_client(user).get(url(course_key)) + legacy = api_client(user).get(legacy_url(course_key), LEGACY_DEFAULT) assert response.status_code == expected, response.content if expected == 200: assert list(response.json()) == list(editable_settings(course_key)) else: - _assert_refused(response, expected) + assert_refused(response, expected) assert legacy.status_code == expected, f"v0 decided {legacy.status_code} for the same caller" def assert_update(self, user, expected, course_key=None): @@ -200,17 +153,17 @@ def assert_update(self, user, expected, course_key=None): course = stored_course(course_key) before = course.display_name if course is not None else None new_name = f"Renamed {uuid.uuid4().hex[:8]}" - response = _client(user).patch(url(course_key), {"display_name": {"value": new_name}}, format="json") + response = api_client(user).patch(url(course_key), {"display_name": {"value": new_name}}, format="json") assert response.status_code == expected, response.content if expected == 200: assert response.json()["display_name"]["value"] == new_name assert stored_display_name(course_key) == new_name else: - _assert_refused(response, expected) + assert_refused(response, expected) if course is not None: assert stored_display_name(course_key) == before legacy_name = f"Legacy {uuid.uuid4().hex[:8]}" - legacy = _client(user).patch(legacy_url(course_key), {"display_name": {"value": legacy_name}}, format="json") + legacy = api_client(user).patch(legacy_url(course_key), {"display_name": {"value": legacy_name}}, format="json") assert legacy.status_code == expected, f"v0 decided {legacy.status_code} for the same caller" def assert_access(self, user, action, expected, course_key=None): @@ -283,16 +236,16 @@ def test_a_superuser_who_is_not_global_staff_is_allowed(self, action): def test_a_version_only_course_key_is_404(self): """The key parses but names no course run; v0 has no address that takes it.""" user = UserFactory(is_staff=True) - _assert_refused(_client(user).get(url(VERSION_ONLY_KEY)), 404) - _assert_refused( - _client(user).patch(url(VERSION_ONLY_KEY), {"display_name": {"value": "x"}}, format="json"), 404, + assert_refused(api_client(user).get(url(VERSION_ONLY_KEY)), 404) + assert_refused( + api_client(user).patch(url(VERSION_ONLY_KEY), {"display_name": {"value": "x"}}, format="json"), 404, ) def test_a_library_is_not_a_course(self): library_key = LibraryFactory.create().location.library_key user = UserFactory(is_staff=True) - _assert_refused(_client(user).get(url(library_key)), 404) - _assert_refused(_client(user).patch(url(library_key), {}, format="json"), 404) + assert_refused(api_client(user).get(url(library_key)), 404) + assert_refused(api_client(user).patch(url(library_key), {}, format="json"), 404) @ddt.ddt @@ -366,7 +319,7 @@ def test_staff_and_superuser_are_allowed_without_a_role(self, action): def test_an_inactive_session_with_a_role_is_accepted(self): """The policy does not look at whether the account is activated, on v1 as on v0.""" user = self._user_with(COURSE_AUDITOR.external_key) - client = _session_client(user) + client = session_client(user) user.is_active = False user.save() assert client.get(url(self.course.id)).status_code == 200 @@ -519,7 +472,7 @@ def test_a_jwt_caller_may_read_and_update(self): assert stored_display_name(self.course.id) == "By JWT" def test_a_session_caller_may_read_and_update(self): - client = _session_client(self._course_staff()) + client = session_client(self._course_staff()) assert client.get(url(self.course.id)).status_code == 200 response = client.patch(url(self.course.id), {"display_name": {"value": "By session"}}, format="json") assert response.status_code == 200 @@ -532,13 +485,13 @@ def test_an_inactive_session_is_authenticated(self): course role does not count while the account is inactive. """ for user, expected in ((UserFactory(is_staff=True), 200), (self._course_staff(), 403)): - client = _session_client(user) + client = session_client(user) user.is_active = False user.save() response = client.get(url(self.course.id)) assert response.status_code == expected, response.content if expected == 403: - _assert_refused(response, 403) + assert_refused(response, 403) assert client.get(legacy_url(self.course.id)).status_code == expected def test_a_bearer_token_is_refused(self): @@ -546,7 +499,7 @@ def test_a_bearer_token_is_refused(self): user = UserFactory(is_staff=True) token = AccessTokenFactory(user=user, application=ApplicationFactory(user=user)).token header = {"HTTP_AUTHORIZATION": f"Bearer {token}"} - _assert_refused(APIClient().get(url(self.course.id), **header), 401) + assert_refused(APIClient().get(url(self.course.id), **header), 401) assert APIClient().get(legacy_url(self.course.id), **header).status_code == 200 @@ -567,7 +520,7 @@ def setUp(self): super().setUp() reader = UserFactory() CourseStaffRole(self.course.id).add_users(reader) - self.client = _client(reader) + self.client = api_client(reader) def get(self, **params): response = self.client.get(url(self.course.id), params) @@ -645,7 +598,6 @@ def test_head_answers_without_a_body(self): assert response.content == b"" - # --------------------------------------------------------------------------- # Updates # --------------------------------------------------------------------------- @@ -659,7 +611,7 @@ def setUp(self): writer = UserFactory() CourseStaffRole(self.course.id).add_users(writer) self.writer = writer - self.client = _client(writer) + self.client = api_client(writer) def patch(self, body): return self.client.patch(url(self.course.id), body, format="json") @@ -802,7 +754,7 @@ def setUp(self): self.course = CourseFactory.create(display_name="Original") writer = UserFactory() CourseStaffRole(self.course.id).add_users(writer) - self.client = _client(writer) + self.client = api_client(writer) def both(self, method, body=None, **params): """The v0 and v1 responses to the same request.""" @@ -819,7 +771,7 @@ def test_a_course_the_modulestore_returns_nothing_for(self): for method, body in (("get", None), ("patch", {"display_name": {"value": "x"}})): legacy, new = self.both(method, body) assert legacy.status_code == 500, method - _assert_refused(new, 404) + assert_refused(new, 404) def test_an_update_whose_body_is_not_an_object(self): legacy, new = self.both("patch", [1, 2]) @@ -844,10 +796,10 @@ def test_a_bearer_token(self): token = AccessTokenFactory(user=user, application=ApplicationFactory(user=user)).token header = {"HTTP_AUTHORIZATION": f"Bearer {token}"} assert APIClient().get(legacy_url(self.course.id), **header).status_code == 200 - _assert_refused(APIClient().get(url(self.course.id), **header), 401) + assert_refused(APIClient().get(url(self.course.id), **header), 401) body = {"display_name": {"value": "By Bearer"}} assert APIClient().patch(legacy_url(self.course.id), body, format="json", **header).status_code == 200 - _assert_refused(APIClient().patch(url(self.course.id), body, format="json", **header), 401) + assert_refused(APIClient().patch(url(self.course.id), body, format="json", **header), 401) def test_a_slash_separated_course_key(self): """v0 routes the slash form and reports the course unknown in JSON; v1 has no route for it.""" @@ -883,7 +835,7 @@ def _course_staff(self, course): def _patch(self, version, user, course): address = url(course.id) if version == "v1" else legacy_url(course.id) - return _client(user).patch(address, self.CHANGE, format="json") + return api_client(user).patch(address, self.CHANGE, format="json") @ddt.data("v1", "v0") def test_course_staff_may_not_change_it_after_the_course_starts(self, version): @@ -932,7 +884,6 @@ def test_course_staff_may_change_it_before_the_course_starts(self, version): ("developer_message", "the v0 error body carries developer_message; the envelope does not"), ("error_code", "the v0 not-found body carries error_code; the envelope does not"), ] -VOLATILE_FIELDS = {"instance"} # Reads of the two addresses that return the same settings. Their parameters # differ by design: the default view is v0's fetch_all=0 response, view=full @@ -947,57 +898,20 @@ def test_course_staff_may_change_it_before_the_course_starts(self, version): ({**FULL, "fields": "tabs,display_name"}, {"filter_fields": "tabs,display_name"}), ] -_ABSENT = object() - - -def _flatten(obj, prefix=""): - """ - A ``{json_path: value}`` map of every leaf of ``obj``, without volatile - members. An empty object or array is a leaf, so a container that empties - or changes kind is seen. - """ - flat = {} - if isinstance(obj, dict) and obj: - for key in sorted(obj): - if key in VOLATILE_FIELDS: - continue - flat.update(_flatten(obj[key], f"{prefix}.{key}" if prefix else key)) - elif isinstance(obj, list) and obj: - for index, item in enumerate(obj): - flat.update(_flatten(item, f"{prefix}[{index}]")) - else: - flat[prefix] = obj - return flat - - -def _same(a, b): - """Whether two leaves are equal and of the same JSON type: true is not 1, 1.0 is not 1, {} is not [].""" - return type(a) is type(b) and a == b - - -def _changed(legacy, new): - """The json paths whose leaf differs between the two bodies.""" - a, b = _flatten(legacy), _flatten(new) - return {key for key in set(a) | set(b) if not _same(a.get(key, _ABSENT), b.get(key, _ABSENT))} - def _undeclared(changed): """The changed paths no declared intentional difference covers.""" - declared = [path for path, _ in INTENTIONAL_DIFFERENCES] - return { - key for key in changed - if not any(key == path or key.startswith(f"{path}.") or key.startswith(f"{path}[") for path in declared) - } + return undeclared_paths(changed, INTENTIONAL_DIFFERENCES) def test_the_comparison_sees_types_order_free_members_and_empty_containers(): - assert _changed({"a": {"b": []}}, {"a": {}}) == {"a", "a.b"} - assert _changed({"x": True}, {"x": 1}) == {"x"} - assert _changed({"x": 1.0}, {"x": 1}) == {"x"} - assert _changed({"x": {}}, {"x": []}) == {"x"} - assert _changed({"x": None, "y": 1}, {"y": 1}) == {"x"} - assert _changed({"instance": "/a"}, {"instance": "/b"}) == set() - assert _changed({"x": [1, {"y": False}]}, {"x": [1, {"y": False}]}) == set() + assert changed_paths({"a": {"b": []}}, {"a": {}}) == {"a", "a.b"} + assert changed_paths({"x": True}, {"x": 1}) == {"x"} + assert changed_paths({"x": 1.0}, {"x": 1}) == {"x"} + assert changed_paths({"x": {}}, {"x": []}) == {"x"} + assert changed_paths({"x": None, "y": 1}, {"y": 1}) == {"x"} + assert changed_paths({"instance": "/a"}, {"instance": "/b"}) == set() + assert changed_paths({"x": [1, {"y": False}]}, {"x": [1, {"y": False}]}) == set() def test_only_declared_differences_are_tolerated(): @@ -1030,26 +944,26 @@ def compare(self, legacy, new, expected_status): assert legacy.status_code == expected_status, legacy.content assert new.status_code == expected_status, new.content legacy_body, new_body = json.loads(legacy.content or b"null"), json.loads(new.content or b"null") - changed = _changed(legacy_body, new_body) + changed = changed_paths(legacy_body, new_body) assert _undeclared(changed) == set(), f"undeclared differences: legacy={legacy_body} new={new_body}" self.observed |= changed return legacy_body, new_body def test_equivalent_reads_return_identical_bodies(self): - client = _client(self.reader) + client = api_client(self.reader) for new_params, legacy_params in EQUIVALENT_READS: legacy_body, new_body = self.compare( client.get(legacy_url(self.course.id), legacy_params), client.get(url(self.course.id), new_params), 200, ) - assert _changed(legacy_body, new_body) == set(), (new_params, legacy_params) + assert changed_paths(legacy_body, new_body) == set(), (new_params, legacy_params) assert list(new_body) == list(legacy_body) assert new_body != {} def test_the_same_parameters_return_different_settings(self): """The default view is not v0's default: the difference is exactly the excluded settings.""" - client = _client(self.reader) + client = api_client(self.reader) legacy_body = client.get(legacy_url(self.course.id)).json() new_body = client.get(url(self.course.id)).json() assert set(legacy_body) - set(new_body) == set(every_setting(self.course.id)) - set( @@ -1060,21 +974,21 @@ def test_the_same_parameters_return_different_settings(self): def test_an_update_returns_an_identical_body(self): body = {"display_name": {"value": "Changed by both"}, "days_early_for_beta": {"value": 2.0}} - client = _client(self.reader) + client = api_client(self.reader) new = client.patch(url(self.course.id), body, format="json") legacy = client.patch(legacy_url(self.course.id), body, format="json") legacy_body, new_body = self.compare(legacy, new, 200) - assert _changed(legacy_body, new_body) == set() + assert changed_paths(legacy_body, new_body) == set() assert new_body["display_name"]["value"] == "Changed by both" def _error_pairs(self): """``(legacy, new, status)`` for each error both addresses produce.""" refused = {INTEGER_SETTING: {"value": "lots"}} cases = [ - (_client(), self.course.id, "get", None, 401), - (_client(self.outsider()), self.course.id, "get", None, 403), - (_client(self.outsider()), UNKNOWN_COURSE_KEY, "get", None, 404), - (_client(self.reader), self.course.id, "patch", refused, 400), + (api_client(), self.course.id, "get", None, 401), + (api_client(self.outsider()), self.course.id, "get", None, 403), + (api_client(self.outsider()), UNKNOWN_COURSE_KEY, "get", None, 404), + (api_client(self.reader), self.course.id, "patch", refused, 400), ] for client, course_key, method, body, status in cases: call = getattr(client, method) @@ -1096,12 +1010,12 @@ def test_a_refused_update_names_the_settings_v0_names_with_a_fixed_message(self) assert [text for text in validator_texts.values() if text in new.content.decode()] == [] def test_a_malformed_entry_of_a_setting_authors_do_not_edit_is_ignored_by_both(self): - client = _client(self.reader) + client = api_client(self.reader) for body in ({"tabs": "not tabs"}, {"start": {}}, {"tabs": "not tabs", "display_name": {"value": "Both"}}): new = client.patch(url(self.course.id), body, format="json") legacy = client.patch(legacy_url(self.course.id), body, format="json") legacy_body, new_body = self.compare(legacy, new, 200) - assert _changed(legacy_body, new_body) == set(), body + assert changed_paths(legacy_body, new_body) == set(), body def test_every_declared_difference_occurs(self): for legacy, new, status in self._error_pairs(): @@ -1141,112 +1055,10 @@ def outsider(self): # Side effects of reads # --------------------------------------------------------------------------- -# The one write the permission library makes on a database's first role or -# permission lookup, and the model signals its save sends. -FIRST_LOOKUP_WRITES = [("INSERT", PolicyCacheControl._meta.db_table)] # pylint: disable=protected-access -FIRST_LOOKUP_SIGNALS = [("pre_save", "PolicyCacheControl"), ("post_save", "PolicyCacheControl")] - -# Signals every request sends without writing anything. -_NEUTRAL_SIGNALS = ( - core_signals.request_started, - core_signals.request_finished, - model_signals.pre_init, - model_signals.post_init, - crum_signals.current_user_getter, - crum_signals.current_user_setter, -) -_MODEL_SIGNAL_NAMES = { - model_signals.pre_save: "pre_save", - model_signals.post_save: "post_save", - model_signals.pre_delete: "pre_delete", - model_signals.post_delete: "post_delete", - model_signals.m2m_changed: "m2m_changed", -} READS = [("get", {}), ("get", FULL), ("get", {"fields": "display_name"}), ("head", {}), ("head", FULL)] -@contextmanager -def count_mongo_calls(): - """Count Mongo reads and writes made inside the block.""" - reads = ("find", "find_one", "count_documents", "aggregate") - writes = ("insert_one", "insert_many", "replace_one", "update_one", "update_many", "bulk_write", - "delete_one", "delete_many", "find_one_and_update", "find_one_and_replace", "find_one_and_delete") - counts = {"reads": 0, "writes": 0} - - def counting(kind, original): - def wrapper(self, *args, **kwargs): - counts[kind] += 1 - return original(self, *args, **kwargs) - return wrapper - - collection = pymongo.collection.Collection - with patch.multiple(collection, **{ - name: counting("reads" if name in reads else "writes", getattr(collection, name)) for name in reads + writes - }): - yield counts - - -def sql_writes(queries): - """``(statement, table)`` for each INSERT, UPDATE and DELETE among the captured ``queries``.""" - found = [] - for query in queries.captured_queries: - words = query["sql"].split() - statement = words[0].upper() - if statement in ("INSERT", "DELETE"): - found.append((statement, words[2].strip('"`'))) - elif statement == "UPDATE": - found.append((statement, words[1].strip('"`'))) - return found - - -@contextmanager -def record_effects(test_case): - """ - Record, inside the block, SQL writes, Mongo writes, modulestore updates, - every Django signal other than the neutral ones (as ``(name, sender)``) and - every openedx-event. Commit callbacks registered inside the block are run, - so anything sent on commit is seen. - """ - effects = {"signals": [], "events": [], "update_item": []} - send, send_robust, send_event = Signal.send, Signal.send_robust, OpenEdxPublicSignal.send_event - update_item = MixedModuleStore.update_item - - def record(signal, sender): - if any(signal is neutral for neutral in _NEUTRAL_SIGNALS): - return - name = _MODEL_SIGNAL_NAMES.get(signal, repr(signal)) - effects["signals"].append((name, getattr(sender, "__name__", repr(sender)))) - - def spy_send(signal, sender, **named): - record(signal, sender) - return send(signal, sender, **named) - - def spy_send_robust(signal, sender, **named): - record(signal, sender) - return send_robust(signal, sender, **named) - - def spy_send_event(signal, *args, **kwargs): - effects["events"].append(signal.event_type) - return send_event(signal, *args, **kwargs) - - def spy_update_item(store, xblock, *args, **kwargs): - effects["update_item"].append(str(xblock.location)) - return update_item(store, xblock, *args, **kwargs) - - with CaptureQueriesContext(connection) as queries, \ - count_mongo_calls() as mongo, \ - patch.object(Signal, "send", spy_send), \ - patch.object(Signal, "send_robust", spy_send_robust), \ - patch.object(OpenEdxPublicSignal, "send_event", spy_send_event), \ - patch.object(MixedModuleStore, "update_item", spy_update_item): - # Innermost, so the callbacks run on its exit while every spy above is installed. - with test_case.captureOnCommitCallbacks(execute=True): - yield effects - effects["sql_writes"] = sql_writes(queries) - effects["mongo_writes"] = mongo["writes"] - - class _ReadsAreSideEffectFreeMixin: """ Every read (default, full, named settings, and HEAD on both views) writes @@ -1266,7 +1078,7 @@ def setUp(self): self.course = CourseFactory.create() def test_reads_have_no_side_effects(self): - client = _client(self.reader_for(self.course)) + client = api_client(self.reader_for(self.course)) assert not PolicyCacheControl.objects.exists() for index, (method, params) in enumerate(READS): with record_effects(self) as effects: @@ -1281,7 +1093,7 @@ def test_reads_have_no_side_effects(self): assert effects["update_item"] == [], read def test_the_v0_read_makes_the_same_first_lookup_write(self): - client = _client(self.reader_for(self.course)) + client = api_client(self.reader_for(self.course)) with record_effects(self) as effects: response = client.get(legacy_url(self.course.id), LEGACY_DEFAULT) assert response.status_code == 200, response.content @@ -1345,14 +1157,6 @@ def reader_for(self, course): # Query counts # --------------------------------------------------------------------------- -def _measure(request): - """SQL queries, Mongo reads and Mongo writes of the second of two identical requests.""" - request() - with CaptureQueriesContext(connection) as queries, count_mongo_calls() as mongo: - response = request() - assert response.status_code == 200, response.content - return len(queries.captured_queries), mongo["reads"], mongo["writes"] - class _QueryCountMixin: """Each read costs no more SQL queries or Mongo reads than the v0 read returning the same settings.""" @@ -1362,10 +1166,10 @@ def reader_for(self, course): def test_reads_cost_no_more_than_the_equivalent_v0_reads(self): course = CourseFactory.create() - client = _client(self.reader_for(course)) + client = api_client(self.reader_for(course)) for new_params, legacy_params in EQUIVALENT_READS[:2]: - legacy = _measure(lambda params=legacy_params: client.get(legacy_url(course.id), params)) - new = _measure(lambda params=new_params: client.get(url(course.id), params)) + legacy = measure(lambda params=legacy_params: client.get(legacy_url(course.id), params)) + new = measure(lambda params=new_params: client.get(url(course.id), params)) assert new[0] <= legacy[0], (new_params, new, legacy) assert new[1] <= legacy[1], (new_params, new, legacy) assert new[2] == legacy[2] == 0, (new_params, new, legacy) @@ -1435,11 +1239,11 @@ def test_the_url_names_are_unique_in_the_namespace(self): @ddt.data("post", "put", "delete") def test_other_methods_are_not_allowed(self, method): - response = getattr(_client(UserFactory(is_staff=True)), method)(url(self.course.id), {}, format="json") + response = getattr(api_client(UserFactory(is_staff=True)), method)(url(self.course.id), {}, format="json") assert_error_envelope(response, expected_status=405, expected_type_slug="internal") def test_unsupported_accept_is_refused_with_406(self): - response = _client(UserFactory(is_staff=True)).get(url(self.course.id), HTTP_ACCEPT="application/xml") + response = api_client(UserFactory(is_staff=True)).get(url(self.course.id), HTTP_ACCEPT="application/xml") assert_error_envelope(response, expected_status=406, expected_type_slug="internal") # Nor ParseError or UnsupportedMediaType: a malformed or unsupported body is @@ -1447,14 +1251,14 @@ def test_unsupported_accept_is_refused_with_406(self): # here once the library catalogues them. def test_a_malformed_json_body_is_a_400_typed_internal(self): - response = _client(UserFactory(is_staff=True)).patch( + response = api_client(UserFactory(is_staff=True)).patch( url(self.course.id), data="{", content_type="application/json", ) envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="internal") assert envelope["title"] == "Internal Server Error" def test_an_unsupported_media_type_is_a_415_typed_internal(self): - response = _client(UserFactory(is_staff=True)).patch( + response = api_client(UserFactory(is_staff=True)).patch( url(self.course.id), data="display_name=Renamed", content_type="text/plain", ) envelope = assert_error_envelope(response, expected_status=415, expected_type_slug="internal") @@ -1467,14 +1271,14 @@ def test_an_unsupported_media_type_is_a_415_typed_internal(self): @ddt.data("not-a-course-key", "edX/DemoX/Demo_Course", "edX+DemoX+Demo_Course") def test_an_unparsable_or_slash_form_key_is_an_html_404(self, key): - response = _client(UserFactory(is_staff=True)).get(f"/api/authoring/v1/courses/{key}/advanced_settings/") + response = api_client(UserFactory(is_staff=True)).get(f"/api/authoring/v1/courses/{key}/advanced_settings/") assert response.status_code == 404 assert response["Content-Type"].startswith("text/html") def test_the_address_without_its_trailing_slash_redirects_to_it(self): """No route answers the address without its slash; the service redirects a read to the slashed one.""" address = f"/api/authoring/v1/courses/{self.course.id}/advanced_settings" - response = _client(UserFactory(is_staff=True)).get(address) + response = api_client(UserFactory(is_staff=True)).get(address) assert response.status_code == 301 assert response["Location"] == url(self.course.id) with pytest.raises(Resolver404): @@ -1494,7 +1298,7 @@ class AdvancedSettingsSchemaTest(TestCase): @classmethod def setUpClass(cls): super().setUpClass() - cls.schema = _published_schema() + cls.schema = published_schema() cls.components = cls.schema["components"]["schemas"] @staticmethod From 3838348ca73af87e1bf8e94e47d5ac0603c0b8a1 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Sun, 25 Oct 2026 22:05:35 +0500 Subject: [PATCH 7/9] feat: add course content editing permission classes The v0 course optimizer views check courses.edit_course_content inline, falling back to legacy read access to start a link check and to legacy write access for everything else. The conforming views need these as permission classes: CanStartLinkCheck makes the read-fallback call and CanEditCourseContent the write-fallback call, so the two stay separate when the endpoints are merged. Existing classes are unchanged. --- .../contentstore/views/permissions.py | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/permissions.py b/cms/djangoapps/contentstore/views/permissions.py index 3701d7149481..3fa362fce959 100644 --- a/cms/djangoapps/contentstore/views/permissions.py +++ b/cms/djangoapps/contentstore/views/permissions.py @@ -2,7 +2,7 @@ Custom permissions for the content store views. """ -from openedx_authz.constants.permissions import COURSES_VIEW_PAGES_AND_RESOURCES +from openedx_authz.constants.permissions import COURSES_EDIT_COURSE_CONTENT, COURSES_VIEW_PAGES_AND_RESOURCES from rest_framework.exceptions import NotFound from rest_framework.permissions import BasePermission @@ -123,3 +123,47 @@ def has_permission(self, request, view): if course_key is None: return False return check_course_advanced_settings_access(request.user, course_key, access_type="write") + + +class CanStartLinkCheck(BasePermission): + """ + Allow callers who may start a check of the links in the course in the path. + + Reads the parsed ``course_key`` URL argument. On courses where + policy-based authoring permissions are enabled, only the permission to + edit course content is consulted; elsewhere Studio read access to the + course is enough, while reading a check's result needs write access + (``CanEditCourseContent``). + """ + + def has_permission(self, request, view): + course_key = view.kwargs.get("course_key") + if course_key is None: + return False + return user_has_course_permission( + request.user, + COURSES_EDIT_COURSE_CONTENT.identifier, + course_key, + LegacyAuthoringPermission.READ, + ) + + +class CanEditCourseContent(BasePermission): + """ + Allow callers who may edit the content of the course in the path. + + Reads the parsed ``course_key`` URL argument. On courses where + policy-based authoring permissions are enabled, only that policy is + consulted; elsewhere the caller needs Studio write access to the course. + """ + + def has_permission(self, request, view): + course_key = view.kwargs.get("course_key") + if course_key is None: + return False + return user_has_course_permission( + request.user, + COURSES_EDIT_COURSE_CONTENT.identifier, + course_key, + LegacyAuthoringPermission.WRITE, + ) From 9779fee9f3b675e1f25dd9233048525c09083ee7 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Sun, 25 Oct 2026 22:05:35 +0500 Subject: [PATCH 8/9] feat: serve the course optimizer from authoring v1 The v0 course optimizer spreads two resources over four verb-style addresses (link_check, link_check_status, rerun_link_update, rerun_link_update_status), answers starts with 200, returns PascalCase and camelCase reports, passes recorded failure text to clients, answers a disabled feature with 400 and accepts Bearer tokens. /api/authoring/v1/courses/{course_key}/link_check/ and .../previous_run_link_update/ each take POST to start a run (202 with its status) and GET to read the latest run the caller may see. The default read is the status; view=full adds the report or the results. Keys are snake_case, failures carry fixed messages, errors use the standard envelope, a disabled feature answers 404, and links sent with action=all are not read, as on v0. Starting a link check keeps v0's read-level check and every other operation its write-level check, and per-user run visibility is reused unchanged. Parity tests compare each read and each queued task with v0. --- .../rest_api/v1/authoring_urls.py | 14 + .../rest_api/v1/course_optimizer_service.py | 139 ++ .../v1/serializers/course_optimizer.py | 279 +++ .../rest_api/v1/views/course_optimizer.py | 332 +++ .../v1/views/tests/test_course_optimizer.py | 1907 +++++++++++++++++ 5 files changed, 2671 insertions(+) create mode 100644 cms/djangoapps/contentstore/rest_api/v1/course_optimizer_service.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/serializers/course_optimizer.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/course_optimizer.py create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_optimizer.py diff --git a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py index afea706611ba..6ad8f3b36cf1 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -8,6 +8,10 @@ YoutubeTranscriptImportsViewSet, ) from cms.djangoapps.contentstore.rest_api.v1.views.advanced_settings import CourseAdvancedSettingsViewSet +from cms.djangoapps.contentstore.rest_api.v1.views.course_optimizer import ( + CourseLinkCheckViewSet, + CoursePreviousRunLinkUpdateViewSet, +) app_name = "authoring_v1" @@ -45,4 +49,14 @@ CourseAdvancedSettingsViewSet.as_view({"get": "retrieve", "patch": "partial_update"}), name="course_advanced_settings", ), + path( + "courses//link_check/", + CourseLinkCheckViewSet.as_view({"get": "retrieve", "post": "create"}), + name="course_link_check", + ), + path( + "courses//previous_run_link_update/", + CoursePreviousRunLinkUpdateViewSet.as_view({"get": "retrieve", "post": "create"}), + name="course_previous_run_link_update", + ), ] diff --git a/cms/djangoapps/contentstore/rest_api/v1/course_optimizer_service.py b/cms/djangoapps/contentstore/rest_api/v1/course_optimizer_service.py new file mode 100644 index 000000000000..671201d9bfe6 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/course_optimizer_service.py @@ -0,0 +1,139 @@ +"""Runs of the course optimizer: link checks and updates of links to a previous course run.""" + +import json + +from rest_framework.exceptions import NotFound +from user_tasks.models import UserTaskArtifact, UserTaskStatus + +from cms.djangoapps.contentstore.core.course_optimizer_provider import ( + _latest_course_link_update_task_status, + _latest_task_status, + generate_broken_links_descriptor, + get_course_link_update_data, + sort_course_sections, +) +from cms.djangoapps.contentstore.tasks import check_broken_links, update_course_rerun_links +from cms.djangoapps.contentstore.toggles import enable_course_optimizer_check_prev_run_links + +# The state of a resource no run of which the caller may see. +UNINITIATED = "Uninitiated" +# The stages the link update task reports while it runs. +SCANNING = "Scanning" +UPDATING = "Updating" + +# Every state a link check or a link update can report. Scanning and Updating +# are reported by link updates only. +RUN_STATES = [ + UNINITIATED, + UserTaskStatus.PENDING, + UserTaskStatus.IN_PROGRESS, + SCANNING, + UPDATING, + UserTaskStatus.SUCCEEDED, + UserTaskStatus.FAILED, + UserTaskStatus.CANCELED, + UserTaskStatus.RETRYING, +] + +ALL_LINKS = "all" +SINGLE_LINKS = "single" + +# Reported for a link check that failed or was canceled with an error recorded. +# The recorded text can be an exception's, so it is not returned. +LINK_CHECK_ERROR_MESSAGE = "The link check could not be completed." +# Reported for each link an update run could not change, in place of the +# recorded reason, which can be an exception's text. +LINK_NOT_UPDATED_MESSAGE = "The link could not be updated." +PREVIOUS_RUN_LINK_UPDATES_DISABLED_MESSAGE = "Updating links to a previous course run is not enabled for this course." + +_FAILED_STATES = (UserTaskStatus.FAILED, UserTaskStatus.CANCELED) + + +def start_link_check(course_key, user, language): + """Queue a check of the links in the course, as ``user``; return the state of the queued run.""" + check_broken_links.delay(user.id, str(course_key), language) + return {"status": UserTaskStatus.PENDING} + + +def link_check_status(request, course_key, full=False): + """ + Return the latest link check of the course that the requester may see. + + Callers see their own runs; superusers see everyone's. With ``full``, the + report of a run that succeeded is added under ``output`` (``None`` + otherwise). + """ + run = _latest_task_status(request, str(course_key)) + if run is None: + status = {"status": UNINITIATED, "created_at": None, "error": None} + else: + failed = run.state in _FAILED_STATES and ( + UserTaskArtifact.objects.filter(status=run, name="Error").exclude(text="").exists() + ) + status = { + "status": run.state, + "created_at": run.created, + "error": LINK_CHECK_ERROR_MESSAGE if failed else None, + } + if full: + succeeded = run is not None and run.state == UserTaskStatus.SUCCEEDED + status["output"] = _link_check_report(request.user, course_key, run) if succeeded else None + return status + + +def _link_check_report(user, course_key, run): + """The flagged links a run found, sections in course order.""" + artifact = UserTaskArtifact.objects.get(status=run, name="BrokenLinks") + with artifact.file as file: + links = json.loads(file.read()) + report = generate_broken_links_descriptor(links, user, course_key) + return sort_course_sections(course_key, {"LinkCheckOutput": report})["LinkCheckOutput"] + + +def ensure_previous_run_link_updates_enabled(course_key): + """Raise ``NotFound`` unless links to a previous run may be updated in the course.""" + if not enable_course_optimizer_check_prev_run_links(course_key): + raise NotFound(PREVIOUS_RUN_LINK_UPDATES_DISABLED_MESSAGE) + + +def start_previous_run_link_update(course_key, user, action, links, language): + """ + Queue an update of the course's links to its previous run, as ``user``. + + ``action`` is ``all`` (every such link the course has) or ``single`` (the + ``links`` given). Returns the state of the queued run. Callers check + ``ensure_previous_run_link_updates_enabled`` first. + """ + update_course_rerun_links.delay(user.id, str(course_key), action, [dict(link) for link in links], language) + return {"status": UserTaskStatus.PENDING} + + +def previous_run_link_update_status(request, course_key, full=False): + """ + Return the latest update of links to the previous run that the requester may see. + + Visibility is as for link checks. With ``full``, the outcome of each link + of a run that succeeded is added under ``results`` (empty otherwise). + Callers check ``ensure_previous_run_link_updates_enabled`` first. + """ + if not full: + run = _latest_course_link_update_task_status(request, str(course_key)) + return {"status": UNINITIATED if run is None else run.state} + data = get_course_link_update_data(request, str(course_key)) + return { + "status": UNINITIATED if data["status"] == "uninitiated" else data["status"], + "results": [_link_update_result(result) for result in data["results"]], + } + + +def _link_update_result(result): + """One link's outcome, with a fixed message in place of the recorded failure reason.""" + success = bool(result["success"]) + return { + "id": result["id"], + "type": result["type"], + "original_url": result.get("original_url"), + "new_url": result["new_url"], + "success": success, + "error_message": None if success else LINK_NOT_UPDATED_MESSAGE, + } diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/course_optimizer.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/course_optimizer.py new file mode 100644 index 000000000000..6190a77ad8d2 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/course_optimizer.py @@ -0,0 +1,279 @@ +"""Serializers for the course optimizer's link checks and previous-run link updates.""" + +from collections.abc import Mapping + +from rest_framework import serializers + +from cms.djangoapps.contentstore.rest_api.v1.course_optimizer_service import ALL_LINKS, RUN_STATES, SINGLE_LINKS + +MINIMAL_VIEW = "minimal" +FULL_VIEW = "full" + + +class QueuedRunSerializer(serializers.Serializer): # pylint: disable=abstract-method + """The state of a run that has just been queued.""" + + status = serializers.ChoiceField( + choices=RUN_STATES, + help_text="State of the run: always Pending, as the run waits to be picked up.", + ) + + class Meta: + ref_name = "CourseOptimizerQueuedRun.v1" + + +class PreviousRunLinkSerializer(serializers.Serializer): # pylint: disable=abstract-method + """A link to the previous run of the course, and whether it now points at this run.""" + + original_link = serializers.CharField(source="originalLink", help_text="The link as it points at the previous run.") + is_updated = serializers.BooleanField( + source="isUpdated", help_text="Whether the link has been changed to point at this run.", + ) + updated_link = serializers.CharField( + source="updatedLink", + allow_null=True, + help_text="The link as it points at this run, or null when it has not been changed.", + ) + + class Meta: + ref_name = "LinkCheckPreviousRunLink.v1" + + +class LinkCheckReportBlockSerializer(serializers.Serializer): # pylint: disable=abstract-method + """A component, course update, handouts page or custom page with the links found in it, by kind.""" + + id = serializers.CharField(help_text="Usage key of the component or page, or the id of the course update.") + display_name = serializers.CharField( + source="displayName", allow_blank=True, help_text="Name of the component or page as Studio shows it.", + ) + url = serializers.CharField(help_text="Path of the Studio page where the content is edited.") + broken_links = serializers.ListField( + source="brokenLinks", child=serializers.CharField(), help_text="Links that could not be reached.", + ) + locked_links = serializers.ListField( + source="lockedLinks", + child=serializers.CharField(), + help_text="Links to Studio content that refused access, such as locked files.", + ) + external_forbidden_links = serializers.ListField( + source="externalForbiddenLinks", + child=serializers.CharField(), + help_text="Links to other sites that refused access.", + ) + previous_run_links = PreviousRunLinkSerializer( + source="previousRunLinks", many=True, help_text="Links that point at the previous run of the course.", + ) + + class Meta: + ref_name = "LinkCheckReportBlock.v1" + + +class LinkCheckReportUnitSerializer(serializers.Serializer): # pylint: disable=abstract-method + """A unit holding components with flagged links.""" + + id = serializers.CharField(help_text="Usage key of the unit.") + display_name = serializers.CharField( + source="displayName", allow_blank=True, help_text="Name of the unit as Studio shows it.", + ) + blocks = LinkCheckReportBlockSerializer(many=True, help_text="Components of the unit with flagged links.") + + class Meta: + ref_name = "LinkCheckReportUnit.v1" + + +class LinkCheckReportSubsectionSerializer(serializers.Serializer): # pylint: disable=abstract-method + """A subsection holding units with flagged links.""" + + id = serializers.CharField(help_text="Usage key of the subsection.") + display_name = serializers.CharField( + source="displayName", allow_blank=True, help_text="Name of the subsection as Studio shows it.", + ) + units = LinkCheckReportUnitSerializer(many=True, help_text="Units of the subsection with flagged links.") + + class Meta: + ref_name = "LinkCheckReportSubsection.v1" + + +class LinkCheckReportSectionSerializer(serializers.Serializer): # pylint: disable=abstract-method + """A section holding subsections with flagged links.""" + + id = serializers.CharField(help_text="Usage key of the section.") + display_name = serializers.CharField( + source="displayName", allow_blank=True, help_text="Name of the section as Studio shows it.", + ) + subsections = LinkCheckReportSubsectionSerializer( + many=True, help_text="Subsections of the section with flagged links.", + ) + + class Meta: + ref_name = "LinkCheckReportSection.v1" + + +class LinkCheckReportSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + The links a check flagged, where they were found. + + Course content is reported as sections, subsections, units and components, + four levels and no more, in course order; only the parts holding flagged + links appear. Course updates (with the handouts page last) and custom + pages are listed whether or not they hold any. + """ + + sections = LinkCheckReportSectionSerializer(many=True, help_text="Sections with flagged links, in course order.") + course_updates = LinkCheckReportBlockSerializer( + many=True, help_text="Course updates and the handouts page, with the links found in each.", + ) + custom_pages = LinkCheckReportBlockSerializer( + many=True, help_text="Custom pages of the course, with the links found in each.", + ) + + class Meta: + ref_name = "LinkCheckReport.v1" + + +class LinkCheckSerializer(serializers.Serializer): # pylint: disable=abstract-method + """The latest link check of a course that the requester may see.""" + + status = serializers.ChoiceField( + choices=RUN_STATES, + help_text=( + "State of the run. Uninitiated when the requester may see no run of this course; " + "Scanning and Updating are not reported for link checks." + ), + ) + created_at = serializers.DateTimeField( + allow_null=True, help_text="When the run was queued, or null when there is no run.", + ) + error = serializers.CharField( + allow_null=True, + help_text="A fixed message when the run failed or was canceled with an error recorded; otherwise null.", + ) + output = LinkCheckReportSerializer( + required=False, + allow_null=True, + help_text="Only with view=full: the links the run flagged, or null unless the run succeeded.", + ) + + class Meta: + ref_name = "LinkCheck.v1" + + +class LinkToUpdateSerializer(serializers.Serializer): # pylint: disable=abstract-method + """A link to change so that it points at this run instead of the previous one.""" + + url = serializers.CharField(help_text="The link as it appears in the content.") + type = serializers.CharField( + help_text=( + "Where the link was found, which decides how it is changed: 'course_content' for a component " + "of the course outline, 'course_updates' for a course update (or the handouts page, when id names " + "it), 'handouts' for the handouts page, and 'custom_pages' for a custom page. Other values are " + "treated as 'course_content'." + ), + ) + id = serializers.CharField( + help_text="Usage key of the component or page holding the link, or the course update id.", + ) + + class Meta: + ref_name = "LinkToUpdate.v1" + + +class PreviousRunLinkUpdateRequestSerializer(serializers.Serializer): # pylint: disable=abstract-method + """Which links to the previous run of the course to change.""" + + action = serializers.ChoiceField( + choices=[ALL_LINKS, SINGLE_LINKS], + help_text="'all' changes every link to the previous run the course holds; 'single' changes the links in data.", + ) + data = LinkToUpdateSerializer( + many=True, + required=False, + help_text=( + "The links to change, as a link check reports them. Required, and not empty, when action is " + "'single'. Not read when action is 'all', so it is neither validated nor used then." + ), + ) + + class Meta: + ref_name = "PreviousRunLinkUpdateRequest.v1" + + def to_internal_value(self, data): + """Drop ``data`` before validation when every link is to be changed, as it is not read then.""" + if isinstance(data, Mapping) and data.get("action") == ALL_LINKS: + data = {key: value for key, value in data.items() if key != "data"} + return super().to_internal_value(data) + + def validate(self, attrs): + if attrs["action"] == SINGLE_LINKS and not attrs.get("data"): + raise serializers.ValidationError({"data": ["This field is required when action is 'single'."]}) + return attrs + + +class LinkUpdateResultSerializer(serializers.Serializer): # pylint: disable=abstract-method + """The outcome of changing one link.""" + + id = serializers.CharField( + allow_blank=True, help_text="Usage key of the component or page holding the link, or the course update id.", + ) + type = serializers.CharField(allow_blank=True, help_text="Where the link was found.") + original_url = serializers.CharField( + allow_blank=True, allow_null=True, help_text="The link before the change, or null when it was not recorded.", + ) + new_url = serializers.CharField( + allow_blank=True, help_text="The link after the change; the original link when it could not be changed.", + ) + success = serializers.BooleanField(help_text="Whether the link was changed.") + error_message = serializers.CharField( + allow_null=True, help_text="A fixed message when the link could not be changed; otherwise null.", + ) + + class Meta: + ref_name = "LinkUpdateResult.v1" + + +class PreviousRunLinkUpdateSerializer(serializers.Serializer): # pylint: disable=abstract-method + """The latest update of links to the previous run that the requester may see.""" + + status = serializers.ChoiceField( + choices=RUN_STATES, + help_text=( + "State of the run; Scanning and Updating are the stages of a run in progress. " + "Uninitiated when the requester may see no run of this course." + ), + ) + results = LinkUpdateResultSerializer( + many=True, + required=False, + help_text="Only with view=full: the outcome of each link of a run that succeeded; empty otherwise.", + ) + + class Meta: + ref_name = "PreviousRunLinkUpdate.v1" + + +def _view_field(full_view_adds): + return serializers.ChoiceField( + choices=[MINIMAL_VIEW, FULL_VIEW], + default=MINIMAL_VIEW, + help_text=( + f"Which representation to return. 'minimal' returns the state of the run; 'full' adds {full_view_adds}." + ), + ) + + +class LinkCheckQuerySerializer(serializers.Serializer): # pylint: disable=abstract-method + """Query parameters of a read of a course's latest link check.""" + + view = _view_field("the links the run flagged, which can be many") + + class Meta: + ref_name = "LinkCheckQuery.v1" + + +class PreviousRunLinkUpdateQuerySerializer(serializers.Serializer): # pylint: disable=abstract-method + """Query parameters of a read of a course's latest update of links to its previous run.""" + + view = _view_field("the outcome of each link the run changed") + + class Meta: + ref_name = "PreviousRunLinkUpdateQuery.v1" diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/course_optimizer.py b/cms/djangoapps/contentstore/rest_api/v1/views/course_optimizer.py new file mode 100644 index 000000000000..3b82f6636c77 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/course_optimizer.py @@ -0,0 +1,332 @@ +"""API views for the course optimizer's link checks and previous-run link updates.""" + +from django.core.exceptions import ImproperlyConfigured +from drf_spectacular.utils import OpenApiExample, OpenApiParameter, OpenApiResponse, extend_schema +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from edx_rest_framework_extensions.errors import ErrorResponseSerializer +from edx_rest_framework_extensions.mixins import StandardizedErrorMixin +from edx_rest_framework_extensions.shaping import project +from edx_rest_framework_extensions.url_converters import CourseKeyConverter +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.course_optimizer_service import ( + LINK_CHECK_ERROR_MESSAGE, + LINK_NOT_UPDATED_MESSAGE, + ensure_previous_run_link_updates_enabled, + link_check_status, + previous_run_link_update_status, + start_link_check, + start_previous_run_link_update, +) +from cms.djangoapps.contentstore.rest_api.v1.serializers.course_optimizer import ( + FULL_VIEW, + MINIMAL_VIEW, + LinkCheckQuerySerializer, + LinkCheckSerializer, + PreviousRunLinkUpdateQuerySerializer, + PreviousRunLinkUpdateRequestSerializer, + PreviousRunLinkUpdateSerializer, + QueuedRunSerializer, +) +from cms.djangoapps.contentstore.views.permissions import CanEditCourseContent, CanStartLinkCheck, IsExistingCourse + +_COURSE_KEY_PARAMETER = OpenApiParameter( + name="course_key", + description="Key of the course, such as course-v1:edX+DemoX+2026.", + location=OpenApiParameter.PATH, + type=str, + pattern=f"^{CourseKeyConverter.regex}$", +) + + +def _view_parameter(query_serializer_class): + return OpenApiParameter( + name="view", + description=query_serializer_class().fields["view"].help_text, + required=False, + location=OpenApiParameter.QUERY, + type=str, + enum=[MINIMAL_VIEW, FULL_VIEW], + default=MINIMAL_VIEW, + ) + + +def _error(description): + return OpenApiResponse(response=ErrorResponseSerializer, description=description) + + +_BLOCK_KEY = "block-v1:edX+DemoX+2026+type@{}+block@{}" +_REPORT_EXAMPLE = OpenApiExample( + "Full report", + summary="A link check that succeeded, read with view=full", + response_only=True, + status_codes=["200"], + value={ + "status": "Succeeded", + "created_at": "2026-10-25T14:32:01.294587Z", + "error": None, + "output": { + "sections": [{ + "id": _BLOCK_KEY.format("chapter", "week1"), + "display_name": "Week 1", + "subsections": [{ + "id": _BLOCK_KEY.format("sequential", "lesson"), + "display_name": "Lesson", + "units": [{ + "id": _BLOCK_KEY.format("vertical", "unit"), + "display_name": "Unit", + "blocks": [{ + "id": _BLOCK_KEY.format("html", "reading"), + "display_name": "Reading", + "url": "/course/course-v1:edX+DemoX+2026/editor/html/" + + _BLOCK_KEY.format("html", "reading"), + "broken_links": ["https://example.com/missing"], + "locked_links": ["/static/handout.pdf"], + "external_forbidden_links": [], + "previous_run_links": [{ + "original_link": "https://studio.example.com/course/course-v1:edX+DemoX+2025/info", + "is_updated": False, + "updated_link": None, + }], + }], + }], + }], + }], + "course_updates": [], + "custom_pages": [], + }, + }, +) + +_QUEUED = OpenApiResponse(response=QueuedRunSerializer, description="The run was queued.") +_UNAUTHENTICATED = _error("The requester is not authenticated.") +_COURSE_NOT_FOUND = _error("The course does not exist.") + + +class _CourseOptimizerViewSet(StandardizedErrorMixin, viewsets.ViewSet): + """ + Checks the course, then the permission the requested action needs. + + Subclasses restate the checks every request passes in + ``permission_classes``, map each action to its own in + ``action_permission_classes``, and name the members of the minimal view + in ``minimal_fields``. An action with no permission declared is refused + with a server error instead of being served unchecked. + """ + + # Kept from the legacy endpoints: an inactive account's browser session stays authenticated, + # and the course permission decides whether it may call. + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated, IsExistingCourse) + action_permission_classes = {} + # A method with no route (answered 405) and OPTIONS read nothing of the + # course's runs, so only the course is checked for them. + unrouted_actions = (None, "metadata") + + def get_permissions(self): + """Check the course, then the permission the requested action needs.""" + if self.action in self.unrouted_actions: + return super().get_permissions() + if self.action not in self.action_permission_classes: + raise ImproperlyConfigured(f"No permission is declared for the {self.action!r} action.") + specific = self.action_permission_classes[self.action] + return [*super().get_permissions(), *(permission() for permission in specific)] + + def get_serializer(self, *args, **kwargs): + """Instantiate the response serializer declared on this viewset.""" + return self.serializer_class(*args, **kwargs) + + @staticmethod + def _full_view_requested(request, query_serializer_class): + """Validate the query parameters and return whether ``view=full`` was requested.""" + query = query_serializer_class(data=request.query_params) + query.is_valid(raise_exception=True) + return query.validated_data["view"] == FULL_VIEW + + def _shaped(self, run, full): + """Serialize ``run``, keeping only ``minimal_fields`` unless the full view was requested.""" + data = self.get_serializer(run).data + return data if full else project(data, self.minimal_fields) + + +@extend_schema(tags=["openedx-platform-sdk"]) +class CourseLinkCheckViewSet(_CourseOptimizerViewSet): + """ + The latest check of the links in a course. + + Starting a check queues a run that follows every link in the course's + content, updates, handouts and custom pages, and records those that are + broken, refuse access, or point at the course's previous run. A read + returns the latest run the requester may see: their own, or anyone's for + a superuser. By default (``view=minimal``) only the state of the run is + returned; ``view=full`` adds the links it flagged. + + Both need the permission to edit course content. Where policy-based + authoring permissions are off, starting a check needs only Studio read + access to the course while reading one needs write access; the two checks + are kept separate on purpose. The course is looked up first, so a signed-in caller learns that a course does + not exist (404) before being refused (403). + + A plain ``ViewSet``: runs are queued as tasks and read from their recorded + status, not addressed as rows. + """ + + permission_classes = (IsAuthenticated, IsExistingCourse) + action_permission_classes = { + "create": (CanStartLinkCheck,), + "retrieve": (CanEditCourseContent,), + } + serializer_class = LinkCheckSerializer + minimal_fields = ("status", "created_at", "error") + + @extend_schema( + summary="Start a check of a course's links", + description=( + "Queues a check of every link in the course and returns the state of the queued run. " + "Read the result from this address." + ), + parameters=[_COURSE_KEY_PARAMETER], + request=None, + responses={ + 202: _QUEUED, + 401: _UNAUTHENTICATED, + 403: _error("The requester may not check this course's links."), + 404: _COURSE_NOT_FOUND, + }, + ) + def create(self, request, course_key): + """Queue a link check of the course as the requester.""" + queued = start_link_check(course_key, request.user, request.LANGUAGE_CODE) + return Response(QueuedRunSerializer(queued).data, status=status.HTTP_202_ACCEPTED) + + @extend_schema( + summary="Get the latest check of a course's links", + description=( + "Returns the state of the latest link check of the course that the requester may see: their own " + "runs, or anyone's for a superuser. The state is Uninitiated when there is none. The error member " + f"of a run that failed is the fixed message '{LINK_CHECK_ERROR_MESSAGE}'. view=full adds the links " + "the run flagged, for a run that succeeded." + ), + parameters=[_COURSE_KEY_PARAMETER, _view_parameter(LinkCheckQuerySerializer)], + responses={ + 200: OpenApiResponse(response=LinkCheckSerializer, description="The latest link check."), + 400: _error("A query parameter has an invalid value."), + 401: _UNAUTHENTICATED, + 403: _error("The requester may not read this course's link checks."), + 404: _COURSE_NOT_FOUND, + }, + examples=[_REPORT_EXAMPLE], + ) + def retrieve(self, request, course_key): + """Return the latest link check the requester may see, in the requested view.""" + full = self._full_view_requested(request, LinkCheckQuerySerializer) + return Response(self._shaped(link_check_status(request, course_key, full=full), full)) + + +_UPDATE_ALL_EXAMPLE = OpenApiExample( + "Every link", + summary="Change every link to the previous run", + request_only=True, + value={"action": "all"}, +) +_UPDATE_SINGLE_EXAMPLE = OpenApiExample( + "Some links", + summary="Change the links a link check reported", + request_only=True, + value={ + "action": "single", + "data": [ + { + "url": "https://studio.example.com/course/course-v1:edX+DemoX+2025/course", + "type": "course_updates", + "id": "1", + }, + ], + }, +) + + +@extend_schema(tags=["openedx-platform-sdk"]) +class CoursePreviousRunLinkUpdateViewSet(_CourseOptimizerViewSet): + """ + The latest update of a course's links that point at its previous run. + + Starting an update queues a run that changes such links to point at this + run: every one the course holds (``action`` ``all``) or those listed + (``single``). A read returns the latest run the requester may see, as for + link checks. By default (``view=minimal``) only the state of the run is + returned; ``view=full`` adds the outcome of each link. + + Both need the permission to edit course content (Studio write access + where policy-based authoring permissions are off). Where updating links + to a previous run is not enabled for the course, both answer 404 once the + caller's permission is established. + + A plain ``ViewSet``, as for link checks. + """ + + permission_classes = (IsAuthenticated, IsExistingCourse) + action_permission_classes = { + "create": (CanEditCourseContent,), + "retrieve": (CanEditCourseContent,), + } + serializer_class = PreviousRunLinkUpdateSerializer + minimal_fields = ("status",) + + @extend_schema( + summary="Start an update of a course's links to its previous run", + description=( + "Queues a run that changes links to the course's previous run so that they point at this run. " + "With action 'all' every such link is changed; with 'single', the links in data. Returns the " + "state of the queued run; read the result from this address." + ), + parameters=[_COURSE_KEY_PARAMETER], + request={"application/json": PreviousRunLinkUpdateRequestSerializer}, + responses={ + 202: _QUEUED, + 400: _error("The body names no valid action, or names 'single' without links."), + 401: _UNAUTHENTICATED, + 403: _error("The requester may not change this course's content."), + 404: _error("The course does not exist, or updating links to its previous run is not enabled."), + }, + examples=[_UPDATE_ALL_EXAMPLE, _UPDATE_SINGLE_EXAMPLE], + ) + def create(self, request, course_key): + """Queue an update of the course's links to its previous run as the requester.""" + ensure_previous_run_link_updates_enabled(course_key) + body = PreviousRunLinkUpdateRequestSerializer(data=request.data) + body.is_valid(raise_exception=True) + queued = start_previous_run_link_update( + course_key, + request.user, + body.validated_data["action"], + body.validated_data.get("data", []), + request.LANGUAGE_CODE, + ) + return Response(QueuedRunSerializer(queued).data, status=status.HTTP_202_ACCEPTED) + + @extend_schema( + summary="Get the latest update of a course's links to its previous run", + description=( + "Returns the state of the latest such update of the course that the requester may see: their own " + "runs, or anyone's for a superuser. The state is Uninitiated when there is none. view=full adds " + "the outcome of each link of a run that succeeded; a link that could not be changed carries the " + f"fixed message '{LINK_NOT_UPDATED_MESSAGE}'." + ), + parameters=[_COURSE_KEY_PARAMETER, _view_parameter(PreviousRunLinkUpdateQuerySerializer)], + responses={ + 200: OpenApiResponse(response=PreviousRunLinkUpdateSerializer, description="The latest update."), + 400: _error("A query parameter has an invalid value."), + 401: _UNAUTHENTICATED, + 403: _error("The requester may not read this course's link updates."), + 404: _error("The course does not exist, or updating links to its previous run is not enabled."), + }, + ) + def retrieve(self, request, course_key): + """Return the latest update the requester may see, in the requested view.""" + ensure_previous_run_link_updates_enabled(course_key) + full = self._full_view_requested(request, PreviousRunLinkUpdateQuerySerializer) + return Response(self._shaped(previous_run_link_update_status(request, course_key, full=full), full)) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_optimizer.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_optimizer.py new file mode 100644 index 000000000000..6af0ffd328ad --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_optimizer.py @@ -0,0 +1,1907 @@ +"""Tests for the v1 course optimizer API: link checks and updates of links to a previous run.""" + +import json +import re +import uuid +from contextlib import contextmanager +from datetime import UTC, datetime, timedelta +from types import SimpleNamespace +from unittest.mock import patch + +import ddt +import pytest +from django.core.exceptions import ImproperlyConfigured +from django.core.files.base import ContentFile +from django.test import SimpleTestCase, TestCase +from django.urls import Resolver404, resolve, reverse +from edx_rest_framework_extensions.testing import assert_error_envelope +from edx_toggles.toggles.testutils import override_waffle_flag +from opaque_keys.edx.keys import CourseKey, UsageKey +from openedx_authz.constants.roles import ( + COURSE_ADMIN, + COURSE_AUDITOR, + COURSE_BETA_TESTER, + COURSE_DATA_RESEARCHER, + COURSE_EDITOR, + COURSE_LIMITED_STAFF, + COURSE_STAFF, +) +from openedx_authz.models.engine import PolicyCacheControl +from rest_framework.permissions import IsAuthenticated +from rest_framework.test import APIClient, APIRequestFactory, force_authenticate +from user_tasks.models import UserTaskArtifact, UserTaskStatus + +from cms.djangoapps.contentstore.rest_api.v1.course_optimizer_service import ( + LINK_CHECK_ERROR_MESSAGE, + LINK_NOT_UPDATED_MESSAGE, + PREVIOUS_RUN_LINK_UPDATES_DISABLED_MESSAGE, + RUN_STATES, +) +from cms.djangoapps.contentstore.rest_api.v1.views.course_optimizer import ( + CourseLinkCheckViewSet, + CoursePreviousRunLinkUpdateViewSet, +) +from cms.djangoapps.contentstore.rest_api.v1.views.tests.helpers import ( + FIRST_LOOKUP_SIGNALS, + FIRST_LOOKUP_WRITES, + UNKNOWN_COURSE_KEY, + VERSION_ONLY_KEY, + api_client, + assert_matches_published_schema, + assert_refused, + changed_paths, + measure, + published_schema, + record_effects, + session_client, + undeclared_paths, +) +from cms.djangoapps.contentstore.rest_api.v2.views.tests.helpers import resolve_ref +from cms.djangoapps.contentstore.tasks import CourseLinkCheckTask, CourseLinkUpdateTask, LinkState +from cms.djangoapps.contentstore.tests.utils import routed_actions +from cms.djangoapps.contentstore.toggles import ENABLE_COURSE_OPTIMIZER_CHECK_PREV_RUN_LINKS +from cms.djangoapps.contentstore.views.permissions import CanEditCourseContent, CanStartLinkCheck, IsExistingCourse +from cms.lib.spectacular import LEGACY_MIGRATED_PATH_PREFIXES, SUPERSEDED_PATHS +from common.djangoapps.student.models import CourseAccessRole +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseLimitedStaffRole, + CourseStaffRole, + OrgInstructorRole, + OrgStaffRole, +) +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.authz.constants import LEGACY_PERMISSION_HANDLER_MAP, LegacyAuthoringPermission +from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.oauth_dispatch.jwt import create_jwt_for_user +from openedx.core.djangoapps.oauth_dispatch.tests.factories import AccessTokenFactory, ApplicationFactory +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.mixed import MixedModuleStore +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, LibraryFactory + +LINK_CHECK = "authoring_v1:course_link_check" +LINK_UPDATE = "authoring_v1:course_previous_run_link_update" +LEGACY_NAMESPACE = "cms.djangoapps.contentstore:v0" +LEGACY_VIEWS = "cms.djangoapps.contentstore.rest_api.v0.views.course_optimizer" +SERVICE = "cms.djangoapps.contentstore.rest_api.v1.course_optimizer_service" +FULL = {"view": "full"} + + +def previous_run_links(active): + """Turn updating links to a previous run on or off for every course, as a decorator or a context.""" + return override_waffle_flag(ENABLE_COURSE_OPTIMIZER_CHECK_PREV_RUN_LINKS, active=active) + + +# Each operation as (HTTP method, v1 URL name, v0 URL name, default body, v1 success status). +OPERATIONS = { + "start_check": ("post", LINK_CHECK, "link_check", None, 202), + "read_check": ("get", LINK_CHECK, "link_check_status", None, 200), + "start_update": ("post", LINK_UPDATE, "rerun_link_update", {"action": "all"}, 202), + "read_update": ("get", LINK_UPDATE, "rerun_link_update_status", None, 200), +} +STARTS = ("start_check", "start_update") +TASK_OF = {"start_check": "check_broken_links", "start_update": "update_course_rerun_links"} +# What each operation answers a caller who has no run of the course. +EMPTY_BODIES = { + "start_check": {"status": "Pending"}, + "read_check": {"status": "Uninitiated", "created_at": None, "error": None}, + "start_update": {"status": "Pending"}, + "read_update": {"status": "Uninitiated"}, +} +LEGACY_EMPTY_BODIES = { + "start_check": {"LinkCheckStatus": "Pending"}, + "read_check": {"LinkCheckStatus": "Uninitiated"}, + "start_update": {"status": "Pending"}, + "read_update": {"status": "uninitiated", "results": []}, +} +# The states a link check reports; a link update also reports Scanning and Updating. +LINK_CHECK_STATES = [ + UserTaskStatus.PENDING, + UserTaskStatus.IN_PROGRESS, + UserTaskStatus.SUCCEEDED, + UserTaskStatus.FAILED, + UserTaskStatus.CANCELED, + UserTaskStatus.RETRYING, +] +LINK_UPDATE_STATES = [*LINK_CHECK_STATES, "Scanning", "Updating"] +# Text a failed run records, standing for an exception's message. +RECORDED_FAILURE = "{'raw_error_msg': 'connection to 10.0.0.7:5432 refused'}" +_DEFAULT = object() + + +def url(name, course_key): + """Return the v1 address ``name`` for ``course_key``.""" + return reverse(name, kwargs={"course_key": str(course_key)}) + + +def legacy_url(name, course_key): + """Return the v0 address ``name`` for ``course_key``.""" + return reverse(f"{LEGACY_NAMESPACE}:{name}", kwargs={"course_id": str(course_key)}) + + +def call(client, operation, course_key, legacy=False, params=None, body=_DEFAULT): + """Perform ``operation`` on ``course_key`` at the v1 address, or the v0 one with ``legacy``.""" + method, name, legacy_name, default_body, _ = OPERATIONS[operation] + address = legacy_url(legacy_name, course_key) if legacy else url(name, course_key) + if method in ("get", "head"): + return getattr(client, method)(address, params or {}) + data = default_body if body is _DEFAULT else body + if data is None: + return client.post(address) + return client.post(address, data, format="json") + + +def assert_feature_off(response): + """Assert ``response`` is the 404 answered where updating links to a previous run is not enabled.""" + envelope = assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + assert envelope["detail"] == PREVIOUS_RUN_LINK_UPDATES_DISABLED_MESSAGE + return envelope + + +def assert_invalid(response): + """Assert ``response`` is the 400 validation envelope, and return its ``errors`` member.""" + return assert_error_envelope(response, expected_status=400, expected_type_slug="validation")["errors"] + + +def record_link_check(user, course_key, state, links=None, error=None, created=None): + """Store a link check of ``course_key`` started by ``user``, as the task records one.""" + run = UserTaskStatus.objects.create( + user=user, + task_id=str(uuid.uuid4()), + task_class="cms.djangoapps.contentstore.tasks.check_broken_links", + name=CourseLinkCheckTask.generate_name({"course_key_string": str(course_key)}), + total_steps=1, + state=state, + ) + if links is not None: + UserTaskArtifact.objects.create( + status=run, name="BrokenLinks", file=ContentFile(json.dumps(links).encode(), name="links.json"), + ) + if error is not None: + UserTaskArtifact.objects.create(status=run, name="Error", text=error) + if created is not None: + UserTaskStatus.objects.filter(pk=run.pk).update(created=created) + return run + + +def record_link_update(user, course_key, state, results=None, created=None): + """Store an update of links to the previous run of ``course_key`` started by ``user``.""" + run = UserTaskStatus.objects.create( + user=user, + task_id=str(uuid.uuid4()), + task_class="cms.djangoapps.contentstore.tasks.update_course_rerun_links", + name=CourseLinkUpdateTask.generate_name({"course_id": str(course_key)}), + total_steps=2, + state=state, + ) + if results is not None: + UserTaskArtifact.objects.create( + status=run, name="LinkUpdateResults", file=ContentFile(json.dumps(results).encode(), name="results.json"), + ) + if created is not None: + UserTaskStatus.objects.filter(pk=run.pk).update(created=created) + return run + + +def _hours_ago(hours): + return datetime.now(UTC) - timedelta(hours=hours) + + +# v0 member names and the v1 names that replace them. +LEGACY_MEMBER_NAMES = { + "LinkCheckStatus": "status", + "LinkCheckCreatedAt": "created_at", + "LinkCheckOutput": "output", + "LinkCheckError": "error", + "displayName": "display_name", + "brokenLinks": "broken_links", + "lockedLinks": "locked_links", + "externalForbiddenLinks": "external_forbidden_links", + "previousRunLinks": "previous_run_links", + "originalLink": "original_link", + "isUpdated": "is_updated", + "updatedLink": "updated_link", +} + + +def in_v1_names(body): + """``body`` with every v0 member name replaced by its v1 name, at every depth.""" + if isinstance(body, dict): + return {LEGACY_MEMBER_NAMES.get(key, key): in_v1_names(value) for key, value in body.items()} + if isinstance(body, list): + return [in_v1_names(item) for item in body] + return body + + +def member_names(body): + """Every member name in ``body``, at every depth.""" + if isinstance(body, dict): + return set(body) | {name for value in body.values() for name in member_names(value)} + if isinstance(body, list): + return {name for item in body for name in member_names(item)} + return set() + + +def test_v0_names_are_translated_at_every_depth(): + body = {"LinkCheckOutput": {"sections": [{"displayName": "A", "x": [{"previousRunLinks": [{"isUpdated": True}]}]}]}} + assert in_v1_names(body) == { + "output": {"sections": [{"display_name": "A", "x": [{"previous_run_links": [{"is_updated": True}]}]}]}, + } + assert member_names(in_v1_names(body)) == {"output", "sections", "display_name", "x", "previous_run_links", + "is_updated"} + + +# --------------------------------------------------------------------------- +# Who may call the endpoints +# --------------------------------------------------------------------------- + +class _PatchedTasksMixin: + """Replace the link check and link update tasks with mocks, on both versions.""" + + def setUp(self): + super().setUp() + self.tasks = {} + for version, module in (("v1", SERVICE), ("v0", LEGACY_VIEWS)): + for task in TASK_OF.values(): + patcher = patch(f"{module}.{task}") + self.tasks[(version, task)] = patcher.start() + self.addCleanup(patcher.stop) + + def delay(self, version, operation): + """The mocked ``delay`` of the task ``operation`` queues on ``version``.""" + return self.tasks[(version, TASK_OF[operation])].delay + + def reset_tasks(self): + """Forget every call the task mocks have recorded.""" + for task in self.tasks.values(): + task.reset_mock() + + +class _AccessMixin(_PatchedTasksMixin): + """ + Checks a caller's status on an operation against the expected one, that a + run is queued exactly when the start is accepted, and that v0 decides the + same for the same caller and queues the same run. + """ + + def assert_access(self, user, operation, expected, course_key=None): + """Assert ``expected`` for ``operation`` by ``user`` on v1, and agreement with v0.""" + course_key = course_key or self.course.id + self.reset_tasks() + client = api_client(user) + response = call(client, operation, course_key) + assert response.status_code == expected, (operation, response.content) + if expected in (200, 202): + assert response.json() == EMPTY_BODIES[operation] + else: + assert_refused(response, expected) + if operation in STARTS: + delay = self.delay("v1", operation) + if expected == 202: + delay.assert_called_once() + assert delay.call_args.args[:2] == (user.id, str(course_key)) + else: + delay.assert_not_called() + if expected == 404 and operation not in STARTS: + # v0 does not look the course up on reads; ResponseDifferencesTest covers them. + return + legacy = call(client, operation, course_key, legacy=True) + legacy_expected = 200 if expected == 202 else expected + assert legacy.status_code == legacy_expected, f"v0 decided {legacy.status_code} for {operation}" + if expected in (200, 202): + assert legacy.json() == LEGACY_EMPTY_BODIES[operation] + if operation in STARTS: + assert self.delay("v0", operation).call_args == self.delay("v1", operation).call_args + + +@ddt.ddt +@previous_run_links(True) +class CourseOptimizerAccessLegacyPermissionsTest(_AccessMixin, SharedModuleStoreTestCase): + """Who may start and read runs when policy-based authoring permissions are off.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + cls.other_course = CourseFactory.create() + + def _user_with(self, role): + user = UserFactory() + role.add_users(user) + return user + + @ddt.data(*OPERATIONS) + def test_anonymous_caller_is_refused_with_401(self, operation): + self.assert_access(None, operation, 401) + + @ddt.data(*OPERATIONS) + def test_anonymous_caller_of_an_unknown_course_is_refused_with_401(self, operation): + self.assert_access(None, operation, 401, course_key=UNKNOWN_COURSE_KEY) + + @ddt.data(*OPERATIONS) + def test_user_without_a_role_is_refused_with_403(self, operation): + self.assert_access(UserFactory(), operation, 403) + + @ddt.data(*OPERATIONS) + def test_unknown_course_is_404_before_permission_is_checked(self, operation): + self.assert_access(UserFactory(), operation, 404, course_key=UNKNOWN_COURSE_KEY) + + @ddt.data(*OPERATIONS) + def test_limited_staff_is_refused(self, operation): + self.assert_access(self._user_with(CourseLimitedStaffRole(self.course.id)), operation, 403) + + @ddt.data(*OPERATIONS) + def test_staff_and_instructors_of_another_course_are_refused(self, operation): + self.assert_access(self._user_with(CourseStaffRole(self.other_course.id)), operation, 403) + self.assert_access(self._user_with(CourseInstructorRole(self.other_course.id)), operation, 403) + + @ddt.data(*OPERATIONS) + def test_course_staff_is_allowed(self, operation): + self.assert_access(self._user_with(CourseStaffRole(self.course.id)), operation, OPERATIONS[operation][4]) + + @ddt.data(*OPERATIONS) + def test_course_instructor_is_allowed(self, operation): + user = self._user_with(CourseInstructorRole(self.course.id)) + self.assert_access(user, operation, OPERATIONS[operation][4]) + + @ddt.data(*OPERATIONS) + def test_org_staff_and_org_instructor_are_allowed(self, operation): + expected = OPERATIONS[operation][4] + self.assert_access(self._user_with(OrgStaffRole(self.course.id.org)), operation, expected) + self.assert_access(self._user_with(OrgInstructorRole(self.course.id.org)), operation, expected) + + @ddt.data(*OPERATIONS) + def test_global_staff_is_allowed(self, operation): + self.assert_access(UserFactory(is_staff=True), operation, OPERATIONS[operation][4]) + + @ddt.data(*OPERATIONS) + def test_a_superuser_who_is_not_global_staff_is_allowed_as_on_v0(self, operation): + """The course role lookup consults the permission library, which admits superusers, on v1 as on v0.""" + self.assert_access(UserFactory(is_superuser=True, is_staff=False), operation, OPERATIONS[operation][4]) + + @ddt.data(*OPERATIONS) + def test_a_version_only_course_key_is_404(self, operation): + """The key parses but names no course run; v0 has no address that takes it.""" + response = call(api_client(UserFactory(is_staff=True)), operation, VERSION_ONLY_KEY) + assert_refused(response, 404) + + @ddt.data(*OPERATIONS) + def test_a_library_is_not_a_course(self, operation): + library_key = LibraryFactory.create().location.library_key + assert_refused(call(api_client(UserFactory(is_staff=True)), operation, library_key), 404) + + def test_starting_a_check_needs_only_read_access_and_the_rest_write_access(self): + """ + With Studio write access refused and read access kept, a check may be + started but not read, and nothing about link updates is allowed; v0 + decides the same. + """ + staff = self._user_with(CourseStaffRole(self.course.id)) + refuse_write = {LegacyAuthoringPermission.WRITE: lambda *_: False} + with patch.dict(LEGACY_PERMISSION_HANDLER_MAP, refuse_write): + self.assert_access(staff, "start_check", 202) + for operation in ("read_check", "start_update", "read_update"): + self.assert_access(staff, operation, 403) + + def test_starting_a_check_is_refused_without_read_access(self): + staff = self._user_with(CourseStaffRole(self.course.id)) + refuse_read = {LegacyAuthoringPermission.READ: lambda *_: False} + with patch.dict(LEGACY_PERMISSION_HANDLER_MAP, refuse_read): + self.assert_access(staff, "start_check", 403) + for operation in ("read_check", "start_update", "read_update"): + self.assert_access(staff, operation, OPERATIONS[operation][4]) + + +@ddt.ddt +@previous_run_links(True) +class CourseOptimizerAccessAuthzTest(CourseAuthoringAuthzTestMixin, _AccessMixin, SharedModuleStoreTestCase): + """ + Who may start and read runs when policy-based authoring permissions are + on: every operation needs the permission to edit course content. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + cls.other_course = CourseFactory.create() + + def _user_with(self, role, course_key=None): + user = UserFactory() + self.add_user_to_role_in_course(user, role, course_key or self.course.id) + return user + + @ddt.data(*OPERATIONS) + def test_anonymous_caller_is_refused_with_401(self, operation): + self.assert_access(None, operation, 401) + + @ddt.data(*OPERATIONS) + def test_user_without_a_role_is_refused_with_403(self, operation): + self.assert_access(self.unauthorized_user, operation, 403) + + @ddt.data(*OPERATIONS) + def test_unknown_course_is_404_before_permission_is_checked(self, operation): + self.assert_access(self.unauthorized_user, operation, 404, course_key=UNKNOWN_COURSE_KEY) + + @ddt.data(*[(operation, role) for operation in OPERATIONS for role in ( + COURSE_EDITOR.external_key, COURSE_STAFF.external_key, COURSE_ADMIN.external_key, + )]) + @ddt.unpack + def test_roles_that_may_edit_content_are_allowed(self, operation, role): + self.assert_access(self._user_with(role), operation, OPERATIONS[operation][4]) + + @ddt.data(*[(operation, role) for operation in OPERATIONS for role in ( + COURSE_AUDITOR.external_key, + COURSE_LIMITED_STAFF.external_key, + COURSE_DATA_RESEARCHER.external_key, + COURSE_BETA_TESTER.external_key, + )]) + @ddt.unpack + def test_roles_that_may_not_edit_content_are_refused(self, operation, role): + """An auditor may view the course, but starting a check needs the editing permission too.""" + self.assert_access(self._user_with(role), operation, 403) + + @ddt.data(*OPERATIONS) + def test_a_role_on_another_course_is_refused(self, operation): + user = self._user_with(COURSE_STAFF.external_key, self.other_course.id) + self.assert_access(user, operation, OPERATIONS[operation][4], course_key=self.other_course.id) + self.assert_access(user, operation, 403) + + @ddt.data(*OPERATIONS) + def test_legacy_course_staff_without_a_policy_role_is_refused(self, operation): + user = UserFactory() + CourseAccessRole.objects.create(user=user, role="staff", course_id=self.course.id, org=self.course.id.org) + self.assert_access(user, operation, 403) + + @ddt.data(*OPERATIONS) + def test_staff_and_superuser_are_allowed_without_a_role(self, operation): + self.assert_access(self.staff_user, operation, OPERATIONS[operation][4]) + self.assert_access(self.super_user, operation, OPERATIONS[operation][4]) + + def test_an_inactive_session_with_a_role_is_accepted(self): + """The policy does not look at whether the account is activated, on v1 as on v0.""" + user = self._user_with(COURSE_EDITOR.external_key) + client = session_client(user) + user.is_active = False + user.save() + for operation in OPERATIONS: + assert call(client, operation, self.course.id).status_code == OPERATIONS[operation][4], operation + assert call(client, operation, self.course.id, legacy=True).status_code == 200, operation + + +class _FeatureOffMixin(_PatchedTasksMixin): + """ + Where updating links to a previous run is not enabled, both link update + operations answer 404 to a caller who may use them, and nothing is + queued; a caller who may not is still refused with 403. v0 answers 400 + and 403. Link checks are not affected. + """ + + def editor(self): + """Return a user who may edit the course's content.""" + raise NotImplementedError + + def outsider(self): + """Return a signed-in user with no access to the course.""" + raise NotImplementedError + + def test_link_updates_are_404_for_a_caller_who_may_use_them(self): + client = api_client(self.editor()) + for operation in ("start_update", "read_update"): + response = call(client, operation, self.course.id) + assert_feature_off(response) + legacy = call(client, operation, self.course.id, legacy=True) + assert legacy.status_code == 400 + assert legacy.json() == {"error": "Course optimizer check for previous run links is not enabled."} + self.delay("v1", "start_update").assert_not_called() + self.delay("v0", "start_update").assert_not_called() + + def test_the_feature_is_checked_before_the_body(self): + response = call(api_client(self.editor()), "start_update", self.course.id, body={"action": "neither"}) + assert_feature_off(response) + + def test_a_caller_who_may_not_use_them_is_still_refused_with_403(self): + client = api_client(self.outsider()) + for operation in ("start_update", "read_update"): + assert_refused(call(client, operation, self.course.id), 403) + assert call(client, operation, self.course.id, legacy=True).status_code == 403 + + def test_link_checks_are_not_affected(self): + client = api_client(self.editor()) + assert call(client, "start_check", self.course.id).status_code == 202 + self.delay("v1", "start_check").assert_called_once() + assert call(client, "read_check", self.course.id).json() == EMPTY_BODIES["read_check"] + + +@previous_run_links(False) +class CourseOptimizerFeatureOffLegacyPermissionsTest(_FeatureOffMixin, SharedModuleStoreTestCase): + """The feature off, with policy-based authoring permissions off.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def editor(self): + user = UserFactory() + CourseStaffRole(self.course.id).add_users(user) + return user + + def outsider(self): + return UserFactory() + + def test_instructors_and_global_staff_get_404_too(self): + instructor = UserFactory() + CourseInstructorRole(self.course.id).add_users(instructor) + for user in (instructor, UserFactory(is_staff=True)): + for operation in ("start_update", "read_update"): + response = call(api_client(user), operation, self.course.id) + assert_feature_off(response) + self.delay("v1", "start_update").assert_not_called() + + +@previous_run_links(False) +class CourseOptimizerFeatureOffAuthzTest(CourseAuthoringAuthzTestMixin, _FeatureOffMixin, SharedModuleStoreTestCase): + """The feature off, with policy-based authoring permissions on.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def editor(self): + user = UserFactory() + self.add_user_to_role_in_course(user, COURSE_EDITOR.external_key, self.course.id) + return user + + def outsider(self): + return self.unauthorized_user + + +class CourseOptimizerPermissionWiringTest(SimpleTestCase): + """Which permission each action checks, and which access each permission asks for.""" + + def test_each_action_checks_the_course_then_its_own_permission(self): + for viewset, action, expected in ( + (CourseLinkCheckViewSet, "create", [IsAuthenticated, IsExistingCourse, CanStartLinkCheck]), + (CourseLinkCheckViewSet, "retrieve", [IsAuthenticated, IsExistingCourse, CanEditCourseContent]), + (CoursePreviousRunLinkUpdateViewSet, "create", [IsAuthenticated, IsExistingCourse, CanEditCourseContent]), + (CoursePreviousRunLinkUpdateViewSet, "retrieve", [IsAuthenticated, IsExistingCourse, CanEditCourseContent]), + (CourseLinkCheckViewSet, None, [IsAuthenticated, IsExistingCourse]), + (CoursePreviousRunLinkUpdateViewSet, "metadata", [IsAuthenticated, IsExistingCourse]), + ): + view = viewset() + view.action = action + # The JWT middleware adds NotJwtRestrictedApplication to JWT views on their first request. + got = [type(p) for p in view.get_permissions() if type(p).__name__ != "NotJwtRestrictedApplication"] + assert got == expected, (viewset, action) + + def test_an_action_without_a_declared_permission_is_refused(self): + for viewset in (CourseLinkCheckViewSet, CoursePreviousRunLinkUpdateViewSet): + view = viewset() + for action in ("destroy", "update", "partial_update", "list"): + view.action = action + with pytest.raises(ImproperlyConfigured): + view.get_permissions() + + def test_a_routed_action_without_a_declared_permission_is_never_served(self): + """A request reaching such an action is answered with a server error, not the handler's body.""" + + class Unchecked(CourseLinkCheckViewSet): + permission_classes = CourseLinkCheckViewSet.permission_classes + + def destroy(self, request, course_key): # pylint: disable=unused-argument + raise AssertionError("the handler ran without a permission check") + + course_key = "course-v1:edX+Wiring+Run" + view = Unchecked.as_view({"delete": "destroy"}) + request = APIRequestFactory().delete(url(LINK_CHECK, course_key)) + force_authenticate(request, user=SimpleNamespace(is_authenticated=True, is_active=True)) + response = view(request, course_key=CourseKey.from_string(course_key)) + assert_error_envelope(response, expected_status=500, expected_type_slug="internal") + + def test_the_permissions_ask_for_the_content_editing_permission_with_read_or_write_access(self): + course_key = "course-v1:edX+Wiring+Run" + request = SimpleNamespace(user=object()) + view = SimpleNamespace(kwargs={"course_key": course_key}) + target = "cms.djangoapps.contentstore.views.permissions.user_has_course_permission" + for permission, access in ( + (CanStartLinkCheck, LegacyAuthoringPermission.READ), + (CanEditCourseContent, LegacyAuthoringPermission.WRITE), + ): + for decision in (True, False): + with patch(target, return_value=decision) as check: + assert permission().has_permission(request, view) is decision + check.assert_called_once_with(request.user, "courses.edit_course_content", course_key, access) + + def test_the_permissions_refuse_a_view_without_a_course(self): + request = SimpleNamespace(user=object()) + view = SimpleNamespace(kwargs={}) + for permission in (CanStartLinkCheck, CanEditCourseContent): + assert permission().has_permission(request, view) is False + + +@previous_run_links(True) +class CourseOptimizerAuthenticationTest(_PatchedTasksMixin, SharedModuleStoreTestCase): + """How callers authenticate, with policy-based authoring permissions off.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def _course_staff(self): + user = UserFactory() + CourseStaffRole(self.course.id).add_users(user) + return user + + def test_a_jwt_caller_may_start_and_read(self): + user = self._course_staff() + client = APIClient() + client.credentials(HTTP_AUTHORIZATION=f"JWT {create_jwt_for_user(user)}") + for operation in OPERATIONS: + response = call(client, operation, self.course.id) + assert response.status_code == OPERATIONS[operation][4], (operation, response.content) + self.delay("v1", "start_check").assert_called_once() + assert self.delay("v1", "start_check").call_args.args[0] == user.id + + def test_a_session_caller_may_start_and_read(self): + user = self._course_staff() + client = session_client(user) + for operation in OPERATIONS: + assert call(client, operation, self.course.id).status_code == OPERATIONS[operation][4], operation + assert self.delay("v1", "start_update").call_args.args[0] == user.id + + def test_an_inactive_session_is_authenticated(self): + """ + A session whose account is not activated is accepted, as on v0. Global + staff then pass; course staff are refused (403, not 401) because a + course role does not count while the account is inactive. + """ + for user, allowed in ((UserFactory(is_staff=True), True), (self._course_staff(), False)): + client = session_client(user) + user.is_active = False + user.save() + for operation in OPERATIONS: + expected = OPERATIONS[operation][4] if allowed else 403 + response = call(client, operation, self.course.id) + assert response.status_code == expected, (operation, response.content) + if not allowed: + assert_refused(response, 403) + legacy_expected = 200 if allowed else 403 + assert call(client, operation, self.course.id, legacy=True).status_code == legacy_expected + + def test_a_bearer_token_is_refused(self): + """Bearer tokens are not accepted here, though v0 takes them.""" + user = UserFactory(is_staff=True) + token = AccessTokenFactory(user=user, application=ApplicationFactory(user=user)).token + header = {"HTTP_AUTHORIZATION": f"Bearer {token}"} + assert_refused(APIClient().get(url(LINK_CHECK, self.course.id), **header), 401) + assert_refused(APIClient().post(url(LINK_CHECK, self.course.id), **header), 401) + self.delay("v1", "start_check").assert_not_called() + assert APIClient().get(legacy_url("link_check_status", self.course.id), **header).status_code == 200 + + +# --------------------------------------------------------------------------- +# Whose runs a caller sees +# --------------------------------------------------------------------------- + +class _VisibilityMixin: + """ + A read returns the latest run of the course started by the caller, never + another user's; a superuser sees the latest run of anyone. v0 shows the + same run. + """ + + def editor(self): + """Return a new user who may start and read runs of the course.""" + raise NotImplementedError + + def superuser(self): + """Return a superuser who may read runs of the course.""" + raise NotImplementedError + + def setUp(self): + super().setUp() + self.course = CourseFactory.create() + self.author, self.other = self.editor(), self.editor() + + def read_status(self, user, operation): + """The state ``user`` reads on v1, after asserting v0 reads the same one.""" + client = api_client(user) + new = call(client, operation, self.course.id) + legacy = call(client, operation, self.course.id, legacy=True) + assert new.status_code == legacy.status_code == 200, (new.content, legacy.content) + state = new.json()["status"] + legacy_body = in_v1_names(legacy.json()) + legacy_state = "Uninitiated" if legacy_body["status"] == "uninitiated" else legacy_body["status"] + assert legacy_state == state + return state + + def _record(self, operation, user, state, created, course_key=None): + course_key = course_key or self.course.id + if operation == "read_check": + return record_link_check(user, course_key, state, created=created) + return record_link_update(user, course_key, state, created=created) + + def _assert_visibility(self, operation): + self._record(operation, self.other, UserTaskStatus.FAILED, created=_hours_ago(2)) + self._record(operation, self.author, UserTaskStatus.IN_PROGRESS, created=_hours_ago(1)) + assert self.read_status(self.author, operation) == UserTaskStatus.IN_PROGRESS + assert self.read_status(self.other, operation) == UserTaskStatus.FAILED + assert self.read_status(self.editor(), operation) == "Uninitiated" + assert self.read_status(self.superuser(), operation) == UserTaskStatus.IN_PROGRESS + + def test_each_caller_sees_only_their_own_link_checks(self): + self._assert_visibility("read_check") + + def test_each_caller_sees_only_their_own_link_updates(self): + self._assert_visibility("read_update") + + def test_a_run_of_another_course_is_not_seen(self): + other_course = CourseFactory.create() + for operation in ("read_check", "read_update"): + self._record(operation, self.author, UserTaskStatus.SUCCEEDED, _hours_ago(1), other_course.id) + assert call(api_client(self.author), operation, self.course.id).json()["status"] == "Uninitiated" + + def test_the_latest_of_the_callers_runs_is_returned(self): + for operation in ("read_check", "read_update"): + self._record(operation, self.author, UserTaskStatus.FAILED, created=_hours_ago(1)) + self._record(operation, self.author, UserTaskStatus.PENDING, created=_hours_ago(3)) + assert self.read_status(self.author, operation) == UserTaskStatus.FAILED + + +@previous_run_links(True) +class CourseOptimizerVisibilityLegacyPermissionsTest(_VisibilityMixin, ModuleStoreTestCase): + """Visibility with policy-based authoring permissions off.""" + + def editor(self): + user = UserFactory() + CourseStaffRole(self.course.id).add_users(user) + return user + + def superuser(self): + return UserFactory(is_superuser=True, is_staff=False) + + def test_global_staff_who_is_not_a_superuser_sees_only_their_own_runs(self): + for operation in ("read_check", "read_update"): + self._record(operation, self.author, UserTaskStatus.IN_PROGRESS, created=_hours_ago(1)) + assert self.read_status(UserFactory(is_staff=True), operation) == "Uninitiated" + + +@previous_run_links(True) +class CourseOptimizerVisibilityAuthzTest(_VisibilityMixin, CourseAuthoringAuthzTestMixin, ModuleStoreTestCase): + """Visibility with policy-based authoring permissions on: a superuser needs no role.""" + + def editor(self): + user = UserFactory() + self.add_user_to_role_in_course(user, COURSE_EDITOR.external_key, self.course.id) + return user + + def superuser(self): + return self.super_user + + +# --------------------------------------------------------------------------- +# What reads return +# --------------------------------------------------------------------------- + +def _reported_blocks(report): + """Every component reported under the sections of a link check report.""" + for section in report["sections"]: + for subsection in section["subsections"]: + for unit in subsection["units"]: + yield from unit["blocks"] + + +@ddt.ddt +@previous_run_links(True) +class CourseOptimizerReadTest(ModuleStoreTestCase): + """What a read returns, by state and by view.""" + + def setUp(self): + super().setUp() + self.course = CourseFactory.create() + self.editor = UserFactory() + CourseStaffRole(self.course.id).add_users(self.editor) + self.client = api_client(self.editor) + + def read(self, operation, legacy=False, **params): + response = call(self.client, operation, self.course.id, legacy=legacy, params=params) + assert response.status_code == 200, response.content + return response.json() + + def _course_with_flagged_links(self): + """Give the course one component and record a successful check that flagged four links in it.""" + chapter = BlockFactory.create(parent_location=self.course.location, category="chapter", display_name="Week 1") + sequential = BlockFactory.create(parent_location=chapter.location, category="sequential", display_name="Lesson") + vertical = BlockFactory.create(parent_location=sequential.location, category="vertical", display_name="Unit") + html = BlockFactory.create(parent_location=vertical.location, category="html", display_name="Reading") + block_id = str(html.location) + links = [ + [block_id, "https://broken.example.com/page", LinkState.BROKEN], + [block_id, "/static/handout.pdf", LinkState.LOCKED], + [block_id, "https://forbidden.example.com/", LinkState.EXTERNAL_FORBIDDEN], + [block_id, "https://studio.example.com/course/course-v1:Old+Run+2025/info", LinkState.PREVIOUS_RUN], + ] + record_link_check(self.editor, self.course.id, UserTaskStatus.SUCCEEDED, links=links) + return chapter, sequential, vertical, html + + @ddt.data(*LINK_CHECK_STATES) + def test_a_link_check_reports_its_state_verbatim(self, state): + links = [] if state == UserTaskStatus.SUCCEEDED else None + run = record_link_check(self.editor, self.course.id, state, links=links) + body = self.read("read_check") + assert body["status"] == state + assert body["created_at"] == self.read("read_check", legacy=True)["LinkCheckCreatedAt"] + assert body["created_at"].startswith(run.created.strftime("%Y-%m-%dT%H:%M:%S")) + assert set(body) == {"status", "created_at", "error"} + + @ddt.data(*LINK_UPDATE_STATES) + def test_a_link_update_reports_its_state_verbatim(self, state): + record_link_update(self.editor, self.course.id, state) + assert self.read("read_update") == {"status": state} + assert self.read("read_update", legacy=True)["status"] == state + + def test_every_reported_state_is_published(self): + assert set(LINK_UPDATE_STATES) | {"Uninitiated"} == set(RUN_STATES) + + def test_a_failed_link_check_reports_a_fixed_message_not_the_recorded_text(self): + record_link_check(self.editor, self.course.id, UserTaskStatus.FAILED, error=RECORDED_FAILURE) + response = call(self.client, "read_check", self.course.id) + assert response.json()["error"] == LINK_CHECK_ERROR_MESSAGE + assert "10.0.0.7" not in response.content.decode() + assert self.read("read_check", legacy=True)["LinkCheckError"] == RECORDED_FAILURE + + def test_a_canceled_link_check_with_an_error_recorded_reports_the_fixed_message(self): + record_link_check(self.editor, self.course.id, UserTaskStatus.CANCELED, error=RECORDED_FAILURE) + assert self.read("read_check")["error"] == LINK_CHECK_ERROR_MESSAGE + + @ddt.data( + (UserTaskStatus.FAILED, None), + (UserTaskStatus.FAILED, ""), + (UserTaskStatus.IN_PROGRESS, RECORDED_FAILURE), + ) + @ddt.unpack + def test_no_error_is_reported_unless_a_stopped_run_recorded_one(self, state, error): + record_link_check(self.editor, self.course.id, state, error=error) + assert self.read("read_check")["error"] is None + assert "LinkCheckError" not in self.read("read_check", legacy=True) + + def test_the_full_view_reports_the_flagged_links_in_the_course(self): + chapter, sequential, vertical, html = self._course_with_flagged_links() + body = self.read("read_check", **FULL) + report = body["output"] + assert [section["display_name"] for section in report["sections"]] == ["Week 1"] + (section,) = report["sections"] + assert UsageKey.from_string(section["id"]).block_id == chapter.location.block_id + (subsection,) = section["subsections"] + assert UsageKey.from_string(subsection["id"]).block_id == sequential.location.block_id + (unit,) = subsection["units"] + assert (UsageKey.from_string(unit["id"]).block_id, unit["display_name"]) == (vertical.location.block_id, "Unit") + (block,) = unit["blocks"] + assert UsageKey.from_string(block["id"]).block_id == html.location.block_id + assert block["display_name"] == "Reading" + assert block["broken_links"] == ["https://broken.example.com/page"] + assert block["locked_links"] == ["/static/handout.pdf"] + assert block["external_forbidden_links"] == ["https://forbidden.example.com/"] + assert block["previous_run_links"] == [{ + "original_link": "https://studio.example.com/course/course-v1:Old+Run+2025/info", + "is_updated": False, + "updated_link": None, + }] + assert (report["course_updates"], report["custom_pages"]) == ([], []) + assert_matches_published_schema(body, "LinkCheck.v1") + + def test_the_full_report_is_v0s_report_in_snake_case(self): + self._course_with_flagged_links() + new = self.read("read_check", **FULL)["output"] + legacy = self.read("read_check", legacy=True)["LinkCheckOutput"] + assert new == in_v1_names(legacy) + assert list(_reported_blocks(new)) != [] + assert [name for name in member_names(new) if not re.fullmatch(r"[a-z][a-z0-9_]*", name)] == [] + + def test_the_default_view_does_not_build_the_report(self): + self._course_with_flagged_links() + with patch(f"{SERVICE}.generate_broken_links_descriptor", side_effect=AssertionError("report built")): + assert set(self.read("read_check")) == {"status", "created_at", "error"} + assert self.read("read_check", view="minimal")["status"] == UserTaskStatus.SUCCEEDED + assert call(self.client, "read_check", self.course.id, params=FULL).status_code == 500 + + @ddt.data(UserTaskStatus.PENDING, UserTaskStatus.FAILED) + def test_the_full_view_of_a_run_that_did_not_succeed_has_no_report(self, state): + record_link_check(self.editor, self.course.id, state, error=RECORDED_FAILURE) + body = self.read("read_check", **FULL) + assert body["output"] is None + assert_matches_published_schema(body, "LinkCheck.v1") + + def test_the_full_view_without_a_run(self): + assert self.read("read_check", **FULL) == {**EMPTY_BODIES["read_check"], "output": None} + assert self.read("read_update", **FULL) == {"status": "Uninitiated", "results": []} + + def _link_update_results(self): + return [ + { + "id": "1", + "type": "course_updates", + "original_url": "https://studio.example.com/course/course-v1:Old+Run+2025/info", + "new_url": f"https://studio.example.com/course/{self.course.id}/info", + "success": True, + }, + { + "id": "block-v1:Org+Course+Run+type@html+block@reading", + "type": "course_content", + "original_url": "https://studio.example.com/course/course-v1:Old+Run+2025/progress", + "new_url": "https://studio.example.com/course/course-v1:Old+Run+2025/progress", + "success": False, + "error_message": "KeyError: 'children' at /edx/app/edxapp/xblock.py:88", + }, + ] + + def test_the_full_view_reports_each_links_outcome_with_a_fixed_failure_message(self): + results = self._link_update_results() + record_link_update(self.editor, self.course.id, UserTaskStatus.SUCCEEDED, results=results) + response = call(self.client, "read_update", self.course.id, params=FULL) + body = response.json() + assert body == { + "status": UserTaskStatus.SUCCEEDED, + "results": [ + {**results[0], "error_message": None}, + {**results[1], "error_message": LINK_NOT_UPDATED_MESSAGE}, + ], + } + assert "xblock.py" not in response.content.decode() + assert_matches_published_schema(body, "PreviousRunLinkUpdate.v1") + legacy = self.read("read_update", legacy=True) + assert legacy["results"][1]["error_message"] == results[1]["error_message"] + + def test_the_default_view_of_a_link_update_is_its_state(self): + record_link_update(self.editor, self.course.id, UserTaskStatus.SUCCEEDED, results=self._link_update_results()) + assert self.read("read_update") == {"status": UserTaskStatus.SUCCEEDED} + assert self.read("read_update", view="minimal") == {"status": UserTaskStatus.SUCCEEDED} + + def test_the_default_view_of_a_link_update_does_not_read_its_results(self): + record_link_update(self.editor, self.course.id, UserTaskStatus.SUCCEEDED, results=self._link_update_results()) + with patch(f"{SERVICE}.get_course_link_update_data", side_effect=AssertionError("results read")): + assert self.read("read_update") == {"status": UserTaskStatus.SUCCEEDED} + assert call(self.client, "read_update", self.course.id, params=FULL).status_code == 500 + + def test_a_flagged_link_below_a_unit_child_fails_the_full_read_on_both_versions(self): + """ + A known gap of the report, kept as v0 has it: a link in a component + nested below a unit's child, such as one inside a library content + block, gives the report a fifth level the four-level shape cannot + hold, and the full read answers 500. The default view still works. + """ + chapter = BlockFactory.create(parent_location=self.course.location, category="chapter") + sequential = BlockFactory.create(parent_location=chapter.location, category="sequential") + vertical = BlockFactory.create(parent_location=sequential.location, category="vertical") + library_content = BlockFactory.create(parent_location=vertical.location, category="library_content") + html = BlockFactory.create(parent_location=library_content.location, category="html") + links = [[str(html.location), "https://broken.example.com/", LinkState.BROKEN]] + record_link_check(self.editor, self.course.id, UserTaskStatus.SUCCEEDED, links=links) + assert call(self.client, "read_check", self.course.id, params=FULL).status_code == 500 + assert call(self.client, "read_check", self.course.id, legacy=True).status_code == 500 + assert self.read("read_check")["status"] == UserTaskStatus.SUCCEEDED + + def test_a_link_update_that_succeeded_without_results_reports_none(self): + record_link_update(self.editor, self.course.id, UserTaskStatus.SUCCEEDED) + assert self.read("read_update", **FULL) == {"status": UserTaskStatus.SUCCEEDED, "results": []} + + @ddt.data("read_check", "read_update") + def test_an_unknown_view_is_refused(self, operation): + response = call(self.client, operation, self.course.id, params={"view": "default"}) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"view": ['"default" is not a valid choice.']} + + @ddt.data("read_check", "read_update") + def test_head_answers_without_a_body(self, operation): + name = OPERATIONS[operation][1] + for params in ({}, FULL): + response = self.client.head(url(name, self.course.id), params) + assert response.status_code == 200 + assert response.content == b"" + + @ddt.data("read_check", "read_update") + def test_the_bodies_match_the_published_schema(self, operation): + component = {"read_check": "LinkCheck.v1", "read_update": "PreviousRunLinkUpdate.v1"}[operation] + for params in ({}, FULL): + assert_matches_published_schema(self.read(operation, **params), component) + + +# --------------------------------------------------------------------------- +# Starting runs +# --------------------------------------------------------------------------- + +@previous_run_links(True) +class CourseOptimizerStartTest(_PatchedTasksMixin, ModuleStoreTestCase): + """What starting a run queues, accepts and refuses.""" + + LINKS = [ + {"url": "https://studio.example.com/course/course-v1:Old+Run+2025/info", "type": "course_updates", "id": "1"}, + { + "url": "/course/course-v1:Old+Run+2025/progress", + "type": "custom_pages", + "id": "block-v1:Old+Run+2025+type@static_tab+block@syllabus", + }, + ] + + def setUp(self): + super().setUp() + self.course = CourseFactory.create() + self.editor = UserFactory() + CourseStaffRole(self.course.id).add_users(self.editor) + self.client = api_client(self.editor) + + def start(self, operation, body=_DEFAULT, legacy=False): + return call(self.client, operation, self.course.id, legacy=legacy, body=body) + + def test_a_link_check_is_queued_as_v0_queues_it(self): + response = self.start("start_check") + assert response.status_code == 202 + assert response.json() == {"status": UserTaskStatus.PENDING} + assert_matches_published_schema(response.json(), "CourseOptimizerQueuedRun.v1") + delay = self.delay("v1", "start_check") + delay.assert_called_once() + assert delay.call_args.args[:2] == (self.editor.id, str(self.course.id)) + assert self.start("start_check", legacy=True).status_code == 200 + assert self.delay("v0", "start_check").call_args == delay.call_args + + def test_a_link_check_ignores_its_body(self): + response = self.client.post(url(LINK_CHECK, self.course.id), data="{not json", content_type="application/json") + assert response.status_code == 202 + self.delay("v1", "start_check").assert_called_once() + + def test_every_link_is_queued_as_v0_queues_it(self): + response = self.start("start_update", {"action": "all"}) + assert response.status_code == 202 + assert response.json() == {"status": UserTaskStatus.PENDING} + delay = self.delay("v1", "start_update") + delay.assert_called_once() + assert delay.call_args.args[:4] == (self.editor.id, str(self.course.id), "all", []) + assert self.start("start_update", {"action": "all"}, legacy=True).status_code == 200 + assert self.delay("v0", "start_update").call_args == delay.call_args + + def test_the_named_links_are_queued_as_v0_queues_them(self): + body = {"action": "single", "data": self.LINKS} + assert self.start("start_update", body).status_code == 202 + delay = self.delay("v1", "start_update") + assert delay.call_args.args[:4] == (self.editor.id, str(self.course.id), "single", self.LINKS) + assert self.start("start_update", body, legacy=True).status_code == 200 + assert self.delay("v0", "start_update").call_args == delay.call_args + + def test_links_sent_with_action_all_are_not_read(self): + """Every link is changed then, so links in the body are neither validated nor queued, as on v0.""" + for links in ("not a list", [{"type": "course_updates"}], self.LINKS): + self.reset_tasks() + body = {"action": "all", "data": links} + assert self.start("start_update", body).status_code == 202, links + assert self.delay("v1", "start_update").call_args.args[2:4] == ("all", []) + assert self.start("start_update", body, legacy=True).status_code == 200, links + self.delay("v0", "start_update").assert_called_once() + + def _refused(self, body): + errors = assert_invalid(self.start("start_update", body)) + self.delay("v1", "start_update").assert_not_called() + return errors + + def test_an_action_is_required(self): + assert self._refused({}) == {"action": ["This field is required."]} + assert self.start("start_update", {}, legacy=True).status_code == 400 + + def test_an_unknown_action_is_refused(self): + assert self._refused({"action": "some"}) == {"action": ['"some" is not a valid choice.']} + + def test_single_needs_links(self): + expected = {"data": ["This field is required when action is 'single'."]} + assert self._refused({"action": "single"}) == expected + assert self._refused({"action": "single", "data": []}) == expected + assert self.start("start_update", {"action": "single"}, legacy=True).status_code == 400 + + # The error library renders the errors of each link as Python text rather + # than as an object of members. Change these to per-link objects once it + # normalizes nested errors. + + def test_each_link_needs_a_url_a_type_and_an_id(self): + errors = self._refused({"action": "single", "data": [{"type": "course_updates", "id": "1"}]}) + assert errors == {"data": ["{'url': [ErrorDetail(string='This field is required.', code='required')]}"]} + link, blank_type = self.LINKS[0], {**self.LINKS[0], "type": ""} + errors = self._refused({"action": "single", "data": [link, blank_type]}) + assert errors == { + "data": ["{}", "{'type': [ErrorDetail(string='This field may not be blank.', code='blank')]}"], + } + + def test_the_links_must_be_a_list(self): + errors = self._refused({"action": "single", "data": {"url": "x", "type": "y", "id": "z"}}) + assert errors == {"data": [ + "{'non_field_errors': [ErrorDetail(string='Expected a list of items but got type \"dict\".', " + "code='not_a_list')]}" + ]} + + def test_a_body_that_is_not_an_object_is_refused(self): + response = self.client.post(url(LINK_UPDATE, self.course.id), data="[1, 2]", content_type="application/json") + assert assert_invalid(response) == {"non_field_errors": ["Invalid data. Expected a dictionary, but got list."]} + self.delay("v1", "start_update").assert_not_called() + + +# --------------------------------------------------------------------------- +# Differences from v0 +# --------------------------------------------------------------------------- + +# Requests v0 and v1 answer differently beyond the error body and the member +# names, as (request, v0 answer, v1 answer, test in CourseOptimizerResponseDifferencesTest). +RESPONSE_DIFFERENCES = [ + ("start a link check or a link update", "200", "202", "test_starting_a_run_is_accepted"), + ("read a link check of an unknown course, as global staff", "500", "404", "test_reads_of_an_unknown_course"), + ("read a link update of an unknown course, as global staff", "200, uninitiated", "404", + "test_reads_of_an_unknown_course"), + ("read either of an unknown course, without a role", "403", "404", "test_reads_of_an_unknown_course"), + ("start or read a link update where the feature is off", "400", "404", "test_the_feature_off"), + ("start a link update naming a link with a missing or blank url, type or id", "200, queued", "400", + "test_a_link_missing_a_member"), + ("start a link update whose body is not a JSON object", "500", "400", "test_a_body_that_is_not_an_object"), + ("read with a view other than minimal or full", "200, view ignored", "400", "test_an_unknown_view"), + ("read a link check after a status was stored in the session", "the stored status", "Uninitiated", + "test_a_status_stored_in_the_session"), + ("read a link update with no run", "uninitiated, results []", "Uninitiated", "test_no_link_update_yet"), + ("start or read with a Bearer token", "200", "401", "test_a_bearer_token"), + ("slash-separated course key", "404 JSON", "404 HTML", "test_a_slash_separated_course_key"), +] + + +@previous_run_links(True) +class CourseOptimizerResponseDifferencesTest(_PatchedTasksMixin, ModuleStoreTestCase): + """Each request v0 and v1 answer differently, both answers observed.""" + + def setUp(self): + super().setUp() + self.course = CourseFactory.create() + self.editor = UserFactory() + CourseStaffRole(self.course.id).add_users(self.editor) + self.client = api_client(self.editor) + + def both(self, operation, course_key=None, client=None, **kwargs): + """The v0 and v1 responses to the same request.""" + client, course_key = client or self.client, course_key or self.course.id + return call(client, operation, course_key, legacy=True, **kwargs), call(client, operation, course_key, **kwargs) + + def test_every_listed_difference_has_its_test(self): + assert [name for *_, name in RESPONSE_DIFFERENCES if not hasattr(self, name)] == [] + + def test_starting_a_run_is_accepted(self): + for operation in STARTS: + legacy, new = self.both(operation) + assert (legacy.status_code, new.status_code) == (200, 202) + + def test_reads_of_an_unknown_course(self): + """ + v0 does not look the course up on reads. For global staff its link + check read fails sorting the report by the course outline the + modulestore cannot find; its link update read reports no run. + """ + global_staff = api_client(UserFactory(is_staff=True)) + legacy, new = self.both("read_check", UNKNOWN_COURSE_KEY, client=global_staff) + assert legacy.status_code == 500 + assert_refused(new, 404) + legacy, new = self.both("read_update", UNKNOWN_COURSE_KEY, client=global_staff) + assert legacy.status_code == 200 + assert legacy.json() == {"status": "uninitiated", "results": []} + assert_refused(new, 404) + for operation in ("read_check", "read_update"): + legacy, new = self.both(operation, UNKNOWN_COURSE_KEY, client=api_client(UserFactory())) + assert legacy.status_code == 403 + assert_refused(new, 404) + + def test_the_feature_off(self): + with previous_run_links(False): + for operation in ("start_update", "read_update"): + legacy, new = self.both(operation) + assert legacy.status_code == 400 + assert_feature_off(new) + + def test_a_link_missing_a_member(self): + complete = {"url": "/course/course-v1:Old+Run+2025/info", "type": "course_updates", "id": "1"} + for link in ( + {"type": "course_updates", "id": "1"}, + {"url": complete["url"], "id": "1"}, + {"url": complete["url"], "type": "course_updates"}, + {**complete, "type": ""}, + {**complete, "id": ""}, + ): + self.reset_tasks() + legacy, new = self.both("start_update", body={"action": "single", "data": [link]}) + assert legacy.status_code == 200, link + self.delay("v0", "start_update").assert_called_once() + assert list(assert_invalid(new)) == ["data"], link + self.delay("v1", "start_update").assert_not_called() + + def test_a_body_that_is_not_an_object(self): + address = {"legacy": legacy_url("rerun_link_update", self.course.id), "new": url(LINK_UPDATE, self.course.id)} + legacy = self.client.post(address["legacy"], data="[1, 2]", content_type="application/json") + new = self.client.post(address["new"], data="[1, 2]", content_type="application/json") + assert legacy.status_code == 500 + assert_invalid(new) + + def test_an_unknown_view(self): + for operation in ("read_check", "read_update"): + legacy, new = self.both(operation, params={"view": "default"}) + assert legacy.status_code == 200, operation + assert assert_invalid(new) == {"view": ['"default" is not a valid choice.']} + + def test_a_status_stored_in_the_session(self): + client = session_client(self.editor) + session = client.session + session["link_check_status"] = {str(self.course.id): "Stored"} + session.save() + legacy, new = self.both("read_check", client=client) + assert legacy.json() == {"LinkCheckStatus": "Stored"} + assert new.json() == EMPTY_BODIES["read_check"] + + def test_no_link_update_yet(self): + legacy, new = self.both("read_update") + assert legacy.json() == {"status": "uninitiated", "results": []} + assert new.json() == {"status": "Uninitiated"} + + def test_a_bearer_token(self): + user = UserFactory(is_staff=True) + token = AccessTokenFactory(user=user, application=ApplicationFactory(user=user)).token + client = APIClient() + client.credentials(HTTP_AUTHORIZATION=f"Bearer {token}") + for operation in OPERATIONS: + legacy, new = self.both(operation, client=client) + assert legacy.status_code == 200, operation + assert_refused(new, 401) + + def test_a_slash_separated_course_key(self): + """v0 routes the slash form and reports the course unknown in JSON; v1 has no route for it.""" + legacy = self.client.post("/api/contentstore/v0/link_check/edX/NoSuch/Course") + new = self.client.post("/api/authoring/v1/courses/edX/NoSuch/Course/link_check/") + assert legacy.status_code == new.status_code == 404 + assert legacy.json()["error_code"] == "course_does_not_exist" + assert new["Content-Type"].startswith("text/html") + + +# --------------------------------------------------------------------------- +# Parity with v0 +# --------------------------------------------------------------------------- + +# Members of an error body that differ between v0 and v1 for the same refused +# request. The comparison asserts nothing else differs, and that each occurs. +ENVELOPE_DIFFERENCES = [ + ("type", "errors use the standardized envelope"), + ("title", "errors use the standardized envelope"), + ("status", "errors use the standardized envelope"), + ("detail", "errors use the standardized envelope"), + ("errors", "a refused body maps each refused member to its messages"), + ("developer_message", "v0 error bodies carry developer_message; the envelope does not"), + ("error_code", "the v0 not-found body carries error_code; the envelope does not"), + ("error", "v0 refuses a link update body with {'error': ...}; the envelope replaces it"), +] +# Members of a successful body, in v1's names, that differ from v0's, each with +# the test asserting exactly where it occurs. +BODY_DIFFERENCES = [ + ("created_at", "always present, null without a run; v0 omits it", "test_reads_without_a_run"), + ("error", "always present: null, or a fixed message where v0 returns the recorded text", + "test_a_failed_link_check"), + ("output", "with view=full, null for a run that did not succeed; v0 omits it", "test_reads_without_a_run"), + ("status", "no link update yet is Uninitiated; v0 spells it uninitiated", "test_reads_without_a_run"), + ("results", "only with view=full; v0 always returns it", "test_reads_without_a_run"), + ("results[1].error_message", "a fixed message where v0 returns the recorded text", + "test_a_link_update_with_a_failed_link"), + ("results[0].error_message", "null for a link that was changed; v0 omits it", + "test_a_link_update_with_a_failed_link"), +] + + +def _occurs(path, observed): + """Whether ``path`` or a member below it is among the ``observed`` changed paths.""" + return any(key == path or key.startswith(f"{path}.") or key.startswith(f"{path}[") for key in observed) + + +def test_only_declared_differences_are_tolerated(): + assert undeclared_paths({"type", "errors.action[0]", "developer_message"}, ENVELOPE_DIFFERENCES) == set() + undeclared = undeclared_paths({"output.sections", "results[0].id"}, ENVELOPE_DIFFERENCES) + assert undeclared == {"output.sections", "results[0].id"} + + +class _ParityMixin(_PatchedTasksMixin): + """ + v1 against v0 for the same course and caller: bodies, with v0's member + names translated, differ only where a declared difference says, and + errors only by the envelope. + """ + + def editor_for(self, course): + """Return a user who may start and read runs of the course.""" + raise NotImplementedError + + def outsider(self): + """Return a signed-in user with no access to the course.""" + raise NotImplementedError + + def setUp(self): + super().setUp() + self.course = CourseFactory.create() + self.editor = self.editor_for(self.course) + self.observed = set() + + def pair(self, operation, client=None, course_key=None, **kwargs): + client, course_key = client or api_client(self.editor), course_key or self.course.id + return call(client, operation, course_key, legacy=True, **kwargs), call(client, operation, course_key, **kwargs) + + def changed(self, legacy, new, expected_status): + """The paths that differ between the bodies, v0's translated to v1's names, after checking statuses.""" + legacy_status = 200 if expected_status == 202 else expected_status + assert legacy.status_code == legacy_status, legacy.content + assert new.status_code == expected_status, new.content + changed = changed_paths(in_v1_names(json.loads(legacy.content or b"null")), json.loads(new.content or b"null")) + self.observed |= changed + return changed + + def test_starts_return_the_same_body(self): + for operation in STARTS: + assert self.changed(*self.pair(operation), 202) == set(), operation + + def test_reads_without_a_run(self): + assert self.changed(*self.pair("read_check"), 200) == {"created_at", "error"} + assert self.changed(*self.pair("read_check", params=FULL), 200) == {"created_at", "error", "output"} + assert self.changed(*self.pair("read_update"), 200) == {"status", "results"} + assert self.changed(*self.pair("read_update", params=FULL), 200) == {"status"} + + def test_reads_of_a_run_in_progress(self): + record_link_check(self.editor, self.course.id, UserTaskStatus.IN_PROGRESS) + record_link_update(self.editor, self.course.id, "Scanning") + assert self.changed(*self.pair("read_check"), 200) == {"error"} + assert self.changed(*self.pair("read_update"), 200) == {"results"} + assert self.changed(*self.pair("read_update", params=FULL), 200) == set() + + def test_a_failed_link_check(self): + record_link_check(self.editor, self.course.id, UserTaskStatus.FAILED, error=RECORDED_FAILURE) + assert self.changed(*self.pair("read_check"), 200) == {"error"} + + def test_a_successful_link_check_has_the_same_report(self): + record_link_check(self.editor, self.course.id, UserTaskStatus.SUCCEEDED, links=[]) + assert self.changed(*self.pair("read_check", params=FULL), 200) == {"error"} + + def test_a_link_update_with_a_failed_link(self): + results = [ + {"id": "1", "type": "course_updates", "original_url": "a", "new_url": "b", "success": True}, + {"id": "2", "type": "course_updates", "original_url": "c", "new_url": "c", "success": False, + "error_message": "Traceback internals"}, + ] + record_link_update(self.editor, self.course.id, UserTaskStatus.SUCCEEDED, results=results) + changed = self.changed(*self.pair("read_update", params=FULL), 200) + assert changed == {"results[0].error_message", "results[1].error_message"} + + def _error_pairs(self): + """``(legacy, new, status)`` for each refusal both addresses produce.""" + outsider = api_client(self.outsider()) + yield (*self.pair("read_check", client=api_client()), 401) + yield (*self.pair("start_update", client=api_client()), 401) + for operation in OPERATIONS: + yield (*self.pair(operation, client=outsider), 403) + for operation in STARTS: + yield (*self.pair(operation, client=outsider, course_key=UNKNOWN_COURSE_KEY), 404) + yield (*self.pair("start_update", body={"action": "neither"}), 400) + + def test_errors_differ_only_by_the_envelope(self): + for legacy, new, status in self._error_pairs(): + changed = self.changed(legacy, new, status) + assert undeclared_paths(changed, ENVELOPE_DIFFERENCES) == set(), (legacy.content, new.content) + + def test_every_declared_difference_occurs(self): + # In this order: the reads without a run come before any run is recorded. + names = [test for *_, test in BODY_DIFFERENCES] + ["test_errors_differ_only_by_the_envelope"] + for name in dict.fromkeys(names): + getattr(self, name)() + for path, *_ in BODY_DIFFERENCES + ENVELOPE_DIFFERENCES: + assert _occurs(path, self.observed), path + + +@previous_run_links(True) +class CourseOptimizerParityLegacyPermissionsTest(_ParityMixin, ModuleStoreTestCase): + """Parity for course staff, with policy-based authoring permissions off.""" + + def editor_for(self, course): + editor = UserFactory() + CourseStaffRole(course.id).add_users(editor) + return editor + + def outsider(self): + return UserFactory() + + +@previous_run_links(True) +class CourseOptimizerParityAuthzTest(_ParityMixin, CourseAuthoringAuthzTestMixin, ModuleStoreTestCase): + """Parity for a course editor role, with policy-based authoring permissions on.""" + + def editor_for(self, course): + editor = UserFactory() + self.add_user_to_role_in_course(editor, COURSE_EDITOR.external_key, course.id) + return editor + + def outsider(self): + return self.unauthorized_user + + +# --------------------------------------------------------------------------- +# Side effects of reads +# --------------------------------------------------------------------------- + +READS = [ + (operation, method, params) + for operation in ("read_check", "read_update") + for method in ("get", "head") + for params in ({}, FULL) +] + + +@contextmanager +def record_item_creations(): + """ + Record the block type of each modulestore item created inside the block. + A full link check read can create a missing course info block, so reads + are checked for creations as well as for the writes ``record_effects`` sees. + """ + created = [] + create_item = MixedModuleStore.create_item + + def spy_create_item(store, user_id, course_key, block_type, *args, **kwargs): + created.append(block_type) + return create_item(store, user_id, course_key, block_type, *args, **kwargs) + + with patch.object(MixedModuleStore, "create_item", spy_create_item): + yield created + + +class _ReadsAreSideEffectFreeMixin: + """ + Every read (both resources, both views, GET and HEAD) of runs that + succeeded writes no row and no Mongo document, creates or updates no + modulestore item and sends no signal or event. The reads start on a + database that has never served a role or permission lookup, so the first + read may insert the permission library's one policy version row and + nothing else; v0's first read writes the same. + """ + + def editor_for(self, course): + """Return a user who may read ``course``'s runs.""" + raise NotImplementedError + + def setUp(self): + super().setUp() + self.course = CourseFactory.create() + chapter = BlockFactory.create(parent_location=self.course.location, category="chapter") + sequential = BlockFactory.create(parent_location=chapter.location, category="sequential") + vertical = BlockFactory.create(parent_location=sequential.location, category="vertical") + self.html = BlockFactory.create(parent_location=vertical.location, category="html") + + def _reader_with_runs(self): + reader = self.editor_for(self.course) + links = [[str(self.html.location), "https://broken.example.com/", LinkState.BROKEN]] + record_link_check(reader, self.course.id, UserTaskStatus.SUCCEEDED, links=links) + results = [{"id": "1", "type": "course_updates", "original_url": "a", "new_url": "b", "success": True}] + record_link_update(reader, self.course.id, UserTaskStatus.SUCCEEDED, results=results) + PolicyCacheControl.objects.all().delete() + return reader + + def test_reads_have_no_side_effects(self): + client = api_client(self._reader_with_runs()) + for index, (operation, method, params) in enumerate(READS): + with record_item_creations() as created, record_effects(self) as effects: + response = call(client, operation, self.course.id, params=params) if method == "get" else \ + client.head(url(OPERATIONS[operation][1], self.course.id), params) + read = (index, operation, method, params) + assert response.status_code == 200, (read, response.content) + if index == 0: + assert effects["sql_writes"] in ([], FIRST_LOOKUP_WRITES), read + assert effects["signals"] == (FIRST_LOOKUP_SIGNALS if effects["sql_writes"] else []), read + else: + assert effects["sql_writes"] == [], read + assert effects["signals"] == [], read + assert effects["events"] == [], read + assert effects["mongo_writes"] == 0, read + assert effects["update_item"] == [], read + assert created == [], read + + def test_the_first_v0_read_writes_what_the_first_v1_read_writes(self): + reader = self._reader_with_runs() + for operation in ("read_check", "read_update"): + seen = [] + for legacy in (True, False): + PolicyCacheControl.objects.all().delete() + with record_effects(self) as effects: + response = call(api_client(reader), operation, self.course.id, legacy=legacy) + assert response.status_code == 200, response.content + seen.append((effects["sql_writes"], effects["signals"], effects["mongo_writes"])) + assert seen[0] == seen[1], operation + + def test_the_creation_recorder_sees_an_item_created(self): + """The check above would notice a creation: one made inside the recorder is seen.""" + with record_item_creations() as created: + modulestore().create_item(self.user.id, self.course.id, "course_info", block_id="probe") + assert created == ["course_info"] + + +@previous_run_links(True) +class CourseOptimizerReadSideEffectsLegacyPermissionsTest(_ReadsAreSideEffectFreeMixin, ModuleStoreTestCase): + """Reads by course staff with policy-based authoring permissions off.""" + + def editor_for(self, course): + reader = UserFactory() + CourseStaffRole(course.id).add_users(reader) + return reader + + +@previous_run_links(True) +class CourseOptimizerReadSideEffectsAuthzTest( + _ReadsAreSideEffectFreeMixin, CourseAuthoringAuthzTestMixin, ModuleStoreTestCase, +): + """ + Reads by a course editor with policy-based authoring permissions on. The + role is assigned through the permission library, which creates the policy + version row; it is deleted before the reads. + """ + + def editor_for(self, course): + reader = UserFactory() + self.add_user_to_role_in_course(reader, COURSE_EDITOR.external_key, course.id) + return reader + + +# --------------------------------------------------------------------------- +# Query counts +# --------------------------------------------------------------------------- + +class _QueryCountMixin: + """ + The default view of each read costs no more SQL queries than v0, and of a + link check fewer Mongo reads, since v0 always builds the report. The full + view costs at most one SQL query more than v0: v1 looks the course up, + which v0's reads do not. + """ + + def editor_for(self, course): + raise NotImplementedError + + def test_reads_cost_no_more_than_v0(self): + course = CourseFactory.create() + # With its overview built, the course lookup is one row read, as for a published course. + CourseOverview.get_from_id(course.id) + chapter = BlockFactory.create(parent_location=course.location, category="chapter") + sequential = BlockFactory.create(parent_location=chapter.location, category="sequential") + vertical = BlockFactory.create(parent_location=sequential.location, category="vertical") + html = BlockFactory.create(parent_location=vertical.location, category="html") + editor = self.editor_for(course) + links = [[str(html.location), "https://broken.example.com/", LinkState.BROKEN]] + record_link_check(editor, course.id, UserTaskStatus.SUCCEEDED, links=links) + results = [{"id": "1", "type": "course_updates", "original_url": "a", "new_url": "b", "success": True}] + record_link_update(editor, course.id, UserTaskStatus.SUCCEEDED, results=results) + client = api_client(editor) + for operation in ("read_check", "read_update"): + legacy = measure(lambda op=operation: call(client, op, course.id, legacy=True)) + minimal = measure(lambda op=operation: call(client, op, course.id)) + full = measure(lambda op=operation: call(client, op, course.id, params=FULL)) + assert minimal[0] <= legacy[0], (operation, minimal, legacy) + assert full[0] <= legacy[0] + 1, (operation, full, legacy) + assert minimal[1] <= full[1] <= legacy[1], (operation, minimal, full, legacy) + if operation == "read_check": + assert minimal[1] < legacy[1], (operation, minimal, legacy) + assert minimal[2] == full[2] == legacy[2] == 0, (operation, minimal, full, legacy) + + +@previous_run_links(True) +class CourseOptimizerQueryCountLegacyPermissionsTest(_QueryCountMixin, ModuleStoreTestCase): + """Costs read by course staff, with policy-based authoring permissions off.""" + + def editor_for(self, course): + editor = UserFactory() + CourseStaffRole(course.id).add_users(editor) + return editor + + +@previous_run_links(True) +class CourseOptimizerQueryCountAuthzTest(CourseAuthoringAuthzTestMixin, _QueryCountMixin, ModuleStoreTestCase): + """Costs read by a course editor, with policy-based authoring permissions on.""" + + def editor_for(self, course): + editor = UserFactory() + self.add_user_to_role_in_course(editor, COURSE_EDITOR.external_key, course.id) + return editor + + +# --------------------------------------------------------------------------- +# Addresses and methods +# --------------------------------------------------------------------------- + +COURSE_KEY = "course-v1:edX+DemoX+Demo_Course" +LEGACY_VIEW_NAMES = { + "link_check": "LinkCheckView", + "link_check_status": "LinkCheckStatusView", + "rerun_link_update": "RerunLinkUpdateView", + "rerun_link_update_status": "RerunLinkUpdateStatusView", +} + + +@ddt.ddt +@previous_run_links(True) +class CourseOptimizerRoutingTest(_PatchedTasksMixin, SharedModuleStoreTestCase): + """The addresses, the v0 addresses, and requests no route or method serves.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def test_the_addresses(self): + for name, path, viewset in ( + (LINK_CHECK, "link_check", CourseLinkCheckViewSet), + (LINK_UPDATE, "previous_run_link_update", CoursePreviousRunLinkUpdateViewSet), + ): + assert url(name, COURSE_KEY) == f"/api/authoring/v1/courses/{COURSE_KEY}/{path}/" + match = resolve(url(name, COURSE_KEY)) + assert match.func.cls is viewset + assert routed_actions(match.func) == {"get": "retrieve", "post": "create"} + + def test_the_v0_addresses_are_unchanged(self): + for name, view_name in LEGACY_VIEW_NAMES.items(): + assert legacy_url(name, COURSE_KEY) == f"/api/contentstore/v0/{name}/{COURSE_KEY}" + view = resolve(legacy_url(name, COURSE_KEY)).func.cls + assert (view.__module__, view.__name__) == (LEGACY_VIEWS, view_name) + + def test_v0_still_takes_a_slash_separated_key_that_v1_does_not(self): + view = resolve("/api/contentstore/v0/link_check/edX/DemoX/Demo_Course").func.cls + assert (view.__module__, view.__name__) == (LEGACY_VIEWS, "LinkCheckView") + for path in ("link_check", "previous_run_link_update"): + with pytest.raises(Resolver404): + resolve(f"/api/authoring/v1/courses/edX/DemoX/Demo_Course/{path}/") + + def test_the_url_names_are_unique_in_the_namespace(self): + # Imported here: at module import the URL converters are not registered yet. + from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls # noqa: PLC0415 + names = [pattern.name for pattern in authoring_urls.urlpatterns] + assert len(names) == len(set(names)) + assert {"course_link_check", "course_previous_run_link_update"} <= set(names) + + # The error library does not catalogue MethodNotAllowed or NotAcceptable, so + # these answer with the envelope typed internal. Change the expected type + # here once it does. + + @ddt.data(*[(name, method) for name in (LINK_CHECK, LINK_UPDATE) for method in ("put", "patch", "delete")]) + @ddt.unpack + def test_other_methods_are_not_allowed(self, name, method): + response = getattr(api_client(UserFactory(is_staff=True)), method)(url(name, self.course.id), {}, format="json") + assert_error_envelope(response, expected_status=405, expected_type_slug="internal") + + @ddt.data(LINK_CHECK, LINK_UPDATE) + def test_unsupported_accept_is_refused_with_406(self, name): + response = api_client(UserFactory(is_staff=True)).get(url(name, self.course.id), HTTP_ACCEPT="application/xml") + assert_error_envelope(response, expected_status=406, expected_type_slug="internal") + + # Nor ParseError or UnsupportedMediaType: a malformed or unsupported body is + # answered with its own status but typed internal. Change the expected type + # here once the library catalogues them. + + def test_a_malformed_json_body_is_a_400_typed_internal(self): + response = api_client(UserFactory(is_staff=True)).post( + url(LINK_UPDATE, self.course.id), data="{", content_type="application/json", + ) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="internal") + assert envelope["title"] == "Internal Server Error" + self.delay("v1", "start_update").assert_not_called() + + def test_an_unsupported_media_type_is_a_415_typed_internal(self): + response = api_client(UserFactory(is_staff=True)).post( + url(LINK_UPDATE, self.course.id), data="action=all", content_type="text/plain", + ) + envelope = assert_error_envelope(response, expected_status=415, expected_type_slug="internal") + assert envelope["title"] == "Internal Server Error" + self.delay("v1", "start_update").assert_not_called() + + # These addresses match no route, so the service's HTML 404 page answers + # them. Change these to the error envelope once a catch-all route under the + # mount answers unknown addresses. + + @ddt.data(*[(key, path) for key in ("not-a-course-key", "edX/DemoX/Demo_Course", "edX+DemoX+Demo_Course") + for path in ("link_check", "previous_run_link_update")]) + @ddt.unpack + def test_an_unparsable_or_slash_form_key_is_an_html_404(self, key, path): + response = api_client(UserFactory(is_staff=True)).get(f"/api/authoring/v1/courses/{key}/{path}/") + assert response.status_code == 404 + assert response["Content-Type"].startswith("text/html") + + def test_the_address_without_its_trailing_slash_redirects_to_it(self): + """No route answers the address without its slash; the service redirects a read to the slashed one.""" + address = f"/api/authoring/v1/courses/{self.course.id}/link_check" + response = api_client(UserFactory(is_staff=True)).get(address) + assert response.status_code == 301 + assert response["Location"] == url(LINK_CHECK, self.course.id) + with pytest.raises(Resolver404): + resolve(address) + + +# --------------------------------------------------------------------------- +# The published schema +# --------------------------------------------------------------------------- + +LINK_CHECK_PATH = "/api/authoring/v1/courses/{course_key}/link_check/" +LINK_UPDATE_PATH = "/api/authoring/v1/courses/{course_key}/previous_run_link_update/" +LEGACY_PATHS = { + "/api/contentstore/v0/link_check/{course_id}": ("post", "v0_link_check_create"), + "/api/contentstore/v0/link_check_status/{course_id}": ("get", "v0_link_check_status_retrieve"), + "/api/contentstore/v0/rerun_link_update/{course_id}": ("post", "v0_rerun_link_update_create"), + "/api/contentstore/v0/rerun_link_update_status/{course_id}": ("get", "v0_rerun_link_update_status_retrieve"), +} +NEW_COMPONENTS = ( + "CourseOptimizerQueuedRun.v1", + "LinkCheck.v1", + "LinkCheckReport.v1", + "LinkCheckReportSection.v1", + "LinkCheckReportSubsection.v1", + "LinkCheckReportUnit.v1", + "LinkCheckReportBlock.v1", + "LinkCheckPreviousRunLink.v1", + "PreviousRunLinkUpdate.v1", + "LinkUpdateResult.v1", + "PreviousRunLinkUpdateRequest.v1", + "LinkToUpdate.v1", +) + + +class CourseOptimizerSchemaTest(TestCase): + """The four operations as published in the CMS authoring schema.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.schema = published_schema() + cls.components = cls.schema["components"]["schemas"] + + @staticmethod + def _ref(node): + return node["content"]["application/json"]["schema"]["$ref"].rsplit("/", 1)[-1] + + @staticmethod + def _enum_ref(node): + """The enum component a property refers to; a described property wraps the reference in allOf.""" + return node.get("$ref") or node["allOf"][0]["$ref"] + + def _enum(self, node): + return set(resolve_ref(self.schema, {"$ref": self._enum_ref(node)})["enum"]) + + def test_the_operations_are_published_for_the_sdk(self): + for path, name, ok in ( + (LINK_CHECK_PATH, "link_check", "LinkCheck.v1"), + (LINK_UPDATE_PATH, "previous_run_link_update", "PreviousRunLinkUpdate.v1"), + ): + operations = self.schema["paths"][path] + assert sorted(operations) == ["get", "post"] + assert operations["get"]["operationId"] == f"v1_courses_{name}_retrieve" + assert operations["post"]["operationId"] == f"v1_courses_{name}_create" + for operation in operations.values(): + assert operation["tags"] == ["openedx-platform-sdk"] + assert "deprecated" not in operation + assert self._ref(operations["get"]["responses"]["200"]) == ok + assert self._ref(operations["post"]["responses"]["202"]) == "CourseOptimizerQueuedRun.v1" + assert "200" not in operations["post"]["responses"] + for method, statuses in (("get", ("400", "401", "403", "404")), ("post", ("401", "403", "404"))): + for status in statuses: + assert self._ref(operations[method]["responses"][status]) == "ErrorResponse", (path, status) + assert self._ref(self.schema["paths"][LINK_UPDATE_PATH]["post"]["responses"]["400"]) == "ErrorResponse" + + def test_the_read_parameters(self): + for path in (LINK_CHECK_PATH, LINK_UPDATE_PATH): + parameters = {p["name"]: p for p in self.schema["paths"][path]["get"]["parameters"]} + assert set(parameters) == {"course_key", "view"} + assert parameters["course_key"]["in"] == "path" + assert parameters["course_key"]["schema"] == {"type": "string", "pattern": "^[^/]+$"} + assert parameters["view"]["in"] == "query" + assert not parameters["view"].get("required") + assert parameters["view"]["schema"] == {"type": "string", "enum": ["full", "minimal"], "default": "minimal"} + + def test_starting_a_link_check_takes_no_body(self): + assert "requestBody" not in self.schema["paths"][LINK_CHECK_PATH]["post"] + + def test_starting_a_link_update_takes_an_action_and_links(self): + body = self.schema["paths"][LINK_UPDATE_PATH]["post"]["requestBody"] + assert list(body["content"]) == ["application/json"] + assert self._ref(body) == "PreviousRunLinkUpdateRequest.v1" + request = self.components["PreviousRunLinkUpdateRequest.v1"] + assert request["required"] == ["action"] + assert self._enum(request["properties"]["action"]) == {"all", "single"} + assert request["properties"]["data"]["items"] == {"$ref": "#/components/schemas/LinkToUpdate.v1"} + assert sorted(self.components["LinkToUpdate.v1"]["required"]) == ["id", "type", "url"] + + def test_the_states_are_one_published_enum(self): + """Both reads and both starts publish the same states, as one component.""" + nodes = [ + self.components["LinkCheck.v1"]["properties"]["status"], + self.components["PreviousRunLinkUpdate.v1"]["properties"]["status"], + self.components["CourseOptimizerQueuedRun.v1"]["properties"]["status"], + ] + assert [self._enum(node) for node in nodes] == [set(RUN_STATES)] * 3 + assert {self._enum_ref(node) for node in nodes} == {"#/components/schemas/StatusEnum"} + + def test_the_minimal_and_full_members(self): + link_check = self.components["LinkCheck.v1"] + assert set(link_check["properties"]) == {"status", "created_at", "error", "output"} + assert sorted(link_check["required"]) == ["created_at", "error", "status"] + nullable = {name for name, field in link_check["properties"].items() if field.get("nullable")} + assert nullable == {"created_at", "error", "output"} + link_update = self.components["PreviousRunLinkUpdate.v1"] + assert set(link_update["properties"]) == {"status", "results"} + assert link_update["required"] == ["status"] + result = self.components["LinkUpdateResult.v1"] + nullable = {name for name, field in result["properties"].items() if field.get("nullable")} + assert nullable == {"original_url", "error_message"} + + def test_the_link_types_are_documented(self): + description = self.components["LinkToUpdate.v1"]["properties"]["type"]["description"] + for link_type in ("course_content", "course_updates", "handouts", "custom_pages"): + assert f"'{link_type}'" in description, link_type + + def test_every_member_is_described(self): + for name in NEW_COMPONENTS: + component = self.components[name] + assert component.get("description"), name + properties = component.get("properties", {}) + undescribed = [field for field, spec in properties.items() if not spec.get("description")] + assert undescribed == [], name + + def test_the_examples_are_valid_bodies(self): + content = self.schema["paths"][LINK_UPDATE_PATH]["post"]["requestBody"]["content"]["application/json"] + assert sorted(content["examples"]) == ["EveryLink", "SomeLinks"] + for example in content["examples"].values(): + assert_matches_published_schema(example["value"], "PreviousRunLinkUpdateRequest.v1") + content = self.schema["paths"][LINK_CHECK_PATH]["get"]["responses"]["200"]["content"]["application/json"] + assert sorted(content["examples"]) == ["FullReport"] + report = content["examples"]["FullReport"]["value"] + assert_matches_published_schema(report, "LinkCheck.v1") + assert set(report) == {"status", "created_at", "error", "output"} + + def test_the_v0_operations_are_deprecated_and_left_out_of_the_sdk(self): + for path, (method, operation_id) in LEGACY_PATHS.items(): + operations = self.schema["paths"][path] + assert sorted(operations) == [method], path + assert operations[method]["operationId"] == operation_id + assert operations[method]["deprecated"] is True + assert "openedx-platform-sdk" not in operations[method].get("tags", []) + + def test_only_superseded_or_migrated_addresses_are_deprecated(self): + """No operation is marked deprecated except by the legacy-address lists, so a stray mark is caught.""" + deprecated = { + (path, method) + for path, operations in self.schema["paths"].items() + for method, operation in operations.items() + if isinstance(operation, dict) and operation.get("deprecated") + } + assert {(path, method) for path, (method, _) in LEGACY_PATHS.items()} <= deprecated + assert {(path, method) for path in (LINK_CHECK_PATH, LINK_UPDATE_PATH) for method in ("get", "post")} & \ + deprecated == set() + stray = sorted( + (path, method) for path, method in deprecated + if path not in SUPERSEDED_PATHS and not path.startswith(LEGACY_MIGRATED_PATH_PREFIXES) + ) + assert stray == [] From 4386a5cc75de90971480940cc11c14fd13f67175 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Sun, 25 Oct 2026 22:05:35 +0500 Subject: [PATCH 9/9] docs: mark the v0 course optimizer deprecated in favour of authoring v1 The four v0 course optimizer operations now have conforming successors under /api/authoring/v1/courses/{course_key}/. They are marked deprecated in the CMS schema through exact SUPERSEDED_PATHS entries, and the views' docstrings name the successors and the deprecation ticket. The views themselves are unchanged. --- .../rest_api/v0/views/course_optimizer.py | 29 +++++++++++++++++++ cms/lib/spectacular.py | 6 ++++ cms/lib/tests/test_spectacular.py | 24 +++++++++++++++ 3 files changed, 59 insertions(+) diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py b/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py index 2af5f148b49c..586a873cd85c 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py @@ -33,6 +33,9 @@ class LinkCheckView(DeveloperErrorViewMixin, APIView): """ View for queueing a celery task to scan a course for broken links. + + Deprecated: use ``POST /api/authoring/v1/courses/{course_key}/link_check/`` instead. + See https://github.com/openedx/openedx-platform/issues/39128. """ @extend_schema( parameters=[ @@ -50,6 +53,10 @@ def post(self, request: Request, course_id: str): """ Queue celery task to scan a course for broken links. + Deprecated: use ``POST /api/authoring/v1/courses/{course_key}/link_check/``, which answers + 202 with ``{"status": "Pending"}``. + See https://github.com/openedx/openedx-platform/issues/39128. + **Example Request** POST /api/contentstore/v0/link_check/{course_id} @@ -77,6 +84,9 @@ def post(self, request: Request, course_id: str): class LinkCheckStatusView(DeveloperErrorViewMixin, APIView): """ View for checking the status of the celery task and returning the results. + + Deprecated: use ``GET /api/authoring/v1/courses/{course_key}/link_check/`` instead. + See https://github.com/openedx/openedx-platform/issues/39128. """ @extend_schema( parameters=[ @@ -103,6 +113,11 @@ def get(self, request: Request, course_id: str): This function adds a status for when status from UserTaskStatus is None: 'Uninitiated' + Deprecated: use ``GET /api/authoring/v1/courses/{course_key}/link_check/``. Its default + view returns ``status``, ``created_at`` and ``error``; ``view=full`` adds ``output``, the + report this endpoint returns, with snake_case member names. + See https://github.com/openedx/openedx-platform/issues/39128. + **Example Request** GET /api/contentstore/v0/link_check_status/{course_id} @@ -233,6 +248,9 @@ def get(self, request: Request, course_id: str): class RerunLinkUpdateView(DeveloperErrorViewMixin, APIView): """ View for queueing a celery task to update course links to the latest re-run. + + Deprecated: use ``POST /api/authoring/v1/courses/{course_key}/previous_run_link_update/`` instead. + See https://github.com/openedx/openedx-platform/issues/39128. """ @extend_schema( @@ -255,6 +273,10 @@ def post(self, request: Request, course_id: str): """ Queue celery task to update course links to the latest re-run. + Deprecated: use ``POST /api/authoring/v1/courses/{course_key}/previous_run_link_update/``, + which takes this body and answers 202 with ``{"status": "Pending"}``. + See https://github.com/openedx/openedx-platform/issues/39128. + **Example Request - Update All Links** POST /api/contentstore/v0/rerun_link_update/{course_id} ```json @@ -341,6 +363,9 @@ def post(self, request: Request, course_id: str): class RerunLinkUpdateStatusView(DeveloperErrorViewMixin, APIView): """ View for checking the status of the course link update task and returning the results. + + Deprecated: use ``GET /api/authoring/v1/courses/{course_key}/previous_run_link_update/`` instead. + See https://github.com/openedx/openedx-platform/issues/39128. """ @extend_schema( @@ -367,6 +392,10 @@ def get(self, request: Request, course_id: str): Possible statuses: 'pending', 'in_progress', 'completed', 'failed', 'uninitiated' + Deprecated: use ``GET /api/authoring/v1/courses/{course_key}/previous_run_link_update/``. + Its default view returns ``status``; ``view=full`` adds ``results``. + See https://github.com/openedx/openedx-platform/issues/39128. + **Example Request** GET /api/contentstore/v0/rerun_link_update_status/{course_id} diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index b4292d975e96..04d6a501d554 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -15,6 +15,12 @@ "/api/contentstore/v0/youtube_transcripts/{course_id}/upload", # → /api/authoring/v1/courses/{course_key}/advanced_settings/ "/api/contentstore/v0/advanced_settings/{course_id}", + # → /api/authoring/v1/courses/{course_key}/link_check/ + "/api/contentstore/v0/link_check/{course_id}", + "/api/contentstore/v0/link_check_status/{course_id}", + # → /api/authoring/v1/courses/{course_key}/previous_run_link_update/ + "/api/contentstore/v0/rerun_link_update/{course_id}", + "/api/contentstore/v0/rerun_link_update_status/{course_id}", ) # Legacy addresses of APIs migrated to /api/authoring/, marked deprecated for diff --git a/cms/lib/tests/test_spectacular.py b/cms/lib/tests/test_spectacular.py index ba7732a260e6..1c22feb3ccc6 100644 --- a/cms/lib/tests/test_spectacular.py +++ b/cms/lib/tests/test_spectacular.py @@ -77,6 +77,10 @@ def test_both_spellings_of_each_superseded_path_are_listed(self): "/api/contentstore/v0/youtube_transcripts/{course_id}/uploa", "/api/contentstore/v0/youtube_transcripts/{course_id}/upload", "/api/contentstore/v0/advanced_settings/{course_id}", + "/api/contentstore/v0/link_check/{course_id}", + "/api/contentstore/v0/link_check_status/{course_id}", + "/api/contentstore/v0/rerun_link_update/{course_id}", + "/api/contentstore/v0/rerun_link_update_status/{course_id}", } def test_the_conforming_paths_are_left_unmarked(self): @@ -94,6 +98,26 @@ def test_the_advanced_settings_operations_are_marked_and_their_successor_is_not( assert result["paths"][legacy] == {"get": {"deprecated": True}, "patch": {"deprecated": True}} assert result["paths"][conforming] == {"get": {}, "patch": {}} + def test_the_course_optimizer_operations_are_marked_and_their_successors_are_not(self): + legacy = { + "/api/contentstore/v0/link_check/{course_id}": "post", + "/api/contentstore/v0/link_check_status/{course_id}": "get", + "/api/contentstore/v0/rerun_link_update/{course_id}": "post", + "/api/contentstore/v0/rerun_link_update_status/{course_id}": "get", + } + conforming = ( + "/api/authoring/v1/courses/{course_key}/link_check/", + "/api/authoring/v1/courses/{course_key}/previous_run_link_update/", + ) + result = self._run({"paths": { + **{path: {method: {}} for path, method in legacy.items()}, + **{path: {"get": {}, "post": {}} for path in conforming}, + }}) + for path, method in legacy.items(): + assert result["paths"][path] == {method: {"deprecated": True}}, path + for path in conforming: + assert result["paths"][path] == {"get": {}, "post": {}}, path + def test_an_unrelated_contentstore_path_is_left_unmarked(self): other = "/api/contentstore/v0/heartbeat" result = self._run({"paths": {other: {"get": {}}}})