From 0a8669a31e70ea9aa2296eb820929c0a47b0fec5 Mon Sep 17 00:00:00 2001 From: "Dave St.Germain" Date: Thu, 3 Jan 2019 12:00:26 -0500 Subject: [PATCH 1/3] Refactored signals to their own module --- edx_proctoring/api.py | 26 ------ edx_proctoring/apps.py | 1 + edx_proctoring/models.py | 189 +++----------------------------------- edx_proctoring/signals.py | 106 +++++++++++++++++++++ 4 files changed, 120 insertions(+), 202 deletions(-) create mode 100644 edx_proctoring/signals.py diff --git a/edx_proctoring/api.py b/edx_proctoring/api.py index ccaf2b50a30..b5a552cdc78 100644 --- a/edx_proctoring/api.py +++ b/edx_proctoring/api.py @@ -16,8 +16,6 @@ from django.utils.translation import ugettext as _, ugettext_noop from django.conf import settings from django.contrib.auth.models import User -from django.dispatch import receiver -from django.db.models.signals import post_save from django.template import loader from django.urls import reverse, NoReverseMatch from django.core.mail.message import EmailMessage @@ -201,30 +199,6 @@ def remove_review_policy(exam_id): exam_review_policy.delete() -@receiver(post_save, sender=ProctoredExamReviewPolicy) -@receiver(post_save, sender=ProctoredExam) -def _save_exam_on_backend(sender, instance, **kwargs): # pylint: disable=unused-argument - """ - Save the exam to the backend provider when our model changes. - It also combines the review policy into the exam when saving to the backend - """ - if sender == ProctoredExam: - exam_obj = instance - review_policy = ProctoredExamReviewPolicy.get_review_policy_for_exam(instance.id) - else: - exam_obj = instance.proctored_exam - review_policy = instance - if exam_obj.is_proctored: - exam = ProctoredExamSerializer(exam_obj).data - if review_policy: - exam['rule_summary'] = review_policy.review_policy - backend = get_backend_provider(exam) - external_id = backend.on_exam_saved(exam) - if external_id and external_id != exam_obj.external_id: - exam_obj.external_id = external_id - exam_obj.save() - - def get_review_policy_by_exam_id(exam_id): """ Looks up exam by the Primary Key. Raises exception if not found. diff --git a/edx_proctoring/apps.py b/edx_proctoring/apps.py index 570550684a6..a085649c52d 100644 --- a/edx_proctoring/apps.py +++ b/edx_proctoring/apps.py @@ -109,6 +109,7 @@ def ready(self): """ Loads the available proctoring backends """ + from edx_proctoring import signals # pylint: disable=unused-variable config = settings.PROCTORING_BACKENDS self.backends = {} # pylint: disable=W0201 diff --git a/edx_proctoring/models.py b/edx_proctoring/models.py index 08869e49e9e..8d83e59e133 100644 --- a/edx_proctoring/models.py +++ b/edx_proctoring/models.py @@ -12,8 +12,6 @@ from django.db import models from django.db.models import Q from django.db.models.base import ObjectDoesNotExist -from django.db.models.signals import pre_save, pre_delete -from django.dispatch import receiver from django.utils.translation import ugettext_noop from model_utils.models import TimeStampedModel @@ -187,44 +185,6 @@ def delete(self, *args, **kwargs): # pylint: disable=arguments-differ raise NotImplementedError() -# Hook up the pre_save signal to record creations in the ProctoredExamReviewPolicyHistory table. -@receiver(pre_save, sender=ProctoredExamReviewPolicy) -def on_review_policy_saved(sender, instance, **kwargs): # pylint: disable=unused-argument - """ - Archiving all changes made to the Student Allowance. - Will only archive on update, and not on new entries created. - """ - - if instance.id: - # only for update cases - original = ProctoredExamReviewPolicy.objects.get(id=instance.id) - _make_review_policy_archive_copy(original) - - -# Hook up the pre_delete signal to record creations in the ProctoredExamReviewPolicyHistory table. -@receiver(pre_delete, sender=ProctoredExamReviewPolicy) -def on_review_policy_deleted(sender, instance, **kwargs): # pylint: disable=unused-argument - """ - Archive the allowance when the item is about to be deleted - """ - - _make_review_policy_archive_copy(instance) - - -def _make_review_policy_archive_copy(instance): - """ - Do the copying into the history table - """ - - archive_object = ProctoredExamReviewPolicyHistory( - original_id=instance.id, - set_by_user_id=instance.set_by_user_id, - proctored_exam=instance.proctored_exam, - review_policy=instance.review_policy, - ) - archive_object.save() - - class ProctoredExamStudentAttemptManager(models.Manager): """ Custom manager @@ -473,67 +433,20 @@ class Meta: verbose_name = 'proctored exam attempt history' -@receiver(pre_delete, sender=ProctoredExamStudentAttempt) -def on_attempt_deleted(sender, instance, **kwargs): # pylint: disable=unused-argument - """ - Archive the exam attempt when the item is about to be deleted - Make a clone and populate in the History table - """ - - archive_object = ProctoredExamStudentAttemptHistory( - user=instance.user, - attempt_id=instance.id, - proctored_exam=instance.proctored_exam, - started_at=instance.started_at, - completed_at=instance.completed_at, - attempt_code=instance.attempt_code, - external_id=instance.external_id, - allowed_time_limit_mins=instance.allowed_time_limit_mins, - status=instance.status, - taking_as_proctored=instance.taking_as_proctored, - is_sample_attempt=instance.is_sample_attempt, - student_name=instance.student_name, - review_policy_id=instance.review_policy_id, - last_poll_timestamp=instance.last_poll_timestamp, - last_poll_ipaddr=instance.last_poll_ipaddr, - - ) - archive_object.save() - - -@receiver(pre_save, sender=ProctoredExamStudentAttempt) -def on_attempt_updated(sender, instance, **kwargs): # pylint: disable=unused-argument +def archive_model(model, instance, **mapping): """ - Archive the exam attempt whenever the attempt status is about to be - modified. Make a new entry with the previous value of the status in the - ProctoredExamStudentAttemptHistory table. + Archives the instance to the given history model + optionally maps field names from the instance model to the history model """ - - if instance.id: - # on an update case, get the original - # and see if the status has changed, if so, then we need - # to archive it - original = ProctoredExamStudentAttempt.objects.get(id=instance.id) - - if original.status != instance.status: - archive_object = ProctoredExamStudentAttemptHistory( - user=original.user, - attempt_id=original.id, - proctored_exam=original.proctored_exam, - started_at=original.started_at, - completed_at=original.completed_at, - attempt_code=original.attempt_code, - external_id=original.external_id, - allowed_time_limit_mins=original.allowed_time_limit_mins, - status=original.status, - taking_as_proctored=original.taking_as_proctored, - is_sample_attempt=original.is_sample_attempt, - student_name=original.student_name, - review_policy_id=original.review_policy_id, - last_poll_timestamp=original.last_poll_timestamp, - last_poll_ipaddr=original.last_poll_ipaddr, - ) - archive_object.save() + archive = model() + # timestampedmodels automatically create these + mapping['created'] = mapping['modified'] = None + for field in instance._meta.get_fields(): + to_name = mapping.get(field.name, field.name) + if to_name is not None: + setattr(archive, to_name, getattr(instance, field.name, None)) + archive.save() + return archive class QuerySetWithUpdateOverride(models.QuerySet): @@ -543,7 +456,7 @@ class QuerySetWithUpdateOverride(models.QuerySet): """ def update(self, **kwargs): super(QuerySetWithUpdateOverride, self).update(**kwargs) - _make_archive_copy(self.get()) + archive_model(ProctoredExamStudentAllowanceHistory, self.get(), id='allowance_id') class ProctoredExamStudentAllowanceManager(models.Manager): @@ -717,43 +630,6 @@ class Meta: verbose_name = 'proctored allowance history' -# Hook up the post_save signal to record creations in the ProctoredExamStudentAllowanceHistory table. -@receiver(pre_save, sender=ProctoredExamStudentAllowance) -def on_allowance_saved(sender, instance, **kwargs): # pylint: disable=unused-argument - """ - Archiving all changes made to the Student Allowance. - Will only archive on update, and not on new entries created. - """ - - if instance.id: - original = ProctoredExamStudentAllowance.objects.get(id=instance.id) - _make_archive_copy(original) - - -@receiver(pre_delete, sender=ProctoredExamStudentAllowance) -def on_allowance_deleted(sender, instance, **kwargs): # pylint: disable=unused-argument - """ - Archive the allowance when the item is about to be deleted - """ - - _make_archive_copy(instance) - - -def _make_archive_copy(item): - """ - Make a clone and populate in the History table - """ - - archive_object = ProctoredExamStudentAllowanceHistory( - allowance_id=item.id, - user=item.user, - proctored_exam=item.proctored_exam, - key=item.key, - value=item.value - ) - archive_object.save() - - class ProctoredExamSoftwareSecureReview(TimeStampedModel): """ This is where we store the proctored exam review feedback @@ -842,45 +718,6 @@ class Meta: verbose_name = 'Proctored exam review archive' -# Hook up the post_save signal to record creations in the ProctoredExamStudentAllowanceHistory table. -@receiver(pre_save, sender=ProctoredExamSoftwareSecureReview) -def on_review_saved(sender, instance, **kwargs): # pylint: disable=unused-argument - """ - Archiving all changes made to the Student Allowance. - Will only archive on update, and not on new entries created. - """ - - if instance.id: - # only for update cases - original = ProctoredExamSoftwareSecureReview.objects.get(id=instance.id) - _make_review_archive_copy(original) - - -@receiver(pre_delete, sender=ProctoredExamSoftwareSecureReview) -def on_review_deleted(sender, instance, **kwargs): # pylint: disable=unused-argument - """ - Archive the allowance when the item is about to be deleted - """ - - _make_review_archive_copy(instance) - - -def _make_review_archive_copy(instance): - """ - Do the copying into the history table - """ - - archive_object = ProctoredExamSoftwareSecureReviewHistory( - attempt_code=instance.attempt_code, - review_status=instance.review_status, - raw_data=instance.raw_data, - reviewed_by=instance.reviewed_by, - student=instance.student, - exam=instance.exam, - ) - archive_object.save() - - class ProctoredExamSoftwareSecureComment(TimeStampedModel): """ This is where we store the proctored exam review comments diff --git a/edx_proctoring/signals.py b/edx_proctoring/signals.py new file mode 100644 index 00000000000..8fd94f72895 --- /dev/null +++ b/edx_proctoring/signals.py @@ -0,0 +1,106 @@ +"edx-proctoring signals" +from django.db.models.signals import pre_save, post_save, pre_delete +from django.dispatch import receiver +from edx_proctoring import models +from edx_proctoring.backends import get_backend_provider + + +@receiver(post_save, sender=models.ProctoredExamReviewPolicy) +@receiver(post_save, sender=models.ProctoredExam) +def _save_exam_on_backend(sender, instance, **kwargs): # pylint: disable=unused-argument + """ + Save the exam to the backend provider when our model changes. + It also combines the review policy into the exam when saving to the backend + """ + if sender == models.ProctoredExam: + exam_obj = instance + review_policy = models.ProctoredExamReviewPolicy.get_review_policy_for_exam(instance.id) + else: + exam_obj = instance.proctored_exam + review_policy = instance + if exam_obj.is_proctored: + from edx_proctoring.serializers import ProctoredExamSerializer + exam = ProctoredExamSerializer(exam_obj).data + if review_policy: + exam['rule_summary'] = review_policy.review_policy + backend = get_backend_provider(exam) + external_id = backend.on_exam_saved(exam) + if external_id and external_id != exam_obj.external_id: + exam_obj.external_id = external_id + exam_obj.save() + + +# Hook up the pre_save signal to record creations in the ProctoredExamReviewPolicyHistory table. +@receiver(pre_save, sender=models.ProctoredExamReviewPolicy) +@receiver(pre_delete, sender=models.ProctoredExamReviewPolicy) +def on_review_policy_changed(sender, instance, signal, **kwargs): # pylint: disable=unused-argument + """ + Archiving all changes made to the Review Policy. + Will only archive on update/delete, and not on new entries created. + """ + if signal is pre_save: + if instance.id: + instance = sender.objects.get(id=instance.id) + else: + return + models.archive_model(models.ProctoredExamReviewPolicyHistory, instance, id='original_id') + + +# Hook up the post_save signal to record creations in the ProctoredExamStudentAllowanceHistory table. +@receiver(pre_save, sender=models.ProctoredExamStudentAllowance) +@receiver(pre_delete, sender=models.ProctoredExamStudentAllowance) +def on_allowance_saved(sender, instance, signal, **kwargs): # pylint: disable=unused-argument + """ + Archiving all changes made to the Student Allowance. + Will only archive on update/delete, and not on new entries created. + """ + + if signal is pre_save: + if instance.id: + instance = sender.objects.get(id=instance.id) + else: + return + models.archive_model(models.ProctoredExamStudentAllowanceHistory, instance, id='allowance_id') + + +@receiver(pre_save, sender=models.ProctoredExamStudentAttempt) +@receiver(pre_delete, sender=models.ProctoredExamStudentAttempt) +def on_attempt_updated(sender, instance, signal, **kwargs): # pylint: disable=unused-argument + """ + Archive the exam attempt whenever the attempt status is about to be + modified. Make a new entry with the previous value of the status in the + ProctoredExamStudentAttemptHistory table. + """ + + if signal is pre_save: + if instance.id: + # on an update case, get the original + # and see if the status has changed, if so, then we need + # to archive it + original = sender.objects.get(id=instance.id) + + if original.status != instance.status: + instance = original + else: + return + else: + return + models.archive_model(models.ProctoredExamStudentAttemptHistory, instance, id='attempt_id') + + +# Hook up the signals to record updates/deletions in the ProctoredExamStudentAllowanceHistory table. +@receiver(pre_save, sender=models.ProctoredExamSoftwareSecureReview) +@receiver(pre_delete, sender=models.ProctoredExamSoftwareSecureReview) +def on_review_saved(sender, instance, signal, **kwargs): # pylint: disable=unused-argument + """ + Archiving all changes made to the Review. + Will only archive on update/delete, and not on new entries created. + """ + if signal is pre_save: + if instance.id: + # only for update cases + instance = sender.objects.get(id=instance.id) + else: + # don't archive on create + return + models.archive_model(models.ProctoredExamSoftwareSecureReviewHistory, instance, id='review_id') From 6a60af4796e7129a4f00d6966fc35f74b764fe7b Mon Sep 17 00:00:00 2001 From: "Dave St.Germain" Date: Thu, 3 Jan 2019 12:22:19 -0500 Subject: [PATCH 2/3] Update the backend when an exam switches from proctored to timed --- edx_proctoring/signals.py | 26 ++++++++++++++++++++++---- edx_proctoring/tests/test_api.py | 10 ++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/edx_proctoring/signals.py b/edx_proctoring/signals.py index 8fd94f72895..235d3d68a0c 100644 --- a/edx_proctoring/signals.py +++ b/edx_proctoring/signals.py @@ -5,9 +5,27 @@ from edx_proctoring.backends import get_backend_provider +@receiver(pre_save, sender=models.ProctoredExam) +def check_for_category_switch(sender, instance, **kwargs): # pylint: disable=unused-argument + """ + If the exam switches from proctored to timed, notify the backend + """ + if instance.id: + original = sender.objects.get(pk=instance.id) + if original.is_proctored and instance.is_proctored != original.is_proctored: + from edx_proctoring.serializers import ProctoredExamSerializer + exam = ProctoredExamSerializer(instance).data + exam['is_active'] = False + exam['is_proctored'] = True + # we have to pretend that the exam is still proctored + # or else we get_backend_provider will return None + backend = get_backend_provider(exam) + backend.on_exam_saved(exam) + + @receiver(post_save, sender=models.ProctoredExamReviewPolicy) @receiver(post_save, sender=models.ProctoredExam) -def _save_exam_on_backend(sender, instance, **kwargs): # pylint: disable=unused-argument +def save_exam_on_backend(sender, instance, **kwargs): # pylint: disable=unused-argument """ Save the exam to the backend provider when our model changes. It also combines the review policy into the exam when saving to the backend @@ -49,7 +67,7 @@ def on_review_policy_changed(sender, instance, signal, **kwargs): # pylint: dis # Hook up the post_save signal to record creations in the ProctoredExamStudentAllowanceHistory table. @receiver(pre_save, sender=models.ProctoredExamStudentAllowance) @receiver(pre_delete, sender=models.ProctoredExamStudentAllowance) -def on_allowance_saved(sender, instance, signal, **kwargs): # pylint: disable=unused-argument +def on_allowance_changed(sender, instance, signal, **kwargs): # pylint: disable=unused-argument """ Archiving all changes made to the Student Allowance. Will only archive on update/delete, and not on new entries created. @@ -65,7 +83,7 @@ def on_allowance_saved(sender, instance, signal, **kwargs): # pylint: disable=u @receiver(pre_save, sender=models.ProctoredExamStudentAttempt) @receiver(pre_delete, sender=models.ProctoredExamStudentAttempt) -def on_attempt_updated(sender, instance, signal, **kwargs): # pylint: disable=unused-argument +def on_attempt_changed(sender, instance, signal, **kwargs): # pylint: disable=unused-argument """ Archive the exam attempt whenever the attempt status is about to be modified. Make a new entry with the previous value of the status in the @@ -91,7 +109,7 @@ def on_attempt_updated(sender, instance, signal, **kwargs): # pylint: disable=u # Hook up the signals to record updates/deletions in the ProctoredExamStudentAllowanceHistory table. @receiver(pre_save, sender=models.ProctoredExamSoftwareSecureReview) @receiver(pre_delete, sender=models.ProctoredExamSoftwareSecureReview) -def on_review_saved(sender, instance, signal, **kwargs): # pylint: disable=unused-argument +def on_review_changed(sender, instance, signal, **kwargs): # pylint: disable=unused-argument """ Archiving all changes made to the Review. Will only archive on update/delete, and not on new entries created. diff --git a/edx_proctoring/tests/test_api.py b/edx_proctoring/tests/test_api.py index d5667d9ad65..525d4138c71 100644 --- a/edx_proctoring/tests/test_api.py +++ b/edx_proctoring/tests/test_api.py @@ -174,6 +174,16 @@ def test_update_timed_exam(self): self.assertEqual(update_timed_exam.hide_after_due, True) + def test_switch_from_proctored_to_timed(self): + """ + Test that switches an exam from proctored to timed. + The backend should be notified that the exam is inactive + """ + proctored_exam = get_exam_by_id(self.proctored_exam_id) + update_exam(self.proctored_exam_id, is_proctored=False) + backend = get_backend_provider(proctored_exam) + self.assertEqual(backend.last_exam['is_active'], False) + def test_update_non_existing_exam(self): """ test to update the non-existing proctored exam From 255470c19b6c1cbe6367eee1527aea50ddc6d61e Mon Sep 17 00:00:00 2001 From: "Dave St.Germain" Date: Fri, 4 Jan 2019 14:59:13 -0500 Subject: [PATCH 3/3] Addressed feedback by allowing one to pass in the name to get_backend_provider --- edx_proctoring/backends/__init__.py | 9 +++++---- edx_proctoring/backends/tests/test_backend.py | 2 ++ edx_proctoring/signals.py | 6 ++---- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/edx_proctoring/backends/__init__.py b/edx_proctoring/backends/__init__.py index d37388bb3f7..99eead980bd 100644 --- a/edx_proctoring/backends/__init__.py +++ b/edx_proctoring/backends/__init__.py @@ -4,15 +4,16 @@ from django.apps import apps -def get_backend_provider(exam=None): +def get_backend_provider(exam=None, name=None): """ Returns an instance of the configured backend provider + Passing in an exam will return the backend for that exam + Passing in a name will return the named backend """ - backend_name = None if exam: if 'is_proctored' in exam and not exam['is_proctored']: # timed exams don't have a backend return None elif exam['backend']: - backend_name = exam['backend'] - return apps.get_app_config('edx_proctoring').get_backend(name=backend_name) + name = exam['backend'] + return apps.get_app_config('edx_proctoring').get_backend(name=name) diff --git a/edx_proctoring/backends/tests/test_backend.py b/edx_proctoring/backends/tests/test_backend.py index 742a6c74ac2..97cedf73c6f 100644 --- a/edx_proctoring/backends/tests/test_backend.py +++ b/edx_proctoring/backends/tests/test_backend.py @@ -212,6 +212,8 @@ def test_get_different_backend(self): """ backend = get_backend_provider({'backend': 'null'}) self.assertIsInstance(backend, NullBackendProvider) + backend = get_backend_provider(name='test') + self.assertIsInstance(backend, TestBackendProvider) def test_backend_choices(self): """ diff --git a/edx_proctoring/signals.py b/edx_proctoring/signals.py index 235d3d68a0c..764940e9ad6 100644 --- a/edx_proctoring/signals.py +++ b/edx_proctoring/signals.py @@ -15,11 +15,9 @@ def check_for_category_switch(sender, instance, **kwargs): # pylint: disable=un if original.is_proctored and instance.is_proctored != original.is_proctored: from edx_proctoring.serializers import ProctoredExamSerializer exam = ProctoredExamSerializer(instance).data + # from the perspective of the backend, the exam is now inactive. exam['is_active'] = False - exam['is_proctored'] = True - # we have to pretend that the exam is still proctored - # or else we get_backend_provider will return None - backend = get_backend_provider(exam) + backend = get_backend_provider(name=exam['backend']) backend.on_exam_saved(exam)