LG-6897/LG-6868/LG-6437: Set pending profile state for IPP by aduth · Pull Request #6634 · 18F/identity-idp

aduth · 2022-07-26T19:56:59Z

Why: So that a user's profile is not activated until they complete the in-person enrollment, and any attempts to revisit the identity verification will present them with the instructions to proof in-person (after GPO confirmation if applicable).

Testing Instructions:

Navigate to http://localhost:3000
Sign in
Navigate to http://localhost:3000/verify
Complete proofing flow up to document capture
Use attention result YAML test document as file upload
Click Submit
Click troubleshooting option to "Verify your ID in person at a Post Office"
Complete proofing flow
1. Optional: Choose to verify address by mail (click "Verify your address by mail instead" on the "Enter a phone number with your name on the plan" step)
After the "You're ready to verify in person", return to account page by clicking the logo
Navigate to http://localhost:3000/verify
Observe that you see the "You're ready to verify in person" instructions

Screen recordings:

Finalizing GPO redirects to in person instructions:

gpo-finalization.mov

Navigating to /verify with pending profile shows instructions:

go-to-verify.mov

**Why**: So that a user who is directed to verify their identity while already having begun an in-person proofing attempt will have the opportunity to view the instructions to proof in person again. changelog: Upcoming Features, In-person proofing, Show in-person instructions on repeat visits

sheldon-b

Looks great!

sheldon-b · 2022-07-27T01:46:58Z

