-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Changes from 1 commit
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 |
---|---|---|
|
@@ -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 | ||
from openedx_events.learning.data import UserData, VerificationAttemptData | ||
|
||
from .utils import auto_verify_for_testing_enabled, earliest_allowed_verification_date, submit_request_to_ss | ||
|
||
|
@@ -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)) | ||
|
||
|
||
|
@@ -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( | ||
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. I read through your explanation of this choice, but I'm still a little unsure of why this change is in the
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. If that's the case, I think 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. 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 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. 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? 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. 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") | ||
|
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 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?
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.
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