Bug 838312 - Well-defined API for opting out of FHR data upload; r=rnewman

You should no longer set policy.healthReportUploadEnabled directly.
Instead, call policy.recordHealthReportUploadEnabled(). This will
trigger data deletion as needed.

browser/base/content/abouthealthreport/abouthealth.js

@@ -72,7 +72,8 @@ function refreshJSONPayload() {
 }
 
 function onOptInClick() {
-  policy.healthReportUploadEnabled = true;
+  policy.recordHealthReportUploadEnabled(true,
+                                         "Clicked opt in button on about page.");
   refreshWithDataSubmissionFlag(true);
 }
 
@@ -88,8 +89,8 @@ function onOptOutClick() {
     return;
   }
 
-  policy.healthReportUploadEnabled = false;
-  reporter.requestDeleteRemoteData("Clicked opt out button on about page.");
+  policy.recordHealthReportUploadEnabled(false,
+                                         "Clicked opt out button on about page.");
   refreshWithDataSubmissionFlag(false);
   updateView("disabled");
 }

browser/components/preferences/advanced.js

@@ -238,7 +238,8 @@ var gAdvancedPane = {
     }
 
     let checkbox = document.getElementById("submitHealthReportBox");
-    policy.healthReportUploadEnabled = checkbox.checked;
+    policy.recordHealthReportUploadEnabled(checkbox.checked,
+                                           "Checkbox from preferences pane");
   },
 #endif
 

browser/components/preferences/in-content/advanced.js

@@ -227,7 +227,8 @@ var gAdvancedPane = {
     }
 
     let checkbox = document.getElementById("submitHealthReportBox");
-    policy.healthReportUploadEnabled = checkbox.checked;
+    policy.recordHealthReportUploadEnabled(checkbox.checked,
+                                           "Checkbox from preferences pane");
   },
 #endif
 

services/datareporting/policy.jsm

@@ -621,6 +621,8 @@ DataReportingPolicy.prototype = Object.freeze({
     return !!this._healthReportPrefs.get("uploadEnabled", true);
   },
 
+  // External callers should update this via `recordHealthReportUploadEnabled`
+  // to ensure appropriate side-effects are performed.
   set healthReportUploadEnabled(value) {
     this._healthReportPrefs.set("uploadEnabled", !!value);
   },
@@ -663,6 +665,39 @@ DataReportingPolicy.prototype = Object.freeze({
     this.dataSubmissionPolicyAccepted = false;
   },
 
+  /**
+   * Record the user's intent for whether FHR should upload data.
+   *
+   * This is the preferred way for the application to record a user's
+   * preference on whether Firefox Health Report should upload data to
+   * a server.
+   *
+   * If upload is disabled through this API, a request for remote data
+   * deletion is initiated automatically.
+   *
+   * If upload is being disabled and this operation is scheduled to
+   * occur immediately, a promise will be returned. This promise will be
+   * fulfilled when the deletion attempt finishes. If upload is being
+   * disabled and a promise is not returned, callers must poll
+   * `haveRemoteData` on the HealthReporter instance to see if remote
+   * data has been deleted.
+   *
+   * @param flag
+   *        (bool) Whether data submission is enabled or disabled.
+   * @param reason
+   *        (string) Why this value is being adjusted. For logging
+   *        purposes only.
+   */
+  recordHealthReportUploadEnabled: function (flag, reason="no-reason") {
+    this.healthReportUploadEnabled = flag;
+
+    if (flag) {
+      return null;
+    }
+
+    return this.deleteRemoteData(reason);
+  },
+
   /**
    * Request that remote data be deleted.
    *

services/datareporting/tests/xpcshell/test_policy.js

@@ -733,3 +733,30 @@ add_test(function test_polling_implicit_acceptance() {
   policy.startPolling();
 });
 
+add_test(function test_record_health_report_upload_enabled() {
+  let [policy, policyPrefs, hrPrefs, listener] = getPolicy("record_health_report_upload_enabled");
+
+  // Preconditions.
+  do_check_false(policy.pendingDeleteRemoteData);
+  do_check_true(policy.healthReportUploadEnabled);
+  do_check_eq(listener.requestRemoteDeleteCount, 0);
+
+  // User intent to disable should immediately result in a pending
+  // delete request.
+  policy.recordHealthReportUploadEnabled(false, "testing 1 2 3");
+  do_check_false(policy.healthReportUploadEnabled);
+  do_check_true(policy.pendingDeleteRemoteData);
+  do_check_eq(listener.requestRemoteDeleteCount, 1);
+
+  // Fulfilling it should make it go away.
+  listener.lastRemoteDeleteRequest.onNoDataAvailable();
+  do_check_false(policy.pendingDeleteRemoteData);
+
+  // User intent to enable should get us back to default state.
+  policy.recordHealthReportUploadEnabled(true, "testing 1 2 3");
+  do_check_false(policy.pendingDeleteRemoteData);
+  do_check_true(policy.healthReportUploadEnabled);
+
+  run_next_test();
+});
+