codeant-ai-for-open-source[bot] commented on code in PR #41133: URL: https://github.com/apache/superset/pull/41133#discussion_r3573314819
########## superset/dashboards/excel_export/email.py: ########## @@ -0,0 +1,180 @@ +# 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 flask_babel import gettext as __, ngettext +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;" +) + +# Reason keys under which the export task groups charts it could not export. +# The task classifies each omitted chart under one of these; the email renders a +# separate, labelled section per non-empty group with its own remediation text. +ERROR_NO_QUERY_CONTEXT = "no-query-context" +ERROR_GENERAL = "general-exception" + + +def _fmt(dt: datetime) -> str: + return dt.strftime(_DATETIME_FORMAT) + + +def _humanize_ttl(seconds: int) -> str: + """Render a TTL as a human-readable, pluralized, translatable duration. + + Whole hours read as "24 hours"; sub-hour and non-hour values keep their + minutes (e.g. "1 hour 30 minutes", "15 minutes") so the stated lifetime + always matches the real pre-signed URL expiration. + """ + hours, remainder = divmod(seconds, 3600) + parts: list[str] = [] + if hours: + parts.append(ngettext("%(num)d hour", "%(num)d hours", hours)) + if minutes := remainder // 60: + parts.append(ngettext("%(num)d minute", "%(num)d minutes", minutes)) + if not parts: + parts.append(ngettext("%(num)d second", "%(num)d seconds", seconds)) Review Comment: **Suggestion:** The pluralized TTL strings use `ngettext` with `%(num)d` placeholders but never pass the `num` variable, which can leave raw placeholders in emails (or raise interpolation errors depending on Babel configuration). Pass `num=...` for each `ngettext` call so durations render correctly. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Export success emails show broken TTL placeholder text. ⚠️ Users unclear on Excel download link expiration time. ⚠️ Localized duration strings never render actual numeric values. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. A user triggers an Excel export from the dashboard Download menu (`Export Data to Excel` or `Export Images to Excel`), wired via `onExportXlsx` in `superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:60-77`, which POSTs to `/api/v1/dashboard/<id>/export_xlsx/`. 2. The backend enqueues the Celery task `export_dashboard_excel` in `superset/tasks/export_dashboard_excel.py:7-24`; when the worker runs, it builds the workbook and uploads it to S3 (`export_dashboard_excel.py:47-59`), then reads the link TTL from `current_app.config["EXCEL_EXPORT_LINK_TTL_SECONDS"]` at line 56 and computes `expires_at` at line 60. 3. On successful completion, the task sends a success email via `email.send_export_email` at `export_dashboard_excel.py:62-75`, passing `email.build_success_email(...)` where `ttl_seconds=ttl` and `expires_at`/`requested_at` are provided (lines 67-73). 4. `build_success_email` in `superset/dashboards/excel_export/email.py:118-149` constructs the expiry line using `_humanize_ttl(ttl_seconds)` (line 133); `_humanize_ttl` at lines 60-68 calls `ngettext("%(num)d hour", "%(num)d hours", hours)`, `ngettext("%(num)d minute", "%(num)d minutes", minutes)`, and `ngettext("%(num)d second", "%(num)d seconds", seconds)` without passing a `num` mapping, so the returned strings contain literal `%(num)d` placeholders. These un-interpolated placeholders are concatenated and injected into the email body, causing recipients to see broken copy like `This link expires in %(num)d hours` instead of a numeric duration whenever a dashboard export email is sent. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6cf57678bb2f479aaaffd142a84aedf2&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=6cf57678bb2f479aaaffd142a84aedf2&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:** 62:67 **Comment:** *Logic Error: The pluralized TTL strings use `ngettext` with `%(num)d` placeholders but never pass the `num` variable, which can leave raw placeholders in emails (or raise interpolation errors depending on Babel configuration). Pass `num=...` for each `ngettext` call so durations render correctly. 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=e8e641ccfe3921ee2c63e6d388b778face939605e454c6bf70a2d6fd9e5e4edd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=e8e641ccfe3921ee2c63e6d388b778face939605e454c6bf70a2d6fd9e5e4edd&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]
