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

Status: VERIFIED FIXED

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

Description

[Jose Manuel Cantera]

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

Comment 2

[Wilfred Mathanaraj [:WDM]]

Carrie: sonething here for you and it seems how do we store the contact and avoid duplicate.

Comment 3

[Noemí Freire (:noemi)]

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.

Comment 4

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

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!

Comment 5

[Noemí Freire (:noemi)]

(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

Comment 6

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

triage: blocker as current unexpected behavior causes confusion

NI to UX: please update spec accordingly

Comment 7

[Maria Angeles Oteo (:oteo)]

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!!

Comment 8

[Maria Angeles Oteo (:oteo)]

Current specs of the feature are available in bug 1007932

Comment 9

[Maria Angeles Oteo (:oteo)]

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?

Comment 10

[Adrián de la Rosa]

Attachment: PR

PR added to get early feedback. Tests are missing.

Comment 11

[Adrián de la Rosa]

Comment on attachment 8551722 [details] [review]
PR

Tests added.

Comment 12

[Jose Manuel Cantera]

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.

Comment 13

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

Hi, 

Yes I've reviewed the spec before and it looks fine to me. Thanks!

Comment 14

[Adrián de la Rosa]

Comment on attachment 8551722 [details] [review]
PR

Every comment addressed. Now the view shows a close button instead of a back button.

Comment 15

[Jose Manuel Cantera]

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.

Comment 16

[Jose Manuel Cantera]

Comment on attachment 8551722 [details] [review]
PR

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

thanks!

Comment 17

[vsireesha246]

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..

Comment 18

[Jose Manuel Cantera]

(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

Comment 19

[vsireesha246]

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..

Comment 20

[Jose Manuel Cantera]

(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.

Comment 21

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

Just left a tiny nit in the PR, thanks guys for taking care of this!

Comment 22

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

Just retriggered all test again to check.

Comment 23

[Jose Manuel Cantera]

Comment on attachment 8551722 [details] [review]
PR

please rebase and address the comments on GH. It seems the current patch is not working properly

Comment 24

[Jose Manuel Cantera]

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!

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

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.)

Comment 27

[Jose Manuel Cantera]

(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.

Comment 28

[Jose Manuel Cantera]

(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

Comment 29

[Jose Manuel Cantera]

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

Comment 30

[Jose Manuel Cantera]

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

Comment 31

[Jose Manuel Cantera]

(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

Comment 32

[Noemí Freire (:noemi)]

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)

Comment 33

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8551722 [details] [review]
PR

(removing my r- for the bit of the patch that was removed)

Comment 34

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/63ce5cc74dab6a72c69f4d7fa9929268ab76c2c9

Comment 35

[Elie]

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

Comment 36

[Elie]

Attachment: VIDEO0279_Compress.MP4

Comment 37

[Shine(Leave from Mozilla)]

Per Comment 35, clear verifyme tag.