From bd75adf515e8b5a18f0c747642f25056869757c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Sun, 16 Aug 2020 22:04:27 -0700 Subject: [PATCH] Templating: Fixes so texts show in picker not the values (#27002) --- .../pickers/OptionsPicker/actions.test.ts | 268 +++++++----------- .../pickers/OptionsPicker/actions.ts | 2 +- .../variables/state/sharedReducer.test.ts | 2 +- .../features/variables/state/sharedReducer.ts | 8 +- 4 files changed, 100 insertions(+), 180 deletions(-) diff --git a/public/app/features/variables/pickers/OptionsPicker/actions.test.ts b/public/app/features/variables/pickers/OptionsPicker/actions.test.ts index 02551d6aaa9..80452394703 100644 --- a/public/app/features/variables/pickers/OptionsPicker/actions.test.ts +++ b/public/app/features/variables/pickers/OptionsPicker/actions.test.ts @@ -4,12 +4,12 @@ import { TemplatingState } from '../../state/reducers'; import { QueryVariableModel, VariableHide, VariableRefresh, VariableSort } from '../../types'; import { hideOptions, + moveOptionsHighlight, showOptions, toggleOption, toggleTag, updateOptionsAndFilter, updateSearchQuery, - moveOptionsHighlight, } from './reducer'; import { commitChangesToVariable, @@ -45,7 +45,10 @@ describe('options picker actions', () => { describe('when navigateOptions is dispatched with navigation key cancel', () => { it('then correct actions are dispatched', async () => { - const variable = createVariable({ options: [createOption('A', 'A', true)] }); + const variable = createMultiVariable({ + options: [createOption('A', 'A', true)], + current: createOption(['A'], ['A'], true), + }); const clearOthers = false; const key = NavigationKey.cancel; @@ -57,33 +60,30 @@ describe('options picker actions', () => { .whenAsyncActionIsDispatched(navigateOptions(key, clearOthers), true); const option = { - ...createOption('A'), + ...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: '' })) - ); - expect(updateOption).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); - expect(locationAction).toEqual(updateLocation({ query: { 'var-Constant': ['A'] } })); - expect(hideAction).toEqual(hideOptions()); - - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual( + setCurrentVariableValue(toVariablePayload(variable, { option })), + changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: '' })), + setCurrentVariableValue(toVariablePayload(variable, { option })), + updateLocation({ query: { 'var-Constant': ['A'] } }), + hideOptions() + ); }); }); describe('when navigateOptions is dispatched with navigation key select without clearOthers', () => { it('then correct actions are dispatched', async () => { const option = createOption('A', 'A', true); - const variable = createVariable({ options: [option], includeAll: false }); + const variable = createMultiVariable({ + options: [option], + current: createOption(['A'], ['A'], true), + includeAll: false, + }); const clearOthers = false; const key = NavigationKey.select; @@ -95,20 +95,18 @@ describe('options picker actions', () => { .whenActionIsDispatched(navigateOptions(NavigationKey.moveDown, false)) .whenAsyncActionIsDispatched(navigateOptions(key, clearOthers), true); - tester.thenDispatchedActionsPredicateShouldEqual(actions => { - const [toggleOptionAction] = actions; - const expectedNumberOfActions = 1; - - expect(toggleOptionAction).toEqual(toggleOption({ option, forceSelect: false, clearOthers })); - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual(toggleOption({ option, forceSelect: false, clearOthers })); }); }); describe('when navigateOptions is dispatched with navigation key select with clearOthers', () => { it('then correct actions are dispatched', async () => { const option = createOption('A', 'A', true); - const variable = createVariable({ options: [option], includeAll: false }); + const variable = createMultiVariable({ + options: [option], + current: createOption(['A'], ['A'], true), + includeAll: false, + }); const clearOthers = true; const key = NavigationKey.select; @@ -120,20 +118,14 @@ describe('options picker actions', () => { .whenActionIsDispatched(navigateOptions(NavigationKey.moveDown, clearOthers)) .whenAsyncActionIsDispatched(navigateOptions(key, clearOthers), true); - tester.thenDispatchedActionsPredicateShouldEqual(actions => { - const [toggleOptionAction] = actions; - const expectedNumberOfActions = 1; - - expect(toggleOptionAction).toEqual(toggleOption({ option, forceSelect: false, clearOthers })); - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual(toggleOption({ option, forceSelect: false, clearOthers })); }); }); describe('when navigateOptions is dispatched with navigation key select after highlighting the third option', () => { it('then correct actions are dispatched', async () => { const options = [createOption('A'), createOption('B'), createOption('C')]; - const variable = createVariable({ options, includeAll: false }); + const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const clearOthers = true; const key = NavigationKey.select; @@ -147,20 +139,14 @@ describe('options picker actions', () => { .whenActionIsDispatched(navigateOptions(NavigationKey.moveDown, clearOthers)) .whenAsyncActionIsDispatched(navigateOptions(key, clearOthers), true); - tester.thenDispatchedActionsPredicateShouldEqual(actions => { - const [toggleOptionAction] = actions; - const expectedNumberOfActions = 1; - - expect(toggleOptionAction).toEqual(toggleOption({ option: options[2], forceSelect: false, clearOthers })); - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual(toggleOption({ option: options[2], forceSelect: false, clearOthers })); }); }); describe('when navigateOptions is dispatched with navigation key select after highlighting the second option', () => { it('then correct actions are dispatched', async () => { const options = [createOption('A'), createOption('B'), createOption('C')]; - const variable = createVariable({ options, includeAll: false }); + const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const clearOthers = true; const key = NavigationKey.select; @@ -175,20 +161,14 @@ describe('options picker actions', () => { .whenActionIsDispatched(navigateOptions(NavigationKey.moveUp, clearOthers)) .whenAsyncActionIsDispatched(navigateOptions(key, clearOthers), true); - tester.thenDispatchedActionsPredicateShouldEqual(actions => { - const [toggleOptionAction] = actions; - const expectedNumberOfActions = 1; - - expect(toggleOptionAction).toEqual(toggleOption({ option: options[1], forceSelect: false, clearOthers })); - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual(toggleOption({ option: options[1], forceSelect: false, clearOthers })); }); }); describe('when navigateOptions is dispatched with navigation key selectAndClose after highlighting the second option', () => { it('then correct actions are dispatched', async () => { const options = [createOption('A'), createOption('B'), createOption('C')]; - const variable = createVariable({ options, includeAll: false }); + const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const clearOthers = false; const key = NavigationKey.selectAndClose; @@ -204,41 +184,27 @@ describe('options picker actions', () => { .whenAsyncActionIsDispatched(navigateOptions(key, clearOthers), true); const option = { - ...createOption('B'), + ...createOption(['B']), selected: true, value: ['B'], tags: [] as any[], }; - tester.thenDispatchedActionsPredicateShouldEqual(actions => { - const [ - toggleOptionAction, - setCurrentValue, - changeQueryValue, - updateOption, - locationAction, - hideAction, - ] = actions; - const expectedNumberOfActions = 6; - - 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: '' })) - ); - expect(updateOption).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); - expect(locationAction).toEqual(updateLocation({ query: { 'var-Constant': ['B'] } })); - expect(hideAction).toEqual(hideOptions()); - - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual( + toggleOption({ option: options[1], forceSelect: true, clearOthers }), + setCurrentVariableValue(toVariablePayload(variable, { option })), + changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: '' })), + setCurrentVariableValue(toVariablePayload(variable, { option })), + updateLocation({ query: { 'var-Constant': ['B'] } }), + hideOptions() + ); }); }); describe('when filterOrSearchOptions is dispatched with simple filter', () => { it('then correct actions are dispatched', async () => { const options = [createOption('A'), createOption('B'), createOption('C')]; - const variable = createVariable({ options, includeAll: false }); + const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const filter = 'A'; const tester = await reduxTester<{ templating: TemplatingState }>() @@ -247,22 +213,14 @@ describe('options picker actions', () => { .whenActionIsDispatched(showOptions(variable)) .whenAsyncActionIsDispatched(filterOrSearchOptions(filter), true); - tester.thenDispatchedActionsPredicateShouldEqual(actions => { - const [updateQueryValue, updateAndFilter] = actions; - const expectedNumberOfActions = 2; - - expect(updateQueryValue).toEqual(updateSearchQuery(filter)); - expect(updateAndFilter).toEqual(updateOptionsAndFilter(variable.options)); - - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual(updateSearchQuery(filter), updateOptionsAndFilter(variable.options)); }); }); describe('when commitChangesToVariable is dispatched with no changes', () => { it('then correct actions are dispatched', async () => { const options = [createOption('A'), createOption('B'), createOption('C')]; - const variable = createVariable({ options, includeAll: false }); + const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const tester = await reduxTester<{ templating: TemplatingState }>() .givenRootReducer(getRootReducer()) @@ -271,31 +229,26 @@ describe('options picker actions', () => { .whenAsyncActionIsDispatched(commitChangesToVariable(), true); const option = { - ...createOption(''), + ...createOption([]), selected: true, value: [] as any[], tags: [] as any[], }; - tester.thenDispatchedActionsPredicateShouldEqual(actions => { - const [setCurrentValue, changeQueryValue, hideAction] = actions; - const expectedNumberOfActions = 3; - - expect(setCurrentValue).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); - expect(changeQueryValue).toEqual( - changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: '' })) - ); - expect(hideAction).toEqual(hideOptions()); - - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual( + setCurrentVariableValue(toVariablePayload(variable, { option })), + changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: '' })), + setCurrentVariableValue(toVariablePayload(variable, { option })), + updateLocation({ query: { 'var-Constant': [] } }), + hideOptions() + ); }); }); describe('when commitChangesToVariable is dispatched with changes', () => { it('then correct actions are dispatched', async () => { const options = [createOption('A'), createOption('B'), createOption('C')]; - const variable = createVariable({ options, includeAll: false }); + const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const clearOthers = false; const tester = await reduxTester<{ templating: TemplatingState }>() @@ -307,33 +260,26 @@ describe('options picker actions', () => { .whenAsyncActionIsDispatched(commitChangesToVariable(), true); const option = { - ...createOption('A'), + ...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: '' })) - ); - expect(updateOption).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); - expect(locationAction).toEqual(updateLocation({ query: { 'var-Constant': ['A'] } })); - expect(hideAction).toEqual(hideOptions()); - - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual( + setCurrentVariableValue(toVariablePayload(variable, { option })), + changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: '' })), + setCurrentVariableValue(toVariablePayload(variable, { option })), + updateLocation({ query: { 'var-Constant': ['A'] } }), + hideOptions() + ); }); }); 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 variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const clearOthers = false; const tester = await reduxTester<{ templating: TemplatingState }>() @@ -346,33 +292,26 @@ describe('options picker actions', () => { .whenAsyncActionIsDispatched(commitChangesToVariable(), true); const option = { - ...createOption('A'), + ...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; - }); + tester.thenDispatchedActionsShouldEqual( + setCurrentVariableValue(toVariablePayload(variable, { option })), + changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: 'C' })), + setCurrentVariableValue(toVariablePayload(variable, { option })), + updateLocation({ query: { 'var-Constant': ['A'] } }), + hideOptions() + ); }); }); describe('when toggleOptionByHighlight is dispatched with changes', () => { it('then correct actions are dispatched', async () => { const options = [createOption('A'), createOption('B'), createOption('C')]; - const variable = createVariable({ options, includeAll: false }); + const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const clearOthers = false; const tester = await reduxTester<{ templating: TemplatingState }>() @@ -384,20 +323,14 @@ describe('options picker actions', () => { const option = createOption('A'); - tester.thenDispatchedActionsPredicateShouldEqual(actions => { - const [toggleOptionAction] = actions; - const expectedNumberOfActions = 1; - - expect(toggleOptionAction).toEqual(toggleOption({ option, forceSelect: false, clearOthers })); - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual(toggleOption({ option, forceSelect: false, clearOthers })); }); }); 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 variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const clearOthers = false; const tester = await reduxTester<{ templating: TemplatingState }>() @@ -414,19 +347,14 @@ describe('options picker actions', () => { 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; - }); + tester.thenDispatchedActionsShouldEqual( + toggleOption({ option: optionA, forceSelect: false, clearOthers }), + updateSearchQuery('B'), + updateOptionsAndFilter(variable.options), + moveOptionsHighlight(1), + moveOptionsHighlight(1), + toggleOption({ option: optionBC, forceSelect: false, clearOthers }) + ); }); }); @@ -434,7 +362,12 @@ describe('options picker actions', () => { it('then correct actions are dispatched', async () => { const options = [createOption('A'), createOption('B'), createOption('C')]; const tag = createTag('tag', []); - const variable = createVariable({ options, includeAll: false, tags: [tag] }); + const variable = createMultiVariable({ + options, + current: createOption(['A'], ['A'], true), + includeAll: false, + tags: [tag], + }); const tester = await reduxTester<{ templating: TemplatingState }>() .givenRootReducer(getRootReducer()) @@ -442,13 +375,7 @@ describe('options picker actions', () => { .whenActionIsDispatched(showOptions(variable)) .whenAsyncActionIsDispatched(toggleAndFetchTag(tag), true); - tester.thenDispatchedActionsPredicateShouldEqual(actions => { - const [toggleTagAction] = actions; - const expectedNumberOfActions = 1; - - expect(toggleTagAction).toEqual(toggleTag(tag)); - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual(toggleTag(tag)); }); }); @@ -457,7 +384,12 @@ describe('options picker actions', () => { const options = [createOption('A'), createOption('B'), createOption('C')]; const tag = createTag('tag'); const values = [createMetric('b')]; - const variable = createVariable({ options, includeAll: false, tags: [tag] }); + const variable = createMultiVariable({ + options, + current: createOption(['A'], ['A'], true), + includeAll: false, + tags: [tag], + }); datasource.metricFindQuery.mockReset(); // @ts-ignore strict null error TS2345: Argument of type '() => Promise<{ value: string; text: string; }[]>' is not assignable to parameter of type '() => Promise' @@ -469,23 +401,17 @@ describe('options picker actions', () => { .whenActionIsDispatched(showOptions(variable)) .whenAsyncActionIsDispatched(toggleAndFetchTag(tag), true); - tester.thenDispatchedActionsPredicateShouldEqual(actions => { - const [toggleTagAction] = actions; - const expectedNumberOfActions = 1; - - expect(toggleTagAction).toEqual(toggleTag({ ...tag, values: ['b'] })); - return actions.length === expectedNumberOfActions; - }); + tester.thenDispatchedActionsShouldEqual(toggleTag({ ...tag, values: ['b'] })); }); }); }); -function createVariable(extend?: Partial): QueryVariableModel { +function createMultiVariable(extend?: Partial): QueryVariableModel { return { type: 'query', id: '0', global: false, - current: createOption(''), + current: createOption([]), options: [], query: 'options-query', name: 'Constant', @@ -508,7 +434,7 @@ function createVariable(extend?: Partial): QueryVariableMode }; } -function createOption(text: string, value?: string, selected?: boolean) { +function createOption(text: string | string[], value?: string | string[], selected?: boolean) { const metric = createMetric(text); return { ...metric, @@ -517,7 +443,7 @@ function createOption(text: string, value?: string, selected?: boolean) { }; } -function createMetric(value: string) { +function createMetric(value: string | string[]) { return { value: value, text: value, diff --git a/public/app/features/variables/pickers/OptionsPicker/actions.ts b/public/app/features/variables/pickers/OptionsPicker/actions.ts index 19b2ac499a9..7a50ba2f09c 100644 --- a/public/app/features/variables/pickers/OptionsPicker/actions.ts +++ b/public/app/features/variables/pickers/OptionsPicker/actions.ts @@ -183,7 +183,7 @@ function mapToCurrent(picker: OptionsPickerState): VariableOption | undefined { return { value: values, - text: texts.join(' + '), + text: texts, tags: picker.tags.filter(t => t.selected), selected: true, }; diff --git a/public/app/features/variables/state/sharedReducer.test.ts b/public/app/features/variables/state/sharedReducer.test.ts index dd50ceafde5..26e977356b8 100644 --- a/public/app/features/variables/state/sharedReducer.test.ts +++ b/public/app/features/variables/state/sharedReducer.test.ts @@ -290,7 +290,7 @@ describe('sharedReducer', () => { { selected: true, text: 'A', value: 'A' }, { selected: true, text: 'B', value: 'B' }, ], - current: { selected: true, text: 'A + B', value: ['A', 'B'] }, + current: { selected: true, text: ['A', 'B'], value: ['A', 'B'] }, } as unknown) as QueryVariableModel, }); }); diff --git a/public/app/features/variables/state/sharedReducer.ts b/public/app/features/variables/state/sharedReducer.ts index 9c240e3bac5..0ac0e7c88b3 100644 --- a/public/app/features/variables/state/sharedReducer.ts +++ b/public/app/features/variables/state/sharedReducer.ts @@ -4,7 +4,7 @@ import { default as lodashDefaults } from 'lodash/defaults'; import { VariableType } from '@grafana/data'; import { VariableModel, VariableOption, VariableWithOptions } from '../types'; -import { AddVariable, ALL_VARIABLE_VALUE, getInstanceState, NEW_VARIABLE_ID, VariablePayload } from './types'; +import { AddVariable, getInstanceState, NEW_VARIABLE_ID, VariablePayload } from './types'; import { variableAdapters } from '../adapters'; import { changeVariableNameSucceeded } from '../editor/reducer'; import { Deferred } from '../../../core/utils/deferred'; @@ -124,12 +124,6 @@ const sharedReducerSlice = createSlice({ const instanceState = getInstanceState(state, action.payload.id); const current = { ...action.payload.data.option }; - if (Array.isArray(current.text) && current.text.length > 0) { - current.text = current.text.join(' + '); - } else if (Array.isArray(current.value) && current.value[0] !== ALL_VARIABLE_VALUE) { - current.text = current.value.join(' + '); - } - instanceState.current = current; instanceState.options = instanceState.options.map(option => { let selected = false;