975918 - [DSDS][Gaia] Voicemail support for multi sim devices

Status: VERIFIED FIXED

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

Description

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

User should be able to see 2 different notifications for voicemails indicating each subscription's status.

Comment 1

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

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

Jose, do you mind take a look at this patch? Which was made based on our discussion yesterday. I'll keep modifying other related parts. (dialer for example) Thanks!

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

Thanks for the WIP patch as it allows me to provide feedback better. Left some comments.

IHMO since you are touching the voicemail thing all over Gaia I would change the title of the bug. Something like 'Voicemail support for multi ICC card devices on Gaia side', but it's up to you!

Comment 3

[Anthony Ricaud (:rik)]

I believe this is a duplicate of bug 965854. If it is, you can close the other one.

Comment 4

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

Do you have another bug tracking the dialer part (long press on the voice mail key)? I will only make it call to the VM number of the first icc card in this patch.

Comment 6

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

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

Comments addressed and test cases added. Do you mind take a look at the patch? Please also see my comments in github, thanks!

Comment 7

[Anthony Ricaud (:rik)]

(In reply to Arthur Chen [:arthurcc] from comment #4)
> Do you have another bug tracking the dialer part (long press on the voice
> mail key)? I will only make it call to the VM number of the first icc card
> in this patch.
Now we do: bug 978114. Thanks for pointing that out!

Comment 8

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

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

Alive, Etienne, could you help review the system app and dialer parts? Thanks!

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

r+ for voicemail

Comment 12

[Etienne Segonzac (:etienne)]

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

Looking good, but I think we should still handle the case where |request.result['ril.iccInfo.mbdn']| is a string to avoid migration issues.

Also, do we have a follow bug already open to honor the user default voice sim card setting? I'd like to mention the URL next to the |// XXX: always use the first icc card|

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

Arthur, I left a comment on the PR. Could you take a look at it please? Thanks!

Comment 14

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

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

Jose, I've already merged your patch, could you take a look at it again?

Etienne, the migration logic was added in operator_variant.js, and the comment was also added. Please help check it, thanks!

Comment 16

[Etienne Segonzac (:etienne)]

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

When is the migration code supposed to be executed?

Just tested (with a full flash) on my phone, and the issue I feared happened : instead of calling/matching "888" (my voicemail number) it dials 8...
So "888"[0]...

Comment 17

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

I went through the changes in the operator variant logic and the settings app. It's looking good but I won't give r=me until I test this patch on a fugu device. I'll do it tomorrow ASAP as I don't have a fugu device right now. Sorry for the inconveniences.

Comment 18

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Etienne Segonzac (:etienne) from comment #16)
> Comment on attachment 8382001 [details]
> Link to https://github.com/mozilla-b2g/gaia/pull/16652
> 
> When is the migration code supposed to be executed?

If you flashed the device the `ril.iccInfo.mbdn` setting should be an array by the time you dial the VM number by long pressing the VM key and the VM number should be the one for the ICC being in use for MO calls. I saw in the PR that the dialer always dials the one for the first ICC card and this is not right, isn't it?

Comment 19

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

(In reply to Etienne Segonzac (:etienne) from comment #16)
> Comment on attachment 8382001 [details]
> Link to https://github.com/mozilla-b2g/gaia/pull/16652
> 
> When is the migration code supposed to be executed?
> 
> Just tested (with a full flash) on my phone, and the issue I feared happened
> : instead of calling/matching "888" (my voicemail number) it dials 8...
> So "888"[0]...

I'm not able to reproduce this. After a full flash, there should be no migration as the `ril.iccInfo.mbdn` is cleaned up when flashing. And then the field will be set as an array in operator_variant.js when a new icc card is detected. Could you share your STR?

Comment 20

[Etienne Segonzac (:etienne)]

(In reply to Arthur Chen [:arthurcc] from comment #19)
> (In reply to Etienne Segonzac (:etienne) from comment #16)
> > Comment on attachment 8382001 [details]
> > Link to https://github.com/mozilla-b2g/gaia/pull/16652
> > 
> > When is the migration code supposed to be executed?
> > 
> > Just tested (with a full flash) on my phone, and the issue I feared happened
> > : instead of calling/matching "888" (my voicemail number) it dials 8...
> > So "888"[0]...
> 
> I'm not able to reproduce this. After a full flash, there should be no
> migration as the `ril.iccInfo.mbdn` is cleaned up when flashing. And then
> the field will be set as an array in operator_variant.js when a new icc card
> is detected. Could you share your STR?

Sorry this was really misleading.
I meant a full gaia flash (all the apps including the system one).

Comment 21

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

Full gaia flash (make reset-gaia) should clean the settings db, there should be no migration needed.

Comment 22

[Etienne Segonzac (:etienne)]

(In reply to Arthur Chen [:arthurcc] from comment #21)
> Full gaia flash (make reset-gaia) should clean the settings db, there should
> be no migration needed.

Yeah but that not what I want to test :)
I want to make sure the migration worked, and that after updating gaia on a phone in the field, we won't call "8" instead of "888" for voicemail.

Comment 23

[José Antonio Olivera Ortega [:jaoo]]

Let me clarify I bit when the migration code (code lives at operator_variant.js). The migration code is supposed to be run for the following cases:

- The device boots for the first time after flashing the device.
- The device boots for the first time after |make reset-gaia|.
- A new ICC card (seeing a different operator numeric value which is MCC and MNC codes) is seen on boot.
- The OS version (checking the 'deviceinfo.os' setting) changes (even if the ICC seen is the known one). This is the case for OTA/FOTA updates.

The migration code *won't* run for cases like first boot after |make install-gaia|.

Comment 24

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

LGTM, thanks Arthur for the work here. r=me

I've tested the patch with Peak device and Fugu device. Everything works as expected and nothing seems to be broken.

Please land at will.

Comment 25

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

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

Per comment 24, the migration cases are being handled. Please let me know if there are any concerns, thanks!

Comment 26

[Etienne Segonzac (:etienne)]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #23)
> The migration code *won't* run for cases like first boot after |make
> install-gaia|.

Sorry to be a pain guys, but this happens a lot.
And I don't want to deal with the Dialer bugs marked as regressions because we take the first char of a string for voicemail number...

Let's add the simple "typeof numbers == 'string'" cases to the dialer, and revisit in bug 978114.

Comment 27

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

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

Patch updated. Could you review it again? Thanks!

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

Thanks Arthur, r=me on the dialer part, and I'll sleep much better tonight :)

Comment 29

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

Thanks Jose and Etienne!

master: 99a31271ff6eea20cb1aa52bee7086664a370f73

Comment 30

[Nigel Babu [:nigelb]]

This seems to have caused opt test gaia-unit test failures.

https://tbpl.mozilla.org/php/getParsedLog.php?id=35862675&tree=B2g-Inbound

Arthur, any idea what's going on?

Comment 31

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

back out per comment 30.

53eba1b36f2379d52ef6e18e4b7d32292f32b860

Comment 32

[Anthony Ricaud (:rik)]

This is another instance of bug 978103. Arthur, you need to move the mock in shared.

Comment 33

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

Thanks for the information!

Comment 34

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

Root cause found. The mocks used in the tests have been moved to the /js folder in bug 975161 although I don't know the reason. Waiting for the decision there.

Comment 35

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

Travis green: https://github.com/mozilla-b2g/gaia/pull/17056
master: c9b617285bccc0556db5976b9ad69edfc78abf8c

Comment 36

[:gerard-majax]

(In reply to Arthur Chen [:arthurcc] from comment #25)
> Comment on attachment 8382001 [details]
> Link to https://github.com/mozilla-b2g/gaia/pull/16652
> 
> Per comment 24, the migration cases are being handled. Please let me know if
> there are any concerns, thanks!

Obviously not, as documented in bug 985090

I'm reproducing on several devices: Nexus S, Keon, Peak, Desire Z. All updated via OTA/FOTA.

Comment 37

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

I did not saw this comment when commented on bug 985090. Let me try OTA on my side.

Comment 38

[Eric Chang [:ericcc] [:echang]]

Just found a bug which breaks the AC2 of 940752
https://bugzilla.mozilla.org/show_bug.cgi?id=985311
[DSDS][Fugu] Tapping voicemail notification for SIM2 and it calls out from default SIM set in Sim manager

Comment 39

[Jason Smith [:jsmith]]

This is a feature, not a regression in itself. Tag the dependency with regression if it's a regression from this patch.

Comment 40

[Eric Chang [:ericcc] [:echang]]

Works fine with v1.4 on Fugu.

http://youtu.be/QJr3RAY6o0Y

GAIA_REV=3534b4880653f24529c973ec2f09ef51a3d6392f
GECKO_REV=b6f99179f940f4e0f9f83a4b09f438618c269330
GAIA_BRANCH=mozillaorg/v1.4
GECKO_BRANCH=mozillaorg/v1.4
BUILD_TAG=jenkins-B2G.v1.4.0.fugu-24
BuildID=20140415041552