Skip to content

Determine whether an AAMVA check is required in the progressive proofer#10796

Merged
jmhooper merged 4 commits intomainfrom
jmhooper-compute-should-proof-state-id-in-job
Jun 18, 2024
Merged

Determine whether an AAMVA check is required in the progressive proofer#10796
jmhooper merged 4 commits intomainfrom
jmhooper-compute-should-proof-state-id-in-job

Conversation

@jmhooper
Copy link
Contributor

Prior to this commit we have been passing should_proof_state_id from the VerifyInfoConcern down to the ProgressiveProofer through the async proofing tooling. This requires this argument to be passed down the call stack and stored as a background job argument.

The should_proof_state_id value is determined by configuration that is available to the IdP and the workers. It is not clear why it needs to be computed in the VerifyInfoConcern and passed down through the job to the ProgressiveProofer. This commit shortcuts everything by moving the logic closer to where the value is actually inspected.

@jmhooper
Copy link
Contributor Author

I am opening this as a draft PR because I expect this change to be incredibly problematic for many unit tests.

@jmhooper jmhooper force-pushed the jmhooper-compute-should-proof-state-id-in-job branch from 39dcf94 to e119608 Compare June 11, 2024 15:14
Copy link
Contributor

Choose a reason for hiding this comment

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

needs a rubocop:disable unused right?

Copy link
Contributor

Choose a reason for hiding this comment

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

worth dropping a comment that this is intended to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I'm also going to have to break this pull request into multiple changes since I need to make the changes in the jobs first before making any changes to the args we pass to them.

Comment on lines 135 to 137
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT of some stronger assertions here? Technically it's new behavior but it might be nice to know if any bad data slips through loudly + early

Suggested change
state_id_jurisdiction = applicant_pii['state_id_jurisdiction']
IdentityConfig.store.aamva_supported_jurisdictions.include?(state_id_jurisdiction)
state_id_jurisdiction = applicant_pii.fetch('state_id_jurisdiction')
raise "blank state_id_jurisdiction" if state_id_jurisdiction.blank?
IdentityConfig.store.aamva_supported_jurisdictions.include?(state_id_jurisdiction)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is a bit of a change. Let me do it; based on what I've seen in the tests I'll bet a bunch of unit tests will start failing and need to be tidied.

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 am going to put this back the way it was. It looks like there is some behavior that depends on this fully functioning when PII is missing:

context 'pii is missing' do
let(:applicant_pii) { {} }
it 'does not make a request to the ThreatMetrix proofer' do
expect(threatmetrix_proofer).not_to have_received(:proof)
end
it 'returns a failed result' do
device_profiling_result = proof.device_profiling_result
expect(device_profiling_result.success).to be(false)
expect(device_profiling_result.client).to eq('tmx_pii_missing')
expect(device_profiling_result.review_status).to eq('reject')
end
end

I am planning on coming through here and modifying the arguments to match the PII structs we added a while back. I'm hoping to come up with a better solution for handling malformed PII at that point.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for trying!

@jmhooper jmhooper force-pushed the jmhooper-compute-should-proof-state-id-in-job branch from e119608 to 67ce2c6 Compare June 11, 2024 16:23
jmhooper added 3 commits June 17, 2024 10:50
Prior to this commit we have been passing `should_proof_state_id` from the `VerifyInfoConcern` down to the `ProgressiveProofer` through the async proofing tooling. This requires this argument to be passed down the call stack and stored as a background job argument.

The `should_proof_state_id` value is determined by configuration that is available to the IdP and the workers. It is not clear why it needs to be computed in the `VerifyInfoConcern` and passed down through the job to the `ProgressiveProofer`. This commit shortcuts everything by moving the logic closer to where the value is actually inspected.

[skip changelog]
@jmhooper jmhooper force-pushed the jmhooper-compute-should-proof-state-id-in-job branch from 97e5f58 to f07fb51 Compare June 17, 2024 14:51
@jmhooper jmhooper marked this pull request as ready for review June 17, 2024 15:43
@jmhooper jmhooper requested a review from a team June 17, 2024 15:43
@jmhooper jmhooper merged commit df0c8a1 into main Jun 18, 2024
@jmhooper jmhooper deleted the jmhooper-compute-should-proof-state-id-in-job branch June 18, 2024 15:05
jmhooper added a commit that referenced this pull request Jun 18, 2024
…l to the `ResolutionProofingJob`

We deprecated this argument in #10796. This commit stops invoking the job with the argument. A future commit should remove the argument from the job.

[skip changelog]
jmhooper added a commit that referenced this pull request Jun 25, 2024
…l to the `ResolutionProofingJob` (#10827)

We deprecated this argument in #10796. This commit stops invoking the job with the argument. A future commit should remove the argument from the job.

[skip changelog]
Comment on lines +28 to +29
# DEPRECATED ARGUMENTS
should_proof_state_id: false # rubocop:disable Lint/UnusedMethodArgument
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we intending to remove this at some point?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants