wp-calypso
wp-calypso copied to clipboard
Emails: add post checkout professional email tracking event
Proposed Changes
Currently, we don't have the calypso_page_view
tracking event to track opening the post-checkout Professional Email page.
With this PR, we have the event.
Testing Instructions
- Create a site
- Go through the flow for selecting a domain and a paid plan
- Checkout
- A Professional Email upsell screen will show up (/checkout/offer-professional-email/:Domain/:receipt_id/:site)
Expected and actual result: in the Tracks Vigilante appeared event calypso_page_view
, and in some time, the same event will appear in our "Tracks" system.
Pre-merge Checklist
- [ ] Have you written new tests for your changes?
- [ ] Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
- [x] Have you checked for TypeScript, React or other console errors?
- [ ] Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
- [ ] Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
Related to # 1200182182542585-as-1202916437620570/f
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:
Sections (~60 bytes added 📈 [gzipped])
name parsed_size gzip_size
checkout +24 B (+0.0%) +60 B (+0.0%)
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.
Legend
What is parsed and gzip size?
Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.
Generated by performance advisor bot at iscalypsofastyet.com.
@nightnei Looks like the tracking call is being fired twice :(
First call:
Second call:
@wongasy good catch, thank you, fixed it - https://github.com/Automattic/wp-calypso/pull/68025/commits/838c802f44f4ffd4734aa9c90e2c5c1a880c254f
The bug and fix are not obvious, so I will explain them:
- During the initial rendering of the component we have
isFetchingCards
asfalse
, since it'strue
only when a request to the server is already sent, so the "Personal Email Upsell" page is rendered and our event is fired. - Then after sending the request to the server we have
isFetchingCards
astrue
, so as a result "Personal Email Upsell" is unmounted, and instead of it is rendered "placeholder". https://github.com/Automattic/wp-calypso/blob/add/post-checkout-professional-email-tracking-event/client/my-sites/checkout/upsell-nudge/index.tsx#L203 - Then when we received a response,
isFetchingCards
becamefalse
and the "Personal Email Upsell" page is rendered a second time. As a result previously we had a bug as blinking UI (Component -> Placeholder -> Component) and it was the reason why the event was fired twice. Additionally, you can notice that I removedisFetchingStoredCards
prop. I checked and it's useless, I don't see any place where it's used.
Test instructions:
This "placeholder" is also used for QuickstartSessionsRetirement
and BusinessPlanUpgradeUpsell
. https://github.com/Automattic/wp-calypso/blob/add/post-checkout-professional-email-tracking-event/client/my-sites/checkout/upsell-nudge/index.tsx#L315
To test that nothing broken and it works as expected you can visit this two links:
- http://calypso.localhost:3000/checkout/offer-quickstart-session?site=test-a8c-letero-nightnei-11-222222.blog
- http://calypso.localhost:3000/checkout/test-a8c-letero-nightnei-11-222222.blog/offer-plan-upgrade/business
@nightnei Thank you for the detailed explanations! I see one shortcoming with the current approach, and that is the tracking event is delayed till after all the required server calls are made. Essentially, we only track the page view when the <ProfessionalEmailUpsell>
component is rendered, which isn't ideal, since we should be tracking the view as early as possible (comment for reference).
With that in mind, it'd be best to add the tracking code in <UpsellNudge>
to ensure we fire the tracking call asap. However, that isn't possible at the moment, since each upsell component appear to have their own tracking, for instance <QuickstartSessionRestirement>
has its own customized tracking code.
Here's my proposal for addressing this:
- Move the placeholder rendering logic for Professional Email Upsell to
<ProfessionalEmailUpsell>
, which I feel is a nice refactor, since the placeholder markup is specific to the Professional Email Upsell component. We can also remove theupsellType
check inrenderPlaceholders()
- Update the conditional for rendering placeholders so that we always call
renderContent()
for professional email upsell - Pass the
isLoading
property to<ProfessionalEmailUpsell>
so the component knows to render the placeholder when the data is still being fetched - Call
recordTracksEvent
to trigger the page tracking call in theuseEffect
hook to ensure it only gets fired once
@nightnei Thank you for the detailed explanations! I see one shortcoming with the current approach, and that is the tracking event is delayed till after all the required server calls are made. Essentially, we only track the page view when the
<ProfessionalEmailUpsell>
component is rendered, which isn't ideal, since we should be tracking the view as early as possible (comment for reference).With that in mind, it'd be best to add the tracking code in
<UpsellNudge>
to ensure we fire the tracking call asap. However, that isn't possible at the moment, since each upsell component appear to have their own tracking, for instance<QuickstartSessionRestirement>
has its own customized tracking code.Here's my proposal for addressing this:
- Move the placeholder rendering logic for Professional Email Upsell to
<ProfessionalEmailUpsell>
, which I feel is a nice refactor, since the placeholder markup is specific to the Professional Email Upsell component. We can also remove theupsellType
check inrenderPlaceholders()
- Update the conditional for rendering placeholders so that we always call
renderContent()
for professional email upsell- Pass the
isLoading
property to<ProfessionalEmailUpsell>
so the component knows to render the placeholder when the data is still being fetched- Call
recordTracksEvent
to trigger the page tracking call in theuseEffect
hook to ensure it only gets fired once
Get your point 👌 Only one exception regarding the last item - I will check it, but theoretically, it should be fired only once as a component, since the component receives always the same props, but I will test it carefully.
@wongasy Done
Regarding using useEffect
- I checked, and it's indeed not necessary, so I used just a simple component.