Add `siteID` and `keyword` attributes to `Customer` and `CustomerSearchResult` entities #7860

iamgabrielma · 2022-10-14T01:13:05Z

Part of #7741

Apologies for the ~4.5k diff, most of it comes from regenerating the data entities, and the mapping model migration from 74 to 75, when adding siteID to the entities that previously didn't have them.

Description

When we created the Storage layer and data model for Customer and CustomerSearchResult on #7841, we (I) didn't realize that we share the same storage across all sites a merchant may have (we do not hold 1 different database per site, but 1 per all sites). This may cause unintended consecuences when a merchant has more than 1 store in their account, as we wouldn't have a way to filter customerID per site basis when we store fetched customers and results.

Changes

This PR updates the Customer and CustomerSearchResult by adding a siteID to both entities, as well as substituting customerID attribute for the keyword attribute in the CustomerSearchResult entity.

Updated Data Model from 74 to 75.
Add non-optional siteID property to Customer and CustomerSearchResults
Add non-optional keyword property to CustomerSearchResult
Remove customerID property from CustomerSearchResult
Used mapping model: WooCommerceModelV74toV75.xcmappingmodel to remove Customer and CustomerSearchResults entities in model version 74
Added a 74_to_75 test to MigrationTests to confirm that the properties are correct after the data model migration.
Updated MIGRATIONS.md

Testing instructions

Confirm that Unit tests pass
Confirm that the app doesn't crash

Adds siteID attributes to Customer & CustomerSearchResults. Currently the migration test 74_75 is failing when attempting to migrate()

wpmobilebot · 2022-10-14T01:27:08Z

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 pr7860-07ae1ca on your iPhone

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

Storage/StorageTests/CoreData/MigrationTests.swift

iamgabrielma · 2022-10-14T03:48:35Z

Storage/StorageTests/CoreData/MigrationTests.swift

