Repository: openwisp/openwisp-network-topology (language: python)

Issue:
[change] Do we really need two routes for the same websocket consumer?
In https://github.com/openwisp/openwisp-network-topology/pull/171 we fixed a bug which was preventing a non-admin view to connect to the websocket consumer to receive updates regarding a topology.

A new route was added due to how the JS is currently written, but I am wondering if this additional route is really necessary and whether it wouldn't be better to use one route in both places (admin and frontend)?
[change] Do we really need two routes for the same websocket consumer?
In https://github.com/openwisp/openwisp-network-topology/pull/171 we fixed a bug which was preventing a non-admin view to connect to the websocket consumer to receive updates regarding a topology.

A new route was added due to how the JS is currently written, but I am wondering if this additional route is really necessary and whether it wouldn't be better to use one route in both places (admin and frontend)?
[change] Do we really need two routes for the same websocket consumer?
In https://github.com/openwisp/openwisp-network-topology/pull/171 we fixed a bug which was preventing a non-admin view to connect to the websocket consumer to receive updates regarding a topology.

A new route was added due to how the JS is currently written, but I am wondering if this additional route is really necessary and whether it wouldn't be better to use one route in both places (admin and frontend)?


Discussion / hints:

Fix patch:
diff --git a/openwisp_network_topology/routing.py b/openwisp_network_topology/routing.py
index 4d34c022..1a24b6a0 100644
--- a/openwisp_network_topology/routing.py
+++ b/openwisp_network_topology/routing.py
@@ -3,13 +3,10 @@
 from . import consumers
 
 websocket_urlpatterns = [
-    # For topology admin view
+    # This route is used by both
+    # the admin and non-admin topology view
     re_path(
-        r'^admin/topology/topology/(?P<pk>[^/]+)/change/$',
+        r'^network-topology/topology/(?P<pk>[^/]+)/$',
         consumers.TopologyConsumer.as_asgi(),
     ),
-    # For topology non admin view
-    re_path(
-        r'^topology/topology/(?P<pk>[^/]+)/$', consumers.TopologyConsumer.as_asgi()
-    ),
 ]
diff --git a/openwisp_network_topology/templates/netjsongraph/netjsongraph-script.html b/openwisp_network_topology/templates/netjsongraph/netjsongraph-script.html
index 8464c825..9a5ba953 100644
--- a/openwisp_network_topology/templates/netjsongraph/netjsongraph-script.html
+++ b/openwisp_network_topology/templates/netjsongraph/netjsongraph-script.html
@@ -1,7 +1,8 @@
 <script>
     loadNetJsonGraph = (el='body', url='{{ graph_url }}') => {
         const history_url = '{{ history_url }}';
-        const wsUrl = `ws://${window.location.host}${window.location.pathname}`;
+        const wsProtocol = window.location.protocol === 'https:' ? 'wss:' : 'ws:';
+        const wsUrl = `${wsProtocol}//${window.location.host}/network-topology/topology/${getTopologyIdFromUrl()}/`;
         const socket = new WebSocket(wsUrl);
 
         const getDataParseOptions = (data) => {
@@ -168,6 +169,21 @@
         graph.render();
         window.initTopologyHistory(django.jQuery);
         return graph
+
+        // Utility function
+        function getTopologyIdFromUrl() {
+            let topologyId;
+            try {
+                topologyId = /\/((\w{4,12}-?)){5}\//.exec(window.location)[0];
+            } catch (error) {
+                try {
+                    topologyId = /\/(\d+)\//.exec(window.location)[0];
+                } catch (error) {
+                    throw error;
+                }
+            }
+            return topologyId.replace(/\//g, '');
+        }
     };
 
 </script>


Test patch:
diff --git a/openwisp_network_topology/tests/test_websockets.py b/openwisp_network_topology/tests/test_websockets.py
index 55a7c6b3..864e0970 100644
--- a/openwisp_network_topology/tests/test_websockets.py
+++ b/openwisp_network_topology/tests/test_websockets.py
@@ -20,19 +20,11 @@ class TestTopologySockets(CreateGraphObjectsMixin, CreateOrgMixin):
     topology_model = Topology
     application = import_string(getattr(settings, 'ASGI_APPLICATION'))
 
-    async def _get_url(self, topology_id, admin_view):
-        non_admin_view_url = f'topology/topology/{topology_id}/'
-        admin_view_url = f'admin/topology/topology/{topology_id}/change/'
-        if not admin_view:
-            return non_admin_view_url
-        return admin_view_url
-
-    async def _get_communicator(self, admin_client, topology_id, admin_view=True):
-        url = await self._get_url(topology_id, admin_view)
+    async def _get_communicator(self, admin_client, topology_id):
         session_id = admin_client.cookies['sessionid'].value
         communicator = WebsocketCommunicator(
             self.application,
-            url,
+            path=f'network-topology/topology/{topology_id}/',
             headers=[
                 (
                     b'cookie',
@@ -135,13 +127,3 @@ async def test_topology_properties_update(self, admin_user, admin_client):
         assert response['topology'] is not None
         assert response['topology'] == expected_response
         await communicator.disconnect()
-
-    async def test_non_admin_view_consumer_connection(self, admin_user, admin_client):
-        org = await database_sync_to_async(self._create_org)()
-        t = await database_sync_to_async(self._create_topology)(organization=org)
-        communicator = await self._get_communicator(
-            admin_client, t.pk, admin_view=False
-        )
-        connected, _ = await communicator.connect()
-        assert connected is True
-        await communicator.disconnect()