Closed Bug 1120878 Opened 5 years ago Closed 5 years ago

[Contacts] View vcard activity should disable edit button if the vcard is an attachment that is already on the device

Categories

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

x86
macOS
defect
Not set

Tracking

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

VERIFIED FIXED
2.2 S5 (6feb)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: jmcf, Assigned: hola)

References

Details

Attachments

(2 files)

PR
46 bytes, text/x-github-pull-request
jmcf
: review+
Details | Review
2.19 MB, video/mp4
Details
STR:

Contacts --> Details --> Share Contact --> Messages --> View Attachment --> Edit button is enabled

Expected: Edit button should be disabled as the contact source is the Contacts App itself
Summary: [Contacts] View vcard activity should disable edit button if the vcard is an attachment that is already on the devices → [Contacts] View vcard activity should disable edit button if the vcard is an attachment that is already on the device
See Also: → 1113605
Assignee: hola → marina.rodrigueziglesias
Assignee: marina.rodrigueziglesias → hola
Duplicate of this bug: 1122452
blocking-b2g: --- → 2.2?
QA Whiteboard: [COM=Gaia::Contacts][2.2-feature-qa+]
Carrie: sonething here for you and it seems how do we store the contact and avoid duplicate.
Flags: needinfo?(cawang)
Hope it helps to clarify the flow described within this bug: 
*There is a local contact that user wants to share, so she goes to "Contacts app->Share Contact->Messages". Once there, user decides to view the contact details before sending it so she clicks on the attachment and contact details screen is opened up, currently "Done" button is available allowing users to modify the existing contact and asking for merging when saving. That behavior is the one this bug wants to avoid just removing Done button.

