Single Result Objects for Public Methods #857
Conversation
| /** | ||
| * @param americanExpressResult the [AmericanExpressResult] | ||
| */ | ||
| fun onAmericanExpressResult(americanExpressResult: AmericanExpressResult?) |
There was a problem hiding this comment.
Does americanExpressResult need to be nullable? It might be easier to handle a non-null value here and wrap any situations where this result type is null into a Failure.
There was a problem hiding this comment.
Nope that was just an oversight - they should all be non-null results good catch!
...ctDebit/src/main/java/com/braintreepayments/api/SEPADirectDebitPaymentAuthRequestCallback.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This is a lot for one PR. There's 21 commits and 46 files changed...
Can we split this up into smaller PRs based on the payment method getting updated?
I know it may seem like more work up front, but it will be easier and faster for me and others to review a PR for each respected namespace (i.e. data collector, visacheckout, amex, etc).
Plus, if we need to revert a change during your major version work, the code is broken into smaller PRs.
| @@ -114,6 +122,27 @@ public void getRewardsBalance_callsListenerWithRewardsBalanceWithErrorCode_OnIns | |||
| assertEquals("Insufficient points on card", rewardsBalance.getErrorMessage()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void getRewardsBalance_callsBackFailure_OnHttpError() { | |||
There was a problem hiding this comment.
love that we're adding this test coverage. Question: do we have testing around analytic events being sent?
| deviceData.put(CORRELATION_ID_KEY, correlationId); | ||
| } | ||
| } catch (JSONException ignored) { | ||
| braintreeClient.getConfiguration((configuration, error) -> { |
There was a problem hiding this comment.
does this work need to be in the CHANGELOG?
Summary of changes
Checklist
Authors