From 13787294c6b3bbf7e7d797c3048758e13ee6443e Mon Sep 17 00:00:00 2001 From: Marcus Andersson Date: Mon, 1 Jun 2020 15:06:51 +0200 Subject: [PATCH] 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. --- .../pickers/OptionsPicker/actions.test.ts | 78 ++- .../pickers/OptionsPicker/actions.ts | 27 +- .../pickers/OptionsPicker/reducer.test.ts | 483 ++++++++++++------ .../pickers/OptionsPicker/reducer.ts | 150 ++++-- .../pickers/shared/VariableInput.tsx | 9 +- 5 files changed, 531 insertions(+), 216 deletions(-) diff --git a/public/app/features/variables/pickers/OptionsPicker/actions.test.ts b/public/app/features/variables/pickers/OptionsPicker/actions.test.ts index 2f39de386e3..4ac2031af27 100644 --- a/public/app/features/variables/pickers/OptionsPicker/actions.test.ts +++ b/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')]; diff --git a/public/app/features/variables/pickers/OptionsPicker/actions.ts b/public/app/features/variables/pickers/OptionsPicker/actions.ts index 44271edece1..93239868c7a 100644 --- a/public/app/features/variables/pickers/OptionsPicker/actions.ts +++ b/public/app/features/variables/pickers/OptionsPicker/actions.ts @@ -1,4 +1,4 @@ -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 => { +export const filterOrSearchOptions = (searchQuery = ''): ThunkResult => { return async (dispatch, getState) => { - const { id } = getState().templating.optionsPicker; + const { id, queryValue } = getState().templating.optionsPicker; const { query, options } = getVariable(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 => { }; }; -export const toggleOptionByHighlight = (clearOthers: boolean): ThunkResult => { +export const toggleOptionByHighlight = (clearOthers: boolean, forceSelect = false): ThunkResult => { return (dispatch, getState) => { - const { id, highlightIndex } = getState().templating.optionsPicker; - const variable = getVariable(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; } diff --git a/public/app/features/variables/pickers/OptionsPicker/reducer.test.ts b/public/app/features/variables/pickers/OptionsPicker/reducer.test.ts index 09aa48b8233..4fbc045d2cf 100644 --- a/public/app/features/variables/pickers/OptionsPicker/reducer.test.ts +++ b/public/app/features/variables/pickers/OptionsPicker/reducer.test.ts @@ -15,7 +15,7 @@ import { updateSearchQuery, } from './reducer'; import { reducerTester } from '../../../../../test/core/redux/reducerTester'; -import { QueryVariableModel, VariableTag } from '../../../templating/types'; +import { QueryVariableModel, VariableTag, VariableOption } from '../../../templating/types'; import { ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE } from '../../state/types'; const getVariableTestContext = (extend: Partial) => { @@ -30,32 +30,17 @@ const getVariableTestContext = (extend: Partial) => { describe('optionsPickerReducer', () => { describe('when toggleOption is dispatched', () => { const opsAll = [ - { text: 'All', value: '$__all', selected: true }, + { text: '$__all', value: '$__all', selected: true }, { text: 'A', value: 'A', selected: false }, { text: 'B', value: 'B', selected: false }, ]; const opsA = [ - { text: 'All', value: '$__all', selected: false }, + { text: '$__all', value: '$__all', selected: false }, { text: 'A', value: 'A', selected: true }, { text: 'B', value: 'B', selected: false }, ]; - const opsB = [ - { text: 'All', value: '$__all', selected: false }, - { text: 'A', value: 'A', selected: false }, - { text: 'B', value: 'B', selected: true }, - ]; const opsAB = [ - { text: 'All', value: '$__all', selected: false }, - { text: 'A', value: 'A', selected: true }, - { text: 'B', value: 'B', selected: true }, - ]; - - const opA = { text: 'A', selected: true, value: 'A' }; - const opANot = { text: 'A', selected: false, value: 'A' }; - const opASel = [{ text: 'A', value: 'A', selected: true }]; - const opBSel = [{ text: 'B', value: 'B', selected: true }]; - const opAllSel = [{ text: 'All', value: '$__all', selected: true }]; - const opABSel = [ + { text: '$__all', value: '$__all', selected: false }, { text: 'A', value: 'A', selected: true }, { text: 'B', value: 'B', selected: true }, ]; @@ -66,125 +51,114 @@ describe('optionsPickerReducer', () => { forceSelect: any; clearOthers: any; option: any; - expOps: any; - expSel: any; + expectSelected: any; }) => { - const { initialState } = getVariableTestContext({ options: args.options, multi: args.multi }); - const payload = { forceSelect: args.forceSelect, clearOthers: args.clearOthers, option: args.option }; + const { initialState } = getVariableTestContext({ + options: args.options, + multi: args.multi, + selectedValues: args.options.filter((o: any) => o.selected), + }); + const payload = { + forceSelect: args.forceSelect, + clearOthers: args.clearOthers, + option: { text: args.option, value: args.option, selected: true }, + }; + const expectedAsRecord: any = args.expectSelected.reduce((all: any, current: any) => { + all[current] = current; + return all; + }, {}); reducerTester() .givenReducer(optionsPickerReducer, cloneDeep(initialState)) .whenActionIsDispatched(toggleOption(payload)) .thenStateShouldEqual({ ...initialState, - selectedValues: args.expSel, - options: args.expOps, + selectedValues: args.expectSelected.map((value: any) => ({ value, text: value, selected: true })), + options: args.options.map((option: any) => { + return { ...option, selected: !!expectedAsRecord[option.value] }; + }), }); }; describe('toggleOption for multi value variable', () => { const multi = true; - describe('and options with All selected', () => { + describe('and value All is selected in options', () => { const options = opsAll; it.each` - option | forceSelect | clearOthers | expOps | expSel - ${opANot} | ${true} | ${false} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${false} | ${opsA} | ${opASel} - ${opANot} | ${true} | ${true} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${true} | ${opsA} | ${opASel} - ${opA} | ${true} | ${false} | ${opsA} | ${opASel} - ${opA} | ${false} | ${false} | ${opsAll} | ${opAllSel} - ${opA} | ${true} | ${true} | ${opsA} | ${opASel} - ${opA} | ${false} | ${true} | ${opsAll} | ${opAllSel} + option | forceSelect | clearOthers | expectSelected + ${'A'} | ${true} | ${false} | ${['A']} + ${'A'} | ${false} | ${false} | ${['A']} + ${'A'} | ${true} | ${true} | ${['A']} + ${'A'} | ${false} | ${true} | ${['A']} + ${'B'} | ${true} | ${false} | ${['B']} + ${'B'} | ${false} | ${false} | ${['B']} + ${'B'} | ${true} | ${true} | ${['B']} + ${'B'} | ${false} | ${true} | ${['B']} + ${'$__all'} | ${true} | ${false} | ${['$__all']} + ${'$__all'} | ${false} | ${false} | ${['$__all']} + ${'$__all'} | ${true} | ${true} | ${['$__all']} + ${'$__all'} | ${false} | ${true} | ${['$__all']} `( - 'when toggleOption is dispatched and option: $option, forceSelect: $forceSelect, clearOthers: $clearOthers, expOps: $expOps, expSel: $expSel', - ({ option, forceSelect, clearOthers, expOps, expSel }) => + 'and we toggle $option with options: { forceSelect: $forceSelect, clearOthers: $clearOthers } we expect $expectSelected to be selected', + ({ option, forceSelect, clearOthers, expectSelected }) => expectToggleOptionState({ options, multi, option, clearOthers, forceSelect, - expOps, - expSel, + expectSelected, }) ); }); - describe('and options with A selected', () => { + describe('and value A is selected in options', () => { const options = opsA; it.each` - option | forceSelect | clearOthers | expOps | expSel - ${opANot} | ${true} | ${false} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${false} | ${opsA} | ${opASel} - ${opANot} | ${true} | ${true} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${true} | ${opsA} | ${opASel} - ${opA} | ${true} | ${false} | ${opsA} | ${opASel} - ${opA} | ${false} | ${false} | ${opsAll} | ${opAllSel} - ${opA} | ${true} | ${true} | ${opsA} | ${opASel} - ${opA} | ${false} | ${true} | ${opsAll} | ${opAllSel} + option | forceSelect | clearOthers | expectSelected + ${'A'} | ${true} | ${false} | ${['A']} + ${'A'} | ${false} | ${false} | ${['$__all']} + ${'A'} | ${true} | ${true} | ${['A']} + ${'A'} | ${false} | ${true} | ${['$__all']} + ${'B'} | ${true} | ${true} | ${['B']} + ${'B'} | ${false} | ${true} | ${['B']} + ${'B'} | ${true} | ${false} | ${['A', 'B']} + ${'B'} | ${false} | ${false} | ${['A', 'B']} `( - 'when toggleOption is dispatched and option: $option, forceSelect: $forceSelect, clearOthers: $clearOthers, expOps: $expOps, expSel: $expSel', - ({ option, forceSelect, clearOthers, expOps, expSel }) => + 'and we toggle $option with options: { forceSelect: $forceSelect, clearOthers: $clearOthers } we expect $expectSelected to be selected', + ({ option, forceSelect, clearOthers, expectSelected }) => expectToggleOptionState({ options, multi, option, clearOthers, forceSelect, - expOps, - expSel, + expectSelected, }) ); }); - describe('and options with B selected', () => { - const options = opsB; - it.each` - option | forceSelect | clearOthers | expOps | expSel - ${opANot} | ${true} | ${false} | ${opsAB} | ${opABSel} - ${opANot} | ${false} | ${false} | ${opsAB} | ${opABSel} - ${opANot} | ${true} | ${true} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${true} | ${opsA} | ${opASel} - ${opA} | ${true} | ${false} | ${opsAB} | ${opABSel} - ${opA} | ${false} | ${false} | ${opsB} | ${opBSel} - ${opA} | ${true} | ${true} | ${opsA} | ${opASel} - ${opA} | ${false} | ${true} | ${opsAll} | ${opAllSel} - `( - 'when toggleOption is dispatched and option: $option, forceSelect: $forceSelect, clearOthers: $clearOthers, expOps: $expOps, expSel: $expSel', - ({ option, forceSelect, clearOthers, expOps, expSel }) => - expectToggleOptionState({ - options, - multi, - option, - clearOthers, - forceSelect, - expOps, - expSel, - }) - ); - }); - describe('and options with A + B selected', () => { + + describe('and values A + B is selected in options', () => { const options = opsAB; it.each` - option | forceSelect | clearOthers | expOps | expSel - ${opANot} | ${true} | ${false} | ${opsAB} | ${opABSel} - ${opANot} | ${false} | ${false} | ${opsAB} | ${opABSel} - ${opANot} | ${true} | ${true} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${true} | ${opsA} | ${opASel} - ${opA} | ${true} | ${false} | ${opsAB} | ${opABSel} - ${opA} | ${false} | ${false} | ${opsB} | ${opBSel} - ${opA} | ${true} | ${true} | ${opsA} | ${opASel} - ${opA} | ${false} | ${true} | ${opsAll} | ${opAllSel} + option | forceSelect | clearOthers | expectSelected + ${'A'} | ${true} | ${false} | ${['A', 'B']} + ${'A'} | ${false} | ${false} | ${['B']} + ${'A'} | ${true} | ${true} | ${['A']} + ${'A'} | ${false} | ${true} | ${['$__all']} + ${'B'} | ${true} | ${true} | ${['B']} + ${'B'} | ${false} | ${true} | ${['$__all']} + ${'B'} | ${true} | ${false} | ${['A', 'B']} + ${'B'} | ${false} | ${false} | ${['A']} `( - 'when toggleOption is dispatched and option: $option, forceSelect: $forceSelect, clearOthers: $clearOthers, expOps: $expOps, expSel: $expSel', - ({ option, forceSelect, clearOthers, expOps, expSel }) => + 'and we toggle $option with options: { forceSelect: $forceSelect, clearOthers: $clearOthers } we expect $expectSelected to be selected', + ({ option, forceSelect, clearOthers, expectSelected }) => expectToggleOptionState({ options, multi, option, clearOthers, forceSelect, - expOps, - expSel, + expectSelected, }) ); }); @@ -192,87 +166,92 @@ describe('optionsPickerReducer', () => { describe('toggleOption for single value variable', () => { const multi = false; - describe('and options with All selected', () => { + describe('and value All is selected in options', () => { const options = opsAll; it.each` - option | forceSelect | clearOthers | expOps | expSel - ${opANot} | ${true} | ${false} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${false} | ${opsA} | ${opASel} - ${opANot} | ${true} | ${true} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${true} | ${opsA} | ${opASel} - ${opA} | ${true} | ${false} | ${opsA} | ${opASel} - ${opA} | ${false} | ${false} | ${opsA} | ${opASel} - ${opA} | ${true} | ${true} | ${opsA} | ${opASel} - ${opA} | ${false} | ${true} | ${opsA} | ${opASel} + option | forceSelect | clearOthers | expectSelected + ${'A'} | ${true} | ${false} | ${['A']} + ${'A'} | ${false} | ${false} | ${['A']} + ${'A'} | ${true} | ${true} | ${['A']} + ${'A'} | ${false} | ${true} | ${['A']} + ${'B'} | ${true} | ${false} | ${['B']} + ${'B'} | ${false} | ${false} | ${['B']} + ${'B'} | ${true} | ${true} | ${['B']} + ${'B'} | ${false} | ${true} | ${['B']} + ${'$__all'} | ${true} | ${false} | ${['$__all']} + ${'$__all'} | ${false} | ${false} | ${['$__all']} + ${'$__all'} | ${true} | ${true} | ${['$__all']} + ${'$__all'} | ${false} | ${true} | ${['$__all']} `( - 'when toggleOption is dispatched and option: $option, forceSelect: $forceSelect, clearOthers: $clearOthers, expOps: $expOps, expSel: $expSel', - ({ option, forceSelect, clearOthers, expOps, expSel }) => + 'and we toggle $option with options: { forceSelect: $forceSelect, clearOthers: $clearOthers } we expect $expectSelected to be selected', + ({ option, forceSelect, clearOthers, expectSelected }) => expectToggleOptionState({ options, multi, option, clearOthers, forceSelect, - expOps, - expSel, + expectSelected, }) ); }); - describe('and options with A selected', () => { + describe('and value A is selected in options', () => { const options = opsA; it.each` - option | forceSelect | clearOthers | expOps | expSel - ${opANot} | ${true} | ${false} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${false} | ${opsA} | ${opASel} - ${opANot} | ${true} | ${true} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${true} | ${opsA} | ${opASel} - ${opA} | ${true} | ${false} | ${opsA} | ${opASel} - ${opA} | ${false} | ${false} | ${opsA} | ${opASel} - ${opA} | ${true} | ${true} | ${opsA} | ${opASel} - ${opA} | ${false} | ${true} | ${opsA} | ${opASel} + option | forceSelect | clearOthers | expectSelected + ${'A'} | ${true} | ${false} | ${['A']} + ${'A'} | ${false} | ${false} | ${['$__all']} + ${'A'} | ${true} | ${true} | ${['A']} + ${'A'} | ${false} | ${true} | ${['$__all']} + ${'B'} | ${true} | ${false} | ${['B']} + ${'B'} | ${false} | ${false} | ${['B']} + ${'B'} | ${true} | ${true} | ${['B']} + ${'B'} | ${false} | ${true} | ${['B']} `( - 'when toggleOption is dispatched and option: $option, forceSelect: $forceSelect, clearOthers: $clearOthers, expOps: $expOps, expSel: $expSel', - ({ option, forceSelect, clearOthers, expOps, expSel }) => + 'and we toggle $option with options: { forceSelect: $forceSelect, clearOthers: $clearOthers } we expect $expectSelected to be selected', + ({ option, forceSelect, clearOthers, expectSelected }) => expectToggleOptionState({ options, multi, option, clearOthers, forceSelect, - expOps, - expSel, - }) - ); - }); - describe('and options with B selected', () => { - const options = opsB; - it.each` - option | forceSelect | clearOthers | expOps | expSel - ${opANot} | ${true} | ${false} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${false} | ${opsA} | ${opASel} - ${opANot} | ${true} | ${true} | ${opsA} | ${opASel} - ${opANot} | ${false} | ${true} | ${opsA} | ${opASel} - ${opA} | ${true} | ${false} | ${opsA} | ${opASel} - ${opA} | ${false} | ${false} | ${opsA} | ${opASel} - ${opA} | ${true} | ${true} | ${opsA} | ${opASel} - ${opA} | ${false} | ${true} | ${opsA} | ${opASel} - `( - 'when toggleOption is dispatched and option: $option, forceSelect: $forceSelect, clearOthers: $clearOthers, expOps: $expOps, expSel: $expSel', - ({ option, forceSelect, clearOthers, expOps, expSel }) => - expectToggleOptionState({ - options, - multi, - option, - clearOthers, - forceSelect, - expOps, - expSel, + expectSelected, }) ); }); }); }); + describe('when showOptions is dispatched', () => { + it('then correct values should be selected', () => { + const { initialState } = getVariableTestContext({}); + const payload = { + type: 'query', + query: '', + options: [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: true }, + ], + multi: false, + id: '0', + } as QueryVariableModel; + + reducerTester() + .givenReducer(optionsPickerReducer, cloneDeep(initialState)) + .whenActionIsDispatched(showOptions(payload)) + .thenStateShouldEqual({ + ...initialState, + options: payload.options, + id: payload.id!, + multi: payload.multi, + selectedValues: [{ text: 'B', value: 'B', selected: true }], + queryValue: '', + }); + }); + }); + describe('when showOptions is dispatched and picker has queryValue and variable has searchFilter', () => { it('then state should be correct', () => { const query = '*.__searchFilter'; @@ -395,6 +374,43 @@ describe('optionsPickerReducer', () => { }); }); + describe('when toggleTag is dispatched when tag is selected', () => { + it('then state should be correct', () => { + const { initialState } = getVariableTestContext({ + tags: [ + { text: 'All A:s', selected: true, values: ['A', 'AA', 'AAA'] }, + { text: 'All B:s', selected: false, values: ['B', 'BB', 'BBB'] }, + { text: 'All C:s', selected: false, values: ['C', 'CC', 'CCC'] }, + ], + options: [ + { text: 'A', selected: true, value: 'A' }, + { text: 'AA', selected: true, value: 'AA' }, + { text: 'AAA', selected: true, value: 'AAA' }, + { text: 'B', selected: false, value: 'B' }, + ], + }); + const payload: VariableTag = { text: 'All A:s', selected: true, values: ['A', 'AA', 'AAA'] }; + reducerTester() + .givenReducer(optionsPickerReducer, cloneDeep(initialState)) + .whenActionIsDispatched(toggleTag(payload)) + .thenStateShouldEqual({ + ...initialState, + options: [ + { text: 'A', selected: false, value: 'A' }, + { text: 'AA', selected: false, value: 'AA' }, + { text: 'AAA', selected: false, value: 'AAA' }, + { text: 'B', selected: false, value: 'B' }, + ], + tags: [ + { text: 'All A:s', selected: false, values: ['A', 'AA', 'AAA'] }, + { text: 'All B:s', selected: false, values: ['B', 'BB', 'BBB'] }, + { text: 'All C:s', selected: false, values: ['C', 'CC', 'CCC'] }, + ], + selectedValues: [], + }); + }); + }); + describe('when toggleTag is dispatched and ALL is previous selected', () => { it('then state should be correct', () => { const { initialState } = getVariableTestContext({ @@ -554,12 +570,14 @@ describe('optionsPickerReducer', () => { }); describe('when toggleAllOptions is dispatched', () => { - it('then state should be correct', () => { + it('should toggle all values to true', () => { const { initialState } = getVariableTestContext({ options: [ + { text: 'All', value: '$__all', selected: false }, { text: 'A', value: 'A', selected: false }, { text: 'B', value: 'B', selected: false }, ], + selectedValues: [], multi: true, }); @@ -569,15 +587,67 @@ describe('optionsPickerReducer', () => { .thenStateShouldEqual({ ...initialState, options: [ + { text: 'All', value: '$__all', selected: true }, { text: 'A', value: 'A', selected: true }, { text: 'B', value: 'B', selected: true }, ], selectedValues: [ + { text: 'All', value: '$__all', selected: true }, { text: 'A', value: 'A', selected: true }, { text: 'B', value: 'B', selected: true }, ], }); }); + + it('should toggle all values to false when $_all is selected', () => { + const { initialState } = getVariableTestContext({ + options: [ + { text: 'All', value: '$__all', selected: true }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: false }, + ], + selectedValues: [{ text: 'All', value: '$__all', selected: true }], + multi: true, + }); + + reducerTester() + .givenReducer(optionsPickerReducer, cloneDeep(initialState)) + .whenActionIsDispatched(toggleAllOptions()) + .thenStateShouldEqual({ + ...initialState, + options: [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: false }, + ], + selectedValues: [], + }); + }); + + it('should toggle all values to false when a option is selected', () => { + const { initialState } = getVariableTestContext({ + options: [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: true }, + ], + selectedValues: [{ text: 'B', value: 'B', selected: true }], + multi: true, + }); + + reducerTester() + .givenReducer(optionsPickerReducer, cloneDeep(initialState)) + .whenActionIsDispatched(toggleAllOptions()) + .thenStateShouldEqual({ + ...initialState, + options: [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: false }, + ], + selectedValues: [], + }); + }); }); describe('when updateOptionsAndFilter is dispatched and searchFilter exists', () => { @@ -636,6 +706,123 @@ describe('optionsPickerReducer', () => { }); }); + describe('when value is selected and filter is applied but then removed', () => { + it('then state should be correct', () => { + const searchQuery = 'A'; + + const options: VariableOption[] = [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: false }, + ]; + + const { initialState } = getVariableTestContext({ + options, + }); + + reducerTester() + .givenReducer(optionsPickerReducer, cloneDeep(initialState)) + .whenActionIsDispatched(toggleOption({ option: options[2], forceSelect: false, clearOthers: false })) + .thenStateShouldEqual({ + ...initialState, + options: [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: true }, + ], + selectedValues: [{ text: 'B', value: 'B', selected: true }], + }) + .whenActionIsDispatched(updateSearchQuery(searchQuery)) + .thenStateShouldEqual({ + ...initialState, + options: [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: true }, + ], + selectedValues: [{ text: 'B', value: 'B', selected: true }], + queryValue: searchQuery, + }) + .whenActionIsDispatched(updateOptionsAndFilter(options)) + .thenStateShouldEqual({ + ...initialState, + options: [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + ], + selectedValues: [{ text: 'B', value: 'B', selected: true }], + queryValue: searchQuery, + highlightIndex: 0, + }) + .whenActionIsDispatched(updateSearchQuery('')) + .thenStateShouldEqual({ + ...initialState, + options: [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + ], + selectedValues: [{ text: 'B', value: 'B', selected: true }], + queryValue: '', + highlightIndex: 0, + }) + .whenActionIsDispatched(updateOptionsAndFilter(options)) + .thenStateShouldEqual({ + ...initialState, + options: [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: true }, + ], + selectedValues: [{ text: 'B', value: 'B', selected: true }], + queryValue: '', + highlightIndex: 0, + }); + }); + }); + + describe('when value is toggled back and forth', () => { + it('then state should be correct', () => { + const options: VariableOption[] = [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: false }, + ]; + + const toggleOptionAction = toggleOption({ + option: options[2], + forceSelect: false, + clearOthers: false, + }); + + const { initialState } = getVariableTestContext({ + options, + }); + + reducerTester() + .givenReducer(optionsPickerReducer, cloneDeep(initialState)) + .whenActionIsDispatched(toggleOptionAction) + .thenStateShouldEqual({ + ...initialState, + options: [ + { text: 'All', value: '$__all', selected: false }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: true }, + ], + selectedValues: [{ text: 'B', value: 'B', selected: true }], + }) + .whenActionIsDispatched(toggleOptionAction) + .thenStateShouldEqual({ + ...initialState, + options: [ + { text: 'All', value: '$__all', selected: true }, + { text: 'A', value: 'A', selected: false }, + { text: 'B', value: 'B', selected: false }, + ], + selectedValues: [{ text: 'All', value: '$__all', selected: true }], + }); + }); + }); + describe('when updateOptionsFromSearch is dispatched and variable has searchFilter', () => { it('then state should be correct', () => { const searchQuery = '__searchFilter'; diff --git a/public/app/features/variables/pickers/OptionsPicker/reducer.ts b/public/app/features/variables/pickers/OptionsPicker/reducer.ts index 0e988fe0394..196a9e47f4b 100644 --- a/public/app/features/variables/pickers/OptionsPicker/reducer.ts +++ b/public/app/features/variables/pickers/OptionsPicker/reducer.ts @@ -1,7 +1,7 @@ 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 => { + if (!Array.isArray(options)) { + return {}; + } + + return options.reduce((all: Record, 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): 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; - } - o.selected = forceSelect ? true : multi ? !option.selected : true; - return o; - }); + const { option, clearOthers, forceSelect } = action.payload; + const { multi, selectedValues } = state; + const selected = !selectedValues.find(o => o.value === option.value); - state.options = newOptions; - return applyStateChanges(state, updateDefaultSelection, updateSelectedValues); + if (option.value === ALL_VARIABLE_VALUE || !multi || clearOthers) { + if (selected || forceSelect) { + state.selectedValues = [{ ...option, selected: true }]; + } else { + state.selectedValues = []; + } + return applyStateChanges(state, updateDefaultSelection, updateAllSelection, updateOptions); + } + + 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): 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): 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): OptionsPickerState => { state.queryValue = action.payload; return state; }, updateOptionsAndFilter: (state, action: PayloadAction): 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): OptionsPickerState => { state.options = applyLimit(action.payload); state.highlightIndex = 0; - return applyStateChanges(state, updateSelectedValues); + return applyStateChanges(state, updateDefaultSelection, updateOptions); }, }, }); diff --git a/public/app/features/variables/pickers/shared/VariableInput.tsx b/public/app/features/variables/pickers/shared/VariableInput.tsx index 37826df396f..c9c9ca15e3b 100644 --- a/public/app/features/variables/pickers/shared/VariableInput.tsx +++ b/public/app/features/variables/pickers/shared/VariableInput.tsx @@ -1,5 +1,4 @@ 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 { }; onChange = (event: React.ChangeEvent) => { - 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 (