Closed Bug 813226 Opened 7 years ago Closed 7 years ago

Add "always accept" preference

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set

Tracking

(firefox18 fixed, firefox19 fixed, firefox20 fixed)

RESOLVED FIXED
Firefox 20
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: gps, Assigned: gps)

Details

Attachments

(1 file, 1 obsolete file)

The B2G ADU ping is always enabled, so we need a way to short circuit display and acceptance of the data upload policy. This patch provides that.
Attachment #683208 - Flags: review?(rnewman)
Minor change: We no longer record user acceptance of the policy. This makes it easier to handle the scenario where the ping didn't require user notification or acceptance and later does. This scenario may well happen with B2G.
Assignee: nobody → gps
Attachment #683208 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #683208 - Flags: review?(rnewman)
Attachment #683212 - Flags: review?(rnewman)
Comment on attachment 683212 [details] [diff] [review]
Ability to short-circuit policy acceptance, v2

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

r+ with name change, slightly more thorough precondition in test, typos.

I expect this'll need revision in the future, if some amount of data is always submitted, with opt-in for richer… but we'll burn that bridge when we come to it.

::: services/healthreport/healthreport-prefs.js
@@ +9,5 @@
>  pref("healthreport.logging.consoleLevel", "Warn");
>  pref("healthreport.policy.currentDaySubmissionFailureCount", 0);
>  pref("healthreport.policy.dataSubmissionEnabled", true);
>  pref("healthreport.policy.dataSubmissionPolicyAccepted", false);
> +pref("healthreport.policy.dataSubmissionPolicyAlwaysAccept", false);

s/AlwaysAccept/BypassAcceptance

::: services/healthreport/policy.jsm
@@ +361,5 @@
>  
>    /**
> +   * Short circuit policy checking and always assume acceptance.
> +   *
> +   * This shuld never be set by theer. Instead, it is a per-application or

"This shuld never be set by theer" indeed.

@@ +362,5 @@
>    /**
> +   * Short circuit policy checking and always assume acceptance.
> +   *
> +   * This shuld never be set by theer. Instead, it is a per-application or
> +   * per-deployment defalt pref.

s/defalt/default

::: services/healthreport/tests/xpcshell/test_policy.js
@@ +163,5 @@
> +
> +  prefs.set("dataSubmissionPolicyAlwaysAccept", true);
> +  defineNow(policy, new Date(policy.nextDataSubmissionDate.getTime()));
> +  policy.checkStateAndTrigger();
> +  do_check_eq(listener.requestDataUploadCount, 1);

Be explicit here and make sure that the acceptance pref is false -- you're testing state and not isolating all of it.
Attachment #683212 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #2)
> ::: services/healthreport/policy.jsm
> @@ +361,5 @@
> >  
> >    /**
> > +   * Short circuit policy checking and always assume acceptance.
> > +   *
> > +   * This shuld never be set by theer. Instead, it is a per-application or
> 
> "This shuld never be set by theer" indeed.

My keyboard has been doing weird things today - dropping key presses, etc. I swear it has!
> 
> ::: services/healthreport/tests/xpcshell/test_policy.js
> @@ +163,5 @@
> > +
> > +  prefs.set("dataSubmissionPolicyAlwaysAccept", true);
> > +  defineNow(policy, new Date(policy.nextDataSubmissionDate.getTime()));
> > +  policy.checkStateAndTrigger();
> > +  do_check_eq(listener.requestDataUploadCount, 1);
> 
> Be explicit here and make sure that the acceptance pref is false -- you're
> testing state and not isolating all of it.

test_prefs does this. However, the extra coverage can't hurt.
Comment on attachment 683212 [details] [diff] [review]
Ability to short-circuit policy acceptance, v2

Bulk-setting approval flags for FHR landing for FxOS ADU ping (Bug 788894).
Attachment #683212 - Flags: approval-mozilla-beta?
Attachment #683212 - Flags: approval-mozilla-aurora?
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/e1a3bae737a7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Attachment #683212 - Flags: approval-mozilla-beta?
Attachment #683212 - Flags: approval-mozilla-beta+
Attachment #683212 - Flags: approval-mozilla-aurora?
Attachment #683212 - Flags: approval-mozilla-aurora+
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla20 → Firefox 20
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.