983594 - Avoid making several calls from detail view at the same time

Status: RESOLVED FIXED

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

Description

[Cristian Rodriguez (:crdlc)]

Attachment: unable to make a phone call.png

STR:

1) Create a contact with a telephone number
2) Go to details
3) Click several times to telephone field

Expected result:

The dialer should be displayed calling to my contact

Current result:

A message is displayed with title "Unable to make a phone call now" and behind this one the dialer trying to make the call. After a couple of seconds the dialer is displayed on the top of this message. When the call is finished, the dialer disappears but the message is still there.

My approach:

I will try to disable phone buttons while a call is trying to be performed in order to create just one Activity.

Comment 1

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1

WIP

The implementation is already done but I am working on unit tests. While I am on it comments/feedback are more than welcome.

Thanks guys for your support

Comment 2

[Jose Manuel Cantera]

Comment on attachment 8391152 [details]
Patch v1

I left some comments on GH but all of them are non substantive

thanks for the work!

Comment 3

[Etienne Segonzac (:etienne)]

Comment on attachment 8391152 [details]
Patch v1

I don't think postMessage is the right approach here.

To make a call the contacts app uses the TelephonyHelper [1], and you can pass optional callbacks to the telephony helper. You can find an example here [2].

You probably want to disable the button until you get an onconnected or onerror or ondisconnected callback.

Bonus: no change required to the dialer app :D

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts.js#L466
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/dialer.js#L254-L293

Comment 4

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

We have a patch for DSDS that needs to land for 1.4 that will modify a bit this functionality, that is still needed.

You can track the process:
https://github.com/mozilla-b2g/gaia/pull/17143

Comment 5

[Jason Smith [:jsmith]]

QA Wanted - does this happen on 1.3?

Comment 6

[Brogan Zumwalt [Inactive]]

Issue does NOT occur on latest 1.3 

Result: After a pause the call initiates without an error message. No message is present when call is ended.

Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20140314004002
Gaia: 932789749406a79c5630c27c724f1856bd3c3f10
Gecko: 0479f5480378
Version: 28.0
Firmware Version: v1.2-device.cfg

Comment 7

[Jason Smith [:jsmith]]

Were you able to see this on 1.4 as a point of comparison?

Comment 8

[Brogan Zumwalt [Inactive]]

Was able to reproduce on todays 1.4 before I tested on 1.3

Result: Error displayed before call initiates and after call ends.

Environmental Variables:
Device: Buri v1.4 Master Mozilla RIL
BuildID: 20140314093423
Gaia: 1cef557f9e9a865d1bf49d99a8f1cca1f0f4f5c4
Gecko: 142911d6d987
Version: 30.0a1
Firmware Version: v1.2-device.cfg

Comment 9

[Cristian Rodriguez (:crdlc)]

Thanks, I understand that, very helpful

