Skip to content
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

Merged
merged 18 commits into from
Sep 26, 2024

Conversation

ilee2u
Copy link
Contributor

@ilee2u ilee2u commented Sep 20, 2024

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:

  • Which edX user roles will this change impact? Common user roles are "Learner", "Course Author",
    "Developer", and "Operator".
  • Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
  • Provide links to the description of corresponding configuration changes. Remember to correctly annotate these
    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.

  • Does this change depend on other changes elsewhere?
  • Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.
  • If your database migration can't be rolled back easily.

@ilee2u ilee2u changed the title Ilee2u/add missing verification attempt methods fields fix: add should_display_status_to_user method and status_changed field to VerificationAttempt model Sep 20, 2024
verification_attempt = VerificationAttempt.objects.create(
user=user,
name=name,
status=status,
expiration_datetime=expiration_datetime,
hide_status_from_user=hide_status_from_user,
Copy link
Contributor Author

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.

lms/djangoapps/verify_student/models.py Outdated Show resolved Hide resolved
lms/djangoapps/verify_student/models.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MichaelRoytman MichaelRoytman left a 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!

lms/djangoapps/verify_student/models.py Outdated Show resolved Hide resolved
lms/djangoapps/verify_student/models.py Outdated Show resolved Hide resolved
lms/djangoapps/verify_student/models.py Outdated Show resolved Hide resolved
@ilee2u ilee2u force-pushed the ilee2u/add-missing-VerificationAttempt-methods-fields branch from 4c787e2 to f3ddad7 Compare September 26, 2024 19:00
@ilee2u ilee2u marked this pull request as ready for review September 26, 2024 19:01
@ilee2u
Copy link
Contributor Author

ilee2u commented Sep 26, 2024

NOTE: We had an issue where inheriting from both StatusModel and TimeStampedModel produced two created fields in the VerificationAttempt model. We ended up deciding to use a custom created_at and updated_at fields (instead of trying to override the save() function to make a custom version of status_changed from the StatusModel) in order to match the pattern of the other models.

Copy link
Contributor

@schenedx schenedx left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

lms/djangoapps/verify_student/models.py Outdated Show resolved Hide resolved
lms/djangoapps/verify_student/tests/test_services.py Outdated Show resolved Hide resolved
@ilee2u ilee2u enabled auto-merge (squash) September 26, 2024 20:04
@ilee2u ilee2u force-pushed the ilee2u/add-missing-VerificationAttempt-methods-fields branch from 25cc20c to 775a57a Compare September 26, 2024 20:10
@ilee2u ilee2u merged commit dac0309 into master Sep 26, 2024
48 checks passed
@ilee2u ilee2u deleted the ilee2u/add-missing-VerificationAttempt-methods-fields branch September 26, 2024 20:58
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants