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

[App Switch] Add analytic events #1111

Merged
merged 25 commits into from
Sep 24, 2024

Conversation

richherrera
Copy link
Contributor

@richherrera richherrera commented Aug 13, 2024

Summary of changes

  • [DTMOBILES-173] & [DTMOBILES-899] Add relevant events for the flow



Checklist

  • Added a changelog entry
  • Relevant test coverage

Authors

@richherrera richherrera requested a review from a team as a code owner August 13, 2024 20:51
@richherrera richherrera marked this pull request as draft August 13, 2024 20:51
@richherrera richherrera changed the title [App Switch] Add analytic events [DO NOT REVIEW] [App Switch] Add analytic events Aug 13, 2024
@richherrera richherrera self-assigned this Sep 20, 2024
@richherrera richherrera changed the title [DO NOT REVIEW] [App Switch] Add analytic events [App Switch] Add analytic events Sep 20, 2024
@richherrera richherrera marked this pull request as ready for review September 20, 2024 21:31
Comment on lines 102 to 106
linkType = if (internalPayPalClient.isAppSwitchEnabled(payPalRequest) &&
internalPayPalClient.isPayPalInstalled(context)) {
LinkType.UNIVERSAL
} else {
LinkType.DEEPLINK
Copy link
Contributor

Choose a reason for hiding this comment

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

For PayPal in v5 all links are going to be universal/app links - we no longer support deep links

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 actually indicates if we fall back to the legacy (CCT) flow or are using the app switch flow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct, this is just for analytics purposes. In this case, we're setting the linkType to differentiate the type of flow that's happening. If the user is eligible for AppSwitch, we send the parameter as UNIVERSAL, otherwise, we send it as DEEPLINK

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we internally rename these to APP_LINK and APP_SWITCH? If we need to send certain values for analytics purposes, we could have a string value for each of these. I think it'd make things a bit clearer inside the SDK.

Comment on lines 112 to 113
if (internalPayPalClient.isAppSwitchEnabled(payPalRequest)) {
braintreeClient.sendAnalyticsEvent(PayPalAnalytics.APP_SWITCH_STARTED, analyticsParams)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this analytic event be moved up to right after the TOKENIZE_STARTED event? I assume we want to trigger this immediately at the start of the flow

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 to trigger this event right before we launch the Venice app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, this event needs to be called before launching the Venice app. The callback that comes afterward (callback.onPayPalPaymentAuthRequest) is the one that tells the fragment to launch the Venice app

Comment on lines 215 to 220
if (isAppSwitchFlow) {
braintreeClient.sendAnalyticsEvent(
PayPalAnalytics.HANDLE_RETURN_STARTED,
analyticsParams
)
}
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 should also move to right at the start of this method (line 189)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated: 209b57c

Comment on lines 102 to 106
linkType = if (internalPayPalClient.isAppSwitchEnabled(payPalRequest) &&
internalPayPalClient.isPayPalInstalled(context)) {
LinkType.UNIVERSAL
} else {
LinkType.DEEPLINK
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 actually indicates if we fall back to the legacy (CCT) flow or are using the app switch flow

Comment on lines 112 to 113
if (internalPayPalClient.isAppSwitchEnabled(payPalRequest)) {
braintreeClient.sendAnalyticsEvent(PayPalAnalytics.APP_SWITCH_STARTED, analyticsParams)
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 to trigger this event right before we launch the Venice app

@@ -153,12 +153,12 @@ internal class PayPalInternalClient(
.build()
}

private fun isAppSwitchEnabled(payPalRequest: PayPalRequest): Boolean {
fun isAppSwitchEnabled(payPalRequest: PayPalRequest): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these remain public or be internal at least? Not sure if we want to make them public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These methods are from PayPalInternalClient, which is now internal. I changed them to public because I need access to them for UTs. PayPalInternalClient and PayPalClient need them to define certain flow criteria, any suggestions? UTs are requiring these methods to be public

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 @tdchow has a suggestion on this, ideally we'd like to keep them private. If we are going to make them public temporarily we should likely add some docstrings saying it's not covered by semantic versioning so we can update in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe @richherrera is correct. Even though we removed the private modifiers, since this entire class is internal these functions will be hidden from merchants. We're also using these functions in PayPalClient. So we'll need to keep them as internal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Thank you both for the explanation.

@@ -153,12 +153,12 @@ internal class PayPalInternalClient(
.build()
}

private fun isAppSwitchEnabled(payPalRequest: PayPalRequest): Boolean {
fun isAppSwitchEnabled(payPalRequest: PayPalRequest): Boolean {
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 @tdchow has a suggestion on this, ideally we'd like to keep them private. If we are going to make them public temporarily we should likely add some docstrings saying it's not covered by semantic versioning so we can update in the future.

@richherrera richherrera merged commit 383917b into paypal-app-switch-feature Sep 24, 2024
3 checks passed
@richherrera richherrera deleted the add-app-switch-analytics branch September 24, 2024 16:36
tdchow added a commit that referenced this pull request Oct 11, 2024
* main: (54 commits)
  Firebase App Distribution Branch (#1182)
  Fix lint warnings - replace kapt with ksp, remove unused properties and resources, added Intent extension functions for handling deprecated functions (#1181)
  [open dev] Cleanup integration tests (#1180)
  Update README and CHANGELOG (#1176)
  App link setup docs (#1171)
  Revert to Braintree Merchant Server (#1173)
  Prepare for development
  Release 5.0.0
  Remove BA toast from PatPal Fragment (#1172)
  [App Switch] Update CHANGELOG (#1166)
  Make pending request constructors public (#1169)
  [App Switch] Add analytic events (#1111)
  Fix 3DS Cancel (#1161)
  Fix `os_version` and `enablePayPalAppSwitch` (#1162)
  Fix analytic event timestamps (#1160)
  Rename handleReturnToAppFromBrowser to handleReturnToApp (#1158)
  Parse error messages from multiple possible fields in the error response (#1156)
  External Interface Access Control (#1155)
  Update Google Pay Integration (#1153)
  Batch Analytics Events by Session ID (#1152)
  ...

# Conflicts:
#	CHANGELOG.md
#	Demo/src/main/java/com/braintreepayments/demo/PayPalFragment.java
#	Demo/src/main/java/com/braintreepayments/demo/PayPalRequestFactory.java
#	Demo/src/main/res/layout/fragment_paypal.xml
#	PayPal/src/main/java/com/braintreepayments/api/paypal/PayPalRequest.kt
#	PayPal/src/main/java/com/braintreepayments/api/paypal/PayPalVaultRequest.kt
#	PayPal/src/test/java/com/braintreepayments/api/paypal/PayPalVaultRequestUnitTest.java
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.

4 participants