877145 - [SMS] [regression] Carrier display in contact search / thread headers

Status: VERIFIED FIXED

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

Description

[rafael.marquez]

Branch:V1-train
Gecko:489c6ee
Gaia:53055a5

Procedure:
1 - Add a contact filling the name and two phone numbers with the same type and the same carrier
2 - Send an SMS to the contact added in the step 1
3 - Open the conversation created with the sms sent in the step 2

Expected result:

The contact has two unique phone numbers with the same type and carrier than the one used for the conversation: show Phone number instead of carrier.

Actual Result
The header shows the carrier and in this case should show the phone number instead of carrier.

Comment 1

[Maria Angeles Oteo (:oteo)]

Requesting leo+ as this is a regression

Comment 2

[Julien Wajsberg [:julienw]]

Can you check this still happens now that Bug 876774 landed ? Maybe this was caused by the same problem ?

Comment 3

[Corey Frang [:gnarf]]

I want to make sure we get this perfectly right:

According to ayman in https://bugzilla.mozilla.org/show_bug.cgi?id=872395#c19 :

Across the Dialer and Message apps Carrier is always king… 

1) So if a phone number has carrier associated with it the output will be:

	Firstname Lastname
	type | carrier

2) If there is no carrier associated with the phone number the output will be:

	Firstname Lastname
	type | phonenumber

3) If for some reason a single contact has two phone numbers with the same type and the same carrier the output will be:

 	Firstname Lastname
	type | phonenumber

--- but then according to bug 876754 - we should use a comma ---- now what?

Just to confirm, these should be: 

type, phonenumber
-or-
type, carrier

We should use carrier where available, unless two numbers with the same carrier exist? And we should use a comma?

Comment 5

[Victoria Gerchinhoren [:vicky]]

This has already been discussed in another bug. Commas are the one we're using for separating those elements.

Comment 6

[ayman maat :maat]

Hey Corey, I can confirm you are correct regarding the information to be displayed and under what condition. If you need more clarity on this, ping me.

Comment 7

[Steve Chung [:steveck]]

I will send my patch later, but it seems a gecko issue to me because the returning contact tel's type is not a string('mobile'), it's an array with single string(['mobile']), and that break the type comparison in the original checking logic. It seems not possible to have a multiple type for a number, so I'm not sure why the returning type will become an array.

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Steve Chung from comment #7)
> I will send my patch later, but it seems a gecko issue to me because the
> returning contact tel's type is not a string('mobile'), it's an array with
> single string(['mobile']), and that break the type comparison in the
> original checking logic. It seems not possible to have a multiple type for a
> number, so I'm not sure why the returning type will become an array.

The 'type' field has always been an array: https://hg.mozilla.org/mozilla-central/file/8f9ba85eb61c/dom/interfaces/contacts/nsIContactProperties.idl#l22

Just because our UI doesn't allow multiple types doesn't mean that the API shouldn't support it.
I don't know why this showed up right now but we should fix the comparison code to support the proper return value.

Comment 9

[Steve Chung [:steveck]]

Attachment: Patch for contact tel type fixing

Comment 10

[Steve Chung [:steveck]]

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

> Just because our UI doesn't allow multiple types doesn't mean that the API
> shouldn't support it.
> I don't know why this showed up right now but we should fix the comparison
> code to support the proper return value.

Thanks Gregor, It's great to know that.

Comment 11

[Corey Frang [:gnarf]]

Comment on attachment 758447 [details]
Patch for contact tel type fixing

Have a request to rename some variables, and protect ourselves a bit from regression in the units.

Neither of these changes are significant, so r=me - please merge after those quick changes, assuming the units are still green

Comment 12

[Steve Chung [:steveck]]

Landed in master: a62f42f5f00f0fd0b7f9c8652ed95ed55c916389

Comment 13

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted a62f42f5f00f0fd0b7f9c8652ed95ed55c916389 to:
v1-train: 3576660cfc1c2dd896cc839e4d7352ac6d83f129

Comment 14

[Deepa Subramanian]

The user sees "Phonetype|Carrier, Phone number" on Master build (V1.1 and V1.2)while on Leo V1.1, The user sees "|Carrier, Phone number" (See screenshot on Leo). 
 
This issue repros on the Leo v.1.1. 
Leo Build ID: 20130809041203
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/45480132106b
Gaia: c9d0901564cf6f50e375ab48e4124b8378a2e246
Platform Version: 18.1

Comment 15

[Deepa Subramanian]

Attachment: Screenshot

Comment 16

[Julien Wajsberg [:julienw]]

Deepa: I don't understand the part "master build (v1.1 and v1.2)". What's the difference between the "v1.1" where it works and the "v1.1" where it doesn't ?

Comment 17

[rafael.marquez]

bug solved in today master build for unagi. Gecko-6a1bd2d Gaia-1b9019a

Comment 18

[Deepa Subramanian]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> Deepa: I don't understand the part "master build (v1.1 and v1.2)". What's
> the difference between the "v1.1" where it works and the "v1.1" where it
> doesn't ?

Julien, Sorry for not being specific. I tested on the Central/master build for Unagi(v1.2) and Buri device (v1.1).

When the user opens SMS app for composing a msg and taps on the "+" to add a recipient with two phone numbers. The user sees the recipient's name is displayed with Phone type, carrier and phone number. On the leo device. The phone type is missing on the specific build mentioned earlier. Let me know if you have further questions.

Comment 19

[Julien Wajsberg [:julienw]]

Rick, could you investigate the comment 18 please ? I think we fixed this in another bug, but maybe it was not leo+ (and therefore to me it's ok).