Variables: fix so the variable picker will remember selected options between filtering (#25119)

* added tests to verify flow.

* refactoring picker reducer.

* made all the tests green.

* removed console.log's

* fixed toggle all and making sure the correct values are set on picker open.

* added more tets.

* refactored and added table tests.

* fixed so we select values from selectedValues instead of options.

* fixed so you can navigate and select even after you have filtered a variable.

* adding tests to verify flows when toggling by highlight.

* fixed so enter always selects value before closing.

* improved the code for tags.

public/app/features/variables/pickers/OptionsPicker/actions.test.ts

@@ -9,6 +9,7 @@ import {
   toggleTag,
   updateOptionsAndFilter,
   updateSearchQuery,
+  moveOptionsHighlight,
 } from './reducer';
 import {
   commitChangesToVariable,
@@ -220,7 +221,7 @@ describe('options picker actions', () => {
         ] = actions;
         const expectedNumberOfActions = 6;
 
-        expect(toggleOptionAction).toEqual(toggleOption({ option: options[1], forceSelect: false, clearOthers }));
+        expect(toggleOptionAction).toEqual(toggleOption({ option: options[1], forceSelect: true, clearOthers }));
         expect(setCurrentValue).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option })));
         expect(changeQueryValue).toEqual(
           changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: '' }))
@@ -329,6 +330,45 @@ describe('options picker actions', () => {
     });
   });
 
