Merge pull request #10 from GabrielSalla/improve-slack-notification-handling

Improve slack notification handling

Author: GabrielSalla

src/plugins/__init__.py

@@ -1,4 +1,5 @@
 from types import ModuleType
+
 from .plugins_loader import load_plugins as __load_plugins
 
 loaded_plugins: dict[str, ModuleType]

src/plugins/plugins_loader.py

@@ -2,8 +2,8 @@
 import logging
 import os
 import traceback
-from types import ModuleType
 from pathlib import Path
+from types import ModuleType
 
 _logger = logging.getLogger("plugins_loader")
 

src/plugins/slack/actions/actions.py

@@ -1,10 +1,42 @@
-from typing import Any
+import logging
+from typing import Any, cast
 
 import registry
-from models import Alert, Notification, NotificationStatus
+from models import Monitor, Notification, NotificationStatus
 from utils.async_tools import do_concurrently
 
-from ..notifications.slack_notification import clear_slack_notification
+from ..notifications import slack_notification
+
+_logger = logging.getLogger("plugin.slack.actions")
+
+
+async def _resend_notification(notification: Notification):
+    """Clear a single notification and send it again"""
+    await registry.wait_monitor_loaded(notification.monitor_id)
+
+    monitor = await Monitor.get_by_id(notification.monitor_id)
+    if monitor is None:
+        return  # pragma: no cover
+
+    # Get the SlackNotification option from the monitor code
+    notification_option = None
+    for notification_option in monitor.code.notification_options:
+        if isinstance(notification_option, slack_notification.SlackNotification):
+            break
+    if notification_option is None:
+        _logger.warning(f"No 'SlackNotification' option for {monitor}")
+        return
+
+    # Clear the notification and send it again
+    await slack_notification.clear_slack_notification(notification)
+    await slack_notification.slack_notification(
+        event_payload={
+            "event_data": {
+                "id": notification.alert_id,
+            }
+        },
+        notification_options=cast(slack_notification.SlackNotification, notification_option),
+    )
 
 
 async def resend_notifications(message_payload: dict[Any, Any]):
@@ -17,18 +49,7 @@ async def resend_notifications(message_payload: dict[Any, Any]):
         Notification.data["channel"].astext == message_payload["slack_channel"],
     )
 
-    if len(notifications) == 0:
-        return
-
-    monitors_ids = {notification.monitor_id for notification in notifications}
-    for monitor_id in monitors_ids:
-        await registry.wait_monitor_loaded(monitor_id)
-
     await do_concurrently(*[
-        clear_slack_notification(notification)
+        _resend_notification(notification)
         for notification in notifications
     ])
-
-    alert_ids = list({notification.alert_id for notification in notifications})
-    alerts = await Alert.get_all(Alert.id.in_(alert_ids))
-    await do_concurrently(*[alert.update() for alert in alerts])

src/plugins/slack/notifications/slack_notification.py

@@ -14,7 +14,7 @@
 
 from .. import slack
 
