Domains: checkout/whois form consolidation #19889

ramonjd · 2017-11-16T03:52:14Z

This PR addresses issue #18808

Goals

The core objective is to consolidate the domain checkout and whois contact details form, which share common fields and functionality.

Because of the varying internal business logic of each form, there's no way to unify them completely, however we can attempt to reduce some of the repetition in the contact form itself.

First steps

identify common functionality
create a <ContactDetailsFormFields /> common component
migrate formState controller into <ContactDetailsFormFields />
integration with domain details form
integrate with WHOIS contact details form

TODO

Update the button class attribute value for the wp-e2e-test.

Screenshots

Domain checkout form

WHOIS form

Afterthoughts

This was a worthy exercise, not only because it exposed the idiosyncratic logic and dependencies of both form, but also highlighted areas of complexity, not limited to the following:

Dealing with the specific logic of each journey, for example, disabling form fields and rendering a 'GApps' state, led to a component that only really shifts the work to another place.
form-state triggers a great deal of rendering and is a little wily

Furthermore, the effort required to consolidate the forms has been quite high, and might turn out to be disproportionate to the need for simplifying two contact forms.

As mentioned, the two forms perform separate tasks, though they share the same contact details requirements. Creating a common dependency could create more work should their internal logic diverge further later. Rather than instructing a swiss-army-like <ContactDetailsFormFields /> to render a form permutations only it knows about, we might come to the conclusion that it would be better to instruct a form-builder component to construct a form using fields controls we specify.

matticbot · 2017-11-16T03:52:15Z

Test live: https://calypso.live/?branch=try/checkout-whois-form-consolidation

a8ci18n · 2017-11-16T04:01:24Z

client/components/contact-details-form-fields/index.jsx

