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

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' },
+        ]);
+      });
+    });
+  });
+});

packages/grafana-ui/src/components/RefreshPicker/RefreshPicker.tsx

@@ -31,14 +31,6 @@ export class RefreshPicker extends PureComponent<Props> {
     super(props);
   }
 
-  intervalsToOptions = (intervals: string[] | undefined): Array<SelectableValue<string>> => {
-    const intervalsOrDefault = intervals || defaultIntervals;
-    const options = intervalsOrDefault.map((interval) => ({ label: interval, value: interval }));
-
-    options.unshift(RefreshPicker.offOption);
-    return options;
-  };
-
   onChangeSelect = (item: SelectableValue<string>) => {
     const { onIntervalChanged } = this.props;
     if (onIntervalChanged) {
@@ -63,11 +55,11 @@ export class RefreshPicker extends PureComponent<Props> {
   render() {
     const { onRefresh, intervals, tooltip, value, text, isLoading, noIntervalPicker } = this.props;
 
-    const options = this.intervalsToOptions(intervals);
     const currentValue = value || '';
     const variant = this.getVariant();
-
+    const options = intervalsToOptions({ intervals });
-    let selectedValue = options.find((item) => item.value === currentValue) || RefreshPicker.offOption;
+    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<Props> {
     );
   }
 }
+
+export function intervalsToOptions({ intervals = defaultIntervals }: { intervals?: string[] } = {}): Array<
+  SelectableValue<string>
+> {
+  const intervalsOrDefault = intervals || defaultIntervals;
+  const options = intervalsOrDefault.map((interval) => ({ label: interval, value: interval }));
+
+  options.unshift(RefreshPicker.offOption);
+  return options;
+}

public/app/features/dashboard/services/TimeSrv.ts

@@ -1,8 +1,5 @@
-// Libraries
 import _ from 'lodash';
-// Utils
+import { ILocationService, ITimeoutService } from 'angular';
-import coreModule from 'app/core/core_module';
-// Types
 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) {
+    this.refresh = getRefreshFromUrl({
-      if (!this.contextSrv.isAllowedInterval(params.refresh)) {
+      params,
-        this.refresh = config.minRefreshInterval;
+      currentRefresh: this.refresh,
-      } else {
+      refreshIntervals: this.dashboard?.timepicker?.refresh_intervals,
-        this.refresh = params.refresh || this.refresh;
+      isAllowedIntervalFn: this.contextSrv.isAllowedInterval,
-      }
+      minRefreshInterval: config.minRefreshInterval,
-    }
+    });
   }
 
   private routeUpdated() {

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);
+      }
+    );
+  });
+});

public/app/features/dashboard/utils/getRefreshFromUrl.ts

@@ -0,0 +1,39 @@
+import { defaultIntervals } from '@grafana/ui';
+
+interface Args {
+  params: Record<string, string>;
+  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;
+}