From 7249b7df31dbe9651d36b066e83e6c0f8b7ce64e Mon Sep 17 00:00:00 2001
From: Romeo Van Snick <romeo@romeovansnick.be>
Date: Tue, 7 May 2024 18:00:32 +0200
Subject: [PATCH] Allow duplicate "combine column" columns in chill mode
(#42314)
* Add suffix to name if column already exists
* Add test for duplicate combine columns
* Use direct question in test
Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
* Fix formatting
* Use createQuestion for other tests too
* Fix table references
---------
Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
---
.../drillthroughs/combine-column.cy.spec.js | 57 +++++++++++++++----
.../CombineColumnsDrill.tsx | 1 +
.../drills/combine-columns-drill/utils.ts | 30 +++++++++-
3 files changed, 76 insertions(+), 12 deletions(-)
diff --git a/e2e/test/scenarios/visualizations-tabular/drillthroughs/combine-column.cy.spec.js b/e2e/test/scenarios/visualizations-tabular/drillthroughs/combine-column.cy.spec.js
index 162c85b490f..fced9452592 100644
--- a/e2e/test/scenarios/visualizations-tabular/drillthroughs/combine-column.cy.spec.js
+++ b/e2e/test/scenarios/visualizations-tabular/drillthroughs/combine-column.cy.spec.js
@@ -1,14 +1,17 @@
import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
import {
+ createQuestion,
describeWithSnowplow,
expectGoodSnowplowEvent,
expectNoBadSnowplowEvents,
- openPeopleTable,
popover,
resetSnowplow,
restore,
} from "e2e/support/helpers";
+const { PEOPLE, PEOPLE_ID } = SAMPLE_DATABASE;
+
describeWithSnowplow(
"scenarios > visualizations > drillthroughs > table_drills > combine columns",
() => {
@@ -23,16 +26,19 @@ describeWithSnowplow(
});
it("should be possible to combine columns from the a table header", () => {
- openPeopleTable({ limit: 3, mode: "notebook" });
-
- cy.findByLabelText("Pick columns").click();
- popover().within(() => {
- cy.findByText("Select none").click();
- cy.findByLabelText("Email").click();
- });
-
- cy.findByLabelText("Pick columns").click();
- cy.button("Visualize").click();
+ createQuestion(
+ {
+ query: {
+ "source-table": PEOPLE_ID,
+ fields: [
+ ["field", PEOPLE.ID, { "base-type": "type/Number" }],
+ ["field", PEOPLE.EMAIL, { "base-type": "type/Text" }],
+ ],
+ limit: 3,
+ },
+ },
+ { visitQuestion: true },
+ );
cy.findAllByTestId("header-cell").contains("Email").click();
popover().findByText("Combine columns").click();
@@ -86,5 +92,34 @@ describeWithSnowplow(
database_id: SAMPLE_DB_ID,
});
});
+
+ it("should handle duplicate column names", () => {
+ createQuestion(
+ {
+ query: {
+ "source-table": PEOPLE_ID,
+ fields: [
+ ["field", PEOPLE.ID, { "base-type": "type/Number" }],
+ ["field", PEOPLE.EMAIL, { "base-type": "type/Text" }],
+ ],
+ limit: 3,
+ },
+ },
+ { visitQuestion: true },
+ );
+
+ // first combine (email + ID)
+ cy.findAllByTestId("header-cell").contains("Email").click();
+ popover().findByText("Combine columns").click();
+ popover().findByText("Done").click();
+
+ // second combine (email + ID)
+ cy.findAllByTestId("header-cell").contains("Email").click();
+ popover().findByText("Combine columns").click();
+ popover().findByText("Done").click();
+
+ cy.findAllByTestId("header-cell").contains("Email ID").should("exist");
+ cy.findAllByTestId("header-cell").contains("Email ID_2").should("exist");
+ });
},
);
diff --git a/frontend/src/metabase/querying/utils/drills/combine-columns-drill/components/CombineColumnsDrill/CombineColumnsDrill.tsx b/frontend/src/metabase/querying/utils/drills/combine-columns-drill/components/CombineColumnsDrill/CombineColumnsDrill.tsx
index 9e0bf517f9f..7778a405f49 100644
--- a/frontend/src/metabase/querying/utils/drills/combine-columns-drill/components/CombineColumnsDrill/CombineColumnsDrill.tsx
+++ b/frontend/src/metabase/querying/utils/drills/combine-columns-drill/components/CombineColumnsDrill/CombineColumnsDrill.tsx
@@ -105,6 +105,7 @@ export const CombineColumnsDrill = ({
column,
columnsAndSeparators,
);
+
const newQuery = Lib.expression(query, stageIndex, name, expressionClause);
onSubmit(newQuery);
diff --git a/frontend/src/metabase/querying/utils/drills/combine-columns-drill/utils.ts b/frontend/src/metabase/querying/utils/drills/combine-columns-drill/utils.ts
index ca62695b49a..c5e41a9c9c4 100644
--- a/frontend/src/metabase/querying/utils/drills/combine-columns-drill/utils.ts
+++ b/frontend/src/metabase/querying/utils/drills/combine-columns-drill/utils.ts
@@ -139,9 +139,37 @@ export const getExpressionName = (
column: Lib.ColumnMetadata,
columnsAndSeparators: ColumnAndSeparator[],
): string => {
+ const columnNames = Lib.returnedColumns(query, stageIndex).map(
+ column => Lib.displayInfo(query, stageIndex, column).displayName,
+ );
+
+ const name = getCombinedColumnName(
+ query,
+ stageIndex,
+ column,
+ columnsAndSeparators,
+ );
+
+ return getNextName(columnNames, name, 1);
+};
+
+function getNextName(names: string[], name: string, index: number): string {
+ const suffixed = index === 1 ? name : `${name}_${index}`;
+ if (!names.includes(suffixed)) {
+ return suffixed;
+ }
+ return getNextName(names, name, index + 1);
+}
+
+function getCombinedColumnName(
+ query: Lib.Query,
+ stageIndex: number,
+ column: Lib.ColumnMetadata,
+ columnsAndSeparators: ColumnAndSeparator[],
+) {
const columns = [column, ...columnsAndSeparators.map(({ column }) => column)];
const names = columns.map(
column => Lib.displayInfo(query, stageIndex, column).displayName,
);
return names.join(" ");
-};
+}
--
GitLab