From 1fe63c52bfed1aacb70923bac9f60850709871c2 Mon Sep 17 00:00:00 2001
From: Uladzimir Havenchyk <125459446+uladzimirdev@users.noreply.github.com>
Date: Mon, 13 May 2024 17:35:14 +0300
Subject: [PATCH] Partially fix columns and viz settings mismatch (#42344)
From now we allow mismatch between `base-type` if we need to match columns and columnSettings
---
.../scenarios/question/settings.cy.spec.js | 17 +-
.../42049-wrong-columns-order.cy.spec.js | 86 ++++++++++
.../metabase-lib/v1/queries/utils/dataset.ts | 21 ++-
.../v1/queries/utils/dataset.unit.spec.ts | 160 ++++++++++++++++++
.../v1/queries/utils/get-column-key.ts | 3 +-
frontend/src/metabase-lib/v1/references.ts | 9 +-
.../src/metabase-lib/viz/column_settings.ts | 1 +
7 files changed, 281 insertions(+), 16 deletions(-)
create mode 100644 e2e/test/scenarios/visualizations-tabular/reproductions/42049-wrong-columns-order.cy.spec.js
create mode 100644 frontend/src/metabase-lib/v1/queries/utils/dataset.unit.spec.ts
diff --git a/e2e/test/scenarios/question/settings.cy.spec.js b/e2e/test/scenarios/question/settings.cy.spec.js
index 3874128342f..004d2b0665b 100644
--- a/e2e/test/scenarios/question/settings.cy.spec.js
+++ b/e2e/test/scenarios/question/settings.cy.spec.js
@@ -138,8 +138,21 @@ describe("scenarios > question > settings", () => {
"source-table": PRODUCTS_ID,
condition: [
"=",
- ["field-id", ORDERS.PRODUCT_ID],
- ["joined-field", "Products", ["field-id", PRODUCTS.ID]],
+ [
+ "field",
+ ORDERS.PRODUCT_ID,
+ {
+ "base-type": "type/Integer",
+ },
+ ],
+ [
+ "field",
+ PRODUCTS.ID,
+ {
+ "base-type": "type/BigInteger",
+ "join-alias": "Products",
+ },
+ ],
],
alias: "Products",
},
diff --git a/e2e/test/scenarios/visualizations-tabular/reproductions/42049-wrong-columns-order.cy.spec.js b/e2e/test/scenarios/visualizations-tabular/reproductions/42049-wrong-columns-order.cy.spec.js
new file mode 100644
index 00000000000..d79e2bfeded
--- /dev/null
+++ b/e2e/test/scenarios/visualizations-tabular/reproductions/42049-wrong-columns-order.cy.spec.js
@@ -0,0 +1,86 @@
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+import { restore, createQuestion } from "e2e/support/helpers";
+const { ORDERS, ORDERS_ID } = SAMPLE_DATABASE;
+
+// unskip once metabase#42049 is addressed
+describe.skip("issue 42049", () => {
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+ });
+
+ it("should not mess up columns order (metabase#42049)", () => {
+ cy.intercept("POST", "/api/card/*/query", req => {
+ req.on("response", res => {
+ const createdAt = res.body.data.cols[1];
+
+ createdAt.field_ref[1] = "created_at"; // simulate named field ref
+
+ res.send();
+ });
+ }).as("cardQuery");
+
+ 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 },
+ );
+
+ cy.log("verify initial columns order");
+
+ cy.findAllByTestId("header-cell").as("headerCells");
+ cy.get("@headerCells").eq(0).should("have.text", "ID");
+ cy.get("@headerCells").eq(1).should("have.text", "Created At");
+ cy.get("@headerCells").eq(2).should("have.text", "Quantity");
+
+ cy.findByRole("button", { name: "Filter" }).click();
+
+ cy.findByRole("dialog").within(() => {
+ cy.findByRole("button", { name: "Last month" }).click();
+ cy.findByRole("button", { name: "Apply filters" }).click();
+ });
+
+ cy.wait("@cardQuery");
+ cy.get("@cardQuery.all").should("have.length", 2);
+
+ cy.log("verify columns order after applying the filter");
+
+ cy.findAllByTestId("header-cell").as("headerCells");
+ cy.get("@headerCells").eq(0).should("have.text", "ID");
+ cy.get("@headerCells").eq(1).should("have.text", "Created At");
+ cy.get("@headerCells").eq(2).should("have.text", "Quantity");
+ });
+});
diff --git a/frontend/src/metabase-lib/v1/queries/utils/dataset.ts b/frontend/src/metabase-lib/v1/queries/utils/dataset.ts
index 09b2378f911..41356713df9 100644
--- a/frontend/src/metabase-lib/v1/queries/utils/dataset.ts
+++ b/frontend/src/metabase-lib/v1/queries/utils/dataset.ts
@@ -9,11 +9,14 @@ import type {
export const datasetContainsNoResults = (data: DatasetData) =>
data.rows == null || data.rows.length === 0;
-export function getColumnSettingKey({
- key,
- name,
- fieldRef,
-}: TableColumnOrderSetting) {
+export function getColumnSettingKey(
+ { key, name, fieldRef }: TableColumnOrderSetting,
+ ignoreBaseType = false,
+) {
+ if (ignoreBaseType) {
+ return getColumnKey({ name, field_ref: normalize(fieldRef) }, true);
+ }
+
return key ?? getColumnKey({ name, field_ref: normalize(fieldRef) });
}
@@ -22,11 +25,11 @@ export function findColumnIndexesForColumnSettings(
columnSettings: TableColumnOrderSetting[],
) {
const columnIndexByKey = new Map(
- columns.map((column, index) => [getColumnKey(column), index]),
+ columns.map((column, index) => [getColumnKey(column, true), index]),
);
return columnSettings.map(
columnSetting =>
- columnIndexByKey.get(getColumnSettingKey(columnSetting)) ?? -1,
+ columnIndexByKey.get(getColumnSettingKey(columnSetting, true)) ?? -1,
);
}
@@ -36,11 +39,11 @@ export function findColumnSettingIndexesForColumns(
) {
const columnSettingIndexByKey = new Map(
columnSettings.map((columnSetting, index) => [
- getColumnSettingKey(columnSetting),
+ getColumnSettingKey(columnSetting, true),
index,
]),
);
return columns.map(
- column => columnSettingIndexByKey.get(getColumnKey(column)) ?? -1,
+ column => columnSettingIndexByKey.get(getColumnKey(column, true)) ?? -1,
);
}
diff --git a/frontend/src/metabase-lib/v1/queries/utils/dataset.unit.spec.ts b/frontend/src/metabase-lib/v1/queries/utils/dataset.unit.spec.ts
new file mode 100644
index 00000000000..4a10f540ef0
--- /dev/null
+++ b/frontend/src/metabase-lib/v1/queries/utils/dataset.unit.spec.ts
@@ -0,0 +1,160 @@
+import {
+ createMockColumn,
+ createMockTableColumnOrderSetting,
+} from "metabase-types/api/mocks";
+import { ORDERS } from "metabase-types/api/mocks/presets";
+
+import {
+ findColumnIndexesForColumnSettings,
+ findColumnSettingIndexesForColumns,
+} from "./dataset";
+
+describe("dataset utils", () => {
+ describe("findColumnIndexesForColumnSettings", () => {
+ it("finds columnIndex for ColumnSettings without base-type", () => {
+ const column = createMockColumn({
+ id: ORDERS.TOTAL,
+ name: "TOTAL",
+ display_name: "Total",
+ field_ref: [
+ "field",
+ ORDERS.TOTAL,
+ {
+ "base-type": "type/Number",
+ },
+ ],
+ });
+ const columnSetting = createMockTableColumnOrderSetting({
+ name: "TOTAL",
+ key: `["ref",["field",${ORDERS.TOTAL},null]]`,
+ fieldRef: ["field", ORDERS.TOTAL, null],
+ enabled: true,
+ });
+
+ expect(
+ findColumnIndexesForColumnSettings([column], [columnSetting]),
+ ).toEqual([0]);
+ });
+
+ it("finds columnIndex for ColumnSettings with base-type", () => {
+ const column = createMockColumn({
+ id: ORDERS.TOTAL,
+ name: "TOTAL",
+ display_name: "Total",
+ field_ref: [
+ "field",
+ ORDERS.TOTAL,
+ {
+ "base-type": "type/Number",
+ },
+ ],
+ });
+ const columnSetting = createMockTableColumnOrderSetting({
+ name: "TOTAL",
+ key: `["ref",["field",${ORDERS.TOTAL},{"base-type":"type/Number"}]]`,
+ fieldRef: ["field", ORDERS.TOTAL, { "base-type": "type/Number" }],
+ enabled: true,
+ });
+
+ expect(
+ findColumnIndexesForColumnSettings([column], [columnSetting]),
+ ).toEqual([0]);
+ });
+
+ it("finds findColumnIndexesForColumnSettings for Columns with different base-type", () => {
+ const column = createMockColumn({
+ id: ORDERS.TOTAL,
+ name: "TOTAL",
+ display_name: "Total",
+ field_ref: [
+ "field",
+ ORDERS.TOTAL,
+ {
+ "base-type": "type/Text",
+ },
+ ],
+ });
+ const columnSetting = createMockTableColumnOrderSetting({
+ name: "TOTAL",
+ key: `["ref",["field",${ORDERS.TOTAL},{"base-type":"type/Number"}]]`,
+ fieldRef: ["field", ORDERS.TOTAL, { "base-type": "type/Number" }],
+ enabled: true,
+ });
+
+ expect(
+ findColumnIndexesForColumnSettings([column], [columnSetting]),
+ ).toEqual([0]);
+ });
+ });
+
+ describe("findColumnSettingIndexesForColumns", () => {
+ it("finds columnSettingsIndex for Column without base-type", () => {
+ const column = createMockColumn({
+ id: ORDERS.TOTAL,
+ name: "TOTAL",
+ display_name: "Total",
+ field_ref: ["field", ORDERS.TOTAL, null],
+ });
+ const columnSetting = createMockTableColumnOrderSetting({
+ name: "TOTAL",
+ key: `["ref",["field",${ORDERS.TOTAL},{"base-type":"type/Number"}]]`,
+ fieldRef: ["field", ORDERS.TOTAL, { "base-type": "type/Number" }],
+ enabled: true,
+ });
+
+ expect(
+ findColumnSettingIndexesForColumns([column], [columnSetting]),
+ ).toEqual([0]);
+ });
+
+ it("finds columnSettingsIndex for Columns with base-type", () => {
+ const column = createMockColumn({
+ id: ORDERS.TOTAL,
+ name: "TOTAL",
+ display_name: "Total",
+ field_ref: [
+ "field",
+ ORDERS.TOTAL,
+ {
+ "base-type": "type/Number",
+ },
+ ],
+ });
+ const columnSetting = createMockTableColumnOrderSetting({
+ name: "TOTAL",
+ key: `["ref",["field",${ORDERS.TOTAL},{"base-type":"type/Number"}]]`,
+ fieldRef: ["field", ORDERS.TOTAL, { "base-type": "type/Number" }],
+ enabled: true,
+ });
+
+ expect(
+ findColumnSettingIndexesForColumns([column], [columnSetting]),
+ ).toEqual([0]);
+ });
+
+ it("finds columnSettingsIndex for Columns with different base-type", () => {
+ const column = createMockColumn({
+ id: ORDERS.TOTAL,
+ name: "TOTAL",
+ display_name: "Total",
+ field_ref: [
+ "field",
+ ORDERS.TOTAL,
+ {
+ "base-type": "type/Text",
+ },
+ ],
+ });
+ const columnSetting = createMockTableColumnOrderSetting({
+ name: "TOTAL",
+ key: `["ref",["field",${ORDERS.TOTAL},{"base-type":"type/Number"}]]`,
+ fieldRef: ["field", ORDERS.TOTAL, { "base-type": "type/Number" }],
+ enabled: true,
+ });
+
+ expect(
+ findColumnSettingIndexesForColumns([column], [columnSetting]),
+ ).toEqual([0]);
+ });
+ });
+});
diff --git a/frontend/src/metabase-lib/v1/queries/utils/get-column-key.ts b/frontend/src/metabase-lib/v1/queries/utils/get-column-key.ts
index 34c34e53d75..d875143e3b3 100644
--- a/frontend/src/metabase-lib/v1/queries/utils/get-column-key.ts
+++ b/frontend/src/metabase-lib/v1/queries/utils/get-column-key.ts
@@ -12,6 +12,7 @@ import type { DatasetColumn } from "metabase-types/api";
export const getColumnKey = (
column: Pick<DatasetColumn, "name" | "field_ref">,
+ ignoreBaseType = false,
) => {
let fieldRef = column.field_ref;
@@ -31,7 +32,7 @@ export const getColumnKey = (
isExpressionReference(fieldRef) ||
isAggregationReference(fieldRef)
) {
- fieldRef = getBaseDimensionReference(fieldRef);
+ fieldRef = getBaseDimensionReference(fieldRef, ignoreBaseType);
}
const isLegacyRef =
diff --git a/frontend/src/metabase-lib/v1/references.ts b/frontend/src/metabase-lib/v1/references.ts
index 2c392b7f487..87cb56f176b 100644
--- a/frontend/src/metabase-lib/v1/references.ts
+++ b/frontend/src/metabase-lib/v1/references.ts
@@ -111,11 +111,12 @@ export const BASE_DIMENSION_REFERENCE_OMIT_OPTIONS = [
export const getBaseDimensionReference = (
mbql: DimensionReferenceWithOptions,
+ ignoreBaseType: boolean,
) =>
- getDimensionReferenceWithoutOptions(
- mbql,
- BASE_DIMENSION_REFERENCE_OMIT_OPTIONS,
- );
+ getDimensionReferenceWithoutOptions(mbql, [
+ ...BASE_DIMENSION_REFERENCE_OMIT_OPTIONS,
+ ...(ignoreBaseType ? ["base-type"] : []),
+ ]);
/**
* Whether this Field clause has a string Field name (as opposed to an integer Field ID). This generally means the
diff --git a/frontend/src/metabase-lib/viz/column_settings.ts b/frontend/src/metabase-lib/viz/column_settings.ts
index ed7ff0b8908..66ae5da594f 100644
--- a/frontend/src/metabase-lib/viz/column_settings.ts
+++ b/frontend/src/metabase-lib/viz/column_settings.ts
@@ -87,6 +87,7 @@ function syncTableColumnSettings(
fieldRef: col.field_ref,
enabled: true,
}));
+
return {
...settings,
"table.columns": [...existingColumnSettings, ...addedColumnSettings],
--
GitLab