Further notification tweaks (#671)

- Improved multi-URL notification handling
- Tweak Apprise validation to catch errors earlier
- Much improved notification logging and UI response
- More robust notification tests

shelfmark/core/notifications.py

@@ -163,6 +163,40 @@ def _log_apprise_records(records: Iterable[tuple[int, str, str, str]]) -> None:
             logger.info("Apprise source [%s]: %s", source_name, full_message)
 
 
+def _log_apprise_exception_debug(*, action: str, scheme: str, exc: Exception) -> None:
+    logger.debug(
+        "Apprise %s raised %s for scheme '%s': %s",
+        action,
+        type(exc).__name__,
+        scheme,
+        exc,
+        exc_info=True,
+    )
+
+
+def _build_apprise_warning_detail(
+    records: Iterable[tuple[int, str, str, str]],
+    *,
+    scheme: str,
+) -> str | None:
+    for level, source, raw_message, raw_exception_summary in records:
+        if level < logging.WARNING:
+            continue
+
+        message = str(raw_message or "").strip()
+        if not message:
+            continue
+
+        source_name = str(source or "").strip()
+        exception_summary = str(raw_exception_summary or "").strip()
+        full_message = message if not exception_summary else f"{message} ({exception_summary})"
+
+        if source_name and source_name != _APPRISE_LOGGER_NAME:
+            return f"{scheme}: {source_name}: {full_message}"
+        return f"{scheme}: {full_message}"
+    return None
+
+
 def _normalize_routes(value: Any) -> list[dict[str, str]]:
     if not isinstance(value, list):
         return []
@@ -305,6 +339,31 @@ def _render_message(context: NotificationContext) -> tuple[str, str]:
     return "Download Failed", f'Failed to download "{title}" by {author}.{error_line}'
 
 
+def _plugin_label(plugin: Any, fallback_scheme: str) -> str:
+    """Build a human-readable label from a validated Apprise plugin.
+
+    Combines the URL scheme with the plugin's service name (app_id) and
+    privacy-safe URL for richer diagnostics, e.g.
+    ``"slack (Slack - slack://TokenA/To...n/To...n/)"``
+    """
+    parts: list[str] = [fallback_scheme]
+
+    app_id = getattr(plugin, "app_id", None)
+    if app_id and str(app_id) != fallback_scheme:
+        privacy_url: str | None = None
+        try:
+            privacy_url = plugin.url(privacy=True)
+        except Exception:
+            pass
+
+        suffix = str(app_id)
+        if privacy_url:
+            suffix = f"{suffix} - {privacy_url}"
+        parts.append(f"({suffix})")
+
+    return " ".join(parts)
+
+
 def _dispatch_to_apprise(
     urls: Iterable[str],
     *,
@@ -320,70 +379,127 @@ def _dispatch_to_apprise(
     if apprise is None:
         return {"success": False, "message": "Apprise is not installed"}
 
-    apobj = _create_apprise_client()
-    if apobj is None:
-        return {"success": False, "message": "Apprise is not installed"}
-    with _capture_apprise_logs(min_level=logging.INFO) as apprise_records:
-        valid_urls = 0
-        invalid_urls = 0
-        for url in normalized_urls:
-            scheme = urlsplit(url).scheme or "unknown"
+    valid_urls = 0
+    invalid_urls = 0
+    delivered_urls = 0
+    failed_delivery_urls = 0
+    failure_details: list[str] = []
+
+    for url in normalized_urls:
+        scheme = urlsplit(url).scheme or "unknown"
+        apobj = _create_apprise_client()
+        if apobj is None:
+            return {"success": False, "message": "Apprise is not installed"}
+
+        registration_failure_detail: str | None = None
+        with _capture_apprise_logs(min_level=logging.INFO) as apprise_records:
             try:
-                added = bool(apobj.add(url))
+                plugin = apprise.Apprise.instantiate(url, asset=getattr(apobj, "asset", None))
             except Exception as exc:
                 logger.warning(
                     "Failed to register notification route URL for scheme '%s': %s",
                     scheme,
                     exc,
                 )
-                added = False
-            if added:
-                valid_urls += 1
-            else:
+                _log_apprise_exception_debug(
+                    action="route registration",
+                    scheme=scheme,
+                    exc=exc,
+                )
+                registration_failure_detail = (
+                    f"{scheme}: route registration failed ({type(exc).__name__}: {exc})"
+                )
+                failure_details.append(registration_failure_detail)
+                plugin = None
+
+            if plugin is None:
                 invalid_urls += 1
                 logger.warning("Apprise rejected notification route URL for scheme '%s'", scheme)
+                _log_apprise_records(apprise_records)
+                warning_detail = _build_apprise_warning_detail(apprise_records, scheme=scheme)
+                if warning_detail:
+                    failure_details.append(warning_detail)
+                elif registration_failure_detail is None:
+                    failure_details.append(f"{scheme}: route URL rejected by Apprise")
+                continue
 
-        if valid_urls == 0:
-            _log_apprise_records(apprise_records)
-            scheme_summary = ", ".join(url_schemes) if url_schemes else "unknown"
-            logger.warning(
-                "No valid Apprise notification routes after registration for scheme(s): %s",
-                scheme_summary,
-            )
-            return {
-                "success": False,
-                "message": "No valid notification URLs configured",
-            }
+            plugin_label = _plugin_label(plugin, scheme)
+            apobj.add(plugin)
+            valid_urls += 1
 
-        try:
-            delivered = bool(apobj.notify(title=title, body=body, notify_type=notify_type))
-        except Exception as exc:
-            _log_apprise_records(apprise_records)
-            scheme_summary = ", ".join(url_schemes) if url_schemes else "unknown"
-            logger.warning(
-                "Apprise notify raised %s for scheme(s): %s",
-                type(exc).__name__,
-                scheme_summary,
-            )
-            return {"success": False, "message": f"Notification send failed: {type(exc).__name__}: {exc}"}
+            try:
+                delivered = bool(apobj.notify(title=title, body=body, notify_type=notify_type))
+            except Exception as exc:
+                _log_apprise_records(apprise_records)
+                failed_delivery_urls += 1
+                logger.warning(
+                    "Apprise notify raised %s for %s: %s",
+                    type(exc).__name__,
+                    plugin_label,
+                    exc,
+                )
+                _log_apprise_exception_debug(action="notify", scheme=scheme, exc=exc)
+                warning_detail = _build_apprise_warning_detail(apprise_records, scheme=scheme)
+                if warning_detail:
+                    failure_details.append(warning_detail)
+                else:
+                    failure_details.append(
+                        f"{scheme}: notify raised {type(exc).__name__}: {exc}"
+                    )
+                continue
 
-    if not delivered:
         _log_apprise_records(apprise_records)
-        scheme_summary = ", ".join(url_schemes) if url_schemes else "unknown"
+        if delivered:
+            delivered_urls += 1
+            logger.debug("Notification delivered via %s", plugin_label)
+            continue
+
+        failed_delivery_urls += 1
+        logger.warning("Apprise notify returned False for %s", plugin_label)
+        warning_detail = _build_apprise_warning_detail(apprise_records, scheme=scheme)
+        if warning_detail:
+            failure_details.append(warning_detail)
+        else:
+            failure_details.append(f"{scheme}: delivery failed")
+
+    scheme_summary = ", ".join(url_schemes) if url_schemes else "unknown"
+    if valid_urls == 0:
         logger.warning(
-            "Apprise notify returned False for scheme(s): %s (valid_urls=%s invalid_urls=%s)",
+            "No valid Apprise notification routes after registration for scheme(s): %s",
+            scheme_summary,
+        )
+        result: dict[str, Any] = {
+            "success": False,
+            "message": "No valid notification URLs configured",
+        }
+        if failure_details:
+            result["details"] = failure_details
+        return result
+
+    if delivered_urls == 0:
+        logger.warning(
+            (
+                "Apprise notify returned False for scheme(s): %s "
+                "(valid_urls=%s invalid_urls=%s failed_deliveries=%s)"
+            ),
             scheme_summary,
             valid_urls,
             invalid_urls,
+            failed_delivery_urls,
         )
-        return {"success": False, "message": "Notification delivery failed"}
+        result = {"success": False, "message": "Notification delivery failed"}
+        if failure_details:
+            result["details"] = failure_details
+        return result
 
-    _log_apprise_records(apprise_records)
-
-    message = f"Notification sent to {valid_urls} URL(s)"
-    if invalid_urls:
-        message += f" ({invalid_urls} invalid URL(s) skipped)"
-    return {"success": True, "message": message}
+    message = f"Notification sent to {delivered_urls} URL(s)"
+    failed_urls = invalid_urls + failed_delivery_urls
+    if failed_urls:
+        message += f" ({failed_urls} URL(s) failed)"
+    result = {"success": True, "message": message}
+    if failure_details:
+        result["details"] = failure_details
+    return result
 
 
 def _create_apprise_client() -> Any:

src/frontend/src/components/settings/fields/ActionButton.tsx

@@ -84,7 +84,14 @@ export const ActionButton = ({ field, onAction, disabled }: ActionButtonProps) =
               : 'bg-red-500/20 text-red-700 dark:text-red-300'
           }`}
         >
-          {result.message}
+          <p>{result.message}</p>
+          {Array.isArray(result.details) && result.details.length > 0 && (
+            <ul className="mt-2 list-disc pl-5 space-y-1 text-xs opacity-90">
+              {result.details.map((detail, index) => (
+                <li key={`${detail}-${index}`}>{detail}</li>
+              ))}
+            </ul>
+          )}
         </div>
       )}
 

src/frontend/src/services/api.ts

@@ -94,6 +94,31 @@ export const isApiResponseError = (error: unknown): error is ApiResponseError =>
   return error instanceof ApiResponseError;
 };
 
+const mapApiErrorToActionResult = (error: unknown): ActionResult | null => {
+  if (!isApiResponseError(error) || !error.payload) {
+    return null;
+  }
+
+  const payload = error.payload;
+  const message =
+    typeof payload.message === 'string'
+      ? payload.message
+      : (typeof payload.error === 'string' ? payload.error : null);
+  if (!message) {
+    return null;
+  }
+
+  const details = Array.isArray(payload.details)
+    ? payload.details.filter((detail): detail is string => typeof detail === 'string' && detail.trim().length > 0)
+    : undefined;
+
+  return {
+    success: false,
+    message,
+    ...(details && details.length > 0 ? { details } : {}),
+  };
+};
+
 // Default request timeout in milliseconds (30 seconds)
 const DEFAULT_TIMEOUT_MS = 30000;
 
@@ -470,10 +495,18 @@ export const executeSettingsAction = async (
   actionKey: string,
   currentValues?: Record<string, unknown>
 ): Promise<ActionResult> => {
-  return fetchJSON<ActionResult>(`${API.settings}/${tabName}/action/${actionKey}`, {
-    method: 'POST',
-    body: currentValues ? JSON.stringify(currentValues) : undefined,
-  });
+  try {
+    return await fetchJSON<ActionResult>(`${API.settings}/${tabName}/action/${actionKey}`, {
+      method: 'POST',
+      body: currentValues ? JSON.stringify(currentValues) : undefined,
+    });
+  } catch (error) {
+    const mapped = mapApiErrorToActionResult(error);
+    if (mapped) {
+      return mapped;
+    }
+    throw error;
+  }
 };
 
 // Onboarding API functions
@@ -709,25 +742,41 @@ export const testAdminUserNotificationPreferences = async (
   userId: number,
   routes: Array<Record<string, unknown>>
 ): Promise<import('../types/settings').ActionResult> => {
-  return fetchJSON<import('../types/settings').ActionResult>(
-    `${API_BASE}/admin/users/${userId}/notification-preferences/test`,
-    {
-      method: 'POST',
-      body: JSON.stringify({ USER_NOTIFICATION_ROUTES: routes }),
+  try {
+    return await fetchJSON<import('../types/settings').ActionResult>(
+      `${API_BASE}/admin/users/${userId}/notification-preferences/test`,
+      {
+        method: 'POST',
+        body: JSON.stringify({ USER_NOTIFICATION_ROUTES: routes }),
+      }
+    );
+  } catch (error) {
+    const mapped = mapApiErrorToActionResult(error);
+    if (mapped) {
+      return mapped;
     }
-  );
+    throw error;
+  }
 };
 
 export const testSelfNotificationPreferences = async (
   routes: Array<Record<string, unknown>>
 ): Promise<import('../types/settings').ActionResult> => {
-  return fetchJSON<import('../types/settings').ActionResult>(
-    `${API_BASE}/users/me/notification-preferences/test`,
-    {
-      method: 'POST',
-      body: JSON.stringify({ USER_NOTIFICATION_ROUTES: routes }),
+  try {
+    return await fetchJSON<import('../types/settings').ActionResult>(
+      `${API_BASE}/users/me/notification-preferences/test`,
+      {
+        method: 'POST',
+        body: JSON.stringify({ USER_NOTIFICATION_ROUTES: routes }),
+      }
+    );
+  } catch (error) {
+    const mapped = mapApiErrorToActionResult(error);
+    if (mapped) {
+      return mapped;
     }
-  );
+    throw error;
+  }
 };
 
 export interface SettingsOverrideUserDetail {

src/frontend/src/types/settings.ts

@@ -220,6 +220,7 @@ export interface SettingsResponse {
 export interface ActionResult {
   success: boolean;
   message: string;
+  details?: string[];
 }
 
 export interface UpdateResult {

tests/core/test_notifications.py

@@ -21,16 +21,35 @@ class _FakeNotifyType:
     FAILURE = "FAILURE"
 
 
+class _FakePlugin:
+    """Fake Apprise plugin returned by instantiate()."""
+
+    def __init__(self, raw_url: str):
+        self._url = raw_url
+        self.app_id = "FakePlugin"
+
+    def url(self, privacy=False):
+        return self._url
+
+
 class _FakeAppriseClient:
     def __init__(self):
         self.add_calls = []
+        self.instantiate_calls: list[dict[str, object | None]] = []
         self.notify_calls = []
         self.notify_result = True
+        self.notify_results_by_url: dict[str, bool] = {}
+        self.reject_urls: set[str] = set()
+        self.instantiate_exceptions_by_url: dict[str, Exception] = {}
+        self.notify_exceptions_by_url: dict[str, Exception] = {}
         self.notify_warning_messages: list[str] = []
         self.notify_info_messages: list[str] = []
+        self._active_url: str | None = None
 
-    def add(self, url):
+    def add(self, plugin):
+        url = getattr(plugin, "_url", str(plugin))
         self.add_calls.append(url)
+        self._active_url = url
         return True
 
     def notify(self, **kwargs):
@@ -39,9 +58,42 @@ class _FakeAppriseClient:
             logging.getLogger("apprise.plugins.pushover").info(message)
         for message in self.notify_warning_messages:
             logging.getLogger("apprise.plugins.pushover").warning(message)
+        if self._active_url and self._active_url in self.notify_exceptions_by_url:
+            raise self.notify_exceptions_by_url[self._active_url]
+        if self._active_url and self._active_url in self.notify_results_by_url:
+            return self.notify_results_by_url[self._active_url]
         return self.notify_result
 
 
+class _FakeAppriseClass:
+    """Fake for apprise.Apprise that acts as both constructor and has instantiate()."""
+
+    def __init__(self, module):
+        self._module = module
+
+    def __call__(self, *args, **kwargs):
+        self._module.apprise_kwargs = kwargs
+        asset = kwargs.get("asset")
+        self._module.asset_kwargs = getattr(asset, "kwargs", None)
+        self._module.client.asset = asset
+        return self._module.client
+
+    def instantiate(self, url, asset=None, tag=None, suppress_exceptions=True):
+        _ = (tag, suppress_exceptions)
+        client = self._module.client
+        client.instantiate_calls.append(
+            {
+                "url": url,
+                "asset_kwargs": getattr(asset, "kwargs", None),
+            }
+        )
+        if url in client.instantiate_exceptions_by_url:
+            raise client.instantiate_exceptions_by_url[url]
+        if url in client.reject_urls:
+            return None
+        return _FakePlugin(url)
+
+
 class _FakeAppriseModule:
     NotifyType = _FakeNotifyType
     asset_kwargs: dict[str, str] | None = None
@@ -49,17 +101,12 @@ class _FakeAppriseModule:
     def __init__(self):
         self.client = _FakeAppriseClient()
         self.apprise_kwargs = {}
+        self.Apprise = _FakeAppriseClass(self)
 
     class AppriseAsset:
         def __init__(self, **kwargs):
             self.kwargs = kwargs
 
-    def Apprise(self, *args, **kwargs):
-        self.apprise_kwargs = kwargs
-        asset = kwargs.get("asset")
-        self.asset_kwargs = getattr(asset, "kwargs", None)
-        return self.client
-
 
 def test_render_message_includes_admin_note_for_rejection():
     context = notifications_module.NotificationContext(
@@ -191,6 +238,26 @@ def test_dispatch_to_apprise_uses_shelfmark_asset_defaults(monkeypatch):
     assert "logo.png" in fake_apprise.asset_kwargs["image_url_logo"]
 
 
+def test_dispatch_to_apprise_passes_shelfmark_asset_to_instantiate(monkeypatch):
+    fake_apprise = _FakeAppriseModule()
+    monkeypatch.setattr(notifications_module, "apprise", fake_apprise)
+
+    result = notifications_module._dispatch_to_apprise(
+        ["ntfys://ntfy.sh/shelfmark"],
+        title="Test",
+        body="Body",
+        notify_type=_FakeNotifyType.INFO,
+    )
+
+    assert result["success"] is True
+    assert fake_apprise.client.instantiate_calls
+    instantiate_call = fake_apprise.client.instantiate_calls[0]
+    asset_kwargs = instantiate_call["asset_kwargs"]
+    assert isinstance(asset_kwargs, dict)
+    assert asset_kwargs["app_id"] == "Shelfmark"
+    assert "logo.png" in asset_kwargs["image_url_logo"]
+
+
 def test_dispatch_to_apprise_logs_captured_apprise_info_messages(monkeypatch):
     fake_apprise = _FakeAppriseModule()
     fake_apprise.client.notify_info_messages = [
@@ -243,9 +310,30 @@ def test_dispatch_to_apprise_notify_false_returns_generic_failure_and_logs(monke
 
     assert result["success"] is False
     assert result["message"] == "Notification delivery failed"
+    assert result["details"] == ["pover: delivery failed"]
     assert any("scheme(s): pover" in message for message in warning_messages)
 
 
+def test_dispatch_to_apprise_partial_success_returns_success(monkeypatch):
+    fake_apprise = _FakeAppriseModule()
+    fake_apprise.client.notify_results_by_url = {
+        "gotifys://gotify.example/token": False,
+        "ntfys://ntfy.sh/shelfmark": True,
+    }
+    monkeypatch.setattr(notifications_module, "apprise", fake_apprise)
+
+    result = notifications_module._dispatch_to_apprise(
+        ["gotifys://gotify.example/token", "ntfys://ntfy.sh/shelfmark"],
+        title="Test",
+        body="Body",
+        notify_type=_FakeNotifyType.INFO,
+    )
+
+    assert result["success"] is True
+    assert result["message"] == "Notification sent to 1 URL(s) (1 URL(s) failed)"
+    assert result["details"] == ["gotifys: delivery failed"]
+
+
 def test_dispatch_to_apprise_logs_captured_apprise_warning_messages(monkeypatch):
     fake_apprise = _FakeAppriseModule()
     fake_apprise.client.notify_result = False
@@ -270,6 +358,11 @@ def test_dispatch_to_apprise_logs_captured_apprise_warning_messages(monkeypatch)
     )
 
     assert result["success"] is False
+    assert any(
+        "pover: apprise.plugins.pushover: Failed to send Pushover notification"
+        in detail
+        for detail in result.get("details", [])
+    )
     assert any(
         "Apprise source [apprise.plugins.pushover]: Failed to send Pushover notification"
         in msg
@@ -277,6 +370,68 @@ def test_dispatch_to_apprise_logs_captured_apprise_warning_messages(monkeypatch)
     )
 
 
+def test_dispatch_to_apprise_logs_add_exception_at_debug_with_trace(monkeypatch):
+    fake_apprise = _FakeAppriseModule()
+    fake_apprise.client.instantiate_exceptions_by_url = {
+        "ntfys://ntfy.sh/shelfmark": RuntimeError("add exploded"),
+    }
+    monkeypatch.setattr(notifications_module, "apprise", fake_apprise)
+
+    debug_calls: list[tuple[str, tuple[object, ...], dict[str, object]]] = []
+
+    def _fake_debug(message, *args, **kwargs):
+        debug_calls.append((str(message), args, kwargs))
+
+    monkeypatch.setattr(notifications_module.logger, "debug", _fake_debug)
+
+    result = notifications_module._dispatch_to_apprise(
+        ["ntfys://ntfy.sh/shelfmark"],
+        title="Test",
+        body="Body",
+        notify_type=_FakeNotifyType.INFO,
+    )
+
+    assert result["success"] is False
+    assert result["message"] == "No valid notification URLs configured"
+    assert result["details"] == ["ntfys: route registration failed (RuntimeError: add exploded)"]
+    assert any(
+        "Apprise route registration raised RuntimeError" in (message % args if args else message)
+        and kwargs.get("exc_info") is True
+        for message, args, kwargs in debug_calls
+    )
+
+
+def test_dispatch_to_apprise_logs_notify_exception_at_debug_with_trace(monkeypatch):
+    fake_apprise = _FakeAppriseModule()
+    fake_apprise.client.notify_exceptions_by_url = {
+        "ntfys://ntfy.sh/shelfmark": RuntimeError("notify exploded"),
+    }
+    monkeypatch.setattr(notifications_module, "apprise", fake_apprise)
+
+    debug_calls: list[tuple[str, tuple[object, ...], dict[str, object]]] = []
+
+    def _fake_debug(message, *args, **kwargs):
+        debug_calls.append((str(message), args, kwargs))
+
+    monkeypatch.setattr(notifications_module.logger, "debug", _fake_debug)
+
+    result = notifications_module._dispatch_to_apprise(
+        ["ntfys://ntfy.sh/shelfmark"],
+        title="Test",
+        body="Body",
+        notify_type=_FakeNotifyType.INFO,
+    )
+
+    assert result["success"] is False
+    assert result["message"] == "Notification delivery failed"
+    assert result["details"] == ["ntfys: notify raised RuntimeError: notify exploded"]
+    assert any(
+        "Apprise notify raised RuntimeError" in (message % args if args else message)
+        and kwargs.get("exc_info") is True
+        for message, args, kwargs in debug_calls
+    )
+
+
 def test_resolve_admin_routes_returns_empty_when_no_routes(monkeypatch):
     def _fake_get(key, default=None):
         if key == "ADMIN_NOTIFICATION_ROUTES":