-
Notifications
You must be signed in to change notification settings - Fork 121
[POS as a tab i1] Cache POS tab visibility per site for initial value while checking eligibility async #15753
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
…. DI `currentDate` to `AppSettingsAction.loadPOSTabVisibility` for unit tests.
…er selecting the tab with the tab being shown from initial value.
|
|
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 for the improvement!
I think it could be slightly improved by making a call to the initial cache synchronous and eliminating a delay entirely.
| guard let self, let posEligibilityChecker else { return } | ||
| let eligibility = await posEligibilityChecker.checkEligibility() | ||
| let isPOSTabVisible = eligibility == .eligible | ||
| async let initialVisibility = posEligibilityChecker.checkInitialVisibility() |
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.
There's still a slight delay for the Point of Sale tab to appear.
updateTabViewControllers(isPOSTabVisible: posEligibilityChecker.checkInitialVisibility())Simulator.Screen.Recording.-.iPad.Air.13-inch.M3.-.2025-06-16.at.15.21.11.mp4
If we look at checkInitialVisibility implementation, it could be synchronous, and we could call it outside the posEligibilityCheckTask, allowing for the tab to appear as soon as possible. It would require getting around dispatching AppSettingsStore and accessing the data directly, but other than that, it would allow for a smoother appearance:
Simulator.Screen.Recording.-.iPad.Air.13-inch.M3.-.2025-06-16.at.15.31.22.mp4
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's interesting that I didn't observe the slight delay in iOS 18.4 simulator, as in the screencast 1:06. But then I tried the same simulator type in iOS 18.5, and I see the delay now. I tried the device type as in my original screencast but in iOS 18.5 and see the same, so likely a behavior difference (probably about the timing of Task) between iOS versions. I'm looking into using a service for loading the initial visibility to make it sync.
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 for the suggestion, I updated the PR with a few commits to move the async initial value to a synchronous call from a new service in Yosemite POSEligibilityService behind a protocol for unit testing. Also moved the AppSettingsAction/store changes to the service for consistency. Please see if the delay is fixed after the initial load when you get a chance, thanks!
| return onCompletion(nil) | ||
| } | ||
|
|
||
| let threeDaysInSeconds: TimeInterval = 3 * 24 * 60 * 60 // 3 days in seconds. |
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 that three days is more than safe. I would even go as far as to say that this cache could always be considered valid since we update it every time the app gets launched.
You can always find a way to break POS, even with a 3-day cache:
- Configure the wp-admin settings to support POS
- Launch Woo App, open POS
- Change the wp-admin settings to stop supporting POS
- Continue using Woo POS, observe some failures
Therefore, we could choose not to overthink it and always return the last tab visibility value for simplicity.
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.
Yea good point, this edge case can already happen in production and not worth adding complexity for. I removed the date check in 396d3a8, so that it's just using the cached value for initial tab visibility.
…to load initial POS tab visibility from cached app settings synchronously.
…CachedPOSTabVisibility` to better describe what it does.
…e` for consistency with updated test cases.
…cribe what it does.
|
Updated the PR for another pass! |
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.
Works perfectly now! Thanks for addressing the comments. Nice job on the functionality. 👏
# Conflicts: # WooCommerce/WooCommerce.xcodeproj/project.pbxproj

Closes WOOMOB-611
Why
Previously, POS eligibility is checked async based on a few API requests for displaying the POS tab. This results in a noticeable delay when the POS tab is shown (just the initial site settings API request for store country/currency already takes ~1.8 on my end for a Pressable site via Jetpack connection) after every launch / login / store switching. This PR aims to reduce the delay after the first POS eligibility check based on the assumption that merchants do not change store configurations that could affect POS eligibility often, by caching the POS eligibility and displaying the initial tab based on the cached value while checking POS eligibility async.
Please feel free to suggest other ways to optimize the POS tab visibility check, and any implementation details in this PR.
Description
This pull request introduces functionality to manage the visibility of the POS (Point of Sale) tab based on eligibility and initial visibility checks. Key changes include updates to caching POS eligibility via app settings, new methods for managing POS tab visibility, and associated unit tests.
Updates to
GeneralStoreSettings:isPOSTabVisibleandlastPOSTabVisibilityCheckDate, to theGeneralStoreSettingsstruct for tracking POS tab visibility and the last check date. (Storage/Storage/Model/GeneralStoreSettings.swift: [1] [2] [3] [4] [5]Copiableextension forGeneralStoreSettingsto include the new properties for copying and merging data. (Storage/Storage/Model/Copiable/Models+Copiable.generated.swift: [1] [2] [3]POS Tab Visibility Management:
checkInitialVisibility, inPOSTabEligibilityCheckerto determine the initial visibility of the POS tab without relying on network requests. (WooCommerce/Classes/ViewRelated/Dashboard/Settings/POS/POSTabEligibilityChecker.swift: [1] [2]MainTabBarControllerto update the visibility of the POS tab based on initial visibility and eligibility checks, and to persist visibility state in app settings. (WooCommerce/Classes/ViewRelated/MainTabBarController.swift: [1] [2] [3]Unit Tests:
MainTabBarControllerTeststo verify POS tab visibility behavior under various conditions, including initial visibility and eligibility changes. (WooCommerce/WooCommerceTests/ViewRelated/MainTabBarControllerTests.swift: WooCommerce/WooCommerceTests/ViewRelated/MainTabBarControllerTests.swiftR454-R525)POSEligibilityCheckerandStoresManagerto support testing of POS tab visibility logic. (WooCommerce/WooCommerceTests/Mocks/MockPOSEligibilityChecker.swift: [1]WooCommerce/WooCommerceTests/ViewRelated/MainTabBarControllerTests.swift: [2]storesManageras a dependency to various components, such asPOSTabCoordinator,HubMenuCoordinator, andHubMenuViewController, to manage POS tab-related actions. (WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swift: [1]WooCommerce/Classes/ViewRelated/Hub Menu/HubMenuCoordinator.swift: [2]WooCommerce/Classes/ViewRelated/Hub Menu/HubMenuViewController.swift: [3]WooCommerce/Classes/ViewRelated/Hub Menu/HubMenuViewModel.swift: [4]Steps to reproduce
Prerequisite: a WPCOM account that has at least two connected stores, one eligible for POS and the other one not eligible.
Testing information
MainTabBarControllertest case)Screenshots
Simulator.Screen.Recording.-.iPad.A16.-.2025-06-16.at.16.48.02.mp4
RELEASE-NOTES.txtif necessary.