From 33f6d77f318995fc6ebab13d018f022ea161fd12 Mon Sep 17 00:00:00 2001 From: Zachary Hancock Date: Thu, 17 Sep 2020 14:15:34 -0400 Subject: [PATCH] Alert banner for proctoring settings error (#24960) --- .../tests/test_course_settings.py | 24 ----- cms/djangoapps/contentstore/views/course.py | 11 ++- .../views/tests/test_exam_settings_view.py | 95 +++++++++++++++++++ .../models/settings/course_metadata.py | 56 ++++++----- cms/templates/course_outline.html | 42 ++++++++ cms/templates/settings_advanced.html | 40 ++++++++ 6 files changed, 214 insertions(+), 54 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index 23c1e45338..ca60d3ef6a 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -1423,30 +1423,6 @@ class CourseMetadataEditingTest(CourseTestCase): self.assertIn('proctoring_provider', test_model) self.assertIn('proctoring_escalation_email', test_model) - @override_settings( - PROCTORING_BACKENDS={ - 'DEFAULT': 'test_proctoring_provider', - 'proctortrack': {} - } - ) - def test_validate_update_escalation_email_not_requirement_disabled(self): - """ - Tests the escalation email is not required if 'ENABLED_EXAM_SETTINGS_HTML_VIEW' - setting is not set to True - """ - json_data = { - "proctoring_provider": {"value": 'proctortrack'}, - } - did_validate, errors, test_model = CourseMetadata.validate_and_update_from_json( - self.course, - json_data, - 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/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 2b1fe9fe8b..2c7be4f27e 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -663,6 +663,10 @@ def course_index(request, course_key): course_authoring_microfrontend_url = get_proctored_exam_settings_url(course_module) + # gather any errors in the currently stored proctoring settings. + advanced_dict = CourseMetadata.fetch(course_module) + proctoring_errors = CourseMetadata.validate_proctoring_settings(course_module, advanced_dict, request.user) + return render_to_response('course_outline.html', { 'language_code': request.LANGUAGE_CODE, 'context_course': course_module, @@ -684,6 +688,8 @@ def course_index(request, course_key): ) if current_action else None, 'frontend_app_publisher_url': frontend_app_publisher_url, 'course_authoring_microfrontend_url': course_authoring_microfrontend_url, + 'advance_settings_url': reverse_course_url('advanced_settings_handler', course_module.id), + 'proctoring_errors': proctoring_errors, }) @@ -1341,13 +1347,16 @@ def advanced_settings_handler(request, course_key_string): course_authoring_microfrontend_url = get_proctored_exam_settings_url(course_module) + # gather any errors in the currently stored proctoring settings. + proctoring_errors = CourseMetadata.validate_proctoring_settings(course_module, advanced_dict, request.user) + return render_to_response('settings_advanced.html', { 'context_course': course_module, 'advanced_dict': advanced_dict, 'advanced_settings_url': reverse_course_url('advanced_settings_handler', course_key), 'publisher_enabled': publisher_enabled, 'course_authoring_microfrontend_url': course_authoring_microfrontend_url, - + 'proctoring_errors': proctoring_errors, }) elif 'application/json' in request.META.get('HTTP_ACCEPT', ''): if request.method == 'GET': 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 adca08d9ab..25efd3bde0 100644 --- a/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py +++ b/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py @@ -5,6 +5,7 @@ Exam Settings View Tests """ import ddt +import lxml from django.conf import settings from django.test.utils import override_settings @@ -35,6 +36,15 @@ class TestExamSettingsView(CourseTestCase, UrlResetMixin): super(TestExamSettingsView, self).setUp() self.reset_urls() + @staticmethod + def _get_exam_settings_alert_text(raw_html_content): + """ Get text content of alert banner """ + parsed_html = lxml.html.fromstring(raw_html_content) + alert_nodes = parsed_html.find_class('exam-settings-alert') + assert len(alert_nodes) == 1 + alert_node = alert_nodes[0] + return alert_node.text_content() + @override_settings(FEATURES=FEATURES_WITH_EXAM_SETTINGS_DISABLED) @ddt.data( "certificates_list_handler", @@ -70,3 +80,88 @@ class TestExamSettingsView(CourseTestCase, UrlResetMixin): resp = self.client.get(outline_url, HTTP_ACCEPT='text/html') self.assertEqual(resp.status_code, 200) self.assertContains(resp, 'Proctored Exam Settings') + + @override_settings( + PROCTORING_BACKENDS={ + 'DEFAULT': 'test_proctoring_provider', + 'proctortrack': {} + }, + FEATURES=FEATURES_WITH_EXAM_SETTINGS_ENABLED, + ) + @ddt.data( + "advanced_settings_handler", + "course_handler", + ) + def test_exam_settings_alert_with_exam_settings_enabled(self, page_handler): + """ + An alert should appear if current exam settings are invalid. + The exam settings alert should direct users to the exam settings page. + """ + # create an error by setting proctortrack as the provider and not setting + # the (required) escalation contact + self.course.proctoring_provider = 'proctortrack' + 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 ( + 'To update these settings go to the Proctored Exam Settings page.' + in alert_text + ) + + @override_settings( + PROCTORING_BACKENDS={ + 'DEFAULT': 'test_proctoring_provider', + 'proctortrack': {} + }, + FEATURES=FEATURES_WITH_EXAM_SETTINGS_DISABLED, + ) + @ddt.data( + "advanced_settings_handler", + "course_handler", + ) + def test_exam_settings_alert_with_exam_settings_disabled(self, page_handler): + """ + An alert should appear if current exam settings are invalid. + The exam settings alert should direct users to the advanced settings page + if the exam settings feature is not available. + """ + # create an error by setting proctortrack as the provider and not setting + # the (required) escalation contact + self.course.proctoring_provider = 'proctortrack' + 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 + ) + self.maxDiff = None + if page_handler == 'advanced_settings_handler': + assert ( + 'You will be unable to make changes until the following settings are updated on the page below.' + in alert_text + ) + else: + assert 'To update these settings go to the Advanced Settings page.' in alert_text + + @ddt.data( + "advanced_settings_handler", + "course_handler", + ) + def test_exam_settings_alert_not_shown(self, page_handler): + """ + Alert should not be visible if no proctored exam setting error exists + """ + url = reverse_course_url(page_handler, self.course.id) + resp = self.client.get(url, HTTP_ACCEPT='text/html') + parsed_html = lxml.html.fromstring(resp.content) + alert_nodes = parsed_html.find_class('exam-settings-alert') + assert len(alert_nodes) == 0 diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index b6a9ca5940..c38dd63d96 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -248,7 +248,7 @@ class CourseMetadata(object): did_validate = False errors.append({'key': key, 'message': text_type(err), 'model': model}) - proctoring_errors = cls._validate_proctoring_settings(descriptor, filtered_dict, user) + proctoring_errors = cls.validate_proctoring_settings(descriptor, filtered_dict, user) if proctoring_errors: errors = errors + proctoring_errors did_validate = False @@ -273,7 +273,7 @@ class CourseMetadata(object): return cls.fetch(descriptor) @classmethod - def _validate_proctoring_settings(cls, descriptor, settings_dict, user): + def validate_proctoring_settings(cls, descriptor, settings_dict, user): """ Verify proctoring settings @@ -299,35 +299,33 @@ class CourseMetadata(object): errors.append({'key': 'proctoring_provider', 'message': message, 'model': proctoring_provider_model}) # Require a valid escalation email if Proctortrack is chosen as the proctoring provider - # This requirement will be disabled until release of the new exam settings view - if settings.FEATURES.get('ENABLE_EXAM_SETTINGS_HTML_VIEW'): - 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 + 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({ - 'key': 'proctoring_provider', - 'message': message, - 'model': proctoring_provider_model - }) + 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({ + 'key': 'proctoring_provider', + '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({ - 'key': 'proctoring_escalation_email', - 'message': message, - 'model': escalation_email_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({ + 'key': 'proctoring_escalation_email', + 'message': message, + 'model': escalation_email_model + }) return errors diff --git a/cms/templates/course_outline.html b/cms/templates/course_outline.html index 1cf4a7aee2..2de2563610 100644 --- a/cms/templates/course_outline.html +++ b/cms/templates/course_outline.html @@ -4,6 +4,7 @@ <%! import logging import six +from six.moves.urllib.parse import quote from cms.djangoapps.contentstore.config.waffle_utils import should_show_checklists_quality from util.date_utils import get_default_time_display @@ -114,6 +115,47 @@ from django.urls import reverse %endif + + %if proctoring_errors: +
+
+ + +
+

${_("This course has proctored exam settings that are incomplete or invalid.")}

+

+ % if course_authoring_microfrontend_url: + <% url_encoded_course_id = quote(six.text_type(context_course.id).encode('utf-8'), safe='') %> + ${Text(_("To update these settings go to the {link_start}Proctored Exam Settings page{link_end}.")).format( + link_start=HTML('').format( + course_authoring_microfrontend_url=course_authoring_microfrontend_url, + url_encoded_course_id=url_encoded_course_id, + ), + link_end=HTML("") + )} + % else: + ${Text(_("To update these settings go to the {link_start}Advanced Settings page{link_end}.")).format( + link_start=HTML('').format(advance_settings_url=advance_settings_url), + link_end=HTML("") + )} + % endif +

+
+ +
+
+
+
+ %endif diff --git a/cms/templates/settings_advanced.html b/cms/templates/settings_advanced.html index 090776f86d..091b243e75 100644 --- a/cms/templates/settings_advanced.html +++ b/cms/templates/settings_advanced.html @@ -33,6 +33,46 @@ }); +<%block name="page_alert"> + %if proctoring_errors: +
+
+ + +
+

${_("This course has proctored exam settings that are incomplete or invalid.")}

+

+ % if course_authoring_microfrontend_url: + <% url_encoded_course_id = quote(six.text_type(context_course.id).encode('utf-8'), safe='') %> + ${Text(_("You will be unable to make changes until the errors are resolved. To update these settings go to the {link_start}Proctored Exam Settings page{link_end}.")).format( + link_start=HTML('').format( + course_authoring_microfrontend_url=course_authoring_microfrontend_url, + url_encoded_course_id=url_encoded_course_id, + ), + link_end=HTML("") + )} + % else: + ${_("You will be unable to make changes until the following settings are updated on the page below.")} + % endif +

+
+ +
+
+
+
+ %endif + + <%block name="content">