[Local catalog] Follow criteria for syncs #16281
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add four new AppSettingsAction cases for getting/setting POS dates - Implement action handlers in AppSettingsStore using SiteSpecificAppSettingsStoreMethods - setPOSLastOpenedDate/getPOSLastOpenedDate for tracking last opened date - setFirstPOSCatalogSyncDate/getFirstPOSCatalogSyncDate for tracking first sync This provides a clean action-based API for accessing the date tracking infrastructure added in the previous commit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
When the POS tab is selected, dispatch an action to record the current date as the last opened date. This will be used for sync eligibility checking to ensure syncs only continue for active POS users. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add catalogEligibilityChecker and siteSettings parameters to the coordinator's initializer: - catalogEligibilityChecker: optional closure to check catalog eligibility - siteSettings: protocol for accessing POS date settings (defaults to new instance) The coordinator creates its own SiteSpecificAppSettingsStoreMethods instance by default (following the pattern used by POSSearchHistoryService), so no changes are needed in AuthenticatedState. Updated tests to pass mock siteSettings explicitly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add eligibility checking to POSCatalogSyncCoordinator.performSmartSync: - Check catalog eligibility first (via optional closure) - Check temporal eligibility based on 30-day criteria: * New users: eligible for 30 days from first sync * Existing users (>30 days from first sync): must have opened POS within 30 days - Record first sync date after successful sync - Log eligibility decisions for debugging The coordinator now enforces that background syncs only continue for sites where POS is actively being used, preventing indefinite syncing for abandoned POS installations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add setCatalogEligibilityChecker method to allow setting the eligibility checker after initialization, and wire it up in MainTabBarController. Since POSTabCoordinator (which has the catalog eligibility service) is created after the sync coordinator in AuthenticatedState, we set the eligibility checker closure after the tab coordinator is initialized. The closure refreshes and checks the eligibility state from POSLocalCatalogEligibilityService, providing the catalog eligibility check needed for the 30-day sync criteria. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Make setCatalogEligibilityChecker nonisolated to avoid actor isolation issues when called from MainTabBarController. The method internally uses Task to safely set the property on the actor. The small race condition window (setter might not complete before first sync) is acceptable because: - The temporal 30-day eligibility check always works regardless - Worst case: first sync might skip catalog eligibility check - The setter completes nearly instantly in practice Moving the coordinator to MainTabBarController would break background sync access via ServiceLocator.stores, so this approach is preferred. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Added 11 tests covering all eligibility scenarios: - Catalog ineligibility blocks sync - New users can sync initially - First sync date is recorded once - 30-day grace period for new users - Existing users past grace period need recent POS opens - Boundary conditions (exactly 30 days) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ecker Changes: - Add eligibility checks to performFullSyncIfApplicable and performIncrementalSyncIfApplicable - Record first sync date after all sync types (not just smart sync) - Remove catalogSizeChecker parameter from POSCatalogSyncCoordinator (catalog size checking now handled by eligibilityChecker) - Remove catalogSizeLimit parameter (no longer needed) - Remove all catalog size tests (functionality now tested via eligibility service) - Simplify shouldPerformFullSync and shouldPerformIncrementalSync methods The catalog eligibility checker (from POSLocalCatalogEligibilityService) now handles both catalog size checks and temporal eligibility checks, eliminating the need for separate size checking logic in the coordinator. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
joshheald
force-pushed
the
woomob-1518-woo-poslocal-catalog-follow-criteria-for-background-and
branch
from
1a056a2 to
f22ea9e
Compare
Generated by 🚫 Danger |
|
|
Changes: - Move POSLocalCatalogEligibilityService to Yosemite module - Move POSLocalCatalogEligibilityServiceProtocol to Yosemite - Change service to accept boolean feature flag instead of FeatureFlagService - Make POSLocalCatalogEligibilityServiceProtocol inherit from POSCatalogEligibilityChecking - Update StoresManager to use POSLocalCatalogEligibilityServiceProtocol type - Remove setCatalogEligibilityChecker() method from sync coordinator - Change catalogEligibilityChecker property to let (immutable) - Create service in AuthenticatedState alongside sync coordinator - Pass service directly in coordinator init (no late binding) - Simplify MainTabBarController (remove async Task creation) - Update all mocks to remove setCatalogEligibilityChecker() method - Make service properties public for Yosemite module export - Update tests to use boolean parameter This eliminates the nonisolated setter workaround and fixes the wiring issue where the eligibility checker was always nil. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
joshheald
force-pushed
the
woomob-1518-woo-poslocal-catalog-follow-criteria-for-background-and
branch
from
0492912 to
f6df1ce
Compare
|
@iamgabrielma This is a long one, sorry about that. Improving the init order for checking eligibility took a lot of attempts, and I also had to make sure all the fiddly details were working correctly. No rush on this, take your time. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR updates the
POSCatalogSyncCoordinatorto check both catalog eligibility, and how long it's been since the POS was opened, before allowing any syncs.The full criteria are:
From catalog eligibility checker
Newly added in sync coordinator
Testing information
Syncs happen on longer schedules than is convenient for testing. I tend to change the code to reduce the delay for each type, to test them.
This PR doesn't include all the incremental sync triggers. It's best to rely on scheduled syncs for testing.
Places to tweak schedules
ForegroundPOSCatalogSyncDispatcher.syncIntervalalready uses seconds, just reduce itPOSCatalogSyncCoordinatorProtocol.performSmartSync(for:)defines 24 hours in seconds for the threshold to do a full sync, you can reduce that. If you don't do that, it'll still do an incremental syncPOSCatalogSyncCoordinator.checkSyncEligibility(for:)uses.daya lot to decide whether we should block syncs for people who don't open POS. You can change those to.secondto make it happen a lot faster!BackgroundTaskSchedule.getNextTask()to always return.posCatalogSync, then put a breakpoint on line 55 ofBackgroundTaskRefreshDispatcher,and background the app. When you hit the breakpoint, use lldb to execute this:e -l objc -- (void)[[BGTaskScheduler sharedScheduler] _simulateLaunchForTaskWithIdentifier:@"com.automattic.woocommerce.refresh.pos.catalog.sync"]then when it finishes, continue execution – the sync task should run.Scenarios I covered:
Note that there's a race condition on switching stores. The POS assumes you're ineligible for local catalog if you open POS before we're able to check the catalog size. That result isn't cached, so if you close and re-open, it will rectify that issue.
Once we remove the async check for catalog size, the race condition will go away too.
RELEASE-NOTES.txtif necessary.