[change] Use device name as node label in admin too if enabled #75

Related to #75

openwisp_network_topology/admin.py

@@ -217,10 +217,12 @@ def visualize_view(self, request, pk):
         return TemplateResponse(request, 'admin/topology/visualize.html', context)
 
 
-class AutoOrgMixin(MultitenantAdminMixin):
+class NodeLinkMixin(MultitenantAdminMixin):
     """
     Hides organization in add form
-    Shows it readonly in change form
+    Shows organization readonly in change form
+    Adds get_queryset class method that enables
+    other extensions to override the admin queryset
     """
 
     readonly_fields = ['organization']
@@ -233,12 +235,15 @@ def get_fields(self, request, obj):
         fields_copy.remove('organization')
         return fields_copy
 
+    def get_queryset(self, request):
+        return self.model.get_queryset(super().get_queryset(request))
+
 
 @admin.register(Node)
-class NodeAdmin(AutoOrgMixin, BaseAdmin):
+class NodeAdmin(NodeLinkMixin, BaseAdmin):
     model = Node
     change_form_template = 'admin/topology/node/change_form.html'
-    list_display = ['name', 'organization', 'topology', 'addresses']
+    list_display = ['get_name', 'organization', 'topology', 'addresses']
     search_fields = ['addresses', 'label', 'properties']
     list_filter = [
         ('organization', MultitenantOrgFilter),
@@ -271,7 +276,7 @@ def change_view(self, request, object_id, form_url='', extra_context=None):
 
 
 @admin.register(Link)
-class LinkAdmin(AutoOrgMixin, BaseAdmin):
+class LinkAdmin(NodeLinkMixin, BaseAdmin):
     model = Link
     raw_id_fields = ['source', 'target']
     search_fields = [

openwisp_network_topology/base/link.py

@@ -65,7 +65,7 @@ def save(self, *args, **kwargs):
         self._initial_status = self.status
 
     def __str__(self):
-        return '{0} - {1}'.format(self.source.name, self.target.name)
+        return '{0} - {1}'.format(self.source.get_name(), self.target.get_name())
 
     def full_clean(self, *args, **kwargs):
         self.organization = self.topology.organization
@@ -144,3 +144,8 @@ def delete_expired_links(cls):
                 print_info('Deleting {0} expired links'.format(expired_links_length))
                 for link in expired_links:
                     link.delete()
+
+    @classmethod
+    def get_queryset(cls, qs):
+        """ admin list queryset """
+        return qs.select_related('organization', 'topology', 'source', 'target')

openwisp_network_topology/base/node.py

@@ -66,19 +66,19 @@ def local_addresses(self):
     def name(self):
         return self.label or self.netjson_id or ''
 
-    def get_label(self):
+    def get_name(self):
         """
         May be overridden/monkey patched to get the node name
         from other sources (e.g: device name in openwisp-controller)
         """
-        return self.label
+        return self.name
 
     def json(self, dict=False, **kwargs):
         """
         returns a NetJSON NetworkGraph Node object
         """
         netjson = OrderedDict({'id': self.netjson_id})
-        label = self.get_label()
+        label = self.get_name()
         if label:
             netjson['label'] = label
         for attr in ['local_addresses', 'properties']:
@@ -140,3 +140,8 @@ def delete_expired_nodes(cls):
                 print_info('Deleting {0} expired nodes'.format(expired_nodes_length))
                 for node in expired_nodes:
                     node.delete()
+
+    @classmethod
+    def get_queryset(cls, qs):
+        """ admin list queryset """
+        return qs.select_related('organization', 'topology')

openwisp_network_topology/integrations/device/apps.py

@@ -1,3 +1,5 @@
+from importlib import import_module
+
 import swapper
 from django.apps import AppConfig
 from django.db import transaction
@@ -41,35 +43,4 @@ def link_status_changed_receiver(cls, link, **kwargs):
         transaction.on_commit(lambda: trigger_device_updates.delay(link.pk))
 
     def override_node_label(self):
-        Node = swapper.load_model('topology', 'Node')
-        Topology = swapper.load_model('topology', 'Topology')
-
-        def get_label(self):
-            if hasattr(self, 'devicenode'):
-                return self.devicenode.device.name
-            return super(Node, self).get_label()
-
-        def get_nodes_queryset(self):
-            """
-            Overrides Topology.get_nodes_queryset
-            to avoid generating 2 additional queries for each node
-            when using the name of the device as the node label
-            """
-            return (
-                super(Topology, self)
-                .get_nodes_queryset()
-                .select_related('devicenode__device')
-                .only(
-                    'id',
-                    'topology_id',
-                    'label',
-                    'addresses',
-                    'properties',
-                    'created',
-                    'modified',
-                    'devicenode__device__name',
-                )
-            )
-
-        Node.get_label = get_label
-        Topology.get_nodes_queryset = get_nodes_queryset
+        import_module('openwisp_network_topology.integrations.device.overrides')

openwisp_network_topology/integrations/device/overrides.py

@@ -0,0 +1,88 @@
+import swapper
+
+Node = swapper.load_model('topology', 'Node')
+Link = swapper.load_model('topology', 'Link')
+Topology = swapper.load_model('topology', 'Topology')
+
+
+def get_name(self):
+    if hasattr(self, 'devicenode'):
+        return self.devicenode.device.name
+    return super(Node, self).get_name()
+
+
+def topology_get_nodes_queryset(self):
+    """
+    Overrides Topology.get_nodes_queryset
+    to avoid generating 2 additional queries for each node
+    when using the name of the device as the node label
+    """
+    return (
+        super(Topology, self)
+        .get_nodes_queryset()
+        .select_related('devicenode__device')
+        .only(
+            'id',
+            'topology_id',
+            'label',
+            'addresses',
+            'properties',
+            'created',
+            'modified',
+            'devicenode__device__name',
+        )
+    )
+
+
+@classmethod
+def node_get_queryset(cls, qs):
+    return (
+        super(Node, cls)
+        .get_queryset(qs)
+        .select_related('devicenode__device')
+        .only(
+            'id',
+            'topology_id',
+            'topology__label',
+            'topology__parser',
+            'organization__id',
+            'organization__name',
+            'organization__is_active',
+            'label',
+            'addresses',
+            'devicenode__device__name',
+        )
+    )
+
+
+@classmethod
+def link_get_queryset(cls, qs):
+    return (
+        super(Link, cls)
+        .get_queryset(qs)
+        .select_related('source__devicenode__device', 'target__devicenode__device')
+        .only(
+            'id',
+            'topology_id',
+            'topology__label',
+            'topology__parser',
+            'organization__id',
+            'organization__name',
+            'organization__is_active',
+            'status',
+            'cost',
+            'cost_text',
+            'source__addresses',
+            'source__label',
+            'source__devicenode__device__name',
+            'target__addresses',
+            'target__label',
+            'target__devicenode__device__name',
+        )
+    )
+
+
+Node.get_name = get_name
+Node.get_queryset = node_get_queryset
+Link.get_queryset = link_get_queryset
+Topology.get_nodes_queryset = topology_get_nodes_queryset

openwisp_network_topology/integrations/device/tests.py

@@ -2,8 +2,10 @@
 
 import django
 import swapper
+from django.contrib.auth import get_user_model
 from django.db import transaction
 from django.test import TransactionTestCase
+from django.urls import reverse
 from django.utils.module_loading import import_string
 from openwisp_controller.config.tests.utils import (
     CreateConfigTemplateMixin,
@@ -216,7 +218,7 @@ def test_link_up_openvpn_failure(self, logger_warning):
     def test_node_label_override(self):
         topology, device, cert = self._create_test_env(parser='netdiff.OpenvpnParser')
         node = self._init_test_node(topology, common_name=cert.common_name)
-        self.assertEqual(node.get_label(), device.name)
+        self.assertEqual(node.get_name(), device.name)
 
     def test_topology_json(self):
         topology, device, cert = self._create_test_env(parser='netdiff.OpenvpnParser')
@@ -265,3 +267,46 @@ def test_monitoring_integration(self, *args):
                 link.save()
             mocked_task.assert_called_once()
             mocked_task.assert_called_with(device.pk, recovery=False)
+
+
+class TestAdmin(Base, TransactionTestCase):
+    module = 'openwisp_network_topology'
+    app_label = 'topology'
+    topology_model = Topology
+    link_model = Link
+    node_model = Node
+    user_model = get_user_model()
+    fixtures = ['test_users.json']
+    api_urls_path = 'api.urls'
+
+    @property
+    def prefix(self):
+        return 'admin:{0}'.format(self.app_label)
+
+    def setUp(self):
+        org = self._create_org()
+        t = self._create_topology(organization=org)
+        topology, device, cert = self._create_test_env(parser='netdiff.OpenvpnParser')
+        node1 = self._init_test_node(topology, common_name=cert.common_name)
+        node2 = self._init_test_node(topology, addresses=['netjson_id2'], label='test2')
+        link = Link(
+            source=node1,
+            target=node2,
+            topology=topology,
+            organization=topology.organization,
+            cost=1,
+        )
+        link.full_clean()
+        link.save()
+        self._create_link(topology=t, source=node1, target=node2)
+        self.client.force_login(self.user_model.objects.get(username='admin'))
+
+    def test_node_change_list_queries(self):
+        path = reverse('{0}_node_changelist'.format(self.prefix))
+        with self.assertNumQueries(7):
+            self.client.get(path)
+
+    def test_link_change_list_queries(self):
+        path = reverse('{0}_link_changelist'.format(self.prefix))
+        with self.assertNumQueries(7):
+            self.client.get(path)

openwisp_network_topology/tests/test_admin.py

@@ -160,6 +160,20 @@ def test_node_change_form(self):
         self.assertNotContains(response, 'organization_id')
         self.assertContains(response, n.topology.organization.name)
 
+    def test_node_change_list_queries(self):
+        path = reverse('{0}_node_changelist'.format(self.prefix))
+        with self.assertNumQueries(7):
+            self.client.get(path)
+
+    def test_link_change_list_queries(self):
+        t = Topology.objects.first()
+        n1 = self._create_node(label='node1org1', topology=t)
+        n2 = self._create_node(label='node2org1', topology=t)
+        self._create_link(topology=t, source=n1, target=n2)
+        path = reverse('{0}_link_changelist'.format(self.prefix))
+        with self.assertNumQueries(7):
+            self.client.get(path)
+
     def test_link_change_form(self):
         t = Topology.objects.first()
         n1 = self._create_node(label='node1org1', topology=t)

tests/openwisp2/sample_integration_device/tests.py

@@ -1,5 +1,8 @@
 import swapper
 
+from openwisp_network_topology.integrations.device.tests import (
+    TestAdmin as BaseTestAdmin,
+)
 from openwisp_network_topology.integrations.device.tests import (
     TestControllerIntegration as BaseTestControllerIntegration,
 )
@@ -19,5 +22,11 @@ class TestMonitoringIntegration(BaseTestMonitoringIntegration):
     pass
 
 
+class TestAdmin(BaseTestAdmin):
+    module = 'openwisp2.sample_network_topology'
+    app_label = 'sample_network_topology'
+
+
 del BaseTestControllerIntegration
 del BaseTestMonitoringIntegration
+del BaseTestAdmin