-
Notifications
You must be signed in to change notification settings - Fork 121
[Woo POS][Local Catalog] Check catalog size in sync coordinator #16119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
29370f0
9c2e389
323cbe4
69ef11f
61a7497
cea0e8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| import Foundation | ||
| import Networking | ||
|
|
||
| /// Protocol for checking the size of a remote POS catalog | ||
| public protocol POSCatalogSizeCheckerProtocol { | ||
| /// Checks the size of the remote catalog for the specified site | ||
| /// - Parameter siteID: The site ID to check catalog size for | ||
| /// - Returns: The size information of the catalog | ||
| /// - Throws: Network or parsing errors | ||
| func checkCatalogSize(for siteID: Int64) async throws -> POSCatalogSize | ||
| } | ||
|
|
||
| /// Implementation of catalog size checker that uses the sync remote to get counts | ||
| public struct POSCatalogSizeChecker: POSCatalogSizeCheckerProtocol { | ||
| private let syncRemote: POSCatalogSyncRemoteProtocol | ||
|
|
||
| public init(syncRemote: POSCatalogSyncRemoteProtocol) { | ||
| self.syncRemote = syncRemote | ||
| } | ||
|
|
||
| public func checkCatalogSize(for siteID: Int64) async throws -> POSCatalogSize { | ||
| // Make concurrent requests to get both counts | ||
| async let productCount = syncRemote.getProductCount(siteID: siteID) | ||
| async let variationCount = syncRemote.getProductVariationCount(siteID: siteID) | ||
|
|
||
| do { | ||
| return try await POSCatalogSize( | ||
| productCount: productCount, | ||
| variationCount: variationCount | ||
| ) | ||
| } catch { | ||
| DDLogError( | ||
| "⚠️ Failed to check POS catalog size for site \(siteID): \(error)" | ||
| ) | ||
| throw error | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public struct POSCatalogSize: Equatable { | ||
| /// Number of products in the catalog | ||
| public let productCount: Int | ||
|
|
||
| /// Number of product variations in the catalog | ||
| public let variationCount: Int | ||
|
|
||
| /// Total number of items (products + variations) | ||
| public var totalCount: Int { | ||
| productCount + variationCount | ||
| } | ||
|
|
||
| public init(productCount: Int, variationCount: Int) { | ||
| self.productCount = productCount | ||
| self.variationCount = variationCount | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,13 +16,30 @@ public protocol POSCatalogSyncCoordinatorProtocol { | |
| /// - Returns: True if a sync should be performed | ||
| func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async -> Bool | ||
|
|
||
| /// Determines if a full sync should be performed based on the age of the last sync | ||
| /// - Parameters: | ||
| /// - siteID: The site ID to check | ||
| /// - maxAge: Maximum age before a sync is considered stale | ||
| /// - maxCatalogSize: Maximum allowed catalog size for syncing (default: 1000) | ||
| /// - Returns: True if a sync should be performed | ||
| func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval, maxCatalogSize: Int) 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 | ||
|
|
||
| /// 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 | ||
| /// - maxCatalogSize: Maximum allowed catalog size for syncing (default: 1000) | ||
| /// - 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, maxCatalogSize: Int) async throws | ||
| } | ||
|
|
||
| public enum POSCatalogSyncError: Error, Equatable { | ||
|
|
@@ -34,6 +51,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { | |
| private let incrementalSyncService: POSCatalogIncrementalSyncServiceProtocol | ||
| private let grdbManager: GRDBManagerProtocol | ||
| private let maxIncrementalSyncAge: TimeInterval | ||
| private let catalogSizeChecker: POSCatalogSizeCheckerProtocol | ||
|
|
||
| /// Tracks ongoing full syncs by site ID to prevent duplicates | ||
| private var ongoingSyncs: Set<Int64> = [] | ||
|
|
@@ -43,11 +61,13 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { | |
| public init(fullSyncService: POSCatalogFullSyncServiceProtocol, | ||
| incrementalSyncService: POSCatalogIncrementalSyncServiceProtocol, | ||
| grdbManager: GRDBManagerProtocol, | ||
| maxIncrementalSyncAge: TimeInterval = 300) { | ||
| maxIncrementalSyncAge: TimeInterval = 300, | ||
| catalogSizeChecker: POSCatalogSizeCheckerProtocol) { | ||
| self.fullSyncService = fullSyncService | ||
| self.incrementalSyncService = incrementalSyncService | ||
| self.grdbManager = grdbManager | ||
| self.maxIncrementalSyncAge = maxIncrementalSyncAge | ||
| self.catalogSizeChecker = catalogSizeChecker | ||
| } | ||
|
|
||
| public func performFullSync(for siteID: Int64) async throws { | ||
|
|
@@ -72,6 +92,20 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { | |
| } | ||
|
|
||
| public func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async -> Bool { | ||
| return await shouldPerformFullSync(for: siteID, maxAge: maxAge, maxCatalogSize: 1000) | ||
|
||
| } | ||
|
|
||
| /// Determines if a full sync should be performed based on the age of the last sync | ||
| /// - Parameters: | ||
| /// - siteID: The site ID to check | ||
| /// - maxAge: Maximum age before a sync is considered stale | ||
| /// - maxCatalogSize: Maximum allowed catalog size for syncing | ||
| /// - Returns: True if a sync should be performed | ||
| public func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval, maxCatalogSize: Int) async -> Bool { | ||
| guard await isCatalogSizeWithinLimit(for: siteID, maxCatalogSize: maxCatalogSize) else { | ||
| return false | ||
| } | ||
|
|
||
| if !siteExistsInDatabase(siteID: siteID) { | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) not found in database, sync needed") | ||
| return true | ||
|
|
@@ -86,20 +120,36 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { | |
| let shouldSync = age > maxAge | ||
|
|
||
| if shouldSync { | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Last sync for site \(siteID) was \(Int(age))s ago (max: \(Int(maxAge))s), sync needed") | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Last sync for site \(siteID) was \(Int(age))s ago " + | ||
| "(max: \(Int(maxAge))s), sync needed") | ||
| } else { | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Last sync for site \(siteID) was \(Int(age))s ago (max: \(Int(maxAge))s), sync not needed") | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Last sync for site \(siteID) was \(Int(age))s ago " + | ||
| "(max: \(Int(maxAge))s), sync not needed") | ||
| } | ||
|
|
||
| return shouldSync | ||
| } | ||
|
|
||
| public func performIncrementalSyncIfApplicable(for siteID: Int64, forceSync: Bool) async throws { | ||
| try await performIncrementalSyncIfApplicable(for: siteID, forceSync: forceSync, maxCatalogSize: 1000) | ||
| } | ||
|
|
||
| /// 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 | ||
| /// - maxCatalogSize: Maximum allowed catalog size for syncing | ||
| /// - Throws: POSCatalogSyncError.syncAlreadyInProgress if a sync is already running for this site | ||
| public func performIncrementalSyncIfApplicable(for siteID: Int64, forceSync: Bool, maxCatalogSize: Int) async throws { | ||
| if ongoingIncrementalSyncs.contains(siteID) { | ||
| DDLogInfo("⚠️ POSCatalogSyncCoordinator: Incremental sync already in progress for site \(siteID)") | ||
| throw POSCatalogSyncError.syncAlreadyInProgress(siteID: siteID) | ||
| } | ||
|
|
||
| guard await isCatalogSizeWithinLimit(for: siteID, maxCatalogSize: maxCatalogSize) else { | ||
| return | ||
| } | ||
|
|
||
| guard let lastFullSyncDate = await lastFullSyncDate(for: siteID) else { | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: No full sync performed yet for site \(siteID), skipping incremental sync") | ||
| return | ||
|
|
@@ -130,6 +180,28 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { | |
|
|
||
| // MARK: - Private | ||
|
|
||
| /// Checks if the catalog size is within the specified sync limit | ||
| /// - Parameters: | ||
| /// - siteID: The site ID to check | ||
| /// - maxCatalogSize: Maximum allowed catalog size for syncing | ||
| /// - Returns: True if catalog size is within limit or if size cannot be determined | ||
| private func isCatalogSizeWithinLimit(for siteID: Int64, maxCatalogSize: Int) async -> Bool { | ||
| guard let catalogSize = try? await catalogSizeChecker.checkCatalogSize(for: siteID) else { | ||
| DDLogError("📋 POSCatalogSyncCoordinator: Could not get catalog size for site \(siteID)") | ||
| return false | ||
| } | ||
|
|
||
| guard catalogSize.totalCount <= maxCatalogSize else { | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) has catalog size \(catalogSize.totalCount), " + | ||
| "greater than \(maxCatalogSize), should not sync.") | ||
| return false | ||
| } | ||
|
|
||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) has catalog size \(catalogSize.totalCount), with " + | ||
| "\(catalogSize.productCount) products and \(catalogSize.variationCount) variations") | ||
| return true | ||
| } | ||
|
|
||
| private func lastFullSyncDate(for siteID: Int64) async -> Date? { | ||
| do { | ||
| return try await grdbManager.databaseConnection.read { db in | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the catalog size checker seems like an internal tool within Yosemite, do you foresee this being used in the app layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. For example, we'll want to check the size when we decide whether to use the existing approach or the local catalog, which needs to be decided in the app layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that there are other sync/async conditions for the local catalog feature as in p1757587795454369/1757587788.433699-slack-C070SJRA8DP, I thought we might have another high-level checker for all the conditions (and implement caching if we decide to). So that this class is for internal use in Yosemite. WDYT?