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>
`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>
Previously every SlackApiError from the v2 probe was logged as "missing
channels:read", which mislabels invalid_auth/ratelimited/server errors as a
permission problem and sends operators chasing the wrong fix. Inspect the
SlackApiError response code and only emit the deprecation + scope-specific
guidance for the genuine scope-missing codes; for other Slack API errors,
log a generic "probe failed" line. Either branch still falls back to v1 so
the send isn't lost, matching the prior contract.
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>
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>
