From 27b784fbe905b938659af7fd711e4f2b8e085921 Mon Sep 17 00:00:00 2001 From: Dhiraj Barnwal Date: Wed, 31 May 2023 17:04:42 +0530 Subject: [PATCH] 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 --- .../time-controls/CustomTimeRangeInput.svelte | 4 +- .../time-controls/TimeControls.svelte | 41 ++++------- .../time-controls/TimeGrainSelector.svelte | 9 +-- .../SmallestTimeGrainSelector.svelte | 1 + web-common/src/lib/time/grains/index.spec.ts | 70 +++++++++++++++++++ web-common/src/lib/time/grains/index.ts | 49 ++++++++++--- web-local/test/ui/dashboards.spec.ts | 2 +- 7 files changed, 129 insertions(+), 47 deletions(-) diff --git a/web-common/src/features/dashboards/time-controls/CustomTimeRangeInput.svelte b/web-common/src/features/dashboards/time-controls/CustomTimeRangeInput.svelte index dccde5576f2..cad0bdde0ce 100644 --- a/web-common/src/features/dashboards/time-controls/CustomTimeRangeInput.svelte +++ b/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 @@
{#if error} -
+
{error}
{/if} diff --git a/web-common/src/features/dashboards/time-controls/TimeControls.svelte b/web-common/src/features/dashboards/time-controls/TimeControls.svelte index 4966b69a660..6f57eb97b35 100644 --- a/web-common/src/features/dashboards/time-controls/TimeControls.svelte +++ b/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 diff --git a/web-common/src/features/dashboards/time-controls/TimeGrainSelector.svelte b/web-common/src/features/dashboards/time-controls/TimeGrainSelector.svelte index 3fdb20e8ef9..81e03ce88ef 100644 --- a/web-common/src/features/dashboards/time-controls/TimeGrainSelector.svelte +++ b/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, diff --git a/web-common/src/features/metrics-views/workspace/config-parameters/SmallestTimeGrainSelector.svelte b/web-common/src/features/metrics-views/workspace/config-parameters/SmallestTimeGrainSelector.svelte index d1db6737375..e5b4a41341f 100644 --- a/web-common/src/features/metrics-views/workspace/config-parameters/SmallestTimeGrainSelector.svelte +++ b/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 diff --git a/web-common/src/lib/time/grains/index.spec.ts b/web-common/src/lib/time/grains/index.spec.ts index c428acf1c5d..b484e031661 100644 --- a/web-common/src/lib/time/grains/index.spec.ts +++ b/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); + }); + }); +}); diff --git a/web-common/src/lib/time/grains/index.ts b/web-common/src/lib/time/grains/index.ts index d7266c6d2f5..bd6574c2a94 100644 --- a/web-common/src/lib/time/grains/index.ts +++ b/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; } diff --git a/web-local/test/ui/dashboards.spec.ts b/web-local/test/ui/dashboards.spec.ts index 078e74aa8df..9238988c39b 100644 --- a/web-local/test/ui/dashboards.spec.ts +++ b/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();