From ea0d06ae08208bc858da54b05c14368e8c39f4fc Mon Sep 17 00:00:00 2001 From: Alex <25013571+alexhb1@users.noreply.github.com> Date: Sat, 28 Feb 2026 10:16:01 +0000 Subject: [PATCH] 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 | 206 ++++++++++++++---- .../settings/fields/ActionButton.tsx | 9 +- src/frontend/src/services/api.ts | 81 +++++-- src/frontend/src/types/settings.ts | 1 + tests/core/test_notifications.py | 169 +++++++++++++- 5 files changed, 397 insertions(+), 69 deletions(-) diff --git a/shelfmark/core/notifications.py b/shelfmark/core/notifications.py index f822a4e2..ecc478c8 100644 --- a/shelfmark/core/notifications.py +++ b/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: diff --git a/src/frontend/src/components/settings/fields/ActionButton.tsx b/src/frontend/src/components/settings/fields/ActionButton.tsx index b06b04ca..13b1907b 100644 --- a/src/frontend/src/components/settings/fields/ActionButton.tsx +++ b/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} +

{result.message}

+ {Array.isArray(result.details) && result.details.length > 0 && ( + + )} )} diff --git a/src/frontend/src/services/api.ts b/src/frontend/src/services/api.ts index b69c7a02..633d8576 100644 --- a/src/frontend/src/services/api.ts +++ b/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 ): Promise => { - return fetchJSON(`${API.settings}/${tabName}/action/${actionKey}`, { - method: 'POST', - body: currentValues ? JSON.stringify(currentValues) : undefined, - }); + try { + return await fetchJSON(`${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> ): Promise => { - return fetchJSON( - `${API_BASE}/admin/users/${userId}/notification-preferences/test`, - { - method: 'POST', - body: JSON.stringify({ USER_NOTIFICATION_ROUTES: routes }), + try { + return await fetchJSON( + `${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> ): Promise => { - return fetchJSON( - `${API_BASE}/users/me/notification-preferences/test`, - { - method: 'POST', - body: JSON.stringify({ USER_NOTIFICATION_ROUTES: routes }), + try { + return await fetchJSON( + `${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 { diff --git a/src/frontend/src/types/settings.ts b/src/frontend/src/types/settings.ts index b4f4a257..8bf17291 100644 --- a/src/frontend/src/types/settings.ts +++ b/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 { diff --git a/tests/core/test_notifications.py b/tests/core/test_notifications.py index e674b26b..31f626c6 100644 --- a/tests/core/test_notifications.py +++ b/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":