Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LG-13761: ProofingComponents to store real vendor used to verify document PII #11499

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

lmgeorge
Copy link
Contributor

@lmgeorge lmgeorge commented Nov 12, 2024

Why

  • Prior to this change, we always stored 'aamva' in a User's proofing_components.source_check column regardless of whether a call to AAMVA was made or not. Accuracy is good and storing the actual "vendor_name" will align our analytics across our events.

How

  • Instead of hard-coding vendor_name in proofing_components.source_check, we dig into the result object (as produced by the ResultAdjudicator) for the actual vendor_name. This value should always be present even though we don't have any validators for it.

  • This makes testing a little messier, but cleaning up the various vendor names in our specs should be done in future work. For now, a new constant with the known source_check names has been added (Idv::Constants::Vendors::SOURCE_CHECK).

  • No new specs have been added as the functionality hasn't changed, but one end-to-end feature test has been updated to check the vendor name is in the expected list.

changelog: Internal, Analytics, Store correct vendor name in ProofingComponents

🎫 Ticket

Link to the relevant ticket:
LG-13761

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Complete IdV using the unsupervised remote path
  • Complete IdV using the in-person proofing path (where your residential address == your id address)
  • Complete IdV using the in-person proofing path (where your residential address != your id address)

Comment on lines 36 to 40
if idv_session.resolution_successful
Idp::Constants::Vendors::AAMVA
elsif idv_session.resolution_successful == false
Idp::Constants::Vendors::AAMVA_UNSUPPORTED_JURISDICTION
end
Copy link
Contributor Author

@lmgeorge lmgeorge Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A/N: This seems less than ideal, but there doesn't seem to be any good means to pulling the supplied vendor_name out like I did in the VerifyInfoConcern#add_proofing_components method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are 2 options here:

  1. Add a value to the session to indicate whether we performed an AAMVA check and use that
  2. Inspect the value of the issuing jurisdiction in pii_from_user or pii_from_doc and infer from that whether AAMVA was contacted

Option 1 may be slightly preferable since it stores the result of what the job actually does. The weakness is it involves adding another value to the session. As a result we'll need to break this into 2 deploys. One deploy to write the new value and a second to read and use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think I've addressed this here, so let me know what you think!

lib/idp/constants.rb Outdated Show resolved Hide resolved
@lmgeorge lmgeorge requested a review from a team November 12, 2024 21:45
@lmgeorge lmgeorge changed the title LG-13671: ProofingComponents to store real vendor used to verify document PII LG-13761: ProofingComponents to store real vendor used to verify document PII Nov 12, 2024
Copy link
Member

@n1zyy n1zyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not leaving an approval as this is still marked as a draft and Hooper's comment implies a pending change, but 👍 to where it's at so far.

Comment on lines 215 to +235
save_threatmetrix_status(form_response)
save_source_check_vendor(form_response)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A/N: I feel like these two methods could be merged into an handle_success_updates_for_idv_session method, but leaving as is for now.

@@ -38,6 +38,7 @@
allow(FeatureManagement).to receive(:proofing_device_profiling_collecting_enabled?).
and_return(true)
idv_session.threatmetrix_review_status = 'pass'
idv_session.source_check_vendor = 'aamva'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A/N: Is there a better way to test this or is the captured by the end_to_end_idv feature spec?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the VerifyInfoController (or the VerifyInfoConcern? I'm not sure which) should have a test asserting that the value in the result object is propagated to the Idv Session

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, will add that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The closest we get to validating the result object is in the end_to_end_idv feature spec -- we have a method, #validate_verify_info_submit, that validates the ProofingComponents structure, but not much else.

With respect to the controller specs, we primarily look at what value was logged in the event (with the exception of our threatmetrix status specs).

For now, I've updated the VerifyInfoController spec to look at what was logged in the state_id stage and am open to expanding the coverage here.

@lmgeorge lmgeorge marked this pull request as ready for review November 18, 2024 19:50
@@ -46,6 +46,7 @@ def self.step_info
idv_session.resolution_successful = nil
idv_session.verify_info_step_document_capture_session_uuid = nil
idv_session.threatmetrix_review_status = nil
idv_session.source_check_vendor = nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any specs for these undo_step things that need to be updated? Or do we just test this as a side effect of other tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, we test undo state primarily as a side effect and only test defaults for individual classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I was waiting for the pipeline to complete to verify that assumption, so taking a look now)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, seems like undo step validation is primarily tested through feature specs.

