-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add should_display_status_to_user method and status_changed field to VerificationAttempt model #35514
fix: add should_display_status_to_user method and status_changed field to VerificationAttempt model #35514
Conversation
verification_attempt = VerificationAttempt.objects.create( | ||
user=user, | ||
name=name, | ||
status=status, | ||
expiration_datetime=expiration_datetime, | ||
hide_status_from_user=hide_status_from_user, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zach helped me come up with this param/var, which we could add to the api function as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me so far! I had several questions to better understand your approach.
The status_changed
field is one thing that I'm not understanding, so I'd like to see that address before you merge, please!
4c787e2
to
f3ddad7
Compare
NOTE: We had an issue where inheriting from both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -0,0 +1,20 @@ | |||
# Generated by Django 4.2.15 on 2024-09-19 16:17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, could you squash these migrations, please? It keeps things tidy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
- should_display_status_to_user now returns False
- comment cleanup
- also made migrations
- also remove extra updated_at property
25cc20c
to
775a57a
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. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Ticket: https://2u-internal.atlassian.net/browse/COSMO-499
Description
This PR fixes a bug where the IDVerificationService was accessing methods and fields of the platform VerificationAttempt model that didn't exist:
should_display_status_to_user
This method is called by:
Note for self: Updating this may not be as straightforward as returning a hardcoded boolean like the other verify_student models do. For example, SSOVerification instances return True, but ManualVerification instances return False. Because the platform VerificationAttempt model is intended to be vendor and implementation agnostic, consider whether this belongs as a field on the model instead.
status_changed (comes from the StatusModel that IdVerificationAttempt inherits from but VerificationAttempt does not)
This field is referenced by
Relevant Docs: Add Extensibility Mechanisms to IDV to Enable Integration of New IDV Vendor Persona · Issue #367 · openedx/platform-roadmap
Environment
Useful information to include:
"Developer", and "Operator".
changes.
Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Include anything else that will help reviewers and consumers understand the change.