Conversation
8fc57bb to
f2adb19
Compare
|
I added a |
f2adb19 to
825248e
Compare
eileen-nava
left a comment
There was a problem hiding this comment.
I left some comments.
I have an overall question about the AttemptsApi's events. When I'm working locally, I frequently run make watch_events in terminal to see what events are being logged. Is there a similar process I could run to see what events are being logged for the AttemptsApi?
| reproof = enrollment.user&.has_proofed_before? | ||
| enrollment.profile&.activate_after_passing_in_person | ||
|
|
||
| if enrollment.profile&.active? |
There was a problem hiding this comment.
Since L494 activates the profile associated with the enrollment, I'm not clear why the conditional on L496 is necessary. Is there a scenario where the profile associated with the enrollment wouldn't be active? Are you concerned about the transaction in activate_after_passing_in_person failing?
There was a problem hiding this comment.
both if it fails, or if an enrollment profile doesn't exist. i don't know what circumstances exist that a profile wouldn't exist on an enrollment, but i am taking my cues from the other code that adds the nil checker (such as enrollment.profile&.activate_after_passing_in_person, above)
| success = true | ||
|
|
||
| if profile.active? | ||
| attempts_api_tracker(profile:).idv_enrollment_complete(reproof:) |
There was a problem hiding this comment.
This looks good from an IPP perspective.
|
For IPP, above your wrote |
|
@gina-yamada it should log when the profile has been activated. @eileen-nava interesting suggestion! there is not a way to do that currently from the idp, but i can consider building that in when i have some free time again. meanwhile, the sample sinatra oidc application does have an |
mitchellhenke
left a comment
There was a problem hiding this comment.
Couple small questions, but otherwise looks good to me
| end | ||
|
|
||
| def init_profile | ||
| reproof = current_user.has_proofed_before? |
There was a problem hiding this comment.
I was going to ask if this could be moved into the conditional, but that wouldn't work because if we ran this check after creating a profile, it would always be true?
There was a problem hiding this comment.
yeah, exactly. i thought about checking to see if there was more than one profile that had the activated_at attribute, but i don't think what was happening was as clear
| end | ||
|
|
||
| if profile.active? | ||
| attempts_api_tracker.idv_enrollment_complete(reproof:) |
There was a problem hiding this comment.
Could this be moved below UserAlerts::AlertUserAboutAccountVerified.call? I think it might be preferable to run the other things first (in terms of importance).
I didn't check but the only edge case you may need to look into- if a user is flagged for fraud. They can be activated after the investigation concludes. It is not in the usps job but there is a script to activate users-I bet you already handled it. |
|
@gina-yamada thanks for the heads-up about that edge case! i believe it is handled in the |
That looks accurate! I should have looked at the code and posted if it was not there. I was replying to comments and then jumped back in. I agree - handled in ReviewPass class. I think you are set. Nice work. |
🎫 Ticket
Link to the relevant ticket:
163
🛠 Summary of changes
This change adds the
idv-enrollment-completeevent.This is a new event that was added to fulfill the
Enrollment completemilestone requirement.It is a little tricky since there are several different pathways for users to finish their identity proofing journey, two of which are not part of the user flow.
The places where we activate a profile (thereby indicating that a profile's enrollment is "complete") is:
EnterPasswordController- where a user encrypts their data with their passwordGpoVerifyForm- where a user submits their mail codeGetUSPSProofingResultsJob- where a user's IPP job is processedActionAccount::ReviewPass- where a user is cleared as valid after a fraud reviewI would love to know if I am missing or misinterpreting any of these steps, or if other work should. (I wish I could just put this event on the Profile model's
#activemethod, but there isn't a way really to inject the tracker into the Profile like that.)