-
Notifications
You must be signed in to change notification settings - Fork 121
Refactor POS tab: separate visibility and eligibility logic while removing POS as a tab i2 feature flag #16173
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
Refactor POS tab: separate visibility and eligibility logic while removing POS as a tab i2 feature flag #16173
Conversation
…lity and eligibility.
Generated by 🚫 Danger |
|
|
…CI uses phone simulator.
…ice` mocks to be reusable.
iamgabrielma
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.
Tests well, switching between an US and UK store loads the site products correctly. I just saw the POS tab glitching for a moment when I first switched stores (POS tab becoming invisible) but was restored immediately, which I guess happened while checking the requirements and waiting for network response. All good!
Tested in iPad Pro 12.9 inch - iOS 26.0
| return .ineligible(reason: .unsupportedInCIABSites) | ||
| } | ||
|
|
||
| guard #available(iOS 17.0, *) else { |
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.
Nit: We can remove this availability check as well and the unsupportedIOSVersion case as ineligible reason
| ) | ||
| } | ||
|
|
||
| @available(iOS 17.0, *) |
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.
Nit: We can remove this availability check as well
| stockStatusKey: "")) | ||
| } | ||
|
|
||
| @available(iOS 17.0, *) |
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.
Nit: We can remove this availability check as well
…e-flag-with-refactoring # Conflicts: # Modules/Sources/Experiments/FeatureFlag.swift
…ed `POSIneligibleReason` cases.
I believe this is due to the recent CIAB change 8b32ea3 that delays the tab visibility check until the full site becomes available, which takes place a bit after the site ID becomes available, when we used to check the tab visibility. This delay causes the tab visibility "glitching" (changing back and forth). I'll see if I can fix this separately for Kiwi's review. |
…e-flag-with-refactoring # Conflicts: # WooCommerce/WooCommerce.xcodeproj/project.pbxproj
|
Update from #16173 (comment):
As the latest visibility check requires the CIAB check from the full site Lines 115 to 117 in 2c9fd8a
I'm not sure if there's a good way to perform this check before the full site becomes available. A workaround is to skip the CIAB check when the site ID first becomes available, then include the CIAB check when the full site becomes available later. However, this could also result in a glitch if the visibility changes in the second check. It feels like a tradeoff from the CIAB change that we are accepting. Once the visibility check is performed from the full site, it should be cached and used for initial visibility next time when the full site is available. Please feel free to share any ideas for making the POS visibility available earlier like before the CIAB change @itsmeichigo @RafaelKayumov. |

For WOOMOB-869
Just one review is required.
Description
This PR refactors the POS tab logic to separate visibility and eligibility concerns, following the architecture discussion in PR #16153. It also removes the
pointOfSaleAsATabi2feature flag, which contributes to ~1000 deletion diffs.Changes Made
POSTabEligibilityCheckerto focus purely on eligibility check based on plugin version and feature switch eligibility, andPOSTabVisibilityCheckernow handles the visibility logic including CIAB checkspointOfSaleAsATabi2feature flag, andLegacyPOSTabEligibilityChecker+LegacyPOSTabEligibilityCheckerTeststhat were used when the feature flag was disabledArchitecture Improvements
Steps to reproduce
Testing information
I tested the above on iPad, and on iPhone to ensure that POS tab wasn't shown.
RELEASE-NOTES.txtif necessary.