[fix] Ignore "connection not working" notifications #314

Added functionality to ignore "connection not working"
notifications generated due to connectivity issues.

Fixes #314

Author: pandafy

README.rst

@@ -2526,6 +2526,7 @@ it is not sent if the response was not successful.
 - ``old_is_working``: previous value of ``DeviceConnection.is_working``,
   either ``None`` (for new connections), ``True`` or ``False``
 - ``failure_reason``: error message explaining reason for failure in establishing connection
+- ``old_failure_reason``: previous value of ``DeviceConnection.failure_reason``
 
 This signal is emitted every time ``DeviceConnection.is_working`` changes.
 

openwisp_controller/connection/apps.py

@@ -21,6 +21,11 @@ class ConnectionConfig(AppConfig):
     name = 'openwisp_controller.connection'
     label = 'connection'
     verbose_name = _('Network Device Credentials')
+    # List of reasons for which notifications should
+    # not be generated if a device connection errors out.
+    # Intended to be used internally by OpenWISP to
+    # ignore notifications generated due to connectivity issues.
+    _ignore_connection_notification_reasons = []
 
     def ready(self):
         """
@@ -106,12 +111,22 @@ def _is_update_in_progress(cls, device_pk):
 
     @classmethod
     def is_working_changed_receiver(
-        cls, instance, is_working, old_is_working, **kwargs
+        cls,
+        instance,
+        is_working,
+        old_is_working,
+        failure_reason,
+        old_failure_reason,
+        **kwargs,
     ):
         # if old_is_working is None, it's a new device connection which wasn't
         # used yet, so nothing is really changing and we won't notify the user
         if old_is_working is None:
             return
+        # don't send notification if error occurred due to connectivity issues
+        for ignore_reason in cls._ignore_connection_notification_reasons:
+            if ignore_reason in failure_reason or ignore_reason in old_failure_reason:
+                return
         device = instance.device
         notification_opts = dict(sender=instance, target=device)
         if not is_working:

openwisp_controller/connection/base/models.py

@@ -248,6 +248,7 @@ class Meta:
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
         self._initial_is_working = self.is_working
+        self._initial_failure_reason = self.failure_reason
 
     def clean(self):
         cred_org = self.credentials.organization
@@ -340,13 +341,15 @@ def save(self, *args, **kwargs):
         if self.is_working != self._initial_is_working:
             self.send_is_working_changed_signal()
         self._initial_is_working = self.is_working
+        self._initial_failure_reason = self.failure_reason
 
     def send_is_working_changed_signal(self):
         is_working_changed.send(
             sender=self.__class__,
             is_working=self.is_working,
             old_is_working=self._initial_is_working,
             failure_reason=self.failure_reason,
+            old_failure_reason=self._initial_failure_reason,
             instance=self,
         )
 

openwisp_controller/connection/signals.py

@@ -2,5 +2,11 @@
 
 is_working_changed = Signal()
 is_working_changed.__doc__ = """
-Providing araguments: ['is_working', 'old_is_working', 'instance', 'failure_reason']
+Providing araguments: [
+    'is_working',
+    'old_is_working',
+    'instance',
+    'failure_reason',
+    'old_failure_reason'
+]
 """

openwisp_controller/connection/tests/test_models.py

@@ -334,6 +334,7 @@ def test_is_working_change_signal_emitted(self):
             dc.save()
         handler.assert_called_once_with(
             failure_reason='',
+            old_failure_reason='',
             instance=dc,
             is_working=True,
             old_is_working=None,

openwisp_controller/connection/tests/test_notifications.py

@@ -1,8 +1,10 @@
 import os
+from unittest.mock import patch
 
 from django.apps.registry import apps
 from django.test import TestCase, TransactionTestCase
 from django.urls import reverse
+from openwisp_notifications.signals import notify
 from openwisp_notifications.types import unregister_notification_type
 from swapper import load_model
 
@@ -142,6 +144,57 @@ def test_default_notification_type_already_unregistered(self):
         app = apps.get_app_config(self.app_label)
         app.register_notification_types()
 
+    @patch(
+        'openwisp_controller.connection.apps.ConnectionConfig'
+        '._ignore_connection_notification_reasons',
+        ['timed out'],
+    )
+    @patch.object(notify, 'send')
+    def test_connection_is_working_changed_timed_out(self, notify_send, *args):
+        credentials = self._create_credentials_with_key(port=self.ssh_server.port)
+        self._create_config(device=self.d)
+        device_conn = self._create_device_connection(
+            credentials=credentials, device=self.d, is_working=True
+        )
+        self.assertEqual(device_conn.is_working, True)
+        device_conn.is_working = False
+        device_conn.failure_reason = 'timed out'
+        device_conn.full_clean()
+        device_conn.save()
+        notify_send.assert_not_called()
+        # Connection recovers, device is reachable again
+        device_conn.is_working = True
+        device_conn.failure_reason = ''
+        device_conn.full_clean()
+        device_conn.save()
+        notify_send.assert_not_called()
+
+    @patch(
+        'openwisp_controller.connection.apps.ConnectionConfig'
+        '._ignore_connection_notification_reasons',
+        ['Unable to connect'],
+    )
+    @patch.object(notify, 'send')
+    def test_connection_is_working_changed_unable_to_connect(self, notify_send, *args):
+        credentials = self._create_credentials_with_key(port=self.ssh_server.port)
+        self._create_config(device=self.d)
+        device_conn = self._create_device_connection(
+            credentials=credentials, device=self.d, is_working=True
+        )
+        device_conn.failure_reason = (
+            '[Errno None] Unable to connect to port 5555 on 127.0.0.1'
+        )
+        device_conn.is_working = False
+        device_conn.full_clean()
+        device_conn.save()
+        notify_send.assert_not_called()
+        # Connection makes recovery.
+        device_conn.failure_reason = ''
+        device_conn.is_working = True
+        device_conn.full_clean()
+        device_conn.save()
+        notify_send.assert_not_called()
+
 
 class TestNotificationTransaction(
     CreateConnectionsMixin, BaseTestNotification, TransactionTestCase