From 4ce6f362aae368332f1f33ff9aaea82ab12d4d79 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 13 Oct 2026 23:40:23 +0500 Subject: [PATCH 1/2] feat: read and partially update course grading through authoring v3 Authoring v3 could only write the grading policy, so every caller still read it through v1. v3 now answers GET with the four-field policy, and with ?view=full adds v1's five derived fields under their v1 names. PATCH validates its body and applies partial updates. A body with all four keys takes exactly the previous path. Other bodies change only the fields sent and never re-send graders, so stored weights cannot drift. Bodies that used to fail with 500, some after writing corrupt data, now get 400 with nothing written. Permissions move into per-action classes with the same checks, fallback and 404-before-403 order. v1 and v0 grading are marked deprecated in the CMS schema. Co-Authored-By: Claude Opus 5.5 --- .../rest_api/v3/authoring_urls.py | 2 +- .../rest_api/v3/grading_service.py | 83 + .../contentstore/rest_api/v3/permissions.py | 45 + .../rest_api/v3/serializers/__init__.py | 1 + .../rest_api/v3/serializers/grading.py | 449 +++++ .../rest_api/v3/views/authoring_grading.py | 280 ++- .../v3/views/tests/test_authoring_grading.py | 1569 +++++++++++++---- cms/lib/spectacular.py | 35 +- cms/lib/tests/test_spectacular.py | 179 ++ 9 files changed, 2193 insertions(+), 450 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v3/grading_service.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/permissions.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/serializers/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v3/serializers/grading.py diff --git a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py index 2d1db8f88434..86dd93e580b0 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py @@ -29,7 +29,7 @@ ), path( "courses//grading/", - AuthoringGradingViewSet.as_view({"patch": "partial_update"}), + AuthoringGradingViewSet.as_view({"get": "retrieve", "patch": "partial_update"}), name="course_grading", ), ] diff --git a/cms/djangoapps/contentstore/rest_api/v3/grading_service.py b/cms/djangoapps/contentstore/rest_api/v3/grading_service.py new file mode 100644 index 000000000000..d52f96faf957 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/grading_service.py @@ -0,0 +1,83 @@ +"""Service layer for the v3 course grading endpoint.""" + +from django.conf import settings + +from cms.djangoapps.contentstore.rest_api.v3.serializers.grading import GRADING_POLICY_FIELDS +from cms.djangoapps.contentstore.utils import get_course_grading +from cms.djangoapps.models.settings.course_grading import CourseGradingModel +from openedx.core.djangoapps.credit.api import is_credit_course +from openedx.core.djangoapps.credit.tasks import update_credit_course_requirements +from xmodule.modulestore.django import modulestore + + +def get_grading_policy(course_key): + """Return the stored grading policy of the course ``course_key``.""" + return CourseGradingModel.fetch(course_key) + + +def get_full_grading_settings(course_key): + """ + Return the grading policy of ``course_key`` with the context the Studio grading page shows beside it. + + The returned mapping holds the four policy fields plus the graded subsections per + assignment type, the default letter grade names, and the credit and proctoring context. + """ + with modulestore().bulk_operations(course_key): + show_credit_eligibility = is_credit_course(course_key) and settings.ENABLE_CREDIT_ELIGIBILITY + context = get_course_grading(course_key) + policy = context["course_details"] + return { + "graders": policy.graders, + "grade_cutoffs": policy.grade_cutoffs, + "grace_period": policy.grace_period, + "minimum_grade_credit": policy.minimum_grade_credit, + "course_assignment_lists": context["course_assignment_lists"], + "default_grade_designations": context["default_grade_designations"], + "is_credit_course": context["is_credit_course"], + "show_credit_eligibility": show_credit_eligibility, + "mfe_proctored_exam_settings_url": context["mfe_proctored_exam_settings_url"], + } + + +def update_grading_policy(course_key, changes, user): + """ + Apply ``changes`` to the grading policy of ``course_key`` and return the stored policy. + + Only the fields present in ``changes`` are written. A body holding all four fields is + applied exactly as a whole-policy update. When ``graders`` is absent the stored graders + are never written back: their weights are fractions that do not survive a round trip + through the percentages the API exchanges, so rewriting them would alter weights + nobody changed. Grade cutoffs are written, and a grading policy change announced, + only when they differ from the stored ones. + + The credit requirements of the course are refreshed whenever ``minimum_grade_credit`` + is present, whether or not its value changes. + """ + if "minimum_grade_credit" in changes: + update_credit_course_requirements.delay(str(course_key)) + + if all(field in changes for field in GRADING_POLICY_FIELDS): + return CourseGradingModel.update_from_json(course_key, changes, user) + + if "graders" in changes: + # The whole-policy update needs every field. The stored cutoffs leave them + # unchanged, and None leaves the grace period and credit minimum unchanged. + stored_cutoffs = modulestore().get_course(course_key).grade_cutoffs + policy = { + "grade_cutoffs": stored_cutoffs, + "grace_period": None, + "minimum_grade_credit": None, + **changes, + } + return CourseGradingModel.update_from_json(course_key, policy, user) + + if "grace_period" in changes: + CourseGradingModel.update_grace_period_from_json(course_key, changes["grace_period"], user) + if "minimum_grade_credit" in changes: + CourseGradingModel.update_minimum_grade_credit_from_json(course_key, changes["minimum_grade_credit"], user) + # Last, so the grading policy change is announced after every other field is stored, + # as the whole-policy update does. + if "grade_cutoffs" in changes: + if modulestore().get_course(course_key).grade_cutoffs != changes["grade_cutoffs"]: + CourseGradingModel.update_cutoffs_from_json(course_key, changes["grade_cutoffs"], user) + return CourseGradingModel.fetch(course_key) diff --git a/cms/djangoapps/contentstore/rest_api/v3/permissions.py b/cms/djangoapps/contentstore/rest_api/v3/permissions.py new file mode 100644 index 000000000000..a0f38f306c48 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/permissions.py @@ -0,0 +1,45 @@ +"""Permissions for the v3 contentstore API.""" + +from openedx_authz.constants.permissions import COURSES_EDIT_GRADING_SETTINGS, COURSES_VIEW_GRADING_SETTINGS +from rest_framework.permissions import BasePermission + +from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission +from openedx.core.djangoapps.authz.decorators import user_has_course_permission + + +class _GradingSettingsPermission(BasePermission): + """ + Allow callers holding ``authz_permission`` on the course in the path. + + The course comes from the view's ``get_course_key()``, which refuses an + unparsable or unknown course with a 404 before permission is checked. On + courses using policy-based authoring permissions that policy decides; + elsewhere Studio read access to the course is required. + """ + + authz_permission = None + + def has_permission(self, request, view): + return user_has_course_permission( + request.user, + self.authz_permission, + view.get_course_key(), + LegacyAuthoringPermission.READ, + ) + + +class CanViewGradingSettings(_GradingSettingsPermission): + """Allow callers who may view the grading settings of the course in the path.""" + + authz_permission = COURSES_VIEW_GRADING_SETTINGS.identifier + + +class CanEditGradingSettings(_GradingSettingsPermission): + """ + Allow callers who may edit the grading settings of the course in the path. + + Where policy-based permissions are off, Studio read access is enough to + edit, as for every other grading endpoint in Studio. + """ + + authz_permission = COURSES_EDIT_GRADING_SETTINGS.identifier diff --git a/cms/djangoapps/contentstore/rest_api/v3/serializers/__init__.py b/cms/djangoapps/contentstore/rest_api/v3/serializers/__init__.py new file mode 100644 index 000000000000..205123e14271 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/serializers/__init__.py @@ -0,0 +1 @@ +"""Serializers for the v3 contentstore API.""" diff --git a/cms/djangoapps/contentstore/rest_api/v3/serializers/grading.py b/cms/djangoapps/contentstore/rest_api/v3/serializers/grading.py new file mode 100644 index 000000000000..f5ae93462940 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/serializers/grading.py @@ -0,0 +1,449 @@ +"""Serializers for a course's grading policy.""" + +import math +from collections.abc import Mapping +from datetime import timedelta + +from django.core.validators import MinLengthValidator +from drf_spectacular.types import OpenApiTypes +from drf_spectacular.utils import extend_schema_field +from rest_framework import serializers +from rest_framework.exceptions import ValidationError +from rest_framework.settings import api_settings +from rest_framework.utils import html + +GRADING_FULL_VIEW = "full" + + +class GraderSerializer(serializers.Serializer): # pylint: disable=abstract-method + """One assignment type of a course's grading policy.""" + + type = serializers.CharField( + allow_null=True, + help_text="Name of the assignment type, for example Homework, or null when the policy stores none.", + ) + min_count = serializers.IntegerField(help_text="Number of assignments of this type the course expects.") + drop_count = serializers.IntegerField( + help_text="Number of lowest-scoring assignments of this type left out of the grade.", + ) + short_label = serializers.CharField( + allow_null=True, + allow_blank=True, + help_text="Abbreviation shown with the assignment type, for example HW.", + ) + weight = serializers.IntegerField( + help_text=( + "Share of the course grade, in percent. The stored fraction is multiplied by 100 " + "and truncated to a whole number." + ), + ) + id = serializers.IntegerField(help_text="Position of the assignment type in the policy, starting at 0.") + + class Meta: + ref_name = "authoring_grading.Grader.v3" + + +class GracePeriodSerializer(serializers.Serializer): # pylint: disable=abstract-method + """How long after a due date a submission is still accepted.""" + + hours = serializers.IntegerField(help_text="Whole hours, with whole days counted as 24 hours.") + minutes = serializers.IntegerField(help_text="Minutes beyond the whole hours.") + seconds = serializers.IntegerField(help_text="Seconds beyond the whole minutes.") + + class Meta: + ref_name = "authoring_grading.GracePeriod.v3" + + +class CourseGradingSerializer(serializers.Serializer): # pylint: disable=abstract-method + """The grading policy of a course.""" + + graders = GraderSerializer( + many=True, + allow_empty=True, + help_text="The assignment types, in stored order.", + ) + grade_cutoffs = serializers.DictField( + child=serializers.FloatField(allow_null=True), + allow_null=True, + help_text=( + "Lowest score, from 0 to 1, that earns each letter grade, keyed by the grade's name. " + "The cutoffs, or a single cutoff, are null when the policy was stored that way." + ), + ) + grace_period = GracePeriodSerializer( + allow_null=True, + help_text="Grace period after each due date, or null when the course has none.", + ) + minimum_grade_credit = serializers.FloatField( + allow_null=True, + help_text="Lowest course score, from 0 to 1, that earns course credit, or null when none is set.", + ) + + class Meta: + ref_name = "authoring_grading.CourseGrading.v3" + + +class CourseGradingFullSerializer(CourseGradingSerializer): # pylint: disable=abstract-method + """The grading policy of a course, with the context the Studio grading page shows beside it.""" + + course_assignment_lists = serializers.DictField( + child=serializers.ListField(child=serializers.CharField()), + help_text=( + "For each assignment type in use, its graded subsections as '
- '. " + "Present only with view=full." + ), + ) + default_grade_designations = serializers.ListField( + child=serializers.CharField(), + help_text="Letter grade names offered for new grade cutoffs. Present only with view=full.", + ) + is_credit_course = serializers.BooleanField( + help_text="Whether the course offers course credit. Present only with view=full.", + ) + show_credit_eligibility = serializers.BooleanField( + help_text=( + "Whether the minimum grade for credit applies: true for a credit course when credit " + "eligibility is enabled on the platform. Present only with view=full." + ), + ) + mfe_proctored_exam_settings_url = serializers.CharField( + allow_blank=True, + help_text=( + "Path of the course's proctored exam settings page in the authoring app, or an empty " + "string when that page is not available. Present only with view=full." + ), + ) + + class Meta: + ref_name = "authoring_grading.CourseGradingFull.v3" + + +class GradingViewQuerySerializer(serializers.Serializer): # pylint: disable=abstract-method + """Query parameters of a grading policy read.""" + + view = serializers.ChoiceField( + choices=[GRADING_FULL_VIEW], + required=False, + help_text=( + "Set to full to add the graded subsections per assignment type, the default letter " + "grade names, and the credit and proctoring context. Omit it for the policy alone." + ), + ) + + +# The request fields below check each value with the conversion the grading +# model applies when it reads it, and the view then hands the model the body as +# sent. A value the model cannot convert or store is refused before anything is +# written. +# +# A body holding every policy field goes to the model's whole-policy update, +# which stores some values outside the documented shape: null or empty cutoffs, +# a grader type of any JSON type, an empty credit minimum. Such a body is +# validated by _WholeGradingPolicyUpdateSerializer, which accepts them. Any other +# body is held to the documented shape, because the model's per-field updates +# store null or empty cutoffs where they break the course's grading. + +GRADING_POLICY_FIELDS = ("graders", "grade_cutoffs", "grace_period", "minimum_grade_credit") + +# The modulestore stores integers in at most 8 bytes and fails part-way through +# a write on a wider one. +_STORABLE_INT_RANGE = range(-(2 ** 63), 2 ** 63) + + +def _storable(value): + """Whether the modulestore can store ``value``, at any depth.""" + if isinstance(value, bool): + return True + if isinstance(value, int): + return value in _STORABLE_INT_RANGE + if isinstance(value, Mapping): + return all(_storable(item) for item in value.values()) + if isinstance(value, list): + return all(_storable(item) for item in value) + return True + + +class _SentValueField(serializers.Field): + """A request value passed to the grading model as sent, after checking it converts and stores.""" + + convert = None + # False where the model stores only the converted value, or nothing. + stored_as_sent = True + default_error_messages = {"invalid": "Enter a valid value."} + + def to_internal_value(self, data): + if self.convert is not None: + try: + converted = self.convert(data) # pylint: disable=not-callable + except (TypeError, ValueError, OverflowError): + self.fail("invalid") + if isinstance(converted, float) and not math.isfinite(converted): + self.fail("invalid") + if not _storable(converted): + self.fail("invalid") + if self.stored_as_sent and not _storable(data): + self.fail("invalid") + return data + + def to_representation(self, value): + return value + + +@extend_schema_field(OpenApiTypes.STR) +class _NameField(_SentValueField): + """A name, which must be a string.""" + + default_error_messages = {"invalid": "A string is required."} + + def to_internal_value(self, data): + if not isinstance(data, str): + self.fail("invalid") + return super().to_internal_value(data) + + +@extend_schema_field(OpenApiTypes.STR) +class _TextField(_SentValueField): + """A label the grading model stores as sent, whatever its type.""" + + def validate_empty_values(self, data): + # The grading model stores null here, so it is accepted, though not published. + if data is None: + return (True, None) + return super().validate_empty_values(data) + + +@extend_schema_field(OpenApiTypes.INT) +class _CountField(_SentValueField): + """A count the grading model reads with ``int()``.""" + + convert = int + stored_as_sent = False + default_error_messages = {"invalid": "A whole number is required."} + + +@extend_schema_field(OpenApiTypes.DOUBLE) +class _NumberField(_SentValueField): + """A number the grading model reads with ``float()``.""" + + convert = float + default_error_messages = {"invalid": "A finite number is required."} + + +@extend_schema_field(OpenApiTypes.DOUBLE) +class _ScoreField(_NumberField): + """A score, which must be a JSON number.""" + + def to_internal_value(self, data): + if isinstance(data, bool) or not isinstance(data, (int, float)): + self.fail("invalid") + return super().to_internal_value(data) + + +@extend_schema_field(OpenApiTypes.DOUBLE) +class _CreditMinimumField(_NumberField): + """A credit minimum; an empty string is stored as no minimum.""" + + def to_internal_value(self, data): + if data == "": + return data + return super().to_internal_value(data) + + +@extend_schema_field(OpenApiTypes.INT) +class _PercentField(_NumberField): + """A whole percentage; the grading model also stores a fractional one as sent.""" + + stored_as_sent = False + + +@extend_schema_field(OpenApiTypes.INT) +class _GraderIdField(_TextField): + """A grader id, which the grading model ignores whatever its value.""" + + stored_as_sent = False + + +class GracePeriodUpdateSerializer(serializers.Serializer): # pylint: disable=abstract-method + """A grace period in an update. A unit left out counts as 0.""" + + hours = serializers.IntegerField(default=0, help_text="Whole hours.") + minutes = serializers.IntegerField(default=0, help_text="Minutes beyond the whole hours.") + seconds = serializers.IntegerField(default=0, help_text="Seconds beyond the whole minutes.") + + class Meta: + ref_name = "authoring_grading.GracePeriodUpdate.v3" + + +@extend_schema_field(GracePeriodUpdateSerializer) +class _GracePeriodField(serializers.Field): + """ + A grace period, in any of the units ``datetime.timedelta`` accepts. + + An object wrapped in a ``grace_period`` key is unwrapped first, as the + grading model does. + """ + + default_error_messages = { + "invalid": "Enter the grace period as an object of hours, minutes and seconds.", + } + + def to_internal_value(self, data): + try: + period = data["grace_period"] if "grace_period" in data else data + timedelta(**period) + except (TypeError, ValueError, OverflowError): + self.fail("invalid") + return data + + def to_representation(self, value): + return value + + +class _AnyGradeCutoffsField(serializers.DictField): + """Letter-grade cutoffs as the whole-policy update stores them: null, or an object of numbers or nulls.""" + + def __init__(self, **kwargs): + super().__init__(child=_NumberField(allow_null=True), **kwargs) + + def validate_empty_values(self, data): + if data is None: + return (True, None) + return super().validate_empty_values(data) + + +class _GraderListSerializer(serializers.ListSerializer): # pylint: disable=abstract-method + """ + Graders as a list. + + The grading model iterates whatever it is given, so it stores an empty + string or an empty object as an empty list, and both are accepted. + Anything else that is not a list is refused. + """ + + def to_internal_value(self, data): + if isinstance(data, (str, Mapping)): + data = list(data) + return super().to_internal_value(data) + + +class GraderUpdateSerializer(serializers.Serializer): # pylint: disable=abstract-method + """One assignment type in a grading policy update.""" + + type = _NameField(help_text="Name of the assignment type, for example Homework.") + min_count = _CountField( + required=False, + help_text="Number of assignments of this type the course expects. Defaults to 0.", + ) + drop_count = _CountField( + required=False, + help_text="Number of lowest-scoring assignments of this type left out of the grade. Defaults to 0.", + ) + short_label = _TextField( + required=False, + allow_null=True, + help_text="Abbreviation shown with the assignment type, for example HW.", + ) + weight = _PercentField( + required=False, + help_text="Share of the course grade, in percent. Defaults to 0.", + ) + id = _GraderIdField( + required=False, + help_text=( + "Ignored: a grader's position in the list is its id. Accepted so that graders " + "read from this endpoint can be sent back unchanged." + ), + ) + + class Meta: + ref_name = "authoring_grading.GraderUpdate.v3" + list_serializer_class = _GraderListSerializer + + +class CourseGradingUpdateSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + A change to a course's grading policy. + + Every field is optional, and a field left out keeps its stored value. + """ + + graders = GraderUpdateSerializer( + many=True, + required=False, + help_text="Replaces every assignment type, in the order given.", + ) + grade_cutoffs = serializers.DictField( + child=_ScoreField(), + allow_empty=False, + # Never reached, as allow_empty refuses an empty object first; it publishes the minimum. + validators=[MinLengthValidator(1)], + required=False, + help_text="Lowest score, from 0 to 1, that earns each letter grade, keyed by the grade's name.", + ) + grace_period = _GracePeriodField( + required=False, + allow_null=True, + help_text=( + "Grace period after each due date. Null leaves the stored grace period unchanged; " + "a grace period cannot be removed here." + ), + ) + minimum_grade_credit = _NumberField( + required=False, + allow_null=True, + help_text="Lowest course score, from 0 to 1, that earns course credit. Null leaves it unchanged.", + ) + + class Meta: + ref_name = "authoring_grading.CourseGradingUpdate.v3" + + def to_internal_value(self, data): + if html.is_html_input(data): + raise ValidationError({api_settings.NON_FIELD_ERRORS_KEY: ["Send the grading policy as a JSON object."]}) + return super().to_internal_value(data) + + +class _WholePolicyGraderUpdateSerializer(GraderUpdateSerializer): # pylint: disable=abstract-method + """One assignment type in a whole-policy update, whose type may be any value.""" + + type = _TextField(help_text="Name of the assignment type, for example Homework.") + + class Meta: + ref_name = "authoring_grading.WholePolicyGraderUpdate.v3" + list_serializer_class = _GraderListSerializer + + +class _WholeGradingPolicyUpdateSerializer(CourseGradingUpdateSerializer): # pylint: disable=abstract-method + """ + A grading policy update holding every policy field. + + The grading model's whole-policy update stores some values beyond the + documented shape, and this accepts them: null cutoffs or cutoff values, + empty cutoffs, a grader type of any JSON type, and an empty string as the + credit minimum, which stores no minimum. + """ + + graders = _WholePolicyGraderUpdateSerializer( + many=True, + required=False, + help_text="Replaces every assignment type, in the order given.", + ) + grade_cutoffs = _AnyGradeCutoffsField( + required=False, + help_text="Lowest score, from 0 to 1, that earns each letter grade, keyed by the grade's name.", + ) + minimum_grade_credit = _CreditMinimumField( + required=False, + allow_null=True, + help_text="Lowest course score, from 0 to 1, that earns course credit. Null leaves it unchanged.", + ) + + class Meta: + ref_name = "authoring_grading.WholeGradingPolicyUpdate.v3" + + +def grading_policy_update_serializer(data): + """Return the serializer that validates the grading policy update body ``data``.""" + if isinstance(data, Mapping) and all(field in data for field in GRADING_POLICY_FIELDS): + return _WholeGradingPolicyUpdateSerializer(data=data) + return CourseGradingUpdateSerializer(data=data) diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py b/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py index 196d8aebcc1b..735051e67e85 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/authoring_grading.py @@ -1,187 +1,175 @@ -""" -API Views for course grading settings — v3. - -This module is the v3 incarnation of the v0 ``AuthoringGradingView`` endpoint, -restructured to apply the FC-0118 ADRs from the start: - - * ADR 0025 – ``serializer_class`` on the viewset - * ADR 0026 – explicit ``authentication_classes`` + ``permission_classes`` - * ADR 0027 – ``drf_spectacular`` for OpenAPI schema generation - * ADR 0028 – consolidated into a single DRF ``ViewSet`` registered via - ``DefaultRouter`` (replaces ``AuthoringGradingView`` ``APIView``) - * ADR 0029 – standardized error envelope via :class:`StandardizedErrorMixin` - (v3-scoped — does not change the project-wide DRF ``EXCEPTION_HANDLER`` - setting) - * ADR 0033 / OEP-68 – the URL kwarg, action parameter, and OpenAPI parameter - are named ``course_key`` (the OEP-68-standardized name) rather than the - legacy ``course_id``. Since this is a brand-new versioned API, no - deprecated alias is needed — clients on the v0 endpoint continue to use - ``course_id`` there. - * ADR 0036 – **largely out of scope.** The ``CourseGradingModelSerializer`` - response is a single top-level ``graders`` list of small fixed-shape - objects (type, min_count, drop_count, short_label, weight, id) — no - tree nesting, no embedded sub-objects, no ``children`` field, no wide - flat object that would benefit from ``?view=minimal`` / ``?fields=``. - - The one ADR 0036 concern is anti-pattern #3 (unbounded child list): the - ``graders`` array has no upper bound in the serializer. In practice each - course has typically ≤8 graders (Homework, Lab, Exam, etc.) and the - update flow is exercised only by course-authoring staff, so the - real-world payload is always small. A hard cap is enforced upstream of - this endpoint by :func:`CourseGradingModel.update_from_json`; we surface - that as a documentation note rather than re-implement the bound here. - * ADR 0034 – auth standardization (OEP-0042). - ``authentication_classes`` is ``(JwtAuthentication, SessionAuthenticationAllowInactiveUser)``; - ``BearerAuthenticationAllowInactiveUser`` has been removed per the - deprecation policy. ``SessionAuthenticationAllowInactiveUser`` is - retained (rather than relying on the platform-default - ``SessionAuthentication``) so Studio authors whose accounts are - temporarily inactive can still update grading. - -Permission model note: - PR #38363 proposed a class-level ``HasStudioReadAccess`` permission. The - current v0 view has since evolved to use the ``openedx_authz`` permission - framework (``COURSES_EDIT_GRADING_SETTINGS``), which is more specific to - grading and aligns with the platform-wide authz direction. - - The v3 viewset preserves the openedx_authz model via an *inline* - ``user_has_course_permission`` check inside the action body (rather than - the ``@authz_permission_required`` decorator). The decorator raises - ``DeveloperErrorResponseException`` — a plain ``Exception`` subclass that - does not flow through DRF's exception handler, so it would bypass - :class:`StandardizedErrorMixin` and surface as an unstructured 500. - Raising ``rest_framework.exceptions.PermissionDenied`` directly keeps the - ADR 0029 envelope intact. -""" - -from drf_spectacular.utils import OpenApiParameter, OpenApiRequest, OpenApiResponse, extend_schema +"""API views for a course's grading policy (v3).""" + +from drf_spectacular.utils import ( + OpenApiParameter, + OpenApiResponse, + PolymorphicProxySerializer, + 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.routers import COURSE_KEY_LOOKUP_REGEX -from openedx_authz.constants.permissions import COURSES_EDIT_GRADING_SETTINGS from rest_framework import viewsets -from rest_framework.exceptions import PermissionDenied from rest_framework.permissions import IsAuthenticated from rest_framework.request import Request from rest_framework.response import Response -from cms.djangoapps.contentstore.rest_api.v0.serializers import CourseGradingModelSerializer -from cms.djangoapps.contentstore.rest_api.v3.utils import COMMON_ERROR_RESPONSES, resolve_course_key -from cms.djangoapps.models.settings.course_grading import CourseGradingModel -from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission -from openedx.core.djangoapps.authz.decorators import user_has_course_permission -from openedx.core.djangoapps.credit.tasks import update_credit_course_requirements +from cms.djangoapps.contentstore.rest_api.v3.grading_service import ( + get_full_grading_settings, + get_grading_policy, + update_grading_policy, +) +from cms.djangoapps.contentstore.rest_api.v3.permissions import CanEditGradingSettings, CanViewGradingSettings +from cms.djangoapps.contentstore.rest_api.v3.serializers.grading import ( + GRADING_FULL_VIEW, + CourseGradingFullSerializer, + CourseGradingSerializer, + CourseGradingUpdateSerializer, + GradingViewQuerySerializer, + grading_policy_update_serializer, +) +from cms.djangoapps.contentstore.rest_api.v3.utils import resolve_course_key _COURSE_KEY_PARAMETER = OpenApiParameter( name="course_key", - description="OEP-68 course key (e.g. course-v1:org+course+run).", + description="Key of the course, for example course-v1:edX+DemoX+Demo_Course.", required=True, type=str, location=OpenApiParameter.PATH, ) +_VIEW_PARAMETER = OpenApiParameter( + name="view", + description=GradingViewQuerySerializer().fields["view"].help_text, + required=False, + type=str, + enum=[GRADING_FULL_VIEW], + location=OpenApiParameter.QUERY, +) + + +# The full shape is listed first: the default shape allows extra keys, so a +# client trying each shape in order would otherwise read a full response as the +# default one and lose the view=full fields. As the shapes overlap, the CMS +# schema hooks publish this component as anyOf (cms.lib.spectacular.ANY_OF_COMPONENTS). +_READ_RESPONSE = PolymorphicProxySerializer( + component_name="authoring_grading.CourseGradingRead.v3", + serializers=[CourseGradingFullSerializer, CourseGradingSerializer], + resource_type_field_name=None, +) + + +def _error(description): + return OpenApiResponse(response=ErrorResponseSerializer, description=description) + @extend_schema(tags=["openedx-platform-sdk"]) class AuthoringGradingViewSet(StandardizedErrorMixin, viewsets.ViewSet): """ - ViewSet for course grading settings (v3). Registered via DefaultRouter - (basename ``authoring_grading``). + The grading policy of one course: its assignment types and their weights, + letter-grade cutoffs, grace period, and minimum grade for course credit. - Router-generated URL:: + Reading the policy needs permission to view the course's grading settings, + changing it permission to edit them. The course is looked up first, so a + signed-in caller learns that a course does not exist (404) before being + refused (403). - PATCH /api/contentstore/v3/authoring_grading/{course_key}/ → partial_update - - Supersedes ``AuthoringGradingView`` at ``POST /api/contentstore/v0/grading/{course_id}``. + A plain ``ViewSet``: the policy is a settings field of the course in the + modulestore, not database rows. """ - # ADR 0034 — JWT + session-with-inactive-user (BearerAuthenticationAllowInactiveUser - # removed per OEP-0042). SessionAuthenticationAllowInactiveUser is retained - # (instead of relying on the platform-default SessionAuthentication) so Studio - # authors whose accounts are temporarily inactive can still update grading. - authentication_classes = ( - JwtAuthentication, - SessionAuthenticationAllowInactiveUser, - ) + # Studio authors whose account is not yet activated use this endpoint from + # the authoring app; the default session class would reject them. + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) permission_classes = (IsAuthenticated,) - serializer_class = CourseGradingModelSerializer + action_permission_classes = { + "retrieve": CanViewGradingSettings, + "partial_update": CanEditGradingSettings, + } + serializer_class = CourseGradingSerializer - # DefaultRouter lookup: matches course-v1:org+course+run (+ or / separators). - # OEP-68: the kwarg name is ``course_key`` (not the legacy ``course_id``). lookup_field = "course_key" lookup_value_regex = COURSE_KEY_LOOKUP_REGEX + _course_key = None + + def get_permissions(self): + """Add the permission of the requested action to the authentication check.""" + permissions = super().get_permissions() + action_permission = self.action_permission_classes.get(self.action) + if action_permission is not None: + permissions.append(action_permission()) + return permissions + + def get_course_key(self): + """Return the course in the path, parsed and checked to exist (404 otherwise).""" + if self._course_key is None: + self._course_key = resolve_course_key(self.kwargs["course_key"]) + return self._course_key + def get_serializer(self, *args, **kwargs): - """Instantiate and return the configured serializer class.""" + """Instantiate the policy serializer declared on this viewset.""" return self.serializer_class(*args, **kwargs) + @extend_schema( + summary="Get a course's grading settings", + description=( + "Returns the course's grading policy. With view=full the response also lists the graded " + "subsections of each assignment type, the default letter grade names, and the course's " + "credit and proctoring context." + ), + parameters=[_COURSE_KEY_PARAMETER, _VIEW_PARAMETER], + responses={ + 200: OpenApiResponse( + response=_READ_RESPONSE, + description="The grading policy, in the full shape with view=full and the default shape otherwise.", + ), + 400: _error("The view parameter has an unknown value."), + 401: _error("The requester is not authenticated."), + 403: _error("The requester cannot view the grading settings of this course."), + 404: _error("The course does not exist."), + }, + ) + # The legacy router also routes ".json" here, passing ``format``. + def retrieve( + self, request: Request, course_key: str, format=None, # pylint: disable=unused-argument,redefined-builtin + ): + """Return the grading policy, with the grading page's context when ``view=full``.""" + query = GradingViewQuerySerializer(data=request.query_params) + query.is_valid(raise_exception=True) + if query.validated_data.get("view") == GRADING_FULL_VIEW: + full = get_full_grading_settings(self.get_course_key()) + return Response(CourseGradingFullSerializer(full).data) + return Response(self.get_serializer(get_grading_policy(self.get_course_key())).data) + @extend_schema( summary="Update a course's grading settings", - description="Partially update the grading settings for the specified course.", - request=OpenApiRequest(request=CourseGradingModelSerializer), + description=( + "Changes the fields present in the body and leaves the others as stored. graders replaces " + "every assignment type. A null grace_period or minimum_grade_credit leaves that field " + "unchanged. Unknown fields are ignored. Returns the stored policy." + ), + request={"application/json": CourseGradingUpdateSerializer}, parameters=[_COURSE_KEY_PARAMETER], responses={ 200: OpenApiResponse( - response=CourseGradingModelSerializer, - description="Grading settings updated successfully.", + response=CourseGradingSerializer, + description="The grading policy as stored after the update.", ), - **COMMON_ERROR_RESPONSES, + 400: _error("The body is not a valid grading policy update."), + 401: _error("The requester is not authenticated."), + 403: _error("The requester cannot edit the grading settings of this course."), + 404: _error("The course does not exist."), }, ) - def partial_update(self, request: Request, course_key: str): - """ - Update a course's grading settings. - - **Example Request** - - PATCH /api/contentstore/v3/authoring_grading/{course_key}/ - - **PATCH Parameters** - - The request body should follow the ``CourseGradingModelSerializer`` - schema. Example:: - - { - "graders": [ - { - "type": "Homework", - "min_count": 1, - "drop_count": 0, - "short_label": "", - "weight": 100, - "id": 0 - } - ], - "grade_cutoffs": {"A": 0.75, "B": 0.63, "C": 0.57, "D": 0.5}, - "grace_period": {"hours": 12, "minutes": 0}, - "minimum_grade_credit": 0.7, - "is_credit_course": true - } - - **Response Values** - - If the request is successful, an HTTP 200 "OK" response is returned - with the updated grading data serialized via - :class:`CourseGradingModelSerializer`. - """ - parsed_course_key = resolve_course_key(course_key) - - # Per-action authorization (ADR 0026): kept inline rather than - # behind ``@authz_permission_required`` because that decorator - # raises ``DeveloperErrorResponseException`` (not a DRF exception), - # which bypasses :class:`StandardizedErrorMixin`. Raising - # ``PermissionDenied`` directly flows through the ADR 0029 envelope. - if not user_has_course_permission( - request.user, - COURSES_EDIT_GRADING_SETTINGS.identifier, - parsed_course_key, - LegacyAuthoringPermission.READ, - ): - raise PermissionDenied("You do not have permission to perform this action.") - - if "minimum_grade_credit" in request.data: - update_credit_course_requirements.delay(str(parsed_course_key)) - - updated_data = CourseGradingModel.update_from_json(parsed_course_key, request.data, request.user) - serializer = self.get_serializer(updated_data) - return Response(serializer.data) + def partial_update( + self, request: Request, course_key: str, format=None, # pylint: disable=unused-argument,redefined-builtin + ): + """Change the grading policy fields present in the body.""" + grading_policy_update_serializer(request.data).is_valid(raise_exception=True) + # The model receives the body as sent: it does its own conversions, and + # converting here first would change what it stores. + policy = update_grading_policy(self.get_course_key(), request.data, request.user) + return Response(self.get_serializer(policy).data) diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py index 7503ee1ff6e9..5fedce281a49 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py @@ -1,360 +1,1331 @@ -""" -Unit tests for AuthoringGradingViewSet (v3). - -Single test module covering every ADR applied to the viewset: - * ADR 0025 / 0026 / 0027 / 0028 — action + permission + routing tests - (``TestAuthoringGradingViewSetPermissions``, ``TestAuthoringGradingViewSetUpdate``, - ``TestAuthoringGradingViewSetRouting``) - * ADR 0029 — standardized error envelope shape tests - (``TestAuthoringGradingViewSetErrorShape``) - -MongoDB-free: every service-layer call (``CourseGradingModel.update_from_json``, -``CourseOverview.course_exists``, ``update_credit_course_requirements.delay``, -and the ``openedx_authz`` permission lookup) is mocked, so these tests run -without a live modulestore or course-overview row. -""" +"""Tests for the v3 course grading API.""" + import json -from types import SimpleNamespace +from contextlib import contextmanager +from datetime import timedelta +from functools import cache +from importlib import import_module +from unittest import TestCase from unittest.mock import patch -from django.test import TestCase -from django.urls import resolve, reverse -from rest_framework import status -from rest_framework.test import APIClient, APITestCase +import ddt +import jsonschema +import pymongo +import pytest +from django.conf import settings +from django.db import connection +from django.test.utils import CaptureQueriesContext, override_settings +from django.urls import include, path, resolve, reverse +from drf_spectacular.generators import SchemaGenerator +from edx_rest_framework_extensions.testing import assert_error_envelope +from edx_toggles.toggles.testutils import override_waffle_flag +from openedx_authz.constants.roles import ( + COURSE_ADMIN, + COURSE_AUDITOR, + COURSE_DATA_RESEARCHER, + COURSE_EDITOR, + COURSE_STAFF, +) +from openedx_authz.models.engine import PolicyCacheControl +from rest_framework.test import APIClient +from cms.djangoapps.contentstore.rest_api.v3.serializers.grading import CourseGradingSerializer +from cms.djangoapps.contentstore.signals.signals import GRADING_POLICY_CHANGED from cms.djangoapps.contentstore.tests.utils import routed_actions +from cms.djangoapps.contentstore.toggles import LEGACY_STUDIO_EXAM_SETTINGS +from cms.djangoapps.models.settings.course_grading import GRADING_POLICY_CHANGED_EVENT_TYPE, CourseGradingModel +from cms.djangoapps.models.settings.waffle import MATERIAL_RECOMPUTE_ONLY_FLAG +from common.djangoapps.student.models import CourseAccessRole +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseLimitedStaffRole, + CourseStaffRole, + OrgStaffRole, +) from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin +from openedx.core.djangoapps.credit.tests.factories import CreditCourseFactory +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 SignalHandler, modulestore +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory -COURSE_ID = "course-v1:edX+ToyX+Toy_Course" - -# Minimal graders payload accepted by CourseGradingModelSerializer. -_GRADERS_PAYLOAD = [ - { - "type": "Homework", - "min_count": 1, - "drop_count": 0, - "short_label": "", - "weight": 100, - "id": 0, - }, +LEGACY = "legacy" +CONFORMING = "conforming" +MOUNTS = (LEGACY, CONFORMING) +UNKNOWN_COURSE_KEY = "course-v1:NoSuchOrg+NoSuchCourse+NoSuchRun" +CREDIT_TASK = "openedx.core.djangoapps.credit.tasks.update_credit_course_requirements.delay" +UPDATE_ITEM = "xmodule.modulestore.mixed.MixedModuleStore.update_item" + +# Stored on every seeded course. 0.29 does not survive the percentage round trip +# (it is shown as 28), so it reveals any write-back of graders nobody sent. +SEED_GRADERS = [ + {"type": "Homework", "min_count": 3, "drop_count": 1, "short_label": "HW", "weight": 0.29}, + {"type": "Lab", "min_count": 2, "drop_count": 0, "short_label": "Lab", "weight": 0.07}, + {"type": "Exam", "min_count": 1, "drop_count": 0, "short_label": None, "weight": 0.64}, ] +SEED_CUTOFFS = {"A": 0.9, "B": 0.8, "Pass": 0.5} +SEED_GRACE = timedelta(hours=2, minutes=5) +SEED_CREDIT = 0.8 -# Fake CourseGradingModel return value: only the field the serializer reads. -_MOCK_GRADING_MODEL = SimpleNamespace(graders=_GRADERS_PAYLOAD) +GRADERS = [ + {"type": "Homework", "min_count": 3, "drop_count": 1, "short_label": "HW", "weight": 28, "id": 0}, + {"type": "Lab", "min_count": 2, "drop_count": 0, "short_label": "Lab", "weight": 7, "id": 1}, + {"type": "Exam", "min_count": 1, "drop_count": 0, "short_label": "", "weight": 64, "id": 2}, +] +# The seeded graders exactly as stored: sending them changes no weight or count. +SEED_GRADERS_AS_SENT = [ + {"type": "Homework", "min_count": 3, "drop_count": 1, "short_label": "HW", "weight": 29}, + {"type": "Lab", "min_count": 2, "drop_count": 0, "short_label": "Lab", "weight": 7}, + {"type": "Exam", "min_count": 1, "drop_count": 0, "short_label": None, "weight": 64}, +] +CUTOFFS = {"A": 0.85, "B": 0.7, "Pass": 0.45} -# CourseOverview.course_exists is called inside ``resolve_course_key()`` (now in -# v3/utils.py), not directly in the view module — so the patch must target the -# utils module's binding, not the view module's. -MOCK_COURSE_EXISTS = ( - "cms.djangoapps.contentstore.rest_api.v3.utils.CourseOverview.course_exists" -) -MOCK_UPDATE_FROM_JSON = ( - "cms.djangoapps.contentstore.rest_api.v3.views.authoring_grading.CourseGradingModel.update_from_json" -) -MOCK_CREDIT_TASK = ( - "cms.djangoapps.contentstore.rest_api.v3.views.authoring_grading." - "update_credit_course_requirements.delay" -) -# Patch the local module-level binding (imported from openedx.core.djangoapps.authz.decorators) -# — patching the source module would not replace the already-bound reference inside the view. -MOCK_HAS_PERMISSION = ( - "cms.djangoapps.contentstore.rest_api.v3.views.authoring_grading.user_has_course_permission" + +def _full(**overrides): + """A whole-policy body, with ``overrides`` replacing its fields.""" + body = { + "graders": GRADERS, + "grade_cutoffs": CUTOFFS, + "grace_period": {"hours": 3, "minutes": 15}, + "minimum_grade_credit": 0.75, + } + body.update(overrides) + return body + + +# Whole-policy bodies the grading model accepts, picked by hand. Each must produce +# the same response bytes, stored policy, events and credit task as the +# whole-policy update. +HAND_PICKED_FULL_BODIES = { + "authoring_app": _full(), + "end_to_end_post": { + "graders": [{"type": "Homework", "min_count": 2, "drop_count": 0, "short_label": "HW", "weight": 100}], + "grade_cutoffs": {"Pass": 0.6}, + "grace_period": {"hours": 0, "minutes": 30}, + "minimum_grade_credit": 0.8, + "is_credit_course": False, + }, + "documented_example_with_credit_flag": { + "graders": [{"type": "Homework", "min_count": 1, "drop_count": 0, "short_label": "", "weight": 100, "id": 0}], + "grade_cutoffs": {"A": 0.75, "B": 0.63, "C": 0.57, "D": 0.5}, + "grace_period": {"hours": 12, "minutes": 0}, + "minimum_grade_credit": 0.7, + "is_credit_course": True, + }, + "graders_without_id": _full(graders=[{k: v for k, v in g.items() if k != "id"} for g in GRADERS]), + "numbers_as_strings": _full(graders=[ + {**g, "min_count": "1", "drop_count": "0", "weight": str(g["weight"])} for g in GRADERS + ]), + "grace_period_null": _full(grace_period=None), + "minimum_grade_credit_null": _full(minimum_grade_credit=None), + "grace_period_with_seconds": _full(grace_period={"hours": 1, "minutes": 2, "seconds": 3}), + "grace_period_with_days": _full(grace_period={"days": 1, "hours": 2}), + "grace_period_wrapped": _full(grace_period={"grace_period": {"hours": 2}}), + "grace_period_empty": _full(grace_period={}), + "grace_period_fractional": _full(grace_period={"hours": 1.5}), + "displayed_weights_resent": _full(graders=[{**GRADERS[0], "weight": 28}, GRADERS[1], GRADERS[2]]), + "stored_policy_resent": _full(graders=SEED_GRADERS_AS_SENT, grade_cutoffs=SEED_CUTOFFS), + "unknown_key": _full(foo="bar"), + "graders_empty_list": _full(graders=[]), + "graders_empty_object": _full(graders={}), + "graders_empty_string": _full(graders=""), + "grader_type_null": _full(graders=[{**GRADERS[0], "type": None}]), + "grader_type_number": _full(graders=[{**GRADERS[0], "type": 123}]), + "grader_count_fractional": _full(graders=[{**GRADERS[0], "min_count": 1.5}]), + "grader_count_boolean": _full(graders=[{**GRADERS[0], "min_count": True}]), + "grader_weight_boolean": _full(graders=[{**GRADERS[0], "weight": True}]), + "grader_label_number": _full(graders=[{**GRADERS[0], "short_label": 5}]), + "grader_extra_key": _full(graders=[{**GRADERS[0], "extra": 1}]), + "cutoffs_null": _full(grade_cutoffs=None), + "cutoffs_empty": _full(grade_cutoffs={}), + "cutoff_as_string": _full(grade_cutoffs={"A": "0.5"}), + "credit_as_string": _full(minimum_grade_credit="0.5"), + "credit_as_integer": _full(minimum_grade_credit=1), +} + +# Every position of a whole-policy body was sent each of these values, plus 10 ** 30. +# WHOLE_POLICY_VALUES lists, per position, the ones the whole-policy update stores +# and answers 200 for; every other combination fails inside the grading model. +_SCALARS = (True, False, 0, 1, -1, 0.5) +_ANY_JSON = ( + None, *_SCALARS, 1e308, "", "x", "1", "0.5", "inf", "nan", "1" * 30, [], ["a"], [1], {}, {"x": 1}, ) +GRACE_PERIOD_UNITS = ("hours", "minutes", "seconds", "days", "weeks", "milliseconds", "microseconds") +WHOLE_POLICY_VALUES = { + "graders": ([], {}, ""), + "grader.type": _ANY_JSON, + "grader.short_label": _ANY_JSON, + "grader.id": (*_ANY_JSON, 10 ** 30), + "grader.min_count": (*_SCALARS, "1"), + "grader.drop_count": (*_SCALARS, "1"), + "grader.weight": (*_SCALARS, 1e308, 10 ** 30, "1", "0.5", "1" * 30), + "grade_cutoffs": (None, {}, {"x": 1}), + "cutoff": (None, *_SCALARS, 1e308, "1", "0.5", "1" * 30), + "grace_period": (None, {}), + "grace_period.unit": _SCALARS, + "minimum_grade_credit": (None, *_SCALARS, 1e308, "", "1", "0.5", "1" * 30), + "unknown_field": (*_ANY_JSON, 10 ** 30), +} + + +def _whole_policy_bodies(): + """One whole-policy body per position and accepted value in WHOLE_POLICY_VALUES.""" + bodies = {} + for position, values in WHOLE_POLICY_VALUES.items(): + for value in values: + name = f"{position}={value!r}" + if position == "graders": + bodies[name] = _full(graders=value) + elif position.startswith("grader."): + field = position.split(".")[1] + bodies[name] = _full(graders=[{**GRADERS[0], field: value}, *GRADERS[1:]]) + elif position == "cutoff": + bodies[name] = _full(grade_cutoffs={"A": value, "Pass": 0.45}) + bodies[f"only_{name}"] = _full(grade_cutoffs={"A": value}) + elif position == "grace_period.unit": + for unit in GRACE_PERIOD_UNITS: + bodies[f"grace_period.{unit}={value!r}"] = _full(grace_period={unit: value}) + elif position == "unknown_field": + bodies[name] = _full(unknown=value) + else: + bodies[name] = _full(**{position: value}) + for field in ("min_count", "drop_count", "short_label", "weight", "id"): + bodies[f"grader_without_{field}"] = _full( + graders=[{key: value for key, value in GRADERS[0].items() if key != field}], + ) + bodies["cutoff_named_with_empty_string"] = _full(grade_cutoffs={"": 0.5}) + return bodies + + +GENERATED_FULL_BODIES = _whole_policy_bodies() +FULL_BODIES = {**HAND_PICKED_FULL_BODIES, **GENERATED_FULL_BODIES} + +# The whole-policy bodies replayed per mount. Both mounts route to one view, so +# each generated body is replayed on one of them, alternating; the hand-picked +# bodies are replayed on both. +FULL_BODY_CASES = [ + *((mount, name, body) for mount in MOUNTS for name, body in HAND_PICKED_FULL_BODIES.items()), + *((MOUNTS[index % len(MOUNTS)], name, body) for index, (name, body) in enumerate(GENERATED_FULL_BODIES.items())), +] + +# Bodies that make the grading model raise. Each is refused with a 400 naming the +# field, before anything is written. +REJECTED_BODIES = { + "graders_null": ({"graders": None}, "graders"), + "graders_not_a_list": ({"graders": 5}, "graders"), + "graders_a_string": ({"graders": "x"}, "graders"), + "grader_not_an_object": (_full(graders=["Homework"]), "graders"), + "grader_without_type": ({"graders": [{"min_count": 1, "weight": 100}]}, "graders"), + "grader_count_not_a_number": ({"graders": [{**GRADERS[0], "min_count": "many"}]}, "graders"), + "grader_count_fractional_string": (_full(graders=[{**GRADERS[0], "min_count": "1.5"}]), "graders"), + "grader_count_null": (_full(graders=[{**GRADERS[0], "min_count": None}]), "graders"), + "grader_drop_count_not_a_number": (_full(graders=[{**GRADERS[0], "drop_count": "x"}]), "graders"), + "grader_weight_not_a_number": (_full(graders=[{**GRADERS[0], "weight": "abc"}]), "graders"), + "grader_weight_null": (_full(graders=[{**GRADERS[0], "weight": None}]), "graders"), + "grader_weight_infinite": (_full(graders=[{**GRADERS[0], "weight": "inf"}]), "graders"), + "cutoffs_a_string": (_full(grade_cutoffs="x"), "grade_cutoffs"), + "cutoffs_a_list": (_full(grade_cutoffs=[]), "grade_cutoffs"), + "cutoff_not_a_number": ({"grade_cutoffs": {"A": "x"}}, "grade_cutoffs"), + "cutoff_an_object": (_full(grade_cutoffs={"A": {"x": 1}}), "grade_cutoffs"), + "grace_period_unknown_unit": (_full(grace_period={"foo": 1}), "grace_period"), + "grace_period_string_value": (_full(grace_period={"hours": "1"}), "grace_period"), + "grace_period_a_string": (_full(grace_period="x"), "grace_period"), + "grace_period_a_list": (_full(grace_period=[]), "grace_period"), + "grace_period_too_long": (_full(grace_period={"days": 10 ** 10}), "grace_period"), + "credit_not_a_number": (_full(minimum_grade_credit="x"), "minimum_grade_credit"), + "credit_a_list": (_full(minimum_grade_credit=[1]), "minimum_grade_credit"), + "body_a_list": ([1, 2], "non_field_errors"), + # A body without every policy field is held to the documented shape: a grader + # type is a string, and the cutoffs are at least one JSON number. + "partial_cutoffs_null": ({"grade_cutoffs": None}, "grade_cutoffs"), + "partial_cutoffs_empty": ({"grade_cutoffs": {}}, "grade_cutoffs"), + "partial_cutoff_null": ({"grade_cutoffs": {"A": None, "Pass": 0.5}}, "grade_cutoffs"), + "partial_only_cutoff_null": ({"grade_cutoffs": {"A": None}}, "grade_cutoffs"), + "partial_cutoff_a_numeric_string": ({"grade_cutoffs": {"A": "0.5"}}, "grade_cutoffs"), + "partial_cutoff_a_boolean": ({"grade_cutoffs": {"A": True}}, "grade_cutoffs"), + "partial_graders_and_cutoffs_null": ({"graders": GRADERS, "grade_cutoffs": None}, "grade_cutoffs"), + "partial_three_fields_cutoffs_empty": ( + {"graders": GRADERS, "grade_cutoffs": {}, "grace_period": {"hours": 1}}, "grade_cutoffs", + ), + "partial_grader_type_null": ({"graders": [{**GRADERS[0], "type": None}]}, "graders"), + "partial_grader_type_a_number": ({"graders": [{**GRADERS[0], "type": 123}]}, "graders"), + "partial_grader_type_a_list": ({"graders": [{**GRADERS[0], "type": ["a"]}]}, "graders"), + "partial_grader_type_an_object": ({"graders": [{**GRADERS[0], "type": {}}]}, "graders"), + "partial_credit_empty_string": ({"minimum_grade_credit": ""}, "minimum_grade_credit"), +} + +# Bodies holding an integer wider than the 8 bytes the modulestore can store, at a +# position where the grading model stores it. The whole-policy update fails part-way +# through its write on these and leaves the modulestore unable to read the course for +# the rest of the process, so they are not replayed against it here. +TOO_LARGE_BODIES = { + "grader_count_huge": (_full(graders=[{**GRADERS[0], "min_count": 10 ** 30}]), "graders"), + "grader_count_huge_float": (_full(graders=[{**GRADERS[0], "min_count": 1e308}]), "graders"), + "grader_count_huge_string": (_full(graders=[{**GRADERS[0], "min_count": "1" * 30}]), "graders"), + "grader_drop_count_huge": (_full(graders=[{**GRADERS[0], "drop_count": 10 ** 30}]), "graders"), + "grader_drop_count_huge_float": (_full(graders=[{**GRADERS[0], "drop_count": 1e308}]), "graders"), + "grader_drop_count_huge_string": (_full(graders=[{**GRADERS[0], "drop_count": "1" * 30}]), "graders"), + "grader_type_huge": (_full(graders=[{**GRADERS[0], "type": 10 ** 30}]), "graders"), + "grader_label_huge": (_full(graders=[{**GRADERS[0], "short_label": 10 ** 30}]), "graders"), + "grader_label_list_holding_huge": (_full(graders=[{**GRADERS[0], "short_label": [10 ** 30]}]), "graders"), + "cutoff_huge": (_full(grade_cutoffs={"A": 10 ** 30, "Pass": 0.45}), "grade_cutoffs"), + "credit_huge": (_full(minimum_grade_credit=10 ** 30), "minimum_grade_credit"), + "partial_grader_count_huge": ({"graders": [{**GRADERS[0], "min_count": 10 ** 30}]}, "graders"), + "partial_cutoff_huge": ({"grade_cutoffs": {"A": 10 ** 30}}, "grade_cutoffs"), + "partial_credit_huge": ({"minimum_grade_credit": 10 ** 30}, "minimum_grade_credit"), +} + + +def grading_url(mount, course_key): + """Return the address of the grading resource of ``course_key`` on ``mount``.""" + if mount == LEGACY: + return reverse( + "cms.djangoapps.contentstore:v3:authoring_grading-detail", kwargs={"course_key": str(course_key)}, + ) + return reverse("authoring_v3:course_grading", kwargs={"course_key": str(course_key)}) + + +def v0_url(course_key): + return reverse("cms.djangoapps.contentstore:v0:cms_api_update_grading", kwargs={"course_id": str(course_key)}) + + +def v1_url(course_key): + return reverse("cms.djangoapps.contentstore:v1:course_grading", 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 _patch(client, url, body): + return client.patch(url, data=json.dumps(body), content_type="application/json") + + +def _post(client, url, body): + return client.post(url, data=json.dumps(body), content_type="application/json") + + +def seed_course(user, **kwargs): + """Create a course holding the seeded grading policy.""" + course = CourseFactory.create(**kwargs) + course.raw_grader = [dict(grader) for grader in SEED_GRADERS] + course.grade_cutoffs = dict(SEED_CUTOFFS) + course.graceperiod = SEED_GRACE + course.minimum_grade_credit = SEED_CREDIT + modulestore().update_item(course, user.id) + return course + + +def stored_policy(course_key): + """The grading fields as the modulestore holds them, weights as raw fractions.""" + course = modulestore().get_course(course_key) + return { + "raw_grader": course.raw_grader, + "grade_cutoffs": course.grade_cutoffs, + "graceperiod": course.graceperiod, + "minimum_grade_credit": course.minimum_grade_credit, + } + + +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 + + +@cache +def _published_components(): + """The schema components the grading routes publish, generated from the real URLconfs.""" + # Importing the service URLconf registers the course key converter the authoring routes use. + import_module(settings.ROOT_URLCONF) + patterns = [ + path("api/contentstore/", include("cms.djangoapps.contentstore.rest_api.urls")), + path("api/authoring/v3/", include("cms.djangoapps.contentstore.rest_api.v3.authoring_urls")), + ] + schema = SchemaGenerator(patterns=patterns).get_schema(request=None, public=True) + return _as_json_schema(schema["components"]["schemas"]) + + +DEFAULT_SHAPE = "authoring_grading.CourseGrading.v3" +FULL_SHAPE = "authoring_grading.CourseGradingFull.v3" +READ_SHAPE = "authoring_grading.CourseGradingRead.v3" + + +def assert_matches_published_schema(body, component): + """Assert ``body`` is valid against the published schema component ``component``.""" + jsonschema.validate( + body, {"$ref": f"#/components/schemas/{component}", "components": {"schemas": _published_components()}}, + ) + + +@contextmanager +def capture_grading_effects(): + """Record grading-policy signals, grading-policy tracking events and credit tasks.""" + effects = {"signals": 0, "events": 0, "credit_tasks": []} + + def receiver(sender, **kwargs): # pylint: disable=unused-argument + effects["signals"] += 1 + + def emit(name, *args, **kwargs): # pylint: disable=unused-argument + if name == GRADING_POLICY_CHANGED_EVENT_TYPE: + effects["events"] += 1 + + GRADING_POLICY_CHANGED.connect(receiver, weak=False) + try: + with patch("eventtracking.tracker.emit", side_effect=emit), patch(CREDIT_TASK) as credit: + yield effects + effects["credit_tasks"] = [call.args for call in credit.call_args_list] + finally: + GRADING_POLICY_CHANGED.disconnect(receiver) + + +@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 + -_REQUIRED_ERROR_FIELDS = ("type", "title", "status", "detail", "instance") +def sql_writes(queries): + """``(statement, table)`` for each INSERT, UPDATE and DELETE among the captured ``queries``.""" + writes = [] + for query in queries.captured_queries: + words = query["sql"].split() + statement = words[0].upper() + if statement in ("INSERT", "DELETE"): + writes.append((statement, words[2].strip('"`'))) + elif statement == "UPDATE": + writes.append((statement, words[1].strip('"`'))) + return writes -# =========================================================================== -# ADR 0026 — permission boundary tests -# =========================================================================== -class TestAuthoringGradingViewSetPermissions(APITestCase): +# --------------------------------------------------------------------------- +# Who may call the endpoint +# --------------------------------------------------------------------------- + +OPERATIONS = ("get", "get_full", "patch_full", "patch_partial") +CALLS = [(mount, operation) for mount in MOUNTS for operation in OPERATIONS] + + +class _AccessMatrixMixin: """ - ADR 0026 — permission boundary tests for the partial_update action. + Calls every operation on both mounts and checks the status against the + expected one and against the decision v1 makes for the same caller. """ - def setUp(self): - super().setUp() - self.client = APIClient() - self.url = reverse( - "cms.djangoapps.contentstore:v3:authoring_grading-detail", - kwargs={"course_key": COURSE_ID}, + def _call(self, user, mount, operation, course_key): + """Call ``operation`` on ``mount`` as ``user``.""" + client = _client(user) + url = grading_url(mount, course_key) + if operation == "get": + return client.get(url) + if operation == "get_full": + return client.get(url, {"view": "full"}) + if operation == "patch_full": + return _patch(client, url, _full()) + return _patch(client, url, {"grade_cutoffs": CUTOFFS}) + + def _v1_status(self, user, operation, course_key): + client = _client(user) + if operation.startswith("get"): + return client.get(v1_url(course_key)).status_code + return _post(client, v1_url(course_key), _full()).status_code + + def assert_access(self, user, mount, operation, expected, course_key=None): + """Assert the status, the error envelope when refused, and agreement with v1.""" + course_key = course_key or self.course.id + with patch(CREDIT_TASK): + response = self._call(user, mount, operation, course_key) + v1_status = self._v1_status(user, operation, course_key) + assert response.status_code == expected, response.content + if expected != 200: + slug = {401: "authn", 403: "authz", 404: "not-found"}[expected] + assert_error_envelope(response, expected_status=expected, expected_type_slug=slug) + assert v1_status == expected, f"v1 decided {v1_status} for the same caller" + + +@ddt.ddt +class GradingAccessLegacyPermissionsTest(_AccessMatrixMixin, SharedModuleStoreTestCase): + """Who may read and change the policy 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(*CALLS) + @ddt.unpack + def test_anonymous_caller_is_refused_with_401(self, mount, operation): + self.assert_access(None, mount, operation, 401) + + @ddt.data(*CALLS) + @ddt.unpack + def test_user_without_a_role_is_refused_with_403(self, mount, operation): + self.assert_access(UserFactory(), mount, operation, 403) + + @ddt.data(*CALLS) + @ddt.unpack + def test_unknown_course_is_404_before_permission_is_checked(self, mount, operation): + self.assert_access(UserFactory(), mount, operation, 404, course_key=UNKNOWN_COURSE_KEY) + + @ddt.data(*CALLS) + @ddt.unpack + def test_course_staff_may_read_and_change(self, mount, operation): + self.assert_access(self._user_with(CourseStaffRole(self.course.id)), mount, operation, 200) + + @ddt.data(*CALLS) + @ddt.unpack + def test_course_instructor_may_read_and_change(self, mount, operation): + self.assert_access(self._user_with(CourseInstructorRole(self.course.id)), mount, operation, 200) + + @ddt.data(*CALLS) + @ddt.unpack + def test_org_staff_may_read_and_change(self, mount, operation): + self.assert_access(self._user_with(OrgStaffRole(self.course.id.org)), mount, operation, 200) + + @ddt.data(*CALLS) + @ddt.unpack + def test_global_staff_may_read_and_change(self, mount, operation): + self.assert_access(UserFactory(is_staff=True), mount, operation, 200) + + @ddt.data(*CALLS) + @ddt.unpack + def test_superuser_may_read_and_change(self, mount, operation): + self.assert_access(UserFactory(is_superuser=True), mount, operation, 200) + + @ddt.data(*CALLS) + @ddt.unpack + def test_limited_staff_is_refused(self, mount, operation): + self.assert_access(self._user_with(CourseLimitedStaffRole(self.course.id)), mount, operation, 403) + + @ddt.data(*CALLS) + @ddt.unpack + def test_staff_of_another_course_is_refused(self, mount, operation): + self.assert_access(self._user_with(CourseStaffRole(self.other_course.id)), mount, operation, 403) + + @ddt.data(*MOUNTS) + def test_inactive_user_with_a_session_may_read_and_change(self, mount): + """Session callers whose account is not activated are admitted.""" + user = UserFactory(is_staff=True, is_active=False, password="password") + client = APIClient() + assert client.login(username=user.username, password="password") + assert client.get(grading_url(mount, self.course.id)).status_code == 200 + response = _patch(client, grading_url(mount, self.course.id), {"grade_cutoffs": CUTOFFS}) + assert response.status_code == 200 + + @ddt.data(*MOUNTS) + def test_jwt_caller_may_read(self, mount): + user = UserFactory(is_staff=True) + response = APIClient().get( + grading_url(mount, self.course.id), HTTP_AUTHORIZATION=f"JWT {create_jwt_for_user(user)}", ) + assert response.status_code == 200 - def test_unauthenticated_patch_returns_401(self): - """Unauthenticated PATCH must return 401 (IsAuthenticated).""" - response = self.client.patch(self.url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_401_UNAUTHORIZED - - @patch(MOCK_HAS_PERMISSION, return_value=False) - @patch(MOCK_COURSE_EXISTS, return_value=True) - def test_authenticated_without_grading_permission_returns_403(self, mock_exists, mock_perm): # noqa: ARG002 - """Authenticated user without grading permission must receive 403.""" - user = UserFactory.create() - self.client.force_authenticate(user=user) - response = self.client.patch(self.url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_403_FORBIDDEN - - -# =========================================================================== -# ADR 0025 / 0028 — action body tests -# =========================================================================== -class TestAuthoringGradingViewSetUpdate(APITestCase): - """ - Action tests for the partial_update flow. + @ddt.data(*MOUNTS) + def test_bearer_token_is_refused(self, mount): + """Bearer tokens are not accepted here, though the v1 read takes them.""" + user = UserFactory(is_staff=True) + token = AccessTokenFactory(user=user, application=ApplicationFactory(user=user)).token + response = APIClient().get(grading_url(mount, self.course.id), HTTP_AUTHORIZATION=f"Bearer {token}") + assert_error_envelope(response, expected_status=401, expected_type_slug="authn") + assert APIClient().get(v1_url(self.course.id), HTTP_AUTHORIZATION=f"Bearer {token}").status_code == 200 - All service-layer interactions are mocked so the test exercises the - routing, serialization, and credit-task wiring without touching MongoDB - or modulestore. - """ + +@ddt.ddt +class GradingAccessAuthzTest(CourseAuthoringAuthzTestMixin, _AccessMatrixMixin, SharedModuleStoreTestCase): + """Who may read and change the policy when policy-based authoring permissions are on.""" + + @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 + + @ddt.data(*CALLS) + @ddt.unpack + def test_anonymous_caller_is_refused_with_401(self, mount, operation): + self.assert_access(None, mount, operation, 401) + + @ddt.data(*CALLS) + @ddt.unpack + def test_user_without_a_role_is_refused_with_403(self, mount, operation): + self.assert_access(self.unauthorized_user, mount, operation, 403) + + @ddt.data(*CALLS) + @ddt.unpack + def test_unknown_course_is_404_before_permission_is_checked(self, mount, operation): + self.assert_access(self.unauthorized_user, mount, operation, 404, course_key=UNKNOWN_COURSE_KEY) + + @ddt.data(*CALLS) + @ddt.unpack + def test_auditor_may_read_but_not_change(self, mount, operation): + expected = 200 if operation.startswith("get") else 403 + self.assert_access(self._user_with(COURSE_AUDITOR.external_key), mount, operation, expected) + + @ddt.data(*[(mount, operation, role) for mount, operation in CALLS for role in ( + COURSE_STAFF.external_key, COURSE_EDITOR.external_key, COURSE_ADMIN.external_key, + )]) + @ddt.unpack + def test_staff_editor_and_admin_may_read_and_change(self, mount, operation, role): + self.assert_access(self._user_with(role), mount, operation, 200) + + @ddt.data(*CALLS) + @ddt.unpack + def test_data_researcher_is_refused(self, mount, operation): + self.assert_access(self._user_with(COURSE_DATA_RESEARCHER.external_key), mount, operation, 403) + + @ddt.data(*CALLS) + @ddt.unpack + def test_legacy_course_staff_without_a_policy_role_is_refused(self, mount, operation): + user = UserFactory() + CourseAccessRole.objects.create(user=user, role="staff", course_id=self.course.id, org=self.course.id.org) + self.assert_access(user, mount, operation, 403) + + @ddt.data(*CALLS) + @ddt.unpack + def test_superuser_may_read_and_change(self, mount, operation): + self.assert_access(self.super_user, mount, operation, 200) + + +# --------------------------------------------------------------------------- +# Reading the policy +# --------------------------------------------------------------------------- + +def _flatten_v1(v1_body, grace_seconds): + """The v1 read with its policy lifted to the top level and the grace period's seconds added.""" + flat = {key: value for key, value in v1_body.items() if key != "course_details"} + flat.update(v1_body["course_details"]) + if flat["grace_period"] is not None: + flat["grace_period"] = {**flat["grace_period"], "seconds": grace_seconds} + return flat + + +@ddt.ddt +class GradingReadTest(ModuleStoreTestCase): + """The default and full representations.""" def setUp(self): super().setUp() - self.client = APIClient() - self.user = UserFactory.create(is_staff=True) - self.client.force_authenticate(user=self.user) - self.url = reverse( - "cms.djangoapps.contentstore:v3:authoring_grading-detail", - kwargs={"course_key": COURSE_ID}, - ) + self.staff = UserFactory(is_staff=True) + self.client = _client(self.staff) + self.course = seed_course(self.staff) + for name, assignment_type in (("One", "Homework"), ("Two", "Exam"), ("Three", "Homework")): + section = BlockFactory.create(parent=self.course, category="chapter", display_name=f"Section {name}") + BlockFactory.create( + parent=section, category="sequential", display_name=f"Sub {name}", format=assignment_type, graded=True, + ) - @patch(MOCK_CREDIT_TASK) - @patch(MOCK_UPDATE_FROM_JSON, return_value=_MOCK_GRADING_MODEL) - @patch(MOCK_HAS_PERMISSION, return_value=True) - @patch(MOCK_COURSE_EXISTS, return_value=True) - def test_patch_with_minimum_grade_credit_fires_credit_task( - self, mock_exists, mock_perm, mock_update, mock_credit_task, # noqa: ARG002 - ): - """``minimum_grade_credit`` in the payload triggers the credit-requirements Celery task.""" - body = { - "graders": _GRADERS_PAYLOAD, - "grade_cutoffs": {"A": 0.75, "B": 0.63, "C": 0.57, "D": 0.5}, - "grace_period": {"hours": 12, "minutes": 0}, - "minimum_grade_credit": 0.7, - "is_credit_course": True, + def _read(self, mount, **params): + response = self.client.get(grading_url(mount, self.course.id), params) + assert response.status_code == 200, response.content + return response.json() + + @ddt.data(*MOUNTS) + def test_default_view_is_the_stored_policy(self, mount): + expected = CourseGradingSerializer(CourseGradingModel.fetch(self.course.id)).data + assert self._read(mount) == expected + assert self._read(mount) == { + "graders": [ + {"type": "Homework", "min_count": 3, "drop_count": 1, "short_label": "HW", "weight": 28, "id": 0}, + {"type": "Lab", "min_count": 2, "drop_count": 0, "short_label": "Lab", "weight": 7, "id": 1}, + {"type": "Exam", "min_count": 1, "drop_count": 0, "short_label": None, "weight": 64, "id": 2}, + ], + "grade_cutoffs": SEED_CUTOFFS, + "grace_period": {"hours": 2, "minutes": 5, "seconds": 0}, + "minimum_grade_credit": SEED_CREDIT, } - response = self.client.patch(self.url, data=json.dumps(body), content_type="application/json") - assert response.status_code == status.HTTP_200_OK - mock_update.assert_called_once() - mock_credit_task.assert_called_once() - - @patch(MOCK_CREDIT_TASK) - @patch(MOCK_UPDATE_FROM_JSON, return_value=_MOCK_GRADING_MODEL) - @patch(MOCK_HAS_PERMISSION, return_value=True) - @patch(MOCK_COURSE_EXISTS, return_value=True) - def test_patch_without_minimum_grade_credit_skips_credit_task( - self, mock_exists, mock_perm, mock_update, mock_credit_task, # noqa: ARG002 - ): - """Absent ``minimum_grade_credit`` keeps the credit-requirements task unscheduled.""" - body = { - "graders": _GRADERS_PAYLOAD, - "grade_cutoffs": {"A": 0.75, "B": 0.63, "C": 0.57, "D": 0.5}, - "grace_period": {"hours": 12, "minutes": 0}, + + @ddt.data(*MOUNTS) + def test_default_view_matches_what_an_update_returns(self, mount): + with patch(CREDIT_TASK): + updated = _patch(self.client, grading_url(mount, self.course.id), {}).json() + assert self._read(mount) == updated + + @ddt.data(*MOUNTS) + def test_full_view_equals_the_v1_read_flattened(self, mount): + v1_body = self.client.get(v1_url(self.course.id)).json() + full = self._read(mount, view="full") + assert full == _flatten_v1(v1_body, grace_seconds=0) + assert full["course_assignment_lists"] == { + "Homework": ["Section One - Sub One", "Section Three - Sub Three"], + "Exam": ["Section Two - Sub Two"], } - response = self.client.patch(self.url, data=json.dumps(body), content_type="application/json") - assert response.status_code == status.HTTP_200_OK - mock_update.assert_called_once() - mock_credit_task.assert_not_called() + + @ddt.data(*[(mount, enabled) for mount in MOUNTS for enabled in (True, False)]) + @ddt.unpack + def test_full_view_credit_context_equals_v1(self, mount, eligibility_enabled): + CreditCourseFactory(course_key=self.course.id, enabled=True) + with override_settings(ENABLE_CREDIT_ELIGIBILITY=eligibility_enabled): + v1_body = self.client.get(v1_url(self.course.id)).json() + full = self._read(mount, view="full") + assert full == _flatten_v1(v1_body, grace_seconds=0) + assert full["is_credit_course"] is True + assert full["show_credit_eligibility"] is eligibility_enabled + + @ddt.data(*[(mount, legacy_exam_settings) for mount in MOUNTS for legacy_exam_settings in (True, False)]) + @ddt.unpack + def test_full_view_proctoring_link_equals_v1(self, mount, legacy_exam_settings): + with override_settings(COURSE_AUTHORING_MICROFRONTEND_URL="http://authoring.example"), \ + override_waffle_flag(LEGACY_STUDIO_EXAM_SETTINGS, active=legacy_exam_settings): + v1_body = self.client.get(v1_url(self.course.id)).json() + full = self._read(mount, view="full") + assert full == _flatten_v1(v1_body, grace_seconds=0) + expected = "" if legacy_exam_settings else f"/course/{self.course.id}/pages-and-resources/proctoring/settings" + assert full["mfe_proctored_exam_settings_url"] == expected + + @ddt.data(*MOUNTS) + def test_grace_period_seconds_are_shown_where_v1_drops_them(self, mount): + self.course.graceperiod = timedelta(hours=1, seconds=30) + modulestore().update_item(self.course, self.staff.id) + v1_body = self.client.get(v1_url(self.course.id)).json() + full = self._read(mount, view="full") + assert v1_body["course_details"]["grace_period"] == {"hours": 1, "minutes": 0} + assert full["grace_period"] == {"hours": 1, "minutes": 0, "seconds": 30} + assert full == _flatten_v1(v1_body, grace_seconds=30) + assert self._read(mount)["grace_period"] == {"hours": 1, "minutes": 0, "seconds": 30} + + @ddt.data(*MOUNTS) + def test_full_view_without_a_grace_period_equals_v1(self, mount): + course = CourseFactory.create() + v1_body = self.client.get(v1_url(course.id)).json() + full = self.client.get(grading_url(mount, course.id), {"view": "full"}).json() + assert full["grace_period"] is None + assert full == _flatten_v1(v1_body, grace_seconds=None) + + @ddt.data(*[(mount, value) for mount in MOUNTS for value in ("minimal", "bogus", "FULL")]) + @ddt.unpack + def test_unknown_view_is_refused_with_400(self, mount, value): + response = self.client.get(grading_url(mount, self.course.id), {"view": value}) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert list(envelope["errors"]) == ["view"] + + @ddt.data(*MOUNTS) + def test_empty_view_is_the_default_view(self, mount): + assert self._read(mount, view="") == self._read(mount) + + @ddt.data(*MOUNTS) + def test_reads_match_the_published_shapes(self, mount): + """ + Including policies stored with each value the schema marks nullable, and a + course never configured. The read publishes both shapes; a full response + also matches the default one. + """ + nullable_values = _full( + graders=[{**GRADERS[0], "type": None, "short_label": None}], + grade_cutoffs={"A": None, "Pass": 0.5}, + grace_period=None, + minimum_grade_credit="", + ) + cases = ( + (nullable_values, { + "graders": [{**GRADERS[0], "type": None, "short_label": None}], + "grade_cutoffs": {"A": None, "Pass": 0.5}, + "grace_period": {"hours": 2, "minutes": 5, "seconds": 0}, + "minimum_grade_credit": None, + }), + (_full(grade_cutoffs=None), { + "graders": GRADERS, + "grade_cutoffs": None, + "grace_period": {"hours": 3, "minutes": 15, "seconds": 0}, + "minimum_grade_credit": 0.75, + }), + ) + for body, expected in cases: + course = seed_course(self.staff) + with patch(CREDIT_TASK): + assert _patch(self.client, grading_url(mount, course.id), body).status_code == 200 + default = self.client.get(grading_url(mount, course.id)).json() + full = self.client.get(grading_url(mount, course.id), {"view": "full"}).json() + assert default == expected + assert {key: full[key] for key in default} == expected + assert_matches_published_schema(default, DEFAULT_SHAPE) + assert_matches_published_schema(full, FULL_SHAPE) + assert_matches_published_schema(default, READ_SHAPE) + assert_matches_published_schema(full, READ_SHAPE) + + never_configured = CourseFactory.create() + default = self.client.get(grading_url(mount, never_configured.id)).json() + full = self.client.get(grading_url(mount, never_configured.id), {"view": "full"}).json() + assert default["grace_period"] is None + assert_matches_published_schema(default, DEFAULT_SHAPE) + assert_matches_published_schema(full, FULL_SHAPE) + assert_matches_published_schema(default, READ_SHAPE) + assert_matches_published_schema(full, READ_SHAPE) + with pytest.raises(jsonschema.ValidationError): + assert_matches_published_schema(default, FULL_SHAPE) + + @ddt.data(*MOUNTS) + def test_head_answers_without_a_body(self, mount): + response = self.client.head(grading_url(mount, self.course.id)) + assert response.status_code == 200 + assert response.content == b"" -# =========================================================================== -# ADR 0028 — routing checks -# =========================================================================== -class TestAuthoringGradingViewSetRouting(TestCase): +# The one write a database's first role or permission lookup makes. +FIRST_LOOKUP_WRITES = [("INSERT", PolicyCacheControl._meta.db_table)] # pylint: disable=protected-access +READS = [(mount, method, params) for mount in MOUNTS + for method, params in (("get", {}), ("get", {"view": "full"}), ("head", {}))] + + +class _ReadIsSideEffectFreeMixin: """ - Routing checks — confirm the URL namespace and HTTP-method mapping are wired correctly. + Every read (default, full, HEAD, both mounts) on a credit and a non-credit + course writes no row and no Mongo document, stores nothing through the + modulestore, sends no grading or publishing signal, and emits no event + beyond the platform's request tracking. + + The reads start on a database that has never served a role or permission + lookup. The permission library keeps a one-row policy version table and + creates that row on its first lookup against a database, whichever API + makes it, so the first read may insert it and nothing else; the v1 read + inserts the same row. """ - def test_detail_url_resolves(self): - """v3 router exposes the viewset under ``v3:authoring_grading-detail``.""" - url = reverse( - "cms.djangoapps.contentstore:v3:authoring_grading-detail", - kwargs={"course_key": COURSE_ID}, - ) - assert "/api/contentstore/v3/authoring_grading/" in url - assert COURSE_ID in url + def reader_for(self, course): + """Return a user who may read ``course``, leaving the database as no lookup had touched it.""" + raise NotImplementedError - def test_post_not_allowed(self): - """The viewset only exposes PATCH (partial_update); POST must return 405.""" - client = APIClient() - client.force_authenticate(user=UserFactory.create(is_staff=True)) - url = reverse( - "cms.djangoapps.contentstore:v3:authoring_grading-detail", - kwargs={"course_key": COURSE_ID}, - ) - response = client.post(url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED + def _assert_reads_have_no_effects(self, course): + """Read ``course`` every way and assert nothing was written or announced.""" + published = [] + changed = [] + + def on_published(sender, **kwargs): # pylint: disable=unused-argument + published.append(kwargs) + + def on_changed(sender, **kwargs): # pylint: disable=unused-argument + changed.append(kwargs) + client = _client(self.reader_for(course)) + assert not PolicyCacheControl.objects.exists() + SignalHandler.course_published.connect(on_published, weak=False) + GRADING_POLICY_CHANGED.connect(on_changed, weak=False) + try: + for index, (mount, method, params) in enumerate(READS): + url = grading_url(mount, course.id) + with CaptureQueriesContext(connection) as queries, \ + count_mongo_calls() as mongo, \ + patch(UPDATE_ITEM) as update_item, \ + patch("eventtracking.tracker.emit") as emit: + response = getattr(client, method)(url, params) + request = (index, mount, method, params) + assert response.status_code == 200, (request, response.content) + assert sql_writes(queries) == (FIRST_LOOKUP_WRITES if index == 0 else []), request + assert mongo["writes"] == 0, request + assert update_item.call_count == 0, request + # Only the platform's per-request tracking event, as for any request. + assert [call.kwargs["name"] for call in emit.call_args_list] == [url], request + finally: + SignalHandler.course_published.disconnect(on_published) + GRADING_POLICY_CHANGED.disconnect(on_changed) + assert not published + assert not changed -# =========================================================================== -# ADR 0029 — standardized error-response envelope tests -# =========================================================================== -class TestAuthoringGradingViewSetErrorShape(APITestCase): + def test_reading_a_course_has_no_side_effects(self): + self._assert_reads_have_no_effects(seed_course(self.user)) + + def test_reading_a_credit_course_has_no_side_effects(self): + course = seed_course(self.user) + CreditCourseFactory(course_key=course.id, enabled=True) + with override_settings(ENABLE_CREDIT_ELIGIBILITY=True): + self._assert_reads_have_no_effects(course) + + def test_the_first_v1_read_writes_the_same_row(self): + course = seed_course(self.user) + client = _client(self.reader_for(course)) + assert not PolicyCacheControl.objects.exists() + with CaptureQueriesContext(connection) as queries, count_mongo_calls() as mongo: + response = client.get(v1_url(course.id)) + assert response.status_code == 200, response.content + assert sql_writes(queries) == FIRST_LOOKUP_WRITES + assert mongo["writes"] == 0 + + +class GradingReadSideEffectsLegacyPermissionsTest(_ReadIsSideEffectFreeMixin, ModuleStoreTestCase): """ - ADR 0029 — error response shape regression tests for AuthoringGradingViewSet. + Reads by course staff are side-effect free with policy-based authoring + permissions off. Course staff rather than global staff, whose access is + decided without a role lookup. + """ + + def reader_for(self, course): + reader = UserFactory() + CourseStaffRole(course.id).add_users(reader) + return reader - The envelope is wired in via - :class:`openedx.core.lib.api.mixins.StandardizedErrorMixin`, which overrides - DRF's per-view ``get_exception_handler`` to point at - ``openedx.core.lib.api.exceptions.standardized_error_exception_handler``. - Scoped to v3 — the project-wide DRF ``EXCEPTION_HANDLER`` setting is - unchanged, so v0 / v1 / v2 / v4 endpoints continue to return the legacy - error shape (locked in by ``test_v0_endpoint_unaffected_by_v3_envelope``). +class GradingReadSideEffectsAuthzTest(CourseAuthoringAuthzTestMixin, _ReadIsSideEffectFreeMixin, ModuleStoreTestCase): + """ + Reads by a course auditor are side-effect free with policy-based authoring + permissions on. A course auditor rather than a superuser, whom the policy + admits on the account's superuser flag without consulting role assignments. + + The test setup loads the policy and assigns roles through the permission + library, which creates the policy version row; it is deleted before the + reads so they start on a database no lookup has touched. + """ + + 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 + + +# --------------------------------------------------------------------------- +# Changing the policy +# --------------------------------------------------------------------------- + +class _UpdateBehaviourMixin: + """ + The update, checked against the v0 update as the reference: v0 hands any + body to the grading model's whole-policy update as sent, so a whole-policy + body must behave identically here. + + Each concrete class runs on one side of the flag that decides whether a + grading change with no new weights or counts is announced. """ + recompute_only = None + def setUp(self): super().setUp() - self.client = APIClient() - self.url = reverse( - "cms.djangoapps.contentstore:v3:authoring_grading-detail", - kwargs={"course_key": COURSE_ID}, - ) + self.enterContext(override_waffle_flag(MATERIAL_RECOMPUTE_ONLY_FLAG, active=self.recompute_only)) + self.staff = UserFactory(is_staff=True) + self.client = _client(self.staff) - def test_unauthenticated_patch_returns_standardized_401(self): - """Unauthenticated PATCH must return 401 with the ADR 0029 envelope.""" - response = self.client.patch(self.url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_401_UNAUTHORIZED - for field in _REQUIRED_ERROR_FIELDS: - assert field in response.data, f"ADR 0029: missing field '{field}'" - - def test_unauthenticated_401_type_uri(self): - """The ``type`` field for 401 must be the ADR 0029 authn URI.""" - response = self.client.patch(self.url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_401_UNAUTHORIZED - assert response.data.get("type") == "https://docs.openedx.org/errors/authn" - - @patch(MOCK_COURSE_EXISTS, return_value=True) - @patch(MOCK_HAS_PERMISSION, return_value=False) - def test_non_author_patch_returns_standardized_403(self, mock_perm, mock_exists): # noqa: ARG002 - """Authenticated non-author PATCH must return 403 with the ADR 0029 envelope.""" - non_author = UserFactory.create() - self.client.force_authenticate(user=non_author) - response = self.client.patch(self.url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_403_FORBIDDEN - for field in _REQUIRED_ERROR_FIELDS: - assert field in response.data, f"ADR 0029: missing field '{field}'" - - @patch(MOCK_COURSE_EXISTS, return_value=True) - @patch(MOCK_HAS_PERMISSION, return_value=False) - def test_non_author_403_type_uri(self, mock_perm, mock_exists): # noqa: ARG002 - """The ``type`` field for 403 must be the ADR 0029 authz URI.""" - non_author = UserFactory.create() - self.client.force_authenticate(user=non_author) - response = self.client.patch(self.url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_403_FORBIDDEN - assert response.data.get("type") == "https://docs.openedx.org/errors/authz" - - @patch(MOCK_COURSE_EXISTS, return_value=False) - def test_nonexistent_course_returns_standardized_404(self, mock_exists): # noqa: ARG002 - """PATCH for a non-existent course must return 404 with the ADR 0029 envelope.""" - staff = UserFactory.create(is_staff=True) - self.client.force_authenticate(user=staff) - response = self.client.patch(self.url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_404_NOT_FOUND - for field in _REQUIRED_ERROR_FIELDS: - assert field in response.data, f"ADR 0029: missing field '{field}'" - - @patch(MOCK_COURSE_EXISTS, return_value=False) - def test_not_found_type_uri(self, mock_exists): # noqa: ARG002 - """The ``type`` field for 404 must be the ADR 0029 not-found URI.""" - staff = UserFactory.create(is_staff=True) - self.client.force_authenticate(user=staff) - response = self.client.patch(self.url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_404_NOT_FOUND - assert response.data.get("type") == "https://docs.openedx.org/errors/not-found" - - def test_error_body_has_no_developer_message(self): - """Error responses must NOT contain old DeveloperErrorViewMixin fields.""" - response = self.client.patch(self.url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_401_UNAUTHORIZED - assert "developer_message" not in response.data - assert "error_code" not in response.data - - def test_instance_field_is_request_path(self): - """The ``instance`` field must equal the request path.""" - response = self.client.patch(self.url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_401_UNAUTHORIZED - assert response.data.get("instance") == self.url + def _twin_courses(self): + return seed_course(self.staff), seed_course(self.staff) - def test_v0_endpoint_unaffected_by_v3_envelope(self): + def _apply(self, request, course, body): + """Send ``body`` through ``request``; return status, bytes, policy before and after, and effects.""" + before = stored_policy(course.id) + with capture_grading_effects() as effects: + response = request(course, body) + effects["credit_tasks"] = [tuple(arg.replace(str(course.id), "") for arg in args) + for args in effects["credit_tasks"]] + content = response.content.replace(str(course.id).encode(), b"") + return response.status_code, content, before, stored_policy(course.id), effects + + def _v0(self, course, body): + return _post(self.client, v0_url(course.id), body) + + def _v3(self, mount): + return lambda course, body: _patch(self.client, grading_url(mount, course.id), body) + + def test_whole_policy_bodies_behave_as_the_v0_update(self): + for mount, name, body in FULL_BODY_CASES: + with self.subTest(mount=mount, body=name): + reference_course, course = self._twin_courses() + expected = self._apply(self._v0, reference_course, body) + actual = self._apply(self._v3(mount), course, body) + assert expected[0] == 200, expected[1] + assert actual[1] == expected[1] # response bytes + assert actual[3] == expected[3] # stored policy + assert actual[4] == expected[4] # signals, events, credit task + assert_matches_published_schema(json.loads(actual[1]), DEFAULT_SHAPE) + + def test_resending_the_stored_policy_is_announced_only_without_recompute_only(self): + """Pins which side of the flag this class runs on.""" + for mount in MOUNTS: + with self.subTest(mount=mount): + _, course = self._twin_courses() + effects = self._apply(self._v3(mount), course, FULL_BODIES["stored_policy_resent"])[4] + expected = 0 if self.recompute_only else 1 + assert (effects["signals"], effects["events"]) == (expected, expected) + + def test_displayed_weights_are_stored_truncated(self): + """A weight of 0.29 is shown as 28, so sending back what was shown stores 0.28.""" + _, course = self._twin_courses() + body = FULL_BODIES["displayed_weights_resent"] + status, content, _, after, _ = self._apply(self._v3(CONFORMING), course, body) + assert status == 200 + assert json.loads(content)["graders"][0]["weight"] == 28 + assert after["raw_grader"][0]["weight"] == 0.28 + + def test_partial_bodies_change_only_the_fields_sent(self): + seeded_weights = [grader["weight"] for grader in SEED_GRADERS] + cases = { + # name: (body, fields that change, grading signals) + "graders_only": ({"graders": GRADERS}, {"raw_grader"}, 1), + "stored_graders_only": ({"graders": SEED_GRADERS_AS_SENT}, set(), 0 if self.recompute_only else 1), + "cutoffs_only": ({"grade_cutoffs": CUTOFFS}, {"grade_cutoffs"}, 1), + "stored_cutoffs_only": ({"grade_cutoffs": SEED_CUTOFFS}, set(), 0), + "grace_period_only": ({"grace_period": {"hours": 5}}, {"graceperiod"}, 0), + "grace_period_null_only": ({"grace_period": None}, set(), 0), + "credit_only": ({"minimum_grade_credit": 0.6}, {"minimum_grade_credit"}, 0), + "credit_null_only": ({"minimum_grade_credit": None}, set(), 0), + "empty": ({}, set(), 0), + "graders_and_credit": ({"graders": GRADERS, "minimum_grade_credit": 0.55}, + {"raw_grader", "minimum_grade_credit"}, 1), + "cutoffs_and_grace_period": ({"grade_cutoffs": CUTOFFS, "grace_period": {"hours": 1}}, + {"grade_cutoffs", "graceperiod"}, 1), + "graders_cutoffs_and_grace_period": ({"graders": GRADERS, "grade_cutoffs": CUTOFFS, + "grace_period": {"minutes": 9}}, + {"raw_grader", "grade_cutoffs", "graceperiod"}, 1), + } + for mount in MOUNTS: + for name, (body, changed, signals) in cases.items(): + with self.subTest(mount=mount, body=name): + reference_course, course = self._twin_courses() + if set(body) != {"graders", "grade_cutoffs", "grace_period", "minimum_grade_credit"}: + # The whole-policy update cannot take a partial body. + assert self._apply(self._v0, reference_course, body)[0] == 500 + status, content, before, after, effects = self._apply(self._v3(mount), course, body) + assert status == 200, content + assert {field for field in before if before[field] != after[field]} == changed + if "graders" not in body: + assert [grader["weight"] for grader in after["raw_grader"]] == seeded_weights + assert (effects["signals"], effects["events"]) == (signals, signals) + expected_tasks = [("",)] if "minimum_grade_credit" in body else [] + assert effects["credit_tasks"] == expected_tasks + assert json.loads(content) == json.loads( + self.client.get(grading_url(mount, course.id)).content.replace( + str(course.id).encode(), b"") + ) + + def test_partial_bodies_without_graders_never_rewrite_stored_weights(self): """ - The ADR 0029 envelope must be scoped to v3 — hitting the legacy v0 - ``grading`` endpoint unauthenticated must NOT return the v3 envelope - (no ``type`` / ``instance`` keys). + A weight such as 0.1019 does not survive the model's own percentage round trip + (0.1019 * 100 / 100 == 0.10189999...), so any write-back of unsent graders shows. """ - v0_url = reverse( - "cms.djangoapps.contentstore:v0:cms_api_update_grading", - # v0 URL uses the legacy ``course_id`` named group; only v3 was renamed - # to ``course_key`` per OEP-68. - kwargs={"course_id": COURSE_ID}, + drifting = [{**SEED_GRADERS[0], "weight": 0.1019}, *SEED_GRADERS[1:]] + assert drifting[0]["weight"] * 100 / 100 != drifting[0]["weight"] + bodies = ( + {}, {"grade_cutoffs": CUTOFFS}, {"grade_cutoffs": SEED_CUTOFFS}, {"grace_period": {"hours": 1}}, + {"grace_period": None}, {"minimum_grade_credit": 0.6}, {"minimum_grade_credit": None}, ) - response = self.client.post(v0_url, data={}, content_type="application/json") - assert response.status_code == status.HTTP_401_UNAUTHORIZED - assert "type" not in response.data - assert "instance" not in response.data + for mount in MOUNTS: + for body in bodies: + with self.subTest(mount=mount, body=body): + course = seed_course(self.staff) + course.raw_grader = [dict(grader) for grader in drifting] + modulestore().update_item(course, self.staff.id) + status = self._apply(self._v3(mount), course, body)[0] + assert status == 200 + assert stored_policy(course.id)["raw_grader"] == drifting + def test_partial_graders_keep_the_stored_cutoffs_grace_period_and_credit_minimum(self): + _, course = self._twin_courses() + _, _, before, after, _ = self._apply(self._v3(LEGACY), course, {"graders": GRADERS}) + assert after["grade_cutoffs"] == SEED_CUTOFFS + assert after["graceperiod"] == SEED_GRACE + assert after["minimum_grade_credit"] == SEED_CREDIT + assert after["raw_grader"] != before["raw_grader"] -class TestAuthoringGradingViewSetUrlStructure(APITestCase): - """The conforming courses/{course_key}/grading/ route serves the same view as the legacy one.""" + def test_bodies_the_model_cannot_store_are_refused_before_anything_is_written(self): + for mount in MOUNTS: + for name, (body, field) in REJECTED_BODIES.items(): + with self.subTest(mount=mount, body=name): + reference_course, course = self._twin_courses() + assert self._apply(self._v0, reference_course, body)[0] == 500 + with capture_grading_effects() as effects: + response = _patch(self.client, grading_url(mount, course.id), body) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert list(envelope["errors"]) == [field] + assert effects == {"signals": 0, "events": 0, "credit_tasks": []} + _, fresh = self._twin_courses() + assert stored_policy(course.id) == stored_policy(fresh.id) + assert modulestore().get_course(course.id).lowest_passing_grade == min(SEED_CUTOFFS.values()) - def setUp(self): - super().setUp() - self.client = APIClient() - self.conforming_url = reverse( - "authoring_v3:course_grading", - kwargs={"course_key": COURSE_ID}, - ) - self.legacy_url = reverse( - "cms.djangoapps.contentstore:v3:authoring_grading-detail", - kwargs={"course_key": COURSE_ID}, + def test_numbers_too_large_to_store_are_refused_before_anything_is_written(self): + for mount in MOUNTS: + for name, (body, field) in TOO_LARGE_BODIES.items(): + with self.subTest(mount=mount, body=name): + course = seed_course(self.staff) + with capture_grading_effects() as effects: + response = _patch(self.client, grading_url(mount, course.id), body) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert list(envelope["errors"]) == [field] + assert effects == {"signals": 0, "events": 0, "credit_tasks": []} + assert stored_policy(course.id) == stored_policy(seed_course(self.staff).id) + + def test_a_form_encoded_body_is_refused(self): + _, course = self._twin_courses() + response = self.client.patch( + grading_url(CONFORMING, course.id), + data="graders[0]type=Homework&grade_cutoffs=x", + content_type="application/x-www-form-urlencoded", ) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert list(envelope["errors"]) == ["non_field_errors"] + _, fresh = self._twin_courses() + assert stored_policy(course.id) == stored_policy(fresh.id) + + +class WholePolicyBodyCasesTest(TestCase): + """Every whole-policy body is replayed on at least one mount.""" + + def test_hand_picked_bodies_run_on_both_mounts_and_generated_bodies_on_one(self): + mounts_by_body = {} + for mount, name, _ in FULL_BODY_CASES: + mounts_by_body.setdefault(name, []).append(mount) + assert len(FULL_BODIES) == len(HAND_PICKED_FULL_BODIES) + len(GENERATED_FULL_BODIES) + assert set(mounts_by_body) == set(FULL_BODIES) + for name in HAND_PICKED_FULL_BODIES: + assert sorted(mounts_by_body[name]) == sorted(MOUNTS), name + for name in GENERATED_FULL_BODIES: + assert len(mounts_by_body[name]) == 1, name + per_mount = [sum(mounts_by_body[name] == [mount] for name in GENERATED_FULL_BODIES) for mount in MOUNTS] + assert max(per_mount) - min(per_mount) <= 1, per_mount + + +class GradingUpdateTest(_UpdateBehaviourMixin, ModuleStoreTestCase): + """Updates when every grading change is announced.""" + + recompute_only = False + + +class GradingUpdateRecomputeOnlyTest(_UpdateBehaviourMixin, ModuleStoreTestCase): + """Updates when only changes to weights or counts are announced.""" + + recompute_only = True + + +# --------------------------------------------------------------------------- +# Routing +# --------------------------------------------------------------------------- + +COURSE_KEY = "course-v1:edX+ToyX+Toy_Course" + + +@ddt.ddt +class GradingRoutingTest(SharedModuleStoreTestCase): + """Both addresses serve one view with the same actions.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def test_detail_url_resolves(self): + assert grading_url(LEGACY, COURSE_KEY) == f"/api/contentstore/v3/authoring_grading/{COURSE_KEY}/" def test_conforming_url_reverses_to_expected_path(self): - assert self.conforming_url == f"/api/authoring/v3/courses/{COURSE_ID}/grading/" + assert grading_url(CONFORMING, COURSE_KEY) == f"/api/authoring/v3/courses/{COURSE_KEY}/grading/" def test_conforming_and_legacy_routes_share_view_and_actions(self): - legacy = resolve(self.legacy_url).func - conforming = resolve(self.conforming_url).func + legacy = resolve(grading_url(LEGACY, COURSE_KEY)).func + conforming = resolve(grading_url(CONFORMING, COURSE_KEY)).func assert conforming.cls is legacy.cls - assert routed_actions(conforming) == routed_actions(legacy) + assert routed_actions(conforming) == routed_actions(legacy) == { + "get": "retrieve", + "patch": "partial_update", + } - def test_invalid_course_key_is_404_on_conforming_route(self): - # The shared course_key converter rejects unparseable keys with a - # routing-level 404. - response = self.client.patch( - "/api/authoring/v3/courses/not-a-course-key/grading/", - data={}, format="json", + def test_legacy_format_suffix_reads_and_updates_like_the_plain_address(self): + """The legacy router also routes ``.json`` to the same actions.""" + client = _client(UserFactory(is_staff=True)) + suffixed = reverse( + "cms.djangoapps.contentstore:v3:authoring_grading-detail", + kwargs={"course_key": str(self.course.id), "format": "json"}, + ) + assert suffixed == f"/api/contentstore/v3/authoring_grading/{self.course.id}.json" + assert resolve(suffixed).func.cls is resolve(grading_url(LEGACY, self.course.id)).func.cls + + read = client.get(suffixed) + assert read.status_code == 200, read.content + assert read.json() == client.get(grading_url(LEGACY, self.course.id)).json() + + with patch(CREDIT_TASK): + updated = _patch(client, suffixed, {"grace_period": {"hours": 7}}) + assert updated.status_code == 200, updated.content + assert updated.json()["grace_period"] == {"hours": 7, "minutes": 0, "seconds": 0} + assert updated.json() == client.get(grading_url(LEGACY, self.course.id)).json() + + # The error library does not catalogue MethodNotAllowed, ParseError, + # UnsupportedMediaType or NotAcceptable, so these four answer with the + # envelope typed internal. Change the expected type here once it does. + + @ddt.data(*[(mount, method) for mount in MOUNTS for method in ("post", "put", "delete")]) + @ddt.unpack + def test_other_methods_are_not_allowed(self, mount, method): + client = _client(UserFactory(is_staff=True)) + response = getattr(client, method)(grading_url(mount, self.course.id), data={}, format="json") + assert_error_envelope(response, expected_status=405, expected_type_slug="internal") + + @ddt.data(*MOUNTS) + def test_malformed_json_is_refused_with_400(self, mount): + client = _client(UserFactory(is_staff=True)) + response = client.patch( + grading_url(mount, self.course.id), data='{"graders": [', content_type="application/json", ) - assert response.status_code == status.HTTP_404_NOT_FOUND + assert_error_envelope(response, expected_status=400, expected_type_slug="internal") + + @ddt.data(*MOUNTS) + def test_unsupported_body_type_is_refused_with_415(self, mount): + client = _client(UserFactory(is_staff=True)) + response = client.patch(grading_url(mount, self.course.id), data="graders", content_type="text/plain") + assert_error_envelope(response, expected_status=415, expected_type_slug="internal") + + @ddt.data(*MOUNTS) + def test_unsupported_accept_is_refused_with_406(self, mount): + client = _client(UserFactory(is_staff=True)) + response = client.get(grading_url(mount, self.course.id), HTTP_ACCEPT="application/xml") + assert_error_envelope(response, expected_status=406, expected_type_slug="internal") + + # 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 each mount + # answers unknown addresses. - def test_version_only_course_key_is_404_on_conforming_route(self): + @ddt.data(*[(key, method) for key in ("not-a-course-key", "edX/ToyX/Toy_Course") for method in ("get", "patch")]) + @ddt.unpack + def test_unparsable_or_slash_form_key_is_404_on_conforming_route(self, key, method): + client = _client(UserFactory(is_staff=True)) + response = getattr(client, method)(f"/api/authoring/v3/courses/{key}/grading/", data={}, format="json") + assert response.status_code == 404 + assert response["Content-Type"].startswith("text/html") + + @ddt.data("get", "patch") + def test_key_without_separators_is_404_on_legacy_route(self, method): + client = _client(UserFactory(is_staff=True)) + response = getattr(client, method)("/api/contentstore/v3/authoring_grading/not-a-key/", data={}, format="json") + assert response.status_code == 404 + assert response["Content-Type"].startswith("text/html") + + @ddt.data("get", "patch") + def test_version_only_course_key_is_404_on_conforming_route(self, method): # The converter accepts a key that names only a version. No course run # is behind it, so the view answers 404 rather than 500. - self.client.force_authenticate(user=UserFactory.create()) - response = self.client.patch( - "/api/authoring/v3/courses/course-v1:version@0123456789abcdef01234567/grading/", - data={}, format="json", + client = _client(UserFactory()) + response = getattr(client, method)( + "/api/authoring/v3/courses/course-v1:version@0123456789abcdef01234567/grading/", data={}, format="json", ) - assert response.status_code == status.HTTP_404_NOT_FOUND - - def test_unauthenticated_patch_returns_401(self): - response = self.client.patch(self.conforming_url, data={}, format="json") - assert response.status_code == status.HTTP_401_UNAUTHORIZED - - @patch(MOCK_CREDIT_TASK) - @patch(MOCK_UPDATE_FROM_JSON, return_value=_MOCK_GRADING_MODEL) - @patch(MOCK_HAS_PERMISSION, return_value=True) - @patch(MOCK_COURSE_EXISTS, return_value=True) - def test_patch_on_conforming_route_updates_grading( - self, mock_exists, mock_perm, mock_update, mock_credit, # noqa: ARG002 - ): - """Same contract on the conforming mount as on the legacy one.""" - user = UserFactory.create() - self.client.force_authenticate(user=user) - response = self.client.patch( - self.conforming_url, - data={"graders": _GRADERS_PAYLOAD}, - format="json", - ) - assert response.status_code == status.HTTP_200_OK - mock_update.assert_called_once() + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + def test_v0_endpoint_unaffected_by_v3_envelope(self): + """The frozen v0 update keeps its own error body.""" + response = _client().post(v0_url(COURSE_KEY), data={}, format="json") + assert response.status_code == 401 + assert "type" not in response.data + assert "instance" not in response.data + assert response.data["developer_message"] == "Authentication credentials were not provided." + + +# --------------------------------------------------------------------------- +# Query counts +# --------------------------------------------------------------------------- + +class _QueryCountMixin: + """ + SQL and Mongo calls per request, measured on the second request so request + caches are warm. A read costs no more than the v1 read, and the full view + costs the same. + """ + + reader = None + + def setUp(self): + super().setUp() + self.client = _client(self.reader) + self.course = CourseFactory.create() + for index in range(2): + section = BlockFactory.create(parent=self.course, category="chapter", display_name=f"S{index}") + BlockFactory.create(parent=section, category="sequential", format="Homework", graded=True) + + def _measure(self, 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"] + + def _v1_read(self): + return self._measure(lambda: self.client.get(v1_url(self.course.id))) + + def _read(self, mount, **params): + return self._measure(lambda: self.client.get(grading_url(mount, self.course.id), params)) + + def test_default_read_costs_no_more_than_the_v1_read(self): + v1_sql, v1_mongo_reads, v1_mongo_writes = self._v1_read() + for mount in MOUNTS: + sql, mongo_reads, mongo_writes = self._read(mount) + assert sql <= v1_sql + assert mongo_reads <= v1_mongo_reads + assert mongo_writes == v1_mongo_writes == 0 + + def test_full_read_costs_the_same_as_the_v1_read(self): + v1 = self._v1_read() + for mount in MOUNTS: + assert self._read(mount, view="full") == v1 + + +@ddt.ddt +class GradingQueryCountTest(_QueryCountMixin, ModuleStoreTestCase): + """ + Pinned counts with policy-based authoring permissions off: SQL queries, + Mongo reads and Mongo writes per request. + """ + + def setUp(self): + self.reader = UserFactory(is_staff=True) + super().setUp() + + @ddt.data(*MOUNTS) + def test_default_read(self, mount): + assert self._read(mount) == (12, 4, 0) + assert self._v1_read() == (14, 4, 0) + + @ddt.data(*MOUNTS) + def test_full_read(self, mount): + assert self._read(mount, view="full") == (14, 4, 0) + + @ddt.data(*MOUNTS) + def test_whole_policy_update(self, mount): + body = { + "graders": [ + {"type": "Homework", "min_count": 2, "drop_count": 0, "short_label": "HW", "weight": 60, "id": 0}, + {"type": "Exam", "min_count": 1, "drop_count": 0, "short_label": "Ex", "weight": 40, "id": 1}, + ], + "grade_cutoffs": {"Pass": 0.5}, + "grace_period": {"hours": 1, "minutes": 0}, + "minimum_grade_credit": 0.8, + } + counts = self._measure(lambda: _patch(self.client, grading_url(mount, self.course.id), body)) + assert counts == (33, 42, 6) + + +class GradingQueryCountAuthzTest(CourseAuthoringAuthzTestMixin, _QueryCountMixin, ModuleStoreTestCase): + """Read costs against the v1 read with policy-based authoring permissions on.""" + + def setUp(self): + super().setUp() + self.client = _client(self.super_user) diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 662cf80c65f0..22adaf56f112 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -5,14 +5,16 @@ 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. +# The YouTube transcript routes make their final character optional, so both +# addresses resolve and each is listed in the truncated spelling as well as the +# full one: which one the generator emits depends on how it renders that regex. 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/contentstore/v0/grading/{course_id}", # → /api/authoring/v3/courses/{course_key}/grading/ + "/api/contentstore/v1/course_grading/{course_id}", # → /api/authoring/v3/courses/{course_key}/grading/ ) # Legacy addresses of APIs migrated to /api/authoring/, marked deprecated for @@ -39,6 +41,17 @@ "/api/authoring/v3/home/", ) +# Response components whose alternative shapes overlap, published as anyOf +# instead of drf-spectacular's oneOf. The default grading shape allows extra +# keys, so a full read matches both shapes and a strict oneOf validator would +# reject it. +ANY_OF_COMPONENTS = { + "authoring_grading.CourseGradingRead.v3": ( + "authoring_grading.CourseGradingFull.v3", + "authoring_grading.CourseGrading.v3", + ), +} + def cms_api_filter(endpoints): """ @@ -90,11 +103,25 @@ def get_operation_id(self): return operation_id +def _publish_overlapping_shapes_as_any_of(result): + """Turn the oneOf of each component in ANY_OF_COMPONENTS into anyOf, if it lists exactly the named shapes.""" + schemas = result.get("components", {}).get("schemas", {}) + for name, shapes in ANY_OF_COMPONENTS.items(): + component = schemas.get(name) + if not component or "oneOf" not in component: + continue + refs = [item.get("$ref") for item in component["oneOf"]] + if refs == [f"#/components/schemas/{shape}" for shape in shapes]: + component["anyOf"] = component.pop("oneOf") + + def cms_mark_migrated_paths(result, generator, request, public): # pylint: disable=unused-argument """ Post-processing hook: mark the legacy addresses of - migrated APIs ``deprecated: true`` and BFF surfaces ``x-internal``. + migrated APIs ``deprecated: true`` and BFF surfaces ``x-internal``, + and publish the components in ANY_OF_COMPONENTS as anyOf. """ + _publish_overlapping_shapes_as_any_of(result) for path, path_item in result.get("paths", {}).items(): legacy = path.startswith(LEGACY_MIGRATED_PATH_PREFIXES) internal = path.startswith(INTERNAL_BFF_PATH_PREFIXES) diff --git a/cms/lib/tests/test_spectacular.py b/cms/lib/tests/test_spectacular.py index 4581f23a6148..359ab46d0420 100644 --- a/cms/lib/tests/test_spectacular.py +++ b/cms/lib/tests/test_spectacular.py @@ -1,9 +1,13 @@ """Tests for the CMS drf-spectacular hooks and schema class.""" +from importlib import import_module from pathlib import Path from unittest import TestCase, mock +from django.conf import settings from django.test import SimpleTestCase +from django.test import TestCase as DjangoTestCase +from django.urls import include from django.urls import path as url_path from drf_spectacular.generators import SchemaGenerator from drf_spectacular.settings import spectacular_settings @@ -12,6 +16,7 @@ from rest_framework.settings import api_settings from cms.lib.spectacular import ( + ANY_OF_COMPONENTS, SUPERSEDED_PATHS, CmsAutoSchema, cms_api_filter, @@ -76,6 +81,8 @@ 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/grading/{course_id}", + "/api/contentstore/v1/course_grading/{course_id}", } def test_the_conforming_paths_are_left_unmarked(self): @@ -134,6 +141,42 @@ def test_paths_are_left_as_full_urls(self): assert list(result["paths"]) == ["/api/contentstore/v1/xblock/"] +def _alternatives(*names): + return [{"$ref": f"#/components/schemas/{name}"} for name in names] + + +class OverlappingShapesTest(SimpleTestCase): + """The post-processing hook publishes the listed overlapping response shapes as anyOf, and nothing else.""" + + READ = "authoring_grading.CourseGradingRead.v3" + SHAPES = ("authoring_grading.CourseGradingFull.v3", "authoring_grading.CourseGrading.v3") + + def _run(self, schemas): + result = {"paths": {}, "components": {"schemas": schemas}} + return cms_mark_migrated_paths(result, None, None, True)["components"]["schemas"] + + def test_only_the_grading_read_is_listed(self): + assert ANY_OF_COMPONENTS == {self.READ: self.SHAPES} + + def test_the_grading_read_becomes_any_of(self): + schemas = self._run({self.READ: {"oneOf": _alternatives(*self.SHAPES)}}) + assert schemas[self.READ] == {"anyOf": _alternatives(*self.SHAPES)} + + def test_other_one_of_components_are_left_alone(self): + other = {"oneOf": _alternatives("A", "B")} + schemas = self._run({"Other": other, self.READ: {"oneOf": _alternatives(*self.SHAPES)}}) + assert schemas["Other"] == {"oneOf": _alternatives("A", "B")} + + def test_the_grading_read_with_other_shapes_is_left_alone(self): + """A change to the shapes the read offers needs a fresh look at whether they overlap.""" + for shapes in (self.SHAPES[::-1], self.SHAPES[:1], (*self.SHAPES, "authoring_grading.Other.v3")): + schemas = self._run({self.READ: {"oneOf": _alternatives(*shapes)}}) + assert schemas[self.READ] == {"oneOf": _alternatives(*shapes)}, shapes + + def test_a_schema_without_components_is_accepted(self): + assert cms_mark_migrated_paths({"paths": {}}, None, None, True) == {"paths": {}} + + class SpectacularSettingsTest(TestCase): """ The service settings the hooks depend on. @@ -210,3 +253,139 @@ def test_only_the_colliding_legacy_address_is_suffixed(self): # Already distinct from its conforming twin's id, so it keeps its name. assert ids["/api/authoring/v4/courses/"] == "v4_courses_list" assert ids["/api/contentstore/v4/home/courses/"] == "v4_home_courses_list" + + +class GradingSchemaTest(DjangoTestCase): + """The grading operations as published, generated from the real contentstore and authoring routes.""" + + CONFORMING = "/api/authoring/v3/courses/{course_key}/grading/" + LEGACY_V3 = "/api/contentstore/v3/authoring_grading/{course_key}/" + V1 = "/api/contentstore/v1/course_grading/{course_id}" + V0 = "/api/contentstore/v0/grading/{course_id}" + + @classmethod + def setUpClass(cls): + super().setUpClass() + # Importing the service URLconf registers the course key converter the authoring routes use. + import_module(settings.ROOT_URLCONF) + patterns = [ + url_path("api/contentstore/", include("cms.djangoapps.contentstore.rest_api.urls")), + url_path("api/authoring/v3/", include("cms.djangoapps.contentstore.rest_api.v3.authoring_urls")), + ] + cls.schema = SchemaGenerator(patterns=patterns).get_schema(request=None, public=True) + + def _operation(self, path, method): + return self.schema["paths"][path][method] + + @staticmethod + def _ref(response_or_body): + return response_or_body["content"]["application/json"]["schema"]["$ref"].rsplit("/", 1)[-1] + + def test_the_superseded_operations_are_deprecated(self): + assert self._operation(self.V1, "get")["deprecated"] is True + assert self._operation(self.V1, "post")["deprecated"] is True + assert self._operation(self.V0, "post")["deprecated"] is True + + def test_the_read_on_the_conforming_address_is_published_for_the_sdk(self): + operation = self._operation(self.CONFORMING, "get") + assert "deprecated" not in operation + assert operation["operationId"] == "v3_courses_grading_retrieve" + assert operation["tags"] == ["openedx-platform-sdk"] + view = [p for p in operation["parameters"] if p["name"] == "view"] + assert len(view) == 1 + assert view[0]["in"] == "query" + assert "required" not in view[0] + assert view[0]["schema"]["enum"] == ["full"] + assert self._ref(operation["responses"]["200"]) == "authoring_grading.CourseGradingRead.v3" + for status in ("400", "401", "403", "404"): + assert self._ref(operation["responses"][status]) == "ErrorResponse", status + + def test_the_read_on_the_legacy_address_is_deprecated(self): + operation = self._operation(self.LEGACY_V3, "get") + assert operation["deprecated"] is True + assert operation["operationId"] == "v3_authoring_grading_retrieve" + + def test_the_update_takes_the_update_body_and_returns_the_policy(self): + for path in (self.CONFORMING, self.LEGACY_V3): + operation = self._operation(path, "patch") + assert self._ref(operation["requestBody"]) == "Patchedauthoring_grading.CourseGradingUpdate.v3", path + assert self._ref(operation["responses"]["200"]) == "authoring_grading.CourseGrading.v3", path + for status in ("400", "401", "403", "404"): + assert self._ref(operation["responses"][status]) == "ErrorResponse", (path, status) + assert "deprecated" not in self._operation(self.CONFORMING, "patch") + assert self._operation(self.LEGACY_V3, "patch")["deprecated"] is True + + def test_the_update_body_requires_nothing_but_a_type_per_grader(self): + components = self.schema["components"]["schemas"] + assert "required" not in components["Patchedauthoring_grading.CourseGradingUpdate.v3"] + assert components["authoring_grading.GraderUpdate.v3"]["required"] == ["type"] + assert "required" not in components["authoring_grading.GracePeriodUpdate.v3"] + + def test_the_update_body_is_json_only(self): + for path in (self.CONFORMING, self.LEGACY_V3): + assert list(self._operation(path, "patch")["requestBody"]["content"]) == ["application/json"], path + + def test_the_update_body_types(self): + components = self.schema["components"]["schemas"] + update = components["Patchedauthoring_grading.CourseGradingUpdate.v3"]["properties"] + assert update["grade_cutoffs"]["additionalProperties"] == {"type": "number", "format": "double"} + assert update["grade_cutoffs"]["minProperties"] == 1 + assert "nullable" not in update["grade_cutoffs"] + assert {key: update["minimum_grade_credit"][key] for key in ("type", "format", "nullable")} == { + "type": "number", "format": "double", "nullable": True, + } + assert "nullable" not in update["graders"] + assert update["grace_period"]["allOf"] == [ + {"$ref": "#/components/schemas/authoring_grading.GracePeriodUpdate.v3"}, + ] + assert update["grace_period"]["nullable"] is True + grace = components["authoring_grading.GracePeriodUpdate.v3"]["properties"] + assert {unit: (grace[unit]["type"], grace[unit]["default"]) for unit in grace} == { + "hours": ("integer", 0), "minutes": ("integer", 0), "seconds": ("integer", 0), + } + + def test_the_read_documents_the_full_and_the_default_shape(self): + components = self.schema["components"]["schemas"] + # anyOf, as a full response also matches the default shape. Full first, so a + # client trying each shape in order reads a full response as full. + assert components["authoring_grading.CourseGradingRead.v3"] == {"anyOf": [ + {"$ref": "#/components/schemas/authoring_grading.CourseGradingFull.v3"}, + {"$ref": "#/components/schemas/authoring_grading.CourseGrading.v3"}, + ]} + for path in (self.CONFORMING, self.LEGACY_V3): + response = self._operation(path, "get")["responses"]["200"] + assert self._ref(response) == "authoring_grading.CourseGradingRead.v3", path + policy = {"graders", "grade_cutoffs", "grace_period", "minimum_grade_credit"} + extras = { + "course_assignment_lists", "default_grade_designations", "is_credit_course", + "show_credit_eligibility", "mfe_proctored_exam_settings_url", + } + default = components["authoring_grading.CourseGrading.v3"] + full = components["authoring_grading.CourseGradingFull.v3"] + assert set(default["properties"]) == set(default["required"]) == policy + assert set(full["properties"]) == set(full["required"]) == policy | extras + + def test_the_policy_response_marks_what_is_always_present_and_what_can_be_null(self): + components = self.schema["components"]["schemas"] + + def nullable(component): + properties = components[component]["properties"] + return {name for name, schema in properties.items() if schema.get("nullable")} + + for component in ("authoring_grading.CourseGrading.v3", "authoring_grading.CourseGradingFull.v3"): + assert nullable(component) == {"grade_cutoffs", "grace_period", "minimum_grade_credit"}, component + cutoffs = components[component]["properties"]["grade_cutoffs"] + assert cutoffs["additionalProperties"] == {"type": "number", "format": "double", "nullable": True} + grader = components["authoring_grading.Grader.v3"] + assert set(grader["required"]) == set(grader["properties"]) == { + "type", "min_count", "drop_count", "short_label", "weight", "id", + } + assert nullable("authoring_grading.Grader.v3") == {"type", "short_label"} + grace = components["authoring_grading.GracePeriod.v3"] + assert grace["required"] == ["hours", "minutes", "seconds"] + assert nullable("authoring_grading.GracePeriod.v3") == set() + + def test_only_get_and_patch_are_published_on_the_grading_addresses(self): + for path in (self.CONFORMING, self.LEGACY_V3): + methods = {key for key in self.schema["paths"][path] if key != "parameters"} + assert methods == {"get", "patch"}, path From 2bb98cdabb6cd118efc9dca1f60d0a5f9c06d7d8 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 13 Oct 2026 23:40:23 +0500 Subject: [PATCH 2/2] docs: mark v1 and v0 course grading deprecated in favour of authoring v3 Docstring-only markers naming the v3 successor and DEPR #39128. No statement, decorator or import changes; with docstrings stripped the modules match the base. Co-Authored-By: Claude Opus 5.5 --- .../rest_api/v0/views/authoring_grading.py | 6 ++++++ .../contentstore/rest_api/v1/views/grading.py | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/authoring_grading.py b/cms/djangoapps/contentstore/rest_api/v0/views/authoring_grading.py index 7aabd8eea6b1..94f04a2c7055 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/authoring_grading.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/authoring_grading.py @@ -21,6 +21,9 @@ class AuthoringGradingView(DeveloperErrorViewMixin, APIView): """ View for getting and setting the advanced settings for a course. + + Deprecated: use ``PATCH /api/authoring/v3/courses/{course_key}/grading/`` + instead. See https://github.com/openedx/openedx-platform/issues/39128. """ @extend_schema( request=CourseGradingModelSerializer, @@ -42,6 +45,9 @@ def post(self, request: Request, course_key: CourseKey): """ Update a course's grading. + Deprecated: use ``PATCH /api/authoring/v3/courses/{course_key}/grading/``, + which accepts this body unchanged. See https://github.com/openedx/openedx-platform/issues/39128. + **Example Request** POST /api/contentstore/v0/course_grading/{course_id} diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/grading.py b/cms/djangoapps/contentstore/rest_api/v1/views/grading.py index a460ee07f2c2..8668965d7765 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/grading.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/grading.py @@ -25,6 +25,9 @@ class CourseGradingView(DeveloperErrorViewMixin, APIView): """ View for Course Grading policy configuration. + + Deprecated: use ``GET`` and ``PATCH /api/authoring/v3/courses/{course_key}/grading/`` + instead. See https://github.com/openedx/openedx-platform/issues/39128. """ @extend_schema( @@ -44,6 +47,10 @@ def get(self, request: Request, course_key: CourseKey): """ Get an object containing course grading settings with model. + Deprecated: use ``GET /api/authoring/v3/courses/{course_key}/grading/?view=full``, + which returns the same fields with the grading policy at the top level. + See https://github.com/openedx/openedx-platform/issues/39128. + **Example Request** GET /api/contentstore/v1/course_grading/{course_id} @@ -124,6 +131,9 @@ def post(self, request: Request, course_key: CourseKey): """ Update a course's grading. + Deprecated: use ``PATCH /api/authoring/v3/courses/{course_key}/grading/``, + which accepts this body unchanged. See https://github.com/openedx/openedx-platform/issues/39128. + **Example Request** POST /api/contentstore/v1/course_grading/{course_id}