codeant-ai-for-open-source[bot] commented on code in PR #41424: URL: https://github.com/apache/superset/pull/41424#discussion_r3475084520
########## docs/admin_docs/configuration/alerts-reports.mdx: ########## @@ -120,7 +120,7 @@ Superset sends an HTTP POST with `Content-Type: application/json`: } ``` -When a report includes file attachments (CSV, PDF, or PNG screenshots), the request is sent as `multipart/form-data` instead. In that case, each top-level payload field (`name`, `text`, `description`, `url`) becomes its own form field, and nested structures like `header` are serialized as a JSON-encoded string in their own field. Every attachment is added as a repeated form field named `files`: +When a report includes file attachments (CSV, Excel, PDF, or PNG screenshots), the request is sent as `multipart/form-data` instead. In that case, each top-level payload field (`name`, `text`, `description`, `url`) becomes its own form field, and nested structures like `header` are serialized as a JSON-encoded string in their own field. Every attachment is added as a repeated form field named `files`: Review Comment: **Suggestion:** The new docs state that webhook multipart attachments include Excel files, but the current webhook implementation only attaches CSV/PDF/PNG files and never adds `xlsx`. This creates a contract mismatch where users will expect `.xlsx` files in webhook deliveries and silently not receive them. Either implement `.xlsx` handling in the webhook attachment builder or remove Excel from this multipart webhook section until backend support exists. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Webhook XLSX report attachments are never delivered. - ⚠️ Webhook payload remains JSON instead of documented multipart. - ⚠️ Webhook consumers cannot rely on Excel attachment presence. - ⚠️ Documentation overstates webhook support for Excel files. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Consult the new webhook documentation at `docs/admin_docs/configuration/alerts-reports.mdx:123`, which states that when a report includes file attachments "(CSV, Excel, PDF, or PNG screenshots)", the webhook request is sent as `multipart/form-data` and each attachment is added as a repeated `files` form field. 2. Follow the report execution path: `superset/commands/report/execute.py` defines `BaseReportState._get_notification_content()` (around lines 700–766). For a chart-based report with `report_format == ReportDataFormat.XLSX` (see `superset/reports/models.py:83-93` for the enum and `tabular()` helper), it calls `_get_data(ChartDataResultFormat.XLSX)` and assigns the bytes to `xlsx_data`, then builds `NotificationContent(..., csv=csv_data, xlsx=xlsx_data, ...)` at `execute.py:757-765`. 3. Observe that `NotificationContent` explicitly supports an Excel payload: in `superset/reports/notifications/base.py:27-33`, the dataclass includes `xlsx: Optional[bytes] = None # bytes for Excel file` alongside `csv`, `pdf`, and `screenshots`, so XLSX bytes are available on `self._content.xlsx` for all notification types, including webhooks. 4. Inspect the webhook implementation in `superset/reports/notifications/webhook.py`. `_get_files()` (around lines 79–95; snippet header "Showing lines 60 to 139", local lines 20–36) only appends attachments for `self._content.csv`, `self._content.pdf`, and `self._content.screenshots`, never checking `self._content.xlsx`. In `send()` (same file, lines ~142–160; snippet "Showing lines 135 to 214", local lines 8–30), the code calls `files = self._get_files()` and sends `requests.post(..., data=data, files=files, ...)` only when `files` is non-empty; otherwise it issues `requests.post(wh_url, json=payload, ...)` with no attachments. 5. Create a report schedule with recipient type `ReportRecipientType.WEBHOOK` and format `ReportDataFormat.XLSX` using the existing models in `superset/reports/models.py` and the execution flow in `superset/commands/report/execute.py`. When it runs, `NotificationContent.xlsx` is populated, but `_get_files()` returns an empty list because it ignores `xlsx`. Consequently, `WebhookNotification.send()` sends an `application/json` payload without any `files` parts, so the webhook consumer never receives an `.xlsx` attachment—contradicting the docs' promise that Excel attachments are included in multipart webhook deliveries. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7773f56a462b4e72a58bfb79ddf3c894&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=7773f56a462b4e72a58bfb79ddf3c894&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:** docs/admin_docs/configuration/alerts-reports.mdx **Line:** 123:123 **Comment:** *Incomplete Implementation: The new docs state that webhook multipart attachments include Excel files, but the current webhook implementation only attaches CSV/PDF/PNG files and never adds `xlsx`. This creates a contract mismatch where users will expect `.xlsx` files in webhook deliveries and silently not receive them. Either implement `.xlsx` handling in the webhook attachment builder or remove Excel from this multipart webhook section until backend support exists. 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%2F41424&comment_hash=744ed69d2cd41eec595f9d18e0811f62230d7e48d7fa14cae4326fb6dc3b5563&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=744ed69d2cd41eec595f9d18e0811f62230d7e48d7fa14cae4326fb6dc3b5563&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]
