feat: Add VerificationAttempt model to IDVerificationService logic #35311
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
should we consider this function out of scope for this ticket? We can't refactor or remove it until we change how the support tools work.
There was a problem hiding this comment.
That makes sense to me, especially given that this is dependent on changes elsewhere?
There was a problem hiding this comment.
I think so, yes. However, it's possible Axim may not approve this without us either fixing it or describing our plan to fix it in detail, so let's see how our conversation goes tomorrow and go from there. I think there's no harm putting it up for review - we could get some additional feedback about how we could solve this.
There was a problem hiding this comment.
[Update] We've decided to move ahead with this PR. I've left a comment on this function detailing that it has not been updated to account for the new model and referenced the DEPR here: #35452
zacharis278
changed the title
| VerificationAttempt.objects.filter(**{ | ||
| 'user__in': users, | ||
| 'status': 'approved', | ||
| 'created__gt': now() - timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) |
There was a problem hiding this comment.
Ah, is this because TimestampedModel changed the name of created_at to created at some point? 🤦♂️
There was a problem hiding this comment.
It's actually not even a timestamped model. There are explicit fields for created_at and updated_at instead of created/modified. Unfortunately I can't just use a model property here since this is a query.
There was a problem hiding this comment.
Ah, I see. I'm tempted to just name the fields created_at and updated_at in the generic model, but I think using TimestampedModel is a better approach longer term.
| @@ -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 | |||
There was a problem hiding this comment.
I think so, yes. However, it's possible Axim may not approve this without us either fixing it or describing our plan to fix it in detail, so let's see how our conversation goes tomorrow and go from there. I think there's no harm putting it up for review - we could get some additional feedback about how we could solve this.
| @@ -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'), | |||
There was a problem hiding this comment.
Should this be 'created_at'? Does it matter if it's not matching others?
There was a problem hiding this comment.
This is due to the old models not inheriting from TimestampedModel
| @@ -117,11 +123,14 @@ def get_expiration_datetime(cls, user, statuses): | |||
| 'status__in': statuses, | |||
| } | |||
|
|
|||
| id_verifications = VerificationAttempt.objects.filter(**filter_kwargs) | |||
There was a problem hiding this comment.
Should there be something in front of id_verifications here to specify what kind of id verification this is? Is persona_id_verifications too specific here?
There was a problem hiding this comment.
IMO this is intended to be the most general verification model so it doesn't need further specification
| @@ -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 | |||
There was a problem hiding this comment.
That makes sense to me, especially given that this is dependent on changes elsewhere?
| @@ -34,12 +40,15 @@ class TestIDVerificationService(ModuleStoreTestCase): | |||
| Tests for IDVerificationService. | |||
| """ | |||
|
|
|||
| def test_user_is_verified(self): | |||
| @ddt.data( | |||
MichaelRoytman
force-pushed
the
michaelroytman/COSMO-421-VerificationAttempt-model
branch
from
844e1a4 to
f734bde
Compare
MichaelRoytman
force-pushed
the
michaelroytman/COSMO-421-VerificationAttempt-model
branch
2 times, most recently
from
5a49c5d to
b30318a
Compare
zacharis278
force-pushed
the
zhancock/generic-verification
branch
from
3035ba7 to
f7ddb7d
Compare
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
The new VerificationAttempt model (#35304) will now be taken into account when determining a user's verification status.
Supporting information
Please see openedx/platform-roadmap#367 and [Proposal] Add Extensibility Mechanisms to IDV to Enable Integration of New IDV Vendor Persona for more details.