Skip to content

Add vendor reasons to proofing step analytics #996

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
monfresh merged 1 commit into from
Jan 26, 2017
Merged

Add vendor reasons to proofing step analytics #996

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
4 changes: 2 additions & 2 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ GIT

GIT
remote: https://github.com/18F/identity-proofer-gem.git
revision: c671e7964a1e4693b500da6fdde7c023beda71b1
revision: 4d0d90c06c102361d310e02edba8a64e4d25cc39
branch: master
specs:
proofer (1.0.0)
Expand Down Expand Up @@ -221,7 +221,7 @@ GEM
rails (>= 3.1.1)
diff-lcs (1.2.5)
docile (1.1.5)
dotenv (2.1.1)
dotenv (2.1.2)
dotiw (3.1.1)
actionpack (>= 3)
i18n
Expand Down
7 changes: 6 additions & 1 deletion app/services/idv/financials_step.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ def vendor_errors
vendor_validator.errors if form_valid?
end

def vendor_reasons
vendor_validator.reasons if form_valid?
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jan 26, 2017

Choose a reason for hiding this comment

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

if form_valid? that means we didn't talk to our vendor, correct? Otherwise we want reasons for both success and failure right?

Copy link
Copy Markdown
Contributor Author

@pkarman pkarman Jan 26, 2017

Choose a reason for hiding this comment

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

we only get reasons if we have talked to the vendor, and we only talk to the vendor if the form is valid. Reasons will include everything the vendor responded with, good and bad, about the final pass/fail they gave.

Copy link
Copy Markdown
Contributor

@monfresh monfresh Jan 26, 2017

Choose a reason for hiding this comment

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

If the form itself is not valid (based on our internal validations), then we don't want to contact the vendor at all because it would be a waste of resources. The Form validation errors are already captured at this point. If the Form is valid, then we perform the second validation with the vendor, and we capture the errors.

However, I question the implementation of reasons in the proofer gem. It seems to me that the reasons should be part of the errors, just like they are in Rails. I would expect to see something like this:

result = {
  success: false,
  idv_attempts_exceeded: false,
  errors: {
    first_name: ['The name was suspicious.']
  }
}

as opposed to this:

result = {
  success: false,
  idv_attempts_exceeded: false,
  errors: {
    first_name: ['Unverified first name.']
  },
  reasons: ['The name was suspicious']
}

where it's not clear which fields the reasons are referring to.

Copy link
Copy Markdown
Contributor

@monfresh monfresh Jan 26, 2017

Choose a reason for hiding this comment

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

What do you think about consolidating the bad reasons into the errors hash, and display the good reasons only in the success scenario, and call it something like vendor_success_details?

Copy link
Copy Markdown
Contributor Author

@pkarman pkarman Jan 26, 2017

Choose a reason for hiding this comment

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

The vendor doesn't specifically tell us which fields the reasons belong to. They only give us a list of reasons. All this PR does is propagate that data to the logs.

Copy link
Copy Markdown
Contributor Author

@pkarman pkarman Jan 26, 2017

Choose a reason for hiding this comment

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

Also, errors are a subset of reasons, not the other way around. Reasons can be positive (indicating why something passed).

Copy link
Copy Markdown
Contributor Author

@pkarman pkarman Jan 26, 2017

Choose a reason for hiding this comment

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

@monfresh I'm open to doing that in future, but I think it should be done completely in the vendor-specific gem, where all that vendor-specific domain knowledge is located. At this point we want to simply log everything the vendor gives us.

Copy link
Copy Markdown
Contributor

@monfresh monfresh Jan 26, 2017

Choose a reason for hiding this comment

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

I see. Fair enough. How about just changing the field name from reasons to vendor_details to make it clear where those messages came from?

Copy link
Copy Markdown
Contributor Author

@pkarman pkarman Jan 26, 2017

Choose a reason for hiding this comment

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

Sure. Refactored to create vendor namespace at the top level with key reasons within it. I suspect we will be adding more to the vendor in near future (probably score etc).

Copy link
Copy Markdown
Contributor

@monfresh monfresh Jan 26, 2017

Choose a reason for hiding this comment

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

Sounds good. Thanks!

end

def vendor_params
finance_type = idv_form.finance_type
{ finance_type => idv_form.idv_params[finance_type] }
Expand All @@ -37,7 +41,8 @@ def vendor_params
def result
{
success: success,
errors: errors
errors: errors,
vendor: { reasons: vendor_reasons }
}
end
end
Expand Down
4 changes: 4 additions & 0 deletions app/services/idv/phone_step.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ def vendor_errors
vendor_validator.errors if form_valid?
end

def vendor_reasons
vendor_validator.reasons if form_valid?
end

def update_idv_session
idv_session.phone_confirmation = true
idv_session.params = idv_form.idv_params
Expand Down
8 changes: 7 additions & 1 deletion app/services/idv/profile_step.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,17 @@ def vendor_errors
idv_session.resolution.try(:errors)
end

