Bugfix - send analytics even when BTAPIClient deallocated #1379
Conversation
scannillo
force-pushed
the
replicate-main-analytics-issue
branch
from
3493592 to
1b47adc
Compare
scannillo
force-pushed
the
replicate-main-analytics-issue
branch
from
1b47adc to
0515294
Compare
scannillo
changed the title
| @@ -0,0 +1,7 @@ | |||
| /// Describes a class that batches and sends analytics events. | |||
| /// - Note: Specifically created to mock the `BTAnalyticsService` singleton. | |||
| protocol AnalyticsSendable: AnyObject { | |||
There was a problem hiding this comment.
If you have a better name suggestion for this, please let me know
| @@ -1,8 +1,10 @@ | |||
| import Foundation | |||
|
|
|||
| class BTAnalyticsService: Equatable { | |||
| class BTAnalyticsService: AnalyticsSendable { | |||
There was a problem hiding this comment.
Take it or leave it, but I know for other classes we have had the protocol conformance in an extension so we can add a MARK: to make it clear where the protocol starts/ends.
There was a problem hiding this comment.
We could! Since the protocol is just for testing purposes, I thought that would make the functionality of BTAnalyticsService harder to read - but open to whatever folks prefer!
| weak var analyticsService: BTAnalyticsService? { | ||
| get { BTAPIClient._analyticsService } | ||
| set { BTAPIClient._analyticsService = newValue } | ||
| } |
Co-authored-by: Jax DesMarais-Leder <[email protected]>
Co-authored-by: Jax DesMarais-Leder <[email protected]>
…raintree/braintree_ios into replicate-main-analytics-issue
| startTime: Int? = nil | ||
| ) { | ||
| /// - Parameter event: A single `FPTIBatchData.Event` | ||
| func sendAnalyticsEvent(_ event: FPTIBatchData.Event) { |
There was a problem hiding this comment.
sendAnalyticsEvent and performEventRequest could be merged, or if we decide to keep both, performEventRequest could be made private, thoughts?
There was a problem hiding this comment.
We have those as 2 separate functions for the purpose of unit testing right now. Since sendAnalyticsEvent launches an async background task, there is no way for our tests to know when it completes to run it's assertions. performEventRequest is acting as an "internal" version that delivers the async result, which we're calling in our unit tests.
We used this pattern in PPCP too to unit test background-type tasks. Of course open to future suggestions here for improving our tests!
| let batchMetadata = FPTIBatchData.Metadata( | ||
| authorizationFingerprint: apiClient.authorization.type == .clientToken ? apiClient.authorization.bearer : nil, | ||
| authorizationFingerprint: apiClient?.authorization.type == .clientToken ? apiClient?.authorization.bearer : nil, |
There was a problem hiding this comment.
I think metadata and authorization are constants in BTAPIClient (they are var for testing purposes). In the future, we might consider removing the BTAPIClient dependency from here altogether and instead inject metadata, authorization, and Configuration
There was a problem hiding this comment.
Yeh this is a good callout. Though it did cross my mind going back to this PR (#1337) that we don't need a config to be present for analytics at the time of BTAPIClient.sendAnalytics() but rather right before we flush/batch upload the analytics after the 15 seconds timer.
If we inject the Config from the BTAPIClient, we might get back to calling the fetchOrReturnRemoteConfig more times than necessary
scannillo
force-pushed
the
replicate-main-analytics-issue
branch
from
ed480e3 to
93c68e8
Compare
- Move `BTAnalyticsService` from "faux" singleton setup in `BTAPIClient`, to actual singleton
- Use protocol abstraction to be able to mock BTAnalyticsService in our tests
- Make `RepeatingTimer` and `AnalyticsEventStorage `properties non-static
- Refactor `BTAnalyticsService.sendAnalyticsEvent()` method to require single `FPTIBatchData.Event` property vs 12 individual properties
---------
Signed-off-by: Jax DesMarais-Leder <[email protected]>
Signed-off-by: Rich Herrera <[email protected]>
See ticket DTMOBILES-882 for full details on bug & how to replicate.
Summary
RepeatingTimerinstance from getting wiped if a merchant is creating and destroyingBTAPIClientinstances often (ex: on button click)Changes
BTAnalyticsServicefrom "faux" singleton setup inBTAPIClient, to actual singletonRepeatingTimerandAnalyticsEventStorageproperties non-staticBTAnalyticsService.sendAnalyticsEvent()method to require singleFPTIBatchData.Eventproperty vs 12 individual propertiesChecklist
Authors
@scannillo @richherrera @jaxdesmarais