Woo installer: Add email field to address step #59970

allilevine · 2022-01-11T22:27:01Z

Changes proposed in this Pull Request

Add an email address field and pre-fill and update it.

Testing instructions

Apply https://href.li/?D72639-code and sandbox the API.
Go to the address step: http://calypso.localhost:3000/start/woocommerce-install/store-address?site=<siteId>
Your customer email address should be pre-filled.
Enter an email address.
Click continue and check that the network request to the settings API includes the email in the update to settings.

Related to #57423

github-actions · 2022-01-11T22:27:17Z

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

matticbot · 2022-01-11T22:27:36Z

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

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

name                                                                 parsed_size           gzip_size
async-load-signup-steps-woocommerce-install-step-store-address            +301 B  (+0.2%)      +92 B  (+0.2%)
async-load-signup-steps-woocommerce-install-hooks-use-site-settings        +72 B  (+1.6%)      +42 B  (+2.5%)

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.

client/signup/steps/woocommerce-install/step-store-address/index.tsx

obenland · 2022-01-11T22:50:36Z

client/signup/steps/woocommerce-install/step-store-address/index.tsx

@@ -92,6 +110,12 @@ export default function StepStoreAddress( props: WooCommerceInstallProps ): Reac
 						/>
 					</CityZipRow>

+					<TextControl
+						label={ __( 'Email address', 'woocommerce-admin' ) }


I saw you remove the 'woocommerce-admin' text domain in #59876. Should we do the same here?

I'm not sure that was intentional. 🤔 I'll take a look.

From (shaky) memory the JS translation functions adhere to text domains strictly so we need to remove it in this case to pick up calypso translations. In PHP we have some fallbacks (I think) to handle this sort of code mixing.

Got it! I'll remove here as well.

Co-authored-by: Konstantin Obenland <obenland@automattic.com>

lsl · 2022-01-11T23:23:18Z

client/signup/steps/woocommerce-install/step-store-address/index.tsx

@@ -54,6 +55,21 @@ export default function StepStoreAddress( props: WooCommerceInstallProps ): Reac
 		update( WOOCOMMERCE_DEFAULT_COUNTRY, event.target.value );
 	};

+	// @todo: Add a general hook to get and update multi-option data like the profile.


I had a look at this yesterday, was thinking instead of exposing WOOCOMMERCE_ONBOARDING_PROFILE, exposing WOOCOMMERCE_STORE_EMAIL at the hook level. Then inside the hook, conditionally managing access to the profile data. This should lead to some simple code in the hook but when trying I ran into issues with the get/update functions leveraging the const/types so that needs breaking first.

Not suggesting we change this now, just dumping some thoughts in case it helps when/if we change this later.

obenland

noahtallen · 2023-01-21T01:27:09Z

client/state/site-settings/selectors.js

@@ -43,6 +43,7 @@ export function getSiteSettingsSaveRequestStatus( state, siteId ) {
 woocommerce_store_city?: string;
 woocommerce_store_postcode?: string;
 woocommerce_default_country?: string;
+ woocommerce_onboarding_profile?: array;


Hey! I was working on #72348, which resulted in array being changed to Array. I think this caused a cascade of type issues which were unfortunately not caught in this branch!

Firstly, it seems like woocommerce_onboarding_profile might be an Object, given that it's set to Record<string, unknown>. But beyond that, I don't think JSDoc/Typescript fully understood the array type.

The result is that before I made changes in #72348, the return type for the getter function was "any", I think because Typescript couldn't infer the type:

wp-calypso/client/signup/steps/woocommerce-install/hooks/use-site-settings/index.tsx

Lines 56 to 66 in ea8bb7f

function get( option: optionNameType ) {

if ( updates[ option ] ) {

return updates[ option ];

}

if ( ! settings || Object.keys( settings ).length === 0 ) {

return '';

}

return settings[ option ] || '';

}

This made all the consuming code compile correctly, but once the JSDoc type gets fixed, Typescript starts inferring the correct return type. Unfortunately, the other types aren't specific enough, so I had to hardcode any as the return type for the getter.

To fix that, I think a couple things need to happen:

Array should probably change to Object here, unless it is an array?

The woocommerce_onboarding_profile needs to be fully typed -- at least the parts of it used in step-store-address.

The getter needs to conditionally infer the right type. So when onboarding profile is the input, then the type of the profile should be returned, and when the others are the input, then "string" is the return type. (This doesn't happen automatically unfortunately)

Something similar to this:

function get(option: typeof WOOCOMMERCE_ONBOARDING_PROFILE): OnboardingProfile; function get( option: optionNameType ): string { if ( updates[ option ] ) { return updates[ option ]; } if ( ! settings || Object.keys( settings ).length === 0 ) { return ''; } return settings[ option ] || ''; }

unfortunately, OnboardingProfile does need to be more specific than Record<string, unknown>, because the consumers need more type info to compile correctly. (Otherwise I would have just added the record type to fix it in that PR!)

I didn't want to get into that myself, since I'm not familiar with the area. Is that something someone in this PR could take a look at?

I didn't want to get into that myself, since I'm not familiar with the area. Is that something someone in this PR could take a look at?

@noahtallen Thanks for letting us know 🙌 How urgent is this fix? Is it blocking a time-sensitive change?

@daledupreez I think Serenity now owns the Woo onboarding flow, is adjusting the types here something you can pick up? Is this code still used?

Nope, it's not urgent! In the worst case, we just continue not having the right types here. Since (i guess) this has been working in prod for a while, it's not a customer-facing issue. I think the code itself works correctly, it's just that the types don't quite line up.

Add email field to address step

629a0de

allilevine self-assigned this Jan 11, 2022

allilevine added the Woop WooCommerce on Plans Pod label Jan 11, 2022

obenland reviewed Jan 11, 2022

View reviewed changes

obenland mentioned this pull request Jan 11, 2022

Woop: Collect and save store email in site settings #59858

Closed

allilevine and others added 2 commits January 11, 2022 17:57

Simplify return

2990bb6

Co-authored-by: Konstantin Obenland <obenland@automattic.com>

Simplify onChange

b2f9c66

Co-authored-by: Konstantin Obenland <obenland@automattic.com>

lsl reviewed Jan 11, 2022

View reviewed changes

allilevine added 3 commits January 12, 2022 12:15

Add correct types to hook

51af260

Update profile object with spread to preserve data

33983db

Fix TypeScript error

7c096db

allilevine marked this pull request as ready for review January 12, 2022 19:14

obenland approved these changes Jan 12, 2022

View reviewed changes

allilevine merged commit 74b53eb into trunk Jan 12, 2022

allilevine deleted the add/woop-address-step-email-field branch January 12, 2022 20:00

andres-blanco mentioned this pull request May 31, 2022

Woo Onboarding: Do we need the email address field? #64202

Closed

noahtallen reviewed Jan 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Woo installer: Add email field to address step #59970

Woo installer: Add email field to address step #59970

allilevine commented Jan 11, 2022 •

edited

Loading

github-actions bot commented Jan 11, 2022 •

edited by matticbot

Loading

matticbot commented Jan 11, 2022 •

edited

Loading

obenland Jan 11, 2022

allilevine Jan 11, 2022

lsl Jan 11, 2022

allilevine Jan 12, 2022 •

edited

Loading

lsl Jan 11, 2022 •

edited

Loading

obenland left a comment

noahtallen Jan 21, 2023 •

edited

Loading

noahtallen Jan 21, 2023

allilevine Jan 23, 2023

noahtallen Jan 23, 2023

	function get( option: optionNameType ) {
	if ( updates[ option ] ) {
	return updates[ option ];
	}

	if ( ! settings \|\| Object.keys( settings ).length === 0 ) {
	return '';
	}

	return settings[ option ] \|\| '';
	}

Woo installer: Add email field to address step #59970

Woo installer: Add email field to address step #59970

Conversation

allilevine commented Jan 11, 2022 • edited Loading

Changes proposed in this Pull Request

Testing instructions

github-actions bot commented Jan 11, 2022 • edited by matticbot Loading

matticbot commented Jan 11, 2022 • edited Loading

obenland Jan 11, 2022

Choose a reason for hiding this comment

allilevine Jan 11, 2022

Choose a reason for hiding this comment

lsl Jan 11, 2022

Choose a reason for hiding this comment

allilevine Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

lsl Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

obenland left a comment

Choose a reason for hiding this comment

noahtallen Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

noahtallen Jan 21, 2023

Choose a reason for hiding this comment

allilevine Jan 23, 2023

Choose a reason for hiding this comment

noahtallen Jan 23, 2023

Choose a reason for hiding this comment

allilevine commented Jan 11, 2022 •

edited

Loading

github-actions bot commented Jan 11, 2022 •

edited by matticbot

Loading

matticbot commented Jan 11, 2022 •

edited

Loading

allilevine Jan 12, 2022 •

edited

Loading

lsl Jan 11, 2022 •

edited

Loading

noahtallen Jan 21, 2023 •

edited

Loading