wp-calypso icon indicating copy to clipboard operation
wp-calypso copied to clipboard

Emails: add post checkout professional email tracking event

Open nightnei opened this issue 2 years ago • 6 comments

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

nightnei avatar Sep 20 '22 12:09 nightnei

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.

matticbot avatar Sep 20 '22 12:09 matticbot

@nightnei Looks like the tracking call is being fired twice :(

First call: Screen Shot 2022-09-20 at 9 48 08 PM

Second call: Screen Shot 2022-09-20 at 9 48 16 PM

@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:

  1. During the initial rendering of the component we have isFetchingCards as false, since it's true only when a request to the server is already sent, so the "Personal Email Upsell" page is rendered and our event is fired.
  2. Then after sending the request to the server we have isFetchingCards as true, 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
  3. Then when we received a response, isFetchingCards became false 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 removed isFetchingStoredCards 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:

  1. http://calypso.localhost:3000/checkout/offer-quickstart-session?site=test-a8c-letero-nightnei-11-222222.blog
  2. http://calypso.localhost:3000/checkout/test-a8c-letero-nightnei-11-222222.blog/offer-plan-upgrade/business

nightnei avatar Sep 21 '22 13:09 nightnei

@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 the upsellType check in renderPlaceholders()
  • 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 the useEffect hook to ensure it only gets fired once

wongasy avatar Sep 21 '22 18:09 wongasy

@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 the upsellType check in renderPlaceholders()
  • 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 the useEffect 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.

nightnei avatar Sep 21 '22 19:09 nightnei

@wongasy Done

Regarding using useEffect - I checked, and it's indeed not necessary, so I used just a simple component.

nightnei avatar Sep 22 '22 12:09 nightnei