-
Notifications
You must be signed in to change notification settings - Fork 121
[POS Orders] Woo version eligibility checker #15868
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
Conversation
|
|
# Conflicts: # WooCommerce/Classes/ViewRelated/Orders/Order Details/Order Summary Section/SummaryTableViewCell.swift
|
|
||
| private extension OrderSalesChannelEligibilityChecker { | ||
| enum Constants { | ||
| static let pluginName = "WooCommerce" |
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.
We can wait until PluginsService refactoring is done to update this, as won't work as soon as requires the string is the plugin path, rather than the plugin name 👍
|
@staskus I just realized Jaclyn is AFK, adding you as well as reviewer 🙇 |
staskus
left a comment
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!
I think the solution could be improved, especially given that the labels don't appear by themselves the first time orders view is loading.
I would seriously consider keeping a previous conclusion and relying on the existence of the created_via field to determine whether to show or not to show POS. It would simplify the implementation and make the user experience faster.
| subtitleLabel.text = viewModel.subtitle | ||
| salesChannelLabel.text = viewModel.salesChannel | ||
| salesChannelLabel.isHidden = (salesChannelLabel.text == nil) | ||
| if viewModel.isEligibleForDisplayingSalesChannelPOSBadge { |
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 agree with this comment https://linear.app/a8c/issue/WOOMOB-704/woo-mobilepos-orders-wc-99-version-check#comment-c873cc1e
It seems simpler just to check the existence of salesChannel label, and no need for additional WooCommerce version checks?
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.
It seems simpler just to check the existence of salesChannel label, and no need for additional WooCommerce version checks?
That's a good point, and I'm definitely leaning back into it.
The intention of adding the eligibility checker now was to remove the badge from the UI in the edge case that they the merchants happen to revert to a non-supported WC version after already having used POS, however, it is true that if they do so the order model in the DB doesn't change, and the order would still have this property (just not for new orders) so there is really no reason to hide pre-existing badges.
This would simplify the design since there is no need for version check in i1, we just check if the property exists in the order and render the badge in consequence 🤔
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'll discuss with Sam before updating or closing this issue 👍
| query: Self.createQuery(siteID: siteID, | ||
| filters: filters)) | ||
| Task { | ||
| let checker = OrderSalesChannelEligibilityChecker(featureFlagService: featureFlagService, storageManager: storageManager) |
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.
It could be good to inject it into OrderListViewModel to have a mocked version within OrderListViewModelTests. I remember encountering a couple of examples where using real implementations slowed down or broke existing tests.
| } | ||
|
|
||
| return OrderListCellViewModel(order: order, currencySettings: ServiceLocator.currencySettings) | ||
| return OrderListCellViewModel(order: order, |
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.
....(this takes a bit to load due other async tasks in the task group)
It takes time to load, but also the rows are not updated after loading is done. Since we're in the UIKit world, we need to manually reload the data once the performEligibilityCheck finishes loading.
ScreenRecording_07-08-2025.17-33-26_1.MP4
|
Thanks for the review and suggestions @staskus , we've decided at p1752034631867549-slack-C070SJRA8DP that indeed having a WC version check here isn't necessary, and keeping the previous implementation is fine. I'll close this one 🙇 |
|
@iamgabrielma nice, thank you! |

Closes WOOMOB-704
Description
This PR adds an eligibility checker that will handle if we should show the POS badge in order list and order details summary. The only requirement is that the minimum WooCommerce version is 9.9.0, as it's where
created_viawas introduced.Initially this check was set directly in the cells for simplicity, as we only checked for feature flag, however as soon we introduced checking the WooCommerce version as well, this would trigger an unnecessary trip to disk for each cell render, so I moved the check up to the view models/datasource.
Testing information
Screen.Recording.2025-07-07.at.12.01.02.mov
pointOfSaleOrdersi1should hide the POS labels: