971816 - [OTA][Data Migration] Voicemail number will restored back default setting in SIM card after updating from v1.1 to v1.3

Status: RESOLVED FIXED

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

Description

[Hubert Lu[:hlu] <[e-mail]>]

Attachment: logcat.txt

- Base ROM version -
    Gaia      53e2a70d85fb3748d0768218a5efffe5806073f0
    Gecko     http://hg.mozilla.org/releases/mozilla-b2g18/rev/c630289d6388
    BuildID   20131009041203
    Version   18.0
    ro.build.version.incremental=eng.cltbld.20131009.072332
    ro.build.date=Wed Oct  9 07:45:58 EDT 2013

* Reproduce Steps
1. Flash device to v1.1 build from PVT
2. Change update URL to http://update.boot2gecko.org/inari/1.3.0/nightly/update_20140211190620.xml
3. Go to settings->Call settings
4. Set up the voicemail number
5. Update the device to v1.3

* Actual results:
1. Voicemail number will be empty after update.

* Expected result:
1. All settings should be kept after update.

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

This is weird. The VM number seen in the call settings panel is stored in the 'ril.iccInfo.mbdn' setting and if you don't see it after updating it might mean the 'ril.iccInfo.mbdn' setting is lost/erased somehow. I have a couple of questions: i) is the VM number stored in the ICC card you are using?, ii) are you able to dial the VM service by long-pressing the '1' key in the dialer app?

Comment 2

[Hubert Lu[:hlu] <[e-mail]>]

For question 1, I am not sure how to make sure if the VM number stored in ICC card. 
[Before update] I modify the VM number from default "777" to my personal number, and it really dails to my phone mumber while long-pressing the "1" key in dialer app.
[After update] The VM will be restored back "777", and it will dail to "777" by long-pressing "1" key. 

For question 2, it will dail the VM number configurated in settings by long-pressing the "1" key in the dialer app.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Hubert Lu[:hlu] <[e-mail]> from comment #2)
> For question 1, I am not sure how to make sure if the VM number stored in
> ICC card.

You could see the RIL logs or ask the customer service of the carrier for the ICC card.

I suspect the VM number is stored in the ICC card you are using because we don't have that number in the apn.json database (FYI the apn.json database has the VM number for some ICC cards and we use it as fallback if the VM number is not stored in the ICC card),

As I see in comment #2 after updating the VM number is not empty, is it? If the VM number is stored in the SIM you should see in the settings app even after updating to v1.3 release.

Comment 4

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

After confirmed with Hubert, the VM number is stored in one of the sim cards that he had used. The issue is, after OTA, the VM number is reset to the number stored in the installed sim card even users had changed the VM number. If there is no VM number stored in the sim card, it becomes empty. 

Is there any chance that even we have a VM number stored in "ril.iccInfo.mbdn" of mozSettings but still gets override after OTA?

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Arthur Chen [:arthurcc] from comment #4)

> Is there any chance that even we have a VM number stored in
> "ril.iccInfo.mbdn" of mozSettings but still gets override after OTA?

Yes, it is. The VM number in the 'ril.iccInfo.mbdn' settings gets populated by the operator variant logic on boot when a new ICC card is seen. I mean, every time the user changes the ICC card the 'ril.iccInfo.mbdn' settings takes the value of the VM number for the new ICC card. We check whether a new ICC card has been inserted by comparing the MCC and MNC codes with the ones from the ICC card previously seen in the device. These MCC and MNC codes are stored into the setting database under the 'operatorvariant.mcc' and 'operatorvariant.mnc' settings respectively. The big change between v1.1 and v1.3 releases for these settings is that they have been become an array due the multi ICC card features. This will cause the operator variant logic to run on the first boot after updating from v1.1 to v1.3 release (even with the same ICC card) and the 'ril.iccInfo.mbdn' to be restored to the default value.

Due the multi ICC card features in v1.3 release the operator variant logic must run on the first boot after upgrading. I've been thinking a fix for that and to be honest I'm out of ideas.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Could we possibly change voicemail variant updates to only happen if the voicemail setting is blank in the first place?

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #6)
> Could we possibly change voicemail variant updates to only happen if the
> voicemail setting is blank in the first place?

