[Woo POS] Tracking preparaion #11537
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,21 @@ | ||
| package com.woocommerce.android.ui.woopos.checkout | ||
|
|
||
| import androidx.lifecycle.SavedStateHandle | ||
| import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalytics | ||
| import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsTracker | ||
| import com.woocommerce.android.viewmodel.ScopedViewModel | ||
| import dagger.hilt.android.lifecycle.HiltViewModel | ||
| import kotlinx.coroutines.launch | ||
| import javax.inject.Inject | ||
|
|
||
| @HiltViewModel | ||
| class WooPosCheckoutViewModel @Inject constructor( | ||
| private val analyticsTracker: WooPosAnalyticsTracker, | ||
| savedStateHandle: SavedStateHandle, | ||
| ) : ScopedViewModel(savedStateHandle) { | ||
| init { | ||
| launch { | ||
| analyticsTracker.track(WooPosAnalytics.Event.Test) | ||
|
There was a problem hiding this comment. That's for testing. Let's remove that before merging There was a problem hiding this comment. Nice: |
||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| package com.woocommerce.android.ui.woopos.util.analytics | ||
|
|
||
| import com.woocommerce.android.analytics.IAnalyticsEvent | ||
| import kotlin.reflect.KClass | ||
|
|
||
| sealed class WooPosAnalytics : IAnalyticsEvent { | ||
| override val siteless: Boolean = false | ||
| override val isPosEvent: Boolean = true | ||
|
|
||
| private val _properties: MutableMap<String, String> = mutableMapOf() | ||
| val properties: Map<String, String> get() = _properties.toMap() | ||
|
|
||
| fun addProperties(additionalProperties: Map<String, String>) { | ||
| _properties.putAll(additionalProperties) | ||
| } | ||
|
|
||
| sealed class Error : WooPosAnalytics() { | ||
| abstract val errorContext: KClass<Any> | ||
| abstract val errorType: String? | ||
| abstract val errorDescription: String? | ||
|
|
||
| data class Test( | ||
| override val errorContext: KClass<Any>, | ||
| override val errorType: String?, | ||
| override val errorDescription: String?, | ||
| ) : Error() { | ||
| override val name: String = "WOO_POS_TEST_ERROR" | ||
| } | ||
| } | ||
|
|
||
| sealed class Event : WooPosAnalytics() { | ||
| data object Test : Event() { | ||
| override val name: String = "WOO_POS_TEST_EVENT" | ||
| } | ||
| } | ||
| } | ||
|
|
||
| internal fun IAnalyticsEvent.addProperties(additionalProperties: Map<String, String>) { | ||
| when (this) { | ||
| is WooPosAnalytics -> addProperties(additionalProperties) | ||
| else -> error("Cannot add properties to non-WooPosAnalytics event") | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| package com.woocommerce.android.ui.woopos.util.analytics | ||
|
|
||
| import javax.inject.Inject | ||
|
|
||
| class WooPosAnalyticsCommonPropertiesProvider @Inject constructor() { | ||
| val commonProperties: Map<String, String> = mapOf() | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package com.woocommerce.android.ui.woopos.util.analytics | ||
|
|
||
| import com.woocommerce.android.analytics.AnalyticsTrackerWrapper | ||
| import com.woocommerce.android.analytics.IAnalyticsEvent | ||
| import kotlinx.coroutines.Dispatchers | ||
| import kotlinx.coroutines.withContext | ||
| import javax.inject.Inject | ||
|
|
||
| class WooPosAnalyticsTracker @Inject constructor( | ||
| private val analyticsTrackerWrapper: AnalyticsTrackerWrapper, | ||
| private val commonPropertiesProvider: WooPosAnalyticsCommonPropertiesProvider | ||
| ) { | ||
| suspend fun track(analytics: IAnalyticsEvent) { | ||
|
There was a problem hiding this comment. @kidinov sorry for chiming here again, just one question here for my learning, since you worked on making the tracking done on a background thread before (#11293), what's the usefulness of making this And one suggestion, if you don't mind: if this has to be There was a problem hiding this comment. Hi @hichamboushaba 👋
I believe we should build things based on the API, not the current implementation. Eventually, if all goes well we may use another implementation of the tracking itself, which may or may not be implemented in a blocking manner, so having this suspended in the first place is a good idea in my mind
Tracking it's a network call - why Default? There was a problem hiding this comment.
I would have agreed with this if the call was going to an external dependency, but in this case, it goes to our
I don't think we can consider Tracking as a network call, we are just saving the items to an in-memory queue, the networking part is handled using a dedicate Thread in the There was a problem hiding this comment. Further discussion on Slack. |
||
| withContext(Dispatchers.IO) { | ||
| analytics.addProperties(commonPropertiesProvider.commonProperties) | ||
| when (analytics) { | ||
| is WooPosAnalytics.Event -> { | ||
| analyticsTrackerWrapper.track( | ||
| analytics, | ||
| analytics.properties | ||
| ) | ||
| } | ||
|
|
||
| is WooPosAnalytics.Error -> { | ||
| analyticsTrackerWrapper.track( | ||
| analytics, | ||
| analytics.properties, | ||
| analytics.errorContext.simpleName, | ||
| analytics.errorType, | ||
| analytics.errorDescription | ||
| ) | ||
| } | ||
|
|
||
| else -> error("Unknown analytics event type: $analytics") | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| package com.woocommerce.android.ui.woopos.util.analytics | ||
|
|
||
| import com.woocommerce.android.analytics.AnalyticsTrackerWrapper | ||
| import com.woocommerce.android.analytics.IAnalyticsEvent | ||
| import kotlinx.coroutines.test.runTest | ||
| import org.mockito.kotlin.mock | ||
| import org.mockito.kotlin.verify | ||
| import org.mockito.kotlin.whenever | ||
| import kotlin.test.Test | ||
| import kotlin.test.assertFails | ||
|
|
||
| class WooPosAnalyticsTrackerTest { | ||
| private val analyticsTrackerWrapper: AnalyticsTrackerWrapper = mock() | ||
| private val commonPropertiesProvider: WooPosAnalyticsCommonPropertiesProvider = mock() | ||
|
|
||
| val tracker = WooPosAnalyticsTracker( | ||
| analyticsTrackerWrapper, | ||
| commonPropertiesProvider, | ||
| ) | ||
|
|
||
| @Test | ||
| fun `given an event, when track is called, then it should track the event via wrapper`() = runTest { | ||
| // GIVEN | ||
| val event = WooPosAnalytics.Event.Test | ||
|
|
||
| // WHEN | ||
| tracker.track(event) | ||
|
|
||
| // THEN | ||
| verify(analyticsTrackerWrapper).track( | ||
| event, | ||
| event.properties | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given an err, when track is called, then it should track the error via wrapper`() = runTest { | ||
| // GIVEN | ||
| val error = WooPosAnalytics.Error.Test( | ||
| errorContext = Any::class, | ||
| errorType = "test", | ||
| errorDescription = "test", | ||
| ) | ||
|
|
||
| // WHEN | ||
| tracker.track(error) | ||
|
|
||
| // THEN | ||
| verify(analyticsTrackerWrapper).track( | ||
| error, | ||
| error.properties, | ||
| error.errorContext.simpleName, | ||
| error.errorType, | ||
| error.errorDescription | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given an event and common properties, when track is called, then it should track the event with common properties`() = runTest { | ||
| // GIVEN | ||
| val event = WooPosAnalytics.Event.Test | ||
| val commonProperties = mapOf("test" to "test") | ||
| whenever(commonPropertiesProvider.commonProperties).thenReturn(commonProperties) | ||
|
|
||
| // WHEN | ||
| tracker.track(event) | ||
|
|
||
| // THEN | ||
| verify(analyticsTrackerWrapper).track( | ||
| event, | ||
| event.properties + commonProperties | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given an error and common properties, when track is called, then it should track the event with common properties`() = runTest { | ||
| // GIVEN | ||
| val error = WooPosAnalytics.Error.Test( | ||
| errorContext = Any::class, | ||
| errorType = "test", | ||
| errorDescription = "test", | ||
| ) | ||
| val commonProperties = mapOf("test" to "test") | ||
| whenever(commonPropertiesProvider.commonProperties).thenReturn(commonProperties) | ||
|
|
||
| // WHEN | ||
| tracker.track(error) | ||
|
|
||
| // THEN | ||
| verify(analyticsTrackerWrapper).track( | ||
| error, | ||
| error.properties + commonProperties, | ||
| error.errorContext.simpleName, | ||
| error.errorType, | ||
| error.errorDescription | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given an non woopos event, when track is called, then it throw an exception`() = runTest { | ||
| // GIVEN | ||
| val event = object : IAnalyticsEvent { | ||
| override val name: String = "test" | ||
| override val siteless: Boolean = false | ||
| override val isPosEvent: Boolean = false | ||
| } | ||
|
|
||
| // WHEN && THEN | ||
| assertFails { tracker.track(event) } | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@backwardstruck @samiuelson
Shell we actually reuse
ScopedViewModel? That couples us to the main code but gives some reduction in the complexity of the POS. wdyt?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.
The understanding I have is that we no longer have to worry as much about keeping things decoupled. Rather we want what is faster, as long as it's still correct. Then we would commit to spending the additional time later, after this stage of the POS is done, to go back and decouple. So it should be fine to keep 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.
@backwardstruck It's quite interesting, but I understand it differently. Even though we've chosen not to use modularization, we still need to build it in a decoupled way. This means we are incorporating the unfavorable aspects of both approaches, but that's my understanding 🤔
@samiuelson what's your take on this? =)
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 the real question is if we want to reuse the
Eventclass (for navigation), and the event triggering mechanism, theScopedViewModelintroduces. Other parts like theCoroutineScopeinterface, are not useful, IMO. Unless we realize we need it, I'd suggest getting rid of another layer of inheritance and extending the ViewModel class directly.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 also dislike the idea of
ScopedViewModel"being" aCoroutineScopeitself – it's not very helpful and makes code harder to debug and read.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.
@samiuelson @backwardstruck
I create an issue on getting rid
ScopedViewModeland the events from the main app. We should probably take this one sooner while we still have 1-2 places where it is used#11622