Test passwordless signup for new users, take 2 #58404

donlair · 2021-11-23T14:36:03Z

Changes proposed in this Pull Request

Note: This PR is a follow-up to #57846, which was merged and then reverted by #57846, due to a bug that broke non-onboarding signup flows. This PR removes the line that caused that issue.

This PR re-implements the passwordless signup, which will run as a test again the control experience. The test is configured to run via two Explat tests, one each for mobile and desktop viewports. Those test names are:

Desktop: registration_email_only_desktop_relaunch
Mobile: registration_email_only_mobile_relaunch

Context behind this test is at p58i-boI-p2#comment-52280.

Before:

After:

Testing instructions

Checkout this PR and start Calypso.
Navigate to both tests in Explat.
Add the control and treatment variation bookmarklets to your nav bar.
Navigate to /start/new on your Calypso instance and assign yourself to the treatment variation.
Repeat the tests for both desktop and mobile viewports.

Related to #57846 #58359

github-actions · 2021-11-23T14:36:26Z

Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-20652
Link to Jetpack Cloud live: https://calypso.live?image=registry.a8c.com/calypso/app:build-20652&env=jetpack

matticbot · 2021-11-23T14:37:09Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~1270 bytes removed 📉 [gzipped])

name         parsed_size           gzip_size
entry-login       -294 B  (-0.0%)    -1234 B  (-0.5%)
entry-main        -257 B  (-0.0%)      -55 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~826 bytes added 📈 [gzipped])

name              parsed_size           gzip_size
jetpack-connect       +8688 B  (+1.1%)    +1268 B  (+0.6%)
accept-invite         +8688 B  (+2.9%)    +1315 B  (+1.6%)
signup                 +678 B  (+0.2%)       +0 B
devdocs                -108 B  (-0.1%)      -37 B  (-0.1%)
posts-custom            +35 B  (+0.0%)      +25 B  (+0.0%)
posts                   +35 B  (+0.0%)      +25 B  (+0.0%)
pages                   -35 B  (-0.0%)      -70 B  (-0.1%)
gutenberg-editor        -35 B  (-0.0%)      -13 B  (-0.0%)
checkout                -35 B  (-0.0%)      -31 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.

Async-loaded Components (~8363 bytes added 📈 [gzipped])

name                                                                          parsed_size           gzip_size
async-load-signup-steps-user                                                     +13934 B  (+7.8%)    +3494 B  (+6.6%)
async-load-calypso-blocks-login-two-factor-authentication-two-factor-content       -338 B  (-1.6%)      -65 B  (-0.9%)
async-load-design-blocks                                                           -161 B  (-0.0%)     -770 B  (-0.1%)
async-load-signup-steps-plans                                                       -35 B  (-0.0%)      -78 B  (-0.1%)
async-load-calypso-blocks-editor-checkout-modal                                     -35 B  (-0.0%)     -106 B  (-0.0%)

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.
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.

sirbrillig

I didn't test the changes themselves, but assuming they're unchanged from before I'm sure they're fine. I did test that /start/wpcc loads correctly, which was the bug in the previous PR, so I'll mark this good from my point of view!

worldomonation · 2021-11-23T17:25:52Z

I've triggered Pre-Release tests for this change in the branch.

worldomonation

Normal E2E and Pre-Release E2E tests all pass. Looking good from the Quality Squad perspective!

niranjan-uma-shankar

I've added a comment on the experiment name. Feel free to deploy without a review once you've updated the experiment name since everything else LGTM.

niranjan-uma-shankar · 2021-11-24T05:57:44Z

client/signup/steps/user/index.jsx

 		const clientId = get( initialContext, 'query.oauth2_client_id', null );

 		this.setSubHeaderText( this.props );

 		if ( oauth2Signup && clientId ) {
 			this.props.fetchOAuth2ClientData( clientId );
 		}
