BE: review fixes

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
 

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

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'"

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])

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()

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