894121 - Using MCC automatically selects incorrect Region and city name in FTE

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[John Hammink]

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/282b5c37cf8d
Gaia   e2ef782119b7e79fc62c48d36f0c36909d982988
BuildID 20130712070210
Version 18.0

Steps to reproduce

1.Performed a factory reset; 
2.Advance to get date & time;
3.Check the continent and city/country.


Expected behavior: The handset should display the continent default: "America" and city: Los Angeles -0 at least in my case.

Actual:
The handset displays America and New York as default.

Comment 1

[Fabien Cazenave [:kaze]]

(In reply to John Hammink from comment #0)
> The handset displays America and New York as default.

Yep, Andreas asked to use America/New_York by default. :-)

If a SIM card is detected and the MCC/MNC information can be read, we propose a default city for each country — so if you’re using an American SIM card, New York will be proposed by default. That’s a feature, and there’s not much we can do to improve it so far.

However, users with non-American SIM cards can reach the Date/Time screen in the FTE app before the MCC/MNC information is read; in which case, America/New_York is selected by default, which is not optimal. Maybe we should wait for the MCC/MNC info to be available before displaying/enabling the timezone selector. Anyway, this is also sub-optimal when NITZ is available — that’s why I suggest to remove this timezone selection screen entirely in bug 826359 when NITZ is detected.

Comment 2

[Fabien Cazenave [:kaze]]

Taking the bug to keep it on my radar, but everybody is welcome to steal it.

Comment 3

[Gregor Wagner [:gwagner]]

