Skip to content
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.

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

Braintree Blue: Update card verfification payload if billing address fields are not present #5142

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

yunnydang
Copy link
Contributor

@yunnydang yunnydang commented Jun 4, 2024

This lets us gracefully return an error code or message from Braintree if the postal zip code is missing.

Local:
5923 tests, 79804 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Unit:
103 tests, 218 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Remote:
121 tests, 649 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

@yunnydang yunnydang requested a review from a team June 4, 2024 23:45
@yunnydang yunnydang self-assigned this Jun 4, 2024
@yunnydang yunnydang force-pushed the braintree_update_verify_zip_code branch from 9c7a95f to 847e910 Compare June 5, 2024 16:41
@yunnydang yunnydang requested a review from jcreiff June 5, 2024 16:41
Copy link
Contributor

@jcreiff jcreiff left a comment

Choose a reason for hiding this comment

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

We discussed this IRL and think maybe we need further investigation:

  • Why are we only sending zip in the billing_address hash?
  • Is billing_address optional? ie, if it's empty in options should we just skip adding it to the payload?

The nature of the error message (Addresses must have at least one field filled in.) led us to wonder about this, because it doesn't seem like the Braintree API is rejecting an empty zip per se; rather, it seems like a billing_address object with no details included is the problem

That error message would also be a strange one to receive (as an end user) if there were other details in the billing_address hash, but not zip

@yunnydang
Copy link
Contributor Author

We discussed this IRL and think maybe we need further investigation:

  • Why are we only sending zip in the billing_address hash?
  • Is billing_address optional? ie, if it's empty in options should we just skip adding it to the payload?

The nature of the error message (Addresses must have at least one field filled in.) led us to wonder about this, because it doesn't seem like the Braintree API is rejecting an empty zip per se; rather, it seems like a billing_address object with no details included is the problem

That error message would also be a strange one to receive (as an end user) if there were other details in the billing_address hash, but not zip

Braintrees card verification documentation requires:
Card number
Card expiration date
Street address and postal code (however im not sure this is actually true, more on that later)
CVV
I ran some remote tests in AM to mess around with this and if we send the street_address: options[:billing_address].try(:[], :address1) but not the zip/postal code, the verification is successful, at least according to the remote test. Same with just sending the zip/postal code by itself. I think the reason why Braintree doesnt like us sending an empty billing_address is because its one of the parameters according to this doc and they expect to have some address related field within it. For example I think billing_address may actually just be optional as I've commented out that field/hash entirely and was able to get a successful verification. So in conclusion I think its like this:
For card verification, its optional to send the billing_address field
If we do send the billing_addressBraintree will expect at least one address related field (zip code, address1, or etc) to be present within it

@yunnydang yunnydang force-pushed the braintree_update_verify_zip_code branch from 847e910 to 1125374 Compare June 7, 2024 20:56
@yunnydang yunnydang requested a review from jcreiff June 7, 2024 20:57
Copy link
Contributor

@jcreiff jcreiff left a comment

Choose a reason for hiding this comment

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

I think the changes to the gateway model look good; my only request is for some updates to the remote test suite:

  • test_successful_credit_card_verification now returns a failure because the AVS Result Code comes back as M instead of P. This is expected because we are sending more information to Braintree; as I understand it, M means street and postal code match, while P means there's a postal code match but street can't be confirmed. P was accurate for the previous zip-only logic, but now we are sending more than that.
  • Can you add assertions for your new tests that check the response.avs_result['code'], similar to the above failing test?

@yunnydang yunnydang force-pushed the braintree_update_verify_zip_code branch from 1125374 to bbae39c Compare June 10, 2024 16:07
@yunnydang
Copy link
Contributor Author

  • response.avs_result['code']

Good callout! I updated the remote tests with more of the avs and cvv code assertions. Interestingly enough and expected, when not sending any address related fields, the code given back was a "B" meaning AVS checks were skipped for this transaction.

@yunnydang yunnydang requested a review from jcreiff June 10, 2024 16:08
Copy link
Contributor

@jcreiff jcreiff left a comment

Choose a reason for hiding this comment

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

🚢

@yunnydang yunnydang force-pushed the braintree_update_verify_zip_code branch from bbae39c to 8a1f34e Compare June 12, 2024 18:49
@yunnydang yunnydang changed the title Braintree Blue: add graceul failure if zipcode is not present Braintree Blue: Update card verfification payload billing address fields are not present Jun 12, 2024
@yunnydang yunnydang force-pushed the braintree_update_verify_zip_code branch from 8a1f34e to 83157ff Compare June 12, 2024 18:50
@yunnydang yunnydang force-pushed the braintree_update_verify_zip_code branch from 83157ff to 32e4da3 Compare June 12, 2024 18:51
@yunnydang yunnydang merged commit 32e4da3 into master Jun 12, 2024
5 checks passed
@yunnydang yunnydang deleted the braintree_update_verify_zip_code branch June 12, 2024 18:54
@yunnydang yunnydang changed the title Braintree Blue: Update card verfification payload billing address fields are not present Braintree Blue: Update card verfification payload if billing address fields are not present Jun 12, 2024
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.

None yet

2 participants