sadpandajoe commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4202893096
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -2327,310 +2613,461 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
),
children: (
<>
- <StyledInputContainer>
- <div className="control-label">
- {t('Content type')}
- <span className="required">*</span>
- </div>
- <Select
- ariaLabel={t('Select content type')}
- onChange={onContentTypeChange}
- value={contentType}
- options={CONTENT_TYPE_OPTIONS}
- placeholder={t('Select content type')}
+ {dynamicExecutorEnabled && !isAdmin && (
+ <Alert
+ type={restrictedExecutor ? 'warning' : 'info'}
+ showIcon
+ message={
+ restrictedExecutor
+ ? t('Content and recipient edits are restricted')
+ : t('Content and permissions')
+ }
+ description={
+ restrictedExecutor ? (
+ <>
+ <p>
+ {t(
Review Comment:
Passing a ternary to `t()` means the extractor never sees either sentence,
so neither lands in the translation catalog and both stay English for a
non-English user. Could each branch call `t('…')` with its own string literal
instead?
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -2327,310 +2613,461 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
),
children: (
<>
- <StyledInputContainer>
- <div className="control-label">
- {t('Content type')}
- <span className="required">*</span>
- </div>
- <Select
- ariaLabel={t('Select content type')}
- onChange={onContentTypeChange}
- value={contentType}
- options={CONTENT_TYPE_OPTIONS}
- placeholder={t('Select content type')}
+ {dynamicExecutorEnabled && !isAdmin && (
+ <Alert
+ type={restrictedExecutor ? 'warning' : 'info'}
+ showIcon
+ message={
+ restrictedExecutor
+ ? t('Content and recipient edits are restricted')
+ : t('Content and permissions')
+ }
+ description={
+ restrictedExecutor ? (
+ <>
+ <p>
+ {t(
+ isReport
+ ? 'You can edit the name and schedule, but
changing the delivered content or recipients requires updating it to execute
with your permissions. Only admins can select other users.'
Review Comment:
This tells an editor they can change just the name and schedule, but the
modal still re-sends the stored `database`/`sql`, and
`UpdateReportScheduleCommand.validate` re-runs `validate_alert_query` as the
editor whenever either key is present. For an alert whose query runs as another
user and an editor who can't read its dataset, a name-only save would fail with
a data-access error even though the condition didn't change. Should unchanged
`sql`/`database` be left out of the payload (or skipped server-side) so this
holds?
##########
superset/daos/report.py:
##########
@@ -392,3 +398,180 @@ def bulk_delete_logs(model: ReportSchedule, from_date:
datetime) -> int | None:
)
.delete(synchronize_session="fetch")
)
+
+ @staticmethod
+ def find_with_email_recipients() -> list[ReportSchedule]:
Review Comment:
The configuration-conflict scan depends on this query, but the command tests
patch `find_with_email_recipients` and the DAO tests only cover
`ReportConfigDAO`, so nothing runs this join/filter. If it ever skipped an
inactive or bcc-only schedule, saving a stricter domain list would succeed with
no conflict reported. Could a DB-backed test seed an inactive schedule with an
external bcc plus a Slack-only schedule and assert `UpdateReportConfigCommand`
reports only the first?
##########
superset/commands/report/alert.py:
##########
@@ -214,37 +216,41 @@ def _execute_query(self) -> pd.DataFrame:
:raises AlertQueryError: SQL query is not valid
:raises AlertQueryTimeout: The SQL query received a celery soft timeout
"""
- sql_template = jinja_context.get_template_processor(
- database=self._report_schedule.database
- )
-
try:
- rendered_sql =
sql_template.process_template(self._report_schedule.sql)
- self._validate_rendered_sql(rendered_sql)
- limited_rendered_sql =
self._report_schedule.database.apply_limit_to_sql(
- rendered_sql, ALERT_SQL_LIMIT
- )
-
- if app.config["MUTATE_ALERT_QUERY"]:
- limited_rendered_sql = (
- self._report_schedule.database.mutate_sql_based_on_config(
- limited_rendered_sql
- )
+ user = get_dynamic_executor(self._report_schedule,
alert_query=True)
+ if user is not None:
+ username = user.username
+ else:
+ executor, username = get_executor( # pylint:
disable=unused-variable
+ executors=app.config["ALERT_REPORTS_EXECUTORS"],
+ model=self._report_schedule,
)
-
- executor, username = get_executor( # pylint:
disable=unused-variable
- executors=app.config["ALERT_REPORTS_EXECUTORS"],
- model=self._report_schedule,
- )
- user = security_manager.find_user(username)
+ user = security_manager.find_user(username)
# A deleted/disabled executor user makes find_user return None.
Raise
# the dedicated error so the handler below re-surfaces it instead
of
# masking it as an opaque AlertQueryError (or letting the missing
user
# surface as a NoneType error from the downstream auth flow).
- if user is None:
+ if user is None or not user.is_active:
raise ReportScheduleExecutorNotFoundError(username)
with override_user(user):
+ sql_template = jinja_context.get_template_processor(
Review Comment:
Executor resolution now runs before templating, and
`test_execute_query_wraps_template_rendering_error` still uses a bare `Mock`
schedule. `get_executor` iterates `model.editors`, which raises `TypeError` on
a Mock, and the broad `except Exception` turns that into `AlertQueryError`, so
the test passes without `process_template` ever being called. Could the fixture
provide a resolvable executor and assert the `TemplateError` is the cause?
--
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]
