-
Notifications
You must be signed in to change notification settings - Fork 121
Add tracks for Customer Search #7907
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
You can test the changes from this Pull Request by:
|
# Conflicts: # WooCommerce/Classes/ViewRelated/Orders/Order Details/Address Edit/EditOrderAddressForm.swift
f2d7276 to
6670176
Compare
Generated by 🚫 dangerJS |
|
Added unit tests for
At the moment we do not track any property in any of these two, so we can iterate as needed if we decide we'll pass something with tracks in the future. Ready for review @joshheald! 👍 |
joshheald
left a comment
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.
Looks good, and tests well! A few minor suggestions.
Also, registering tracks events is covered here: PCYsg-mUj-p2
| case .orderCreationSearchCustomers: | ||
| return buildConfig == .localDeveloper || buildConfig == .alpha |
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 checked a release build, and the feature is enabled there 👍
| /// Enables the Search Customers functionality in the Order Creation screen | ||
| /// | ||
| case orderCreationSearchCustomers | ||
|
|
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.
Well remembered
| /// Fills Order AddressFormFields with Customer details | ||
| /// | ||
| func customerSelectedFromSearch(customer: Customer) { | ||
| analytics.analyticsProvider.track(WooAnalyticsStat.orderCreationCustomerAdded.rawValue) |
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.
| analytics.analyticsProvider.track(WooAnalyticsStat.orderCreationCustomerAdded.rawValue) | |
| analytics.track(.orderCreationCustomerAdded) |
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.
Fixed here: 6a326f7
| viewModel.customerSelectedFromSearch(customer: customer) | ||
|
|
||
| // Then | ||
| assertEqual(analyticsProvider.receivedEvents, [WooAnalyticsStat.orderCreationCustomerAdded.rawValue]) |
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.
This works fine, but is a little brittle.
It would be nice if tracking an extra analytics event in the course of selecting a customer from search didn't make this test fail, even though the expected event is still logged
You could see if there's anything you can do to make it a little more future proof?
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 didn't thought about it, if some other event sneaks in while we're waiting from our customerSelectedFromSearch() to come back, the test would fail indeed. I changed the test to confirm if receivedEvents contains it instead.
// Then
- assertEqual(analyticsProvider.receivedEvents, "order_creation_customer_added")
+ XCTAssert(analyticsProvider.receivedEvents.contains("order_creation_customer_added"))
Fixed here: 169094d
| guard let eventIndex = analyticsProvider.receivedEvents.firstIndex(of: "order_creation_customer_search") else { | ||
| return XCTFail("Analytics not logged") | ||
| } | ||
| XCTAssertNotNil(eventIndex) |
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.
This test works well 👍 but this XCTAssertNotNil can't fail. It's a little more expressive like this:
| guard let eventIndex = analyticsProvider.receivedEvents.firstIndex(of: "order_creation_customer_search") else { | |
| return XCTFail("Analytics not logged") | |
| } | |
| XCTAssertNotNil(eventIndex) | |
| XCTAssert(analyticsProvider.receivedEvents.contains("order_creation_customer_search")) |
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.
Oh! That's true, if passes the guard, it would never fail. Thanks for the suggestion! Changed at 2be5053
Thanks for the link, I was unaware of the registration process. It would seem the Android counterpart is not registered either so I'll take care of those as well 👍 |
Closes: #7741
Description
This PR:
The Android tracks counterpart uses the same names:
order_creation_customer_search: Triggered each time a new query is performed.order_creation_customer_added: Triggered when a Customer is selected.Testing instructions