[Controls] Clear range/time slider selections when field changes (#12…

…9824) (#130827) * Reset selections on save of existing control * Allow reset to force render for range/time sliders * Reset selections only when field name or data view changes * Make generic DataControlInput interface * Fix infinite useEffect + imports + types * Simpler solution without resetSelections() * Add functional tests * Apply Devon's changes (cherry picked from commit 5a86421) # Conflicts: # src/plugins/controls/common/control_types/options_list/types.ts
elastic · Apr 21, 2022 · 0cd869c · 0cd869c
1 parent ee81956
commit 0cd869c
Show file tree

Hide file tree

Showing 12 changed files with 98 additions and 66 deletions.
diff --git a/src/plugins/controls/common/control_types/options_list/types.ts b/src/plugins/controls/common/control_types/options_list/types.ts
@@ -8,14 +8,11 @@
 
 import { BoolQuery } from '@kbn/es-query';
 import { FieldSpec } from '../../../../data_views/common';
-import { ControlInput } from '../../types';
+import { DataControlInput } from '../../types';
 
 export const OPTIONS_LIST_CONTROL = 'optionsListControl';
 
-export interface OptionsListEmbeddableInput extends ControlInput {
-  fieldName: string;
-  dataViewId: string;
-
+export interface OptionsListEmbeddableInput extends DataControlInput {
   selectedOptions?: string[];
   singleSelect?: boolean;
   loading?: boolean;

diff --git a/src/plugins/controls/common/control_types/range_slider/types.ts b/src/plugins/controls/common/control_types/range_slider/types.ts
@@ -6,14 +6,12 @@
  * Side Public License, v 1.
  */
 
-import { ControlInput } from '../../types';
+import { DataControlInput } from '../../types';
 
 export const RANGE_SLIDER_CONTROL = 'rangeSliderControl';
 
 export type RangeValue = [string, string];
 
-export interface RangeSliderEmbeddableInput extends ControlInput {
-  fieldName: string;
-  dataViewId: string;
+export interface RangeSliderEmbeddableInput extends DataControlInput {
   value: RangeValue;
 }
diff --git a/src/plugins/controls/common/control_types/time_slider/types.ts b/src/plugins/controls/common/control_types/time_slider/types.ts
@@ -6,12 +6,10 @@
  * Side Public License, v 1.
  */
 
-import { ControlInput } from '../../types';
+import { DataControlInput } from '../../types';
 
 export const TIME_SLIDER_CONTROL = 'timeSlider';
 
-export interface TimeSliderControlEmbeddableInput extends ControlInput {
-  fieldName: string;
-  dataViewId: string;
+export interface TimeSliderControlEmbeddableInput extends DataControlInput {
   value?: [number | null, number | null];
 }
diff --git a/src/plugins/controls/common/types.ts b/src/plugins/controls/common/types.ts
@@ -27,3 +27,8 @@ export type ControlInput = EmbeddableInput & {
   controlStyle?: ControlStyle;
   ignoreParentSettings?: ParentIgnoreSettings;
 };
+
+export type DataControlInput = ControlInput & {
+  fieldName: string;
+  dataViewId: string;
+};
diff --git a/src/plugins/controls/public/control_types/range_slider/range_slider.component.tsx b/src/plugins/controls/public/control_types/range_slider/range_slider.component.tsx
@@ -6,7 +6,7 @@
  * Side Public License, v 1.
  */
 
-import React, { FC, useCallback, useState } from 'react';
+import React, { FC, useCallback } from 'react';
 import { BehaviorSubject } from 'rxjs';
 
 import { DataViewField } from '../../../../data_views/public';
@@ -45,16 +45,13 @@ export const RangeSliderComponent: FC<Props> = ({ componentStateSubject }) => {
     componentStateSubject.getValue()
   );
 
-  const { value = ['', ''], id, title } = useEmbeddableSelector((state) => state);
-
-  const [selectedValue, setSelectedValue] = useState<RangeValue>(value || ['', '']);
+  const { value, id, title } = useEmbeddableSelector((state) => state);
 
   const onChangeComplete = useCallback(
     (range: RangeValue) => {
       dispatch(selectRange(range));
-      setSelectedValue(range);
     },
-    [selectRange, setSelectedValue, dispatch]
+    [selectRange, dispatch]
   );
 
   return (
@@ -64,7 +61,7 @@ export const RangeSliderComponent: FC<Props> = ({ componentStateSubject }) => {
       min={min}
       max={max}
       title={title}
-      value={selectedValue}
+      value={value ?? ['', '']}
       onChange={onChangeComplete}
       fieldFormatter={fieldFormatter}
     />

diff --git a/src/plugins/controls/public/control_types/range_slider/range_slider_embeddable.tsx b/src/plugins/controls/public/control_types/range_slider/range_slider_embeddable.tsx
@@ -306,7 +306,7 @@ export class RangeSliderEmbeddable extends Embeddable<RangeSliderEmbeddableInput
     this.updateOutput({ filters: [rangeFilter], dataViews: [dataView], loading: false });
   };
 
-  reload = () => {
+  public reload = () => {
     this.fetchMinMax();
   };
 

diff --git a/src/plugins/controls/public/control_types/range_slider/range_slider_embeddable_factory.tsx b/src/plugins/controls/public/control_types/range_slider/range_slider_embeddable_factory.tsx
@@ -37,8 +37,8 @@ export class RangeSliderEmbeddableFactory
   ) => {
     if (
       embeddable &&
-      (!deepEqual(newInput.fieldName, embeddable.getInput().fieldName) ||
-        !deepEqual(newInput.dataViewId, embeddable.getInput().dataViewId))
+      ((newInput.fieldName && !deepEqual(newInput.fieldName, embeddable.getInput().fieldName)) ||
+        (newInput.dataViewId && !deepEqual(newInput.dataViewId, embeddable.getInput().dataViewId)))
     ) {
       // if the field name or data view id has changed in this editing session, selected values are invalid, so reset them.
       newInput.value = ['', ''];

diff --git a/src/plugins/controls/public/control_types/time_slider/time_slider.tsx b/src/plugins/controls/public/control_types/time_slider/time_slider.tsx
@@ -6,7 +6,7 @@
  * Side Public License, v 1.
  */
 
-import React, { FC, useCallback, useState, useMemo } from 'react';
+import React, { FC, useCallback, useMemo } from 'react';
 import { BehaviorSubject } from 'rxjs';
 import { debounce } from 'lodash';
 import { useStateObservable } from '../../hooks/use_state_observable';
@@ -59,10 +59,6 @@ export const TimeSlider: FC<TimeSliderProps> = ({
 
   const { value } = useEmbeddableSelector((state) => state);
 
-  const [selectedValue, setSelectedValue] = useState<[number | null, number | null]>(
-    value || [null, null]
-  );
-
   const dispatchChange = useCallback(
     (range: [number | null, number | null]) => {
       dispatch(selectRange(range));
@@ -75,15 +71,14 @@ export const TimeSlider: FC<TimeSliderProps> = ({
   const onChangeComplete = useCallback(
     (range: [number | null, number | null]) => {
       debouncedDispatchChange(range);
-      setSelectedValue(range);
     },
-    [setSelectedValue, debouncedDispatchChange]
+    [debouncedDispatchChange]
   );
 
   return (
     <Component
       onChange={onChangeComplete}
-      value={selectedValue}
+      value={value ?? [null, null]}
       range={[min, max]}
       dateFormat={dateFormat}
       timezone={timezone}

diff --git a/src/plugins/controls/public/control_types/time_slider/time_slider_embeddable_factory.tsx b/src/plugins/controls/public/control_types/time_slider/time_slider_embeddable_factory.tsx
@@ -39,8 +39,8 @@ export class TimesliderEmbeddableFactory
   ) => {
     if (
       embeddable &&
-      (!deepEqual(newInput.fieldName, embeddable.getInput().fieldName) ||
-        !deepEqual(newInput.dataViewId, embeddable.getInput().dataViewId))
+      ((newInput.fieldName && !deepEqual(newInput.fieldName, embeddable.getInput().fieldName)) ||
+        (newInput.dataViewId && !deepEqual(newInput.dataViewId, embeddable.getInput().dataViewId)))
     ) {
       // if the field name or data view id has changed in this editing session, selected options are invalid, so reset them.
       newInput.value = undefined;

diff --git a/src/plugins/controls/public/types.ts b/src/plugins/controls/public/types.ts
@@ -75,4 +75,4 @@ export interface ControlsPluginStartDeps {
 }
 
 // re-export from common
-export type { ControlWidth, ControlInput, ControlStyle } from '../common/types';
+export type { ControlWidth, ControlInput, DataControlInput, ControlStyle } from '../common/types';
diff --git a/test/functional/apps/dashboard_elements/controls/options_list.ts b/test/functional/apps/dashboard_elements/controls/options_list.ts
@@ -116,6 +116,36 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
         });
       });
 
+      it('editing field clears selections', async () => {
+        const secondId = (await dashboardControls.getAllControlIds())[1];
+        await dashboardControls.optionsListOpenPopover(secondId);
+        await dashboardControls.optionsListPopoverSelectOption('hiss');
+        await dashboardControls.optionsListEnsurePopoverIsClosed(secondId);
+
+        await dashboardControls.editExistingControl(secondId);
+        await dashboardControls.controlsEditorSetfield('animal.keyword');
+        await dashboardControls.controlEditorSave();
+
+        const selectionString = await dashboardControls.optionsListGetSelectionsString(secondId);
+        expect(selectionString).to.be('Select...');
+      });
+
+      it('editing other control settings keeps selections', async () => {
+        const secondId = (await dashboardControls.getAllControlIds())[1];
+        await dashboardControls.optionsListOpenPopover(secondId);
+        await dashboardControls.optionsListPopoverSelectOption('dog');
+        await dashboardControls.optionsListPopoverSelectOption('cat');
+        await dashboardControls.optionsListEnsurePopoverIsClosed(secondId);
+
+        await dashboardControls.editExistingControl(secondId);
+        await dashboardControls.controlEditorSetTitle('Animal');
+        await dashboardControls.controlEditorSetWidth('large');
+        await dashboardControls.controlEditorSave();
+
+        const selectionString = await dashboardControls.optionsListGetSelectionsString(secondId);
+        expect(selectionString).to.be('dog, cat');
+      });
+
       it('deletes an existing control', async () => {
         const firstId = (await dashboardControls.getAllControlIds())[0];
 

diff --git a/test/functional/apps/dashboard_elements/controls/range_slider.ts b/test/functional/apps/dashboard_elements/controls/range_slider.ts
@@ -25,6 +25,20 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
     'header',
   ]);
 
+  const validateRange = async (
+    compare: 'value' | 'placeholder', // if 'value', compare actual selections; otherwise, compare the default range
+    controlId: string,
+    expectedLowerBound: string,
+    expectedUpperBound: string
+  ) => {
+    expect(await dashboardControls.rangeSliderGetLowerBoundAttribute(controlId, compare)).to.be(
+      expectedLowerBound
+    );
+    expect(await dashboardControls.rangeSliderGetUpperBoundAttribute(controlId, compare)).to.be(
+      expectedUpperBound
+    );
+  };
+
   describe('Range Slider Control', async () => {
     before(async () => {
       await security.testUser.setRoles([
@@ -82,12 +96,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
         });
         expect(await dashboardControls.getControlsCount()).to.be(2);
         const secondId = (await dashboardControls.getAllControlIds())[1];
-        expect(
-          await dashboardControls.rangeSliderGetLowerBoundAttribute(secondId, 'placeholder')
-        ).to.be('100');
-        expect(
-          await dashboardControls.rangeSliderGetUpperBoundAttribute(secondId, 'placeholder')
-        ).to.be('1200');
+        validateRange('placeholder', secondId, '100', '1200');
         // data views should be properly propagated from the control group to the dashboard
         expect(await filterBar.getIndexPatterns()).to.be('logstash-*,kibana_sample_data_flights');
       });
@@ -112,12 +121,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
         await dashboardControls.controlsEditorSetfield('dayOfWeek');
         await dashboardControls.controlEditorSave();
         await dashboardControls.rangeSliderWaitForLoading();
-        expect(
-          await dashboardControls.rangeSliderGetLowerBoundAttribute(firstId, 'placeholder')
-        ).to.be('0');
-        expect(
-          await dashboardControls.rangeSliderGetUpperBoundAttribute(firstId, 'placeholder')
-        ).to.be('6');
+        validateRange('placeholder', firstId, '0', '6');
+
         // when creating a new filter, the ability to select a data view should be removed, because the dashboard now only has one data view
         await retry.try(async () => {
           await testSubjects.click('addFilter');
@@ -150,31 +155,38 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
       it('applies filter from the first control on the second control', async () => {
         await dashboardControls.rangeSliderWaitForLoading();
         const secondId = (await dashboardControls.getAllControlIds())[1];
-        const availableMin = await dashboardControls.rangeSliderGetLowerBoundAttribute(
-          secondId,
-          'placeholder'
-        );
-        expect(availableMin).to.be('100');
-        const availabeMax = await dashboardControls.rangeSliderGetUpperBoundAttribute(
-          secondId,
-          'placeholder'
-        );
-        expect(availabeMax).to.be('1000');
+        validateRange('placeholder', secondId, '100', '1000');
+      });
+
+      it('editing field clears selections', async () => {
+        const secondId = (await dashboardControls.getAllControlIds())[1];
+        await dashboardControls.editExistingControl(secondId);
+        await dashboardControls.controlsEditorSetfield('FlightDelayMin');
+        await dashboardControls.controlEditorSave();
+
+        await dashboardControls.rangeSliderWaitForLoading();
+        validateRange('value', secondId, '', '');
+      });
+
+      it('editing other control settings keeps selections', async () => {
+        const secondId = (await dashboardControls.getAllControlIds())[1];
+        await dashboardControls.rangeSliderSetLowerBound(secondId, '50');
+        await dashboardControls.rangeSliderSetUpperBound(secondId, '100');
+        await dashboardControls.rangeSliderWaitForLoading();
+
+        await dashboardControls.editExistingControl(secondId);
+        await dashboardControls.controlEditorSetTitle('Minimum Flight Delay');
+        await dashboardControls.controlEditorSetWidth('large');
+        await dashboardControls.controlEditorSave();
+
+        await dashboardControls.rangeSliderWaitForLoading();
+        validateRange('value', secondId, '50', '100');
       });
 
       it('can clear out selections by clicking the reset button', async () => {
         const firstId = (await dashboardControls.getAllControlIds())[0];
         await dashboardControls.rangeSliderClearSelection(firstId);
-        const lowerBoundSelection = await dashboardControls.rangeSliderGetLowerBoundAttribute(
-          firstId,
-          'value'
-        );
-        expect(lowerBoundSelection.length).to.be(0);
-        const upperBoundSelection = await dashboardControls.rangeSliderGetUpperBoundAttribute(
-          firstId,
-          'value'
-        );
-        expect(upperBoundSelection.length).to.be(0);
+        validateRange('value', firstId, '', '');
       });
 
       it('deletes an existing control', async () => {