1119738 - [Contacts] Pick vcard Activity should not return Facebook data

Status: VERIFIED FIXED

Description

[Jose Manuel Cantera]

STR: MMS App, Attach Contact, Facebook Contact. 

We should not allow to attach Facebook Contacts in an MMS Message.

Comment 1

[Jose Manuel Cantera]

Asking blocking as contractually we should not allow to export FB Contacts to any media.

Comment 2

[Jose Manuel Cantera]

Attachment: GH PR 27274

Comment 3

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

Hi Jose,
Does it actually share the data out?
If so, we should block since the case is worse than bug 925988.

Comment 4

[Jose Manuel Cantera]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #3)
> Hi Jose,
> Does it actually share the data out?

yes,

> If so, we should block since the case is worse than bug 925988.

I think so

Comment 5

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

Comment on attachment 8546536 [details] [review]
GH PR 27274

Hi,

I tried out the patch and works perfectly, but was wondering if it's necessary that we add another facebook dependencies to the activities file.
Can we move this dependency to a class that already contains the FB dependency, and make this 'selection' a bit more agnostic.

I mean, something like let the list, or the list model to decide if you can select an element. And if not giving a 'reason'. So later on we could reuse this in different cases to allow or disallow (better said) the selection of some elements.

I think that we have another patch to gray out this kind of contacts too, made the same comment, try to do the selection generic. In that case is for the select mode, but kinds of apply here too.

Also the wording: 'The contact cannot be shared due to Facebook constraint.' sounds weird to me.

Comment 6

[Noemí Freire (:noemi)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #5)
> Comment on attachment 8546536 [details] [review]
> GH PR 27274
> 
> Also the wording: 'The contact cannot be shared due to Facebook constraint.'
> sounds weird to me.

asking Carrie for her input here. Thanks!

Comment 7

[Noemí Freire (:noemi)]

removing ni to Carrie since the final idea is FB contacts appearing "disabled". Sorry for the misunderstanding.

Comment 8

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

triage: blocking per comment 4. And we'll also make bug 925988 2.2+ for consistency.

Comment 9

[Adrián de la Rosa]

Attachment: PR

Pull request with the proposed new approach. Facebook contacts are disabled when trying to pick one of them instead of showing an error.

Comment 10

[Jose Manuel Cantera]

Comment on attachment 8549640 [details] [review]
PR

We need more work on the PR

thanks

Comment 11

[Adrián de la Rosa]

Comment on attachment 8549640 [details] [review]
PR

Everything pointed out in comments has been solved. Works as expected on device.

Comment 12

[Jose Manuel Cantera]

Comment on attachment 8549640 [details] [review]
PR

We need another round. Please address all the comments on GH

thanks!

Comment 13

[Adrián de la Rosa]

Comment on attachment 8549640 [details] [review]
PR

Everything fixed. Thanks!

Comment 14

[Jose Manuel Cantera]

Comment on attachment 8549640 [details] [review]
PR

the code is not what was suggested at 

https://github.com/mozilla-b2g/gaia/pull/27419#discussion_r23208018

particularly it does not make any sense that activityType is not defined neither it is an array

Comment 15

[Adrián de la Rosa]

Comment on attachment 8549640 [details] [review]
PR

Code updated to work with arrays or strings.

Comment 16

[Jose Manuel Cantera]

Comment on attachment 8549640 [details] [review]
PR

please address the nits on GH

good work!

thanks

Comment 17

[Jose Manuel Cantera]

https://github.com/mozilla-b2g/gaia/commit/1296f6713a5274534347ae5db7696c396fefc2d6

Comment 18

[Jose Manuel Cantera]

Comment on attachment 8549640 [details] [review]
PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): contact vcard attachments
[User impact] if declined: User impact is not high but the very high impact is on the current agreements with Facebook in order no to share FB data with the outside world
[Testing completed]: unit tests are present in the patch
[Risk to taking this patch] (and alternatives if risky): No alternative is possible
[String changes made]: None

Comment 19

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/310a45c0b0ead803f55c7cd1c593d1c11ba979b0

Comment 20

[Coler]

This issue has verify successfully on Flame 2.2.
See attachment: verified_Flame2.2.mp4.
Reproduce rate: 0/5.

STR：
Prerequisites： There are some Facebook contacts in Contact app.
1. Launch SMS.
2. Create a new message.
3. Tap "Attachments" button, select "Communications".
4. Check the status of Facebook contacts.
**They all are gary, can't be selected.

Flame2.2 build version:

Gaia-Rev        0518f4581a0925c0b703d730ef289ab15cbd1216
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c6aa604a7967
Build-ID        20150125002503
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150125.035924
FW-Date         Sun Jan 25 03:59:36 EST 2015
Bootloader      L1TC000118D0

Comment 21

[Coler]

Attachment: Verify_Flame2.2.MP4

Comment 22

[Sue]

Attachment: verify_v3.0.MP4

This issue has been verified successfully on Flame v3.0
See attachment:verify_v3.0.MP4
Rate:0/5

Flame 3.0 build:
Gaia-Rev        b02ec9713e6de8d96c6954d2c0dfd0442b0656ac
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/38e4719e71af
Build-ID        20150127010228
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150127.043726
FW-Date         Tue Jan 27 04:37:39 EST 2015
Bootloader      L1TC000118D0