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

Rename PointOfSaleProductService to PointOfSaleItemService with basic unit tests on variations #14793

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

jaclync
Copy link
Contributor

@jaclync jaclync commented Jan 6, 2025

Part of #14702

Description

As we're adding variation items fetching to PointOfSaleItemServiceProtocol, it starts to look odd with the name PointOfSaleProductService that seems to be product-specific. This PR simply renames PointOfSaleProductService to PointOfSaleItemService across different files and references. Two test cases were also added for providePointOfSaleVariationItems function that didn't fit in #14786 that introduced it.

Renaming and Refactoring:

Steps to reproduce

A confidence test to ensure the products & variations are loaded properly in the item selector would be helpful.

Testing information

  • @jaclync performs a confidence test above before merging

Screenshots


  • 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.

@jaclync jaclync added the status: draft This is a draft, still need more work but can be reviewed and commented if asked. label Jan 6, 2025
@jaclync jaclync added this to the 21.4 milestone Jan 6, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 6, 2025

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 Numberpr14793-346e6c1
Version21.3
Bundle IDcom.automattic.alpha.woocommerce
Commit346e6c1
App Center BuildWooCommerce - Prototype Builds #12372
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Base automatically changed from feat/14702-fetch-variations-and-show-in-navigation-stack to trunk January 7, 2025 02:42
…est cases for `providePointOfSaleVariationItems`.
@jaclync jaclync force-pushed the feat/14702-rename-item-service-and-add-unit-tests branch from f62e655 to 346e6c1 Compare January 7, 2025 03:15
@jaclync jaclync added type: technical debt Represents or solves tech debt of the project. feature: POS and removed status: draft This is a draft, still need more work but can be reviewed and commented if asked. labels Jan 7, 2025
@jaclync jaclync marked this pull request as ready for review January 7, 2025 03:47
@iamgabrielma iamgabrielma self-assigned this Jan 7, 2025
Copy link
Contributor

@iamgabrielma iamgabrielma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚢

@joshheald
Copy link
Contributor

LGTM, but threw up some future questions for our overall approach.

It makes sense if we have a single service for fetching both types of items. I think this is forced on us by the reliance on server pagination.

However, setting pagination aside, different item types that make up the ItemListState would only need to be combined in the PointOfSaleItemsController. The request for variations lists is different for the one for product lists.

In future, if we have custom grids or add in other types of items (e.g. shipping, discounts, coupons, customers), we'd probably want to have other services to provide each of these.

Unless we have a single endpoint for all POS items (perhaps this would be a good idea) we may not want to combine them all at the service level. Pagination will definitely be a sticking point for this!

@jaclync
Copy link
Contributor Author

jaclync commented Jan 8, 2025

It makes sense if we have a single service for fetching both types of items. I think this is forced on us by the reliance on server pagination.

However, setting pagination aside, different item types that make up the ItemListState would only need to be combined in the PointOfSaleItemsController. The request for variations lists is different for the one for product lists.

In future, if we have custom grids or add in other types of items (e.g. shipping, discounts, coupons, customers), we'd probably want to have other services to provide each of these.

Unless we have a single endpoint for all POS items (perhaps this would be a good idea) we may not want to combine them all at the service level. Pagination will definitely be a sticking point for this!

Yea good question, was the original plan to have different services like PointOfSaleProductService, PointOfSaleVariationService (and similar for other item types) in the items controller? If so, the variation service might have to take in the parent product dependency from the initializer or some separate function/property setter. I can work on this change after the confirmation.

@jaclync jaclync enabled auto-merge January 8, 2025 01:13
@jaclync jaclync merged commit a8190bb into trunk Jan 8, 2025
28 of 30 checks passed
@jaclync jaclync deleted the feat/14702-rename-item-service-and-add-unit-tests branch January 8, 2025 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: POS type: technical debt Represents or solves tech debt of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants