From 6087980de0006ccd293a6a18fd3937332b16885f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Tue, 6 Apr 2021 09:21:37 +0200 Subject: [PATCH] Variables: Confirms selection before opening new picker (#32586) * Variables: Confirms selection before opening new picker * Tests: fixes e2e tests --- .../specs/variables/load-options-from-url.ts | 9 +- .../OptionsPicker/OptionPicker.test.tsx | 140 ++++++++++++++++++ .../pickers/OptionsPicker/OptionsPicker.tsx | 15 +- .../pickers/OptionsPicker/actions.test.ts | 98 ++++++++++++ .../pickers/OptionsPicker/actions.ts | 19 ++- 5 files changed, 267 insertions(+), 14 deletions(-) create mode 100644 public/app/features/variables/pickers/OptionsPicker/OptionPicker.test.tsx diff --git a/e2e/suite1/specs/variables/load-options-from-url.ts b/e2e/suite1/specs/variables/load-options-from-url.ts index 2fa8ff7f239..62533462d8b 100644 --- a/e2e/suite1/specs/variables/load-options-from-url.ts +++ b/e2e/suite1/specs/variables/load-options-from-url.ts @@ -150,14 +150,7 @@ describe('Variables - Load options from Url', () => { e2e.pages.Dashboard.SubMenu.submenuItemValueDropDownDropDown() .should('be.visible') .within(() => { - e2e().get('.variable-option').should('have.length', 0); - }); - - e2e.pages.Dashboard.SubMenu.submenuItemValueDropDownValueLinkTexts('All').should('be.visible').click(); - e2e.pages.Dashboard.SubMenu.submenuItemValueDropDownDropDown() - .should('be.visible') - .within(() => { - e2e().get('.variable-option').should('have.length', 0); + e2e().get('.variable-option').should('have.length', 10); }); }); }); diff --git a/public/app/features/variables/pickers/OptionsPicker/OptionPicker.test.tsx b/public/app/features/variables/pickers/OptionsPicker/OptionPicker.test.tsx new file mode 100644 index 00000000000..afbfeb2d4e5 --- /dev/null +++ b/public/app/features/variables/pickers/OptionsPicker/OptionPicker.test.tsx @@ -0,0 +1,140 @@ +import React from 'react'; +import { Provider } from 'react-redux'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { selectors } from '@grafana/e2e-selectors'; +import { LoadingState } from '@grafana/data'; + +import { VariablePickerProps } from '../types'; +import { QueryVariableModel } from '../../types'; +import { queryBuilder } from '../../shared/testing/builders'; +import { optionPickerFactory } from './OptionsPicker'; +import { initialState, OptionsPickerState } from './reducer'; + +interface Args { + pickerState?: Partial; + variable?: Partial; +} + +const defaultVariable = queryBuilder() + .withId('query0') + .withName('query0') + .withMulti() + .withCurrent(['A', 'C']) + .withOptions('A', 'B', 'C') + .build(); + +function setupTestContext({ pickerState = {}, variable = {} }: Args = {}) { + const v = { + ...defaultVariable, + ...variable, + }; + const onVariableChange = jest.fn(); + const props: VariablePickerProps = { + variable: v, + onVariableChange, + }; + const Picker = optionPickerFactory(); + const optionsPicker: OptionsPickerState = { ...initialState, ...pickerState }; + const dispatch = jest.fn(); + const subscribe = jest.fn(); + const getState = jest.fn().mockReturnValue({ + templating: { + variables: { + [v.id]: { ...v }, + }, + optionsPicker, + }, + }); + const store: any = { getState, dispatch, subscribe }; + const { rerender } = render( + + + + ); + return { onVariableChange, variable, rerender, dispatch }; +} + +function getSubMenu(text: string) { + return screen.getByLabelText(selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownValueLinkTexts(text)); +} + +function getOption(text: string) { + return screen.getByLabelText(selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownOptionTexts('A')); +} + +describe('OptionPicker', () => { + describe('when mounted and picker id is not set', () => { + it('should render link with correct text', () => { + setupTestContext(); + expect(getSubMenu('A + C')).toBeInTheDocument(); + }); + + it('link text should be clickable', () => { + const { dispatch } = setupTestContext(); + + dispatch.mockClear(); + userEvent.click(getSubMenu('A + C')); + expect(dispatch).toHaveBeenCalledTimes(1); + }); + }); + + describe('when mounted and picker id differs from variable id', () => { + it('should render link with correct text', () => { + setupTestContext({ + variable: defaultVariable, + pickerState: { id: 'Other' }, + }); + expect(getSubMenu('A + C')).toBeInTheDocument(); + }); + + it('link text should be clickable', () => { + const { dispatch } = setupTestContext({ + variable: defaultVariable, + pickerState: { id: 'Other' }, + }); + + dispatch.mockClear(); + userEvent.click(getSubMenu('A + C')); + expect(dispatch).toHaveBeenCalledTimes(1); + }); + }); + + describe('when mounted and variable is loading', () => { + it('should render link with correct text and loading indicator should be visible', () => { + setupTestContext({ + variable: { ...defaultVariable, state: LoadingState.Loading }, + }); + expect(getSubMenu('A + C')).toBeInTheDocument(); + expect(screen.getByLabelText(selectors.components.LoadingIndicator.icon)).toBeInTheDocument(); + }); + + it('link text should not be clickable', () => { + const { dispatch } = setupTestContext({ + variable: { ...defaultVariable, state: LoadingState.Loading }, + }); + + dispatch.mockClear(); + userEvent.click(getSubMenu('A + C')); + expect(dispatch).toHaveBeenCalledTimes(0); + }); + }); + + describe('when mounted and picker id equals the variable id', () => { + it('should render input, drop down list with correct options', () => { + setupTestContext({ + variable: defaultVariable, + pickerState: { id: defaultVariable.id, options: defaultVariable.options, multi: defaultVariable.multi }, + }); + + expect(screen.getByRole('textbox')).toBeInTheDocument(); + expect(screen.getByRole('textbox')).toHaveValue(''); + expect( + screen.getByLabelText(selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownDropDown) + ).toBeInTheDocument(); + expect(getOption('A')).toBeInTheDocument(); + expect(getOption('B')).toBeInTheDocument(); + expect(getOption('C')).toBeInTheDocument(); + }); + }); +}); diff --git a/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx b/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx index 6249fd72cd6..e3019c7ba06 100644 --- a/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx +++ b/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx @@ -5,8 +5,14 @@ import { LoadingState } from '@grafana/data'; import { StoreState } from 'app/types'; import { VariableInput } from '../shared/VariableInput'; -import { commitChangesToVariable, filterOrSearchOptions, navigateOptions, toggleAndFetchTag } from './actions'; -import { OptionsPickerState, showOptions, toggleAllOptions, toggleOption } from './reducer'; +import { + commitChangesToVariable, + filterOrSearchOptions, + navigateOptions, + openOptions, + toggleAndFetchTag, +} from './actions'; +import { OptionsPickerState, toggleAllOptions, toggleOption } from './reducer'; import { VariableOption, VariableTag, VariableWithMultiSupport, VariableWithOptions } from '../../types'; import { VariableOptions } from '../shared/VariableOptions'; import { isMulti, isQuery } from '../../guard'; @@ -20,7 +26,7 @@ export const optionPickerFactory = > => { const mapDispatchToProps = { - showOptions, + openOptions, commitChangesToVariable, filterOrSearchOptions, toggleAllOptions, @@ -40,7 +46,8 @@ export const optionPickerFactory = ; class OptionsPickerUnconnected extends PureComponent { - onShowOptions = () => this.props.showOptions(this.props.variable); + onShowOptions = () => + this.props.openOptions(toVariableIdentifier(this.props.variable), this.props.onVariableChange); onHideOptions = () => this.props.commitChangesToVariable(this.props.onVariableChange); onToggleOption = (option: VariableOption, clearOthers: boolean) => { diff --git a/public/app/features/variables/pickers/OptionsPicker/actions.test.ts b/public/app/features/variables/pickers/OptionsPicker/actions.test.ts index 2479db1406e..88f059f2509 100644 --- a/public/app/features/variables/pickers/OptionsPicker/actions.test.ts +++ b/public/app/features/variables/pickers/OptionsPicker/actions.test.ts @@ -3,6 +3,7 @@ import { getRootReducer, RootReducerType } from '../../state/helpers'; import { initialVariableModelState, QueryVariableModel, VariableRefresh, VariableSort } from '../../types'; import { hideOptions, + initialState, moveOptionsHighlight, showOptions, toggleOption, @@ -14,6 +15,7 @@ import { commitChangesToVariable, filterOrSearchOptions, navigateOptions, + openOptions, toggleAndFetchTag, toggleOptionByHighlight, } from './actions'; @@ -23,6 +25,7 @@ import { addVariable, changeVariableProp, setCurrentVariableValue } from '../../ import { variableAdapters } from '../../adapters'; import { createQueryVariableAdapter } from '../../query/adapter'; import { locationService } from '@grafana/runtime'; +import { queryBuilder } from '../../shared/testing/builders'; const datasource = { metricFindQuery: jest.fn(() => Promise.resolve([])), @@ -218,6 +221,101 @@ describe('options picker actions', () => { }); }); + describe('when openOptions is dispatched and there is no picker state yet', () => { + it('then correct actions are dispatched', async () => { + const variable = queryBuilder() + .withId('query0') + .withName('query0') + .withMulti() + .withCurrent(['A', 'C']) + .withOptions('A', 'B', 'C') + .build(); + + const preloadedState: any = { + templating: { + variables: { + [variable.id]: { ...variable }, + }, + optionsPicker: { ...initialState }, + }, + }; + + const tester = await reduxTester({ preloadedState }) + .givenRootReducer(getRootReducer()) + .whenAsyncActionIsDispatched(openOptions(variable, undefined)); + + tester.thenDispatchedActionsShouldEqual(showOptions(variable)); + }); + }); + + describe('when openOptions is dispatched and picker.id is same as variable.id', () => { + it('then correct actions are dispatched', async () => { + const variable = queryBuilder() + .withId('query0') + .withName('query0') + .withMulti() + .withCurrent(['A', 'C']) + .withOptions('A', 'B', 'C') + .build(); + + const preloadedState: any = { + templating: { + variables: { + [variable.id]: { ...variable }, + }, + optionsPicker: { ...initialState, id: variable.id }, + }, + }; + + const tester = await reduxTester({ preloadedState }) + .givenRootReducer(getRootReducer()) + .whenAsyncActionIsDispatched(openOptions(variable, undefined)); + + tester.thenDispatchedActionsShouldEqual(showOptions(variable)); + }); + }); + + describe('when openOptions is dispatched and picker.id is not the same as variable.id', () => { + it('then correct actions are dispatched', async () => { + const variableInPickerState = queryBuilder() + .withId('query1') + .withName('query1') + .withMulti() + .withCurrent(['A', 'C']) + .withOptions('A', 'B', 'C') + .build(); + + const variable = queryBuilder() + .withId('query0') + .withName('query0') + .withMulti() + .withCurrent(['A']) + .withOptions('A', 'B', 'C') + .build(); + + const preloadedState: any = { + templating: { + variables: { + [variable.id]: { ...variable }, + [variableInPickerState.id]: { ...variableInPickerState }, + }, + optionsPicker: { ...initialState, id: variableInPickerState.id }, + }, + }; + + const tester = await reduxTester({ preloadedState }) + .givenRootReducer(getRootReducer()) + .whenAsyncActionIsDispatched(openOptions(variable, undefined)); + + tester.thenDispatchedActionsShouldEqual( + setCurrentVariableValue({ type: 'query', id: 'query1', data: { option: undefined } }), + changeVariableProp({ type: 'query', id: 'query1', data: { propName: 'queryValue', propValue: '' } }), + hideOptions(), + showOptions(variable) + ); + }); + }); + describe('when commitChangesToVariable is dispatched with no changes', () => { it('then correct actions are dispatched', async () => { const options = [createOption('A', 'A', true), createOption('B'), createOption('C')]; diff --git a/public/app/features/variables/pickers/OptionsPicker/actions.ts b/public/app/features/variables/pickers/OptionsPicker/actions.ts index 585c570ab66..6fcd2f192ef 100644 --- a/public/app/features/variables/pickers/OptionsPicker/actions.ts +++ b/public/app/features/variables/pickers/OptionsPicker/actions.ts @@ -16,6 +16,7 @@ import { hideOptions, moveOptionsHighlight, OptionsPickerState, + showOptions, toggleOption, toggleTag, updateOptionsAndFilter, @@ -25,7 +26,7 @@ import { import { getDataSourceSrv } from '@grafana/runtime'; import { getTimeSrv } from 'app/features/dashboard/services/TimeSrv'; import { changeVariableProp, setCurrentVariableValue } from '../../state/sharedReducer'; -import { toVariablePayload } from '../../state/types'; +import { toVariablePayload, VariableIdentifier } from '../../state/types'; import { containsSearchFilter, getCurrentText } from '../../utils'; export const navigateOptions = (key: NavigationKey, clearOthers: boolean): ThunkResult => { @@ -98,10 +99,24 @@ export const commitChangesToVariable = (callback?: (updated: any) => void): Thun return callback(updated); } - return setVariable(updated); + return await setVariable(updated); }; }; +export const openOptions = ({ id }: VariableIdentifier, callback?: (updated: any) => void): ThunkResult => async ( + dispatch, + getState +) => { + const picker = getState().templating.optionsPicker; + + if (picker.id && picker.id !== id) { + await dispatch(commitChangesToVariable(callback)); + } + + const variable = getVariable(id, getState()); + dispatch(showOptions(variable)); +}; + export const toggleOptionByHighlight = (clearOthers: boolean, forceSelect = false): ThunkResult => { return (dispatch, getState) => { const { highlightIndex, options } = getState().templating.optionsPicker;