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 - Added Presentment Details Object and Update Presented Events #1484

Merged

Conversation

stechiu
Copy link
Collaborator

@stechiu stechiu commented Dec 12, 2024

Summary of changes

  • New BTButtonType enum
  • New BTButtonOrder enum
  • New BTExperimentType enum
  • New BTPageType enum
  • Replaced sendPayPalPresentedEvent and sendVenmoPresentedEvent to sendPresentedEvent(for buttonType: BTButtonType, presentmentDetails: BTPresentmentDetails)

Checklist

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

Authors

List GitHub usernames for everyone who contributed to this pull request.

@stechiu stechiu marked this pull request as ready for review December 16, 2024 21:33
@stechiu stechiu requested a review from a team as a code owner December 16, 2024 21:33
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.

It looks like there is a compiler issue currently causing the build to fail that we will want to address. Also, do we want to add unit tests to this PR or do you want to make a follow up ticket for testing?

CHANGELOG.md Outdated Show resolved Hide resolved
Sources/BraintreeCore/Analytics/FPTIBatchData.swift Outdated Show resolved Hide resolved
Sources/BraintreeShopperInsights/BTButtonType.swift Outdated Show resolved Hide resolved
@stechiu
Copy link
Collaborator Author

stechiu commented Dec 17, 2024

It looks like there is a compiler issue currently causing the build to fail that we will want to address. Also, do we want to add unit tests to this PR or do you want to make a follow up ticket for testing?

I was working on trying to figure out why there's a compiler issue yesterday. It's likely due to a bad merge from a parent branch. Will continue to figure it out today

I wrote tests in BTShopperInsightsClient_Tests unless you're expecting something different

@stechiu
Copy link
Collaborator Author

stechiu commented Dec 17, 2024

It looks like there is a compiler issue currently causing the build to fail that we will want to address. Also, do we want to add unit tests to this PR or do you want to make a follow up ticket for testing?

I was working on trying to figure out why there's a compiler issue yesterday. It's likely due to a bad merge from a parent branch. Will continue to figure it out today

I wrote tests in BTShopperInsightsClient_Tests unless you're expecting something different

I added a few more tests to check that buttonOrder, buttonType, experimentType, and pageType were passing to the API Client

case connectionStartTime = "connect_start_time"
case correlationID = "correlation_id"
case errorDescription = "error_desc"
case eventName = "event_name"
case experimentType = "experiment_type"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed, we don't need this, lets reuse merchantExperiment below

…/braintree/braintree_ios into shopper-insights-rp2-presentment

# Conflicts:
#	Sources/BraintreeShopperInsights/BTShopperInsightsClient.swift
/// - Warning: This module is in beta. It's public API may change or be removed in future releases.
public enum BTButtonOrder: String {

/// 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets update these docstrings to represent the values accurately

Comment on lines 46 to 47
/// The experiment type that is sent to analytics to help improve the Shopper Insights feature experience.
let experimentType: String?
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 delete this and use the experiment that already exists

Comment on lines 985 to 1002
func testInvokedOpenURLSuccessfully_whenSuccess_sendsAppSwitchSucceededWithAppSwitchURL() {
let eventName = BTPayPalAnalytics.appSwitchSucceeded
let fakeURL = URL(string: "some-url")!
payPalClient.invokedOpenURLSuccessfully(true, url: fakeURL) { _, _ in }

XCTAssertEqual(mockAPIClient.postedAnalyticsEvents.last!, eventName)
XCTAssertEqual(mockAPIClient.postedAppSwitchURL[eventName], fakeURL.absoluteString)
}

func testInvokedOpenURLSuccessfully_whenFailure_sendsAppSwitchFailedWithAppSwitchURL() {
let eventName = BTPayPalAnalytics.appSwitchFailed
let fakeURL = URL(string: "some-url")!
payPalClient.invokedOpenURLSuccessfully(false, url: fakeURL) { _, _ in }

XCTAssertEqual(mockAPIClient.postedAnalyticsEvents.first!, eventName)
XCTAssertEqual(mockAPIClient.postedAppSwitchURL[eventName], fakeURL.absoluteString)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets revert deleting these tests

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines +107 to +139
private func togglePayPalVaultButton(enabled: Bool) {
payPalVaultButton.isEnabled = enabled

guard enabled else { return }

let presentmentDetails = BTPresentmentDetails(
buttonOrder: .first,
experimentType: .control,
pageType: .about
)

shopperInsightsClient.sendPresentedEvent(
for: .payPal,
presentmentDetails: presentmentDetails
)
}

private func toggleVenmoButton(enabled: Bool) {
venmoButton.isEnabled = enabled

guard enabled else { return }

let presentmentDetails = BTPresentmentDetails(
buttonOrder: .second,
experimentType: .control,
pageType: .about
)

shopperInsightsClient.sendPresentedEvent(
for: .venmo,
presentmentDetails: presentmentDetails
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Take it or leave it but we could combine this logic if we wanted:

Suggested change
private func togglePayPalVaultButton(enabled: Bool) {
payPalVaultButton.isEnabled = enabled
guard enabled else { return }
let presentmentDetails = BTPresentmentDetails(
buttonOrder: .first,
experimentType: .control,
pageType: .about
)
shopperInsightsClient.sendPresentedEvent(
for: .payPal,
presentmentDetails: presentmentDetails
)
}
private func toggleVenmoButton(enabled: Bool) {
venmoButton.isEnabled = enabled
guard enabled else { return }
let presentmentDetails = BTPresentmentDetails(
buttonOrder: .second,
experimentType: .control,
pageType: .about
)
shopperInsightsClient.sendPresentedEvent(
for: .venmo,
presentmentDetails: presentmentDetails
)
}
private func toggleButtons(result: BTShopperInsightsResult) {
payPalVaultButton.isEnabled = result.isPayPalRecommended
venmoButton.isEnabled = result.isVenmoRecommended
let payPalPresentmentDetails = BTPresentmentDetails(
buttonOrder: .first,
experimentType: .control,
pageType: .about
)
let venmoPresentmentDetails = BTPresentmentDetails(
buttonOrder: .second,
experimentType: .control,
pageType: .about
)
if result.isPayPalRecommended {
shopperInsightsClient.sendPresentedEvent(
for: .payPal,
presentmentDetails: payPalPresentmentDetails
)
}
if result.isVenmoRecommended {
shopperInsightsClient.sendPresentedEvent(
for: .venmo,
presentmentDetails: venmoPresentmentDetails
)
}
}

@warmkesselj warmkesselj merged commit 365a059 into shopper-insights-rp2-feature Dec 20, 2024
8 checks passed
@warmkesselj warmkesselj deleted the shopper-insights-rp2-presentment branch December 20, 2024 19:53
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.

3 participants