codeant-ai-for-open-source[bot] commented on code in PR #38711:
URL: https://github.com/apache/superset/pull/38711#discussion_r2954594547
##########
tests/integration_tests/reports/api_tests.py:
##########
@@ -1527,6 +1527,177 @@ def test_update_report_schedule(self):
assert updated_model.chart_id == report_schedule_data["chart"]
assert updated_model.database_id == report_schedule_data["database"]
+ @pytest.mark.usefixtures("create_report_schedules")
+ def test_update_report_schedule_clear_recipients(self):
+ """
+ ReportSchedule API: clear recipients on empty list
+ """
+ report_schedule = (
+ db.session.query(ReportSchedule)
+ .filter(ReportSchedule.name == "name2")
+ .one_or_none()
+ )
+ assert len(report_schedule.recipients) == 2
+
+ self.login(ADMIN_USERNAME)
+ report_schedule_data = {
+ "recipients": [],
+ }
+
+ uri = f"api/v1/report/{report_schedule.id}"
+ rv = self.put_assert_metric(uri, report_schedule_data, "put")
+ assert rv.status_code == 200
+ db.session.expire(report_schedule)
+ assert report_schedule.recipients == []
+
+ @pytest.mark.usefixtures("create_report_schedules")
+ def test_update_report_schedule_empty_email_target(self):
+ """
+ ReportSchedule API: Test update with empty email target returns 400
+ """
+ report_schedule = (
+ db.session.query(ReportSchedule)
+ .filter(ReportSchedule.name == "name2")
+ .one_or_none()
+ )
+ self.login(ADMIN_USERNAME)
+ report_schedule_data = {
+ "recipients": [
+ {
+ "type": ReportRecipientType.EMAIL,
+ "recipient_config_json": {"target": ""},
+ }
+ ],
+ }
+ uri = f"api/v1/report/{report_schedule.id}"
+ rv = self.put_assert_metric(uri, report_schedule_data, "put")
+ assert rv.status_code == 400
+
+ @pytest.mark.usefixtures("create_report_schedules")
+ def test_update_report_schedule_invalid_email(self):
+ """
+ ReportSchedule API: Test update with invalid email returns 400
+ """
+ report_schedule = (
+ db.session.query(ReportSchedule)
+ .filter(ReportSchedule.name == "name2")
+ .one_or_none()
+ )
+ self.login(ADMIN_USERNAME)
+ report_schedule_data = {
+ "recipients": [
+ {
+ "type": ReportRecipientType.EMAIL,
+ "recipient_config_json": {"target": "notanemail"},
+ }
+ ],
+ }
+ uri = f"api/v1/report/{report_schedule.id}"
+ rv = self.put_assert_metric(uri, report_schedule_data, "put")
+ assert rv.status_code == 400
+
+ @pytest.mark.usefixtures("create_report_schedules")
+ def test_update_report_schedule_invalid_cc_email(self):
+ """
+ ReportSchedule API: Test update with invalid ccTarget
+ """
+ report_schedule = (
+ db.session.query(ReportSchedule)
+ .filter(ReportSchedule.name == "name2")
+ .one_or_none()
+ )
+ self.login(ADMIN_USERNAME)
+ report_schedule_data = {
+ "recipients": [
+ {
+ "type": ReportRecipientType.EMAIL,
+ "recipient_config_json": {
+ "target": "[email protected]",
+ "ccTarget": "bademail",
+ },
+ }
+ ],
+ }
+ uri = f"api/v1/report/{report_schedule.id}"
+ rv = self.put_assert_metric(uri, report_schedule_data, "put")
+ assert rv.status_code == 400
+
+ @pytest.mark.usefixtures("create_report_schedules")
+ def test_update_report_schedule_invalid_bcc_email(self):
+ """
+ ReportSchedule API: Test update with invalid bccTarget
+ """
+ report_schedule = (
+ db.session.query(ReportSchedule)
+ .filter(ReportSchedule.name == "name2")
+ .one_or_none()
+ )
+ self.login(ADMIN_USERNAME)
+ report_schedule_data = {
+ "recipients": [
+ {
+ "type": ReportRecipientType.EMAIL,
+ "recipient_config_json": {
+ "target": "[email protected]",
+ "bccTarget": "bademail",
+ },
+ }
+ ],
+ }
+ uri = f"api/v1/report/{report_schedule.id}"
+ rv = self.put_assert_metric(uri, report_schedule_data, "put")
+ assert rv.status_code == 400
+
+ @pytest.mark.usefixtures("create_report_schedules")
+ def test_update_report_schedule_slack_empty_target_allowed(self):
+ """
+ ReportSchedule API: Test that Slack recipients skip email validation
+ """
+ report_schedule = (
+ db.session.query(ReportSchedule)
+ .filter(ReportSchedule.name == "name2")
+ .one_or_none()
+ )
+ self.login(ADMIN_USERNAME)
+ report_schedule_data = {
+ "recipients": [
+ {
+ "type": ReportRecipientType.SLACK,
+ "recipient_config_json": {"target": ""},
+ }
+ ],
+ }
+ uri = f"api/v1/report/{report_schedule.id}"
+ rv = self.put_assert_metric(uri, report_schedule_data, "put")
+ assert rv.status_code == 200
Review Comment:
**Suggestion:** This test currently enforces that an empty Slack `target` is
accepted, but downstream Slack delivery code requires at least one channel and
raises runtime notification errors when recipients are empty. The API should
reject this invalid configuration during update, so the assertion should expect
a validation failure instead of success. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Scheduled Slack reports fail during `reports.execute` task.
- ⚠️ Report state flips to ERROR after send attempt.
- ⚠️ Invalid Slack recipients persist via PUT API.
```
</details>
```suggestion
rv = self.put_assert_metric(uri, report_schedule_data, "put")
assert rv.status_code == 400
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call `PUT /api/v1/report/{id}` with recipient payload
`{"type":"Slack","recipient_config_json":{"target":""}}`; request is handled
by
`ReportScheduleRestApi.put()` at `superset/reports/api.py:394-95`.
2. The payload passes schema validation because
`ReportRecipientSchema.validate_email_recipients()` returns early for
non-email recipients
(`superset/reports/schemas.py:146-148`), and update persists recipients in
`ReportScheduleDAO.update()` (`superset/daos/report.py:190-202`).
3. This behavior is explicitly exercised by the new integration test
`test_update_report_schedule_slack_empty_target_allowed` in
`tests/integration_tests/reports/api_tests.py:1652-1672`, which expects
`200`.
4. When the schedule later executes through Celery
(`superset/tasks/scheduler.py:116-134`), notification send path
(`superset/commands/report/execute.py:671-699`) processes Slack recipients;
for Slack v2
path, empty `target` becomes no channels and raises
`NotificationParamException` in
`superset/reports/notifications/slackv2.py:91-93`, causing execution errors.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/reports/api_tests.py
**Line:** 1671:1672
**Comment:**
*Logic Error: This test currently enforces that an empty Slack `target`
is accepted, but downstream Slack delivery code requires at least one channel
and raises runtime notification errors when recipients are empty. The API
should reject this invalid configuration during update, so the assertion should
expect a validation failure instead of success.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38711&comment_hash=8f7d09acf0956edde1961ea1d5e88ccdf618346f591a48240771b1f44f8ae697&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38711&comment_hash=8f7d09acf0956edde1961ea1d5e88ccdf618346f591a48240771b1f44f8ae697&reaction=dislike'>👎</a>
--
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]
