Copilot commented on code in PR #36083:
URL: https://github.com/apache/superset/pull/36083#discussion_r2524504174
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -862,6 +862,47 @@ describe('plugin-chart-table', () => {
'rgba(172, 225, 196, 1)',
);
});
+
+ it('recalculates totals when user filters data', async () => {
+ const formDataWithTotals = {
+ ...testData.basic.formData,
+ show_totals: true,
+ include_search: true,
+ server_pagination: false,
+ metrics: ['sum__num'],
+ };
+
+ const data = testData.basic.queriesData[0].data;
+ const totalBeforeFilter = data.reduce(
+ (sum, row) => sum + Number(row.sum__num || 0),
+ 0,
+ );
+ const totalAfterFilter = data.find(item => item.name === 'Michael')
+ ?.sum__num || 0;
+
+ const props = transformProps({
+ ...testData.basic,
+ formData: formDataWithTotals,
+ });
+
+ props.totals = { sum__num: totalBeforeFilter };
Review Comment:
[nitpick] Trailing whitespace on this line. Consider removing it to maintain
code cleanliness.
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -862,6 +862,47 @@ describe('plugin-chart-table', () => {
'rgba(172, 225, 196, 1)',
);
});
+
+ it('recalculates totals when user filters data', async () => {
+ const formDataWithTotals = {
+ ...testData.basic.formData,
+ show_totals: true,
+ include_search: true,
+ server_pagination: false,
+ metrics: ['sum__num'],
+ };
+
+ const data = testData.basic.queriesData[0].data;
+ const totalBeforeFilter = data.reduce(
+ (sum, row) => sum + Number(row.sum__num || 0),
+ 0,
+ );
+ const totalAfterFilter = data.find(item => item.name === 'Michael')
+ ?.sum__num || 0;
+
+ const props = transformProps({
+ ...testData.basic,
+ formData: formDataWithTotals,
+ });
+
+ props.totals = { sum__num: totalBeforeFilter };
+
+ props.includeSearch = true;
+
+ render(<TableChart {...props} sticky={false} />);
+
+ const table = screen.getByRole('table');
+ const totalCellBefore =
within(table).getByText(String(totalBeforeFilter));
+ expect(totalCellBefore).toBeInTheDocument();
Review Comment:
[nitpick] Trailing whitespace on this line. Consider removing it to maintain
code cleanliness.
##########
superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts:
##########
@@ -235,5 +235,46 @@ describe('plugin-chart-table', () => {
expect(queries[0].post_processing).toEqual([]);
});
});
+
+ describe('Testing for server pagination with search filter', () => {
+ const baseFormDataWithServerPagination: TableChartFormData = {
+ ...basicFormData,
+ query_mode: QueryMode.Aggregate,
+ metrics: ['count'],
+ server_pagination: true,
+ search_filter: 'A',
+ groupby: ['category'],
+ };
+
+ const ownState = {
+ searchText: 'A',
+ searchColumn: 'category',
+ };
+
Review Comment:
[nitpick] Trailing whitespace on this line. Consider removing it to maintain
code cleanliness.
```suggestion
```
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -862,6 +862,47 @@ describe('plugin-chart-table', () => {
'rgba(172, 225, 196, 1)',
);
});
+
+ it('recalculates totals when user filters data', async () => {
+ const formDataWithTotals = {
+ ...testData.basic.formData,
+ show_totals: true,
+ include_search: true,
+ server_pagination: false,
+ metrics: ['sum__num'],
+ };
+
+ const data = testData.basic.queriesData[0].data;
+ const totalBeforeFilter = data.reduce(
+ (sum, row) => sum + Number(row.sum__num || 0),
+ 0,
+ );
+ const totalAfterFilter = data.find(item => item.name === 'Michael')
+ ?.sum__num || 0;
+
Review Comment:
[nitpick] Trailing whitespace on this line. Consider removing it to maintain
code cleanliness.
```suggestion
```
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -862,6 +862,47 @@ describe('plugin-chart-table', () => {
'rgba(172, 225, 196, 1)',
);
});
+
+ it('recalculates totals when user filters data', async () => {
+ const formDataWithTotals = {
+ ...testData.basic.formData,
+ show_totals: true,
+ include_search: true,
+ server_pagination: false,
+ metrics: ['sum__num'],
+ };
+
Review Comment:
[nitpick] Trailing whitespace on this line. Consider removing it to maintain
code cleanliness.
```suggestion
```
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -862,6 +862,47 @@ describe('plugin-chart-table', () => {
'rgba(172, 225, 196, 1)',
);
});
+
+ it('recalculates totals when user filters data', async () => {
+ const formDataWithTotals = {
+ ...testData.basic.formData,
+ show_totals: true,
+ include_search: true,
+ server_pagination: false,
+ metrics: ['sum__num'],
+ };
+
+ const data = testData.basic.queriesData[0].data;
+ const totalBeforeFilter = data.reduce(
+ (sum, row) => sum + Number(row.sum__num || 0),
+ 0,
+ );
+ const totalAfterFilter = data.find(item => item.name === 'Michael')
+ ?.sum__num || 0;
+
+ const props = transformProps({
+ ...testData.basic,
+ formData: formDataWithTotals,
+ });
+
+ props.totals = { sum__num: totalBeforeFilter };
+
+ props.includeSearch = true;
+
+ render(<TableChart {...props} sticky={false} />);
Review Comment:
[nitpick] Trailing whitespace on this line. Consider removing it to maintain
code cleanliness.
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -862,6 +862,47 @@ describe('plugin-chart-table', () => {
'rgba(172, 225, 196, 1)',
);
});
+
+ it('recalculates totals when user filters data', async () => {
+ const formDataWithTotals = {
+ ...testData.basic.formData,
+ show_totals: true,
+ include_search: true,
+ server_pagination: false,
+ metrics: ['sum__num'],
+ };
+
+ const data = testData.basic.queriesData[0].data;
+ const totalBeforeFilter = data.reduce(
+ (sum, row) => sum + Number(row.sum__num || 0),
+ 0,
+ );
+ const totalAfterFilter = data.find(item => item.name === 'Michael')
+ ?.sum__num || 0;
+
+ const props = transformProps({
+ ...testData.basic,
+ formData: formDataWithTotals,
+ });
+
+ props.totals = { sum__num: totalBeforeFilter };
+
+ props.includeSearch = true;
+
+ render(<TableChart {...props} sticky={false} />);
Review Comment:
[nitpick] Consider wrapping the test in `ProviderWrapper` for consistency
with other TableChart tests, especially those involving interactive features.
While the test may work without it, using the wrapper ensures theme and other
context providers are available if needed.
```suggestion
render(
<ProviderWrapper>
<TableChart {...props} sticky={false} />
</ProviderWrapper>
);
```
##########
superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:
##########
@@ -18,6 +18,7 @@
*/
import {
AdhocColumn,
+ // BinaryQueryObjectFilterClause,
Review Comment:
[nitpick] The comment on line 21 appears to be leftover debug code or an
incomplete change. It's commented out but doesn't serve any documentation
purpose. Consider removing it to keep the code clean.
```suggestion
```
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -862,6 +862,47 @@ describe('plugin-chart-table', () => {
'rgba(172, 225, 196, 1)',
);
});
+
+ it('recalculates totals when user filters data', async () => {
+ const formDataWithTotals = {
+ ...testData.basic.formData,
+ show_totals: true,
+ include_search: true,
+ server_pagination: false,
+ metrics: ['sum__num'],
+ };
+
+ const data = testData.basic.queriesData[0].data;
+ const totalBeforeFilter = data.reduce(
+ (sum, row) => sum + Number(row.sum__num || 0),
+ 0,
+ );
+ const totalAfterFilter = data.find(item => item.name === 'Michael')
+ ?.sum__num || 0;
+
+ const props = transformProps({
+ ...testData.basic,
+ formData: formDataWithTotals,
+ });
+
+ props.totals = { sum__num: totalBeforeFilter };
+
+ props.includeSearch = true;
Review Comment:
[nitpick] Trailing whitespace on this line. Consider removing it to maintain
code cleanliness.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1174,6 +1180,41 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
const [searchOptions, setSearchOptions] = useState<SearchOption[]>([]);
+ const handleFilteredDataChange = useCallback(
+ (rows: Row<D>[], searchText?: string) => {
+ if (!totals || serverPagination) {
+ return;
+ }
+
+ if (!searchText?.trim()) {
+ setDisplayedTotals(totals);
+ return;
+ }
+
+ const updatedTotals: Record<string, DataRecordValue> = { ...totals };
+
+ filteredColumnsMeta.forEach(column => {
+ if (column.isMetric || column.isPercentMetric) {
+ const aggregatedValue = rows.reduce<number>((acc, row) => {
+ const rawValue = row.original?.[column.key];
+ const numValue =
+ typeof rawValue === 'number'
+ ? rawValue
+ : typeof rawValue === 'string'
+ ? Number(rawValue.replace(/,/g, ''))
+ : NaN;
+ return Number.isFinite(numValue) ? acc + numValue : acc;
+ }, 0);
+
+ updatedTotals[column.key] = aggregatedValue;
+ }
+ });
Review Comment:
The `handleFilteredDataChange` function only recalculates totals for metric
and percent metric columns. However, non-metric columns that have totals in the
original `totals` object will retain their old values in `updatedTotals`. This
could lead to inconsistent totals where some columns reflect filtered data
while others don't. Consider either clearing non-metric totals or explicitly
handling all columns in the totals object.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
