Variables: Fix issue with empty dropdowns on navigation (#37776)

public/app/features/variables/guard.ts

@@ -44,13 +44,21 @@ export const isMulti = (model: VariableModel): model is VariableWithMultiSupport
 };
 
 export const hasOptions = (model: VariableModel): model is VariableWithOptions => {
+  return hasObjectProperty(model, 'options');
+};
+
+export const hasCurrent = (model: VariableModel): model is VariableWithOptions => {
+  return hasObjectProperty(model, 'current');
+};
+
+function hasObjectProperty(model: VariableModel, property: string): model is VariableWithOptions {
   if (!model) {
     return false;
   }
 
-  const withOptions = model as VariableWithOptions;
-  return withOptions.hasOwnProperty('options') && typeof withOptions.options === 'object';
-};
+  const withProperty = model as Record<string, any>;
+  return withProperty.hasOwnProperty(property) && typeof withProperty[property] === 'object';
+}
 
 interface DataSourceWithLegacyVariableSupport<
   TQuery extends DataQuery = DataQuery,

public/app/features/variables/state/actions.ts

@@ -39,7 +39,14 @@ import {
 import { contextSrv } from 'app/core/services/context_srv';
 import { getTemplateSrv, TemplateSrv } from '../../templating/template_srv';
 import { alignCurrentWithMulti } from '../shared/multiOptions';
-import { hasLegacyVariableSupport, hasOptions, hasStandardVariableSupport, isMulti, isQuery } from '../guard';
+import {
+  hasCurrent,
+  hasLegacyVariableSupport,
+  hasOptions,
+  hasStandardVariableSupport,
+  isMulti,
+  isQuery,
+} from '../guard';
 import { getTimeSrv } from 'app/features/dashboard/services/TimeSrv';
 import { DashboardModel } from 'app/features/dashboard/state';
 import { createErrorNotification } from '../../../core/copy/appNotification';
@@ -51,7 +58,7 @@ import {
 } from './transactionReducer';
 import { getBackendSrv } from '../../../core/services/backend_srv';
 import { cleanVariables } from './variablesReducer';
-import { ensureStringValues, getCurrentText, getVariableRefresh } from '../utils';
+import { ensureStringValues, ExtendedUrlQueryMap, getCurrentText, getVariableRefresh } from '../utils';
 import { store } from 'app/store/store';
 import { getDatasourceSrv } from '../../plugins/datasource_srv';
 import { cleanEditorState } from '../editor/reducer';
@@ -576,21 +583,40 @@ const timeRangeUpdated = (identifier: VariableIdentifier): ThunkResult<Promise<v
   }
 };
 
