Last Comment Bug 1005862 - [Call Screen][Conference call] Wrong participant leaving the conference notification message
: [Call Screen][Conference call] Wrong participant leaving the conference notif...
: regression
Product: Firefox OS
Classification: Client Software
Component: Gaia::Dialer (show other bugs)
: unspecified
: x86 Gonk (Firefox OS)
-- normal (vote)
: 2.0 S2 (23may)
Assigned To: Zibi Braniecki [:gandalf][:zibi]
Depends on: 1086269
Blocks: 914414
  Show dependency treegraph
Reported: 2014-05-05 04:24 PDT by Germán Toro del Valle (:gtorodelvalle)
Modified: 2014-11-13 10:13 PST (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard: [fxosqa-auto-backlog+]
Iteration: ---
Points: ---

[screenshot] callscreen_conference_call_participant_call_ended.png (132.64 KB, image/png)
2014-05-05 04:24 PDT, Germán Toro del Valle (:gtorodelvalle)
no flags Details
maybe a fix? (542 bytes, patch)
2014-05-13 13:16 PDT, Zibi Braniecki [:gandalf][:zibi]
no flags Details | Diff | Splinter Review
pull request (46 bytes, text/x-github-pull-request)
2014-05-14 12:56 PDT, Zibi Braniecki [:gandalf][:zibi]
anthony: review+
anthony: feedback+
Details | Review | Splinter Review

Description User image Germán Toro del Valle (:gtorodelvalle) 2014-05-05 04:24:27 PDT
Created attachment 8417325 [details]
[screenshot] callscreen_conference_call_participant_call_ended.png

  1. Establish a conference call with at least 2 additional parties. At least one of them has to be a contact from the contact list. 
  2. Hang up the call in the device of the contact.

  3. The text "NameOfTheContact has left the call." is shown where NameOfTheContact is the name of the contact :)

  3. The text "{{ caller }} has left the call." is shown. Please the attached screenshot.

It works as expected when the participant who leaves the conference is a phone number with no associated contact in the contacts list.
Comment 1 User image Noemí Freire (:noemi) 2014-05-05 05:01:55 PDT

This issue is not occurring on today's (5/5) v1.4 build:
Device: Hamachi
BuildId: 20140505063510
Gecko: 3ed3bbf
Gaia: fccb15d
Platform version: 30.0

Under this scenario, "The "NameOfTheContact has left the call" message is properly shown so adding regression keyword and nominating to 2.0?
Comment 2 User image Wesley Huang [:wesley_huang] (EPM) (NI me) 2014-05-08 00:13:53 PDT
triage: it's a regression
Comment 3 User image Anthony Ricaud (:rik) 2014-05-12 05:06:47 PDT
I think this is a regression of bug 914414. It might be the same kind of fix as in bug 994417.
Comment 4 User image Zibi Braniecki [:gandalf][:zibi] 2014-05-13 08:21:43 PDT
Comment 5 User image Anthony Ricaud (:rik) 2014-05-13 09:25:05 PDT
From IRC: "<gandalf> Rik: oh, didn't ask you. Are you ok with me taking over bug 1005862". Yup, I haven't started on it yet. Thanks!
Comment 6 User image Zibi Braniecki [:gandalf][:zibi] 2014-05-13 13:16:59 PDT
Created attachment 8421952 [details] [diff] [review]
maybe a fix?

Ok, so first of all, I don't know how to establish a conference call in Gaia. I only got two 1-1 calls and was able to switch between them.

Second of all, I'm really concerned about the amount of multi-type variables in callscreen code. It makes it really hard to debug.

My guess is that in one case self._cachedInfo is in fact an Object, when it gets data from getPhoneNumberPrimaryInfo in line 178 (and line 187), but the function itself may return multiple data types.

I also verified that all other uses of mozL10n.get in callscreen/js and it seems that there are no other type mismatches.
Comment 7 User image Zibi Braniecki [:gandalf][:zibi] 2014-05-14 08:40:28 PDT
Germán, can you provide STR on how to reproduce this bug? (including, how to initialize conference call)
Comment 8 User image Anthony Ricaud (:rik) 2014-05-14 10:38:12 PDT
To merge calls, you can press the "merge arrow" on the right of one of the calls. If it doesn't work, you should see the message "Unable to merge calls" at the bottom of the screen.
Comment 9 User image Zibi Braniecki [:gandalf][:zibi] 2014-05-14 12:56:12 PDT
Created attachment 8422677 [details] [review]
pull request

I was able to reproduce the bug and confirm that this patch fixes it.
Comment 10 User image Anthony Ricaud (:rik) 2014-05-17 09:26:21 PDT
Comment on attachment 8422677 [details] [review]
pull request

Yup, proper fix. We need a new test inside this suite though:
Comment 11 User image Zibi Braniecki [:gandalf][:zibi] 2014-05-18 16:48:18 PDT
Comment on attachment 8422677 [details] [review]
pull request

New, updated patch, now with a test!
Review while it's hot!
Comment 12 User image Anthony Ricaud (:rik) 2014-05-19 06:09:28 PDT
Comment on attachment 8422677 [details] [review]
pull request

Good change but the linter is not happy, see Travis.

You should install jshint and gjslint to have the pre-commit hook do the linting for you. You should have a link to the instructions to do that in your terminal when you do |git commit|.
Comment 14 User image Geo Mealer [:geo] -- This account is inactive after 2015-07-07 2014-11-13 10:13:05 PST
I'm plussing for backlog, but I'm concerned with the number of moving parts for an automation test.

Note You need to log in before you can comment on or make changes to this bug.