From 696a6ecd1e1edec402f52400196af0aefa986145 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Fri, 30 Apr 2021 11:17:35 +0200 Subject: [PATCH] Variables: Removes the never refresh option (#33533) * Variables: Removes the never refresh option * Tests: fixes DashboardModel repeat tests * Tests: fixs snapshots * Tests: fixes processVariable test * Tests: fixes DashboardModel tests --- .../variable-types/add-query-variable.md | 1 - .../specs/variables/new-query-variable.ts | 2 +- .../__snapshots__/DashboardPage.test.tsx.snap | 14 +- .../__snapshots__/DashboardGrid.test.tsx.snap | 8 +- .../dashboard/state/DashboardMigrator.test.ts | 145 +++++++++++++++++- .../dashboard/state/DashboardMigrator.ts | 18 ++- .../state/DashboardModel.repeat.test.ts | 10 ++ .../dashboard/state/DashboardModel.test.ts | 9 +- .../query/QueryVariableRefreshSelect.tsx | 1 - .../app/features/variables/query/reducer.ts | 2 +- .../features/variables/state/actions.test.ts | 17 +- public/app/features/variables/types.ts | 2 +- 12 files changed, 208 insertions(+), 21 deletions(-) diff --git a/docs/sources/variables/variable-types/add-query-variable.md b/docs/sources/variables/variable-types/add-query-variable.md index 42c17705d67..47f7a17ae80 100644 --- a/docs/sources/variables/variable-types/add-query-variable.md +++ b/docs/sources/variables/variable-types/add-query-variable.md @@ -30,7 +30,6 @@ Query expressions are different for each data source. For more information, refe 1. In the **Data source** list, select the target data source for the query. For more information about data sources, refer to [Add a data source]({{< relref "../../datasources/add-a-data-source.md" >}}). 1. In the **Refresh** list, select when the variable should update options. - - **Never -** Variables queries are cached and values are not updated. This is fine if the values never change, but problematic if they are dynamic and change a lot. - **On Dashboard Load -** Queries the data source every time the dashboard loads. This slows down dashboard loading, because the variable query needs to be completed before dashboard can be initialized. - **On Time Range Change -** Queries the data source when the dashboard time range changes. Only use this option if your variable options query contains a time range filter or is dependent on the dashboard time range. 1. In the **Query** field, enter a query. diff --git a/e2e/suite1/specs/variables/new-query-variable.ts b/e2e/suite1/specs/variables/new-query-variable.ts index 8df5486a240..d214b224c81 100644 --- a/e2e/suite1/specs/variables/new-query-variable.ts +++ b/e2e/suite1/specs/variables/new-query-variable.ts @@ -48,7 +48,7 @@ describe('Variables - Add variable', () => { e2e.pages.Dashboard.Settings.Variables.Edit.QueryVariable.queryOptionsRefreshSelect() .should('be.visible') .within((select) => { - e2e.components.Select.singleValue().should('have.text', 'Never'); + e2e.components.Select.singleValue().should('have.text', 'On dashboard load'); }); e2e.pages.Dashboard.Settings.Variables.Edit.QueryVariable.queryOptionsRegExInput() .should('be.visible') diff --git a/public/app/features/dashboard/containers/__snapshots__/DashboardPage.test.tsx.snap b/public/app/features/dashboard/containers/__snapshots__/DashboardPage.test.tsx.snap index ae474b91460..a0cab9cfe22 100644 --- a/public/app/features/dashboard/containers/__snapshots__/DashboardPage.test.tsx.snap +++ b/public/app/features/dashboard/containers/__snapshots__/DashboardPage.test.tsx.snap @@ -92,7 +92,7 @@ exports[`DashboardPage Dashboard init completed Should render dashboard grid 1` ], "refresh": undefined, "revision": undefined, - "schemaVersion": 28, + "schemaVersion": 29, "snapshot": undefined, "style": "dark", "tags": Array [], @@ -228,7 +228,7 @@ exports[`DashboardPage Dashboard init completed Should render dashboard grid 1` ], "refresh": undefined, "revision": undefined, - "schemaVersion": 28, + "schemaVersion": 29, "snapshot": undefined, "style": "dark", "tags": Array [], @@ -334,7 +334,7 @@ exports[`DashboardPage Dashboard init completed Should render dashboard grid 1` ], "refresh": undefined, "revision": undefined, - "schemaVersion": 28, + "schemaVersion": 29, "snapshot": undefined, "style": "dark", "tags": Array [], @@ -462,7 +462,7 @@ exports[`DashboardPage When dashboard has editview url state should render setti ], "refresh": undefined, "revision": undefined, - "schemaVersion": 28, + "schemaVersion": 29, "snapshot": undefined, "style": "dark", "tags": Array [], @@ -598,7 +598,7 @@ exports[`DashboardPage When dashboard has editview url state should render setti ], "refresh": undefined, "revision": undefined, - "schemaVersion": 28, + "schemaVersion": 29, "snapshot": undefined, "style": "dark", "tags": Array [], @@ -704,7 +704,7 @@ exports[`DashboardPage When dashboard has editview url state should render setti ], "refresh": undefined, "revision": undefined, - "schemaVersion": 28, + "schemaVersion": 29, "snapshot": undefined, "style": "dark", "tags": Array [], @@ -814,7 +814,7 @@ exports[`DashboardPage When dashboard has editview url state should render setti ], "refresh": undefined, "revision": undefined, - "schemaVersion": 28, + "schemaVersion": 29, "snapshot": undefined, "style": "dark", "tags": Array [], diff --git a/public/app/features/dashboard/dashgrid/__snapshots__/DashboardGrid.test.tsx.snap b/public/app/features/dashboard/dashgrid/__snapshots__/DashboardGrid.test.tsx.snap index bdff0be2f4f..637c7b1f19e 100644 --- a/public/app/features/dashboard/dashgrid/__snapshots__/DashboardGrid.test.tsx.snap +++ b/public/app/features/dashboard/dashgrid/__snapshots__/DashboardGrid.test.tsx.snap @@ -244,7 +244,7 @@ exports[`DashboardGrid Can render dashboard grid Should render 1`] = ` ], "refresh": undefined, "revision": undefined, - "schemaVersion": 28, + "schemaVersion": 29, "snapshot": undefined, "style": "dark", "tags": Array [], @@ -505,7 +505,7 @@ exports[`DashboardGrid Can render dashboard grid Should render 1`] = ` ], "refresh": undefined, "revision": undefined, - "schemaVersion": 28, + "schemaVersion": 29, "snapshot": undefined, "style": "dark", "tags": Array [], @@ -766,7 +766,7 @@ exports[`DashboardGrid Can render dashboard grid Should render 1`] = ` ], "refresh": undefined, "revision": undefined, - "schemaVersion": 28, + "schemaVersion": 29, "snapshot": undefined, "style": "dark", "tags": Array [], @@ -1027,7 +1027,7 @@ exports[`DashboardGrid Can render dashboard grid Should render 1`] = ` ], "refresh": undefined, "revision": undefined, - "schemaVersion": 28, + "schemaVersion": 29, "snapshot": undefined, "style": "dark", "tags": Array [], diff --git a/public/app/features/dashboard/state/DashboardMigrator.test.ts b/public/app/features/dashboard/state/DashboardMigrator.test.ts index 93ecd137c95..018b94083f0 100644 --- a/public/app/features/dashboard/state/DashboardMigrator.test.ts +++ b/public/app/features/dashboard/state/DashboardMigrator.test.ts @@ -162,7 +162,7 @@ describe('DashboardModel', () => { }); it('dashboard schema version should be set to latest', () => { - expect(model.schemaVersion).toBe(28); + expect(model.schemaVersion).toBe(29); }); it('graph thresholds should be migrated', () => { @@ -925,6 +925,149 @@ describe('DashboardModel', () => { }); }); }); + + describe('when migrating variable refresh to on dashboard load', () => { + let model: DashboardModel; + + beforeEach(() => { + model = new DashboardModel({ + templating: { + list: [ + { + type: 'query', + name: 'variable_with_never_refresh_with_options', + options: [{ text: 'A', value: 'A' }], + refresh: 0, + }, + { + type: 'query', + name: 'variable_with_never_refresh_without_options', + options: [], + refresh: 0, + }, + { + type: 'query', + name: 'variable_with_dashboard_refresh_with_options', + options: [{ text: 'A', value: 'A' }], + refresh: 1, + }, + { + type: 'query', + name: 'variable_with_dashboard_refresh_without_options', + options: [], + refresh: 1, + }, + { + type: 'query', + name: 'variable_with_timerange_refresh_with_options', + options: [{ text: 'A', value: 'A' }], + refresh: 2, + }, + { + type: 'query', + name: 'variable_with_timerange_refresh_without_options', + options: [], + refresh: 2, + }, + { + type: 'query', + name: 'variable_with_no_refresh_with_options', + options: [{ text: 'A', value: 'A' }], + }, + { + type: 'query', + name: 'variable_with_no_refresh_without_options', + options: [], + }, + { + type: 'query', + name: 'variable_with_unknown_refresh_with_options', + options: [{ text: 'A', value: 'A' }], + refresh: 2001, + }, + { + type: 'query', + name: 'variable_with_unknown_refresh_without_options', + options: [], + refresh: 2001, + }, + { + type: 'custom', + name: 'custom', + options: [{ text: 'custom', value: 'custom' }], + }, + { + type: 'textbox', + name: 'textbox', + options: [{ text: 'Hello', value: 'World' }], + }, + { + type: 'datasource', + name: 'datasource', + options: [{ text: 'ds', value: 'ds' }], // fake example doesn't exist + }, + { + type: 'interval', + name: 'interval', + options: [{ text: '1m', value: '1m' }], + }, + ], + }, + }); + }); + + it('should have 11 variables after migration', () => { + expect(model.templating.list.length).toBe(14); + }); + + it('should not affect custom variable types', () => { + const custom = model.templating.list[10]; + expect(custom.type).toEqual('custom'); + expect(custom.options).toEqual([{ text: 'custom', value: 'custom' }]); + }); + + it('should not affect textbox variable types', () => { + const textbox = model.templating.list[11]; + expect(textbox.type).toEqual('textbox'); + expect(textbox.options).toEqual([{ text: 'Hello', value: 'World' }]); + }); + + it('should not affect datasource variable types', () => { + const datasource = model.templating.list[12]; + expect(datasource.type).toEqual('datasource'); + expect(datasource.options).toEqual([{ text: 'ds', value: 'ds' }]); + }); + + it('should not affect interval variable types', () => { + const interval = model.templating.list[13]; + expect(interval.type).toEqual('interval'); + expect(interval.options).toEqual([{ text: '1m', value: '1m' }]); + }); + + it('should removed options from all query variables', () => { + const queryVariables = model.templating.list.filter((v) => v.type === 'query'); + expect(queryVariables).toHaveLength(10); + const noOfOptions = queryVariables.reduce((all, variable) => all + variable.options.length, 0); + expect(noOfOptions).toBe(0); + }); + + it('should set the refresh prop to on dashboard load for all query variables that have never or unknown', () => { + expect(model.templating.list[0].refresh).toBe(1); + expect(model.templating.list[1].refresh).toBe(1); + expect(model.templating.list[2].refresh).toBe(1); + expect(model.templating.list[3].refresh).toBe(1); + expect(model.templating.list[4].refresh).toBe(2); + expect(model.templating.list[5].refresh).toBe(2); + expect(model.templating.list[6].refresh).toBe(1); + expect(model.templating.list[7].refresh).toBe(1); + expect(model.templating.list[8].refresh).toBe(1); + expect(model.templating.list[9].refresh).toBe(1); + expect(model.templating.list[10].refresh).toBeUndefined(); + expect(model.templating.list[11].refresh).toBeUndefined(); + expect(model.templating.list[12].refresh).toBeUndefined(); + expect(model.templating.list[13].refresh).toBeUndefined(); + }); + }); }); function createRow(options: any, panelDescriptions: any[]) { diff --git a/public/app/features/dashboard/state/DashboardMigrator.ts b/public/app/features/dashboard/state/DashboardMigrator.ts index 16432bb4a89..ccc45478094 100644 --- a/public/app/features/dashboard/state/DashboardMigrator.ts +++ b/public/app/features/dashboard/state/DashboardMigrator.ts @@ -45,7 +45,7 @@ export class DashboardMigrator { let i, j, k, n; const oldVersion = this.dashboard.schemaVersion; const panelUpgrades = []; - this.dashboard.schemaVersion = 28; + this.dashboard.schemaVersion = 29; if (oldVersion === this.dashboard.schemaVersion) { return; @@ -594,6 +594,22 @@ export class DashboardMigrator { } } + if (oldVersion < 29) { + for (const variable of this.dashboard.templating.list) { + if (variable.type !== 'query') { + continue; + } + + if (variable.refresh !== 1 && variable.refresh !== 2) { + variable.refresh = 1; + } + + if (variable.options?.length) { + variable.options = []; + } + } + } + if (panelUpgrades.length === 0) { return; } diff --git a/public/app/features/dashboard/state/DashboardModel.repeat.test.ts b/public/app/features/dashboard/state/DashboardModel.repeat.test.ts index 12d59ad7ff5..3bf4355540c 100644 --- a/public/app/features/dashboard/state/DashboardModel.repeat.test.ts +++ b/public/app/features/dashboard/state/DashboardModel.repeat.test.ts @@ -19,6 +19,7 @@ describe('given dashboard with panel repeat', () => { list: [ { name: 'apps', + type: 'custom', current: { text: 'se1, se2, se3', value: ['se1', 'se2', 'se3'], @@ -74,6 +75,7 @@ describe('given dashboard with panel repeat in horizontal direction', () => { list: [ { name: 'apps', + type: 'custom', current: { text: 'se1, se2, se3', value: ['se1', 'se2', 'se3'], @@ -204,6 +206,7 @@ describe('given dashboard with panel repeat in vertical direction', () => { list: [ { name: 'apps', + type: 'custom', current: { text: 'se1, se2, se3', value: ['se1', 'se2', 'se3'], @@ -246,6 +249,7 @@ describe('given dashboard with row repeat and panel repeat in horizontal directi list: [ { name: 'region', + type: 'custom', current: { text: 'reg1, reg2', value: ['reg1', 'reg2'], @@ -257,6 +261,7 @@ describe('given dashboard with row repeat and panel repeat in horizontal directi }, { name: 'app', + type: 'custom', current: { text: 'se1, se2, se3, se4, se5, se6', value: ['se1', 'se2', 'se3', 'se4', 'se5', 'se6'], @@ -339,6 +344,7 @@ describe('given dashboard with row repeat', () => { list: [ { name: 'apps', + type: 'custom', current: { text: 'se1, se2', value: ['se1', 'se2'], @@ -435,6 +441,7 @@ describe('given dashboard with row repeat', () => { ]; dashboardJSON.templating.list.push({ name: 'hosts', + type: 'custom', current: { text: 'backend01, backend02', value: ['backend01', 'backend02'], @@ -543,6 +550,7 @@ describe('given dashboard with row and panel repeat', () => { list: [ { name: 'region', + type: 'custom', current: { text: 'reg1, reg2', value: ['reg1', 'reg2'], @@ -555,6 +563,7 @@ describe('given dashboard with row and panel repeat', () => { }, { name: 'app', + type: 'custom', current: { text: 'se1, se2', value: ['se1', 'se2'], @@ -690,6 +699,7 @@ describe('given panel is in view mode', () => { list: [ { name: 'apps', + type: 'custom', current: { text: 'se1, se2, se3', value: ['se1', 'se2', 'se3'], diff --git a/public/app/features/dashboard/state/DashboardModel.test.ts b/public/app/features/dashboard/state/DashboardModel.test.ts index 7d7a20a6a53..ae16f89c207 100644 --- a/public/app/features/dashboard/state/DashboardModel.test.ts +++ b/public/app/features/dashboard/state/DashboardModel.test.ts @@ -5,9 +5,14 @@ import { getDashboardModel } from '../../../../test/helpers/getDashboardModel'; import { variableAdapters } from '../../variables/adapters'; import { createAdHocVariableAdapter } from '../../variables/adhoc/adapter'; import { createQueryVariableAdapter } from '../../variables/query/adapter'; +import { createCustomVariableAdapter } from '../../variables/custom/adapter'; jest.mock('app/core/services/context_srv', () => ({})); -variableAdapters.setInit(() => [createQueryVariableAdapter(), createAdHocVariableAdapter()]); +variableAdapters.setInit(() => [ + createQueryVariableAdapter(), + createAdHocVariableAdapter(), + createCustomVariableAdapter(), +]); describe('DashboardModel', () => { describe('when creating new dashboard model defaults only', () => { @@ -522,6 +527,7 @@ describe('DashboardModel', () => { list: [ { name: 'dc', + type: 'custom', current: { text: 'dc1 + dc2', value: ['dc1', 'dc2'], @@ -533,6 +539,7 @@ describe('DashboardModel', () => { }, { name: 'app', + type: 'custom', current: { text: 'se1 + se2', value: ['se1', 'se2'], diff --git a/public/app/features/variables/query/QueryVariableRefreshSelect.tsx b/public/app/features/variables/query/QueryVariableRefreshSelect.tsx index 64cab716600..7e587d3073d 100644 --- a/public/app/features/variables/query/QueryVariableRefreshSelect.tsx +++ b/public/app/features/variables/query/QueryVariableRefreshSelect.tsx @@ -10,7 +10,6 @@ interface Props { } const REFRESH_OPTIONS = [ - { label: 'Never', value: VariableRefresh.never }, { label: 'On dashboard load', value: VariableRefresh.onDashboardLoad }, { label: 'On time range change', value: VariableRefresh.onTimeRangeChanged }, ]; diff --git a/public/app/features/variables/query/reducer.ts b/public/app/features/variables/query/reducer.ts index 15de292d73e..cceb8310c2e 100644 --- a/public/app/features/variables/query/reducer.ts +++ b/public/app/features/variables/query/reducer.ts @@ -39,7 +39,7 @@ export const initialQueryVariableModelState: QueryVariableModel = { query: '', regex: '', sort: VariableSort.disabled, - refresh: VariableRefresh.never, + refresh: VariableRefresh.onDashboardLoad, multi: false, includeAll: false, allValue: null, diff --git a/public/app/features/variables/state/actions.test.ts b/public/app/features/variables/state/actions.test.ts index 1e01394b375..a4fbbe56715 100644 --- a/public/app/features/variables/state/actions.test.ts +++ b/public/app/features/variables/state/actions.test.ts @@ -61,6 +61,8 @@ import { ConstantVariableModel, VariableRefresh } from '../types'; import { updateVariableOptions } from '../query/reducer'; import { setVariableQueryRunner, VariableQueryRunner } from '../query/VariableQueryRunner'; import { setDataSourceSrv, setLocationService } from '@grafana/runtime'; +import { LoadingState } from '@grafana/data'; +import { toAsyncOfResult } from '../../query/state/DashboardQueryRunner/testHelpers'; variableAdapters.setInit(() => [ createQueryVariableAdapter(), @@ -148,6 +150,13 @@ describe('shared actions', () => { }; const locationService: any = { getSearchObject: () => ({}) }; setLocationService(locationService); + const variableQueryRunner: any = { + cancelRequest: jest.fn(), + queueRequest: jest.fn(), + getResponse: () => toAsyncOfResult({ state: LoadingState.Done, identifier: toVariableIdentifier(query) }), + destroy: jest.fn(), + }; + setVariableQueryRunner(variableQueryRunner); const tester = await reduxTester({ preloadedState }) .givenRootReducer(getTemplatingRootReducer()) @@ -156,10 +165,10 @@ describe('shared actions', () => { .whenAsyncActionIsDispatched(processVariables(), true); await tester.thenDispatchedActionsPredicateShouldEqual((dispatchedActions) => { - expect(dispatchedActions.length).toEqual(4); + expect(dispatchedActions.length).toEqual(5); expect(dispatchedActions[0]).toEqual( - variableStateCompleted(toVariablePayload({ ...query, id: dispatchedActions[0].payload.id })) + variableStateFetching(toVariablePayload({ ...query, id: dispatchedActions[0].payload.id })) ); expect(dispatchedActions[1]).toEqual( @@ -174,6 +183,10 @@ describe('shared actions', () => { variableStateCompleted(toVariablePayload({ ...textbox, id: dispatchedActions[3].payload.id })) ); + expect(dispatchedActions[4]).toEqual( + variableStateCompleted(toVariablePayload({ ...query, id: dispatchedActions[4].payload.id })) + ); + return true; }); }); diff --git a/public/app/features/variables/types.ts b/public/app/features/variables/types.ts index b75eb77eac1..6a1f9c7273c 100644 --- a/public/app/features/variables/types.ts +++ b/public/app/features/variables/types.ts @@ -12,7 +12,7 @@ import { NEW_VARIABLE_ID } from './state/types'; import { VariableQueryProps } from '../../types'; export enum VariableRefresh { - never, + never, // removed from the UI onDashboardLoad, onTimeRangeChanged, }