Closed Bug 1232761 Opened 4 years ago Closed 4 years ago

Sort out data collection settings for restricted profiles

Categories

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

35 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 46
Tracking Status
firefox45 --- verified
firefox46 --- verified

People

(Reporter: Margaret, Assigned: Margaret)

References

(Depends on 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

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?
Flags: needinfo?(bbermes)
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
Flags: needinfo?(merwin)
Flags: needinfo?(ellee)
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.
Flags: needinfo?(merwin)
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.
Flags: needinfo?(ellee)
Flags: needinfo?(bbermes)
Flags: needinfo?(bbermes)
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
Flags: needinfo?(bbermes) → needinfo?(ellee)
Attached image screenshot (current)
Here's a better screenshot that shows our improved UI for this (what this will look like in Fx45).
Attached image 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).
Attachment #8700047 - Flags: feedback?(bbermes)
Assignee: nobody → margaret.leibovic
Attachment #8700061 - Flags: review?(s.kaspari) → review+
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. :)
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?
(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?
Flags: needinfo?(ellee)
(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.
Flags: needinfo?(ellee)
Oops, that's what I get for not reading carefully. I thought c8 was the patch itself. Looks good!
Flags: needinfo?(ellee)
(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.
https://hg.mozilla.org/integration/fx-team/rev/b96e844b0289c5363b5988214d6343b05a7996e8
Bug 1232761 - Expose telemetry/fhr settings directly in restricted profile admin UI. r=sebastian
Attachment #8700047 - Flags: feedback?(bbermes)
https://hg.mozilla.org/mozilla-central/rev/b96e844b0289
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Depends on: 1234846
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.
Attachment #8700061 - Flags: approval-mozilla-aurora?
Attachment #8700061 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(francesco.lodolo)
Attachment #8700061 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
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.
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(margaret.leibovic)
Attached patch Patch for Fx45 aurora (obsolete) — Splinter Review
This needed rebasing anyway, since Aurora doesn't have the patch for bug 1222377.
Flags: needinfo?(margaret.leibovic)
Attachment #8702577 - Flags: approval-mozilla-aurora?
Attachment #8700061 - Flags: approval-mozilla-aurora?
(Noticed a typo in the last patch. Also, making a build to verify this locally.)
Attachment #8702577 - Attachment is obsolete: true
Attachment #8702577 - Flags: approval-mozilla-aurora?
Attachment #8702579 - Flags: approval-mozilla-aurora?
Comment on attachment 8702579 [details] [diff] [review]
Patch for Fx45 aurora

thanks
Attachment #8702579 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in builds:
- Firefox 45 Beta 3;
- 46.0a2 2016-02-08;
Device: Nexus 7 (Android 5.1.1)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.