msyavuz commented on code in PR #44336:
URL: https://github.com/apache/superset/pull/44336#discussion_r4059837716
##########
superset/reports/notifications/email.py:
##########
@@ -148,12 +148,8 @@ def _get_smtp_domain() -> str:
return parseaddr(current_app.config["SMTP_MAIL_FROM"])[1].split("@")[1]
def _error_template(self, text: str) -> str:
- # The error text is derived from exception messages that can embed
- # data-controlled content (e.g. crafted table/column names in a DB
- # error). Strip all HTML before interpolating it into the email body,
- # matching the sanitization applied to the normal content path.
- # pylint: disable=no-member
- safe_text = nh3.clean(text, tags=set(), attributes={})
+ # Diagnostics remain in execution history, not outbound notifications.
+ safe_text = __("Contact the report owner for error details.")
Review Comment:
`send_error` only goes to the schedule's editors, so this tells the owner to
contact themselves ("...because of the following error: Contact the report
owner for error details.") and drops the only diagnostic they had. Could we
keep the `nh3`-sanitized text for editor-only error notifications?
##########
superset/commands/report/execute.py:
##########
@@ -142,26 +149,8 @@ def resolve_executor_user(model: ReportSchedule) ->
tuple["User", str]:
def _should_build_execution_context(model: ReportSchedule) -> bool:
- """
- Whether an execution should run under a :class:`ReportExecutionContext`.
-
- Reports always do — their behavior is unchanged. Alerts join them only when
- they deliver a rendered PNG/PDF screenshot to recipients, which happens
when
- ``ALERTS_ATTACH_REPORTS`` is enabled. Delivered screenshots must fail
closed:
- the context selects the fail-closed readiness predicate and disables
- partial-tile fallback, so a blank or incomplete capture raises instead of
- being delivered.
-
- CSV/text alerts, alerts without the attach flag, the non-delivered
- query-context capture, and UI thumbnails are deliberately excluded and keep
- their lenient capture contract.
- """
- if model.type == ReportScheduleType.REPORT:
- return True
- return model.report_format in (
- ReportDataFormat.PNG,
- ReportDataFormat.PDF,
- ) and feature_flag_manager.is_feature_enabled("ALERTS_ATTACH_REPORTS")
+ """Give every scheduled report and alert a shared deadline and ownership
context."""
+ return model.type in (ReportScheduleType.REPORT, ReportScheduleType.ALERT)
Review Comment:
This puts CSV/text alerts under the fail-closed capture too, so the
`_update_query_context` bootstrap screenshot for a chart without a saved query
context can now raise `ScreenshotBlankCaptureError` where it was lenient
before. Is that intended, given the description says capture checks are
unchanged?
--
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]
