1232761 - Sort out data collection settings for restricted profiles

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[:Margaret Leibovic]

There's some confusion in bug 1125286. Let's use this to sort it out.

Proposal: Instead of including an admin toggle to enable/disable the data collection prefs, let's just add toggles to enable/disable the FHR/telemetry prefs directly, and hide the settings UI for that in the restricted profile.

Barbara, can you help figure out what's expected here?

Comment 1

[Barbara Bermes [:barbara] - NI please!]

Marshall and Elvin, we propose to show the data choices options directly in the Admin UI as Margaret mentioned. Those will be the same as the ones for the non-restricted profile (assuming the other family member has not changed them), i.e. FHR on by default on beta and release, Telemetry off by default in release and on in beta.

The parent can decide, in the Admin UI, to turn these on/off. There will be no setting in the restricted profile to change this though for simplicity.

Any concerns? 

The mirroring of what the parent has is technically challenging and hence not suggested.

In addition, by moving the FFB mode probe into FHR, we will be able to monitor the usage and success of this mode. https://bugzilla.mozilla.org/show_bug.cgi?id=1226322

Comment 2

[Merwin]

This should be fine. Defaulting FFB data choices to the default approaches generally seems appropriate to me.  To be clear, you are saying that if the user has changed their settings in regular mode, you will not mirror those changes in the Admin UI? If the user has turned off FHR in for regular mode, the Admin UI will still default FHR on and allow the user to turn FHR off...? That approach is okay by me, unless Elvin has objections.

Comment 3

[Elvin Lee [:ellee] (no longer with Mozilla)]

Unfortunately I don't have a Android Tablet handy ... On the UX side, when the admin creates a restricted profile for Firefox, are they always presented with the admin UI so that they will be made aware of what the (default) settings for the new restricted profile are? If so, then the approach seems fine to me as well.

Comment 4

[Barbara Bermes [:barbara] - NI please!]

As soon as you create a restricted profile, the admin is presented with an UI to control on/off all installed apps, when they scroll to Firefox, they see a gear, opening that gear shows all items they can enable/disable.

Attaching some screenshots

Comment 5

[Barbara Bermes [:barbara] - NI please!]

Attachment: Screenshot_20151021-160654.png

Comment 6

[Barbara Bermes [:barbara] - NI please!]

Attachment: Screenshot_20151021-160633.png

Comment 7

[:Margaret Leibovic]

Attachment: screenshot (current)

Here's a better screenshot that shows our improved UI for this (what this will look like in Fx45).

Comment 8

[:Margaret Leibovic]

Attachment: screenshot (proposed)

I threw together a patch for my proposal. What do you think?

I took these strings directly from our settings, which means that we would be able to uplift this patch as it is. However, we could also edit the description strings for the future (it bothers me that they don't end in periods, but the other descriptions do).

Comment 9

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1232761 - Expose telemetry/fhr settings directly in restricted profile admin UI. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/28557/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28557/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8700061 [details]
MozReview Request: Bug 1232761 - Expose telemetry/fhr settings directly in restricted profile admin UI. r=sebastian

https://reviewboard.mozilla.org/r/28557/#review25377

The patch is looking good!

The first thing I noticed are the missing periods though. :)

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/28557/#review25379

