Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Description • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 1 • 11 years ago

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!

	José Antonio Olivera Ortega [:jaoo]
	Comment 2 • 11 years ago

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!

	Anthony Ricaud (:rik)
	Comment 3 • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 4 • 11 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 6 • 11 years ago

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!

	Anthony Ricaud (:rik)
	Comment 7 • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 8 • 11 years ago

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!

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 9 • 11 years ago

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

r+ for voicemail

	Etienne Segonzac (:etienne)
	Comment 12 • 11 years ago

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|

	José Antonio Olivera Ortega [:jaoo]
	Comment 13 • 11 years ago

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!

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 14 • 11 years ago

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!

	Etienne Segonzac (:etienne)
	Comment 16 • 11 years ago

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

	José Antonio Olivera Ortega [:jaoo]
	Comment 17 • 11 years ago

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.

	José Antonio Olivera Ortega [:jaoo]
	Comment 18 • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 19 • 11 years ago

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

	Etienne Segonzac (:etienne)
	Comment 20 • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 21 • 11 years ago

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

	Etienne Segonzac (:etienne)
	Comment 22 • 11 years ago

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

	José Antonio Olivera Ortega [:jaoo]
	Comment 23 • 11 years ago

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

	José Antonio Olivera Ortega [:jaoo]
	Comment 24 • 11 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 25 • 11 years ago

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!

	Etienne Segonzac (:etienne)
	Comment 26 • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 27 • 11 years ago

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

Patch updated. Could you review it again? Thanks!

	Etienne Segonzac (:etienne)
	Comment 28 • 11 years ago

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 :)

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 29 • 11 years ago

Thanks Jose and Etienne!

master: 99a31271ff6eea20cb1aa52bee7086664a370f73

	Nigel Babu [:nigelb]
	Comment 30 • 11 years ago

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?

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 31 • 11 years ago

back out per comment 30.

53eba1b36f2379d52ef6e18e4b7d32292f32b860

	Anthony Ricaud (:rik)
	Comment 32 • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 33 • 11 years ago

Thanks for the information!

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 34 • 11 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 35 • 11 years ago

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

	:gerard-majax [PTO 01/08/2023-10/09/2023]
	Comment 36 • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 37 • 11 years ago

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

	Eric Chang [:ericcc] [:echang]
	Comment 38 • 11 years ago

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

	Jason Smith [:jsmith]
	Comment 39 • 11 years ago

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

	Eric Chang [:ericcc] [:echang]
	Comment 40 • 11 years ago

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