diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a62c032..da53395f 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 - Support e2e tests in Tilt and Makefile ([#3516](https://github.com/grafana/oncall/pull/3516)) +### Fixed + +- Check reason to skip notification in Slack to avoid task perform_notification retries @Ferril ([#3562](https://github.com/grafana/oncall/pull/3562)) + ## v1.3.80 (2023-12-14) ### Added diff --git a/engine/apps/alerts/models/alert_group.py b/engine/apps/alerts/models/alert_group.py index 54e86764..f13c6c45 100644 --- a/engine/apps/alerts/models/alert_group.py +++ b/engine/apps/alerts/models/alert_group.py @@ -489,6 +489,7 @@ class AlertGroup(AlertGroupSlackRenderingMixin, EscalationSnapshotMixin, models. AlertGroup.ACCOUNT_INACTIVE, AlertGroup.RATE_LIMITED, AlertGroup.CHANNEL_NOT_SPECIFIED, + AlertGroup.RESTRICTED_ACTION, ) def is_alert_a_resolve_signal(self, alert): diff --git a/engine/apps/alerts/tasks/notify_user.py b/engine/apps/alerts/tasks/notify_user.py index b3165558..4a125b64 100644 --- a/engine/apps/alerts/tasks/notify_user.py +++ b/engine/apps/alerts/tasks/notify_user.py @@ -287,19 +287,29 @@ def perform_notification(log_record_pk): # Code below is not consistent. # We check various slack reasons to skip escalation in this task, in send_slack_notification, # before and after posting of slack message. - if alert_group.reason_to_skip_escalation == alert_group.RATE_LIMITED: + if alert_group.skip_escalation_in_slack: + notification_error_code = UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_IN_SLACK + if alert_group.reason_to_skip_escalation == alert_group.RATE_LIMITED: + notification_error_code = UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_IN_SLACK_RATELIMIT + elif alert_group.reason_to_skip_escalation == alert_group.CHANNEL_ARCHIVED: + notification_error_code = ( + UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_IN_SLACK_CHANNEL_IS_ARCHIVED + ) + elif alert_group.reason_to_skip_escalation == alert_group.ACCOUNT_INACTIVE: + notification_error_code = UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_IN_SLACK_TOKEN_ERROR task_logger.debug( - f"send_slack_notification for alert_group {alert_group.pk} failed because of slack ratelimit." + f"send_slack_notification for alert_group {alert_group.pk} failed because escalation in slack is " + f"skipped, reason: '{alert_group.get_reason_to_skip_escalation_display()}'" ) UserNotificationPolicyLogRecord( author=user, type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED, notification_policy=notification_policy, - reason="Slack ratelimit", + reason=f"Skipped escalation in Slack, reason: '{alert_group.get_reason_to_skip_escalation_display()}'", alert_group=alert_group, notification_step=notification_policy.step, notification_channel=notification_channel, - notification_error_code=UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_IN_SLACK_RATELIMIT, + notification_error_code=notification_error_code, ).save() return diff --git a/engine/apps/alerts/tests/test_notify_user.py b/engine/apps/alerts/tests/test_notify_user.py index e6cffe1c..98d8bc2b 100644 --- a/engine/apps/alerts/tests/test_notify_user.py +++ b/engine/apps/alerts/tests/test_notify_user.py @@ -2,10 +2,12 @@ from unittest.mock import patch import pytest +from apps.alerts.models import AlertGroup from apps.alerts.tasks.notify_user import notify_user_task, perform_notification from apps.api.permissions import LegacyAccessControlRole from apps.base.models.user_notification_policy import UserNotificationPolicy from apps.base.models.user_notification_policy_log_record import UserNotificationPolicyLogRecord +from apps.slack.models import SlackMessage NOTIFICATION_UNAUTHORIZED_MSG = "notification is not allowed for user" @@ -178,3 +180,63 @@ def test_notify_user_error_if_viewer( assert error_log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED assert error_log_record.reason == NOTIFICATION_UNAUTHORIZED_MSG assert error_log_record.notification_error_code == UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_FORBIDDEN + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "reason_to_skip_escalation,error_code", + [ + (AlertGroup.RATE_LIMITED, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_IN_SLACK_RATELIMIT), + (AlertGroup.CHANNEL_ARCHIVED, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_IN_SLACK_CHANNEL_IS_ARCHIVED), + (AlertGroup.ACCOUNT_INACTIVE, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_IN_SLACK_TOKEN_ERROR), + (AlertGroup.RESTRICTED_ACTION, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_IN_SLACK), + (AlertGroup.NO_REASON, None), + ], +) +def test_perform_notification_reason_to_skip_escalation_in_slack( + reason_to_skip_escalation, + error_code, + make_organization, + make_slack_team_identity, + make_user, + make_user_notification_policy, + make_alert_receive_channel, + make_alert_group, + make_user_notification_policy_log_record, + make_slack_message, +): + organization = make_organization() + slack_team_identity = make_slack_team_identity() + organization.slack_team_identity = slack_team_identity + organization.save() + user = make_user(organization=organization) + user_notification_policy = make_user_notification_policy( + user=user, + step=UserNotificationPolicy.Step.NOTIFY, + notify_by=UserNotificationPolicy.NotificationChannel.SLACK, + ) + alert_receive_channel = make_alert_receive_channel(organization=organization) + alert_group = make_alert_group(alert_receive_channel=alert_receive_channel) + alert_group.reason_to_skip_escalation = reason_to_skip_escalation + alert_group.save() + log_record = make_user_notification_policy_log_record( + author=user, + alert_group=alert_group, + notification_policy=user_notification_policy, + type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_TRIGGERED, + ) + if not error_code: + make_slack_message(alert_group=alert_group, channel_id="test_channel_id", slack_id="test_slack_id") + with patch.object(SlackMessage, "send_slack_notification") as mocked_send_slack_notification: + perform_notification(log_record.pk) + last_log_record = UserNotificationPolicyLogRecord.objects.last() + + if error_code: + log_reason = f"Skipped escalation in Slack, reason: '{alert_group.get_reason_to_skip_escalation_display()}'" + mocked_send_slack_notification.assert_not_called() + assert last_log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED + assert last_log_record.reason == log_reason + assert last_log_record.notification_error_code == error_code + else: + mocked_send_slack_notification.assert_called() + assert last_log_record.type != UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED