Closed Bug 945150 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, b2g-v1.3 verified, b2g-v1.3T fixed, b2g-v1.4 fixed)

VERIFIED FIXED
1.3 C2/1.4 S2(17jan)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- verified
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: echu, Assigned: jaoo)

References

Details

(Whiteboard: [dsds_US_test])

Attachments

(2 files, 5 obsolete files)

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%
blocking-b2g: --- → 1.3?
Whiteboard: [dsds_US_test]
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.
Summary: [DSDS] SIM manager, Call Settings, Message Settings and Cellular & Data are still accessible when airplane mode is on. → [DSDS] SIM manager(when PIN locked or no SIM), Call Settings, Message Settings and Cellular & Data are still accessible when airplane mode is on.
Call Settings and Cellular & Data wont be accessible when airplane mode is on. I'll take care of it on bugs 928297 and 928294.
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.
Flags: needinfo?(josea.olivera)
(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!
Flags: needinfo?(josea.olivera)
Blocks: 926342
Triage: user confusion.

Update assignee based on comment 2.
Assignee: nobody → josea.olivera
blocking-b2g: 1.3? → 1.3+
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #8344627 - Flags: review?(arthur.chen)
Depends on: 928297
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.
Attachment #8344627 - Flags: review?(arthur.chen)
No longer blocks: 926342
See Also: → 926342
Attached patch v2 (obsolete) — Splinter Review
Implements what I commented on comment #7. I'll request review after performing some tests on the real devices.
Attachment #8344627 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
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.
Attachment #8348793 - Attachment is obsolete: true
Attachment #8349507 - Flags: review?(arthur.chen)
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!
Attachment #8349507 - Flags: review?(arthur.chen)
(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!
(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.
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.
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.
Depends on: 948847
Attached patch v4 (obsolete) — Splinter Review
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.
Attachment #8357201 - Flags: feedback?(arthur.chen)
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.
Attachment #8357201 - Flags: feedback?(arthur.chen)
Attached patch v5 (obsolete) — Splinter Review
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!
Attachment #8357201 - Attachment is obsolete: true
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.
Attachment #8358475 - Attachment is obsolete: true
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!
Attachment #8357198 - Flags: review?(arthur.chen)
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?
Attachment #8357198 - Flags: review?(arthur.chen)
(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 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!
Attachment #8357198 - Flags: review+
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
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
Flags: needinfo?(josea.olivera)
Verified on fugu.

Gaia      ee25b0e45649d2955f340ce4f71ad55712dd0fab
Gecko     913cf2b92845441c9578787362ddad6f2ac15df6
BuildID   20140121095108
Version   28.0a2
Status: RESOLVED → VERIFIED
Attached image 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
You need to log in before you can comment on or make changes to this bug.