LG-7400 Add update functionality to VerifyInfoController outside Flow State Machine by soniaconnolly · Pull Request #7661 · 18F/identity-idp

soniaconnolly · 2023-01-18T23:19:13Z

ETA: It looks like rebasing on main caused this PR to include extra commits. Created PR #7676 from the correct cherry-picked commits, recommend merging that one instead.

What
Add update functionality to VerifyInfoController outside Flow State Machine

After submitting the info on the verify screen it is submitted to the enabled vendor(s)
Errors from the vendor are rendered to the user
If there are no errors the user is sent to the phone step
Rate limiting is enforced on this step
Async functionality included (LG-7401) because it wasn't feasible to separate it out, but LG-7401 will now be the ticket for adding the Spinner button back in.

All changes are hidden behind doc_auth_verify_info_controller_enabled IdentityConfig feature flag which is false in prod.

🎫 Ticket

LG-7400

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

Confirm that doc_auth_verify_info_controller_enabled flag is false
Run through remote proofing, confirm unchanged
Set doc_auth_verify_info_controller_enabled flag to true
Run through remote proofing, will see /verify/verify_info url in browser for Verify step
Screens should otherwise remain unchanged
Confirm other proofing flows also unchanged

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Replace resolution_info_verified with already existing resolution_info_verified Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Look for proofing results in :stages, :resolution rather than :extra Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Redirect back to verify_info if needed from review controller before action Proofing results exception found deeper in data structure on failure Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov>

zachmargolis · 2023-01-19T00:40:16Z

app/controllers/idv/review_controller.rb

+      # rubocop:disable Style/IfUnlessModifier
+      if IdentityConfig.store.doc_auth_verify_info_controller_enabled
+        if !idv_session.resolution_successful
+          redirect_to idv_verify_info_url
+        end
+      end
+      # rubocop:enable Style/IfUnlessModifier


QQ what was the reason for disabling the lint? is this WIP?

Also, I think the lint would be quiet if we combined the two conditions to be one, so that it's too long to qualify for that modifier approach?

Suggested change

# rubocop:disable Style/IfUnlessModifier

if IdentityConfig.store.doc_auth_verify_info_controller_enabled

if !idv_session.resolution_successful

redirect_to idv_verify_info_url

end

end

# rubocop:enable Style/IfUnlessModifier

if IdentityConfig.store.doc_auth_verify_info_controller_enabled && !idv_session.resolution_successful

redirect_to idv_verify_info_url

end

Disabled lint when the outer if statement was in another location because it 1) complained that the line was too long and then 2) complained when I made it not a one-liner. It might not even be an issue here anymore with the inner if, and your solution should work, thanks.

Fixed in 3a2c085

zachmargolis · 2023-01-19T00:44:11Z

app/controllers/idv/verify_info_controller.rb

+    def redact(text)
+      text.gsub(/[a-z]/i, 'X').gsub(/\d/i, '#')
+    end


I think this is the 3rd time we have copy-pasted this method definition (one, two), I think that makes it a good candidate to be unified by a helper. like StringRedacter.redact_alphanumeric or something

How is that usually done? Separate ticket + small PR? I looked at the existing helpers and looks like we would need a new one rather than adding to an existing one. Maybe something a little more general than StringRedacter, like PrivacyHelper?

