974589 - [contacts] Contacts needs "open" activity to support text/vcard because bug 887663 added it to the MimeMapper so email thinks they are downloadable

Status: VERIFIED FIXED

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

Description

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

In Bug 887663 (receive contacts via bluetooth), "text/vcard" was added to MimeMapper.  The e-mail app uses/introduced MimeMapper to know what MIME types have "open" web activities associated with them (as done in gallery/video/music).  The idea is that if there is an "open" activity, we can download the attachment and expose a "View" button in the e-mail UI which then triggers an "open" activity when pressed.  (Our choice of names for activities is obviously not great in general :)

Looking at the comments in mime_mapper.js, I can see that that the comments really are not sufficiently clear about the implications of adding a MIME type to the entries there.  There are hints if you already know what's going on, but too much of a light touch was used.

The implications of not exposing the open activity are that a user will be able to download a vcard in the e-mail app but not be able to view it because there is no webactivity available.

The e-mail app currently generates a WebActivity like this:
          var activity = new MozActivity({
            name: 'open',
            data: {
              type: mappedType,
              blob: blob
            }
          });
https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/message_reader.js#L612
(In the future a filename will be passed in the data too for consistency with what gallery does, although the blob in question is actually a File and so does have a "name" on it.)

The logic in contacts/js/activities.js will need to be updated slightly since "open" currently is used only to "view-contact-details" using a mozContacts id and a made-up MIME type.  It can pivot on the MIME type or presence of a blob?  Once it does that, it looks like importContactFromFile(activity) will work directly.


I'm requesting blocking 1.3 to get initial feedback on whether this is a fix that would be desired on 1.3.  We already shipped v1.2 with this and vCards are small files, so it's not the end of the world if we don't uplift it.  The fix does look to be trivial.

I'll file a separate bug and provide a fix for mime_mapper.js and its comments.  (Noting that when e-mail integrates with the download manager, which could happen in 1.4 if partners provide the engineering, this could become slightly moot.  Or at least it will then be the download manager who is being betrayed by mime_mapper.js instead :)

Comment 1

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

IMHO, not a blocker, but something to add as soon as possible.

The implementation shouldn't be really hard, since we already have a web activity to create new contact, perhaps we could reuse this one, parsing the vcard and showing the option of create a new one with the fields already filled with the vcard information.

Comment 2

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

In fact, now that I'm remembering, we already open a vcard when we receive it via bluethooth, so adapting it to receive the vcard from the web activity should be easy.

Comment 3

[Jason Smith [:jsmith]]

Agree with comment 1. Not blocking.

Comment 4

[Adrián de la Rosa]

If we want to just open the vcard without importing it first, we will need to wait for https://bugzilla.mozilla.org/show_bug.cgi?id=1059715 to merge. I haven't been able to test it but I suppose that when transferring contacts with Bluetooth they are automatically imported too, so maybe it's not what we need here.

Comment 5

[Adrián de la Rosa]

I haven't been able to use bluetooth with my Flame and the latest version, but I shared a contact from Android to an Hamachi with 1.3 and the contact was imported to my contacts list with no option to check it first. After experimenting with the code, I think that to solve this bug we have 2 choices: we can make vcards from mail work the same as vcards from bluetooth and import them as soon as you open them, or we can work in a new view for contacts that shows all contacts from the vcard and lets you choose which ones you want to import. In fact, the second choice seems more like a new feature than a way of fixing this bug.

Anyway, I'm not the one that decides how this have to be fixed, so what do you think, Francisco?

Comment 6

[Jose Manuel Cantera]

The way to proceed is as follows:

If we have a vcard with 1 contact, we will open the new contact activity passing the vcard content as parameter, then the user will be able to import. 

If we have a vCard with multiple contacts we will display a list of contacts and a button to import them.

Comment 7

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

