Improve Analytics#444
Conversation
| * [com.paypal.android.paymentbuttons.analytics.PaymentButtonAnalytics]. | ||
| */ | ||
| @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
| interface AnalyticsServiceWrapper { |
There was a problem hiding this comment.
q: What's the purpose of this interface?
There was a problem hiding this comment.
It enforces a consistent pattern across all analytics wrappers in the SDK. Every wrapper (CardAnalytics, PayPalWebAnalytics, PaymentButtonAnalytics) must hold an AnalyticsService instance.
Without the interface, that's just a convention with nothing enforcing it. It also makes the pattern self-documenting for anyone adding a new analytics wrapper in the future.
There was a problem hiding this comment.
I'm still confused about this "wrapper" concept. The genericness of the Wrapper suffix feels like a code smell–if it had a legitimate purpose it would arguably have a better name explaining the role that it takes. We also have no declarations in this PR of a variable of type AnalyticsServiceWrapper meaning this interface isn't being used.
| * Internal so subclasses can fire INITIALIZED in their own init blocks. | ||
| */ | ||
| internal val analytics: PaymentButtonAnalytics? = | ||
| coreConfig?.let { PaymentButtonAnalytics(AnalyticsService(context, it)) } |
There was a problem hiding this comment.
qq: CoreConfig may be null here and we won't get analytics. I feel like merchant's probably won't be instantiating these buttons programmatically (they'll probably be inflated via XML). Would merchants have to set CoreConfig programatically after the view is inflated?
There was a problem hiding this comment.
I intentionally removed XML attribute support for CoreConfig after discussion in the Slack thread.
Since Android Views are moving to maintenance mode, won't merchants be using compose?
I can re-add support for passing in clientId and env in XML attributes, if that's what's preferred Happy to do it.
There was a problem hiding this comment.
We'll still need support for XML since technically merchants will be able to use these views in XML layouts by virtual of them being Android Views. If we want to avoid this, we will have to author pure Composables (which isn't a bad idea, the buttons have a considerable amount of technical debt and there are new button designs in the wild that we have yet to adopt).
There was a problem hiding this comment.
Added analytics support for PaymentButtons added via XML
| verify(exactly = 1) { | ||
| analyticsService.sendAnalyticsEvent( | ||
| "payment-button:tapped", | ||
| params = mapOf(AnalyticsParams.BUTTON_TYPE to "Pay Later") |
There was a problem hiding this comment.
| params = mapOf(AnalyticsParams.BUTTON_TYPE to "Pay Later") | |
| params = mapOf(AnalyticsParams.BUTTON_TYPE to "pay_later") |
random q: Would snake_case or hyphen-case work better here for analytics? I'm not sure if there are any rules around the formatting about the values we send.
There was a problem hiding this comment.
I'm not sure, but that's something we can discuss separately and address as a follow up. Android and iOS both need to have consistent naming here.
There was a problem hiding this comment.
Yeah we'll want these values to align across both iOS and Android (and possibly JS where possible).
|
@anibalb2500 we may need to figure out a branching strategy when @kgangineni gets back. This may belong in a feature branch (or at least go into |
349446840464
left a comment
There was a problem hiding this comment.
Can I ask about the sandbox new resperatory analizer , as new SQL that you create? With data files set?
… architecture PaymentButtons: - Add PaymentButtonEvent enum (INITIALIZED, TAPPED) and PaymentButtonAnalytics wrapper, following the same pattern as CheckoutEvent/VaultEvent (PayPalWebPayments) and ApproveOrderEvent/VaultEvent (CardPayments) - Replace AnalyticsService(clientId='N/A', environment=Environment.LIVE) in PaymentButton with an optional CoreConfig constructor param — analytics is created from the provided config; if none is given analytics is null and no events fire, eliminating fake-credential events hitting live FPTI - INITIALIZED fires in each subclass init block; TAPPED fires via performClick() so it fires regardless of whether a click listener is set - Make PaymentButton always clickable (isClickable = true) so performClick() is reachable on XML-inflated buttons with no click listener CorePayments: - sendAnalyticsEvent(name, orderId?, buttonType?) replaced with sendAnalyticsEvent(name, params: Map<String, String>) — AnalyticsService no longer knows about checkout or button concepts - Add AnalyticsParams object with ORDER_ID, SETUP_TOKEN_ID, and BUTTON_TYPE constants accessible across all SDK modules - Add AnalyticsServiceWrapper interface enforcing that every analytics wrapper holds an AnalyticsService; implemented by CardAnalytics, PayPalWebAnalytics, and PaymentButtonAnalytics - Vault event paths in CardAnalytics and PayPalWebAnalytics now correctly use AnalyticsParams.SETUP_TOKEN_ID instead of ORDER_ID - Extract log tag in AnalyticsService into a constant; correct log severity on failure paths (Log.d -> Log.w / Log.e) - Add KDoc to CoreConfig documenting the intentional live-endpoint override Tests: - Add PaymentButtonAnalyticsTest and PaymentButtonTest with Robolectric - Add test infrastructure (MockK, Robolectric, includeAndroidResources) to PaymentButtons module which had no prior test setup - Update AnalyticsServiceTest and TrackingEventsAPIUnitTest for new signatures
f7367dc to
0b0c696
Compare
9017e6a to
529dacb
Compare
529dacb to
9fbd980
Compare
|
@sshropshire I've rebased the PR and refactored the event params. Please take a look when you get a chance. |
Thank you for your contribution to PayPal.
Summary of changes
PaymentButtons — introduce typed analytics wrapper
PaymentButtonpreviously fired analytics events inline using a bareAnalyticsService.This PR introduces a thin typed wrapper layer consistent with how other modules handle analytics:
PaymentButtonEventenum with typedINITIALIZEDandTAPPEDconstants, followingthe pattern established by
CheckoutEvent/VaultEvent(PayPalWebPayments) andApproveOrderEvent/VaultEvent(CardPayments)PaymentButtonAnalyticswrapper class delegating toAnalyticsService, consistentwith
CardAnalyticsandPayPalWebAnalyticsin other modulessetOnClickListeneroverride to dispatchTAPPEDthrough the new wrapperCorePayments — generic analytics params and vault token fix
AnalyticsService.sendAnalyticsEventpreviously leaked checkout and button concepts(
orderId,buttonType) into the core layer. These are now replaced with a genericAnalyticsEventParamsdata class soAnalyticsServiceandAnalyticsEventDatahave noknowledge of what the params mean.
sendAnalyticsEvent(name, orderId?, buttonType?)→sendAnalyticsEvent(name, params: AnalyticsEventParams)AnalyticsEventParamsholds typed fields:orderId,vaultSetupToken,buttonType,appSwitchEnabledTrackingEventsAPImaps known FPTI keys (order_id,vault_setup_token) from the paramsstruct when building the serialized request — the wire format is unchanged
PayPalWebAnalyticsandCardAnalyticswas passing setup token asorderId; now uses dedicatedvaultSetupTokenfieldvault_setup_tokenFPTI param wired throughTrackingEventDataCorePayments — minor logging and documentation improvements
"[PayPal SDK]"log tag into a namedTAGconstant to eliminate duplicationLog.d→Log.w/Log.e)CoreConfigdocumenting that FPTI analytics always route to the live endpointregardless of the configured
EnvironmentEvidence
Web checkout flow validated end-to-end using Order-ID
2DH291271Y007604BAnalytics.Improvement.Evidence.6-12.mov
Web vault flow validated end-to-end using vault_setup_token
5C810075PJ396333LVault.Evidence.mov
Buttons Regression Testing
Buttons.Regression.mov
Note I removed the following improvements from PaymentButton that were previously added, to stay in scope of our work
Checklist
Authors