Sadly that cannot work because the VM number must change when a new SIM is seen. If we go for that solution we would not update/change it if the VM number was set for the previously ICC card seen.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Do we have any sort of last known good mnc/mcc pair we can check against to make sure this isn't run if it's the same between upgrades?

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

This patch would solve the issue since the OperatorVariantHandler.applySettings() function wouldn't be called and the VM number wouldn't be reset to the default one. It would keep the one the user set. This patch has a downside effect which is not populate ril.data.apnSettings setting. The ril.data.apnSettings was added in v1.2 and OperatorVariantHandler.applySettings() is charge of populating it. I'll try to get a fix for this downside effect as well. I'm not taking this bug as I have more blockers to fix before 2/28 but I'll try to fix it today.

Comment 10

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

This version of the patch should solve the downside effect of the previous version of the patch I commented about in comment #c9. I'll try to add unit tests for the change. Hub, could you test if this patch solves the issue in the mean time? Thanks!

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v3

This version of the patch should solve the downside effect of the v1 patch commented about in comment #9 and solves a failure of v2 patch. I'll try to add unit tests for the change. In the mean time, Hub, could you check whether this version of the patch solves the issue you reported? Thanks!

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v4

Test added.

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Pointer to Github PR https://github.com/mozilla-b2g/gaia/pull/16631

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v5

Last version of the WIP patch, this one is ready for review. Kyle, as you've involved in the discussion and you have a pretty good knowledge of the operator variant logic I'm wondering if you could take a look at the patch. The solution for the operator variant logic run after updates has been solved by using the persist setting key. This key is now based in the OS version underneath. If the ICC card seen is the known one we run customization again if the persist setting key changes. This allows us to not run customization for settings already set and to run customizations for those settings that have been added in the latest OS version and to run customizations for those ones that need to be overwritten/re-set. This is a bit of background on the change added I'll be happy any help you need. Thanks.

I'd would be great a doble-check from any other side than mine. Maybe Hub might help as he reported the issue.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8382180 [details] [diff] [review]
v5

Review of attachment 8382180 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r+ with nits addressed.

::: shared/js/operator_variant_helper.js
@@ +128,5 @@
> +      if (!mccs) {
> +        this._iccSettings.mcc = '000';
> +      } else if (!Array.isArray(mccs)) {
> +        this._iccSettings.mcc = mccs;
> +      } else if (!mccs[this._iccCardIndex]) {

Nit: Why not || this with the !mccs check and short circuit?

@@ +141,5 @@
> +          this._iccSettings.mnc = '00';
> +        } else if (!Array.isArray(mncs)) {
> +          this._iccSettings.mnc = mncs;
> +        } else if (!mncs[this._iccCardIndex]) {
> +          this._iccSettings.mnc = '00';

Nit: Same question as above except mncs this time :)

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8382180 [details] [diff] [review]
v5

Adding aus as a fb? since he did a lot of work on operator variant too.

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

BTW, using the OS key is a good idea. That's how we're doing update checks in the FTE for launching the edge gesture tutorial on update also, so it's a pattern that seems agreed upon for the moment.

Comment 18

[Aus Lacroix [:aus]]

Comment on attachment 8382180 [details] [diff] [review]
v5

Looks reasonable to me.

Comment 19

[Hubert Lu[:hlu] <[e-mail]>]

Have verifed the v4 and v5 patch, and the bug is fixed.

Comment 20

[José Antonio Olivera Ortega [:jaoo]]

Thank you guys. I'll try to land this ASAP, sorry for the delay.

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

Test passes, travis went green and everything seems to work correctly. Patch merged on Gaia master branch at https://github.com/mozilla-b2g/gaia/commit/104fe6838d226ceb21cf0834e577918918baaf84.

Comment 22

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8382180 [details] [diff] [review]
v5

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):

None

[User impact] if declined:

User preference value (VM number) lost after update.

[Testing completed]:

