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: rework idv cert trigger #35580

Merged
merged 2 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ def test_verification_signal(self):
"""
Verification signal is sent upon approval.
"""
with mock.patch('openedx_events.learning.signals.IDV_ATTEMPT_APPROVED.send_event') as mock_signal:
with mock.patch('openedx.core.djangoapps.signals.signals.LEARNER_SSO_VERIFIED.send_robust') as mock_signal:
# Begin the pipeline.
pipeline.set_id_verification_status(
auth_entry=pipeline.AUTH_ENTRY_LOGIN,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ workspace {

grades_app -> signal_handlers "Emits COURSE_GRADE_NOW_PASSED signal"
verify_student_app -> signal_handlers "Emits IDV_ATTEMPT_APPROVED signal"
verify_student_app -> signal_handlers "Emits LEARNER_SSO_VERIFIED signal"
student_app -> signal_handlers "Emits ENROLLMENT_TRACK_UPDATED signal"
allowlist -> signal_handlers "Emits APPEND_CERTIFICATE_ALLOWLIST signal"
signal_handlers -> generation_handler "Invokes generate_allowlist_certificate()"
Expand Down
27 changes: 21 additions & 6 deletions lms/djangoapps/certificates/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
from openedx.core.djangoapps.signals.signals import (
COURSE_GRADE_NOW_FAILED,
COURSE_GRADE_NOW_PASSED,
LEARNER_SSO_VERIFIED,
)
from openedx_events.learning.signals import EXAM_ATTEMPT_REJECTED, IDV_ATTEMPT_APPROVED

Expand Down Expand Up @@ -117,17 +118,13 @@ def _listen_for_failing_grade(sender, user, course_id, grade, **kwargs): # pyli
log.info(f'Certificate marked not passing for {user.id} : {course_id} via failing grade')


@receiver(IDV_ATTEMPT_APPROVED, dispatch_uid="learner_track_changed")
def _listen_for_id_verification_status_changed(sender, signal, **kwargs): # pylint: disable=unused-argument
def _handle_id_verification_approved(user):
"""
Listen for a signal indicating that the user's id verification status has changed.
Generate a certificate for the user if they are now verified
"""
if not auto_certificate_generation_enabled():
return

event_data = kwargs.get('idv_attempt')
user = User.objects.get(id=event_data.user.id)

user_enrollments = CourseEnrollment.enrollments_for_user(user=user)
expected_verification_status = IDVerificationService.user_status(user)
expected_verification_status = expected_verification_status['status']
Expand All @@ -145,6 +142,24 @@ def _listen_for_id_verification_status_changed(sender, signal, **kwargs): # pyl
)


@receiver(LEARNER_SSO_VERIFIED, dispatch_uid="sso_learner_verified")
def _listen_for_sso_verification_approved(sender, user, **kwargs): # pylint: disable=unused-argument
"""
Listen for a signal on SSOVerification indicating that the user has been verified.
"""
_handle_id_verification_approved(user)


@receiver(IDV_ATTEMPT_APPROVED, dispatch_uid="openedx_idv_attempt_approved")
def _listen_for_id_verification_approved_event(sender, signal, **kwargs): # pylint: disable=unused-argument
"""
Listen for an openedx event indicating that the user's id verification status has changed.
"""
event_data = kwargs.get('idv_attempt')
user = User.objects.get(id=event_data.user.id)
_handle_id_verification_approved(user)


@receiver(ENROLLMENT_TRACK_UPDATED)
def _listen_for_enrollment_mode_change(sender, user, course_key, mode, **kwargs): # pylint: disable=unused-argument
"""
Expand Down
6 changes: 3 additions & 3 deletions lms/djangoapps/certificates/tests/test_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
from lms.djangoapps.certificates.models import GeneratedCertificate
from lms.djangoapps.certificates.signals import (
_listen_for_enrollment_mode_change,
_listen_for_id_verification_status_changed,
_handle_id_verification_approved,
listen_for_passing_grade
)
from lms.djangoapps.certificates.tests.factories import CertificateAllowlistFactory
Expand Down Expand Up @@ -272,15 +272,15 @@ def test_listen_for_passing_grade(self):
mock.Mock(return_value={"status": "approved"})
)
@mock.patch("lms.djangoapps.certificates.api.auto_certificate_generation_enabled", mock.Mock(return_value=True))
def test_listen_for_id_verification_status_changed(self):
def test_handle_id_verification_approved(self):
"""
Test stop certificate generation process after the verification status changed by raising a filters exception.

Expected result:
- CertificateCreationRequested is triggered and executes TestStopCertificateGenerationStep.
- The certificate is not generated.
"""
_listen_for_id_verification_status_changed(None, self.user)
_handle_id_verification_approved(self.user)

self.assertFalse(
GeneratedCertificate.objects.filter(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def test_performance(self):
#self.assertNumQueries(100)

def test_signal_called(self):
with patch('openedx_events.learning.signals.IDV_ATTEMPT_APPROVED.send_event') as mock_signal:
with patch('openedx.core.djangoapps.signals.signals.LEARNER_SSO_VERIFIED.send_robust') as mock_signal:
call_command('backfill_sso_verifications_for_old_account_links', '--provider-slug', self.provider.provider_id) # lint-amnesty, pylint: disable=line-too-long
assert mock_signal.call_count == 1

Expand Down
48 changes: 16 additions & 32 deletions lms/djangoapps/verify_student/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,8 @@
rsa_decrypt,
rsa_encrypt
)
from openedx.core.djangoapps.signals.signals import LEARNER_SSO_VERIFIED
from openedx.core.storage import get_storage
from openedx_events.learning.signals import IDV_ATTEMPT_APPROVED
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is independent of your changes here, but this removal got me thinking about whether it's enough for this event to be emitted from the API. I'm wondering if we should also emit these events when the model is saved to ensure that these events are logged when the instances are modified a different way (e.g. via the admin). Do you have any thoughts about that since you've worked so much with the signals and handlers in this app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I also saw this and do think it's problematic. We should be firing events like this as a result of the model changing. Maybe not needed to release but we should clean it up before the inevitable bug/confusion when this record gets saved by another method

from openedx_events.learning.data import UserData, VerificationAttemptData

from .utils import auto_verify_for_testing_enabled, earliest_allowed_verification_date, submit_request_to_ss

Expand Down Expand Up @@ -249,23 +248,13 @@ def send_approval_signal(self, approved_by='None'):
user_id=self.user, reviewer=approved_by
))

# Emit event to find and generate eligible certificates
verification_data = VerificationAttemptData(
attempt_id=self.id,
user=UserData(
pii=None,
id=self.user.id,
is_active=self.user.is_active,
),
status=self.status,
name=self.name,
expiration_date=self.expiration_datetime,
)
IDV_ATTEMPT_APPROVED.send_event(
idv_attempt=verification_data,
# Emit signal to find and generate eligible certificates
LEARNER_SSO_VERIFIED.send_robust(
sender=PhotoVerification,
user=self.user,
)

message = 'IDV_ATTEMPT_APPROVED signal fired for {user} from SSOVerification'
message = 'LEARNER_SSO_VERIFIED signal fired for {user} from SSOVerification'
log.info(message.format(user=self.user.username))


Expand Down Expand Up @@ -463,23 +452,18 @@ def approve(self, user_id=None, service=""):
)
self.save()

# Emit event to find and generate eligible certificates
verification_data = VerificationAttemptData(
attempt_id=self.id,
user=UserData(
pii=None,
id=self.user.id,
is_active=self.user.is_active,
),
status=self.status,
name=self.name,
expiration_date=self.expiration_datetime,
)
IDV_ATTEMPT_APPROVED.send_event(
idv_attempt=verification_data,
# Emit signal to find and generate eligible certificates
# This model uses the LEARNER_SSO_VERIFIED signal for backwards compatibility with changes
# to existing records. This model is being deprecated and all future IDV records
# will use the new VerificationAttempt model and corresponding openedx events signal.
# This is temporary until this model can be removed.
# DEPR details: https://github.com/openedx/edx-platform/issues/35128
LEARNER_SSO_VERIFIED.send_robust(
Copy link
Contributor

Choose a reason for hiding this comment

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

I read through your explanation of this choice, but I'm still a little unsure of why this change is in the PhotoVerification model and not the SSOVerification model. I'll try explaining it in the way that makes sense to me - could you let me know if I have this right?

PhotoVerification is the ABC of the SoftwareSecurePhotoVerification. SoftwareSecurePhotoVerification approvals should continue to be caught by the certificates application. Because the approve method is on the PhotoVerification model, that's where we emit this event.

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the case, I think LEARNER_SSO_VERIFIED is a confusing name. I'm kind of torn on it, especially because who knows when this model will be deprecated and removed. If the intention is to use the name that will make the most sense in the future when PhotoVerification is deprecated, I think a brief ADR codifying this choice would be helpful. Alternatively, we could use a more generic name and leave the renaming from the generic name to LEARNER_SSO_VERIFIED to whomever refactors this codebase during deprecation. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's correct, it's where this event has always been emitted.

I'm kinda stumped on the name because if I call it something generic (like learner_verified) is also becomes wrong in the sense that it doesn't actually capture all cases of learner_verified just this one off instance. I'll think on this more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay after mulling on this, I can just create a separate event here for PhotoVerification that uses the same cert handler. Less confusion, and the 2nd can just be deleted up during the DEPR. @MichaelRoytman thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's a good plan!

sender=PhotoVerification,
user=self.user,
)

message = 'IDV_ATTEMPT_APPROVED signal fired for {user} from PhotoVerification'
message = 'LEARNER_SSO_VERIFIED signal fired for {user} from PhotoVerification'
log.info(message.format(user=self.user.username))

@status_before_must_be("ready", "must_retry")
Expand Down
4 changes: 4 additions & 0 deletions openedx/core/djangoapps/signals/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,9 @@
# ]
COURSE_GRADE_NOW_FAILED = Signal()

# Signal that indicates that a user has become verified via SSO for certificate purposes
# providing_args=['user']
LEARNER_SSO_VERIFIED = Signal()

# providing_args=['user']
USER_ACCOUNT_ACTIVATED = Signal() # Signal indicating email verification
Loading