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

Add IDV Status to VerifiedName #215

Merged
merged 10 commits into from
Sep 9, 2024
Merged

Conversation

rijuma
Copy link
Member

@rijuma rijuma commented Aug 28, 2024

Description:

The purpose of this update is to decouple the VerifiedName component in the Support Tools MFE from the verify_student application. The VerifiedName component is used to display a table of verified name information in the Support Tools. A screenshot is attached.

The solution might not be the most optimal, since it would make a call for each individual VerifiedName, but it's made like this due to the verification_attempt_id not having a proper foreign key currently in the database. This shouldn't be a problem since the verified names per user are at most just a few rows. This would be revisited in the near future.

Screenshot

image

JIRA:

COSMO-442🔒

Pre-Merge Checklist:

  • Updated the version number in edx_name_affirmation/__init__.py if these changes are to be released. See OEP-47: Semantic Versioning.
  • Described your changes in CHANGELOG.rst.
  • Confirmed Github reports all automated tests/checks are passing.
  • Approved by at least one additional reviewer.

Post-Merge:

  • Create a tag matching the new version number.

@rijuma rijuma force-pushed the rijuma/add-idv-status-to-verified-name branch 10 times, most recently from 60a0860 to 180fd69 Compare August 30, 2024 16:04
Copy link

github-actions bot commented Aug 30, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  edx_name_affirmation
  models.py
  serializers.py
  edx_name_affirmation/tests
  test_models.py 85
  test_views.py
Project Total  

This report was generated by python-coverage-comment-action

@rijuma rijuma force-pushed the rijuma/add-idv-status-to-verified-name branch 6 times, most recently from 9bd4e7d to d0cacdb Compare September 3, 2024 15:55
@rijuma rijuma marked this pull request as ready for review September 3, 2024 15:56
Copy link
Member

@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.

I left a few comments with requested changes, but this looks good overall so far.

edx_name_affirmation/models.py Outdated Show resolved Hide resolved
edx_name_affirmation/models.py Outdated Show resolved Hide resolved
edx_name_affirmation/tests/test_models.py Outdated Show resolved Hide resolved
edx_name_affirmation/tests/test_views.py Show resolved Hide resolved
@rijuma rijuma force-pushed the rijuma/add-idv-status-to-verified-name branch 3 times, most recently from a3da5fe to 33627a4 Compare September 9, 2024 14:51
Copy link
Member

@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. I added three nits that you can take or leave.

edx_name_affirmation/models.py Outdated Show resolved Hide resolved
edx_name_affirmation/tests/test_models.py Outdated Show resolved Hide resolved
from django.test import TestCase

from edx_name_affirmation.models import VerifiedName
from edx_name_affirmation.statuses import VerifiedNameStatus

User = get_user_model()

idv_attempt_id = 34455
Copy link
Member

Choose a reason for hiding this comment

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

Nit. idv_attempt_id makes sense to define in a common place because it's used across test methods, but I think making it an attribute of the test class instance, set in a setUp method would be better.

def setUp(self):
    super().setUp()
    self.idv_attempt_id = 3455

and then change references to it in the tests to self.idv_attempt_id.

Copy link
Member Author

Choose a reason for hiding this comment

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

idv_attempt_id is being used within the helper method _mocked_model_get(). I'll try to scope it inside the test. Maybe just adding it as a helper method there is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved these variables and the helper methods to be defined within VerifiedNameModelTests class so the scope is more limited. Let me know what you think.

@rijuma rijuma force-pushed the rijuma/add-idv-status-to-verified-name branch 3 times, most recently from 95354e5 to aa456e8 Compare September 9, 2024 18:14
@rijuma rijuma force-pushed the rijuma/add-idv-status-to-verified-name branch from aa456e8 to b8069b7 Compare September 9, 2024 18:19
if id == self.idv_attempt_id:
return self._obj({'status': self.idv_attempt_status})

return self._obj({'status': None})
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can omit the Missing coverage warning since this is just a mock for testing. I'm not sure why is this a thing on a test file.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure. I think it's fine to ignore!

@rijuma rijuma merged commit bf0493f into main Sep 9, 2024
6 checks passed
@rijuma rijuma deleted the rijuma/add-idv-status-to-verified-name branch September 9, 2024 19:10
github-actions bot pushed a commit to openedx/edx-platform that referenced this pull request Sep 12, 2024
edx/edx-name-affirmation#215

Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
rijuma added a commit to openedx/edx-platform that referenced this pull request Sep 12, 2024
edx/edx-name-affirmation#215

Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
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.

2 participants