LG-15168 | Proofing components now only takes idv_session #11771

n1zyy · 2025-01-17T20:14:22Z

🎫 Ticket

Link to the relevant ticket:
LG-15168

🛠 Summary of changes

Now that LG-15166 has landed, we can simply ProofingComponents a bit. The initializer only needs to take an idv_session, which also simplifies the code.

changelog: Internal, IDV, Update ProofingComponents to only take an idv_session

…that call it to avoid everything breaking in an utterly predicatable manner.

n1zyy

I hit some really bizarre test failures. I'm starting to wrap up for the day, but want to dig into them tomorrow morning.

n1zyy · 2025-01-27T22:28:09Z

app/controllers/idv/in_person/usps_locations_controller.rb

@@ -63,6 +63,8 @@ def update
          sponsor_id: enrollment_sponsor_id,
        )

+        idv_session.doc_auth_vendor = Idp::Constants::Vendors::USPS


I question later down in the spec whether this is actually correct behavior. Arguably this isn't a doc auth proofing component here?

In the IPP flow, the USPS effectively takes the role of doc auth, so that is how it gets logged (the history seems to go back to this commit from ~2 years ago)

Ok, actually: We are doing this exact thing over in StateIdController now.

So I think we can:

Delete this line

Move the tests for proofing components a that are currently in usps_locations_controller_spec.rb over to state_id_controller_spec.rb

Oh, it's even better than that. The tests already do it: https://github.com/18F/identity-idp/blob/main/spec/controllers/idv/in_person/state_id_controller_spec.rb#L208-L212

Just pushed an update removing this line and the obsolete tests.

n1zyy · 2025-01-27T22:30:07Z

spec/services/idv/agent_spec.rb

