Bug 575767: Stop Feedback from collection data until the user has bee…

…n notified. r=dtownsend --HG-- branch : GECKO20b1_20100628_RELBRANCH
rnewman · Jun 30, 2010 · 6bcd9de · 6bcd9de
1 parent 6660cd9
commit 6bcd9de
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 26 deletions.
diff --git a/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/setup.js b/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/setup.js
@@ -62,7 +62,7 @@ let TestPilotSetup = {
   startupComplete: false,
   _shortTimer: null,
   _longTimer: null,
-  _remoteExperimentLoader: null,
+  _remoteExperimentLoader: null, // TODO make this a lazy initializer too?
   taskList: [],
   version: "",
 
@@ -372,10 +372,16 @@ let TestPilotSetup = {
                                                     iconClass, showSubmit,
 						    showAlwaysSubmitCheckbox,
                                                     linkText, linkUrl,
-						    isExtensionUpdate) {
+						    isExtensionUpdate,
+                                                    onCloseCallback) {
+    /* TODO: Refactor the arguments of this function, it's getting really
+     * unweildly.... maybe pass in an object, or even make a notification an
+     * object that you create and then call .show() on. */
+
     // If there are multiple windows, show notifications in the frontmost
     // window.
-    let doc = this._getFrontBrowserWindow().document;
+    let window = this._getFrontBrowserWindow();
+    let doc = window.document;
     let popup = doc.getElementById("pilot-notification-popup");
 
     let anchor;
@@ -422,14 +428,14 @@ let TestPilotSetup = {
 	    "testpilot.notification.update"));
 	submitBtn.onclick = function() {
           this._extensionUpdater.check(EXTENSION_ID);
-          self._hideNotification();
+          self._hideNotification(window, onCloseCallback);
 	};
       } else {
         submitBtn.setAttribute("label",
 	  this._stringBundle.GetStringFromName("testpilot.submit"));
         // Functionality for submit button:
         submitBtn.onclick = function() {
-          self._hideNotification();
+          self._hideNotification(window, onCloseCallback);
           if (showAlwaysSubmitCheckbox && alwaysSubmitCheckbox.checked) {
             self._prefs.setValue(ALWAYS_SUBMIT_DATA, true);
           }
@@ -464,7 +470,7 @@ let TestPilotSetup = {
 	  } else {
             self._openChromeless(linkUrl);
 	  }
-          self._hideNotification();
+          self._hideNotification(window, onCloseCallback);
         }
       };
       link.setAttribute("hidden", false);
@@ -473,7 +479,7 @@ let TestPilotSetup = {
     }
 
     closeBtn.onclick = function() {
-      self._hideNotification();
+      self._hideNotification(window, onCloseCallback);
     };
 
     // Show the popup:
@@ -487,13 +493,19 @@ let TestPilotSetup = {
     window.TestPilotWindowUtils.openChromeless(url);
   },
 
-  _hideNotification: function TPS__hideNotification() {
-    let window = this._getFrontBrowserWindow();
+  _hideNotification: function TPS__hideNotification(window, onCloseCallback) {
+    /* Note - we take window as an argument instead of just using the frontmost
+     * window because the window order might have changed since the notification
+     * appeared and we want to be sure we close the notification in the same
+     * window as we opened it in! */
     let popup = window.document.getElementById("pilot-notification-popup");
     popup.hidden = true;
     popup.setAttribute("open", "false");
     popup.removeAttribute("tpisextensionupdate");
     popup.hidePopup();
+    if (onCloseCallback) {
+      onCloseCallback();
+    }
   },
 
   _isShowingUpdateNotification : function() {
@@ -543,26 +555,28 @@ let TestPilotSetup = {
     if (this._prefs.getValue(POPUP_SHOW_ON_NEW, false)) {
       for (i = 0; i < this.taskList.length; i++) {
         task = this.taskList[i];
-        if (task.status == TaskConstants.STATUS_STARTING ||
+        if (task.status == TaskConstants.STATUS_PENDING ||
             task.status == TaskConstants.STATUS_NEW) {
           if (task.taskType == TaskConstants.TYPE_EXPERIMENT) {
 	    this._showNotification(
-	      task, true,
+	      task, false,
 	      this._stringBundle.formatStringFromName(
 		"testpilot.notification.newTestPilotStudy.message",
 		[task.title], 1),
 	      this._stringBundle.GetStringFromName(
 		"testpilot.notification.newTestPilotStudy"),
 	      "new-study", false, false,
 	      this._stringBundle.GetStringFromName("testpilot.moreInfo"),
-	      task.defaultUrl);
-            // Having shown the notification, update task status so that this
-            // notification won't be shown again.
-            task.changeStatus(TaskConstants.STATUS_IN_PROGRESS, true);
+	      task.defaultUrl, false, function() {
+                /* on close callback (Bug 575767) -- when the "new study
+                 * starting" popup is dismissed, then the study can start. */
+                task.changeStatus(TaskConstants.STATUS_IN_PROGRESS, true);
+                TestPilotSetup.reloadRemoteExperiments();
+              });
             return;
           } else if (task.taskType == TaskConstants.TYPE_SURVEY) {
 	    this._showNotification(
-	      task, true,
+	      task, false,
 	      this._stringBundle.formatStringFromName(
 		"testpilot.notification.newTestPilotSurvey.message",
 		[task.title], 1),

diff --git a/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/tasks.js b/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/tasks.js
@@ -203,6 +203,7 @@ var TestPilotTask = {
   },
 
   onDetailPageOpened: function TestPilotTask_onDetailPageOpened(){
+    // TODO fold this into loadPage()?
   },
 
   checkDate: function TestPilotTask_checkDate() {
@@ -444,14 +445,9 @@ TestPilotExperiment.prototype = {
   },
 
   experimentIsRunning: function TestPilotExperiment_isRunning() {
-    if (this._optInRequired) {
-      return (this._status == TaskConstants.STATUS_STARTING ||
-              this._status == TaskConstants.STATUS_IN_PROGRESS);
-    } else {
-      // Tests that don't require extra opt-in should start running even
-      // if you haven't seen them yet.
-      return (this._status < TaskConstants.STATUS_FINISHED);
-    }
+    // bug 575767
+    return (this._status == TaskConstants.STATUS_STARTING ||
+            this._status == TaskConstants.STATUS_IN_PROGRESS);
   },
 
   // Pass events along to handlers:
@@ -599,11 +595,23 @@ TestPilotExperiment.prototype = {
       }
     }
 
-    // No-opt-in required tests skip PENDING and go straight to STARTING.
+    // If the notify-on-new-study pref is turned off, and the test doesn't
+    // require opt-in, then it can jump straight ahead to STARTING.
     if (!this._optInRequired &&
-        this._status < TaskConstants.STATUS_STARTING &&
+        !Application.prefs.getValue("extensions.testpilot.popup.showOnNewStudy",
+                                    false) &&
+        (this._status == TaskConstants.STATUS_NEW ||
+         this._status == TaskConstants.STATUS_PENDING)) {
+      this._logger.info("Skipping pending and going straight to starting.");
+      this.changeStatus(TaskConstants.STATUS_STARTING, true);
+    }
+
+    // If a study is STARTING, and we're in the right date range,
+    // then start it, and move it to IN_PROGRESS.
+    if ( this._status == TaskConstants.STATUS_STARTING &&
         currentDate >= this._startDate &&
         currentDate <= this._endDate) {
+      this._logger.info("Study now starting.");
       let uuidGenerator =
         Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
       let uuid = uuidGenerator.generateUUID().toString();