Skip to content

Add vendor reasons to proofing step analytics#996

Merged
monfresh merged 1 commit intomasterfrom
pek-vendor-logging
Jan 26, 2017
Merged

Add vendor reasons to proofing step analytics#996
monfresh merged 1 commit intomasterfrom
pek-vendor-logging

Conversation

@pkarman
Copy link
Copy Markdown
Contributor

@pkarman pkarman commented Jan 26, 2017

Why: Track vendor rationales for success/failure in each
verification step.

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, one quick question

Copy link
Copy Markdown
Contributor

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

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

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

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

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

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

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

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

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

Choose a reason for hiding this comment

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

Sounds good. Thanks!

@pkarman pkarman force-pushed the pek-vendor-logging branch from 8923344 to 8edddef Compare January 26, 2017 20:33
**Why**: Track vendor rationales for success/failure in each
verification step.
@pkarman pkarman force-pushed the pek-vendor-logging branch from 8edddef to 0f91f2c Compare January 26, 2017 22:58
Copy link
Copy Markdown
Contributor

@monfresh monfresh left a comment

Choose a reason for hiding this comment

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

LGTM

@monfresh monfresh merged commit 8497969 into master Jan 26, 2017
@monfresh monfresh deleted the pek-vendor-logging branch January 26, 2017 23:22
amoose pushed a commit that referenced this pull request Mar 7, 2017
**Why**: Track vendor rationales for success/failure in each
verification step.
amoose pushed a commit that referenced this pull request Mar 8, 2017
**Why**: Track vendor rationales for success/failure in each
verification step.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants