Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No reload on changes to disabled filters in dashboard #41144

Merged
merged 11 commits into from
Jul 29, 2019
Merged

No reload on changes to disabled filters in dashboard #41144

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
141394c
dont update embeddables on changes in disabled filters
flash1293 Jul 15, 2019
a8729f7
dont update editor on changes in disabled filters
flash1293 Jul 15, 2019
74219df
filter out disabled filters in individual embeddables
flash1293 Jul 18, 2019
7d1d61c
simplify code
flash1293 Jul 18, 2019
6f914cf
Merge remote-tracking branch 'upstream/master' into fix/33926-disable…
flash1293 Jul 18, 2019
53b8f2d
check vis customizations for changes and check for changes in filter …
flash1293 Jul 19, 2019
f36ac79
Merge remote-tracking branch 'upstream/master' into fix/33926-disable…
flash1293 Jul 19, 2019
89f7bca
Merge remote-tracking branch 'upstream/master' into fix/33926-disable…
flash1293 Jul 22, 2019
d54ceed
Merge remote-tracking branch 'upstream/master' into fix/33926-disable…
flash1293 Jul 25, 2019
56aa53b
Merge remote-tracking branch 'upstream/master' into fix/33926-disable…
flash1293 Jul 29, 2019
8bf3094
use onlyDisabledFiltersChanged to avoid redundant code
flash1293 Jul 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions src/legacy/core_plugins/kibana/public/dashboard/dashboard_app_controller.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,23 @@ import { DashboardAppScope } from './dashboard_app';
import { VISUALIZE_EMBEDDABLE_TYPE } from '../visualize/embeddable';
import { convertSavedDashboardPanelToPanelState } from './lib/embeddable_saved_object_converters';

function areFilterChangesFetchRelevant(oldFilters: Filter[], newFilters: Filter[]) {
if (oldFilters.length !== newFilters.length) {
return true;
}

for (let i = 0; i < oldFilters.length; i++) {
const oldFilter = oldFilters[i];
const newFilter = newFilters[i];

if (oldFilter.meta.disabled !== newFilter.meta.disabled) {
return true;
}
}

return false;
}

export class DashboardAppController {
// Part of the exposed plugin API - do not remove without careful consideration.
appStatus: {
Expand Down Expand Up @@ -361,7 +378,15 @@ export class DashboardAppController {

const refreshDashboardContainer = () => {
const changes = getChangesFromAppStateForContainerState();
if (changes && dashboardContainer) {
if (
changes &&
dashboardContainer &&
!(
changes.filters &&
Object.keys(changes).length === 1 &&
!areFilterChangesFetchRelevant($scope.model.filters, changes.filters)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

) {
dashboardContainer.updateInput(changes);
}
};
Expand Down Expand Up @@ -675,9 +700,10 @@ export class DashboardAppController {
// update root source when filters update
const updateSubscription = queryFilter.getUpdates$().subscribe({
next: () => {
const oldFilters = $scope.model.filters;
$scope.model.filters = queryFilter.getFilters();
dashboardStateManager.applyFilters($scope.model.query, $scope.model.filters);
if (dashboardContainer) {
if (dashboardContainer && areFilterChangesFetchRelevant(oldFilters, $scope.model.filters)) {
dashboardContainer.updateInput({ filters: $scope.model.filters });
}
},
Expand Down
22 changes: 21 additions & 1 deletion src/legacy/core_plugins/kibana/public/visualize/editor/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,23 @@ uiModules
};
});

function areFilterChangesFetchRelevant(oldFilters, newFilters) {
if (oldFilters.length !== newFilters.length) {
return true;
}

for (let i = 0; i < oldFilters.length; i++) {
const oldFilter = oldFilters[i];
const newFilter = newFilters[i];

if (oldFilter.meta.disabled !== newFilter.meta.disabled) {
return true;
}
}

return false;
}

function VisEditor(
$scope,
$element,
Expand Down Expand Up @@ -418,8 +435,11 @@ function VisEditor(
// update the searchSource when filters update
const filterUpdateSubscription = subscribeWithScope($scope, queryFilter.getUpdates$(), {
next: () => {
const oldFilters = $scope.filters;
$scope.filters = queryFilter.getFilters();
$scope.fetch();
if (areFilterChangesFetchRelevant(oldFilters, $scope.filters)) {
$scope.fetch();
}
}
});

Expand Down