feat(reports): add XLSX (Excel) attachments for Alerts & Reports#41424
feat(reports): add XLSX (Excel) attachments for Alerts & Reports#41424gr33nak wants to merge 1 commit into
Conversation
dosubot
Bot
added
alert-reports
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The suggestion to add a type annotation for the Since the file Regarding other comments on this PR, there are no additional review comments available in the provided context to evaluate. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #6bd892
Actionable Suggestions - 2
-
docs/admin_docs/configuration/alerts-reports.mdx - 1
- Documentation inaccurate for webhook · Line 123-123
-
tests/unit_tests/reports/notifications/slack_tests.py - 1
- Incomplete test coverage for CSV · Line 173-174
Additional Suggestions - 3
-
superset/reports/notifications/slackv2.py - 1
-
Missing test coverage for xlsx · Line 74-75The xlsx handling is correctly implemented, but there's no dedicated unit test for `SlackV2Notification._get_inline_files()` with xlsx content. The existing `test_get_inline_files_with_xlsx` tests `SlackNotification` (slack.py), not `SlackV2Notification`. Add a test covering this code path to prevent regressions per rule [11730].
-
-
superset/reports/notifications/email.py - 1
-
Missing test for CSV path · Line 208-214The XLSX attachment path (added in this change) has test coverage via `test_email_content_with_xlsx_attachment`, but the corresponding CSV attachment path lacks a dedicated test. Consider adding a test that validates `NotificationContent(csv=b'csv_content', ...)` produces the correct `email_content.data` attachment.
-
-
superset-frontend/src/features/alerts/AlertReportModal.tsx - 1
-
Consistency: mixed case in format arrays · Line 2471-2471The `['PDF', 'TEXT', 'CSV', 'XLSX']` array at line 2471 uses uppercase values while the `['pdf', 'png', 'csv', 'xlsx']` array at line 2497 uses lowercase keys. This inconsistency increases cognitive load and creates divergence risk if code is copy-pasted between contexts. Consider standardizing on lowercase keys (matching the `FORMAT_OPTIONS_KEY` type) unless uppercase is semantically required.
-
Review Details
-
Files reviewed - 17 · Commit Range:
9082196..9082196- docs/admin_docs/configuration/alerts-reports.mdx
- superset-frontend/src/features/alerts/AlertReportModal.test.tsx
- superset-frontend/src/features/alerts/AlertReportModal.tsx
- superset-frontend/src/features/reports/ReportModal/index.tsx
- superset-frontend/src/features/reports/types.ts
- superset/commands/report/exceptions.py
- superset/commands/report/execute.py
- superset/reports/models.py
- superset/reports/notifications/base.py
- superset/reports/notifications/email.py
- superset/reports/notifications/slack.py
- superset/reports/notifications/slackv2.py
- tests/integration_tests/reports/commands_tests.py
- tests/integration_tests/reports/utils.py
- tests/unit_tests/commands/report/execute_test.py
- tests/unit_tests/reports/notifications/email_tests.py
- tests/unit_tests/reports/notifications/slack_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Lots of bot review comments. Can we start by assessing/addressing those, and resolving the merge conflict(s)?
gr33nak
force-pushed
the
feat/xlsx-report-attachments
branch
from
9082196 to
f1c5ee7
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41424 +/- ##
==========================================
- Coverage 64.47% 64.43% -0.04%
==========================================
Files 2661 2662 +1
Lines 145673 145854 +181
Branches 33620 33652 +32
==========================================
+ Hits 93916 93988 +72
- Misses 50053 50160 +107
- Partials 1704 1706 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Adds "Send as Excel" as an attachment format for chart-based alerts and reports, alongside the existing CSV/PNG/PDF/text options. XLSX bytes are fetched from the chart data export endpoint (result_format=XLSX), reusing the same post-processing, index handling, and formula-injection-safe df_to_excel path as a chart's manual Excel export. Finalizes the long-stalled PR apache#19810. Resolves the spurious-index-column issue that blocked the original by fetching pre-rendered XLSX from the export endpoint instead of re-parsing CSV into a DataFrame locally. - models: add ReportDataFormat.XLSX + tabular() helper - execute: unify CSV/XLSX fetching into _get_data(result_format); surface format-specific errors (XLSX vs CSV) from the query-context fallback - notifications: add NotificationContent.xlsx; attach in email, Slack (v1/v2), and webhook - frontend: add "Send as Excel" option to AlertReportModal and ReportModal - docs: list Excel among report attachment formats - tests: unit + integration coverage for the XLSX path across all notification types Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gr33nak
force-pushed
the
feat/xlsx-report-attachments
branch
from
f1c5ee7 to
3bd6f2b
Compare
|
Thanks for taking a look! Just pushed an update: rebased onto master (conflicts resolved), and worked through the bot comments — the two real ones (webhook xlsx attachment + XLSX error misclassification) are fixed with tests; the rest were typing/docstring nits, now addressed. |
There was a problem hiding this comment.
Code Review Agent Run #daaf18
Actionable Suggestions - 2
-
superset/commands/report/execute.py - 1
- Missing XLSX timeout test coverage · Line 516-588
-
tests/unit_tests/commands/report/execute_test.py - 1
- Deprecated datetime.utcnow usage · Line 1199-1199
Additional Suggestions - 1
-
superset/commands/report/execute.py - 1
-
Hardcoded CSV config for XLSX timeout · Line 557-557The refactored `_get_data` (line 516) handles both CSV and XLSX but still reads only `ALERT_REPORTS_CSV_REQUEST_TIMEOUT` (line 557). If XLSX generation proves to be significantly slower than CSV in production, operators cannot adjust its timeout independently. The hardcoded CSV name in the config key is also a maintenance risk if the set of tabular formats grows.
-
Review Details
-
Files reviewed - 19 · Commit Range:
3bd6f2b..3bd6f2b- docs/admin_docs/configuration/alerts-reports.mdx
- superset-frontend/src/features/alerts/AlertReportModal.test.tsx
- superset-frontend/src/features/alerts/AlertReportModal.tsx
- superset-frontend/src/features/reports/ReportModal/index.tsx
- superset-frontend/src/features/reports/types.ts
- superset/commands/report/exceptions.py
- superset/commands/report/execute.py
- superset/reports/models.py
- superset/reports/notifications/base.py
- superset/reports/notifications/email.py
- superset/reports/notifications/slack.py
- superset/reports/notifications/slackv2.py
- superset/reports/notifications/webhook.py
- tests/integration_tests/reports/commands_tests.py
- tests/integration_tests/reports/utils.py
- tests/unit_tests/commands/report/execute_test.py
- tests/unit_tests/reports/notifications/email_tests.py
- tests/unit_tests/reports/notifications/slack_tests.py
- tests/unit_tests/reports/notifications/webhook_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| @@ -522,17 +548,18 @@ def _get_csv_data(self) -> bytes: | |||
|
|
|||
| if self._report_schedule.chart.query_context is None: | |||
| logger.warning("No query context found, taking a screenshot to generate it") | |||
| self._update_query_context() | |||
| self._update_query_context(failed_error) | |||
|
|
|||
| try: | |||
| csv_data = get_chart_csv_data( | |||
| data = get_chart_csv_data( | |||
| chart_url=url, | |||
| auth_cookies=auth_cookies, | |||
| timeout=app.config["ALERT_REPORTS_CSV_REQUEST_TIMEOUT"], | |||
| ) | |||
| elapsed_seconds = (datetime.utcnow() - start_time).total_seconds() | |||
| logger.info( | |||
| "CSV data generation from %s as user %s took %.2fs - execution_id: %s", | |||
| "%s data generation from %s as user %s took %.2fs - execution_id: %s", | |||
| label, | |||
| url, | |||
| username, | |||
| elapsed_seconds, | |||
| @@ -541,24 +568,24 @@ def _get_csv_data(self) -> bytes: | |||
| except SoftTimeLimitExceeded as ex: | |||
| elapsed_seconds = (datetime.utcnow() - start_time).total_seconds() | |||
| logger.warning( | |||
| "CSV generation timeout after %.2fs - execution_id: %s", | |||
| "%s generation timeout after %.2fs - execution_id: %s", | |||
| label, | |||
| elapsed_seconds, | |||
| self._execution_id, | |||
| ) | |||
| raise ReportScheduleCsvTimeout() from ex | |||
| raise timeout_error() from ex | |||
| except Exception as ex: | |||
| elapsed_seconds = (datetime.utcnow() - start_time).total_seconds() | |||
| logger.exception( | |||
| "CSV generation failed after %.2fs - execution_id: %s", | |||
| "%s generation failed after %.2fs - execution_id: %s", | |||
| label, | |||
| elapsed_seconds, | |||
| self._execution_id, | |||
| ) | |||
| raise ReportScheduleCsvFailedError( | |||
| f"Failed generating csv {str(ex)}" | |||
| ) from ex | |||
| if not csv_data: | |||
| raise ReportScheduleCsvFailedError() | |||
| return csv_data | |||
| raise failed_error(f"Failed generating {label.lower()} {str(ex)}") from ex | |||
| if not data: | |||
| raise failed_error() | |||
| return data | |||
There was a problem hiding this comment.
The _get_data method now dispatches XLSX-specific exceptions (lines 527–532, 571–576, 585), but no test exercises the SoftTimeLimitExceeded → ReportScheduleXlsxTimeout path for XLSX format. The existing tests only cover the CSV timeout variant (test_get_notification_content_csv_format at test line ~1300). Without this test, an incorrect exception mapping for XLSX would go undetected.
Code Review Run #daaf18
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| ) -> None: | ||
| """_update_query_context surfaces the caller's error class (XLSX, not CSV).""" | ||
| schedule = mocker.Mock(spec=ReportSchedule) | ||
| state = BaseReportState(schedule, datetime.utcnow(), uuid4()) |
There was a problem hiding this comment.
datetime.utcnow() is deprecated in Python 3.12+ and returns a naive datetime. Replace with datetime.now(timezone.utc) from datetime module.
Code Review Run #daaf18
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
2 participants
SUMMARY
Adds Excel (
.xlsx) as an attachment format for chart-based Alerts & Reports, alongside the existing CSV / PNG / PDF / text options. Users can now pick "Send as Excel" in the report/alert modal and receive a formatted spreadsheetby email or Slack.
This picks up and finalizes the long-stalled #19810, which was closed in Nov 2025 due to inactivity. It resolves the two issues that blocked the original:
result_format=xlsx), so it reuses the exact same post-processing,include_indexlogic, and the formula-injection-safesuperset.utils.excel.df_to_excelpath as a chart's normal "Export to Excel" — no spurious column, and no duplicated logic._get_data(result_format)method instead of separate per-format methods.Key changes
ReportDataFormat: addXLSX+ atabular()helper ({CSV, XLSX}).BaseReportState._get_data(result_format): unified CSV/XLSX byte fetch; addsReportScheduleXlsx{Failed,Timeout}Error.NotificationContent.xlsx; email attaches<name>.xlsx; Slack v1 + v2 upload xlsx.NotificationFormats.XLSXand a "Send as Excel" option in bothAlertReportModaland theReportModal.XLSX availability mirrors CSV (chart-based reports only). No new feature flag, config key (the existing
EXCEL_EXPORTconfig is reused), or DB migration is required.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Automated:
pytest tests/unit_tests/commands/report/execute_test.py tests/unit_tests/reports/notifications/pytest tests/integration_tests/reports/commands_tests.py -k "csv or xlsx"npm run test -- AlertReportModalandnpm run test -- ReportModalManual:
email recipient, and trigger it.
<report name>.xlsxthat opens inExcel/LibreOffice with the expected data and no extra leading index column.
.xlsxfile is uploaded.ADDITIONAL INFORMATION