(In reply to Jose Manuel Cantera from comment #6)
> The way to proceed is as follows:
> 
> If we have a vcard with 1 contact, we will open the new contact activity
> passing the vcard content as parameter, then the user will be able to
> import. 
> 
> If we have a vCard with multiple contacts we will display a list of contacts
> and a button to import them.

+1 to Jose here, we will need the work on bug 1059715

Comment 8

[Adrián de la Rosa]

Attachment: Diff from branch with prototype

I've implemented a partial solution based on the not yet finished API from bug 1059715. It's partial because right now it would only handle vCards with one contact. I would like to know if I'm on the right path.

I can continue doing some more work here but I'm blocked by that bug.

Comment 9

[Adrián de la Rosa]

I've updated the WIP in my branch but it's not working because I get this error:

[JavaScript Error: "TypeError: data.replace is not a function" {file: "app://communications.gaiamobile.org/shared/js/contacts/import/utilities/vcard_reader.js" line: 30}]

I am using the code from https://github.com/mozilla-b2g/gaia/pull/24821/files, which I know it hasn't even landed and is in development, but maybe this helps.

The execution doesn't arrive to the line with the 'console.log("Before onsuccess")', and it can be related.

Comment 10

[Jose Manuel Cantera]

Comment on attachment 8500463 [details]
Diff from branch with prototype

cancelling feedback until we have another version

Comment 11

[Adrián de la Rosa]

Comment on attachment 8500463 [details]
Diff from branch with prototype

I've updated my branch again. This time, everything works for vCards with one contact. I've also started an integration test for this bug but I haven't been able to do much progress on it because integration tests are broken in my computer right now.

I would like to receive feedback to see whether you think this is a good approach or what can be improved.

I have yet to wait for Bug 1059715 to land but meanwhile I will continue with the tests and thinking about handling vCards with multiple contacts.

Comment 12

[Jose Manuel Cantera]

Comment on attachment 8500463 [details]
Diff from branch with prototype

I don't like some parts of the code neither the overall solution

Comment 13

[Jose Manuel Cantera]

I have left the comments here. Please in the future do not attach diffs but directly a PR, otherwise I won't be able to comment. 

https://github.com/mozilla-b2g/gaia/pull/24920/files

Comment 14

[Adrián de la Rosa]

Attachment: Pull request #24920

Much more clean code now thanks to all the comments. The approach has been changed as suggested.

Comment 15

[Jose Manuel Cantera]

Comment on attachment 8501752 [details]
Pull request #24920

thanks Adrian. Please ask for a review once the vcard reader component has landed and you have rebased your code

Comment 16

[Adrián de la Rosa]

Comment on attachment 8501752 [details]
Pull request #24920

Code ready to land, but it is missing some things. First, integration tests. I have a test that I haven't been able to try because I am having problems setting up my integration test environment, so I removed the test from the PR for the moment. Then, vCards with multiple contacts are not supported yet because it would mean to create a new view, which is a big change that can break things, so we decided to do it in a follow up bug. Also, we have to wait for the vcard reader PR to land so I can rebase this PR on that commit.

Comment 17

[Jose Manuel Cantera]

(In reply to Adrián de la Rosa from comment #16)
> Comment on attachment 8501752 [details]
> Pull request #24920
> 
> Code ready to land, but it is missing some things. First, integration tests.
> I have a test that I haven't been able to try because I am having problems
> setting up my integration test environment, so I removed the test from the
> PR for the moment. Then, vCards with multiple contacts are not supported yet
> because it would mean to create a new view, which is a big change that can
> break things, so we decided to do it in a follow up bug. Also, we have to
> wait for the vcard reader PR to land so I can rebase this PR on that commit.

thanks Adrian. once the dependencies land I will review and we will land this

Comment 18

[Jose Manuel Cantera]

Comment on attachment 8501752 [details]
Pull request #24920

cancelling review until I talk to Adrian

Comment 19

[Adrián de la Rosa]

Comment on attachment 8501752 [details]
Pull request #24920

Asking for review since we already talked about the last small change I did. We have to wait for the tests to pass but everything is working in local afaik.

Comment 20

[Jose Manuel Cantera]

Comment on attachment 8501752 [details]
Pull request #24920

thanks Adrian.

landed in master:

https://github.com/mozilla-b2g/gaia/commit/280f8e33e6810e6cf43ffbae4b4b530fdbd54cd0

Comment 21

[Warren WU]

[Blocking Requested - why for this release]:

Comment 22

[Johan Lorenzo [:jlorenzo]]

(In reply to [e-mail] from comment #21)
> [Blocking Requested - why for this release]:

Please explain why you think we can't ship 2.0 with that bug (https://wiki.mozilla.org/B2G/Triage#Blocker_Triage_Guidelines).

Comment 23

[Warren WU]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #22)
> (In reply to [e-mail] from comment #21)
> > [Blocking Requested - why for this release]:
> 
> Please explain why you think we can't ship 2.0 with that bug
> (https://wiki.mozilla.org/B2G/Triage#Blocker_Triage_Guidelines).

we have a o2 Germany bug about SMS or MMS with V-Card attachment is not supported,see bug 1030677,974276

Since this bug is fixed,So we hope to solve this in 2.0,thanks!

Comment 24

[Wesly Huang (EPM)]

Hi wuww, would you cherry-pick the fix to your SW? thanks.

Comment 25

[Johan Lorenzo [:jlorenzo]]

Verified with the following steps:

1. In email app, open an email that has attached a vCard containing only 1 contact.
2. Tap on the download button
3. Tap on the view button
4. The "Add contact" activity is now open, tap the done button

The contact is now added.

I checked with multiple contacts, the error message "Not enabled yet" popped. But it's another feature in bug 849729.

Comment 26

[Wesly Huang (EPM)]

[Blocking Requested - why for this release]:

[Traige] after discussion we nom. to 2.1, partner can pick the fix for 2.0 since it's available.

Comment 27

[bhavana bajaj [:bajaj]]

(In reply to Wesly Huang from comment #26)
> [Blocking Requested - why for this release]:
> 
> [Traige] after discussion we nom. to 2.1, partner can pick the fix for 2.0
> since it's available.

(In reply to [e-mail] from comment #23)
> (In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #22)
> > (In reply to [e-mail] from comment #21)
> > > [Blocking Requested - why for this release]:
> > 
> > Please explain why you think we can't ship 2.0 with that bug
> > (https://wiki.mozilla.org/B2G/Triage#Blocker_Triage_Guidelines).
> 
> we have a o2 Germany bug about SMS or MMS with V-Card attachment is not
> supported,see bug 1030677,974276
> 
> Since this bug is fixed,So we hope to solve this in 2.0,thanks!

Although this is fixed we cannot land non-ship blockers to the release at this point given the risk here, so this will get fixed in 2.2.

Comment 28

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Is there any special reason why this issue didn't need tests? If there isn't, could we do a follow up bug to add them?

Comment 29

[Jose Manuel Cantera]

integration tests were done in another bug. Adrian has the details

Comment 30

[Adrián de la Rosa]

Yup, tests were made and landed in https://bugzilla.mozilla.org/show_bug.cgi?id=1087402 , sorry for not making it clearer.

Comment 31

[Shally Li（leave from Mozilla）]

Per comment 25,clear "verifyme".