def vendor_reasons
resolution = idv_session.resolution
resolution.vendor_resp.reasons if resolution
end

def analytics_result
{
success: complete?,
idv_attempts_exceeded: attempts_exceeded?,
errors: errors
errors: errors,
vendor: { reasons: vendor_reasons }
}
end

Expand Down
2 changes: 1 addition & 1 deletion app/services/idv/step.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def analytics_event
end

def analytics_result
{ success: complete?, errors: errors }
{ success: complete?, errors: errors, vendor: { reasons: vendor_reasons } }
end

def track_event
Expand Down
4 changes: 4 additions & 0 deletions app/services/idv/vendor_validator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ def initialize(idv_session:, vendor_params:)
@vendor_params = vendor_params
end

def reasons
result.vendor_resp.reasons
end

def validate
raise NotImplementedError "Must implement validate for #{self}"
end
Expand Down
11 changes: 7 additions & 4 deletions spec/controllers/verify/finance_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@

put :create, idv_finance_form: { finance_type: :ccn, ccn: '12345678' }

result = { success: true, errors: {} }
result = { success: true, errors: {}, vendor: { reasons: ['Good number'] } }

expect(@analytics).to have_received(:track_event).with(
Analytics::IDV_FINANCE_CONFIRMATION, result
Expand Down Expand Up @@ -135,7 +135,8 @@

result = {
success: true,
errors: {}
errors: {},
vendor: { reasons: ['Good number'] }
}

expect(@analytics).to have_received(:track_event).with(
Expand All @@ -150,7 +151,8 @@

result = {
success: false,
errors: { ccn: ['The ccn could not be verified.'] }
errors: { ccn: ['The ccn could not be verified.'] },
vendor: { reasons: ['Bad number'] }
}

expect(@analytics).to have_received(:track_event).
Expand All @@ -166,7 +168,8 @@

result = {
success: false,
errors: { ccn: ['Credit card number should be only last 8 digits.'] }
errors: { ccn: ['Credit card number should be only last 8 digits.'] },
vendor: { reasons: nil }
}

expect(@analytics).to have_received(:track_event).
Expand Down
9 changes: 6 additions & 3 deletions spec/controllers/verify/sessions_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@
result = {
success: false,
idv_attempts_exceeded: false,
errors: { ssn: [t('idv.errors.duplicate_ssn')] }
errors: { ssn: [t('idv.errors.duplicate_ssn')] },
vendor: { reasons: nil }
}

expect(@analytics).to receive(:track_event).
Expand Down Expand Up @@ -140,7 +141,8 @@
idv_attempts_exceeded: false,
errors: {
first_name: ['Unverified first name.']
}
},
vendor: { reasons: ['The name was suspicious'] }
}

expect(@analytics).to have_received(:track_event).
Expand All @@ -166,7 +168,8 @@
result = {
success: true,
idv_attempts_exceeded: false,
errors: {}
errors: {},
vendor: { reasons: ['Everything looks good'] }
}

expect(@analytics).to have_received(:track_event).
Expand Down
9 changes: 6 additions & 3 deletions spec/services/idv/financials_step_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ def build_step(params)

result = {
success: false,
errors: { ccn: [t('idv.errors.invalid_ccn')] }
errors: { ccn: [t('idv.errors.invalid_ccn')] },
vendor: { reasons: nil }
}

expect(step.submit).to eq result
Expand All @@ -36,7 +37,8 @@ def build_step(params)

result = {
success: true,
errors: {}
errors: {},
vendor: { reasons: ['Good number'] }
}

expect(step.submit).to eq result
Expand All @@ -49,7 +51,8 @@ def build_step(params)

result = {
success: false,
errors: { ccn: ['The ccn could not be verified.'] }
errors: { ccn: ['The ccn could not be verified.'] },
vendor: { reasons: ['Bad number'] }
}

expect(step.submit).to eq result
Expand Down
12 changes: 8 additions & 4 deletions spec/services/idv/profile_step_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ def expect_analytics_result(result)
result = {
success: true,
idv_attempts_exceeded: false,
errors: {}
errors: {},
vendor: { reasons: ['Everything looks good'] }
}

expect_analytics_result(result)
Expand All @@ -62,7 +63,8 @@ def expect_analytics_result(result)
idv_attempts_exceeded: false,
errors: {
ssn: ['Unverified SSN.']
}
},
vendor: { reasons: ['The SSN was suspicious'] }
}

expect_analytics_result(result)
Expand All @@ -79,7 +81,8 @@ def expect_analytics_result(result)
idv_attempts_exceeded: false,
errors: {
first_name: ['Unverified first name.']
}
},
vendor: { reasons: ['The name was suspicious'] }
}

expect_analytics_result(result)
Expand All @@ -98,7 +101,8 @@ def expect_analytics_result(result)
result = {
success: false,
idv_attempts_exceeded: true,
errors: {}
errors: {},
vendor: { reasons: nil }
}

expect_analytics_result(result)
Expand Down