PivotGrid - Fix depth calculation of header items when a column is empty#32296
PivotGrid - Fix depth calculation of header items when a column is empty#32296AlisherAmonulloev wants to merge 7 commits into26_1from
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes PivotGrid header depth calculation in scenarios where hideEmptySummaryCells hides an entire column (e.g., when calculateSummaryValue returns null), preventing incorrect header layout/export results after expand/collapse operations.
Changes:
- Adjusted PivotGrid header depth computation to skip fully empty header items.
- Added PivotGrid QUnit regression tests covering expand/collapse behavior and export column stability when empty summary cells are hidden.
- Added ExcelJS exporter regression test verifying export output when
hideEmptySummaryCellsis enabled and summary values can benull.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/pivot_grid/data_controller/m_data_controller.ts | Updates header depth calculation to ignore empty header items when computing depth. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js | Adds UI-level regression tests around expand/collapse sequences and exported column stability with hidden empty cells. |
| packages/devextreme/testing/tests/DevExpress.exporter/exceljsParts/exceljs.pivotGrid.tests.js | Adds exporter regression test to ensure hidden empty columns aren’t exported after expand/collapse. |
packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js
Show resolved
Hide resolved
packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js
Outdated
Show resolved
Hide resolved
packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js
Show resolved
Hide resolved
packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js
Show resolved
Hide resolved
packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js
Show resolved
Hide resolved
packages/devextreme/js/__internal/grids/pivot_grid/data_controller/m_data_controller.ts
Outdated
Show resolved
Hide resolved
packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js
Show resolved
Hide resolved
packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js
Show resolved
Hide resolved
packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js
Show resolved
Hide resolved
packages/devextreme/js/__internal/grids/pivot_grid/data_controller/m_data_controller.ts
Show resolved
Hide resolved
packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js
Show resolved
Hide resolved
packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js
Show resolved
Hide resolved
packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js
Show resolved
Hide resolved
| clock.restore(); | ||
|
|
There was a problem hiding this comment.
Same as above: restoring the Sinon fake timers before exportPivotGrid(...) can leave pending PivotGrid timers to execute on real timers during the export, which is a common source of flaky tests. Align with the established pattern in this file by restoring the clock after the export promise resolves.
| clock.restore(); |
| assert.ok(true, `Initial Grand Total width: ${initialGrandTotalWidth}`); | ||
| assert.ok(true, `Final Grand Total width: ${finalGrandTotalWidth}`); |
There was a problem hiding this comment.
The two assert.ok(true, ...) calls are effectively no-ops (they always pass) and add noise to the test output. If you want diagnostics, use assert.comment(...) (or remove these lines) and keep assertions focused on verifiable conditions.
| assert.ok(true, `Initial Grand Total width: ${initialGrandTotalWidth}`); | |
| assert.ok(true, `Final Grand Total width: ${finalGrandTotalWidth}`); | |
| assert.comment(`Initial Grand Total width: ${initialGrandTotalWidth}`); | |
| assert.comment(`Final Grand Total width: ${finalGrandTotalWidth}`); |
| dataSource.collapseHeaderItem('column', [2013]); | ||
| clock.tick(10); | ||
|
|
||
| clock.restore(); | ||
|
|
There was a problem hiding this comment.
The test restores fake timers before starting exportPivotGrid(...). In this file, other tests typically keep fake timers active until the export promise resolves (and restore inside the .then) to avoid pending PivotGrid timeouts running on real timers and making the test flaky. Consider moving clock.restore() into the then handler and advancing the clock as needed (similar to the existing pivot.rtlEnabled = true test).
| assert.strictEqual( | ||
| finalGrandTotalWidth, | ||
| initialGrandTotalWidth, | ||
| 'Grand Total column width should remain the same after collapse sequence' | ||
| ); |
There was a problem hiding this comment.
This test asserts that the bug is fixed by comparing rendered DOM widths. DOM measurements can be environment-dependent (fonts, rounding, rendering engine) and make the test flaky. Consider asserting the underlying state instead (e.g., header items depth/colspan from the data controller, or the visible columns/header structure) rather than pixel width equality.
| this.clock.tick(10); | ||
|
|
||
| const initialColumnCount = initialItems[0].length; | ||
| assert.ok(true, `Initial export column count: ${initialColumnCount}`); |
There was a problem hiding this comment.
assert.ok(true, ...) always passes and only logs a message. Please remove this debug assertion (or replace with an assertion that can fail).
| assert.ok(true, `Initial export column count: ${initialColumnCount}`); |
| const initialDataProvider = pivotGrid.getDataProvider(); | ||
| const initialColumnsInfo = $.extend(true, [], pivotGrid._dataController.getColumnsInfo(true)); | ||
| const initialRowsInfo = $.extend(true, [], pivotGrid._dataController.getRowsInfo(true)); | ||
| const initialCellsInfo = pivotGrid._dataController.getCellsInfo(true); | ||
| const initialItems = pivotGrid._getAllItems(initialColumnsInfo, initialRowsInfo, initialCellsInfo); | ||
| initialDataProvider.ready(); |
There was a problem hiding this comment.
This export-related test relies on internal/private APIs (pivotGrid._dataController and pivotGrid._getAllItems). That makes the test brittle to refactors. If possible, validate via the public export surface (e.g., exporter output / data provider public methods) rather than widget internals.
No description provided.