Sfoglia il codice sorgente

fix(notifications): send the ntfy priority the dialog was collecting (issue #3139)

The per-event Priority header from #990 never reached ntfy. The dialog
builds its rows from the provider's event toggles and stores the map
under those names -- on_print_complete, on_print_failed -- while every
sender is called with the bare event name, print_complete. The lookup
missed for all 18 events the dialog offers, so every notification went
out at the ntfy server's default with the configured priority sitting
untouched in the database.

Both ends looked healthy, which is why it shipped. The stored config
held exactly what was set, and the tests were green because they called
_send_ntfy directly with the prefixed name -- the one spelling the
running system never produces.

- notification_service.py: accept either spelling, bare first, so
  existing configs keep working and nothing needs migrating.
- schemas/notification.py: document both key forms, and which one the
  UI writes.
- tests: use the bare names, and add one that runs from a finished
  print through to the outgoing request. Without the fix it fails on a
  header dict holding only Title, which is the assertion that was
  missing.

The daily digest is unchanged: send_digest sends with no event_type at
all, and the dialog offers no priority for it -- it is one message for
several events.
maziggy 1 giorno fa
parent
commit
1b20d1a968

File diff suppressed because it is too large
+ 1 - 0
CHANGELOG.md


+ 5 - 3
backend/app/schemas/notification.py

@@ -301,9 +301,11 @@ class NtfyConfig(BaseModel):
     event_priorities: dict[str, int] | None = Field(
         default=None,
         description=(
-            "Per-event priority override. Keys are event names (e.g. 'on_print_failed'); "
-            "values are ntfy priorities 1-5 (1=min, 2=low, 3=default, 4=high, 5=urgent). "
-            "Events without an entry use ntfy's server-side default."
+            "Per-event priority override. Keys are event names, either the provider's "
+            "toggle column ('on_print_failed', what the UI writes) or the bare event "
+            "name ('print_failed'); both are accepted. Values are ntfy priorities 1-5 "
+            "(1=min, 2=low, 3=default, 4=high, 5=urgent). Events without an entry use "
+            "ntfy's server-side default."
         ),
     )
 

+ 9 - 0
backend/app/services/notification_service.py

@@ -398,9 +398,18 @@ class NotificationService:
         # Per-event Priority header (#990). Only set when the user has
         # explicitly mapped this event to a 1-5 value; otherwise fall through
         # to the ntfy server's default so existing setups stay unchanged.
+        #
+        # The map is keyed by the provider's toggle column ("on_print_failed"),
+        # because that is what the dialog builds its rows from -- but every
+        # sender is called with the bare event name ("print_failed"), so the
+        # lookup used to miss for every real notification and hit only in tests
+        # that called this method with the prefixed name (issue #3139). Both
+        # spellings are accepted, which also leaves stored configs untouched.
         event_priorities = config.get("event_priorities") or {}
         if event_type and isinstance(event_priorities, dict):
             raw = event_priorities.get(event_type)
+            if raw is None and not event_type.startswith("on_"):
+                raw = event_priorities.get(f"on_{event_type}")
             try:
                 priority = int(raw) if raw is not None else None
             except (TypeError, ValueError):

+ 66 - 6
backend/tests/unit/services/test_notification_service.py

@@ -8,6 +8,7 @@ from unittest.mock import AsyncMock, MagicMock, patch
 
 import pytest
 
+from backend.app.models.notification import NotificationProvider
 from backend.app.services.notification_service import NotificationService
 
 
@@ -757,7 +758,15 @@ class TestDiscordProvider:
 
 
 class TestNtfyPriority:
-    """Per-event ntfy Priority header (#990)."""
+    """Per-event ntfy Priority header (#990).
+
+    The map is stored under the provider's toggle columns ("on_print_failed"),
+    which is what the dialog builds its rows from, but every sender is called
+    with the bare event name ("print_failed"). These tests use the bare form on
+    purpose: the feature shipped broken because they used to pass the prefixed
+    name straight into ``_send_ntfy``, the one spelling production never
+    produces, so the lookup hit here and missed everywhere else (issue #3139).
+    """
 
     @pytest.fixture
     def service(self):
@@ -783,12 +792,25 @@ class TestNtfyPriority:
         mock_client = self._mock_client(service)
         with patch.object(service, "_get_client", new_callable=AsyncMock) as mock_get:
             mock_get.return_value = mock_client
-            success, _ = await service._send_ntfy(config, "Title", "Body", event_type="on_print_failed")
+            success, _ = await service._send_ntfy(config, "Title", "Body", event_type="print_failed")
 
         assert success is True
         headers = mock_client.post.call_args.kwargs["headers"]
         assert headers.get("Priority") == "5"
 
+    @pytest.mark.asyncio
+    async def test_priority_header_set_for_bare_key(self, service):
+        """A map keyed by the bare event name resolves too, so a config written
+        by hand (or by any future caller that drops the prefix) still works."""
+        config = {"topic": "bambuddy", "event_priorities": {"print_failed": 4}}
+        mock_client = self._mock_client(service)
+        with patch.object(service, "_get_client", new_callable=AsyncMock) as mock_get:
+            mock_get.return_value = mock_client
+            await service._send_ntfy(config, "Title", "Body", event_type="print_failed")
+
+        headers = mock_client.post.call_args.kwargs["headers"]
+        assert headers.get("Priority") == "4"
+
     @pytest.mark.asyncio
     async def test_priority_header_omitted_for_unmapped_event(self, service):
         """Unmapped event → no Priority header so ntfy uses its server default."""
@@ -799,7 +821,7 @@ class TestNtfyPriority:
         mock_client = self._mock_client(service)
         with patch.object(service, "_get_client", new_callable=AsyncMock) as mock_get:
             mock_get.return_value = mock_client
-            await service._send_ntfy(config, "Title", "Body", event_type="on_print_complete")
+            await service._send_ntfy(config, "Title", "Body", event_type="print_complete")
 
         headers = mock_client.post.call_args.kwargs["headers"]
         assert "Priority" not in headers
@@ -811,7 +833,7 @@ class TestNtfyPriority:
         mock_client = self._mock_client(service)
         with patch.object(service, "_get_client", new_callable=AsyncMock) as mock_get:
             mock_get.return_value = mock_client
-            await service._send_ntfy(config, "Title", "Body", event_type="on_print_failed")
+            await service._send_ntfy(config, "Title", "Body", event_type="print_failed")
 
         headers = mock_client.post.call_args.kwargs["headers"]
         assert "Priority" not in headers
@@ -842,7 +864,7 @@ class TestNtfyPriority:
             mock_client = self._mock_client(service)
             with patch.object(service, "_get_client", new_callable=AsyncMock) as mock_get:
                 mock_get.return_value = mock_client
-                await service._send_ntfy(config, "Title", "Body", event_type="on_print_failed")
+                await service._send_ntfy(config, "Title", "Body", event_type="print_failed")
 
             headers = mock_client.post.call_args.kwargs["headers"]
             assert "Priority" not in headers, f"unexpected header for bad value {bad!r}"
@@ -862,12 +884,50 @@ class TestNtfyPriority:
                 "Title",
                 "Body",
                 image_data=b"\xff\xd8\xff\xe0fake-jpeg",
-                event_type="on_first_layer_complete",
+                event_type="first_layer_complete",
             )
 
         headers = mock_client.put.call_args.kwargs["headers"]
         assert headers.get("Priority") == "4"
 
+    @pytest.mark.asyncio
+    async def test_priority_reaches_ntfy_from_a_real_event(self, service):
+        """The wiring, end to end: a finished print, a provider configured the
+        way the dialog writes it, and the header on the request that leaves.
+
+        Everything above calls ``_send_ntfy`` directly, so none of it can see a
+        caller passing a key shape the lookup does not understand -- which is
+        exactly how #3139 shipped green.
+        """
+        provider = NotificationProvider(
+            id=1,
+            name="ntfy",
+            provider_type="ntfy",
+            enabled=True,
+            config=json.dumps({"topic": "bambuddy", "event_priorities": {"on_print_complete": 5}}),
+            quiet_hours_enabled=False,
+            daily_digest_enabled=False,
+        )
+        mock_client = self._mock_client(service)
+        mock_db = AsyncMock()
+
+        with (
+            patch.object(service, "_get_client", new_callable=AsyncMock) as mock_get,
+            patch.object(service, "_get_providers_for_event", new_callable=AsyncMock) as mock_providers,
+            patch.object(service, "_build_message_from_template", new_callable=AsyncMock) as mock_template,
+            patch.object(service, "_update_provider_status", new_callable=AsyncMock),
+            patch.object(service, "_log_notification", new_callable=AsyncMock),
+        ):
+            mock_get.return_value = mock_client
+            mock_providers.return_value = [provider]
+            mock_template.return_value = ("Print complete", "Benchy finished")
+
+            await service.on_print_complete(1, "X1C", "completed", {"filename": "benchy.3mf"}, mock_db)
+
+        mock_client.post.assert_called_once()
+        headers = mock_client.post.call_args.kwargs["headers"]
+        assert headers.get("Priority") == "5"
+
 
 class TestHomeAssistantProvider:
     """Tests for Home Assistant notification provider."""

Some files were not shown because too many files changed in this diff