feat: add escalation_email to course exam configurations

edx_exams/apps/api/serializers.py

@@ -7,7 +7,14 @@
 from edx_exams.apps.api.constants import ASSESSMENT_CONTROL_CODES
 from edx_exams.apps.core.api import get_exam_attempt_time_remaining, get_exam_url_path
 from edx_exams.apps.core.exam_types import EXAM_TYPES
-from edx_exams.apps.core.models import AssessmentControlResult, Exam, ExamAttempt, ProctoringProvider, User
+from edx_exams.apps.core.models import (
+    AssessmentControlResult,
+    CourseExamConfiguration,
+    Exam,
+    ExamAttempt,
+    ProctoringProvider,
+    User
+)
 
 
 class UserSerializer(serializers.ModelSerializer):
@@ -40,6 +47,67 @@ class Meta:
         fields = ['name', 'verbose_name', 'lti_configuration_id']
 
 
+class CourseExamConfigurationWriteSerializer(serializers.ModelSerializer):
+    """
+    Serializer for writing to 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(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']
+
+    def validate_provider(self, value):
+        """
+        Validate that the provider value corresponds to an existing ProctoringProvider. If the value is None,
+        the value is valid.
+        """
+        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 e" and risk
+            # getting the stack trace included in client facing errors.
+            except ProctoringProvider.DoesNotExist:
+                raise serializers.ValidationError('Proctoring provider does not exist.') from None
+            return value
+
+        return value
+
+    def validate(self, attrs):
+        """
+        Validate that escalalation_email is None if the provider is None.
+        """
+        if attrs.get('provider') is None and attrs.get('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,23 +358,46 @@ 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)
 
-    def test_patch_invalid_data(self):
+    @ddt.data(
+        {},
+        {'escalation_email': '[email protected]'},
+        {'provider': 'test-provider'}
+    )
+    def test_patch_invalid_data_missing_data(self, data):
+        """
+        Assert that endpoint returns 400 if any required parameter is missing
+        """
+        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 missing
+        Assert that endpoint returns 400 if provider is None but escalation_email is not
         """
-        data = {}
+        data = {'provider': None, 'escalation_email': '[email protected]'}
 
         response = self.patch_api(self.user, data)
         self.assertEqual(400, response.status_code)
 
-    def test_patch_invalid_provider(self):
+    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)
+
+    @ddt.data('nonexistent_provider', '')
+    def test_patch_invalid_provider(self, provider_name):
         """
         Assert endpoint returns 400 if provider is invalid
         """
-        data = {'provider': 'nonexistent_provider'}
+        data = {'provider': provider_name}
 
         response = self.patch_api(self.user, data)
         self.assertEqual(400, response.status_code)
@@ -392,14 +415,17 @@ def test_patch_config_update(self):
             verbose_name='testing_provider_2',
             lti_configuration_id='223456789'
         )
-        data = {'provider': provider.name}
+        escalation_email = '[email protected]'
+
+        data = {'provider': provider.name, 'escalation_email': escalation_email}
 
         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, provider)
+        self.assertEqual(config.escalation_email, escalation_email)
 
     def test_patch_config_update_exams(self):
         """
@@ -441,12 +467,15 @@ def test_patch_config_update_exams(self):
         exams = Exam.objects.filter(course_id=self.course_id, is_active=True)
         self.assertEqual(2, len(exams))
 
-        data = {'provider': provider.name}
+        escalation_email = '[email protected]'
+
+        data = {'provider': provider.name, 'escalation_email': escalation_email}
         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, provider)
+        self.assertEqual(config.escalation_email, escalation_email)
 
         exams = Exam.objects.filter(course_id=self.course_id, is_active=True)
         self.assertEqual(2, len(exams))
@@ -459,12 +488,13 @@ def test_patch_config_update_exams(self):
             self.assertEqual(self.test_provider, exam.provider)
 
         # updating to the same provider is a do nothing, no new exams
-        data = {'provider': provider.name}
+        data = {'provider': provider.name, 'escalation_email': escalation_email}
         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, provider)
+        self.assertEqual(config.escalation_email, escalation_email)
 
         exams = Exam.objects.filter(course_id=self.course_id, is_active=True)
         self.assertEqual(2, len(exams))
@@ -477,12 +507,13 @@ def test_patch_config_update_exams(self):
             self.assertEqual(self.test_provider, exam.provider)
 
         # updating back to the original provider creates two new active exams, now 4 inactive
-        data = {'provider': self.test_provider.name}
+        data = {'provider': self.test_provider.name, 'escalation_email': '[email protected]'}
         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, self.test_provider)
+        self.assertEqual(config.escalation_email, escalation_email)
 
         exams = Exam.objects.filter(course_id=self.course_id, is_active=True)
         self.assertEqual(2, len(exams))
@@ -496,27 +527,30 @@ def test_patch_config_create(self):
         """
         Test that config is created
         """
-        data = {'provider': 'test_provider'}
+        escalation_email = '[email protected]'
+        data = {'provider': 'test_provider', 'escalation_email': escalation_email}
 
         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, self.test_provider)
+        self.assertEqual(config.escalation_email, escalation_email)
 
     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,6 +17,8 @@
 
 from edx_exams.apps.api.permissions import CourseStaffOrReadOnlyPermissions, CourseStaffUserPermissions
 from edx_exams.apps.api.serializers import (
+    CourseExamConfigurationReadSerializer,
+    CourseExamConfigurationWriteSerializer,
     ExamSerializer,
     InstructorViewAttemptSerializer,
     ProctoringProviderSerializer,
@@ -26,6 +28,7 @@
 from edx_exams.apps.core.api import (
     check_if_exam_timed_out,
     create_exam_attempt,
+    create_or_update_course_exam_configuration,
     get_active_attempt_for_user,
     get_attempt_by_id,
     get_course_exams,
@@ -227,15 +230,19 @@ class CourseExamConfigurationsView(ExamsAPIView):
     **Returns**
     {
         'provider': 'test_provider',
+        'escalation_email': '[email protected]',
     }
 
     HTTP PATCH
     Creates or updates a CourseExamConfiguration.
     Expected PATCH data: {
         'provider': 'test_provider',
+        'escalation_email': '[email protected]',
     }
     **PATCH data Parameters**
-        * name: This is the name of the proctoring provider.
+        * provider: This is the name of the selected proctoring provider for the course.
+        * escalation_email: This is the email to which learners should send emails to escalate problems for the course.
+
     **Exceptions**
         * HTTP_400_BAD_REQUEST
     """
@@ -246,46 +253,32 @@ class CourseExamConfigurationsView(ExamsAPIView):
     def get(self, request, course_id):
         """
         Get exam configuration for a course
-
-        TODO: This view should use a serializer to ensure the read/write bodies are the same
-        once more fields are added.
         """
         try:
-            provider = CourseExamConfiguration.objects.get(course_id=course_id).provider
-        except ObjectDoesNotExist:
-            provider = None
+            configuration = CourseExamConfiguration.objects.get(course_id=course_id)
+        except CourseExamConfiguration.DoesNotExist:
+            # If configuration is set to None, then the provider is serialized to the empty string instead of None.
+            configuration = {}
 
-        return Response({
-            'provider': provider.name if provider else None
-        })
+        serializer = CourseExamConfigurationReadSerializer(configuration)
+        return Response(serializer.data)
 
     def patch(self, request, course_id):
         """
         Create/update course exam configuration.
         """
-        error = None
+        serializer = CourseExamConfigurationWriteSerializer(data=request.data)
 
-        # check that proctoring provider is in request
-        if 'provider' not in request.data:
-            error = {'detail': 'No proctoring provider name in request.'}
-        elif request.data.get('provider') is None:
-            provider = None
-        else:
-            try:
-                provider = ProctoringProvider.objects.get(name=request.data['provider'])
-            # return 400 if proctoring provider does not exist
-            except ObjectDoesNotExist:
-                error = {'detail': 'Proctoring provider does not exist.'}
-
-        if not error:
-            CourseExamConfiguration.create_or_update(provider, course_id)
-            response_status = status.HTTP_204_NO_CONTENT
-            data = {}
+        if serializer.is_valid():
+            validated_data = serializer.validated_data
+            create_or_update_course_exam_configuration(
+                course_id,
+                validated_data['provider'],
+                validated_data['escalation_email'],
+            )
+            return Response({}, status=status.HTTP_204_NO_CONTENT)
         else:
-            response_status = status.HTTP_400_BAD_REQUEST
-            data = error
-
-        return Response(status=response_status, data=data)
+            return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
 
 
 class ProctoringProvidersView(ListAPIView):

edx_exams/apps/core/admin.py

@@ -70,9 +70,9 @@ def change_view(self, request, object_id, form_url='', extra_context=None):
 @admin.register(CourseExamConfiguration)
 class CourseExamConfigurationAdmin(admin.ModelAdmin):
     """ Admin configuration for the Course Exam Configuration model """
-    list_display = ('course_id', 'provider', 'allow_opt_out')
+    list_display = ('course_id', 'provider', 'allow_opt_out', 'escalation_email')
     readonly_fields = ('course_id', 'provider')
-    search_fields = ('course_id', 'provider__name', 'allow_opt_out')
+    search_fields = ('course_id', 'provider__name', 'allow_opt_out', 'escalation_email')
     ordering = ('course_id',)