diff --git a/frontend/src/metabase/query_builder/actions.js b/frontend/src/metabase/query_builder/actions.js index 66027a73f8dae548e4e346cf99ca80e2e2f8f428..55c2958540b2db4cf272db482f40100528733de5 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 18b3755e0edf2161f1ae3767a29ce5bd4cab0f0a..af34f0a0531fccf63c09df81c91c4eb2139266d8 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 ac957f88ab1c6cd57b54941b098a3132ef54c9b3..90204ed8a5bda4705196b551eb7b78dbbbfe66d2 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"); + }); });