From 08e60aa0b3295ddc240f24ec34bfae1f1ee58f41 Mon Sep 17 00:00:00 2001
From: Anton Kulyk <kuliks.anton@gmail.com>
Date: Wed, 9 Feb 2022 10:55:43 +0200
Subject: [PATCH] Fix drill actions dispatching redux actions are broken
(#20098)
* Make `performDefaultAction` usage more readable
* Connect `Visualization` to redux
* Fix visualization unit tests
---
.../components/Visualization.jsx | 13 +++++--
.../components/ChartSettings.unit.spec.js | 22 +++++------
.../components/SmartScalar.unit.spec.js | 10 ++---
.../components/Visualization-pie.unit.spec.js | 26 +++++++++----
.../Visualization-table.unit.spec.js | 4 +-
.../components/Visualization.unit.spec.js | 38 +++++++++++--------
.../ChartNestedSettingsSeries.unit.spec.js | 6 +--
7 files changed, 70 insertions(+), 49 deletions(-)
diff --git a/frontend/src/metabase/visualizations/components/Visualization.jsx b/frontend/src/metabase/visualizations/components/Visualization.jsx
index 84a38b537cb..0d9d22d6b23 100644
--- a/frontend/src/metabase/visualizations/components/Visualization.jsx
+++ b/frontend/src/metabase/visualizations/components/Visualization.jsx
@@ -1,5 +1,6 @@
/* eslint-disable react/prop-types */
import React from "react";
+import { connect } from "react-redux";
import ExplicitSize from "metabase/components/ExplicitSize";
import ChartCaption from "metabase/visualizations/components/ChartCaption";
@@ -43,6 +44,7 @@ import { memoize } from "metabase-lib/lib/utils";
// NOTE: pass `CardVisualization` so that we don't include header when providing size to child element
@ExplicitSize({ selector: ".CardVisualization" })
+@connect()
export default class Visualization extends React.PureComponent {
constructor(props) {
super(props);
@@ -248,12 +250,15 @@ export default class Visualization extends React.PureComponent {
return;
}
- if (
- performDefaultAction(this.getClickActions(clicked), {
+ const didPerformDefaultAction = performDefaultAction(
+ this.getClickActions(clicked),
+ {
dispatch: this.props.dispatch,
onChangeCardAndRun: this.handleOnChangeCardAndRun,
- })
- ) {
+ },
+ );
+
+ if (didPerformDefaultAction) {
return;
}
diff --git a/frontend/test/metabase/visualizations/components/ChartSettings.unit.spec.js b/frontend/test/metabase/visualizations/components/ChartSettings.unit.spec.js
index 5995eddd7ce..a70231a6ee9 100644
--- a/frontend/test/metabase/visualizations/components/ChartSettings.unit.spec.js
+++ b/frontend/test/metabase/visualizations/components/ChartSettings.unit.spec.js
@@ -1,5 +1,5 @@
import React from "react";
-import { render, fireEvent } from "@testing-library/react";
+import { renderWithProviders, fireEvent } from "__support__/ui";
import ChartSettings from "metabase/visualizations/components/ChartSettings";
@@ -21,10 +21,10 @@ function widget(widget = {}) {
describe("ChartSettings", () => {
it("should not crash if there are no widgets", () => {
- render(<ChartSettings {...DEFAULT_PROPS} widgets={[]} />);
+ renderWithProviders(<ChartSettings {...DEFAULT_PROPS} widgets={[]} />);
});
it("should not crash if the initial section is invalid", () => {
- render(
+ renderWithProviders(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[widget({ section: "Foo" })]}
@@ -33,7 +33,7 @@ describe("ChartSettings", () => {
);
});
it("should default to the first section (if no section in DEFAULT_TAB_PRIORITY)", () => {
- const { getByLabelText } = render(
+ const { getByLabelText } = renderWithProviders(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[widget({ section: "Foo" }), widget({ section: "Bar" })]}
@@ -43,7 +43,7 @@ describe("ChartSettings", () => {
expect(getByLabelText("Bar")).not.toBeChecked();
});
it("should default to the DEFAULT_TAB_PRIORITY", () => {
- const { getByLabelText } = render(
+ const { getByLabelText } = renderWithProviders(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[
@@ -56,7 +56,7 @@ describe("ChartSettings", () => {
expect(getByLabelText("Display")).toBeChecked();
});
it("should be able to switch sections", () => {
- const { getByText, getByLabelText } = render(
+ const { getByText, getByLabelText } = renderWithProviders(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[widget({ section: "Foo" }), widget({ section: "Bar" })]}
@@ -70,7 +70,7 @@ describe("ChartSettings", () => {
});
it("should show widget names", () => {
- const { getByText } = render(
+ const { getByText } = renderWithProviders(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[
@@ -84,7 +84,7 @@ describe("ChartSettings", () => {
});
it("should not show hidden widgets", () => {
- const { getByText, queryByText } = render(
+ const { getByText, queryByText } = renderWithProviders(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[
@@ -98,7 +98,7 @@ describe("ChartSettings", () => {
});
it("should show the section picker if there are multiple sections", () => {
- const { getByText } = render(
+ const { getByText } = renderWithProviders(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[
@@ -111,7 +111,7 @@ describe("ChartSettings", () => {
});
it("should not show the section picker if there's only one section", () => {
- const { queryByText } = render(
+ const { queryByText } = renderWithProviders(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[
@@ -130,7 +130,7 @@ describe("ChartSettings", () => {
hidden: true,
id: "column_settings",
});
- const { queryByText } = render(
+ const { queryByText } = renderWithProviders(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[
diff --git a/frontend/test/metabase/visualizations/components/SmartScalar.unit.spec.js b/frontend/test/metabase/visualizations/components/SmartScalar.unit.spec.js
index 9fa4607d7fc..9da208fa1a2 100644
--- a/frontend/test/metabase/visualizations/components/SmartScalar.unit.spec.js
+++ b/frontend/test/metabase/visualizations/components/SmartScalar.unit.spec.js
@@ -1,5 +1,5 @@
import React from "react";
-import { render } from "@testing-library/react";
+import { renderWithProviders } from "__support__/ui";
import { NumberColumn, DateTimeColumn } from "../__support__/visualizations";
@@ -26,7 +26,7 @@ describe("SmartScalar", () => {
col: "Count",
},
];
- const { getAllByText } = render(
+ const { getAllByText } = renderWithProviders(
<Visualization rawSeries={series({ rows, insights })} />,
);
getAllByText("120");
@@ -46,7 +46,7 @@ describe("SmartScalar", () => {
col: "Count",
},
];
- const { getAllByText } = render(
+ const { getAllByText } = renderWithProviders(
<Visualization rawSeries={series({ rows, insights })} />,
);
getAllByText("80");
@@ -66,7 +66,7 @@ describe("SmartScalar", () => {
col: "Count",
},
];
- const { getAllByText } = render(
+ const { getAllByText } = renderWithProviders(
<Visualization rawSeries={series({ rows, insights })} />,
);
getAllByText("100");
@@ -87,7 +87,7 @@ describe("SmartScalar", () => {
col: "Count",
},
];
- const { getAllByText } = render(
+ const { getAllByText } = renderWithProviders(
<Visualization rawSeries={series({ rows, insights })} />,
);
getAllByText("8,000%");
diff --git a/frontend/test/metabase/visualizations/components/Visualization-pie.unit.spec.js b/frontend/test/metabase/visualizations/components/Visualization-pie.unit.spec.js
index d120a81083a..f7a7b6c6fc7 100644
--- a/frontend/test/metabase/visualizations/components/Visualization-pie.unit.spec.js
+++ b/frontend/test/metabase/visualizations/components/Visualization-pie.unit.spec.js
@@ -1,5 +1,5 @@
import React from "react";
-import { render, fireEvent } from "@testing-library/react";
+import { renderWithProviders, fireEvent } from "__support__/ui";
import { NumberColumn, StringColumn } from "../__support__/visualizations";
@@ -20,7 +20,9 @@ describe("pie chart", () => {
["bar", 2],
["baz", 2],
];
- const { getAllByText } = render(<Visualization rawSeries={series(rows)} />);
+ const { getAllByText } = renderWithProviders(
+ <Visualization rawSeries={series(rows)} />,
+ );
getAllByText("20%");
getAllByText("40%");
});
@@ -31,7 +33,9 @@ describe("pie chart", () => {
["bar", 0.499],
["baz", 0.001],
];
- const { getAllByText } = render(<Visualization rawSeries={series(rows)} />);
+ const { getAllByText } = renderWithProviders(
+ <Visualization rawSeries={series(rows)} />,
+ );
getAllByText("50.0%");
getAllByText("49.9%");
getAllByText("0.1%");
@@ -44,7 +48,9 @@ describe("pie chart", () => {
["baz", 0.002],
["qux", 0.008],
];
- const { getAllByText } = render(<Visualization rawSeries={series(rows)} />);
+ const { getAllByText } = renderWithProviders(
+ <Visualization rawSeries={series(rows)} />,
+ );
getAllByText("50%");
getAllByText("49%");
getAllByText("1%");
@@ -62,7 +68,9 @@ describe("pie chart", () => {
data: { rows: [["foo", 1]], cols },
},
];
- const { getAllByText } = render(<Visualization rawSeries={series} />);
+ const { getAllByText } = renderWithProviders(
+ <Visualization rawSeries={series} />,
+ );
getAllByText("100%"); // shouldn't multiply legend percent by `scale`
getAllByText("123"); // should multiply the count in the center by `scale`
});
@@ -85,7 +93,9 @@ describe("pie chart", () => {
},
},
];
- const { getAllByText } = render(<Visualization rawSeries={series} />);
+ const { getAllByText } = renderWithProviders(
+ <Visualization rawSeries={series} />,
+ );
getAllByText("50,1%");
});
@@ -96,7 +106,7 @@ describe("pie chart", () => {
["baz", 0.002],
["qux", 0.008],
];
- const { container, getAllByText, queryAllByText } = render(
+ const { container, getAllByText, queryAllByText } = renderWithProviders(
<Visualization rawSeries={series(rows)} />,
);
const paths = container.querySelectorAll("path");
@@ -117,7 +127,7 @@ describe("pie chart", () => {
["bar", 0.49],
["baz", 0.002],
];
- const { container, queryAllByText } = render(
+ const { container, queryAllByText } = renderWithProviders(
<Visualization rawSeries={series(rows)} />,
);
const paths = container.querySelectorAll("path");
diff --git a/frontend/test/metabase/visualizations/components/Visualization-table.unit.spec.js b/frontend/test/metabase/visualizations/components/Visualization-table.unit.spec.js
index dd83fad7f83..622fbea8cfb 100644
--- a/frontend/test/metabase/visualizations/components/Visualization-table.unit.spec.js
+++ b/frontend/test/metabase/visualizations/components/Visualization-table.unit.spec.js
@@ -1,5 +1,5 @@
import React from "react";
-import { render } from "@testing-library/react";
+import { renderWithProviders } from "__support__/ui";
import { NumberColumn } from "../__support__/visualizations";
@@ -37,7 +37,7 @@ describe("Table", () => {
},
],
};
- const { getByText } = render(
+ const { getByText } = renderWithProviders(
<Visualization rawSeries={series(rows, settings)} />,
);
jest.runAllTimers();
diff --git a/frontend/test/metabase/visualizations/components/Visualization.unit.spec.js b/frontend/test/metabase/visualizations/components/Visualization.unit.spec.js
index 4ae49dec697..97a9c63ba83 100644
--- a/frontend/test/metabase/visualizations/components/Visualization.unit.spec.js
+++ b/frontend/test/metabase/visualizations/components/Visualization.unit.spec.js
@@ -1,5 +1,5 @@
import React from "react";
-import ReactDOM from "react-dom";
+import { renderWithProviders } from "__support__/ui";
import {
NumberColumn,
@@ -16,41 +16,47 @@ import Visualization from "metabase/visualizations/components/Visualization";
describe("Visualization", () => {
// eslint-disable-next-line no-unused-vars
let element;
- const qs = s => element.querySelector(s);
- const qsa = s => [...element.querySelectorAll(s)];
const renderViz = async series => {
- ReactDOM.render(<Visualization rawSeries={series} />, element);
+ const utils = renderWithProviders(
+ <Visualization rawSeries={series} />,
+ element,
+ );
// The chart isn't rendered until the next tick. This is due to ExplicitSize
// not setting the dimensions until after mounting.
await delay(0);
+ return utils;
+ };
+
+ const getBarColors = container => {
+ const bars = [...container.querySelectorAll(".bar")];
+ return bars.map(bar => bar.getAttribute("fill"));
};
beforeEach(() => {
element = createFixture();
});
+
afterEach(() => {
- ReactDOM.unmountComponentAtNode(element);
cleanupFixture(element);
});
describe("scalar", () => {
it("should render", async () => {
- await renderViz([
+ const { container } = await renderViz([
{
card: { display: "scalar" },
data: { rows: [[1]], cols: [NumberColumn({ name: "Count" })] },
},
]);
- expect(qs("h1").textContent).toEqual("1");
+ expect(container.querySelector("h1").textContent).toEqual("1");
});
});
describe("bar", () => {
- const getBarColors = () => qsa(".bar").map(bar => bar.getAttribute("fill"));
describe("single series", () => {
it("should have correct colors", async () => {
- await renderViz([
+ const { container } = await renderViz([
{
card: { name: "Card", display: "bar" },
data: {
@@ -65,7 +71,7 @@ describe("Visualization", () => {
},
},
]);
- expect(getBarColors()).toEqual([
+ expect(getBarColors(container)).toEqual([
color("brand"), // "count"
color("brand"), // "count"
]);
@@ -73,7 +79,7 @@ describe("Visualization", () => {
});
describe("multiseries: multiple metrics", () => {
it("should have correct colors", async () => {
- await renderViz([
+ const { container } = await renderViz([
{
card: { name: "Card", display: "bar" },
data: {
@@ -89,7 +95,7 @@ describe("Visualization", () => {
},
},
]);
- expect(getBarColors()).toEqual([
+ expect(getBarColors(container)).toEqual([
color("brand"), // "count"
color("brand"), // "count"
color("accent1"), // "sum"
@@ -99,7 +105,7 @@ describe("Visualization", () => {
});
describe("multiseries: multiple breakouts", () => {
it("should have correct colors", async () => {
- await renderViz([
+ const { container } = await renderViz([
{
card: { name: "Card", display: "bar" },
data: {
@@ -117,7 +123,7 @@ describe("Visualization", () => {
},
},
]);
- expect(getBarColors()).toEqual([
+ expect(getBarColors(container)).toEqual([
color("accent1"), // "a"
color("accent1"), // "a"
color("accent2"), // "b"
@@ -127,7 +133,7 @@ describe("Visualization", () => {
});
describe("multiseries: dashcard", () => {
it("should have correct colors", async () => {
- await renderViz([
+ const { container } = await renderViz([
{
card: { name: "Card1", display: "bar" },
data: {
@@ -155,7 +161,7 @@ describe("Visualization", () => {
},
},
]);
- expect(getBarColors()).toEqual([
+ expect(getBarColors(container)).toEqual([
color("brand"), // "count"
color("brand"), // "count"
color("accent2"), // "Card2"
diff --git a/frontend/test/metabase/visualizations/components/settings/ChartNestedSettingsSeries.unit.spec.js b/frontend/test/metabase/visualizations/components/settings/ChartNestedSettingsSeries.unit.spec.js
index 80be9864d64..ed3133581d5 100644
--- a/frontend/test/metabase/visualizations/components/settings/ChartNestedSettingsSeries.unit.spec.js
+++ b/frontend/test/metabase/visualizations/components/settings/ChartNestedSettingsSeries.unit.spec.js
@@ -1,5 +1,5 @@
import React from "react";
-import { render } from "@testing-library/react";
+import { renderWithProviders } from "__support__/ui";
// these tests use ChartSettings directly, but logic we're testing lives in ChartNestedSettingSeries
import ChartSettings from "metabase/visualizations/components/ChartSettings";
@@ -20,7 +20,7 @@ function getSeries(display) {
}
describe("ChartNestedSettingSeries", () => {
it("shouldn't show line/area/bar buttons for row charts", () => {
- const { queryByRole } = render(
+ const { queryByRole } = renderWithProviders(
<ChartSettings
series={getSeries("row")}
initial={{ section: "Display" }}
@@ -33,7 +33,7 @@ describe("ChartNestedSettingSeries", () => {
});
it("should show line/area/bar buttons for bar charts", () => {
- const { getByRole } = render(
+ const { getByRole } = renderWithProviders(
<ChartSettings
series={getSeries("bar")}
initial={{ section: "Display" }}
--
GitLab