feat: change empty string to null and refactor serializers

edx_exams/apps/api/serializers.py

@@ -47,15 +47,18 @@ class Meta:
         fields = ['name', 'verbose_name', 'lti_configuration_id']
 
 
-class CourseExamConfigurationSerializer(serializers.ModelSerializer):
+class CourseExamConfigurationWriteSerializer(serializers.ModelSerializer):
     """
-    Serializer for the CourseExamConfiguration model
-    """
-    provider = serializers.CharField(source='provider.name', allow_null=True)
+    Serializer for writing to the CourseExamConfiguration model.
 
-    # 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)
+    We have separate read and write serializers because the read serializer uses the name field of the provider instance
+    to populate the provider serializer field by using the source attribute. The write serializer, on the other hand,
+    accepts the name as the value for the provider field. If we use the same serializer, deserialized data gets
+    represented as {'provider': {'name': <provider>}}, which makes reading the data less clear.
+    """
+    provider = serializers.CharField(allow_null=True)
+    # The escalation_email is a nullable field, but we require that clients send us an escalation_email.
+    escalation_email = serializers.EmailField(allow_null=True, allow_blank=False, required=True)
 
     class Meta:
         model = CourseExamConfiguration
@@ -79,17 +82,38 @@ def validate_provider(self, value):
 
     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.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.
+        Validate that escalalation_email is provided when the provider is provided and that it is None if the provider
+        is None.
         """
-        if attrs.get('provider') and attrs.get('provider').get('name') and not attrs.get('escalation_email'):
+        provider_name = attrs.get('provider')
+        escalation_email = attrs.get('escalation_email')
+
+        if provider_name and not escalation_email:
             raise serializers.ValidationError('Escalation email is a required field when provider is provided.')
+        if provider_name is None and escalation_email is not None:
+            raise serializers.ValidationError('Escalation email must be None when provider is None.')
+
         return attrs
 
 
+class CourseExamConfigurationReadSerializer(serializers.ModelSerializer):
+    """
+    Serializer for reading from the CourseExamConfiguration model
+
+    We have separate read and write serializers because the read serializer uses the name field of the provider instance
+    to populate the provider serializer field by using the source attribute. The write serializer, on the other hand,
+    accepts the name as the value for the provider field. If we use the same serializer, deserialized data gets
+    represented as {'provider': {'name': <provider>}}, which makes reading the data less clear.
+    """
+    provider = serializers.CharField(source='provider.name', allow_null=True)
+    # The escalation_email is a nullable field, but we require that clients send us an escalation_email.
+    escalation_email = serializers.EmailField(allow_null=True, allow_blank=False, required=True)
+
+    class Meta:
+        model = CourseExamConfiguration
+        fields = ['provider', 'escalation_email']
+
+
 class ExamSerializer(serializers.ModelSerializer):
     """
     Serializer for the Exam Model

edx_exams/apps/api/v1/tests/test_views.py

@@ -358,6 +358,7 @@ def test_course_staff_write_access(self):
         CourseStaffRole.objects.create(user=course_staff_user, course_id=self.course_id)
         response = self.patch_api(course_staff_user, {
             'provider': None,
+            'escalation_email': None,
         })
         self.assertEqual(204, response.status_code)
 
@@ -370,11 +371,30 @@ def test_patch_invalid_data_no_provider(self):
         response = self.patch_api(self.user, data)
         self.assertEqual(400, response.status_code)
 
-    def test_patch_invalid_data_no_escalation_email(self):
+    @ddt.data('test_provider', None)
+    def test_patch_invalid_data_no_escalation_email(self, provider_name):
         """
         Assert that endpoint returns 400 if provider is provided but escalation_email is missing
         """
-        data = {'provider': 'test_provider'}
+        data = {'provider': provider_name}
+
+        response = self.patch_api(self.user, data)
+        self.assertEqual(400, response.status_code)
+
+    def test_patch_invalid_data_escalation_email(self):
+        """
+        Assert that endpoint returns 400 if provider is None but escalation_email is not
+        """
+        data = {'provider': None, 'escalation_email': '[email protected]'}
+
+        response = self.patch_api(self.user, data)
+        self.assertEqual(400, response.status_code)
+
+    def test_patch_invalid_data_invalid_escalation_email(self):
+        """
+        Assert that endpoint returns 400 if the escalation email is not a valid email.
+        """
+        data = {'provider': 'test_provider', 'escalation_email': 'test'}
 
         response = self.patch_api(self.user, data)
         self.assertEqual(400, response.status_code)
@@ -529,14 +549,15 @@ def test_patch_null_provider(self):
         """
         Assert provider can be explicitly set to null
         """
-        data = {'provider': None}
+        data = {'provider': None, 'escalation_email': None}
 
         response = self.patch_api(self.user, data)
         self.assertEqual(204, response.status_code)
         self.assertEqual(len(CourseExamConfiguration.objects.all()), 1)
 
         config = CourseExamConfiguration.get_configuration_for_course(self.course_id)
         self.assertEqual(config.provider, None)
+        self.assertEqual(config.escalation_email, None)
 
     def test_get_config(self):
         """

edx_exams/apps/api/v1/views.py

