diff --git a/CHANGELOG.md b/CHANGELOG.md index 7acfee7e..bb99cd31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add option to create new contact point for existing integrations ([#2909](https://github.com/grafana/oncall/issues/2909)) +### Changed + +- Handle slack resolution note errors consistently ([#2976](https://github.com/grafana/oncall/pull/2976)) + ## v1.3.35 (2023-09-05) ### Fixed diff --git a/engine/apps/slack/scenarios/resolution_note.py b/engine/apps/slack/scenarios/resolution_note.py index f50535c4..f6c640d6 100644 --- a/engine/apps/slack/scenarios/resolution_note.py +++ b/engine/apps/slack/scenarios/resolution_note.py @@ -30,6 +30,48 @@ logger = logging.getLogger(__name__) logger.setLevel(logging.DEBUG) +def handle_resolution_note_message_exception(step, action_name, exc): + """Check common API errors when updating a resolution note message.""" + if exc.response["error"] == "channel_not_found": + logger.warning( + f"Unable to {action_name} resolution note message in slack. " + f"Slack team identity pk: {step.slack_team_identity.pk}.\n" + f"Reason: 'channel_not_found'" + ) + elif exc.response["error"] == "message_not_found": + logger.warning( + f"Unable to {action_name} resolution note message in slack. " + f"Slack team identity pk: {step.slack_team_identity.pk}.\n" + f"Reason: 'message_not_found'" + ) + elif exc.response["error"] == "is_archived": + logger.warning( + f"Unable to {action_name} resolution note message in slack. " + f"Slack team identity pk: {step.slack_team_identity.pk}.\n" + f"Reason: 'is_archived'" + ) + elif exc.response["error"] == "invalid_auth": + logger.warning( + f"Unable to {action_name} resolution note message in slack. " + f"Slack team identity pk: {step.slack_team_identity.pk}.\n" + f"Reason: 'invalid_auth'" + ) + elif exc.response["error"] == "account_inactive": + logger.warning( + f"Unable to {action_name} resolution note message in slack. " + f"Slack team identity pk: {step.slack_team_identity.pk}.\n" + f"Reason: 'account_inactive'" + ) + elif exc.response["error"] == "is_inactive": + logger.warning( + f"Unable to {action_name} resolution note message in slack. " + f"Slack team identity pk: {step.slack_team_identity.pk}.\n" + f"Reason: 'is_inactive'" + ) + else: + raise exc + + class AddToResolutionNoteStep(scenario_step.ScenarioStep): callback_id = [ "add_resolution_note", @@ -189,38 +231,7 @@ class UpdateResolutionNoteStep(scenario_step.ScenarioStep): ts=resolution_note_slack_message.ts, ) except SlackAPIException as e: - if e.response["error"] == "channel_not_found": - logger.warning( - f"Unable to delete resolution note message in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'channel_not_found'" - ) - elif e.response["error"] == "message_not_found": - logger.warning( - f"Unable to delete resolution note message in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'message_not_found'" - ) - elif e.response["error"] == "is_archived": - logger.warning( - f"Unable to delete resolution note message in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'is_archived'" - ) - elif e.response["error"] == "invalid_auth": - logger.warning( - f"Unable to delete resolution note message in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'invalid_auth'" - ) - elif e.response["error"] == "is_inactive": - logger.warning( - f"Unable to delete resolution note message in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'is_inactive'" - ) - else: - raise e + handle_resolution_note_message_exception(self, "delete", e) else: self.remove_resolution_note_reaction(resolution_note_slack_message) @@ -241,26 +252,7 @@ class UpdateResolutionNoteStep(scenario_step.ScenarioStep): blocks=blocks, ) except SlackAPIException as e: - if e.response["error"] == "channel_not_found": - logger.warning( - f"Unable to post resolution note message to slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'channel_not_found'" - ) - elif e.response["error"] == "is_archived": - logger.warning( - f"Unable to post resolution note message to slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'is_archived'" - ) - elif e.response["error"] == "invalid_auth": - logger.warning( - f"Unable to post resolution note message to slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'invalid_auth'" - ) - else: - raise e + handle_resolution_note_message_exception(self, "post", e) else: message_ts = result["message"]["ts"] result_permalink = self._slack_client.chat_getPermalink( @@ -294,38 +286,7 @@ class UpdateResolutionNoteStep(scenario_step.ScenarioStep): blocks=blocks, ) except SlackAPIException as e: - if e.response["error"] == "channel_not_found": - logger.warning( - f"Unable to update resolution note message in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'channel_not_found'" - ) - elif e.response["error"] == "message_not_found": - logger.warning( - f"Unable to update resolution note message in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'message_not_found'" - ) - elif e.response["error"] == "invalid_auth": - logger.warning( - f"Unable to update resolution note message in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'invalid_auth'" - ) - elif e.response["error"] == "is_inactive": - logger.warning( - f"Unable to update resolution note message in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'is_inactive'" - ) - elif e.response["error"] == "account_inactive": - logger.warning( - f"Unable to update resolution note message in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'account_inactive'" - ) - else: - raise e + handle_resolution_note_message_exception(self, "update", e) else: resolution_note_slack_message.text = resolution_note.text resolution_note_slack_message.save(update_fields=["text"]) diff --git a/engine/apps/slack/scenarios/slack_channel_integration.py b/engine/apps/slack/scenarios/slack_channel_integration.py index b3e2a6d7..b24f8040 100644 --- a/engine/apps/slack/scenarios/slack_channel_integration.py +++ b/engine/apps/slack/scenarios/slack_channel_integration.py @@ -1,7 +1,9 @@ import logging import typing +from apps.slack.client import SlackAPIException from apps.slack.scenarios import scenario_step +from apps.slack.scenarios.resolution_note import handle_resolution_note_message_exception from apps.slack.types import EventPayload, EventType, MessageEventSubtype, PayloadType, ScenarioRoute if typing.TYPE_CHECKING: @@ -75,7 +77,12 @@ class SlackChannelMessageEventStep(scenario_step.ScenarioStep): # SlackMessage instances without alert_group set (e.g., SSR Slack messages) return - result = self._slack_client.chat_getPermalink(channel=channel, message_ts=message_ts) + try: + result = self._slack_client.chat_getPermalink(channel=channel, message_ts=message_ts) + except SlackAPIException as e: + handle_resolution_note_message_exception(self, "save thread message", e) + return + permalink = None if result["permalink"] is not None: permalink = result["permalink"] diff --git a/engine/apps/slack/tests/test_scenario_steps/test_slack_channel_integration.py b/engine/apps/slack/tests/test_scenario_steps/test_slack_channel_integration.py index 207bd7c3..7052d28f 100644 --- a/engine/apps/slack/tests/test_scenario_steps/test_slack_channel_integration.py +++ b/engine/apps/slack/tests/test_scenario_steps/test_slack_channel_integration.py @@ -3,6 +3,7 @@ from unittest.mock import Mock, patch import pytest from apps.alerts.models import ResolutionNoteSlackMessage +from apps.slack.client import SlackAPIException from apps.slack.scenarios.slack_channel_integration import SlackChannelMessageEventStep @@ -262,6 +263,53 @@ class TestSlackChannelMessageEventStep: ) MockResolutionNoteSlackMessage.objects.get_or_create.assert_not_called() + @patch("apps.alerts.models.ResolutionNoteSlackMessage") + def test_save_thread_message_for_resolution_note_api_errors( + self, + MockResolutionNoteSlackMessage, + make_organization_and_user_with_slack_identities, + make_alert_receive_channel, + make_alert_group, + make_slack_message, + ) -> None: + ( + organization, + user, + slack_team_identity, + slack_user_identity, + ) = make_organization_and_user_with_slack_identities() + integration = make_alert_receive_channel(organization) + alert_group = make_alert_group(integration) + + channel = "potato" + ts = 88945.4849 + thread_ts = 16789.123 + + make_slack_message(alert_group, slack_id=thread_ts, channel_id=channel) + + step = SlackChannelMessageEventStep(slack_team_identity, organization, user) + step._slack_client = Mock() + step._slack_client.chat_getPermalink.side_effect = [ + SlackAPIException("error!", response={"ok": False, "error": "message_not_found"}) + ] + + payload = { + "event": { + "channel": channel, + "ts": ts, + "thread_ts": thread_ts, + "text": "h" * 2901, + }, + } + + step.save_thread_message_for_resolution_note(slack_user_identity, payload) + + step._slack_client.chat_getPermalink.assert_called_once_with( + channel=payload["event"]["channel"], + message_ts=payload["event"]["ts"], + ) + MockResolutionNoteSlackMessage.objects.get_or_create.assert_not_called() + @pytest.mark.parametrize("resolution_note_slack_message_already_exists", [True, False]) def test_save_thread_message_for_resolution_note( self,