981674 - [Contacts] Contact's name merged to another one previously then exported to sd card and imported again, is not shown fine

Status: VERIFIED FIXED

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

Description

[isabelrios]

Buri master build (03/10)
Gecko-3f542fd
Gaia-e91db4c

STR
1. Create a contact ie: Name1, LN1 and a valid phone number: 611223344
2. Create another contact: Name2, LN2 and the same phone number: 611223344
3. Merge both contact
4. Export that contact to SD Card
5. Remove the contact from the addressbook
6. Import the contact from the SD Card

EXPECTED
The contact is correctly imported from the sd card

ACTUAL
After importing from the sd card, the contact appears in the contact list as:
Name: Name2, Name1
Last Name: LN2, LN1
Phone Number: 611223344

Comment 1

[isabelrios]

Fyi, v1.2 and v1.3 are also affected

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

triage: minus, not blocking. please fix this on trunk .thanks

Comment 3

[Michał Budzyński (:michalbe)]

It's probably affected by the same part of the contacts merges as Bug 935636, since I probably know how to fix the previous one, I would like to work on this one as well.

Comment 4

[Fernando Campo (:fcampo)]

Attachment: Link to PR - https://github.com/mozilla-b2g/gaia/pull/20238

Quick patch (1 line!).

Problem is that when we merge we keep all the data (previous and current) on an array. Then we export to vcard, which doesn't accept arrays (we use comma separated), and when we import again from vcard, we didn't have this situation in mind.
So fix is easy, split the strings for the NAME PARTS from the vcard when importing.

Which I'm not sure of is if we should use this same approach for the rest of the fields. Advice?

P.S. I'm getting some tests for this specific situation, update if confirm that the approach is correct.

Comment 5

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

Can I haz unit test?

Comment 6

[Fernando Campo (:fcampo)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #5)
> Can I haz unit test?

(In reply to Fernando Campo (:fcampo) from comment #4)
> Created attachment 8436995 [details] [review]
> Link to PR - https://github.com/mozilla-b2g/gaia/pull/20238
> P.S. I'm getting some tests for this specific situation, update if confirm
> that the approach is correct.

:D

Comment 7

[Jose Manuel Cantera]

Fernando,

Looks good. We should do the same with the givenName and familyName fields. 

And please be careful with rebasing you were breaking the recently landed duplicates contacts notification

thanks

Comment 8

[Fernando Campo (:fcampo)]

Comment on attachment 8436995 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/20238

PR updated, test added

Comment 10

[Jose Manuel Cantera]

Comment on attachment 8436995 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/20238

thanks Fernando, good work

Please land once you get a Green Travis

Comment 11

[Fernando Campo (:fcampo)]

green green, merged merged - b85f072304d7b3eead596100182fd1b1f1adc421

Comment 12

[Loli (:lolimartinezcr)]

Tested and working
Hamachi
2.1
Gecko-42ab217
Gaia-56812bb

is this bug going to resolve in 2.0?

Comment 13

[Fernando Campo (:fcampo)]

Attachment: request for approval

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 #): no regression, always've been like this
[User impact] if declined: import previously merged contact looks bad
[Testing completed]: unit tests plus testing on device
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]:

Comment 14

[bhavana bajaj [:bajaj]]

Comment on attachment 8439070 [details]
request for approval

Thanks for the unit tests and master verification on this bug, looks low risk to get uplifted to 2.0 given where we are in the cycle, so approving this change.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/1800fb7270de15a75e24f05cfff3ec13bf168d97

Comment 16

[Loli (:lolimartinezcr)]

Tested and working
Hamachi
2.1
Gecko-a3c4c53
Gaia-5f9f64a

2.0
Gecko-3bfc06b
Gaia-5e14503

Comment 17

[Shally Li（leave from Mozilla）]

This issue has been successfully verified on Flame 2.1&2.0.
See attachment: verified_v2.1.mp4.
Reproducing rate: 0/5

Flame 2.0 build:
Gaia-Rev        f9d6e3d83c3922e9399a6c27f5ce4cdd27bdfd05
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/45112935086f
Build-ID        20141126000203
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.032754
FW-Date         Wed Nov 26 03:28:05 EST 2014
Bootloader      L1TC00011880

Flame 2.1 build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.033519
FW-Date         Wed Nov 26 03:35:30 EST 2014
Bootloader      L1TC00011880

Comment 18

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.1.MP4