feat: add escalation_email to course exam configurations #221

MichaelRoytman · 2023-12-05T19:45:45Z

Description: This commit adds an escalation email to course exam configurations. This value is used to specify who learners can contact in the event of issues with or questions about their exam attempts.

Author concerns: None.

Dependencies: None.

Installation instructions: None.

Testing instructions: None.

Merge checklist:

Post merge:

Delete working branch (if not needed anymore)

MichaelRoytman · 2023-12-06T13:15:18Z

edx_exams/apps/api/serializers.py

+    """
+    Serializer for the CourseExamConfiguration model
+    """
+    provider = serializers.CharField(source='provider.name', allow_null=True)


I set the source so that you when you serialize a CourseExamConfiguration instance, it pulls the provider's name for the provider field and not the primary key. However, a side effect is that if you deserialize some data (i.e. in the PATCH handler) like the following,

{'provider': 'test', 'escalation_email': '[email protected]'}

you end up with this data.

{'provider': {'name': 'test'}, 'escalation_email': '[email protected]'}

I'm not really sure of a way around this, unless I write two separate serializers - one for GETs and one for PUTs/PATCHEs.

I don't like it, but I have no better solution either. Maybe a quick comment on the class? I feel like this is going to bite someone later.

edx_exams/apps/api/v1/views.py

MichaelRoytman · 2023-12-06T13:20:18Z

edx_exams/apps/core/api.py

+        return course_config.escalation_email
+
+
+def create_or_update_course_exam_configuration(course_id, provider_name, escalation_email):


I added an API function so that callers don't have to have references to the CourseExamConfiguration model to call the class method.

MichaelRoytman · 2023-12-06T16:52:01Z

edx_exams/apps/api/v1/views.py

@@ -246,46 +253,33 @@ 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


I've added a serializer, so I've removed this comment.

MichaelRoytman · 2023-12-06T16:54:01Z

edx_exams/apps/api/serializers.py

+        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


Let me know if you disagree here. This is regarding disabling the Pylint rule below.

never ran into this rule. Solution seems fine. But, would raise ValidationError from None work?

MichaelRoytman · 2023-12-06T16:57:37Z

edx_exams/apps/core/migrations/0021_add_courseexamconfiguration_escalation_email.py

