bito-code-review[bot] commented on code in PR #41424: URL: https://github.com/apache/superset/pull/41424#discussion_r3475129351
########## 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: <div> <div id="suggestion"> <div id="issue"><b>Documentation inaccurate for webhook</b></div> <div id="fix"> The `_get_files()` method in `webhook.py` only handles CSV, PDF, and PNG attachments (lines 81-94). Excel (.xlsx) is not included, despite the `xlsx` field existing in the base `NotificationContent` class. The change contradicts the actual implementation and may mislead users expecting Excel attachments to work with webhooks. </div> </div> <small><i>Code Review Run #6bd892</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## tests/unit_tests/reports/notifications/slack_tests.py: ########## @@ -145,6 +145,33 @@ def test_get_inline_files_with_screenshots(mock_header_data) -> None: assert result == ("png", [b"screenshot1", b"screenshot2"]) + +def test_get_inline_files_with_xlsx(mock_header_data) -> None: + """ + Test the _get_inline_files function to ensure it returns the correct tuple + when content has an Excel attachment + """ + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + from superset.reports.notifications.slack import SlackNotification + + content = NotificationContent( + name="test alert", + header_data=mock_header_data, + xlsx=b"xlsx_content", + ) + slack_notification = SlackNotification( + recipient=ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json='{"target": "some_channel"}', + ), + content=content, + ) + + result = slack_notification._get_inline_files() + + assert result == ("xlsx", [b"xlsx_content"]) + Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete test coverage for CSV</b></div> <div id="fix"> The new xlsx test covers one of four attachment paths in `_get_inline_files()`. Per rule 11730 (comprehensive unit tests for new tools), add a corresponding `test_get_inline_files_with_csv` that mirrors the xlsx test structure to achieve complete coverage of the method's attachment-type branches. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion assert result == ("xlsx", [b"xlsx_content"]) def test_get_inline_files_with_csv(mock_header_data) -> None: """ Test the _get_inline_files function to ensure it returns the correct tuple when content has a CSV attachment """ from superset.reports.models import ReportRecipients, ReportRecipientType from superset.reports.notifications.base import NotificationContent from superset.reports.notifications.slack import SlackNotification content = NotificationContent( name="test alert", header_data=mock_header_data, csv=b"csv_content", ) slack_notification = SlackNotification( recipient=ReportRecipients( type=ReportRecipientType.SLACK, recipient_config_json='{"target": "some_channel"}', ), content=content, ) result = slack_notification._get_inline_files() assert result == ("csv", [b"csv_content"]) ```` </div> </details> </div> <small><i>Code Review Run #6bd892</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
