From bcc1ddd375e851e28a55e5125834aa767e065223 Mon Sep 17 00:00:00 2001 From: Zachary Hancock Date: Wed, 24 Jun 2020 16:02:17 -0400 Subject: [PATCH] add proctoring escalation contact setting (#24243) --- .../tests/test_course_settings.py | 121 ++++++++++++++++++ .../models/settings/course_metadata.py | 107 +++++++++++----- common/lib/xmodule/xmodule/course_module.py | 24 ++++ 3 files changed, 218 insertions(+), 34 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index fe7d5033b9..b5b9bbe718 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -881,6 +881,32 @@ class CourseMetadataEditingTest(CourseTestCase): test_model = CourseMetadata.fetch(self.fullcourse) self.assertNotIn('giturl', test_model) + @override_settings( + PROCTORING_BACKENDS={ + 'DEFAULT': 'test_proctoring_provider', + 'proctortrack': {} + }, + ) + def test_fetch_proctoring_escalation_email_present(self): + """ + If 'proctortrack' is an available provider, show the escalation email setting + """ + test_model = CourseMetadata.fetch(self.fullcourse) + self.assertIn('proctoring_escalation_email', test_model) + + @override_settings( + PROCTORING_BACKENDS={ + 'DEFAULT': 'test_proctoring_provider', + 'alternate_provider': {} + }, + ) + def test_fetch_proctoring_escalation_email_not_present(self): + """ + If 'proctortrack' is not an available provider, don't show the escalation email setting + """ + test_model = CourseMetadata.fetch(self.fullcourse) + self.assertNotIn('proctoring_escalation_email', test_model) + @patch.dict(settings.FEATURES, {'ENABLE_EXPORT_GIT': False}) def test_validate_update_filtered_off(self): """ @@ -1354,6 +1380,101 @@ class CourseMetadataEditingTest(CourseTestCase): ) self.assertNotIn(field_name, test_model) + @ddt.data(True, False) + @override_settings( + PROCTORING_BACKENDS={ + 'DEFAULT': 'test_proctoring_provider', + 'test_proctoring_provider': {}, + 'proctortrack': {} + } + ) + @override_waffle_flag(ENABLE_PROCTORING_PROVIDER_OVERRIDES, True) + def test_validate_update_requires_escalation_email_for_proctortrack(self, include_blank_email): + json_data = { + "proctoring_provider": {"value": 'proctortrack'}, + } + if include_blank_email: + json_data["proctoring_escalation_email"] = {"value": ""} + + did_validate, errors, test_model = CourseMetadata.validate_and_update_from_json( + self.course, + json_data, + user=self.user + ) + self.assertFalse(did_validate) + self.assertEqual(len(errors), 1) + self.assertIsNone(test_model) + self.assertEqual( + errors[0].get('message'), + 'Provider \'proctortrack\' requires an exam escalation contact.' + ) + + @override_settings( + PROCTORING_BACKENDS={ + 'DEFAULT': 'test_proctoring_provider', + 'test_proctoring_provider': {}, + 'proctortrack': {} + } + ) + @override_waffle_flag(ENABLE_PROCTORING_PROVIDER_OVERRIDES, True) + def test_validate_update_does_not_require_escalation_email_by_default(self): + did_validate, errors, test_model = CourseMetadata.validate_and_update_from_json( + self.course, + { + "proctoring_provider": {"value": "test_proctoring_provider"}, + }, + user=self.user + ) + self.assertTrue(did_validate) + self.assertEqual(len(errors), 0) + self.assertIn('proctoring_provider', test_model) + + @override_settings( + PROCTORING_BACKENDS={ + 'DEFAULT': 'proctortrack', + 'proctortrack': {} + } + ) + @override_waffle_flag(ENABLE_PROCTORING_PROVIDER_OVERRIDES, True) + def test_validate_update_cannot_unset_escalation_email_when_proctortrack_is_provider(self): + course = CourseFactory.create() + CourseMetadata.update_from_dict({"proctoring_provider": 'proctortrack'}, course, self.user) + did_validate, errors, test_model = CourseMetadata.validate_and_update_from_json( + course, + { + "proctoring_escalation_email": {"value": ""}, + }, + user=self.user + ) + self.assertFalse(did_validate) + self.assertEqual(len(errors), 1) + self.assertIsNone(test_model) + self.assertEqual( + errors[0].get('message'), + 'Provider \'proctortrack\' requires an exam escalation contact.' + ) + + @override_settings( + PROCTORING_BACKENDS={ + 'DEFAULT': 'proctortrack', + 'proctortrack': {} + } + ) + @override_waffle_flag(ENABLE_PROCTORING_PROVIDER_OVERRIDES, True) + def test_validate_update_set_proctortrack_provider_with_valid_escalation_email(self): + did_validate, errors, test_model = CourseMetadata.validate_and_update_from_json( + self.course, + { + "proctoring_provider": {"value": "proctortrack"}, + "proctoring_escalation_email": {"value": "foo@bar.com"}, + }, + user=self.user + ) + self.assertTrue(did_validate) + self.assertEqual(len(errors), 0) + self.assertIn('proctoring_provider', test_model) + self.assertIn('proctoring_escalation_email', test_model) + def test_create_zendesk_tickets_present_for_edx_staff(self): """ Tests that create zendesk tickets field is not filtered out when the user is an edX staff member. diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index bd92002c4e..f9e05b9089 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -6,6 +6,7 @@ Django module for Course Metadata class -- manages advanced settings and related from datetime import datetime import six from crum import get_current_user +from django.core.exceptions import ValidationError from django.conf import settings from django.utils.translation import ugettext as _ import pytz @@ -143,6 +144,11 @@ class CourseMetadata(object): if not GlobalStaff().has_user(get_current_user()): exclude_list.append('create_zendesk_tickets') + # Do not show "Proctortrack Exam Escalation Contact" if Proctortrack is not + # an available proctoring backend. + if not settings.PROCTORING_BACKENDS or settings.PROCTORING_BACKENDS.get('proctortrack') is None: + exclude_list.append('proctoring_escalation_email') + return exclude_list @classmethod @@ -244,29 +250,14 @@ class CourseMetadata(object): val = model['value'] if hasattr(descriptor, key) and getattr(descriptor, key) != val: key_values[key] = descriptor.fields[key].from_json(val) - except (TypeError, ValueError) as err: + except (TypeError, ValueError, ValidationError) as err: did_validate = False errors.append({'message': text_type(err), 'model': model}) - # Disallow updates to the proctoring provider after course start - proctoring_provider_model = filtered_dict.get('proctoring_provider', {}) - - # If the user is not edX staff, the user has requested a change to the proctoring_provider - # Advanced Setting, and and it is after course start, prevent the user from changing the - # proctoring provider. - if ( - not user.is_staff and - cls._has_requested_proctoring_provider_changed( - descriptor.proctoring_provider, proctoring_provider_model.get('value') - ) and - datetime.now(pytz.UTC) > descriptor.start - ): + proctoring_errors = cls._validate_proctoring_settings(descriptor, filtered_dict, user) + if proctoring_errors: + errors = errors + proctoring_errors did_validate = False - message = ( - 'The proctoring provider cannot be modified after a course has started.' - ' Contact {support_email} for assistance' - ).format(support_email=settings.PARTNER_SUPPORT_EMAIL or 'support') - errors.append({'message': message, 'model': proctoring_provider_model}) # If did validate, go ahead and update the metadata if did_validate: @@ -274,21 +265,6 @@ class CourseMetadata(object): return did_validate, errors, updated_data - @staticmethod - def _has_requested_proctoring_provider_changed(current_provider, requested_provider): - """ - Return whether the requested proctoring provider is different than the current proctoring provider, indicating - that the user has requested a change to the proctoring_provider Advanced Setting. - - The requested_provider will be None if the proctoring_provider setting is not available (e.g. if the - ENABLE_PROCTORING_PROVIDER_OVERRIDES waffle flag is not enabled for the course). In this case, we consider - that there is no change in the requested proctoring provider. - """ - if requested_provider is None: - return False - else: - return current_provider != requested_provider - @classmethod def update_from_dict(cls, key_values, descriptor, user, save=True): """ @@ -301,3 +277,66 @@ class CourseMetadata(object): modulestore().update_item(descriptor, user.id) return cls.fetch(descriptor) + + @classmethod + def _validate_proctoring_settings(cls, descriptor, settings_dict, user): + """ + Verify proctoring settings + + Returns a list of error objects + """ + errors = [] + + # If the user is not edX staff, the user has requested a change to the proctoring_provider + # Advanced Setting, and it is after course start, prevent the user from changing the + # proctoring provider. + proctoring_provider_model = settings_dict.get('proctoring_provider', {}) + if ( + not user.is_staff and + cls._has_requested_proctoring_provider_changed( + descriptor.proctoring_provider, proctoring_provider_model.get('value') + ) and + datetime.now(pytz.UTC) > descriptor.start + ): + message = ( + 'The proctoring provider cannot be modified after a course has started.' + ' Contact {support_email} for assistance' + ).format(support_email=settings.PARTNER_SUPPORT_EMAIL or 'support') + errors.append({'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: + escalation_email = escalation_email_model.get('value') + else: + escalation_email = descriptor.proctoring_escalation_email + + missing_escalation_email_msg = 'Provider \'{provider}\' requires an exam escalation contact.' + if proctoring_provider_model and proctoring_provider_model.get('value') == 'proctortrack': + if not escalation_email: + message = missing_escalation_email_msg.format(provider=proctoring_provider_model.get('value')) + errors.append({'message': message, 'model': proctoring_provider_model}) + + if ( + escalation_email_model and not proctoring_provider_model and + descriptor.proctoring_provider == 'proctortrack' + ): + if not escalation_email: + message = missing_escalation_email_msg.format(provider=descriptor.proctoring_provider) + errors.append({'message': message, 'model': escalation_email_model}) + + return errors + + @staticmethod + def _has_requested_proctoring_provider_changed(current_provider, requested_provider): + """ + Return whether the requested proctoring provider is different than the current proctoring provider, indicating + that the user has requested a change to the proctoring_provider Advanced Setting. + The requested_provider will be None if the proctoring_provider setting is not available (e.g. if the + ENABLE_PROCTORING_PROVIDER_OVERRIDES waffle flag is not enabled for the course). In this case, we consider + that there is no change in the requested proctoring provider. + """ + if requested_provider is None: + return False + else: + return current_provider != requested_provider diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 8710255c28..1fd2c6342f 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -12,6 +12,7 @@ import dateutil.parser import requests import six from django.conf import settings +from django.core.validators import validate_email from lazy import lazy from lxml import etree from path import Path as path @@ -83,6 +84,18 @@ class StringOrDate(Date): return result +class EmailString(String): + """ + Parse String with email validation + """ + def from_json(self, value): + if value: + validate_email(value) + return value + else: + return None + + edx_xml_parser = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_comments=True, remove_blank_text=True) @@ -876,6 +889,17 @@ class CourseFields(object): scope=Scope.settings, ) + proctoring_escalation_email = EmailString( + display_name=_("Proctortrack Exam Escalation Contact"), + help=_( + "Required if 'proctortrack' is selected as your proctoring provider. " + "Enter an email address to be contacted by the support team whenever there are escalations " + "(e.g. appeals, delayed reviews, etc.)." + ), + default=None, + scope=Scope.settings + ) + allow_proctoring_opt_out = Boolean( display_name=_("Allow Opting Out of Proctored Exams"), help=_(