Import contacts from memory card in VCF (vCard) format

RESOLVED FIXED

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sergi, Assigned: sergi)

Tracking

({feature})

Dependency tree / graph
Bug Flags:
in-moztrap ?

Firefox Tracking Flags

(blocking-b2g:-, b2g18+ verified)

Details

Attachments

(6 attachments)

The user should be able to import vcards from the memory card into the Contacts app. Vcard is the standard in contact format and most feature phones export to it. Firefox OS should be able to import these contacts easily from the Contacts app and in the FTU.
Posted file Github PR URL
Attachment #738439 - Flags: review?(arcturus)
Attachment #738439 - Flags: review?(anygregor)
Attachment #738439 - Flags: review?(arcturus) → review?(francisco.jordano)
Blocks: 838121
You can test easily with real-world examples of multiple VCF by exporting from http://contacts.google.com in case you have a gmail account.
Comment on attachment 738439 [details] [review]
Github PR URL

r+, check the functionality and the look and feel and looks awesome.

Please merge once the rebased and squashed :)
Attachment #738439 - Flags: review?(francisco.jordano) → review+
Nominating as leo? since the functionality closes the story of importing contacts.
blocking-b2g: --- → leo?
Comment on attachment 738439 [details] [review]
Github PR URL

The new pull request rebased and squashed here: https://github.com/mozilla-b2g/gaia/pull/9338
Attachment #738439 - Flags: review?(anygregor)
I think before landing this code we would need some review by the UX team. Particularly I'm not convinced we should add a new option to the Contacts Settings screen. Perhaps import from vCArd and SDCard is something could be implemented by an independent app. 

In any case I think this needs a review from UX both from an interaction and visual perspective.
Flags: needinfo?(sergiov)

Updated

