Skip to content

Restructure analytics error_details as hash #9572

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
aduth merged 1 commit into from
Nov 16, 2023
Merged

Restructure analytics error_details as hash #9572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 8 additions & 3 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 @@ -192,9 +192,14 @@ def async_state_done(current_async_state)
extra: {
address_edited: !!idv_session.address_edited,
address_line2_present: !pii[:address2].blank?,
pii_like_keypaths: [[:errors, :ssn], [:response_body, :first_name],
[:same_address_as_id],
[:state_id, :state_id_jurisdiction]],
pii_like_keypaths: [
[:errors, :ssn],
[:proofing_results, :context, :stages, :resolution, :errors, :ssn],
[:proofing_results, :context, :stages, :residential_address, :errors, :ssn],
[:proofing_results, :context, :stages, :threatmetrix, :response_body, :first_name],
[:same_address_as_id],
[:proofing_results, :context, :stages, :state_id, :state_id_jurisdiction],
],
},
)
log_idv_verification_submitted_event(
Expand Down
5 changes: 4 additions & 1 deletion app/controllers/concerns/idv_step_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,10 @@ def confirm_address_step_complete

def extra_analytics_properties
extra = {
pii_like_keypaths: [[:same_address_as_id], [:state_id, :state_id_jurisdiction]],
pii_like_keypaths: [
[:same_address_as_id],
[:proofing_results, :context, :stages, :state_id, :state_id_jurisdiction],
],
}

unless flow_session.dig(:pii_from_user, :same_address_as_id).nil?
Expand Down
6 changes: 5 additions & 1 deletion app/controllers/users/verify_personal_key_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ def create

analytics.personal_key_reactivation_submitted(
**result.to_h,
pii_like_keypaths: [[:errors, :personal_key], [:error_details, :personal_key]],
pii_like_keypaths: [
[:errors, :personal_key],
[:error_details, :personal_key],
[:error_details, :personal_key, :personal_key],
],
)
irs_attempts_api_tracker.personal_key_reactivation_submitted(
success: result.success?,
Expand Down
2 changes: 1 addition & 1 deletion app/forms/idv/doc_pii_form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ def submit
def self.pii_like_keypaths
keypaths = [[:pii]]
attrs = %i[name dob dob_min_age address1 state zipcode jurisdiction]
keypaths << attrs
attrs.each do |k|
keypaths << [:errors, k]
keypaths << [:error_details, k]
keypaths << [:error_details, k, k]
end
keypaths
end
Expand Down
4 changes: 3 additions & 1 deletion app/services/form_response.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ def merge_arrays(_key, first, second)
end

def flatten_details(details)
details.transform_values { |errors| errors.pluck(:error) }
details.transform_values do |errors|
errors.map { |error| error[:type] || error[:error] }.index_with(true)
end
end

attr_reader :success
Expand Down
5 changes: 4 additions & 1 deletion app/services/idv/flows/in_person_flow.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ def self.session_idv(session)

def extra_analytics_properties
extra = {
pii_like_keypaths: [[:same_address_as_id], [:state_id, :state_id_jurisdiction]],
pii_like_keypaths: [
[:same_address_as_id],
[:proofing_results, :context, :stages, :state_id, :state_id_jurisdiction],
],
}
unless @flow_session[:pii_from_user]&.[](:same_address_as_id).nil?
extra[:same_address_as_id] =
Expand Down
2 changes: 1 addition & 1 deletion app/services/irs_attempts_api/tracker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ def track_event(event_type, metadata = {})
end

def parse_failure_reason(result)
return result.to_h[:error_details] || result.errors.presence
return result.to_h[:error_details]&.transform_values(&:keys) || result.errors.presence
end
Comment on lines 8 to 10
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Nov 9, 2023

Choose a reason for hiding this comment

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

we could probably yank every usage of this method and just inline it to failure_reason: form_response.error_details

Copy link
Copy Markdown
Contributor Author

@aduth aduth Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

At this point, that's just more work? This mostly preserves the original intent, if we're okay to keep as-is.

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Nov 9, 2023

Choose a reason for hiding this comment

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

yeah I might make a second PR to do that, because it just means we have less code hanging around that deals with these structures, so it's less to worry about if we ever have to make another similar refactor

end
end
3 changes: 2 additions & 1 deletion app/services/password_reset_token_validator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ def submit
attr_accessor :user

def valid_token
errors.add(:user, 'token_expired', type: :user) unless user.reset_password_period_valid?
return if user.reset_password_period_valid?
errors.add(:user, 'token_expired', type: :token_expired)
end
end
6 changes: 3 additions & 3 deletions spec/controllers/account_reset/cancel_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
success: false,
errors: { token: [t('errors.account_reset.cancel_token_invalid', app_name: APP_NAME)] },
error_details: {
token: [t('errors.account_reset.cancel_token_invalid', app_name: APP_NAME)],
token: { cancel_token_invalid: true },
},
user_id: 'anonymous-uuid',
}
Expand All @@ -55,7 +55,7 @@
analytics_hash = {
success: false,
errors: { token: [t('errors.account_reset.cancel_token_missing', app_name: APP_NAME)] },
error_details: { token: [:blank] },
error_details: { token: { blank: true } },
user_id: 'anonymous-uuid',
}

Expand Down Expand Up @@ -100,7 +100,7 @@
success: false,
errors: { token: [t('errors.account_reset.cancel_token_invalid', app_name: APP_NAME)] },
error_details: {
token: [t('errors.account_reset.cancel_token_invalid', app_name: APP_NAME)],
token: { cancel_token_invalid: true },
},
}
expect(@analytics).to receive(:track_event).
Expand Down
14 changes: 5 additions & 9 deletions spec/controllers/account_reset/delete_account_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
user_id: 'anonymous-uuid',
success: false,
errors: invalid_token_error,
error_details: invalid_token_error,
error_details: { token: { granted_token_invalid: true } },
mfa_method_counts: {},
pii_like_keypaths: [[:mfa_method_counts, :phone]],
account_age_in_days: 0,
Expand All @@ -60,7 +60,7 @@
user_id: 'anonymous-uuid',
success: false,
errors: { token: [t('errors.account_reset.granted_token_missing', app_name: APP_NAME)] },
error_details: { token: [:blank] },
error_details: { token: { blank: true } },
mfa_method_counts: {},
pii_like_keypaths: [[:mfa_method_counts, :phone]],
account_age_in_days: 0,
Expand All @@ -86,9 +86,7 @@
user_id: user.uuid,
success: false,
errors: { token: [t('errors.account_reset.granted_token_expired', app_name: APP_NAME)] },
error_details: {
token: [t('errors.account_reset.granted_token_expired', app_name: APP_NAME)],
},
error_details: { token: { granted_token_expired: true } },
mfa_method_counts: {},
pii_like_keypaths: [[:mfa_method_counts, :phone]],
account_age_in_days: 2,
Expand All @@ -114,7 +112,7 @@
user_id: 'anonymous-uuid',
success: false,
errors: invalid_token_error,
error_details: invalid_token_error,
error_details: { token: { granted_token_invalid: true } },
}
expect(@analytics).to receive(:track_event).
with('Account Reset: granted token validation', properties)
Expand All @@ -134,9 +132,7 @@
user_id: user.uuid,
success: false,
errors: { token: [t('errors.account_reset.granted_token_expired', app_name: APP_NAME)] },
error_details: {
token: [t('errors.account_reset.granted_token_expired', app_name: APP_NAME)],
},
error_details: { token: { granted_token_expired: true } },
}
expect(@analytics).to receive(:track_event).
with('Account Reset: granted token validation', properties)
Expand Down
9 changes: 4 additions & 5 deletions spec/controllers/idv/by_mail/enter_code_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,6 @@

