From 2911c4b95c7ee5a66a03d07029e7c9144e502f05 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Wed, 26 Jan 2022 17:13:14 -0500
Subject: [PATCH] Fix a couple of edge cases in column ordering for exports
(#19882)
Co-authored-by: Nemanja <31325167+nemanjaglumac@users.noreply.github.com>
---
...ative-query-export-column-order.cy.spec.js | 83 +++++++++++++++++++
.../shared/models/visualization_settings.cljc | 9 +-
src/metabase/query_processor/streaming.clj | 13 +--
.../query_processor/streaming_test.clj | 7 +-
4 files changed, 101 insertions(+), 11 deletions(-)
create mode 100644 frontend/test/metabase/scenarios/downloads/reproductions/19889-native-query-export-column-order.cy.spec.js
diff --git a/frontend/test/metabase/scenarios/downloads/reproductions/19889-native-query-export-column-order.cy.spec.js b/frontend/test/metabase/scenarios/downloads/reproductions/19889-native-query-export-column-order.cy.spec.js
new file mode 100644
index 00000000000..35214d20ff3
--- /dev/null
+++ b/frontend/test/metabase/scenarios/downloads/reproductions/19889-native-query-export-column-order.cy.spec.js
@@ -0,0 +1,83 @@
+import { restore, downloadAndAssert } from "__support__/e2e/cypress";
+
+const questionDetails = {
+ name: "19889",
+ native: {
+ query: 'select 1 "column a", 2 "column b", 3 "column c"',
+ },
+};
+
+const testCases = ["csv", "xlsx"];
+
+describe("issue 19889", () => {
+ beforeEach(() => {
+ cy.intercept("POST", "/api/dataset").as("dataset");
+
+ restore();
+ cy.signInAsAdmin();
+
+ cy.createNativeQuestion(questionDetails).then(({ body: { id } }) => {
+ cy.wrap(id).as("questionId");
+
+ cy.intercept("POST", `/api/card/${id}/query`).as("cardQuery");
+ cy.visit(`/question/${id}`);
+
+ cy.wait("@cardQuery");
+
+ // Reorder columns a and b
+ cy.findByText("column a")
+ .trigger("mousedown", 0, 0, { force: true })
+ .trigger("mousemove", 5, 5, { force: true })
+ .trigger("mousemove", 100, 0, { force: true })
+ .trigger("mouseup", 100, 0, { force: true });
+ cy.findByText("Started from").click(); // Give DOM some time to update
+ });
+ });
+
+ testCases.forEach(fileType => {
+ it(`should order columns correctly in unsaved native query exports`, () => {
+ downloadAndAssert({ fileType, raw: true }, sheet => {
+ expect(sheet["A1"].v).to.equal("column b");
+ expect(sheet["B1"].v).to.equal("column a");
+ expect(sheet["C1"].v).to.equal("column c");
+ });
+ });
+
+ it(`should order columns correctly in saved native query exports`, () => {
+ saveAndOverwrite();
+
+ cy.get("@questionId").then(questionId => {
+ downloadAndAssert({ fileType, questionId, raw: true }, sheet => {
+ expect(sheet["A1"].v).to.equal("column b");
+ expect(sheet["B1"].v).to.equal("column a");
+ expect(sheet["C1"].v).to.equal("column c");
+ });
+ });
+ });
+
+ it(`should order columns correctly in saved native query exports when the query was modified but not re-run before save (#19889)`, () => {
+ cy.contains(/open editor/i).click();
+ cy.get(".ace_editor").type(
+ '{selectall}select 1 "column x", 2 "column y", 3 "column c"',
+ );
+
+ saveAndOverwrite();
+
+ cy.get("@questionId").then(questionId => {
+ cy.visit(`/question/${questionId}`);
+ cy.wait("@cardQuery");
+
+ downloadAndAssert({ fileType, questionId, raw: true }, sheet => {
+ expect(sheet["A1"].v).to.equal("column x");
+ expect(sheet["B1"].v).to.equal("column y");
+ expect(sheet["C1"].v).to.equal("column c");
+ });
+ });
+ });
+ });
+});
+
+function saveAndOverwrite() {
+ cy.findByText("Save").click();
+ cy.button("Save").click();
+}
diff --git a/shared/src/metabase/shared/models/visualization_settings.cljc b/shared/src/metabase/shared/models/visualization_settings.cljc
index df3afe50624..a1e64ea6337 100644
--- a/shared/src/metabase/shared/models/visualization_settings.cljc
+++ b/shared/src/metabase/shared/models/visualization_settings.cljc
@@ -420,11 +420,12 @@
(set/map-invert db->norm-param-ref-keys))
(def ^:private db->norm-table-columns-keys
- {:name ::table-column-name
+ {:name ::table-column-name
; for now, do not translate the value of this key (the field vector)
- :fieldref ::table-column-field-ref
- :fieldRef ::table-column-field-ref
- :enabled ::table-column-enabled})
+ :fieldref ::table-column-field-ref
+ :field_ref ::table-column-field-ref
+ :fieldRef ::table-column-field-ref
+ :enabled ::table-column-enabled})
(def ^:private norm->db-table-columns-keys
(set/map-invert db->norm-table-columns-keys))
diff --git a/src/metabase/query_processor/streaming.clj b/src/metabase/query_processor/streaming.clj
index 8ed184b00b9..e8cfe536c44 100644
--- a/src/metabase/query_processor/streaming.clj
+++ b/src/metabase/query_processor/streaming.clj
@@ -35,14 +35,15 @@
(mbql.u/uniquify-names (map :name cols))))
(defn- validate-table-columms
- "Validate that all of the field refs in `table-columns` correspond to actual columns in `cols`, if `cols` contains
- field refs. Returns `nil` if any do not, so that we fall back to using `cols` directly for the export (#19465).
+ "Validate that all of the columns in `table-columns` correspond to actual columns in `cols`, correlating them by
+ field ref or name. Returns `nil` if any do not, so that we fall back to using `cols` directly for the export (#19465).
Otherwise returns `table-columns`."
[table-columns cols]
- (let [col-field-refs (set (remove nil? (map :field_ref cols)))]
- ;; If there are no field refs in `cols` (e.g. for native queries), we should use `table-columns` as-is
- (when (or (empty? col-field-refs)
- (every? (fn [table-col] (col-field-refs (::mb.viz/table-column-field-ref table-col))) table-columns))
+ (let [col-field-refs (set (remove nil? (map :field_ref cols)))
+ col-names (set (remove nil? (map :name cols)))]
+ (when (every? (fn [table-col] (or (col-field-refs (::mb.viz/table-column-field-ref table-col))
+ (col-names (::mb.viz/table-column-name table-col))))
+ table-columns)
table-columns)))
(defn- export-column-order
diff --git a/test/metabase/query_processor/streaming_test.clj b/test/metabase/query_processor/streaming_test.clj
index 2d6ed8677c2..1f8e6b3a654 100644
--- a/test/metabase/query_processor/streaming_test.clj
+++ b/test/metabase/query_processor/streaming_test.clj
@@ -476,7 +476,12 @@
(@#'qp.streaming/export-column-order
[{:id 0, :name "Col1" :field_ref [:field 0 nil]}]
[{::mb.viz/table-column-field-ref [:field 1 nil], ::mb.viz/table-column-enabled true}
- {::mb.viz/table-column-field-ref [:field 2 nil], ::mb.viz/table-column-enabled true}]))))
+ {::mb.viz/table-column-field-ref [:field 2 nil], ::mb.viz/table-column-enabled true}])))
+ (is (= [0]
+ (@#'qp.streaming/export-column-order
+ [{:id 0, :name "Col1" :field_ref [:field 0 nil]}]
+ [{::mb.viz/table-column-name "Col1" , ::mb.viz/table-column-enabled true}
+ {::mb.viz/table-column-name "Col2" , ::mb.viz/table-column-enabled true}]))))
(testing "if table-columns is nil, original order of cols is used"
(is (= [0 1]
--
GitLab