Closed Bug 975918 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
1.4 S3 (14mar)

People

(Reporter: arthurcc, Assigned: arthurcc)

References

Details

Attachments

(1 file)

User should be able to see 2 different notifications for voicemails indicating each subscription's status.
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!
Attachment #8382001 - Flags: feedback?(josea.olivera)
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!
Attachment #8382001 - Flags: feedback?(josea.olivera)
I believe this is a duplicate of bug 965854. If it is, you can close the other one.
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.
Flags: needinfo?(anthony)
Duplicate of this bug: 965854
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!
Attachment #8382001 - Flags: review?(josea.olivera)
Summary: [DSDS][Gaia] Need to show the Voicemail notifications. → [DSDS][Gaia] Voicemail support for multi sim devices
(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!
Flags: needinfo?(anthony)
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!
Attachment #8382001 - Flags: review?(etienne)
Attachment #8382001 - Flags: review?(alive)
Duplicate of this bug: 960387
Target Milestone: 1.4 S2 (28feb) → 1.4 S3 (14mar)
Duplicate of this bug: 940752
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|
Attachment #8382001 - Flags: review?(etienne)
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!
Attachment #8382001 - Flags: review?(josea.olivera)
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!
Attachment #8382001 - Flags: review?(josea.olivera)
Attachment #8382001 - Flags: review?(etienne)
Duplicate of this bug: 979736
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]...
Attachment #8382001 - Flags: review?(etienne)
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.
(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?
(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?
Flags: needinfo?(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).
Flags: needinfo?(etienne)
Full gaia flash (make reset-gaia) should clean the settings db, there should be no migration needed.
(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.
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 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.
Attachment #8382001 - Flags: review?(josea.olivera) → review+
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!
Attachment #8382001 - Flags: review?(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.
Attachment #8382001 - Flags: review?(etienne)
Comment on attachment 8382001 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/16652

Patch updated. Could you review it again? Thanks!
Attachment #8382001 - Flags: review?(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 :)
Attachment #8382001 - Flags: review?(etienne) → review+
Thanks Jose and Etienne!

master: 99a31271ff6eea20cb1aa52bee7086664a370f73
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
back out per comment 30.

53eba1b36f2379d52ef6e18e4b7d32292f32b860
This is another instance of bug 978103. Arthur, you need to move the mock in shared.
Thanks for the information!
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.
Travis green: https://github.com/mozilla-b2g/gaia/pull/17056
master: c9b617285bccc0556db5976b9ad69edfc78abf8c
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
QA Contact: atsai
Keywords: verifyme
(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.
I did not saw this comment when commented on bug 985090. Let me try OTA on my side.
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
Depends on: 985311
Keywords: regression
This is a feature, not a regression in itself. Tag the dependency with regression if it's a regression from this patch.
Keywords: regression
Depends on: 985117
No longer depends on: 985273
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.