(In reply to Fabien Cazenave [:kaze] from comment #1)
> (In reply to John Hammink from comment #0)
> > The handset displays America and New York as default.
> 
> Yep, Andreas asked to use America/New_York by default. :-)
> 
> If a SIM card is detected and the MCC/MNC information can be read, we
> propose a default city for each country — so if you’re using an American SIM
> card, New York will be proposed by default. That’s a feature, and there’s
> not much we can do to improve it so far.
> 
> However, users with non-American SIM cards can reach the Date/Time screen in
> the FTE app before the MCC/MNC information is read; in which case,
> America/New_York is selected by default, which is not optimal. Maybe we
> should wait for the MCC/MNC info to be available before displaying/enabling
> the timezone selector. Anyway, this is also sub-optimal when NITZ is
> available — that’s why I suggest to remove this timezone selection screen
> entirely in bug 826359 when NITZ is detected.

We should have the information available before we reach the timezone selector for FTU. It should even be available for the default language screen as well.

Comment 4

[Jason Smith [:jsmith]]

In triage I think we actually meant to minus this.

Comment 5

[Fernando Campo (:fcampo)]

So after few tests with different simcards (thanks Beatriz!), I discovered something:
- When the SIM card is pin protected, as we are unable to retrieve mcc/mnc from the card, all the process (launched at FTU startup) ends up selecting the hardcoded default timezone (America/New York), which is acceptable. Problem is that when user inserts correct pin, this process is not called again, so we end up with new york even if now we have access to mcc/mnc data.

- There's a setting called "time.timezone.user-selected" that is supposed to keep track of the user changes on the timezone, so if the user does change it, we keep using that one on next reboot, even if mcc/mnc indicates another timezone. Problem is, with our current code, we are storing a timezone in there even if the user didn't do anything, so it doesn't work the way it is supposed to. (bug 847342)

So I have a few questions:
- Should we solve the two problems in this bug, or split in two different ones?.
- Is the flow we are using still the correct one or should we change it?
1. on FTU startup, check mobile connection. If we have, take mcc/mnc from it. If not, from SIM card.
1.1. [needs fix] if SIM is pin protected, wait till SIM ready, then repeat the process from step 1.
2. based on mcc/mnc, guess a timezone from the JSON file.
3. show the data&time screen with the guessed tz selected. If user changes it, keep the new one on next reboot [needs fix]. If not, repeat the process.

There's other bugs related with this, that might affect this flow a little, e.g.
Bug 1026098 - Skip Time&Date screen (if we can get the time automatically).
Bug 820696 - Let the user choose between automatic or manual date on FTU (and sync that with settings).

Comment 6

[Beatriz Rodríguez [:brg]]

IMHO we should fix the first issue explained by :fcampo in comment 5 section 1.1(or in first paragraph). This is the most common use case and reported during certification. 
Regarding the second fix... maybe file a new bug is the best option.

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Fernando Campo (:fcampo) from comment #5)
> So after few tests with different simcards (thanks Beatriz!), I discovered
> something:
> - When the SIM card is pin protected, as we are unable to retrieve mcc/mnc
> from the card, all the process (launched at FTU startup) ends up selecting
> the hardcoded default timezone (America/New York), which is acceptable.
> Problem is that when user inserts correct pin, this process is not called
> again, so we end up with new york even if now we have access to mcc/mnc data.
> 
> - There's a setting called "time.timezone.user-selected" that is supposed to
> keep track of the user changes on the timezone, so if the user does change
> it, we keep using that one on next reboot, even if mcc/mnc indicates another
> timezone. Problem is, with our current code, we are storing a timezone in
> there even if the user didn't do anything, so it doesn't work the way it is
> supposed to. (bug 847342)

I cannot see what caused this after I took anther look at shared/js/tz_select.js after not touching that code for a while, this "time.timezone.user-selected" should be updated when you change the time zone manually with changing the value selector, either by region or city.

Comment 8

[Fernando Campo (:fcampo)]

(In reply to Beatriz Rodríguez [:brg] from comment #6)
> IMHO we should fix the first issue explained by :fcampo in comment 5 section
> 1.1(or in first paragraph). This is the most common use case and reported
> during certification. 
> Regarding the second fix... maybe file a new bug is the best option.
After playing with the code trying to get one fixed, I think they're so coupled that is not a good idea to split it in two bugs. I'm trying to provide a unique patch to solve all.


(In reply to Rudy Lu [:rudyl] from comment #7)
> I cannot see what caused this after I took anther look at
> shared/js/tz_select.js after not touching that code for a while, this
> "time.timezone.user-selected" should be updated when you change the time
> zone manually with changing the value selector, either by region or city.
I don't know if a change provoked this or it was working like that from the beginning (I need to make some tests with old versions for that), but the problem I see now is that the the update of the selector is triggered at least once on the beginning without user interaction. When we load the JSON file, we fill the regions, that fill the cities, and that sets a new timezone, setting the 'user-selected' variable at startup (and with the wrong timezone if the sim is pin locked). I'm trying to change this on my patch.

Comment 9

[Fernando Campo (:fcampo)]

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

WIP - still working on the tests, which is taking a little longer than expected because there wasn't any for the timezone updating process, and everything is in shared, not in ftu.

Asking for feedback to Rudy cause he was working on it before, and changes could affect settings side.

Comment 10

[Juanjo Iglesias]

Tested using a Unagi master:
Gecko: c0de5b
Gaia eca491b

I follow the steps to reproduce the bug, but I can see that its working fine. 

The handset display “Europe” and “Madrid”,  the correct info for me.

Comment 11

[Beatriz Rodríguez [:brg]]

Previous comment forgot to say that test was done included FCampo's patch. Sorry for the inconvenience!

Comment 12

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Fernando Campo (:fcampo) from comment #8)
> (In reply to Beatriz Rodríguez [:brg] from comment #6)
> > IMHO we should fix the first issue explained by :fcampo in comment 5 section
> > 1.1(or in first paragraph). This is the most common use case and reported
> > during certification. 
> > Regarding the second fix... maybe file a new bug is the best option.
> After playing with the code trying to get one fixed, I think they're so
> coupled that is not a good idea to split it in two bugs. I'm trying to
> provide a unique patch to solve all.
> 
> 
> (In reply to Rudy Lu [:rudyl] from comment #7)
> > I cannot see what caused this after I took anther look at
> > shared/js/tz_select.js after not touching that code for a while, this
> > "time.timezone.user-selected" should be updated when you change the time
> > zone manually with changing the value selector, either by region or city.
> I don't know if a change provoked this or it was working like that from the
> beginning (I need to make some tests with old versions for that), but the
> problem I see now is that the the update of the selector is triggered at
> least once on the beginning without user interaction. When we load the JSON
> file, we fill the regions, that fill the cities, and that sets a new
> timezone, setting the 'user-selected' variable at startup (and with the
> wrong timezone if the sim is pin locked). I'm trying to change this on my
> patch.

Yes, just tested this with a SIM card available, the "user-selected" would be updated in this case, which is not as expected.
And I think it might be caused by this line,
https://github.com/mozilla-b2g/gaia/blob/94245d2f0b50339450fefda7ce56db47f9e36921/shared/js/tz_select.js#L163

--
If this is not an urgent request on v2.0, I would suggest we refactor this part of code to an instantiable object with public interface, so that we could add unit test to this module, and we might need to mock the RIL related dependencies out, also for testing.

Thanks.

Comment 13

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8451785 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459

Just briefly took a look at the the code, and I think it should be working, but still suggest we refactor this as my previous comment.
Thanks.

Comment 14

[Fernando Campo (:fcampo)]

(In reply to Rudy Lu [:rudyl] from comment #13)
> Comment on attachment 8451785 [details] [review]
> Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459
> 
> Just briefly took a look at the the code, and I think it should be working,
Thanks for the f+ Rudy, just one question: would you be able to review this fully, or do you know who I can assign for it?

> but still suggest we refactor this as my previous comment.
> Thanks.
Problem is that this is a certification blocker at the moment, so it's quite urgent (Beatriz might be able to give more details). I'd rather to have this landed asap, and I'll open a followup for the refactor.

Comment 15

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I don't think I am qualified to give r+ on this change since it lives in shared/js and should belong to the "Gaia" module, you might need to find owner/peer under Gaia section to review this,
https://wiki.mozilla.org/Modules/FirefoxOS#Gaia

Thanks.

Comment 16

[Beatriz Rodríguez [:brg]]

(In reply to Fernando Campo (:fcampo) from comment #14)
> (In reply to Rudy Lu [:rudyl] from comment #13)
> > but still suggest we refactor this as my previous comment.
> > Thanks.
> Problem is that this is a certification blocker at the moment, so it's quite
> urgent (Beatriz might be able to give more details). I'd rather to have this
> landed asap, and I'll open a followup for the refactor.
Yes. This was a cert waiver and if the patch has low risk we should ask for approval to 2.0 branch.

Comment 17

[Fernando Campo (:fcampo)]

Comment on attachment 8451785 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459

Updated the PR after Rudy's comments on github.

As per Beatriz's comment 16, I opened bug 1036414 to refactor the current code so we are able to make some tests. At the moment is quite difficult to do anything, as everything happens inside shared/tz_select.js.

I'm adding Francisco and Arthur as reviewers as the changes affects both FTU and Settings, but not sure if that's the proper path, so please delegate if needed.

Comment 18

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

Comment on attachment 8451785 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459

Looks good to me, thanks!

Comment 19

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

Comment on attachment 8451785 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459

Sorry, after checked the code again I realized that 'newTZObserver' should not be called twice. What to do when the sim card becomes available should only be updating the selectors instead of initializing them again.

Comment 20

[Fernando Campo (:fcampo)]

Comment on attachment 8451785 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459

Updated PR, now I only ask for the default timezone, but had to change the listener from 'cardstatechange' to 'iccinfochange' because of timing (even if card is 'ready', the info is not available for a few moments, and didn't want to use a timeout).

Manual tests on device seemed to work both on FTU and Settings, but please review again.

Comment 21

[Andreas Gal :gal]

This is awesome. Thanks!

Comment 22

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

Comment on attachment 8451785 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459

r=me with the comment addressed.

Comment 23

[Francisco Jordano [:arcturus] [:francisco]]

Can I haz unit test?

Comment 24

[Francisco Jordano [:arcturus] [:francisco]]

Sorry, didn't add feedback.

Code wise LGTM, testing on the device works, but will be happy if we add unit test to check when the change was triggered by the user or by the sim.

Comment 25

[Fernando Campo (:fcampo)]

you haz your cheezeburger now, sir, code updated.

Comment 26

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8451785 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459

Great job Fernando!

Comment 27

[Fernando Campo (:fcampo)]

waiting for travis to merge, but it looks we have a permared :(

Comment 28

[Fernando Campo (:fcampo)]

green and merged.

master - d4583df788d33119f1fc4c68dca40a986a9bf18b

thanks all for help and patience :)

Comment 29

[Fernando Campo (:fcampo)]

Attachment: 2.0 uplift request

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 #): wrong auto selection of timezone
[User impact] if declined: user with SIM won't get the timezone correctly
[Testing completed]: unit test, device
[Risk to taking this patch] (and alternatives if risky): medium-low risk, we are dealing with timezone selection files, affects FTE and Settings app
[String changes made]: none

Comment 30

[Beatriz Rodríguez [:brg]]

Tested using Flame - master- Gecko-c431020.Gaia-1c9eb3d, with a Movistar ES SIM card with PIN code activated: Europe/Madrid is properly set in the FTE. Thanks for the work to fix this issue.
Bug marked as Verified.

I would like to add that this feature is a cert waiver in 1.1, 1.3.... and due to the low risk, we would like to see it working in 2.0.

Comment 31

[Preeti Raghunath(:Preeti)]

Comment on attachment 8459462 [details]
2.0 uplift request

Given that it was past cert waivers providing approval for 2.0

Comment 32

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/05ca48fe797e03c18f91e3779489ab9c38f6363f

Comment 33

[Sue]

Attachment: verify_video.MP4

This issue has been verified successfully on Flame v2.0 & v2.1
STR:
0. Insert a SIM card.
1. Performed a factory reset; 
2. Next to get date & time;
3. Check the continent and city/country.
**The city/country shows agrees with SIM card.
See attachment: verify_video.MP4
Reproducing rate: 0/5
Flame 2.0 versions:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141127000203
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141127.034818
FW-Date         Thu Nov 27 03:48:29 EST 2014
Bootloader      L1TC00011880

Flame 2.1 versions:
Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b
Build-ID        20141127001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141127.035005
FW-Date         Thu Nov 27 03:50:16 EST 2014
Bootloader      L1TC00011880