Move calls for email magic link to data-layer #38866

gwwar · 2020-01-15T21:33:00Z

This PR cleans up some older data patterns, and moves calls to /auth/send-login-email from undocumented.js to the data-layer.

Part of #38861 this will enable cleaner reuse in other components.

For reference the new sendEmailLogin may trigger two types of emails, one for Native Mobile App logins, and the other for WordPress.com logins

Mobile App Email Login	Desktop Login

update usage in /me/get-apps
update usage in /client/blocks/app-promo/index.jsx
update usage in logged out view
remove fetchMagicLoginRequestEmail from undocumented.js
Add full testing instruction and go through manual testing again

Testing Instructions

/me/get-apps

Navigate to /me/get-apps
Click on "Email me a login link"
See the info notice appear
Info notice should be removed when a success or error msg occurs (note that I think a8c users may not get a magic link, use a test account). The info notice feels a bit unnecessary to me, but this was existing behavior.
With localStorage.debug='calypso:analytics*'; Verify that a tracks event called calypso_get_apps_magic_link_button_click fires.
If successful, verify that an email was sent to your inbox
Verify that the sent email is for the native app login and that it works

Reader Promo

Navigate to Reader, http://calypso.localhost:3000
Update code in client/reader/sidebar/promo.jsx and client/blocks/app-promo/index.jsx to force display of the mobile promo
Click on the promo the dialog should display
Verify this analytic event fires

- If successful, verify that a mobile email was sent - We should see a modal pop up with a message that an email has been sent

Login Flow

