From 96f63ecc547c73cc27c55bd3a087a871379a4302 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Tue, 27 Oct 2026 15:06:30 +1100 Subject: [PATCH] BE: review fixes --- server/conf.py | 1 + server/messaging/notification_sections.py | 7 +- server/messaging/reporting.py | 15 ++- server/models/notification_instance.py | 14 +-- .../test_notification_down_clear_timezone.py | 108 +++++++++++++++++- .../test_notification_orphaned_events.py | 2 +- 6 files changed, 130 insertions(+), 17 deletions(-) diff --git a/server/conf.py b/server/conf.py index 4eda3e847..eebd17cc1 100755 --- a/server/conf.py +++ b/server/conf.py @@ -22,6 +22,7 @@ startTime = "" last_scan_run = "" last_version_check = "" arpscan_devices = [] +down_alert_cutoff = "" # ACTUAL CONFIGRATION ITEMS set to defaults diff --git a/server/messaging/notification_sections.py b/server/messaging/notification_sections.py index 14d826811..058700634 100644 --- a/server/messaging/notification_sections.py +++ b/server/messaging/notification_sections.py @@ -41,7 +41,10 @@ DATETIME_FIELDS = { # # Placeholders: # {condition} — optional WHERE clause appended by condition builder -# {alert_down_minutes} — runtime value, only used by down_devices +# {alert_down_cutoff} — UTC 'YYYY-MM-DD HH:MM:SS' cutoff computed once per +# get_notifications() call (also stored in +# conf.down_alert_cutoff for the clear step), only +# used by down_devices # # Every section here reads through Events_Devices, which LEFT JOINs Devices # onto Events - a device's pending event survives deleting the device (Events @@ -85,7 +88,7 @@ SQL_TEMPLATES = { FROM Events_Devices AS down_events WHERE evePendingAlertEmail = 1 AND down_events.eveEventType = 'Device Down' - AND eveDateTime < datetime('now', '-{alert_down_minutes} minutes') + AND eveDateTime < '{alert_down_cutoff}' AND down_events.devMac IS NOT NULL AND NOT EXISTS ( SELECT 1 diff --git a/server/messaging/reporting.py b/server/messaging/reporting.py index cd001b2a7..72eafb04d 100755 --- a/server/messaging/reporting.py +++ b/server/messaging/reporting.py @@ -13,6 +13,7 @@ import os import json import sys +from datetime import timedelta from zoneinfo import ZoneInfo # Register NetAlertX directories @@ -24,7 +25,7 @@ from helper import ( # noqa: E402 [flake8 lint suppression] ) from logger import mylog # noqa: E402 [flake8 lint suppression] from db.sql_safe_builder import create_safe_condition_builder # noqa: E402 [flake8 lint suppression] -from utils.datetime_utils import format_date_iso # noqa: E402 [flake8 lint suppression] +from utils.datetime_utils import format_date_iso, timeNowUTC, DATETIME_PATTERN # noqa: E402 [flake8 lint suppression] from messaging.notification_sections import ( # noqa: E402 [flake8 lint suppression] SECTION_ORDER, SECTION_TITLES, @@ -126,7 +127,13 @@ def get_notifications(db): AND eveMac IN (SELECT devMac FROM Devices WHERE devAlertDown = 0) """) + # Computed once and shared with NotificationInstance.clearPendingEmailFlag() + # via conf, so the clear step acts on exactly the events selected here. alert_down_minutes = int(get_setting_value("NTFPRCS_alert_down_time") or 0) + alert_down_cutoff = ( + timeNowUTC(as_string=False) - timedelta(minutes=alert_down_minutes) + ).strftime(DATETIME_PATTERN) + conf.down_alert_cutoff = alert_down_cutoff sections = get_setting_value("NTFPRCS_INCLUDED_SECTIONS") or [] mylog("verbose", ["[Notification] Included sections: ", sections]) @@ -158,7 +165,7 @@ def get_notifications(db): return "" # SQL templates with placeholders for runtime values - # {condition} and {alert_down_minutes} are formatted at query time + # {condition} and {alert_down_cutoff} are formatted at query time # Initialize final structure final_json = {} @@ -197,7 +204,7 @@ def get_notifications(db): # Format template with runtime placeholders format_vars = {"condition": safe_condition} if section == "down_devices": - format_vars["alert_down_minutes"] = alert_down_minutes + format_vars["alert_down_cutoff"] = alert_down_cutoff if section == "events": # 'Down Reconnected' has its own dedicated section; exclude it # from events when that section is also active to prevent the @@ -212,7 +219,7 @@ def get_notifications(db): mylog("verbose", [f"[Notification] Error building condition for {section}: ", e]) fallback_vars = {"condition": ""} if section == "down_devices": - fallback_vars["alert_down_minutes"] = alert_down_minutes + fallback_vars["alert_down_cutoff"] = alert_down_cutoff if section == "events": if "down_reconnected" in sections: fallback_vars["event_types"] = "'Connected', 'Disconnected','IP Changed'" diff --git a/server/models/notification_instance.py b/server/models/notification_instance.py index 284cb7c7a..32a7fb2ef 100755 --- a/server/models/notification_instance.py +++ b/server/models/notification_instance.py @@ -251,20 +251,20 @@ class NotificationInstance: WHERE evePendingAlertEmail = 1 AND eveEventType !='Device Down' """) - # Clear down events flag after the reporting window passed. eveDateTime - # is always UTC (timeNowUTC()), matching the down_devices selection - # query's own UTC-only cutoff in notification_sections.py - applying a - # timezone offset here would desync the two (#1851). - minutes = int(get_setting_value("NTFPRCS_alert_down_time") or 0) + # Clear down events flag using the exact UTC cutoff the down_devices + # selection query used (conf.down_alert_cutoff, set by + # get_notifications()), so an event that crossed the threshold after + # selection isn't cleared without ever being reported. eveDateTime is + # always UTC - applying a timezone offset here would desync the two (#1851). self.db.sql.execute( """ UPDATE Events SET evePendingAlertEmail = 0 WHERE evePendingAlertEmail = 1 AND eveEventType = 'Device Down' - AND eveDateTime < datetime('now', ?) + AND eveDateTime < ? """, - (f"-{minutes} minutes",), + (conf.down_alert_cutoff,), ) mylog("minimal", ["[Notification] Notifications changes: ", self.db.sql.rowcount]) diff --git a/test/backend/test_notification_down_clear_timezone.py b/test/backend/test_notification_down_clear_timezone.py index 167bc90f9..bb2f8e929 100644 --- a/test/backend/test_notification_down_clear_timezone.py +++ b/test/backend/test_notification_down_clear_timezone.py @@ -19,7 +19,8 @@ import sys import os import types import unittest -from unittest.mock import patch +from datetime import datetime, timedelta, timezone +from unittest.mock import Mock, patch from zoneinfo import ZoneInfo sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) @@ -57,6 +58,9 @@ from models.notification_instance import NotificationInstance # noqa: E402 if _json2table_was_stubbed: sys.modules.pop("json2table", None) +from messaging.reporting import get_notifications # noqa: E402 +from utils.datetime_utils import timeNowUTC, DATETIME_PATTERN # noqa: E402 + def _make_db(): """make_db() plus Plugins_Events, which clearPendingEmailFlag() always @@ -85,12 +89,20 @@ def _get_flag(conn, mac): return row[0] if row else None -def _clear_with_tz(conn, alert_down_minutes, tz_name): +def _clear_with_tz(conn, alert_down_minutes, tz_name, selection_cutoff=None): """Instantiate NotificationInstance without running its real __init__ (which needs a live settings DB/Logger) and call clearPendingEmailFlag() - with conf.tz set to tz_name, restoring the previous conf.tz afterward.""" + with conf.tz set to tz_name and conf.down_alert_cutoff set to + selection_cutoff (the cutoff get_notifications() would have recorded), + restoring both afterward. When selection_cutoff is None the cutoff is + derived from alert_down_minutes, as get_notifications() would have.""" original_tz = getattr(conf, "tz", None) + had_cutoff = hasattr(conf, "down_alert_cutoff") + original_cutoff = getattr(conf, "down_alert_cutoff", None) conf.tz = ZoneInfo(tz_name) if tz_name else None + conf.down_alert_cutoff = ( + selection_cutoff if selection_cutoff is not None else minutes_ago(alert_down_minutes) + ) try: ni = NotificationInstance.__new__(NotificationInstance) ni.db = DummyDB(conn) @@ -101,6 +113,10 @@ def _clear_with_tz(conn, alert_down_minutes, tz_name): ni.clearPendingEmailFlag() finally: conf.tz = original_tz + if had_cutoff: + conf.down_alert_cutoff = original_cutoff + else: + del conf.down_alert_cutoff class TestClearPendingEmailFlagTimezone(unittest.TestCase): @@ -173,5 +189,91 @@ class TestClearPendingEmailFlagTimezone(unittest.TestCase): ) +class TestClearPendingEmailFlagSelectionCutoff(unittest.TestCase): + """The clear step must reuse the cutoff the down_devices selection query + used, not recompute eligibility against the current time: plugins run + between selection and clear, so an event can cross the threshold in that + gap and would otherwise be cleared without ever having been notified.""" + + def test_event_crossing_cutoff_after_selection_stays_pending(self): + """Selection ran with a 5-minute cutoff, when this 4-minute-old event + was not yet eligible. Recomputing from 'now' with a 3-minute window + stands in for the time that elapsed since selection: the event looks + eligible now, but was never selected so must stay pending.""" + conn = _make_db() + mac = "aa:bb:cc:dd:ee:04" + insert_device(conn, mac, alert_down=1) + _insert_down_event(conn, mac, minutes_old=4) + conn.commit() + + _clear_with_tz( + conn, + alert_down_minutes=3, + tz_name=None, + selection_cutoff=minutes_ago(5), + ) + + self.assertEqual( + _get_flag(conn, mac), + 1, + "Event not eligible at selection time must not be cleared.", + ) + + def test_event_selected_before_cutoff_is_cleared(self): + """An event older than the selection-time cutoff was selected for the + alert, so it must be cleared.""" + conn = _make_db() + mac = "aa:bb:cc:dd:ee:05" + insert_device(conn, mac, alert_down=1) + _insert_down_event(conn, mac, minutes_old=10) + conn.commit() + + _clear_with_tz( + conn, + alert_down_minutes=60, + tz_name=None, + selection_cutoff=minutes_ago(5), + ) + + self.assertEqual(_get_flag(conn, mac), 0) + + def test_get_notifications_records_cutoff_used_in_query(self): + """get_notifications() computes the cutoff once, records it on conf for + the clear step and embeds the same value in the down_devices query.""" + mock_db = Mock() + mock_json_obj = Mock() + mock_json_obj.columnNames = [] + mock_json_obj.json = {"data": []} + mock_db.get_table_as_json.return_value = mock_json_obj + + settings = { + "NTFPRCS_INCLUDED_SECTIONS": ["down_devices"], + "NTFPRCS_alert_down_time": "5", + } + had_cutoff = hasattr(conf, "down_alert_cutoff") + original_cutoff = getattr(conf, "down_alert_cutoff", None) + try: + before = timeNowUTC(as_string=False) + with patch( + "messaging.reporting.get_setting_value", + side_effect=lambda key: settings.get(key, ""), + ): + get_notifications(mock_db) + after = timeNowUTC(as_string=False) + + cutoff = conf.down_alert_cutoff + query = mock_db.get_table_as_json.call_args[0][0] + finally: + if had_cutoff: + conf.down_alert_cutoff = original_cutoff + elif hasattr(conf, "down_alert_cutoff"): + del conf.down_alert_cutoff + + self.assertIn(f"'{cutoff}'", query) + cutoff_dt = datetime.strptime(cutoff, DATETIME_PATTERN).replace(tzinfo=timezone.utc) + self.assertGreaterEqual(cutoff_dt, before - timedelta(minutes=5)) + self.assertLessEqual(cutoff_dt, after - timedelta(minutes=5)) + + if __name__ == "__main__": unittest.main() diff --git a/test/backend/test_notification_orphaned_events.py b/test/backend/test_notification_orphaned_events.py index 9cd7916a1..c1aa73f7d 100644 --- a/test/backend/test_notification_orphaned_events.py +++ b/test/backend/test_notification_orphaned_events.py @@ -81,7 +81,7 @@ class TestDownDevicesExcludesOrphanedEvent: _insert_event(conn, EXISTING_MAC, "Device Down", minutes_old=10) _insert_event(conn, DELETED_MAC, "Device Down", minutes_old=10) - rows = _run_template(conn, "down_devices", alert_down_minutes=0) + rows = _run_template(conn, "down_devices", alert_down_cutoff=now_utc()) macs = {r["eveMac"] for r in rows} assert EXISTING_MAC in macs