From f2d61bf306820c5ceb06dee8fe2a9e58d22c6baa Mon Sep 17 00:00:00 2001
From: Nick Fitzpatrick <nick@metabase.com>
Date: Thu, 4 Aug 2022 16:37:04 -0300
Subject: [PATCH] Enforcing Filter Widget Lables be present (#24576)
---
.../metabase-lib/lib/queries/NativeQuery.ts | 3 +
.../template_tags/TagEditorParam.jsx | 80 ++++++++++---------
.../template_tags/TagEditorParam.styled.tsx | 56 +++++++++++++
.../lib/queries/NativeQuery.unit.spec.js | 23 ++++++
.../reproductions/11580.cy.spec.js | 2 +-
5 files changed, 125 insertions(+), 39 deletions(-)
create mode 100644 frontend/src/metabase/query_builder/components/template_tags/TagEditorParam.styled.tsx
diff --git a/frontend/src/metabase-lib/lib/queries/NativeQuery.ts b/frontend/src/metabase-lib/lib/queries/NativeQuery.ts
index 81ab6fcb956..4f02690a637 100644
--- a/frontend/src/metabase-lib/lib/queries/NativeQuery.ts
+++ b/frontend/src/metabase-lib/lib/queries/NativeQuery.ts
@@ -302,6 +302,9 @@ export default class NativeQuery extends AtomicQuery {
validateTemplateTags() {
return this.templateTags()
.map(tag => {
+ if (!tag["display-name"]) {
+ return new ValidationError(t`Missing wiget label: ${tag.name}`);
+ }
const dimension = new TemplateTagDimension(
tag.name,
this.metadata(),
diff --git a/frontend/src/metabase/query_builder/components/template_tags/TagEditorParam.jsx b/frontend/src/metabase/query_builder/components/template_tags/TagEditorParam.jsx
index 26203c57893..55a834cf090 100644
--- a/frontend/src/metabase/query_builder/components/template_tags/TagEditorParam.jsx
+++ b/frontend/src/metabase/query_builder/components/template_tags/TagEditorParam.jsx
@@ -8,9 +8,7 @@ import { Link } from "react-router";
import Schemas from "metabase/entities/schemas";
import Toggle from "metabase/core/components/Toggle";
-import InputBlurChange from "metabase/components/InputBlurChange";
import Select, { Option } from "metabase/core/components/Select";
-import ParameterValueWidget from "metabase/parameters/components/ParameterValueWidget";
import { getParameterOptionsForField } from "metabase/parameters/utils/template-tag-options";
@@ -19,6 +17,16 @@ import { getMetadata } from "metabase/selectors/metadata";
import { SchemaTableAndFieldDataSelector } from "metabase/query_builder/components/DataSelector";
import MetabaseSettings from "metabase/lib/settings";
+import {
+ ErrorSpan,
+ TagName,
+ TagContainer,
+ ContainerLabel,
+ InputContainer,
+ WidgetLabelInput,
+ DefaultParameterValueWidget,
+} from "./TagEditorParam.styled";
+
const propTypes = {
tag: PropTypes.object.isRequired,
parameter: PropTypes.object,
@@ -141,16 +149,16 @@ export class TagEditorParam extends Component {
const hasWidgetOptions = widgetOptions && widgetOptions.length > 0;
const hasNoWidgetType =
tag["widget-type"] === "none" || !tag["widget-type"];
+ const hasNoWidgetLabel = !tag["display-name"];
return (
- <div className="px3 pt3 mb1 border-top">
- <h4 className="text-medium py1">{t`Variable name`}</h4>
- <h3 className="text-heavy text-brand align-self-end mb4">{tag.name}</h3>
+ <TagContainer>
+ <ContainerLabel paddingTop>{t`Variable name`}</ContainerLabel>
+ <TagName>{tag.name}</TagName>
- <div className="pb4">
- <h4 className="text-medium pb1">{t`Variable type`}</h4>
+ <InputContainer>
+ <ContainerLabel>{t`Variable type`}</ContainerLabel>
<Select
- className="block"
value={tag.type}
onChange={e => this.setType(e.target.value)}
isInitiallyOpen={!tag.type}
@@ -162,16 +170,14 @@ export class TagEditorParam extends Component {
<Option value="date">{t`Date`}</Option>
<Option value="dimension">{t`Field Filter`}</Option>
</Select>
- </div>
+ </InputContainer>
{tag.type === "dimension" && (
- <div className="pb4">
- <h4 className="text-medium pb1">
+ <InputContainer>
+ <ContainerLabel>
{t`Field to map to`}
- {tag.dimension == null && (
- <span className="text-error mx1">{t`(required)`}</span>
- )}
- </h4>
+ {tag.dimension == null && <ErrorSpan>{t`(required)`}</ErrorSpan>}
+ </ContainerLabel>
{(!hasSelectedDimensionField ||
(hasSelectedDimensionField && fieldMetadataLoaded)) && (
@@ -196,17 +202,15 @@ export class TagEditorParam extends Component {
)}
</Schemas.Loader>
)}
- </div>
+ </InputContainer>
)}
{hasSelectedDimensionField && (
- <div className="pb4">
- <h4 className="text-medium pb1">
+ <InputContainer>
+ <ContainerLabel>
{t`Filter widget type`}
- {hasNoWidgetType && (
- <span className="text-error mx1">{t`(required)`}</span>
- )}
- </h4>
+ {hasNoWidgetType && <ErrorSpan>{t`(required)`}</ErrorSpan>}
+ </ContainerLabel>
<Select
className="block"
value={this.getFilterWidgetTypeValue(tag, widgetOptions)}
@@ -242,38 +246,39 @@ export class TagEditorParam extends Component {
</Link>
</p>
)}
- </div>
+ </InputContainer>
)}
{(hasWidgetOptions || !isDimension) && (
- <div className="pb4">
- <h4 className="text-medium pb1">{t`Filter widget label`}</h4>
- <InputBlurChange
+ <InputContainer>
+ <ContainerLabel>
+ {t`Filter widget label`}
+ {hasNoWidgetLabel && <ErrorSpan>{t`(required)`}</ErrorSpan>}
+ </ContainerLabel>
+ <WidgetLabelInput
type="text"
value={tag["display-name"]}
- className="AdminSelect p1 text-bold text-dark bordered border-medium rounded full"
- style={{ fontSize: "14px" }}
onBlurChange={e =>
this.setParameterAttribute("display-name", e.target.value)
}
/>
- </div>
+ </InputContainer>
)}
- <div className="pb3">
- <h4 className="text-medium pb1">{t`Required?`}</h4>
+ <InputContainer lessBottomPadding>
+ <ContainerLabel>{t`Required?`}</ContainerLabel>
<Toggle
value={tag.required}
onChange={value => this.setRequired(value)}
/>
- </div>
+ </InputContainer>
{((tag.type !== "dimension" && tag.required) ||
tag.type === "dimension" ||
tag["widget-type"]) && (
- <div className="pb3">
- <h4 className="text-medium pb1">{t`Default filter widget value`}</h4>
- <ParameterValueWidget
+ <InputContainer lessBottomPadding>
+ <ContainerLabel>{t`Default filter widget value`}</ContainerLabel>
+ <DefaultParameterValueWidget
parameter={
tag.type === "dimension"
? parameter || {
@@ -293,13 +298,12 @@ export class TagEditorParam extends Component {
}
value={tag.default}
setValue={value => this.setParameterAttribute("default", value)}
- className="AdminSelect p1 text-bold text-medium bordered border-medium rounded bg-white"
isEditing
commitImmediately
/>
- </div>
+ </InputContainer>
)}
- </div>
+ </TagContainer>
);
}
}
diff --git a/frontend/src/metabase/query_builder/components/template_tags/TagEditorParam.styled.tsx b/frontend/src/metabase/query_builder/components/template_tags/TagEditorParam.styled.tsx
new file mode 100644
index 00000000000..5fb33db7c5e
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/template_tags/TagEditorParam.styled.tsx
@@ -0,0 +1,56 @@
+import styled from "@emotion/styled";
+import InputBlurChange from "metabase/components/InputBlurChange";
+import { color } from "metabase/lib/colors";
+import ParameterValueWidget from "metabase/parameters/components/ParameterValueWidget";
+
+export const TagContainer = styled.div`
+ padding: 1.5rem 1.5rem 0 1.5rem;
+ margin-bottom: 1.5rem;
+ border-top: 1px solid ${color("border")};
+`;
+export const TagName = styled.h3`
+ font-weight: 900;
+ margin-bottom: 2rem;
+ align-self: flex-end;
+ color: ${color("brand")};
+`;
+
+interface ContainerLabelProps {
+ paddingTop: boolean;
+}
+export const ContainerLabel = styled.h4<ContainerLabelProps>`
+ padding-bottom: 0.5rem;
+ color: ${color("text-medium")};
+ padding-top: ${props => (props.paddingTop ? "0.5rem" : "0")};
+`;
+
+export const ErrorSpan = styled.span`
+ margin: 0 0.5rem;
+ color: ${color("error")};
+`;
+
+interface InputContainerProps {
+ lessBottomPadding: boolean;
+}
+export const InputContainer = styled.div<InputContainerProps>`
+ padding-bottom: ${props => (props.lessBottomPadding ? "1.5rem" : "2rem")};
+`;
+
+export const WidgetLabelInput = styled(InputBlurChange)`
+ font-weight: 700;
+ padding: 0.5rem;
+ border: 1px solid ${color("border-dark")};
+ border-radius: 0.5rem;
+ width: 100%;
+ color: ${color("text-dark")};
+ font-size: 0.875rem;
+`;
+
+export const DefaultParameterValueWidget = styled(ParameterValueWidget)`
+ padding: 0.5rem;
+ font-weight: 700;
+ color: ${color("text-medium")};
+ border-radius: 0.5rem;
+ background-color: ${color("white")};
+ border: 2px solid ${color("border")};
+`;
diff --git a/frontend/test/metabase-lib/lib/queries/NativeQuery.unit.spec.js b/frontend/test/metabase-lib/lib/queries/NativeQuery.unit.spec.js
index d39f0b8213f..8ebad7fd155 100644
--- a/frontend/test/metabase-lib/lib/queries/NativeQuery.unit.spec.js
+++ b/frontend/test/metabase-lib/lib/queries/NativeQuery.unit.spec.js
@@ -194,11 +194,33 @@ describe("NativeQuery", () => {
expect(q.canRun()).toBe(true);
});
+ it("requires a display name", () => {
+ q = q.setDatasetQuery(
+ assocIn(q.datasetQuery(), ["native", "template-tags", "foo"], {
+ name: "foo",
+ type: "text",
+ }),
+ );
+
+ expect(q.canRun()).toBe(false);
+
+ q = q.setDatasetQuery(
+ assocIn(q.datasetQuery(), ["native", "template-tags", "foo"], {
+ name: "foo",
+ type: "text",
+ "display-name": "Foo",
+ }),
+ );
+
+ expect(q.canRun()).toBe(true);
+ });
+
it("dimension type without a dimension", () => {
q = q.setDatasetQuery(
assocIn(q.datasetQuery(), ["native", "template-tags", "foo"], {
type: "dimension",
"widget-type": "category",
+ "display-name": "bar",
}),
);
@@ -210,6 +232,7 @@ describe("NativeQuery", () => {
type: "dimension",
"widget-type": "category",
dimension: ["field", 123, null],
+ "display-name": "bar",
}),
);
diff --git a/frontend/test/metabase/scenarios/native-filters/reproductions/11580.cy.spec.js b/frontend/test/metabase/scenarios/native-filters/reproductions/11580.cy.spec.js
index 727710b78e4..99a52432c73 100644
--- a/frontend/test/metabase/scenarios/native-filters/reproductions/11580.cy.spec.js
+++ b/frontend/test/metabase/scenarios/native-filters/reproductions/11580.cy.spec.js
@@ -12,7 +12,7 @@ describe("issue 11580", () => {
openNativeEditor();
SQLFilter.enterParameterizedQuery("{{foo}} {{bar}}");
- cy.findByTestId("sidebar-right").find(".text-brand").as("variableLabels");
+ cy.findAllByText("Variable name").next().as("variableLabels");
// ensure they're in the right order to start
assertVariablesOrder();
--
GitLab