diff --git a/edx_exams/apps/api/serializers.py b/edx_exams/apps/api/serializers.py index e6714e55..2248567c 100644 --- a/edx_exams/apps/api/serializers.py +++ b/edx_exams/apps/api/serializers.py @@ -55,7 +55,7 @@ class CourseExamConfigurationSerializer(serializers.ModelSerializer): # The escalation_email is only required when the provider is not None. # We enforce this constraint in the validate function. - escalation_email = serializers.EmailField(required=False, default='') + # escalation_email = serializers.EmailField(required=False) class Meta: model = CourseExamConfiguration @@ -69,12 +69,10 @@ def validate_provider(self, value): if value is not None: try: ProctoringProvider.objects.get(name=value) - # This exception is handled by the Django Rest Framework, so we don't want to use raise from and risk + # This exception is handled by the Django Rest Framework, so we don't want to use "raise from e" and risk # getting the stack trace included in client facing errors. except ProctoringProvider.DoesNotExist: - raise serializers.ValidationError( # pylint: disable=raise-missing-from - 'Proctoring provider does not exist.' - ) + raise serializers.ValidationError('Proctoring provider does not exist.') from None return value return value @@ -83,11 +81,9 @@ def validate(self, attrs): """ Validate that escalalation_email is provided when the provider is provided. - NOTE: Currently, the LTI-based proctoring providers will require an escalation_email. Because the - escalation_email field was added to the model later, a default of the empty string was required. For - this reason, DRF generates an escalation_email serializer field that is optional. The current requirement is - that escalation_email is required unless the provider is None. Exceptions to this requirement can be added to - this function in the future. + NOTE: Currently, the LTI-based proctoring providers will require an escalation_email.DRF generates an + escalation_email serializer field that is optional. The current requirement is that escalation_email is required + unless the provider is None. Exceptions to this requirement can be added to this function in the future. """ if attrs.get('provider') and attrs.get('provider').get('name') and not attrs.get('escalation_email'): raise serializers.ValidationError('Escalation email is a required field when provider is provided.') diff --git a/edx_exams/apps/core/admin.py b/edx_exams/apps/core/admin.py index 59cebb2c..78a57fcb 100644 --- a/edx_exams/apps/core/admin.py +++ b/edx_exams/apps/core/admin.py @@ -71,7 +71,7 @@ def change_view(self, request, object_id, form_url='', extra_context=None): class CourseExamConfigurationAdmin(admin.ModelAdmin): """ Admin configuration for the Course Exam Configuration model """ list_display = ('course_id', 'provider', 'allow_opt_out', 'escalation_email') - readonly_fields = ('course_id', 'provider', 'escalation_email') + readonly_fields = ('course_id', 'provider') search_fields = ('course_id', 'provider__name', 'allow_opt_out', 'escalation_email') ordering = ('course_id',) diff --git a/edx_exams/apps/core/api.py b/edx_exams/apps/core/api.py index 97fd25b0..00b63089 100644 --- a/edx_exams/apps/core/api.py +++ b/edx_exams/apps/core/api.py @@ -461,5 +461,9 @@ def create_or_update_course_exam_configuration(course_id, provider_name, escalat provider = provider_name if provider_name is not None: provider = ProctoringProvider.objects.get(name=provider_name) + else: + # If the provider is set to None, then we must clear the escalation_email, + # even if a non-empty/non-null value is provided. + escalation_email = '' CourseExamConfiguration.create_or_update(course_id, provider, escalation_email) diff --git a/edx_exams/apps/core/migrations/0021_add_courseexamconfiguration_escalation_email.py b/edx_exams/apps/core/migrations/0021_add_courseexamconfiguration_escalation_email.py deleted file mode 100644 index c955c3e3..00000000 --- a/edx_exams/apps/core/migrations/0021_add_courseexamconfiguration_escalation_email.py +++ /dev/null @@ -1,36 +0,0 @@ -# Generated by Django 3.2.21 on 2023-12-06 17:09 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ('core', '0020_auto_20231010_1442'), - ] - - operations = [ - migrations.AlterModelOptions( - name='historicalexam', - options={'get_latest_by': 'history_date', 'ordering': ('-history_date', '-history_id'), 'verbose_name': 'historical exam'}, - ), - migrations.AlterModelOptions( - name='historicalexamattempt', - options={'get_latest_by': 'history_date', 'ordering': ('-history_date', '-history_id'), 'verbose_name': 'historical exam attempt'}, - ), - migrations.AddField( - model_name='courseexamconfiguration', - name='escalation_email', - field=models.EmailField(blank=True, max_length=254), - ), - migrations.AlterField( - model_name='exam', - name='exam_type', - field=models.CharField(choices=[('onboarding', 'onboarding'), ('practice', 'practice'), ('proctored', 'proctored'), ('timed', 'timed')], db_index=True, max_length=255), - ), - migrations.AlterField( - model_name='historicalexam', - name='exam_type', - field=models.CharField(choices=[('onboarding', 'onboarding'), ('practice', 'practice'), ('proctored', 'proctored'), ('timed', 'timed')], db_index=True, max_length=255), - ), - ] diff --git a/edx_exams/apps/core/migrations/0022_courseexamconfiguration_escalation_email.py b/edx_exams/apps/core/migrations/0022_courseexamconfiguration_escalation_email.py new file mode 100644 index 00000000..ac731612 --- /dev/null +++ b/edx_exams/apps/core/migrations/0022_courseexamconfiguration_escalation_email.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.23 on 2023-12-07 15:03 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('core', '0021_alter_exam_exam_types'), + ] + + operations = [ + migrations.AddField( + model_name='courseexamconfiguration', + name='escalation_email', + field=models.EmailField(blank=True, max_length=254), + ), + ] diff --git a/edx_exams/apps/core/models.py b/edx_exams/apps/core/models.py index cfe792f2..0ec4d9eb 100644 --- a/edx_exams/apps/core/models.py +++ b/edx_exams/apps/core/models.py @@ -353,6 +353,12 @@ def create_or_update(cls, course_id, provider, escalation_email): existing exams. """ provider_name = provider.name if provider else None + + # If the provider is set to None, then we must clear the escalation_email, + # even if a non-empty/non-null value is provided. + if provider is None: + escalation_email = '' + existing_config = CourseExamConfiguration.get_configuration_for_course(course_id) if existing_config: existing_provider = existing_config.provider @@ -386,11 +392,7 @@ def update_course_config(cls, existing_config, new_provider, escalation_email): * escalation_email: a string representing an email address; the escalation_email to be set """ existing_config.provider = new_provider - - if new_provider is None: - existing_config.escalation_email = '' - else: - existing_config.escalation_email = escalation_email + existing_config.escalation_email = escalation_email existing_config.save() diff --git a/edx_exams/apps/core/tests/test_api.py b/edx_exams/apps/core/tests/test_api.py index 35020d75..e2d575a7 100644 --- a/edx_exams/apps/core/tests/test_api.py +++ b/edx_exams/apps/core/tests/test_api.py @@ -940,7 +940,7 @@ def setUp(self): provider=self.test_provider, ) - def create_or_update_course_exam_configuration_none_provider(self): + def test_create_or_update_course_exam_configuration_none_provider(self): """ Test that the create_or_update_course_exam_configuration function correctly sets the provider to None. """ @@ -958,7 +958,7 @@ def create_or_update_course_exam_configuration_none_provider(self): self.assertEqual(config.provider, expected_provider) self.assertEqual(config.escalation_email, '') - def create_or_update_course_exam_configuration_none_provider_escalation_email(self): + def test_create_or_update_course_exam_configuration_none_provider_escalation_email(self): """ Test that the create_or_update_course_exam_configuration function correctly sets the provider to None and disregards any provided escalation_email, because it must be set to the empty string. @@ -977,7 +977,7 @@ def create_or_update_course_exam_configuration_none_provider_escalation_email(se self.assertEqual(config.provider, expected_provider) self.assertEqual(config.escalation_email, '') - def create_or_update_course_exam_configuration_provider(self): + def test_create_or_update_course_exam_configuration_provider(self): """ Test that the create_or_update_course_exam_configuration function correctly sets the provider to None. """ @@ -988,7 +988,7 @@ def create_or_update_course_exam_configuration_provider(self): provider=self.test_provider, ) - create_or_update_course_exam_configuration(self.exam.course_id, expected_provider, config.escalation_email) + create_or_update_course_exam_configuration(self.exam.course_id, expected_provider.name, config.escalation_email) config.refresh_from_db()