-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tracks events to Jetpack logged-out checkout. #53678
Add tracks events to Jetpack logged-out checkout. #53678
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~253 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~231 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. 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. Generated by performance advisor bot at iscalypsofastyet.com. |
client/my-sites/checkout/composite-checkout/record-analytics.js
Outdated
Show resolved
Hide resolved
Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-8211 |
This PR modifies the release build for wpcom-block-editor To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-l4k-p2 |
client/my-sites/checkout/composite-checkout/composite-checkout.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/checkout/composite-checkout/components/wp-checkout.tsx
Outdated
Show resolved
Hide resolved
if ( isJetpackCheckout ) { | ||
return isUserComingFromLoginForm | ||
? 'jetpack_site_only_coming_from_login' | ||
: 'Jetpack_site_only'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny consistency issue: the first letter is uppercase here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, good catch! Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything tested well with a WordPress.com account, and code looks good.
The only blocker as far as I'm concerned is the case typo in Jetpack_site_only
, since it can skew later data readings.
Thank you both for your thorough review. Sorry about all the little mistakes. 🤦♂️ 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Code looks good to me. I didn't test it since the changes but I trust the review of @monsieur-z and the e2es.
@elliottprogrammer, we didn't merge this one because of the problem with the redirect after the login step, is that right? If so, and since we know the bug only happens when we are behind the proxy, I could test this without the proxy to see if everything works as expected. If it does, you could safely merge it. What do you think? |
Yes exactly, the redirect after login step bug was preventing us from being able to test this PR fully, but you're right, since now we know the bug only happens behind the proxy, we could test this PR proxy disabled, and that would allow us test all aspects of this PR so we can safely merge. Great idea! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I ran the instructions with the proxy disabled, and everything worked as expected.
@@ -71,7 +72,8 @@ export default function useCreatePaymentCompleteCallback( { | |||
isComingFromUpsell?: boolean; | |||
isFocusedLaunch?: boolean; | |||
siteSlug: string | undefined; | |||
isJetpackCheckout?: boolean; | |||
isJetpackCheckout: boolean; | |||
checkoutFlow: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this in my review, but this adds a new required argument to useCreatePaymentCompleteCallback
, but doesn't change all the instances where that function is used. Notably:
const onComplete = useCreatePaymentCompleteCallback( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you for spotting this @sirbrillig. I don't see why these 2 arguments, isJetpackCheckout
and checkoutFlow
need to be required.
I created PR #54116 to make them optional args. If you don't mind, could you give it a review? 🙂
Changes proposed in this Pull Request
This PR adds tracks events to the Jetpack logged-out checkout flow. More specifically it adds events & properties for the purpose of being able to measure the number of new account signups vs existing account logins during the logged-out(site-only) checkout flow.
Asana Task: 1189109545411729-as-1199371465586276
Implementation notes:
checkout_flow
property to thecalypso_checkout_page_view
event. Thischeckout_flow
property value will be one of:wpcom_checkout
jetpack_checkout
Jetpack_site_only
jetpack_site_only_coming_from_login
wpcom_registrationless
checkout_flow
property to thecalypso_checkout_composite_payment_complete
event.calypso_checkout_wpcom_email_exists
event that occurs when the user enters an email that is already associated with a WordPress.com account.calypso_checkout_composite_login_click
event that occurs when the user clicks the link to log into their existing account.Testing instructions
yarn start
.wordpress.com
withcalypso.localhost:3000
leaving the rest of the URL intact.localStorage.debug='calypso:analytics*';
Make sure you have thePreserve Log
option selected.calypso_checkout_page_view
event and that it contains thecheckout_flow
property with the valuejetpack_site_only
.calypso_checkout_wpcom_email_exists
event with props,email
andcheckout_flow
.calypso_checkout_composite_login_click
event with props,email
andcheckout_flow
.calypso_checkout_page_view
event again and verify that it contains thecheckout_flow
property with the valuejetpack_site_only_coming_from_login
.calypso_checkout_composite_payment_complete
event and that it contains the propcheckout_flow
and it's value isjetpack_site_only_coming_from_login
;If you feel so inclined, to be thorough, you can create another JN site and complete these steps again, except instead of entering an email address that is already linked to an existing WordPress.com account, you can enter an email that is not associated to any WordPress.com account, and complete the purchase. (To do this you'll need to add,
define( 'USE_STORE_SANDBOX', true );
into/wp-content/mu-plugins/0-sandbox.php
in your sandbox. And pointpublic-api.wordpress.com
to your sandbox IP (in your hosts file). This is so we can use checkout with a test credit card. See p1HpG7-c5P-p2 for instructions on testing out the logged-out visitor checkout flow using a test credit card.)When testing this flow, in the final
calypso_checkout_composite_payment_complete
event, verify that it contains the propcheckout_flow
and it's value isjetpack_site_only
;