[Local catalog] Use specific skip reasons in analytics (#16382)

joshheald · web-flow · commit dc27720108ec · 2025-11-24T16:40:37.000Z
diff --git a/Modules/Sources/WooFoundationCore/Analytics/WooAnalyticsEvent.swift b/Modules/Sources/WooFoundationCore/Analytics/WooAnalyticsEvent.swift
@@ -139,9 +139,10 @@ extension WooAnalyticsEvent {
                                      error: error)
         }
 
-        public static func syncSkipped(reason: String) -> WooAnalyticsEvent {
+        public static func syncSkipped(reason: String, syncType: String) -> WooAnalyticsEvent {
             WooAnalyticsEvent(statName: .pointOfSaleLocalCatalogSyncSkipped,
-                              properties: [Key.reason: reason])
+                              properties: [Key.reason: reason,
+                                           Key.syncType: syncType])
         }
     }
 }
diff --git a/Modules/Sources/Yosemite/Protocols/POSLocalCatalogEligibilityServiceProtocol.swift b/Modules/Sources/Yosemite/Protocols/POSLocalCatalogEligibilityServiceProtocol.swift
@@ -22,7 +22,7 @@ public enum POSLocalCatalogIneligibleReason: Equatable {
     public var skipReason: String {
         switch self {
         case .posTabNotEligible:
-            return "pos_inactive"
+            return "pos_not_eligible"
         case .featureFlagDisabled:
             return "feature_flag_disabled"
         case .unsupportedWooCommerceVersion:
diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift
@@ -149,7 +149,8 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
 
         guard try await shouldPerformFullSync(for: siteID, maxAge: maxAge) else {
             let reason = await getSyncSkipReason(for: siteID, maxAge: maxAge)
-            trackAnalytics(WooAnalyticsEvent.LocalCatalog.syncSkipped(reason: reason))
+            trackAnalytics(WooAnalyticsEvent.LocalCatalog.syncSkipped(reason: reason,
+                                                                      syncType: POSCatalogSyncType.full.rawValue))
             throw POSCatalogSyncError.shouldNotSync
         }
 
@@ -315,7 +316,8 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
 
         guard try await shouldPerformIncrementalSync(for: siteID, maxAge: maxAge) else {
             let reason = await getIncrementalSyncSkipReason(for: siteID, maxAge: maxAge)
-            trackAnalytics(WooAnalyticsEvent.LocalCatalog.syncSkipped(reason: reason))
+            trackAnalytics(WooAnalyticsEvent.LocalCatalog.syncSkipped(reason: reason,
+                                                                      syncType: POSCatalogSyncType.incremental.rawValue))
             return
         }
 
@@ -596,6 +598,16 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
             return "eligibility_check_failed"
         }
 
+        // Check sync eligibility
+        do {
+            let eligibility = try await syncEligibility(for: siteID)
+            if case .ineligible(let reason) = eligibility {
+                return reason.skipReason
+            }
+        } catch {
+            return "sync_eligibility_check_failed"
+        }
+
         // Check if sync is needed based on age
         guard let lastSyncDate = await lastFullSyncDate(for: siteID) else {
             return "no_previous_sync" // This shouldn't happen if shouldPerformFullSync returned false
@@ -610,7 +622,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
     }
 
     private func getIncrementalSyncSkipReason(for siteID: Int64, maxAge: TimeInterval) async -> String {
-        // Check eligibility first
+        // Check overall local catalog eligibility first
         do {
             let eligibility = try await catalogEligibilityChecker.catalogEligibility(for: siteID)
             if case .ineligible(let reason) = eligibility {
@@ -620,6 +632,16 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
             return "eligibility_check_failed"
         }
 
+        // Check sync eligibility
+        do {
+            let eligibility = try await syncEligibility(for: siteID)
+            if case .ineligible(let reason) = eligibility {
+                return reason.skipReason
+            }
+        } catch {
+            return "sync_eligibility_check_failed"
+        }
+
         // Check if full sync exists
         guard await lastFullSyncDate(for: siteID) != nil else {
             return "no_full_sync"
@@ -703,19 +725,43 @@ private extension POSCatalogSyncCoordinator {
 
     /// Checks if sync is eligible for the given site based on catalog eligibility and temporal criteria
     func checkSyncEligibility(for siteID: Int64) async throws -> Bool {
-        guard try await catalogEligibilityChecker.catalogEligibility(for: siteID) == .eligible else {
-            DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) - Catalog ineligible")
+        switch try await syncEligibility(for: siteID) {
+        case .eligible(reason: let reason):
+            switch reason {
+            case .neverSynced:
+                DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (no first sync date recorded)")
+            case .openedRecently(daysSinceOpened: let daysSinceOpened):
+                DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (last opened \(daysSinceOpened) days ago)")
+            case .recentFirstSync(daysSinceFirstSync: let daysSinceFirstSync):
+                DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (within grace period: \(daysSinceFirstSync) days since first sync)")
+            }
+            return true
+        case .ineligible(reason: let reason):
+            switch reason {
+            case .notEligibleForLocalCatalog:
+                DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) - Catalog ineligible")
+            case .notOpenedRecently(daysSinceOpened: let daysSinceOpened):
+                DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) ineligible (POS last opened \(daysSinceOpened) days ago)")
+            case .neverOpenedAndPastFirstSyncGracePeriod(daysSinceFirstSync: let daysSinceFirstSync):
+                DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) ineligible (past 30-day grace period, " +
+                          "no recent POS open, \(daysSinceFirstSync) days since first sync)")
+            }
             return false
         }
+    }
+
+    func syncEligibility(for siteID: Int64) async throws -> SyncEligibility {
+        guard try await catalogEligibilityChecker.catalogEligibility(for: siteID) == .eligible else {
+            return .ineligible(reason: .notEligibleForLocalCatalog)
+        }
 
         // Then check temporal eligibility (30-day criteria)
         let firstSyncDate = siteSettings.getFirstPOSCatalogSyncDate(siteID: siteID)
         let lastOpenedDate = siteSettings.getPOSLastOpenedDate(siteID: siteID)
 
         // Case 1: No first sync date yet - eligible (will be set on first sync)
         guard let firstSync = firstSyncDate else {
-            DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (no first sync date recorded)")
-            return true
+            return .eligible(reason: .neverSynced)
         }
 
         // Case 2: Has synced before. Check if within 30-day window from first sync
@@ -724,23 +770,44 @@ private extension POSCatalogSyncCoordinator {
         if daysSinceFirstSync > Constants.maxDaysSinceLastOpened {
             // More than 30 days since first sync - must have opened POS recently to remain eligible
             guard let lastOpened = lastOpenedDate else {
-                DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) ineligible (past 30-day grace period, no recent POS open)")
-                return false
+                return .ineligible(reason: .neverOpenedAndPastFirstSyncGracePeriod(daysSinceFirstSync: daysSinceFirstSync))
             }
 
             let daysSinceLastOpened = Calendar.current.dateComponents([.day], from: lastOpened, to: Date()).day ?? 0
 
             if daysSinceLastOpened <= Constants.maxDaysSinceLastOpened {
-                DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (last opened \(daysSinceLastOpened) days ago)")
-                return true
+                return .eligible(reason: .openedRecently(daysSinceOpened: daysSinceLastOpened))
             } else {
-                DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) ineligible (POS last opened \(daysSinceLastOpened) days ago)")
-                return false
+                return .ineligible(reason: .notOpenedRecently(daysSinceOpened: daysSinceLastOpened))
             }
         } else {
             // Within 30 days of first sync - always eligible (new user grace period)
-            DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (within grace period: \(daysSinceFirstSync) days since first sync)")
-            return true
+            return .eligible(reason: .recentFirstSync(daysSinceFirstSync: daysSinceFirstSync))
+        }
+    }
+
+    enum SyncEligibility {
+        case eligible(reason: EligibleReason)
+        case ineligible(reason: IneligibleReason)
+
+        enum EligibleReason {
+            case neverSynced
+            case openedRecently(daysSinceOpened: Int)
+            case recentFirstSync(daysSinceFirstSync: Int)
+        }
+
+        enum IneligibleReason {
+            case notEligibleForLocalCatalog
+            case notOpenedRecently(daysSinceOpened: Int)
+            case neverOpenedAndPastFirstSyncGracePeriod(daysSinceFirstSync: Int)
+
+            var skipReason: String {
+                switch self {
+                    case .notEligibleForLocalCatalog: "not_eligible"
+                    case .notOpenedRecently: "pos_not_opened_30_days"
+                    case .neverOpenedAndPastFirstSyncGracePeriod: "pos_not_opened_30_days"
+                }
+            }
         }
     }
 
diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift
@@ -1331,6 +1331,7 @@ extension POSCatalogSyncCoordinatorTests {
         let syncSkipped = mockAnalytics.trackedEvents.first { $0.eventName == "local_catalog_sync_skipped" }
         #expect(syncSkipped != nil)
         #expect(syncSkipped?.properties?["reason"] as? String == "no_full_sync")
+        #expect(syncSkipped?.properties?["sync_type"] as? String == "incremental")
     }
 
     @Test func performFullSyncIfApplicable_tracks_sync_skipped_when_not_stale() async throws {
@@ -1354,6 +1355,87 @@ extension POSCatalogSyncCoordinatorTests {
         let syncSkipped = mockAnalytics.trackedEvents.first { $0.eventName == "local_catalog_sync_skipped" }
         #expect(syncSkipped != nil)
         #expect(syncSkipped?.properties?["reason"] as? String == "catalog_not_stale")
+        #expect(syncSkipped?.properties?["sync_type"] as? String == "full")
+    }
+
+    // MARK: - Sync Type Analytics
+
+    @Test("POSCatalogSyncType enum has correct raw values")
+    func testPOSCatalogSyncTypeRawValues() {
+        #expect(POSCatalogSyncType.full.rawValue == "full")
+        #expect(POSCatalogSyncType.incremental.rawValue == "incremental")
+    }
+
+    // MARK: - POS Not Opened 30 Days Skip Reason Tests
+
+    @Test func performFullSyncIfApplicable_tracks_pos_not_opened_30_days_when_not_opened_recently() async throws {
+        // Given - Store with first sync > 30 days ago and last opened > 30 days ago
+        let mockAnalytics = MockAnalytics()
+        let mockSiteSettings = MockSiteSpecificAppSettingsStoreMethods()
+
+        // Set first sync date to 40 days ago
+        let firstSyncDate = Calendar.current.date(byAdding: .day, value: -40, to: Date())!
+        mockSiteSettings.setFirstPOSCatalogSyncDate(siteID: sampleSiteID, date: firstSyncDate)
+
+        // Set last opened date to 35 days ago (more than 30 days)
+        let lastOpenedDate = Calendar.current.date(byAdding: .day, value: -35, to: Date())!
+        mockSiteSettings.setPOSLastOpenedDate(siteID: sampleSiteID, date: lastOpenedDate)
+
+        // Create site in database with full sync date
+        try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: firstSyncDate)
+
+        let sut = POSCatalogSyncCoordinator(
+            fullSyncService: mockSyncService,
+            incrementalSyncService: mockIncrementalSyncService,
+            grdbManager: grdbManager,
+            catalogEligibilityChecker: mockEligibilityChecker,
+            siteSettings: mockSiteSettings,
+            analytics: mockAnalytics,
+            connectivityObserver: nil
+        )
+
+        // When - Try to perform sync
+        try? await sut.performFullSyncIfApplicable(for: sampleSiteID, maxAge: .zero)
+
+        // Then - Should track pos_not_opened_30_days
+        let syncSkipped = mockAnalytics.trackedEvents.first { $0.eventName == "local_catalog_sync_skipped" }
+        #expect(syncSkipped != nil)
+        #expect(syncSkipped?.properties?["reason"] as? String == "pos_not_opened_30_days")
+        #expect(syncSkipped?.properties?["sync_type"] as? String == "full")
+    }
+
+    @Test func performIncrementalSyncIfApplicable_tracks_pos_not_opened_30_days_when_never_opened_and_past_grace_period() async throws {
+        // Given - Store with first sync > 30 days ago and never opened POS
+        let mockAnalytics = MockAnalytics()
+        let mockSiteSettings = MockSiteSpecificAppSettingsStoreMethods()
+
+        // Set first sync date to 40 days ago
+        let firstSyncDate = Calendar.current.date(byAdding: .day, value: -40, to: Date())!
+        mockSiteSettings.setFirstPOSCatalogSyncDate(siteID: sampleSiteID, date: firstSyncDate)
+
+        // Don't set last opened date (nil = never opened)
+
+        // Create site in database with full sync date
+        try createSiteInDatabase(siteID: sampleSiteID, lastFullSyncDate: firstSyncDate)
+
+        let sut = POSCatalogSyncCoordinator(
+            fullSyncService: mockSyncService,
+            incrementalSyncService: mockIncrementalSyncService,
+            grdbManager: grdbManager,
+            catalogEligibilityChecker: mockEligibilityChecker,
+            siteSettings: mockSiteSettings,
+            analytics: mockAnalytics,
+            connectivityObserver: nil
+        )
+
+        // When - Try to perform incremental sync
+        try await sut.performIncrementalSyncIfApplicable(for: sampleSiteID, maxAge: .zero)
+
+        // Then - Should track pos_not_opened_30_days
+        let syncSkipped = mockAnalytics.trackedEvents.first { $0.eventName == "local_catalog_sync_skipped" }
+        #expect(syncSkipped != nil)
+        #expect(syncSkipped?.properties?["reason"] as? String == "pos_not_opened_30_days")
+        #expect(syncSkipped?.properties?["sync_type"] as? String == "incremental")
     }
 }
 
diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSLocalCatalogEligibilityServiceTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSLocalCatalogEligibilityServiceTests.swift
@@ -536,4 +536,31 @@ struct POSLocalCatalogEligibilityServiceTests {
         #expect(reason == expectedReason)
         #expect(sizeChecker.checkCatalogSizeCallCount == 0)
     }
+
+    // MARK: - Skip Reason Analytics
+
+    @Test("POSLocalCatalogIneligibleReason skipReason returns correct analytics string")
+    func testSkipReasonReturnsCorrectAnalyticsString() {
+        #expect(POSLocalCatalogIneligibleReason.posTabNotEligible.skipReason == "pos_not_eligible")
+        #expect(POSLocalCatalogIneligibleReason.featureFlagDisabled.skipReason == "feature_flag_disabled")
+        #expect(POSLocalCatalogIneligibleReason.unsupportedWooCommerceVersion(minimumVersion: "10.3.0").skipReason == "unsupported_woocommerce_version")
+        #expect(POSLocalCatalogIneligibleReason.catalogSizeTooLarge(totalCount: 1500, limit: 1000).skipReason == "catalog_too_large")
+        #expect(POSLocalCatalogIneligibleReason.catalogSizeCheckFailed(underlyingError: "error").skipReason == "catalog_size_check_failed")
+    }
+
+    @Test("Skip reason strings are consistent regardless of associated values")
+    func testSkipReasonConsistentRegardlessOfAssociatedValues() {
+        // Test that associated values don't affect the skip reason string
+        let version1 = POSLocalCatalogIneligibleReason.unsupportedWooCommerceVersion(minimumVersion: "10.3.0")
+        let version2 = POSLocalCatalogIneligibleReason.unsupportedWooCommerceVersion(minimumVersion: "11.0.0")
+        #expect(version1.skipReason == version2.skipReason)
+
+        let sizeTooLarge1 = POSLocalCatalogIneligibleReason.catalogSizeTooLarge(totalCount: 1500, limit: 1000)
+        let sizeTooLarge2 = POSLocalCatalogIneligibleReason.catalogSizeTooLarge(totalCount: 2000, limit: 1000)
+        #expect(sizeTooLarge1.skipReason == sizeTooLarge2.skipReason)
+
+        let checkFailed1 = POSLocalCatalogIneligibleReason.catalogSizeCheckFailed(underlyingError: "error1")
+        let checkFailed2 = POSLocalCatalogIneligibleReason.catalogSizeCheckFailed(underlyingError: "error2")
+        #expect(checkFailed1.skipReason == checkFailed2.skipReason)
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -139,9 +139,10 @@ extension WooAnalyticsEvent {`
`139`	`139`	`error: error)`
`140`	`140`	`}`
`141`	`141`
`142`		`- public static func syncSkipped(reason: String) -> WooAnalyticsEvent {`
	`142`	`+ public static func syncSkipped(reason: String, syncType: String) -> WooAnalyticsEvent {`
`143`	`143`	`WooAnalyticsEvent(statName: .pointOfSaleLocalCatalogSyncSkipped,`
`144`		`- properties: [Key.reason: reason])`
	`144`	`+ properties: [Key.reason: reason,`
	`145`	`+ Key.syncType: syncType])`
`145`	`146`	`}`
`146`	`147`	`}`
`147`	`148`	`}`