LG-12067 TMX Integration Specs by JackRyan1989 · Pull Request #10231 · 18F/identity-idp

JackRyan1989 · 2024-03-12T00:14:57Z

🎫 Ticket

Link to the relevant ticket:
LG-12067

🛠 Summary of changes

Lots of specs written.

To do:

Confirm that all testing conditions outlined in this document are met (I think they are but they need to be double checked)
Add a changelog
Integrate @n1zyy's changes from Bugfix: Clear in_person_verification_pending_at on fraud deactivation #10222 and get the specs all green again.
Re Set up Code Climate #4 - two specs are failing for me when I add in this change: the cancellation spec starting on line 252 of in_person_threatmetrix_spec and the IPP fail and TMX fail spec starting on line 234. See if you can replicate these failures and then fix. There's a third test that's failing as well but I think that one is chromedriver based. Not sure though.

📜 Testing Plan

Get the specs to pass

…7-ipp-tmx-integration-tests Merging main to hopefully handle weird js errors during tests.

JackRyan1989 · 2024-03-12T13:38:33Z

spec/features/idv/in_person_threatmetrix_spec.rb

+      update(in_person_proofing_enabled: true)
+  end
+
+  def deactivate_profile_update_enrollment(status:)


So in thinking about this a bit more, this maybe isn't the best idea. I'm obscuring some important things that happen in the flow, and are basically stand-ins for the get_usps_proofing_job. Not sure what the best approach here is. I suppose this can be unwound so that the deactivation and updates are more explicit in the spec, or this thing can be moved to the in_person_helper if that is a more appropriate place for this sort of method.

i do like the idea of moving this function out of the file...maybe to a threatmetrix_helper file? though the in_person_helper is also a good place

I keep going back and forth on the best approach here. I have two somewhat contradictory opinions:

Too many of our tests are really redundant, repeating with just a few changes like 20 lines of setup for each case, that could really be factored out. Beyond some ideas about cleanliness, I think this would make it easier to reason about what was happening.

Some of our tests take shortcuts with setup that aren't consistent with what happens "in the real world," and occasionally this masks issues.

So I like this approach for helping with #1. For #2, I wonder if you could actually call GetUspsProofingResultsJob live here?

But, I also think this is an improvement as-is and am on board with your improved, expanded test coverage and potentially coming back later to polish it a bit more.

those are great points. as for using that job I believe we could do that here. I'll try to update it

I'm encountering an error related to usermailer when I try to include GetUspsProofingResultsJob. I wonder if that's why we're updating the enrollment as we are here, to get around the mailer issue?

@svalexander that's exactly why I went with directly updating the enrollment.

JackRyan1989 · 2024-03-12T13:39:30Z

spec/support/features/in_person_helper.rb

  end

-  def complete_ssn_step(_user = nil)
+  def complete_ssn_step(_user = nil, tmx_status = nil)


added this arg to help toggle the tmx_status. However, we don't always toggle the value, hence the default nil.

this is a great addition

JackRyan1989 · 2024-03-12T13:40:23Z

spec/support/features/in_person_helper.rb

    complete_verify_step(user)
  end

+  def complete_entire_ipp_flow(user = user_with_2fa, tmx_status = nil, same_address_as_id: true)


Decided to make a helper method for the entire flow from sign in to personal key acknowledgement. I don't think I'm duplicating work here?

from what i can see that seems accurate. good addition to have

I'm biased, but I like this approach. It DRYs up a lot.

JackRyan1989 · 2024-03-12T13:41:17Z

spec/features/idv/in_person_threatmetrix_spec.rb

+    # update the enrollment:
+    enrollment.update(
+      status: status,
+      proofed_at: Time.zone.now,


The proofed_at and status_check_completed_at values are never checked. So I suppose we could remove them, but I wanted to more closely emulate how the enrollment gets updated.

JackRyan1989 · 2024-03-12T13:41:46Z

spec/features/idv/in_person_spec.rb

    allow(IdentityConfig.store).to receive(:in_person_proofing_enabled).and_return(true)
  end

-  context 'ThreatMetrix review pending' do


Moved the TMX related specs to in_person_threatmetrix_spec.rb

spec/features/idv/in_person_threatmetrix_spec.rb

svalexander · 2024-03-15T14:58:49Z

spec/features/idv/in_person_threatmetrix_spec.rb

+          # user can restart
+          click_on t('idv.cancel.actions.start_over')
+
+          expect(page).to have_current_path(idv_welcome_path)


this now fails after the merge of the recent bug fix. The before action confirm_fraud_pending in the please_call_controller is not redirecting as fraud_review_pending? is true. (though that method would redirect us to the account page and not the welcome page)

svalexander · 2024-03-15T17:15:59Z

spec/features/idv/in_person_threatmetrix_spec.rb

+        end
+      end
+
+      # To be completed in a future ticket


Ticket added to address these 2 commented out specs

n1zyy

Nice work here, both @JackRyan1989 for laying an excellent foundation that exposed some bugs, and @svalexander for picking it up and running with it (and also catching bugs 🥲).

Please remove WIP from the PR title before merging, but I'm really happy with where this ended up. All that remains is rubocop yelling that a commented-out line is now one character too long.

n1zyy · 2024-03-15T18:27:41Z

spec/features/idv/in_person_threatmetrix_spec.rb

+      update(in_person_proofing_enabled: true)
+  end
+
+  def deactivate_profile_update_enrollment(status:)


I keep going back and forth on the best approach here. I have two somewhat contradictory opinions:

Too many of our tests are really redundant, repeating with just a few changes like 20 lines of setup for each case, that could really be factored out. Beyond some ideas about cleanliness, I think this would make it easier to reason about what was happening.

Some of our tests take shortcuts with setup that aren't consistent with what happens "in the real world," and occasionally this masks issues.

So I like this approach for helping with #1. For #2, I wonder if you could actually call GetUspsProofingResultsJob live here?

But, I also think this is an improvement as-is and am on board with your improved, expanded test coverage and potentially coming back later to polish it a bit more.

n1zyy · 2024-03-15T19:48:06Z

spec/features/idv/in_person_threatmetrix_spec.rb

+      expect_in_person_step_indicator_current_step(
+        t('step_indicator.flows.idv.go_to_the_post_office'),
+      )
+      expect(page).to have_content(t('in_person_proofing.headings.barcode').tr(' ', ' '))


Not for this PR, especially because this was just moved around (and I think I may have written the original line I'm now objecting to 😆), but I wonder if we can move the .tr(' ', ' ')) pattern into a helper method explaining what the heck it's doing later on. It's not at all intuitive that these are two different strings (replacing a non-breaking space with a regular space, or vice versa).

But again, not for now. I'll file a ticket.

LG-12736 created for this.

spec/features/idv/in_person_threatmetrix_spec.rb

JackRyan1989 added 15 commits March 1, 2024 16:50

in progress changes maybe revert

5de046d

update helper to accept tmx status for ssn step

10f1b2b

new tmx only spec

3c89dec

Merge branch 'main' of github.com:18F/identity-idp into jryan/lg-1206…

ddda20f

…7-ipp-tmx-integration-tests Merging main to hopefully handle weird js errors during tests.

Add new method and allow arguments

4e5e877

Utilize new arguments for ssn step

e1dd289

begin specs

49f73bd

initial specs complete

f6ffed1

added in test for cancel ipp option

df2787a

New spec for failed ipp enrollment

4bb92a1

lintage

1a30cad

adding in action_account emulation

478a473

move tmx related specs to new file

c610cdf

move tmx specs to new file

a5d08fe

lintereedoo

576f04e

JackRyan1989 added the status - do not merge label Mar 12, 2024

JackRyan1989 requested review from a team and racingspider March 12, 2024 00:14

JackRyan1989 commented Mar 12, 2024

View reviewed changes

svalexander reviewed Mar 12, 2024

View reviewed changes

spec/features/idv/in_person_threatmetrix_spec.rb Outdated Show resolved Hide resolved

svalexander added 3 commits March 13, 2024 17:31

add pass ipp fail tmx case

31f8639

use one before statement

e1b2b7d

fix lint issue and update descriptions

33f23b2

svalexander marked this pull request as ready for review March 14, 2024 17:00

svalexander added 2 commits March 14, 2024 17:44

Merge branch 'main' into jryan/lg-12067-ipp-tmx-integration-tests

3f93276

changelog: Internal, TMX, tests

f70aef3

svalexander reviewed Mar 15, 2024

View reviewed changes

svalexander added 2 commits March 15, 2024 12:20

comment out failing test to be completed in a future ticket

50fb04f

comment out another failing test

5cdd5fb

svalexander reviewed Mar 15, 2024

View reviewed changes

n1zyy approved these changes Mar 15, 2024

View reviewed changes

n1zyy added 2 commits March 15, 2024 17:00

Fixes trivial rubocop nit

26143d0

Merge branch 'main' into jryan/lg-12067-ipp-tmx-integration-tests

6c96897

svalexander changed the title ~~WIP LG-12067 TMX Integration Specs~~ LG-12067 TMX Integration Specs Mar 18, 2024

JackRyan1989 commented Mar 18, 2024

View reviewed changes

spec/features/idv/in_person_threatmetrix_spec.rb Outdated Show resolved Hide resolved

JackRyan1989 commented Mar 18, 2024

View reviewed changes

spec/features/idv/in_person_threatmetrix_spec.rb Outdated Show resolved Hide resolved

ensure email count is updated after tmx review

27e6455

JackRyan1989 removed the status - do not merge label Mar 19, 2024

JackRyan1989 self-assigned this Mar 19, 2024

JackRyan1989 merged commit 77148e9 into main Mar 19, 2024

JackRyan1989 deleted the jryan/lg-12067-ipp-tmx-integration-tests branch March 19, 2024 14:11

matthinz mentioned this pull request Mar 19, 2024

Deploy RC 365 to Production #10268

Merged

JackRyan1989 mentioned this pull request Mar 28, 2024

LG-12729 Please Call page shows for passed IPP enrollments only #10333

Merged

21 tasks

Conversation

JackRyan1989 commented Mar 12, 2024

🎫 Ticket

🛠 Summary of changes

📜 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!

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!

JackRyan1989 Mar 12, 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!

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!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JackRyan1989 Mar 12, 2024 •

edited

Loading