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

Instantiate BraintreeClient Within Clients #830

Merged
merged 16 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import android.net.Uri;

import androidx.annotation.NonNull;
import androidx.annotation.VisibleForTesting;

import org.json.JSONException;

Expand All @@ -16,7 +17,15 @@ public class AmericanExpressClient {

private final BraintreeClient braintreeClient;

public AmericanExpressClient(@NonNull BraintreeClient braintreeClient) {
/**
* Initializes a new {@link AmericanExpressClient} instance
* @param clientParams configurable {@link ClientParams}
*/
public AmericanExpressClient(@NonNull ClientParams clientParams) {
this.braintreeClient = new BraintreeClient(clientParams);
}

@VisibleForTesting AmericanExpressClient(@NonNull BraintreeClient braintreeClient) {
this.braintreeClient = braintreeClient;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import com.braintreepayments.api.IntegrationType.Integration
* Core Braintree class that handles network requests.
*/
@Suppress("LargeClass", "LongParameterList", "TooManyFunctions")
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
open class BraintreeClient @VisibleForTesting internal constructor(
Comment on lines +16 to 17
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change impact DropIn at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


/**
Expand Down Expand Up @@ -63,6 +64,13 @@ open class BraintreeClient @VisibleForTesting internal constructor(
braintreeDeepLinkReturnUrlScheme = params.braintreeReturnUrlScheme
)

/**
* @suppress
*/
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
constructor(clientParams: ClientParams) :
this(clientParams.context, clientParams.authorization, clientParams.returnUrlScheme)

/**
* @suppress
*/
Expand All @@ -76,6 +84,7 @@ open class BraintreeClient @VisibleForTesting internal constructor(
* @param authorization The tokenization key or client token to use. If an invalid authorization
* is provided, a [BraintreeException] will be returned via callback.
*/
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
constructor(context: Context, authorization: String) :
this(BraintreeOptions(context = context, initialAuthString = authorization))

Expand All @@ -86,6 +95,7 @@ open class BraintreeClient @VisibleForTesting internal constructor(
* @param clientTokenProvider An implementation of [ClientTokenProvider] that [BraintreeClient]
* will use to fetch a client token on demand.
*/
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
constructor(context: Context, clientTokenProvider: ClientTokenProvider) :
this(BraintreeOptions(context = context, clientTokenProvider = clientTokenProvider))

Expand All @@ -104,7 +114,8 @@ open class BraintreeClient @VisibleForTesting internal constructor(
* authorization is provided, a [BraintreeException] will be returned via callback.
* @param returnUrlScheme A custom return url to use for browser and app switching
*/
constructor (context: Context, authorization: String, returnUrlScheme: String) : this(
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
constructor (context: Context, authorization: String, returnUrlScheme: String?) : this(
BraintreeOptions(
context = context,
initialAuthString = authorization,
Expand All @@ -127,6 +138,7 @@ open class BraintreeClient @VisibleForTesting internal constructor(
* will use to fetch a client token on demand.
* @param returnUrlScheme A custom return url to use for browser and app switching
*/
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
constructor(
context: Context,
clientTokenProvider: ClientTokenProvider,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.braintreepayments.api

import android.content.Context

data class ClientParams(val context: Context,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

val authorization: String,
val returnUrlScheme: String?) {

constructor(context: Context, authorization: String) : this(context, authorization, null)
}
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
* All Modules
* Bump `minSdkVersion` to API 23
* Bump target Java version to Java 11
* Remove `BraintreeClient` public constructors
* Add `ClientParams` for instantiating payment method clients
* UnionPay
* Remove `union-pay` module
* UnionPay cards can now be processed as regular cards (through the `card` module) due to their partnership with Discover
Expand Down
11 changes: 10 additions & 1 deletion Card/src/main/java/com/braintreepayments/api/CardClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,16 @@ public class CardClient {
private final BraintreeClient braintreeClient;
private final ApiClient apiClient;

public CardClient(@NonNull BraintreeClient braintreeClient) {
/**
* Initializes a new {@link CardClient} instance
* @param clientParams configurable {@link ClientParams}
*/
public CardClient(@NonNull ClientParams clientParams) {
this(new BraintreeClient(clientParams));
}

@VisibleForTesting
CardClient(@NonNull BraintreeClient braintreeClient) {
this(braintreeClient, new ApiClient(braintreeClient));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,16 @@ public class DataCollector {
private final UUIDHelper uuidHelper;
private final BraintreeClient braintreeClient;

public DataCollector(@NonNull BraintreeClient braintreeClient) {
/**
* Initializes a new {@link DataCollector} instance
* @param clientParams configurable {@link ClientParams}
*/
public DataCollector(@NonNull ClientParams clientParams) {
this(new BraintreeClient(clientParams));
}

@VisibleForTesting
DataCollector(@NonNull BraintreeClient braintreeClient) {
this(braintreeClient, new MagnesInternalClient(), new UUIDHelper());
}

Expand Down
13 changes: 13 additions & 0 deletions Demo/src/main/java/com/braintreepayments/demo/BaseFragment.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import com.braintreepayments.api.BraintreeClient;
import com.braintreepayments.api.PaymentMethodNonce;

import java.util.Objects;

public abstract class BaseFragment extends Fragment {

@CallSuper
Expand Down Expand Up @@ -54,4 +56,15 @@ protected BraintreeClient getBraintreeClient() {
}
return null;
}

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

String getAuthStringArg() {
return Objects.requireNonNull(requireArguments().getString("authString"));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.braintreepayments.demo;

import androidx.annotation.NonNull;

import com.braintreepayments.api.Authorization;
import com.braintreepayments.api.BraintreeClient;

public interface BraintreeAuthorizationCallback {
void onResult(@NonNull String authString);
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,5 @@
// TODO: remove when AuthorizationProvider is released
public class BraintreeClientFactory {

static public BraintreeClient createBraintreeClientWithAuthorizationProvider(Context context) {
return new BraintreeClient(context, new DemoClientTokenProvider(context));
}

}
45 changes: 20 additions & 25 deletions Demo/src/main/java/com/braintreepayments/demo/CardFragment.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.braintreepayments.api.Card;
import com.braintreepayments.api.CardClient;
import com.braintreepayments.api.CardNonce;
import com.braintreepayments.api.ClientParams;
import com.braintreepayments.api.DataCollector;
import com.braintreepayments.api.PaymentMethodNonce;
import com.braintreepayments.api.ThreeDSecureAdditionalInformation;
Expand Down Expand Up @@ -71,12 +72,12 @@ public class CardFragment extends BaseFragment implements OnCardFormSubmitListen
public void onCreate(Bundle onSaveInstanceState) {
super.onCreate(onSaveInstanceState);

BraintreeClient braintreeClient = getBraintreeClient();
americanExpressClient = new AmericanExpressClient(braintreeClient);
cardClient = new CardClient(braintreeClient);
threeDSecureClient = new ThreeDSecureClient(braintreeClient);
ClientParams clientParams = new ClientParams(requireContext(), super.getAuthStringArg());
americanExpressClient = new AmericanExpressClient(clientParams);
cardClient = new CardClient(clientParams);
threeDSecureClient = new ThreeDSecureClient(clientParams);

dataCollector = new DataCollector(braintreeClient);
dataCollector = new DataCollector(clientParams);

if (onSaveInstanceState != null) {
threeDSecureRequested = onSaveInstanceState.getBoolean(EXTRA_THREE_D_SECURE_REQUESTED);
Expand Down Expand Up @@ -126,26 +127,20 @@ public void onSaveInstanceState(Bundle outState) {

private void configureCardForm() {
final AppCompatActivity activity = (AppCompatActivity) getActivity();
BraintreeClient braintreeClient = getBraintreeClient();

braintreeClient.getConfiguration((configuration, configError) -> {
if (configuration != null) {
cardForm.cardRequired(true)
.expirationRequired(true)
.cvvRequired(configuration.isCvvChallengePresent())
.postalCodeRequired(configuration.isPostalCodeChallengePresent())
.mobileNumberRequired(false)
.actionLabel(cardFormActionLabel)
.setup(activity);

if (getArguments().getBoolean(MainFragment.EXTRA_COLLECT_DEVICE_DATA, false)) {
dataCollector.collectDeviceData(activity,
(deviceData, e) -> this.deviceData = deviceData);
}
} else {
handleError(configError);
}
});

// TODO: Configure card form via settings
cardForm.cardRequired(true)
.expirationRequired(true)
.cvvRequired(true)
.postalCodeRequired(true)
.mobileNumberRequired(false)
.actionLabel(cardFormActionLabel)
.setup(activity);

if (getArguments().getBoolean(MainFragment.EXTRA_COLLECT_DEVICE_DATA, false)) {
dataCollector.collectDeviceData(activity,
(deviceData, e) -> this.deviceData = deviceData);
}
}

@Override
Expand Down
19 changes: 9 additions & 10 deletions Demo/src/main/java/com/braintreepayments/demo/DemoActivity.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import android.view.Window;
import android.widget.ArrayAdapter;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.appcompat.app.ActionBar;
import androidx.appcompat.app.AppCompatActivity;
Expand All @@ -30,13 +31,16 @@ public class DemoActivity extends AppCompatActivity implements ActivityCompat.On
private BraintreeClient braintreeClient;
private AppBarConfiguration appBarConfiguration;

private DemoClientTokenProvider clientTokenProvider;

private SharedPreferences.OnSharedPreferenceChangeListener sharedPreferenceChangeListener;

@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
requestWindowFeature(Window.FEATURE_INDETERMINATE_PROGRESS);
setContentView(R.layout.activity_demo);
clientTokenProvider = new DemoClientTokenProvider(this);

setupActionBar();
setProgressBarIndeterminateVisibility(true);
Expand All @@ -46,16 +50,11 @@ protected void onCreate(Bundle savedInstanceState) {

public BraintreeClient getBraintreeClient() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - removed

// lazily instantiate braintree client in case the demo has been reset
if (braintreeClient == null) {
if (Settings.useTokenizationKey(this)) {
String tokenizationKey = Settings.getTokenizationKey(this);
braintreeClient = new BraintreeClient(this, tokenizationKey);
} else {
braintreeClient =
BraintreeClientFactory.createBraintreeClientWithAuthorizationProvider(this);
}
}
return braintreeClient;
return null;
}

public void fetchAuthorization(BraintreeAuthorizationCallback callback) {
clientTokenProvider.getClientToken(callback);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import com.braintreepayments.demo.R;
import com.braintreepayments.demo.Settings;

public class DemoClientTokenProvider implements ClientTokenProvider {
public class DemoClientTokenProvider {

private final Merchant merchant;
private final Context appContext;
Expand All @@ -21,15 +21,14 @@ public DemoClientTokenProvider(Context context) {
appContext = context.getApplicationContext();
}

@Override
public void getClientToken(@NonNull ClientTokenCallback callback) {
public void getClientToken(@NonNull BraintreeAuthorizationCallback callback) {
String authType = Settings.getAuthorizationType(appContext);
if (authType.equals(getString(appContext, R.string.client_token))) {
merchant.fetchClientToken(appContext, (clientToken, error) -> {
if (clientToken != null) {
callback.onSuccess(clientToken);
callback.onResult(clientToken);
} else if (error != null) {
callback.onFailure(error);
callback.onResult(null);
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import androidx.navigation.fragment.NavHostFragment;

import com.braintreepayments.api.BraintreeClient;
import com.braintreepayments.api.ClientParams;
import com.braintreepayments.api.GooglePayCapabilities;
import com.braintreepayments.api.GooglePayClient;
import com.braintreepayments.api.GooglePayLauncher;
Expand All @@ -22,10 +23,11 @@
import com.google.android.gms.wallet.TransactionInfo;
import com.google.android.gms.wallet.WalletConstants;

import java.util.Objects;

public class GooglePayFragment extends BaseFragment {

private ImageButton googlePayButton;
private BraintreeClient braintreeClient;
private GooglePayClient googlePayClient;
private GooglePayLauncher googlePayLauncher;

Expand All @@ -38,8 +40,7 @@ public View onCreateView(@NonNull LayoutInflater inflater,
googlePayButton = view.findViewById(R.id.google_pay_button);
googlePayButton.setOnClickListener(this::launchGooglePay);

braintreeClient = getBraintreeClient();
googlePayClient = new GooglePayClient(braintreeClient);
googlePayClient = new GooglePayClient(new ClientParams(requireContext(), super.getAuthStringArg()));
googlePayLauncher = new GooglePayLauncher(this,
paymentAuthResult -> googlePayClient.tokenize(paymentAuthResult,
(paymentMethodNonce, error) -> {
Expand All @@ -57,27 +58,13 @@ public View onCreateView(@NonNull LayoutInflater inflater,
public void onResume() {
super.onResume();

braintreeClient.getConfiguration((configuration, error) -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (configuration == null) {
return;
}

if (GooglePayCapabilities.isGooglePayEnabled(requireActivity(), configuration)) {

googlePayClient.isReadyToPay(requireActivity(), (isReadyToPay, e) -> {
if (isReadyToPay) {
googlePayButton.setVisibility(View.VISIBLE);
} else {
showDialog(
"Google Payments are not available. The following issues could be the cause:\n\n" +
"No user is logged in to the device.\n\n" +
"Google Play Services is missing or out of date.");
}
});
googlePayClient.isReadyToPay(requireActivity(), (isReadyToPay, e) -> {
if (isReadyToPay) {
googlePayButton.setVisibility(View.VISIBLE);
} else {
showDialog(
"Google Payments are not available. The following issues could be the cause:\n\n" +
"Google Payments are not enabled for the current merchant.\n\n" +
"No user is logged in to the device.\n\n" +
"Google Play Services is missing or out of date.");
}
});
Expand Down
Loading