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

Shopper insights rp2 feature presentment details #1244

Conversation

warmkesselj
Copy link
Contributor

@warmkesselj warmkesselj commented Dec 16, 2024

Summary of changes

Adding PresentmentDetails and additional properties to track experiments related to the ButtonTypes, what Page the button is presented on, and the order of which they are presented. Additionally, refactoring functions to centralize the flow for tracking these experiments.

Screenshot 2024-12-18 at 10 23 38 AM

Checklist

  • Added a changelog entry
  • Relevant test coverage
  • Tested and confirmed payment flows affected by this change are functioning as expected

Authors

@warmkesselj
@jwarmkessel

* @param type An ExperimentType that is either a control or test type
*/
data class PresentmentDetails (
val treatmentName: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

The parameters here don't seem to match the ticket description - can you confirm? It should align with iOS too.

@sarahkoop
Copy link
Contributor

It might be good to resolve the conflicts in this branch before making any of the commented changes too!

…-feature-presentment-details

# Conflicts:
#	BraintreeCore/src/main/java/com/braintreepayments/api/core/AnalyticsEventParams.kt
#	ShopperInsights/src/main/java/com/braintreepayments/api/shopperinsights/ShopperInsightsClient.kt
/**
* PayPal button
*/
PAYPAL("paypal"),
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
PAYPAL("paypal"),
PAYPAL("PayPal"),

/**
* Venmo button
*/
VENMO("venmo"),
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
VENMO("venmo"),
VENMO("Venmo"),

/**
* Other button
*/
OTHER("other"),
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
OTHER("other"),
OTHER("Other"),

experiment: String? = null,
paymentMethodsDisplayed: List<String> = emptyList()
fun sendPresentedEvent(
shopperSessionId: String? = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to pass the session ID here, we will have it when this class is initialized and can pass it under the hood.

fun sendPayPalPresentedEvent(
experiment: String? = null,
paymentMethodsDisplayed: List<String> = emptyList()
fun sendPresentedEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a CHANGELOG entry for this method change

fun sendPresentedEvent(
shopperSessionId: String? = null,
presentmentDetails: PresentmentDetails? = null,
paymentMethodsDisplayed: List<String> = emptyList(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove paymentMethodsDisplayed

shopperSessionId: String? = null,
presentmentDetails: PresentmentDetails? = null,
paymentMethodsDisplayed: List<String> = emptyList(),
buttonType: ButtonType? = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets list the buttonType first to match the signature on iOS and Android.

fun sendPayPalPresentedEvent(
experiment: String? = null,
paymentMethodsDisplayed: List<String> = emptyList()
fun sendPresentedEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want both buttonType and presentmentDetails to be required vs defaulted to null

// * @param paymentMethodsDisplayed optional The list of available payment methods,
// * rendered in the same order in which they are displayed
// */
// fun sendVenmoPresentedEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be removed vs commented out

@warmkesselj warmkesselj marked this pull request as ready for review December 17, 2024 20:35
@warmkesselj warmkesselj requested a review from a team as a code owner December 17, 2024 20:35
/**
* First place
*/
FIRST("1"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it matter if we are sending these numbers as strings vs Int on iOS? Does it come through in FPTI the same way?

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 think we may need to do it this way because of the last enum being "other".

/**
* The homepage
*/
HOMEPAGE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does capitalization here matter? On iOS we are sending these all lowercase values - wondering if we should do the same on Android

val shopperSessionId: String? = null,
val buttonType: String? = null,
val buttonOrder: String? = null,
val pageType: String? = null
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't technically need doc strings for this class, but since we have them for all other params lets add them for consistency and for our future reference

CHANGELOG.md Outdated
@@ -7,6 +7,11 @@
* Add `shopperSessionId` parameter to `ShopperInsightsClient`
* BraintreePayPal
* Add `shopperSessionId` to `PayPalCheckoutRequest` and `PayPalVaultRequest`
* Replace `sendPayPalPresentedEvent()` and `sendVenmoPresentedEvent()` with `sendPresentedEvent(for buttonType: ButtonType, presentmentDetails: PresentmentDetail, shopperSessionId: String)`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Replace `sendPayPalPresentedEvent()` and `sendVenmoPresentedEvent()` with `sendPresentedEvent(for buttonType: ButtonType, presentmentDetails: PresentmentDetail, shopperSessionId: String)`
* Replace `sendPayPalPresentedEvent()` and `sendVenmoPresentedEvent()` with `sendPresentedEvent(buttonType: ButtonType, presentmentDetails: PresentmentDetail, shopperSessionId: String)`

@@ -0,0 +1,15 @@
package com.braintreepayments.api.shopperinsights

enum class ExperimentType(val rawValue: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a doc string for this enum? Can rawValue also be an internal property, or can it even be private if it's only used in formattedExperiment()?

Comment on lines 6 to 8
* @param type An ExperimentType that is either a control or test type
* @param buttonOrder optional Represents this buttons order in context of other buttons.
* @param pageType optional Represents the page or view the button is rendered on.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @param type An ExperimentType that is either a control or test type
* @param buttonOrder optional Represents this buttons order in context of other buttons.
* @param pageType optional Represents the page or view the button is rendered on.
* @property type An ExperimentType that is either a control or test type
* @property buttonOrder optional Represents this buttons order in context of other buttons.
* @property pageType optional Represents the page or view the button is rendered on.

We should use the property annotation for class properties.

Comment on lines 3 to 5
/**
The button type to be displayed or presented.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/**
The button type to be displayed or presented.
*/
/**
* The button type to be displayed or presented.
*/

CONTROL("control"),
TEST("test");

fun formattedExperiment(): String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this function only used inside the SDK? If so, can we make it internal?

CHANGELOG.md Outdated
@@ -7,6 +7,11 @@
* Add `shopperSessionId` parameter to `ShopperInsightsClient`
* BraintreePayPal
* Add `shopperSessionId` to `PayPalCheckoutRequest` and `PayPalVaultRequest`
* Replace `sendPayPalPresentedEvent()` and `sendVenmoPresentedEvent()` with `sendPresentedEvent(for buttonType: ButtonType, presentmentDetails: PresentmentDetail, shopperSessionId: String)`
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
* Replace `sendPayPalPresentedEvent()` and `sendVenmoPresentedEvent()` with `sendPresentedEvent(for buttonType: ButtonType, presentmentDetails: PresentmentDetail, shopperSessionId: String)`
* Replace `sendPayPalPresentedEvent()` and `sendVenmoPresentedEvent()` with `sendPresentedEvent()`

Nitpick but I don't think we need the whole method signature in the CHANGELOG

@@ -0,0 +1,15 @@
package com.braintreepayments.api.shopperinsights

enum class ExperimentType(val rawValue: String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doc strings

* @param paymentMethodsDisplayed optional The list of available payment methods,
* rendered in the same order in which they are displayed
* @param buttonType Represents the tapped button type.
* @param presentmentDetails JSON string representing an experiment you want to run.
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
* @param presentmentDetails JSON string representing an experiment you want to run.
* @param presentmentDetails Detailed information, including button order, experiment type, and page type about the payment button that is sent to analytics to help improve the Shopper Insights feature experience.

To align with iOS - or open to different wording

* @param experiment optional JSON string representing an experiment you want to run
* @param paymentMethodsDisplayed optional The list of available payment methods,
* rendered in the same order in which they are displayed
* @param buttonType Represents the tapped button type.
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
* @param buttonType Represents the tapped button type.
* @param buttonType Type of button presented - PayPal, Venmo, or Other

CHANGELOG.md Outdated
@@ -7,6 +7,11 @@
* Add `shopperSessionId` parameter to `ShopperInsightsClient`
* BraintreePayPal
* Add `shopperSessionId` to `PayPalCheckoutRequest` and `PayPalVaultRequest`
* Replace `sendPayPalPresentedEvent()` and `sendVenmoPresentedEvent()` with `sendPresentedEvent(for buttonType: ButtonType, presentmentDetails: PresentmentDetail, shopperSessionId: String)`
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the shopper session ID is passed into this method

/**
* The homepage
*/
homepage,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to clarify my comment here - I think these enum names should be capitalized, but they may need an additional lowercase string value like HOMEPAGE("homepage") if it's a requirement to pass the values lower cased. @jaxdesmarais do you know if they need to be case sensitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When doing an SQL Query a dev can specify case insensitivity. But I think for Lighthouse IQ I don't think we can do that. I'll update to use HOMEPAGE("homepage").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

We will want it to be lowercase to make life easier on the Analytics team since that's how iOS and Web will be passing these. I think the syntax y'all noted makes sense!

@@ -7,6 +7,7 @@
* Add `shopperSessionId` parameter to `ShopperInsightsClient`
* BraintreePayPal
* Add `shopperSessionId` to `PayPalCheckoutRequest` and `PayPalVaultRequest`
* Replace `sendPayPalPresentedEvent()` and `sendVenmoPresentedEvent()` with `sendPresentedEvent()`
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if in Android it's similar to iOS - sendPresentedEvent(for:presentmentDetails:) instead of sendPresentedEvent()

Copy link
Contributor

@stechiu stechiu left a comment

Choose a reason for hiding this comment

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

Other than my one comment, everything else LGTM

Copy link
Contributor

@jaxdesmarais jaxdesmarais left a comment

Choose a reason for hiding this comment

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

🔥

@warmkesselj warmkesselj merged commit 894fc2d into shopper-insights-rp2-feature Dec 19, 2024
3 checks passed
@warmkesselj warmkesselj deleted the shopper-insights-rp2-feature-presentment-details branch December 19, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants