-
Notifications
You must be signed in to change notification settings - Fork 6
chore: Implement analytics for Checkout Components #1379
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
base: bn/feature/checkout-components
Are you sure you want to change the base?
chore: Implement analytics for Checkout Components #1379
Conversation
Generated by 🚫 Danger Swift against dff6019 |
Appetize link: https://appetize.io/app/qhkawl3zzxsmmzgz5dcktoxkdq |
0372335
to
ea4d456
Compare
Enhanced all CheckoutComponents analytics events to include required and optional metadata fields according to the event definitions spec: - Add userLocale (Locale.current.identifier) to all analytics events - Extract paymentId and paymentMethod from PrimerError.paymentFailed for PAYMENT_FAILURE events - Include paymentMethod context in card form analytics events (PAYMENT_DETAILS_ENTERED, PAYMENT_SUBMITTED, PAYMENT_PROCESSING_STARTED) - Add paymentMethod to PAYMENT_THREEDS event from token data - Add userLocale to PAYMENT_REDIRECT_TO_THIRD_PARTY event Changed files: - DefaultCheckoutScope.swift: Enhanced state tracking with metadata extraction helper for failure cases - CheckoutSDKInitializer.swift: Added userLocale to SDK init events - DefaultCardFormScope.swift: Added metadata to card form events - DefaultPaymentMethodSelectionScope.swift: Added userLocale - HeadlessRepositoryImpl.swift: Enhanced 3DS and redirect event metadata Note: PAYMENT_REATTEMPTED event not implemented as retry feature does not exist yet.
Sources/PrimerSDK/Classes/CheckoutComponents/Analytics/Data/Models/AnalyticsEventMetadata.swift
Show resolved
Hide resolved
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.
Not related to this task. Just a quick SwiftLint warning cleanup.
Sources/PrimerSDK/Classes/CheckoutComponents/Analytics/Utils/DeviceInfoProvider.swift
Outdated
Show resolved
Hide resolved
Sources/PrimerSDK/Classes/CheckoutComponents/Analytics/Utils/DeviceInfoProvider.swift
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,326 @@ | |||
// |
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.
We have a very similar file in the SDK UIDeviceExtension.swift
. May be premature, but if we need both, this could be in a shared utils space
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.
I know, I separated it intentionally because of future (let's hear from Henry) modularisation. But to add my 2 cents, the first thing that should be extracted to some kind of Util module is LogReporter, DeviceInfoProvider, and similar.
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.
I think we should point any device info calls to the existing Device
and Model
objects and modularise that one when the time comes and avoid duplicating here.
Device info it's often, but not strictly the domain of analytics. Any analytics-specific properties can, in their domain, extend the current Device.swift
or Model.swift
Sources/PrimerSDK/Classes/CheckoutComponents/Analytics/Utils/UUIDGenerator.swift
Outdated
Show resolved
Hide resolved
.../PrimerSDK/Classes/CheckoutComponents/Internal/Presentation/Scope/DefaultCardFormScope.swift
Outdated
Show resolved
Hide resolved
Sources/PrimerSDK/Classes/CheckoutComponents/Internal/Services/CheckoutSDKInitializer.swift
Outdated
Show resolved
Hide resolved
1d83746
to
622c18b
Compare
622c18b
to
540eafd
Compare
|
||
/// Core analytics event service responsible for constructing and sending events. | ||
/// Thread-safe actor that maintains session state and handles HTTP communication. | ||
actor AnalyticsEventService: CheckoutComponentsAnalyticsServiceProtocol, LogReporter { |
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.
This object has a few too many responsibilities. You could certainly delegate the constructing of the payload, networking out of here and perhaps also the buffering
@@ -0,0 +1,326 @@ | |||
// |
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.
I think we should point any device info calls to the existing Device
and Model
objects and modularise that one when the time comes and avoid duplicating here.
Device info it's often, but not strictly the domain of analytics. Any analytics-specific properties can, in their domain, extend the current Device.swift
or Model.swift
switch state { | ||
case .ready: | ||
// Checkout flow is now interactive | ||
await analyticsInteractor?.trackEvent(.checkoutFlowStarted, metadata: .general(GeneralEvent())) |
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.
Associated values can have default values, eg case general(GeneralEvent = GeneralEvent())
so you could just do metadata: .general()
|
||
do { | ||
guard let container = await DIContainer.current else { | ||
// DI Container not available |
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.
There's a few superfluous comments here
private func isLikelyURL(_ string: String) -> Bool { | ||
guard !string.isEmpty else { return false } | ||
let lowercased = string.lowercased() | ||
return lowercased.hasPrefix("http://") || lowercased.hasPrefix("https://") |
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.
private func isLikelyURL(_ string: String) -> Bool {
["http://", "https://"].contains { string.lowercased().hasPrefix($0) }
}
} | ||
} | ||
|
||
private func resolveThreeDSProvider() -> String? { |
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.
probably more appropriate as a computed property
private var settingsService: Any? | ||
|
||
/// Analytics interactor for tracking events (iOS 15.0+ only) | ||
private var analyticsInteractor: Any? |
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.
Why doesn't this have a type?
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.
Slip. Good catch.
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
func trackEvent(_ eventType: AnalyticsEventType, metadata: AnalyticsEventMetadata?) async { | ||
// Launch a child task to keep fire-and-forget behavior without leaving structured concurrency | ||
// This prevents blocking the caller even if the service is busy | ||
let eventMetadata = metadata |
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.
is this assignment necessary?
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.
removed.
} else if ["i386", "x86_64", "arm64"].contains(identifier) { | ||
// Simulator - check what device it's simulating | ||
if let simModelCode = ProcessInfo().environment["SIMULATOR_MODEL_IDENTIFIER"] { | ||
if simModelCode.hasPrefix("iPhone") { |
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.
This identical-to-above logic could be extracted into a private helper
func testBuffer_AddsEventToBuffer() async { | ||
// Given | ||
let eventType = AnalyticsEventType.sdkInitStart | ||
let metadata: AnalyticsEventMetadata = .general(GeneralEvent()) |
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.
can be shortened to use the default value
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.
A lot of the tests in this and the next files aren't asserting anything, they are just firing the events, is this deliberate?
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.
These are intentional smoke tests for fire-and-forget patterns. This is deliberate but not ideal. Let me improve it...
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.
There is a static extension on string uuid
that avoids having to create a new object which maybe worth a consideration here
- Remove unnecessary metadata variable assignment in DefaultAnalyticsInteractor - Simplify AnalyticsEventBufferTests to use default nil metadata value - Replace UUIDGenerator with existing String.uuid extension - Extract duplicate device type checking logic into private helper method in UIDeviceExtension - Add proper assertions to AnalyticsEventServiceTests"
|
Overview
This PR implements a comprehensive analytics system for CheckoutComponents following the event definitions specification. The analytics system tracks all key lifecycle events throughout the checkout
and payment flow, providing crucial insights into user behavior and system performance.
What Changed
Initial Implementation
Built the complete analytics infrastructure from the ground up:
Core Components
Data Models
AnalyticsEventType
: Enum defining all 13 trackable eventsAnalyticsEventMetadata
: Metadata container for optional event attributesAnalyticsPayload
: Complete payload structure for backend submissionAnalyticsSessionConfig
: Session configuration with account/client token infoEvent Integration
Integrated analytics tracking across the CheckoutComponents flow:
SDK_INIT_START
,SDK_INIT_END
)CHECKOUT_FLOW_STARTED
)PAYMENT_METHOD_SELECTION
)PAYMENT_DETAILS_ENTERED
,PAYMENT_SUBMITTED
)PAYMENT_PROCESSING_STARTED
)PAYMENT_THREEDS
)PAYMENT_REDIRECT_TO_THIRD_PARTY
)PAYMENT_SUCCESS
,PAYMENT_FAILURE
)PAYMENT_FLOW_EXITED
)Dependency Injection
Registered analytics components in the DI container with proper lifecycle management
Tests
Added comprehensive test coverage:
AnalyticsEventServiceTests
(267 tests)DeviceInfoProviderTests
(579 tests)AnalyticsModelsTests
(409 tests)CheckoutComponentsAnalyticsInteractorTests
(217 tests)AnalyticsEnvironmentProviderTests
(234 tests)UUIDGeneratorTests
(189 tests)Discriminated Union Refactoring (Based on Review Feedback)
Context: Following code review feedback from @NQuinn27, refactored
AnalyticsEventMetadata
from an optional-heavy struct to a type-safe discriminated union.Problem: The original struct design had 10 optional fields, allowing invalid combinations like 3DS metadata on general events.
Solution: Implemented Swift enum with associated values for each event category:
Benefits:
Event Coverage
Implemented Events ✅
SDK_INIT_START
- SDK initialization beginsSDK_INIT_END
- SDK initialization completesCHECKOUT_FLOW_STARTED
- UI becomes interactivePAYMENT_METHOD_SELECTION
- User selects payment methodPAYMENT_DETAILS_ENTERED
- Payment details validatedPAYMENT_SUBMITTED
- User taps PayPAYMENT_PROCESSING_STARTED
- Payment processing beginsPAYMENT_REDIRECT_TO_THIRD_PARTY
- Redirect to third-party initiatedPAYMENT_THREEDS
- 3DS challenge presentedPAYMENT_SUCCESS
- Payment completes successfullyPAYMENT_FAILURE
- Payment failsPAYMENT_FLOW_EXITED
- User exits checkoutNot Implemented ⏸️
PAYMENT_REATTEMPTED
- Requires retry feature (not yet available)Technical Details
Architecture
Event Flow
SDK Init → Checkout Ready → Payment Method Selection →
Details Entry → Submit → Processing → [3DS/Redirect] →
Success/Failure → Exit
Metadata Schema
All events include:
id
,eventName
,timestamp
,checkoutSessionId
,clientSessionId
,sdkType
,sdkVersion
,primerAccountId
userLocale
,paymentMethod
,paymentId
,redirectDestinationUrl
,threedsProvider
,threedsResponse
,device
,deviceType
Testing
References
Notes