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

@@ -10,8 +10,6 @@ from leggen.services.notification_service import NotificationService
 
 # Constants for notification
 EXPIRED_DAYS_LEFT = 0
-ACCOUNT_SYNC_RETRY_COUNT = 1
-ACCOUNT_SYNC_MAX_RETRIES = 1
 
 
 class SyncService:
@@ -175,20 +173,13 @@ class SyncService:
                     logs.append(error_msg)
 
                     # Send notification for account sync failure
-                    try:
-                        await self.notifications.send_sync_failure_notification(
-                            {
-                                "account_id": account_id,
-                                "error": error_msg,
-                                "type": "account_sync_failure",
-                                "retry_count": ACCOUNT_SYNC_RETRY_COUNT,
-                                "max_retries": ACCOUNT_SYNC_MAX_RETRIES,
-                            }
-                        )
-                    except Exception as notif_error:
-                        logger.error(
-                            f"Failed to send sync failure notification: {notif_error}"
-                        )
+                    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()
@@ -277,7 +268,11 @@ class SyncService:
             self._sync_status.is_running = False
 
     async def _check_requisition_expiry(self, requisitions: List[dict]) -> None:
-        """Check requisitions for expiry and send notifications"""
+        """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")
@@ -288,17 +283,14 @@ class SyncService:
                 logger.warning(
                     f"Requisition {requisition_id} for {institution_id} has expired"
                 )
-                try:
-                    await self.notifications.send_expiry_notification(
-                        {
-                            "bank": institution_id,
-                            "requisition_id": requisition_id,
-                            "status": "expired",
-                            "days_left": EXPIRED_DAYS_LEFT,
-                        }
-                    )
-                except Exception as e:
-                    logger.error(f"Failed to send expiry notification: {e}")
+                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"

tests/unit/test_sync_notifications.py

@@ -217,8 +217,11 @@ class TestSyncNotifications:
                 sync_service.gocardless, "get_account_details"
             ) as mock_get_details,
             patch.object(
-                sync_service.notifications, "send_sync_failure_notification"
-            ) as mock_send_notification,
+                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
             ),
@@ -238,15 +241,14 @@ class TestSyncNotifications:
             # Make account details fail
             mock_get_details.side_effect = Exception("API Error")
 
-            # Make notification sending fail
-            mock_send_notification.side_effect = Exception("Notification 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
-            try:
-                result = await sync_service.sync_all_accounts()
-                # The sync should complete with errors but not crash
-                assert result.success is False
-                assert len(result.errors) > 0
-            except Exception as e:
-                # If exception is raised, it should not be the notification error
-                assert "Notification Error" not in str(e)
+            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]