925988 - [B2G] [Contacts] Facebook contacts are present when choosing which contacts to export

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[Chris Kreinbring [:CKreinbring]]

Attachment: Screenshot of export page with facebook contacts

Description:
Facebook contacts are present on the Export Contacts pages.

Repro Steps:
1) Update Buri to Build ID: 20131011004001
2) Launch the Contacts app.
3) If there aren't already contacts imported from Facebook, import some from the Settings page.
4) Select Settings then Export Contacts.
5) Choose any of the export options.
6) Look for the Facebook contacts on the Export page.

Actual:
Facebook contacts are present on the Export Contacts page.

Expected:
Facebook contacts are filtered out of the Export Contacts page.

Environmental Variables
Device: Buri 1.2 mozilla RIL
Build ID: 20131011004001
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/c8e97fd5b94d
Gaia: 79abf09f2b5b6440f43cb5ae44ef6c85c0437e8d
Platform Version: 26.0a2

Notes:
Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/cases/?filter-id=9785
See attached screenshot

Comment 1

[Jason Smith [:jsmith]]

Why is this necessary?

Comment 2

[Jose Manuel Cantera]

yes, they are present but its info is not actually exported.

Closing worksforme

Comment 4

[Johan Lorenzo [:jlorenzo]]

Carrying over the flags from bug 1108148.

I don't think a Facebook contact should be displayed in the export list as it may confuse an end user. Or at least, we should display an error message to explain to a user that the contact data will be partially exported.

Currently, we export a FB contact's name via BT or memory card without any warning. A user will probably think that everything is correctly exported.

Jose, I think this is still an issue, what's your take on this?

Comment 5

[Jose Manuel Cantera]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #4)
> Carrying over the flags from bug 1108148.
> 
> I don't think a Facebook contact should be displayed in the export list as
> it may confuse an end user. Or at least, we should display an error message
> to explain to a user that the contact data will be partially exported.
> 
> Currently, we export a FB contact's name via BT or memory card without any
> warning. A user will probably think that everything is correctly exported.
> 
> Jose, I think this is still an issue, what's your take on this?

We keep the FB Contacts in order to render the list faster. Filtering out FB contacts will make the list build longer affecting performance.

Comment 6

[Johan Lorenzo [:jlorenzo]]

I understand the performance concern. I think we should at least show a warning when we export one or more FB contacts because a user would expect to have all the data exported.

Carrie, what behavior should we have here?

Comment 7

[Carrie Wang [New account: [e-mail]]]

Hi,

I think if we don't allow users export FB contacts, we should 
1. filter out the FB contacts 
or
2. keep the FB contacts unselectable (being greyed-out)

Comment 8

[Johan Lorenzo [:jlorenzo]]

[Blocking Requested - why for this release]: Even though this issue exists since the dawn of this project, the behavior is confusing for an end user who might think the functionality is broken. Moreover, if you export all your contacts, having FB contacts in the export list can lead to have a lot of absolutely useless entries in the generated vCard file. This will pollute the user's next importation and will create a lot a dupes at the next FB sync (as we don't automatically merge FB contacts).

Comment 9

[Johan Lorenzo [:jlorenzo]]

Oops, comment 8 was only a request.

Comment 10

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: keep the nomination for now and we'll go back to triage this after hearing feedbacks from developers regarding UX's suggestion.

Comment 11

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Maria,
Mind to let us know if you get any feedback?

Comment 12

[Jose Manuel Cantera]

I think the best way to resolve this bug is as suggested by Carrie in comment 7, option 2. Actually it can be done using CSS, which should not have impact on performance.

Comment 13

[Wilfred Mathanaraj [:WDM]]

Jose  - can you have a look at fixing it but I would not block on it. If this is ready in time for 2.2 then we will pick it up based on risk.

Comment 14

[Jose Manuel Cantera]

yes, it should be ready for v2.2

Comment 15

[Jose Manuel Cantera]

Adrian,

I left some comments over 

https://github.com/ADLR-es/gaia/commit/ed144259ba822cd988688fd8e36d4e7b415a0084

Comment 16

[Francisco Jordano [:arcturus] [:francisco]]

Can we do this more generic.

Instead of adding a new class to the select list for exporting, that is pretty similar to selecting, I would like something generic for 'disabling clicking in fb contacts' no matter if we are exporting or not.

Then we can use that for, selecting ice contacts, or any other kind of action over contacts in the list.

Comment 17

[Jose Manuel Cantera]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #16)
> Can we do this more generic.
> 
> Instead of adding a new class to the select list for exporting, that is
> pretty similar to selecting, I would like something generic for 'disabling
> clicking in fb contacts' no matter if we are exporting or not.

That's certainly a good idea. 

> 
> Then we can use that for, selecting ice contacts, or any other kind of
> action over contacts in the list.

Comment 18

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

I think it's a backlog item rather than a minus. 
We would want it to be fixed.

Comment 19

[Adrián de la Rosa]

Attachment: Pull request

Comment 20

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: for it's blocking bug 1119738

Comment 21

[Adrián de la Rosa]

Attachment: Pull request

Comment 22

[Jose Manuel Cantera]

Comment on attachment 8547597 [details] [review]
Pull request

please see the comments on GH. We need another round ...

Comment 23

[Jose Manuel Cantera]

Comment on attachment 8547597 [details] [review]
Pull request

please address the three pending comments on GH

good work!

Comment 24

[Jose Manuel Cantera]

https://github.com/mozilla-b2g/gaia/commit/bcc58499fa9429ee1a480f18357ba319c70bf505

Comment 25

[Jose Manuel Cantera]

Comment on attachment 8547597 [details] [review]
Pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Facebook restrictions for sharing data outside our device
[User impact] if declined: Medium, the user would expect her Facebook data exported but in reality it would not be exported. 
[Testing completed]: Yes, unit tests are provided 
[Risk to taking this patch] (and alternatives if risky): Low-risk patch
[String changes made]: None

Comment 26

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/5658ebd260b0c7481e018503c4819a4d4bd9c100

Comment 27

[Brogan Zumwalt [Inactive]]

Issue verified fixed on Flame 2.2 and 3.0

Imported Facebook contacts are greyed out and unselectable in list when trying to export them. Locally stored non-Facebook contacts are still selectable in export list. Tested with Facebook contacts only as well as with a mix of facebook and non-facebook contacts.

Device: Flame 2.2
BuildID: 20150121093932
Gaia: e4f9b5da3751798f9cc5d95f302c30722cc11fca
Gecko: 53421e00294d
Version: 37.0a2 (2.2)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 3.0 Master
BuildID: 20150121141532
Gaia: 917b6c36717fddc6e71ffc1ec249633c8044c93c
Gecko: 06b590bf59f4
Version: 38.0a1 (3.0 Master)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0