bito-code-review[bot] commented on code in PR #38591:
URL: https://github.com/apache/superset/pull/38591#discussion_r2997895366
##########
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:
<!-- Bito Reply -->
The PUT body assertions have been added in a later commit, now verifying
type, name, crontab, report_format, dashboard, and recipients as suggested. The
original comment referred to the initial test version without these checks.
--
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]