-export const templateVarsChangedInUrl = (vars: UrlQueryMap): ThunkResult<void> => async (dispatch, getState) => {
+export const templateVarsChangedInUrl = (vars: ExtendedUrlQueryMap): ThunkResult<void> => async (
+  dispatch,
+  getState
+) => {
   const update: Array<Promise<any>> = [];
+  const dashboard = getState().dashboard.getModel();
   for (const variable of getVariables(getState())) {
     const key = `var-${variable.name}`;
-    if (vars.hasOwnProperty(key)) {
-      if (isVariableUrlValueDifferentFromCurrent(variable, vars[key])) {
-        const promise = variableAdapters.get(variable.type).setValueFromUrl(variable, vars[key]);
-        update.push(promise);
+    if (!vars.hasOwnProperty(key)) {
+      // key not found quick exit
+      continue;
+    }
+
+    if (!isVariableUrlValueDifferentFromCurrent(variable, vars[key].value)) {
+      // variable values doesn't differ quick exit
+      continue;
+    }
+
+    let value = vars[key].value; // as the default the value is set to the value passed into templateVarsChangedInUrl
+    if (vars[key].removed) {
+      // for some reason (panel|data link without variable) the variable url value (var-xyz) has been removed from the url
+      // so we need to revert the value to the value stored in dashboard json
+      const variableInModel = dashboard?.templating.list.find((v) => v.name === variable.name);
+      if (variableInModel && hasCurrent(variableInModel)) {
+        value = variableInModel.current.value; // revert value to the value stored in dashboard json
       }
     }
+
+    const promise = variableAdapters.get(variable.type).setValueFromUrl(variable, value);
+    update.push(promise);
   }
 
   if (update.length) {
     await Promise.all(update);
-    const dashboard = getState().dashboard.getModel();
     dashboard?.templateVariableValueUpdated();
     dashboard?.startRefresh();
   }

public/app/features/variables/state/sharedReducer.test.ts

@@ -20,9 +20,9 @@ import { ConstantVariableModel, QueryVariableModel, VariableHide, VariableOption
 import {
   ALL_VARIABLE_TEXT,
   ALL_VARIABLE_VALUE,
+  initialVariablesState,
   toVariablePayload,
   VariableIdentifier,
-  initialVariablesState,
   VariablesState,
 } from './types';
 import { variableAdapters } from '../adapters';
@@ -343,58 +343,6 @@ describe('sharedReducer', () => {
     });
   });
 
-  describe('when setCurrentVariableValue is dispatched and current.value has no value', () => {
-    it('then the first available option should be selected', () => {
-      const adapter = createQueryVariableAdapter();
-      const { initialState } = getVariableTestContext(adapter, {
-        options: [
-          { text: 'A', value: 'A', selected: false },
-          { text: 'B', value: 'B', selected: false },
-          { text: 'C', value: 'C', selected: false },
-        ],
-      });
-      const current = { text: '', value: '', selected: false };
-      const payload = toVariablePayload({ id: '0', type: 'query' }, { option: current });
-      reducerTester<VariablesState>()
-        .givenReducer(sharedReducer, cloneDeep(initialState))
-        .whenActionIsDispatched(setCurrentVariableValue(payload))
-        .thenStateShouldEqual({
-          ...initialState,
-          '0': ({
-            ...initialState[0],
-            options: [
-              { selected: true, text: 'A', value: 'A' },
-              { selected: false, text: 'B', value: 'B' },
-              { selected: false, text: 'C', value: 'C' },
-            ],
-            current: { selected: true, text: 'A', value: 'A' },
-          } as unknown) as QueryVariableModel,
-        });
-    });
-  });
-
-  describe('when setCurrentVariableValue is dispatched and current.value has no value and there are no options', () => {
-    it('then no option should be selected', () => {
-      const adapter = createQueryVariableAdapter();
-      const { initialState } = getVariableTestContext(adapter, {
-        options: [],
-      });
-      const current = { text: '', value: '', selected: false };
-      const payload = toVariablePayload({ id: '0', type: 'query' }, { option: current });
-      reducerTester<VariablesState>()
-        .givenReducer(sharedReducer, cloneDeep(initialState))
-        .whenActionIsDispatched(setCurrentVariableValue(payload))
-        .thenStateShouldEqual({
-          ...initialState,
-          '0': ({
-            ...initialState[0],
-            options: [],
-            current: { selected: false, text: '', value: '' },
-          } as unknown) as QueryVariableModel,
-        });
-    });
-  });
-
   describe('when setCurrentVariableValue is dispatched and current.value is an Array with values containing All value', () => {
     it('then state should be correct', () => {
       const adapter = createQueryVariableAdapter();

public/app/features/variables/state/sharedReducer.ts

@@ -121,15 +121,6 @@ const sharedReducerSlice = createSlice({
       const { option } = action.payload.data;
       const current = { ...option, text: ensureStringValues(option?.text), value: ensureStringValues(option?.value) };
 
-      // If no value is set, default to the first avilable
-      if (!current.value && instanceState.options.length) {
-        instanceState.options.forEach((option, index) => {
-          option.selected = !Boolean(index);
-        });
-        instanceState.current = instanceState.options[0];
-        return;
-      }
-
       instanceState.current = current;
       instanceState.options = instanceState.options.map((option) => {
         option.value = ensureStringValues(option.value);

public/app/features/variables/state/templateVarsChangedInUrl.test.ts

@@ -0,0 +1,121 @@
+import { variableAdapters } from '../adapters';
+import { customBuilder } from '../shared/testing/builders';
+import { DashboardState, StoreState } from '../../../types';
+import { initialState } from '../../dashboard/state/reducers';
+import { TemplatingState } from './reducers';
+import { ExtendedUrlQueryMap } from '../utils';
+import { templateVarsChangedInUrl } from './actions';
+import { createCustomVariableAdapter } from '../custom/adapter';
+import { VariablesState } from './types';
+import { DashboardModel } from '../../dashboard/state';
+
+variableAdapters.setInit(() => [createCustomVariableAdapter()]);
+
+async function getTestContext(urlQueryMap: ExtendedUrlQueryMap = {}) {
+  jest.clearAllMocks();
+
+  const custom = customBuilder().withId('custom').withCurrent(['A', 'C']).withOptions('A', 'B', 'C').build();
+  const setValueFromUrlMock = jest.fn();
+  variableAdapters.get('custom').setValueFromUrl = setValueFromUrlMock;
+
+  const templateVariableValueUpdatedMock = jest.fn();
+  const startRefreshMock = jest.fn();
+  const dashboardModel: Partial<DashboardModel> = {
+    templateVariableValueUpdated: templateVariableValueUpdatedMock,
+    startRefresh: startRefreshMock,
+    templating: { list: [custom] },
+  };
+  const dashboard: DashboardState = {
+    ...initialState,
+    getModel: () => (dashboardModel as unknown) as DashboardModel,
+  };
+
+  const variables: VariablesState = { custom };
+  const templating = ({ variables } as unknown) as TemplatingState;
+  const state: Partial<StoreState> = {
+    dashboard,
+    templating,
+  };
+  const getState = () => (state as unknown) as StoreState;
+
+  const dispatch = jest.fn();
+  const thunk = templateVarsChangedInUrl(urlQueryMap);
+
+  await thunk(dispatch, getState, undefined);
+
+  return { setValueFromUrlMock, templateVariableValueUpdatedMock, startRefreshMock, custom };
+}
+
+describe('templateVarsChangedInUrl', () => {
+  describe('when called with no variables in url query map', () => {
+    it('then no value should change and dashboard should not be refreshed', async () => {
+      const { setValueFromUrlMock, templateVariableValueUpdatedMock, startRefreshMock } = await getTestContext();
+
+      expect(setValueFromUrlMock).not.toHaveBeenCalled();
+      expect(templateVariableValueUpdatedMock).not.toHaveBeenCalled();
+      expect(startRefreshMock).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('when called with no variables in url query map matching variables in state', () => {
+    it('then no value should change and dashboard should not be refreshed', async () => {
+      const { setValueFromUrlMock, templateVariableValueUpdatedMock, startRefreshMock } = await getTestContext({
+        'var-query': { value: 'A' },
+      });
+
+      expect(setValueFromUrlMock).not.toHaveBeenCalled();
+      expect(templateVariableValueUpdatedMock).not.toHaveBeenCalled();
+      expect(startRefreshMock).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('when called with variables in url query map matching variables in state', () => {
+    describe('and the values in url query map are the same as current in state', () => {
+      it('then no value should change and dashboard should not be refreshed', async () => {
+        const { setValueFromUrlMock, templateVariableValueUpdatedMock, startRefreshMock } = await getTestContext({
+          'var-custom': { value: ['A', 'C'] },
+        });
+
+        expect(setValueFromUrlMock).not.toHaveBeenCalled();
+        expect(templateVariableValueUpdatedMock).not.toHaveBeenCalled();
+        expect(startRefreshMock).not.toHaveBeenCalled();
+      });
+    });
+
+    describe('and the values in url query map are the not the same as current in state', () => {
+      it('then the value should change to the value in url query map and dashboard should be refreshed', async () => {
+        const {
+          setValueFromUrlMock,
+          templateVariableValueUpdatedMock,
+          startRefreshMock,
+          custom,
+        } = await getTestContext({
+          'var-custom': { value: 'B' },
+        });
+
+        expect(setValueFromUrlMock).toHaveBeenCalledTimes(1);
+        expect(setValueFromUrlMock).toHaveBeenCalledWith(custom, 'B');
+        expect(templateVariableValueUpdatedMock).toHaveBeenCalledTimes(1);
+        expect(startRefreshMock).toHaveBeenCalledTimes(1);
+      });
+
+      describe('but the values in url query map were removed', () => {
+        it('then the value should change to the value in dashboard json and dashboard should be refreshed', async () => {
+          const {
+            setValueFromUrlMock,
+            templateVariableValueUpdatedMock,
+            startRefreshMock,
+            custom,
+          } = await getTestContext({
+            'var-custom': { value: '', removed: true },
+          });
+
+          expect(setValueFromUrlMock).toHaveBeenCalledTimes(1);
+          expect(setValueFromUrlMock).toHaveBeenCalledWith(custom, ['A', 'C']);
+          expect(templateVariableValueUpdatedMock).toHaveBeenCalledTimes(1);
+          expect(startRefreshMock).toHaveBeenCalledTimes(1);
+        });
+      });
+    });
+  });
+});

public/app/features/variables/utils.test.ts

@@ -99,8 +99,8 @@ describe('findTemplateVarChanges', () => {
       aaa: 'ignore me',
     };
 
-    expect(findTemplateVarChanges(b, a)).toEqual({ 'var-xyz': 'hello' });
-    expect(findTemplateVarChanges(a, b)).toEqual({ 'var-xyz': '' });
+    expect(findTemplateVarChanges(b, a)).toEqual({ 'var-xyz': { value: 'hello' } });
+    expect(findTemplateVarChanges(a, b)).toEqual({ 'var-xyz': { value: '', removed: true } });
   });
 
   it('then should ignore equal values', () => {
@@ -161,7 +161,7 @@ describe('findTemplateVarChanges', () => {
       'var-test': 'asd',
     };
 
-    expect(findTemplateVarChanges(a, b)!['var-test']).toEqual(['test']);
+    expect(findTemplateVarChanges(a, b)!['var-test']).toEqual({ value: ['test'] });
   });
 });
 

public/app/features/variables/utils.ts

@@ -1,5 +1,5 @@
 import { isArray, isEqual } from 'lodash';
-import { ScopedVars, UrlQueryMap, VariableType } from '@grafana/data';
+import { ScopedVars, UrlQueryMap, UrlQueryValue, VariableType } from '@grafana/data';
 import { getTemplateSrv } from '@grafana/runtime';
 
 import { ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE } from './state/types';
@@ -185,9 +185,16 @@ function getUrlValueForComparison(value: any): any {
   return value;
 }
 
-export function findTemplateVarChanges(query: UrlQueryMap, old: UrlQueryMap): UrlQueryMap | undefined {
+export interface UrlQueryType {
+  value: UrlQueryValue;
+  removed?: boolean;
+}
+
+export interface ExtendedUrlQueryMap extends Record<string, UrlQueryType> {}
+
+export function findTemplateVarChanges(query: UrlQueryMap, old: UrlQueryMap): ExtendedUrlQueryMap | undefined {
   let count = 0;
-  const changes: UrlQueryMap = {};
+  const changes: ExtendedUrlQueryMap = {};
 
   for (const key in query) {
     if (!key.startsWith('var-')) {
@@ -198,7 +205,7 @@ export function findTemplateVarChanges(query: UrlQueryMap, old: UrlQueryMap): Ur
     let newValue = getUrlValueForComparison(query[key]);
 
     if (!isEqual(newValue, oldValue)) {
-      changes[key] = query[key];
+      changes[key] = { value: query[key] };
       count++;
     }
   }
@@ -216,7 +223,7 @@ export function findTemplateVarChanges(query: UrlQueryMap, old: UrlQueryMap): Ur
     }
 
     if (!query.hasOwnProperty(key)) {
-      changes[key] = ''; // removed
+      changes[key] = { value: '', removed: true }; // removed
       count++;
     }
   }