29287 column order hidden column (#31780)

* changing ChartSettingOrderedColumns to functional component

* Adding e2e test

* PR Feedback

e2e/test/scenarios/question/settings.cy.spec.js

@@ -6,6 +6,7 @@ import {
   visitQuestionAdhoc,
   popover,
   sidebar,
+  moveColumnDown,
 } from "e2e/support/helpers";
 
 import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
@@ -70,6 +71,22 @@ describe("scenarios > question > settings", () => {
       cy.get("@table").contains("Total").should("not.exist");
     });
 
+    it("should allow you to re-order columns even when one has been removed (metabase2#9287)", () => {
+      cy.viewport(1600, 800);
+
+      openOrdersTable();
+      cy.findByTestId("viz-settings-button").click();
+
+      cy.findByTestId("Subtotal-hide-button").click();
+      cy.findByTestId("Tax-hide-button").click();
+
+      getSidebarColumns().eq("3").as("total").contains("Total");
+
+      moveColumnDown(cy.get("@total"), -2);
+
+      getSidebarColumns().eq("1").should("contain.text", "Total");
+    });
+
     it.skip("should preserve correct order of columns after column removal via sidebar (metabase#13455)", () => {
       cy.viewport(2000, 1200);
       // Orders join Products

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

 /* eslint-disable react/prop-types */
-import { Component } from "react";
+import { useCallback, useMemo } from "react";
 import { t } from "ttag";
 import _ from "underscore";
 
@@ -11,136 +11,159 @@ import ColumnItem from "./ColumnItem";
 
 import { ChartSettingOrderedItems } from "./ChartSettingOrderedItems";
 
-export default class ChartSettingOrderedColumns extends Component {
-  handleEnable = columnSetting => {
-    const columnSettings = [...this.props.value];
-    const index = columnSetting.index;
-    columnSettings[index] = { ...columnSettings[index], enabled: true };
-    this.props.onChange(columnSettings);
-  };
+export const ChartSettingOrderedColumns = ({
+  value,
+  onChange,
+  question,
+  columns,
+  onShowWidget,
+  getColumnName: _getColumnName,
+}) => {
+  const query = question?.query();
+  const [enabledColumns, disabledColumns] = useMemo(
+    () =>
+      _.partition(
+        value
+          .filter(columnSetting =>
+            findColumnForColumnSetting(columns, columnSetting),
+          )
+          .map((columnSetting, index) => ({ ...columnSetting, index })),
+        columnSetting => columnSetting.enabled,
+      ),
+    [value, columns],
+  );
 
-  handleDisable = columnSetting => {
-    const columnSettings = [...this.props.value];
-    const index = columnSetting.index;
-    columnSettings[index] = { ...columnSettings[index], enabled: false };
-    this.props.onChange(columnSettings);
-  };
-
-  handleSortEnd = ({ oldIndex, newIndex }) => {
-    const fields = [...this.props.value];
-    fields.splice(newIndex, 0, fields.splice(oldIndex, 1)[0]);
-    this.props.onChange(fields);
-  };
-
-  handleEdit = (columnSetting, targetElement) => {
-    const column = findColumnForColumnSetting(
-      this.props.columns,
-      columnSetting,
-    );
-    if (column) {
-      this.props.onShowWidget(
-        {
-          id: "column_settings",
-          props: {
-            initialKey: getColumnKey(column),
-          },
-        },
-        targetElement,
+  let additionalFieldOptions = { count: 0 };
+  if (columns && query instanceof StructuredQuery) {
+    additionalFieldOptions = query.fieldsOptions(dimension => {
+      return !_.find(columns, column =>
+        dimension.isSameBaseDimension(column.field_ref),
       );
-    }
-  };
+    });
+  }
 
-  handleAddNewField = fieldRef => {
-    const { value, onChange } = this.props;
-    const columnSettings = [...value, { fieldRef, enabled: true }];
-    onChange(columnSettings);
-  };
+  const handleEnable = useCallback(
+    columnSetting => {
+      const columnSettings = [...value];
+      const index = columnSetting.index;
+      columnSettings[index] = { ...columnSettings[index], enabled: true };
+      onChange(columnSettings);
+    },
+    [value, onChange],
+  );
 
-  getColumnName = columnSetting => {
-    const { getColumnName } = this.props;
-    return getColumnName(columnSetting) || "[Unknown]";
-  };
+  const handleDisable = useCallback(
+    columnSetting => {
+      const columnSettings = [...value];
+      const index = columnSetting.index;
+      columnSettings[index] = { ...columnSettings[index], enabled: false };
+      onChange(columnSettings);
+    },
+    [value, onChange],
+  );
 
-  render() {
-    const { value, question, columns } = this.props;
-    const query = question && question.query();
+  const handleSortEnd = useCallback(
+    ({ oldIndex, newIndex }) => {
+      const adjustedOldIndex = enabledColumns[oldIndex].index;
+      const adjustedNewIndex = enabledColumns[newIndex].index;
 
-    let additionalFieldOptions = { count: 0 };
-    if (columns && query instanceof StructuredQuery) {
-      additionalFieldOptions = query.fieldsOptions(dimension => {
-        return !_.find(columns, column =>
-          dimension.isSameBaseDimension(column.field_ref),
+      const fields = [...value];
+      fields.splice(adjustedNewIndex, 0, fields.splice(adjustedOldIndex, 1)[0]);
+      onChange(fields);
+    },
+    [value, onChange, enabledColumns],
+  );
+
+  const handleEdit = useCallback(
+    (columnSetting, targetElement) => {
+      const column = findColumnForColumnSetting(columns, columnSetting);
+      if (column) {
+        onShowWidget(
+          {
+            id: "column_settings",
+            props: {
+              initialKey: getColumnKey(column),
+            },
+          },
+          targetElement,
         );
-      });
-    }
+      }
+    },
+    [onShowWidget, columns],
+  );
+
+  const handleAddNewField = useCallback(
+    fieldRef => {
+      const columnSettings = [...value, { fieldRef, enabled: true }];
+      onChange(columnSettings);
+    },
+    [value, onChange],
+  );
 
-    const [enabledColumns, disabledColumns] = _.partition(
-      value
-        .filter(columnSetting =>
-          findColumnForColumnSetting(columns, columnSetting),
-        )
-        .map((columnSetting, index) => ({ ...columnSetting, index })),
-      columnSetting => columnSetting.enabled,
-    );
+  const getColumnName = useCallback(
+    columnSetting => {
+      return _getColumnName(columnSetting) || "[Unknown]";
+    },
+    [_getColumnName],
+  );
 
-    return (
-      <div className="list">
-        {enabledColumns.length > 0 ? (
-          <ChartSettingOrderedItems
-            items={enabledColumns}
-            getItemName={this.getColumnName}
-            onEdit={this.handleEdit}
-            onRemove={this.handleDisable}
-            onSortEnd={this.handleSortEnd}
-            distance={5}
+  return (
+    <div className="list">
+      {enabledColumns.length > 0 ? (
+        <ChartSettingOrderedItems
+          items={enabledColumns}
+          getItemName={getColumnName}
+          onEdit={handleEdit}
+          onRemove={handleDisable}
+          onSortEnd={handleSortEnd}
+          distance={5}
+        />
+      ) : (
+        <div className="my2 p2 flex layout-centered bg-grey-0 text-light text-bold rounded">
+          {t`Add fields from the list below`}
+        </div>
+      )}
+      {disabledColumns.length > 0 || additionalFieldOptions.count > 0 ? (
+        <h4 className="mb2 mt4 pt4 border-top">{t`More columns`}</h4>
+      ) : null}
+      <div data-testid="disabled-columns">
+        {disabledColumns.map((columnSetting, index) => (
+          <ColumnItem
+            key={index}
+            title={getColumnName(columnSetting)}
+            onAdd={() => handleEnable(columnSetting)}
+            onClick={() => handleEnable(columnSetting)}
           />
-        ) : (
-          <div className="my2 p2 flex layout-centered bg-grey-0 text-light text-bold rounded">
-            {t`Add fields from the list below`}
-          </div>
-        )}
-        {disabledColumns.length > 0 || additionalFieldOptions.count > 0 ? (
-          <h4 className="mb2 mt4 pt4 border-top">{t`More columns`}</h4>
-        ) : null}
-        <div data-testid="disabled-columns">
-          {disabledColumns.map((columnSetting, index) => (
+        ))}
+      </div>
+      {additionalFieldOptions.count > 0 && (
+        <div>
+          {additionalFieldOptions.dimensions.map((dimension, index) => (
             <ColumnItem
               key={index}
-              title={this.getColumnName(columnSetting)}
-              onAdd={() => this.handleEnable(columnSetting)}
-              onClick={() => this.handleEnable(columnSetting)}
+              title={dimension.displayName()}
+              onAdd={() => handleAddNewField(dimension.mbql())}
             />
           ))}
-        </div>
-        {additionalFieldOptions.count > 0 && (
-          <div>
-            {additionalFieldOptions.dimensions.map((dimension, index) => (
-              <ColumnItem
-                key={index}
-                title={dimension.displayName()}
-                onAdd={() => this.handleAddNewField(dimension.mbql())}
-              />
-            ))}
-            {additionalFieldOptions.fks.map((fk, index) => (
-              <div key={fk.id}>
-                <div className="my2 text-medium text-bold text-uppercase text-small">
-                  {fk.name ||
-                    (fk.field.target
-                      ? fk.field.target.table.display_name
-                      : fk.field.display_name)}
-                </div>
-                {fk.dimensions.map((dimension, index) => (
-                  <ColumnItem
-                    key={index}
-                    title={dimension.displayName()}
-                    onAdd={() => this.handleAddNewField(dimension.mbql())}
-                  />
-                ))}
+          {additionalFieldOptions.fks.map((fk, index) => (
+            <div key={fk.id}>
+              <div className="my2 text-medium text-bold text-uppercase text-small">
+                {fk.name ||
+                  (fk.field.target
+                    ? fk.field.target.table.display_name
+                    : fk.field.display_name)}
               </div>
-            ))}
-          </div>
-        )}
-      </div>
-    );
-  }
-}
+              {fk.dimensions.map((dimension, index) => (
+                <ColumnItem
+                  key={index}
+                  title={dimension.displayName()}
+                  onAdd={() => handleAddNewField(dimension.mbql())}
+                />
+              ))}
+            </div>
+          ))}
+        </div>
+      )}
+    </div>
+  );
+};

frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedColumns.unit.spec.js

 import { render, screen } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 import { createMockMetadata } from "__support__/metadata";
-import ChartSettingOrderedColumns from "metabase/visualizations/components/settings/ChartSettingOrderedColumns";
+import { ChartSettingOrderedColumns } from "metabase/visualizations/components/settings/ChartSettingOrderedColumns";
 import {
   ORDERS,
   ORDERS_ID,

frontend/src/metabase/visualizations/lib/settings/column.js

@@ -3,7 +3,7 @@ import moment from "moment-timezone";
 import _ from "underscore";
 
 import ChartNestedSettingColumns from "metabase/visualizations/components/settings/ChartNestedSettingColumns";
-import ChartSettingOrderedColumns from "metabase/visualizations/components/settings/ChartSettingOrderedColumns";
+import { ChartSettingOrderedColumns } from "metabase/visualizations/components/settings/ChartSettingOrderedColumns";
 
 // HACK: cyclical dependency causing errors in unit tests
 // import { getVisualizationRaw } from "metabase/visualizations";