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

feat: integrate with verification attempt events #226

Merged
merged 12 commits into from
Oct 4, 2024
5 changes: 5 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,13 @@ Change Log

Unreleased
~~~~~~~~~~

[3.0.0] - 2024-09-30
~~~~~~~~~~~~~~~~~~~~
* Add platform verification id field to the VerifiedName model
* Integrate platform verification id into app
* Added event handlers for new IDV events on the VerifiedName model
* Removed event handlers for SoftwareSecurePhotoVerification updates. This is a breaking change.

[2.4.0] - 2024-04-23
~~~~~~~~~~~~~~~~~~~~
Expand Down
2 changes: 1 addition & 1 deletion edx_name_affirmation/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
Django app housing name affirmation logic.
"""

__version__ = '2.4.2'
__version__ = '3.0.0'
8 changes: 2 additions & 6 deletions edx_name_affirmation/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,9 @@ class EdxNameAffirmationConfig(AppConfig):
'relative_path': 'handlers',
'receivers': [
{
'receiver_func_name': 'idv_attempt_handler',
'signal_path': 'lms.djangoapps.verify_student.signals.signals.idv_update_signal',
},
{
'receiver_func_name': 'idv_delete_handler',
'receiver_func_name': 'platform_verification_delete_handler',
'signal_path': 'django.db.models.signals.post_delete',
'sender_path': 'lms.djangoapps.verify_student.models.SoftwareSecurePhotoVerification',
'sender_path': 'lms.djangoapps.verify_student.models.VerificationAttempt',
},
{
'receiver_func_name': 'proctoring_attempt_handler',
Expand Down
89 changes: 50 additions & 39 deletions edx_name_affirmation/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@

import logging

from openedx_events.learning.signals import (
IDV_ATTEMPT_APPROVED,
IDV_ATTEMPT_CREATED,
IDV_ATTEMPT_DENIED,
IDV_ATTEMPT_PENDING
)

from django.contrib.auth import get_user_model
from django.db.models.signals import post_save
from django.dispatch.dispatcher import receiver
Expand Down Expand Up @@ -35,56 +42,60 @@
)


def idv_attempt_handler(attempt_id, user_id, status, photo_id_name, full_name, **kwargs):
Copy link
Contributor Author

@zacharis278 zacharis278 Sep 27, 2024

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.

Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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:

  1. name-affirmation is only used by 2U
  2. name-affirmation can drop support for SoftwareSecurePhotoVerifications prior to it's full removal in the LMS
  3. functionally this would work as it did before for any verifications using the new models

@receiver(IDV_ATTEMPT_APPROVED)
@receiver(IDV_ATTEMPT_CREATED)
@receiver(IDV_ATTEMPT_DENIED)
@receiver(IDV_ATTEMPT_PENDING)
def handle_idv_event(sender, signal, **kwargs): # pylint: disable=unused-argument
"""
Receiver for IDV attempt updates

Args:
attempt_id(int): ID associated with the IDV attempt
user_id(int): ID associated with the IDV attempt's user
status(str): status in IDV language for the IDV attempt
photo_id_name(str): name to be used as verified name
full_name(str): user's pending name change or current profile name
Trigger update to verified names based on open edX IDV events.
"""
trigger_status = VerifiedNameStatus.trigger_state_change_from_idv(status)

# only trigger celery task if status is relevant to name affirmation
if trigger_status:
log.info('VerifiedName: idv_attempt_handler triggering Celery task for user %(user_id)s '
'with photo_id_name %(photo_id_name)s and status %(status)s',
{
'user_id': user_id,
'photo_id_name': photo_id_name,
'status': status
}
)
idv_update_verified_name_task.delay(attempt_id, user_id, trigger_status, photo_id_name, full_name)
event_data = kwargs.get('idv_attempt')
user = User.objects.get(id=event_data.user.id)

# If the user has a pending name change, use that as the full name
try:
user_full_name = user.pending_name_change
except AttributeError:
user_full_name = event_data.user.pii.name

status = None
if signal == IDV_ATTEMPT_APPROVED:
status = VerifiedNameStatus.APPROVED
elif signal == IDV_ATTEMPT_CREATED:
status = VerifiedNameStatus.PENDING
elif signal == IDV_ATTEMPT_PENDING:
status = VerifiedNameStatus.SUBMITTED
elif signal == IDV_ATTEMPT_DENIED:
status = VerifiedNameStatus.DENIED
else:
log.info('VerifiedName: idv_attempt_handler will not trigger Celery task for user %(user_id)s '
'with photo_id_name %(photo_id_name)s because of status %(status)s',
{
'user_id': user_id,
'photo_id_name': photo_id_name,
'status': status
}
)
log.info(f'IDV_ATTEMPT {signal} signal not recognized') # driven by receiver decorator so should never happen
return

Check failure on line 73 in edx_name_affirmation/handlers.py

View workflow job for this annotation

GitHub Actions / Tests (ubuntu-20.04, 3.11, 10, django42)

Missing coverage

Missing coverage on lines 72-73

log.info(f'IDV_ATTEMPT {status} signal triggering Celery task for user {user.id} '
f'with name {event_data.name}')
idv_update_verified_name_task.delay(
event_data.attempt_id,
user.id,
status,
event_data.name,
user_full_name,
)

def idv_delete_handler(sender, instance, signal, **kwargs): # pylint: disable=unused-argument
"""
Receiver for IDV attempt deletions

Args:
attempt_id(int): ID associated with the IDV attempt
def platform_verification_delete_handler(sender, instance, signal, **kwargs): # pylint: disable=unused-argument
"""
Receiver for VerificationAttempt deletions
"""
idv_attempt_id = instance.id
platform_verification_attempt_id = instance.id
log.info(
'VerifiedName: idv_delete_handler triggering Celery task for idv_attempt_id=%(idv_attempt_id)s',
'VerifiedName: platform_verification_delete_handler triggering Celery task for '
'platform_verification_attempt_id=%(platform_verification_attempt_id)s',
{
'idv_attempt_id': idv_attempt_id,
'platform_verification_attempt_id': platform_verification_attempt_id,
}
)
delete_verified_name_task.delay(idv_attempt_id, None)
delete_verified_name_task.delay(platform_verification_attempt_id, None)


def proctoring_attempt_handler(
Expand Down
16 changes: 0 additions & 16 deletions edx_name_affirmation/statuses.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,22 +32,6 @@ class VerifiedNameStatus(str, Enum):
APPROVED = "approved"
DENIED = "denied"

@classmethod
def trigger_state_change_from_idv(cls, idv_status):
"""
Return the translated IDV status if it should trigger a state transition, otherwise return None
"""
# mapping from an idv status (key) to it's associated verified name status (value). We only want to
# include idv statuses that would cause a status transition for a verified name
idv_state_transition_mapping = {
'created': cls.PENDING,
'submitted': cls.SUBMITTED,
'approved': cls.APPROVED,
'denied': cls.DENIED
}

return idv_state_transition_mapping.get(idv_status, None)

@classmethod
def trigger_state_change_from_proctoring(cls, proctoring_status):
"""
Expand Down
44 changes: 25 additions & 19 deletions edx_name_affirmation/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 SoftwareSecurePhotoVerification changes.

& 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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand Down Expand Up @@ -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'
Expand All @@ -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()
Expand Down
Loading
Loading