Skip to content

Commit

Permalink
[Controls] Do not ignore invalid selections (elastic#174201)
Browse files Browse the repository at this point in the history
Fixes elastic#164633 

## Summary

Improves dashboard performance by not ignoring invalid Controls
selections.

This improves Dashboard performance as panels do not wait until Controls
selections are validated. Prior to this, the Controls would run
validations to find selections that would result in No Data. These
"invalid selections" would be ignored and not applied to the filters.

With this PR, all selections whether valid or invalid are applied to the
filters. This means all controls and panels can be loaded immediately
which improves the performance of the Dashboard.

Since this can be considered a breaking UI change, I've added a
suppressible toast warning users about the change.

The screenshots below show the same dashboard with invalid selections.
In the "Before" screenshot, the "Agent version" control selection is
validated before the rest of the panels are loaded. Once validated, the
invalid selection is ignored and the panels load based only on valid
selections.

In the "After" screenshot the "Agent version" control selection is
immediately applied to the filters and the rest of the dashboard is
loaded. The control selections are checked for validity only after the
filters are applied and highlighted. The warning popover notifies the
user that invalid selections are no longer ignored. Clicking the "Do not
show again" button updates the browser's localStorage so that future
warnings are suppressed.

Before:

![localhost_5601_wgf_app_home](https://github.com/elastic/kibana/assets/1638483/56f37376-7685-4225-b49a-65aa21f90f14)

 After: 
![localhost_5701_vbw_app_dashboards
(2)](https://github.com/elastic/kibana/assets/1638483/d0000b7e-8591-40ab-9302-6d1d5387b073)




@amyjtechwriter Can you please review the text in the warnings? We want
to make users aware of the change as it could abruptly change the data
in their dashboards.


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Hannah Mudge <hannah.wright@elastic.co>
Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>
  • Loading branch information
3 people authored and semd committed Mar 4, 2024
1 parent 1731a9c commit cefbec6
Show file tree
Hide file tree
Showing 34 changed files with 779 additions and 376 deletions.
3 changes: 2 additions & 1 deletion src/plugins/controls/kibana.jsonc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
"unifiedSearch",
"uiActions"
],
"extraPublicDirs": ["common"]
"extraPublicDirs": ["common"],
"requiredBundles": ["kibanaUtils"]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,34 +8,44 @@

import '../control_group.scss';

import {
arrayMove,
SortableContext,
rectSortingStrategy,
sortableKeyboardCoordinates,
} from '@dnd-kit/sortable';
import classNames from 'classnames';
import React, { useEffect, useMemo, useState } from 'react';
import { TypedUseSelectorHook, useSelector } from 'react-redux';

import {
closestCenter,
DndContext,
DragEndEvent,
DragOverlay,
KeyboardSensor,
LayoutMeasuringStrategy,
PointerSensor,
useSensor,
useSensors,
LayoutMeasuringStrategy,
} from '@dnd-kit/core';
import classNames from 'classnames';
import React, { useMemo, useState } from 'react';
import { TypedUseSelectorHook, useSelector } from 'react-redux';
import { EuiButtonIcon, EuiFlexGroup, EuiFlexItem, EuiPanel } from '@elastic/eui';

import {
arrayMove,
rectSortingStrategy,
SortableContext,
sortableKeyboardCoordinates,
} from '@dnd-kit/sortable';
import {
EuiButtonEmpty,
EuiButtonIcon,
EuiCheckbox,
EuiFlexGroup,
EuiFlexItem,
EuiIcon,
EuiPanel,
EuiText,
EuiTourStep,
} from '@elastic/eui';
import { ViewMode } from '@kbn/embeddable-plugin/public';

import { ControlGroupReduxState } from '../types';
import { ControlGroupStrings } from '../control_group_strings';
import { ControlClone, SortableControl } from './control_group_sortable_item';
import { useControlGroupContainer } from '../embeddable/control_group_container';
import { ControlGroupReduxState } from '../types';
import { ControlClone, SortableControl } from './control_group_sortable_item';

const contextSelect = useSelector as TypedUseSelectorHook<ControlGroupReduxState>;

Expand All @@ -47,6 +57,12 @@ export const ControlGroup = () => {
const viewMode = contextSelect((state) => state.explicitInput.viewMode);
const controlStyle = contextSelect((state) => state.explicitInput.controlStyle);
const showAddButton = contextSelect((state) => state.componentState.showAddButton);
const controlWithInvalidSelectionsId = contextSelect(
(state) => state.componentState.controlWithInvalidSelectionsId
);
const [tourStepOpen, setTourStepOpen] = useState<boolean>(true);
const [suppressTourChecked, setSuppressTourChecked] = useState<boolean>(false);
const [renderTourStep, setRenderTourStep] = useState(false);

const isEditable = viewMode === ViewMode.EDIT;

Expand All @@ -61,6 +77,87 @@ export const ControlGroup = () => {
[panels]
);

useEffect(() => {
/**
* This forces the tour step to get unmounted so that it can attach to the new invalid
* control - otherwise, the anchor will remain attached to the old invalid control
*/
setRenderTourStep(false);
setTimeout(() => setRenderTourStep(true), 100);
}, [controlWithInvalidSelectionsId]);

const tourStep = useMemo(() => {
if (
!renderTourStep ||
!controlGroup.canShowInvalidSelectionsWarning() ||
!tourStepOpen ||
!controlWithInvalidSelectionsId
) {
return null;
}
const invalidControlType = panels[controlWithInvalidSelectionsId].type;

return (
<EuiTourStep
step={1}
stepsTotal={1}
minWidth={300}
maxWidth={300}
display="block"
isStepOpen={true}
repositionOnScroll
onFinish={() => {}}
panelPaddingSize="m"
anchorPosition="downCenter"
panelClassName="controlGroup--invalidSelectionsTour"
anchor={`#controlFrame--${controlWithInvalidSelectionsId}`}
title={
<EuiFlexGroup gutterSize="s" alignItems="center">
<EuiFlexItem grow={false}>
<EuiIcon type="warning" color="warning" />
</EuiFlexItem>
<EuiFlexItem>{ControlGroupStrings.invalidControlWarning.getTourTitle()}</EuiFlexItem>
</EuiFlexGroup>
}
content={ControlGroupStrings.invalidControlWarning.getTourContent(invalidControlType)}
footerAction={[
<EuiCheckbox
compressed
checked={suppressTourChecked}
id={'controlGroup--suppressTourCheckbox'}
className="controlGroup--suppressTourCheckbox"
onChange={(e) => setSuppressTourChecked(e.target.checked)}
label={
<EuiText size="xs" className="controlGroup--suppressTourCheckboxLabel">
{ControlGroupStrings.invalidControlWarning.getSuppressTourLabel()}
</EuiText>
}
/>,
<EuiButtonEmpty
size="xs"
flush="right"
color="text"
onClick={() => {
setTourStepOpen(false);
if (suppressTourChecked) {
controlGroup.suppressInvalidSelectionsWarning();
}
}}
>
{ControlGroupStrings.invalidControlWarning.getDismissButton()}
</EuiButtonEmpty>,
]}
/>
);
}, [
panels,
controlGroup,
tourStepOpen,
renderTourStep,
suppressTourChecked,
controlWithInvalidSelectionsId,
]);

const [draggingId, setDraggingId] = useState<string | null>(null);
const draggingIndex = useMemo(
() => (draggingId ? idsInOrder.indexOf(draggingId) : -1),
Expand Down Expand Up @@ -117,6 +214,7 @@ export const ControlGroup = () => {
alignItems="center"
data-test-subj="controls-group"
>
{tourStep}
<EuiFlexItem>
<DndContext
onDragStart={({ active }) => setDraggingId(active.id)}
Expand Down
9 changes: 9 additions & 0 deletions src/plugins/controls/public/control_group/control_group.scss
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,12 @@ $controlMinWidth: $euiSize * 14;
top: (-$euiSizeXS) !important;
}
}

.controlGroup--invalidSelectionsTour {
.controlGroup--suppressTourCheckbox {
height: 22px;
&Label {
font-weight: $euiFontWeightMedium;
}
}
}
39 changes: 37 additions & 2 deletions src/plugins/controls/public/control_group/control_group_strings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,42 @@ import { i18n } from '@kbn/i18n';
import { RANGE_SLIDER_CONTROL } from '../range_slider';

export const ControlGroupStrings = {
invalidControlWarning: {
getTourTitle: () =>
i18n.translate('controls.controlGroup.invalidControlWarning.tourStepTitle.default', {
defaultMessage: 'Invalid selections are no longer ignored',
}),
getTourContent: (controlType: string) => {
switch (controlType) {
case RANGE_SLIDER_CONTROL: {
return i18n.translate(
'controls.controlGroup.invalidControlWarning.tourStepContent.rangeSlider',
{
defaultMessage: 'The selected range is returning no results. Try changing the range.',
}
);
}
default: {
return i18n.translate(
'controls.controlGroup.invalidControlWarning.tourStepContent.default',
{
defaultMessage:
'Some selections are returning no results. Try changing the selections.',
}
);
}
}
},

getDismissButton: () =>
i18n.translate('controls.controlGroup.invalidControlWarning.dismissButtonLabel', {
defaultMessage: 'Dismiss',
}),
getSuppressTourLabel: () =>
i18n.translate('controls.controlGroup.invalidControlWarning.suppressTourLabel', {
defaultMessage: "Don't show again",
}),
},
manageControl: {
getFlyoutCreateTitle: () =>
i18n.translate('controls.controlGroup.manageControl.createFlyoutTitle', {
Expand Down Expand Up @@ -258,8 +294,7 @@ export const ControlGroupStrings = {
}),
getValidateSelectionsSubTitle: () =>
i18n.translate('controls.controlGroup.management.validate.subtitle', {
defaultMessage:
'Automatically ignore any control selection that would result in no data.',
defaultMessage: 'Highlight control selections that result in no data.',
}),
},
controlChaining: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { compareFilters, COMPARE_ALL_OPTIONS, Filter, uniqFilters } from '@kbn/es-query';
import { isEqual, pick } from 'lodash';
import React, { createContext, useContext } from 'react';
Expand All @@ -24,6 +25,7 @@ import {
persistableControlGroupInputKeys,
} from '../../../common';
import { pluginServices } from '../../services';
import { ControlsStorageService } from '../../services/storage/types';
import { ControlEmbeddable, ControlInput, ControlOutput } from '../../types';
import { ControlGroup } from '../component/control_group_component';
import { openAddDataControlFlyout } from '../editor/open_add_data_control_flyout';
Expand Down Expand Up @@ -86,11 +88,15 @@ export class ControlGroupContainer extends Container<

private initialized$ = new BehaviorSubject(false);

private storageService: ControlsStorageService;

private subscriptions: Subscription = new Subscription();
private domNode?: HTMLElement;
private recalculateFilters$: Subject<null>;
private relevantDataViewId?: string;
private lastUsedDataViewId?: string;
private invalidSelectionsState: { [childId: string]: boolean };

public diffingSubscription: Subscription = new Subscription();

// state management
Expand Down Expand Up @@ -126,6 +132,8 @@ export class ControlGroupContainer extends Container<
ControlGroupChainingSystems[initialInput.chainingSystem]?.getContainerSettings(initialInput)
);

({ storage: this.storageService } = pluginServices.getServices());

this.recalculateFilters$ = new Subject();
this.onFiltersPublished$ = new Subject<Filter[]>();
this.onControlRemoved$ = new Subject<string>();
Expand Down Expand Up @@ -153,6 +161,10 @@ export class ControlGroupContainer extends Container<

this.store = reduxEmbeddableTools.store;

this.invalidSelectionsState = this.getChildIds().reduce((prev, id) => {
return { ...prev, [id]: false };
}, {});

// when all children are ready setup subscriptions
this.untilAllChildrenReady().then(() => {
this.recalculateDataViews();
Expand All @@ -164,6 +176,32 @@ export class ControlGroupContainer extends Container<
this.fieldFilterPredicate = fieldFilterPredicate;
}

public canShowInvalidSelectionsWarning = () =>
this.storageService.getShowInvalidSelectionWarning() ?? true;

public suppressInvalidSelectionsWarning = () => {
this.storageService.setShowInvalidSelectionWarning(false);
};

public reportInvalidSelections = ({
id,
hasInvalidSelections,
}: {
id: string;
hasInvalidSelections: boolean;
}) => {
this.invalidSelectionsState = { ...this.invalidSelectionsState, [id]: hasInvalidSelections };

const childrenWithInvalidSelections = cachedChildEmbeddableOrder(
this.getInput().panels
).idsInOrder.filter((childId) => {
return this.invalidSelectionsState[childId];
});
this.dispatch.setControlWithInvalidSelectionsId(
childrenWithInvalidSelections.length > 0 ? childrenWithInvalidSelections[0] : undefined
);
};

private setupSubscriptions = () => {
/**
* refresh control order cache and make all panels refreshInputFromParent whenever panel orders change
Expand Down Expand Up @@ -201,7 +239,9 @@ export class ControlGroupContainer extends Container<
* debounce output recalculation
*/
this.subscriptions.add(
this.recalculateFilters$.pipe(debounceTime(10)).subscribe(() => this.recalculateFilters())
this.recalculateFilters$.pipe(debounceTime(10)).subscribe(() => {
this.recalculateFilters();
})
);
};

Expand All @@ -211,9 +251,14 @@ export class ControlGroupContainer extends Container<
} = this.getState();
if (!persistableControlGroupInputIsEqual(this.getPersistableInput(), lastSavedInput)) {
this.updateInput(lastSavedInput);
this.reload(); // this forces the children to update their inputs + perform validation as necessary
}
}

public reload() {
super.reload();
}

public getPersistableInput: () => PersistableControlGroupInput & { id: string } = () => {
const input = this.getInput();
return pick(input, [...persistableControlGroupInputKeys, 'id']);
Expand Down Expand Up @@ -284,13 +329,14 @@ export class ControlGroupContainer extends Container<
private recalculateFilters = () => {
const allFilters: Filter[] = [];
let timeslice;
Object.values(this.children).map((child) => {
Object.values(this.children).map((child: ControlEmbeddable) => {
const childOutput = child.getOutput() as ControlOutput;
allFilters.push(...(childOutput?.filters ?? []));
if (childOutput.timeslice) {
timeslice = childOutput.timeslice;
}
});

// if filters are different, publish them
if (
!compareFilters(this.output.filters ?? [], allFilters ?? [], COMPARE_ALL_OPTIONS) ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ export const controlGroupReducers = {
) => {
state.componentState.lastSavedInput = action.payload;
},
setControlWithInvalidSelectionsId: (
state: WritableDraft<ControlGroupReduxState>,
action: PayloadAction<ControlGroupComponentState['controlWithInvalidSelectionsId']>
) => {
state.componentState.controlWithInvalidSelectionsId = action.payload;
},
setControlStyle: (
state: WritableDraft<ControlGroupReduxState>,
action: PayloadAction<ControlGroupInput['controlStyle']>
Expand Down
1 change: 1 addition & 0 deletions src/plugins/controls/public/control_group/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export interface ControlGroupSettings {

export type ControlGroupComponentState = ControlGroupSettings & {
lastSavedInput: PersistableControlGroupInput;
controlWithInvalidSelectionsId?: string;
};

export {
Expand Down
Loading

0 comments on commit cefbec6

Please sign in to comment.