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

VERIFIED FIXED in Firefox 34

Status

defect
P1
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: mikedeboer)

Tracking

unspecified
mozilla36
Points:
1
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox34 verified, firefox35 verified, firefox36 verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

5 years ago
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 .
Reporter

Comment 1

5 years ago
(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. :-/)
(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?
Flags: needinfo?(mdeboer)
This sounds like an issue on the Gmail side of things (and how the contacts are organized), but Mike is the expert here.
Reporter

Comment 4

5 years ago
(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.
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 hidden (spam)
Reporter

Comment 7

5 years ago
(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.)
Assignee

Comment 8

5 years ago
(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?
Flags: needinfo?(mdeboer) → needinfo?(adam)

Comment 9

5 years ago
(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.
Flags: needinfo?(adam)
Assignee

Updated

5 years ago
Flags: needinfo?(mdeboer)
Assignee

Updated

5 years ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 36.2
Points: --- → 1
Flags: needinfo?(mdeboer) → needinfo?(mmucci)
OS: Linux → All
Hardware: x86_64 → All
Assignee

Comment 11

5 years ago
Attachment #8512578 - Attachment is obsolete: true
Attachment #8512578 - Flags: review?(adam)
Attachment #8512582 - Flags: review?(adam)
Iteration: 36.2 → ---
Flags: needinfo?(mmucci)
Iteration: --- → 36.2
Flags: qe-verify?
Flags: firefox-backlog+
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?
Flags: needinfo?(mdeboer)
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?

Updated

5 years ago
backlog: --- → Fx35+
Priority: -- → P1
Assignee

Comment 14

5 years ago
(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.
Flags: needinfo?(mdeboer)
Assignee

Comment 15

5 years ago
(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.
Assignee

Comment 16

5 years ago
Attachment #8512582 - Attachment is obsolete: true
Attachment #8512582 - Flags: review?(adam)
Attachment #8513378 - Flags: review?(MattN+bmo)
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?
Attachment #8513378 - Flags: review?(MattN+bmo) → review+
Assignee

Comment 18

5 years ago
(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.
Assignee

Comment 20

5 years ago
Patch with review comment addressed. Carrying over r=MattN.
Attachment #8513378 - Attachment is obsolete: true
Attachment #8513600 - Flags: review+
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
Attachment #8513600 - Flags: approval-mozilla-beta?
Attachment #8513600 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b4729ee1bb81
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Hi Mike, can you mark this bug for QE verification.
Flags: needinfo?(mdeboer)
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!
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(paul.silaghi)
Flags: needinfo?(mdeboer)
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 ?
Flags: needinfo?(paul.silaghi) → needinfo?(mdeboer)
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 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.
Attachment #8513600 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified on Aurora nightly; same results as m-c.
Also verified on beta try run (linux64)
landed per discussion with lmandel after aurora verification
https://hg.mozilla.org/releases/mozilla-beta/rev/8b1b897ca39c
Attachment #8513600 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Comment 33

5 years ago
Randall, thanks for verifying for me!
Flags: needinfo?(mdeboer)
Verified fixed FF 34b9 Win7
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.