From c0d788a7ff87ea321a39f28c921c0143467577a4 Mon Sep 17 00:00:00 2001 From: michaelroytman Date: Mon, 7 Oct 2024 12:38:21 -0400 Subject: [PATCH] fix: VerificationAttempt.expiration_datetime field may be None. This commit fixes a bug introduced by the new VerificationAttempt model. The expiration_datetime field is nullable. Other IDV models in the verify_student application have a default value for expiration_date, so they are typically not null, despite being nullable. This commit updates code that queries this field to function correctly when the value of expiration_datetime is None. In this case, a None expiration_datetime indicates a non-expiring attempt. --- lms/djangoapps/verify_student/models.py | 16 +++ lms/djangoapps/verify_student/services.py | 32 +++++- .../verify_student/tests/__init__.py | 11 +- .../verify_student/tests/test_models.py | 12 ++ .../verify_student/tests/test_services.py | 105 ++++++++++++------ 5 files changed, 132 insertions(+), 44 deletions(-) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index ae00ca8a1c57..7d055ffc70ab 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1229,6 +1229,22 @@ def should_display_status_to_user(self): """When called, returns true or false based on the type of VerificationAttempt""" return not self.hide_status_from_user + def active_at_datetime(self, deadline): + """Check whether the verification was active at a particular datetime. + + Arguments: + deadline (datetime): The date at which the verification was active + (created before and expiration datetime is after today). + + Returns: + bool + + """ + return ( + self.created_at <= deadline and + (self.expiration_datetime is None or self.expiration_datetime > now()) + ) + @classmethod def retire_user(cls, user_id): """ diff --git a/lms/djangoapps/verify_student/services.py b/lms/djangoapps/verify_student/services.py index bee016de7740..7b1d6d9df75e 100644 --- a/lms/djangoapps/verify_student/services.py +++ b/lms/djangoapps/verify_student/services.py @@ -173,13 +173,35 @@ def user_status(cls, user): verifications = cls.verifications_for_user(user) + # Get the first verification with an expiration date. + with_expiration_index = next( + (index for index, verification in enumerate(verifications) if verification.expiration_datetime), + None + ) + if verifications: attempt = verifications[0] for verification in verifications: - if verification.expiration_datetime > now() and verification.status == 'approved': - # Always select the LATEST non-expired approved verification if there is such - if attempt.status != 'approved' or ( - attempt.expiration_datetime < verification.expiration_datetime + + # # A verification may not have an expiration date. If it doesn't, ignore it for the purpose of + # # determining the latest non-expired approved verification. + # if not verification.expiration_datetime: + # continue + + # If a verification has no expiration_datetime, it's implied that it never expires, so we should still + # consider verifications in the approved state that have no expiration date. + if ( + not verification.expiration_datetime or + verification.expiration_datetime > now() + ) and verification.status == 'approved': + # Always select the LATEST non-expired approved verification if there is such. + if ( + attempt.status != 'approved' or + ( + attempt.expiration_datetime and + verification.expiration_datetime and + attempt.expiration_datetime < verification.expiration_datetime + ) ): attempt = verification @@ -188,7 +210,7 @@ def user_status(cls, user): user_status['should_display'] = attempt.should_display_status_to_user() - if attempt.expiration_datetime < now() and attempt.status == 'approved': + if attempt.expiration_datetime and attempt.expiration_datetime < now() and attempt.status == 'approved': if user_status['should_display']: user_status['status'] = 'expired' user_status['error'] = _("Your {platform_name} verification has expired.").format( diff --git a/lms/djangoapps/verify_student/tests/__init__.py b/lms/djangoapps/verify_student/tests/__init__.py index cea71e52c37d..015cd354aba7 100644 --- a/lms/djangoapps/verify_student/tests/__init__.py +++ b/lms/djangoapps/verify_student/tests/__init__.py @@ -9,7 +9,7 @@ from django.utils.timezone import now from common.djangoapps.student.tests.factories import UserFactory -from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification +from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification, VerificationAttempt class TestVerificationBase(TestCase): @@ -52,11 +52,14 @@ def verification_active_at_datetime(self, attempt): # Active immediately before expiration date expiration = attempt.expiration_datetime - before_expiration = expiration - timedelta(seconds=1) - assert attempt.active_at_datetime(before_expiration) + if expiration: + before_expiration = expiration - timedelta(seconds=1) + assert attempt.active_at_datetime(before_expiration) # Not active after the expiration date - attempt.expiration_date = now() - timedelta(days=1) + field = 'expiration_datetime' if isinstance(attempt, VerificationAttempt) else 'expiration_date' + setattr(attempt, field, now() - timedelta(days=1)) + # attempt.expiration_date = now() - timedelta(days=1) attempt.save() assert not attempt.active_at_datetime(now()) diff --git a/lms/djangoapps/verify_student/tests/test_models.py b/lms/djangoapps/verify_student/tests/test_models.py index 9ea45463011d..dfea8081e4d4 100644 --- a/lms/djangoapps/verify_student/tests/test_models.py +++ b/lms/djangoapps/verify_student/tests/test_models.py @@ -20,6 +20,7 @@ PhotoVerification, SoftwareSecurePhotoVerification, SSOVerification, + VerificationAttempt, VerificationException ) from lms.djangoapps.verify_student.tests import TestVerificationBase @@ -437,3 +438,14 @@ def test_active_at_datetime(self): user = UserFactory.create() verification = ManualVerification.objects.create(user=user) self.verification_active_at_datetime(verification) + + +class VerificationAttemptTest(TestVerificationBase): + """ + Tests for the VerificationAttempt model + """ + + def test_active_at_datetime(self): + user = UserFactory.create() + attempt = VerificationAttempt.objects.create(user=user) + self.verification_active_at_datetime(attempt) diff --git a/lms/djangoapps/verify_student/tests/test_services.py b/lms/djangoapps/verify_student/tests/test_services.py index 5b6d740a1ba0..4bfa8c27d5b3 100644 --- a/lms/djangoapps/verify_student/tests/test_services.py +++ b/lms/djangoapps/verify_student/tests/test_services.py @@ -2,6 +2,7 @@ Tests for the service classes in verify_student. """ +import itertools from datetime import datetime, timedelta, timezone from unittest.mock import patch @@ -273,6 +274,29 @@ def test_denied_software_secure_verification(self): } self.assertDictEqual(status, expected_status) + def test_approved_verification_attempt_verification(self): + with freeze_time('2015-01-02'): + # test for when Verification Attempt verification has been created + VerificationAttempt.objects.create(user=self.user, status='approved') + status = IDVerificationService.user_status(self.user) + expected_status = {'status': 'approved', 'error': '', 'should_display': True, 'verification_expiry': '', + 'status_date': datetime.now(utc)} + self.assertDictEqual(status, expected_status) + + def test_denied_verification_attempt_verification(self): + with freeze_time('2015-2-02'): + # create denied photo verification for the user, make sure the denial + # is handled properly + VerificationAttempt.objects.create( + user=self.user, status='denied' + ) + status = IDVerificationService.user_status(self.user) + expected_status = { + 'status': 'must_reverify', 'error': '', + 'should_display': True, 'verification_expiry': '', 'status_date': '', + } + self.assertDictEqual(status, expected_status) + def test_approved_sso_verification(self): with freeze_time('2015-03-02'): # test for when sso verification has been created @@ -303,22 +327,19 @@ def test_manual_verification(self): 'status_date': datetime.now(utc)} self.assertDictEqual(status, expected_status) - @ddt.data( - 'submitted', - 'denied', - 'approved', - 'created', - 'ready', - 'must_retry' + @ddt.idata(itertools.product( + [SoftwareSecurePhotoVerification, VerificationAttempt], + ['submitted', 'denied', 'approved', 'created', 'ready', 'must_retry']) ) - def test_expiring_software_secure_verification(self, new_status): + def test_expiring_software_secure_verification(self, value): + verification_model, new_status = value with freeze_time('2015-07-11') as frozen_datetime: # create approved photo verification for the user - SoftwareSecurePhotoVerification.objects.create(user=self.user, status='approved') + verification_model.objects.create(user=self.user, status='approved') expiring_datetime = datetime.now(utc) frozen_datetime.move_to('2015-07-14') # create another according to status passed in. - SoftwareSecurePhotoVerification.objects.create(user=self.user, status=new_status) + verification_model.objects.create(user=self.user, status=new_status) status_date = expiring_datetime if new_status == 'approved': status_date = datetime.now(utc) @@ -327,13 +348,16 @@ def test_expiring_software_secure_verification(self, new_status): status = IDVerificationService.user_status(self.user) self.assertDictEqual(status, expected_status) - def test_expired_verification(self): + @ddt.data(SoftwareSecurePhotoVerification, VerificationAttempt) + def test_expired_verification(self, verification_model): with freeze_time('2015-07-11') as frozen_datetime: - # create approved photo verification for the user - SoftwareSecurePhotoVerification.objects.create( + key = 'expiration_datetime' if verification_model == VerificationAttempt else 'expiration_date' + + # create approved verification for the user + verification_model.objects.create( user=self.user, status='approved', - expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + **{key: now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"])}, ) frozen_datetime.move_to('2016-07-11') expected_status = { @@ -348,28 +372,28 @@ def test_expired_verification(self): status = IDVerificationService.user_status(self.user) self.assertDictEqual(status, expected_status) - @ddt.data( - 'submitted', - 'denied', - 'approved', - 'created', - 'ready', - 'must_retry' + @ddt.idata(itertools.product( + [SoftwareSecurePhotoVerification, VerificationAttempt], + ['submitted', 'denied', 'approved', 'created', 'ready', 'must_retry']) ) - def test_reverify_after_expired(self, new_status): + def test_reverify_after_expired(self, value): + verification_model, new_status = value with freeze_time('2015-07-11') as frozen_datetime: - # create approved photo verification for the user - SoftwareSecurePhotoVerification.objects.create( + key = 'expiration_datetime' if verification_model == VerificationAttempt else 'expiration_date' + + # create approved verification for the user + verification_model.objects.create( user=self.user, status='approved', - expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + **{key: now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"])}, ) + frozen_datetime.move_to('2016-07-12') # create another according to status passed in. - SoftwareSecurePhotoVerification.objects.create( + verification_model.objects.create( user=self.user, status=new_status, - expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + **{key: now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"])}, ) check_status = new_status @@ -390,9 +414,10 @@ def test_reverify_after_expired(self, new_status): @ddt.data( SSOVerification, - ManualVerification + ManualVerification, + VerificationAttempt ) - def test_override_verification(self, verification_type): + def test_override_verification(self, verification_model): with freeze_time('2015-07-11') as frozen_datetime: # create approved photo verification for the user SoftwareSecurePhotoVerification.objects.create( @@ -401,19 +426,27 @@ def test_override_verification(self, verification_type): expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) ) frozen_datetime.move_to('2015-07-14') - verification_type.objects.create( + + key = 'expiration_datetime' if verification_model == VerificationAttempt else 'expiration_date' + verification_model.objects.create( user=self.user, status='approved', - expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + **{key: now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"])}, ) expected_status = { - 'status': 'approved', 'error': '', 'should_display': False, + 'status': 'approved', 'error': '', + 'should_display': verification_model == VerificationAttempt, 'verification_expiry': '', 'status_date': now() } status = IDVerificationService.user_status(self.user) self.assertDictEqual(status, expected_status) - def test_denied_after_approved_verification(self): + @ddt.data( + SSOVerification, + ManualVerification, + VerificationAttempt + ) + def test_denied_after_approved_verification(self, verification_model): with freeze_time('2015-07-11') as frozen_datetime: # create approved photo verification for the user SoftwareSecurePhotoVerification.objects.create( @@ -423,10 +456,12 @@ def test_denied_after_approved_verification(self): ) expected_date = now() frozen_datetime.move_to('2015-07-14') - SoftwareSecurePhotoVerification.objects.create( + + key = 'expiration_datetime' if verification_model == VerificationAttempt else 'expiration_date' + verification_model.objects.create( user=self.user, status='denied', - expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + **{key: now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"])}, ) expected_status = { 'status': 'approved', 'error': '', 'should_display': True,