From a75a3513f36847fd01fc42cc94f473f307cf1115 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Fri, 15 Nov 2024 12:02:43 +0100 Subject: [PATCH 01/14] Fix wrong end_time /.. also, attempt to clean up/document event types, and event type validation in the API. --- src/argus/incident/models.py | 79 +++++++++++++++++++++++++++++++-- src/argus/incident/views.py | 53 ++++++++++++---------- tests/incident/test_incident.py | 32 +++++++++++++ 3 files changed, 138 insertions(+), 26 deletions(-) diff --git a/src/argus/incident/models.py b/src/argus/incident/models.py index c1dabd66d..f7225c652 100644 --- a/src/argus/incident/models.py +++ b/src/argus/incident/models.py @@ -4,6 +4,7 @@ import logging from operator import and_ from random import randint, choice +from typing import Optional from urllib.parse import urljoin from django.contrib.auth import get_user_model @@ -215,7 +216,28 @@ class Type(models.TextChoices): Type.INCIDENT_CHANGE, Type.STATELESS, } - ALLOWED_TYPES_FOR_END_USERS = {Type.CLOSE, Type.REOPEN, Type.ACKNOWLEDGE, Type.OTHER} + ALLOWED_TYPES_FOR_END_USERS = { + Type.ACKNOWLEDGE, + Type.OTHER, + Type.CLOSE, + Type.REOPEN, + } + CLOSING_TYPES = { + Type.INCIDENT_END, + Type.CLOSE, + } + OPENING_TYPES = { + Type.INCIDENT_START, + Type.REOPEN, + } + STATE_TYPES = OPENING_TYPES | CLOSING_TYPES + SHARED_TYPES = { + Type.ACKNOWLEDGE, + Type.OTHER, + Type.INCIDENT_CHANGE, + } + STATELESS_TYPES = SHARED_TYPES | {Type.STATELESS} + STATEFUL_TYPES = SHARED_TYPES | STATE_TYPES incident = models.ForeignKey(to="Incident", on_delete=models.PROTECT, related_name="events") actor = models.ForeignKey(to=User, on_delete=models.PROTECT, related_name="caused_events") @@ -334,8 +356,9 @@ def create_events(self, actor: User, event_type: Event.Type, timestamp=None, des def close(self, actor: User, timestamp=None, description=""): "Close incidents correctly and create the needed events" + timestamp = timestamp or timezone.now() qs = self.open() - qs.update(end_time=timestamp or timezone.now()) + qs.update(end_time=timestamp) qs = self.all() # Reload changes from database event_type = Event.Type.CLOSE events = qs.create_events(actor, event_type, timestamp, description) @@ -439,17 +462,36 @@ def tags(self): def incident_relations(self): return IncidentRelation.objects.filter(Q(incident1=self) | Q(incident2=self)) + def all_opening_events(self): + open_events = (Event.Type.INCIDENT_START, Event.Type.REOPEN) + return self.events.filter(type__in=open_events).order_by("timestamp") + + def all_reopen_events(self): + return self.events.filter(typen=Event.Type.REOPEN).order_by("timestamp") + + def all_closing_events(self): + close_events = (Event.Type.CLOSE, Event.Type.INCIDENT_END) + return self.events.filter(type__in=close_events).order_by("timestamp") + @property def start_event(self): return self.events.filter(type=Event.Type.INCIDENT_START).order_by("timestamp").first() + @property + def reopen_event(self): + return self.all_reopen_events.last() + @property def end_event(self): return self.events.filter(type=Event.Type.INCIDENT_END).order_by("timestamp").first() + @property + def close_event(self): + return self.events.filter(type=Event.Type.CLOSE).order_by("timestamp").first() + @property def last_close_or_end_event(self): - return self.events.filter(type__in=(Event.Type.CLOSE, Event.Type.INCIDENT_END)).order_by("timestamp").last() + return self.all_close_events().last() @property def latest_change_event(self): @@ -475,6 +517,37 @@ def acked(self): return self.events.filter((acks_query & acks_not_expired_query) | ack_is_just_being_created).exists() + def event_already_exists(self, event_type): + return self.events.filter(type=event_type).exists() + + def repair_end_time(self) -> Optional[bool]: + if not self.stateful: + return + + if self.stateless_event: + # Weird, stateless event without stateless end_time, fix + self.end_time = None + self.save() + LOG.warn("Mismatch between self %s end_time and event type: set stateless", self.pk) + return True + + close_events = self.all_close_events() + if not self.open and close_events.exists(): + # end_time is already set closed (golden path) + return False + + reopen_event = self.reopen_event + last_close_event = close_events.last() + if not reopen_event or reopen_event.timestamp < last_close_event.timestamp: + # end_time was not set when making closing event, fix + self.end_time = close_events.first().timestamp + self.save() + LOG.warn("Mismatch between self %s end_time and event type: set end_time to less than infinity", self.pk) + return True + + # a reopen event correctly exists and the incident is correctly open + return False + def is_acked_by(self, group: str) -> bool: return group in self.acks.active().group_names() diff --git a/src/argus/incident/views.py b/src/argus/incident/views.py index 407c5c648..54257b876 100644 --- a/src/argus/incident/views.py +++ b/src/argus/incident/views.py @@ -474,37 +474,44 @@ def perform_create(self, serializer: EventSerializer): def validate_event_type_for_user(self, event_type: str, user: User): if user.is_source_system: if event_type not in Event.ALLOWED_TYPES_FOR_SOURCE_SYSTEMS: - self._raise_type_validation_error(f"A source system cannot post events of type '{event_type}'.") + self._abort_due_to_type_validation_error(f"A source system cannot post events of type '{event_type}'.") else: if event_type not in Event.ALLOWED_TYPES_FOR_END_USERS: - self._raise_type_validation_error(f"An end user cannot post events of type '{event_type}'.") + self._abort_due_to_type_validation_error(f"An end user cannot post events of type '{event_type}'.") def validate_event_type_for_incident(self, event_type: str, incident: Incident): - def validate_incident_has_no_relation_to_event_type(): - if incident.events.filter(type=event_type).exists(): - self._raise_type_validation_error(f"The incident already has a related event of type '{event_type}'.") - - if incident.stateful: - if event_type in {Event.Type.INCIDENT_START, Event.Type.INCIDENT_END}: - validate_incident_has_no_relation_to_event_type() - if event_type in {Event.Type.INCIDENT_END, Event.Type.CLOSE} and not incident.open: - self._raise_type_validation_error("The incident is already closed.") - elif event_type == Event.Type.REOPEN and incident.open: - self._raise_type_validation_error("The incident is already open.") - else: - if event_type == Event.Type.STATELESS: - validate_incident_has_no_relation_to_event_type() - elif event_type == Event.Type.INCIDENT_START: - self._raise_type_validation_error("Stateless incident cannot have an INCIDENT_START event.") - elif event_type in {Event.Type.INCIDENT_END, Event.Type.CLOSE, Event.Type.REOPEN}: - self._raise_type_validation_error("Cannot change the state of a stateless incident.") + def abort_due_to_too_many_events(incident, event_type): + error_msg = f"Incident #{incident.pk} can only have one event of type '{event_type}'." + LOG.warn(error_msg) + self._abort_due_to_type_validation_error(error_msg) if event_type == Event.Type.ACKNOWLEDGE: acks_endpoint = reverse("incident:incident-acks", args=[incident.pk], request=self.request) - self._raise_type_validation_error( - f"Acknowledgements of this incidents should be posted through {acks_endpoint}." + self._abort_due_to_type_validation_error( + f"Acknowledgement of an incident should be posted through {acks_endpoint}." ) + if incident.stateful: + if incident.event_already_exists(event_type): + if event_type == Event.Type.INCIDENT_START: + abort_due_to_too_many_events(incident, event_type) + if event_type == Event.Type.INCIDENT_END: + incident.repair_end_time() + abort_due_to_too_many_events(incident, event_type) + if event_type in Event.CLOSING_TYPES and not incident.open: + self._abort_due_to_type_validation_error("The incident is already closed.") + if event_type == Event.Type.REOPEN and incident.open: + self._abort_due_to_type_validation_error("The incident is already open.") + + # type ok for stateful + return + + # stateless from here + if event_type == Event.Type.STATELESS and incident.event_already_exists(event_type): + abort_due_to_too_many_events(incident, event_type) + if event_type in Event.STATE_TYPES: + self._abort_due_to_type_validation_error("Cannot change the state of a stateless incident.") + def update_incident(self, validated_data: dict, incident: Incident): timestamp = validated_data["timestamp"] event_type = validated_data["type"] @@ -516,7 +523,7 @@ def update_incident(self, validated_data: dict, incident: Incident): incident.save() @staticmethod - def _raise_type_validation_error(message: str): + def _abort_due_to_type_validation_error(message: str): raise serializers.ValidationError({"type": message}) diff --git a/tests/incident/test_incident.py b/tests/incident/test_incident.py index 6dddffb11..90ac27333 100644 --- a/tests/incident/test_incident.py +++ b/tests/incident/test_incident.py @@ -11,6 +11,7 @@ ) from argus.incident.models import Event from argus.incident.factories import SourceSystemFactory, SourceUserFactory +from argus.util.datetime_utils import INFINITY_REPR from argus.util.testing import disconnect_signals, connect_signals @@ -89,3 +90,34 @@ def tearDown(self): def test_level_str_returns_name_of_level(self): incident = StatefulIncidentFactory(level=1) self.assertEqual(incident.pp_level(), "Critical") + + +class IncidentRepairEndTimeTests(TestCase): + def setup(self): + disconnect_signals() + + def tearDown(self): + connect_signals() + + def test_golden_path(self): + incident = StatefulIncidentFactory(level=1) + end_time = incident.end_time + self.assertFalse(incident.repair_end_time()) + self.assertEqual(end_time, incident.end_time) + + def test_stateless_event_on_stateful_incident_should_fix_end_time(self): + incident = StatefulIncidentFactory(level=1) + end_time = incident.end_time + EventFactory(incident=incident, type=Event.Type.STATELESS) + self.asserTrue(incident.repair_end_time()) + self.assertNotEqual(end_time, incident.end_time) + self.assertEqual(incident.end_time, None) + + def test_closing_event_without_finite_end_time_and_reopen_event_should_fix_endt_time(self): + incident = StatefulIncidentFactory(level=1) + end_time = incident.end_time + self.assertEqual(incident.end_time, INFINITY_REPR) + EventFactory(incident=incident, type=Event.Type.INCIDENT_END) + self.asserTrue(incident.repair_end_time()) + self.assertNotEqual(end_time, incident.end_time) + self.assertLessThan(incident.end_time, INFINITY_REPR) From f837230247f9865ad9783454b77006900c411c1f Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Fri, 15 Nov 2024 12:48:55 +0100 Subject: [PATCH 02/14] fix typo --- src/argus/incident/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/argus/incident/models.py b/src/argus/incident/models.py index f7225c652..ce448dd7d 100644 --- a/src/argus/incident/models.py +++ b/src/argus/incident/models.py @@ -491,7 +491,7 @@ def close_event(self): @property def last_close_or_end_event(self): - return self.all_close_events().last() + return self.all_closing_events().last() @property def latest_change_event(self): @@ -531,7 +531,7 @@ def repair_end_time(self) -> Optional[bool]: LOG.warn("Mismatch between self %s end_time and event type: set stateless", self.pk) return True - close_events = self.all_close_events() + close_events = self.all_closing_events() if not self.open and close_events.exists(): # end_time is already set closed (golden path) return False From 234cc22a1a933a2ccb8581768bb05ded9e64521f Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Fri, 15 Nov 2024 13:15:03 +0100 Subject: [PATCH 03/14] fix typo --- src/argus/incident/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/argus/incident/models.py b/src/argus/incident/models.py index ce448dd7d..f276af776 100644 --- a/src/argus/incident/models.py +++ b/src/argus/incident/models.py @@ -479,7 +479,7 @@ def start_event(self): @property def reopen_event(self): - return self.all_reopen_events.last() + return self.all_reopen_events().last() @property def end_event(self): From 240a44f27f3146ccb0f9b058219b10be6eb1251c Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Fri, 15 Nov 2024 13:17:04 +0100 Subject: [PATCH 04/14] fix test problem --- tests/incident/test_incident.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/incident/test_incident.py b/tests/incident/test_incident.py index 90ac27333..175e417cf 100644 --- a/tests/incident/test_incident.py +++ b/tests/incident/test_incident.py @@ -1,4 +1,4 @@ -from datetime import timedelta +from datetime import timedelta, datetime from django.test import TestCase from django.utils import timezone @@ -116,8 +116,8 @@ def test_stateless_event_on_stateful_incident_should_fix_end_time(self): def test_closing_event_without_finite_end_time_and_reopen_event_should_fix_endt_time(self): incident = StatefulIncidentFactory(level=1) end_time = incident.end_time - self.assertEqual(incident.end_time, INFINITY_REPR) + self.assertEqual(incident.end_time, datetime.max) EventFactory(incident=incident, type=Event.Type.INCIDENT_END) self.asserTrue(incident.repair_end_time()) self.assertNotEqual(end_time, incident.end_time) - self.assertLessThan(incident.end_time, INFINITY_REPR) + self.assertLessThan(incident.end_time, datetime.max) From aa56697b3f35976f56379caf0fd0152f46468e2c Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Fri, 15 Nov 2024 13:20:01 +0100 Subject: [PATCH 05/14] add changelog fragment --- changelog.d/+fix-wrong-end-time.changed.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelog.d/+fix-wrong-end-time.changed.md diff --git a/changelog.d/+fix-wrong-end-time.changed.md b/changelog.d/+fix-wrong-end-time.changed.md new file mode 100644 index 000000000..73d269fdb --- /dev/null +++ b/changelog.d/+fix-wrong-end-time.changed.md @@ -0,0 +1,2 @@ +Ensure that `end_time` is correct according to state: `None` for stateless, +less than datetime.max for closed. From c0975fbea1ac17711709eb6fbdeafb7a86e2d2a8 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Mon, 9 Dec 2024 14:20:01 +0100 Subject: [PATCH 06/14] lint --- tests/incident/test_incident.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/incident/test_incident.py b/tests/incident/test_incident.py index 175e417cf..92c82dc83 100644 --- a/tests/incident/test_incident.py +++ b/tests/incident/test_incident.py @@ -11,7 +11,6 @@ ) from argus.incident.models import Event from argus.incident.factories import SourceSystemFactory, SourceUserFactory -from argus.util.datetime_utils import INFINITY_REPR from argus.util.testing import disconnect_signals, connect_signals From ed86365f689f60aeb691cf09d2e9995c76cef13a Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Tue, 10 Dec 2024 08:28:06 +0100 Subject: [PATCH 07/14] fixup: use constants --- src/argus/incident/models.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/argus/incident/models.py b/src/argus/incident/models.py index f276af776..cef89fee8 100644 --- a/src/argus/incident/models.py +++ b/src/argus/incident/models.py @@ -463,14 +463,14 @@ def incident_relations(self): return IncidentRelation.objects.filter(Q(incident1=self) | Q(incident2=self)) def all_opening_events(self): - open_events = (Event.Type.INCIDENT_START, Event.Type.REOPEN) + open_events = Event.OPENING_TYPES return self.events.filter(type__in=open_events).order_by("timestamp") def all_reopen_events(self): - return self.events.filter(typen=Event.Type.REOPEN).order_by("timestamp") + return self.events.filter(type=Event.Type.REOPEN).order_by("timestamp") def all_closing_events(self): - close_events = (Event.Type.CLOSE, Event.Type.INCIDENT_END) + close_events = Event.CLOSING_TYPES return self.events.filter(type__in=close_events).order_by("timestamp") @property From 4849416985bda1a4624dfabbfe07714e4b2ea0f9 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Tue, 10 Dec 2024 10:42:48 +0100 Subject: [PATCH 08/14] not finished: improved/safer repair logic and reporting --- src/argus/incident/models.py | 37 +++++++++++++++++++++++++++++++++--- src/argus/incident/views.py | 13 +++++++++++-- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/argus/incident/models.py b/src/argus/incident/models.py index cef89fee8..bb95eba65 100644 --- a/src/argus/incident/models.py +++ b/src/argus/incident/models.py @@ -521,7 +521,21 @@ def event_already_exists(self, event_type): return self.events.filter(type=event_type).exists() def repair_end_time(self) -> Optional[bool]: + """Repairs end_time if there is a mismatch between events and end_time + + This can happen under race-conditions and because we still cannot use + the ``atomic``-decorator everwhere. + + Returns: + * True if a repair needed to be made + * False if it was stateful and ok + * None if it was stateless and ok + """ + LOG.info("Incident %s: Detected potential mismatch of end_time and events", self.pk) + if not self.stateful: + # the vital part for statelessness is set correctly + LOG.info("Incident %s: No mismatch, correctly stateless", self.pk) return if self.stateless_event: @@ -531,21 +545,38 @@ def repair_end_time(self) -> Optional[bool]: LOG.warn("Mismatch between self %s end_time and event type: set stateless", self.pk) return True + # Only stateful incidents from this point on + close_events = self.all_closing_events() - if not self.open and close_events.exists(): - # end_time is already set closed (golden path) + if not close_events.exists(): + if self.open: + # Golden path for open incidents + LOG.info("Incident %s: No mismatch, correctly stateful and open", self.pk) + return False + else: + # missing close event. This is serious. + message = "Incident %s has been closed without adding an event" + LOG.error(message, self.pk) + raise ValueError(message) + + # Only incidents with at least one close event from this point on + + if not self.open: + # Golden path for closed incidents + LOG.info("Incident %s: No mismatch, correctly stateful and closed", self.pk) return False reopen_event = self.reopen_event last_close_event = close_events.last() if not reopen_event or reopen_event.timestamp < last_close_event.timestamp: # end_time was not set when making closing event, fix - self.end_time = close_events.first().timestamp + self.end_time = last_close_event.timestamp self.save() LOG.warn("Mismatch between self %s end_time and event type: set end_time to less than infinity", self.pk) return True # a reopen event correctly exists and the incident is correctly open + LOG.info("Incident %s: No mismatch, correctly stateful and reopened", self.pk) return False def is_acked_by(self, group: str) -> bool: diff --git a/src/argus/incident/views.py b/src/argus/incident/views.py index 54257b876..f6da84907 100644 --- a/src/argus/incident/views.py +++ b/src/argus/incident/views.py @@ -465,6 +465,9 @@ def perform_create(self, serializer: EventSerializer): # is sent after the incident has been manually closed if not user.is_source_system: raise e + except AttributeError: + # Do not save new event, it was redundant + return else: # Only update incident if everything is valid; otherwise, just record the event self.update_incident(serializer.validated_data, incident) @@ -494,10 +497,16 @@ def abort_due_to_too_many_events(incident, event_type): if incident.stateful: if incident.event_already_exists(event_type): if event_type == Event.Type.INCIDENT_START: + # Only ever 1 abort_due_to_too_many_events(incident, event_type) if event_type == Event.Type.INCIDENT_END: - incident.repair_end_time() - abort_due_to_too_many_events(incident, event_type) + # Only ever 1, but might not have been saved correctly earlier + repaired = incident.repair_end_time() + if repaired: + raise AttributeError("end_time mismatch repaired, see logs") + # should never happen + LOG.error("Something weird happened, see other logs") + raise AttributeError("end_time mismatch was in error, see logs") if event_type in Event.CLOSING_TYPES and not incident.open: self._abort_due_to_type_validation_error("The incident is already closed.") if event_type == Event.Type.REOPEN and incident.open: From 1fdac4511f3767150daa676f5498e67d4645314d Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Tue, 10 Dec 2024 13:58:21 +0100 Subject: [PATCH 09/14] add new exceptions --- src/argus/incident/models.py | 3 ++- src/argus/incident/views.py | 9 +++++---- src/argus/util/exceptions.py | 13 +++++++++++++ 3 files changed, 20 insertions(+), 5 deletions(-) create mode 100644 src/argus/util/exceptions.py diff --git a/src/argus/incident/models.py b/src/argus/incident/models.py index bb95eba65..c160647a5 100644 --- a/src/argus/incident/models.py +++ b/src/argus/incident/models.py @@ -15,6 +15,7 @@ from django.utils import timezone from argus.util.datetime_utils import INFINITY_REPR, get_infinity_repr +from argus.util.exceptions import InconsistentDatabaseException from .constants import Level from .fields import DateTimeInfinityField from .validators import validate_lowercase, validate_key @@ -557,7 +558,7 @@ def repair_end_time(self) -> Optional[bool]: # missing close event. This is serious. message = "Incident %s has been closed without adding an event" LOG.error(message, self.pk) - raise ValueError(message) + raise InconsistentDatabaseException(message) # Only incidents with at least one close event from this point on diff --git a/src/argus/incident/views.py b/src/argus/incident/views.py index f6da84907..a32f18ebe 100644 --- a/src/argus/incident/views.py +++ b/src/argus/incident/views.py @@ -23,6 +23,7 @@ from argus.drf.permissions import IsSuperuserOrReadOnly from argus.filter import get_filter_backend from argus.util.datetime_utils import INFINITY_REPR +from argus.util.exceptions import InconceivableException, SuccessfulRepairException from argus.util.signals import bulk_changed from .forms import AddSourceSystemForm @@ -465,7 +466,7 @@ def perform_create(self, serializer: EventSerializer): # is sent after the incident has been manually closed if not user.is_source_system: raise e - except AttributeError: + except (SuccessfulRepairException, InconceivableException): # Do not save new event, it was redundant return else: @@ -503,10 +504,10 @@ def abort_due_to_too_many_events(incident, event_type): # Only ever 1, but might not have been saved correctly earlier repaired = incident.repair_end_time() if repaired: - raise AttributeError("end_time mismatch repaired, see logs") - # should never happen + raise SuccessfulRepairException("end_time mismatch repaired, see logs") + # should never happen, insufficent preceeding logic construct? LOG.error("Something weird happened, see other logs") - raise AttributeError("end_time mismatch was in error, see logs") + raise InconceivableException("Found end_time mismatch was in error, see logs") if event_type in Event.CLOSING_TYPES and not incident.open: self._abort_due_to_type_validation_error("The incident is already closed.") if event_type == Event.Type.REOPEN and incident.open: diff --git a/src/argus/util/exceptions.py b/src/argus/util/exceptions.py new file mode 100644 index 000000000..d9d0cc981 --- /dev/null +++ b/src/argus/util/exceptions.py @@ -0,0 +1,13 @@ +class SuccessfulRepairException(Exception): + # Something was repaired and we need to skip another action + pass + + +class InconceivableException(Exception): + # This should not happen, ever! + pass + + +class InconsistentDatabaseException(ValueError): + # Something is missing or doubled up in the database, or has neen accidentally deleted + pass From 5cd1754061268b34685408257211273138bba320 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Tue, 10 Dec 2024 14:19:59 +0100 Subject: [PATCH 10/14] improve exception comments --- src/argus/util/exceptions.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/argus/util/exceptions.py b/src/argus/util/exceptions.py index d9d0cc981..9e2a74ba7 100644 --- a/src/argus/util/exceptions.py +++ b/src/argus/util/exceptions.py @@ -9,5 +9,7 @@ class InconceivableException(Exception): class InconsistentDatabaseException(ValueError): - # Something is missing or doubled up in the database, or has neen accidentally deleted + # Something is missing or doubled up in the database, + # or has been accidentally deleted + # It cannot be fixed automatically so we must bail out pass From 9f5c925189099f4932d39aa15ac7a355c81d92f1 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Tue, 10 Dec 2024 14:23:21 +0100 Subject: [PATCH 11/14] fix comment where Inconceivable! is used --- src/argus/incident/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/argus/incident/views.py b/src/argus/incident/views.py index a32f18ebe..3fb810c20 100644 --- a/src/argus/incident/views.py +++ b/src/argus/incident/views.py @@ -505,7 +505,7 @@ def abort_due_to_too_many_events(incident, event_type): repaired = incident.repair_end_time() if repaired: raise SuccessfulRepairException("end_time mismatch repaired, see logs") - # should never happen, insufficent preceeding logic construct? + # should never happen, here for completeness LOG.error("Something weird happened, see other logs") raise InconceivableException("Found end_time mismatch was in error, see logs") if event_type in Event.CLOSING_TYPES and not incident.open: From b4fd6f8e986ddbf00310405f55adeee9ccf13075 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Wed, 18 Dec 2024 13:55:37 +0100 Subject: [PATCH 12/14] redirect when client is out of date --- src/argus/incident/views.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/argus/incident/views.py b/src/argus/incident/views.py index 3fb810c20..a8dc94698 100644 --- a/src/argus/incident/views.py +++ b/src/argus/incident/views.py @@ -4,6 +4,7 @@ from django.conf import settings from django.contrib.auth import get_user_model from django.db import IntegrityError +from django.http import HttpResponseRedirect from django.utils import timezone from django_filters import rest_framework as filters @@ -467,8 +468,8 @@ def perform_create(self, serializer: EventSerializer): if not user.is_source_system: raise e except (SuccessfulRepairException, InconceivableException): - # Do not save new event, it was redundant - return + # Do not save new event, it was redundant, redirect to incident + return HttpResponseRedirect(reverse("incident:incident-detail", kwargs={"pk": incident.pk})) else: # Only update incident if everything is valid; otherwise, just record the event self.update_incident(serializer.validated_data, incident) From 3d8fad48a19f43194150dc98d83ad6b6bff91ce8 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Wed, 18 Dec 2024 13:56:02 +0100 Subject: [PATCH 13/14] improve method name --- src/argus/incident/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/argus/incident/views.py b/src/argus/incident/views.py index a8dc94698..bafd11684 100644 --- a/src/argus/incident/views.py +++ b/src/argus/incident/views.py @@ -472,7 +472,7 @@ def perform_create(self, serializer: EventSerializer): return HttpResponseRedirect(reverse("incident:incident-detail", kwargs={"pk": incident.pk})) else: # Only update incident if everything is valid; otherwise, just record the event - self.update_incident(serializer.validated_data, incident) + self.update_incident_end_time(serializer.validated_data, incident) serializer.save(incident=incident, actor=user) @@ -523,7 +523,7 @@ def abort_due_to_too_many_events(incident, event_type): if event_type in Event.STATE_TYPES: self._abort_due_to_type_validation_error("Cannot change the state of a stateless incident.") - def update_incident(self, validated_data: dict, incident: Incident): + def update_incident_end_time(self, validated_data: dict, incident: Incident): timestamp = validated_data["timestamp"] event_type = validated_data["type"] if event_type in {Event.Type.INCIDENT_END, Event.Type.CLOSE}: From 68b22a78c04bd7438f56cd8f4c24a01010bc5db9 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Wed, 18 Dec 2024 13:57:40 +0100 Subject: [PATCH 14/14] improve/fix exception message --- src/argus/incident/models.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/argus/incident/models.py b/src/argus/incident/models.py index c160647a5..bb1d480c6 100644 --- a/src/argus/incident/models.py +++ b/src/argus/incident/models.py @@ -555,10 +555,10 @@ def repair_end_time(self) -> Optional[bool]: LOG.info("Incident %s: No mismatch, correctly stateful and open", self.pk) return False else: - # missing close event. This is serious. - message = "Incident %s has been closed without adding an event" + # missing close event. This is serious. 500 Internal Server Error serious. + message = "Incident %s was previously closed without the creation of an appropriate event" LOG.error(message, self.pk) - raise InconsistentDatabaseException(message) + raise InconsistentDatabaseException(message % self.pk) # Only incidents with at least one close event from this point on