Re #2014 - When any of a list of notifications fails, the others should still work

changedetectionio/notification.py

@@ -1,4 +1,5 @@
 import apprise
+import time
 from jinja2 import Environment, BaseLoader
 from apprise import NotifyFormat
 import json
@@ -131,20 +132,23 @@ def process_notification(n_object, datastore):
         # Initially text or whatever
         n_format = datastore.data['settings']['application'].get('notification_format', valid_notification_formats[default_notification_format])
 
-
     # https://github.com/caronc/apprise/wiki/Development_LogCapture
     # Anything higher than or equal to WARNING (which covers things like Connection errors)
     # raise it as an exception
-    apobjs=[]
-    sent_objs=[]
+
+    sent_objs = []
     from .apprise_asset import asset
-    for url in n_object['notification_urls']:
-        url = jinja2_env.from_string(url).render(**notification_parameters)
     apobj = apprise.Apprise(debug=True, asset=asset)
-        url = url.strip()
-        if len(url):
-            print(">> Process Notification: AppRise notifying {}".format(url))
+
+    if not n_object.get('notification_urls'):
+        return None
+
     with apprise.LogCapture(level=apprise.logging.DEBUG) as logs:
+        for url in n_object['notification_urls']:
+            url = url.strip()
+            print(">> Process Notification: AppRise notifying {}".format(url))
+            url = jinja2_env.from_string(url).render(**notification_parameters)
+
             # Re 323 - Limit discord length to their 2000 char limit total or it wont send.
             # Because different notifications may require different pre-processing, run each sequentially :(
             # 2000 bytes minus -
@@ -173,7 +177,8 @@ def process_notification(n_object, datastore):
                 n_title = n_title[0:payload_max_size]
                 n_body = n_body[0:body_limit]
 
-                elif url.startswith('discord://') or url.startswith('https://discordapp.com/api/webhooks') or url.startswith('https://discord.com/api'):
+            elif url.startswith('discord://') or url.startswith('https://discordapp.com/api/webhooks') or url.startswith(
+                    'https://discord.com/api'):
                 # real limit is 2000, but minus some for extra metadata
                 payload_max_size = 1700
                 body_limit = max(0, payload_max_size - len(n_title))
@@ -193,6 +198,12 @@ def process_notification(n_object, datastore):
 
             apobj.add(url)
 
+            sent_objs.append({'title': n_title,
+                              'body': n_body,
+                              'url': url,
+                              'body_format': n_format})
+
+        # Blast off the notifications tht are set in .add()
         apobj.notify(
             title=n_title,
             body=n_body,
@@ -201,21 +212,15 @@ def process_notification(n_object, datastore):
             attach=n_object.get('screenshot', None)
         )
 
-                apobj.clear()
-
-                # Incase it needs to exist in memory for a while after to process(?)
-                apobjs.append(apobj)
+        # Give apprise time to register an error
+        time.sleep(3)
 
         # Returns empty string if nothing found, multi-line string otherwise
         log_value = logs.getvalue()
+
         if log_value and 'WARNING' in log_value or 'ERROR' in log_value:
             raise Exception(log_value)
 
-                sent_objs.append({'title': n_title,
-                                  'body': n_body,
-                                  'url' : url,
-                                  'body_format': n_format})
-
     # Return what was sent for better logging - after the for loop
     return sent_objs
 

changedetectionio/tests/test_notification_errors.py

@@ -1,8 +1,7 @@
 import os
 import time
-import re
 from flask import url_for
-from . util import set_original_response, set_modified_response, live_server_setup
+from .util import set_original_response, set_modified_response, live_server_setup, wait_for_all_checks
 import logging
 
 def test_check_notification_error_handling(client, live_server):
@@ -11,7 +10,7 @@ def test_check_notification_error_handling(client, live_server):
     set_original_response()
 
     # Give the endpoint time to spin up
-    time.sleep(2)
+    time.sleep(1)
 
     # Set a URL and fetch it, then set a notification URL which is going to give errors
     test_url = url_for('test_endpoint', _external=True)
@@ -22,12 +21,16 @@ def test_check_notification_error_handling(client, live_server):
     )
     assert b"Watch added" in res.data
 
-    time.sleep(2)
+    wait_for_all_checks(client)
     set_modified_response()
 
+    working_notification_url = url_for('test_notification_endpoint', _external=True).replace('http', 'json')
+    broken_notification_url = "jsons://broken-url-xxxxxxxx123/test"
+
     res = client.post(
         url_for("edit_page", uuid="first"),
-        data={"notification_urls": "jsons://broken-url-xxxxxxxx123/test",
+        # A URL with errors should not block the one that is working
+        data={"notification_urls": f"{broken_notification_url}\r\n{working_notification_url}",
               "notification_title": "xxx",
               "notification_body": "xxxxx",
               "notification_format": "Text",
@@ -63,4 +66,10 @@ def test_check_notification_error_handling(client, live_server):
     found_name_resolution_error = b"Temporary failure in name resolution" in res.data or b"Name or service not known" in res.data
     assert found_name_resolution_error
 
+    # And the working one, which is after the 'broken' one should still have fired
+    with open("test-datastore/notification.txt", "r") as f:
+        notification_submission = f.read()
+    os.unlink("test-datastore/notification.txt")
+    assert 'xxxxx' in notification_submission
+
     client.get(url_for("form_delete", uuid="all"), follow_redirects=True)