From f596f0b00f3670d5624d23f7eafe0499c0cdc2ed Mon Sep 17 00:00:00 2001
From: Kamil Mielnik <kamil@kamilmielnik.com>
Date: Mon, 13 May 2024 18:04:20 +0700
Subject: [PATCH] Cover `Offset()` in aggregations with e2e tests (#42455)
* Add repro for 32323
* Fix offset not working in case
* Make offset function return any
* Add a repro for #42377
* Fix order of adjustments
* Revert unrelated changes
* Remove commented code
* Revert unrelated changes
* Refactor test
* Type offset aggregation
* Add a test for no order by or breakout clause
* Add a basic test for breakout clause
* Fix assertion
* Remove problematic dependency
* Fix uuids generation
* Remove redundant limit
* Add a test for multiple breakouts
* Update test names
* Extract OFFSET_SUM_TOTAL_AGGREGATION
* Add a repro for metabase#42509
* Add a test for multiple aggregations and breakouts
* Remove unused intercept
* Move uuid utils to separate file
- it wasn't possible to import the utils file in e2e tests without it
* Make isUuid a type guard
* Add tests for sorting
* Tag the test
* Add extra assertion to verify expression parsing
* Add a complex scenario
* Reverse isFirst logic
---
e2e/test/scenarios/question/offset.cy.spec.ts | 380 +++++++++++++++++-
frontend/src/metabase-types/api/query.ts | 9 +-
.../widgets/CustomGeoJSONWidget.jsx | 2 +-
frontend/src/metabase/dashboard/utils.ts | 5 +-
frontend/src/metabase/lib/utils.ts | 30 --
frontend/src/metabase/lib/uuid.ts | 29 ++
.../TagEditorHelp/TagEditorHelp.tsx | 2 +-
.../SmartScalarSettingsWidgets.tsx | 9 +-
.../visualizations/SmartScalar/utils.ts | 2 +-
9 files changed, 413 insertions(+), 55 deletions(-)
create mode 100644 frontend/src/metabase/lib/uuid.ts
diff --git a/e2e/test/scenarios/question/offset.cy.spec.ts b/e2e/test/scenarios/question/offset.cy.spec.ts
index f20370dc1e3..76ac383fc08 100644
--- a/e2e/test/scenarios/question/offset.cy.spec.ts
+++ b/e2e/test/scenarios/question/offset.cy.spec.ts
@@ -1,43 +1,395 @@
import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
-import type { StructuredQuestionDetails } from "e2e/support/helpers";
import {
createQuestion,
+ echartsContainer,
enterCustomColumnDetails,
getNotebookStep,
+ modal,
openNotebook,
popover,
restore,
+ startNewQuestion,
+ visitQuestion,
+ visualize,
} from "e2e/support/helpers";
+import { uuid } from "metabase/lib/uuid";
+import type {
+ Aggregation,
+ Breakout,
+ FieldReference,
+ StructuredQuery,
+} from "metabase-types/api";
-const { ORDERS_ID } = SAMPLE_DATABASE;
+const { ORDERS, ORDERS_ID, PRODUCTS } = SAMPLE_DATABASE;
+
+const ORDERS_TOTAL_FIELD_REF: FieldReference = [
+ "field",
+ ORDERS.TOTAL,
+ { "base-type": "type/Float" },
+];
+
+const ORDERS_CREATED_AT_BREAKOUT: Breakout = [
+ "field",
+ ORDERS.CREATED_AT,
+ { "base-type": "type/DateTime", "temporal-unit": "month" },
+];
+
+const PRODUCTS_CATEGORY_BREAKOUT: Breakout = [
+ "field",
+ PRODUCTS.CATEGORY,
+ { "base-type": "type/text", "source-field": ORDERS.PRODUCT_ID },
+];
+
+const SUM_TOTAL_AGGREGATION: Aggregation = ["sum", ORDERS_TOTAL_FIELD_REF];
+
+const OFFSET_SUM_TOTAL_AGGREGATION_NAME = "Offsetted sum of total";
+
+const OFFSET_SUM_TOTAL_AGGREGATION: Aggregation = [
+ "offset",
+ createOffsetOptions(OFFSET_SUM_TOTAL_AGGREGATION_NAME),
+ SUM_TOTAL_AGGREGATION,
+ -1,
+];
describe("scenarios > question > offset", () => {
beforeEach(() => {
restore();
cy.signInAsAdmin();
+ cy.intercept("POST", "/api/card").as("saveQuestion");
+ });
+
+ it("does not work without a breakout", () => {
+ const query: StructuredQuery = {
+ "source-table": ORDERS_ID,
+ aggregation: [OFFSET_SUM_TOTAL_AGGREGATION],
+ };
+
+ createQuestion({ query }, { visitQuestion: true });
+
+ verifyQuestionError(
+ "Window function requires either breakouts or order by in the query",
+ );
+ });
+
+ it("works with a single breakout", () => {
+ const query: StructuredQuery = {
+ "source-table": ORDERS_ID,
+ aggregation: [OFFSET_SUM_TOTAL_AGGREGATION],
+ breakout: [ORDERS_CREATED_AT_BREAKOUT],
+ limit: 5,
+ };
+
+ createQuestion({ query }, { visitQuestion: true });
+
+ verifyNoQuestionError();
+ verifyTableContent([
+ ["April 2022", ""],
+ ["May 2022", "52.76"],
+ ]);
+ });
+
+ it("works with a single breakout and sorting by breakout", () => {
+ const query: StructuredQuery = {
+ "source-table": ORDERS_ID,
+ aggregation: [OFFSET_SUM_TOTAL_AGGREGATION],
+ breakout: [ORDERS_CREATED_AT_BREAKOUT],
+ limit: 5,
+ "order-by": [["desc", ORDERS_CREATED_AT_BREAKOUT]],
+ };
+
+ createQuestion({ query }, { visitQuestion: true });
+
+ verifyNoQuestionError();
+ verifyTableContent([
+ ["April 2026", "45,683.68"],
+ ["March 2026", "47,403.97"],
+ ]);
+ });
+
+ it.skip("works with a single breakout and sorting by aggregation (metabase#42554)", () => {
+ const query: StructuredQuery = {
+ "source-table": ORDERS_ID,
+ aggregation: [OFFSET_SUM_TOTAL_AGGREGATION],
+ breakout: [ORDERS_CREATED_AT_BREAKOUT],
+ limit: 5,
+ "order-by": [["desc", ["aggregation", 0]]],
+ };
+
+ createQuestion({ query }, { visitQuestion: true });
+
+ verifyNoQuestionError();
+ /* TODO: assert actual values */
+ // verifyTableContent([[]]);
+ });
+
+ it("works with multiple breakouts", () => {
+ const query: StructuredQuery = {
+ "source-table": ORDERS_ID,
+ aggregation: [OFFSET_SUM_TOTAL_AGGREGATION],
+ breakout: [ORDERS_CREATED_AT_BREAKOUT, PRODUCTS_CATEGORY_BREAKOUT],
+ };
+
+ createQuestion({ query }, { visitQuestion: true });
+
+ verifyNoQuestionError();
+ verifyTableContent([
+ ["April 2022", "", "", "", ""],
+ ["May 2022", "", "52.76", "", ""],
+ ["June 2022", "339.14", "203.57", "493.51", "229.5"],
+ ]);
+ });
+
+ it.skip("works with multiple breakouts and a limit (metabase#42509)", () => {
+ const query: StructuredQuery = {
+ "source-table": ORDERS_ID,
+ aggregation: [OFFSET_SUM_TOTAL_AGGREGATION],
+ breakout: [ORDERS_CREATED_AT_BREAKOUT, PRODUCTS_CATEGORY_BREAKOUT],
+ limit: 5,
+ };
+
+ createQuestion({ query }, { visitQuestion: true });
+
+ verifyNoQuestionError();
+ verifyTableContent([
+ ["April 2022", "", "", "", ""],
+ ["May 2022", "", "52.76", "", ""],
+ ["June 2022", "339.14", "203.57", "493.51", "229.5"],
+ ]);
+ });
+
+ it("works with multiple aggregations and breakouts", () => {
+ const query: StructuredQuery = {
+ "source-table": ORDERS_ID,
+ aggregation: [SUM_TOTAL_AGGREGATION, OFFSET_SUM_TOTAL_AGGREGATION],
+ breakout: [ORDERS_CREATED_AT_BREAKOUT, PRODUCTS_CATEGORY_BREAKOUT],
+ limit: 9,
+ };
+
+ createQuestion({ query }, { visitQuestion: true });
+
+ verifyNoQuestionError();
+ verifyTableContent([
+ ["April 2022", "Gadget", "52.76", ""],
+ ["May 2022", "Doohickey", "339.14", ""],
+ ["May 2022", "Gadget", "203.57", "52.76"],
+ ["May 2022", "Gizmo", "493.51", ""],
+ ["May 2022", "Widget", "229.5", ""],
+ ["June 2022", "Doohickey", "482.56", "339.14"],
+ ["June 2022", "Gadget", "515.53", "203.57"],
+ ["June 2022", "Gizmo", "387.79", "493.51"],
+ ["June 2022", "Widget", "687.06", "229.5"],
+ ]);
+ });
+
+ it("works after saving a question (metabase#42323)", () => {
+ const breakoutName = "Created At";
+
+ startNewQuestion();
+ popover().within(() => {
+ cy.findByText("Raw Data").click();
+ cy.findByText("Orders").click();
+ });
+ addCustomAggregation({
+ formula: "Offset(Sum([Total]), -1)",
+ name: OFFSET_SUM_TOTAL_AGGREGATION_NAME,
+ isFirst: true,
+ });
+ addBreakout(breakoutName);
+
+ visualize();
+ verifyNoQuestionError();
+ verifyLineChart({
+ xAxis: breakoutName,
+ yAxis: OFFSET_SUM_TOTAL_AGGREGATION_NAME,
+ });
+
+ saveQuestion().then(({ response }) => {
+ visitQuestion(response?.body.id);
+ verifyNoQuestionError();
+ verifyLineChart({
+ xAxis: breakoutName,
+ yAxis: OFFSET_SUM_TOTAL_AGGREGATION_NAME,
+ });
+ });
+ });
+
+ it("works in a complex real-life scenario", () => {
+ const breakoutName = "Created At";
+ const totalSalesName = "Total sales (this month)";
+ const totalSalesLastMonthName = "Total sales (last month)";
+ const percentSalesGrowthName = "Percent sales growth over last month";
+ const rollingTotalName = "Rolling total of sales last 3 months";
+ const rollingAverageName = "Rolling average of sales last 3 months";
+ const legendItems = [
+ totalSalesName,
+ totalSalesLastMonthName,
+ percentSalesGrowthName,
+ rollingTotalName,
+ rollingAverageName,
+ ];
+
+ startNewQuestion();
+ popover().within(() => {
+ cy.findByText("Raw Data").click();
+ cy.findByText("Orders").click();
+ });
+ addCustomAggregation({
+ formula: "Sum([Total])",
+ name: totalSalesName,
+ isFirst: true,
+ });
+ addCustomAggregation({
+ formula: "Offset(Sum([Total]), -1)",
+ name: totalSalesLastMonthName,
+ });
+ addCustomAggregation({
+ formula: "Sum([Total]) / Offset(Sum([Total]), -1) - 1",
+ name: percentSalesGrowthName,
+ });
+ addCustomAggregation({
+ formula:
+ "Sum([Total]) + Offset(Sum([Total]), -1) + Offset(Sum([Total]), -2)",
+ name: rollingTotalName,
+ });
+ addCustomAggregation({
+ formula:
+ "(Sum([Total]) + Offset(Sum([Total]), -1) + Offset(Sum([Total]), -2)) / 3",
+ name: rollingAverageName,
+ });
+ addBreakout(breakoutName);
+
+ visualize();
+ verifyNoQuestionError();
+ verifyLineChart({
+ xAxis: breakoutName,
+ legendItems,
+ });
+
+ saveQuestion().then(({ response }) => {
+ visitQuestion(response?.body.id);
+ verifyNoQuestionError();
+ verifyLineChart({
+ xAxis: breakoutName,
+ legendItems,
+ });
+ });
});
it("should allow using OFFSET as a CASE argument (metabase#42377)", () => {
const formula = "Sum(case([Total] > 0, Offset([Total], -1)))";
const name = "Aggregation";
- const questionDetails: StructuredQuestionDetails = {
- query: {
- "source-table": ORDERS_ID,
- limit: 5,
- },
+ const query: StructuredQuery = {
+ "source-table": ORDERS_ID,
};
- createQuestion(questionDetails, { visitQuestion: true });
- openNotebook();
+ createQuestion({ query }, { visitQuestion: true });
+ openNotebook();
cy.icon("sum").click();
- getNotebookStep("summarize")
- .findByText("Pick the metric you want to see")
- .click();
- popover().contains("Custom Expression").click();
- enterCustomColumnDetails({ formula, name });
+ addCustomAggregation({ formula, name, isFirst: true });
+
+ cy.findAllByTestId("notebook-cell-item").contains(name).click();
+ cy.findByTestId("expression-editor-textfield").should("contain", formula);
cy.on("uncaught:exception", error => {
expect(error.message.includes("Error normalizing")).not.to.be.true;
});
});
});
+
+function addCustomAggregation({
+ formula,
+ name,
+ isFirst,
+}: {
+ formula: string;
+ name: string;
+ isFirst?: boolean;
+}) {
+ if (isFirst) {
+ getNotebookStep("summarize")
+ .findByText("Pick the metric you want to see")
+ .click();
+ } else {
+ getNotebookStep("summarize").icon("add").click();
+ }
+
+ popover().contains("Custom Expression").click();
+ enterCustomColumnDetails({ formula, name });
+ popover().button("Done").click();
+}
+
+function addBreakout(name: string) {
+ getNotebookStep("summarize").findByText("Pick a column to group by").click();
+ popover().findByText(name).click();
+}
+
+function saveQuestion() {
+ cy.button("Save").click();
+ modal().button("Save").click();
+ return cy.wait("@saveQuestion");
+}
+
+function verifyLineChart({
+ xAxis,
+ yAxis,
+ legendItems,
+}: {
+ xAxis: string;
+ yAxis?: string;
+ legendItems?: string[];
+}) {
+ echartsContainer().within(() => {
+ cy.findByText(xAxis).should("be.visible");
+
+ if (yAxis) {
+ cy.findByText(yAxis).should("be.visible");
+ }
+ });
+
+ if (legendItems) {
+ for (const legendItem of legendItems) {
+ cy.findAllByTestId("legend-item").contains(legendItem).should("exist");
+ }
+ }
+}
+
+function verifyTableContent(rows: string[][]) {
+ const columnsCount = rows[0].length;
+ const pairs = rows.flatMap((row, rowIndex) => {
+ return row.map((text, cellIndex) => {
+ const index = rowIndex * columnsCount + cellIndex;
+ return { index, text };
+ });
+ });
+
+ for (const { index, text } of pairs) {
+ verifyTableCellContent(index, text);
+ }
+}
+
+function verifyTableCellContent(index: number, text: string) {
+ cy.findAllByRole("gridcell").eq(index).should("have.text", text);
+}
+
+function verifyQuestionError(error: string) {
+ cy.findByTestId("query-builder-main").within(() => {
+ cy.findByText("There was a problem with your question").should("exist");
+ cy.findByText("Show error details").click();
+ cy.findByText(error).should("exist");
+ });
+}
+
+function verifyNoQuestionError() {
+ cy.findByTestId("query-builder-main").within(() => {
+ cy.findByText("There was a problem with your question").should("not.exist");
+ cy.findByText("Show error details").should("not.exist");
+ });
+}
+
+function createOffsetOptions(name = "offset") {
+ return {
+ "lib/uuid": uuid(),
+ name,
+ "display-name": name,
+ };
+}
diff --git a/frontend/src/metabase-types/api/query.ts b/frontend/src/metabase-types/api/query.ts
index d009da26dce..15ad7636331 100644
--- a/frontend/src/metabase-types/api/query.ts
+++ b/frontend/src/metabase-types/api/query.ts
@@ -155,6 +155,12 @@ type StdDevAgg = ["stddev", ConcreteFieldReference];
type SumAgg = ["sum", ConcreteFieldReference];
type MinAgg = ["min", ConcreteFieldReference];
type MaxAgg = ["max", ConcreteFieldReference];
+type OffsetAgg = [
+ "offset",
+ { "lib/uuid": string; name: string; "display-name": string },
+ Aggregation,
+ number,
+];
type CommonAggregation =
| CountAgg
@@ -166,7 +172,8 @@ type CommonAggregation =
| StdDevAgg
| SumAgg
| MinAgg
- | MaxAgg;
+ | MaxAgg
+ | OffsetAgg;
type MetricAgg = ["metric", MetricId];
diff --git a/frontend/src/metabase/admin/settings/components/widgets/CustomGeoJSONWidget.jsx b/frontend/src/metabase/admin/settings/components/widgets/CustomGeoJSONWidget.jsx
index 3b0f107c125..926c295dc7d 100644
--- a/frontend/src/metabase/admin/settings/components/widgets/CustomGeoJSONWidget.jsx
+++ b/frontend/src/metabase/admin/settings/components/widgets/CustomGeoJSONWidget.jsx
@@ -12,7 +12,7 @@ import Select, { Option } from "metabase/core/components/Select";
import AdminS from "metabase/css/admin.module.css";
import ButtonsS from "metabase/css/components/buttons.module.css";
import CS from "metabase/css/core/index.css";
-import { uuid } from "metabase/lib/utils";
+import { uuid } from "metabase/lib/uuid";
import { SettingsApi, GeoJSONApi } from "metabase/services";
import LeafletChoropleth from "metabase/visualizations/components/LeafletChoropleth";
diff --git a/frontend/src/metabase/dashboard/utils.ts b/frontend/src/metabase/dashboard/utils.ts
index 5d8b70c0c49..14c6856be4a 100644
--- a/frontend/src/metabase/dashboard/utils.ts
+++ b/frontend/src/metabase/dashboard/utils.ts
@@ -3,7 +3,8 @@ import _ from "underscore";
import { IS_EMBED_PREVIEW } from "metabase/lib/embed";
import { SERVER_ERROR_TYPES } from "metabase/lib/errors";
-import { isUUID, isJWT } from "metabase/lib/utils";
+import { isJWT } from "metabase/lib/utils";
+import { isUuid } from "metabase/lib/uuid";
import {
getGenericErrorMessage,
getPermissionErrorMessage,
@@ -182,7 +183,7 @@ export function getDashboardType(id: unknown) {
if (id == null || typeof id === "object") {
// HACK: support inline dashboards
return "inline";
- } else if (isUUID(id)) {
+ } else if (isUuid(id)) {
return "public";
} else if (isJWT(id)) {
return "embed";
diff --git a/frontend/src/metabase/lib/utils.ts b/frontend/src/metabase/lib/utils.ts
index a7dc02eb659..830fafaa9b3 100644
--- a/frontend/src/metabase/lib/utils.ts
+++ b/frontend/src/metabase/lib/utils.ts
@@ -6,12 +6,6 @@ import { PLUGIN_IS_EE_BUILD } from "metabase/plugins";
const EMAIL_REGEX =
/^(([^<>()[\]\\.,;:\s@\"]+(\.[^<>()[\]\\.,;:\s@\"]+)*)|(\".+\"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/;
-function s4() {
- return Math.floor((1 + Math.random()) * 0x10000)
- .toString(16)
- .substring(1);
-}
-
export function isEmpty(str: string | null) {
if (str != null) {
str = String(str);
@@ -41,30 +35,6 @@ export function numberToWord(num: number) {
}
}
-export function uuid() {
- return (
- s4() +
- s4() +
- "-" +
- s4() +
- "-" +
- s4() +
- "-" +
- s4() +
- "-" +
- s4() +
- s4() +
- s4()
- );
-}
-
-export function isUUID(uuid: unknown) {
- return (
- typeof uuid === "string" &&
- /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/.test(uuid)
- );
-}
-
export function isJWT(string: unknown) {
return (
typeof string === "string" &&
diff --git a/frontend/src/metabase/lib/uuid.ts b/frontend/src/metabase/lib/uuid.ts
new file mode 100644
index 00000000000..d9c1e40418d
--- /dev/null
+++ b/frontend/src/metabase/lib/uuid.ts
@@ -0,0 +1,29 @@
+export function uuid() {
+ return (
+ s4() +
+ s4() +
+ "-" +
+ s4() +
+ "-" +
+ s4() +
+ "-" +
+ s4() +
+ "-" +
+ s4() +
+ s4() +
+ s4()
+ );
+}
+
+function s4() {
+ return Math.floor((1 + Math.random()) * 0x10000)
+ .toString(16)
+ .substring(1);
+}
+
+export function isUuid(uuid: unknown): uuid is string {
+ return (
+ typeof uuid === "string" &&
+ /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/.test(uuid)
+ );
+}
diff --git a/frontend/src/metabase/query_builder/components/template_tags/TagEditorHelp/TagEditorHelp.tsx b/frontend/src/metabase/query_builder/components/template_tags/TagEditorHelp/TagEditorHelp.tsx
index 6e86de5fc0f..7ed4865bc82 100644
--- a/frontend/src/metabase/query_builder/components/template_tags/TagEditorHelp/TagEditorHelp.tsx
+++ b/frontend/src/metabase/query_builder/components/template_tags/TagEditorHelp/TagEditorHelp.tsx
@@ -7,7 +7,7 @@ import ExternalLink from "metabase/core/components/ExternalLink";
import CS from "metabase/css/core/index.css";
import { useSelector } from "metabase/lib/redux";
import MetabaseSettings from "metabase/lib/settings";
-import { uuid } from "metabase/lib/utils";
+import { uuid } from "metabase/lib/uuid";
import { getShowMetabaseLinks } from "metabase/selectors/whitelabel";
import type Database from "metabase-lib/v1/metadata/Database";
import type { DatabaseId, NativeDatasetQuery } from "metabase-types/api";
diff --git a/frontend/src/metabase/visualizations/visualizations/SmartScalar/SettingsComponents/SmartScalarSettingsWidgets.tsx b/frontend/src/metabase/visualizations/visualizations/SmartScalar/SettingsComponents/SmartScalarSettingsWidgets.tsx
index f60ce9680d8..eb774a45a94 100644
--- a/frontend/src/metabase/visualizations/visualizations/SmartScalar/SettingsComponents/SmartScalarSettingsWidgets.tsx
+++ b/frontend/src/metabase/visualizations/visualizations/SmartScalar/SettingsComponents/SmartScalarSettingsWidgets.tsx
@@ -1,21 +1,20 @@
import type { DragEndEvent } from "@dnd-kit/core";
-import { DndContext, useSensor, PointerSensor } from "@dnd-kit/core";
+import { DndContext, PointerSensor, useSensor } from "@dnd-kit/core";
import {
- restrictToVerticalAxis,
restrictToParentElement,
+ restrictToVerticalAxis,
} from "@dnd-kit/modifiers";
import {
+ SortableContext,
arrayMove,
verticalListSortingStrategy,
- SortableContext,
} from "@dnd-kit/sortable";
import { useCallback } from "react";
import { usePreviousDistinct } from "react-use";
import { t } from "ttag";
-import _ from "underscore";
import { Sortable } from "metabase/core/components/Sortable";
-import { uuid } from "metabase/lib/utils";
+import { uuid } from "metabase/lib/uuid";
import { Stack } from "metabase/ui";
import type { DatasetColumn, SmartScalarComparison } from "metabase-types/api";
diff --git a/frontend/src/metabase/visualizations/visualizations/SmartScalar/utils.ts b/frontend/src/metabase/visualizations/visualizations/SmartScalar/utils.ts
index fa00f21a63a..28ff1ab4469 100644
--- a/frontend/src/metabase/visualizations/visualizations/SmartScalar/utils.ts
+++ b/frontend/src/metabase/visualizations/visualizations/SmartScalar/utils.ts
@@ -4,7 +4,7 @@ import _ from "underscore";
import { formatNumber } from "metabase/lib/formatting/numbers";
import { measureText } from "metabase/lib/measure-text";
-import { uuid } from "metabase/lib/utils";
+import { uuid } from "metabase/lib/uuid";
import { isEmpty } from "metabase/lib/validate";
import { isDate, isNumeric } from "metabase-lib/v1/types/utils/isa";
import type {
--
GitLab