-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
60a0860
to
180fd69
Compare
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
9bd4e7d
to
d0cacdb
Compare
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.
I left a few comments with requested changes, but this looks good overall so far.
a3da5fe
to
33627a4
Compare
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. I added three nits that you can take or leave.
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 |
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.
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
.
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.
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.
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.
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.
95354e5
to
aa456e8
Compare
aa456e8
to
b8069b7
Compare
if id == self.idv_attempt_id: | ||
return self._obj({'status': self.idv_attempt_status}) | ||
|
||
return self._obj({'status': None}) |
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.
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.
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.
I'm not sure. I think it's fine to ignore!
edx/edx-name-affirmation#215 Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
edx/edx-name-affirmation#215 Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
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 theverification_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
JIRA:
COSMO-442🔒
Pre-Merge Checklist:
edx_name_affirmation/__init__.py
if these changes are to be released. See OEP-47: Semantic Versioning.CHANGELOG.rst
.Post-Merge: