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

RESOLVED FIXED in Firefox 35

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mreavy, Assigned: jaws)

Tracking

unspecified
mozilla36
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Blocks: 1081322
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Points: --- → 2
Flags: qe-verify?
Flags: firefox-backlog+
Posted patch Patch (obsolete) — Splinter Review
This actually turned out to be pretty simple since we were already using linkedState.
Attachment #8522503 - Flags: review?(MattN+bmo)
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 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.
Attachment #8522503 - Flags: review?(MattN+bmo) → review+
Carrying forward r+ from MattN.

NiKo`, do you know why the test is failing? It fails at line 99 and also at line 113.
Flags: needinfo?(nperriault)
Attachment #8522597 - Flags: review+
Attachment #8522573 - Attachment is obsolete: true
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"}});
Clearing needinfo.
Flags: needinfo?(nperriault)
Flags: qe-verify?
Flags: qe-verify-
Flags: in-testsuite+
Posted patch Patch with tests (obsolete) — Splinter Review
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.
Attachment #8522597 - Attachment is obsolete: true
Attachment #8523012 - Flags: review+
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/91b67b9a26bb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.