bito-code-review[bot] commented on code in PR #35083:
URL: https://github.com/apache/superset/pull/35083#discussion_r3455581589
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -2688,57 +2712,51 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
) : (
<Loading size="s" muted position="normal" />
)}
- </StyledInputContainer>
- <StyledInputContainer>
- <div className="control-label">
- {t('Log retention')}
- <span className="required">*</span>
- </div>
- <div className="input-container">
- <Select
- ariaLabel={t('Log retention')}
- placeholder={t('Log retention')}
- onChange={onLogRetentionChange}
- value={currentAlert?.log_retention}
- options={RETENTION_OPTIONS}
- sortComparator={propertyComparator('value')}
- />
- </div>
- </StyledInputContainer>
- <StyledInputContainer css={noMarginBottom}>
- {isReport ? (
- <>
- <div className="control-label">
- {t('Working timeout')}
- <span className="required">*</span>
- </div>
- <div className="input-container">
- <NumberInput
- min={1}
- name="working_timeout"
- value={currentAlert?.working_timeout || ''}
- placeholder={t('Time in seconds')}
- onChange={onTimeoutVerifyChange}
- timeUnit={t('seconds')}
- />
- </div>
- </>
- ) : (
- <>
- <div className="control-label">{t('Grace
period')}</div>
- <div className="input-container">
- <NumberInput
- min={1}
- name="grace_period"
- value={currentAlert?.grace_period || ''}
- placeholder={t('Time in seconds')}
- onChange={onTimeoutVerifyChange}
- timeUnit={t('seconds')}
- />
- </div>
- </>
- )}
- </StyledInputContainer>
+ </ModalFormField>
+ <ModalFormField
+ label={t('Log retention')}
+ required
+ error={
+ validationStatus[Sections.Schedule]?.hasErrors &&
+ !currentAlert?.log_retention
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Bug: False error on 'None' option</b></div>
<div id="fix">
The error condition `!currentAlert?.log_retention` incorrectly triggers when
`log_retention` is `0` (the 'None' option in `RETENTION_OPTIONS`). Since `!0
=== true`, users selecting 'None' see a false validation error. Use `== null`
instead to check for null/undefined only.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset-frontend/src/features/alerts/AlertReportModal.tsx
+++ b/superset-frontend/src/features/alerts/AlertReportModal.tsx
@@ -2718,7 +2718,7 @@ export default function AlertReportModal({
error={
validationStatus[Sections.Schedule]?.hasErrors &&
- !currentAlert?.log_retention
+ currentAlert?.log_retention == null
? t('Log retention is required')
: undefined
}
```
</div>
</details>
</div>
<small><i>Code Review Run #997e80</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/reports/api.py:
##########
@@ -678,3 +683,77 @@ def slack_channels(self, **kwargs: Any) -> Response:
except SupersetException as ex:
logger.error("Error fetching slack channels %s", str(ex))
return self.response_422(message=str(ex))
+
+ @expose("/<int:pk>/execute", methods=("POST",))
+ @protect()
+ @safe
+ @permission_name("write")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.execute",
+ log_to_statsd=False,
+ )
+ def execute(self, pk: int) -> Response:
+ """Execute a report schedule immediately.
+ ---
+ post:
+ summary: Execute a report schedule immediately
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The report schedule pk
+ responses:
+ 200:
+ description: Report schedule execution started
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ execution_id:
+ type: string
+ description: UUID to track the execution status
+ message:
+ type: string
+ description: Success message
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ try:
+ execution_id = ExecuteReportScheduleNowCommand(pk).run()
+ response_schema = ReportScheduleExecuteResponseSchema()
+ return self.response(
+ 200,
+ **response_schema.dump(
+ {
+ "execution_id": execution_id,
+ "message": "Report schedule execution started
successfully",
+ }
+ ),
+ )
+ except ReportScheduleNotFoundError:
+ return self.response_404()
+ except ReportScheduleForbiddenError:
+ return self.response_403()
+ except ReportScheduleCeleryNotConfiguredError as ex:
+ logger.error(
+ "Celery backend not configured for report schedule execution:
%s",
+ str(ex),
+ )
+ return self.response(503, message=str(ex))
+ except ReportScheduleExecuteNowFailedError as ex:
+ logger.error(
+ "Error executing report schedule %s: %s",
+ self.__class__.__name__,
+ str(ex),
+ exc_info=True,
+ )
+ return self.response_422(message=str(ex))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new endpoint</b></div>
<div id="fix">
The new `execute` endpoint and its corresponding
`ExecuteReportScheduleNowCommand` lack unit test coverage. Per project
guidelines, new tools must include dedicated tests covering success paths,
error scenarios, validation failures, and edge cases.
</div>
</div>
<small><i>Code Review Run #997e80</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]
