926572 - [B2G][Contacts] Last name appears twice when merging an imported SIM contact with Facebook contact

Status: RESOLVED FIXED

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

Description

[Matthew Vaughan]

Attachment: Screenshot of contact with last name appearing twice.

Description:
A contact's last name will appear twice in a row when the user synchronizes their Facebook contact to a contact imported from a SIM card.

Repro Steps:
1) Create and befriend a new contact with at least a first and last name on a test Facebook account
2) Update Buri to BuildID: 20131014004003
2) Launch the Contacts app
3) Create a new contact with the same name as the Facebook friend created earlier
4) Export the new contact to the SIM card
5) Delete the contact from the Contacts list
6) Import the contact from the SIM card
7) Sync your Facebook account and import the contact
8) Return to the Contacts list

Actual:
Contact's last name appears twice in a row. (ex. Joe Tester appears as Joe Tester Tester)

Expected:
Contact's name remains the same as how the user entered it.

Environmental Variables:
Device: Buri
BuildID: 20131014004003
Gaia: d562c8f6eaed158bf5afa556f10c2f2e8ae8137f
Gecko: 690f28662ace
Version: 26.0a2

Notes:

Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/10380/
See attached: ContactLastNameTwice.png & logcat.txt

Comment 1

[Matthew Vaughan]

Attachment: LogCat for synchronizing Facebook contact with contact imported from SIM.

Comment 2

[Jason Smith [:jsmith]]

Seems like this is something basic that isn't working - last name is being displayed incorrectly.

Comment 3

[Jason Smith [:jsmith]]

Blocking+ for regression

Comment 4

[Matthew Vaughan]

UPDATE to repro steps provided below. Bug occurs when merging passively or linking a contact.

Repro Steps:
1) Create and befriend a new contact with at least a first and last name and phone number on a test Facebook account
2) Update Buri to BuildID: 20131014004003
2) Launch the Contacts app
3) Create a new contact with the same name and phone number as the Facebook friend created earlier
4) Export the new contact to the SIM card
5) Delete the contact from the Contacts list
6) Import the contact from the SIM card
7) Sync your Facebook account and import the contact
8) Return to the Contacts list

Comment 5

[Sarah Parsons]

This issue started to occur on the Buri 1.2 Build ID: 20130801030224

Environmental Variables:
BuildID: 20130801030224
Gaia: c1620a242c937c7fcb171b88c85ef63561165081
Gecko: 05d3797276d3
Version: 25.0a1

Last Working Buri 1.2 Build ID: 20130730030200

Gaia   ba5ff211fbf6a930326cc6a0d4a1205a7528630b
SourceStamp 3d40d270c031
BuildID 20130730030200
Version 25.0a1

Comment 6

[Jose Manuel Cantera]

(In reply to Jason Smith [:jsmith] from comment #3)
> Blocking+ for regression

This is not a bug nor a regression. It is a limitation on how the importation from SIM works. When importing a contact from SIM the givenName will be made equal to the full name. Thus, when linking such a contact with the FB Contact, the familyName will be obtained and as a result, the name will have the familyName two times.

That's not catastrophic for the user as he can always edit the contact and put the correct name avoiding duplication. IMHO, this is not blocking and trying to solve it might introduce other regressions. LKast but not least the use case is pretty weird. 

Noemi, Please could you retriage this? 

thanks

Comment 7

[Jason Smith [:jsmith]]

(In reply to Jose M. Cantera from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #3)
> > Blocking+ for regression
> 
> This is not a bug nor a regression. It is a limitation on how the
> importation from SIM works. When importing a contact from SIM the givenName
> will be made equal to the full name. Thus, when linking such a contact with
> the FB Contact, the familyName will be obtained and as a result, the name
> will have the familyName two times.

No. comment 5 has already proven that this is a regression that used to work on 7/30/2013.

> 
> That's not catastrophic for the user as he can always edit the contact and
> put the correct name avoiding duplication. IMHO, this is not blocking and
> trying to solve it might introduce other regressions. LKast but not least
> the use case is pretty weird. 

It's silly to expose a bug to a user in which you manage to get their last name incorrect. This is not acceptable regression for ship by any means.

There's no doubt in my mind this is a blocker for 1.2.

Comment 8

[Jose Manuel Cantera]

Just to be clear. This is not a regression it's been always happeming with SIM Contacts. Actually, the test case is easier than what it is described in this bug. Just, import a SIM Contact and link it with a FB Contact. You will notice that the familyName coming from FB will be added to whatever name has the contact on the SIM. This is because, in the SIM card there is no way to distinguish between givenName and familyName. As a result, while importing we put the content of the SIM card contact's name on the local givenName field. So, that's the root cause of the limitation described here. As you know, the FB linking process links the FB Contact surname if the local contact does not define any familyName, as it happens with contacts imported from SIM. 

Unfortunately trying to find workarounds here is too late for this version and there is a high risk of introducing regressions and unexpected behaviours with other contacts which are not SIM Contacts. 

So, my proposal here would be to study a better solution starting from re-thinking about the SIM importing process and all the consequences for import, merge and linking processes. And try to solve it in v1.3.

Comment 9

[Jason Smith [:jsmith]]

Your analysis is incorrect - this was working in comment 5 before, so you need to provide a reasoning why this was working originally in 1.2. This still stands as a regression until you explain comment 5. Risk also doesn't have any effect on whether we block on something or not - it only effects if we can take something on an uplift or not.

Comment 10

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

comms team will discuss this further

Comment 11

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

triage: eng evaluation for improving this in 1.3. ni? Ayman/Jose for options

Comment 12

[Jason Smith [:jsmith]]

(In reply to Joe Cheng [:jcheng] from comment #11)
> triage: eng evaluation for improving this in 1.3. ni? Ayman/Jose for options

Not happening. We aren't shipping with a silly last name duplication regression. Sorry, but this needs to block until comment 5 can be effectively explained.

Comment 13

[Jose Manuel Cantera]

(In reply to Jason Smith [:jsmith] from comment #12)
> (In reply to Joe Cheng [:jcheng] from comment #11)
> > triage: eng evaluation for improving this in 1.3. ni? Ayman/Jose for options
> 
> Not happening. We aren't shipping with a silly last name duplication
> regression. Sorry, but this needs to block until comment 5 can be
> effectively explained.

Jason it has been explained thouroughly. It is not our problem you cannot understand

thanks

Comment 14

[Jason Smith [:jsmith]]

(In reply to Jose M. Cantera from comment #13)
> (In reply to Jason Smith [:jsmith] from comment #12)
> > (In reply to Joe Cheng [:jcheng] from comment #11)
> > > triage: eng evaluation for improving this in 1.3. ni? Ayman/Jose for options
> > 
> > Not happening. We aren't shipping with a silly last name duplication
> > regression. Sorry, but this needs to block until comment 5 can be
> > effectively explained.
> 
> Jason it has been explained thouroughly. It is not our problem you cannot
> understand
> 
> thanks

I think I understand a regression range that says your analysis is incorrect. You haven't explained at all why this was working fine on July 30th in the 1.2 timeframe. As long as this was working previously, I don't think your analysis is correct as it stands.

Comment 15

[ayman maat :maat]

(In reply to Joe Cheng [:jcheng] from comment #11)
> triage: eng evaluation for improving this in 1.3. ni? Ayman/Jose for options

Though this is clearly a nuisance I would support improvement in V1.3. Jose is correct in his analysis with the high risk of further regression and at the end of the day the phenomenon poses no loss of data or negative impact the usability of the system.

Comment 16

[Jason Smith [:jsmith]]

(In reply to ayman maat :maat from comment #15)
> (In reply to Joe Cheng [:jcheng] from comment #11)
> > triage: eng evaluation for improving this in 1.3. ni? Ayman/Jose for options
> 
> Though this is clearly a nuisance I would support improvement in V1.3. Jose
> is correct in his analysis with the high risk of further regression and at
> the end of the day the phenomenon poses no loss of data or negative impact
> the usability of the system.

I think QA has made it pretty clear that this is a regression from a past release, so I don't see how your assessment here is valid at all. This will remain as a blocker until comment 5 can be explained.

Comment 17

[Jason Smith [:jsmith]]

This is likely a regression from bug 899525.

Comment 18

[isabelrios]

Just checked with latest leo build available:
buildid: 20131023...
30fb7fb...

Not merging since this is v1.1 and it is not implemented but linking both contacts, and the behaviour is the same. The last name appears twice. Just to confirm that was implemented that way and does not seem to be a regression.

Comment 19

[Jason Smith [:jsmith]]

(In reply to Isabel Rios [:isabel_rios] from comment #18)
> Just checked with latest leo build available:
> buildid: 20131023...
> 30fb7fb...
> 
> Not merging since this is v1.1 and it is not implemented but linking both
> contacts, and the behaviour is the same. The last name appears twice. Just
> to confirm that was implemented that way and does not seem to be a
> regression.

I don't understand what this statement means at all & how it relates to this bug, as the STR involved in the bug uses contact exporting, which is only supported in 1.2 or later.

Sarah - Can you double check your regression window in https://bugzilla.mozilla.org/show_bug.cgi?id=926572#c5 to confirm it's correct? What happens on 1.1 as well?

Comment 20

[isabelrios]

Sorry I did not explain myself well enough. 
When it comes to facebook contacts, when merging with a contact in the contact list what happens is actually a link between them. 
Just an example that the problem is also seen on v1.1.

Comment 21

[Jason Smith [:jsmith]]

(In reply to Isabel Rios [:isabel_rios] from comment #20)
> Sorry I did not explain myself well enough. 
> When it comes to facebook contacts, when merging with a contact in the
> contact list what happens is actually a link between them. 
> Just an example that the problem is also seen on v1.1.

Okay. I guess the other question I have for you is this - why is comment 5 indicating the regression window it states? My only guess is that some other issue was preventing testing in the working build, but it broke in the following build where bug 899525 was implemented to expose the bug (i.e. not regressed by bug 899525, but rather bug 899525 exposes a way to reproduce the bug). 

Going to pull nom for now based on your testing, but I'd really like clarity on why comment 5 indicates it says.

Sarah - Instead of doing what I stated in comment 19, can you clarify the differences of the behavior seen in the 7/30/2013 build vs. 8/1/2013 build?

Comment 22

[Sarah Parsons]

The difference between the 7/30 and the 8/1 builds was that importing contacts from the SIM was broken per bug 888263.

When trying to import contacts to the SIM on the 7/30 build, and error message pops up and says "unable to import contacts" which importing from the SIM is an essential step in reproducing this bug. 

This is why I gave these two builds as the regression window because importing SIM contacts works in the 8/01 build and this bug 926572 can be successfully reproduced. 

Also, I was unable to reproduce this issue on the Leo 1.1 Build ID 20131101041316

Gaia   39b0203fa9809052c8c4d4332fef03bbaf0426fc
SourceStamp 31fa87bfba88
BuildID 20131101041316
Version 18.0

Comment 23

[Jose Manuel Cantera]

As I stated weeks ago, this not a regression. Thus, nominating this for v1.3 as it happens on v1.1, v1.2 and we want it to be fixed in v1.3 .

Comment 24

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

can this be fixed in the 1.3 convergence period?

Comment 25

[Jose Manuel Cantera]

yes, we have a possible solution in mind

Comment 26

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

triage: since it's a new feature, not blocking release. may ask for approval to land in 1.3 later when we have clearer picture on the solution

Comment 28

[Jose Manuel Cantera]

Attachment: 17155.html

Comment 29

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

Comment on attachment 8390449 [details]
17155.html

Left some comments in the bug.

I'm a bit worried about languages written from right to left, pinging Kaze for clarifying if this will affect the piece of code.

A part from that, really happy to see this, we could reuse this piece of code in more places like SIM, gmail, etc.

Thanks Jose

Comment 30

[Jose Manuel Cantera]

(In reply to Francisco Jordano [:arcturus] from comment #29)
> Comment on attachment 8390449 [details]
> 17155.html
> 
> Left some comments in the bug.
> 
> I'm a bit worried about languages written from right to left, pinging Kaze
> for clarifying if this will affect the piece of code.
> 

I think Fabien is on PTO. Once he comes back we can open a follow-up to deal with arabic names. 

> A part from that, really happy to see this, we could reuse this piece of
> code in more places like SIM, gmail, etc.

In SIM it is already done, in gmail etc we can do it in follow-up bugs

thanks! 

> 
> Thanks Jose

Comment 31

[Jose Manuel Cantera]

landed in master: 

https://github.com/mozilla-b2g/gaia/commit/2b5e6cb4dfad3cea948de6653843ca00848b313e

Comment 32

[Fabien Cazenave [:kaze]]

Comment on attachment 8390449 [details]
17155.html

(In reply to Francisco Jordano [:arcturus] from comment #29)
> I'm a bit worried about languages written from right to left, pinging Kaze
> for clarifying if this will affect the piece of code.

This patch makes sense for standard cases such as Western names, it even gives surprisingly good results for tricky cases like Spanish names, but we can’t expect it to work for any language. The two main issues are unicameral (unicase) alphabets [1] and the Eastern name order (family name before personal name) [2].

[1] http://en.wikipedia.org/wiki/Unicase
[2] http://en.wikipedia.org/wiki/Name_order

There’s no particular problem with RTL per se, but the `startsWithUpper' function will always return true for unicase alphabets like the RTL ones (Arabic/Farsi, Hebrew) and a few LTR ones (e.g. Thai, Devanagari, Georgian, Hangul…). Note also that for Chinese and Japanese (unicase as well), the `MIN_LENGHT_SIGNIFICATIVE' constant is probably not very meaningful.

The minimum would be to limit this logic in the `parseName' helper to bicameral (non-unicase) alphabets like Latin, Cyrillic and Greek alphabets. This can be tested with a simple regexp like this one:

  var alpha = {
    'Latn': /^[\u0000-\u024F]/, // basic + supplement + ext-A + ext-B
    'Cyrl': /^[\u0400-\u052F]/, // basic + supplement
    'Grek': /^[\u0370-\u03FF]/
  };

Then we have an issue with locales where the “Eastern order” is used, i.e. when the family name comes first — e.g. Chinese, Japanese, Korean. Again, we could detect the alphabet to guess if the first name is the family name or the given name, though I can already think of at least two cases where this will fail:

 • some locales are written with a Latin alphabet but use the Eastern order, e.g.: Vietnamese, Hungagrian;
 • for Chinese and Japanese, this wouldn’t work with pinyin and rōmaji input (= phonetic transcription with the Latin alphabet).

I’d be tempted to recommend not doing any assumption at all, i.e. to drop the `parseName' idea completely; but if that’s the only workaround we can think of to avoid duplication when importing contacts, let’s make sure we’re limiting the logic to the cases where it has the best chances to work and let’s comment the related code properly for the cases where we expect it to fail.

Comment 33

[Shine(Leave from Mozilla)]

Attachment: video

This problem is verified pass on latest build of Flame3.0
See attachments: Verify_Flame3.0.MP4
Rate: 0/5

Flame 3.0 build:
Build ID               20150226010233
Gaia Revision          7894b929f1b0394f3c997f72a6482bc7813e758d
Gaia Date              2015-02-25 20:50:05
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/dd6353d61993
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150226.043500
Firmware Date          Thu Feb 26 04:35:10 EST 2015
Bootloader             L1TC000118D0