Trim space and normalize empty string in admin settings (#36693)

* Trim space and normalize empty string in admin settings

* Attempt to use `SettingInput`

* Fix the input doesn't change value to the normalized value

This happened when the normalized value is the same value before we call
`onChange`

* Remove unnecessary type check + prevent rerendering

enterprise/frontend/src/metabase-enterprise/whitelabel/components/HelpLinkSettings/HelpLinkSettings.tsx

@@ -2,7 +2,7 @@ import { t } from "ttag";
 import { useState } from "react";
 import { Radio, Stack, Text } from "metabase/ui";
 import type { HelpLinkSetting, SettingKey, Settings } from "metabase-types/api";
-import { SettingInputBlurChange } from "metabase/admin/settings/components/widgets/SettingInput.styled";
+import { SettingInputBlurChange } from "metabase/admin/settings/components/widgets/SettingInput/SettingInput.styled";
 
 interface Props {
   setting: {

enterprise/frontend/src/metabase-enterprise/whitelabel/components/LogoUpload/LogoUpload.jsx

 /* eslint-disable react/prop-types */
 import Button from "metabase/core/components/Button";
 import LogoIcon from "metabase/components/LogoIcon";
-import SettingInput from "metabase/admin/settings/components/widgets/SettingInput";
+import { SettingInput } from "metabase/admin/settings/components/widgets/SettingInput";
 import { color } from "metabase/lib/colors";
 
 import { LogoFileInput } from "./LogoUpload.styled";

enterprise/frontend/src/metabase-enterprise/whitelabel/index.js

@@ -69,6 +69,16 @@ if (hasPremiumFeature("whitelabel")) {
           display_name: t`Landing Page`,
           type: "string",
           placeholder: "/",
+          props: {
+            normalize(value) {
+              if (typeof value === "string") {
+                const normalizedValue = value.trim();
+                return normalizedValue === "" ? null : normalizedValue;
+              }
+
+              return value;
+            },
+          },
         },
         {
           key: "loading-message",

frontend/src/metabase/admin/settings/components/SettingsSetting.jsx

@@ -5,7 +5,7 @@ import { jt } from "ttag";
 import MetabaseSettings from "metabase/lib/settings";
 import ExternalLink from "metabase/core/components/ExternalLink";
 import SettingHeader from "./SettingHeader";
-import SettingInput from "./widgets/SettingInput";
+import { SettingInput } from "./widgets/SettingInput";
 import SettingNumber from "./widgets/SettingNumber";
 import SettingPassword from "./widgets/SettingPassword";
 import SettingRadio from "./widgets/SettingRadio";

frontend/src/metabase/admin/settings/components/widgets/SecretKeyWidget/SecretKeyWidget.tsx

 import { t } from "ttag";
 import Confirm from "metabase/components/Confirm";
 import { UtilApi } from "metabase/services";
-import SettingInput from "../SettingInput";
+import { SettingInput } from "../SettingInput";
 import { GenerateButton, SecretKeyWidgetRoot } from "./SecretKeyWidget.styled";
 
 interface SecretKeyWidgetProps {

frontend/src/metabase/admin/settings/components/widgets/SettingCommaDelimitedInput.jsx

 /* eslint-disable react/prop-types */
-import { SettingInputBlurChange } from "./SettingInput.styled";
+import { SettingInputBlurChange } from "./SettingInput/SettingInput.styled";
 
 const maybeSingletonList = value => (value ? [value] : null);
 

frontend/src/metabase/admin/settings/components/widgets/SettingInput.unit.spec.tsx

-import { render, screen } from "@testing-library/react";
-import userEvent from "@testing-library/user-event";
-import SettingInput from "./SettingInput";
-
-describe("SettingInput", () => {
-  it("when type=number should allow decimal values", () => {
-    const onChange = jest.fn();
-    render(
-      <SettingInput
-        onChange={onChange}
-        setting={{
-          key: "test",
-          value: "100",
-          default: "100",
-          placeholder: "numeric value",
-        }}
-        type="number"
-      />,
-    );
-
-    userEvent.type(screen.getByPlaceholderText("numeric value"), ".25");
-    userEvent.tab(); // blur
-
-    expect(onChange).toHaveBeenCalledWith(100.25);
-  });
-});

frontend/src/metabase/admin/settings/components/widgets/SettingInput.tsx → frontend/src/metabase/admin/settings/components/widgets/SettingInput/SettingInput.tsx

@@ -10,22 +10,27 @@ const getValue = (value: string, type: string) => {
   return value;
 };
 
-interface SettingInputProps {
+type Value = string | number | null;
+
+export interface SettingInputProps {
   setting: {
     key: string;
     value: string | null;
     default?: string;
     placeholder?: string;
   };
-  onChange: (value: string | number | null) => void;
+  onChange: (value: Value) => void;
   autoFocus?: boolean;
   fireOnChange?: boolean;
   errorMessage?: string;
   id?: string;
   type?: string;
+  normalize?: (value: Value) => Value;
 }
 
-const SettingInput = ({
+const identity = (value: Value) => value;
+
+export const SettingInput = ({
   setting,
   onChange,
   autoFocus,
@@ -33,10 +38,11 @@ const SettingInput = ({
   fireOnChange,
   id,
   type = "text",
+  normalize = identity,
 }: SettingInputProps) => {
   const changeHandler = (e: { target: HTMLInputElement }) => {
     const value = getValue(e.target.value, type);
-    onChange(value);
+    onChange(normalize(value));
   };
 
   return (
@@ -45,6 +51,7 @@ const SettingInput = ({
         SettingsInput: type !== "password",
         SettingsPassword: type === "password",
       })}
+      normalize={normalize}
       size="large"
       error={!!errorMessage}
       id={id}

frontend/src/metabase/admin/settings/components/widgets/SettingInput/SettingInput.unit.spec.tsx

+import { render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+import { SettingInput } from "./SettingInput";
+
+type Value = string | number | null;
+
+interface SetupOpts {
+  setting: Setting;
+  value: Value;
+  type: string;
+  normalize?: (value: Value) => Value;
+}
+
+interface Setting {
+  key: string;
+  value?: string;
+  default?: string;
+  placeholder?: string;
+  display_name?: string;
+  type?: string;
+}
+
+function setup({ setting, value, type, normalize }: SetupOpts) {
+  const onChange = jest.fn();
+  render(
+    <SettingInput
+      setting={{ ...setting, value: String(value) }}
+      onChange={onChange}
+      type={type}
+      normalize={normalize}
+    />,
+  );
+
+  return { onChange };
+}
+
+describe("SettingInput", () => {
+  it("when type=number should allow decimal values", () => {
+    const setting = {
+      key: "test",
+      value: "100",
+      default: "100",
+      placeholder: "numeric value",
+    };
+    const { onChange } = setup({ setting, value: 100, type: "number" });
+
+    userEvent.type(screen.getByPlaceholderText("numeric value"), ".25");
+    userEvent.tab(); // blur
+
+    expect(onChange).toHaveBeenCalledWith(100.25);
+  });
+
+  describe("normalize value (metabase#25482)", () => {
+    const setting = {
+      key: "landing-page",
+      display_name: "Landing Page",
+      type: "string",
+      placeholder: "/",
+    };
+
+    function normalize(value: Value) {
+      if (typeof value === "string") {
+        const normalizedValue = value.trim();
+        return normalizedValue === "" ? null : normalizedValue;
+      }
+
+      return value;
+    }
+    it("should render the input", () => {
+      const value = "/";
+      setup({ setting, value, type: "text", normalize });
+
+      expect(screen.getByDisplayValue(value)).toBeInTheDocument();
+    });
+
+    it("should  call onChange without leading or trailing spaces string", () => {
+      const value = "/";
+      const { onChange } = setup({ setting, value, type: "text", normalize });
+
+      const input = screen.getByDisplayValue(value);
+      userEvent.clear(input);
+      userEvent.type(input, "  /  ");
+      input.blur();
+      expect(onChange).toHaveBeenCalledWith("/");
+    });
+
+    it("should not onChange with null", () => {
+      const value = "/";
+      const { onChange } = setup({ setting, value, type: "text", normalize });
+
+      const input = screen.getByDisplayValue(value);
+      userEvent.clear(input);
+      input.blur();
+      expect(onChange).toHaveBeenCalledWith(null);
+    });
+
+    it("should not onChange with number", () => {
+      const value = "1";
+      const { onChange } = setup({ setting, value, type: "number", normalize });
+
+      const input = screen.getByDisplayValue(value);
+      userEvent.clear(input);
+      userEvent.type(input, "2");
+      input.blur();
+      expect(onChange).toHaveBeenCalledWith(2);
+    });
+  });
+});

frontend/src/metabase/admin/settings/components/widgets/SettingInput/index.ts

+export { SettingInput } from "./SettingInput";

frontend/src/metabase/admin/settings/components/widgets/SettingNumber.jsx

 /* eslint-disable react/prop-types */
-import SettingInput from "./SettingInput";
+import { SettingInput } from "./SettingInput";
 
 const SettingNumber = ({ type = "number", ...props }) => (
   <SettingInput {...props} type="number" />

frontend/src/metabase/admin/settings/components/widgets/SettingPassword.jsx

-import SettingInput from "./SettingInput";
+import { SettingInput } from "./SettingInput";
 
 const SettingPassword = props => <SettingInput {...props} type="password" />;
 

frontend/src/metabase/components/InputBlurChange/InputBlurChange.tsx

@@ -10,15 +10,25 @@ import Input from "metabase/core/components/Input";
  * `onBlurChange` feature, otherwise you should use <input> directly
  */
 
+type Value = string | number | null;
+type HTMLInputValue = string | number | undefined;
+
 export interface InputBlurChangeProps
   extends Omit<InputProps, "inputRef" | "value" | "onBlur"> {
   value: string | undefined;
   onBlurChange?: (event: { target: HTMLInputElement }) => void;
+  normalize?: (value: Value) => Value;
 }
 
 const InputBlurChange = (props: InputBlurChangeProps) => {
-  const { value, onChange, onBlurChange, ...restProps } = props;
-  const [internalValue, setInternalValue] = useState(value);
+  const {
+    value,
+    onChange,
+    onBlurChange,
+    normalize = value => value,
+    ...restProps
+  } = props;
+  const [internalValue, setInternalValue] = useState<HTMLInputValue>(value);
   const inputRef = useRef<HTMLInputElement>(null);
 
   useLayoutEffect(() => {
@@ -31,18 +41,20 @@ const InputBlurChange = (props: InputBlurChangeProps) => {
 
       if (onChange) {
         onChange(event);
+        setInternalValue(normalize(event.target.value) ?? undefined);
       }
     },
-    [onChange],
+    [normalize, onChange],
   );
 
   const handleBlur = useCallback(
     (event: React.ChangeEvent<HTMLInputElement>) => {
       if (onBlurChange && (value || "") !== event.target.value) {
         onBlurChange(event);
+        setInternalValue(normalize(event.target.value) ?? undefined);
       }
     },
-    [value, onBlurChange],
+    [normalize, onBlurChange, value],
   );
 
   useUnmount(() => {

frontend/src/metabase/components/InputBlurChange/InputBlurChange.unit.spec.tsx

@@ -41,11 +41,23 @@ describe("InputBlurChange", () => {
     expect(onBlurChange).toHaveBeenCalledTimes(1);
     expect(onBlurChange.mock.results[0].value).toBe("test");
   });
+
+  it("should set `internalValue` to the normalized value even if the normalized value is the same as the previous one", () => {
+    const value = "/";
+    setup({ value, normalize: value => (value as string).trim() });
+    const input = screen.getByDisplayValue(value) as HTMLInputElement;
+    userEvent.clear(input);
+    userEvent.type(input, "           /         ");
+
+    const normalizedValue = "/";
+    expect(input.value).toEqual(normalizedValue);
+  });
 });
 
 function setup({
   value = "",
   placeholder = "Type some texto",
+  normalize,
 }: Partial<InputBlurChangeProps> = {}) {
   const onChange = jest.fn();
   const onBlurChange = jest.fn(e => e.target.value);
@@ -56,6 +68,7 @@ function setup({
       placeholder={placeholder}
       onChange={onChange}
       onBlurChange={onBlurChange}
+      normalize={normalize}
     />,
   );