diff --git a/Modules/Sources/WooFoundationCore/Analytics/WooAnalyticsEvent.swift b/Modules/Sources/WooFoundationCore/Analytics/WooAnalyticsEvent.swift index ebcf55d5760..a79b9679610 100644 --- 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 index 6a1d43cc6ec..54dac1965a1 100644 --- 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 index d940c4126f3..48784c45919 100644 --- 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,10 +725,35 @@ 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) @@ -714,8 +761,7 @@ private extension POSCatalogSyncCoordinator { // 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 index 1dfd8b5f063..c2b0d11b178 100644 --- 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 index 540bc6e1006..d0dc42d4a4a 100644 --- 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) + } }