Do not allow to enter an empty relative time interval (#25542)

frontend/src/metabase/lib/time.ts

@@ -74,15 +74,16 @@ function addAbbreviatedLocale() {
   moment.locale(initialLocale);
 }
 
-export function checkIfTimeSpanTooGreat(
-  amount: DurationInputArg1,
-  key: DurationInputArg2,
-) {
+export function isValidTimeInterval(interval: number, unit: DurationInputArg2) {
+  if (!interval) {
+    return false;
+  }
+
   const now = moment();
-  const newTime = moment().add(amount, key);
+  const newTime = moment().add(interval, unit);
   const diff = now.diff(newTime, "years");
 
-  return Number.isNaN(diff);
+  return !Number.isNaN(diff);
 }
 
 export function formatFrame(frame: "first" | "last" | "mid") {

frontend/src/metabase/query_builder/components/filters/pickers/DatePicker/RelativeDatePicker.tsx

-/* eslint-disable react/prop-types */
 import React from "react";
 import { t } from "ttag";
 import { assoc } from "icepick";
@@ -13,7 +12,7 @@ import {
   setStartingFrom,
   toTimeInterval,
 } from "metabase/lib/query_time";
-import { checkIfTimeSpanTooGreat, getRelativeTime } from "metabase/lib/time";
+import { isValidTimeInterval } from "metabase/lib/time";
 import TippyPopover from "metabase/components/Popover/TippyPopover";
 
 import Filter from "metabase-lib/lib/queries/structured/Filter";
@@ -31,8 +30,8 @@ type RelativeDatePickerProps = {
   className?: string;
   filter: Filter;
   onFilterChange: (filter: any[]) => void;
-  formatter: (value: number) => number;
-  offsetFormatter: (value: number) => number;
+  formatter?: (value: number) => number;
+  offsetFormatter?: (value: number) => number;
   primaryColor?: string;
   reverseIconDirection?: boolean;
   supportsExpressions?: boolean;
@@ -100,14 +99,14 @@ function getCurrentIntervalName(filter: Filter) {
   return null;
 }
 
-const OptionsContent: React.FC<OptionsContentProps> = ({
+const OptionsContent = ({
   filter,
   primaryColor,
   onFilterChange,
   reverseIconDirection,
   setOptionsVisible,
   supportsExpressions,
-}) => {
+}: OptionsContentProps) => {
   const options = filter[4] || {};
   const includeCurrent = !!options["include-current"];
   const currentString = getCurrentString(filter);
@@ -152,7 +151,7 @@ const OptionsContent: React.FC<OptionsContentProps> = ({
   );
 };
 
-const RelativeDatePicker: React.FC<RelativeDatePickerProps> = props => {
+const RelativeDatePicker = (props: RelativeDatePickerProps) => {
   const {
     filter,
     onFilterChange,
@@ -175,15 +174,15 @@ const RelativeDatePicker: React.FC<RelativeDatePickerProps> = props => {
   );
 
   const handleChangeDateNumericInput = (newIntervals: number) => {
-    const timeSpanTooGreat = checkIfTimeSpanTooGreat(newIntervals, unit);
-    const valueToUse = timeSpanTooGreat ? intervals : newIntervals;
+    const isValid = isValidTimeInterval(newIntervals, unit);
+    const valueToUse = isValid ? newIntervals : Math.abs(intervals);
 
     onFilterChange(setRelativeDatetimeValue(filter, formatter(valueToUse)));
   };
 
   const handleChangeUnitInput = (newUnit: DurationInputArg2) => {
-    const timeSpanTooGreat = checkIfTimeSpanTooGreat(intervals, newUnit);
-    const unitToUse = timeSpanTooGreat ? unit : newUnit;
+    const isValid = isValidTimeInterval(intervals, newUnit);
+    const unitToUse = isValid ? newUnit : unit;
 
     onFilterChange(setRelativeDatetimeUnit(filter, unitToUse));
   };

frontend/src/metabase/query_builder/components/filters/pickers/DatePicker/RelativeDatePicker.unit.spec.tsx

+import React from "react";
+import { render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+import Filter from "metabase-lib/lib/queries/structured/Filter";
+import { PastPicker, NextPicker } from "./RelativeDatePicker";
+
+describe("PastPicker", () => {
+  it("should change a filter", () => {
+    const filter = new Filter(["time-interval", null, -10, "month"]);
+    const onFilterChange = jest.fn();
+
+    render(<PastPicker filter={filter} onFilterChange={onFilterChange} />);
+    typeByDisplayValue("10", "20");
+
+    const expectedFilter = ["time-interval", null, -20, "month"];
+    expect(onFilterChange).toHaveBeenCalledWith(expectedFilter);
+  });
+
+  it("should not allow to enter an empty time interval", () => {
+    const filter = new Filter(["time-interval", null, -10, "month"]);
+    const onFilterChange = jest.fn();
+
+    render(<PastPicker filter={filter} onFilterChange={onFilterChange} />);
+    typeByDisplayValue("10", "0");
+
+    expect(onFilterChange).toHaveBeenCalledWith(filter);
+  });
+});
+
+describe("NextPicker", () => {
+  it("should change a filter", () => {
+    const filter = new Filter(["time-interval", null, 10, "month"]);
+    const onFilterChange = jest.fn();
+
+    render(<NextPicker filter={filter} onFilterChange={onFilterChange} />);
+    typeByDisplayValue("10", "20");
+
+    const expectedFilter = ["time-interval", null, 20, "month"];
+    expect(onFilterChange).toHaveBeenCalledWith(expectedFilter);
+  });
+
+  it("should not allow to enter an empty time interval", () => {
+    const filter = new Filter(["time-interval", null, 10, "month"]);
+    const onFilterChange = jest.fn();
+
+    render(<NextPicker filter={filter} onFilterChange={onFilterChange} />);
+    typeByDisplayValue("10", "0");
+
+    expect(onFilterChange).toHaveBeenCalledWith(filter);
+  });
+});
+
+const typeByDisplayValue = (label: string, value: string) => {
+  const input = screen.getByDisplayValue(label);
+  userEvent.clear(input);
+  userEvent.type(input, value);
+  userEvent.tab();
+};

frontend/test/metabase/lib/time.unit.spec.js

 import moment from "moment-timezone";
 import {
-  checkIfTimeSpanTooGreat,
-  parseTime,
-  parseTimestamp,
   getRelativeTimeAbbreviated,
-  msToSeconds,
-  msToMinutes,
-  msToHours,
   hoursToSeconds,
+  isValidTimeInterval,
+  msToHours,
+  msToMinutes,
+  msToSeconds,
+  parseTime,
+  parseTimestamp,
 } from "metabase/lib/time";
 
 describe("time", () => {
@@ -153,15 +153,17 @@ describe("time", () => {
     });
   });
 
-  describe("checkIfTimeSpanTooGreat", () => {
-    it(`returns false for small time spans`, () => {
-      const isTimeSpanTooGreat = checkIfTimeSpanTooGreat(10, "days");
-      expect(isTimeSpanTooGreat).toBeFalsy();
+  describe("isValidTimeInterval", () => {
+    it(`is not valid for 0 time span`, () => {
+      expect(isValidTimeInterval(0, "days")).toBeFalsy();
+    });
+
+    it(`is valid for small time spans`, () => {
+      expect(isValidTimeInterval(10, "days")).toBeTruthy();
     });
 
-    it(`returns truthy for large time spans`, () => {
-      const isTimeSpanTooGreat = checkIfTimeSpanTooGreat(1000000000, "years");
-      expect(isTimeSpanTooGreat).toBeTruthy();
+    it(`is not valid for large time spans`, () => {
+      expect(isValidTimeInterval(1000000000, "years")).toBeFalsy();
     });
   });
 });