Google pay single result #842

sarahkoop · 2023-12-05T21:21:58Z

Summary of changes

Add single result object patter to all public Google Pay APIs to align with result handling pattern across payment modules

Checklist

Added a changelog entry

Authors

@sarahkoop

BraintreeCore/schemas/com.braintreepayments.api.AnalyticsDatabase/1.json

GooglePay/src/main/java/com/braintreepayments/api/GooglePayReadinessResult.kt

GooglePay/src/main/java/com/braintreepayments/api/GooglePayClient.java

…ent.java Co-authored-by: sshropshire <[email protected]>

sshropshire · 2023-12-07T20:31:55Z

GooglePay/src/main/java/com/braintreepayments/api/GooglePayReadinessResult.kt

+     * The Google Pay API is supported or not set up on this device, or there was an [error]
+     * determining readiness.
+     */
+    class NotReadyToPay(val error: Exception?) : GooglePayReadinessResult()


Suggested change

class NotReadyToPay(val error: Exception?) : GooglePayReadinessResult()

class NotReadyToPay(val cause: Exception?) : GooglePayReadinessResult()

nit: There is an Exception constructor public Exception(Throwable cause). We could borrow the name cause to signal to merchants that the exception was produced by an underlying cause.

^ The cause property also allows us to chain stack traces. It'll be super useful for GitHub inbounds if we leverage that constructor internally when creating BraintreeException instances.

Updated - do you think we should change to cause for all Failure results or just this one?

I think wherever it feels right. That may be a separate PR, but sometimes the OG stack trace can provide a lot of context to get to the root issue, especially with everything being asynchronous.

Ok I've created another ticket to look into where else in these result objects it would make sense to return throwable. For the GooglePayResult object in this PR, we only ever return Exception objects (stack trace included where possible), so I'm not sure there is a benefit to making it the more generic Throwable.

We should keep it as Exception yeah, Exception extends Throwable so we shouldn't have to change any types. There may not always be a cause too, and that's fine cause == null in that case.

If we do have an exception that signifies the root cause, then a merchant can inspect braintreeException?.cause to give us more info. Also, the toString() implementation for Exception prints out the combined stack trace of the exception chain.

new BraintreeException("Error with Config", ioExceptionCausedByConfigFetching) as an example would allow us to chain the exception returned to the merchant by the SDK to the original root cause exception, like in this example the user's WiFi could be disabled.

Ok actually I think the cause wording here is confusing - we aren't returning the reason why the user isn't ready to pay (we are just returning NotReadyToPay in those cases). We are only returning an error if there was an error determining if the user was ready to pay (in which case it's not really the cause of why they aren't ready to pay, it's that we can't determine if they are ready to pay or not). I'm going to put it back to error but I still have a ticket to investigate using cause/throwables

Let's also document this design decision to include cause where we think it's applicable, and some loose guidelines around what "applicable" means somewhere internally.

I can see one of us in the future taking a look at the code and thinking "now why did we do this here but that there?", or even an external dev wondering why we did it 😄

I put this back to error, which aligns with all other Failure results because I think it is more descriptive of what we are actually returning - so no additional design decision doc needed 👍

Yeah cause is mostly useful for acquiring stack traces from merchants when possible.

hollabaq86

Great work on some meaningful cleanup. Got a few Qs and things I'd like to confirm before we merge

hollabaq86 · 2023-12-14T21:29:05Z

Demo/src/main/java/com/braintreepayments/demo/GooglePayFragment.java

+            if (paymentAuthRequest instanceof GooglePayPaymentAuthRequest.ReadyToLaunch) {
+                googlePayLauncher.launch(
+                        ((GooglePayPaymentAuthRequest.ReadyToLaunch) paymentAuthRequest).getRequestParams());
+            } else if (paymentAuthRequest instanceof GooglePayPaymentAuthRequest.Failure) {


are there any other possible values for GooglePayPaymentAuthRequest? Curious if we need a fallback for scenarios we don't catch.

These are the only options - the benefit of defining GooglePayPaymentAuthRequest as a sealed class is that it is exhaustive, and when we migrate this file to Kotlin, we can use the when switch statement without a default/fallback scenario.

Curious if we need a fallback for scenarios we don't catch.

This is one thing that keeps me up at night lol. Adding a new case to an existing enum can totally break a merchant app. The tradeoff is reduced compiler enforcement in exchange for not breaking a merchant integration.

I do think it's worth calling out in our docs to encourage merchants to catch "UNKNOWN" results so that their integrations are forward compatible.

hollabaq86 · 2023-12-14T21:29:37Z

Demo/src/main/java/com/braintreepayments/demo/GooglePayFragment.java

+                                handleError(((GooglePayResult.Failure) googlePayResult).getError());
+                            } else if (googlePayResult instanceof GooglePayResult.Success){
+                                handleGooglePayActivityResult(((GooglePayResult.Success) googlePayResult).getNonce());
+                            } else if (googlePayResult instanceof GooglePayResult.Cancel) {


should we add an else block for other values, or are these the only options from google?

We define the GooglePayResult object as a sealed class so these are the only options.

GooglePay/src/main/java/com/braintreepayments/api/GooglePayClient.java

hollabaq86 · 2023-12-14T21:32:06Z

GooglePay/src/main/java/com/braintreepayments/api/GooglePayClient.java

-                            "manifest, or did not have a theme of R.style.bt_transparent_activity"));
+            callback.onGooglePayPaymentAuthRequest(new GooglePayPaymentAuthRequest.Failure(new BraintreeException(
+                    "GooglePayActivity was not found in the Android " +
+                            "manifest, or did not have a theme of R.style.bt_transparent_activity")));


this is oddly specific heh. No action, just commenting

Yeah... it is getting pretty verbose open to alternative callback naming! The previous pattern was always naming it onResult which is a little confusing when the result we are returning is actually a request object

GooglePay/src/main/java/com/braintreepayments/api/GooglePayClient.java

hollabaq86 · 2023-12-14T21:37:56Z

GooglePay/src/main/java/com/braintreepayments/api/GooglePayInternalClient.java

-                    callback.onResult(false, e);
+        paymentsClient.isReadyToPay(isReadyToPayRequest).addOnCompleteListener(task -> {
+            try {
+                Boolean isReady = task.getResult(ApiException.class);


Maybe this is some android syntax nuance that I'm missing, but this currently reads like:

"isReady = is there an API Exception?"
if yes, provide readiness result (which can be a failure?) <-- can I get a confirmation of this expectation?
otherwise not ready

This is saying isReady = the result of the task to fetch readiness status (which will be true or false if successfully fetched), but if there is an error executing this task, then throw an APIException.

randomtwocents: The Google Pay API is legit weird IMO. Merchant wins when we wrap! 🌯

hollabaq86 · 2023-12-14T21:44:36Z

GooglePay/src/main/java/com/braintreepayments/api/GooglePayReadinessResult.kt

+     * The Google Pay API is supported or not set up on this device, or there was an [error]
+     * determining readiness.
+     */
+    class NotReadyToPay(val error: Exception?) : GooglePayReadinessResult()


Let's also document this design decision to include cause where we think it's applicable, and some loose guidelines around what "applicable" means somewhere internally.

I can see one of us in the future taking a look at the code and thinking "now why did we do this here but that there?", or even an external dev wondering why we did it 😄

…ent.java Co-authored-by: Holly Richko <[email protected]>

…ee/braintree_android into google_pay_single_result

sshropshire · 2023-12-18T16:37:56Z

Let's also document this design decision to include cause where we think it's applicable, and some loose guidelines around what "applicable" means somewhere internally.

It's mostly objective, the standard Exception type in Java takes a cause property in its constructor, and BraintreeException extends Exception. If the exception we're reporting to the merchant has a root cause e.g. CardinalException, BrowserSwitchException, then we can still provide our generic BraintreeException message to the merchant like 3D Secure verification failed., and then error.cause == CardinalException.

Also BraintreeException.printStackTrace() will include the OG cardinal exception as well, and we can take that to them to help diagnose the cause of an inbound.

v5_MIGRATION_GUIDE.md

Co-authored-by: sshropshire <[email protected]>

sarahkoop added 14 commits December 5, 2023 13:27

Add GooglePayResult

c8f27df

Implement GooglePayResult

353524b

Fix unit tests for tokenize

832c75e

Add GooglePayIsReadyToPayResult

d9bc436

Implement ready to pay result

fdda053

Fix demo

912cb3f

Rename payment auth request

20827d0

Add GooglePayPaymentAuthRequest

3101608

Implement GooglePayPaymentAuthRequest result

ac1c9d2

Fix unit tests

a4c56fe

Fix demo app

9746f27

Fix demo integration

c94af04

Update CHANGELOG and migration guide

a52e641

Add ready to pay to migration guide

45994f0

sarahkoop requested a review from a team as a code owner December 5, 2023 21:21

sarahkoop commented Dec 5, 2023

View reviewed changes

BraintreeCore/schemas/com.braintreepayments.api.AnalyticsDatabase/1.json Outdated Show resolved Hide resolved

sarahkoop commented Dec 5, 2023

View reviewed changes

GooglePay/src/main/java/com/braintreepayments/api/GooglePayReadinessResult.kt Show resolved Hide resolved

sarahkoop commented Dec 5, 2023

View reviewed changes

GooglePay/src/main/java/com/braintreepayments/api/GooglePayReadinessResult.kt Show resolved Hide resolved

Merge branch 'v5' into google_pay_single_result

9a1699c

sshropshire reviewed Dec 7, 2023

View reviewed changes

GooglePay/src/main/java/com/braintreepayments/api/GooglePayClient.java Show resolved Hide resolved

Update GooglePay/src/main/java/com/braintreepayments/api/GooglePayCli…

110ba31

…ent.java Co-authored-by: sshropshire <[email protected]>

sshropshire reviewed Dec 7, 2023

View reviewed changes

sarahkoop added 2 commits December 7, 2023 14:53

Convert error to cause

f5e5c19

Revert error to cause changes

a5525b6

jaxdesmarais approved these changes Dec 13, 2023

View reviewed changes

hollabaq86 reviewed Dec 14, 2023

View reviewed changes

sarahkoop and others added 4 commits December 14, 2023 16:12

Update GooglePay/src/main/java/com/braintreepayments/api/GooglePayCli…

8b834c0

…ent.java Co-authored-by: Holly Richko <[email protected]>

Update GooglePay/src/main/java/com/braintreepayments/api/GooglePayCli…

ab9e6f2

…ent.java Co-authored-by: Holly Richko <[email protected]>

Fix spacing

179f897

Merge branch 'google_pay_single_result' of https://github.com/braintr…

db4037c

…ee/braintree_android into google_pay_single_result

sshropshire reviewed Dec 18, 2023

View reviewed changes

v5_MIGRATION_GUIDE.md Outdated Show resolved Hide resolved

sshropshire reviewed Dec 18, 2023

View reviewed changes

v5_MIGRATION_GUIDE.md Outdated Show resolved Hide resolved

sshropshire approved these changes Dec 18, 2023

View reviewed changes

sarahkoop and others added 3 commits December 18, 2023 11:38

Update v5_MIGRATION_GUIDE.md

77e1a97

Co-authored-by: sshropshire <[email protected]>

Update v5_MIGRATION_GUIDE.md

7f8e8af

Co-authored-by: sshropshire <[email protected]>

Merge branch 'v5' into google_pay_single_result

397a941

sarahkoop merged commit 54f7f5d into v5 Dec 18, 2023
2 checks passed

sarahkoop deleted the google_pay_single_result branch December 18, 2023 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Google pay single result #842

Google pay single result #842

sarahkoop commented Dec 5, 2023

sshropshire Dec 7, 2023

sshropshire Dec 7, 2023

sarahkoop Dec 7, 2023

sshropshire Dec 7, 2023

sarahkoop Dec 11, 2023

sshropshire Dec 12, 2023

sarahkoop Dec 12, 2023

hollabaq86 Dec 14, 2023

sarahkoop Dec 14, 2023

sshropshire Dec 18, 2023

hollabaq86 left a comment

hollabaq86 Dec 14, 2023

sarahkoop Dec 14, 2023

sshropshire Dec 18, 2023

hollabaq86 Dec 14, 2023

sarahkoop Dec 14, 2023

hollabaq86 Dec 14, 2023

sarahkoop Dec 14, 2023

hollabaq86 Dec 14, 2023

sarahkoop Dec 14, 2023

sshropshire Dec 18, 2023

hollabaq86 Dec 14, 2023

sshropshire commented Dec 18, 2023

	class NotReadyToPay(val error: Exception?) : GooglePayReadinessResult()
	class NotReadyToPay(val cause: Exception?) : GooglePayReadinessResult()

Google pay single result #842

Google pay single result #842

Conversation

sarahkoop commented Dec 5, 2023

Summary of changes

Checklist

Authors

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hollabaq86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sshropshire commented Dec 18, 2023