985117 - Voicemail setting retrieval is broken in System

Status: RESOLVED FIXED

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

Description

[:gerard-majax]

Following bug 985090, and investigating bug 983459, I noticed that the voicemail retrieval seems broken.

When you receive a voicemail notification from mozVoicemail, we send a notification. Building this one we retrieve the voicemail number from settings, and if we have none, then we try to use the voicemail number from the mozVoicemail (i.e., from the SIM card).

Adding some debug to callback of this.voiceMailNumberHelper.get in apps/system/js/voicemail.js:
+  58       console.debug('numbers=' + JSON.stringify(numbers));
   59       var voicemail = navigator.mozVoicemail;
   60       var number = numbers && numbers[simIndex];
+  61       console.debug('setting number=' + JSON.stringify(number));
   62 
'  63       if (!number && voicemail) {
   64        number = voicemail.getNumber(status.serviceId);
+  65        console.debug('voicemail number=' + JSON.stringify(number));
   66       }

I get this output:
E/GeckoConsole( 3680): Content JS DEBUG at app://system.gaiamobile.org/js/voicemail.js:58 in gotVMNumbers: numbers=["[phone]"]
E/GeckoConsole( 3680): Content JS DEBUG at app://system.gaiamobile.org/js/voicemail.js:61 in gotVMNumbers: setting number=undefined
E/GeckoConsole( 3680): Content JS DEBUG at app://system.gaiamobile.org/js/voicemail.js:65 in gotVMNumbers: voicemail number="[phone]"

Which exposes that the 'numbers' array is potentially correctly used. My device has only one SIM (Nexus S), and I had to workaround in bug 985090 to get the numbers array.

The simIndex is built as |var simIndex = status.serviceId + 1;|

Which, in my case, produces:
E/GeckoConsole( 3680): Content JS DEBUG at app://system.gaiamobile.org/js/voicemail.js:38 in vm_updateNotification: status.serviceId=0
E/GeckoConsole( 3680): Content JS DEBUG at app://system.gaiamobile.org/js/voicemail.js:39 in vm_updateNotification: simIndex=1

I'm not sure it makes sense to query index 1 in an array on a single device SIM.

Comment 1

[Arthur Chen [:arthurcc][inactive after 6/30]]

This is a bug. We should use status.serviceId instead of simIndex for retrieving the voicemail number. :(

Comment 2

[Preeti Raghunath(:Preeti)]

Gregor,

Please review.

Comment 3

[Gregor Wagner [:gwagner]]

Probably DSDS regression. Can we find out if we have to fix this for tarako as well?

Comment 4

[Jason Smith [:jsmith]]

(In reply to Gregor Wagner [:gwagner] from comment #3)
> Probably DSDS regression. Can we find out if we have to fix this for tarako
> as well?

We need STR to be able to test this. Although to my understanding, the suspected cause of this was bug 975918, which only landed on 1.4.

Comment 5

[:gerard-majax]

(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to Gregor Wagner [:gwagner] from comment #3)
> > Probably DSDS regression. Can we find out if we have to fix this for tarako
> > as well?
> 
> We need STR to be able to test this. Although to my understanding, the
> suspected cause of this was bug 975918, which only landed on 1.4.

No, this is another issue where we do not make use of the correct index to access the array.

Comment 6

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/17388

Please find attached a Github pull request that fixes the issue

Comment 7

[:gerard-majax]

Comment on attachment 8394199 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/17388

Updated the PR with a new test.

The issue is only exposed when retrieving voicemail from the settings.

Comment 8

[:gerard-majax]

Pending travis at https://travis-ci.org/mozilla-b2g/gaia/builds/21189945

Comment 9

[:gerard-majax]

Comment on attachment 8394199 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/17388

Arthur, do you mind reviewing this? Thanks!

Comment 10

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8394199 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/17388

Thanks for the patch. f=me. Flagging the system app owner.

Comment 11

[:gerard-majax]

(In reply to Arthur Chen [:arthurcc] from comment #10)
> Comment on attachment 8394199 [details] [review]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/17388
> 
> Thanks for the patch. f=me. Flagging the system app owner.

Thanks arthur and alive. The nit has been addressed, a new travis is pending at https://travis-ci.org/mozilla-b2g/gaia/builds/2124118

Comment 12

[Arthur Chen [:arthurcc][inactive after 6/30]]

Hi Alexandre, it should be "MockNavigatorSettings.mSet" instead of "MockNavigatorSettings.createLock().mSet".

Comment 13

[:gerard-majax]

(In reply to Arthur Chen [:arthurcc] from comment #12)
> Hi Alexandre, it should be "MockNavigatorSettings.mSet" instead of
> "MockNavigatorSettings.createLock().mSet".

Should be fixed in https://travis-ci.org/mozilla-b2g/gaia/builds/21241901

Comment 14

[:gerard-majax]

(In reply to Arthur Chen [:arthurcc] from comment #12)
> Hi Alexandre, it should be "MockNavigatorSettings.mSet" instead of
> "MockNavigatorSettings.createLock().mSet".

This does not seems to be working.

Comment 15

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #14)
> (In reply to Arthur Chen [:arthurcc] from comment #12)
> > Hi Alexandre, it should be "MockNavigatorSettings.mSet" instead of
> > "MockNavigatorSettings.createLock().mSet".
> 
> This does not seems to be working.

i.e., MockNavigatorSettings.mSet() but then the resulting showNotification calls are not done with the correct values.

Comment 16

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #15)
> (In reply to Alexandre LISSY :gerard-majax from comment #14)
> > (In reply to Arthur Chen [:arthurcc] from comment #12)
> > > Hi Alexandre, it should be "MockNavigatorSettings.mSet" instead of
> > > "MockNavigatorSettings.createLock().mSet".
> > 
> > This does not seems to be working.
> 
> i.e., MockNavigatorSettings.mSet() but then the resulting showNotification
> calls are not done with the correct values.

The correct solution was to use the mock for SettingsHelper. This has made the code to change a bit. If you think it needs another review, feel free.

Comment 17

[Michael Henretty [:mikehenrty][:mhenretty]]

Travis is green: https://travis-ci.org/mozilla-b2g/gaia/builds/21255461

I'll land when the tree re-opens.

Comment 18

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/7e25bb805660b462fc3f3ae93a5f026f263b694d

Comment 19

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/6d6ca750be162bf7b154c5773253b8762e172567