-
Notifications
You must be signed in to change notification settings - Fork 121
[Woo POS][Local Catalog] Add a periodic sync when the app is in the foreground #16246
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][Local Catalog] Add a periodic sync when the app is in the foreground #16246
Conversation
|
|
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.
LGTM 🚢
| func stop() { | ||
| guard isRunning else { return } | ||
|
|
||
| DDLogInfo("🛑 ForegroundPOSCatalogSyncDispatcher: Stopping foreground sync dispatcher") |
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.
Consider if passing the siteID in the DDLog here would be useful: When I switched from a POS-eligible store to a non-eligible one the only notice logged was that the timer stopped. Perhaps not necessary as we would see the siteID changing from context of other events.
|
|
||
| DDLogInfo("🔄 ForegroundPOSCatalogSyncDispatcher: Starting sync for site \(siteID)") | ||
|
|
||
| Task.detached(priority: .utility) { |
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.
Good use of detached and priority queue!
| private var posTabVisibilityChecker: POSTabVisibilityCheckerProtocol? | ||
| private var posEligibilityCheckTask: Task<Void, Never>? | ||
| private var posCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol? | ||
| private lazy var posSyncDispatcher = ForegroundPOSCatalogSyncDispatcher() |
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'm guessing this needs to be lazy because the POS tab visibility and eligibility? Otherwise we would be paying for initializing the sync dispatcher despite the merchant not being able to use POS.
…odic-sync-when-the-app-is-in-foreground

WOOMOB-1517
Related PR: #16244
Description
In #16244 I implemented a functionality to run hourly POS sync in the background.
In this PR, I'm implementing a similar functionality that should run when the app is in the foreground.
Outside the scope:
Solution
ForegroundPOSCatalogSyncDispatcherto be next to the background dispatcher for easier discoverabilityForegroundPOSCatalogSyncDispatcheruses DispatchSourceTimer, which is an efficient way to run a timer in the background. It has parameters such asleeway, which allows the system to prioritize other ongoing tasks and perform the work without precision, since precision is not that important for us.ForegroundPOSCatalogSyncDispatcherobserves the app going to and from the background to stop and restart the timerForegroundPOSCatalogSyncDispatcherstarts, the first sync is executed almost immediately, the next after 1 hourPOSCatalogSyncCoordinatordecides whether to do incremental or full sync (if 24h have passed)ForegroundPOSCatalogSyncDispatcheris started when the POS tab becomes visible and stopped if the POS tab becomes invisible, or if the app no longer has a coordinator or a siteID (logged out)Steps to reproduce
It should be much easier to test compared to system background tasks! Important to verify that timer starts in expected times when logging in, launching app, going into foreground, switching sites...
Prerequisites:
syncIntervalinForegroundPOSCatalogSyncDispatcher, for example 30 secondsForegroundPOSCatalogSyncDispatcherforDDLog..callssyncIntervalit should repeatsyncIntervalwhen the system notices that the app is no longer logged inTesting information
Tested using iPad Air M2 26.0 simulator
RELEASE-NOTES.txtif necessary.