1963764 - "Report broken site" in the protections panel + hamburger menu should be hidden when the WebCompat Reporter is disabled and/or when the `DisableFeedbackCommands` policy is configured

Status: REOPENED

(Web Compatibility :: Tooling & Investigations, defect, P1)

Description

[celenity [:celenity]]

Steps to reproduce:

Set extensions.webcompat-reporter.enabled & ui.new-webcompat-reporter.enabled to false (via the about:config or some other means).

Actual results:

Observe that the Report broken site option is still visible at the top of the protections panel.

Expected results:

When the WebCompat Reporter is disabled, this option should also be hidden from the protections panel. At the very least, there needs to be some kind of preference to control this. This is very important for forks/custom configs/custom user.js files/etc. to prevent users from filing invalid reports and wasting Mozilla's time/resources.

Comment 1

[celenity [:celenity]]

It just occurred to me that this is also important to fix for enterprise/policies, as this option appears regardless of the DisableFeedbackCommands policy.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::Protections UI' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Mike Kaply [:mkaply]]

Yep, I agree. I didn't even know it was on the protections panel.

Comment 4

[celenity [:celenity]]

It looks like there's also a new Report broken site option in the hamburger menu as well (towards the bottom, above Help) - which also isn't covered by the existing preferences (extensions.webcompat-reporter.enabled & ui.new-webcompat-reporter.enabled) and/or the DisableFeedbackCommands policy.

Comment 5

[YUKI "Piro" Hiroshi]

This is a regression of the bug 1952334. The logic to hide the item in the hamburger menu is missing in the commit:
https://hg-edge.mozilla.org/mozilla-central/rev/f4fae47c5010ae1c3a26c55faa9cafbfd406b90b#l11.108

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

:twisniewski, since you are the author of the regressor, bug 1952334, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 7

[Thomas Wisniewski [:twisniewski]]

Thank you, Piro. I'll re-add that code ASAP.

Comment 8

[Thomas Wisniewski [:twisniewski]]

Oh.. I had forgotten about this, but what we do when Report Broken Site is disabled right now is that we change the feature to instead send users to webcompat.com to submit their reports, rather than using the new reporter.

I will keep that for the case where the new reporter is preffed off, but will explore how to disable and hide it entirely when DisableFeedbackCommands is set.

Comment 9

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1963764 - make sure Report Broken Site is still hidden appropriately when enterprise DisableFeedbackCommands policy is active (regressed by bug 1952334); r?gijs

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1952334

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d148785a48cc
https://hg.mozilla.org/integration/autoland/rev/d3c2fae28af0
make sure Report Broken Site is still hidden appropriately when enterprise DisableFeedbackCommands policy is active (regressed by bug 1952334); r=Gijs

Comment 12

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/d3c2fae28af0

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:twisniewski, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox142 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Thomas Wisniewski [:twisniewski]]

Comment on attachment 9503074 [details]
Bug 1963764 - make sure Report Broken Site is still hidden appropriately when enterprise DisableFeedbackCommands policy is active (regressed by bug 1952334); r?gijs

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Users who have DisableFeedbackCommands enabled in their enterprise policy settings will erroneously still see Report Broken Site in the menus.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Set DisableFeedbackCommands in enterprise policies, and see if Report Broken Site appears in the help menu and app menu (it should not appear at all with that policy set).
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a regression, and the code's logic is the same as the working version used before, just checking for the DisableFeedbackCommands enterprise policy to be set, rather than a pref.
• String changes made/needed:
• Is Android affected?: No

Comment 15

[Dianna Smith [:diannaS]]

Comment on attachment 9503074 [details]
Bug 1963764 - make sure Report Broken Site is still hidden appropriately when enterprise DisableFeedbackCommands policy is active (regressed by bug 1952334); r?gijs

Approved for 142.0b6

Comment 16

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/363681e78ec1
https://hg.mozilla.org/releases/mozilla-beta/rev/5cbbb43692a3

Comment 17

[Thomas Wisniewski [:twisniewski]]

Comment on attachment 9503074 [details]
Bug 1963764 - make sure Report Broken Site is still hidden appropriately when enterprise DisableFeedbackCommands policy is active (regressed by bug 1952334); r?gijs

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Users who have DisableFeedbackCommands enabled in their enterprise policy settings will erroneously still see Report Broken Site in the menus.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Set DisableFeedbackCommands in enterprise policies, and see if Report Broken Site appears in the help menu and app menu (it should not appear at all with that policy set).
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a regression, and the code's logic is the same as the working version used before, just checking for the DisableFeedbackCommands enterprise policy to be set, rather than a pref.
• String changes made/needed:
• Is Android affected?: No

Comment 18

[Mike Kaply [:mkaply]]

Can we uplift this to ESR 140 as well?

Comment 19

[Thomas Wisniewski [:twisniewski]]

Sure, it ought to apply cleanly on ESR 140 as well.

Comment 20

[Thomas Wisniewski [:twisniewski]]

Comment on attachment 9503074 [details]
Bug 1963764 - make sure Report Broken Site is still hidden appropriately when enterprise DisableFeedbackCommands policy is active (regressed by bug 1952334); r?gijs

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Given that it affects enterprise policies, it seems appropriate to target ESR as well.
• User impact if declined: Users who have DisableFeedbackCommands enabled in their enterprise policy settings will erroneously still see Report Broken Site in the menus.
• Fix Landed on Version: 142 (uplifts to other branches also happening now)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch is very small and limited to only our report broken site feature.

Comment 21

[Dianna Smith [:diannaS]]

Comment on attachment 9503074 [details]
Bug 1963764 - make sure Report Broken Site is still hidden appropriately when enterprise DisableFeedbackCommands policy is active (regressed by bug 1952334); r?gijs

Approved for 140.2.0esr

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/cf0f1e09c21d

Comment 23

[Monica Chiorean]

Attachment: recording

Celenity, I set up the policy as indicated but the "Report Broken Site" option in not gray out in none of the menus as in the recording attached. Is this expected? Thank you.

Comment 24

[Monica Chiorean]

Mike, can you please confirm if attached recording is expected behavior? Thank you.

Comment 25

[Pascal Chevrel:pascalc]

Comment on attachment 9503074 [details]
Bug 1963764 - make sure Report Broken Site is still hidden appropriately when enterprise DisableFeedbackCommands policy is active (regressed by bug 1952334); r?gijs

QA wasn't able to verify that the fix actually works and we have shipped multiple releases with this regression.
Also, I think this is not an S2 ("Major functionality/product severely impaired or a high impact issue and a satisfactory workaround does not exist"), probably more an S3/S4 given that it involves non default configurations.

Comment 26

[Mike Kaply [:mkaply]]

Maybe I'm remembering wrong, but this fix was to bring back the fact that it was showing in the Browser menus.

We'll need a separate fix for the protections panel

Comment 27

[celenity [:celenity]]

