From f0ac75a392b34bbbdca8bdb6aefe5c25e12b1fb4 Mon Sep 17 00:00:00 2001
From: Alexander Polyankin <alexander.polyankin@metabase.com>
Date: Thu, 5 Sep 2024 15:23:49 -0400
Subject: [PATCH] Metrics notebook step restrictions (#47368)
---
.../metrics/metrics-editing.cy.spec.js | 73 ++++----
.../notebook-link-to-data-source.cy.spec.ts | 9 -
.../DatasetEditor/DatasetEditor.jsx | 40 +++--
.../MetricEmptyState/MetricEmptyState.tsx | 38 ++++
.../MetricEmptyState.unit.spec.tsx | 40 +++++
.../DatasetEditor/MetricEmptyState/index.ts | 1 +
.../AggregateStep/AggregateStep.tsx | 12 +-
.../AggregateStep/AggregateStep.unit.spec.tsx | 91 ++++++----
.../components/BreakoutStep/BreakoutStep.tsx | 36 +++-
.../BreakoutStep/BreakoutStep.unit.spec.tsx | 165 +++++++++++++++---
.../components/ClauseStep/ClauseStep.tsx | 11 +-
.../notebook/components/DataStep/DataStep.tsx | 10 +-
.../DataStep/DataStep.unit.spec.tsx | 78 +++++++--
.../NotebookCell/NotebookCell.styled.tsx | 2 +-
.../test/__support__/server-mocks/database.ts | 2 +-
.../test/__support__/server-mocks/table.ts | 2 +-
16 files changed, 447 insertions(+), 163 deletions(-)
create mode 100644 frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/MetricEmptyState.tsx
create mode 100644 frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/MetricEmptyState.unit.spec.tsx
create mode 100644 frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/index.ts
diff --git a/e2e/test/scenarios/metrics/metrics-editing.cy.spec.js b/e2e/test/scenarios/metrics/metrics-editing.cy.spec.js
index 3da11423ad6..e040f7e4a84 100644
--- a/e2e/test/scenarios/metrics/metrics-editing.cy.spec.js
+++ b/e2e/test/scenarios/metrics/metrics-editing.cy.spec.js
@@ -130,9 +130,9 @@ describe("scenarios > metrics > editing", () => {
);
openQuestionActions();
popover().findByText("Edit metric definition").click();
- addBreakout({ tableName: "Product", columnName: "Category" });
+ addBreakout({ tableName: "Product", columnName: "Created At" });
updateMetric();
- verifyLineAreaBarChart({ xAxis: "Product → Category", yAxis: "Count" });
+ verifyLineAreaBarChart({ xAxis: "Product → Created At", yAxis: "Count" });
});
it("should be able to change the query definition of a metric based on a model", () => {
@@ -141,9 +141,9 @@ describe("scenarios > metrics > editing", () => {
);
openQuestionActions();
popover().findByText("Edit metric definition").click();
- addBreakout({ tableName: "Product", columnName: "Category" });
+ addBreakout({ tableName: "Product", columnName: "Created At" });
updateMetric();
- verifyLineAreaBarChart({ xAxis: "Product → Category", yAxis: "Count" });
+ verifyLineAreaBarChart({ xAxis: "Created At", yAxis: "Count" });
});
it("should pin new metrics automatically", () => {
@@ -154,7 +154,6 @@ describe("scenarios > metrics > editing", () => {
entityPickerModalTab("Tables").click();
cy.findByText("Orders").click();
});
- addAggregation({ operatorName: "Count of rows" });
saveMetric({ name: "New metric" });
cy.findByTestId("head-crumbs-container")
@@ -180,7 +179,6 @@ describe("scenarios > metrics > editing", () => {
columnName: "Category",
values: ["Gadget"],
});
- addAggregation({ operatorName: "Count of rows" });
saveMetric();
verifyScalarValue("4,939");
});
@@ -196,7 +194,6 @@ describe("scenarios > metrics > editing", () => {
columnName: "Category",
values: ["Gadget"],
});
- addAggregation({ operatorName: "Count of rows" });
saveMetric();
verifyScalarValue("4,939");
});
@@ -213,7 +210,6 @@ describe("scenarios > metrics > editing", () => {
minValue: 5,
maxValue: 100,
});
- addAggregation({ operatorName: "Count of rows" });
saveMetric();
verifyScalarValue("5");
});
@@ -229,7 +225,6 @@ describe("scenarios > metrics > editing", () => {
columnName: "Category",
values: ["Gadget"],
});
- addAggregation({ operatorName: "Count of rows" });
saveMetric();
verifyScalarValue("4,939");
});
@@ -246,7 +241,6 @@ describe("scenarios > metrics > editing", () => {
minValue: 5,
maxValue: 100,
});
- addAggregation({ operatorName: "Count of rows" });
saveMetric();
verifyScalarValue("5");
});
@@ -257,9 +251,20 @@ describe("scenarios > metrics > editing", () => {
entityPickerModalTab("Models").click();
cy.findByText("Orders Model").click();
});
- addAggregation({ operatorName: "Count of rows" });
getActionButton("Summarize").should("not.exist");
});
+
+ it("should allow to run the query from the metric empty state", () => {
+ startNewMetric();
+ entityPickerModal().within(() => {
+ entityPickerModalTab("Tables").click();
+ cy.findByText("Orders").click();
+ });
+ cy.intercept("POST", "/api/dataset").as("dataset");
+ cy.findByTestId("metric-empty-state").button("Visualize").click();
+ cy.wait("@dataset");
+ verifyScalarValue("18,760");
+ });
});
describe("joins", () => {
@@ -281,7 +286,6 @@ describe("scenarios > metrics > editing", () => {
cy.findByText("CA").click();
cy.button("Add filter").click();
});
- addAggregation({ operatorName: "Count of rows" });
saveMetric();
verifyScalarValue("613");
});
@@ -327,7 +331,11 @@ describe("scenarios > metrics > editing", () => {
name: "Total2",
});
popover().button("Done").click();
- addAggregation({ operatorName: "Sum of ...", columnName: "Total2" });
+ getNotebookStep("summarize").findByText("Count").click();
+ popover().within(() => {
+ cy.findByText("Sum of ...").click();
+ cy.findByText("Total2").click();
+ });
saveMetric();
verifyScalarValue("755,310.84");
});
@@ -344,7 +352,11 @@ describe("scenarios > metrics > editing", () => {
name: "Price2",
});
popover().button("Done").click();
- addAggregation({ operatorName: "Average of ...", columnName: "Price2" });
+ getNotebookStep("summarize").findByText("Count").click();
+ popover().within(() => {
+ cy.findByText("Average of ...").click();
+ cy.findByText("Price2").click();
+ });
saveMetric();
verifyScalarValue("111.38");
});
@@ -357,37 +369,18 @@ describe("scenarios > metrics > editing", () => {
entityPickerModalTab("Tables").click();
cy.findByText("Orders").click();
});
- addAggregation({ operatorName: "Sum of ...", columnName: "Total" });
+ getNotebookStep("summarize").findByText("Count").click();
+ popover().within(() => {
+ cy.findByText("Sum of ...").click();
+ cy.findByText("Total").click();
+ });
addBreakout({ columnName: "Created At" });
saveMetric();
verifyLineAreaBarChart({ xAxis: "Created At", yAxis: "Sum of Total" });
});
-
- it("should create a geo metric with multiple breakouts", () => {
- startNewMetric();
- entityPickerModal().within(() => {
- entityPickerModalTab("Tables").click();
- cy.findByText("People").click();
- });
- addAggregation({ operatorName: "Count of rows" });
- addBreakout({ columnName: "Latitude" });
- addBreakout({ columnName: "Longitude" });
- saveMetric();
- verifyPinMap();
- });
});
describe("aggregations", () => {
- it("should not be possible to save a metric without an aggregation clause", () => {
- startNewMetric();
- entityPickerModal().within(() => {
- entityPickerModalTab("Tables").click();
- cy.findByText("Orders").click();
- });
- cy.button("Save").should("be.disabled");
- cy.findByTestId("run-button").should("not.be.visible");
- });
-
it("should create a metric with a custom aggregation expression based on 1 metric", () => {
createQuestion(ORDERS_SCALAR_METRIC);
startNewMetric();
@@ -623,7 +616,3 @@ function verifyLineAreaBarChart({ xAxis, yAxis }) {
cy.findByText(xAxis).should("be.visible");
});
}
-
-function verifyPinMap() {
- cy.get("[data-element-id=pin-map]").should("exist");
-}
diff --git a/e2e/test/scenarios/question/notebook-link-to-data-source.cy.spec.ts b/e2e/test/scenarios/question/notebook-link-to-data-source.cy.spec.ts
index 09750e8bdad..eb29e375887 100644
--- a/e2e/test/scenarios/question/notebook-link-to-data-source.cy.spec.ts
+++ b/e2e/test/scenarios/question/notebook-link-to-data-source.cy.spec.ts
@@ -635,9 +635,6 @@ describe("scenarios > notebook > link to data source", () => {
cy.log("Model should open in a new tab");
getNotebookStep("join", { stage: 0, index: 0 }).within(() => {
- // Clicking on a left join cell does not have any effect
- cy.findByLabelText("Left table").click(clickConfig);
-
cy.findByLabelText("Right table")
.should("have.text", "Orders Model")
.click(clickConfig);
@@ -663,9 +660,6 @@ describe("scenarios > notebook > link to data source", () => {
cy.log("Saved question should open in a new tab");
getNotebookStep("join", { stage: 0, index: 1 }).within(() => {
- // Clicking on a left join cell does not have any effect
- cy.findByLabelText("Left table").click(clickConfig);
-
cy.findByLabelText("Right table")
.should("have.text", savedQuestion.name)
.click(clickConfig);
@@ -690,9 +684,6 @@ describe("scenarios > notebook > link to data source", () => {
(function testRawTable() {
cy.log("Raw table should open in a new tab");
getNotebookStep("join", { stage: 0, index: 2 }).within(() => {
- // Clicking on a left join cell does not have any effect
- cy.findByLabelText("Left table").click(clickConfig);
-
cy.findByLabelText("Right table")
.should("have.text", "Reviews")
.click(clickConfig);
diff --git a/frontend/src/metabase/query_builder/components/DatasetEditor/DatasetEditor.jsx b/frontend/src/metabase/query_builder/components/DatasetEditor/DatasetEditor.jsx
index a712e539303..a1afccd513b 100644
--- a/frontend/src/metabase/query_builder/components/DatasetEditor/DatasetEditor.jsx
+++ b/frontend/src/metabase/query_builder/components/DatasetEditor/DatasetEditor.jsx
@@ -54,6 +54,7 @@ import {
import DatasetFieldMetadataSidebar from "./DatasetFieldMetadataSidebar";
import DatasetQueryEditor from "./DatasetQueryEditor";
import { EditorTabs } from "./EditorTabs";
+import { MetricEmptyState } from "./MetricEmptyState";
import { TabHintToast } from "./TabHintToast";
import { EDITOR_TAB_INDEXES } from "./constants";
@@ -200,6 +201,7 @@ function DatasetEditor(props) {
height,
isDirty: isModelQueryDirty,
isResultDirty,
+ isRunning,
setQueryBuilderMode,
runQuestionQuery,
setDatasetEditorTab,
@@ -213,6 +215,7 @@ function DatasetEditor(props) {
} = props;
const isMetric = question.type() === "metric";
+ const isMetricEmptyState = isMetric && !result && !isRunning;
const { isNative } = Lib.queryDisplayInfo(question.query());
const isDirty = isModelQueryDirty || isMetadataDirty;
const [showCancelEditWarning, setShowCancelEditWarning] = useState(false);
@@ -456,11 +459,7 @@ function DatasetEditor(props) {
) {
return t`You must run the query before you can save this model`;
}
-
- if (isMetric && Lib.aggregations(question.query(), -1).length === 0) {
- return t`You must define how the measure is calculated to save this metric`;
- }
- }, [isNative, isMetric, isDirty, isResultDirty, question]);
+ }, [isNative, isDirty, isResultDirty, question]);
const sidebar = getSidebar(
{ ...props, modelIndexes },
@@ -542,18 +541,25 @@ function DatasetEditor(props) {
</QueryEditorContainer>
<TableContainer isSidebarOpen={!!sidebar}>
<DebouncedFrame className={cx(CS.flexFull)} enabled>
- <QueryVisualization
- {...props}
- className={CS.spread}
- noHeader
- queryBuilderMode="dataset"
- isShowingDetailsOnlyColumns={datasetEditorTab === "metadata"}
- hasMetadataPopovers={false}
- handleVisualizationClick={handleTableElementClick}
- tableHeaderHeight={isEditingMetadata && TABLE_HEADER_HEIGHT}
- renderTableHeaderWrapper={renderTableHeaderWrapper}
- scrollToColumn={focusedFieldIndex + scrollToColumnModifier}
- />
+ {isMetricEmptyState ? (
+ <MetricEmptyState
+ query={question.query()}
+ runQuestionQuery={runQuestionQuery}
+ />
+ ) : (
+ <QueryVisualization
+ {...props}
+ className={CS.spread}
+ noHeader
+ queryBuilderMode="dataset"
+ isShowingDetailsOnlyColumns={datasetEditorTab === "metadata"}
+ hasMetadataPopovers={false}
+ handleVisualizationClick={handleTableElementClick}
+ tableHeaderHeight={isEditingMetadata && TABLE_HEADER_HEIGHT}
+ renderTableHeaderWrapper={renderTableHeaderWrapper}
+ scrollToColumn={focusedFieldIndex + scrollToColumnModifier}
+ />
+ )}
</DebouncedFrame>
<TabHintToastContainer
isVisible={isEditingMetadata && isTabHintVisible && !result.error}
diff --git a/frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/MetricEmptyState.tsx b/frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/MetricEmptyState.tsx
new file mode 100644
index 00000000000..20552f92b26
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/MetricEmptyState.tsx
@@ -0,0 +1,38 @@
+import { t } from "ttag";
+
+import { Button, Flex, Text } from "metabase/ui";
+import * as Lib from "metabase-lib";
+
+type MetricEmptyStateProps = {
+ query: Lib.Query;
+ runQuestionQuery: () => void;
+};
+
+export function MetricEmptyState({
+ query,
+ runQuestionQuery,
+}: MetricEmptyStateProps) {
+ const canRun = Lib.canRun(query, "metric");
+
+ return (
+ <Flex
+ direction="column"
+ align="center"
+ justify="center"
+ mih="100%"
+ data-testid="metric-empty-state"
+ >
+ <Text mb="sm" fw="bold" fz="lg">
+ {t`A metric is one of the key numbers you want to keep track of`}
+ </Text>
+ <Text mb="lg" c="text-medium" maw="28.75rem" align="center">
+ {t`To create one, you’ll need to define how its calculated, add any required filters, and optionally pick the main dimension for your metric.`}
+ </Text>
+ {canRun && (
+ <Button variant="filled" onClick={() => runQuestionQuery()}>
+ {t`Visualize`}
+ </Button>
+ )}
+ </Flex>
+ );
+}
diff --git a/frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/MetricEmptyState.unit.spec.tsx b/frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/MetricEmptyState.unit.spec.tsx
new file mode 100644
index 00000000000..cc61904904f
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/MetricEmptyState.unit.spec.tsx
@@ -0,0 +1,40 @@
+import userEvent from "@testing-library/user-event";
+
+import { renderWithProviders, screen } from "__support__/ui";
+import type * as Lib from "metabase-lib";
+import { createQuery, createQueryWithClauses } from "metabase-lib/test-helpers";
+
+import { MetricEmptyState } from "./MetricEmptyState";
+
+type SetupOpts = {
+ query: Lib.Query;
+};
+
+function setup({ query }: SetupOpts) {
+ const runQuestionQuery = jest.fn();
+
+ renderWithProviders(
+ <MetricEmptyState query={query} runQuestionQuery={runQuestionQuery} />,
+ );
+
+ return { runQuestionQuery };
+}
+
+describe("MetricEmptyState", () => {
+ it("should allow to run a valid metric query", async () => {
+ const query = createQueryWithClauses({
+ aggregations: [{ operatorName: "count" }],
+ });
+ const { runQuestionQuery } = setup({ query });
+ await userEvent.click(screen.getByRole("button", { name: "Visualize" }));
+ expect(runQuestionQuery).toHaveBeenCalled();
+ });
+
+ it("should not allow to run an invalid metric query", () => {
+ const query = createQuery();
+ setup({ query });
+ expect(
+ screen.queryByRole("button", { name: "Visualize" }),
+ ).not.toBeInTheDocument();
+ });
+});
diff --git a/frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/index.ts b/frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/index.ts
new file mode 100644
index 00000000000..d57e00af170
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/DatasetEditor/MetricEmptyState/index.ts
@@ -0,0 +1 @@
+export * from "./MetricEmptyState";
diff --git a/frontend/src/metabase/querying/notebook/components/AggregateStep/AggregateStep.tsx b/frontend/src/metabase/querying/notebook/components/AggregateStep/AggregateStep.tsx
index 826c6f1f120..6522b837426 100644
--- a/frontend/src/metabase/querying/notebook/components/AggregateStep/AggregateStep.tsx
+++ b/frontend/src/metabase/querying/notebook/components/AggregateStep/AggregateStep.tsx
@@ -15,12 +15,16 @@ export function AggregateStep({
readOnly,
updateQuery,
}: NotebookStepProps) {
- const { stageIndex } = step;
+ const { question, stageIndex } = step;
+ const isMetric = question.type() === "metric";
- const clauses = useMemo(() => {
+ const aggregations = useMemo(() => {
return Lib.aggregations(query, stageIndex);
}, [query, stageIndex]);
+ const hasAddButton = !readOnly && (!isMetric || aggregations.length === 0);
+ const hasRemoveButton = !readOnly && !isMetric;
+
const handleReorderAggregation = (
sourceClause: Lib.AggregationClause,
targetClause: Lib.AggregationClause,
@@ -44,11 +48,13 @@ export function AggregateStep({
return (
<ClauseStep
- items={clauses}
+ items={aggregations}
initialAddText={t`Pick the metric you want to see`}
readOnly={readOnly}
color={color}
isLastOpened={isLastOpened}
+ hasAddButton={hasAddButton}
+ hasRemoveButton={hasRemoveButton}
renderName={renderAggregationName}
renderPopover={({ item: aggregation, index, onClose }) => (
<AggregationPopover
diff --git a/frontend/src/metabase/querying/notebook/components/AggregateStep/AggregateStep.unit.spec.tsx b/frontend/src/metabase/querying/notebook/components/AggregateStep/AggregateStep.unit.spec.tsx
index fe50b7f3e16..37afa296b08 100644
--- a/frontend/src/metabase/querying/notebook/components/AggregateStep/AggregateStep.unit.spec.tsx
+++ b/frontend/src/metabase/querying/notebook/components/AggregateStep/AggregateStep.unit.spec.tsx
@@ -1,38 +1,37 @@
import userEvent from "@testing-library/user-event";
-import { getIcon, renderWithProviders, screen } from "__support__/ui";
-import * as Lib from "metabase-lib";
import {
- columnFinder,
- createQuery,
- findAggregationOperator,
-} from "metabase-lib/test-helpers";
+ getIcon,
+ queryIcon,
+ renderWithProviders,
+ screen,
+} from "__support__/ui";
+import * as Lib from "metabase-lib";
+import { createQueryWithClauses } from "metabase-lib/test-helpers";
import { createMockCard } from "metabase-types/api/mocks";
import {
createMockQueryBuilderState,
createMockState,
} from "metabase-types/store/mocks";
-import { createMockNotebookStep } from "../../test-utils";
+import { DEFAULT_QUESTION, createMockNotebookStep } from "../../test-utils";
+import type { NotebookStep } from "../../types";
import { AggregateStep } from "./AggregateStep";
-function createAggregatedQuery({
- table = "ORDERS",
- column = "QUANTITY",
-}: { table?: string; column?: string } = {}) {
- const initialQuery = createQuery();
- const average = findAggregationOperator(initialQuery, "avg");
- const findColumn = columnFinder(
- initialQuery,
- Lib.aggregationOperatorColumns(average),
- );
- const quantity = findColumn(table, column);
- const clause = Lib.aggregationClause(average, quantity);
- return Lib.aggregate(initialQuery, 0, clause);
+function createAggregatedQuery() {
+ return createQueryWithClauses({
+ aggregations: [
+ { operatorName: "avg", tableName: "ORDERS", columnName: "QUANTITY" },
+ ],
+ });
+}
+
+interface SetupOpts {
+ step?: NotebookStep;
}
-function setup(step = createMockNotebookStep()) {
+function setup({ step = createMockNotebookStep() }: SetupOpts = {}) {
const updateQuery = jest.fn();
renderWithProviders(
@@ -81,19 +80,24 @@ describe("AggregateStep", () => {
});
it("should render correctly with an aggregation", () => {
- setup(createMockNotebookStep({ query: createAggregatedQuery() }));
+ setup({ step: createMockNotebookStep({ query: createAggregatedQuery() }) });
expect(screen.getByText("Average of Quantity")).toBeInTheDocument();
});
it("should use foreign key name for foreign table columns", () => {
- setup(
- createMockNotebookStep({
- query: createAggregatedQuery({
- table: "PRODUCTS",
- column: "RATING",
+ setup({
+ step: createMockNotebookStep({
+ query: createQueryWithClauses({
+ aggregations: [
+ {
+ operatorName: "avg",
+ tableName: "PRODUCTS",
+ columnName: "RATING",
+ },
+ ],
}),
}),
- );
+ });
expect(screen.getByText("Average of Product → Rating")).toBeInTheDocument();
});
@@ -114,9 +118,9 @@ describe("AggregateStep", () => {
});
it("should change an aggregation operator", async () => {
- const { getNextQuery, getRecentAggregationClause } = setup(
- createMockNotebookStep({ query: createAggregatedQuery() }),
- );
+ const { getNextQuery, getRecentAggregationClause } = setup({
+ step: createMockNotebookStep({ query: createAggregatedQuery() }),
+ });
await userEvent.click(screen.getByText("Average of Quantity"));
await userEvent.click(screen.getByText("Average of ...")); // go back to operator selection
@@ -134,9 +138,9 @@ describe("AggregateStep", () => {
});
it("should change an aggregation column", async () => {
- const { getNextQuery, getRecentAggregationClause } = setup(
- createMockNotebookStep({ query: createAggregatedQuery() }),
- );
+ const { getNextQuery, getRecentAggregationClause } = setup({
+ step: createMockNotebookStep({ query: createAggregatedQuery() }),
+ });
await userEvent.click(screen.getByText("Average of Quantity"));
await userEvent.click(screen.getByText("Total"));
@@ -153,13 +157,26 @@ describe("AggregateStep", () => {
});
it("should remove an aggregation", async () => {
- const { getNextQuery } = setup(
- createMockNotebookStep({ query: createAggregatedQuery() }),
- );
+ const { getNextQuery } = setup({
+ step: createMockNotebookStep({ query: createAggregatedQuery() }),
+ });
await userEvent.click(getIcon("close"));
const nextQuery = getNextQuery();
expect(Lib.aggregations(nextQuery, 0)).toHaveLength(0);
});
+
+ describe("metrics", () => {
+ it("should not allow to remove an existing aggregation or add another one", () => {
+ const query = createAggregatedQuery();
+ const question = DEFAULT_QUESTION.setType("metric").setQuery(query);
+ const step = createMockNotebookStep({ question, query });
+ setup({ step });
+
+ expect(screen.getByText("Average of Quantity")).toBeInTheDocument();
+ expect(queryIcon("close")).not.toBeInTheDocument();
+ expect(queryIcon("add")).not.toBeInTheDocument();
+ });
+ });
});
diff --git a/frontend/src/metabase/querying/notebook/components/BreakoutStep/BreakoutStep.tsx b/frontend/src/metabase/querying/notebook/components/BreakoutStep/BreakoutStep.tsx
index 06e368838e6..3ca32da3cbe 100644
--- a/frontend/src/metabase/querying/notebook/components/BreakoutStep/BreakoutStep.tsx
+++ b/frontend/src/metabase/querying/notebook/components/BreakoutStep/BreakoutStep.tsx
@@ -15,11 +15,22 @@ export function BreakoutStep({
readOnly,
updateQuery,
}: NotebookStepProps) {
- const { stageIndex } = step;
+ const { question, stageIndex } = step;
+ const isMetric = question.type() === "metric";
- const breakouts = useMemo(() => {
- return Lib.breakouts(query, stageIndex);
- }, [query, stageIndex]);
+ const breakouts = useMemo(
+ () => Lib.breakouts(query, stageIndex),
+ [query, stageIndex],
+ );
+
+ const metricColumns = useMemo(() => {
+ return isMetric
+ ? Lib.breakoutableColumns(query, stageIndex).filter(Lib.isDateOrDateTime)
+ : [];
+ }, [query, stageIndex, isMetric]);
+
+ const hasAddButton = !readOnly && (!isMetric || breakouts.length === 0);
+ const isAddButtonDisabled = isMetric && metricColumns.length === 0;
const renderBreakoutName = (clause: Lib.BreakoutClause) =>
Lib.displayInfo(query, stageIndex, clause).longDisplayName;
@@ -58,10 +69,16 @@ export function BreakoutStep({
return (
<ClauseStep
items={breakouts}
- initialAddText={t`Pick a column to group by`}
+ initialAddText={
+ isAddButtonDisabled
+ ? t`No datetime columns available`
+ : t`Pick a column to group by`
+ }
readOnly={readOnly}
color={color}
isLastOpened={isLastOpened}
+ hasAddButton={hasAddButton}
+ isAddButtonDisabled={isAddButtonDisabled}
renderName={renderBreakoutName}
renderPopover={({ item: breakout, index, onClose }) => (
<BreakoutPopover
@@ -69,6 +86,7 @@ export function BreakoutStep({
stageIndex={stageIndex}
breakout={breakout}
breakoutIndex={index}
+ isMetric={isMetric}
onAddBreakout={handleAddBreakout}
onUpdateBreakoutColumn={handleUpdateBreakoutColumn}
onClose={onClose}
@@ -86,6 +104,7 @@ interface BreakoutPopoverProps {
stageIndex: number;
breakout: Lib.BreakoutClause | undefined;
breakoutIndex: number | undefined;
+ isMetric: boolean;
onAddBreakout: (column: Lib.ColumnMetadata) => void;
onUpdateBreakoutColumn: (
breakout: Lib.BreakoutClause,
@@ -99,6 +118,7 @@ const BreakoutPopover = ({
stageIndex,
breakout,
breakoutIndex,
+ isMetric,
onAddBreakout,
onUpdateBreakoutColumn,
onClose,
@@ -108,7 +128,9 @@ const BreakoutPopover = ({
const filteredColumns = columns.reduce(
(columns: Lib.ColumnMetadata[], column) => {
const columnInfo = Lib.displayInfo(query, stageIndex, column);
- if (breakout && checkColumnSelected(columnInfo, breakoutIndex)) {
+ if (isMetric && !Lib.isDateOrDateTime(column)) {
+ return columns;
+ } else if (breakout && checkColumnSelected(columnInfo, breakoutIndex)) {
columns.push(Lib.breakoutColumn(query, stageIndex, breakout));
} else {
columns.push(column);
@@ -118,7 +140,7 @@ const BreakoutPopover = ({
[],
);
return Lib.groupColumns(filteredColumns);
- }, [query, stageIndex, breakout, breakoutIndex]);
+ }, [query, stageIndex, breakout, breakoutIndex, isMetric]);
return (
<QueryColumnPicker
diff --git a/frontend/src/metabase/querying/notebook/components/BreakoutStep/BreakoutStep.unit.spec.tsx b/frontend/src/metabase/querying/notebook/components/BreakoutStep/BreakoutStep.unit.spec.tsx
index e465b09602a..251e7cb4b56 100644
--- a/frontend/src/metabase/querying/notebook/components/BreakoutStep/BreakoutStep.unit.spec.tsx
+++ b/frontend/src/metabase/querying/notebook/components/BreakoutStep/BreakoutStep.unit.spec.tsx
@@ -1,10 +1,26 @@
import userEvent from "@testing-library/user-event";
-import { fireEvent, getIcon, render, screen, within } from "__support__/ui";
+import { createMockMetadata } from "__support__/metadata";
+import {
+ fireEvent,
+ getIcon,
+ queryIcon,
+ render,
+ screen,
+ within,
+} from "__support__/ui";
import * as Lib from "metabase-lib";
import { createQueryWithClauses } from "metabase-lib/test-helpers";
+import Question from "metabase-lib/v1/Question";
+import { createMockCard } from "metabase-types/api/mocks";
+import {
+ createOrdersIdField,
+ createOrdersTable,
+ createSampleDatabase,
+} from "metabase-types/api/mocks/presets";
-import { createMockNotebookStep } from "../../test-utils";
+import { DEFAULT_QUESTION, createMockNotebookStep } from "../../test-utils";
+import type { NotebookStep } from "../../types";
import { BreakoutStep } from "./BreakoutStep";
@@ -74,7 +90,12 @@ function createQueryWithMultipleBreakoutsAndTemporalBucket() {
});
}
-function setup(step = createMockNotebookStep()) {
+interface SetupOpts {
+ step?: NotebookStep;
+ readOnly?: boolean;
+}
+
+function setup({ step = createMockNotebookStep(), readOnly }: SetupOpts = {}) {
const updateQuery = jest.fn();
render(
@@ -83,6 +104,7 @@ function setup(step = createMockNotebookStep()) {
stageIndex={step.stageIndex}
query={step.query}
color="summarize"
+ readOnly={readOnly}
isLastOpened={false}
reportTimezone="UTC"
updateQuery={updateQuery}
@@ -115,7 +137,7 @@ describe("BreakoutStep", () => {
it("should render a breakout correctly", async () => {
const query = createQueryWithBreakout();
- setup(createMockNotebookStep({ query }));
+ setup({ step: createMockNotebookStep({ query }) });
await userEvent.click(screen.getByText("Tax"));
@@ -136,7 +158,9 @@ describe("BreakoutStep", () => {
it("should change a breakout column", async () => {
const query = createQueryWithBreakout();
- const { getNextBreakouts } = setup(createMockNotebookStep({ query }));
+ const { getNextBreakouts } = setup({
+ step: createMockNotebookStep({ query }),
+ });
await userEvent.click(screen.getByText("Tax"));
await userEvent.click(await screen.findByText("Discount"));
@@ -147,7 +171,7 @@ describe("BreakoutStep", () => {
it("should remove a breakout", async () => {
const query = createQueryWithBreakout();
- const { getNextQuery } = setup(createMockNotebookStep({ query }));
+ const { getNextQuery } = setup({ step: createMockNotebookStep({ query }) });
await userEvent.click(getIcon("close"));
@@ -186,7 +210,7 @@ describe("BreakoutStep", () => {
it("should highlight selected binning strategy", async () => {
const query = createQueryWithBreakoutAndBinningStrategy();
- setup(createMockNotebookStep({ query }));
+ setup({ step: createMockNotebookStep({ query }) });
await userEvent.click(screen.getByText("Tax: 10 bins"));
const option = await screen.findByRole("option", { name: "Tax" });
@@ -199,7 +223,9 @@ describe("BreakoutStep", () => {
it("shouldn't update a query when clicking a selected binned column", async () => {
const query = createQueryWithBreakoutAndBinningStrategy();
- const { updateQuery } = setup(createMockNotebookStep({ query }));
+ const { updateQuery } = setup({
+ step: createMockNotebookStep({ query }),
+ });
await userEvent.click(screen.getByText("Tax: 10 bins"));
await userEvent.click(await screen.findByText("Tax"));
@@ -209,7 +235,7 @@ describe("BreakoutStep", () => {
it("should highlight the `Don't bin` option when a column is not binned", async () => {
const query = createQueryWithBreakoutAndBinningStrategy("Don't bin");
- setup(createMockNotebookStep({ query }));
+ setup({ step: createMockNotebookStep({ query }) });
await userEvent.click(screen.getByText("Tax"));
const option = await screen.findByRole("option", {
@@ -251,7 +277,7 @@ describe("BreakoutStep", () => {
it("should highlight selected temporal bucket", async () => {
const query = createQueryWithBreakoutAndTemporalBucket("Quarter");
- setup(createMockNotebookStep({ query }));
+ setup({ step: createMockNotebookStep({ query }) });
await userEvent.click(screen.getByText("Created At: Quarter"));
const option = await screen.findByRole("option", { name: "Created At" });
@@ -264,7 +290,7 @@ describe("BreakoutStep", () => {
it("should handle `Don't bin` option for temporal bucket (metabase#19684)", async () => {
const query = createQueryWithBreakoutAndTemporalBucket("Don't bin");
- setup(createMockNotebookStep({ query }));
+ setup({ step: createMockNotebookStep({ query }) });
await userEvent.click(screen.getByText("Created At"));
const option = await screen.findByRole("option", {
@@ -286,7 +312,9 @@ describe("BreakoutStep", () => {
it("shouldn't update a query when clicking a selected column with temporal bucketing", async () => {
const query = createQueryWithBreakoutAndTemporalBucket("Quarter");
- const { updateQuery } = setup(createMockNotebookStep({ query }));
+ const { updateQuery } = setup({
+ step: createMockNotebookStep({ query }),
+ });
await userEvent.click(screen.getByText("Created At: Quarter"));
await userEvent.click(await screen.findByText("Created At"));
@@ -296,7 +324,9 @@ describe("BreakoutStep", () => {
it("should allow to add a breakout for a column with an existing breakout but with a different binning strategy", async () => {
const query = createQueryWithBreakoutAndBinningStrategy("10 bins");
- const { getNextBreakouts } = setup(createMockNotebookStep({ query }));
+ const { getNextBreakouts } = setup({
+ step: createMockNotebookStep({ query }),
+ });
await userEvent.click(getIcon("add"));
const option = await screen.findByRole("option", { name: "Tax" });
@@ -311,7 +341,9 @@ describe("BreakoutStep", () => {
it("should ignore attempts to add a breakout for a column with the same binning strategy", async () => {
const query = createQueryWithBreakoutAndBinningStrategy("10 bins");
- const { getNextBreakouts } = setup(createMockNotebookStep({ query }));
+ const { getNextBreakouts } = setup({
+ step: createMockNotebookStep({ query }),
+ });
await userEvent.click(getIcon("add"));
const option = await screen.findByRole("option", { name: "Tax" });
@@ -325,7 +357,9 @@ describe("BreakoutStep", () => {
it("should allow to change a breakout for a column with an existing breakout but with a different binning strategy", async () => {
const query = createQueryWithMultipleBreakoutsAndBinningStrategy();
- const { getNextBreakouts } = setup(createMockNotebookStep({ query }));
+ const { getNextBreakouts } = setup({
+ step: createMockNotebookStep({ query }),
+ });
await userEvent.click(await screen.findByText("Tax: 10 bins"));
const option = await screen.findByRole("option", { name: "Tax" });
@@ -340,7 +374,9 @@ describe("BreakoutStep", () => {
it("should ignore attempts to create duplicate breakouts by changing the binning strategy for an existing breakout", async () => {
const query = createQueryWithMultipleBreakoutsAndBinningStrategy();
- const { getNextBreakouts } = setup(createMockNotebookStep({ query }));
+ const { getNextBreakouts } = setup({
+ step: createMockNotebookStep({ query }),
+ });
await userEvent.click(await screen.findByText("Tax: 10 bins"));
const option = await screen.findByRole("option", { name: "Tax" });
@@ -355,7 +391,9 @@ describe("BreakoutStep", () => {
it("should allow to remove a breakout for a column with an existing breakout but with a different binning strategy", async () => {
const query = createQueryWithMultipleBreakoutsAndBinningStrategy();
- const { getNextBreakouts } = setup(createMockNotebookStep({ query }));
+ const { getNextBreakouts } = setup({
+ step: createMockNotebookStep({ query }),
+ });
const clause = await screen.findByText("Tax: 50 bins");
await userEvent.click(within(clause).getByLabelText("close icon"));
@@ -367,7 +405,9 @@ describe("BreakoutStep", () => {
it("should allow to add a breakout for a column with an existing breakout but with a different temporal bucket", async () => {
const query = createQueryWithBreakoutAndTemporalBucket("Year");
- const { getNextBreakouts } = setup(createMockNotebookStep({ query }));
+ const { getNextBreakouts } = setup({
+ step: createMockNotebookStep({ query }),
+ });
await userEvent.click(getIcon("add"));
const option = await screen.findByRole("option", { name: "Created At" });
@@ -382,7 +422,9 @@ describe("BreakoutStep", () => {
it("should ignore attempts to add a breakout for a column with the same temporal bucket", async () => {
const query = createQueryWithBreakoutAndTemporalBucket("Year");
- const { getNextBreakouts } = setup(createMockNotebookStep({ query }));
+ const { getNextBreakouts } = setup({
+ step: createMockNotebookStep({ query }),
+ });
await userEvent.click(getIcon("add"));
const option = await screen.findByRole("option", { name: "Created At" });
@@ -396,7 +438,9 @@ describe("BreakoutStep", () => {
it("should allow to change a breakout for a column with an existing breakout but with a different temporal bucket", async () => {
const query = createQueryWithMultipleBreakoutsAndTemporalBucket();
- const { getNextBreakouts } = setup(createMockNotebookStep({ query }));
+ const { getNextBreakouts } = setup({
+ step: createMockNotebookStep({ query }),
+ });
await userEvent.click(await screen.findByText("Created At: Month"));
const option = await screen.findByRole("option", { name: "Created At" });
@@ -411,7 +455,9 @@ describe("BreakoutStep", () => {
it("should ignore attempts to create duplicate breakouts by changing the temporal bucket for an existing breakout", async () => {
const query = createQueryWithMultipleBreakoutsAndTemporalBucket();
- const { getNextBreakouts } = setup(createMockNotebookStep({ query }));
+ const { getNextBreakouts } = setup({
+ step: createMockNotebookStep({ query }),
+ });
await userEvent.click(await screen.findByText("Created At: Month"));
const option = await screen.findByRole("option", { name: "Created At" });
@@ -426,7 +472,9 @@ describe("BreakoutStep", () => {
it("should allow to remove a breakout for a column with an existing breakout but with a different temporal bucket", async () => {
const query = createQueryWithMultipleBreakoutsAndTemporalBucket();
- const { getNextBreakouts } = setup(createMockNotebookStep({ query }));
+ const { getNextBreakouts } = setup({
+ step: createMockNotebookStep({ query }),
+ });
const clause = await screen.findByText("Created At: Month");
await userEvent.click(within(clause).getByLabelText("close icon"));
@@ -436,4 +484,77 @@ describe("BreakoutStep", () => {
expect(breakouts[0].displayName).toBe("Created At: Year");
});
});
+
+ describe("metrics", () => {
+ it("should allow to select date and datetime columns only", async () => {
+ const question = DEFAULT_QUESTION.setType("metric");
+ const step = createMockNotebookStep({ question });
+ const { getNextBreakouts } = setup({ step });
+
+ await userEvent.click(screen.getByText("Pick a column to group by"));
+ expect(await screen.findByText("Order")).toBeInTheDocument();
+ expect(await screen.findByText("Created At")).toBeInTheDocument();
+ expect(screen.queryByText("Tax")).not.toBeInTheDocument();
+
+ await userEvent.click(screen.getByText("User"));
+ expect(await screen.findByText("Created At")).toBeInTheDocument();
+ expect(await screen.findByText("Birth Date")).toBeInTheDocument();
+ expect(screen.queryByText("Email")).not.toBeInTheDocument();
+
+ await userEvent.click(await screen.findByText("Created At"));
+ expect(getNextBreakouts()).toMatchObject([
+ { displayName: "Created At: Month" },
+ ]);
+ });
+
+ it("should not allow to select columns in readonly mode", () => {
+ const question = DEFAULT_QUESTION.setType("metric");
+ const step = createMockNotebookStep({ question });
+ setup({ step, readOnly: true });
+
+ expect(
+ screen.queryByText("Pick a column to group by"),
+ ).not.toBeInTheDocument();
+ expect(
+ screen.queryByText("No datetime columns available"),
+ ).not.toBeInTheDocument();
+ });
+
+ it("should not allow to select when there are no date or datetime columns", () => {
+ const metadata = createMockMetadata({
+ databases: [
+ createSampleDatabase({
+ tables: [createOrdersTable({ fields: [createOrdersIdField()] })],
+ }),
+ ],
+ });
+ const question = new Question(
+ createMockCard({ type: "metric" }),
+ metadata,
+ );
+ const step = createMockNotebookStep({
+ question,
+ query: question.query(),
+ });
+ setup({ step });
+
+ expect(
+ screen.getByText("No datetime columns available"),
+ ).toBeInTheDocument();
+ expect(
+ screen.queryByText("Pick a column to group by"),
+ ).not.toBeInTheDocument();
+ });
+
+ it("should not allow to add more than 1 breakout", async () => {
+ const query = createQueryWithClauses({
+ breakouts: [{ tableName: "ORDERS", columnName: "CREATED_AT" }],
+ });
+ const question = DEFAULT_QUESTION.setType("metric").setQuery(query);
+ const step = createMockNotebookStep({ question });
+ setup({ step });
+
+ expect(queryIcon("add")).not.toBeInTheDocument();
+ });
+ });
});
diff --git a/frontend/src/metabase/querying/notebook/components/ClauseStep/ClauseStep.tsx b/frontend/src/metabase/querying/notebook/components/ClauseStep/ClauseStep.tsx
index c8b8c64b46f..9cc0ee3840b 100644
--- a/frontend/src/metabase/querying/notebook/components/ClauseStep/ClauseStep.tsx
+++ b/frontend/src/metabase/querying/notebook/components/ClauseStep/ClauseStep.tsx
@@ -36,6 +36,9 @@ export type ClauseStepProps<T> = {
initialAddText?: string;
readOnly?: boolean;
isLastOpened?: boolean;
+ hasAddButton?: boolean;
+ isAddButtonDisabled?: boolean;
+ hasRemoveButton?: boolean;
renderName: (item: T, index: number) => JSX.Element | string;
renderPopover: (opts: RenderPopoverOpts<T>) => JSX.Element | null;
onRemove: (item: T, index: number) => void;
@@ -49,6 +52,9 @@ export const ClauseStep = <T,>({
initialAddText,
readOnly = false,
isLastOpened = false,
+ hasAddButton = !readOnly,
+ isAddButtonDisabled = false,
+ hasRemoveButton = !readOnly,
renderName,
renderPopover,
onRemove,
@@ -59,7 +65,7 @@ export const ClauseStep = <T,>({
<ClauseStepDndItem index={index} readOnly={readOnly}>
<NotebookCellItem color={color} readOnly={readOnly} onClick={onOpen}>
{renderName(item, index)}
- {!readOnly && (
+ {hasRemoveButton && (
<Icon
className={CS.ml1}
name="close"
@@ -77,6 +83,7 @@ export const ClauseStep = <T,>({
<NotebookCellAdd
initialAddText={items.length === 0 && initialAddText}
color={color}
+ disabled={isAddButtonDisabled}
onClick={onOpen}
/>
);
@@ -92,7 +99,7 @@ export const ClauseStep = <T,>({
/>
))}
</ClauseStepDndContext>
- {!readOnly && (
+ {hasAddButton && (
<ClausePopover
isInitiallyOpen={isLastOpened}
renderItem={onOpen => renderNewItem({ onOpen })}
diff --git a/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.tsx b/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.tsx
index 80f4c1e72a9..fb0bd60ec91 100644
--- a/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.tsx
+++ b/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.tsx
@@ -18,9 +18,10 @@ export const DataStep = ({
color,
updateQuery,
}: NotebookStepProps) => {
- const { stageIndex } = step;
+ const { question, stageIndex } = step;
const tableId = Lib.sourceTableOrCardId(query);
const table = tableId ? Lib.tableOrCardMetadata(query, tableId) : undefined;
+ const isMetric = question.type() === "metric";
const isRaw = useMemo(() => {
return (
@@ -36,7 +37,12 @@ export const DataStep = ({
metadataProvider: Lib.MetadataProvider,
) => {
const newQuery = Lib.queryFromTableOrCardMetadata(metadataProvider, table);
- await updateQuery(newQuery);
+ const newAggregations = Lib.aggregations(newQuery, stageIndex);
+ if (isMetric && newAggregations.length === 0) {
+ await updateQuery(Lib.aggregateByCount(newQuery, stageIndex));
+ } else {
+ await updateQuery(newQuery);
+ }
};
return (
diff --git a/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.unit.spec.tsx b/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.unit.spec.tsx
index ccafa71fa79..1558bbc6306 100644
--- a/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.unit.spec.tsx
+++ b/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.unit.spec.tsx
@@ -28,7 +28,8 @@ import {
createMockState,
} from "metabase-types/store/mocks";
-import { createMockNotebookStep } from "../../test-utils";
+import { DEFAULT_QUESTION, createMockNotebookStep } from "../../test-utils";
+import type { NotebookStep } from "../../types";
import { DataStep } from "./DataStep";
@@ -54,13 +55,17 @@ const createQueryWithBreakout = () => {
return Lib.breakout(query, 0, column);
};
-const setup = (
+interface SetupOpts {
+ step?: NotebookStep;
+ readOnly?: boolean;
+ isEmbeddingSdk?: boolean;
+}
+
+const setup = ({
step = createMockNotebookStep(),
- {
- readOnly = false,
- isEmbeddingSdk = false,
- }: { readOnly?: boolean; isEmbeddingSdk?: boolean } = {},
-) => {
+ readOnly = false,
+ isEmbeddingSdk = false,
+}: SetupOpts = {}) => {
const mockWindowOpen = jest.spyOn(window, "open").mockImplementation();
const updateQuery = jest.fn();
@@ -111,7 +116,7 @@ const setup = (
const setupEmptyQuery = () => {
const question = Question.create({ databaseId: SAMPLE_DB_ID });
const query = question.query();
- return setup(createMockNotebookStep({ query }));
+ return setup({ step: createMockNotebookStep({ query }) });
};
describe("DataStep", () => {
@@ -179,7 +184,7 @@ describe("DataStep", () => {
Lib.tableOrCardMetadata(metadataProvider, `card__${card.id}`),
);
const step = createMockNotebookStep({ query });
- setup(step);
+ setup({ step });
expect(screen.getByText(card.name)).toBeInTheDocument();
expect(getIcon(icon)).toBeInTheDocument();
@@ -208,7 +213,7 @@ describe("DataStep", () => {
it("should render with a single column selected", async () => {
const query = createQueryWithFields(["ID"]);
- setup(createMockNotebookStep({ query }));
+ setup({ step: createMockNotebookStep({ query }) });
await userEvent.click(screen.getByLabelText("Pick columns"));
expect(screen.getByLabelText("Select all")).not.toBeChecked();
@@ -220,7 +225,7 @@ describe("DataStep", () => {
it("should render with multiple columns selected", async () => {
const query = createQueryWithFields(["ID", "TOTAL"]);
- setup(createMockNotebookStep({ query }));
+ setup({ step: createMockNotebookStep({ query }) });
await userEvent.click(screen.getByLabelText("Pick columns"));
expect(screen.getByLabelText("Select all")).not.toBeChecked();
@@ -235,7 +240,7 @@ describe("DataStep", () => {
it("should allow selecting a column", async () => {
const query = createQueryWithFields(["ID"]);
const step = createMockNotebookStep({ query });
- const { getNextColumn } = setup(step);
+ const { getNextColumn } = setup({ step });
await userEvent.click(screen.getByLabelText("Pick columns"));
await userEvent.click(screen.getByLabelText("Tax"));
@@ -259,7 +264,7 @@ describe("DataStep", () => {
it("should allow selecting all columns", async () => {
const query = createQueryWithFields(["ID"]);
const step = createMockNotebookStep({ query });
- const { getNextColumn } = setup(step);
+ const { getNextColumn } = setup({ step });
await userEvent.click(screen.getByLabelText("Pick columns"));
await userEvent.click(screen.getByLabelText("Select all"));
@@ -280,7 +285,7 @@ describe("DataStep", () => {
});
it("should not display fields picker in read-only mode", () => {
- setup(createMockNotebookStep(), { readOnly: true });
+ setup({ readOnly: true });
expect(screen.queryByLabelText("Pick columns")).not.toBeInTheDocument();
});
@@ -292,7 +297,7 @@ describe("DataStep", () => {
it("should not display fields picker if a query has aggregations", () => {
const query = createQueryWithAggregation();
const step = createMockNotebookStep({ query });
- setup(step);
+ setup({ step });
expect(screen.queryByLabelText("Pick columns")).not.toBeInTheDocument();
});
@@ -300,7 +305,7 @@ describe("DataStep", () => {
it("should not display fields picker if a query has breakouts", () => {
const query = createQueryWithBreakout();
const step = createMockNotebookStep({ query });
- setup(step);
+ setup({ step });
expect(screen.queryByLabelText("Pick columns")).not.toBeInTheDocument();
});
@@ -379,7 +384,7 @@ describe("DataStep", () => {
describe("embedding SDK context", () => {
it("should not show the tooltip", async () => {
- setup(createMockNotebookStep(), { isEmbeddingSdk: true });
+ setup({ isEmbeddingSdk: true });
await userEvent.hover(screen.getByText("Orders"));
expect(screen.queryByRole("tooltip")).not.toBeInTheDocument();
@@ -388,7 +393,7 @@ describe("DataStep", () => {
it.each([{ metaKey: true }, { ctrlKey: true }])(
"meta/ctrl click should not open the data source",
async clickConfig => {
- const { mockWindowOpen } = setup(createMockNotebookStep(), {
+ const { mockWindowOpen } = setup({
isEmbeddingSdk: true,
});
@@ -404,7 +409,7 @@ describe("DataStep", () => {
);
it("middle click should not open the data source", async () => {
- const { mockWindowOpen } = setup(createMockNotebookStep(), {
+ const { mockWindowOpen } = setup({
isEmbeddingSdk: true,
});
@@ -424,4 +429,39 @@ describe("DataStep", () => {
});
});
});
+
+ describe("metrics", () => {
+ it("should automatically aggregate by count for metrics", async () => {
+ const step = createMockNotebookStep({
+ question: DEFAULT_QUESTION.setType("metric"),
+ });
+ const { getNextQuery } = setup({ step });
+
+ await userEvent.click(screen.getByText("Orders"));
+ await userEvent.click(await screen.findByText("Products"));
+
+ const { stageIndex } = step;
+ const nextQuery = getNextQuery();
+ const nextAggregations = Lib.aggregations(nextQuery, stageIndex);
+ expect(nextAggregations).toHaveLength(1);
+ expect(
+ Lib.displayInfo(nextQuery, stageIndex, nextAggregations[0]),
+ ).toMatchObject({
+ displayName: "Count",
+ });
+ });
+
+ it("should not automatically aggregate by count for non-metrics", async () => {
+ const step = createMockNotebookStep();
+ const { getNextQuery } = setup({ step });
+
+ await userEvent.click(screen.getByText("Orders"));
+ await userEvent.click(await screen.findByText("Products"));
+
+ const { stageIndex } = step;
+ const nextQuery = getNextQuery();
+ const nextAggregations = Lib.aggregations(nextQuery, stageIndex);
+ expect(nextAggregations).toHaveLength(0);
+ });
+ });
});
diff --git a/frontend/src/metabase/querying/notebook/components/NotebookCell/NotebookCell.styled.tsx b/frontend/src/metabase/querying/notebook/components/NotebookCell/NotebookCell.styled.tsx
index 2f8ed4fdd78..11ff972bcba 100644
--- a/frontend/src/metabase/querying/notebook/components/NotebookCell/NotebookCell.styled.tsx
+++ b/frontend/src/metabase/querying/notebook/components/NotebookCell/NotebookCell.styled.tsx
@@ -35,6 +35,7 @@ export const NotebookCellItemContainer = styled.div<{
(!props.inactive || props.onClick) && !props.readOnly && !props.disabled
? "pointer"
: "default"};
+ pointer-events: ${props => (props.disabled ? "none" : "auto")};
&:hover {
border-color: ${props => props.inactive && alpha(props.color, 0.8)};
@@ -59,7 +60,6 @@ export const NotebookCellItemContentContainer = styled.div<{
align-items: center;
padding: ${CONTAINER_PADDING};
background-color: ${props => (props.inactive ? "transparent" : props.color)};
- pointer-events: ${props => (props.disabled ? "none" : "auto")};
&:hover {
background-color: ${props =>
diff --git a/frontend/test/__support__/server-mocks/database.ts b/frontend/test/__support__/server-mocks/database.ts
index c80606f7b93..0425e4c4bbf 100644
--- a/frontend/test/__support__/server-mocks/database.ts
+++ b/frontend/test/__support__/server-mocks/database.ts
@@ -15,7 +15,7 @@ export function setupDatabaseEndpoints(db: Database) {
fetchMock.post(`path:/api/database/${db.id}/discard_values`, {});
setupSchemaEndpoints(db);
setupDatabaseIdFieldsEndpoints(db);
- db.tables?.forEach(table => setupTableEndpoints(table));
+ db.tables?.forEach(table => setupTableEndpoints({ ...table, db }));
fetchMock.put(`path:/api/database/${db.id}`, async url => {
const call = fetchMock.lastCall(url);
diff --git a/frontend/test/__support__/server-mocks/table.ts b/frontend/test/__support__/server-mocks/table.ts
index a76953814fa..f02eb59a2e4 100644
--- a/frontend/test/__support__/server-mocks/table.ts
+++ b/frontend/test/__support__/server-mocks/table.ts
@@ -13,7 +13,7 @@ export function setupTableEndpoints(
fetchMock.post(`path:/api/table/${table.id}/rescan_values`, {});
fetchMock.post(`path:/api/table/${table.id}/discard_values`, {});
setupTableQueryMetadataEndpoint(table);
- table.fields?.forEach(field => setupFieldEndpoints(field));
+ table.fields?.forEach(field => setupFieldEndpoints({ ...field, table }));
}
export function setupTableQueryMetadataEndpoint(table: Table) {
--
GitLab