-
Notifications
You must be signed in to change notification settings - Fork 114
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
[Woo POS] MVP order, cart, and miscellaneous events #15047
Conversation
Generated by 🚫 Danger |
|
We already track this event by checking the tap on the support button via “pos_get_support_tapped”. But “support_new_request_viewed” will get decorated with “pos_” as we do not exit POS mode to send the support request.
Declaring WooAnalytics as dependency at file level makes the test suite crash when ran as a whole but not independently test by test. This is resolved by injecting the dependency at the test level, most likely a threading issue?
...ommerce/Classes/POS/Presentation/CardReaderConnection/UI States/PointOfSaleLoadingView.swift
Outdated
Show resolved
Hide resolved
WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleOrderControllerTests.swift
Outdated
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.
Thanks for tackling all the POS analytics! I took some time to catch up on the previous analytics changes during AFK. I left some comments/questions, most of the testing worked well with two issues below.
✅
- pos_loaded (prop milliseconds_time_elapsed_in_splash_screen)
- pos_simple_products_explanation_dialog_shown
- get_support_tapped
- pos_products_pull_to_refresh
- pos_variations_pull_to_refresh
- pos_exit_pos_menu_item_tapped
- pos_exit_pos_confirmed
- pos_checkout_tapped with
items_in_cart
prop - pos_order_creation_success
- pos_email_receipt_tapped
- pos_email_receipt_send_tapped
- pos_create_new_order_tapped
- pos_item_removed_from_cart
- pos_back_to_cart_tapped
Issues from testing:
- If I turn off the internet after adding items to cart and then tap “Check out,” pos_order_creation_failed was tracked twice
- In the simple products dialog: when I tapped “Create an order in store management” in the dialog and the orders tab was shown,
pos_orders_add_new
got logged unexpectedly (shouldn’t have thepos
prefix).- After that, I tried going back to the Menu tab but tapping on POS entry point was no-op (
hub_menu_option_tapped
withoption: pointOfSale
was logged but POS wasn’t shown). However, I couldn’t reproduce this.
- After that, I tried going back to the Menu tab but tapping on POS entry point was no-op (
Other tracking events I saw:
- pos_item_added_to_cart with
product_type
prop - pos_card_reader_connection_success
- pos_card_present_collect_payment_canceled
- pos_card_present_collect_payment_success
Questions:
- I didn’t see an event tracked when tapping “Connect your reader.” Is this something we want to track, maybe also with the connection state?
- Similarly, the “Connect to reader” CTA in the totals screen.
- Do we want to track the CTA tap to clear the cart?
- Do we want to track the cash payment flow? Like tapping to pay with cash and completion etc.
- Do we want to track when the success screen is shown?
WooCommerce/Classes/POS/Presentation/Item Selector/ChildItemList.swift
Outdated
Show resolved
Hide resolved
...ommerce/Classes/POS/Presentation/CardReaderConnection/UI States/PointOfSaleLoadingView.swift
Show resolved
Hide resolved
...ommerce/Classes/POS/Presentation/CardReaderConnection/UI States/PointOfSaleLoadingView.swift
Outdated
Show resolved
Hide resolved
WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleOrderControllerTests.swift
Outdated
Show resolved
Hide resolved
# Conflicts: # WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift
Thanks for the review @jaclync , this is ready for another round when you have the time. I believe I answered all items below, let me know if I missed something:
I cannot replicate this one. By using the Network Link Conditioner with a 100% loss profile as well as disconnecting WIFI manually I seem to track this once only:
If I switch the connection back on before the
Good catch, added to the list here: 3f6ae93
I couldn't neither :/
Ahhh this one went missing! As is not tracked through the card reader service . I've added both events and tests here: a4152d0
We do, I missed this one or I was waiting for the rest of cart events. I've added it here: f4509cc
Yes, we do, but the events are TBD for now. These are logged here: #15057
I'm not sure, the cash or card succeed might be enough. I'll ask in the P2.
That's a good question: By backgrounding the app or switching and then back I cannot see the Moving the event to, for example, |
} | ||
|
||
private enum Keys { | ||
static let waitingTime = "waiting_time" | ||
static let milliseconds_time_elapsed = "milliseconds_time_elapsed_in_splash_screen" |
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.
nit: as the value is pretty specific to "splash screen", probably want to name the key accordingly. Also would be great to keep it camel case.
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.
True that, updated: 47924ea
@@ -21,6 +41,9 @@ private extension PointOfSaleLoadingView { | |||
static let textSpacing: CGFloat = 16 | |||
static let progressViewSpacing: CGFloat = 72 | |||
} | |||
enum Constants { | |||
static let toMilliseconds: Double = 1000.0 |
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.
nit: this doesn't seem needed anymore.
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.
Updated! 9c57d76
Actually, I just encountered this in the first run of PR testing again today. I did some debugging, but couldn't figure out the cause in the time I had: the ![]() I also recalled the order creation form not being shown in the orders tab. It might be some UI issue related to race conditions. |
Ah never mind, the multiple |
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 for the updates! LGTM once the ❕ comment is addressed.
WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleOrderControllerTests.swift
Show resolved
Hide resolved
@@ -2504,6 +2506,8 @@ extension WooAnalyticsEvent { | |||
return WooAnalyticsEvent(statName: .analyticsHubWaitingTimeLoaded, properties: [Keys.waitingTime: elapsedTime]) | |||
case .appStartup: | |||
return WooAnalyticsEvent(statName: .applicationOpenedWaitingTimeLoaded, properties: [Keys.waitingTime: elapsedTime]) | |||
case .pointOfSaleLoaded: | |||
return WooAnalyticsEvent(statName: .pointOfSaleLoaded, properties: [Keys.milliseconds_time_elapsed: elapsedTime]) |
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.
❕ During testing it looks like the prop is recording the time in seconds, whereas it's expected to be milliseconds:
🔵 Tracked pos_pos_loaded, properties: [milliseconds_time_elapsed_in_splash_screen: 2.351064920425415, ...]
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 that's not the case, is it? I might be reading this wrongly but when we log the event on WaitingTimeTracker we log the elapsed time, so in this case we're still capturing it in milliseconds right? So 1.9ms in this instance, which we pass to the event:
po currentTimeInMillis()
> 1738832706.298878
po waitingStartedTimestamp
> 1738832690.2819052
po elapsedTime (as currentTimeInMillis() - waitingStartedTimestamp)
> 1.9627389907836914
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.
Actually the calculation seems off too 1738832706.298878 - 1738832690.2819052 it's not what we get but ~16, checking as I have just used the WaitingTimeTracker that was already there 👀
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 4d49861 I've updated the interface of WaitingTimeTracker as there was a very miss-leading property in the initializer: currentTimeInMillis
captures the time in seconds, not in milliseconds.
I've added a new endInMilliseconds()
method just for our use case, so I avoid touching the current implementation much, and now captures the elapsed time properly:
🔵 Tracked pos_pos_loaded, properties: [is_wpcom_store: false, was_ecommerce_trial: false, milliseconds_time_elapsed_in_splash_screen: 2172.056198120117, site_url: https://indiemelon.mystagingwebsite.com, plan: , store_id: c5bd46cc-1804-4f7b-badb-bb98c449127f, blog_id: -1]
# Conflicts: # WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift
Thanks again for the review! I just have a question regarding the milliseconds tracking here.
Yes, that's the one. It also seems we're removing the |
Closes: #13613
Closes: #15018
Description
This PR adds the POS track events discussed on pdfdoF-6hn-p2 for MVP. Final events and properties are still TBD, so most likely we'll need to update some before launch. Events are not validated yet, I'll do that when we have a final decision on all events and their properties, as its very time consuming.
You should see all events decorated with
pos_*
in the console, and aswoocommerceios_pos_*
in Track events.I've opted to call analytics from ServiceLocator in most views. I'm sure we can inject some of them directly or through the viewModel but didn't seem worthy just for a single event or two. Happy to iterate on this as needed.
Testing
Go through POS performing different actions like adding, removing items from cart, checkout, create order, pay by cash or card, send receipt, exit POS, get support, etc, ... I've tested all of them manually, and the ones where we inject analytics have unit tests as well, so feel free to go through all of them or just some.
RELEASE-NOTES.txt
if necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: