diff --git a/CHANGELOG.md b/CHANGELOG.md index 6500ab98..2dea7e4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased + +### Added + +- Create success log records for delivered personal notifications ([3557](https://github.com/grafana/oncall/pull/3557)) + ## v1.3.79 (2023-12-14) ### Added diff --git a/engine/apps/alerts/tasks/notify_user.py b/engine/apps/alerts/tasks/notify_user.py index bf09a412..b3165558 100644 --- a/engine/apps/alerts/tasks/notify_user.py +++ b/engine/apps/alerts/tasks/notify_user.py @@ -161,7 +161,7 @@ def notify_user_task( type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED, notification_policy=notification_policy, alert_group=alert_group, - reason=reason, + reason="Alert group slack notifications are disabled", slack_prevent_posting=prevent_posting_to_thread, notification_step=notification_policy.step, notification_channel=notification_policy.notify_by, diff --git a/engine/apps/base/models/user_notification_policy_log_record.py b/engine/apps/base/models/user_notification_policy_log_record.py index d8c83e31..2c20fc51 100644 --- a/engine/apps/base/models/user_notification_policy_log_record.py +++ b/engine/apps/base/models/user_notification_policy_log_record.py @@ -171,7 +171,7 @@ class UserNotificationPolicyLogRecord(models.Model): result += f"SMS to {user_verbal} was delivered successfully" elif notification_channel == UserNotificationPolicy.NotificationChannel.PHONE_CALL: result += f"phone call to {user_verbal} was successful" - elif notification_channel is None: + else: result += f"notification to {user_verbal} was delivered successfully" elif self.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED: if self.notification_error_code == UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_SMS_LIMIT_EXCEEDED: diff --git a/engine/apps/email/tasks.py b/engine/apps/email/tasks.py index 8d67577b..1b0b7e12 100644 --- a/engine/apps/email/tasks.py +++ b/engine/apps/email/tasks.py @@ -121,3 +121,13 @@ def notify_user_async(user_pk, alert_group_pk, notification_policy_pk): ) logger.error(f"Error while sending email: {e}") return + + # record success log + UserNotificationPolicyLogRecord.objects.create( + author=user, + type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS, + notification_policy=notification_policy, + alert_group=alert_group, + notification_step=notification_policy.step, + notification_channel=notification_policy.notify_by, + ) diff --git a/engine/apps/email/tests/test_notify_user.py b/engine/apps/email/tests/test_notify_user.py index b336459e..fd892e10 100644 --- a/engine/apps/email/tests/test_notify_user.py +++ b/engine/apps/email/tests/test_notify_user.py @@ -45,6 +45,9 @@ def test_notify_user( notify_user_async(user.pk, alert_group.pk, notification_policy.pk) assert len(mail.outbox) == 1 + log_record = notification_policy.personal_log_records.last() + assert log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS + @pytest.mark.django_db def test_notify_empty_email_host( diff --git a/engine/apps/mobile_app/tasks/new_alert_group.py b/engine/apps/mobile_app/tasks/new_alert_group.py index 869f4c98..27c97544 100644 --- a/engine/apps/mobile_app/tasks/new_alert_group.py +++ b/engine/apps/mobile_app/tasks/new_alert_group.py @@ -142,4 +142,16 @@ def notify_user_about_new_alert_group(user_pk, alert_group_pk, notification_poli return message = _get_fcm_message(alert_group, user, device_to_notify, critical) - send_push_notification(device_to_notify, message, _create_error_log_record) + succeeded = send_push_notification(device_to_notify, message, _create_error_log_record) + + if succeeded: + # record success log + # (note: send_push_notification should have created a failed log entry if there was an error) + UserNotificationPolicyLogRecord.objects.create( + author=user, + type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS, + notification_policy=notification_policy, + alert_group=alert_group, + notification_step=notification_policy.step, + notification_channel=notification_policy.notify_by, + ) diff --git a/engine/apps/mobile_app/tests/tasks/test_new_alert_group.py b/engine/apps/mobile_app/tests/tasks/test_new_alert_group.py index fec871e1..f33e5c3d 100644 --- a/engine/apps/mobile_app/tests/tasks/test_new_alert_group.py +++ b/engine/apps/mobile_app/tests/tasks/test_new_alert_group.py @@ -47,6 +47,8 @@ def test_notify_user_about_new_alert_group( ) mock_send_push_notification.assert_called_once() + log_record = notification_policy.personal_log_records.last() + assert log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS @patch("apps.mobile_app.tasks.new_alert_group.send_push_notification") diff --git a/engine/apps/mobile_app/tests/test_utils.py b/engine/apps/mobile_app/tests/test_utils.py index 50c13b9f..4dc7d974 100644 --- a/engine/apps/mobile_app/tests/test_utils.py +++ b/engine/apps/mobile_app/tests/test_utils.py @@ -30,7 +30,8 @@ def test_send_push_notification_cloud( settings.LICENSE = CLOUD_LICENSE_NAME settings.IS_OPEN_SOURCE = False - utils.send_push_notification(device, mock_message) + succeeded = utils.send_push_notification(device, mock_message) + assert succeeded mock_send_message.assert_called_once_with(mock_message) @@ -77,8 +78,8 @@ def test_send_push_notification_oss( device = FCMDevice.objects.create(user=user, registration_id="test_device_id") mock_message = {"foo": "bar"} - utils.send_push_notification(device, mock_message, mock_error_cb) - + succeeded = utils.send_push_notification(device, mock_message, mock_error_cb) + assert succeeded mock_error_cb.assert_not_called() mock_send_push_notification_to_fcm_relay.assert_called_once_with(mock_message) @@ -99,8 +100,9 @@ def test_send_push_notification_oss_no_cloud_connector( device = FCMDevice.objects.create(user=user, registration_id="test_device_id") mock_message = {"foo": "bar"} - utils.send_push_notification(device, mock_message, mock_error_cb) + succeeded = utils.send_push_notification(device, mock_message, mock_error_cb) + assert not succeeded mock_error_cb.assert_called_once_with() mock_send_push_notification_to_fcm_relay.assert_not_called() @@ -128,7 +130,8 @@ def test_send_push_notification_oss_fcm_relay_returns_client_error( device = FCMDevice.objects.create(user=user, registration_id="test_device_id") mock_message = {"foo": "bar"} - utils.send_push_notification(device, mock_message, mock_error_cb) + succeeded = utils.send_push_notification(device, mock_message, mock_error_cb) + assert not succeeded mock_send_push_notification_to_fcm_relay.assert_called_once_with(mock_message) diff --git a/engine/apps/mobile_app/utils.py b/engine/apps/mobile_app/utils.py index 4a3991d7..a4637aa6 100644 --- a/engine/apps/mobile_app/utils.py +++ b/engine/apps/mobile_app/utils.py @@ -58,7 +58,7 @@ def send_message_to_fcm_device(device: "FCMDevice", message: Message) -> None: def send_push_notification( device_to_notify: "FCMDevice", message: Message, error_cb: typing.Optional[typing.Callable[..., None]] = None -) -> None: +) -> bool: logger.debug(f"Sending push notification to device type {device_to_notify.type} with message: {message}") def _error_cb(): @@ -72,7 +72,7 @@ def send_push_notification( if not CloudConnector.objects.exists(): _error_cb() logger.error("Error while sending a mobile push notification: not connected to cloud") - return + return False try: response = _send_push_notification_to_fcm_relay(message) @@ -84,12 +84,15 @@ def send_push_notification( logger.error( f"Error while sending a mobile push notification: HTTP client error {e.response.status_code}" ) - return + return False else: raise else: send_message_to_fcm_device(device_to_notify, message) + # notification succeeded (otherwise raised exception before) + return True + def construct_fcm_message( message_type: MessageType, diff --git a/engine/apps/slack/models/slack_message.py b/engine/apps/slack/models/slack_message.py index a3ae3647..f5f9cfd5 100644 --- a/engine/apps/slack/models/slack_message.py +++ b/engine/apps/slack/models/slack_message.py @@ -173,6 +173,15 @@ class SlackMessage(models.Model): _slack_team_identity=self.slack_team_identity, channel_id=channel_id, ) + # create success record + UserNotificationPolicyLogRecord.objects.create( + author=user, + type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS, + notification_policy=notification_policy, + alert_group=alert_group, + notification_step=notification_policy.step, + notification_channel=notification_policy.notify_by, + ) # Check if escalated user is in channel. Otherwise send notification and request to invite him. try: diff --git a/engine/apps/slack/tests/test_slack_message.py b/engine/apps/slack/tests/test_slack_message.py new file mode 100644 index 00000000..24e36c82 --- /dev/null +++ b/engine/apps/slack/tests/test_slack_message.py @@ -0,0 +1,38 @@ +from unittest.mock import patch + +import pytest + +from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord + + +@pytest.mark.django_db +def test_send_slack_notification( + make_organization_and_user_with_slack_identities, + make_alert_receive_channel, + make_alert_group, + make_alert, + make_user_notification_policy, + make_slack_channel, + make_slack_message, +): + organization, user, slack_team_identity, slack_user_identity = make_organization_and_user_with_slack_identities() + + # set up notification policy and alert group + notification_policy = make_user_notification_policy( + user, + UserNotificationPolicy.Step.NOTIFY, + notify_by=UserNotificationPolicy.NotificationChannel.SLACK, + ) + alert_receive_channel = make_alert_receive_channel(organization) + alert_group = make_alert_group(alert_receive_channel) + make_alert(alert_group=alert_group, raw_request_data={}) + + slack_channel = make_slack_channel(slack_team_identity) + slack_message = make_slack_message(alert_group=alert_group, channel_id=slack_channel.slack_id) + + with patch("apps.slack.client.SlackClient.conversations_members") as mock_members: + mock_members.return_value = {"members": [slack_user_identity.slack_id]} + slack_message.send_slack_notification(user, alert_group, notification_policy) + + log_record = notification_policy.personal_log_records.last() + assert log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS diff --git a/engine/apps/telegram/models/connectors/personal.py b/engine/apps/telegram/models/connectors/personal.py index cad4fe64..9e988712 100644 --- a/engine/apps/telegram/models/connectors/personal.py +++ b/engine/apps/telegram/models/connectors/personal.py @@ -35,6 +35,7 @@ class TelegramToUserConnector(models.Model): alert_group=alert_group, user=user, notification_policy=notification_policy, + reason="No linked telegram account", error_code=UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_IS_NOT_LINKED_TO_SLACK_ACC, ) @@ -51,19 +52,36 @@ class TelegramToUserConnector(models.Model): self.send_full_alert_group(alert_group=alert_group, notification_policy=notification_policy) @staticmethod - def create_telegram_notification_error( - alert_group: AlertGroup, user: User, notification_policy: UserNotificationPolicy, error_code: int + def create_telegram_notification_success( + alert_group: AlertGroup, user: User, notification_policy: UserNotificationPolicy ) -> None: - log_record = UserNotificationPolicyLogRecord( + UserNotificationPolicyLogRecord.objects.create( + author=user, + type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS, + notification_policy=notification_policy, + alert_group=alert_group, + notification_step=notification_policy.step if notification_policy else None, + notification_channel=notification_policy.notify_by if notification_policy else None, + ) + + @staticmethod + def create_telegram_notification_error( + alert_group: AlertGroup, + user: User, + notification_policy: UserNotificationPolicy, + error_code: int, + reason: str | None, + ) -> None: + UserNotificationPolicyLogRecord.objects.create( author=user, type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED, notification_policy=notification_policy, alert_group=alert_group, notification_error_code=error_code, + reason=reason, notification_step=notification_policy.step if notification_policy else None, notification_channel=notification_policy.notify_by if notification_policy else None, ) - log_record.save() # send the actual alert group and log to user's DM def send_full_alert_group(self, alert_group: AlertGroup, notification_policy: UserNotificationPolicy) -> None: @@ -75,6 +93,7 @@ class TelegramToUserConnector(models.Model): self.user, notification_policy, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_TOKEN_ERROR, + reason="Invalid token", ) return @@ -106,6 +125,7 @@ class TelegramToUserConnector(models.Model): self.user, notification_policy, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_BOT_IS_DELETED, + reason="Bot was blocked by the user", ) elif e.message == "Invalid token": TelegramToUserConnector.create_telegram_notification_error( @@ -113,6 +133,7 @@ class TelegramToUserConnector(models.Model): self.user, notification_policy, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_TOKEN_ERROR, + reason="Invalid token", ) elif e.message == "Forbidden: user is deactivated": TelegramToUserConnector.create_telegram_notification_error( @@ -120,9 +141,14 @@ class TelegramToUserConnector(models.Model): self.user, notification_policy, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_USER_IS_DEACTIVATED, + reason="Telegram user was disabled", ) else: raise e + else: + TelegramToUserConnector.create_telegram_notification_success( + alert_group, self.user, notification_policy + ) else: self._nudge_about_alert_group_message(telegram_client, old_alert_group_message) @@ -136,6 +162,7 @@ class TelegramToUserConnector(models.Model): self.user, notification_policy, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_TOKEN_ERROR, + reason="Invalid token", ) return @@ -159,6 +186,7 @@ class TelegramToUserConnector(models.Model): self.user, notification_policy, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_BOT_IS_DELETED, + reason="Bot was blocked by the user", ) elif e.message == "Invalid token": TelegramToUserConnector.create_telegram_notification_error( @@ -166,6 +194,7 @@ class TelegramToUserConnector(models.Model): self.user, notification_policy, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_TOKEN_ERROR, + reason="Invalid token", ) elif e.message == "Forbidden: user is deactivated": TelegramToUserConnector.create_telegram_notification_error( @@ -173,9 +202,12 @@ class TelegramToUserConnector(models.Model): self.user, notification_policy, UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_USER_IS_DEACTIVATED, + reason="Telegram user was disabled", ) else: raise e + else: + TelegramToUserConnector.create_telegram_notification_success(alert_group, self.user, notification_policy) @staticmethod @ignore_reply_to_message_deleted diff --git a/engine/apps/telegram/tests/factories.py b/engine/apps/telegram/tests/factories.py index c9edb2c2..f00a89a0 100644 --- a/engine/apps/telegram/tests/factories.py +++ b/engine/apps/telegram/tests/factories.py @@ -18,7 +18,7 @@ class TelegramToUserConnectorFactory(factory.DjangoModelFactory): class TelegramChannelFactory(factory.DjangoModelFactory): - channel_chat_id = factory.LazyAttribute(lambda v: str(UniqueFaker("pyint").generate())) + channel_chat_id = factory.LazyAttribute(lambda v: "-" + str(UniqueFaker("pyint").generate())) channel_name = factory.Faker("word") discussion_group_chat_id = factory.LazyAttribute(lambda v: str(UniqueFaker("pyint").generate())) discussion_group_name = factory.Faker("word") diff --git a/engine/apps/telegram/tests/test_personal_connector.py b/engine/apps/telegram/tests/test_personal_connector.py index add403cb..12c7e9a3 100644 --- a/engine/apps/telegram/tests/test_personal_connector.py +++ b/engine/apps/telegram/tests/test_personal_connector.py @@ -50,16 +50,22 @@ def test_personal_connector_replied_message_not_found( @pytest.mark.parametrize( - "side_effect,notification_error_code", + "side_effect,notification_error_code,reason", [ ( error.Unauthorized("Forbidden: bot was blocked by the user"), UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_BOT_IS_DELETED, + "Bot was blocked by the user", + ), + ( + error.Unauthorized("Invalid token"), + UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_TOKEN_ERROR, + "Invalid token", ), - (error.Unauthorized("Invalid token"), UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_TOKEN_ERROR), ( error.Unauthorized("Forbidden: user is deactivated"), UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_USER_IS_DEACTIVATED, + "Telegram user was disabled", ), ], ) @@ -67,6 +73,7 @@ def test_personal_connector_replied_message_not_found( def test_personal_connector_send_link_to_channel_message_handle_exceptions( side_effect, notification_error_code, + reason, make_organization_and_user, make_telegram_user_connector, make_user_notification_policy, @@ -96,3 +103,86 @@ def test_personal_connector_send_link_to_channel_message_handle_exceptions( log_records = user.personal_log_records.filter(alert_group=alert_group) assert log_records.count() == 1 assert log_records.first().notification_error_code == notification_error_code + assert log_records.first().reason == reason + + +@patch.object(TelegramClient, "send_message") +@pytest.mark.django_db +def test_personal_connector_send_link_to_channel_message( + mock_send_message, + make_organization_and_user, + make_telegram_user_connector, + make_user_notification_policy, + make_alert_receive_channel, + make_alert_group, + make_alert, + make_telegram_channel, + make_telegram_message, +): + # set up a user with Telegram account connected + organization, user = make_organization_and_user() + make_telegram_user_connector(user) + notification_policy = make_user_notification_policy( + user, + UserNotificationPolicy.Step.NOTIFY, + notify_by=UserNotificationPolicy.NotificationChannel.TELEGRAM, + important=False, + ) + + # create an alert group with an existing Telegram message in channel + alert_receive_channel = make_alert_receive_channel(organization) + alert_group = make_alert_group(alert_receive_channel) + make_alert(alert_group=alert_group, raw_request_data=alert_receive_channel.config.example_payload) + telegram_channel = make_telegram_channel(organization, is_default_channel=True) + make_telegram_message( + alert_group=alert_group, + message_type=TelegramMessage.ALERT_GROUP_MESSAGE, + chat_id=str(telegram_channel.channel_chat_id), + ) + + user.telegram_connection.send_link_to_channel_message(alert_group, notification_policy) + mock_send_message.assert_called_once_with( + chat_id=user.telegram_connection.telegram_chat_id, + message_type=TelegramMessage.LINK_TO_CHANNEL_MESSAGE, + alert_group=alert_group, + ) + + log_record = notification_policy.personal_log_records.last() + assert log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS + + +@patch.object(TelegramClient, "send_message") +@pytest.mark.django_db +def test_personal_connector_send_full_alert_group( + mock_send_message, + make_organization_and_user, + make_telegram_user_connector, + make_user_notification_policy, + make_alert_receive_channel, + make_alert_group, + make_alert, +): + # set up a user with Telegram account connected + organization, user = make_organization_and_user() + make_telegram_user_connector(user) + notification_policy = make_user_notification_policy( + user, + UserNotificationPolicy.Step.NOTIFY, + notify_by=UserNotificationPolicy.NotificationChannel.TELEGRAM, + important=False, + ) + + # create an alert group with an existing Telegram message in channel + alert_receive_channel = make_alert_receive_channel(organization) + alert_group = make_alert_group(alert_receive_channel) + make_alert(alert_group=alert_group, raw_request_data=alert_receive_channel.config.example_payload) + + user.telegram_connection.send_full_alert_group(alert_group, notification_policy) + mock_send_message.assert_called_once_with( + chat_id=user.telegram_connection.telegram_chat_id, + message_type=TelegramMessage.PERSONAL_MESSAGE, + alert_group=alert_group, + ) + + log_record = notification_policy.personal_log_records.last() + assert log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS diff --git a/engine/conftest.py b/engine/conftest.py index 821d8830..fa5ae281 100644 --- a/engine/conftest.py +++ b/engine/conftest.py @@ -147,6 +147,7 @@ IS_RBAC_ENABLED = os.getenv("ONCALL_TESTING_RBAC_ENABLED", "True") == "True" def mock_slack_api_call(monkeypatch): def mock_api_call(*args, **kwargs): return { + "ts": timezone.now().isoformat(), "status": 200, "usergroups": [], "channel": {"id": "TEST_CHANNEL_ID"},