Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 24, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4314a71
Track specific sync skip reasons
joshheald Nov 21, 2025
15cfef7
Add tests for skipReason computed properties and sync type
joshheald Nov 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Modules/Sources/WooFoundationCore/Analytics/WooAnalyticsEvent.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}
}
}
2 changes: 1 addition & 1 deletion Modules/Sources/Yosemite/Protocols/POSLocalCatalogEligibilityServiceProtocol.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
97 changes: 82 additions & 15 deletions Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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"
Expand Down Expand Up @@ -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)")
}
Comment on lines +728 to +748
Copy link
Contributor

@iamgabrielma iamgabrielma Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these DDLogInfo? If we don't, we could either return the bool directly or remove the function entirely (depending on the other usages of syncEligibility(for siteID: Int64) async throws -> SyncEligibility)

    func checkSyncEligibility(for siteID: Int64) async throws -> Bool {
        switch try await syncEligibility(for: siteID) {
        case .eligible:
            return true
        case .ineligible:
            return false
        }
    }

Copy link
Contributor Author

@joshheald joshheald Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're the pre-existing, I just moved them to reuse the underlying logic, and they're pretty helpful to figure out what's going on...

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
Expand All @@ -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"
Comment on lines +807 to +808
Copy link
Contributor

@iamgabrielma iamgabrielma Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok to use the same pos_not_opened_30_days value for both cases notOpenedRecently and neverOpenedAndPastFirstSyncGracePeriod? I noticed it is expected by the coordinator tests, but raising it just in case 👍

Copy link
Contributor Author

@joshheald joshheald Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, they're basically the same thing

}
}
}
}

Expand Down
82 changes: 82 additions & 0 deletions Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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")
}
}

Expand Down
27 changes: 27 additions & 0 deletions Modules/Tests/YosemiteTests/Tools/POS/POSLocalCatalogEligibilityServiceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}