1098251 - New Contact view insists on an email address being entered even if I only want to enter a Firefox OS phone number

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Maire Reavy [:mreavy]]

Bug 1081322 added the ability to enter a Firefox OS phone number.  When I tried to enter a contact with just a Firefox OS phone, I could not save the entry unless I included an email address as well.  

Since FxA allows for email addresses or Firefox OS phone numbers, having either one (email address or FxOS phone number) present should be sufficient for a user to save the entry.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

The plan here is to visually indicate that both fields are required if the user tries to submit the form with only the Name field filled in.

However, we will accept the field with the name and either the phone or email address present.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This actually turned out to be pretty simple since we were already using linkedState.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8522503 [details] [diff] [review]
Patch

Review of attachment 8522503 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/content/js/contacts.jsx
@@ +575,5 @@
>        switch (this.props.mode) {
>          case "edit":
>            this.state.contact.name[0] = this.state.name.trim();
>            setPreferred(this.state.contact,
> +                       {field: "email", value: this.state.email.trim(), optional: true});

Both calls to setPreferred now use the optional:true argument. We could update setPreferred to always treat it as optional, but I'm on the fence. What do you think?

@@ +633,5 @@
>            <input ref="name" required pattern="\s*\S.*" type="text"
>                   className={cx({pristine: this.state.pristine})}
>                   valueLink={this.linkState("name")} />
>            <label>{mozL10n.get("edit_contact_email_label")}</label>
> +          <input ref="email" type="email" required={phoneOrEmailRequired}

In case you're curious, if required=false, React won't create the "required" attribute.

Once a value appears in this field the "required" attribute is removed, but you can't submit a type="email" field with a non-email value, so it's OK that the field is not marked as "required" once a value is present.

linkState operates on the onChange event, so it is practically updated after each keystroke, meaning we don't have to worry about the requiredness-state of a field being out of date.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8522503 [details] [diff] [review]
Patch

Review of attachment 8522503 [details] [diff] [review]:
-----------------------------------------------------------------

Tests to make sure we handle the 4 cases (all combinations of phone and email fields {empty, valid}) would be good. We might already have tests for some of the 4.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch with tests failing for unknown reasons

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch with tests failing for unknown reasons

Carrying forward r+ from MattN.

NiKo`, do you know why the test is failing? It fails at line 99 and also at line 113.

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8522597 [details] [diff] [review]
Patch with tests failing for unknown reasons

Review of attachment 8522597 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +91,5 @@
> +            loop.contacts.ContactDetailsForm({mode: "add"}));
> +          var nameInput = view.getDOMNode().querySelector("input[type='text']");
> +          nameInput.value = "Jenny";
> +          var telInput = view.getDOMNode().querySelector("input[type='tel']");
> +          telInput.value = "867-5309";

This is not the way you're supposed to update a field value; you should try using the TestUtils.Simulate event helpers: http://facebook.github.io/react/docs/test-utils.html#simulate

Try something like this instead:

TestUtils.Simulate.change(telInput, {target: {value: "867-5309"}});

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Clearing needinfo.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch with tests

Thanks Niko, that was exactly my problem. I had been trying to call the Simulate functionality but wasn't passing the new value to the Simulate function. With that change it all works as expected. I'll see if I can file a bug with Chai to get their documentation made clearer.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

s/Chai/React/

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for checkin

Landed as https://hg.mozilla.org/integration/fx-team/rev/91b67b9a26bb

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91b67b9a26bb

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8f242cbe2a2