[Lens] Do not reset filter state on incoming app navigation #83786
Conversation
flash1293
added
the
Team:Visualizations
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
There was a problem hiding this comment.
This looks great to me - especially all the tests. (Code only review)
I do think that carrying over the filters from the dashboard is the right call - even if they aren't pinned - because it keeps context the same when moving between apps.
Edit: After further discussion with Joe, I noticed I was missing a major piece of the puzzle. The fact that any filters on lens get saved into the panel. This means that even with this PR, it would still be very easy for a user to accidentally add an unwanted filter to a panel, by editing from dashboard with a filter, then clicking save and return. In this case, I think we should try to actually clear the filters when lens starts.
In Visualize, when coming from dashboards, the filters are reset and only pinned filters / saved filters are kept. We should write a follow up to ensure that this behaviour is made consistent.
In my opinion it would be best to make Lens follow the visualize behaviour, but it would be worth getting input from product. @AlonaNadler, do you have any context around this?
|
Tested, code LGTM 🆗 |
|
I see your point @ThomThomson and I think I agree. @AlonaNadler can you confirm this is the right behavior? Changing the PR this way shouldn't be difficult |
|
I rewrote this PR to not carry over non-pinned filters from Dashboard as I think it's the right behavior as well and it's kind of urgent because I definitely wont to get it into the 7.10.1 release. |
|
Ideally, when creating a new Lens panel from the dashboard, the filters should not pass to Lens unless the filter was pinned. |
|
@wylieconlon That's not the exactly same thing - we have too keep the state of the |
|
@elasticmachine merge upstream |
|
@mbondyra @ThomThomson could you have another look just to be sure? As the fix is a very different one now. It has to be done in that place as well because line 145 Wylie pointed out is just removing the app filters from the filter manager state of the data plugin, not from the Lens local state. |
| filters: data.query.filterManager.getFilters(), | ||
| filters: !initialContext | ||
| ? data.query.filterManager.getGlobalFilters() | ||
| : data.query.filterManager.getFilters(), |
There was a problem hiding this comment.
Stylistic comment: could you flip the branches of this ternary? This might simplify the understanding of the code.
Also since we've had issues with this part of the code, maybe it deserves a code comment here?
| @@ -116,6 +121,18 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { | |||
| ); | |||
| const hasGeoDestFilter = await filterBar.hasFilter('geo.dest', 'LS'); | |||
| expect(hasGeoDestFilter).to.be(true); | |||
| await filterBar.addFilter('geo.src', 'is', 'US'); | |||
| await filterBar.toggleFilterPinned('geo.src'); | |||
There was a problem hiding this comment.
In terms of code style, I'm not a big fan of how the next text depends on test setup from this test. Could you move this logic into a single test?
|
Good points @wylieconlon , adjusted the PR |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Fixes #83601
When switching from dashboard to Lens using the "Create new" button, unpinned filters would get carried over, but not shown in the UI.
This happened because the Lens app always resets the filter state in the data plugin on load. I added an exception to that rule to not reset the state if there's an
originatingAppin the incoming package.