mikebridge commented on code in PR #43402: URL: https://github.com/apache/superset/pull/43402#discussion_r3833443793
########## superset/commands/chart/delete.py: ########## @@ -32,11 +33,62 @@ from superset.daos.report import ReportScheduleDAO from superset.exceptions import SupersetSecurityException from superset.models.slice import Slice +from superset.reports.models import ReportSchedule from superset.utils.decorators import on_error, transaction logger = logging.getLogger(__name__) +def build_blocked_by_reports_message( + charts: list[Slice], + reports: list[ReportSchedule], + single_target: bool, +) -> str: + """Build the user-facing message naming the alerts/reports that block deletion. + + Groups the blocking reports per chart, sorted by chart name (chart id as + the tie-breaker) and then report name so the message is deterministic + across database backends, and appends the remedy sentence. When the + command targets a single chart id the group prefix is dropped — in the + single-delete endpoint the surrounding toast already names the chart, and + in a one-chart bulk selection the user's own selection provides the + context. A multi-id command keeps the prefix on every group because the + bulk toast is generic, so the prefix is the only chart identification the + user gets. + """ + sentences: list[str] = [] + if single_target: + names = sorted(report.name for report in reports) + sentences.append( + str( + _( + "This chart is used by alerts or reports: %(names)s.", + names=", ".join(names), + ) + ) + ) Review Comment: This disclosure is pre-existing behavior on master, not introduced by this PR — scoping it per the repo's security model (CLAUDE.md → SECURITY.md): - On master, the same guard already returns the same unfiltered report names in the same 422: `ReportScheduleDAO.find_by_chart_ids` (`superset/daos/report.py:121-126`, no `ReportScheduleFilter`) feeds `"There are associated alerts or reports: %(report_names)s"` ([master `superset/commands/chart/delete.py:56-64`](https://github.com/apache/superset/blob/a05a0999877f6f3aa73c447f722e1e9630b98910/superset/commands/chart/delete.py#L56-L64)). This PR reformats that message (per-chart grouping + remedy); it emits no name master didn't already emit, and the recipient set is unchanged — the guard has always run before the editorship check, on both master and this branch. - The **new** surface this PR adds (the pre-confirm list in the Archive modal) deliberately keeps the boundary: it queries the report list API, which *does* apply `ReportScheduleFilter`, and silently degrades to no list when rows are filtered away. That asymmetry (filtered modal, unfiltered guard message) is a documented design decision in the PR's contract. - Per this repo's requirements for automated findings: a principal without chart `can_write` cannot reach this 422 at all, and reports rely on route-level authorization plus DAO `base_filters` by design — the finding doesn't identify a capability-matrix row this change newly violates. If the project wants to tighten the guard's disclosure (redact names the caller can't read), that changes long-standing behavior across the single, bulk, and MCP delete flows and trades away exactly the explainability this bug fix restores — it deserves its own scoped issue rather than a rider on this PR. 🤖 Generated by Claude (AI) on behalf of @mikebridge. -- 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]