-_logger = logging.getLogger("reaction_handler")
+_logger = logging.getLogger("plugin.slack.notifications")
 
 RESEND_ERRORS = [
     "message_not_found",
@@ -388,29 +388,41 @@ async def slack_notification(
     event_payload: dict[str, Any],
     notification_options: SlackNotification,
 ):
+    """Handle the Slack notification for an alert"""
     alert_data = event_payload["event_data"]
 
-    # Lower number for priority is more important, so this operation is reversed
-    if alert_data["priority"] > notification_options.min_priority_to_send:
-        return
-
     alert = await Alert.get_by_id(alert_data["id"])
     if alert is None:
         return
 
-    monitor = await Monitor.get_by_id(alert.monitor_id)
-    # This check is just to make the typing check happy, as there's a foreign key constraint in the
-    # database
-    if monitor is None:
-        return  # pragma: no cover
-
-    notification = await Notification.get_or_create(
-        monitor_id=alert.monitor_id, alert_id=alert.id, target="slack"
+    notification = await Notification.get(
+        Notification.monitor_id == alert.monitor_id,
+        Notification.alert_id == alert.id,
+        Notification.target == "slack",
     )
 
+    # Only continue if the notification already exists or if the alert priority triggers a new
+    # notification
+    if notification is None:
+        # Lower number for priority is more important, so this operation is reversed
+        if alert.priority > notification_options.min_priority_to_send:
+            return
+        if alert.status == AlertStatus.solved:
+            return
+
+        notification = await Notification.create(
+            monitor_id=alert.monitor_id, alert_id=alert.id, target="slack"
+        )
+
     if alert.status == AlertStatus.solved:
         await notification.close()
 
+    monitor = await Monitor.get_by_id(alert.monitor_id)
+    # This check is just to make the typing check happy, as the monitor must exist because of the
+    # alert's 'monitor_id' foreign key
+    if monitor is None:
+        return  # pragma: no cover
+
     attachments = await _build_attachments(monitor, alert, notification_options)
 
     if notification.data is not None and notification.data.get("ts") is not None:

src/plugins/slack/services/websocket.py

@@ -11,7 +11,7 @@
 from .. import slack
 from .pattern_match import get_message_request
 
-_logger = logging.getLogger("slack_websocket")
+_logger = logging.getLogger("plugin.slack.websocket")
 
 _handler: AsyncSocketModeHandler | None
 

tests/plugins/slack/actions/test_actions.py

@@ -2,18 +2,87 @@
 
 import pytest
 
-from plugins.slack.actions import resend_notifications
 import registry as registry
-from models import Alert, Monitor, Notification
+import plugins.slack.actions.actions as actions
+import plugins.slack.notifications.slack_notification as slack_notification
+from models import Alert, Monitor, Notification, NotificationStatus
+from tests.test_utils import assert_message_in_log
 
 pytestmark = pytest.mark.asyncio(loop_scope="session")
 
 
-async def test_resend_notifications(mocker, sample_monitor: Monitor):
+async def test_resend_notification_no_slack_notification_option(
+    caplog, monkeypatch, sample_monitor: Monitor
+):
+    """'_resend_notification' should just return when there is no SlackNotification option"""
+    clear_notification_mock = AsyncMock()
+    monkeypatch.setattr(slack_notification, "clear_slack_notification", clear_notification_mock)
+    slack_notification_mock = AsyncMock()
+    monkeypatch.setattr(slack_notification, "slack_notification", slack_notification_mock)
+    monkeypatch.setattr(sample_monitor.code, "notification_options", [], raising=False)
+
+    alert = await Alert.create(
+        monitor_id=sample_monitor.id,
+        priority=2,
+    )
+    notification = await Notification.create(
+        monitor_id=alert.monitor_id,
+        alert_id=alert.id,
+        target="slack",
+        data={"channel": "channel", "ts": "123"},
+    )
+
+    await actions._resend_notification(notification)
+
+    clear_notification_mock.assert_not_called()
+    slack_notification_mock.assert_not_called()
+    assert_message_in_log(caplog, f"No 'SlackNotification' option for {sample_monitor}")
+
+
+async def test_resend_notification(
+    mocker, monkeypatch, sample_monitor: Monitor
+):
+    """'_resend_notification' should clear the notification and send it again"""
+    wait_monitor_loaded_spy: AsyncMock = mocker.spy(registry, "wait_monitor_loaded")
+    clear_notification_mock = AsyncMock()
+    monkeypatch.setattr(slack_notification, "clear_slack_notification", clear_notification_mock)
+    slack_notification_mock = AsyncMock()
+    monkeypatch.setattr(slack_notification, "slack_notification", slack_notification_mock)
+    notification_options = slack_notification.SlackNotification(
+        channel="channel",
+        title="title",
+        issues_fields=["col"],
+        min_priority_to_send=3,
+        mention="mention",
+        min_priority_to_mention=2,
+    )
+    monkeypatch.setattr(
+        sample_monitor.code, "notification_options", [notification_options], raising=False
+    )
+
+    alert = await Alert.create(
+        monitor_id=sample_monitor.id,
+        priority=2,
+    )
+    notification = await Notification.create(
+        monitor_id=alert.monitor_id,
+        alert_id=alert.id,
+        target="slack",
+        data={"channel": "channel", "ts": "123"},
+    )
+
+    await actions._resend_notification(notification)
+
+    wait_monitor_loaded_spy.assert_awaited_once_with(sample_monitor.id)
+    clear_notification_mock.assert_awaited_once()
+    slack_notification_mock.assert_awaited_once()
+
+
+async def test_resend_notifications(monkeypatch, sample_monitor: Monitor):
     """'resend_notifications' should clear all notifications for the provided channel and
     update all alerts"""
-    wait_monitor_loaded_spy: AsyncMock = mocker.spy(registry, "wait_monitor_loaded")
-    alert_update_spy: AsyncMock = mocker.spy(Alert, "update")
+    resend_notification_mock = AsyncMock()
+    monkeypatch.setattr(actions, "_resend_notification", resend_notification_mock)
 
     alert_test_channel = await Alert.create(
         monitor_id=sample_monitor.id,
@@ -30,61 +99,75 @@ async def test_resend_notifications(mocker, sample_monitor: Monitor):
         monitor_id=sample_monitor.id,
         priority=2,
     )
-    notification_other_channel = await Notification.create(
+    await Notification.create(
         monitor_id=alert_other_channel.monitor_id,
         alert_id=alert_other_channel.id,
         target="slack",
         data={"channel": "test_resend_notification_other", "ts": "123"},
     )
 
-    await resend_notifications(
+    await actions.resend_notifications(
         {"slack_channel": "test_resend_notification"}
     )
 
-    await notification_test_channel.refresh()
-    assert notification_test_channel.data == {"channel": None, "ts": None, "mention_ts": None}
-
-    await notification_other_channel.refresh()
-    assert notification_other_channel.data == {
-        "channel": "test_resend_notification_other",
-        "ts": "123",
-    }
-
-    wait_monitor_loaded_spy.assert_awaited_once_with(sample_monitor.id)
-    alert_update_spy.assert_awaited_once()
-    call_args = alert_update_spy.call_args
-    assert call_args[0][0].id == alert_test_channel.id
+    assert resend_notification_mock.await_args is not None
+    assert resend_notification_mock.await_args[0][0].id == notification_test_channel.id
 
 
 async def test_resend_notifications_no_notifications_in_channel(
-        mocker,
-        sample_monitor: Monitor
+    monkeypatch, sample_monitor: Monitor
 ):
     """'resend_notifications' should just return when there are no notifications for the
     provided channel"""
-    wait_monitor_loaded_spy: AsyncMock = mocker.spy(registry, "wait_monitor_loaded")
-    alert_update_spy: AsyncMock = mocker.spy(Alert, "update")
+    resend_notification_mock = AsyncMock()
+    monkeypatch.setattr(actions, "_resend_notification", resend_notification_mock)
 
     alert_other_channel = await Alert.create(
         monitor_id=sample_monitor.id,
         priority=2,
     )
-    notification_other_channel = await Notification.create(
+    await Notification.create(
         monitor_id=alert_other_channel.monitor_id,
         alert_id=alert_other_channel.id,
         target="slack",
         data={"channel": "test_resend_notification_other", "ts": "123"},
     )
 
-    await resend_notifications(
-        {"slack_channel": "test_resend_notification"}
+    await actions.resend_notifications(
+        {"slack_channel": "test_resend_notifications_no_notifications_in_channel"}
+    )
+
+    resend_notification_mock.assert_not_called()
+
+
+async def test_resend_notifications_ignore_other_notifications(
+    monkeypatch, sample_monitor: Monitor
+):
+    """'resend_notifications' should just not resend notifications that are not Slack notifications
+    or are not active"""
+    resend_notification_mock = AsyncMock()
+    monkeypatch.setattr(actions, "_resend_notification", resend_notification_mock)
+
+    alert_other_channel = await Alert.create(
+        monitor_id=sample_monitor.id,
+        priority=2,
+    )
+    await Notification.create(
+        monitor_id=alert_other_channel.monitor_id,
+        alert_id=alert_other_channel.id,
+        target="other_target",
+        data={"channel": "test_resend_notifications_no_notifications_in_channel", "ts": "123"},
+    )
+    await Notification.create(
+        monitor_id=alert_other_channel.monitor_id,
+        alert_id=alert_other_channel.id,
+        target="slack",
+        data={"channel": "test_resend_notifications_no_notifications_in_channel", "ts": "123"},
+        status=NotificationStatus.closed,
     )
 
-    await notification_other_channel.refresh()
-    assert notification_other_channel.data == {
-        "channel": "test_resend_notification_other",
-        "ts": "123",
-    }
+    await actions.resend_notifications(
+        {"slack_channel": "test_resend_notifications_no_notifications_in_channel"}
+    )
 
-    wait_monitor_loaded_spy.assert_not_called()
-    alert_update_spy.assert_not_called()
+    resend_notification_mock.assert_not_called()