994417 - [B2G][System]Missed call notification from a contact does not display the contact's name

Status: RESOLVED FIXED

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

Description

[Andrew Stole [:astole]]

Attachment: logcat

Instead of showing the contact's name in a missed call notification, the notification says 'Missed call From {{ contact }}'

Repro Steps:
1) Update a Buri to BuildID: 20140409130909
2) Created a contact with a valid phone number
3) Call the DUT with the number attached to the contact in step 2
4) End the call from the device making the call
5) Slide down notification bar

Actual:
The Missed call notification does not match the contact's information

Expected:
The name from the missed call should match the contact's information

1.5 Environmental Variables:
Device: Buri 1.5 MOZ
BuildID: 20140409130909
Gaia: 9d0b1bdf746823a94b13e6574c1d8304dc584763
Gecko: 5a5ed08df529
Version: 31.0a1
Firmware Version: V1.2-device.cfg

Repro frequency: 100%
See attached: logcat, screenshot

Comment 1

[Andrew Stole [:astole]]

Attachment: Screenshot

Comment 2

[Jason Smith [:jsmith]]

Can we check to see if this happens on 1.4?

Comment 3

[Gregor Wagner [:gwagner]]

(In reply to Andrew Stole from comment #0)
> Created attachment 8404347 [details]
> logcat
> 
> Instead of showing the contact's name in a missed call notification, the
> notification says 'Missed call From {{ contact }}'
> 
> Repro Steps:
> 1) Update a Buri to BuildID: 20140409130909
> 2) Created a contact with a valid phone number

What fields did you fill out when you created the contact?

Comment 4

[Marcia Knous [:marcia]]

I also saw this bug today when testing Master, but I haven't been able to reproduce it. I thought initially that I had a missed call from a contact and then tried to add the number without filling out any fields, but I tried those steps and wasn't able to repro again. Will keep trying to get some better steps - in the meantime also adding Andrew for ni.

Comment 5

[Dietrich Ayala (:dietrich)]

STR:

1. import contacts from Google
2. receive a call from a Google contact, and miss it
3. see notification
4. cry tears of sadness

Comment 6

[Dietrich Ayala (:dietrich)]

Re comment #5:

* master + m-c, 4/10
* Nexus 4

Comment 7

[Gregor Wagner [:gwagner]]