Make sure this branch is checked out, then run npm start (changes won't be picked up if folks switched branches while Calypso was running)
While logged out try to navigate to a Calypso page
Click to login
Then click on the Email me a login link option
Type in your email
Verify that we see the following UI and analytics events fire

- Verify that the email sent is the **desktop** login variant and that it works - On Error (try an email you own that is not associated with an account) we should see an error in UI and the following analytics events fire.

Login Flow with User account that has no password

I'm actually not sure how to create a user account like that, but I'm reasonably sure the code is correct here.

Edit: according to @kriskarkoski we should be able to create an account like this starting from https://wordpress.com/start/user?ref=logged-out-homepage-lp and choosing apple or google for user creation.

matticbot · 2020-01-15T21:33:03Z

Test live: https://calypso.live/?branch=update/home-go-mobile

matticbot · 2020-01-15T21:45:48Z

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

App Entrypoints (~25886 bytes added 📈 [gzipped])

name                   parsed_size            gzip_size
entry-jetpack-cloud       -92803 B  (-38.5%)   -25362 B  (-34.8%)
entry-login               +29676 B   (+2.6%)   +13293 B   (+4.6%)
entry-gutenboarding       +28584 B   (+1.6%)   +12677 B   (+2.6%)
entry-main                +28392 B   (+1.7%)   +12639 B   (+3.2%)
entry-domains-landing     +28392 B   (+5.5%)   +12639 B   (+9.7%)

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

Sections (~27809 bytes added 📈 [gzipped])

name                   parsed_size             gzip_size
zoninator                -833884 B   (-76.2%)  -194559 B   (-73.4%)
woocommerce              +797992 B   (+60.9%)  +187934 B   (+51.9%)
signup                   -134336 B   (-45.9%)   -32415 B   (-44.8%)
devdocs                   +95765 B  (+173.9%)   +26432 B  (+186.6%)
wp-super-cache            +75707 B   (+51.9%)   +15682 B   (+39.3%)
purchases                 +40447 B    (+4.4%)   +18325 B    (+8.9%)
site-blocks                +2416 B    (+1.0%)     +641 B    (+1.0%)
security                   +2416 B    (+0.6%)     +641 B    (+0.6%)
privacy                    +2416 B    (+1.1%)     +641 B    (+1.1%)
notification-settings      +2416 B    (+0.8%)     +641 B    (+0.8%)
me                         +2416 B    (+1.1%)     +641 B    (+1.1%)
help                       +2416 B    (+0.5%)     +641 B    (+0.6%)
happychat                  +2416 B    (+1.0%)     +641 B    (+1.0%)
checklist                  +2416 B    (+0.8%)     +641 B    (+0.9%)
account-close              +2416 B    (+0.9%)     +641 B    (+0.9%)
account                    +2416 B    (+0.8%)     +641 B    (+0.8%)

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 (~11905 bytes removed 📉 [gzipped])

name                                 parsed_size            gzip_size
async-load-lib-preferences-helper       -51122 B  (-78.0%)   -13779 B  (-74.8%)
async-load-lib-happychat-connection     -13219 B  (-17.7%)     +547 B   (+3.2%)
async-load-reader-sidebar                +3628 B   (+6.9%)     +942 B   (+7.0%)
async-load-design-blocks                  +736 B   (+0.0%)     +385 B   (+0.1%)

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.

gwwar · 2020-01-16T00:24:10Z

If I tagged the wrong folks, feel free to suggest more appropriate reviewer from @Automattic/hogwarts

dmsnell · 2020-01-16T00:25:19Z

this.props
	.fetchMagicLoginRequestEmail( usernameOrEmail, this.props.redirectTo )
	.then( () => {
		this.props.recordTracksEvent( 'calypso_login_email_link_success' );
	} )
	.catch( error => {
		this.props.recordTracksEvent( 'calypso_login_email_link_failure', {
			error_code: error.error,
			error_message: error.message,
		} );
	} );

In these cases did you try to send the analytics event names in the original action? I know it sounds a little dirty but I think it could one way to remove the coupling.

sendMobileEmailLogin( email, { showGlobalNotices: true, statsType: 'login-form' } )

export const displaySuccessMessage = ( { showGlobalNotices, infoNoticeId = null, statsType } ) => [
 	// Default Global Notice Handling
 	...( showGlobalNotices
 		? [
 			removeNotice( infoNoticeId ),
 			successNotice( translate( 'Email Sent. Check your mail app!' ), {
 				duration: 4000,
 			} ),
 		  ]
 		: [] ),
	statsTypes === 'login' ?
		tracksEvent( 'calypso_login_block_login_form_send_magic_link_success' ) :
	statsTypes === 'magic-login' ?
		tracksEvent( 'calypso_login_email_link_success' ) :
	emptyAction
 ];

or since they are just actions, maybe even passing the onSuccess/onFailure tracks events as properties on the action

sendMobileEmailLogin( email, { 
	showGlobalNotices: true,
	actionOnAPISuccess: tracksEvent( … ),
	actionOnAPIFailure: tracksEvent( … )
} )

export const displaySuccessMessage = ( { showGlobalNotices, infoNoticeId = null, actionOnAPISuccess } ) => [
 	// Default Global Notice Handling
 	...( showGlobalNotices
 		? [
 			removeNotice( infoNoticeId ),
 			successNotice( translate( 'Email Sent. Check your mail app!' ), {
 				duration: 4000,
 			} ),
 		  ]
 		: [] ),
	actionOnAPISuccess
 ];

gwwar · 2020-01-16T00:43:01Z

I did try passing arrays of arbitrary actions in 76195ba but it does require some action decoration in the error handler (it expects error information for example).

I could try that again tomorrow and see if it doesn't bloat this PR.

aerych

I've tested locally and with the branch's test site. Happy to confirm that the promo and the get-apps CTA both still worked for me with these changes.
I'll leave it to others with modern JS skills to comment on the code and approve.

.eslintrc.js

blowery · 2020-01-16T14:50:20Z

client/state/mobile-apps/actions.js

+
+export const sendMobileEmailLogin = ( email, { redirectTo, showGlobalNotices = false } ) => {
+	//Kind of weird usage, but this is a straight port from undocumented.js for now.
+	//I can move this to the caller, if there's equivalent info in the state tree


I think @jsnajdr had some thoughts on where the authoritative l10n info should come from?

Happy to update, if y'all had recommendations 👍

gwwar · 2020-01-16T23:03:08Z

d59f35185bd4581ceb3567939ca1edd24af91104 hasn't been manually tested yet but I think this approach should be viable. Ran into some existing lint errors (had to no-verify to bypass)

mmtr · 2020-01-17T11:25:10Z

Ran into some existing lint errors (had to no-verify to bypass)

I think this is caused by changes in #38190. Asked for help in p4TIVU-9l4-p2#comment-10027.

EDIT: A rebase should fix it: #38893

gwwar · 2020-01-17T15:30:59Z

Thanks I'll try rebasing @mmtr !

… and error events

gwwar · 2020-01-17T18:26:08Z

Looking at usages, I was only able to find an import for client/blocks/login/login-form.jsx in a test file. Do we still need this block?

gwwar · 2020-01-17T18:48:32Z

oh hmm there are usages calling recordTracksEventWithClientId with an alias of recordTracksEvent that doesn't actually return a plain object here.

🤔@jsnajdr if someone wanted to dispatch recordTracksEventWithClientId in a data-layer handler, what does usage look like?

blowery · 2020-01-17T18:55:08Z

Looking at usages, I was only able to find an import for client/blocks/login/login-form.jsx in a test file. Do we still need this block?

@gwwar ditto, I don't see any usage. Looks safe to drop.

gwwar · 2020-01-17T19:20:22Z

@gwwar ditto, I don't see any usage. Looks safe to drop

@blowery looks like there was a relative import in client/blocks/login, so I added it back

gwwar · 2020-01-17T19:59:12Z

Oh hmm, okay maybe I'll just try the inverse again and let the data-layer handler be knowledgeable about all flows. If this doesn't work I'll scale this PR back out of login form flows

gwwar · 2020-01-17T22:08:38Z

All set for another look 👍 I think lint will probably be unhappy from autofocus, but I didn't want to change behavior here.

dmsnell · 2020-01-17T22:41:39Z

client/state/data-layer/wpcom/auth/send-login-email/index.js

+			: [] ),
+		...( loginFormFlow
+			? [ recordTracksEventWithClientId( 'calypso_login_block_login_form_send_magic_link' ) ]
+			: [] ),


😄

I know I know I know, but still, I approve these changes. thanks for making a hard call

I do hope folks know we can also add add tracks and other types of analytics events in the API in server code :D I think it's a lot less interesting for the client to track non-user initiated actions.

mmtr · 2020-01-20T14:57:08Z

client/blocks/app-promo/index.jsx

-			scheme: 'wordpress',
-		} );
-
+		this.props.sendEmailLogin( email, { showGlobalNotices: false, isMobileAppLogin: true } );


