[feature] Flagged the HT/VHT field of WiFi clients as null #351

If HT/VHT is not being used, set the HT/VHT field
of WiFi clients to None. This is necessary because
some clients may be VHT capable but VHT is not enabled
at the radio level, which can lead to false information.

Closes #351

.github/workflows/ci.yml

@@ -78,7 +78,7 @@ jobs:
       - name: Install openwisp-monitoring
         run: |
           pip install -U -I -e .
-          pip install ${{ matrix.django-version }}
+          pip install --force-reinstall ${{ matrix.django-version }}
 
       - name: QA checks
         run: |

openwisp_monitoring/device/admin.py

@@ -344,7 +344,12 @@ class DeviceAdminExportable(ImportExportMixin, DeviceAdmin):
 
 class WifiSessionAdminHelperMixin:
     def _get_boolean_html(self, value):
-        icon = static('admin/img/icon-{}.svg'.format('yes' if value is True else 'no'))
+        icon_type = 'unknown'
+        if value is True:
+            icon_type = 'yes'
+        elif value is False:
+            icon_type = 'no'
+        icon = static(f'admin/img/icon-{icon_type}.svg')
         return mark_safe(f'<img src="{icon}">')
 
     def ht(self, obj):

openwisp_monitoring/device/base/models.py

@@ -183,6 +183,27 @@ def _transform_data(self):
                 for signal_key, signal_values in interface['mobile']['signal'].items():
                     for key, value in signal_values.items():
                         signal_values[key] = float(value)
+            # If HT/VHT is not being used ie. htmode = 'NOHT',
+            # set the HT/VHT field of WiFi clients to None.
+            # This is necessary because some clients may be
+            # VHT capable but VHT is not enabled at the radio level,
+            # which can mislead into thinking the client is not HT/VHT capable.
+            if (
+                'wireless' in interface
+                and 'htmode' in interface['wireless']
+                and 'clients' in interface['wireless']
+            ):
+                for client in interface['wireless']['clients']:
+                    # NOHT : disables 11n (ie. ht and vht both are False)
+                    if interface['wireless']['htmode'] == 'NOHT':
+                        client['ht'] = None
+                        client['vht'] = None
+                    # If only HT is enabled, set client vht to None
+                    elif (
+                        interface['wireless']['htmode'].startswith('HT')
+                        and not client['vht']
+                    ):
+                        client['vht'] = None
             # add mac vendor to wireless clients if present
             if (
                 not mac_detection
@@ -350,8 +371,8 @@ class AbstractWifiClient(TimeStampedEditableModel):
         help_text=_('MAC address'),
     )
     vendor = models.CharField(max_length=200, blank=True, null=True)
-    ht = models.BooleanField(default=False, verbose_name='HT')
-    vht = models.BooleanField(default=False, verbose_name='VHT')
+    ht = models.BooleanField(null=True, blank=True, default=None, verbose_name='HT')
+    vht = models.BooleanField(null=True, blank=True, default=None, verbose_name='VHT')
     wmm = models.BooleanField(default=False, verbose_name='WMM')
     wds = models.BooleanField(default=False, verbose_name='WDS')
     wps = models.BooleanField(default=False, verbose_name='WPS')

openwisp_monitoring/device/migrations/0006_alter_wificlient_field_ht_vht.py

@@ -0,0 +1,27 @@
+# Generated by Django 3.2.18 on 2023-04-04 10:25
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('device_monitoring', '0005_add_group_permissions'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='wificlient',
+            name='ht',
+            field=models.BooleanField(
+                blank=True, default=None, null=True, verbose_name='HT'
+            ),
+        ),
+        migrations.AlterField(
+            model_name='wificlient',
+            name='vht',
+            field=models.BooleanField(
+                blank=True, default=None, null=True, verbose_name='VHT'
+            ),
+        ),
+    ]

openwisp_monitoring/device/templates/admin/config/device/change_form.html

