From c4812b4a3e49d3daa130d9e5e599b40d3d1054a7 Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Mon, 9 Oct 2023 16:55:01 +0300
Subject: [PATCH] Prevent empty dashboard parameter labels (#34358)
---
.../dashboard-filters/parameters.cy.spec.js | 17 ++------
.../ParameterSettings/ParameterSettings.tsx | 38 ++++++++++++++----
.../ParameterSettings.unit.spec.tsx | 39 ++++++++++++++++++-
.../metabase/parameters/utils/dashboards.ts | 5 +--
.../parameters/utils/dashboards.unit.spec.js | 6 +--
5 files changed, 75 insertions(+), 30 deletions(-)
diff --git a/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js b/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js
index 3edab3ff42c..5857132800c 100644
--- a/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js
+++ b/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");
diff --git a/frontend/src/metabase/parameters/components/ParameterSettings/ParameterSettings.tsx b/frontend/src/metabase/parameters/components/ParameterSettings/ParameterSettings.tsx
index da3c53d764b..92c2453bbbd 100644
--- a/frontend/src/metabase/parameters/components/ParameterSettings/ParameterSettings.tsx
+++ b/frontend/src/metabase/parameters/components/ParameterSettings/ParameterSettings.tsx
@@ -1,6 +1,5 @@
-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) && (
diff --git a/frontend/src/metabase/parameters/components/ParameterSettings/ParameterSettings.unit.spec.tsx b/frontend/src/metabase/parameters/components/ParameterSettings/ParameterSettings.unit.spec.tsx
index 80d79e7ef7d..d91aa162b0e 100644
--- a/frontend/src/metabase/parameters/components/ParameterSettings/ParameterSettings.unit.spec.tsx
+++ b/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 };
};
diff --git a/frontend/src/metabase/parameters/utils/dashboards.ts b/frontend/src/metabase/parameters/utils/dashboards.ts
index 728826838f7..9682b97768b 100644
--- a/frontend/src/metabase/parameters/utils/dashboards.ts
+++ b/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,
diff --git a/frontend/src/metabase/parameters/utils/dashboards.unit.spec.js b/frontend/src/metabase/parameters/utils/dashboards.unit.spec.js
index 1c0ad619c45..ab58a83d8ff 100644
--- a/frontend/src/metabase/parameters/utils/dashboards.unit.spec.js
+++ b/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: "",
});
});
});
--
GitLab