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 846381ecc52534d9991a442345bd5652202b4afd..ba9d11bbc6abeff681ef4a32986c884e76ed79c3 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 19425f0925b15b22f85f311be0fb6191a07e404b..06c47e22c1cfd91911f9518e6bd37d1dab6693a6 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 5936ebb597e0640a53f096098cc369d1d3932fb9..51bcf6a02bb8f95c75cb1cd2e438962ec927da32 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] : ""; +};