EnxDev commented on PR #42850:
URL: https://github.com/apache/superset/pull/42850#issuecomment-5339790496
## EnxDev's Review Agent โ apache/superset#42850 ยท HEAD e78ff93
comment โ the fix is sound and the v1 carve-out is correct and load-bearing,
but the "a name never delivers" premise only holds for attachment-bearing
sends, so this narrows the API contract further than the description says.
### ๐ด Functional
- **`superset/reports/schemas.py:185`** ยท _Medium_ โ The validator rejects a
channel name for every SlackV2 recipient, but `SlackV2Notification.send()` only
reaches `files_upload_v2` when the content carries a file (`slackv2.py:149`).
With no attachment it falls through to `chat_postMessage(channel=...)`
(`slackv2.py:160`), which resolves a channel name. Two supported configs land
there: `report_format: TEXT` (`commands/report/execute.py:1010`) and any alert
with `ALERTS_ATTACH_REPORTS=False` (`config.py:834`, default `True`). Those
SlackV2 recipients deliver today with a name and now get a 400 on `POST`/`PUT`
โ an API-only break, since the picker always submits ids. Either scope the
check to attachment-bearing formats, or keep the hard requirement (defensible:
a name silently breaks the moment the format flips to PNG) and say so in the
description plus an `UPDATING.md` entry, since it's a behavior change for
existing clients. **regression test:** `POST /api/v1/report/` with
`report_format: TEXT` and a SlackV2 recipient targeting a channel name โ pins
whichever contract you choose.
### ๐ก Should-fix
- **`superset/reports/schemas.py:209-216`** โ The message states an absolute
("a name is saved successfully and then never delivers") that is false for the
text-only path above, hardcodes a Slack UI navigation path that will drift, and
hands translators a six-clause paragraph. Same claim in the docstring at
`:186`. Cut to one sentence: name the offending part, say a channel id is
required, point at the dropdown.
- **`superset/reports/schemas.py:136`** โ `^[CGD][A-Z0-9]{6,}$` also rejects
user ids (`Uโฆ`/`Wโฆ`), which `chat_postMessage` accepts as a DM target on the
same no-attachment path. The `D` branch is already speculative โ
`/api/v1/report/slack_channels/` only returns public and private channels โ so
decide deliberately whether DM targets are in or out, and cover it either way.
- **CI has not run on HEAD.** `e78ff937` (today's master merge) carries 5
check runs: labeler and the docs preview. The Python suites and the Codecov
report are from `64f797e`, 9 days back, so "all checks have passed" does not
cover the current head or the integration test on it. Worth a push to
re-trigger before merge.
### ๐ Praise
- `superset/reports/schemas.py:187-193` โ The v1 carve-out is right for a
reason beyond the one in the comment: `NotificationMethod.tsx:688` flips to the
v1 free-text field when the channel fetch fails (`onError={() =>
setUseSlackV1(true)}`), so blanket validation would have broken the UI's own
fallback. Verified too that `update_report_schedule_slack_v2`
(`commands/report/execute.py:184-213`) resolves names to ids before writing, so
the upgrade path never persists a name as v2 โ the carve-out costs nothing.
- `tests/unit_tests/reports/schemas_test.py` โ The mixed-target test
asserting the good id is *absent* from the message is the right shape for an
error-text guarantee.
<!-- enxdev-review-agent:e78ff93 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD e78ff93._
--
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]