On the other hand, notice that in case of receiving a contact via MMS, users will be able to store the contact just clicking on the attachment (within Messaging app) -> View -> Add contact -> Done button.
I think not only the "Done" but all the editable buttons shall be disabled or even removed. I remember that Jenny has dealt with the "attachment" issue in Messages before. ni? Jenny to confirm the solution. Thanks!
Flags: needinfo?(cawang) → needinfo?(jelee)
(In reply to Carrie Wang [:carrie] from comment #4)
> I think not only the "Done" but all the editable buttons shall be disabled
> or even removed. I remember that Jenny has dealt with the "attachment" issue
> in Messages before. ni? Jenny to confirm the solution. Thanks!

Yes, that's the proposal, non-editable fields + no "Done" button
triage: blocker as current unexpected behavior causes confusion

NI to UX: please update spec accordingly
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(cawang)
Clearing the ni to Carrie, as Pau was the one who provided the spec.
Pau, could you update the spec with the last changes according in this bug? and after that setting ni to Carrie to confirm the UI?
Thanks a lot!!
Flags: needinfo?(cawang) → needinfo?(b.pmm)
Current specs of the feature are available in bug 1007932
Blocks: 1007932
Francisco, Wesley, rechecking the spec with Noemi we have seen that this flow is already covered. See attachments in bug 1007932. 

The flows were already reviewed by Jenny and Carrie, the problem is that was not implemented according to the spec.

Let us know if you need Pau to cover something else in the spec or a specific visual for that screen, anyway Adrian can ask Fang or Carol for ui-review before landing this patch. Is it ok with you?
Flags: needinfo?(whuang)
Flags: needinfo?(francisco)
Attached file PR
PR added to get early feedback. Tests are missing.
Attachment #8551722 - Flags: feedback?(jmcf)
Comment on attachment 8551722 [details] [review]
PR

Tests added.
Attachment #8551722 - Flags: feedback?(jmcf) → review?(jmcf)
Comment on attachment 8551722 [details] [review]
PR

it goes in the right direction but there are some style changes made in JS that should be done using CSS classes.
Attachment #8551722 - Flags: review?(jmcf) → review-
Hi, 

Yes I've reviewed the spec before and it looks fine to me. Thanks!
Flags: needinfo?(jelee)
Comment on attachment 8551722 [details] [review]
PR

Every comment addressed. Now the view shows a close button instead of a back button.
Attachment #8551722 - Flags: review- → review?(jmcf)
Flags: needinfo?(whuang)
Flags: needinfo?(b.pmm)
Comment on attachment 8551722 [details] [review]
PR

looks good but we would need to modify the SMS app as well to pass the parameter 'allowSave' when a view text/vcard attachment request is made.
Attachment #8551722 - Flags: review?(jmcf)
Attachment #8551722 - Flags: review?(jmcf)
Comment on attachment 8551722 [details] [review]
PR

please check the latest nits on GH and make all tests pass

thanks!
Attachment #8551722 - Flags: review?(jmcf) → review+
After applying this patch,i found the below issue.

Contacts --> Details --> Share Contact --> Messages --> View Attachment -->again share-->Messages

Message app is shown as blank.

Here we need to disable share button to avoid circular share..
(In reply to vsireesha246 from comment #17)
> After applying this patch,i found the below issue.
> 
> Contacts --> Details --> Share Contact --> Messages --> View Attachment
> -->again share-->Messages
> 
> Message app is shown as blank.
> 
> Here we need to disable share button to avoid circular share..

Yes, good point
One more thing we need to check here

Contacts --> Details --> Share Contact --> Messages --> Send-->after Send-->Click on vcf attachment-->Done button is active again..
(In reply to vsireesha246 from comment #19)
> One more thing we need to check here
> 
> Contacts --> Details --> Share Contact --> Messages --> Send-->after
> Send-->Click on vcf attachment-->Done button is active again..

that's the expected behavior as in that case the vcard is not in the draft attachment state but in the thread and in that particular case is sensible to allow the user to import it. Imagine for instance that I've deleted the contact by mistake and i want to restore it from a previous conversation on which I sent it.
Flags: needinfo?(francisco)
Just left a tiny nit in the PR, thanks guys for taking care of this!
Just retriggered all test again to check.
Comment on attachment 8551722 [details] [review]
PR

please rebase and address the comments on GH. It seems the current patch is not working properly
Attachment #8551722 - Flags: review+
Attachment #8551722 - Flags: review?(jmcf)
Comment on attachment 8551722 [details] [review]
PR

r+ for the Contacts part

Andrew please could you check the tiny change in the email app?

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

Currently, anything the email app saves to allow it to be opened is already stored in DeviceStorage.  At least the gallery app interprets "allowSave" as a boolean as to whether it should show the "save" button to allow it to be saved to DeviceStorage.  So always passing save is the wrong thing for the email app right now.

The good news is I'm already changing some stuff related to attachments in the email app on bug 825318.  Ideally we would clean up/normalize our web activity situation (and document it on https://developer.mozilla.org/en-US/docs/Web/API/Web_Activities#Firefox_OS_activities !), but that hasn't really gone anywhere, so the good news is we can customize by MIME types when there is just a single MIME type we're dealing with, like vcard.

So I think the question is what would you like email to do with text/vcard attachments?  Because vcards are generally quite small, I can also initially store them in IndexedDB.

Really, our options are:

1) When downloaded, store in IndexedDB.  Have "open" activity specify whatever you want for allowSave for just text/vcard.

2) When downloaded, store in DeviceStorage *and track with the download manager* so the user can delete the file later on.  Have "open" activity specify whatever you want for allowSave.

The one slight advantage to the second option is that it allows a flow where the user can save a vcard and then re-attach it later (via the download manager) without round-tripping it through contacts (I think?), although it's not clear that would be a particularly common use-case.  (However, because email can't forward attachments right now, it is sort-of a work-around we might like to support.)

Let me know what you want, and I'll put it in my patch.  But in the meantime, please just remove the email part of the patch from your pull request.
Attachment #8551722 - Flags: review?(bugmail) → review-
Uh, and the main difference between 1 and 2 for your purposes would be whether you need to deal with the download manager triggering an "open" activity on the vcard.  Since users can already download vcards via other sources, it's something you would already need to be dealing with.  (And then download manager might need its "open" activity customized too.)
(In reply to Andrew Sutherland [:asuth] from comment #26)
> Uh, and the main difference between 1 and 2 for your purposes would be
> whether you need to deal with the download manager triggering an "open"
> activity on the vcard.  Since users can already download vcards via other
> sources, it's something you would already need to be dealing with.  (And
> then download manager might need its "open" activity customized too.)

