about:studies should show a notice if Shield is disabled

VERIFIED FIXED in Firefox 60

Status

()

P1
normal
VERIFIED FIXED
9 months ago
9 months ago

People

(Reporter: Felipe, Assigned: mythmon)

Tracking

unspecified
Firefox 60
Points:
---

Firefox Tracking Flags

(firefox60 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
If Shield is disabled (by the main pref, opt-out pref, or now by policies (bug 1429162), it would be nice if about:studies showed a text saying that it is in fact disabled, in order to not confuse users.
(Assignee)

Comment 1

9 months ago
This is a feature of the Normandy Client, not of the Shield study program.
Component: General → Normandy Client
Product: Shield → Firefox
(Assignee)

Updated

9 months ago
Priority: -- → P2
(Assignee)

Updated

9 months ago
Assignee: nobody → mcooper
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 3

9 months ago
Ethan, I tagged you for review specifically for the React parts of this patch, since Gijs has asked for other reviewers for React code in the past, and I'm guessing you have some experience there. If I guessed wrong, let me know and I'll tag someone else for review.
Comment hidden (mozreview-request)

Comment 5

9 months ago
mozreview-review
Comment on attachment 8949570 [details]
Bug 1435838 - Show a notice on about:studies if Studies are disabled

https://reviewboard.mozilla.org/r/218940/#review224680


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/extensions/shield-recipe-client/test/browser/browser_about_studies.js:186
(Diff revision 2)
> +    try {
> +      RecipeRunner.disable();
> +
> +      await ContentTask.spawn(browser, null, async () => {
> +        const doc = content.document;
> +        await ContentTaskUtils.waitForCondition(() => !!doc.querySelector(".info-box-content > span"))

Error: Missing semicolon. [eslint: semi]
Comment hidden (mozreview-request)

Comment 7

9 months ago
mozreview-review
Comment on attachment 8949570 [details]
Bug 1435838 - Show a notice on about:studies if Studies are disabled

https://reviewboard.mozilla.org/r/218940/#review224782

Tentative r+, assuming someone else reviews the react stuff. We should have a bug on using proper localization here.

(in the style of Cato / grumpy-old-man-get-off-my-lawn) Moreover, I would like us to stop shipping a copy of react just for this about: page.

::: browser/extensions/shield-recipe-client/content/about-studies/shield-studies.js:84
(Diff revision 3)
> +      return null;
> +    }
> +
> +    let info = null;
> +    if (studies.length === 0) {
> +      info = r("p", {className: "study-list-info"}, "You have not participated in any studies.");

Huh, so, is there a bug on localizing these messages?

::: browser/extensions/shield-recipe-client/lib/RecipeRunner.jsm:91
(Diff revision 3)
> -    if (!this.enabled) {
> +    if (this.enabled) {
> -      return;
> -    }
> -    this.unregisterTimer();
> +      this.unregisterTimer();
> +    }
> +    // this.enabled may be null, so always set it to false

Null or undefined?

::: browser/extensions/shield-recipe-client/test/browser/browser_about_studies.js:166
(Diff revision 3)
> +  AddonStudies.withStudies([]),
> +  withAboutStudies,
> +  async function testStudyListing(studies, browser) {
> +    await ContentTask.spawn(browser, null, async () => {
> +      const doc = content.document;
> +      await ContentTaskUtils.waitForCondition(() => doc.querySelectorAll(".study-list").length);

Nit: could just use `querySelector()` and falsy-check (ie null-check) that.
Attachment #8949570 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 8

9 months ago
mozreview-review
Comment on attachment 8949570 [details]
Bug 1435838 - Show a notice on about:studies if Studies are disabled

https://reviewboard.mozilla.org/r/218940/#review224892

My r+ doesn't count for much but the React stuff looks OK to me. I don't claim to have perfect understanding of the context here but it mostly consists of moving some pretty typical React stuff around. The browser stuff looks sane to me too.
Attachment #8949570 - Flags: review?(eglassercamp) → review+

Comment 9

9 months ago
mozreview-review
Comment on attachment 8949570 [details]
Bug 1435838 - Show a notice on about:studies if Studies are disabled

https://reviewboard.mozilla.org/r/218940/#review224914

::: browser/extensions/shield-recipe-client/lib/RecipeRunner.jsm:91
(Diff revision 3)
> -    if (!this.enabled) {
> +    if (this.enabled) {
> -      return;
> -    }
> -    this.unregisterTimer();
> +      this.unregisterTimer();
> +    }
> +    // this.enabled may be null, so always set it to false

Doesn't the init() guarantee that it's either null or a boolean?

::: browser/extensions/shield-recipe-client/lib/RecipeRunner.jsm:164
(Diff revision 3)
> -      log.warn(`Disabling shield because ${API_URL_PREF} is not an HTTPS url: ${apiUrl}.`);
> +      log.warn(`Disabling Shield because ${API_URL_PREF} is not an HTTPS url: ${apiUrl}.`);
>        this.disable();
>        return;
>      }
>  
> +    log.debug(`Enabling Shield`);

Wouldn't it be very useful to use 
```
`Enabling Shield for ${API_URL_PREF}`
```
(Assignee)

Comment 10

9 months ago
mozreview-review-reply
Comment on attachment 8949570 [details]
Bug 1435838 - Show a notice on about:studies if Studies are disabled

https://reviewboard.mozilla.org/r/218940/#review224782

I'm going to get someone from my team to review the react parts. I knew you wouldn't want to cover that part :)

You're 100% right about the localization bit. A while ago we decided to block localization this UI on Fluent being available. In retrospect, that may have been the wrong choice.

As far as your grumpiness about having this having its own copy of React: I agree. I just filed bug 1437122 to track that. Right now I think it should block on converting the normandy client from a system add-on to a normal component (bug 1436113).

> Huh, so, is there a bug on localizing these messages?

Yes, bug 1435875.

> Null or undefined?

It is explicitly set to null in the constructor.

> Nit: could just use `querySelector()` and falsy-check (ie null-check) that.

That would also work. Is it better in some way? It is slightly shorter, but it seems to be about the same otherwise.
(Assignee)

Comment 11

9 months ago
mozreview-review-reply
Comment on attachment 8949570 [details]
Bug 1435838 - Show a notice on about:studies if Studies are disabled

https://reviewboard.mozilla.org/r/218940/#review224914

> Doesn't the init() guarantee that it's either null or a boolean?

I'm not sure what you're getting at. `init()` *does* guarantee that `this.enabled` will always be null or a boolean, which is exactly the set of scenarios this code is handling.

> Wouldn't it be very useful to use 
> ```
> `Enabling Shield for ${API_URL_PREF}`
> ```

I don't see that being useful in the debugging I've done. The API url doesn't change much, and the question I was trying to answer with this log line wouldn't be concerned about the API anyways. I was specifically trying to figure out when RecipeRunner changed states. Do you think it would be useful to answer other questsions?

Comment 12

9 months ago
(In reply to Mike Cooper [:mythmon] from comment #10)
> > Nit: could just use `querySelector()` and falsy-check (ie null-check) that.
> 
> That would also work. Is it better in some way? It is slightly shorter, but
> it seems to be about the same otherwise.

My main reason for suggesting it was it's shorter and more obviously an "is this present" check than "how many of these things have we got".

Otherwise, in theory querySelector will find a thing once and can then stop immediately, whereas querySelectorAll has to build the entire list (especially if you check .length, otherwise in theory it could be a bit more lazy - though not really in practice because it's supposed to be a non-live nodelist, IIRC, unlike getElementsBy* ). In practice, I would be thoroughly surprised if both gecko and servo DOM implementations didn't have a shortcut cache for items based on IDs, and possibly also class names. Also, it's a test, so really we don't care about performance much.

Comment 13

9 months ago
Pushed by mcooper@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76a698b6e418
Show a notice on about:studies if Studies are disabled r=Gijs,glasserc

Comment 14

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/76a698b6e418
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60

Comment 15

9 months ago
I can see this implemented in Latest Nightly in 64 bit Linux

Build ID 	20180217100053
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
QA Whiteboard: [testday-20180216]

Comment 16

9 months ago
I can see this implemented in Latest Nightly 60.0a1 (2018-02-17) (32-bit) windows 7(32 bit)

Build ID 	20180217100053
Mozilla/5.0 (Windows NT 6.1; rv:60.0) Gecko/20100101 Firefox/60.0

 [testday-20180216]

Comment 17

9 months ago
As this issue is fixed in both Linux (comment 15) and Windows (comment 16) I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
I can also confirm that an informative notification is displayed in about:studies, if Shield is disabled on the latest Nightly 60.0a1(2018-02-20)under macOS 10.13.
You need to log in before you can comment on or make changes to this bug.