+		if ( flowName === 'onboarding' ) {
+			const experimentCheck = this.state.isDesktop
+				? 'registration_email_only_desktop'


We'll have to create a new experiment in Abacus to start clean, since this experiment already has participants from the previous run.

client/blocks/signup-form/passwordless.jsx

tyxla · 2021-12-01T09:33:55Z

client/blocks/signup-form/passwordless.jsx

+	onFormSubmit = async ( event ) => {
+		event.preventDefault();
+
+		if ( ! this.state.email || ! emailValidator.validate( this.state.email ) ) {


Updating the state of a component based on the current value of this.state may potentially lead to inconsistent component state.

This is also documented in the React docs:

setState() does not always immediately update the component. It may batch or defer the update until later. This makes reading this.state right after calling setState() a potential pitfall. Instead, use componentDidUpdate or a setState callback (setState(updater, callback))

The easiest way to fix this is to just change setState() to the callback form, where it will always properly access the current state.email value.

Thanks @tyxla, I've run into state timing issues with useEffect, but I'm not as familiar with the class-based nuances. Quick question about this - wouldn't it be okay how it's used here since it reads the state first then sets state based on that value?

I've seen problems happening in complex components when we access it before we update the state, too. At the same time, it's very hard to reproduce those issues, so it is mostly a theoretical issue. I'll leave it up to you, but I'd suggest fixing it to ensure this.state has the expected up-to-date value in that condition.

* Passwordless signup initial commit * Bug fix and style update * Addressing review feedback. * Implementing separate tests for mobile andd desktop viewports * Copy tweak and removing reference to unused flow * Limiting test assignment to onboarding flow * Migrating to wpcom.req * Fixing bug in non-onboarding signup flows * Changing experiment names

donlair added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account. labels Nov 23, 2021

donlair requested review from niranjan-uma-shankar, sirbrillig and worldomonation November 23, 2021 14:36

donlair self-assigned this Nov 23, 2021

sirbrillig approved these changes Nov 23, 2021

View reviewed changes

worldomonation approved these changes Nov 23, 2021

View reviewed changes

niranjan-uma-shankar approved these changes Nov 24, 2021

View reviewed changes

sirbrillig changed the title ~~Update/register remove username test fix~~ Test passwordless signup for new users, take 2 Nov 24, 2021

donlair added 8 commits November 29, 2021 09:46

Passwordless signup initial commit

cf9d4b8

Bug fix and style update

925deb1

Addressing review feedback.

50caa9d

Implementing separate tests for mobile andd desktop viewports

82c9b44

Copy tweak and removing reference to unused flow

c9e09b5

Limiting test assignment to onboarding flow

b86402f

Migrating to wpcom.req

722c58e

Fixing bug in non-onboarding signup flows

e49fba4

donlair force-pushed the update/register-remove-username-test-fix branch from 2c26d0a to e49fba4 Compare November 29, 2021 14:47

Changing experiment names

aeea79c

donlair merged commit 7aac635 into trunk Nov 29, 2021

donlair deleted the update/register-remove-username-test-fix branch November 29, 2021 17:53

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 29, 2021

tyxla reviewed Dec 1, 2021

View reviewed changes

donlair mentioned this pull request Dec 12, 2021

Follow-up email-only registration test #59100

Merged

donlair mentioned this pull request Jan 4, 2022

Relaunching email-only registration test with cookies #59715

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test passwordless signup for new users, take 2 #58404

Test passwordless signup for new users, take 2 #58404

donlair commented Nov 23, 2021 •

edited

github-actions bot commented Nov 23, 2021 •

edited by matticbot

matticbot commented Nov 23, 2021 •

edited

sirbrillig left a comment

worldomonation commented Nov 23, 2021

worldomonation left a comment

niranjan-uma-shankar left a comment

niranjan-uma-shankar Nov 24, 2021

tyxla Dec 1, 2021

donlair Dec 7, 2021

tyxla Dec 8, 2021

Test passwordless signup for new users, take 2 #58404

Test passwordless signup for new users, take 2 #58404

Conversation

donlair commented Nov 23, 2021 • edited

Changes proposed in this Pull Request

Testing instructions

github-actions bot commented Nov 23, 2021 • edited by matticbot

matticbot commented Nov 23, 2021 • edited

sirbrillig left a comment

Choose a reason for hiding this comment

worldomonation commented Nov 23, 2021

worldomonation left a comment

Choose a reason for hiding this comment

niranjan-uma-shankar left a comment

Choose a reason for hiding this comment

niranjan-uma-shankar Nov 24, 2021

Choose a reason for hiding this comment

tyxla Dec 1, 2021

Choose a reason for hiding this comment

donlair Dec 7, 2021

Choose a reason for hiding this comment

tyxla Dec 8, 2021

Choose a reason for hiding this comment

donlair commented Nov 23, 2021 •

edited

github-actions bot commented Nov 23, 2021 •

edited by matticbot

matticbot commented Nov 23, 2021 •

edited