@@ -427,10 +427,10 @@ <h2>{% trans 'Interface status' %}: {{ interface.name }}</h2>
                           </td>
                           {% endif %}
                           <td class="ht">
-                              <img src="{% get_static_prefix %}admin/img/icon-{{ client.ht|yesno:'yes,no,no' }}.svg">
+                              <img src="{% get_static_prefix %}admin/img/icon-{{ client.ht|yesno:'yes,no,unknown' }}.svg">
                           </td>
                           <td class="vht">
-                              <img src="{% get_static_prefix %}admin/img/icon-{{ client.vht|yesno:'yes,no,no' }}.svg">
+                              <img src="{% get_static_prefix %}admin/img/icon-{{ client.vht|yesno:'yes,no,unknown' }}.svg">
                           </td>
                           <td class="wmm">
                               <img src="{% get_static_prefix %}admin/img/icon-{{ client.wmm|yesno:'yes,no,no' }}.svg">

openwisp_monitoring/device/tests/__init__.py

@@ -52,6 +52,42 @@ def _create_device_monitoring(self):
         dm.save()
         return dm
 
+    def _transform_wireless_interface_test_data(self, data):
+        interfaces = data.get('interfaces', [])
+        wireless_interfaces = [
+            interface
+            for interface in interfaces
+            if 'wireless' in interface
+            and 'htmode' in interface['wireless']
+            and 'clients' in interface['wireless']
+        ]
+        for interface in wireless_interfaces:
+            for client in interface['wireless']['clients']:
+                if (
+                    interface['wireless']['htmode'] == 'NOHT'
+                    and not client['ht']
+                    and not client['vht']
+                ):
+                    client['ht'] = None
+                    client['vht'] = None
+                elif (
+                    interface['wireless']['htmode'].startswith('HT')
+                    and client['ht']
+                    and not client['vht']
+                ):
+                    client['vht'] = None
+
+        return data
+
+    def assertDataDict(self, dd_data, data):
+        """
+        This method is necessary as the wireless interface data
+        is modified by the `AbstractDeviceData._transform_data`
+        method.
+        """
+        data = self._transform_wireless_interface_test_data(data)
+        self.assertDictEqual(dd_data, data)
+
     def create_test_data(self, no_resources=False):
         o = self._create_org()
         d = self._create_device(organization=o)
@@ -63,7 +99,7 @@ def create_test_data(self, no_resources=False):
         r = self._post_data(d.id, d.key, data)
         self.assertEqual(r.status_code, 200)
         dd = DeviceData(pk=d.pk)
-        self.assertDictEqual(dd.data, data)
+        self.assertDataDict(dd.data, data)
         if no_resources:
             metric_count, chart_count = 4, 4
         else:
