Add VerificationAttempt model to verify_student application. #35304
Conversation
MichaelRoytman
force-pushed
the
michaelroytman/COSMO-421-VerificationAttempt-model
branch
2 times, most recently
from
6a74d5d
to
844e1a4
Compare
|
@openedx/committers-edx-platform and @ormsbee, could you review, please? The openedx/platform-roadmap#367 ticket details the entire project. Thank you! |
MichaelRoytman
force-pushed
the
michaelroytman/COSMO-421-VerificationAttempt-model
branch
from
844e1a4
to
f734bde
Compare
| * The `verify_student Django application`_ will contain both concrete implementations of forms of IDV (i.e. manual, SSO, | ||
| Software Secure, etc.) and a generic, extensible implementation. The work to deprecate and remove the Software Secure | ||
| integration and to transition the other existing forms of IDV (i.e. manual and SSO) to Django plugins will occur | ||
| indepentendly of the improvements to extensibility described in this decision. |
There was a problem hiding this comment.
| indepentendly of the improvements to extensibility described in this decision. | |
| independently of the improvements to extensibility described in this decision. |
| the platform. | ||
| """ | ||
| user = models.ForeignKey(User, db_index=True, on_delete=models.CASCADE) | ||
| name = models.CharField(blank=True, max_length=255) |
There was a problem hiding this comment.
Is a blank name really ever okay?
There was a problem hiding this comment.
It depends. Based on the current Software Secure flow, it is possible but is difficult and likely occurs seldomly. You'd have to really try to get around providing a name, likely by hitting the legacy IDV flow somehow.
The SoftwareSecurePhotoVerification instance can be created without a name if the full name isn't provided during IDV - this is the legacy view but is also true of the MFE based flow. The name is added in a subsequent mark_ready step, but profile_name is an optional field of the UserProfile model, so it can be empty.
I don't know the full history of the choice for blank=True, but my guess is that before we added the Name Affirmation feature, we only cared that the user was ID verified, and the name was secondary to that fact and was only used optionally for the purposes of a name change - it wasn't necessary to supply a name to become IDV verified. Now, you need to supply a name before IDV because of the Name Affirmation feature; although, there are ways around it.
Also, an SSOVerification can be created without a name because profile_name is an optional field. If we adopt the position that IDV is used to verify names and identities, then we could only create SSOVerifications only when the learner has a name to verify and that has been synced from the third party provider.
I don't think ManualVerification or our Persona integration will make sense without a name, but I tried to mimic the schema of the IDVerificationAttempt model here. I'm comfortable making it required based on those two verification methods, but I'm unsure about SSOVerification, and I don't know if additional implementations of IDV may require an IDV attempt without name.
If that's something we'd like to enforce, then I can make it required, but it's going to require some help from product re: SSOVerifications and will complicate process of deprecating the Software Secure integration. When deprecating the SoftwareSecurePhotoVerification model and migrating the data into VerificationAttempt, we'll have to drop any instances without a name value.
Do you have any thoughts?
There was a problem hiding this comment.
I asked mostly just to check that it wasn't a simple oversight. If this were something being built from scratch, I might push to disallow blank entries here. But I'm not looking to do anything that would complicate the migration process, so blank=True it is.
| null=True, | ||
| blank=True, | ||
| db_index=True, | ||
| ) |
There was a problem hiding this comment.
If your most common query is something like, "does this user have an unexpired verification?", you might want to explicitly make a composite index for that (["user", "-expiration_datetime"]). MySQL on RDS isn't always great about combining the intersection of two indexes, and can sometimes do a lot more work than it needs to.
There was a problem hiding this comment.
I took a look at the the query patterns on the IDV tables, and they're almost always querying on the user field and then sometimes on the status field. I didn't see any examples of querying on the expiration_datetime; the rows are sorted in Python. I removed the index on that field and left the index on the user field.
I considered adding a composite index on ["user", "status"], but I wasn't sure if that the overhead of maintaining the index would be worth the benefits of the index, since the number of VerificationAttempts per learner should be relatively small. I wish I could see the query plan for MySQL with and without that index but couldn't figure out how to do that.
If you have any thoughts about an index like this, let me know. I'll merge this PR as-is, but I'm happy to put in a follow up PR to adjust the index. Thanks!
MichaelRoytman
force-pushed
the
michaelroytman/COSMO-421-VerificationAttempt-model
branch
from
f734bde
to
5a49c5d
Compare
This commits adds a VerificationAttempt model to store implementation and provider agnostic information about identity verification attempts in the platform.
MichaelRoytman
force-pushed
the
michaelroytman/COSMO-421-VerificationAttempt-model
branch
from
5a49c5d
to
b30318a
Compare
MichaelRoytman
deleted the
michaelroytman/COSMO-421-VerificationAttempt-model
branch
|
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. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
This commits adds a
VerificationAttemptmodel to store implementation and provider agnostic information about identity verification attempts in the platform.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.
Testing instructions
None.
Deadline
ASAP.