1435838 - about:studies should show a notice if Shield is disabled

Status: VERIFIED FIXED

(Firefox :: Normandy Client, enhancement, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 1

[Michael Cooper [:mythmon]]

This is a feature of the Normandy Client, not of the Shield study program.

Comment 2

[Michael Cooper [:mythmon]]

Attachment: Bug 1435838 - Show a notice on about:studies if Studies are disabled

Review commit: https://reviewboard.mozilla.org/r/218940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218940/

Comment 3

[Michael Cooper [:mythmon]]

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 4

[Michael Cooper [:mythmon]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218940/diff/1-2/

Comment 5

[Code Review Bot [:reviewbot]]

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 6

[Michael Cooper [:mythmon]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218940/diff/2-3/

Comment 7

[:Gijs (he/him)]

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.

Comment 8

[Ethan Glasser-Camp (:glasserc)]

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.

Comment 9

[Peter Bengtsson [:peterbe]]

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}`
```

Comment 10

[Michael Cooper [:mythmon]]

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.

Comment 11

[Michael Cooper [:mythmon]]

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

[:Gijs (he/him)]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76a698b6e418
Show a notice on about:studies if Studies are disabled r=Gijs,glasserc

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/76a698b6e418

Comment 15

[Sayed [:LiquidO2]]

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

Comment 16

[Md. Rahimul Islam]

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

[Sayed [:LiquidO2]]

As this issue is fixed in both Linux (comment 15) and Windows (comment 16) I am marking this bug as verified fixed.

Comment 18

[Anca Soncutean, Desktop QA]

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.