From 50b3409474a0f680c60ac0ea2e1a4c38fb0ef91e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Wed, 4 Nov 2020 08:56:02 +0100 Subject: [PATCH] Variables: Fixes URL values for dependent variables (#28798) --- .../shared/testing/multiVariableBuilder.ts | 4 + .../features/variables/state/actions.test.ts | 88 ++++++++++++++++++- .../app/features/variables/state/actions.ts | 10 ++- public/app/features/variables/utils.test.ts | 18 +++- public/app/features/variables/utils.ts | 19 ++++ 5 files changed, 133 insertions(+), 6 deletions(-) diff --git a/public/app/features/variables/shared/testing/multiVariableBuilder.ts b/public/app/features/variables/shared/testing/multiVariableBuilder.ts index 2bc67bef70d..3e1b1d5861f 100644 --- a/public/app/features/variables/shared/testing/multiVariableBuilder.ts +++ b/public/app/features/variables/shared/testing/multiVariableBuilder.ts @@ -6,4 +6,8 @@ export class MultiVariableBuilder extends Op this.variable.multi = multi; return this; } + withIncludeAll(includeAll = true) { + this.variable.includeAll = includeAll; + return this; + } } diff --git a/public/app/features/variables/state/actions.test.ts b/public/app/features/variables/state/actions.test.ts index 5f373882bd7..50885bf7a5c 100644 --- a/public/app/features/variables/state/actions.test.ts +++ b/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)', () => { diff --git a/public/app/features/variables/state/actions.ts b/public/app/features/variables/state/actions.ts index c717cc6144f..a68c4cb595d 100644 --- a/public/app/features/variables/state/actions.ts +++ b/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> => { 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(); } diff --git a/public/app/features/variables/utils.test.ts b/public/app/features/variables/utils.test.ts index 2619fb8bcbd..57c22152334 100644 --- a/public/app/features/variables/utils.test.ts +++ b/public/app/features/variables/utils.test.ts @@ -1,4 +1,5 @@ -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); + }); +}); diff --git a/public/app/features/variables/utils.ts b/public/app/features/variables/utils.ts index 80bacd6e6a3..c731224b2dd 100644 --- a/public/app/features/variables/utils.ts +++ b/public/app/features/variables/utils.ts @@ -1,6 +1,7 @@ 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; +}