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
Follow-up email-only registration test #59100
Conversation
Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-22557 |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~551 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 (~298 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/components/wp-checkout-order-review.tsx
Outdated
Show resolved
Hide resolved
@donlair There seems to be a git conflict, this will require a rebase :). Also, a unit test seems to be failing. |
b32f390
to
f2b4193
Compare
I fixed that rebase conflict. Not sure why that unit test is failing - looks like it's having trouble connecting to the logstash api, but I don't see why these changes would break that. Rerunning those tests to see if it resolves itself. |
loadExperimentAssignment( experimentCheck ).then( ( experimentObject ) => { | ||
setExperiment( experimentObject ); | ||
} ); | ||
}, [] ); |
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.
We should probably add isDesktop
to the dependencies of useEffect
here, or add a // eslint-ignore-next-line react-hooks/exhaustive-deps
above it to make it explicit that we only want to run this once.
That's weird. I tried running the failing tests manually (
Also this error: And those errors don't show up in trunk... 🤔 I wonder if |
Probably we can run this line on all the tests in the file instead of just one: wp-calypso/client/my-sites/checkout/composite-checkout/test/composite-checkout.js Line 496 in d1175b3
|
@sirbrillig @niranjan-uma-shankar getting those tests to work sent me down a number of rabbit holes, but they're passing now. This PR is ready for another look whenever you get a chance. Thanks! |
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.
Looks good to me!
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7045063 Thank you @donlair for including a screenshot in the description! This is really helpful for our translators. |
@@ -133,6 +159,7 @@ export default function WPCheckoutOrderReview( { | |||
}; | |||
|
|||
const planIsP2Plus = hasP2PlusPlan( responseCart ); | |||
const shouldShowDomainNote = experiment?.variationName && domainUrl?.includes( 'wordpress.com' ); |
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.
Noting that .includes( 'wordpress.com' )
will be positive also for domains like:
maliciouswordpress.com
wordpress.com.somemalicioussite.com
wordpress.comalicioussite.xyz
Should we be checking if the domain ends on .wordpress.com
instead?
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.
Good point @tyxla, however I don't think it's possible to map or transfer a domain that contains "wordpress" as it's a blacklisted string, and this is checked in the domain suggestions API 2b0a4-pb.
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.
Glad to hear! I guess my comment was mostly about precision here. Having a precedent for an imprecise check somewhere can easily lead to introducing more imprecision in the future, and it could happen in a place where we don't perform a special verification.
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 indeed, and it makes sense to be specific in code to avoid unintended consequences down the line. Since this is an experiment, I don't foresee an immediate issue, but if the variant wins and we decide to launch it, we should definitely make the change you suggested. Appreciate your eyes on this @tyxla , thanks!
Translation for this Pull Request has now been finished. |
Changes proposed in this Pull Request
This is a follow-up test to the email-only test described and discussed at pbxNRc-144-p2. That flow auto-generates a site name based on the user's email address and is likely leading to a drop in purchase rate for this flow (pbxNRc-144-p2#comment-2770).
This PR changes the generated site name to one based on random words and adds a note to the checkout page. The note informs the user that they can change the site name at any point.
Here's a screenshot of the updated checkout page:
The new Explat test names are:
registration_email_only_mobile_random_usernames
registration_email_only_desktop_random_usernames
Testing instructions
[LOCALCALYPSO]/start/new
in an incognito browser/start/personal
)Related to #58404