It depends on the complexity. To me, it looks like all 3 are the exact same method? And it would be a straightforward static method extraction, so I would consider doing it in this PR (because the additional overhead is almost the same as copy-pasting like we've done here). But if needed, we could file a ticket and follow up, which makes sense in more complicated refactors

As far as naming, I don't feel that strongly, PrivacyHelper seems fine, in general I feel like xyzHelper classes are not always super clear, I'd name it after what it does (redact strings), but to me the more important bit is extracting the shared code

Fixed by adding a StringRedacter concern 41a6339. Will shift over the other places it's used separately from this PR. Also noting that VerifyInfoController will eventually replace VerifyBaseStep, so one of the uses will go away.

Tried adding a Helper first, but helpers are expected to be used in Views. Then tried adding it in lib, then settled on a Concern since that's what's used in Controllers.

per PR feedback. This is now available for a future PR to use in PhoneController and VerifyBaseStep. Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov>

changelog: Internal improvements, Flow State Machine, Add update method to new VerifyInfoController

jmhooper · 2023-01-20T18:32:26Z

app/controllers/idv/review_controller.rb

      params.fetch(:user, {})[:password].presence
    end

+    def confirm_verify_info_complete


I'm looking at #confirm_idv_steps_complete which is also a before action and has a call to #idv_profile_complete?. Wdyt of wrapping this logic into that method? That way a user can confirm in either the FSM or the new controller and still pass the before actions here.

Sadly it doesn't get to that before action, but makes a redirect loop between review and verify_info somewhere before that. I couldn't figure out where, but putting this before action right at the top fixed it.

Paired on this with the team. We figured out that the problem is a little complicated. The process looks like this:

The user fails proofing at the verify info controller

The user is redirected to the idv session error page

There is a link on the idv session error page that redirects back to the doc auth FSM

The user enters the doc auth FSM which sees that all steps are complete and redirects to the review controller

The review controller runs the confirm_idv_session_started before action which checks for an applicant. Since resolution did not succeed there is no applicant. This causes it to redirect back to the doc auth FSM

The doc auth FSM proceeds as if in step 4 and begins thrashing

With all that in mind we are all convinced the before action is probably the best route. We will have to figure out how to handle the redirects back to the flow state machine or our new code when we handle the deployment ticket (LG-7402).

jmhooper · 2023-01-20T18:35:49Z

app/controllers/idv/verify_info_controller.rb

+
+      pii[:uuid_prefix] = ServiceProvider.find_by(issuer: sp_session[:issuer])&.app_id
+
+      if ssn_throttle.throttled_else_increment?


I see the SSN rate limiting in effect here, but I do not see the resolution rate limiting in effect. I only see that in the new method. Is there something I am missing?

There are two different throttles as you noted. In the old code they are both referred to as "throttle." We decided to resolve the confusion. Verify_base_step was only checking the ssn throttle on enqueue_job (see second link) which is our update method. Do you want us to add resolution throttle here? See

identity-idp/app/services/idv/steps/verify_base_step.rb

Lines 72 to 74 in cc482cb

@throttle ||= Throttle.new(

user: current_user,

throttle_type: :idv_resolution,

and

identity-idp/app/services/idv/steps/verify_base_step.rb

Lines 175 to 177 in cc482cb

throttle = Throttle.new(

target: Pii::Fingerprinter.fingerprint(pii[:ssn]),

throttle_type: :proof_ssn,

Chatted with the team; we're going to address this in the PR for LG-7401

jmhooper · 2023-01-20T18:37:07Z

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

  </div>
  <div class="margin-top-5">
-    <%= render SpinnerButtonComponent.new(
+    <%= render ButtonComponent.new(


I believe we need to make this back into a SpinnerButtonComponent, but presumably we can save that for the async story?

Yes, that's the idea. We are working on that part of things in the sonia-lg-7401 branch at the moment

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Replace resolution_info_verified with already existing resolution_info_verified Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Look for proofing results in :stages, :resolution rather than :extra Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Redirect back to verify_info if needed from review controller before action Proofing results exception found deeper in data structure on failure Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov>

per PR feedback. This is now available for a future PR to use in PhoneController and VerifyBaseStep. Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov>

…lt as expected

…a-lg-7400

changelog: Internal, Flow State Machine, Add update action for new VerifyInfoController

soniaconnolly · 2023-01-23T21:59:27Z

Closing and replacing with PR #7676 because of rebase issues.

soniaconnolly and others added 9 commits January 11, 2023 13:00

Add test for Continue from verify_info screen

e2f0c5e

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Add scaffolding to remove verify_wait from flow state machine

9d7bb48

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Add more methods to make verify_info#update work

25a5151

Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Update test to go to next step, now passes

7085cd8

New specs and Idv::Session field change

60f5117

Replace resolution_info_verified with already existing resolution_info_verified Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Small ssn_step cleanup

d3dfc1a

throttle and continue spec passes

f0ed81f

Separate kinds of throttles

64254dc

Look for proofing results in :stages, :resolution rather than :extra Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Verify_info feature tests passing

0d8334d

Redirect back to verify_info if needed from review controller before action Proofing results exception found deeper in data structure on failure Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov>

zachmargolis reviewed Jan 19, 2023

View reviewed changes

soniaconnolly and others added 4 commits January 19, 2023 08:33

Remove line disabling lint and rearrange.

3a2c085

Create Concern for redact method

41a6339

per PR feedback. This is now available for a future PR to use in PhoneController and VerifyBaseStep. Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Clean up comments and lint for verify_info show.html.erb

98c7232

Add changelog

fcd3ebe

changelog: Internal improvements, Flow State Machine, Add update method to new VerifyInfoController

soniaconnolly changed the title ~~LG-7400, LG-7401 Add update functionality to VerifyInfoController outside Flow State Machine~~ LG-7400 Add update functionality to VerifyInfoController outside Flow State Machine Jan 19, 2023

soniaconnolly marked this pull request as ready for review January 19, 2023 19:42

soniaconnolly requested review from a team and jmhooper January 19, 2023 19:43

jmhooper reviewed Jan 20, 2023

View reviewed changes

theabrad mentioned this pull request Jan 20, 2023

LG-7401 Add Spinner Button for VerifyInfoController #7672

Closed

6 tasks

soniaconnolly and others added 7 commits January 20, 2023 12:45

Add test for Continue from verify_info screen

a37b795

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Add scaffolding to remove verify_wait from flow state machine

b4ea51d

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Add more methods to make verify_info#update work

1d1f1d2

Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Update test to go to next step, now passes

e41beaf

New specs and Idv::Session field change

5658495

Replace resolution_info_verified with already existing resolution_info_verified Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Small ssn_step cleanup

cd03803

throttle and continue spec passes

0d97dab

soniaconnolly and others added 7 commits January 20, 2023 12:45

Separate kinds of throttles

f3c3d86

Look for proofing results in :stages, :resolution rather than :extra Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Verify_info feature tests passing

383b95f

Redirect back to verify_info if needed from review controller before action Proofing results exception found deeper in data structure on failure Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov>

Remove line disabling lint and rearrange.

63ce60b

Create Concern for redact method

8a9d0c0

per PR feedback. This is now available for a future PR to use in PhoneController and VerifyBaseStep. Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Clean up comments and lint for verify_info show.html.erb

4c4068e

fix an issue where exceptions were not appearing in the proofing resu…

4efe550

…lt as expected

Merge branch 'sonia-lg-7400' of github.com:18F/identity-idp into soni…

02c29ac

…a-lg-7400

jmhooper approved these changes Jan 20, 2023

View reviewed changes

Add changelog

6c8669d

changelog: Internal, Flow State Machine, Add update action for new VerifyInfoController

soniaconnolly force-pushed the sonia-lg-7400 branch from a241b3e to 6c8669d Compare January 20, 2023 22:08

soniaconnolly mentioned this pull request Jan 20, 2023

LG-7400 Add update functionality to VerifyInfoController outside Flow State Machine (rebased) #7676

Merged

6 tasks

soniaconnolly closed this Jan 23, 2023

soniaconnolly deleted the sonia-lg-7400 branch March 16, 2023 20:51


		pii[:uuid_prefix] = ServiceProvider.find_by(issuer: sp_session[:issuer])&.app_id

		if ssn_throttle.throttled_else_increment?

	@throttle \|\|= Throttle.new(
	user: current_user,
	throttle_type: :idv_resolution,

	throttle = Throttle.new(
	target: Pii::Fingerprinter.fingerprint(pii[:ssn]),
	throttle_type: :proof_ssn,

Conversation

soniaconnolly commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

📜 Testing Plan

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!

soniaconnolly Jan 19, 2023 • 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!

soniaconnolly Jan 20, 2023 • 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!

soniaconnolly commented Jan 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

soniaconnolly commented Jan 18, 2023 •

edited

Loading

soniaconnolly Jan 19, 2023 •

edited

Loading

soniaconnolly Jan 20, 2023 •

edited

Loading