We are going to remove, for the moment, the email change. I can understand that the e-mail app should treat all the attachments in the same way. But, as you say, a vcard attachment is so tiny that it might be worth allowing the user to save it as a contact directly, without going through the Download Manager path. 

I don't have an strong opinion here.
(In reply to Jose Manuel Cantera from comment #27)
> (In reply to Andrew Sutherland [:asuth] from comment #26)
> > Uh, and the main difference between 1 and 2 for your purposes would be
> > whether you need to deal with the download manager triggering an "open"
> > activity on the vcard.  Since users can already download vcards via other
> > sources, it's something you would already need to be dealing with.  (And
> > then download manager might need its "open" activity customized too.)
> 
> We are going to remove, for the moment, the email change. I can understand
> that the e-mail app should treat all the attachments in the same way. But,
> as you say, a vcard attachment is so tiny that it might be worth allowing
> the user to save it as a contact directly, without going through the
> Download Manager path. 
> 
> I don't have an strong opinion here.

After a careful analysis of this issue our opinion is that option 1/ is preferrable for the following reasons:

a/ It is natural to treat vcard files differently (or vcal ones) as they are unmeaningful outside of an address book or a calendar. In fact they are data serializations which main purpose is to be imported by another app.  So, in these particular cases it does make totally sense to give the user the opportunity to add to their calendars or address books directly from the email app. 

b/ Currently we are allowing users to follow the path described above, so if we do not implement option 1 we will be regressing with respect to what we have right now. 

c/ files downloaded by email are not currently tracked by the Download Manager. Actually, the download manager currently can deal with vcards seamlessly.  

thanks Andrew
https://github.com/mozilla-b2g/gaia/commit/a8c3583f60567f1eac58b445743e1fcbbe552a30
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8551722 [details] [review]
PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): vcard as attachments of MMS / share contact functionality for v2.2
[User impact] if declined: High. User can perceive a malfunctioning device
[Testing completed]: Yes: Unit and integration tests are provided
[Risk to taking this patch] (and alternatives if risky): Low Risk patch. alternative does not exist. 
[String changes made]: none
Attachment #8551722 - Flags: approval-gaia-v2.2?
(In reply to Jose Manuel Cantera from comment #30)
> Comment on attachment 8551722 [details] [review]
> PR
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): vcard as attachments of MMS /
> share contact functionality for v2.2
> [User impact] if declined: High. User can perceive a malfunctioning device
> [Testing completed]: Yes: Unit and integration tests are provided
> [Risk to taking this patch] (and alternatives if risky): Low Risk patch.
> alternative does not exist. 
> [String changes made]: none

please note that the r- is for the email part of the patch, that was removed from the final commit. see comment #25
Adding dependency on Bug 825318 since the change needed in email app will be performed as part of that bug (see comment 25 and comment 28 for further details)
Depends on: 825318
Keywords: verifyme
Target Milestone: --- → 2.2 S5 (6feb)
Comment on attachment 8551722 [details] [review]
PR

(removing my r- for the bit of the patch that was removed)
Attachment #8551722 - Flags: review-
Attachment #8551722 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
This issue verified successfully on flame 2.2&3.0

Flame 2.2:
Build ID               20150204002509
Gaia Revision          a4c4cc86303a554facb8f45b7e764e5c4473c3de
Gaia Date              2015-02-04 00:41:59
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8669c26fd4a5
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
FLame 3.0 :
Build ID               20150204010225
Gaia Revision          dfebaaa8aab43470f482d09d71137bab840c3ae9
Gaia Date              2015-02-03 18:49:40
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/0c2f7434c325
Gecko Version          38.0a1
Device Name            flame
Firmware(Release)      4.4.2
Status: RESOLVED → VERIFIED
Attached video VIDEO0279_Compress.MP4
Per Comment 35, clear verifyme tag.
Keywords: verifyme
See Also: → 1017924
See Also: → 1149938
You need to log in before you can comment on or make changes to this bug.