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)
Hello (Loop)
Client
Tracking
(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)
1.37 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
25.95 KB,
image/png
|
Details | |
22.26 KB,
patch
|
dmosedale
:
review+
|
Details | Diff | Splinter Review |
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+
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Iteration: --- → 35.3
Comment 2•9 years ago
|
||
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+
Comment 3•9 years ago
|
||
Matt's right. We should change the p to a capital "P" in phone.
Comment 4•9 years ago
|
||
'p' -> 'P' and carry forward r+
Attachment #8503450 -
Attachment is obsolete: true
Attachment #8503478 -
Flags: review+
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4c6a3e307c60
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•9 years ago
|
Reporter | ||
Updated•9 years ago
|
Status: REOPENED → ASSIGNED
Updated•9 years ago
|
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]
Comment 7•9 years ago
|
||
Darrin - can we get a mock up of adding one more row under email address?
backlog: --- → Fx35+
Flags: needinfo?(dhenein)
Comment 9•9 years ago
|
||
The label is "Firefox OS Phone" btw and I guess it won't show imported phone numbers?
Reporter | ||
Comment 10•9 years ago
|
||
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
Updated•9 years ago
|
Target Milestone: mozilla35 → ---
Reporter | ||
Comment 11•9 years ago
|
||
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.
Reporter | ||
Updated•9 years ago
|
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Iteration: 36.1 → ---
Flags: needinfo?(mmucci)
Assignee | ||
Comment 13•9 years ago
|
||
(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?
Assignee | ||
Updated•9 years ago
|
Points: 2 → 3
Comment 14•9 years ago
|
||
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)
Comment 15•9 years ago
|
||
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•9 years ago
|
||
Gaia also has libraries for phone number validation written in JS IIRC
Comment 17•9 years ago
|
||
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.
Updated•9 years ago
|
Points: 3 → 5
Comment 18•9 years ago
|
||
(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
Updated•9 years ago
|
User Story: (updated)
Updated•9 years ago
|
backlog: Fx35? → Fx35+
Comment 19•9 years ago
|
||
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>.
Updated•9 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 36.2
Comment 20•9 years ago
|
||
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)
Assignee | ||
Comment 21•9 years ago
|
||
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)
Reporter | ||
Comment 22•9 years ago
|
||
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-
Assignee | ||
Comment 23•9 years ago
|
||
Attachment #8517229 -
Attachment is obsolete: true
Attachment #8518169 -
Flags: review?(mdeboer)
Comment 24•9 years ago
|
||
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.
Assignee | ||
Comment 25•9 years ago
|
||
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)
Comment 28•9 years ago
|
||
Attachment #8519095 -
Attachment is obsolete: true
Attachment #8519095 -
Flags: review?(mdeboer)
Assignee | ||
Comment 29•9 years ago
|
||
Finished out the rest of the tests.
Attachment #8520104 -
Attachment is obsolete: true
Attachment #8520157 -
Flags: review?(dmose)
Updated•9 years ago
|
Iteration: 36.2 → 36.3
Comment 30•9 years ago
|
||
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+
Assignee | ||
Comment 31•9 years ago
|
||
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]
Comment 32•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9c862ea1669f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Whiteboard: [good first verify][fixed-in-fx-team] → [good first verify]
Target Milestone: --- → mozilla36
Comment 33•9 years ago
|
||
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)
Comment 34•9 years ago
|
||
(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 37•9 years ago
|
||
I verified this on Aurora
You need to log in
before you can comment on or make changes to this bug.
Description
•