1081322 - Add a phone number field to the New/ Edit Contact view

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mike de Boer [:mikedeboer]]

The work for this bug consists of two parts:

1) Land the string that says 'Firefox OS phone'
2) Add a field below 'Email' to enter a phone number of a new or existing contact

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: add string for new contacts phone number editing

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8503450 [details] [diff] [review]
Patch 1: add string for new contacts phone number editing

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

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +96,5 @@
>  ## See https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#contacts
>  ## and click the 'New Contact' button to see the fields.
>  new_contact_name_placeholder=Name
>  new_contact_email_placeholder=Email
> +new_contact_phone_placeholder=Firefox OS phone

It looks like we're using title case so I think "phone" should be capitalized but Matej/Darrin would know for sure.

Comment 3

[Maire Reavy [:mreavy]]

Matt's right.  We should change the p to a capital "P" in phone.

Comment 4

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

Attachment: Patch 1: add string for new contacts phone number editing

'p' -> 'P' and carry forward r+

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c6a3e307c60

Comment 6

[u279076]

I don't think this needs explicit QE verification, but would be a good first bug for a new QE contributor.

Comment 7

[:shell escalante]

Darrin - can we get a mock up of adding one more row under email address?

Comment 8

[Darrin Henein [:darrin]]

Attachment: Phone Number Field

Is this enough Shell?

Comment 9

[Matthew N. [:MattN]]

The label is "Firefox OS Phone" btw and I guess it won't show imported phone numbers?

Comment 10

[Mike de Boer [:mikedeboer]]

Additionally, I think we'll want to do some kind of enforcement/ validation of input to make sure we only get phone number in our address book in a specific format. This is how Google does it: http://note.io/1vRgppF

Comment 11

[Mike de Boer [:mikedeboer]]

First iteration would be to implement the phone number field as Darrin mocked up in attachment 8505608 [details] and add input validation to check if the phone number is in the right format before the form is submitted.

Comment 12

[Marco Mucci [:MarcoM]]

Removed from IT 36.1

Comment 13

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

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> First iteration would be to implement the phone number field as Darrin
> mocked up in attachment 8505608 [details] and add input validation to check
> if the phone number is in the right format before the form is submitted.

How is this going to be validated? Phone numbers can have many different forms (7 digits, 10 digits, 5 digits, alpha-numeric, and written in many different conventions). Also, we'll have to deal with Google-imported phone numbers, do we just skip the phone number if it doesn't validate or do we skip validation with automatic-importing?

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Adding extra validation along the lines of what Google does would appear to be a substantial amount of work and additionally would need it's own UX and graphic design.  Expanding this bug beyond the originally designed scope seems guaranteed to make it slip.  Let's spin that off into another bug.  Adding needinfo? RT so that he can frame any spinoff appropriately.

Do we know that the FxOS team actually considers this a priority?  jaws is under the impression that they haven't asked for it, and if that's true, it seems a little odd to block on this bug at all.  Resetting flag to blocking-35? in the hopes of finding more clarity.

Comment 15

[Maire Reavy [:mreavy]]

To add some clarity: without this bug fixed (ability to add a contact based on his/her FxOS phone number), there is no way for a user to call a FxOS phone directly without adding the FxOS phone to his/her Google contacts and importing all his/her contacts. If we're supporting FxOS-Desktop interop, this seems an important user story to support.

I think the amount of phone number validation we do is up for debate and can spread across more than one bug.

Can we ask the Loop app developers what they do in the Loop app in terms of validation?  Perhaps we could even leverage some of that code.

Comment 16

[Matthew N. [:MattN]]

Gaia also has libraries for phone number validation written in JS IIRC

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

If we can reuse libs for phone validation in this bug, that would be seem to be appropriate; but adding substantial UI still should be spun out, I would argue.

Comment 18

[Adam Roach [:abr]]

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #14)
> Do we know that the FxOS team actually considers this a priority?  jaws is
> under the impression that they haven't asked for it, and if that's true, it
> seems a little odd to block on this bug at all.  Resetting flag to
> blocking-35? in the hopes of finding more clarity.

I've added this to the Loop wiki, to help with guidance on the topic whenever there hasn't been any obviously related discussion: https://wiki.mozilla.org/Loop/Reminder

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Validator code appears to be available at <http://mxr.mozilla.org/mozilla-central/source/dom/phonenumberutils/PhoneNumber.jsm> and/or <https://github.com/andreasgal/PhoneNumber.js>.

