Commit ac7af7d4 by Marcus Andersson Committed by GitHub

TemplateVariables: make sure we handle multi/single value with correct data type (#23208)

* Fixed issue with multi value.

* Made some refactorings after feedback from Torkel and Hugo.

* minor refactorings.

* changed so we don't make the current value to array if multi is false.

* added snapshot to contain v23.

* Fixed so we always use the correct type when setting value for multi/non-multi.

* added some more tests.

* added tests.

* some small adjustments after feedback
parent 6402dde6
......@@ -79,7 +79,7 @@ exports[`DashboardPage Dashboard init completed Should render dashboard grid 1`
],
"refresh": undefined,
"revision": undefined,
"schemaVersion": 22,
"schemaVersion": 23,
"snapshot": undefined,
"style": "dark",
"tags": Array [],
......@@ -194,7 +194,7 @@ exports[`DashboardPage Dashboard init completed Should render dashboard grid 1`
],
"refresh": undefined,
"revision": undefined,
"schemaVersion": 22,
"schemaVersion": 23,
"snapshot": undefined,
"style": "dark",
"tags": Array [],
......@@ -289,7 +289,7 @@ exports[`DashboardPage Dashboard init completed Should render dashboard grid 1`
],
"refresh": undefined,
"revision": undefined,
"schemaVersion": 22,
"schemaVersion": 23,
"snapshot": undefined,
"style": "dark",
"tags": Array [],
......@@ -414,7 +414,7 @@ exports[`DashboardPage When dashboard has editview url state should render setti
],
"refresh": undefined,
"revision": undefined,
"schemaVersion": 22,
"schemaVersion": 23,
"snapshot": undefined,
"style": "dark",
"tags": Array [],
......@@ -529,7 +529,7 @@ exports[`DashboardPage When dashboard has editview url state should render setti
],
"refresh": undefined,
"revision": undefined,
"schemaVersion": 22,
"schemaVersion": 23,
"snapshot": undefined,
"style": "dark",
"tags": Array [],
......@@ -624,7 +624,7 @@ exports[`DashboardPage When dashboard has editview url state should render setti
],
"refresh": undefined,
"revision": undefined,
"schemaVersion": 22,
"schemaVersion": 23,
"snapshot": undefined,
"style": "dark",
"tags": Array [],
......@@ -725,7 +725,7 @@ exports[`DashboardPage When dashboard has editview url state should render setti
],
"refresh": undefined,
"revision": undefined,
"schemaVersion": 22,
"schemaVersion": 23,
"snapshot": undefined,
"style": "dark",
"tags": Array [],
......
......@@ -241,7 +241,7 @@ exports[`DashboardGrid Can render dashboard grid Should render 1`] = `
],
"refresh": undefined,
"revision": undefined,
"schemaVersion": 22,
"schemaVersion": 23,
"snapshot": undefined,
"style": "dark",
"tags": Array [],
......@@ -489,7 +489,7 @@ exports[`DashboardGrid Can render dashboard grid Should render 1`] = `
],
"refresh": undefined,
"revision": undefined,
"schemaVersion": 22,
"schemaVersion": 23,
"snapshot": undefined,
"style": "dark",
"tags": Array [],
......@@ -737,7 +737,7 @@ exports[`DashboardGrid Can render dashboard grid Should render 1`] = `
],
"refresh": undefined,
"revision": undefined,
"schemaVersion": 22,
"schemaVersion": 23,
"snapshot": undefined,
"style": "dark",
"tags": Array [],
......@@ -985,7 +985,7 @@ exports[`DashboardGrid Can render dashboard grid Should render 1`] = `
],
"refresh": undefined,
"revision": undefined,
"schemaVersion": 22,
"schemaVersion": 23,
"snapshot": undefined,
"style": "dark",
"tags": Array [],
......
......@@ -128,7 +128,7 @@ describe('DashboardModel', () => {
});
it('dashboard schema version should be set to latest', () => {
expect(model.schemaVersion).toBe(22);
expect(model.schemaVersion).toBe(23);
});
it('graph thresholds should be migrated', () => {
......@@ -575,6 +575,53 @@ describe('DashboardModel', () => {
});
});
});
describe('when migrating variables with multi support', () => {
let model: DashboardModel;
beforeEach(() => {
model = new DashboardModel({
templating: {
list: [
{
multi: false,
current: {
value: ['value'],
text: ['text'],
},
},
{
multi: true,
current: {
value: ['value'],
text: ['text'],
},
},
],
},
});
});
it('should have two variables after migration', () => {
expect(model.templating.list.length).toBe(2);
});
it('should be migrated if being out of sync', () => {
expect(model.templating.list[0].multi).toBe(false);
expect(model.templating.list[0].current).toEqual({
text: 'text',
value: 'value',
});
});
it('should not be migrated if being in sync', () => {
expect(model.templating.list[1].multi).toBe(true);
expect(model.templating.list[1].current).toEqual({
text: ['text'],
value: ['value'],
});
});
});
});
function createRow(options: any, panelDescriptions: any[]) {
......
......@@ -18,6 +18,8 @@ import {
MIN_PANEL_HEIGHT,
} from 'app/core/constants';
import { DataLinkBuiltInVars } from '@grafana/ui';
import { isMulti } from 'app/features/variables/guard';
import { alignCurrentWithMulti } from 'app/features/variables/shared/multiOptions';
export class DashboardMigrator {
dashboard: DashboardModel;
......@@ -30,7 +32,7 @@ export class DashboardMigrator {
let i, j, k, n;
const oldVersion = this.dashboard.schemaVersion;
const panelUpgrades = [];
this.dashboard.schemaVersion = 22;
this.dashboard.schemaVersion = 23;
if (oldVersion === this.dashboard.schemaVersion) {
return;
......@@ -497,6 +499,16 @@ export class DashboardMigrator {
});
}
if (oldVersion < 23) {
for (const variable of this.dashboard.templating.list) {
if (!isMulti(variable)) {
continue;
}
const { multi, current } = variable;
variable.current = alignCurrentWithMulti(current, multi);
}
}
if (panelUpgrades.length === 0) {
return;
}
......
......@@ -2,10 +2,22 @@ import React, { ChangeEvent, FocusEvent, PureComponent } from 'react';
import { CustomVariableModel, VariableWithMultiSupport } from '../../templating/types';
import { SelectionOptionsEditor } from '../editor/SelectionOptionsEditor';
import { OnPropChangeArguments, VariableEditorProps } from '../editor/types';
import { connectWithStore } from 'app/core/utils/connectWithReduxStore';
import { MapDispatchToProps, MapStateToProps } from 'react-redux';
import { StoreState } from 'app/types';
import { changeVariableMultiValue } from '../state/actions';
export interface Props extends VariableEditorProps<CustomVariableModel> {}
interface OwnProps extends VariableEditorProps<CustomVariableModel> {}
export class CustomVariableEditor extends PureComponent<Props> {
interface ConnectedProps {}
interface DispatchProps {
changeVariableMultiValue: typeof changeVariableMultiValue;
}
export type Props = OwnProps & ConnectedProps & DispatchProps;
class CustomVariableEditorUnconnected extends PureComponent<Props> {
onChange = (event: ChangeEvent<HTMLInputElement>) => {
this.props.onPropChange({
propName: 'query',
......@@ -44,8 +56,24 @@ export class CustomVariableEditor extends PureComponent<Props> {
/>
</div>
</div>
<SelectionOptionsEditor variable={this.props.variable} onPropChange={this.onSelectionOptionsChange} />
<SelectionOptionsEditor
variable={this.props.variable}
onPropChange={this.onSelectionOptionsChange}
onMultiChanged={this.props.changeVariableMultiValue}
/>
</>
);
}
}
const mapStateToProps: MapStateToProps<ConnectedProps, OwnProps, StoreState> = (state, ownProps) => ({});
const mapDispatchToProps: MapDispatchToProps<DispatchProps, OwnProps> = {
changeVariableMultiValue,
};
export const CustomVariableEditor = connectWithStore(
CustomVariableEditorUnconnected,
mapStateToProps,
mapDispatchToProps
);
......@@ -10,6 +10,7 @@ import { initDataSourceVariableEditor } from './actions';
import { MapDispatchToProps, MapStateToProps } from 'react-redux';
import { StoreState } from '../../../types';
import { connectWithStore } from '../../../core/utils/connectWithReduxStore';
import { changeVariableMultiValue } from '../state/actions';
export interface OwnProps extends VariableEditorProps<DataSourceVariableModel> {}
......@@ -19,6 +20,7 @@ interface ConnectedProps {
interface DispatchProps {
initDataSourceVariableEditor: typeof initDataSourceVariableEditor;
changeVariableMultiValue: typeof changeVariableMultiValue;
}
type Props = OwnProps & ConnectedProps & DispatchProps;
......@@ -110,7 +112,11 @@ export class DataSourceVariableEditorUnConnected extends PureComponent<Props> {
</div>
</div>
<SelectionOptionsEditor variable={this.props.variable} onPropChange={this.onSelectionOptionsChange} />
<SelectionOptionsEditor
variable={this.props.variable}
onPropChange={this.onSelectionOptionsChange}
onMultiChanged={this.props.changeVariableMultiValue}
/>
</>
);
}
......@@ -122,6 +128,7 @@ const mapStateToProps: MapStateToProps<ConnectedProps, OwnProps, StoreState> = (
const mapDispatchToProps: MapDispatchToProps<DispatchProps, OwnProps> = {
initDataSourceVariableEditor,
changeVariableMultiValue,
};
export const DataSourceVariableEditor = connectWithStore(
......
......@@ -72,5 +72,4 @@ export const dataSourceVariableSlice = createSlice({
});
export const dataSourceVariableReducer = dataSourceVariableSlice.reducer;
export const { createDataSourceOptions } = dataSourceVariableSlice.actions;
......@@ -4,16 +4,19 @@ import { e2e } from '@grafana/e2e';
import { VariableWithMultiSupport } from '../../templating/types';
import { VariableEditorProps } from './types';
import { VariableIdentifier, toVariableIdentifier } from '../state/types';
export interface SelectionOptionsEditorProps<Model extends VariableWithMultiSupport = VariableWithMultiSupport>
extends VariableEditorProps<Model> {}
extends VariableEditorProps<Model> {
onMultiChanged: (identifier: VariableIdentifier, value: boolean) => void;
}
export const SelectionOptionsEditor: FunctionComponent<SelectionOptionsEditorProps> = props => {
const onMultiChanged = useCallback(
(event: React.ChangeEvent<HTMLInputElement>) => {
props.onPropChange({ propName: 'multi', propValue: event.target.checked });
props.onMultiChanged(toVariableIdentifier(props.variable), event.target.checked);
},
[props.onPropChange]
[props.onMultiChanged]
);
const onIncludeAllChanged = useCallback(
......
import { QueryVariableModel, VariableModel, AdHocVariableModel } from '../templating/types';
import { QueryVariableModel, VariableModel, AdHocVariableModel, VariableWithMultiSupport } from '../templating/types';
export const isQuery = (model: VariableModel): model is QueryVariableModel => {
return model.type === 'query';
......@@ -7,3 +7,8 @@ export const isQuery = (model: VariableModel): model is QueryVariableModel => {
export const isAdHoc = (model: VariableModel): model is AdHocVariableModel => {
return model.type === 'adhoc';
};
export const isMulti = (model: VariableModel): model is VariableWithMultiSupport => {
const withMulti = model as VariableWithMultiSupport;
return withMulti.hasOwnProperty('multi') && typeof withMulti.multi === 'boolean';
};
......@@ -13,6 +13,7 @@ import { MapDispatchToProps, MapStateToProps } from 'react-redux';
import { StoreState } from '../../../types';
import { connectWithStore } from '../../../core/utils/connectWithReduxStore';
import { toVariableIdentifier } from '../state/types';
import { changeVariableMultiValue } from '../state/actions';
export interface OwnProps extends VariableEditorProps<QueryVariableModel> {}
......@@ -24,6 +25,7 @@ interface DispatchProps {
initQueryVariableEditor: typeof initQueryVariableEditor;
changeQueryVariableDataSource: typeof changeQueryVariableDataSource;
changeQueryVariableQuery: typeof changeQueryVariableQuery;
changeVariableMultiValue: typeof changeVariableMultiValue;
}
type Props = OwnProps & ConnectedProps & DispatchProps;
......@@ -237,7 +239,11 @@ export class QueryVariableEditorUnConnected extends PureComponent<Props, State>
</div>
</div>
<SelectionOptionsEditor variable={this.props.variable} onPropChange={this.onSelectionOptionsChange} />
<SelectionOptionsEditor
variable={this.props.variable}
onPropChange={this.onSelectionOptionsChange}
onMultiChanged={this.props.changeVariableMultiValue}
/>
<div className="gf-form-group">
<h5>Value groups/tags (Experimental feature)</h5>
......@@ -300,6 +306,7 @@ const mapDispatchToProps: MapDispatchToProps<DispatchProps, OwnProps> = {
initQueryVariableEditor,
changeQueryVariableDataSource,
changeQueryVariableQuery,
changeVariableMultiValue,
};
export const QueryVariableEditor = connectWithStore(
......
import { VariableOption } from 'app/features/templating/types';
import { alignCurrentWithMulti } from './multiOptions';
describe('alignCurrentWithMulti', () => {
describe('when current has string array values and multi is false', () => {
it('should return current without string arrays', () => {
const current: VariableOption = {
value: ['A'],
text: ['A'],
selected: false,
};
const next = alignCurrentWithMulti(current, false);
expect(next).toEqual({
value: 'A',
text: 'A',
selected: false,
});
});
});
describe('when current has string values and multi is true', () => {
it('should return current with string arrays', () => {
const current: VariableOption = {
value: 'A',
text: 'A',
selected: false,
};
const next = alignCurrentWithMulti(current, true);
expect(next).toEqual({
value: ['A'],
text: ['A'],
selected: false,
});
});
});
describe('when current has string values and multi is false', () => {
it('should return current without string arrays', () => {
const current: VariableOption = {
value: 'A',
text: 'A',
selected: false,
};
const next = alignCurrentWithMulti(current, false);
expect(next).toEqual({
value: 'A',
text: 'A',
selected: false,
});
});
});
describe('when current has string array values and multi is true', () => {
it('should return current with string arrays', () => {
const current: VariableOption = {
value: ['A'],
text: ['A'],
selected: false,
};
const next = alignCurrentWithMulti(current, true);
expect(next).toEqual({
value: ['A'],
text: ['A'],
selected: false,
});
});
});
});
import { VariableOption } from 'app/features/templating/types';
export const alignCurrentWithMulti = (current: VariableOption, value: boolean): VariableOption => {
if (!current) {
return current;
}
if (value && !Array.isArray(current.value)) {
return {
...current,
value: convertToMulti(current.value),
text: convertToMulti(current.text),
};
}
if (!value && Array.isArray(current.value)) {
return {
...current,
value: convertToSingle(current.value),
text: convertToSingle(current.text),
};
}
return current;
};
const convertToSingle = (value: string | string[]): string => {
if (!Array.isArray(value)) {
return value;
}
if (value.length > 0) {
return value[0];
}
return '';
};
const convertToMulti = (value: string | string[]): string[] => {
if (Array.isArray(value)) {
return value;
}
return [value];
};
......@@ -9,7 +9,13 @@ import { createTextBoxVariableAdapter } from '../textbox/adapter';
import { createConstantVariableAdapter } from '../constant/adapter';
import { reduxTester } from '../../../../test/core/redux/reduxTester';
import { TemplatingState } from 'app/features/variables/state/reducers';
import { initDashboardTemplating, processVariables, setOptionFromUrl, validateVariableSelectionState } from './actions';
import {
initDashboardTemplating,
processVariables,
setOptionFromUrl,
validateVariableSelectionState,
changeVariableMultiValue,
} from './actions';
import {
addInitLock,
addVariable,
......@@ -17,6 +23,7 @@ import {
removeVariable,
resolveInitLock,
setCurrentVariableValue,
changeVariableProp,
} from './sharedReducer';
import { NEW_VARIABLE_ID, toVariableIdentifier, toVariablePayload } from './types';
import {
......@@ -136,17 +143,24 @@ describe('shared actions', () => {
describe('when setOptionFromUrl is dispatched with a custom variable (no refresh property)', () => {
it.each`
urlValue | expected
${'B'} | ${['B']}
${['B']} | ${['B']}
${'X'} | ${['X']}
${''} | ${['']}
${['A', 'B']} | ${['A', 'B']}
${null} | ${[null]}
${undefined} | ${[undefined]}
`('and urlValue is $urlValue then correct actions are dispatched', async ({ urlValue, expected }) => {
urlValue | isMulti | expected
${'B'} | ${false} | ${'B'}
${['B']} | ${false} | ${'B'}
${'X'} | ${false} | ${'X'}
${''} | ${false} | ${''}
${null} | ${false} | ${null}
${undefined} | ${false} | ${undefined}
${'B'} | ${true} | ${['B']}
${['B']} | ${true} | ${['B']}
${'X'} | ${true} | ${['X']}
${''} | ${true} | ${['']}
${['A', 'B']} | ${true} | ${['A', 'B']}
${null} | ${true} | ${[null]}
${undefined} | ${true} | ${[undefined]}
`('and urlValue is $urlValue then correct actions are dispatched', async ({ urlValue, expected, isMulti }) => {
const custom = customBuilder()
.withId('0')
.withMulti(isMulti)
.withOptions('A', 'B', 'C')
.withCurrent('A')
.build();
......@@ -439,4 +453,72 @@ describe('shared actions', () => {
});
});
});
describe('changeVariableMultiValue', () => {
describe('when changeVariableMultiValue is dispatched for variable with multi enabled', () => {
it('then correct actions are dispatched', () => {
const custom = customBuilder()
.withId('custom')
.withMulti(true)
.withCurrent(['A'], ['A'])
.build();
reduxTester<{ templating: TemplatingState }>()
.givenRootReducer(getTemplatingRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(custom, { global: false, index: 0, model: custom })))
.whenActionIsDispatched(changeVariableMultiValue(toVariableIdentifier(custom), false), true)
.thenDispatchedActionsShouldEqual(
changeVariableProp(
toVariablePayload(custom, {
propName: 'multi',
propValue: false,
})
),
changeVariableProp(
toVariablePayload(custom, {
propName: 'current',
propValue: {
value: 'A',
text: 'A',
selected: true,
},
})
)
);
});
});
describe('when changeVariableMultiValue is dispatched for variable with multi disabled', () => {
it('then correct actions are dispatched', () => {
const custom = customBuilder()
.withId('custom')
.withMulti(false)
.withCurrent(['A'], ['A'])
.build();
reduxTester<{ templating: TemplatingState }>()
.givenRootReducer(getTemplatingRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(custom, { global: false, index: 0, model: custom })))
.whenActionIsDispatched(changeVariableMultiValue(toVariableIdentifier(custom), true), true)
.thenDispatchedActionsShouldEqual(
changeVariableProp(
toVariablePayload(custom, {
propName: 'multi',
propValue: true,
})
),
changeVariableProp(
toVariablePayload(custom, {
propName: 'current',
propValue: {
value: ['A'],
text: ['A'],
selected: true,
},
})
)
);
});
});
});
});
......@@ -9,16 +9,26 @@ import {
VariableOption,
VariableRefresh,
VariableWithOptions,
VariableWithMultiSupport,
} from '../../templating/types';
import { StoreState, ThunkResult } from '../../../types';
import { getVariable, getVariables } from './selectors';
import { variableAdapters } from '../adapters';
import { Graph } from '../../../core/utils/dag';
import { updateLocation } from 'app/core/actions';
import { addInitLock, addVariable, removeInitLock, resolveInitLock, setCurrentVariableValue } from './sharedReducer';
import {
addInitLock,
addVariable,
removeInitLock,
resolveInitLock,
setCurrentVariableValue,
changeVariableProp,
} from './sharedReducer';
import { toVariableIdentifier, toVariablePayload, VariableIdentifier } from './types';
import { appEvents } from '../../../core/core';
import templateSrv from '../../templating/template_srv';
import { alignCurrentWithMulti } from '../shared/multiOptions';
import { isMulti } from '../guard';
// process flow queryVariable
// thunk => processVariables
......@@ -68,6 +78,16 @@ export const initDashboardTemplating = (list: VariableModel[]): ThunkResult<void
};
};
export const changeVariableMultiValue = (identifier: VariableIdentifier, multi: boolean): ThunkResult<void> => {
return (dispatch, getState) => {
const variable = getVariable<VariableWithMultiSupport>(identifier.id!, getState());
const current = alignCurrentWithMulti(variable.current, multi);
dispatch(changeVariableProp(toVariablePayload(identifier, { propName: 'multi', propValue: multi })));
dispatch(changeVariableProp(toVariablePayload(identifier, { propName: 'current', propValue: current })));
};
};
export const processVariableDependencies = async (variable: VariableModel, state: StoreState) => {
let dependencies: Array<Promise<any>> = [];
......@@ -173,10 +193,13 @@ export const setOptionFromUrl = (identifier: VariableIdentifier, urlValue: UrlQu
option = { text: defaultText, value: defaultValue, selected: false };
}
if (variableFromState.hasOwnProperty('multi')) {
if (isMulti(variableFromState)) {
// In case variable is multiple choice, we cast to array to preserve the same behaviour as when selecting
// the option directly, which will return even single value in an array.
option = { text: castArray(option.text), value: castArray(option.value), selected: false };
option = alignCurrentWithMulti(
{ text: castArray(option.text), value: castArray(option.value), selected: false },
variableFromState.multi
);
}
await variableAdapters.get(variable.type).setValue(variableFromState, option);
......
......@@ -132,10 +132,7 @@ describe('processVariable', () => {
await tester.thenDispatchedActionsShouldEqual(
setCurrentVariableValue(
toVariablePayload(
{ type: 'custom', id: 'custom' },
{ option: { text: ['B'], value: ['B'], selected: false } }
)
toVariablePayload({ type: 'custom', id: 'custom' }, { option: { text: 'B', value: 'B', selected: false } })
),
resolveInitLock(toVariablePayload({ type: 'custom', id: 'custom' }))
);
......@@ -218,7 +215,7 @@ describe('processVariable', () => {
setCurrentVariableValue(
toVariablePayload(
{ type: 'query', id: 'queryNoDepends' },
{ option: { text: ['B'], value: ['B'], selected: false } }
{ option: { text: 'B', value: 'B', selected: false } }
)
),
resolveInitLock(toVariablePayload({ type: 'query', id: 'queryNoDepends' }))
......@@ -263,7 +260,7 @@ describe('processVariable', () => {
setCurrentVariableValue(
toVariablePayload(
{ type: 'query', id: 'queryNoDepends' },
{ option: { text: ['B'], value: ['B'], selected: false } }
{ option: { text: 'B', value: 'B', selected: false } }
)
),
resolveInitLock(toVariablePayload({ type: 'query', id: 'queryNoDepends' }))
......@@ -364,7 +361,7 @@ describe('processVariable', () => {
setCurrentVariableValue(
toVariablePayload(
{ type: 'query', id: 'queryDependsOnCustom' },
{ option: { text: ['AB'], value: ['AB'], selected: false } }
{ option: { text: 'AB', value: 'AB', selected: false } }
)
),
resolveInitLock(toVariablePayload({ type: 'query', id: 'queryDependsOnCustom' }))
......@@ -414,7 +411,7 @@ describe('processVariable', () => {
setCurrentVariableValue(
toVariablePayload(
{ type: 'query', id: 'queryDependsOnCustom' },
{ option: { text: ['AB'], value: ['AB'], selected: false } }
{ option: { text: 'AB', value: 'AB', selected: false } }
)
),
resolveInitLock(toVariablePayload({ type: 'query', id: 'queryDependsOnCustom' }))
......
......@@ -15,13 +15,14 @@ const sharedReducerSlice = createSlice({
reducers: {
addVariable: (state: VariablesState, action: PayloadAction<VariablePayload<AddVariable>>) => {
const id = action.payload.id ?? action.payload.data.model.name; // for testing purposes we can call this with an id
state[id] = {
const variable = {
...cloneDeep(variableAdapters.get(action.payload.type).initialState),
...action.payload.data.model,
id: id,
index: action.payload.data.index,
global: action.payload.data.global,
};
state[id].id = id;
state[id].index = action.payload.data.index;
state[id].global = action.payload.data.global;
state[id] = variable;
},
addInitLock: (state: VariablesState, action: PayloadAction<VariablePayload>) => {
const instanceState = getInstanceState(state, action.payload.id!);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment