LG-13864 Create profile after USPS enrollment by shanechesnutt-ft · Pull Request #11034 · 18F/identity-idp

shanechesnutt-ft · 2024-08-06T14:14:52Z

🎫 Ticket

🛠 Summary of changes

Change the order of events when the enter password form is submitted in the IPP flow. The USPS enrollment request will be made before a profile is created. Currently, when the USPS enrollment request fails the following issues occur:

Users are no longer about to retry enter password form because the IdvStepConcern before action no_pending_profile check automatically navigates the user back to the account page.
Users are stuck in a loop in account page where they cannot re-attempt verification.

These issues stem from the user having a profile with an enrollment in the establishing state after a USPS enrollment failure occurs.

📜 Testing Plan

Scenario: USPS enrollment failure during ID-IPP enrollment

Login through the oidc sinatra application selecting the Identity Verified level of service.
Create a new account
Continue through the in-person enrollment flow
Once on the /verify/in_person/state_id page use the first name of usps client error when filling out the form and submit
Continue through the flow until reaching the /verify/enter_password
Re-enter password and submit the form
Verify that the There was an internal error processing your request. Please try again. error message appears on the /verify/enter_password page and you are able to re-enter your password.

Scenario: User drops off after USPS enrollment failure during ID-IPP enrollment

n1zyy

This looks good to me. A profile is meant to represent a proofing attempt. IMHO it's fair to say that if the proofing attempt never really got off the ground, we won't create the profile in the first place.

I wouldn't mind getting @jmhooper or @matthinz to weigh in on this as they have much deeper experience with Profiles, but it's a yes from me.

I'm leaving a comment voicing support vs. giving an approval only because I haven't tested this and Team Joy is awesome about doing thorough testing.

My two inline comments are unrelated to the core issue here and totally optional.

n1zyy · 2024-08-07T14:38:22Z

app/services/idv/session.rb

Unimportant nit: I think these two lines read kind of confusingly: "If the user already as an in-person enrollment, schedule an in-person enrollment for them." I'm not sure if there's an easy way to fix this with a rename, though?

I also find this confusing. Maybe we could rename has_in_person_enrollment? to has_unscheduled_in_person_enrollment?

I agree with that the naming could be better... my only issue is that the has_in_person_enrollment? checks for pending or establishing in-person enrollments. Unscheduled should only care about establishing in-person enrollments right?

Would needs_in_person_verification? be a clearer method name? I got that idea from the in_person_verification_needed param for profile_maker’s save_profile method.

It could also be renamed has_begun_in_person_proofing?, but I worry that’s too vague. Technically, a user who has a failed in_person_enrollment has also begun in person proofing.

Unscheduled should only care about establishing in-person enrollments right?

I have a n00b question. If unscheduled only refers to establishing in_person_enrollments, why schedule an enrollment when a user has a pending or establishing in_person_enrollment? Could this conditional be refactored to only check if the user has an establishing in_person_enrollment?

The enrollment helper’s schedule_in_person_enrollment returns if the user doesn’t have an establishing in_person_enrollment, which makes me think that this refactor wouldn’t actually change the method’s behavior.

Let me know if I’m missing something.

n1zyy · 2024-08-07T14:44:20Z

spec/features/idv/in_person_spec.rb

Idle musing: it's easy to miss what this is doing, and instead just skim this and see it as doing normal steps, in which case it's unclear why an error would be expected. We had experimented a bit with descriptively-named helper methods to make some of this clearer, though I don't know where that ended up. I wonder if it would make sense to put this in a method like complete_up_to_enter_password_step_with_usps_client_error and just call that in the before here? (It's totally possible this is a bad idea. Just mentioning it in case it resonates.)

Yeah I feel like it is unclear as well... I think this is because I was working around the fact that we are using the mock proofer in the tests. Maybe I should change the tests to use the real proofer, and mock the http endpoint. That would make the test setup have an explicit step for mocking the OptInIPPApplicant endpoint to return a 400 response.

Actually this is turning out to be a bigger fish than I want to fry right now... I ended up adding a comment above that line that states why the first_name is set to usps client error. I think it would be nice to improve these feature tests at a later date.

I wonder if it would make sense to put this in a method like complete_up_to_enter_password_step_with_usps_client_error and just call that in the before here?

I get wanting to make the test easier to follow. In the spirit of write everything twice, I wanted to point out that making a helper method which isn't used much could be a premature optimization.

matthinz

I agree with @n1zyy's assessment here: looks good to me! I will leave it to Team Joy to approve

eileen-nava

I left a lot of comments. 😅 Hope they're helpful! I was mostly curious about the conversation about potentially renaming current_user.has_in_person_enrollment?.

Test coverage looks good. Thanks for fixing this so quickly. 👏🏻

spec/services/idv/session_spec.rb

eileen-nava · 2024-08-07T22:45:45Z

spec/services/idv/session_spec.rb

Would you be up for calling the subject inside each it block, instead of in the before block? I tend to think of before blocks as a home for setup. It feels kind of odd to have what we're testing in the before block, rather than in the it block.

So I moved this to before block because I felt like it would dry up the it blocks a bit. My understanding is that before blocks are for both setup and execution of code under test. That being said I don't mind putting subject call in the it block it, it just feels like a lot of unnecessary duplication.

spec/services/idv/session_spec.rb

eileen-nava · 2024-08-07T22:50:05Z

spec/services/idv/session_spec.rb