::: mobile/android/chrome/content/browser.js:541
(Diff revision 1)
>      if (ParentalControls.parentalControlsEnabled) {
>          let isBlockListEnabled = ParentalControls.isAllowed(ParentalControls.BLOCK_LIST);
>          Services.prefs.setBoolPref("browser.safebrowsing.forbiddenURIs.enabled", isBlockListEnabled);
>          Services.prefs.setBoolPref("browser.safebrowsing.allowOverride", !isBlockListEnabled);
> +
> +        let isTelemetryEnabled = ParentalControls.isAllowed(ParentalControls.TELEMETRY);
> +        Services.prefs.setBoolPref("toolkit.telemetry.enabled", isTelemetryEnabled);
> +
> +        let isHealthReportEnabled = ParentalControls.isAllowed(ParentalControls.HEALTH_REPORT);
> +        SharedPreferences.forApp().setBoolPref("android.not_a_preference.healthreport.uploadEnabled", isHealthReportEnabled);

I missed one thing here: ParentalControls.parentalControlsEnabled returns true for the guest profile too.

Instead of exposing guest mode and adding a bunch of code, maybe we can configure it in GuestProfileConfiguration to hide data choices and just use the defaults?

Comment 12

[Elvin Lee [:ellee] (no longer with Mozilla)]

(In reply to :Margaret Leibovic from comment #7)
> Created attachment 8700046 [details]
> screenshot (current)
> 
> Here's a better screenshot that shows our improved UI for this (what this
> will look like in Fx45).

Hmm... I thought the plan from c0 was to have individual toggles in the admin UI for each data choice. Is your screenshot out of date or are we going with a single admin data choice that controls all of them for the restricted profile?

Comment 13

[:Margaret Leibovic]

(In reply to Elvin Lee [:ellee] from comment #12)
> (In reply to :Margaret Leibovic from comment #7)
> > Created attachment 8700046 [details]
> > screenshot (current)
> > 
> > Here's a better screenshot that shows our improved UI for this (what this
> > will look like in Fx45).
> 
> Hmm... I thought the plan from c0 was to have individual toggles in the
> admin UI for each data choice. Is your screenshot out of date or are we
> going with a single admin data choice that controls all of them for the
> restricted profile?

Yes, that's the plan for what to implement in this bug. See my (proposed) screenshot above to see what my patch here does. If you like that, we can land it and uplift it to Fx45.

Comment 14

[Elvin Lee [:ellee] (no longer with Mozilla)]

Oops, that's what I get for not reading carefully. I thought c8 was the patch itself. Looks good!

Comment 15

[:Margaret Leibovic]

(In reply to Sebastian Kaspari (:sebastian) from comment #11)
> https://reviewboard.mozilla.org/r/28557/#review25379
> 
> ::: mobile/android/chrome/content/browser.js:541
> (Diff revision 1)
> >      if (ParentalControls.parentalControlsEnabled) {
> >          let isBlockListEnabled = ParentalControls.isAllowed(ParentalControls.BLOCK_LIST);
> >          Services.prefs.setBoolPref("browser.safebrowsing.forbiddenURIs.enabled", isBlockListEnabled);
> >          Services.prefs.setBoolPref("browser.safebrowsing.allowOverride", !isBlockListEnabled);
> > +
> > +        let isTelemetryEnabled = ParentalControls.isAllowed(ParentalControls.TELEMETRY);
> > +        Services.prefs.setBoolPref("toolkit.telemetry.enabled", isTelemetryEnabled);
> > +
> > +        let isHealthReportEnabled = ParentalControls.isAllowed(ParentalControls.HEALTH_REPORT);
> > +        SharedPreferences.forApp().setBoolPref("android.not_a_preference.healthreport.uploadEnabled", isHealthReportEnabled);
> 
> I missed one thing here: ParentalControls.parentalControlsEnabled returns
> true for the guest profile too.
> 
> Instead of exposing guest mode and adding a bunch of code, maybe we can
> configure it in GuestProfileConfiguration to hide data choices and just use
> the defaults?

This actually raises an interesting question... since FHR is an app-level setting, currently a guest user could disable FHR for the main profile (this sounds like a bug).

Since the FHR setting in guest profiles mirrors the main profile, and since telemetry is disabled by default on release, I agree with your suggestion here to just hide the "Data choices" section in guest profiles, and use the default settings for FHR/telemetry submissions.

I'll update my patch.

Comment 16

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ce1d4296aff

Comment 17

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/b96e844b0289c5363b5988214d6343b05a7996e8
Bug 1232761 - Expose telemetry/fhr settings directly in restricted profile admin UI. r=sebastian

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b96e844b0289

Comment 19

[:Margaret Leibovic]

Comment on attachment 8700061 [details]
MozReview Request: Bug 1232761 - Expose telemetry/fhr settings directly in restricted profile admin UI. r=sebastian

Approval Request Comment
[Feature/regressing bug #]: Bug 1125286.

[User impact if declined]: Confusing admin UI for disabling data collection in restricted profiles.

[Describe test coverage new/current, TreeHerder]: Basic automated test coverage for not breaking regular profiles. Tested restricted profiles changes manually.

[Risks and why]: Low-risk, only affects restricted profiles.

[String/UUID change made/needed]: None.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8700061 [details]
MozReview Request: Bug 1232761 - Expose telemetry/fhr settings directly in restricted profile admin UI. r=sebastian

https://hg.mozilla.org/mozilla-central/diff/b96e844b0289/mobile/android/base/locales/en-US/android_strings.dtd
=> reverting my a+ as two strings have been removed.
Flod, is that ok with you?

Comment 21

[Francesco Lodolo [:flod]]

I know it's a pain for devs, but please provide a patch that doesn't touch android_strings.dtd
Removing strings create unnecessary noise in tools and dashboards.

Comment 22

[:Margaret Leibovic]

Attachment: Patch for Fx45 aurora

This needed rebasing anyway, since Aurora doesn't have the patch for bug 1222377.

Comment 23

[:Margaret Leibovic]

Attachment: Patch for Fx45 aurora

(Noticed a typo in the last patch. Also, making a build to verify this locally.)

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8702579 [details] [diff] [review]
Patch for Fx45 aurora

thanks

Comment 25

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc5b44c0aa5f

Comment 26

[Flaviu Cos]

Verified as fixed in builds:
- Firefox 45 Beta 3;
- 46.0a2 2016-02-08;
Device: Nexus 7 (Android 5.1.1)