945150 - [DSDS] SIM manager(when PIN locked or no SIM), Call Settings, Message Settings and Cellular & Data are still accessible when airplane mode is on.

Status: VERIFIED FIXED

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

Description

[Enpei]

SIM manager, Call Settings, Message Settings and Cellular & Data are still accessible when airplane mode is on.

* Build Number                
Gaia:     f64d282b6b44eaaf8cddc8be8f3250cb6fbacd95
Gecko:    0e3362fb5625eb6d98c7617b1b3019a2cc553d47
BuildID   20131202094818
Version   28.0a1

* Reproduce Steps
Enable airplane mode.

* Expected Result
Unable to access SIM manager, Call Settings, Message Settings and Cellular & Data.

* Actual Result
User can still access SIM manager, Call Settings, Message Settings and Cellular & Data.

* Occurrence rate
100%

Comment 1

[Enpei]

Correct my expected result to follow UX spec.

* Expected Result
1. Unable to access Call Settings, Message Settings and Cellular & Data.
2. If PIN is locked or no SIM cards inserted in device, SIM manager cannot be access as well under airplane mode.

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

Call Settings and Cellular & Data wont be accessible when airplane mode is on. I'll take care of it on bugs 928297 and 928294.

Comment 3

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

Jose, note that the card state remains the same when user enabled airplane mode on the fugu device. Please use mozMobileConnection.radioState and the radiostatechanged event instead of checking the card state.

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Arthur Chen [:arthurcc] from comment #3)
> Jose, note that the card state remains the same when user enabled airplane
> mode on the fugu device. Please use mozMobileConnection.radioState and the
> radiostatechanged event instead of checking the card state.

Oh, thanks Arthur!

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Triage: user confusion.

Update assignee based on comment 2.

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

As I said in comment #2 I would take care of Call Settings and Cellular & Data items in bugs 928297 and 928294 so we only need to take care here of disabling the SIM manager in case of SIM absent or airplane mode.

Arthur, would you review this patch please? Thanks.

PS. This bug would depend -at least- on bug 928297 per comment above.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8344627 [details] [diff] [review]
v1

After an off-line discussion with Arthur we agreed that we also should disable all the first level menu items in case of airplane mode and ICC card absent. That means we should disable 'Call Setting' and 'Cellular & Data' menu items as well.

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

WIP at branch at https://github.com/jaoo/gaia/tree/945150

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

Implements what I commented on comment #7. I'll request review after performing some tests on the real devices.

Comment 10

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v3

Arthur, could you take a look please?

This patch implements what I commented on comment #7. Tested on both Peak and fugu devices. Seems to work correctly.

Comment 11

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

Comment on attachment 8349507 [details] [diff] [review]
v3

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

Jose, thanks for the patch. 

I had some tests and found that if we enable airplane mode from quick settings or action menu (long press the power button), the states of the items are incorrect. And there are more and more similar logic dealing with card state change and radio state change (in settings.js and connectivity.js), which makes the code difficult to trace and maintain. Given that we are going to refactor airplane mode related logic in bug 948847, I would like to do refactor this part based on it. I'm okay to work on this part as you are going to have PTO soon. Please let me know what you think, thanks!

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Arthur Chen [:arthurcc] (PTO 12/25 ~ 1/5) from comment #11)
> Comment on attachment 8349507 [details] [diff] [review]
> v3
> 
> Review of attachment 8349507 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Jose, thanks for the patch.

Thanks for the review.

> I had some tests and found that if we enable airplane mode from quick
> settings or action menu (long press the power button), the states of the
> items are incorrect. And there are more and more similar logic dealing with
> card state change and radio state change (in settings.js and
> connectivity.js), which makes the code difficult to trace and maintain.

Yeah, I was tempted to unify the logic from both files but I didn't do it to be fast fixing the bug.

> Given that we are going to refactor airplane mode related logic in bug
> 948847, I would like to do refactor this part based on it. I'm okay to work
> on this part as you are going to have PTO soon. Please let me know what you
> think, thanks!

I'm ok with having a helper for dealing with radio and card states It will be useful, indeed. So lets pause a bit here and wait for the helper.

Thanks Arthur!

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Arthur Chen [:arthurcc] (PTO 12/25 ~ 1/5) from comment #11)
> Comment on attachment 8349507 [details] [diff] [review]

> I had some tests and found that if we enable airplane mode from quick
> settings or action menu (long press the power button), the states of the
> items are incorrect.

FYI I've retested those cases and everything works fine.

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

Arthur, after taking a look at all the pieces I have ended up with a plan for this bug. The plan is to add the helper and to unify the code from settings.js and connectivity files in settings.js file. I'll work on top of EJ's patches that way it will be faster since there won't be need to wait for the helper to be landed. I hope you will be ok with it.

