1089011 - Firefox Hello "Import Contacts" imports a bunch of people that I've emailed / received email from, but who aren't in my contacts list

Status: VERIFIED FIXED

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

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Sign into Firefox Hello (using the toolbar icon)
 2. Click the "Import" button, & import your gmail contacts

ACTUAL RESULTS:
I ended up with a bunch of imported contacts (in Firefox Hello's list) that are not actually in my gmail contacts list. They are people who I've emailed once or twice, or received email from, but never added to my contacts list, and are not listed at https://mail.google.com/mail/u/0/#contacts .

Comment 1

[Daniel Holbert [:dholbert]]

(Moreover, "Edit Contact" & "Remove Contact" are grayed out in the Firefox Hello per-contact dropdown menu, so I don't think I can actually remove all of these extra unwanted contacts. :-/)

Comment 2

[Maire Reavy [:mreavy]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> STR:
>  1. Sign into Firefox Hello (using the toolbar icon)
>  2. Click the "Import" button, & import your gmail contacts
> 
> ACTUAL RESULTS:
> I ended up with a bunch of imported contacts (in Firefox Hello's list) that
> are not actually in my gmail contacts list. They are people who I've emailed
> once or twice, or received email from, but never added to my contacts list,
> and are not listed at https://mail.google.com/mail/u/0/#contacts .

Mike -- Can you look into this?

Comment 3

[Maire Reavy [:mreavy]]

This sounds like an issue on the Gmail side of things (and how the contacts are organized), but Mike is the expert here.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #3)
> This sounds like an issue on the Gmail side of things

That's what I would expect, too, except that Firefox OS's "Contacts" app imports a different set of contacts -- the set that I would expect. (It looks like its imported contacts exactly match https://mail.google.com/mail/u/0/#contacts , unlike Firefox Hello.)

I just re-imported contacts into Firefox OS as a sanity-check & confirmed this.

For example: there's a random guy named Aaron who accidentally emailed me once, back in 2011. (He was trying to reach someone else; he emailed me due to a typo.)  I've never added him to my contact list, and he doesn't show up in my Gmail Contacts list, nor does he show up in Firefox OS gmail contacts import. (The first "A" name listed in a Firefox OS gmail-import is a close friend of mine named "Adam")

But this Aaron guy shows up at the top of my Firefox Hello contacts for some reason.

Comment 5

[u279076]

I've seen something similar in that I pruned 200+ contacts from contacts.google.com but they still synced when I imported my Google Contacts account with Firefox Hello.

Comment 7

[Daniel Holbert [:dholbert]]

(This makes me think that maybe Firefox Hello requests Gmail's contact list in a different way [or using a different permission?] as compared to how the Firefox OS Contacts App requests Gmail's contact list.)

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> (This makes me think that maybe Firefox Hello requests Gmail's contact list
> in a different way [or using a different permission?] as compared to how the
> Firefox OS Contacts App requests Gmail's contact list.)

We might, I'm not sure if the Firefox OS Contacts app uses the Google Contacts API or the regular CardDAV method.
Regardless, perhaps we ought to look at the group a contact belongs to - Google has the notion of system groups, among which a group called 'My Contacts': https://developers.google.com/google-apps/contacts/v3/#working_with_contact_groups

It could very well be that these are the contacts we really want and discard the contacts that don't belong to any group.

Adam, what do you think?

Comment 9

[Adam Roach [:abr]]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> (In reply to Daniel Holbert [:dholbert] from comment #7)
> > (This makes me think that maybe Firefox Hello requests Gmail's contact list
> > in a different way [or using a different permission?] as compared to how the
> > Firefox OS Contacts App requests Gmail's contact list.)
> 
> We might, I'm not sure if the Firefox OS Contacts app uses the Google
> Contacts API or the regular CardDAV method.

It's OAuth, which is what we're using here:

https://github.com/mozilla-b2g/gaia/blob/master/shared/js/contacts/import/gmail/gmail_connector.js

> Regardless, perhaps we ought to look at the group a contact belongs to -
> Google has the notion of system groups, among which a group called 'My
> Contacts':
> https://developers.google.com/google-apps/contacts/v3/
> #working_with_contact_groups

That does appear to be how FFxOS deals with it:

https://github.com/mozilla-b2g/gaia/blob/master/shared/js/contacts/import/gmail/gmail_connector.js#L78
 
> It could very well be that these are the contacts we really want and discard
> the contacts that don't belong to any group.

That sounds like a reasonable, and very small, change. Go for it.

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: make sure to only import contacts that are part of the default contacts group

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: make sure to only import contacts that are part of the default contacts group

Comment 12

[Adam Roach [:abr]]

Mike: this is a pretty big change. That has two implications: (1) I'm not going to have time to review it today, and tomorrow will be iffy; and (2) uplifting something this large to Beta is likely to be too risky to attempt.

I haven't had time to look closely at the contacts we get back from Google -- but do we have some indication in the contact itself about which group it belongs to? I know it's not elegant, but if they do, then we could land a minimal patch that simply ignores those not in the "Contacts" list (I think this would be maybe two or three lines in the google importer and nowhere else). Note that this would be *only* for 34, and we would take a more complete, correct approach for 35 and later. The goal is to get something small enough that we're comfortable uplifting to Beta.

Mike: Is that possible?

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8512582 [details] [diff] [review]
Patch v1: make sure to only import contacts that are part of the default contacts group

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

Flag me for review when you test the 8th contact and explain why it's now 8.

::: browser/components/loop/GoogleImporter.jsm
@@ +378,5 @@
>     *                                      contacts.
>     * @returns An `Error` object upon failure or an Object with statistics in the
>     *          following format: `{ total: 25, success: 13, ids: {} }`.
>     */
> +  _processContacts: Task.async(function* (contactEntries, db, tokenSet) {

JSDoc should be updated

::: browser/components/loop/test/mochitest/browser_GoogleImporter.js
@@ +16,5 @@
>      }, mockDb, window);
>    });
>  }
>  
> +const kContactsCount = 8;

I'm confused about why there should now be 8 as I only count 7 with http://www.google.com/m8/feeds/groups/tester%40mochi.com/base/6 for the groupMembershipInfo. Can you add testing the 8th contact to the bottom of the test like the existing 7?

Comment 14

[Mike de Boer [:mikedeboer]]

(In reply to Adam Roach [:abr] from comment #12)
> Mike: this is a pretty big change. That has two implications: (1) I'm not
> going to have time to review it today, and tomorrow will be iffy; and (2)
> uplifting something this large to Beta is likely to be too risky to attempt.

(1) Alright, I see that (awesome) Matt has stepped up to review this patch instead.
(2) Likely, yes, so that's why I prune _all_ incoming contacts from the database, before filtering on Group ID. I need to do this regardless, because contacts may be removed/ moved from one group to another in the meantime, so this also ensures that the Loop list stays in sync with the remote data source (Google).

> I haven't had time to look closely at the contacts we get back from Google
> -- but do we have some indication in the contact itself about which group it
> belongs to? 

I'm afraid not; the group names are mapped to IDs, which may be different per account. This is why I have to first fetch the list of groups, grab the system group 'Contacts' and keep hold of its ID. Each contact entry keeps track of the group it belongs to by using the ID, so there's no real way of hacking this in, I'm afraid.

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #13)
> I'm confused about why there should now be 8 as I only count 7 with
> http://www.google.com/m8/feeds/groups/tester%40mochi.com/base/6 for the
> groupMembershipInfo. Can you add testing the 8th contact to the bottom of
> the test like the existing 7?

I *think* the confusion stems from the name I gave to the `kContactsCount` constant; I should've called it `kIncomingTotalContactsCount` or something. What it means is that there are _8_ contact entries present in the XML returned by the mock server and only _7_ of those are part of the system group called 'Contacts'. That's why the database is supposed to contain only _7_ contacts and the contact that is not part of the correct group should be ignored and not be found in the database.
This is the exact thing I'm testing in the last testcase.

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: make sure to only import contacts that are part of the default contacts group

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8513378 [details] [diff] [review]
Patch v1.1: make sure to only import contacts that are part of the default contacts group

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

IMO this patch isn't too big/risky for Beta. I don't think the nodeValue => textContent conversion was necessary but it seems low risk (it seems like it's being done on elements and not attribute nodes as one would expect).

(In reply to Mike de Boer [:mikedeboer] from comment #15)
> This is the exact thing I'm testing in the last testcase.

I see but you're not asserting that there are only 7 contacts imported at the end which I think you should do.

::: browser/components/loop/GoogleImporter.jsm
@@ +401,5 @@
>        if (existing) {
>          yield db.promise("remove", existing._guid);
>        }
>  
> +      // After contact removal, check if the entry is part of the correct group.

Somewhat off-topic since I'm not that familiar with other code in this module: Do we properly handle if a contact was deleted remotely and then we re-import? Will it get deleted locally?

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to Matthew N. [:MattN] (focused on Loop) from comment #17)
> I see but you're not asserting that there are only 7 contacts imported at
> the end which I think you should do.

Alright, I'll add that.

> Somewhat off-topic since I'm not that familiar with other code in this
> module: Do we properly handle if a contact was deleted remotely and then we
> re-import? Will it get deleted locally?

If we re-import, the contact will be deleted locally as well. This is done to make sure we stay in-sync with the remote data source.

Comment 19

[Mike de Boer [:mikedeboer]]

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a8fad7641200

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.2: make sure to only import contacts that are part of the default contacts group

Patch with review comment addressed. Carrying over r=MattN.

Comment 21

[Maire Reavy [:mreavy]]

Comment on attachment 8513600 [details] [diff] [review]
Patch v1.2: make sure to only import contacts that are part of the default contacts group

Approval Request Comment
[Feature/regressing bug #]:Importing contacts
[User impact if declined]: Users will import email addresses that aren't in their Google contacts.  
[Describe test coverage new/current, TBPL]: tbpl, manual testing and verification
[Risks and why]:  Without this fix, the user will import email addresses that they have received emails from or sent email to, in addition to their true google contacts.  As a result, they'll see email addresses that surprise them.  This change makes us filter contacts just like FxOS contacts.  
[String/UUID change made/needed]: No strings

Comment 22

[Mike de Boer [:mikedeboer]]

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/b4729ee1bb81

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4729ee1bb81

Comment 24

[Marco Mucci [:MarcoM]]

Hi Mike, can you mark this bug for QE verification.

Comment 25

[Maire Reavy [:mreavy]]

Hi Pauly, can you test this in tomorrow's Nightly?  (It landed today so it should be ready in Nightly for you tomorrow to test and verify.)  Thanks!

Comment 26

[Paul Silaghi, QA [:pauly]]

This is what I see:
1. contacts without email/phone aren't imported
2. contacts from circles(google+), most contacted, other contacts aren't imported
3. imported contacts can't be deleted
4. contacts from 'my contacts' are imported
5. create a new group, add a new contact, select it in the list, from group options(from the bar above contacts) uncheck "my contacts" (with other words make the contact belong only to the new group) - the contact is not imported to Loop
Thoughts ?

Comment 27

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

Ok, I've done this on Nightly 11/1.  The import matches FxOS Contact import, with the known/correct exception of contacts with no email and no phone number.

Imported contacts can't be deleted (not an issue with this patch, known design decision).

Perf issue (not this bug, existing): We import at somewhere between 0.4 and 1 contact per second; FxOS imports at ~10 contacts/second.  Not a huge problem, might be "by design".

It correctly does not import a contact in a group that is not also in "My Contacts".  This is the same as FxOS (verified).

So: Verified.

Comment 28

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

Comment on attachment 8513600 [details] [diff] [review]
Patch v1.2: make sure to only import contacts that are part of the default contacts group

Aurora+

Plan is to test on Aurora tomorrow and, if successful, uplift tomorrow in time for beta6.

Comment 29

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/7aa2b763b628

Comment 30

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

Verified on Aurora nightly; same results as m-c.

Comment 31

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

Also verified on beta try run (linux64)

Comment 32

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

landed per discussion with lmandel after aurora verification
https://hg.mozilla.org/releases/mozilla-beta/rev/8b1b897ca39c

Comment 33

[Mike de Boer [:mikedeboer]]

Randall, thanks for verifying for me!

Comment 34

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b9 Win7