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" } 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 -----