-
Notifications
You must be signed in to change notification settings - Fork 238
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
Create FPTI Analytics Event Enums #820
Conversation
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: it looks like most of the files don't have spaces at the end, is that our preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No there should be newlines - fixed 👍
Card/src/main/java/com/braintreepayments/api/CardAnalytics.java
Outdated
Show resolved
Hide resolved
AmericanExpress/src/main/java/com/braintreepayments/api/AmericanExpressAnalytics.java
Outdated
Show resolved
Hide resolved
Referencing our events doc again, I wonder if any of these jump out to you as not-trackable on Android? Mostly around the browser-presentation / challenge (CCT) succeeded, failed, and canceled. I know we have limitations on Android to our insights into the CCT. |
AmericanExpress/src/test/java/com/braintreepayments/api/AmericanExpressAnalyticsUnitTest.java
Outdated
Show resolved
Hide resolved
I think everything in there should be trackable on Android - but if we want to change any during implementation that's fine too. For cancellation, we can't track the actual cancel from CCT currently, but there is still the "cancel" button on the actual PayPal page that can be tracked (returned as cancel via parsed URL). |
AmericanExpress/src/main/java/com/braintreepayments/api/AmericanExpressClient.java
Outdated
Show resolved
Hide resolved
…anExpressClient.java Co-authored-by: scannillo <[email protected]>
…ree_android into analytics_enums
Gotcha! If whatever we do send differs in meaning from iOS, we should update the description/info in our excel doc. Also if events don't apply to one platform vs the other we should make a note of it (for example the alert-cancel ones). Maybe we can add a column with a dropdown like 'iOS/Android/Both' next to the events? I don't mind adding that if we think it would help. |
BROWSER_SWITCH_SUCCEEDED("paypal:tokenize:browser-presentation:succeeded"), | ||
BROWSER_SWITCH_FAILED("paypal:tokenize:browser-presentation:failed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On iOS we have a synchronous bool
return value from ASWeb into if the ASWeb view presented or didn't.
Do we have this insight on Android? A way to know if the CCT displayed or didn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BrowserSwitchClient#start
(which launches the CCT) throws a BrowserSwitchException
in some cases, mainly for improper deep linking configuration, but also if the host activity is finishing while trying to launch CCT. In those cases if we receive that BrowserSwitchException
, we would send that BROWSER_SWITCH_FAILED
analytic event on Android. So it's not a 1 to 1 match with the iOS meaning of that event, but I think it's still useful for Android
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, this is great - thanks for the explanation
I'll add a comment to each of the analytics implementation tickets to update the source of truth doc with the event description for Android and to note if any don't apply. I think it will be easier to confirm once the events are actually migrated/implemented. Agreed a column for iOS/Android/Both would be good |
PAYMENT_REQUEST_FAILED("google-pay:payment-request:failed"), | ||
PAYMENT_REQUEST_SUCCEEDED("google-pay:payment-request:succeeded"), | ||
|
||
TOKENIZE_STARTED("google-pay:tokenize:started"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - should we add the // Conversion events
comment here too?
Summary of changes
Checklist
[ ] Added a changelog entryAuthors