1093029 - Filter short-string MMI codes depending on call state

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto] (PTO)]

+++ This bug was initially created as a clone of Bug #1092161 +++

Bug 1092161 introduced detecting short-string MMI codes however after a discussion with Hsin-Yi on IRC we figured out that the support there is incomplete. 3GPP TS 22.030 [1] 6.5.3.2 also mandates different handling if there is an ongoing call or not. We'll use this bug to add those further checks.

[1] http://www.3gpp.org/DynaReport/22030.htm

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Do not treat two-digit phone numbers starting with 1 as MMI codes when outside of a call

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] Do not treat two-digit phone numbers starting with 1 as MMI codes when outside of a call

Comment 3

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8515944 [details] [diff] [review]
[PATCH] Do not treat two-digit phone numbers starting with 1 as MMI codes when outside of a call

Review of attachment 8515944 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/communications/dialer/js/mmi.js
@@ +417,3 @@
>        return (number.charAt(number.length - 1) === '#') ||
> +             ((number.length <= 2) &&
> +              !((number.length == 2) && !onCall && number.startsWith('1')));

This is really confusing. We should break it up into clearly labeled variables instead of having this really long condition.

::: apps/communications/dialer/test/unit/mmi_test.js
@@ +59,5 @@
>  var mocksHelperForMMI = new MocksHelper([
>    'LazyL10n',
>    'LazyLoader',
>    'MobileOperator',
> +  'NavigatorMozTelephony',

You shouldn't need this.

@@ +286,5 @@
>      realMozIccManager = window.navigator.mozIccManager;
>      window.navigator.mozIccManager = MockNavigatorMozIccManager;
>  
> +    realMozTelephony = window.navigator.mozTelephony;
> +    window.navigator.mozTelephony = MockNavigatorMozTelephony;

The `MockNavigatorMozTelephony` class has both `mTeardown()` and `mSuiteTeardown()` functions that we should call. See my comment about breaking up a test below, as we will need to make this change to support that one.

@@ +359,5 @@
> +      test('Check an MMI single-digit short-string code', function() {
> +        assert.isTrue(MmiManager.isMMI('1', 0));
> +      });
> +
> +      test('Check an MMI double-digit short-string code', function() {

'Check a non-1X MMI double-digit short-string code'

@@ +373,5 @@
> +        assert.isTrue(MmiManager.isMMI('11', 0));
> +
> +        MockNavigatorMozTelephony.calls = [];
> +        MockNavigatorMozTelephony.conferenceGroup.calls = [{}];
> +        assert.isTrue(MmiManager.isMMI('11', 0));

This should be broken up into two tests.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

Thanks for the review, just a quick comment, I'll update the patch tomorrow:

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #3)
> This is really confusing. We should break it up into clearly labeled
> variables instead of having this really long condition.

Yeah, good point.

> You shouldn't need this.
> [...]
> The `MockNavigatorMozTelephony` class has both `mTeardown()` and
> `mSuiteTeardown()` functions that we should call.

The MocksHelper ensures that mTeardown() and mSuiteTeardown() are called automatically when appropriate. It's better than calling them manually.

> This should be broken up into two tests.

OK, will do.

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v2] Do not treat two-digit phone numbers starting with 1 as MMI codes when outside of a call

Updated patch, the condition is clearer now and the unit-tests have been split.

Comment 6

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8516628 [details] [diff] [review]
[PATCH v2] Do not treat two-digit phone numbers starting with 1 as MMI codes when outside of a call

Review of attachment 8516628 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/communications/dialer/js/mmi.js
@@ +410,5 @@
> +      var telephony = navigator.mozTelephony;
> +      var onCall = telephony && !!(telephony.calls.length ||
> +                                   telephony.conferenceGroup.calls.length);
> +      var shortString = (number.length <= 2);
> +      var startsWithOne = (number.length === 2) && number.startsWith('1');

s/startsWithOne/doubleDigitAndStartsWithOne/

::: apps/communications/dialer/test/unit/mmi_test.js
@@ +59,5 @@
>  var mocksHelperForMMI = new MocksHelper([
>    'LazyL10n',
>    'LazyLoader',
>    'MobileOperator',
> +  'NavigatorMozTelephony',

Even though this calls the `mSuiteTeardown()` and `mTeardown()` functions for us, it is highly unusual to include a `navigator` object in this section. This also has the side effect of calling `window.NavigatorMozTelephony = MockNavigatorMozTelephony`, which is not useful here. The classes passed into `MocksHelper` should be reserved for only the subset which is used by the code that is being tested.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v3] Do not treat two-digit phone numbers starting with 1 as MMI codes when outside of a call

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #6)
> Even though this calls the `mSuiteTeardown()` and `mTeardown()` functions
> for us, it is highly unusual to include a `navigator` object in this
> section. This also has the side effect of calling
> `window.NavigatorMozTelephony = MockNavigatorMozTelephony`, which is not
> useful here. The classes passed into `MocksHelper` should be reserved for
> only the subset which is used by the code that is being tested.

OK, I hadn't thought about that, I just learned something new about how to write unit-tests :)

Comment 8

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8517005 [details] [diff] [review]
[PATCH v3] Do not treat two-digit phone numbers starting with 1 as MMI codes when outside of a call

Review of attachment 8517005 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/communications/dialer/test/unit/mmi_test.js
@@ +354,5 @@
>  
>          mobileConn.supportedNetworkTypes = ['cdma', 'evdo'];
>          mobileConn.voice = { type: 'evdoa' };
>          assert.isFalse(MmiManager.isMMI('*#06#', 0));
>        });

Please fix the indentation of this test.

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

The green try run is here: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=f0f1e06d0249

Pushed to gaia/master 82d2f18154a29c306c6458bac38af8d0b471db1a

https://github.com/mozilla-b2g/gaia/commit/82d2f18154a29c306c6458bac38af8d0b471db1a

Since bug 1087241 is not a blocker anymore I don't think we should uplift this. It can always be done at a later time since it applies cleanly to both 2.0 and 2.1.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

I forgot to add, I addressed the final nit mentioned in comment 8 before pushing.

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

Wei Gao, can you cherry-pick this patch to the v2.1 branch and see if it fixes bug 1117461 for you? This command will be enough to pick the correct patch as the fix here applies cleanly to v2.1:

git cherry-pick -x 82d2f18154a29c306c6458bac38af8d0b471db1a

Comment 13

[Wei Gao (Spreadtrum)]

(In reply to Gabriele Svelto [:gsvelto] from comment #12)
> Wei Gao, can you cherry-pick this patch to the v2.1 branch and see if it
> fixes bug 1117461 for you? This command will be enough to pick the correct
> patch as the fix here applies cleanly to v2.1:
> 
> git cherry-pick -x 82d2f18154a29c306c6458bac38af8d0b471db1a

Dear Gabriele

Yes, It can clearly fix my issue.
Thanks a lot.

Comment 14

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8517005 [details] [diff] [review]
[PATCH v3] Do not treat two-digit phone numbers starting with 1 as MMI codes when outside of a call

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1092161
[User impact] if declined: Dialing a 1x number will send an MMI message instead of making a regular call, this is wrong and was noticed by our partners (see bug 1117461)
[Testing completed]: This patch lived for a while in master, is covered with unit-tests and was also tested on a device, see comment 12
[Risk to taking this patch] (and alternatives if risky): This might affect calling 2-character numbers and sending MMI codes of the same format
[String changes made]: None

Comment 15

[bhavana bajaj [:bajaj]]

Comment on attachment 8517005 [details] [diff] [review]
[PATCH v3] Do not treat two-digit phone numbers starting with 1 as MMI codes when outside of a call

Approving given 1092161 was landed on 2.1.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/175a07e59aa78d71f8a965c0af929b5e5206a5c3