Improve bulk action error handling

src/geant_argus/geant_argus/dashboard_alarms.py

@@ -1,11 +1,14 @@
 from django.conf import settings
+from http import HTTPStatus
 import requests
 
 UPDATE_ALARM_URL = "/alarms/{}"
 ACK_ALARM_URL = "/alarms/{}/ack"
 CLOSE_ALARM_URL = "/alarms/{}/close"
 CLEAR_ALARM_URL = "/alarms/{}/clear"
 
+ERROR_MESSAGES = {404: "Alarm not found", 400: "Bad request, alarm may be pending"}
+
 
 def api_url():
     result = getattr(settings, "DASHBOARD_ALARMS_API_URL", None)
@@ -27,13 +30,16 @@ def clear_alarm(alarm_id, payload):
     return _succeed_request("POST", api_url() + CLEAR_ALARM_URL.format(alarm_id), json=payload)
 
 
-def _succeed_request(*args, timeout=5, **kwargs) -> bool:
+def _succeed_request(*args, timeout=5, **kwargs) -> str | None:
     disable_synchronization = getattr(settings, "DASHBOARD_ALARMS_DISABLE_SYNCHRONIZATION", False)
     if disable_synchronization:
-        return True
-
+        return None
     try:
         response = requests.request(*args, timeout=timeout, **kwargs)
     except requests.Timeout:
-        return False
-    return response.status_code == 200
+        return "Request timed out while updating alarm"
+    if response.status_code != 200:
+        error_description = ERROR_MESSAGES.get(
+            response.status_code, HTTPStatus(response.status_code).description
+        )
+        return f"{error_description} (HTTP {response.status_code})"

src/geant_argus/geant_argus/incidents/bulk_actions.py

@@ -1,18 +1,20 @@
 import functools
 import itertools
+import logging
 from typing import Any, Dict
 
 from argus.htmx.utils import bulk_close_queryset
 from django import forms
 from django.conf import settings
-from django.http import HttpResponseServerError
 from django.utils import timezone
 from django.contrib import messages
 from geant_argus.auth import has_write_permission
 from geant_argus.geant_argus.dashboard_alarms import clear_alarm, close_alarm, update_alarm
 
 from .common import TicketRefField
 
+logger = logging.getLogger(__name__)
+
 
 class TicketRefForm(forms.Form):
     ticket_ref = TicketRefField()
@@ -24,55 +26,82 @@ class ClearAlarmForm(forms.Form):
 
 def bulk_action_require_write(func):
     @functools.wraps(func)
-    def wrapper(actor, *args, **kwargs):
-        if not has_write_permission(actor):
-            messages.error("Insufficient permissions")
+    def wrapper(request, *args, **kwargs):
+        if not has_write_permission(request.user):
+            messages.error(request, "Insufficient permissions")
             return []
-        return func(actor, *args, **kwargs)
+        return func(request, *args, **kwargs)
 
     return wrapper
 
 
 @bulk_action_require_write
-def bulk_close_incidents(actor, qs, data: Dict[str, Any]):
-    incidents = bulk_close_queryset(actor, qs, data)
+def bulk_close_incidents(request, qs, data: Dict[str, Any]):
+    incidents = bulk_close_queryset(request, qs, data)
+    processed_incidents = []
     for incident in incidents:
-        if not close_alarm(incident.source_incident_id):
-            raise HttpResponseServerError("Error while closing incident")
+        error = close_alarm(incident.source_incident_id)
+        if error is not None:
+            message = (
+                f"API error while closing incident with ID "
+                f"{incident.source_incident_id}: {error}"
+            )
+            logger.error(message)
+            messages.error(request, message)
+            break
         incident.metadata["status"] = "CLOSED"
         incident.metadata["clear_time"] = data["timestamp"].isoformat()
         incident.save()
+        processed_incidents.append(incident)
 
-    return incidents
+    return processed_incidents
 
 
 @bulk_action_require_write
-def bulk_clear_incidents(actor, qs, data: Dict[str, Any]):
+def bulk_clear_incidents(request, qs, data: Dict[str, Any]):
     clear_time = (data["timestamp"] or timezone.now()).replace(tzinfo=None).isoformat()
     incidents = list(qs)
+    processed_incidents = []
     for incident in incidents:
-        if not clear_alarm(incident.source_incident_id, {"clear_time": clear_time}):
-            return HttpResponseServerError("Error while clearing incident")
+        error = clear_alarm(incident.source_incident_id, {"clear_time": clear_time})
+        if error is not None:
+            message = (
+                f"API error while clearing incident with ID "
+                f"{incident.source_incident_id}: {error}"
+            )
+            logger.error(message)
+            messages.error(request, message)
+            break
         clear_incident_in_metadata(incident.metadata, clear_time=clear_time)
         incident.save()
+        processed_incidents.append(incident)
 
-    return incidents
+    return processed_incidents
 
 
 @bulk_action_require_write
-def bulk_update_ticket_ref(actor, qs, data: Dict[str, Any]):
+def bulk_update_ticket_ref(request, qs, data: Dict[str, Any]):
     ticket_url_base = getattr(settings, "TICKET_URL_BASE", "")
     ticket_ref = data["ticket_ref"]
     payload = {"ticket_ref": ticket_ref}
     incidents = list(qs)
+    processed_incidents = []
     for incident in incidents:
