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

Fix `Transaction` field names: `apple_pay` and `android_pay_card` #60

Merged
merged 5 commits into from May 24, 2019

Conversation

@balexand
Copy link
Contributor

commented May 22, 2019

No description provided.

@@ -18,7 +18,7 @@ defmodule Braintree.Transaction do
additional_processor_response: String.t(),
amount: number,
android_pay_details: String.t(),
apple_pay_details: String.t(),
apple_pay: map,

This comment has been minimized.

Copy link
@sorentwo

sorentwo May 23, 2019

Owner

Is there a reference to when this change was made? Also, does it change android_pay_details as well?

This comment has been minimized.

Copy link
@balexand

balexand May 23, 2019

Author Contributor

Here's a line in the official Braintree Python lib showing this field name. And here's the commit where it was added 5 years ago. I think braintree-elixir has had the wrong name the whole time and nobody has noticed. Thanks for being careful and double checking before merging 😄 .

I have integration tests in my system that make requests to the Braintree Sandbox, and I have test coverage verifying this PR as well as #54 and #57. I included a test in #57, but this PR, #54, and Google Pay could use coverage.

For Google Pay, it looks like the field is android_pay_card.

I'll try to find time today to do the following:

  • Add integration test coverage to this PR (or separately if you merge it first).
  • Create a pull-request with integration test coverage for #54.
  • Create a pull-request for android_pay_card with integration test coverage.

Thanks for maintaining this library!

@balexand balexand changed the title Fix `Transaction.apple_pay` field name Fix `Transaction` field names: `apple_pay` and `android_pay_card` May 23, 2019
@balexand

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

I've updated the Android Pay field name. I'm pretty sure #59 was untested.

@balexand

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

I've added some related fixes and integration test coverage. Braintree's inconsistent naming is a little disappointing. Also, it looks like with PayPal they return a status of "settling" instead of "submitted_for_settlement" 😞 . Hurray for integration tests that make real requests. This library could try to handle inconsistencies like that but it might turn into a huge job to do that throughout the API.

@sorentwo sorentwo merged commit dbc0d93 into sorentwo:master May 24, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sorentwo

This comment has been minimized.

Copy link
Owner

commented May 24, 2019

Thanks for the fixes and the integration test additions, I really appreciate the effort. 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.