Fix missing columns after query fields update (#40398)

e2e/test/scenarios/models/reproductions/23421-visualization-settins-breaks-ui.cy.spec.js

-import { restore, openQuestionActions } from "e2e/support/helpers";
+import { restore, openQuestionActions, popover } from "e2e/support/helpers";
 
 const query = 'SELECT 1 AS "id", current_timestamp::timestamp AS "created_at"';
 
-const questionDetails = {
+const emptyColumnsQuestionDetails = {
   native: {
     query,
   },
@@ -15,28 +15,62 @@ const questionDetails = {
   type: "model",
 };
 
+const hiddenColumnsQuestionDetails = {
+  native: {
+    query,
+  },
+  displayIsLocked: true,
+  visualization_settings: {
+    "table.columns": [
+      {
+        name: "id",
+        key: '["name","id"]',
+        enabled: false,
+        fieldRef: ["field", "id", { "base-type": "type/Integer" }],
+      },
+      {
+        name: "created_at",
+        key: '["name","created_at"]',
+        enabled: false,
+        fieldRef: ["field", "created_at", { "base-type": "type/DateTime" }],
+      },
+    ],
+    "table.pivot_column": "orphaned1",
+    "table.cell_column": "orphaned2",
+  },
+  type: "model",
+};
+
 describe("issue 23421", () => {
   beforeEach(() => {
     restore();
     cy.signInAsAdmin();
-
-    cy.createNativeQuestion(questionDetails, { visitQuestion: true });
   });
 
   it("`visualization_settings` should not break UI (metabase#23421)", () => {
+    cy.createNativeQuestion(emptyColumnsQuestionDetails, {
+      visitQuestion: true,
+    });
     openQuestionActions();
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Edit query definition").click();
+    popover().findByText("Edit query definition").click();
 
     cy.get(".ace_content").should("contain", query);
-    // This test used to check for the presense of 4 cell data elements, implying that we should generate a default
-    // value for table.columns. However, as of #33841, having an empty table.columns setting is valid, so the
-    // assertion in this test has changed
-    cy.findByTestId("visualization-root").should(
-      "contain.text",
-      "Every field is hidden right now",
-    );
+    cy.findByRole("columnheader", { name: "id" }).should("be.visible");
+    cy.findByRole("columnheader", { name: "created_at" }).should("be.visible");
+    cy.button("Save changes").should("be.disabled");
+  });
+
+  it("`visualization_settings` with hidden columns should not break UI (metabase#23421)", () => {
+    cy.createNativeQuestion(hiddenColumnsQuestionDetails, {
+      visitQuestion: true,
+    });
+    openQuestionActions();
+    popover().findByText("Edit query definition").click();
 
+    cy.get(".ace_content").should("contain", query);
+    cy.findByTestId("visualization-root")
+      .findByText("Every field is hidden right now")
+      .should("be.visible");
     cy.button("Save changes").should("be.disabled");
   });
 });

e2e/test/scenarios/question/reproductions/40435-hidden-columns.cy.spec.js

+import {
+  getNotebookStep,
+  modal,
+  openNotebook,
+  openOrdersTable,
+  popover,
+  queryBuilderHeader,
+  restore,
+  saveQuestion,
+  visualize,
+} from "e2e/support/helpers";
+
+describe("issue 40435", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsNormalUser();
+    cy.intercept("PUT", "/api/card/*").as("updateCard");
+  });
+
+  it("should make new query columns visible by default (metabase#40435)", () => {
+    openOrdersTable();
+    openNotebook();
+    getNotebookStep("data").button("Pick columns").click();
+    popover().within(() => {
+      cy.findByText("Select none").click();
+      cy.findByText("User ID").click();
+    });
+    getNotebookStep("data").button("Pick columns").click();
+    visualize();
+    cy.findByTestId("viz-settings-button").click();
+    cy.findByTestId("sidebar-left").within(() => {
+      cy.findByTestId("ID-hide-button").click();
+      cy.findByTestId("ID-show-button").click();
+    });
+    saveQuestion();
+
+    openNotebook();
+    getNotebookStep("data").button("Pick columns").click();
+    popover().findByText("Product ID").click();
+    queryBuilderHeader().findByText("Save").click();
+    modal().last().findByText("Save").click();
+    cy.wait("@updateCard");
+    visualize();
+
+    cy.findByRole("columnheader", { name: "ID" }).should("be.visible");
+    cy.findByRole("columnheader", { name: "User ID" }).should("be.visible");
+    cy.findByRole("columnheader", { name: "Product ID" }).should("be.visible");
+  });
+});

frontend/src/metabase/visualizations/components/settings/ChartSettingTableColumns/TableColumnPanel/TableColumnPanel.unit.spec.tsx

@@ -105,16 +105,6 @@ describe("DatasetColumnSelector", () => {
     expect(items[3]).toHaveTextContent("Subtotal");
   });
 
