Instantiate BraintreeClient Within Clients #830

sarahkoop · 2023-11-16T20:50:04Z

Summary of changes

Remove BraintreeClient public constructors and instead instantiate BraintreeClient under the hood within each payment method client
Add ClientParams object to be configurable with parameters required to instantiate clients (and BraintreeClient)
Update client public constructors to take ClientParams

Checklist

Added a changelog entry

Authors

@sarahkoop

sarahkoop · 2023-11-16T20:52:16Z

Open question for this PR: Is this cleaner/preferred over having merchants instantiate BraintreeClient? BraintreeClient feels a little bit like exposing unnecessary internals to the merchant (and that was feedback we received), but constructing a ClientParams object is essentially the same in code as instantiating a BraintreeClient, so this could be unnecessary migration lift for merchants.

scannillo · 2023-11-20T16:04:05Z

BraintreeCore/src/main/java/com/braintreepayments/api/BraintreeClient.kt

+@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
 open class BraintreeClient @VisibleForTesting internal constructor(


Does this change impact DropIn at all?

It shouldn't - drop-in uses the BraintreeClient constructor with BraintreeOptions param, and has the same "com.braintreepayments.api" group ID as the core libraries, so they all can share these "package-private"/library group methods.

scannillo · 2023-11-20T16:07:59Z

BraintreeCore/src/main/java/com/braintreepayments/api/ClientParams.kt

+
+import android.content.Context
+
+data class ClientParams(val context: Context,


IMO it would be more clear to require the <Feature>Client constructors to require each param individually, versus encapsulating them all into this additional ClientParams class. Also that lets the feature clients that actually need the returnURL to enforce it's requirement

Yeah ClientParams is more tech-debt than API. It's internal in v4, it can be removed in v5.

On second thought, the parameter object pattern here can be useful. We created too many convenience initializers in v4 to support multiple integration patterns. With v5 we do get to wipe the slate clean and have a single constructor, but theoretically a param object like ClientParams can help to keep the constructor count low as we add new features.

Agreed that just plain auth and context as direct constructor parameters is definitely the cleanest - but yeah in v4 we ended up with too many overloads I wanted to avoid that with ClientParams. Return URL isn't a required param for any clients, it's optional for some. But hopefully we won't add a bunch of params to client constructors in this version, so maybe we are safe to just go with auth and context as the main constructor and one overload for the clients that can accept return URL. What do you all think?

Yeah there's definitely a middle ground. Thinking on it more, our request objects can also be considered parameter objects. It may make sense to move properties like returnUrl from ClientParams to <FEATURE>Request. That will cause some redundancy, but it also allows us to make returnUrl required (or optional to allow merchant overrides) on a per-feature basis.

Good idea I think that's a good compromise here - I'll go with that

v5_MIGRATION_GUIDE.md

scannillo · 2023-11-20T16:12:19Z

v5_MIGRATION_GUIDE.md

-        braintreClient = BraintreeClient(context, "TOKENIZATION_KEY_OR_CLIENT_TOKEN")
+-       braintreClient = BraintreeClient(context, "TOKENIZATION_KEY_OR_CLIENT_TOKEN")
 -       localPaymentClient = LocalPaymentClient(this, braintreeClient)
-+       localPaymentClient = LocalPaymentClient(braintreeClient)
+       localPaymentClient = LocalPaymentClient(ClientParams(context, "TOKENIZATION_KEY_OR_CLIENT_TOKEN")
 -       localPaymentClient.setListener(this)


I like this! It's a change we can make on iOS too (in a major version), dropping the merchant step to instantiate a BTAPIClient. It always felt redundant to me to have merchants instantiate that class when there was no public functionality on it they would ever use.

It'd be great to coordinate this major version alongside the package access modifier in Swift 5.9+ (proposal here) as a whole. But totally agree having merchant just access the client like this would be dope:

// iOS v6 let apiClient = BTAPIClient(authorization: "AUTHORIZATION") let cardClient = BTCardClient(apiClient: apiClient) // iOS v7 let cardClient = BTCardClient(authorization: "AUTHORIZATION")

Added this to our v7 doc!

OMG if package is approved that will benefit us in so many ways! 🤞

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

sarahkoop · 2023-11-27T21:36:33Z

Updated this PR to remove the ClientParams object and just construct each payment method client with a context and authorization string directly (which will hopefully align with an iOS v7 approach).

For the overload parameters (currently just browser switching params like returnUrlScheme), the plan will be to add those parameters to the payment method request objects themselves (ex: PayPalRequest.setReturnUrlScheme()). This is a simpler integration path for merchants as those request objects are already configurable, and it removes the need to make payment method specific parameters available to all clients. There is another ticket to handle moving those browser switch BraintreeClient params to the payment method specific request objects.

sshropshire · 2023-11-28T16:13:42Z

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

@@ -46,16 +50,11 @@ protected void onCreate(Bundle savedInstanceState) {

    public BraintreeClient getBraintreeClient() {


We should be able to remove the getBrainreeClient() eventually. If it can be removed in this PR that would be a nice win. Generally the less DemoActivity does the better.

Good call - removed

scannillo · 2023-11-28T22:06:20Z

v5_MIGRATION_GUIDE.md

-+       venmoClient = VenmoClient(braintreeClient)
+       venmoClient = VenmoClient(context, "TOKENIZATION_KEY_OR_CLIENT_TOKEN")


👏 So clean!

scannillo · 2023-11-28T22:22:42Z

SEPADirectDebit/src/main/java/com/braintreepayments/api/SEPADirectDebitClient.java

+    @VisibleForTesting
+    SEPADirectDebitClient(@NonNull BraintreeClient braintreeClient) {


Are we planning to remove these in a follow-up? Or are we keeping for ease of unit testing?

For now they are nice to keep for unit testing since we pass in a MockBraintreeClient with responses to various API calls

scannillo · 2023-11-28T22:29:23Z

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

@@ -57,27 +53,13 @@ public View onCreateView(@NonNull LayoutInflater inflater,
    public void onResume() {
        super.onResume();

-        braintreeClient.getConfiguration((configuration, error) -> {


Curious - why were we doing config fetches in the demo app?

Yeah we probably could've deleted this a while back. This has roots in v3. I remember removing it from the Core SDK since it would require a runtime dependency on the Google Pay SDK, but we probably kept it in when we went to GA because we needed to get a release out quickly.

scannillo

A few small questions, but LGTM!

sshropshire · 2023-11-30T16:58:42Z

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

+    protected void fetchAuthorization(BraintreeAuthorizationCallback callback) {
        DemoActivity demoActivity = getDemoActivity();
        if (demoActivity != null) {
-            return demoActivity.getBraintreeClient();
+            demoActivity.fetchAuthorization(callback);
        }
-        return null;
+    }


2cents: I always felt like phasing out BaseFragment would be the best call. We should be able to fetch client tokens within each feature fragment. Do you feel like it makes sense to remove eventually?

Agreed - will look into that in another demo refactor PR

sarahkoop added 8 commits November 14, 2023 09:54

Refactor demo app for auth fetching

d8ed6a6

Pass auth string to more fragments

39f6559

Make braintreeclient internal

f3409c0

Fix visa checkout

e335e03

Update migration guide

9bb9dfe

Add public doc strings

72c6f29

Fix constructor

3254786

update migration guide and CHANGELOG

edc0aee

sarahkoop requested a review from a team as a code owner November 16, 2023 20:50

sarahkoop mentioned this pull request Nov 17, 2023

Remove ClientTokenProvider #831

Merged

1 task

scannillo reviewed Nov 20, 2023

View reviewed changes

v5_MIGRATION_GUIDE.md Outdated Show resolved Hide resolved

scannillo reviewed Nov 20, 2023

View reviewed changes

sarahkoop and others added 5 commits November 27, 2023 10:55

Update v5_MIGRATION_GUIDE.md

17367a7

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

Merge v5

10660d2

Remove ClientParams

20479e8

Update doc strings

0970691

Update migration guide and CHANGELOG

78c3c94

sshropshire reviewed Nov 28, 2023

View reviewed changes

Remove getBraintreeClient from demo

d5670aa

scannillo reviewed Nov 28, 2023

View reviewed changes

scannillo approved these changes Nov 28, 2023

View reviewed changes

Merge branch 'v5' into instantiate_braintree_client

d997cec

sarahkoop mentioned this pull request Nov 29, 2023

Send analytics to FPTI #834

Merged

1 task

sshropshire approved these changes Nov 30, 2023

View reviewed changes

sshropshire reviewed Nov 30, 2023

View reviewed changes

Merge branch 'v5' into instantiate_braintree_client

e6e5a34

sarahkoop merged commit 347905c into v5 Nov 30, 2023
2 checks passed

sarahkoop deleted the instantiate_braintree_client branch November 30, 2023 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instantiate BraintreeClient Within Clients #830

Instantiate BraintreeClient Within Clients #830

sarahkoop commented Nov 16, 2023

sarahkoop commented Nov 16, 2023

scannillo Nov 20, 2023

sarahkoop Nov 29, 2023

scannillo Nov 20, 2023

sshropshire Nov 20, 2023

sshropshire Nov 27, 2023

sarahkoop Nov 27, 2023

sshropshire Nov 27, 2023

sarahkoop Nov 27, 2023

scannillo Nov 20, 2023

jaxdesmarais Nov 21, 2023 •

edited

Loading

scannillo Nov 27, 2023

sarahkoop commented Nov 27, 2023

sshropshire Nov 28, 2023

sarahkoop Nov 28, 2023

scannillo Nov 28, 2023

scannillo Nov 28, 2023

sarahkoop Nov 29, 2023

scannillo Nov 28, 2023

sshropshire Nov 28, 2023

scannillo left a comment

sshropshire Nov 30, 2023

sarahkoop Nov 30, 2023

		@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
		open class BraintreeClient @VisibleForTesting internal constructor(


		import android.content.Context

		data class ClientParams(val context: Context,

		@@ -46,16 +50,11 @@ protected void onCreate(Bundle savedInstanceState) {

		public BraintreeClient getBraintreeClient() {

		+ venmoClient = VenmoClient(braintreeClient)
		+ venmoClient = VenmoClient(context, "TOKENIZATION_KEY_OR_CLIENT_TOKEN")

		@VisibleForTesting
		SEPADirectDebitClient(@NonNull BraintreeClient braintreeClient) {

Instantiate BraintreeClient Within Clients #830

Instantiate BraintreeClient Within Clients #830

Conversation

sarahkoop commented Nov 16, 2023

Summary of changes

Checklist

Authors

sarahkoop commented Nov 16, 2023

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

jaxdesmarais Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahkoop commented Nov 27, 2023

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

scannillo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaxdesmarais Nov 21, 2023 •

edited

Loading