Copy link
Member

@n1zyy n1zyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice fix, and it cleans up formatting a bit as well!

app/services/idv/session.rb Show resolved Hide resolved
spec/services/idv/proofing_components_spec.rb Show resolved Hide resolved
@@ -16,6 +16,9 @@ module Vendors
MOCK = 'mock'
USPS = 'usps'
AAMVA = 'aamva'
AAMVA_UNSUPPORTED_JURISDICTION = 'UnsupportedJurisdiction'
STATE_ID_MOCK = 'StateIdMock'
SOURCE_CHECK = [AAMVA, AAMVA_UNSUPPORTED_JURISDICTION, STATE_ID_MOCK].freeze
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Possible bikeshedding) Is there a clearer name for this? I.e., SOCURE and LEXIS_NEXIS are sources that we check; can we make it obvious why they don't belong in this array? STATE_ID_SOURCE_CHECK?

Suggesting only in case you agree or have a better idea; I don't feel strongly at all here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was on the fence about what to call this, but I wanted the relationship between this and ProofingComponents#source_check to be clear and deeply coupled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back and forth about asking for state_id_source_check, but the idea here is: you have given us a document of some kind. we then verify it with the issuing source. right now that is a drivers license + AAMVA. In the near future, hopefully, that will be other document types + other verifiers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, source_check is probably good

@lmgeorge lmgeorge added status - ready for review ruby Pull requests that update Ruby code and removed status - work in progress labels Nov 18, 2024
@lmgeorge lmgeorge requested a review from matthinz November 19, 2024 17:17
…ment PII

**Why**

* Prior to this change, we always stored 'aamva' in a `User`'s
  `proofing_components.source_check` column regardless of whether a call
  to AAMVA was made or not. Accuracy is good and storing the actual
  "vendor_name" will align our analytics across our events.

**How**

* Instead of hard-coding `vendor_name` in `proofing_components.source_check`,
  we dig into the result object (as produced by the `ResultAdjudicator`)
  for the actual vendor_name. This value should always be present even
  though we don't have any validators for it.

* Adds `:source_check_vendor` field to `Idv::Session` to capture the
  vendor name actually utilized to validate the state ID attributes.

* Pulling `vendor_name` from the result object makes testing a little messier,
  cleaning up the various vendor names in our specs should be done in future work.
  For now, a new constant with the known `source_check` names
  has been added (`Idv::Constants::Vendors::SOURCE_CHECK`).

* No new specs have been added as the functionality hasn't changed,
  but the relevant specs, including one end-to-end feature test, have been updated to check the vendor
  name is in the expected list or matches a placeholder value.

changelog: Internal, Analytics, Store correct vendor name in ProofingComponents
@@ -33,7 +33,7 @@ def document_type
end

def source_check
Idp::Constants::Vendors::AAMVA if idv_session.verify_info_step_complete?
idv_session.source_check_vendor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to tweak to account for the 50/50 state:

Suggested change
idv_session.source_check_vendor
idv_session.source_check_vendor || (Idp::Constants::Vendors::AAMVA if idv_session.verify_info_step_complete?)

(you can probably come up with a better / more ruby-ish way to do ☝️ )

Basically, if the key is not in the session, we need to fall back to the old (bad) way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

let(:document_capture_session) do
DocumentCaptureSession.create(user:)
end

let(:success) { true }
let(:errors) { {} }
let(:exception) { nil }
let(:vendor_name) { :aamva }
let(:vendor_name) { 'aamva_placeholder' }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will not approve without emoji vendor name

Copy link
Member

@matthinz matthinz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this looks good, just the one 50/50 state thing

@lmgeorge lmgeorge merged commit fa0524f into main Nov 20, 2024
2 checks passed
@lmgeorge lmgeorge deleted the lmgeorge/LG-13761 branch November 20, 2024 16:16
@aduth aduth mentioned this pull request Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ruby Pull requests that update Ruby code status - ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants