1191336 - Don't show the datachoices infobar when sending deletion pings

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Alessio Placitelli [:Dexter]]

When sending a deletion ping on the first run, if the infobar wasn't shown already, disabling datareporting.healthreport.uploadEnabled immediately triggers the infobar.

This happens because disabling upload generates a deletion ping that needs to be sent to the servers.

FHR doesn't show the data choices infobar when sending deletion requests.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1191336.patch

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Don't show the infobar when checking if user can upload

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Allow deletion pings to be sent w/o prompting the datachoices infobar

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8644954 [details] [diff] [review]
part 2 - Don't show the infobar when checking if user can upload

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

::: browser/base/content/test/general/browser_datachoices_notification.js
@@ +133,5 @@
>    Assert.ok(!TelemetryReportingPolicy.canUpload(),
> +            "User should not be allowed to upload.");
> +
> +  // Trigger the infobar.
> +  TelemetryReportingPolicy.testShowInfobar();

Lets trigger the timer this waits on instead using a policy object.
Ditto below.

::: toolkit/components/telemetry/TelemetryReportingPolicy.jsm
@@ +158,5 @@
>  
>    /**
> +   * Test only method, used to show the infobar.
> +   */
> +  testShowInfobar: function() {

We don't need this, we can just trigger the timers directly here similar to other Telemetry test.

@@ +345,5 @@
>      }
>  
> +    // Submission is enabled. We enable upload if user is notified or we need to bypass
> +    // the policy.
> +    const BYPASS_NOTIFICATION = Preferences.get(PREF_BYPASS_NOTIFICATION, false);

Nit: const bypass = ...

@@ +361,5 @@
>    },
>  
>    /**
>     * Make sure the user is notified about the policy before allowing upload.
>     * @return {Boolean} True if the user was notified, false otherwise.

Update this.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8644983 [details] [diff] [review]
part 1 - Allow deletion pings to be sent w/o prompting the datachoices infobar

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

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +676,5 @@
> +      if (isDeletionPing(ping)) {
> +        return TelemetryStorage.saveDeletionPing(ping);
> +      } else {
> +        return TelemetryStorage.savePendingPing(ping);
> +      }

This is duplicated below and in sendPings(), let's move this to a helper function.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Allow deletion pings to be sent w/o prompting the datachoices infobar - v2

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Don't show the infobar when checking if user can upload - v2

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8645051 [details] [diff] [review]
part 2 - Don't show the infobar when checking if user can upload - v2

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

::: browser/base/content/test/general/browser_datachoices_notification.js
@@ +146,4 @@
>    Assert.ok(!TelemetryReportingPolicy.canUpload(),
> +            "User should not be allowed to upload.");
> +
> +  // Trigger the infobar.

This section is duplicated below, lets move it to a helper function triggerInfoBar(expectedTimeout) or so.

@@ +149,5 @@
> +  // Trigger the infobar.
> +  let showInfobarCallback = null;
> +  fakeShowPolicyTimeout((callback, timeout) => showInfobarCallback = callback, () => {});
> +  sendSessionRestoredNotification();
> +  Assert.ok(!!showInfobarCallback, "Must have a timer callback.")

While we are here, we should also check the timeout being correct.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1 - Don't show the datachoices infobar when sending deletion pings

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2 - Don't trigger the datachoices infobar when checking if user is allowed to upload

Comment 11

[Georg Fritzsche [:gfritzsche]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8886969b66fe

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/20565e58ac99
https://hg.mozilla.org/integration/fx-team/rev/f63aa8d0a399

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/20565e58ac99
https://hg.mozilla.org/mozilla-central/rev/f63aa8d0a399