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.

Sign up for GitHub

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

VenmoListener NPE Fix #848

Merged
merged 8 commits into from
Dec 11, 2023
Merged

VenmoListener NPE Fix #848

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a4f8056
Check for listener before returning result
sarahkoop Dec 11, 2023
a7d8117
Add unit test
sarahkoop Dec 11, 2023
2def0c6
Add CHANGELOG
sarahkoop Dec 11, 2023
0721b43
Fix spacing
sarahkoop Dec 11, 2023
c457018
Log warning if listener is null
sarahkoop Dec 11, 2023
dc02b8e
Move logging to utils class
sarahkoop Dec 11, 2023
475ff15
Update tag name
sarahkoop Dec 11, 2023
033bfb8
Fix lint
sarahkoop Dec 11, 2023
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
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Braintree Android SDK Release Notes

## unreleased

* Venmo
* Fix NPE when `VenmoListener` is null (fixes #832)

## 4.40.0 (2023-11-16)

* PayPalNativeCheckout
Expand Down
39 changes: 29 additions & 10 deletions Venmo/src/main/java/com/braintreepayments/api/VenmoClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import android.content.Intent;
import android.net.Uri;
import android.text.TextUtils;
import android.util.Log;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
Expand Down Expand Up @@ -33,6 +34,8 @@ public class VenmoClient {
static final String EXTRA_USERNAME = "com.braintreepayments.api.EXTRA_USER_NAME";
static final String EXTRA_RESOURCE_ID = "com.braintreepayments.api.EXTRA_RESOURCE_ID";

static final String LOGGING_TAG = "Braintree";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static final String LOGGING_TAG = "Braintree";
static final String LOGGING_TAG = "Braintree SDK";

nit: take or leave. Also I could see us moving this constant to a Core file one day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update and moved to a LoggingUtils class 👍


private final BraintreeClient braintreeClient;
private final VenmoApi venmoApi;
private final VenmoSharedPrefsWriter sharedPrefsWriter;
Expand Down Expand Up @@ -132,7 +135,7 @@ public void tokenizeVenmoAccount(@NonNull final FragmentActivity activity, @NonN
@Override
public void onResult(@Nullable Exception error) {
if (error != null) {
listener.onVenmoFailure(error);
deliverVenmoFailure(error);
}
}
});
Expand Down Expand Up @@ -254,19 +257,19 @@ public void onResult(@Nullable VenmoAccountNonce nonce, @Nullable Exception erro
@Override
public void onResult(@Nullable VenmoAccountNonce venmoAccountNonce, @Nullable Exception error) {
if (venmoAccountNonce != null) {
listener.onVenmoSuccess(venmoAccountNonce);
deliverVenmoSuccess(venmoAccountNonce);
} else if (error != null) {
listener.onVenmoFailure(error);
deliverVenmoFailure(error);
}
}
});
} else {
braintreeClient.sendAnalyticsEvent("pay-with-venmo.app-switch.failure");
listener.onVenmoSuccess(nonce);
deliverVenmoSuccess(nonce);
}
} else {
braintreeClient.sendAnalyticsEvent("pay-with-venmo.app-switch.failure");
listener.onVenmoFailure(error);
deliverVenmoFailure(error);
}
}
});
Expand All @@ -279,21 +282,21 @@ public void onResult(@Nullable VenmoAccountNonce venmoAccountNonce, @Nullable Ex
@Override
public void onResult(@Nullable VenmoAccountNonce venmoAccountNonce, @Nullable Exception error) {
if (venmoAccountNonce != null) {
listener.onVenmoSuccess(venmoAccountNonce);
deliverVenmoSuccess(venmoAccountNonce);
} else if (error != null) {
listener.onVenmoFailure(error);
deliverVenmoFailure(error);
}
}
});
} else {
String venmoUsername = venmoResult.getVenmoUsername();
VenmoAccountNonce venmoAccountNonce = new VenmoAccountNonce(nonce, venmoUsername, false);
listener.onVenmoSuccess(venmoAccountNonce);
deliverVenmoSuccess(venmoAccountNonce);
}

}
} else if (authError != null) {
listener.onVenmoFailure(authError);
deliverVenmoFailure(authError);
}
}
});
Expand All @@ -302,7 +305,23 @@ public void onResult(@Nullable VenmoAccountNonce venmoAccountNonce, @Nullable Ex
if (venmoResult.getError() instanceof UserCanceledException) {
braintreeClient.sendAnalyticsEvent("pay-with-venmo.app-switch.canceled");
}
listener.onVenmoFailure(venmoResult.getError());
deliverVenmoFailure(venmoResult.getError());
}
}

private void deliverVenmoSuccess(VenmoAccountNonce venmoAccountNonce) {
if (listener != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to handle if the listener is null in either of these cases? Not sure of the best way to do that or even if that'd be helpful to a merchant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure there is any way to handle when the listener is null aside from just not crashing - without the listener we can't deliver a result, and updating the method to throw an error would be a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 is there any way for us to output something to the console? Mainly thinking for merchants who do something weird to have anything to point them potentially towards what they may have done wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added warning logs here if the listeners are null

listener.onVenmoSuccess(venmoAccountNonce);
} else {
Log.w(LOGGING_TAG, "Unable to deliver result to null listener");
}
}

private void deliverVenmoFailure(Exception error) {
if (listener != null) {
listener.onVenmoFailure(error);
} else {
Log.w(LOGGING_TAG, "Unable to deliver result to null listener");
}
}

Expand Down
25 changes: 25 additions & 0 deletions Venmo/src/test/java/com/braintreepayments/api/VenmoClientUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,31 @@ public void showVenmoInGooglePlayStore_sendsAnalyticsEvent() {
verify(braintreeClient).sendAnalyticsEvent("android.pay-with-venmo.app-store.invoked");
}

@Test
public void tokenizeVenmoAccount_whenListenerNull_returnsNothing() {
BraintreeClient braintreeClient = new MockBraintreeClientBuilder()
.configuration(venmoEnabledConfiguration)
.sessionId("session-id")
.integration("custom")
.authorizationSuccess(clientToken)
.build();

VenmoApi venmoApi = new MockVenmoApiBuilder()
.createPaymentContextSuccess("venmo-payment-context-id")
.build();

when(deviceInspector.isVenmoAppSwitchAvailable(activity)).thenReturn(true);

VenmoRequest request = new VenmoRequest(VenmoPaymentMethodUsage.SINGLE_USE);
request.setProfileId("sample-venmo-merchant");
request.setCollectCustomerBillingAddress(true);

VenmoClient sut = new VenmoClient(null, null, braintreeClient, venmoApi, sharedPrefsWriter, deviceInspector);
sut.tokenizeVenmoAccount(activity, request);
verify(listener, never()).onVenmoFailure(any(Exception.class));
verify(listener, never()).onVenmoSuccess(any(VenmoAccountNonce.class));
}

@Test
public void tokenizeVenmoAccount_whenCreatePaymentContextSucceeds_withObserver_launchesObserverWithVenmoIntentData_andSendsAnalytics() {
BraintreeClient braintreeClient = new MockBraintreeClientBuilder()
Expand Down
Loading