-
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
feat: integrate with verification attempt events #226
Changes from all commits
d59f7e6
23c4243
aacb6cd
576f04c
81a41c3
932e817
220af76
010ced2
2596ec4
c1ebde1
c0b5184
8e0d3d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,4 +2,4 @@ | |
Django app housing name affirmation logic. | ||
""" | ||
|
||
__version__ = '2.4.2' | ||
__version__ = '3.0.0' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,58 +43,64 @@ def idv_update_verified_name_task(self, attempt_id, user_id, name_affirmation_st | |
# want to grab all verified names for the same user and name combination, because | ||
# some of those records may already be associated with a different IDV attempt. | ||
verified_names = VerifiedName.objects.filter( | ||
(Q(verification_attempt_id=attempt_id) | Q(verification_attempt_id__isnull=True)) | ||
(Q(platform_verification_attempt_id=attempt_id) | Q(platform_verification_attempt_id__isnull=True)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. both here and with delete this library now assumes any future idv change is related to the new model, we've completely dropped support for |
||
& Q(user__id=user_id) | ||
& Q(verified_name=photo_id_name) | ||
).order_by('-created') | ||
verified_names_updated = False | ||
if verified_names: | ||
# if there are VerifiedName objects, we want to update existing entries | ||
# for each attempt with no attempt id (either proctoring or idv), update attempt id | ||
updated_for_attempt_id = verified_names.filter( | ||
proctored_exam_attempt_id=None, | ||
verification_attempt_id=None | ||
).update(verification_attempt_id=attempt_id) | ||
verification_attempt_id=None, | ||
platform_verification_attempt_id=None | ||
).update(platform_verification_attempt_id=attempt_id) | ||
|
||
if updated_for_attempt_id: | ||
verified_names_updated = True | ||
log.info( | ||
'Updated VerifiedNames for user={user_id} to verification_attempt_id={attempt_id}'.format( | ||
'Updated VerifiedNames for user={user_id} to platform_verification_attempt_id={attempt_id}'.format( | ||
user_id=user_id, | ||
attempt_id=attempt_id, | ||
) | ||
) | ||
|
||
# then for all matching attempt ids, update the status | ||
verified_name_qs = verified_names.filter( | ||
verification_attempt_id=attempt_id, | ||
platform_verification_attempt_id=attempt_id, | ||
verification_attempt_id=None, | ||
proctored_exam_attempt_id=None | ||
) | ||
|
||
# Individually update to ensure that post_save signals send | ||
for verified_name_obj in verified_name_qs: | ||
verified_name_obj.status = name_affirmation_status | ||
verified_name_obj.save() | ||
verified_names_updated = True | ||
|
||
log.info( | ||
'Updated VerifiedNames for user={user_id} with verification_attempt_id={attempt_id} to ' | ||
'Updated VerifiedNames for user={user_id} with platform_verification_attempt_id={attempt_id} to ' | ||
'have status={status}'.format( | ||
user_id=user_id, | ||
attempt_id=attempt_id, | ||
status=name_affirmation_status | ||
) | ||
) | ||
else: | ||
# otherwise if there are no entries, we want to create one. | ||
|
||
# if there are no entries to update, we want to create one. | ||
if not verified_names_updated: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This actually fixes what appears to be an existing bug. If a user has a previously verified name using a different method (eg proctoring) and an new IDV creation event is emitted a new verified name record is not created. The event just gets dropped. From a user standpoint this doesn't really happen since the UI will post a blank name before that event but it still seemed like a gap. |
||
user = User.objects.get(id=user_id) | ||
verified_name = VerifiedName.objects.create( | ||
user=user, | ||
verified_name=photo_id_name, | ||
profile_name=full_name, | ||
verification_attempt_id=attempt_id, | ||
platform_verification_attempt_id=attempt_id, | ||
status=name_affirmation_status, | ||
) | ||
log.error( | ||
'Created VerifiedName for user={user_id} to have status={status} ' | ||
'and verification_attempt_id={attempt_id}, because no matching ' | ||
'and platform_verification_attempt_id={attempt_id}, because no matching ' | ||
'attempt_id or verified_name were found.'.format( | ||
user_id=user_id, | ||
attempt_id=attempt_id, | ||
|
@@ -187,23 +193,23 @@ def proctoring_update_verified_name_task( | |
bind=True, autoretry_for=(Exception,), default_retry_delay=DEFAULT_RETRY_SECONDS, max_retries=MAX_RETRIES, | ||
) | ||
@set_code_owner_attribute | ||
def delete_verified_name_task(self, idv_attempt_id, proctoring_attempt_id): | ||
def delete_verified_name_task(self, platform_verification_attempt_id, proctoring_attempt_id): | ||
""" | ||
Celery task to delete a verified name based on an idv or proctoring attempt | ||
""" | ||
# this case shouldn't happen, but should log as an error in case | ||
if (idv_attempt_id and proctoring_attempt_id) or (not idv_attempt_id and not proctoring_attempt_id): | ||
if (proctoring_attempt_id, platform_verification_attempt_id).count(None) != 1: | ||
log.error( | ||
'A maximum of one attempt id should be provided for either a proctored exam attempt or IDV attempt.' | ||
'A maximum of one attempt id should be provided' | ||
) | ||
return | ||
|
||
log_message = {'field_name': '', 'attempt_id': ''} | ||
|
||
if idv_attempt_id: | ||
verified_names = VerifiedName.objects.filter(verification_attempt_id=idv_attempt_id) | ||
log_message['field_name'] = 'verification_attempt_id' | ||
log_message['attempt_id'] = idv_attempt_id | ||
if platform_verification_attempt_id: | ||
verified_names = VerifiedName.objects.filter(platform_verification_attempt_id=platform_verification_attempt_id) | ||
log_message['field_name'] = 'platform_verification_attempt_id' | ||
log_message['attempt_id'] = platform_verification_attempt_id | ||
else: | ||
verified_names = VerifiedName.objects.filter(proctored_exam_attempt_id=proctoring_attempt_id) | ||
log_message['field_name'] = 'proctored_exam_attempt_id' | ||
|
@@ -212,10 +218,10 @@ def delete_verified_name_task(self, idv_attempt_id, proctoring_attempt_id): | |
if verified_names: | ||
log.info( | ||
'Deleting {num_names} VerifiedName(s) associated with {field_name}=' | ||
'{verification_attempt_id}'.format( | ||
'{platform_verification_attempt_id}'.format( | ||
num_names=len(verified_names), | ||
field_name=log_message['field_name'], | ||
verification_attempt_id=log_message['attempt_id'], | ||
platform_verification_attempt_id=log_message['attempt_id'], | ||
) | ||
) | ||
verified_names.delete() | ||
|
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 is only ever triggered by a
SoftwareSecurePhotoVerification
and can removed since we will no longer be creating those records.Why remove this?
We run
idv_update_verified_name_task
on an attempt id which is not unique between the two data models making it ambiguous as to what field the attempt id from the LMS is really for. While we could start passing two fields (type, id) throughout the service that's going to be messy and type serves no purpose once SoftwareSecurePhotoVerification support is deprecated.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.
Question: Do we need approval from Axim for this? I know we believe no one uses Software Secure outside of 2U, but if it's not deprecated, I'm a little concerned about breaking this flow. The Sumac release is being cut on 10/23/2024, so this will make it into that release before the Software Secure integration is removed. Do you have any concerns about this?
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.
ahh this is a great point. I originally wrote this under the assumption this repo was not part of Open edX since 2U is the only one using it. But as you pointed out earlier this is a default dependency so maybe we need to reconsider here
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.
to close out this thread. We've decided to move forward with this as is. Given the following: