pulling out column title logic and sharing with settings (#28937) (#29086)

* pulling out column title logic and sharing with settings

* Pivot Table Custom Column Names

* removing cyclic dependency

* adjusting tests

Co-authored-by: Nick Fitzpatrick <nick@metabase.com>

e2e/test/scenarios/binning/binning-reproductions.cy.spec.js

@@ -207,7 +207,7 @@ describe("binning related reproductions", () => {
     cy.findByTestId("sidebar-left").within(() => {
       cy.findByTestId("Table-button").click();
       cy.findByTextEnsureVisible("Table options");
-      cy.findByText("Created At")
+      cy.findByText("Created At: Month")
         .siblings("[data-testid$=hide-button]")
         .click();
       cy.button("Done").click();

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

@@ -102,6 +102,7 @@ class ChartSettingFieldsPartition extends React.Component {
         .filter(col => col != null),
     );
 
+    const { getColumnTitle } = this.props;
     return (
       <DragDropContext onDragEnd={this.handleDragEnd}>
         {this.props.partitions.map(({ name: partitionName, title }, index) => {
@@ -141,6 +142,7 @@ class ChartSettingFieldsPartition extends React.Component {
                                 column={col}
                                 index={index}
                                 onEditFormatting={this.handleEditFormatting}
+                                title={getColumnTitle(col)}
                               />
                             </div>
                           )}
@@ -170,11 +172,10 @@ class Column extends React.Component {
   };
 
   render() {
-    const { column } = this.props;
-
+    const { title } = this.props;
     return (
       <FieldPartitionColumn
-        title={column.display_name}
+        title={title}
         onEdit={this.handleEditFormatting}
         draggable
         isDisabled={false}

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

@@ -3,7 +3,6 @@ import React, { Component } from "react";
 import { t } from "ttag";
 import _ from "underscore";
 
-import { getFriendlyName } from "metabase/visualizations/lib/utils";
 import { getColumnKey } from "metabase-lib/queries/utils/get-column-key";
 import { findColumnForColumnSetting } from "metabase-lib/queries/utils/dataset";
 import StructuredQuery from "metabase-lib/queries/StructuredQuery";
@@ -57,12 +56,10 @@ export default class ChartSettingOrderedColumns extends Component {
     onChange(columnSettings);
   };
 
-  getColumnName = columnSetting =>
-    getFriendlyName(
-      findColumnForColumnSetting(this.props.columns, columnSetting) || {
-        display_name: "[Unknown]",
-      },
-    );
+  getColumnName = columnSetting => {
+    const { getColumnName } = this.props;
+    return getColumnName(columnSetting) || "[Unknown]";
+  };
 
   render() {
     const { value, question, columns } = this.props;
@@ -105,14 +102,16 @@ export default class ChartSettingOrderedColumns extends Component {
         {disabledColumns.length > 0 || additionalFieldOptions.count > 0 ? (
           <h4 className="mb2 mt4 pt4 border-top">{t`More columns`}</h4>
         ) : null}
-        {disabledColumns.map((columnSetting, index) => (
-          <ColumnItem
-            key={index}
-            title={this.getColumnName(columnSetting)}
-            onAdd={() => this.handleEnable(columnSetting)}
-            onClick={() => this.handleEnable(columnSetting)}
-          />
-        ))}
+        <div data-testid="disabled-columns">
+          {disabledColumns.map((columnSetting, index) => (
+            <ColumnItem
+              key={index}
+              title={this.getColumnName(columnSetting)}
+              onAdd={() => this.handleEnable(columnSetting)}
+              onClick={() => this.handleEnable(columnSetting)}
+            />
+          ))}
+        </div>
         {additionalFieldOptions.count > 0 && (
           <div>
             {additionalFieldOptions.dimensions.map((dimension, index) => (

frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.tsx

@@ -23,7 +23,6 @@ import {
   isPivotGroupColumn,
   multiLevelPivot,
 } from "metabase/lib/data_grid";
-import { formatColumn } from "metabase/lib/formatting";
 
 import type { DatasetData } from "metabase-types/types/Dataset";
 import type { VisualizationSettings } from "metabase-types/api";
@@ -61,7 +60,11 @@ import {
   LEFT_HEADER_LEFT_SPACING,
   MIN_HEADER_CELL_WIDTH,
 } from "./constants";
-import { settings, _columnSettings as columnSettings } from "./settings";
+import {
+  settings,
+  _columnSettings as columnSettings,
+  getTitleForColumn,
+} from "./settings";
 
 const mapStateToProps = (state: State) => ({
   fontFamily: getSetting(state, "application-font"),
@@ -131,11 +134,10 @@ function PivotTable({
 
   const getColumnTitle = useCallback(
     function (columnIndex) {
-      const columns = data.cols.filter(col => !isPivotGroupColumn(col));
-      const { column, column_title: columnTitle } = settings.column(
-        columns[columnIndex],
-      );
-      return columnTitle || formatColumn(column);
+      const column = data.cols.filter(col => !isPivotGroupColumn(col))[
+        columnIndex
+      ];
+      return getTitleForColumn(column, settings);
     },
     [data, settings],
   );

frontend/src/metabase/visualizations/visualizations/PivotTable/settings.ts

@@ -39,6 +39,15 @@ import {
 } from "./utils";
 import { PivotSetting } from "./types";
 
+export const getTitleForColumn = (
+  column: Column,
+  settings: VisualizationSettings,
+) => {
+  const { column: _column, column_title: columnTitle } =
+    settings.column(column);
+  return columnTitle || formatColumn(_column);
+};
+
 export const settings = {
   ...columnSettings({ hidden: true }),
   [COLLAPSED_ROWS_SETTING]: {
@@ -75,6 +84,9 @@ export const settings = {
       partitions,
       columns: data == null ? [] : data.cols,
       settings,
+      getColumnTitle: (column: Column) => {
+        return getTitleForColumn(column, settings);
+      },
     }),
     getValue: (
       [{ data, card }]: [{ data: DatasetData; card: Card }],

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

@@ -34,6 +34,17 @@ import * as Q_DEPRECATED from "metabase-lib/queries/utils";
 import TableSimple from "../components/TableSimple";
 import TableInteractive from "../components/TableInteractive/TableInteractive.jsx";
 
+const getTitleForColumn = (column, series, settings) => {
+  const isPivoted = Table.isPivoted(series, settings);
+  if (isPivoted) {
+    return formatColumn(column) || t`Unset`;
+  } else {
+    return (
+      settings.column(column)["_column_title_full"] || formatColumn(column)
+    );
+  }
+};
+
 export default class Table extends Component {
   static uiName = t`Table`;
   static identifier = "table";
@@ -189,13 +200,26 @@ export default class Table extends Component {
           fieldRef: col.field_ref,
           enabled: col.visibility_type !== "details-only",
         })),
-      getProps: ([
-        {
-          data: { cols },
-        },
-      ]) => ({
-        columns: cols,
-      }),
+      getProps: (series, settings) => {
+        const [
+          {
+            data: { cols },
+          },
+        ] = series;
+
+        return {
+          columns: cols,
+          getColumnName: columnSetting => {
+            const columnIndex = findColumnIndexForColumnSetting(
+              cols,
+              columnSetting,
+            );
+            if (columnIndex >= 0) {
+              return getTitleForColumn(cols[columnIndex], series, settings);
+            }
+          },
+        };
+      },
     },
     "table.column_widths": {},
     [DataGrid.COLUMN_FORMATTING_SETTING]: {
@@ -419,15 +443,7 @@ export default class Table extends Component {
       return null;
     }
     const { series, settings } = this.props;
-    const isPivoted = Table.isPivoted(series, settings);
-    const column = cols[columnIndex];
-    if (isPivoted) {
-      return formatColumn(column) || (columnIndex !== 0 ? t`Unset` : null);
-    } else {
-      return (
-        settings.column(column)["_column_title_full"] || formatColumn(column)
-      );
-    }
+    return getTitleForColumn(cols[columnIndex], series, settings);
   };
 
   render() {

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

 import React from "react";
-import { render, screen, fireEvent } from "@testing-library/react";
+import { render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
 import { ORDERS } from "__support__/sample_database_fixture";
 import ChartSettingOrderedColumns from "metabase/visualizations/components/settings/ChartSettingOrderedColumns";
 
@@ -8,6 +9,7 @@ function renderChartSettingOrderedColumns(props) {
     <ChartSettingOrderedColumns
       onChange={() => {}}
       columns={[{ name: "Foo" }, { name: "Bar" }]}
+      getColumnName={columnSetting => columnSetting.name}
       {...props}
     />,
   );
@@ -38,7 +40,7 @@ describe("ChartSettingOrderedColumns", () => {
     });
     const ADD = screen.getByRole("img", { name: /add/i });
 
-    fireEvent.click(ADD);
+    userEvent.click(ADD);
     expect(onChange.mock.calls).toEqual([
       [
         [
@@ -60,7 +62,7 @@ describe("ChartSettingOrderedColumns", () => {
     });
     const CLOSE = screen.getByRole("img", { name: /eye_outline/i });
 
-    fireEvent.click(CLOSE);
+    userEvent.click(CLOSE);
     expect(onChange.mock.calls).toEqual([
       [
         [
@@ -85,7 +87,7 @@ describe("ChartSettingOrderedColumns", () => {
       const FIRST = ADD_ICONS[0];
 
       expect(ADD_ICONS).toHaveLength(28);
-      fireEvent.click(FIRST);
+      userEvent.click(FIRST);
       expect(onChange.mock.calls).toEqual([
         [[{ fieldRef: ["field", 1, null], enabled: true }]],
       ]);

frontend/test/metabase/visualizations/visualizations/PivotTable.unit.spec.js

+import React, { useState } from "react";
+import { thaw } from "icepick";
+import userEvent from "@testing-library/user-event";
+import { render, screen } from "__support__/ui";
+
+import {
+  SAMPLE_DATABASE,
+  ORDERS,
+  PEOPLE,
+  metadata,
+} from "__support__/sample_database_fixture";
+import ChartSettings from "metabase/visualizations/components/ChartSettings";
+
+import Question from "metabase-lib/Question";
+
+const setup = () => {
+  const Container = () => {
+    const [question, setQuestion] = useState(
+      new Question(
+        {
+          dataset_query: {
+            type: "query",
+            query: {
+              "source-table": ORDERS.id,
+              aggregation: ["count"],
+              breakout: [
+                ["field", ORDERS.CREATED_AT.id, { "temporal-unit": "year" }],
+
+                [
+                  "field",
+                  PEOPLE.SOURCE.id,
+                  { "source-field": ORDERS.USER_ID.id },
+                ],
+              ],
+            },
+            database: SAMPLE_DATABASE.id,
+          },
+          display: "pivot",
+          visualization_settings: {},
+        },
+        metadata,
+      ),
+    );
+
+    const onChange = update => {
+      setQuestion(q => {
+        const newQuestion = q.updateSettings(update);
+        return new Question(thaw(newQuestion.card()), metadata);
+      });
+    };
+
+    return (
+      <ChartSettings
+        onChange={onChange}
+        series={[
+          {
+            card: question.card(),
+            data: {
+              rows: [],
+              // Need to add missing sources so that the setting displays
+              cols: [
+                ...question
+                  .query()
+                  .columns()
+                  .map(c => ({ ...c, source: c.source || "breakout" })),
+                {
+                  base_type: "type/Integer",
+                  name: "pivot-grouping",
+                  display_name: "pivot-grouping",
+                  expression_name: "pivot-grouping",
+                  field_ref: ["expression", "pivot-grouping"],
+                  source: "breakout",
+                  effective_type: "type/Integer",
+                },
+              ],
+            },
+          },
+        ]}
+        initial={{ section: "Data" }}
+        noPreview
+        question={question}
+      />
+    );
+  };
+
+  render(<Container />);
+};
+
+describe("table settings", () => {
+  it("should allow you to update a column name", async () => {
+    setup();
+    userEvent.click(await screen.findByTestId("Source-settings-button"));
+    userEvent.type(await screen.findByDisplayValue("Source"), " Updated");
+    userEvent.click(await screen.findByText("Count"));
+    expect(await screen.findByText("Source Updated")).toBeInTheDocument();
+  });
+});

frontend/test/metabase/visualizations/visualizations/Table.unit.spec.js

+import React, { useState } from "react";
+import { thaw } from "icepick";
+import userEvent from "@testing-library/user-event";
+import { renderWithProviders, screen, within } from "__support__/ui";
+
+import {
+  SAMPLE_DATABASE,
+  ORDERS,
+  metadata,
+} from "__support__/sample_database_fixture";
+import ChartSettings from "metabase/visualizations/components/ChartSettings";
+
+import Question from "metabase-lib/Question";
+
+const setup = () => {
+  const Container = () => {
+    const [question, setQuestion] = useState(
+      new Question(
+        {
+          dataset_query: {
+            type: "query",
+            query: {
+              "source-table": ORDERS.id,
+            },
+          },
+          database: SAMPLE_DATABASE.id,
+          visualization_settings: {},
+        },
+        metadata,
+      ),
+    );
+
+    const onChange = update => {
+      setQuestion(q => {
+        const newQuestion = q.updateSettings(update);
+        return new Question(thaw(newQuestion.card()), metadata);
+      });
+    };
+
+    return (
+      <ChartSettings
+        onChange={onChange}
+        series={[
+          {
+            card: question.card(),
+            data: {
+              rows: [],
+              cols: ORDERS.fields.map(f => f.column()),
+            },
+          },
+        ]}
+        initial={{ section: "Data" }}
+        noPreview
+        question={question}
+      />
+    );
+  };
+
+  renderWithProviders(<Container />);
+};
+
+describe("table settings", () => {
+  it("should show you related columns in structured queries", async () => {
+    setup();
+    expect(await screen.findByText("More columns")).toBeInTheDocument();
+    expect(await screen.findByText("People")).toBeInTheDocument();
+    expect(await screen.findByText("Products")).toBeInTheDocument();
+  });
+
+  it("should allow you to show and hide columns", async () => {
+    setup();
+    userEvent.click(await screen.findByTestId("Tax-hide-button"));
+
+    expect(
+      await within(await screen.findByTestId("disabled-columns")).findByText(
+        "Tax",
+      ),
+    ).toBeInTheDocument();
+
+    userEvent.click(await screen.findByTestId("Tax-add-button"));
+    //If we can see the hide button, then we know it's been added back in.
+    expect(await screen.findByTestId("Tax-hide-button")).toBeInTheDocument();
+  });
+
+  it("should allow you to update a column name", async () => {
+    setup();
+    userEvent.click(await screen.findByTestId("Subtotal-settings-button"));
+    userEvent.type(await screen.findByDisplayValue("Subtotal"), " Updated");
+    userEvent.click(await screen.findByText("Tax"));
+    expect(await screen.findByText("Subtotal Updated")).toBeInTheDocument();
+  });
+});