From d2f3fa8c0372562fcaabd3c7e3538cc69984a51d Mon Sep 17 00:00:00 2001 From: James Ladd Date: Thu, 15 May 2025 15:15:35 +0100 Subject: [PATCH 1/6] Return instead of raise HttpResponseServerError --- src/geant_argus/geant_argus/incidents/bulk_actions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/geant_argus/geant_argus/incidents/bulk_actions.py b/src/geant_argus/geant_argus/incidents/bulk_actions.py index b8fe493..1c83672 100644 --- a/src/geant_argus/geant_argus/incidents/bulk_actions.py +++ b/src/geant_argus/geant_argus/incidents/bulk_actions.py @@ -38,7 +38,7 @@ def bulk_close_incidents(actor, qs, data: Dict[str, Any]): incidents = bulk_close_queryset(actor, qs, data) for incident in incidents: if not close_alarm(incident.source_incident_id): - raise HttpResponseServerError("Error while closing incident") + return HttpResponseServerError("Error while closing incident") incident.metadata["status"] = "CLOSED" incident.metadata["clear_time"] = data["timestamp"].isoformat() incident.save() From 2518c435dd39d9aba984fec1bf1b1b4521762e7c Mon Sep 17 00:00:00 2001 From: James Ladd Date: Thu, 15 May 2025 16:47:55 +0100 Subject: [PATCH 2/6] Remove error handling and add assertion to ensure errors are logged and propagated to UI --- src/geant_argus/geant_argus/dashboard_alarms.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/geant_argus/geant_argus/dashboard_alarms.py b/src/geant_argus/geant_argus/dashboard_alarms.py index 0ad47d7..d3c67ed 100644 --- a/src/geant_argus/geant_argus/dashboard_alarms.py +++ b/src/geant_argus/geant_argus/dashboard_alarms.py @@ -31,9 +31,6 @@ def _succeed_request(*args, timeout=5, **kwargs) -> bool: disable_synchronization = getattr(settings, "DASBHOARD_ALARMS_DISABLE_SYNCHRONIZATION", False) if disable_synchronization: return True - - try: - response = requests.request(*args, timeout=timeout, **kwargs) - except requests.Timeout: - return False - return response.status_code == 200 + response = requests.request(*args, timeout=timeout, **kwargs) + assert response.status_code == 200 + return True From daf51063e73d63198e9a2e6b24b063256783ebe0 Mon Sep 17 00:00:00 2001 From: James Ladd Date: Mon, 2 Jun 2025 15:15:02 +0100 Subject: [PATCH 3/6] Support Argus PR#1497 to get access to request --- .../geant_argus/incidents/bulk_actions.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/geant_argus/geant_argus/incidents/bulk_actions.py b/src/geant_argus/geant_argus/incidents/bulk_actions.py index 1c83672..3417907 100644 --- a/src/geant_argus/geant_argus/incidents/bulk_actions.py +++ b/src/geant_argus/geant_argus/incidents/bulk_actions.py @@ -24,18 +24,18 @@ 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) for incident in incidents: if not close_alarm(incident.source_incident_id): return HttpResponseServerError("Error while closing incident") @@ -47,7 +47,7 @@ def bulk_close_incidents(actor, qs, data: Dict[str, Any]): @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()).isoformat() incidents = list(qs) for incident in incidents: @@ -60,7 +60,7 @@ def bulk_clear_incidents(actor, qs, data: Dict[str, Any]): @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} From 4ecbe93537fd3ddf08820f14be5155120997bfd7 Mon Sep 17 00:00:00 2001 From: James Ladd Date: Mon, 2 Jun 2025 17:35:35 +0100 Subject: [PATCH 4/6] Log and send bulk action errors to the UI --- .../geant_argus/dashboard_alarms.py | 13 +++--- .../geant_argus/incidents/bulk_actions.py | 41 ++++++++++++++----- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/src/geant_argus/geant_argus/dashboard_alarms.py b/src/geant_argus/geant_argus/dashboard_alarms.py index d3c67ed..7b1d5ff 100644 --- a/src/geant_argus/geant_argus/dashboard_alarms.py +++ b/src/geant_argus/geant_argus/dashboard_alarms.py @@ -27,10 +27,13 @@ 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, "DASBHOARD_ALARMS_DISABLE_SYNCHRONIZATION", False) if disable_synchronization: - return True - response = requests.request(*args, timeout=timeout, **kwargs) - assert response.status_code == 200 - return True + return None + try: + response = requests.request(*args, timeout=timeout, **kwargs) + except requests.Timeout: + return "Timed Out" + if response.status_code != 200: + return str(response.status_code) diff --git a/src/geant_argus/geant_argus/incidents/bulk_actions.py b/src/geant_argus/geant_argus/incidents/bulk_actions.py index 3417907..b5f78e1 100644 --- a/src/geant_argus/geant_argus/incidents/bulk_actions.py +++ b/src/geant_argus/geant_argus/incidents/bulk_actions.py @@ -1,11 +1,11 @@ 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 @@ -13,6 +13,8 @@ from .common import TicketRefField +logger = logging.getLogger(__name__) + class TicketRefForm(forms.Form): ticket_ref = TicketRefField() @@ -36,27 +38,39 @@ def wrapper(request, *args, **kwargs): @bulk_action_require_write 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): - return HttpResponseServerError("Error while closing incident") + error = close_alarm(incident.source_incident_id) + if error is not None: + message = f"Error while closing incident: {error}" + logger.exception(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(request, qs, data: Dict[str, Any]): clear_time = (data["timestamp"] or timezone.now()).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"Error while clearing incident: {error}" + logger.exception(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 @@ -65,14 +79,19 @@ def bulk_update_ticket_ref(request, qs, data: Dict[str, Any]): 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"Error while updating ticket_ref: {error}" + logger.exception(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): From 48c745311ad6a07f648d3989a9e36173eeed3281 Mon Sep 17 00:00:00 2001 From: James Ladd Date: Tue, 3 Jun 2025 16:16:03 +0100 Subject: [PATCH 5/6] Improved error messages --- .../geant_argus/dashboard_alarms.py | 10 +++++++-- .../geant_argus/incidents/bulk_actions.py | 22 ++++++++++++++----- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/geant_argus/geant_argus/dashboard_alarms.py b/src/geant_argus/geant_argus/dashboard_alarms.py index 7b1d5ff..21d2024 100644 --- a/src/geant_argus/geant_argus/dashboard_alarms.py +++ b/src/geant_argus/geant_argus/dashboard_alarms.py @@ -1,4 +1,5 @@ from django.conf import settings +from http import HTTPStatus import requests UPDATE_ALARM_URL = "/alarms/{}" @@ -6,6 +7,8 @@ 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) @@ -34,6 +37,9 @@ def _succeed_request(*args, timeout=5, **kwargs) -> str | None: try: response = requests.request(*args, timeout=timeout, **kwargs) except requests.Timeout: - return "Timed Out" + return "Request timed out while updating alarm" if response.status_code != 200: - return str(response.status_code) + error_description = ERROR_MESSAGES.get( + response.status_code, HTTPStatus(response.status_code).description + ) + return f"{error_description} (HTTP {response.status_code})" diff --git a/src/geant_argus/geant_argus/incidents/bulk_actions.py b/src/geant_argus/geant_argus/incidents/bulk_actions.py index b5f78e1..62d75d4 100644 --- a/src/geant_argus/geant_argus/incidents/bulk_actions.py +++ b/src/geant_argus/geant_argus/incidents/bulk_actions.py @@ -42,8 +42,11 @@ def bulk_close_incidents(request, qs, data: Dict[str, Any]): for incident in incidents: error = close_alarm(incident.source_incident_id) if error is not None: - message = f"Error while closing incident: {error}" - logger.exception(message) + 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" @@ -62,8 +65,11 @@ def bulk_clear_incidents(request, qs, data: Dict[str, Any]): for incident in incidents: error = clear_alarm(incident.source_incident_id, {"clear_time": clear_time}) if error is not None: - message = f"Error while clearing incident: {error}" - logger.exception(message) + 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) @@ -83,8 +89,12 @@ def bulk_update_ticket_ref(request, qs, data: Dict[str, Any]): for incident in incidents: error = update_alarm(incident.source_incident_id, payload) if error is not None: - message = f"Error while updating ticket_ref: {error}" - logger.exception(message) + 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) From 35fc69b7804497485472871a707eff7f15170cbb Mon Sep 17 00:00:00 2001 From: James Ladd Date: Mon, 9 Jun 2025 12:00:18 +0100 Subject: [PATCH 6/6] Added tests for bulk action error messages --- test/test_auth.py | 2 + test/test_bulk_actions.py | 84 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/test/test_auth.py b/test/test_auth.py index af375a8..4a8846b 100644 --- a/test/test_auth.py +++ b/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) diff --git a/test/test_bulk_actions.py b/test/test_bulk_actions.py index 990dc10..06a4aad 100644 --- a/test/test_bulk_actions.py +++ b/test/test_bulk_actions.py @@ -3,8 +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 @@ -260,3 +267,78 @@ def test_bulk_clear_incidents(mock_clear_alarm, default_user): 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