Sort fields before checking if a query has been updated (#11311)

frontend/src/metabase/query_builder/selectors.js

@@ -2,7 +2,7 @@
 
 import { createSelector } from "reselect";
 import _ from "underscore";
-import { getIn } from "icepick";
+import { getIn, updateIn } from "icepick";
 
 // Needed due to wrong dependency resolution order
 // eslint-disable-next-line no-unused-vars
@@ -149,10 +149,36 @@ export const getIsResultDirty = createSelector(
     getNextRunDatasetQuery,
     getLastRunParameterValues,
     getNextRunParameterValues,
+    getTableMetadata,
   ],
-  (lastDatasetQuery, nextDatasetQuery, lastParameters, nextParameters) =>
-    !Utils.equals(lastDatasetQuery, nextDatasetQuery) ||
-    !Utils.equals(lastParameters, nextParameters),
+  (
+    lastDatasetQuery,
+    nextDatasetQuery,
+    lastParameters,
+    nextParameters,
+    tableMetadata,
+  ) => {
+    // this function sorts fields so that reordering doesn't dirty the result
+    const queryWithSortedFields = query =>
+      query && query.query && tableMetadata
+        ? updateIn(query, ["query", "fields"], fields => {
+            fields = fields
+              ? // if the query has fields, copy them before sorting
+                [...fields]
+              : // if the fields aren't set, we get them from the table metadata
+                tableMetadata.fields.map(({ id }) => ["field-id", id]);
+            return fields.sort((a, b) =>
+              JSON.stringify(b).localeCompare(JSON.stringify(a)),
+            );
+          })
+        : query;
+    lastDatasetQuery = queryWithSortedFields(lastDatasetQuery);
+    nextDatasetQuery = queryWithSortedFields(nextDatasetQuery);
+    return (
+      !Utils.equals(lastDatasetQuery, nextDatasetQuery) ||
+      !Utils.equals(lastParameters, nextParameters)
+    );
+  },
 );
 
 export const getQuestion = createSelector(

frontend/test/metabase/query_builder/selectors.unit.spec.js

+import { getIsResultDirty } from "metabase/query_builder/selectors";
+import { state as sampleState } from "__support__/sample_dataset_fixture";
+
+describe("getIsResultDirty", () => {
+  function getState(q1, q2) {
+    const card = query => ({
+      dataset_query: { database: 1, type: "query", query },
+    });
+    const qb = { lastRunCard: card(q1), card: card(q2) };
+    return { ...sampleState, qb };
+  }
+
+  it("should not be dirty for empty queries", () => {
+    const state = getState({}, {});
+    expect(getIsResultDirty(state)).toBe(false);
+  });
+
+  it("should be dirty if the table was changed", () => {
+    const state = getState({ "source-table": 1 }, { "source-table": 2 });
+    expect(getIsResultDirty(state)).toBe(true);
+  });
+
+  it("should be dirty if the fields were changed", () => {
+    const state = getState(
+      { "source-table": 1, fields: [["field-id", 1]] },
+      { "source-table": 1, fields: [["field-id", 2]] },
+    );
+    expect(getIsResultDirty(state)).toBe(true);
+  });
+
+  it("should not be dirty if the fields were reordered", () => {
+    const state = getState(
+      { "source-table": 1, fields: [["field-id", 1], ["field-id", 2]] },
+      { "source-table": 1, fields: [["field-id", 2], ["field-id", 1]] },
+    );
+    expect(getIsResultDirty(state)).toBe(false);
+  });
+
+  it("should not be dirty if fields with fk refs were reordered", () => {
+    const state = getState(
+      {
+        "source-table": 1,
+        fields: [["fk->", ["field-id", 1], ["field-id", 2]], ["field-id", 1]],
+      },
+      {
+        "source-table": 1,
+        fields: [["field-id", 1], ["fk->", ["field-id", 1], ["field-id", 2]]],
+      },
+    );
+    expect(getIsResultDirty(state)).toBe(false);
+  });
+
+  it("should not be dirty if fields were just made explicit", () => {
+    const state = getState(
+      { "source-table": 1 },
+      {
+        "source-table": 1,
+        fields: [
+          ["field-id", 1],
+          ["field-id", 2],
+          ["field-id", 3],
+          ["field-id", 4],
+          ["field-id", 5],
+          ["field-id", 6],
+          ["field-id", 7],
+        ],
+      },
+    );
+    expect(getIsResultDirty(state)).toBe(false);
+  });
+});