Shouldn't we defer isMobileAppLogin to this.state.promoItem.type === 'mobile'? Or am I'm missing the intent of the promoItem object?

Nevermind, just noted sendMagicLink is only called from mobilePromo

mmtr

Overall this worked pretty well on my tests. Nice refactor @gwwar!

I couldn't fully review the code yet, but I'm not sure if we should check for the promo type on the app promo component. Will try to review rest of code before heading out.

I wanted to comment, not approve 😅

mmtr

Alright, code looks good to me 👍 E2E failures seem unrelated.

Login Flow with User account that has no password
I'm actually not sure how to create a user account like that, but I'm reasonably sure the code is correct here.

Yeah, I'm confident too. We can always open a follow up PR if we spot any regression.

gwwar · 2020-01-20T18:17:29Z

Thanks @mmtr, I'll give this a shot and will revert if this causes regressions.

gwwar added [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Janitorial labels Jan 15, 2020

gwwar self-assigned this Jan 15, 2020

gwwar removed the [Status] In Progress label Jan 16, 2020

gwwar requested review from a team, aerych and jkmassel and removed request for aerych January 16, 2020 00:18

jkmassel requested a review from aerych January 16, 2020 03:06

aerych reviewed Jan 16, 2020

View reviewed changes

mmtr reviewed Jan 16, 2020

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

blowery reviewed Jan 16, 2020

View reviewed changes

gwwar force-pushed the update/home-go-mobile branch from 5d434f3 to d59f351 Compare January 16, 2020 23:01

gwwar added the [Status] In Progress label Jan 16, 2020

gwwar added 6 commits January 17, 2020 11:14

Add data-layer hander for email login

6920276

update send-login-email usage in get-apps

9c39d73

update usage to sendMobileEmailLogin

e1099e9

try removing dialog

b65a337

add option to toggle global notices and add option for custom success…

dcfbca0

… and error events

remove custom error and success dispatches

c2076a7

mmtr and others added 3 commits January 17, 2020 11:14

Fix no undefined types linter issue

b087480

Use globalThis on jsdoc

0cd0937

Try dispatching all the actions!

35c6b1f

gwwar force-pushed the update/home-go-mobile branch from d59f351 to 35c6b1f Compare January 17, 2020 17:20

gwwar force-pushed the update/home-go-mobile branch from fd6a6f1 to 35c6b1f Compare January 17, 2020 19:19

gwwar added 4 commits January 17, 2020 14:33

move recordTracksEventWithClientId calls to data-layer handler

ece7838

make sure the right type of email is sent

f968f91

refactor to make usage clear between mobile app and normal email login

aece705

add jsdoc

880f8eb

gwwar removed the [Status] In Progress label Jan 17, 2020

dmsnell reviewed Jan 17, 2020

View reviewed changes

mmtr reviewed Jan 20, 2020

View reviewed changes

mmtr previously approved these changes Jan 20, 2020

View reviewed changes

mmtr approved these changes Jan 20, 2020

View reviewed changes

gwwar merged commit 5365eac into master Jan 20, 2020

gwwar deleted the update/home-go-mobile branch January 20, 2020 18:18

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 20, 2020

gwwar mentioned this pull request Jan 20, 2020

Customer Home: Add link to text or email a download app link #38949

Closed

gwwar mentioned this pull request Jun 10, 2020

Extract Inline Help action thunk in favour of data layer. #42698

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move calls for email magic link to data-layer #38866

Move calls for email magic link to data-layer #38866

gwwar commented Jan 15, 2020 •

edited

Loading

matticbot commented Jan 15, 2020

matticbot commented Jan 15, 2020 •

edited

Loading

gwwar commented Jan 16, 2020

dmsnell commented Jan 16, 2020 •

edited

Loading

gwwar commented Jan 16, 2020

aerych left a comment

blowery Jan 16, 2020

gwwar Jan 16, 2020

gwwar commented Jan 16, 2020

mmtr commented Jan 17, 2020 •

edited

Loading

gwwar commented Jan 17, 2020

gwwar commented Jan 17, 2020

gwwar commented Jan 17, 2020 •

edited

Loading

blowery commented Jan 17, 2020

gwwar commented Jan 17, 2020

gwwar commented Jan 17, 2020

gwwar commented Jan 17, 2020

dmsnell Jan 17, 2020

gwwar Jan 17, 2020 •

edited

Loading

mmtr Jan 20, 2020

mmtr Jan 20, 2020

mmtr left a comment

mmtr left a comment

gwwar commented Jan 20, 2020

Move calls for email magic link to data-layer #38866

Move calls for email magic link to data-layer #38866

Conversation

gwwar commented Jan 15, 2020 • edited Loading

Testing Instructions

/me/get-apps

Reader Promo

Login Flow

Login Flow with User account that has no password

matticbot commented Jan 15, 2020

matticbot commented Jan 15, 2020 • edited Loading

gwwar commented Jan 16, 2020

dmsnell commented Jan 16, 2020 • edited Loading

gwwar commented Jan 16, 2020

aerych left a comment

Choose a reason for hiding this comment

blowery Jan 16, 2020

Choose a reason for hiding this comment

gwwar Jan 16, 2020

Choose a reason for hiding this comment

gwwar commented Jan 16, 2020

mmtr commented Jan 17, 2020 • edited Loading

gwwar commented Jan 17, 2020

gwwar commented Jan 17, 2020

gwwar commented Jan 17, 2020 • edited Loading

blowery commented Jan 17, 2020

gwwar commented Jan 17, 2020

gwwar commented Jan 17, 2020

gwwar commented Jan 17, 2020

dmsnell Jan 17, 2020

Choose a reason for hiding this comment

gwwar Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

mmtr Jan 20, 2020

Choose a reason for hiding this comment

mmtr Jan 20, 2020

Choose a reason for hiding this comment

mmtr left a comment

Choose a reason for hiding this comment

mmtr left a comment

Choose a reason for hiding this comment

gwwar commented Jan 20, 2020

gwwar commented Jan 15, 2020 •

edited

Loading

matticbot commented Jan 15, 2020 •

edited

Loading

dmsnell commented Jan 16, 2020 •

edited

Loading

mmtr commented Jan 17, 2020 •

edited

Loading

gwwar commented Jan 17, 2020 •

edited

Loading

gwwar Jan 17, 2020 •

edited

Loading