-  it("should display columns without matching setting", () => {
-    setup({ columnSettings: [] });
-    const items = screen.getAllByTestId(/draggable-item/);
-    expect(items).toHaveLength(4);
-    expect(items[0]).toHaveTextContent("ID");
-    expect(items[1]).toHaveTextContent("Total");
-    expect(items[2]).toHaveTextContent("Tax");
-    expect(items[3]).toHaveTextContent("Subtotal");
-  });
-
   it("should allow to enable a column", () => {
     const { onChange } = setup();
 

frontend/src/metabase/visualizations/components/settings/ChartSettingTableColumns/TableColumnPanel/utils.ts

 import type { IconName } from "metabase/ui";
 import { getIconForField } from "metabase-lib/v1/metadata/utils/fields";
-import { findColumnSettingIndexesForColumns } from "metabase-lib/v1/queries/utils/dataset";
+import { findColumnIndexesForColumnSettings } from "metabase-lib/v1/queries/utils/dataset";
 import { getColumnKey } from "metabase-lib/v1/queries/utils/get-column-key";
 import type {
   DatasetColumn,
@@ -13,31 +13,16 @@ import type { ColumnItem } from "./types";
 
 export function getColumnItems(
   columns: DatasetColumn[],
-  originalSettings: TableColumnOrderSetting[],
+  columnSettings: TableColumnOrderSetting[],
 ): ColumnItem[] {
-  const originalIndexes = findColumnSettingIndexesForColumns(
+  const columnIndexes = findColumnIndexesForColumnSettings(
     columns,
-    originalSettings,
+    columnSettings,
   );
 
-  const updatedIndexes = [...originalIndexes];
-  const updatedSettings = [...originalSettings];
-  columns.forEach((column, columnIndex) => {
-    const columnSettingIndex = originalIndexes[columnIndex];
-    if (columnSettingIndex < 0) {
-      updatedIndexes[columnIndex] = updatedSettings.length;
-      updatedSettings.push({
-        name: column.name,
-        key: getColumnKey(column),
-        fieldRef: column.field_ref,
-        enabled: false,
-      });
-    }
-  });
-
-  const columnItems = columns.map((column, columnIndex) => {
-    const columnSettingIndex = updatedIndexes[columnIndex];
-    const columnSetting = updatedSettings[columnSettingIndex];
+  return columnSettings.map((columnSetting, columnSettingIndex) => {
+    const columnIndex = columnIndexes[columnSettingIndex];
+    const column = columns[columnIndex];
 
     return {
       name: column.name,
@@ -48,8 +33,6 @@ export function getColumnItems(
       columnSetting,
     };
   });
-
-  return columnItems.sort((a, b) => a.index - b.index);
 }
 
 export function toggleColumnInSettings(

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

@@ -27,6 +27,7 @@ import {
 import { getColumnKey } from "metabase-lib/v1/queries/utils/get-column-key";
 import {
   findColumnIndexesForColumnSettings,
+  findColumnSettingIndexesForColumns,
   getColumnSettingKey,
 } from "metabase-lib/v1/queries/utils/dataset";
 import { nestedSettings } from "./nested";
@@ -530,34 +531,42 @@ export const buildTableColumnSettings = ({
     // title: t`Columns`,
     widget: ChartSettingTableColumns,
     getHidden: (series, vizSettings) => vizSettings["table.pivot"],
-    getValue: (series, vizSettings) => {
-      function isValid([{ card, data }], columnSettings) {
+    getValue: ([{ data }], vizSettings) => {
+      const { cols } = data;
+
+      function isValid(columnSettings) {
         const columnIndexes = findColumnIndexesForColumnSettings(
-          data.cols,
+          cols,
           columnSettings.filter(({ enabled }) => enabled),
         );
         return columnIndexes.every(columnIndex => columnIndex >= 0);
       }
 
-      function getDefault([{ data }]) {
-        return data.cols.map(col => ({
-          name: col.name,
-          key: getColumnKey(col),
-          enabled: getIsColumnVisible(col),
-          fieldRef: col.field_ref,
-        }));
-      }
-
       function getValue(columnSettings) {
-        return columnSettings.map(setting => ({
-          ...setting,
-          key: getColumnSettingKey(setting),
-        }));
+        const settingIndexes = findColumnSettingIndexesForColumns(
+          cols,
+          columnSettings,
+        );
+
+        return [
+          ...columnSettings.map(setting => ({
+            ...setting,
+            key: getColumnSettingKey(setting),
+          })),
+          ...cols
+            .filter((_, columnIndex) => settingIndexes[columnIndex] < 0)
+            .map(column => ({
+              name: column.name,
+              key: getColumnKey(column),
+              enabled: getIsColumnVisible(column),
+              fieldRef: column.field_ref,
+            })),
+        ];
       }
 
       const columnSettings = vizSettings["table.columns"];
-      if (!columnSettings || !isValid(series, columnSettings)) {
-        return getDefault(series);
+      if (!columnSettings || !isValid(columnSettings)) {
+        return getValue([]);
       } else {
         return getValue(columnSettings);
       }