Prevent empty dashboard parameter labels (#34358)

e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js

@@ -100,7 +100,7 @@ describe("scenarios > dashboard > parameters", () => {
     cy.get(".DashCard").last().should("contain", "4,939");
   });
 
-  it("should remove parameter name or the whole parameter (metabase#10829, metabase#17933)", () => {
+  it("should be able to remove parameter (metabase#17933)", () => {
     // Mirrored issue in metabase-enterprise#275
 
     const questionDetails = {
@@ -229,20 +229,11 @@ describe("scenarios > dashboard > parameters", () => {
     cy.findByText("Remove").click();
     cy.location("search").should("eq", `?${endsWith.slug}=zmo`);
 
-    // Remove filter name (metabase#10829)
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText(endsWith.name).find(".Icon-gear").click();
-    cy.findByDisplayValue(endsWith.name).clear().blur();
-
-    cy.location("search").should("eq", "?unnamed=zmo");
-    cy.findByDisplayValue("unnamed");
-
     cy.button("Save").click();
 
-    cy.log("Filter name should be 'unnamed' and the value cleared");
-    filterWidget().contains(/unnamed/i);
-
-    cy.location("search").should("eq", "?unnamed=");
+    cy.log("There should only be one filter remaining and its value cleared");
+    filterWidget().contains(new RegExp(`${endsWith.name}`, "i"));
+    cy.location("search").should("eq", `?${endsWith.slug}=`);
 
     // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
     cy.findByText("37.65");

frontend/src/metabase/parameters/components/ParameterSettings/ParameterSettings.tsx

-import { useCallback } from "react";
+import { useCallback, useLayoutEffect, useState } from "react";
 import { t } from "ttag";
-import InputBlurChange from "metabase/components/InputBlurChange";
 import Radio from "metabase/core/components/Radio";
 import type {
   Parameter,
@@ -8,6 +7,7 @@ import type {
   ValuesSourceConfig,
   ValuesSourceType,
 } from "metabase-types/api";
+import { TextInput } from "metabase/ui";
 import { canUseCustomSource } from "metabase-lib/parameters/utils/parameter-source";
 import { getIsMultiSelect } from "../../utils/dashboards";
 import { isSingleOrMultiSelectable } from "../../utils/parameter-type";
@@ -46,11 +46,30 @@ const ParameterSettings = ({
   onChangeSourceConfig,
   onRemoveParameter,
 }: ParameterSettingsProps): JSX.Element => {
-  const handleNameChange = useCallback(
+  const [internalValue, setInternalValue] = useState(parameter.name);
+
+  useLayoutEffect(() => {
+    setInternalValue(parameter.name);
+  }, [parameter.name]);
+
+  const handleLabelChange = useCallback(
+    (event: React.ChangeEvent<HTMLInputElement>) => {
+      setInternalValue(event.target.value);
+    },
+    [],
+  );
+
+  const labelError = internalValue ? null : t`Required`;
+
+  const handleLabelBlur = useCallback(
     (event: { target: HTMLInputElement }) => {
-      onChangeName(event.target.value);
+      if (labelError) {
+        setInternalValue(parameter.name);
+      } else {
+        onChangeName(event.target.value);
+      }
     },
-    [onChangeName],
+    [onChangeName, parameter.name, labelError],
   );
 
   const handleSourceSettingsChange = useCallback(
@@ -65,9 +84,12 @@ const ParameterSettings = ({
     <SettingsRoot>
       <SettingSection>
         <SettingLabel>{t`Label`}</SettingLabel>
-        <InputBlurChange
-          value={parameter.name}
-          onBlurChange={handleNameChange}
+        <TextInput
+          onChange={handleLabelChange}
+          value={internalValue}
+          onBlur={handleLabelBlur}
+          error={labelError}
+          aria-label={t`Label`}
         />
       </SettingSection>
       {canUseCustomSource(parameter) && (

frontend/src/metabase/parameters/components/ParameterSettings/ParameterSettings.unit.spec.tsx

@@ -8,6 +8,13 @@ interface SetupOpts {
   parameter?: UiParameter;
 }
 
+function fillValue(input: HTMLElement, value: string) {
+  userEvent.clear(input);
+  if (value.length) {
+    userEvent.type(input, value);
+  }
+}
+
 describe("ParameterSidebar", () => {
   it("should allow to change source settings for string parameters", () => {
     const { onChangeQueryType } = setup({
@@ -22,6 +29,33 @@ describe("ParameterSidebar", () => {
     expect(onChangeQueryType).toHaveBeenCalledWith("search");
   });
 
+  it("should not update the label if the input is blank", () => {
+    const { onChangeName } = setup({
+      parameter: createMockUiParameter({
+        name: "foo",
+        type: "string/=",
+        sectionId: "string",
+      }),
+    });
+    const labelInput = screen.getByLabelText("Label");
+    expect(labelInput).toHaveValue("foo");
+    fillValue(labelInput, "");
+    // expect there to be an error message with the text "Required"
+    expect(screen.getByText(/required/i)).toBeInTheDocument();
+    labelInput.blur();
+    // when the input blurs, the value should have reverted to the original
+    expect(onChangeName).not.toHaveBeenCalled();
+    expect(labelInput).toHaveValue("foo");
+    // the error message should disappear
+    expect(screen.queryByText(/required/i)).not.toBeInTheDocument();
+
+    // sanity check with a non-blank value
+    fillValue(labelInput, "bar");
+    labelInput.blur();
+    expect(onChangeName).toHaveBeenCalledWith("bar");
+    expect(labelInput).toHaveValue("bar");
+  });
+
   it("should allow to change source settings for location parameters", () => {
     const { onChangeQueryType } = setup({
       parameter: createMockUiParameter({
@@ -38,11 +72,12 @@ describe("ParameterSidebar", () => {
 
 const setup = ({ parameter = createMockUiParameter() }: SetupOpts = {}) => {
   const onChangeQueryType = jest.fn();
+  const onChangeName = jest.fn();
 
   renderWithProviders(
     <ParameterSettings
       parameter={parameter}
-      onChangeName={jest.fn()}
+      onChangeName={onChangeName}
       onChangeDefaultValue={jest.fn()}
       onChangeIsMultiSelect={jest.fn()}
       onChangeQueryType={onChangeQueryType}
@@ -52,5 +87,5 @@ const setup = ({ parameter = createMockUiParameter() }: SetupOpts = {}) => {
     />,
   );
 
-  return { onChangeQueryType };
+  return { onChangeQueryType, onChangeName };
 };

frontend/src/metabase/parameters/utils/dashboards.ts

@@ -53,11 +53,8 @@ export function createParameter(
 
 export function setParameterName(
   parameter: Parameter,
-  name?: string,
+  name: string,
 ): Parameter {
-  if (!name) {
-    name = "unnamed";
-  }
   const slug = slugify(name);
   return {
     ...parameter,

frontend/src/metabase/parameters/utils/dashboards.unit.spec.js

@@ -98,10 +98,10 @@ describe("metabase/parameters/utils/dashboards", () => {
       });
     });
 
-    it("should default", () => {
+    it("should not default", () => {
       expect(setParameterName({}, "")).toEqual({
-        name: "unnamed",
-        slug: "unnamed",
+        name: "",
+        slug: "",
       });
     });
   });