From 54c0c783ba38a0118d30355755c7c7c3f58e575a Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 5 Sep 2025 16:13:13 +0100 Subject: [PATCH 1/3] =?UTF-8?q?Always=20perform=20full=20sync=20if=20the?= =?UTF-8?q?=20site=E2=80=99s=20not=20stored?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Tools/POS/POSCatalogSyncCoordinator.swift | 23 ++++++++- .../POS/POSCatalogSyncCoordinatorTests.swift | 49 +++++++++++++++++-- .../ViewRelated/MainTabBarController.swift | 2 +- 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift index 2c1507c5045..e3a65ce838d 100644 --- a/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift +++ b/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift @@ -1,6 +1,7 @@ import Foundation import Storage import CocoaLumberjackSwift +import GRDB public protocol POSCatalogSyncCoordinatorProtocol { /// Performs a full catalog sync for the specified site @@ -19,11 +20,14 @@ public protocol POSCatalogSyncCoordinatorProtocol { public final class POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { private let syncService: POSCatalogFullSyncServiceProtocol private let settingsStore: SiteSpecificAppSettingsStoreMethodsProtocol + private let grdbManager: GRDBManagerProtocol public init(syncService: POSCatalogFullSyncServiceProtocol, - settingsStore: SiteSpecificAppSettingsStoreMethodsProtocol? = nil) { + settingsStore: SiteSpecificAppSettingsStoreMethodsProtocol? = nil, + grdbManager: GRDBManagerProtocol) { self.syncService = syncService self.settingsStore = settingsStore ?? SiteSpecificAppSettingsStoreMethods(fileStorage: PListFileStorage()) + self.grdbManager = grdbManager } public func performFullSync(for siteID: Int64) async throws -> POSCatalog { @@ -39,6 +43,11 @@ public final class POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol } public func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) -> Bool { + if !siteExistsInDatabase(siteID: siteID) { + DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) not found in database, sync needed") + return true + } + guard let lastSyncDate = lastFullSyncDate(for: siteID) else { DDLogInfo("📋 POSCatalogSyncCoordinator: No previous sync found for site \(siteID), sync needed") return true @@ -61,4 +70,16 @@ public final class POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol private func lastFullSyncDate(for siteID: Int64) -> Date? { return settingsStore.getPOSLastFullSyncDate(for: siteID) } + + private func siteExistsInDatabase(siteID: Int64) -> Bool { + do { + return try grdbManager.databaseConnection.read { db in + return try PersistedSite.filter(key: siteID).fetchCount(db) > 0 + } + } catch { + DDLogError("⛔️ POSCatalogSyncCoordinator: Error checking if site \(siteID) exists in database: \(error)") + // On error, assume site exists to avoid unnecessary syncs + return true + } + } } diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift index e83f2684212..0bfee35ec75 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift @@ -1,20 +1,23 @@ import Foundation import Testing @testable import Yosemite -import Storage +@testable import Storage struct POSCatalogSyncCoordinatorTests { private let mockSyncService: MockPOSCatalogFullSyncService private let mockSettingsStore: MockSiteSpecificAppSettingsStoreMethods + private let grdbManager: GRDBManager private let sut: POSCatalogSyncCoordinator private let sampleSiteID: Int64 = 134 - init() { + init() throws { self.mockSyncService = MockPOSCatalogFullSyncService() self.mockSettingsStore = MockSiteSpecificAppSettingsStoreMethods() + self.grdbManager = try GRDBManager() self.sut = POSCatalogSyncCoordinator( syncService: mockSyncService, - settingsStore: mockSettingsStore + settingsStore: mockSettingsStore, + grdbManager: grdbManager ) } @@ -128,10 +131,11 @@ struct POSCatalogSyncCoordinatorTests { #expect(shouldSyncB == true) // No previous sync } - @Test func shouldPerformFullSync_with_zero_maxAge_always_returns_true() { + @Test func shouldPerformFullSync_with_zero_maxAge_always_returns_true() throws { // Given - previous sync was just now 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) @@ -139,6 +143,43 @@ struct POSCatalogSyncCoordinatorTests { // Then #expect(shouldSync == true) } + + // MARK: - Database Check Tests + + @Test func shouldPerformFullSync_returns_true_when_site_not_in_database() { + // 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) + + // Then - should sync because site doesn't exist in database + #expect(shouldSync == true) + } + + @Test func shouldPerformFullSync_respects_time_when_site_exists_in_database() 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) + + // Then - should not sync because site exists and time hasn't passed + #expect(shouldSync == false) + } + + // MARK: - Helper Methods + + private func createSiteInDatabase(siteID: Int64) throws { + try grdbManager.databaseConnection.write { db in + let site = PersistedSite(id: siteID) + try site.insert(db) + } + } } // MARK: - Mock Services diff --git a/WooCommerce/Classes/ViewRelated/MainTabBarController.swift b/WooCommerce/Classes/ViewRelated/MainTabBarController.swift index 89bbcb5bed9..cfc79bac8b0 100644 --- a/WooCommerce/Classes/ViewRelated/MainTabBarController.swift +++ b/WooCommerce/Classes/ViewRelated/MainTabBarController.swift @@ -799,7 +799,7 @@ private extension MainTabBarController { return nil } - return POSCatalogSyncCoordinator(syncService: syncService) + return POSCatalogSyncCoordinator(syncService: syncService, grdbManager: ServiceLocator.grdbManager) } func triggerPOSCatalogSyncIfNeeded(for siteID: Int64) async { From bf089469dfd3da37b1fbf77f81626ae38c377605 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 5 Sep 2025 17:56:27 +0100 Subject: [PATCH 2/3] Update tests to include site in database --- .../POS/POSCatalogSyncCoordinatorTests.swift | 36 ++++++++++++++----- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift index 0bfee35ec75..28926f8745e 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift @@ -77,9 +77,23 @@ struct POSCatalogSyncCoordinatorTests { // MARK: - Should Sync Decision Tests - @Test func shouldPerformFullSync_returns_true_when_no_previous_sync() { - // Given - no previous sync date stored + @Test func shouldPerformFullSync_site_not_in_database_with_no_sync_history() { + // 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) + + // Then - should sync because site doesn't exist in database + #expect(shouldSync == true) + } + + @Test func shouldPerformFullSync_returns_true_when_no_previous_sync() 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) @@ -89,10 +103,11 @@ struct POSCatalogSyncCoordinatorTests { #expect(mockSettingsStore.getPOSLastFullSyncDateCallCount == 1) } - @Test func shouldPerformFullSync_returns_true_when_sync_is_stale() { - // Given - previous sync was 2 hours ago + @Test func shouldPerformFullSync_returns_true_when_sync_is_stale() 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) @@ -101,10 +116,11 @@ struct POSCatalogSyncCoordinatorTests { #expect(shouldSync == true) } - @Test func shouldPerformFullSync_returns_false_when_sync_is_fresh() { - // Given - previous sync was 30 minutes ago + @Test func shouldPerformFullSync_returns_false_when_sync_is_fresh() 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) @@ -113,7 +129,7 @@ struct POSCatalogSyncCoordinatorTests { #expect(shouldSync == false) } - @Test func shouldPerformFullSync_handles_different_sites_independently() { + @Test func shouldPerformFullSync_handles_different_sites_independently() throws { // Given let siteA: Int64 = 123 let siteB: Int64 = 456 @@ -122,6 +138,10 @@ struct POSCatalogSyncCoordinatorTests { mockSettingsStore.storedDates[siteA] = oneHourAgo // Has previous sync // siteB has no previous sync + // Create both sites in database to test timing logic + try createSiteInDatabase(siteID: siteA) + 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 @@ -132,7 +152,7 @@ struct POSCatalogSyncCoordinatorTests { } @Test func shouldPerformFullSync_with_zero_maxAge_always_returns_true() throws { - // Given - previous sync was just now + // Given - previous sync was just now, and site exists in database let justNow = Date() mockSettingsStore.storedDates[sampleSiteID] = justNow try createSiteInDatabase(siteID: sampleSiteID) From c9e3758ba9c4a3e1daa00b233ab016ff3e8fadaf Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Tue, 9 Sep 2025 14:07:25 +0100 Subject: [PATCH 3/3] Test renames from code review --- .../Tools/POS/POSCatalogSyncCoordinatorTests.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift index 7cbeaaed6bf..5cf006ca67a 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift @@ -75,7 +75,7 @@ struct POSCatalogSyncCoordinatorTests { // MARK: - Should Sync Decision Tests - @Test func shouldPerformFullSync_site_not_in_database_with_no_sync_history() { + @Test func shouldPerformFullSync_returns_true_when_site_is_not_in_database_with_no_sync_history() { // Given - site doesn't exist in database AND has no sync history mockSettingsStore.storedDates = [:] // Note: NOT creating site in database @@ -87,7 +87,7 @@ struct POSCatalogSyncCoordinatorTests { #expect(shouldSync == true) } - @Test func shouldPerformFullSync_returns_true_when_no_previous_sync() throws { + @Test func shouldPerformFullSync_returns_true_when_site_is_in_database_with_no_previous_sync() 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 = [:]