Closed Bug 1119738 Opened 6 years ago Closed 6 years ago

[Contacts] Pick vcard Activity should not return Facebook data

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S4 (23jan)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: jmcf, Assigned: hola)

References

Details

Attachments

(3 files, 1 obsolete file)

PR
46 bytes, text/x-github-pull-request
jmcf
: review+
Details | Review
4.38 MB, video/mp4
Details
2.67 MB, video/mp4
Details
STR: MMS App, Attach Contact, Facebook Contact. 

We should not allow to attach Facebook Contacts in an MMS Message.
Asking blocking as contractually we should not allow to export FB Contacts to any media.
blocking-b2g: --- → 2.2?
Blocks: 1007932
Attached file GH PR 27274 (obsolete) —
Attachment #8546536 - Flags: review?(francisco)
Hi Jose,
Does it actually share the data out?
If so, we should block since the case is worse than bug 925988.
Flags: needinfo?(jmcf)
(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
Flags: needinfo?(jmcf)
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.
Attachment #8546536 - Flags: review?(francisco)
(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!
Flags: needinfo?(cawang)
removing ni to Carrie since the final idea is FB contacts appearing "disabled". Sorry for the misunderstanding.
Flags: needinfo?(cawang)
Assignee: jmcf → hola
Depends on: 925988
triage: blocking per comment 4. And we'll also make bug 925988 2.2+ for consistency.
blocking-b2g: 2.2? → 2.2+
Attached file PR
Pull request with the proposed new approach. Facebook contacts are disabled when trying to pick one of them instead of showing an error.
Attachment #8546536 - Attachment is obsolete: true
Attachment #8549640 - Flags: review?(jmcf)
Comment on attachment 8549640 [details] [review]
PR

We need more work on the PR

thanks
Attachment #8549640 - Flags: review?(jmcf)
Target Milestone: --- → 2.2 S4 (23jan)
Comment on attachment 8549640 [details] [review]
PR

Everything pointed out in comments has been solved. Works as expected on device.
Attachment #8549640 - Flags: review?(jmcf)
Comment on attachment 8549640 [details] [review]
PR

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

thanks!
Attachment #8549640 - Flags: review?(jmcf)
Comment on attachment 8549640 [details] [review]
PR

Everything fixed. Thanks!
Attachment #8549640 - Flags: review?(jmcf)
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
Attachment #8549640 - Flags: review?(jmcf) → review-
Comment on attachment 8549640 [details] [review]
PR

Code updated to work with arrays or strings.
Attachment #8549640 - Flags: review- → review?(jmcf)
Comment on attachment 8549640 [details] [review]
PR

please address the nits on GH

good work!

thanks
Attachment #8549640 - Flags: review?(jmcf) → review+
https://github.com/mozilla-b2g/gaia/commit/1296f6713a5274534347ae5db7696c396fefc2d6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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
Attachment #8549640 - Flags: approval-gaia-v2.2?
Keywords: verifyme
Attachment #8549640 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
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
QA Whiteboard: MGSEI-Triage+
Attached video Verify_Flame2.2.MP4
Attached video 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
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.