app/forms/api/profile_creation_form.rb

      if user.present?
        @extra_attributes ||= {
-          profile_pending: user.pending_profile?,
+          profile_pending: user.pending_profile? && user_bundle.gpo_address_verification?,


Is this essentially disambiguating pending profiles due to in-person proofing and due to GPO verification?

Yeah, in earlier iterations I had tried to avoid adding a new deactivation reason and reusing the existing verification_pending, disambiguating based on the profile address verification method. However, this couldn't account for a GPO + IPP profile, where we'd have an issue that we'd need to be able to mark the profile as GPO-complete + IPP-incomplete, something which didn't seem possible without the new deactivation reason.

So technically this change might not be necessary any longer. I could go either way about keeping it, since technically the other changes here sorta lean into the idea that "pending profile" = "gpo". On the other hand, there is some other existing code that this helps aligns toward:

identity-idp/app/controllers/idv/personal_key_controller.rb

Lines 36 to 37 in b25790b

if pending_profile? && idv_session.address_verification_mechanism == 'gpo'

idv_come_back_later_url

sheldon-b · 2022-07-27T01:53:06Z

app/models/profile.rb

    encryption_error: 2,
    verification_pending: 3,
    verification_cancelled: 4,
+    in_person_verification_pending: 5,


Oh nice this makes sense

Since the enums are plain ints in the Db, what about renaming 3 to gpo_verification_pending?

Since the enums are plain ints in the Db, what about renaming 3 to gpo_verification_pending?

Yeah, I think that makes sense. Along the same lines, in an earlier iteration I had also renamed the UserDecorator#pending_profile_requires_verification? method to pending_profile_requires_gpo_verification?. I have a feeling there's a lot of different references to this "pending" state, so I may want to be a bit conservative with those updates for now. I think renaming the deactivation reason may be a good start for now at least.

Also, it may be worth noting that, in practice, "verification pending" is used exclusively to mean GPO verification, the process of creating and activating a profile does seem to treat it as a bit more generic, i.e. any new profile is pending by default until activated (see Idv::ProfileMaker#save_profile).

I wonder if we'd even need the deactivation_reason for the initialization of the profile? Or if we could just assume active: false would suffice on its own, and the GPO proofing path would be responsible for assigning that deactivation reason.

I didn't realize that profiles start as pending like that. That makes me think maybe we would want to keep a single pending enum and maybe add a separate enum column for what it's waiting on (gpo, ipp, etc), unsure if deactivation reason is a super good fit for that? But it is already there.

That's what I was getting at with the second paragraph of my previous comment. It seems like we already have the two-field "pending" + "what it's waiting on" via active: false + deactivation_reason:. Currently, we initialize an active: false profile with deactivation_reason: :verification_pending, but I'm not sure there's a reason we have to?

In other words:

# New profile Profile.new(active: false, deactivation_reason: nil) # GPO verification profile Profile.new(active: false, deactivation_reason: :gpo_verification_pending) # IPP verification profile Profile.new(active: false, deactivation_reason: :in_person_verification_pending) # Active profile Profile.new(active: true, deactivation_reason: nil)

I pushed what I had in mind in 3dec5e2 . Also worth noting that with the logic flow for initializing a profile, the profile would immediately be either activated or assigned deactivation_reason: :gpo_verification_pending or deactivation_reason: :in_person_verification_pending depending on the route the user had selected during the proofing attempt.

identity-idp/app/controllers/idv/review_controller.rb

Lines 89 to 91 in b25790b

idv_session.create_profile_from_applicant_with_password(password)

idv_session.cache_encrypted_pii(password)

idv_session.complete_session

Because of this, we could even consider changing save_profile to accept deactivation_reason as an optional argument and assign it at creation, which could have the added advantage of avoiding a second database update.

Yeah that seems the clearest overall. And yes also updating save_profile to save a round trip seems worthwhile as well, maybe for a follow up PR if this one is getting bogged down

I'm going to merge this and follow-on with a few other improvements (more exhaustive specs, etc). I may do it there or in a follow-on-follow-on.

I'm going to merge this and follow-on with a few other improvements (more exhaustive specs, etc). I may do it there or in a follow-on-follow-on.

For posterity: #6680, #6681

sheldon-b · 2022-07-27T01:54:41Z

app/services/idv/session.rb

+      if phone_confirmed?
+        if pending_in_person_enrollment?
+          current_user.pending_profile&.deactivate(:in_person_verification_pending)
+        else
+          complete_profile
+        end
+      end


spec/features/idv/in_person_spec.rb

Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov>

See: #6634 (comment)

So that we don't have to worry about "initial pending" being conflated with "gpo verification pending", and instead defer to `active: false` as indicator See: #6634 (comment)

* Create enrollment even in FSM v1 config * Fix broken helper * LG-6437: Show "Ready to verify" page for pending IPP profile **Why**: So that a user who is directed to verify their identity while already having begun an in-person proofing attempt will have the opportunity to view the instructions to proof in person again. changelog: Upcoming Features, In-person proofing, Show in-person instructions on repeat visits * Use correct user pii session data * Ensure IPP profiles don't get activated in fsm v1 * LG-6897/LG-6868/LG-6437: Set pending profile state for IPP * Fix lint failure changelog: Upcoming features, In-person proofing, enroll users in USPS IPP in FSM v1 flow * Fix "clear and start over" with pending enrollment * Add specs for IPP users * Assert that account is not verified * Fix password confirm controller spec considering GPO pending * Move enrollment-creation to profile creation form * Clarify reproofing instruction spec comment Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov> * Rename verification_pending to gpo_verification_pending See: #6634 (comment) * Initialize profile with nil deactivation_reason So that we don't have to worry about "initial pending" being conflated with "gpo verification pending", and instead defer to `active: false` as indicator See: #6634 (comment) * Use proofing component to check IPP status * Remove conflict error message * Remove lint error message * Fix merge conflicts * Use proofing component instead of enrollment * Save same_address_as_id to PII * Stub PII using user profile * Mock user session for profile creation form * Fix line length lint error * Use pii from session instead of user_session * Only check the current GPO profile for IPP Only checks the current pending profile for proofing components related to IPP, just in case the user manages to create additional profiles while they are awaiting GPO verification Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

aduth added 2 commits July 26, 2022 15:56

LG-6897/LG-6868/LG-6437: Set pending profile state for IPP

f916589

aduth changed the title ~~LG-6437: Show "Ready to verify" page for pending IPP profile~~ LG-6897/LG-6868/LG-6437: Set pending profile state for IPP Jul 27, 2022

aduth added 3 commits July 26, 2022 20:38

Fix "clear and start over" with pending enrollment

6afc73a

Assert that account is not verified

3df32b5

Fix password confirm controller spec considering GPO pending

96db7ca

aduth marked this pull request as ready for review July 27, 2022 01:18

aduth requested review from a team and NavaTim July 27, 2022 01:18

sheldon-b approved these changes Jul 27, 2022

View reviewed changes

aduth and others added 4 commits July 26, 2022 22:38

Clarify reproofing instruction spec comment

8c09373

Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov>

Rename verification_pending to gpo_verification_pending

c2369c7

See: #6634 (comment)

Initialize profile with nil deactivation_reason

3dec5e2

So that we don't have to worry about "initial pending" being conflated with "gpo verification pending", and instead defer to `active: false` as indicator See: #6634 (comment)

Add feature flag guard for IPP

7d20b7a

aduth merged commit cd464e7 into main Jul 27, 2022

aduth deleted the aduth-lg-6437-verify-entry-ipp branch July 27, 2022 16:59

This was referenced Aug 2, 2022

Improve test coverage for in-person proofing #6680

Merged

Collapse profile creation to single method #6681

Merged

solipet mentioned this pull request Aug 9, 2022

Deploy RC 202 to Prod #6688

Merged

	if pending_profile? && idv_session.address_verification_mechanism == 'gpo'
	idv_come_back_later_url

	idv_session.create_profile_from_applicant_with_password(password)
	idv_session.cache_encrypted_pii(password)
	idv_session.complete_session

Conversation

aduth commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sheldon-b 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!

aduth Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aduth commented Jul 26, 2022 •

edited

Loading

aduth Jul 27, 2022 •

edited

Loading

aduth Jul 27, 2022 •

edited

Loading