You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "gr33nak (via GitHub)" <gi...@apache.org> on 2026/06/25 14:08:30 UTC
[PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
gr33nak opened a new pull request, #41424:
URL: https://github.com/apache/superset/pull/41424
### 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 spreadsheet
by 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:
1. **Spurious index column** — the original generated the workbook by re-parsing the CSV into a DataFrame and writing it locally, which produced an extra index column. This implementation instead fetches the **pre-rendered XLSX from the chart data export endpoint** (`result_format=xlsx`), so it reuses the exact same post-processing, `include_index` logic, and the formula-injection-safe `superset.utils.excel.df_to_excel` path as a chart's normal "Export to Excel" — no spurious column, and no duplicated logic.
2. **Unified data fetching** — per maintainer feedback on the original PR, CSV and XLSX now share a single `_get_data(result_format)` method instead of separate per-format methods.
**Key changes**
- `ReportDataFormat`: add `XLSX` + a `tabular()` helper (`{CSV, XLSX}`).
- `BaseReportState._get_data(result_format)`: unified CSV/XLSX byte fetch; adds `ReportScheduleXlsx{Failed,Timeout}Error`.
- `NotificationContent.xlsx`; email attaches `<name>.xlsx`; Slack v1 + v2 upload xlsx.
- Frontend: `NotificationFormats.XLSX` and a "Send as Excel" option in both `AlertReportModal` and the `ReportModal`.
- Docs: list Excel among report attachment formats.
XLSX availability mirrors CSV (chart-based reports only). No new feature flag, config key (the existing `EXCEL_EXPORT` config is reused), or DB migration is required.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!-- Please attach a screenshot of the report/alert modal's "Content format" dropdown showing the new "Send as Excel" option, and (optionally) the received .xlsx email attachment. -->
<img width="378" height="557" alt="image" src="https://github.com/user-attachments/assets/4a44c468-ae1a-438c-bcb4-4f0731911f42" />
### TESTING INSTRUCTIONS
Automated:
- Unit: `pytest tests/unit_tests/commands/report/execute_test.py tests/unit_tests/reports/notifications/`
- Integration: `pytest tests/integration_tests/reports/commands_tests.py -k "csv or xlsx"`
- Frontend: `npm run test -- AlertReportModal` and `npm run test -- ReportModal`
Manual:
1. Enable the **Alerts & Reports** feature and have a worker/beat running.
2. Create a **Report** on a chart, set **Content format → Send as Excel**, add an
email recipient, and trigger it.
3. Confirm the email contains a valid `<report name>.xlsx` that opens in
Excel/LibreOffice with the expected data and **no extra leading index column**.
4. Repeat with a Slack recipient and confirm the `.xlsx` file is uploaded.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "gr33nak (via GitHub)" <gi...@apache.org>.
gr33nak commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4803014032
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "gr33nak (via GitHub)" <gi...@apache.org>.
gr33nak commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4834031273
Rebased, conflicts resolved, timeout test added, explicit type allocation addressed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3590547783
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -988,6 +1019,50 @@ def test_email_chart_report_schedule_with_csv(
assert_log(ReportState.SUCCESS)
+@pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices",
+ "create_report_email_chart_with_xlsx",
+)
+@patch("superset.utils.csv.urllib.request.urlopen")
+@patch("superset.utils.csv.urllib.request.OpenerDirector.open")
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Wrong mock module path for XLSX</b></div>
<div id="fix">
The XLSX test mocks `superset.utils.csv.urllib.request.urlopen` and `superset.utils.csv.urllib.request.OpenerDirector.open`, but XLSX data fetching goes through `_post_chart_data` in `execute.py` (lines 687-697) using `urllib.request.build_opener().open()` — not the `get_chart_csv_data` path in `csv.py`. This mismatch means the test isn't verifying the correct code path for XLSX generation. Note: `test_email_chart_report_schedule_with_csv` correctly patches `superset.utils.csv.*` because CSV does use `get_chart_csv_data` from `csv.py`.
</div>
</div>
<small><i>Code Review Run #6e2348</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3492584520
##########
tests/unit_tests/commands/report/execute_test.py:
##########
@@ -1190,6 +1191,22 @@ def test_update_query_context_wraps_screenshot_failure(mocker: MockerFixture) ->
state._update_query_context()
+def test_update_query_context_wraps_screenshot_failure_xlsx(
+ mocker: MockerFixture,
+) -> None:
+ """_update_query_context surfaces the caller's error class (XLSX, not CSV)."""
+ schedule = mocker.Mock(spec=ReportSchedule)
+ state = BaseReportState(schedule, datetime.utcnow(), uuid4())
Review Comment:
<!-- Bito Reply -->
The suggestion to replace `datetime.utcnow()` with `datetime.now(timezone.utc)` is correct. `datetime.utcnow()` is deprecated in Python 3.12+ and returns a naive datetime object, whereas `datetime.now(timezone.utc)` returns an aware datetime object, which is the recommended best practice for handling timezones in modern Python.
**tests/unit_tests/commands/report/execute_test.py**
```
schedule = mocker.Mock(spec=ReportSchedule)
state = BaseReportState(schedule, datetime.now(timezone.utc), uuid4())
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4835265133
<!-- translation-regression-bot -->
## ⚠️ Translation Regression Detected
A source change in this PR renamed or reworded strings, invalidating existing translations (they are now `#, fuzzy`) in `fi`, `th`. Please resolve the affected `.po` files before merging.
_Note: intentionally **deleting** a translatable string is not a regression and is not flagged here — only translations invalidated by a renamed/reworded source string are._
| Language | Fuzzy before | Fuzzy after | New |
|----------|-------------:|------------:|----:|
| `fi` | 4809 | 4811 | +2 |
| `th` | 4809 | 4811 | +2 |
### How to fix
**1. Install dependencies** (if not already set up):
```bash
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext
```
**2. Re-extract strings and sync `.po` files:**
```bash
./scripts/translations/babel_update.sh
```
This rewrites `superset/translations/messages.pot` from the current source files and merges the changes into every `.po` file. Strings whose `msgid` changed will be marked `#, fuzzy`.
**3. Resolve the fuzzy entries** in the affected language files (`fi`, `th`):
```bash
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.po
```
For each fuzzy entry, either rewrite the `msgstr` to match the new string and remove the `#, fuzzy` line, or clear the `msgstr` to `""` if you cannot provide a translation.
**4. Commit your changes to the `.po` files.**
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3475109463
##########
superset/commands/report/execute.py:
##########
@@ -510,9 +513,32 @@ def _get_pdf(self) -> bytes:
return pdf
- def _get_csv_data(self) -> bytes:
+ def _get_data(self, result_format: ChartDataResultFormat) -> bytes:
+ """
+ Fetch tabular chart data (CSV or Excel) as raw bytes.
+
+ Both formats are produced by the chart data export endpoint, so the
+ bytes are fetched the same way and only differ by ``result_format``.
+ This reuses the export path's post-processing and index handling,
+ keeping report output consistent with a chart's manual export.
+ """
+ timeout_error: type[CommandException]
+ failed_error: type[CommandException]
+ if result_format == ChartDataResultFormat.XLSX:
+ label, timeout_error, failed_error = (
+ "Excel",
+ ReportScheduleXlsxTimeout,
+ ReportScheduleXlsxFailedError,
+ )
Review Comment:
**Suggestion:** The XLSX-specific error mapping is incomplete: when query context is missing, `_update_query_context()` can raise `ReportScheduleCsvFailedError` before the XLSX timeout/failure classes are used, so Excel report failures are misclassified as CSV failures. Wrap/translate that prefetch error path to `ReportScheduleXlsxFailedError` when `result_format` is XLSX. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Excel report errors surfaced as CSV failure type.
- ⚠️ Error messages mention CSV despite Excel selection.
- ⚠️ Monitoring/metrics misattribute Excel failures to CSV.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a chart-based report schedule with `report_format == ReportDataFormat.XLSX`
so that `_get_notification_content()` in `BaseReportState`
(`superset/commands/report/execute.py:689-766`) will call
`_get_data(ChartDataResultFormat.XLSX)` at lines `720-723`.
2. Ensure the chart has no saved `query_context`
(`self._report_schedule.chart.query_context is None`), so `_get_data()` at
`execute.py:549-552` logs a warning and calls `_update_query_context()` before attempting
to fetch data.
3. Induce a screenshot failure (for example, a WebDriver issue) so that
`_get_screenshots()` (`execute.py:422-504`) raises `ReportScheduleScreenshotFailedError`
or `ReportScheduleScreenshotTimeout`; `_update_query_context()` at `execute.py:636-655`
catches these and re-raises `ReportScheduleCsvFailedError` with a message about missing
query context.
4. Because this `ReportScheduleCsvFailedError` is raised before the `try/except` block in
`_get_data()` (lines `553-581`) and `_get_data()` only maps later failures to the
XLSX-specific `ReportScheduleXlsxFailedError` / `ReportScheduleXlsxTimeout`, the XLSX run
propagates a CSV-specific error (with message defined in
`superset/commands/report/exceptions.py:16-21`), misclassifying Excel failures as CSV
failures.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7168a6bcf5ee4036a46dbe481593a97b&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=7168a6bcf5ee4036a46dbe481593a97b&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/execute.py
**Line:** 527:532
**Comment:**
*Logic Error: The XLSX-specific error mapping is incomplete: when query context is missing, `_update_query_context()` can raise `ReportScheduleCsvFailedError` before the XLSX timeout/failure classes are used, so Excel report failures are misclassified as CSV failures. Wrap/translate that prefetch error path to `ReportScheduleXlsxFailedError` when `result_format` is XLSX.
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=069cb69d4ad6e0f8af89d0a507fb689253e3bab08addf8d058333e1a6441d818&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=069cb69d4ad6e0f8af89d0a507fb689253e3bab08addf8d058333e1a6441d818&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -687,11 +715,16 @@ def _get_notification_content(self) -> NotificationContent: # noqa: C901
error_text = "Unexpected missing pdf"
elif (
self._report_schedule.chart
- and self._report_schedule.report_format == ReportDataFormat.CSV
+ and self._report_schedule.report_format in ReportDataFormat.tabular()
):
- csv_data = self._get_csv_data()
- if not csv_data:
- error_text = "Unexpected missing csv file"
+ if self._report_schedule.report_format == ReportDataFormat.XLSX:
+ xlsx_data = self._get_data(ChartDataResultFormat.XLSX)
+ if not xlsx_data:
+ error_text = "Unexpected missing Excel file"
Review Comment:
**Suggestion:** XLSX generation is now enabled for all tabular report schedules, but webhook notifications still only serialize CSV/PDF/PNG attachments. For webhook recipients, `xlsx` bytes are produced here and then silently dropped downstream, so users receive a successful notification without the requested file. Either gate XLSX to recipient types that support it or add XLSX handling in webhook file packaging. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Webhook reports with Excel format lack XLSX attachment.
- ⚠️ Webhook consumers cannot access requested tabular report.
- ⚠️ Users see successful run but missing Excel content.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a chart-based report schedule with format `ReportDataFormat.XLSX` and at least
one webhook recipient (type `ReportRecipientType.WEBHOOK`) in `ReportSchedule.recipients`
(used by `BaseReportState._send` at `superset/commands/report/execute.py:768-784`).
2. When the scheduler runs `AsyncExecuteReportScheduleCommand.run()`
(`superset/commands/report/execute.py:1198-1217`), it builds a `BaseReportState` and calls
`BaseReportState.send()`, which in turn calls `_get_notification_content()` at
`superset/commands/report/execute.py:689-766`.
3. In `_get_notification_content()`, the XLSX branch at lines `716-723` executes: because
`self._report_schedule.chart` is set and `report_format` is in
`ReportDataFormat.tabular()`, `self._get_data(ChartDataResultFormat.XLSX)` is called and
the returned bytes are stored in `xlsx_data`, which is then passed into
`NotificationContent(..., xlsx=xlsx_data, ...)` at lines `756-763`.
4. For the webhook recipient, `create_notification()`
(`superset/reports/notifications/__init__.py:18-30`) instantiates `WebhookNotification`,
whose `_get_files()` method at `superset/reports/notifications/webhook.py:79-95` only
attaches `content.csv`, `content.pdf`, and `content.screenshots`, ignoring `content.xlsx`,
so the outgoing `requests.post()` in `WebhookNotification.send()` (`webhook.py:134-164`)
never includes the Excel file even though the report format was XLSX and the execution
succeeds.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5076a0776fae4f25b2552dd0116d7fcf&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=5076a0776fae4f25b2552dd0116d7fcf&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/execute.py
**Line:** 716:723
**Comment:**
*Incomplete Implementation: XLSX generation is now enabled for all tabular report schedules, but webhook notifications still only serialize CSV/PDF/PNG attachments. For webhook recipients, `xlsx` bytes are produced here and then silently dropped downstream, so users receive a successful notification without the requested file. Either gate XLSX to recipient types that support it or add XLSX handling in webhook file packaging.
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=d0209730c4dd03c247476f0e6e63b938d4aa0abcec89bd2fd879064c4de125bc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=d0209730c4dd03c247476f0e6e63b938d4aa0abcec89bd2fd879064c4de125bc&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3592710241
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -988,6 +1019,50 @@ def test_email_chart_report_schedule_with_csv(
assert_log(ReportState.SUCCESS)
+@pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices",
+ "create_report_email_chart_with_xlsx",
+)
+@patch("superset.utils.csv.urllib.request.urlopen")
+@patch("superset.utils.csv.urllib.request.OpenerDirector.open")
Review Comment:
False positive on this one. `patch("superset.utils.csv.urllib.request.OpenerDirector.open")` isn't patching a csv-local copy, it resolves to the same global `urllib.request.OpenerDirector` class, so the `build_opener().open()` in `_post_chart_data` goes through the exact same mock. The xlsx fixture sets a query_context, so the request takes that path, and the test only passes because the attachment actually comes back as `XLSX_FILE`. Resolving.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4802657436
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41424?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `26.19048%` with `31 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.31%. Comparing base ([`83f7dc9`](https://app.codecov.io/gh/apache/superset/commit/83f7dc9d5b3babd52f305cd12d2f4a4fbd1f1486?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f1c5ee7`](https://app.codecov.io/gh/apache/superset/commit/f1c5ee71fb91fef21bbc224953ab54a1904817f4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/41424?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/report/execute.py](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&filepath=superset%2Fcommands%2Freport%2Fexecute.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvcmVwb3J0L2V4ZWN1dGUucHk=) | 9.09% | [20 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/reports/notifications/email.py](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&filepath=superset%2Freports%2Fnotifications%2Femail.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL2VtYWlsLnB5) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/reports/notifications/slack.py](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&filepath=superset%2Freports%2Fnotifications%2Fslack.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL3NsYWNrLnB5) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/reports/notifications/slackv2.py](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&filepath=superset%2Freports%2Fnotifications%2Fslackv2.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL3NsYWNrdjIucHk=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/reports/notifications/webhook.py](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&filepath=superset%2Freports%2Fnotifications%2Fwebhook.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL3dlYmhvb2sucHk=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/reports/models.py](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&filepath=superset%2Freports%2Fmodels.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9tb2RlbHMucHk=) | 75.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41424?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (83f7dc9) and HEAD (f1c5ee7). Click for more details.
>
> <details><summary>HEAD has 3 uploads less than BASE</summary>
>
>| Flag | BASE (83f7dc9) | HEAD (f1c5ee7) |
>|------|------|------|
>|python|6|3|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41424 +/- ##
==========================================
- Coverage 64.47% 56.31% -8.16%
==========================================
Files 2661 2661
Lines 145673 145697 +24
Branches 33620 33625 +5
==========================================
- Hits 93916 82053 -11863
- Misses 50053 62903 +12850
+ Partials 1704 741 -963
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41424/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/41424/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.23% <26.19%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/41424/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/41424/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/41424/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.81% <26.19%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/41424/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `42.10% <26.19%> (-17.40%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/41424/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/41424/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41424?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4984929439
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "kominma3 (via GitHub)" <gi...@apache.org>.
kominma3 commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4984928223
/review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4800237228
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 9082196e8585bbd2f5054801b5bc953b7c679eb9 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a3d365f997ec80008804e7a |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-41424--superset-docs-preview.netlify.app](https://deploy-preview-41424--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcoxDoAgDADAv3TWSIIi-AOfUaGEQaWBDhjj32W74V7gki8W2AAGOArePnVHQpnaWdtYiHOREUXQp4tuqf0F4jM_e-jToA7aLNG5lbxVSlmrZloRvh8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41424--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3592710779
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -988,6 +1019,50 @@ def test_email_chart_report_schedule_with_csv(
assert_log(ReportState.SUCCESS)
+@pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices",
+ "create_report_email_chart_with_xlsx",
+)
+@patch("superset.utils.csv.urllib.request.urlopen")
+@patch("superset.utils.csv.urllib.request.OpenerDirector.open")
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion regarding the mock path for XLSX is a false positive. As you correctly identified, patching `superset.utils.csv.urllib.request.OpenerDirector.open` correctly intercepts the `urllib.request.build_opener().open()` call used in `_post_chart_data` because it resolves to the same global class. The test is functioning as intended.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3477596959
##########
superset/commands/report/execute.py:
##########
@@ -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
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing XLSX timeout test coverage</b></div>
<div id="fix">
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.
</div>
</div>
<small><i>Code Review Run #daaf18</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/commands/report/execute_test.py:
##########
@@ -1190,6 +1191,22 @@ def test_update_query_context_wraps_screenshot_failure(mocker: MockerFixture) ->
state._update_query_context()
+def test_update_query_context_wraps_screenshot_failure_xlsx(
+ mocker: MockerFixture,
+) -> None:
+ """_update_query_context surfaces the caller's error class (XLSX, not CSV)."""
+ schedule = mocker.Mock(spec=ReportSchedule)
+ state = BaseReportState(schedule, datetime.utcnow(), uuid4())
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Deprecated datetime.utcnow usage</b></div>
<div id="fix">
`datetime.utcnow()` is deprecated in Python 3.12+ and returns a naive datetime. Replace with `datetime.now(timezone.utc)` from `datetime` module.
</div>
</div>
<small><i>Code Review Run #daaf18</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3493684986
##########
superset/reports/models.py:
##########
@@ -84,8 +84,14 @@ class ReportDataFormat(StrEnum):
PDF = "PDF"
PNG = "PNG"
CSV = "CSV"
+ XLSX = "XLSX"
TEXT = "TEXT"
+ @classmethod
+ def tabular(cls: type["ReportDataFormat"]) -> set["ReportDataFormat"]:
+ """Formats produced from tabular chart data via the chart export path."""
+ return {cls.CSV, cls.XLSX}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test for tabular() method</b></div>
<div id="fix">
The `tabular()` classmethod is a new public method introduced in this PR, but it lacks direct unit test coverage. Existing tests use `XLSX` directly but do not verify the `tabular()` method itself. Per adaptive rule [11730], new functionality should be covered by tests validating success paths. A test like `test_tabular_returns_csv_and_xlsx` would ensure this method behaves correctly as the canonical source of truth for tabular formats.
</div>
</div>
<small><i>Code Review Run #99dbdf</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3492552286
##########
superset/reports/notifications/email.py:
##########
@@ -205,9 +205,13 @@ def _get_content(self) -> EmailContent:
</html>
"""
)
- csv_data = None
+ # CSV and Excel are mutually exclusive (a report has a single format),
+ # so at most one tabular attachment is present in the data dict.
+ attachment_data = None
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly introduced local variable to satisfy the type-hint requirement. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new local variable is introduced without a type hint, and its type is readily inferable as an optional dict of attachment names to bytes. That matches the custom rule requiring type hints on relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=31bf1551e43444ca90e5de83823bd862&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=31bf1551e43444ca90e5de83823bd862&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/reports/notifications/email.py
**Line:** 210:210
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced local variable to satisfy the type-hint requirement.
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=b0a29f80f734a5aab8f66d8dc9661b4c5127aa4092129f0a8aacdbf8ef1e315c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=b0a29f80f734a5aab8f66d8dc9661b4c5127aa4092129f0a8aacdbf8ef1e315c&reaction=dislike'>👎</a>
##########
tests/integration_tests/reports/utils.py:
##########
@@ -42,6 +42,9 @@
TEST_ID = str(uuid4())
CSV_FILE = read_fixture("trends.csv")
+# Reports fetch tabular data as opaque bytes from the (mocked) chart export
+# endpoint, so any distinct non-empty payload is sufficient for Excel tests.
+XLSX_FILE = b"PK\x03\x04 mock xlsx bytes"
Review Comment:
**Suggestion:** Add an explicit type annotation for the new XLSX payload constant so the newly introduced variable is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new module-level constant is clearly a bytes value and can be annotated as `bytes`, but it is left untyped. This matches the rule for newly added Python variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8c72fb754fae47148d6aa834db262a50&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=8c72fb754fae47148d6aa834db262a50&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/integration_tests/reports/utils.py
**Line:** 47:47
**Comment:**
*Custom Rule: Add an explicit type annotation for the new XLSX payload constant so the newly introduced variable 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%2F41424&comment_hash=33e226125ebe468274f1bac766de1fa813bfc75a8ecec9132c2db3c6b211d842&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=33e226125ebe468274f1bac766de1fa813bfc75a8ecec9132c2db3c6b211d842&reaction=dislike'>👎</a>
##########
superset/commands/report/exceptions.py:
##########
@@ -296,6 +302,13 @@ class ReportScheduleCsvTimeout(CommandException):
message = _("A timeout occurred while generating a csv.")
+class ReportScheduleXlsxTimeout(CommandException):
+ """Raised when generating the Excel (xlsx) attachment for a report times out."""
+
+ status = 408
+ message = _("A timeout occurred while generating an Excel file.")
Review Comment:
**Suggestion:** Add an explicit type annotation to the class-level `status` attribute in this new exception class. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new class-level integer attribute is added without a type annotation, and the custom rule requires type hints on new or modified Python variables that can be annotated. The violation is real.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e27bbbe776014b0db3afcc86c56c3fe5&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=e27bbbe776014b0db3afcc86c56c3fe5&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:** 305:309
**Comment:**
*Custom Rule: Add an explicit type annotation to the class-level `status` attribute in this new exception class.
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=cbecaf09743cea2bb2efe2e0acee5a247431e943159b682584dfc5f7e6e29f7d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=cbecaf09743cea2bb2efe2e0acee5a247431e943159b682584dfc5f7e6e29f7d&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -717,6 +748,7 @@ def _get_notification_content(self) -> NotificationContent: # noqa: C901
:raises: ReportScheduleScreenshotFailedError
"""
csv_data = None
+ xlsx_data = None
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly introduced local variable to satisfy the type-hint requirement for relevant variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new local variable is introduced without a type annotation even though its type is clear and it is part of modified Python code. This matches the rule requiring type hints for relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b1f02d9570f248ff8d334f60c1ee1fe2&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=b1f02d9570f248ff8d334f60c1ee1fe2&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/execute.py
**Line:** 751:751
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced local variable to satisfy the type-hint requirement for relevant variables.
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=871b607f3ff22353c95539707039164b66fcabca76bd57783a86c9510985a7f2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=871b607f3ff22353c95539707039164b66fcabca76bd57783a86c9510985a7f2&reaction=dislike'>👎</a>
##########
tests/unit_tests/reports/notifications/webhook_tests.py:
##########
@@ -171,6 +171,37 @@ def test_get_files_includes_all_content_types(mock_header_data) -> None:
assert mime_types.count("image/png") == 2
+def test_get_files_includes_xlsx(mock_header_data: HeaderDataType) -> None:
+ """_get_files attaches xlsx bytes as report.xlsx with the spreadsheet MIME type."""
Review Comment:
**Suggestion:** Add an explicit type annotation to the new local variable so the newly introduced test code fully complies with the required type-hinting rule. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new local variable is introduced without a type annotation, and it can clearly be annotated as `bytes`. This matches the rule requiring type hints for new or modified Python code where applicable.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=02e73bd81203466b94485a93096d9f3f&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=02e73bd81203466b94485a93096d9f3f&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:** 175:175
**Comment:**
*Custom Rule: Add an explicit type annotation to the new local variable so the newly introduced test code fully complies with the required type-hinting 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%2F41424&comment_hash=10b2a409ac61b3f56cc0d71ac407823c5cf97f2ea7393a42857dc4ce72a1069c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=10b2a409ac61b3f56cc0d71ac407823c5cf97f2ea7393a42857dc4ce72a1069c&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4835405698
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #41424:
URL: https://github.com/apache/superset/pull/41424
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3475040086
##########
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:
Review Comment:
**Suggestion:** Add a type annotation for the `mock_header_data` parameter in this newly added test function so the new Python code is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test function, and its `mock_header_data` parameter has no type annotation. The custom rule requires newly added Python functions to be fully typed, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3a8f59c40e624b8e8d7d8989bcb25f76&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=3a8f59c40e624b8e8d7d8989bcb25f76&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/slack_tests.py
**Line:** 149:149
**Comment:**
*Custom Rule: Add a type annotation for the `mock_header_data` parameter in this newly added test function so the new Python code 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%2F41424&comment_hash=a0c7999936b1eb82c025a7255fed591713406d79aa5cff681354e728168ec060&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=a0c7999936b1eb82c025a7255fed591713406d79aa5cff681354e728168ec060&reaction=dislike'>👎</a>
##########
superset/reports/models.py:
##########
@@ -84,8 +84,14 @@ class ReportDataFormat(StrEnum):
PDF = "PDF"
PNG = "PNG"
CSV = "CSV"
+ XLSX = "XLSX"
TEXT = "TEXT"
+ @classmethod
+ def tabular(cls) -> set["ReportDataFormat"]:
+ """Formats produced from tabular chart data via the chart export path."""
+ return {cls.CSV, cls.XLSX}
Review Comment:
**Suggestion:** Add an explicit type hint for the class method parameter so the new method has complete parameter and return typing. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added class method in modified Python code, and its `cls` parameter has no type annotation. The custom rule requires new Python functions and methods to be fully typed, so the suggestion identifies a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d9b3e27464b4463fb4bab36c7660f0fb&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=d9b3e27464b4463fb4bab36c7660f0fb&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/reports/models.py
**Line:** 90:93
**Comment:**
*Custom Rule: Add an explicit type hint for the class method parameter so the new method has complete parameter and return typing.
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=de9fd7aa82c48bd69825b69c08c7b304727b7ca54984e5a5d1a4e1dcfc3543a2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=de9fd7aa82c48bd69825b69c08c7b304727b7ca54984e5a5d1a4e1dcfc3543a2&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4800238132
<!-- Bito Reply -->
The suggestion to add a type annotation for the `mock_header_data` parameter is correct, as it ensures the new test code adheres to the project's typing standards. To resolve this, you should update the function signature in the test file to include the appropriate type hint for the parameter.
Since the file `tests/unit_tests/reports/notifications/slack_tests.py` is not present in the provided PR context, I cannot implement the fix directly. Please apply the type annotation manually in that file.
Regarding other comments on this PR, there are no additional review comments available in the provided context to evaluate.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4800221477
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3476426498
##########
superset/commands/report/exceptions.py:
##########
@@ -275,6 +279,11 @@ class ReportScheduleCsvTimeout(CommandException):
message = _("A timeout occurred while generating a csv.")
+class ReportScheduleXlsxTimeout(CommandException):
+ status = 408
+ message = _("A timeout occurred while generating an Excel file.")
Review Comment:
**Suggestion:** Add a concise docstring to this new timeout exception class to comply with documentation requirements for new classes. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is also a newly added Python class and it lacks a docstring. That directly violates the rule requiring new Python classes to be documented inline, so the suggestion is valid.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=26879b4b952e4d3786d10caa92ea4333&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=26879b4b952e4d3786d10caa92ea4333&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:** 282:284
**Comment:**
*Custom Rule: Add a concise docstring to this new timeout exception class to comply with documentation requirements for new classes.
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=aa25cdfa6ab22fb3afac7b904ddad002eb54c85a1fd70ab429315befe90713c0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=aa25cdfa6ab22fb3afac7b904ddad002eb54c85a1fd70ab429315befe90713c0&reaction=dislike'>👎</a>
##########
superset/commands/report/exceptions.py:
##########
@@ -196,6 +196,10 @@ class ReportScheduleCsvFailedError(CommandException):
message = _("Report Schedule execution failed when generating a csv.")
+class ReportScheduleXlsxFailedError(CommandException):
+ message = _("Report Schedule execution failed when generating an Excel file.")
Review Comment:
**Suggestion:** Add a short class docstring describing the purpose of this new exception class. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python class and it has no docstring immediately under the class definition. The custom rule requires newly added Python classes to include docstrings, so the suggestion identifies a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=af8a1b031a6b4e88af83bd2c0f281d69&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=af8a1b031a6b4e88af83bd2c0f281d69&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:** 199:200
**Comment:**
*Custom Rule: Add a short class docstring describing the purpose of this new exception class.
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=553ea8cf97280b6c35df738749b3e4ba336fd170a3f806c7ea13315cb54fe79e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=553ea8cf97280b6c35df738749b3e4ba336fd170a3f806c7ea13315cb54fe79e&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4803996838
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "gr33nak (via GitHub)" <gi...@apache.org>.
gr33nak commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3492583161
##########
tests/unit_tests/commands/report/execute_test.py:
##########
@@ -1190,6 +1191,22 @@ def test_update_query_context_wraps_screenshot_failure(mocker: MockerFixture) ->
state._update_query_context()
+def test_update_query_context_wraps_screenshot_failure_xlsx(
+ mocker: MockerFixture,
+) -> None:
+ """_update_query_context surfaces the caller's error class (XLSX, not CSV)."""
+ schedule = mocker.Mock(spec=ReportSchedule)
+ state = BaseReportState(schedule, datetime.utcnow(), uuid4())
Review Comment:
the whole test module use datetime.utcnow() as the established convention
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3475046184
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -246,6 +247,19 @@ def create_report_email_chart_with_csv():
cleanup_report_schedule(report_schedule)
+@pytest.fixture
+def create_report_email_chart_with_xlsx():
Review Comment:
**Suggestion:** Add an explicit return type annotation to this new fixture function so newly added code remains fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is newly added Python code and the function has no return type annotation. The custom 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=29c5c0a2b67449afa403bc9703e9cefc&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=29c5c0a2b67449afa403bc9703e9cefc&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/integration_tests/reports/commands_tests.py
**Line:** 251:251
**Comment:**
*Custom Rule: Add an explicit return type annotation to this new fixture function so newly added code remains 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%2F41424&comment_hash=6adb216285f06cee8db43fe55346a8fac65c3b9465700c8b8bb0a6fbef90827e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=6adb216285f06cee8db43fe55346a8fac65c3b9465700c8b8bb0a6fbef90827e&reaction=dislike'>👎</a>
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -978,6 +1006,50 @@ def test_email_chart_report_schedule_with_csv(
assert_log(ReportState.SUCCESS)
+@pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices",
+ "create_report_email_chart_with_xlsx",
+)
+@patch("superset.utils.csv.urllib.request.urlopen")
+@patch("superset.utils.csv.urllib.request.OpenerDirector.open")
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch("superset.utils.csv.get_chart_csv_data")
+def test_email_chart_report_schedule_with_xlsx(
+ xlsx_mock,
+ email_mock,
+ mock_open,
+ mock_urlopen,
+ create_report_email_chart_with_xlsx,
+):
Review Comment:
**Suggestion:** Add type hints for all parameters and the return value in this new test function to align with the fully typed new-code rule. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new test function has no parameter annotations or return type annotation. Under the custom rule, newly added Python functions in changed code should be fully typed, so this is a verified violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8a3a7129151a44b8baf25931ad42c364&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=8a3a7129151a44b8baf25931ad42c364&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/integration_tests/reports/commands_tests.py
**Line:** 1017:1023
**Comment:**
*Custom Rule: Add type hints for all parameters and the return value in this new test function to align with the fully typed new-code 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%2F41424&comment_hash=b17ab872ad54b17d6af578154308ffb7c250ba8c130e22484ae90c87b62103c5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=b17ab872ad54b17d6af578154308ffb7c250ba8c130e22484ae90c87b62103c5&reaction=dislike'>👎</a>
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -334,6 +348,20 @@ def create_report_slack_chart_with_csv():
cleanup_report_schedule(report_schedule)
+@pytest.fixture
+def create_report_slack_chart_with_xlsx():
+ chart = db.session.query(Slice).first()
+ chart.query_context = '{"mock": "query_context"}'
+ report_schedule = create_report_notification(
+ slack_channel="slack_channel",
+ chart=chart,
+ report_format=ReportDataFormat.XLSX,
+ )
+ yield report_schedule
+
+ cleanup_report_schedule(report_schedule)
Review Comment:
**Suggestion:** Add a concise docstring to this new fixture function so its setup intent is documented inline. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced fixture function and it does not have a docstring. That matches the custom rule requiring docstrings for newly added Python functions and classes.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=92f7c3cbd53642caa8630cc6d6eb9efd&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=92f7c3cbd53642caa8630cc6d6eb9efd&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/integration_tests/reports/commands_tests.py
**Line:** 352:362
**Comment:**
*Custom Rule: Add a concise docstring to this new fixture function so its setup intent is documented inline.
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=613f5e7fe4480353ddd7584a82a7ee291ffa6e5c9ff7792c7b9910f1f805d49c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=613f5e7fe4480353ddd7584a82a7ee291ffa6e5c9ff7792c7b9910f1f805d49c&reaction=dislike'>👎</a>
##########
tests/unit_tests/reports/notifications/email_tests.py:
##########
@@ -144,3 +144,34 @@ def test_email_subject_with_datetime() -> None:
subject = notification._get_subject()
assert datetime_pattern not in subject
assert frozen_now.strftime(datetime_pattern) in subject
+
+
+def test_email_content_with_xlsx_attachment() -> None:
+ # `superset.models.helpers`, a dependency of following imports,
+ # requires app context
Review Comment:
**Suggestion:** Add a function docstring immediately under the test definition to document the behavior being validated before the inline comments and setup logic. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added test function and it does not include a docstring immediately under the function definition. The custom rule requires newly added Python functions and classes to be documented inline, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bdc803ee11ff463e856cabfb9036bdcd&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=bdc803ee11ff463e856cabfb9036bdcd&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/email_tests.py
**Line:** 149:151
**Comment:**
*Custom Rule: Add a function docstring immediately under the test definition to document the behavior being validated before the inline comments and setup logic.
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=646160368fdc32e418b1798a9f700ee87e2e0b56686e7de55d176890a7c5fbf1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=646160368fdc32e418b1798a9f700ee87e2e0b56686e7de55d176890a7c5fbf1&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/report/execute_test.py:
##########
@@ -1208,10 +1208,24 @@ def test_get_notification_content_csv_format(mock_ff, mocker: MockerFixture) ->
state = _make_notification_state(
mocker, report_format=ReportDataFormat.CSV, has_chart=True
)
- mocker.patch.object(state, "_get_csv_data", return_value=b"col1,col2\n1,2")
+ mocker.patch.object(state, "_get_data", return_value=b"col1,col2\n1,2")
content = state._get_notification_content()
assert content.csv == b"col1,col2\n1,2"
+ assert content.xlsx is None
+
+
+@patch("superset.commands.report.execute.feature_flag_manager")
+def test_get_notification_content_xlsx_format(mock_ff, mocker: MockerFixture) -> None:
Review Comment:
**Suggestion:** Add a type annotation for the untyped `mock_ff` parameter so this newly added test function has complete parameter and return typing. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test function, and the custom rule requires new or modified Python functions to be fully typed. The `mock_ff` parameter has no type annotation, so the suggestion correctly identifies a real rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=25d498cd5e6c42f58a1f4145a53eb3f7&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=25d498cd5e6c42f58a1f4145a53eb3f7&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:** 1219:1219
**Comment:**
*Custom Rule: Add a type annotation for the untyped `mock_ff` parameter so this newly added test function has complete parameter and return typing.
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=c4041717d0b375f4dcf0b18906a3cad10b6e97e18174a1d0c174350bc77e7e9a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=c4041717d0b375f4dcf0b18906a3cad10b6e97e18174a1d0c174350bc77e7e9a&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4982676899
**Bito Review Failed - Technical Failure** Bito encountered technical difficulties while starting a code review session. To retry, type `/review` in a comment and save. If the issue persists, contact support@bito.ai.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "kominma3 (via GitHub)" <gi...@apache.org>.
kominma3 commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4982549553
@rusackas @gr33nak Hello, I tried to finish the neccessary changes. Hope that could be enough now.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add XLSX (Excel) attachments for Alerts & Reports [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41424:
URL: https://github.com/apache/superset/pull/41424#issuecomment-4931139096
Thanks @gr33nak, this cleaned up nicely since the last pass! The CSV/XLSX merge into `_get_data` reads well and the timeout/failure classification tests cover the cases I'd worry about.
One bot thread is still open though, the Bito one above. Mind adding a quick test for that one?
Also conflicting with master again, needs another rebase (sorry, a huge swath of Python changes just merged).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org