1138371 - Not able to save in the Agenda a received vcard

Status: VERIFIED FIXED

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

Description

[Maria Angeles Oteo (:oteo)]

Pre-requisite: Share a contact (vcard) with a FxOS user via E-mail application. The .vcf file can be sent from another FxOS, Android device...

STR:
1. E-mail is received in FxOS device.
2. Click on the received vcard attachment to download the corresponding .vcf file
3. After completing the Download, click on the "eye" icon in the mail to preview the vcard.

Actual Result:The vcard is opened via contacts application, but right now there is no any option to save the new contact in the Agenda (perhaps with a "done" or "Save" button)


Expected Result: The vcard is opened via contacts application and there should be a way to add this new contact in the Agenda. 

Note: In the vcard preview you have available the "Add to Favorites" button, when you press on it, the contact is saved in the Agenda (and in the Favourite section too)

Environmental Variables:
Device: Flame Master (
Build ID: 20150302073244    
Gaia: cf3b761
Gecko: 75fc721
Platform Version: 39.0a1 (Master)
Firmware Version: v18D

Comment 1

[Jose Manuel Cantera]

The email application is not calling the activity with the 'allowSave' parameter set to true

Comment 2

[Maria Angeles Oteo (:oteo)]

Thanks Jose Manuel for your answer!
After commnet 1, setting the correct component.

Comment 3

[Maria Angeles Oteo (:oteo)]

Andrew, could you have a look at it? Thanks a lot!

Comment 4

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

We're running into the problem that our web-activities are a hodge-podge of convention with no documented semantics.

The gallery, music, and video apps use "allowSave" to indicate that they activity-caller wants to allow the provided Blob to be saved to DeviceStorage.  Gallery/friends do not seem to have a guard that disables this mechanism if the file is already persisted on DeviceStorage, so you would not want to pass { allowSave: true } if the Blob is already saved there.

It seems like the contacts app is also using "allowSave" to indicate that the activity-caller wants to allow the contact to be saved to mozContacts.  I don't understand the rationale for this.  Why would an activity-caller passing a Blob be able to make a decision about whether it's appropriate for its contents to be saved to mozContacts?  My best guess is that this was the simplest solution to address some contacts bug and someone noticed that SMS was passing { allowSave: true } in certain circumstances.

Unfortunately I don't entirely understand SMS's attachment-handling flow/Blob handling.  I see that it can trigger an 'open' activity from compose which will not pass { allowSave: true }, but thread_ui.js will pass it.  I do see that MozMmsMessage exposes Blobs, so maybe in maintains some type of storage separate from DeviceStorage?  In which case its use of allowSave seems 100% correct.


In any event, I think contacts is in the wrong here and the right course of action is for it to stop using "allowSave" and instead address whatever motivated its use via other means.  It's worth noting that the "downloads" settings UI also does not pass { allowSave: true } in its triggering of the 'open' activity, so even if we specialized email here, we'd also need to specialize the downloads logic too, plus all other users of the 'open' activity.

I'm needinfo'ing :djf for the media apps and :julienw for SMS discussion of this.

Comment 5

[Julien Wajsberg [:julienw]]

(In reply to Andrew Sutherland [:asuth] from comment #4)
> We're running into the problem that our web-activities are a hodge-podge of
> convention with no documented semantics.
> 
> The gallery, music, and video apps use "allowSave" to indicate that they
> activity-caller wants to allow the provided Blob to be saved to
> DeviceStorage.  Gallery/friends do not seem to have a guard that disables
> this mechanism if the file is already persisted on DeviceStorage, so you
> would not want to pass { allowSave: true } if the Blob is already saved
> there.
> 
> It seems like the contacts app is also using "allowSave" to indicate that
> the activity-caller wants to allow the contact to be saved to mozContacts. 
> I don't understand the rationale for this.  Why would an activity-caller
> passing a Blob be able to make a decision about whether it's appropriate for
> its contents to be saved to mozContacts?  My best guess is that this was the
> simplest solution to address some contacts bug and someone noticed that SMS
> was passing { allowSave: true } in certain circumstances.
> 
> Unfortunately I don't entirely understand SMS's attachment-handling
> flow/Blob handling.  I see that it can trigger an 'open' activity from
> compose which will not pass { allowSave: true }, but thread_ui.js will pass
> it.  I do see that MozMmsMessage exposes Blobs, so maybe in maintains some
> type of storage separate from DeviceStorage?  In which case its use of
> allowSave seems 100% correct.

The rationale is that the blob you have in Compose comes from another app, and so the user won't want to save it (it's already saved elsewhere). On the contrary, the blob we have in ThreadUI comes from the network and the user may want to save it if he displays it.

That said, we could just as well always pass allowSave, and let the user choose.

From what I understand, "allowSave" is here as a hint to the activity handler that the activity caller wants a UX that lets the user save the blob. The activity handler can ignore it or use it.

I don't think I understand properly what you're saying about the Contacts app. Do you recommend that the Contacts app always allows saving a vcard even if allowSave is missing?

Comment 6

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

(In reply to Julien Wajsberg [:julienw] from comment #5)
> I don't think I understand properly what you're saying about the Contacts
> app. Do you recommend that the Contacts app always allows saving a vcard
> even if allowSave is missing?

Yes, or at least it should not be using "allowSave" for whatever its needs are.  For example, if this has to do with Facebook stuff, a different thing like { forbidSaveBecauseOfLicensing: 'facebook' } is much more explicit.  I think:

- "allowSave" really means "provide UI to save this Blob to DeviceStorage because it's not currently stored there".

- the "open" activity is analogous to double-clicking on a file on a traditional desktop computer.  In some cases that might be a temporary file (the user picked open and the browser used a temporary directory), or a permanent file (the user chose to save the file).  Arguably the file manager or web browser that launched the file should not be doing much more than providing a reference to the file and relevant meta-data to its origins (filename, MIME type).

Comment 7

[David Flanagan [:djf]]

I agree that allowSave doesn't seem necessary for the contacts app: it should always just give the user the choice to save or not save.

I was vocally opposed to the idea of the "open" activity displaying any kind of save button in the first place. I thought it should be the responsiblity of the email app and messaging app to decide when to save attachments to device storage and when to keep them in a private database.  But I was overruled on this, and when we had to add Save buttons to the open activities for the media apps, we also needed to add the allowSave flag for exactly the reason Andrew states: sometimes the open activity is for a blob that is already saved to device storage (after bluetooth download, for example) and it wouldn't make any sense to save another copy of it.  

Given that the contacts app is not an app that scans device storage looking for .vcf files to import, it does not have the same need for allowSave.

I'd say that this bug should be resolved by modifying Contacts to behave as if all activities had allowSave:true.  Unless Jose has some use case for why saving should not be allowed.

Comment 8

[Jose Manuel Cantera]

(In reply to David Flanagan [:djf] from comment #7)
> I agree that allowSave doesn't seem necessary for the contacts app: it
> should always just give the user the choice to save or not save.


Sorry David, there is at least one use case for which we do need such a functionality: When you are attaching an existing contact to an MMS, you might want to inspect the attachment and in that particular case it does not make sense to allow the user to save a contact that is already present on the device.

> 
> I was vocally opposed to the idea of the "open" activity displaying any kind
> of save button in the first place. I thought it should be the responsiblity
> of the email app and messaging app to decide when to save attachments to
> device storage and when to keep them in a private database.  But I was
> overruled on this, and when we had to add Save buttons to the open
> activities for the media apps, we also needed to add the allowSave flag for
> exactly the reason Andrew states: sometimes the open activity is for a blob
> that is already saved to device storage (after bluetooth download, for
> example) and it wouldn't make any sense to save another copy of it.  

A similar case than the one I've described above. 

> 
> Given that the contacts app is not an app that scans device storage looking
> for .vcf files to import, it does not have the same need for allowSave.

No, for the reasons state above. The Contacts App does not save vCards to the device storage but save them as mozContacts in the mozContacts database. 

> 
> I'd say that this bug should be resolved by modifying Contacts to behave as
> if all activities had allowSave:true.  Unless Jose has some use case for why
> saving should not be allowed.

Yes I have, as noted above.

Thanks for your explanations, David.

Comment 9

[Julien Wajsberg [:julienw]]

Hey José,

When we display a vcard, could you somehow know this contact is already saved?

IMO this makes sense anyway, from an UX point of view, if we'd display such an indication.

Comment 10

[Jose Manuel Cantera]

If we are in draft mode perhaps we could send a parameter from the MMS App to the Contacts activity so that 
We would always allow to 'import' the contact unless such a parameter is present. 

WDYT?

Comment 11

[Julien Wajsberg [:julienw]]

I don't see the difference between the existing allowSave and this new parameter :/

What would be the downside of always showing the "save" button? Because Contacts knows how to dedupe...

Comment 12

[Jose Manuel Cantera]

(In reply to Julien Wajsberg [:julienw] from comment #11)
> I don't see the difference between the existing allowSave and this new
> parameter :/
> 
> What would be the downside of always showing the "save" button? Because
> Contacts knows how to dedupe...

if UX agree on that solution. I will be happy as well, ni Carrie to confirm.

Comment 13

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

Hi, 

I don't really understand from comment 1 to comment 10, but if we are talking about not having any way to save a vcard downloaded from Email, then adding a "save" button could work. Can you please provide a screenshot of this page for me? (can't access to that step from my device) so that I can provide the correct string and also ni? visual input if we need it. Thanks!

Comment 14

[Noemí Freire (:noemi)]

(In reply to Carrie Wang [:carrie] from comment #13)
> Hi, 
> 
> I don't really understand from comment 1 to comment 10, but if we are
> talking about not having any way to save a vcard downloaded from Email, then
> adding a "save" button could work. Can you please provide a screenshot of
> this page for me? (can't access to that step from my device) so that I can
> provide the correct string and also ni? visual input if we need it. Thanks!

Hi,

Currently when a vcard downloaded from Email app is opened there is no way to save the contact (except of adding it as favorite), please see "Current_Contact_Import_From_Email.png" screenshot as reference. 
In the last proposal raised in comment 11, the idea is that when a vcard attachment is open the user will be presented with the add contact screen and he will be able to add it. If the contact already exists the duplicates window will appear. Notice that it will behave like this in all the scenarios the contact activity is called (no matter from which app or flow). Please see the current flow to import a vcard from messaging app as reference.

Comment 15

[Noemí Freire (:noemi)]

Attachment: Current_Contact_Import_From_Email.png

Comment 16

[Maria Angeles Oteo (:oteo)]

Attachment: Current_Contact_Import_From_MMS.png

Carrie, here you have the screenshot relative to the current implementation of importing a vcard from messaging app (master and 2.2).

When vcard attachment is open the user can see the "add contact" screen so he will be able to add it to his Agenda, the idea is doing the same when receiving the vcard via E-mail application as Noemí has explained in comment 14.

Comment 17

[Maria Angeles Oteo (:oteo)]

Nominating to 2.2 so the user can save the received and previewed vcard via E-mail application in the same way that he can already do it via Messaging application in 2.2.

Comment 18

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

I'm not happy implementing a save option in the open activity at this stage of 2.2, definitely agree that is the solution we should go for 3.0, but doing this 2 weeks before FC is like adding a new feature

Comment 19

[Maria Angeles Oteo (:oteo)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #18)
> I'm not happy implementing a save option in the open activity at this stage
> of 2.2, definitely agree that is the solution we should go for 3.0, but
> doing this 2 weeks before FC is like adding a new feature

It would be offering the same behaviour when receiving a vcard via e-mail than the one we are already providing in 2.2 when receiving a vcard from Messaging application, where we are giving the user the option for saving the contact (in master and in 2.2)

The idea is that both applications (e-mail and Messaging) behave in the same way when receiving a vcard in master and 2.2. Now the behaviour is different in both branches.

Comment 20

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

triage: non-blocking. we should have this done before FL, but now is not a good timing.

Comment 21

[GitHub Autolander Bot]

Attachment: [gaia] ADLR-es:always-allow-save > mozilla-b2g:master

Comment 22

[Jose Manuel Cantera]

Comment on attachment 8574710 [details] [review]
[gaia] ADLR-es:always-allow-save > mozilla-b2g:master

LGTM

Comment 23

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/7946dbaa22684bf60ff4536bab0fd15ea2a668e0

Comment 24

[Noemí Freire (:noemi)]

Please find below a few scenarios checked with master build once this patch landed
Flame master device
Gecko: 014849c
Gaia: 7946dba
Platform version: 39.0a1

1a- Save a contact from a vcard received via E-mail app --> download vcf file, click on the view icon, add contact window is shown with all the contact details, clicking on Done button properly save the contact
1b- duplicate contact scenario --> i.e, having the previous contact already saved, click again on the view icon from E-mail app -->  add contact window is shown with all the contact details, clicking on Done button, "Duplicates found" window is shown (users can merge or cancel)
2a- Save a contact from a vcard received via Messages app --> click on the attachment icon, add contact window is shown with all the contact details, clicking on Done button properly save the contact
2b- duplicate contact scenario --> i.e, Having the previous contact already saved, click again on the attachment icon from Messages app --> add contact window is shown with all the contact details, then click on Done button, "duplicates found" window appears (users can merge or cancel)
3- Save a contact from the download notification (vcard download) --> click on the notification, add contact window is shown with all the contact details, clicking on Done button properly save the contact
4a- Save a contact from a vcard received via DM --> click on the vcf entry, add contact window is shown with all the contact details, clicking on Done button properly save the contact
4b- duplicate contact scenario --> from DM download list click again on the vcf entry, add contact window is shown with all the contact details, then click on Done button, "duplicates found" window appears (users can merge or cancel)
A pair of circular test cases:
5- From Messages app, click on new message -> attach a contact, once attached, click on it to view it ->  add contact window is shown, then click on Done button, "duplicates found" window appears (users can merge or cancel)
6- From Contact app, click on a contact -> contact details screen appears -> share contact via MMS ->once the contact is attached click on it -> view -> add contact window is shown, then click on Done button, "duplicates found" window appears (users can merge or cancel)

all the test cases work as expected

Comment 25

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

(In reply to Noemí Freire (:noemi) from comment #24)
> Please find below a few scenarios checked with master build once this patch
> landed
> Flame master device
> Gecko: 014849c
> Gaia: 7946dba
> Platform version: 39.0a1
> 
> 1a- Save a contact from a vcard received via E-mail app --> download vcf
> file, click on the view icon, add contact window is shown with all the
> contact details, clicking on Done button properly save the contact
> 1b- duplicate contact scenario --> i.e, having the previous contact already
> saved, click again on the view icon from E-mail app -->  add contact window
> is shown with all the contact details, clicking on Done button, "Duplicates
> found" window is shown (users can merge or cancel)
> 2a- Save a contact from a vcard received via Messages app --> click on the
> attachment icon, add contact window is shown with all the contact details,
> clicking on Done button properly save the contact
> 2b- duplicate contact scenario --> i.e, Having the previous contact already
> saved, click again on the attachment icon from Messages app --> add contact
> window is shown with all the contact details, then click on Done button,
> "duplicates found" window appears (users can merge or cancel)
> 3- Save a contact from the download notification (vcard download) --> click
> on the notification, add contact window is shown with all the contact
> details, clicking on Done button properly save the contact
> 4a- Save a contact from a vcard received via DM --> click on the vcf entry,
> add contact window is shown with all the contact details, clicking on Done
> button properly save the contact
> 4b- duplicate contact scenario --> from DM download list click again on the
> vcf entry, add contact window is shown with all the contact details, then
> click on Done button, "duplicates found" window appears (users can merge or
> cancel)
> A pair of circular test cases:
> 5- From Messages app, click on new message -> attach a contact, once
> attached, click on it to view it ->  add contact window is shown, then click
> on Done button, "duplicates found" window appears (users can merge or cancel)
> 6- From Contact app, click on a contact -> contact details screen appears ->
> share contact via MMS ->once the contact is attached click on it -> view ->
> add contact window is shown, then click on Done button, "duplicates found"
> window appears (users can merge or cancel)
> 
> all the test cases work as expected

What happen if we have multiple contacts in the vcard in those cases?

Comment 26

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

Comment on attachment 8574710 [details] [review]
[gaia] ADLR-es:always-allow-save > mozilla-b2g:master

Pretty simple change, easier than anticipated. Just waiting to know what happen in the scenarios of a vcard with multiple contacts.

Comment 27

[Maria Angeles Oteo (:oteo)]

Hi Francisco,
we have just tested the scenarios when receiving a vcard with multiple contacts via E-mail application, Messaging application and also from Downloads. The behavior is the same for all of them, when the multiple contacts vcard is opened, the Import screen is shown with all the Contacts included in the multiple Vcard. 

If user has the option to import them, all of them are included in the Agenda (we have already tested with vcards with more that 10 contacts to ensure that the scroll and the importing is working fine).

In case of duplicated contacts scenario, the passive merging is applied.

Note: just fyi we have detected a visual issue after importing the contacts from Downloads that has already been reported in System in bug 1142185.

Comment 28

[Maria Angeles Oteo (:oteo)]

After deep testing of the patch in master and after ensuring with Francisco that all the possible scenarios are covered and the fix is working fine (in order to avoid any regression in 2.2), we consider that the patch is safe enough to be uplifted to 2.2.

Nominating to 2.2 again for the same reasons explained in Comment 17, to allow user to save the received and previewed vcard via E-mail application in the same way that he can already do it via Messaging application in 2.2.

Comment 29

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

Hey guys, 


Sorry that I missed this one. I thought I've already replied the bug but apparently I missed the last step of sending the comment! 
Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=1134995
This one is dealing with single/ multiple vcard imported from Messaging. 
I think we can adopt the same approach fro Email. Thanks!

Comment 30

[Johan Lorenzo [:jlorenzo]]

Triage: Simple patch, tested by Telefónica. Agreed to consider it as a patch we need in 2.2.

Comment 31

[Adrián de la Rosa]

Comment on attachment 8574710 [details] [review]
[gaia] ADLR-es:always-allow-save > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Opening vCards from mail app
[User impact] if declined: Users won't be able to import a vCard they received from mail, just see its content.
[Testing completed]: Multiple scenarios tested by Telefónica, tests modified to fit this patch.
[Risk to taking this patch] (and alternatives if risky): The change is pretty small, low risk.
[String changes made]: None.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.2 uplift.

Comment 33

[GitHub Autolander Bot]

Attachment: [gaia] ADLR-es:always-allow-save-2.2 > mozilla-b2g:v2.2

Comment 34

[Adrián de la Rosa]

Comment on attachment 8580071 [details] [review]
[gaia] ADLR-es:always-allow-save-2.2 > mozilla-b2g:v2.2

The rebase had conflicts because the code which needed to be changed was in a different file in v2.2. Submitting the change to review again, this time to merge into v2.2.

Comment 35

[Jose Manuel Cantera]

Comment on attachment 8580071 [details] [review]
[gaia] ADLR-es:always-allow-save-2.2 > mozilla-b2g:v2.2

LGTM, thanks Adrian!

Comment 36

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/28983

Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/403fc898fbfa5ae58a8d43a4145ed358002b6f65

Comment 38

[Beatriz Rodríguez [:brg]]

Testing today with Flame v2.2(Gecko-c768752.Gaia-c8136ef), after downloading the vcard attachment in a mail, it is opened by Contacts app detail panel and user can choose "Done" (upper right corner) to add contact to Contact list.
Thanks a lot for the improvement.

Comment 39

[Shally Li（leave from Mozilla）]

This bug has been verified as "pass" on latest Nightly build of Flame v2.2&3.0 by the STR in Comment 0.

Actual results: 
On Flame v2.2, it can tap "Done" to save the contact from a vcard (single contact vcard) received via E-mail app, and can merge/cancel the duplicate contact.
On Flame v3.0, it can save the contact from a vcard (single contact /multiple contacts vcard) received via E-mail app, and can merge/cancel the duplicate contact.

See attachment: verified_v2.2.mp4 and verified_v3.0.mp4
Reproduce rate: 0/10(v2.2&3.0)


Device: Flame v2.2 build(Pass)
Build ID               20150607002503
Gaia Revision          8fc797527a3eca7665bc1d1828848f2fb77ca99f
Gaia Date              2015-06-04 07:46:11
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d213237e11e9
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150607.035848
Firmware Date          Sun Jun  7 03:58:59 EDT 2015
Bootloader             L1TC000118D0

Device: Flame v3.0 build(Pass)
Build ID               20150607160204
Gaia Revision          1d62b32408567f9f7cf1c71c1e5a0c6593be757b
Gaia Date              2015-06-05 17:55:07
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/7d4ab4a9febd
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150607.193246
Firmware Date          Sun Jun  7 19:32:58 EDT 2015
Bootloader             L1TC000118D0

Comment 40

[Shally Li（leave from Mozilla）]

Attachment: verified_v3.0.mp4

Comment 41

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2.mp4