Skip to content

Conversation

@joshheald
Copy link
Contributor

@joshheald joshheald commented Nov 12, 2025

Merge after #16342

Description

This PR adds analytics for sync, launch, and loading with the local catalog.

See pdfdoF-84z-p2 for full details. Note that the checkout events will be tackled in WOOMOB-1686

Events

Event Identifier Properties Description
woo_pos_local_catalog_downloading_screen_shown   Tracked when the app doesn’t have the local catalog in local storage and the app dispalys “sync in progress” screen.
woo_pos_local_catalog_downloading_screen_exit_pos_tapped   Tracked when the user taps on Exit POS or presses back button while local catalog is being synced.
woo_pos_splash_screen_error_shown   Tracked when opening the POS fails (e.g. catalog is empty and there isn’t an internet connection).
woo_pos_splash_screen_retry_tapped   Tracked when the user taps on retry on the splash screen error.
woo_pos_local_catalog_stale_warning_shown hours_since_last_sync Tracked when the POS displays a stale catalog notice.
woo_pos_local_catalog_stale_warning_dismissed   Tracked when the user taps on dismiss on the stale catalog notice.
woo_pos_loaded(existing event) sync_strategy (remote, local_catalog) Existing event – new attribute that indicates whether the new syncing approach is enabled.
woo_pos_local_catalog_sync_started sync_type (full/incremental),connection_type (wifi/cellular/unknown) Tracked when the sync job starts the syncing.
woo_pos_local_catalog_sync_completed sync_type, products_synced, variations_synced,total_products, total_variations, sync_duration_ms Tracked when the sync job finishes successfully.
woo_pos_local_catalog_sync_failed sync_type, error_context, error_type (network_error/authentication_error/unexpected_error/insufficient_free_space/catalog_integrity), error_description Tracked when the sync job finishes unsuccessfully.
woo_pos_local_catalog_sync_skipped reason (feature_flag_disabled/user_not_logged_in/no_site_selected/pos_inactive/catalog_too_large) Tracked when the sync job is skipped.

I've tested using WPcom and site credentials connections, which can cause differences in error types

Test Steps

Some of these will be easier to test on a large store, e.g. largefuntesting.mystagingwebsite.com. To use this, set POSLocalCatalogEligibilityService.Constants.defaultCatalogSizeLimit to 100000.

When you need to see the splash screen events, log out first, then log in, as the splash screen is only shown for the first successful sync.

You can force a sync to fail by repeatedly failing a single page of products – set a network breakpoint on /wc/v3/products, then fail one particular page for it's initial attempt, and all 3 backed-off retries. Use a 500 or similar.

You can simulate database errors by editing the grdbManager.databaseConnection.write blocks in POSCatalogPersistenceService, and throwing errors there, e.g. throw DatabaseError(resultCode: .SQLITE_FULL)

Screenshots

CleanShot 2025-11-12 at 11 24 37@2x CleanShot 2025-11-12 at 11 25 00@2x CleanShot 2025-11-12 at 11 23 47@2x CleanShot 2025-11-12 at 11 19 43@2x CleanShot 2025-11-12 at 11 17 30@2x CleanShot 2025-11-12 at 11 28 23@2x CleanShot 2025-11-12 at 12 20 33@2x
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

joshheald and others added 7 commits November 12, 2025 05:19
- Simplify syncFailed event to automatically extract error details
- Map catalog_too_large to sync_skipped reason (not error)
- Add comprehensive ineligibility reason mapping for skip events
- Remove unnecessary categorizeError helper method
- Track splashScreenErrorShown when error view appears
- Track splashScreenRetryTapped when retry button is pressed
- Only track for initialCatalogSyncError type (splash screen errors)
- Completes analytics implementation for local catalog feature

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Track sync started with connection type for incremental syncs
- Track sync completed with duration and counts for incremental syncs
- Track sync failed for incremental syncs (cancellation and errors)
- Ensures pull-to-refresh sync events are tracked via performSmartSync

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Created MockAnalytics conforming to Analytics protocol for Yosemite tests
- Added 3 focused tests verifying analytics integration:
  * Full sync tracks started and completed events
  * Full sync failure tracks error_type property (network_error)
  * Incremental sync tracks started and completed events
