[Woo POS] Tracking preparaion #11537

kidinov · 2024-05-17T14:23:14Z

Closes: #11517

Description

The PR:

Decouple current tracking logic from the POS, but reusing that
Adds woocommerceandroid_pos_ to POS events
Makes tracking more safe and enforces us to do it without blocking the UI

Testing instructions

Use the app and notice that there are no changes in tracking for the current screens
Use the POS screen and navigate to the checkout screen -> notice test event tracked with new prefix

Use tracks live view to do so

Images/gif

I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

dangermattic · 2024-05-17T14:26:24Z

	2 Warnings
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	This PR is assigned to the milestone 18.9. This milestone is due in less than 2 days. Please make sure to get it merged by then or assign it to a milestone with a later deadline.

	1 Message
📖	This PR contains changes to Tracks-related logic. Please ensure (author and reviewer) the following are completed: The tracks events must be validated in the Tracks system. Verify the internal Tracks spreadsheet has also been updated. Please consider registering any new events. The PR must be assigned the category: tracks label.

Generated by 🚫 Danger

wpmobilebot · 2024-05-17T14:41:56Z

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`efd04a8`
	Direct Download	`woocommerce-prototype-build-pr11537-efd04a8.apk`

codecov-commenter · 2024-05-17T14:58:15Z

Codecov Report

Attention: Patch coverage is 38.52097% with 557 lines in your changes are missing coverage. Please review.

Project coverage is 39.39%. Comparing base (50cd183) to head (efd04a8).

Files	Patch %	Lines
...om/woocommerce/android/analytics/AnalyticsEvent.kt	36.54%	540 Missing ⚠️
...roid/ui/woopos/checkout/WooPosCheckoutViewModel.kt	0.00%	7 Missing ⚠️
...ui/woopos/util/analytics/WooPosAnalyticsTracker.kt	80.00%	2 Missing and 2 partials ⚠️
.../woocommerce/android/analytics/AnalyticsTracker.kt	25.00%	3 Missing ⚠️
...alytics/WooPosAnalyticsCommonPropertiesProvider.kt	0.00%	2 Missing ⚠️
...ndroid/ui/woopos/util/analytics/WooPosAnalytics.kt	95.23%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #11537      +/-   ##
============================================
- Coverage     40.22%   39.39%   -0.83%     
- Complexity     5206     5210       +4     
============================================
  Files          1089     1092       +3     
  Lines         63521    63573      +52     
  Branches       8723     8727       +4     
============================================
- Hits          25549    25047     -502     
- Misses        35665    36217     +552     
- Partials       2307     2309       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kidinov · 2024-05-21T11:08:39Z

WooCommerce/src/main/kotlin/com/woocommerce/android/analytics/AnalyticsEvent.kt

@@ -1,1057 +1,1066 @@
 package com.woocommerce.android.analytics

