885545 - Create Blobs for Contact photos in the VCF importer

Status: RESOLVED FIXED

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

Description

[Reuben Morais [:reuben]]

+++ This bug was initially created as a clone of Bug #878820 +++

The code currently saves them as strings. That doesn't work, and the strings will be ignored once bug 878820 is fixed. Blobs should be saved instead.

Comment 1

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

In VCF, the `photo` field can have a URI value (for example of a gif file in some website). What would you suggest we do in that case? We can ignore it or we can try to retrieve the file, although that would make imports extremely slow and probably it is not a good security practice.

Comment 2

[Reuben Morais [:reuben]]

How slow would it be, and how often do vCard exporters output URLs instead of a text representation of the image? I think we should support it unless it makes things unbearably slow (that would surprise me).

Comment 3

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

Well, the slowness would obviously depend on the network. In case there is no network we would not be able to retrieve the pictures, to start with. Also, if the user is importing a big amount of data with many remote pictures, that could be a considerable amount of bandwidth, which we can't really assume the connection can handle.

Comment 4

[Reuben Morais [:reuben]]

That's a good point about the bandwidth. I think we can ignore URLs for now.

Comment 5

[Jose Manuel Cantera]

(In reply to Sergi Mansilla from comment #3)
> Well, the slowness would obviously depend on the network. In case there is
> no network we would not be able to retrieve the pictures, to start with.
> Also, if the user is importing a big amount of data with many remote
> pictures, that could be a considerable amount of bandwidth, which we can't
> really assume the connection can handle.

In that case pictures should downloaded in chunks as we do in other importers that follow a common architecture ...

Comment 7

[Germán Toro del Valle (:gtorodelvalle)]

Hi Sergi, shall this bug 970909 be considered a duplicate of this one? Thanks!

Comment 8

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

Hi Germán,

Yeah, it should be considered a duplicate.

Cheers!

Comment 10

[Germán Toro del Valle (:gtorodelvalle)]

Hi again, Sergi ;-) Could bug 961628 be another duplicate of this bug? If it is, please feel free to mark it or just confirm it and I'll do it :-) Thanks!

Comment 11

[Germán Toro del Valle (:gtorodelvalle)]

Forgot the need-info :-(

Comment 13

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

Hi Germán, I marked it as duplicate when I saw your message, and I forgot to clear the ni back then :P

Thanks!

Comment 14

[Jason Smith [:jsmith]]

We are not taking features at this point in the release, so not blocking.

Comment 15

[demerick]

This bug failed a test case in burirun1.4-1, but the tag was not added. Since it also fails burirun1.4-2 I am changing the tag to 'permafail'.

Comment 16

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

Attachment: Github Pull Request

This patch adds support for parsing vCard format photos in its standard base64 and URL forms. Besides the parsing process itself, the main change is that the vCard processing becomes callback based because of the potential asynchronous nature of retrieving photos from URLs.

This patch also addresses some minor bugs found along the way.

Comment 17

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

Comment on attachment 8395608 [details] [review]
Github Pull Request

Some comments on the PR, not blocking as I know that will be resolved ;)

Thanks for the amazing work, we are getting there with vcard!

Comment 18

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

Fixed reviewer's comments and commited to master at 7f7f77278c60bc76949bc34293464b0e305fc839

Comment 19

[Kevin Grandon :kgrandon]

This was backed out for causing bug 989306. Note the issue only reproduced when building with PRODUCTION=1. You should see this with: PRODUCTION=1 make reset-gaia, FTU will fail to load.

https://github.com/mozilla-b2g/gaia/commit/8ad27d2f7d8b26d5d7551f232ce4a3650bcc1503

Comment 20

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

Attachment: Github pull request

Requesting a review again.

The issue that made this patch to be backed out has been addressed. The problem was the use of 'require' in vcard_parser.js. `require` does not exist in this context and made everything crash. Now the file mimemapper.js is loaded in the settings.html file inside contacts, as it should be. Tested on the phone with both development and production builds, and checked that all unit tests pass as well.

Comment 21

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

Comment on attachment 8401883 [details] [review]
Github pull request

This is not mergeable probably cause bug 990002 landed.

We move a lot of the importers code to shared and that's the reason. Please flag me again for review when rebased (should be simple).

Thanks!

Comment 22

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

Attachment: Pointer to PR 18130

Had to rebase Sergi's code and also found some problems with the 'require'.

I already reviewd this, but since I had to change it a bit, will ask for a second opinion from Jose.

Comment 23

[Jose Manuel Cantera]

ETA for the review is tomorrow Apr 12

Comment 24

[Jose Manuel Cantera]

Comment on attachment 8404043 [details] [review]
Pointer to PR 18130

Hi Francisco,

I left several substantive comments on GH. I think we need another round

thanks!

Comment 25

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

Comment on attachment 8404043 [details] [review]
Pointer to PR 18130

Second round, tried to fix the appointed comments.

Thanks!

Comment 26

[Jose Manuel Cantera]

Comment on attachment 8404043 [details] [review]
Pointer to PR 18130

LGTM

The only issue is photo thumbnails but that will be done in a follow-up

thanks Francisco!

Comment 27

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

Thanks Jose for the review,

finally landed:

https://github.com/arcturus/gaia/commit/9987411704b802fb48f4e4645c7f7244f8847d6c