Nit: Instead of having let(:profile) { subject.profile } on both L197 and L241, you could just define profile once at the top of the "with establishing in person enrollment" context block.

eileen-nava · 2024-08-07T22:56:17Z

spec/services/idv/session_spec.rb

Same comment as above: could the subject be called in the it block instead of in the before block?

eileen-nava · 2024-08-07T23:06:36Z

app/services/idv/session.rb

Would needs_in_person_verification? be a clearer method name? I got that idea from the in_person_verification_needed param for profile_maker’s save_profile method.

It could also be renamed has_begun_in_person_proofing?, but I worry that’s too vague. Technically, a user who has a failed in_person_enrollment has also begun in person proofing.

eileen-nava · 2024-08-07T23:19:08Z

app/services/idv/session.rb

Unscheduled should only care about establishing in-person enrollments right?

I have a n00b question. If unscheduled only refers to establishing in_person_enrollments, why schedule an enrollment when a user has a pending or establishing in_person_enrollment? Could this conditional be refactored to only check if the user has an establishing in_person_enrollment?

The enrollment helper’s schedule_in_person_enrollment returns if the user doesn’t have an establishing in_person_enrollment, which makes me think that this refactor wouldn’t actually change the method’s behavior.

Let me know if I’m missing something.

eileen-nava · 2024-08-07T23:25:23Z

spec/features/idv/in_person_spec.rb

I wonder if it would make sense to put this in a method like complete_up_to_enter_password_step_with_usps_client_error and just call that in the before here?

I get wanting to make the test easier to follow. In the spirit of write everything twice, I wanted to point out that making a helper method which isn't used much could be a premature optimization.

eileen-nava · 2024-08-07T23:26:03Z

spec/features/idv/in_person_spec.rb

Thanks for adding test coverage for this! 👏🏻

eileen-nava · 2024-08-07T23:26:48Z

spec/support/features/in_person_helper.rb

eileen-nava

Approved, good work. 👍🏻 (I think we should try and get another Joy engineer to approve this before we merge it, too. I'll post in slack.)

spec/services/idv/session_spec.rb

gina-yamada · 2024-08-09T14:12:21Z

@shanechesnutt-ft I worked through both scenarios in your testing plan. The flow is working as I'd expect now. I also went on to enter a good first name and eventually all the way to successfully create a barcode. Going to review code changes now (more to come) but this is looking good. As on-call engineer this week, I am very excited for this to go in. Thank you

I tested main. I am able to create a barcode when I use a random first name.

When I tested on main and use usps client error I got error undefined method due datefor nil inapp/presenters/account_show_presenter.rb` inside of
formatted_ipp_due_date.

This error does not occur on Shane's branch (after main was merged in)- it is fixed. Shane and I walked through this. Shane's logic will help with the behavior I observed and what looks like pesky bugs that have popped up.

WilliamBirdsall · 2024-08-09T14:46:27Z

Code LGTM. Added method on user seems helpful!

WilliamBirdsall

Ran through testing steps, looks good!

changelog: Internal, In-person Proofing, Ensure the USPS schedule enrollment request executes before a profile is created during the in-person proofing flow.

shanechesnutt-ft force-pushed the sc/LG-13864 branch 2 times, most recently from 00487b8 to a4824c2 Compare August 7, 2024 13:58

shanechesnutt-ft marked this pull request as ready for review August 7, 2024 14:08

shanechesnutt-ft requested review from a team and jennyverdeyen August 7, 2024 14:08

n1zyy reviewed Aug 7, 2024

View reviewed changes

matthinz reviewed Aug 7, 2024

View reviewed changes

eileen-nava reviewed Aug 7, 2024

View reviewed changes

shanechesnutt-ft force-pushed the sc/LG-13864 branch 3 times, most recently from 1a8fc6c to b4653c7 Compare August 8, 2024 19:56

eileen-nava approved these changes Aug 8, 2024

View reviewed changes

spec/services/idv/session_spec.rb Outdated Show resolved Hide resolved

shanechesnutt-ft force-pushed the sc/LG-13864 branch from b4653c7 to 434afb9 Compare August 9, 2024 14:12

WilliamBirdsall approved these changes Aug 9, 2024

View reviewed changes

gina-yamada self-requested a review August 9, 2024 15:51

LG-13864 Create profile after USPS enrollment

4a5e059

changelog: Internal, In-person Proofing, Ensure the USPS schedule enrollment request executes before a profile is created during the in-person proofing flow.

gina-yamada approved these changes Aug 9, 2024

View reviewed changes

shanechesnutt-ft force-pushed the sc/LG-13864 branch from 434afb9 to 4a5e059 Compare August 9, 2024 15:52

shanechesnutt-ft merged commit 9972472 into main Aug 9, 2024

shanechesnutt-ft deleted the sc/LG-13864 branch August 9, 2024 16:42

mitchellhenke mentioned this pull request Aug 13, 2024

Deploy RC 405 to Production #11074

Merged

Conversation

shanechesnutt-ft commented Aug 6, 2024 • edited by eileen-nava Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Uh oh!

n1zyy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthinz left a comment

Choose a reason for hiding this comment

Uh oh!

eileen-nava left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eileen-nava left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gina-yamada commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WilliamBirdsall commented Aug 9, 2024

Uh oh!

WilliamBirdsall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shanechesnutt-ft commented Aug 6, 2024 •

edited by eileen-nava

Loading

gina-yamada commented Aug 9, 2024 •

edited

Loading