From f0ac47d34c6f8a264d9f496a7b656deef76480f1 Mon Sep 17 00:00:00 2001
From: Anton Kulyk <kuliks.anton@gmail.com>
Date: Wed, 14 Jul 2021 08:53:34 +0300
Subject: [PATCH] Fix native question looses visualisation settings if query
fails (#17009)
* Add test IDs for visualization buttons
* Add `runNativeQuery` helper to e2e-misc-helpers
* Add repro for #16914
* Don't sync native query columns if it failed
* Fix `table.columns: []` treated as "all hidden"
---
frontend/src/metabase-lib/lib/Question.js | 3 +-
.../components/view/ViewFooter.jsx | 18 +++++++-
.../visualizations/visualizations/Table.jsx | 31 +++++++------
.../e2e/helpers/e2e-misc-helpers.js | 11 +++++
.../native/reproductions/16914.cy.spec.js | 44 +++++++++++++++++++
5 files changed, 91 insertions(+), 16 deletions(-)
create mode 100644 frontend/test/metabase/scenarios/native/reproductions/16914.cy.spec.js
diff --git a/frontend/src/metabase-lib/lib/Question.js b/frontend/src/metabase-lib/lib/Question.js
index d7ec233f69c..744da5b07ea 100644
--- a/frontend/src/metabase-lib/lib/Question.js
+++ b/frontend/src/metabase-lib/lib/Question.js
@@ -645,7 +645,8 @@ export default class Question {
syncColumnsAndSettings(previous, queryResults) {
const query = this.query();
- if (query instanceof NativeQuery && queryResults) {
+ const isQueryResultValid = queryResults && !queryResults.error;
+ if (query instanceof NativeQuery && isQueryResultValid) {
return this._syncNativeQuerySettings(queryResults);
}
const previousQuery = previous && previous.query();
diff --git a/frontend/src/metabase/query_builder/components/view/ViewFooter.jsx b/frontend/src/metabase/query_builder/components/view/ViewFooter.jsx
index d46f09d434b..23162b04221 100644
--- a/frontend/src/metabase/query_builder/components/view/ViewFooter.jsx
+++ b/frontend/src/metabase/query_builder/components/view/ViewFooter.jsx
@@ -191,14 +191,28 @@ const VizTypeButton = ({ question, result, ...props }) => {
const icon = visualization && visualization.iconName;
return (
- <ViewButton medium p={[2, 1]} icon={icon} labelBreakpoint="sm" {...props}>
+ <ViewButton
+ medium
+ p={[2, 1]}
+ icon={icon}
+ labelBreakpoint="sm"
+ data-testid="viz-type-button"
+ {...props}
+ >
{t`Visualization`}
</ViewButton>
);
};
const VizSettingsButton = ({ ...props }) => (
- <ViewButton medium p={[2, 1]} icon="gear" labelBreakpoint="sm" {...props}>
+ <ViewButton
+ medium
+ p={[2, 1]}
+ icon="gear"
+ labelBreakpoint="sm"
+ data-testid="viz-settings-button"
+ {...props}
+ >
{t`Settings`}
</ViewButton>
);
diff --git a/frontend/src/metabase/visualizations/visualizations/Table.jsx b/frontend/src/metabase/visualizations/visualizations/Table.jsx
index db1d8a15039..bb6bb400eba 100644
--- a/frontend/src/metabase/visualizations/visualizations/Table.jsx
+++ b/frontend/src/metabase/visualizations/visualizations/Table.jsx
@@ -160,6 +160,11 @@ export default class Table extends Component {
widget: ChartSettingOrderedColumns,
getHidden: (series, vizSettings) => vizSettings["table.pivot"],
isValid: ([{ card, data }]) =>
+ // If "table.columns" happened to be an empty array,
+ // it will be treated as "all columns are hidden",
+ // This check ensures it's not empty,
+ // otherwise it will be overwritten by `getDefault` below
+ card.visualization_settings["table.columns"].length !== 0 &&
_.all(
card.visualization_settings["table.columns"],
columnSetting =>
@@ -392,15 +397,15 @@ export default class Table extends Component {
const { data } = this.state;
const sort = getIn(card, ["dataset_query", "query", "order-by"]) || null;
const isPivoted = settings["table.pivot"];
- const isColumnsDisabled =
- (settings["table.columns"] || []).filter(f => f.enabled).length < 1;
+ const columnSettings = settings["table.columns"] || [];
+ const areAllColumnsHidden = !columnSettings.some(f => f.enabled);
const TableComponent = isDashboard ? TableSimple : TableInteractive;
if (!data) {
return null;
}
- if (isColumnsDisabled) {
+ if (areAllColumnsHidden) {
return (
<div
className={cx(
@@ -420,16 +425,16 @@ export default class Table extends Component {
<span className="h4 text-bold">{t`Every field is hidden right now`}</span>
</div>
);
- } else {
- return (
- <TableComponent
- {...this.props}
- data={data}
- isPivoted={isPivoted}
- sort={sort}
- getColumnTitle={this.getColumnTitle}
- />
- );
}
+
+ return (
+ <TableComponent
+ {...this.props}
+ data={data}
+ isPivoted={isPivoted}
+ sort={sort}
+ getColumnTitle={this.getColumnTitle}
+ />
+ );
}
}
diff --git a/frontend/test/__support__/e2e/helpers/e2e-misc-helpers.js b/frontend/test/__support__/e2e/helpers/e2e-misc-helpers.js
index 2b941d96b11..78252c323e5 100644
--- a/frontend/test/__support__/e2e/helpers/e2e-misc-helpers.js
+++ b/frontend/test/__support__/e2e/helpers/e2e-misc-helpers.js
@@ -28,3 +28,14 @@ export function openNativeEditor(alias = "editor") {
.as(alias)
.should("be.visible");
}
+
+/**
+ * Executes native query and waits for the results to load.
+ * Makes sure that the question is not "dirty" after the query successfully ran.
+ * @param {string} [xhrAlias ="dataset"]
+ */
+export function runNativeQuery(xhrAlias = "dataset") {
+ cy.get(".NativeQueryEditor .Icon-play").click();
+ cy.wait("@" + xhrAlias);
+ cy.icon("play").should("not.exist");
+}
diff --git a/frontend/test/metabase/scenarios/native/reproductions/16914.cy.spec.js b/frontend/test/metabase/scenarios/native/reproductions/16914.cy.spec.js
new file mode 100644
index 00000000000..cdedddc2e33
--- /dev/null
+++ b/frontend/test/metabase/scenarios/native/reproductions/16914.cy.spec.js
@@ -0,0 +1,44 @@
+import {
+ restore,
+ openNativeEditor,
+ runNativeQuery,
+} from "__support__/e2e/cypress";
+
+describe("issue 16914", () => {
+ beforeEach(() => {
+ restore();
+ cy.intercept("POST", "api/dataset").as("dataset");
+ cy.signInAsAdmin();
+ });
+
+ it("should recover visualization settings after a failed query (metabase#16914)", () => {
+ const FAILING_PIECE = " foo";
+ const highlightSelectedText = "{shift}{leftarrow}".repeat(
+ FAILING_PIECE.length,
+ );
+
+ openNativeEditor().type("SELECT 'a' as hidden, 'b' as visible");
+ runNativeQuery();
+
+ cy.findByTestId("viz-settings-button").click();
+ cy.findByTestId("sidebar-left")
+ .contains(/hidden/i)
+ .siblings(".Icon-close")
+ .click();
+ cy.button("Done").click();
+
+ cy.get("@editor").type(FAILING_PIECE);
+ runNativeQuery();
+
+ cy.get("@editor").type(
+ "{movetoend}" + highlightSelectedText + "{backspace}",
+ );
+ runNativeQuery();
+
+ cy.get(".Visualization").within(() => {
+ cy.findByText("Every field is hidden right now").should("not.exist");
+ cy.findByText("VISIBLE");
+ cy.findByText("HIDDEN").should("not.exist");
+ });
+ });
+});
--
GitLab