Fix bug when min time grain is set (#2517)

* Fix bug when min time grain is set

* Simplify TimeControls

* Add test case for no minTimeGrain

* Remove invalid time grain from e2e test

web-common/src/features/dashboards/time-controls/CustomTimeRangeInput.svelte

@@ -50,7 +50,7 @@
     if (start > end) {
       return "Start date must be before end date";
     } else if (!isGrainPossible) {
-      return "Range is smaller than min time grain";
+      return "Range is not valid for given min time grain";
     } else {
       return undefined;
     }
@@ -108,7 +108,7 @@
   </div>
   <div class="flex mt-3 items-center">
     {#if error}
-      <div style:font-size="11px" class="text-red-600">
+      <div style:font-size="11px" class="text-red-600 mr-2">
         {error}
       </div>
     {/if}

web-common/src/features/dashboards/time-controls/TimeControls.svelte

@@ -8,14 +8,12 @@
     getAvailableComparisonsForTimeRange,
     getComparisonRange,
   } from "@rilldata/web-common/lib/time/comparisons";
-  import {
-    DEFAULT_TIME_RANGES,
-    TIME_GRAIN,
-  } from "@rilldata/web-common/lib/time/config";
+  import { DEFAULT_TIME_RANGES } from "@rilldata/web-common/lib/time/config";
   import {
     checkValidTimeGrain,
     getDefaultTimeGrain,
-    getTimeGrainOptions,
+    findValidTimeGrain,
+    getAllowedTimeGrains,
   } from "@rilldata/web-common/lib/time/grains";
   import {
     ISODurationToTimePreset,
@@ -25,7 +23,7 @@
   import {
     DashboardTimeControls,
     TimeComparisonOption,
-    TimeGrainOption,
+    TimeGrain,
     TimeRange,
     TimeRangePreset,
     TimeRangeType,
@@ -157,9 +155,8 @@
 
   // we get the timeGrainOptions so that we can assess whether or not the
   // activeTimeGrain is valid whenever the baseTimeRange changes
-  let timeGrainOptions: TimeGrainOption[];
-  // FIXME: we should be deprecating this getTimeGrainOptions in favor of getAllowedTimeGrains.
-  $: timeGrainOptions = getTimeGrainOptions(
+  let timeGrainOptions: TimeGrain[];
+  $: timeGrainOptions = getAllowedTimeGrains(
     new Date($dashboardStore?.selectedTimeRange?.start),
     new Date($dashboardStore?.selectedTimeRange?.end)
   );
@@ -211,33 +208,21 @@
     const { name, start, end } = timeRange;
 
     // validate time range name + time grain combination
-    // (necessary because when the time range name is changed, the current time grain may not be valid for the new time range name)
-    timeGrainOptions = getTimeGrainOptions(start, end);
+    // (necessary because when the time range name is changed, the default time grain may not be valid for the new time range name)
+    timeGrainOptions = getAllowedTimeGrains(start, end);
     const isValidTimeGrain = checkValidTimeGrain(
       timeGrain,
       timeGrainOptions,
       minTimeGrain
     );
+
     if (!isValidTimeGrain) {
       const defaultTimeGrain = getDefaultTimeGrain(start, end).grain;
-      const timeGrainEnums = Object.values(TIME_GRAIN).map(
-        (timeGrain) => timeGrain.grain
+      timeGrain = findValidTimeGrain(
+        defaultTimeGrain,
+        timeGrainOptions,
+        minTimeGrain
       );
-
-      const defaultGrainIndex = timeGrainEnums.indexOf(defaultTimeGrain);
-      timeGrain = defaultTimeGrain;
-      let i = defaultGrainIndex;
-      // loop through time grains until we find a valid one
-      while (!checkValidTimeGrain(timeGrain, timeGrainOptions, minTimeGrain)) {
-        timeGrain = timeGrainEnums[i + 1] as V1TimeGrain;
-        i = i == timeGrainEnums.length - 1 ? -1 : i + 1;
-        if (i == defaultGrainIndex) {
-          // if we've looped through all the time grains and haven't found
-          // a valid one, use default
-          timeGrain = defaultTimeGrain;
-          break;
-        }
-      }
     }
 
     // the adjusted time range

web-common/src/features/dashboards/time-controls/TimeGrainSelector.svelte

@@ -4,13 +4,13 @@
   import WithSelectMenu from "@rilldata/web-common/components/menu/wrappers/WithSelectMenu.svelte";
   import { TIME_GRAIN } from "@rilldata/web-common/lib/time/config";
   import { isGrainBigger } from "@rilldata/web-common/lib/time/grains";
-  import type { TimeGrainOption } from "@rilldata/web-common/lib/time/types";
+  import type { TimeGrain } from "@rilldata/web-common/lib/time/types";
   import { createEventDispatcher } from "svelte";
   import type { V1TimeGrain } from "../../../runtime-client";
   import { useDashboardStore } from "../dashboard-stores";
 
   export let metricViewName: string;
-  export let timeGrainOptions: TimeGrainOption[];
+  export let timeGrainOptions: TimeGrain[];
   export let minTimeGrain: V1TimeGrain;
 
   const dispatch = createEventDispatcher();
@@ -21,10 +21,7 @@
 
   $: timeGrains = timeGrainOptions
     ? timeGrainOptions
-        .filter(
-          (timeGrain) =>
-            !isGrainBigger(minTimeGrain, timeGrain.grain) && timeGrain.enabled
-        )
+        .filter((timeGrain) => !isGrainBigger(minTimeGrain, timeGrain.grain))
         .map((timeGrain) => {
           return {
             main: timeGrain.label,

web-common/src/features/metrics-views/workspace/config-parameters/SmallestTimeGrainSelector.svelte

@@ -57,6 +57,7 @@
   let selectableTimeGrains: TimeGrainOption[] = [];
   let maxTimeGrainPossibleIndex = 0;
   $: if (allTimeRange) {
+    // FIXME: we should be deprecating this getTimeGrainOptions in favor of getAllowedTimeGrains.
     selectableTimeGrains = getTimeGrainOptions(
       allTimeRange.start,
       allTimeRange.end

web-common/src/lib/time/grains/index.spec.ts

@@ -1,10 +1,13 @@
+import { V1TimeGrain } from "../../../runtime-client";
 import { TIME_GRAIN } from "../config";
 import {
   durationToMillis,
+  findValidTimeGrain,
   getAllowedTimeGrains,
   getDefaultTimeGrain,
 } from "../grains";
 import { describe, it, expect } from "vitest";
+import { Period, TimeGrain } from "../types";
 
 const allowedGrainTests = [
   {
@@ -29,6 +32,12 @@ const allowedGrainTests = [
     end: new Date(durationToMillis(TIME_GRAIN.TIME_GRAIN_DAY.duration) - 1),
     expected: [TIME_GRAIN.TIME_GRAIN_HOUR],
   },
+  {
+    test: "should return TIME_GRAIN_HOUR for 24 hours",
+    start: new Date(0),
+    end: new Date(durationToMillis(TIME_GRAIN.TIME_GRAIN_DAY.duration)),
+    expected: [TIME_GRAIN.TIME_GRAIN_HOUR],
+  },
   {
     test: "should return TIME_GRAIN_HOUR and TIME_GRAIN_DAY if otherwise < 14 days",
     start: new Date(0),
@@ -124,6 +133,54 @@ const defaultTimeGrainTests = [
   },
 ];
 
+const timeGrainOptions: TimeGrain[] = [
+  {
+    grain: V1TimeGrain.TIME_GRAIN_DAY,
+    label: "day",
+    duration: Period.DAY,
+    formatDate: {},
+  },
+  {
+    grain: V1TimeGrain.TIME_GRAIN_WEEK,
+    label: "week",
+    duration: Period.WEEK,
+    formatDate: {},
+  },
+  {
+    grain: V1TimeGrain.TIME_GRAIN_MONTH,
+    label: "month",
+    duration: Period.MONTH,
+    formatDate: {},
+  },
+];
+
+const findValidTimeGrainTests = [
+  {
+    test: "findValidTimeGrain returns a valid time grain",
+    timeGrain: V1TimeGrain.TIME_GRAIN_WEEK,
+    minTimeGrain: V1TimeGrain.TIME_GRAIN_WEEK,
+    expected: V1TimeGrain.TIME_GRAIN_WEEK,
+  },
+  {
+    test: "findValidTimeGrain returns a valid time grain when there is no minTimeGrain",
+    timeGrain: V1TimeGrain.TIME_GRAIN_HOUR,
+    minTimeGrain: undefined,
+    expected: V1TimeGrain.TIME_GRAIN_DAY,
+  },
+  {
+    test: "findValidTimeGrain returns the default time grain as fallback",
+    timeGrain: V1TimeGrain.TIME_GRAIN_WEEK,
+    minTimeGrain: V1TimeGrain.TIME_GRAIN_HOUR,
+    expected: V1TimeGrain.TIME_GRAIN_WEEK,
+  },
+  {
+    test: "findValidTimeGrain finds and returns a valid time grain",
+    timeGrain: V1TimeGrain.TIME_GRAIN_DAY,
+    minTimeGrain: V1TimeGrain.TIME_GRAIN_WEEK,
+    expected: V1TimeGrain.TIME_GRAIN_WEEK,
+  },
+];
+
 describe("getAllowedTimeGrains", () => {
   allowedGrainTests.forEach((testCase) => {
     it(testCase.test, () => {
@@ -147,3 +204,16 @@ describe("getDefaultTimeGrain", () => {
     });
   });
 });
+
+describe("findValidTimeGrain", () => {
+  findValidTimeGrainTests.forEach((testCase) => {
+    it(testCase.test, () => {
+      const defaultTimeGrain = findValidTimeGrain(
+        testCase.timeGrain,
+        timeGrainOptions,
+        testCase.minTimeGrain
+      );
+      expect(defaultTimeGrain).toEqual(testCase.expected);
+    });
+  });
+});

web-common/src/lib/time/grains/index.ts

@@ -88,11 +88,11 @@ export function getAllowedTimeGrains(start: Date, end: Date): TimeGrain[] {
   ) {
     return [TIME_GRAIN.TIME_GRAIN_MINUTE, TIME_GRAIN.TIME_GRAIN_HOUR];
   } else if (
-    timeRangeDurationMs < durationToMillis(TIME_GRAIN.TIME_GRAIN_DAY.duration)
+    timeRangeDurationMs <= durationToMillis(TIME_GRAIN.TIME_GRAIN_DAY.duration)
   ) {
     return [TIME_GRAIN.TIME_GRAIN_HOUR];
   } else if (
-    timeRangeDurationMs <
+    timeRangeDurationMs <=
     14 * durationToMillis(TIME_GRAIN.TIME_GRAIN_DAY.duration)
   ) {
     return [TIME_GRAIN.TIME_GRAIN_HOUR, TIME_GRAIN.TIME_GRAIN_DAY];
@@ -140,19 +140,48 @@ export function isGrainBigger(
   );
 }
 
-//TODO: Simplify use of this method
 export function checkValidTimeGrain(
   timeGrain: V1TimeGrain,
-  timeGrainOptions: TimeGrainOption[],
+  timeGrainOptions: TimeGrain[],
   minTimeGrain: V1TimeGrain
 ): boolean {
-  const timeGrainOption = timeGrainOptions.find(
-    (timeGrainOption) => timeGrainOption.grain === timeGrain
-  );
+  if (!timeGrainOptions.find((t) => t.grain === timeGrain)) return false;
 
-  if (minTimeGrain === V1TimeGrain.TIME_GRAIN_UNSPECIFIED)
-    return timeGrainOption?.enabled;
+  // If minTimeGrain is not specified, then all available timeGrains are valid
+  if (minTimeGrain === V1TimeGrain.TIME_GRAIN_UNSPECIFIED) return true;
 
   const isGrainPossible = !isGrainBigger(minTimeGrain, timeGrain);
-  return timeGrainOption?.enabled && isGrainPossible;
+  return isGrainPossible;
+}
+
+export function findValidTimeGrain(
+  timeGrain: V1TimeGrain,
+  timeGrainOptions: TimeGrain[],
+  minTimeGrain: V1TimeGrain
+): V1TimeGrain {
+  const timeGrains = Object.values(TIME_GRAIN).map(
+    (timeGrain) => timeGrain.grain
+  );
+
+  const defaultIndex = timeGrains.indexOf(timeGrain);
+
+  // Loop through the timeGrains starting from the default value
+  for (let i = defaultIndex; i < timeGrains.length; i++) {
+    const currentGrain = timeGrains[i];
+
+    if (checkValidTimeGrain(currentGrain, timeGrainOptions, minTimeGrain)) {
+      return currentGrain;
+    }
+  }
+  // If no valid timeGrain is found, loop from the beginning of the array
+  for (let i = 0; i < defaultIndex; i++) {
+    const currentGrain = timeGrains[i];
+
+    if (checkValidTimeGrain(currentGrain, timeGrainOptions, minTimeGrain)) {
+      return currentGrain;
+    }
+  }
+
+  // If no valid timeGrain is found, return the default timeGrain as fallback
+  return timeGrain;
 }

web-local/test/ui/dashboards.spec.ts

@@ -103,7 +103,7 @@ describe("dashboards", () => {
 
     // Change the metric trend granularity
     await page.getByRole("button", { name: "Metric trends by day" }).click();
-    await page.getByRole("menuitem", { name: "hour" }).click();
+    await page.getByRole("menuitem", { name: "day" }).click();
 
     // Change the time range
     await page.getByLabel("Select time range").click();