Stats: add analytics events for the pricing grid - #113372
Conversation
|
WordPress.com
Automattic for Agencies
|
f07aa1a to
fc90308
Compare
matticbot
added
the
[Status] Needs Review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM — events and prefixing are right, and the folded-in feedback checks out (symbolPosition matches the before/after pattern already used in client/dashboard/sites/site-plans/index.tsx; the fail-closed eligibility holds because PURCHASES_SITE_FETCH_FAILED marks the store loaded).
The 2s cap is a solid improvement on trunk's fire-and-forget + 250ms, but it can't close the race on its own: the mutation is retry: 1, retryDelay: 3 * 1000, so a failed first POST always retries past the cap, and with no onSuccess on the mutation, nothing invalidates the pricing_grid: true the gate cached at 2s (staleTime 30s). Someone who lands on the purchase page directly — no notices cache, so the notices && patch is a no-op — and hits a failed POST gets shown the grid right after declining a plan.
Adding an onSuccess that invalidates the notices query would close it for good, and for every other notice using that mutation. Inherited from #113366, so fine as a follow-up — after which the 2s race stops being load-bearing and could go away, along with the duplicated Promise.race block in the two skip handlers.
One small thing: the skip button has no busy state, so it can now sit unresponsive for up to 2s where it used to be 250ms.
Nikschavan
left a comment
Nikschavan
left a comment
There was a problem hiding this comment.
Thank you for the PR, this tested well. I have added some suggestions inline
|
Feedback addressed:
|
|
Looks like one of the E2E tests has failed. You can fix them following these steps:
|
There was a problem hiding this comment.
Re-reviewed after the three follow-up commits — both points from my earlier review are addressed, and the invalidation is done more carefully than I suggested: putting onSuccess at the mutation level rather than on the mutate() call is right, since query-core only runs the per-call callbacks while the calling component is still mounted, and the skip handlers navigate away before the retry lands.
ToS
| Before | After |
|---|---|
 |
 |
Feature list
| Before | After |
|---|---|
 |
 |
Events
Dropping the Promise.race follows from that — with self-correction in place the cap wasn't buying correctness any more, and the button is back to its original 250ms. Worth watching in testing: the gate can now fetch pre-dismissal state and briefly show the grid before the invalidation clears it, so a short flash after I will do it later is expected rather than a regression.
from on the skip events comes off the typed prop, and the eligibility tests cover the launch-date boundary, the plan check and the fail-closed error path.
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.
- 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).
…rchase 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.
…r 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.
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.
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.
Also widen the getPurchasesError comment: the field is shared across all purchases actions, not just the site fetch — fail-closed either way.
f79baaf to
20b7fb2
Compare
github-actions
Bot
removed
the
[Status] Needs Review
4 participants
Part of STATS-366. Follow-up to #113366 (now merged; this branch is rebased onto trunk): adds the analytics events and folds in the review feedback from #113366 (review).
Proposed Changes
Add three analytics events to the pricing grid, all through
trackStatsAnalyticsEvent(which prefixesjetpack_odyssey_/calypso_by surface) and all carryingblog_id, following the pattern established in Stats: Add blog_id to upsell impression and analytics events #113364:stats_pricing_grid_view— fires once on mount (the grid renders in place of the dashboard, so a mount is an impression).stats_pricing_grid_paid_cta_clicked— Get Paid Stats, fired before navigating to the purchase page.stats_pricing_grid_free_cta_clicked— Start for free, fired before the dismissal reveals the dashboard.The purchase page's I will do it later button already records
{prefix}_stats_purchase_commercial_skip_button_clicked/{prefix}_stats_purchase_flow_skip_button_clicked— no new event needed there. Checkout attribution is already covered by thefrom=jetpack-stats-pricing-gridreferrer on the paid path.Fold in the Stats: show a pricing grid instead of the dashboard for new sites without a plan #113366 review feedback (second commit):
getCurrencyObject'ssymbolPosition(after-symbol locales like de-DE EUR).nowrap/overflow: hidden).site‘sU+2018 apostrophe to U+2019 before translation.Await the dismissal before navigating from the purchase page (third commit, from this follow-up comment): the dismiss hook returns the mutation promise and both "I will do it later" handlers await it — bounded by a 2s cap so a hanging request (the mutation retries once after 3s) cannot leave the button looking dead — so in the healthy path the gate's notices GET on the destination route can never read the pre-dismissal state — closing the fire-and-forget race and the empty-cache
setQueryDatano-op without seeding fabricated notices state.Why are these changes being made?
The pricing grid replaces the Stats dashboard for eligible new sites, so its funnel (impressions → paid vs free choice → checkout) needs to be measurable to evaluate STATS-366. Events follow the existing stats naming and the
blog_idproperty convention from #113364 so they aggregate alongside the rest of the stats upsell analytics.Testing Instructions
jetpack_odyssey_stats_pricing_grid_viewfires withblog_id.jetpack_odyssey_stats_pricing_grid_paid_cta_clickedfires before the purchase page loads.jetpack_odyssey_stats_pricing_grid_free_cta_clickedfires and the dashboard reveals.Pre-merge Checklist