Skip to content

Commit b919c3e

Browse files
joshhealdjoshheald
committed
Ignore time based sync eligibility inside POS
When we set a max catalog age of zero in a refresh, we’re forcing a sync – e.g. because of a pull to refresh, or a full sync from settings, or after a purchase. When we do this, we should ignore temporal reasons for not syncing. Otherwise, it’s possible to get into a position where syncs don’t work any more even though they should.
1 parent 07b8775 commit b919c3e

File tree

2 files changed

+69
-12
lines changed

2 files changed

+69
-12
lines changed

‎Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
262262
/// - maxAge: Maximum age before a sync is considered stale
263263
/// - Returns: True if a sync should be performed
264264
private func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async throws -> Bool {
265-
guard try await checkSyncEligibility(for: siteID) else {
265+
guard try await checkSyncEligibility(for: siteID, ignoreTemporalCriteria: maxAge == .zero) else {
266266
DDLogInfo("📋 POSCatalogSyncCoordinator: Full sync skipped - site \(siteID) is not eligible")
267267
return false
268268
}
@@ -402,7 +402,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
402402
}
403403

404404
private func shouldPerformIncrementalSync(for siteID: Int64, maxAge: TimeInterval) async throws -> Bool {
405-
guard try await checkSyncEligibility(for: siteID) else {
405+
guard try await checkSyncEligibility(for: siteID, ignoreTemporalCriteria: maxAge == .zero) else {
406406
DDLogInfo("📋 POSCatalogSyncCoordinator: Incremental sync skipped - site \(siteID) is not eligible")
407407
return false
408408
}
@@ -600,7 +600,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
600600

601601
// Check sync eligibility
602602
do {
603-
let eligibility = try await syncEligibility(for: siteID)
603+
let eligibility = try await syncEligibility(for: siteID, ignoreTemporalCriteria: maxAge == .zero)
604604
if case .ineligible(let reason) = eligibility {
605605
return reason.skipReason
606606
}
@@ -634,7 +634,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
634634

635635
// Check sync eligibility
636636
do {
637-
let eligibility = try await syncEligibility(for: siteID)
637+
let eligibility = try await syncEligibility(for: siteID, ignoreTemporalCriteria: maxAge == .zero)
638638
if case .ineligible(let reason) = eligibility {
639639
return reason.skipReason
640640
}
@@ -724,16 +724,19 @@ private extension POSCatalogSyncCoordinator {
724724
// MARK: - Sync Eligibility
725725

726726
/// Checks if sync is eligible for the given site based on catalog eligibility and temporal criteria
727-
func checkSyncEligibility(for siteID: Int64) async throws -> Bool {
728-
switch try await syncEligibility(for: siteID) {
727+
func checkSyncEligibility(for siteID: Int64, ignoreTemporalCriteria: Bool = false) async throws -> Bool {
728+
switch try await syncEligibility(for: siteID, ignoreTemporalCriteria: ignoreTemporalCriteria) {
729729
case .eligible(reason: let reason):
730730
switch reason {
731731
case .neverSynced:
732732
DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (no first sync date recorded)")
733733
case .openedRecently(daysSinceOpened: let daysSinceOpened):
734734
DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (last opened \(daysSinceOpened) days ago)")
735735
case .recentFirstSync(daysSinceFirstSync: let daysSinceFirstSync):
736-
DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (within grace period: \(daysSinceFirstSync) days since first sync)")
736+
DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (within grace period: " +
737+
"\(daysSinceFirstSync) days since first sync)")
738+
case .ignoredTemporalEligibilityCheck:
739+
DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (ignored temporal criteria check)")
737740
}
738741
return true
739742
case .ineligible(reason: let reason):
@@ -750,11 +753,15 @@ private extension POSCatalogSyncCoordinator {
750753
}
751754
}
752755

753-
func syncEligibility(for siteID: Int64) async throws -> SyncEligibility {
756+
func syncEligibility(for siteID: Int64, ignoreTemporalCriteria: Bool) async throws -> SyncEligibility {
754757
guard try await catalogEligibilityChecker.catalogEligibility(for: siteID) == .eligible else {
755758
return .ineligible(reason: .notEligibleForLocalCatalog)
756759
}
757760

761+
guard !ignoreTemporalCriteria else {
762+
return .eligible(reason: .ignoredTemporalEligibilityCheck)
763+
}
764+
758765
// Then check temporal eligibility (30-day criteria)
759766
let firstSyncDate = siteSettings.getFirstPOSCatalogSyncDate(siteID: siteID)
760767
let lastOpenedDate = siteSettings.getPOSLastOpenedDate(siteID: siteID)
@@ -794,6 +801,7 @@ private extension POSCatalogSyncCoordinator {
794801
case neverSynced
795802
case openedRecently(daysSinceOpened: Int)
796803
case recentFirstSync(daysSinceFirstSync: Int)
804+
case ignoredTemporalEligibilityCheck
797805
}
798806

799807
enum IneligibleReason {

‎Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift‎

Lines changed: 53 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -884,6 +884,53 @@ extension POSCatalogSyncCoordinatorTests {
884884
#expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 1)
885885
}
886886

887+
@Test func performFullSyncIfApplicable_with_zero_maxAge_ignores_temporal_eligibility() async throws {
888+
// Given - user past 30-day grace period with no recent open (normally ineligible)
889+
let fortyDaysAgo = Date().addingTimeInterval(-40 * 24 * 60 * 60)
890+
mockSiteSettings.mockFirstPOSCatalogSyncDate = fortyDaysAgo
891+
// No lastPOSOpenedDate set (never opened recently)
892+
893+
let coordinator = POSCatalogSyncCoordinator(
894+
fullSyncService: mockSyncService,
895+
incrementalSyncService: mockIncrementalSyncService,
896+
grdbManager: grdbManager,
897+
catalogEligibilityChecker: MockPOSLocalCatalogEligibilityService(),
898+
siteSettings: mockSiteSettings
899+
)
900+
try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: fortyDaysAgo)
901+
mockSyncService.startFullSyncResult = .success(POSCatalog(products: [], variations: [], syncDate: .now))
902+
903+
// When - maxAge is zero (forced sync, e.g. pull to refresh)
904+
try await coordinator.performFullSyncIfApplicable(for: sampleSiteID, maxAge: .zero)
905+
906+
// Then - sync should proceed despite temporal ineligibility
907+
#expect(mockSyncService.startFullSyncCallCount == 1)
908+
}
909+
910+
@Test func performIncrementalSyncIfApplicable_with_zero_maxAge_ignores_temporal_eligibility() async throws {
911+
// Given - user past 30-day grace period with no recent open (normally ineligible)
912+
let fortyDaysAgo = Date().addingTimeInterval(-40 * 24 * 60 * 60)
913+
mockSiteSettings.mockFirstPOSCatalogSyncDate = fortyDaysAgo
914+
// No lastPOSOpenedDate set (never opened recently)
915+
916+
let coordinator = POSCatalogSyncCoordinator(
917+
fullSyncService: mockSyncService,
918+
incrementalSyncService: mockIncrementalSyncService,
919+
grdbManager: grdbManager,
920+
catalogEligibilityChecker: MockPOSLocalCatalogEligibilityService(),
921+
siteSettings: mockSiteSettings
922+
)
923+
// Set up database with full sync date (required for incremental sync)
924+
try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: fortyDaysAgo)
925+
mockIncrementalSyncService.startIncrementalSyncResult = .success(POSCatalog(products: [], variations: [], syncDate: .now))
926+
927+
// When - maxAge is zero (forced sync, e.g. after a purchase)
928+
try await coordinator.performIncrementalSyncIfApplicable(for: sampleSiteID, maxAge: .zero)
929+
930+
// Then - sync should proceed despite temporal ineligibility
931+
#expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 1)
932+
}
933+
887934
// MARK: - isSyncStale Tests
888935

889936
@Test func isSyncStale_returns_true_when_no_full_sync_performed() async throws {
@@ -1394,8 +1441,9 @@ extension POSCatalogSyncCoordinatorTests {
13941441
connectivityObserver: nil
13951442
)
13961443

1397-
// When - Try to perform sync
1398-
try? await sut.performFullSyncIfApplicable(for: sampleSiteID, maxAge: .zero)
1444+
// When - Try to perform sync with non-zero maxAge (temporal criteria are checked)
1445+
// Note: maxAge of .zero would bypass temporal eligibility checks
1446+
try? await sut.performFullSyncIfApplicable(for: sampleSiteID, maxAge: sampleMaxAge)
13991447

14001448
// Then - Should track pos_not_opened_30_days
14011449
let syncSkipped = mockAnalytics.trackedEvents.first { $0.eventName == "local_catalog_sync_skipped" }
@@ -1428,8 +1476,9 @@ extension POSCatalogSyncCoordinatorTests {
14281476
connectivityObserver: nil
14291477
)
14301478

1431-
// When - Try to perform incremental sync
1432-
try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, maxAge: .zero)
1479+
// When - Try to perform incremental sync with non-zero maxAge (temporal criteria are checked)
1480+
// Note: maxAge of .zero would bypass temporal eligibility checks
1481+
try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, maxAge: sampleMaxAge)
14331482

14341483
// Then - Should track pos_not_opened_30_days
14351484
let syncSkipped = mockAnalytics.trackedEvents.first { $0.eventName == "local_catalog_sync_skipped" }

0 commit comments

Comments
 (0)