AryaKetanShCt commented on code in PR #42850:
URL: https://github.com/apache/superset/pull/42850#discussion_r3730790040
##########
superset/reports/schemas.py:
##########
@@ -175,6 +177,43 @@ def validate_addresses(field: str, value: str | None,
required: bool) -> None:
validate_addresses("ccTarget", config.get("ccTarget"), required=False)
validate_addresses("bccTarget", config.get("bccTarget"),
required=False)
+ @validates_schema
+ def validate_slack_recipients(self, data: dict[str, Any], **kwargs: Any)
-> None:
+ """Slack recipients must be channel ids, because a name never
delivers."""
+ if data.get("type") not in (
+ ReportRecipientType.SLACK.value,
+ ReportRecipientType.SLACKV2.value,
+ ):
+ return
Review Comment:
Good catch, and correct on all three counts. Fixed in the latest commits by
limiting the validation to `SlackV2`.
Verifying before changing it, the deprecated v1 path really does accept
names:
- `SlackNotification._get_channel()` returns the raw target and sends via
`files_upload(channels=...)` / `chat_postMessage(channel=...)`, both of which
resolve a channel name.
- `slack.py` says so itself: *"existing v1 recipients are auto-upgraded to
SlackV2 on first send via `update_report_schedule_slack_v2`"*. Rejecting names
at creation would have blocked the input that upgrade path exists to convert.
And it would have broken existing tests, which I should have run against
before opening:
-
`tests/unit_tests/reports/schemas_test.py::test_report_recipient_schema_slack_skips_email_validation`
loads `{"type": "Slack", "target": "#general"}` and asserts success.
- `tests/integration_tests/reports/api_tests.py` creates
`ReportRecipientType.SLACK` with `{"target": "channel"}` in three places.
The validator now returns early for anything that is not `SlackV2`, with a
comment recording why. My test asserting v1 rejection is replaced by one
asserting v1 **acceptance**, so the carve-out is pinned rather than incidental.
Re-checked against the real schema, loaded in a running instance:
| type | target | result |
|---|---|---|
| SlackV2 | `C08CSCSDCSY` | accepted |
| SlackV2 | `data-alerts` | rejected |
| SlackV2 | `C08CSCSDCSY,data-alerts` | rejected |
| Slack | `#general` | accepted |
| Slack | `channel` | accepted |
| Slack | `C08CSCSDCSY` | accepted |
| Email | `[email protected]` | accepted |
This also lines up with v1 being removed next major: the validation applies
only to the type that survives, and v1 recipients keep flowing through the
auto-upgrade untouched.
--
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]
