Skip to content

Woo installer: Add email field to address step #59970

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.

Sign up for GitHub

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

Merged
allilevine merged 6 commits into from
Jan 12, 2022
Merged

Woo installer: Add email field to address step #59970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
629a0de
Add email field to address step
allilevine Jan 11, 2022
2990bb6
Simplify return
allilevine Jan 11, 2022
b2f9c66
Simplify onChange
allilevine Jan 11, 2022
51af260
Add correct types to hook
allilevine Jan 12, 2022
33983db
Update profile object with spread to preserve data
allilevine Jan 12, 2022
7c096db
Fix TypeScript error
allilevine Jan 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions client/signup/steps/woocommerce-install/hooks/use-site-settings/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,18 @@ export const WOOCOMMERCE_STORE_ADDRESS_2 = 'woocommerce_store_address_2';
export const WOOCOMMERCE_STORE_CITY = 'woocommerce_store_city';
export const WOOCOMMERCE_DEFAULT_COUNTRY = 'woocommerce_default_country';
export const WOOCOMMERCE_STORE_POSTCODE = 'woocommerce_store_postcode';
export const WOOCOMMERCE_ONBOARDING_PROFILE = 'woocommerce_onboarding_profile';

type optionNameType =
| 'blog_public'
| typeof WOOCOMMERCE_STORE_ADDRESS_1
| typeof WOOCOMMERCE_STORE_ADDRESS_2
| typeof WOOCOMMERCE_STORE_CITY
| typeof WOOCOMMERCE_DEFAULT_COUNTRY
| typeof WOOCOMMERCE_STORE_POSTCODE;
| typeof WOOCOMMERCE_STORE_POSTCODE
| typeof WOOCOMMERCE_ONBOARDING_PROFILE;

type OptionValueType = Record< string, unknown > | string;

/**
* Simple react custom hook to deal with site settings.
Expand Down Expand Up @@ -63,7 +67,7 @@ export function useSiteSettings( siteId: number ) {
* Changes are applied to the Redux store.
*/
const update = useCallback(
( option: optionNameType, value: string ) => {
( option: optionNameType, value: OptionValueType ) => {
setEditedSettings( ( state ) => uniqueBy( [ ...state, option ] ) );

// Store the edited option in the private store.
Expand Down
25 changes: 25 additions & 0 deletions client/signup/steps/woocommerce-install/step-store-address/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
WOOCOMMERCE_STORE_CITY,
WOOCOMMERCE_DEFAULT_COUNTRY,
WOOCOMMERCE_STORE_POSTCODE,
WOOCOMMERCE_ONBOARDING_PROFILE,
} from '../hooks/use-site-settings';
import useWooCommerceOnPlansEligibility from '../hooks/use-woop-handling';
import type { WooCommerceInstallProps } from '..';
Expand Down Expand Up @@ -54,6 +55,24 @@ 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.
Copy link
Contributor

@lsl lsl Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

function updateProfileEmail( email: string ) {
const onboardingProfile = get( WOOCOMMERCE_ONBOARDING_PROFILE ) || {};

const updatedOnboardingProfile = {
...onboardingProfile,
store_email: email,
};

update( WOOCOMMERCE_ONBOARDING_PROFILE, updatedOnboardingProfile );
}

function getProfileEmail() {
const onboardingProfile = get( WOOCOMMERCE_ONBOARDING_PROFILE );

return onboardingProfile[ 'store_email' ] || '';
}

function getContent() {
return (
<>
Expand Down Expand Up @@ -92,6 +111,12 @@ export default function StepStoreAddress( props: WooCommerceInstallProps ): Reac
/>
</CityZipRow>

<TextControl
label={ __( 'Email address' ) }
value={ getProfileEmail() }
onChange={ updateProfileEmail }
/>

<ActionSection>
<SupportCard />
<StyledNextButton
Expand Down
1 change: 1 addition & 0 deletions client/state/site-settings/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

@noahtallen noahtallen Jan 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Array should probably change to Object here, unless it is an array?
  2. The woocommerce_onboarding_profile needs to be fully typed -- at least the parts of it used in step-store-address.
  3. 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!)

Copy link
Contributor

@noahtallen noahtallen Jan 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@allilevine allilevine Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@noahtallen noahtallen Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}} SiteSettingsItem
*/

Expand Down