Skip to content
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

Merged
merged 10 commits into from
Sep 4, 2024

Conversation

pmusolino
Copy link
Member

@pmusolino pmusolino commented Sep 2, 2024

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

  • Adds a customFields property to the Product model, allowing for additional metadata to be associated with products.
  • Updates the encoding process to include the customFields when serializing the Product object.
  • Modifies various files to ensure that customFields are initialized and handled correctly across the system.
  • Introduces relevant TODO comments for future implementation of custom field handling within the storage layer.

Testing

  • I verified that products works like before.
  • I did a smoke testing to understand if AddOns and Products with subscriptions works like before.

  • I have considered if this change warrants user-facing release notes and have added them to 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:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

- 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.
@pmusolino pmusolino added type: task An internally driven task. feature: product details Related to adding or editing products, including Product Settings. labels Sep 2, 2024
@pmusolino pmusolino added this to the 20.3 milestone Sep 2, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 2, 2024

WooCommerce iOS📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr13814-ff9b457
Version20.2
Bundle IDcom.automattic.alpha.woocommerce
Commitff9b457
App Center BuildWooCommerce - Prototype Builds #10725
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@selanthiraiyan selanthiraiyan self-assigned this Sep 3, 2024
Copy link
Contributor

@selanthiraiyan selanthiraiyan left a 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.

Comment on lines +528 to +529
// 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("_")})) ?? []
Copy link
Contributor

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?

Copy link
Member Author

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".
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is assigned to the milestone 20.3. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

Copy link
Contributor

@selanthiraiyan selanthiraiyan left a 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. 🚢

@pmusolino pmusolino merged commit 4b7fd10 into trunk Sep 4, 2024
22 checks passed
@pmusolino pmusolino deleted the issue/13493-new-metadata-entity-in-product branch September 4, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: product details Related to adding or editing products, including Product Settings. type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update MetaData entity to include Product row
4 participants