1409836 - 'reset filters' doesn't persist filter prefs

Status: VERIFIED FIXED

(DevTools :: Console, defect, P1)

Description

[Zach Psyche]

Attachment: 801c05f3bdb0884607f9747c73cc5ce0.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171016185129

Steps to reproduce:

- Right click
- Click on "Inspect Element"
- Click on the console tab


Actual results:

Console only shows errors and warnings, so I need to reset the filters to see everything.


Expected results:

I should be able to see everything in console without needing to reset the filters every time I open console.

Comment 1

[Zach Psyche]

Tried restarting in safe mode, issue still exists.

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Zach from comment #1)
> Tried restarting in safe mode, issue still exists.

Do you see this issue on any page?  Like `data:text/html,<script>console.log("foo")</script>`?  If so, then when you reset the filters and close/reopen the console do they come back again?

Comment 3

[Brian Grinstead [:bgrins]]

Also, if log messages are being filtered out can you check the value of the "devtools.webconsole.filter.log" pref?

Comment 4

[Zach Psyche]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> (In reply to Zach from comment #1)
> > Tried restarting in safe mode, issue still exists.
> 
> Do you see this issue on any page?  Like
> `data:text/html,<script>console.log("foo")</script>`?  If so, then when you
> reset the filters and close/reopen the console do they come back again?

It happens on any page and it goes back to only filtering errors and warnings when I close/reopen console.

Comment 5

[Zach Psyche]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Also, if log messages are being filtered out can you check the value of the
> "devtools.webconsole.filter.log" pref?
its set to false

Comment 6

[Brian Grinstead [:bgrins]]

OK thanks for reporting this - I'm seeing it as well. Nicolas, can you confirm these STR:

* Open data:text/html,<script>console.log("foo")</script>
* Open console, disable 'logs' filter
* Click 'reset filters' (message shows back up and logs get enabled again)
* Close and reopen console
* Log filter is still disabled

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Zach from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > (In reply to Zach from comment #1)
> > > Tried restarting in safe mode, issue still exists.
> > 
> > Do you see this issue on any page?  Like
> > `data:text/html,<script>console.log("foo")</script>`?  If so, then when you
> > reset the filters and close/reopen the console do they come back again?
> 
> It happens on any page and it goes back to only filtering errors and
> warnings when I close/reopen console.

A workaround in the meantime instead of doing reset is to open the filter menu by clicking on the funnel icon to the left of the filter textbox, and then click on 'logs' and whatever other filters that are off to toggle them on permanently.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

I confirm the bug.
I suspect something not working as it should in http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/devtools/client/webconsole/new-console-output/actions/filters.js#69-73

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

We were getting the filter state after dispatching the action, which made
all the filters to have an enabled state. Getting the state before dispatching
fixes the issue.
This patch enhance the Service mock in order to have a better idea of what is going on
with prefs. This allow us to introduce some tests to make sure prefs are updated in
reaction to given actions.

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

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

The fix was an easy one, and I added some tests to make sure we handle prefs as expected.
Since it's a small change, I hope we'll be able to uplift it to beta.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8920106 [details]
Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

https://reviewboard.mozilla.org/r/191096/#review196382

::: devtools/client/webconsole/new-console-output/actions/filters.js:47
(Diff revision 1)
>    return (dispatch, getState) => {
>      dispatch({
>        type: FILTERS_CLEAR,
>      });
>  
> -    const filterState = getAllFilters(getState());
> +    const filterState = getAllFilters(getState()).toJS();

Is this change required? It looks like the fix is below in defaultFiltersReset - I'm not sure what this is doing

::: devtools/client/webconsole/new-console-output/test/store/filters.test.js:230
(Diff revision 1)
> +      "css": true,
>        "net": true,
>        "netxhr": true,
>        "text": "foobar",
>      });
> +    let prefs = ServicesMock.prefs.testHelpers.getAllPrefs();

Nit: could you rewrite these assertion and the ones  below to be analagous to the one above, like:

expect(prefs).toEqual({
  [PREFS.FILTER.WARN]: true,
  [PREFS.FILTER.LOG]: true,
  ...
});

May be easier to compare the two states / copy for new tests that way.

::: devtools/client/webconsole/new-console-output/test/store/filters.test.js:269
(Diff revision 1)
>    });
>  });
>  
>  describe("Resets filters", () => {
>    it("resets default filters value to their original one.", () => {
> +    ServicesMock.prefs.testHelpers.clearPrefs();

Nit: would it be possible to add something like this at the top of the file (i.e. outisde of any `describe`).  And if so, would it make sense to do so (i.e. do we not rely on persisted pref state between individual tests)?

  afterEach(() => {
    ServicesMock.prefs.testHelpers.clearPrefs();
  });

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8920106 [details]
Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

https://reviewboard.mozilla.org/r/191096/#review196382

> Is this change required? It looks like the fix is below in defaultFiltersReset - I'm not sure what this is doing

So this is needed for the `clearFilters` actions. This goes through all filters and clear them.
The action is only used in tests to tidy up things between tests.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8920106 [details]
Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

https://reviewboard.mozilla.org/r/191096/#review196382

> Nit: could you rewrite these assertion and the ones  below to be analagous to the one above, like:
> 
> expect(prefs).toEqual({
>   [PREFS.FILTER.WARN]: true,
>   [PREFS.FILTER.LOG]: true,
>   ...
> });
> 
> May be easier to compare the two states / copy for new tests that way.

yeah, could be. I initially did not do it like that because prefs hold the persist and filter bar visibility as well. But we can have another helper in ServicesMock to only return filter prefs.

> Nit: would it be possible to add something like this at the top of the file (i.e. outisde of any `describe`).  And if so, would it make sense to do so (i.e. do we not rely on persisted pref state between individual tests)?
> 
>   afterEach(() => {
>     ServicesMock.prefs.testHelpers.clearPrefs();
>   });

I need to check. Ideally each test should have a clean slate, so I'm willing to do this.

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
> > Nit: could you rewrite these assertion and the ones  below to be analagous to the one above, like:
> > 
> > expect(prefs).toEqual({
> >   [PREFS.FILTER.WARN]: true,
> >   [PREFS.FILTER.LOG]: true,
> >   ...
> > });
> > 
> > May be easier to compare the two states / copy for new tests that way.
> 
> yeah, could be. I initially did not do it like that because prefs hold the
> persist and filter bar visibility as well. But we can have another helper in
> ServicesMock to only return filter prefs.

Oh, don't worry about it then

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Nit: would it be possible to add something like this at the top of the file
> (i.e. outisde of any `describe`).  And if so, would it make sense to do so
> (i.e. do we not rely on persisted pref state between individual tests)?
> 
>   afterEach(() => {
>     ServicesMock.prefs.testHelpers.clearPrefs();
>   });

This is already handled by the beforeEach function at the top of the file, in which we call filtersClear (which also resets the prefs). When I added this line, filtersClear was broken, but now that it's fixed, we don't need it anymore, thus I'll remove it.

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8920106 [details]
Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8baf55e85dae
Fix setting filters prefs when filters are reset or cleared; r=bgrins.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8baf55e85dae

Comment 19

[Julien Cristau [:jcristau]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)
> The fix was an easy one, and I added some tests to make sure we handle prefs
> as expected.
> Since it's a small change, I hope we'll be able to uplift it to beta.

Please request uplift ASAP if you still want this in 57.

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8920106 [details]
Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

Approval Request Comment
[Feature/Bug causing the regression]: Always been there in new console frontend (enabled in 57)
[User impact if declined]: Filters state in the console toolbar are not persisted across devtools restart. This would confuse people to see a filter is off when they toggle it before. It can be even more confusing since the toolbar is hidden by default.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes
1. Open the console
2. Click on the filter icon in the toolbar
3. The filter toolbar should be shown
4. Toggle off the "warning" filter
5. evaluate `console.warning("foo")`
6. You should see a "Reset filters" button in the top-right corner
7. Click it
8. You should now see the warning message
9. Close the devtools
10. Open the console again
11. You should still see the warning message (i.e. the warning filter should be on)
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a really small change (only the order of 2 instructions where changed), and it is well tested.

[String changes made/needed]: -


---

Thanks for the reminder jcristau !

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8920106 [details]
Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

Recent problem in 57, low risk fix, Beta57+

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4b4ff6b8f353

Comment 23

[Oana Botisan, Desktop QA]

I managed to reproduce the bug using the steps from comment 20 on an older version of Nightly (2017-10-18) on Windows 10x64 and macOS 10.13.
I retested everything using latest Nightly 58 and beta 57.0b12 on Windows 10, macOS 10.13 and Ubuntu 16.04x64, but the bug is not reproducing anymore. After the console is reopened, the log filters are still enabled (you can still see the warning message).