Fix changing column order/visibility via sidebar is inconsistent #13455 (#21338)

* Fix changing column order/visibility via sidebar is inconsistent #13455

* Fix can't reorder newly added column

https://github.com/metabase/metabase/pull/21338#pullrequestreview-928807257

frontend/src/metabase/query_builder/selectors.js

@@ -323,6 +323,7 @@ export function normalizeQuery(query, tableMetadata) {
     return query;
   }
   if (query.query) {
+    // sort query.fields
     if (tableMetadata) {
       query = updateIn(query, ["query", "fields"], fields => {
         fields = fields
@@ -335,6 +336,23 @@ export function normalizeQuery(query, tableMetadata) {
         );
       });
     }
+
+    // sort query.joins[int].fields
+    if (query.query.joins) {
+      query = updateIn(query, ["query", "joins"], joins =>
+        joins.map(joinedTable => {
+          if (!joinedTable.fields || joinedTable.fields === "all") {
+            return joinedTable;
+          }
+
+          const joinedTableFields = [...joinedTable.fields];
+          joinedTableFields.sort((a, b) =>
+            JSON.stringify(b).localeCompare(JSON.stringify(a)),
+          );
+          return { ...joinedTable, fields: joinedTableFields };
+        }),
+      );
+    }
     ["aggregation", "breakout", "filter", "joins", "order-by"].forEach(
       clauseList => {
         if (query.query[clauseList]) {

frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedColumns.jsx

@@ -59,9 +59,11 @@ export default class ChartSettingOrderedColumns extends Component {
   };
 
   handleSortEnd = ({ oldIndex, newIndex }) => {
-    const fields = [...this.props.value];
-    fields.splice(newIndex, 0, fields.splice(oldIndex, 1)[0]);
-    this.props.onChange(fields);
+    const enabledColumns = [...this.props.value].filter(columnSetting =>
+      findColumnForColumnSetting(this.props.columns, columnSetting),
+    );
+    enabledColumns.splice(newIndex, 0, enabledColumns.splice(oldIndex, 1)[0]);
+    this.props.onChange(enabledColumns);
   };
 
   handleEdit = columnSetting => {

frontend/src/metabase/visualizations/visualizations/Table.jsx

@@ -173,7 +173,9 @@ export default class Table extends Component {
         // otherwise it will be overwritten by `getDefault` below
         card.visualization_settings["table.columns"].length !== 0 &&
         _.all(
-          card.visualization_settings["table.columns"],
+          card.visualization_settings["table.columns"].filter(
+            columnSetting => columnSetting.enabled,
+          ),
           columnSetting =>
             findColumnIndexForColumnSetting(data.cols, columnSetting) >= 0,
         ),

frontend/test/metabase/scenarios/question/settings.cy.spec.js

@@ -72,7 +72,7 @@ describe("scenarios > question > settings", () => {
         .should("not.exist");
     });
 
-    it.skip("should preserve correct order of columns after column removal via sidebar (metabase#13455)", () => {
+    it("should preserve correct order of columns after column removal via sidebar (metabase#13455)", () => {
       cy.viewport(2000, 1200);
       // Orders join Products
       visitQuestionAdhoc({
@@ -130,18 +130,47 @@ describe("scenarios > question > settings", () => {
       cy.findByText("Visible columns").click();
       findColumnAtIndex("Products → Category", 5);
 
+      // https://github.com/metabase/metabase/pull/21338#pullrequestreview-928807257
+
+      // Add "Address"
+      cy.findByText("Address")
+        .siblings(".Icon-add")
+        .click();
       /**
        * Helper functions related to THIS test only
        */
+      // The result automatically load when adding new fields
+      cy.wait("@dataset");
+
+      // Refresh @sidebarColumns as we added a new field
+      cy.findByText("Click and drag to change their order")
+        .parent()
+        .find(".cursor-grab")
+        .as("sidebarColumns"); // Store all columns in an array
+
+      findColumnAtIndex("User → Address", -1).as("user-address");
+
+      cy.get("@user-address")
+        .trigger("mousedown", 0, 0, { force: true })
+        .trigger("mousemove", 5, 5, { force: true })
+        .trigger("mousemove", 0, -50, { force: true })
+        .trigger("mouseup", 0, -50, { force: true });
+
+      findColumnAtIndex("User → Address", 15);
 
       function reloadResults() {
         cy.icon("play")
           .last()
           .click();
+
+        // Prevent performing actions while the query is being executed.
+        // Which caused some race condition and failed the test.
+        cy.wait("@dataset");
       }
 
       function findColumnAtIndex(column_name, index) {
-        cy.get("@sidebarColumns")
+        return cy
+          .get("@sidebarColumns")
           .eq(index)
           .contains(column_name);
       }