Merge pull request #82 from edx/alangsto/add_delete_handlers

feat: if a proctored exam or IDV attempt is deleted, we should delete the VerifiedName

CHANGELOG.rst

@@ -14,6 +14,11 @@ Change Log
 Unreleased
 ~~~~~~~~~~
 
+[2.3.1] - 2022-03-02
+~~~~~~~~~~~~~~~~~~~~
+* Add two signal handlers to capture post_delete signals from ProctoredExamStudentAttempt and SoftwareSecurePhotoVerification models.
+  If those signals are received, the corresponding VerifiedName(s), if it exists, will be deleted.
+
 [2.3.0] - 2022-02-28
 ~~~~~~~~~~~~~~~~~~~~
 * Add REST API functionality to update verified name status, and to delete verified names.

edx_name_affirmation/__init__.py

@@ -2,6 +2,6 @@
 Django app housing name affirmation logic.
 """
 
-__version__ = '2.3.0'
+__version__ = '2.3.1'
 
 default_app_config = 'edx_name_affirmation.apps.EdxNameAffirmationConfig'  # pylint: disable=invalid-name

edx_name_affirmation/apps.py

@@ -28,10 +28,20 @@ class EdxNameAffirmationConfig(AppConfig):
                         'receiver_func_name': 'idv_attempt_handler',
                         'signal_path': 'lms.djangoapps.verify_student.signals.idv_update_signal',
                     },
+                    {
+                        'receiver_func_name': 'idv_delete_handler',
+                        'signal_path': 'django.db.models.signals.post_delete',
+                        'sender_path': 'lms.djangoapps.verify_student.models.SoftwareSecurePhotoVerification',
+                    },
                     {
                         'receiver_func_name': 'proctoring_attempt_handler',
                         'signal_path': 'edx_proctoring.signals.exam_attempt_status_signal',
-                    }
+                    },
+                    {
+                        'receiver_func_name': 'proctoring_delete_handler',
+                        'signal_path': 'django.db.models.signals.post_delete',
+                        'sender_path': 'edx_proctoring.models.ProctoredExamStudentAttempt',
+                    },
                 ],
             }
         }

edx_name_affirmation/handlers.py

@@ -11,7 +11,11 @@
 from edx_name_affirmation.models import VerifiedName
 from edx_name_affirmation.signals import VERIFIED_NAME_APPROVED
 from edx_name_affirmation.statuses import VerifiedNameStatus
-from edx_name_affirmation.tasks import idv_update_verified_name_task, proctoring_update_verified_name_task
+from edx_name_affirmation.tasks import (
+    delete_verified_name_task,
+    idv_update_verified_name_task,
+    proctoring_update_verified_name_task
+)
 
 User = get_user_model()
 
@@ -66,6 +70,23 @@ def idv_attempt_handler(attempt_id, user_id, status, photo_id_name, full_name, *
                  )
 
 
+def idv_delete_handler(sender, instance, signal, **kwargs):  # pylint: disable=unused-argument
+    """
+    Receiver for IDV attempt deletions
+
+    Args:
+        attempt_id(int): ID associated with the IDV attempt
+    """
+    idv_attempt_id = instance.id
+    log.info(
+        'VerifiedName: idv_delete_handler triggering Celery task for idv_attempt_id=%(idv_attempt_id)s',
+        {
+            'idv_attempt_id': idv_attempt_id,
+        }
+    )
+    delete_verified_name_task.delay(idv_attempt_id, None)
+
+
 def proctoring_attempt_handler(
     attempt_id,
     user_id,
@@ -119,3 +140,21 @@ def proctoring_attempt_handler(
                      'status': status,
                  }
                  )
+
+
+def proctoring_delete_handler(sender, instance, signal, **kwargs):  # pylint: disable=unused-argument
+    """
+    Receiver for proctoring attempt deletions
+
+    Args:
+        attempt_id(int): ID associated with the proctoring attempt
+    """
+    proctoring_attempt_id = instance.id
+    log.info(
+        'VerifiedName: proctoring_delete_handler triggering Celery task for '
+        'proctoring_attempt_id=%(proctoring_attempt_id)s',
+        {
+            'proctoring_attempt_id': proctoring_attempt_id,
+        }
+    )
+    delete_verified_name_task.delay(None, proctoring_attempt_id)

edx_name_affirmation/tasks.py

@@ -170,3 +170,45 @@ def proctoring_update_verified_name_task(
                     attempt_id=attempt_id,
                 )
             )
+
+
+@shared_task(
+    bind=True, autoretry_for=(Exception,), default_retry_delay=DEFAULT_RETRY_SECONDS, max_retries=MAX_RETRIES,
+)
+@set_code_owner_attribute
+def delete_verified_name_task(self, idv_attempt_id, proctoring_attempt_id):
+    """
+    Celery task to delete a verified name based on an idv or proctoring attempt
+    """
+    # this case shouldn't happen, but should log as an error in case
+    if (idv_attempt_id and proctoring_attempt_id) or (not idv_attempt_id and not proctoring_attempt_id):
+        log.error(
+            'A maximum of one attempt id should be provided for either a proctored exam attempt or IDV attempt.'
+        )
+        return
+
+    log_message = {'field_name': '', 'attempt_id': ''}
+
+    if idv_attempt_id:
+        verified_names = VerifiedName.objects.filter(verification_attempt_id=idv_attempt_id)
+        log_message['field_name'] = 'verification_attempt_id'
+        log_message['attempt_id'] = idv_attempt_id
+    else:
+        verified_names = VerifiedName.objects.filter(proctored_exam_attempt_id=proctoring_attempt_id)
+        log_message['field_name'] = 'proctored_exam_attempt_id'
+        log_message['attempt_id'] = proctoring_attempt_id
+
+    if verified_names:
+        log.info(
+            'Deleting {num_names} VerifiedName(s) associated with {field_name}='
+            '{verification_attempt_id}'.format(
+                num_names=len(verified_names),
+                field_name=log_message['field_name'],
+                verification_attempt_id=log_message['attempt_id'],
+            )
+        )
+        verified_names.delete()
+
+    log.info(
+        'No VerifiedNames deleted because no VerifiedNames were associated with the provided attempt ID.'
+    )

edx_name_affirmation/tests/test_handlers.py

@@ -3,12 +3,17 @@
 """
 
 import ddt
