fix(cache): Fix two bugs in Admin / Performance (#44255)

Ensures that minimum query durations are correctly displayed in the form post-save (#44249) and that all times can be used in the Schedule component  (#44257)

enterprise/frontend/src/metabase-enterprise/caching/components/InvalidateNowButton.tsx

@@ -3,18 +3,12 @@ import { useCallback } from "react";
 import { c, t } from "ttag";
 
 import { IconInButton } from "metabase/admin/performance/components/StrategyForm.styled";
+import { useInvalidateTarget } from "metabase/admin/performance/hooks/useInvalidateTarget";
 import { useIsFormPending } from "metabase/admin/performance/hooks/useIsFormPending";
-import {
-  isErrorWithMessage,
-  resolveSmoothly,
-} from "metabase/admin/performance/utils";
 import { Form, FormProvider } from "metabase/forms";
 import { useConfirmation } from "metabase/hooks/use-confirmation";
 import { color } from "metabase/lib/colors";
-import { useDispatch } from "metabase/lib/redux";
 import type { InvalidateNowButtonProps } from "metabase/plugins";
-import { addUndo } from "metabase/redux/undo";
-import { CacheConfigApi } from "metabase/services";
 import { Group, Icon, Loader, Text } from "metabase/ui";
 
 import { StyledInvalidateNowButton } from "./InvalidateNowButton.styled";
@@ -24,30 +18,7 @@ export const InvalidateNowButton = ({
   targetModel,
   targetName,
 }: InvalidateNowButtonProps) => {
-  const dispatch = useDispatch();
-
-  const invalidateTarget = useCallback(async () => {
-    try {
-      const invalidate = CacheConfigApi.invalidate(
-        { include: "overrides", [targetModel]: targetId },
-        { hasBody: false },
-      );
-      await resolveSmoothly([invalidate]);
-    } catch (e) {
-      if (isErrorWithMessage(e)) {
-        dispatch(
-          addUndo({
-            icon: "warning",
-            message: e.data.message,
-            toastColor: "error",
-            dismissIconColor: color("text-white"),
-          }),
-        );
-      }
-      throw e;
-    }
-  }, [dispatch, targetId, targetModel]);
-
+  const invalidateTarget = useInvalidateTarget(targetId, targetModel);
   return (
     <FormProvider initialValues={{}} onSubmit={invalidateTarget}>
       <InvalidateNowFormBody targetName={targetName} />
@@ -64,7 +35,9 @@ const InvalidateNowFormBody = ({ targetName }: { targetName?: string }) => {
   const confirmInvalidation = useCallback(
     () =>
       askConfirmation({
-        title: t`Clear all cached results for ${targetName || t`this object`}?`,
+        title: targetName
+          ? t`Clear all cached results for ${targetName}`
+          : t`Clear all cached results for this object?`,
         message: "",
         confirmButtonText: t`Clear cache`,
         onConfirm: submitForm,

frontend/src/metabase/admin/performance/components/StrategyEditorForDatabases.styled.tsx

@@ -28,7 +28,7 @@ export const RoundedBox = styled.div<{ twoColumns?: boolean }>`
   border: 2px solid var(--mb-color-border);
 `;
 
-export const TabWrapper = styled.div`
+export const TabWrapper = styled.main`
   display: grid;
   grid-template-rows: auto 1fr;
   width: calc(min(65rem, 100vw));

frontend/src/metabase/admin/performance/components/StrategyEditorForDatabases.tsx

@@ -127,7 +127,7 @@ const StrategyEditorForDatabases_Base = ({
   }
 
   return (
-    <TabWrapper role="region" aria-label={t`Data caching settings`}>
+    <TabWrapper aria-label={t`Database caching settings`}>
       <Stack spacing="xl" lh="1.5rem" maw="32rem" mb="1.5rem">
         <aside>
           {t`Speed up queries by caching their results.`}

frontend/src/metabase/admin/performance/components/StrategyForm.tsx

@@ -163,9 +163,14 @@ const StrategyFormBody = ({
     }
   }, [selectedStrategyType, values, setFieldValue]);
 
+  const headingId = "strategy-form-heading";
+
   return (
     <FormWrapper>
-      <StyledForm style={{ overflow: isInSidebar ? undefined : "auto" }}>
+      <StyledForm
+        style={{ overflow: isInSidebar ? undefined : "auto" }}
+        aria-labelledby={headingId}
+      >
         <FormBox>
           {shouldShowName && (
             <Box lh="1rem" pt="md" color="text-medium">
@@ -180,7 +185,11 @@ const StrategyFormBody = ({
             </Box>
           )}
           <Stack maw="35rem" pt={targetId === rootId ? "xl" : 0} spacing="xl">
-            <StrategySelector targetId={targetId} model={targetModel} />
+            <StrategySelector
+              targetId={targetId}
+              model={targetModel}
+              headingId={headingId}
+            />
             {selectedStrategyType === "ttl" && (
               <>
                 <Field
@@ -375,9 +384,11 @@ const SaveAndDiscardButtons = ({
 const StrategySelector = ({
   targetId,
   model,
+  headingId,
 }: {
   targetId: number | null;
   model?: CacheableModel;
+  headingId: string;
 }) => {
   const { strategies } = PLUGIN_CACHING;
 
@@ -392,7 +403,7 @@ const StrategySelector = ({
       <FormRadioGroup
         label={
           <Stack spacing="xs">
-            <Text lh="1rem" color="text-medium">
+            <Text lh="1rem" color="text-medium" id={headingId}>
               {t`Select the cache invalidation policy`}
             </Text>
             <Text lh="1rem" fw="normal" size="sm" color="text-medium">
@@ -419,6 +430,7 @@ const StrategySelector = ({
                 key={name}
                 label={optionLabelFormatted}
                 autoFocus={values.type === name}
+                role="radio"
               />
             );
           })}

frontend/src/metabase/admin/performance/hooks/useInvalidateTarget.tsx

+import { useCallback } from "react";
+
+import { useDispatch } from "metabase/lib/redux";
+import { addUndo } from "metabase/redux/undo";
+import { CacheConfigApi } from "metabase/services";
+import type { CacheableModel } from "metabase-types/api";
+
+import { resolveSmoothly, isErrorWithMessage } from "../utils";
+
+export const useInvalidateTarget = (
+  targetId: number | null,
+  targetModel: CacheableModel,
+  { smooth = true, shouldThrowErrors = true } = {},
+) => {
+  const dispatch = useDispatch();
+  const invalidateTarget = useCallback(async () => {
+    if (targetId === null) {
+      return;
+    }
+    try {
+      const invalidate = CacheConfigApi.invalidate(
+        { include: "overrides", [targetModel]: targetId },
+        { hasBody: false },
+      );
+      if (smooth) {
+        await resolveSmoothly([invalidate]);
+      } else {
+        await invalidate;
+      }
+    } catch (e) {
+      if (isErrorWithMessage(e)) {
+        dispatch(
+          addUndo({
+            icon: "warning",
+            message: e.data.message,
+            toastColor: "error",
+            dismissIconColor: "var(--mb-color-text-white)",
+          }),
+        );
+      }
+      if (shouldThrowErrors) {
+        throw e;
+      }
+    }
+  }, [dispatch, targetId, targetModel, smooth, shouldThrowErrors]);
+  return invalidateTarget;
+};

frontend/src/metabase/admin/performance/hooks/useSaveStrategy.tsx

@@ -11,7 +11,11 @@ import type {
 } from "metabase-types/api";
 
 import { rootId } from "../constants/simple";
-import { getFieldsForStrategyType, translateConfigToAPI } from "../utils";
+import {
+  getFieldsForStrategyType,
+  populateMinDurationSeconds,
+  translateConfigToAPI,
+} from "../utils";
 
 export const useSaveStrategy = (
   targetId: number | null,
@@ -52,13 +56,18 @@ export const useSaveStrategy = (
         const validatedStrategy =
           strategies[values.type].validateWith.validateSync(newStrategy);
 
-        const newConfig = {
+        const newConfig: CacheConfig = {
           ...baseConfig,
           strategy: validatedStrategy,
         };
 
         const translatedConfig = translateConfigToAPI(newConfig);
         await CacheConfigApi.update(translatedConfig);
+
+        if (newConfig.strategy.type === "ttl") {
+          newConfig.strategy = populateMinDurationSeconds(newConfig.strategy);
+        }
+
         setConfigs([...otherConfigs, newConfig]);
       }
     },

frontend/src/metabase/admin/performance/utils.tsx

+import dayjs from "dayjs";
 import { memoize } from "underscore";
 import type { SchemaObjectDescription } from "yup/lib/schema";
 
@@ -9,6 +10,7 @@ import {
 import { isNullOrUndefined } from "metabase/lib/types";
 import { PLUGIN_CACHING } from "metabase/plugins";
 import type {
+  AdaptiveStrategy,
   CacheConfig,
   CacheStrategy,
   CacheStrategyType,
@@ -22,6 +24,9 @@ import type {
 import { defaultMinDurationMs, rootId } from "./constants/simple";
 import type { StrategyLabel } from "./types";
 
+const AM = 0;
+const PM = 1;
+
 const dayToCron = (day: ScheduleSettings["schedule_day"]) => {
   const index = weekdays.findIndex(o => o.value === day);
   if (index === -1) {
@@ -153,9 +158,17 @@ const defaultSchedule: ScheduleSettings = {
 };
 export const defaultCron = scheduleSettingsToCron(defaultSchedule);
 
+const isValidAmPm = (amPm: number) => amPm === AM || amPm === PM;
+
 export const hourToTwelveHourFormat = (hour: number) => hour % 12 || 12;
-export const hourTo24HourFormat = (hour: number, amPm: number) =>
-  hour + amPm * 12;
+export const hourTo24HourFormat = (hour: number, amPm: number): number => {
+  if (!isValidAmPm(amPm)) {
+    amPm = AM;
+  }
+  const amPmString = amPm === AM ? "AM" : "PM";
+  const convertedString = dayjs(`${hour} ${amPmString}`, "h A").format("HH");
+  return parseInt(convertedString);
+};
 
 type ErrorWithMessage = { data: { message: string } };
 export const isErrorWithMessage = (error: unknown): error is ErrorWithMessage =>
@@ -239,9 +252,7 @@ export const translateConfig = (
 
   if (translated.strategy.type === "ttl") {
     if (direction === "fromAPI") {
-      translated.strategy.min_duration_seconds = Math.ceil(
-        translated.strategy.min_duration_ms / 1000,
-      );
+      translated.strategy = populateMinDurationSeconds(translated.strategy);
     } else {
       translated.strategy.min_duration_ms =
         translated.strategy.min_duration_seconds === undefined
@@ -253,7 +264,15 @@ export const translateConfig = (
   return translated;
 };
 
+export const populateMinDurationSeconds = (strategy: AdaptiveStrategy) => ({
+  ...strategy,
+  min_duration_seconds: Math.ceil(strategy.min_duration_ms / 1000),
+});
+
+/** Translate a config from the API into a format the frontend can use */
 export const translateConfigFromAPI = (config: CacheConfig): CacheConfig =>
   translateConfig(config, "fromAPI");
+
+/** Translate a config from the frontend's format into the API's preferred format */
 export const translateConfigToAPI = (config: CacheConfig): CacheConfig =>
   translateConfig(config, "toAPI");

frontend/src/metabase/admin/performance/utils.unit.spec.ts

@@ -2,6 +2,7 @@ import type { ScheduleSettings } from "metabase-types/api";
 
 import {
   cronToScheduleSettings,
+  hourTo24HourFormat,
   hourToTwelveHourFormat,
   scheduleSettingsToCron,
 } from "./utils";
@@ -226,3 +227,52 @@ describe("hourToTwelveHourFormat", () => {
     expect(hourToTwelveHourFormat(1)).toBe(1);
   });
 });
+
+describe("hourTo24HourFormat", () => {
+  // Test AM cases
+  it("converts 12 AM to 0", () => {
+    expect(hourTo24HourFormat(12, 0)).toBe(0);
+  });
+
+  it("converts 1 AM to 1", () => {
+    expect(hourTo24HourFormat(1, 0)).toBe(1);
+  });
+
+  it("converts 11 AM to 11", () => {
+    expect(hourTo24HourFormat(11, 0)).toBe(11);
+  });
+
+  // Test PM cases
+  it("converts 12 PM to 12", () => {
+    expect(hourTo24HourFormat(12, 1)).toBe(12);
+  });
+
+  it("converts 1 PM to 13", () => {
+    expect(hourTo24HourFormat(1, 1)).toBe(13);
+  });
+
+  it("converts 11 PM to 23", () => {
+    expect(hourTo24HourFormat(11, 1)).toBe(23);
+  });
+
+  // Edge cases
+  it("converts 0 AM to 0", () => {
+    expect(hourTo24HourFormat(0, 0)).toBe(0);
+  });
+
+  it("converts 0 PM to 12", () => {
+    expect(hourTo24HourFormat(0, 1)).toBe(12);
+  });
+
+  it("converts NaN PM to NaN", () => {
+    expect(hourTo24HourFormat(NaN, 1)).toBeNaN();
+  });
+
+  it("converts 11 NaN to 11 (that is, fall back to AM if the AM/PM is NaN)", () => {
+    expect(hourTo24HourFormat(11, NaN)).toBe(11);
+  });
+
+  it("converts NaN NaN to NaN", () => {
+    expect(hourTo24HourFormat(NaN, NaN)).toBeNaN();
+  });
+});

frontend/src/metabase/components/Schedule/components.tsx

@@ -70,6 +70,7 @@ export const SelectTime = ({
   const hourIndex = isClock12Hour && hour === 12 ? 0 : hour;
   return (
     <Group spacing={isClock12Hour ? "xs" : "sm"} style={{ rowGap: ".5rem" }}>
+      {/* Select the hour */}
       <AutoWidthSelect
         value={hourIndex.toString()}
         data={getHours()}
@@ -81,13 +82,17 @@ export const SelectTime = ({
           );
         }}
       />
+      {/* Choose between AM and PM */}
       <Group spacing="sm">
         {isClock12Hour && (
           <SegmentedControl
             radius="sm"
             value={amPm.toString()}
             onChange={value =>
-              updateSchedule("schedule_hour", hour + Number(value) * 12)
+              updateSchedule(
+                "schedule_hour",
+                hourTo24HourFormat(hour, parseInt(value)),
+              )
             }
             data={amAndPM}
           />

frontend/src/metabase/components/Schedule/utils.tsx

@@ -74,5 +74,3 @@ export const getLongestSelectLabel = (data: SelectProps["data"]) =>
     const label = typeof option === "string" ? option : option.label || "";
     return label.length > acc.length ? label : acc;
   }, "");
-
-// HIIII