API functions for the new generic VerificationAttempt model in the verify_student app

[ilee2u]

Description

This PR adds api functions which can be called by IDV plugins to create and update instances of the recently added generic VerificationAttempt model.

Context for this decision can be found in this excerpt from our internal spec doc:

We will introduce one new generic model - the VerificationAttempt model. The
purpose of this model is to store data about verification attempts on the
platform. 

The VerificationAttempt model will store information about each IDV attempt. It
will have the majority, if not all, the fields on the
[IDVerificationAttempt](https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/verify_student/models.py#L94) abstract base model. The biggest change is that the set of
available statuses will be created, pending, approved, and denied. 

Eventually, the vision is for all forms of IDV to be installed into the platform
as plugins. The plugins would write to the VerificationAttempt model so that the
platform has a record of the IDV information it needs. The rationale is that the
platform only cares whether the learner is ID verified and not how they are ID
verified.

This would replace the majority of filter hooks proposed in the original spec.
This is because doing database queries via filter hooks can cause unpredictable
performance problems. This has been a pain point for Open edX. Write now, read
later is a better pattern to follow.

Note that the IDVerificationURLRequested(url) filter hook is still a part of the
design to replace the IDV URL on the platform. Although we do not need to use it
to integrate Persona, it is necessary to enable additional implementations of
IDV via these changes.

Design decisions and their rationales should be documented in the repo (docstring / ADR), per
OEP-19, and can be
linked here.

Useful information to include:

• Which edX user roles will this change impact? Common user roles are "Learner", "Course Author",
  "Developer", and "Operator".
  • Learners will primarily be impacted due to our primary IDV vendor changing.
  • Course Authors will also be somewhat impacted, at the very least because they will need to be notified of the changed IDV process (which we've already made efforts to do).
  • Developers and Operators will also be somewhat impacted, as they'll also need to be notified of this change and how this impacts support as well as future development on IDV and IDV-related systems.

Supporting information

Info from internal ticket:

introduce a Python API for the generic VerificationAttempt model in the verify_student application. The purpose of this API is to allow plugins that implement different types of IDV to import these methods and effect changes in the verify_student VerificationAttempt model.

Below is an idea for the function signatures.

id = create_verification_attempt(user, name, status, expiration_date=None)
update_verification_attempt(id, status)

Note that an id must be returned from create_verification_attempt so that the plugin can specify the id on any following update_verification calls.

Deadline

None

Other information

Other changes will be made in openedx-events and edx-name-affirmation to implement the downstream effects of calling these new API functions.

[varshamenon4]

Looks good so far! Just a couple of nits and questions.

[MichaelRoytman]

Looking good! I left a few more comments.

Could you also update the PR description and answer the prompts so that others can better review your changes? Thanks!

[MichaelRoytman]

My concern here is that this is brittle. If any other attributes are added to the class, they'll be treated as statuses - even methods!

I see three alternatives.

1. We could enumerate the list of valid statuses here.
2. We could add an attribute like all_statuses to the VerificationAttemptStatus class.
3. We could add an is_valid_status to the VerificationAttemptStatus class that update_verification_attempt_status class calls.

I'm partial to #3, because then the status class is responsible for defining what a valid status is, not the API function.

What do you think?

[MichaelRoytman]

In retrospect, trying to save a non-valid status (i.e. something that isn't in the choices list on the model) will raise a ValidationError, so Django handles this for us out of the box. You could consider keeping the default Django behavior instead of explicitly checking the status. My mistake.

[ilee2u]

Added a comment below with how I decided to handle this: #35338 (comment)

[MichaelRoytman]

Bump.

[ilee2u]

I had to add this call to attempt.clean_fields() in order for this ValidationError to work as @MichaelRoytman mentioned in their previous comment. Let me know if there's something else I should do that's best practice.

[zacharis278]

this reads like it would take any validation error and report back that the status is invalid. If we're only checking for bad status we should do that a bit more explicitly. I'd fear this would create confusing behavior where another field fails to validate but our system incorrectly tells us the status is bad.

[MichaelRoytman]

We should probably just call full_clean() here to validate the changes fully if we take this approach.

That said, maybe it would be easier to do what you had before and validate the status values in Python? I didn't realize that calling attempt.save() wouldn't perform that validation.

I haven't seen the use of Django forms a lot at edX, but using a form would perform the validation automatically, because calling save() on a form will call full_clean().

But I agree with Zach. The validation error will look something like this, so you could key off that.

django.core.exceptions.ValidationError: {'status': ["Value 'invalid' is not a valid choice."]}

[ilee2u]

I put back the code to only validate which I had earlier

[MichaelRoytman]

We should probably just call full_clean() here to validate the changes fully if we take this approach.

That said, maybe it would be easier to do what you had before and validate the status values in Python? I didn't realize that calling attempt.save() wouldn't perform that validation.

I haven't seen the use of Django forms a lot at edX, but using a form would perform the validation automatically, because calling save() on a form will call full_clean().

But I agree with Zach. The validation error will look something like this, so you could key off that.

django.core.exceptions.ValidationError: {'status': ["Value 'invalid' is not a valid choice."]}

[ormsbee]

Minor comments/nits for your consideration. Nothing blocking. Thank you for your patience with my late review.

[ormsbee]

Is it the case that if VerificationAttempt.expiration_datetime is None, that means "this verification never expires"? If that's the case, then if a expiration_datetime was set to be sometime next year, and I wanted to update it to never expire, is that possible?

[ilee2u]

If the value of the expiration_datetime parameter sent to this function is None, then the expiration_datetime of the IDV attempt will not be updated. This is to maintain that all IDV attempts will expire/retire at some point (which at least for IDV attempts via Persona, should be 14 days).

[MichaelRoytman]

Historically, we have set the expiration date from the time that an IDV attempt is marked approved. For this reason, I suggested that we make expiration_datetime optional because it may not make sense for a non-approved IDV attempt to have an expiration date. For example, if you were to update an instance of VerificationAttempt from the status created to pending, there should be no expiration date.

However, this API should remain flexible, because that depends on a particular implementation of IDV. I'd agree that we should be able to update the expiration_datetime for any status.

Isaac, to Dave's point, I think that we shouldn't prevent the user of this function from setting expiration_datetime to None, necessarily. We should just not require that they supply a value for the parameter. What you think?

[ilee2u]

That makes sense to me, thank you for clarifying that! I'll push this change soon.

[ormsbee]

Nit (optional): FWIW, Python 3.11 has StrEnum now, which might be a more natural way to express this.

[ilee2u]

I attempted to use StrEnum for this, however that involved needing to change the VerificationAttemptStatus class itself, which in turn would require me to refactor quite a bit in this application (especially within tests) so this unfortunately doesn't seem worth it.

[zacharis278]

a pattern I've used for this in the past is to give my constant 'enum' class a list property with valid values. Eg https://github.com/edx/edx-exams/blob/1b76cf33d8b0fbca6e15742cad8ab49f02960f1d/edx_exams/apps/core/statuses.py#L54

That, or can't we just use VerificationAttempt.status_choices?

[MichaelRoytman]

At a minimum, I'd much prefer that we be explicit about the statuses in this list (i.e. status_list = [VerificationAttemptStatus.created, ...]) over implicit. This is brittle and will stick anything that isn't a dunder method into this list, including any future attributes or methods.

But I do not think that converting that class to use the Enum module would be that hard. Where is this class used in tests? I currently only see it in use in the VerificationAttempt model.

from enum import StrEnum, auto

class VerificationAttemptStatus(StrEnum):
    CREATED = auto()
    PENDING = auto()
    APPROVED = auto()
    DENIED = auto()

Here, you could do the following...

status_list = list(VerificationAttemptStatus)

You'd have to update STATUS_CHOICES in the model as well. I'm not sure whether that would require a migration.

My only concern is that we sometimes like to add methods to these status classes, like is_terminal_status, for example. But we don't seem to have a need for that right now, and this does really work like an Enum, so it's probably fine to make this change.

Please let me know if I'm missing something about the difficulty of this change.

[ilee2u]

Oh I see what happened, I misread the docs and had VerificationAttemptStatus inherit from enum.Flag instead of enum.StrEnum 🤦. I implemented what you suggested, and I didn't have to make any migrations, and everything seems to work just fine. Thanks so much!

[ormsbee]

Nit (optional): A number of APIs use type annotations these days, which can be helpful for documentation and correctness checking.

[ilee2u]

I've added annotations to these functions!

[MichaelRoytman]

I left some comments. I'll clear my change request so that it's not blocking. However, even if you choose not to use the StrEnum class, I think that we should address the way status_list is constructed to be more explicit before merging.

[MichaelRoytman]

Historically, we have set the expiration date from the time that an IDV attempt is marked approved. For this reason, I suggested that we make expiration_datetime optional because it may not make sense for a non-approved IDV attempt to have an expiration date. For example, if you were to update an instance of VerificationAttempt from the status created to pending, there should be no expiration date.

However, this API should remain flexible, because that depends on a particular implementation of IDV. I'd agree that we should be able to update the expiration_datetime for any status.

Isaac, to Dave's point, I think that we shouldn't prevent the user of this function from setting expiration_datetime to None, necessarily. We should just not require that they supply a value for the parameter. What you think?

[MichaelRoytman]

At a minimum, I'd much prefer that we be explicit about the statuses in this list (i.e. status_list = [VerificationAttemptStatus.created, ...]) over implicit. This is brittle and will stick anything that isn't a dunder method into this list, including any future attributes or methods.

But I do not think that converting that class to use the Enum module would be that hard. Where is this class used in tests? I currently only see it in use in the VerificationAttempt model.

from enum import StrEnum, auto

class VerificationAttemptStatus(StrEnum):
    CREATED = auto()
    PENDING = auto()
    APPROVED = auto()
    DENIED = auto()

Here, you could do the following...

status_list = list(VerificationAttemptStatus)

You'd have to update STATUS_CHOICES in the model as well. I'm not sure whether that would require a migration.

My only concern is that we sometimes like to add methods to these status classes, like is_terminal_status, for example. But we don't seem to have a need for that right now, and this does really work like an Enum, so it's probably fine to make this change.

Please let me know if I'm missing something about the difficulty of this change.

I want to remove the blocker.

[ormsbee]

Thank you!

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX production environment.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX production environment.