877718 - [Messages] Contact's correct phone number and type is displayed

Status: RESOLVED FIXED

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

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/2d3a45c58878
Gaia   0bfcaf8546c6d365b0474d980a154d859d22c872
BuildID 20130529230207
Version 18.0

Getting a message from a contact with multiple phone numbers and types, the wrong number and type is shown in the message pane. In my case I sent the message from my O2 phone but the number shown is my land line. In my contact information this is the first listed number. It looks like a regression from the latest SMS change. Probably from bug 876774?

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It looks like this only involves threading or the display of the number. When I reply the message get sent to the right phone number.

Comment 2

[bhavana bajaj [:bajaj]]

:whimboo, can you please provide a couple of screenshots to illustrate the description ? 

In addition already cc'ing Alexandre to check if the problem is understood and a regression from 876774 as suspected ?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: screenshot (contact entry)

I will attach two screenshots. One for the contact entry in the database and one for the sms application and how the phone number is shown. It should help you to understand the problem in not being able to find the right phone number.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: screenshot (auto-complete list without type)

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ups, sorry. I mixed up the bugs. The screenshots were not for this one.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: screenshot (contact entry)

The following two screenshots will show the exact problem. The message was sent from the '+49(163)...' number but is displayed as being sent via my office phone.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: screenshot (message thread with wrong phone number)

Comment 8

[:gerard-majax]

I can't reproduce: when I create a contact with two distinct phone numbers and that I send a message to each one, I get two entries in the thread list, and each one is mapped on the correct phone number.

My gaia is at d5d898bb28b5ff931ec2bbdfe0b096b8e45329c9 ...

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I think you misunderstood the steps. So I will make them clearer:

1. Create a contact with two phone numbers. The first should be a landline you never will be able to send a sms from. The second number setup with a number of your second phone.
2. Send a message from your second phone to your Firefox OS phone
3. Open the SMS application and the new thread
4. Observe that the phone number lists the landline and not the other mobile phone

Comment 10

[:gerard-majax]

I just redid this, and I still can't reproduce: the thread shows the correct mobile phone number and not the landline number.

Comment 11

[Borja Salguero [:borjasalguero]]

Henrik, I cant reproduce the bug as well... :S

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can you both make screenshots please how your contact entries look like? Not sure if it is important to have +() and spaces in the numbers.

Comment 13

[:gerard-majax]

I've retried with a contact which has sent a SMS from +336xxxxxxxx: contact has two phones numbers, first one tagged as home and being +33(2)47xxxxxx, the second one being the correct +336xxxxxxxx number. With this setup, thread list shows the contact name and the number and type displayed when reading the thread are correct, i.e., it's not the landline number but the mobile number.

I can't type a phone number with spaces. If you can check in your case that removing the spaces worksaround the bug, it would be very cool.

Comment 14

[Marcia Knous [:marcia]]

Adding qawanted - we should try to see if someone can reproduce this issue since this could be a blocker if it is reproducible - discussed during 6-4 triage.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm currently in MV, so if any of the B2G folks are around and want to have a look at my phone, please let me know.

Comment 16

[Darren]

Tried to repro this issue on: 
Unagi Build ID: 20130529230207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/2d3a45c58878
Gaia: 0bfcaf8546c6d365b0474d980a154d859d22c872

The only issue I could see was having the number that was called not showing the contacts name, it only showed as the number(when put as the second phone number for the contact).

Comment 17

[bov]

Tested on: 
Unagi Build ID: 20130607111630
Gecko-7af1737.Gaia-b5937ae

I can't reproduce the bug. The thread view shows the contact name and the correct mobile phone number (if there isn't carrier register), and the name and the carrier if this one is register. See attachments: "screenshot(contact entry)I", "screenshot (thread)II_1", "screenshot (thread)II_2"

Comment 18

[bov]

Attachment: "screenshot (contact entry)II"

Comment 19

[bov]

Attachment: screenshot (thread)II_1

Comment 20

[bov]

Attachment: screenshot (thread)II_2

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Guys, this is not how it should work. As I have mentioned it above I can perfectly see this on my phone, and I asked if someone in MV can have a look at. I'm still here today, so take your chance. I showed it to Marcia yesterday, so she can confirm that behavior.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Henrik please check in with Tony who will be in the MV office today.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I talked with Fabrice and he was able to see that behavior. By our talk I changed the first phone number of my own contact which is the work number displayed in the message, and it gets dynamically updated. That means we really grab the number dynamically from the contacts, and take the first one instead the one the message really came from.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, one good news is that I know now why it is happening. The mobile number I have sent the message from is in the format of +xx(xyz)12345678. When I remove the brackets in the address book the number is finally shown as the one for the sender.

So this issue depends on the correct detection of phone numbers with brackets included. Not sure if there is a bug about it, if you know please add the dependency.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I think that's enough qawanted here. The underlying problem is clear and simply has to be fixed. The good thing is that this only happens for hte display, but not when responding to such a message. That will be send to the right phone.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking since this is our data sanitation at issue (checking for brackets) and it impacts who the user thinks they are messaging.

Comment 27

[Julien Wajsberg [:julienw]]

(In reply to Henrik Skupin (:whimboo) from comment #24)
> When I
> remove the brackets in the address book the number is finally shown as the
> one for the sender.
> 

Could it be that doing that change effectively moves the number on top of the internal stack, and as a result it's been displayed ?

Anyway, I'm taking this.

Henrik, thanks for this report, I agree with you that if you're confident that you have the problem we should not close as "works for me"...

Comment 28

[Julien Wajsberg [:julienw]]

Gregor, I think we need help from Gecko here, because we don't have access to the PhoneNumber library.

I can see 2 things that could help from the mozContacts API:

* get a normalized value in addition to the user-entered value (eg in a "normalizedValue" property), so that we can easily compare with the numbers we get from the network
* when searching a contact with a "tel" search, have an option to return only the matched number for this contact, instead of returning all numbers for the found contact.

The first one looks like a need-to-have to properly support some use cases, the second one is more a nice-to-have and would allow us to not do this filtering in Gaia, which feels cleaner.

Should I file one or 2 bugs about these cases ?

Comment 29

[Julien Wajsberg [:julienw]]

About the second proposition: I'd be happy with another method, or getting that info with the usual results in another property too, ie without needing an option.

Comment 30

[Julien Wajsberg [:julienw]]

Filed bug 883923 and bug 883929, any of them would help us here.

Comment 31

[Gregor Wagner [:gwagner]]

(In reply to Julien Wajsberg [:julienw] from comment #28)
> Gregor, I think we need help from Gecko here, because we don't have access
> to the PhoneNumber library.
> 
> I can see 2 things that could help from the mozContacts API:
> 
> * get a normalized value in addition to the user-entered value (eg in a
> "normalizedValue" property), so that we can easily compare with the numbers
> we get from the network

I don't think exposing the normalized number is a good idea. Maybe it's simpler to just expose the normalization interface?

> * when searching a contact with a "tel" search, have an option to return
> only the matched number for this contact, instead of returning all numbers
> for the found contact.

Currently we can't return partial contacts. If you save a partial contact, you would overwrite the existing one and wipe out all the missing fields.

Comment 32

[Julien Wajsberg [:julienw]]

(In reply to Gregor Wagner [:gwagner] from comment #31)

> 
> Currently we can't return partial contacts. If you save a partial contact,
> you would overwrite the existing one and wipe out all the missing fields.

I understand this.

Please check bug 883929 as I've put other possible suggestions here.

Comment 33

[Gene Lian (I already quit Mozilla)]

Just having a discussion with Julien and Gregor, we're hoping to normalize the sender/receiver of the messages exposing to the Gaia end, so that the (normalized) numbers can be correctly associated with the (normalized) numbers in the Contacts API.

Fire bug 886723.

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Gene Lian [:gene] from comment #33)
> Just having a discussion with Julien and Gregor, we're hoping to normalize
> the sender/receiver of the messages exposing to the Gaia end, so that the
> (normalized) numbers can be correctly associated with the (normalized)
> numbers in the Contacts API.
> 
> Fire bug 886723.

Sorry, I've closed bug 886723 as WONTFIX.  Anyone interested in this method may want to drop a comment there.

Comment 35

[Julien Wajsberg [:julienw]]

Michal will work on this bug as soon as bug 883923 is fixed. We'll try to do without bug 886723.

Comment 36

[Rick Waldron [:rwaldron]]

Julien, I think this was actually fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=881248

Comment 37

[Julien Wajsberg [:julienw]]

not in all cases :) (eg: the "number with letters" case). I think we'll remove part of that PR  (eg the removeNonDialables part) and some of the tests, as I was explaining in https://github.com/mozilla-b2g/gaia/pull/10472#issuecomment-19687960 ...

at the end we'll get the normalized value out of the mozcontacts api.

Comment 38

[Julien Wajsberg [:julienw]]

So, we're still waiting for bug 883923 here...

Comment 39

[Julien Wajsberg [:julienw]]

unassigning myself as I won't be available in the next 2 weeks.

Should not be difficult once bug 883923 is solved.

Comment 40

[:CSchmoeckel]

In MozTrap added SMS Suite Test Case #9010 - Verify the number displayed in the message pane is the phone number and type the message originated from

Comment 41

[Alex Keybl [:akeybl]]

Over to David to reassign. Thanks for the heads up Julien.

Comment 42

[Corey Frang [:gnarf]]

Could you please flag me for review and rwaldron for feedback on any patch here?  Thanks!

Comment 43

[Alex Keybl [:akeybl]]

Etienne - can you help with reassignment?

Comment 44

[Borja Salguero [:borjasalguero]]

Etienne, I could be in charge of this. Wdyt? Re-assign to me if you agree! :)

Comment 45

[Etienne Segonzac (:etienne)]

(In reply to Borja Salguero [:borjasalguero] from comment #44)
> Etienne, I could be in charge of this. Wdyt? Re-assign to me if you agree! :)

It's SMS, you know better than me :)

Comment 46

[Borja Salguero [:borjasalguero]]

Waiting Gecko's patch for moving forward here!

Comment 47

[Julien Wajsberg [:julienw]]

Since I'm back from holiday feel free to reassign or ask review from me here (as I followed the whole story from the start).

Comment 48

[Fernando Campo (:fcampo)]

I'll definitely ping you on monday about this ;)

Comment 49

[bhavana bajaj [:bajaj]]

:fcampo, :julienw : what are the next steps here ? Trying to get an eta given how close we are to 1.1 release.

Comment 50

[Julien Wajsberg [:julienw]]

Well, we're still waiting for bug 883923.... that bug is moving forward but they had to change the whole approach after a first patch was quite finalized.

Comment 51

[Julien Wajsberg [:julienw]]

However this is imho not a so big bug to hold the release from, so feel free to move to 1.2.

Comment 52

[Julien Wajsberg [:julienw]]

asking leo? again, I'm not sure this should block the release.

Henrik, could you please update us of what is the current status on 1.1 for this bug ?

My understanding is that we fixed some of the cases in bug 881248 (doing a limited fuzzy matching in Gaia) and that the current remaining bug happens only with what I call "numbers-with-letters" (eg: 1-800-BUY-A-CAR). My second understanding of the code (I haven't checked this) is that when the bug happens we no longer display the first phone but rather we display nothing, although I'm not sure of that.

Comment 53

[bhavana bajaj [:bajaj]]

Making it a non-blocker for leo given this may have been mitigated by patch in bug 881248.

Henrik, can yo please help with comment #52 ?

Comment 54

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Julien Wajsberg [:julienw] from comment #52)
> Henrik, could you please update us of what is the current status on 1.1 for
> this bug ?

Julien, how can I enter white-spaces in the phone number on the Firefox OS device? I removed those during testing a while back, and as it seems like I cannot do it via the UI. Also I cannot re-import the contacts from Google and I don't want to reset my phone. Which database I would have to update?

Comment 55

[Julien Wajsberg [:julienw]]

Henrik> maybe you can create a new dummy contact on Google and import only this one ?

Comment 56

[Julien Wajsberg [:julienw]]

Bug 883923 was fixed so I guess we can start moving forward here.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Julien Wajsberg [:julienw] from comment #55)
> Henrik> maybe you can create a new dummy contact on Google and import only
> this one ?

As mentioned above, there is no way to reimport the contacts from Google when running the FTU app again.

Comment 58

[Julien Wajsberg [:julienw]]

I think you can import from the contacts app too.

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I have re-imported my own contact from google contacts, deleted the old thread, and send a new message from my other mobile phone. This time the number is displayed correctly. So I assume we are fine here now.

Comment 60

[Julien Wajsberg [:julienw]]

Thanks Henrik ! So confirming for koi then :)

Let's use the Fuzzy Matcher to back our Utils.compareDialables implementation !

Comment 61

[Fernando Campo (:fcampo)]

I'm focused right now in new features for this sprint and bugfixing for the next one, so if someone thinks this should be done right now, feel free to reassign, as I won't be able to fix it soon

Comment 62

[Fernando Campo (:fcampo)]

Thanks Rick

Comment 63

[Julien Wajsberg [:julienw]]

Rick, do you think you'd be able to finish this ? I don't think there is much to do here.

We could as well move this to 1.3, but this bug is there for too long ;)

Comment 64

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] from comment #63)
> Rick, do you think you'd be able to finish this ? I don't think there is
> much to do here.
> 
> We could as well move this to 1.3, but this bug is there for too long ;)

I will work on this next (ie. today)

Comment 65

[Rick Waldron [:rwaldron]]

Waiting for response to https://bugzilla.mozilla.org/show_bug.cgi?id=883923#c86

Comment 66

[Julien Wajsberg [:julienw]]

The only remaining case is small let's move this to 1.3.

If we have a fix soon we could request an approval.

Comment 67

[Rick Waldron [:rwaldron]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/12513

Part 1:
    - Implement a desktop version of navigator.mozPhoneNumberService
    - Move all desktop-specific code into js/desktop-only/
    - Updates startup.js to include new dir locations and mock files

The following files are copied almost exactly from https://mxr.mozilla.org/mozilla-central/source/dom/phonenumberutils:

- PhoneNumber.js
- PhoneNumberNormalizer.js
- PhoneNumberMetaData.js
- mcc_iso3166_table.js


Part 2: 
    - Rename Utils.compareDialables => Utils.probablyMatches
    - Utils.probablyMatches now normalizes with navigator.mozPhoneNumberService.normalize
    - Update navigator.mozContacts.find desktop "mock" to also normalize when searching numbers

Comment 68

[Julien Wajsberg [:julienw]]

Instead of copying all of PhoneNumberService so that we can use it when running the app in Firefox, I'd rather find a way to enable it with a DEBUG=1 profile.

Needinfo Kevin and Mike to see if they have an idea.

Comment 69

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] (in MozSummit until next monday) from comment #68)
> Instead of copying all of PhoneNumberService so that we can use it when
> running the app in Firefox, I'd rather find a way to enable it with a
> DEBUG=1 profile.
> 
> Needinfo Kevin and Mike to see if they have an idea.

