Closed
Bug 1435838
Opened 7 years ago
Closed 7 years ago
about:studies should show a notice if Shield is disabled
Categories
(Firefox :: Normandy Client, enhancement, P1)
Firefox
Normandy Client
Tracking
()
VERIFIED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | verified |
People
(Reporter: Felipe, Assigned: mythmon)
Details
Attachments
(1 file)
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•7 years ago
|
||
This is a feature of the Normandy Client, not of the Shield study program.
Component: General → Normandy Client
Product: Shield → Firefox
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mcooper
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment 15•7 years 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•7 years 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•7 years 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
Comment 18•7 years ago
|
||
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.
Description
•