sadpandajoe commented on code in PR #39914:
URL: https://github.com/apache/superset/pull/39914#discussion_r3508331626
##########
superset/utils/slack.py:
##########
@@ -224,21 +256,75 @@ def get_channels_with_search(
return channels
+_SCOPE_MISSING_ERROR_CODES = frozenset(
+ {"missing_scope", "not_allowed_token_type", "no_permission"}
+)
Review Comment:
`_SCOPE_MISSING_ERROR_CODES` is a module-level `frozenset` literal; mypy
already infers `frozenset[str]`, and the membership test on line 296
type-checks fine. This matches the other unannotated constants in the module,
so no change is warranted; mypy/ruff pass.
##########
superset/utils/slack.py:
##########
@@ -224,21 +256,75 @@ def get_channels_with_search(
return channels
+_SCOPE_MISSING_ERROR_CODES = frozenset(
+ {"missing_scope", "not_allowed_token_type", "no_permission"}
+)
+
+
def should_use_v2_api() -> bool:
if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_SLACK_V2"):
+ _emit_v1_flag_off_deprecation()
return False
try:
client = get_slack_client()
+ extra_params = {"types": _SLACK_CONVERSATION_TYPES}
Review Comment:
Same as the `_get_channels` case: `extra_params` in the `should_use_v2_api`
probe is a local dict that mypy infers as `dict[str, str]`, and the
`**extra_params` call into `conversations_list` type-checks fine. A local
annotation isn't warranted, so skipping; mypy/ruff pass.
##########
tests/unit_tests/commands/report/execute_test.py:
##########
@@ -1309,73 +1309,109 @@ def
test_update_recipient_to_slack_v2_missing_channels(mocker: MockerFixture):
mock_cmmd.update_report_schedule_slack_v2()
-def test_update_recipient_to_slack_v2_reverts_all_on_partial_failure(
+def test_update_recipient_to_slack_v2_multiple_recipients(
mocker: MockerFixture,
) -> None:
- """
- When the second of two Slack recipients fails channel resolution, BOTH
- recipients are fully reverted — type AND exact original
- ``recipient_config_json`` string — not just the loop variable's type. This
- prevents the intervening ``create_log`` commit from flushing a
half-migrated,
- inconsistent state.
+ """All Slack recipients are upgraded atomically when every channel
resolves."""
+
+ def fake_get_channels(search_string, types, exact_match):
Review Comment:
The `fake_get_channels(search_string, types, exact_match)` helper is a
test-local mock `side_effect` inside a unit test. The project intentionally
relaxes typing for tests via the `[[tool.mypy.overrides]]` block for `module =
"tests.*"` in pyproject.toml (lines 277-280), which sets `check_untyped_defs`,
`disallow_untyped_calls`, and `disallow_untyped_defs` all to false. mypy and
ruff already pass green on this branch without annotating this helper, so I'll
skip this stylistic nit.
--
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]