(In reply to Monica Chiorean from comment #23)

Created attachment 9504455 [details]
recording

Celenity, I set up the policy as indicated but the "Report Broken Site" option in not gray out in none of the menus as in the recording attached. Is this expected? Thank you.

Apologies for the late response here, thanks for taking a look at this. To answer your question: No, I don't think that's expected. I think that once the DisableFeedbackCommands policy is set, the option in the protections panel should also be hidden or grayed out.

Ideally, I think that these Report Broken Site UI options should also be hidden or grayed out when extensions.webcompat-reporter.enabled and/or ui.new-webcompat-reporter.enabled is set to false. It's currently somewhat confusing, since there's now those 2 separate preferences to control this functionality, and neither of them seem to work properly at the moment.

Comment 28

[Monica Chiorean]

Reopen issue based on comment #27, please update if otherwise. Thank you.

Comment 29

[Thomas Wisniewski [:twisniewski]]

Ah, I see what's happening. I should have been more thorough before landing the patch here, sorry. It turns out that the policies disable the new reporter by pref, so my code to hide the reporter were being skipped outright. I'll make a new patch to fix that.

(In reply to celenity from comment #27)

Ideally, I think that these Report Broken Site UI options should also be hidden or grayed out when extensions.webcompat-reporter.enabled and/or ui.new-webcompat-reporter.enabled is set to false. It's currently somewhat confusing, since there's now those 2 separate preferences to control this functionality, and neither of them seem to work properly at the moment.

I'm fine with dropping one of the prefs (I would prefer the old one, since we do have others in the new- branch for controlling the behavior of the new reporter). Did you or others have a preference?

Comment 30

[Thomas Wisniewski [:twisniewski]]

Actually no, it seems that despite about:policies showing me that DisableFeedbackCommands is true (or false), Services.policies.isAllowed("DisableFeedbackCommands") is always returning true to my code. Weird. I'm not sure what's up yet.

Comment 31

[Thomas Wisniewski [:twisniewski]]

Oh my. I think I'm just being a bonehead, and the value I ought to be be checking is feedbackCommands, not DisableFeedbackCommands.

Comment 32

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1963764 - read the correct enterprise policy when deciding whether to hide Report Broken Site (also improve the related tests); r?gijs

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/adf2bb3c9288
https://hg.mozilla.org/integration/autoland/rev/f01d2c2f2791
read the correct enterprise policy when deciding whether to hide Report Broken Site (also improve the related tests); r=Gijs

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f198fbbe796e
https://hg.mozilla.org/integration/autoland/rev/3b91bc351cea
Revert "Bug 1963764 - read the correct enterprise policy when deciding whether to hide Report Broken Site (also improve the related tests); r=Gijs" for causing mochitest failures in browser_parent_menuitems.js

Comment 35

[Cristian Tuns]

Backed out for causing bc failures

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/components/reportbrokensite/test/browser/browser_parent_menuitems.js | isMenuItemHidden(AppMenu) when disabled by DisableFeedbackCommands enterprise policy - menu item is hidden -

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c5c06a5a32ee
https://hg.mozilla.org/integration/autoland/rev/e25921a70361
read the correct enterprise policy when deciding whether to hide Report Broken Site (also improve the related tests); r=Gijs

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c97b6d9b7938
https://hg.mozilla.org/integration/autoland/rev/d308f45d9571
Revert "Bug 1963764 - read the correct enterprise policy when deciding whether to hide Report Broken Site (also improve the related tests); r=Gijs" for causing mochitests failures in browser_parent_menuitems.js.

Comment 38

[Serban Stanca [:SerbanS]]

Reverted this because it was causing mochitests failures in browser_parent_menuitems.js.

• Revert link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/components/reportbrokensite/test/browser/browser_parent_menuitems.js | isMenuItemHidden(AppMenu) when disabled by DisableFeedbackCommands enterprise policy - menu item is hidden -

Comment 39

[Dianna Smith [:diannaS]]

:twisniewski Since RC week is next week, do we need to roll back https://phabricator.services.mozilla.com/D258785 and give this more time in nightly?

Comment 40

[Thomas Wisniewski [:twisniewski]]

Thankfully, what we've landed does no harm, so I don't think it's worth rolling it back (it effectively does nothing). I do still need to work on my patch, since I don't understand why it's intermittently failing on Linux yet, and I don't know if I'll have time to figure it out during this release cycle (but I'll try!)

Comment 41

[Dianna Smith [:diannaS]]

:twisniewski any update here on next steps?

Comment 42

[Thomas Wisniewski [:twisniewski]]

I (or someone else) needs to figure out what's causing my patch's tests to intermittently fail, and I don't have time to check into that right now.

One option is to ship the patch and live with it intermittently failing, but I suspect that we would not want to do that.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9503074 [details]
Bug 1963764 - make sure Report Broken Site is still hidden appropriately when enterprise DisableFeedbackCommands policy is active (regressed by bug 1952334); r?gijs

Clearing the approval flag to get this off the needs-uplift radar for ESR140.