From 9f22669f39623ab5a2173d50c0090dd9ffac1749 Mon Sep 17 00:00:00 2001
From: Nick Fitzpatrick <nick@metabase.com>
Date: Sat, 25 Mar 2023 19:19:17 -0300
Subject: [PATCH] keep standalone values when collapsing column (#29283)
* keep standalone values when collapsing column
* adjusting unit test
* PR Feedback
* adding space
* removing only
* Adjusting behavior and tests
* adjusting data_grid unit tests
---
.../visualizations/pivot_tables.cy.spec.js | 51 +++++++
...tandalone-values-when-collapsed.cy.spec.js | 59 --------
frontend/src/metabase/lib/data_grid.js | 2 +-
.../PivotTable/PivotTable.unit.spec.tsx | 70 ++++++++--
.../PivotTable/PivotTableCell.tsx | 1 +
.../PivotTable/RowToggleIcon.tsx | 3 +
.../test/metabase/lib/data_grid.unit.spec.js | 130 +++++++++++++-----
7 files changed, 212 insertions(+), 104 deletions(-)
delete mode 100644 e2e/test/scenarios/visualizations/reproductions/25250-pivot-no-standalone-values-when-collapsed.cy.spec.js
diff --git a/e2e/test/scenarios/visualizations/pivot_tables.cy.spec.js b/e2e/test/scenarios/visualizations/pivot_tables.cy.spec.js
index 8ad0cac213f..b1e1535f09d 100644
--- a/e2e/test/scenarios/visualizations/pivot_tables.cy.spec.js
+++ b/e2e/test/scenarios/visualizations/pivot_tables.cy.spec.js
@@ -216,6 +216,57 @@ describe("scenarios > visualizations > pivot tables", () => {
cy.findByText("294").should("not.exist"); // the other one is still hidden
});
+ it("should show standalone values when collapsed to the sub-level grouping (metabase#25250)", () => {
+ const questionDetails = {
+ name: "25250",
+ dataset_query: {
+ type: "query",
+ query: {
+ "source-table": ORDERS_ID,
+ filter: ["<", ["field", ORDERS.CREATED_AT, null], "2016-06-01"],
+ aggregation: [["count"]],
+ breakout: [
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
+ ["field", ORDERS.USER_ID, null],
+ ["field", ORDERS.PRODUCT_ID, null],
+ ],
+ },
+ database: SAMPLE_DB_ID,
+ },
+ display: "pivot",
+ visualization_settings: {
+ "pivot_table.column_split": {
+ rows: [
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
+ ["field", ORDERS.USER_ID, null],
+ ["field", ORDERS.PRODUCT_ID, null],
+ ],
+ columns: [],
+ values: [["aggregation", 0]],
+ },
+ "pivot_table.collapsed_rows": {
+ value: [],
+ rows: [
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
+ ["field", ORDERS.USER_ID, null],
+ ["field", ORDERS.PRODUCT_ID, null],
+ ],
+ },
+ },
+ };
+
+ visitQuestionAdhoc(questionDetails);
+ cy.findByText("1162").should("be.visible");
+ // Collapse "User ID" column
+ cy.findByText("User ID").parent().find(".Icon-dash").click();
+ cy.findByText("Totals for 1162").should("be.visible");
+
+ //Expanding the grouped column should still work
+ cy.findByText("Totals for 1162").parent().find(".Icon-add").click();
+ cy.findByText("1162").should("be.visible");
+ cy.findByText("34").should("be.visible");
+ });
+
it("should allow hiding subtotals", () => {
visitQuestionAdhoc({
dataset_query: testQuery,
diff --git a/e2e/test/scenarios/visualizations/reproductions/25250-pivot-no-standalone-values-when-collapsed.cy.spec.js b/e2e/test/scenarios/visualizations/reproductions/25250-pivot-no-standalone-values-when-collapsed.cy.spec.js
deleted file mode 100644
index 7b9a01192c5..00000000000
--- a/e2e/test/scenarios/visualizations/reproductions/25250-pivot-no-standalone-values-when-collapsed.cy.spec.js
+++ /dev/null
@@ -1,59 +0,0 @@
-import { restore, visitQuestionAdhoc } from "e2e/support/helpers";
-import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
-import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
-
-const { ORDERS_ID, ORDERS } = SAMPLE_DATABASE;
-
-const questionDetails = {
- name: "25250",
- dataset_query: {
- type: "query",
- query: {
- "source-table": ORDERS_ID,
- filter: ["<", ["field", ORDERS.CREATED_AT, null], "2016-06-01"],
- aggregation: [["count"]],
- breakout: [
- ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
- ["field", ORDERS.USER_ID, null],
- ["field", ORDERS.PRODUCT_ID, null],
- ],
- },
- database: SAMPLE_DB_ID,
- },
- display: "pivot",
- visualization_settings: {
- "pivot_table.column_split": {
- rows: [
- ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
- ["field", ORDERS.USER_ID, null],
- ["field", ORDERS.PRODUCT_ID, null],
- ],
- columns: [],
- values: [["aggregation", 0]],
- },
- "pivot_table.collapsed_rows": {
- value: [],
- rows: [
- ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
- ["field", ORDERS.USER_ID, null],
- ["field", ORDERS.PRODUCT_ID, null],
- ],
- },
- },
-};
-
-describe.skip("issue 25250", () => {
- beforeEach(() => {
- restore();
- cy.signInAsAdmin();
-
- visitQuestionAdhoc(questionDetails);
- });
-
- it("pivot table should show standalone values when collapsed to the sub-level grouping (metabase#25250)", () => {
- cy.findByText("1162").should("be.visible");
- // Collapse "User ID" column
- cy.findByText("User ID").parent().find(".Icon-dash").click();
- cy.findByText("1162").should("be.visible");
- });
-});
diff --git a/frontend/src/metabase/lib/data_grid.js b/frontend/src/metabase/lib/data_grid.js
index 31f1bb2c282..2808032df25 100644
--- a/frontend/src/metabase/lib/data_grid.js
+++ b/frontend/src/metabase/lib/data_grid.js
@@ -430,7 +430,7 @@ function addSubtotal(
// add subtotals until the last level
child.children.length > 0
? addSubtotal(child, showSubtotalsByColumn, {
- shouldShowSubtotal: child.children.length > 1,
+ shouldShowSubtotal: child.children.length > 1 || child.isCollapsed,
})
: child,
),
diff --git a/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.unit.spec.tsx b/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.unit.spec.tsx
index 962a22944ef..4efb6f75000 100644
--- a/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.unit.spec.tsx
+++ b/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.unit.spec.tsx
@@ -1,5 +1,6 @@
import React from "react";
-import { render, screen } from "@testing-library/react";
+import { render, screen, within } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
import _ from "underscore";
import type { VisualizationSettings } from "metabase-types/api";
import type { Column } from "metabase-types/types/Dataset";
@@ -45,6 +46,7 @@ const cols = [
const rows = [
["foo1", "bar1", "baz1", 0, 111, 222],
+ ["foo1", "bar1", "baz2", 0, 777, 888],
["foo2", "bar2", "baz2", 0, 333, 444],
["foo3", "bar3", "baz3", 0, 555, 666],
];
@@ -156,14 +158,14 @@ describe("Visualizations > PivotTable > PivotTable", () => {
rows.forEach(rowData => {
columnIndexes.forEach(colIndex => {
- expect(
- screen.getByText(rowData[colIndex].toString()),
- ).toBeInTheDocument();
+ expect(screen.getByTestId("pivot-table")).toHaveTextContent(
+ rowData[colIndex].toString(),
+ );
});
});
});
- it("should not render hidden column values", () => {
+ it("should collapse columns", () => {
const hiddenSettings = {
...settings,
"pivot_table.collapsed_rows": {
@@ -174,14 +176,58 @@ describe("Visualizations > PivotTable > PivotTable", () => {
setup({ initialSettings: hiddenSettings });
- rows.forEach(rowData => {
- expect(screen.getByText(rowData[0].toString())).toBeInTheDocument();
+ const COLLAPSED_COLUMN_INDEX = 1;
- [1, 2, 4, 5].forEach(colIndex => {
- expect(
- screen.queryByText(rowData[colIndex].toString()),
- ).not.toBeInTheDocument();
- });
+ rows.forEach(row => {
+ const totalsElement = screen.getByText(
+ `Totals for ${row[COLLAPSED_COLUMN_INDEX]}`,
+ );
+ expect(totalsElement).toBeInTheDocument();
+
+ const totalsContainer = screen.getByTestId(
+ `${row[COLLAPSED_COLUMN_INDEX]}-toggle-button`,
+ );
+
+ expect(
+ within(totalsContainer).getByRole("img", {
+ name: /add/i,
+ }),
+ ).toBeInTheDocument();
+ });
+ });
+
+ it("expanding collapsed columns", () => {
+ const hiddenSettings = {
+ ...settings,
+ "pivot_table.collapsed_rows": {
+ rows: [cols[0].field_ref, cols[1].field_ref, cols[2].field_ref],
+ value: ["2"],
+ },
+ } as unknown as VisualizationSettings;
+
+ setup({ initialSettings: hiddenSettings });
+
+ const COLLAPSED_COLUMN_INDEX = 1;
+
+ const LAST_ROW = rows[3];
+
+ // Find and click the toggle button to expand the last row
+ // as it's the easiest to make assertions on
+ const toggleButton = screen.getByTestId(
+ `${LAST_ROW[COLLAPSED_COLUMN_INDEX]}-toggle-button`,
+ );
+
+ expect(
+ within(toggleButton).getByRole("img", { name: /add/i }),
+ ).toBeInTheDocument();
+
+ userEvent.click(toggleButton);
+
+ //Ensure that collapsed data is now visible
+ columnIndexes.forEach(columnIndex => {
+ expect(
+ screen.getByText(LAST_ROW[columnIndex].toString()),
+ ).toBeInTheDocument();
});
});
});
diff --git a/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTableCell.tsx b/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTableCell.tsx
index b9b9961ab4b..c29dc2f5bb3 100644
--- a/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTableCell.tsx
+++ b/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTableCell.tsx
@@ -176,6 +176,7 @@ export const LeftHeaderCell = ({
icon={
(isSubtotal || hasSubtotal) && (
<RowToggleIcon
+ data-testid={`${item.rawValue}-toggle-button`}
value={path}
settings={settings}
updateSettings={onUpdateVisualizationSettings}
diff --git a/frontend/src/metabase/visualizations/visualizations/PivotTable/RowToggleIcon.tsx b/frontend/src/metabase/visualizations/visualizations/PivotTable/RowToggleIcon.tsx
index c0fcf9d5bd0..e469142cbbb 100644
--- a/frontend/src/metabase/visualizations/visualizations/PivotTable/RowToggleIcon.tsx
+++ b/frontend/src/metabase/visualizations/visualizations/PivotTable/RowToggleIcon.tsx
@@ -18,6 +18,7 @@ interface RowToggleIconProps {
updateSettings: (settings: VisualizationSettings) => void;
hideUnlessCollapsed?: boolean;
rowIndex?: string[];
+ "data-testid"?: string;
}
export function RowToggleIcon({
@@ -26,6 +27,7 @@ export function RowToggleIcon({
updateSettings,
hideUnlessCollapsed,
rowIndex = [],
+ "data-testid": testId,
}: RowToggleIconProps) {
if (value == null) {
return null;
@@ -84,6 +86,7 @@ export function RowToggleIcon({
return (
<RowToggleIconRoot
+ data-testid={testId}
onClick={e => {
e.stopPropagation();
updateSettings({
diff --git a/frontend/test/metabase/lib/data_grid.unit.spec.js b/frontend/test/metabase/lib/data_grid.unit.spec.js
index 8745dcbd4ce..bba06a4a790 100644
--- a/frontend/test/metabase/lib/data_grid.unit.spec.js
+++ b/frontend/test/metabase/lib/data_grid.unit.spec.js
@@ -595,40 +595,106 @@ describe("data_grid", () => {
});
describe("row collapsing", () => {
- const cols = [D1, D2, M];
- const primaryGroup = 0;
- const subtotalOne = 2;
- const rows = [
- ["a", "x", 1, primaryGroup],
- ["a", "y", 2, primaryGroup],
- ["b", "x", 3, primaryGroup],
- ["b", "y", 4, primaryGroup],
- ["a", null, 3, subtotalOne],
- ["b", null, 7, subtotalOne],
- ];
- const data = {
- rows,
- cols: [...cols, { name: "pivot-grouping", base_type: TYPE.Text }],
- };
- it("hides single collapsed rows", () => {
- const { getRowSection, leftHeaderItems, rowCount } =
- multiLevelPivotForIndexes(data, [], [0, 1], [2], {
- collapsedRows: ['["a"]'],
- });
- expect(rowCount).toEqual(5);
- expect(leftHeaderItems[0].value).toEqual("Totals for a"); // a is collapsed
- expect(getRowSection(0, 0)).toEqual([{ isSubtotal: true, value: "3" }]);
+ describe("single level pivot", () => {
+ const cols = [D1, D2, M];
+ const primaryGroup = 0;
+ const subtotalOne = 2;
+ const rows = [
+ ["a", "x", 1, primaryGroup],
+ ["a", "y", 2, primaryGroup],
+ ["b", "x", 3, primaryGroup],
+ ["b", "y", 4, primaryGroup],
+ ["a", null, 3, subtotalOne],
+ ["b", null, 7, subtotalOne],
+ ];
+ const data = {
+ rows,
+ cols: [...cols, { name: "pivot-grouping", base_type: TYPE.Text }],
+ };
+ it("hides single collapsed rows", () => {
+ const { getRowSection, leftHeaderItems, rowCount } =
+ multiLevelPivotForIndexes(data, [], [0, 1], [2], {
+ collapsedRows: ['["a"]'],
+ });
+ expect(rowCount).toEqual(5);
+ expect(leftHeaderItems[0].value).toEqual("Totals for a"); // a is collapsed
+ expect(getRowSection(0, 0)).toEqual([
+ { isSubtotal: true, value: "3" },
+ ]);
+ });
+ it("hides collapsed columns", () => {
+ const { getRowSection, leftHeaderItems, rowCount } =
+ multiLevelPivotForIndexes(data, [], [0, 1], [2], {
+ collapsedRows: ["1"],
+ });
+ expect(rowCount).toEqual(3);
+ expect(leftHeaderItems[0].value).toEqual("Totals for a"); // a is collapsed
+ expect(leftHeaderItems[1].value).toEqual("Totals for b"); // b is also collapsed
+ expect(getRowSection(0, 0)).toEqual([
+ { isSubtotal: true, value: "3" },
+ ]);
+ expect(getRowSection(0, 1)).toEqual([
+ { isSubtotal: true, value: "7" },
+ ]);
+ });
});
- it("hides collapsed columns", () => {
- const { getRowSection, leftHeaderItems, rowCount } =
- multiLevelPivotForIndexes(data, [], [0, 1], [2], {
- collapsedRows: ["1"],
+
+ describe("multi level pivot", () => {
+ const cols = [D1, D2, D3];
+ const rows = [
+ ["May", "Affiliate", "Gizmo", 0, 1],
+ ["May", "Facebook", "Gizmo", 0, 1],
+ ["May", "Google", "Doohickey", 0, 2],
+ ["May", "Google", "Gadget", 0, 1],
+ ["May", "Google", "Gizmo", 0, 2],
+ ["May", "Google", "Widget", 0, 1],
+ ["May", "Organic", "Doohickey", 0, 1],
+ ["May", "Organic", "Gadget", 0, 1],
+ ["May", "Organic", "Gizmo", 0, 1],
+ ["May", "Organic", "Widget", 0, 1],
+ ["May", "Twitter", "Doohickey", 0, 2],
+ ["May", "Twitter", "Gadget", 0, 2],
+ ["May", "Twitter", "Widget", 0, 2],
+ ["May", "Affiliate", null, 4, 1],
+ ["May", "Facebook", null, 4, 1],
+ ["May", "Google", null, 4, 6],
+ ["May", "Organic", null, 4, 4],
+ ["May", "Twitter", null, 4, 6],
+ ["May", null, null, 6, 18],
+ [null, null, null, 7, 18],
+ ];
+ const data = {
+ rows,
+ cols: [...cols, { name: "pivot-grouping", base_type: TYPE.Text }, M],
+ };
+
+ it("should convert single rows to subtotal rows when collapsed", () => {
+ // Collapsing the "source" column
+ const { getRowSection, leftHeaderItems, rowCount } =
+ multiLevelPivotForIndexes(data, [3], [0, 1, 2], [4], {
+ collapsedRows: ["2"],
+ });
+ expect(rowCount).toEqual(6);
+ expect(leftHeaderItems[1]).toMatchObject({
+ value: "Totals for Affiliate",
+ isSubtotal: true,
+ });
+ expect(leftHeaderItems[5]).toMatchObject({
+ value: "Totals for Twitter",
+ isSubtotal: true,
});
- expect(rowCount).toEqual(3);
- expect(leftHeaderItems[0].value).toEqual("Totals for a"); // a is collapsed
- expect(leftHeaderItems[1].value).toEqual("Totals for b"); // b is also collapsed
- expect(getRowSection(0, 0)).toEqual([{ isSubtotal: true, value: "3" }]);
- expect(getRowSection(0, 1)).toEqual([{ isSubtotal: true, value: "7" }]);
+ expect(getRowSection(1, 0)).toMatchObject([
+ {
+ value: "1",
+ },
+ ]); //Value for affiliate, but not a subtotal so it has children
+ expect(getRowSection(1, 2)).toEqual([
+ { isSubtotal: true, value: "6" },
+ ]);
+ expect(getRowSection(1, 3)).toEqual([
+ { isSubtotal: true, value: "4" },
+ ]);
+ });
});
});
--
GitLab