No reload on changes to disabled filters in dashboard #41144
Conversation
💚 Build Succeeded |
| changes.filters && | ||
| Object.keys(changes).length === 1 && | ||
| !areFilterChangesFetchRelevant($scope.model.filters, changes.filters) | ||
| ) |
There was a problem hiding this comment.
nit: Might just be me, but I re-read this conditional 3 times before I understood it 😄 Probably due to the double-negation. Maybe there's a way to simplify?
There was a problem hiding this comment.
That's exactly what I meant by
Input greatly appreciated before I clean up and add tests.
in the PR description 😄
Just wanted to make sure the approach is sound. I will ping again once this leaves draft status.
There was a problem hiding this comment.
I think having this logic here will get us into trouble. I didn't test but I feel like things can get out of sync now. Like, lets say you change a disabled filter from abc to xyz. You then add a new panel into the container. This change comes from the container, so this code gets run:
inputSubscription = dashboardContainer.getInput$().subscribe(async () => {
let dirty = false;
// This has to be first because handleDashboardContainerChanges causes
// appState.save which will cause refreshDashboardContainer to be called.
// Add filters modifies the object passed to it, hence the clone deep.
if (!_.isEqual(container.getInput().filters, queryFilter.getFilters())) {
await queryFilter.addFilters(_.cloneDeep(container.getInput().filters));
dashboardStateManager.applyFilters($scope.model.query, container.getInput().filters);
dirty = true;
}
At this point, I think we might have a bug because it's going to look like the container updated the disabled filter back to abc and the xyz changes will get overwritten.
I think maybe the safer bet is to just let every embeddable handle when to fetch or not. We could have a bigger discussion on whether it even makes sense to send disabled filters down to the embeddables. The logic in this class is difficult to follow with state updates coming from so many different places but I think it will get much clearer once we get rid of angular and can clean this up.
There was a problem hiding this comment.
You are right, that's exactly what happens. Is this code block handling the case when a filter is added from within the dashboard (e.g. clicking a bar in a bar chart)?
Not sending disabled filters down to the embeddables is an interesting thought, but it might be useful to have this information there in the future (another thing which isn't solved right in my code). Handling this for each embeddable individually is probably the best solution, I will adjust my PR.
There was a problem hiding this comment.
this code block handling the case when a filter is added from within the dashboard (e.g. clicking a bar in a bar chart)?
Yes, at least for saved searches. For visualizations I think it actually goes through the angular service and by-passes the container but it shouldn't and we need to fix it to get drilldowns to work properly. Also handles any other changes coming from a container, e.g. any actions that modify any container or embeddable state, etc.
Handling this for each embeddable individually is probably the best solution, I will adjust my PR.
sounds good!
flash1293
added
bug
|
Pinging @elastic/kibana-app |
💚 Build Succeeded |
There was a problem hiding this comment.
That's exactly what I meant by
Input greatly appreciated before I clean up and add tests.
in the PR description 😄
Ahh my apologies @flash1293! I totally missed that line 😬
Saw that you switched it from draft status, so I pulled it down locally to test & I'm having a hard time verifying the changes:
- In visualize, when I disable a filter it actually removes it completely in the UI
- It appears to still correctly update the global state (e.g. if I pin a filter in visualize, then disable it, the vis displays the correct value, but the pill disappears. However, pill is visible when i go over to dashboard)
- In dashboard, when I disable a filter and then edit or invert, I still see the
_msearchrequests going out.
|
@lukeelmers It seems like I “optimized” too much here, sorry. I’m going to take care of it. I can’t put the PR back into draft status but I will ping you once this is working as intended. |
flash1293
changed the title
…context for search embeddable
💔 Build Failed |
💚 Build Succeeded |
flash1293
changed the title
|
I tested around a bit and I can't find a case it's not working anymore. Ping @lukeelmers @nreese |
💚 Build Succeeded |
There was a problem hiding this comment.
LGTM
Tested and confirmed that in dashboard, disabling a filter and inverting it (include/exclude) doesn’t create additional requests. Visualize behavior remains unchanged since @lukasolson said he would incorporate it into #41204 instead.
flash1293
changed the title
💚 Build Succeeded |
There was a problem hiding this comment.
I would suggest that you use the onlyDisabled function from src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_disabled.js (export it on the top level of the plugin to use it)
This function is (a) fully covered by tests and (b) also used by filter manager to detect similar changes.
If you don't like the lodash-y implementaiton, feel free to change it, I would just like us to use the same logic.
|
Thanks @lizozom I'll update my implementation |
|
Filter manager code LGTM |
💔 Build Failed |
|
Jenkins, test this. |
💚 Build Succeeded |
Summary
Related #33926
This PR prevents requests to the backend if disabled filters are changed. It updates the app state but prevents the call to search source by not calling
forceReloadon the vis object in case of the editor resp. not propagating the input changes to the dashboard container in case of dashboard.I'm not sure whether this is the right place for that, and maybe this even becomes a non-issue when we have generic caching on the data layer (not sure whether this is planned).
Ping
@stacey-gammon (because of embeddable and dashboard knowledge)
@lukeelmers (because of data access layer knowledge)
@timroes (because of visualize editor knowledge)
Input greatly appreciated before I clean up and add tests.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] This was checked for cross-browser compatibility, including a check against IE11- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers