Closed Bug 1084384 Opened 10 years ago Closed 10 years ago

Google import fails if a contact contains a phone number in a non-standard format

Categories

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

defect
Points:
1

Tracking

(firefox33 unaffected, firefox34+ verified, firefox35+ verified, firefox36+ verified)

VERIFIED FIXED
mozilla36
Iteration:
36.1
Tracking Status
firefox33 --- unaffected
firefox34 + verified
firefox35 + verified
firefox36 + verified
backlog Fx34?

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

Attachments

(1 file, 1 obsolete file)

See summary.
Attachment #8506903 - Flags: review?(adam)
Flags: qe-verify+
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Added to IT 36.1
Flags: needinfo?(mmucci)
Comment on attachment 8506903 [details] [diff] [review]
Patch v1: support importing contacts with phone numbers in a different format

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

Looks good to me.

Suggestion (r+ not predicated on changing this): "Object.keys(mockDb._store).length" and "mockDb._next_guid" are pretty cumbersome, and the second one is confusing to read. I suggest adding something like a "get size()" method to the mock DB, and using it in the tests.

::: browser/components/loop/test/mochitest/browser_GoogleImporter.js
@@ +30,3 @@
>  
>    yield promiseImport();
> +  Assert.equal(Object.keys(mockDb._store).length, kContactsCount, "Database should contain only five contact after reimport");

Fix the message here -- it's hardcoded to say "five". Suggest: "Database should be the same size after reimport."
Attachment #8506903 - Flags: review?(adam) → review+
Patch with comments addressed. Carrying over r=abr.
Attachment #8506903 - Attachment is obsolete: true
Attachment #8506991 - Flags: review+
Showing a green try, pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/3b8d0c8b28a5
backlog: --- → Fx34?
[Tracking Requested - why for this release]:
Fixes a regression in bug 1081130
https://hg.mozilla.org/mozilla-central/rev/3b8d0c8b28a5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8506991 [details] [diff] [review]
Patch v1.1: support importing contacts with phone numbers in a different format

Approval Request Comment
[Feature/regressing bug #]: Loop, bug 1081130.
[User impact if declined]: A couple of users trying out the Google Import feature reported failures when importing contacts from their account. This is most likely due to some of their contacts' phone numbers not being in a format we expected before. With this patch applied we do and import will succeed.
[Describe test coverage new/current, TBPL]: Landed in m-c. Added regression tests in the patch.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.
Attachment #8506991 - Flags: approval-mozilla-beta?
Attachment #8506991 - Flags: approval-mozilla-aurora?
As best I can tell on Nightly 10/18, Windows and Linux (modulo I can't easily see what the underlying database entry I'm importing is).  Note that my contacts failed to import with the bug 1081130 patch alone, so almost certainly my contact list hits this bug.

Note that the "non-standard format" is a raw number (in the test, 2151234567890 instead of uri="tel:+<countrycode><number>"); i.e. uri="tel:<...>" vs just a number.

As per above, this also has a test.
Comment on attachment 8506991 [details] [diff] [review]
Patch v1.1: support importing contacts with phone numbers in a different format

Thanks for testing jesup. Approved for Aurora. If everything goes well with your testing on Aurora on Sunday, we'll get this uplifted for beta2.
Attachment #8506991 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tested and good on Aurora nightly build; windows and linux.
Comment on attachment 8506991 [details] [diff] [review]
Patch v1.1: support importing contacts with phone numbers in a different format

Previously approved offline. Adding approval to the bug.
Attachment #8506991 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Randell Jesup [:jesup] from comment #12)
> Tested and good on Aurora nightly build; windows and linux.
Verified fixed 36.0a1 (2014-10-20) Win 7
Status: RESOLVED → VERIFIED
Paul, can you please verify this in the next Beta?
Flags: needinfo?(paul.silaghi)
QA Contact: anthony.s.hughes → paul.silaghi
Verified fixed FF 34b2 Win 7, OS X 10.9.5
Flags: needinfo?(paul.silaghi)
You need to log in before you can comment on or make changes to this bug.