@@ -17,7 +17,8 @@
 
 from edx_exams.apps.api.permissions import CourseStaffOrReadOnlyPermissions, CourseStaffUserPermissions
 from edx_exams.apps.api.serializers import (
-    CourseExamConfigurationSerializer,
+    CourseExamConfigurationReadSerializer,
+    CourseExamConfigurationWriteSerializer,
     ExamSerializer,
     InstructorViewAttemptSerializer,
     ProctoringProviderSerializer,
@@ -260,22 +261,21 @@ def get(self, request, course_id):
             # If configuration is set to None, then the provider is serialized to the empty string instead of None.
             configuration = {}
 
-        serializer = CourseExamConfigurationSerializer(configuration)
+        serializer = CourseExamConfigurationReadSerializer(configuration)
         return Response(serializer.data)
 
     def patch(self, request, course_id):
         """
         Create/update course exam configuration.
         """
-        serializer = CourseExamConfigurationSerializer(data=request.data)
+        serializer = CourseExamConfigurationWriteSerializer(data=request.data)
 
         if serializer.is_valid():
             validated_data = serializer.validated_data
             create_or_update_course_exam_configuration(
                 course_id,
-                validated_data['provider']['name'],
-                # Escalation email may be optional; use None if it's not provided.
-                validated_data.get('escalation_email')
+                validated_data['provider'],
+                validated_data['escalation_email'],
             )
             return Response({}, status=status.HTTP_204_NO_CONTENT)
         else:

edx_exams/apps/core/api.py

@@ -458,12 +458,12 @@ def create_or_update_course_exam_configuration(course_id, provider_name, escalat
         * provider_name: the name of the proctoring provider
         * escalation_email: the escalation email
     """
-    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 = ''
+        # even if a non-null value is provided.
+        escalation_email = None
+        provider = None
 
     CourseExamConfiguration.create_or_update(course_id, provider, escalation_email)

edx_exams/apps/core/migrations/0022_courseexamconfiguration_escalation_email.py

@@ -1,4 +1,4 @@
-# Generated by Django 3.2.23 on 2023-12-07 15:03
+# Generated by Django 3.2.23 on 2023-12-07 20:12
 
 from django.db import migrations, models
 
@@ -13,6 +13,6 @@ class Migration(migrations.Migration):
         migrations.AddField(
             model_name='courseexamconfiguration',
             name='escalation_email',
-            field=models.EmailField(blank=True, max_length=254),
+            field=models.EmailField(max_length=254, null=True),
         ),
     ]

edx_exams/apps/core/models.py

@@ -325,7 +325,7 @@ class CourseExamConfiguration(TimeStampedModel):
 
     allow_opt_out = models.BooleanField(default=False)
 
-    escalation_email = models.EmailField(blank=True)
+    escalation_email = models.EmailField(null=True, blank=False)
 
     class Meta:
         """ Meta class for this Django model """
@@ -354,10 +354,9 @@ def create_or_update(cls, course_id, provider, escalation_email):
         """
         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 the provider is set to None, then we must clear the escalation_email, regardless of the value provided.
         if provider is None:
-            escalation_email = ''
+            escalation_email = None
 
         existing_config = CourseExamConfiguration.get_configuration_for_course(course_id)
         if existing_config:

edx_exams/apps/core/tests/test_api.py

@@ -926,6 +926,7 @@ def test_get_escalation_email_no_configuration(self):
         self.assertEqual(get_escalation_email(self.exam.id), None)
 
 
+@ddt.ddt
 class TestCreateOrUpdateCourseExamConfiguration(ExamsAPITestCase):
     """
     Tests for the create_or_update_course_exam_configuration API function.
@@ -940,28 +941,11 @@ def setUp(self):
             provider=self.test_provider,
         )
 
-    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.
-        """
-        expected_provider = None
-
-        config = CourseExamConfigurationFactory(
-            course_id=self.exam.course_id,
-            provider=self.test_provider,
-        )
-
-        create_or_update_course_exam_configuration(self.exam.course_id, expected_provider, '')
-
-        config.refresh_from_db()
-
-        self.assertEqual(config.provider, expected_provider)
-        self.assertEqual(config.escalation_email, '')
-
-    def test_create_or_update_course_exam_configuration_none_provider_escalation_email(self):
+    @ddt.data('[email protected]', '')
+    def test_create_or_update_course_exam_configuration_none_provider(self, escalation_email):
         """
-        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.
+        Test that the create_or_update_course_exam_configuration function correctly sets the provider to None and that
+        the function disregards any provided escalation_email, because it must be set to None.
         """
         expected_provider = None
 
@@ -970,12 +954,12 @@ def test_create_or_update_course_exam_configuration_none_provider_escalation_ema
             provider=self.test_provider,
         )
 
-        create_or_update_course_exam_configuration(self.exam.course_id, expected_provider, '[email protected]')
+        create_or_update_course_exam_configuration(self.exam.course_id, expected_provider, escalation_email)
 
         config.refresh_from_db()
 
         self.assertEqual(config.provider, expected_provider)
-        self.assertEqual(config.escalation_email, '')
+        self.assertEqual(config.escalation_email, None)
 
     def test_create_or_update_course_exam_configuration_provider(self):
         """

edx_exams/apps/core/tests/test_models.py

@@ -90,7 +90,7 @@ def test_create_or_update_no_provider(self):
         self.assertEqual(num_configs, 1)
 
         self.assertEqual(self.config.provider, None)
-        self.assertEqual(self.config.escalation_email, '')
+        self.assertEqual(self.config.escalation_email, None)
 
     def test_create_or_update_provider_change_and_sync(self):
         other_provider = ProctoringProviderFactory()