1035553 - [User Story] Restricting mobile data usage on a per-app basis

Status: RESOLVED INVALID

(Firefox OS Graveyard :: Gaia::Cost Control, defect)

Comment 1

[Justin D'Arcangelo [:justindarc]]

Re-assigning to myself.

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I know its late in the game here, but can we go consider going further than just restricting "mobile data" and make it possible to restrict wifi as well as mobile data? This would be a powerful security control but I don't know how much extra work this would be. 

Actually even from a cost perspective, omitting control of wifi limits the intended use case: just because you are using wifi, doesn't mean that data is free/cheap. In my own use case, I use a pocket wifi 3g access point since mobile data is relatively expensive. I've have had to stop using it with my tablet as it regular chews up all my data if I forget to disable tablet wifi.

Comment 3

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Marshall: I'm flagging you for review. If you're not the appropriate reviewer, could you please re-assign?

Amy: Could you review just the "app detail" screen? (when you tap an app in the list on the main screen, the detail screen appears for that specific app).

Thanks!

Comment 4

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8481426 [details] [review]
pull-request (master)

Re-assigning r? to :salva, flagging feedback? from :marshall_law

Comment 5

[Amy Lee [:amylee] UX]

Comment on attachment 8481426 [details] [review]
pull-request (master)

Hi Justin, 

I've attached a screen of my edits. Please let me know if you have any questions. 

Thanks!

Comment 6

[Amy Lee [:amylee] UX]

Attachment: App_Usage_Edits.png

Edits for app usage screen

Comment 7

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8481426 [details] [review]
pull-request (master)

Amy: I made the tweaks you mentioned. Could you please re-review? Thanks!

Comment 8

[Amy Lee [:amylee] UX]

Comment on attachment 8481426 [details] [review]
pull-request (master)

Looks great! Thanks

Comment 9

[Maria Angeles Oteo (:oteo)]

Justin, as Salva is on PTO, you can ask Marina (:mai) for the review

Comment 10

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

I've been ask to give feedback, but couldn't merge the patch, also do I need a special version of gecko to run this? I'm seeing things like: |navigator.mozApps.mgmt.enableMobileData| that are not familiar to me

Comment 11

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

Just realised that feature will be provided by bug 1042745 that is not landed or have patch yet.

Comment 12

[David Scravaglieri [:scravag]]

There's no way this to be part of 2.1, nominating for 2.2

Comment 13

[Justin D'Arcangelo [:justindarc]]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #11)
> Just realised that feature will be provided by bug 1042745 that is not
> landed or have patch yet.

You can run this, however, you will see a JS error when toggling the on/off switch in the app until the patch lands with the API updates.

Comment 14

[Wilfred Mathanaraj [:WDM]]

if there are omissions that cause an error then this should not land in 2.1. also this is not part of the items we discussed to land as part tako and 2.1.

Comment 15

[Marina Rodríguez [:mai]]

Justin, 
would you mind rebasing the patch onto latest master? 
Thanks!

Comment 16

[Justin D'Arcangelo [:justindarc]]

(In reply to Marina Rodríguez [:mai] from comment #15)
> Justin, 
> would you mind rebasing the patch onto latest master? 
> Thanks!

Done!

Comment 17

[Marina Rodríguez [:mai]]

Comment on attachment 8481426 [details] [review]
pull-request (master)

Sorry for delay, I think we need another round, address the comments on GitHub. 

Thanks for your work ! ;)

Comment 18

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8481426 [details] [review]
pull-request (master)

Updated PR to address comments from first review.

Comment 19

[Marina Rodríguez [:mai]]

added some more comments

Thanks for this good work !

Comment 20

[Marina Rodríguez [:mai]]

Hi Salva, 

Justin and me have a question about the new screeen. 
It's about the amount of data showed and the reset period (the options is monthly, weekly and never). IMHO the amount of data must be always visible. 

WDYT?
Regards

Comment 21

[Salvador de la Puente González [:salva]]

Katie, according to [1] we change the label A to point monthly or weekly usage but what about `never` case? What if the user has chosen no period? I think we should simply display the total amount spent data.

What do you think?

[1] http://val.ninja/SmartData/specs.html#application-data-control

Comment 22

[Justin D'Arcangelo [:justindarc]]

(In reply to Marina Rodríguez [:mai] from comment #19)
> added some more comments
> 
> Thanks for this good work !

No problem! I've updated the PR to address your remaining comments. The only item that remains unaddressed is the issue about the label for the "never" period that we're waiting for UX input on.

Comment 23

[katieC]

(In reply to Salvador de la Puente González [:salva] from comment #21)
> Katie, according to [1] we change the label A to point monthly or weekly
> usage but what about `never` case? What if the user has chosen no period? I
> think we should simply display the total amount spent data.
> 
> What do you think?
> 
> [1] http://val.ninja/SmartData/specs.html#application-data-control

Hi Salva, I agree, the label should display total amount of data used, but it would also be beneficial to include the amount of time the data usage has been tracked for. Ex: "3.1 GB of data has been used since June 06, 2014." This, in addition to the visualization (chart) below provides the user a stronger sense of their time/data usage relationship.

Comment 24

[Marina Rodríguez [:mai]]

 Justin, 
would you mind rebasing the patch onto latest master? 
Thanks!

Comment 25

[Justin D'Arcangelo [:justindarc]]

(In reply to Marina Rodríguez [:mai] from comment #24)
>  Justin, 
> would you mind rebasing the patch onto latest master? 
> Thanks!

Done!

Comment 26

[Justin D'Arcangelo [:justindarc]]

Marina,
I moved those constants to reside *inside* the ChartUtils module. Can you take another look and see if what I did looks correct? Thanks!

-Justin

Comment 27

[Justin D'Arcangelo [:justindarc]]

Marina,
I've updated the PR to address the case of showing the start date text for the "never" period as described in Comment 23.

Comment 28

[Marina Rodríguez [:mai]]

Comment on attachment 8481426 [details] [review]
pull-request (master)

Hi Justin,
 I've reviewed the code and I think you've made a great job. I put only a nit on the pr, but to get the r+ you must add some test, please.

Comment 29

[Marina Rodríguez [:mai]]

Hi Salva, 
I was thinking that this patch must be splitted in two, Justin has made a great job, but the behaviour of the switch could not be tested until some blockers bugs will be resolved. Currently, when you active the switch the console shows the following error:

E/Usage   ( 2425): [JavaScript Error: "TypeError: navigator.mozApps.mgmt.disableMobileData is not a function" {file: "app://costcontrol.gaiamobile.org/js/views/appdetail.js" line: 112}]
E/Usage   ( 2425): [JavaScript Error: "TypeError: navigator.mozApps.mgmt.enableMobileData is not a function" {file: "app://costcontrol.gaiamobile.org/js/views/appdetail.js" line: 110}]

WDYT about splitting this bug in two:
  One with the new functionality (the app detail view) without the Data control section.
  Other with only the data control section.

IMHO, I'm not sure if have sense allow to the user disable the data mobile use in all app. (e.g, when is allowed on the browser and disbled on system app). 

This way, we can land the first bug, because this functionallity doesn't have blockers.

Regards

Comment 30

[Justin D'Arcangelo [:justindarc]]

(In reply to Marina Rodríguez [:mai] from comment #29)
> Hi Salva, 
> I was thinking that this patch must be splitted in two, Justin has made a
> great job, but the behaviour of the switch could not be tested until some
> blockers bugs will be resolved. Currently, when you active the switch the
> console shows the following error:
> 
> E/Usage   ( 2425): [JavaScript Error: "TypeError:
> navigator.mozApps.mgmt.disableMobileData is not a function" {file:
> "app://costcontrol.gaiamobile.org/js/views/appdetail.js" line: 112}]
> E/Usage   ( 2425): [JavaScript Error: "TypeError:
> navigator.mozApps.mgmt.enableMobileData is not a function" {file:
> "app://costcontrol.gaiamobile.org/js/views/appdetail.js" line: 110}]
> 
> WDYT about splitting this bug in two:
>   One with the new functionality (the app detail view) without the Data
> control section.
>   Other with only the data control section.
> 
> IMHO, I'm not sure if have sense allow to the user disable the data mobile
> use in all app. (e.g, when is allowed on the browser and disbled on system
> app). 
> 
> This way, we can land the first bug, because this functionallity doesn't
> have blockers.
> 
> Regards

This makes sense to me. As an alternative, I can also add a build-time flag to turn the toggle switch feature on/off to prevent us from waiting on the dependency.

Comment 31

[Salvador de la Puente González [:salva]]

(In reply to Marina Rodríguez [:mai] from comment #29)
> 
> WDYT about splitting this bug in two:
>   One with the new functionality (the app detail view) without the Data
> control section.
>   Other with only the data control section.

Makes sense. Let's split it in two.

> IMHO, I'm not sure if have sense allow to the user disable the data mobile
> use in all app. (e.g, when is allowed on the browser and disbled on system
> app). 

Indeed, there are some sceneries that worth to be considered specifically, let's file a bug.

> 
> This way, we can land the first bug, because this functionallity doesn't
> have blockers.

Perfect.

Comment 32

[Justin D'Arcangelo [:justindarc]]

Created Bug 1077546 to track the addition of the "app detail" view. Moving the patch over to the new bug. I'll then add a much smaller patch to this bug that simply adds the toggle switch.

Comment 33

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Re-submitted PR on top of the bug1077546 patch to separate the app detail view from the controls for enabling/disabling data as discussed in Comment 31 and Comment 32. Carrying ui-review+ over from previous PR.

Comment 34

[Frederik Braun [:freddy]]

Are there any discussions to include wifi data blocking? Paul already raised this in comment 2.

Comment 35

[Frederik Braun [:freddy]]

Nevermind. It seems like this is offline mode we have in Firefox, according to bug 786419 and bug 1042745.

Comment 36

[Chris Lord [:cwiiis]]

I'm taking this, but just to rebase and land Justin's patch.

Comment 37

[Chris Lord [:cwiiis]]

Attachment: Add controls to app-detail page to restrict mobile data usage

Rebase.

Comment 38

[Jacob Shen]

[Tracking Requested - why for this release]:

Comment 39

[Valentin Gosu [:valentin] (he/him)]

Closing bug as b2g code is getting removed (bug 1306391).