Variables: Fixes URL values for dependent variables (#28798)

public/app/features/variables/shared/testing/multiVariableBuilder.ts

@@ -6,4 +6,8 @@ export class MultiVariableBuilder<T extends VariableWithMultiSupport> extends Op
     this.variable.multi = multi;
     return this;
   }
+  withIncludeAll(includeAll = true) {
+    this.variable.includeAll = includeAll;
+    return this;
+  }
 }

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

@@ -25,9 +25,16 @@ import {
   removeVariable,
   setCurrentVariableValue,
   variableStateCompleted,
+  variableStateFetching,
   variableStateNotStarted,
 } from './sharedReducer';
-import { NEW_VARIABLE_ID, toVariableIdentifier, toVariablePayload } from './types';
+import {
+  ALL_VARIABLE_TEXT,
+  ALL_VARIABLE_VALUE,
+  NEW_VARIABLE_ID,
+  toVariableIdentifier,
+  toVariablePayload,
+} from './types';
 import {
   constantBuilder,
   customBuilder,
@@ -52,6 +59,8 @@ import {
 import { initialState } from '../pickers/OptionsPicker/reducer';
 import { cleanVariables } from './variablesReducer';
 import { expect } from '../../../../test/lib/common';
+import { VariableRefresh } from '../types';
+import { updateVariableOptions } from '../query/reducer';
 
 variableAdapters.setInit(() => [
   createQueryVariableAdapter(),
@@ -60,12 +69,26 @@ variableAdapters.setInit(() => [
   createConstantVariableAdapter(),
 ]);
 
+const metricFindQuery = jest
+  .fn()
+  .mockResolvedValueOnce([{ text: 'responses' }, { text: 'timers' }])
+  .mockResolvedValue([{ text: '200' }, { text: '500' }]);
+const getMetricSources = jest.fn().mockReturnValue([]);
+const getDatasource = jest.fn().mockResolvedValue({ metricFindQuery });
+
 jest.mock('app/features/dashboard/services/TimeSrv', () => ({
   getTimeSrv: () => ({
     timeRange: jest.fn().mockReturnValue(undefined),
   }),
 }));
 
+jest.mock('app/features/plugins/datasource_srv', () => ({
+  getDatasourceSrv: jest.fn(() => ({
+    get: getDatasource,
+    getMetricSources,
+  })),
+}));
+
 describe('shared actions', () => {
   describe('when initDashboardTemplating is dispatched', () => {
     it('then correct actions are dispatched', () => {
@@ -153,6 +176,69 @@ describe('shared actions', () => {
         return true;
       });
     });
+
+    // Fix for https://github.com/grafana/grafana/issues/28791
+    it('fix for https://github.com/grafana/grafana/issues/28791', async () => {
+      const stats = queryBuilder()
+        .withId('stats')
+        .withName('stats')
+        .withQuery('stats.*')
+        .withRefresh(VariableRefresh.onDashboardLoad)
+        .withCurrent(['response'], ['response'])
+        .withMulti()
+        .withIncludeAll()
+        .build();
+
+      const substats = queryBuilder()
+        .withId('substats')
+        .withName('substats')
+        .withQuery('stats.$stats.*')
+        .withRefresh(VariableRefresh.onDashboardLoad)
+        .withCurrent([ALL_VARIABLE_TEXT], [ALL_VARIABLE_VALUE])
+        .withMulti()
+        .withIncludeAll()
+        .build();
+
+      const list = [stats, substats];
+      const query = { orgId: '1', 'var-stats': 'response', 'var-substats': ALL_VARIABLE_TEXT };
+      const tester = await reduxTester<{ templating: TemplatingState; location: { query: UrlQueryMap } }>({
+        preloadedState: { templating: ({} as unknown) as TemplatingState, location: { query } },
+        debug: true,
+      })
+        .givenRootReducer(getTemplatingAndLocationRootReducer())
+        .whenActionIsDispatched(variablesInitTransaction({ uid: '' }))
+        .whenActionIsDispatched(initDashboardTemplating(list))
+        .whenAsyncActionIsDispatched(processVariables(), true);
+
+      await tester.thenDispatchedActionsShouldEqual(
+        variableStateFetching(toVariablePayload(stats)),
+        updateVariableOptions(
+          toVariablePayload(stats, { results: [{ text: 'responses' }, { text: 'timers' }], templatedRegex: '' })
+        ),
+        setCurrentVariableValue(
+          toVariablePayload(stats, { option: { text: ALL_VARIABLE_TEXT, value: ALL_VARIABLE_VALUE, selected: false } })
+        ),
+        variableStateCompleted(toVariablePayload(stats)),
+        setCurrentVariableValue(
+          toVariablePayload(stats, { option: { text: ['response'], value: ['response'], selected: false } })
+        ),
+        variableStateFetching(toVariablePayload(substats)),
+        updateVariableOptions(
+          toVariablePayload(substats, { results: [{ text: '200' }, { text: '500' }], templatedRegex: '' })
+        ),
+        setCurrentVariableValue(
+          toVariablePayload(substats, {
+            option: { text: [ALL_VARIABLE_TEXT], value: [ALL_VARIABLE_VALUE], selected: true },
+          })
+        ),
+        variableStateCompleted(toVariablePayload(substats)),
+        setCurrentVariableValue(
+          toVariablePayload(substats, {
+            option: { text: [ALL_VARIABLE_TEXT], value: [ALL_VARIABLE_VALUE], selected: false },
+          })
+        )
+      );
+    });
   });
 
   describe('when setOptionFromUrl is dispatched with a custom variable (no refresh property)', () => {

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

@@ -46,7 +46,7 @@ import {
 import { getBackendSrv } from '../../../core/services/backend_srv';
 import { cleanVariables } from './variablesReducer';
 import isEqual from 'lodash/isEqual';
-import { getCurrentText } from '../utils';
+import { getCurrentText, getVariableRefresh } from '../utils';
 import { store } from 'app/store/store';
 
 // process flow queryVariable
@@ -271,7 +271,7 @@ export const setOptionFromUrl = (
 ): ThunkResult<Promise<void>> => {
   return async (dispatch, getState) => {
     const variable = getVariable(identifier.id, getState());
-    if (variable.hasOwnProperty('refresh') && (variable as QueryVariableModel).refresh !== VariableRefresh.never) {
+    if (getVariableRefresh(variable) !== VariableRefresh.never) {
       // updates options
       await dispatch(updateOptions(toVariableIdentifier(variable)));
     }
@@ -447,8 +447,10 @@ export const variableUpdated = (
 
     // if we're initializing variables ignore cascading update because we are in a boot up scenario
     if (getState().templating.transaction.status === TransactionStatus.Fetching) {
-      // for all variable types with updates that go the setValueFromUrl path in the update let's make sure their state is set to Done.
-      dispatch(completeVariableLoading(identifier));
+      if (getVariableRefresh(variableInState) === VariableRefresh.never) {
+        // for variable types with updates that go the setValueFromUrl path in the update let's make sure their state is set to Done.
+        dispatch(completeVariableLoading(identifier));
+      }
       return Promise.resolve();
     }
 

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

-import { getCurrentText, isAllVariable } from './utils';
+import { getCurrentText, getVariableRefresh, isAllVariable } from './utils';
+import { VariableRefresh } from './types';
 
 describe('isAllVariable', () => {
   it.each`
@@ -47,3 +48,18 @@ describe('getCurrentText', () => {
     expect(getCurrentText(variable)).toEqual(expected);
   });
 });
+
+describe('getVariableRefresh', () => {
+  it.each`
+    variable                                           | expected
+    ${null}                                            | ${VariableRefresh.never}
+    ${undefined}                                       | ${VariableRefresh.never}
+    ${{}}                                              | ${VariableRefresh.never}
+    ${{ refresh: VariableRefresh.never }}              | ${VariableRefresh.never}
+    ${{ refresh: VariableRefresh.onTimeRangeChanged }} | ${VariableRefresh.onTimeRangeChanged}
+    ${{ refresh: VariableRefresh.onDashboardLoad }}    | ${VariableRefresh.onDashboardLoad}
+    ${{ refresh: 'invalid' }}                          | ${VariableRefresh.never}
+  `("when called with params: 'variable': '$variable' then result should be '$expected'", ({ variable, expected }) => {
+    expect(getVariableRefresh(variable)).toEqual(expected);
+  });
+});

public/app/features/variables/utils.ts

 import isString from 'lodash/isString';
 import { ScopedVars } from '@grafana/data';
 import { ALL_VARIABLE_TEXT } from './state/types';
+import { QueryVariableModel, VariableModel, VariableRefresh } from './types';
 
 /*
  * This regex matches 3 types of variable reference with an optional format specifier
@@ -103,3 +104,21 @@ export const getCurrentText = (variable: any): string => {
 
   return variable.current.text;
 };
+
+export function getVariableRefresh(variable: VariableModel): VariableRefresh {
+  if (!variable || !variable.hasOwnProperty('refresh')) {
+    return VariableRefresh.never;
+  }
+
+  const queryVariable = variable as QueryVariableModel;
+
+  if (
+    queryVariable.refresh !== VariableRefresh.onTimeRangeChanged &&
+    queryVariable.refresh !== VariableRefresh.onDashboardLoad &&
+    queryVariable.refresh !== VariableRefresh.never
+  ) {
+    return VariableRefresh.never;
+  }
+
+  return queryVariable.refresh;
+}