[req-change] Changed evaluation criteria for WifiClients checks

- The check counts the number of connected clients for the period
  defined in OPENWISP_MONITORING_WIFI_CLIENTS_MIN_CHECK_INTERVAL
  and OPENWISP_MONITORING_WIFI_CLIENTS_MAX_CHECK_INTERVAL
- Don't send alerts for devices that have "critical" health status

docs/user/settings.rst

@@ -352,19 +352,37 @@ E.g.:
         - If you omit the time, the exclusion will apply to the **entire
           day**.
 
-.. _openwisp_monitoring_wifi_clients_check_interval:
+.. _openwisp_monitoring_wifi_clients_max_check_interval:
 
-``OPENWISP_MONITORING_WIFI_CLIENTS_CHECK_INTERVAL``
----------------------------------------------------
+``OPENWISP_MONITORING_WIFI_CLIENTS_MAX_CHECK_INTERVAL``
+-------------------------------------------------------
 
-============ =======
+============ ==================
 **type**:    ``int``
-**default**: ``5``
-============ =======
+**default**: ``5`` (in minutes)
+============ ==================
+
+Time period in minutes used by :ref:`WiFi Clients checks
+<wifi_clients_check>` to monitor the maximum number of connected clients.
+
+This determines how far back the check looks when evaluating the maximum
+client threshold.
+
+.. _openwisp_monitoring_wifi_clients_min_check_interval:
+
+``OPENWISP_MONITORING_WIFI_CLIENTS_MIN_CHECK_INTERVAL``
+-------------------------------------------------------
+
+============ ============================
+**type**:    ``int``
+**default**: ``4320`` (3 days in minutes)
+============ ============================
+
+Time period in minutes used by :ref:`WiFi Clients checks
+<wifi_clients_check>` to monitor the minimum number of connected clients.
 
-This setting allows you to configure the WiFi Clients check interval used
-by :ref:`WiFi Clients checks <wifi_clients_check>`. By default it is set
-to 5 minutes.
+This determines how far back the check looks when evaluating the minimum
+clients threshold.
 
 .. _openwisp_monitoring_auto_charts:
 

openwisp_monitoring/check/classes/wifi_clients.py

@@ -78,7 +78,7 @@ def may_execute(cls):
                     return False
         return True
 
-    def check(self, store=True):
+    def _get_wifi_clients_count(self, time_interval):
         values = timeseries_db.read(
             key='wifi_clients',
             fields=['clients'],
@@ -88,24 +88,44 @@ def check(self, store=True):
                 'content_type': self.related_object._meta.label_lower,
                 'object_id': str(self.related_object.pk),
             },
-            since=(
-                timezone.localtime()
-                - timezone.timedelta(minutes=app_settings.WIFI_CLIENTS_CHECK_INTERVAL)
-            ),
+            since=(timezone.localtime() - timezone.timedelta(minutes=time_interval)),
         )
         if not values:
             result = 0
         else:
             result = values[0]['count']
+        return result
+
+    def _check_wifi_clients(self, check_type, interval, store=True, send_alert=True):
+        result = self._get_wifi_clients_count(interval)
         if store:
-            self.store_result(result)
+            metric = self._get_metric(f'wifi_clients_{check_type}')
+            metric.write(result, send_alert=send_alert)
         return result
 
-    def store_result(self, result):
-        max_metric = self._get_metric('wifi_clients_max')
-        max_metric.write(result)
-        min_metric = self._get_metric('wifi_clients_min')
-        min_metric.write(result)
+    def _check_wifi_clients_min(self, store=True, send_alert=True):
+        return self._check_wifi_clients(
+            'min', app_settings.WIFI_CLIENTS_MIN_CHECK_INTERVAL, store, send_alert
+        )
+
+    def _check_wifi_clients_max(self, store=True, send_alert=True):
+        return self._check_wifi_clients(
+            'max', app_settings.WIFI_CLIENTS_MAX_CHECK_INTERVAL, store, send_alert
+        )
+
+    def check(self, store=True):
+        # If the device health status is unknown,
+        # then do not run the check.
+        if self.related_object.monitoring.status in [
+            'unknown',
+        ]:
+            return
+        # If the device monitoring status is critical,
+        # then don't send alerts.
+        send_alert = self.related_object.monitoring.status != 'critical'
+        min = self._check_wifi_clients_min(store, send_alert)
+        max = self._check_wifi_clients_max(store, send_alert)
+        return {'wifi_clients_min': min, 'wifi_clients_max': max}
 
     def _get_metric(self, configuration):
         metric, created = self._get_or_create_metric(configuration=configuration)

openwisp_monitoring/check/settings.py

