codeant-ai-for-open-source[bot] commented on code in PR #41133: URL: https://github.com/apache/superset/pull/41133#discussion_r3429857097
########## 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: **Suggestion:** This message hardcodes that skipped charts were omitted only for missing saved query context, but the export task also skips charts on runtime/query exceptions. Users will receive misleading remediation instructions for failures unrelated to saved context; use neutral wording or pass skip reasons so the email reflects the real cause. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Skipped charts from runtime errors misreported as unsaved. - ⚠️ Users follow incorrect remediation steps for failing charts. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create a dashboard with at least one chart whose `query_context` is non-empty (so `chart.query_context` is truthy) but whose execution will fail, for example by pointing the chart at a dataset with an invalid SQL column; this chart is exported by `_write_chart_sheets` using `ChartDataCommand` at `superset/tasks/export_dashboard_excel.py:96-99`. 2. Trigger an export by calling `POST /api/v1/dashboard/<pk>/export_xlsx/` in `DashboardRestApi.export_xlsx` at `superset/dashboards/api.py:206-236`, which enqueues the `export_dashboard_excel` Celery task with the dashboard id and the requesting user's id. 3. In `export_dashboard_excel` (`superset/tasks/export_dashboard_excel.py:169-244`), the function `_build_workbook` is invoked at line 206; `_build_workbook` iterates over charts from `get_charts_in_layout_order(dashboard)` and for each chart with a saved `query_context` calls `_write_chart_sheets` inside a broad `try/except` at lines 125-136. 4. When `_write_chart_sheets` raises due to the runtime/query error, the `except Exception` block at lines 131-135 logs the failure and appends `_chart_label(chart)` to the `skipped` list, which is later passed as `skipped_charts` into `email.build_success_email` at lines 224-231; `build_success_email` calls `_skipped_section` at `superset/dashboards/excel_export/email.py:54-62`, which renders a note stating the charts were omitted "because they have no saved query context" and instructs users to re-save in Explore, even though in this path the chart was skipped due to a runtime/query exception, making the remediation text misleading. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=71aaaa7017624e0b85a0fc5877576a38&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=71aaaa7017624e0b85a0fc5877576a38&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/dashboards/excel_export/email.py **Line:** 59:61 **Comment:** *Logic Error: This message hardcodes that skipped charts were omitted only for missing saved query context, but the export task also skips charts on runtime/query exceptions. Users will receive misleading remediation instructions for failures unrelated to saved context; use neutral wording or pass skip reasons so the email reflects the real cause. 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%2F41133&comment_hash=bba51696c035b3be158a7048a23a2e20a0d53e9dbe1536b0c320ec9792163622&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=bba51696c035b3be158a7048a23a2e20a0d53e9dbe1536b0c320ec9792163622&reaction=dislike'>👎</a> ########## 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: **Suggestion:** The expiry text truncates the TTL to whole hours, so links with TTLs under one hour are reported as "0 hours" and non-hour TTLs are underreported. This produces incorrect expiry information in user emails; format the TTL with minutes (or round up) so the displayed lifetime matches the real pre-signed URL expiration. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Excel export emails show incorrect download link lifetimes. - ⚠️ DashboardRestApi.export_xlsx users receive misleading expiry text. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure a non-hour TTL in `superset/config.py:1304-1312`, for example set `EXCEL_EXPORT_LINK_TTL_SECONDS = 900` (15 minutes) instead of the default 86400 seconds. 2. As a logged-in user with `can_export` permission, call `POST /api/v1/dashboard/<pk>/export_xlsx/` implemented in `DashboardRestApi.export_xlsx` at `superset/dashboards/api.py:206-236`; this enqueues the `export_dashboard_excel` Celery task with the dashboard id and user id. 3. The Celery worker executes `export_dashboard_excel` in `superset/tasks/export_dashboard_excel.py:169-244`, which reads `ttl = current_app.config["EXCEL_EXPORT_LINK_TTL_SECONDS"]` at line 213 and computes `expires_at = datetime.utcnow() + timedelta(seconds=ttl)` at line 217, then calls `email.build_success_email(..., ttl_seconds=ttl, skipped_charts=skipped)` at lines 221-231. 4. In `build_success_email` at `superset/dashboards/excel_export/email.py:65-82`, the code computes `hours = ttl_seconds // 3600` (line 76) and renders `This link expires in {hours} hours ({_fmt(expires_at)} UTC).`; for a 900-second TTL this produces `0 hours` even though the pre-signed URL actually expires after 15 minutes, so the email's human-readable lifetime is incorrect relative to the real expiration. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=004ec65f36054dc4811a823a12906ec4&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=004ec65f36054dc4811a823a12906ec4&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/dashboards/excel_export/email.py **Line:** 76:81 **Comment:** *Logic Error: The expiry text truncates the TTL to whole hours, so links with TTLs under one hour are reported as "0 hours" and non-hour TTLs are underreported. This produces incorrect expiry information in user emails; format the TTL with minutes (or round up) so the displayed lifetime matches the real pre-signed URL expiration. 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%2F41133&comment_hash=54c4ca107b1135cb3cd95919cd6746319b347f85e8dc5ef726b58f687dfdd781&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=54c4ca107b1135cb3cd95919cd6746319b347f85e8dc5ef726b58f687dfdd781&reaction=dislike'>👎</a> ########## superset/dashboards/excel_export/layout.py: ########## @@ -0,0 +1,97 @@ +# 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. +"""Determine the order in which a dashboard's charts appear in its layout.""" + +from __future__ import annotations + +from typing import Any, TYPE_CHECKING + +if TYPE_CHECKING: + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + +CHART_TYPE = "CHART" +ROOT_ID = "ROOT_ID" + + +def _walk_chart_ids(position: dict[str, Any]) -> list[int]: + """ + Depth-first walk of a dashboard ``position_json`` returning chart ids in + visual (layout) order, including tab-nested charts. Each chart id appears + once (first occurrence wins); cycles are guarded against. + """ + if ROOT_ID not in position: + return [] + + ordered: list[int] = [] + seen_charts: set[int] = set() + visited_nodes: set[str] = set() + stack: list[str] = [ROOT_ID] + + while stack: + node_id = stack.pop() + if node_id in visited_nodes: + continue + visited_nodes.add(node_id) + + node = position.get(node_id) + if not isinstance(node, dict): + continue + + if node.get("type") == CHART_TYPE: + chart_id = node.get("meta", {}).get("chartId") + if isinstance(chart_id, int) and chart_id not in seen_charts: + seen_charts.add(chart_id) + ordered.append(chart_id) + + # Push children in reverse so they are popped in their declared order. + children = node.get("children", []) + for child_id in reversed(children): + stack.append(child_id) Review Comment: **Suggestion:** Iteration assumes `children` is always a reversible sequence; if persisted layout JSON has `children` as `null` or another non-sequence value, `reversed(children)` raises `TypeError` and aborts the export flow. Guard the value type before reversing to avoid crashing on malformed layout nodes. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Excel export task fails on dashboards with malformed layouts. - ⚠️ Affected dashboards cannot produce Excel workbook exports. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create or edit a dashboard so that its `position_json` (stored in the `Dashboard.position_json` column defined at `superset/models/dashboard.py:137` and exposed in the dashboard admin via `position_json` fields in `superset/views/dashboard/mixin.py:36-50`) contains a layout node with a non-list `children` value, for example `"children": null` under `"ROOT_ID"`. 2. Trigger an Excel export for this dashboard by calling `POST /api/v1/dashboard/<pk>/export_xlsx/` handled by `DashboardRestApi.export_xlsx` at `superset/dashboards/api.py:206-236`, which enqueues the `export_dashboard_excel` Celery task with the dashboard id. 3. In `export_dashboard_excel` (`superset/tasks/export_dashboard_excel.py:169-244`), the function `_build_workbook` is called at line 206; `_build_workbook` iterates charts using `for chart in get_charts_in_layout_order(dashboard):` at line 125, so it invokes `get_charts_in_layout_order` from `superset/dashboards/excel_export/layout.py:69-97`. 4. `get_charts_in_layout_order` calls `_walk_chart_ids(dashboard.position)` at lines 82-87, and `_walk_chart_ids` at `superset/dashboards/excel_export/layout.py:31-66` does `children = node.get("children", [])` (line 62) followed by `for child_id in reversed(children):` (line 63); when `children` is `None` or another non-sequence from the malformed `position_json`, `reversed(children)` raises a `TypeError` which is not caught in `_walk_chart_ids` or `_build_workbook`, causing the `export_dashboard_excel` task to hit its general `except Exception` block at lines 241-243, log a failure, send a failure email, and abort the Excel export for that dashboard. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=66e3d2f6a73741d4bd3192fa84e9bb65&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=66e3d2f6a73741d4bd3192fa84e9bb65&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/dashboards/excel_export/layout.py **Line:** 62:64 **Comment:** *Type Error: Iteration assumes `children` is always a reversible sequence; if persisted layout JSON has `children` as `null` or another non-sequence value, `reversed(children)` raises `TypeError` and aborts the export flow. Guard the value type before reversing to avoid crashing on malformed layout nodes. 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%2F41133&comment_hash=ac875138478152d281b91214c40ef95df7a9ec3852097ca6db6c3358a2c6c206&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=ac875138478152d281b91214c40ef95df7a9ec3852097ca6db6c3358a2c6c206&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: [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]