+			<div className="checkout__domain-details-contact-details-fields">
+				{ this.createField( 'organization', HiddenInput, {
+					label: translate( 'Organization' ),
+					text: translate( "+ Add your organization's name" ),


Hi! I've found a possible matching string that has already been translated 15 times:
translate( 'Add Organization Name' ) ES Score: 7

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-11-23T03:14:12Z

client/components/contact-details-form-fields/index.jsx

+			<div className="contact-details-form-fields__container">
+				{ this.createField( 'organization', HiddenInput, {
+					label: translate( 'Organization' ),
+					text: translate( "+ Add your organization's name" ),


Hi! I've found a possible matching string that has already been translated 15 times:
translate( 'Add Organization Name' ) ES Score: 7

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-11-28T03:40:28Z

client/components/contact-details-form-fields/index.jsx

+					HiddenInput,
+					{
+						label: translate( 'Organization' ),
+						text: labelTexts.organization || translate( '+ Add organization name' ),


Hi! I've found a possible matching string that has already been translated 15 times:
translate( 'Add Organization Name' ) ES Score: 17
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-11-29T05:01:26Z

client/components/domains/contact-details-form-fields/index.jsx

+					HiddenInput,
+					{
+						label: translate( 'Organization' ),
+						text: labelTexts.organization || translate( '+ Add organization name' ),


Hi! I've found a possible matching string that has already been translated 15 times:
translate( 'Add Organization Name' ) ES Score: 17
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

klimeryk

Pure awesomeness 👍
Very well done - these two forms have gathered a lot of cruft and it's great to a major cleanup like this 🙇

I've noticed just a few regressions, but given the scope of this PR (and the vast responsibilities of both forms) it was expected. I've tested a few scenarios: registering a new domain, a new domain that does not support privacy, just G Suite, editing WHOIS for an HRS domain, editing WHOIS for a .nl domain (mandatory fax field, weeeee), editing WHOIS for a WWD domain.

It seems it's not possible to update WHOIS info for a WWD domain. The reason is that fax is not passed to the backend, so it defaults to null when sent to WWD... and WWD fails with an error:

{
  "raw_response": null,
  "response": {
    "code": "INVALID_BODY",
    "message": "Request body doesn't fulfill schema, see details in `fields`",
    "fields": [
      {
        "message": "is not a string",
        "path": "contacts.contactRegistrant.fax",
        "code": "UNEXPECTED_TYPE"
      },
...

Either we should send empty fax or fix the backend.

Additionally, when saving WWD info if you change the email, a confirmation dialog is shown (to inform the user what will happen - that they need verify the change from both emails, etc.). However, when you hit Request Confirmation button, the main area of Calypso goes blank and there's this confusing error in the console:

I'm guessing something is undefined.

Apart from what I've mentioned in the comments, I've noticed that the check introduced in #16240 was undone by this - we should not allow the user to re-save the same WHOIS info. They often did this thinking that it will allow them to lift the 60-day lock :/

Also worth noting: the PR needs a rebase 😁

klimeryk · 2017-12-03T09:19:54Z

client/components/domains/contact-details-form-fields/README.md

+	};
+```
+
+### onContactDetailsChange {Func} (required)


You probably meant onSubmit :)

Hawkeye!
Now that I'm reviewing it, I've actually noticed several mistakes in there ;)
Thanks!

klimeryk · 2017-12-03T09:24:00Z

client/components/phone-input/index.jsx

@@ -243,7 +243,8 @@ class PhoneInput extends React.PureComponent {
 					name={ this.props.name }
 					ref={ this.setNumberInputRef }
 					type="tel"
-					className={ classnames( { 'is-error': this.props.isError } ) }
+					disabled={ this.props.disabled }
+					className={ this.props.isError && 'is-error' }


I'm guessing this line was added before #19940 😀

Edit: hah, it's a warning even:

Rebase fixed it! :)

klimeryk · 2017-12-03T09:27:27Z

client/components/domains/contact-details-form-fields/index.jsx

+export class ContactDetailsFormFields extends Component {
+	static propTypes = {
+		eventFormName: PropTypes.string,
+		contactDetails: PropTypes.shape( {


Great usage of shape! 👍
But the list of field names seems unnecessarily duplicated. Maybe it could be generated based on this.fieldNames with some map calls and the spread operator?

contactDetails: Object.assign( {}, ...this.fieldNames.map( ( field ) => ( { [field]: PropTypes.string } ) ) )

Similarly, defaultProps can be written as:

contactDetails: Object.assign( {}, ...this.fieldNames.map( ( field ) => ( { [field]: '' } ) ) )

Or is that too magical? 😁

I like less repetition :) It makes sense, since they are, and must be, equal.

We'd have to pull this.fieldNames out of the constructor. Making it another static var would work (and we could access it through ContactDetailsFormFields.fieldNames).

But now I reflect on it, this.fieldNames is a definitive and immutable list of form fields. It might work better as a constant. 👍

Ah, right, I did not think about the static/instance problem, but yup, you're right - it's a constant/immutable list anyway.

klimeryk · 2017-12-03T09:48:45Z

client/components/domains/contact-details-form-fields/index.jsx

+		translate: identity,
+	};
+
+	constructor() {


You should pass props through the constructor to super - otherwise this.props will be undefined. See https://reactjs.org/docs/react-component.html#constructor

Still no props? 😢

klimeryk · 2017-12-03T09:50:27Z

client/components/domains/contact-details-form-fields/index.jsx

+		} );
+	}
+
+	loadFormState = fn => {


Can you something more descriptive than fn? 🙏 Initially I thought it was something from lodash or something.

It's legacy from the previous form-state hydration. You're right, it's a little opaque. Will change.

Ah, should have noticed that, sorry about that. But, yeah, let's rename it to something more descriptive 🙇

klimeryk · 2017-12-03T10:02:14Z

client/components/phone-input/index.jsx

@@ -243,7 +243,8 @@ class PhoneInput extends React.PureComponent {
 					name={ this.props.name }
 					ref={ this.setNumberInputRef }
 					type="tel"
-					className={ classnames( { 'is-error': this.props.isError } ) }
+					disabled={ this.props.disabled }
+					className={ this.props.isError && 'is-error' }


It seems that once there's a validation error in the phone field, it stays in the is-error state 🤔

Definitely 🤔 ... Haven't been able to reproduce this yet. But I'm going to try!

klimeryk · 2017-12-03T10:10:09Z

client/my-sites/checkout/checkout/domain-details-form.jsx

@@ -344,117 +156,46 @@ export class DomainDetailsForm extends PureComponent {
 	renderPrivacySection() {
 		return (
 			<PrivacyProtection
-				checkPrivacyRadio={ this.allDomainItemsHavePrivacy() }


It seems the PrivacyProtection component was not updated after this rename and it's still relying on this.props.checkPrivacyRadio, which leads to this interesting state:

How can I reproduce this?

FYI, I ditched most of the props on this component since countriesList, disabled, fields, and onCheckboxChange weren't being used by <PrivacyProtection /> at all.

And this.handleCheckboxChange didn't even exist in the domain details component :)

IIRC, I did not do anything special - just try to register a new domain that supports privacy. And try to check a different radio button - both will be checked as a result.

klimeryk · 2017-12-03T10:10:47Z

client/my-sites/checkout/checkout/domain-details-form.jsx

-			cartItems.getDomainRegistrationsWithoutPrivacy( this.props.cart ).length === 0 &&
-			cartItems.getDomainTransfersWithoutPrivacy( this.props.cart ).length === 0
-		);
+	allDomainRegistrationsHavePrivacy() {


Why only domain registrations? Domain transfers are domains too! (or was this discussed with @aidvu?)

This is probably why it needs a rebase. :)

Aaaah, damn GitHub, confusing me! 🤜 🤛

Going to rebase as the first task 👍

ramonjd · 2017-12-04T06:54:46Z

@klimeryk Huge thanks for your review so far. Really helpful.

Apart from what I've mentioned in the comments, I've noticed that the check introduced in #16240 was undone by this - we should not allow the user to re-save the same WHOIS info.

I had this on my radar, and I thought it was working. I must have broken my fix. :)

Edit:

You did, perhaps without knowing it, find the solution to the above for me:

Either we should send empty fax or fix the backend.

I was deleting the fax (as we were doing in the original domain-details-form.jsx) and therefore the comparison check failed. 👍

ramonjd · 2017-12-05T02:07:59Z

However, when you hit Request Confirmation button, the main area of Calypso goes blank and there's this confusing error in the console

My mistake. I wasn't caching the form values. Also noticed the effect of #16239 Domains: After updating, the WHOIS store is not automatically updated, whereby the button isn't disabled. I might take a look at that later, but I think we just need to do another upgradesActions.fetchWhois to update the store after submit, or something... :)

klimeryk · 2017-12-05T16:19:47Z

I was deleting the fax (as we were doing in the original domain-details-form.jsx) and therefore the comparison check failed. 👍

That probably worked because we use Tucows for new domains, while this empty value seems to be required by WWD only 🤷‍♂️

deBhal · 2018-02-01T06:52:25Z

We're looking at addressing the conflict in #21781, and then we can circle back to this.

aidvu · 2018-02-01T15:32:50Z

Thanks for the update @deBhal.

ramonjd · 2018-02-07T06:33:05Z

We're looking at addressing the conflict in #21781, and then we can circle back to this.

Fixed in #22156. Full steam ahead 🚂 !!

ramonjd · 2018-02-08T01:54:25Z

Here are some bundle stats for this branch.

TL;DR This PR adds circa 20k to the gzipped domains chunk mainly due to phone number validation code.

Looks like it's adding about 20k to the gzipped payload in domains/components. From what I can tell, more than 3/4 of this comes down to <FormPhoneMediaInput /> and its dependency data.js, which contains the rules for validating phone numbers. Both comprise a fairly hefty slab of code that's only used in this form.

Previously we were only pulling this component in the checkout chunk, but with the consolidated form it's now in domains.

I experimented with loading the shared <ContactDetailsFormFields /> using <AsyncLoad />. This reduces the payload overall, only slightly, but creates a 25k chunk that has to be downloaded. Given that checkout is one of our most important flows, I'd argue we wouldn't want to risk keeping the user waiting for a sequential HTTP call.

Instead, I'm inclined to see if we can load data.js asynchronously, and refactor the phone input to handle the case when no validation is present. I guess this depends on how crucial phone validation is to WordPress / the domain registrar.

Overall, it's a slight step backwards in the quest to lower our bundle, but we might be able to claw back some of those bytes later. (?)

Review changes implemented some time ago

klimeryk

Amazing job, @ramonjd. Tested all the cases I could think of:

Registering a gTLD
Registering .ca
Registering .fr
Editing WHOIS for Tucows gTLD
Editing WHOIS for WWD gTLD
Editing WHOIS for .fr
Purchasing a domain by a brand-new user.
Purchasing G Suite for a mapped domain.
Purchasing a domain in the domain-only flow.

Everything looks solid - really stellar job on this, @ramonjd 💖
One small request: please merge this when there are many HEs around, just in case there's a regression, so that it will be quickly reported and handled appropriately 🙇

klimeryk · 2018-02-18T12:26:26Z

client/components/domains/contact-details-form-fields/index.jsx

+		translate: identity,
+	};
+
+	constructor() {


Still no props? 😢

ramonjd · 2018-02-20T09:49:22Z

Thanks @klimeryk !
Wouldn't have got this one even near the goal posts without you and everyone's reviews! I'm going to look at how to reduce the phone data.js when I get time.

Still no props?

Do you mean the props arg in the constructor? We don't need to since we're not accessing this.props in the constructor, so it's fine to leave out.

please merge this when there are many HEs around, just in case there's a regression, so that it will be quickly reported and handled appropriately

Will do. 🙇

klimeryk · 2018-02-22T08:25:57Z

Do you mean the props arg in the constructor? We don't need to since we're not accessing this.props in the constructor, so it's fine to leave out.

That might be true, but in the future it might change and someone will get unpleasantly surprised ;) You're calling the parent constructor either way, might as well pass the props to it ¯\_(ツ)_/¯

klimeryk · 2018-02-26T14:02:12Z

Please merge this, @ramonjd, before it grows any more merge conflicts 😅 🌵

Fixed focus events and state field refresh

Hooked in contact details cache to form card Focussing on address field when country select changed Removing useless props from Privacy Protection Added README.md and moved component under component/domains Passing props to super :) Not deleting the fax field in the state so that we pass an empty value to the backend Creating a constant out of this.fieldNames and deconstructing into proptypes Fixed some some strange reoccurring formatting in the tracksData object Updated disable button check from PR #20549

…n arrow functions etc

…acy selection radio boxes, because the first name field stole the focus using the autofocus attribute

alisterscott · 2018-02-26T23:33:40Z

Hi 👋

I noticed a couple of potential issues with this:

Firstly, at ~900px or narrower the state and postcode fields merge into one. Also, the "Continue to Checkout" label has been renamed to a less descriptive "Continue" - was this intentional? I didn't see this mentioned in the PR.

I'll raise the fields issue as a separate bug

Now

Previously

ramonjd · 2018-02-26T23:39:08Z

@alisterscott Thanks for testing! Appreciate it. And for catching the classname change in the e2e tests. Totally slipped past me. (Even though I'd made a note of it in the PR :( )

I'll look into those issues today.

ramonjd added [Feature] Checkout The checkout screen and process for purchases made on WordPress.com. [Feature Group] Emails & Domains Features related to email integrations and domain management. [Status] In Progress labels Nov 16, 2017

ramonjd force-pushed the try/checkout-whois-form-consolidation branch from 50bbc39 to 20145dd Compare November 16, 2017 03:56

ramonjd added the [Pri] Low label Nov 16, 2017

a8ci18n reviewed Nov 16, 2017

View reviewed changes

ramonjd force-pushed the try/checkout-whois-form-consolidation branch 2 times, most recently from 5183f6e to b93ea94 Compare November 23, 2017 03:09

a8ci18n reviewed Nov 23, 2017

View reviewed changes

ramonjd force-pushed the try/checkout-whois-form-consolidation branch from b93ea94 to 613f135 Compare November 28, 2017 03:35

a8ci18n reviewed Nov 28, 2017

View reviewed changes

ramonjd force-pushed the try/checkout-whois-form-consolidation branch from 613f135 to e0e8e6b Compare November 28, 2017 04:50

ramonjd added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 28, 2017

ramonjd force-pushed the try/checkout-whois-form-consolidation branch from 200b5b2 to 123a844 Compare November 29, 2017 04:54

a8ci18n reviewed Nov 29, 2017

View reviewed changes

ramonjd removed the [Status] In Progress label Nov 29, 2017

ramonjd changed the title ~~[WIP] Checkout: Domains checkout/whois form consolidation~~ Domains: checkout/whois form consolidation Nov 29, 2017

ramonjd requested a review from klimeryk November 29, 2017 06:59

ramonjd force-pushed the try/checkout-whois-form-consolidation branch from a0faa3c to 93c813c Compare November 30, 2017 05:33

klimeryk requested review from matthusby, aidvu, hambai and delputnam December 1, 2017 05:01

klimeryk previously requested changes Dec 3, 2017

View reviewed changes

ramonjd force-pushed the try/checkout-whois-form-consolidation branch from 93c813c to 4ced986 Compare December 5, 2017 02:14

ramonjd force-pushed the try/checkout-whois-form-consolidation branch from 4ced986 to f545c20 Compare December 7, 2017 07:18

ramonjd force-pushed the try/checkout-whois-form-consolidation branch from 30d97c2 to 42d9d95 Compare February 7, 2018 06:31

ramonjd force-pushed the try/checkout-whois-form-consolidation branch from 42d9d95 to d906e24 Compare February 15, 2018 01:04

klimeryk approved these changes Feb 18, 2018

View reviewed changes

client/components/domains/contact-details-form-fields/index.jsx

translate: identity,

};

constructor() {

Copy link

Contributor

klimeryk Feb 18, 2018

Choose a reason for hiding this comment

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

Still no props? 😢

klimeryk added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 18, 2018

ramonjd added 7 commits February 27, 2018 08:24

- auditing whois and checkout forms

be774c4

- using internal state for common component

b8d6d7b

Moved formstate over into contact details form fields

903cf8d

Fixed focus events and state field refresh

Clean up after rebasing with master, reordering of imports, returns o…

9d6aa75

…n arrow functions etc

updated support message import after rebase

7da6944

Removed autofocus pursuant to #21954: Screen readers skipped the priv…

cccafa3

…acy selection radio boxes, because the first name field stole the focus using the autofocus attribute

ramonjd force-pushed the try/checkout-whois-form-consolidation branch from d906e24 to cccafa3 Compare February 26, 2018 21:33

git update snapshot

e8b23b2

ramonjd merged commit 1cc89e2 into master Feb 26, 2018

ramonjd deleted the try/checkout-whois-form-consolidation branch February 26, 2018 22:25

matticbot removed the [Status] Ready to Merge label Feb 26, 2018

alisterscott mentioned this pull request Feb 26, 2018

Fix checkout selector Automattic/wp-e2e-tests#983

Merged

alisterscott mentioned this pull request Feb 26, 2018

Domains: Checkout form State and Post Code fields merge at narrow widths #22833

Closed

jsnajdr mentioned this pull request Oct 12, 2018

Update legacy api: string refs #27677

Closed

Domains: checkout/whois form consolidation #19889

Domains: checkout/whois form consolidation #19889

Conversation

ramonjd commented Nov 16, 2017 • edited Loading

Goals

First steps

TODO

Screenshots

Domain checkout form

WHOIS form

Afterthoughts

matticbot commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klimeryk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd commented Dec 4, 2017 • edited Loading

ramonjd commented Dec 5, 2017

klimeryk commented Dec 5, 2017

deBhal commented Feb 1, 2018

aidvu commented Feb 1, 2018

ramonjd commented Feb 7, 2018 • edited Loading

ramonjd commented Feb 8, 2018

klimeryk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd commented Feb 20, 2018

klimeryk commented Feb 22, 2018

klimeryk commented Feb 26, 2018

alisterscott commented Feb 26, 2018 • edited Loading

ramonjd commented Feb 26, 2018 • edited Loading

ramonjd commented Nov 16, 2017 •

edited

Loading

ramonjd commented Dec 4, 2017 •

edited

Loading

ramonjd commented Feb 7, 2018 •

edited

Loading

alisterscott commented Feb 26, 2018 •

edited

Loading

ramonjd commented Feb 26, 2018 •

edited

Loading