- Tests verify event names and key properties without over-specifying

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@joshheald joshheald added this to the 23.7 milestone Nov 12, 2025
@joshheald joshheald added type: task An internally driven task. category: tracks Related to analytics, including Tracks Events. feature: POS labels Nov 12, 2025
@joshheald joshheald changed the base branch from trunk to feat/WOOMOB-1173-background-catalog-download-updated November 12, 2025 07:17
@dangermattic
Copy link
Collaborator

dangermattic commented Nov 12, 2025

1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
1 Message
📖

This PR contains changes to Tracks-related logic. Please ensure (author and reviewer) the following are completed:

  • The tracks events must be validated in the Tracks system.
  • Verify the internal Tracks spreadsheet has also been updated.
  • Please consider registering any new events.
  • The PR must be assigned the category: tracks label.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 12, 2025

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Numberpr16345-50e3729
Version23.6
Bundle IDcom.automattic.alpha.woocommerce
Commit50e3729
Installation URL7gl4bmallogd8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@joshheald joshheald changed the base branch from feat/WOOMOB-1173-background-catalog-download-updated to feat/WOOMOB-1173-parse-catalog-downloads-in-the-background November 12, 2025 09:36
@joshheald joshheald marked this pull request as ready for review November 12, 2025 10:01
@joshheald joshheald modified the milestones: 23.7, 23.8 Nov 13, 2025
Base automatically changed from feat/WOOMOB-1173-parse-catalog-downloads-in-the-background to trunk November 14, 2025 15:02
@iamgabrielma iamgabrielma self-assigned this Nov 17, 2025
Copy link
Contributor

@iamgabrielma iamgabrielma left a comment

Choose a reason for hiding this comment

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

Works well!

✅ pos_local_catalog_downloading_screen_shown
✅ pos_local_catalog_downloading_screen_exit_pos_tapped
✅ local_catalog_stale_warning_shown
✅ local_catalog_stale_warning_dismissed
✅ pos_local_catalog_sync_started

	[connection_type: wifi, sync_type: full]
	[sync_type: incremental, connection_type: wifi]

✅ pos_local_catalog_sync_completed

	[sync_duration_ms: 108584, products_synced: 4976, variations_synced: 12458, total_variations: 12458, sync_type: full, total_products: 4976]
	[total_variations: 12458, total_products: 4976, sync_type: incremental, products_synced: 0, sync_duration_ms: 2001, variations_synced: 0]

✅ pos_local_catalog_sync_failed

	[error_type: unexpected_error, sync_type: full, error_domain: Yosemite.POSCatalogSyncError, error_code: 5, error_description: Yosemite.POSCatalogSyncError.requestCancelled]

✅ pos_local_catalog_sync_skipped

	[reason: catalog_not_stale]
	[reason: catalog_too_large]

