Customer search: Fill customer fields on Order screen #7885

iamgabrielma · 2022-10-20T06:46:30Z

Part of: #7741
Closes: #7941

Description

This PR deals with:

Shows an empty view when we start a new keyword search, for parity with Android.
Fill the customer details into the Order details fields once a Customer is selected.
Adds a new view when no Customer search results are found.
Ignores searches for non-registered customers.

Changes

The OrderCustomerListView receives a completion handler with a Customer object when a merchant taps in the search button. Then we use this data to fill the EditOrderAddressForm viewModel.
Removes customerSearchTapped(), as we're not using it anymore.
When mapping SearchResults to Customer objects, we skip mapping those with id = 0 (unregistered), avoiding requests to /wc/v3/customers/0 that will always fail.

Testing instructions

Go to Orders > + > Create new Order > + Add Customer details > Tap on the magnifying glass icon.
Search for a Customer name, and tap on it.

Confirm that Customer fields are filled (will fill both Shipping/Billing if these are different as well).
When searching for non-matching keywords, confirm that the empty search results show up.

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

wpmobilebot · 2022-10-20T07:01:41Z

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 pr7885-2b772b7 on your iPhone

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

…arch

iamgabrielma · 2022-10-27T10:06:12Z

WooCommerce/WooCommerceTests/ViewRelated/Search/Customer/CustomerSearchUICommandTests.swift

+
+        // When
+        let predicate = command.searchResultsPredicate(keyword: "some")
+        let expectedQuery = "siteID == 123 AND ANY searchResults.keyword == \"some\""


Double check if this actually filters down multiple sites.

Confirmed 👍 , for 2 customers with the same name and customer ID, in two different sites, only one is shown.

WooCommerce/WooCommerceTests/ViewRelated/Search/Customer/CustomerSearchUICommandTests.swift

iamgabrielma · 2022-10-28T03:16:51Z

Thanks for your time yesterday to go through the review/pairing @joshheald, I made the following changes:

Extracted the fillCustomerFields() method to the viewModel: 9cf8388
Added Unit Tests to check if fields are correctly being filled: ed04317
Make the shipping details form display automatically if addresses differ: 5dffa97

joshheald

Pretty much there on this, good work.

I noticed another difference from Android (the fields used for name and email) and a problem with one of your tests. A couple of other small comments too.

joshheald · 2022-10-28T09:22:49Z

...rce/Classes/ViewRelated/Orders/Order Details/Address Edit/AddressFormViewModelProtocol.swift

+        fields = populate(fields: fields, with: customer, address: customer.billing)
+        secondaryFields = populate(fields: secondaryFields, with: customer, address: customer.shipping)
+    }
+
+    private func populate(fields: AddressFormFields, with customer: Customer, address: Address?) -> AddressFormFields {
+        var fields = fields
+
+        fields.firstName = customer.firstName ?? ""
+        fields.lastName = customer.lastName ?? ""
+        fields.email = customer.email


Suggested change

fields = populate(fields: fields, with: customer, address: customer.billing)

secondaryFields = populate(fields: secondaryFields, with: customer, address: customer.shipping)

}

private func populate(fields: AddressFormFields, with customer: Customer, address: Address?) -> AddressFormFields {

var fields = fields

fields.firstName = customer.firstName ?? ""

fields.lastName = customer.lastName ?? ""

fields.email = customer.email

fields = populate(fields: fields, with: customer.billing)

secondaryFields = populate(fields: secondaryFields, with: customer.shipping)

}

