betodealmeida commented on code in PR #41133: URL: https://github.com/apache/superset/pull/41133#discussion_r3455686645
########## superset/utils/excel_streaming.py: ########## @@ -0,0 +1,190 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Streaming, constant-memory XLSX writer for multi-sheet dashboard exports. + +Unlike :mod:`superset.utils.excel`, which materializes a whole DataFrame in +memory, this writer streams rows one at a time into an ``xlsxwriter`` workbook +opened in ``constant_memory`` mode, so a dashboard with many large charts never +holds more than one row per sheet in memory at once. Review Comment: I don't think this is true — we're still loading all the chart data into memory at once, since it comes from the JSON response. We iterate over it row-by-row, but the data is already there. ########## superset/dashboards/excel_export/email.py: ########## @@ -0,0 +1,113 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Email rendering and delivery for dashboard Excel exports. + +Bodies use inline styles only (no external CSS, no logo) to match Superset's +existing report notification emails, and all user-controlled values (dashboard +title, chart names) are HTML-escaped to avoid injection. +""" + +from __future__ import annotations + +from datetime import datetime + +from flask import current_app +from markupsafe import escape + +from superset.utils.core import send_email_smtp + +_DATETIME_FORMAT = "%Y-%m-%d %H:%M:%S" +_FOOTER_STYLE = "color:#888;font-size:12px;" +_BUTTON_STYLE = ( + "display:inline-block;padding:10px 16px;background:#20a7c9;color:#ffffff;" + "text-decoration:none;border-radius:4px;" +) + + +def _fmt(dt: datetime) -> str: + return dt.strftime(_DATETIME_FORMAT) + + +def build_subject(dashboard_title: str, *, success: bool) -> str: + """Build the email subject, prefixed with EMAIL_REPORTS_SUBJECT_PREFIX.""" + prefix = current_app.config["EMAIL_REPORTS_SUBJECT_PREFIX"] + if success: + return f"{prefix}Your dashboard export is ready: {dashboard_title}" + return f"{prefix}Your dashboard export could not be completed: {dashboard_title}" + + +def _skipped_section(skipped_charts: list[str]) -> str: + if not skipped_charts: + return "" + items = "".join(f"<li>{escape(label)}</li>" for label in skipped_charts) + return ( + "<p>Note: the following charts were omitted because they have no saved " + "query context. To include them, open each chart in Explore and re-save." + f"</p><ul>{items}</ul>" + ) + + +def build_success_email( + dashboard_title: str, + download_url: str, + requested_at: datetime, + expires_at: datetime, + ttl_seconds: int, + skipped_charts: list[str], +) -> str: + """Render the success email body (HTML).""" + title = escape(dashboard_title) + url = escape(download_url) + hours = ttl_seconds // 3600 + return ( + '<html><body style="font-family:Arial,sans-serif;color:#333;">' + f'<p>Your export of "{title}" is ready.</p>' + f'<p><a href="{url}" style="{_BUTTON_STYLE}">Download Excel file</a></p>' + f"<p>This link expires in {hours} hours ({_fmt(expires_at)} UTC).</p>" + f"{_skipped_section(skipped_charts)}" + "<hr/>" + f'<p style="{_FOOTER_STYLE}">This export was requested on ' + f"{_fmt(requested_at)} UTC.<br/>" + "If you did not request this, you can ignore this email.</p>" + "</body></html>" + ) + + +def build_failure_email(dashboard_title: str, requested_at: datetime) -> str: + """Render the failure email body (HTML).""" + title = escape(dashboard_title) + return ( + '<html><body style="font-family:Arial,sans-serif;color:#333;">' Review Comment: Let's translate all these messages. ########## superset/config.py: ########## @@ -1302,6 +1302,21 @@ class D3TimeFormat(TypedDict, total=False): # note: index option should not be overridden EXCEL_EXPORT: dict[str, Any] = {} +# --------------------------------------------------- +# Dashboard "Export Data to Excel" (async, S3-backed) +# --------------------------------------------------- +# Destination S3 bucket for generated dashboard .xlsx exports. The feature is +# disabled until this is set: the export endpoint returns 501 when it is None. +EXCEL_EXPORT_S3_BUCKET: str | None = None +# Key prefix for export objects: {prefix}{dashboard_id}/{job_id}.xlsx +EXCEL_EXPORT_S3_KEY_PREFIX = "dashboard-exports/" +# Lifetime (seconds) of the pre-signed download URL emailed to the user (24h). +EXCEL_EXPORT_LINK_TTL_SECONDS = 86400 +# Extra kwargs passed to boto3.client("s3", ...) — e.g. region_name, or an +# endpoint_url for S3-compatible stores (MinIO/LocalStack). Credentials +# otherwise resolve through the standard boto3 chain. +EXCEL_EXPORT_S3_CLIENT_KWARGS: dict[str, Any] = {} Review Comment: I wonder if we should keep this more generic, in case we decide in the future to support other formats. Maybe we drop the `EXCEL_` prefix? ########## superset/dashboards/excel_export/email.py: ########## @@ -0,0 +1,113 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Email rendering and delivery for dashboard Excel exports. + +Bodies use inline styles only (no external CSS, no logo) to match Superset's +existing report notification emails, and all user-controlled values (dashboard +title, chart names) are HTML-escaped to avoid injection. +""" + +from __future__ import annotations + +from datetime import datetime + +from flask import current_app +from markupsafe import escape + +from superset.utils.core import send_email_smtp + +_DATETIME_FORMAT = "%Y-%m-%d %H:%M:%S" +_FOOTER_STYLE = "color:#888;font-size:12px;" +_BUTTON_STYLE = ( + "display:inline-block;padding:10px 16px;background:#20a7c9;color:#ffffff;" + "text-decoration:none;border-radius:4px;" +) + + +def _fmt(dt: datetime) -> str: + return dt.strftime(_DATETIME_FORMAT) + + +def build_subject(dashboard_title: str, *, success: bool) -> str: + """Build the email subject, prefixed with EMAIL_REPORTS_SUBJECT_PREFIX.""" + prefix = current_app.config["EMAIL_REPORTS_SUBJECT_PREFIX"] + if success: + return f"{prefix}Your dashboard export is ready: {dashboard_title}" + return f"{prefix}Your dashboard export could not be completed: {dashboard_title}" + + +def _skipped_section(skipped_charts: list[str]) -> str: + if not skipped_charts: + return "" + items = "".join(f"<li>{escape(label)}</li>" for label in skipped_charts) + return ( + "<p>Note: the following charts were omitted because they have no saved " + "query context. To include them, open each chart in Explore and re-save." + f"</p><ul>{items}</ul>" + ) + + +def build_success_email( + dashboard_title: str, + download_url: str, + requested_at: datetime, + expires_at: datetime, + ttl_seconds: int, + skipped_charts: list[str], +) -> str: + """Render the success email body (HTML).""" + title = escape(dashboard_title) + url = escape(download_url) + hours = ttl_seconds // 3600 + return ( + '<html><body style="font-family:Arial,sans-serif;color:#333;">' + f'<p>Your export of "{title}" is ready.</p>' + f'<p><a href="{url}" style="{_BUTTON_STYLE}">Download Excel file</a></p>' + f"<p>This link expires in {hours} hours ({_fmt(expires_at)} UTC).</p>" Review Comment: I agree, let's have a helper function that can convert the TTL into a human readable delta ("1 hours, 15 minutes") — we might have one already in the codebase. ########## superset/utils/s3.py: ########## @@ -0,0 +1,72 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Minimal S3 helpers for uploading export artifacts and minting pre-signed URLs. + +Credentials and region come from the standard boto3 resolution chain (env vars, +shared config, instance role). Operators can override client construction via +the ``EXCEL_EXPORT_S3_CLIENT_KWARGS`` config (e.g. ``region_name`` or an +``endpoint_url`` for S3-compatible stores such as MinIO/LocalStack). +""" + +from __future__ import annotations + +import logging +from typing import Any + +import boto3 Review Comment: I don't think we have a direct dependency on `boto3`, only indirect. We should add it to `pyproject.toml`. ########## superset/tasks/export_dashboard_excel.py: ########## @@ -0,0 +1,246 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Celery task that exports every chart on a dashboard to a single multi-sheet +``.xlsx`` file, uploads it to S3, and emails the requesting user a pre-signed +download link. + +The task re-runs each chart's saved query context under the requesting user, +applies the live dashboard filter state, and streams the results row-by-row into +a constant-memory workbook so large dashboards never load all data at once. +""" + +from __future__ import annotations + +import logging +import os +import tempfile +from datetime import datetime, timedelta +from typing import Any + +from celery.exceptions import SoftTimeLimitExceeded +from flask import current_app, g + +from superset import db, security_manager +from superset.charts.data.dashboard_filter_context import ( + apply_dashboard_filter_context, + get_dashboard_filter_context, +) +from superset.charts.schemas import ChartDataQueryContextSchema +from superset.commands.chart.data.get_data_command import ChartDataCommand +from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType +from superset.dashboards.excel_export import email +from superset.dashboards.excel_export.layout import get_charts_in_layout_order +from superset.extensions import celery_app +from superset.utils import json, s3 +from superset.utils.core import override_user +from superset.utils.excel_streaming import StreamingXlsxWriter + +logger = logging.getLogger(__name__) + + +def _chart_label(chart: Any) -> str: + """Human-readable label for a chart in the skipped-charts list.""" + return f"{chart.id} - {chart.slice_name or ''}".strip() + + +def _record_to_row(record: dict[str, Any], colnames: list[str]) -> list[Any]: + return [record.get(col) for col in colnames] + + +def _write_chart_sheets( + writer: StreamingXlsxWriter, + chart: Any, + dashboard_id: int, + active_data_mask: dict[str, Any], +) -> None: + """ + Run a single chart's query and stream its result(s) into the workbook. + + Charts may yield more than one query (e.g. mixed-series charts); each becomes + its own sheet. Raises if the chart cannot be exported, so the caller can skip + it and note it in the email. + """ + json_body = json.loads(chart.query_context) + # Override any stale saved values: we always want full JSON results. + json_body["result_format"] = ChartDataResultFormat.JSON + json_body["result_type"] = ChartDataResultType.FULL + json_body.pop("force", None) + + filter_context = get_dashboard_filter_context( + dashboard_id=dashboard_id, + chart_id=chart.id, + active_data_mask=active_data_mask, + ) + if filter_context.extra_form_data: + apply_dashboard_filter_context(json_body, filter_context.extra_form_data) + + # Jinja macros resolve form data from g.form_data; expose the saved context. + g.form_data = json_body + + query_context = ChartDataQueryContextSchema().load(json_body) + command = ChartDataCommand(query_context) + command.validate() + result = command.run() + + for index, query in enumerate(result["queries"]): + colnames = query.get("colnames") or [] + data = query.get("data") or [] + if index == 0: + name = f"{chart.id} - {chart.slice_name or ''}" + else: + name = f"{chart.id}.{index} - {chart.slice_name or ''}" + writer.add_sheet( + name, + colnames, + (_record_to_row(record, colnames) for record in data), + ) + + +def _build_workbook( + path: str, + dashboard: Any, + active_data_mask: dict[str, Any], + job_id: str, +) -> list[str]: + """Build the workbook on disk; return the list of skipped chart labels.""" + skipped: list[str] = [] Review Comment: We could rename this to `errorred: dict[str, list[str]]`, and we would store the reason, eg: ```python { "no-query-context": ["foo", "bar"], "timeout": ["baz"], "general-exception": [], } ``` ########## tests/integration_tests/dashboards/api_tests.py: ########## @@ -725,6 +725,7 @@ def test_info_security_dashboard(self): "can_write", "can_export", "can_export_as_example", + "can_export_xlsx", Review Comment: Shouldn't this be `can_export`, since we rename the endpoint permission name? ########## superset/tasks/export_dashboard_excel.py: ########## @@ -0,0 +1,247 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Celery task that exports every chart on a dashboard to a single multi-sheet +``.xlsx`` file, uploads it to S3, and emails the requesting user a pre-signed +download link. + +The task re-runs each chart's saved query context under the requesting user, +applies the live dashboard filter state, and streams the results row-by-row into +a constant-memory workbook so large dashboards never load all data at once. +""" + +from __future__ import annotations + +import logging +import os +import tempfile +from datetime import datetime, timedelta +from typing import Any + +from celery.exceptions import SoftTimeLimitExceeded +from flask import current_app, g + +from superset import db, security_manager +from superset.charts.data.dashboard_filter_context import ( + apply_extra_form_data_to_query_context_json, + get_dashboard_filter_context, +) +from superset.charts.schemas import ChartDataQueryContextSchema +from superset.commands.chart.data.get_data_command import ChartDataCommand +from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType +from superset.dashboards.excel_export import email +from superset.dashboards.excel_export.layout import get_charts_in_layout_order +from superset.extensions import celery_app +from superset.utils import json, s3 +from superset.utils.core import override_user +from superset.utils.excel_streaming import StreamingXlsxWriter + +logger = logging.getLogger(__name__) + + +def _chart_label(chart: Any) -> str: + """Human-readable label for a chart in the skipped-charts list.""" + return f"{chart.id} - {chart.slice_name or ''}".strip() + + +def _record_to_row(record: dict[str, Any], colnames: list[str]) -> list[Any]: + return [record.get(col) for col in colnames] + + +def _write_chart_sheets( + writer: StreamingXlsxWriter, + chart: Any, + dashboard_id: int, + active_data_mask: dict[str, Any], +) -> None: + """ + Run a single chart's query and stream its result(s) into the workbook. + + Charts may yield more than one query (e.g. mixed-series charts); each becomes + its own sheet. Raises if the chart cannot be exported, so the caller can skip + it and note it in the email. + """ + json_body = json.loads(chart.query_context) + # Override any stale saved values: we always want full JSON results. + json_body["result_format"] = ChartDataResultFormat.JSON + json_body["result_type"] = ChartDataResultType.FULL + json_body.pop("force", None) + + filter_context = get_dashboard_filter_context( + dashboard_id=dashboard_id, + chart_id=chart.id, + active_data_mask=active_data_mask, + ) + apply_extra_form_data_to_query_context_json( + json_body, filter_context.extra_form_data + ) + + # Jinja macros resolve form data from g.form_data; expose the saved context. + g.form_data = json_body + + query_context = ChartDataQueryContextSchema().load(json_body) + command = ChartDataCommand(query_context) + command.validate() + result = command.run() + + for index, query in enumerate(result["queries"]): + colnames = query.get("colnames") or [] + data = query.get("data") or [] + if index == 0: + name = f"{chart.id} - {chart.slice_name or ''}" + else: + name = f"{chart.id}.{index} - {chart.slice_name or ''}" + writer.add_sheet( + name, + colnames, + (_record_to_row(record, colnames) for record in data), + ) + + +def _build_workbook( + path: str, + dashboard: Any, + active_data_mask: dict[str, Any], + job_id: str, +) -> list[str]: + """Build the workbook on disk; return the list of skipped chart labels.""" + skipped: list[str] = [] + writer = StreamingXlsxWriter(path) + try: + for chart in get_charts_in_layout_order(dashboard): + if not chart.query_context: + skipped.append(_chart_label(chart)) + continue + try: + _write_chart_sheets(writer, chart, dashboard.id, active_data_mask) + except Exception: # pylint: disable=broad-except + logger.exception( + "Skipping chart %s in dashboard export %s", chart.id, job_id + ) + skipped.append(_chart_label(chart)) + + if writer.sheet_count == 0: + writer.add_summary_sheet( + "Export Summary", + ["No chart data could be exported.", *skipped], + ) + finally: + writer.close() + return skipped + + +def _send_failure_email( + user: Any, dashboard_title: str, requested_at: datetime +) -> None: + if not (user and getattr(user, "email", None)): + return + try: + email.send_export_email( + user.email, + email.build_subject(dashboard_title, success=False), + email.build_failure_email(dashboard_title, requested_at), + ) + except Exception: # pylint: disable=broad-except + logger.exception("Failed to send export failure email") + + +@celery_app.task( + name="export_dashboard_excel", + bind=True, + soft_time_limit=600, + time_limit=660, + max_retries=0, +) +def export_dashboard_excel( + self: Any, # pylint: disable=unused-argument + dashboard_id: int, + user_id: int, + active_data_mask: dict[str, Any], + job_id: str, +) -> None: + """ + Export a dashboard's chart data to an ``.xlsx`` and email a download link. + + :param dashboard_id: The dashboard to export + :param user_id: The requesting user (the task runs with their permissions) + :param active_data_mask: Live dashboard filter state keyed by native filter id + :param job_id: Correlation id, also the Celery task id and S3 object name + """ + # pylint: disable=import-outside-toplevel + from superset.models.dashboard import Dashboard + + requested_at = datetime.utcnow() Review Comment: ^ This ########## superset/dashboards/api.py: ########## @@ -1375,6 +1385,97 @@ def export_as_example(self, pk: int) -> Response: response.set_cookie(token, "done", max_age=600) return response + @expose("/<pk>/export_xlsx/", methods=("POST",)) + @protect() + @safe + @permission_name("export") + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.export_xlsx", + log_to_statsd=False, + ) + def export_xlsx(self, pk: int) -> WerkzeugResponse: + """Export all of a dashboard's chart data to an Excel workbook (async). + --- + post: + summary: Export dashboard chart data to Excel + description: >- + Enqueues an async task that writes each chart's data to its own + worksheet, uploads the .xlsx to S3, and emails the requesting user a + pre-signed download link. Returns immediately with a job id. + parameters: + - in: path + schema: + type: integer + name: pk + description: The dashboard id + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/DashboardExportXlsxPostSchema' + responses: + 202: + description: Export task accepted + content: + application/json: + schema: + $ref: '#/components/schemas/DashboardExportXlsxResponseSchema' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + 501: + description: Excel export is not configured on this server + """ Review Comment: We should add throttling here to prevent a DDOS. Store user_id:pk in Redis/DB when we start the export, remove when complete. Redis would be nice because we can set a TTL, in case removing when complete fails. Then here we can check for presence of they key, and return [202 Accepted](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/202) or something else if we're already computing it. ########## superset/dashboards/excel_export/email.py: ########## @@ -0,0 +1,113 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Email rendering and delivery for dashboard Excel exports. + +Bodies use inline styles only (no external CSS, no logo) to match Superset's +existing report notification emails, and all user-controlled values (dashboard +title, chart names) are HTML-escaped to avoid injection. +""" + +from __future__ import annotations + +from datetime import datetime + +from flask import current_app +from markupsafe import escape + +from superset.utils.core import send_email_smtp + +_DATETIME_FORMAT = "%Y-%m-%d %H:%M:%S" +_FOOTER_STYLE = "color:#888;font-size:12px;" +_BUTTON_STYLE = ( + "display:inline-block;padding:10px 16px;background:#20a7c9;color:#ffffff;" + "text-decoration:none;border-radius:4px;" +) + + +def _fmt(dt: datetime) -> str: + return dt.strftime(_DATETIME_FORMAT) + + +def build_subject(dashboard_title: str, *, success: bool) -> str: + """Build the email subject, prefixed with EMAIL_REPORTS_SUBJECT_PREFIX.""" + prefix = current_app.config["EMAIL_REPORTS_SUBJECT_PREFIX"] + if success: + return f"{prefix}Your dashboard export is ready: {dashboard_title}" + return f"{prefix}Your dashboard export could not be completed: {dashboard_title}" + + +def _skipped_section(skipped_charts: list[str]) -> str: + if not skipped_charts: + return "" + items = "".join(f"<li>{escape(label)}</li>" for label in skipped_charts) + return ( + "<p>Note: the following charts were omitted because they have no saved " + "query context. To include them, open each chart in Explore and re-save." + f"</p><ul>{items}</ul>" Review Comment: Let's fix this too. If it's not too complicated we could separate in groups: timed out/no query context/runtime error. ########## superset/dashboards/excel_export/email.py: ########## @@ -0,0 +1,113 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Email rendering and delivery for dashboard Excel exports. + +Bodies use inline styles only (no external CSS, no logo) to match Superset's +existing report notification emails, and all user-controlled values (dashboard +title, chart names) are HTML-escaped to avoid injection. +""" + +from __future__ import annotations + +from datetime import datetime + +from flask import current_app +from markupsafe import escape + +from superset.utils.core import send_email_smtp + +_DATETIME_FORMAT = "%Y-%m-%d %H:%M:%S" +_FOOTER_STYLE = "color:#888;font-size:12px;" +_BUTTON_STYLE = ( + "display:inline-block;padding:10px 16px;background:#20a7c9;color:#ffffff;" + "text-decoration:none;border-radius:4px;" +) + + +def _fmt(dt: datetime) -> str: + return dt.strftime(_DATETIME_FORMAT) + + +def build_subject(dashboard_title: str, *, success: bool) -> str: + """Build the email subject, prefixed with EMAIL_REPORTS_SUBJECT_PREFIX.""" + prefix = current_app.config["EMAIL_REPORTS_SUBJECT_PREFIX"] + if success: + return f"{prefix}Your dashboard export is ready: {dashboard_title}" + return f"{prefix}Your dashboard export could not be completed: {dashboard_title}" Review Comment: Let's do this. ########## tests/integration_tests/dashboards/api_tests.py: ########## @@ -3279,6 +3280,91 @@ def _base_filter(query): response_roles = [result["text"] for result in response["result"]] assert response_roles == ["Alpha"] + def test_export_xlsx_501_when_bucket_unset(self): + """Dashboard API: export_xlsx returns 501 when the S3 bucket is unset.""" + admin = self.get_user("admin") + dashboard = self.insert_dashboard("xlsx-501", None, [admin.id]) + self.login(ADMIN_USERNAME) + try: + rv = self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/") + assert rv.status_code == 501 + finally: + db.session.delete(dashboard) + db.session.commit() + + @patch("superset.dashboards.api.export_dashboard_excel") + def test_export_xlsx_404_for_missing_dashboard(self, mock_task): + """Dashboard API: export_xlsx returns 404 for an unknown dashboard.""" + self.login(ADMIN_USERNAME) + with patch.dict( Review Comment: We have the `@with_config` decorator, let's use that here instead. (And elsewhere.) ########## superset/utils/excel_streaming.py: ########## @@ -0,0 +1,190 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Streaming, constant-memory XLSX writer for multi-sheet dashboard exports. + +Unlike :mod:`superset.utils.excel`, which materializes a whole DataFrame in +memory, this writer streams rows one at a time into an ``xlsxwriter`` workbook +opened in ``constant_memory`` mode, so a dashboard with many large charts never +holds more than one row per sheet in memory at once. Review Comment: If this is the case, it would be better to just use the existing module instead of adding more logic to generate XLS. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
