sadpandajoe commented on code in PR #38591:
URL: https://github.com/apache/superset/pull/38591#discussion_r3013379973
##########
superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx:
##########
@@ -56,115 +57,260 @@ 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 });
+
+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');
Review Comment:
This project uses `@testing-library/user-event` v12.8.3, which has a
**synchronous** API for `type()` and `clear()`. The async API was introduced in
v14. No `await` is needed here.
##########
superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx:
##########
@@ -56,115 +57,260 @@ 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 });
+
+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);
Review Comment:
Same as above — `userEvent` v12.8.3 has synchronous `clear()`. No race
condition possible.
##########
superset-frontend/src/pages/AlertReportList/AlertReportList.test.tsx:
##########
@@ -16,271 +16,469 @@
* specific language governing permissions and limitations
* under the License.
*/
+import type React from 'react';
import fetchMock from 'fetch-mock';
-import configureStore from 'redux-mock-store';
-import thunk from 'redux-thunk';
import {
render,
screen,
fireEvent,
waitFor,
+ createStore,
} from 'spec/helpers/testing-library';
+import { Provider } from 'react-redux';
import { MemoryRouter } from 'react-router-dom';
import { QueryParamProvider } from 'use-query-params';
import { ReactRouter5Adapter } from 'use-query-params/adapters/react-router-5';
-import React from 'react';
import AlertListComponent from 'src/pages/AlertReportList';
-// Cast to accept partial mock props in tests
+jest.setTimeout(30000);
+
const AlertList = AlertListComponent as unknown as React.FC<
Record<string, any>
>;
-const mockStore = configureStore([thunk]);
-const store = mockStore({});
-
-const alertsEndpoint = 'glob:*/api/v1/report/?*';
-const alertEndpoint = 'glob:*/api/v1/report/*';
-const alertsInfoEndpoint = 'glob:*/api/v1/report/_info*';
-const alertsCreatedByEndpoint = 'glob:*/api/v1/report/related/created_by*';
-
-const mockalerts = Array.from({ length: 3 }, (_, i) => ({
- active: true,
- changed_by: {
- first_name: `user ${i}`,
- id: i,
+// -- Mock data (IDs start at 1 to avoid the `if (data?.id)` falsy guard) --
+
+const mockAlerts = [
+ {
+ id: 1,
+ name: 'Weekly Sales Alert',
+ active: true,
+ last_state: 'Success',
+ type: 'Alert',
+ owners: [{ id: 1, first_name: 'Admin', last_name: 'User' }],
+ recipients: [{ id: 1, type: 'Email' }],
+ changed_by: { id: 1, first_name: 'Admin', last_name: 'User' },
+ changed_on_delta_humanized: '1 day ago',
+ created_by: { id: 1, first_name: 'Admin', last_name: 'User' },
+ created_on: new Date().toISOString(),
+ last_eval_dttm: Date.now(),
+ crontab: '0 9 * * 1',
+ crontab_humanized: 'Every Monday at 09:00',
+ timezone: 'UTC',
},
- changed_on_delta_humanized: `${i} day(s) ago`,
- created_by: {
- first_name: `user ${i}`,
- id: i,
+ {
+ id: 2,
+ name: 'Daily Error Alert',
+ active: true,
+ last_state: 'Error',
+ type: 'Alert',
+ owners: [{ id: 2, first_name: 'Data', last_name: 'Analyst' }],
+ recipients: [{ id: 2, type: 'Slack' }],
+ changed_by: { id: 2, first_name: 'Data', last_name: 'Analyst' },
+ changed_on_delta_humanized: '2 days ago',
+ created_by: { id: 2, first_name: 'Data', last_name: 'Analyst' },
+ created_on: new Date().toISOString(),
+ last_eval_dttm: Date.now(),
+ crontab: '0 8 * * *',
+ crontab_humanized: 'Every day at 08:00',
+ timezone: 'US/Pacific',
},
- created_on: new Date().toISOString,
- id: i,
- last_eval_dttm: Date.now(),
- last_state: 'ok',
- name: `alert ${i} `,
- owners: [{ id: 1 }],
- recipients: [
- {
- id: `${i}`,
- type: 'email',
- },
- ],
- type: 'alert',
-}));
+ {
+ id: 3,
+ name: 'Monthly Revenue Alert',
+ active: false,
+ last_state: 'Working',
+ type: 'Alert',
+ owners: [{ id: 1, first_name: 'Admin', last_name: 'User' }],
+ recipients: [{ id: 3, type: 'Email' }],
+ changed_by: { id: 1, first_name: 'Admin', last_name: 'User' },
+ changed_on_delta_humanized: '5 days ago',
+ created_by: { id: 1, first_name: 'Admin', last_name: 'User' },
+ created_on: new Date().toISOString(),
+ last_eval_dttm: Date.now(),
+ crontab: '0 0 1 * *',
+ crontab_humanized: 'First day of the month',
+ timezone: 'UTC',
+ },
+];
+
+const mockReports = [
+ {
+ id: 10,
+ name: 'Weekly Dashboard Report',
+ active: true,
+ last_state: 'Success',
+ type: 'Report',
+ owners: [{ id: 1, first_name: 'Admin', last_name: 'User' }],
+ recipients: [{ id: 10, type: 'Email' }],
+ changed_by: { id: 1, first_name: 'Admin', last_name: 'User' },
+ changed_on_delta_humanized: '1 day ago',
+ created_by: { id: 1, first_name: 'Admin', last_name: 'User' },
+ created_on: new Date().toISOString(),
+ last_eval_dttm: Date.now(),
+ crontab: '0 9 * * 1',
+ crontab_humanized: 'Every Monday at 09:00',
+ timezone: 'UTC',
+ },
+ {
+ id: 11,
+ name: 'Monthly KPI Report',
+ active: false,
+ last_state: 'Not triggered',
+ type: 'Report',
+ owners: [{ id: 1, first_name: 'Admin', last_name: 'User' }],
+ recipients: [{ id: 11, type: 'Slack' }],
+ changed_by: { id: 1, first_name: 'Admin', last_name: 'User' },
+ changed_on_delta_humanized: '3 days ago',
+ created_by: { id: 1, first_name: 'Admin', last_name: 'User' },
+ created_on: new Date().toISOString(),
+ last_eval_dttm: Date.now(),
+ crontab: '0 0 1 * *',
+ crontab_humanized: 'First day of the month',
+ timezone: 'UTC',
+ },
+];
const mockUser = {
userId: 1,
- firstName: 'user 1',
- lastName: 'lastname',
+ firstName: 'Admin',
+ lastName: 'User',
+};
+
+// -- API endpoints (named for cleanup) --
+
+const ENDPOINTS = {
+ LIST: 'glob:*/api/v1/report/?*',
+ INFO: 'glob:*/api/v1/report/_info*',
+ SINGLE: 'glob:*/api/v1/report/*',
Review Comment:
The SINGLE pattern is only registered for PUT and DELETE methods. No test
performs a bulk DELETE API call — the bulk select test only verifies UI state
(selected count, controls). The routing concern is theoretical and has no
impact.
##########
tests/unit_tests/reports/notifications/slack_tests.py:
##########
@@ -377,3 +377,57 @@ def test_send_slack_no_feature_flag(
```
""",
)
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_slackv2_send_without_channels_raises(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+ from superset.reports.notifications.base import NotificationContent
+ from superset.reports.notifications.exceptions import
NotificationParamException
+
+ flask_global_mock.logs_context = {}
+ content = NotificationContent(name="test", header_data=mock_header_data)
+ notification = SlackV2Notification(
+ recipient=ReportRecipients(
+ type=ReportRecipientType.SLACKV2,
+ recipient_config_json='{"target": ""}',
+ ),
+ content=content,
+ )
+ with pytest.raises(NotificationParamException, match="No recipients"):
+ notification.send()
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_slack_mixin_get_body_truncates_large_table(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+ from superset.reports.notifications.base import NotificationContent
+
+ flask_global_mock.logs_context = {}
+ # Create a large DataFrame that exceeds the 4000-char message limit
+ large_df = pd.DataFrame({"col_" + str(i): range(100) for i in range(10)})
+ content = NotificationContent(
+ name="test",
+ header_data=mock_header_data,
+ embedded_data=large_df,
+ description="desc",
+ )
+ notification = SlackV2Notification(
+ recipient=ReportRecipients(
+ type=ReportRecipientType.SLACKV2,
+ recipient_config_json='{"target": "some_channel"}',
+ ),
+ content=content,
+ )
+ body = notification._get_body(content=content)
Review Comment:
The truncation marker assertion confirms the feature works. Adding a length
check is a nice-to-have but out of scope for this test-coverage PR.
##########
tests/unit_tests/reports/schemas_test.py:
##########
@@ -75,3 +75,184 @@ def test_report_post_schema_custom_width_validation(mocker:
MockerFixture) -> No
assert excinfo.value.messages == {
"custom_width": ["Screenshot width must be between 100px and 200px"]
}
+
+
+MINIMAL_POST_PAYLOAD = {
+ "type": "Report",
+ "name": "A report",
+ "crontab": "* * * * *",
+ "timezone": "America/Los_Angeles",
+}
+
+CUSTOM_WIDTH_CONFIG = {
+ "ALERT_REPORTS_MIN_CUSTOM_SCREENSHOT_WIDTH": 600,
+ "ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH": 2400,
+}
+
+
[email protected](
+ "schema_class,payload_base",
+ [
+ (ReportSchedulePostSchema, MINIMAL_POST_PAYLOAD),
+ (ReportSchedulePutSchema, {}),
+ ],
+ ids=["post", "put"],
+)
[email protected](
+ "width,should_pass",
+ [
+ (599, False),
+ (600, True),
+ (2400, True),
+ (2401, False),
+ (None, True),
+ ],
+)
+def test_custom_width_boundary_values(
+ mocker: MockerFixture,
+ schema_class: type,
+ payload_base: dict[str, object],
+ width: int | None,
+ should_pass: bool,
+) -> None:
+ mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
+ schema = schema_class()
+ payload = {**payload_base, "custom_width": width}
+
+ if should_pass:
+ schema.load(payload)
+ else:
+ with pytest.raises(ValidationError) as exc:
+ schema.load(payload)
+ assert "custom_width" in exc.value.messages
+
+
+def test_working_timeout_validation(mocker: MockerFixture) -> None:
+ mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
+ post_schema = ReportSchedulePostSchema()
+ put_schema = ReportSchedulePutSchema()
+
+ # POST: working_timeout=0 and -1 are invalid (min=1)
+ with pytest.raises(ValidationError) as exc:
+ post_schema.load({**MINIMAL_POST_PAYLOAD, "working_timeout": 0})
+ assert "working_timeout" in exc.value.messages
+
+ with pytest.raises(ValidationError) as exc:
+ post_schema.load({**MINIMAL_POST_PAYLOAD, "working_timeout": -1})
+ assert "working_timeout" in exc.value.messages
+
+ # POST: working_timeout=1 is valid
+ post_schema.load({**MINIMAL_POST_PAYLOAD, "working_timeout": 1})
+
+ # PUT: working_timeout=None is valid (allow_none=True)
+ put_schema.load({"working_timeout": None})
+
+
+def test_log_retention_post_vs_put_parity(mocker: MockerFixture) -> None:
+ mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
+ post_schema = ReportSchedulePostSchema()
+ put_schema = ReportSchedulePutSchema()
+
+ # POST: log_retention=0 is invalid (min=1)
+ with pytest.raises(ValidationError) as exc:
+ post_schema.load({**MINIMAL_POST_PAYLOAD, "log_retention": 0})
+ assert "log_retention" in exc.value.messages
+
+ # POST: log_retention=1 is valid
+ post_schema.load({**MINIMAL_POST_PAYLOAD, "log_retention": 1})
+
+ # PUT: log_retention=0 is valid (min=0)
+ put_schema.load({"log_retention": 0})
+
+
+def test_report_type_disallows_database(mocker: MockerFixture) -> None:
+ mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
+ schema = ReportSchedulePostSchema()
+
+ with pytest.raises(ValidationError) as exc:
+ schema.load({**MINIMAL_POST_PAYLOAD, "database": 1})
+ assert "database" in exc.value.messages
+
+
+def test_alert_type_allows_database(mocker: MockerFixture) -> None:
+ """Alert type should accept database; only Report type blocks it."""
+ mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
+ schema = ReportSchedulePostSchema()
+ result = schema.load({**MINIMAL_POST_PAYLOAD, "type": "Alert", "database":
1})
+ assert result["database"] == 1
+
+
+# ---------------------------------------------------------------------------
+# Phase 1b gap closure: crontab validator, name length, PUT parity
+# ---------------------------------------------------------------------------
+
+
[email protected](
+ "crontab,should_pass",
Review Comment:
`"crontab,should_pass"` is standard pytest parametrize syntax — both
comma-separated strings and tuples are valid. See [pytest
docs](https://docs.pytest.org/en/stable/how-to/parametrize.html).
--
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]
