Enforcing Filter Widget Lables be present (#24576)

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(),

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>
     );
   }
 }

frontend/src/metabase/query_builder/components/template_tags/TagEditorParam.styled.tsx

+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")};
+`;

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",
           }),
         );
 

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();