-
Notifications
You must be signed in to change notification settings - Fork 121
[Woo POS][Local Catalog] Basic mapping from POS models to Persisted models #16062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Woo POS][Local Catalog] Basic mapping from POS models to Persisted models #16062
Conversation
Fix description use when making persistedproduct
|
|
| public func toProductAttribute(siteID: Int64) -> ProductAttribute { | ||
| return ProductAttribute( | ||
| siteID: siteID, | ||
| attributeID: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we decide to set the remote ID in a separate column (#16047 (comment)), this can be set. For now, do we want to set this with id ?? 0 like in the variation attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in f822a09
The risk (which was already worse than it is now) is that if we call toProductAttribute on a global attribute which hasn't been saved, we'll get 0 instead of nil, which suggests a local attribute. But then only local attributes are properly handled so far anyway.
| let images: [ProductImage] = [ | ||
| ProductImage(imageID: 100, | ||
| dateCreated: Date(timeIntervalSince1970: 1), | ||
| dateModified: nil, | ||
| src: "https://example.com/1.png", | ||
| name: "img1", | ||
| alt: "alt1"), | ||
| ProductImage(imageID: 101, | ||
| dateCreated: Date(timeIntervalSince1970: 2), | ||
| dateModified: Date(timeIntervalSince1970: 3), | ||
| src: "https://example.com/2.png", | ||
| name: "img2", | ||
| alt: nil) | ||
| ] | ||
| let attributes: [ProductAttribute] = [ | ||
| ProductAttribute(siteID: siteID, attributeID: 0, name: "Color", position: 0, visible: true, variation: true, options: ["Red", "Blue"]), | ||
| ProductAttribute(siteID: siteID, attributeID: 0, name: "Size", position: 1, visible: false, variation: false, options: ["S", "M"]) | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: since these aren't being asserted on in this test case, maybe can pass empty arrays for simplicity.
| struct PersistedProductConversionsTests { | ||
|
|
||
| @Test("PersistedProduct init(from:) maps all POSProduct fields") | ||
| func test_product_init_from_posProduct_maps_all_fields() throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: with Swift Testing, we probably don't need the test_ prefix in the function name as each test case is already marked with @Test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update them. TBH I think we could also leave behind the snake case, if we use descriptive names as arguments to @Test – they display much nicer anyway, at least within Xcode. I'm not sure whether CI pays any attention to them though.
…d' into woomob-1210-relationship-based-mapping-and-saving-for-POS-entities
…OS-entities' into woomob-1210-tests-for-saving-pos-models
64b030a
into
woomob-1210-local-catalog-persistence-classes

Part of: WOOMOB-1210
Merge after: #16061
Description
This PR adds mapping to convert between POS models and Persisted models. Only basic properties are covered, not relationships, which will be handled in a future PR.
Steps to reproduce
None, these are not used in the app yet, just check unit tests.
RELEASE-NOTES.txtif necessary.