From 23c52423c497ee8a5d006434968cfad5f2d37168 Mon Sep 17 00:00:00 2001 From: Zacharis278 Date: Tue, 13 Aug 2024 15:58:33 -0400 Subject: [PATCH 1/3] feat: add new VerificationAttempt model to idv logic --- lms/djangoapps/verify_student/models.py | 10 ++++ lms/djangoapps/verify_student/services.py | 18 +++++-- .../verify_student/tests/test_services.py | 54 +++++++++++++++---- 3 files changed, 70 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 903d80bf9245..6dadd14634e4 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1214,3 +1214,13 @@ class VerificationAttempt(TimeStampedModel): null=True, blank=True, ) + + @property + def expiration_datetime(self): + """Backwards compatibility with existing IDVerification models""" + return self.expiration_date + + @property + def updated_at(self): + """Backwards compatibility with existing IDVerification models""" + return self.modified diff --git a/lms/djangoapps/verify_student/services.py b/lms/djangoapps/verify_student/services.py index bdfa31fee6d6..5b88025719ca 100644 --- a/lms/djangoapps/verify_student/services.py +++ b/lms/djangoapps/verify_student/services.py @@ -17,7 +17,7 @@ from lms.djangoapps.verify_student.utils import is_verification_expiring_soon from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers -from .models import ManualVerification, SoftwareSecurePhotoVerification, SSOVerification +from .models import ManualVerification, SoftwareSecurePhotoVerification, SSOVerification, VerificationAttempt from .utils import most_recent_verification log = logging.getLogger(__name__) @@ -75,7 +75,8 @@ def verifications_for_user(cls, user): Return a list of all verifications associated with the given user. """ verifications = [] - for verification in chain(SoftwareSecurePhotoVerification.objects.filter(user=user).order_by('-created_at'), + for verification in chain(VerificationAttempt.objects.filter(user=user).order_by('-created'), + SoftwareSecurePhotoVerification.objects.filter(user=user).order_by('-created_at'), SSOVerification.objects.filter(user=user).order_by('-created_at'), ManualVerification.objects.filter(user=user).order_by('-created_at')): verifications.append(verification) @@ -92,6 +93,11 @@ def get_verified_user_ids(cls, users): 'created_at__gt': now() - timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) } return chain( + VerificationAttempt.objects.filter(**{ + 'user__in': users, + 'status': 'approved', + 'created__gt': now() - timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + }).values_list('user_id', flat=True), SoftwareSecurePhotoVerification.objects.filter(**filter_kwargs).values_list('user_id', flat=True), SSOVerification.objects.filter(**filter_kwargs).values_list('user_id', flat=True), ManualVerification.objects.filter(**filter_kwargs).values_list('user_id', flat=True) @@ -117,11 +123,14 @@ def get_expiration_datetime(cls, user, statuses): 'status__in': statuses, } + id_verifications = VerificationAttempt.objects.filter(**filter_kwargs) photo_id_verifications = SoftwareSecurePhotoVerification.objects.filter(**filter_kwargs) sso_id_verifications = SSOVerification.objects.filter(**filter_kwargs) manual_id_verifications = ManualVerification.objects.filter(**filter_kwargs) - attempt = most_recent_verification((photo_id_verifications, sso_id_verifications, manual_id_verifications)) + attempt = most_recent_verification( + (photo_id_verifications, sso_id_verifications, manual_id_verifications, id_verifications) + ) return attempt and attempt.expiration_datetime @classmethod @@ -244,6 +253,9 @@ def get_verification_details_by_id(cls, attempt_id): If the verification object cannot be found, returns None """ verification = None + + # TODO: Not updated for new model since we can't query multiple tables + # by id. verification_models = [ SoftwareSecurePhotoVerification, SSOVerification, diff --git a/lms/djangoapps/verify_student/tests/test_services.py b/lms/djangoapps/verify_student/tests/test_services.py index 56f388b7c97e..19c85d9149ab 100644 --- a/lms/djangoapps/verify_student/tests/test_services.py +++ b/lms/djangoapps/verify_student/tests/test_services.py @@ -2,8 +2,8 @@ Tests for the service classes in verify_student. """ -from datetime import datetime, timedelta, timezone import itertools +from datetime import datetime, timedelta, timezone from random import randint from unittest.mock import patch @@ -16,10 +16,16 @@ from pytz import utc from common.djangoapps.student.tests.factories import UserFactory -from lms.djangoapps.verify_student.models import ManualVerification, SoftwareSecurePhotoVerification, SSOVerification +from lms.djangoapps.verify_student.models import ( + ManualVerification, + SoftwareSecurePhotoVerification, + SSOVerification, + VerificationAttempt +) from lms.djangoapps.verify_student.services import IDVerificationService from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.modulestore.tests.django_utils import \ + ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order FAKE_SETTINGS = { @@ -34,12 +40,15 @@ class TestIDVerificationService(ModuleStoreTestCase): Tests for IDVerificationService. """ - def test_user_is_verified(self): + @ddt.data( + SoftwareSecurePhotoVerification, VerificationAttempt + ) + def test_user_is_verified(self, verification_model): """ Test to make sure we correctly answer whether a user has been verified. """ user = UserFactory.create() - attempt = SoftwareSecurePhotoVerification(user=user) + attempt = verification_model(user=user) attempt.save() # If it's any of these, they're not verified... @@ -49,16 +58,21 @@ def test_user_is_verified(self): assert not IDVerificationService.user_is_verified(user), status attempt.status = "approved" + attempt.expiration_date = now() + timedelta(days=19) attempt.save() + assert IDVerificationService.user_is_verified(user), attempt.status - def test_user_has_valid_or_pending(self): + @ddt.data( + SoftwareSecurePhotoVerification, VerificationAttempt + ) + def test_user_has_valid_or_pending(self, verification_model): """ Determine whether we have to prompt this user to verify, or if they've already at least initiated a verification submission. """ user = UserFactory.create() - attempt = SoftwareSecurePhotoVerification(user=user) + attempt = verification_model(user=user) # If it's any of these statuses, they don't have anything outstanding for status in ["created", "ready", "denied"]: @@ -70,6 +84,7 @@ def test_user_has_valid_or_pending(self): # -- must_retry, and submitted both count until we hear otherwise for status in ["submitted", "must_retry", "approved"]: attempt.status = status + attempt.expiration_date = now() + timedelta(days=19) attempt.save() assert IDVerificationService.user_has_valid_or_pending(user), status @@ -102,18 +117,22 @@ def test_get_verified_user_ids(self): user_a = UserFactory.create() user_b = UserFactory.create() user_c = UserFactory.create() + user_d = UserFactory.create() user_unverified = UserFactory.create() user_denied = UserFactory.create() + user_denied_b = UserFactory.create() SoftwareSecurePhotoVerification.objects.create(user=user_a, status='approved') ManualVerification.objects.create(user=user_b, status='approved') SSOVerification.objects.create(user=user_c, status='approved') + VerificationAttempt.objects.create(user=user_d, status='approved') SSOVerification.objects.create(user=user_denied, status='denied') + VerificationAttempt.objects.create(user=user_denied_b, status='denied') verified_user_ids = set(IDVerificationService.get_verified_user_ids([ - user_a, user_b, user_c, user_unverified, user_denied + user_a, user_b, user_c, user_d, user_unverified, user_denied ])) - expected_user_ids = {user_a.id, user_b.id, user_c.id} + expected_user_ids = {user_a.id, user_b.id, user_c.id, user_d.id} assert expected_user_ids == verified_user_ids @@ -158,6 +177,23 @@ def test_get_expiration_datetime(self): expiration_datetime = IDVerificationService.get_expiration_datetime(user_a, ['approved']) assert expiration_datetime == newer_record.expiration_datetime + def test_get_expiration_datetime_mixed_models(self): + """ + Test that the latest expiration datetime is returned if there are both instances of + IDVerification models and VerificationAttempt models + """ + user = UserFactory.create() + + SoftwareSecurePhotoVerification.objects.create( + user=user, status='approved', expiration_date=datetime(2021, 11, 12, 0, 0, tzinfo=timezone.utc) + ) + newest = VerificationAttempt.objects.create( + user=user, status='approved', expiration_date=datetime(2022, 1, 12, 0, 0, tzinfo=timezone.utc) + ) + + expiration_datetime = IDVerificationService.get_expiration_datetime(user, ['approved']) + assert expiration_datetime == newest.expiration_datetime + @ddt.data( {'status': 'denied', 'error_msg': '[{"generalReasons": ["Name mismatch"]}]'}, {'status': 'approved', 'error_msg': ''}, From f7ddb7da951797130bb934af2f677151b71e7aaf Mon Sep 17 00:00:00 2001 From: Zacharis278 Date: Mon, 9 Sep 2024 16:19:51 -0400 Subject: [PATCH 2/3] feat: comment on to-be-removed function --- lms/djangoapps/verify_student/models.py | 5 ----- lms/djangoapps/verify_student/services.py | 11 +++++++++-- lms/djangoapps/verify_student/tests/test_services.py | 12 +++++++++--- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 6dadd14634e4..e0b6c7643c8e 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1215,11 +1215,6 @@ class VerificationAttempt(TimeStampedModel): blank=True, ) - @property - def expiration_datetime(self): - """Backwards compatibility with existing IDVerification models""" - return self.expiration_date - @property def updated_at(self): """Backwards compatibility with existing IDVerification models""" diff --git a/lms/djangoapps/verify_student/services.py b/lms/djangoapps/verify_student/services.py index 5b88025719ca..2a5f77e3fbfb 100644 --- a/lms/djangoapps/verify_student/services.py +++ b/lms/djangoapps/verify_student/services.py @@ -251,11 +251,18 @@ def get_verification_details_by_id(cls, attempt_id): """ Returns a verification attempt object by attempt_id If the verification object cannot be found, returns None + + This method does not take into account verifications stored in the + VerificationAttempt model used for pluggable IDV implementations. + + As part of the work to implement pluggable IDV, this method's use + will be deprecated: DEPR-XXX """ verification = None - # TODO: Not updated for new model since we can't query multiple tables - # by id. + # This does not look at the VerificationAttempt model since the provided id would become + # ambiguous between tables. The verification models in this list all inherit from the same + # base class and share the same id space. verification_models = [ SoftwareSecurePhotoVerification, SSOVerification, diff --git a/lms/djangoapps/verify_student/tests/test_services.py b/lms/djangoapps/verify_student/tests/test_services.py index 19c85d9149ab..5351e3ede699 100644 --- a/lms/djangoapps/verify_student/tests/test_services.py +++ b/lms/djangoapps/verify_student/tests/test_services.py @@ -58,7 +58,10 @@ def test_user_is_verified(self, verification_model): assert not IDVerificationService.user_is_verified(user), status attempt.status = "approved" - attempt.expiration_date = now() + timedelta(days=19) + if verification_model == VerificationAttempt: + attempt.expiration_datetime = now() + timedelta(days=19) + else: + attempt.expiration_date = now() + timedelta(days=19) attempt.save() assert IDVerificationService.user_is_verified(user), attempt.status @@ -84,7 +87,10 @@ def test_user_has_valid_or_pending(self, verification_model): # -- must_retry, and submitted both count until we hear otherwise for status in ["submitted", "must_retry", "approved"]: attempt.status = status - attempt.expiration_date = now() + timedelta(days=19) + if verification_model == VerificationAttempt: + attempt.expiration_datetime = now() + timedelta(days=19) + else: + attempt.expiration_date = now() + timedelta(days=19) attempt.save() assert IDVerificationService.user_has_valid_or_pending(user), status @@ -188,7 +194,7 @@ def test_get_expiration_datetime_mixed_models(self): user=user, status='approved', expiration_date=datetime(2021, 11, 12, 0, 0, tzinfo=timezone.utc) ) newest = VerificationAttempt.objects.create( - user=user, status='approved', expiration_date=datetime(2022, 1, 12, 0, 0, tzinfo=timezone.utc) + user=user, status='approved', expiration_datetime=datetime(2022, 1, 12, 0, 0, tzinfo=timezone.utc) ) expiration_datetime = IDVerificationService.get_expiration_datetime(user, ['approved']) From 8b9d92b905be10d397cb437dfa19d0b018ad08ec Mon Sep 17 00:00:00 2001 From: Zacharis278 Date: Tue, 10 Sep 2024 09:28:34 -0400 Subject: [PATCH 3/3] test: fix failing test --- lms/djangoapps/instructor_task/tests/test_tasks_helper.py | 2 +- lms/djangoapps/verify_student/services.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 8fa590c37f8c..1fb25aeb8c07 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -406,7 +406,7 @@ def test_query_counts(self): with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): with check_mongo_calls(2): - with self.assertNumQueries(53): + with self.assertNumQueries(54): CourseGradeReport.generate(None, None, course.id, {}, 'graded') def test_inactive_enrollments(self): diff --git a/lms/djangoapps/verify_student/services.py b/lms/djangoapps/verify_student/services.py index 2a5f77e3fbfb..f1c5543e8536 100644 --- a/lms/djangoapps/verify_student/services.py +++ b/lms/djangoapps/verify_student/services.py @@ -256,7 +256,7 @@ def get_verification_details_by_id(cls, attempt_id): VerificationAttempt model used for pluggable IDV implementations. As part of the work to implement pluggable IDV, this method's use - will be deprecated: DEPR-XXX + will be deprecated: https://openedx.atlassian.net/browse/OSPR-1011 """ verification = None