DashboardSettings: Fixes auto refresh crash with space in interval (#27438)

* DashboardSettings: Fixes auto refresh crash with space in interval

* Tests: fixes typings

* Refactor: validets onBlur and other PR comments

* Tests: adds component tests

* Refactor: changes after PR feedback

public/app/core/angular_wrappers.ts

@@ -10,15 +10,14 @@ import { MetricSelect } from './components/Select/MetricSelect';
 import AppNotificationList from './components/AppNotifications/AppNotificationList';
 import {
   ColorPicker,
+  DataLinksInlineEditor,
   DataSourceHttpSettings,
   GraphContextMenu,
-  SeriesColorPickerPopoverWithTheme,
-  UnitPicker,
   Icon,
   LegacyForms,
-  DataLinksInlineEditor,
+  SeriesColorPickerPopoverWithTheme,
+  UnitPicker,
 } from '@grafana/ui';
-const { SecretFormField } = LegacyForms;
 import { FunctionEditor } from 'app/plugins/datasource/graphite/FunctionEditor';
 import { LokiAnnotationsQueryEditor } from '../plugins/datasource/loki/components/AnnotationsQueryEditor';
 import { HelpModal } from './components/help/HelpModal';
@@ -29,9 +28,11 @@ import {
   SaveDashboardButtonConnected,
 } from '../features/dashboard/components/SaveDashboard/SaveDashboardButton';
 import { VariableEditorContainer } from '../features/variables/editor/VariableEditorContainer';
-import { SearchField, SearchResults, SearchWrapper, SearchResultsFilter } from '../features/search';
+import { SearchField, SearchResults, SearchResultsFilter, SearchWrapper } from '../features/search';
 import { TimePickerSettings } from 'app/features/dashboard/components/DashboardSettings/TimePickerSettings';
 
+const { SecretFormField } = LegacyForms;
+
 export function registerAngularDirectives() {
   react2AngularDirective('footer', Footer, []);
   react2AngularDirective('icon', Icon, [
@@ -200,7 +201,11 @@ export function registerAngularDirectives() {
   ]);
   react2AngularDirective('variableEditorContainer', VariableEditorContainer, []);
   react2AngularDirective('timePickerSettings', TimePickerSettings, [
-    ['getDashboard', { watchDepth: 'reference', wrapApply: true }],
+    'renderCount',
+    'refreshIntervals',
+    'timePickerHidden',
+    'nowDelay',
+    'timezone',
     ['onTimeZoneChange', { watchDepth: 'reference', wrapApply: true }],
     ['onRefreshIntervalChange', { watchDepth: 'reference', wrapApply: true }],
     ['onNowDelayChange', { watchDepth: 'reference', wrapApply: true }],

public/app/features/dashboard/components/DashboardSettings/AutoRefreshIntervals.test.tsx

@@ -0,0 +1,179 @@
+import React from 'react';
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import { defaultIntervals } from '@grafana/ui/src/components/RefreshPicker/RefreshPicker';
+
+import { AutoRefreshIntervals, getValidIntervals, Props, validateIntervals } from './AutoRefreshIntervals';
+import { TimeSrv } from 'app/core/services/TimeSrv';
+
+const setupTestContext = (options: Partial<Props>) => {
+  const defaults: Props = {
+    renderCount: 0,
+    refreshIntervals: ['1s', '5s', '10s'],
+    onRefreshIntervalChange: jest.fn(),
+    getIntervalsFunc: intervals => intervals,
+    validateIntervalsFunc: () => null,
+  };
+
+  const props = { ...defaults, ...options };
+  const { rerender } = render(<AutoRefreshIntervals {...props} />);
+
+  return { rerender, props };
+};
+
+describe('AutoRefreshIntervals', () => {
+  describe('when component is mounted with refreshIntervals', () => {
+    it('then supplied intervals should be shown', () => {
+      setupTestContext({ getIntervalsFunc: () => ['5s', '10s'] }); // remove 1s entry to validate we're calling getIntervalsFunc
+
+      expect(screen.getByRole('textbox')).toHaveValue('5s,10s');
+    });
+  });
+
+  describe('when component is mounted without refreshIntervals', () => {
+    it('then default intervals should be shown', () => {
+      setupTestContext({ refreshIntervals: (null as unknown) as string[] });
+
+      expect(screen.getByRole('textbox')).toHaveValue('5s,10s,30s,1m,5m,15m,30m,1h,2h,1d');
+    });
+  });
+
+  describe('when component is updated from Angular', () => {
+    it('then intervals should be updated', () => {
+      const { rerender, props } = setupTestContext({});
+      const newProps = { ...props, renderCount: 1, refreshIntervals: ['2s', '6s', '11s'] };
+
+      rerender(<AutoRefreshIntervals {...newProps} />);
+
+      expect(screen.getByRole('textbox')).toHaveValue('2s,6s,11s');
+    });
+  });
+
+  describe('when input loses focus and intervals are valid', () => {
+    it('then onRefreshIntervalChange should be called', () => {
+      const { props } = setupTestContext({ validateIntervalsFunc: () => null });
+
+      userEvent.type(screen.getByRole('textbox'), ',30s');
+      userEvent.tab();
+
+      expect(screen.getByRole('textbox')).toHaveValue('1s,5s,10s,30s');
+      expect(props.onRefreshIntervalChange).toHaveBeenCalledTimes(1);
+      expect(props.onRefreshIntervalChange).toHaveBeenCalledWith(['1s', '5s', '10s', '30s']);
+    });
+  });
+
+  describe('when input loses focus and intervals are invalid', () => {
+    it('then onRefreshIntervalChange should not be called', () => {
+      const { props } = setupTestContext({ validateIntervalsFunc: () => 'Not valid' });
+
+      userEvent.type(screen.getByRole('textbox'), ',30q');
+      userEvent.tab();
+
+      expect(screen.getByRole('textbox')).toHaveValue('1s,5s,10s,30q');
+      expect(props.onRefreshIntervalChange).toHaveBeenCalledTimes(0);
+    });
+  });
+
+  describe('when input loses focus and previous intervals were invalid', () => {
+    it('then onRefreshIntervalChange should be called', () => {
+      const validateIntervalsFunc = jest
+        .fn()
+        .mockReturnValueOnce('Not valid')
+        .mockReturnValue(null);
+      const { props } = setupTestContext({ validateIntervalsFunc });
+
+      userEvent.type(screen.getByRole('textbox'), ',30q');
+      userEvent.tab();
+      userEvent.type(screen.getByRole('textbox'), '{backspace}s');
+      userEvent.tab();
+
+      expect(screen.getByRole('textbox')).toHaveValue('1s,5s,10s,30s');
+      expect(props.onRefreshIntervalChange).toHaveBeenCalledTimes(1);
+      expect(props.onRefreshIntervalChange).toHaveBeenCalledWith(['1s', '5s', '10s', '30s']);
+    });
+  });
+});
+
+describe('getValidIntervals', () => {
+  describe('when called with empty intervals', () => {
+    it('then is should all non empty intervals', () => {
+      const emptyIntervals = ['', '5s', ' ', '10s', '  '];
+      const dependencies = {
+        getTimeSrv: () =>
+          (({
+            getValidIntervals: (intervals: any) => intervals,
+          } as unknown) as TimeSrv),
+      };
+
+      const result = getValidIntervals(emptyIntervals, dependencies);
+
+      expect(result).toEqual(['5s', '10s']);
+    });
+  });
+
+  describe('when called with duplicate intervals', () => {
+    it('then is should return no duplicates', () => {
+      const duplicateIntervals = ['5s', '10s', '1m', '5s', '30s', '10s', '5s', '2m'];
+      const dependencies = {
+        getTimeSrv: () =>
+          (({
+            getValidIntervals: (intervals: any) => intervals,
+          } as unknown) as TimeSrv),
+      };
+
+      const result = getValidIntervals(duplicateIntervals, dependencies);
+
+      expect(result).toEqual(['5s', '10s', '1m', '30s', '2m']);
+    });
+  });
+
+  describe('when called with untrimmed intervals', () => {
+    it('then is should return trimmed intervals', () => {
+      const duplicateIntervals = [' 5s', '10s ', ' 1m ', ' 3 0 s ', '   2      m     '];
+      const dependencies = {
+        getTimeSrv: () =>
+          (({
+            getValidIntervals: (intervals: any) => intervals,
+          } as unknown) as TimeSrv),
+      };
+
+      const result = getValidIntervals(duplicateIntervals, dependencies);
+
+      expect(result).toEqual(['5s', '10s', '1m', '30s', '2m']);
+    });
+  });
+});
+
+describe('validateIntervals', () => {
+  describe('when getValidIntervals does not throw', () => {
+    it('then it should return null', () => {
+      const dependencies = {
+        getTimeSrv: () =>
+          (({
+            getValidIntervals: (intervals: any) => intervals,
+          } as unknown) as TimeSrv),
+      };
+
+      const result = validateIntervals(defaultIntervals, dependencies);
+
+      expect(result).toBe(null);
+    });
+  });
+
+  describe('when getValidIntervals throws', () => {
+    it('then it should return the exception message', () => {
+      const dependencies = {
+        getTimeSrv: () =>
+          (({
+            getValidIntervals: () => {
+              throw new Error('Some error');
+            },
+          } as unknown) as TimeSrv),
+      };
+
+      const result = validateIntervals(defaultIntervals, dependencies);
+
+      expect(result).toEqual('Some error');
+    });
+  });
+});

public/app/features/dashboard/components/DashboardSettings/AutoRefreshIntervals.tsx

@@ -0,0 +1,93 @@
+import React, { FC, useCallback, useEffect, useMemo, useState } from 'react';
+import { Input, Tooltip } from '@grafana/ui';
+import { defaultIntervals } from '@grafana/ui/src/components/RefreshPicker/RefreshPicker';
+
+import { getTimeSrv } from '../../services/TimeSrv';
+
+export interface Props {
+  renderCount: number; // hack to make sure Angular changes are propagated properly, please remove when DashboardSettings are migrated to React
+  refreshIntervals: string[];
+  onRefreshIntervalChange: (interval: string[]) => void;
+  getIntervalsFunc?: typeof getValidIntervals;
+  validateIntervalsFunc?: typeof validateIntervals;
+}
+
+export const AutoRefreshIntervals: FC<Props> = ({
+  renderCount,
+  refreshIntervals,
+  onRefreshIntervalChange,
+  getIntervalsFunc = getValidIntervals,
+  validateIntervalsFunc = validateIntervals,
+}) => {
+  const [intervals, setIntervals] = useState<string[]>(getIntervalsFunc(refreshIntervals ?? defaultIntervals));
+  const [invalidIntervalsMessage, setInvalidIntervalsMessage] = useState<string | null>(null);
+
+  useEffect(() => {
+    const intervals = getIntervalsFunc(refreshIntervals ?? defaultIntervals);
+    setIntervals(intervals);
+  }, [renderCount, refreshIntervals]);
+
+  const intervalsString = useMemo(() => {
+    if (!Array.isArray(intervals)) {
+      return '';
+    }
+
+    return intervals.join(',');
+  }, [intervals]);
+
+  const onIntervalsChange = useCallback(
+    (event: React.FormEvent<HTMLInputElement>) => {
+      const newIntervals = event.currentTarget.value ? event.currentTarget.value.split(',') : [];
+
+      setIntervals(newIntervals);
+    },
+    [setIntervals]
+  );
+
+  const onIntervalsBlur = useCallback(
+    (event: React.FormEvent<HTMLInputElement>) => {
+      const invalidMessage = validateIntervalsFunc(intervals);
+
+      if (invalidMessage === null) {
+        // only refresh dashboard JSON if intervals are valid
+        onRefreshIntervalChange(getIntervalsFunc(intervals));
+      }
+
+      setInvalidIntervalsMessage(invalidMessage);
+    },
+    [intervals, onRefreshIntervalChange, setInvalidIntervalsMessage]
+  );
+
+  return (
+    <div className="gf-form">
+      <label className="gf-form-label width-7">Auto-refresh</label>
+      {invalidIntervalsMessage ? (
+        <Tooltip placement="right" content={invalidIntervalsMessage}>
+          <Input width={60} invalid value={intervalsString} onChange={onIntervalsChange} onBlur={onIntervalsBlur} />
+        </Tooltip>
+      ) : (
+        <Input width={60} value={intervalsString} onChange={onIntervalsChange} onBlur={onIntervalsBlur} />
+      )}
+    </div>
+  );
+};
+
+export const validateIntervals = (
+  intervals: string[],
+  dependencies: { getTimeSrv: typeof getTimeSrv } = { getTimeSrv }
+): string | null => {
+  try {
+    getValidIntervals(intervals, dependencies);
+    return null;
+  } catch (err) {
+    return err.message;
+  }
+};
+
+export const getValidIntervals = (
+  intervals: string[],
+  dependencies: { getTimeSrv: typeof getTimeSrv } = { getTimeSrv }
+) => {
+  const cleanIntervals = intervals.filter(i => i.trim() !== '').map(interval => interval.replace(/\s+/g, ''));
+  return [...new Set(dependencies.getTimeSrv().getValidIntervals(cleanIntervals))];
+};

public/app/features/dashboard/components/DashboardSettings/SettingsCtrl.ts

@@ -23,6 +23,7 @@ export class SettingsCtrl {
   sections: any[];
   hasUnsavedFolderChange: boolean;
   selectors: typeof selectors.pages.Dashboard.Settings.General;
+  renderCount: number; // hack to update React when Angular changes
 
   /** @ngInject */
   constructor(
@@ -57,6 +58,7 @@ export class SettingsCtrl {
     appEvents.on(CoreEvents.dashboardSaved, this.onPostSave.bind(this), $scope);
 
     this.selectors = selectors.pages.Dashboard.Settings.General;
+    this.renderCount = 0;
   }
 
   buildSectionList() {
@@ -253,18 +255,22 @@ export class SettingsCtrl {
 
   onRefreshIntervalChange = (intervals: string[]) => {
     this.dashboard.timepicker.refresh_intervals = intervals.filter(i => i.trim() !== '');
+    this.renderCount++;
   };
 
   onNowDelayChange = (nowDelay: string) => {
     this.dashboard.timepicker.nowDelay = nowDelay;
+    this.renderCount++;
   };
 
   onHideTimePickerChange = (hide: boolean) => {
     this.dashboard.timepicker.hidden = hide;
+    this.renderCount++;
   };
 
   onTimeZoneChange = (timeZone: TimeZone) => {
     this.dashboard.timezone = timeZone;
+    this.renderCount++;
   };
 }
 

public/app/features/dashboard/components/DashboardSettings/TimePickerSettings.tsx

@@ -1,17 +1,20 @@
 import React, { PureComponent } from 'react';
-import { TimeZonePicker, Input, Tooltip, LegacyForms } from '@grafana/ui';
-import { DashboardModel } from '../../state/DashboardModel';
-import { TimeZone, rangeUtil } from '@grafana/data';
-import { config } from '@grafana/runtime';
+import { Input, LegacyForms, TimeZonePicker, Tooltip } from '@grafana/ui';
+import { rangeUtil, TimeZone } from '@grafana/data';
 import isEmpty from 'lodash/isEmpty';
 import { selectors } from '@grafana/e2e-selectors';
+import { AutoRefreshIntervals } from './AutoRefreshIntervals';
 
 interface Props {
-  getDashboard: () => DashboardModel;
   onTimeZoneChange: (timeZone: TimeZone) => void;
   onRefreshIntervalChange: (interval: string[]) => void;
   onNowDelayChange: (nowDelay: string) => void;
   onHideTimePickerChange: (hide: boolean) => void;
+  renderCount: number; // hack to make sure Angular changes are propagated properly, please remove when DashboardSettings are migrated to React
+  refreshIntervals: string[];
+  timePickerHidden: boolean;
+  nowDelay: string;
+  timezone: TimeZone;
 }
 
 interface State {
@@ -21,47 +24,6 @@ interface State {
 export class TimePickerSettings extends PureComponent<Props, State> {
   state: State = { isNowDelayValid: true };
 
-  componentDidMount() {
-    const { timepicker } = this.props.getDashboard();
-    let intervals: string[] = timepicker.refresh_intervals ?? [
-      '5s',
-      '10s',
-      '30s',
-      '1m',
-      '5m',
-      '15m',
-      '30m',
-      '1h',
-      '2h',
-      '1d',
-    ];
-
-    if (config.minRefreshInterval) {
-      intervals = intervals.filter(rate => {
-        return rangeUtil.intervalToMs(rate) >= rangeUtil.intervalToMs(config.minRefreshInterval);
-      });
-    }
-
-    this.props.onRefreshIntervalChange(intervals);
-  }
-
-  getRefreshIntervals = () => {
-    const dashboard = this.props.getDashboard();
-    if (!Array.isArray(dashboard.timepicker.refresh_intervals)) {
-      return '';
-    }
-    return dashboard.timepicker.refresh_intervals.join(',');
-  };
-
-  onRefreshIntervalChange = (event: React.FormEvent<HTMLInputElement>) => {
-    if (!event.currentTarget.value) {
-      return;
-    }
-    const intervals = event.currentTarget.value.split(',');
-    this.props.onRefreshIntervalChange(intervals);
-    this.forceUpdate();
-  };
-
   onNowDelayChange = (event: React.FormEvent<HTMLInputElement>) => {
     const value = event.currentTarget.value;
 
@@ -79,9 +41,7 @@ export class TimePickerSettings extends PureComponent<Props, State> {
   };
 
   onHideTimePickerChange = () => {
-    const dashboard = this.props.getDashboard();
-    this.props.onHideTimePickerChange(!dashboard.timepicker.hidden);
-    this.forceUpdate();
+    this.props.onHideTimePickerChange(!this.props.timePickerHidden);
   };
 
   onTimeZoneChange = (timeZone: string) => {
@@ -89,12 +49,9 @@ export class TimePickerSettings extends PureComponent<Props, State> {
       return;
     }
     this.props.onTimeZoneChange(timeZone);
-    this.forceUpdate();
   };
 
   render() {
-    const dashboard = this.props.getDashboard();
-
     return (
       <div className="editor-row">
         <h5 className="section-heading">Time Options</h5>
@@ -103,16 +60,16 @@ export class TimePickerSettings extends PureComponent<Props, State> {
             <label className="gf-form-label width-7">Timezone</label>
             <TimeZonePicker
               includeInternal={true}
-              value={dashboard.timezone}
+              value={this.props.timezone}
               onChange={this.onTimeZoneChange}
               width={40}
             />
           </div>
-
-          <div className="gf-form">
-            <span className="gf-form-label width-7">Auto-refresh</span>
-            <Input width={60} defaultValue={this.getRefreshIntervals()} onBlur={this.onRefreshIntervalChange} />
-          </div>
+          <AutoRefreshIntervals
+            renderCount={this.props.renderCount}
+            refreshIntervals={this.props.refreshIntervals}
+            onRefreshIntervalChange={this.props.onRefreshIntervalChange}
+          />
           <div className="gf-form">
             <span className="gf-form-label width-7">Now delay now-</span>
             <Tooltip
@@ -124,7 +81,7 @@ export class TimePickerSettings extends PureComponent<Props, State> {
                 invalid={!this.state.isNowDelayValid}
                 placeholder="0m"
                 onChange={this.onNowDelayChange}
-                defaultValue={dashboard.timepicker.nowDelay}
+                defaultValue={this.props.nowDelay}
               />
             </Tooltip>
           </div>
@@ -133,7 +90,7 @@ export class TimePickerSettings extends PureComponent<Props, State> {
             <LegacyForms.Switch
               labelClass="width-7"
               label="Hide time picker"
-              checked={dashboard.timepicker.hidden ?? false}
+              checked={this.props.timePickerHidden ?? false}
               onChange={this.onHideTimePickerChange}
             />
           </div>

public/app/features/dashboard/components/DashboardSettings/template.html

@@ -50,7 +50,18 @@
     </gf-form-switch>
   </div>
 
-  <time-picker-settings getDashboard="ctrl.getDashboard" onTimeZoneChange="ctrl.onTimeZoneChange" onRefreshIntervalChange="ctrl.onRefreshIntervalChange" onNowDelayChange="ctrl.onNowDelayChange" onHideTimePickerChange="ctrl.onHideTimePickerChange"></time-picker-settings>
+  <time-picker-settings
+    onTimeZoneChange="ctrl.onTimeZoneChange"
+    onRefreshIntervalChange="ctrl.onRefreshIntervalChange"
+    onNowDelayChange="ctrl.onNowDelayChange"
+    onHideTimePickerChange="ctrl.onHideTimePickerChange"
+    renderCount="ctrl.renderCount"
+    refreshIntervals="ctrl.dashboard.timepicker.refresh_intervals"
+    timePickerHidden="ctrl.dashboard.timepicker.hidden"
+    nowDelay="ctrl.dashboard.timepicker.nowDelay"
+    timezone="ctrl.dashboard.timezone"
+  >
+  </time-picker-settings>
 
   <h5 class="section-heading">Panel Options</h5>
   <div class="gf-form">