From e462e5225eedfc5a8ef82e3cd2dae64de84a23b5 Mon Sep 17 00:00:00 2001 From: michaelroytman Date: Mon, 12 Aug 2024 16:55:45 -0400 Subject: [PATCH 1/9] feat: add VerificationAttempt model to verify_student application This commits adds a VerificationAttempt model to store implementation and provider agnostic information about identity verification attempts in the platform. --- lms/djangoapps/verify_student/models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 903d80bf9245..d85f172a7df0 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1213,4 +1213,5 @@ class VerificationAttempt(TimeStampedModel): expiration_datetime = models.DateTimeField( null=True, blank=True, + db_index=True, ) From e6351597a83f2fc458809c1935809b934d72f78d Mon Sep 17 00:00:00 2001 From: ilee2u Date: Wed, 21 Aug 2024 15:42:30 -0400 Subject: [PATCH 2/9] feat: add api for VerificationAttempt model --- lms/djangoapps/verify_student/api.py | 40 +++++++++ .../commands/approve_id_verifications.py | 2 +- .../verify_student/tests/test_api.py | 85 ++++++++++++++++++- 3 files changed, 124 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index c974fa0c8e5f..034a6002f954 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -5,6 +5,7 @@ from django.utils.translation import gettext as _ from lms.djangoapps.verify_student.emails import send_verification_approved_email +from lms.djangoapps.verify_student.models import VerificationAttempt from lms.djangoapps.verify_student.tasks import send_verification_status_email @@ -33,3 +34,42 @@ def send_approval_email(attempt): else: email_context = {'user': attempt.user, 'expiration_datetime': expiration_datetime.strftime("%m/%d/%Y")} send_verification_approved_email(context=email_context) + + +def create_verification_attempt(user, name, status, expiration_datetime=None): + """ + Create a verification attempt. + + Args: + user (User): the user (usually a learner) performing the verfication attempt + name (string): the name of the user + status (string): the initial status of the verification attempt + expiration_datetime (datetime, optional): When the verification attempt expires. Defaults to None. + + Returns: + VerificationAttempt (VerificationAttempt): The created VerificationAttempt instance + """ + verification_attempt = VerificationAttempt.objects.create( + user=user, + name=name, + status=status, + expiration_datetime=expiration_datetime, + ) + + return verification_attempt.id + + +def update_verification_status(attempt_id, status): + """ + Update the VerificationAttempt status. + + Arguments: + * id (str): the verification attempt id + * status (str): the new status + + Returns: + * None + """ + attempt = VerificationAttempt.objects.get(id=attempt_id) + attempt.status = status + attempt.save() diff --git a/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py b/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py index b87b2eee4559..6d505618bcb3 100644 --- a/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py +++ b/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py @@ -8,7 +8,7 @@ import time from pprint import pformat -from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user +from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user, unused-import from django.core.management.base import BaseCommand, CommandError from lms.djangoapps.verify_student.api import send_approval_email diff --git a/lms/djangoapps/verify_student/tests/test_api.py b/lms/djangoapps/verify_student/tests/test_api.py index acdebaa70c1c..4f5f42aedc04 100644 --- a/lms/djangoapps/verify_student/tests/test_api.py +++ b/lms/djangoapps/verify_student/tests/test_api.py @@ -3,14 +3,20 @@ """ from unittest.mock import patch +from datetime import datetime, timezone import ddt from django.conf import settings from django.core import mail from django.test import TestCase from common.djangoapps.student.tests.factories import UserFactory -from lms.djangoapps.verify_student.api import send_approval_email -from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification +from lms.djangoapps.verify_student.api import ( + create_verification_attempt, + send_approval_email, + update_verification_status, +) +from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification, VerificationAttempt +from lms.djangoapps.verify_student.statuses import VerificationAttemptStatus @ddt.ddt @@ -18,6 +24,7 @@ class TestSendApprovalEmail(TestCase): """ Test cases for the send_approval_email API method. """ + def setUp(self): super().setUp() @@ -41,3 +48,77 @@ def test_send_approval(self, use_ace): with patch.dict(settings.VERIFY_STUDENT, {'USE_DJANGO_MAIL': use_ace}): send_approval_email(self.attempt) self._assert_verification_approved_email(self.attempt.expiration_datetime) + + +@ddt.ddt +class CreateVerificationAttempt(TestCase): + """ + Test cases for the send_approval_email API method. + """ + + def setUp(self): + super().setUp() + + self.user = UserFactory.create() + self.attempt = VerificationAttempt( + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.created, + expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) + ) + self.attempt.save() + + def test_create_verification_attempt(self): + expected_id = 2 + self.assertEqual( + create_verification_attempt( + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.created, + expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) + ), + expected_id + ) + verification_attempt = VerificationAttempt.objects.get(id=expected_id) + + self.assertEqual(verification_attempt.user, self.user) + self.assertEqual(verification_attempt.name, 'Tester McTest') + self.assertEqual(verification_attempt.status, VerificationAttemptStatus.created) + self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo=timezone.utc)) + + +@ddt.ddt +class UpdateVerificationStatus(TestCase): + """ + Test cases for the update_verification_status API method. + """ + + def setUp(self): + super().setUp() + + self.user= UserFactory.create() + self.attempt= VerificationAttempt( + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.created, + expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) + ) + self.attempt.save() + + @ddt.data( + VerificationAttemptStatus.pending, + VerificationAttemptStatus.approved, + VerificationAttemptStatus.denied, + ) + def test_update_verification_status(self, to_status): + update_verification_status(attempt_id=self.attempt.id, status=to_status) + + verification_attempt = VerificationAttempt.objects.get(id=self.attempt.id) + + # These are fields whose values should not change as a result of this update. + self.assertEqual(verification_attempt.user, self.user) + self.assertEqual(verification_attempt.name, 'Tester McTest') + self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo=timezone.utc)) + + # This field's value should change as a result of this update. + self.assertEqual(verification_attempt.status, to_status) From 4ec05fb53ec1961cb7d58a03d989b391d7e113e0 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Mon, 26 Aug 2024 15:28:22 -0400 Subject: [PATCH 3/9] fix: error handling for update - added tests accordingly - also took care of some nits --- lms/djangoapps/verify_student/api.py | 42 ++++++++++++--- lms/djangoapps/verify_student/exceptions.py | 3 ++ .../verify_student/tests/test_api.py | 54 ++++++++++++++++--- 3 files changed, 86 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index 034a6002f954..f83d6144a468 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -1,13 +1,19 @@ """ API module. """ +import logging + from django.conf import settings from django.utils.translation import gettext as _ from lms.djangoapps.verify_student.emails import send_verification_approved_email +from lms.djangoapps.verify_student.exceptions import VerificationAttemptInvalidStatus from lms.djangoapps.verify_student.models import VerificationAttempt +from lms.djangoapps.verify_student.statuses import VerificationAttemptStatus from lms.djangoapps.verify_student.tasks import send_verification_status_email +log = logging.getLogger(__name__) + def send_approval_email(attempt): """ @@ -40,14 +46,16 @@ def create_verification_attempt(user, name, status, expiration_datetime=None): """ Create a verification attempt. + This method is intended to be used by IDV implementation plugins to create VerificationAttempt instances. + Args: - user (User): the user (usually a learner) performing the verfication attempt - name (string): the name of the user + user (User): the user (usually a learner) performing the verification attempt + name (string): the name being ID verified status (string): the initial status of the verification attempt expiration_datetime (datetime, optional): When the verification attempt expires. Defaults to None. Returns: - VerificationAttempt (VerificationAttempt): The created VerificationAttempt instance + id (int): The id of the created VerificationAttempt instance """ verification_attempt = VerificationAttempt.objects.create( user=user, @@ -56,20 +64,40 @@ def create_verification_attempt(user, name, status, expiration_datetime=None): expiration_datetime=expiration_datetime, ) + return verification_attempt.id -def update_verification_status(attempt_id, status): +def update_verification_attempt_status(attempt_id, status): """ - Update the VerificationAttempt status. + Update the status of a verification attempt. + + This method is intended to be used by IDV implementation plugins to update VerificationAttempt instances. Arguments: - * id (str): the verification attempt id + * id (str): the verification attempt id of the attempt to update * status (str): the new status Returns: * None """ - attempt = VerificationAttempt.objects.get(id=attempt_id) + status_list = [attr for attr in dir(VerificationAttemptStatus) if not attr.startswith('__')] + + if status not in status_list: + log.error( + f'update_verification_attempt_status called with invalid status. ' + f'Status must be one of: {status_list}', + ) + raise VerificationAttemptInvalidStatus + + try: + attempt = VerificationAttempt.objects.get(id=attempt_id) + except VerificationAttempt.DoesNotExist: + log.error( + f'VerificationAttempt with id {attempt_id} was not found ' + f'when updating the attempt to status={status}', + ) + raise VerificationAttempt.DoesNotExist # pylint: disable=raise-missing-from + attempt.status = status attempt.save() diff --git a/lms/djangoapps/verify_student/exceptions.py b/lms/djangoapps/verify_student/exceptions.py index 59e7d5623f05..7b7f266a9f3a 100644 --- a/lms/djangoapps/verify_student/exceptions.py +++ b/lms/djangoapps/verify_student/exceptions.py @@ -5,3 +5,6 @@ class WindowExpiredException(Exception): pass + +class VerificationAttemptInvalidStatus(Exception): + pass diff --git a/lms/djangoapps/verify_student/tests/test_api.py b/lms/djangoapps/verify_student/tests/test_api.py index 4f5f42aedc04..28c7ced9d49a 100644 --- a/lms/djangoapps/verify_student/tests/test_api.py +++ b/lms/djangoapps/verify_student/tests/test_api.py @@ -13,8 +13,9 @@ from lms.djangoapps.verify_student.api import ( create_verification_attempt, send_approval_email, - update_verification_status, + update_verification_attempt_status, ) +from lms.djangoapps.verify_student.exceptions import VerificationAttemptInvalidStatus from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification, VerificationAttempt from lms.djangoapps.verify_student.statuses import VerificationAttemptStatus @@ -53,7 +54,7 @@ def test_send_approval(self, use_ace): @ddt.ddt class CreateVerificationAttempt(TestCase): """ - Test cases for the send_approval_email API method. + Test cases for the create_verification_attempt API method. """ def setUp(self): @@ -86,11 +87,28 @@ def test_create_verification_attempt(self): self.assertEqual(verification_attempt.status, VerificationAttemptStatus.created) self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo=timezone.utc)) + def test_create_verification_attempt_no_expiration_datetime(self): + expected_id = 2 + self.assertEqual( + create_verification_attempt( + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.created, + ), + expected_id + ) + verification_attempt = VerificationAttempt.objects.get(id=expected_id) + + self.assertEqual(verification_attempt.user, self.user) + self.assertEqual(verification_attempt.name, 'Tester McTest') + self.assertEqual(verification_attempt.status, VerificationAttemptStatus.created) + self.assertEqual(verification_attempt.expiration_datetime, None) + @ddt.ddt -class UpdateVerificationStatus(TestCase): +class UpdateVerificationAttemptStatus(TestCase): """ - Test cases for the update_verification_status API method. + Test cases for the update_verification_attempt_status API method. """ def setUp(self): @@ -110,8 +128,8 @@ def setUp(self): VerificationAttemptStatus.approved, VerificationAttemptStatus.denied, ) - def test_update_verification_status(self, to_status): - update_verification_status(attempt_id=self.attempt.id, status=to_status) + def test_update_verification_attempt_status(self, to_status): + update_verification_attempt_status(attempt_id=self.attempt.id, status=to_status) verification_attempt = VerificationAttempt.objects.get(id=self.attempt.id) @@ -122,3 +140,27 @@ def test_update_verification_status(self, to_status): # This field's value should change as a result of this update. self.assertEqual(verification_attempt.status, to_status) + + # These are statuses used in edx-name-affirmation's VerifiedName model and persona-integration's unique + # VerificationAttempt model, and not by verify_student's VerificationAttempt model. + @ddt.data( + 'completed', + 'failed', + 'submitted', + 'expired', + ) + def test_update_verification_attempt_status_invalid(self, to_status): + self.assertRaises( + VerificationAttemptInvalidStatus, + update_verification_attempt_status, + attempt_id=self.attempt.id, + status=to_status, + ) + + def test_update_verification_attempt_status_not_found(self): + self.assertRaises( + VerificationAttempt.DoesNotExist, + update_verification_attempt_status, + attempt_id=999999, + status=VerificationAttemptStatus.approved, + ) From 2fdac2e65961f8f678800ef25b42981273a28756 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Mon, 26 Aug 2024 15:44:06 -0400 Subject: [PATCH 4/9] chore: lint --- lms/djangoapps/verify_student/api.py | 1 - lms/djangoapps/verify_student/exceptions.py | 1 + .../verify_student/tests/test_api.py | 58 +++++++++---------- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index f83d6144a468..34a614f100f7 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -64,7 +64,6 @@ def create_verification_attempt(user, name, status, expiration_datetime=None): expiration_datetime=expiration_datetime, ) - return verification_attempt.id diff --git a/lms/djangoapps/verify_student/exceptions.py b/lms/djangoapps/verify_student/exceptions.py index 7b7f266a9f3a..d13e52d3e737 100644 --- a/lms/djangoapps/verify_student/exceptions.py +++ b/lms/djangoapps/verify_student/exceptions.py @@ -6,5 +6,6 @@ class WindowExpiredException(Exception): pass + class VerificationAttemptInvalidStatus(Exception): pass diff --git a/lms/djangoapps/verify_student/tests/test_api.py b/lms/djangoapps/verify_student/tests/test_api.py index 28c7ced9d49a..f798c15faca0 100644 --- a/lms/djangoapps/verify_student/tests/test_api.py +++ b/lms/djangoapps/verify_student/tests/test_api.py @@ -31,8 +31,8 @@ def setUp(self): self.user = UserFactory.create() self.attempt = SoftwareSecurePhotoVerification( - status="submitted", - user=self.user + status = "submitted", + user = self.user ) self.attempt.save() @@ -62,10 +62,10 @@ def setUp(self): self.user = UserFactory.create() self.attempt = VerificationAttempt( - user=self.user, - name='Tester McTest', - status=VerificationAttemptStatus.created, - expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) + user = self.user, + name = 'Tester McTest', + status = VerificationAttemptStatus.created, + expiration_datetime = datetime(2024, 12, 31, tzinfo = timezone.utc) ) self.attempt.save() @@ -73,31 +73,31 @@ def test_create_verification_attempt(self): expected_id = 2 self.assertEqual( create_verification_attempt( - user=self.user, - name='Tester McTest', - status=VerificationAttemptStatus.created, - expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) + user = self.user, + name = 'Tester McTest', + status = VerificationAttemptStatus.created, + expiration_datetime = datetime(2024, 12, 31, tzinfo = timezone.utc) ), expected_id ) - verification_attempt = VerificationAttempt.objects.get(id=expected_id) + verification_attempt = VerificationAttempt.objects.get(id = expected_id) self.assertEqual(verification_attempt.user, self.user) self.assertEqual(verification_attempt.name, 'Tester McTest') self.assertEqual(verification_attempt.status, VerificationAttemptStatus.created) - self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo=timezone.utc)) + self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo = timezone.utc)) def test_create_verification_attempt_no_expiration_datetime(self): expected_id = 2 self.assertEqual( create_verification_attempt( - user=self.user, - name='Tester McTest', - status=VerificationAttemptStatus.created, + user = self.user, + name = 'Tester McTest', + status = VerificationAttemptStatus.created, ), expected_id ) - verification_attempt = VerificationAttempt.objects.get(id=expected_id) + verification_attempt = VerificationAttempt.objects.get(id = expected_id) self.assertEqual(verification_attempt.user, self.user) self.assertEqual(verification_attempt.name, 'Tester McTest') @@ -114,12 +114,12 @@ class UpdateVerificationAttemptStatus(TestCase): def setUp(self): super().setUp() - self.user= UserFactory.create() - self.attempt= VerificationAttempt( - user=self.user, - name='Tester McTest', - status=VerificationAttemptStatus.created, - expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) + self.user = UserFactory.create() + self.attempt = VerificationAttempt( + user = self.user, + name = 'Tester McTest', + status = VerificationAttemptStatus.created, + expiration_datetime = datetime(2024, 12, 31, tzinfo = timezone.utc) ) self.attempt.save() @@ -129,14 +129,14 @@ def setUp(self): VerificationAttemptStatus.denied, ) def test_update_verification_attempt_status(self, to_status): - update_verification_attempt_status(attempt_id=self.attempt.id, status=to_status) + update_verification_attempt_status(attempt_id = self.attempt.id, status = to_status) - verification_attempt = VerificationAttempt.objects.get(id=self.attempt.id) + verification_attempt = VerificationAttempt.objects.get(id = self.attempt.id) # These are fields whose values should not change as a result of this update. self.assertEqual(verification_attempt.user, self.user) self.assertEqual(verification_attempt.name, 'Tester McTest') - self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo=timezone.utc)) + self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo = timezone.utc)) # This field's value should change as a result of this update. self.assertEqual(verification_attempt.status, to_status) @@ -153,14 +153,14 @@ def test_update_verification_attempt_status_invalid(self, to_status): self.assertRaises( VerificationAttemptInvalidStatus, update_verification_attempt_status, - attempt_id=self.attempt.id, - status=to_status, + attempt_id = self.attempt.id, + status = to_status, ) def test_update_verification_attempt_status_not_found(self): self.assertRaises( VerificationAttempt.DoesNotExist, update_verification_attempt_status, - attempt_id=999999, - status=VerificationAttemptStatus.approved, + attempt_id = 999999, + status = VerificationAttemptStatus.approved, ) From d4acad090ca143418c85cb3a53e8eb0d6e8f6369 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Mon, 26 Aug 2024 15:55:08 -0400 Subject: [PATCH 5/9] chore: lint for equals spaces --- .../verify_student/tests/test_api.py | 54 +++++++++---------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/lms/djangoapps/verify_student/tests/test_api.py b/lms/djangoapps/verify_student/tests/test_api.py index f798c15faca0..40f303f105b1 100644 --- a/lms/djangoapps/verify_student/tests/test_api.py +++ b/lms/djangoapps/verify_student/tests/test_api.py @@ -31,8 +31,8 @@ def setUp(self): self.user = UserFactory.create() self.attempt = SoftwareSecurePhotoVerification( - status = "submitted", - user = self.user + status="submitted", + user=self.user ) self.attempt.save() @@ -62,10 +62,10 @@ def setUp(self): self.user = UserFactory.create() self.attempt = VerificationAttempt( - user = self.user, - name = 'Tester McTest', - status = VerificationAttemptStatus.created, - expiration_datetime = datetime(2024, 12, 31, tzinfo = timezone.utc) + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.created, + expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) ) self.attempt.save() @@ -73,31 +73,31 @@ def test_create_verification_attempt(self): expected_id = 2 self.assertEqual( create_verification_attempt( - user = self.user, - name = 'Tester McTest', - status = VerificationAttemptStatus.created, - expiration_datetime = datetime(2024, 12, 31, tzinfo = timezone.utc) + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.created, + expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) ), expected_id ) - verification_attempt = VerificationAttempt.objects.get(id = expected_id) + verification_attempt = VerificationAttempt.objects.get(id=expected_id) self.assertEqual(verification_attempt.user, self.user) self.assertEqual(verification_attempt.name, 'Tester McTest') self.assertEqual(verification_attempt.status, VerificationAttemptStatus.created) - self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo = timezone.utc)) + self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo=timezone.utc)) def test_create_verification_attempt_no_expiration_datetime(self): expected_id = 2 self.assertEqual( create_verification_attempt( - user = self.user, - name = 'Tester McTest', - status = VerificationAttemptStatus.created, + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.created, ), expected_id ) - verification_attempt = VerificationAttempt.objects.get(id = expected_id) + verification_attempt = VerificationAttempt.objects.get(id=expected_id) self.assertEqual(verification_attempt.user, self.user) self.assertEqual(verification_attempt.name, 'Tester McTest') @@ -116,10 +116,10 @@ def setUp(self): self.user = UserFactory.create() self.attempt = VerificationAttempt( - user = self.user, - name = 'Tester McTest', - status = VerificationAttemptStatus.created, - expiration_datetime = datetime(2024, 12, 31, tzinfo = timezone.utc) + user=self.user, + name='Tester McTest', + status=VerificationAttemptStatus.created, + expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) ) self.attempt.save() @@ -129,14 +129,14 @@ def setUp(self): VerificationAttemptStatus.denied, ) def test_update_verification_attempt_status(self, to_status): - update_verification_attempt_status(attempt_id = self.attempt.id, status = to_status) + update_verification_attempt_status(attempt_id=self.attempt.id, status=to_status) - verification_attempt = VerificationAttempt.objects.get(id = self.attempt.id) + verification_attempt = VerificationAttempt.objects.get(id=self.attempt.id) # These are fields whose values should not change as a result of this update. self.assertEqual(verification_attempt.user, self.user) self.assertEqual(verification_attempt.name, 'Tester McTest') - self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo = timezone.utc)) + self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo=timezone.utc)) # This field's value should change as a result of this update. self.assertEqual(verification_attempt.status, to_status) @@ -153,14 +153,14 @@ def test_update_verification_attempt_status_invalid(self, to_status): self.assertRaises( VerificationAttemptInvalidStatus, update_verification_attempt_status, - attempt_id = self.attempt.id, - status = to_status, + attempt_id=self.attempt.id, + status=to_status, ) def test_update_verification_attempt_status_not_found(self): self.assertRaises( VerificationAttempt.DoesNotExist, update_verification_attempt_status, - attempt_id = 999999, - status = VerificationAttemptStatus.approved, + attempt_id=999999, + status=VerificationAttemptStatus.approved, ) From 8fd774a1b6505cc89d95a00915a4246da49aca8d Mon Sep 17 00:00:00 2001 From: ilee2u Date: Wed, 28 Aug 2024 14:18:41 -0400 Subject: [PATCH 6/9] feat: using generic update function instead - can now update name, status, and exp. date on generic attempts - changed tests accordingly - a few nits --- lms/djangoapps/verify_student/api.py | 41 +++++++---- .../commands/approve_id_verifications.py | 1 - lms/djangoapps/verify_student/models.py | 1 - .../verify_student/tests/test_api.py | 70 ++++++++++++------- 4 files changed, 72 insertions(+), 41 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index 34a614f100f7..9563fd9f5cb5 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -5,11 +5,11 @@ from django.conf import settings from django.utils.translation import gettext as _ +from django.core.exceptions import ValidationError from lms.djangoapps.verify_student.emails import send_verification_approved_email from lms.djangoapps.verify_student.exceptions import VerificationAttemptInvalidStatus from lms.djangoapps.verify_student.models import VerificationAttempt -from lms.djangoapps.verify_student.statuses import VerificationAttemptStatus from lms.djangoapps.verify_student.tasks import send_verification_status_email log = logging.getLogger(__name__) @@ -67,9 +67,9 @@ def create_verification_attempt(user, name, status, expiration_datetime=None): return verification_attempt.id -def update_verification_attempt_status(attempt_id, status): +def update_verification_attempt(attempt_id, name=None, status=None, expiration_datetime=None): """ - Update the status of a verification attempt. + Update a verification attempt. This method is intended to be used by IDV implementation plugins to update VerificationAttempt instances. @@ -80,15 +80,6 @@ def update_verification_attempt_status(attempt_id, status): Returns: * None """ - status_list = [attr for attr in dir(VerificationAttemptStatus) if not attr.startswith('__')] - - if status not in status_list: - log.error( - f'update_verification_attempt_status called with invalid status. ' - f'Status must be one of: {status_list}', - ) - raise VerificationAttemptInvalidStatus - try: attempt = VerificationAttempt.objects.get(id=attempt_id) except VerificationAttempt.DoesNotExist: @@ -96,7 +87,27 @@ def update_verification_attempt_status(attempt_id, status): f'VerificationAttempt with id {attempt_id} was not found ' f'when updating the attempt to status={status}', ) - raise VerificationAttempt.DoesNotExist # pylint: disable=raise-missing-from + raise + + if name is not None: + attempt.name = name - attempt.status = status - attempt.save() + if status is not None: + attempt.status = status + + if expiration_datetime is not None: + attempt.expiration_datetime = expiration_datetime + + try: + attempt.clean_fields() + attempt.save() + except ValidationError: + log.error( + 'Attempted to call update_verification_attempt called with invalid status: %(status)s. ' + 'Status must be one of: %(status_list)s', + { + 'status': status, + 'status_list': VerificationAttempt.STATUS_CHOICES, + }, + ) + raise VerificationAttemptInvalidStatus # pylint: disable=raise-missing-from diff --git a/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py b/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py index 6d505618bcb3..3a08ede0aaf6 100644 --- a/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py +++ b/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py @@ -8,7 +8,6 @@ import time from pprint import pformat -from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user, unused-import from django.core.management.base import BaseCommand, CommandError from lms.djangoapps.verify_student.api import send_approval_email diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index d85f172a7df0..903d80bf9245 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1213,5 +1213,4 @@ class VerificationAttempt(TimeStampedModel): expiration_datetime = models.DateTimeField( null=True, blank=True, - db_index=True, ) diff --git a/lms/djangoapps/verify_student/tests/test_api.py b/lms/djangoapps/verify_student/tests/test_api.py index 40f303f105b1..fb947328dad3 100644 --- a/lms/djangoapps/verify_student/tests/test_api.py +++ b/lms/djangoapps/verify_student/tests/test_api.py @@ -13,7 +13,7 @@ from lms.djangoapps.verify_student.api import ( create_verification_attempt, send_approval_email, - update_verification_attempt_status, + update_verification_attempt, ) from lms.djangoapps.verify_student.exceptions import VerificationAttemptInvalidStatus from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification, VerificationAttempt @@ -106,9 +106,9 @@ def test_create_verification_attempt_no_expiration_datetime(self): @ddt.ddt -class UpdateVerificationAttemptStatus(TestCase): +class UpdateVerificationAttempt(TestCase): """ - Test cases for the update_verification_attempt_status API method. + Test cases for the update_verification_attempt API method. """ def setUp(self): @@ -124,22 +124,50 @@ def setUp(self): self.attempt.save() @ddt.data( - VerificationAttemptStatus.pending, - VerificationAttemptStatus.approved, - VerificationAttemptStatus.denied, + ('Tester McTest', VerificationAttemptStatus.pending, datetime(2024, 12, 31, tzinfo=timezone.utc)), + ('Tester McTest2', VerificationAttemptStatus.approved, datetime(2025, 12, 31, tzinfo=timezone.utc)), + ('Tester McTest3', VerificationAttemptStatus.denied, datetime(2026, 12, 31, tzinfo=timezone.utc)), ) - def test_update_verification_attempt_status(self, to_status): - update_verification_attempt_status(attempt_id=self.attempt.id, status=to_status) + @ddt.unpack + def test_update_verification_attempt(self, name, status, expiration_datetime): + update_verification_attempt( + attempt_id=self.attempt.id, + name=name, + status=status, + expiration_datetime=expiration_datetime, + ) verification_attempt = VerificationAttempt.objects.get(id=self.attempt.id) - # These are fields whose values should not change as a result of this update. + # Values should change as a result of this update. self.assertEqual(verification_attempt.user, self.user) - self.assertEqual(verification_attempt.name, 'Tester McTest') - self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo=timezone.utc)) + self.assertEqual(verification_attempt.name, name) + self.assertEqual(verification_attempt.status, status) + self.assertEqual(verification_attempt.expiration_datetime, expiration_datetime) + + def test_update_verification_attempt_none_values(self): + update_verification_attempt( + attempt_id=self.attempt.id, + name=None, + status=None, + expiration_datetime=None, + ) + + verification_attempt = VerificationAttempt.objects.get(id=self.attempt.id) + + # Values should not change as a result of the values passed in being None. + self.assertEqual(verification_attempt.user, self.user) + self.assertEqual(verification_attempt.name, self.attempt.name) + self.assertEqual(verification_attempt.status, self.attempt.status) + self.assertEqual(verification_attempt.expiration_datetime, self.attempt.expiration_datetime) - # This field's value should change as a result of this update. - self.assertEqual(verification_attempt.status, to_status) + def test_update_verification_attempt_not_found(self): + self.assertRaises( + VerificationAttempt.DoesNotExist, + update_verification_attempt, + attempt_id=999999, + status=VerificationAttemptStatus.approved, + ) # These are statuses used in edx-name-affirmation's VerifiedName model and persona-integration's unique # VerificationAttempt model, and not by verify_student's VerificationAttempt model. @@ -149,18 +177,12 @@ def test_update_verification_attempt_status(self, to_status): 'submitted', 'expired', ) - def test_update_verification_attempt_status_invalid(self, to_status): + def test_update_verification_attempt_invalid(self, status): self.assertRaises( VerificationAttemptInvalidStatus, - update_verification_attempt_status, + update_verification_attempt, attempt_id=self.attempt.id, - status=to_status, - ) - - def test_update_verification_attempt_status_not_found(self): - self.assertRaises( - VerificationAttempt.DoesNotExist, - update_verification_attempt_status, - attempt_id=999999, - status=VerificationAttemptStatus.approved, + name=None, + status=status, + expiration_datetime=None, ) From 53b9bdc186e0ef87cd0f5aa4857fbc898c23b7eb Mon Sep 17 00:00:00 2001 From: ilee2u Date: Wed, 28 Aug 2024 14:30:52 -0400 Subject: [PATCH 7/9] chore: fix docstring args --- lms/djangoapps/verify_student/api.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index 9563fd9f5cb5..bfd5149b36e1 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -74,8 +74,10 @@ def update_verification_attempt(attempt_id, name=None, status=None, expiration_d This method is intended to be used by IDV implementation plugins to update VerificationAttempt instances. Arguments: - * id (str): the verification attempt id of the attempt to update - * status (str): the new status + * attempt_id (int): the verification attempt id of the attempt to update + * name (string, optional): the new name being ID verified + * status (string, optional): the new status of the verification attempt + * expiration_datetime (datetime, optional): The new expiration date and time Returns: * None From dd3980be251618cb493bde6f6255b7262cee35a4 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Tue, 3 Sep 2024 14:04:07 -0400 Subject: [PATCH 8/9] fix: corrected status validation - reverted to old status validation method - fixed tests accordingly --- lms/djangoapps/verify_student/api.py | 28 +++++++++---------- .../verify_student/tests/test_api.py | 2 -- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index bfd5149b36e1..44310a4b188f 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -5,11 +5,11 @@ from django.conf import settings from django.utils.translation import gettext as _ -from django.core.exceptions import ValidationError from lms.djangoapps.verify_student.emails import send_verification_approved_email from lms.djangoapps.verify_student.exceptions import VerificationAttemptInvalidStatus from lms.djangoapps.verify_student.models import VerificationAttempt +from lms.djangoapps.verify_student.statuses import VerificationAttemptStatus from lms.djangoapps.verify_student.tasks import send_verification_status_email log = logging.getLogger(__name__) @@ -97,19 +97,19 @@ def update_verification_attempt(attempt_id, name=None, status=None, expiration_d if status is not None: attempt.status = status + status_list = [attr for attr in dir(VerificationAttemptStatus) if not attr.startswith('__')] + if status not in status_list: + log.error( + 'Attempted to call update_verification_attempt called with invalid status: %(status)s. ' + 'Status must be one of: %(status_list)s', + { + 'status': status, + 'status_list': VerificationAttempt.STATUS_CHOICES, + }, + ) + raise VerificationAttemptInvalidStatus + if expiration_datetime is not None: attempt.expiration_datetime = expiration_datetime - try: - attempt.clean_fields() - attempt.save() - except ValidationError: - log.error( - 'Attempted to call update_verification_attempt called with invalid status: %(status)s. ' - 'Status must be one of: %(status_list)s', - { - 'status': status, - 'status_list': VerificationAttempt.STATUS_CHOICES, - }, - ) - raise VerificationAttemptInvalidStatus # pylint: disable=raise-missing-from + attempt.save() diff --git a/lms/djangoapps/verify_student/tests/test_api.py b/lms/djangoapps/verify_student/tests/test_api.py index fb947328dad3..af78abf42dec 100644 --- a/lms/djangoapps/verify_student/tests/test_api.py +++ b/lms/djangoapps/verify_student/tests/test_api.py @@ -169,8 +169,6 @@ def test_update_verification_attempt_not_found(self): status=VerificationAttemptStatus.approved, ) - # These are statuses used in edx-name-affirmation's VerifiedName model and persona-integration's unique - # VerificationAttempt model, and not by verify_student's VerificationAttempt model. @ddt.data( 'completed', 'failed', From ae312a52ec5dcf4d0fc7e3fd5c91c087ae409dad Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 5 Sep 2024 16:51:33 -0400 Subject: [PATCH 9/9] fix: datetime, status, and annotation fixes - expiration_datetime can be updated to None - VerificationAttemptStatus is now StrEnum - Added type annotations for api functions --- lms/djangoapps/verify_student/api.py | 22 +++++++++++++---- lms/djangoapps/verify_student/models.py | 8 +++---- lms/djangoapps/verify_student/statuses.py | 11 +++++---- .../verify_student/tests/test_api.py | 24 +++++++++---------- 4 files changed, 39 insertions(+), 26 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index 44310a4b188f..f61b90d682ff 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -4,8 +4,12 @@ import logging from django.conf import settings +from django.contrib.auth import get_user_model from django.utils.translation import gettext as _ +from datetime import datetime +from typing import Optional + from lms.djangoapps.verify_student.emails import send_verification_approved_email from lms.djangoapps.verify_student.exceptions import VerificationAttemptInvalidStatus from lms.djangoapps.verify_student.models import VerificationAttempt @@ -14,6 +18,8 @@ log = logging.getLogger(__name__) +User = get_user_model() + def send_approval_email(attempt): """ @@ -42,7 +48,7 @@ def send_approval_email(attempt): send_verification_approved_email(context=email_context) -def create_verification_attempt(user, name, status, expiration_datetime=None): +def create_verification_attempt(user: User, name: str, status: str, expiration_datetime: Optional[datetime] = None): """ Create a verification attempt. @@ -67,7 +73,12 @@ def create_verification_attempt(user, name, status, expiration_datetime=None): return verification_attempt.id -def update_verification_attempt(attempt_id, name=None, status=None, expiration_datetime=None): +def update_verification_attempt( + attempt_id: int, + name: Optional[str] = None, + status: Optional[str] = None, + expiration_datetime: Optional[datetime] = None +): """ Update a verification attempt. @@ -97,7 +108,7 @@ def update_verification_attempt(attempt_id, name=None, status=None, expiration_d if status is not None: attempt.status = status - status_list = [attr for attr in dir(VerificationAttemptStatus) if not attr.startswith('__')] + status_list = list(VerificationAttemptStatus) if status not in status_list: log.error( 'Attempted to call update_verification_attempt called with invalid status: %(status)s. ' @@ -109,7 +120,8 @@ def update_verification_attempt(attempt_id, name=None, status=None, expiration_d ) raise VerificationAttemptInvalidStatus - if expiration_datetime is not None: - attempt.expiration_datetime = expiration_datetime + # NOTE: Generally, we only set the expiration date from the time that an IDV attempt is marked approved, + # so we allow expiration_datetime to = None for other status updates (e.g. pending). + attempt.expiration_datetime = expiration_datetime attempt.save() diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 903d80bf9245..72b52d403849 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1203,10 +1203,10 @@ class VerificationAttempt(TimeStampedModel): name = models.CharField(blank=True, max_length=255) STATUS_CHOICES = [ - VerificationAttemptStatus.created, - VerificationAttemptStatus.pending, - VerificationAttemptStatus.approved, - VerificationAttemptStatus.denied, + VerificationAttemptStatus.CREATED, + VerificationAttemptStatus.PENDING, + VerificationAttemptStatus.APPROVED, + VerificationAttemptStatus.DENIED, ] status = models.CharField(max_length=64, choices=[(status, status) for status in STATUS_CHOICES]) diff --git a/lms/djangoapps/verify_student/statuses.py b/lms/djangoapps/verify_student/statuses.py index b55a9042e0f6..41ef381cfe06 100644 --- a/lms/djangoapps/verify_student/statuses.py +++ b/lms/djangoapps/verify_student/statuses.py @@ -1,21 +1,22 @@ """ Status enums for verify_student. """ +from enum import StrEnum, auto -class VerificationAttemptStatus: +class VerificationAttemptStatus(StrEnum): """This class describes valid statuses for a verification attempt to be in.""" # This is the initial state of a verification attempt, before a learner has started IDV. - created = "created" + CREATED = auto() # A verification attempt is pending when it has been started but has not yet been completed. - pending = "pending" + PENDING = auto() # A verification attempt is approved when it has been approved by some mechanism (e.g. automatic review, manual # review, etc). - approved = "approved" + APPROVED = auto() # A verification attempt is denied when it has been denied by some mechanism (e.g. automatic review, manual review, # etc). - denied = "denied" + DENIED = auto() diff --git a/lms/djangoapps/verify_student/tests/test_api.py b/lms/djangoapps/verify_student/tests/test_api.py index af78abf42dec..747c76f82b61 100644 --- a/lms/djangoapps/verify_student/tests/test_api.py +++ b/lms/djangoapps/verify_student/tests/test_api.py @@ -64,7 +64,7 @@ def setUp(self): self.attempt = VerificationAttempt( user=self.user, name='Tester McTest', - status=VerificationAttemptStatus.created, + status=VerificationAttemptStatus.CREATED, expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) ) self.attempt.save() @@ -75,7 +75,7 @@ def test_create_verification_attempt(self): create_verification_attempt( user=self.user, name='Tester McTest', - status=VerificationAttemptStatus.created, + status=VerificationAttemptStatus.CREATED, expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) ), expected_id @@ -84,7 +84,7 @@ def test_create_verification_attempt(self): self.assertEqual(verification_attempt.user, self.user) self.assertEqual(verification_attempt.name, 'Tester McTest') - self.assertEqual(verification_attempt.status, VerificationAttemptStatus.created) + self.assertEqual(verification_attempt.status, VerificationAttemptStatus.CREATED) self.assertEqual(verification_attempt.expiration_datetime, datetime(2024, 12, 31, tzinfo=timezone.utc)) def test_create_verification_attempt_no_expiration_datetime(self): @@ -93,7 +93,7 @@ def test_create_verification_attempt_no_expiration_datetime(self): create_verification_attempt( user=self.user, name='Tester McTest', - status=VerificationAttemptStatus.created, + status=VerificationAttemptStatus.CREATED, ), expected_id ) @@ -101,7 +101,7 @@ def test_create_verification_attempt_no_expiration_datetime(self): self.assertEqual(verification_attempt.user, self.user) self.assertEqual(verification_attempt.name, 'Tester McTest') - self.assertEqual(verification_attempt.status, VerificationAttemptStatus.created) + self.assertEqual(verification_attempt.status, VerificationAttemptStatus.CREATED) self.assertEqual(verification_attempt.expiration_datetime, None) @@ -118,15 +118,15 @@ def setUp(self): self.attempt = VerificationAttempt( user=self.user, name='Tester McTest', - status=VerificationAttemptStatus.created, + status=VerificationAttemptStatus.CREATED, expiration_datetime=datetime(2024, 12, 31, tzinfo=timezone.utc) ) self.attempt.save() @ddt.data( - ('Tester McTest', VerificationAttemptStatus.pending, datetime(2024, 12, 31, tzinfo=timezone.utc)), - ('Tester McTest2', VerificationAttemptStatus.approved, datetime(2025, 12, 31, tzinfo=timezone.utc)), - ('Tester McTest3', VerificationAttemptStatus.denied, datetime(2026, 12, 31, tzinfo=timezone.utc)), + ('Tester McTest', VerificationAttemptStatus.PENDING, datetime(2024, 12, 31, tzinfo=timezone.utc)), + ('Tester McTest2', VerificationAttemptStatus.APPROVED, datetime(2025, 12, 31, tzinfo=timezone.utc)), + ('Tester McTest3', VerificationAttemptStatus.DENIED, datetime(2026, 12, 31, tzinfo=timezone.utc)), ) @ddt.unpack def test_update_verification_attempt(self, name, status, expiration_datetime): @@ -155,18 +155,18 @@ def test_update_verification_attempt_none_values(self): verification_attempt = VerificationAttempt.objects.get(id=self.attempt.id) - # Values should not change as a result of the values passed in being None. + # Values should not change as a result of the values passed in being None, except for expiration_datetime. self.assertEqual(verification_attempt.user, self.user) self.assertEqual(verification_attempt.name, self.attempt.name) self.assertEqual(verification_attempt.status, self.attempt.status) - self.assertEqual(verification_attempt.expiration_datetime, self.attempt.expiration_datetime) + self.assertEqual(verification_attempt.expiration_datetime, None) def test_update_verification_attempt_not_found(self): self.assertRaises( VerificationAttempt.DoesNotExist, update_verification_attempt, attempt_id=999999, - status=VerificationAttemptStatus.approved, + status=VerificationAttemptStatus.APPROVED, ) @ddt.data(