From c5279bba7f0a9f81e57d84614287b4fba1d6b519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Mon, 25 Jan 2021 08:37:41 +0100 Subject: [PATCH] RefreshPicker: Fixes so valid intervals in url are visible in RefreshPicker (#30474) * RefreshPicker: Fixes so refresh intervals from url are visible in RefreshPicker * Refactor: changes after PR comments * Chore: adds comment --- .../RefreshPicker/RefreshPicker.test.tsx | 39 ++++++++++ .../RefreshPicker/RefreshPicker.tsx | 24 +++--- .../features/dashboard/services/TimeSrv.ts | 24 +++--- .../dashboard/utils/getRefreshFromUrl.test.ts | 75 +++++++++++++++++++ .../dashboard/utils/getRefreshFromUrl.ts | 39 ++++++++++ 5 files changed, 177 insertions(+), 24 deletions(-) create mode 100644 packages/grafana-ui/src/components/RefreshPicker/RefreshPicker.test.tsx create mode 100644 public/app/features/dashboard/utils/getRefreshFromUrl.test.ts create mode 100644 public/app/features/dashboard/utils/getRefreshFromUrl.ts diff --git a/packages/grafana-ui/src/components/RefreshPicker/RefreshPicker.test.tsx b/packages/grafana-ui/src/components/RefreshPicker/RefreshPicker.test.tsx new file mode 100644 index 00000000000..12cb111e31a --- /dev/null +++ b/packages/grafana-ui/src/components/RefreshPicker/RefreshPicker.test.tsx @@ -0,0 +1,39 @@ +import { intervalsToOptions } from './RefreshPicker'; + +describe('RefreshPicker', () => { + describe('intervalsToOptions', () => { + describe('when called without intervals', () => { + it('then default options should be used', () => { + const result = intervalsToOptions(); + + expect(result).toEqual([ + { value: '', label: 'Off' }, + { value: '5s', label: '5s' }, + { value: '10s', label: '10s' }, + { value: '30s', label: '30s' }, + { value: '1m', label: '1m' }, + { value: '5m', label: '5m' }, + { value: '15m', label: '15m' }, + { value: '30m', label: '30m' }, + { value: '1h', label: '1h' }, + { value: '2h', label: '2h' }, + { value: '1d', label: '1d' }, + ]); + }); + }); + + describe('when called with intervals', () => { + it('then the resulting options should be correct', () => { + const intervals = ['5s', '10s']; + + const result = intervalsToOptions({ intervals }); + + expect(result).toEqual([ + { value: '', label: 'Off' }, + { value: '5s', label: '5s' }, + { value: '10s', label: '10s' }, + ]); + }); + }); + }); +}); diff --git a/packages/grafana-ui/src/components/RefreshPicker/RefreshPicker.tsx b/packages/grafana-ui/src/components/RefreshPicker/RefreshPicker.tsx index 795adc3702e..9e6b87d7fe3 100644 --- a/packages/grafana-ui/src/components/RefreshPicker/RefreshPicker.tsx +++ b/packages/grafana-ui/src/components/RefreshPicker/RefreshPicker.tsx @@ -31,14 +31,6 @@ export class RefreshPicker extends PureComponent { super(props); } - intervalsToOptions = (intervals: string[] | undefined): Array> => { - const intervalsOrDefault = intervals || defaultIntervals; - const options = intervalsOrDefault.map((interval) => ({ label: interval, value: interval })); - - options.unshift(RefreshPicker.offOption); - return options; - }; - onChangeSelect = (item: SelectableValue) => { const { onIntervalChanged } = this.props; if (onIntervalChanged) { @@ -63,11 +55,11 @@ export class RefreshPicker extends PureComponent { render() { const { onRefresh, intervals, tooltip, value, text, isLoading, noIntervalPicker } = this.props; - const options = this.intervalsToOptions(intervals); const currentValue = value || ''; const variant = this.getVariant(); - - let selectedValue = options.find((item) => item.value === currentValue) || RefreshPicker.offOption; + const options = intervalsToOptions({ intervals }); + const option = options.find(({ value }) => value === currentValue); + let selectedValue = option || RefreshPicker.offOption; if (selectedValue.label === RefreshPicker.offOption.label) { selectedValue = { value: '' }; @@ -100,3 +92,13 @@ export class RefreshPicker extends PureComponent { ); } } + +export function intervalsToOptions({ intervals = defaultIntervals }: { intervals?: string[] } = {}): Array< + SelectableValue +> { + const intervalsOrDefault = intervals || defaultIntervals; + const options = intervalsOrDefault.map((interval) => ({ label: interval, value: interval })); + + options.unshift(RefreshPicker.offOption); + return options; +} diff --git a/public/app/features/dashboard/services/TimeSrv.ts b/public/app/features/dashboard/services/TimeSrv.ts index ae3ff72d5f4..df59d33bdd2 100644 --- a/public/app/features/dashboard/services/TimeSrv.ts +++ b/public/app/features/dashboard/services/TimeSrv.ts @@ -1,8 +1,5 @@ -// Libraries import _ from 'lodash'; -// Utils -import coreModule from 'app/core/core_module'; -// Types +import { ILocationService, ITimeoutService } from 'angular'; import { dateMath, dateTime, @@ -13,15 +10,16 @@ import { TimeRange, toUtc, } from '@grafana/data'; -import { ILocationService, ITimeoutService } from 'angular'; + +import coreModule from 'app/core/core_module'; import { ContextSrv } from 'app/core/services/context_srv'; import { DashboardModel } from '../state/DashboardModel'; import { GrafanaRootScope } from 'app/routes/GrafanaCtrl'; import { getShiftedTimeRange, getZoomedTimeRange } from 'app/core/utils/timePicker'; import { appEvents } from '../../../core/core'; import { CoreEvents } from '../../../types'; - import { config } from 'app/core/config'; +import { getRefreshFromUrl } from '../utils/getRefreshFromUrl'; export class TimeSrv { time: any; @@ -151,13 +149,13 @@ export class TimeSrv { this.dashboard.refresh = false; } // but if refresh explicitly set then use that - if (params.refresh) { - if (!this.contextSrv.isAllowedInterval(params.refresh)) { - this.refresh = config.minRefreshInterval; - } else { - this.refresh = params.refresh || this.refresh; - } - } + this.refresh = getRefreshFromUrl({ + params, + currentRefresh: this.refresh, + refreshIntervals: this.dashboard?.timepicker?.refresh_intervals, + isAllowedIntervalFn: this.contextSrv.isAllowedInterval, + minRefreshInterval: config.minRefreshInterval, + }); } private routeUpdated() { diff --git a/public/app/features/dashboard/utils/getRefreshFromUrl.test.ts b/public/app/features/dashboard/utils/getRefreshFromUrl.test.ts new file mode 100644 index 00000000000..c63ab1b64c9 --- /dev/null +++ b/public/app/features/dashboard/utils/getRefreshFromUrl.test.ts @@ -0,0 +1,75 @@ +import { getRefreshFromUrl } from './getRefreshFromUrl'; + +describe('getRefreshFromUrl', () => { + describe('when refresh is not part of params', () => { + it('then it should return current refresh value', () => { + const params = {}; + const currentRefresh = false; + const minRefreshInterval = '5s'; + const isAllowedIntervalFn = () => false; + + const actual = getRefreshFromUrl({ + params, + currentRefresh, + minRefreshInterval, + isAllowedIntervalFn, + }); + + expect(actual).toBe(false); + }); + }); + + describe('when refresh is part of params', () => { + describe('and refresh is an existing and valid interval', () => { + it('then it should return the refresh value', () => { + const params = { refresh: '10s' }; + const currentRefresh = ''; + const minRefreshInterval = '5s'; + const isAllowedIntervalFn = () => true; + const refreshIntervals = ['5s', '10s', '30s']; + + const actual = getRefreshFromUrl({ + params, + currentRefresh, + minRefreshInterval, + isAllowedIntervalFn, + refreshIntervals, + }); + + expect(actual).toBe('10s'); + }); + }); + + it.each` + refresh | isAllowedInterval | minRefreshInterval | refreshIntervals | expected + ${'6s'} | ${true} | ${'1s'} | ${['5s', '6s', '10s', '30s']} | ${'6s'} + ${'6s'} | ${true} | ${'10s'} | ${['5s', '10s', '30s']} | ${'10s'} + ${'6s'} | ${true} | ${'1s'} | ${['5s', '10s', '30s']} | ${'5s'} + ${'6s'} | ${true} | ${'1s'} | ${undefined} | ${'5s'} + ${'6s'} | ${true} | ${'10s'} | ${undefined} | ${'10s'} + ${'6s'} | ${true} | ${'1s'} | ${[]} | ${'currentRefresh'} + ${'6s'} | ${true} | ${'10s'} | ${[]} | ${'currentRefresh'} + ${'6s'} | ${false} | ${'1s'} | ${['5s', '6s', '10s', '30s']} | ${'5s'} + ${'6s'} | ${false} | ${'10s'} | ${['5s', '6s', '10s', '30s']} | ${'10s'} + ${'6s'} | ${false} | ${'1s'} | ${['5s', '10s', '30s']} | ${'5s'} + ${'6s'} | ${false} | ${'10s'} | ${['5s', '10s', '30s']} | ${'10s'} + ${'6s'} | ${false} | ${'1s'} | ${undefined} | ${'5s'} + ${'6s'} | ${false} | ${'10s'} | ${undefined} | ${'10s'} + ${'6s'} | ${false} | ${'1s'} | ${[]} | ${'currentRefresh'} + ${'6s'} | ${false} | ${'10s'} | ${[]} | ${'currentRefresh'} + `( + 'when called with refresh:{$refresh}, isAllowedInterval:{$isAllowedInterval}, minRefreshInterval:{$minRefreshInterval}, refreshIntervals:{$refreshIntervals} then it should return: $expected', + ({ refresh, isAllowedInterval, minRefreshInterval, refreshIntervals, expected }) => { + const actual = getRefreshFromUrl({ + params: { refresh }, + currentRefresh: 'currentRefresh', + minRefreshInterval, + isAllowedIntervalFn: () => isAllowedInterval, + refreshIntervals, + }); + + expect(actual).toBe(expected); + } + ); + }); +}); diff --git a/public/app/features/dashboard/utils/getRefreshFromUrl.ts b/public/app/features/dashboard/utils/getRefreshFromUrl.ts new file mode 100644 index 00000000000..a838313b662 --- /dev/null +++ b/public/app/features/dashboard/utils/getRefreshFromUrl.ts @@ -0,0 +1,39 @@ +import { defaultIntervals } from '@grafana/ui'; + +interface Args { + params: Record; + currentRefresh: string | boolean | undefined; + isAllowedIntervalFn: (interval: string) => boolean; + minRefreshInterval: string; + refreshIntervals?: string[]; +} + +// getRefreshFromUrl function returns the value from the supplied &refresh= param in url. +// If the supplied interval is not allowed or does not exist in the refresh intervals for the dashboard then we +// try to find the first refresh interval that matches the minRefreshInterval (min_refresh_interval in ini) +// or just take the first interval. +export function getRefreshFromUrl({ + params, + currentRefresh, + isAllowedIntervalFn, + minRefreshInterval, + refreshIntervals = defaultIntervals, +}: Args): string | boolean | undefined { + if (!params.refresh) { + return currentRefresh; + } + + const isAllowedInterval = isAllowedIntervalFn(params.refresh); + const isExistingInterval = refreshIntervals.find((interval) => interval === params.refresh); + + if (!isAllowedInterval || !isExistingInterval) { + const minRefreshIntervalInIntervals = minRefreshInterval + ? refreshIntervals.find((interval) => interval === minRefreshInterval) + : undefined; + const lowestRefreshInterval = refreshIntervals?.length ? refreshIntervals[0] : undefined; + + return minRefreshIntervalInIntervals ?? lowestRefreshInterval ?? currentRefresh; + } + + return params.refresh || currentRefresh; +}