LG-12631: handoff ipp by dawei-nava · Pull Request #10267 · 18F/identity-idp

dawei-nava · 2024-03-19T16:01:30Z

🎫 Ticket

Link to the relevant ticket:
LG-12631

Previous PR with wrong branch name: #10260

🛠 Summary of changes

IPP options on hybrid handoff page when IPP is available. User can select IPP and start document capture from handoff page on desktop.

A new skip_doc_auth_from_handoff flag is added for idv_session, simlar to skip_doc_auth which works with the optin page.

Test cases for configuration items combination:

📜 Testing Plan

Provide a checklist of steps to confirm the changes.
Scenario 1:

Login with an SP with IPP enabled and biometrics on desktop
Navigate to hybrid handoff page
Verify section for IPP option is visible
Click link to direct IPP and landed on document capture
Click back button and will be back on hybrid handoff page

Scenario 2:

Login with an SP with IPP enabled and no biometrics on desktop
Navigate to hybrid handoff page
Verify section for IPP option is not visible

Please refer the test cases spreadsheet, it may not be realistic to test all combinations in staging environment.

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Details

After:

IPP available and selfie required

English	Spanish	French

IPP available and NO selfie required

English	Spanish	French

IPP NOT available and Selfie required

English	Spanish	French

IPP NOT available and NO Selfie required

English	Spanish	French

changelog: User-Facing Improvements, Doc Auth, Allow user select IPP if available from handoff page.

n1zyy

I haven't run this code, but wanted to make sure to take a look at the PR since you had asked about it.

Most of our IPP code isn't actually using Idv::InPersonConfig (because we didn't know about it when we were first starting out), but it's something we want to start using, so I think your use of it is good. The wrinkle is that it doesn't currently take into account the opt-in feature flag, when I think it should. That feature flag will go away after we have it running in the "permanent" condition for a while, but not immediately.

n1zyy · 2024-03-20T19:42:37Z

app/controllers/idv/document_capture_controller.rb

+                      idv_session.flow_path == 'hybrid'
+      # Only allow direct access to document capture if IPP available
+      return false unless IdentityConfig.store.in_person_doc_auth_button_enabled &&
+                          Idv::InPersonConfig.enabled_for_issuer?(decorated_sp_session.sp_issuer)


For posterity (you're the one that shared this, just want to note it here as well): that first line is due to https://gsa-tts.slack.com/archives/C5QUGUANN/p1689380596444809?thread_ts=1689376275.536679&cid=C5QUGUANN — it's a rarely-used shutoff valve.

We have not been using Idv::InPersonConfig, but arguably should be. The only issue with it right now is that it doesn't take into account the in_person_proofing_opt_in_enabled feature flag. Long-term that's going away and it'll just always be on. (Right now, in_person_proofing_enabled controls IPP globally, including as a fallback to remote proofing, while in_person_proofing_opt_in_enabled only controls whether we offer IPP up front.) I think we may want to add that conditional for now. It might sense to add it right on the enabled? method in InPersonConfig.

I chatted with @JackRyan1989 a bit about this, and filed LG-12768 for us to update our code to use Idv::InPersonConfig.enabled_for_issuer?. Don't hold off until we do it, though, just noting it for your awareness.

@n1zyy , i used this condition based on like in the frontend

identity-idp/app/views/idv/shared/_document_capture.html.erb

Line 33 in 0d1343d

idv_in_person_url: (IdentityConfig.store.in_person_doc_auth_button_enabled && Idv::InPersonConfig.enabled_for_issuer?(decorated_sp_session.sp_issuer)) ? idv_in_person_url : nil,

Here I did not add in_person_proofing_opt_in_enabled because regardless of the how to verify opt-in screen enabled or not, the handoff page ipp option is not affected, only affected by system wide feature and sp option.

@n1zyy is there existing documentation about IdentityConfig.store.in_person_doc_auth_button_enabled and Idv::InPersonConfig? I want to make sure this information is accessible outside of this github conversation.

@eileen-nava I don't think there is (yet). :( For want of a better solution, I left comments here as little breadcrumbs. I'm still not quite sure what the best way to capture this information is. Do you have a better sense than I do of where we could capture this knowledge?

@n1zyy Since the knowledge that needs to be documented is Team Joy domain, I will leave the decision up to you of where to document it.

n1zyy · 2024-03-20T19:45:45Z

app/controllers/idv/document_capture_controller.rb

+      case params[:step]
+      when 'hybrid_handoff'
+        @previous_step_url = idv_hybrid_handoff_path
+      else
+        @previous_step_url = nil
+      end


This is an unimportant nit (or maybe just personal preference), but if there's only one case (other than the else), I'd personally be inclined to make this a simple conditional:

Suggested change

case params[:step]

when 'hybrid_handoff'

@previous_step_url = idv_hybrid_handoff_path

else

@previous_step_url = nil

end

if params[:step] == 'hybrid_handoff'

@previous_step_url = idv_hybrid_handoff_path

else

@previous_step_url = nil

end

@n1zyy, agree, did this based on possibility that if we think top down, basically from opt-in page and handoff page we can go directly to document capture page, hence can use similar logic.

Indeed it seems extra in current stage.

n1zyy · 2024-03-20T20:01:26Z

app/javascript/packages/document-capture/components/document-capture.tsx

  // If the user got here by opting-in to in-person proofing, when skipDocAuth === true,
  // then set steps to inPersonSteps
-  const steps: FormStep[] = skipDocAuth ? inPersonSteps : defaultSteps;
+  const steps: FormStep[] = skipDocAuth || skipDocAuthFromHandoff ? inPersonSteps : defaultSteps;


I've gradually become a fan of trying to rewrite anything I have to stop and think about for a minute, not so much because I'm intellectually lazy, but because I feel like those are the places where bugs can hide.

How would you feel about adding (technically unnecessary) parenthesis to this line to make explicit the order of operations? I think this assignment could be read as either:

(skipDocAuth || skipDocAuthFromHandoff) ? inPersonSteps : defaultSteps;

or

skipDocAuth || (skipDocAuthFromHandoff ? inPersonSteps : defaultSteps);

I'm pretty sure I know which one is correct, but I think we could make it so obvious that it's immediately obvious to everyone.

@n1zyy funny if I use (skipDocAuth || skipDocAuthFromHandoff) ? inPersonSteps : defaultSteps;, the prettier formatter seems automatically removed the parentheses.

I changed to the following, since the condition used at other locations too.

const isInPersonStepEnabled = skipDocAuth || skipDocAuthFromHandoff; const steps: FormStep[] = isInPersonStepEnabled ? inPersonSteps : defaultSteps;

spec/features/idv/doc_auth/how_to_verify_spec.rb

eileen-nava · 2024-03-20T15:28:45Z

app/controllers/idv/document_capture_controller.rb

+      return false if params[:step].blank? || params[:action].to_s != 'show' ||
+                      idv_session.flow_path == 'hybrid'
+      # Only allow direct access to document capture if IPP available
+      return false unless IdentityConfig.store.in_person_doc_auth_button_enabled &&


eileen-nava · 2024-03-20T15:46:15Z

app/controllers/idv/document_capture_controller.rb

+      else
+        @previous_step_url = nil
+      end
+      # allow


Helpful comment, I didn't wrap my head around this at first.

eileen-nava · 2024-03-20T17:25:43Z

app/controllers/idv/hybrid_handoff_controller.rb

         idv_session.service_provider&.in_person_proofing_enabled
-        idv_session.skip_doc_auth == false
+        idv_session.skip_doc_auth == false ||
+          idv_session.skip_doc_auth_from_handoff == true


My understanding is that idv_session.skip_doc_auth == false represents the scenario where the user did not select IPP on the IPP opt-in page. My understanding is also that idv_session.skip_doc_auth_from_handoff == true represents the scenario where the user did not select IPP on the hybrid handoff page.

Based on the above understanding, I get why self.selected_remote returns true for idv_session.skip_doc_auth == false || idv_session.skip_doc_auth_from_handoff == true. I also think it's confusing to have the same outcome for idv_session.skip_doc_auth == false and idv_session.skip_doc_auth_from_handoff == true. Would it be possible to change the logic to make this clearer?

(Also, let me know if my understanding is off.)

@eileen-nava , your understanding is correct, I extract the new part out of this existing part.

spec/features/idv/doc_auth/how_to_verify_spec.rb

eileen-nava

I can look at this more tomorrow - I think I am still finding the variables
skip_doc_auth and skip_doc_auth_from_handoff confusing. They're named similarly but seem to have each other's logic flipped.

eileen-nava · 2024-03-25T20:47:27Z

app/controllers/idv/hybrid_handoff_controller.rb

      @upload_disabled = idv_session.selfie_check_required &&
                         !idv_session.desktop_selfie_test_mode_enabled?

+      @direct_ipp_with_selfie_enabled = IdentityConfig.store.in_person_doc_auth_button_enabled &&


eileen-nava · 2024-03-25T21:01:24Z

app/controllers/idv/hybrid_handoff_controller.rb

      )

+      # reset if we visit or come back
+      idv_session.skip_doc_auth_from_handoff = nil


I'm checking my own understanding: why do we need to reset skip_doc_auth_from_handoff to nil if the user returns to this page? Is it to enable a user to select IPP on the hybrid handoff page (skip_doc_auth_from_handoff = true), navigate forward, change their mind, then navigate backward and select remote proofing (skip_doc_auth_from_handoff = false)?

@eileen-nava , yes you are correct, it's kind of non-reentrant ticket, once you exit the venue, you have to get a ticket again.

dawei-nava · 2024-03-26T13:06:36Z

I can look at this more tomorrow - I think I am still finding the variables skip_doc_auth and skip_doc_auth_from_handoff confusing. They're named similarly but seem to have each other's logic flipped.

kind of, my original thought is to unify these two under one flag, but that will involve large scope and multiple teams.

So right now these are like two entrances to IPP doc capture, you can only enter from one entrance, if you choose one, then for sure you are not from the other one. No wave–particle duality here.

gina-yamada · 2024-03-26T21:03:29Z

app/controllers/idv/hybrid_handoff_controller.rb

                         !idv_session.desktop_selfie_test_mode_enabled?

+      @direct_ipp_with_selfie_enabled = IdentityConfig.store.in_person_doc_auth_button_enabled &&
+                                        Idv::InPersonConfig.enabled_for_issuer?(


I may very much be wrong here so please poke back- vacation brain.

I am wondering if in_person_doc_auth_button_enabled can be replaced by the already existing in_person_proofing_enabled flag? If you follow the flow through- if you fail remote and this flag is set to false- you will only be asked to retry remote- you will not have the option to move into IPP. If the flag is set to true (and you fail remote)- you will have the option to retry remote or start IPP. It seems like when we want the same logic here. Is there a situation in which the flags could be different? Or wondering if we want that extra control- should we also be evaluating the in_person_proofing_enabled flag here? Again, sorry if I am thinking about this all wrong but maybe good to think about more.

@gina-yamada , that in_person_proofing_enabled flag is already considered in Idv::InPersonConfig.enabled_for_issuer?.

This raises the question should we push in_person_doc_auth_button_enabled down there too?

@dawei-nava looks like we maybe don't want to since that is a global configuration value independent of the service provider? See link

JackRyan1989 · 2024-03-27T15:05:01Z

I can look at this more tomorrow - I think I am still finding the variables skip_doc_auth and skip_doc_auth_from_handoff confusing. They're named similarly but seem to have each other's logic flipped.

kind of, my original thought is to unify these two under one flag, but that will involve large scope and multiple teams.

So right now these are like two entrances to IPP doc capture, you can only enter from one entrance, if you choose one, then for sure you are not from the other one. No wave–particle duality here.

In thinking about these variables, I'm wondering if it makes sense to say something about where you are skipping from, and where you are skipping to in the name? Implicit in the skip_doc_auth variable is that you are skipping from the how_to_verify_page to the document_capture#prepare page. Which is only obvious from the context in which it is used. @dawei-nava is my understanding correct that skip_doc_auth_from_handoff is basically going from hybrid_handoff to the selfie capture url? Because in that case you could maybe say skip_from_handoff_to_selfie_capture.

@JackRyan1989 , basically right and it's really skip_from_handoff_to_ipp?, for sure only when selfie required for now.

Or direct_ipp_from_handoff?

dawei-nava · 2024-03-27T15:19:26Z

@kellular, could you take a look at the contents when having time?

eileen-nava

I'm approving this PR. Since it is such a big PR, and since it affects both Timnit and Joy domain, I do think the PR needs to get a second approval before we merge it.

JackRyan1989 · 2024-03-27T15:09:23Z

app/controllers/idv/hybrid_handoff_controller.rb

                         !idv_session.desktop_selfie_test_mode_enabled?

+      @direct_ipp_with_selfie_enabled = IdentityConfig.store.in_person_doc_auth_button_enabled &&
+                                        Idv::InPersonConfig.enabled_for_issuer?(


@dawei-nava looks like we maybe don't want to since that is a global configuration value independent of the service provider? See link

JackRyan1989 · 2024-03-27T15:59:00Z

app/views/idv/document_capture/show.html.erb

@@ -9,5 +9,6 @@
      acuant_version: acuant_version,
      opted_in_to_in_person_proofing: opted_in_to_in_person_proofing,
      skip_doc_auth: skip_doc_auth,


@n1zyy I vote for opening a ticket to rename skip_doc_auth to skip_doc_auth_from_opt_in, what do you think?

@n1zyy and @JackRyan1989 ... I also strongly vote to renameskip_doc_auth ... i think renaming skip_doc_auth to be more inline with what is trying to be accomplished would be alot more helpful to folks across appDev teams to understand what is happening.

When reviewing this PR, I was confused and really had to dig around in the code to understand the goal of skip_doc_auth. Could the rename more so reflect the goal with something like ie: ipp_requested or opted_in_to_ipp?

is there a ticket open that I can follow? Thanks 🙏🏿

@amirbey hot off the presses: https://cm-jira.usa.gov/browse/LG-13006

amirbey · 2024-03-28T17:51:36Z

app/controllers/idv/document_capture_controller.rb


    before_action :confirm_not_rate_limited, except: [:update]
-    before_action :confirm_step_allowed
+    before_action :confirm_step_allowed, unless: -> { allow_direct_ipp? }


this does not enforce that welcome and agreement pages were completed if IPP is enabled? 🤔
perhaps we should make this less lax?

@amirbey, good suggestions, i was thinking add checking the referer, but this sounds better. Added check and tests.

kellular

LGTM!

amirbey · 2024-03-29T00:55:17Z

app/controllers/idv/document_capture_controller.rb

+      return false unless idv_session.welcome_visited == true &&
+                          idv_session.idv_consent_given == true


Suggested change

return false unless idv_session.welcome_visited == true &&

idv_session.idv_consent_given == true

return false unless idv_session.welcome_visited &&

idv_session.idv_consent_given

amirbey · 2024-03-29T00:55:37Z

app/controllers/idv/hybrid_handoff_controller.rb

-                          self.selected_remote(idv_session: idv_session)
+                           (self.selected_remote(idv_session: idv_session) || # from opt-in screen
+                             # back from ipp doc capture screen
+                             idv_session.skip_doc_auth_from_handoff == true)


Suggested change

idv_session.skip_doc_auth_from_handoff == true)

idv_session.skip_doc_auth_from_handoff)

amirbey

A few minor comments ... LGTM 👍🏿

dawei-nava added 11 commits March 19, 2024 11:41

LG-12631: WIP

841a0f2

LG-12631: deal with back button on ipp prepare page.

b26d41b

LG-12631: UI update and tests.

188a45d

LG-12631: translation format.

be3faa3

LG-12631: Use conditional action.

2cd1977

changelog: User-Facing Improvements, Doc Auth, Allow user select IPP if available from handoff page.

LG-12631: test update.

95d23c1

LG-12631: erb template test update.

b31b55b

LG-12631: explicitly allow skip handoff in test.

4e23871

LG-12631: introduce dedicated flag for choosing IPP from handoff page.

0b308b1

LG-12631: set step indicator correctly.

527e65a

LG-12631: rename controller var to avoid confusion.

22580ba

dawei-nava requested review from JackRyan1989, gina-yamada and n1zyy March 19, 2024 16:01

dawei-nava mentioned this pull request Mar 19, 2024

LG-12670: handoff ipp option #10260

Closed

LG-12631: update tests WIP.

36d42d4

dawei-nava requested review from amirbey and eileen-nava March 19, 2024 20:57

dawei-nava changed the title ~~LG-2631: handoff ipp~~ LG-12631: handoff ipp Mar 20, 2024

eileen-nava requested a review from kellular March 20, 2024 15:20

n1zyy reviewed Mar 20, 2024

View reviewed changes

LG-12631: update tests WIP.

3441b4b

eileen-nava reviewed Mar 20, 2024

View reviewed changes

dawei-nava added 5 commits March 20, 2024 20:19

LG-12631: update tests WIP.

62063fd

LG-12631: update tests for various scenarios.

0d1343d

LG-12631: update tests for various scenarios.

308f3f0

LG-12631: update based on comments.

d3b4557

LG-12631: fix tests.

2a11005

dawei-nava marked this pull request as ready for review March 22, 2024 01:04

dawei-nava requested a review from n1zyy March 22, 2024 13:14

eileen-nava self-requested a review March 25, 2024 20:27

eileen-nava reviewed Mar 25, 2024

View reviewed changes

LG-12631: some minor changes.

850a873

gina-yamada reviewed Mar 26, 2024

View reviewed changes

eileen-nava approved these changes Mar 27, 2024

View reviewed changes

JackRyan1989 reviewed Mar 27, 2024

View reviewed changes

LG-12631: extra character.

b83783e

amirbey reviewed Mar 28, 2024

View reviewed changes

dawei-nava added 2 commits March 28, 2024 14:41

LG-12631: check whether the consent was given upon direct access of ipp.

e44e1ff

LG-12631: update test to confirm IPP from how to verify page.

4dda380

kellular approved these changes Mar 28, 2024

View reviewed changes

amirbey reviewed Mar 29, 2024

View reviewed changes

amirbey approved these changes Mar 29, 2024

View reviewed changes

LG-12631: minor change.

2f130b0

dawei-nava merged commit ac5b955 into main Mar 29, 2024

dawei-nava deleted the dwang/LG-12631_handoff_ipp branch March 29, 2024 13:46

aduth mentioned this pull request Apr 2, 2024

Deploy RC 368 to Prod #10349

Merged

		return false unless idv_session.welcome_visited == true &&
		idv_session.idv_consent_given == true

	idv_session.skip_doc_auth_from_handoff == true)
	idv_session.skip_doc_auth_from_handoff)

Conversation

dawei-nava commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 Screenshots

IPP available and selfie required

IPP available and NO selfie required

IPP NOT available and Selfie required

IPP NOT available and NO Selfie required

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!

dawei-nava Mar 21, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

dawei-nava Mar 21, 2024 • 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!

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!

eileen-nava 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!

dawei-nava commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gina-yamada Mar 26, 2024 • 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!

JackRyan1989 commented Mar 27, 2024 • edited by dawei-nava Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dawei-nava commented Mar 27, 2024

Uh oh!

eileen-nava left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dawei-nava commented Mar 19, 2024 •

edited

Loading

dawei-nava Mar 21, 2024 •

edited

Loading

dawei-nava Mar 21, 2024 •

edited

Loading

dawei-nava commented Mar 26, 2024 •

edited

Loading

gina-yamada Mar 26, 2024 •

edited

Loading

JackRyan1989 commented Mar 27, 2024 •

edited by dawei-nava

Loading

amirbey Mar 28, 2024 •

edited

Loading