feat: address review feedback

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.')

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',)
 

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)

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),
+        ),
+    ]

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()
 

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()