Stats: Add blog_id to upsell impression and analytics events - #113364
Conversation
- Add blog_id parameter to stats_card_upsell_view tracking event in StatsCardUpsell - Add blog_id parameter to commercial site upgrade notice viewed event - Add blog_id parameter to free site upgrade notice viewed event - Add blog_id parameter to do you love Jetpack stats notice viewed event - Add blog_id parameter to all stats date control events (apply button, shortcut clicks, trigger button) - Add blog_id parameter to stats date range navigation event - Add blog_id parameter to feedback panel and card tracking events - Add blog_id parameter to stats locations module menu clicks and country filter changes - Add blog_id parameter to stats top posts module menu clicks - Change promo cards to use blog_id instead of site_id for consistency Fixes STATS-421 by ensuring upsell impression events carry blog_id so they can be properly attributed to sites and measured for impression-to-purchase conversion.
|
WordPress.com
Automattic for Agencies
|
givenSiteId is essential for tracking analytics and should not be optional.
matticbot
added
the
[Status] Needs Review
…dback modal, mini-carousel, and upsell components - Add blog_id to stats_devices_module_menu_clicked event - Add blog_id to stats_utm_dropdown_opened and stats_utm_dropdown_option_selected events - Add blog_id to feedback modal tracking events (close_form_modal, submit_form) - Change site_id to blog_id in mini-carousel events for consistency - Add blog_id to stats_card_upsell_jetpack upgrade tracking events - Add blog_id to stats_card_update_jetpack_version tracking events Ensures comprehensive blog_id coverage across all stats analytics tracking calls.
…hase and upsell flows - Add blog_id to stats_upsell events (submit, expand, learn more, view) - Add blog_id to stats_upsell_modal events (submit, view) - Add blog_id to all stats_purchase events (skip, plan switch, personal switch, classification update, checkout) - Add blog_id to stats_purchase_personal flow skip event - Add blog_id to stats_purchase_notice upgrade events - Ensure complete blog_id coverage for all stats module tracking calls This completes comprehensive blog_id tracking across the entire stats module.
- Add site_id alongside blog_id in mini-carousel and promo-cards for data continuity - Use non-null siteId prop instead of nullable selectedSiteId in StatsUpsellModal tracking - Ensures backward compatibility while transitioning to blog_id field
There was a problem hiding this comment.
Edited after reading STATS-421 — coverage check added, event registration raised, and the super-props claim below corrected.
Checked this against STATS-421 and its follow-up comment.
Coverage against the issue: complete
All three headline events are handled, and so are the eight added in the follow-up comment. Two non-obvious mappings I verified rather than assumed:
jetpack_odyssey_stats_date_picker_openedis thetrigger_buttonkey instats-date-control/index.tsx:73.jetpack_odyssey_stats_feedback_action_present_persistent_sectionisTRACKS_EVENT_DID_PRESENT_FEEDBACK_CARDinfeedback/index.tsx:22.
The useEffect dependency change the issue asked for is there too.
Part 2 of the issue is not addressed
STATS-421 asks for the five events to be registered in Tracks, and notes that none of them is, that none has an owner or description, and that jetpack_odyssey_stats_card_upsell_view already feeds four funnels. The issue makes the case that registration is the step that would have caught this defect in the first place.
Nothing in this PR does that. Either fold it in, or split it into a follow-up issue and say so here, so it doesn't get lost when this closes.
Also worth wiring the issue's verification query into the acceptance check: it expects blog_id_pct at or above 95 for the three headline events after deploy.
Missed events
These are in files this PR already touches.
1. promo-cards/index.jsx:112 — recordTracksEvent( tracksEventName ) with no properties at all. This is the click counterpart to calypso_stats_traffic_mobile_cta_jetpack_view, which the follow-up comment lists at 45.4%. The view events ~40 lines above got blog_id; the click did not, so the impression/click pair stays inconsistent.
2. components/stats-date-control/index.tsx:237 — the gated branch. Non-gated shortcut clicks now send blog_id (line 244), but gated clicks go via onGatedHandler( [ { name: … } ], … ) with no params. Gated users are the upsell audience, so the same event name will keep splitting into with- and without-blog_id rows.
3. stats-period-navigation/index.jsx:310 — events.forEach( ( event ) => recordTracksEvent( event.name, event.params ) ) in onGatedHandler. this.props.siteId is in scope and used on the next line. Same pattern untouched in stats-chart-tabs/chart-header.jsx:37 and stats-module/all-time-nav.jsx:91.
Adjacent, outside the issue's scope
Not blocking, but the three notices got _viewed only, and every sibling event in the same file was left alone:
| File | Untouched events |
|---|---|
commercial-site-upgrade-notice.tsx |
*_notice_dismissed (45/47), *_notice_support_button_clicked (56/60), stats_upgrade_clicked {type:'notice-commercial'} (63) |
do-you-love-jetpack-stats-notice.tsx |
*_notice_dismissed (50), calypso_..._upgrade_button_clicked (61), *_support_button_clicked (66), stats_upgrade_clicked {type:'notice-love-stats'} (72) |
free-site-upgrade-notice.tsx |
*_notice_dismissed (47), *_upgrade_button_clicked (59), *_support_button_clicked (64), stats_upgrade_clicked {type:'notice-free-site-upgrade'} (70) |
stats_upgrade_clicked is the one I would still pick up here: this PR adds blog_id to it in stats-card-upsell-jetpack.tsx:40 and stats-purchase-notice.jsx:30 but leaves it off in all three notices, so one event name ends up with two property shapes.
blog_id can be null
undefined is safe — recordTracksEvent strips undefined properties (packages/calypso-analytics/src/tracks.ts:270-273). null is not stripped, so those fires record blog_id: null instead of omitting the property.
The follow-up comment argues every Stats route is scoped to exactly one site, which I take as the reason this is acceptable in practice. Flagging it anyway because the acceptance bar is 95%, not 100%, and the issue itself reports 30% of card_upsell_view rows as anonymous — so it is worth knowing whether nulls are landing rather than assuming they aren't.
Nullable sources feeding the new calls:
StatsNoticeProps.siteId: number | null→ all three_viewedevents.stats-purchase-checkout-redirect.tsx:178—siteId: number | null, feedingstats_purchase_button_clicked.stats-purchase-single-item.tsx— all five props interfaces (49, 64, 72, 82, 96) declarenumber | null; four events.stats-purchase-personal.tsx:25—number | null.useSelector( getSelectedSiteId )(returnsnumber | null) instats-top-posts/nav-tabs.tsx,stats-upsell/index.tsx,mini-carousel,promo-cards.stats-locations.tsx:57andstats-module-devices.tsx:102useas numberovergetSelectedSiteId— the cast silences the type, not the runtime null.stats-date-control/index.tsx:106—getSiteId( state, slug ). The one I would look at first, since it resolves by slug rather than selected-site and is null until the site is in state.locations-nav-tabs.tsxtypesgivenSiteId: number, but its only caller (summary/index.jsx:385) is plain JSX so the type is never checked, and the value passed isgetSelectedSiteId( state ).
On the shared helper
Worth settling explicitly, because the issue argues both sides: the Suggested fix section says adding it inside trackStatsAnalyticsEvent "would cover the whole Stats surface in one change", while the Scope section says this is per-call-site rather than a helper defect.
The case for the helper (client/my-sites/stats/utils/index.js:82): it covers every trackStatsAnalyticsEvent caller in one place, including the notice events listed above, and removes the prop-drilling this PR adds in feedback/index.tsx and locations-nav-tabs.tsx. It would not cover the raw recordTracksEvent calls, so those still need the manual pass either way.
One data point in favour of the current per-call-site approach: blog_id cannot be leaned on from super props. client/lib/analytics/super-props.js:46 sets it from the selected site and super props are spread after the event properties (tracks.ts:266), so they win when present — but they are frequently absent. Every jetpack_odyssey_* event in the issue is at 0%, and even the Calypso-prefixed calypso_stats_traffic_mobile_cta_jetpack_view is only at 45.4%. So the explicit property is doing real work on both surfaces, not just Odyssey.
- Add blog_id to promo-cards click tracking - Add blog_id to GDPR notice dismiss event - Add blog_id to tier upgrade notice dismiss event - Add blog_id to free plan purchase success notice upgrade click
- Notice dismiss/CTA/support-click events in commercial, free-site, and do-you-love notices (sibling viewed events already carried blog_id) - Mini-carousel block click/dismiss events via selected site selector - Purchase page upgrade-source attribution events - Gated-upsell event forwarding in chart header, period navigation, and all-time nav (payloads built by callers never included blog_id) - Commercial and PWYW upgrade slider change events - UTM builder open/interaction events - Empty-state action interaction events
…g_id Guard the attribution event behind a ref so the siteId dependency added in the previous commit cannot double-record it while site selection resolves.
|
Thanks for the thorough pass — replying point by point. The review snapshot predates the last three commits on the branch, so several items were already picked up; commit hashes below. Missed events
Adjacent notice eventsAlso picked up in 110aba6: Part 2 — event registrationSplit into STATS-423 rather than folded in here: registration (owner + description) happens in the Tracks registry, not in this repo, and bundling it would gate the code fix on that process. The follow-up also carries the acceptance check from the issue (
|
|
Checked the four new commits — the gated handlers, the notices, One cheap follow-on that falls directly out of that decision: import { trackStatsAnalyticsEvent } from './utils';
trackStatsAnalyticsEvent( 'stats_module_expanded', { module: 'referrers' } );With no helper-level injection, that snippet is what new Stats code gets copied from, and it is exactly the shape that produced STATS-421. Since this PR is the thing establishing the convention, it seems worth writing it down where the next person will actually look: import { trackStatsAnalyticsEvent } from './utils';
const siteId = useSelector( getSelectedSiteId );
trackStatsAnalyticsEvent( 'stats_module_expanded', { module: 'referrers', blog_id: siteId } );Plus a line in the Analytics section along the lines of: every event fired on a Stats page is site-scoped and must pass That gives reviewers something concrete to point at, and it is the one guard against this recurring that does not depend on STATS-423 getting picked up. |
adamwoodnz
left a comment
adamwoodnz
left a comment
There was a problem hiding this comment.
👍 Code LGTM, but I have not run it.
Left one comment about agent docs.
github-actions
Bot
removed
the
[Status] Needs Review
stats_pricing_grid_view fires on mount, stats_pricing_grid_paid_cta_clicked and stats_pricing_grid_free_cta_clicked on the two CTAs — all through trackStatsAnalyticsEvent (which prefixes jetpack_odyssey_/calypso_ by surface) and all carrying blog_id, following the pattern from #113364. The purchase page's skip button already has its own event.
|
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/34682248 Some locales (Hebrew) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday. Hi @kangzj, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include this string: Thank you in advance! |
stats_pricing_grid_view fires on mount, stats_pricing_grid_paid_cta_clicked and stats_pricing_grid_free_cta_clicked on the two CTAs — all through trackStatsAnalyticsEvent (which prefixes jetpack_odyssey_/calypso_ by surface) and all carrying blog_id, following the pattern from #113364. The purchase page's skip button already has its own event.
* Stats pricing grid: add view and CTA analytics events stats_pricing_grid_view fires on mount, stats_pricing_grid_paid_cta_clicked and stats_pricing_grid_free_cta_clicked on the two CTAs — all through trackStatsAnalyticsEvent (which prefixes jetpack_odyssey_/calypso_ by surface) and all carrying blog_id, following the pattern from #113364. The purchase page's skip button already has its own event. * Stats pricing grid: address review feedback from #113366 - Eligibility fails closed when the purchases fetch errors: FETCH_FAILED marks the store loaded with an empty list, which read as 'no plan' and would have shown the grid to a site that holds one. An error now means 'plan state unknown' and falls back to the dashboard. - The price respects getCurrencyObject's symbolPosition, so locales that put the currency symbol after the amount (e.g. de-DE EUR) render correctly. - The Terms of Service line wraps instead of truncating: dropped the jetpack component's nowrap/overflow-hidden, which cut off the disclosure in locales that run longer than English. - Fixed the apostrophe in 'sync your site's data' (was U+2018, the opening quote) before the string goes out for translation. - Dropped the redundant 'Included' labels on the three boolean differentiator rows so the mobile fallback names the feature instead of showing a bare 'Included' with nothing identifying the row. Desktop rendering is unchanged (the default label is already 'Included', bolded via strong). * Stats pricing grid: await the dismissal before navigating from the purchase page The dismissal was fire-and-forget with a fixed 250 ms before navigating, so on a slow request the gate mounted on the destination route could issue its notices GET before the POST landed and re-render the grid after 'I will do it later'. The cache patch couldn't cover this either: the raw notices entry is absent when the purchase page was reached directly (nothing on it queries notices) or when the dwell outlived the cache's gcTime, and setQueryData with an updater returning undefined is a no-op — while seeding a fabricated notices object would feed the other notice consumers made-up server state. The dismiss hook now returns the mutation promise (mutateAsync); both skip handlers await it before navigating, still navigating if the request fails. The grid's free CTA stays same-route and unawaited, just absorbing the rejection. * Stats pricing grid: cap the awaited dismissal so the skip button never stalls The dismissal mutation retries once after 3s, so awaiting it unbounded could leave 'I will do it later' visually dead for many seconds on a flaky network. Race it against a 2s cap: the healthy path (a fast POST) stays fully ordered before navigation, while past the cap the request continues in flight and navigation proceeds. * Stats: self-correct notice dismissals with query invalidation on success The mutation retries once after 3s, so any capped await on the dismissal was arithmetically guaranteed to lose to a failed first POST, leaving the stale pricing_grid state cached for up to 30s. Invalidating the notices query from the mutation (mutation-level, since per-call callbacks are skipped once the caller unmounts on navigation) self-corrects whenever the POST lands, for every notice using this mutation. That makes the awaited navigation redundant, so the skip handlers return to the plain dismiss-and-navigate shape and the rejection handling moves into the dismiss hook. * Stats purchase: carry the referrer on the skip events The dismissal now fires regardless of referrer, so without `from` a skip that came from a module upsell is indistinguishable from one off the pricing grid. * Stats pricing grid: cover the eligibility hook with tests Also widen the getPurchasesError comment: the field is shared across all purchases actions, not just the site fetch — fail-closed either way. * Stats pricing grid: record which CTA was clicked
4 participants
Fixes STATS-421
Follow-up: STATS-423 (register the affected events in Tracks with owners and descriptions, and run the STATS-421 verification query —
blog_id_pct≥ 95 — after deploy).Why
Stats upsell impression events were not carrying the blog_id parameter, which prevented them from being tied to specific sites for impact measurement. Without blog_id, it's impossible to measure impression-to-purchase conversion and determine if upsells are reaching the right audience.
Changes
Added blog_id parameter to all stats-related tracking events:
Mini-carousel and promo-card view events keep the pre-existing
site_idkey alongside the newblog_idfor data continuity. The purchase-page upgrade-source attribution event now waits for a resolved site ID and fires at most once.Deliberately left alone:
calypso_jetpack_module_toggle— its schema is shared with non-stats producers (site-settings) that also omit blog_id; changing only the stats emitters would fragment the event.Testing