From d060850ec0661f0a369e35a2b86aebb1e5827e48 Mon Sep 17 00:00:00 2001
From: Anton Kulyk <kuliks.anton@gmail.com>
Date: Tue, 28 Mar 2023 12:24:31 +0100
Subject: [PATCH] Fix notebook editor shows the remove button on the data step
(#29575)
* Add basic tests to `NotebookStep`
* Fix irreversible steps have "remove" button
* Use `aria-label` instead of a test ID
* Use `IconButtonWrapper`
---
...nested-query-remove-expressions.cy.spec.js | 2 +-
.../reproductions/17514-ui-overlay.cy.spec.js | 2 +-
.../notebook/NotebookStep/NotebookStep.tsx | 21 ++--
.../NotebookStep/NotebookStep.unit.spec.tsx | 108 ++++++++++++++++++
4 files changed, 124 insertions(+), 9 deletions(-)
create mode 100644 frontend/src/metabase/query_builder/components/notebook/NotebookStep/NotebookStep.unit.spec.tsx
diff --git a/e2e/test/scenarios/custom-column/reproductions/19745-cc-nested-query-remove-expressions.cy.spec.js b/e2e/test/scenarios/custom-column/reproductions/19745-cc-nested-query-remove-expressions.cy.spec.js
index b50f98ffd31..ed8ef366a05 100644
--- a/e2e/test/scenarios/custom-column/reproductions/19745-cc-nested-query-remove-expressions.cy.spec.js
+++ b/e2e/test/scenarios/custom-column/reproductions/19745-cc-nested-query-remove-expressions.cy.spec.js
@@ -97,7 +97,7 @@ function removeExpression(name) {
function removeAllExpressions() {
getNotebookStep("expression", { stage: 1 }).within(() => {
- cy.findByTestId("remove-step").click({ force: true });
+ cy.findByLabelText("Remove step").click({ force: true });
});
}
diff --git a/e2e/test/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js b/e2e/test/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js
index 0ca2f08448f..331663bf241 100644
--- a/e2e/test/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js
+++ b/e2e/test/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js
@@ -172,7 +172,7 @@ function removeJoinedTable() {
cy.findAllByText("Join data")
.first()
.parent()
- .findByTestId("remove-step")
+ .findByLabelText("Remove step")
.click({ force: true });
}
diff --git a/frontend/src/metabase/query_builder/components/notebook/NotebookStep/NotebookStep.tsx b/frontend/src/metabase/query_builder/components/notebook/NotebookStep/NotebookStep.tsx
index 2c53c1f2ed9..773ec5f8226 100644
--- a/frontend/src/metabase/query_builder/components/notebook/NotebookStep/NotebookStep.tsx
+++ b/frontend/src/metabase/query_builder/components/notebook/NotebookStep/NotebookStep.tsx
@@ -5,6 +5,7 @@ import { color as c } from "metabase/lib/colors";
import { useToggle } from "metabase/hooks/use-toggle";
import Icon from "metabase/components/Icon";
+import IconButtonWrapper from "metabase/components/IconButtonWrapper";
import ExpandingContent from "metabase/components/ExpandingContent";
import type Question from "metabase-lib/Question";
@@ -104,6 +105,7 @@ function NotebookStep({
const color = getColor();
const canPreview = step?.previewQuery?.isValid?.();
const hasPreviewButton = !isPreviewOpen && canPreview;
+ const canRevert = typeof step.revert === "function";
return (
<ExpandingContent isInitiallyOpen={!isLastOpened} isOpen>
@@ -113,13 +115,18 @@ function NotebookStep({
>
<StepHeader color={color}>
{title}
- <Icon
- name="close"
- className="ml-auto cursor-pointer text-light text-medium-hover hover-child"
- tooltip={t`Remove`}
- onClick={handleClickRevert}
- data-testid="remove-step"
- />
+ {canRevert && (
+ <IconButtonWrapper
+ className="ml-auto text-light text-medium-hover hover-child"
+ onClick={handleClickRevert}
+ >
+ <Icon
+ name="close"
+ tooltip={t`Remove`}
+ aria-label={t`Remove step`}
+ />
+ </IconButtonWrapper>
+ )}
</StepHeader>
{NotebookStepComponent && (
diff --git a/frontend/src/metabase/query_builder/components/notebook/NotebookStep/NotebookStep.unit.spec.tsx b/frontend/src/metabase/query_builder/components/notebook/NotebookStep/NotebookStep.unit.spec.tsx
new file mode 100644
index 00000000000..c21a2f9cc2b
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/notebook/NotebookStep/NotebookStep.unit.spec.tsx
@@ -0,0 +1,108 @@
+import React from "react";
+import { renderWithProviders, screen } from "__support__/ui";
+import {
+ setupDatabasesEndpoints,
+ setupSearchEndpoints,
+} from "__support__/server-mocks";
+
+import { createSampleDatabase } from "metabase-types/api/mocks/presets";
+
+import { getSavedStructuredQuestion } from "metabase-lib/mocks";
+import type Question from "metabase-lib/Question";
+import type StructuredQuery from "metabase-lib/queries/StructuredQuery";
+
+import {
+ NotebookStep as INotebookStep,
+ NotebookStepType,
+} from "../lib/steps.types";
+import NotebookStep from "./NotebookStep";
+
+type SetupOpts = {
+ step?: INotebookStep;
+ question?: Question;
+};
+
+const DEFAULT_QUESTION = getSavedStructuredQuestion();
+const DEFAULT_QUERY = DEFAULT_QUESTION.query() as StructuredQuery;
+
+function createNotebookStep(opts = {}): INotebookStep {
+ return {
+ id: "test-step",
+ type: "data",
+ stageIndex: 0,
+ itemIndex: 0,
+ query: DEFAULT_QUERY,
+ valid: true,
+ active: true,
+ visible: true,
+ actions: [],
+ previewQuery: null,
+ next: null,
+ previous: null,
+ revert: jest.fn(),
+ clean: jest.fn(),
+ update: jest.fn(),
+ ...opts,
+ };
+}
+
+function setup({
+ step = createNotebookStep(),
+ question = DEFAULT_QUESTION,
+}: SetupOpts = {}) {
+ const openStep = jest.fn();
+ const updateQuery = jest.fn();
+
+ setupDatabasesEndpoints([createSampleDatabase()]);
+ setupSearchEndpoints([]);
+
+ renderWithProviders(
+ <NotebookStep
+ step={step}
+ sourceQuestion={question}
+ isLastStep={false}
+ isLastOpened={false}
+ openStep={openStep}
+ updateQuery={updateQuery}
+ />,
+ );
+
+ return {
+ openStep,
+ updateQuery,
+ };
+}
+
+const STEP_TYPES: NotebookStepType[] = [
+ "data",
+ "join",
+ "expression",
+ "filter",
+ "summarize",
+ "aggregate",
+ "breakout",
+ "sort",
+ "limit",
+];
+
+describe("NotebookStep", () => {
+ test.each(STEP_TYPES)(`renders a %s step correctly`, type => {
+ const step = createNotebookStep({ type });
+ const testId = `step-${type}-${step.stageIndex}-${step.itemIndex}`;
+ setup({ step });
+
+ expect(screen.getByTestId(testId)).toBeInTheDocument();
+ expect(
+ screen.getByRole("button", { name: "Remove step" }),
+ ).toBeInTheDocument();
+ });
+
+ it("doesn't render the remove button if step revert isn't implemented", () => {
+ const step = createNotebookStep({ type: "data", revert: null });
+ setup({ step });
+
+ expect(
+ screen.queryByRole("button", { name: "Remove step" }),
+ ).not.toBeInTheDocument();
+ });
+});
--
GitLab