-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add Custom Fields to Product Model in Networking #13814
Conversation
…493-new-metadata-entity-in-product
…493-new-metadata-entity-in-product
…493-new-metadata-entity-in-product
…493-new-metadata-entity-in-product
…493-new-metadata-entity-in-product
- Update `Networking.generated.swift` to include `customFields` in `Networking.Product`. - Modify `Models+Copiable.generated.swift` to add `customFields` parameter in product methods. - Adjust `AIProduct.swift` to initialize `customFields` as an empty array. - Enhance `Product.swift` to declare and handle `customFields`. - Update `Product+SwiftUIPreviewHelpers.swift` to include `customFields` in product initialization. - Modify `ProductFactory.swift` to initialize products with empty `customFields`. - Add `customFields` to mock product in `MockObjectGraph.swift`. - Adjust `Product+ReadOnlyConvertible.swift` to handle `customFields` in product storage.
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
…493-new-metadata-entity-in-product
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 smoke testes products tab in general, and it works as before. ✅
I think we should add unit tests to ensure that customFields
are parsed as expected from a mock JSON. Please correct me if I am missing any context here.
// Filter out metadata if the key is prefixed with an underscore (internal meta keys) | ||
let customFields = (try? container.decode([MetaData].self, forKey: .metadata).filter({ !$0.key.hasPrefix("_")})) ?? [] |
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 think we should add unit tests to ensure that customFields
are mapped as expected. Probably to ProductMapperTests
. What do you think?
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.
Good point. I implemented this test: ff9b457
- Implement test_custom_fields_are_properly_parsed in ProductMapperTests.swift to verify custom fields parsing. - Update product.json to include a new custom field with id 6000, key "just_a_custom_field", and value "10".
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.
Thanks for adding the unit test, Paolo! The code looks good to me. ✅
The PR is ready to merge once we fix that failing unit test. 🚢
Closes: #13751 part of #13493
Summary description of changes
This PR introduces custom fields to the product model, improving the flexibility of metadata handling. The changes include updates to the product struct, encoding logic, and associated mock objects to accommodate these custom fields, which will be used later. Right now, the custom fields are not used in the code, and also the Storage/Yosemite layer implementation is missing and will be handled later on.
In future PRs, AddOns and subscription that currently use metadata, should be also refactored.
Summary of changes
customFields
property to theProduct
model, allowing for additional metadata to be associated with products.customFields
when serializing theProduct
object.customFields
are initialized and handled correctly across the system.Testing
RELEASE-NOTES.txt
if necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: