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

P1
normal
VERIFIED FIXED
4 years ago
4 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

4 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

4 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

4 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

4 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

4 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

4 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

4 years ago
Flags: needinfo?(mdeboer)
(Assignee)

Updated

4 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 10

4 years ago
Created attachment 8512578 [details] [diff] [review]
Patch v1: make sure to only import contacts that are part of the default contacts group
Attachment #8512578 - Flags: review?(adam)
(Assignee)

Comment 11

4 years ago
Created attachment 8512582 [details] [diff] [review]
Patch v1: make sure to only import contacts that are part of the default contacts group
Attachment #8512578 - Attachment is obsolete: true
Attachment #8512578 - Flags: review?(adam)
Attachment #8512582 - Flags: review?(adam)

Updated

4 years ago
Iteration: 36.2 → ---
Flags: needinfo?(mmucci)

Updated

4 years ago
Iteration: --- → 36.2
Flags: qe-verify?
Flags: firefox-backlog+

Comment 12

4 years ago
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

4 years ago
backlog: --- → Fx35+
Priority: -- → P1
(Assignee)

Comment 14

4 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

4 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

4 years ago
Created attachment 8513378 [details] [diff] [review]
Patch v1.1: make sure to only import contacts that are part of the default contacts group
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

4 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)

Updated

4 years ago
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
(Assignee)

Comment 20

4 years ago
Created attachment 8513600 [details] [diff] [review]
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.
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
Last Resolved: 4 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)
status-firefox36: affected → fixed
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.

Updated

4 years ago
status-firefox36: fixed → 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.
status-firefox35: fixed → verified
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

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