-enum class AnalyticsEvent(val siteless: Boolean = false) {
+interface IAnalyticsEvent {


I'd want to avoid doing so, but with current enum usage I didn't find a way to reuse current AnalyticsTrackers*

I'm also not aware of a better option so it seems worth it to make this change. Should be a good foundation as POS expands.

kidinov · 2024-05-21T11:09:33Z

...mmerce/src/main/kotlin/com/woocommerce/android/ui/woopos/checkout/WooPosCheckoutViewModel.kt

+) : ScopedViewModel(savedStateHandle) {
+    init {
+        launch {
+            analyticsTracker.track(WooPosAnalytics.Event.Test)


That's for testing. Let's remove that before merging

Nice:

2024-05-21 16:04:02.385 8173-8684 WooCommerce-UTILS com.woocommerce.android.dev I 🔵 Tracked: woo_pos_test_event, Properties: {"blog_id":227901557,"is_wpcom_store":true,"was_ecommerce_trial":true,"plan_product_slug":"business-bundle","store_id":"79059489-918b-4632-9fee-465239092a2e","is_debug":true,"site_url":"https:\/\/woo-totally-selfless-bear.wpcomstaging.com","cached_woo_core_version":"8.9.1"}

kidinov · 2024-05-21T11:13:11Z

...mmerce/src/main/kotlin/com/woocommerce/android/ui/woopos/checkout/WooPosCheckoutViewModel.kt

    savedStateHandle: SavedStateHandle,
-) : ScopedViewModel(savedStateHandle)
+) : ScopedViewModel(savedStateHandle) {


@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?

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.

@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? =)

I think the real question is if we want to reuse the Event class (for navigation), and the event triggering mechanism, the ScopedViewModel introduces. Other parts like the CoroutineScope interface, 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.

I also dislike the idea of ScopedViewModel "being" a CoroutineScope itself – it's not very helpful and makes code harder to debug and read.

@samiuelson @backwardstruck

I create an issue on getting rid ScopedViewModel and 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

backwardstruck

Code changes look good. I see the event on Tracks live as well. Feel free to merge once other feedback is in and the test event is removed:

# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/analytics/AnalyticsEvent.kt

samiuelson

I tested the new implementation and existing analytics against regressions. LGTM!

hichamboushaba · 2024-06-07T07:49:42Z

...e/src/main/kotlin/com/woocommerce/android/ui/woopos/util/analytics/WooPosAnalyticsTracker.kt

+    private val analyticsTrackerWrapper: AnalyticsTrackerWrapper,
+    private val commonPropertiesProvider: WooPosAnalyticsCommonPropertiesProvider
+) {
+    suspend fun track(analytics: IAnalyticsEvent) {


@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 suspendable?

And one suggestion, if you don't mind: if this has to be suspendable, I think a more appropriate dispatcher to use would be Dispatchers.Default, Dispatchers.IO is destined more for blocking IO operations, which is not the case here, WDYT?

Hi @hichamboushaba 👋

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 suspendable?

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

I think a more appropriate dispatcher to use would be Dispatchers.Default, Dispatchers.IO is destined more for blocking IO operations, which is not the case here

Tracking it's a network call - why Default?

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

I would have agreed with this if the call was going to an external dependency, but in this case, it goes to our AnalyticsTracker class, which internally keeps a queue and handles the events, then these events are also emitted to another queue in Tracks library (which is also our internal library), I feel like we are overengineering this part 🙂, and with the complexity, we increase the change of any other issues:

Ordering (that we discussed on the other PR), I think it would be a major issue here given the parallelism involved with using Dispatchers.IO.

Non-accurate timestamps.

Other stuff that we didn't think of.

Tracking it's a network call - why Default?

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 Tracks library.

Further discussion on Slack.

kidinov added 7 commits May 16, 2024 17:16

Tinkering about how to wrap and abstract analytics

953149e

Merge branch 'trunk' into 11517-woo-pos-prepare-tracking-with-pos-prefix

b7f5295

Implemented track method

7ffb17b

Fixed formatting

cc3e97c

Track properties for the error events

c91f45b

Common properties

abcb1aa

Code to test the new wrapper

8e15d00

kidinov linked an issue May 17, 2024 that may be closed by this pull request

[Woo POS] Prepare tracking with POS prefix #11517

Closed

kidinov marked this pull request as draft May 17, 2024 14:27

kidinov added 2 commits May 21, 2024 12:49

POS event prefix

8710b70

Added a test for the new tracker

1cfc9a3

kidinov added the category: tracks Related to analytics, including Tracks Events. label May 21, 2024

kidinov added this to the 18.8 milestone May 21, 2024

kidinov commented May 21, 2024

View reviewed changes

Two more test to test common props

9dc4dac

kidinov commented May 21, 2024

View reviewed changes

kidinov marked this pull request as ready for review May 21, 2024 11:13

kidinov requested review from backwardstruck and samiuelson May 21, 2024 11:13

backwardstruck self-assigned this May 21, 2024

backwardstruck approved these changes May 21, 2024

View reviewed changes

backwardstruck modified the milestones: 18.8, 18.9 May 24, 2024

Merge branch 'trunk' into 11517-woo-pos-prepare-tracking-with-pos-prefix

efd04a8

# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/analytics/AnalyticsEvent.kt

samiuelson self-assigned this May 29, 2024

samiuelson approved these changes May 29, 2024

View reviewed changes

kidinov merged commit 9f3dd6e into trunk May 30, 2024
14 checks passed

kidinov deleted the 11517-woo-pos-prepare-tracking-with-pos-prefix branch May 30, 2024 10:01

hichamboushaba reviewed Jun 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Woo POS] Tracking preparaion #11537

[Woo POS] Tracking preparaion #11537

kidinov commented May 17, 2024 •

edited

Loading

dangermattic commented May 17, 2024 •

edited

Loading

wpmobilebot commented May 17, 2024 •

edited

Loading

codecov-commenter commented May 17, 2024 •

edited

Loading

kidinov May 21, 2024

backwardstruck May 21, 2024

kidinov May 21, 2024

backwardstruck May 21, 2024

kidinov May 21, 2024

backwardstruck May 21, 2024

kidinov May 29, 2024

samiuelson May 29, 2024

samiuelson May 29, 2024

kidinov May 30, 2024

backwardstruck left a comment

samiuelson left a comment

hichamboushaba Jun 7, 2024

kidinov Jun 7, 2024

hichamboushaba Jun 7, 2024

backwardstruck Jul 4, 2024

[Woo POS] Tracking preparaion #11537

[Woo POS] Tracking preparaion #11537

Conversation

kidinov commented May 17, 2024 • edited Loading

Description

Testing instructions

Images/gif

dangermattic commented May 17, 2024 • edited Loading

wpmobilebot commented May 17, 2024 • edited Loading

codecov-commenter commented May 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

backwardstruck left a comment

Choose a reason for hiding this comment

samiuelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidinov commented May 17, 2024 •

edited

Loading

dangermattic commented May 17, 2024 •

edited

Loading

wpmobilebot commented May 17, 2024 •

edited

Loading

codecov-commenter commented May 17, 2024 •

edited

Loading