+    func insertCustomerSearchResult(to context: NSManagedObjectContext, forModel modelVersion: Int) -> NSManagedObject {
+        if modelVersion < 75 {
+            let customerSearchResult = context.insert(entityName: "CustomerSearchResult", properties: [
+                "customerID": 1])
+            return customerSearchResult
+        } else {
+            // Required since model 75
+            let customerSearchResult = context.insert(entityName: "CustomerSearchResult", properties: [
+                "siteID": 1,
+                "keyword": ""
+            ])
+            return customerSearchResult
+        }


Interestingly, here I had to use this logic rather than the one I used on insertCustomer(to:forModel:), as we're removing the customerID attribute entirely, migration tests were failing due to incompatibility between models. Happy to refactor if there's a better way to test this.

Makes sense. You might just want to do the old version in-line in the test which uses it, since it'll never be relevant again, but up to you on that one...

Much more cleaner indeed. Changed here 312e9fd and removed the now unused helper function here 761736b

joshheald

Looks good... a question about the optional keyword, and some general observations, but feel free to merge with or without the optional change.

joshheald · 2022-10-14T05:59:20Z

Storage/Storage/Model/Customer+CoreDataProperties.swift


-    @NSManaged public var customerID: Int64
-    @NSManaged public var email: String?
-    @NSManaged public var firstName: String?
-    @NSManaged public var lastName: String?
-    @NSManaged public var billingFirstName: String?
-    @NSManaged public var billingLastName: String?
-    @NSManaged public var billingCompany: String?
    @NSManaged public var billingAddress1: String?
    @NSManaged public var billingAddress2: String?
    @NSManaged public var billingCity: String?
-    @NSManaged public var billingState: String?
-    @NSManaged public var billingPostcode: String?
+    @NSManaged public var billingCompany: String?
    @NSManaged public var billingCountry: String?
-    @NSManaged public var billingPhone: String?
    @NSManaged public var billingEmail: String?
-    @NSManaged public var shippingFirstName: String?
-    @NSManaged public var shippingLastName: String?
-    @NSManaged public var shippingCompany: String?
+    @NSManaged public var billingFirstName: String?
+    @NSManaged public var billingLastName: String?
+    @NSManaged public var billingPhone: String?
+    @NSManaged public var billingPostcode: String?
+    @NSManaged public var billingState: String?
+    @NSManaged public var customerID: Int64
+    @NSManaged public var email: String?
+    @NSManaged public var firstName: String?
+    @NSManaged public var lastName: String?
    @NSManaged public var shippingAddress1: String?
    @NSManaged public var shippingAddress2: String?
    @NSManaged public var shippingCity: String?
-    @NSManaged public var shippingState: String?
-    @NSManaged public var shippingPostcode: String?
+    @NSManaged public var shippingCompany: String?
    @NSManaged public var shippingCountry: String?
-    @NSManaged public var shippingPhone: String?
    @NSManaged public var shippingEmail: String?
+    @NSManaged public var shippingFirstName: String?
+    @NSManaged public var shippingLastName: String?
+    @NSManaged public var shippingPhone: String?
+    @NSManaged public var shippingPostcode: String?
+    @NSManaged public var shippingState: String?
+    @NSManaged public var siteID: Int64


Yay inconsistent code generation... this isn't a problem, but this is what I meant when I mentioned that I'll often just run the generation to check it works, then revert it and add/update properties on the generated files so that the change is easier to see. No need to do anything, but it's a good practical example.

joshheald · 2022-10-14T06:00:07Z

Storage/Storage/Model/CustomerSearchResult+CoreDataProperties.swift


-    @NSManaged public var customerID: Int64
+    @NSManaged public var siteID: Int64
+    @NSManaged public var keyword: String?


Add optional keyword property to CustomerSearchResult

Why optional? I don't think it makes sense to have a search result with a nil keyword...

🙇 Made the changes discussed in Slack here: 312e9fd by changing keyword to be non-optional and providing a default value so we're sure that the entity conforms to the new schema when migrating

Since we’re making the “keyword” attribute non-optional, we’re also providing a default value to the CustomerSearchResult entity so we can migrate between data models without issues, as any existing CustomerSearchResult would need to have a “keyword” set to it when we’re migrating data sets. With this we also altered the 74_to_75 tests to check migrated entities between data models, rather than new entities in the new data model.

Storage/Storage/Model/MIGRATIONS.md

itsmeichigo

❓ Looking at the changes here, I think we won't have the correct siteID for Customer and CustomerSearchResult after the migration between 74 and 75 - since the attributes didn't exist in version 74 (the default values will be 0). Do you think we should do heavyweight migration to handle the missing attributes instead?

iamgabrielma · 2022-10-17T04:06:05Z

❓ Looking at the changes here, I think we won't have the correct siteID for Customer and CustomerSearchResult after the migration between 74 and 75 - since the attributes didn't exist in version 74 (the default values will be 0). Do you think we should do heavyweight migration to handle the missing attributes instead?

Thanks for your help on this! In db370f8 a new WooCommerceModelV74toV75 has been added as well as new tests to test_migrating_from_74_to_75_adds_siteID_and_keyword_attributes_to_Customer_and_CustomerSearchResult so we can confirm the changes to both entities and attributes, pre and post-migration.

iamgabrielma · 2022-10-17T08:04:46Z

Quick update: There's a step missing and tests were passing because of an error here: https://github.com/woocommerce/woocommerce-ios/pull/7860/files#diff-03c54fe20fd9822d0011069f12658bf05260422a448742db92b81c339a6882deR1418-R1419

Currently, in the MigrationTests we're checking if the entities deletion happens in the sourceContext, which is wrong in this step as we should be checking the targetContext instead:

- XCTAssertEqual(try sourceContext.count(entityName: "Customer"), 0)
- XCTAssertEqual(try sourceContext.count(entityName: "CustomerSearchResult"), 0)
+ XCTAssertEqual(try targetContext.count(entityName: "Customer"), 0)
+ XCTAssertEqual(try targetContext.count(entityName: "CustomerSearchResult"), 0)

The missing step is to use DeleteEntityObjectsMigrationPolicy in the Mapping Model to be sure that this entity deletion happens between models. This, and the tests, have been fixed on 07ae1ca

itsmeichigo

Great work 🎉

Update data model to 75

2f312fd

Adds siteID attributes to Customer & CustomerSearchResults. Currently the migration test 74_75 is failing when attempting to migrate()

iamgabrielma commented Oct 14, 2022

View reviewed changes

Storage/StorageTests/CoreData/MigrationTests.swift Outdated Show resolved Hide resolved

Make keyword attribute optional. fix migration tests

017f527

iamgabrielma changed the title ~~Add siteID and keyword to~~ Add siteID and keyword attributes to Customer and CustomerSearchResult entities Oct 14, 2022

Update migrations.md

352537a

iamgabrielma commented Oct 14, 2022

View reviewed changes

iamgabrielma marked this pull request as ready for review October 14, 2022 03:52

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 14, 2022

iamgabrielma added this to the 10.8 milestone Oct 14, 2022

iamgabrielma requested review from ThomazFB, itsmeichigo and joshheald October 14, 2022 03:53

joshheald approved these changes Oct 14, 2022

View reviewed changes

iamgabrielma added 2 commits October 14, 2022 16:01

Remove unneeded helper function

761736b

iamgabrielma requested a review from joshheald October 14, 2022 07:37

spencertransier modified the milestones: 10.8, 10.9 Oct 15, 2022

iamgabrielma commented Oct 17, 2022

View reviewed changes

Storage/Storage/Model/MIGRATIONS.md Outdated Show resolved Hide resolved

itsmeichigo reviewed Oct 17, 2022

View reviewed changes

iamgabrielma added 2 commits October 17, 2022 12:50

Add WooCommerceModelV74toV75 mapping model

db370f8

Fix line length warning

e08e392

iamgabrielma requested a review from itsmeichigo October 17, 2022 04:26

iamgabrielma added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Oct 17, 2022

Add NSEntityMigrationPolicy to mapping migration

07ae1ca

iamgabrielma removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Oct 17, 2022

itsmeichigo approved these changes Oct 18, 2022

View reviewed changes

iamgabrielma merged commit b019b67 into trunk Oct 18, 2022

iamgabrielma deleted the issue/7741-add-siteID-to-data-model branch October 18, 2022 01:49

iamgabrielma mentioned this pull request Oct 18, 2022

Order creation: Search Customers #7741

Closed

15 tasks

Add siteID and keyword attributes to Customer and CustomerSearchResult entities #7860

Add siteID and keyword attributes to Customer and CustomerSearchResult entities #7860

Uh oh!

Conversation

iamgabrielma commented Oct 14, 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 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

iamgabrielma Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

joshheald Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

iamgabrielma Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

joshheald left a comment

Choose a reason for hiding this comment

Uh oh!

joshheald Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

joshheald Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

iamgabrielma Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

itsmeichigo left a comment • 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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

itsmeichigo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Add `siteID` and `keyword` attributes to `Customer` and `CustomerSearchResult` entities #7860

Add `siteID` and `keyword` attributes to `Customer` and `CustomerSearchResult` entities #7860

iamgabrielma commented Oct 14, 2022 •

edited

Loading

wpmobilebot commented Oct 14, 2022 •

edited

Loading

itsmeichigo left a comment •

edited

Loading

iamgabrielma commented Oct 17, 2022 •

edited

Loading

iamgabrielma commented Oct 17, 2022 •

edited

Loading