Skip to content

Conversation

@iamgabrielma
Copy link
Contributor

@iamgabrielma iamgabrielma commented Oct 24, 2022

Closes: #7741

Description

This PR:

  1. Adds tracking to the Customer Search project.
  2. Removes the feature flag.

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

  1. Go to Orders > + > Create new Order > + Add Customer details > Tap on the magnifying glass icon:
  2. Search for a Customer name
  3. Confirm that once the search completes, the following is logged in the console:
🔵 Tracked order_creation_customer_search, properties: [AnyHashable("is_wpcom_store"): false, AnyHashable("blog_id"): 121710041]
  1. Tap on it:
  2. Confirm that once tapped, the following is logged in the console:
🔵 Tracked order_creation_customer_added, properties: [AnyHashable("is_wpcom_store"): false, AnyHashable("blog_id"): 121710041]
  • The PR must be assigned the Tracks label. Not applicable.
  • The tracks events must be validated in the Tracks system.

Screenshot 2022-11-01 at 13 31 52

  • Verify the internal tracks spreadsheet has also been updated.

@iamgabrielma iamgabrielma added type: enhancement A request for an enhancement. status: do not merge Dependent on another PR, ready for review but not ready for merge. category: tracks Related to analytics, including Tracks Events. category: parity Match what's supported by the other platform. feature: order creation All tasks related to creating an order labels Oct 24, 2022
@iamgabrielma iamgabrielma added this to the 11.0 milestone Oct 24, 2022
@iamgabrielma iamgabrielma mentioned this pull request Oct 24, 2022
15 tasks
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 24, 2022

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr7907-169094d on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@iamgabrielma iamgabrielma added status: draft This is a draft, still need more work but can be reviewed and commented if asked. and removed status: do not merge Dependent on another PR, ready for review but not ready for merge. labels Oct 24, 2022
Base automatically changed from issue/741-implement-customer-search-ui to trunk October 26, 2022 07:45
@iamgabrielma iamgabrielma modified the milestones: 11.0, 11.1 Oct 28, 2022
@iamgabrielma iamgabrielma changed the base branch from trunk to issue/7741-fill-order-data-from-customer-search October 28, 2022 05:49
Base automatically changed from issue/7741-fill-order-data-from-customer-search to trunk November 1, 2022 03:01
# Conflicts:
#	WooCommerce/Classes/ViewRelated/Orders/Order Details/Address Edit/EditOrderAddressForm.swift
@iamgabrielma iamgabrielma force-pushed the issue/7741-add-customer-search-tracks branch from f2d7276 to 6670176 Compare November 1, 2022 04:19
@iamgabrielma iamgabrielma marked this pull request as ready for review November 1, 2022 04:19
@iamgabrielma iamgabrielma removed the status: draft This is a draft, still need more work but can be reviewed and commented if asked. label Nov 1, 2022
@peril-woocommerce
Copy link

peril-woocommerce bot commented Nov 1, 2022

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@iamgabrielma
Copy link
Contributor Author

Added unit tests for orderCreationCustomerAdded and orderCreationCustomerSearch track events on 1ed53af and f20e20f respectively.

order_creation_customer_added shows now without properties since I moved the tracking to the existing viewModel, and used its existing declaration of Analytics, as well as for coherence with all other track tests on EditOrderAddressFormViewModelTests.

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 joshheald self-assigned this Nov 2, 2022
Copy link
Contributor

@joshheald joshheald left a 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

Comment on lines -36 to -37
case .orderCreationSearchCustomers:
return buildConfig == .localDeveloper || buildConfig == .alpha
Copy link
Contributor

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 👍

Comment on lines -73 to -76
/// Enables the Search Customers functionality in the Order Creation screen
///
case orderCreationSearchCustomers

Copy link
Contributor

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
analytics.analyticsProvider.track(WooAnalyticsStat.orderCreationCustomerAdded.rawValue)
analytics.track(.orderCreationCustomerAdded)

Copy link
Contributor Author

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])
Copy link
Contributor

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?

Copy link
Contributor Author

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

Comment on lines 62 to 65
guard let eventIndex = analyticsProvider.receivedEvents.firstIndex(of: "order_creation_customer_search") else {
return XCTFail("Analytics not logged")
}
XCTAssertNotNil(eventIndex)
Copy link
Contributor

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:

Suggested change
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"))

Copy link
Contributor Author

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

@iamgabrielma
Copy link
Contributor Author

registering tracks events is covered here: PCYsg-mUj-p2

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 👍

@iamgabrielma iamgabrielma merged commit 30bc4e9 into trunk Nov 3, 2022
@iamgabrielma iamgabrielma deleted the issue/7741-add-customer-search-tracks branch November 3, 2022 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: parity Match what's supported by the other platform. category: tracks Related to analytics, including Tracks Events. feature: order creation All tasks related to creating an order type: enhancement A request for an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Order creation: Search Customers

4 participants