Closed Bug 1081322 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Points:
5

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
backlog Fx35+

People

(Reporter: mikedeboer, Assigned: jaws)

References

Details

User Story

As a user, I should be able to enter a Firefox OS phone number for a contact and have it locally saved.

Tech Checklist
==
* add simple unit-test setup for contact detail form

Then, test & implement

* add field
* validity check
* saving field

Attachments

(3 files, 5 obsolete files)

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
Flags: qe-verify+
Flags: firefox-backlog+
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Attachment #8503450 - Flags: review?(jaws)
Iteration: --- → 35.3
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.
Attachment #8503450 - Flags: review?(jaws) → review+
Matt's right.  We should change the p to a capital "P" in phone.
'p' -> 'P' and carry forward r+
Attachment #8503450 - Attachment is obsolete: true
Attachment #8503478 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4c6a3e307c60
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Iteration: 35.3 → 36.1
I don't think this needs explicit QE verification, but would be a good first bug for a new QE contributor.
Flags: qe-verify+ → qe-verify-
Whiteboard: [good first verify]
Darrin - can we get a mock up of adding one more row under email address?
backlog: --- → Fx35+
Flags: needinfo?(dhenein)
Attached image Phone Number Field
Is this enough Shell?
Flags: needinfo?(dhenein)
The label is "Firefox OS Phone" btw and I guess it won't show imported phone numbers?
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
Target Milestone: mozilla35 → ---
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.
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Iteration: 36.1 → ---
Flags: needinfo?(mmucci)
Removed from IT 36.1
Flags: needinfo?(mmucci)
(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?
Points: 2 → 3
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.
backlog: Fx35+ → Fx35?
Flags: needinfo?(rtestard)
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.
Gaia also has libraries for phone number validation written in JS IIRC
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.
Points: 3 → 5
(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
User Story: (updated)
backlog: Fx35? → Fx35+
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 36.2
Clearing my NI - for clarity this one is in as it seems relatively low effort to provide us much better FxOS interoperability.
Flags: needinfo?(rtestard)
Attached patch Patch (obsolete) — Splinter Review
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?
Attachment #8517229 - Flags: review?(mdeboer)
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.
Attachment #8517229 - Flags: review?(mdeboer) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8517229 - Attachment is obsolete: true
Attachment #8518169 - Flags: review?(mdeboer)
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.
Attached patch Patch v2.1 (obsolete) — Splinter Review
Addressed review comments that were given over IRC, http://logs.glob.uno/?c=mozilla%23loop&s=6+Nov+2014&e=6+Nov+2014#c16445
Attachment #8518169 - Attachment is obsolete: true
Attachment #8518169 - Flags: review?(mdeboer)
Attachment #8519095 - Flags: review?(mdeboer)
Dan, can you pair with me on that test?
Flags: needinfo?(dmose)
Sure.  Let's talk on Monday morning to schedule.
Flags: needinfo?(dmose)
Attachment #8519095 - Attachment is obsolete: true
Attachment #8519095 - Flags: review?(mdeboer)
Attached patch PatchSplinter Review
Finished out the rest of the tests.
Attachment #8520104 - Attachment is obsolete: true
Attachment #8520157 - Flags: review?(dmose)
Iteration: 36.2 → 36.3
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.
Attachment #8520157 - Flags: review?(dmose) → review+
https://hg.mozilla.org/integration/fx-team/rev/9c862ea1669f
Flags: in-testsuite+
Keywords: leave-open
Whiteboard: [good first verify] → [good first verify][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9c862ea1669f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [good first verify][fixed-in-fx-team] → [good first verify]
Target Milestone: --- → mozilla36
Depends on: 1098251
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.
Flags: needinfo?(jaws)
(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.
Yeah, I'll take bug 1098251.
Flags: needinfo?(jaws)
I verified this on Aurora
Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.