[Contacts] Pick vcard Activity should not return Facebook data

VERIFIED FIXED in 2.2 S4 (23jan)

Status

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: jmcf, Assigned: hola)

Tracking

unspecified
2.2 S4 (23jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(3 attachments, 1 obsolete attachment)

PR
46 bytes, text/x-github-pull-request
jmcf
: review+
Details | Review
4.38 MB, video/mp4
Details
2.67 MB, video/mp4
Details
(Reporter)

Description

4 years ago
STR: MMS App, Attach Contact, Facebook Contact. 

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

Comment 1

4 years ago
Asking blocking as contractually we should not allow to export FB Contacts to any media.
blocking-b2g: --- → 2.2?
Blocks: 1007932
(Reporter)

Comment 2

4 years ago
Posted file GH PR 27274 (obsolete) —
Attachment #8546536 - Flags: review?(francisco)
See Also: → 925988
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)
(Reporter)

Comment 4

4 years ago
(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)
(Reporter)

Updated

4 years ago
Assignee: jmcf → hola
(Reporter)

Updated

4 years ago
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+
(Assignee)

Comment 9

4 years ago
Posted 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)
(Reporter)

Comment 10

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

Comment 11

4 years ago
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)
(Reporter)

Comment 12

4 years ago
Comment on attachment 8549640 [details] [review]
PR

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

thanks!
Attachment #8549640 - Flags: review?(jmcf)
(Assignee)

Comment 13

4 years ago
Comment on attachment 8549640 [details] [review]
PR

Everything fixed. Thanks!
Attachment #8549640 - Flags: review?(jmcf)
(Reporter)

Comment 14

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

Comment 15

4 years ago
Comment on attachment 8549640 [details] [review]
PR

Code updated to work with arrays or strings.
Attachment #8549640 - Flags: review- → review?(jmcf)
(Reporter)

Comment 16

4 years ago
Comment on attachment 8549640 [details] [review]
PR

please address the nits on GH

good work!

thanks
Attachment #8549640 - Flags: review?(jmcf) → review+
(Reporter)

Comment 17

4 years ago
https://github.com/mozilla-b2g/gaia/commit/1296f6713a5274534347ae5db7696c396fefc2d6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Comment 18

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

Comment 20

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

Comment 21

4 years ago
Posted video Verify_Flame2.2.MP4

Comment 22

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

Updated

4 years ago
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.