From ca2957395843d53b49f52ed4d3efe1688344d0d5 Mon Sep 17 00:00:00 2001
From: Raphael Krut-Landau <raphael.kl@gmail.com>
Date: Wed, 28 Aug 2024 10:10:03 -0400
Subject: [PATCH] fix(admin/performance): Ensure that the strategy form is not
considered dirty if the user changes an empty field to that field's default
value, or vice versa (#46543)
---
.../StrategyEditorForDatabases.unit.spec.tsx | 13 ++++
.../performance/components/StrategyForm.tsx | 69 ++++++++++++++-----
.../src/metabase/admin/performance/utils.tsx | 10 +++
3 files changed, 75 insertions(+), 17 deletions(-)
diff --git a/frontend/src/metabase/admin/performance/components/StrategyEditorForDatabases.unit.spec.tsx b/frontend/src/metabase/admin/performance/components/StrategyEditorForDatabases.unit.spec.tsx
index 846381ecc52..ba9d11bbc6a 100644
--- a/frontend/src/metabase/admin/performance/components/StrategyEditorForDatabases.unit.spec.tsx
+++ b/frontend/src/metabase/admin/performance/components/StrategyEditorForDatabases.unit.spec.tsx
@@ -45,4 +45,17 @@ describe("StrategyEditorForDatabases", () => {
(await screen.findByTestId("strategy-form-submit-button")).click();
});
+
+ it("does not regard form as dirty when a default value is entered into an input (metabase#42974)", async () => {
+ const adaptiveStrategyRadioButton = await screen.findByRole("radio", {
+ name: /Adaptive/i,
+ });
+ await userEvent.click(adaptiveStrategyRadioButton);
+ await userEvent.click(await getSaveButton());
+ await changeInput(/multiplier/i, 10, 10);
+ // The form is not considered dirty, so the save button is not present
+ expect(
+ screen.queryByTestId("strategy-form-submit-button"),
+ ).not.toBeInTheDocument();
+ });
});
diff --git a/frontend/src/metabase/admin/performance/components/StrategyForm.tsx b/frontend/src/metabase/admin/performance/components/StrategyForm.tsx
index 19425f0925b..06c47e22c1c 100644
--- a/frontend/src/metabase/admin/performance/components/StrategyForm.tsx
+++ b/frontend/src/metabase/admin/performance/components/StrategyForm.tsx
@@ -44,8 +44,8 @@ import { useIsFormPending } from "../hooks/useIsFormPending";
import { isModelWithClearableCache } from "../types";
import {
cronToScheduleSettings,
+ getDefaultValueForField,
getLabelString,
- getStrategyValidationSchema,
scheduleSettingsToCron,
} from "../utils";
@@ -95,14 +95,19 @@ export const StrategyForm = ({
buttonLabels?: ButtonLabels;
isInSidebar?: boolean;
}) => {
- const defaultStrategy: CacheStrategy = {
- type: targetId === rootId ? "nocache" : "inherit",
- };
+ const defaultStrategy: CacheStrategy = useMemo(
+ () => ({
+ type: targetId === rootId ? "nocache" : "inherit",
+ }),
+ [targetId],
+ );
+
+ const initialValues = savedStrategy ?? defaultStrategy;
return (
<FormProvider<CacheStrategy>
key={targetId}
- initialValues={savedStrategy ?? defaultStrategy}
+ initialValues={initialValues}
validationSchema={strategyValidationSchema}
onSubmit={saveStrategy}
onReset={onReset}
@@ -117,11 +122,38 @@ export const StrategyForm = ({
shouldShowName={shouldShowName}
buttonLabels={buttonLabels}
isInSidebar={isInSidebar}
+ strategyType={initialValues.type}
/>
</FormProvider>
);
};
+/** Don't count the addition/deletion of a default value as a reason to consider the form dirty */
+const isFormDirty = (values: CacheStrategy, initialValues: CacheStrategy) => {
+ const fieldNames = [...Object.keys(values), ...Object.keys(initialValues)];
+ const defaultValues = _.object(
+ _.map(fieldNames, fieldName => [
+ fieldName,
+ getDefaultValueForField(values.type, fieldName),
+ ]),
+ );
+ const initialValuesWithDefaults = { ...defaultValues, ...initialValues };
+ const valuesWithDefaults = { ...defaultValues, ...values };
+ // If the default value is a number and the value is a string, coerce the value to a number
+ const coercedValuesWithDefaults = _.chain(valuesWithDefaults)
+ .pairs()
+ .map(([key, value]) => [
+ key,
+ typeof getDefaultValueForField(values.type, key) === "number" &&
+ typeof value === "string"
+ ? Number(value)
+ : value,
+ ])
+ .object()
+ .value();
+ return !_.isEqual(initialValuesWithDefaults, coercedValuesWithDefaults);
+};
+
const StrategyFormBody = ({
targetId,
targetModel,
@@ -135,13 +167,21 @@ const StrategyFormBody = ({
targetId: number | null;
targetModel: CacheableModel;
targetName: string;
+ strategyType: CacheStrategyType;
setIsDirty: (isDirty: boolean) => void;
shouldAllowInvalidation: boolean;
shouldShowName?: boolean;
buttonLabels: ButtonLabels;
isInSidebar?: boolean;
}) => {
- const { dirty, values, setFieldValue } = useFormikContext<CacheStrategy>();
+ const { values, initialValues, setFieldValue } =
+ useFormikContext<CacheStrategy>();
+
+ const dirty = useMemo(
+ () => isFormDirty(values, initialValues),
+ [values, initialValues],
+ );
+
const { setStatus } = useFormContext();
const [wasDirty, setWasDirty] = useState(false);
@@ -238,6 +278,7 @@ const StrategyFormBody = ({
shouldAllowInvalidation={shouldAllowInvalidation}
buttonLabels={buttonLabels}
isInSidebar={isInSidebar}
+ dirty={dirty}
/>
</StyledForm>
</FormWrapper>
@@ -265,6 +306,7 @@ type FormButtonsProps = {
targetName?: string;
buttonLabels: ButtonLabels;
isInSidebar?: boolean;
+ dirty: boolean;
};
const FormButtons = ({
@@ -274,8 +316,11 @@ const FormButtons = ({
targetName,
buttonLabels,
isInSidebar,
+ dirty,
}: FormButtonsProps) => {
- const { dirty } = useFormikContext<CacheStrategy>();
+ if (targetId === rootId) {
+ shouldAllowInvalidation = false;
+ }
const { isFormPending, wasFormRecentlyPending } = useIsFormPending();
@@ -503,16 +548,6 @@ const Field = ({
);
};
-const getDefaultValueForField = (
- strategyType: CacheStrategyType,
- fieldName?: string,
-) => {
- const schema = getStrategyValidationSchema(
- PLUGIN_CACHING.strategies[strategyType],
- );
- return fieldName ? schema.cast({})[fieldName] : "";
-};
-
const MultiplierFieldSubtitle = () => (
<Text size="sm">
{t`To determine how long each cached result should stick around, we take that query's average execution time and multiply that by what you input here. The result is how many seconds the cache should remain valid for.`}{" "}
diff --git a/frontend/src/metabase/admin/performance/utils.tsx b/frontend/src/metabase/admin/performance/utils.tsx
index 5936ebb597e..51bcf6a02bb 100644
--- a/frontend/src/metabase/admin/performance/utils.tsx
+++ b/frontend/src/metabase/admin/performance/utils.tsx
@@ -320,3 +320,13 @@ export const getPerformanceTabName = (tabId: PerformanceTabId) =>
PLUGIN_CACHING.getTabMetadata().find(
({ key }) => key === `performance-${tabId}`,
)?.name;
+
+export const getDefaultValueForField = (
+ strategyType: CacheStrategyType,
+ fieldName?: string,
+) => {
+ const schema = getStrategyValidationSchema(
+ PLUGIN_CACHING.strategies[strategyType],
+ );
+ return fieldName ? schema.cast({})[fieldName] : "";
+};
--
GitLab