I did not test the following 2 cases:

  • pos_splash_screen_error_shown
  • pos_splash_screen_retry_tapped

}
.background(Color.posSurface)
.onAppear {
if isCatalogSyncing {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do this on the view's init to avoid tracking multiple events on potential view recreation?

]

// Apply prefix if: (POS mode is active AND event is in the list) OR event is a local catalog event
guard (Self.isPOSModeActive && pointOfSaleEventList.contains(event)) || localCatalogEventList.contains(event) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, when decorating pos events we've always considered that POS mode would be active...until now. Each time we deal with tracking events makes me wonder of the scalability of what we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Even just finding and remembering to add them to the list is a pain, tbh...

Copy link
Contributor

Choose a reason for hiding this comment

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

A bunch of these seem to fallback to network_error, do we plan to specialize them further later on? Otherwise perhaps can be simplified, ie: classifyURLError differentiates between 4 cases to just log network_error anyway, so:

    private static func classifyURLError(_ error: URLError) -> String {
        switch error.code {
        case .timedOut:
            return "network_timeout"
        default:
            return "network_error"
        }
    }

return WooAnalyticsEvent(statName: .orderDetailWaitingTimeLoaded, properties: [Keys.waitingTime: elapsedTime])
return WooAnalyticsEvent(
statName: .orderDetailWaitingTimeLoaded,
properties: [Keys.waitingTime: elapsedTime].merging(typedAdditionalProperties) { $1 })
Copy link
Contributor

Choose a reason for hiding this comment

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

All of them seem to be using waiting time + additional props, we could declare this at the top:

            let props = [Keys.waitingTime: elapsedTime]
                .merging(additionalProperties.mapValues { $0 as WooAnalyticsEventPropertyType }) { $1 }

            switch scenario {
            case .orderDetails:
                return WooAnalyticsEvent(
                    statName: .orderDetailWaitingTimeLoaded,
                    properties: props)

And update the case for pointOfSaleLoaded

}
// Track sync failed analytics
trackAnalytics(WooAnalyticsEvent.LocalCatalog.syncFailed(
syncType: "full",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe is worth to make an enum for the syncType we use in analytics

}
} catch {
DDLogError("⛔️ POSCatalogSyncCoordinator: Failed to get storage counts: \(error)")
return (0, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, just for readability

Suggested change
return (0, 0)
return (products: 0, variations: 0)

// Map ineligibility reasons to skip reasons
switch reason {
case .posTabNotEligible:
return "pos_inactive"
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider extending or moving this to POSLocalCatalogIneligibleReason

extension POSLocalCatalogIneligibleReason {
    var skipReason: String {
        switch self {
        case .posTabNotEligible:
            return "pos_inactive"
        case .featureFlagDisabled:
     
...

grdbManager: ServiceLocator.grdbManager,
catalogEligibilityChecker: eligibilityService
catalogEligibilityChecker: eligibilityService,
siteSettings: nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

siteSettings is currently only set internally on POSCatalogSyncCoordinator, consider if we need to expose it or we can keep it private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also use it in tests. We could make a new internal init for that but it seems pretty clunky. We can perhaps just rely on the default nil to omit it here though.

@iamgabrielma
Copy link
Contributor

iamgabrielma commented Nov 17, 2025

Forgot to add some notes from testing:

The local catalog settings tab does not seem to show if there's an error syncing, which disallows me to retry and click on the full sync button if this happens. It could be coincidence as well, since the tab is a bit finicky and does not always show at the moment.

When you need to see the splash screen events, log out first, then log in, as the splash screen is only shown for the first successful sync.

I couldn't trigger this one to show the splash screen when log in/out, neither uninstalling the app. Not specifically related to the event itself.

@joshheald
Copy link
Contributor Author

The local catalog settings tab does not seem to show if there's an error syncing, which disallows me to retry and click on the full sync button if this happens. It could be coincidence as well, since the tab is a bit finicky and does not always show at the moment.

Yeah, I'll add a new ticket for this. Because it's essentially settings screen UI I've not really done anything about it yet, but it's at risk of getting missed.

I couldn't trigger this one to show the splash screen when log in/out, neither uninstalling the app. Not specifically related to the event itself.

No worries, I've tested them and they work. Couldn't have registered them otherwise! It's easier with a large catalog store and overriding the max catalog size, but you have to wait a moment so that you're deemed eligible for local catalog, so there's a slight balance. With <1000 products and fast network it's very difficult to hit it.

Thanks for the other points... I'll take a look before merging.

joshheald and others added 5 commits November 17, 2025 18:02
Consolidates switch cases that differentiate errors but return the same
value, making the code more maintainable without losing functionality.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Extracts common property merging logic to avoid repeating the same
pattern across all switch cases.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Add POSCatalogSyncType enum to replace string literals
- Add skipReason computed property to POSLocalCatalogIneligibleReason
- Improve tuple readability with named parameters
- Centralize ineligibility reason mapping to avoid duplication

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Relies on the default parameter value for cleaner call site.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@joshheald joshheald merged commit 526ca67 into trunk Nov 18, 2025
14 checks passed
@joshheald joshheald deleted the feat/woomob-1104-local-catalog-analytics branch November 18, 2025 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: tracks Related to analytics, including Tracks Events. feature: POS type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants