bito-code-review[bot] commented on code in PR #38591:
URL: https://github.com/apache/superset/pull/38591#discussion_r2922571893
##########
superset-frontend/src/features/alerts/AlertReportModal.test.tsx:
##########
@@ -1488,3 +2251,310 @@ test('tabs metadata overwrites seeded filter options',
async () => {
within(selectContainer).queryByTitle('Country'),
).not.toBeInTheDocument();
});
+
+test('selecting filter triggers chart data request with correct params', async
() => {
+ mockGetChartDataRequest.mockReset();
+ fetchMock.removeRoute(tabsEndpoint);
+ fetchMock.get(tabsEndpoint, tabsWithFilters, { name: tabsEndpoint });
+
+ mockGetChartDataRequest.mockResolvedValue({
+ json: { result: [{ data: [{ country: 'US' }, { country: 'UK' }] }] },
+ });
+
+ render(<AlertReportModal {...generateMockedProps(true, true)} />, {
+ useRedux: true,
+ });
+
+ userEvent.click(screen.getByTestId('contents-panel'));
+ await screen.findByText(/test dashboard/i);
+
+ // Wait for filter dropdown to be available
+ const filterDropdown = await waitFor(() =>
+ screen.getByRole('combobox', { name: /select filter/i }),
+ );
+
+ // Select the Country Filter using comboboxSelect pattern
+ await comboboxSelect(filterDropdown, 'Country Filter', () =>
+ document.querySelector(
+ '.ant-select-selection-item[title="Country Filter"]',
+ ),
+ );
+
+ // getChartDataRequest should have been called for filter values
+ await waitFor(() => {
+ expect(mockGetChartDataRequest).toHaveBeenCalledTimes(1);
+ });
+
+ // Verify it was called with correct datasource and groupby
+ const callArgs = mockGetChartDataRequest.mock.calls[0][0];
+ expect(callArgs.formData.groupby).toEqual(['country']);
+ expect(callArgs.formData.datasource).toBe('1__table');
+
+ mockGetChartDataRequest.mockReset();
+});
+
+test('selected filter excluded from other row dropdowns', async () => {
+ mockGetChartDataRequest.mockReset();
+ fetchMock.removeRoute(tabsEndpoint);
+ fetchMock.get(tabsEndpoint, tabsWithFilters, { name: tabsEndpoint });
+
+ mockGetChartDataRequest.mockResolvedValue({
+ json: { result: [{ data: [{ country: 'US' }] }] },
+ });
+
+ render(<AlertReportModal {...generateMockedProps(true, true)} />, {
+ useRedux: true,
+ });
+
+ userEvent.click(screen.getByTestId('contents-panel'));
+ await screen.findByText(/test dashboard/i);
+
+ // Wait for filter dropdown
+ const filterDropdown = await waitFor(() =>
+ screen.getByRole('combobox', { name: /select filter/i }),
+ );
+
+ // Select Country Filter in row 1
+ await comboboxSelect(filterDropdown, 'Country Filter', () =>
+ document.querySelector(
+ '.ant-select-selection-item[title="Country Filter"]',
+ ),
+ );
+
+ // Wait for getChartDataRequest to complete AND state update to propagate.
+ // The value dropdown becomes non-disabled once optionFilterValues is
populated.
+ await waitFor(() => {
+ expect(mockGetChartDataRequest).toHaveBeenCalled();
+ });
+ await waitFor(
+ () => {
+ const valueSelects = screen.queryAllByRole('combobox', {
+ name: /select value/i,
+ });
+ expect(valueSelects.length).toBeGreaterThan(0);
+ },
+ { timeout: 5000 },
+ );
+
+ // Add second filter row
+ const addFilterButton = screen.getByText(/apply another dashboard filter/i);
+ userEvent.click(addFilterButton);
+
+ // Wait for second row
+ await waitFor(() => {
+ expect(
+ screen.getAllByRole('combobox', { name: /select filter/i }),
+ ).toHaveLength(2);
+ });
+
+ // Open filter dropdown in row 2
+ const filterDropdowns = screen.getAllByRole('combobox', {
+ name: /select filter/i,
+ });
+ userEvent.click(filterDropdowns[1]);
+
+ // Country Filter should be excluded (dedup), City Filter should remain.
+ // Use the last virtual list since the first may belong to the closed row 1
dropdown.
+ await waitFor(() => {
+ const virtualLists = document.querySelectorAll('.rc-virtual-list');
+ const lastVirtualList = virtualLists[virtualLists.length - 1];
+ expect(
+ within(lastVirtualList as HTMLElement).queryByText('Country Filter'),
+ ).not.toBeInTheDocument();
+ expect(
+ within(lastVirtualList as HTMLElement).getByText('City Filter'),
+ ).toBeInTheDocument();
+ });
+
+ mockGetChartDataRequest.mockReset();
+}, 30000);
+
+test('invalid CC email blocks submit', async () => {
+ render(<AlertReportModal {...generateMockedProps(false, true, false)} />, {
+ useRedux: true,
+ });
+
+ // Wait for validation to fully propagate (fetch → state → validate →
enforce)
+ await screen.findByText('Edit alert');
+ await waitFor(
+ () => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeEnabled();
+ },
+ { timeout: 5000 },
+ );
+
+ // Open notification panel and show CC field
+ userEvent.click(screen.getByTestId('notification-method-panel'));
+ const addCcButton = await screen.findByText(/Add CC Recipients/i);
+ userEvent.click(addCcButton);
+
+ // Type invalid email in CC field
+ const ccInput = await screen.findByTestId('cc');
+ userEvent.type(ccInput, 'not-an-email');
+ fireEvent.blur(ccInput);
+
+ // Save should now be disabled due to invalid email format
+ await waitFor(() => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeDisabled();
+ });
+});
+
+test('invalid BCC email blocks submit', async () => {
+ render(<AlertReportModal {...generateMockedProps(false, true, false)} />, {
+ useRedux: true,
+ });
+
+ // Wait for validation to fully propagate (fetch → state → validate →
enforce)
+ await screen.findByText('Edit alert');
+ await waitFor(
+ () => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeEnabled();
+ },
+ { timeout: 5000 },
+ );
+
+ // Open notification panel and show BCC field
+ userEvent.click(screen.getByTestId('notification-method-panel'));
+ const addBccButton = await screen.findByText(/Add BCC Recipients/i);
+ userEvent.click(addBccButton);
+
+ // Type invalid email in BCC field
+ const bccInput = await screen.findByTestId('bcc');
+ userEvent.type(bccInput, 'not-an-email');
+ fireEvent.blur(bccInput);
+
+ // Save should now be disabled due to invalid email format
+ await waitFor(() => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeDisabled();
+ });
+});
+
+test('invalid saved anchor is reset on dashboard load', async () => {
+ fetchMock.removeRoute(tabsEndpoint);
+ fetchMock.get(tabsEndpoint, tabsWithFilters, { name: tabsEndpoint });
+
+ const props = generateMockedProps(true, true);
+ const editProps = {
+ ...props,
+ alert: { ...validAlert, id: 7 },
+ };
+
+ render(<AlertReportModal {...editProps} />, {
+ useRedux: true,
+ });
+
+ userEvent.click(screen.getByTestId('contents-panel'));
+ await screen.findByText(/test dashboard/i);
+
+ // Wait for dashboard tabs to load
+ await waitFor(() => {
+
expect(fetchMock.callHistory.calls(tabsEndpoint).length).toBeGreaterThan(0);
+ });
+
+ // The saved anchor 'TAB_999' is NOT in the dashboard's tabs (TAB_1, TAB_2),
+ // so it should be reset to undefined. Tab selector shows placeholder.
+ await waitFor(() => {
+ expect(screen.getByText(/select a tab/i)).toBeInTheDocument();
+ });
+
+ // TAB_999 should NOT appear as a selected value anywhere
+ expect(
+ document.querySelector('.ant-select-selection-item[title="TAB_999"]'),
+ ).not.toBeInTheDocument();
+});
+
+test('clearing notification recipients disables submit and prevents API call',
async () => {
+ fetchMock.put(
+ 'glob:*/api/v1/report/2',
+ { id: 2, result: {} },
+ { name: 'put-no-recipients' },
+ );
+
+ render(<AlertReportModal {...generateMockedProps(false, true, false)} />, {
+ useRedux: true,
+ });
+
+ // Wait for all validation to pass (5 checkmarks = fully valid alert)
+ await waitFor(() => {
+ expect(
+ screen.queryAllByRole('img', { name: /check-circle/i }),
+ ).toHaveLength(5);
+ });
+
+ // Save should be enabled initially
+ expect(screen.getByRole('button', { name: /save/i })).toBeEnabled();
+
+ // Open notification panel and clear the recipients field
+ userEvent.click(screen.getByTestId('notification-method-panel'));
+ const recipientInput = await screen.findByTestId('recipients');
+ userEvent.clear(recipientInput);
+ fireEvent.blur(recipientInput);
+
+ // Save should be disabled — empty recipients block submission
+ await waitFor(() => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeDisabled();
+ });
+
+ // No PUT was emitted — the disabled button prevented any payload from being
sent.
+ // This confirms that a payload with empty recipients is never transmitted.
+ expect(fetchMock.callHistory.calls('put-no-recipients')).toHaveLength(0);
+
+ fetchMock.removeRoute('put-no-recipients');
+});
+
+test('empty recipients array is omitted from payload', () => {
+ // Direct test of the payload cleanup logic from AlertReportModal (lines
941-943).
+ // UI validation prevents submitting with empty recipients, so this exercises
+ // the defensive `delete data.recipients` branch in isolation.
+ const data: Record<string, unknown> = {
+ name: 'Test Alert',
+ recipients: [],
+ type: 'Alert',
+ };
+
+ // Mirrors AlertReportModal.tsx lines 941-943
+ if (data.recipients && !(data.recipients as unknown[]).length) {
+ delete data.recipients;
+ }
+
+ expect(data).not.toHaveProperty('recipients');
+ expect(data).toHaveProperty('name', 'Test Alert');
+});
+
+test('non-empty recipients array is preserved in payload', () => {
+ // Complementary to the empty-recipients test: verifies that populated
+ // recipients survive the cleanup logic unchanged.
+ const data: Record<string, unknown> = {
+ name: 'Test Alert',
+ recipients: [{ type: 'Email', recipient_config_json: { target:
'[email protected]' } }],
+ type: 'Alert',
+ };
+
+ if (data.recipients && !(data.recipients as unknown[]).length) {
+ delete data.recipients;
+ }
+
+ expect(data).toHaveProperty('recipients');
+ expect((data.recipients as unknown[]).length).toBe(1);
+});
+
+test('modal reopen resets local state', async () => {
+ const props = generateMockedProps(true, false, true);
+
+ render(<AlertReportModal {...props} />, { useRedux: true });
+
+ // Type a name to dirty the form
+ const nameInput = screen.getByPlaceholderText(/enter report name/i);
+ userEvent.type(nameInput, 'Temporary Report');
+ expect(nameInput).toHaveValue('Temporary Report');
+
+ // Click Cancel to trigger hide() which resets state
+ userEvent.click(screen.getByRole('button', { name: /cancel/i }));
+
+ // After hide(), state was cleared: name should be empty
+ await waitFor(() => {
+ expect(
+ screen.getByPlaceholderText(/enter report name/i),
+ ).toHaveValue('');
+ });
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test doesn't verify modal reopen reset</b></div>
<div id="fix">
The test 'modal reopen resets local state' types a name, clicks cancel
(which calls onHide), and expects the input to be empty. However, the cancel
button only calls onHide without resetting component state, and since onHide is
a jest.fn() in the test, no reset occurs. This doesn't test 'modal reopen' as
the modal isn't reopened, and the assertion would fail since the value remains
unchanged.
</div>
</div>
<small><i>Code Review Run #c3be0c</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/features/alerts/AlertReportModal.test.tsx:
##########
@@ -1488,3 +2251,310 @@ test('tabs metadata overwrites seeded filter options',
async () => {
within(selectContainer).queryByTitle('Country'),
).not.toBeInTheDocument();
});
+
+test('selecting filter triggers chart data request with correct params', async
() => {
+ mockGetChartDataRequest.mockReset();
+ fetchMock.removeRoute(tabsEndpoint);
+ fetchMock.get(tabsEndpoint, tabsWithFilters, { name: tabsEndpoint });
+
+ mockGetChartDataRequest.mockResolvedValue({
+ json: { result: [{ data: [{ country: 'US' }, { country: 'UK' }] }] },
+ });
+
+ render(<AlertReportModal {...generateMockedProps(true, true)} />, {
+ useRedux: true,
+ });
+
+ userEvent.click(screen.getByTestId('contents-panel'));
+ await screen.findByText(/test dashboard/i);
+
+ // Wait for filter dropdown to be available
+ const filterDropdown = await waitFor(() =>
+ screen.getByRole('combobox', { name: /select filter/i }),
+ );
+
+ // Select the Country Filter using comboboxSelect pattern
+ await comboboxSelect(filterDropdown, 'Country Filter', () =>
+ document.querySelector(
+ '.ant-select-selection-item[title="Country Filter"]',
+ ),
+ );
+
+ // getChartDataRequest should have been called for filter values
+ await waitFor(() => {
+ expect(mockGetChartDataRequest).toHaveBeenCalledTimes(1);
+ });
+
+ // Verify it was called with correct datasource and groupby
+ const callArgs = mockGetChartDataRequest.mock.calls[0][0];
+ expect(callArgs.formData.groupby).toEqual(['country']);
+ expect(callArgs.formData.datasource).toBe('1__table');
+
+ mockGetChartDataRequest.mockReset();
+});
+
+test('selected filter excluded from other row dropdowns', async () => {
+ mockGetChartDataRequest.mockReset();
+ fetchMock.removeRoute(tabsEndpoint);
+ fetchMock.get(tabsEndpoint, tabsWithFilters, { name: tabsEndpoint });
+
+ mockGetChartDataRequest.mockResolvedValue({
+ json: { result: [{ data: [{ country: 'US' }] }] },
+ });
+
+ render(<AlertReportModal {...generateMockedProps(true, true)} />, {
+ useRedux: true,
+ });
+
+ userEvent.click(screen.getByTestId('contents-panel'));
+ await screen.findByText(/test dashboard/i);
+
+ // Wait for filter dropdown
+ const filterDropdown = await waitFor(() =>
+ screen.getByRole('combobox', { name: /select filter/i }),
+ );
+
+ // Select Country Filter in row 1
+ await comboboxSelect(filterDropdown, 'Country Filter', () =>
+ document.querySelector(
+ '.ant-select-selection-item[title="Country Filter"]',
+ ),
+ );
+
+ // Wait for getChartDataRequest to complete AND state update to propagate.
+ // The value dropdown becomes non-disabled once optionFilterValues is
populated.
+ await waitFor(() => {
+ expect(mockGetChartDataRequest).toHaveBeenCalled();
+ });
+ await waitFor(
+ () => {
+ const valueSelects = screen.queryAllByRole('combobox', {
+ name: /select value/i,
+ });
+ expect(valueSelects.length).toBeGreaterThan(0);
+ },
+ { timeout: 5000 },
+ );
+
+ // Add second filter row
+ const addFilterButton = screen.getByText(/apply another dashboard filter/i);
+ userEvent.click(addFilterButton);
+
+ // Wait for second row
+ await waitFor(() => {
+ expect(
+ screen.getAllByRole('combobox', { name: /select filter/i }),
+ ).toHaveLength(2);
+ });
+
+ // Open filter dropdown in row 2
+ const filterDropdowns = screen.getAllByRole('combobox', {
+ name: /select filter/i,
+ });
+ userEvent.click(filterDropdowns[1]);
+
+ // Country Filter should be excluded (dedup), City Filter should remain.
+ // Use the last virtual list since the first may belong to the closed row 1
dropdown.
+ await waitFor(() => {
+ const virtualLists = document.querySelectorAll('.rc-virtual-list');
+ const lastVirtualList = virtualLists[virtualLists.length - 1];
+ expect(
+ within(lastVirtualList as HTMLElement).queryByText('Country Filter'),
+ ).not.toBeInTheDocument();
+ expect(
+ within(lastVirtualList as HTMLElement).getByText('City Filter'),
+ ).toBeInTheDocument();
+ });
+
+ mockGetChartDataRequest.mockReset();
+}, 30000);
+
+test('invalid CC email blocks submit', async () => {
+ render(<AlertReportModal {...generateMockedProps(false, true, false)} />, {
+ useRedux: true,
+ });
+
+ // Wait for validation to fully propagate (fetch → state → validate →
enforce)
+ await screen.findByText('Edit alert');
+ await waitFor(
+ () => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeEnabled();
+ },
+ { timeout: 5000 },
+ );
+
+ // Open notification panel and show CC field
+ userEvent.click(screen.getByTestId('notification-method-panel'));
+ const addCcButton = await screen.findByText(/Add CC Recipients/i);
+ userEvent.click(addCcButton);
+
+ // Type invalid email in CC field
+ const ccInput = await screen.findByTestId('cc');
+ userEvent.type(ccInput, 'not-an-email');
+ fireEvent.blur(ccInput);
+
+ // Save should now be disabled due to invalid email format
+ await waitFor(() => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeDisabled();
+ });
+});
+
+test('invalid BCC email blocks submit', async () => {
+ render(<AlertReportModal {...generateMockedProps(false, true, false)} />, {
+ useRedux: true,
+ });
+
+ // Wait for validation to fully propagate (fetch → state → validate →
enforce)
+ await screen.findByText('Edit alert');
+ await waitFor(
+ () => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeEnabled();
+ },
+ { timeout: 5000 },
+ );
+
+ // Open notification panel and show BCC field
+ userEvent.click(screen.getByTestId('notification-method-panel'));
+ const addBccButton = await screen.findByText(/Add BCC Recipients/i);
+ userEvent.click(addBccButton);
+
+ // Type invalid email in BCC field
+ const bccInput = await screen.findByTestId('bcc');
+ userEvent.type(bccInput, 'not-an-email');
+ fireEvent.blur(bccInput);
+
+ // Save should now be disabled due to invalid email format
+ await waitFor(() => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeDisabled();
+ });
+});
+
+test('invalid saved anchor is reset on dashboard load', async () => {
+ fetchMock.removeRoute(tabsEndpoint);
+ fetchMock.get(tabsEndpoint, tabsWithFilters, { name: tabsEndpoint });
+
+ const props = generateMockedProps(true, true);
+ const editProps = {
+ ...props,
+ alert: { ...validAlert, id: 7 },
+ };
+
+ render(<AlertReportModal {...editProps} />, {
+ useRedux: true,
+ });
+
+ userEvent.click(screen.getByTestId('contents-panel'));
+ await screen.findByText(/test dashboard/i);
+
+ // Wait for dashboard tabs to load
+ await waitFor(() => {
+
expect(fetchMock.callHistory.calls(tabsEndpoint).length).toBeGreaterThan(0);
+ });
+
+ // The saved anchor 'TAB_999' is NOT in the dashboard's tabs (TAB_1, TAB_2),
+ // so it should be reset to undefined. Tab selector shows placeholder.
+ await waitFor(() => {
+ expect(screen.getByText(/select a tab/i)).toBeInTheDocument();
+ });
+
+ // TAB_999 should NOT appear as a selected value anywhere
+ expect(
+ document.querySelector('.ant-select-selection-item[title="TAB_999"]'),
+ ).not.toBeInTheDocument();
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test misses invalid anchor setup</b></div>
<div id="fix">
The test 'invalid saved anchor is reset on dashboard load' claims to verify
that an invalid saved anchor gets reset, but it doesn't initialize the alert
with an invalid anchor value. Without setting extra.dashboard.anchor to
'TAB_999', the reset logic isn't exercised, potentially allowing bugs in anchor
validation to go undetected.
</div>
</div>
<small><i>Code Review Run #c3be0c</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/features/alerts/components/AlertReportCronScheduler.test.tsx:
##########
@@ -0,0 +1,125 @@
+/**
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing picker onChange test</b></div>
<div id="fix">
The component's CronPicker can trigger onChange when its value changes in
picker mode, but this behavior is not tested. This could allow bugs in picker
mode updates to go undetected.
</div>
</div>
<small><i>Code Review Run #c3be0c</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/features/reports/ReportModal/ReportModal.test.tsx:
##########
@@ -56,115 +57,248 @@ jest.mock('@superset-ui/core', () => ({
}));
const mockedIsFeatureEnabled = isFeatureEnabled as jest.Mock;
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
-describe('Email Report Modal', () => {
- beforeEach(() => {
- mockedIsFeatureEnabled.mockImplementation(
- featureFlag => featureFlag === FeatureFlag.AlertReports,
- );
- render(<ReportModal {...defaultProps} />, { useRedux: true });
- });
- test('inputs respond correctly', () => {
- // ----- Report name textbox
- // Initial value
- const reportNameTextbox = screen.getByTestId('report-name-test');
- expect(reportNameTextbox).toHaveDisplayValue('Weekly Report');
- // Type in the textbox and assert that it worked
- userEvent.clear(reportNameTextbox);
- userEvent.type(reportNameTextbox, 'Report name text test');
- expect(reportNameTextbox).toHaveDisplayValue('Report name text test');
-
- // ----- Report description textbox
- // Initial value
- const reportDescriptionTextbox = screen.getByTestId(
- 'report-description-test',
- );
- expect(reportDescriptionTextbox).toHaveDisplayValue('');
- // Type in the textbox and assert that it worked
- userEvent.type(reportDescriptionTextbox, 'Report description text test');
- expect(reportDescriptionTextbox).toHaveDisplayValue(
- 'Report description text test',
- );
-
- // ----- Crontab
- const crontabInputs = screen.getAllByRole('combobox');
- expect(crontabInputs).toHaveLength(5);
+beforeEach(() => {
+ mockedIsFeatureEnabled.mockImplementation(
+ featureFlag => featureFlag === FeatureFlag.AlertReports,
+ );
+});
+
+test('inputs respond correctly', () => {
+ render(<ReportModal {...defaultProps} />, { useRedux: true });
+ // ----- Report name textbox
+ const reportNameTextbox = screen.getByTestId('report-name-test');
+ expect(reportNameTextbox).toHaveDisplayValue('Weekly Report');
+ userEvent.clear(reportNameTextbox);
+ userEvent.type(reportNameTextbox, 'Report name text test');
+ expect(reportNameTextbox).toHaveDisplayValue('Report name text test');
+
+ // ----- Report description textbox
+ const reportDescriptionTextbox = screen.getByTestId(
+ 'report-description-test',
+ );
+ expect(reportDescriptionTextbox).toHaveDisplayValue('');
+ userEvent.type(reportDescriptionTextbox, 'Report description text test');
+ expect(reportDescriptionTextbox).toHaveDisplayValue(
+ 'Report description text test',
+ );
+
+ // ----- Crontab
+ const crontabInputs = screen.getAllByRole('combobox');
+ expect(crontabInputs).toHaveLength(5);
+});
+
+test('does not allow user to create a report without a name', () => {
+ render(<ReportModal {...defaultProps} />, { useRedux: true });
+ const reportNameTextbox = screen.getByTestId('report-name-test');
+ const addButton = screen.getByRole('button', { name: /add/i });
+
+ expect(reportNameTextbox).toHaveDisplayValue('Weekly Report');
+ expect(addButton).toBeEnabled();
+
+ userEvent.clear(reportNameTextbox);
+
+ expect(reportNameTextbox).toHaveDisplayValue('');
+ expect(addButton).toBeDisabled();
+});
+
+test('creates a new email report via modal Add button', async () => {
+ fetchMock.post(
+ REPORT_ENDPOINT,
+ { id: 1, result: {} },
+ { name: 'post-report' },
+ );
+
+ render(<ReportModal {...defaultProps} />, { useRedux: true });
+
+ const addButton = screen.getByRole('button', { name: /add/i });
+ await waitFor(() => userEvent.click(addButton));
+
+ // Verify exactly one POST from the modal submit path
+ await waitFor(() => {
+ const postCalls = fetchMock.callHistory.calls('post-report');
+ expect(postCalls).toHaveLength(1);
});
- test('does not allow user to create a report without a name', () => {
- // Grab name textbox and add button
- const reportNameTextbox = screen.getByTestId('report-name-test');
- const addButton = screen.getByRole('button', { name: /add/i });
+ const postCalls = fetchMock.callHistory.calls('post-report');
+ const body = JSON.parse(postCalls[0].options.body as string);
+ expect(body.name).toBe('Weekly Report');
+ expect(body.type).toBe('Report');
+ expect(body.creation_method).toBe('dashboards');
+ expect(body.crontab).toBeDefined();
+ expect(body.recipients).toBeDefined();
+ expect(body.recipients[0].type).toBe('Email');
+
+ fetchMock.removeRoute('post-report');
+});
+
+test('text-based chart hides screenshot width and shows message content', ()
=> {
+ // Table is text-based: should show message content but hide custom width
+ const textChartProps = {
+ ...defaultProps,
+ dashboardId: undefined,
+ chart: { id: 1, sliceFormData: { viz_type: VizType.Table } },
+ chartName: 'My Table Chart',
+ creationMethod: 'charts' as const,
+ };
+ render(<ReportModal {...textChartProps} />, { useRedux: true });
+
+ // Message content section should be visible
+ expect(screen.getByText('Message content')).toBeInTheDocument();
+ expect(screen.getByText(/Text embedded in email/i)).toBeInTheDocument();
+
+ // Screenshot width should NOT be visible for text-based chart
+ expect(screen.queryByText('Screenshot width')).not.toBeInTheDocument();
+});
+
+test('non-text chart shows screenshot width and message content', () => {
+ const lineChartProps = {
+ ...defaultProps,
+ dashboardId: undefined,
+ chart: { id: 1, sliceFormData: { viz_type: VizType.Line } },
+ chartName: 'My Line Chart',
+ creationMethod: 'charts' as const,
+ };
+ render(<ReportModal {...lineChartProps} />, { useRedux: true });
- // Add button should be enabled while name textbox has text
- expect(reportNameTextbox).toHaveDisplayValue('Weekly Report');
- expect(addButton).toBeEnabled();
+ // Both message content and screenshot width should be visible
+ expect(screen.getByText('Message content')).toBeInTheDocument();
+ expect(screen.getByText('Screenshot width')).toBeInTheDocument();
+});
+
+test('dashboard report hides message content section', () => {
+ const dashboardProps = {
+ ...defaultProps,
+ chart: undefined,
+ dashboardName: 'My Dashboard',
+ };
+ render(<ReportModal {...dashboardProps} />, { useRedux: true });
+
+ // Message content (radio group) should NOT be visible for dashboard
+ expect(screen.queryByText('Message content')).not.toBeInTheDocument();
+ // Screenshot width SHOULD be visible
+ expect(screen.getByText('Screenshot width')).toBeInTheDocument();
+});
+
+test('renders edit mode when report exists in store', () => {
+ const existingReport = {
+ id: 42,
+ name: 'Existing Dashboard Report',
+ description: 'An existing report',
+ crontab: '0 9 * * 1',
+ creation_method: 'dashboards',
+ report_format: 'PNG',
+ timezone: 'America/New_York',
+ active: true,
+ type: 'Report',
+ dashboard: 1,
+ owners: [1],
+ recipients: [
+ {
+ recipient_config_json: { target: '[email protected]' },
+ type: 'Email',
+ },
+ ],
+ };
+ const store = createStore(
+ {
+ reports: {
+ dashboards: { 1: existingReport },
+ },
+ },
+ reducerIndex,
+ );
+
+ render(<ReportModal {...defaultProps} />, { useRedux: true, store });
+
+ // Edit mode title
+ expect(screen.getByText('Edit email report')).toBeInTheDocument();
+ // Report name populated from store
+ expect(screen.getByTestId('report-name-test')).toHaveDisplayValue(
+ 'Existing Dashboard Report',
+ );
+ // Save button instead of Add
+ expect(screen.getByRole('button', { name: /save/i })).toBeInTheDocument();
+});
+
+test('edit mode dispatches editReport via PUT on save', async () => {
+ const existingReport = {
+ id: 42,
+ name: 'Existing Report',
+ description: '',
+ crontab: '0 12 * * 1',
+ creation_method: 'dashboards',
+ report_format: 'PNG',
+ timezone: 'America/New_York',
+ active: true,
+ type: 'Report',
+ dashboard: 1,
+ owners: [1],
+ recipients: [
+ {
+ recipient_config_json: { target: '[email protected]' },
+ type: 'Email',
+ },
+ ],
+ };
+ const store = createStore(
+ {
+ reports: {
+ dashboards: { 1: existingReport },
+ },
+ },
+ reducerIndex,
+ );
- // Clear the text from the name textbox
- userEvent.clear(reportNameTextbox);
+ fetchMock.put(
+ 'glob:*/api/v1/report/42',
+ { id: 42, result: {} },
+ {
+ name: 'put-report-42',
+ },
+ );
+
+ render(<ReportModal {...defaultProps} />, { useRedux: true, store });
+
+ expect(screen.getByText('Edit email report')).toBeInTheDocument();
+ const saveButton = screen.getByRole('button', { name: /save/i });
+ await waitFor(() => userEvent.click(saveButton));
- // Add button should now be disabled, blocking user from creation
- expect(reportNameTextbox).toHaveDisplayValue('');
- expect(addButton).toBeDisabled();
+ await waitFor(() => {
+ const calls = fetchMock.callHistory.calls('put-report-42');
+ expect(calls.length).toBeGreaterThan(0);
});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete PUT body assertion</b></div>
<div id="fix">
This test verifies that a PUT request is dispatched but doesn't check the
request body, which could allow bugs in data transmission to pass. For
consistency with the 'creates a new email report' test, add body assertions to
confirm the expected report data is sent in the PUT request.
</div>
</div>
<small><i>Code Review Run #c3be0c</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]
