From 4e8889337e963ad4574813843c0e54de2512aa3d Mon Sep 17 00:00:00 2001
From: Paul Rosenzweig <paulrosenzweig@users.noreply.github.com>
Date: Thu, 26 Dec 2019 18:16:02 -0500
Subject: [PATCH] Preserve template-tags key order (#11582)
---
.../src/metabase/query_builder/actions.js | 18 ++++----
.../components/NativeQueryEditor.jsx | 5 ++-
.../components/NativeQueryEditor.cy.spec.js | 41 +++++++++++++++++++
3 files changed, 56 insertions(+), 8 deletions(-)
diff --git a/frontend/src/metabase/query_builder/actions.js b/frontend/src/metabase/query_builder/actions.js
index 66027a73f8d..55c2958540b 100644
--- a/frontend/src/metabase/query_builder/actions.js
+++ b/frontend/src/metabase/query_builder/actions.js
@@ -629,15 +629,19 @@ export const updateTemplateTag = createThunkAction(
delete updatedCard.description;
}
- // using updateIn instead of assocIn due to not preserving order of keys
+ // we need to preserve the order of the keys to avoid UI jumps
return updateIn(
updatedCard,
- ["dataset_query", "native", "template-tags", templateTag.name],
- tag =>
- // when we switch type, null out any default
- tag.type !== templateTag.type
- ? { ...templateTag, default: null }
- : templateTag,
+ ["dataset_query", "native", "template-tags"],
+ tags => {
+ const { name } = templateTag;
+ const newTag =
+ tags[name] && tags[name].type !== templateTag.type
+ ? // when we switch type, null out any default
+ { ...templateTag, default: null }
+ : templateTag;
+ return { ...tags, [name]: newTag };
+ },
);
};
},
diff --git a/frontend/src/metabase/query_builder/components/NativeQueryEditor.jsx b/frontend/src/metabase/query_builder/components/NativeQueryEditor.jsx
index 18b3755e0ed..af34f0a0531 100644
--- a/frontend/src/metabase/query_builder/components/NativeQueryEditor.jsx
+++ b/frontend/src/metabase/query_builder/components/NativeQueryEditor.jsx
@@ -161,7 +161,10 @@ export default class NativeQueryEditor extends Component {
return;
}
- if (this._editor.getValue() !== query.queryText()) {
+ // Check that the query prop changed before updating the editor. Otherwise,
+ // we might overwrite just typed characters before onChange is called.
+ const queryPropUpdated = this.props.query !== prevProps.query;
+ if (queryPropUpdated && this._editor.getValue() !== query.queryText()) {
// This is a weird hack, but the purpose is to avoid an infinite loop caused by the fact that calling editor.setValue()
// will trigger the editor 'change' event, update the query, and cause another rendering loop which we don't want, so
// we need a way to update the editor without causing the onChange event to go through as well
diff --git a/frontend/test/metabase/query_builder/components/NativeQueryEditor.cy.spec.js b/frontend/test/metabase/query_builder/components/NativeQueryEditor.cy.spec.js
index ac957f88ab1..90204ed8a5b 100644
--- a/frontend/test/metabase/query_builder/components/NativeQueryEditor.cy.spec.js
+++ b/frontend/test/metabase/query_builder/components/NativeQueryEditor.cy.spec.js
@@ -64,4 +64,45 @@ describe("NativeQueryEditor", () => {
"You'll need to pick a value for 'X' before this query can run.",
);
});
+
+ it("doesn't reorder template tags when updated", () => {
+ cy.visit("/question/new");
+ cy.contains("Native query").click();
+
+ // Write a query with parameter x. It defaults to a text parameter.
+ cy.get(".ace_content").type("{{foo}} {{bar}}", {
+ parseSpecialCharSequences: false,
+ });
+
+ cy.contains("Variables")
+ .parent()
+ .parent()
+ .find(".text-brand")
+ .as("variableLabels");
+
+ // ensure they're in the right order to start
+ cy.get("@variableLabels")
+ .first()
+ .should("have.text", "foo");
+ cy.get("@variableLabels")
+ .last()
+ .should("have.text", "bar");
+
+ // change the parameter to a number.
+ cy.contains("Variable type")
+ .first()
+ .next()
+ .as("variableType");
+ cy.get("@variableType").click();
+ cy.contains("Number").click();
+ cy.get("@variableType").should("have.text", "Number");
+
+ // ensure they're still in the right order
+ cy.get("@variableLabels")
+ .first()
+ .should("have.text", "foo");
+ cy.get("@variableLabels")
+ .last()
+ .should("have.text", "bar");
+ });
});
--
GitLab