sadpandajoe commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4214194303
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -985,10 +1072,64 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
extra: contentType === ContentType.Dashboard ? currentAlert?.extra : {},
};
+ if (!isReport && !attachmentControlsVisible && !isEditMode) {
+ data.report_format = 'NONE';
+ }
+ if (!attachmentControlsVisible && isEditMode) {
+ // Hidden settings stay stored; only the format controls attachment
delivery.
+ delete data.chart;
+ delete data.dashboard;
+ delete data.extra;
+ delete data.custom_width;
+ delete data.force_screenshot;
+ delete data.include_cta;
Review Comment:
The "Include a link back to Superset" checkbox lives inside the
attachment-controls block, and saving without those controls deletes
`include_cta` from the payload. So if an editor unchecks the link and then
turns the attachment off (or attachments are globally off for alerts), the
unchecked choice is silently dropped and the notifications keep the chart link.
`include_cta` is stored independently of the format and is read in the executor
regardless of attachments, so should this be left in the payload and the
checkbox kept outside the attachment-only block?
##########
superset/commands/report/update.py:
##########
@@ -193,6 +489,18 @@ def validate(self) -> None: # noqa: C901
include_viewers=False,
)
+ self.validate_report_format(
+ report_type,
+ self._properties.get("report_format", self._model.report_format),
+ exceptions,
+ )
+ if "recipients" in self._properties:
+ self.validate_recipients_policy(exceptions)
Review Comment:
The PUT path's recipient-policy check has no test that would fail if this
call were removed. `run_as_test.py` stubs `validate_recipients_policy` on
`UpdateReportScheduleCommand`, and the API recipient-update tests run with no
allowlist, while the only unstubbed policy test
(`test_create_enforces_recipient_policy`) covers create. Could you add an
update-command test with a stored allowlist and a recipient outside it,
asserting the invalid-error contains `ReportScheduleRecipientNotAllowedError`
and the stored recipients are unchanged?
##########
superset/reports/schemas.py:
##########
@@ -628,6 +743,106 @@ def validate_custom_width(
)
+def validate_email_domain(value: str) -> None:
+ if not EMAIL_DOMAIN_REGEX.match(value.strip()):
+ raise ValidationError(_("Invalid e-mail domain: %(domain)s",
domain=value))
+
+
+class ReportConfigurationSchema(Schema):
+ """
+ Global Alerts & Reports configuration (SIP-209).
+
+ Every field is optional on update: absent fields keep their stored value
and
+ ``null`` clears a setting without restoring its legacy fallback.
+ """
+
+ alerts_attach_reports = fields.Boolean(
+ metadata={
+ "description": "Whether alerts deliver their attachment
(screenshot, "
+ "PDF, CSV or XLSX). When disabled, alerts only send the message
and "
+ "link. Falls back to the FF until a row has first been saved."
+ },
+ required=False,
+ allow_none=True,
+ )
+ date_format_in_email_subject = fields.Boolean(
+ metadata={
+ "description": "Render strftime date placeholders in email
subjects. "
+ "Falls back to the DATE_FORMAT_IN_EMAIL_SUBJECT FF until first
saved."
+ },
+ required=False,
+ allow_none=True,
+ )
+ alert_minimum_interval = fields.Integer(
+ metadata={
+ "description": "Minimum interval between alert executions, in
seconds. "
+ "Values below 120 do not restrict schedules. Falls back to "
+ "ALERT_MINIMUM_INTERVAL only before this setting has been saved.",
+ "example": 3600,
+ },
+ required=False,
+ allow_none=True,
+ validate=[Range(min=0, error=lazy_gettext("Value must be 0 or
greater"))],
+ )
+ report_minimum_interval = fields.Integer(
+ metadata={
+ "description": "Minimum interval between report executions, in
seconds. "
+ "Values below 120 do not restrict schedules. Falls back to "
+ "REPORT_MINIMUM_INTERVAL only before this setting has been saved.",
+ "example": 3600,
+ },
+ required=False,
+ allow_none=True,
+ validate=[Range(min=0, error=lazy_gettext("Value must be 0 or
greater"))],
+ )
+ limit_recipients_to_users = fields.Boolean(
+ metadata={
+ "description": "When enabled, only e-mail addresses of existing
active "
+ "users are accepted as recipients."
+ },
+ required=False,
+ allow_none=True,
+ )
+ allowed_email_domains = fields.List(
+ fields.String(validate=validate_email_domain),
+ metadata={
+ "description": "E-mail domains accepted as recipients. An empty
list "
+ "allows any domain.",
+ "example": ["example.com", "superset.com"],
+ },
+ required=False,
+ allow_none=True,
+ )
+
+ @pre_load
+ def normalize_domains(self, data: dict[str, Any], **kwargs: Any) ->
dict[str, Any]:
+ domains = data.get("allowed_email_domains")
Review Comment:
`normalize_domains` calls `data.get(...)` before Marshmallow checks that the
body is an object, so an admin `PUT /configuration/` with a JSON body of `[]`
or `null` raises `AttributeError`. The endpoint only catches `ValidationError`,
so that surfaces as a 500 instead of the documented 400. Would a guard like `if
not isinstance(data, dict): return data` here let the normal type validation
produce the 400?
--
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]
