Shopper insights rp1 feature include session #1235
Conversation
…intree/braintree_android into shopper-insights-rp1-feature-includeSessionId
| ) { | ||
|
|
||
| PayPalVaultRequest request = new PayPalVaultRequest(true); | ||
|
|
||
| if (!buyerEmailAddress.isEmpty()) { |
There was a problem hiding this comment.
Do we need to change all these other params? It seems like it should be unaffected by this PR
There was a problem hiding this comment.
My understanding is that the original code is potentially a run time crash if the value is null. In the case of the demo app and how its setup, setting the toggles to null didn't have an impact on this flow because the values would always be true.
There was a problem hiding this comment.
Ah ok in that case do we need to check that it's not null and it's not empty?
There was a problem hiding this comment.
Yes, great catch!
| @@ -72,6 +72,8 @@ import org.json.JSONException | |||
| * where the user has a PayPal Account with the same email. | |||
| * @property userPhoneNumber User phone number used to initiate a quicker authentication flow in | |||
| * cases where the user has a PayPal Account with the phone number. | |||
| * @property shopperSessionId value should be the shopper session ID returned from your server SDK | |||
There was a problem hiding this comment.
I think this doc string was included in the ticket but I'm wondering if it's clear enough for when we are outside of the shopper insights module. Should we include something like the shopper session ID returned from your shopper insights server SDK integration?. Also should this param be beta/experimental API @jaxdesmarais?
There was a problem hiding this comment.
I like it.
Current:
shopperSessionId value should be the shopper session ID returned from your server SDK request.
To:
shopperSessionId the shopper session ID returned from your shopper insights server SDK integration
There was a problem hiding this comment.
I'll make the changes in iOS as well.
There was a problem hiding this comment.
Yep we can for sure give this the beta/experimental API tag
| shopperSessionId?.let { | ||
| if (it.isNotEmpty()) parameters.put(SHOPPER_SESSION_ID, it) | ||
| } |
There was a problem hiding this comment.
We should be able to use the same simplified putOpt logic from the PayPalVaultRequest here
|
Did we manually test and confirm passing this parameter is working with the checkout and vault flows? |
Yes, but will need to test again with the updates. |
CHANGELOG.md
Outdated
| @@ -7,6 +7,8 @@ | |||
| * ShopperInsights (BETA) | |||
| * Add `isPayPalAppInstalled` and `isVenmoAppInstalled` methods | |||
| * Add `shopperSessionId` parameter to `ShopperInsightsClient` | |||
| * BraintreePayPal | |||
| * Add `shopperSessionInsight` | |||
There was a problem hiding this comment.
| * Add `shopperSessionInsight` | |
| * Add `shopperSessionId` to `PayPalCheckoutRequest` and `PayPalVaultRequest` |
| @@ -148,15 +148,17 @@ private void launchPayPal( | |||
| activity, | |||
| buyerEmailAddress, | |||
| buyerPhoneCountryCode, | |||
| buyerPhoneNationalNumber | |||
| buyerPhoneNationalNumber, | |||
| null | |||
There was a problem hiding this comment.
nit: it looks like there are some extra spaces for the 2 null properties here
| nationalNumberInput.editText?.text.toString() | ||
| nationalNumberInput.editText?.text.toString(), | ||
| shopperSessionId | ||
|
|
Summary of changes
Add public property shopperSessionId to BTPaypalRequest. Value should be passed to both the create_payment_resource and setup_billing_agreement endpoints. Demo app should be updated enable/disable passing this value.
Checklist
Authors
@stechiu
@warmkesselj
@jwarmkessel