(In reply to Dietrich Ayala (:dietrich) from comment #5)
> STR:
> 
> 1. import contacts from Google
> 2. receive a call from a Google contact, and miss it
> 3. see notification
> 4. cry tears of sadness

Can you let me know how the contact looks like in the contact app? Are the name fields correct?

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Dietrich Ayala (:dietrich) from comment #5)
> STR:
> 
> 4. cry tears of sadness

There is an easy fix for this. Next time, just pick up the phone so you don't have to cry :)

Comment 9

[Matthew Vaughan]

(In reply to Jason Smith [:jsmith] from comment #2)
> Can we check to see if this happens on 1.4?

This does not reproduce for me on the 04/11/14 1.4 build on a Buri.

Device: Buri v1.4 MOZ RIL
BuildID: 20140411000202
Gaia: 6c50349f41d40ba175ea0fc0c2c2cbd739ba7170
Gecko: 28b419f0e857
Version: 30.0a2
Firmware Version: v1.2-device.cfg


(In reply to Marcia Knous [:marcia - use needinfo] from comment #4)
> I also saw this bug today when testing Master, but I haven't been able to
> reproduce it. I thought initially that I had a missed call from a contact
> and then tried to add the number without filling out any fields, but I tried
> those steps and wasn't able to repro again. Will keep trying to get some
> better steps - in the meantime also adding Andrew for ni.

I was able to reproduce the issue using the STR from comment 0 consistently. It appears that so long as the contact has either a first or last name, or both, the notification will display 'Missed call From {{ contact }}'. If the contact only has a phone number, it will display 'Missed call From <contact number>' where "contact number" actually shows the phone number from the contact who is calling the DUT.

Comment 10

[Marcela Oniga]

Same issue on my Keon device,1.5.0.0 version, build identifier: 20140413025413

Comment 11

[:gerard-majax]

100% reproducing too.

Comment 12

[Matthew Vaughan]

TINDERBOX:
This looks to be a gaia issue.

last working gaia/first broken gecko = NO REPRO
Gaia  650e8c2c611ed07495d3bf3769f44a0efd88a492
Gecko 7160658c4be3

first broken gaia/last working gecko = REPRO
Gaia  9d0b1bdf746823a94b13e6574c1d8304dc584763
Gecko e84d0f010637

B2G INBOUND:
- Last Working -
Device: Buri ENG Master (2.0) MOZ RIL
BuildID: 20140409004946
Gaia: 650e8c2c611ed07495d3bf3769f44a0efd88a492
Gecko: 2d76999fe595
Version: 31.0a1
Firmware Version: v1.2-device.cfg

- First Broken -
Device: Buri ENG Master (2.0) MOZ RIL
BuildID: 20140409010346
Gaia: 9d0b1bdf746823a94b13e6574c1d8304dc584763
Gecko: 1497cb76b145
Version: 31.0a1
Firmware Version: v1.2-device.cfg

Push log: https://github.com/mozilla-b2g/gaia/compare/650e8c2c611ed07495d3bf3769f44a0efd88a492...9d0b1bdf746823a94b13e6574c1d8304dc584763


Please note: The gaias for the Tinderbox and Inbound last working and first broken builds match. This is an odd situation, but I double checked to make sure this window was correct.

Comment 13

[Jason Smith [:jsmith]]

The window here can be done deeper with b2g-inbound builds.

Comment 14

[Matthew Vaughan]

(In reply to Jason Smith [:jsmith] from comment #13)
> The window here can be done deeper with b2g-inbound builds.

I can confirm the above regression window was found using B2G Inbound builds. I double checked by downloading the two builds directly from the PVT website and matching the gaia and geckos for both builds. If there is another area I can go to get a more refined window, please let me know.

Comment 15

[bhavana bajaj [:bajaj]]

Starting with a NI on :rik from dialer here to get some direction as this is 100% reproducible. :rik, anything obvious form the long regression window we have here or something obviously wrong going by the logcat ?

Comment 16

[Anthony Ricaud (:rik)]

This is a regression of the new l10n.js library. https://github.com/mozilla-b2g/gaia/commit/fd2d573db38fa4ff5e607e62a35a23cb15c025d7

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

taking

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request

Ok, it's a trivial fix.

So, the refactor is a bit more strict about the type of the variable you can pass as placeable.

The old code was just implicitly stringifying it while hte new code has a check to reject anything but strings and numbers.

https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L784-L788

Fixing this by just explicitly stringifying the param passed to mozL10n.get.

Comment 19

[Anthony Ricaud (:rik)]

Comment on attachment 8407864 [details] [review]
pull request

(Please ask for review to the module peers: https://wiki.mozilla.org/Modules/FirefoxOS)

I don't think this is the proper fix. We probably have other places of the code base relying on this old behaviour so the new lib should respect it imho.

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Anthony Ricaud (:rik) from comment #19)
> Comment on attachment 8407864 [details] [review]
> pull request
> 
> (Please ask for review to the module peers:
> https://wiki.mozilla.org/Modules/FirefoxOS)
> 
> I don't think this is the proper fix. We probably have other places of the
> code base relying on this old behaviour so the new lib should respect it
> imho.

The API in the future will enable passing objects as context data like this:

`mozL10n.get('id', {'user': {'name': 'Zibi', 'gender': 'male'});`

and allow localizers to refer to 'user.name' or 'user.gender' as needed.
and we do not want to implicitly stringify that.

Comment 21

[Anthony Ricaud (:rik)]

Alright. Since we're early in the cycle, I guess we can keep that check and fix all call sites. But late in the cycle, we might have to be less strict if we're still seeing such regressions.

So let's go with this approach. But we still need new tests to verify that we're sending a string to the l10n lib.

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

Cool, thanks. 

I agree with this approach.

Can you help me design the test you'd like to see?

We're talking about this code: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/dialer.js#L64-L80

Since I want to run toString() on primaryInfo in that scenario, it doesn't make much sense - it will be a string.

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8407864 [details] [review]
pull request

updated the patch to add the tests. re-requesting r=

Comment 24

[Anthony Ricaud (:rik)]

Comment on attachment 8407864 [details] [review]
pull request

Reviewed via IRC.

Thanks!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/56a06e3e843e97b2cb1877d395caf8d43c8ac103

Comment 26

[Johan Lorenzo [:jlorenzo]]

Added in bug 1086676