From 5d5deac1da6aace93a09c017a3c213dcd8542399 Mon Sep 17 00:00:00 2001 From: Dalton <daltojohnso@users.noreply.github.com> Date: Fri, 18 Mar 2022 09:25:38 -0700 Subject: [PATCH] Add validation logic to TemplateTagDimension dimension tag handling (#20878) * Add validation logic to TemplateTagDimension dimension tag handling * Add basic error message to query builder * Update error text * Add unique error message for validation errors * Update text of error state Co-authored-by: Maz Ameli <maz@metabase.com> * Fix types * Fix NativeQuery unit test * Add unit tests for new components Co-authored-by: Maz Ameli <maz@metabase.com> --- frontend/src/metabase-lib/lib/Dimension.ts | 30 ++++- .../src/metabase-lib/lib/ValidationError.ts | 14 +++ .../metabase-lib/lib/queries/NativeQuery.ts | 34 +++++- .../ErrorActionButton.tsx | 64 +++++++++++ .../ErrorActionButton.unit.spec.tsx | 103 ++++++++++++++++++ .../QueryValidationError.styled.tsx | 34 ++++++ .../QueryValidationError.tsx | 36 ++++++ .../QueryValidationError.unit.spec.tsx | 53 +++++++++ .../components/QueryValidationError/index.ts | 1 + .../query_builder/components/view/View.jsx | 30 +++-- .../metabase-lib/lib/Dimension.unit.spec.js | 72 ++++++++++++ .../lib/queries/NativeQuery.unit.spec.js | 1 + 12 files changed, 453 insertions(+), 19 deletions(-) create mode 100644 frontend/src/metabase-lib/lib/ValidationError.ts create mode 100644 frontend/src/metabase/query_builder/components/QueryValidationError/ErrorActionButton.tsx create mode 100644 frontend/src/metabase/query_builder/components/QueryValidationError/ErrorActionButton.unit.spec.tsx create mode 100644 frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.styled.tsx create mode 100644 frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.tsx create mode 100644 frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.unit.spec.tsx create mode 100644 frontend/src/metabase/query_builder/components/QueryValidationError/index.ts diff --git a/frontend/src/metabase-lib/lib/Dimension.ts b/frontend/src/metabase-lib/lib/Dimension.ts index 9188fde0418..51e5dba00a4 100644 --- a/frontend/src/metabase-lib/lib/Dimension.ts +++ b/frontend/src/metabase-lib/lib/Dimension.ts @@ -19,6 +19,7 @@ import { ExpressionReference, DatetimeUnit, } from "metabase-types/types/Query"; +import { ValidationError, VALIDATION_ERROR_TYPES } from "./ValidationError"; import { IconName } from "metabase-types/types"; import { getFieldValues, getRemappings } from "metabase/lib/query/field"; import { DATETIME_UNITS, formatBucketing } from "metabase/lib/query_time"; @@ -1515,6 +1516,27 @@ export class TemplateTagDimension extends FieldDimension { return Array.isArray(clause) && clause[0] === "template-tag"; } + validateTemplateTag(): ValidationError | null { + const tag = this.tag(); + if (!tag) { + return new ValidationError(t`Invalid template tag "${this.tagName()}"`); + } + + if (this.isDimensionType() && tag.dimension == null) { + return new ValidationError( + t`The variable "${this.tagName()}" needs to be mapped to a field.`, + VALIDATION_ERROR_TYPES.MISSING_TAG_DIMENSION, + ); + } + + return null; + } + + isValidDimensionType() { + const maybeErrors = this.validateTemplateTag(); + return this.isDimensionType() && maybeErrors === null; + } + isDimensionType() { const maybeTag = this.tag(); return maybeTag?.type === "dimension"; @@ -1526,7 +1548,7 @@ export class TemplateTagDimension extends FieldDimension { } dimension() { - if (this.isDimensionType()) { + if (this.isValidDimensionType()) { const tag = this.tag(); return Dimension.parseMBQL(tag.dimension, this._metadata, this._query); } @@ -1549,7 +1571,7 @@ export class TemplateTagDimension extends FieldDimension { } field() { - if (this.isDimensionType()) { + if (this.isValidDimensionType()) { return this.dimension().field(); } @@ -1557,7 +1579,7 @@ export class TemplateTagDimension extends FieldDimension { } name() { - return this.isDimensionType() ? this.field().name : this.tagName(); + return this.isValidDimensionType() ? this.field().name : this.tagName(); } tagName() { @@ -1574,7 +1596,7 @@ export class TemplateTagDimension extends FieldDimension { } icon() { - if (this.isDimensionType()) { + if (this.isValidDimensionType()) { return this.dimension().icon(); } else if (this.isVariableType()) { return this.variable().icon(); diff --git a/frontend/src/metabase-lib/lib/ValidationError.ts b/frontend/src/metabase-lib/lib/ValidationError.ts new file mode 100644 index 00000000000..3669619ead1 --- /dev/null +++ b/frontend/src/metabase-lib/lib/ValidationError.ts @@ -0,0 +1,14 @@ +export const VALIDATION_ERROR_TYPES = { + MISSING_TAG_DIMENSION: "MISSING_TAG_DIMENSION", +} as const; + +export type ErrorType = typeof VALIDATION_ERROR_TYPES[keyof typeof VALIDATION_ERROR_TYPES]; + +export class ValidationError extends Error { + type?: ErrorType; + + constructor(message: string, errorType?: ErrorType) { + super(message); + this.type = errorType; + } +} diff --git a/frontend/src/metabase-lib/lib/queries/NativeQuery.ts b/frontend/src/metabase-lib/lib/queries/NativeQuery.ts index 77e8018bc51..9e1bac7f04d 100644 --- a/frontend/src/metabase-lib/lib/queries/NativeQuery.ts +++ b/frontend/src/metabase-lib/lib/queries/NativeQuery.ts @@ -1,5 +1,7 @@ // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-nocheck +import { t } from "ttag"; + import Database from "metabase-lib/lib/metadata/Database"; import Table from "metabase-lib/lib/metadata/Table"; import { countLines } from "metabase/lib/string"; @@ -24,6 +26,8 @@ import AtomicQuery from "metabase-lib/lib/queries/AtomicQuery"; import Dimension, { TemplateTagDimension, FieldDimension } from "../Dimension"; import Variable, { TemplateTagVariable } from "../Variable"; import DimensionOptions from "../DimensionOptions"; +import { ValidationError } from "../ValidationError"; + type DimensionFilter = (dimension: Dimension) => boolean; type VariableFilter = (variable: Variable) => boolean; export const NATIVE_QUERY_TEMPLATE: NativeDatasetQuery = { @@ -285,11 +289,33 @@ export default class NativeQuery extends AtomicQuery { return getIn(this.datasetQuery(), ["native", "template-tags"]) || {}; } + validate() { + const tagErrors = this.validateTemplateTags(); + return tagErrors; + } + + validateTemplateTags() { + return this.templateTags() + .map(tag => { + const dimension = new TemplateTagDimension( + tag.name, + this.metadata(), + this, + ); + if (!dimension) { + return new ValidationError(t`Invalid template tag: ${tag.name}`); + } + + return dimension.validateTemplateTag(); + }) + .filter( + (maybeError): maybeError is ValidationError => maybeError != null, + ); + } + allTemplateTagsAreValid() { - return this.templateTags().every( - // field filters require a field - t => !(t.type === "dimension" && t.dimension == null), - ); + const tagErrors = this.validateTemplateTags(); + return tagErrors.length === 0; } setTemplateTag(name, tag) { diff --git a/frontend/src/metabase/query_builder/components/QueryValidationError/ErrorActionButton.tsx b/frontend/src/metabase/query_builder/components/QueryValidationError/ErrorActionButton.tsx new file mode 100644 index 00000000000..78948dd7ccb --- /dev/null +++ b/frontend/src/metabase/query_builder/components/QueryValidationError/ErrorActionButton.tsx @@ -0,0 +1,64 @@ +import React from "react"; +import { t } from "ttag"; +import { connect } from "react-redux"; + +import { + ValidationError, + VALIDATION_ERROR_TYPES, + ErrorType, +} from "metabase-lib/lib/ValidationError"; +import { getUiControls } from "metabase/query_builder/selectors"; +import { toggleTemplateTagsEditor } from "metabase/query_builder/actions"; + +import { QueryValidationErrorProps } from "./QueryValidationError"; +import { QueryErrorActionButton } from "./QueryValidationError.styled"; + +type QueryBuilderUiControls = { + isShowingTemplateTagsEditor?: boolean; +}; + +export type ErrorActionButtonProps = QueryValidationErrorProps & { + uiControls: QueryBuilderUiControls; + toggleTemplateTagsEditor: () => void; +}; + +const mapStateToProps = (state: any, props: QueryValidationErrorProps) => ({ + uiControls: getUiControls(state), +}); + +const mapDispatchToProps = { + toggleTemplateTagsEditor, +}; + +export const BUTTON_ACTIONS: Record< + ErrorType, + [string, (props: ErrorActionButtonProps) => void] +> = { + [VALIDATION_ERROR_TYPES.MISSING_TAG_DIMENSION]: [ + t`Edit variables`, + ({ uiControls, toggleTemplateTagsEditor }) => { + if (!uiControls.isShowingTemplateTagsEditor) { + toggleTemplateTagsEditor(); + } + }, + ], +}; + +export function ErrorActionButton(props: ErrorActionButtonProps) { + const { error } = props; + const type = error instanceof ValidationError ? error.type : undefined; + + if (!type || !BUTTON_ACTIONS[type]) { + return null; + } + + const [buttonLabel, actionFn] = BUTTON_ACTIONS[type]; + + return ( + <QueryErrorActionButton onClick={() => actionFn(props)}> + {buttonLabel} + </QueryErrorActionButton> + ); +} + +export default connect(mapStateToProps, mapDispatchToProps)(ErrorActionButton); diff --git a/frontend/src/metabase/query_builder/components/QueryValidationError/ErrorActionButton.unit.spec.tsx b/frontend/src/metabase/query_builder/components/QueryValidationError/ErrorActionButton.unit.spec.tsx new file mode 100644 index 00000000000..4b9e19417f8 --- /dev/null +++ b/frontend/src/metabase/query_builder/components/QueryValidationError/ErrorActionButton.unit.spec.tsx @@ -0,0 +1,103 @@ +import React from "react"; +import { render, screen } from "__support__/ui"; +import userEvent from "@testing-library/user-event"; + +import { + ValidationError, + VALIDATION_ERROR_TYPES, +} from "metabase-lib/lib/ValidationError"; + +import { + ErrorActionButton, + BUTTON_ACTIONS, + ErrorActionButtonProps, +} from "./ErrorActionButton"; + +let props: ErrorActionButtonProps; +describe("ErrorActionButton", () => { + beforeEach(() => { + props = { + error: new ValidationError( + "oof", + VALIDATION_ERROR_TYPES.MISSING_TAG_DIMENSION, + ), + uiControls: { + isShowingTemplateTagsEditor: false, + }, + toggleTemplateTagsEditor: jest.fn(), + }; + }); + + describe("when using an error that does not have an associated action", () => { + const errorWithoutType = new ValidationError("oof"); // undefined action type + beforeEach(() => { + props.error = errorWithoutType; + render(<ErrorActionButton {...props} />); + }); + + it("should not render an action button", () => { + expect(screen.queryByRole("button")).not.toBeInTheDocument(); + }); + }); + + describe("when using an error with an associated action", () => { + const [buttonLabel] = BUTTON_ACTIONS[ + VALIDATION_ERROR_TYPES.MISSING_TAG_DIMENSION + ]; + + beforeEach(() => { + render(<ErrorActionButton {...props} />); + }); + + it("should render an action button using the button label it is mapped to", () => { + expect( + screen.getByRole("button", { + name: buttonLabel, + }), + ).toBeInTheDocument(); + }); + }); + + describe("when clicking an ErrorActionButton mapped to the MISSING_TAG_DIMENSION validation error", () => { + const validationError = new ValidationError( + "oof", + VALIDATION_ERROR_TYPES.MISSING_TAG_DIMENSION, + ); + const [buttonLabel] = BUTTON_ACTIONS[ + VALIDATION_ERROR_TYPES.MISSING_TAG_DIMENSION + ]; + + describe("when `isShowingTemplateTagsEditor` is falsy", () => { + beforeEach(() => { + props.error = validationError; + render(<ErrorActionButton {...props} />); + }); + + it("should call the toggleTemplateTagsEditor action", () => { + userEvent.click( + screen.getByRole("button", { + name: buttonLabel, + }), + ); + expect(props.toggleTemplateTagsEditor).toHaveBeenCalled(); + }); + }); + + describe("when `isShowingTemplateTagsEditor` is true", () => { + beforeEach(() => { + props.error = validationError; + props.uiControls.isShowingTemplateTagsEditor = true; + render(<ErrorActionButton {...props} />); + }); + + it("should not call the toggleTemplateTagsEditor action", () => { + userEvent.click( + screen.getByRole("button", { + name: buttonLabel, + }), + ); + expect(props.toggleTemplateTagsEditor).not.toHaveBeenCalled(); + }); + }); + }); +}); diff --git a/frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.styled.tsx b/frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.styled.tsx new file mode 100644 index 00000000000..33b54b187fa --- /dev/null +++ b/frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.styled.tsx @@ -0,0 +1,34 @@ +import styled from "@emotion/styled"; + +import Button from "metabase/core/components/Button"; +import { color } from "metabase/lib/colors"; + +export const QueryValidationErrorRoot = styled.div` + height: 100%; + display: flex; + align-items: center; + justify-content: center; + flex-direction: column; + row-gap: 0.75rem; +`; + +export const QueryValidationErrorHeader = styled.div` + font-size: 20px; + font-weight: bold; + color: ${color("text-medium")}; +`; + +export const QueryValidationErrorMessage = styled.div` + color: ${color("text-medium")}; +`; + +export const QueryErrorActionButton = styled(Button)` + color: ${color("brand")}; + border: none; + padding: 0; + + &:hover { + text-decoration: underline; + background-color: transparent; + } +`; diff --git a/frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.tsx b/frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.tsx new file mode 100644 index 00000000000..023d0e62670 --- /dev/null +++ b/frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.tsx @@ -0,0 +1,36 @@ +import React from "react"; +import { t } from "ttag"; + +import { ValidationError } from "metabase-lib/lib/ValidationError"; + +import { + QueryValidationErrorRoot, + QueryValidationErrorHeader, + QueryValidationErrorMessage, +} from "./QueryValidationError.styled"; +import ErrorActionButton from "./ErrorActionButton"; + +type QueryBuilderUiControls = { + isShowingTemplateTagsEditor?: boolean; +}; + +export type QueryValidationErrorProps = { + error: Error | ValidationError; +}; + +type ErrorActionButton = QueryValidationErrorProps & { + uiControls: QueryBuilderUiControls; + toggleTemplateTagsEditor: () => void; +}; + +function QueryValidationError({ error }: QueryValidationErrorProps) { + return ( + <QueryValidationErrorRoot> + <QueryValidationErrorHeader>{t`Something's wrong with your question`}</QueryValidationErrorHeader> + <QueryValidationErrorMessage>{error.message}</QueryValidationErrorMessage> + <ErrorActionButton error={error} /> + </QueryValidationErrorRoot> + ); +} + +export default QueryValidationError; diff --git a/frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.unit.spec.tsx b/frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.unit.spec.tsx new file mode 100644 index 00000000000..169982421df --- /dev/null +++ b/frontend/src/metabase/query_builder/components/QueryValidationError/QueryValidationError.unit.spec.tsx @@ -0,0 +1,53 @@ +import React from "react"; +import { renderWithProviders, screen } from "__support__/ui"; + +import { + ValidationError, + VALIDATION_ERROR_TYPES, +} from "metabase-lib/lib/ValidationError"; + +import QueryValidationError from "./QueryValidationError"; + +const providers = { + reducers: { + qb: () => ({}), + }, +}; + +describe("QueryValidationError", () => { + describe("when using an Error", () => { + const error = new Error("oof"); + beforeEach(() => { + renderWithProviders(<QueryValidationError error={error} />, providers); + }); + + it("should render the error message", () => { + expect(screen.getByText("oof")).toBeInTheDocument(); + }); + + it("should not render an action button because there is no associated action", () => { + expect(screen.queryByRole("button")).not.toBeInTheDocument(); + }); + }); + + describe("when using a ValidationError with an associated action", () => { + const validationError = new ValidationError( + "oof", + VALIDATION_ERROR_TYPES.MISSING_TAG_DIMENSION, + ); + beforeEach(() => { + renderWithProviders( + <QueryValidationError error={validationError} />, + providers, + ); + }); + + it("should render the error message", () => { + expect(screen.getByText("oof")).toBeInTheDocument(); + }); + + it("should render an action button", () => { + expect(screen.getByRole("button")).toBeInTheDocument(); + }); + }); +}); diff --git a/frontend/src/metabase/query_builder/components/QueryValidationError/index.ts b/frontend/src/metabase/query_builder/components/QueryValidationError/index.ts new file mode 100644 index 00000000000..c88a26c6132 --- /dev/null +++ b/frontend/src/metabase/query_builder/components/QueryValidationError/index.ts @@ -0,0 +1 @@ +export { default } from "./QueryValidationError"; diff --git a/frontend/src/metabase/query_builder/components/view/View.jsx b/frontend/src/metabase/query_builder/components/view/View.jsx index 0b8648060c0..7d1a5baec36 100644 --- a/frontend/src/metabase/query_builder/components/view/View.jsx +++ b/frontend/src/metabase/query_builder/components/view/View.jsx @@ -1,9 +1,11 @@ /* eslint-disable react/prop-types */ import React from "react"; import { Motion, spring } from "react-motion"; +import _ from "underscore"; import ExplicitSize from "metabase/components/ExplicitSize"; import Popover from "metabase/components/Popover"; +import QueryValidationError from "metabase/query_builder/components/QueryValidationError"; import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper"; import NativeQuery from "metabase-lib/lib/queries/NativeQuery"; @@ -285,6 +287,8 @@ export default class View extends React.Component { const isStructured = query instanceof StructuredQuery; const isNative = query instanceof NativeQuery; + const validationError = _.first(query.validate?.()); + const topQuery = isStructured && query.topLevelQuery(); // only allow editing of series for structured queries @@ -322,17 +326,21 @@ export default class View extends React.Component { setIsPreviewing={setIsPreviewing} /> - <StyledDebouncedFrame enabled={!isLiveResizable}> - <QueryVisualization - {...this.props} - noHeader - className="spread" - onAddSeries={onAddSeries} - onEditSeries={onEditSeries} - onRemoveSeries={onRemoveSeries} - onEditBreakout={onEditBreakout} - /> - </StyledDebouncedFrame> + {validationError ? ( + <QueryValidationError error={validationError} /> + ) : ( + <StyledDebouncedFrame enabled={!isLiveResizable}> + <QueryVisualization + {...this.props} + noHeader + className="spread" + onAddSeries={onAddSeries} + onEditSeries={onEditSeries} + onRemoveSeries={onRemoveSeries} + onEditBreakout={onEditBreakout} + /> + </StyledDebouncedFrame> + )} {ModeFooter && ( <ModeFooter {...this.props} className="flex-no-shrink" /> diff --git a/frontend/test/metabase-lib/lib/Dimension.unit.spec.js b/frontend/test/metabase-lib/lib/Dimension.unit.spec.js index 217653afff6..8c2f5cc943a 100644 --- a/frontend/test/metabase-lib/lib/Dimension.unit.spec.js +++ b/frontend/test/metabase-lib/lib/Dimension.unit.spec.js @@ -977,6 +977,12 @@ describe("Dimension", () => { }); }); + describe("isValidDimensionType", () => { + it("should evaluate to true", () => { + expect(dimension.isValidDimensionType()).toBe(true); + }); + }); + describe("isVariableType", () => { it("should evaluate to false", () => { expect(dimension.isVariableType()).toBe(false); @@ -1185,5 +1191,71 @@ describe("Dimension", () => { }); }); }); + + describe("broken dimension tag", () => { + const templateTagClause = ["template-tag", "foo"]; + const query = new NativeQuery(PRODUCTS.question(), { + database: SAMPLE_DATABASE.id, + type: "native", + native: { + query: "select * from PRODUCTS where {{foo}}", + "template-tags": { + foo: { + id: "5928ca74-ca36-8706-7bed-0143d7646b6a", + name: "foo", + "display-name": "Foo", + type: "dimension", + "widget-type": "category", + // this should be defined + dimension: null, + }, + }, + }, + }); + + const brokenDimension = Dimension.parseMBQL( + templateTagClause, + metadata, + query, + ); + + describe("instance methods", () => { + describe("isDimensionType", () => { + it("should evaluate to true", () => { + expect(brokenDimension.isDimensionType()).toBe(true); + }); + }); + + describe("isValidDimensionType", () => { + it("should return false", () => { + expect(brokenDimension.isValidDimensionType()).toBe(false); + }); + }); + + describe("isVariableType", () => { + it("should evaluate to false", () => { + expect(brokenDimension.isVariableType()).toBe(false); + }); + }); + + describe("field", () => { + it("should evaluate to null", () => { + expect(brokenDimension.field()).toBeNull(); + }); + }); + + describe("name", () => { + it("should evaluate to the tag's name instead of the field's", () => { + expect(brokenDimension.name()).toEqual("foo"); + }); + }); + + describe("icon", () => { + it("should use a fallback icon", () => { + expect(brokenDimension.icon()).toEqual("label"); + }); + }); + }); + }); }); }); 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 827e1d21db9..3ed07478ba6 100644 --- a/frontend/test/metabase-lib/lib/queries/NativeQuery.unit.spec.js +++ b/frontend/test/metabase-lib/lib/queries/NativeQuery.unit.spec.js @@ -205,6 +205,7 @@ describe("NativeQuery", () => { q = q.setDatasetQuery( assocIn(q.datasetQuery(), ["native", "template-tags", "foo"], { + name: "foo", type: "dimension", "widget-type": "category", dimension: ["field", 123, null], -- GitLab