From 3cc37304059a9f5afde01e300876d6030a8d28db Mon Sep 17 00:00:00 2001 From: Brian Buck Date: Fri, 4 Sep 2026 14:11:48 -0600 Subject: [PATCH 1/2] feat: Add CCX schedule endpoints --- lms/djangoapps/ccx/api/v2/serializers.py | 6 + .../ccx/api/v2/tests/test_schedule_views.py | 126 +++++++++ lms/djangoapps/ccx/api/v2/urls.py | 15 ++ lms/djangoapps/ccx/api/v2/views.py | 143 ++++++++++- lms/djangoapps/ccx/tests/test_views.py | 9 +- lms/djangoapps/ccx/utils.py | 239 +++++++++++++++++- lms/djangoapps/ccx/views.py | 165 +----------- 7 files changed, 532 insertions(+), 171 deletions(-) create mode 100644 lms/djangoapps/ccx/api/v2/tests/test_schedule_views.py diff --git a/lms/djangoapps/ccx/api/v2/serializers.py b/lms/djangoapps/ccx/api/v2/serializers.py index 8ccf29698694..39ca1202c57d 100644 --- a/lms/djangoapps/ccx/api/v2/serializers.py +++ b/lms/djangoapps/ccx/api/v2/serializers.py @@ -101,3 +101,9 @@ class CreateCCXRequestSerializer(serializers.Serializer): # pylint: disable=abs """Validate the `create_ccx` POST body: `{ "name": str }`.""" name = serializers.CharField(max_length=255, allow_blank=False, trim_whitespace=True) + + +class RemoveScheduleRequestSerializer(serializers.Serializer): # pylint: disable=abstract-method + """Validate the `remove_schedule` POST body: `{ "location": str }`.""" + + location = serializers.CharField(allow_blank=False, trim_whitespace=True) diff --git a/lms/djangoapps/ccx/api/v2/tests/test_schedule_views.py b/lms/djangoapps/ccx/api/v2/tests/test_schedule_views.py new file mode 100644 index 000000000000..29b6f5e6fa53 --- /dev/null +++ b/lms/djangoapps/ccx/api/v2/tests/test_schedule_views.py @@ -0,0 +1,126 @@ +""" +Tests for the CCX Coach API v2 schedule endpoints. +""" + +from ccx_keys.locator import CCXLocator +from django.test.utils import override_settings +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient + +from common.djangoapps.student.tests.factories import UserFactory +from lms.djangoapps.ccx.tests.utils import CcxTestCase + + +class ScheduleTestMixin: + """Shared setup for the schedule endpoint tests.""" + + endpoint_name = None + + def setUp(self): + super().setUp() + self.make_coach() + self.ccx = self.make_ccx() + self.ccx_key = CCXLocator.from_course_locator(self.course.id, str(self.ccx.id)) + self.api_client = APIClient() + self.api_client.force_authenticate(user=self.coach) + + def _url(self, course_id): + return reverse(f'ccx_coach_api_v2:{self.endpoint_name}', kwargs={'course_id': str(course_id)}) + + +@override_settings(CUSTOM_COURSES_EDX=True) +class CCXCoachV2ScheduleGetViewTest(ScheduleTestMixin, CcxTestCase): + """Tests for `GET /api/ccx_coach/v2/courses/{ccxId}/schedule`.""" + + endpoint_name = 'schedule' + + def test_returns_schedule_tree(self): + response = self.api_client.get(self._url(self.ccx_key)) + + assert response.status_code == status.HTTP_200_OK + # one node per master-course section, each with subsection children + assert len(response.data) == len(self.chapters) + first = response.data[0] + assert {'location', 'display_name', 'category', 'start', 'hidden'} <= set(first.keys()) + assert 'children' in first + + def test_master_course_id_rejected(self): + response = self.api_client.get(self._url(self.course.id)) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_non_coach_forbidden(self): + self.api_client.force_authenticate(user=UserFactory.create()) + response = self.api_client.get(self._url(self.ccx_key)) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_requires_authentication(self): + self.api_client.force_authenticate(user=None) + response = self.api_client.get(self._url(self.ccx_key)) + assert response.status_code in (status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN) + + +@override_settings(CUSTOM_COURSES_EDX=True) +class CCXCoachV2RemoveScheduleViewTest(ScheduleTestMixin, CcxTestCase): + """Tests for `POST /api/ccx_coach/v2/courses/{ccxId}/remove_schedule`.""" + + endpoint_name = 'remove_schedule' + + def test_remove_hides_block_and_descendants(self): + location = str(self.chapters[0].location) + + response = self.api_client.post(self._url(self.ccx_key), {'location': location}, format='json') + + assert response.status_code == status.HTTP_200_OK + node = next(n for n in response.data if n['location'] == location) + assert node['hidden'] is True + for child in node.get('children', []): + assert child['hidden'] is True + + def test_missing_location_returns_400(self): + response = self.api_client.post(self._url(self.ccx_key), {}, format='json') + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_unknown_location_returns_400(self): + bogus = str(self.course.id.make_usage_key('chapter', 'does_not_exist')) + response = self.api_client.post(self._url(self.ccx_key), {'location': bogus}, format='json') + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.data.get('error_code') == 'schedule_block_not_found' + + def test_non_coach_forbidden(self): + self.api_client.force_authenticate(user=UserFactory.create()) + response = self.api_client.post( + self._url(self.ccx_key), {'location': str(self.chapters[0].location)}, format='json' + ) + assert response.status_code == status.HTTP_403_FORBIDDEN + + +@override_settings(CUSTOM_COURSES_EDX=True) +class CCXCoachV2SaveScheduleViewTest(ScheduleTestMixin, CcxTestCase): + """Tests for `POST /api/ccx_coach/v2/courses/{ccxId}/save_schedule`.""" + + endpoint_name = 'save_schedule' + + def test_save_hides_section_and_returns_payload(self): + location = str(self.chapters[0].location) + payload = [{'location': location, 'hidden': True, 'start': ''}] + + response = self.api_client.post(self._url(self.ccx_key), payload, format='json') + + assert response.status_code == status.HTTP_200_OK + assert 'schedule' in response.data + assert 'grading_policy' in response.data + node = next(n for n in response.data['schedule'] if n['location'] == location) + assert node['hidden'] is True + + def test_invalid_payload_returns_400(self): + response = self.api_client.post(self._url(self.ccx_key), {'not': 'a list'}, format='json') + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.data.get('error_code') == 'invalid_schedule_payload' + + def test_unknown_location_returns_json_400(self): + bogus = str(self.course.id.make_usage_key('chapter', 'does_not_exist')) + payload = [{'location': bogus, 'hidden': True, 'start': ''}] + response = self.api_client.post(self._url(self.ccx_key), payload, format='json') + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.data.get('error_code') == 'invalid_schedule_payload' diff --git a/lms/djangoapps/ccx/api/v2/urls.py b/lms/djangoapps/ccx/api/v2/urls.py index 73078c2a485f..3bbfcb17df5c 100644 --- a/lms/djangoapps/ccx/api/v2/urls.py +++ b/lms/djangoapps/ccx/api/v2/urls.py @@ -21,4 +21,19 @@ views.CreateCCXView.as_view(), name='create_ccx', ), + re_path( + fr'^courses/{settings.COURSE_ID_PATTERN}/schedule$', + views.CCXScheduleView.as_view(), + name='schedule', + ), + re_path( + fr'^courses/{settings.COURSE_ID_PATTERN}/save_schedule$', + views.SaveScheduleView.as_view(), + name='save_schedule', + ), + re_path( + fr'^courses/{settings.COURSE_ID_PATTERN}/remove_schedule$', + views.RemoveScheduleView.as_view(), + name='remove_schedule', + ), ] diff --git a/lms/djangoapps/ccx/api/v2/views.py b/lms/djangoapps/ccx/api/v2/views.py index 943418578457..6591adbfc618 100644 --- a/lms/djangoapps/ccx/api/v2/views.py +++ b/lms/djangoapps/ccx/api/v2/views.py @@ -10,6 +10,7 @@ `DeveloperErrorViewMixin`, JWT/session auth) and reuse existing CCX logic. """ +import json import logging from ccx_keys.locator import CCXLocator @@ -25,9 +26,20 @@ from lms.djangoapps.ccx.api.v0.views import get_valid_course from lms.djangoapps.ccx.api.v2.permissions import IsCCXCoach -from lms.djangoapps.ccx.api.v2.serializers import CCXCoachMetadataSerializer, CreateCCXRequestSerializer -from lms.djangoapps.ccx.utils import create_ccx_course, get_ccx_for_coach +from lms.djangoapps.ccx.api.v2.serializers import ( + CCXCoachMetadataSerializer, + CreateCCXRequestSerializer, + RemoveScheduleRequestSerializer, +) +from lms.djangoapps.ccx.utils import ( + create_ccx_course, + get_ccx_for_coach, + get_ccx_schedule, + remove_block_from_ccx_schedule, + save_ccx_schedule, +) from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin +from openedx.core.lib.courses import get_course_by_id log = logging.getLogger(__name__) @@ -46,15 +58,31 @@ def _error_response(error_code, http_status, field_errors=None): return Response(payload, status=http_status) +def _resolve_ccx_course(course_id): + """ + Resolve a CCX course id to `(master_course, ccx, error_response)`. + + `master_course` is the master :class:`CourseBlock` (loaded with full + depth for schedule traversal) and `ccx` is the + :class:`CustomCourseForEdX`. On failure, `error_response` is a DRF + `Response` and the first two values are `None`. + """ + ccx, ccx_key, error_code, http_status = get_valid_course(course_id, is_ccx=True) + if error_code: + return None, None, _error_response(error_code, http_status) + master_course = get_course_by_id(ccx_key.to_course_locator(), depth=None) + return master_course, ccx, None + + class CCXCoachMetadataView(DeveloperErrorViewMixin, APIView): """ Return CCX Coach metadata for a master course or CCX course. - **Example Request** + *Example Request* GET /api/ccx_coach/v2/courses/{course_id|ccx_course_id}/metadata - **Response Values** + *Response Values* { "course_id": "course-v1:edX+DemoX+Demo_Course", @@ -103,7 +131,7 @@ class CreateCCXView(DeveloperErrorViewMixin, APIView): """ Create a CCX course for a master course and return its metadata payload. - **Example Request** + *Example Request* POST /api/ccx_coach/v2/courses/{course_id}/create_ccx { "name": "My CCX" } @@ -158,3 +186,108 @@ def post(self, request, course_id): data = {'master_course_key': master_course_key, 'ccx_course_key': ccx_course_key} return Response(CCXCoachMetadataSerializer(data).data, status=status.HTTP_201_CREATED) + + +class CCXScheduleView(DeveloperErrorViewMixin, APIView): + """ + Return the CCX schedule for a CCX course. + + *Example Request* + + GET /api/ccx_coach/v2/courses/{ccx_course_id}/schedule + + *Response Values* + + A JSON array of schedule blocks (sections -> subsections -> units), each + with `location`, `display_name`, `category`, `start`, optional + `due`, `hidden` and optional `children`. This mirrors the legacy + `ccx_schedule` output. + """ + + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated, IsCCXCoach) + + def get(self, request, course_id): + """Return the CCX schedule for the given CCX course id.""" + master_course, ccx, error_response = _resolve_ccx_course(course_id) + if error_response: + return error_response + return Response(get_ccx_schedule(master_course, ccx), status=status.HTTP_200_OK) + + +class SaveScheduleView(DeveloperErrorViewMixin, APIView): + """ + Apply an edited schedule tree to a CCX course. + + *Example Request* + + POST /api/ccx_coach/v2/courses/{ccx_course_id}/save_schedule + [ { "location": "...", "hidden": false, "start": "...", "due": "...", "children": [...] }, ... ] + + *Response Values* + + { "schedule": [...], "grading_policy": "" } + + Mirrors the legacy `save_ccx` behavior (including automatic grading-policy + adjustment) but with DRF/JWT auth and JSON in/out. + """ + + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated, IsCCXCoach) + + def post(self, request, course_id): + """Save the supplied schedule tree to the CCX course.""" + master_course, ccx, error_response = _resolve_ccx_course(course_id) + if error_response: + return error_response + + schedule_data = request.data + if not isinstance(schedule_data, list): + return _error_response('invalid_schedule_payload', status.HTTP_400_BAD_REQUEST) + + try: + schedule, policy = save_ccx_schedule(master_course, ccx, schedule_data) + except (KeyError, ValueError, TypeError): + # Unknown block location, missing required keys, or malformed dates + # in the payload. Return a structured JSON error rather than a 500. + return _error_response('invalid_schedule_payload', status.HTTP_400_BAD_REQUEST) + + return Response( + {'schedule': schedule, 'grading_policy': json.dumps(policy, indent=4)}, + status=status.HTTP_200_OK, + ) + + +class RemoveScheduleView(DeveloperErrorViewMixin, APIView): + """ + Remove a block (and its descendants) from a CCX schedule. + + *Example Request* + + POST /api/ccx_coach/v2/courses/{ccx_course_id}/remove_schedule + { "location": "block-v1:edX+DemoX+Demo_Course+type@chapter+block@week1" } + + Hides the block and its descendants and clears their start/due overrides, + then returns the updated schedule (same shape as the schedule endpoint) so + the client can refresh in a single call. + """ + + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated, IsCCXCoach) + + def post(self, request, course_id): + """Remove the block identified by `location` from the CCX schedule.""" + master_course, ccx, error_response = _resolve_ccx_course(course_id) + if error_response: + return error_response + + request_serializer = RemoveScheduleRequestSerializer(data=request.data) + request_serializer.is_valid(raise_exception=True) + location = request_serializer.validated_data['location'] + + try: + schedule = remove_block_from_ccx_schedule(ccx, master_course, location) + except ValueError: + return _error_response('schedule_block_not_found', status.HTTP_400_BAD_REQUEST) + + return Response(schedule, status=status.HTTP_200_OK) diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 6d6526ea849c..271ef2a1d5d3 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -29,8 +29,7 @@ from lms.djangoapps.ccx.overrides import get_override_for_ccx, override_field_for_ccx from lms.djangoapps.ccx.tests.factories import CcxFactory from lms.djangoapps.ccx.tests.utils import CcxTestCase, flatten -from lms.djangoapps.ccx.utils import ccx_course, create_ccx_course, is_email -from lms.djangoapps.ccx.views import get_date +from lms.djangoapps.ccx.utils import ccx_course, create_ccx_course, get_date, is_email from lms.djangoapps.courseware.tabs import get_course_tab_list from lms.djangoapps.courseware.tests.factories import StudentModuleFactory from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase @@ -339,7 +338,7 @@ def test_no_ccx_created(self): def test_create_ccx_with_ccx_connector_set(self): """ - Assert that coach cannot create ccx when ``ccx_connector`` url is set. + Assert that coach cannot create ccx when `ccx_connector` url is set. """ role = CourseCcxCoachRole(self.course_with_ccx_connect_set.id) role.add_users(self.coach) @@ -358,9 +357,9 @@ def test_create_ccx_with_ccx_connector_set(self): def test_create_ccx_course_service(self): """ - The extracted ``create_ccx_course`` service performs the full CCX + The extracted `create_ccx_course` service performs the full CCX creation side effects independently of the legacy view. This guards the - refactor that moved the creation logic out of ``create_ccx`` so the v2 + refactor that moved the creation logic out of `create_ccx` so the v2 API can reuse it. """ ccx_name = 'Service CCX' diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index f4ad10b733f9..da76c5190315 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -8,6 +8,7 @@ import datetime import logging from contextlib import contextmanager +from copy import deepcopy from smtplib import SMTPException import pytz @@ -23,7 +24,14 @@ from common.djangoapps.student.roles import CourseCcxCoachRole, CourseInstructorRole, CourseStaffRole from lms.djangoapps.ccx.custom_exception import CCXUserValidationException from lms.djangoapps.ccx.models import CustomCourseForEdX -from lms.djangoapps.ccx.overrides import get_override_for_ccx, override_field_for_ccx +from lms.djangoapps.ccx.overrides import ( + bulk_delete_ccx_override_fields, + clear_ccx_field_info_from_ccx_map, + clear_override_for_ccx, + get_override_for_ccx, + override_field_for_ccx, +) +from lms.djangoapps.courseware.field_overrides import disable_overrides from lms.djangoapps.instructor.access import allow_access, list_with_level, revoke_access from lms.djangoapps.instructor.enrollment import enroll_email, get_email_params, unenroll_email from lms.djangoapps.instructor.views.api import _split_input_list @@ -514,3 +522,232 @@ def create_ccx_course(course, coach, display_name): log.info('Signal fired when course is published. Receiver: %s. Response: %s', rec, response) return ccx + + +def get_ccx_schedule(course, ccx): + """ + Generate a JSON serializable CCX schedule. + + Visits student-visible nodes only; children of hidden nodes are skipped. + Dates are converted to strings for the JS date widgets. Only start dates + apply to sections; subsections have both start and due; units inherit their + subsection's dates when not overridden. + """ + def visit(node, depth=1): + """ + Recursive generator function which yields CCX schedule nodes. + """ + for child in node.get_children(): + # in case the children are visible to staff only, skip them + if child.visible_to_staff_only: + continue + + hidden = get_override_for_ccx( + ccx, child, 'visible_to_staff_only', + child.visible_to_staff_only) + + start = get_date(ccx, child, 'start') + if depth > 1: + # Subsection has both start and due dates and unit inherit dates from their subsections + if depth == 2: + due = get_date(ccx, child, 'due') + elif depth == 3: + # Get start and due date of subsection in case unit has not override dates. + due = get_date(ccx, child, 'due', node) + start = get_date(ccx, child, 'start', node) + + visited = { + 'location': str(child.location), + 'display_name': child.display_name, + 'category': child.category, + 'start': start, + 'due': due, + 'hidden': hidden, + } + else: + visited = { + 'location': str(child.location), + 'display_name': child.display_name, + 'category': child.category, + 'start': start, + 'hidden': hidden, + } + if depth < 3: + children = tuple(visit(child, depth + 1)) + if children: + visited['children'] = children + yield visited + else: + yield visited + + with disable_overrides(): + return tuple(visit(course)) + + +def save_ccx_schedule(course, ccx, schedule): # pylint: disable=too-many-statements + """ + Apply an edited CCX `schedule` tree to the CCX and republish it. + + Recursively overrides the `visible_to_staff_only`, `start` and `due` + fields for units in the course from the supplied schedule data, adjusts the + grading policy's `min_count` values when graded sections were hidden, and + fires the `course_published` signal. + + This is the shared logic behind the legacy `save_ccx` view and the CCX + Coach v2 save-schedule endpoint. Callers are responsible for access control. + + Arguments: + course (CourseBlock): the master course. + ccx (CustomCourseForEdX): the CCX being edited. + schedule (list): the schedule tree (list of block dicts with + `location`, `hidden`, `start`, optional `due` and + `children`). + + Returns: + tuple: `(schedule, grading_policy)` where `schedule` is the + regenerated schedule (see :func:`get_ccx_schedule`) and + `grading_policy` is the (possibly adjusted) grading policy dict. + """ + def override_fields(parent, data, graded, earliest=None, ccx_ids_to_delete=None): + """ + Recursively apply CCX schedule data to CCX by overriding the + `visible_to_staff_only`, `start` and `due` fields for units in the + course. + """ + if ccx_ids_to_delete is None: + ccx_ids_to_delete = [] + blocks = { + str(child.location): child + for child in parent.get_children()} + + for unit in data: + block = blocks[unit['location']] + override_field_for_ccx( + ccx, block, 'visible_to_staff_only', unit['hidden']) + + start = parse_date(unit['start']) + if start: + if not earliest or start < earliest: + earliest = start + override_field_for_ccx(ccx, block, 'start', start) + else: + ccx_ids_to_delete.append(get_override_for_ccx(ccx, block, 'start_id')) + clear_ccx_field_info_from_ccx_map(ccx, block, 'start') + + # Only subsection (aka sequential) and unit (aka vertical) have due dates. + if 'due' in unit: # checking that the key (due) exist in dict (unit). + due = parse_date(unit['due']) + if due: + override_field_for_ccx(ccx, block, 'due', due) + else: + ccx_ids_to_delete.append(get_override_for_ccx(ccx, block, 'due_id')) + clear_ccx_field_info_from_ccx_map(ccx, block, 'due') + else: + # In case of section aka chapter we do not have due date. + ccx_ids_to_delete.append(get_override_for_ccx(ccx, block, 'due_id')) + clear_ccx_field_info_from_ccx_map(ccx, block, 'due') + + if not unit['hidden'] and block.graded: + graded[block.format] = graded.get(block.format, 0) + 1 + + children = unit.get('children', None) + # For a vertical, override start and due dates of all its problems. + if unit.get('category', None) == 'vertical': + for component in block.get_children(): + # override start and due date of problem (Copy dates of vertical into problems) + if start: + override_field_for_ccx(ccx, component, 'start', start) + + if due: + override_field_for_ccx(ccx, component, 'due', due) + + if children: + override_fields(block, children, graded, earliest, ccx_ids_to_delete) + return earliest, ccx_ids_to_delete + + graded = {} + earliest, ccx_ids_to_delete = override_fields(course, schedule, graded, []) + bulk_delete_ccx_override_fields(ccx, ccx_ids_to_delete) + if earliest: + override_field_for_ccx(ccx, course, 'start', earliest) + + # Attempt to automatically adjust grading policy + changed = False + policy = get_override_for_ccx( + ccx, course, 'grading_policy', course.grading_policy + ) + policy = deepcopy(policy) + grader = policy['GRADER'] + for section in grader: + count = graded.get(section.get('type'), 0) + if count < section.get('min_count', 0): + changed = True + section['min_count'] = count + if changed: + override_field_for_ccx(ccx, course, 'grading_policy', policy) + + # using CCX object as sender here. + responses = SignalHandler.course_published.send( + sender=ccx, + course_key=CCXLocator.from_course_locator(course.id, str(ccx.id)) + ) + for rec, response in responses: + log.info('Signal fired when course is published. Receiver: %s. Response: %s', rec, response) + + return get_ccx_schedule(course, ccx), policy + + +def remove_block_from_ccx_schedule(ccx, course, location): + """ + Remove a block (and its descendants) from the CCX schedule. + + Hides the block identified by `location` and all of its descendants from + learners (`visible_to_staff_only=True`) and clears any CCX start/due date + overrides on them, then republishes the CCX. This is the inverse of adding + a block to the schedule and mirrors what the legacy `save_ccx` flow does + when a unit is hidden. + + Arguments: + ccx (CustomCourseForEdX): the CCX being edited. + course (CourseBlock): the master course. + location (str): the usage-key string of the block to remove. + + Returns: + list: the regenerated schedule (see :func:`get_ccx_schedule`). + + Raises: + ValueError: if `location` does not identify a block in the course. + """ + def find_block(node): + """Depth-first search for the block whose location matches `location`.""" + for child in node.get_children(): + if str(child.location) == location: + return child + found = find_block(child) + if found is not None: + return found + return None + + def hide(block): + """Hide `block` and its descendants and clear their date overrides.""" + override_field_for_ccx(ccx, block, 'visible_to_staff_only', True) + clear_override_for_ccx(ccx, block, 'start') + clear_override_for_ccx(ccx, block, 'due') + for child in block.get_children(): + hide(child) + + block = find_block(course) + if block is None: + raise ValueError(f'Block "{location}" is not part of course "{course.id}"') + + hide(block) + + # using CCX object as sender here. + responses = SignalHandler.course_published.send( + sender=ccx, + course_key=CCXLocator.from_course_locator(course.id, str(ccx.id)) + ) + for rec, response in responses: + log.info('Signal fired when course is published. Receiver: %s. Response: %s', rec, response) + + return get_ccx_schedule(course, ccx) diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index 2104052e7ea5..938ccada130a 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -6,7 +6,6 @@ import functools import json import logging -from copy import deepcopy from ccx_keys.locator import CCXLocator from django.contrib import messages @@ -26,8 +25,6 @@ from common.djangoapps.student.roles import CourseCcxCoachRole from lms.djangoapps.ccx.models import CustomCourseForEdX from lms.djangoapps.ccx.overrides import ( - bulk_delete_ccx_override_fields, - clear_ccx_field_info_from_ccx_map, get_override_for_ccx, override_field_for_ccx, ) @@ -40,11 +37,10 @@ get_ccx_by_ccx_id, get_ccx_creation_dict, get_ccx_for_coach, - get_date, + get_ccx_schedule, get_enrollment_action_and_identifiers, - parse_date, + save_ccx_schedule, ) -from lms.djangoapps.courseware.field_overrides import disable_overrides from lms.djangoapps.grades.api import CourseGradeFactory from lms.djangoapps.instructor.enrollment import get_email_params from lms.djangoapps.instructor.views.gradebook_api import get_grade_book_page @@ -190,102 +186,18 @@ def create_ccx(request, course, ccx=None): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @coach_dashboard -def save_ccx(request, course, ccx=None): # pylint: disable=too-many-statements +def save_ccx(request, course, ccx=None): """ Save changes to CCX. """ if not ccx: raise Http404 - def override_fields(parent, data, graded, earliest=None, ccx_ids_to_delete=None): - """ - Recursively apply CCX schedule data to CCX by overriding the - `visible_to_staff_only`, `start` and `due` fields for units in the - course. - """ - if ccx_ids_to_delete is None: - ccx_ids_to_delete = [] - blocks = { - str(child.location): child - for child in parent.get_children()} - - for unit in data: - block = blocks[unit['location']] - override_field_for_ccx( - ccx, block, 'visible_to_staff_only', unit['hidden']) - - start = parse_date(unit['start']) - if start: - if not earliest or start < earliest: - earliest = start - override_field_for_ccx(ccx, block, 'start', start) - else: - ccx_ids_to_delete.append(get_override_for_ccx(ccx, block, 'start_id')) - clear_ccx_field_info_from_ccx_map(ccx, block, 'start') - - # Only subsection (aka sequential) and unit (aka vertical) have due dates. - if 'due' in unit: # checking that the key (due) exist in dict (unit). - due = parse_date(unit['due']) - if due: - override_field_for_ccx(ccx, block, 'due', due) - else: - ccx_ids_to_delete.append(get_override_for_ccx(ccx, block, 'due_id')) - clear_ccx_field_info_from_ccx_map(ccx, block, 'due') - else: - # In case of section aka chapter we do not have due date. - ccx_ids_to_delete.append(get_override_for_ccx(ccx, block, 'due_id')) - clear_ccx_field_info_from_ccx_map(ccx, block, 'due') - - if not unit['hidden'] and block.graded: - graded[block.format] = graded.get(block.format, 0) + 1 - - children = unit.get('children', None) - # For a vertical, override start and due dates of all its problems. - if unit.get('category', None) == 'vertical': - for component in block.get_children(): - # override start and due date of problem (Copy dates of vertical into problems) - if start: - override_field_for_ccx(ccx, component, 'start', start) - - if due: - override_field_for_ccx(ccx, component, 'due', due) - - if children: - override_fields(block, children, graded, earliest, ccx_ids_to_delete) - return earliest, ccx_ids_to_delete - - graded = {} - earliest, ccx_ids_to_delete = override_fields(course, json.loads(request.body.decode('utf8')), graded, []) - bulk_delete_ccx_override_fields(ccx, ccx_ids_to_delete) - if earliest: - override_field_for_ccx(ccx, course, 'start', earliest) - - # Attempt to automatically adjust grading policy - changed = False - policy = get_override_for_ccx( - ccx, course, 'grading_policy', course.grading_policy - ) - policy = deepcopy(policy) - grader = policy['GRADER'] - for section in grader: - count = graded.get(section.get('type'), 0) - if count < section.get('min_count', 0): - changed = True - section['min_count'] = count - if changed: - override_field_for_ccx(ccx, course, 'grading_policy', policy) - - # using CCX object as sender here. - responses = SignalHandler.course_published.send( - sender=ccx, - course_key=CCXLocator.from_course_locator(course.id, str(ccx.id)) - ) - for rec, response in responses: - log.info('Signal fired when course is published. Receiver: %s. Response: %s', rec, response) + schedule, policy = save_ccx_schedule(course, ccx, json.loads(request.body.decode('utf8'))) return HttpResponse( # pylint: disable=http-response-with-content-type-json, http-response-with-json-dumps json.dumps({ - 'schedule': get_ccx_schedule(course, ccx), + 'schedule': schedule, 'grading_policy': json.dumps(policy, indent=4)}), content_type='application/json', ) @@ -319,73 +231,6 @@ def set_grading_policy(request, course, ccx=None): return redirect(url) -def get_ccx_schedule(course, ccx): - """ - Generate a JSON serializable CCX schedule. - """ - def visit(node, depth=1): - """ - Recursive generator function which yields CCX schedule nodes. - We convert dates to string to get them ready for use by the js date - widgets, which use text inputs. - Visits students visible nodes only; nodes children of hidden ones - are skipped as well. - - Dates: - Only start date is applicable to a section. If ccx coach did not override start date then - getting it from the master course. - Both start and due dates are applicable to a subsection (aka sequential). If ccx coach did not override - these dates then getting these dates from corresponding subsection in master course. - Unit inherits start date and due date from its subsection. If ccx coach did not override these dates - then getting them from corresponding subsection in master course. - """ - for child in node.get_children(): - # in case the children are visible to staff only, skip them - if child.visible_to_staff_only: - continue - - hidden = get_override_for_ccx( - ccx, child, 'visible_to_staff_only', - child.visible_to_staff_only) - - start = get_date(ccx, child, 'start') - if depth > 1: - # Subsection has both start and due dates and unit inherit dates from their subsections - if depth == 2: - due = get_date(ccx, child, 'due') - elif depth == 3: - # Get start and due date of subsection in case unit has not override dates. - due = get_date(ccx, child, 'due', node) - start = get_date(ccx, child, 'start', node) - - visited = { - 'location': str(child.location), - 'display_name': child.display_name, - 'category': child.category, - 'start': start, - 'due': due, - 'hidden': hidden, - } - else: - visited = { - 'location': str(child.location), - 'display_name': child.display_name, - 'category': child.category, - 'start': start, - 'hidden': hidden, - } - if depth < 3: - children = tuple(visit(child, depth + 1)) - if children: - visited['children'] = children - yield visited - else: - yield visited - - with disable_overrides(): - return tuple(visit(course)) - - @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @coach_dashboard From 787e2ddd1f55ee81bc22d400d888dd1c640254c3 Mon Sep 17 00:00:00 2001 From: Brian Buck Date: Thu, 17 Sep 2026 14:31:41 -0600 Subject: [PATCH 2/2] fix: Add transaction.atomic() to Save and Remove views --- .../ccx/api/v2/tests/test_schedule_views.py | 28 +++++++++++++++++++ lms/djangoapps/ccx/api/v2/views.py | 15 ++++++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/ccx/api/v2/tests/test_schedule_views.py b/lms/djangoapps/ccx/api/v2/tests/test_schedule_views.py index 29b6f5e6fa53..4561760e8467 100644 --- a/lms/djangoapps/ccx/api/v2/tests/test_schedule_views.py +++ b/lms/djangoapps/ccx/api/v2/tests/test_schedule_views.py @@ -9,6 +9,7 @@ from rest_framework.test import APIClient from common.djangoapps.student.tests.factories import UserFactory +from lms.djangoapps.ccx.models import CcxFieldOverride from lms.djangoapps.ccx.tests.utils import CcxTestCase @@ -118,6 +119,33 @@ def test_invalid_payload_returns_400(self): assert response.status_code == status.HTTP_400_BAD_REQUEST assert response.data.get('error_code') == 'invalid_schedule_payload' + def test_partial_failure_rolls_back_overrides(self): + """ + A payload that fails part-way through leaves no overrides applied. + + The first entry is valid and would be written, the second references an + unknown block and raises. Because the view catches that exception to + return JSON, the ATOMIC_REQUESTS rollback is suppressed, so the save + runs inside an explicit atomic block. This guards that rollback. + """ + good_location = str(self.chapters[0].location) + bogus_location = str(self.course.id.make_usage_key('chapter', 'does_not_exist')) + payload = [ + {'location': good_location, 'hidden': True, 'start': ''}, + {'location': bogus_location, 'hidden': True, 'start': ''}, + ] + + response = self.api_client.post(self._url(self.ccx_key), payload, format='json') + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.data.get('error_code') == 'invalid_schedule_payload' + # The valid entry's override must not have been committed. (Note the CCX + # fixture itself creates an override on the course, so this assertion is + # scoped to the section touched by this payload.) + assert not CcxFieldOverride.objects.filter( + ccx=self.ccx, location=self.chapters[0].location + ).exists() + def test_unknown_location_returns_json_400(self): bogus = str(self.course.id.make_usage_key('chapter', 'does_not_exist')) payload = [{'location': bogus, 'hidden': True, 'start': ''}] diff --git a/lms/djangoapps/ccx/api/v2/views.py b/lms/djangoapps/ccx/api/v2/views.py index 6591adbfc618..edc7af0189f0 100644 --- a/lms/djangoapps/ccx/api/v2/views.py +++ b/lms/djangoapps/ccx/api/v2/views.py @@ -246,7 +246,13 @@ def post(self, request, course_id): return _error_response('invalid_schedule_payload', status.HTTP_400_BAD_REQUEST) try: - schedule, policy = save_ccx_schedule(master_course, ccx, schedule_data) + # Explicit atomic block: the exception is caught below and converted + # into a response, which would otherwise let the ATOMIC_REQUESTS + # transaction commit. `save_ccx_schedule` writes overrides as it + # walks the tree, so a failure part-way through would leave the + # schedule half-applied. Exiting via the exception rolls it back. + with transaction.atomic(): + schedule, policy = save_ccx_schedule(master_course, ccx, schedule_data) except (KeyError, ValueError, TypeError): # Unknown block location, missing required keys, or malformed dates # in the payload. Return a structured JSON error rather than a 500. @@ -286,7 +292,12 @@ def post(self, request, course_id): location = request_serializer.validated_data['location'] try: - schedule = remove_block_from_ccx_schedule(ccx, master_course, location) + # Explicit atomic block, for the same reason as the save endpoint: + # the caught exception suppresses the ATOMIC_REQUESTS rollback, and + # `remove_block_from_ccx_schedule` clears overrides as it walks the + # block's descendants. + with transaction.atomic(): + schedule = remove_block_from_ccx_schedule(ccx, master_course, location) except ValueError: return _error_response('schedule_block_not_found', status.HTTP_400_BAD_REQUEST)