diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py index 66b5f46128c8..8e220a334c06 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py @@ -277,18 +277,14 @@ def test_update_exam_settings_invalid_value(self): # response is correct assert response.status_code == status.HTTP_400_BAD_REQUEST - self.assertDictEqual( - response.data, + self.assertIn( { - "detail": [ - { - "proctoring_provider": ( - "The selected proctoring provider, notvalidprovider, is not a valid provider. " - "Please select from one of ['test_proctoring_provider']." - ) - } - ] + "proctoring_provider": ( + "The selected proctoring provider, notvalidprovider, is not a valid provider. " + "Please select from one of ['test_proctoring_provider']." + ) }, + response.data['detail'], ) # course settings have been updated @@ -408,18 +404,14 @@ def test_400_for_disabled_lti(self): # response is correct assert response.status_code == status.HTTP_400_BAD_REQUEST - self.assertDictEqual( - response.data, + self.assertIn( { - "detail": [ - { - "proctoring_provider": ( - "The selected proctoring provider, lti_external, is not a valid provider. " - "Please select from one of ['null']." - ) - } - ] + "proctoring_provider": ( + "The selected proctoring provider, lti_external, is not a valid provider. " + "Please select from one of ['null']." + ) }, + response.data['detail'], ) # course settings have been updated diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 17e94112bafe..dd00f245ef55 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1537,6 +1537,7 @@ def get_library_context(request, request_is_json=False): ) from cms.djangoapps.contentstore.views.library import ( LIBRARIES_ENABLED, + user_can_view_create_library_button, ) libraries = _accessible_libraries_iter(request.user) if LIBRARIES_ENABLED else [] @@ -1550,7 +1551,7 @@ def get_library_context(request, request_is_json=False): 'in_process_course_actions': [], 'courses': [], 'libraries_enabled': LIBRARIES_ENABLED, - 'show_new_library_button': LIBRARIES_ENABLED and request.user.is_active, + 'show_new_library_button': user_can_view_create_library_button(request.user) and request.user.is_active, 'user': request.user, 'request_course_creator_url': reverse('request_course_creator'), 'course_creator_status': _get_course_creator_status(request.user), diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index 870c192653d2..8c314caa6697 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -69,31 +69,7 @@ def should_redirect_to_library_authoring_mfe(): ) -def user_can_view_create_library_button(user): - """ - Helper method for displaying the visibilty of the create_library_button. - """ - if not LIBRARIES_ENABLED: - return False - elif user.is_staff: - return True - elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): - is_course_creator = get_course_creator_status(user) == 'granted' - has_org_staff_role = OrgStaffRole().get_orgs_for_user(user).exists() - has_course_staff_role = UserBasedRole(user=user, role=CourseStaffRole.ROLE).courses_with_role().exists() - has_course_admin_role = UserBasedRole(user=user, role=CourseInstructorRole.ROLE).courses_with_role().exists() - return is_course_creator or has_org_staff_role or has_course_staff_role or has_course_admin_role - else: - # EDUCATOR-1924: DISABLE_LIBRARY_CREATION overrides DISABLE_COURSE_CREATION, if present. - disable_library_creation = settings.FEATURES.get('DISABLE_LIBRARY_CREATION', None) - disable_course_creation = settings.FEATURES.get('DISABLE_COURSE_CREATION', False) - if disable_library_creation is not None: - return not disable_library_creation - else: - return not disable_course_creation - - -def user_can_create_library(user, org): +def _user_can_create_library_for_org(user, org=None): """ Helper method for returning the library creation status for a particular user, taking into account the value LIBRARIES_ENABLED. @@ -109,29 +85,29 @@ def user_can_create_library(user, org): Course Staff: Can make libraries in the organization which has courses of which they are staff. Course Admin: Can make libraries in the organization which has courses of which they are Admin. """ - if org is None: - return False if not LIBRARIES_ENABLED: return False elif user.is_staff: return True - if settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + org_filter_params = {} + if org: + org_filter_params['org'] = org is_course_creator = get_course_creator_status(user) == 'granted' - has_org_staff_role = org in OrgStaffRole().get_orgs_for_user(user) + has_org_staff_role = OrgStaffRole().get_orgs_for_user(user).filter(**org_filter_params).exists() has_course_staff_role = ( UserBasedRole(user=user, role=CourseStaffRole.ROLE) .courses_with_role() - .filter(org=org) + .filter(**org_filter_params) .exists() ) has_course_admin_role = ( UserBasedRole(user=user, role=CourseInstructorRole.ROLE) .courses_with_role() - .filter(org=org) + .filter(**org_filter_params) .exists() ) return is_course_creator or has_org_staff_role or has_course_staff_role or has_course_admin_role - else: # EDUCATOR-1924: DISABLE_LIBRARY_CREATION overrides DISABLE_COURSE_CREATION, if present. disable_library_creation = settings.FEATURES.get('DISABLE_LIBRARY_CREATION', None) @@ -142,6 +118,22 @@ def user_can_create_library(user, org): return not disable_course_creation +def user_can_view_create_library_button(user): + """ + Helper method for displaying the visibilty of the create_library_button. + """ + return _user_can_create_library_for_org(user) + + +def user_can_create_library(user, org): + """ + Helper method for to check if user can create library for given org. + """ + if org is None: + return False + return _user_can_create_library_for_org(user, org) + + @login_required @ensure_csrf_cookie @require_http_methods(('GET', 'POST')) diff --git a/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py b/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py index a7ee7f0ab0cd..0f38722e1208 100644 --- a/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py +++ b/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py @@ -162,6 +162,39 @@ def test_exam_settings_alert_with_exam_settings_disabled(self, page_handler): else: assert 'To update these settings go to the Advanced Settings page.' in alert_text + @override_settings( + PROCTORING_BACKENDS={ + 'DEFAULT': 'test_proctoring_provider', + 'proctortrack': {}, + 'test_proctoring_provider': {}, + }, + FEATURES=FEATURES_WITH_EXAM_SETTINGS_ENABLED, + ) + @ddt.data( + "advanced_settings_handler", + "course_handler", + ) + def test_invalid_provider_alert(self, page_handler): + """ + An alert should appear if the course has a proctoring provider that is not valid. + """ + # create an error by setting an invalid proctoring provider + self.course.proctoring_provider = 'invalid_provider' + self.course.enable_proctored_exams = True + self.save_course() + + url = reverse_course_url(page_handler, self.course.id) + resp = self.client.get(url, HTTP_ACCEPT='text/html') + alert_text = self._get_exam_settings_alert_text(resp.content) + assert ( + 'This course has proctored exam settings that are incomplete or invalid.' + in alert_text + ) + assert ( + 'The proctoring provider configured for this course, \'invalid_provider\', is not valid.' + in alert_text + ) + @ddt.data( "advanced_settings_handler", "course_handler", diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index fd5219dfb472..5d4ac5a4a336 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -217,7 +217,10 @@ def update_from_json(cls, block, jsondict, user, filter_tabs=True): try: val = model['value'] if hasattr(block, key) and getattr(block, key) != val: - key_values[key] = block.fields[key].from_json(val) + if key == 'proctoring_provider': + key_values[key] = block.fields[key].from_json(val, validate_providers=True) + else: + key_values[key] = block.fields[key].from_json(val) except (TypeError, ValueError) as err: raise ValueError(_("Incorrect format for field '{name}'. {detailed_message}").format( # lint-amnesty, pylint: disable=raise-missing-from name=model['display_name'], detailed_message=str(err))) @@ -253,7 +256,10 @@ def validate_and_update_from_json(cls, block, jsondict, user, filter_tabs=True): try: val = model['value'] if hasattr(block, key) and getattr(block, key) != val: - key_values[key] = block.fields[key].from_json(val) + if key == 'proctoring_provider': + key_values[key] = block.fields[key].from_json(val, validate_providers=True) + else: + key_values[key] = block.fields[key].from_json(val) except (TypeError, ValueError, ValidationError) as err: did_validate = False errors.append({'key': key, 'message': str(err), 'model': model}) @@ -484,6 +490,24 @@ def validate_proctoring_settings(cls, block, settings_dict, user): enable_proctoring = block.enable_proctored_exams if enable_proctoring: + + if proctoring_provider_model: + proctoring_provider = proctoring_provider_model.get('value') + else: + proctoring_provider = block.proctoring_provider + + # If the proctoring provider stored in the course block no longer + # matches the available providers for this instance, show an error + if proctoring_provider not in available_providers: + message = ( + f'The proctoring provider configured for this course, \'{proctoring_provider}\', is not valid.' + ) + errors.append({ + 'key': 'proctoring_provider', + 'message': message, + 'model': proctoring_provider_model + }) + # Require a valid escalation email if Proctortrack is chosen as the proctoring provider escalation_email_model = settings_dict.get('proctoring_escalation_email') if escalation_email_model: @@ -491,11 +515,6 @@ def validate_proctoring_settings(cls, block, settings_dict, user): else: escalation_email = block.proctoring_escalation_email - if proctoring_provider_model: - proctoring_provider = proctoring_provider_model.get('value') - else: - proctoring_provider = block.proctoring_provider - missing_escalation_email_msg = 'Provider \'{provider}\' requires an exam escalation contact.' if proctoring_provider_model and proctoring_provider == 'proctortrack': if not escalation_email: diff --git a/cms/envs/common.py b/cms/envs/common.py index 8daa08aeb1c8..34dd8503f35e 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -949,7 +949,6 @@ 'openedx.core.djangoapps.cache_toolbox.middleware.CacheBackedAuthenticationMiddleware', 'common.djangoapps.student.middleware.UserStandingMiddleware', - 'openedx.core.djangoapps.contentserver.middleware.StaticContentServerMiddleware', 'django.contrib.messages.middleware.MessageMiddleware', 'common.djangoapps.track.middleware.TrackMiddleware', diff --git a/common/djangoapps/util/tests/test_db.py b/common/djangoapps/util/tests/test_db.py index 88358ce20512..4a16c2a20aa6 100644 --- a/common/djangoapps/util/tests/test_db.py +++ b/common/djangoapps/util/tests/test_db.py @@ -1,6 +1,5 @@ """Tests for util.db module.""" -import unittest from io import StringIO import ddt @@ -121,9 +120,6 @@ class MigrationTests(TestCase): Tests for migrations. """ - @unittest.skip( - "Temporary skip for ENT-8971 while the client id and secret columns in Canvas replaced." - ) @override_settings(MIGRATION_MODULES={}) def test_migrations_are_in_sync(self): """ diff --git a/common/static/data/geoip/GeoLite2-Country.mmdb b/common/static/data/geoip/GeoLite2-Country.mmdb index b4dcff0b4ab8..2e9d48109648 100644 Binary files a/common/static/data/geoip/GeoLite2-Country.mmdb and b/common/static/data/geoip/GeoLite2-Country.mmdb differ diff --git a/lms/djangoapps/bulk_email/signals.py b/lms/djangoapps/bulk_email/signals.py index afac652debf9..086b3636f7ef 100644 --- a/lms/djangoapps/bulk_email/signals.py +++ b/lms/djangoapps/bulk_email/signals.py @@ -44,10 +44,15 @@ def ace_email_sent_handler(sender, **kwargs): course_id = message.context.get('course_id') if not course_id: course_id = course_email.course_id if course_email else None + try: + channel = sender.__class__.__name__ + except AttributeError: + channel = 'Other' tracker.emit( - 'edx.bulk_email.sent', + 'edx.ace.message_sent', { 'message_type': message.name, + 'channel': channel, 'course_id': course_id, 'user_id': user_id, } diff --git a/lms/djangoapps/bulk_email/views.py b/lms/djangoapps/bulk_email/views.py index 927699091558..7ee3ea81b19a 100644 --- a/lms/djangoapps/bulk_email/views.py +++ b/lms/djangoapps/bulk_email/views.py @@ -61,12 +61,16 @@ def opt_out_email_updates(request, token, course_id): course_id, ) - tracker.emit( - 'edx.bulk_email.opt_out', - { - 'course_id': course_id, - 'user_id': user.id, - } - ) + event_name = 'edx.bulk_email.opt_out' + event_data = { + "username": user.username, + "user_id": user.id, + "course_id": course_id, + } + with tracker.get_tracker().context(event_name, event_data): + tracker.emit( + event_name, + event_data + ) return render_to_response('bulk_email/unsubscribe_success.html', context) diff --git a/lms/djangoapps/course_home_api/course_metadata/serializers.py b/lms/djangoapps/course_home_api/course_metadata/serializers.py index 7683a9089453..29b92fc7b004 100644 --- a/lms/djangoapps/course_home_api/course_metadata/serializers.py +++ b/lms/djangoapps/course_home_api/course_metadata/serializers.py @@ -43,6 +43,7 @@ class CourseHomeMetadataSerializer(VerifiedModeSerializer): """ celebrations = serializers.DictField() course_access = serializers.DictField() + studio_access = serializers.BooleanField() course_id = serializers.CharField() is_enrolled = serializers.BooleanField() is_self_paced = serializers.BooleanField() diff --git a/lms/djangoapps/course_home_api/course_metadata/views.py b/lms/djangoapps/course_home_api/course_metadata/views.py index 248f90389d40..02c30ff62e91 100644 --- a/lms/djangoapps/course_home_api/course_metadata/views.py +++ b/lms/djangoapps/course_home_api/course_metadata/views.py @@ -20,7 +20,7 @@ from lms.djangoapps.course_api.api import course_detail from lms.djangoapps.course_goals.models import UserActivity from lms.djangoapps.course_home_api.course_metadata.serializers import CourseHomeMetadataSerializer -from lms.djangoapps.courseware.access import has_access +from lms.djangoapps.courseware.access import has_access, has_cms_access from lms.djangoapps.courseware.context_processor import user_timezone_locale_prefs from lms.djangoapps.courseware.courses import check_course_access from lms.djangoapps.courseware.masquerade import setup_masquerade @@ -124,6 +124,7 @@ def get(self, request, *args, **kwargs): data = { 'course_id': course.id, 'username': username, + 'studio_access': has_cms_access(request.user, course_key), 'is_staff': has_access(request.user, 'staff', course_key).has_access, 'original_user_is_staff': original_user_is_staff, 'number': course.display_number_with_default, diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 74f1d74f837f..436cb3514a54 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -53,7 +53,8 @@ GlobalStaff, OrgInstructorRole, OrgStaffRole, - SupportStaffRole + SupportStaffRole, + CourseLimitedStaffRole, ) from common.djangoapps.util import milestones_helpers as milestones_helpers # lint-amnesty, pylint: disable=useless-import-alias from common.djangoapps.util.milestones_helpers import ( @@ -97,6 +98,31 @@ def has_ccx_coach_role(user, course_key): return False +def has_cms_access(user, course_key): + """ + Check if user has access to the CMS. When requesting from the LMS, a user with the + limited staff access role needs access to the CMS APIs, but not the CMS site. This + function accounts for this edge case when determining if a user has access to the CMS + site. + + Arguments: + user (User): the user whose course access we are checking. + course_key: Key to course. + + Returns: + bool: whether user has access to the CMS site. + """ + has_course_author_access = auth.has_course_author_access(user, course_key) + is_limited_staff = auth.user_has_role( + user, CourseLimitedStaffRole(course_key) + ) and not GlobalStaff().has_user(user) + + if is_limited_staff and has_course_author_access: + return False + + return has_course_author_access + + @function_trace('has_access') def has_access(user, action, obj, course_key=None): """ diff --git a/lms/djangoapps/courseware/tests/test_about.py b/lms/djangoapps/courseware/tests/test_about.py index d53d620d3e34..bd0c1854ab76 100644 --- a/lms/djangoapps/courseware/tests/test_about.py +++ b/lms/djangoapps/courseware/tests/test_about.py @@ -156,7 +156,10 @@ def test_pre_requisite_course(self): assert resp.status_code == 200 pre_requisite_courses = get_prerequisite_courses_display(course) pre_requisite_course_about_url = reverse('about_course', args=[str(pre_requisite_courses[0]['key'])]) - assert '{}'.format(pre_requisite_course_about_url, pre_requisite_courses[0]['display']) in resp.content.decode(resp.charset).strip('\n') # pylint: disable=line-too-long + assert ( + f'You must successfully complete ' + f'{pre_requisite_courses[0]["display"]} before you begin this course.' + ) in resp.content.decode(resp.charset).strip('\n') @patch.dict(settings.FEATURES, {'ENABLE_PREREQUISITE_COURSES': True}) def test_about_page_unfulfilled_prereqs(self): @@ -190,7 +193,10 @@ def test_about_page_unfulfilled_prereqs(self): assert resp.status_code == 200 pre_requisite_courses = get_prerequisite_courses_display(course) pre_requisite_course_about_url = reverse('about_course', args=[str(pre_requisite_courses[0]['key'])]) - assert '{}'.format(pre_requisite_course_about_url, pre_requisite_courses[0]['display']) in resp.content.decode(resp.charset).strip('\n') # pylint: disable=line-too-long + assert ( + f'You must successfully complete ' + f'{pre_requisite_courses[0]["display"]} before you begin this course.' + ) in resp.content.decode(resp.charset).strip('\n') url = reverse('about_course', args=[str(pre_requisite_course.id)]) resp = self.client.get(url) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index b0e533ee6f7f..65d35221d4f5 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -3515,6 +3515,14 @@ def test_send_email_but_not_logged_in(self): self.client.logout() url = reverse('send_email', kwargs={'course_id': str(self.course.id)}) response = self.client.post(url, self.full_test_message) + assert response.status_code == 401 + + def test_send_email_logged_in_but_no_perms(self): + self.client.logout() + user = UserFactory() + self.client.login(username=user.username, password=self.TEST_PASSWORD) + url = reverse('send_email', kwargs={'course_id': str(self.course.id)}) + response = self.client.post(url, self.full_test_message) assert response.status_code == 403 def test_send_email_but_not_staff(self): @@ -3635,6 +3643,7 @@ def test_send_email_with_lapsed_date_expect_error(self): url = reverse('send_email', kwargs={'course_id': str(self.course.id)}) with LogCapture() as log: + response = self.client.post(url, self.full_test_message) assert response.status_code == 400 diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index d9a301b07e7f..09a2b9ffd140 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -107,7 +107,8 @@ from lms.djangoapps.instructor_task.data import InstructorTaskTypes from lms.djangoapps.instructor_task.models import ReportStore from lms.djangoapps.instructor.views.serializer import ( - AccessSerializer, RoleNameSerializer, ShowStudentExtensionSerializer, UserSerializer + AccessSerializer, RoleNameSerializer, ShowStudentExtensionSerializer, + UserSerializer, SendEmailSerializer, StudentAttemptsSerializer ) from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, is_course_cohorted @@ -1816,23 +1817,24 @@ def post(self, request, course_id): return Response(serializer.data) -@transaction.non_atomic_requests -@require_POST -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.GIVE_STUDENT_EXTENSION) -@require_post_params( - problem_to_reset="problem urlname to reset" -) -@common_exceptions_400 -def reset_student_attempts(request, course_id): +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +@method_decorator(transaction.non_atomic_requests, name='dispatch') +class ResetStudentAttempts(DeveloperErrorViewMixin, APIView): """ - Resets a students attempts counter or starts a task to reset all students attempts counters. Optionally deletes student state for a problem. Limited to staff access. Some sub-methods limited to instructor access. + """ + http_method_names = ['post'] + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.GIVE_STUDENT_EXTENSION + serializer_class = StudentAttemptsSerializer - Takes some of the following query parameters + @method_decorator(ensure_csrf_cookie) + @transaction.non_atomic_requests + def post(self, request, course_id): + """ + Takes some of the following query parameters - problem_to_reset is a urlname of a problem - unique_student_identifier is an email or username - all_students is a boolean @@ -1842,65 +1844,74 @@ def reset_student_attempts(request, course_id): - delete_module is a boolean requires instructor access mutually exclusive with all_students - """ - course_id = CourseKey.from_string(course_id) - course = get_course_with_access( - request.user, 'staff', course_id, depth=None - ) - all_students = _get_boolean_param(request, 'all_students') - - if all_students and not has_access(request.user, 'instructor', course): - return HttpResponseForbidden("Requires instructor access.") + """ + course_id = CourseKey.from_string(course_id) + serializer_data = self.serializer_class(data=request.data) - problem_to_reset = strip_if_string(request.POST.get('problem_to_reset')) - student_identifier = request.POST.get('unique_student_identifier', None) - student = None - if student_identifier is not None: - student = get_student_from_identifier(student_identifier) - delete_module = _get_boolean_param(request, 'delete_module') + if not serializer_data.is_valid(): + return HttpResponseBadRequest(reason=serializer_data.errors) - # parameter combinations - if all_students and student: - return HttpResponseBadRequest( - "all_students and unique_student_identifier are mutually exclusive." - ) - if all_students and delete_module: - return HttpResponseBadRequest( - "all_students and delete_module are mutually exclusive." + course = get_course_with_access( + request.user, 'staff', course_id, depth=None ) - try: - module_state_key = UsageKey.from_string(problem_to_reset).map_into_course(course_id) - except InvalidKeyError: - return HttpResponseBadRequest() + all_students = serializer_data.validated_data.get('all_students') - response_payload = {} - response_payload['problem_to_reset'] = problem_to_reset + if all_students and not has_access(request.user, 'instructor', course): + return HttpResponseForbidden("Requires instructor access.") - if student: - try: - enrollment.reset_student_attempts( - course_id, - student, - module_state_key, - requesting_user=request.user, - delete_module=delete_module + problem_to_reset = strip_if_string(serializer_data.validated_data.get('problem_to_reset')) + student_identifier = request.POST.get('unique_student_identifier', None) + student = serializer_data.validated_data.get('unique_student_identifier') + delete_module = serializer_data.validated_data.get('delete_module') + + # parameter combinations + if all_students and student: + return HttpResponseBadRequest( + "all_students and unique_student_identifier are mutually exclusive." + ) + if all_students and delete_module: + return HttpResponseBadRequest( + "all_students and delete_module are mutually exclusive." ) - except StudentModule.DoesNotExist: - return HttpResponseBadRequest(_("Module does not exist.")) - except sub_api.SubmissionError: - # Trust the submissions API to log the error - error_msg = _("An error occurred while deleting the score.") - return HttpResponse(error_msg, status=500) - response_payload['student'] = student_identifier - elif all_students: - task_api.submit_reset_problem_attempts_for_all_students(request, module_state_key) - response_payload['task'] = TASK_SUBMISSION_OK - response_payload['student'] = 'All Students' - else: - return HttpResponseBadRequest() - return JsonResponse(response_payload) + try: + module_state_key = UsageKey.from_string(problem_to_reset).map_into_course(course_id) + except InvalidKeyError: + return HttpResponseBadRequest() + + response_payload = {} + response_payload['problem_to_reset'] = problem_to_reset + + if student: + try: + enrollment.reset_student_attempts( + course_id, + student, + module_state_key, + requesting_user=request.user, + delete_module=delete_module + ) + except StudentModule.DoesNotExist: + return HttpResponseBadRequest(_("Module does not exist.")) + except sub_api.SubmissionError: + # Trust the submissions API to log the error + error_msg = _("An error occurred while deleting the score.") + return HttpResponse(error_msg, status=500) + response_payload['student'] = student_identifier + + elif all_students: + try: + task_api.submit_reset_problem_attempts_for_all_students(request, module_state_key) + response_payload['task'] = TASK_SUBMISSION_OK + response_payload['student'] = 'All Students' + except Exception: # pylint: disable=broad-except + error_msg = _("An error occurred while attempting to reset for all students.") + return HttpResponse(error_msg, status=500) + else: + return HttpResponseBadRequest() + + return JsonResponse(response_payload) @transaction.non_atomic_requests @@ -1937,8 +1948,10 @@ def reset_student_attempts_for_entrance_exam(request, course_id): student_identifier = request.POST.get('unique_student_identifier', None) student = None + if student_identifier is not None: student = get_student_from_identifier(student_identifier) + all_students = _get_boolean_param(request, 'all_students') delete_module = _get_boolean_param(request, 'delete_module') @@ -2540,16 +2553,22 @@ def get(self, request, course_id): return _list_report_downloads(request=request, course_id=course_id) -@require_POST -@ensure_csrf_cookie -def list_report_downloads(request, course_id): +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +class ListReportDownloads(APIView): + """ List grade CSV files that are available for download for this course. Takes the following query parameters: - (optional) report_name - name of the report """ - return _list_report_downloads(request=request, course_id=course_id) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.CAN_RESEARCH + + @method_decorator(ensure_csrf_cookie) + def post(self, request, course_id): + + return _list_report_downloads(request=request, course_id=course_id) @cache_control(no_cache=True, no_store=True, must_revalidate=True) @@ -2763,81 +2782,96 @@ def extract_user_info(user): return JsonResponse(response_payload) -@transaction.non_atomic_requests -@require_POST -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.EMAIL) -@require_post_params(send_to="sending to whom", subject="subject line", message="message text") -@common_exceptions_400 -def send_email(request, course_id): +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +@method_decorator(transaction.non_atomic_requests, name='dispatch') +class SendEmail(DeveloperErrorViewMixin, APIView): """ Send an email to self, staff, cohorts, or everyone involved in a course. - Query Parameters: - - 'send_to' specifies what group the email should be sent to - Options are defined by the CourseEmail model in - lms/djangoapps/bulk_email/models.py - - 'subject' specifies email's subject - - 'message' specifies email's content """ - course_id = CourseKey.from_string(course_id) - course_overview = CourseOverview.get_from_id(course_id) + http_method_names = ['post'] + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.EMAIL + serializer_class = SendEmailSerializer - if not is_bulk_email_feature_enabled(course_id): - log.warning(f"Email is not enabled for course {course_id}") - return HttpResponseForbidden("Email is not enabled for this course.") + @method_decorator(ensure_csrf_cookie) + @method_decorator(transaction.non_atomic_requests) + def post(self, request, course_id): + """ + Query Parameters: + - 'send_to' specifies what group the email should be sent to + Options are defined by the CourseEmail model in + lms/djangoapps/bulk_email/models.py + - 'subject' specifies email's subject + - 'message' specifies email's content + """ + course_id = CourseKey.from_string(course_id) + course_overview = CourseOverview.get_from_id(course_id) - targets = json.loads(request.POST.get("send_to")) - subject = request.POST.get("subject") - message = request.POST.get("message") - # optional, this is a date and time in the form of an ISO8601 string - schedule = request.POST.get("schedule", "") + if not is_bulk_email_feature_enabled(course_id): + log.warning(f"Email is not enabled for course {course_id}") + return HttpResponseForbidden("Email is not enabled for this course.") - schedule_dt = None - if schedule: - try: - # convert the schedule from a string to a datetime, then check if its a valid future date and time, dateutil - # will throw a ValueError if the schedule is no good. - schedule_dt = dateutil.parser.parse(schedule).replace(tzinfo=pytz.utc) - if schedule_dt < datetime.datetime.now(pytz.utc): - raise ValueError("the requested schedule is in the past") - except ValueError as value_error: - error_message = ( - f"Error occurred creating a scheduled bulk email task. Schedule provided: '{schedule}'. Error: " - f"{value_error}" - ) - log.error(error_message) - return HttpResponseBadRequest(error_message) + serializer_data = self.serializer_class(data=request.data) + if not serializer_data.is_valid(): + return HttpResponseBadRequest(reason=serializer_data.errors) - # Retrieve the customized email "from address" and email template from site configuration for the course/partner. If - # there is no site configuration enabled for the current site then we use system defaults for both. - from_addr = _get_branded_email_from_address(course_overview) - template_name = _get_branded_email_template(course_overview) + # Skipping serializer validation to avoid potential disruptions. + # The API handles numerous input variations, and changes here could introduce breaking issues. - # Create the CourseEmail object. This is saved immediately so that any transaction that has been pending up to this - # point will also be committed. - try: - email = create_course_email( - course_id, - request.user, - targets, - subject, - message, - template_name=template_name, - from_addr=from_addr, - ) - except ValueError as err: - return HttpResponseBadRequest(repr(err)) + targets = json.loads(request.POST.get("send_to")) - # Submit the task, so that the correct InstructorTask object gets created (for monitoring purposes) - task_api.submit_bulk_course_email(request, course_id, email.id, schedule_dt) + subject = serializer_data.validated_data.get("subject") + message = serializer_data.validated_data.get("message") + # optional, this is a date and time in the form of an ISO8601 string + schedule = serializer_data.validated_data.get("schedule", "") - response_payload = { - 'course_id': str(course_id), - 'success': True, - } + schedule_dt = None + if schedule: + try: + # convert the schedule from a string to a datetime, then check if its a + # valid future date and time, dateutil + # will throw a ValueError if the schedule is no good. + schedule_dt = dateutil.parser.parse(schedule).replace(tzinfo=pytz.utc) + if schedule_dt < datetime.datetime.now(pytz.utc): + raise ValueError("the requested schedule is in the past") + except ValueError as value_error: + error_message = ( + f"Error occurred creating a scheduled bulk email task. Schedule provided: '{schedule}'. Error: " + f"{value_error}" + ) + log.error(error_message) + return HttpResponseBadRequest(error_message) - return JsonResponse(response_payload) + # Retrieve the customized email "from address" and email template from site configuration for the c + # ourse/partner. + # If there is no site configuration enabled for the current site then we use system defaults for both. + from_addr = _get_branded_email_from_address(course_overview) + template_name = _get_branded_email_template(course_overview) + + # Create the CourseEmail object. This is saved immediately so that any transaction that has been + # pending up to this point will also be committed. + try: + email = create_course_email( + course_id, + request.user, + targets, + subject, + message, + template_name=template_name, + from_addr=from_addr, + ) + except ValueError as err: + return HttpResponseBadRequest(repr(err)) + + # Submit the task, so that the correct InstructorTask object gets created (for monitoring purposes) + task_api.submit_bulk_course_email(request, course_id, email.id, schedule_dt) + + response_payload = { + 'course_id': str(course_id), + 'success': True, + } + + return JsonResponse(response_payload) @require_POST diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index 14fe15c83c2e..b54d38d80451 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -34,7 +34,7 @@ path('get_anon_ids', api.GetAnonIds.as_view(), name='get_anon_ids'), path('get_student_enrollment_status', api.get_student_enrollment_status, name="get_student_enrollment_status"), path('get_student_progress_url', api.StudentProgressUrl.as_view(), name='get_student_progress_url'), - path('reset_student_attempts', api.reset_student_attempts, name='reset_student_attempts'), + path('reset_student_attempts', api.ResetStudentAttempts.as_view(), name='reset_student_attempts'), path('rescore_problem', api.rescore_problem, name='rescore_problem'), path('override_problem_score', api.override_problem_score, name='override_problem_score'), path('reset_student_attempts_for_entrance_exam', api.reset_student_attempts_for_entrance_exam, @@ -49,7 +49,7 @@ path('list_email_content', api.ListEmailContent.as_view(), name='list_email_content'), path('list_forum_members', api.list_forum_members, name='list_forum_members'), path('update_forum_role_membership', api.update_forum_role_membership, name='update_forum_role_membership'), - path('send_email', api.send_email, name='send_email'), + path('send_email', api.SendEmail.as_view(), name='send_email'), path('change_due_date', api.change_due_date, name='change_due_date'), path('reset_due_date', api.reset_due_date, name='reset_due_date'), path('show_unit_extensions', api.show_unit_extensions, name='show_unit_extensions'), @@ -59,7 +59,7 @@ path('get_proctored_exam_results', api.get_proctored_exam_results, name='get_proctored_exam_results'), # Grade downloads... - path('list_report_downloads', api.list_report_downloads, name='list_report_downloads'), + path('list_report_downloads', api.ListReportDownloads.as_view(), name='list_report_downloads'), path('calculate_grades_csv', api.calculate_grades_csv, name='calculate_grades_csv'), path('problem_grade_report', api.problem_grade_report, name='problem_grade_report'), diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index 0697bed6832d..d4a3d4e8328a 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -77,3 +77,75 @@ def validate_student(self, value): return None return user + + +class StudentAttemptsSerializer(serializers.Serializer): + """ + Serializer for resetting a students attempts counter or starts a task to reset all students + attempts counters. + """ + problem_to_reset = serializers.CharField( + help_text="The identifier or description of the problem that needs to be reset." + ) + + # following are optional params. + unique_student_identifier = serializers.CharField( + help_text="Email or username of student.", required=False + ) + all_students = serializers.CharField(required=False) + delete_module = serializers.CharField(required=False) + + def validate_all_students(self, value): + """ + converts the all_student params value to bool. + """ + return self.verify_bool(value) + + def validate_delete_module(self, value): + """ + converts the all_student params value. + """ + return self.verify_bool(value) + + def validate_unique_student_identifier(self, value): + """ + Validate that the student corresponds to an existing user. + """ + try: + user = get_student_from_identifier(value) + except User.DoesNotExist: + return None + + return user + + def verify_bool(self, value): + """ + Returns the value of the boolean parameter with the given + name in the POST request. Handles translation from string + values to boolean values. + """ + if value is not None: + return value in ['true', 'True', True] + + return False + + +class SendEmailSerializer(serializers.Serializer): + """ + Serializer for sending an email with optional scheduling. + + Fields: + send_to (str): The email address of the recipient. This field is required. + subject (str): The subject line of the email. This field is required. + message (str): The body of the email. This field is required. + schedule (str, optional): + An optional field to specify when the email should be sent. + If provided, this should be a string that can be parsed into a + datetime format or some other scheduling logic. + """ + send_to = serializers.CharField(write_only=True, required=True) + + # set max length as per model field. + subject = serializers.CharField(max_length=128, write_only=True, required=True) + message = serializers.CharField(required=True) + schedule = serializers.CharField(required=False) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index c974fa0c8e5f..f61b90d682ff 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -1,12 +1,25 @@ """ API module. """ +import logging + from django.conf import settings +from django.contrib.auth import get_user_model from django.utils.translation import gettext as _ +from datetime import datetime +from typing import Optional + from lms.djangoapps.verify_student.emails import send_verification_approved_email +from lms.djangoapps.verify_student.exceptions import VerificationAttemptInvalidStatus +from lms.djangoapps.verify_student.models import VerificationAttempt +from lms.djangoapps.verify_student.statuses import VerificationAttemptStatus from lms.djangoapps.verify_student.tasks import send_verification_status_email +log = logging.getLogger(__name__) + +User = get_user_model() + def send_approval_email(attempt): """ @@ -33,3 +46,82 @@ def send_approval_email(attempt): else: email_context = {'user': attempt.user, 'expiration_datetime': expiration_datetime.strftime("%m/%d/%Y")} send_verification_approved_email(context=email_context) + + +def create_verification_attempt(user: User, name: str, status: str, expiration_datetime: Optional[datetime] = None): + """ + Create a verification attempt. + + This method is intended to be used by IDV implementation plugins to create VerificationAttempt instances. + + Args: + user (User): the user (usually a learner) performing the verification attempt + name (string): the name being ID verified + status (string): the initial status of the verification attempt + expiration_datetime (datetime, optional): When the verification attempt expires. Defaults to None. + + Returns: + id (int): The id of the created VerificationAttempt instance + """ + verification_attempt = VerificationAttempt.objects.create( + user=user, + name=name, + status=status, + expiration_datetime=expiration_datetime, + ) + + return verification_attempt.id + + +def update_verification_attempt( + attempt_id: int, + name: Optional[str] = None, + status: Optional[str] = None, + expiration_datetime: Optional[datetime] = None +): + """ + Update a verification attempt. + + This method is intended to be used by IDV implementation plugins to update VerificationAttempt instances. + + Arguments: + * attempt_id (int): the verification attempt id of the attempt to update + * name (string, optional): the new name being ID verified + * status (string, optional): the new status of the verification attempt + * expiration_datetime (datetime, optional): The new expiration date and time + + Returns: + * None + """ + try: + attempt = VerificationAttempt.objects.get(id=attempt_id) + except VerificationAttempt.DoesNotExist: + log.error( + f'VerificationAttempt with id {attempt_id} was not found ' + f'when updating the attempt to status={status}', + ) + raise + + if name is not None: + attempt.name = name + + if status is not None: + attempt.status = status + + status_list = list(VerificationAttemptStatus) + if status not in status_list: + log.error( + 'Attempted to call update_verification_attempt called with invalid status: %(status)s. ' + 'Status must be one of: %(status_list)s', + { + 'status': status, + 'status_list': VerificationAttempt.STATUS_CHOICES, + }, + ) + raise VerificationAttemptInvalidStatus + + # NOTE: Generally, we only set the expiration date from the time that an IDV attempt is marked approved, + # so we allow expiration_datetime to = None for other status updates (e.g. pending). + attempt.expiration_datetime = expiration_datetime + + attempt.save() diff --git a/lms/djangoapps/verify_student/exceptions.py b/lms/djangoapps/verify_student/exceptions.py index 59e7d5623f05..d13e52d3e737 100644 --- a/lms/djangoapps/verify_student/exceptions.py +++ b/lms/djangoapps/verify_student/exceptions.py @@ -5,3 +5,7 @@ class WindowExpiredException(Exception): pass + + +class VerificationAttemptInvalidStatus(Exception): + pass diff --git a/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py b/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py index b87b2eee4559..3a08ede0aaf6 100644 --- a/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py +++ b/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py @@ -8,7 +8,6 @@ import time from pprint import pformat -from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.management.base import BaseCommand, CommandError from lms.djangoapps.verify_student.api import send_approval_email diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 903d80bf9245..57ef79e8b05b 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1203,10 +1203,10 @@ class VerificationAttempt(TimeStampedModel): name = models.CharField(blank=True, max_length=255) STATUS_CHOICES = [ - VerificationAttemptStatus.created, - VerificationAttemptStatus.pending, - VerificationAttemptStatus.approved, - VerificationAttemptStatus.denied, + VerificationAttemptStatus.CREATED, + VerificationAttemptStatus.PENDING, + VerificationAttemptStatus.APPROVED, + VerificationAttemptStatus.DENIED, ] status = models.CharField(max_length=64, choices=[(status, status) for status in STATUS_CHOICES]) @@ -1214,3 +1214,13 @@ class VerificationAttempt(TimeStampedModel): null=True, blank=True, ) + + @classmethod + def retire_user(cls, user_id): + """ + Retire user as part of GDPR pipeline + + :param user_id: int + """ + verification_attempts = cls.objects.filter(user_id=user_id) + verification_attempts.delete() diff --git a/lms/djangoapps/verify_student/signals.py b/lms/djangoapps/verify_student/signals.py index d929af68dd06..ae54deb74214 100644 --- a/lms/djangoapps/verify_student/signals.py +++ b/lms/djangoapps/verify_student/signals.py @@ -10,9 +10,9 @@ from xmodule.modulestore.django import SignalHandler, modulestore from common.djangoapps.student.models_api import get_name, get_pending_name_change -from openedx.core.djangoapps.user_api.accounts.signals import USER_RETIRE_LMS_CRITICAL +from openedx.core.djangoapps.user_api.accounts.signals import USER_RETIRE_LMS_CRITICAL, USER_RETIRE_LMS_MISC -from .models import SoftwareSecurePhotoVerification, VerificationDeadline +from .models import SoftwareSecurePhotoVerification, VerificationDeadline, VerificationAttempt log = logging.getLogger(__name__) @@ -75,3 +75,9 @@ def send_idv_update(sender, instance, **kwargs): # pylint: disable=unused-argum photo_id_name=instance.name, full_name=full_name ) + + +@receiver(USER_RETIRE_LMS_MISC) +def _listen_for_lms_retire_verification_attempts(sender, **kwargs): # pylint: disable=unused-argument + user = kwargs.get('user') + VerificationAttempt.retire_user(user.id) diff --git a/lms/djangoapps/verify_student/statuses.py b/lms/djangoapps/verify_student/statuses.py index b55a9042e0f6..41ef381cfe06 100644 --- a/lms/djangoapps/verify_student/statuses.py +++ b/lms/djangoapps/verify_student/statuses.py @@ -1,21 +1,22 @@ """ Status enums for verify_student. """ +from enum import StrEnum, auto -class VerificationAttemptStatus: +class VerificationAttemptStatus(StrEnum): """This class describes valid statuses for a verification attempt to be in.""" # This is the initial state of a verification attempt, before a learner has started IDV. - created = "created" + CREATED = auto() # A verification attempt is pending when it has been started but has not yet been completed. - pending = "pending" + PENDING = auto() # A verification attempt is approved when it has been approved by some mechanism (e.g. automatic review, manual # review, etc). - approved = "approved" + APPROVED = auto() # A verification attempt is denied when it has been denied by some mechanism (e.g. automatic review, manual review, # etc). - denied = "denied" + DENIED = auto() diff --git a/lms/djangoapps/verify_student/tests/factories.py b/lms/djangoapps/verify_student/tests/factories.py index da35e98cc53f..d7eaeaf30211 100644 --- a/lms/djangoapps/verify_student/tests/factories.py +++ b/lms/djangoapps/verify_student/tests/factories.py @@ -3,7 +3,7 @@ """ from factory.django import DjangoModelFactory -from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification, SSOVerification +from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification, SSOVerification, VerificationAttempt class SoftwareSecurePhotoVerificationFactory(DjangoModelFactory): @@ -19,3 +19,8 @@ class Meta: class SSOVerificationFactory(DjangoModelFactory): class Meta(): model = SSOVerification + + +class VerificationAttemptFactory(DjangoModelFactory): + class Meta: + model = VerificationAttempt diff --git a/lms/djangoapps/verify_student/tests/test_api.py b/lms/djangoapps/verify_student/tests/test_api.py index acdebaa70c1c..747c76f82b61 100644 --- a/lms/djangoapps/verify_student/tests/test_api.py +++ b/lms/djangoapps/verify_student/tests/test_api.py @@ -3,14 +3,21 @@ """ from unittest.mock import patch +from datetime import datetime, timezone import ddt from django.conf import settings from django.core import mail from django.test import TestCase from common.djangoapps.student.tests.factories import UserFactory -from lms.djangoapps.verify_student.api import send_approval_email -from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification +from lms.djangoapps.verify_student.api import ( + create_verification_attempt, + send_approval_email, + update_verification_attempt, +) +from lms.djangoapps.verify_student.exceptions import VerificationAttemptInvalidStatus +from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification, VerificationAttempt +from lms.djangoapps.verify_student.statuses import VerificationAttemptStatus @ddt.ddt @@ -18,6 +25,7 @@ class TestSendApprovalEmail(TestCase): """ Test cases for the send_approval_email API method. """ + def setUp(self): super().setUp() @@ -41,3 +49,138 @@ def test_send_approval(self, use_ace): with patch.dict(settings.VERIFY_STUDENT, {'USE_DJANGO_MAIL': use_ace}): send_approval_email(self.attempt) self._assert_verification_approved_email(self.attempt.expiration_datetime) + + +@ddt.ddt +class CreateVerificationAttempt(TestCase): + """ + Test cases for the create_verification_attempt API method. + """ + + def setUp(self): + super().setUp() + + self.user = UserFactory.create() + self.attempt = VerificationAttempt( + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.CREATED, + expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) + ) + self.attempt.save() + + def test_create_verification_attempt(self): + expected_id = 2 + self.assertEqual( + create_verification_attempt( + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.CREATED, + expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) + ), + expected_id + ) + verification_attempt = VerificationAttempt.objects.get(id=expected_id) + + self.assertEqual(verification_attempt.user, self.user) + self.assertEqual(verification_attempt.name, 'Tester McTest') + self.assertEqual(verification_attempt.status, VerificationAttemptStatus.CREATED) + self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo=timezone.utc)) + + def test_create_verification_attempt_no_expiration_datetime(self): + expected_id = 2 + self.assertEqual( + create_verification_attempt( + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.CREATED, + ), + expected_id + ) + verification_attempt = VerificationAttempt.objects.get(id=expected_id) + + self.assertEqual(verification_attempt.user, self.user) + self.assertEqual(verification_attempt.name, 'Tester McTest') + self.assertEqual(verification_attempt.status, VerificationAttemptStatus.CREATED) + self.assertEqual(verification_attempt.expiration_datetime, None) + + +@ddt.ddt +class UpdateVerificationAttempt(TestCase): + """ + Test cases for the update_verification_attempt API method. + """ + + def setUp(self): + super().setUp() + + self.user = UserFactory.create() + self.attempt = VerificationAttempt( + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.CREATED, + expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) + ) + self.attempt.save() + + @ddt.data( + ('Tester McTest', VerificationAttemptStatus.PENDING, datetime(2024, 12, 31, tzinfo=timezone.utc)), + ('Tester McTest2', VerificationAttemptStatus.APPROVED, datetime(2025, 12, 31, tzinfo=timezone.utc)), + ('Tester McTest3', VerificationAttemptStatus.DENIED, datetime(2026, 12, 31, tzinfo=timezone.utc)), + ) + @ddt.unpack + def test_update_verification_attempt(self, name, status, expiration_datetime): + update_verification_attempt( + attempt_id=self.attempt.id, + name=name, + status=status, + expiration_datetime=expiration_datetime, + ) + + verification_attempt = VerificationAttempt.objects.get(id=self.attempt.id) + + # Values should change as a result of this update. + self.assertEqual(verification_attempt.user, self.user) + self.assertEqual(verification_attempt.name, name) + self.assertEqual(verification_attempt.status, status) + self.assertEqual(verification_attempt.expiration_datetime, expiration_datetime) + + def test_update_verification_attempt_none_values(self): + update_verification_attempt( + attempt_id=self.attempt.id, + name=None, + status=None, + expiration_datetime=None, + ) + + verification_attempt = VerificationAttempt.objects.get(id=self.attempt.id) + + # Values should not change as a result of the values passed in being None, except for expiration_datetime. + self.assertEqual(verification_attempt.user, self.user) + self.assertEqual(verification_attempt.name, self.attempt.name) + self.assertEqual(verification_attempt.status, self.attempt.status) + self.assertEqual(verification_attempt.expiration_datetime, None) + + def test_update_verification_attempt_not_found(self): + self.assertRaises( + VerificationAttempt.DoesNotExist, + update_verification_attempt, + attempt_id=999999, + status=VerificationAttemptStatus.APPROVED, + ) + + @ddt.data( + 'completed', + 'failed', + 'submitted', + 'expired', + ) + def test_update_verification_attempt_invalid(self, status): + self.assertRaises( + VerificationAttemptInvalidStatus, + update_verification_attempt, + attempt_id=self.attempt.id, + name=None, + status=status, + expiration_datetime=None, + ) diff --git a/lms/djangoapps/verify_student/tests/test_signals.py b/lms/djangoapps/verify_student/tests/test_signals.py index fb32edeccde0..8d607988d4b4 100644 --- a/lms/djangoapps/verify_student/tests/test_signals.py +++ b/lms/djangoapps/verify_student/tests/test_signals.py @@ -10,9 +10,20 @@ from common.djangoapps.student.models_api import do_name_change_request from common.djangoapps.student.tests.factories import UserFactory -from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification, VerificationDeadline -from lms.djangoapps.verify_student.signals import _listen_for_course_publish, _listen_for_lms_retire -from lms.djangoapps.verify_student.tests.factories import SoftwareSecurePhotoVerificationFactory +from lms.djangoapps.verify_student.models import ( + SoftwareSecurePhotoVerification, + VerificationDeadline, + VerificationAttempt +) +from lms.djangoapps.verify_student.signals import ( + _listen_for_course_publish, + _listen_for_lms_retire, + _listen_for_lms_retire_verification_attempts +) +from lms.djangoapps.verify_student.tests.factories import ( + SoftwareSecurePhotoVerificationFactory, + VerificationAttemptFactory +) from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import fake_completed_retirement from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order @@ -174,3 +185,26 @@ def test_post_save_signal_pending_name(self, mock_signal): photo_id_name=attempt.name, full_name=pending_name_change.new_name ) + + +class RetirementSignalVerificationAttemptsTest(ModuleStoreTestCase): + """ + Tests for the LMS User Retirement signal for Verification Attempts + """ + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.other_user = UserFactory.create() + VerificationAttemptFactory.create(user=self.user) + VerificationAttemptFactory.create(user=self.other_user) + + def test_retirement_signal(self): + _listen_for_lms_retire_verification_attempts(sender=self.__class__, user=self.user) + self.assertEqual(len(VerificationAttempt.objects.filter(user=self.user)), 0) + self.assertEqual(len(VerificationAttempt.objects.filter(user=self.other_user)), 1) + + def test_retirement_signal_no_attempts(self): + no_attempt_user = UserFactory.create() + _listen_for_lms_retire_verification_attempts(sender=self.__class__, user=no_attempt_user) + self.assertEqual(len(VerificationAttempt.objects.all()), 2) diff --git a/lms/envs/common.py b/lms/envs/common.py index 122ce7383bca..334669215397 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2295,7 +2295,6 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring 'openedx.core.djangoapps.safe_sessions.middleware.EmailChangeMiddleware', 'common.djangoapps.student.middleware.UserStandingMiddleware', - 'openedx.core.djangoapps.contentserver.middleware.StaticContentServerMiddleware', # Adds user tags to tracking events # Must go before TrackMiddleware, to get the context set up diff --git a/lms/static/sass/multicourse/_course_about.scss b/lms/static/sass/multicourse/_course_about.scss index 629b3065778e..a8a34ccec589 100644 --- a/lms/static/sass/multicourse/_course_about.scss +++ b/lms/static/sass/multicourse/_course_about.scss @@ -44,6 +44,11 @@ > div.table { display: table; width: 100%; + + @include media-breakpoint-down(sm) { + display: flex; + flex-direction: column; + } } .intro { @@ -51,6 +56,11 @@ @include clearfix(); + @include media-breakpoint-down(sm) { + width: auto; + order: 2; + } + display: table-cell; vertical-align: middle; padding: $baseline; @@ -127,6 +137,10 @@ a.add-to-cart { @include button(shiny, $button-color); + @include media-breakpoint-down(md) { + width: 100%; + } + box-sizing: border-box; border-radius: 3px; display: block; @@ -189,6 +203,11 @@ @include float(left); @include margin(1px, flex-gutter(8), 0, 0); @include transition(none); + @include media-breakpoint-down(md) { + width: 100%; + margin-right: 0; + margin-bottom: 10px; + } width: flex-grid(5, 8); } @@ -213,6 +232,11 @@ width: flex-grid(4); z-index: 2; + @include media-breakpoint-down(sm) { + width: auto; + order: 1; + } + .hero { border: 1px solid $border-color-3; height: 100%; diff --git a/lms/templates/courseware/course_about.html b/lms/templates/courseware/course_about.html index 91d7a2a28e57..eec9caeadbec 100644 --- a/lms/templates/courseware/course_about.html +++ b/lms/templates/courseware/course_about.html @@ -62,11 +62,10 @@
-

