codeant-ai-for-open-source[bot] commented on code in PR #36127:
URL: https://github.com/apache/superset/pull/36127#discussion_r2587172410
##########
superset-frontend/src/features/alerts/components/NotificationMethod.tsx:
##########
@@ -338,6 +338,8 @@ export const NotificationMethod:
FunctionComponent<NotificationMethodProps> = ({
((!isFeatureEnabled(FeatureFlag.AlertReportSlackV2) ||
useSlackV1) &&
method === NotificationMethodOption.Slack) ||
+ (isFeatureEnabled(FeatureFlag.AlertReportWebhook) &&
+ method === NotificationMethodOption.Webhook) ||
method === NotificationMethodOption.Email,
)
Review Comment:
**Suggestion:** The new filter in `methodOptions` reads feature flags inline
(via `isFeatureEnabled`) but the useMemo dependency array does not include
those derived flag values; if feature flags change at runtime `methodOptions`
may become stale and not update. Evaluate the feature flags into local
variables and include them in the useMemo dependency array so the memo
recomputes when flags change. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const slackV2FeatureEnabled =
isFeatureEnabled(FeatureFlag.AlertReportSlackV2);
const webhookFeatureEnabled =
isFeatureEnabled(FeatureFlag.AlertReportWebhook);
const methodOptions = useMemo(
() =>
(options || [])
.filter(
method =>
(slackV2FeatureEnabled && !useSlackV1 && method ===
NotificationMethodOption.SlackV2) ||
((!slackV2FeatureEnabled || useSlackV1) && method ===
NotificationMethodOption.Slack) ||
(webhookFeatureEnabled && method ===
NotificationMethodOption.Webhook) ||
method === NotificationMethodOption.Email,
)
.map(method => ({
label:
method === NotificationMethodOption.SlackV2
? NotificationMethodOption.Slack
: method,
value: method,
})),
[options, useSlackV1, slackV2FeatureEnabled,
webhookFeatureEnabled],
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This suggestion is valid: the code calls isFeatureEnabled inside the
memoized callback but doesn't include the resulting booleans in the dependency
array, so if feature flags can change at runtime the memo won't recompute.
Evaluating the flags into local booleans and adding them to deps guarantees
methodOptions updates correctly. It's not urgent if flags are static, but the
change is correct and low-risk.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/features/alerts/components/NotificationMethod.tsx
**Line:** 338:344
**Comment:**
*Logic Error: The new filter in `methodOptions` reads feature flags
inline (via `isFeatureEnabled`) but the useMemo dependency array does not
include those derived flag values; if feature flags change at runtime
`methodOptions` may become stale and not update. Evaluate the feature flags
into local variables and include them in the useMemo dependency array so the
memo recomputes when flags change.
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>
##########
superset-frontend/src/features/alerts/components/RecipientIcon.tsx:
##########
@@ -52,6 +52,12 @@ export default function RecipientIcon({ type }: { type:
string }) {
);
recipientIconConfig.label = NotificationMethodOption.Slack;
break;
+ case NotificationMethodOption.Webhook:
+ recipientIconConfig.icon = (
+ <Icons.ApiOutlined css={notificationStyledIcon} iconSize="l" />
Review Comment:
**Suggestion:** Accessibility issue: the icon added for Webhook has no
accessible name/role; add an accessible label/role on the icon (or ensure
Tooltip provides the accessible name) so screen readers can announce the
control. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
<Icons.ApiOutlined
css={notificationStyledIcon}
iconSize="l"
role="img"
aria-label="Webhook"
/>
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Improving accessibility is valid here — the Tooltip may visually label the
icon but adding an explicit accessible name (aria-label) or role to the icon
ensures screen readers announce it reliably. The suggested change is practical
and fixes a real accessibility shortcoming.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/components/RecipientIcon.tsx
**Line:** 57:57
**Comment:**
*Possible Bug: Accessibility issue: the icon added for Webhook has no
accessible name/role; add an accessible label/role on the icon (or ensure
Tooltip provides the accessible name) so screen readers can announce the
control.
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>
##########
superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts:
##########
@@ -26,6 +26,7 @@ export enum FeatureFlag {
AlertReports = 'ALERT_REPORTS',
AlertReportTabs = 'ALERT_REPORT_TABS',
AlertReportSlackV2 = 'ALERT_REPORT_SLACK_V2',
Review Comment:
**Suggestion:** The enum list is required to be sorted alphabetically (per
the file comment) but the new entry was inserted without reordering the
surrounding lines, leaving the block out of alphabetical order; reorder the
nearby enum members so the whole list remains alphabetically sorted to obey the
file contract and avoid churn/confusion when future flags are inserted. [logic
error]
**Severity Level:** Minor ⚠️
```suggestion
AlertReportSlackV2 = 'ALERT_REPORT_SLACK_V2',
AlertReportTabs = 'ALERT_REPORT_TABS',
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The enum comment explicitly requests alphabetical ordering. The new member
was inserted without reordering, which breaks that contract and makes future
diffs noisier. The proposed reorder (placing Slack before Tabs) is a correct,
safe, non-breaking cosmetic fix that keeps the list consistent.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts
**Line:** 27:28
**Comment:**
*Logic Error: The enum list is required to be sorted alphabetically
(per the file comment) but the new entry was inserted without reordering the
surrounding lines, leaving the block out of alphabetical order; reorder the
nearby enum members so the whole list remains alphabetically sorted to obey the
file contract and avoid churn/confusion when future flags are inserted.
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>
##########
superset-frontend/src/features/reports/types.ts:
##########
@@ -23,7 +23,7 @@
export type ReportScheduleType = 'Alert' | 'Report';
export type ReportCreationMethod = 'charts' | 'dashboards' | 'alerts_reports';
-export type ReportRecipientType = 'Email' | 'Slack';
+export type ReportRecipientType = 'Email' | 'Slack' | 'Webhook';
Review Comment:
**Suggestion:** Adding 'Webhook' to `ReportRecipientType` without updating
the `recipient_config_json` shape makes the exported type inaccurate and will
allow a recipient type that the rest of the `ReportObject` shape does not
describe; revert the addition until the recipients' config shape is extended,
or update the recipient type usage. The minimal safe fix is to remove 'Webhook'
here (restore previous two allowed literals) so TypeScript types remain
consistent with the existing `recipient_config_json`. [type error]
**Severity Level:** Minor ⚠️
```suggestion
export type ReportRecipientType = 'Email' | 'Slack';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The PR introduced 'Webhook' while the recipients' shape
(recipient_config_json) still requires ccTarget and bccTarget as non-optional
strings. That makes the union inaccurate for some consumers and could permit
types that don't match the existing config shape. Removing 'Webhook' is a
minimal, safe fix that restores consistency until the config shape is expanded.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/reports/types.ts
**Line:** 26:26
**Comment:**
*Type Error: Adding 'Webhook' to `ReportRecipientType` without updating
the `recipient_config_json` shape makes the exported type inaccurate and will
allow a recipient type that the rest of the `ReportObject` shape does not
describe; revert the addition until the recipients' config shape is extended,
or update the recipient type usage. The minimal safe fix is to remove 'Webhook'
here (restore previous two allowed literals) so TypeScript types remain
consistent with the existing `recipient_config_json`.
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>
##########
superset/config.py:
##########
@@ -538,6 +538,7 @@ class D3TimeFormat(TypedDict, total=False):
"ALERT_REPORT_TABS": False,
"ALERT_REPORTS_FILTER": False,
"ALERT_REPORT_SLACK_V2": False,
+ "ALERT_REPORT_WEBHOOK": False,
Review Comment:
**Suggestion:** Typo / naming inconsistency in the feature flag key: the new
key is named `ALERT_REPORT_WEBHOOK` (singular "REPORT") while the rest of the
alert/report feature flags and related settings use the "ALERT_REPORTS_*"
(plural "REPORTS") convention; this mismatch will likely cause the feature
toggle to be ignored where code expects `ALERT_REPORTS_WEBHOOK`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
"ALERT_REPORTS_WEBHOOK": False,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The new key is singular ("ALERT_REPORT_WEBHOOK") while the rest of the
alert/report flags follow the "ALERT_REPORTS_*" plural convention. That
mismatch is likely a real bug: callers checking ALERT_REPORTS_WEBHOOK will not
find this flag. Renaming to ALERT_REPORTS_WEBHOOK fixes a probable logic bug
and keeps naming consistent.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/config.py
**Line:** 541:541
**Comment:**
*Possible Bug: Typo / naming inconsistency in the feature flag key: the
new key is named `ALERT_REPORT_WEBHOOK` (singular "REPORT") while the rest of
the alert/report feature flags and related settings use the "ALERT_REPORTS_*"
(plural "REPORTS") convention; this mismatch will likely cause the feature
toggle to be ignored where code expects `ALERT_REPORTS_WEBHOOK`.
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>
##########
superset/reports/notifications/webhook.py:
##########
@@ -0,0 +1,140 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+from typing import Any
+from urllib.parse import urlparse
+
+import backoff
+import requests
+from flask import current_app
+
+from superset import feature_flag_manager
+from superset.reports.models import ReportRecipientType
+from superset.reports.notifications.base import BaseNotification
+from superset.reports.notifications.exceptions import (
+ NotificationParamException,
+ NotificationUnprocessableException,
+)
+from superset.utils import json
+from superset.utils.decorators import statsd_gauge
+
+logger = logging.getLogger(__name__)
+
+
+class WebhookNotification(BaseNotification):
+ """
+ Sends a post request to a webhook url
+ """
+
+ type = ReportRecipientType.WEBHOOK
+
+ def _get_webhook_url(self) -> str:
+ """
+ Get the webhook URL from the recipient configuration
+ :returns: The webhook URL
+ :raises NotificationParamException: If the webhook URL is not provided
in the recipient configuration
+ """ # noqa: E501
+ try:
+ return json.loads(self._recipient.recipient_config_json)["target"]
+ except (json.JSONDecodeError, KeyError) as ex:
Review Comment:
**Suggestion:**
`json.loads(self._recipient.recipient_config_json)["target"]` can raise
TypeError if `recipient_config_json` is None/non-string or return a non-dict
value lacking `target`; also an empty or missing `target` should be treated as
an invalid parameter. Parse into a variable, validate that it's a dict and
contains a non-empty `target`, and catch `TypeError` as well as JSON errors.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
cfg = json.loads(self._recipient.recipient_config_json)
target = cfg.get("target") if isinstance(cfg, dict) else None
if not target:
raise NotificationParamException("Webhook URL is required")
return target
except (json.JSONDecodeError, KeyError, TypeError) as ex:
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing code doesn't handle cases where recipient_config_json is
None/non-string or when json.loads returns a non-dict; those can raise
TypeError or leave you without a usable "target". The suggested change to
validate the parsed value, ensure it's a dict, check for a non-empty target,
and include TypeError in the except clause fixes real failure modes and
produces a clearer, consistent NotificationParamException for malformed or
missing config.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/notifications/webhook.py
**Line:** 53:54
**Comment:**
*Possible Bug:
`json.loads(self._recipient.recipient_config_json)["target"]` can raise
TypeError if `recipient_config_json` is None/non-string or return a non-dict
value lacking `target`; also an empty or missing `target` should be treated as
an invalid parameter. Parse into a variable, validate that it's a dict and
contains a non-empty `target`, and catch `TypeError` as well as JSON errors.
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>
##########
superset-frontend/src/features/alerts/types.ts:
##########
@@ -173,6 +174,7 @@ export enum RecipientIconName {
Email = 'Email',
Slack = 'Slack',
SlackV2 = 'SlackV2',
+ Webhook = 'Webhook',
Review Comment:
**Suggestion:** Duplication risk: `RecipientIconName` reproduces the same
literal strings as `NotificationMethodOption`; if the value for `Webhook` ever
changes in one enum they can diverge and cause icon lookup or UI logic to
break. Make `RecipientIconName.Webhook` reference the source-of-truth
`NotificationMethodOption.Webhook` to prevent drift. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
Webhook = NotificationMethodOption.Webhook,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Reducing duplication by referencing NotificationMethodOption.Webhook
prevents drift between two enums and is a valid, minimal change
(NotificationMethodOption is declared earlier in the same file). This directly
addresses the risk mentioned and is implementable in TS as a constant
expression.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/types.ts
**Line:** 177:177
**Comment:**
*Logic Error: Duplication risk: `RecipientIconName` reproduces the same
literal strings as `NotificationMethodOption`; if the value for `Webhook` ever
changes in one enum they can diverge and cause icon lookup or UI logic to
break. Make `RecipientIconName.Webhook` reference the source-of-truth
`NotificationMethodOption.Webhook` to prevent drift.
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>
##########
superset/views/base.py:
##########
@@ -460,10 +460,12 @@ def cached_common_bootstrap_data( # pylint:
disable=unused-argument
ReportRecipientType.EMAIL,
ReportRecipientType.SLACK,
ReportRecipientType.SLACKV2,
+ ReportRecipientType.WEBHOOK,
]
else:
frontend_config["ALERT_REPORTS_NOTIFICATION_METHODS"] = [
ReportRecipientType.EMAIL,
+ ReportRecipientType.WEBHOOK,
Review Comment:
**Suggestion:** Type/serialization bug: Enum members (ReportRecipientType.*)
may not be JSON-serializable for the frontend payload; convert enum members to
primitive values (e.g., .value or .name) when building the list so json.dumps
produces a stable, serializable structure. [type error]
**Severity Level:** Minor ⚠️
```suggestion
methods = [
ReportRecipientType.EMAIL,
ReportRecipientType.SLACK,
ReportRecipientType.SLACKV2,
ReportRecipientType.WEBHOOK,
]
else:
methods = [
ReportRecipientType.EMAIL,
ReportRecipientType.WEBHOOK,
]
frontend_config["ALERT_REPORTS_NOTIFICATION_METHODS"] = [
m.value if hasattr(m, "value") else m for m in
list(dict.fromkeys(methods))
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Leaving Enum members in the payload can cause json.dumps to fail or produce
unexpected objects on the frontend; converting to a primitive (e.g., .value or
.name) before inserting into frontend_config is a sensible, low-risk fix that
prevents serialization/runtime issues.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 461:468
**Comment:**
*Type Error: Type/serialization bug: Enum members
(ReportRecipientType.*) may not be JSON-serializable for the frontend payload;
convert enum members to primitive values (e.g., .value or .name) when building
the list so json.dumps produces a stable, serializable structure.
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>
##########
superset/reports/notifications/webhook.py:
##########
@@ -0,0 +1,140 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+from typing import Any
+from urllib.parse import urlparse
+
+import backoff
+import requests
+from flask import current_app
+
+from superset import feature_flag_manager
+from superset.reports.models import ReportRecipientType
+from superset.reports.notifications.base import BaseNotification
+from superset.reports.notifications.exceptions import (
+ NotificationParamException,
+ NotificationUnprocessableException,
+)
+from superset.utils import json
+from superset.utils.decorators import statsd_gauge
+
+logger = logging.getLogger(__name__)
+
+
+class WebhookNotification(BaseNotification):
+ """
+ Sends a post request to a webhook url
+ """
+
+ type = ReportRecipientType.WEBHOOK
+
+ def _get_webhook_url(self) -> str:
+ """
+ Get the webhook URL from the recipient configuration
+ :returns: The webhook URL
+ :raises NotificationParamException: If the webhook URL is not provided
in the recipient configuration
+ """ # noqa: E501
+ try:
+ return json.loads(self._recipient.recipient_config_json)["target"]
+ except (json.JSONDecodeError, KeyError) as ex:
+ raise NotificationParamException("Webhook URL is required") from ex
+
+ def _get_req_payload(self) -> dict[str, Any]:
+ header_content = {
+ "notification_format":
self._content.header_data.get("notification_format"),
+ "notification_type":
self._content.header_data.get("notification_type"),
+ "notification_source":
self._content.header_data.get("notification_source"),
+ "chart_id": self._content.header_data.get("chart_id"),
+ "dashboard_id": self._content.header_data.get("dashboard_id"),
+ }
+ content = {
+ "name": self._content.name,
+ "header": header_content,
+ "text": self._content.text,
+ "description": self._content.description,
+ "url": self._content.url,
+ }
+ return content
+
+ def _get_files(self) -> list[tuple[str, tuple[str, bytes, str]]]:
+ files = []
+ if self._content.csv:
+ files.append(("files", ("report.csv", self._content.csv,
"text/csv")))
+ if self._content.pdf:
+ files.append(
+ ("files", ("report.pdf", self._content.pdf, "application/pdf"))
+ )
+ if self._content.screenshots:
+ for i, screenshot in enumerate(self._content.screenshots):
+ files.append(
+ (
+ "files",
+ (f"screenshot_{i}.png", screenshot, "image/png"),
+ )
+ )
+ return files
+
+ @backoff.on_exception(
+ backoff.expo, NotificationUnprocessableException, factor=10, base=2,
max_tries=5
+ )
+ @statsd_gauge("reports.webhook.send")
+ def send(self) -> None:
+ if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_WEBHOOK"):
+ raise NotificationUnprocessableException(
+ "Attempted to send a Webhook notification but Webhook feature
flag \
+ is not enabled."
+ )
+ wh_url = self._get_webhook_url()
+ if current_app.config["ALERT_REPORTS_WEBHOOK_HTTPS_ONLY"]:
Review Comment:
**Suggestion:** Accessing
`current_app.config["ALERT_REPORTS_WEBHOOK_HTTPS_ONLY"]` will raise a KeyError
if the configuration key is absent; use `current_app.config.get(..., False)` to
safely default to False when the setting is missing. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
if current_app.config.get("ALERT_REPORTS_WEBHOOK_HTTPS_ONLY", False):
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Accessing current_app.config[...] can raise KeyError if the key isn't
present. Switching to current_app.config.get(..., False) is a defensive,
low-risk improvement that prevents unexpected KeyError in environments where
that config isn't set. It's a safe change that doesn't alter behavior when the
key exists and True/False semantics are preserved.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/notifications/webhook.py
**Line:** 103:103
**Comment:**
*Possible Bug: Accessing
`current_app.config["ALERT_REPORTS_WEBHOOK_HTTPS_ONLY"]` will raise a KeyError
if the configuration key is absent; use `current_app.config.get(..., False)` to
safely default to False when the setting is missing.
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>
##########
superset/reports/notifications/webhook.py:
##########
@@ -0,0 +1,140 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+from typing import Any
+from urllib.parse import urlparse
+
+import backoff
+import requests
+from flask import current_app
+
+from superset import feature_flag_manager
+from superset.reports.models import ReportRecipientType
+from superset.reports.notifications.base import BaseNotification
+from superset.reports.notifications.exceptions import (
+ NotificationParamException,
+ NotificationUnprocessableException,
+)
+from superset.utils import json
+from superset.utils.decorators import statsd_gauge
+
+logger = logging.getLogger(__name__)
+
+
+class WebhookNotification(BaseNotification):
+ """
+ Sends a post request to a webhook url
+ """
+
+ type = ReportRecipientType.WEBHOOK
+
+ def _get_webhook_url(self) -> str:
+ """
+ Get the webhook URL from the recipient configuration
+ :returns: The webhook URL
+ :raises NotificationParamException: If the webhook URL is not provided
in the recipient configuration
+ """ # noqa: E501
+ try:
+ return json.loads(self._recipient.recipient_config_json)["target"]
+ except (json.JSONDecodeError, KeyError) as ex:
+ raise NotificationParamException("Webhook URL is required") from ex
+
+ def _get_req_payload(self) -> dict[str, Any]:
+ header_content = {
+ "notification_format":
self._content.header_data.get("notification_format"),
+ "notification_type":
self._content.header_data.get("notification_type"),
+ "notification_source":
self._content.header_data.get("notification_source"),
+ "chart_id": self._content.header_data.get("chart_id"),
+ "dashboard_id": self._content.header_data.get("dashboard_id"),
+ }
+ content = {
+ "name": self._content.name,
+ "header": header_content,
+ "text": self._content.text,
+ "description": self._content.description,
+ "url": self._content.url,
+ }
+ return content
+
+ def _get_files(self) -> list[tuple[str, tuple[str, bytes, str]]]:
+ files = []
+ if self._content.csv:
+ files.append(("files", ("report.csv", self._content.csv,
"text/csv")))
+ if self._content.pdf:
+ files.append(
+ ("files", ("report.pdf", self._content.pdf, "application/pdf"))
+ )
+ if self._content.screenshots:
+ for i, screenshot in enumerate(self._content.screenshots):
+ files.append(
+ (
+ "files",
+ (f"screenshot_{i}.png", screenshot, "image/png"),
+ )
+ )
+ return files
+
+ @backoff.on_exception(
+ backoff.expo, NotificationUnprocessableException, factor=10, base=2,
max_tries=5
+ )
+ @statsd_gauge("reports.webhook.send")
Review Comment:
**Suggestion:** The `@statsd_gauge` decorator (imported from
utils.decorators) relies on a global `app` variable and will raise a NameError
at runtime when invoked; remove the decorator usage here to avoid propagating
that error (or replace with a safe metrics call that uses `current_app`). This
change keeps the backoff retry behavior while avoiding an external decorator
bug causing the method to fail immediately. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion correctly points out a real problem: the provided
statsd_gauge implementation (seen in the repo) references `app` which is not
defined, so invoking the decorator wrapper will raise a NameError at runtime
when the wrapped function executes. Removing (or replacing) the decorator here
prevents the notification send path from failing due to an unrelated metrics
bug. It's a pragmatic, safety-first change. That said, the long-term fix is to
correct the decorator implementation so it uses `current_app` or otherwise
acquires the app; simply removing the decorator is acceptable as a mitigation
but not ideal as a permanent solution.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/notifications/webhook.py
**Line:** 95:95
**Comment:**
*Possible Bug: The `@statsd_gauge` decorator (imported from
utils.decorators) relies on a global `app` variable and will raise a NameError
at runtime when invoked; remove the decorator usage here to avoid propagating
that error (or replace with a safe metrics call that uses `current_app`). This
change keeps the backoff retry behavior while avoiding an external decorator
bug causing the method to fail immediately.
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>
--
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]
