diff --git a/docs/admin_docs/configuration/alerts-reports.mdx b/docs/admin_docs/configuration/alerts-reports.mdx index 6f3b67e51c7..bedc88ac4c9 100644 --- a/docs/admin_docs/configuration/alerts-reports.mdx +++ b/docs/admin_docs/configuration/alerts-reports.mdx @@ -361,7 +361,7 @@ This is the best source of information about the problem. In a docker compose d ### Check web browser and webdriver installation -To take a screenshot, the worker visits the dashboard or chart using a headless browser, then takes a screenshot. If you are able to send a chart as CSV or text but can't send as PNG, your problem may lie with the browser. +To take a screenshot, the worker visits the dashboard or chart using a headless browser, then takes a screenshot. If you are able to send a chart as CSV, XLSX, or text but can't send as PNG, your problem may lie with the browser. If you are handling the installation of the headless browser on your own, do your own verification to ensure that the headless browser opens successfully in the worker environment. diff --git a/docs/user_docs_versioned_docs/version-6.0.0/configuration/alerts-reports.mdx b/docs/user_docs_versioned_docs/version-6.0.0/configuration/alerts-reports.mdx index 3bd6e334903..f5fc0d2e6db 100644 --- a/docs/user_docs_versioned_docs/version-6.0.0/configuration/alerts-reports.mdx +++ b/docs/user_docs_versioned_docs/version-6.0.0/configuration/alerts-reports.mdx @@ -291,7 +291,7 @@ This is the best source of information about the problem. In a docker compose d ### Check web browser and webdriver installation -To take a screenshot, the worker visits the dashboard or chart using a headless browser, then takes a screenshot. If you are able to send a chart as CSV or text but can't send as PNG, your problem may lie with the browser. +To take a screenshot, the worker visits the dashboard or chart using a headless browser, then takes a screenshot. If you are able to send a chart as CSV, XLSX, or text but can't send as PNG, your problem may lie with the browser. Superset docker images that have a tag ending with `-dev` have the Firefox headless browser and geckodriver already installed. You can test that these are installed and in the proper path by entering your Superset worker and running `firefox --headless` and then `geckodriver`. Both commands should start those applications. diff --git a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx index 22ca73ed44b..2d7f474fded 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx @@ -885,6 +885,29 @@ test('shows screenshot width when PDF is selected', async () => { expect(screenshotWidth).not.toHaveAttribute('type', 'number'); }); +test('does not show screenshot width when excel is selected', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('contents-panel')); + await screen.findByText(/test chart/i); + const contentTypeSelector = screen.getByRole('combobox', { + name: /select content type/i, + }); + await comboboxSelect(contentTypeSelector, 'Chart', () => + screen.getByText(/select chart/i), + ); + const reportFormatSelector = screen.getByRole('combobox', { + name: /select format/i, + }); + await comboboxSelect( + reportFormatSelector, + 'XLSX', + () => screen.getAllByText(/Send as Excel/i)[0], + ); + expect(screen.queryByRole('spinbutton')).not.toBeInTheDocument(); +}); + // Schedule Section test('opens Schedule Section on click', async () => { render(, { diff --git a/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx b/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx index b7a45e7e890..5766b4b4303 100644 --- a/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx +++ b/superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx @@ -135,6 +135,13 @@ test('does not allow user to create a report without a name', () => { expect(addButton).toBeDisabled(); }); +test('shows xlsx notification format option', () => { + render(, { useRedux: true }); + expect( + screen.getByText('Formatted Excel attached in email'), + ).toBeInTheDocument(); +}); + test('creates a new email report via modal Add button', async () => { // The modal calls POST /api/v1/report/subscribe; creation_method, editors, and // recipients are derived server-side — the client payload intentionally omits them. @@ -434,7 +441,7 @@ test('submit failure dispatches danger toast and keeps modal open', async () => ).toBe(true); }); - // Modal stays open — onHide should NOT have been called + // Modal stays open; onHide should NOT have been called. expect(onHide).not.toHaveBeenCalled(); expect(screen.getByText('Schedule a new email report')).toBeInTheDocument(); diff --git a/superset/charts/client_processing.py b/superset/charts/client_processing.py index 36bf9a1b9d4..e0c2db71af7 100644 --- a/superset/charts/client_processing.py +++ b/superset/charts/client_processing.py @@ -25,7 +25,7 @@ In order to do that, we reproduce the post-processing in Python for these chart """ import logging -from io import StringIO +from io import BytesIO, StringIO from typing import Any, Optional, TYPE_CHECKING, Union import numpy as np @@ -35,7 +35,7 @@ from flask_babel import gettext as __ from superset.common.chart_data import ChartDataResultFormat from superset.extensions import event_logger -from superset.utils import csv +from superset.utils import csv, excel from superset.utils.core import ( extract_dataframe_dtypes, get_column_names, @@ -310,6 +310,35 @@ post_processors = { } +def _is_default_index_column(series: pd.Series) -> bool: + return series.tolist() == list(range(len(series))) + + +def _read_excel_for_client_processing( + data: bytes, + form_data: dict[str, Any], +) -> pd.DataFrame: + df = pd.read_excel(BytesIO(data)) + if len(df.columns) == 0: + return df + + first_column = df.columns[0] + expected_columns = { + *get_column_names(form_data.get("columns")), + *get_column_names(form_data.get("groupbyRows")), + *get_column_names(form_data.get("groupbyColumns")), + *get_metric_names(form_data.get("metrics")), + } + + if first_column in expected_columns: + return df + + if _is_default_index_column(df.iloc[:, 0]): + return df.iloc[:, 1:].reset_index(drop=True) + + return df.set_index(first_column) + + @event_logger.log_this def apply_client_processing( # noqa: C901 result: dict[Any, Any], @@ -357,6 +386,8 @@ def apply_client_processing( # noqa: C901 sep=sep, decimal=decimal, ) + elif query["result_format"] == ChartDataResultFormat.XLSX: + df = _read_excel_for_client_processing(data, form_data) # convert all columns to verbose (label) name if datasource: @@ -403,5 +434,14 @@ def apply_client_processing( # noqa: C901 index=show_default_index, **current_app.config["CSV_EXPORT"], ) + elif query["result_format"] == ChartDataResultFormat.XLSX: + excel.apply_column_types(processed_df, query["coltypes"]) + query["data"] = excel.df_to_excel( + processed_df, + **{ + **current_app.config["EXCEL_EXPORT"], + "index": show_default_index, + }, + ) return result diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 50cfe33af0b..d0788b00cc5 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -306,6 +306,7 @@ class BaseReportState: ChartDataResultFormat.CSV, ChartDataResultFormat.XLSX, ChartDataResultFormat.JSON, + ChartDataResultFormat.XLSX, }: return get_url_path( "ChartDataRestApi.get_data", @@ -718,6 +719,11 @@ class BaseReportState: This reuses the export path's post-processing and index handling, keeping report output consistent with a chart's manual export. """ + if result_format not in ChartDataResultFormat.table_like(): + raise ReportScheduleExecuteUnexpectedError( + f"Unsupported chart data result format: {result_format}" + ) + timeout_error: type[CommandException] failed_error: type[CommandException] if result_format == ChartDataResultFormat.XLSX: @@ -940,6 +946,10 @@ class BaseReportState: csv_data = self._get_data(ChartDataResultFormat.CSV) if not csv_data: error_text = "Unexpected missing csv file" + elif self._report_schedule.report_format == ReportDataFormat.XLSX: + raise ReportScheduleXlsxFailedError( + "XLSX reports are only supported for chart schedules" + ) if error_text: return NotificationContent( name=sanitize_title(self._report_schedule.name), diff --git a/superset/reports/notifications/email.py b/superset/reports/notifications/email.py index 4682978cb83..46887172e4b 100644 --- a/superset/reports/notifications/email.py +++ b/superset/reports/notifications/email.py @@ -19,7 +19,9 @@ import textwrap from dataclasses import dataclass from datetime import datetime from email.utils import make_msgid, parseaddr -from typing import Any, Optional +from io import BytesIO +from typing import IO, Optional +from zipfile import BadZipFile, ZipFile import nh3 from flask import current_app @@ -66,17 +68,49 @@ ALLOWED_ATTRIBUTES = { "acronym": {"title"}, **ALLOWED_TABLE_ATTRIBUTES, } +ZIP_LOCAL_FILE_HEADER = b"PK\x03\x04" @dataclass class EmailContent: body: str header_data: Optional[HeaderDataType] = None - data: Optional[dict[str, Any]] = None + data: Optional[dict[str, bytes | str]] = None pdf: Optional[dict[str, bytes]] = None images: Optional[dict[str, bytes]] = None +def _get_xlsx_attachment_extension(content: bytes) -> str: + """ + Return the attachment extension for bytes returned by the XLSX export endpoint. + """ + try: + with ZipFile(BytesIO(content)) as zip_file: + names = zip_file.namelist() + if _is_xlsx_zip(names): + return "xlsx" + + files = [name for name in names if not name.endswith("/")] + if files and all(name.lower().endswith(".xlsx") for name in files): + for name in files: + with zip_file.open(name) as xlsx_file: + if not _has_zip_signature(xlsx_file): + return "xlsx" + return "zip" + except BadZipFile: + return "xlsx" + + return "xlsx" + + +def _is_xlsx_zip(names: list[str]) -> bool: + return "[Content_Types].xml" in names and "xl/workbook.xml" in names + + +def _has_zip_signature(content: IO[bytes]) -> bool: + return content.read(len(ZIP_LOCAL_FILE_HEADER)) == ZIP_LOCAL_FILE_HEADER + + class EmailNotification(BaseNotification): # pylint: disable=too-few-public-methods """ Sends an email notification for a report recipient @@ -207,11 +241,18 @@ class EmailNotification(BaseNotification): # pylint: disable=too-few-public-met ) # 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: dict[str, bytes] | None = None + attachment_data: dict[str, bytes | str] | None = None if self._content.csv: attachment_data = {__("%(name)s.csv", name=self._name): self._content.csv} elif self._content.xlsx: - attachment_data = {__("%(name)s.xlsx", name=self._name): self._content.xlsx} + extension = _get_xlsx_attachment_extension(self._content.xlsx) + attachment_data = { + __( + "%(name)s.%(extension)s", + name=self._name, + extension=extension, + ): self._content.xlsx + } pdf_data = None if self._content.pdf: diff --git a/superset/translations/messages.pot b/superset/translations/messages.pot index 196cfbdcc8a..f3c9ea83989 100644 --- a/superset/translations/messages.pot +++ b/superset/translations/messages.pot @@ -209,6 +209,10 @@ msgstr "" msgid "%(label)s file" msgstr "" +#, python-format +msgid "%(name)s.%(extension)s" +msgstr "" + #, python-format msgid "%(name)s.csv" msgstr "" @@ -894,6 +898,9 @@ msgstr "" msgid "A timeout occurred while generating a dataframe." msgstr "" +msgid "A timeout occurred while generating an Excel file." +msgstr "" + msgid "A timeout occurred while taking a screenshot." msgstr "" @@ -6485,6 +6492,9 @@ msgstr "" msgid "Formatted CSV attached in email" msgstr "" +msgid "Formatted Excel attached in email" +msgstr "" + msgid "Formatted value" msgstr "" @@ -9987,6 +9997,9 @@ msgstr "" msgid "Report Schedule execution failed when generating a screenshot." msgstr "" +msgid "Report Schedule execution failed when generating an Excel file." +msgstr "" + msgid "Report Schedule execution got an unexpected error." msgstr "" @@ -11162,6 +11175,9 @@ msgstr "" msgid "Send as CSV" msgstr "" +msgid "Send as Excel" +msgstr "" + msgid "Send as PDF" msgstr "" diff --git a/superset/translations/ru/LC_MESSAGES/messages.po b/superset/translations/ru/LC_MESSAGES/messages.po index fe1767936b4..d11d40021c5 100644 --- a/superset/translations/ru/LC_MESSAGES/messages.po +++ b/superset/translations/ru/LC_MESSAGES/messages.po @@ -270,6 +270,10 @@ msgstr "" msgid "%(label)s file" msgstr "Файл %(label)s" +#, python-format +msgid "%(name)s.%(extension)s" +msgstr "%(name)s.%(extension)s" + #, python-format msgid "%(name)s.csv" msgstr "%(name)s.csv" @@ -1042,6 +1046,9 @@ msgstr "Превышен таймаут создания файла CSV" msgid "A timeout occurred while generating a dataframe." msgstr "Превышен таймаут создания датафрейма" +msgid "A timeout occurred while generating an Excel file." +msgstr "Превышен таймаут создания файла Excel" + msgid "A timeout occurred while taking a screenshot." msgstr "Превышен таймаут создания скриншота" @@ -7155,6 +7162,9 @@ msgstr "" msgid "Formatted CSV attached in email" msgstr "Форматированный CSV, прикрепленный к письму" +msgid "Formatted Excel attached in email" +msgstr "Форматированный Excel во вложении к письму" + msgid "Formatted value" msgstr "Форматированное значение" @@ -10934,6 +10944,11 @@ msgstr "" "Произошла ошибка при создании скриншота для отправки оповещения по " "расписанию" +msgid "Report Schedule execution failed when generating an Excel file." +msgstr "" +"Произошла ошибка при создании файла Excel для отправки оповещения по " +"расписанию" + msgid "Report Schedule execution got an unexpected error." msgstr "Произошла неожиданная ошибка при отправке оповещения по расписанию" @@ -12246,6 +12261,9 @@ msgstr "Семантические представления" msgid "Send as CSV" msgstr "Отправить как CSV" +msgid "Send as Excel" +msgstr "Отправить как Excel" + msgid "Send as PDF" msgstr "Отправить как PDF" diff --git a/superset/utils/core.py b/superset/utils/core.py index 689ef302800..cdeba702802 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -120,6 +120,28 @@ if TYPE_CHECKING: logging.getLogger("MARKDOWN").setLevel(logging.INFO) logger = logging.getLogger(__name__) +EMAIL_ATTACHMENT_SUBTYPES: dict[str, str] = { + ".pdf": "pdf", + ".zip": "zip", + ".xlsx": "vnd.openxmlformats-officedocument.spreadsheetml.sheet", +} + + +def build_email_attachment(name: str, body: bytes | str) -> MIMEApplication: + """ + Create an email attachment part with stable filename metadata. + """ + subtype = EMAIL_ATTACHMENT_SUBTYPES.get(os.path.splitext(name)[1].lower()) + payload = body.encode("utf-8") if isinstance(body, str) else body + attachment = MIMEApplication( + payload, + _subtype=subtype or "octet-stream", + Name=name, + ) + attachment.add_header("Content-Disposition", "attachment", filename=name) + return attachment + + DTTM_ALIAS = "__timestamp" TIME_COMPARISON = "__" @@ -830,7 +852,7 @@ def send_email_smtp( # pylint: disable=invalid-name,too-many-arguments,too-many html_content: str, config: dict[str, Any], files: list[str] | None = None, - data: dict[str, str] | None = None, + data: dict[str, bytes | str] | None = None, pdf: dict[str, bytes] | None = None, images: dict[str, bytes] | None = None, dryrun: bool = False, @@ -877,30 +899,14 @@ def send_email_smtp( # pylint: disable=invalid-name,too-many-arguments,too-many for fname in files or []: basename = os.path.basename(fname) with open(fname, "rb") as f: - msg.attach( - MIMEApplication( - f.read(), - Content_Disposition=f"attachment; filename='{basename}'", - Name=basename, - ) - ) + msg.attach(build_email_attachment(basename, f.read())) # Attach any files passed directly for name, body in (data or {}).items(): - msg.attach( - MIMEApplication( - body, Content_Disposition=f"attachment; filename='{name}'", Name=name - ) - ) + msg.attach(build_email_attachment(name, body)) for name, body_pdf in (pdf or {}).items(): - msg.attach( - MIMEApplication( - body_pdf, - Content_Disposition=f"attachment; filename='{name}'", - Name=name, - ) - ) + msg.attach(build_email_attachment(name, body_pdf)) # Attach any inline images, which may be required for display in # HTML content (inline) diff --git a/tests/unit_tests/charts/test_client_processing.py b/tests/unit_tests/charts/test_client_processing.py index 376289b6e42..600c7cf4c7f 100644 --- a/tests/unit_tests/charts/test_client_processing.py +++ b/tests/unit_tests/charts/test_client_processing.py @@ -15,6 +15,7 @@ # specific language governing permissions and limitations # under the License. +from io import BytesIO import pandas as pd import pytest @@ -23,6 +24,7 @@ from sqlalchemy.orm.session import Session from superset.charts.client_processing import apply_client_processing, pivot_df, table from superset.common.chart_data import ChartDataResultFormat +from superset.utils import excel from superset.utils.core import GenericDataType from tests.conftest import with_config @@ -2836,6 +2838,100 @@ def test_apply_client_processing_csv_format_default_na_behavior(): ) # Second data row should have empty last_name (NA converted to null) +def _assert_xlsx_client_processing(index: bool) -> None: + """Assert XLSX post-processing preserves columns and configured index.""" + source_df = pd.DataFrame( + { + "city": ["Paris", "London"], + "value": [10, 20], + }, + index=pd.Index(["row-1", "row-2"], name="row"), + ) + result = { + "queries": [ + { + "result_format": ChartDataResultFormat.XLSX, + "data": excel.df_to_excel(source_df, index=index), + } + ] + } + form_data = { + "viz_type": "table", + "columns": ["city", "value"], + "metrics": [], + } + + processed_result = apply_client_processing(result, form_data) + query = processed_result["queries"][0] + output_df = pd.read_excel( + BytesIO(query["data"]), + index_col=0 if index else None, + ) + expected_df = source_df if index else source_df.reset_index(drop=True) + + pd.testing.assert_frame_equal(output_df, expected_df, check_names=False) + assert query["colnames"] == ["city", "value"] + if index: + assert query["indexnames"] == ["row-1", "row-2"] + else: + assert query["indexnames"] == [0, 1] + assert query["rowcount"] == 2 + + +@with_config({"EXCEL_EXPORT": {"index": True}}) +def test_apply_client_processing_xlsx_format_with_index() -> None: + """XLSX post-processing should preserve an exported index.""" + _assert_xlsx_client_processing(index=True) + + +@with_config({"EXCEL_EXPORT": {"index": False}}) +def test_apply_client_processing_xlsx_format_without_index() -> None: + """XLSX post-processing should not shift columns when index is omitted.""" + _assert_xlsx_client_processing(index=False) + + +@with_config({"EXCEL_EXPORT": {}}) +def test_apply_client_processing_xlsx_format_without_index_default_config() -> None: + """XLSX post-processing derives omitted index from the payload.""" + _assert_xlsx_client_processing(index=False) + + +@with_config({"EXCEL_EXPORT": {"index": False}}) +def test_apply_client_processing_xlsx_format_pivot_table_groupby_columns() -> None: + """XLSX post-processing should preserve pivot groupby columns.""" + source_df = pd.DataFrame( + { + "city": ["Paris", "Paris", "London"], + "segment": ["Consumer", "Corporate", "Consumer"], + "value": [10, 20, 30], + }, + ) + result = { + "queries": [ + { + "result_format": ChartDataResultFormat.XLSX, + "data": excel.df_to_excel(source_df, index=False), + } + ] + } + form_data = { + "viz_type": "pivot_table_v2", + "groupbyColumns": ["segment"], + "groupbyRows": ["city"], + "metrics": ["value"], + } + + processed_result = apply_client_processing(result, form_data) + query = processed_result["queries"][0] + output_df = pd.read_excel(BytesIO(query["data"]), index_col=0) + + assert query["rowcount"] == 2 + assert query["indexnames"] == [("London",), ("Paris",)] + assert set(output_df.index) == {"London", "Paris"} + assert "value Consumer" in output_df.columns + assert "value Corporate" in output_df.columns + + @with_config({"CSV_EXPORT": {"sep": ";", "decimal": ","}}) def test_apply_client_processing_csv_format_custom_delimiter(): """ diff --git a/tests/unit_tests/commands/report/execute_test.py b/tests/unit_tests/commands/report/execute_test.py index 232d6ab48a0..71bdfe911a1 100644 --- a/tests/unit_tests/commands/report/execute_test.py +++ b/tests/unit_tests/commands/report/execute_test.py @@ -23,6 +23,7 @@ from urllib.error import URLError from uuid import UUID, uuid4 import pytest +from celery.exceptions import SoftTimeLimitExceeded from pytest_mock import MockerFixture from superset.app import SupersetApp @@ -1326,6 +1327,197 @@ def test_get_csv_data_keeps_screenshot_fallback_without_query_context( post_chart_data.assert_not_called() +def test_get_url_for_xlsx_report(mocker: MockerFixture) -> None: + """XLSX reports should request post-processed chart data.""" + report_schedule = create_report_schedule(mocker) + report_schedule.chart_id = 1 + report_schedule.force_screenshot = False + report_state = BaseReportState( + report_schedule, "January 1, 2021", "execution_id_example" + ) + get_url_path = mocker.patch( + "superset.commands.report.execute.get_url_path", + return_value="/api/v1/chart/1/data/xlsx", + ) + + url = report_state._get_url(result_format=ChartDataResultFormat.XLSX) + + assert url == "/api/v1/chart/1/data/xlsx" + get_url_path.assert_called_once_with( + "ChartDataRestApi.get_data", + pk=1, + format=ChartDataResultFormat.XLSX.value, + type=ChartDataResultType.POST_PROCESSED.value, + force="false", + ) + + +def test_get_chart_data_rejects_non_table_format(mocker: MockerFixture) -> None: + """Chart data retrieval should reject formats it cannot download.""" + report_state = BaseReportState( + create_report_schedule(mocker), + "January 1, 2021", + "execution_id_example", + ) + get_url = mocker.patch.object(report_state, "_get_url") + + with pytest.raises( + ReportScheduleExecuteUnexpectedError, + match="Unsupported chart data result format: json", + ): + report_state._get_data(ChartDataResultFormat.JSON) + + get_url.assert_not_called() + + +def _mock_xlsx_chart_data_dependencies( + mocker: MockerFixture, + report_state: BaseReportState, +) -> tuple[MagicMock, dict[str, str]]: + """Mock external services used by the chart data download path.""" + report_state._report_schedule.chart.query_context = None + mocker.patch.object(report_state, "_update_query_context") + mocker.patch("superset.commands.report.execute.db.session.refresh") + get_url = mocker.patch.object( + report_state, + "_get_url", + return_value="/api/v1/chart/1/data/xlsx", + ) + mocker.patch( + "superset.commands.report.execute.get_executor", + return_value=(None, "report_executor"), + ) + user = mocker.MagicMock(username="report_executor") + mocker.patch( + "superset.commands.report.execute.security_manager.find_user", + return_value=user, + ) + auth_cookies = {"session": "cookie"} + auth_provider = mocker.patch( + "superset.commands.report.execute.machine_auth_provider_factory" + ) + auth_provider.instance.get_auth_cookies.return_value = auth_cookies + return get_url, auth_cookies + + +def test_get_data_xlsx_fetches_chart_data( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + """XLSX report data should be fetched through the chart data endpoint.""" + report_state = BaseReportState( + create_report_schedule(mocker), + "January 1, 2021", + "execution_id_example", + ) + get_url, auth_cookies = _mock_xlsx_chart_data_dependencies(mocker, report_state) + get_chart_csv_data = mocker.patch( + "superset.commands.report.execute.get_chart_csv_data", + return_value=b"xlsx-data", + ) + + assert report_state._get_data(ChartDataResultFormat.XLSX) == b"xlsx-data" + get_url.assert_called_once_with(result_format=ChartDataResultFormat.XLSX) + get_chart_csv_data.assert_called_once_with( + chart_url="/api/v1/chart/1/data/xlsx", + auth_cookies=auth_cookies, + timeout=app.config["ALERT_REPORTS_CSV_REQUEST_TIMEOUT"], + ) + + +@pytest.mark.parametrize( + ("side_effect", "expected_exception", "expected_message"), + [ + ( + SoftTimeLimitExceeded(), + ReportScheduleXlsxTimeout, + "timeout occurred while generating an Excel file", + ), + ( + RuntimeError("export failed"), + ReportScheduleXlsxFailedError, + "Failed generating excel export failed", + ), + ], +) +def test_get_data_xlsx_maps_errors( + app: SupersetApp, + mocker: MockerFixture, + side_effect: Exception, + expected_exception: type[Exception], + expected_message: str, +) -> None: + """XLSX generation errors should use XLSX-specific report exceptions.""" + report_state = BaseReportState( + create_report_schedule(mocker), + "January 1, 2021", + "execution_id_example", + ) + _mock_xlsx_chart_data_dependencies(mocker, report_state) + mocker.patch( + "superset.commands.report.execute.get_chart_csv_data", + side_effect=side_effect, + ) + + with pytest.raises(expected_exception, match=expected_message) as exc_info: + report_state._get_data(ChartDataResultFormat.XLSX) + + assert exc_info.value.__cause__ is side_effect + + +def test_get_data_xlsx_rejects_empty_result( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + """An empty XLSX response should fail report generation.""" + report_state = BaseReportState( + create_report_schedule(mocker), + "January 1, 2021", + "execution_id_example", + ) + _mock_xlsx_chart_data_dependencies(mocker, report_state) + mocker.patch( + "superset.commands.report.execute.get_chart_csv_data", + return_value=None, + ) + + with pytest.raises( + ReportScheduleXlsxFailedError, + match="Report Schedule execution failed when generating an Excel file", + ) as exc_info: + report_state._get_data(ChartDataResultFormat.XLSX) + + assert exc_info.value.__cause__ is None + + +def test_notification_content_contains_xlsx(mocker: MockerFixture) -> None: + """XLSX chart reports should populate the XLSX notification field.""" + report_schedule = create_report_schedule(mocker) + report_schedule.report_format = ReportDataFormat.XLSX + report_schedule.force_screenshot = False + report_schedule.email_subject = None + report_schedule.owners = [] + report_schedule.recipients = [] + report_state = BaseReportState( + report_schedule, + "January 1, 2021", + "execution_id_example", + ) + mocker.patch.object(report_state, "_get_url", return_value="/chart/1") + mocker.patch.object(report_state, "_get_log_data", return_value={}) + get_data = mocker.patch.object( + report_state, + "_get_data", + return_value=b"xlsx-data", + ) + + content = report_state._get_notification_content() + + assert content.xlsx == b"xlsx-data" + assert content.csv is None + get_data.assert_called_once_with(ChartDataResultFormat.XLSX) + + @pytest.mark.parametrize( "test_id,custom_width,max_width,window_width,expected_width", [ @@ -1956,6 +2148,22 @@ def test_get_notification_content_xlsx_format( assert content.csv is None +@patch("superset.commands.report.execute.feature_flag_manager") +def test_get_notification_content_xlsx_rejects_dashboard( + mock_ff, mocker: MockerFixture +) -> None: + mock_ff.is_feature_enabled.return_value = False + state = _make_notification_state( + mocker, report_format=ReportDataFormat.XLSX, has_chart=False + ) + + with pytest.raises( + ReportScheduleXlsxFailedError, + match="XLSX reports are only supported for chart schedules", + ): + state._get_notification_content() + + @patch("superset.commands.report.execute.feature_flag_manager") def test_get_notification_content_text_format(mock_ff, mocker: MockerFixture) -> None: import pandas as pd diff --git a/tests/unit_tests/reports/notifications/email_tests.py b/tests/unit_tests/reports/notifications/email_tests.py index 04676d3389e..2763519403e 100644 --- a/tests/unit_tests/reports/notifications/email_tests.py +++ b/tests/unit_tests/reports/notifications/email_tests.py @@ -15,13 +15,19 @@ # specific language governing permissions and limitations # under the License. from datetime import datetime +from typing import TYPE_CHECKING +from zipfile import ZipExtFile import pandas as pd +import pytest from freezegun import freeze_time from pytz import timezone from tests.unit_tests.conftest import with_feature_flags +if TYPE_CHECKING: + from superset.reports.notifications.email import EmailNotification + def test_render_description_with_html() -> None: # `superset.models.helpers`, a dependency of following imports, @@ -146,17 +152,17 @@ def test_email_subject_with_datetime() -> None: assert frozen_now.strftime(datetime_pattern) in subject -def test_email_content_with_xlsx_attachment() -> None: - """Email content attaches xlsx bytes under an ``.xlsx`` filename.""" - # `superset.models.helpers`, a dependency of following imports, - # requires app context +def _make_notification(xlsx: bytes) -> "EmailNotification": + """Build an email notification for attachment tests.""" from superset.reports.models import ReportRecipients, ReportRecipientType from superset.reports.notifications.base import NotificationContent from superset.reports.notifications.email import EmailNotification + recipient = ReportRecipients(type=ReportRecipientType.EMAIL) content = NotificationContent( name="test report", - xlsx=b"xlsx_content", + text=None, + xlsx=xlsx, header_data={ "notification_format": "XLSX", "notification_type": "Report", @@ -168,11 +174,97 @@ def test_email_content_with_xlsx_attachment() -> None: "execution_id": "test-execution-id", }, ) - email_content = EmailNotification( - recipient=ReportRecipients(type=ReportRecipientType.EMAIL), content=content - )._get_content() + return EmailNotification(recipient=recipient, content=content) - assert email_content.data is not None - attachment_name = list(email_content.data.keys())[0] - assert attachment_name.endswith(".xlsx") - assert email_content.data[attachment_name] == b"xlsx_content" + +def test_get_xlsx_attachment_extension_for_non_zip_content() -> None: + """Non-ZIP responses retain the XLSX extension.""" + from superset.reports.notifications.email import ( + _get_xlsx_attachment_extension, + ) + + assert _get_xlsx_attachment_extension(b"xlsx-response") == "xlsx" + + +def test_get_xlsx_attachment_extension_for_xlsx_content() -> None: + """An OOXML workbook is identified as XLSX.""" + from superset.reports.notifications.email import ( + _get_xlsx_attachment_extension, + ) + from superset.utils import excel + + xlsx = excel.df_to_excel(pd.DataFrame({"value": [1, 2]}), index=False) + + assert _get_xlsx_attachment_extension(xlsx) == "xlsx" + + +def test_get_xlsx_attachment_extension_for_invalid_xlsx_zip_content() -> None: + """A ZIP with invalid XLSX entries is not identified as a report archive.""" + from superset.reports.notifications.email import ( + _get_xlsx_attachment_extension, + ) + from superset.utils.core import create_zip + + archive = create_zip({"query_1.xlsx": b"xlsx-response"}).getvalue() + + assert _get_xlsx_attachment_extension(archive) == "xlsx" + + +def test_get_xlsx_attachment_extension_reads_nested_zip_signatures_only( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Nested XLSX detection should not read each workbook into memory.""" + from superset.reports.notifications.email import ( + _get_xlsx_attachment_extension, + ) + from superset.utils import excel + from superset.utils.core import create_zip + + read_sizes: list[int] = [] + original_read = ZipExtFile.read + + def read_signature(self: ZipExtFile, n: int = -1) -> bytes: + read_sizes.append(n) + return original_read(self, n) + + monkeypatch.setattr(ZipExtFile, "read", read_signature) + + xlsx = excel.df_to_excel(pd.DataFrame({"value": [1, 2]}), index=False) + archive = create_zip({"query_1.xlsx": xlsx, "query_2.xlsx": xlsx}).getvalue() + + assert _get_xlsx_attachment_extension(archive) == "zip" + assert read_sizes == [4, 4] + + +@pytest.mark.parametrize( + ("server_pagination", "expected_extension"), + [ + (False, "xlsx"), + (True, "zip"), + ], +) +def test_xlsx_report_attachment_extension( + server_pagination: bool, + expected_extension: str, +) -> None: + """Server-paginated XLSX reports should be attached as ZIP archives.""" + from superset.utils import excel + from superset.utils.core import create_zip + + xlsx = excel.df_to_excel(pd.DataFrame({"value": [1, 2]}), index=False) + attachment = ( + create_zip( + { + "query_1.xlsx": xlsx, + "query_2.xlsx": xlsx, + } + ).getvalue() + if server_pagination + else xlsx + ) + + email_content = _make_notification(xlsx=attachment)._get_content() + + assert email_content.data == { + f"test report.{expected_extension}": attachment, + } diff --git a/tests/unit_tests/utils/test_core.py b/tests/unit_tests/utils/test_core.py index 5285ffb998b..9a33ee55ed4 100644 --- a/tests/unit_tests/utils/test_core.py +++ b/tests/unit_tests/utils/test_core.py @@ -28,6 +28,7 @@ from pytest_mock import MockerFixture from superset.exceptions import SupersetException from superset.utils.core import ( + build_email_attachment, cast_to_boolean, check_is_safe_zip, DateColumn, @@ -70,6 +71,43 @@ EXTRA_FILTER: QueryObjectFilterClause = { } +@pytest.mark.parametrize( + ("name", "body", "expected_payload", "content_type"), + [ + ( + "report.xlsx", + b"attachment", + b"attachment", + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + ), + ( + "report.zip", + "архив".encode("utf-8"), + "архив".encode("utf-8"), + "application/zip", + ), + ( + "report.csv", + "город,value\nМосква,1", + "город,value\nМосква,1".encode("utf-8"), + "application/octet-stream", + ), + ], +) +def test_build_email_attachment( + name: str, + body: bytes | str, + expected_payload: bytes, + content_type: str, +) -> None: + """Email attachments should expose the expected MIME type and filename.""" + attachment = build_email_attachment(name, body) + + assert attachment.get_content_type() == content_type + assert attachment.get_filename() == name + assert attachment.get_payload(decode=True) == expected_payload + + @dataclass class MockZipInfo: file_size: int