-        if not update_alarm(incident.source_incident_id, payload):
-            return HttpResponseServerError("Error while updating ticket_ref")
-
+        error = update_alarm(incident.source_incident_id, payload)
+        if error is not None:
+            message = (
+                f"API error while updating ticket_ref for incident with ID "
+                f"{incident.source_incident_id}: {error}"
+            )
+
+            logger.error(message)
+            messages.error(request, message)
+            break
         incident.metadata.update(payload)
         incident.ticket_url = ticket_url_base + ticket_ref if ticket_ref else ""
         incident.save()
-    return incidents
+        processed_incidents.append(incident)
+    return processed_incidents
 
 
 def clear_incident_in_metadata(metadata: dict, clear_time: str):

test/test_auth.py

@@ -2,6 +2,7 @@
 from unittest.mock import call, patch
 
 import pytest
+from django.contrib.messages import get_messages
 from django.contrib.messages.middleware import MessageMiddleware
 from django.contrib.sessions.middleware import SessionMiddleware
 from django.http import HttpResponse
@@ -62,6 +63,7 @@ def test_protected_view_with_unauthorized_user(self, http_request, protected_vie
         http_request.user.save()
         protected_view(http_request)
         assert not protected_view.called
+        assert len(list(get_messages(http_request))) == 1
 
 
 @pytest.fixture(autouse=True)

test/test_bulk_actions.py

@@ -3,7 +3,15 @@
 
 from geant_argus.geant_argus.incidents.bulk_actions import clear_incident_in_metadata
 from unittest.mock import patch, MagicMock
-from geant_argus.geant_argus.incidents.bulk_actions import bulk_clear_incidents
+from geant_argus.geant_argus.incidents.bulk_actions import (
+    bulk_clear_incidents,
+    bulk_close_incidents,
+    bulk_update_ticket_ref,
+)
+from django.test.client import RequestFactory
+from django.contrib.messages import get_messages
+from django.contrib.messages.middleware import MessageMiddleware
+from django.contrib.sessions.middleware import SessionMiddleware
 
 
 @pytest.fixture
@@ -244,17 +252,93 @@ def test_clear_incident_in_metadata(metadata):
 @pytest.mark.django_db
 @patch("geant_argus.geant_argus.incidents.bulk_actions.clear_alarm")
 def test_bulk_clear_incidents(mock_clear_alarm, default_user):
-    actor = default_user
+    request = RequestFactory().get("/foo")
+    request.user = default_user
     qs = [MagicMock(metadata={"status": "ACTIVE", "endpoints": {}})]
     data = {"timestamp": timezone.now()}
     clear_time = data["timestamp"].replace(tzinfo=None).isoformat()
-    mock_clear_alarm.return_value = True
+    mock_clear_alarm.return_value = None
 
-    incidents = bulk_clear_incidents(actor, qs, data)
+    incidents = bulk_clear_incidents(request, qs, data)
 
     assert len(incidents) == 1
     assert incidents[0].metadata["status"] == "CLEAR"
     assert incidents[0].metadata["clear_time"] == clear_time
     assert mock_clear_alarm.call_args == ((qs[0].source_incident_id, {"clear_time": clear_time}),)
     assert mock_clear_alarm.call_count == 1
     assert qs[0].save.call_count == 1
+
+
+def bulk_close_incidents_mock_qs():
+    mock_incidents = MagicMock(
+        metadata={"status": "ACTIVE", "endpoints": {}}, source_incident_id=12345
+    )
+    mock_events = MagicMock()
+    mock_events.incident = mock_incidents
+    mock_qs = MagicMock()
+    mock_qs.close.return_value = [mock_events]
+    return mock_qs
+
+
+@pytest.mark.parametrize(
+    "status_code, expected_message_template",
+    [
+        (
+            # Custom message
+            400,
+            "API error while {message} with ID 12345: "
+            "Bad request, alarm may be pending (HTTP 400)",
+        ),
+        (
+            # No custom message
+            418,
+            "API error while {message} with ID 12345: "
+            "Server refuses to brew coffee because it is a teapot. (HTTP 418)",
+        ),
+    ],
+)
+@pytest.mark.parametrize(
+    "mock_qs, bulk_func, custom_message",
+    [
+        (bulk_close_incidents_mock_qs(), bulk_close_incidents, "closing incident"),
+        (
+            [MagicMock(metadata={"status": "ACTIVE", "endpoints": {}}, source_incident_id=12345)],
+            bulk_clear_incidents,
+            "clearing incident",
+        ),
+        (
+            [MagicMock(metadata={"status": "ACTIVE", "endpoints": {}}, source_incident_id=12345)],
+            bulk_update_ticket_ref,
+            "updating ticket_ref for incident",
+        ),
+    ],
+)
+@pytest.mark.django_db
+@patch("requests.request")
+def test_bulk_action_messages(
+    mock_request,
+    default_user,
+    settings,
+    mock_qs,
+    bulk_func,
+    custom_message,
+    status_code,
+    expected_message_template,
+):
+    settings.DASHBOARD_ALARMS_DISABLE_SYNCHRONIZATION = 0
+    settings.DASHBOARD_ALARMS_API_URL = "doesnt matter"
+    request = RequestFactory().get("/foo")
+    SessionMiddleware(lambda x: x).process_request(request)
+    MessageMiddleware(lambda x: x).process_request(request)
+    request.user = default_user
+
+    data = {"timestamp": timezone.now(), "ticket_ref": "1234"}
+    mock_request.return_value = MagicMock()
+    mock_request.return_value.status_code = status_code
+
+    incidents = bulk_func(request, mock_qs, data)
+
+    messages = list(get_messages(request))
+    assert len(messages) == 1
+    assert messages[0].message == expected_message_template.format(message=custom_message)
+    assert len(incidents) == 0