Yes, even new unit test added for the chance introduced.

[Risk to taking this patch] (and alternatives if risky):

Low.

[String changes made]:

None.

Comment 23

[Noemí Freire (:noemi)]

Asking for verify to land this in 1.3. Thanks!

Comment 24

[Noemí Freire (:noemi)]

(In reply to Noemí Freire (:noemi) from comment #23)
> Asking for verify to land this in 1.3. Thanks!

once landed in 1.3!

Comment 25

[Roland Kunkel [:RolandK]]

Asking for clarification. 

1) What is the proper way to verify this issue does not reproduce in the latest v1.4 build?
   - Do I need to start from v1.1 and flash to v1.4? 

2) What does your second step in your STR mean? Is this done through an update channel script or through other means.

Comment 26

[Jason Smith [:jsmith]]

(In reply to Roland Kunkel [:RolandK] from comment #25)
> Asking for clarification. 
> 
> 1) What is the proper way to verify this issue does not reproduce in the
> latest v1.4 build?
>    - Do I need to start from v1.1 and flash to v1.4? 
> 
> 2) What does your second step in your STR mean? Is this done through an
> update channel script or through other means.

Let's let Hubert handle the testing here - he's specializes in OTA testing.

Hubert - Can you verify this?

Comment 27

[Hubert Lu[:hlu] <[e-mail]>]

(In reply to Roland Kunkel [:RolandK] from comment #25)
> Asking for clarification. 
> 
> 1) What is the proper way to verify this issue does not reproduce in the
> latest v1.4 build?
>    - Do I need to start from v1.1 and flash to v1.4? 
> 
> 2) What does your second step in your STR mean? Is this done through an
> update channel script or through other means.

Hi Roland,

For Q1, the answer is YES. Because the migration action is handled by targeted version. So we need to configure the setting in v1.1 first, then update to targeted version,v1.3 or v1.4, with solution and check if the issue has been fixed or not. (I will handle it and update the result it later.)

For Q2, step2 is going to change the update URL. The default update URL is "http://update.boot2gecko.org/JrdDisableMozSysUp/update.xml", but the update xml is not stored there. So we need to change it to correct location via script. The script,change_ota_url.sh, is stored in "https://github.com/Mozilla-TWQA/B2G-flash-tool"

Hi Jose,
    The flag is marked as "1.3+", and I saw the patch is merged on master in comment 21. I think the patch should be also merged on v1.3 branch. Could you help it to merge the patch on v1.3 branch? Thanks.

Comment 28

[Jason Smith [:jsmith]]

Hubert - We can't merge this to 1.3 yet. We need to verify that this patch works on trunk first.

Comment 29

[Hubert Lu[:hlu] <[e-mail]>]

This bug could not be found in master build, and below is the gaia/gecko information.

Gaia      4bb8b32d8f1a3a64bfac8f2da7878dc63e613ca5
Gecko     ef8c2889969b5ba8e043edd9a1707cdd08bb3ecb

Comment 30

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Hubert Lu[:hlu] <[e-mail]> from comment #27)
> Hi Jose,
>     The flag is marked as "1.3+", and I saw the patch is merged on master in
> comment 21. I think the patch should be also merged on v1.3 branch. Could
> you help it to merge the patch on v1.3 branch? Thanks.

We need the approval here before uplifting.

Comment 31

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Jason Smith [:jsmith] from comment #28)
> Hubert - We can't merge this to 1.3 yet. We need to verify that this patch
> works on trunk first.

As Hub commented the bug won't be found in master. You guys need to perform an update.

Comment 32

[bhavana bajaj [:bajaj]]

Comment on attachment 8382180 [details] [diff] [review]
v5

Approving but leaving verifyme for hubert to help with verification once it lands.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

v1.3: 78c30d7ed6f6e30337d6c05453b867f5e97e42bc

Comment 34

[Hubert Lu[:hlu] <[e-mail]>]

This bug CAN'T be reproduced in v1.3 (Gaia: 78c30d7ed6f6e30337d6c05453b867f5e97e42bc)