Extract action column placement (#39958)

e2e/test/scenarios/visualizations-tabular/drillthroughs/column_extract_drill.cy.spec.js

 import _ from "underscore";
 
 import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+import { ORDERS_QUESTION_ID } from "e2e/support/cypress_sample_instance_data";
 import {
   enterCustomColumnDetails,
   getNotebookStep,
@@ -8,6 +9,7 @@ import {
   openOrdersTable,
   popover,
   restore,
+  visitQuestion,
   visualize,
 } from "e2e/support/helpers";
 
@@ -77,6 +79,96 @@ describe("extract action", () => {
       });
     });
 
+    describe("should add a new column after the selected column", () => {
+      it("ad-hoc question", () => {
+        openOrdersTable();
+        extractColumnAndCheck({
+          column: "Created At",
+          option: "Year",
+        });
+        const columnIndex = 7;
+        checkColumnIndex({
+          column: "Created At",
+          columnIndex,
+        });
+        checkColumnIndex({
+          column: "Year",
+          columnIndex: columnIndex + 1,
+        });
+      });
+
+      it("saved question without viz settings", () => {
+        visitQuestion(ORDERS_QUESTION_ID);
+        extractColumnAndCheck({
+          column: "Created At",
+          option: "Year",
+        });
+        const columnIndex = 7;
+        checkColumnIndex({
+          column: "Created At",
+          columnIndex,
+        });
+        checkColumnIndex({
+          column: "Year",
+          columnIndex: columnIndex + 1,
+        });
+      });
+
+      it("saved question with viz settings", () => {
+        cy.createQuestion(
+          {
+            query: {
+              "source-table": ORDERS_ID,
+              fields: [
+                ["field", ORDERS.ID, { "base-type": "type/BigInteger" }],
+                ["field", ORDERS.CREATED_AT, { "base-type": "type/DateTime" }],
+                ["field", ORDERS.QUANTITY, { "base-type": "type/Integer" }],
+              ],
+            },
+            visualization_settings: {
+              "table.columns": [
+                {
+                  name: "ID",
+                  fieldRef: ["field", ORDERS.ID, null],
+                  enabled: true,
+                },
+                {
+                  name: "CREATED_AT",
+                  fieldRef: [
+                    "field",
+                    ORDERS.CREATED_AT,
+                    {
+                      "temporal-unit": "default",
+                    },
+                  ],
+                  enabled: true,
+                },
+                {
+                  name: "QUANTITY",
+                  fieldRef: ["field", ORDERS.QUANTITY, null],
+                  enabled: true,
+                },
+              ],
+            },
+          },
+          { visitQuestion: true },
+        );
+        extractColumnAndCheck({
+          column: "Created At",
+          option: "Year",
+        });
+        const columnIndex = 1;
+        checkColumnIndex({
+          column: "Created At",
+          columnIndex,
+        });
+        checkColumnIndex({
+          column: "Year",
+          columnIndex: columnIndex + 1,
+        });
+      });
+    });
+
     it("should add an expression based on a breakout column", () => {
       cy.createQuestion(DATE_QUESTION, { visitQuestion: true });
       extractColumnAndCheck({
@@ -151,3 +243,7 @@ function extractColumnAndCheck({ column, option, newColumn = option, value }) {
     cy.findByRole("gridcell", { name: value }).should("be.visible");
   }
 }
+
+function checkColumnIndex({ column, columnIndex }) {
+  cy.findAllByRole("columnheader").eq(columnIndex).should("have.text", column);
+}

frontend/src/metabase-lib/v1/Question.ts

@@ -45,10 +45,6 @@ import { getTemplateTagParametersFromCard } from "metabase-lib/v1/parameters/uti
 import { fieldFilterParameterToFilter } from "metabase-lib/v1/parameters/utils/mbql";
 import { getQuestionVirtualTableId } from "metabase-lib/v1/metadata/utils/saved-questions";
 import { isTransientId } from "metabase-lib/v1/queries/utils/card";
-import {
-  findColumnIndexesForColumnSettings,
-  findColumnSettingIndexesForColumns,
-} from "metabase-lib/v1/queries/utils/dataset";
 import {
   ALERT_TYPE_PROGRESS_BAR_GOAL,
   ALERT_TYPE_ROWS,
@@ -56,7 +52,6 @@ import {
 } from "metabase-lib/v1/Alert";
 
 import type { Query } from "../types";
-import { getColumnKey } from "metabase-lib/v1/queries/utils/get-column-key";
 
 export type QuestionCreatorOpts = {
   databaseId?: DatabaseId;
@@ -470,110 +465,24 @@ class Question {
     });
   }
 
-  private _syncGraphMetricSettings(
-    { data: { cols = [] } = {} }: Dataset,
-    { data: { cols: prevCols = [] } = {} }: Dataset,
+  syncColumnsAndSettings(
+    queryResults?: Dataset,
+    prevQueryResults?: Dataset,
+    options?: Lib.SettingsSyncOptions,
   ) {
-    const graphMetrics = this.setting("graph.metrics");
-    if (!graphMetrics) {
-      return this;
-    }
-
-    const hasNativeColumns =
-      cols.some(column => column.source === "native") ||
-      prevCols.some(column => column.source === "native");
-    if (hasNativeColumns) {
-      return this;
-    }
-
-    const metricColumnNames = new Set(
-      cols
-        .filter(column => column.source === "aggregation")
-        .map(column => column.name),
-    );
-    const prevMetricColumnNames = new Set(
-      prevCols
-        .filter(column => column.source === "aggregation")
-        .map(column => column.name),
-    );
-    const addedMetricColumnNames = new Set(
-      [...metricColumnNames].filter(name => !prevMetricColumnNames.has(name)),
-    );
-    const removedMetricColumnNames = new Set(
-      [...prevMetricColumnNames].filter(name => !metricColumnNames.has(name)),
+    const settings = this.settings();
+    const newSettings = Lib.syncColumnSettings(
+      settings,
+      queryResults,
+      prevQueryResults,
+      options,
     );
 
-    if (addedMetricColumnNames.size > 0 || removedMetricColumnNames.size > 0) {
-      return this.updateSettings({
-        "graph.metrics": [
-          ...graphMetrics.filter(name => !removedMetricColumnNames.has(name)),
-          ...addedMetricColumnNames,
-        ],
-      });
-    }
-
-    return this;
-  }
-
-  _syncTableColumnSettings({ data = {} }: Dataset) {
-    const columnSettings = this.setting("table.columns") || [];
-    // "table.columns" receive a value only if there are custom settings
-    // e.g. some columns are hidden. If it's empty, it means everything is visible
-    const isUsingDefaultSettings = columnSettings.length === 0;
-    if (isUsingDefaultSettings) {
-      return this;
-    }
-
-    // remove columns used for remapping only
-    const cols = data.cols.filter(col => col.remapped_from == null);
-    const columnIndexes = findColumnIndexesForColumnSettings(
-      cols,
-      columnSettings,
-    );
-    const columnSettingIndexes = findColumnSettingIndexesForColumns(
-      cols,
-      columnSettings,
-    );
-    const addedColumns = cols.filter((col, colIndex) => {
-      const hasVizSettings = columnSettingIndexes[colIndex] >= 0;
-      return !hasVizSettings;
-    });
-    const existingColumnSettings = columnSettings.filter(
-      (setting, settingIndex) => columnIndexes[settingIndex] >= 0,
-    );
-    const noColumnsRemoved =
-      existingColumnSettings.length === columnSettings.length;
-
-    if (noColumnsRemoved && addedColumns.length === 0) {
+    if (newSettings !== settings) {
+      return this.setSettings(newSettings);
+    } else {
       return this;
     }
-
-    const addedColumnSettings = addedColumns.map(col => ({
-      name: col.name,
-      key: getColumnKey(col),
-      fieldRef: col.field_ref,
-      enabled: true,
-    }));
-    return this.updateSettings({
-      "table.columns": [...existingColumnSettings, ...addedColumnSettings],
-    });
-  }
-
-  syncColumnsAndSettings(queryResults?: Dataset, prevQueryResults?: Dataset) {
-    let question = this;
-
-    if (queryResults && !queryResults.error) {
-      question = question._syncTableColumnSettings(queryResults);
-
-      if (prevQueryResults && !prevQueryResults.error) {
-        question = question._syncGraphMetricSettings(
-          queryResults,
-          prevQueryResults,
-        );
-      }
-    }
-
-    return question;
   }
 
   /**

frontend/src/metabase-lib/viz/column_settings.ts

+import {
+  findColumnIndexesForColumnSettings,
+  findColumnSettingIndexesForColumns,
+} from "metabase-lib/v1/queries/utils/dataset";
+import { getColumnKey } from "metabase-lib/v1/queries/utils/get-column-key";
+import type {
+  Dataset,
+  DatasetColumn,
+  VisualizationSettings,
+} from "metabase-types/api";
+
+export type SettingsSyncOptions = {
+  column: DatasetColumn;
+};
+
+export function syncColumnSettings(
+  settings: VisualizationSettings,
+  queryResults?: Dataset,
+  prevQueryResults?: Dataset,
+  options?: SettingsSyncOptions,
+): VisualizationSettings {
+  let newSettings = settings;
+
+  if (queryResults && !queryResults.error) {
+    newSettings = syncTableColumnSettings(newSettings, queryResults, options);
+
+    if (prevQueryResults && !prevQueryResults.error) {
+      newSettings = syncGraphMetricSettings(
+        newSettings,
+        queryResults,
+        prevQueryResults,
+      );
+
+      if (options) {
+        newSettings = moveNewTableColumnsAfterColumn(
+          newSettings,
+          queryResults,
+          prevQueryResults,
+          options,
+        );
+      }
+    }
+  }
+
+  return newSettings;
+}
+
+function syncTableColumnSettings(
+  settings: VisualizationSettings,
+  { data }: Dataset,
+  options?: SettingsSyncOptions,
+): VisualizationSettings {
+  // "table.columns" receive a value only if there are custom settings
+  // e.g. some columns are hidden. If it's empty, it means everything is visible
+  const columnSettings = settings["table.columns"] ?? [];
+  if (columnSettings.length === 0 && !options?.column) {
+    return settings;
+  }
+
+  // remove columns used for remapping only
+  const cols = data.cols.filter(col => col.remapped_from == null);
+  const columnIndexes = findColumnIndexesForColumnSettings(
+    cols,
+    columnSettings,
+  );
+  const columnSettingIndexes = findColumnSettingIndexesForColumns(
+    cols,
+    columnSettings,
+  );
+  const addedColumns = cols.filter((col, colIndex) => {
+    const hasVizSettings = columnSettingIndexes[colIndex] >= 0;
+    return !hasVizSettings;
+  });
+  const existingColumnSettings = columnSettings.filter(
+    (setting, settingIndex) => columnIndexes[settingIndex] >= 0,
+  );
+  const noColumnsRemoved =
+    existingColumnSettings.length === columnSettings.length;
+
+  if (noColumnsRemoved && addedColumns.length === 0) {
+    return settings;
+  }
+
+  const addedColumnSettings = addedColumns.map(col => ({
+    name: col.name,
+    key: getColumnKey(col),
+    fieldRef: col.field_ref,
+    enabled: true,
+  }));
+  return {
+    ...settings,
+    "table.columns": [...existingColumnSettings, ...addedColumnSettings],
+  };
+}
+
+function moveNewTableColumnsAfterColumn(
+  settings: VisualizationSettings,
+  { data: { cols } }: Dataset,
+  { data: { cols: prevCols } }: Dataset,
+  { column }: SettingsSyncOptions,
+): VisualizationSettings {
+  const columnSettings = settings["table.columns"];
+  if (!column || !columnSettings) {
+    return settings;
+  }
+
+  const prevColumnNames = new Set(prevCols.map(col => col.name));
+  const addedColumns = cols.filter(col => !prevColumnNames.has(col.name));
+  const addedColumnSettingIndexes = findColumnSettingIndexesForColumns(
+    addedColumns,
+    columnSettings,
+  );
+  const addedColumnSettings = addedColumnSettingIndexes.map(
+    index => columnSettings[index],
+  );
+  const existingColumnSettings = columnSettings.filter(
+    (_, index) => !addedColumnSettingIndexes.includes(index),
+  );
+  const [columnSettingIndex] = findColumnSettingIndexesForColumns(
+    [column],
+    existingColumnSettings,
+  );
+  if (columnSettingIndex < 0) {
+    return settings;
+  }
+
+  return {
+    ...settings,
+    "table.columns": [
+      ...existingColumnSettings.slice(0, columnSettingIndex + 1), // before and including the selected column
+      ...addedColumnSettings,
+      ...existingColumnSettings.slice(columnSettingIndex + 1), // after the selected column
+    ],
+  };
+}
+
+function syncGraphMetricSettings(
+  settings: VisualizationSettings,
+  { data: { cols } }: Dataset,
+  { data: { cols: prevCols } }: Dataset,
+): VisualizationSettings {
+  const graphMetrics = settings["graph.metrics"];
+  if (!graphMetrics) {
+    return settings;
+  }
+
+  const hasNativeColumns =
+    cols.some(column => column.source === "native") ||
+    prevCols.some(column => column.source === "native");
+  if (hasNativeColumns) {
+    return settings;
+  }
+
+  const metricColumnNames = new Set(
+    cols
+      .filter(column => column.source === "aggregation")
+      .map(column => column.name),
+  );
+  const prevMetricColumnNames = new Set(
+    prevCols
+      .filter(column => column.source === "aggregation")
+      .map(column => column.name),
+  );
+  const addedMetricColumnNames = new Set(
+    [...metricColumnNames].filter(name => !prevMetricColumnNames.has(name)),
+  );
+  const removedMetricColumnNames = new Set(
+    [...prevMetricColumnNames].filter(name => !metricColumnNames.has(name)),
+  );
+  if (
+    addedMetricColumnNames.size === 0 &&
+    removedMetricColumnNames.size === 0
+  ) {
+    return settings;
+  }
+
+  return {
+    ...settings,
+    "graph.metrics": [
+      ...graphMetrics.filter(name => !removedMetricColumnNames.has(name)),
+      ...addedMetricColumnNames,
+    ],
+  };
+}

frontend/src/metabase-lib/viz/index.ts

 export * from "./display";
+export * from "./column_settings";

frontend/src/metabase/query_builder/actions/core/core.js

@@ -34,6 +34,7 @@ import {
   isBasedOnExistingQuestion,
   getParameters,
   getSubmittableQuestion,
+  getQueryResults,
 } from "../../selectors";
 import { updateUrl } from "../navigation";
 import { zoomInRow } from "../object-detail";
@@ -111,7 +112,10 @@ export const reloadCard = createThunkAction(RELOAD_CARD, () => {
  *     - `navigateToNewCardInsideQB` is being called (see below)
  */
 export const SET_CARD_AND_RUN = "metabase/qb/SET_CARD_AND_RUN";
-export const setCardAndRun = (nextCard, shouldUpdateUrl = true) => {
+export const setCardAndRun = (
+  nextCard,
+  { shouldUpdateUrl = true, prevQueryResults, settingsSyncOptions } = {},
+) => {
   return async (dispatch, getState) => {
     // clone
     const card = copy(nextCard);
@@ -127,7 +131,13 @@ export const setCardAndRun = (nextCard, shouldUpdateUrl = true) => {
 
     // Update the card and originalCard before running the actual query
     dispatch({ type: SET_CARD_AND_RUN, payload: { card, originalCard } });
-    dispatch(runQuestionQuery({ shouldUpdateUrl }));
+    dispatch(
+      runQuestionQuery({
+        shouldUpdateUrl,
+        prevQueryResults,
+        settingsSyncOptions,
+      }),
+    );
 
     // Load table & database metadata for the current question
     dispatch(loadMetadataForCard(card));
@@ -147,14 +157,16 @@ export const setCardAndRun = (nextCard, shouldUpdateUrl = true) => {
 export const NAVIGATE_TO_NEW_CARD = "metabase/qb/NAVIGATE_TO_NEW_CARD";
 export const navigateToNewCardInsideQB = createThunkAction(
   NAVIGATE_TO_NEW_CARD,
-  ({ nextCard, previousCard, objectId }) => {
+  ({ nextCard, previousCard, objectId, settingsSyncOptions }) => {
     return async (dispatch, getState) => {
       if (previousCard === nextCard) {
         // Do not reload questions with breakouts when clicked on a legend item
       } else if (cardIsEquivalent(previousCard, nextCard)) {
         // This is mainly a fallback for scenarios where a visualization legend is clicked inside QB
         dispatch(
-          setCardAndRun(await loadCard(nextCard.id, { dispatch, getState })),
+          setCardAndRun(await loadCard(nextCard.id, { dispatch, getState }), {
+            settingsSyncOptions,
+          }),
         );
       } else {
         const card = getCardAfterVisualizationClick(nextCard, previousCard);
@@ -163,13 +175,19 @@ export const navigateToNewCardInsideQB = createThunkAction(
           dispatch(openUrl(url));
         } else {
           dispatch(onCloseSidebars());
+          const prevQueryResults = getQueryResults(getState());
           if (!cardQueryIsEquivalent(previousCard, nextCard)) {
             // clear the query result so we don't try to display the new visualization before running the new query
             dispatch(clearQueryResult());
           }
           // When the dataset query changes, we should change the type,
           // to start building a new ad-hoc question based on a dataset
-          dispatch(setCardAndRun({ ...card, type: "question" }));
+          dispatch(
+            setCardAndRun(
+              { ...card, type: "question" },
+              { prevQueryResults, settingsSyncOptions },
+            ),
+          );
         }
         if (objectId !== undefined) {
           dispatch(zoomInRow({ objectId }));

frontend/src/metabase/query_builder/actions/navigation.js

@@ -57,8 +57,8 @@ export const popState = createThunkAction(
     const card = getCard(getState());
     if (location.state && location.state.card) {
       if (!equals(card, location.state.card)) {
-        const shouldRefreshUrl = location.state.card.type === "model";
-        await dispatch(setCardAndRun(location.state.card, shouldRefreshUrl));
+        const shouldUpdateUrl = location.state.card.type === "model";
+        await dispatch(setCardAndRun(location.state.card, { shouldUpdateUrl }));
         await dispatch(setCurrentState(location.state));
         await dispatch(resetUIControls());
       }

frontend/src/metabase/query_builder/actions/querying.js

@@ -94,6 +94,8 @@ export const runQuestionQuery = ({
   shouldUpdateUrl = true,
   ignoreCache = false,
   overrideWithQuestion = null,
+  prevQueryResults = undefined,
+  settingsSyncOptions = undefined,
 } = {}) => {
   return async (dispatch, getState) => {
     dispatch(loadStartUIControls());
@@ -135,7 +137,12 @@ export const runQuestionQuery = ({
             duration,
           ),
         );
-        return dispatch(queryCompleted(question, queryResults));
+        return dispatch(
+          queryCompleted(question, queryResults, {
+            prevQueryResults: prevQueryResults ?? getQueryResults(getState()),
+            settingsSyncOptions,
+          }),
+        );
       })
       .catch(error => dispatch(queryErrored(startTime, error)));
 
@@ -168,10 +175,13 @@ export const CLEAR_QUERY_RESULT = "metabase/query_builder/CLEAR_QUERY_RESULT";
 export const clearQueryResult = createAction(CLEAR_QUERY_RESULT);
 
 export const QUERY_COMPLETED = "metabase/qb/QUERY_COMPLETED";
-export const queryCompleted = (question, queryResults) => {
+export const queryCompleted = (
+  question,
+  queryResults,
+  { prevQueryResults, settingsSyncOptions } = {},
+) => {
   return async (dispatch, getState) => {
     const [{ data }] = queryResults;
-    const prevQueryResults = getQueryResults(getState());
     const [{ data: prevData }] = prevQueryResults ?? [{}];
     const originalQuestion = getOriginalQuestionWithParameterValues(getState());
     const { isEditable } = Lib.queryDisplayInfo(question.query());
@@ -181,6 +191,7 @@ export const queryCompleted = (question, queryResults) => {
       question = question.syncColumnsAndSettings(
         queryResults[0],
         prevQueryResults?.[0],
+        settingsSyncOptions,
       );
 
       question = question.maybeResetDisplay(

frontend/src/metabase/querying/utils/drills/column-extract-drill/column-extract-drill.tsx

@@ -9,6 +9,7 @@ import type * as Lib from "metabase-lib";
 export const columnExtractDrill: Drill<Lib.ColumnExtractDrillThruInfo> = ({
   drill,
   drillInfo,
+  clicked,
   applyDrill,
 }) => {
   const DrillPopover = ({ onClick }: ClickActionPopoverProps) => {
@@ -19,6 +20,7 @@ export const columnExtractDrill: Drill<Lib.ColumnExtractDrillThruInfo> = ({
         section: "extract-popover",
         buttonType: "horizontal",
         question: () => applyDrill(drill, extraction.key),
+        extra: () => ({ settingsSyncOptions: { column: clicked.column } }),
       }),
     );
 

frontend/src/metabase/querying/utils/drills/query-drill.ts

@@ -6,17 +6,17 @@ import { DRILLS } from "./constants";
 
 export function queryDrill(
   question: Question,
-  clicked?: Lib.ClickObject,
+  clicked: Lib.ClickObject,
 ): ClickAction[] {
   const query = question.query();
   const stageIndex = -1;
   const drills = Lib.availableDrillThrus(
     query,
     stageIndex,
-    clicked?.column,
-    clicked?.value,
-    clicked?.data,
-    clicked?.dimensions,
+    clicked.column,
+    clicked.value,
+    clicked.data,
+    clicked.dimensions,
   );
 
   const applyDrill = (drill: Lib.DrillThru, ...args: unknown[]) => {
@@ -33,7 +33,7 @@ export function queryDrill(
       stageIndex,
       drill,
       drillInfo,
-      isDashboard: clicked?.extraData?.dashboard != null,
+      clicked,
       applyDrill,
     });
   });

frontend/src/metabase/querying/utils/drills/zoom-drill/zoom-drill.ts

@@ -7,10 +7,11 @@ import type * as Lib from "metabase-lib";
 export const zoomDrill: Drill<Lib.ZoomDrillThruInfo> = ({
   drill,
   drillInfo,
-  isDashboard,
+  clicked,
   applyDrill,
 }) => {
   const { objectId, isManyPks } = drillInfo;
+  const isDashboard = clicked.extraData?.dashboard != null;
 
   return [
     {

frontend/src/metabase/visualizations/click-actions/Mode/Mode.ts

@@ -25,7 +25,7 @@ export class Mode {
   }
 
   actionsForClick(
-    clicked: ClickObject | undefined,
+    clicked: ClickObject,
     settings: Record<string, any>,
     extraData?: Record<string, any>,
   ): ClickAction[] {

frontend/src/metabase/visualizations/components/Visualization/Visualization.jsx

@@ -302,13 +302,23 @@ class Visualization extends PureComponent {
   };
 
   // Add the underlying card of current series to onChangeCardAndRun if available
-  handleOnChangeCardAndRun = ({ nextCard, seriesIndex, objectId }) => {
+  handleOnChangeCardAndRun = ({
+    nextCard,
+    seriesIndex,
+    objectId,
+    settingsSyncOptions,
+  }) => {
     const { series, clicked } = this.state;
 
     const index = seriesIndex || (clicked && clicked.seriesIndex) || 0;
     const previousCard = series && series[index] && series[index].card;
 
-    this.props.onChangeCardAndRun({ nextCard, previousCard, objectId });
+    this.props.onChangeCardAndRun({
+      nextCard,
+      previousCard,
+      objectId,
+      settingsSyncOptions,
+    });
   };
 
   onRender = ({ yAxisSplit, warnings = [] } = {}) => {

frontend/src/metabase/visualizations/types/click-actions.ts

@@ -149,7 +149,7 @@ export type Drill<
   stageIndex: number;
   drill: Lib.DrillThru;
   drillInfo: T;
-  isDashboard: boolean;
+  clicked: Lib.ClickObject;
   applyDrill: (drill: Lib.DrillThru, ...args: any[]) => Question;
 }) => ClickAction[];
 

frontend/test/metabase-lib/lib/Question.unit.spec.js

@@ -610,7 +610,7 @@ describe("Question", () => {
     });
   });
 
-  describe("Question.prototype._syncTableColumnSettings", () => {
+  describe("Question.prototype.syncColumnsAndSettings", () => {
     let question;
     const cols = [
       {
@@ -656,39 +656,37 @@ describe("Question", () => {
 
     beforeEach(() => {
       question = native_orders_count_question.clone();
-      question.setting = jest.fn();
-      question.updateSettings = jest.fn();
+      question.settings = jest.fn(() => ({}));
+      question.setSettings = jest.fn();
     });
 
     describe("when columns have not been defined", () => {
       it("should do nothing when given no cols", () => {
-        question._syncTableColumnSettings({});
-        question._syncTableColumnSettings({ data: { cols: [] } });
-        question._syncTableColumnSettings({ data: { cols } });
+        question.syncColumnsAndSettings({});
+        question.syncColumnsAndSettings({ data: { cols: [] } });
+        question.syncColumnsAndSettings({ data: { cols } });
 
-        expect(question.updateSettings).not.toHaveBeenCalled();
+        expect(question.setSettings).not.toHaveBeenCalled();
       });
 
       it("should do nothing when given cols", () => {
-        question._syncTableColumnSettings({ data: { cols } });
+        question.syncColumnsAndSettings({ data: { cols } });
 
-        expect(question.updateSettings).not.toHaveBeenCalled();
+        expect(question.setSettings).not.toHaveBeenCalled();
       });
     });
 
     describe("after vizSetting columns have been defined", () => {
       beforeEach(() => {
-        question.setting.mockImplementation(property => {
-          if (property === "table.columns") {
-            return vizSettingCols;
-          }
-        });
+        question.settings.mockImplementation(() => ({
+          "table.columns": vizSettingCols,
+        }));
       });
 
       // Adding a column with same name is covered as well, as name is generated at FE and it will
       // be unique (e.g. foo -> foo_2)
       it("should handle the addition and removal of columns", () => {
-        question._syncTableColumnSettings({
+        question.syncColumnsAndSettings({
           data: {
             cols: [
               ...cols.slice(1),
@@ -709,7 +707,7 @@ describe("Question", () => {
           },
         });
 
-        expect(question.updateSettings).toHaveBeenCalledWith({
+        expect(question.setSettings).toHaveBeenCalledWith({
           "table.columns": [
             ...vizSettingCols.slice(1),
             {
@@ -742,13 +740,13 @@ describe("Question", () => {
           name: "foo",
           base_type: "type/Float",
         };
-        question._syncTableColumnSettings({
+        question.syncColumnsAndSettings({
           data: {
             cols: [updatedColumn, ...cols.slice(1)],
           },
         });
 
-        expect(question.updateSettings).toHaveBeenCalledWith({
+        expect(question.setSettings).toHaveBeenCalledWith({
           "table.columns": [
             ...vizSettingCols.slice(1),
             {
@@ -768,7 +766,7 @@ describe("Question", () => {
       });
 
       it("should handle the mutation of a field_ref on an existing column", () => {
-        question._syncTableColumnSettings({
+        question.syncColumnsAndSettings({
           data: {
             cols: [
               {
@@ -789,7 +787,7 @@ describe("Question", () => {
           },
         });
 
-        expect(question.updateSettings).toHaveBeenCalledWith({
+        expect(question.setSettings).toHaveBeenCalledWith({
           "table.columns": [
             ...vizSettingCols.slice(1),
             {
@@ -803,13 +801,13 @@ describe("Question", () => {
       });
 
       it("shouldn't update settings if order of columns has changed", () => {
-        question._syncTableColumnSettings({
+        question.syncColumnsAndSettings({
           data: {
             cols: [cols[1], cols[0]],
           },
         });
 
-        expect(question.updateSettings).not.toHaveBeenCalled();
+        expect(question.setSettings).not.toHaveBeenCalled();
       });
     });
   });