-
Notifications
You must be signed in to change notification settings - Fork 136
[Woo POS][Historical Orders] Order Details: Analytics #14863
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
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14863 +/- ##
=========================================
Coverage 38.26% 38.26%
- Complexity 10085 10087 +2
=========================================
Files 2131 2131
Lines 120708 120793 +85
Branches 16537 16541 +4
=========================================
+ Hits 46187 46225 +38
- Misses 69825 69870 +45
- Partials 4696 4698 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thanks @toupper ! Overal looks good to me, but I'd propose a few changes with the main goal to keep it simplier
Implement_WooPosOrdersAnalyticsTracker_for_enhanced_order_analytics_tracking.patch
Proposed Changes: Extract Tracking Logic from WooPosOrdersViewModel
- Add tracking fields to OrderItemViewState, to avoid separate "caching"
// WooPosOrdersState.kt
data class OrderItemViewState(
...
val statusSlug: String,
val createdAtMillis: Long
)
- Create WooPosOrdersAnalyticsTracker class
New file with all tracking methods as suspend functions:
- trackOrdersListFetched(elapsedMs)
- trackOrdersListRowTapped(orderId, orderStatus, listPosition, createdAtMillis)
- trackOrderDetailsLoaded(orderId, orderStatus, createdAtMillis)
- etc.
- Remove from ViewModel:
- OrderTrackingMeta data class
- trackingMeta mutable map
- cacheOrderTrackingMeta() method
- retrieveOrderTrackingData() helper
- trackOrderDetailsShown() and trackOrdersListRowTapped() private methods
- 9 analytics event imports + MILLIS_PER_DAY
- Update ViewModel:
- Inject WooPosOrdersAnalyticsTracker instead of WooPosAnalyticsTracker
- Replace all direct analyticsTracker.track() calls with ordersAnalyticsTracker.track*() in viewModelScope.launch
- Track OrderDetailsLoaded directly in onOrderSelected() (no separate UI callback needed)
Why This Is Better:
- Separation of concerns: Tracking logic separated from state management
- No mutable cache: Tracking data lives naturally in the view state
- Cleaner: Removes ~60 lines from ViewModel
- Testable: Analytics logic can be tested in isolation
- Maintainable: Changes to tracking don't touch ViewModel and don't make it even bigger
Wdyt?
|
Thanks for the patch @kidinov! I added it here a8dec03 One question — as in the patch, I initially added the tracking data into the state, but after some research, I found that it’s generally better to keep only what’s strictly needed for the UI there. In this case, memory usage would be the same since we store it in another structure, but keeping it out of the state avoids unnecessary recompositions and keeps the state cleaner and more focused on UI data. What do you think — do we have a convention for handling this kind of case? |
|
@kidinov Also about this one
If we couple the |
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.
In this case, memory usage would be the same since we store it in another structure, but keeping it out of the state avoids unnecessary recompositions and keeps the state cleaner and more focused on UI data. What do you think — do we have a convention for handling this kind of case?
That's all correct, except in this case you do the changes only when the state that changes the UI changes as well, so no extra recompositions unless I am missing something. All this should be handled by having all the orders we show in cache, and then we would take the order data lazily, when it's needed to be tracked, from the cache. I hope we'll implement this in I2 of this project alongside the replacement of the refetching of the order after email sent and/or refund made.
If we couple the OrderDetailsLoaded event to onOrderSelected, we’ll miss the initial auto-selected case (first item on load/refresh), and we’re not matching the event’s intent. Selection is an intention; loading details is an outcome. A bug could select an order without actually rendering/loading its details. I know that second case shouldn’t happen with our current flow, but it’s still a leaky proxy. What do you think?
Yep, you are right. My intention was to simplify it by not passing it via the UI layer, but indeed it's not ideal. The workaround, which I don't like but avoids having this round trip, is here. So apparently no ideal solution here - up to you
Track_order_details_loading_in_WooPosOrdersViewModel.patch
Also, I noticed that we now track pos_orders_list_fetched and pos_orders_list_search_results_fetched on success, but as peer description it's about request completion - check the patch out
Track_elapsed_time_for_orders_list_search_and_fetch_operations.patch
Other than that, LGTM!
WOOMOB-1155
Description
Analytics events: pdfdoF-7XF-p2
orders_menu_item_tapped- Orders menu item tapped (no properties)orders_list_loaded- Orders view loaded (no properties)orders_list_pull_to_refresh- Pull to refresh triggered (no properties)orders_list_fetched- Orders fetched completion withmilliseconds_since_request_sentorders_list_next_page_loaded- Next page loaded withpage_numberorders_list_row_tapped- Order row tapped withorder_id,order_status,list_position(0 for topmost),days_since_created(0 for today, 1 for yesterday)orders_list_search_button_tapped- Search button tapped (no properties)orders_list_search_results_fetched- Search results fetched completion withmilliseconds_since_request_sentorder_details_loaded- Order details loaded withorder_id,order_status,days_since_created(0 for today, 1 for yesterday)order_details_email_receipt_tapped- Email receipt tapped (no properties)Events registering are coming.
Steps to reproduce
RELEASE-NOTES.txtif necessary.