-from mock import patch
+from mock import MagicMock, patch
 
 from django.contrib.auth import get_user_model
 from django.test import TestCase
 
-from edx_name_affirmation.handlers import idv_attempt_handler, proctoring_attempt_handler
+from edx_name_affirmation.handlers import (
+    idv_attempt_handler,
+    idv_delete_handler,
+    proctoring_attempt_handler,
+    proctoring_delete_handler
+)
 from edx_name_affirmation.models import VerifiedName
 from edx_name_affirmation.statuses import VerifiedNameStatus
 
@@ -216,6 +221,21 @@ def test_idv_non_trigger_status(self, status, mock_task):
 
         mock_task.assert_not_called()
 
+    @patch('edx_name_affirmation.tasks.delete_verified_name_task.delay')
+    def test_idv_delete_handler(self, mock_task):
+        """
+        Test that a celery task is triggered if an idv delete signal is received
+        """
+        mock_idv_object = MagicMock()
+        mock_idv_object.id = 'abcdef'
+        idv_delete_handler(
+            {},
+            mock_idv_object,
+            '',
+        )
+
+        mock_task.assert_called_with(mock_idv_object.id, None)
+
 
 @ddt.ddt
 class ProctoringSignalTests(SignalTestCase):
@@ -435,3 +455,18 @@ def test_proctoring_non_trigger_status(self, status, mock_task):
         )
 
         mock_task.assert_not_called()
+
+    @patch('edx_name_affirmation.tasks.delete_verified_name_task.delay')
+    def test_proctoring_delete_handler(self, mock_task):
+        """
+        Test that a celery task is triggered if an idv delete signal is received
+        """
+        mock_proctoring_object = MagicMock()
+        mock_proctoring_object.id = 'abcdef'
+        proctoring_delete_handler(
+            {},
+            mock_proctoring_object,
+            '',
+        )
+
+        mock_task.assert_called_with(None, mock_proctoring_object.id)

edx_name_affirmation/tests/test_tasks.py

