Skip to content

Conversation

@iamgabrielma
Copy link
Contributor

@iamgabrielma iamgabrielma commented Jul 3, 2025

Description

As follow-up from p1751433227323889-slack-CGPNUU63E , we move the sales channel representation from WooCommerce to Networking, removing some code duplication, and avoiding having to handle the pos-rest-api raw value directly in the app layer.

"how about adding the calculated property for order.salesChannel similar to product.productType so that the use cases don't have to work with the raw value? The sales channel can be an enum too like the product type."

Let me know if this is what you had in mind @jaclync , happy to iterate further!

Changes

  • Declared a SalesChannelType , where the created_via value is passed directly and returns the specific associated case and it's description ( for now, only .pointOfSale - "POS" is handled, any other value will return nil)
  • Access this description directly from the appropriate cells.

Testing information

  • Navigate to your orders, POS badges should render both in order list and order details when a POS order is processed (there is no backfilling, you might need to process a fresh POS order if created_via hasn't been set yet)

Simulator Screenshot - iPad mini (A17 Pro) - US store - 2025-07-03 at 17 49 09

@iamgabrielma iamgabrielma added type: task An internally driven task. feature: order details Related to order details. feature: order list Related to the order list. feature: POS labels Jul 3, 2025
@iamgabrielma iamgabrielma added this to the 22.8 milestone Jul 3, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jul 3, 2025

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

App NameWooCommerce iOS Prototype
Build Numberpr15863-7df616b
Version22.7
Bundle IDcom.automattic.alpha.woocommerce
Commit7df616b
Installation URL3iid6t6j8u6c8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@iamgabrielma iamgabrielma marked this pull request as ready for review July 4, 2025 06:22
@iamgabrielma iamgabrielma requested a review from jaclync July 4, 2025 06:22

// Then
XCTAssertEqual(viewModel.salesChannel, OrderListCellViewModel.Localization.salesChannelPOSText)
XCTAssertEqual(viewModel.salesChannel, "POS")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to revert the localization here as now the localized string sits in Networking 🤔

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 this is fine, a bunch of unit tests are already based on the English locale assumption.

Base automatically changed from task/update-pos-badge-color to trunk July 4, 2025 10:04
Copy link
Contributor

@jaclync jaclync 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 making the updates! LGTM :shipit:

Unrelated to this PR, the POS badge seems to be truncated in large font size in both the order list and order details. We can also wait for the final design to make any accessibility enhancements.

large font size small font size
Simulator Screenshot - iPad (A16) - 2025-07-04 at 08 19 33 Simulator Screenshot - iPad (A16) - 2025-07-04 at 08 19 45

@@ -0,0 +1,32 @@
import Foundation

public enum SalesChannelType {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: how about just SalesChannel? Doesn't seem like there is a naming conflict, just wondering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No special reason, updated here! 98aaff9

public var description: String {
switch self {
case .pointOfSale:
return NSLocalizedString("saleschanneltype.pos.description",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: or salesChannel.pos.description depending on the final name of the enum

Suggested change
return NSLocalizedString("saleschanneltype.pos.description",
return NSLocalizedString("salesChannelType.pos.description",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

init(order: Order,
status: OrderStatus?,
calendar: Calendar = .current) {
self.order = order
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I see a salesChannel: String? above, maybe can set either the sales channel description or enum to the salesChannel property here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True 🤔 , I've updated it on 7df616b but since the enum is in Networking I've continued using a String, in which case we can remove also some duplication around the description


// Then
XCTAssertEqual(viewModel.salesChannel, OrderListCellViewModel.Localization.salesChannelPOSText)
XCTAssertEqual(viewModel.salesChannel, "POS")
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 this is fine, a bunch of unit tests are already based on the English locale assumption.

@iamgabrielma
Copy link
Contributor Author

Thanks for the review!

Unrelated to this PR, the POS badge seems to be truncated in large font size in both the order list and order details. We can also wait for the final design to make any accessibility enhancements.

Good catch, specially the order list details seem to break very easily in larger accessibility sizes, I don't think we'll get much additional design feedback around these badges apart from the recently updated colors, so I'll take a look into this next 👍

@iamgabrielma iamgabrielma enabled auto-merge July 7, 2025 02:38
@iamgabrielma iamgabrielma merged commit 300a2f9 into trunk Jul 7, 2025
13 checks passed
@iamgabrielma iamgabrielma deleted the task/move-sales-channel-representation-to-networking branch July 7, 2025 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: order details Related to order details. feature: order list Related to the order list. feature: POS type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants