From e48ab063803028ffef153344b8d55ae3b1ac6a29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20Benk=C5=91?= Date: Sat, 18 Nov 2023 02:37:20 +0300 Subject: [PATCH 01/96] Add expression-clause-for-legacy-expression Fixes #34830. --- frontend/src/metabase-lib/expression.ts | 8 ++++++++ src/metabase/lib/js.cljs | 6 ++++++ test/metabase/lib/js_test.cljs | 12 ++++++++++++ 3 files changed, 26 insertions(+) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index d71965ec314dd..8fa8a85dd4659 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -50,3 +50,11 @@ export function expressionClause( ): ExpressionClause { return ML.expression_clause(operator, args, options); } + +export function expressionClauseForLegacyExpression( + query: Query, + stageIndex: number, + mbql: any, +): ExpressionClause { + return ML.expression_clause_for_legacy_expression(query, stageIndex, mbql); +} diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs index f367419267586..72e190b6be648 100644 --- a/src/metabase/lib/js.cljs +++ b/src/metabase/lib/js.cljs @@ -1038,3 +1038,9 @@ "Returns the count of stages in query" [a-query] (lib.core/stage-count a-query)) + +(defn ^:export expression-clause-for-legacy-expression + "Create an expression clause from `legacy-expression` at stage `stage-number` of `a-query`." + [a-query stage-number legacy-expression] + (lib.convert/with-aggregation-list (lib.core/aggregations a-query stage-number) + (lib.convert/->pMBQL legacy-expression))) diff --git a/test/metabase/lib/js_test.cljs b/test/metabase/lib/js_test.cljs index 484e599a586e2..f9b2454827f1f 100644 --- a/test/metabase/lib/js_test.cljs +++ b/test/metabase/lib/js_test.cljs @@ -2,9 +2,12 @@ (:require [clojure.test :refer [deftest is testing]] [goog.object :as gobject] + [metabase.lib.core :as lib] [metabase.lib.js :as lib.js] + [metabase.lib.options :as lib.options] [metabase.lib.test-metadata :as meta] [metabase.lib.test-util :as lib.tu] + [metabase.test-runner.assert-exprs.approximately-equal] [metabase.test.util.js :as test.js])) (deftest ^:parallel query=-test @@ -137,3 +140,12 @@ (deftest ^:parallel cljs-key->js-key-test (is (= "isManyPks" (#'lib.js/cljs-key->js-key :many-pks?)))) + +(deftest ^:parallel expression-clause-for-legacy-expression-test + (let [query (-> lib.tu/venues-query + (lib/expression "double-price" (lib/* (meta/field-metadata :venues :price) 2)) + (lib/aggregate (lib/sum [:expression {:lib/uuid (str (random-uuid))} "double-price"]))) + agg-uuid (-> query lib/aggregations first lib.options/uuid) + legacy-expr [:> [:aggregation 0] 100]] + (is (=? [:> {} [:aggregation {} agg-uuid] 100] + (lib.js/expression-clause-for-legacy-expression query -1 legacy-expr))))) From e4df56efff49dfb1db17adbe2ca87dcbf996b7aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20Benk=C5=91?= Date: Mon, 20 Nov 2023 22:40:44 +0300 Subject: [PATCH 02/96] Add legacy-expression-for-expression-clause --- frontend/src/metabase-lib/expression.ts | 12 ++++++++++++ src/metabase/lib/js.cljs | 6 ++++++ test/metabase/lib/js_test.cljs | 12 ++++++++---- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index 8fa8a85dd4659..3a3b2a826f8c8 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -58,3 +58,15 @@ export function expressionClauseForLegacyExpression( ): ExpressionClause { return ML.expression_clause_for_legacy_expression(query, stageIndex, mbql); } + +export function LegacyExpressionForExpressionClause( + query: Query, + stageIndex: number, + expressionClause: ExpressionClause, +): any { + return ML.legacy_expression_for_expression_clause( + query, + stageIndex, + expressionClause, + ); +} diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs index 72e190b6be648..130b835f832bc 100644 --- a/src/metabase/lib/js.cljs +++ b/src/metabase/lib/js.cljs @@ -1044,3 +1044,9 @@ [a-query stage-number legacy-expression] (lib.convert/with-aggregation-list (lib.core/aggregations a-query stage-number) (lib.convert/->pMBQL legacy-expression))) + +(defn ^:export legacy-expression-for-expression-clause + "Create a legacy expression from `an-expression-clause` at stage `stage-number` of `a-query`." + [a-query stage-number an-expression-clause] + (lib.convert/with-aggregation-list (lib.core/aggregations a-query stage-number) + (lib.convert/->legacy-MBQL an-expression-clause))) diff --git a/test/metabase/lib/js_test.cljs b/test/metabase/lib/js_test.cljs index f9b2454827f1f..c0164d2a48f0d 100644 --- a/test/metabase/lib/js_test.cljs +++ b/test/metabase/lib/js_test.cljs @@ -141,11 +141,15 @@ (is (= "isManyPks" (#'lib.js/cljs-key->js-key :many-pks?)))) -(deftest ^:parallel expression-clause-for-legacy-expression-test +(deftest ^:parallel expression-clause-<->-legacy-expression-test (let [query (-> lib.tu/venues-query (lib/expression "double-price" (lib/* (meta/field-metadata :venues :price) 2)) (lib/aggregate (lib/sum [:expression {:lib/uuid (str (random-uuid))} "double-price"]))) agg-uuid (-> query lib/aggregations first lib.options/uuid) - legacy-expr [:> [:aggregation 0] 100]] - (is (=? [:> {} [:aggregation {} agg-uuid] 100] - (lib.js/expression-clause-for-legacy-expression query -1 legacy-expr))))) + legacy-expr [:> [:aggregation 0] 100] + pmbql-expr (lib.js/expression-clause-for-legacy-expression query -1 legacy-expr) + legacy-expr' (lib.js/legacy-expression-for-expression-clause query -1 pmbql-expr)] + (testing "from legacy expression" + (is (=? [:> {} [:aggregation {} agg-uuid] 100] pmbql-expr))) + (testing "from pMBQL expression" + (is (=? legacy-expr legacy-expr'))))) From e93d1142eee3be1ecc6b1f32826654d9a8177fe8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20Benk=C5=91?= Date: Mon, 20 Nov 2023 23:16:03 +0300 Subject: [PATCH 03/96] Fix wrapper function name --- frontend/src/metabase-lib/expression.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index 3a3b2a826f8c8..f4e63df700949 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -59,7 +59,7 @@ export function expressionClauseForLegacyExpression( return ML.expression_clause_for_legacy_expression(query, stageIndex, mbql); } -export function LegacyExpressionForExpressionClause( +export function legacyExpressionForExpressionClause( query: Query, stageIndex: number, expressionClause: ExpressionClause, From 0994d762eb50f142a6649b8949db535c3dc433cc Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Tue, 21 Nov 2023 20:05:23 +0700 Subject: [PATCH 04/96] Include ExpressionClause in Aggregatable type --- frontend/src/metabase-lib/types.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frontend/src/metabase-lib/types.ts b/frontend/src/metabase-lib/types.ts index 61f3346fa558a..abbe26ad5d9e8 100644 --- a/frontend/src/metabase-lib/types.ts +++ b/frontend/src/metabase-lib/types.ts @@ -25,7 +25,10 @@ export type MetricMetadata = unknown & { _opaque: typeof MetricMetadata }; declare const AggregationClause: unique symbol; export type AggregationClause = unknown & { _opaque: typeof AggregationClause }; -export type Aggregatable = AggregationClause | MetricMetadata; +export type Aggregatable = + | AggregationClause + | MetricMetadata + | ExpressionClause; declare const AggregationOperator: unique symbol; export type AggregationOperator = unknown & { From 55b4f315a2c4b99f3a227c3a1dcae1ba4e8fb82f Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Tue, 21 Nov 2023 20:05:40 +0700 Subject: [PATCH 05/96] Use Aggregatable type in aggregate function --- frontend/src/metabase-lib/aggregation.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/metabase-lib/aggregation.ts b/frontend/src/metabase-lib/aggregation.ts index 79931409556c1..9ee2fb3cbbacd 100644 --- a/frontend/src/metabase-lib/aggregation.ts +++ b/frontend/src/metabase-lib/aggregation.ts @@ -1,9 +1,9 @@ import * as ML from "cljs/metabase.lib.js"; import type { + Aggregatable, AggregationClause, AggregationOperator, ColumnMetadata, - MetricMetadata, Query, } from "./types"; @@ -30,7 +30,7 @@ export function selectedAggregationOperators( export function aggregate( query: Query, stageIndex: number, - clause: AggregationClause | MetricMetadata, + clause: Aggregatable, ): Query { return ML.aggregate(query, stageIndex, clause); } From 95697f7475775b8f8e2216675789e70366ad2f3d Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Tue, 21 Nov 2023 21:55:50 +0700 Subject: [PATCH 06/96] Rename query to legacyQuery in Expressions (part 1) --- .../expressions/__support__/expressions.js | 6 ++-- .../metabase-lib/expressions/diagnostics.js | 12 +++---- .../src/metabase-lib/expressions/index.js | 12 +++---- .../src/metabase-lib/expressions/suggest.ts | 16 +++++----- .../expressions/suggest.unit.spec.ts | 28 ++++++++--------- .../AggregationPicker/AggregationPicker.tsx | 2 +- .../ExpressionEditorTextfield.tsx | 31 +++++++++++-------- .../expressions/ExpressionWidget.tsx | 6 ++-- .../ExpressionWidget.unit.spec.tsx | 2 +- 9 files changed, 60 insertions(+), 55 deletions(-) diff --git a/frontend/src/metabase-lib/expressions/__support__/expressions.js b/frontend/src/metabase-lib/expressions/__support__/expressions.js index e8227900a6b71..be1b0f143cf20 100644 --- a/frontend/src/metabase-lib/expressions/__support__/expressions.js +++ b/frontend/src/metabase-lib/expressions/__support__/expressions.js @@ -106,6 +106,6 @@ const metadata = createMockMetadata({ ], }); -export const query = metadata.table(TABLE_ID).query(); -export const expressionOpts = { query, startRule: "expression" }; -export const aggregationOpts = { query, startRule: "aggregation" }; +export const legacyQuery = metadata.table(TABLE_ID).query(); +export const expressionOpts = { legacyQuery, startRule: "expression" }; +export const aggregationOpts = { legacyQuery, startRule: "aggregation" }; diff --git a/frontend/src/metabase-lib/expressions/diagnostics.js b/frontend/src/metabase-lib/expressions/diagnostics.js index ce7cc73975b94..2145dff86cc00 100644 --- a/frontend/src/metabase-lib/expressions/diagnostics.js +++ b/frontend/src/metabase-lib/expressions/diagnostics.js @@ -35,11 +35,11 @@ export function countMatchingParentheses(tokens) { * @private * @param {string} source * @param {string} startRule - * @param {object} query + * @param {object} legacyQuery * @param {string | null} name * @returns {ErrorWithMessage | null} */ -export function diagnose(source, startRule, query, name = null) { +export function diagnose(source, startRule, legacyQuery, name = null) { if (!source || source.length === 0) { return null; } @@ -82,15 +82,15 @@ export function diagnose(source, startRule, query, name = null) { } try { - return prattCompiler(source, startRule, query, name); + return prattCompiler(source, startRule, legacyQuery, name); } catch (err) { return err; } } -function prattCompiler(source, startRule, query, name) { +function prattCompiler(source, startRule, legacyQuery, name) { const tokens = lexify(source); - const options = { source, startRule, query, name }; + const options = { source, startRule, legacyQuery, name }; // PARSE const { root, errors } = parse(tokens, { @@ -102,7 +102,7 @@ function prattCompiler(source, startRule, query, name) { } function resolveMBQLField(kind, name, node) { - if (!query) { + if (!legacyQuery) { return [kind, name]; } if (kind === "metric") { diff --git a/frontend/src/metabase-lib/expressions/index.js b/frontend/src/metabase-lib/expressions/index.js index c052bf7e01e95..07055a15b7362 100644 --- a/frontend/src/metabase-lib/expressions/index.js +++ b/frontend/src/metabase-lib/expressions/index.js @@ -58,8 +58,8 @@ function isReservedWord(word) { // METRICS -export function parseMetric(metricName, { query }) { - return query +export function parseMetric(metricName, { legacyQuery }) { + return legacyQuery .table() .metrics.find( metric => metric.name.toLowerCase() === metricName.toLowerCase(), @@ -72,8 +72,8 @@ export function formatMetricName(metric, options) { // SEGMENTS -export function parseSegment(segmentName, { query }) { - const table = query.table(); +export function parseSegment(segmentName, { legacyQuery }) { + const table = legacyQuery.table(); const segment = table.segments.find( segment => segment.name.toLowerCase() === segmentName.toLowerCase(), ); @@ -98,9 +98,9 @@ export function formatSegmentName(segment, options) { /** * Find dimension with matching `name` in query. TODO - How is this "parsing" a dimension? Not sure about this name. */ -export function parseDimension(name, { reference, query }) { +export function parseDimension(name, { reference, legacyQuery }) { // FIXME: this is pretty inefficient, create a lookup table? - return query + return legacyQuery .dimensionOptions() .all() .filter( diff --git a/frontend/src/metabase-lib/expressions/suggest.ts b/frontend/src/metabase-lib/expressions/suggest.ts index 4ff31a466f9e0..436077ca0cd8e 100644 --- a/frontend/src/metabase-lib/expressions/suggest.ts +++ b/frontend/src/metabase-lib/expressions/suggest.ts @@ -46,7 +46,7 @@ const suggestionText = (func: MBQLClauseFunctionConfig) => { type SuggestArgs = { source: string; - query: StructuredQuery; + legacyQuery: StructuredQuery; reportTimezone?: string; startRule: string; targetOffset?: number; @@ -54,7 +54,7 @@ type SuggestArgs = { export function suggest({ source, - query, + legacyQuery, reportTimezone, startRule, targetOffset = source.length, @@ -72,7 +72,7 @@ export function suggest({ const functionDisplayName = enclosingFunction(partialSource); if (functionDisplayName) { const name = getMBQLName(functionDisplayName); - const database = query.database(); + const database = legacyQuery.database(); if (name && database) { const helpText = getHelpText(name, database, reportTimezone); @@ -103,7 +103,7 @@ export function suggest({ }, ); - const database = query.database(); + const database = legacyQuery.database(); if (_.first(matchPrefix) !== "[") { suggestions.push({ type: "functions", @@ -149,7 +149,7 @@ export function suggest({ if (_.last(matchPrefix) !== "]") { suggestions.push( - ...query + ...legacyQuery .dimensionOptions(() => true) .all() .map(dimension => ({ @@ -166,7 +166,7 @@ export function suggest({ })), ); - const segments = query.table()?.segments; + const segments = legacyQuery.table()?.segments; if (segments) { suggestions.push( ...segments.map(segment => ({ @@ -181,7 +181,7 @@ export function suggest({ } if (startRule === "aggregation") { - const metrics = query.table()?.metrics; + const metrics = legacyQuery.table()?.metrics; if (metrics) { suggestions.push( ...metrics.map(metric => ({ @@ -236,7 +236,7 @@ export function suggest({ const { icon } = suggestions[0]; if (icon === "function") { const name = getMBQLName(matchPrefix); - const database = query.database(); + const database = legacyQuery.database(); if (name && database) { const helpText = getHelpText(name, database, reportTimezone); diff --git a/frontend/src/metabase-lib/expressions/suggest.unit.spec.ts b/frontend/src/metabase-lib/expressions/suggest.unit.spec.ts index fc023b348a7bf..fd3c67b7aaa43 100644 --- a/frontend/src/metabase-lib/expressions/suggest.unit.spec.ts +++ b/frontend/src/metabase-lib/expressions/suggest.unit.spec.ts @@ -96,7 +96,7 @@ describe("metabase/lib/expression/suggest", () => { expect( suggest({ source: "User", - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "expression", }), ).toEqual([ @@ -121,7 +121,7 @@ describe("metabase/lib/expression/suggest", () => { expect( suggest({ source: "Foo", - query: ordersTable.query().join({ + legacyQuery: ordersTable.query().join({ alias: "Foo", "source-table": REVIEWS_ID, }), @@ -141,7 +141,7 @@ describe("metabase/lib/expression/suggest", () => { expect( suggest({ source: "T", - query: ordersTable + legacyQuery: ordersTable .query() .aggregate(["count"]) .breakout(ordersTotalField) @@ -162,7 +162,7 @@ describe("metabase/lib/expression/suggest", () => { expect( helpText({ source: "substring(", - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "expression", }), ).toMatchObject({ @@ -176,7 +176,7 @@ describe("metabase/lib/expression/suggest", () => { expect( helpText({ source: "lower", // doesn't need to be "lower(" since it's a unique match - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "expression", }), ).toMatchObject({ @@ -190,7 +190,7 @@ describe("metabase/lib/expression/suggest", () => { expect( helpText({ source: "trim(Total ", - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "expression", })?.name, ).toEqual("trim"); @@ -200,7 +200,7 @@ describe("metabase/lib/expression/suggest", () => { expect( helpText({ source: "coalesce(Total ", - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "expression", })?.name, ).toEqual("coalesce"); @@ -256,7 +256,7 @@ describe("metabase/lib/expression/suggest", () => { expect( suggest({ source: "to", - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "aggregation", }), ).toEqual([ @@ -269,7 +269,7 @@ describe("metabase/lib/expression/suggest", () => { expect( suggest({ source: "cou", - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "aggregation", }), ).toEqual([ @@ -282,7 +282,7 @@ describe("metabase/lib/expression/suggest", () => { expect( helpText({ source: "Sum(", - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "aggregation", }), ).toMatchObject({ name: "sum", example: "Sum([Subtotal])" }); @@ -294,7 +294,7 @@ describe("metabase/lib/expression/suggest", () => { expect( suggest({ source: "c", - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "boolean", }), ).toEqual([ @@ -315,7 +315,7 @@ describe("metabase/lib/expression/suggest", () => { expect( suggest({ source: "ca", - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "boolean", }), ).toEqual([ @@ -328,7 +328,7 @@ describe("metabase/lib/expression/suggest", () => { expect( suggest({ source: "[", - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "boolean", }), ).toEqual([...FIELDS_ORDERS, ...SEGMENTS_ORDERS].sort(suggestionSort)); @@ -338,7 +338,7 @@ describe("metabase/lib/expression/suggest", () => { expect( helpText({ source: "Contains(Total ", - query: ordersTable.query(), + legacyQuery: ordersTable.query(), startRule: "boolean", }), ).toMatchObject({ diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index a839b2e06cc12..3a1ea42526bef 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -213,7 +213,7 @@ export function AggregationPicker({ if (isEditingExpression) { return ( , ) { // we only refresh our state if we had no previous state OR if our expression changed - const { expression, query, startRule } = newProps; + const { expression, legacyQuery, startRule } = newProps; if (!this.state || !_.isEqual(this.props.expression, expression)) { - const source = format(expression, { query, startRule }); + const source = format(expression, { legacyQuery, startRule }); const currentSource = this.state.source; this.setState(transformPropsToState(newProps)); @@ -331,11 +331,16 @@ class ExpressionEditorTextfield extends React.Component< compileExpression() { const { source } = this.state; - const { query, startRule, name } = this.props; + const { legacyQuery, startRule, name } = this.props; if (!source || source.length === 0) { return null; } - const { expression } = processSource({ name, source, query, startRule }); + const { expression } = processSource({ + name, + source, + legacyQuery, + startRule, + }); return expression; } @@ -343,26 +348,26 @@ class ExpressionEditorTextfield extends React.Component< diagnoseExpression(): ErrorWithMessage | null { const { source } = this.state; const { - query, + legacyQuery, startRule = ExpressionEditorTextfield.defaultProps.startRule, name, } = this.props; if (!source || source.length === 0) { return { message: t`Empty expression` }; } - return diagnose(source, startRule, query, name); + return diagnose(source, startRule, legacyQuery, name); } commitExpression() { const { - query, + legacyQuery, startRule = ExpressionEditorTextfield.defaultProps.startRule, } = this.props; const { source } = this.state; const errorMessage = diagnose( source, startRule, - query, + legacyQuery, ) as ErrorWithMessage | null; this.setState({ errorMessage }); @@ -396,13 +401,13 @@ class ExpressionEditorTextfield extends React.Component< const cursor = selection.getCursor(); const { - query, + legacyQuery, reportTimezone, startRule = ExpressionEditorTextfield.defaultProps.startRule, } = this.props; const { source } = this.state; const { suggestions, helpText } = suggest({ - query, + legacyQuery, reportTimezone, startRule, source, diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index f20905c8f03b5..edb065d055fb6 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -28,7 +28,7 @@ const EXPRESSIONS_DOCUMENTATION_URL = MetabaseSettings.docsUrl( ); export interface ExpressionWidgetProps { - query: StructuredQuery; + legacyQuery: StructuredQuery; expression: Expression | undefined; name?: string; withName?: boolean; @@ -43,7 +43,7 @@ export interface ExpressionWidgetProps { export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const { - query, + legacyQuery, name: initialName, expression: initialExpression, withName = false, @@ -106,7 +106,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { expression={expression} startRule={startRule} name={name} - query={query} + legacyQuery={legacyQuery} reportTimezone={reportTimezone} textAreaId="expression-content" onChange={handleExpressionChange} diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index 44966354b3151..31b30795d26e3 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -166,7 +166,7 @@ function setup(additionalProps?: Partial) { const props = { expression: undefined, name: undefined, - query: createMockQueryForExpressions(), + legacyQuery: createMockQueryForExpressions(), reportTimezone: "UTC", ...mocks, ...additionalProps, From d6eb1dc81df63120e2710d09ef207dc1ae783d14 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 22 Nov 2023 14:53:01 +0700 Subject: [PATCH 07/96] Rename query to legacyQuery in Expressions (part 2) --- .../components/AggregationPopover/AggregationPopover.jsx | 2 +- .../components/filters/FilterPopover/FilterPopover.tsx | 2 +- .../query_builder/components/notebook/steps/ExpressionStep.tsx | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/AggregationPopover/AggregationPopover.jsx b/frontend/src/metabase/query_builder/components/AggregationPopover/AggregationPopover.jsx index 1f39ec14f0a0a..1acdd2e087861 100644 --- a/frontend/src/metabase/query_builder/components/AggregationPopover/AggregationPopover.jsx +++ b/frontend/src/metabase/query_builder/components/AggregationPopover/AggregationPopover.jsx @@ -332,7 +332,7 @@ export default class AggregationPopover extends Component { return ( } diff --git a/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx b/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx index 0e9c0a7e174ae..864c99eb4149e 100644 --- a/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx +++ b/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx @@ -23,7 +23,7 @@ const ExpressionStep = ({ readOnly={readOnly} renderPopover={item => ( Date: Wed, 22 Nov 2023 15:14:15 +0700 Subject: [PATCH 08/96] Rename query to legacyQuery in Expressions (part 3) --- .../expressions/__support__/shared.js | 8 ++++---- .../src/metabase-lib/expressions/format.js | 18 ++++++++++-------- .../metabase-lib/queries/StructuredQuery.ts | 2 +- .../lib/expressions/index.unit.spec.js | 2 +- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/frontend/src/metabase-lib/expressions/__support__/shared.js b/frontend/src/metabase-lib/expressions/__support__/shared.js index 64634988007ed..2d0398b7de2e5 100644 --- a/frontend/src/metabase-lib/expressions/__support__/shared.js +++ b/frontend/src/metabase-lib/expressions/__support__/shared.js @@ -62,7 +62,7 @@ const userName = metadata const segment = metadata.segment(SEGMENT_ID).filterClause(); const metric = metadata.metric(METRIC_ID).aggregationClause(); -const query = metadata.table(ORDERS_ID).query().addExpression("foo", 42); +const legacyQuery = metadata.table(ORDERS_ID).query().addExpression("foo", 42); // shared test cases used in compile, formatter, and syntax tests: // @@ -287,9 +287,9 @@ const filter = [ ]; export default [ - ["expression", expression, { startRule: "expression", query }], - ["aggregation", aggregation, { startRule: "aggregation", query }], - ["filter", filter, { startRule: "boolean", query }], + ["expression", expression, { startRule: "expression", legacyQuery }], + ["aggregation", aggregation, { startRule: "aggregation", legacyQuery }], + ["filter", filter, { startRule: "boolean", legacyQuery }], ]; /** diff --git a/frontend/src/metabase-lib/expressions/format.js b/frontend/src/metabase-lib/expressions/format.js index 854dffa094fe3..8b1e9c826722f 100644 --- a/frontend/src/metabase-lib/expressions/format.js +++ b/frontend/src/metabase-lib/expressions/format.js @@ -60,18 +60,20 @@ function formatNumberLiteral(mbql) { } function formatDimension(fieldRef, options) { - const { query } = options; - if (query) { - const dimension = query.parseFieldReference(fieldRef); + const { legacyQuery } = options; + if (legacyQuery) { + const dimension = legacyQuery.parseFieldReference(fieldRef); return formatDimensionName(dimension, options); } else { - throw new Error("`query` is a required parameter to format expressions"); + throw new Error( + "`legacyQuery` is a required parameter to format expressions", + ); } } function formatMetric([, metricId], options) { - const { query } = options; - const metric = _.findWhere(query.table().metrics, { id: metricId }); + const { legacyQuery } = options; + const metric = _.findWhere(legacyQuery.table().metrics, { id: metricId }); if (!metric) { throw "metric with ID does not exist: " + metricId; } @@ -79,8 +81,8 @@ function formatMetric([, metricId], options) { } function formatSegment([, segmentId], options) { - const { query } = options; - const segment = _.findWhere(query.table().segments, { id: segmentId }); + const { legacyQuery } = options; + const segment = _.findWhere(legacyQuery.table().segments, { id: segmentId }); if (!segment) { throw "segment with ID does not exist: " + segment; } diff --git a/frontend/src/metabase-lib/queries/StructuredQuery.ts b/frontend/src/metabase-lib/queries/StructuredQuery.ts index ace8933404432..b58d5ddcf5cdd 100644 --- a/frontend/src/metabase-lib/queries/StructuredQuery.ts +++ b/frontend/src/metabase-lib/queries/StructuredQuery.ts @@ -728,7 +728,7 @@ class StructuredQuery extends AtomicQuery { return formatExpression(expression, { quotes, ...options, - query: this, + legacyQuery: this, }); } diff --git a/frontend/test/metabase/lib/expressions/index.unit.spec.js b/frontend/test/metabase/lib/expressions/index.unit.spec.js index 306ea703e0053..e3dae5cb01547 100644 --- a/frontend/test/metabase/lib/expressions/index.unit.spec.js +++ b/frontend/test/metabase/lib/expressions/index.unit.spec.js @@ -1,6 +1,6 @@ import { quoteString, unquoteString } from "metabase-lib/expressions"; -describe("metabase-lib/expressions/format", () => { +describe("metabase-lib/expressions", () => { // double- and single-quote const dq = str => quoteString(str, '"'); const sq = str => quoteString(str, "'"); From 3e4fb53c0f29ac5b01d809ddfdfa99168ec1d7f5 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 22 Nov 2023 21:13:10 +0700 Subject: [PATCH 09/96] Support MLv2 expressionClause in processSource and ExpressionWidget --- .../src/metabase-lib/expressions/process.js | 17 ++++++- .../AggregationPicker/AggregationPicker.tsx | 2 + .../ExpressionEditorTextfield.tsx | 51 ++++++++++++++----- .../expressions/ExpressionWidget.tsx | 7 +++ .../ExpressionWidget.unit.spec.tsx | 7 ++- .../notebook/steps/ExpressionStep.tsx | 4 ++ 6 files changed, 70 insertions(+), 18 deletions(-) diff --git a/frontend/src/metabase-lib/expressions/process.js b/frontend/src/metabase-lib/expressions/process.js index 3ff300b5945b2..74db6bf96d1ca 100644 --- a/frontend/src/metabase-lib/expressions/process.js +++ b/frontend/src/metabase-lib/expressions/process.js @@ -1,5 +1,7 @@ import { t } from "ttag"; +import * as Lib from "metabase-lib"; + import { parse, adjustBooleans } from "./recursive-parser"; import { resolve } from "./resolver"; @@ -31,13 +33,23 @@ export function processSource(options) { } }; - const { source, startRule } = options; + const { source, query, stageIndex, startRule } = options; - let expression; + let expression = null; + let expressionClause = null; let compileError; try { const parsed = parse(source); expression = adjustBooleans(resolve(parsed, startRule, resolveMBQLField)); + + // query and stageIndex are not available outside of notebook editor (e.g. in Metrics or Segments). + if (typeof query !== "undefined" && typeof stageIndex !== "undefined") { + expressionClause = Lib.expressionClauseForLegacyExpression( + query, + stageIndex, + expression, + ); + } } catch (e) { console.warn("compile error", e); compileError = e; @@ -46,6 +58,7 @@ export function processSource(options) { return { source, expression, + expressionClause, compileError, }; } diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 3a1ea42526bef..cb6f3e11cdcd9 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -214,6 +214,8 @@ export function AggregationPicker({ return ( void; + onChange: ( + expression: Expression | null, + expressionClause: Lib.ExpressionClause | null, + ) => void; onError: (error: ErrorWithMessage | null) => void; onBlankChange: (isBlank: boolean) => void; - onCommit: (expression: Expression | null) => void; + onCommit: ( + expression: Expression | null, + expressionClause: Lib.ExpressionClause | null, + ) => void; helpTextTarget: RefObject; } @@ -279,6 +288,8 @@ class ExpressionEditorTextfield extends React.Component< return; } + const { onChange, onError } = this.props; + this.clearSuggestions(); const errorMessage = this.diagnoseExpression(); @@ -286,16 +297,20 @@ class ExpressionEditorTextfield extends React.Component< // whenever our input blurs we push the updated expression to our parent if valid if (errorMessage) { - this.props.onError(errorMessage); + onError(errorMessage); } else { - const expression = this.compileExpression(); - if (expression) { + const compiledExpression = this.compileExpression(); + + if (compiledExpression) { + const { expression, expressionClause } = compiledExpression; + if (!isExpression(expression)) { console.warn("isExpression=false", expression); } - this.props.onChange(expression); + + onChange(expression, expressionClause); } else { - this.props.onError({ message: t`Invalid expression` }); + onError({ message: t`Invalid expression` }); } } }; @@ -331,18 +346,20 @@ class ExpressionEditorTextfield extends React.Component< compileExpression() { const { source } = this.state; - const { legacyQuery, startRule, name } = this.props; + const { legacyQuery, query, stageIndex, startRule, name } = this.props; if (!source || source.length === 0) { return null; } - const { expression } = processSource({ + const { expression, expressionClause } = processSource({ name, source, + query, legacyQuery, + stageIndex, startRule, }); - return expression; + return { expression, expressionClause }; } diagnoseExpression(): ErrorWithMessage | null { @@ -362,6 +379,8 @@ class ExpressionEditorTextfield extends React.Component< const { legacyQuery, startRule = ExpressionEditorTextfield.defaultProps.startRule, + onCommit, + onError, } = this.props; const { source } = this.state; const errorMessage = diagnose( @@ -372,12 +391,16 @@ class ExpressionEditorTextfield extends React.Component< this.setState({ errorMessage }); if (errorMessage) { - this.props.onError(errorMessage); + onError(errorMessage); } else { - const expression = this.compileExpression(); + const compiledExpression = this.compileExpression(); + + if (compiledExpression) { + const { expression, expressionClause } = compiledExpression; - if (isExpression(expression)) { - this.props.onCommit(expression); + if (isExpression(expression)) { + onCommit(expression, expressionClause); + } } } } diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index edb065d055fb6..8d7098f415757 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -7,6 +7,7 @@ import Input from "metabase/core/components/Input/Input"; import Tooltip from "metabase/core/components/Tooltip"; import MetabaseSettings from "metabase/lib/settings"; import type { Expression } from "metabase-types/api"; +import type * as Lib from "metabase-lib"; import { isExpression } from "metabase-lib/expressions"; import type StructuredQuery from "metabase-lib/queries/StructuredQuery"; @@ -29,6 +30,8 @@ const EXPRESSIONS_DOCUMENTATION_URL = MetabaseSettings.docsUrl( export interface ExpressionWidgetProps { legacyQuery: StructuredQuery; + query?: Lib.Query; + stageIndex?: number; expression: Expression | undefined; name?: string; withName?: boolean; @@ -44,6 +47,8 @@ export interface ExpressionWidgetProps { export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const { legacyQuery, + query, + stageIndex, name: initialName, expression: initialExpression, withName = false, @@ -107,6 +112,8 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { startRule={startRule} name={name} legacyQuery={legacyQuery} + query={query} + stageIndex={stageIndex} reportTimezone={reportTimezone} textAreaId="expression-content" onChange={handleExpressionChange} diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index 31b30795d26e3..5c681919f518f 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -9,6 +9,7 @@ import { createSampleDatabase, ORDERS_ID, } from "metabase-types/api/mocks/presets"; +import { createQuery } from "metabase-lib/test-helpers"; import { ExpressionWidgetHeader } from "./ExpressionWidgetHeader"; import type { ExpressionWidgetProps } from "./ExpressionWidget"; import { ExpressionWidget } from "./ExpressionWidget"; @@ -144,7 +145,7 @@ describe("ExpressionWidget", () => { }); }); -const createMockQueryForExpressions = () => { +const createMockLegacyQueryForExpressions = () => { const state = createMockState({ entities: createMockEntitiesState({ databases: [createSampleDatabase()], @@ -166,7 +167,9 @@ function setup(additionalProps?: Partial) { const props = { expression: undefined, name: undefined, - legacyQuery: createMockQueryForExpressions(), + legacyQuery: createMockLegacyQueryForExpressions(), + query: createQuery(), + stageIndex: 0, reportTimezone: "UTC", ...mocks, ...additionalProps, diff --git a/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx b/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx index 864c99eb4149e..6dacbb7c1f714 100644 --- a/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx +++ b/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx @@ -6,6 +6,8 @@ import ClauseStep from "./ClauseStep"; const ExpressionStep = ({ color, query, + topLevelQuery, + step, updateQuery, isLastOpened, reportTimezone, @@ -24,6 +26,8 @@ const ExpressionStep = ({ renderPopover={item => ( Date: Wed, 22 Nov 2023 21:21:50 +0700 Subject: [PATCH 10/96] Revert changes to ExpressionStep as it's out of scope of this task --- .../components/notebook/steps/ExpressionStep.tsx | 4 ---- 1 file changed, 4 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx b/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx index 6dacbb7c1f714..864c99eb4149e 100644 --- a/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx +++ b/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx @@ -6,8 +6,6 @@ import ClauseStep from "./ClauseStep"; const ExpressionStep = ({ color, query, - topLevelQuery, - step, updateQuery, isLastOpened, reportTimezone, @@ -26,8 +24,6 @@ const ExpressionStep = ({ renderPopover={item => ( Date: Thu, 23 Nov 2023 16:35:33 +0700 Subject: [PATCH 11/96] Use new expressionClause in ExpressionWidget --- .../expressions/ExpressionWidget.tsx | 37 ++++++++++++++----- .../ExpressionWidget.unit.spec.tsx | 3 +- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index 8d7098f415757..483373deaea6b 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -33,13 +33,22 @@ export interface ExpressionWidgetProps { query?: Lib.Query; stageIndex?: number; expression: Expression | undefined; + /** + * Presence of this prop is not enforced due to backwards-compatibility + * with ExpressionWidget usages outside of GUI editor. + */ + expressionClause?: Lib.ExpressionClause | undefined; name?: string; withName?: boolean; startRule?: string; reportTimezone?: string; header?: ReactNode; - onChangeExpression: (name: string, expression: Expression) => void; + onChangeExpression: ( + name: string, + expression: Expression, + expressionClause: Lib.ExpressionClause, + ) => void; onRemoveExpression?: (name: string) => void; onClose?: () => void; } @@ -51,6 +60,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { stageIndex, name: initialName, expression: initialExpression, + expressionClause: initialExpressionClause, withName = false, startRule, reportTimezone, @@ -64,6 +74,8 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const [expression, setExpression] = useState( initialExpression || null, ); + const [expressionClause, setExpressionClause] = + useState(initialExpressionClause || null); const [error, setError] = useState(null); const helpTextTargetRef = useRef(null); @@ -73,15 +85,22 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const isValid = !error && isValidName && isValidExpression; - const handleCommit = (expression: Expression | null) => { - if (isValid && isNotNull(expression)) { - onChangeExpression(name, expression); - onClose && onClose(); + const handleCommit = ( + expression: Expression | null, + expressionClause: Lib.ExpressionClause | null, + ) => { + if (isValid && isNotNull(expression) && isNotNull(expressionClause)) { + onChangeExpression(name, expression, expressionClause); + onClose?.(); } }; - const handleExpressionChange = (parsedExpression: Expression | null) => { - setExpression(parsedExpression); + const handleExpressionChange = ( + expression: Expression | null, + expressionClause: Lib.ExpressionClause | null, + ) => { + setExpression(expression); + setExpressionClause(expressionClause); setError(null); }; @@ -134,7 +153,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { onChange={event => setName(event.target.value)} onKeyPress={e => { if (e.key === "Enter") { - handleCommit(expression); + handleCommit(expression, expressionClause); } }} /> @@ -147,7 +166,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index 5c681919f518f..f651a6a8adf46 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -164,8 +164,9 @@ function setup(additionalProps?: Partial) { onChangeExpression: jest.fn(), }; - const props = { + const props: ExpressionWidgetProps = { expression: undefined, + expressionClause: undefined, name: undefined, legacyQuery: createMockLegacyQueryForExpressions(), query: createQuery(), From 20e9406c75d9769571b2e1de03af0e93a085cfc4 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 23 Nov 2023 18:09:21 +0700 Subject: [PATCH 12/96] Use onSelect instead of onSelectLegacy in handleExpressionChange --- .../AggregationPicker/AggregationPicker.tsx | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index cb6f3e11cdcd9..2e25e4528bf53 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -202,12 +202,19 @@ export function AggregationPicker({ ); const handleExpressionChange = useCallback( - (name: string, expression: LegacyExpressionClause) => { - const aggregation = AGGREGATION.setName(expression, name); - onSelectLegacy(aggregation as LegacyAggregationClause); + ( + name: string, + _expression: LegacyExpressionClause, + expressionClause: Lib.ExpressionClause, + ) => { + const updatedExpressionClause = Lib.withExpressionName( + expressionClause, + name, + ); + onSelect(updatedExpressionClause); onClose?.(); }, - [onSelectLegacy, onClose], + [onSelect, onClose], ); if (isEditingExpression) { From 248009a61d49f6dd6dda993c2b2ef536d59d5907 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 23 Nov 2023 21:07:19 +0700 Subject: [PATCH 13/96] Remove onSelectLegacy from AggregationPicker --- .../AggregationPicker/AggregationPicker.tsx | 7 +------ .../AggregationPicker.unit.spec.tsx | 8 +++----- .../notebook/steps/AggregateStep/AggregateStep.tsx | 12 ------------ .../AddAggregationButton/AddAggregationButton.tsx | 6 ------ .../AggregationItem/AggregationItem.tsx | 11 ----------- .../sidebars/SummarizeSidebar/SummarizeSidebar.tsx | 10 ---------- 6 files changed, 4 insertions(+), 50 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 2e25e4528bf53..220ea7db297ef 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -9,10 +9,7 @@ import { useToggle } from "metabase/hooks/use-toggle"; import { ExpressionWidget } from "metabase/query_builder/components/expressions/ExpressionWidget"; import { ExpressionWidgetHeader } from "metabase/query_builder/components/expressions/ExpressionWidgetHeader"; -import type { - Aggregation as LegacyAggregationClause, - Expression as LegacyExpressionClause, -} from "metabase-types/api"; +import type { Expression as LegacyExpressionClause } from "metabase-types/api"; import * as Lib from "metabase-lib"; import * as AGGREGATION from "metabase-lib/queries/utils/aggregation"; import type LegacyAggregation from "metabase-lib/queries/structured/Aggregation"; @@ -40,7 +37,6 @@ interface AggregationPickerProps { legacyClause?: LegacyAggregation; maxHeight?: number; onSelect: (operator: Lib.Aggregatable) => void; - onSelectLegacy: (operator: LegacyAggregationClause) => void; onClose?: () => void; } @@ -75,7 +71,6 @@ export function AggregationPicker({ legacyClause, maxHeight = DEFAULT_MAX_HEIGHT, onSelect, - onSelectLegacy, onClose, }: AggregationPickerProps) { const [ diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index 210515bd6acea..4f582a15eb53c 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -141,7 +141,6 @@ function setup({ : baseOperators; const onSelect = jest.fn(); - const onSelectLegacy = jest.fn(); function handleSelect(clause: Lib.Aggregatable) { const nextQuery = Lib.aggregate(query, 0, clause); @@ -159,7 +158,6 @@ function setup({ operators={operators} hasExpressionInput={hasExpressionInput} onSelect={handleSelect} - onSelectLegacy={onSelectLegacy} />, ); @@ -168,7 +166,7 @@ function setup({ return lastCall?.[0]; } - return { metadata, getRecentClause, onSelectLegacy }; + return { metadata, getRecentClause, onSelect }; } describe("AggregationPicker", () => { @@ -392,7 +390,7 @@ describe("AggregationPicker", () => { describe("custom expressions", () => { it("should allow to enter a custom expression", async () => { - const { onSelectLegacy } = setup(); + const { onSelect } = setup(); userEvent.click(screen.getByText("Custom Expression")); userEvent.type(screen.getByLabelText("Expression"), "1 + 1"); @@ -400,7 +398,7 @@ describe("AggregationPicker", () => { userEvent.click(screen.getByRole("button", { name: "Done" })); await waitFor(() => - expect(onSelectLegacy).toHaveBeenCalledWith([ + expect(onSelect).toHaveBeenCalledWith([ "aggregation-options", ["+", 1, 1], { "display-name": "My expression", name: "My expression" }, diff --git a/frontend/src/metabase/query_builder/components/notebook/steps/AggregateStep/AggregateStep.tsx b/frontend/src/metabase/query_builder/components/notebook/steps/AggregateStep/AggregateStep.tsx index 78aa7dd11b66e..ba5245ac2f598 100644 --- a/frontend/src/metabase/query_builder/components/notebook/steps/AggregateStep/AggregateStep.tsx +++ b/frontend/src/metabase/query_builder/components/notebook/steps/AggregateStep/AggregateStep.tsx @@ -77,7 +77,6 @@ export function AggregateStep({ legacyQuery={legacyQuery} onAddAggregation={handleAddAggregation} onUpdateAggregation={handleUpdateAggregation} - onLegacyQueryChange={updateQuery} /> )} onRemove={handleRemoveAggregation} @@ -98,7 +97,6 @@ interface AggregationPopoverProps { legacyQuery: StructuredQuery; clauseIndex?: number; - onLegacyQueryChange: (query: StructuredQuery) => void; // Implicitly passed by metabase/components/Triggerable onClose?: () => void; @@ -112,7 +110,6 @@ function AggregationPopover({ legacyQuery, onAddAggregation, onUpdateAggregation, - onLegacyQueryChange, onClose, }: AggregationPopoverProps) { const isUpdate = clause != null && clauseIndex != null; @@ -142,15 +139,6 @@ function AggregationPopover({ onAddAggregation(aggregation); } }} - onSelectLegacy={newLegacyAggregation => { - if (isUpdate) { - onLegacyQueryChange( - legacyQuery.updateAggregation(clauseIndex, newLegacyAggregation), - ); - } else { - onLegacyQueryChange(legacyQuery.aggregate(newLegacyAggregation)); - } - }} onClose={onClose} /> ); diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx index 7389563ad4ee5..11c6a46566d98 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx @@ -12,14 +12,12 @@ interface AddAggregationButtonProps { query: Lib.Query; legacyQuery: StructuredQuery; onAddAggregation: (aggregation: Lib.Aggregatable) => void; - onLegacyQueryChange: (nextLegacyQuery: StructuredQuery) => void; } export function AddAggregationButton({ query, legacyQuery, onAddAggregation, - onLegacyQueryChange, }: AddAggregationButtonProps) { const hasAggregations = Lib.aggregations(query, STAGE_INDEX).length > 0; const operators = Lib.availableAggregationOperators(query, STAGE_INDEX); @@ -53,10 +51,6 @@ export function AddAggregationButton({ onAddAggregation(aggregation); closePopover(); }} - onSelectLegacy={legacyAggregation => { - onLegacyQueryChange(legacyQuery.aggregate(legacyAggregation)); - closePopover(); - }} /> )} /> diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AggregationItem/AggregationItem.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AggregationItem/AggregationItem.tsx index b51818f5bb59d..14dbdda97bf68 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AggregationItem/AggregationItem.tsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AggregationItem/AggregationItem.tsx @@ -13,7 +13,6 @@ interface AggregationItemProps { legacyQuery: StructuredQuery; onUpdate: (nextAggregation: Lib.Aggregatable) => void; onRemove: () => void; - onLegacyQueryChange: (nextLegacyQuery: StructuredQuery) => void; } export function AggregationItem({ @@ -23,7 +22,6 @@ export function AggregationItem({ legacyQuery, onUpdate, onRemove, - onLegacyQueryChange, }: AggregationItemProps) { const { displayName } = Lib.displayInfo(query, STAGE_INDEX, aggregation); @@ -58,15 +56,6 @@ export function AggregationItem({ onUpdate(nextAggregation); closePopover(); }} - onSelectLegacy={legacyAggregation => { - onLegacyQueryChange( - legacyQuery.updateAggregation( - aggregationIndex, - legacyAggregation, - ), - ); - closePopover(); - }} /> )} /> diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx index f0698ac1c4a62..61a661bc6beeb 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx @@ -51,14 +51,6 @@ export function SummarizeSidebar({ const aggregations = Lib.aggregations(query, STAGE_INDEX); const hasAggregations = aggregations.length > 0; - const handleLegacyQueryChange = useCallback( - (nextLegacyQuery: StructuredQuery) => { - const nextQuery = nextLegacyQuery.question()._getMLv2Query(); - onQueryChange(nextQuery); - }, - [onQueryChange], - ); - const handleAddAggregation = useCallback( (aggregation: Lib.Aggregatable) => { const nextQuery = Lib.aggregate(query, STAGE_INDEX, aggregation); @@ -155,14 +147,12 @@ export function SummarizeSidebar({ handleUpdateAggregation(aggregation, nextAggregation) } onRemove={() => handleRemoveAggregation(aggregation)} - onLegacyQueryChange={handleLegacyQueryChange} /> ))} {hasAggregations && ( From 7ae92bd20f5693e3f11470eb8b82dfb332cf7988 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 15:51:03 +0700 Subject: [PATCH 14/96] Use nullish coalescing operator --- .../query_builder/components/expressions/ExpressionWidget.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index 483373deaea6b..5f43d88582bc1 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -72,10 +72,10 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const [name, setName] = useState(initialName || ""); const [expression, setExpression] = useState( - initialExpression || null, + initialExpression ?? null, ); const [expressionClause, setExpressionClause] = - useState(initialExpressionClause || null); + useState(initialExpressionClause ?? null); const [error, setError] = useState(null); const helpTextTargetRef = useRef(null); From 864631752e0e72394f0b0a51ab8a0a8c2597591a Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 16:01:22 +0700 Subject: [PATCH 15/96] Assert new onChangeExpression argument in test --- .../ExpressionWidget.unit.spec.tsx | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index f651a6a8adf46..fe52249b3fc03 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -9,6 +9,7 @@ import { createSampleDatabase, ORDERS_ID, } from "metabase-types/api/mocks/presets"; +import * as Lib from "metabase-lib"; import { createQuery } from "metabase-lib/test-helpers"; import { ExpressionWidgetHeader } from "./ExpressionWidgetHeader"; import type { ExpressionWidgetProps } from "./ExpressionWidget"; @@ -55,7 +56,18 @@ describe("ExpressionWidget", () => { }); it("should trigger onChangeExpression if expression is valid", () => { - const { onChangeExpression } = setup(); + const onChangeExpression = jest.fn( + (_name, expression, expressionClause) => { + expect( + Lib.legacyExpressionForExpressionClause( + createQuery(), + 0, + expressionClause, + ), + ).toEqual(expression); + }, + ); + setup({ onChangeExpression }); const doneButton = screen.getByRole("button", { name: "Done" }); expect(doneButton).toBeDisabled(); @@ -71,7 +83,11 @@ describe("ExpressionWidget", () => { userEvent.click(doneButton); expect(onChangeExpression).toHaveBeenCalledTimes(1); - expect(onChangeExpression).toHaveBeenCalledWith("", ["+", 1, 1]); + expect(onChangeExpression).toHaveBeenCalledWith( + "", + ["+", 1, 1], + expect.anything(), // asserted inside onChangeExpression mock + ); }); it(`should render interactive header if it is passed`, () => { From 71ac3aaf86e9624488dd638df2156f77847453f0 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 16:02:26 +0700 Subject: [PATCH 16/96] Assert new onChangeExpression argument in test --- .../expressions/ExpressionWidget.unit.spec.tsx | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index fe52249b3fc03..d801120429921 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -115,7 +115,18 @@ describe("ExpressionWidget", () => { it("should validate name value", () => { const expression: Expression = ["+", 1, 1]; - const { onChangeExpression } = setup({ expression, withName: true }); + const onChangeExpression = jest.fn( + (_name, expression, expressionClause) => { + expect( + Lib.legacyExpressionForExpressionClause( + createQuery(), + 0, + expressionClause, + ), + ).toEqual(expression); + }, + ); + setup({ expression, withName: true, onChangeExpression }); const doneButton = screen.getByRole("button", { name: "Done" }); const expressionNameInput = screen.getByPlaceholderText( @@ -156,6 +167,7 @@ describe("ExpressionWidget", () => { expect(onChangeExpression).toHaveBeenCalledWith( "Some n_am!e 2q$w&YzT(6i~#sLXv7+HjP}Ku1|9c*RlF@4o5N=e8;G*-bZ3/U0:Qa'V,t(W-_D", expression, + expect.anything(), // asserted inside onChangeExpression mock ); }); }); From 30b9b2757bcff7718c7aa2be6a65816aff09c847 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 20:00:23 +0700 Subject: [PATCH 17/96] Update AggregationPicker tests --- .../AggregationPicker.unit.spec.tsx | 68 +++++++++---------- 1 file changed, 33 insertions(+), 35 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index 4f582a15eb53c..fd13820133f5c 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -1,7 +1,7 @@ import _ from "underscore"; import userEvent from "@testing-library/user-event"; import { createMockMetadata } from "__support__/metadata"; -import { render, screen, waitFor, within } from "__support__/ui"; +import { render, screen, within } from "__support__/ui"; import { checkNotNull } from "metabase/lib/types"; import type { Metric, StructuredDatasetQuery } from "metabase-types/api"; @@ -142,13 +142,6 @@ function setup({ const onSelect = jest.fn(); - function handleSelect(clause: Lib.Aggregatable) { - const nextQuery = Lib.aggregate(query, 0, clause); - const aggregations = Lib.aggregations(nextQuery, 0); - const recentAggregation = aggregations[aggregations.length - 1]; - onSelect(Lib.displayInfo(nextQuery, 0, recentAggregation)); - } - render( , ); function getRecentClause() { - const [lastCall] = onSelect.mock.calls.slice(-1); - return lastCall?.[0]; + expect(onSelect).toHaveBeenCalled(); + const lastCall = onSelect.mock.calls.at(-1); + return lastCall[0]; } - return { metadata, getRecentClause, onSelect }; + function getRecentClauseInfo() { + return Lib.displayInfo(query, 0, getRecentClause()); + } + + return { metadata, getRecentClause, getRecentClauseInfo, onSelect }; } describe("AggregationPicker", () => { it("should allow switching between aggregation approaches", () => { const metadata = createMetadata({ metrics: [TEST_METRIC] }); - const { getRecentClause } = setup({ + const { getRecentClauseInfo } = setup({ query: createQueryWithCountAggregation({ metadata }), metadata, }); @@ -181,7 +179,7 @@ describe("AggregationPicker", () => { userEvent.click(screen.getByText("Common Metrics")); userEvent.click(screen.getByText(TEST_METRIC.name)); - expect(getRecentClause()).toEqual( + expect(getRecentClauseInfo()).toEqual( expect.objectContaining({ displayName: metric.displayName(), }), @@ -224,11 +222,11 @@ describe("AggregationPicker", () => { }); it("should apply a column-less operator", () => { - const { getRecentClause } = setup(); + const { getRecentClauseInfo } = setup(); userEvent.click(screen.getByText("Count of rows")); - expect(getRecentClause()).toEqual( + expect(getRecentClauseInfo()).toEqual( expect.objectContaining({ name: "count", displayName: "Count", @@ -237,12 +235,12 @@ describe("AggregationPicker", () => { }); it("should apply an operator requiring columns", () => { - const { getRecentClause } = setup(); + const { getRecentClauseInfo } = setup(); userEvent.click(screen.getByText("Average of ...")); userEvent.click(screen.getByText("Quantity")); - expect(getRecentClause()).toEqual( + expect(getRecentClauseInfo()).toEqual( expect.objectContaining({ name: "avg", displayName: "Average of Quantity", @@ -251,13 +249,13 @@ describe("AggregationPicker", () => { }); it("should allow picking a foreign column", () => { - const { getRecentClause } = setup(); + const { getRecentClauseInfo } = setup(); userEvent.click(screen.getByText("Average of ...")); userEvent.click(screen.getByText("Product")); userEvent.click(screen.getByText("Rating")); - expect(getRecentClause()).toEqual( + expect(getRecentClauseInfo()).toEqual( expect.objectContaining({ name: "avg", displayName: "Average of Rating", @@ -300,7 +298,7 @@ describe("AggregationPicker", () => { }); it("should allow to change an operator for existing aggregation", () => { - const { getRecentClause } = setup({ + const { getRecentClauseInfo } = setup({ query: createQueryWithMaxAggregation(), }); @@ -308,7 +306,7 @@ describe("AggregationPicker", () => { userEvent.click(screen.getByText("Average of ...")); userEvent.click(screen.getByText("Quantity")); - expect(getRecentClause()).toEqual( + expect(getRecentClauseInfo()).toEqual( expect.objectContaining({ name: "avg", displayName: "Average of Quantity", @@ -317,13 +315,13 @@ describe("AggregationPicker", () => { }); it("should allow to change a column for existing aggregation", () => { - const { getRecentClause } = setup({ + const { getRecentClauseInfo } = setup({ query: createQueryWithMaxAggregation(), }); userEvent.click(screen.getByText("Discount")); - expect(getRecentClause()).toEqual( + expect(getRecentClauseInfo()).toEqual( expect.objectContaining({ name: "max", displayName: "Max of Discount", @@ -375,12 +373,12 @@ describe("AggregationPicker", () => { it("should allow picking a metric", () => { const metadata = createMetadata({ metrics: [TEST_METRIC] }); - const { getRecentClause } = setupMetrics({ metadata }); + const { getRecentClauseInfo } = setupMetrics({ metadata }); const metric = checkNotNull(metadata.metric(TEST_METRIC.id)); userEvent.click(screen.getByText(TEST_METRIC.name)); - expect(getRecentClause()).toEqual( + expect(getRecentClauseInfo()).toEqual( expect.objectContaining({ displayName: metric.displayName(), }), @@ -390,20 +388,20 @@ describe("AggregationPicker", () => { describe("custom expressions", () => { it("should allow to enter a custom expression", async () => { - const { onSelect } = setup(); + const { getRecentClause, getRecentClauseInfo } = setup(); + + const expression = "1 + 1"; + const expressionName = "My expression"; userEvent.click(screen.getByText("Custom Expression")); - userEvent.type(screen.getByLabelText("Expression"), "1 + 1"); - userEvent.type(screen.getByLabelText("Name"), "My expression"); + userEvent.type(screen.getByLabelText("Expression"), expression); + userEvent.type(screen.getByLabelText("Name"), expressionName); userEvent.click(screen.getByRole("button", { name: "Done" })); - await waitFor(() => - expect(onSelect).toHaveBeenCalledWith([ - "aggregation-options", - ["+", 1, 1], - { "display-name": "My expression", name: "My expression" }, - ]), + expect(getRecentClauseInfo()).toEqual( + expect.objectContaining({ displayName: expression }), ); + expect(Lib.expressionName(getRecentClause())).toBe(expressionName); }); it("should open the editor when an expression is used", async () => { From 2f50b3fd153aaa93746eeea3812cb7f94acfb296 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 20:05:14 +0700 Subject: [PATCH 18/96] Replace toEqual + expect.objectContaining with toMatchObject --- .../AggregationPicker.unit.spec.tsx | 70 +++++++------------ 1 file changed, 27 insertions(+), 43 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index fd13820133f5c..dc252a1c34868 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -179,11 +179,9 @@ describe("AggregationPicker", () => { userEvent.click(screen.getByText("Common Metrics")); userEvent.click(screen.getByText(TEST_METRIC.name)); - expect(getRecentClauseInfo()).toEqual( - expect.objectContaining({ - displayName: metric.displayName(), - }), - ); + expect(getRecentClauseInfo()).toMatchObject({ + displayName: metric.displayName(), + }); }); describe("basic operators", () => { @@ -226,12 +224,10 @@ describe("AggregationPicker", () => { userEvent.click(screen.getByText("Count of rows")); - expect(getRecentClauseInfo()).toEqual( - expect.objectContaining({ - name: "count", - displayName: "Count", - }), - ); + expect(getRecentClauseInfo()).toMatchObject({ + name: "count", + displayName: "Count", + }); }); it("should apply an operator requiring columns", () => { @@ -240,12 +236,10 @@ describe("AggregationPicker", () => { userEvent.click(screen.getByText("Average of ...")); userEvent.click(screen.getByText("Quantity")); - expect(getRecentClauseInfo()).toEqual( - expect.objectContaining({ - name: "avg", - displayName: "Average of Quantity", - }), - ); + expect(getRecentClauseInfo()).toMatchObject({ + name: "avg", + displayName: "Average of Quantity", + }); }); it("should allow picking a foreign column", () => { @@ -255,12 +249,10 @@ describe("AggregationPicker", () => { userEvent.click(screen.getByText("Product")); userEvent.click(screen.getByText("Rating")); - expect(getRecentClauseInfo()).toEqual( - expect.objectContaining({ - name: "avg", - displayName: "Average of Rating", - }), - ); + expect(getRecentClauseInfo()).toMatchObject({ + name: "avg", + displayName: "Average of Rating", + }); }); it("should highlight selected operator", () => { @@ -306,12 +298,10 @@ describe("AggregationPicker", () => { userEvent.click(screen.getByText("Average of ...")); userEvent.click(screen.getByText("Quantity")); - expect(getRecentClauseInfo()).toEqual( - expect.objectContaining({ - name: "avg", - displayName: "Average of Quantity", - }), - ); + expect(getRecentClauseInfo()).toMatchObject({ + name: "avg", + displayName: "Average of Quantity", + }); }); it("should allow to change a column for existing aggregation", () => { @@ -321,12 +311,10 @@ describe("AggregationPicker", () => { userEvent.click(screen.getByText("Discount")); - expect(getRecentClauseInfo()).toEqual( - expect.objectContaining({ - name: "max", - displayName: "Max of Discount", - }), - ); + expect(getRecentClauseInfo()).toMatchObject({ + name: "max", + displayName: "Max of Discount", + }); }); }); @@ -378,11 +366,9 @@ describe("AggregationPicker", () => { userEvent.click(screen.getByText(TEST_METRIC.name)); - expect(getRecentClauseInfo()).toEqual( - expect.objectContaining({ - displayName: metric.displayName(), - }), - ); + expect(getRecentClauseInfo()).toMatchObject({ + displayName: metric.displayName(), + }); }); }); @@ -398,9 +384,7 @@ describe("AggregationPicker", () => { userEvent.type(screen.getByLabelText("Name"), expressionName); userEvent.click(screen.getByRole("button", { name: "Done" })); - expect(getRecentClauseInfo()).toEqual( - expect.objectContaining({ displayName: expression }), - ); + expect(getRecentClauseInfo()).toMatchObject({ displayName: expression }); expect(Lib.expressionName(getRecentClause())).toBe(expressionName); }); From 24a131c5122bab162812ec8bb7c96564d508f8fb Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 20:14:11 +0700 Subject: [PATCH 19/96] Add stageIndex to setup, use destructuring for onSelect arguments --- .../AggregationPicker.unit.spec.tsx | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index dc252a1c34868..c7dcedbf0a0f3 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -129,13 +129,14 @@ function setup({ query = createQuery({ metadata }), hasExpressionInput = true, }: SetupOpts = {}) { + const stageIndex = 0; const dataset_query = Lib.toLegacyQuery(query) as StructuredDatasetQuery; const question = new Question(createAdHocCard({ dataset_query }), metadata); const legacyQuery = question.query() as StructuredQuery; - const clause = Lib.aggregations(query, 0)[0]; + const clause = Lib.aggregations(query, stageIndex)[0]; - const baseOperators = Lib.availableAggregationOperators(query, 0); + const baseOperators = Lib.availableAggregationOperators(query, stageIndex); const operators = clause ? Lib.selectedAggregationOperators(baseOperators, clause) : baseOperators; @@ -147,7 +148,7 @@ function setup({ query={query} legacyQuery={legacyQuery} legacyClause={legacyQuery.aggregations()[0]} - stageIndex={0} + stageIndex={stageIndex} operators={operators} hasExpressionInput={hasExpressionInput} onSelect={onSelect} @@ -156,12 +157,12 @@ function setup({ function getRecentClause() { expect(onSelect).toHaveBeenCalled(); - const lastCall = onSelect.mock.calls.at(-1); - return lastCall[0]; + const [operator] = onSelect.mock.calls.at(-1); + return operator; } function getRecentClauseInfo() { - return Lib.displayInfo(query, 0, getRecentClause()); + return Lib.displayInfo(query, stageIndex, getRecentClause()); } return { metadata, getRecentClause, getRecentClauseInfo, onSelect }; From 5c75c838fab493657b6f516d0eff12c6b946d8c5 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 20:15:38 +0700 Subject: [PATCH 20/96] Deal with awkward assertions --- .../ExpressionWidget.unit.spec.tsx | 63 ++++++++++--------- 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index d801120429921..5b8ac9b27f67c 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -56,18 +56,7 @@ describe("ExpressionWidget", () => { }); it("should trigger onChangeExpression if expression is valid", () => { - const onChangeExpression = jest.fn( - (_name, expression, expressionClause) => { - expect( - Lib.legacyExpressionForExpressionClause( - createQuery(), - 0, - expressionClause, - ), - ).toEqual(expression); - }, - ); - setup({ onChangeExpression }); + const { onChangeExpression, getRecentExpressionClauseInfo } = setup(); const doneButton = screen.getByRole("button", { name: "Done" }); expect(doneButton).toBeDisabled(); @@ -86,8 +75,11 @@ describe("ExpressionWidget", () => { expect(onChangeExpression).toHaveBeenCalledWith( "", ["+", 1, 1], - expect.anything(), // asserted inside onChangeExpression mock + expect.anything(), ); + expect(getRecentExpressionClauseInfo()).toMatchObject({ + displayName: "1 + 1", + }); }); it(`should render interactive header if it is passed`, () => { @@ -115,18 +107,10 @@ describe("ExpressionWidget", () => { it("should validate name value", () => { const expression: Expression = ["+", 1, 1]; - const onChangeExpression = jest.fn( - (_name, expression, expressionClause) => { - expect( - Lib.legacyExpressionForExpressionClause( - createQuery(), - 0, - expressionClause, - ), - ).toEqual(expression); - }, - ); - setup({ expression, withName: true, onChangeExpression }); + const { onChangeExpression, getRecentExpressionClauseInfo } = setup({ + expression, + withName: true, + }); const doneButton = screen.getByRole("button", { name: "Done" }); const expressionNameInput = screen.getByPlaceholderText( @@ -167,8 +151,11 @@ describe("ExpressionWidget", () => { expect(onChangeExpression).toHaveBeenCalledWith( "Some n_am!e 2q$w&YzT(6i~#sLXv7+HjP}Ku1|9c*RlF@4o5N=e8;G*-bZ3/U0:Qa'V,t(W-_D", expression, - expect.anything(), // asserted inside onChangeExpression mock + expect.anything(), ); + expect(getRecentExpressionClauseInfo()).toMatchObject({ + displayName: "1 + 1", + }); }); }); }); @@ -192,19 +179,37 @@ function setup(additionalProps?: Partial) { onChangeExpression: jest.fn(), }; + const query = createQuery(); + const stageIndex = 0; + const props: ExpressionWidgetProps = { expression: undefined, expressionClause: undefined, name: undefined, legacyQuery: createMockLegacyQueryForExpressions(), - query: createQuery(), - stageIndex: 0, + query, + stageIndex, reportTimezone: "UTC", ...mocks, ...additionalProps, }; + function getRecentExpressionClause() { + expect(mocks.onChangeExpression).toHaveBeenCalled(); + const [_name, _expression, expressionClause] = + mocks.onChangeExpression.mock.calls.at(-1); + return expressionClause; + } + + function getRecentExpressionClauseInfo() { + return Lib.displayInfo(query, stageIndex, getRecentExpressionClause()); + } + render(); - return mocks; + return { + ...mocks, + getRecentExpressionClause, + getRecentExpressionClauseInfo, + }; } From d64c7c4726f0cde7ec4942317ee036f9f0901ba9 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 20:19:17 +0700 Subject: [PATCH 21/96] Use props in a conservative way --- .../ExpressionWidget.unit.spec.tsx | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index 5b8ac9b27f67c..bc4591ae10b0d 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -56,7 +56,7 @@ describe("ExpressionWidget", () => { }); it("should trigger onChangeExpression if expression is valid", () => { - const { onChangeExpression, getRecentExpressionClauseInfo } = setup(); + const { getRecentExpressionClauseInfo, onChangeExpression } = setup(); const doneButton = screen.getByRole("button", { name: "Done" }); expect(doneButton).toBeDisabled(); @@ -107,7 +107,7 @@ describe("ExpressionWidget", () => { it("should validate name value", () => { const expression: Expression = ["+", 1, 1]; - const { onChangeExpression, getRecentExpressionClauseInfo } = setup({ + const { getRecentExpressionClauseInfo, onChangeExpression } = setup({ expression, withName: true, }); @@ -174,30 +174,15 @@ const createMockLegacyQueryForExpressions = () => { }; function setup(additionalProps?: Partial) { - const mocks = { - onClose: jest.fn(), - onChangeExpression: jest.fn(), - }; - const query = createQuery(); const stageIndex = 0; - - const props: ExpressionWidgetProps = { - expression: undefined, - expressionClause: undefined, - name: undefined, - legacyQuery: createMockLegacyQueryForExpressions(), - query, - stageIndex, - reportTimezone: "UTC", - ...mocks, - ...additionalProps, - }; + const onChangeExpression = jest.fn(); + const onClose = jest.fn(); function getRecentExpressionClause() { - expect(mocks.onChangeExpression).toHaveBeenCalled(); + expect(onChangeExpression).toHaveBeenCalled(); const [_name, _expression, expressionClause] = - mocks.onChangeExpression.mock.calls.at(-1); + onChangeExpression.mock.calls.at(-1); return expressionClause; } @@ -205,11 +190,24 @@ function setup(additionalProps?: Partial) { return Lib.displayInfo(query, stageIndex, getRecentExpressionClause()); } - render(); + render( + , + ); return { - ...mocks, - getRecentExpressionClause, getRecentExpressionClauseInfo, + onChangeExpression, + onClose, }; } From 89041b9e6a500a9fd89f18c637923f80eed21104 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 20:23:38 +0700 Subject: [PATCH 22/96] Replace legacyQuery.database() with query + metadata --- .../AggregationPicker/AggregationPicker.tsx | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 220ea7db297ef..7d9174bef837a 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -5,6 +5,8 @@ import AccordionList from "metabase/core/components/AccordionList"; import { Icon } from "metabase/core/components/Icon"; import { useToggle } from "metabase/hooks/use-toggle"; +import { useSelector } from "metabase/lib/redux"; +import { getMetadata } from "metabase/selectors/metadata"; import { ExpressionWidget } from "metabase/query_builder/components/expressions/ExpressionWidget"; import { ExpressionWidgetHeader } from "metabase/query_builder/components/expressions/ExpressionWidgetHeader"; @@ -73,6 +75,7 @@ export function AggregationPicker({ onSelect, onClose, }: AggregationPickerProps) { + const metadata = useSelector(getMetadata); const [ isEditingExpression, { turnOn: openExpressionEditor, turnOff: closeExpressionEditor }, @@ -96,9 +99,9 @@ export function AggregationPicker({ const sections: Section[] = []; const metrics = Lib.availableMetrics(query); - const canUseExpressions = legacyQuery - .database() - ?.hasFeature("expression-aggregations"); + const databaseId = Lib.databaseID(query); + const database = metadata.database(databaseId); + const canUseExpressions = database?.hasFeature("expression-aggregations"); if (operators.length > 0) { sections.push({ @@ -132,7 +135,7 @@ export function AggregationPicker({ } return sections; - }, [query, legacyQuery, stageIndex, operators, hasExpressionInput]); + }, [metadata, query, stageIndex, operators, hasExpressionInput]); const checkIsItemSelected = useCallback( (item: ListItem) => item.selected, From 39496c106b3e6b42324a83753c27f0b19af09b38 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 20:28:44 +0700 Subject: [PATCH 23/96] Add a TODO comment --- .../common/components/AggregationPicker/AggregationPicker.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 7d9174bef837a..a8f7bfbf59bb6 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -323,6 +323,9 @@ function getInitialOperator( } function isExpressionEditorInitiallyOpen(legacyClause?: LegacyAggregation) { + // TODO: we need to add more information to AggregationOperatorDisplayInfo + // to be able to migrate legacyClause to MLv2 Lib.Aggregatable. + // This requires changes in Clojure code. return ( legacyClause && (AGGREGATION.isCustom(legacyClause) || AGGREGATION.isNamed(legacyClause)) From 221e950aca00bb22bc21afd9000605c4d95a99a3 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 21:12:18 +0700 Subject: [PATCH 24/96] Add clause prop to AggregationPicker --- .../common/components/AggregationPicker/AggregationPicker.tsx | 1 + .../components/AggregationPicker/AggregationPicker.unit.spec.tsx | 1 + .../components/notebook/steps/AggregateStep/AggregateStep.tsx | 1 + .../SummarizeSidebar/AggregationItem/AggregationItem.tsx | 1 + 4 files changed, 4 insertions(+) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index a8f7bfbf59bb6..cf57ba0b52ef7 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -32,6 +32,7 @@ const DEFAULT_MAX_HEIGHT = 610; interface AggregationPickerProps { className?: string; query: Lib.Query; + clause?: Lib.AggregationClause; stageIndex: number; operators: Lib.AggregationOperator[]; hasExpressionInput?: boolean; diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index c7dcedbf0a0f3..5c7ed56d311c3 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -146,6 +146,7 @@ function setup({ render( ( Date: Fri, 24 Nov 2023 21:20:53 +0700 Subject: [PATCH 25/96] Migrate clause name --- .../common/components/AggregationPicker/AggregationPicker.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index cf57ba0b52ef7..51289f0ca11b2 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -67,6 +67,7 @@ function isOperatorListItem(item: ListItem): item is OperatorListItem { export function AggregationPicker({ className, query, + clause, stageIndex, operators, hasExpressionInput = true, @@ -222,7 +223,7 @@ export function AggregationPicker({ legacyQuery={legacyQuery} query={query} stageIndex={stageIndex} - name={AGGREGATION.getName(legacyClause)} + name={clause ? Lib.displayName(query, clause) : clause} expression={AGGREGATION.getContent(legacyClause)} withName startRule="aggregation" From a9d7b5bba809a0d6bd303b2a981e05b866217b9c Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 24 Nov 2023 21:42:55 +0700 Subject: [PATCH 26/96] Replace 3rd argument in onChangeExpression with onChangeExpressionClause --- .../AggregationPicker/AggregationPicker.tsx | 11 ++---- .../expressions/ExpressionWidget.tsx | 39 ++++++++++++------- .../ExpressionWidget.unit.spec.tsx | 31 ++++++++++----- 3 files changed, 50 insertions(+), 31 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 51289f0ca11b2..d6ad9763dadc5 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -11,7 +11,6 @@ import { getMetadata } from "metabase/selectors/metadata"; import { ExpressionWidget } from "metabase/query_builder/components/expressions/ExpressionWidget"; import { ExpressionWidgetHeader } from "metabase/query_builder/components/expressions/ExpressionWidgetHeader"; -import type { Expression as LegacyExpressionClause } from "metabase-types/api"; import * as Lib from "metabase-lib"; import * as AGGREGATION from "metabase-lib/queries/utils/aggregation"; import type LegacyAggregation from "metabase-lib/queries/structured/Aggregation"; @@ -201,12 +200,8 @@ export function AggregationPicker({ [openExpressionEditor], ); - const handleExpressionChange = useCallback( - ( - name: string, - _expression: LegacyExpressionClause, - expressionClause: Lib.ExpressionClause, - ) => { + const handleExpressionClauseChange = useCallback( + (name: string, expressionClause: Lib.ExpressionClause) => { const updatedExpressionClause = Lib.withExpressionName( expressionClause, name, @@ -228,7 +223,7 @@ export function AggregationPicker({ withName startRule="aggregation" header={} - onChangeExpression={handleExpressionChange} + onChangeExpressionClause={handleExpressionClauseChange} onClose={closeExpressionEditor} /> ); diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index 5f43d88582bc1..37f51caa31fc2 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -32,7 +32,10 @@ export interface ExpressionWidgetProps { legacyQuery: StructuredQuery; query?: Lib.Query; stageIndex?: number; - expression: Expression | undefined; + /** + * expression should not be present in components migrated to MLv2 + */ + expression?: Expression | undefined; /** * Presence of this prop is not enforced due to backwards-compatibility * with ExpressionWidget usages outside of GUI editor. @@ -44,9 +47,9 @@ export interface ExpressionWidgetProps { reportTimezone?: string; header?: ReactNode; - onChangeExpression: ( + onChangeExpression?: (name: string, expression: Expression) => void; + onChangeExpressionClause?: ( name: string, - expression: Expression, expressionClause: Lib.ExpressionClause, ) => void; onRemoveExpression?: (name: string) => void; @@ -66,6 +69,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { reportTimezone, header, onChangeExpression, + onChangeExpressionClause, onRemoveExpression, onClose, } = props; @@ -81,16 +85,25 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const helpTextTargetRef = useRef(null); const isValidName = withName ? name.trim().length > 0 : true; - const isValidExpression = !!expression && isExpression(expression); + const isValid = !error && isValidName; - const isValid = !error && isValidName && isValidExpression; + const handleCommit = () => { + if (!isValid) { + return; + } - const handleCommit = ( - expression: Expression | null, - expressionClause: Lib.ExpressionClause | null, - ) => { - if (isValid && isNotNull(expression) && isNotNull(expressionClause)) { - onChangeExpression(name, expression, expressionClause); + const isValidExpression = isNotNull(expression) && isExpression(expression); + const isValidExpressionClause = isNotNull(expressionClause); + + if (isValidExpression) { + onChangeExpression?.(name, expression); + } + + if (isValidExpressionClause) { + onChangeExpressionClause?.(name, expressionClause); + } + + if (isValidExpression || isValidExpressionClause) { onClose?.(); } }; @@ -153,7 +166,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { onChange={event => setName(event.target.value)} onKeyPress={e => { if (e.key === "Enter") { - handleCommit(expression, expressionClause); + handleCommit(); } }} /> @@ -166,7 +179,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index bc4591ae10b0d..0c6aed4729aa6 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -55,8 +55,8 @@ describe("ExpressionWidget", () => { ).toBeInTheDocument(); }); - it("should trigger onChangeExpression if expression is valid", () => { - const { getRecentExpressionClauseInfo, onChangeExpression } = setup(); + it("should trigger onChangeExpressionClause if expression is valid", () => { + const { getRecentExpressionClauseInfo, onChangeExpressionClause } = setup(); const doneButton = screen.getByRole("button", { name: "Done" }); expect(doneButton).toBeDisabled(); @@ -71,8 +71,8 @@ describe("ExpressionWidget", () => { userEvent.click(doneButton); - expect(onChangeExpression).toHaveBeenCalledTimes(1); - expect(onChangeExpression).toHaveBeenCalledWith( + expect(onChangeExpressionClause).toHaveBeenCalledTimes(1); + expect(onChangeExpressionClause).toHaveBeenCalledWith( "", ["+", 1, 1], expect.anything(), @@ -107,7 +107,11 @@ describe("ExpressionWidget", () => { it("should validate name value", () => { const expression: Expression = ["+", 1, 1]; - const { getRecentExpressionClauseInfo, onChangeExpression } = setup({ + const { + getRecentExpressionClauseInfo, + onChangeExpression, + onChangeExpressionClause, + } = setup({ expression, withName: true, }); @@ -121,8 +125,8 @@ describe("ExpressionWidget", () => { userEvent.type(screen.getByDisplayValue("1 + 1"), "{enter}"); - // enter in expression editor should not trigger "onChangeExpression" as popover is not valid with empty "name" - expect(onChangeExpression).toHaveBeenCalledTimes(0); + // enter in expression editor should not trigger "onChangeExpressionClause" as popover is not valid with empty "name" + expect(onChangeExpressionClause).toHaveBeenCalledTimes(0); // The name must not be empty userEvent.type(expressionNameInput, ""); @@ -151,6 +155,10 @@ describe("ExpressionWidget", () => { expect(onChangeExpression).toHaveBeenCalledWith( "Some n_am!e 2q$w&YzT(6i~#sLXv7+HjP}Ku1|9c*RlF@4o5N=e8;G*-bZ3/U0:Qa'V,t(W-_D", expression, + ); + expect(onChangeExpressionClause).toHaveBeenCalledTimes(1); + expect(onChangeExpressionClause).toHaveBeenCalledWith( + "Some n_am!e 2q$w&YzT(6i~#sLXv7+HjP}Ku1|9c*RlF@4o5N=e8;G*-bZ3/U0:Qa'V,t(W-_D", expect.anything(), ); expect(getRecentExpressionClauseInfo()).toMatchObject({ @@ -177,12 +185,13 @@ function setup(additionalProps?: Partial) { const query = createQuery(); const stageIndex = 0; const onChangeExpression = jest.fn(); + const onChangeExpressionClause = jest.fn(); const onClose = jest.fn(); function getRecentExpressionClause() { - expect(onChangeExpression).toHaveBeenCalled(); - const [_name, _expression, expressionClause] = - onChangeExpression.mock.calls.at(-1); + expect(onChangeExpressionClause).toHaveBeenCalled(); + const [_name, expressionClause] = + onChangeExpressionClause.mock.calls.at(-1); return expressionClause; } @@ -200,6 +209,7 @@ function setup(additionalProps?: Partial) { reportTimezone="UTC" stageIndex={stageIndex} onChangeExpression={onChangeExpression} + onChangeExpressionClause={onChangeExpressionClause} onClose={onClose} {...additionalProps} />, @@ -208,6 +218,7 @@ function setup(additionalProps?: Partial) { return { getRecentExpressionClauseInfo, onChangeExpression, + onChangeExpressionClause, onClose, }; } From 079abb52a2bc40fad51e8164433b904811b20512 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 27 Nov 2023 16:05:48 +0700 Subject: [PATCH 27/96] Use destructuring --- .../ExpressionEditorTextfield.tsx | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx index 764f9a2cffda3..106da1fd2d56d 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx @@ -504,8 +504,16 @@ class ExpressionEditorTextfield extends React.Component< ]; render() { - const { source, suggestions, errorMessage, hasChanges, isFocused } = - this.state; + const { helpTextTarget, width } = this.props; + const { + source, + suggestions, + errorMessage, + hasChanges, + isFocused, + highlightedSuggestionIndex, + helpText, + } = this.state; return ( @@ -537,16 +545,16 @@ class ExpressionEditorTextfield extends React.Component< target={this.suggestionTarget.current} suggestions={suggestions} onSuggestionMouseDown={this.onSuggestionSelected} - highlightedIndex={this.state.highlightedSuggestionIndex} + highlightedIndex={highlightedSuggestionIndex} /> {errorMessage && hasChanges && ( {errorMessage.message} )} ); From 7658ec93d3136b0f7cb47948b3d702fa19aebb89 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 27 Nov 2023 18:38:34 +0700 Subject: [PATCH 28/96] Allow AggregationClause, drop expression from ExpressionEditorTextfield --- frontend/src/metabase-lib/expression.ts | 3 +- .../AggregationPicker/AggregationPicker.tsx | 2 +- .../ExpressionEditorTextfield.tsx | 36 +++++++++++++------ .../expressions/ExpressionWidget.tsx | 11 +++--- 4 files changed, 35 insertions(+), 17 deletions(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index ac30a10752a5f..ec54d28c13dd3 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -1,5 +1,6 @@ import * as ML from "cljs/metabase.lib.js"; import type { + AggregationClause, ColumnMetadata, ExpressionArg, ExpressionClause, @@ -73,7 +74,7 @@ export function expressionClauseForLegacyExpression( export function legacyExpressionForExpressionClause( query: Query, stageIndex: number, - expressionClause: ExpressionClause, + expressionClause: ExpressionClause | AggregationClause, ): any { return ML.legacy_expression_for_expression_clause( query, diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index d6ad9763dadc5..6ebb5105305a6 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -219,7 +219,7 @@ export function AggregationPicker({ query={query} stageIndex={stageIndex} name={clause ? Lib.displayName(query, clause) : clause} - expression={AGGREGATION.getContent(legacyClause)} + expressionClause={clause} withName startRule="aggregation" header={} diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx index 106da1fd2d56d..5821a97fbe18d 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx @@ -8,7 +8,7 @@ import * as ace from "ace-builds/src-noconflict/ace"; import type { Ace } from "ace-builds"; import type { Expression } from "metabase-types/api"; import ExplicitSize from "metabase/components/ExplicitSize"; -import type * as Lib from "metabase-lib"; +import * as Lib from "metabase-lib"; import { format } from "metabase-lib/expressions/format"; import { processSource } from "metabase-lib/expressions/process"; import { diagnose } from "metabase-lib/expressions/diagnostics"; @@ -45,11 +45,11 @@ const ACE_OPTIONS = { }; interface ExpressionEditorTextfieldProps { - expression: Expression | undefined; + clause: Lib.AggregationClause | Lib.ExpressionClause | undefined; name: string; legacyQuery: StructuredQuery; - query?: Lib.Query; - stageIndex?: number; + query: Lib.Query; + stageIndex: number; startRule?: string; width?: number; reportTimezone?: string; @@ -70,7 +70,6 @@ interface ExpressionEditorTextfieldProps { interface ExpressionEditorTextfieldState { source: string; - expression: Expression; suggestions: Suggestion[]; highlightedSuggestionIndex: number; isFocused: boolean; @@ -83,15 +82,20 @@ function transformPropsToState( props: ExpressionEditorTextfieldProps, ): ExpressionEditorTextfieldState { const { - expression = ExpressionEditorTextfield.defaultProps.expression, legacyQuery, startRule = ExpressionEditorTextfield.defaultProps.startRule, + clause, + query, + stageIndex, } = props; + + const expression = clause + ? Lib.legacyExpressionForExpressionClause(query, stageIndex, clause)[1] // TODO: remove [1], see https://github.com/metabase/metabase/issues/36120 + : undefined; const source = format(expression, { legacyQuery, startRule }); return { source, - expression, highlightedSuggestionIndex: 0, helpText: null, suggestions: [], @@ -129,9 +133,21 @@ class ExpressionEditorTextfield extends React.Component< newProps: Readonly, ) { // we only refresh our state if we had no previous state OR if our expression changed - const { expression, legacyQuery, startRule } = newProps; - if (!this.state || !_.isEqual(this.props.expression, expression)) { - const source = format(expression, { legacyQuery, startRule }); + const { legacyQuery, startRule, query, stageIndex, clause } = newProps; + const previousExpression = this.props.clause + ? Lib.legacyExpressionForExpressionClause( + query, + stageIndex, + this.props.clause, + )[1] // TODO: remove [1], see https://github.com/metabase/metabase/issues/36120 + : undefined; + const newExpression = clause + ? Lib.legacyExpressionForExpressionClause(query, stageIndex, clause)[1] // TODO: remove [1], see https://github.com/metabase/metabase/issues/36120 + : undefined; + const hasExpressionChanged = !_.isEqual(previousExpression, newExpression); + + if (!this.state || hasExpressionChanged) { + const source = format(newExpression, { legacyQuery, startRule }); const currentSource = this.state.source; this.setState(transformPropsToState(newProps)); diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index 37f51caa31fc2..da93947b3efa1 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -40,7 +40,7 @@ export interface ExpressionWidgetProps { * Presence of this prop is not enforced due to backwards-compatibility * with ExpressionWidget usages outside of GUI editor. */ - expressionClause?: Lib.ExpressionClause | undefined; + expressionClause?: Lib.AggregationClause | Lib.ExpressionClause | undefined; name?: string; withName?: boolean; startRule?: string; @@ -50,7 +50,7 @@ export interface ExpressionWidgetProps { onChangeExpression?: (name: string, expression: Expression) => void; onChangeExpressionClause?: ( name: string, - expressionClause: Lib.ExpressionClause, + expressionClause: Lib.AggregationClause | Lib.ExpressionClause, ) => void; onRemoveExpression?: (name: string) => void; onClose?: () => void; @@ -78,8 +78,9 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const [expression, setExpression] = useState( initialExpression ?? null, ); - const [expressionClause, setExpressionClause] = - useState(initialExpressionClause ?? null); + const [expressionClause, setExpressionClause] = useState< + Lib.AggregationClause | Lib.ExpressionClause | null + >(initialExpressionClause ?? null); const [error, setError] = useState(null); const helpTextTargetRef = useRef(null); @@ -140,7 +141,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => {
Date: Mon, 27 Nov 2023 18:58:03 +0700 Subject: [PATCH 29/96] Make withExpressionName work with AggregationClause --- frontend/src/metabase-lib/expression.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index ec54d28c13dd3..51cc8e5d00bb9 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -25,10 +25,9 @@ export function expressionName(clause: ExpressionClause): string { return ML.expression_name(clause); } -export function withExpressionName( - clause: ExpressionClause, - newName: string, -): ExpressionClause { +export function withExpressionName< + Clause extends AggregationClause | ExpressionClause, +>(clause: Clause, newName: string): Clause { return ML.with_expression_name(clause, newName); } From 94bc358a4736861a36929077aef820412152099f Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 27 Nov 2023 18:58:28 +0700 Subject: [PATCH 30/96] Rename expressionClause to clause --- .../AggregationPicker/AggregationPicker.tsx | 15 +++++------- .../expressions/ExpressionWidget.tsx | 24 +++++++++---------- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 6ebb5105305a6..030ed06f53df8 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -200,13 +200,10 @@ export function AggregationPicker({ [openExpressionEditor], ); - const handleExpressionClauseChange = useCallback( - (name: string, expressionClause: Lib.ExpressionClause) => { - const updatedExpressionClause = Lib.withExpressionName( - expressionClause, - name, - ); - onSelect(updatedExpressionClause); + const handleClauseChange = useCallback( + (name: string, clause: Lib.AggregationClause | Lib.ExpressionClause) => { + const updatedClause = Lib.withExpressionName(clause, name); + onSelect(updatedClause); onClose?.(); }, [onSelect, onClose], @@ -219,11 +216,11 @@ export function AggregationPicker({ query={query} stageIndex={stageIndex} name={clause ? Lib.displayName(query, clause) : clause} - expressionClause={clause} + clause={clause} withName startRule="aggregation" header={} - onChangeExpressionClause={handleExpressionClauseChange} + onChangeClause={handleClauseChange} onClose={closeExpressionEditor} /> ); diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index da93947b3efa1..d19aefa42c394 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -40,7 +40,7 @@ export interface ExpressionWidgetProps { * Presence of this prop is not enforced due to backwards-compatibility * with ExpressionWidget usages outside of GUI editor. */ - expressionClause?: Lib.AggregationClause | Lib.ExpressionClause | undefined; + clause?: Lib.AggregationClause | Lib.ExpressionClause | undefined; name?: string; withName?: boolean; startRule?: string; @@ -48,9 +48,9 @@ export interface ExpressionWidgetProps { header?: ReactNode; onChangeExpression?: (name: string, expression: Expression) => void; - onChangeExpressionClause?: ( + onChangeClause?: ( name: string, - expressionClause: Lib.AggregationClause | Lib.ExpressionClause, + clause: Lib.AggregationClause | Lib.ExpressionClause, ) => void; onRemoveExpression?: (name: string) => void; onClose?: () => void; @@ -63,13 +63,13 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { stageIndex, name: initialName, expression: initialExpression, - expressionClause: initialExpressionClause, + clause: initialClause, withName = false, startRule, reportTimezone, header, onChangeExpression, - onChangeExpressionClause, + onChangeClause, onRemoveExpression, onClose, } = props; @@ -78,9 +78,9 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const [expression, setExpression] = useState( initialExpression ?? null, ); - const [expressionClause, setExpressionClause] = useState< + const [clause, setClause] = useState< Lib.AggregationClause | Lib.ExpressionClause | null - >(initialExpressionClause ?? null); + >(initialClause ?? null); const [error, setError] = useState(null); const helpTextTargetRef = useRef(null); @@ -94,14 +94,14 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { } const isValidExpression = isNotNull(expression) && isExpression(expression); - const isValidExpressionClause = isNotNull(expressionClause); + const isValidExpressionClause = isNotNull(clause); if (isValidExpression) { onChangeExpression?.(name, expression); } if (isValidExpressionClause) { - onChangeExpressionClause?.(name, expressionClause); + onChangeClause?.(name, clause); } if (isValidExpression || isValidExpressionClause) { @@ -111,10 +111,10 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const handleExpressionChange = ( expression: Expression | null, - expressionClause: Lib.ExpressionClause | null, + clause: Lib.AggregationClause | Lib.ExpressionClause | null, ) => { setExpression(expression); - setExpressionClause(expressionClause); + setClause(clause); setError(null); }; @@ -141,7 +141,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => {
Date: Mon, 27 Nov 2023 19:50:37 +0700 Subject: [PATCH 31/96] Update tests with new interface --- .../ExpressionWidget.unit.spec.tsx | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index 0c6aed4729aa6..25e49035db9c4 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -55,8 +55,8 @@ describe("ExpressionWidget", () => { ).toBeInTheDocument(); }); - it("should trigger onChangeExpressionClause if expression is valid", () => { - const { getRecentExpressionClauseInfo, onChangeExpressionClause } = setup(); + it("should trigger onChangeClause if expression is valid", () => { + const { getRecentExpressionClauseInfo, onChangeClause } = setup(); const doneButton = screen.getByRole("button", { name: "Done" }); expect(doneButton).toBeDisabled(); @@ -71,8 +71,8 @@ describe("ExpressionWidget", () => { userEvent.click(doneButton); - expect(onChangeExpressionClause).toHaveBeenCalledTimes(1); - expect(onChangeExpressionClause).toHaveBeenCalledWith( + expect(onChangeClause).toHaveBeenCalledTimes(1); + expect(onChangeClause).toHaveBeenCalledWith( "", ["+", 1, 1], expect.anything(), @@ -110,7 +110,7 @@ describe("ExpressionWidget", () => { const { getRecentExpressionClauseInfo, onChangeExpression, - onChangeExpressionClause, + onChangeClause, } = setup({ expression, withName: true, @@ -125,8 +125,8 @@ describe("ExpressionWidget", () => { userEvent.type(screen.getByDisplayValue("1 + 1"), "{enter}"); - // enter in expression editor should not trigger "onChangeExpressionClause" as popover is not valid with empty "name" - expect(onChangeExpressionClause).toHaveBeenCalledTimes(0); + // enter in expression editor should not trigger "onChangeClause" as popover is not valid with empty "name" + expect(onChangeClause).toHaveBeenCalledTimes(0); // The name must not be empty userEvent.type(expressionNameInput, ""); @@ -156,8 +156,8 @@ describe("ExpressionWidget", () => { "Some n_am!e 2q$w&YzT(6i~#sLXv7+HjP}Ku1|9c*RlF@4o5N=e8;G*-bZ3/U0:Qa'V,t(W-_D", expression, ); - expect(onChangeExpressionClause).toHaveBeenCalledTimes(1); - expect(onChangeExpressionClause).toHaveBeenCalledWith( + expect(onChangeClause).toHaveBeenCalledTimes(1); + expect(onChangeClause).toHaveBeenCalledWith( "Some n_am!e 2q$w&YzT(6i~#sLXv7+HjP}Ku1|9c*RlF@4o5N=e8;G*-bZ3/U0:Qa'V,t(W-_D", expect.anything(), ); @@ -185,14 +185,13 @@ function setup(additionalProps?: Partial) { const query = createQuery(); const stageIndex = 0; const onChangeExpression = jest.fn(); - const onChangeExpressionClause = jest.fn(); + const onChangeClause = jest.fn(); const onClose = jest.fn(); function getRecentExpressionClause() { - expect(onChangeExpressionClause).toHaveBeenCalled(); - const [_name, expressionClause] = - onChangeExpressionClause.mock.calls.at(-1); - return expressionClause; + expect(onChangeClause).toHaveBeenCalled(); + const [_name, clause] = onChangeClause.mock.calls.at(-1); + return clause; } function getRecentExpressionClauseInfo() { @@ -202,14 +201,14 @@ function setup(additionalProps?: Partial) { render( , @@ -218,7 +217,7 @@ function setup(additionalProps?: Partial) { return { getRecentExpressionClauseInfo, onChangeExpression, - onChangeExpressionClause, + onChangeClause, onClose, }; } From 3953f85b8f63764cbb09825fa45f52d82efd7b71 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 27 Nov 2023 20:05:31 +0700 Subject: [PATCH 32/96] Use overloading instead of generics --- frontend/src/metabase-lib/expression.ts | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index 51cc8e5d00bb9..97d4aa3bdadfa 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -25,11 +25,17 @@ export function expressionName(clause: ExpressionClause): string { return ML.expression_name(clause); } -export function withExpressionName< - Clause extends AggregationClause | ExpressionClause, ->(clause: Clause, newName: string): Clause { - return ML.with_expression_name(clause, newName); -} +declare function WithExpressionName( + clause: AggregationClause, + newName: string, +): AggregationClause; +declare function WithExpressionName( + clause: ExpressionClause, + newName: string, +): ExpressionClause; + +export const withExpressionName: typeof WithExpressionName = + ML.with_expression_name; export function expressions( query: Query, From 2074e8b54ec14a35eba0530fe5c0c0fd8e5d88c5 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 27 Nov 2023 20:07:20 +0700 Subject: [PATCH 33/96] Add function body --- frontend/src/metabase-lib/expression.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index 97d4aa3bdadfa..d406c6fac8267 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -34,8 +34,12 @@ declare function WithExpressionName( newName: string, ): ExpressionClause; -export const withExpressionName: typeof WithExpressionName = - ML.with_expression_name; +export const withExpressionName: typeof WithExpressionName = ( + clause, + newName, +) => { + return ML.with_expression_name(clause, newName); +}; export function expressions( query: Query, From ee9d5382ecab938c54a94d6fa38ad5fea4cc0aa2 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 27 Nov 2023 20:17:50 +0700 Subject: [PATCH 34/96] Rename operator to clause --- .../AggregationPicker/AggregationPicker.unit.spec.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index 5c7ed56d311c3..b94a3aaef7827 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -158,8 +158,8 @@ function setup({ function getRecentClause() { expect(onSelect).toHaveBeenCalled(); - const [operator] = onSelect.mock.calls.at(-1); - return operator; + const [clause] = onSelect.mock.calls.at(-1); + return clause; } function getRecentClauseInfo() { From f18cd7530b00c74a4881ca230dc9872ac80108ba Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 27 Nov 2023 20:19:14 +0700 Subject: [PATCH 35/96] Revert "Use overloading instead of generics" This reverts commit 3953f85b8f63764cbb09825fa45f52d82efd7b71. --- frontend/src/metabase-lib/expression.ts | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index d406c6fac8267..51cc8e5d00bb9 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -25,21 +25,11 @@ export function expressionName(clause: ExpressionClause): string { return ML.expression_name(clause); } -declare function WithExpressionName( - clause: AggregationClause, - newName: string, -): AggregationClause; -declare function WithExpressionName( - clause: ExpressionClause, - newName: string, -): ExpressionClause; - -export const withExpressionName: typeof WithExpressionName = ( - clause, - newName, -) => { +export function withExpressionName< + Clause extends AggregationClause | ExpressionClause, +>(clause: Clause, newName: string): Clause { return ML.with_expression_name(clause, newName); -}; +} export function expressions( query: Query, From 7baa4340631b6dd07c40f2e027613e257424c3dd Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 27 Nov 2023 21:00:53 +0700 Subject: [PATCH 36/96] Fix tests failing due to useSelect usage in AggregationPicker --- .../AggregationPicker/AggregationPicker.unit.spec.tsx | 4 ++-- .../notebook/steps/AggregateStep/AggregateStep.unit.spec.tsx | 4 ++-- .../sidebars/SummarizeSidebar/SummarizeSidebar.unit.spec.tsx | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index b94a3aaef7827..55469d222aef1 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -1,7 +1,7 @@ import _ from "underscore"; import userEvent from "@testing-library/user-event"; import { createMockMetadata } from "__support__/metadata"; -import { render, screen, within } from "__support__/ui"; +import { renderWithProviders, screen, within } from "__support__/ui"; import { checkNotNull } from "metabase/lib/types"; import type { Metric, StructuredDatasetQuery } from "metabase-types/api"; @@ -143,7 +143,7 @@ function setup({ const onSelect = jest.fn(); - render( + renderWithProviders( ); + renderWithProviders(); if (!withDefaultAggregation) { const countButton = screen.getByLabelText("Count"); From f28b10a00cfa9b1608907e7cee0e2a7172f7e4c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20Benk=C5=91?= Date: Tue, 28 Nov 2023 00:08:19 +0300 Subject: [PATCH 37/96] Omit aggregation options converting expressions to legacy Fixes #36120. --- frontend/src/metabase-lib/expression.ts | 3 ++- src/metabase/lib/js.cljs | 9 +++++-- test/metabase/lib/js_test.cljs | 31 ++++++++++++++++--------- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index ac30a10752a5f..ec54d28c13dd3 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -1,5 +1,6 @@ import * as ML from "cljs/metabase.lib.js"; import type { + AggregationClause, ColumnMetadata, ExpressionArg, ExpressionClause, @@ -73,7 +74,7 @@ export function expressionClauseForLegacyExpression( export function legacyExpressionForExpressionClause( query: Query, stageIndex: number, - expressionClause: ExpressionClause, + expressionClause: ExpressionClause | AggregationClause, ): any { return ML.legacy_expression_for_expression_clause( query, diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs index 1c4c58075c695..1faadcc41b7d5 100644 --- a/src/metabase/lib/js.cljs +++ b/src/metabase/lib/js.cljs @@ -1076,7 +1076,12 @@ (lib.convert/->pMBQL (lib.core/normalize (js->clj legacy-expression :keywordize-keys true))))) (defn ^:export legacy-expression-for-expression-clause - "Create a legacy expression from `an-expression-clause` at stage `stage-number` of `a-query`." + "Create a legacy expression from `an-expression-clause` at stage `stage-number` of `a-query`. + When processing aggregation clauses, the aggregation-options wrapper (e.g., specifying the name + of the aggregation expression) (if any) is thrown away." [a-query stage-number an-expression-clause] (lib.convert/with-aggregation-list (lib.core/aggregations a-query stage-number) - (-> an-expression-clause lib.convert/->legacy-MBQL clj->js))) + (let [legacy-expr (-> an-expression-clause lib.convert/->legacy-MBQL)] + (clj->js (cond-> legacy-expr + (= (first legacy-expr) :aggregation-options) + (get 1)))))) diff --git a/test/metabase/lib/js_test.cljs b/test/metabase/lib/js_test.cljs index 0b127ba502d73..bf68c151a33e4 100644 --- a/test/metabase/lib/js_test.cljs +++ b/test/metabase/lib/js_test.cljs @@ -142,17 +142,26 @@ (#'lib.js/cljs-key->js-key :many-pks?)))) (deftest ^:parallel expression-clause-<->-legacy-expression-test - (let [query (-> lib.tu/venues-query - (lib/expression "double-price" (lib/* (meta/field-metadata :venues :price) 2)) - (lib/aggregate (lib/sum [:expression {:lib/uuid (str (random-uuid))} "double-price"]))) - agg-uuid (-> query lib/aggregations first lib.options/uuid) - legacy-expr #js [">" #js ["aggregation" 0] 100] - pmbql-expr (lib.js/expression-clause-for-legacy-expression query -1 legacy-expr) - legacy-expr' (lib.js/legacy-expression-for-expression-clause query -1 pmbql-expr)] - (testing "from legacy expression" - (is (=? [:> {} [:aggregation {} agg-uuid] 100] pmbql-expr))) - (testing "from pMBQL expression" - (is (= (js->clj legacy-expr) (js->clj legacy-expr')))))) + (testing "conversion works both ways, even with aggregations (#34830, #36087)" + (let [query (-> lib.tu/venues-query + (lib/expression "double-price" (lib/* (meta/field-metadata :venues :price) 2)) + (lib/aggregate (lib/sum [:expression {:lib/uuid (str (random-uuid))} "double-price"]))) + agg-uuid (-> query lib/aggregations first lib.options/uuid) + legacy-expr #js [">" #js ["aggregation" 0] 100] + pmbql-expr (lib.js/expression-clause-for-legacy-expression query -1 legacy-expr) + legacy-expr' (lib.js/legacy-expression-for-expression-clause query -1 pmbql-expr)] + (testing "from legacy expression" + (is (=? [:> {} [:aggregation {} agg-uuid] 100] pmbql-expr))) + (testing "from pMBQL expression" + (is (= (js->clj legacy-expr) (js->clj legacy-expr')))))) + (testing "conversion drops aggregation-options (#36120)" + (let [query (-> lib.tu/venues-query + (lib/aggregate (lib.options/update-options (lib/sum (meta/field-metadata :venues :price)) + assoc :display-name "price sum"))) + agg-expr (-> query lib/aggregations first) + legacy-agg-expr #js ["sum" #js ["field" (meta/id :venues :price) #js {:base-type "Integer"}]] + legacy-agg-expr' (lib.js/legacy-expression-for-expression-clause query -1 agg-expr)] + (is (= (js->clj legacy-agg-expr) (js->clj legacy-agg-expr')))))) (deftest ^:parallel filter-drill-details-test (testing ":value field on the filter drill" From 1e2485c109d3c7b6cefd10fa7fb0344a4338d43a Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Tue, 28 Nov 2023 14:52:40 +0700 Subject: [PATCH 38/96] Remove temporary hack --- .../ExpressionEditorTextfield/ExpressionEditorTextfield.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx index 5821a97fbe18d..fc28e7b625f97 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx @@ -90,7 +90,7 @@ function transformPropsToState( } = props; const expression = clause - ? Lib.legacyExpressionForExpressionClause(query, stageIndex, clause)[1] // TODO: remove [1], see https://github.com/metabase/metabase/issues/36120 + ? Lib.legacyExpressionForExpressionClause(query, stageIndex, clause) : undefined; const source = format(expression, { legacyQuery, startRule }); @@ -139,10 +139,10 @@ class ExpressionEditorTextfield extends React.Component< query, stageIndex, this.props.clause, - )[1] // TODO: remove [1], see https://github.com/metabase/metabase/issues/36120 + ) : undefined; const newExpression = clause - ? Lib.legacyExpressionForExpressionClause(query, stageIndex, clause)[1] // TODO: remove [1], see https://github.com/metabase/metabase/issues/36120 + ? Lib.legacyExpressionForExpressionClause(query, stageIndex, clause) : undefined; const hasExpressionChanged = !_.isEqual(previousExpression, newExpression); From 505d11f1e7096776c2867d977a24db84859c9c55 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Tue, 28 Nov 2023 18:41:44 +0700 Subject: [PATCH 39/96] Get rid of props spread --- .../query_builder/components/notebook/steps/ClauseStep.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/notebook/steps/ClauseStep.tsx b/frontend/src/metabase/query_builder/components/notebook/steps/ClauseStep.tsx index 2671f5e2299cd..9419194cd0e2a 100644 --- a/frontend/src/metabase/query_builder/components/notebook/steps/ClauseStep.tsx +++ b/frontend/src/metabase/query_builder/components/notebook/steps/ClauseStep.tsx @@ -32,10 +32,10 @@ const ClauseStep = ({ initialAddText = null, tetherOptions = null, readOnly, - ...props + "data-testid": dataTestId, }: ClauseStepProps): JSX.Element => { return ( - + {items.map((item, index) => ( Date: Tue, 28 Nov 2023 21:51:04 +0700 Subject: [PATCH 40/96] Migrate isExpressionEditorInitiallyOpen to MLv2 --- .../AggregationPicker/AggregationPicker.tsx | 27 +++++++++---------- .../AggregationPicker.unit.spec.tsx | 1 - .../steps/AggregateStep/AggregateStep.tsx | 5 ---- .../AggregationItem/AggregationItem.tsx | 5 ---- .../SummarizeSidebar/SummarizeSidebar.tsx | 3 +-- 5 files changed, 14 insertions(+), 27 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 030ed06f53df8..16c50fb4b5fb6 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -6,14 +6,13 @@ import { Icon } from "metabase/core/components/Icon"; import { useToggle } from "metabase/hooks/use-toggle"; import { useSelector } from "metabase/lib/redux"; +import { isNotNull } from "metabase/lib/types"; import { getMetadata } from "metabase/selectors/metadata"; import { ExpressionWidget } from "metabase/query_builder/components/expressions/ExpressionWidget"; import { ExpressionWidgetHeader } from "metabase/query_builder/components/expressions/ExpressionWidgetHeader"; import * as Lib from "metabase-lib"; -import * as AGGREGATION from "metabase-lib/queries/utils/aggregation"; -import type LegacyAggregation from "metabase-lib/queries/structured/Aggregation"; import type StructuredQuery from "metabase-lib/queries/StructuredQuery"; import { QueryColumnPicker } from "../QueryColumnPicker"; @@ -36,7 +35,6 @@ interface AggregationPickerProps { operators: Lib.AggregationOperator[]; hasExpressionInput?: boolean; legacyQuery: StructuredQuery; - legacyClause?: LegacyAggregation; maxHeight?: number; onSelect: (operator: Lib.Aggregatable) => void; onClose?: () => void; @@ -71,24 +69,24 @@ export function AggregationPicker({ operators, hasExpressionInput = true, legacyQuery, - legacyClause, maxHeight = DEFAULT_MAX_HEIGHT, onSelect, onClose, }: AggregationPickerProps) { const metadata = useSelector(getMetadata); + const initialOperator = getInitialOperator(query, stageIndex, operators); const [ isEditingExpression, { turnOn: openExpressionEditor, turnOff: closeExpressionEditor }, - ] = useToggle(isExpressionEditorInitiallyOpen(legacyClause)); + ] = useToggle( + isExpressionEditorInitiallyOpen(query, clause, initialOperator), + ); // For really simple inline expressions like Average([Price]), // MLv2 can figure out that "Average" operator is used. // We don't want that though, so we don't break navigation inside the picker const [operator, setOperator] = useState( - isEditingExpression - ? null - : getInitialOperator(query, stageIndex, operators), + isEditingExpression ? null : initialOperator, ); const operatorInfo = useMemo( @@ -316,13 +314,14 @@ function getInitialOperator( return operator ?? null; } -function isExpressionEditorInitiallyOpen(legacyClause?: LegacyAggregation) { - // TODO: we need to add more information to AggregationOperatorDisplayInfo - // to be able to migrate legacyClause to MLv2 Lib.Aggregatable. - // This requires changes in Clojure code. +function isExpressionEditorInitiallyOpen( + query: Lib.Query, + clause: Lib.AggregationClause | Lib.ExpressionClause | undefined, + initialOperator: Lib.AggregationOperator | null, +) { return ( - legacyClause && - (AGGREGATION.isCustom(legacyClause) || AGGREGATION.isNamed(legacyClause)) + isNotNull(clause) && + (initialOperator === null || Lib.displayName(query, clause).length > 0) ); } diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index 55469d222aef1..d17bacfe5ea38 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -148,7 +148,6 @@ function setup({ query={query} clause={clause} legacyQuery={legacyQuery} - legacyClause={legacyQuery.aggregations()[0]} stageIndex={stageIndex} operators={operators} hasExpressionInput={hasExpressionInput} diff --git a/frontend/src/metabase/query_builder/components/notebook/steps/AggregateStep/AggregateStep.tsx b/frontend/src/metabase/query_builder/components/notebook/steps/AggregateStep/AggregateStep.tsx index 78661ace5beee..81cc806587ba9 100644 --- a/frontend/src/metabase/query_builder/components/notebook/steps/AggregateStep/AggregateStep.tsx +++ b/frontend/src/metabase/query_builder/components/notebook/steps/AggregateStep/AggregateStep.tsx @@ -121,16 +121,11 @@ function AggregationPopover({ : baseOperators; }, [query, clause, stageIndex, isUpdate]); - const legacyClause = isUpdate - ? legacyQuery.aggregations()[clauseIndex] - : undefined; - return ( { diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AggregationItem/AggregationItem.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AggregationItem/AggregationItem.tsx index 2ffe6b25f2da3..300f6ca4b912a 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AggregationItem/AggregationItem.tsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AggregationItem/AggregationItem.tsx @@ -9,7 +9,6 @@ const STAGE_INDEX = -1; interface AggregationItemProps { query: Lib.Query; aggregation: Lib.AggregationClause; - aggregationIndex: number; legacyQuery: StructuredQuery; onUpdate: (nextAggregation: Lib.Aggregatable) => void; onRemove: () => void; @@ -18,7 +17,6 @@ interface AggregationItemProps { export function AggregationItem({ query, aggregation, - aggregationIndex, legacyQuery, onUpdate, onRemove, @@ -30,8 +28,6 @@ export function AggregationItem({ aggregation, ); - const legacyClause = legacyQuery.aggregations()[aggregationIndex]; - return ( ( @@ -52,7 +48,6 @@ export function AggregationItem({ operators={operators} hasExpressionInput={false} legacyQuery={legacyQuery} - legacyClause={legacyClause} onSelect={nextAggregation => { onUpdate(nextAggregation); closePopover(); diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx index 61a661bc6beeb..655e8f300a2fa 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx @@ -134,14 +134,13 @@ export function SummarizeSidebar({ onDone={handleDoneClick} > - {aggregations.map((aggregation, index) => ( + {aggregations.map(aggregation => ( handleUpdateAggregation(aggregation, nextAggregation) From 95ddd4c74058e29840513bae75b76143e8d80582 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20Benk=C5=91?= Date: Tue, 28 Nov 2023 00:08:19 +0300 Subject: [PATCH 41/96] Omit aggregation options converting expressions to legacy Fixes #36120. --- frontend/src/metabase-lib/expression.ts | 3 ++- src/metabase/lib/js.cljs | 9 +++++-- test/metabase/lib/js_test.cljs | 31 ++++++++++++++++--------- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index ac30a10752a5f..ec54d28c13dd3 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -1,5 +1,6 @@ import * as ML from "cljs/metabase.lib.js"; import type { + AggregationClause, ColumnMetadata, ExpressionArg, ExpressionClause, @@ -73,7 +74,7 @@ export function expressionClauseForLegacyExpression( export function legacyExpressionForExpressionClause( query: Query, stageIndex: number, - expressionClause: ExpressionClause, + expressionClause: ExpressionClause | AggregationClause, ): any { return ML.legacy_expression_for_expression_clause( query, diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs index 1c4c58075c695..1faadcc41b7d5 100644 --- a/src/metabase/lib/js.cljs +++ b/src/metabase/lib/js.cljs @@ -1076,7 +1076,12 @@ (lib.convert/->pMBQL (lib.core/normalize (js->clj legacy-expression :keywordize-keys true))))) (defn ^:export legacy-expression-for-expression-clause - "Create a legacy expression from `an-expression-clause` at stage `stage-number` of `a-query`." + "Create a legacy expression from `an-expression-clause` at stage `stage-number` of `a-query`. + When processing aggregation clauses, the aggregation-options wrapper (e.g., specifying the name + of the aggregation expression) (if any) is thrown away." [a-query stage-number an-expression-clause] (lib.convert/with-aggregation-list (lib.core/aggregations a-query stage-number) - (-> an-expression-clause lib.convert/->legacy-MBQL clj->js))) + (let [legacy-expr (-> an-expression-clause lib.convert/->legacy-MBQL)] + (clj->js (cond-> legacy-expr + (= (first legacy-expr) :aggregation-options) + (get 1)))))) diff --git a/test/metabase/lib/js_test.cljs b/test/metabase/lib/js_test.cljs index 0b127ba502d73..bf68c151a33e4 100644 --- a/test/metabase/lib/js_test.cljs +++ b/test/metabase/lib/js_test.cljs @@ -142,17 +142,26 @@ (#'lib.js/cljs-key->js-key :many-pks?)))) (deftest ^:parallel expression-clause-<->-legacy-expression-test - (let [query (-> lib.tu/venues-query - (lib/expression "double-price" (lib/* (meta/field-metadata :venues :price) 2)) - (lib/aggregate (lib/sum [:expression {:lib/uuid (str (random-uuid))} "double-price"]))) - agg-uuid (-> query lib/aggregations first lib.options/uuid) - legacy-expr #js [">" #js ["aggregation" 0] 100] - pmbql-expr (lib.js/expression-clause-for-legacy-expression query -1 legacy-expr) - legacy-expr' (lib.js/legacy-expression-for-expression-clause query -1 pmbql-expr)] - (testing "from legacy expression" - (is (=? [:> {} [:aggregation {} agg-uuid] 100] pmbql-expr))) - (testing "from pMBQL expression" - (is (= (js->clj legacy-expr) (js->clj legacy-expr')))))) + (testing "conversion works both ways, even with aggregations (#34830, #36087)" + (let [query (-> lib.tu/venues-query + (lib/expression "double-price" (lib/* (meta/field-metadata :venues :price) 2)) + (lib/aggregate (lib/sum [:expression {:lib/uuid (str (random-uuid))} "double-price"]))) + agg-uuid (-> query lib/aggregations first lib.options/uuid) + legacy-expr #js [">" #js ["aggregation" 0] 100] + pmbql-expr (lib.js/expression-clause-for-legacy-expression query -1 legacy-expr) + legacy-expr' (lib.js/legacy-expression-for-expression-clause query -1 pmbql-expr)] + (testing "from legacy expression" + (is (=? [:> {} [:aggregation {} agg-uuid] 100] pmbql-expr))) + (testing "from pMBQL expression" + (is (= (js->clj legacy-expr) (js->clj legacy-expr')))))) + (testing "conversion drops aggregation-options (#36120)" + (let [query (-> lib.tu/venues-query + (lib/aggregate (lib.options/update-options (lib/sum (meta/field-metadata :venues :price)) + assoc :display-name "price sum"))) + agg-expr (-> query lib/aggregations first) + legacy-agg-expr #js ["sum" #js ["field" (meta/id :venues :price) #js {:base-type "Integer"}]] + legacy-agg-expr' (lib.js/legacy-expression-for-expression-clause query -1 agg-expr)] + (is (= (js->clj legacy-agg-expr) (js->clj legacy-agg-expr')))))) (deftest ^:parallel filter-drill-details-test (testing ":value field on the filter drill" From 58bd581c317da810cc32b80063c03ec45ba215aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20Benk=C5=91?= Date: Wed, 29 Nov 2023 01:25:02 +0300 Subject: [PATCH 42/96] Normalize legacy expressions as MBQL expressions --- src/metabase/lib/js.cljs | 5 ++++- test/metabase/lib/js_test.cljs | 7 +++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs index 1faadcc41b7d5..dd5ab34f32e31 100644 --- a/src/metabase/lib/js.cljs +++ b/src/metabase/lib/js.cljs @@ -25,6 +25,7 @@ [metabase.lib.stage :as lib.stage] [metabase.lib.util :as lib.util] [metabase.mbql.js :as mbql.js] + [metabase.mbql.normalize :as mbql.normalize] [metabase.shared.util.time :as shared.ut] [metabase.util :as u] [metabase.util.log :as log] @@ -1073,7 +1074,9 @@ "Create an expression clause from `legacy-expression` at stage `stage-number` of `a-query`." [a-query stage-number legacy-expression] (lib.convert/with-aggregation-list (lib.core/aggregations a-query stage-number) - (lib.convert/->pMBQL (lib.core/normalize (js->clj legacy-expression :keywordize-keys true))))) + (let [expr (js->clj legacy-expression :keywordize-keys true) + expr (first (mbql.normalize/normalize-fragment [:query :aggregation] [expr]))] + (lib.convert/->pMBQL expr)))) (defn ^:export legacy-expression-for-expression-clause "Create a legacy expression from `an-expression-clause` at stage `stage-number` of `a-query`. diff --git a/test/metabase/lib/js_test.cljs b/test/metabase/lib/js_test.cljs index bf68c151a33e4..ca476ce329bfe 100644 --- a/test/metabase/lib/js_test.cljs +++ b/test/metabase/lib/js_test.cljs @@ -161,6 +161,13 @@ agg-expr (-> query lib/aggregations first) legacy-agg-expr #js ["sum" #js ["field" (meta/id :venues :price) #js {:base-type "Integer"}]] legacy-agg-expr' (lib.js/legacy-expression-for-expression-clause query -1 agg-expr)] + (is (= (js->clj legacy-agg-expr) (js->clj legacy-agg-expr'))))) + (testing "legacy expressions are converted properly (#36120)" + (let [query (-> lib.tu/venues-query + (lib/aggregate (lib/count))) + agg-expr (-> query lib/aggregations first) + legacy-agg-expr #js ["count"] + legacy-agg-expr' (lib.js/legacy-expression-for-expression-clause query -1 agg-expr)] (is (= (js->clj legacy-agg-expr) (js->clj legacy-agg-expr')))))) (deftest ^:parallel filter-drill-details-test From 8afeac379d86108add913fa7d13bbb57e8718456 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 14:53:58 +0700 Subject: [PATCH 43/96] Fix isExpressionEditorInitiallyOpen --- .../components/AggregationPicker/AggregationPicker.tsx | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 16c50fb4b5fb6..ee53852685611 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -78,9 +78,7 @@ export function AggregationPicker({ const [ isEditingExpression, { turnOn: openExpressionEditor, turnOff: closeExpressionEditor }, - ] = useToggle( - isExpressionEditorInitiallyOpen(query, clause, initialOperator), - ); + ] = useToggle(isExpressionEditorInitiallyOpen(clause, initialOperator)); // For really simple inline expressions like Average([Price]), // MLv2 can figure out that "Average" operator is used. @@ -315,13 +313,12 @@ function getInitialOperator( } function isExpressionEditorInitiallyOpen( - query: Lib.Query, clause: Lib.AggregationClause | Lib.ExpressionClause | undefined, initialOperator: Lib.AggregationOperator | null, ) { return ( isNotNull(clause) && - (initialOperator === null || Lib.displayName(query, clause).length > 0) + (initialOperator === null || Boolean(Lib.expressionName(clause))) ); } From 7f7152ece70a4c2f1798c33dcc06527a75085360 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 14:54:05 +0700 Subject: [PATCH 44/96] Update expressionName signature --- frontend/src/metabase-lib/expression.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index 51cc8e5d00bb9..803146a2ee28f 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -21,7 +21,9 @@ export function expression( return ML.expression(query, stageIndex, expressionName, clause); } -export function expressionName(clause: ExpressionClause): string { +export function expressionName( + clause: AggregationClause | ExpressionClause, +): string | null { return ML.expression_name(clause); } From 3eb2181236c06cb17184ad496912cab8f5df840a Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 15:36:44 +0700 Subject: [PATCH 45/96] Pass props in ExpressionStep tests in a usual way --- .../steps/ExpressionStep.unit.spec.tsx | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.unit.spec.tsx b/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.unit.spec.tsx index a22927474a335..ce6dfd947317c 100644 --- a/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.unit.spec.tsx @@ -19,8 +19,7 @@ describe("Notebook Editor > Expression Step", () => { it("should handle updating existing expression", async () => { const expression: Expression = ["abs", ["field", 17, null]]; const { - props: { updateQuery }, - mocks: { addExpression, updateExpression }, + mocks: { addExpression, updateExpression, updateQuery }, } = setup(undefined, { // add an existing custom column expression "old name": expression, @@ -107,21 +106,20 @@ function setup( query, }); - const props = { - step, - color: "#93A1AB", - query, - topLevelQuery: step.topLevelQuery, - updateQuery, - isLastOpened: false, - reportTimezone: "UTC", - ...additionalProps, - }; - - render(); + render( + , + ); return { - props, - mocks: { addExpression, updateExpression, removeExpression }, + mocks: { addExpression, updateExpression, removeExpression, updateQuery }, }; } From c36cebd3fd946bb5b7bc384fa8e68c2986f28720 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 17:09:56 +0700 Subject: [PATCH 46/96] Drop a conditional statement --- .../query_builder/components/expressions/ExpressionWidget.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index d19aefa42c394..5baa37089769d 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -98,13 +98,11 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { if (isValidExpression) { onChangeExpression?.(name, expression); + onClose?.(); } if (isValidExpressionClause) { onChangeClause?.(name, clause); - } - - if (isValidExpression || isValidExpressionClause) { onClose?.(); } }; From 594b3899a62bac1917f6d257ab9b184905bda95d Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 17:15:19 +0700 Subject: [PATCH 47/96] Bring back expression prop for backwards-compatibility --- .../ExpressionEditorTextfield.tsx | 7 +++++-- .../components/expressions/ExpressionWidget.tsx | 1 + 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx index fc28e7b625f97..e1e9fa942bc85 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx @@ -45,6 +45,7 @@ const ACE_OPTIONS = { }; interface ExpressionEditorTextfieldProps { + expression: Expression | undefined; clause: Lib.AggregationClause | Lib.ExpressionClause | undefined; name: string; legacyQuery: StructuredQuery; @@ -83,15 +84,17 @@ function transformPropsToState( ): ExpressionEditorTextfieldState { const { legacyQuery, + expression: legacyExpression = ExpressionEditorTextfield.defaultProps + .expression, startRule = ExpressionEditorTextfield.defaultProps.startRule, clause, query, stageIndex, } = props; - - const expression = clause + const expressionFromClause = clause ? Lib.legacyExpressionForExpressionClause(query, stageIndex, clause) : undefined; + const expression = expressionFromClause ?? legacyExpression; const source = format(expression, { legacyQuery, startRule }); return { diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index 5baa37089769d..567cf7d40bf4a 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -139,6 +139,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => {
Date: Wed, 29 Nov 2023 17:24:17 +0700 Subject: [PATCH 48/96] Update ExpressionWidget validation & tests --- .../expressions/ExpressionWidget.tsx | 8 +++--- .../ExpressionWidget.unit.spec.tsx | 26 +++++++++++++++---- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index 567cf7d40bf4a..b02cb4f779eee 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -86,16 +86,16 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const helpTextTargetRef = useRef(null); const isValidName = withName ? name.trim().length > 0 : true; - const isValid = !error && isValidName; + const isValidExpression = isNotNull(expression) && isExpression(expression); + const isValidExpressionClause = isNotNull(clause); + const isValid = + !error && isValidName && (isValidExpression || isValidExpressionClause); const handleCommit = () => { if (!isValid) { return; } - const isValidExpression = isNotNull(expression) && isExpression(expression); - const isValidExpressionClause = isNotNull(clause); - if (isValidExpression) { onChangeExpression?.(name, expression); onClose?.(); diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index 25e49035db9c4..6f9f81c38e52c 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -55,6 +55,26 @@ describe("ExpressionWidget", () => { ).toBeInTheDocument(); }); + it("should trigger onChangeExpression if expression is valid", () => { + const { onChangeExpression } = setup(); + + const doneButton = screen.getByRole("button", { name: "Done" }); + expect(doneButton).toBeDisabled(); + + const expressionInput = screen.getByRole("textbox"); + expect(expressionInput).toHaveClass("ace_text-input"); + + userEvent.type(expressionInput, "1 + 1"); + userEvent.tab(); + + expect(doneButton).toBeEnabled(); + + userEvent.click(doneButton); + + expect(onChangeExpression).toHaveBeenCalledTimes(1); + expect(onChangeExpression).toHaveBeenCalledWith("", ["+", 1, 1]); + }); + it("should trigger onChangeClause if expression is valid", () => { const { getRecentExpressionClauseInfo, onChangeClause } = setup(); @@ -72,11 +92,7 @@ describe("ExpressionWidget", () => { userEvent.click(doneButton); expect(onChangeClause).toHaveBeenCalledTimes(1); - expect(onChangeClause).toHaveBeenCalledWith( - "", - ["+", 1, 1], - expect.anything(), - ); + expect(onChangeClause).toHaveBeenCalledWith("", expect.anything()); expect(getRecentExpressionClauseInfo()).toMatchObject({ displayName: "1 + 1", }); From 0a0802a89d618d774c58e8cb6fc175803be49e26 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 17:49:19 +0700 Subject: [PATCH 49/96] Bring back using expression in componentWillReceiveProps --- .../ExpressionEditorTextfield.tsx | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx index e1e9fa942bc85..941d245c50e39 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx @@ -136,18 +136,18 @@ class ExpressionEditorTextfield extends React.Component< newProps: Readonly, ) { // we only refresh our state if we had no previous state OR if our expression changed - const { legacyQuery, startRule, query, stageIndex, clause } = newProps; - const previousExpression = this.props.clause - ? Lib.legacyExpressionForExpressionClause( - query, - stageIndex, - this.props.clause, - ) - : undefined; - const newExpression = clause + const { expression, clause, legacyQuery, startRule, query, stageIndex } = + newProps; + const hasLegacyExpressionChanged = !_.isEqual( + this.props.expression, + expression, + ); + const hasClauseChanged = !_.isEqual(this.props.clause, clause); + const hasExpressionChanged = hasLegacyExpressionChanged || hasClauseChanged; + const expressionFromClause = clause ? Lib.legacyExpressionForExpressionClause(query, stageIndex, clause) : undefined; - const hasExpressionChanged = !_.isEqual(previousExpression, newExpression); + const newExpression = expressionFromClause ?? expression; if (!this.state || hasExpressionChanged) { const source = format(newExpression, { legacyQuery, startRule }); From f7d14cf1e5917799e30535c9e06ba83ac1ce0d40 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 17:51:02 +0700 Subject: [PATCH 50/96] Bring back removed assertion --- .../components/expressions/ExpressionWidget.unit.spec.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index 6f9f81c38e52c..72b9dee3aaf4c 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -141,8 +141,10 @@ describe("ExpressionWidget", () => { userEvent.type(screen.getByDisplayValue("1 + 1"), "{enter}"); - // enter in expression editor should not trigger "onChangeClause" as popover is not valid with empty "name" + // enter in expression editor should not trigger "onChangeClause" or "onChangeExpression" + // as popover is not valid with empty "name" expect(onChangeClause).toHaveBeenCalledTimes(0); + expect(onChangeExpression).toHaveBeenCalledTimes(0); // The name must not be empty userEvent.type(expressionNameInput, ""); From 92aa48f3b95e0d78df22d5058c754bb9e15563d8 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 18:23:13 +0700 Subject: [PATCH 51/96] Remove legacyQuery prop from AggregationPicker --- .../AggregationPicker/AggregationPicker.tsx | 11 ++++++++--- .../AggregationPicker.unit.spec.tsx | 10 +--------- .../notebook/steps/AggregateStep/AggregateStep.tsx | 6 ------ .../metabase/query_builder/components/view/View.jsx | 2 -- .../AddAggregationButton/AddAggregationButton.tsx | 4 ---- .../AggregationItem/AggregationItem.tsx | 4 ---- .../sidebars/SummarizeSidebar/SummarizeSidebar.tsx | 12 ------------ .../SummarizeSidebar/SummarizeSidebar.unit.spec.tsx | 6 ------ 8 files changed, 9 insertions(+), 46 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index ee53852685611..7f2374fcb235b 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -13,7 +13,8 @@ import { ExpressionWidget } from "metabase/query_builder/components/expressions/ import { ExpressionWidgetHeader } from "metabase/query_builder/components/expressions/ExpressionWidgetHeader"; import * as Lib from "metabase-lib"; -import type StructuredQuery from "metabase-lib/queries/StructuredQuery"; +import StructuredQuery from "metabase-lib/queries/StructuredQuery"; +import Question from "metabase-lib/Question"; import { QueryColumnPicker } from "../QueryColumnPicker"; import { @@ -34,7 +35,6 @@ interface AggregationPickerProps { stageIndex: number; operators: Lib.AggregationOperator[]; hasExpressionInput?: boolean; - legacyQuery: StructuredQuery; maxHeight?: number; onSelect: (operator: Lib.Aggregatable) => void; onClose?: () => void; @@ -68,7 +68,6 @@ export function AggregationPicker({ stageIndex, operators, hasExpressionInput = true, - legacyQuery, maxHeight = DEFAULT_MAX_HEIGHT, onSelect, onClose, @@ -205,6 +204,12 @@ export function AggregationPicker({ [onSelect, onClose], ); + const datasetQuery = Lib.toLegacyQuery(query); + const legacyQuery = new StructuredQuery( + new Question({ dataset_query: datasetQuery }), + datasetQuery, + ); + if (isEditingExpression) { return ( @@ -95,7 +92,6 @@ interface AggregationPopoverProps { ) => void; onAddAggregation: (aggregation: Lib.Aggregatable) => void; - legacyQuery: StructuredQuery; clauseIndex?: number; // Implicitly passed by metabase/components/Triggerable @@ -107,7 +103,6 @@ function AggregationPopover({ stageIndex, clause, clauseIndex, - legacyQuery, onAddAggregation, onUpdateAggregation, onClose, @@ -125,7 +120,6 @@ function AggregationPopover({ { diff --git a/frontend/src/metabase/query_builder/components/view/View.jsx b/frontend/src/metabase/query_builder/components/view/View.jsx index ed239628e106e..5d1a8d7060a95 100644 --- a/frontend/src/metabase/query_builder/components/view/View.jsx +++ b/frontend/src/metabase/query_builder/components/view/View.jsx @@ -169,11 +169,9 @@ class View extends Component { if (isShowingSummarySidebar) { const query = question._getMLv2Query(); - const legacyQuery = question.query(); return ( { const datesetQuery = Lib.toLegacyQuery(nextQuery); const nextQuestion = question.setDatasetQuery(datesetQuery); diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx index 11c6a46566d98..fc6c5f5f61a35 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx @@ -2,7 +2,6 @@ import { t } from "ttag"; import Tooltip from "metabase/core/components/Tooltip"; import TippyPopoverWithTrigger from "metabase/components/PopoverWithTrigger/TippyPopoverWithTrigger"; import * as Lib from "metabase-lib"; -import type StructuredQuery from "metabase-lib/queries/StructuredQuery"; import { AggregationPicker } from "../SummarizeSidebar.styled"; import { AddAggregationButtonRoot } from "./AddAggregationButton.styled"; @@ -10,13 +9,11 @@ const STAGE_INDEX = -1; interface AddAggregationButtonProps { query: Lib.Query; - legacyQuery: StructuredQuery; onAddAggregation: (aggregation: Lib.Aggregatable) => void; } export function AddAggregationButton({ query, - legacyQuery, onAddAggregation, }: AddAggregationButtonProps) { const hasAggregations = Lib.aggregations(query, STAGE_INDEX).length > 0; @@ -43,7 +40,6 @@ export function AddAggregationButton({ popoverContent={({ closePopover }) => ( void; onRemove: () => void; } @@ -17,7 +15,6 @@ interface AggregationItemProps { export function AggregationItem({ query, aggregation, - legacyQuery, onUpdate, onRemove, }: AggregationItemProps) { @@ -47,7 +44,6 @@ export function AggregationItem({ stageIndex={STAGE_INDEX} operators={operators} hasExpressionInput={false} - legacyQuery={legacyQuery} onSelect={nextAggregation => { onUpdate(nextAggregation); closePopover(); diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx index 655e8f300a2fa..6038bb97de652 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx @@ -4,7 +4,6 @@ import { t } from "ttag"; import { color } from "metabase/lib/colors"; import * as Lib from "metabase-lib"; -import type StructuredQuery from "metabase-lib/queries/StructuredQuery"; import { AddAggregationButton } from "./AddAggregationButton"; import { AggregationItem } from "./AggregationItem"; @@ -21,7 +20,6 @@ const STAGE_INDEX = -1; interface SummarizeSidebarProps { className?: string; query: Lib.Query; - legacyQuery: StructuredQuery; onQueryChange: (query: Lib.Query) => void; onClose: () => void; } @@ -29,7 +27,6 @@ interface SummarizeSidebarProps { export function SummarizeSidebar({ className, query: initialQuery, - legacyQuery: initialLegacyQuery, onQueryChange, onClose, }: SummarizeSidebarProps) { @@ -41,13 +38,6 @@ export function SummarizeSidebar({ [initialQuery, isDefaultAggregationRemoved], ); - const legacyQuery = useMemo(() => { - const question = initialLegacyQuery.question(); - return question - .setDatasetQuery(Lib.toLegacyQuery(query)) - .query() as StructuredQuery; - }, [query, initialLegacyQuery]); - const aggregations = Lib.aggregations(query, STAGE_INDEX); const hasAggregations = aggregations.length > 0; @@ -141,7 +131,6 @@ export function SummarizeSidebar({ } query={query} aggregation={aggregation} - legacyQuery={legacyQuery} onUpdate={nextAggregation => handleUpdateAggregation(aggregation, nextAggregation) } @@ -150,7 +139,6 @@ export function SummarizeSidebar({ ))} diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.unit.spec.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.unit.spec.tsx index e3a543738a50a..38ab5aeb66b0a 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.unit.spec.tsx @@ -16,7 +16,6 @@ import { } from "metabase-types/api/mocks/presets"; import * as Lib from "metabase-lib"; import Question from "metabase-lib/Question"; -import type StructuredQuery from "metabase-lib/queries/StructuredQuery"; import { SummarizeSidebar } from "./SummarizeSidebar"; type SetupOpts = { @@ -61,14 +60,9 @@ function setup({ function Wrapper() { const [query, setQuery] = useState(question._getMLv2Query()); - const legacyQuery = question - .setDatasetQuery(Lib.toLegacyQuery(query)) - .query() as StructuredQuery; - return ( { setQuery(nextQuery); onQueryChange(nextQuery); From e719fc2168b1dad1bcb622f3e5e2051725d157a9 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 20:45:48 +0700 Subject: [PATCH 52/96] Add a test case for isExpressionEditorInitiallyOpen (which uses Lib.expressionName) --- .../AggregationPicker.unit.spec.tsx | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index c2ee3f16be850..ec0dc8c2e7402 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -72,6 +72,24 @@ function createQueryWithInlineExpression() { }); } +function createQueryWithInlineExpressionWithOperator() { + return createQuery({ + query: { + database: SAMPLE_DB_ID, + type: "query", + query: { + aggregation: [ + [ + "aggregation-options", + ["count"], + { name: "My count", "display-name": "My count" }, + ], + ], + }, + }, + }); +} + const TEST_METRIC = createMockMetric({ id: 1, table_id: ORDERS_ID, @@ -381,13 +399,20 @@ describe("AggregationPicker", () => { expect(Lib.expressionName(getRecentClause())).toBe(expressionName); }); - it("should open the editor when an expression is used", async () => { + it("should open the editor when a named expression without operator is used", async () => { setup({ query: createQueryWithInlineExpression() }); expect(screen.getByText("Custom Expression")).toBeInTheDocument(); expect(screen.getByDisplayValue("Avg Q")).toBeInTheDocument(); }); + it("should open the editor when a named expression with operator is used", async () => { + setup({ query: createQueryWithInlineExpressionWithOperator() }); + + expect(screen.getByText("Custom Expression")).toBeInTheDocument(); + expect(screen.getByDisplayValue("My count")).toBeInTheDocument(); + }); + it("shouldn't be available if database doesn't support custom expressions", () => { setup({ metadata: createMetadata({ hasExpressionSupport: false }) }); expect(screen.queryByText("Custom Expression")).not.toBeInTheDocument(); From 0aec0270e6719ecdf5e353b17d1d741484d2aeb2 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 21:32:22 +0700 Subject: [PATCH 53/96] Fix legacyQuery creation --- .../common/components/AggregationPicker/AggregationPicker.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 7f2374fcb235b..28f743ae0edcc 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -206,7 +206,7 @@ export function AggregationPicker({ const datasetQuery = Lib.toLegacyQuery(query); const legacyQuery = new StructuredQuery( - new Question({ dataset_query: datasetQuery }), + new Question({ dataset_query: datasetQuery }, metadata), datasetQuery, ); From 45a50d34c44f48f974a6f0f44f73e2fcaaa19d08 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 21:46:20 +0700 Subject: [PATCH 54/96] Add assertion for expression name --- e2e/test/scenarios/question/notebook.cy.spec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/e2e/test/scenarios/question/notebook.cy.spec.js b/e2e/test/scenarios/question/notebook.cy.spec.js index f1c06c67d3664..8b93e209b0b9c 100644 --- a/e2e/test/scenarios/question/notebook.cy.spec.js +++ b/e2e/test/scenarios/question/notebook.cy.spec.js @@ -361,6 +361,8 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { cy.button("Done").should("not.be.disabled").click(); + cy.findByTestId("aggregate-step").should("contain.text", filter); + visualize(); // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage From b5bfee8b5b3c20bb49dc33235d890396b7e536e5 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 22:18:07 +0700 Subject: [PATCH 55/96] Fix committing expression with done button --- .../ExpressionEditorTextfield.tsx | 2 ++ .../components/expressions/ExpressionWidget.tsx | 14 +++++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx index 941d245c50e39..eb567574ae0f5 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx @@ -420,6 +420,8 @@ class ExpressionEditorTextfield extends React.Component< if (isExpression(expression)) { onCommit(expression, expressionClause); } + } else { + onError({ message: t`Invalid expression` }); } } } diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index b02cb4f779eee..115b84546815e 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -91,7 +91,15 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const isValid = !error && isValidName && (isValidExpression || isValidExpressionClause); - const handleCommit = () => { + const handleCommit = ( + expression: Expression | null, + clause: Lib.AggregationClause | Lib.ExpressionClause | null, + ) => { + const isValidExpression = isNotNull(expression) && isExpression(expression); + const isValidExpressionClause = isNotNull(clause); + const isValid = + !error && isValidName && (isValidExpression || isValidExpressionClause); + if (!isValid) { return; } @@ -166,7 +174,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { onChange={event => setName(event.target.value)} onKeyPress={e => { if (e.key === "Enter") { - handleCommit(); + handleCommit(expression, clause); } }} /> @@ -179,7 +187,7 @@ export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { From db7190742b739eeb9d05e1ec13bfeaac961467b4 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Wed, 29 Nov 2023 22:18:25 +0700 Subject: [PATCH 56/96] Improve assertions in tests --- e2e/test/scenarios/question/notebook.cy.spec.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/e2e/test/scenarios/question/notebook.cy.spec.js b/e2e/test/scenarios/question/notebook.cy.spec.js index 8b93e209b0b9c..f150c8e6ed38d 100644 --- a/e2e/test/scenarios/question/notebook.cy.spec.js +++ b/e2e/test/scenarios/question/notebook.cy.spec.js @@ -173,16 +173,21 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { cy.button("Done").click(); // change the corresponding custom expression - // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage - cy.findByText("Price is greater than 1").click(); + cy.findByTestId("step-filter-0-0") + .contains("Price is greater than 1") + .click(); cy.get(".Icon-chevronleft").click(); // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage cy.findByText("Custom Expression").click(); cy.get("@formula").clear().type("[Price] > 1 AND [Price] < 5{enter}"); - // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage - cy.contains(/^Price is less than 5/i); + cy.findByTestId("step-filter-0-0") + .contains("Price is greater than 1") + .should("exist"); + cy.findByTestId("step-filter-0-0") + .contains("Price is less than 5") + .should("exist"); }); it("should show the real number of rows instead of HARD_ROW_LIMIT when loading (metabase#17397)", () => { From 3b8f15bfa8867bfc2a054a5573ac6b08e235f3ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20Benk=C5=91?= Date: Wed, 29 Nov 2023 20:21:04 +0300 Subject: [PATCH 57/96] Make with-expression-name set the display-name of the clause Fixes #36167. This is needed to be able to set/change the display name of custom aggregation columns too. --- src/metabase/lib/expression.cljc | 4 +++- src/metabase/lib/js.cljs | 3 ++- test/metabase/lib/expression_test.cljc | 4 +++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/metabase/lib/expression.cljc b/src/metabase/lib/expression.cljc index 17d9436eea4d3..c9e8ccb630eb0 100644 --- a/src/metabase/lib/expression.cljc +++ b/src/metabase/lib/expression.cljc @@ -353,10 +353,12 @@ (-> an-expression-clause lib.options/options :lib/expression-name)) (mu/defn with-expression-name :- ::lib.schema.expression/expression - "Return a new expression clause like `an-expression-clause` but with name `new-name`." + "Return a new expression clause like `an-expression-clause` but with name `new-name`. + This also sets the :display-name property to support named aggregation expressions (custom aggregation columns)." [an-expression-clause :- ::lib.schema.expression/expression new-name :- :string] (lib.options/update-options an-expression-clause assoc :lib/expression-name new-name + :display-name new-name :lib/uuid (str (random-uuid)))) diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs index 1c4c58075c695..3378a55016b1c 100644 --- a/src/metabase/lib/js.cljs +++ b/src/metabase/lib/js.cljs @@ -701,7 +701,8 @@ (lib.core/expression-name an-expression-clause)) (defn ^:export with-expression-name - "Return an new expressions clause like `an-expression-clause` but with name `new-name`." + "Return an new expressions clause like `an-expression-clause` but with name `new-name`. + This also sets the :display-name property to support named aggregation expressions (custom aggregation columns)." [an-expression-clause new-name] (lib.core/with-expression-name an-expression-clause new-name)) diff --git a/test/metabase/lib/expression_test.cljc b/test/metabase/lib/expression_test.cljc index 7398265412126..49413df251a1b 100644 --- a/test/metabase/lib/expression_test.cljc +++ b/test/metabase/lib/expression_test.cljc @@ -369,6 +369,8 @@ (is (= ["expr"] (map lib/expression-name orig-exprs))) (is (= "newly-named-expression" - (lib/expression-name expr))) + (lib/expression-name expr) + (lib/display-name query expr) + (:display-name (lib/display-info query expr)))) (is (not= (lib.options/uuid orig-expr) (lib.options/uuid expr)))))) From fd32c5f29758e80193d4cd0ff3a53453eeb27f54 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 14:52:08 +0700 Subject: [PATCH 58/96] Add expression name assertion --- e2e/test/scenarios/filters/filter.cy.spec.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/e2e/test/scenarios/filters/filter.cy.spec.js b/e2e/test/scenarios/filters/filter.cy.spec.js index 894534e820a31..c744a7777dad7 100644 --- a/e2e/test/scenarios/filters/filter.cy.spec.js +++ b/e2e/test/scenarios/filters/filter.cy.spec.js @@ -1086,6 +1086,9 @@ describe("scenarios > question > filter", () => { ); cy.findByText("Done").click(); }); + cy.findByTestId("aggregate-step") + .contains("count if boolean is true") + .should("exist"); visualize(() => { cy.contains("2").should("exist"); }); From 33aec8f3012ed1005be050a4c048a497871ef094 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 14:54:36 +0700 Subject: [PATCH 59/96] Add expression name assertion --- e2e/test/scenarios/question/summarization.cy.spec.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/e2e/test/scenarios/question/summarization.cy.spec.js b/e2e/test/scenarios/question/summarization.cy.spec.js index 73fb402dab855..e065446159487 100644 --- a/e2e/test/scenarios/question/summarization.cy.spec.js +++ b/e2e/test/scenarios/question/summarization.cy.spec.js @@ -164,6 +164,9 @@ describe("scenarios > question > summarize sidebar", () => { ); cy.findByText("Done").click(); }); + cy.findByTestId("aggregate-step") + .contains("twice max total") + .should("exist"); visualize(); From e80ac60df2402ea0422ed2abafedc1bb123f0ba6 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 14:57:02 +0700 Subject: [PATCH 60/96] Add expression name assertion --- ...4-cc-postgres-percentile-accepts-two-params.cy.spec.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/e2e/test/scenarios/question/reproductions/15714-cc-postgres-percentile-accepts-two-params.cy.spec.js b/e2e/test/scenarios/question/reproductions/15714-cc-postgres-percentile-accepts-two-params.cy.spec.js index ba01364cb1bb8..fdd5c2c01fe0f 100644 --- a/e2e/test/scenarios/question/reproductions/15714-cc-postgres-percentile-accepts-two-params.cy.spec.js +++ b/e2e/test/scenarios/question/reproductions/15714-cc-postgres-percentile-accepts-two-params.cy.spec.js @@ -31,13 +31,17 @@ describe("postgres > question > custom columns", { tags: "@external" }, () => { cy.findByText("Custom Expression").click(); enterCustomColumnDetails({ formula: "Percentile([Subtotal], 0.1)" }); cy.findByPlaceholderText("Something nice and descriptive") - .as("description") + .as("name") .click(); // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage cy.findByText("Function Percentile expects 1 argument").should("not.exist"); - cy.get("@description").type("A"); + cy.get("@name").type("Expression name"); cy.button("Done").should("not.be.disabled").click(); // Todo: Add positive assertions once this is fixed + + cy.findByTestId("aggregate-step") + .contains("Expression name") + .should("exist"); }); }); From 6a0c758ce288060bf181bfa537f5c02230621caa Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 14:59:24 +0700 Subject: [PATCH 61/96] Add expression name assertion --- .../scenarios/question/reproductions/17512.cy.spec.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/e2e/test/scenarios/question/reproductions/17512.cy.spec.js b/e2e/test/scenarios/question/reproductions/17512.cy.spec.js index f44f025adff4c..ad637a33db8c5 100644 --- a/e2e/test/scenarios/question/reproductions/17512.cy.spec.js +++ b/e2e/test/scenarios/question/reproductions/17512.cy.spec.js @@ -22,6 +22,8 @@ describe("issue 17512", () => { "CE", ); + cy.findByTestId("aggregate-step").contains("CE").should("exist"); + // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage cy.findByText("Pick a column to group by").click(); // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage @@ -33,10 +35,8 @@ describe("issue 17512", () => { expect(body.error).to.not.exist; }); - // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage - cy.findByText("CE"); - // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage - cy.findByText("CC"); + cy.findAllByTestId("header-cell").contains("CE").should("exist"); + cy.findAllByTestId("header-cell").contains("CC").should("exist"); }); }); From 2795c3d6c9a0ff72bd04ff73a44f1874529be941 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 15:01:32 +0700 Subject: [PATCH 62/96] Add expression name assertion --- .../reproductions/18207-string-min-max.cy.spec.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/e2e/test/scenarios/question/reproductions/18207-string-min-max.cy.spec.js b/e2e/test/scenarios/question/reproductions/18207-string-min-max.cy.spec.js index 62679bb2b583b..84eebc4ae1831 100644 --- a/e2e/test/scenarios/question/reproductions/18207-string-min-max.cy.spec.js +++ b/e2e/test/scenarios/question/reproductions/18207-string-min-max.cy.spec.js @@ -71,13 +71,15 @@ describe("issue 18207", () => { it("should be possible to group by a string expression (metabase#18207)", () => { popover().contains("Custom Expression").click(); popover().within(() => { - enterCustomColumnDetails({ formula: "Max([Vendor])" }); - cy.findByPlaceholderText("Something nice and descriptive").type( - "LastVendor", - ); + enterCustomColumnDetails({ + formula: "Max([Vendor])", + name: "LastVendor", + }); cy.findByText("Done").click(); }); + cy.findByTestId("aggregate-step").contains("LastVendor").should("exist"); + // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage cy.contains("Pick a column to group by").click(); popover().contains("Category").click(); From 9f85ea9e5c943e8580db7396f9d9acf4c3f0a2a7 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 15:02:54 +0700 Subject: [PATCH 63/96] Add expression name assertion --- .../question/reproductions/6239-sort-using-cust-exp.cy.spec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/e2e/test/scenarios/question/reproductions/6239-sort-using-cust-exp.cy.spec.js b/e2e/test/scenarios/question/reproductions/6239-sort-using-cust-exp.cy.spec.js index 658a35cf377f6..bb9c63095fb07 100644 --- a/e2e/test/scenarios/question/reproductions/6239-sort-using-cust-exp.cy.spec.js +++ b/e2e/test/scenarios/question/reproductions/6239-sort-using-cust-exp.cy.spec.js @@ -22,6 +22,8 @@ describe("issue 6239", () => { cy.findByPlaceholderText("Something nice and descriptive").type("CE"); cy.button("Done").click(); + cy.findByTestId("aggregate-step").contains("CE").should("exist"); + // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage cy.findByText("Pick a column to group by").click(); popover().contains("Created At").first().click(); From a7a9ec692d4ae5453144561c7daaa70dc57aafaa Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 15:10:45 +0700 Subject: [PATCH 64/96] Add expression name assertion --- ...-aggregation-with-implicit-joins-and-nested-query.cy.spec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/e2e/test/scenarios/joins/reproductions/20519-cannot-join-on-aggregation-with-implicit-joins-and-nested-query.cy.spec.js b/e2e/test/scenarios/joins/reproductions/20519-cannot-join-on-aggregation-with-implicit-joins-and-nested-query.cy.spec.js index b19210732747a..41aa68cc7ac0e 100644 --- a/e2e/test/scenarios/joins/reproductions/20519-cannot-join-on-aggregation-with-implicit-joins-and-nested-query.cy.spec.js +++ b/e2e/test/scenarios/joins/reproductions/20519-cannot-join-on-aggregation-with-implicit-joins-and-nested-query.cy.spec.js @@ -55,6 +55,8 @@ describe("issue 20519", () => { cy.button("Done").click(); + cy.findByTestId("aggregate-step").contains("Two").should("exist"); + visualize(response => { expect(response.body.error).not.to.exist; }); From 49dfc003b16f7e5af71e724737979a8766f37696 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 15:11:44 +0700 Subject: [PATCH 65/96] Unskip an e2e test --- .../reproductions/28193-cannot-use-custom-column.cy.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/test/scenarios/models/reproductions/28193-cannot-use-custom-column.cy.spec.js b/e2e/test/scenarios/models/reproductions/28193-cannot-use-custom-column.cy.spec.js index 87dcf7fbc8283..c5334e92f83e6 100644 --- a/e2e/test/scenarios/models/reproductions/28193-cannot-use-custom-column.cy.spec.js +++ b/e2e/test/scenarios/models/reproductions/28193-cannot-use-custom-column.cy.spec.js @@ -3,7 +3,7 @@ import { ORDERS_QUESTION_ID } from "e2e/support/cypress_sample_instance_data"; const ccName = "CTax"; -describe.skip("issue 28193", () => { +describe("issue 28193", () => { beforeEach(() => { cy.intercept("POST", "/api/dataset").as("dataset"); From a7ed491903601cfec27ff2adb6feee5190665712 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 15:25:02 +0700 Subject: [PATCH 66/96] Add expression name assertion --- e2e/test/scenarios/question/notebook.cy.spec.js | 7 +++++-- e2e/test/scenarios/question/summarization.cy.spec.js | 8 ++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/e2e/test/scenarios/question/notebook.cy.spec.js b/e2e/test/scenarios/question/notebook.cy.spec.js index f150c8e6ed38d..397cf89b9a579 100644 --- a/e2e/test/scenarios/question/notebook.cy.spec.js +++ b/e2e/test/scenarios/question/notebook.cy.spec.js @@ -172,10 +172,11 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { cy.button("Done").click(); - // change the corresponding custom expression cy.findByTestId("step-filter-0-0") .contains("Price is greater than 1") .click(); + + // change the corresponding custom expression cy.get(".Icon-chevronleft").click(); // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage cy.findByText("Custom Expression").click(); @@ -311,6 +312,8 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { cy.button("Done").should("not.be.disabled").click(); + cy.findByTestId("step-filter-0-0").contains("Example"); + visualize(); // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage @@ -366,7 +369,7 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { cy.button("Done").should("not.be.disabled").click(); - cy.findByTestId("aggregate-step").should("contain.text", filter); + cy.findByTestId("aggregate-step").contains(filter).should("exist"); visualize(); diff --git a/e2e/test/scenarios/question/summarization.cy.spec.js b/e2e/test/scenarios/question/summarization.cy.spec.js index e065446159487..5099d3ed67aa5 100644 --- a/e2e/test/scenarios/question/summarization.cy.spec.js +++ b/e2e/test/scenarios/question/summarization.cy.spec.js @@ -158,10 +158,10 @@ describe("scenarios > question > summarize sidebar", () => { summarize({ mode: "notebook" }); popover().contains("Custom Expression").click(); popover().within(() => { - enterCustomColumnDetails({ formula: "2 * Max([Total])" }); - cy.findByPlaceholderText("Something nice and descriptive").type( - "twice max total", - ); + enterCustomColumnDetails({ + formula: "2 * Max([Total])", + name: "twice max total", + }); cy.findByText("Done").click(); }); cy.findByTestId("aggregate-step") From fc350054ca77e3f40cdc4ec3d27334c6e5871902 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 15:28:01 +0700 Subject: [PATCH 67/96] Add expression name assertion --- e2e/test/scenarios/admin/datamodel/metrics.cy.spec.js | 3 +++ e2e/test/scenarios/filters/filter.cy.spec.js | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/e2e/test/scenarios/admin/datamodel/metrics.cy.spec.js b/e2e/test/scenarios/admin/datamodel/metrics.cy.spec.js index c1d52d7b15791..befb7cf7fc559 100644 --- a/e2e/test/scenarios/admin/datamodel/metrics.cy.spec.js +++ b/e2e/test/scenarios/admin/datamodel/metrics.cy.spec.js @@ -120,6 +120,9 @@ describe("scenarios > admin > datamodel > metrics", () => { cy.findByPlaceholderText("Something nice and descriptive").type("Foo"); cy.button("Done").click(); + + cy.findByTestId("aggregate-step").contains("Foo").should("exist"); + cy.wait("@dataset"); // The test should fail on this step first diff --git a/e2e/test/scenarios/filters/filter.cy.spec.js b/e2e/test/scenarios/filters/filter.cy.spec.js index c744a7777dad7..f0c9eda4f5f9e 100644 --- a/e2e/test/scenarios/filters/filter.cy.spec.js +++ b/e2e/test/scenarios/filters/filter.cy.spec.js @@ -1080,10 +1080,10 @@ describe("scenarios > question > filter", () => { summarize({ mode: "notebook" }); popover().contains("Custom Expression").click(); popover().within(() => { - enterCustomColumnDetails({ formula: "CountIf(boolean)" }); - cy.findByPlaceholderText("Something nice and descriptive").type( - "count if boolean is true", - ); + enterCustomColumnDetails({ + formula: "CountIf(boolean)", + name: "count if boolean is true", + }); cy.findByText("Done").click(); }); cy.findByTestId("aggregate-step") From da11fb9f397feb2e47df7743cccf22eb862e854d Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 17:55:39 +0700 Subject: [PATCH 68/96] Use displayInfo instead of displayName - see https://github.com/metabase/metabase/pull/36203#discussion_r1410496658 --- .../AggregationPicker/AggregationPicker.tsx | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 28f743ae0edcc..ce1a2219b5bc6 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -78,6 +78,14 @@ export function AggregationPicker({ isEditingExpression, { turnOn: openExpressionEditor, turnOff: closeExpressionEditor }, ] = useToggle(isExpressionEditorInitiallyOpen(clause, initialOperator)); + const datasetQuery = Lib.toLegacyQuery(query); + const legacyQuery = new StructuredQuery( + new Question({ dataset_query: datasetQuery }, metadata), + datasetQuery, + ); + const displayInfo = clause + ? Lib.displayInfo(query, stageIndex, clause) + : clause; // For really simple inline expressions like Average([Price]), // MLv2 can figure out that "Average" operator is used. @@ -204,19 +212,13 @@ export function AggregationPicker({ [onSelect, onClose], ); - const datasetQuery = Lib.toLegacyQuery(query); - const legacyQuery = new StructuredQuery( - new Question({ dataset_query: datasetQuery }, metadata), - datasetQuery, - ); - if (isEditingExpression) { return ( Date: Thu, 30 Nov 2023 17:57:58 +0700 Subject: [PATCH 69/96] Deprecate displayName in favor of displayInfo - see https://metaboat.slack.com/archives/C0645JP1W81/p1701341786914219?thread_ts=1701335646.088359&cid=C0645JP1W81 --- frontend/src/metabase-lib/metadata.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frontend/src/metabase-lib/metadata.ts b/frontend/src/metabase-lib/metadata.ts index 9e665be8f6a59..d5e7a2b2b3fb3 100644 --- a/frontend/src/metabase-lib/metadata.ts +++ b/frontend/src/metabase-lib/metadata.ts @@ -41,6 +41,9 @@ export function metadataProvider( return ML.metadataProvider(databaseId, metadata); } +/** + * @deprecated use displayInfo instead + */ export function displayName(query: Query, clause: Clause): string { return ML_MetadataCalculation.display_name(query, clause); } From 9acc7ba910cc3c0ee3f694c08b6a9c50bade07e4 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 18:04:13 +0700 Subject: [PATCH 70/96] Use clearer notation --- .../common/components/AggregationPicker/AggregationPicker.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index ce1a2219b5bc6..1ddc297d97a59 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -85,7 +85,7 @@ export function AggregationPicker({ ); const displayInfo = clause ? Lib.displayInfo(query, stageIndex, clause) - : clause; + : undefined; // For really simple inline expressions like Average([Price]), // MLv2 can figure out that "Average" operator is used. From 91be09a591a873ad39926e6248a4176f210e2238 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Thu, 30 Nov 2023 18:39:46 +0700 Subject: [PATCH 71/96] Format code --- e2e/test/scenarios/question/notebook.cy.spec.js | 2 +- .../components/AggregationPicker/AggregationPicker.tsx | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/e2e/test/scenarios/question/notebook.cy.spec.js b/e2e/test/scenarios/question/notebook.cy.spec.js index 397cf89b9a579..b46a934dafd4c 100644 --- a/e2e/test/scenarios/question/notebook.cy.spec.js +++ b/e2e/test/scenarios/question/notebook.cy.spec.js @@ -312,7 +312,7 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { cy.button("Done").should("not.be.disabled").click(); - cy.findByTestId("step-filter-0-0").contains("Example"); + cy.findByTestId("step-filter-0-0").contains("Example").should("exist"); visualize(); diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 1ddc297d97a59..1872a20f005d5 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -6,7 +6,6 @@ import { Icon } from "metabase/core/components/Icon"; import { useToggle } from "metabase/hooks/use-toggle"; import { useSelector } from "metabase/lib/redux"; -import { isNotNull } from "metabase/lib/types"; import { getMetadata } from "metabase/selectors/metadata"; import { ExpressionWidget } from "metabase/query_builder/components/expressions/ExpressionWidget"; @@ -323,10 +322,10 @@ function isExpressionEditorInitiallyOpen( clause: Lib.AggregationClause | Lib.ExpressionClause | undefined, initialOperator: Lib.AggregationOperator | null, ) { - return ( - isNotNull(clause) && - (initialOperator === null || Boolean(Lib.expressionName(clause))) - ); + const isCustomExpression = initialOperator === null; + const hasCustomName = Boolean(clause && Lib.expressionName(clause)); + + return clause && (isCustomExpression || hasCustomName); } function getOperatorListItem( From cb6890116c407c340b08e2a0db1545ebf69c6b34 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 1 Dec 2023 14:23:58 +0700 Subject: [PATCH 72/96] Use displayInfo instead of expressionName in tests --- .../AggregationPicker.unit.spec.tsx | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index ec0dc8c2e7402..cdcf43b1fac2e 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -175,7 +175,14 @@ function setup({ return Lib.displayInfo(query, stageIndex, getRecentClause()); } - return { metadata, getRecentClause, getRecentClauseInfo, onSelect }; + return { + metadata, + query, + stageIndex, + getRecentClause, + getRecentClauseInfo, + onSelect, + }; } describe("AggregationPicker", () => { @@ -385,7 +392,8 @@ describe("AggregationPicker", () => { describe("custom expressions", () => { it("should allow to enter a custom expression", async () => { - const { getRecentClause, getRecentClauseInfo } = setup(); + const { query, stageIndex, getRecentClause, getRecentClauseInfo } = + setup(); const expression = "1 + 1"; const expressionName = "My expression"; @@ -396,7 +404,9 @@ describe("AggregationPicker", () => { userEvent.click(screen.getByRole("button", { name: "Done" })); expect(getRecentClauseInfo()).toMatchObject({ displayName: expression }); - expect(Lib.expressionName(getRecentClause())).toBe(expressionName); + expect( + Lib.displayInfo(query, stageIndex, getRecentClause()).displayName, + ).toBe(expressionName); }); it("should open the editor when a named expression without operator is used", async () => { From f699c9044c047667a4f53a559c71500e0eccd198 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 1 Dec 2023 14:27:38 +0700 Subject: [PATCH 73/96] Add isNamed attribute to ColumnDisplayInfo and ClauseDisplayInfo --- frontend/src/metabase-lib/types.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frontend/src/metabase-lib/types.ts b/frontend/src/metabase-lib/types.ts index 6351613c65eae..ede973f74cbd4 100644 --- a/frontend/src/metabase-lib/types.ts +++ b/frontend/src/metabase-lib/types.ts @@ -101,6 +101,7 @@ export type ColumnDisplayInfo = { name: string; displayName: string; longDisplayName: string; + isNamed: boolean; fkReferenceName?: string; isCalculated: boolean; @@ -133,7 +134,7 @@ export type MetricDisplayInfo = { export type ClauseDisplayInfo = Pick< ColumnDisplayInfo, - "name" | "displayName" | "longDisplayName" | "table" + "name" | "displayName" | "longDisplayName" | "isNamed" | "table" >; export type AggregationClauseDisplayInfo = ClauseDisplayInfo; From 73283d918bd74fb4b2a716807ec7d403deebe7aa Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 1 Dec 2023 14:27:45 +0700 Subject: [PATCH 74/96] Use displayInfo instead of expressionName in AggregationPicker --- .../AggregationPicker/AggregationPicker.tsx | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 1872a20f005d5..97e9c52e2a201 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -76,7 +76,9 @@ export function AggregationPicker({ const [ isEditingExpression, { turnOn: openExpressionEditor, turnOff: closeExpressionEditor }, - ] = useToggle(isExpressionEditorInitiallyOpen(clause, initialOperator)); + ] = useToggle( + isExpressionEditorInitiallyOpen(query, stageIndex, clause, initialOperator), + ); const datasetQuery = Lib.toLegacyQuery(query); const legacyQuery = new StructuredQuery( new Question({ dataset_query: datasetQuery }, metadata), @@ -319,13 +321,20 @@ function getInitialOperator( } function isExpressionEditorInitiallyOpen( + query: Lib.Query, + stageIndex: number, clause: Lib.AggregationClause | Lib.ExpressionClause | undefined, initialOperator: Lib.AggregationOperator | null, -) { +): boolean { + if (!clause) { + return false; + } + const isCustomExpression = initialOperator === null; - const hasCustomName = Boolean(clause && Lib.expressionName(clause)); + const displayInfo = Lib.displayInfo(query, stageIndex, clause); + const hasCustomName = Boolean(displayInfo?.isNamed); - return clause && (isCustomExpression || hasCustomName); + return isCustomExpression || hasCustomName; } function getOperatorListItem( From 2d6ca04aa0f17ef42212aa7414887bbecd550abd Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 1 Dec 2023 14:27:59 +0700 Subject: [PATCH 75/96] Drop expressionName --- frontend/src/metabase-lib/expression.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index 803146a2ee28f..de1f9f7764c2b 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -21,12 +21,6 @@ export function expression( return ML.expression(query, stageIndex, expressionName, clause); } -export function expressionName( - clause: AggregationClause | ExpressionClause, -): string | null { - return ML.expression_name(clause); -} - export function withExpressionName< Clause extends AggregationClause | ExpressionClause, >(clause: Clause, newName: string): Clause { From 31ce8e73da9d07411449e0f3b10af0d493dd1fff Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 1 Dec 2023 14:35:35 +0700 Subject: [PATCH 76/96] Drop redundant cast --- .../common/components/AggregationPicker/AggregationPicker.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 97e9c52e2a201..179ad7f53e093 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -332,7 +332,7 @@ function isExpressionEditorInitiallyOpen( const isCustomExpression = initialOperator === null; const displayInfo = Lib.displayInfo(query, stageIndex, clause); - const hasCustomName = Boolean(displayInfo?.isNamed); + const hasCustomName = displayInfo.isNamed; return isCustomExpression || hasCustomName; } From 86c6a802de9a78113ca72050c56c35bbe959fb13 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Fri, 1 Dec 2023 15:16:15 +0700 Subject: [PATCH 77/96] Simplify isExpressionEditorInitiallyOpen interface --- .../AggregationPicker/AggregationPicker.tsx | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 179ad7f53e093..6dcfa74d7b0e1 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -72,21 +72,19 @@ export function AggregationPicker({ onClose, }: AggregationPickerProps) { const metadata = useSelector(getMetadata); + const displayInfo = clause + ? Lib.displayInfo(query, stageIndex, clause) + : undefined; const initialOperator = getInitialOperator(query, stageIndex, operators); const [ isEditingExpression, { turnOn: openExpressionEditor, turnOff: closeExpressionEditor }, - ] = useToggle( - isExpressionEditorInitiallyOpen(query, stageIndex, clause, initialOperator), - ); + ] = useToggle(isExpressionEditorInitiallyOpen(displayInfo, initialOperator)); const datasetQuery = Lib.toLegacyQuery(query); const legacyQuery = new StructuredQuery( new Question({ dataset_query: datasetQuery }, metadata), datasetQuery, ); - const displayInfo = clause - ? Lib.displayInfo(query, stageIndex, clause) - : undefined; // For really simple inline expressions like Average([Price]), // MLv2 can figure out that "Average" operator is used. @@ -321,18 +319,11 @@ function getInitialOperator( } function isExpressionEditorInitiallyOpen( - query: Lib.Query, - stageIndex: number, - clause: Lib.AggregationClause | Lib.ExpressionClause | undefined, + displayInfo: Lib.ClauseDisplayInfo | undefined, initialOperator: Lib.AggregationOperator | null, ): boolean { - if (!clause) { - return false; - } - const isCustomExpression = initialOperator === null; - const displayInfo = Lib.displayInfo(query, stageIndex, clause); - const hasCustomName = displayInfo.isNamed; + const hasCustomName = Boolean(displayInfo?.isNamed); return isCustomExpression || hasCustomName; } From 1ee3df62c5322f9e5ce965d78eb66c485a881e86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20Benk=C5=91?= Date: Sat, 2 Dec 2023 02:06:56 +0300 Subject: [PATCH 78/96] Use display-info with :named? instead of expression-name --- frontend/src/metabase-lib/expression.ts | 4 -- src/metabase/lib/core.cljc | 1 - src/metabase/lib/expression.cljc | 22 ++++---- src/metabase/lib/js.cljs | 10 ++-- src/metabase/lib/metadata/calculation.cljc | 24 ++++++++- src/metabase/lib/util.cljc | 6 +-- test/metabase/lib/expression_test.cljc | 60 ++++++++++++++++------ 7 files changed, 84 insertions(+), 43 deletions(-) diff --git a/frontend/src/metabase-lib/expression.ts b/frontend/src/metabase-lib/expression.ts index ac30a10752a5f..d0ee85c96ddde 100644 --- a/frontend/src/metabase-lib/expression.ts +++ b/frontend/src/metabase-lib/expression.ts @@ -20,10 +20,6 @@ export function expression( return ML.expression(query, stageIndex, expressionName, clause); } -export function expressionName(clause: ExpressionClause): string { - return ML.expression_name(clause); -} - export function withExpressionName( clause: ExpressionClause, newName: string, diff --git a/src/metabase/lib/core.cljc b/src/metabase/lib/core.cljc index e9f6a7c18a975..91a4c464815c1 100644 --- a/src/metabase/lib/core.cljc +++ b/src/metabase/lib/core.cljc @@ -115,7 +115,6 @@ find-matching-column] [lib.expression expression - expression-name expressions expressions-metadata expressionable-columns diff --git a/src/metabase/lib/expression.cljc b/src/metabase/lib/expression.cljc index c9e8ccb630eb0..e98cdb7415612 100644 --- a/src/metabase/lib/expression.cljc +++ b/src/metabase/lib/expression.cljc @@ -347,18 +347,20 @@ (expression-metadata query stage-number) lib.ref/ref))) -(mu/defn expression-name :- :string - "Return the name of `an-expression-clause`." - [an-expression-clause :- ::lib.schema.expression/expression] - (-> an-expression-clause lib.options/options :lib/expression-name)) - (mu/defn with-expression-name :- ::lib.schema.expression/expression "Return a new expression clause like `an-expression-clause` but with name `new-name`. - This also sets the :display-name property to support named aggregation expressions (custom aggregation columns)." + For expressions from the :expressions clause of a pMBQL query this sets the :lib/expression-name option, + for other expressions (for example named aggregation expressions) the :display-name option is set. + + Note that always setting :lib/expression-name would lead to confusion, because that option is used + to decide what kind of reference is to be created. For example, expression are referenced by name, + aggregations are referenced by position." [an-expression-clause :- ::lib.schema.expression/expression new-name :- :string] (lib.options/update-options - an-expression-clause assoc - :lib/expression-name new-name - :display-name new-name - :lib/uuid (str (random-uuid)))) + an-expression-clause + (fn [opts] + (let [opts (assoc opts :lib/uuid (str (random-uuid)))] + (if (:lib/expression-name opts) + (assoc opts :lib/expression-name new-name) + (assoc opts :display-name new-name)))))) diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs index 3378a55016b1c..404828e8be0fa 100644 --- a/src/metabase/lib/js.cljs +++ b/src/metabase/lib/js.cljs @@ -695,14 +695,10 @@ [a-query stage-number expression-name an-expression-clause] (lib.core/expression a-query stage-number expression-name an-expression-clause)) -(defn ^:export expression-name - "Return the name of `an-expression-clause`." - [an-expression-clause] - (lib.core/expression-name an-expression-clause)) - (defn ^:export with-expression-name - "Return an new expressions clause like `an-expression-clause` but with name `new-name`. - This also sets the :display-name property to support named aggregation expressions (custom aggregation columns)." + "Return a new expression clause like `an-expression-clause` but with name `new-name`. + For expressions from the :expressions clause of a pMBQL query this sets the :lib/expression-name option, + for other expressions (for example named aggregation expressions) the :display-name option is set." [an-expression-clause new-name] (lib.core/with-expression-name an-expression-clause new-name)) diff --git a/src/metabase/lib/metadata/calculation.cljc b/src/metabase/lib/metadata/calculation.cljc index 23bac7b0f1ca4..0f34134d199e7 100644 --- a/src/metabase/lib/metadata/calculation.cljc +++ b/src/metabase/lib/metadata/calculation.cljc @@ -67,7 +67,7 @@ style :- DisplayNameStyle] (or ;; if this is an MBQL clause with `:display-name` in the options map, then use that rather than calculating a name. - (:display-name (lib.options/options x)) + ((some-fn :display-name :lib/expression-name) (lib.options/options x)) (try (display-name-method query stage-number x style) (catch #?(:clj Throwable :cljs js/Error) e @@ -97,7 +97,6 @@ (defmethod display-name-method :default [_query _stage-number x _stage] - ;; hopefully this is dev-facing only, so not i18n'ed. (log/warnf "Don't know how to calculate display name for %s. Add an impl for %s for %s" (pr-str x) `display-name-method @@ -338,6 +337,24 @@ {:query query, :stage-number stage-number, :x x} e)))))))) +(defmulti custom-name-method + "Implementation for [[custom-name]]." + {:arglists '([x])} + lib.dispatch/dispatch-value + :hierarchy lib.hierarchy/hierarchy) + +(defn custom-name + "Return the user supplied name of `x`, if any." + [x] + (custom-name-method x)) + +(defmethod custom-name-method :default + [x] + ;; we assume that clauses only get a :display-name option if the user explicitly specifies it + ;; expressions from the :expressions clause of pMBQL queries have custom names by default + (when (lib.util/clause? x) + ((some-fn :display-name :lib/expression-name) (lib.options/options x)))) + (defn default-display-info "Default implementation of [[display-info-method]], available in case you want to use this in a different implementation and add additional information to it." @@ -347,6 +364,9 @@ ;; TODO -- not 100% convinced the FE should actually have access to `:name`, can't it use `:display-name` ;; everywhere? Determine whether or not this is the case. (select-keys x-metadata [:name :display-name :semantic-type]) + (when-let [custom (custom-name x)] + {:display-name custom + :named? true}) (when-let [long-display-name (display-name query stage-number x :long)] {:long-display-name long-display-name}) ;; don't return `:base-type`, FE should just use `:effective-type` everywhere and not even need to know diff --git a/src/metabase/lib/util.cljc b/src/metabase/lib/util.cljc index aa868232965b1..97ecd7253c80b 100644 --- a/src/metabase/lib/util.cljc +++ b/src/metabase/lib/util.cljc @@ -41,10 +41,10 @@ "Returns true if this is a clause." [clause] (and (vector? clause) - (> (count clause) 1) (keyword? (first clause)) - (map? (second clause)) - (contains? (second clause) :lib/uuid))) + (let [opts (get clause 1)] + (and (map? opts) + (contains? opts :lib/uuid))))) (defn clause-of-type? "Returns true if this is a clause." diff --git a/test/metabase/lib/expression_test.cljc b/test/metabase/lib/expression_test.cljc index 49413df251a1b..be3ff8fde4b09 100644 --- a/test/metabase/lib/expression_test.cljc +++ b/test/metabase/lib/expression_test.cljc @@ -244,11 +244,12 @@ (-> lib.tu/venues-query (lib/expression "expr" (lib/absolute-datetime "2020" :month)) lib/expressions-metadata))) - (is (= ["expr"] - (-> lib.tu/venues-query - (lib/expression "expr" (lib/absolute-datetime "2020" :month)) - lib/expressions - (->> (map lib/expression-name)))))) + (is (=? [{:display-name "expr" + :named? true}] + (-> lib.tu/venues-query + (lib/expression "expr" (lib/absolute-datetime "2020" :month)) + lib/expressions + (->> (map (fn [expr] (lib/display-info lib.tu/venues-query expr)))))))) (testing "collisions with other column names are detected and rejected" (let [query (lib/query meta/metadata-provider (meta/table-metadata :categories)) ex (try @@ -358,19 +359,46 @@ (is (empty? (lib/expressions dropped))))))) (deftest ^:parallel with-expression-name-test - (let [query (-> lib.tu/venues-query - (lib/expression "expr" (lib/absolute-datetime "2020" :month))) - [orig-expr :as orig-exprs] (lib/expressions query) - expr (lib/with-expression-name orig-expr "newly-named-expression")] + (let [query (-> lib.tu/venues-query + (lib/expression "expr" (lib/absolute-datetime "2020" :month)) + (lib/aggregate (lib/count))) + [orig-expr] (lib/expressions query) + expr (lib/with-expression-name orig-expr "newly-named-expression") + [orig-agg] (lib/aggregations query) + agg (lib/with-expression-name orig-agg "my count")] (testing "expressions should include the original expression name" (is (=? [{:name "expr" :display-name "expr"}] - (lib/expressions-metadata query))) - (is (= ["expr"] - (map lib/expression-name orig-exprs))) + (lib/expressions-metadata query)))) + (testing "expressions from the expressions query clause can be renamed" + (is (= "newly-named-expression" + (lib/display-name query expr))) + (is (nil? (:display-name (lib.options/options expr)))) + (is (=? {:display-name "expr" + :named? true} + (lib/display-info query orig-expr))) + (is (= "expr" + (lib/display-name query orig-expr))) + (is (=? {:display-name "newly-named-expression" + :named? true} + (lib/display-info query expr))) (is (= "newly-named-expression" - (lib/expression-name expr) - (lib/display-name query expr) - (:display-name (lib/display-info query expr)))) + (lib/display-name query expr))) (is (not= (lib.options/uuid orig-expr) - (lib.options/uuid expr)))))) + (lib.options/uuid expr)))) + (testing "aggregation expressions can be renamed" + (is (= "my count" + (lib/display-name query agg))) + (is (nil? (:lib/expression-name (lib.options/options agg)))) + (is (=? {:display-name "Count" + :named? (symbol "nil #_\"key is not present.\"")} + (lib/display-info query orig-agg))) + (is (= "Count" + (lib/display-name query orig-agg))) + (is (=? {:display-name "my count" + :named? true} + (lib/display-info query agg))) + (is (= "my count" + (lib/display-name query agg))) + (is (not= (lib.options/uuid orig-agg) + (lib.options/uuid agg)))))) From 0bfcedd886de9259f879ea337fac704177917517 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20Benk=C5=91?= Date: Sat, 2 Dec 2023 02:12:00 +0300 Subject: [PATCH 79/96] Improve comment --- src/metabase/lib/metadata/calculation.cljc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/metabase/lib/metadata/calculation.cljc b/src/metabase/lib/metadata/calculation.cljc index 0f34134d199e7..ef300b5eab4fa 100644 --- a/src/metabase/lib/metadata/calculation.cljc +++ b/src/metabase/lib/metadata/calculation.cljc @@ -350,8 +350,8 @@ (defmethod custom-name-method :default [x] - ;; we assume that clauses only get a :display-name option if the user explicitly specifies it - ;; expressions from the :expressions clause of pMBQL queries have custom names by default + ;; We assume that clauses only get a :display-name option if the user explicitly specifies it. + ;; Expressions from the :expressions clause of pMBQL queries have custom names by default. (when (lib.util/clause? x) ((some-fn :display-name :lib/expression-name) (lib.options/options x)))) From c3e015ea48245d564a210bb4d8773b2197961e26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20Benk=C5=91?= Date: Sat, 2 Dec 2023 02:22:48 +0300 Subject: [PATCH 80/96] Add :named? to the schema of display-info --- frontend/src/metabase-lib/types.ts | 4 +++- src/metabase/lib/metadata/calculation.cljc | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/frontend/src/metabase-lib/types.ts b/frontend/src/metabase-lib/types.ts index 0091c695afd89..5366e2dd2ba25 100644 --- a/frontend/src/metabase-lib/types.ts +++ b/frontend/src/metabase-lib/types.ts @@ -131,7 +131,9 @@ export type MetricDisplayInfo = { export type ClauseDisplayInfo = Pick< ColumnDisplayInfo, "name" | "displayName" | "longDisplayName" | "table" ->; +> & { + isNamed?: boolean; +}; export type AggregationClauseDisplayInfo = ClauseDisplayInfo; diff --git a/src/metabase/lib/metadata/calculation.cljc b/src/metabase/lib/metadata/calculation.cljc index ef300b5eab4fa..27b35797acfb5 100644 --- a/src/metabase/lib/metadata/calculation.cljc +++ b/src/metabase/lib/metadata/calculation.cljc @@ -278,6 +278,8 @@ [:map [:display-name :string] [:long-display-name {:optional true} :string] + ;; for things with user specified names + [:named? {:optional true} :boolean] ;; for things that have a Table, e.g. a Field [:table {:optional true} [:maybe [:ref ::display-info]]] ;; these are derived from the `:lib/source`/`:metabase.lib.schema.metadata/column-source`, but instead of using From f6d7c0ab08fe391703e9b1f5700f4384aa586aa2 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 18:14:25 +0700 Subject: [PATCH 81/96] Use conditional types for ExpressionWidget prop --- .../components/expressions/ExpressionWidget.tsx | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx index 115b84546815e..f5bd198316518 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.tsx @@ -28,7 +28,17 @@ const EXPRESSIONS_DOCUMENTATION_URL = MetabaseSettings.docsUrl( "questions/query-builder/expressions", ); -export interface ExpressionWidgetProps { +interface LegacyQueryProps { + query?: never; + stageIndex?: never; +} + +interface QueryProps { + query: Lib.Query; + stageIndex: number; +} + +export type ExpressionWidgetProps = { legacyQuery: StructuredQuery; query?: Lib.Query; stageIndex?: number; @@ -54,7 +64,7 @@ export interface ExpressionWidgetProps { ) => void; onRemoveExpression?: (name: string) => void; onClose?: () => void; -} +} & (QueryProps | LegacyQueryProps); export const ExpressionWidget = (props: ExpressionWidgetProps): JSX.Element => { const { From 7cf0bb7fe90cee8a7ea506e58aac0b380d3e8178 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 18:36:22 +0700 Subject: [PATCH 82/96] Use getNotebookStep --- e2e/test/scenarios/question/notebook.cy.spec.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/e2e/test/scenarios/question/notebook.cy.spec.js b/e2e/test/scenarios/question/notebook.cy.spec.js index b46a934dafd4c..13f5e67fffc3a 100644 --- a/e2e/test/scenarios/question/notebook.cy.spec.js +++ b/e2e/test/scenarios/question/notebook.cy.spec.js @@ -172,9 +172,7 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { cy.button("Done").click(); - cy.findByTestId("step-filter-0-0") - .contains("Price is greater than 1") - .click(); + getNotebookStep("filter").contains("Price is greater than 1").click(); // change the corresponding custom expression cy.get(".Icon-chevronleft").click(); @@ -183,12 +181,10 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { cy.get("@formula").clear().type("[Price] > 1 AND [Price] < 5{enter}"); - cy.findByTestId("step-filter-0-0") + getNotebookStep("filter") .contains("Price is greater than 1") .should("exist"); - cy.findByTestId("step-filter-0-0") - .contains("Price is less than 5") - .should("exist"); + getNotebookStep("filter").contains("Price is less than 5").should("exist"); }); it("should show the real number of rows instead of HARD_ROW_LIMIT when loading (metabase#17397)", () => { @@ -312,7 +308,7 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { cy.button("Done").should("not.be.disabled").click(); - cy.findByTestId("step-filter-0-0").contains("Example").should("exist"); + getNotebookStep("filter").contains("Example").should("exist"); visualize(); From e6a0bed8dac2a2550be912aba500b99887c8a0a9 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 18:37:30 +0700 Subject: [PATCH 83/96] Simplify condition --- frontend/src/metabase-lib/expressions/process.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/metabase-lib/expressions/process.js b/frontend/src/metabase-lib/expressions/process.js index 74db6bf96d1ca..27c40b5c01318 100644 --- a/frontend/src/metabase-lib/expressions/process.js +++ b/frontend/src/metabase-lib/expressions/process.js @@ -43,7 +43,7 @@ export function processSource(options) { expression = adjustBooleans(resolve(parsed, startRule, resolveMBQLField)); // query and stageIndex are not available outside of notebook editor (e.g. in Metrics or Segments). - if (typeof query !== "undefined" && typeof stageIndex !== "undefined") { + if (query && typeof stageIndex !== "undefined") { expressionClause = Lib.expressionClauseForLegacyExpression( query, stageIndex, From 8780eeea9fdb54ff1ee4dcc6e54f0be8b5811a29 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 18:38:48 +0700 Subject: [PATCH 84/96] Simplify assertions --- .../components/expressions/ExpressionWidget.unit.spec.tsx | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index 72b9dee3aaf4c..18c3cab9121dd 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -93,9 +93,7 @@ describe("ExpressionWidget", () => { expect(onChangeClause).toHaveBeenCalledTimes(1); expect(onChangeClause).toHaveBeenCalledWith("", expect.anything()); - expect(getRecentExpressionClauseInfo()).toMatchObject({ - displayName: "1 + 1", - }); + expect(getRecentExpressionClauseInfo().displayName).toBe("1 + 1"); }); it(`should render interactive header if it is passed`, () => { @@ -179,9 +177,7 @@ describe("ExpressionWidget", () => { "Some n_am!e 2q$w&YzT(6i~#sLXv7+HjP}Ku1|9c*RlF@4o5N=e8;G*-bZ3/U0:Qa'V,t(W-_D", expect.anything(), ); - expect(getRecentExpressionClauseInfo()).toMatchObject({ - displayName: "1 + 1", - }); + expect(getRecentExpressionClauseInfo().displayName).toBe("1 + 1"); }); }); }); From 98ab29f796d22439ef04f025b9d5ba63c14ebc07 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 18:39:43 +0700 Subject: [PATCH 85/96] Use .lastCall --- .../components/expressions/ExpressionWidget.unit.spec.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx index 18c3cab9121dd..47148b8fa95c8 100644 --- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx @@ -204,7 +204,7 @@ function setup(additionalProps?: Partial) { function getRecentExpressionClause() { expect(onChangeClause).toHaveBeenCalled(); - const [_name, clause] = onChangeClause.mock.calls.at(-1); + const [_name, clause] = onChangeClause.mock.lastCall; return clause; } From 34b87a2a8fa08668da40e78a438d9326eb2914a0 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 18:41:02 +0700 Subject: [PATCH 86/96] Remove isNamed attribute from ColumnDisplayInfo --- frontend/src/metabase-lib/types.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/frontend/src/metabase-lib/types.ts b/frontend/src/metabase-lib/types.ts index ede973f74cbd4..6f9d70b561512 100644 --- a/frontend/src/metabase-lib/types.ts +++ b/frontend/src/metabase-lib/types.ts @@ -101,7 +101,6 @@ export type ColumnDisplayInfo = { name: string; displayName: string; longDisplayName: string; - isNamed: boolean; fkReferenceName?: string; isCalculated: boolean; @@ -134,8 +133,10 @@ export type MetricDisplayInfo = { export type ClauseDisplayInfo = Pick< ColumnDisplayInfo, - "name" | "displayName" | "longDisplayName" | "isNamed" | "table" ->; + "name" | "displayName" | "longDisplayName" | "table" +> & { + isNamed: boolean; +}; export type AggregationClauseDisplayInfo = ClauseDisplayInfo; From dd0da115ba04081844434066dbc37507ce93f4c1 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 18:58:29 +0700 Subject: [PATCH 87/96] Use .lastCall --- .../AggregationPicker/AggregationPicker.unit.spec.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index cdcf43b1fac2e..f3170b87c4d59 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -167,7 +167,7 @@ function setup({ function getRecentClause() { expect(onSelect).toHaveBeenCalled(); - const [clause] = onSelect.mock.calls.at(-1); + const [clause] = onSelect.mock.lastCall; return clause; } From 8d14a11ae81b5c30b59be20a016c1a768c26a5b7 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 19:17:37 +0700 Subject: [PATCH 88/96] Fix typing --- frontend/src/metabase-lib/aggregation.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/metabase-lib/aggregation.ts b/frontend/src/metabase-lib/aggregation.ts index 9ee2fb3cbbacd..4858ace125623 100644 --- a/frontend/src/metabase-lib/aggregation.ts +++ b/frontend/src/metabase-lib/aggregation.ts @@ -1,6 +1,6 @@ import * as ML from "cljs/metabase.lib.js"; import type { - Aggregatable, + Aggregable, AggregationClause, AggregationOperator, ColumnMetadata, @@ -30,7 +30,7 @@ export function selectedAggregationOperators( export function aggregate( query: Query, stageIndex: number, - clause: Aggregatable, + clause: Aggregable, ): Query { return ML.aggregate(query, stageIndex, clause); } From 3ef1bcb9a124396d4a330c43da35a10e7a92b623 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 19:27:32 +0700 Subject: [PATCH 89/96] Bring back legacyQuery prop to AggregationPicker --- .../AggregationPicker/AggregationPicker.tsx | 10 +++------- .../AggregationPicker.unit.spec.tsx | 9 ++++++++- .../notebook/steps/AggregateStep/AggregateStep.tsx | 6 ++++++ .../metabase/query_builder/components/view/View.jsx | 2 ++ .../AddAggregationButton/AddAggregationButton.tsx | 4 ++++ .../AggregationItem/AggregationItem.tsx | 4 ++++ .../sidebars/SummarizeSidebar/SummarizeSidebar.tsx | 12 ++++++++++++ .../SummarizeSidebar/SummarizeSidebar.unit.spec.tsx | 6 ++++++ 8 files changed, 45 insertions(+), 8 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 368002007ab6a..60e9b403c5431 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -12,8 +12,7 @@ import { ExpressionWidget } from "metabase/query_builder/components/expressions/ import { ExpressionWidgetHeader } from "metabase/query_builder/components/expressions/ExpressionWidgetHeader"; import * as Lib from "metabase-lib"; -import StructuredQuery from "metabase-lib/queries/StructuredQuery"; -import Question from "metabase-lib/Question"; +import type StructuredQuery from "metabase-lib/queries/StructuredQuery"; import { QueryColumnPicker } from "../QueryColumnPicker"; import { @@ -34,6 +33,7 @@ interface AggregationPickerProps { stageIndex: number; operators: Lib.AggregationOperator[]; hasExpressionInput?: boolean; + legacyQuery: StructuredQuery; maxHeight?: number; onSelect: (operator: Lib.Aggregable) => void; onClose?: () => void; @@ -63,6 +63,7 @@ function isOperatorListItem(item: ListItem): item is OperatorListItem { export function AggregationPicker({ className, query, + legacyQuery, clause, stageIndex, operators, @@ -80,11 +81,6 @@ export function AggregationPicker({ isEditingExpression, { turnOn: openExpressionEditor, turnOff: closeExpressionEditor }, ] = useToggle(isExpressionEditorInitiallyOpen(displayInfo, initialOperator)); - const datasetQuery = Lib.toLegacyQuery(query); - const legacyQuery = new StructuredQuery( - new Question({ dataset_query: datasetQuery }, metadata), - datasetQuery, - ); // For really simple inline expressions like Average([Price]), // MLv2 can figure out that "Average" operator is used. diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index f3170b87c4d59..16c66267aa202 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -4,12 +4,13 @@ import { createMockMetadata } from "__support__/metadata"; import { renderWithProviders, screen, within } from "__support__/ui"; import { checkNotNull } from "metabase/lib/types"; -import type { Metric } from "metabase-types/api"; +import type { Metric, StructuredDatasetQuery } from "metabase-types/api"; import { createMockMetric, COMMON_DATABASE_FEATURES, } from "metabase-types/api/mocks"; import { + createAdHocCard, createSampleDatabase, createOrdersTable, createPeopleTable, @@ -22,7 +23,9 @@ import { SAMPLE_DB_ID, } from "metabase-types/api/mocks/presets"; import * as Lib from "metabase-lib"; +import Question from "metabase-lib/Question"; import type Metadata from "metabase-lib/metadata/Metadata"; +import type StructuredQuery from "metabase-lib/queries/StructuredQuery"; import { createQuery, columnFinder, @@ -144,6 +147,9 @@ function setup({ query = createQuery({ metadata }), hasExpressionInput = true, }: SetupOpts = {}) { + const dataset_query = Lib.toLegacyQuery(query) as StructuredDatasetQuery; + const question = new Question(createAdHocCard({ dataset_query }), metadata); + const legacyQuery = question.query() as StructuredQuery; const stageIndex = 0; const clause = Lib.aggregations(query, stageIndex)[0]; @@ -157,6 +163,7 @@ function setup({ renderWithProviders( ( void; onAddAggregation: (aggregation: Lib.Aggregable) => void; + legacyQuery: StructuredQuery; clauseIndex?: number; // Implicitly passed by metabase/components/Triggerable @@ -103,6 +107,7 @@ function AggregationPopover({ stageIndex, clause, clauseIndex, + legacyQuery, onAddAggregation, onUpdateAggregation, onClose, @@ -119,6 +124,7 @@ function AggregationPopover({ return ( { const datesetQuery = Lib.toLegacyQuery(nextQuery); const nextQuestion = question.setDatasetQuery(datesetQuery); diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx index db51348307e6e..6f577cdc35cf3 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.tsx @@ -2,6 +2,7 @@ import { t } from "ttag"; import Tooltip from "metabase/core/components/Tooltip"; import TippyPopoverWithTrigger from "metabase/components/PopoverWithTrigger/TippyPopoverWithTrigger"; import * as Lib from "metabase-lib"; +import type StructuredQuery from "metabase-lib/queries/StructuredQuery"; import { AggregationPicker } from "../SummarizeSidebar.styled"; import { AddAggregationButtonRoot } from "./AddAggregationButton.styled"; @@ -9,11 +10,13 @@ const STAGE_INDEX = -1; interface AddAggregationButtonProps { query: Lib.Query; + legacyQuery: StructuredQuery; onAddAggregation: (aggregation: Lib.Aggregable) => void; } export function AddAggregationButton({ query, + legacyQuery, onAddAggregation, }: AddAggregationButtonProps) { const hasAggregations = Lib.aggregations(query, STAGE_INDEX).length > 0; @@ -40,6 +43,7 @@ export function AddAggregationButton({ popoverContent={({ closePopover }) => ( void; onRemove: () => void; } @@ -15,6 +17,7 @@ interface AggregationItemProps { export function AggregationItem({ query, aggregation, + legacyQuery, onUpdate, onRemove, }: AggregationItemProps) { @@ -44,6 +47,7 @@ export function AggregationItem({ stageIndex={STAGE_INDEX} operators={operators} hasExpressionInput={false} + legacyQuery={legacyQuery} onSelect={nextAggregation => { onUpdate(nextAggregation); closePopover(); diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx index 9c657e17a4309..11e37b41d6769 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.tsx @@ -4,6 +4,7 @@ import { t } from "ttag"; import { color } from "metabase/lib/colors"; import * as Lib from "metabase-lib"; +import type StructuredQuery from "metabase-lib/queries/StructuredQuery"; import { AddAggregationButton } from "./AddAggregationButton"; import { AggregationItem } from "./AggregationItem"; @@ -20,6 +21,7 @@ const STAGE_INDEX = -1; interface SummarizeSidebarProps { className?: string; query: Lib.Query; + legacyQuery: StructuredQuery; onQueryChange: (query: Lib.Query) => void; onClose: () => void; } @@ -27,6 +29,7 @@ interface SummarizeSidebarProps { export function SummarizeSidebar({ className, query: initialQuery, + legacyQuery: initialLegacyQuery, onQueryChange, onClose, }: SummarizeSidebarProps) { @@ -41,6 +44,13 @@ export function SummarizeSidebar({ const aggregations = Lib.aggregations(query, STAGE_INDEX); const hasAggregations = aggregations.length > 0; + const legacyQuery = useMemo(() => { + const question = initialLegacyQuery.question(); + return question + .setDatasetQuery(Lib.toLegacyQuery(query)) + .query() as StructuredQuery; + }, [query, initialLegacyQuery]); + const handleAddAggregation = useCallback( (aggregation: Lib.Aggregable) => { const nextQuery = Lib.aggregate(query, STAGE_INDEX, aggregation); @@ -131,6 +141,7 @@ export function SummarizeSidebar({ } query={query} aggregation={aggregation} + legacyQuery={legacyQuery} onUpdate={nextAggregation => handleUpdateAggregation(aggregation, nextAggregation) } @@ -139,6 +150,7 @@ export function SummarizeSidebar({ ))} diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.unit.spec.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.unit.spec.tsx index 38ab5aeb66b0a..e3a543738a50a 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeSidebar.unit.spec.tsx @@ -16,6 +16,7 @@ import { } from "metabase-types/api/mocks/presets"; import * as Lib from "metabase-lib"; import Question from "metabase-lib/Question"; +import type StructuredQuery from "metabase-lib/queries/StructuredQuery"; import { SummarizeSidebar } from "./SummarizeSidebar"; type SetupOpts = { @@ -60,9 +61,14 @@ function setup({ function Wrapper() { const [query, setQuery] = useState(question._getMLv2Query()); + const legacyQuery = question + .setDatasetQuery(Lib.toLegacyQuery(query)) + .query() as StructuredQuery; + return ( { setQuery(nextQuery); onQueryChange(nextQuery); From da95422f9814749c46a5d2c178ad0f74a2e0b235 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 19:36:53 +0700 Subject: [PATCH 90/96] Fix isExpressionEditorInitiallyOpen for new clauses --- .../AggregationPicker/AggregationPicker.tsx | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx index 60e9b403c5431..d4c8cb999b8c9 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx @@ -80,7 +80,9 @@ export function AggregationPicker({ const [ isEditingExpression, { turnOn: openExpressionEditor, turnOff: closeExpressionEditor }, - ] = useToggle(isExpressionEditorInitiallyOpen(displayInfo, initialOperator)); + ] = useToggle( + isExpressionEditorInitiallyOpen(query, stageIndex, clause, operators), + ); // For really simple inline expressions like Average([Price]), // MLv2 can figure out that "Average" operator is used. @@ -315,10 +317,18 @@ function getInitialOperator( } function isExpressionEditorInitiallyOpen( - displayInfo: Lib.ClauseDisplayInfo | undefined, - initialOperator: Lib.AggregationOperator | null, + query: Lib.Query, + stageIndex: number, + clause: Lib.AggregationClause | undefined, + operators: Lib.AggregationOperator[], ): boolean { + if (!clause) { + return false; + } + + const initialOperator = getInitialOperator(query, stageIndex, operators); const isCustomExpression = initialOperator === null; + const displayInfo = Lib.displayInfo(query, stageIndex, clause); const hasCustomName = Boolean(displayInfo?.isNamed); return isCustomExpression || hasCustomName; From 277766a3df0fc8247c863bb6517c0a4772206dd0 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 20:09:58 +0700 Subject: [PATCH 91/96] Fix unit tests --- .../AggregationPicker.unit.spec.tsx | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx index 16c66267aa202..88e6718015ea8 100644 --- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx +++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx @@ -1,6 +1,7 @@ import _ from "underscore"; import userEvent from "@testing-library/user-event"; import { createMockMetadata } from "__support__/metadata"; +import { createMockEntitiesState } from "__support__/store"; import { renderWithProviders, screen, within } from "__support__/ui"; import { checkNotNull } from "metabase/lib/types"; @@ -22,6 +23,8 @@ import { PRODUCTS, SAMPLE_DB_ID, } from "metabase-types/api/mocks/presets"; +import type { State } from "metabase-types/store"; +import { createMockState } from "metabase-types/store/mocks"; import * as Lib from "metabase-lib"; import Question from "metabase-lib/Question"; import type Metadata from "metabase-lib/metadata/Metadata"; @@ -137,12 +140,18 @@ function createMetadata({ } type SetupOpts = { - query?: Lib.Query; + state?: State; metadata?: Metadata; + query?: Lib.Query; hasExpressionInput?: boolean; }; function setup({ + state = createMockState({ + entities: createMockEntitiesState({ + databases: [createSampleDatabase()], + }), + }), metadata = createMetadata(), query = createQuery({ metadata }), hasExpressionInput = true, @@ -170,6 +179,7 @@ function setup({ hasExpressionInput={hasExpressionInput} onSelect={onSelect} />, + { storeInitialState: state }, ); function getRecentClause() { @@ -399,8 +409,7 @@ describe("AggregationPicker", () => { describe("custom expressions", () => { it("should allow to enter a custom expression", async () => { - const { query, stageIndex, getRecentClause, getRecentClauseInfo } = - setup(); + const { getRecentClauseInfo } = setup(); const expression = "1 + 1"; const expressionName = "My expression"; @@ -409,11 +418,7 @@ describe("AggregationPicker", () => { userEvent.type(screen.getByLabelText("Expression"), expression); userEvent.type(screen.getByLabelText("Name"), expressionName); userEvent.click(screen.getByRole("button", { name: "Done" })); - - expect(getRecentClauseInfo()).toMatchObject({ displayName: expression }); - expect( - Lib.displayInfo(query, stageIndex, getRecentClause()).displayName, - ).toBe(expressionName); + expect(getRecentClauseInfo().displayName).toBe(expressionName); }); it("should open the editor when a named expression without operator is used", async () => { @@ -431,7 +436,22 @@ describe("AggregationPicker", () => { }); it("shouldn't be available if database doesn't support custom expressions", () => { - setup({ metadata: createMetadata({ hasExpressionSupport: false }) }); + setup({ + state: createMockState({ + entities: createMockEntitiesState({ + databases: [ + { + ...createSampleDatabase(), + features: _.without( + COMMON_DATABASE_FEATURES, + "expression-aggregations", + ), + }, + ], + }), + }), + metadata: createMetadata({ hasExpressionSupport: false }), + }); expect(screen.queryByText("Custom Expression")).not.toBeInTheDocument(); }); From e582696701f01979c7d676693dea492589bf457c Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 20:51:22 +0700 Subject: [PATCH 92/96] Remove wrong assertion --- e2e/test/scenarios/admin/datamodel/metrics.cy.spec.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/e2e/test/scenarios/admin/datamodel/metrics.cy.spec.js b/e2e/test/scenarios/admin/datamodel/metrics.cy.spec.js index befb7cf7fc559..25043e2617b28 100644 --- a/e2e/test/scenarios/admin/datamodel/metrics.cy.spec.js +++ b/e2e/test/scenarios/admin/datamodel/metrics.cy.spec.js @@ -121,8 +121,6 @@ describe("scenarios > admin > datamodel > metrics", () => { cy.button("Done").click(); - cy.findByTestId("aggregate-step").contains("Foo").should("exist"); - cy.wait("@dataset"); // The test should fail on this step first From db821e025cabc01f9a95587a04239d15c483504c Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 21:00:10 +0700 Subject: [PATCH 93/96] Fix assertion --- e2e/test/scenarios/question/notebook.cy.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/test/scenarios/question/notebook.cy.spec.js b/e2e/test/scenarios/question/notebook.cy.spec.js index 3de8e5de36e39..3c222c7f8dcf8 100644 --- a/e2e/test/scenarios/question/notebook.cy.spec.js +++ b/e2e/test/scenarios/question/notebook.cy.spec.js @@ -308,7 +308,7 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { cy.button("Done").should("not.be.disabled").click(); - getNotebookStep("filter").contains("Example").should("exist"); + getNotebookStep("expression").contains("Example").should("exist"); visualize(); From 254913dd85d7aa1059ca712e19e4e9f1e901dc8e Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 21:25:40 +0700 Subject: [PATCH 94/96] Trigger a change --- e2e/test/scenarios/question/notebook.cy.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/test/scenarios/question/notebook.cy.spec.js b/e2e/test/scenarios/question/notebook.cy.spec.js index 3c222c7f8dcf8..56b16abb38d4b 100644 --- a/e2e/test/scenarios/question/notebook.cy.spec.js +++ b/e2e/test/scenarios/question/notebook.cy.spec.js @@ -561,7 +561,7 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { openTable({ database: WRITABLE_DB_ID, table: tableId, - mode: "notebook", + mode: "notebook2", }); }, ); From a76248aa0389888f727f839f281952ea43a501e9 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 21:25:51 +0700 Subject: [PATCH 95/96] Revert "Trigger a change" This reverts commit 254913dd85d7aa1059ca712e19e4e9f1e901dc8e. --- e2e/test/scenarios/question/notebook.cy.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/test/scenarios/question/notebook.cy.spec.js b/e2e/test/scenarios/question/notebook.cy.spec.js index 56b16abb38d4b..3c222c7f8dcf8 100644 --- a/e2e/test/scenarios/question/notebook.cy.spec.js +++ b/e2e/test/scenarios/question/notebook.cy.spec.js @@ -561,7 +561,7 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => { openTable({ database: WRITABLE_DB_ID, table: tableId, - mode: "notebook2", + mode: "notebook", }); }, ); From 13571b176931726797de7210f7cbed613c6c7d07 Mon Sep 17 00:00:00 2001 From: Kamil Mielnik Date: Mon, 4 Dec 2023 21:34:20 +0700 Subject: [PATCH 96/96] Fix assertion --- ...aggregation-with-implicit-joins-and-nested-query.cy.spec.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/e2e/test/scenarios/joins/reproductions/20519-cannot-join-on-aggregation-with-implicit-joins-and-nested-query.cy.spec.js b/e2e/test/scenarios/joins/reproductions/20519-cannot-join-on-aggregation-with-implicit-joins-and-nested-query.cy.spec.js index 41aa68cc7ac0e..f167c384f8154 100644 --- a/e2e/test/scenarios/joins/reproductions/20519-cannot-join-on-aggregation-with-implicit-joins-and-nested-query.cy.spec.js +++ b/e2e/test/scenarios/joins/reproductions/20519-cannot-join-on-aggregation-with-implicit-joins-and-nested-query.cy.spec.js @@ -2,6 +2,7 @@ import { restore, enterCustomColumnDetails, visualize, + getNotebookStep, } from "e2e/support/helpers"; import { SAMPLE_DB_ID } from "e2e/support/cypress_data"; @@ -55,7 +56,7 @@ describe("issue 20519", () => { cy.button("Done").click(); - cy.findByTestId("aggregate-step").contains("Two").should("exist"); + getNotebookStep("expression", { stage: 1 }).contains("Two").should("exist"); visualize(response => { expect(response.body.error).not.to.exist;