Comment 20

[Romain Testard [:RT]]

Clearing my NI - for clarity this one is in as it seems relatively low effort to provide us much better FxOS interoperability.

Comment 21

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

Attachment: Patch

I didn't find any tests that tested creating or editing a contact through the Loop panel.

Also, is it a bug that at line 512 of contacts.jsx we are not using getPreferredEmail?

Comment 22

[Mike de Boer [:mikedeboer]]

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

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

I'll be around this evening to r+ it, as the change I request is pretty straightforward ;)

Thanks for working on this!

::: browser/components/loop/content/js/contacts.jsx
@@ +40,5 @@
>      }
>      return contact.email.find(e => e.pref) || contact.email[0];
>    };
>  
> +  let getPreferredTel = function(contact) {

Can you make this a method like `getPreferred = function(contact, field = "email") {}`?

You can then use that throughout this file and introduce it in line 512 as well.

Comment 23

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

Attachment: Patch v2

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

I would expect/hope that it shouldn't be too hard to write a unit test for the ContactDetailsForm that tests only this added functionality without having to entrain testing everything else in the form.  Unless this turns out to be substantially harder than I'm imagining, I don't believe this code should land without that.

If you wish, I'm open to pairing on writing that code.

Comment 25

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

Attachment: Patch v2.1

Addressed review comments that were given over IRC, http://logs.glob.uno/?c=mozilla%23loop&s=6+Nov+2014&e=6+Nov+2014#c16445

Comment 26

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

Dan, can you pair with me on that test?

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Sure.  Let's talk on Monday morning to schedule.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Add a phone number field to the New/ Edit Contact view.

Comment 29

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

Attachment: Patch

Finished out the rest of the tests.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

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

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

r=dmose with the comments addressed.  It'd be nice to be able to look over the unit tests for the set/getPreferred stuff, but that's not truly required.

::: browser/components/loop/content/js/contacts.jsx
@@ +33,5 @@
>      };
>    };
>  
> +  // Used to retrieve the preferred email or phone number
> +  // for the contact. Both fields are optional.

jsdoc comments here would be nice.

@@ +45,5 @@
> +  let setPreferred = function(contact, data) {
> +    if (data.optional) {
> +      // Don't clear the field if it doesn't exist.
> +      if (!data.value &&
> +          (!contact[data.field] || contact[data.field].length == 0)) {

Using ! on the .length value would improve readability.

@@ +54,5 @@
> +    if (contact[data.field].length == 0) {
> +      contact[data.field][0] = {"value": data.value};
> +      return;
> +    }
> +    for (let i in contact[data.field]) {

Adding a comment here would be nice.

@@ +60,5 @@
> +        contact[data.field][i].value = data.value;
> +        return;
> +      }
> +    }
> +    contact[data.field][0].value = data.value;

Please export these using _ prefixes to indicate private functions and write unit tests for them since future uncaught errors could mean dataloss.

@@ +189,1 @@
>          nextState.showMenu !== this.state.showMenu

Bonus points if you feel like making this more readable, but not required for r+.

@@ +613,5 @@
>            <input ref="email" required type="email"
>                   className={cx({pristine: this.state.pristine})}
>                   valueLink={this.linkState("email")} />
> +          <label>{mozL10n.get("new_contact_phone_placeholder")}</label>
> +          <input ref="tel" type="tel"

Can you add an XXX somewhere around here asking if we could just remove these refs?

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +15,5 @@
> +    navigator.mozLoop = {
> +      getStrings: function(entityName) {
> +        var textContentValue = "fakeText";
> +        if (entityName == "add_contact_button") {
> +          textContentValue = "Add Contact";

If you could change this to something like "Fake Add Contact Label" or something similar so that reading it in context is clearer.

Comment 31

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

https://hg.mozilla.org/integration/fx-team/rev/9c862ea1669f

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c862ea1669f

Comment 33

[Maire Reavy [:mreavy]]

Hi Jared -- I just entered bug 1098251.  It looks like the current code requires that a contact always have an email address.  I can't enter a contact if I only have a Firefox OS phone number.

Comment 34

[Maire Reavy [:mreavy]]

(I hit reply too soon.)  Jared -- Would you like me to find someone else to fix bug 1098251, or do you want to take it?  Thanks.

Comment 35

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

Yeah, I'll take bug 1098251.

Comment 36

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e035844cd25

Comment 37

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

I verified this on Aurora