Domains/cctlds: add organization field to fr-form #20043

deBhal · 2017-11-21T07:08:27Z

This PR adds an organization field to the ccTLD forms to address a problem with validating the organization field where the organization field on the first form is required only if the registrantType is set to organization on the second form (T553-code).

By adding the organization field to the tld forms, we can give the user the opportunity to fix the problem.

There are several things we want to check:

The new field is hooked up to the state correctly

Any value from the initial form is carried over
Changing the form updates the state (visible in redux devtools at State > domains > management > items > _contactDetailsCache )
The changed value is sent to the back end ( visible in the payload of the api call to https://public-api.wordpress.com/rest/v1.1/me/transactions )

Validation

The user should not be able to submit the form with an empty field
Validation errors should show if the organization field is empty while the registrantType is set to organization
Non-alphanumeric characters are not permitted in the Organization field
Match other validation that happens on the back end (length <= 100)

Tests should run and pass
~~Punted: .ca~~

Discussion:
Both the length and characters could be enforced by a sanitizing function, but I pulled the character sanitizing out and replaced it with the validation message because it's easier to understand. I'm particularly worried about this in the context of international input. A user that's trying to type Asian characters will get an empty field with no explanation.

I've punted .ca because there are quite involved rules around it implemented on the back end, so I think I'm going to have to take a different approach so I thought I'd push that out into a different PR.

matticbot · 2017-11-21T07:08:28Z

Test live: https://calypso.live/?branch=update/cctlds/add-organization-to-tld-forms

a8ci18n · 2017-11-21T07:13:10Z

client/components/domains/registrant-extra-info/fr-form.jsx

@@ -190,6 +198,24 @@ class RegistrantExtraInfoFrForm extends React.PureComponent {

 		return (
 			<div>
+				<FormFieldset>
+					<FormLabel className="registrant-extra-info__optional" htmlFor="organization">
+						{ translate( 'Organization Name' ) }


Hi! I've found a possible matching string that has already been translated 15 times:
translate( 'Add Organization Name' ) ES Score: 15
See 2 additional suggestions 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).

👎 The "Add" makes a pretty big difference to the meaning in this case

a8ci18n · 2017-11-22T04:57:32Z

client/components/domains/registrant-extra-info/fr-form.jsx

 		return (
 			<div>
+				<FormFieldset>
+					<FormLabel className="registrant-extra-info" htmlFor="organization">
+						{ translate( 'Organization Name' ) }


Hi! I've found a possible matching string that has already been translated 15 times:
translate( 'Add Organization Name' ) ES Score: 15
See 2 additional suggestions 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-22T16:17:35Z

client/components/domains/registrant-extra-info/fr-form.jsx

 		return (
 			<div>
+				<FormFieldset>
+					<FormLabel className="registrant-extra-info__organization" htmlFor="organization">
+						{ translate( 'Organization Name' ) }


Hi! I've found a possible matching string that has already been translated 15 times:
translate( 'Add Organization Name' ) ES Score: 15
See 2 additional suggestions 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).

mhsdef · 2017-11-22T16:22:02Z

Tested, works beautifully. I did get a warning (see 1c536-pb) which I think we should fix if we can.

mhsdef

Looks good! I had a question on naming, the hack, and whether we can quash that warning reasonably or not [eg, some larger module issue?]. Almost RTG.

mhsdef · 2017-11-22T17:30:45Z

client/components/domains/registrant-extra-info/fr-schema.json

@@ -9,70 +10,98 @@
 					"maxLength": 0
 				}
 			]
+		},
+		"organizationRequiredForIndividualTypeOrganization": {


"IndividualTypeOrganization", not quite sure I understand the naming intention there. Shouldn't it be something like organizationRequiredForRegistrantTypeOrganization?

mhsdef · 2017-11-22T17:34:17Z

client/components/domains/registrant-extra-info/fr-validate-contact-details.js

+			// path.
+			// We've only got one such case at the moment, so we can insert this
+			// hack, but if we need to tell multiple such rules apart, we're
+			// going to need to add a some magic to map schemas to fields


How hard would said map be? I don't understand this part of the code well but hack sense is tingling and if the investment is straightward and cheap enough (say, half a day?) my vote would be worthwhile.

If it'd be a big deal, then, ignore this.

Mini essay in comments, short version: let's not in this PR.

deBhal · 2017-11-22T23:48:13Z

I had a question on naming

Good catch! I think the self-documentation is worth a few extra characters, so decided to go big with organizationIsRequiredWhenRegistrantTypeIsOrganization.

whether we can quash that warning reasonably or not [eg, some larger module issue?].

I've got a fix for that warning, but it's not related to this PR so I'll open a fresh one: #20139

the hack
Yeah, "hack sense is tingling" is spot on.

The map itself would easy, but it would just be another hack. Json schema can totally do this, so we'd be starting down the slippery slope of writing our own schema validator, and closing ourselves off from the spec.

The root of the problem is that we don't get good information about the failing rule. If we could grab the rule that failed, we can add any meta data that we want.

For example, in this case we want to be able to specify that this rule describes a problem with the organization field, so we could use a convention where a field attribute overrides the path if it's present and add field: 'organization' to the rule in the schema to say "when this rule fails, it indicates a problem with the organization field. We could also add an errorCode, or even errorMessage directly, and maybe also the variable extraction that @klimeryk was musing about to feed variables into error messages (e.g. naming the specific banned word in the error message). In short: whatever we need.

is-my-json-valid is excellent in terms of getting pass/fail right, it's just a bit terse. There are several issues related to it that have been open for several years (mafintosh/is-my-json-valid#38, mafintosh/is-my-json-valid#39, mafintosh/is-my-json-valid#22).

Trying to fix that looks pretty fun but not very easy, so at the moment I'm leaning towards trying a different lib (ajv in particular looks very promising), and that requires discussion and investigation that I'd prefer to do after landing this bugfix.

mhsdef · 2017-11-23T06:55:12Z

LGTM. Test an org .fr purchase all the way through (if you haven't already) and ship it!

Linters be complainin’

@hewsut

props @hewsut

a8ci18n reviewed Nov 21, 2017

View reviewed changes

designsimply added [Feature Group] Emails & Domains Features related to email integrations and domain management. [Status] In Progress labels Nov 21, 2017

a8ci18n reviewed Nov 22, 2017

View reviewed changes

deBhal changed the title ~~[WIP] Domains/cctlds: add organization to fr forms~~ Domains/cctlds: add organization field to fr-form Nov 22, 2017

deBhal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Nov 22, 2017

deBhal requested a review from mhsdef November 22, 2017 05:44

deBhal self-assigned this Nov 22, 2017

a8ci18n reviewed Nov 22, 2017

View reviewed changes

mhsdef added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 22, 2017

mhsdef requested changes Nov 22, 2017

View reviewed changes

deBhal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Nov 23, 2017

mhsdef approved these changes Nov 23, 2017

View reviewed changes

Julian de Bhal and others added 9 commits November 24, 2017 07:24

Add Organization Name to fr-form

aaf8dbf

Add required organization tests (red)

fed6c77

Validate organization for registrantType:'organization'

0a870b1

Add schema version to fr-schema

2b2d9f6

Validate organization characters and string length

8fcf546

Use appropriate CSS prefix on organization field

fcc4d4a

Linters be complainin’

Validate organization characters and string length

4ccb11d

refactor update event handling

b3ff233

Improve schema reference name

ce7fe11

props @hewsut

deBhal force-pushed the update/cctlds/add-organization-to-tld-forms branch from d2b3cfb to ce7fe11 Compare November 23, 2017 21:43

deBhal merged commit 8d708e4 into master Nov 23, 2017

deBhal deleted the update/cctlds/add-organization-to-tld-forms branch November 23, 2017 23:46

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

deBhal mentioned this pull request Nov 27, 2017

Domains: .fr organization field can be left blank #17666

Closed

deBhal mentioned this pull request Dec 20, 2017

Domains/ccTLDs: Add organization field to ca-form #20984

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Domains/cctlds: add organization field to fr-form #20043

Domains/cctlds: add organization field to fr-form #20043

deBhal commented Nov 21, 2017 •

edited

matticbot commented Nov 21, 2017

a8ci18n Nov 21, 2017

deBhal Nov 21, 2017

a8ci18n Nov 22, 2017

mhsdef Nov 22, 2017

a8ci18n Nov 22, 2017

mhsdef commented Nov 22, 2017

mhsdef left a comment

mhsdef Nov 22, 2017

mhsdef Nov 22, 2017

deBhal Nov 22, 2017

deBhal commented Nov 22, 2017 •

edited

mhsdef commented Nov 23, 2017

Domains/cctlds: add organization field to fr-form #20043

Domains/cctlds: add organization field to fr-form #20043

Conversation

deBhal commented Nov 21, 2017 • edited

matticbot commented Nov 21, 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

Choose a reason for hiding this comment

mhsdef commented Nov 22, 2017

mhsdef 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

deBhal commented Nov 22, 2017 • edited

mhsdef commented Nov 23, 2017

deBhal commented Nov 21, 2017 •

edited

deBhal commented Nov 22, 2017 •

edited