diff --git a/public/app/features/variables/guard.ts b/public/app/features/variables/guard.ts index 19b2e281690..3ffca2a248c 100644 --- a/public/app/features/variables/guard.ts +++ b/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; + return withProperty.hasOwnProperty(property) && typeof withProperty[property] === 'object'; +} interface DataSourceWithLegacyVariableSupport< TQuery extends DataQuery = DataQuery, diff --git a/public/app/features/variables/state/actions.ts b/public/app/features/variables/state/actions.ts index 6bc96693666..b251e08f9a6 100644 --- a/public/app/features/variables/state/actions.ts +++ b/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 => async (dispatch, getState) => { +export const templateVarsChangedInUrl = (vars: ExtendedUrlQueryMap): ThunkResult => async ( + dispatch, + getState +) => { const update: Array> = []; + 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(); } diff --git a/public/app/features/variables/state/sharedReducer.test.ts b/public/app/features/variables/state/sharedReducer.test.ts index bf3a7300152..593d3d51c80 100644 --- a/public/app/features/variables/state/sharedReducer.test.ts +++ b/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() - .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() - .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(); diff --git a/public/app/features/variables/state/sharedReducer.ts b/public/app/features/variables/state/sharedReducer.ts index 9ed2956aba2..b6fb2a5b25a 100644 --- a/public/app/features/variables/state/sharedReducer.ts +++ b/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); diff --git a/public/app/features/variables/state/templateVarsChangedInUrl.test.ts b/public/app/features/variables/state/templateVarsChangedInUrl.test.ts new file mode 100644 index 00000000000..7a68a5b2b4b --- /dev/null +++ b/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 = { + 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 = { + 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); + }); + }); + }); + }); +}); diff --git a/public/app/features/variables/utils.test.ts b/public/app/features/variables/utils.test.ts index 188def5efed..4b80c9af250 100644 --- a/public/app/features/variables/utils.test.ts +++ b/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'] }); }); }); diff --git a/public/app/features/variables/utils.ts b/public/app/features/variables/utils.ts index cbcd37ef1c9..f0f0882c90a 100644 --- a/public/app/features/variables/utils.ts +++ b/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 {} + +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++; } }