Skip to content

Fix wiring up of uuid_prefix and request_ip for TMX#6851

Merged
zachmargolis merged 4 commits intomainfrom
margolis-tmx-ip-address
Aug 26, 2022
Merged

Fix wiring up of uuid_prefix and request_ip for TMX#6851
zachmargolis merged 4 commits intomainfrom
margolis-tmx-ip-address

Conversation

@zachmargolis
Copy link
Contributor

And a few other fixes:

  • updating the mock proofer to be more realistic
  • update plumbing
  • remove uuid_prefix as a job argument since it's already
    inside applicant_pii

changelog: Internal, Bug fixes, Updates data sent to vendors

And a few other fixes:
- updating the mock proofer to be more realistic
- update plumbing
- remove uuid_prefix as a job argument since it's already
  inside applicant_pii

changelog: Internal, Bug fixes, Updates data sent to vendors
@zachmargolis zachmargolis requested a review from a team August 26, 2022 18:19

def perform(result_id:, encrypted_arguments:, trace_id:, should_proof_state_id:,
dob_year_only:, user_id: nil, threatmetrix_session_id: nil,
uuid_prefix: nil, request_ip: nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is set here

pii[:uuid_prefix] = ServiceProvider.find_by(issuer: sp_session[:issuer])&.app_id
and lives in applicant_pii already.

Additionally, it was never set in idv/agents.rb so it is safe to remove this argument here

Comment on lines -118 to +116
ddp_pii[:input_ip_address] = request_ip
ddp_pii[:local_attrib_1] = uuid_prefix
ddp_pii[:request_ip] = request_ip
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 DDP proofer reads :uuid_prefix and :request_ip from the applicant:

input_ip_address: applicant[:request_ip],
local_attrib_1: applicant[:uuid_prefix],

Copy link
Contributor

@stevegsa stevegsa left a comment

Choose a reason for hiding this comment

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

Looks great!

double(
'request',
remote_ip: Faker::Internet.ip_v4_address,
headers: { 'X-Amzn-Trace-Id' => amzn_trace_id },
Copy link
Contributor

@stevegsa stevegsa Aug 26, 2022

Choose a reason for hiding this comment

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

just one spec failing on trace_id

Copy link
Contributor

Choose a reason for hiding this comment

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

huh, this spec passes for me

Copy link
Contributor Author

@zachmargolis zachmargolis Aug 26, 2022

Choose a reason for hiding this comment

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

spec/services/idv/steps/verify_step_spec.rb worked, but its cousin in features spec/features/idv/doc_auth/verify_step_spec.rb was failing, fixed in 9e4acce

double(
'request',
remote_ip: Faker::Internet.ip_v4_address,
headers: { 'X-Amzn-Trace-Id' => amzn_trace_id },
Copy link
Contributor

Choose a reason for hiding this comment

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

huh, this spec passes for me


def attributes
required_attributes.concat optional_attributes
[*required_attributes, *optional_attributes]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.concat is a mutation (same as +=) and this was causing some test flakes because it was modifying the required attributes array, and the new specs that read these accessors were sometimes seeing extra attributes

@zachmargolis zachmargolis merged commit de28b66 into main Aug 26, 2022
@zachmargolis zachmargolis deleted the margolis-tmx-ip-address branch August 26, 2022 21:08
@aduth aduth mentioned this pull request Aug 30, 2022
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