That would be the most ideal approach :)

Comment 70

[Kevin Grandon :kgrandon]

It should certainly be possible to have all of this functionality inside of nightly without copying the code. We may have to have either a pref, or to import some jsm file. Contacts works by importing a jsm file in the addon. Unfortunately, my gecko skills are still lacking so I'm not able to quickly produce a patch for this.

If you need this, I would say that you can go ahead and land as-is, and we can open a follow-up bug to use the platform code. In the meantime I'll also try to dig into gecko code to figure out the proper way of doing it.

Comment 71

[Reuben Morais [:reuben]]

Having PhoneNumberService on desktop (behind a pref) should be simple.

Comment 72

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Julien Wajsberg [:julienw] (in Paris-Web conference thursday and friday) from comment #68)
> Instead of copying all of PhoneNumberService so that we can use it when
> running the app in Firefox, I'd rather find a way to enable it with a
> DEBUG=1 profile.
> 
> Needinfo Kevin and Mike to see if they have an idea.

Yeah, should be super simple. We can just add a check for the pref here: https://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1653

Comment 73

[Gregor Wagner [:gwagner]]

(In reply to Reuben Morais [:reuben] from comment #71)
> Having PhoneNumberService on desktop (behind a pref) should be simple.

Why do we need this? For testing?

Comment 74

[Julien Wajsberg [:julienw]]

Gregor> both for running in Firefox Desktop and for running unit tests.

Comment 75

[David Scravaglieri [:scravag]]

Blocking as it is a regression

Comment 76

[David Scravaglieri [:scravag]]

blocking per comment 75

Comment 77

[Julien Wajsberg [:julienw]]

The remaining bug here is not a regression, the regression was fixed in bug 881248.

Comment 78

[Julien Wajsberg [:julienw]]

I've outlined a new plan in https://github.com/mozilla-b2g/gaia/pull/12513#issuecomment-26582824 that would let us work without exposing mozPhoneNumberService to content with a pref (bug 926414).

Comment 79

[Julien Wajsberg [:julienw]]

Comment on attachment 813555 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12513

r=me with the nits fixed
and provided Travis is green-ish

Comment 80

[Julien Wajsberg [:julienw]]

Hey Rick,

I just noticed we haven't closed this yet ;)

Any chance you can wrap up soon?

Comment 81

[Julien Wajsberg [:julienw]]

Just noticed it landed but this bug was not updated.

master: d7696ab98db897db75a7fb633cbe3fc3a68ae820