diff --git a/public/app/features/dashboard/state/DashboardModel.ts b/public/app/features/dashboard/state/DashboardModel.ts index e19bafee286..82865549c58 100644 --- a/public/app/features/dashboard/state/DashboardModel.ts +++ b/public/app/features/dashboard/state/DashboardModel.ts @@ -25,6 +25,7 @@ import { GetVariables, getVariables } from 'app/features/variables/state/selecto import { variableAdapters } from 'app/features/variables/adapters'; import { onTimeRangeUpdated } from 'app/features/variables/state/actions'; import { dispatch } from '../../../store/store'; +import { isAllVariable } from '../../variables/utils'; export interface CloneOptions { saveVariables?: boolean; @@ -651,7 +652,7 @@ export class DashboardModel { getSelectedVariableOptions(variable: any) { let selectedOptions: any[]; - if (variable.current.text === 'All') { + if (isAllVariable(variable)) { selectedOptions = variable.options.slice(1, variable.options.length); } else { selectedOptions = _.filter(variable.options, { selected: true }); diff --git a/public/app/features/variables/custom/adapter.ts b/public/app/features/variables/custom/adapter.ts index 3165484e9e0..1858f8a21cd 100644 --- a/public/app/features/variables/custom/adapter.ts +++ b/public/app/features/variables/custom/adapter.ts @@ -8,6 +8,7 @@ import { OptionsPicker } from '../pickers'; import { CustomVariableEditor } from './CustomVariableEditor'; import { updateCustomVariableOptions } from './actions'; import { ALL_VARIABLE_TEXT, toVariableIdentifier } from '../state/types'; +import { isAllVariable } from '../utils'; export const createCustomVariableAdapter = (): VariableAdapter => { return { @@ -35,7 +36,7 @@ export const createCustomVariableAdapter = (): VariableAdapter { - if (variable.current.text === ALL_VARIABLE_TEXT) { + if (isAllVariable(variable)) { return ALL_VARIABLE_TEXT; } return variable.current.value; diff --git a/public/app/features/variables/datasource/adapter.ts b/public/app/features/variables/datasource/adapter.ts index 6428266dded..40fb9d29fb0 100644 --- a/public/app/features/variables/datasource/adapter.ts +++ b/public/app/features/variables/datasource/adapter.ts @@ -8,7 +8,7 @@ import { OptionsPicker } from '../pickers'; import { ALL_VARIABLE_TEXT, toVariableIdentifier } from '../state/types'; import { DataSourceVariableEditor } from './DataSourceVariableEditor'; import { updateDataSourceVariableOptions } from './actions'; -import { containsVariable } from '../utils'; +import { containsVariable, isAllVariable } from '../utils'; export const createDataSourceVariableAdapter = (): VariableAdapter => { return { @@ -39,7 +39,7 @@ export const createDataSourceVariableAdapter = (): VariableAdapter { - if (variable.current.text === ALL_VARIABLE_TEXT) { + if (isAllVariable(variable)) { return ALL_VARIABLE_TEXT; } return variable.current.value; diff --git a/public/app/features/variables/pickers/OptionsPicker/actions.test.ts b/public/app/features/variables/pickers/OptionsPicker/actions.test.ts index 80452394703..3daf82e7f28 100644 --- a/public/app/features/variables/pickers/OptionsPicker/actions.test.ts +++ b/public/app/features/variables/pickers/OptionsPicker/actions.test.ts @@ -69,8 +69,6 @@ describe('options picker actions', () => { tester.thenDispatchedActionsShouldEqual( setCurrentVariableValue(toVariablePayload(variable, { option })), changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: '' })), - setCurrentVariableValue(toVariablePayload(variable, { option })), - updateLocation({ query: { 'var-Constant': ['A'] } }), hideOptions() ); }); @@ -219,7 +217,7 @@ describe('options picker actions', () => { describe('when commitChangesToVariable is dispatched with no changes', () => { it('then correct actions are dispatched', async () => { - const options = [createOption('A'), createOption('B'), createOption('C')]; + const options = [createOption('A', 'A', true), createOption('B'), createOption('C')]; const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const tester = await reduxTester<{ templating: TemplatingState }>() @@ -228,10 +226,39 @@ describe('options picker actions', () => { .whenActionIsDispatched(showOptions(variable)) .whenAsyncActionIsDispatched(commitChangesToVariable(), true); + const option = { + ...createOption(['A']), + selected: true, + value: ['A'] as any[], + tags: [] as any[], + }; + + tester.thenDispatchedActionsShouldEqual( + setCurrentVariableValue(toVariablePayload(variable, { option })), + changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: '' })), + hideOptions() + ); + }); + }); + + describe('when commitChangesToVariable is dispatched with changes', () => { + it('then correct actions are dispatched', async () => { + const options = [createOption('A', 'A', true), createOption('B'), createOption('C')]; + const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), 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)) + .whenAsyncActionIsDispatched(commitChangesToVariable(), true); + const option = { ...createOption([]), selected: true, - value: [] as any[], + value: [], tags: [] as any[], }; @@ -245,40 +272,9 @@ describe('options picker actions', () => { }); }); - describe('when commitChangesToVariable is dispatched with changes', () => { - it('then correct actions are dispatched', async () => { - const options = [createOption('A'), createOption('B'), createOption('C')]; - const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), 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)) - .whenAsyncActionIsDispatched(commitChangesToVariable(), true); - - const option = { - ...createOption(['A']), - selected: true, - value: ['A'], - tags: [] as any[], - }; - - 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 options = [createOption('A', 'A', true), createOption('B'), createOption('C')]; const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const clearOthers = false; @@ -292,9 +288,9 @@ describe('options picker actions', () => { .whenAsyncActionIsDispatched(commitChangesToVariable(), true); const option = { - ...createOption(['A']), + ...createOption([]), selected: true, - value: ['A'], + value: [], tags: [] as any[], }; @@ -302,7 +298,7 @@ describe('options picker actions', () => { setCurrentVariableValue(toVariablePayload(variable, { option })), changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: 'C' })), setCurrentVariableValue(toVariablePayload(variable, { option })), - updateLocation({ query: { 'var-Constant': ['A'] } }), + updateLocation({ query: { 'var-Constant': [] } }), hideOptions() ); }); diff --git a/public/app/features/variables/pickers/OptionsPicker/actions.ts b/public/app/features/variables/pickers/OptionsPicker/actions.ts index 7a50ba2f09c..657e1fd6081 100644 --- a/public/app/features/variables/pickers/OptionsPicker/actions.ts +++ b/public/app/features/variables/pickers/OptionsPicker/actions.ts @@ -26,7 +26,7 @@ import { getDataSourceSrv } from '@grafana/runtime'; import { getTimeSrv } from 'app/features/dashboard/services/TimeSrv'; import { changeVariableProp, setCurrentVariableValue } from '../../state/sharedReducer'; import { toVariablePayload } from '../../state/types'; -import { containsSearchFilter } from '../../utils'; +import { containsSearchFilter, getCurrentText } from '../../utils'; export const navigateOptions = (key: NavigationKey, clearOthers: boolean): ThunkResult => { return async (dispatch, getState) => { @@ -83,7 +83,7 @@ export const commitChangesToVariable = (): ThunkResult => { dispatch(changeVariableProp(toVariablePayload(existing, searchQueryPayload))); const updated = getVariable(picker.id, getState()); - if (existing.current.text === updated.current.text) { + if (getCurrentText(existing) === getCurrentText(updated)) { return dispatch(hideOptions()); } diff --git a/public/app/features/variables/query/adapter.ts b/public/app/features/variables/query/adapter.ts index 959a9173afc..c6f6a01696f 100644 --- a/public/app/features/variables/query/adapter.ts +++ b/public/app/features/variables/query/adapter.ts @@ -9,7 +9,7 @@ import { OptionsPicker } from '../pickers'; import { QueryVariableEditor } from './QueryVariableEditor'; import { updateQueryVariableOptions } from './actions'; import { ALL_VARIABLE_TEXT, toVariableIdentifier } from '../state/types'; -import { containsVariable } from '../utils'; +import { containsVariable, isAllVariable } from '../utils'; export const createQueryVariableAdapter = (): VariableAdapter => { return { @@ -42,7 +42,7 @@ export const createQueryVariableAdapter = (): VariableAdapter { - if (variable.current.text === ALL_VARIABLE_TEXT) { + if (isAllVariable(variable)) { return ALL_VARIABLE_TEXT; } return variable.current.value; diff --git a/public/app/features/variables/state/actions.ts b/public/app/features/variables/state/actions.ts index 32e80cf2d78..5cd4d2b50da 100644 --- a/public/app/features/variables/state/actions.ts +++ b/public/app/features/variables/state/actions.ts @@ -45,6 +45,7 @@ import { import { getBackendSrv } from '../../../core/services/backend_srv'; import { cleanVariables } from './variablesReducer'; import isEqual from 'lodash/isEqual'; +import { getCurrentText } from '../utils'; // process flow queryVariable // thunk => processVariables @@ -359,7 +360,8 @@ export const validateVariableSelectionState = ( let option: VariableOption | undefined | null = null; // 1. find the current value - option = variableInState.options?.find(v => v.text === current.text); + const text = getCurrentText(variableInState); + option = variableInState.options?.find(v => v.text === text); if (option) { return setValue(variableInState, option); } diff --git a/public/app/features/variables/utils.test.ts b/public/app/features/variables/utils.test.ts new file mode 100644 index 00000000000..2619fb8bcbd --- /dev/null +++ b/public/app/features/variables/utils.test.ts @@ -0,0 +1,49 @@ +import { getCurrentText, isAllVariable } from './utils'; + +describe('isAllVariable', () => { + it.each` + variable | expected + ${null} | ${false} + ${undefined} | ${false} + ${{}} | ${false} + ${{ current: {} }} | ${false} + ${{ current: { text: '' } }} | ${false} + ${{ current: { text: null } }} | ${false} + ${{ current: { text: undefined } }} | ${false} + ${{ current: { text: 'Alll' } }} | ${false} + ${{ current: { text: 'All' } }} | ${true} + ${{ current: { text: [] } }} | ${false} + ${{ current: { text: [null] } }} | ${false} + ${{ current: { text: [undefined] } }} | ${false} + ${{ current: { text: ['Alll'] } }} | ${false} + ${{ current: { text: ['Alll', 'All'] } }} | ${false} + ${{ current: { text: ['All'] } }} | ${true} + ${{ current: { text: { prop1: 'test' } } }} | ${false} + `("when called with params: 'variable': '$variable' then result should be '$expected'", ({ variable, expected }) => { + expect(isAllVariable(variable)).toEqual(expected); + }); +}); + +describe('getCurrentText', () => { + it.each` + variable | expected + ${null} | ${''} + ${undefined} | ${''} + ${{}} | ${''} + ${{ current: {} }} | ${''} + ${{ current: { text: '' } }} | ${''} + ${{ current: { text: null } }} | ${''} + ${{ current: { text: undefined } }} | ${''} + ${{ current: { text: 'A' } }} | ${'A'} + ${{ current: { text: 'All' } }} | ${'All'} + ${{ current: { text: [] } }} | ${''} + ${{ current: { text: [null] } }} | ${''} + ${{ current: { text: [undefined] } }} | ${''} + ${{ current: { text: ['A'] } }} | ${'A'} + ${{ current: { text: ['A', 'All'] } }} | ${'A,All'} + ${{ current: { text: ['All'] } }} | ${'All'} + ${{ current: { text: { prop1: 'test' } } }} | ${''} + `("when called with params: 'variable': '$variable' then result should be '$expected'", ({ variable, expected }) => { + expect(getCurrentText(variable)).toEqual(expected); + }); +}); diff --git a/public/app/features/variables/utils.ts b/public/app/features/variables/utils.ts index e50915e0b45..80bacd6e6a3 100644 --- a/public/app/features/variables/utils.ts +++ b/public/app/features/variables/utils.ts @@ -1,5 +1,6 @@ import isString from 'lodash/isString'; import { ScopedVars } from '@grafana/data'; +import { ALL_VARIABLE_TEXT } from './state/types'; /* * This regex matches 3 types of variable reference with an optional format specifier @@ -58,3 +59,47 @@ export function containsVariable(...args: any[]) { return !!isMatchingVariable; } + +export const isAllVariable = (variable: any): boolean => { + if (!variable) { + return false; + } + + if (!variable.current) { + return false; + } + + if (!variable.current.text) { + return false; + } + + if (Array.isArray(variable.current.text)) { + return variable.current.text.length ? variable.current.text[0] === ALL_VARIABLE_TEXT : false; + } + + return variable.current.text === ALL_VARIABLE_TEXT; +}; + +export const getCurrentText = (variable: any): string => { + if (!variable) { + return ''; + } + + if (!variable.current) { + return ''; + } + + if (!variable.current.text) { + return ''; + } + + if (Array.isArray(variable.current.text)) { + return variable.current.text.toString(); + } + + if (typeof variable.current.text !== 'string') { + return ''; + } + + return variable.current.text; +};