Closed Bug 993161 Opened 6 years ago Closed 6 years ago

new voicemail notification dials the phone number "1" rather than the voicemail number

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
1.4 S6 (25apr)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: dbaron, Assigned: arthurcc)

Details

(Keywords: dataloss, regression, Whiteboard: [FT:System-Platform][ETA: 4/30][p=2])

Attachments

(2 files)

I've been dogfooding my own builds of master on a hamachi.

Sometime in the past few weeks (although it's *possible* it's been broken since I switched to trunk about a month ago, but I really don't think so) the new voicemail notification broke, so that it no longer connects to my voicemail.

The new voicemail now says: "New Voicemail" / "Dial 1".  If I press it, it actually tries to dial the number "1" which then says that the call cannot be completed as dialed.  (It used to say "Dial NNN" for some other value of NNN that I've forgotten.)

Long-pressing 1 in the dialer still gets to voicemail, but that's somewhat obscure UI that many users don't know about.

(I've only tested master, so I don't know if this bug is present on other branches.)
How old is your current build ? This smells a lot like bug 985117 and/or bug 985090.
Without much build info, it seems dupe of those fixed bugs.
Flags: needinfo?(dbaron)
gaia c4468bc33303dac135f6acf1e18b3a6a2473f92c; gecko d8b2e3738785 plus local patches.

i.e., about 10-12 hours old.
Flags: needinfo?(dbaron)
(I think it's more likely a *regression* from one of those patches.)
Hmn, your commit is a local one?

https://github.com/mozilla-b2g/gaia/commit/c4468bc33303dac135f6acf1e18b3a6a2473f92c returns a 404.

Anyway, can you cross check your Voicemail settings ? Also, you confirm that you see "Dial 1" under the Voicemail message. I just did a try on my Nexus S running a master roughly as old as yours, and on my device it's okay.
Flags: needinfo?(dbaron)
Can you make sure your gaia tree has this commit: 14ba0ddfc8653e445417d0bafff28bba126a4507 ?
Whiteboard: [systemsfe]
Interesting; repo actually rebased my local commit (which was bug 991927), which I wasn't expecting it to do.  So my real gaia hash was beeba080bf39f846010d3171ddbbdde126ccf590 (with just bug 991927 on top of it).

I do have 14ba0ddfc8653e445417d0bafff28bba126a4507.
Flags: needinfo?(dbaron)
(In reply to Alexandre LISSY :gerard-majax from comment #5)
> Anyway, can you cross check your Voicemail settings ?

I haven't actually *changed* my voicemail settings at all since it worked (or ever, I think), so that's not particularly relevant.  That said, Settings -> Call Settings -> Voicemail -> Voicemail number says "1".

> Also, you confirm that
> you see "Dial 1" under the Voicemail message.

That's what I see, but "1" is the wrong thing to dial, and it didn't used to say "Dial 1".

(I tried taking a screenshot yesterday, but the screenshot was corrupted.)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #8)
> (In reply to Alexandre LISSY :gerard-majax from comment #5)
> > Anyway, can you cross check your Voicemail settings ?
> 
> I haven't actually *changed* my voicemail settings at all since it worked
> (or ever, I think), so that's not particularly relevant.  That said,
> Settings -> Call Settings -> Voicemail -> Voicemail number says "1".

Then it means the setting value is bugged :(. Bug 985090 was supposed to fix this.

> 
> > Also, you confirm that
> > you see "Dial 1" under the Voicemail message.
> 
> That's what I see, but "1" is the wrong thing to dial, and it didn't used to
> say "Dial 1".

I don't get the point there. This message may be coming from your operator OR from us. It may have changed. The "1" here is the voicemail number.
Can you try to set the correct value for your voicemail in Settings ? If it does fix the issue, upgrade path for bug 985090 is still wrong.
Flags: needinfo?(dbaron)
Flags: needinfo?(arthur.chen)
How do I know what the correct value is?  It used to just work, and it detected it correctly from the network.
Flags: needinfo?(dbaron)
(There is a 10 digit number that it dials when I long press 1 in the dialer, but I *thought* there was a shorter number that it used to show and dial, though maybe I'm misremembering.)
Also, it seems really odd that we'd do different things for a long-press of 1 in the dialer, and tapping on the new voicemail notification.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #13)
> Also, it seems really odd that we'd do different things for a long-press of
> 1 in the dialer, and tapping on the new voicemail notification.

I missed this point, and I can't explain why.

Voicemail number should be provided by your carrier in some documentations. For the current purpose, you may even try to set any valid number in settings, it should be enough to check.
It does indeed appear to be a data migration issue; if I "make reset-gaia" (and clobber a bit more too, actually), then the new voicemail notification says "Dial 123" instead of "Dial 1".  The Voicemail setting in Call Settings also says "123" in the clean system.  Dialing "123" actually ends up dialing +1 805 637 7243, which is the same that a long press of 1 also dials.  (I asked a colleague with a T-Mobile Android phone, and 123 seems to dial the same number for him as well.)
(Also, my phone is bricked as a result of the data restore following this 'make reset-gaia', so I'm unable to test anything else for a while.)
I would say the issue might be caused by `make install-gaia`. Doing `make install-gaia` does not clean the settings field storing voicemail number. After bug 975918 landed, the settings field becomes an array. If the value of the field is a string, for example, "123", it returns "1" when trying to fetch the voicemail number of the first sim card. Doing `make reset-gaia` fixes the issue.
Flags: needinfo?(arthur.chen)
Then doesn't there need to be migration code to migrate the old setting for upgrades?
Flags: needinfo?(arthur.chen)
We assumed that `make install-gaia` is only used by developers and currently there is no proper way to do migration in this scenario. The migration is performed when doing system upgrades or detecting new sim cards.
Flags: needinfo?(arthur.chen)
What's supposed to trigger that upgrade path, and why did I not hit it?  This is the first time I've been told that, and I've been updating using the same profile data since before v1.1.  (Shouldn't it typically be based on database versions?)
Flags: needinfo?(arthur.chen)
(If there isn't a mechanism for upgrading the settings DB, then maybe you should use a new setting name rather than reusing one with different meaning?)
That said, I suspect there's an existing best practice for how to deal with this sort of case; I just don't know what it is.
I agree with you that the best practice of upgrading a database is based on database versions. But currently we don't have database version for mozSettings. And mozSettings works more like async storage except for that it is cross-app. Apps with mozSettings permission are able to create fields and also responsible for maintaining the fields and do the migration. So that for now there is no centralized place to do the migration for all settings fields.

Voicemail number is determined by apn database and the mcc, mnc numbers of the inserted icc card. For this kind of settings we don't update the values unless a new icc card is detected or when performing a system upgrade.
Flags: needinfo?(arthur.chen)
What constitutes "performing a system upgrade"?  Where is the code for it, and how do I test it?
Flags: needinfo?(arthur.chen)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #25)
> What constitutes "performing a system upgrade"?  Where is the code for it,
> and how do I test it?

I suspect you want to build an OTA/FOTA package. On Hamachi, you have to build a FOTA, which is applied in recovery mode. This means you need to have a recovery partition with testkeys.

This has been done in bug 935059. You can also switch app.update.url to update.boot2gecko.org for hamachi, that will do the same.
System upgrade means updating via Settings->Device information->Check for updates. Note that we use the value stored under "deviceinfo.os" to do the version comparison, which means changes within one release are not allowed.

To test the migration code without OTA you can set "this._deviceInfoOs" as a random value [1], which forces the migration path in the voicemail update function [2] to be executed.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/operator_variant/operator_variant.js#L52
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/operator_variant/operator_variant.js#L423
Flags: needinfo?(arthur.chen)
(In reply to Arthur Chen [:arthurcc] from comment #27)
> System upgrade means updating via Settings->Device information->Check for
> updates. Note that we use the value stored under "deviceinfo.os" to do the
> version comparison, which means changes within one release are not allowed.
> 
> To test the migration code without OTA you can set "this._deviceInfoOs" as a
> random value [1], which forces the migration path in the voicemail update
> function [2] to be executed.
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> operator_variant/operator_variant.js#L52
> [2]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> operator_variant/operator_variant.js#L423

This looks clearly wrong to me, relying on deviceinfo.os. It means all dogfooders will get caught in this bug.
If they are not upgraded from v1.3, they are caught in this bug. Will deviceinfo.platform_build_id be sufficient in this case?
(In reply to Arthur Chen [:arthurcc] from comment #29)
> If they are not upgraded from v1.3, they are caught in this bug. Will
> deviceinfo.platform_build_id be sufficient in this case?

can't you rely just on the type of the setting ?
(In reply to Arthur Chen [:arthurcc] from comment #24)
> I agree with you that the best practice of upgrading a database is based on
> database versions. But currently we don't have database version for
> mozSettings. And mozSettings works more like async storage except for that
> it is cross-app. Apps with mozSettings permission are able to create fields
> and also responsible for maintaining the fields and do the migration. So
> that for now there is no centralized place to do the migration for all
> settings fields.
> 
> Voicemail number is determined by apn database and the mcc, mnc numbers of
> the inserted icc card. For this kind of settings we don't update the values
> unless a new icc card is detected or when performing a system upgrade.

Uh thats a bad bug. We have to perform a settingsDB upgrade if we need to. We never had to so far and a version bump just triggers an import of the default settings file.
Please bump the version number and perform a proper upgrade if you rely on a different data format.
Gregor, could you tell more about how to bump the version number of settingsDB? By the default settings file did you mean "common-settings.json"? If the default settings file is imported, how can we ensure it does not overwrite the existing values?
Flags: needinfo?(anygregor)
(In reply to Arthur Chen [:arthurcc] from comment #32)
> Gregor, could you tell more about how to bump the version number of
> settingsDB? By the default settings file did you mean
> "common-settings.json"? If the default settings file is imported, how can we
> ensure it does not overwrite the existing values?

You increase the DB version here: http://hg.mozilla.org/mozilla-central/file/43cd629879c2/dom/settings/SettingsDB.jsm#l22
then you have to add your upgrade logic for the version step in http://hg.mozilla.org/mozilla-central/file/43cd629879c2/dom/settings/SettingsDB.jsm#l35

The import logic is smart enough to import new values, delete old ones and don't overwrite values set by the user. Just read the code around defaultValue and userValue.
Flags: needinfo?(anygregor)
Nominating 1.4? since I believe this is a regression from bug 975918 / bug 985090.
blocking-b2g: --- → 1.4?
Component: Gaia::System → Gaia::Dialer
(Also, I'm still seeing essentially bug 985090 in that if I try to change the voicemail setting in the UI, the change doesn't take, and it continues showing up as "1".)
triage: 1.4+ for regression. 
Arthur, wonder if you are the right person to take this bug? thanks
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(arthur.chen)
Keywords: regression
Assignee: nobody → arthur.chen
Flags: needinfo?(arthur.chen)
Keywords: dataloss
Whiteboard: [systemsfe] → [FT:System-Platform]
Moving to system since Dialer never sets the voicemail numbers.
Component: Gaia::Dialer → Gaia::System
Arthur,

Can you please provide next steps here?
Flags: needinfo?(arthur.chen)
Jose, could you help check if the patch makes sense to you? Now we check the type of the value stored under 'ril.iccInfo.mbdn' when starting up. It does not involve apn database lookup but only does the migration when needed. Thanks!
Attachment #8407948 - Flags: feedback?(josea.olivera)
Flags: needinfo?(arthur.chen)
Target Milestone: --- → 1.4 S6 (25apr)
Jose, can you help to check the patch and provide your feedback as soon as possible? 
Thank you!
Flags: needinfo?(josea.olivera)
Comment on attachment 8407948 [details]
link to https://github.com/mozilla-b2g/gaia/pull/18395

To ensure the voicemail setting gets converted to the new format before the voicemail setup, I have to move the loading of voicemail.js after operator_variant_iccs.js. Alive, could you help check it?
Attachment #8407948 - Flags: review?(josea.olivera)
Attachment #8407948 - Flags: review?(alive)
Attachment #8407948 - Flags: feedback?(josea.olivera)
Attachment #8407948 - Flags: review?(alive) → review+
(In reply to Kevin Hu [:khu] from comment #40)
> Jose, can you help to check the patch and provide your feedback as soon as
> possible? 

Sure, let me finish first the 1.3+ reviews and I'll take care of it ASAP.
Flags: needinfo?(josea.olivera)
Whiteboard: [FT:System-Platform] → [FT:System-Platform][ETA: 4/30][p=2]
Are we still waiting for review here?
Flags: needinfo?(josea.olivera)
Comment on attachment 8407948 [details]
link to https://github.com/mozilla-b2g/gaia/pull/18395

LGTM, r=me

Left a comment on the PR. Please address it before landing this patch please. Thanks Arthur and sorry for the delay on reviewing this.
Attachment #8407948 - Flags: review?(josea.olivera) → review+
Flags: needinfo?(josea.olivera)
Thanks Jose!

master: d62f9f37ed325df7726dd999f0d49f80be16c18b
v1.4: 9aeb5c73970f498fcc6347293fbcfd375947923c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.