[Contacts] "undefined" as contact's last name after merging

VERIFIED FIXED in Firefox OS v1.3

Status

Firefox OS
Gaia::Contacts
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: gtorodelvalle, Assigned: gtorodelvalle)

Tracking

({regression})

unspecified
1.3 C3/1.4 S3(31jan)
x86
Gonk (Firefox OS)
regression

Firefox Tracking Flags

(blocking-b2g:1.3+, b2g-v1.3 fixed)

Details

Attachments

(1 attachment)

It seems there is some issue when merging contacts who only have given names when the final contact's given name is alphabetically before the one whose data is being merged.

TO REPRODUCE:
1) Create a contact with no last name and name (as for given or first name) "B" and phone number 666666666.
2) Create a contact with no last name and name (as for given or first name) "A" and phone number 666666666.
3) The duplicates found page shows up.
4) Click on the merge button.

OBSERVED:
5) The information is merged into the contact with name "A" but magically his last name appears as "undefined".

EXPECTED:
5) The information is merged into the contact with name "A" who ends up as name "A", no last name and phone number "666666666".
Ups, sorry, it seems the STR is a little bit more complex than the one I included in comment 1. The right one should be:

TO REPRODUCE:
1) Create a contact with no last name and name (as for given or first name) "B" and phone number 666666666.
2) Create a contact with no last name and name (as for given or first name) "A" and phone number 666666666.
3) The duplicates found page shows up.
4) Click on the merge button. Both contact are merged and only "B" is kept in the contact list.
5) Create a contact with no last name and name (as for given or first name) "B" and phone number 666666666.
6) The duplicates found page shows up.
7) Click on the merge button.

OBSERVED:
8) The information is merged into the contact with name "B" but magically his last name appears as "undefined".

EXPECTED:
8) The information is merged into the contact with name "B" who ends up as name "B", no last name and phone number "666666666".
New typo:
4) Click on the merge button. Both contact are merged and only "A" is kept in the contact list. IT IS "A" THE ONE WHO IS KEPT.

Comment 3

4 years ago
FYi, this is reproducible with 01/16 1.3 buri build:
Gecko-0f53789
Gaia-423326d
blocking-b2g: --- → 1.3?
David

Please check from contacts.
Flags: needinfo?(dscravaglieri)

Comment 5

4 years ago
Can we check if this is a regression?  Does it repro on 1.2?
Keywords: qawanted

Updated

4 years ago
QA Contact: mvaughan

Comment 6

4 years ago
This issue does not reproduce on the 01/21/14 1.2 build.

This issue appears to have started reproducing on the 10/25/13 1.3 build. The last working build seems to have been 10/18/13. On the builds between them, the user is unable to create a contact in the Contacts app.

- Works -
Device: Buri v1.3 MOZ RIL
BuildID: 20131018040217
Gaia: 18c2ab4b7b2d4cacee90fb85e97354142b2fad4a
Gecko: 855da6d8a327
Version: 27.0a1
Firmware Version: V1.2-device.cfg

- Broken -
Device: Buri v1.3 MOZ RIL
BuildID: 20131025100746
Gaia: afbf45f26a73b7cd5e0a831bea48087331975286
Gecko: 2f2a45f04e7c
Version: 27.0a1
Firmware Version: V1.2-device.cfg


Also in case it helps, this issue reproduces on the 01/21/14 Master (1.4) build.
Keywords: qawanted → regression
Assignee: nobody → gtorodelvalle
triage: 1.3+
blocking-b2g: 1.3? → 1.3+
since 1.3+ is the highest priority now, set target milestone, please make the change if it doesn't work. thanks
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
OK, so there is really weird stuff going on here :-O Let's see if I am able to explain it.

According to the STR in comment 1, the merged contact looks like this:
JSON.stringify(mergedContact): {"givenName":["B","A"],"familyName":[null],"photo":[],"bday":null,"adr":[],"tel":[{"type":["mobile"],"value":"666666666","carrier":""}],"email":[],"org":[],"category":[],"url":[],"note":[],"name":["B"]}

So the first thing to note here is the "familyName":[null] which I am currently investigating.

The point is that if we try to inspect that supposed array we get:
Array.isArray(mergedContact.familyName): true
mergedContact.familyName.length: 1
mergedContact.familyName.indexOf(null): -1
typeof mergedContact[aField][0]: undefined
mergedContact[aField][0]: undefined
mergedContact[aField][0] === null: false
typeof mergedContact[aField][1]: undefined
mergedContact[aField][1]: undefined

So, first WAAAT??? here regarding the length, indexOf() and strict equality results... :-O

Even further, as you can see here https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts_merger.js#L221 , before saving the merged contact using the Contacts API, some properties are referenced/copied. Among them, the 'familyName' one. After that referencing/copying, we observe this:
JSON.stringify(masterContact): {"id":"undefined","published":null,"updated":null,"bday":null,"anniversary":null,"sex":null,"genderIdentity":null,"adr":null,"email":null,"url":null,"impp":null,"tel":[{"type":["mobile"],"value":"666666666","carrier":""}],"name":["B"],"honorificPrefix":null,"givenName":["B"],"additionalName":[""],"familyName":["undefined"],"honorificSuffix":null,"nickname":null,"category":null,"org":null,"jobTitle":null,"note":null,"key":null}

This is, masterContact.familyName magically became ["undefined"] instead of [null] or whatever it is in mergedContact.familyName... Yes, this is our second WAAAT??? :-)

Although I have a very simple working patch for this concrete bug, I didn't want to attach it before we find out the final reasons of the observed behaviour.

I'll keep you posted.
Created attachment 8365043 [details]
15680.html

OK, so I talked to José Manuel and we decided to fix the issue in Gaia since this is a 1.3+ bug and create a new one for the "strange contact Array property copying" issue. The new bug is bug 963536

BTW, José Manuel, I also fixed some JSHint issues in contacts_merge.js.
Attachment #8365043 - Flags: review?(jmcf)
(In reply to gtorodelvalle from comment #9)
> Even further, as you can see here
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> js/contacts_merger.js#L221 , before saving the merged contact using the
> Contacts API, some properties are referenced/copied. Among them, the
> 'familyName' one. After that referencing/copying, we observe this:
> JSON.stringify(masterContact):
> {"id":"undefined","published":null,"updated":null,"bday":null,"anniversary":
> null,"sex":null,"genderIdentity":null,"adr":null,"email":null,"url":null,
> "impp":null,"tel":[{"type":["mobile"],"value":"666666666","carrier":""}],
> "name":["B"],"honorificPrefix":null,"givenName":["B"],"additionalName":[""],
> "familyName":["undefined"],"honorificSuffix":null,"nickname":null,"category":
> null,"org":null,"jobTitle":null,"note":null,"key":null}
> 
> This is, masterContact.familyName magically became ["undefined"] instead of
> [null] or whatever it is in mergedContact.familyName... Yes, this is our
> second WAAAT??? :-)

Assuming masterContact is a mozContact object there, this is expected. The real question is how we're getting that [null] that behaves like [undefined] but-not-really there.

> According to the STR in comment 1, the merged contact looks like this:
> JSON.stringify(mergedContact):
> {"givenName":["B","A"],"familyName":[null],"photo":[],"bday":null,"adr":[],
> "tel":[{"type":["mobile"],"value":"666666666","carrier":""}],"email":[],
> "org":[],"category":[],"url":[],"note":[],"name":["B"]}

Is mergedContact a mozContact object?
First off, thank you very much for your reply, Reuben ;-)

(In reply to Reuben Morais [:reuben] from comment #11)
> (In reply to gtorodelvalle from comment #9)
> > Even further, as you can see here
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> > js/contacts_merger.js#L221 , before saving the merged contact using the
> > Contacts API, some properties are referenced/copied. Among them, the
> > 'familyName' one. After that referencing/copying, we observe this:
> > JSON.stringify(masterContact):
> > {"id":"undefined","published":null,"updated":null,"bday":null,"anniversary":
> > null,"sex":null,"genderIdentity":null,"adr":null,"email":null,"url":null,
> > "impp":null,"tel":[{"type":["mobile"],"value":"666666666","carrier":""}],
> > "name":["B"],"honorificPrefix":null,"givenName":["B"],"additionalName":[""],
> > "familyName":["undefined"],"honorificSuffix":null,"nickname":null,"category":
> > null,"org":null,"jobTitle":null,"note":null,"key":null}
> > 
> > This is, masterContact.familyName magically became ["undefined"] instead of
> > [null] or whatever it is in mergedContact.familyName... Yes, this is our
> > second WAAAT??? :-)
> 
> Assuming masterContact is a mozContact object there, this is expected. The
> real question is how we're getting that [null] that behaves like [undefined]
> but-not-really there.
> 
If this is the expected behaviour, we are fine since my patch (attachment 8365043 [details]) solves the issue regarding getting [null] as the value for familyName (also for givenName ;-) ).

> > According to the STR in comment 1, the merged contact looks like this:
> > JSON.stringify(mergedContact):
> > {"givenName":["B","A"],"familyName":[null],"photo":[],"bday":null,"adr":[],
> > "tel":[{"type":["mobile"],"value":"666666666","carrier":""}],"email":[],
> > "org":[],"category":[],"url":[],"note":[],"name":["B"]}
> 
> Is mergedContact a mozContact object?
Nope, mergedContact is a plain object (see https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts_merger.js#L56).

So, are we fine once my patch lands since as you mention that it the expected behaviour when copying? Thanks!
Flags: needinfo?(reuben.bmo)

Comment 13

4 years ago
Comment on attachment 8365043 [details]
15680.html

r+ provided the nits are fixed

thanks Germán!
Attachment #8365043 - Flags: review?(jmcf) → review+
After discussing it with @jmcanterafonseca , we decided not to solve the nits since this is the "format" already used in other test cases of the affected file ;-)
Merged in master: https://github.com/mozilla-b2g/gaia/commit/ac717698a31dcefbcdd13574ebeb66ff7e7b57bf
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
status-b2g-v1.3: --- → affected
(In reply to gtorodelvalle from comment #12)
> So, are we fine once my patch lands since as you mention that it the
> expected behaviour when copying? Thanks!

Well, the real fix would be finding out where the data was coming from in the first place, or using mozContact everywhere so the data doesn't get there. With your fix, it looks like |familyName: ["foo", null]| could still happen? Also, what about |familyName: [""]|? Though I don't think losing the data in the second case matters since it shouldn't affect any UI behaviors in the Contact app.
Flags: needinfo?(reuben.bmo)

Comment 17

4 years ago
This is already fixed in master build (01/28):
Gecko-acf3242
Gaia-846c481

Will verify it when the fix is in v1.3
clear n?
Flags: needinfo?(dscravaglieri)
I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 ac717698a31dcefbcdd13574ebeb66ff7e7b57bf
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(gtorodelvalle)
(In reply to Reuben Morais [:reuben] from comment #16)
> (In reply to gtorodelvalle from comment #12)
> > So, are we fine once my patch lands since as you mention that it the
> > expected behaviour when copying? Thanks!
> 
> Well, the real fix would be finding out where the data was coming from in
> the first place, or using mozContact everywhere so the data doesn't get
> there. With your fix, it looks like |familyName: ["foo", null]| could still
> happen? Also, what about |familyName: [""]|? Though I don't think losing the
> data in the second case matters since it shouldn't affect any UI behaviors
> in the Contact app.

Hi Reuben, the data was not coming, we were creating it via this piece of code: |mergedContact.givenName[0] = givenName[0];| in cases where givenName[0] was undefined.
I included |givenName[0]| because in the coming code it is this value the one used and referenced. I didn't want to include |givenName.length > 0| to avoid cases such as [undefined, "foo"] which would pass the condition but provoke the issue reported in this bug.
Regarding the "" case, it is true that it would end up not being copied as |mergedContact.givenName[0]| but I guess that if we manage to get "" as givenName[0] we may have deeper problems than the one reported by this bug. If you would prefer me to control that case, please do not hesitate to mention it and I will create a follow-up.
Thanks!
Flags: needinfo?(gtorodelvalle)
Merged in v1.3: https://github.com/mozilla-b2g/gaia/commit/86f0489a9c02b1779f43f4edadbcae5e93b783f1

BTW, John, you may find many upcoming uplifts to v1.3 failing if those patches fix JSHint issues and update the build/jshint/xfail.list which does not exist in v1.3 and was included in master. This patch was one of those cases. Just to make you aware of that issue ;-)
status-b2g-v1.3: affected → fixed
need-infoing Reuben regarding comment 20 just to avoid it not being read in case the follow-up is recommended/needed ;-) Thanks!
Flags: needinfo?(reuben.bmo)
(In reply to gtorodelvalle from comment #20)
SGTM.
Flags: needinfo?(reuben.bmo)

Comment 24

4 years ago
This is working fine on v1.3 now. Verified with today's (01/30) buri build:
Gecko-93b16ed
Gaia-8defa5b
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.