Comment 15

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

A helper sounds good to me. We also need a base object that can handle the listeners on mozIcc objects so that we don't need to add or remove the listeners manually when the mozIcc objects become invalid. Let's discuss more when you start working on it.

Comment 16

[José Antonio Olivera Ortega [:jaoo]]

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

Comment 17

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v4

Arthur, this is the new approach so far. I've added a new singleton object that handles the menu items related to telephony features. Next would be to use the airplane mode helper being implemented by EJ. Some feedback from you would be nice.

Comment 18

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

Comment on attachment 8357201 [details] [diff] [review]
v4

Thanks for the work, Jose.

It is much cleaner for settings.js and connectivity.js now. Can we further extract the logic of handling sim card state to another object? So that we can reuse the object for all items that need to reflect the sim card states.

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v5

New version (v5) of the patch (PR updated also).

(In reply to Arthur Chen [:arthurcc] from comment #18)
> Comment on attachment 8357201 [details] [diff] [review]
> v4

> It is much cleaner for settings.js and connectivity.js now. Can we further
> extract the logic of handling sim card state to another object? So that we
> can reuse the object for all items that need to reflect the sim card states.

Sure, v5 patch addresses the comment above. Once EJ finishes with the airplane mode helper I'll add it to the new object that handles ICC card states and request review at you again.

Thanks!

Comment 20

[José Antonio Olivera Ortega [:jaoo]]

https://github.com/mozilla-b2g/gaia/pull/15117 PR updated. Now we make use of the AirplaneModeHelper object implemented by EJ. I'll request review for this patch once EJ lands his work.

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8357198 [details] [review]
Pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/15117

Arthur, EJ's work just landed. Would you review this now please? Thanks!

Comment 22

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

Comment on attachment 8357198 [details] [review]
Pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/15117

I still think there is duplicate logic that should be included in the reusable object, for example, event handling things in call_iccs.js and carrier_iccs.js.

However, as we need to fix all 1.3+ bugs by the end of this week and there is a potential risk of landing such a big patch, is it possible to have a simple fix for now and create another bug for refactoring?

Comment 23

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Arthur Chen [:arthurcc] from comment #22)
> Comment on attachment 8357198 [details] [review]
> Pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/15117

Thanks for the review Arthur.

> I still think there is duplicate logic that should be included in the
> reusable object, for example, event handling things in call_iccs.js and
> carrier_iccs.js.
> 
> However, as we need to fix all 1.3+ bugs by the end of this week and there
> is a potential risk of landing such a big patch, is it possible to have a
> simple fix for now and create another bug for refactoring?

Talked to Arthur on IRC. The thing here is that this bug is 1.3+ and the refactor could end up with a big patch and it could be risky landing it. The plan would be to land this patch as it is (after more test performed by Arthur) and to work on the refactor after code freeze.

Comment 24

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

Comment on attachment 8357198 [details] [review]
Pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/15117

Per an offline discussion, let's land this patch as the code structure is better for the refactor in the future. r=me with the nits addressed. Thanks!

Comment 25

[José Antonio Olivera Ortega [:jaoo]]

Thanks Arthur, I've tested the path several times in both peak and fugu devices. Also I've run several times Travis. Travis's green and everything seems to work correctly.

Landed Gaia master branch at:
https://github.com/mozilla-b2g/gaia/commit/cc28d9cf5214cbefb67171551ce2c367f7f80a7f

Comment 26

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 cc28d9cf5214cbefb67171551ce2c367f7f80a7f
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 27

[José Antonio Olivera Ortega [:jaoo]]

Landed on Gaia v1.3 branch at https://github.com/mozilla-b2g/gaia/commit/7443da6780336eac68d4228834372e16393d10d8

Comment 28

[Enpei]

Verified on fugu.

Gaia      ee25b0e45649d2955f340ce4f71ad55712dd0fab
Gecko     913cf2b92845441c9578787362ddad6f2ac15df6
BuildID   20140121095108
Version   28.0a2

Comment 29

[Eric Chang [:ericcc] [:echang]]

Attachment: GrayOut.png

Works on v1.4 Fugu with airplane mode on. 

GAIA_REV=441c4bcd8ac4f8c01a9bc5a2f8d64eaa87844803
GECKO_REV=0c18beacb37a7e1e80bcf6d46cc8ff9a18c0c653
GAIA_BRANCH=mozillaorg/v1.4
GECKO_BRANCH=mozillaorg/v1.4
BUILD_TAG=jenkins-B2G.v1.4.0.fugu-25
BuildID=20140416041552