- ${course.display_name_with_default} -

+

${course.display_org_with_default}

+

${course.display_name_with_default}


- ${course.display_org_with_default} +

${get_course_about_section(request, course, 'short_description')}

@@ -160,7 +159,11 @@

<%block name="course_about_important_dates">
    -

  1. ${_("Course Number")}

    ${course.display_number_with_default}
    2. +
  2. + +

    ${_("Course Number")}

    + ${course.display_number_with_default} +
    3. % if not course.start_date_is_still_default: <% course_start_date = course.advertised_start or course.start @@ -231,7 +234,11 @@

    % endif % if get_course_about_section(request, course, "prerequisites"): -

  3. ${_("Requirements")}

    ${get_course_about_section(request, course, "prerequisites")}
    4. +
  4. + +

    ${_("Requirements")}

    + ${get_course_about_section(request, course, "prerequisites")} +
    5. % endif

diff --git a/lms/templates/courseware/courseware-chromeless.html b/lms/templates/courseware/courseware-chromeless.html index e8411c9e4217..deeda26c431d 100644 --- a/lms/templates/courseware/courseware-chromeless.html +++ b/lms/templates/courseware/courseware-chromeless.html @@ -144,10 +144,12 @@ // to stay in relatively the same position so viewing of the video // is not disrupted. if ($(this).attr('class') === 'transcript-start'||$(this).attr('class') === 'transcript-end') { + var target = $(targetId)[0]; event.preventDefault(); - $(targetId)[0].scrollIntoView({ + target.scrollIntoView({ block: 'nearest', }); + target.focus(); } else { var targetName = $(this).attr('href').slice(1); // Checks if the target uses an id or name. @@ -159,6 +161,7 @@ target.scrollIntoView({ block: 'start', }); + target.focus(); } } } diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 12a11e96e13c..2062f96d93ae 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -268,15 +268,7 @@ class ContentLibraryCollectionUpdateSerializer(serializers.Serializer): """ title = serializers.CharField() - description = serializers.CharField() - - -class ContentLibraryCollectionCreateSerializer(ContentLibraryCollectionUpdateSerializer): - """ - Serializer for adding a Collection in a Content Library - """ - - key = serializers.CharField() + description = serializers.CharField(allow_blank=True) class UsageKeyV2Serializer(serializers.Serializer): diff --git a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py index ad027f2412bc..bc600759b5b3 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py @@ -162,16 +162,15 @@ def test_create_library_collection(self): Test creating a Content Library Collection """ post_data = { - "key": "COL4", "title": "Collection 4", "description": "Description for Collection 4", } resp = self.client.post( URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data, format="json" ) - # Check that the new Content Library Collection is returned in response and created in DB assert resp.status_code == 200 + post_data["key"] = 'collection-4' self.assertDictContainsEntries(resp.data, post_data) created_collection = Collection.objects.get(id=resp.data["id"]) @@ -183,7 +182,6 @@ def test_create_library_collection(self): with self.as_user(reader): post_data = { - "key": "COL5", "title": "Collection 5", "description": "Description for Collection 5", } @@ -193,6 +191,31 @@ def test_create_library_collection(self): assert resp.status_code == 403 + def test_create_collection_same_key(self): + """ + Test collection creation with same key + """ + post_data = { + "title": "Same Collection", + "description": "Description for Collection 4", + } + self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data, format="json" + ) + + for i in range(100): + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data, format="json" + ) + expected_data = { + "key": f"same-collection-{i + 1}", + "title": "Same Collection", + "description": "Description for Collection 4", + } + + assert resp.status_code == 200 + self.assertDictContainsEntries(resp.data, expected_data) + def test_create_invalid_library_collection(self): """ Test creating an invalid Content Library Collection diff --git a/openedx/core/djangoapps/content_libraries/views_collections.py b/openedx/core/djangoapps/content_libraries/views_collections.py index 482b6e2bb9f1..2f40a1788628 100644 --- a/openedx/core/djangoapps/content_libraries/views_collections.py +++ b/openedx/core/djangoapps/content_libraries/views_collections.py @@ -5,6 +5,8 @@ from __future__ import annotations from django.db.models import QuerySet +from django.utils.text import slugify +from django.db import transaction from rest_framework.decorators import action from rest_framework.response import Response @@ -21,7 +23,6 @@ from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, ContentLibraryCollectionComponentsUpdateSerializer, - ContentLibraryCollectionCreateSerializer, ContentLibraryCollectionUpdateSerializer, ) @@ -109,17 +110,30 @@ def create(self, request, *args, **kwargs) -> Response: Create a Collection that belongs to a Content Library """ content_library = self.get_content_library() - create_serializer = ContentLibraryCollectionCreateSerializer(data=request.data) + create_serializer = ContentLibraryCollectionUpdateSerializer(data=request.data) create_serializer.is_valid(raise_exception=True) - collection = api.create_library_collection( - library_key=content_library.library_key, - content_library=content_library, - collection_key=create_serializer.validated_data["key"], - title=create_serializer.validated_data["title"], - description=create_serializer.validated_data["description"], - created_by=request.user.id, - ) + title = create_serializer.validated_data['title'] + key = slugify(title) + + attempt = 0 + collection = None + while not collection: + modified_key = key if attempt == 0 else key + '-' + str(attempt) + try: + # Add transaction here to avoid TransactionManagementError on retry + with transaction.atomic(): + collection = api.create_library_collection( + library_key=content_library.library_key, + content_library=content_library, + collection_key=modified_key, + title=title, + description=create_serializer.validated_data["description"], + created_by=request.user.id, + ) + except api.LibraryCollectionAlreadyExists: + attempt += 1 + serializer = self.get_serializer(collection) return Response(serializer.data) diff --git a/openedx/core/djangoapps/contentserver/middleware.py b/openedx/core/djangoapps/contentserver/middleware.py deleted file mode 100644 index 6eb72da458a0..000000000000 --- a/openedx/core/djangoapps/contentserver/middleware.py +++ /dev/null @@ -1,368 +0,0 @@ -""" -Middleware to serve assets. -""" - - -import datetime -import logging - -from django.http import ( - HttpResponse, - HttpResponseBadRequest, - HttpResponseForbidden, - HttpResponseNotFound, - HttpResponseNotModified, - HttpResponsePermanentRedirect -) -from django.utils.deprecation import MiddlewareMixin -from edx_django_utils.monitoring import set_custom_attribute -from edx_toggles.toggles import WaffleFlag -from opaque_keys import InvalidKeyError -from opaque_keys.edx.locator import AssetLocator - -from openedx.core.djangoapps.header_control import force_header_for_response -from common.djangoapps.student.models import CourseEnrollment -from xmodule.assetstore.assetmgr import AssetManager # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.contentstore.content import XASSET_LOCATION_TAG, StaticContent # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.exceptions import NotFoundError # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore import InvalidLocationError # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order - -from .caching import get_cached_content, set_cached_content -from .models import CdnUserAgentsConfig, CourseAssetCacheTtlConfig - -log = logging.getLogger(__name__) - -# .. toggle_name: content_server.use_view -# .. toggle_implementation: WaffleFlag -# .. toggle_default: False -# .. toggle_description: Deployment flag for switching asset serving from a middleware -# to a view. Intended to be used once in each environment to test the cutover and -# ensure there are no errors or changes in behavior. Once this has been tested, -# the middleware can be fully converted to a view. -# .. toggle_use_cases: temporary -# .. toggle_creation_date: 2024-05-02 -# .. toggle_target_removal_date: 2024-07-01 -# .. toggle_tickets: https://github.com/openedx/edx-platform/issues/34702 -CONTENT_SERVER_USE_VIEW = WaffleFlag('content_server.use_view', module_name=__name__) - -# TODO: Soon as we have a reasonable way to serialize/deserialize AssetKeys, we need -# to change this file so instead of using course_id_partial, we're just using asset keys - -HTTP_DATE_FORMAT = "%a, %d %b %Y %H:%M:%S GMT" - - -class StaticContentServerMiddleware(MiddlewareMixin): - """ - Shim to maintain old pattern of serving course assets from a middleware. See views.py. - """ - def process_request(self, request): - """Intercept asset request or allow view to handle it, depending on config.""" - if CONTENT_SERVER_USE_VIEW.is_enabled(): - return - else: - set_custom_attribute('content_server.handled_by.middleware', True) - return IMPL.process_request(request) - - -class StaticContentServer(): - """ - Serves course assets to end users. Colloquially referred to as "contentserver." - """ - def is_asset_request(self, request): - """Determines whether the given request is an asset request""" - # Don't change this without updating urls.py! See docstring of views.py. - return ( - request.path.startswith('/' + XASSET_LOCATION_TAG + '/') - or - request.path.startswith('/' + AssetLocator.CANONICAL_NAMESPACE) - or - StaticContent.is_versioned_asset_path(request.path) - ) - - # pylint: disable=too-many-statements - def process_request(self, request): - """Process the given request""" - asset_path = request.path - - if self.is_asset_request(request): # lint-amnesty, pylint: disable=too-many-nested-blocks - # Make sure we can convert this request into a location. - if AssetLocator.CANONICAL_NAMESPACE in asset_path: - asset_path = asset_path.replace('block/', 'block@', 1) - - # If this is a versioned request, pull out the digest and chop off the prefix. - requested_digest = None - if StaticContent.is_versioned_asset_path(asset_path): - requested_digest, asset_path = StaticContent.parse_versioned_asset_path(asset_path) - - # Make sure we have a valid location value for this asset. - try: - loc = StaticContent.get_location_from_path(asset_path) - except (InvalidLocationError, InvalidKeyError): - return HttpResponseBadRequest() - - # Attempt to load the asset to make sure it exists, and grab the asset digest - # if we're able to load it. - actual_digest = None - try: - content = self.load_asset_from_location(loc) - actual_digest = getattr(content, "content_digest", None) - except (ItemNotFoundError, NotFoundError): - return HttpResponseNotFound() - - # If this was a versioned asset, and the digest doesn't match, redirect - # them to the actual version. - if requested_digest is not None and actual_digest is not None and (actual_digest != requested_digest): - actual_asset_path = StaticContent.add_version_to_asset_path(asset_path, actual_digest) - return HttpResponsePermanentRedirect(actual_asset_path) - - # Set the basics for this request. Make sure that the course key for this - # asset has a run, which old-style courses do not. Otherwise, this will - # explode when the key is serialized to be sent to NR. - safe_course_key = loc.course_key - if safe_course_key.run is None: - safe_course_key = safe_course_key.replace(run='only') - - set_custom_attribute('course_id', safe_course_key) - set_custom_attribute('org', loc.org) - set_custom_attribute('contentserver.path', loc.path) - - # Figure out if this is a CDN using us as the origin. - is_from_cdn = StaticContentServer.is_cdn_request(request) - set_custom_attribute('contentserver.from_cdn', is_from_cdn) - - # Check if this content is locked or not. - locked = self.is_content_locked(content) - set_custom_attribute('contentserver.locked', locked) - - # Check that user has access to the content. - if not self.is_user_authorized(request, content, loc): - return HttpResponseForbidden('Unauthorized') - - # Figure out if the client sent us a conditional request, and let them know - # if this asset has changed since then. - last_modified_at_str = content.last_modified_at.strftime(HTTP_DATE_FORMAT) - if 'HTTP_IF_MODIFIED_SINCE' in request.META: - if_modified_since = request.META['HTTP_IF_MODIFIED_SINCE'] - if if_modified_since == last_modified_at_str: - return HttpResponseNotModified() - - # *** File streaming within a byte range *** - # If a Range is provided, parse Range attribute of the request - # Add Content-Range in the response if Range is structurally correct - # Request -> Range attribute structure: "Range: bytes=first-[last]" - # Response -> Content-Range attribute structure: "Content-Range: bytes first-last/totalLength" - # http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.35 - response = None - if request.META.get('HTTP_RANGE'): - # If we have a StaticContent, get a StaticContentStream. Can't manipulate the bytes otherwise. - if isinstance(content, StaticContent): - content = AssetManager.find(loc, as_stream=True) - - header_value = request.META['HTTP_RANGE'] - try: - unit, ranges = parse_range_header(header_value, content.length) - except ValueError as exception: - # If the header field is syntactically invalid it should be ignored. - log.exception( - "%s in Range header: %s for content: %s", - str(exception), header_value, str(loc) - ) - else: - if unit != 'bytes': - # Only accept ranges in bytes - log.warning("Unknown unit in Range header: %s for content: %s", header_value, str(loc)) - elif len(ranges) > 1: - # According to Http/1.1 spec content for multiple ranges should be sent as a multipart message. - # http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.16 - # But we send back the full content. - log.warning( - "More than 1 ranges in Range header: %s for content: %s", header_value, str(loc) - ) - else: - first, last = ranges[0] - - if 0 <= first <= last < content.length: - # If the byte range is satisfiable - response = HttpResponse(content.stream_data_in_range(first, last)) - response['Content-Range'] = 'bytes {first}-{last}/{length}'.format( - first=first, last=last, length=content.length - ) - response['Content-Length'] = str(last - first + 1) - response.status_code = 206 # Partial Content - - set_custom_attribute('contentserver.ranged', True) - else: - log.warning( - "Cannot satisfy ranges in Range header: %s for content: %s", - header_value, str(loc) - ) - return HttpResponse(status=416) # Requested Range Not Satisfiable - - # If Range header is absent or syntactically invalid return a full content response. - if response is None: - response = HttpResponse(content.stream_data()) - response['Content-Length'] = content.length - - set_custom_attribute('contentserver.content_len', content.length) - set_custom_attribute('contentserver.content_type', content.content_type) - - # "Accept-Ranges: bytes" tells the user that only "bytes" ranges are allowed - response['Accept-Ranges'] = 'bytes' - response['Content-Type'] = content.content_type - response['X-Frame-Options'] = 'ALLOW' - - # Set any caching headers, and do any response cleanup needed. Based on how much - # middleware we have in place, there's no easy way to use the built-in Django - # utilities and properly sanitize and modify a response to ensure that it is as - # cacheable as possible, which is why we do it ourselves. - self.set_caching_headers(content, response) - - return response - - def set_caching_headers(self, content, response): - """ - Sets caching headers based on whether or not the asset is locked. - """ - - is_locked = getattr(content, "locked", False) - - # We want to signal to the end user's browser, and to any intermediate proxies/caches, - # whether or not this asset is cacheable. If we have a TTL configured, we inform the - # caller, for unlocked assets, how long they are allowed to cache it. Since locked - # assets should be restricted to enrolled students, we simply send headers that - # indicate there should be no caching whatsoever. - cache_ttl = CourseAssetCacheTtlConfig.get_cache_ttl() - if cache_ttl > 0 and not is_locked: - set_custom_attribute('contentserver.cacheable', True) - - response['Expires'] = StaticContentServer.get_expiration_value(datetime.datetime.utcnow(), cache_ttl) - response['Cache-Control'] = "public, max-age={ttl}, s-maxage={ttl}".format(ttl=cache_ttl) - elif is_locked: - set_custom_attribute('contentserver.cacheable', False) - - response['Cache-Control'] = "private, no-cache, no-store" - - response['Last-Modified'] = content.last_modified_at.strftime(HTTP_DATE_FORMAT) - - # Force the Vary header to only vary responses on Origin, so that XHR and browser requests get cached - # separately and don't screw over one another. i.e. a browser request that doesn't send Origin, and - # caches a version of the response without CORS headers, in turn breaking XHR requests. - force_header_for_response(response, 'Vary', 'Origin') - - @staticmethod - def is_cdn_request(request): - """ - Attempts to determine whether or not the given request is coming from a CDN. - - Currently, this is a static check because edx.org only uses CloudFront, but may - be expanded in the future. - """ - cdn_user_agents = CdnUserAgentsConfig.get_cdn_user_agents() - user_agent = request.META.get('HTTP_USER_AGENT', '') - if user_agent in cdn_user_agents: - # This is a CDN request. - return True - - return False - - @staticmethod - def get_expiration_value(now, cache_ttl): - """Generates an RFC1123 datetime string based on a future offset.""" - expire_dt = now + datetime.timedelta(seconds=cache_ttl) - return expire_dt.strftime(HTTP_DATE_FORMAT) - - def is_content_locked(self, content): - """ - Determines whether or not the given content is locked. - """ - return bool(getattr(content, "locked", False)) - - def is_user_authorized(self, request, content, location): - """ - Determines whether or not the user for this request is authorized to view the given asset. - """ - if not self.is_content_locked(content): - return True - - if not hasattr(request, "user") or not request.user.is_authenticated: - return False - - if not request.user.is_staff: - deprecated = getattr(location, 'deprecated', False) - if deprecated and not CourseEnrollment.is_enrolled_by_partial(request.user, location.course_key): - return False - if not deprecated and not CourseEnrollment.is_enrolled(request.user, location.course_key): - return False - - return True - - def load_asset_from_location(self, location): - """ - Loads an asset based on its location, either retrieving it from a cache - or loading it directly from the contentstore. - """ - - # See if we can load this item from cache. - content = get_cached_content(location) - if content is None: - # Not in cache, so just try and load it from the asset manager. - try: - content = AssetManager.find(location, as_stream=True) - except (ItemNotFoundError, NotFoundError): # lint-amnesty, pylint: disable=try-except-raise - raise - - # Now that we fetched it, let's go ahead and try to cache it. We cap this at 1MB - # because it's the default for memcached and also we don't want to do too much - # buffering in memory when we're serving an actual request. - if content.length is not None and content.length < 1048576: - content = content.copy_to_in_mem() - set_cached_content(content) - - return content - - -IMPL = StaticContentServer() - - -def parse_range_header(header_value, content_length): - """ - Returns the unit and a list of (start, end) tuples of ranges. - - Raises ValueError if header is syntactically invalid or does not contain a range. - - See spec for details: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.35 - """ - - unit = None - ranges = [] - - if '=' in header_value: - unit, byte_ranges_string = header_value.split('=') - - # Parse the byte ranges. - for byte_range_string in byte_ranges_string.split(','): - byte_range_string = byte_range_string.strip() - # Case 0: - if '-' not in byte_range_string: # Invalid syntax of header value. # lint-amnesty, pylint: disable=no-else-raise - raise ValueError('Invalid syntax.') - # Case 1: -500 - elif byte_range_string.startswith('-'): - first = max(0, (content_length + int(byte_range_string))) - last = content_length - 1 - # Case 2: 500- - elif byte_range_string.endswith('-'): - first = int(byte_range_string[0:-1]) - last = content_length - 1 - # Case 3: 500-999 - else: - first, last = byte_range_string.split('-') - first = int(first) - last = min(int(last), content_length - 1) - - ranges.append((first, last)) - - if len(ranges) == 0: - raise ValueError('Invalid syntax') - - return unit, ranges diff --git a/openedx/core/djangoapps/contentserver/test/test_contentserver.py b/openedx/core/djangoapps/contentserver/test/test_contentserver.py index e9ee1f7ee5b2..e1c5e01c0a7f 100644 --- a/openedx/core/djangoapps/contentserver/test/test_contentserver.py +++ b/openedx/core/djangoapps/contentserver/test/test_contentserver.py @@ -4,7 +4,6 @@ import copy - import datetime import logging import unittest @@ -17,18 +16,18 @@ from django.test.client import Client from django.test.utils import override_settings from opaque_keys import InvalidKeyError + +from common.djangoapps.student.models import CourseEnrollment +from common.djangoapps.student.tests.factories import AdminFactory, UserFactory +from xmodule.assetstore.assetmgr import AssetManager +from xmodule.contentstore.content import VERSIONED_ASSETS_PREFIX, StaticContent from xmodule.contentstore.django import contentstore -from xmodule.contentstore.content import StaticContent, VERSIONED_ASSETS_PREFIX from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, SharedModuleStoreTestCase from xmodule.modulestore.xml_importer import import_course_from_xml -from xmodule.assetstore.assetmgr import AssetManager -from xmodule.modulestore.exceptions import ItemNotFoundError - -from common.djangoapps.student.models import CourseEnrollment -from common.djangoapps.student.tests.factories import UserFactory, AdminFactory -from ..middleware import parse_range_header, HTTP_DATE_FORMAT, StaticContentServer +from ..views import HTTP_DATE_FORMAT, StaticContentServer, parse_range_header log = logging.getLogger(__name__) diff --git a/openedx/core/djangoapps/contentserver/views.py b/openedx/core/djangoapps/contentserver/views.py index 84bd4c3b1046..7c40026415bb 100644 --- a/openedx/core/djangoapps/contentserver/views.py +++ b/openedx/core/djangoapps/contentserver/views.py @@ -9,28 +9,41 @@ `resolve` utility, but these URLs get a Resolver404 (because there's no registered urlpattern). -We'd like to turn this into a proper view: -https://github.com/openedx/edx-platform/issues/34702 - -The first step, seen here, is to have urlpatterns (redundant with the -middleware's `is_asset_request` method) and a view, but the view just calls into -the same code the middleware uses. The implementation of the middleware has been -moved into StaticContentServerImpl, leaving the middleware as just a shell -around the latter. - -A waffle flag chooses whether to allow the middleware to handle the request, or -whether to pass the request along to the view. Why? Because we might be relying -by accident on some weird behavior inherent to misusing a middleware this way, -and we need a way to quickly switch back if we encounter problems. - -If the view works, we can move all of StaticContentServerImpl directly into the -view and drop the middleware and the waffle flag. +We've turned it into a proper view, with a few warts remaining: + +- The view implementation is all bundled into a StaticContentServer class that + doesn't appear to have any state. The methods could likely just be extracted + as top-level functions. +- All three urlpatterns are registered to the same view, which then has to + re-parse the URL to determine which pattern is in effect. We should probably + have 3 views as entry points. """ -from django.http import HttpResponseNotFound +import datetime +import logging + +from django.http import ( + HttpResponse, + HttpResponseBadRequest, + HttpResponseForbidden, + HttpResponseNotFound, + HttpResponseNotModified, + HttpResponsePermanentRedirect +) from django.views.decorators.http import require_safe from edx_django_utils.monitoring import set_custom_attribute +from opaque_keys import InvalidKeyError +from opaque_keys.edx.locator import AssetLocator + +from common.djangoapps.student.models import CourseEnrollment +from openedx.core.djangoapps.header_control import force_header_for_response +from xmodule.assetstore.assetmgr import AssetManager +from xmodule.contentstore.content import XASSET_LOCATION_TAG, StaticContent +from xmodule.exceptions import NotFoundError +from xmodule.modulestore import InvalidLocationError +from xmodule.modulestore.exceptions import ItemNotFoundError -from .middleware import CONTENT_SERVER_USE_VIEW, IMPL +from .caching import get_cached_content, set_cached_content +from .models import CdnUserAgentsConfig, CourseAssetCacheTtlConfig @require_safe @@ -38,21 +51,315 @@ def course_assets_view(request): """ Serve course assets to end users. Colloquially referred to as "contentserver." """ - set_custom_attribute('content_server.handled_by.view', True) - - if not CONTENT_SERVER_USE_VIEW.is_enabled(): - # Should never happen; keep track of occurrences. - set_custom_attribute('content_server.view.called_when_disabled', True) - # But handle the request anyhow. - - # We'll delegate request handling to an instance of the middleware - # until we can verify that the behavior is identical when requests - # come all the way through to the view. - response = IMPL.process_request(request) - - if response is None: - # Shouldn't happen - set_custom_attribute('content_server.view.no_response_from_impl', True) - return HttpResponseNotFound() - else: - return response + return IMPL.process_request(request) + + +log = logging.getLogger(__name__) + +# TODO: Soon as we have a reasonable way to serialize/deserialize AssetKeys, we need +# to change this file so instead of using course_id_partial, we're just using asset keys + +HTTP_DATE_FORMAT = "%a, %d %b %Y %H:%M:%S GMT" + + +class StaticContentServer(): + """ + Serves course assets to end users. Colloquially referred to as "contentserver." + """ + def is_asset_request(self, request): + """Determines whether the given request is an asset request""" + # Don't change this without updating urls.py! See docstring of views.py. + return ( + request.path.startswith('/' + XASSET_LOCATION_TAG + '/') + or + request.path.startswith('/' + AssetLocator.CANONICAL_NAMESPACE) + or + StaticContent.is_versioned_asset_path(request.path) + ) + + # pylint: disable=too-many-statements + def process_request(self, request): + """Process the given request""" + asset_path = request.path + + if self.is_asset_request(request): # lint-amnesty, pylint: disable=too-many-nested-blocks + # Make sure we can convert this request into a location. + if AssetLocator.CANONICAL_NAMESPACE in asset_path: + asset_path = asset_path.replace('block/', 'block@', 1) + + # If this is a versioned request, pull out the digest and chop off the prefix. + requested_digest = None + if StaticContent.is_versioned_asset_path(asset_path): + requested_digest, asset_path = StaticContent.parse_versioned_asset_path(asset_path) + + # Make sure we have a valid location value for this asset. + try: + loc = StaticContent.get_location_from_path(asset_path) + except (InvalidLocationError, InvalidKeyError): + return HttpResponseBadRequest() + + # Attempt to load the asset to make sure it exists, and grab the asset digest + # if we're able to load it. + actual_digest = None + try: + content = self.load_asset_from_location(loc) + actual_digest = getattr(content, "content_digest", None) + except (ItemNotFoundError, NotFoundError): + return HttpResponseNotFound() + + # If this was a versioned asset, and the digest doesn't match, redirect + # them to the actual version. + if requested_digest is not None and actual_digest is not None and (actual_digest != requested_digest): + actual_asset_path = StaticContent.add_version_to_asset_path(asset_path, actual_digest) + return HttpResponsePermanentRedirect(actual_asset_path) + + # Set the basics for this request. Make sure that the course key for this + # asset has a run, which old-style courses do not. Otherwise, this will + # explode when the key is serialized to be sent to NR. + safe_course_key = loc.course_key + if safe_course_key.run is None: + safe_course_key = safe_course_key.replace(run='only') + + set_custom_attribute('course_id', safe_course_key) + set_custom_attribute('org', loc.org) + set_custom_attribute('contentserver.path', loc.path) + + # Figure out if this is a CDN using us as the origin. + is_from_cdn = StaticContentServer.is_cdn_request(request) + set_custom_attribute('contentserver.from_cdn', is_from_cdn) + + # Check if this content is locked or not. + locked = self.is_content_locked(content) + set_custom_attribute('contentserver.locked', locked) + + # Check that user has access to the content. + if not self.is_user_authorized(request, content, loc): + return HttpResponseForbidden('Unauthorized') + + # Figure out if the client sent us a conditional request, and let them know + # if this asset has changed since then. + last_modified_at_str = content.last_modified_at.strftime(HTTP_DATE_FORMAT) + if 'HTTP_IF_MODIFIED_SINCE' in request.META: + if_modified_since = request.META['HTTP_IF_MODIFIED_SINCE'] + if if_modified_since == last_modified_at_str: + return HttpResponseNotModified() + + # *** File streaming within a byte range *** + # If a Range is provided, parse Range attribute of the request + # Add Content-Range in the response if Range is structurally correct + # Request -> Range attribute structure: "Range: bytes=first-[last]" + # Response -> Content-Range attribute structure: "Content-Range: bytes first-last/totalLength" + # http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.35 + response = None + if request.META.get('HTTP_RANGE'): + # If we have a StaticContent, get a StaticContentStream. Can't manipulate the bytes otherwise. + if isinstance(content, StaticContent): + content = AssetManager.find(loc, as_stream=True) + + header_value = request.META['HTTP_RANGE'] + try: + unit, ranges = parse_range_header(header_value, content.length) + except ValueError as exception: + # If the header field is syntactically invalid it should be ignored. + log.exception( + "%s in Range header: %s for content: %s", + str(exception), header_value, str(loc) + ) + else: + if unit != 'bytes': + # Only accept ranges in bytes + log.warning("Unknown unit in Range header: %s for content: %s", header_value, str(loc)) + elif len(ranges) > 1: + # According to Http/1.1 spec content for multiple ranges should be sent as a multipart message. + # http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.16 + # But we send back the full content. + log.warning( + "More than 1 ranges in Range header: %s for content: %s", header_value, str(loc) + ) + else: + first, last = ranges[0] + + if 0 <= first <= last < content.length: + # If the byte range is satisfiable + response = HttpResponse(content.stream_data_in_range(first, last)) + response['Content-Range'] = 'bytes {first}-{last}/{length}'.format( + first=first, last=last, length=content.length + ) + response['Content-Length'] = str(last - first + 1) + response.status_code = 206 # Partial Content + + set_custom_attribute('contentserver.ranged', True) + else: + log.warning( + "Cannot satisfy ranges in Range header: %s for content: %s", + header_value, str(loc) + ) + return HttpResponse(status=416) # Requested Range Not Satisfiable + + # If Range header is absent or syntactically invalid return a full content response. + if response is None: + response = HttpResponse(content.stream_data()) + response['Content-Length'] = content.length + + set_custom_attribute('contentserver.content_len', content.length) + set_custom_attribute('contentserver.content_type', content.content_type) + + # "Accept-Ranges: bytes" tells the user that only "bytes" ranges are allowed + response['Accept-Ranges'] = 'bytes' + response['Content-Type'] = content.content_type + response['X-Frame-Options'] = 'ALLOW' + + # Set any caching headers, and do any response cleanup needed. Based on how much + # middleware we have in place, there's no easy way to use the built-in Django + # utilities and properly sanitize and modify a response to ensure that it is as + # cacheable as possible, which is why we do it ourselves. + self.set_caching_headers(content, response) + + return response + + def set_caching_headers(self, content, response): + """ + Sets caching headers based on whether or not the asset is locked. + """ + + is_locked = getattr(content, "locked", False) + + # We want to signal to the end user's browser, and to any intermediate proxies/caches, + # whether or not this asset is cacheable. If we have a TTL configured, we inform the + # caller, for unlocked assets, how long they are allowed to cache it. Since locked + # assets should be restricted to enrolled students, we simply send headers that + # indicate there should be no caching whatsoever. + cache_ttl = CourseAssetCacheTtlConfig.get_cache_ttl() + if cache_ttl > 0 and not is_locked: + set_custom_attribute('contentserver.cacheable', True) + + response['Expires'] = StaticContentServer.get_expiration_value(datetime.datetime.utcnow(), cache_ttl) + response['Cache-Control'] = "public, max-age={ttl}, s-maxage={ttl}".format(ttl=cache_ttl) + elif is_locked: + set_custom_attribute('contentserver.cacheable', False) + + response['Cache-Control'] = "private, no-cache, no-store" + + response['Last-Modified'] = content.last_modified_at.strftime(HTTP_DATE_FORMAT) + + # Force the Vary header to only vary responses on Origin, so that XHR and browser requests get cached + # separately and don't screw over one another. i.e. a browser request that doesn't send Origin, and + # caches a version of the response without CORS headers, in turn breaking XHR requests. + force_header_for_response(response, 'Vary', 'Origin') + + @staticmethod + def is_cdn_request(request): + """ + Attempts to determine whether or not the given request is coming from a CDN. + + Currently, this is a static check because edx.org only uses CloudFront, but may + be expanded in the future. + """ + cdn_user_agents = CdnUserAgentsConfig.get_cdn_user_agents() + user_agent = request.META.get('HTTP_USER_AGENT', '') + if user_agent in cdn_user_agents: + # This is a CDN request. + return True + + return False + + @staticmethod + def get_expiration_value(now, cache_ttl): + """Generates an RFC1123 datetime string based on a future offset.""" + expire_dt = now + datetime.timedelta(seconds=cache_ttl) + return expire_dt.strftime(HTTP_DATE_FORMAT) + + def is_content_locked(self, content): + """ + Determines whether or not the given content is locked. + """ + return bool(getattr(content, "locked", False)) + + def is_user_authorized(self, request, content, location): + """ + Determines whether or not the user for this request is authorized to view the given asset. + """ + if not self.is_content_locked(content): + return True + + if not hasattr(request, "user") or not request.user.is_authenticated: + return False + + if not request.user.is_staff: + deprecated = getattr(location, 'deprecated', False) + if deprecated and not CourseEnrollment.is_enrolled_by_partial(request.user, location.course_key): + return False + if not deprecated and not CourseEnrollment.is_enrolled(request.user, location.course_key): + return False + + return True + + def load_asset_from_location(self, location): + """ + Loads an asset based on its location, either retrieving it from a cache + or loading it directly from the contentstore. + """ + + # See if we can load this item from cache. + content = get_cached_content(location) + if content is None: + # Not in cache, so just try and load it from the asset manager. + try: + content = AssetManager.find(location, as_stream=True) + except (ItemNotFoundError, NotFoundError): # lint-amnesty, pylint: disable=try-except-raise + raise + + # Now that we fetched it, let's go ahead and try to cache it. We cap this at 1MB + # because it's the default for memcached and also we don't want to do too much + # buffering in memory when we're serving an actual request. + if content.length is not None and content.length < 1048576: + content = content.copy_to_in_mem() + set_cached_content(content) + + return content + + +IMPL = StaticContentServer() + + +def parse_range_header(header_value, content_length): + """ + Returns the unit and a list of (start, end) tuples of ranges. + + Raises ValueError if header is syntactically invalid or does not contain a range. + + See spec for details: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.35 + """ + + unit = None + ranges = [] + + if '=' in header_value: + unit, byte_ranges_string = header_value.split('=') + + # Parse the byte ranges. + for byte_range_string in byte_ranges_string.split(','): + byte_range_string = byte_range_string.strip() + # Case 0: + if '-' not in byte_range_string: # Invalid syntax of header value. # lint-amnesty, pylint: disable=no-else-raise + raise ValueError('Invalid syntax.') + # Case 1: -500 + elif byte_range_string.startswith('-'): + first = max(0, (content_length + int(byte_range_string))) + last = content_length - 1 + # Case 2: 500- + elif byte_range_string.endswith('-'): + first = int(byte_range_string[0:-1]) + last = content_length - 1 + # Case 3: 500-999 + else: + first, last = byte_range_string.split('-') + first = int(first) + last = min(int(last), content_length - 1) + + ranges.append((first, last)) + + if len(ranges) == 0: + raise ValueError('Invalid syntax') + + return unit, ranges diff --git a/requirements/constraints.txt b/requirements/constraints.txt index fbf15e766149..b5d838156124 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -26,7 +26,7 @@ celery>=5.2.2,<6.0.0 # The team that owns this package will manually bump this package rather than having it pulled in automatically. # This is to allow them to better control its deployment and to do it in a process that works better # for them. -edx-enterprise==4.25.0 +edx-enterprise==4.25.9 # Stay on LTS version, remove once this is added to common constraint Django<5.0 @@ -93,7 +93,7 @@ libsass==0.10.0 click==8.1.6 # pinning this version to avoid updates while the library is being developed -openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key +openedx-learning==0.11.4 # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. openai<=0.28.1 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index d85039139ad0..40d64855cb52 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -467,7 +467,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.25.0 +edx-enterprise==4.25.9 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in @@ -824,7 +824,7 @@ openedx-filters==1.9.0 # -r requirements/edx/kernel.in # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key +openedx-learning==0.11.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 8f401727ded1..a18ddb26b8b9 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -741,7 +741,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.25.0 +edx-enterprise==4.25.9 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt @@ -1373,7 +1373,7 @@ openedx-filters==1.9.0 # -r requirements/edx/testing.txt # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key +openedx-learning==0.11.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 9a3e3ea80367..00fc580dedc4 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -547,7 +547,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.25.0 +edx-enterprise==4.25.9 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt @@ -983,7 +983,7 @@ openedx-filters==1.9.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key +openedx-learning==0.11.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 40d1f7e8dcb9..966bd772a876 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -571,7 +571,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.25.0 +edx-enterprise==4.25.9 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt @@ -1034,7 +1034,7 @@ openedx-filters==1.9.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key +openedx-learning==0.11.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/xmodule/course_block.py b/xmodule/course_block.py index 52422ffa3427..46ad7476f6d1 100644 --- a/xmodule/course_block.py +++ b/xmodule/course_block.py @@ -228,7 +228,7 @@ class ProctoringProvider(String): and default that pulls from edx platform settings. """ - def from_json(self, value): + def from_json(self, value, validate_providers=False): """ Return ProctoringProvider as full featured Python type. Perform validation on the provider and include any inherited values from the platform default. @@ -237,7 +237,8 @@ def from_json(self, value): if settings.FEATURES.get('ENABLE_PROCTORED_EXAMS'): # Only validate the provider value if ProctoredExams are enabled on the environment # Otherwise, the passed in provider does not matter. We should always return default - self._validate_proctoring_provider(value) + if validate_providers: + self._validate_proctoring_provider(value) value = self._get_proctoring_value(value) return value else: diff --git a/xmodule/tests/test_course_block.py b/xmodule/tests/test_course_block.py index 39c6c39e8783..f2956cca0d7e 100644 --- a/xmodule/tests/test_course_block.py +++ b/xmodule/tests/test_course_block.py @@ -542,14 +542,27 @@ def test_from_json_with_invalid_provider(self, proctored_exams_setting_enabled): with override_settings(FEATURES=FEATURES_WITH_PROCTORED_EXAMS): if proctored_exams_setting_enabled: with pytest.raises(InvalidProctoringProvider) as context_manager: - self.proctoring_provider.from_json(provider) + self.proctoring_provider.from_json(provider, validate_providers=True) expected_error = f'The selected proctoring provider, {provider}, is not a valid provider. ' \ f'Please select from one of {allowed_proctoring_providers}.' assert str(context_manager.value) == expected_error else: - provider_value = self.proctoring_provider.from_json(provider) + provider_value = self.proctoring_provider.from_json(provider, validate_providers=True) assert provider_value == self.proctoring_provider.default + def test_from_json_validate_providers(self): + """ + Test that an invalid provider is ignored if validate providers is set to false + """ + provider = 'invalid-provider' + + FEATURES_WITH_PROCTORED_EXAMS = settings.FEATURES.copy() + FEATURES_WITH_PROCTORED_EXAMS['ENABLE_PROCTORED_EXAMS'] = True + + with override_settings(FEATURES=FEATURES_WITH_PROCTORED_EXAMS): + provider_value = self.proctoring_provider.from_json(provider, validate_providers=False) + assert provider_value == provider + def test_from_json_adds_platform_default_for_missing_provider(self): """ Test that a value with no provider will inherit the default provider