Closed Bug 981694 Opened 6 years ago Closed 6 years ago

Show a notice to beta users when we turn telemetry on by default on the beta channel - Firefox for Android

Categories

(Firefox for Android :: General, enhancement, P2)

All
Android
enhancement

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox31 + fixed
fennec 30+ ---

People

(Reporter: benjamin, Assigned: liuche, NeedInfo)

References

Details

Attachments

(2 files, 2 obsolete files)

We plan on turning Telemetry on by default on all prerelease channels. This means that telemetry will now be on by default on the beta channel when it was previously off. We will need to show some notice to users that the default has changed.

We did this once before in bug 725987 but that code has since been altered by the FHR notification. I don't know whether it would be sufficient to re-show the notification or whether we need something specifically calling out that there has been a change; Jishnu can clarify.
Flags: needinfo?(jmenon)
OS: All → Android
Summary: Show a notice to beta users when we turn telemetry on by default on the beta channel - Firefox Desktop → Show a notice to beta users when we turn telemetry on by default on the beta channel - Firefox for Android
Hi - 

So here is the notice framework - we need to modify the privacy policy before landing the change:

User sees the following on first run or when we change telemetry status for current users: "%1$S automatically sends some data to %2$S so that we can improve your experience. [ Choose What I Share ]"

We need to modify the Firefox privacy policy - Ben can you confirm the accuracy? Geoff - can you please implement the change once Ben has confirmed?:

"Usage Statistics (also known as Telemetry). Beginning with version 7, Firefox includes functionality that sends Mozilla usage, performance, and responsiveness statistics about user interface features, memory, hardware configuration along with IP address.

This feature is turned off by default in general release versions of Firefox. In order to enable Nightly, Aurora and Beta testers to provide more efficient feedback, Usage Statistics are enabled by default on Nightly, Aurora and Beta. In either case, if this functionality is enabled, users can disable it in Firefox's Options/Preferences by simply deselecting the "Submit performance data" item.

Usage statistics are transmitted using SSL (a method of protecting data in transit) and help us improve future versions of Firefox. Once sent to Mozilla, usage statistics are stored in an aggregate form and made available to a broad range of developers, including both Mozilla employees and public contributors. "
Flags: needinfo?(jmenon)
Flags: needinfo?(gpiper)
Flags: needinfo?(benjamin)
> This feature is turned off by default in general release versions of
> Firefox. In order to enable Nightly, Aurora and Beta testers to provide more
> efficient feedback, Usage Statistics are enabled by default on Nightly,
> Aurora and Beta. In either case, if this functionality is enabled, users can
> disable it in Firefox's Options/Preferences by simply deselecting the
> "Submit performance data" item.

My recommendation is that instead of listing nightly/aurora/beta separately, we simply lump them together as "prerelease" users, but that's really your call.

But the current string is not "Submit performance data". There are currently two checkboxes:

"Enable Telemetry" and "Enable Nightly Health Report"
Flags: needinfo?(benjamin)
This UI isn't managed by Gecko; it'll involve a conditional change to org.mozilla.gecko.DataReportingNotification.
Component: Telemetry → General
Product: Toolkit → Firefox for Android
bsmedberg wants this to go out in 30, so there's a little bit of work prioritization that needs to happen.
tracking-fennec: --- → ?
I believe all that needs to change is that version in DataReportingNotification.java at [1] needs to be updated in the push that changes configure.in to make the beta build a non-release build.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DataReportingNotification.java#32
Won't that cause us to reshow the notification to everyone? We should not reshow this to release users (or nightly/aurora): only beta users.
Hm, do we need to notify all pre-release users that our privacy policy has been updated? I suppose this isn't a large change, but I was under impression that's why we tracked the policy version.

If we want to just show the notification for Beta, we could add another SharedPreference that checks if Beta has been notified. I assume we would want to show this notification regardless of whether the Beta user has enabled telemetry, because it's the policy change that we want to notify about, not necessarily the enabling of telemetry?
See my note "conditional change".

If you're an Aurora or Nightly user, or a Beta or Release user with telemetry enabled -- in short, if you already have telemetry turned on -- we don't notify and just bump the version.

Otherwise, we bump the version and notify.
tracking-fennec: ? → 30+
Note: legal is currently considering re-showing the notification to all users instead of just beta users. I should know the answer on this later today or tomorrow.
Chenxia, do you have time to put this on your plate? I can review.
We are not going to re-prompt everybody. Carry on with the beta-only reprompt plan.
Ok, sounds good, I'll do that.
Assignee: nobody → liuche
I wonder if we shouldn't have a new string for the "policy changed" notification, instead of just showing the same notification again.
Attachment #8397435 - Flags: feedback?(rnewman)
Comment on attachment 8397435 [details] [diff] [review]
Patch: Re-show data notification to beta users

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

::: mobile/android/base/DataReportingNotification.java
@@ +100,5 @@
>      }
> +
> +    private static boolean shouldRenotify(SharedPreferences sharedPrefs) {
> +        // Beta channel telemetry policy update for 30 (bug 981694).
> +        if (TextUtils.equals(AppConstants.MOZ_UPDATE_CHANNEL, "beta") && !sharedPrefs.getBoolean(PREFS_BETA_TELEMETRY_NOTIFIED, false)) {

Here's an alternative approach, which avoids a proliferation of special-case flags.

* Bump DATA_REPORTING_VERSION to 2. After all, it's changed, and that's what the version is for.

* Have checkAndNotifyPolicy act on two numbers:
   currentVersion = dataPrefs.getInt(PREFS_POLICY_VERSION, -1);
   DATA_REPORTING_VERSION.

* Build your logic according to the comparisons of those numbers.
  * If the currentVersion >= DATA_REPORTING_VERSION, do nothing.
  * If the currentVersion is 1, and we're on the Beta channel, perform the work required by this bug: 
    show a change notification and bump our currentVersion. If necessary, turn on telemetry?
  * If the currentVersion is 1, and we're not on the Beta channel, quietly bump our currentVersion to 2.
  * If the currentVersion < 1, do first-time work: show the main notification, set prefs etc., and set our currentVersion to 2.

Note that this is identical to the common DB versioning idiom, it avoids adding another pref, and it provides us with a nice measured place to hang different behaviors for different transitions.
Attachment #8397435 - Flags: feedback?(rnewman)
Don't automatically set the telemetry pref for beta because we can't know if the user has toggled it.

I'm assuming that the default gecko pref for telemetry in beta will be set correctly by the patch that enables telemetry.
Attachment #8397435 - Attachment is obsolete: true
Attachment #8400305 - Flags: review?(rnewman)
Bug 986582 has that code.
Comment on attachment 8400305 [details] [diff] [review]
Patch: Policy notification and update

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

::: mobile/android/base/DataReportingNotification.java
@@ +48,2 @@
>              }
> +        } else if (currentVersion == 1) {

I'd be inclined to phrase these as 

  if (...) {
    ...
    return;
  }

  if (...) {
    ...
    return;
  }

@@ +48,4 @@
>              }
> +        } else if (currentVersion == 1) {
> +            // Redisplay notification only for Beta because version 2 updates Beta policy and update version.
> +            if (TextUtils.equals("beta", AppConstants.MOZ_UPDATE_CHANNEL)){

Nit: space before {.

@@ +56,5 @@
> +                editor.putInt(PREFS_POLICY_VERSION, DATA_REPORTING_VERSION);
> +                editor.commit();
> +            }
> +        } else if (currentVersion >= DATA_REPORTING_VERSION) {
> +            // Do nothing, we're at a current version.

Or a future version, but in that case there's nothing we can do.

@@ +63,5 @@
>          }
>      }
> +
> +    /**
> +     * Launch a notification to the user of the data policy, and record notification time and version.

s/to the user//

@@ +65,5 @@
> +
> +    /**
> +     * Launch a notification to the user of the data policy, and record notification time and version.
> +     * @param context Context of activity
> +     * @param sharedPrefs SharedPreferences instance to use for preferences

I don't think these @params add a great deal of value! :D

@@ +83,5 @@
> +        String notificationSummary;
> +        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN) {
> +          notificationSummary = context.getResources().getString(R.string.datareporting_notification_action);
> +        } else {
> +          // Display partial version of Big Style notification for supporting devices.

4-space indenting.

@@ +86,5 @@
> +        } else {
> +          // Display partial version of Big Style notification for supporting devices.
> +          notificationSummary = context.getResources().getString(R.string.datareporting_notification_summary);
> +        }
> +        String notificationAction = context.getResources().getString(R.string.datareporting_notification_action);

Maybe worth having 

  final Resources res = context.getResources();

at the top of this method?

@@ +115,5 @@
> +        // Record version and notification time.
> +        SharedPreferences.Editor editor = sharedPrefs.edit();
> +        long now = System.currentTimeMillis();
> +        editor.putLong(PREFS_POLICY_NOTIFIED_TIME, now);
> +        editor.putInt(PREFS_POLICY_VERSION, DATA_REPORTING_VERSION);

If we fail for some reason to display the notification, we'll never get here. But that's probably an unrecoverable error. What do you think about doing this `finally`?
Attachment #8400305 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #17)
> Comment on attachment 8400305 [details] [diff] [review]
> Patch: Policy notification and update
> 

Nits addressed. Good call on my copypasta of the getResources() calls - why would anyone do that...? :P

> @@ +115,5 @@
> > +        // Record version and notification time.
> > +        SharedPreferences.Editor editor = sharedPrefs.edit();
> > +        long now = System.currentTimeMillis();
> > +        editor.putLong(PREFS_POLICY_NOTIFIED_TIME, now);
> > +        editor.putInt(PREFS_POLICY_VERSION, DATA_REPORTING_VERSION);
> 
> If we fail for some reason to display the notification, we'll never get
> here. But that's probably an unrecoverable error. What do you think about
> doing this `finally`?

I'd be inclined to let those values stay un-updated, because that almost certainly means that we failed to display the notification. I suppose continuing to try might be a waste of resources though, if we continue to fail for catastrophic reasons. Let me know if you feel strongly about it though.
Attachment #8400305 - Attachment is obsolete: true
Attachment #8401016 - Flags: review+
(In reply to Chenxia Liu [:liuche] from comment #18)

> I'd be inclined to let those values stay un-updated, because that almost
> certainly means that we failed to display the notification. I suppose
> continuing to try might be a waste of resources though, if we continue to
> fail for catastrophic reasons. Let me know if you feel strongly about it
> though.

How about a middle-ground: record success or failure (displayed/not displayed for some reason) in UI telemetry? If we see failures, particularly repeated for individual users, we can address that. And we can track that through to Beta, which should be plenty of time.
Status: NEW → ASSIGNED
Bug 986582 turned this on now, so tracking to make sure the notices happen.
Only question is whether we should stick with true/false; it's *possible* we'd want to record a numerical state instead, so we're not constrained to a boolean. But whatever, we could also just update the event version we need to track some other state! \o/ versioning!

(I'll update the first patch to be a "Part 1" patch when I land everything.)
Attachment #8402135 - Flags: review?(rnewman)
Attachment #8402135 - Flags: review?(rnewman) → review+
Can one detail the conditionals introduced here and what to expect?
You need to log in before you can comment on or make changes to this bug.