sadpandajoe commented on code in PR #39914: URL: https://github.com/apache/superset/pull/39914#discussion_r3508326299
########## superset/utils/slack.py: ########## @@ -34,6 +36,33 @@ logger = logging.getLogger(__name__) +_SLACK_V1_DEPRECATION_MESSAGE = ( + "Slack v1 (the legacy `Slack` recipient type and `files.upload` API) is " + "deprecated and will be removed in the next major release. Slack retired " + "the `files.upload` endpoint in 2025, so v1 file uploads no longer work; " + "only text-only `chat_postMessage` sends still succeed. Grant your Slack " + "bot the `channels:read` (and `groups:read` if you use private channels) " + "scopes so existing v1 recipients can be auto-upgraded to SlackV2 on " + "their next send." +) + + +# functools.cache gives us a process-lifetime, thread-safe one-shot guard +# without the read-then-write race that bare module globals would have under +# multi-threaded WSGI workers. The cached return value (None) is irrelevant — +# we only care that the body executes at most once per process. [email protected] +def _emit_v1_flag_off_deprecation() -> None: + warnings.warn(_SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=3) + logger.warning( + "ALERT_REPORT_SLACK_V2 is disabled; %s", _SLACK_V1_DEPRECATION_MESSAGE + ) Review Comment: The four-line comment block directly above `_emit_v1_flag_off_deprecation` (lines 50-53) already explains the `functools.cache` one-shot-guard rationale and that it fires the v1 deprecation warning/log at most once per process, and the name is self-describing. A docstring would just restate that; mypy/ruff pass, so skipping. ########## superset/utils/slack.py: ########## @@ -34,6 +36,33 @@ logger = logging.getLogger(__name__) +_SLACK_V1_DEPRECATION_MESSAGE = ( + "Slack v1 (the legacy `Slack` recipient type and `files.upload` API) is " + "deprecated and will be removed in the next major release. Slack retired " + "the `files.upload` endpoint in 2025, so v1 file uploads no longer work; " + "only text-only `chat_postMessage` sends still succeed. Grant your Slack " + "bot the `channels:read` (and `groups:read` if you use private channels) " + "scopes so existing v1 recipients can be auto-upgraded to SlackV2 on " + "their next send." +) + + +# functools.cache gives us a process-lifetime, thread-safe one-shot guard +# without the read-then-write race that bare module globals would have under +# multi-threaded WSGI workers. The cached return value (None) is irrelevant — +# we only care that the body executes at most once per process. [email protected] +def _emit_v1_flag_off_deprecation() -> None: + warnings.warn(_SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=3) + logger.warning( + "ALERT_REPORT_SLACK_V2 is disabled; %s", _SLACK_V1_DEPRECATION_MESSAGE + ) + + [email protected] +def _emit_v1_scope_missing_deprecation() -> None: + warnings.warn(_SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=3) + Review Comment: `_emit_v1_scope_missing_deprecation` is a two-line private helper that emits the shared `_SLACK_V1_DEPRECATION_MESSAGE` once per process; it mirrors `_emit_v1_flag_off_deprecation`, whose rationale is covered by the comment block above (lines 50-53), and the name is self-describing. No docstring needed and mypy/ruff pass, so skipping. ########## tests/integration_tests/reports/commands_tests.py: ########## @@ -1978,11 +1978,13 @@ def test_slack_chart_alert_no_attachment(email_mock, create_alert_email_chart): "load_birth_names_dashboard_with_slices", "create_report_slack_chart", ) +@patch("superset.commands.report.execute.get_channels_with_search") @patch("superset.utils.slack.WebClient") @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") def test_slack_token_callable_chart_report( screenshot_mock, slack_client_mock_class, + get_channels_with_search_mock, create_report_slack_chart, Review Comment: This is the `create_report_slack_chart` pytest fixture parameter of `test_slack_token_callable_chart_report`. Per the `[[tool.mypy.overrides]] module = "tests.*"` block in pyproject.toml (`disallow_untyped_defs = false`, `disallow_untyped_calls = false`, `check_untyped_defs = false`), test functions in this suite intentionally don't require parameter annotations, and both mypy and ruff already pass on this branch. Annotating fixture parameters isn't the convention here, so no change is warranted. ########## tests/unit_tests/reports/notifications/slack_tests.py: ########## @@ -431,3 +465,524 @@ def test_slack_mixin_get_body_truncates_large_table( ) body = notification._get_body(content=content) assert "(table was truncated)" in body + + +# --------------------------------------------------------------------------- +# Bulletproof v2 send-path coverage +# +# The tests above exercise the chat_postMessage path (text-only sends). The +# tests below cover files_upload_v2 across screenshots/CSV/PDF, multi-channel +# fan-out, exception mapping, backoff, statsd, and logs propagation. Together +# they guarantee that every observable behavior of SlackV2Notification.send() +# is locked down before Slack v1 is removed. +# --------------------------------------------------------------------------- + + +def _make_v2_notification(content, target: str = "C12345"): Review Comment: `_make_v2_notification` is a test-local helper. Annotating its `content` parameter and return type isn't required because the `[[tool.mypy.overrides]] module = "tests.*"` override in pyproject.toml sets `disallow_untyped_defs = false`, and mypy, ruff, and ruff-format all pass as-is. Skipping. -- 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]
