diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift index 1a4a04b0ca6..983d5bd80cc 100644 --- a/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift +++ b/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift @@ -15,6 +15,14 @@ public protocol POSCatalogSyncCoordinatorProtocol { /// - maxAge: Maximum age before a sync is considered stale /// - Returns: True if a sync should be performed func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async -> Bool + + /// Performs an incremental sync if applicable based on sync conditions + /// - Parameters: + /// - siteID: The site ID to sync catalog for + /// - forceSync: Whether to bypass age checks and always sync + /// - Throws: POSCatalogSyncError.syncAlreadyInProgress if a sync is already running for this site + //periphery:ignore - remove ignore comment when incremental sync is integrated with POS + func performIncrementalSyncIfApplicable(for siteID: Int64, forceSync: Bool) async throws } public enum POSCatalogSyncError: Error, Equatable { @@ -23,26 +31,23 @@ public enum POSCatalogSyncError: Error, Equatable { public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { private let fullSyncService: POSCatalogFullSyncServiceProtocol - private let persistenceService: POSCatalogPersistenceServiceProtocol + private let incrementalSyncService: POSCatalogIncrementalSyncServiceProtocol private let grdbManager: GRDBManagerProtocol + private let maxIncrementalSyncAge: TimeInterval - /// Tracks ongoing syncs by site ID to prevent duplicates + /// Tracks ongoing full syncs by site ID to prevent duplicates private var ongoingSyncs: Set = [] + /// Tracks ongoing incremental syncs by site ID to prevent duplicates + private var ongoingIncrementalSyncs: Set = [] public init(fullSyncService: POSCatalogFullSyncServiceProtocol, - grdbManager: GRDBManagerProtocol) { - self.fullSyncService = fullSyncService - self.persistenceService = POSCatalogPersistenceService(grdbManager: grdbManager) - self.grdbManager = grdbManager - } - - //periphery:ignore - used for tests to inject persistence service - init(fullSyncService: POSCatalogFullSyncServiceProtocol, - persistenceService: POSCatalogPersistenceServiceProtocol, - grdbManager: GRDBManagerProtocol) { + incrementalSyncService: POSCatalogIncrementalSyncServiceProtocol, + grdbManager: GRDBManagerProtocol, + maxIncrementalSyncAge: TimeInterval = 300) { self.fullSyncService = fullSyncService - self.persistenceService = persistenceService + self.incrementalSyncService = incrementalSyncService self.grdbManager = grdbManager + self.maxIncrementalSyncAge = maxIncrementalSyncAge } public func performFullSync(for siteID: Int64) async throws { @@ -61,7 +66,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { DDLogInfo("🔄 POSCatalogSyncCoordinator starting full sync for site \(siteID)") - let catalog = try await fullSyncService.startFullSync(for: siteID) + _ = try await fullSyncService.startFullSync(for: siteID) DDLogInfo("✅ POSCatalogSyncCoordinator completed full sync for site \(siteID)") } @@ -89,6 +94,40 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { return shouldSync } + public func performIncrementalSyncIfApplicable(for siteID: Int64, forceSync: Bool) async throws { + if ongoingIncrementalSyncs.contains(siteID) { + DDLogInfo("⚠️ POSCatalogSyncCoordinator: Incremental sync already in progress for site \(siteID)") + throw POSCatalogSyncError.syncAlreadyInProgress(siteID: siteID) + } + + guard let lastFullSyncDate = await lastFullSyncDate(for: siteID) else { + DDLogInfo("📋 POSCatalogSyncCoordinator: No full sync performed yet for site \(siteID), skipping incremental sync") + return + } + + if !forceSync, let lastIncrementalSyncDate = await lastIncrementalSyncDate(for: siteID) { + let age = Date().timeIntervalSince(lastIncrementalSyncDate) + + if age <= maxIncrementalSyncAge { + return DDLogInfo("📋 POSCatalogSyncCoordinator: Last incremental sync for site \(siteID) was \(Int(age))s ago, sync not needed") + } + } + + ongoingIncrementalSyncs.insert(siteID) + + defer { + ongoingIncrementalSyncs.remove(siteID) + } + + DDLogInfo("🔄 POSCatalogSyncCoordinator starting incremental sync for site \(siteID)") + + try await incrementalSyncService.startIncrementalSync(for: siteID, + lastFullSyncDate: lastFullSyncDate, + lastIncrementalSyncDate: lastIncrementalSyncDate(for: siteID)) + + DDLogInfo("✅ POSCatalogSyncCoordinator completed incremental sync for site \(siteID)") + } + // MARK: - Private private func lastFullSyncDate(for siteID: Int64) async -> Date? { @@ -102,6 +141,17 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { } } + private func lastIncrementalSyncDate(for siteID: Int64) async -> Date? { + do { + return try await grdbManager.databaseConnection.read { db in + return try PersistedSite.filter(key: siteID).fetchOne(db)?.lastCatalogIncrementalSyncDate + } + } catch { + DDLogError("⛔️ POSCatalogSyncCoordinator: Error loading site \(siteID) for incremental sync date: \(error)") + return nil + } + } + private func siteExistsInDatabase(siteID: Int64) -> Bool { do { return try grdbManager.databaseConnection.read { db in diff --git a/Modules/Tests/YosemiteTests/Mocks/MockPOSCatalogIncrementalSyncService.swift b/Modules/Tests/YosemiteTests/Mocks/MockPOSCatalogIncrementalSyncService.swift new file mode 100644 index 00000000000..c07f42d5eb9 --- /dev/null +++ b/Modules/Tests/YosemiteTests/Mocks/MockPOSCatalogIncrementalSyncService.swift @@ -0,0 +1,46 @@ +import Foundation +@testable import Yosemite + +final class MockPOSCatalogIncrementalSyncService: POSCatalogIncrementalSyncServiceProtocol { + var startIncrementalSyncResult: Result = .success(()) + + private(set) var startIncrementalSyncCallCount = 0 + private(set) var lastSyncSiteID: Int64? + private(set) var lastFullSyncDate: Date? + private(set) var lastIncrementalSyncDate: Date? + + private var syncContinuation: CheckedContinuation? + private var shouldBlockSync = false + + func startIncrementalSync(for siteID: Int64, lastFullSyncDate: Date, lastIncrementalSyncDate: Date?) async throws { + startIncrementalSyncCallCount += 1 + lastSyncSiteID = siteID + self.lastFullSyncDate = lastFullSyncDate + self.lastIncrementalSyncDate = lastIncrementalSyncDate + + if shouldBlockSync { + await withCheckedContinuation { continuation in + syncContinuation = continuation + } + } + + switch startIncrementalSyncResult { + case .success: + return + case .failure(let error): + throw error + } + } +} + +extension MockPOSCatalogIncrementalSyncService { + func blockNextSync() { + shouldBlockSync = true + } + + func resumeBlockedSync() { + syncContinuation?.resume() + syncContinuation = nil + shouldBlockSync = false + } +} diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift index 6e18daf25cb..7cc1c0d6e0d 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift @@ -5,18 +5,18 @@ import Testing struct POSCatalogSyncCoordinatorTests { private let mockSyncService: MockPOSCatalogFullSyncService - private let mockPersistenceService: MockPOSCatalogPersistenceService + private let mockIncrementalSyncService: MockPOSCatalogIncrementalSyncService private let grdbManager: GRDBManager private let sut: POSCatalogSyncCoordinator private let sampleSiteID: Int64 = 134 init() throws { self.mockSyncService = MockPOSCatalogFullSyncService() - self.mockPersistenceService = MockPOSCatalogPersistenceService() + self.mockIncrementalSyncService = MockPOSCatalogIncrementalSyncService() self.grdbManager = try GRDBManager() self.sut = POSCatalogSyncCoordinator( fullSyncService: mockSyncService, - persistenceService: mockPersistenceService, + incrementalSyncService: mockIncrementalSyncService, grdbManager: grdbManager ) } @@ -130,6 +130,31 @@ struct POSCatalogSyncCoordinatorTests { #expect(shouldSync == true) } + // MARK: - Database Check Tests + + @Test func shouldPerformFullSync_returns_true_when_site_not_in_database() async { + // Given - site does not exist in database, but has recent sync date + // Note: not creating site in database so it won't exist + + // When - max age is 1 hour (normally wouldn't sync) + let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60) + + // Then - should sync because site doesn't exist in database + #expect(shouldSync == true) + } + + @Test func shouldPerformFullSync_respects_time_when_site_exists_in_database() async throws { + // Given - site exists in database with recent sync date + let recentSyncDate = Date().addingTimeInterval(-30 * 60) // 30 minutes ago + try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: recentSyncDate) + + // When - max age is 1 hour + let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60) + + // Then - should not sync because site exists and time hasn't passed + #expect(shouldSync == false) + } + // MARK: - Sync Tracking Tests @Test func performFullSync_throws_error_when_sync_already_in_progress() async throws { @@ -196,11 +221,190 @@ struct POSCatalogSyncCoordinatorTests { try await sut.performFullSync(for: sampleSiteID) } + // MARK: - Incremental Sync Tests + + @Test(arguments: [true, false]) + func performIncrementalSyncIfApplicable_skips_sync_when_no_full_sync_performed(forceSync: Bool) async throws { + // Given - site exists but has no full sync date + try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: nil) + + // When + try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, forceSync: forceSync) + + // Then + #expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 0) + } + + @Test func performIncrementalSyncIfApplicable_skips_sync_when_incremental_sync_is_within_max_age() async throws { + // Given + let maxAge: TimeInterval = 2 + let incrementalSyncDate = Date().addingTimeInterval(-(maxAge - 0.2)) // Just within max age + let fullSyncDate = Date().addingTimeInterval(-7200) + try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: fullSyncDate, lastIncrementalSyncDate: incrementalSyncDate) + + let sut = POSCatalogSyncCoordinator( + fullSyncService: mockSyncService, + incrementalSyncService: mockIncrementalSyncService, + grdbManager: grdbManager, + maxIncrementalSyncAge: maxAge + ) + + // When + try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, forceSync: false) + + // Then + #expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 0) + } + + @Test(arguments: [true, false]) + func performIncrementalSyncIfApplicable_performs_sync_when_incremental_sync_is_stale(forceSync: Bool) async throws { + // Given + let maxAge: TimeInterval = 2 + let incrementalSyncDate = Date().addingTimeInterval(-(maxAge + 0.2)) // Just above max age + let fullSyncDate = Date().addingTimeInterval(-3600) + try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: fullSyncDate, lastIncrementalSyncDate: incrementalSyncDate) + + let sut = POSCatalogSyncCoordinator( + fullSyncService: mockSyncService, + incrementalSyncService: mockIncrementalSyncService, + grdbManager: grdbManager, + maxIncrementalSyncAge: maxAge + ) + + // When + try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, forceSync: forceSync) + + // Then + #expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 1) + #expect(mockIncrementalSyncService.lastSyncSiteID == sampleSiteID) + } + + @Test(arguments: [true, false]) + func performIncrementalSyncIfApplicable_performs_sync_when_no_incremental_sync_date(forceSync: Bool) async throws { + // Given + let fullSyncDate = Date().addingTimeInterval(-3600) + try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: fullSyncDate, lastIncrementalSyncDate: nil) + + // When + try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, forceSync: false) + + // Then + #expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 1) + #expect(mockIncrementalSyncService.lastSyncSiteID == sampleSiteID) + } + + @Test func performIncrementalSyncIfApplicable_forceSync_bypasses_age_check() async throws { + // Given + let maxAge: TimeInterval = 2 + let incrementalSyncDate = Date().addingTimeInterval(-(maxAge - 0.2)) // Just within max age + let fullSyncDate = Date().addingTimeInterval(-3600) + try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: fullSyncDate, lastIncrementalSyncDate: incrementalSyncDate) + + let sut = POSCatalogSyncCoordinator( + fullSyncService: mockSyncService, + incrementalSyncService: mockIncrementalSyncService, + grdbManager: grdbManager, + maxIncrementalSyncAge: maxAge + ) + + // When + try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, forceSync: true) + + // Then + #expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 1) + #expect(mockIncrementalSyncService.lastSyncSiteID == sampleSiteID) + } + + @Test func performIncrementalSyncIfApplicable_throws_error_when_sync_already_in_progress() async throws { + // Given + let fullSyncDate = Date().addingTimeInterval(-3600) + try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: fullSyncDate) + mockIncrementalSyncService.blockNextSync() + + // Start first incremental sync (it will block) + let firstSyncTask = Task { + try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, forceSync: false) + } + + // Give first sync a moment to start and get blocked + try await Task.sleep(nanoseconds: 10_000_000) // 10ms + + // When - try to start second incremental sync while first is blocked + do { + _ = try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, forceSync: false) + #expect(Bool(false), "Should have thrown syncAlreadyInProgress error") + } catch let error as POSCatalogSyncError { + // Then + #expect(error == POSCatalogSyncError.syncAlreadyInProgress(siteID: sampleSiteID)) + } + + // Cleanup + mockIncrementalSyncService.resumeBlockedSync() + _ = try await firstSyncTask.value + } + + @Test func performIncrementalSyncIfApplicable_allows_concurrent_syncs_for_different_sites() async throws { + // Given + let siteA: Int64 = 123 + let siteB: Int64 = 456 + let fullSyncDate = Date().addingTimeInterval(-3600) + + try createSiteInDatabase(siteID: siteA, lastFullSyncDate: fullSyncDate) + try createSiteInDatabase(siteID: siteB, lastFullSyncDate: fullSyncDate) + + // When + async let syncA: () = sut.performIncrementalSyncIfApplicable(for: siteA, forceSync: false) + async let syncB: () = sut.performIncrementalSyncIfApplicable(for: siteB, forceSync: false) + + // Then + try await syncA + try await syncB + #expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 2) + } + + @Test func performIncrementalSyncIfApplicable_propagates_errors() async throws { + // Given + let fullSyncDate = Date().addingTimeInterval(-3600) + try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: fullSyncDate) + + let expectedError = NSError(domain: "incremental_sync", code: 500, userInfo: [NSLocalizedDescriptionKey: "Incremental sync failed"]) + mockIncrementalSyncService.startIncrementalSyncResult = .failure(expectedError) + + // When/Then + await #expect(throws: expectedError) { + try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, forceSync: false) + } + } + + @Test(arguments: [true, false]) + func performIncrementalSyncIfApplicable_incremental_tracking_cleaned_up_on_error(forceSync: Bool) async throws { + // Given + let fullSyncDate = Date().addingTimeInterval(-3600) + try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: fullSyncDate) + + let expectedError = NSError(domain: "test", code: 1, userInfo: nil) + mockIncrementalSyncService.startIncrementalSyncResult = .failure(expectedError) + + // When - incremental sync fails + do { + _ = try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, forceSync: forceSync) + #expect(Bool(false), "Should have thrown error") + } catch { + // Expected error + } + + // Then - subsequent incremental sync should be allowed + mockIncrementalSyncService.startIncrementalSyncResult = .success(()) + + try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, forceSync: forceSync) + #expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 2) + } + // MARK: - Helper Methods - private func createSiteInDatabase(siteID: Int64, lastFullSyncDate: Date? = nil) throws { + private func createSiteInDatabase(siteID: Int64, lastFullSyncDate: Date? = nil, lastIncrementalSyncDate: Date? = nil) throws { try grdbManager.databaseConnection.write { db in - let site = PersistedSite(id: siteID, lastCatalogFullSyncDate: lastFullSyncDate) + let site = PersistedSite(id: siteID, lastCatalogIncrementalSyncDate: lastIncrementalSyncDate, lastCatalogFullSyncDate: lastFullSyncDate) try site.insert(db) } } diff --git a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift index 96acf04bbe9..1f251d3ed8b 100644 --- a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift +++ b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift @@ -147,9 +147,11 @@ class AuthenticatedState: StoresManagerState { // Initialize POS catalog sync coordinator if feature flag is enabled if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.pointOfSaleLocalCatalogi1), - let fullSyncService = POSCatalogFullSyncService(credentials: credentials, grdbManager: ServiceLocator.grdbManager) { + let fullSyncService = POSCatalogFullSyncService(credentials: credentials, grdbManager: ServiceLocator.grdbManager), + let incrementalSyncService = POSCatalogIncrementalSyncService(credentials: credentials, grdbManager: ServiceLocator.grdbManager) { posCatalogSyncCoordinator = POSCatalogSyncCoordinator( fullSyncService: fullSyncService, + incrementalSyncService: incrementalSyncService, grdbManager: ServiceLocator.grdbManager ) } else {