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

[Woo POS] [Variable Products] Show variation name based on variation and variable product attributes #14807

Merged
merged 6 commits into from
Jan 8, 2025

Conversation

jaclync
Copy link
Contributor

@jaclync jaclync commented Jan 7, 2025

Closes: #14702

Please review #14793 first.

Description

The PR adds support to show the name for variations in the POS item selector the same way as in the products tab. The key changes involve updating the POSParentProductType to include attributes from a variable product, moving and modifying the ProductVariationFormatter and VariationAttributeViewModel classes, and generating variation name in the PointOfSaleItemService.

Changes to Product Variations and Attributes Handling:

  • POSParentProductType now includes attributes, which allows for generating the name of product variations to show attributes in the expected order including "any" attributes. (Yosemite/Yosemite/PointOfSale/POSParentProduct.swift)
  • Added error handling for invalid parent products in PointOfSaleItemService. (Yosemite/Yosemite/PointOfSale/PointOfSaleItemService.swift) [1] [2]
  • Updated the providePointOfSaleVariationItems method to use ProductVariationFormatter for generating variation names. (Yosemite/Yosemite/PointOfSale/PointOfSaleItemService.swift)

Code Refactoring and File Management:

  • Moved ProductVariationFormatter from WooCommerce to Yosemite and made it public. (Yosemite/Yosemite/Tools/ProductVariations/ProductVariationFormatter.swift) [1] [2]
  • Added VariationAttributeViewModel to Yosemite project files to support variation attribute handling. (Yosemite/Yosemite.xcodeproj/project.pbxproj) [1] [2]

Miscellaneous Changes:

  • Implemented Hashable for ProductAttribute to support hashing. (Networking/Networking/Model/Product/ProductAttribute.swift)
  • Removed unused import statements and cleaned up code. (WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift)
  • Updated various mock data and preview methods to include attributes. Empty value is used as previews use a different items service from PointOfSaleItemService that generates the variation name based on attributes. (WooCommerce/Classes/POS/Presentation/Item Selector/ChildItemList.swift, WooCommerce/Classes/POS/Presentation/Item Selector/ItemList.swift, WooCommerce/Classes/POS/Presentation/ParentProductCardView.swift, WooCommerce/Classes/POS/Utils/PreviewHelpers.swift) [1] [2] [3] [4]

Steps to reproduce

🗒️ As the variable products support requires WC version 9.6+ which is not publicly released at the time of writing, I'd recommend commenting out L214 in ProductsRemote to include all product types.

  • Switch to a store eligible for POS and has at least one variable product
  • Go to Menu > Point of Sale mode
  • Tap on a variable product --> variations should be loaded shortly. each variation card should show the correct name with the attributes in the right order and include any attributes
  • Check a few other variable products and verify that the variation name is displayed correctly

Testing information

Screenshots

Simulator.Screen.Recording.-.iPad.Pro.11-inch.M4.-.2025-01-07.at.13.19.13.mp4

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

…om WooCommerce to Yosemite and use `ProductVariationFormatter` to generate variation name in POS with `allAttributes` in `POSParentProductType.variable` associated type.
@jaclync jaclync added type: task An internally driven task. status: feature-flagged Behind a feature flag. Milestone is not strongly held. labels Jan 7, 2025
@jaclync jaclync added this to the 21.4 milestone Jan 7, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 7, 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 Numberpr14807-8faf104
Version21.3
Bundle IDcom.automattic.alpha.woocommerce
Commit8faf104
App Center BuildWooCommerce - Prototype Builds #12395
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@jaclync jaclync marked this pull request as ready for review January 7, 2025 07:35
@jaclync jaclync requested a review from joshheald January 7, 2025 07:35
@joshheald joshheald self-assigned this Jan 7, 2025
Copy link
Contributor

@joshheald joshheald left a comment

Choose a reason for hiding this comment

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

Works as expected, thanks!

One suggestion inline to improve encapsulation of allAttributes.

One thought for future improvement: the names the existing code generates might not be ideal for POS. If that would stop you reusing the code, perhaps it would be better not to merge this one with the move and reuse, and instead add a new name generator. Or we improve the names in both places.

The issue is that the names are primarily made up of the value of the attribute, and don't include attribute names.

With a single simple attribute like Colour, it's pretty clear, but what if a product had two colour attributes making up it's variation – e.g. a printed t-shirt with fabric colour, and printing colour. That might be called White - Black - S, for a white T-shirt with Black printing in size S... but it might be called Black - White - M for a black T with white printing. The merchant would need to use the image (if supplied for every variation) or remember the order.

This is probably also something we should improve in the main app, as when fulfilling web orders it'd be bad to pick the wrong variation. In POS we currently have lots of space for the name, so it looks particularly silly to leave attribute names off. See this screenshot – there's context for the Yes/No bit from the Any Logo variant, but most stores shouldn't use Any variations anyway.

IMG_1E4C792B5F6F-1

Comment on lines 3 to 4
public enum POSParentProductType: Equatable, Hashable {
case variable(allAttributes: [ProductAttribute])
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we ever use the associated value outside of Yosemite, right?

What do you think about having a specific POSVariableParentProduct type, instead of using a type enum here? If we did that, attributes could be an internal property, and only used inside Yosemite. It might prove easier to handle than an enum with associated types, too.

It would mean having a different display approach for POSGroupedProduct or POSCategory, but it's reasonably likely that we'll want to display those differently in the grid anyway.

Copy link
Contributor Author

@jaclync jaclync Jan 8, 2025

Choose a reason for hiding this comment

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

I don't think we ever use the associated value outside of Yosemite, right?

Correct, allAttributes is only used in Yosemite.

What do you think about having a specific POSVariableParentProduct type, instead of using a type enum here? If we did that, attributes could be an internal property, and only used inside Yosemite. It might prove easier to handle than an enum with associated types, too.

It would mean having a different display approach for POSGroupedProduct or POSCategory, but it's reasonably likely that we'll want to display those differently in the grid anyway.

Good point on making allAttributes an internal property in Yosemite. I tried it first by renaming POSParentProductType to POSVariableParentProduct, but it means views like ChildItemList and ParentProductCardView start depending on a "variable parent product" instead of a general parent product. It requires some refactoring, I will merge this first and show the changes in a separate PR.

As a medium approach to keep allAttributes internal, I updated the associated type of allAttributes to a struct POSVariableParentProduct in 7c365a3.

Base automatically changed from feat/14702-rename-item-service-and-add-unit-tests to trunk January 8, 2025 01:13
@jaclync
Copy link
Contributor Author

jaclync commented Jan 8, 2025

One thought for future improvement: the names the existing code generates might not be ideal for POS. If that would stop you reusing the code, perhaps it would be better not to merge this one with the move and reuse, and instead add a new name generator. Or we improve the names in both places.

Thanks for raising this, I was following the Android approach (example pffQ75-XG-p2#comment-762) which reuses the variation name in other parts of the app. I posted a question in p1736303462081769-slack-C070SJRA8DP for discussion to have a consistent variation name display across platforms. Ideally, we can improve it for other parts of the app as well.

I'm merging this PR first and will open a PR to follow up on #14807 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feature-flagged Behind a feature flag. Milestone is not strongly held. type: task An internally driven task.
Projects
None yet
3 participants