private func populate(fields: AddressFormFields, with address: Address?) -> AddressFormFields {

var fields = fields

fields.firstName = address.firstName ?? ""

fields.lastName = address.lastName ?? ""

fields.email = address.email

Instead of the name fields on customer, I think we should use address.firstName and address.lastName. Testing Android, with my always-strange test data with different names all over the place, shows that they use these fields to update the data.

This makes sense, because the shipping address may be to a different person, e.g. if it's a gift.

There's address.email, which could differ from customer.email. Android seems to use billingAddress.email there. The email for the secondary fields isn't shown on either platform... but I'm not sure whether it's used in any requests.

It's worth checking that: what happens if we save a different email address in each place? Does it get added to the order twice? Is that what Android does? Is it a bug, one way or the other?

It's also just a nicer function... "Populate address fields with address"

That makes sense. I made the changes here: 121ba8a

Something that peaks my interest with this change is that the email property is now an optional, but it can't actually be optional because is a mandatory field for shippable orders. This seems to happen because when we declare both fields and secondaryFields in the protocol, both inherit from AddressFormFields, however these have slightly different parameters in the API (for example shipping details does not have an email field)

what happens if we save a different email address in each place? Does it get added to the order twice? Is that what Android does? Is it a bug, one way or the other?

This seems a bug from our end when declaring the model, as with the email example above, we're reusing the same AddressFormFields model for both shipping and billing, but these are not exactly the same in the API or WC core (it would seem that the API doc is outdated as well, as phone does not appear as property for shipping but in WC core it does). I've opened an issue here: #7993 as most likely this can be improved.

joshheald · 2022-10-28T09:26:05Z

WooCommerce/Classes/ViewRelated/Orders/Order Details/Address Edit/EditOrderAddressForm.swift

-                print("3 - Customer Callback. Fill Order data with Customer details")
-                print("4 - Customer ID: \(customer.customerID) - Name: \(customer.firstName ?? ""))")
+                viewModel.customerSelectedFromSearch(customer: customer)
+                showingCustomerSearch = false


This can move to the view model too if you like? Can wait for a future PR

joshheald · 2022-10-28T09:27:06Z

...rceTests/ViewRelated/Orders/Order Details/Addresses/EditOrderAddressFormViewModelTests.swift

+        viewModel.customerSelectedFromSearch(customer: customer)
+
+        // Then
+        XCTAssertEqual(viewModel.fields.email, customer.email)


Don't forget about assertEqual: the nicer diffs when stuff is wrong are worth it, even though it's annoying having to swap around the actual and expected when you swap from XCTAssertEqual

joshheald · 2022-10-28T09:27:50Z

...rceTests/ViewRelated/Orders/Order Details/Addresses/EditOrderAddressFormViewModelTests.swift

+    func test_OrderAddressForm_shows_different_address_form_fields_when_addresses_differ_and_customerSelectedFromSearch() {
+        // Given
+        let viewModel = EditOrderAddressFormViewModel(order: Order.fake(), type: .billing)
+        let billing = sampleAddressWithEmptyNullableFields()
+        let shipping = Address.fake().copy(address1: "123 different fake street")
+        let customer = Customer.fake().copy(billing: billing, shipping: shipping)
+
+        // When
+        viewModel.customerSelectedFromSearch(customer: customer)
+
+        // Then
+        XCTAssertTrue(viewModel.showDifferentAddressForm)
+    }


👏👏👏 So good to be able to test this kind of thing with confidence

joshheald · 2022-10-28T09:28:06Z

WooCommerce/WooCommerceTests/ViewRelated/Search/Customer/CustomerSearchUICommandTests.swift

+    override func setUp() {
+        super.setUp()
+    }
+
+    override func tearDown() {
+        super.tearDown()
+    }


You can delete these

Done! 7b58531

joshheald · 2022-10-28T10:15:17Z

Yosemite/YosemiteTests/Stores/CustomerStoreTests.swift

+    func test_searchCustomers_returns_no_customers_when_customer_is_not_registered() throws {
+        // Given
+        let customerID = 0
+        network.simulateResponse(requestUrlSuffix: "customers", filename: "wc-analytics-customers")
+        network.simulateResponse(requestUrlSuffix: "customers/\(customerID)", filename: "customer")
+
+        // When
+        let response: Result<[Networking.Customer], Error> = waitFor { promise in
+            let action = CustomerAction.searchCustomers(siteID: self.dummySiteID, keyword: self.dummyKeyword) { result in
+                promise(result)
+            }
+            self.dispatcher.dispatch(action)
+        }
+
+        // Then
+        let customers = try response.get()
+        XCTAssertEqual(customers.count, 0)
+    }


Nice test! This is business logic which is well worth checking.

However... it doesn't really test what it looks like. I had a go at making it fail (by removing the if result.userID == 0 check in the store) and it still passed.... When you're testing a negative, it's especially worthwhile making sure that the test fails when you expect. Think Red-Green-Refactor, even when you're not actually doing TDD.

The reason is that we don't have anyone in wc-analytics-customer.json with user_id: 0, you'll need to add another entry to that file to get the test to work.

We can improve it further though... here's another way to do it, which more directly checks that the request is not sent, without needing to see what the customer response is:

(N.B. This still needs an analytics customer with user_id: 0 in the json file)

Suggested change

func test_searchCustomers_returns_no_customers_when_customer_is_not_registered() throws {

// Given

let customerID = 0

network.simulateResponse(requestUrlSuffix: "customers", filename: "wc-analytics-customers")

network.simulateResponse(requestUrlSuffix: "customers/\(customerID)", filename: "customer")

// When

let response: Result<[Networking.Customer], Error> = waitFor { promise in

let action = CustomerAction.searchCustomers(siteID: self.dummySiteID, keyword: self.dummyKeyword) { result in

promise(result)

}

self.dispatcher.dispatch(action)

}

// Then

let customers = try response.get()

XCTAssertEqual(customers.count, 0)

}

func test_searchCustomers_returns_no_customers_when_customer_is_not_registered() throws {

// Given

network.simulateResponse(requestUrlSuffix: "customers", filename: "wc-analytics-customers")

// When

waitFor { promise in

let action = CustomerAction.searchCustomers(siteID: self.dummySiteID, keyword: self.dummyKeyword) { result in

promise(())

}

self.dispatcher.dispatch(action)

}

// Then

let requests = network.requestsForResponseData.compactMap { $0 as? JetpackRequest }

XCTAssertFalse(requests.contains(where: { request in

request.path == "customers/0"

}))

}

Oh that's nice! Made the changes on 7b58531

We’ll be populating the fields using the Address data rather than Customer data for these 3 fields, for consistency with Android. There’s some inconsistency with the API though, reported here: #7993

fill Customer fields on Order screen

5869ec0

iamgabrielma changed the title ~~fill Customer fields on Order screen~~ Customer search: Fill customer fields on Order screen Oct 20, 2022

iamgabrielma mentioned this pull request Oct 20, 2022

Order creation: Search Customers #7741

Closed

15 tasks

iamgabrielma added type: enhancement A request for an enhancement. category: parity Match what's supported by the other platform. feature: order creation All tasks related to creating an order labels Oct 20, 2022

iamgabrielma modified the milestones: 10.9, 11.0 Oct 20, 2022

iamgabrielma added the status: draft This is a draft, still need more work but can be reviewed and commented if asked. label Oct 24, 2022

Base automatically changed from issue/741-implement-customer-search-ui to trunk October 26, 2022 07:45

iamgabrielma added 8 commits October 26, 2022 16:47

Merge branch 'trunk' into issue/7741-fill-order-data-from-customer-se…

d7060dc

…arch

Add shipping fields to EditAddressOrderForm

e05a33d

Add CustomerSearchUICommand unit tests

db2fb24

Hound: trailing_newline

5419298

Internal comment cleanup

fae8db5

Skip searchCustomer request for customerID = 0

dff4aed

Remove unnecessary method

64cb6fe

Add empty view for empty search results

c5fbc90

iamgabrielma removed the status: draft This is a draft, still need more work but can be reviewed and commented if asked. label Oct 27, 2022

iamgabrielma marked this pull request as ready for review October 27, 2022 02:40

iamgabrielma requested review from ThomazFB and joshheald October 27, 2022 03:04

Set empty state for new keyword searches

1733984

iamgabrielma commented Oct 27, 2022

View reviewed changes

WooCommerce/WooCommerceTests/ViewRelated/Search/Customer/CustomerSearchUICommandTests.swift Show resolved Hide resolved

iamgabrielma modified the milestones: 11.0, 11.1 Oct 28, 2022

Extract fillCustomerFields method to viewModel

9cf8388

iamgabrielma added 2 commits October 28, 2022 11:36

Add Unit Test for fillCustomerFields()

ed04317

Display shipping details form if addresses differ

5dffa97

joshheald self-assigned this Oct 28, 2022

joshheald approved these changes Oct 28, 2022

View reviewed changes

iamgabrielma added 4 commits November 1, 2022 09:08

Add/improve test case for unregistered customer

7b58531

Populate fields with Address details, not Customer

121ba8a

We’ll be populating the fields using the Address data rather than Customer data for these 3 fields, for consistency with Android. There’s some inconsistency with the API though, reported here: #7993

Update WCAnalyticsCustomerMapperTests

d343863

Update EdirOrderAddressFormViewModel tests

2b772b7

iamgabrielma merged commit daffe2d into trunk Nov 1, 2022

iamgabrielma deleted the issue/7741-fill-order-data-from-customer-search branch November 1, 2022 03:01

Customer search: Fill customer fields on Order screen #7885

Customer search: Fill customer fields on Order screen #7885

Uh oh!

Conversation

iamgabrielma commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Testing instructions

Uh oh!

wpmobilebot commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iamgabrielma commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshheald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamgabrielma Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iamgabrielma commented Oct 20, 2022 •

edited

Loading

wpmobilebot commented Oct 20, 2022 •

edited

Loading

iamgabrielma commented Oct 28, 2022 •

edited

Loading

iamgabrielma Nov 1, 2022 •

edited

Loading