From b919c3e1fa42d4361d012f5a857dc6a7517d3cf9 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Tue, 2 Dec 2025 16:55:25 +0000 Subject: [PATCH 1/2] Ignore time based sync eligibility inside POS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we set a max catalog age of zero in a refresh, we’re forcing a sync – e.g. because of a pull to refresh, or a full sync from settings, or after a purchase. When we do this, we should ignore temporal reasons for not syncing. Otherwise, it’s possible to get into a position where syncs don’t work any more even though they should. --- .../Tools/POS/POSCatalogSyncCoordinator.swift | 24 +++++--- .../POS/POSCatalogSyncCoordinatorTests.swift | 57 +++++++++++++++++-- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift index 48784c45919..878a00f1a03 100644 --- a/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift +++ b/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift @@ -262,7 +262,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { /// - maxAge: Maximum age before a sync is considered stale /// - Returns: True if a sync should be performed private func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async throws -> Bool { - guard try await checkSyncEligibility(for: siteID) else { + guard try await checkSyncEligibility(for: siteID, ignoreTemporalCriteria: maxAge == .zero) else { DDLogInfo("πŸ“‹ POSCatalogSyncCoordinator: Full sync skipped - site \(siteID) is not eligible") return false } @@ -402,7 +402,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { } private func shouldPerformIncrementalSync(for siteID: Int64, maxAge: TimeInterval) async throws -> Bool { - guard try await checkSyncEligibility(for: siteID) else { + guard try await checkSyncEligibility(for: siteID, ignoreTemporalCriteria: maxAge == .zero) else { DDLogInfo("πŸ“‹ POSCatalogSyncCoordinator: Incremental sync skipped - site \(siteID) is not eligible") return false } @@ -600,7 +600,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { // Check sync eligibility do { - let eligibility = try await syncEligibility(for: siteID) + let eligibility = try await syncEligibility(for: siteID, ignoreTemporalCriteria: maxAge == .zero) if case .ineligible(let reason) = eligibility { return reason.skipReason } @@ -634,7 +634,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { // Check sync eligibility do { - let eligibility = try await syncEligibility(for: siteID) + let eligibility = try await syncEligibility(for: siteID, ignoreTemporalCriteria: maxAge == .zero) if case .ineligible(let reason) = eligibility { return reason.skipReason } @@ -724,8 +724,8 @@ private extension POSCatalogSyncCoordinator { // MARK: - Sync Eligibility /// Checks if sync is eligible for the given site based on catalog eligibility and temporal criteria - func checkSyncEligibility(for siteID: Int64) async throws -> Bool { - switch try await syncEligibility(for: siteID) { + func checkSyncEligibility(for siteID: Int64, ignoreTemporalCriteria: Bool = false) async throws -> Bool { + switch try await syncEligibility(for: siteID, ignoreTemporalCriteria: ignoreTemporalCriteria) { case .eligible(reason: let reason): switch reason { case .neverSynced: @@ -733,7 +733,10 @@ private extension POSCatalogSyncCoordinator { case .openedRecently(daysSinceOpened: let daysSinceOpened): DDLogInfo("πŸ“‹ POSCatalogSyncCoordinator: Site \(siteID) eligible (last opened \(daysSinceOpened) days ago)") case .recentFirstSync(daysSinceFirstSync: let daysSinceFirstSync): - DDLogInfo("πŸ“‹ POSCatalogSyncCoordinator: Site \(siteID) eligible (within grace period: \(daysSinceFirstSync) days since first sync)") + DDLogInfo("πŸ“‹ POSCatalogSyncCoordinator: Site \(siteID) eligible (within grace period: " + + "\(daysSinceFirstSync) days since first sync)") + case .ignoredTemporalEligibilityCheck: + DDLogInfo("πŸ“‹ POSCatalogSyncCoordinator: Site \(siteID) eligible (ignored temporal criteria check)") } return true case .ineligible(reason: let reason): @@ -750,11 +753,15 @@ private extension POSCatalogSyncCoordinator { } } - func syncEligibility(for siteID: Int64) async throws -> SyncEligibility { + func syncEligibility(for siteID: Int64, ignoreTemporalCriteria: Bool) async throws -> SyncEligibility { guard try await catalogEligibilityChecker.catalogEligibility(for: siteID) == .eligible else { return .ineligible(reason: .notEligibleForLocalCatalog) } + guard !ignoreTemporalCriteria else { + return .eligible(reason: .ignoredTemporalEligibilityCheck) + } + // Then check temporal eligibility (30-day criteria) let firstSyncDate = siteSettings.getFirstPOSCatalogSyncDate(siteID: siteID) let lastOpenedDate = siteSettings.getPOSLastOpenedDate(siteID: siteID) @@ -794,6 +801,7 @@ private extension POSCatalogSyncCoordinator { case neverSynced case openedRecently(daysSinceOpened: Int) case recentFirstSync(daysSinceFirstSync: Int) + case ignoredTemporalEligibilityCheck } enum IneligibleReason { diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift index c2b0d11b178..ce822f223a2 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift @@ -884,6 +884,53 @@ extension POSCatalogSyncCoordinatorTests { #expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 1) } + @Test func performFullSyncIfApplicable_with_zero_maxAge_ignores_temporal_eligibility() async throws { + // Given - user past 30-day grace period with no recent open (normally ineligible) + let fortyDaysAgo = Date().addingTimeInterval(-40 * 24 * 60 * 60) + mockSiteSettings.mockFirstPOSCatalogSyncDate = fortyDaysAgo + // No lastPOSOpenedDate set (never opened recently) + + let coordinator = POSCatalogSyncCoordinator( + fullSyncService: mockSyncService, + incrementalSyncService: mockIncrementalSyncService, + grdbManager: grdbManager, + catalogEligibilityChecker: MockPOSLocalCatalogEligibilityService(), + siteSettings: mockSiteSettings + ) + try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: fortyDaysAgo) + mockSyncService.startFullSyncResult = .success(POSCatalog(products: [], variations: [], syncDate: .now)) + + // When - maxAge is zero (forced sync, e.g. pull to refresh) + try await coordinator.performFullSyncIfApplicable(for: sampleSiteID, maxAge: .zero) + + // Then - sync should proceed despite temporal ineligibility + #expect(mockSyncService.startFullSyncCallCount == 1) + } + + @Test func performIncrementalSyncIfApplicable_with_zero_maxAge_ignores_temporal_eligibility() async throws { + // Given - user past 30-day grace period with no recent open (normally ineligible) + let fortyDaysAgo = Date().addingTimeInterval(-40 * 24 * 60 * 60) + mockSiteSettings.mockFirstPOSCatalogSyncDate = fortyDaysAgo + // No lastPOSOpenedDate set (never opened recently) + + let coordinator = POSCatalogSyncCoordinator( + fullSyncService: mockSyncService, + incrementalSyncService: mockIncrementalSyncService, + grdbManager: grdbManager, + catalogEligibilityChecker: MockPOSLocalCatalogEligibilityService(), + siteSettings: mockSiteSettings + ) + // Set up database with full sync date (required for incremental sync) + try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: fortyDaysAgo) + mockIncrementalSyncService.startIncrementalSyncResult = .success(POSCatalog(products: [], variations: [], syncDate: .now)) + + // When - maxAge is zero (forced sync, e.g. after a purchase) + try await coordinator.performIncrementalSyncIfApplicable(for: sampleSiteID, maxAge: .zero) + + // Then - sync should proceed despite temporal ineligibility + #expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 1) + } + // MARK: - isSyncStale Tests @Test func isSyncStale_returns_true_when_no_full_sync_performed() async throws { @@ -1394,8 +1441,9 @@ extension POSCatalogSyncCoordinatorTests { connectivityObserver: nil ) - // When - Try to perform sync - try? await sut.performFullSyncIfApplicable(for: sampleSiteID, maxAge: .zero) + // When - Try to perform sync with non-zero maxAge (temporal criteria are checked) + // Note: maxAge of .zero would bypass temporal eligibility checks + try? await sut.performFullSyncIfApplicable(for: sampleSiteID, maxAge: sampleMaxAge) // Then - Should track pos_not_opened_30_days let syncSkipped = mockAnalytics.trackedEvents.first { $0.eventName == "local_catalog_sync_skipped" } @@ -1428,8 +1476,9 @@ extension POSCatalogSyncCoordinatorTests { connectivityObserver: nil ) - // When - Try to perform incremental sync - try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, maxAge: .zero) + // When - Try to perform incremental sync with non-zero maxAge (temporal criteria are checked) + // Note: maxAge of .zero would bypass temporal eligibility checks + try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, maxAge: sampleMaxAge) // Then - Should track pos_not_opened_30_days let syncSkipped = mockAnalytics.trackedEvents.first { $0.eventName == "local_catalog_sync_skipped" } From f062cb2efbb07fc0d85774dfff0e6b86329df6d2 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Tue, 2 Dec 2025 17:27:43 +0000 Subject: [PATCH 2/2] Add sync fix to 23.9 release notes --- RELEASE-NOTES.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 2d8737aeb2f..cfe24a44577 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -3,7 +3,7 @@ 23.9 ----- - +- [*] Fixed possible sync issue in POS (https://github.com/woocommerce/woocommerce-ios/pull/16423) 23.8 -----