-
Notifications
You must be signed in to change notification settings - Fork 121
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
LG-14522 Enable password reset for pending in-person profiles #11762
base: main
Are you sure you want to change the base?
Conversation
47ac630
to
05cb0a4
Compare
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 am still trying to understand what pending_profile_invalidated
means in the password reset event. Does anyone know what this is and I am wondering how I should update this event when we include pending in-person profile
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.
bat signal to @jmhooper
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.
That is there to give us a sense for how often a user reseting their password invalidates their pending profile. Prior to this change the presence of a pending profile indicated that. After this change the presence of a pending profile that is not pending in-person will indicate that.
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.
In response to this feedback, I updated the value for pending_profile_invalidated
in commit 9f7abb2.
Can you describe what's unique about password reset with pending in-person proofing profiles that isn't also true of other pending profiles (verification-by-mail, fraud review)? I'm wondering if the behavior here should apply to any pending profile, not specifically in-person profiles. |
if password_reset? | ||
transaction do | ||
update!(deactivation_reason: nil) | ||
activate(reason_deactivated: :password_reset) if activated_at.present? |
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.
What's the difference between activated_at
and verified_at
? (Wanting to ensure we don't risk activating IPP profiles where the user hasn't actually completed the required steps)
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 am not sure what the difference is, but the logic I am following is consistent with the user model methods around active profiles. Does anyone from team Ada have insight on 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.
I am going to (try to) spare you a rant about how I think the Profile model is way more complicated than it needs to be, and how it should just present a simple outside interface rather than burdening users with a bunch of timestamps having nuanced meanings, but oops there I go.
The Profile#activate method sheds some light here. Calling activate
looks to always set active: true
and activated_at: now
, but conditionally assigns verified_at
:
attrs[:verified_at] = now unless reason_deactivated == :password_reset || verified_at
I suspect (but will readily defer to Hooper or Hinz) that this is meant to reflect that a profile can be deactivated and reactivated, but not "de-verified". If we're (re)activating an account that had been deactivated due to password reset, or that already had verified_at
set, we leave that timestamp alone.
So what you're doing here makes sense to me.
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 agree that this is probably the correct approach. verified_at
is intended to reflect when the profile completed verification. activate_at
is when the profile was last made the active profile.
I don't think there is a big difference between them, but team Joy was wanting to narrow the scope of this story. One difference that I can think of is that during in-person proofing the user receives their personal key so that a password reset can be performed using the personal key. I think that is biggest difference between IPP and the GPO flow. I do think I agree it would be nice to expand it, however it would increase the testing effort and the scope of this story. Not sure how much scope we are willing to take on. cc: @eileen-nava |
Noting a couple of things based on my most recent push to this branch @eileen-nava and I found the following issue when testing with a legacy active and a pending facial match required profile.
Note: I also took some time to rewrite the tests in
|
changelog: User-facing Improvement, In-person Proofing, Allow users to perform a password reset on pending in-person profiles without losing progress if the user supplies their personal key.
… required Reproduce by creating an IPP enrollment arriving at the ready to verify page and logout. Then log back in you will land on the welcome page. This is incorrect because the user has a pending in person enrollment.
The personal key only works for the most recent profile that had a personal key generated
7bb7a4f
to
ab6ef8f
Compare
I fixed the issue in 1. I found on Friday and fixed the build pipeline. I am still waiting for more feedback on this PR. |
) | ||
cancel_enrollment(enrollment) | ||
if profile_deactivation_reason == 'password_reset' | ||
skip_enrollment(enrollment, profile_deactivation_reason) |
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 want to confirm that the enrollments getting skipped won't be skipped forever. ie: If someone resets their pw but does not sign in- I want to ensure these get expired/cancelled after so long. Updated: I checked, these enrollments would expire eventually at the Post Office but we currently don't have anything in place to handle this on our side as is - they would remain and queue and be skipped.
Update: Shane and I talked about this. He is going to move where this check happens to prevent this.
@@ -176,13 +176,14 @@ def activate_after_passing_in_person | |||
end | |||
end | |||
|
|||
def activate_after_password_reset |
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.
Do you need to delete this in a follow up PR for 50/50 state now that is is no longer being read in app/forms/verify_password_form.rb?
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 don't think that this would cause 50/50 state problems. If anyone disagrees or if I'm missing something, please let me know.
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.
@gina-yamada I am responding in thread here because I can't respond in thread to your bigger comment. I agree with wanting to handle the warning banner on the password reset page. I made two tickets for the work.
🎟️ LG-15624: Hide the warning banner on the password reset page when feature_pending_in_person_password_reset_enabled is true
🎟️ LG-15625: After enabling ID-IPP password reset, delete the warning banner on the password reset page
Allow enrollments with password reset profiles to be updated when the enrollment is not passed/failed. So that enrollments can expire when still in password reset.
@@ -131,7 +130,11 @@ def check_enrollment(enrollment) | |||
rescue StandardError => err | |||
handle_standard_error(err, enrollment) | |||
else | |||
process_enrollment_response(enrollment, response) | |||
if profile_deactivation_reason == 'password_reset' |
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.
Is the idea here that we will continue fetching this enrollments status until the user recovers this profile? Do these get expired eventually? Some users will never come back and recover.
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.
Yes. I had this same concern. Previously skip_enrollment was being called at the beginning of check_enrollment here. The enrollment would not get expired because we didn't check them with USPS. Shane moved skip_enrolment so they would.
Path: If an enrollment expired with USPS, we get a 400 status code with response message "More than 30 days have passed since opt-in to IPP" so we hit handle_bad_request_error above. We then hit elsif match for IPP_EXPIRED_ERROR_MESSAGE. The flag is false in app.yml.default. In AWS- not set in all env except prod where it is false. Code updates enrollment (status: expired and sets state check completed at).
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.
Overall this looks to me like is should work. I left a quick question about how the job works. |
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 manually tested with the steps you provided. Everything behaves as described! Looks great! Great job with all this, especially all the test coverage!
🎫 Ticket
Link to the relevant ticket:
LG-14522
🛠 Summary of changes
Allow users to reset their password with an in-person pending profile/enrollment without losing progress when their personal key is provided. This feature is setup with a feature flag so it can be rolled out slowly.
📜 Testing Plan
Scenario: When pending in-person password reset is enabled and User uses personal key
Set the feature_pending_in_person_password_reset_enabled flag to true
Scenario: When pending in-person password reset is enabled and User does not use personal key
Set the feature_pending_in_person_password_reset_enabled flag to true
Scenario: When pending in-person password reset is disabled
Set the feature_pending_in_person_password_reset_enabled flag to false