chore(reports): deprecate Slack v1 and harden Slack v2 tests#39914
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39914 +/- ##
==========================================
- Coverage 64.63% 64.45% -0.18%
==========================================
Files 2685 2680 -5
Lines 148530 148015 -515
Branches 34267 34007 -260
==========================================
- Hits 95999 95401 -598
- Misses 50775 50849 +74
- Partials 1756 1765 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sadpandajoe
added
risk:breaking-change
0002901 to
a7a5d8e
Compare
a7a5d8e to
fe4c923
Compare
There was a problem hiding this comment.
Pull request overview
This PR deprecates the legacy Slack v1 integration for Alerts & Reports, flips ALERT_REPORT_SLACK_V2 to default True, and adds extensive unit test coverage around Slack v2 notification behavior and the v1→v2 upgrade decision path.
Changes:
- Default-enable Slack v2 via
ALERT_REPORT_SLACK_V2=Trueand document the deprecation/removal plan for Slack v1 (incl. operator action items). - Add once-per-process deprecation warning emitters for Slack v1 fallback paths and expand
should_use_v2_api()test coverage. - Add broad Slack v2 send-path unit tests (uploads, multi-channel fan-out, exception mapping, statsd gauges, logging context) and adjust one integration test for the new probe/upgrade behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
UPDATING.md |
Adds an operator-facing “Next” entry describing Slack v1 deprecation and Slack v2 defaults/scopes. |
superset/config.py |
Flips ALERT_REPORT_SLACK_V2 default to True and expands the flag’s inline description. |
docs/static/feature-flags.json |
Syncs the feature flag default/description to match config.py. |
superset/utils/slack.py |
Adds deprecation warning helpers and emits them from should_use_v2_api(); updates fallback warning text. |
superset/reports/notifications/slack.py |
Updates Slack v1 deprecation comment to reflect current removal plan/Slack API changes. |
superset/reports/notifications/slackv2.py |
Changes backoff decorator to retry the exception type actually raised by send(). |
tests/unit_tests/utils/slack_test.py |
Adds unit tests for should_use_v2_api() warning behavior and cache reset fixture. |
tests/unit_tests/reports/notifications/slack_tests.py |
Adds extensive Slack v2 send-path tests and a backoff-sleep skipping fixture. |
tests/integration_tests/reports/commands_tests.py |
Mocks the v2 probe/upgrade channel resolution for a Slack token callable integration test. |
87903a0 to
08a45de
Compare
dosubot
Bot
added
the
alert-reports
Code Review Agent Run #8ef6f1Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…True, harden v2 tests Flips the ALERT_REPORT_SLACK_V2 feature flag default to True so the v2 auto-upgrade path runs out of the box, and adds one-shot DeprecationWarning + logger.warning emissions when v1 still runs (flag explicitly off, or bot missing the channels:read scope). Slack retired the legacy files.upload endpoint in 2025, so v1 file uploads are already broken at the API level — only text-only chat_postMessage sends still succeed via the legacy path. The bulk of the change is bulletproof unit-test coverage for SlackV2Notification ahead of v1 removal in the next major: - files_upload_v2 invocation with PNG (single + multiple), CSV, and PDF, asserting channel, file, title, filename, and initial_comment kwargs - multi-channel fan-out (3 channels x 2 files = 6 uploads) and text-only multi-channel chat_postMessage - inline-file precedence (CSV beats screenshots beats PDF) - parametrized exception mapping across 7 slack_sdk error types -> the 4 NotificationException subclasses - statsd .ok and .warning gauge emission via the @statsd_gauge decorator - execution_id propagation from g.logs_context to the success log, plus the falsy g.logs_context fallback path - end-to-end auto-upgrade round-trip: v1 SLACK recipient with channel names raises SlackV1NotificationError -> update_report_schedule_slack_v2 rewrites the row to channel IDs -> SlackV2Notification fast-paths the next send with no further channel resolution - should_use_v2_api() warning behavior: deprecation warning emitted exactly once across multiple calls in both the flag-off and scope-missing paths, with the scope-missing logger.warning continuing to fire each call so operators see the actionable scope hint in their report-execution logs Also locks in current behavior of the @backoff.on_exception(SlackApiError, ...) decorator on send(): because send() catches every SlackApiError internally and re-raises as NotificationUnprocessableException, backoff never sees the target exception type and no retries actually fire. Test asserts call_count == 1 with a docstring marking this as a known design issue to address separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…test With ALERT_REPORT_SLACK_V2 now defaulting to True, a SLACK recipient's first send triggers the v1->v2 auto-upgrade, which calls get_channels_with_search to resolve channel names to channel IDs. The existing test mocked WebClient.conversations_list to return a plain dict that lacked the `.data` attribute the upgrade path reads, so the upgrade raised "'dict' object has no attribute 'data'" and the test errored. Patch get_channels_with_search directly (matching the pattern already used by the other v2-conversion tests in this file) so the upgrade can resolve channels without going through the WebClient mock plumbing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t errors The @backoff.on_exception decorator on SlackV2Notification.send() was configured to retry on SlackApiError, but the function's own try/except catches every SlackApiError and re-raises as NotificationUnprocessableException before the decorator can see it. As a result, no retries were happening — a single transient failure (rate limit, connection blip) would fail the report immediately, defeating the intent of the 5-attempt retry budget. Switch the decorator to retry on NotificationUnprocessableException, which is the exception type that send() actually raises for transient Slack failures (SlackApiError, SlackClientNotConnectedError, and the SlackClientError catch-all). Mirrors the working pattern already in webhook.py. Non-transient errors (NotificationParamException, NotificationMalformedException, NotificationAuthorizationException) still surface immediately — they aren't retryable and shouldn't be retried. Test changes: - Replaces the prior "locks in broken behavior" regression test with test_v2_send_retries_on_transient_slack_api_error asserting call_count == 5 - Adds test_v2_send_does_not_retry_param_errors verifying that BotUserAccessError → NotificationParamException is NOT retried (call_count == 1) - Adds an autouse fixture that patches backoff._sync.time.sleep so unit-test retries complete in milliseconds rather than the ~150s of real exponential backoff. Without this, the parametrized exception-mapping cases that map to NotificationUnprocessableException balloon the test runtime by ~75s The v1 SlackNotification has the same bug but is being deprecated in this release; not worth fixing there since v1's file_uploads endpoint is already dead at Slack's side and only the text-only chat_postMessage path still works. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-liner style Replaces the multi-section paragraph form with the single-bullet, PR-link-prefixed style used by the historical entries in this file (see the original Slack v2 deprecation in 4.1.0 / #29264). Same information, less ceremony. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code-review changes: - Replace module-level `_v1_*_warning_emitted` booleans with `functools.cache`- decorated `_emit_v1_*_deprecation` helpers. Bare module globals had a read-then-write race under multi-threaded WSGI workers; functools.cache is thread-safe under the GIL and produces actually-once-per-process semantics without the noqa: PLW0603 escape hatch. - Mention `groups:read` (in addition to `channels:read`) wherever the scope requirement appears: deprecation message constant, config.py comment, the scope-missing logger.warning, UPDATING.md, and (auto-synced) feature-flags.json. The v2 channel resolver queries both public_channel and private_channel types, so granting only `channels:read` silently breaks private-channel reports. - Add `test_propagates_non_slack_api_errors_from_probe` — locks in that any exception other than SlackApiError (network, transport) propagates out of should_use_v2_api rather than masquerading as a missing-scope warning. - Drop a tautological `assert_not_called()` on `get_channels_with_search` in the auto-upgrade round-trip test. SlackV2Notification.send() never calls that helper in any path, so the assertion was true by construction rather than by the test exercising a real fast path. - Pin assertions on the deprecation-warning *message* to the exported `_SLACK_V1_DEPRECATION_MESSAGE` constant instead of substring fragments. - Update the test autouse fixture to clear the new functools.cache caches rather than reset the now-removed module globals. Three architectural concerns from review (auto-upgrade transaction race, concurrent worker upgrade race, end-of-deprecation cleanup migration) are pre-existing on the upgrade path and tracked as separate follow-up tasks rather than expanded into this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…esponse `should_use_v2_api` only read the probe error code via `response.data`, so a `SlackApiError` carrying a plain-dict response (as the SDK and the unit tests produce) resolved to an empty code, skipped the scope-missing branch, and never emitted the v1 DeprecationWarning. Read the code from `.data` when present and fall back to the response dict itself, fixing test_returns_false_when_scope_missing_and_emits_deprecation_once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Close two gaps in the Slack v1-deprecation tests surfaced in review: - should_use_v2_api scope detection was only exercised with a plain-dict response; the production-default path reads the error code via getattr(response, "data") on a real SlackResponse. Add a case using the existing MockResponse helper so that branch is covered. - The backoff switch to NotificationUnprocessableException is meant to let a transient failure retry and ultimately succeed; only persistent-fail (5x) and never-retry were tested. Add a fail-twice-then-succeed case asserting 3 attempts and the success gauge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
59f9beb to
b4ce1ff
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #b90a32
Actionable Suggestions - 2
-
tests/unit_tests/reports/notifications/slack_tests.py - 2
- Unverified feature flag patch · Line 355-355
- Unused mock parameter · Line 928-928
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/reports/notifications/slack.py - 1
- Misleading doc on v2 upgrade trigger · Line 59-60
Review Details
-
Files reviewed - 7 · Commit Range:
3a46222..b4ce1ff- superset/config.py
- superset/reports/notifications/slack.py
- superset/reports/notifications/slackv2.py
- superset/utils/slack.py
- tests/integration_tests/reports/commands_tests.py
- tests/unit_tests/reports/notifications/slack_tests.py
- tests/unit_tests/utils/slack_test.py
-
Files skipped - 2
- UPDATING.md - Reason: Filter setting
- docs/static/feature-flags.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
There was a problem hiding this comment.
Code Review Agent Run #6adeae
Actionable Suggestions - 1
-
superset/reports/notifications/slackv2.py - 1
- Duplicate Slack error handling · Line 158-158
Review Details
-
Files reviewed - 5 · Commit Range:
b4ce1ff..3f60bf2- superset/config.py
- superset/reports/notifications/slackv2.py
- superset/utils/slack.py
- tests/unit_tests/reports/notifications/slack_tests.py
- tests/unit_tests/utils/slack_test.py
-
Files skipped - 2
- UPDATING.md - Reason: Filter setting
- docs/static/feature-flags.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
…test coverage Address three review findings on the Slack v1 deprecation path: - utils/slack.py: should_use_v2_api() now catches the SDK base SlackClientError (aliased to avoid the local class) so probe failures like SlackClientNotConnectedError fall back to v1 instead of escaping raw and aborting the recipient loop in _send(). Non-SDK errors still propagate. - commands/report/execute.py: update_report_schedule_slack_v2() resolves all Slack recipients into a temporary list and swaps type/config only after every recipient resolves, so a mid-loop failure can no longer persist a partially upgraded schedule. - slack_tests.py: add an SDK-faithful SlackResponse (real status_code) so the 429/5xx retry branch in _give_up_slack_api_retry is exercised; the prior plain-dict mocks left it uncovered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review Agent Run #1ff323Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…-10b01d # Conflicts: # superset/commands/report/execute.py # superset/utils/slack.py # tests/unit_tests/commands/report/execute_test.py # tests/unit_tests/utils/slack_test.py
|
|
||
| The migration is transactional (all-or-nothing) and idempotent — it can be safely re-run or resumed. Note that AES-GCM, unlike AES-CBC, does not support querying directly over encrypted columns; audit any code that filters on an encrypted column before switching. See the SIP at `docs/sip/authenticated-encryption-at-rest.md` for details. | ||
|
|
||
| - [39914](https://github.com/apache/superset/pull/39914) `ALERT_REPORT_SLACK_V2` now defaults to `True` and the legacy Slack v1 integration (`Slack` recipient type, `files.upload` API) is deprecated for removal in the next major. Slack retired `files.upload` in 2025, so v1 file-bearing sends already fail at the API level — only text-only `chat_postMessage` still works via the legacy path. Grant your Slack bot the `channels:read` scope (and `groups:read` if you use private channels) so existing `Slack` recipients can be auto-upgraded to `SlackV2` on next send. Operators who explicitly override the flag to `False` will see a one-shot `DeprecationWarning` plus a `logger.warning`; remove the override or grant the scopes to clear it. |
There was a problem hiding this comment.
Posting on Elizabeth's behalf — this is her PR reviewer agent. Forward any pushback to her and she'll loop me back in.
Quick clarification on the files.upload framing in the PR description and UPDATING.md — my understanding is that Slack deprecated files.upload for new apps created after the cutoff, but existing/legacy apps that were already using it before that date are grandfathered and the endpoint still works for them. The PR description says "Slack retired the files.upload endpoint in 2025" and the UPDATING.md entry repeats this, which reads as a universal retirement.
If that distinction is right, the claim that "v1 file-bearing sends already fail at the API level" is only true for newer app installs — operators running older Slack apps may still be getting working file uploads via v1 today. That affects how urgent the migration feels to those operators.
Could you confirm whether the retirement is app-creation-date-gated, and if so, would it be worth softening the wording in UPDATING.md to something like "Slack deprecated files.upload for new apps in 2025 and has announced its eventual full retirement" rather than stating it as already done universally? Happy to be corrected if I have the timeline wrong.
There was a problem hiding this comment.
You're right that there was a new-vs-existing split — that's worth being precise about, so I've tightened the wording.
Timeline: Slack stopped letting new apps register files.upload on May 16, 2024, but existing apps were grandfathered, so legacy integrations kept working past that date. The sunset for existing apps was originally announced for March 2025, then pushed back, and files.upload was finally retired for all apps on November 12, 2025. So there's no grandfathering left — every app, new or old, now gets an API error on files.upload, which is why Superset's v1 file-bearing sends already fail rather than "will eventually."
I went a bit more specific than the "eventual retirement" framing precisely because that window has since closed. Updated the entry to call out both dates and the new-vs-existing distinction.
Refs: https://docs.slack.dev/changelog/2024/05/16/apps/ (new-app cutoff) and https://docs.slack.dev/changelog/2025/03/17/files-upload-extension/ (final Nov 12 2025 retirement).
Add precise dates and the new-vs-existing-apps distinction to the UPDATING.md Slack v2 entry: new apps were blocked from files.upload in May 2024 and the method was fully retired for all apps on November 12, 2025. Keeps the existing "v1 file-bearing sends now fail at the API level" conclusion, which holds now that retirement is universal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The v1 SlackNotification deprecation comment said recipients with the `channels:read` scope are auto-upgraded to SlackV2, but the v2 probe (`should_use_v2_api`) lists both public and private channel types and so requires `channels:read` and `groups:read`. Name both scopes to match the deprecation message and the probe's log line. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review Agent Run #f13d86Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…-10b01d # Conflicts: # UPDATING.md
| @@ -34,12 +36,42 @@ | |||
|
|
|||
| logger = logging.getLogger(__name__) | |||
|
|
|||
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation to this module-level constant to satisfy the type-hint requirement for relevant variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new module-level constant is unannotated even though its type is clear and stable, so it matches the rule requiring type hints on newly added Python variables that can be annotated.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/slack.py
**Line:** 38:38
**Comment:**
*Custom Rule: Add an explicit type annotation to this module-level constant to satisfy the type-hint requirement for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| def test_slack_token_callable_chart_report( | ||
| screenshot_mock, | ||
| slack_client_mock_class, | ||
| get_channels_with_search_mock, | ||
| create_report_slack_chart, | ||
| ): |
There was a problem hiding this comment.
Suggestion: Add a type annotation for the newly added get_channels_with_search_mock parameter and ensure the modified test function includes explicit type hints (including return type). [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The modified test function test_slack_token_callable_chart_report still omits type hints on its parameters and has no return type annotation, which violates the Python type-hints rule for newly added or modified code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/reports/commands_tests.py
**Line:** 1990:1995
**Comment:**
*Custom Rule: Add a type annotation for the newly added `get_channels_with_search_mock` parameter and ensure the modified test function includes explicit type hints (including return type).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| @pytest.fixture(autouse=True) | ||
| def _skip_backoff_sleep(): | ||
| """Make any @backoff.on_exception retries instant. | ||
|
|
||
| SlackV2Notification.send() retries up to 5 times with `backoff.expo(factor=10, | ||
| base=2)` — that's ~150s of real sleep on a persistently-failing send. We | ||
| don't care about the wall-clock waits in unit tests; patching `time.sleep` | ||
| inside backoff's sync runner keeps the assertion semantics (call_count, | ||
| raised exception type) without the wait. | ||
| """ | ||
| with patch("backoff._sync.time.sleep"): | ||
| yield |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this fixture function to satisfy the type-hint requirement. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a new Python fixture function with no return type annotation, which violates the type-hint requirement for modified code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/reports/notifications/slack_tests.py
**Line:** 49:60
**Comment:**
*Custom Rule: Add an explicit return type annotation to this fixture function to satisfy the type-hint requirement.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| def test_v2_inline_files_precedence(mock_header_data) -> None: | ||
| """CSV beats screenshots beats PDF; only one inline-file type is sent.""" | ||
| content = _make_content( | ||
| mock_header_data, | ||
| csv=b"a,b\n1,2", | ||
| screenshots=[b"shot-1"], | ||
| pdf=b"%PDF", | ||
| ) | ||
| notification = _make_v2_notification(content, target="C12345") | ||
| file_type, files = notification._get_inline_files() | ||
| assert file_type == "csv" | ||
| assert files == [b"a,b\n1,2"] | ||
|
|
||
| content = _make_content( | ||
| mock_header_data, | ||
| screenshots=[b"shot-1"], | ||
| pdf=b"%PDF", | ||
| ) | ||
| notification = _make_v2_notification(content, target="C12345") | ||
| file_type, files = notification._get_inline_files() | ||
| assert file_type == "png" | ||
| assert files == [b"shot-1"] | ||
|
|
||
| content = _make_content(mock_header_data, pdf=b"%PDF") | ||
| notification = _make_v2_notification(content, target="C12345") | ||
| file_type, files = notification._get_inline_files() | ||
| assert file_type == "pdf" | ||
| assert files == [b"%PDF"] |
There was a problem hiding this comment.
Suggestion: Add a type annotation for the mock_header_data test parameter. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The test parameter mock_header_data is unannotated even though it can be typed, so this is a real type-hint omission under the rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/reports/notifications/slack_tests.py
**Line:** 650:677
**Comment:**
*Custom Rule: Add a type annotation for the `mock_header_data` test parameter.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| def test_returns_false_when_scope_missing_and_emits_deprecation_once(self, mocker): | ||
| mocker.patch( | ||
| "superset.utils.slack.feature_flag_manager.is_feature_enabled", | ||
| return_value=True, | ||
| ) | ||
| mock_client = mocker.Mock() | ||
| # The Slack SDK exposes the error code as `response["error"]`; that is | ||
| # what `should_use_v2_api` branches on to decide whether the v1 | ||
| # deprecation warning is the appropriate signal. | ||
| mock_client.conversations_list.side_effect = SlackApiError( | ||
| message="missing_scope", response={"ok": False, "error": "missing_scope"} | ||
| ) | ||
| mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) | ||
| logger_mock = mocker.patch("superset.utils.slack.logger") | ||
|
|
||
| with warnings.catch_warnings(record=True) as caught: | ||
| warnings.simplefilter("always") | ||
| assert should_use_v2_api() is False | ||
| assert should_use_v2_api() is False | ||
| assert should_use_v2_api() is False | ||
|
|
||
| deprecation_warnings = [ | ||
| w for w in caught if issubclass(w.category, DeprecationWarning) | ||
| ] | ||
| # DeprecationWarning emitted exactly once across multiple calls. | ||
| assert len(deprecation_warnings) == 1 | ||
| assert str(deprecation_warnings[0].message) == _SLACK_V1_DEPRECATION_MESSAGE | ||
| # The user-visible scope-missing log fires every time, since operators | ||
| # need to see the actionable message in their report-execution logs. | ||
| assert logger_mock.warning.call_count == 3 |
There was a problem hiding this comment.
Suggestion: Add missing type annotations to this method signature so all parameters and the return type are explicitly typed. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This added test method lacks explicit annotations for its parameters and return value, which is exactly the type-hint omission the rule flags.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/slack_test.py
**Line:** 370:399
**Comment:**
*Custom Rule: Add missing type annotations to this method signature so all parameters and the return type are explicitly typed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| def test_scope_missing_detected_via_slack_response_data_shape(self, mocker): | ||
| """The real Slack SDK sets `SlackApiError.response` to a `SlackResponse` | ||
| whose payload lives in `.data` — not a plain dict. This is the | ||
| production-default code path, so it must be exercised directly: | ||
| `should_use_v2_api` reads the error code via `getattr(response, "data")` | ||
| and the scope-missing branch must still fire. | ||
| """ | ||
| mocker.patch( | ||
| "superset.utils.slack.feature_flag_manager.is_feature_enabled", | ||
| return_value=True, | ||
| ) | ||
| mock_client = mocker.Mock() | ||
| # MockResponse mirrors SlackResponse: the error payload is on `.data`, | ||
| # exactly as the live SDK delivers it. | ||
| mock_client.conversations_list.side_effect = SlackApiError( | ||
| message="missing_scope", | ||
| response=MockResponse({"ok": False, "error": "missing_scope"}), | ||
| ) | ||
| mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) | ||
| logger_mock = mocker.patch("superset.utils.slack.logger") | ||
|
|
||
| with warnings.catch_warnings(record=True) as caught: | ||
| warnings.simplefilter("always") | ||
| assert should_use_v2_api() is False | ||
|
|
||
| deprecation_warnings = [ | ||
| w for w in caught if issubclass(w.category, DeprecationWarning) | ||
| ] | ||
| assert len(deprecation_warnings) == 1 | ||
| assert logger_mock.warning.call_count == 1 | ||
| assert "channels:read" in logger_mock.warning.call_args.args[0] | ||
|
|
There was a problem hiding this comment.
Suggestion: Provide complete type hints on this new test method signature, including typing for injected fixtures and the return value. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This newly added test method is missing type hints on its parameters and return type, so it violates the stated Python type-hint rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/slack_test.py
**Line:** 404:435
**Comment:**
*Custom Rule: Provide complete type hints on this new test method signature, including typing for injected fixtures and the return value.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
LGTM! Nice cleanup on the atomic v1→v2 upgrade path, and the per-call retry helper is a real fix — the old decorator never actually fired since send() swallowed the SlackApiError before backoff could see it. Test coverage here is thorough, and thanks for tightening up the files.upload timeline wording per Elizabeth's note.
One non-blocking nit: the PR description (and one test docstring) still describe the retry fix as swapping the decorator's exception type, but the final code retries at the per-API-call level via _call_slack_api — worth a quick edit for posterity, but not holding this up over it.
github-project-automation
Bot
moved this from Propose for Lazy Consensus
to Lazy Consensus Reached
in Apache Superset 7.0
SUMMARY
Deprecates the legacy Slack v1 integration for Alerts and Reports and adds bulletproof unit-test coverage for
SlackV2Notificationahead of v1 removal in the next major.Why now. Slack retired the
files.uploadendpoint in 2025, so v1 sends that include screenshots, CSVs, or PDFs already fail at the API level — only text-onlychat_postMessagesends still succeed via the legacy path. Existing recipients with thechannels:readscope already auto-upgrade toSlackV2on first send via theupdate_report_schedule_slack_v2flow; the only thing keeping the upgrade from running out of the box wasALERT_REPORT_SLACK_V2defaulting toFalse.What this change does:
ALERT_REPORT_SLACK_V2default toTrue(the docs JSON re-syncs fromconfig.pyautomatically via thefeature-flags-syncpre-commit hook).DeprecationWarning+logger.warningemissions when the v1 path still runs — both the flag-off and the missing-channels:readpaths. The scope-missinglogger.warningcontinues to fire every send so operators see the actionable scope hint in report-execution logs.# TODO: Remove in 6.0.0comment onSlackNotificationwith an accurate deprecation note (6.0 already shipped).## Nextwith operator action items (grantchannels:read, recipients auto-upgrade on next send, v1 removal targeted for the next major).Bulletproof v2 test coverage added in
tests/unit_tests/reports/notifications/slack_tests.pyandtests/unit_tests/utils/slack_test.py:files_upload_v2invocation with PNG (single + multiple), CSV, and PDF — assertingchannel,file,title,filename, andinitial_commentchat_postMessageslack_sdkerror types → the 4NotificationExceptionsubclasses.okand.warninggauge emission via the@statsd_gaugedecoratorexecution_idpropagation fromg.logs_contextto the success log, plus the falsy-fallback pathSLACKrecipient with channel names →SlackV1NotificationError→update_report_schedule_slack_v2rewrites the row to channel IDs → freshSlackV2Notificationfast-paths the next send with no resolver callshould_use_v2_api()warning behavior:DeprecationWarningemitted exactly once across multiple calls in both flag-off and scope-missing pathsRetry bug fixed (intended, in scope):
SlackV2Notification.send()was decorated with@backoff.on_exception(SlackApiError, ..., max_tries=5), but the function catches everySlackApiErrorinternally and re-raises it asNotificationUnprocessableExceptionbefore backoff can see it — so no retries ever fired and a single transient blip failed the report immediately. This PR switches the decorator to retry onNotificationUnprocessableException(the typesend()actually raises for transient Slack failures), mirroring the working pattern inwebhook.py. Transient failures now retry up to 5 times with exponential backoff (~150s worst case, matching the existingwebhook.pybudget); non-transient errors (NotificationParamException,NotificationMalformedException,NotificationAuthorizationException) still surface immediately with no retry. Locked in bytest_v2_send_retries_on_transient_slack_api_error(persistent failure → 5 attempts),test_v2_send_retries_then_succeeds_on_transient_failure(fail twice then succeed → 3 attempts), andtest_v2_send_does_not_retry_param_errors(permanent error → 1 attempt).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no UI changes.
TESTING INSTRUCTIONS
# Unit tests PYTHONPATH=/path/to/superset_core python3 -m pytest \ tests/unit_tests/reports/ \ tests/unit_tests/commands/report/ \ tests/unit_tests/utils/slack_test.pyAll 319 tests pass on this branch (29 in
slack_tests.py, 14 inutils/slack_test.py, plus the existing report unit suite).To validate the deprecation behavior manually:
ALERT_REPORT_SLACK_V2: Falseset explicitly insuperset_config.pyand a Slack recipient configured, fire a report. You should see a singleDeprecationWarningandlogger.warningline on the first send, then no more for that process.True) and a Slack bot lackingchannels:read, fire a report. You should seelogger.warningdescribing the missing scope on every send, plus a one-shotDeprecationWarning.channels:read, the first send for aSLACK-type recipient auto-upgrades the row toSLACKV2with channel IDs (existing behavior — now exercised by the round-trip unit test).ADDITIONAL INFORMATION
ALERT_REPORT_SLACK_V2default flipped toTrue🤖 Generated with Claude Code