bito-code-review[bot] commented on code in PR #37679: URL: https://github.com/apache/superset/pull/37679#discussion_r2785221051
########## superset-frontend/src/dashboard/components/ExportDashboardDataModal/useBulkExport.ts: ########## @@ -0,0 +1,250 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useState, useCallback } from 'react'; +import { SupersetClient } from '@superset-ui/core'; +import { buildV1ChartDataPayload } from 'src/explore/exploreUtils'; +import { ensureAppRoot } from 'src/utils/pathUtils'; + +export interface ChartExportData { + chartId: number; + chartName: string; + data: any[]; + columns: string[]; + status: 'pending' | 'exporting' | 'success' | 'error'; + error?: string; +} + +export interface BulkExportProgress { + current: number; + total: number; + charts: ChartExportData[]; +} + +export interface UseBulkExportProps { + onComplete?: (results: ChartExportData[]) => void; + onError?: (error: string) => void; +} + +export const useBulkExport = ({ + onComplete, + onError, +}: UseBulkExportProps = {}) => { + const [isExporting, setIsExporting] = useState(false); + const [progress, setProgress] = useState<BulkExportProgress>({ + current: 0, + total: 0, + charts: [], + }); + + const sanitizeSheetName = (name: string): string => { + // Excel sheet names: max 31 chars, no special characters + let sanitized = name + .replace(/[:\\/?*[\]]/g, '_') // Replace invalid chars + .substring(0, 31); // Truncate to 31 chars + + return sanitized || 'Sheet'; + }; + + const exportSingleChart = async ( + chartId: number, + chartName: string, + formData: any, + ): Promise<ChartExportData> => { + const chartData: ChartExportData = { + chartId, + chartName, + data: [], + columns: [], + status: 'exporting', + }; + + try { + // Build query payload + const payload = await buildV1ChartDataPayload({ + formData: { + ...formData, + slice_id: chartId, + }, + force: false, + resultFormat: 'json', + resultType: 'full', + setDataMask: () => {}, + ownState: {}, + }); + + // Call chart data API + const response = await SupersetClient.post({ + endpoint: ensureAppRoot('/api/v1/chart/data'), + jsonPayload: payload, + }); + + // Extract data from response + if (response.json?.result?.[0]) { + const [queryResult] = response.json.result; + chartData.data = queryResult.data || []; + chartData.columns = queryResult.colnames || []; + chartData.status = 'success'; + } else { + throw new Error('No data returned from API'); + } + } catch (error: any) { + chartData.status = 'error'; + chartData.error = error.message || 'Failed to export chart'; + } + + return chartData; + }; + + const exportCharts = useCallback( + async (charts: { id: number; name: string; formData: any }[]) => { + setIsExporting(true); + + const initialCharts: ChartExportData[] = charts.map(chart => ({ + chartId: chart.id, + chartName: chart.name, + data: [], + columns: [], + status: 'pending', + })); + + setProgress({ + current: 0, + total: charts.length, + charts: initialCharts, + }); + + const results: ChartExportData[] = []; + + // Export charts sequentially to avoid overwhelming the server + for (let i = 0; i < charts.length; i += 1) { + const chart = charts[i]; + + // Update progress: mark current chart as exporting + setProgress(prev => ({ + ...prev, + current: i + 1, + charts: prev.charts.map(c => + c.chartId === chart.id ? { ...c, status: 'exporting' } : c, + ), + })); + + // Export the chart + const result = await exportSingleChart( + chart.id, + chart.name, + chart.formData, + ); + results.push(result); + + // Update progress: mark chart as complete + setProgress(prev => ({ + ...prev, + charts: prev.charts.map(c => (c.chartId === chart.id ? result : c)), + })); + } + + setIsExporting(false); + + // Check if any succeeded + const successCount = results.filter(r => r.status === 'success').length; + + if (successCount === 0) { + onError?.('All chart exports failed'); + } else { + onComplete?.(results); + } + + return results; + }, + [onComplete, onError], + ); + + const generateExcelFile = useCallback( + async (results: ChartExportData[], dashboardTitle: string) => { + // Dynamically import xlsx to reduce initial bundle size + const XLSX = await import('xlsx'); + + const workbook = XLSX.utils.book_new(); + + // Track sheet names to handle duplicates + const usedSheetNames = new Set<string>(); + + results.forEach(result => { + if (result.status !== 'success' || !result.data.length) { + return; // Skip failed or empty charts + } + + // Generate unique sheet name + let sheetName = sanitizeSheetName(result.chartName); + let counter = 1; + + while (usedSheetNames.has(sheetName)) { + const baseName = sanitizeSheetName(result.chartName).substring(0, 28); + sheetName = `${baseName}(${counter})`; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Excel Compatibility Bug</b></div> <div id="fix"> Excel sheet names cannot exceed 31 characters. With counter like (10), 28 + 4 = 32 chars, violating the limit. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion while (usedSheetNames.has(sheetName)) { const baseName = sanitizeSheetName(result.chartName).substring(0, 26); sheetName = `${baseName}(${counter})`; ```` </div> </details> </div> <small><i>Code Review Run #06c69f</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/dashboard/components/ExportDashboardDataModal/ExportDashboardDataModal.test.tsx: ########## @@ -0,0 +1,654 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +/* eslint-disable import/first, import/order */ +// Mock SupersetClient.post - must be before imports to avoid hoisting issues +jest.mock('@superset-ui/core', () => { + const actual = jest.requireActual('@superset-ui/core'); + return { + ...actual, + SupersetClient: { + ...actual.SupersetClient, + post: jest.fn(), + }, + }; +}); + +// Mock buildV1ChartDataPayload and other exploreUtils functions +jest.mock('src/explore/exploreUtils', () => ({ + ...jest.requireActual('src/explore/exploreUtils'), + buildV1ChartDataPayload: jest.fn(async ({ formData }) => ({ + queries: [formData], + })), +})); + +// Mock xlsx library for Excel generation - use inline jest.fn() to avoid hoisting issues +jest.mock('xlsx', () => ({ + utils: { + book_new: jest.fn(() => ({ SheetNames: [], Sheets: {} })), + json_to_sheet: jest.fn(() => ({})), + book_append_sheet: jest.fn(), + }, + writeFile: jest.fn(), +})); + +// Mock toast notifications - use inline jest.fn() to avoid hoisting issues +jest.mock('src/components/MessageToasts/withToasts', () => ({ + __esModule: true, + default: (Component: React.ComponentType) => Component, + useToasts: jest.fn(() => ({ + addSuccessToast: jest.fn(), + addDangerToast: jest.fn(), + addWarningToast: jest.fn(), + })), +})); + +import { + render, + screen, + userEvent, + waitFor, +} from 'spec/helpers/testing-library'; +import ExportDashboardDataModal from '.'; +import { SupersetClient } from '@superset-ui/core'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import * as XLSX from 'xlsx'; + +// Get references to mocked functions +const mockPost = SupersetClient.post as jest.Mock; +const mockUseToasts = useToasts as jest.Mock; +const mockWriteFile = XLSX.writeFile as jest.Mock; + +// Mock toast functions that will be returned by useToasts +let mockAddSuccessToast: jest.Mock; +let mockAddDangerToast: jest.Mock; +let mockAddWarningToast: jest.Mock; + +const mockCharts = [ + { id: 1, name: 'Sales by Region', vizType: 'table' }, + { id: 2, name: 'Revenue Trend', vizType: 'line' }, + { id: 3, name: 'Top Products', vizType: 'bar' }, +]; + +const mockSlices = { + 1: { + slice_id: 1, + slice_name: 'Sales by Region', + form_data: { + datasource: '1__table', + viz_type: 'table', + }, + }, + 2: { + slice_id: 2, + slice_name: 'Revenue Trend', + form_data: { + datasource: '2__table', + viz_type: 'line', + }, + }, + 3: { + slice_id: 3, + slice_name: 'Top Products', + form_data: { + datasource: '3__table', + viz_type: 'bar', + }, + }, +}; + +const defaultProps = { + show: true, + onHide: jest.fn(), + dashboardTitle: 'Sales Dashboard', + charts: mockCharts, + slices: mockSlices, +}; + +beforeEach(() => { + jest.clearAllMocks(); + + // Set up fresh toast mocks for each test + mockAddSuccessToast = jest.fn(); + mockAddDangerToast = jest.fn(); + mockAddWarningToast = jest.fn(); + + mockUseToasts.mockReturnValue({ + addSuccessToast: mockAddSuccessToast, + addDangerToast: mockAddDangerToast, + addWarningToast: mockAddWarningToast, + }); +}); + +/** + * USER STORY: Opening the export modal + * As a user, when I click "Export Dashboard Data" from the Download menu, + * I should see a modal with all charts selected by default + */ +test('User opens export modal and sees all charts selected by default', () => { + render(<ExportDashboardDataModal {...defaultProps} />, { + useRedux: true, + }); + + // Modal should be visible + expect(screen.getByText('Export Dashboard Data')).toBeInTheDocument(); + + // All 3 charts should be listed + expect(screen.getByText('Sales by Region')).toBeInTheDocument(); + expect(screen.getByText('Revenue Trend')).toBeInTheDocument(); + expect(screen.getByText('Top Products')).toBeInTheDocument(); + + // Selection count should show all selected + expect(screen.getByText('3 of 3 charts selected')).toBeInTheDocument(); + + // Export button should show correct count + expect(screen.getByText('Export 3 charts')).toBeInTheDocument(); +}); + +/** + * USER STORY: Selecting specific charts + * As a user, I want to export only specific charts, not all of them + */ +test('User can select only specific charts to export', async () => { + render(<ExportDashboardDataModal {...defaultProps} />, { + useRedux: true, + }); + + // User clicks "Deselect all" to start fresh + const deselectAllButton = screen.getByText('Deselect all'); + await userEvent.click(deselectAllButton); + + expect(screen.getByText('0 of 3 charts selected')).toBeInTheDocument(); + expect(screen.getByText('Export 0 charts')).toBeInTheDocument(); + + // User selects only 2 charts + const salesChart = screen.getByText('Sales by Region'); + const revenueChart = screen.getByText('Revenue Trend'); + + await userEvent.click(salesChart); + await userEvent.click(revenueChart); + + // Should show correct selection count + expect(screen.getByText('2 of 3 charts selected')).toBeInTheDocument(); + expect(screen.getByText('Export 2 charts')).toBeInTheDocument(); +}); + +/** + * USER STORY: Successfully exporting charts + * As a user, when I click "Export X charts", I should see progress + * and eventually receive an Excel file with my data + */ +test('User exports selected charts and receives Excel file', async () => { + const mockSalesData = { + result: [ + { + data: [ + { region: 'North', sales: 1000 }, + { region: 'South', sales: 1500 }, + ], + colnames: ['region', 'sales'], + }, + ], + }; + + const mockRevenueData = { + result: [ + { + data: [ + { month: 'Jan', revenue: 5000 }, + { month: 'Feb', revenue: 6000 }, + ], + colnames: ['month', 'revenue'], + }, + ], + }; + + const mockProductsData = { + result: [ + { + data: [ + { product: 'Widget A', count: 100 }, + { product: 'Widget B', count: 150 }, + ], + colnames: ['product', 'count'], + }, + ], + }; + + mockPost + .mockResolvedValueOnce({ json: mockSalesData }) + .mockResolvedValueOnce({ json: mockRevenueData }) + .mockResolvedValueOnce({ json: mockProductsData }); + + render(<ExportDashboardDataModal {...defaultProps} />, { + useRedux: true, + }); + + const exportButton = screen.getByText('Export 3 charts'); + await userEvent.click(exportButton); + + // User should see progress indicator + await waitFor(() => { + expect(screen.getByText(/Exporting chart/)).toBeInTheDocument(); + }); + + // All 3 charts should be exported + await waitFor( + () => { + expect(mockPost).toHaveBeenCalledTimes(3); + }, + { timeout: 10000 }, + ); + + // Excel file should be generated + await waitFor( + () => { + expect(mockWriteFile).toHaveBeenCalled(); + }, + { timeout: 10000 }, + ); + + // User should see success message + await waitFor( + () => { + expect(mockAddSuccessToast).toHaveBeenCalled(); + }, + { timeout: 10000 }, + ); +}); + +/** + * EDGE CASE: Exporting with permission errors + * As a user, if I don't have permission to export some charts, + * I should still get an Excel file with the charts I can access + */ +test('User exports charts but some fail due to permissions - receives partial export', async () => { + const mockSalesData = { + result: [ + { + data: [{ region: 'North', sales: 1000 }], + colnames: ['region', 'sales'], + }, + ], + }; + + const mockProductsData = { + result: [ + { + data: [{ product: 'Widget A', count: 100 }], + colnames: ['product', 'count'], + }, + ], + }; + + mockPost + .mockResolvedValueOnce({ json: mockSalesData }) + .mockRejectedValueOnce(new Error('Permission denied')) + .mockResolvedValueOnce({ json: mockProductsData }); + + render(<ExportDashboardDataModal {...defaultProps} />, { + useRedux: true, + }); + + const exportButton = screen.getByText('Export 3 charts'); + await userEvent.click(exportButton); + + await waitFor( + () => { + expect(mockPost).toHaveBeenCalledTimes(3); + }, + { timeout: 10000 }, + ); + + // Should show which chart failed + await waitFor(() => { + expect(screen.getByText('Revenue Trend')).toBeInTheDocument(); + expect(screen.getByText(/Failed/)).toBeInTheDocument(); + }); + + // User should see success toast about partial success (not warning) + await waitFor( + () => { + expect(mockAddSuccessToast).toHaveBeenCalled(); + // Toast should mention partial success + const [[call]] = mockAddSuccessToast.mock.calls; + expect(call).toMatch(/2 of 3 charts exported successfully/); + }, + { timeout: 10000 }, + ); + + // Excel file should still be generated with successful charts + await waitFor( + () => { + expect(mockWriteFile).toHaveBeenCalled(); + }, + { timeout: 10000 }, + ); +}); + +/** + * EDGE CASE: Large dashboard warning + * As a user exporting a dashboard with >20 charts, + * I should see a warning about performance + */ +test('User sees warning when exporting more than 20 charts', () => { + const manyCharts = Array.from({ length: 25 }, (_, i) => ({ + id: i + 1, + name: `Chart ${i + 1}`, + vizType: 'table', + })); + + const manySlices = Object.fromEntries( + manyCharts.map(chart => [ + chart.id, + { + slice_id: chart.id, + slice_name: chart.name, + form_data: { datasource: '1__table', viz_type: 'table' }, + }, + ]), + ); + + render( + <ExportDashboardDataModal + {...defaultProps} + charts={manyCharts} + slices={manySlices} + />, + { + useRedux: true, + }, + ); + + expect( + screen.getByText(/Exporting many charts may take a while/), + ).toBeInTheDocument(); +}); + +/** + * EDGE CASE: Empty dashboard + * As a user opening export modal on a dashboard with no charts, + * the selection should show 0 of 0 charts and export button should be disabled + */ +test('User opens export modal on empty dashboard and cannot export', () => { + render( + <ExportDashboardDataModal {...defaultProps} charts={[]} slices={{}} />, + { + useRedux: true, + }, + ); + + expect(screen.getByText('0 of 0 charts selected')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /Export 0 charts/ })).toBeDisabled(); +}); + +/** + * EDGE CASE: Charts with special characters in names + * As a user exporting charts with special characters (/, :, *, etc.), + * the Excel sheet names should be sanitized properly + */ +test('User exports charts with special characters in names - sanitized for Excel', async () => { + const specialCharCharts = [ + { id: 1, name: 'Sales/Revenue: 2024*', vizType: 'table' }, + { id: 2, name: 'Users [Active]', vizType: 'bar' }, + ]; + + const specialCharSlices = { + 1: { + slice_id: 1, + slice_name: 'Sales/Revenue: 2024*', + form_data: { datasource: '1__table', viz_type: 'table' }, + }, + 2: { + slice_id: 2, + slice_name: 'Users [Active]', + form_data: { datasource: '2__table', viz_type: 'bar' }, + }, + }; + + mockPost.mockResolvedValue({ + json: { + result: [ + { + data: [{ value: 123 }], + colnames: ['value'], + }, + ], + }, + }); + + render( + <ExportDashboardDataModal + {...defaultProps} + charts={specialCharCharts} + slices={specialCharSlices} + />, + { + useRedux: true, + }, + ); + + const exportButton = screen.getByText('Export 2 charts'); + await userEvent.click(exportButton); + + await waitFor( + () => { + expect(mockPost).toHaveBeenCalledTimes(2); + }, + { timeout: 10000 }, + ); + + // Excel file should be generated with sanitized sheet names + await waitFor( + () => { + expect(mockWriteFile).toHaveBeenCalled(); + }, + { timeout: 10000 }, + ); +}); + +/** + * USER STORY: Canceling export + * As a user, I should be able to close the modal without exporting + */ +test('User closes modal without exporting', async () => { + const mockOnHide = jest.fn(); + + render(<ExportDashboardDataModal {...defaultProps} onHide={mockOnHide} />, { + useRedux: true, + }); + + const closeButton = screen.getByRole('button', { name: /Close/ }); + await userEvent.click(closeButton); + + expect(mockOnHide).toHaveBeenCalled(); + expect(mockPost).not.toHaveBeenCalled(); +}); + +/** + * EDGE CASE: No charts selected + * As a user, if I deselect all charts, the export button should be disabled + */ +test('User cannot export when no charts are selected', async () => { + render(<ExportDashboardDataModal {...defaultProps} />, { + useRedux: true, + }); + + const deselectAllButton = screen.getByText('Deselect all'); + await userEvent.click(deselectAllButton); + + const exportButton = screen.getByRole('button', { name: /Export 0 charts/ }); + expect(exportButton).toBeDisabled(); +}); + +/** + * EDGE CASE: Duplicate chart names + * As a user exporting charts with duplicate names, + * Excel sheets should be numbered to avoid conflicts + */ +test('User exports charts with duplicate names - sheets are numbered', async () => { + const duplicateNameCharts = [ + { id: 1, name: 'Sales Chart', vizType: 'table' }, + { id: 2, name: 'Sales Chart', vizType: 'bar' }, + { id: 3, name: 'Sales Chart', vizType: 'line' }, + ]; + + const duplicateNameSlices = Object.fromEntries( + duplicateNameCharts.map(chart => [ + chart.id, + { + slice_id: chart.id, + slice_name: chart.name, + form_data: { datasource: '1__table', viz_type: chart.vizType }, + }, + ]), + ); + + mockPost.mockResolvedValue({ + json: { + result: [ + { + data: [{ value: 123 }], + colnames: ['value'], + }, + ], + }, + }); + + render( + <ExportDashboardDataModal + {...defaultProps} + charts={duplicateNameCharts} + slices={duplicateNameSlices} + />, + { + useRedux: true, + }, + ); + + const exportButton = screen.getByText('Export 3 charts'); + await userEvent.click(exportButton); + + await waitFor( + () => { + expect(mockPost).toHaveBeenCalledTimes(3); + }, + { timeout: 10000 }, + ); + + // Excel file should be generated successfully with all charts + await waitFor( + () => { + expect(mockWriteFile).toHaveBeenCalled(); + }, + { timeout: 10000 }, + ); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Weak Test Assertion</b></div> <div id="fix"> The test for duplicate chart names only verifies that export completes, but doesn't check that Excel sheets are properly numbered to avoid conflicts. </div> </div> <small><i>Code Review Run #06c69f</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/dashboard/components/ExportDashboardDataModal/ExportDashboardDataModal.test.tsx: ########## @@ -0,0 +1,654 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +/* eslint-disable import/first, import/order */ +// Mock SupersetClient.post - must be before imports to avoid hoisting issues +jest.mock('@superset-ui/core', () => { + const actual = jest.requireActual('@superset-ui/core'); + return { + ...actual, + SupersetClient: { + ...actual.SupersetClient, + post: jest.fn(), + }, + }; +}); + +// Mock buildV1ChartDataPayload and other exploreUtils functions +jest.mock('src/explore/exploreUtils', () => ({ + ...jest.requireActual('src/explore/exploreUtils'), + buildV1ChartDataPayload: jest.fn(async ({ formData }) => ({ + queries: [formData], + })), +})); + +// Mock xlsx library for Excel generation - use inline jest.fn() to avoid hoisting issues +jest.mock('xlsx', () => ({ + utils: { + book_new: jest.fn(() => ({ SheetNames: [], Sheets: {} })), + json_to_sheet: jest.fn(() => ({})), + book_append_sheet: jest.fn(), + }, + writeFile: jest.fn(), +})); + +// Mock toast notifications - use inline jest.fn() to avoid hoisting issues +jest.mock('src/components/MessageToasts/withToasts', () => ({ + __esModule: true, + default: (Component: React.ComponentType) => Component, + useToasts: jest.fn(() => ({ + addSuccessToast: jest.fn(), + addDangerToast: jest.fn(), + addWarningToast: jest.fn(), + })), +})); + +import { + render, + screen, + userEvent, + waitFor, +} from 'spec/helpers/testing-library'; +import ExportDashboardDataModal from '.'; +import { SupersetClient } from '@superset-ui/core'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import * as XLSX from 'xlsx'; + +// Get references to mocked functions +const mockPost = SupersetClient.post as jest.Mock; +const mockUseToasts = useToasts as jest.Mock; +const mockWriteFile = XLSX.writeFile as jest.Mock; + +// Mock toast functions that will be returned by useToasts +let mockAddSuccessToast: jest.Mock; +let mockAddDangerToast: jest.Mock; +let mockAddWarningToast: jest.Mock; + +const mockCharts = [ + { id: 1, name: 'Sales by Region', vizType: 'table' }, + { id: 2, name: 'Revenue Trend', vizType: 'line' }, + { id: 3, name: 'Top Products', vizType: 'bar' }, +]; + +const mockSlices = { + 1: { + slice_id: 1, + slice_name: 'Sales by Region', + form_data: { + datasource: '1__table', + viz_type: 'table', + }, + }, + 2: { + slice_id: 2, + slice_name: 'Revenue Trend', + form_data: { + datasource: '2__table', + viz_type: 'line', + }, + }, + 3: { + slice_id: 3, + slice_name: 'Top Products', + form_data: { + datasource: '3__table', + viz_type: 'bar', + }, + }, +}; + +const defaultProps = { + show: true, + onHide: jest.fn(), + dashboardTitle: 'Sales Dashboard', + charts: mockCharts, + slices: mockSlices, +}; + +beforeEach(() => { + jest.clearAllMocks(); + + // Set up fresh toast mocks for each test + mockAddSuccessToast = jest.fn(); + mockAddDangerToast = jest.fn(); + mockAddWarningToast = jest.fn(); + + mockUseToasts.mockReturnValue({ + addSuccessToast: mockAddSuccessToast, + addDangerToast: mockAddDangerToast, + addWarningToast: mockAddWarningToast, + }); +}); + +/** + * USER STORY: Opening the export modal + * As a user, when I click "Export Dashboard Data" from the Download menu, + * I should see a modal with all charts selected by default + */ +test('User opens export modal and sees all charts selected by default', () => { + render(<ExportDashboardDataModal {...defaultProps} />, { + useRedux: true, + }); + + // Modal should be visible + expect(screen.getByText('Export Dashboard Data')).toBeInTheDocument(); + + // All 3 charts should be listed + expect(screen.getByText('Sales by Region')).toBeInTheDocument(); + expect(screen.getByText('Revenue Trend')).toBeInTheDocument(); + expect(screen.getByText('Top Products')).toBeInTheDocument(); + + // Selection count should show all selected + expect(screen.getByText('3 of 3 charts selected')).toBeInTheDocument(); + + // Export button should show correct count + expect(screen.getByText('Export 3 charts')).toBeInTheDocument(); +}); + +/** + * USER STORY: Selecting specific charts + * As a user, I want to export only specific charts, not all of them + */ +test('User can select only specific charts to export', async () => { + render(<ExportDashboardDataModal {...defaultProps} />, { + useRedux: true, + }); + + // User clicks "Deselect all" to start fresh + const deselectAllButton = screen.getByText('Deselect all'); + await userEvent.click(deselectAllButton); + + expect(screen.getByText('0 of 3 charts selected')).toBeInTheDocument(); + expect(screen.getByText('Export 0 charts')).toBeInTheDocument(); + + // User selects only 2 charts + const salesChart = screen.getByText('Sales by Region'); + const revenueChart = screen.getByText('Revenue Trend'); + + await userEvent.click(salesChart); + await userEvent.click(revenueChart); + + // Should show correct selection count + expect(screen.getByText('2 of 3 charts selected')).toBeInTheDocument(); + expect(screen.getByText('Export 2 charts')).toBeInTheDocument(); +}); + +/** + * USER STORY: Successfully exporting charts + * As a user, when I click "Export X charts", I should see progress + * and eventually receive an Excel file with my data + */ +test('User exports selected charts and receives Excel file', async () => { + const mockSalesData = { + result: [ + { + data: [ + { region: 'North', sales: 1000 }, + { region: 'South', sales: 1500 }, + ], + colnames: ['region', 'sales'], + }, + ], + }; + + const mockRevenueData = { + result: [ + { + data: [ + { month: 'Jan', revenue: 5000 }, + { month: 'Feb', revenue: 6000 }, + ], + colnames: ['month', 'revenue'], + }, + ], + }; + + const mockProductsData = { + result: [ + { + data: [ + { product: 'Widget A', count: 100 }, + { product: 'Widget B', count: 150 }, + ], + colnames: ['product', 'count'], + }, + ], + }; + + mockPost + .mockResolvedValueOnce({ json: mockSalesData }) + .mockResolvedValueOnce({ json: mockRevenueData }) + .mockResolvedValueOnce({ json: mockProductsData }); + + render(<ExportDashboardDataModal {...defaultProps} />, { + useRedux: true, + }); + + const exportButton = screen.getByText('Export 3 charts'); + await userEvent.click(exportButton); + + // User should see progress indicator + await waitFor(() => { + expect(screen.getByText(/Exporting chart/)).toBeInTheDocument(); + }); + + // All 3 charts should be exported + await waitFor( + () => { + expect(mockPost).toHaveBeenCalledTimes(3); + }, + { timeout: 10000 }, + ); + + // Excel file should be generated + await waitFor( + () => { + expect(mockWriteFile).toHaveBeenCalled(); + }, + { timeout: 10000 }, + ); + + // User should see success message + await waitFor( + () => { + expect(mockAddSuccessToast).toHaveBeenCalled(); + }, + { timeout: 10000 }, + ); +}); + +/** + * EDGE CASE: Exporting with permission errors + * As a user, if I don't have permission to export some charts, + * I should still get an Excel file with the charts I can access + */ +test('User exports charts but some fail due to permissions - receives partial export', async () => { + const mockSalesData = { + result: [ + { + data: [{ region: 'North', sales: 1000 }], + colnames: ['region', 'sales'], + }, + ], + }; + + const mockProductsData = { + result: [ + { + data: [{ product: 'Widget A', count: 100 }], + colnames: ['product', 'count'], + }, + ], + }; + + mockPost + .mockResolvedValueOnce({ json: mockSalesData }) + .mockRejectedValueOnce(new Error('Permission denied')) + .mockResolvedValueOnce({ json: mockProductsData }); + + render(<ExportDashboardDataModal {...defaultProps} />, { + useRedux: true, + }); + + const exportButton = screen.getByText('Export 3 charts'); + await userEvent.click(exportButton); + + await waitFor( + () => { + expect(mockPost).toHaveBeenCalledTimes(3); + }, + { timeout: 10000 }, + ); + + // Should show which chart failed + await waitFor(() => { + expect(screen.getByText('Revenue Trend')).toBeInTheDocument(); + expect(screen.getByText(/Failed/)).toBeInTheDocument(); + }); + + // User should see success toast about partial success (not warning) + await waitFor( + () => { + expect(mockAddSuccessToast).toHaveBeenCalled(); + // Toast should mention partial success + const [[call]] = mockAddSuccessToast.mock.calls; + expect(call).toMatch(/2 of 3 charts exported successfully/); + }, + { timeout: 10000 }, + ); + + // Excel file should still be generated with successful charts + await waitFor( + () => { + expect(mockWriteFile).toHaveBeenCalled(); + }, + { timeout: 10000 }, + ); +}); + +/** + * EDGE CASE: Large dashboard warning + * As a user exporting a dashboard with >20 charts, + * I should see a warning about performance + */ +test('User sees warning when exporting more than 20 charts', () => { + const manyCharts = Array.from({ length: 25 }, (_, i) => ({ + id: i + 1, + name: `Chart ${i + 1}`, + vizType: 'table', + })); + + const manySlices = Object.fromEntries( + manyCharts.map(chart => [ + chart.id, + { + slice_id: chart.id, + slice_name: chart.name, + form_data: { datasource: '1__table', viz_type: 'table' }, + }, + ]), + ); + + render( + <ExportDashboardDataModal + {...defaultProps} + charts={manyCharts} + slices={manySlices} + />, + { + useRedux: true, + }, + ); + + expect( + screen.getByText(/Exporting many charts may take a while/), + ).toBeInTheDocument(); +}); + +/** + * EDGE CASE: Empty dashboard + * As a user opening export modal on a dashboard with no charts, + * the selection should show 0 of 0 charts and export button should be disabled + */ +test('User opens export modal on empty dashboard and cannot export', () => { + render( + <ExportDashboardDataModal {...defaultProps} charts={[]} slices={{}} />, + { + useRedux: true, + }, + ); + + expect(screen.getByText('0 of 0 charts selected')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /Export 0 charts/ })).toBeDisabled(); +}); + +/** + * EDGE CASE: Charts with special characters in names + * As a user exporting charts with special characters (/, :, *, etc.), + * the Excel sheet names should be sanitized properly + */ +test('User exports charts with special characters in names - sanitized for Excel', async () => { + const specialCharCharts = [ + { id: 1, name: 'Sales/Revenue: 2024*', vizType: 'table' }, + { id: 2, name: 'Users [Active]', vizType: 'bar' }, + ]; + + const specialCharSlices = { + 1: { + slice_id: 1, + slice_name: 'Sales/Revenue: 2024*', + form_data: { datasource: '1__table', viz_type: 'table' }, + }, + 2: { + slice_id: 2, + slice_name: 'Users [Active]', + form_data: { datasource: '2__table', viz_type: 'bar' }, + }, + }; + + mockPost.mockResolvedValue({ + json: { + result: [ + { + data: [{ value: 123 }], + colnames: ['value'], + }, + ], + }, + }); + + render( + <ExportDashboardDataModal + {...defaultProps} + charts={specialCharCharts} + slices={specialCharSlices} + />, + { + useRedux: true, + }, + ); + + const exportButton = screen.getByText('Export 2 charts'); + await userEvent.click(exportButton); + + await waitFor( + () => { + expect(mockPost).toHaveBeenCalledTimes(2); + }, + { timeout: 10000 }, + ); + + // Excel file should be generated with sanitized sheet names + await waitFor( + () => { + expect(mockWriteFile).toHaveBeenCalled(); + }, + { timeout: 10000 }, + ); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Weak Test Assertion</b></div> <div id="fix"> The test for special characters in chart names only verifies that export completes, but doesn't check that Excel sheet names are properly sanitized. This could allow sanitization bugs to go undetected. </div> </div> <small><i>Code Review Run #06c69f</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
