sadpandajoe commented on code in PR #42484:
URL: https://github.com/apache/superset/pull/42484#discussion_r3824173451
##########
tests/unit_tests/utils/slack_test.py:
##########
@@ -164,6 +164,61 @@ def test_handle_slack_client_error_listing_channels(self,
mocker):
The server responded with: missing scope: channels:read"""
)
+ @pytest.mark.parametrize(
+ "error_code",
+ [
+ "not_authed",
+ "invalid_auth",
+ "account_inactive",
+ "token_revoked",
+ "token_expired",
+ ],
+ )
+ def test_logs_slack_api_error_at_warning_not_error(self, error_code: str,
mocker):
+ """An expired/revoked/inactive bot token is an expected multi-tenant
+ config state that is already handled end-to-end (re-raised as a
+ ``SupersetException`` and turned into a 422), so it should be logged
+ at WARNING, not ERROR, to avoid polluting Sentry."""
+ from superset.exceptions import SupersetException
+
+ mock_client = mocker.Mock()
+ mock_client.conversations_list.side_effect = SlackApiError(
+ message="foo", response={"ok": False, "error": error_code}
+ )
+ mocker.patch("superset.utils.slack.get_slack_client",
return_value=mock_client)
+ logger_mock = mocker.patch("superset.utils.slack.logger")
+
+ with pytest.raises(SupersetException):
+ get_channels_with_search()
+
+ logger_mock.error.assert_not_called()
+ logger_mock.warning.assert_called_once()
Review Comment:
The warning-path test does not assert that the traceback is omitted, so
changing this call to `logger.warning(..., exc_info=True)` would still pass
even though dropping the traceback is part of the intended behavior. Could this
also assert that `exc_info` is absent or false?
##########
superset/utils/slack.py:
##########
@@ -139,6 +139,25 @@ def get_channels(
return _get_channels(cache_key, team_id=team_id, **kwargs)
+_AUTH_ERROR_CODES = frozenset(
+ {"not_authed", "invalid_auth", "account_inactive", "token_revoked",
"token_expired"}
+)
+
+
+def _get_slack_error_code(ex: SlackApiError) -> str:
Review Comment:
This PR currently conflicts with `master`, which already exposes
`get_slack_api_error_code()` for this response-shape parsing and uses it from
another Slack module. Resolving the conflict by keeping this private copy
leaves two helpers that can drift; could you rebase and apply the logging split
through the shared helper?
--
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]
