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

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: support importing contacts with phone numbers in a different format

See summary.

Comment 1

[Marco Mucci [:MarcoM]]

Added to IT 36.1

Comment 2

[Adam Roach [:abr]]

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."

Comment 3

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6fdb1b14a8fd

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: support importing contacts with phone numbers in a different format

Patch with comments addressed. Carrying over r=abr.

Comment 5

[Mike de Boer [:mikedeboer]]

Showing a green try, pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/3b8d0c8b28a5

Comment 6

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:
Fixes a regression in bug 1081130

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3b8d0c8b28a5

Comment 8

[Mike de Boer [:mikedeboer]]

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.

Comment 9

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

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 10

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 11

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/c278b3312388

Comment 12

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

Tested and good on Aurora nightly build; windows and linux.

Comment 13

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

https://hg.mozilla.org/releases/mozilla-beta/rev/8c42ccaf8aa1

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 15

[Paul Silaghi, QA [:pauly]]

(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

Comment 16

[u279076]

Paul, can you please verify this in the next Beta?

Comment 17

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b2 Win 7, OS X 10.9.5