diff --git a/Modules/Sources/Yosemite/Base/StoresManager.swift b/Modules/Sources/Yosemite/Base/StoresManager.swift index 9a7df27705e..a3bc525f5ce 100644 --- a/Modules/Sources/Yosemite/Base/StoresManager.swift +++ b/Modules/Sources/Yosemite/Base/StoresManager.swift @@ -65,6 +65,10 @@ public protocol StoresManager { /// var site: AnyPublisher { get } + /// Provides access to the session-scoped POS catalog sync coordinator + /// + var posCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol? { get } + /// Indicates if we need a Default StoreID, or there's one already set. /// var needsDefaultStore: Bool { get } diff --git a/Modules/Sources/Yosemite/Model/Mocks/MockStoresManager.swift b/Modules/Sources/Yosemite/Model/Mocks/MockStoresManager.swift index c947842ec13..9e133598b91 100644 --- a/Modules/Sources/Yosemite/Model/Mocks/MockStoresManager.swift +++ b/Modules/Sources/Yosemite/Model/Mocks/MockStoresManager.swift @@ -253,6 +253,10 @@ public class MockStoresManager: StoresManager { public func shouldAuthenticateAdminPage(for site: Site) -> Bool { return false } + + public var posCatalogSyncCoordinator: (any POSCatalogSyncCoordinatorProtocol)? { + nil + } } private extension MockStoresManager { diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift index 4416be7eed7..be349fa078c 100644 --- a/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift +++ b/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift @@ -5,6 +5,7 @@ import GRDB public protocol POSCatalogSyncCoordinatorProtocol { /// Performs a full catalog sync for the specified site /// - Parameter siteID: The site ID to sync catalog for + /// - Throws: POSCatalogSyncError.syncAlreadyInProgress if a sync is already running for this site func performFullSync(for siteID: Int64) async throws /// Determines if a full sync should be performed based on the age of the last sync @@ -12,14 +13,21 @@ public protocol POSCatalogSyncCoordinatorProtocol { /// - siteID: The site ID to check /// - maxAge: Maximum age before a sync is considered stale /// - Returns: True if a sync should be performed - func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) -> Bool + func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async -> Bool } -public final class POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { +public enum POSCatalogSyncError: Error, Equatable { + case syncAlreadyInProgress(siteID: Int64) +} + +public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { private let fullSyncService: POSCatalogFullSyncServiceProtocol private let settingsStore: SiteSpecificAppSettingsStoreMethodsProtocol private let grdbManager: GRDBManagerProtocol + /// Tracks ongoing syncs by site ID to prevent duplicates + private var ongoingSyncs: Set = [] + public init(fullSyncService: POSCatalogFullSyncServiceProtocol, settingsStore: SiteSpecificAppSettingsStoreMethodsProtocol? = nil, grdbManager: GRDBManagerProtocol) { @@ -29,17 +37,29 @@ public final class POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol } public func performFullSync(for siteID: Int64) async throws { + if ongoingSyncs.contains(siteID) { + DDLogInfo("⚠ī¸ POSCatalogSyncCoordinator: Sync already in progress for site \(siteID)") + throw POSCatalogSyncError.syncAlreadyInProgress(siteID: siteID) + } + + // Mark sync as in progress + ongoingSyncs.insert(siteID) + + // Ensure cleanup happens regardless of success or failure + defer { + ongoingSyncs.remove(siteID) + } + DDLogInfo("🔄 POSCatalogSyncCoordinator starting full sync for site \(siteID)") let catalog = try await fullSyncService.startFullSync(for: siteID) - // Record the sync timestamp settingsStore.setPOSLastFullSyncDate(Date(), for: siteID) DDLogInfo("✅ POSCatalogSyncCoordinator completed full sync for site \(siteID)") } - public func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) -> Bool { + public func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async -> Bool { if !siteExistsInDatabase(siteID: siteID) { DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) not found in database, sync needed") return true diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift index 5cf006ca67a..5a8ff2ea4b9 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift @@ -46,7 +46,7 @@ struct POSCatalogSyncCoordinatorTests { mockSyncService.startFullSyncResult = .success(expectedCatalog) // When - _ = try await sut.performFullSync(for: sampleSiteID) + try await sut.performFullSync(for: sampleSiteID) let afterSync = Date() // Then @@ -66,7 +66,7 @@ struct POSCatalogSyncCoordinatorTests { // When/Then await #expect(throws: expectedError) { - _ = try await sut.performFullSync(for: sampleSiteID) + try await sut.performFullSync(for: sampleSiteID) } // Should not store timestamp on failure @@ -75,59 +75,59 @@ struct POSCatalogSyncCoordinatorTests { // MARK: - Should Sync Decision Tests - @Test func shouldPerformFullSync_returns_true_when_site_is_not_in_database_with_no_sync_history() { + @Test func shouldPerformFullSync_returns_true_when_site_is_not_in_database_with_no_sync_history() async { // Given - site doesn't exist in database AND has no sync history mockSettingsStore.storedDates = [:] // Note: NOT creating site in database // When - let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60) + 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_returns_true_when_site_is_in_database_with_no_previous_sync() throws { + @Test func shouldPerformFullSync_returns_true_when_site_is_in_database_with_no_previous_sync() async throws { // Given - no previous sync date stored, but site exists in database // This is much less likely to happen, but could help at a migration point mockSettingsStore.storedDates = [:] try createSiteInDatabase(siteID: sampleSiteID) // When - let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 3600) + let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 3600) // Then #expect(shouldSync == true) #expect(mockSettingsStore.getPOSLastFullSyncDateCallCount == 1) } - @Test func shouldPerformFullSync_returns_true_when_sync_is_stale() throws { + @Test func shouldPerformFullSync_returns_true_when_sync_is_stale() async throws { // Given - previous sync was 2 hours ago, and site exists in database let twoHoursAgo = Date().addingTimeInterval(-2 * 60 * 60) mockSettingsStore.storedDates[sampleSiteID] = twoHoursAgo try createSiteInDatabase(siteID: sampleSiteID) // When - max age is 1 hour - let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60) + let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60) // Then #expect(shouldSync == true) } - @Test func shouldPerformFullSync_returns_false_when_sync_is_fresh() throws { + @Test func shouldPerformFullSync_returns_false_when_sync_is_fresh() async throws { // Given - previous sync was 30 minutes ago, and site exists in database let thirtyMinutesAgo = Date().addingTimeInterval(-30 * 60) mockSettingsStore.storedDates[sampleSiteID] = thirtyMinutesAgo try createSiteInDatabase(siteID: sampleSiteID) // When - max age is 1 hour - let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60) + let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60) // Then #expect(shouldSync == false) } - @Test func shouldPerformFullSync_handles_different_sites_independently() throws { + @Test func shouldPerformFullSync_handles_different_sites_independently() async throws { // Given let siteA: Int64 = 123 let siteB: Int64 = 456 @@ -141,22 +141,22 @@ struct POSCatalogSyncCoordinatorTests { try createSiteInDatabase(siteID: siteB) // When - let shouldSyncA = sut.shouldPerformFullSync(for: siteA, maxAge: 2 * 60 * 60) // 2 hours - let shouldSyncB = sut.shouldPerformFullSync(for: siteB, maxAge: 2 * 60 * 60) // 2 hours + let shouldSyncA = await sut.shouldPerformFullSync(for: siteA, maxAge: 2 * 60 * 60) // 2 hours + let shouldSyncB = await sut.shouldPerformFullSync(for: siteB, maxAge: 2 * 60 * 60) // 2 hours // Then #expect(shouldSyncA == false) // Recent sync exists #expect(shouldSyncB == true) // No previous sync } - @Test func shouldPerformFullSync_with_zero_maxAge_always_returns_true() throws { + @Test func shouldPerformFullSync_with_zero_maxAge_always_returns_true() async throws { // Given - previous sync was just now, and site exists in database let justNow = Date() mockSettingsStore.storedDates[sampleSiteID] = justNow try createSiteInDatabase(siteID: sampleSiteID) // When - max age is 0 (always sync) - let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 0) + let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 0) // Then #expect(shouldSync == true) @@ -164,32 +164,98 @@ struct POSCatalogSyncCoordinatorTests { // MARK: - Database Check Tests - @Test func shouldPerformFullSync_returns_true_when_site_not_in_database() { + @Test func shouldPerformFullSync_returns_true_when_site_not_in_database() async { // Given - site does not exist in database, but has recent sync date let recentSyncDate = Date().addingTimeInterval(-30 * 60) // 30 minutes ago mockSettingsStore.storedDates[sampleSiteID] = recentSyncDate // Note: not creating site in database so it won't exist // When - max age is 1 hour (normally wouldn't sync) - let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60) + 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() throws { + @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 mockSettingsStore.storedDates[sampleSiteID] = recentSyncDate try createSiteInDatabase(siteID: sampleSiteID) // When - max age is 1 hour - let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60) + 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 { + // Given - block the sync service so first sync will wait + let expectedCatalog = POSCatalog(products: [], variations: []) + mockSyncService.startFullSyncResult = .success(expectedCatalog) + mockSyncService.blockNextSync() + + // Start first sync in a task (it will block waiting for continuation) + let firstSyncTask = Task { + try await sut.performFullSync(for: sampleSiteID) + } + + // Give first sync a moment to start and get blocked + try await Task.sleep(nanoseconds: 10_000_000) // 10ms + + // When - try to start second sync while first is blocked + do { + _ = try await sut.performFullSync(for: sampleSiteID) + #expect(Bool(false), "Should have thrown syncAlreadyInProgress error") + } catch let error as POSCatalogSyncError { + // Then + #expect(error == POSCatalogSyncError.syncAlreadyInProgress(siteID: sampleSiteID)) + } + + // Cleanup - resume the first sync and wait for it to complete + mockSyncService.resumeBlockedSync() + _ = try await firstSyncTask.value + } + + @Test func performFullSync_allows_concurrent_syncs_for_different_sites() async throws { + // Given + let siteA: Int64 = 123 + let siteB: Int64 = 456 + let expectedCatalog = POSCatalog(products: [], variations: []) + mockSyncService.startFullSyncResult = .success(expectedCatalog) + + // When - start syncs for different sites concurrently + async let syncA: () = sut.performFullSync(for: siteA) + async let syncB: () = sut.performFullSync(for: siteB) + + // Then - both should complete successfully + try await syncA + try await syncB + #expect(mockSyncService.startFullSyncCallCount == 2) + } + + @Test func sync_tracking_cleaned_up_on_error() async throws { + // Given + let expectedError = NSError(domain: "test", code: 1, userInfo: nil) + mockSyncService.startFullSyncResult = .failure(expectedError) + + // When - sync fails + do { + _ = try await sut.performFullSync(for: sampleSiteID) + #expect(Bool(false), "Should have thrown error") + } catch { + // Expected error + } + + // Then - subsequent sync should be allowed + mockSyncService.startFullSyncResult = .success(POSCatalog(products: [], variations: [])) + + try await sut.performFullSync(for: sampleSiteID) + } + // MARK: - Helper Methods private func createSiteInDatabase(siteID: Int64) throws { @@ -204,6 +270,11 @@ struct POSCatalogSyncCoordinatorTests { final class MockPOSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol { var startFullSyncResult: Result = .success(POSCatalog(products: [], variations: [])) + var syncDelay: UInt64 = 0 // nanoseconds to delay before returning + + // Controlled sync mechanism + private var syncContinuation: CheckedContinuation? + private var shouldBlockSync = false private(set) var startFullSyncCallCount = 0 private(set) var lastSyncSiteID: Int64? @@ -212,6 +283,18 @@ final class MockPOSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol { startFullSyncCallCount += 1 lastSyncSiteID = siteID + // If we should block, wait for continuation to be resumed + if shouldBlockSync { + await withCheckedContinuation { continuation in + syncContinuation = continuation + } + } + + // Add delay if specified + if syncDelay > 0 { + try await Task.sleep(nanoseconds: syncDelay) + } + switch startFullSyncResult { case .success(let catalog): return catalog @@ -219,4 +302,14 @@ final class MockPOSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol { throw error } } + + func blockNextSync() { + shouldBlockSync = true + } + + func resumeBlockedSync() { + syncContinuation?.resume() + syncContinuation = nil + shouldBlockSync = false + } } diff --git a/WooCommerce/Classes/ServiceLocator/ServiceLocator.swift b/WooCommerce/Classes/ServiceLocator/ServiceLocator.swift index fc0c27ca33f..bbe1d764bc3 100644 --- a/WooCommerce/Classes/ServiceLocator/ServiceLocator.swift +++ b/WooCommerce/Classes/ServiceLocator/ServiceLocator.swift @@ -299,6 +299,17 @@ final class ServiceLocator { static var startupWaitingTimeTracker: AppStartupWaitingTimeTracker { _startupWaitingTimeTracker } + + /// Provides access point to the `POSCatalogSyncCoordinator`. + /// Returns nil if feature flag is disabled or user is not authenticated. + /// + static var posCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol? { + guard featureFlagService.isFeatureFlagEnabled(.pointOfSaleLocalCatalogi1) else { + return nil + } + + return stores.posCatalogSyncCoordinator + } } diff --git a/WooCommerce/Classes/ViewRelated/MainTabBarController.swift b/WooCommerce/Classes/ViewRelated/MainTabBarController.swift index 7d041a0d326..577f17ee367 100644 --- a/WooCommerce/Classes/ViewRelated/MainTabBarController.swift +++ b/WooCommerce/Classes/ViewRelated/MainTabBarController.swift @@ -767,9 +767,8 @@ private extension MainTabBarController { ) // Configure POS catalog sync coordinator for local catalog syncing - if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.pointOfSaleLocalCatalogi1) { - posCatalogSyncCoordinator = createPOSCatalogSyncCoordinator() - } + // Get POS catalog sync coordinator (will be nil if feature flag disabled or not authenticated) + posCatalogSyncCoordinator = ServiceLocator.posCatalogSyncCoordinator // Configure hub menu tab coordinator once per logged in session potentially with multiple sites. if hubMenuTabCoordinator == nil { @@ -792,16 +791,6 @@ private extension MainTabBarController { OrdersSplitViewWrapperController(siteID: siteID) } - func createPOSCatalogSyncCoordinator() -> POSCatalogSyncCoordinatorProtocol? { - guard let credentials = ServiceLocator.stores.sessionManager.defaultCredentials, - let fullSyncService = POSCatalogFullSyncService(credentials: credentials, grdbManager: ServiceLocator.grdbManager) - else { - return nil - } - - return POSCatalogSyncCoordinator(fullSyncService: fullSyncService, grdbManager: ServiceLocator.grdbManager) - } - func triggerPOSCatalogSyncIfNeeded(for siteID: Int64) async { guard let coordinator = posCatalogSyncCoordinator else { return @@ -809,7 +798,7 @@ private extension MainTabBarController { // Check if sync is needed (older than 24 hours) let maxAge: TimeInterval = 24 * 60 * 60 - guard coordinator.shouldPerformFullSync(for: siteID, maxAge: maxAge) else { + guard await coordinator.shouldPerformFullSync(for: siteID, maxAge: maxAge) else { return } @@ -817,6 +806,8 @@ private extension MainTabBarController { Task.detached { do { _ = try await coordinator.performFullSync(for: siteID) + } catch POSCatalogSyncError.syncAlreadyInProgress { + DDLogInfo("ℹī¸ POS catalog sync already in progress for site \(siteID), skipping") } catch { DDLogError("⚠ī¸ POS catalog sync failed: \(error)") } diff --git a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift index 7b5fbe82627..f5710b4378b 100644 --- a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift +++ b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift @@ -28,6 +28,10 @@ class AuthenticatedState: StoresManagerState { private var cancellables: Set = [] + /// POS Catalog Sync Coordinator (session-scoped) + /// + private(set) var posCatalogSyncCoordinator: POSCatalogSyncCoordinator? + /// Designated Initializer /// init(credentials: Credentials, sessionManager: SessionManagerProtocol) { @@ -136,6 +140,17 @@ class AuthenticatedState: StoresManagerState { self.services = services + // Initialize POS catalog sync coordinator if feature flag is enabled + if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.pointOfSaleLocalCatalogi1), + let fullSyncService = POSCatalogFullSyncService(credentials: credentials, grdbManager: ServiceLocator.grdbManager) { + posCatalogSyncCoordinator = POSCatalogSyncCoordinator( + fullSyncService: fullSyncService, + grdbManager: ServiceLocator.grdbManager + ) + } else { + posCatalogSyncCoordinator = nil + } + trackEventRequestNotificationHandler = TrackEventRequestNotificationHandler() startListeningToNotifications() diff --git a/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift b/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift index 2f5eb8c814f..8e631a36b32 100644 --- a/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift +++ b/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift @@ -120,6 +120,12 @@ class DefaultStoresManager: StoresManager { sessionManager.defaultSitePublisher } + /// Provides access to the session-scoped POS catalog sync coordinator + /// + var posCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol? { + (state as? AuthenticatedState)?.posCatalogSyncCoordinator + } + /// Designated Initializer /// init(sessionManager: SessionManagerProtocol,