bito-code-review[bot] commented on code in PR #41336: URL: https://github.com/apache/superset/pull/41336#discussion_r3459650620
########## superset/commands/report/execute_now.py: ########## @@ -0,0 +1,148 @@ +# 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. +import logging +from datetime import datetime, timezone +from typing import Any, Optional +from uuid import uuid4 + +from flask import current_app + +from superset import security_manager +from superset.commands.base import BaseCommand +from superset.commands.exceptions import CommandException +from superset.commands.report.exceptions import ( + ReportScheduleCeleryNotConfiguredError, + ReportScheduleExecuteNowFailedError, + ReportScheduleForbiddenError, + ReportScheduleNotFoundError, +) +from superset.daos.report import ReportScheduleDAO +from superset.exceptions import SupersetSecurityException +from superset.reports.models import ReportSchedule + +logger = logging.getLogger(__name__) + + +class ExecuteReportScheduleNowCommand(BaseCommand): + """ + Execute a report schedule immediately (manual trigger). + + Validates permissions and triggers immediate execution of a report or alert + via Celery task, similar to scheduled execution but without waiting for the + cron schedule. Sets ``eta`` to the current UTC time so that the downstream + task receives a valid ``scheduled_dttm`` value. + """ + + def __init__(self, model_id: int) -> None: + self._model_id = model_id + self._model: Optional[ReportSchedule] = None + + def run(self) -> str: + """ + Execute the command and return an execution UUID for tracking. + + Returns: + str: Execution UUID that can be used to track the execution status. + + Raises: + ReportScheduleNotFoundError: Report schedule not found. + ReportScheduleForbiddenError: User doesn't have permission to execute. + ReportScheduleCeleryNotConfiguredError: Celery broker is not reachable. + ReportScheduleExecuteNowFailedError: Execution failed to start. + """ + try: + self.validate() + if not self._model: + raise ReportScheduleExecuteNowFailedError() + + execution_id = str(uuid4()) + + logger.info( + "Manually executing report schedule %s (id: %d), execution_id: %s", + self._model.name, + self._model.id, + execution_id, + ) + + # Import here to avoid circular imports + from superset.tasks.scheduler import execute + + # Set eta to now so the downstream task receives a valid scheduled_dttm. + async_options: dict[str, Any] = { + "task_id": execution_id, + "eta": datetime.now(tz=timezone.utc), + } + + if self._model.working_timeout is not None and current_app.config.get( + "ALERT_REPORTS_WORKING_TIME_OUT_KILL", True + ): + async_options["time_limit"] = ( + self._model.working_timeout + + current_app.config.get("ALERT_REPORTS_WORKING_TIME_OUT_LAG", 10) + ) + async_options["soft_time_limit"] = ( + self._model.working_timeout + + current_app.config.get( + "ALERT_REPORTS_WORKING_SOFT_TIME_OUT_LAG", 5 + ) + ) + + try: + execute.apply_async((self._model.id,), **async_options) + except Exception as celery_ex: + # Distinguish between broker connectivity errors and other failures. + # kombu raises transport/connection errors; we surface a user-friendly + # message in the former case rather than a generic 422. + error_msg = str(celery_ex).lower() + connectivity_keywords = ( + "no broker", + "broker connection", + "kombu", + "redis", + "rabbitmq", + "not registered", + "connection refused", + "connection error", + "connect timeout", + ) + if any(kw in error_msg for kw in connectivity_keywords): + logger.error("Celery backend not configured: %s", str(celery_ex)) + raise ReportScheduleCeleryNotConfiguredError() from celery_ex + logger.error("Celery task execution failed: %s", str(celery_ex)) + raise ReportScheduleExecuteNowFailedError() from celery_ex + + return execution_id + + except CommandException: + raise + except Exception as ex: + logger.exception( + "Unexpected error executing report schedule %d", self._model_id + ) + raise ReportScheduleExecuteNowFailedError() from ex + + def validate(self) -> None: + """Validate the report schedule exists and the user has permission to + execute.""" + self._model = ReportScheduleDAO.find_by_id(self._model_id) + if not self._model: + raise ReportScheduleNotFoundError() + + try: + security_manager.raise_for_ownership(self._model) + except SupersetSecurityException as ex: + raise ReportScheduleForbiddenError() from ex Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing unit tests for new command</b></div> <div id="fix"> This new command has no unit test coverage. Per BITO.md adaptive rule 11730, new tools should include tests covering success paths, error scenarios, validation failures, and edge cases. The absence of tests creates regression risk and prevents verification of the error-handling logic (lines 106-126). </div> </div> <small><i>Code Review Run #bd7374</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/pages/AlertReportList/index.tsx: ########## @@ -405,6 +449,15 @@ function AlertList({ onClick: handleEdit, } : null, + allowEdit + ? { + label: 'trigger-now-action', + tooltip: t('Trigger now'), + placement: 'bottom', + icon: 'ThunderboltOutlined', + onClick: () => handleExecuteReport(original), + } + : null, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing unit test for trigger-now action</b></div> <div id="fix"> The new `trigger-now-action` button and its `handleExecuteReport` handler lack unit-test coverage in `AlertReportList.test.tsx`. According to BITO.md rule 6262, tests should verify the actual business logic (execute call, toast display) rather than just rendering. </div> </div> <small><i>Code Review Run #bd7374</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
