Closed Bug 981689 Opened 6 years ago Closed 5 years ago

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

Categories

(Toolkit :: Telemetry, enhancement, P2)

enhancement

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox31 + verified
firefox32 --- verified

People

(Reporter: benjamin, Assigned: mano)

References

Details

(Whiteboard: [no l10n impact] p=5 s=it-32c-31a-30b.2 [qa!])

Attachments

(5 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 699806 but that code has since been removed from the product because we landed 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.

The wording of the existing notification is:

%1$S automatically sends some data to %2$S so that we can improve your experience. [ Choose What I Share ]

If we need new strings, we need to get those landed this week.

Some other technical notes:
There is something called "datareporting.policy.dataSubmissionPolicyAcceptedVersion" but it looks as if we only ever write it, never read it. Bumping that version (if it worked) would show the notification to all users not just beta users.
Flags: needinfo?(jmenon)
Blocks: fxdesktoptriage
No longer blocks: fxdesktopbacklog
Blocks: 929091
Yes, we should use dataSubmissionPolicyAcceptedVersion and its corresponding notification mechanism for this.

The reason we don't ever read that pref is because only version 1 is present. We were holding off writing that code until it was needed.
I don't think we should actually use that pref in this context: unless we special-case the version, that would end up showing a notice to all channels and not just beta. Also we need Jishnu to verify what the notification should say.
I think we should use the notice we use today which encapsulates both FHR and Telemetry: "%1$S automatically sends some data to %2$S so that we can improve your experience. [ Choose What I Share ]."
Flags: needinfo?(jmenon)
ok cool. No l10n impact, so this doesn't require work this week before branch.
Whiteboard: no l10n impact
Blocks: fxdesktopbacklog
No longer blocks: fxdesktoptriage
Whiteboard: no l10n impact → [no l10n impact] p=0
Status: NEW → ASSIGNED
Whiteboard: [no l10n impact] p=0 → [no l10n impact] p=5 s=it-31c-30a-29b.1
Assignee: nobody → mano
Assigning to Kamil to sign-off for this iteration.
QA Contact: kamiljoz
Whiteboard: [no l10n impact] p=5 s=it-31c-30a-29b.1 → [no l10n impact] p=5 s=it-31c-30a-29b.1 [qa+]
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. That will make this bug easier!
We are not going to re-prompt everybody. Carry on with the beta-only reprompt plan.
Comment on attachment 8396260 [details] [diff] [review]
Make policy-prefs unique for each update channel, but keep the current prefs for the release channel

A couple things:

* this patch doesn't appear to change the expected/actual policy version for beta... will that be a separate patch, or does this just reprompt as a result of changing the entire prefs tree? Won't it also reprompt nightly/aurora?
* The data reporting policy has code to record the currently accepted version but no code to read it or compare it against the actual current policy version? Do you intend to bump the policy version to "2" for just the beta channel?
* Changing the entire prefs tree per-channel seems unfortunately complex and could interact poorly with distribution builds which sometimes tweak the channel. Could we maybe just read the "expected" version from one general pref and one channel-specific pref?
** datareporting.policy.expectedVersion
** datareporting.policy.expectedVersion.channel-<channel>

So that in general "datareporting.policy.expectedVersion" will be "1" but "datareporting.policy.expectedVersion.channel-beta" is "2"
Attachment #8396260 - Flags: review?(benjamin) → review-
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Comment on attachment 8396260 [details] [diff] [review]
> Make policy-prefs unique for each update channel, but keep the current prefs
> for the release channel
> 
> A couple things:
> 
> * this patch doesn't appear to change the expected/actual policy version for
> beta... will that be a separate patch, or does this just reprompt as a
> result of changing the entire prefs tree? Won't it also reprompt
> nightly/aurora?

I thought the preference change is going to be done in bug 929091.

Yes, the patch would affect nightly and aurora as well. I considered that a minor issue.

> * The data reporting policy has code to record the currently accepted
> version but no code to read it or compare it against the actual current
> policy version? Do you intend to bump the policy version to "2" for just the
> beta channel?

My approach deliberately tried to avoid using this (partially implemented) versioning mechanism, because it seemed somewhat complex to me to have just a different version for each channel (keep in mind users can switch between channels). That's why I opted for a whole separate prefs branch.

> * Changing the entire prefs tree per-channel seems unfortunately complex and
> could interact poorly with distribution builds which sometimes tweak the
> channel. Could we maybe just read the "expected" version from one general
> pref and one channel-specific pref?
> ** datareporting.policy.expectedVersion
> ** datareporting.policy.expectedVersion.channel-<channel>
> So that in general "datareporting.policy.expectedVersion" will be "1" but
> "datareporting.policy.expectedVersion.channel-beta" is "2"

Again, I'm not sure that reving just the version for one of the update channels is going to work well.

How about using a separate prefs tree, but making it opt-in (so that it's only the beta channel that has its own branch)?
> I thought the preference change is going to be done in bug 929091.

It can be, but I'm not sure what change that would be in this case.

> Yes, the patch would affect nightly and aurora as well. I considered that a
> minor issue.

It's unfortunate, but not the end of the world.

> My approach deliberately tried to avoid using this (partially implemented)
> versioning mechanism, because it seemed somewhat complex to me to have just
> a different version for each channel (keep in mind users can switch between
> channels). That's why I opted for a whole separate prefs branch.

I was making an assumption that the versioning compare wouldn't be a strict compare, so channel switching wouldn't be a problem: If you're using release/aurora/nightly builds, we'd continue to record `1` and make sure that .acceptedVersion is >= 1. On beta we'd record .acceptedVersion == 2 and ensure that .acceptedVersion is >= 2.

> How about using a separate prefs tree, but making it opt-in (so that it's
> only the beta channel that has its own branch)?

That just punts the problem down the road to whenever we end up on version 3: in that case people who do switch channels are going to be prompted against separately for beta and release, which I really do want to avoid.
Whiteboard: [no l10n impact] p=5 s=it-31c-30a-29b.1 [qa+] → [no l10n impact] p=5 s=it-31c-30a-29b.2 [qa+]
Attached patch patch (obsolete) — Splinter Review
Btw, in configure.ac, what should I use for detecting "really-release" builds (RELEASE_BUILDS confusingly includes beta builds)?
Attachment #8399869 - Flags: review?(benjamin)
I forgot to remove the setter for dataSubmissionPolicyAcceptedVersion. Fixed locally.
Comment on attachment 8399869 [details] [diff] [review]
patch

>diff --git a/services/datareporting/datareporting-prefs.js b/services/datareporting/datareporting-prefs.js
>--- a/services/datareporting/datareporting-prefs.js
>+++ b/services/datareporting/datareporting-prefs.js
>@@ -1,12 +1,17 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>+#filter substitution
>+
> pref("datareporting.policy.dataSubmissionEnabled", true);
> pref("datareporting.policy.dataSubmissionPolicyAccepted", false);
> pref("datareporting.policy.dataSubmissionPolicyBypassAcceptance", false);
> pref("datareporting.policy.dataSubmissionPolicyNotifiedTime", "0");
> pref("datareporting.policy.dataSubmissionPolicyResponseType", "");
> pref("datareporting.policy.dataSubmissionPolicyResponseTime", "0");
> pref("datareporting.policy.firstRunTime", "0");
> 
>+pref("datareporting.policy.minimumPolicyVersion", 1);
>+pref("datareporting.policy.minimumPolicyVersion.channel-@MOZ_UPDATE_CHANNEL@", 2);

Why are we preprocessing here instead of hardcoding "beta"?

Note that you can't tell the difference between a beta and release build at build time, especially because we send the release build as the last update the beta audience.

>+  /**
>    * Whether the user has accepted that data submission can occur.
>    *
>    * This overrides dataSubmissionEnabled.
>    */
>   get dataSubmissionPolicyAccepted() {
>     // Be conservative and default to false.
>-    return this._prefs.get("dataSubmissionPolicyAccepted", false);
>+    let enabled = this._prefs.get("dataSubmissionPolicyAccepted", false);
>+    if (!enabled)
>+      return false;
>+
>+    let acceptedVersion = this._prefs.get("dataSubmissionPolicyAcceptedVersion");
>+    dump("\nacceptedVersion: " + acceptedVersion + "\n");
>+    dump("this.minimumPolicyVersion: " + this.minimumPolicyVersion + "\n");

Remove dumps before checkin.

>+    return acceptedVersion >= this.minimumPolicyVersion;
>   },
> 
>   set dataSubmissionPolicyAccepted(value) {
>     this._prefs.set("dataSubmissionPolicyAccepted", !!value);
>+    if (!!value)
>+      this._prefs.set("dataSubmissionPolicyAcceptedVersion", 2);

The "current" version should either be in a const at the top of this file or in a pref itself.

r=me with the hardcoded beta and a const, or please re-request review if that's not correct
Attachment #8399869 - Flags: review?(benjamin) → review+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #14)
> Comment on attachment 8399869 [details] [diff] [review]
> patch
> 
> >diff --git a/services/datareporting/datareporting-prefs.js b/services/datareporting/datareporting-prefs.js
> >--- a/services/datareporting/datareporting-prefs.js
> >+++ b/services/datareporting/datareporting-prefs.js
> >@@ -1,12 +1,17 @@
> > /* This Source Code Form is subject to the terms of the Mozilla Public
> >  * License, v. 2.0. If a copy of the MPL was not distributed with this
> >  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > 
> >+#filter substitution
> >+
> > pref("datareporting.policy.dataSubmissionEnabled", true);
> > pref("datareporting.policy.dataSubmissionPolicyAccepted", false);
> > pref("datareporting.policy.dataSubmissionPolicyBypassAcceptance", false);
> > pref("datareporting.policy.dataSubmissionPolicyNotifiedTime", "0");
> > pref("datareporting.policy.dataSubmissionPolicyResponseType", "");
> > pref("datareporting.policy.dataSubmissionPolicyResponseTime", "0");
> > pref("datareporting.policy.firstRunTime", "0");
> > 
> >+pref("datareporting.policy.minimumPolicyVersion", 1);
> >+pref("datareporting.policy.minimumPolicyVersion.channel-@MOZ_UPDATE_CHANNEL@", 2);
> 
> Why are we preprocessing here instead of hardcoding "beta"?

Silly silly me. yes.

 
> Note that you can't tell the difference between a beta and release build at
> build time, especially because we send the release build as the last update
> the beta audience.

Hrm, so how are we going to turn on telmetry in the the beta channel? The code in configure.ac relies on RELEASE_BUILD, which won't work.
It's going to be based on the value of the default-channel pref. See the patch in bug 986582.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)

> I was making an assumption that the versioning compare wouldn't be a strict
> compare, so channel switching wouldn't be a problem: If you're using
> release/aurora/nightly builds, we'd continue to record `1` and make sure
> that .acceptedVersion is >= 1. On beta we'd record .acceptedVersion == 2 and
> ensure that .acceptedVersion is >= 2.

This is not the approach we've taken on Android. We used the very simple technique that DB versioning takes: bump the version for everyone, and the 'upgrade' method -- i.e., redisplaying the data choices notification -- only fires on Beta.

If you're not on Beta, you don't get a notification. If you're on Beta, you get a notification. If you were on Beta and you switch channels, you don't see another notification. If you were on another channel, and you switch to Beta, you don't see another notification, but (a) that edge case only applies after v2 reaches release, and (b) we already encounter this.

I would want some really strong justification for doing something different (and, as far as I can tell, more complicated) on desktop.

A separate prefs tree, or separate current versions for different channels, meets my definition of "more complicated".
As noted in the other bug, if you switch from release to beta and back, that would re-display the notification each time, which is undesirable. I'd rather record the version that the user actually saw.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> As noted in the other bug, if you switch from release to beta and back, that
> would re-display the notification each time, which is undesirable. I'd
> rather record the version that the user actually saw.

Why do you think it would do that?

You record a pref as soon as you (or don't show) the notification: version = 2. The version number only advances. You only display the notice when it advances. How would it hop back and cause redisplay?
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Bug 986582 turned this on now, so tracking to make sure the notices happen.
https://hg.mozilla.org/mozilla-central/rev/915628323414
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Note: this does need verification. I was not able to test this properly.
Hi Kamil, will verification of this bug be able to be completed before the end of our iteration on Monday April 14?
Flags: needinfo?(kamiljoz)
Progress:

I talked Benjamin via IRC and it appears that the notification doesn't appear right away once you've installed Nightly. I have Nightly installed on several VM's which I have running and waiting for one of them to display the notification, once that happens I'll change the channel-prefs.js to BETA and go through the verification.
Flags: needinfo?(kamiljoz)
I received the telemetry notification bar at the bottom of the browser and tried the following:

* Changing channel-pref.js to "beta", close the notification and never received it the second time around
* Tried launching the notification bar using the following command:
- Cc["@mozilla.org/datareporting/service;1"].getService().wrappedJSObject.policy.checkStateAndTrigger();

I couldn't get the telemetry notification bar the second time around. As this was only pushed into m-c, I need a way to trick the browser to think it's a BETA version so the notification bar can appear the second time once it's been dismissed the first time around as per this ticket.

gps, would you happen to know a way I can do this or verify this? If you're not sure, is there anyone that might know?

I did however I did go through the following test cases in the meantime:

- Ensured that the notification bar re-appears if the browser was closed and and the notification bar wasn't dismissed
- Ensured that selecting the "X" dismissed the bar and didn't re-appear after relaunching the browser several times
- Ensured that selecting "Choose what I share" correctly opened "Options -> Advanced -> Data Choices" without any issues
- Ensured that selecting closing the "Options" window using "OK", "Cancel", "X" dismissed the telemetry notification bar
- Updated the build while the notification bar was visible and ensured that it re-appeared once the update was completed
Flags: needinfo?(gps)
https://hg.mozilla.org/mozilla-central/rev/915628323414 landed without a commit message referencing a reviewer. I believe tree rules dictate that it should be backed out and landed with a proper commit message. I could be wrong.
I wouldn't have granted this patch r+. This bug needs more work.

* I disagree with the change to |set dataSubmissionPolicyAcceptedVersion(value)|. Ignoring the value and hardcoding the current policy version prohibits effective testing.

* There is no automated test coverage that bumping the current policy version will actually retrigger the notification! Now, this may happen to work, but there is no test coverage.

I would strongly, strongly urge automated test coverage here. If this ever regresses, users could be angry. FWIW, I believe the current privacy policy offers a legal escape hatch that absolves Mozilla of responsibility to notify (yay for lawyers anticipating). But our users deserve better.

I'm inclined to assume the patch is busted and Kamil's issues would be caught through automated testing. My response to the needinfo is to fix the patch and write more tests.
Status: RESOLVED → REOPENED
Flags: needinfo?(gps)
Resolution: FIXED → ---
Whiteboard: [no l10n impact] p=5 s=it-31c-30a-29b.2 [qa+] → [no l10n impact] p=5 s=it-31c-30a-29b.3 [qa+]
> * I disagree with the change to |set
> dataSubmissionPolicyAcceptedVersion(value)|. Ignoring the value and
> hardcoding the current policy version prohibits effective testing.

The "value" being set is true/false for whether the user accepted the policy, not a version. Changing that is not only an API change but seems unnecessary.

> * There is no automated test coverage that bumping the current policy
> version will actually retrigger the notification! Now, this may happen to
> work, but there is no test coverage.

This is true, and a review oversight on my part. This really does need to have an xpcshell test to make sure that the policy is reshown; we can mock the current channel in xpcshell tests by setting appinfo.

In any case Mano please help Kamil figure out a way to manually test this in ways that don't involve waiting 24 hours at a time ;-)
@Kamil (comment 25 and comment 26): Indeed, there is a serious problem verifying this bug before the merge happens (one needs a beta build with this fix in alongside an old beta build). The patch relies on nsIXULRuntime::defaultUpdateChannel, which is effectively set on build time. This means that, the only real way to test this before it lands on beta is to build a fake-beta build. There are some ways to partially test this (set the new pref for the aurora channel rather than the beta channel, or slightly tweaking the code), but it's very suboptimal. Since this is no trivial task, just let me known and I'll happily do the some QA on my mac later this week. I'm sorry I haven't made this issue clear earlier.

@gps few things...
1) The setter in question was removed - see comment 13.

2) Automated testing would be nice indeed, but I'm not sure how to test this in a way that actually helps us. Testing the use case covered here involves an update channel change (since we want to ensure the notification does not reappear when it shouldn't - it's as important as the other way around). The most simple use case (upgrading the version for all users) involves a restart. Sure, we can simulate this use case somewhat without a restart, but the value of such a test is going to be quite limited. Again, for the use case covered in this bug there's not much we can do without changing the code to be tests-friendly, which is something I'm usually trying to avoid.

This also addresses your concern about Kamil's issue: it's not that his test failed, but rather that it's pretty hard to test this.

3) I don't own this code, and I respect your last call here whatever it is. That said, I figured Benjamin's review was sufficient, and approached this bug with your discussion (in early comments here) in mind. Moreover, I worked on this bug slowly enough for reservations to be made on time (as you can see, the patch was rewritten once). So, if you do decide to back this out for any reason, I suggest to find a better owner to move this issue forward.

4) Indeed, my checkin comment lacked the r=bsmedberg. Such things happen to me every once in four years or so (give or take!). I once even omitted the bug number (this is when I learned about cvs commit -f). This time I noticed this right after I checked in the patch, but I figured that since the bug number was there and the patch did get reviewed, it's not the end of the world.

***
Moving forward, here's what I'm going to do unless the patch is backed out:
1) File a bug for *automated* testing of this feature. I'll happily take that bug if there is a good way to test this in a way that at least remotely resembles our use cases.

2) Verify it's fixed using my home made beta builds. Generally though, It'd be nice if QA could have access to fake-beta/aurora builds for such cases

3) *Iff* there's any issue, fix it here. Automated testing will be covered in a new bug.

I believe that for backlog-purposes I better keep this bug open until the fix is verified.
Automated testing via xpcshell shouldn't be too painful, because you can mock nsIXULAppInfo.defaultUpdateChannel to any value you need. See "createAppInfo" in http://mxr.mozilla.org/mozilla-central/source/browser/experiments/test/xpcshell/head.js#158 It currently doesn't have a defaultUpdateChannel property but adding one to that mock is simple.

For manual QA, I had suggested to Kamil that he can pretend to be on the beta channel by editing channel-prefs.js in the install. It turns out this isn't true because defaultUpdateChannel is hard-coded at build time. However, I think this shows a bug in the patch. For showing this notice we don't care what the *default* update channel is: we care what the *current* update channel is, which is retrieved using this code:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/UpdateChannel.jsm#23
and http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/Preferences.cpp#1321

For this bug we don't want the partner bits of the channel. I think we need to fix this code to use UpdateChannel.jsm without the partner extensions rather than using nsIXULAppInfo.defaultUpdateChannel.
I filed (and took) bug 999380 for xpcshell tests. We'll close this bug once manual testing shows it's fixed. The last patch should make it possible to override the channel in install.js.
Comment on attachment 8410196 [details] [diff] [review]
use UpdateChannel.jsm

I'd love for the UpdateChannel.jsm changes here also to have automated tests. If that is not covered by 999380 could you file a followup bug specifically for those tests? Might make a good mentored bug.
Attachment #8410196 - Flags: review?(benjamin) → review+
Where's the partners stuff documented? It seems like a very trivial test I could just attach here (btw, UpdateChannel.jsm isn't tested at all).
I don't know about the partner stuff! That's mconnor's gig though!
Flags: needinfo?(mconnor)
The tl;dr is that the docs suck.

* https://wiki.mozilla.org/Distribution_INI_File explains the file (which is found at %appdir%/distribution/distribution.ini)
* Typically we set app.partner.<something>=<partnerid>
* https://hg.mozilla.org/build/partner-repacks/file/b19586d8dd1f/partners/msn-canada/distribution/distribution.ini is an example of a random build.  (basically, we just copy everything in /distribution into the installer (unpack, add directory, repack)

I can answer lots more questions, but it's sort of irrelevant beyond that.
Flags: needinfo?(mconnor)
Hey Mike,

I guess my key question is whether or not it's viable to xpcshell-test partner stuff in our builds, and if it is, what's the right way to do so (I could just set the prefs UpdateChannel.jsm checks for, but that doesn't test much).
Flags: needinfo?(mconnor)
Don't bother doing large-scale testing of the partner stuff. For this, it should be sufficient to set the partner prefs and make sure that UpdateChannel.jsm honors them with aIncludePartners and doesn't in the other case.
Flags: needinfo?(mconnor)
Attachment #8413578 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/cf36a41c1648
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla30 → mozilla32
Whiteboard: [no l10n impact] p=5 s=it-31c-30a-29b.3 [qa+] → [no l10n impact] p=5 s=it-32c-31a-30b.1 [qa+]
Hey Kamil, this can be verified now by altering the update channel in install.js.
Flags: needinfo?(kamiljoz)
Keep in mind you want to test with builds done prior to the *first* checkin and *after" the *last* checkin.
Went through the following verification process using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-29-03-02-01-mozilla-central/

Can't seem to get this working when I change the channel to "beta" from "nightly" in channel-pref.js

- Installed the latest Nightly build mentioned above
- Moved the computer ahead by one day and launched Nightly (received the telemetry notification after a few minutes)
- Closed Nightly and moved the computer ahead by another day and re-launched Nightly (received the telemetry notification after a few minutes)
- Closed Nightly and moved the computer ahead by another day and re-launched Nightly, this time I dismissed the telemetry notification that appeared and closed Nightly
- Changed channel-pref.js from "nightly" to "beta" and moved the computer ahead by another day and then re-launched Nightly

Waited for about 10-15 minutes and tried moving the machine ahead a few days and couldn't get the telemetry notification to appear.

Please let me me know if I'm doing anything incorrectly, is there any other way to test this? Doesn't seem like the channel-pref.js route is working :(
Flags: needinfo?(kamiljoz)
What build did you use for the "older" build? With which channel?
The key tests here: start with a fresh profile in an old-enough beta build -> enable telemetry or not (both modes should be tested) -> start with the same profile in a new "beta" build.
Mano, this is what i've been trying so far:

* Install an older version of Nightly, I used the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/03/2014-03-10-03-02-01-mozilla-central/
* Moved the date of the computer by one day and launched Nightly (received the telemetry notification)
* Closed the telemetry notification and firefox and downloaded a newer Nightly build, used the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-02-03-02-02-mozilla-central/
* Once installed/upgraded (I tried both ways), changed channel-pref.js to "beta"
* Launched Nightly, never received the telemetry notification (waited about 6 minutes or so, usually it appears by now)
* Also tried moving the computers date ahead by one day and still didn't receive the notification

Let me know if I'm doing something incorrectly.. Is the build that I'm using old enough?? I've tried a few others that even older but still get the same results..
So, I think I found the problem.

notifyState remains STATE_NOTIFY_COMPLETE because dataSubmissionPolicyResponseDate is set.

That's my fault. But I have to say this is the kind of issue I was worried of when I chose a separate prefs branch in the first patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Oh well, testing this was quite tricky, but I can now confirm it works well. I've also included an xpcshell test at last.
Attachment #8417336 - Flags: review?(benjamin)
Attachment #8417336 - Flags: review?(benjamin) → review+
No longer blocks: 999380
Duplicate of this bug: 999380
Test using this code create their own preference branches, but these branches don't have the defaults I set in the defaults preferences file, so, as much as I don't like it, we need this fallback

https://hg.mozilla.org/integration/fx-team/rev/032022e0dfcd
https://hg.mozilla.org/mozilla-central/rev/f1a48f161f70
https://hg.mozilla.org/mozilla-central/rev/032022e0dfcd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
Hey Kamil,

Tomorrow's build is ready for Take 43 :)

Your testing process seems fine to me, and worked for me this time (except I moved the days 2 days at a time, just to be on the safe side, and only after launching the build).

I used the script from comment 26.
Flags: needinfo?(kamiljoz)
Awesome, looks like it's working Mano :)

Used the following builds for the verification process:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/03/2014-03-10-03-02-01-mozilla-central/
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-07-03-02-02-mozilla-central/

- Ensured that the telemetry notification appeared originally on Nightly without any issues
- Ensured that the telemetry notification re-appears until it's dismissed via "Choose what I Share" or "X"
- Ensured that the telemetry notification is dismissed when selecting either "Choose what I Share" or the "X" button
- Ensured that once the telemetry notification was dismissed via "Choose what I Share" or the "X" button, the notification doesn't re-appear again
- Ensured that the telemetry notification appeared once the channel-pref has been changed to "beta"
- Ensured that the telemetry notification re-appears until it's dismissed while on the "beta" channel
- Ensured that once the telemetry notification was dismissed on the "beta" channel via "Choose what I Share" or the "X" button, the notification doesn't re-appear again

Should this issue be updated to track+ FF32 since it was re-opened and the new change set was only on FF32?
Flags: needinfo?(kamiljoz)
Status: RESOLVED → VERIFIED
Whiteboard: [no l10n impact] p=5 s=it-32c-31a-30b.1 [qa+] → [no l10n impact] p=5 s=it-32c-31a-30b.1 [qa!]
@Benjamin,

1) Kamil's question (comment 56)
2) Since telemetry was enabled on the beta channel way before this bug was actually fixed, should we merge this to the other channels?
Flags: needinfo?(benjamin)
Thanks a ton for your close attention here, Kamil.
This should be uplifted to 31 since bug 986582 is in 31.

I'm still blocked on bug 993046 uplifting this to 30, but if that comes through soon I may request uplift of both of them to 30.
Flags: needinfo?(benjamin)
Comment on attachment 8413578 [details] [diff] [review]
test for UpdateChannel.get

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:
Attachment #8413578 - Flags: approval-mozilla-aurora?
Comment on attachment 8417336 [details] [diff] [review]
Make it work for real (including an xpcshell test)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:
Attachment #8417336 - Flags: approval-mozilla-aurora?
Comment on attachment 8417885 [details] [diff] [review]
checked in: don't rely on the preference being set

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:
Attachment #8417885 - Flags: approval-mozilla-aurora?
Changing this back to "Resolved" and "[qa+]" until it's tested in the Aurora channel once the changes have been pushed.
Status: VERIFIED → RESOLVED
Closed: 5 years ago5 years ago
Whiteboard: [no l10n impact] p=5 s=it-32c-31a-30b.1 [qa!] → [no l10n impact] p=5 s=it-32c-31a-30b.1 [qa+]
Attachment #8413578 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8417336 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8417885 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [no l10n impact] p=5 s=it-32c-31a-30b.1 [qa+] → [no l10n impact] p=5 s=it-32c-31a-30b.2 [qa+]
NB: needs verification on 31.
Keywords: verifyme
Went through verification using the following builds:

* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/03/2014-03-19-00-40-02-mozilla-aurora/ (older build)
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-21-00-40-03-mozilla-aurora/ (latest build)

- Installed the first build that's listed above and ensured that the telemetry notification appeared
- Dismissed the telemetry notification and installed the newer version listed above
- Before launching the newer build, changed channel-pref to "beta" and ensured that the telemetry notification re-appeared without any issues
- Also went through all the test cases listed in comment #56 without any issues
Status: RESOLVED → VERIFIED
Keywords: verifyme
Whiteboard: [no l10n impact] p=5 s=it-32c-31a-30b.2 [qa+] → [no l10n impact] p=5 s=it-32c-31a-30b.2 [qa!]
Per bsmedberg's request, Kamil, please do a quick check that this is by ready default on latest 31beta.
Flags: needinfo?(kamiljoz)
Went through verification using the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/31.0b1-candidates/build1/win32/en-US/

- ensured that telemetry is enabled by default
- moved the computers time ahead by one day and re-launched fx, ensured the telemetry experiment notification appeared without issues
- ensured that the telemetry notification doesn't appear after selecting "Choose What I Share"
- ensured that the telemetry notification doesn't appear after selecting the "X"
- downloaded the beta 30 (telemetry disabled) and updated via the "releasetest" channel to beta 31.0b1 (telemetry enabled)
Flags: needinfo?(kamiljoz)
Blocks: 1037120
You need to log in before you can comment on or make changes to this bug.