@@ -2,18 +2,24 @@
 Tests for Name Affirmation tasks
 """
 
+import ddt
 from mock import patch
 
 from django.contrib.auth import get_user_model
 from django.test import TestCase
 
 from edx_name_affirmation.models import VerifiedName
 from edx_name_affirmation.statuses import VerifiedNameStatus
-from edx_name_affirmation.tasks import idv_update_verified_name_task, proctoring_update_verified_name_task
+from edx_name_affirmation.tasks import (
+    delete_verified_name_task,
+    idv_update_verified_name_task,
+    proctoring_update_verified_name_task
+)
 
 User = get_user_model()
 
 
+@ddt.ddt
 class TaskTests(TestCase):
     """
     Tests for tasks.py
@@ -51,3 +57,101 @@ def test_proctoring_retry(self, mock_retry):
             self.verified_name_obj.profile_name,
         )
         mock_retry.assert_called()
+
+    def test_idv_delete(self):
+        """
+        Assert that only relevant VerifiedNames are deleted for a given idv_attempt_id
+        """
+        # associated test object with an idv attempt
+        self.verified_name_obj.verification_attempt_id = self.idv_attempt_id
+        self.verified_name_obj.save()
+
+        other_attempt_id = 123456
+
+        # create another VerifiedName with the same idv attempt
+        VerifiedName(
+            user=self.user,
+            verified_name='Jonathan X Doe',
+            profile_name='Jon D',
+            verification_attempt_id=self.idv_attempt_id
+        ).save()
+
+        # create VerifiedName not associated with idv attempt
+        other_verified_name_obj = VerifiedName(
+            user=self.user,
+            verified_name='Jonathan X Doe',
+            profile_name='Jon D',
+            verification_attempt_id=other_attempt_id
+        )
+        other_verified_name_obj.save()
+
+        delete_verified_name_task.delay(
+            self.idv_attempt_id,
+            None
+        )
+
+        # check that there is only VerifiedName object
+        self.assertEqual(len(VerifiedName.objects.filter(verification_attempt_id=self.idv_attempt_id)), 0)
+        self.assertEqual(len(VerifiedName.objects.filter(verification_attempt_id=other_attempt_id)), 1)
+
+    def test_proctoring_delete(self):
+        """
+        Assert that only relevant VerifiedNames are deleted for a given proctoring_attempt_id
+        """
+        # associated test object with a proctoring attempt
+        self.verified_name_obj.proctored_exam_attempt_id = self.proctoring_attempt_id
+        self.verified_name_obj.save()
+
+        other_attempt_id = 123456
+
+        # create another VerifiedName with the same proctoring attempt
+        VerifiedName(
+            user=self.user,
+            verified_name='Jonathan X Doe',
+            profile_name='Jon D',
+            proctored_exam_attempt_id=self.proctoring_attempt_id
+        ).save()
+
+        # create VerifiedName not associated with proctoring attempt
+        other_verified_name_obj = VerifiedName(
+            user=self.user,
+            verified_name='Jonathan X Doe',
+            profile_name='Jon D',
+            proctored_exam_attempt_id=other_attempt_id
+        )
+        other_verified_name_obj.save()
+
+        delete_verified_name_task.delay(
+            None,
+            self.proctoring_attempt_id
+        )
+
+        # check that there is only VerifiedName object
+        self.assertEqual(len(VerifiedName.objects.filter(proctored_exam_attempt_id=self.proctoring_attempt_id)), 0)
+        self.assertEqual(len(VerifiedName.objects.filter(proctored_exam_attempt_id=other_attempt_id)), 1)
+
+    @ddt.data(
+        (1234, 5678),
+        (None, None)
+    )
+    @ddt.unpack
+    @patch('logging.Logger.error')
+    def test_incorrect_args_delete(self, idv_attempt_id, proctoring_attempt_id, mock_logger):
+        """
+        Assert that error log is called and that no VerifiedNames are deleted when incorrect args are passed to task
+        """
+        delete_verified_name_task.delay(
+            idv_attempt_id,
+            proctoring_attempt_id
+        )
+        mock_logger.assert_called()
+
+    @patch('logging.Logger.info')
+    def test_no_names_delete(self, mock_logger):
+        delete_verified_name_task.delay(
+            self.idv_attempt_id,
+            None
+        )
+        mock_logger.assert_called_with(
+            'No VerifiedNames deleted because no VerifiedNames were associated with the provided attempt ID.'
+        )