@@ -24,8 +24,11 @@
 WIFI_CLIENTS_CHECK_SNOOZE_SCHEDULE = get_settings_value(
     'WIFI_CLIENTS_CHECK_SNOOZE_SCHEDULE', []
 )
-WIFI_CLIENTS_CHECK_INTERVAL = int(
-    get_settings_value('WIFI_CLIENTS_CHECK_INTERVAL', 5)
+WIFI_CLIENTS_MAX_CHECK_INTERVAL = int(
+    get_settings_value('WIFI_CLIENTS_MAX_CHECK_INTERVAL', 5)
+)  # in minutes
+WIFI_CLIENTS_MIN_CHECK_INTERVAL = int(
+    get_settings_value('WIFI_CLIENTS_MIN_CHECK_INTERVAL', 4320)
 )  # in minutes
 AUTO_IPERF3 = get_settings_value('AUTO_IPERF3', False)
 IPERF3_CHECK_CONFIG = get_settings_value('IPERF3_CHECK_CONFIG', {})

openwisp_monitoring/check/tests/test_wifi_client.py

@@ -31,6 +31,12 @@ class TestWifiClient(
     def _run_wifi_clients_checks(self):
         tasks.run_checks(checks=[self._WIFI_CLIENTS])
 
+    def _create_device(self, monitoring_status='ok', *args, **kwargs):
+        device = super()._create_device(*args, **kwargs)
+        device.monitoring.status = monitoring_status
+        device.monitoring.save()
+        return device
+
     def test_store_result(self):
         def _assert_wifi_clients_metric(key):
             metric = metric_qs.get(key=key)
@@ -54,7 +60,7 @@ def _assert_wifi_clients_metric(key):
         self.assertEqual(alert_settings_qs.count(), 0)
         check = Check.objects.filter(check_type=self._WIFI_CLIENTS).first()
         result = check.perform_check()
-        self.assertEqual(result, 3)
+        self.assertEqual(result, {'wifi_clients_min': 3, 'wifi_clients_max': 3})
         self.assertEqual(metric_qs.count(), 2)
         self.assertEqual(alert_settings_qs.count(), 2)
 
@@ -67,7 +73,7 @@ def test_device_no_wifi_client(self):
         device = self._create_device()
         check = Check.objects.filter(check_type=self._WIFI_CLIENTS).first()
         result = check.perform_check()
-        self.assertEqual(result, 0)
+        self.assertEqual(result, {'wifi_clients_min': 0, 'wifi_clients_max': 0})
         wifi_clients_max = Metric.objects.filter(
             key='wifi_clients_max', object_id=device.id
         ).first()
@@ -81,6 +87,32 @@ def test_device_no_wifi_client(self):
         self.assertEqual(len(points), 1)
         self.assertEqual(points[0]['clients'], 0)
 
+    @patch.object(WifiClients, '_check_wifi_clients_min')
+    @patch.object(WifiClients, '_check_wifi_clients_max')
+    def test_check_skipped_unknown_status(self, max_mocked, min_mocked):
+        self._create_device(monitoring_status='unknown')
+        check = Check.objects.filter(check_type=self._WIFI_CLIENTS).first()
+        result = check.perform_check()
+        self.assertEqual(result, None)
+        max_mocked.assert_not_called()
+        min_mocked.assert_not_called()
+
+    @patch.object(Metric, 'write')
+    def test_sending_alert(self, mocked_write):
+        device = self._create_device(monitoring_status='critical')
+        check = Check.objects.filter(check_type=self._WIFI_CLIENTS).first()
+
+        with self.subTest('Test skip alert when device is critical'):
+            for calls in mocked_write.call_args_list:
+                self.assertEqual(calls[1]['send_alert'], False)
+
+        with self.subTest('Test alert when device has PROBLEM status'):
+            device.monitoring.status = 'problem'
+            device.monitoring.save()
+            check.perform_check()
+            for calls in mocked_write.call_args_list:
+                self.assertEqual(calls[1]['send_alert'], True)
+
     @patch.object(
         app_settings,
         'WIFI_CLIENTS_CHECK_SNOOZE_SCHEDULE',

openwisp_monitoring/monitoring/configuration.py

@@ -322,7 +322,7 @@ def _get_access_tech():
         'name': '{name}',
         'key': 'wifi_clients_min',
         'field_name': 'clients',
-        'alert_settings': {'operator': '<', 'threshold': 1, 'tolerance': 4320},
+        'alert_settings': {'operator': '<', 'threshold': 1, 'tolerance': 0},
         'notification': {
             'problem': {
                 'verbose_name': 'Min WiFi clients PROBLEM',