575767 - FF4 Beta 1 Test Pilot starts collecting data automatically?

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Justin Dolske [:Dolske]]

I just grabbed the build1 candidate to look at the new feedback mechanism and integrated Test Pilot.

Seems like it started to collect data without asking me? I thought it required an explicit opt-in. Under "Current Studies" it says "Currently collecting data" for "Menu Item Usage v2". http://grab.by/5cdA

Comment 1

[Justin Dolske [:Dolske]]

And, uhh, just now out of nowhere (I was typing in Thunderbird), I got a popup notification from Firefox saying that the "Menu Item Usage v2 study is now beginning."

Comment 2

[Justin Dolske [:Dolske]]

Just verified that it's definitely collecting data; not just a UI glitch. As I use menu items data is stored in $profile/testpilot_menu_item_usage_2.sqlite

Comment 3

[Jono Xia]

Hi Dolske,
Not a bug.  This is as designed.  We decided not to have the opt-in.  Please see the discussion in bug 570870.

Comment 4

[Justin Dolske [:Dolske]]

I can't, because I don't have access to that bug. (!)

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

And you're not using a new profile?

The design is:

 - nothing happens for first 10 minutes
 - first survey presented is background survey
 - data isn't collected until users are notified that a study starts
 - data gets discarded if a user opts out

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Agree with comment 3, but this is different than no-opt in, this is no-notification-that-we've-started-to-collect

Comment 7

[Jono Xia]

OK, it seems I misunderstood that part of the design.  I wrote it to notify users "when the study starts", but in the special case of Firefox startup to delay the notification by ten minutes.  I didn't realize that we also wanted to delay starting the study in that case.

I will write a patch that puts each study into a dormant state until its notification has been displayed, if the "notify me on new studies" pref is turned on.  (This pref is on by default in Feedback, off by default in Test Pilot.)

Do we want to clear the dormant state and start the study:
1. As soon as the notification is displayed?
2. Some fixed amount of time after the notification is displayed?
3. After the user has interacted with the notification in some way?

Please note that since we aren't requiring an opt-in, there's no guarantee that the user has read the notification, or that they even saw it (since it will disappear as soon as they click anywhere else, it's easy to dismiss by accident).

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #7)
> 3. After the user has interacted with the notification in some way?

As soon as the notification is dismissed, even if by accident. We want to know that it's been shown and the user has been at their computer since.

Comment 9

[Jono Xia]

Roger.  Will try to get this patch done before I leave work today.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Jono: any update? This is a release blocker, we need to respin builds because of it.

Comment 11

[Jono Xia]

Attachment: Patch implementing the requested changes.

OK, this patch makes it so that if notifications for new studies are preffed on, then new studies get put into STATUS_PENDING until the notification is closed either by the close button or the "more info" link being clicked, at which point they change to STATUS_IN_PROGRESS and start running.

It makes the "new study" notification non-fragile as well, meaning that it won't disappear just because you clicked outside it: you have to click the close button or the more info link to dismiss it.  This is so the the user will have to actually interact with the notification before the study starts, making it less likely that it starts without the user knowing it.

Comment 12

[Jono Xia]

Note: I've tested the patch to the best of my ability on my own system, but I consider these to be somewhat "risky" changes because they affect the core logic of how tasks change state, which could have far-reaching effects.  I would greatly appreciate if others could test the patch and see if they find any regressions.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

I tested it locally and looked to work fine:

http://grab.by/5cG9 - not collecting data
http://grab.by/5cHn - still not collecting data as I haven't dismissed the dialog

Comment 14

[Jono Xia]

Attachment: Patch implementing the requested changes, without dump().

Oops - I accidentally left a debugging "dump" statement in the previous patch.  Uploaded a new patch that fixes this.  Use the new patch instead please.

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 455144 [details] [diff] [review]
Patch implementing the requested changes, without dump().

r=me and landed: http://hg.mozilla.org/mozilla-central/rev/accdced3e5b8
http://hg.mozilla.org/mozilla-central/rev/65c30e4ee631