Customer Search: Coordinate Customer and CustomerSearchResults entities with Storage

[iamgabrielma]

Part of: #7741

Description

This PR updates the Network's Customer and WCAnalyticsCustomer entities with a siteID parameter. Then adds support for upserting Customer and CustomerSearchResults to Storage.

Changes

• Networking: Adds siteID to Customer and WCAnalyticsCustomer models.
• Yosemite: Adds upsertCustomer and upsertSearchCustomerResult methods to CustomerStore + Unit Tests
• Storage: Adds Customer+ReadOnlyConvertible
• Storage: Adds necessary StorageType+Extensions + Unit Tests

Testing instructions

1. Confirm that Unit Tests pass.
2. The current implementation assumes that you have at least one customer in your store, and that customer name contains the word "hello", if that's not the case, you can change this to the keyword you prefer:

woocommerce-ios/WooCommerce/Classes/ViewRelated/Orders/Order Creation/CustomerSection/CreateOrderAddressFormViewModel.swift

Lines 108 to 112 in ec11323

	keyword: "hello") { result in
	switch result {
	case .success(let customers):
	let storage = ServiceLocator.storageManager
	guard let result = storage.viewStorage.loadCustomerSearchResult(siteID: self.siteID, keyword: "hello") else {

3. Go to "Orders" > "+" > "Create new Order" > "+ "Add Customer" and tap in the "Magnifying glass" icon at the top of the screen:

4. You should see your siteID, the keyword used, and the number of Customers inserted to Storage, as well as the CustomerSearchResults in the console. Similar to:

Site ID: 121710041, keyword: hello, Customers: Optional(2)
Customer: 4,
Name: Optional("hello") Optional("merch")
Customer: 5,
Name: Optional("Hello") Optional("maldon")

[wpmobilebot]

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 pr7848-0a79129 on your iPhone

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

[rachelmcr]

Why are we adding a siteID property to WCAnalyticsCustomer? I followed how the changes to Customer support upserting everything to storage, but I don't see how the changes to WCAnalyticsCustomer fit into that. (I might be missing something here!)

Otherwise, my comments below are mostly just nits. These changes work as expected, and thanks for adding test coverage!

[rachelmcr]

Why are we adding a siteID property to WCAnalyticsCustomer? It's very possible I missed it, but I didn't see where we're using this. It looks like we may only need it for the Customer model.

[iamgabrielma]

You're right that we're not actively using it at the moment. That said I'd prefer to leave it for consistency with the rest of the models, and at some point we may need to save this to storage directly and siteID will be necessary ( I just haven't reached that point yet :D ). I can always circle back once the feature is completed and remove it if needed 👍

[rachelmcr]

I generally prefer to wait to make changes until they're definitely needed, but you have more context about the overall changes needed for this project so I'd say go with what makes most sense for your plans here!

[iamgabrielma]

Thanks for review @rachelmcr 🙇 , feedback has been addressed and there was also a conflict with trunk that has been resolved. Ready for another look when you can!

[rachelmcr]

Looks good! I left a comment about a point where a strong reference to self is being captured implicitly, but once that's resolved this should be good to go.

[rachelmcr]

Since we're using self.sharedDerivedStorage in the closure here, it would be good to capture weak self, rather than implicitly capturing a strong reference to self. (Another option is to assign sharedDerivedStorage to a constant in the method before using it, so you don't need a reference to self in the closure.)

[rachelmcr]

I generally prefer to wait to make changes until they're definitely needed, but you have more context about the overall changes needed for this project so I'd say go with what makes most sense for your plans here!