+++ 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 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.

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.

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

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.

(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 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.

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.

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

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

(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 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 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.

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