From d87e086d6f4e5877cad20449a298382ec072146d Mon Sep 17 00:00:00 2001 From: fabio-silva Date: Tue, 6 Jul 2021 09:46:17 +0100 Subject: [PATCH] Dashboard: avoid and correct invalid date ranges (i.e. to < from) (#34543) * Dashboard: invert invalid date ranges from URL * Dashboard: discard invalid date ranges from inputs * Dashboard: move time range reset * Dashboard: simplify undefined dashboard verification * Dashboard: show form error on invalid date range * Dashboard: rename function to isRangeInvalid * Dashboard: refactor invalid check functions * Dashboard: different error messages for date picker * Dashboard: add date tests to TimeRangeForm --- .../TimeRangePicker/TimeRangeForm.test.tsx | 52 +++++++++++++++ .../TimeRangePicker/TimeRangeForm.tsx | 63 +++++++++++++------ .../dashboard/services/TimeSrv.test.ts | 22 +++++++ .../features/dashboard/services/TimeSrv.ts | 12 ++++ 4 files changed, 131 insertions(+), 18 deletions(-) diff --git a/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeForm.test.tsx b/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeForm.test.tsx index d96d2b14839..ce39c06511a 100644 --- a/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeForm.test.tsx +++ b/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeForm.test.tsx @@ -96,4 +96,56 @@ describe('TimeRangeForm', () => { expect(from).toHaveClass('react-calendar__tile--rangeStart'); expect(to).toHaveClass('react-calendar__tile--rangeEnd'); }); + + describe('dates error handling', () => { + it('should show error on invalid dates', () => { + const invalidTimeRange: TimeRange = { + from: dateTimeParse('foo', { timeZone: 'utc' }), + to: dateTimeParse('2021-06-19 23:59:00', { timeZone: 'utc' }), + raw: { + from: 'foo', + to: '2021-06-19 23:59:00', + }, + }; + const { getAllByRole } = setup(invalidTimeRange, 'Asia/Tokyo'); + const error = getAllByRole('alert'); + + expect(error).toHaveLength(1); + expect(error[0]).toBeVisible(); + expect(error[0]).toHaveTextContent('Please enter a past date or "now"'); + }); + + it('should show error on invalid range', () => { + const invalidTimeRange: TimeRange = { + from: dateTimeParse('2021-06-19 00:00:00', { timeZone: 'utc' }), + to: dateTimeParse('2021-06-17 23:59:00', { timeZone: 'utc' }), + raw: { + from: '2021-06-19 00:00:00', + to: '2021-06-17 23:59:00', + }, + }; + const { getAllByRole } = setup(invalidTimeRange, 'Asia/Tokyo'); + const error = getAllByRole('alert'); + + expect(error[0]).toBeVisible(); + expect(error[0]).toHaveTextContent('"From" can\'t be after "To"'); + }); + + it('should not show range error when "to" is invalid', () => { + const invalidTimeRange: TimeRange = { + from: dateTimeParse('2021-06-19 00:00:00', { timeZone: 'utc' }), + to: dateTimeParse('foo', { timeZone: 'utc' }), + raw: { + from: '2021-06-19 00:00:00', + to: 'foo', + }, + }; + const { getAllByRole } = setup(invalidTimeRange, 'Asia/Tokyo'); + const error = getAllByRole('alert'); + + expect(error).toHaveLength(1); + expect(error[0]).toBeVisible(); + expect(error[0]).toHaveTextContent('Please enter a past date or "now"'); + }); + }); }); diff --git a/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeForm.tsx b/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeForm.tsx index b5c68ab12b0..dbad82eceeb 100644 --- a/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeForm.tsx +++ b/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeForm.tsx @@ -28,21 +28,27 @@ interface Props { interface InputState { value: string; invalid: boolean; + errorMessage: string; } -const errorMessage = 'Please enter a past date or "now"'; +const ERROR_MESSAGES = { + default: 'Please enter a past date or "now"', + range: '"From" can\'t be after "To"', +}; export const TimeRangeForm: React.FC = (props) => { const { value, isFullscreen = false, timeZone, onApply: onApplyFromProps, isReversed } = props; + const [fromValue, toValue] = valueToState(value.raw.from, value.raw.to, timeZone); - const [from, setFrom] = useState(valueToState(value.raw.from, false, timeZone)); - const [to, setTo] = useState(valueToState(value.raw.to, true, timeZone)); + const [from, setFrom] = useState(fromValue); + const [to, setTo] = useState(toValue); const [isOpen, setOpen] = useState(false); // Synchronize internal state with external value useEffect(() => { - setFrom(valueToState(value.raw.from, false, timeZone)); - setTo(valueToState(value.raw.to, true, timeZone)); + const [fromValue, toValue] = valueToState(value.raw.from, value.raw.to, timeZone); + setFrom(fromValue); + setTo(toValue); }, [value.raw.from, value.raw.to, timeZone]); const onOpen = useCallback( @@ -79,9 +85,10 @@ export const TimeRangeForm: React.FC = (props) => { ); const onChange = useCallback( - (from: DateTime, to: DateTime) => { - setFrom(valueToState(from, false, timeZone)); - setTo(valueToState(to, true, timeZone)); + (from: DateTime | string, to: DateTime | string) => { + const [fromValue, toValue] = valueToState(from, to, timeZone); + setFrom(fromValue); + setTo(toValue); }, [timeZone] ); @@ -90,21 +97,21 @@ export const TimeRangeForm: React.FC = (props) => { return ( <> - + event.stopPropagation()} onFocus={onFocus} - onChange={(event) => setFrom(eventToState(event, false, timeZone))} + onChange={(event) => onChange(event.currentTarget.value, to.value)} addonAfter={icon} aria-label={selectors.components.TimePicker.fromField} value={from.value} /> - + event.stopPropagation()} onFocus={onFocus} - onChange={(event) => setTo(eventToState(event, true, timeZone))} + onChange={(event) => onChange(from.value, event.currentTarget.value)} addonAfter={icon} aria-label={selectors.components.TimePicker.toField} value={to.value} @@ -129,14 +136,34 @@ export const TimeRangeForm: React.FC = (props) => { ); }; -function eventToState(event: FormEvent, roundup?: boolean, timeZone?: TimeZone): InputState { - return valueToState(event.currentTarget.value, roundup, timeZone); +function isRangeInvalid(from: string, to: string, timezone?: string): boolean { + const raw: RawTimeRange = { from, to }; + const timeRange = rangeUtil.convertRawToRange(raw, timezone); + const valid = timeRange.from.isSame(timeRange.to) || timeRange.from.isBefore(timeRange.to); + + return !valid; } -function valueToState(raw: DateTime | string, roundup?: boolean, timeZone?: TimeZone): InputState { - const value = valueAsString(raw, timeZone); - const invalid = !isValid(value, roundup, timeZone); - return { value, invalid }; +function valueToState( + rawFrom: DateTime | string, + rawTo: DateTime | string, + timeZone?: TimeZone +): [InputState, InputState] { + const fromValue = valueAsString(rawFrom, timeZone); + const toValue = valueAsString(rawTo, timeZone); + const fromInvalid = !isValid(fromValue, false, timeZone); + const toInvalid = !isValid(toValue, true, timeZone); + // If "To" is invalid, we should not check the range anyways + const rangeInvalid = isRangeInvalid(fromValue, toValue, timeZone) && !toInvalid; + + return [ + { + value: fromValue, + invalid: fromInvalid || rangeInvalid, + errorMessage: rangeInvalid && !fromInvalid ? ERROR_MESSAGES.range : ERROR_MESSAGES.default, + }, + { value: toValue, invalid: toInvalid, errorMessage: ERROR_MESSAGES.default }, + ]; } function valueAsString(value: DateTime | string, timeZone?: TimeZone): string { diff --git a/public/app/features/dashboard/services/TimeSrv.test.ts b/public/app/features/dashboard/services/TimeSrv.test.ts index cde22696f41..bbb7f4c52d2 100644 --- a/public/app/features/dashboard/services/TimeSrv.test.ts +++ b/public/app/features/dashboard/services/TimeSrv.test.ts @@ -163,6 +163,28 @@ describe('timeSrv', () => { expect(time.from.valueOf()).toEqual(1410337640000); expect(time.to.valueOf()).toEqual(1410337650000); }); + + it('corrects inverted from/to dates in ms', () => { + locationService.push('/d/id?from=1621436828909&to=1621436818909'); + + timeSrv = new TimeSrv(new ContextSrvStub() as any); + + timeSrv.init(_dashboard); + const time = timeSrv.timeRange(); + expect(time.from.valueOf()).toEqual(1621436818909); + expect(time.to.valueOf()).toEqual(1621436828909); + }); + + it('corrects inverted from/to dates as relative times', () => { + locationService.push('/d/id?from=now&to=now-1h'); + + timeSrv = new TimeSrv(new ContextSrvStub() as any); + + timeSrv.init(_dashboard); + const time = timeSrv.timeRange(); + expect(time.raw.from).toBe('now-1h'); + expect(time.raw.to).toBe('now'); + }); }); }); diff --git a/public/app/features/dashboard/services/TimeSrv.ts b/public/app/features/dashboard/services/TimeSrv.ts index 2bb7346138d..734af2c0f20 100644 --- a/public/app/features/dashboard/services/TimeSrv.ts +++ b/public/app/features/dashboard/services/TimeSrv.ts @@ -60,6 +60,18 @@ export class TimeSrv { // remember time at load so we can go back to it this.timeAtLoad = cloneDeep(this.time); + const range = rangeUtil.convertRawToRange(this.time, this.dashboard?.getTimezone()); + + if (range.to.isBefore(range.from)) { + this.setTime( + { + from: range.raw.to, + to: range.raw.from, + }, + false + ); + } + if (this.refresh) { this.setAutoRefresh(this.refresh); }