codeant-ai-for-open-source[bot] commented on code in PR #41177:
URL: https://github.com/apache/superset/pull/41177#discussion_r3482974283
##########
superset/commands/report/exceptions.py:
##########
@@ -200,6 +201,26 @@ class ReportScheduleDataFrameFailedError(CommandException):
message = _("Report Schedule execution failed when generating a
dataframe.")
+class ReportScheduleExecutorNotFoundError(CommandException):
+ """Raised when the configured report executor user cannot be resolved."""
+
+ # 5xx (not 4xx): a missing executor user is a server-side misconfiguration,
+ # not a malformed client request. The status drives get_logger_from_status,
+ # which marks the Celery task FAILURE for 5xx — keeping the executor
problem
+ # visible to ops task-state alerting. A 4xx would log a WARNING and leave
the
+ # task non-FAILURE, hiding the misconfiguration from that signal.
+ status = 500
+
+ def __init__(self, username: str = "", exception: Optional[Exception] =
None):
Review Comment:
**Suggestion:** Add an explicit return type annotation to the new
initializer so the method is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new initializer is partially typed: its parameters have type hints, but
the function itself omits an explicit return annotation (`-> None`).
This matches the custom rule requiring newly added Python functions/methods
to be fully typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dfd5d248280447a896039909c12f9e1e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=dfd5d248280447a896039909c12f9e1e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/exceptions.py
**Line:** 214:214
**Comment:**
*Custom Rule: Add an explicit return type annotation to the new
initializer so the method is fully typed.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=97e0fa39eeb432cccd56e839cc6dcb9fc42b2c58eeb7171527ad0453799a3948&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=97e0fa39eeb432cccd56e839cc6dcb9fc42b2c58eeb7171527ad0453799a3948&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/report/execute_test.py:
##########
@@ -1096,6 +1097,103 @@ def test_screenshot_width_calculation(
)
+def _executor_report_state(mocker: MockerFixture) -> BaseReportState:
+ report_schedule = create_report_schedule(mocker)
+ # _get_csv_data/_get_embedded_data build a chart-data URL from chart_id
+ # before resolving the executor; give it a concrete value so URL building
+ # succeeds and the executor resolution is actually reached.
+ report_schedule.chart_id = 1
+ report_schedule.force_screenshot = False
+ return BaseReportState(
+ report_schedule=report_schedule,
+ scheduled_dttm=datetime.now(),
+ execution_id=UUID("084e7ee6-5557-4ecd-9632-b7f39c9ec524"),
+ )
Review Comment:
**Suggestion:** Add a short docstring to this new helper function so it
complies with the requirement that newly added functions are documented.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python helper function and it does not include a
docstring.
The custom rule requires new functions to be documented inline, so the
suggestion
correctly identifies a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6e4f1ffb14074c2ca269f2da65d4ab84&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6e4f1ffb14074c2ca269f2da65d4ab84&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/report/execute_test.py
**Line:** 1100:1111
**Comment:**
*Custom Rule: Add a short docstring to this new helper function so it
complies with the requirement that newly added functions are documented.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=88d3dfa1fc629f70bf4e536c41b9d55964348582171fa6a858d4938be549bc6b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=88d3dfa1fc629f70bf4e536c41b9d55964348582171fa6a858d4938be549bc6b&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/report/execute_test.py:
##########
@@ -1171,6 +1269,122 @@ def
test_update_recipient_to_slack_v2_missing_channels(mocker: MockerFixture):
mock_cmmd.update_report_schedule_slack_v2()
+def test_update_recipient_to_slack_v2_reverts_all_on_partial_failure(
+ mocker: MockerFixture,
+) -> None:
+ """
+ When the second of two Slack recipients fails channel resolution, BOTH
+ recipients are fully reverted — type AND exact original
+ ``recipient_config_json`` string — not just the loop variable's type. This
+ prevents the intervening ``create_log`` commit from flushing a
half-migrated,
+ inconsistent state.
+ """
+
+ def channels_side_effect(search_string, types, exact_match):
+ if search_string == "Channel-1":
+ return [
+ {
+ "id": "id_channel_1",
+ "name": "Channel-1",
+ "is_member": True,
+ "is_private": False,
+ }
+ ]
+ # Second recipient: no channel found → length mismatch →
UpdateFailedError
+ return []
Review Comment:
**Suggestion:** Add parameter and return type annotations to this newly
added nested function to satisfy the full typing requirement for new Python
code. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added nested function omits type hints for all parameters and its
return value. The rule requires new Python functions to be fully typed, so
this
is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ca30c9dec0144553a19a2aa05a304e93&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ca30c9dec0144553a19a2aa05a304e93&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/report/execute_test.py
**Line:** 1283:1294
**Comment:**
*Custom Rule: Add parameter and return type annotations to this newly
added nested function to satisfy the full typing requirement for new Python
code.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=602c3d2d83baf8e1e40ad3180658f54585d2e2a012d25c48d2486239d4643a79&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=602c3d2d83baf8e1e40ad3180658f54585d2e2a012d25c48d2486239d4643a79&reaction=dislike'>👎</a>
##########
tests/unit_tests/reports/notifications/webhook_tests.py:
##########
@@ -269,3 +271,156 @@ class MockResponse:
with pytest.raises(NotificationParamException, match="redirect"):
webhook_notification.send()
+
+
+def _make_webhook(mock_header_data) -> WebhookNotification:
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+ from superset.reports.notifications.base import NotificationContent
+
+ content = NotificationContent(
+ name="test alert", header_data=mock_header_data, description="Test
description"
+ )
+ return WebhookNotification(
+ recipient=ReportRecipients(
+ type=ReportRecipientType.WEBHOOK,
+ recipient_config_json='{"target": "https://example.com/webhook"}',
+ ),
+ content=content,
+ )
+
+
+class _MockServerErrorResponse:
+ status_code = 500
+ text = ""
+
+
+def _allow_internal_app() -> type:
Review Comment:
**Suggestion:** Add an inline docstring to this newly added helper function
to document its purpose and behavior. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added helper function has no docstring immediately under its
definition. The rule requires newly added Python functions and classes to
include docstrings, so this is a verified violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4df1a92ce5844d89b9f996aafc3e1a5e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4df1a92ce5844d89b9f996aafc3e1a5e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/reports/notifications/webhook_tests.py
**Line:** 297:297
**Comment:**
*Custom Rule: Add an inline docstring to this newly added helper
function to document its purpose and behavior.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=b9da8ce8f68f21f61fa2b2049b3ad4ba01f21f1e01ad190fb7a20c4eeca3094a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=b9da8ce8f68f21f61fa2b2049b3ad4ba01f21f1e01ad190fb7a20c4eeca3094a&reaction=dislike'>👎</a>
##########
tests/unit_tests/reports/notifications/webhook_tests.py:
##########
@@ -269,3 +271,156 @@ class MockResponse:
with pytest.raises(NotificationParamException, match="redirect"):
webhook_notification.send()
+
+
+def _make_webhook(mock_header_data) -> WebhookNotification:
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+ from superset.reports.notifications.base import NotificationContent
+
+ content = NotificationContent(
+ name="test alert", header_data=mock_header_data, description="Test
description"
+ )
+ return WebhookNotification(
+ recipient=ReportRecipients(
+ type=ReportRecipientType.WEBHOOK,
+ recipient_config_json='{"target": "https://example.com/webhook"}',
+ ),
+ content=content,
+ )
+
+
+class _MockServerErrorResponse:
+ status_code = 500
+ text = ""
+
+
+def _allow_internal_app() -> type:
+ class MockCurrentApp:
+ config = {
+ "ALERT_REPORTS_WEBHOOK_HTTPS_ONLY": True,
+ "ALERT_REPORTS_WEBHOOK_ALLOW_INTERNAL_HOSTS": True,
+ }
+
+ return MockCurrentApp
+
+
+def test_send_backoff_bounded_by_max_time(monkeypatch, mock_header_data) ->
None:
Review Comment:
**Suggestion:** Add explicit type annotations for both test function
parameters to satisfy full typing requirements in new code. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added test function has untyped parameters (`monkeypatch`,
`mock_header_data`). The custom rule explicitly flags new Python functions that
omit parameter type hints, so the violation is real.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bbae1f4963324ce3b171272b7b79d257&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bbae1f4963324ce3b171272b7b79d257&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/reports/notifications/webhook_tests.py
**Line:** 307:307
**Comment:**
*Custom Rule: Add explicit type annotations for both test function
parameters to satisfy full typing requirements in new code.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=56ff5341301ad28c8a5d5c825961a0e382e320cea208768c131cc5f5b8de57bd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=56ff5341301ad28c8a5d5c825961a0e382e320cea208768c131cc5f5b8de57bd&reaction=dislike'>👎</a>
##########
tests/unit_tests/reports/notifications/webhook_tests.py:
##########
@@ -269,3 +271,156 @@ class MockResponse:
with pytest.raises(NotificationParamException, match="redirect"):
webhook_notification.send()
+
+
+def _make_webhook(mock_header_data) -> WebhookNotification:
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+ from superset.reports.notifications.base import NotificationContent
+
+ content = NotificationContent(
+ name="test alert", header_data=mock_header_data, description="Test
description"
+ )
+ return WebhookNotification(
+ recipient=ReportRecipients(
+ type=ReportRecipientType.WEBHOOK,
+ recipient_config_json='{"target": "https://example.com/webhook"}',
+ ),
+ content=content,
+ )
+
+
+class _MockServerErrorResponse:
+ status_code = 500
+ text = ""
+
+
+def _allow_internal_app() -> type:
+ class MockCurrentApp:
+ config = {
+ "ALERT_REPORTS_WEBHOOK_HTTPS_ONLY": True,
+ "ALERT_REPORTS_WEBHOOK_ALLOW_INTERNAL_HOSTS": True,
+ }
+
+ return MockCurrentApp
+
+
+def test_send_backoff_bounded_by_max_time(monkeypatch, mock_header_data) ->
None:
+ """
+ A persistently failing (500) target gives up on wall-time (``max_time``),
+ not just ``max_tries``. ``freeze_time(auto_tick_seconds=30)`` advances the
+ clock on every time access (backoff measures elapsed via ``datetime.now``,
+ which freezegun freezes and ticks), so cumulative elapsed crosses
``max_time=120``
+ before ``max_tries=5`` is exhausted. We assert the discriminating
*property*
+ — gave up on wall-time strictly before exhausting ``max_tries`` — rather
than
+ a pinned count, because freezegun's per-call tick count is an opaque
+ implementation detail. If ``max_time`` is removed this rises to the full 5
+ (RED); the flat-clock companion test below anchors that 5 is the ceiling.
+ The terminal exception type is unchanged on giveup.
+ """
+ webhook_notification = _make_webhook(mock_header_data)
+ post_calls: list[int] = []
+
+ def fake_post(*args, **kwargs) -> _MockServerErrorResponse:
+ post_calls.append(1)
+ return _MockServerErrorResponse()
+
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.current_app",
_allow_internal_app()
+ )
+ monkeypatch.setattr(
+
"superset.reports.notifications.webhook.feature_flag_manager.is_feature_enabled",
+ lambda flag: True,
+ )
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.requests.post", fake_post
+ )
+ monkeypatch.setattr("time.sleep", lambda *a, **k: None)
+
+ with freeze_time("2020-01-01", auto_tick_seconds=30):
+ with pytest.raises(NotificationUnprocessableException):
+ webhook_notification.send()
+
+ # 1 < count < max_tries(5): wall-time bound fired before tries exhausted.
+ assert 1 < len(post_calls) < 5
+
+
+def test_send_flat_clock_falls_back_to_max_tries(monkeypatch,
mock_header_data) -> None:
Review Comment:
**Suggestion:** Add explicit parameter type hints for this new test function
so it follows the project's typing rule. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test function and both parameters are untyped.
That directly violates the rule requiring new Python code to be fully typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=768f76c1011f4edba53ff322397c8709&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=768f76c1011f4edba53ff322397c8709&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/reports/notifications/webhook_tests.py
**Line:** 347:347
**Comment:**
*Custom Rule: Add explicit parameter type hints for this new test
function so it follows the project's typing rule.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=dc5eeffbc8c2efd033fd4a9d90454153861ca66cd86d5034e1b4d6e4b35486bc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=dc5eeffbc8c2efd033fd4a9d90454153861ca66cd86d5034e1b4d6e4b35486bc&reaction=dislike'>👎</a>
##########
tests/unit_tests/reports/notifications/webhook_tests.py:
##########
@@ -269,3 +271,156 @@ class MockResponse:
with pytest.raises(NotificationParamException, match="redirect"):
webhook_notification.send()
+
+
+def _make_webhook(mock_header_data) -> WebhookNotification:
Review Comment:
**Suggestion:** Add a concrete type annotation for the `mock_header_data`
parameter so this new helper is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python helper function, and its `mock_header_data`
parameter is not type-annotated. The rule requires new Python functions to be
fully typed, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b557761017004acca180a3b9e52aa1d8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b557761017004acca180a3b9e52aa1d8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/reports/notifications/webhook_tests.py
**Line:** 276:276
**Comment:**
*Custom Rule: Add a concrete type annotation for the `mock_header_data`
parameter so this new helper is fully typed.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=3415841c99fdfd544c13751c2ed85c1b8de2b4d8f6dc9edc5b7fa64354880522&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=3415841c99fdfd544c13751c2ed85c1b8de2b4d8f6dc9edc5b7fa64354880522&reaction=dislike'>👎</a>
##########
tests/unit_tests/reports/notifications/webhook_tests.py:
##########
@@ -269,3 +271,156 @@ class MockResponse:
with pytest.raises(NotificationParamException, match="redirect"):
webhook_notification.send()
+
+
+def _make_webhook(mock_header_data) -> WebhookNotification:
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+ from superset.reports.notifications.base import NotificationContent
+
+ content = NotificationContent(
+ name="test alert", header_data=mock_header_data, description="Test
description"
+ )
+ return WebhookNotification(
+ recipient=ReportRecipients(
+ type=ReportRecipientType.WEBHOOK,
+ recipient_config_json='{"target": "https://example.com/webhook"}',
+ ),
+ content=content,
+ )
+
+
+class _MockServerErrorResponse:
+ status_code = 500
+ text = ""
+
+
+def _allow_internal_app() -> type:
+ class MockCurrentApp:
+ config = {
+ "ALERT_REPORTS_WEBHOOK_HTTPS_ONLY": True,
+ "ALERT_REPORTS_WEBHOOK_ALLOW_INTERNAL_HOSTS": True,
+ }
+
+ return MockCurrentApp
+
+
+def test_send_backoff_bounded_by_max_time(monkeypatch, mock_header_data) ->
None:
+ """
+ A persistently failing (500) target gives up on wall-time (``max_time``),
+ not just ``max_tries``. ``freeze_time(auto_tick_seconds=30)`` advances the
+ clock on every time access (backoff measures elapsed via ``datetime.now``,
+ which freezegun freezes and ticks), so cumulative elapsed crosses
``max_time=120``
+ before ``max_tries=5`` is exhausted. We assert the discriminating
*property*
+ — gave up on wall-time strictly before exhausting ``max_tries`` — rather
than
+ a pinned count, because freezegun's per-call tick count is an opaque
+ implementation detail. If ``max_time`` is removed this rises to the full 5
+ (RED); the flat-clock companion test below anchors that 5 is the ceiling.
+ The terminal exception type is unchanged on giveup.
+ """
+ webhook_notification = _make_webhook(mock_header_data)
+ post_calls: list[int] = []
+
+ def fake_post(*args, **kwargs) -> _MockServerErrorResponse:
+ post_calls.append(1)
+ return _MockServerErrorResponse()
+
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.current_app",
_allow_internal_app()
+ )
+ monkeypatch.setattr(
+
"superset.reports.notifications.webhook.feature_flag_manager.is_feature_enabled",
+ lambda flag: True,
+ )
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.requests.post", fake_post
+ )
+ monkeypatch.setattr("time.sleep", lambda *a, **k: None)
+
+ with freeze_time("2020-01-01", auto_tick_seconds=30):
+ with pytest.raises(NotificationUnprocessableException):
+ webhook_notification.send()
+
+ # 1 < count < max_tries(5): wall-time bound fired before tries exhausted.
+ assert 1 < len(post_calls) < 5
+
+
+def test_send_flat_clock_falls_back_to_max_tries(monkeypatch,
mock_header_data) -> None:
+ """
+ Characterization (NOT a RED discriminator): with the clock held flat,
+ ``max_time`` can never fire, so ``max_tries=5`` governs and exactly 5 POSTs
+ happen. Passes on both buggy and fixed code; its job is to prove the 3-vs-5
+ delta in ``test_send_backoff_bounded_by_max_time`` is attributable to
+ wall-time, not to ``max_tries``.
+ """
+ webhook_notification = _make_webhook(mock_header_data)
+ post_calls: list[int] = []
+
+ def fake_post(*args, **kwargs) -> _MockServerErrorResponse:
+ post_calls.append(1)
+ return _MockServerErrorResponse()
+
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.current_app",
_allow_internal_app()
+ )
+ monkeypatch.setattr(
+
"superset.reports.notifications.webhook.feature_flag_manager.is_feature_enabled",
+ lambda flag: True,
+ )
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.requests.post", fake_post
+ )
+ monkeypatch.setattr("time.sleep", lambda *a, **k: None)
+
+ with freeze_time("2020-01-01"):
+ with pytest.raises(NotificationUnprocessableException):
+ webhook_notification.send()
+
+ assert len(post_calls) == 5
+
+
+def test_send_max_time_does_not_abandon_recovering_target(
+ monkeypatch, mock_header_data
+) -> None:
Review Comment:
**Suggestion:** Add type annotations for `monkeypatch` and
`mock_header_data` in this new test to keep parameter typing complete.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This multiline test function is newly added and its parameters are not
type-annotated. The rule requires new functions to include parameter type
hints, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a6aacbbb362e4f3bb8d978a512d8b618&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a6aacbbb362e4f3bb8d978a512d8b618&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/reports/notifications/webhook_tests.py
**Line:** 381:383
**Comment:**
*Custom Rule: Add type annotations for `monkeypatch` and
`mock_header_data` in this new test to keep parameter typing complete.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=f057b754e0168f3ec8c6c8adf5e3aa0099059702a7eda02f2bb799a2ddf460fd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=f057b754e0168f3ec8c6c8adf5e3aa0099059702a7eda02f2bb799a2ddf460fd&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/report/execute_test.py:
##########
@@ -1171,6 +1269,122 @@ def
test_update_recipient_to_slack_v2_missing_channels(mocker: MockerFixture):
mock_cmmd.update_report_schedule_slack_v2()
+def test_update_recipient_to_slack_v2_reverts_all_on_partial_failure(
+ mocker: MockerFixture,
+) -> None:
+ """
+ When the second of two Slack recipients fails channel resolution, BOTH
+ recipients are fully reverted — type AND exact original
+ ``recipient_config_json`` string — not just the loop variable's type. This
+ prevents the intervening ``create_log`` commit from flushing a
half-migrated,
+ inconsistent state.
+ """
+
+ def channels_side_effect(search_string, types, exact_match):
+ if search_string == "Channel-1":
+ return [
+ {
+ "id": "id_channel_1",
+ "name": "Channel-1",
+ "is_member": True,
+ "is_private": False,
+ }
+ ]
+ # Second recipient: no channel found → length mismatch →
UpdateFailedError
+ return []
+
+ mocker.patch(
+ "superset.commands.report.execute.get_channels_with_search",
+ side_effect=channels_side_effect,
+ )
+ original_config_1 = json.dumps({"target": "Channel-1"})
+ original_config_2 = json.dumps({"target": "Channel-2"})
+ mock_report_schedule = ReportSchedule(
+ name="Test Report",
+ recipients=[
+ ReportRecipients(
+ type=ReportRecipientType.SLACK,
+ recipient_config_json=original_config_1,
+ ),
+ ReportRecipients(
+ type=ReportRecipientType.SLACK,
+ recipient_config_json=original_config_2,
+ ),
+ ],
+ )
+
+ mock_cmmd = BaseReportState(
+ mock_report_schedule, "January 1, 2021", "execution_id_example"
+ )
+
+ with pytest.raises(UpdateFailedError):
+ mock_cmmd.update_report_schedule_slack_v2()
+
+ first, second = mock_report_schedule.recipients
+ # The first recipient was mutated to v2 before the second failed; it must
be
+ # reverted to its exact original type AND config string.
+ assert first.type == ReportRecipientType.SLACK
+ assert first.recipient_config_json == original_config_1
+ assert second.type == ReportRecipientType.SLACK
+ assert second.recipient_config_json == original_config_2
+
+
+def test_update_recipient_to_slack_v2_pre_iteration_failure(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A failure raised while accessing/iterating the recipients (before the loop
+ variable is bound) surfaces as ``UpdateFailedError``, not a ``NameError``
+ that would mask the real error.
+ """
+
+ class _ExplodingRecipients:
+ def __iter__(self):
+ raise RuntimeError("recipients exploded")
Review Comment:
**Suggestion:** Add a return type annotation to this newly added method to
keep new code fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The newly added method lacks a return type annotation. Since the surrounding
code
is new, the rule requires full typing for new Python code, so this is a
genuine
violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c5eb955263c44a80a1f277551e383d82&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c5eb955263c44a80a1f277551e383d82&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/report/execute_test.py
**Line:** 1342:1343
**Comment:**
*Custom Rule: Add a return type annotation to this newly added method
to keep new code fully typed.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=9b9e0fe139b2a088ee533f13b0ce4c47430638e62e1ab217c95afdf1f09f00a3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41177&comment_hash=9b9e0fe139b2a088ee533f13b0ce4c47430638e62e1ab217c95afdf1f09f00a3&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]