6 years ago
Flags: needinfo?(aymanmaat)
(In reply to Jose M. Cantera from comment #10)
> I think before landing this code we would need some review by the UX team.
> Particularly I'm not convinced we should add a new option to the Contacts
> Settings screen. Perhaps import from vCArd and SDCard is something could be
> implemented by an independent app. 
> 
> In any case I think this needs a review from UX both from an interaction and
> visual perspective.

We requested UX feedback to Sergi, as I couldn't find Ayman past friday. Anyway sounds good to me if Ayman takes a look to it.

Thanks,
F.

Comment 12

6 years ago
I think it looks good as it's currently implemented and makes sense to do it from the contact settings screen. I'm not sure about creating a new app to import contacts, away from the place you manage them, basically the contacts app. Anyway, that's my opinion. You can check it with Ayman.
Flags: needinfo?(sergiov)
(In reply to Sergi from comment #12)
> I think it looks good as it's currently implemented and makes sense to do it
> from the contact settings screen. I'm not sure about creating a new app to
> import contacts, away from the place you manage them, basically the contacts
> app. Anyway, that's my opinion. You can check it with Ayman.

Agreed. An independent app for a transaction like this is a non-starter. Ayman should review and chime in on this.
This sounds more like a nice-to-have, very valuable new feature so we won't block here but will track and consider an uplift nomination to v1-train if low risk and very independent of other code.
blocking-b2g: leo? → -
tracking-b2g18: --- → +

Comment 15

6 years ago
Hey guys

Ok I have checked this and it looks all good to me.

Ayman
Flags: needinfo?(aymanmaat)
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Comment on attachment 738439 [details] [review]
Github PR URL

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):
String or UUID changes made by this patch:

Final Github Pull request: https://github.com/mozilla-b2g/gaia/pull/9338
Attachment #738439 - Flags: approval-gaia-v1?
this broke the linter, please fix the errors.

see https://travis-ci.org/mozilla-b2g/gaia/builds/6634027 for example

Next week we'll start backing out patches because of such errors, so take care :)

Also, for your next commits, please put the bug number in the commit log, this is easier to find bugs from the commit hash.

And also put the commit hash in the comments next time, like that :

master: 957057ef791be7c1a30cb127e90915bb0ef98f3a
Thanks Julien, will do. I am still learning bugzilla bureacracy.

I am working on fixing linter error and will commit asap.
And you should fill in the approval form too ;) otherwise the triagers will reject the approval.
Attachment #738439 - Flags: approval-gaia-v1?
Attachment #741921 - Flags: review?(francisco.jordano)
I've put some comments on the PR already
(In reply to Julien Wajsberg [:julienw] from comment #21)
> I've put some comments on the PR already

Thanks, I went through them and updated the PR.
I've put some more comments, all only test-related, Francisco should review what the code is doing because I have no clue ;)
Comment on attachment 738439 [details] [review]
Github PR URL

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 862742
User impact if declined: Users won't be able to import contacts from SDCard in vCard format. This affects mainly users of feature phones with systems such as Nokia S40, where the main way to extract contacts is through vCard exporting using bluetooth or SDCard. In both cases, importing vCard files from SDCard solves the issue. Countries like Serbia depend highly on vCard-based systems.
Testing completed: Basic UI and import testing included 
Risk to taking this patch (and alternatives if risky): This is a low-risk patch. UI flows are tested and verified.
String or UUID changes made by this patch: Yes. Only in  apps/communications/contacts/locales/contacts.en-US.properties. See https://github.com/mozilla-b2g/gaia/pull/9338/files#diff-6.
Attachment #738439 - Attachment description: Githup PR URL → Github PR URL
Attachment #738439 - Flags: approval-gaia-v1?
Comment on attachment 741921 [details] [review]
Github PR fixing linter and unit test errors

r+

agree with comments by Julien, but ok to do it on a follow up.
Attachment #741921 - Flags: review?(francisco.jordano) → review+
Comment on attachment 738439 [details] [review]
Github PR URL

Adding qawanted to get some additional testing around this beyond the basic UI flows - please stress-test this so that we know this (very much needed) additional functionality works well for the user.
Attachment #738439 - Flags: approval-gaia-v1? → approval-gaia-v1+
Depends on: 866135
Uplifted cbea03490518734515b7e6c1508759768342b18c to:
v1-train: a5fed32b0eddcd1354477c7da66e99f183463fa9
Sergi, please put the master commit for the linter+test fixes here, ask approval because the new patch touches some application code (I still didn't really understand why btw), and add needinfo in john ford to uplift when it gets the approval.
Flags: needinfo?(nobody)
Assignee: nobody → sergi.mansilla
Flags: needinfo?(nobody) → needinfo?(sergi.mansilla)

Updated

6 years ago
Depends on: 865999
Comment on attachment 741921 [details] [review]
Github PR fixing linter and unit test errors

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Request approval for this commit, as per Julien's advice, and because it touches application code

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 862742
User impact if declined: None, but tests will break the build.
Testing completed: Yes.
Risk to taking this patch (and alternatives if risky): No risk.
String or UUID changes made by this patch: None.
Attachment #741921 - Flags: approval-gaia-v1?
Flags: needinfo?(sergi.mansilla)
Attachment #741921 - Flags: approval-gaia-v1? → approval-gaia-v1+
need info John for uplifting the test fix
Flags: needinfo?(jhford)
There is a small merge conflict when I try to uplift this, which looks easy to resolve, but I'd like to make sure that it's OK:

++<<<<<<< HEAD
 +MockFb.getWorksAt = function(fbData) {
 +  return 'Telef<97>nica';
++=======
+ Mockfb.getWorksAt = function(fbData) {
+   return 'Telefónica';
++>>>>>>> a75b373... Merge pull request #9413 from comoyo/fix_sdcard_tests
Flags: needinfo?(jhford) → needinfo?(sergi.mansilla)
John, I think that's beause Bug 851476 was not uplifted. Could you do it as it's npotb ?
(and I sure hope it will be uplifted without conflicts)
Flags: needinfo?(sergi.mansilla) → needinfo?(jhford)
Follow up v1-train: 0d624b6c55094eaaa26318e1a3424cb8137f1580
Flags: needinfo?(jhford)
Keywords: qawantedverifyme
Keywords: feature
Sreenidhi to add a few moztrap testcases for this feature.
Flags: in-moztrap?(sreenidimn)

Updated

6 years ago
Depends on: 890752

Comment 36

6 years ago
I actually tested this case in leorun4, with all of the VCF contact versions.  

In my tests, All contact types are importing OK.
Attachment mime type: text/plain → text/x-github-pull-request
Attachment mime type: text/plain → text/x-github-pull-request

Comment 37

5 years ago
This bug no longer reproduces on buri 1.1 
Environmental Variables 
Device: Buri 1.1 MOZ
BuildID: 20140102041202
Gaia: 6ff3a607f873320d00cb036fa76117f6fadd010f
Gecko: bdac595a4e46
Version: 18.0
You need to log in before you can comment on or make changes to this bug.