Closed Bug 985117 Opened 7 years ago Closed 7 years ago

Voicemail setting retrieval is broken in System

Categories

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

x86_64
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S4 (28mar)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Keywords: regression, Whiteboard: [systemsfe],)

Attachments

(1 file)

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=["+33695600011"]
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="+33695600011"

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.
Blocks: 983459
This is a bug. We should use status.serviceId instead of simIndex for retrieving the voicemail number. :(
Gregor,

Please review.
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(anygregor)
Flags: needinfo?(anygregor)
Whiteboard: [systemsfe]
Probably DSDS regression. Can we find out if we have to fix this for tarako as well?
Keywords: qawanted
Whiteboard: [systemsfe] → [systemsfe],
(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.
Blocks: 975918
(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.
Please find attached a Github pull request that fixes the issue
Assignee: nobody → lissyx+mozillians
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.
Attachment #8394199 - Flags: review?(mhenretty)
Comment on attachment 8394199 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/17388

Arthur, do you mind reviewing this? Thanks!
Attachment #8394199 - Flags: review?(mhenretty) → review?(arthur.chen)
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.
Attachment #8394199 - Flags: review?(arthur.chen)
Attachment #8394199 - Flags: review?(alive)
Attachment #8394199 - Flags: feedback+
Attachment #8394199 - Flags: review?(alive) → review+
(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
Hi Alexandre, it should be "MockNavigatorSettings.mSet" instead of "MockNavigatorSettings.createLock().mSet".
(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
(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.
(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.
(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.
Travis is green: https://travis-ci.org/mozilla-b2g/gaia/builds/21255461

I'll land when the tree re-opens.
Flags: needinfo?(mhenretty)
master: https://github.com/mozilla-b2g/gaia/commit/7e25bb805660b462fc3f3ae93a5f026f263b694d
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(mhenretty)
Resolution: --- → FIXED
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.