+        migrations.AddField(
+            model_name='courseexamconfiguration',
+            name='escalation_email',
+            field=models.EmailField(default='', max_length=254),


I made the decision to set the default of '' only for existing instances in the database and not for all past and future instances. This is why default='' is not on the escalation_email model field.

MichaelRoytman · 2023-12-06T16:58:23Z

edx_exams/apps/core/models.py

+            exam.provider = new_provider
+            exam.save()
+
+        return len(exams)


The original function wasn't actually returning a length, so the count variable was None. I've fixed that here.

MichaelRoytman · 2023-12-06T16:58:35Z

edx_exams/apps/core/models.py

+        existing_config.save()
+
+    @classmethod
+    def _sync_exams_with_new_provider(cls, course_id, new_provider):


I pulled the exam sync behavior into its own utility function.

edx_exams/apps/core/test_utils/factories.py

edx_exams/apps/core/tests/test_api.py

zacharis278

still reviewing but a few comments to start

zacharis278 · 2023-12-06T18:54:29Z

edx_exams/apps/api/serializers.py

+        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


never ran into this rule. Solution seems fine. But, would raise ValidationError from None work?

zacharis278 · 2023-12-06T19:00:07Z

edx_exams/apps/core/migrations/0021_add_courseexamconfiguration_escalation_email.py

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


huh something does seem off here. Also looking into what might cause this

edx_exams/apps/api/v1/views.py

zacharis278 · 2023-12-06T19:09:22Z

edx_exams/apps/api/serializers.py

+        """
+        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


Is this just because of our existing data. If we split the PR to just add the migration for the model field first and than give existing configs a value in stage does that eliminate the need for the empty string default on the model?

Just to be sure we're on the same page, the empty string default is on the serializer. There is no default on the model, but blanks are allowed. Do you mean the default on the serializer? Or the blank on the model?

I changed the model field and forgot to update this comment, so that might be why I was talking about the model here.

If I don't define the escalation_email serializer field explicitly, the DRF-generated one will look like this.

CourseExamConfigurationSerializer(): provider = CharField(allow_null=True, source='provider.name') escalation_email = EmailField(allow_blank=True, max_length=254, required=False)

If I remove the explicit escalation_email serializer field and remove this comment, does that work? I think a more accurate comment is, "Because an escalation email is only required when a proctoring provider is set, we have to enforce this requirement in the validate function."

ah yeah I just got confused by this comment and thought you were referring to the model itself. I'm still fuzzy on what the explicit escalation_email serializer field is doing for us. The default looks fine to me but I might be missing some nuance here.

zacharis278 · 2023-12-06T19:10:13Z

edx_exams/apps/core/models.py

+        existing_config.provider = new_provider
+
+        if new_provider is None:
+            existing_config.escalation_email = ''


basically same as my earlier comment is this only because of our existing data?

I wanted to avoid a situation where someone sends this data {'provider': None, 'escalation_email': '[email protected]'}. I believe we should always set the escalation_email to the empty string if there is no provider. Do you want to leave it up to the client to be sure to do this? Or we could raise a validation error, alternatively. That would be more explicit.

oh this make sense. I misread the condition as if escalation_email is None: thinking we were doing some Null -> '' conversion.

MichaelRoytman · 2023-12-07T15:51:14Z

edx_exams/apps/core/models.py

+        # 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 = ''


I'm finding that I'm duplicating this validation in api.py and models.py. There’s only so much one can do to prevent someone from saving invalid data into the database if the invariant isn’t at the database level, but I think it's okay to have it on the model as well because I cannot guarantee the API will be used, and I don't want to save invalid data into the database.

Some learnings I've taken from this design are the negatives of rigidly following the pattern for view <-> api <-> model. This kind of duplication being one of them.

zacharis278 · 2023-12-07T17:44:50Z

edx_exams/apps/core/models.py

+        # 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 = ''


Some learnings I've taken from this design are the negatives of rigidly following the pattern for view <-> api <-> model. This kind of duplication being one of them.

zacharis278 · 2023-12-07T17:48:34Z

edx_exams/apps/api/serializers.py

+    """
+    Serializer for the CourseExamConfiguration model
+    """
+    provider = serializers.CharField(source='provider.name', allow_null=True)


I don't like it, but I have no better solution either. Maybe a quick comment on the class? I feel like this is going to bite someone later.

edx_exams/apps/core/tests/test_api.py

MichaelRoytman · 2023-12-08T14:35:43Z

edx_exams/apps/api/serializers.py

+    """
+    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


Based on an earlier conversation with Zach, I decided it would be better to have two separate serializers than to have to worry about someone reading the data from the serializer improperly. This will be less error prone, and it's explicit that there are two different serializers - one for reading and one for writing. I can revert this to the old code (see my first commit), but I think this is an improvement.

MichaelRoytman · 2023-12-08T14:40:23Z

edx_exams/apps/api/serializers.py

+    """
+    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)


This is a change.

The original was escalation_email = EmailField(allow_blank=True, max_length=254, required=False), which is the default generated by DRF.

The new one is escalation_email = serializers.EmailField(allow_null=True, allow_blank=False, required=True).

The two differences are...

We now allow nulls and forbid blanks. Although this is not Django/DRF best-practice for character based fields, this more closely matches both the existing API on this endpoint, because provider is a nullable character field, and the Studio proctoring settings API, which accepts a null for the escalation_email. I have set allow_blank=False so that we only have one representation of the empty value.

This field is now required. I think it's a clearer API contract with the client for this value to always be required. Otherwise, the client is left wondering why a value is required sometimes and not other times. Sending escalation_email: null from the client is more explicit.

MichaelRoytman · 2023-12-08T14:42:47Z

edx_exams/apps/api/serializers.py

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


This is a new validation error. In the spirit of being more explicit, I've removed the behavior I had originally, which was to set the escalation_email to None when the provider is None, even when the escalation_email is not None (e.g. {'provider': None, 'escalation_email': '[email protected]'}). The client should supply valid data or receive a 400 error. I don't want to get in the habit of guessing what the client's intentions are when the escalation is not valid.

MichaelRoytman · 2023-12-08T14:44:50Z

edx_exams/apps/core/api.py

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


Despite my comment in the serializers.py file, I think we still need to enforce this requirement in the Python API.

zacharis278

nice improvements, this is a bit easier to understand now 👍

This commit adds an escalation email to course exam configurations. This value is used to specify who learners can contact in the event of issues with or questions about their exam attempts.

MichaelRoytman force-pushed the michaelroytman/COSMO-39-escalalation-email branch from cfd7c4a to 1945397 Compare December 5, 2023 20:02

MichaelRoytman changed the title ~~[WIP] feat: add escalation_email to course exam configurations~~ feat: add escalation_email to course exam configurations Dec 6, 2023

MichaelRoytman force-pushed the michaelroytman/COSMO-39-escalalation-email branch 2 times, most recently from fe4505a to b7563f2 Compare December 6, 2023 17:37

MichaelRoytman commented Dec 6, 2023

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/COSMO-39-escalalation-email branch 2 times, most recently from 156eb50 to 3e7ea8b Compare December 6, 2023 17:50

zacharis278 reviewed Dec 6, 2023

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/COSMO-39-escalalation-email branch 3 times, most recently from 85168c9 to 6624ee5 Compare December 7, 2023 15:48

MichaelRoytman commented Dec 7, 2023

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/COSMO-39-escalalation-email branch from 6624ee5 to d6cd4ec Compare December 7, 2023 16:24

zacharis278 approved these changes Dec 7, 2023

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/COSMO-39-escalalation-email branch from bec8966 to be2e50e Compare December 8, 2023 14:45

MichaelRoytman commented Dec 8, 2023

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/COSMO-39-escalalation-email branch 2 times, most recently from 381a070 to 5eb4b10 Compare December 8, 2023 15:08

zacharis278 approved these changes Dec 8, 2023

View reviewed changes

MichaelRoytman mentioned this pull request Dec 8, 2023

[COSMO-39] Add escalation email field for LTI-based proctoring providers. openedx/frontend-app-authoring#736

Merged

feat: add escalation_email to course exam configuration

2bc4859

This commit adds an escalation email to course exam configurations. This value is used to specify who learners can contact in the event of issues with or questions about their exam attempts.

MichaelRoytman force-pushed the michaelroytman/COSMO-39-escalalation-email branch from 5eb4b10 to 2bc4859 Compare December 11, 2023 14:56

MichaelRoytman merged commit ceceedd into main Dec 11, 2023
8 checks passed

MichaelRoytman deleted the michaelroytman/COSMO-39-escalalation-email branch December 11, 2023 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add escalation_email to course exam configurations #221

feat: add escalation_email to course exam configurations #221

MichaelRoytman commented Dec 5, 2023 •

edited

Loading

MichaelRoytman Dec 6, 2023

zacharis278 Dec 7, 2023

MichaelRoytman Dec 6, 2023

MichaelRoytman Dec 6, 2023

MichaelRoytman Dec 6, 2023

zacharis278 Dec 6, 2023

MichaelRoytman Dec 6, 2023

MichaelRoytman Dec 6, 2023

MichaelRoytman Dec 6, 2023

zacharis278 left a comment

zacharis278 Dec 6, 2023

zacharis278 Dec 6, 2023

zacharis278 Dec 6, 2023

MichaelRoytman Dec 6, 2023

zacharis278 Dec 6, 2023 •

edited

Loading

zacharis278 Dec 6, 2023

MichaelRoytman Dec 6, 2023 •

edited

Loading

zacharis278 Dec 6, 2023

MichaelRoytman Dec 7, 2023

zacharis278 Dec 7, 2023

zacharis278 Dec 7, 2023

zacharis278 Dec 7, 2023

MichaelRoytman Dec 8, 2023

MichaelRoytman Dec 8, 2023

MichaelRoytman Dec 8, 2023

MichaelRoytman Dec 8, 2023

zacharis278 left a comment

		return course_config.escalation_email


		def create_or_update_course_exam_configuration(course_id, provider_name, escalation_email):

feat: add escalation_email to course exam configurations #221

feat: add escalation_email to course exam configurations #221

Conversation

MichaelRoytman commented Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharis278 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharis278 Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelRoytman Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharis278 left a comment

Choose a reason for hiding this comment

MichaelRoytman commented Dec 5, 2023 •

edited

Loading

zacharis278 Dec 6, 2023 •

edited

Loading

MichaelRoytman Dec 6, 2023 •

edited

Loading