fix: Address code review feedback on notification error handling.

Co-authored-by: elisiariocouto <818914+elisiariocouto@users.noreply.github.com>

leggen/notifications/discord.py

@@ -61,17 +61,13 @@ def send_sync_failure_notification(ctx: click.Context, notification: dict):
     info("Sending sync failure notification to Discord")
     webhook = DiscordWebhook(url=ctx.obj["notifications"]["discord"]["webhook"])
 
-    # Determine color and title based on failure type
-    if notification.get("type") == "sync_final_failure":
-        color = "ff0000"  # Red for final failure
-        title = "🚨 Sync Final Failure"
-        description = (
-            f"Sync failed permanently after {notification['retry_count']} attempts"
-        )
-    else:
-        color = "ffaa00"  # Orange for retry
-        title = "⚠️ Sync Failure"
-        description = f"Sync failed (attempt {notification['retry_count']}/{notification['max_retries']}). Will retry automatically..."
+    color = "ffaa00"  # Orange for sync failure
+    title = "⚠️ Sync Failure"
+
+    # Build description with account info if available
+    description = "Account sync failed"
+    if notification.get("account_id"):
+        description = f"Account {notification['account_id']} sync failed"
 
     embed = DiscordEmbed(
         title=title,

leggen/notifications/telegram.py

@@ -87,19 +87,14 @@ def send_sync_failure_notification(ctx: click.Context, notification: dict):
     bot_url = f"https://api.telegram.org/bot{token}/sendMessage"
     info("Sending sync failure notification to Telegram")
 
-    message = "*🚨 [Leggen](https://github.com/elisiariocouto/leggen)*\n"
+    message = "*⚠️ [Leggen](https://github.com/elisiariocouto/leggen)*\n"
     message += "*Sync Failed*\n\n"
-    message += escape_markdown(f"Error: {notification['error']}\n")
 
-    if notification.get("type") == "sync_final_failure":
-        message += escape_markdown(
-            f"❌ Final failure after {notification['retry_count']} attempts\n"
-        )
-    else:
-        message += escape_markdown(
-            f"🔄 Attempt {notification['retry_count']}/{notification['max_retries']}\n"
-        )
-        message += escape_markdown("Will retry automatically...\n")
+    # Add account info if available
+    if notification.get("account_id"):
+        message += escape_markdown(f"Account: {notification['account_id']}\n")
+
+    message += escape_markdown(f"Error: {notification['error']}\n")
 
     res = requests.post(
         bot_url,

leggen/services/notification_service.py

@@ -52,11 +52,17 @@ class NotificationService:
 
     async def send_expiry_notification(self, notification_data: Dict[str, Any]) -> None:
         """Send notification about account expiry"""
-        if self._is_discord_enabled():
-            await self._send_discord_expiry(notification_data)
+        try:
+            if self._is_discord_enabled():
+                await self._send_discord_expiry(notification_data)
+        except Exception as e:
+            logger.error(f"Failed to send Discord expiry notification: {e}")
 
-        if self._is_telegram_enabled():
-            await self._send_telegram_expiry(notification_data)
+        try:
+            if self._is_telegram_enabled():
+                await self._send_telegram_expiry(notification_data)
+        except Exception as e:
+            logger.error(f"Failed to send Telegram expiry notification: {e}")
 
     def _filter_transactions(
         self, transactions: List[Dict[str, Any]]
@@ -262,7 +268,6 @@ class NotificationService:
             logger.info(f"Sent Discord expiry notification: {notification_data}")
         except Exception as e:
             logger.error(f"Failed to send Discord expiry notification: {e}")
-            raise
 
     async def _send_telegram_expiry(self, notification_data: Dict[str, Any]) -> None:
         """Send Telegram expiry notification"""
@@ -288,17 +293,22 @@ class NotificationService:
             logger.info(f"Sent Telegram expiry notification: {notification_data}")
         except Exception as e:
             logger.error(f"Failed to send Telegram expiry notification: {e}")
-            raise
 
     async def send_sync_failure_notification(
         self, notification_data: Dict[str, Any]
     ) -> None:
         """Send notification about sync failure"""
-        if self._is_discord_enabled():
-            await self._send_discord_sync_failure(notification_data)
+        try:
+            if self._is_discord_enabled():
+                await self._send_discord_sync_failure(notification_data)
+        except Exception as e:
+            logger.error(f"Failed to send Discord sync failure notification: {e}")
 
-        if self._is_telegram_enabled():
-            await self._send_telegram_sync_failure(notification_data)
+        try:
+            if self._is_telegram_enabled():
+                await self._send_telegram_sync_failure(notification_data)
+        except Exception as e:
+            logger.error(f"Failed to send Telegram sync failure notification: {e}")
 
     async def _send_discord_sync_failure(
         self, notification_data: Dict[str, Any]
@@ -326,7 +336,6 @@ class NotificationService:
             logger.info(f"Sent Discord sync failure notification: {notification_data}")
         except Exception as e:
             logger.error(f"Failed to send Discord sync failure notification: {e}")
-            raise
 
     async def _send_telegram_sync_failure(
         self, notification_data: Dict[str, Any]
@@ -354,4 +363,3 @@ class NotificationService:
             logger.info(f"Sent Telegram sync failure notification: {notification_data}")
         except Exception as e:
             logger.error(f"Failed to send Telegram sync failure notification: {e}")
-            raise

leggen/services/sync_service.py

@@ -8,6 +8,9 @@ from leggen.services.database_service import DatabaseService
 from leggen.services.gocardless_service import GoCardlessService
 from leggen.services.notification_service import NotificationService
 
+# Constants for notification
+EXPIRED_DAYS_LEFT = 0
+
 
 class SyncService:
     def __init__(self):
@@ -67,6 +70,9 @@ class SyncService:
             self._sync_status.total_accounts = len(all_accounts)
             logs.append(f"Found {len(all_accounts)} accounts to sync")
 
+            # Check for expired or expiring requisitions
+            await self._check_requisition_expiry(requisitions.get("results", []))
+
             # Process each account
             for account_id in all_accounts:
                 try:
@@ -166,6 +172,15 @@ class SyncService:
                     logger.error(error_msg)
                     logs.append(error_msg)
 
+                    # Send notification for account sync failure
+                    await self.notifications.send_sync_failure_notification(
+                        {
+                            "account_id": account_id,
+                            "error": error_msg,
+                            "type": "account_sync_failure",
+                        }
+                    )
+
             end_time = datetime.now()
             duration = (end_time - start_time).total_seconds()
 
@@ -252,6 +267,31 @@ class SyncService:
         finally:
             self._sync_status.is_running = False
 
+    async def _check_requisition_expiry(self, requisitions: List[dict]) -> None:
+        """Check requisitions for expiry and send notifications.
+
+        Args:
+            requisitions: List of requisition dictionaries to check
+        """
+        for req in requisitions:
+            requisition_id = req.get("id", "unknown")
+            institution_id = req.get("institution_id", "unknown")
+            status = req.get("status", "")
+
+            # Check if requisition is expired
+            if status == "EX":
+                logger.warning(
+                    f"Requisition {requisition_id} for {institution_id} has expired"
+                )
+                await self.notifications.send_expiry_notification(
+                    {
+                        "bank": institution_id,
+                        "requisition_id": requisition_id,
+                        "status": "expired",
+                        "days_left": EXPIRED_DAYS_LEFT,
+                    }
+                )
+
     async def sync_specific_accounts(
         self, account_ids: List[str], force: bool = False, trigger_type: str = "manual"
     ) -> SyncResult:

tests/unit/test_sync_notifications.py

@@ -0,0 +1,254 @@
+"""Tests for sync service notification functionality."""
+
+from unittest.mock import patch
+
+import pytest
+
+from leggen.services.sync_service import SyncService
+
+
+@pytest.mark.unit
+class TestSyncNotifications:
+    """Test sync service notification functionality."""
+
+    @pytest.mark.asyncio
+    async def test_sync_failure_sends_notification(self):
+        """Test that sync failures trigger notifications."""
+        sync_service = SyncService()
+
+        # Mock the dependencies
+        with (
+            patch.object(
+                sync_service.gocardless, "get_requisitions"
+            ) as mock_get_requisitions,
+            patch.object(
+                sync_service.gocardless, "get_account_details"
+            ) as mock_get_details,
+            patch.object(
+                sync_service.notifications, "send_sync_failure_notification"
+            ) as mock_send_notification,
+            patch.object(
+                sync_service.database, "persist_sync_operation", return_value=1
+            ),
+        ):
+            # Setup: One requisition with one account that will fail
+            mock_get_requisitions.return_value = {
+                "results": [
+                    {
+                        "id": "req-123",
+                        "institution_id": "TEST_BANK",
+                        "status": "LN",
+                        "accounts": ["account-1"],
+                    }
+                ]
+            }
+
+            # Make account details fail
+            mock_get_details.side_effect = Exception("API Error")
+
+            # Execute: Run sync which should fail for the account
+            await sync_service.sync_all_accounts()
+
+            # Assert: Notification should be sent for the failed account
+            mock_send_notification.assert_called_once()
+            call_args = mock_send_notification.call_args[0][0]
+            assert call_args["account_id"] == "account-1"
+            assert "API Error" in call_args["error"]
+            assert call_args["type"] == "account_sync_failure"
+
+    @pytest.mark.asyncio
+    async def test_expired_requisition_sends_notification(self):
+        """Test that expired requisitions trigger expiry notifications."""
+        sync_service = SyncService()
+
+        # Mock the dependencies
+        with (
+            patch.object(
+                sync_service.gocardless, "get_requisitions"
+            ) as mock_get_requisitions,
+            patch.object(
+                sync_service.notifications, "send_expiry_notification"
+            ) as mock_send_expiry,
+            patch.object(
+                sync_service.database, "persist_sync_operation", return_value=1
+            ),
+        ):
+            # Setup: One expired requisition
+            mock_get_requisitions.return_value = {
+                "results": [
+                    {
+                        "id": "req-expired",
+                        "institution_id": "EXPIRED_BANK",
+                        "status": "EX",
+                        "accounts": [],
+                    }
+                ]
+            }
+
+            # Execute: Run sync
+            await sync_service.sync_all_accounts()
+
+            # Assert: Expiry notification should be sent
+            mock_send_expiry.assert_called_once()
+            call_args = mock_send_expiry.call_args[0][0]
+            assert call_args["requisition_id"] == "req-expired"
+            assert call_args["bank"] == "EXPIRED_BANK"
+            assert call_args["status"] == "expired"
+            assert call_args["days_left"] == 0
+
+    @pytest.mark.asyncio
+    async def test_multiple_failures_send_multiple_notifications(self):
+        """Test that multiple account failures send multiple notifications."""
+        sync_service = SyncService()
+
+        # Mock the dependencies
+        with (
+            patch.object(
+                sync_service.gocardless, "get_requisitions"
+            ) as mock_get_requisitions,
+            patch.object(
+                sync_service.gocardless, "get_account_details"
+            ) as mock_get_details,
+            patch.object(
+                sync_service.notifications, "send_sync_failure_notification"
+            ) as mock_send_notification,
+            patch.object(
+                sync_service.database, "persist_sync_operation", return_value=1
+            ),
+        ):
+            # Setup: One requisition with two accounts that will fail
+            mock_get_requisitions.return_value = {
+                "results": [
+                    {
+                        "id": "req-123",
+                        "institution_id": "TEST_BANK",
+                        "status": "LN",
+                        "accounts": ["account-1", "account-2"],
+                    }
+                ]
+            }
+
+            # Make all account details fail
+            mock_get_details.side_effect = Exception("API Error")
+
+            # Execute: Run sync
+            await sync_service.sync_all_accounts()
+
+            # Assert: Two notifications should be sent
+            assert mock_send_notification.call_count == 2
+
+    @pytest.mark.asyncio
+    async def test_successful_sync_no_failure_notification(self):
+        """Test that successful syncs don't send failure notifications."""
+        sync_service = SyncService()
+
+        # Mock the dependencies
+        with (
+            patch.object(
+                sync_service.gocardless, "get_requisitions"
+            ) as mock_get_requisitions,
+            patch.object(
+                sync_service.gocardless, "get_account_details"
+            ) as mock_get_details,
+            patch.object(
+                sync_service.gocardless, "get_account_balances"
+            ) as mock_get_balances,
+            patch.object(
+                sync_service.gocardless, "get_account_transactions"
+            ) as mock_get_transactions,
+            patch.object(
+                sync_service.notifications, "send_sync_failure_notification"
+            ) as mock_send_notification,
+            patch.object(sync_service.notifications, "send_transaction_notifications"),
+            patch.object(sync_service.database, "persist_account_details"),
+            patch.object(sync_service.database, "persist_balance"),
+            patch.object(
+                sync_service.database, "process_transactions", return_value=[]
+            ),
+            patch.object(
+                sync_service.database, "persist_transactions", return_value=[]
+            ),
+            patch.object(
+                sync_service.database, "persist_sync_operation", return_value=1
+            ),
+        ):
+            # Setup: One requisition with one account that succeeds
+            mock_get_requisitions.return_value = {
+                "results": [
+                    {
+                        "id": "req-123",
+                        "institution_id": "TEST_BANK",
+                        "status": "LN",
+                        "accounts": ["account-1"],
+                    }
+                ]
+            }
+
+            mock_get_details.return_value = {
+                "id": "account-1",
+                "institution_id": "TEST_BANK",
+                "status": "READY",
+                "iban": "TEST123",
+            }
+
+            mock_get_balances.return_value = {
+                "balances": [{"balanceAmount": {"amount": "100", "currency": "EUR"}}]
+            }
+
+            mock_get_transactions.return_value = {"transactions": {"booked": []}}
+
+            # Execute: Run sync
+            await sync_service.sync_all_accounts()
+
+            # Assert: No failure notification should be sent
+            mock_send_notification.assert_not_called()
+
+    @pytest.mark.asyncio
+    async def test_notification_failure_does_not_stop_sync(self):
+        """Test that notification failures don't stop the sync process."""
+        sync_service = SyncService()
+
+        # Mock the dependencies
+        with (
+            patch.object(
+                sync_service.gocardless, "get_requisitions"
+            ) as mock_get_requisitions,
+            patch.object(
+                sync_service.gocardless, "get_account_details"
+            ) as mock_get_details,
+            patch.object(
+                sync_service.notifications, "_send_discord_sync_failure"
+            ) as mock_discord_notification,
+            patch.object(
+                sync_service.notifications, "_send_telegram_sync_failure"
+            ) as mock_telegram_notification,
+            patch.object(
+                sync_service.database, "persist_sync_operation", return_value=1
+            ),
+        ):
+            # Setup: One requisition with one account that will fail
+            mock_get_requisitions.return_value = {
+                "results": [
+                    {
+                        "id": "req-123",
+                        "institution_id": "TEST_BANK",
+                        "status": "LN",
+                        "accounts": ["account-1"],
+                    }
+                ]
+            }
+
+            # Make account details fail
+            mock_get_details.side_effect = Exception("API Error")
+
+            # Make both notification methods fail
+            mock_discord_notification.side_effect = Exception("Discord Error")
+            mock_telegram_notification.side_effect = Exception("Telegram Error")
+
+            # Execute: Run sync - should not raise exception from notification
+            result = await sync_service.sync_all_accounts()
+
+            # The sync should complete with errors but not crash from notifications
+            assert result.success is False
+            assert len(result.errors) > 0
+            assert "API Error" in result.errors[0]