@@ -136,7 +172,7 @@ def _create_multiple_measurements(self, create=True, no_resources=False, count=4
         data4['interfaces'][1]['statistics']['tx_bytes'] = 500000000
         r = self._post_data(d.id, d.key, data4)
         self.assertEqual(r.status_code, 200)
-        self.assertDictEqual(dd.data, data4)
+        self.assertDataDict(dd.data, data4)
         return dd
 
     def _data(self):

openwisp_monitoring/device/tests/test_admin.py

@@ -158,22 +158,64 @@ def test_status_data_contains_wifi_version(self):
         self.assertContains(
             response,
             """
-            <div class="readonly">
-                WiFi 4 (802.11n): HT20
-            </div>
+                <div class="readonly">
+                    WiFi 4 (802.11n): HT20
+                </div>
             """,
             html=True,
         )
         self.assertContains(
             response,
             """
-            <div class="readonly">
-                WiFi 5 (802.11ac): VHT80
-            </div>
+                <div class="readonly">
+                    WiFi 5 (802.11ac): VHT80
+                </div>
             """,
             html=True,
         )
 
+    def test_status_data_contains_wifi_client_ht_vht_unknown(self):
+        data = deepcopy(self._data())
+        d = self._create_device(organization=self._create_org())
+        url = reverse('admin:config_device_change', args=[d.pk])
+        wireless_interface = data['interfaces'][0]['wireless']
+        client = data['interfaces'][0]['wireless']['clients'][0]
+
+        with self.subTest('Test when HT is disabled'):
+            wireless_interface.update({'htmode': 'NOHT'})
+            self._post_data(d.id, d.key, data)
+            response = self.client.get(url)
+            self.assertContains(
+                response,
+                """
+                    <td class="ht">
+                        <img src="/static/admin/img/icon-unknown.svg">
+                    </td>
+                    <td class="vht">
+                        <img src="/static/admin/img/icon-unknown.svg">
+                    </td>
+                """,
+                html=True,
+            )
+
+        with self.subTest('Test when HT is enabled'):
+            wireless_interface.update({'htmode': 'HT40'})
+            self.assertEqual(client['vht'], False)
+            self._post_data(d.id, d.key, data)
+            response = self.client.get(url)
+            self.assertContains(
+                response,
+                """
+                    <td class="ht">
+                        <img src="/static/admin/img/icon-yes.svg">
+                    </td>
+                    <td class="vht">
+                        <img src="/static/admin/img/icon-unknown.svg">
+                    </td>
+                """,
+                html=True,
+            )
+
     def test_no_device_data(self):
         d = self._create_device(organization=self._create_org())
         url = reverse('admin:config_device_change', args=[d.pk])
@@ -698,16 +740,17 @@ class TestWifiSessionAdmin(
     TestWifiClientSessionMixin,
     TestCase,
 ):
+
+    wifi_session_app_label = WifiSession._meta.app_label
+    wifi_session_model_name = WifiSession._meta.model_name
+
     def setUp(self):
         admin = self._create_admin()
         self.client.force_login(admin)
 
     def test_changelist_filters_and_multitenancy(self):
         url = reverse(
-            'admin:{app_label}_{model_name}_changelist'.format(
-                app_label=WifiSession._meta.app_label,
-                model_name=WifiSession._meta.model_name,
-            )
+            f'admin:{self.wifi_session_app_label}_{self.wifi_session_model_name}_changelist'
         )
         org1 = self._create_org(name='org1', slug='org1')
         org1_device_group = self._create_device_group(
@@ -858,3 +901,69 @@ def test_dashboard_wifi_session_chart(self):
             response.context['dashboard_charts'][13]['query_params'],
             {'labels': [], 'values': []},
         )
+
+    def test_wifi_client_ht_vht_unknown(self):
+        test_wifi_client = self._create_wifi_client(ht=None, vht=None)
+        test_wifi_session = self._create_wifi_session(wifi_client=test_wifi_client)
+        device = Device.objects.all().first()
+
+        with self.subTest('Test device wifi session inline'):
+            url = reverse('admin:config_device_change', args=[device.id])
+            response = self.client.get(url)
+            self.assertContains(
+                response,
+                """
+                    <td class="field-ht">
+                        <p><img src="/static/admin/img/icon-unknown.svg"></p>
+                    </td>
+                    <td class="field-vht">
+                        <p><img src="/static/admin/img/icon-unknown.svg"></p>
+                    </td>
+                """,
+                html=True,
+            )
+        with self.subTest('Test device wifi session list'):
+            url = reverse(
+                f'admin:{self.wifi_session_app_label}_{self.wifi_session_model_name}_changelist'
+            )
+            response = self.client.get(url)
+            self.assertContains(
+                response,
+                """
+                    <td class="field-ht">
+                        <img src="/static/admin/img/icon-unknown.svg">
+                    </td>
+                    <td class="field-vht">
+                        <img src="/static/admin/img/icon-unknown.svg">
+                    </td>
+                """,
+                html=True,
+            )
+        with self.subTest('Test device wifi session change'):
+            url = reverse(
+                f'admin:{self.wifi_session_app_label}_{self.wifi_session_model_name}_change',
+                args=[test_wifi_session.id],
+            )
+            response = self.client.get(url)
+            self.assertContains(
+                response,
+                """
+                    <div class="form-row field-ht">
+                    <div>
+                        <label>HT:</label>
+                            <div class="readonly">
+                                <img src="/static/admin/img/icon-unknown.svg">
+                            </div>
+                        </div>
+                    </div>
+                    <div class="form-row field-vht">
+                    <div>
+                        <label>VHT:</label>
+                            <div class="readonly">
+                                <img src="/static/admin/img/icon-unknown.svg">
+                            </div>
+                        </div>
+                    </div>
+                """,
+                html=True,
+            )

openwisp_monitoring/device/tests/test_api.py

@@ -176,7 +176,7 @@ def test_200_traffic_counter_incremented(self):
         data2['interfaces'][1]['statistics']['tx_bytes'] = 4567
         r = self._post_data(d.id, d.key, data2)
         self.assertEqual(r.status_code, 200)
-        self.assertDictEqual(dd.data, data2)
+        self.assertDataDict(dd.data, data2)
         # Add 1 for general metric and chart
         self.assertEqual(self.metric_queryset.count(), 4)
         self.assertEqual(self.chart_queryset.count(), 4)
@@ -208,7 +208,7 @@ def test_200_traffic_counter_reset(self):
         data2['interfaces'][1]['statistics']['tx_bytes'] = 120
         r = self._post_data(d.id, d.key, data2)
         self.assertEqual(r.status_code, 200)
-        self.assertDictEqual(dd.data, data2)
+        self.assertDataDict(dd.data, data2)
         # Add 1 for general metric and chart
         self.assertEqual(self.metric_queryset.count(), 4)
         self.assertEqual(self.chart_queryset.count(), 4)

openwisp_monitoring/device/tests/test_models.py

@@ -729,7 +729,7 @@ def test_wifi_client_session_created(self):
         wifi_client1 = WifiClient.objects.get(mac_address='00:ee:ad:34:f5:3b')
         self.assertEqual(wifi_client1.vendor, None)
         self.assertEqual(wifi_client1.ht, True)
-        self.assertEqual(wifi_client1.vht, False)
+        self.assertEqual(wifi_client1.vht, None)
         self.assertEqual(wifi_client1.wmm, True)
         self.assertEqual(wifi_client1.wds, False)
         self.assertEqual(wifi_client1.wps, False)
@@ -798,6 +798,7 @@ def test_opening_closing_existing_sessions(self):
             self.assertEqual(WifiClient.objects.count(), 3)
 
         with self.subTest('Test re-opening session for exising clients'):
+            data = deepcopy(self._sample_data)
             self._save_device_data(device_data, data)
             self.assertEqual(WifiSession.objects.filter(stop_time=None).count(), 3)
             self.assertEqual(WifiSession.objects.count(), 6)
@@ -811,23 +812,27 @@ def test_delete_old_wifi_sessions(self):
         self.assertEqual(WifiSession.objects.count(), 0)
 
     def test_database_queries(self):
-        data = deepcopy(self._sample_data)
         device_data = self._create_device_data()
+
         with self.subTest('Test creating new clients and sessions'):
+            data = deepcopy(self._sample_data)
             with self.assertNumQueries(28):
                 self._save_device_data(device_data, data)
 
         with self.subTest('Test updating existing clients and sessions'):
+            data = deepcopy(self._sample_data)
             with self.assertNumQueries(7):
                 self._save_device_data(device_data, data)
 
         with self.subTest('Test closing existing sessions'):
+            data = deepcopy(self._sample_data)
             with self.assertNumQueries(1):
                 self._save_device_data(
                     device_data, data={'type': 'DeviceMonitoring', 'interface': []}
                 )
 
         with self.subTest('Test new sessions for existing clients'):
+            data = deepcopy(self._sample_data)
             with self.assertNumQueries(16):
                 self._save_device_data(device_data, data)