From cd380e511886bdbc05e6d57942a4178baecaa487 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 19 Sep 2024 11:38:53 -0400 Subject: [PATCH 01/18] fix: add placeholder should_display_status_to_user --- lms/djangoapps/verify_student/models.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 23729c99a0b9..9df7fc4bf38d 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1251,3 +1251,8 @@ def retire_user(cls, user_id): """ verification_attempts = cls.objects.filter(user_id=user_id) verification_attempts.delete() + + @classmethod + def should_display_status_to_user(cls): + """Temporary placeholder so that calls to this method do not break edx-platform""" + return From 790ee4935899f6fbecec5a2d1938c2d097ffe703 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 19 Sep 2024 12:09:24 -0400 Subject: [PATCH 02/18] fix: have VerificationAttempt inherit StatusModel - should_display_status_to_user now returns False --- lms/djangoapps/verify_student/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 9df7fc4bf38d..383855f7e3da 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1214,7 +1214,7 @@ def __str__(self): return str(self.arguments) -class VerificationAttempt(TimeStampedModel): +class VerificationAttempt(TimeStampedModel, StatusModel): """ The model represents impelementation-agnostic information about identity verification (IDV) attempts. @@ -1255,4 +1255,4 @@ def retire_user(cls, user_id): @classmethod def should_display_status_to_user(cls): """Temporary placeholder so that calls to this method do not break edx-platform""" - return + return False From 433412067f4e5b2ba7b75cfc813d403af4978c3b Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 19 Sep 2024 12:17:43 -0400 Subject: [PATCH 03/18] chore: makemigrations --- ...0016_verificationattempt_status_changed.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 lms/djangoapps/verify_student/migrations/0016_verificationattempt_status_changed.py diff --git a/lms/djangoapps/verify_student/migrations/0016_verificationattempt_status_changed.py b/lms/djangoapps/verify_student/migrations/0016_verificationattempt_status_changed.py new file mode 100644 index 000000000000..d5aba6fd369d --- /dev/null +++ b/lms/djangoapps/verify_student/migrations/0016_verificationattempt_status_changed.py @@ -0,0 +1,20 @@ +# Generated by Django 4.2.15 on 2024-09-19 16:17 + +from django.db import migrations +import django.utils.timezone +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('verify_student', '0015_verificationattempt'), + ] + + operations = [ + migrations.AddField( + model_name='verificationattempt', + name='status_changed', + field=model_utils.fields.MonitorField(default=django.utils.timezone.now, monitor='status', verbose_name='status changed'), + ), + ] From 1c5ed0c3a6b46c902354ae11c48206bc232c8eb0 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Tue, 24 Sep 2024 16:43:20 -0400 Subject: [PATCH 04/18] feat: status_changed field added --- lms/djangoapps/verify_student/models.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 383855f7e3da..4286d95718f0 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1237,6 +1237,11 @@ class VerificationAttempt(TimeStampedModel, StatusModel): blank=True, ) + @property + def status_changed(self): + """Named as such for backwards compatibility with existing IDVerification models""" + return self.created + @property def updated_at(self): """Backwards compatibility with existing IDVerification models""" @@ -1254,5 +1259,5 @@ def retire_user(cls, user_id): @classmethod def should_display_status_to_user(cls): - """Temporary placeholder so that calls to this method do not break edx-platform""" + """When called, returns true or false based on the type of VerificationAttempt""" return False From b7e6211c2c43482dfb1a078febbddaf95cfa54db Mon Sep 17 00:00:00 2001 From: ilee2u Date: Tue, 24 Sep 2024 16:56:21 -0400 Subject: [PATCH 05/18] temp: idea to add should_display_status_to_user --- common/djangoapps/student/helpers.py | 3 ++- lms/djangoapps/verify_student/models.py | 12 ++++++++++-- lms/djangoapps/verify_student/services.py | 3 ++- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index fb0a7236c0b5..b5c08fc050eb 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -168,7 +168,8 @@ def check_verify_status_by_course(user, course_enrollments): # Check whether the user was approved or is awaiting approval if relevant_verification is not None: - should_display = relevant_verification.should_display_status_to_user() + idv_type = relevant_verification.__name__ + should_display = relevant_verification.should_display_status_to_user(idv_type=idv_type) if relevant_verification.status == "approved": if verification_expiring_soon: diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 4286d95718f0..f68d7e55735f 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1258,6 +1258,14 @@ def retire_user(cls, user_id): verification_attempts.delete() @classmethod - def should_display_status_to_user(cls): + def should_display_status_to_user(cls, idv_type): """When called, returns true or false based on the type of VerificationAttempt""" - return False + # NOTE: Do we also need another one here for persona + if idv_type == 'VerificationAttempt': + return False # Not sure what to return for this one. + elif idv_type == 'SoftwareSecurePhotoVerification': + return True + elif idv_type == 'SSOVerification': + return False + elif idv_type == 'ManualVerification': + return False diff --git a/lms/djangoapps/verify_student/services.py b/lms/djangoapps/verify_student/services.py index 1a2d145e892a..3381a3afcd61 100644 --- a/lms/djangoapps/verify_student/services.py +++ b/lms/djangoapps/verify_student/services.py @@ -187,7 +187,8 @@ def user_status(cls, user): if not attempt: return user_status - user_status['should_display'] = attempt.should_display_status_to_user() + idv_type = attempt.__name__ + user_status['should_display'] = attempt.should_display_status_to_user(idv_type=idv_type) if attempt.expiration_datetime < now() and attempt.status == 'approved': if user_status['should_display']: From 2b0d5a8aa7d126740523df4f55d92cfeb429ad73 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Wed, 25 Sep 2024 15:55:32 -0400 Subject: [PATCH 06/18] feat: add should_display_status_to_user --- lms/djangoapps/verify_student/api.py | 10 +++++++++- lms/djangoapps/verify_student/models.py | 18 +++++++----------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index 941dd60453d4..9302a5ba8a6e 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -54,7 +54,13 @@ def send_approval_email(attempt): send_verification_approved_email(context=email_context) -def create_verification_attempt(user: User, name: str, status: str, expiration_datetime: Optional[datetime] = None): +def create_verification_attempt( + user: User, + name: str, + status: str, + expiration_datetime: Optional[datetime] = None, + hide_status_from_user: Optional[bool] = None, +): """ Create a verification attempt. @@ -69,11 +75,13 @@ def create_verification_attempt(user: User, name: str, status: str, expiration_d Returns: id (int): The id of the created VerificationAttempt instance """ + # TODO: make sure writing this API stuff is cool w/ michael verification_attempt = VerificationAttempt.objects.create( user=user, name=name, status=status, expiration_datetime=expiration_datetime, + hide_status_from_user=hide_status_from_user, ) emit_idv_attempt_created_event( diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index f68d7e55735f..afeecfd262ae 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1257,15 +1257,11 @@ def retire_user(cls, user_id): verification_attempts = cls.objects.filter(user_id=user_id) verification_attempts.delete() - @classmethod - def should_display_status_to_user(cls, idv_type): + hide_status_from_user = models.BooleanField( + default=False, + ) + + # TODO: Get feedback from michael about adding these new fields/methods + def should_display_status_to_user(cls): """When called, returns true or false based on the type of VerificationAttempt""" - # NOTE: Do we also need another one here for persona - if idv_type == 'VerificationAttempt': - return False # Not sure what to return for this one. - elif idv_type == 'SoftwareSecurePhotoVerification': - return True - elif idv_type == 'SSOVerification': - return False - elif idv_type == 'ManualVerification': - return False + return not cls.hide_status_from_user From 8d579bebd5ee31dfd709372e6a8b81b2ed754383 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Wed, 25 Sep 2024 15:57:40 -0400 Subject: [PATCH 07/18] fix: correct call in helpers+services --- common/djangoapps/student/helpers.py | 3 +-- lms/djangoapps/verify_student/services.py | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index b5c08fc050eb..fb0a7236c0b5 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -168,8 +168,7 @@ def check_verify_status_by_course(user, course_enrollments): # Check whether the user was approved or is awaiting approval if relevant_verification is not None: - idv_type = relevant_verification.__name__ - should_display = relevant_verification.should_display_status_to_user(idv_type=idv_type) + should_display = relevant_verification.should_display_status_to_user() if relevant_verification.status == "approved": if verification_expiring_soon: diff --git a/lms/djangoapps/verify_student/services.py b/lms/djangoapps/verify_student/services.py index 3381a3afcd61..1a2d145e892a 100644 --- a/lms/djangoapps/verify_student/services.py +++ b/lms/djangoapps/verify_student/services.py @@ -187,8 +187,7 @@ def user_status(cls, user): if not attempt: return user_status - idv_type = attempt.__name__ - user_status['should_display'] = attempt.should_display_status_to_user(idv_type=idv_type) + user_status['should_display'] = attempt.should_display_status_to_user() if attempt.expiration_datetime < now() and attempt.status == 'approved': if user_status['should_display']: From 29d1985444958110040d8f263c4fbf9bd05187a5 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Wed, 25 Sep 2024 16:07:09 -0400 Subject: [PATCH 08/18] chore: lint+test fix --- lms/djangoapps/verify_student/models.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index afeecfd262ae..a70398fade8b 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1259,9 +1259,10 @@ def retire_user(cls, user_id): hide_status_from_user = models.BooleanField( default=False, + null=True, ) # TODO: Get feedback from michael about adding these new fields/methods - def should_display_status_to_user(cls): + def should_display_status_to_user(self): """When called, returns true or false based on the type of VerificationAttempt""" - return not cls.hide_status_from_user + return not self.hide_status_from_user From d1763b2b87fc797dfb59f29133c81e870ab8b8b9 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 26 Sep 2024 12:45:20 -0400 Subject: [PATCH 09/18] fix: default hide_status_user as False --- lms/djangoapps/verify_student/api.py | 2 +- lms/djangoapps/verify_student/models.py | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index 9302a5ba8a6e..40880639e123 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -59,7 +59,7 @@ def create_verification_attempt( name: str, status: str, expiration_datetime: Optional[datetime] = None, - hide_status_from_user: Optional[bool] = None, + hide_status_from_user: Optional[bool] = False, ): """ Create a verification attempt. diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index a70398fade8b..00d03a849888 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1224,12 +1224,12 @@ class VerificationAttempt(TimeStampedModel, StatusModel): user = models.ForeignKey(User, db_index=True, on_delete=models.CASCADE) name = models.CharField(blank=True, max_length=255) - STATUS_CHOICES = [ + STATUS = Choices( VerificationAttemptStatus.CREATED, VerificationAttemptStatus.PENDING, VerificationAttemptStatus.APPROVED, VerificationAttemptStatus.DENIED, - ] + ) status = models.CharField(max_length=64, choices=[(status, status) for status in STATUS_CHOICES]) expiration_datetime = models.DateTimeField( @@ -1237,11 +1237,6 @@ class VerificationAttempt(TimeStampedModel, StatusModel): blank=True, ) - @property - def status_changed(self): - """Named as such for backwards compatibility with existing IDVerification models""" - return self.created - @property def updated_at(self): """Backwards compatibility with existing IDVerification models""" From 8baffee28afd39da7856ea1fcad11eb6c9067b22 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 26 Sep 2024 13:00:53 -0400 Subject: [PATCH 10/18] chore: rename field call to STATUS --- lms/djangoapps/verify_student/api.py | 2 +- lms/djangoapps/verify_student/models.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index 40880639e123..55cc850e49c3 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -137,7 +137,7 @@ def update_verification_attempt( 'Status must be one of: %(status_list)s', { 'status': status, - 'status_list': VerificationAttempt.STATUS_CHOICES, + 'status_list': VerificationAttempt.STATUS, }, ) raise VerificationAttemptInvalidStatus diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 00d03a849888..12a32dd18ce9 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1230,7 +1230,7 @@ class VerificationAttempt(TimeStampedModel, StatusModel): VerificationAttemptStatus.APPROVED, VerificationAttemptStatus.DENIED, ) - status = models.CharField(max_length=64, choices=[(status, status) for status in STATUS_CHOICES]) + status = models.CharField(max_length=64, choices=[(status, status) for status in STATUS]) expiration_datetime = models.DateTimeField( null=True, From eefe9704639d2f597bc2a4e765332e18d836764e Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 26 Sep 2024 13:29:51 -0400 Subject: [PATCH 11/18] chore: remove extra status field - comment cleanup --- lms/djangoapps/verify_student/api.py | 1 - lms/djangoapps/verify_student/models.py | 20 +++++++++----------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index 55cc850e49c3..7b8310fde030 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -75,7 +75,6 @@ def create_verification_attempt( Returns: id (int): The id of the created VerificationAttempt instance """ - # TODO: make sure writing this API stuff is cool w/ michael verification_attempt = VerificationAttempt.objects.create( user=user, name=name, diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 12a32dd18ce9..8a65f1ad5976 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1230,13 +1230,21 @@ class VerificationAttempt(TimeStampedModel, StatusModel): VerificationAttemptStatus.APPROVED, VerificationAttemptStatus.DENIED, ) - status = models.CharField(max_length=64, choices=[(status, status) for status in STATUS]) expiration_datetime = models.DateTimeField( null=True, blank=True, ) + hide_status_from_user = models.BooleanField( + default=False, + null=True, + ) + + 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 + @property def updated_at(self): """Backwards compatibility with existing IDVerification models""" @@ -1251,13 +1259,3 @@ def retire_user(cls, user_id): """ verification_attempts = cls.objects.filter(user_id=user_id) verification_attempts.delete() - - hide_status_from_user = models.BooleanField( - default=False, - null=True, - ) - - # TODO: Get feedback from michael about adding these new fields/methods - 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 From 332fd0c76bbfd554c88dd6aaa547710b8f71f597 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 26 Sep 2024 13:34:46 -0400 Subject: [PATCH 12/18] temp: lint + comment out created status for now --- lms/djangoapps/verify_student/models.py | 2 +- lms/djangoapps/verify_student/tests/test_services.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 8a65f1ad5976..4134d0aac0e5 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1225,7 +1225,7 @@ class VerificationAttempt(TimeStampedModel, StatusModel): name = models.CharField(blank=True, max_length=255) STATUS = Choices( - VerificationAttemptStatus.CREATED, + # VerificationAttemptStatus.CREATED, VerificationAttemptStatus.PENDING, VerificationAttemptStatus.APPROVED, VerificationAttemptStatus.DENIED, diff --git a/lms/djangoapps/verify_student/tests/test_services.py b/lms/djangoapps/verify_student/tests/test_services.py index d57993d368af..6a8a04991ca4 100644 --- a/lms/djangoapps/verify_student/tests/test_services.py +++ b/lms/djangoapps/verify_student/tests/test_services.py @@ -287,6 +287,7 @@ class TestIDVerificationServiceUserStatus(TestCase): verifications and in order to control the recency, we just put everything inside of a frozen time """ + def setUp(self): super().setUp() self.user = UserFactory.create() From 6fc1aae039d136c652f666c86be3d2c083931c1b Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 26 Sep 2024 14:18:01 -0400 Subject: [PATCH 13/18] fix: revamp status_changed for back-compat --- lms/djangoapps/verify_student/api.py | 2 +- lms/djangoapps/verify_student/models.py | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index 7b8310fde030..94bd41442164 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -136,7 +136,7 @@ def update_verification_attempt( 'Status must be one of: %(status_list)s', { 'status': status, - 'status_list': VerificationAttempt.STATUS, + 'status_list': VerificationAttempt.STATUS_CHOICES, }, ) raise VerificationAttemptInvalidStatus diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 4134d0aac0e5..73a02738d63b 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1224,12 +1224,14 @@ class VerificationAttempt(TimeStampedModel, StatusModel): user = models.ForeignKey(User, db_index=True, on_delete=models.CASCADE) name = models.CharField(blank=True, max_length=255) - STATUS = Choices( - # VerificationAttemptStatus.CREATED, + STATUS_CHOICES = [ + VerificationAttemptStatus.CREATED, VerificationAttemptStatus.PENDING, VerificationAttemptStatus.APPROVED, VerificationAttemptStatus.DENIED, - ) + ] + + status = models.CharField(max_length=64, choices=[(status, status) for status in STATUS_CHOICES]) expiration_datetime = models.DateTimeField( null=True, @@ -1245,6 +1247,11 @@ 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 + @property + def status_changed(self): + """Backwards compatibility with existing IDVerification models""" + return self.modified + @property def updated_at(self): """Backwards compatibility with existing IDVerification models""" From 11355a217358dfc530b4241b03bdaf48ef5f006f Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 26 Sep 2024 14:39:17 -0400 Subject: [PATCH 14/18] fix: override save for status_changed --- lms/djangoapps/verify_student/models.py | 27 ++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 73a02738d63b..2230ea266f66 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -15,9 +15,12 @@ import logging import os.path import uuid + from datetime import timedelta from email.utils import formatdate +from typing import Any + import requests from config_models.models import ConfigurationModel from django.conf import settings @@ -1233,6 +1236,11 @@ class VerificationAttempt(TimeStampedModel, StatusModel): status = models.CharField(max_length=64, choices=[(status, status) for status in STATUS_CHOICES]) + status_changed = models.DateTimeField( + null=True, + blank=True, + ) + expiration_datetime = models.DateTimeField( null=True, blank=True, @@ -1243,15 +1251,24 @@ class VerificationAttempt(TimeStampedModel, StatusModel): null=True, ) + def save(self, *args: Any, **kwargs: Any) -> None: + """ + Overriding the save method in order to make sure that + status_changed field is updated whenever the status is + updated, even if it is not given as a parameter to the + update field argument. + """ + update_fields = kwargs.get('update_fields', None) + if update_fields and 'status' in update_fields: + self.status_changed = now() + kwargs['update_fields'] = set(update_fields).union({'status_changed'}) + + super().save(*args, **kwargs) + 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 - @property - def status_changed(self): - """Backwards compatibility with existing IDVerification models""" - return self.modified - @property def updated_at(self): """Backwards compatibility with existing IDVerification models""" From 72f7e26daa0934a4997e64eba92b9626f631208e Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 26 Sep 2024 15:00:12 -0400 Subject: [PATCH 15/18] fix: replace created/updated instead of status - also made migrations --- lms/djangoapps/verify_student/api.py | 2 +- ...ve_verificationattempt_created_and_more.py | 40 +++++++++++++++++++ lms/djangoapps/verify_student/models.py | 26 ++---------- 3 files changed, 45 insertions(+), 23 deletions(-) create mode 100644 lms/djangoapps/verify_student/migrations/0017_remove_verificationattempt_created_and_more.py diff --git a/lms/djangoapps/verify_student/api.py b/lms/djangoapps/verify_student/api.py index 94bd41442164..7b8310fde030 100644 --- a/lms/djangoapps/verify_student/api.py +++ b/lms/djangoapps/verify_student/api.py @@ -136,7 +136,7 @@ def update_verification_attempt( 'Status must be one of: %(status_list)s', { 'status': status, - 'status_list': VerificationAttempt.STATUS_CHOICES, + 'status_list': VerificationAttempt.STATUS, }, ) raise VerificationAttemptInvalidStatus diff --git a/lms/djangoapps/verify_student/migrations/0017_remove_verificationattempt_created_and_more.py b/lms/djangoapps/verify_student/migrations/0017_remove_verificationattempt_created_and_more.py new file mode 100644 index 000000000000..38f89402ad83 --- /dev/null +++ b/lms/djangoapps/verify_student/migrations/0017_remove_verificationattempt_created_and_more.py @@ -0,0 +1,40 @@ +# Generated by Django 4.2.15 on 2024-09-26 18:59 + +from django.db import migrations, models +import django.utils.timezone +import lms.djangoapps.verify_student.statuses +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('verify_student', '0016_verificationattempt_status_changed'), + ] + + operations = [ + migrations.RemoveField( + model_name='verificationattempt', + name='created', + ), + migrations.RemoveField( + model_name='verificationattempt', + name='modified', + ), + migrations.AddField( + model_name='verificationattempt', + name='created_at', + field=models.DateTimeField(auto_now_add=True, db_index=True, default=django.utils.timezone.now), + preserve_default=False, + ), + migrations.AddField( + model_name='verificationattempt', + name='hide_status_from_user', + field=models.BooleanField(default=False, null=True), + ), + migrations.AlterField( + model_name='verificationattempt', + name='status', + field=model_utils.fields.StatusField(choices=[(lms.djangoapps.verify_student.statuses.VerificationAttemptStatus['CREATED'], lms.djangoapps.verify_student.statuses.VerificationAttemptStatus['CREATED']), (lms.djangoapps.verify_student.statuses.VerificationAttemptStatus['PENDING'], lms.djangoapps.verify_student.statuses.VerificationAttemptStatus['PENDING']), (lms.djangoapps.verify_student.statuses.VerificationAttemptStatus['APPROVED'], lms.djangoapps.verify_student.statuses.VerificationAttemptStatus['APPROVED']), (lms.djangoapps.verify_student.statuses.VerificationAttemptStatus['DENIED'], lms.djangoapps.verify_student.statuses.VerificationAttemptStatus['DENIED'])], default=lms.djangoapps.verify_student.statuses.VerificationAttemptStatus['CREATED'], max_length=100, no_check_for_status=True, verbose_name='status'), + ), + ] diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 2230ea266f66..32f02acd4af5 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1217,7 +1217,7 @@ def __str__(self): return str(self.arguments) -class VerificationAttempt(TimeStampedModel, StatusModel): +class VerificationAttempt(StatusModel): """ The model represents impelementation-agnostic information about identity verification (IDV) attempts. @@ -1227,18 +1227,11 @@ class VerificationAttempt(TimeStampedModel, StatusModel): user = models.ForeignKey(User, db_index=True, on_delete=models.CASCADE) name = models.CharField(blank=True, max_length=255) - STATUS_CHOICES = [ + STATUS = Choices( VerificationAttemptStatus.CREATED, VerificationAttemptStatus.PENDING, VerificationAttemptStatus.APPROVED, VerificationAttemptStatus.DENIED, - ] - - status = models.CharField(max_length=64, choices=[(status, status) for status in STATUS_CHOICES]) - - status_changed = models.DateTimeField( - null=True, - blank=True, ) expiration_datetime = models.DateTimeField( @@ -1251,19 +1244,8 @@ class VerificationAttempt(TimeStampedModel, StatusModel): null=True, ) - def save(self, *args: Any, **kwargs: Any) -> None: - """ - Overriding the save method in order to make sure that - status_changed field is updated whenever the status is - updated, even if it is not given as a parameter to the - update field argument. - """ - update_fields = kwargs.get('update_fields', None) - if update_fields and 'status' in update_fields: - self.status_changed = now() - kwargs['update_fields'] = set(update_fields).union({'status_changed'}) - - super().save(*args, **kwargs) + created_at = models.DateTimeField(auto_now_add=True, db_index=True) + updated_at = models.DateTimeField(auto_now=True, db_index=True) def should_display_status_to_user(self): """When called, returns true or false based on the type of VerificationAttempt""" From e5a2e836f34ff4aed6a5ca20f16cc8e18f9a8ec6 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 26 Sep 2024 15:52:54 -0400 Subject: [PATCH 16/18] fix: squash commits - also remove extra updated_at property --- ...e_verificationattempt_created_and_more.py} | 9 +++++++-- ...0016_verificationattempt_status_changed.py | 20 ------------------- lms/djangoapps/verify_student/models.py | 5 ----- lms/djangoapps/verify_student/services.py | 4 ++-- 4 files changed, 9 insertions(+), 29 deletions(-) rename lms/djangoapps/verify_student/migrations/{0017_remove_verificationattempt_created_and_more.py => 0016_remove_verificationattempt_created_and_more.py} (84%) delete mode 100644 lms/djangoapps/verify_student/migrations/0016_verificationattempt_status_changed.py diff --git a/lms/djangoapps/verify_student/migrations/0017_remove_verificationattempt_created_and_more.py b/lms/djangoapps/verify_student/migrations/0016_remove_verificationattempt_created_and_more.py similarity index 84% rename from lms/djangoapps/verify_student/migrations/0017_remove_verificationattempt_created_and_more.py rename to lms/djangoapps/verify_student/migrations/0016_remove_verificationattempt_created_and_more.py index 38f89402ad83..4954c32e12f1 100644 --- a/lms/djangoapps/verify_student/migrations/0017_remove_verificationattempt_created_and_more.py +++ b/lms/djangoapps/verify_student/migrations/0016_remove_verificationattempt_created_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.15 on 2024-09-26 18:59 +# Generated by Django 4.2.15 on 2024-09-26 19:39 from django.db import migrations, models import django.utils.timezone @@ -9,7 +9,7 @@ class Migration(migrations.Migration): dependencies = [ - ('verify_student', '0016_verificationattempt_status_changed'), + ('verify_student', '0015_verificationattempt'), ] operations = [ @@ -32,6 +32,11 @@ class Migration(migrations.Migration): name='hide_status_from_user', field=models.BooleanField(default=False, null=True), ), + migrations.AddField( + model_name='verificationattempt', + name='status_changed', + field=model_utils.fields.MonitorField(default=django.utils.timezone.now, monitor='status', verbose_name='status changed'), + ), migrations.AlterField( model_name='verificationattempt', name='status', diff --git a/lms/djangoapps/verify_student/migrations/0016_verificationattempt_status_changed.py b/lms/djangoapps/verify_student/migrations/0016_verificationattempt_status_changed.py deleted file mode 100644 index d5aba6fd369d..000000000000 --- a/lms/djangoapps/verify_student/migrations/0016_verificationattempt_status_changed.py +++ /dev/null @@ -1,20 +0,0 @@ -# Generated by Django 4.2.15 on 2024-09-19 16:17 - -from django.db import migrations -import django.utils.timezone -import model_utils.fields - - -class Migration(migrations.Migration): - - dependencies = [ - ('verify_student', '0015_verificationattempt'), - ] - - operations = [ - migrations.AddField( - model_name='verificationattempt', - name='status_changed', - field=model_utils.fields.MonitorField(default=django.utils.timezone.now, monitor='status', verbose_name='status changed'), - ), - ] diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 32f02acd4af5..53e464ef9389 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -1251,11 +1251,6 @@ 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 - @property - def updated_at(self): - """Backwards compatibility with existing IDVerification models""" - return self.modified - @classmethod def retire_user(cls, user_id): """ diff --git a/lms/djangoapps/verify_student/services.py b/lms/djangoapps/verify_student/services.py index 1a2d145e892a..f0d8a8631482 100644 --- a/lms/djangoapps/verify_student/services.py +++ b/lms/djangoapps/verify_student/services.py @@ -76,7 +76,7 @@ def verifications_for_user(cls, user): Return a list of all verifications associated with the given user. """ verifications = [] - for verification in chain(VerificationAttempt.objects.filter(user=user).order_by('-created'), + for verification in chain(VerificationAttempt.objects.filter(user=user).order_by('-created_at'), 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')): @@ -97,7 +97,7 @@ def get_verified_user_ids(cls, users): VerificationAttempt.objects.filter(**{ 'user__in': users, 'status': 'approved', - 'created__gt': now() - timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + 'created_at__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), From f3f7b9f07707b572c92aadf26911a6663fc6eb84 Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 26 Sep 2024 15:58:31 -0400 Subject: [PATCH 17/18] chore: nits --- lms/djangoapps/verify_student/models.py | 1 - lms/djangoapps/verify_student/tests/test_services.py | 1 - 2 files changed, 2 deletions(-) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 53e464ef9389..9a0ac369640a 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -19,7 +19,6 @@ from datetime import timedelta from email.utils import formatdate -from typing import Any import requests from config_models.models import ConfigurationModel diff --git a/lms/djangoapps/verify_student/tests/test_services.py b/lms/djangoapps/verify_student/tests/test_services.py index 6a8a04991ca4..d57993d368af 100644 --- a/lms/djangoapps/verify_student/tests/test_services.py +++ b/lms/djangoapps/verify_student/tests/test_services.py @@ -287,7 +287,6 @@ class TestIDVerificationServiceUserStatus(TestCase): verifications and in order to control the recency, we just put everything inside of a frozen time """ - def setUp(self): super().setUp() self.user = UserFactory.create() From 775a57ab533ae46ee29471207c54b38c1bf504ed Mon Sep 17 00:00:00 2001 From: ilee2u Date: Thu, 26 Sep 2024 16:09:41 -0400 Subject: [PATCH 18/18] chore: revise migrations --- .../0016_remove_verificationattempt_created_and_more.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/verify_student/migrations/0016_remove_verificationattempt_created_and_more.py b/lms/djangoapps/verify_student/migrations/0016_remove_verificationattempt_created_and_more.py index 4954c32e12f1..d972dba3dbbd 100644 --- a/lms/djangoapps/verify_student/migrations/0016_remove_verificationattempt_created_and_more.py +++ b/lms/djangoapps/verify_student/migrations/0016_remove_verificationattempt_created_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.15 on 2024-09-26 19:39 +# Generated by Django 4.2.15 on 2024-09-26 20:08 from django.db import migrations, models import django.utils.timezone @@ -37,6 +37,11 @@ class Migration(migrations.Migration): name='status_changed', field=model_utils.fields.MonitorField(default=django.utils.timezone.now, monitor='status', verbose_name='status changed'), ), + migrations.AddField( + model_name='verificationattempt', + name='updated_at', + field=models.DateTimeField(auto_now=True, db_index=True), + ), migrations.AlterField( model_name='verificationattempt', name='status',