Skip to content

LG-9865: Add same_address_as_id to in person verify visited analytics #8585

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.

Sign up for GitHub

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

Merged
gina-yamada merged 7 commits into from
Jun 13, 2023
Merged

LG-9865: Add same_address_as_id to in person verify visited analytics #8585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3c4a185
add same_address_as_id to verify visited analytics
gina-yamada Jun 12, 2023
0d6edd6
mark in person verify info set as complete
gina-yamada Jun 12, 2023
52731f9
changelog: Interal, In-person proofing, Add same_address_as_id to in …
gina-yamada Jun 12, 2023
0f3c14c
Moved setting value on flow session to verify info concern
gina-yamada Jun 13, 2023
d825718
Merge branch 'main' of github.com:18F/identity-idp into gyamada/fix-v…
gina-yamada Jun 13, 2023
3bcdc04
Update app/controllers/concerns/idv/verify_info_concern.rb
gina-yamada Jun 13, 2023
b41acd5
Modified how I was accessing data for readability
gina-yamada Jun 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/controllers/concerns/idv/verify_info_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ def update
flow_session[:flow_path] ||= 'standard'
end

# Mark the FSM verify step completed. This is for the 50/50 state
flow_session['Idv::Steps::InPerson::VerifyStep'] = true
redirect_to after_update_url
end

Expand Down
16 changes: 14 additions & 2 deletions app/controllers/idv/in_person/verify_info_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,23 @@ def flow_session

def analytics_arguments
{
flow_path: flow_path,
flow_path: flow_session[:flow_path],
step: 'verify',
analytics_id: 'In Person Proofing',
irs_reproofing: irs_reproofing?,
}.merge(**acuant_sdk_ab_test_analytics_args)
}.merge(**acuant_sdk_ab_test_analytics_args).
merge(**extra_analytics_properties)
end

def extra_analytics_properties
extra = {
pii_like_keypaths: [[:same_address_as_id], [:state_id, :state_id_jurisdiction]],
}
unless flow_session.dig(:pii_from_user, :same_address_as_id).nil?
extra[:same_address_as_id] =
flow_session[:pii_from_user][:same_address_as_id].to_s == 'true'
Copy link
Contributor

@JackRyan1989 JackRyan1989 Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just curious, but why are we coercing to a string here? Do we not know the type of :same_address_as_id?

In the spec it's a boolean, so if that's the case then you shouldn't have to coerce the type or compare the value.

Copy link
Contributor Author

@gina-yamada gina-yamada Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@JackRyan1989 This bit of code was taken from in_person_flow.rb, I kept it the exact same as is it just missing on analytics when moving verify out of the flow state machine (discovered in the delete PR). There is a ticket, LG-9543, to refactor all string instances of same_address_as_id to a boolean because both are being used. That ticket was started but presented roadblocks. How do you feel about that being handled in ticket LG-9543?

Copy link
Contributor

@JackRyan1989 JackRyan1989 Jun 13, 2023

Choose a reason for hiding this comment

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

Sounds good!

Super nit: Maybe change the "same_address_as_id" to be a string in the test then? This is not a blocking comment.

Copy link
Contributor Author

@gina-yamada gina-yamada Jun 13, 2023

Choose a reason for hiding this comment

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

This is not needed. Analytic data is a boolean. @JackRyan1989 and I jumped on a call.

end
extra
end
end
end
Expand Down
4 changes: 3 additions & 1 deletion spec/controllers/idv/in_person/verify_info_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
RSpec.describe Idv::InPerson::VerifyInfoController do
include IdvHelper

let(:pii_from_user) { Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN.dup }
let(:pii_from_user) { Idp::Constants::MOCK_IDV_APPLICANT_SAME_ADDRESS_AS_ID.dup }
let(:flow_session) do
{ 'document_capture_session_uuid' => 'fd14e181-6fb1-4cdc-92e0-ef66dad0df4e',
:pii_from_user => pii_from_user,
Expand Down Expand Up @@ -67,6 +67,8 @@
flow_path: 'standard',
irs_reproofing: false,
step: 'verify',
same_address_as_id: true,
pii_like_keypaths: [[:same_address_as_id], [:state_id, :state_id_jurisdiction]],
Copy link
Contributor

@soniaconnolly soniaconnolly Jun 12, 2023

Choose a reason for hiding this comment

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

I just checked, and the first thing Analytics#track_events does is attributes.delete(:pii_like_keypaths), so that's why they don't show up in the feature specs.

}
end

Expand Down