Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions Experiments/Experiments/DefaultFeatureFlagService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ public struct DefaultFeatureFlagService: FeatureFlagService {
return true
case .searchProductsBySKU:
return true
case .orderCreationSearchCustomers:
return buildConfig == .localDeveloper || buildConfig == .alpha
Comment on lines -36 to -37
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 👍

case .wpcomSignup:
guard isFeatureFlagEnabled(.simplifiedLoginFlowI1) else {
return buildConfig == .localDeveloper || buildConfig == .alpha
Expand Down
4 changes: 0 additions & 4 deletions Experiments/Experiments/FeatureFlag.swift
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,6 @@ public enum FeatureFlag: Int {
///
case searchProductsBySKU

/// Enables the Search Customers functionality in the Order Creation screen
///
case orderCreationSearchCustomers

Comment on lines -73 to -76
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

/// Enables signing up for a WP.com account.
///
case wpcomSignup
Expand Down
2 changes: 1 addition & 1 deletion RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

11.1
-----

- [**] You can now search customers when creating or editing an order. [https://github.com/woocommerce/woocommerce-ios/issues/7741]

11.0
-----
Expand Down
2 changes: 2 additions & 0 deletions WooCommerce/Classes/Analytics/WooAnalyticsStat.swift
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,8 @@ public enum WooAnalyticsStat: String {
case orderCreateButtonTapped = "order_create_button_tapped"
case orderCreationSuccess = "order_creation_success"
case orderCreationFailed = "order_creation_failed"
case orderCreationCustomerAdded = "order_creation_customer_added"
case orderCreationCustomerSearch = "order_creation_customer_search"
case orderContactAction = "order_contact_action"
case orderCustomerAdd = "order_customer_add"
case orderEditButtonTapped = "order_edit_button_tapped"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,13 @@ final class CustomerSearchUICommand: SearchUICommand {

private let siteID: Int64

init(siteID: Int64, onDidSelectSearchResult: @escaping ((Customer) -> Void)) {
private let analytics: Analytics

init(siteID: Int64,
analytics: Analytics = ServiceLocator.analytics,
onDidSelectSearchResult: @escaping ((Customer) -> Void)) {
self.siteID = siteID
self.analytics = analytics
self.onDidSelectSearchResult = onDidSelectSearchResult
}

Expand Down Expand Up @@ -59,6 +64,7 @@ final class CustomerSearchUICommand: SearchUICommand {
}

func synchronizeModels(siteID: Int64, keyword: String, pageNumber: Int, pageSize: Int, onCompletion: ((Bool) -> Void)?) {
analytics.track(.orderCreationCustomerSearch)
let action = CustomerAction.searchCustomers(siteID: siteID, keyword: keyword) { result in
switch result {
case .success(_):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,7 @@ open class AddressFormViewModel: ObservableObject {
/// 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

fillCustomerFields(customer: customer)
let addressesDiffer = customer.billing != customer.shipping
showDifferentAddressForm = addressesDiffer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,6 @@ struct EditOrderAddressForm<ViewModel: AddressFormViewModelProtocol>: View {
@Environment(\.safeAreaInsets) var safeAreaInsets: EdgeInsets
@State private var showingCustomerSearch: Bool = false

let isSearchCustomersEnabled = DefaultFeatureFlagService().isFeatureFlagEnabled(.orderCreationSearchCustomers)

var body: some View {
Group {
ScrollView {
Expand Down Expand Up @@ -151,13 +149,11 @@ struct EditOrderAddressForm<ViewModel: AddressFormViewModelProtocol>: View {
})
}
ToolbarItem(placement: .automatic) {
if isSearchCustomersEnabled {
Button(action: {
showingCustomerSearch = true
}, label: {
Image(systemName: "magnifyingglass")
})
}
Button(action: {
showingCustomerSearch = true
}, label: {
Image(systemName: "magnifyingglass")
})
}
ToolbarItem(placement: .confirmationAction) {
navigationBarTrailingItem()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,25 @@ final class EditOrderAddressFormViewModelTests: XCTestCase {
assertEqual(analyticsProvider.receivedProperties.first?["subject"] as? String, "billing_address")
}

func test_view_model_when_customerSelectedFromSearch_then_tracks_orderCreationCustomerAdded() {
// Given
let analyticsProvider = MockAnalyticsProvider()
let viewModel = EditOrderAddressFormViewModel(order: Order.fake(), type: .billing, analytics: WooAnalytics(analyticsProvider: analyticsProvider))
let customer = Customer.fake().copy(
email: "[email protected]",
firstName: "Johnny",
lastName: "Appleseed",
billing: sampleAddressWithEmptyNullableFields(),
shipping: sampleAddressWithEmptyNullableFields()
)

// When
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

}

func test_view_model_fires_error_notice_when_providing_an_invalid_email() {
// Given
let viewModel = EditOrderAddressFormViewModel(order: Order.fake(), type: .billing)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@ import Yosemite

final class CustomerSearchUICommandTests: XCTestCase {
private let sampleSiteID: Int64 = 123
private var analyticsProvider: MockAnalyticsProvider!
private var analytics: Analytics!

override func setUp() {
analyticsProvider = MockAnalyticsProvider()
analytics = WooAnalytics(analyticsProvider: analyticsProvider)
}

func test_searchResultsPredicate_includes_siteID_and_keyword_when_keyword() {
// Given
Expand Down Expand Up @@ -35,4 +42,26 @@ final class CustomerSearchUICommandTests: XCTestCase {
XCTAssertEqual(cellViewModel.title, "\(customer.firstName ?? "") \(customer.lastName ?? "")")
XCTAssertEqual(cellViewModel.subtitle, String(customer.email))
}

func test_CustomerSearchUICommand_when_synchronizeModels_then_tracks_orderCreationCustomerSearch_event() {
// Given
let command = CustomerSearchUICommand(
siteID: sampleSiteID,
analytics: analytics) { _ in }

// When
command.synchronizeModels(
siteID: sampleSiteID,
keyword: "",
pageNumber: 1,
pageSize: 1,
onCompletion: { _ in }
)

// Then
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

}
}