describe '#create' do
let(:otp_code_error_message) { { otp: [t('errors.messages.confirmation_code_incorrect')] } }
let(:otp_code_incorrect) { { otp: [:confirmation_code_incorrect] } }
let(:success_properties) { { success: true, failure_reason: nil } }

subject(:action) do
Expand Down Expand Up @@ -387,11 +386,11 @@
which_letter: nil,
letter_count: 1,
attempts: 1,
error_details: otp_code_incorrect,
error_details: { otp: { confirmation_code_incorrect: true } },
pii_like_keypaths: [[:errors, :otp], [:error_details, :otp]],
)
expect(@irs_attempts_api_tracker).to receive(:idv_gpo_verification_submitted).
with(success: false, failure_reason: otp_code_incorrect)
with(success: false, failure_reason: { otp: [:confirmation_code_incorrect] })

action

Expand Down Expand Up @@ -425,7 +424,7 @@
which_letter: nil,
letter_count: 1,
attempts: 1,
error_details: otp_code_incorrect,
error_details: { otp: { confirmation_code_incorrect: true } },
pii_like_keypaths: [[:errors, :otp], [:error_details, :otp]],
}
expect(@analytics).to receive(:track_event).with(
Expand Down Expand Up @@ -474,7 +473,7 @@
which_letter: nil,
letter_count: 1,
attempts: 1,
error_details: otp_code_incorrect,
error_details: { otp: { confirmation_code_incorrect: true } },
pii_like_keypaths: [[:errors, :otp], [:error_details, :otp]],
).exactly(max_attempts - 1).times
expect(@analytics).to receive(:track_event).with(
Expand Down
12 changes: 6 additions & 6 deletions spec/controllers/idv/image_uploads_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
front: ['Please fill in this field.'],
},
error_details: {
front: [:blank],
front: { blank: true },
},
user_id: user.uuid,
attempts: 1,
Expand Down Expand Up @@ -123,7 +123,7 @@
front: [I18n.t('doc_auth.errors.not_a_file')],
},
error_details: {
front: [I18n.t('doc_auth.errors.not_a_file')],
front: { not_a_file: true },
},
user_id: user.uuid,
attempts: 1,
Expand Down Expand Up @@ -261,7 +261,7 @@
limit: [I18n.t('errors.doc_auth.rate_limited_heading')],
},
error_details: {
limit: [I18n.t('errors.doc_auth.rate_limited_heading')],
limit: { rate_limited: true },
},
user_id: user.uuid,
attempts: IdentityConfig.store.doc_auth_max_attempts,
Expand Down Expand Up @@ -603,7 +603,7 @@
name: [I18n.t('doc_auth.errors.alerts.full_name_check')],
},
error_details: {
name: [I18n.t('doc_auth.errors.alerts.full_name_check')],
name: { name: true },
},
attention_with_barcode: false,
user_id: user.uuid,
Expand Down Expand Up @@ -703,7 +703,7 @@
state: [I18n.t('doc_auth.errors.general.no_liveness')],
},
error_details: {
state: [:wrong_length],
state: { wrong_length: true },
},
attention_with_barcode: false,
user_id: user.uuid,
Expand Down Expand Up @@ -803,7 +803,7 @@
dob: [I18n.t('doc_auth.errors.alerts.birth_date_checks')],
},
error_details: {
dob: [I18n.t('doc_auth.errors.alerts.birth_date_checks')],
dob: { dob: true },
},
attention_with_barcode: false,
user_id: user.uuid,
Expand Down
7 changes: 5 additions & 2 deletions spec/controllers/idv/in_person/ssn_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@
irs_reproofing: false,
step: 'ssn',
same_address_as_id: true,
pii_like_keypaths: [[:same_address_as_id], [:state_id, :state_id_jurisdiction]],
pii_like_keypaths: [
[:same_address_as_id],
[:proofing_results, :context, :stages, :state_id, :state_id_jurisdiction],
],
}.merge(ab_test_args)
end