@@ -147,7 +142,6 @@
        expect(ResolutionProofingJob).to receive(:perform_later).with(
          hash_including(
            proofing_components: {
-              document_check: 'mock',


I want to sync up with someone tomorrow on this. This was previously being inferred in the ProofingComponents class. Should we figure out how to do that again, or should we not make this assumption? I slightly leaned towards the latter.

@matthinz I would value your input on this. This was previously being inferred in a Rube Goldberg fashion from DocAuthRouter. It seems like doc_auth_vendor is being set where it's actually used, and thus this test is no longer accurate because it was testing it in isolation. But I want to treat cautiously removing test coverage.

This test is just checking that the proofing components are being passed to the ResolutionProofingJob. So I would say removing this one line is fine

n1zyy · 2025-01-27T22:31:21Z

spec/services/idv/proofing_components_spec.rb

    end

    it 'returns expected result' do
      expect(subject.to_h).to eql(
        {
-          document_check: 'test_vendor',
+          document_check: 'feedabee',


I somehow made gitleaks very angry when refactoring and it decided this was a secret. 'feedabee' is one of its approved test values. I just went with it.

Good ol' deadbeef anagrams!

(But much more happy!)

It looks like the ProofingComponents class was referencing this causing the let() to fire, maksing that it shoudl have been a let!() from the beginning.

n1zyy · 2025-01-28T19:41:36Z

spec/controllers/idv/cancellations_controller_spec.rb

@@ -212,7 +212,7 @@

      context 'with in establishing in-person enrollment' do
        let(:user) { build(:user, :with_establishing_in_person_enrollment) }
-        let(:enrollment) { user.establishing_in_person_enrollment }
+        let!(:enrollment) { user.establishing_in_person_enrollment }


This had me tearing my hair out because the enrollment stopped existing in tests here which seemed orthogonal to my changes.

This really should have been let! from the start. But the if user.establishing_in_person_enrollment check in ProofingComponents was causing this to pop into existence as a stealthy side effect. Removing that code as no longer necessary removed that unnoticed side effect and suddenly these tests failed because they didn't have an enrollment. 🫨

spec/controllers/idv/in_person/usps_locations_controller_spec.rb

n1zyy · 2025-01-28T19:55:01Z

spec/services/idv/agent_spec.rb

@@ -147,7 +142,6 @@
        expect(ResolutionProofingJob).to receive(:perform_later).with(
          hash_including(
            proofing_components: {
-              document_check: 'mock',


@matthinz I would value your input on this. This was previously being inferred in a Rube Goldberg fashion from DocAuthRouter. It seems like doc_auth_vendor is being set where it's actually used, and thus this test is no longer accurate because it was testing it in isolation. But I want to treat cautiously removing test coverage.

Co-authored-by: Matt Wagner <[email protected]>

We have an agency partner who requested a specialized report the involves a table with columns representing proofing events and rows representing users for the partner where the values in each cell correspond to whether that user encountered that event in the given time period. We have been manually servicing this request for a little while. This commit adds tooling to generate this report automatically so it does not require manual processing. [skip changelog]

* add requested attribute to accounts_show_presenter * add logic for decorated_sp_session * check for requested_attributes and ial2_requested * add tests for requested attribes and ial2 requested * changelog: User-Facing Improvements, account management, no change available if partner shares all emails * add logic to show/hide change button * clean up controller and presenter * clean up requested_attributes in the rest of controllers and specs * change logic for all emails requested as well as `nil` to `false` in specs * remove `ial2_requested` * lintfix * fix logic to show desired behavior when is false * remove debugger * fix logic * add supporting test for view * match `all_emails_requested` to be a boolean value * add logic for if the partner does not have `email` as a consented attribute * logic change to `all_emails_requested?` * refine check for no email (WIP), refine method name * fix all broken tests * change service provider -> partner * more logic correction * rename and fix logic * Update app/controllers/accounts/connected_accounts_controller.rb Co-authored-by: Andrew Duthie <[email protected]> * change `all_emails requested` -> `change_email_available?` * remove `change_email_available` * move logic around * logic in presenter and view * change to `change_option_available`, fix test * edit test block * change name to be more intuitive * add tests to view * fix account show presenter tests * method name cleanup * more test fixes * more changes * fix eager loading problem Co-authored-by: Andrew Duthie <[email protected]> * lintfix * add test * test 1 for connected apps * check against `identities` and pass value * place logic in model, remove logic from presenter and controller, add check to presenter * no longer doing logic in controller, so return connected apps to previous state * remove `requested_attributes` * fix logic in model and view * method name change * remove unneeded keyword * fix tests * Repurpose ServiceProviderIdentity#hide_change_email? as verified_single_email_attribute? * Revert changes to ApplicationController, AccountShowPresenter spec * Handle nil verified_attributes * Update tests for connected accounts view * Exempt eager loading error validation for connected accounts * Update stubbed identity in connected accounts feature spec to include email --------- Co-authored-by: Andrew Duthie <[email protected]> Co-authored-by: Andrew Duthie <[email protected]>

changelog: Internal, Code Quality, Extract mixin for common MFA deletion behaviors

Log Pii validation event to analytics in the Socure flow changelog: Upcoming Features, Socure, Log Pii validation --------- Co-authored-by: Amir Reavis-Bey <[email protected]>

changelog: Internal, Reporting, Create MFA Report script

We have a partner who requested a specialized IdV drop-off report for their service providers. This report needs to run every Monday and collect weekly data from a specified date up through the previous week. We have been running this report manually for a few weeks now. This commit moves the code the generates this report into the IdP so it can be run automatically. [skip changelog]

* set socure_docv_wait_polling timestamp to nil in show on hybrid mobile * add changelog changelog: Internal, IdV Doc Auth, Allow timed out user to retry docv * have spec ensure socure_docv_wait_polling_started_at is nil

…#11819) We have received clarification on the following from our vendor: 1. The `Fields_Sex` attribute is no longer available in the `ID_AUTH_FIELDS` group 2. The `Sex` attribute available in the `AUTHENTICATION_RESULT` group can be used to reliably determine the sex displayed on the document 3. The `Sex` attribute in the `AUTHENTICATION_RESULT` group takes on the values of `Male` or `Female`. This commit updates the `DocPiiReader` and the TrueID response fixtures to reflect this. [skip changelog]

n1zyy · 2025-02-03T22:15:52Z

app/controllers/idv/in_person/state_id_controller.rb

+
+          analytics.idv_in_person_proofing_state_id_submitted(
+            **analytics_arguments.merge(**form_result),
+          )


We need to log this after setting doc_auth_vendor now.

This may or may not be my fault. (Unfortunately, it was the least janky way of doing it.)

n1zyy · 2025-02-03T22:18:01Z

spec/features/idv/analytics_spec.rb

@@ -584,7 +584,7 @@
        flow_path: 'standard', opted_in_to_in_person_proofing: false
      },
      'IdV: in person proofing state_id visited' => {
-        step: 'state_id', flow_path: 'standard', analytics_id: 'In Person Proofing', proofing_components: { document_check: 'usps' }
+        step: 'state_id', flow_path: 'standard', analytics_id: 'In Person Proofing'


I want to call out this behavior change. doc_auth_vendor is now only set in the update method. IMHO this is correct behavior, because you don't really have a vendor until you've done the thing. But if any old events are depending on this field being here, it won't be going forward.

follow-up: We should reach out to any teams that are depending on this field being present through the whole flow or just generally make a few announcements at eng huddle.

I have some scripts that I can share which check for usage of a particular field in CW queries and dashboard.

lmgeorge · 2025-02-04T16:51:41Z

app/services/idv/proofing_components.rb

-          user:,
-        )
-      end
+      idv_session.doc_auth_vendor


issue: There is probably some 50/50 state here that needs handling in the interim. I had to do something similar when I added source_check and haven't gotten around to cleaning it up.

I'm not sure if I'm seeing it, but let's make sure I'm not just overlooking something. doc_auth_vendor is already being set in prod, right?

The constructor change should only be problematic if there's background job usage where what's available changes, and I didn't see any of that. I think all the controller-level action will be consistent since we're already storing this in the session in prod.

But my comment here is just intended to outline my thinking. What else should I be looking at?

You're totally right. I was thinking in terms of jobs not the controller context. This is fine

lmgeorge

Looks mostly good. My only "blocker" would be the 50/50 state issue.

I think that it will only affect folks in GPO/IPP flows since they may have started their IdV process before doc_auth_vendor was added to Idv::Session.

lmgeorge · 2025-02-04T16:53:27Z

app/services/idv/proofing_components.rb

-          user:,
-        )
-      end
+      idv_session.doc_auth_vendor


issue: There is probably some 50/50 state here that needs handling in the interim. I had to do something similar when I added source_check and haven't gotten around to cleaning it up.

lmgeorge · 2025-02-04T16:56:17Z

spec/features/idv/analytics_spec.rb

@@ -584,7 +584,7 @@
        flow_path: 'standard', opted_in_to_in_person_proofing: false
      },
      'IdV: in person proofing state_id visited' => {
-        step: 'state_id', flow_path: 'standard', analytics_id: 'In Person Proofing', proofing_components: { document_check: 'usps' }
+        step: 'state_id', flow_path: 'standard', analytics_id: 'In Person Proofing'


follow-up: We should reach out to any teams that are depending on this field being present through the whole flow or just generally make a few announcements at eng huddle.

I have some scripts that I can share which check for usage of a particular field in CW queries and dashboard.

lmgeorge · 2025-02-04T17:00:19Z

spec/services/idv/proofing_components_spec.rb

@@ -58,46 +56,10 @@
  end

  describe '#document_check' do
-    it 'returns nil by default' do


suggestion: these tests still seem valuable, no?

I think it's debatable. The method now just returns whatever doc_auth_vendor contains.

The IPP ones no longer make sense here, since we're looking at proofing components in isolation and it doesn't know anything about IPP. Equivalent coverage exists in the IPP code.

'returns nil by default' is valid-ish, but I figured I'd remove the whole thing because really it would just be testing that Ruby returns nil for uninitialized attributes.

lmgeorge

Question resolved, LGTM!

n1zyy added 7 commits January 16, 2025 14:39

LG-15168 | ProofingComponents now only use idv_session

34f820c

changelog: Internal, IDV, Update ProofingComponents to only take an idv_session

Merge branch 'main' into mattw/LG-15168-proofing-components-session

64006e2

Further cleanup now that LG-15166 is merged

269f3eb

LPT: When changing a method definition, consider changing the places …

f486fdf

…that call it to avoid everything breaking in an utterly predicatable manner.

Merge branch 'main' into mattw/LG-15168-proofing-components-session

dfaa0e4

Merge branch 'main' into mattw/LG-15168-proofing-components-session

d4a259b

Address some failing tests

2636f8c

n1zyy commented Jan 27, 2025

View reviewed changes

n1zyy added 2 commits January 28, 2025 14:25

Create enrollment ahead of time

d22972b

It looks like the ProofingComponents class was referencing this causing the let() to fire, maksing that it shoudl have been a let!() from the beginning.

Merge branch 'main' into mattw/LG-15168-proofing-components-session

ac46a70

n1zyy commented Jan 28, 2025

View reviewed changes

n1zyy marked this pull request as ready for review January 28, 2025 19:55

n1zyy requested a review from a team January 28, 2025 19:55

n1zyy commented Jan 28, 2025

View reviewed changes

Update spec/controllers/idv/in_person/usps_locations_controller_spec.rb

28cdc2e

Co-authored-by: Matt Wagner <[email protected]>

matthinz self-requested a review January 29, 2025 22:55

jmhooper and others added 12 commits January 31, 2025 13:53

Extract mixin for common MFA deletion behaviors (#11796)

d4dd239

changelog: Internal, Code Quality, Extract mixin for common MFA deletion behaviors

LG-15309 simple log pii validation to analytics (#11813)

0fd5a07

Log Pii validation event to analytics in the Socure flow changelog: Upcoming Features, Socure, Log Pii validation --------- Co-authored-by: Amir Reavis-Bey <[email protected]>

Create MFA Report script (#11740)

f9c2f0a

changelog: Internal, Reporting, Create MFA Report script

Merge branch 'main' into mattw/LG-15168-proofing-components-session

261d971

Removes unnecessary dead code

664cf93

Fix IPP analytics logging

04b395f

Merge branch 'main' into mattw/LG-15168-proofing-components-session

870be44

n1zyy commented Feb 3, 2025

View reviewed changes

lmgeorge reviewed Feb 4, 2025

View reviewed changes

lmgeorge approved these changes Feb 4, 2025

View reviewed changes

n1zyy merged commit d5cf8e0 into main Feb 4, 2025
2 checks passed

n1zyy deleted the mattw/LG-15168-proofing-components-session branch February 4, 2025 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LG-15168 | Proofing components now only takes idv_session #11771

LG-15168 | Proofing components now only takes idv_session #11771

n1zyy commented Jan 17, 2025

n1zyy left a comment

n1zyy Jan 27, 2025

matthinz Jan 28, 2025

matthinz Jan 28, 2025

n1zyy Jan 31, 2025

n1zyy Jan 27, 2025

n1zyy Jan 28, 2025

matthinz Jan 28, 2025

n1zyy Jan 27, 2025

lmgeorge Feb 3, 2025

n1zyy Feb 4, 2025

n1zyy Jan 28, 2025

n1zyy Jan 28, 2025

n1zyy Feb 3, 2025

lmgeorge Feb 4, 2025

n1zyy Feb 3, 2025

lmgeorge Feb 4, 2025

lmgeorge Feb 4, 2025

lmgeorge Feb 4, 2025

n1zyy Feb 4, 2025

lmgeorge Feb 4, 2025

lmgeorge left a comment

lmgeorge Feb 4, 2025

lmgeorge Feb 4, 2025

lmgeorge Feb 4, 2025

n1zyy Feb 4, 2025

lmgeorge left a comment

LG-15168 | Proofing components now only takes idv_session #11771

LG-15168 | Proofing components now only takes idv_session #11771

Conversation

n1zyy commented Jan 17, 2025

🎫 Ticket

🛠 Summary of changes

n1zyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmgeorge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmgeorge left a comment

Choose a reason for hiding this comment