(In reply to Etienne Segonzac (:etienne) from comment #3)
> Comment on attachment 8391152 [details]
> Patch v1
> 
> I don't think postMessage is the right approach here.
> 
> To make a call the contacts app uses the TelephonyHelper [1], and you can
> pass optional callbacks to the telephony helper. You can find an example
> here [2].
> 
> You probably want to disable the button until you get an onconnected or
> onerror or ondisconnected callback.
> 
> Bonus: no change required to the dialer app :D
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> js/contacts.js#L466
> [2]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/
> js/dialer.js#L254-L293

Comment 10

[Cristian Rodriguez (:crdlc)]

I gonna wait to implement this bug because I don't want to work more than once

(In reply to Francisco Jordano [:arcturus] from comment #4)
> We have a patch for DSDS that needs to land for 1.4 that will modify a bit
> this functionality, that is still needed.
> 
> You can track the process:
> https://github.com/mozilla-b2g/gaia/pull/17143

Comment 11

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v2

I followed the Etienne's suggestion and Jose's feedback

Comment 12

[Jose Manuel Cantera]

Comment on attachment 8392197 [details]
Patch v2

Tested this on the hamachi and works perfectly. And now the code is clean and terse. 

thanks for the work!

Comment 13

[Cristian Rodriguez (:crdlc)]

Rebased with latest changes on ActionButton

Comment 14

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

triage: should not block release as the user need to try breaking it but please ask for approval to land in 1.4 when the fix is avaialble. thanks

Comment 15

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

Comment on attachment 8392197 [details]
Patch v2

Just left a small comment, but great work here.
Tried with a DSDS device as well and working as expected.

Thanks Cristian!

Comment 16

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

Comment on attachment 8392197 [details]
Patch v2

w00t! forgot to r+!

Comment 17

[Cristian Rodriguez (:crdlc)]

Addressed my friend!

Comment 18

[Etienne Segonzac (:etienne)]

Comment on attachment 8392197 [details]
Patch v2

It's a contact only patch now, but definitely feedback+ !

Comment 19

[Cristian Rodriguez (:crdlc)]

Merged in master:

https://github.com/mozilla-b2g/gaia/commit/3977de3c913fedb331ad2445ad245501e3a62239

Comment 20

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8392197 [details]
Patch v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: medium thought
[Testing completed]: Jordano and me
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]:

Requested by comment 14

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Cristian, can you please expand on your answers to the uplift request below? (Specific feedback inline.)

(In reply to Cristian Rodriguez (:crdlc) from comment #20)

> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #):
Reading through this bug this appears to be a regression. Do you know the bug or the regression range?
> [User impact] if declined: medium thought
Can you be specific as to the user impact in this case? Note that the user impact may differ from the str after you have figured out how to fix the issue.
> [Testing completed]: Jordano and me
Please comment on what testing you completed. Manual and/or automated? (I see that the patch does included automated tests. Are there cases that the automated tests can't cover?) Can you provide details about what scenarios the tests cover?
> [Risk to taking this patch] (and alternatives if risky): low
Why is the risk low?
> [String changes made]:
It is appreciated if you can answer none if no string changes have been made in order to be explicit about this. In this case I see no string changes in the pr.

Comment 22

[Cristian Rodriguez (:crdlc)]

> Reading through this bug this appears to be a regression. Do you know the bug or the regression range?

I don't really think so. IMHO it was there for the beginning. Francisco?

> Can you be specific as to the user impact in this case? Note that the user impact may differ from the 
> str after you have figured out how to fix the issue

After clicking several times, a message is displayed with title "Unable to make a phone call now" and behind this message, the dialer is trying to make the call. After a couple of seconds the dialer is displayed on the top of this message. When the call is finished, the dialer disappears but the message is still there. This is the real impact for the user.

https://bug983594.bugzilla.mozilla.org/attachment.cgi?id=8391138

> Please comment on what testing you completed. Manual and/or automated? (I see that the patch does
> included automated tests. Are there cases that the automated tests can't cover?) Can you provide
> details about what scenarios the tests cover?

1. I was tested by ourself in our devices
2. I added some unit tests in order to check that call buttons are disabled when they are clicked the first time and they are restored when the call is successful or failed.

> Why is the risk low?

It is low because I provided with a set of tests and there are not a lot of changes in the code and it was reviewed by two people (one of them is the owner of contacts app)

> It is appreciated if you can answer none if no string changes have been made in order to be explicit
> about this. In this case I see no string changes in the pr

No idea what it is, translation strings? The answer is "none"

Comment 23

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

(In reply to Cristian Rodriguez (:crdlc) from comment #22)
> > Reading through this bug this appears to be a regression. Do you know the bug or the regression range?
> 
> I don't really think so. IMHO it was there for the beginning. Francisco?
> 

This is not a regression, the code have been always be the same. Perhaps now the attention screen is a bit slower to come out, but we never had specific code to prevent this happening, so this bug is, IMHO, needed in our code base.

Comment 24

[Noemí Freire (:noemi)]

verifyme to check it works in master before the uplift. Thanks!

Comment 25

[Noemí Freire (:noemi)]

Hi Ryan,

Could you please help us with the uplift to v1.4 branch?. Thanks a lot!.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/430b3efac648bdb2927cfd49b0e13baf73a7fb9e