+  describe('when commitChangesToVariable is dispatched with changes and list of options is filtered', () => {
+    it('then correct actions are dispatched', async () => {
+      const options = [createOption('A'), createOption('B'), createOption('C')];
+      const variable = createVariable({ options, includeAll: false });
+      const clearOthers = false;
+
+      const tester = await reduxTester<{ templating: TemplatingState }>()
+        .givenRootReducer(getRootReducer())
+        .whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
+        .whenActionIsDispatched(showOptions(variable))
+        .whenActionIsDispatched(navigateOptions(NavigationKey.moveDown, clearOthers))
+        .whenActionIsDispatched(toggleOptionByHighlight(clearOthers))
+        .whenActionIsDispatched(filterOrSearchOptions('C'))
+        .whenAsyncActionIsDispatched(commitChangesToVariable(), true);
+
+      const option = {
+        ...createOption('A'),
+        selected: true,
+        value: ['A'],
+        tags: [] as any[],
+      };
+
+      tester.thenDispatchedActionsPredicateShouldEqual(actions => {
+        const [setCurrentValue, changeQueryValue, updateOption, locationAction, hideAction] = actions;
+        const expectedNumberOfActions = 5;
+
+        expect(setCurrentValue).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option })));
+        expect(changeQueryValue).toEqual(
+          changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: 'C' }))
+        );
+        expect(updateOption).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option })));
+        expect(locationAction).toEqual(updateLocation({ query: { 'var-Constant': ['A'] } }));
+        expect(hideAction).toEqual(hideOptions());
+
+        return actions.length === expectedNumberOfActions;
+      });
+    });
+  });
+
   describe('when toggleOptionByHighlight is dispatched with changes', () => {
     it('then correct actions are dispatched', async () => {
       const options = [createOption('A'), createOption('B'), createOption('C')];
@@ -354,6 +394,42 @@ describe('options picker actions', () => {
     });
   });
 
+  describe('when toggleOptionByHighlight is dispatched with changes selected from a filtered options list', () => {
+    it('then correct actions are dispatched', async () => {
+      const options = [createOption('A'), createOption('B'), createOption('BC'), createOption('BD')];
+      const variable = createVariable({ options, includeAll: false });
+      const clearOthers = false;
+
+      const tester = await reduxTester<{ templating: TemplatingState }>()
+        .givenRootReducer(getRootReducer())
+        .whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
+        .whenActionIsDispatched(showOptions(variable))
+        .whenActionIsDispatched(navigateOptions(NavigationKey.moveDown, clearOthers))
+        .whenActionIsDispatched(toggleOptionByHighlight(clearOthers), true)
+        .whenActionIsDispatched(filterOrSearchOptions('B'))
+        .whenActionIsDispatched(navigateOptions(NavigationKey.moveDown, clearOthers))
+        .whenActionIsDispatched(navigateOptions(NavigationKey.moveDown, clearOthers))
+        .whenActionIsDispatched(toggleOptionByHighlight(clearOthers));
+
+      const optionA = createOption('A');
+      const optionBC = createOption('BD');
+
+      tester.thenDispatchedActionsPredicateShouldEqual(actions => {
+        const [toggleOptionA, filterOnB, updateAndFilter, firstMoveDown, secondMoveDown, toggleOptionBC] = actions;
+        const expectedNumberOfActions = 6;
+
+        expect(toggleOptionA).toEqual(toggleOption({ option: optionA, forceSelect: false, clearOthers }));
+        expect(filterOnB).toEqual(updateSearchQuery('B'));
+        expect(updateAndFilter).toEqual(updateOptionsAndFilter(variable.options));
+        expect(firstMoveDown).toEqual(moveOptionsHighlight(1));
+        expect(secondMoveDown).toEqual(moveOptionsHighlight(1));
+        expect(toggleOptionBC).toEqual(toggleOption({ option: optionBC, forceSelect: false, clearOthers }));
+
+        return actions.length === expectedNumberOfActions;
+      });
+    });
+  });
+
   describe('when toggleAndFetchTag is dispatched with values', () => {
     it('then correct actions are dispatched', async () => {
       const options = [createOption('A'), createOption('B'), createOption('C')];

public/app/features/variables/pickers/OptionsPicker/actions.ts

-import debounce from 'lodash/debounce';
+import { debounce, trim } from 'lodash';
 import { StoreState, ThunkDispatch, ThunkResult } from 'app/types';
 import {
   QueryVariableModel,
@@ -38,7 +38,7 @@ export const navigateOptions = (key: NavigationKey, clearOthers: boolean): Thunk
     }
 
     if (key === NavigationKey.selectAndClose) {
-      dispatch(toggleOptionByHighlight(clearOthers));
+      dispatch(toggleOptionByHighlight(clearOthers, true));
       return await dispatch(commitChangesToVariable());
     }
 
@@ -54,12 +54,16 @@ export const navigateOptions = (key: NavigationKey, clearOthers: boolean): Thunk
   };
 };
 
-export const filterOrSearchOptions = (searchQuery: string): ThunkResult<void> => {
+export const filterOrSearchOptions = (searchQuery = ''): ThunkResult<void> => {
   return async (dispatch, getState) => {
-    const { id } = getState().templating.optionsPicker;
+    const { id, queryValue } = getState().templating.optionsPicker;
     const { query, options } = getVariable<VariableWithOptions>(id!, getState());
     dispatch(updateSearchQuery(searchQuery));
 
+    if (trim(queryValue) === trim(searchQuery)) {
+      return;
+    }
+
     if (containsSearchFilter(query)) {
       return searchForOptionsWithDebounce(dispatch, getState, searchQuery);
     }
@@ -88,12 +92,11 @@ export const commitChangesToVariable = (): ThunkResult<void> => {
   };
 };
 
-export const toggleOptionByHighlight = (clearOthers: boolean): ThunkResult<void> => {
+export const toggleOptionByHighlight = (clearOthers: boolean, forceSelect = false): ThunkResult<void> => {
   return (dispatch, getState) => {
-    const { id, highlightIndex } = getState().templating.optionsPicker;
-    const variable = getVariable<VariableWithMultiSupport>(id, getState());
-    const option = variable.options[highlightIndex];
-    dispatch(toggleOption({ option, forceSelect: false, clearOthers }));
+    const { highlightIndex, options } = getState().templating.optionsPicker;
+    const option = options[highlightIndex];
+    dispatch(toggleOption({ option, forceSelect, clearOthers }));
   };
 };
 
@@ -155,20 +158,20 @@ const searchForOptions = async (dispatch: ThunkDispatch, getState: () => StoreSt
 const searchForOptionsWithDebounce = debounce(searchForOptions, 500);
 
 function mapToCurrent(picker: OptionsPickerState): VariableOption | undefined {
-  const { options, queryValue: searchQuery, multi } = picker;
+  const { options, selectedValues, queryValue: searchQuery, multi } = picker;
 
   if (options.length === 0 && searchQuery && searchQuery.length > 0) {
     return { text: searchQuery, value: searchQuery, selected: false };
   }
 
   if (!multi) {
-    return options.find(o => o.selected);
+    return selectedValues.find(o => o.selected);
   }
 
   const texts: string[] = [];
   const values: string[] = [];
 
-  for (const option of options) {
+  for (const option of selectedValues) {
     if (!option.selected) {
       continue;
     }

public/app/features/variables/pickers/OptionsPicker/reducer.ts

 import { createSlice, PayloadAction } from '@reduxjs/toolkit';
-import { cloneDeep } from 'lodash';
+import { cloneDeep, isString, trim } from 'lodash';
 import { VariableOption, VariableTag, VariableWithMultiSupport } from '../../../templating/types';
-import { ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE } from '../../state/types';
+import { ALL_VARIABLE_VALUE } from '../../state/types';
 import { isQuery } from '../../guard';
 import { applyStateChanges } from '../../../../core/utils/applyStateChanges';
 import { containsSearchFilter } from '../../../templating/utils';
@@ -43,8 +43,41 @@ const getTags = (model: VariableWithMultiSupport) => {
   return [];
 };
 
-const updateSelectedValues = (state: OptionsPickerState): OptionsPickerState => {
-  state.selectedValues = state.options.filter(o => o.selected);
+const optionsToRecord = (options: VariableOption[]): Record<string, VariableOption> => {
+  if (!Array.isArray(options)) {
+    return {};
+  }
+
+  return options.reduce((all: Record<string, VariableOption>, option) => {
+    if (isString(option.value)) {
+      all[option.value] = option;
+    }
+    return all;
+  }, {});
+};
+
+const updateOptions = (state: OptionsPickerState): OptionsPickerState => {
+  if (!Array.isArray(state.options)) {
+    state.options = [];
+    return state;
+  }
+
+  const selectedOptions = optionsToRecord(state.selectedValues);
+  state.selectedValues = Object.values(selectedOptions);
+
+  state.options = state.options.map(option => {
+    if (!isString(option.value)) {
+      return option;
+    }
+
+    const selected = !!selectedOptions[option.value];
+
+    if (option.selected === selected) {
+      return option;
+    }
+
+    return { ...option, selected };
+  });
   return state;
 };
 
@@ -52,13 +85,31 @@ const applyLimit = (options: VariableOption[]): VariableOption[] => {
   if (!Array.isArray(options)) {
     return [];
   }
-  return options.slice(0, Math.min(options.length, OPTIONS_LIMIT));
+  if (options.length <= OPTIONS_LIMIT) {
+    return options;
+  }
+  return options.slice(0, OPTIONS_LIMIT);
 };
 
 const updateDefaultSelection = (state: OptionsPickerState): OptionsPickerState => {
-  const { options } = state;
-  if (options.length > 0 && options.filter(o => o.selected).length === 0) {
-    options[0].selected = true;
+  const { options, selectedValues } = state;
+
+  if (options.length === 0 || selectedValues.length > 0) {
+    return state;
+  }
+
+  if (!options[0] || options[0].value !== ALL_VARIABLE_VALUE) {
+    return state;
+  }
+
+  state.selectedValues = [{ ...options[0], selected: true }];
+  return state;
+};
+
+const updateAllSelection = (state: OptionsPickerState): OptionsPickerState => {
+  const { selectedValues } = state;
+  if (selectedValues.length > 1) {
+    state.selectedValues = selectedValues.filter(option => option.value !== ALL_VARIABLE_VALUE);
   }
   return state;
 };
@@ -83,33 +134,33 @@ const optionsPickerSlice = createSlice({
         state.queryValue = queryHasSearchFilter && queryValue ? queryValue : '';
       }
 
-      return applyStateChanges(state, updateSelectedValues);
+      state.selectedValues = state.options.filter(option => option.selected);
+      return applyStateChanges(state, updateDefaultSelection, updateOptions);
     },
     hideOptions: (state, action: PayloadAction): OptionsPickerState => {
       return { ...initialState };
     },
     toggleOption: (state, action: PayloadAction<ToggleOption>): OptionsPickerState => {
-      const { option, forceSelect, clearOthers } = action.payload;
-      const { multi } = state;
-      const newOptions: VariableOption[] = state.options.map(o => {
-        if (o.value !== option.value) {
-          let selected = o.selected;
-          if (o.text === ALL_VARIABLE_TEXT || option.text === ALL_VARIABLE_TEXT) {
-            selected = false;
-          } else if (!multi) {
-            selected = false;
-          } else if (clearOthers) {
-            selected = false;
-          }
-          o.selected = selected;
-          return o;
+      const { option, clearOthers, forceSelect } = action.payload;
+      const { multi, selectedValues } = state;
+      const selected = !selectedValues.find(o => o.value === option.value);
+
+      if (option.value === ALL_VARIABLE_VALUE || !multi || clearOthers) {
+        if (selected || forceSelect) {
+          state.selectedValues = [{ ...option, selected: true }];
+        } else {
+          state.selectedValues = [];
         }
-        o.selected = forceSelect ? true : multi ? !option.selected : true;
-        return o;
-      });
+        return applyStateChanges(state, updateDefaultSelection, updateAllSelection, updateOptions);
+      }
 
-      state.options = newOptions;
-      return applyStateChanges(state, updateDefaultSelection, updateSelectedValues);
+      if (forceSelect || selected) {
+        state.selectedValues.push({ ...option, selected: true });
+        return applyStateChanges(state, updateDefaultSelection, updateAllSelection, updateOptions);
+      }
+
+      state.selectedValues = selectedValues.filter(o => o.value !== option.value);
+      return applyStateChanges(state, updateDefaultSelection, updateAllSelection, updateOptions);
     },
     toggleTag: (state, action: PayloadAction<VariableTag>): OptionsPickerState => {
       const tag = action.payload;
@@ -133,20 +184,21 @@ const optionsPickerSlice = createSlice({
         return t;
       });
 
-      state.options = state.options.map(option => {
-        if (option.value === ALL_VARIABLE_VALUE && selected === true) {
-          option.selected = false;
-        }
+      const availableOptions = optionsToRecord(state.options);
 
-        if (values.indexOf(option.value) === -1) {
-          return option;
-        }
+      if (!selected) {
+        state.selectedValues = state.selectedValues.filter(
+          option => !isString(option.value) || !availableOptions[option.value]
+        );
+        return applyStateChanges(state, updateDefaultSelection, updateOptions);
+      }
 
-        option.selected = selected;
-        return option;
-      });
+      const optionsFromTag = values
+        .filter(value => value !== ALL_VARIABLE_VALUE && !!availableOptions[value])
+        .map(value => ({ selected, value, text: value }));
 
-      return applyStateChanges(state, updateDefaultSelection, updateSelectedValues);
+      state.selectedValues.push.apply(state.selectedValues, optionsFromTag);
+      return applyStateChanges(state, updateDefaultSelection, updateOptions);
     },
     moveOptionsHighlight: (state, action: PayloadAction<number>): OptionsPickerState => {
       let nextIndex = state.highlightIndex + action.payload;
@@ -163,20 +215,24 @@ const optionsPickerSlice = createSlice({
       };
     },
     toggleAllOptions: (state, action: PayloadAction): OptionsPickerState => {
-      const selected = !state.options.find(option => option.selected);
-      state.options = state.options.map(option => ({
+      if (state.selectedValues.length > 0) {
+        state.selectedValues = [];
+        return applyStateChanges(state, updateOptions);
+      }
+
+      state.selectedValues = state.options.map(option => ({
         ...option,
-        selected,
+        selected: true,
       }));
 
-      return applyStateChanges(state, updateSelectedValues);
+      return applyStateChanges(state, updateOptions);
     },
     updateSearchQuery: (state, action: PayloadAction<string>): OptionsPickerState => {
       state.queryValue = action.payload;
       return state;
     },
     updateOptionsAndFilter: (state, action: PayloadAction<VariableOption[]>): OptionsPickerState => {
-      const searchQuery = (state.queryValue ?? '').toLowerCase();
+      const searchQuery = trim((state.queryValue ?? '').toLowerCase());
 
       const filteredOptions = action.payload.filter(option => {
         const text = Array.isArray(option.text) ? option.text.toString() : option.text;
@@ -186,13 +242,13 @@ const optionsPickerSlice = createSlice({
       state.options = applyLimit(filteredOptions);
       state.highlightIndex = 0;
 
-      return applyStateChanges(state, updateSelectedValues);
+      return applyStateChanges(state, updateDefaultSelection, updateOptions);
     },
     updateOptionsFromSearch: (state, action: PayloadAction<VariableOption[]>): OptionsPickerState => {
       state.options = applyLimit(action.payload);
       state.highlightIndex = 0;
 
-      return applyStateChanges(state, updateSelectedValues);
+      return applyStateChanges(state, updateDefaultSelection, updateOptions);
     },
   },
 });

public/app/features/variables/pickers/shared/VariableInput.tsx

 import React, { PureComponent } from 'react';
-import { trim } from 'lodash';
 import { NavigationKey } from '../types';
 
 export interface Props {
@@ -17,15 +16,9 @@ export class VariableInput extends PureComponent<Props> {
   };
 
   onChange = (event: React.ChangeEvent<HTMLInputElement>) => {
-    if (this.shouldUpdateValue(event.target.value)) {
-      this.props.onChange(event.target.value);
-    }
+    this.props.onChange(event.target.value);
   };
 
-  private shouldUpdateValue(value: string) {
-    return trim(value ?? '').length > 0 || trim(this.props.value ?? '').length > 0;
-  }
-
   render() {
     return (
       <input