Expand Down Expand Up @@ -199,7 +202,7 @@
errors: {
ssn: ['Enter a nine-digit Social Security number'],
},
error_details: { ssn: [:invalid] },
error_details: { ssn: { invalid: true } },
same_address_as_id: true,
pii_like_keypaths: [[:same_address_as_id], [:errors, :ssn], [:error_details, :ssn]],
}.merge(ab_test_args)
Expand Down
16 changes: 8 additions & 8 deletions spec/controllers/idv/phone_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,12 +239,6 @@
allow(subject).to receive(:ab_test_analytics_buckets).and_return(ab_test_args)
end
context 'when form is invalid' do
let(:improbable_phone_error) do
{
phone: [:improbable_phone],
otp_delivery_preference: [:inclusion],
}
end
let(:improbable_phone_message) { t('errors.messages.improbable_phone') }
let(:improbable_otp_message) { 'is not included in the list' }
let(:improbable_phone_number) { '703' }
Expand Down Expand Up @@ -285,7 +279,10 @@
expect(@irs_attempts_api_tracker).to receive(:idv_phone_submitted).with(
success: false,
phone_number: improbable_phone_number,
failure_reason: improbable_phone_error,
failure_reason: {
phone: [:improbable_phone],
otp_delivery_preference: [:inclusion],
},
)

put :create, params: improbable_phone_form
Expand All @@ -296,7 +293,10 @@
phone: [improbable_phone_message],
otp_delivery_preference: [improbable_otp_message],
},
error_details: improbable_phone_error,
error_details: {
phone: { improbable_phone: true },
otp_delivery_preference: { inclusion: true },
},
pii_like_keypaths: [[:errors, :phone], [:error_details, :phone]],
country_code: nil,
area_code: nil,
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/idv/ssn_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@
errors: {
ssn: [t('idv.errors.pattern_mismatch.ssn')],
},
error_details: { ssn: [:invalid] },
error_details: { ssn: { invalid: true } },
pii_like_keypaths: [[:same_address_as_id], [:errors, :ssn], [:error_details, :ssn]],
}.merge(ab_test_args)
end
Expand Down
10 changes: 5 additions & 5 deletions spec/controllers/saml_idp_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ def name_id_version(format_urn)
analytics_hash = {
success: false,
errors: { authn_context: [t('errors.messages.unauthorized_authn_context')] },
error_details: { authn_context: [:unauthorized_authn_context] },
error_details: { authn_context: { unauthorized_authn_context: true } },
nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT,
authn_context: ['http://idmanagement.gov/ns/assurance/loa/5'],
authn_context_comparison: 'exact',
Expand Down Expand Up @@ -984,7 +984,7 @@ def name_id_version(format_urn)
analytics_hash = {
success: false,
errors: { service_provider: [t('errors.messages.unauthorized_service_provider')] },
error_details: { service_provider: [:unauthorized_service_provider] },
error_details: { service_provider: { unauthorized_service_provider: true } },
nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT,
authn_context: request_authn_contexts,
authn_context_comparison: 'exact',
Expand Down Expand Up @@ -1026,8 +1026,8 @@ def name_id_version(format_urn)
authn_context: [t('errors.messages.unauthorized_authn_context')],
},
error_details: {
authn_context: [:unauthorized_authn_context],
service_provider: [:unauthorized_service_provider],
authn_context: { unauthorized_authn_context: true },
service_provider: { unauthorized_service_provider: true },
},
nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT,
authn_context: ['http://idmanagement.gov/ns/assurance/loa/5'],
Expand Down Expand Up @@ -1417,7 +1417,7 @@ def name_id_version(format_urn)
analytics_hash = {
success: false,
errors: { nameid_format: [t('errors.messages.unauthorized_nameid_format')] },
error_details: { nameid_format: [:unauthorized_nameid_format] },
error_details: { nameid_format: { unauthorized_nameid_format: true } },
nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL,
authn_context: request_authn_contexts,
authn_context_comparison: 'exact',
Expand Down
Loading