diff --git a/edx_exams/apps/api/serializers.py b/edx_exams/apps/api/serializers.py index 2248567c..506b11fe 100644 --- a/edx_exams/apps/api/serializers.py +++ b/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': }}, 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': }}, 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 diff --git a/edx_exams/apps/api/v1/tests/test_views.py b/edx_exams/apps/api/v1/tests/test_views.py index a7aadc1a..61658fcf 100644 --- a/edx_exams/apps/api/v1/tests/test_views.py +++ b/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': 'test@example.com'} + + 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,7 +549,7 @@ 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) @@ -537,6 +557,7 @@ def test_patch_null_provider(self): 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): """ diff --git a/edx_exams/apps/api/v1/views.py b/edx_exams/apps/api/v1/views.py index 33441c50..5d609ad2 100644 --- a/edx_exams/apps/api/v1/views.py +++ b/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: diff --git a/edx_exams/apps/core/api.py b/edx_exams/apps/core/api.py index 00b63089..02730cf0 100644 --- a/edx_exams/apps/core/api.py +++ b/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) diff --git a/edx_exams/apps/core/migrations/0022_courseexamconfiguration_escalation_email.py b/edx_exams/apps/core/migrations/0022_courseexamconfiguration_escalation_email.py index ac731612..f16bb682 100644 --- a/edx_exams/apps/core/migrations/0022_courseexamconfiguration_escalation_email.py +++ b/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), ), ] diff --git a/edx_exams/apps/core/models.py b/edx_exams/apps/core/models.py index 0ec4d9eb..821abbd5 100644 --- a/edx_exams/apps/core/models.py +++ b/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: diff --git a/edx_exams/apps/core/tests/test_api.py b/edx_exams/apps/core/tests/test_api.py index e2d575a7..b449e8bc 100644 --- a/edx_exams/apps/core/tests/test_api.py +++ b/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('test@example.com', '') + 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, 'test@example.com') + 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): """ diff --git a/edx_exams/apps/core/tests/test_models.py b/edx_exams/apps/core/tests/test_models.py index 92ace985..9b7c0808 100644 --- a/edx_exams/apps/core/tests/test_models.py +++ b/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()