Variables: Fixes issue with All variable not being resolved (#27151)

* Variables: Fixes issue with All variable not being resolved

* Tests: update tests according to changes

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 });

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<CustomVariableModel> => {
   return {
@@ -35,7 +36,7 @@ export const createCustomVariableAdapter = (): VariableAdapter<CustomVariableMod
       return rest;
     },
     getValueForUrl: variable => {
-      if (variable.current.text === ALL_VARIABLE_TEXT) {
+      if (isAllVariable(variable)) {
         return ALL_VARIABLE_TEXT;
       }
       return variable.current.value;

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<DataSourceVariableModel> => {
   return {
@@ -39,7 +39,7 @@ export const createDataSourceVariableAdapter = (): VariableAdapter<DataSourceVar
       return { ...rest, options: [] };
     },
     getValueForUrl: variable => {
-      if (variable.current.text === ALL_VARIABLE_TEXT) {
+      if (isAllVariable(variable)) {
         return ALL_VARIABLE_TEXT;
       }
       return variable.current.value;

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 }>()
@@ -229,17 +227,15 @@ describe('options picker actions', () => {
         .whenAsyncActionIsDispatched(commitChangesToVariable(), true);
 
       const option = {
-        ...createOption([]),
+        ...createOption(['A']),
         selected: true,
-        value: [] as any[],
+        value: ['A'] as any[],
         tags: [] as any[],
       };
 
       tester.thenDispatchedActionsShouldEqual(
         setCurrentVariableValue(toVariablePayload(variable, { option })),
         changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: '' })),
-        setCurrentVariableValue(toVariablePayload(variable, { option })),
-        updateLocation({ query: { 'var-Constant': [] } }),
         hideOptions()
       );
     });
@@ -247,7 +243,7 @@ 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 options = [createOption('A', 'A', true), createOption('B'), createOption('C')];
       const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false });
       const clearOthers = false;
 
@@ -260,9 +256,9 @@ describe('options picker actions', () => {
         .whenAsyncActionIsDispatched(commitChangesToVariable(), true);
 
       const option = {
-        ...createOption(['A']),
+        ...createOption([]),
         selected: true,
-        value: ['A'],
+        value: [],
         tags: [] as any[],
       };
 
@@ -270,7 +266,7 @@ describe('options picker actions', () => {
         setCurrentVariableValue(toVariablePayload(variable, { option })),
         changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: '' })),
         setCurrentVariableValue(toVariablePayload(variable, { option })),
-        updateLocation({ query: { 'var-Constant': ['A'] } }),
+        updateLocation({ query: { 'var-Constant': [] } }),
         hideOptions()
       );
     });
@@ -278,7 +274,7 @@ 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 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()
       );
     });

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<void> => {
   return async (dispatch, getState) => {
@@ -83,7 +83,7 @@ export const commitChangesToVariable = (): ThunkResult<void> => {
     dispatch(changeVariableProp(toVariablePayload(existing, searchQueryPayload)));
     const updated = getVariable<VariableWithMultiSupport>(picker.id, getState());
 
-    if (existing.current.text === updated.current.text) {
+    if (getCurrentText(existing) === getCurrentText(updated)) {
       return dispatch(hideOptions());
     }
 

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<QueryVariableModel> => {
   return {
@@ -42,7 +42,7 @@ export const createQueryVariableAdapter = (): VariableAdapter<QueryVariableModel
       return rest;
     },
     getValueForUrl: variable => {
-      if (variable.current.text === ALL_VARIABLE_TEXT) {
+      if (isAllVariable(variable)) {
         return ALL_VARIABLE_TEXT;
       }
       return variable.current.value;

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);
     }

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);
+  });
+});

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;
+};