[req-change] Simplified code

Author: pandafy

openwisp_controller/config/base/device.py

@@ -437,10 +437,14 @@ def get_temp_config_instance(self, **options):
 
     def should_skip_push_update(self):
         """
-        Returns True if the device should skip push update.
-        Can be overridden with custom logic if needed.
+        Check if push update should be skipped for this device.
+
+        Clears the skip flag after reading it.
         """
-        return getattr(self, "_skip_push_update", False)
+        result = getattr(self, "_skip_push_update", False)
+        if result:
+            delattr(self, "_skip_push_update")
+        return result
 
     def skip_push_update_on_save(self):
         """
@@ -450,14 +454,6 @@ def skip_push_update_on_save(self):
         """
         self._skip_push_update = True
 
-    def clear_skip_push_update(self):
-        """
-        Clears the skip push update flag.
-        This should be called after the flag has been checked and consumed.
-        """
-        if hasattr(self, "_skip_push_update"):
-            delattr(self, "_skip_push_update")
-
     def can_be_updated(self):
         """
         returns True if the device can and should be updated

openwisp_controller/config/controller/views.py

@@ -289,7 +289,7 @@ class DeviceRegisterView(UpdateLastIpMixin, CsrfExtemptMixin, View):
     model = Device
     org_config_settings_model = OrganizationConfigSettings
 
-    UPDATABLE_FIELDS = ["os", "model", "system"]
+    UPDATABLE_FIELDS = ["name", "os", "model", "system"]
 
     def init_object(self, **kwargs):
         """
@@ -405,11 +405,13 @@ def post(self, request, *args, **kwargs):
             # update hw info
             for attr in self.UPDATABLE_FIELDS:
                 if attr in request.POST:
+                    if attr == "name":
+                        if self._should_update_hostname(request, device):
+                            device.skip_push_update_on_save()
+                        else:
+                            # Don't update Device.name
+                            continue
                     setattr(device, attr, request.POST.get(attr))
-            # update hostname if it's not a default/generic value
-            if self._should_update_hostname(request, device):
-                device.name = request.POST.get("name")
-                device.skip_push_update_on_save()
             config = device.config
         # if get queryset fails, instantiate a new Device and Config
         except self.model.DoesNotExist:
@@ -470,24 +472,15 @@ def _should_update_hostname(self, request, device):
 
         This prevents overwriting user configured hostnames with default/generic values.
         """
-        if "name" not in request.POST:
-            return False
-
         name = request.POST.get("name")
         mac_address = device.mac_address
-        # normalize MAC address format (strip colons/dashes and lowercase)
         normalized_mac = (
             mac_address.replace(":", "").replace("-", "").lower()
             if mac_address
             else None
         )
-        # normalize hostname for comparison
         normalized_name = name.replace(":", "").replace("-", "").lower()
-        # don't update if hostname equals the MAC address
-        if normalized_name == normalized_mac:
-            return False
-
-        return True
+        return normalized_name != normalized_mac
 
 
 class GetVpnView(SingleObjectMixin, View):

openwisp_controller/config/tests/test_controller.py

@@ -5,7 +5,7 @@
 from django.core.exceptions import ValidationError
 from django.http.response import Http404
 from django.test import TestCase
-from django.urls import reverse
+from django.urls import reverse, reverse_lazy
 from swapper import load_model
 
 from openwisp_utils.tests import capture_any_output, catch_signal
@@ -38,13 +38,8 @@
 Organization = load_model("openwisp_users", "Organization")
 
 
-class TestController(CreateConfigTemplateMixin, TestVpnX509Mixin, TestCase):
-    """
-    tests for config.controller
-    """
-
-    def setUp(self):
-        self.register_url = reverse("controller:device_register")
+class TestControllerMixin:
+    register_url = reverse_lazy("controller:device_register")
 
     def _create_org(self, shared_secret=TEST_ORG_SHARED_SECRET, **kwargs):
         org = super()._create_org(**kwargs)
@@ -53,6 +48,27 @@ def _create_org(self, shared_secret=TEST_ORG_SHARED_SECRET, **kwargs):
         )
         return org
 
+    def _get_reregistration_payload(self, device, **kwargs):
+        data = {
+            "secret": str(device.organization.config_settings.shared_secret),
+            "key": TEST_CONSISTENT_KEY,
+            "mac_address": device.mac_address,
+            "backend": "netjsonconfig.OpenWrt",
+            "model": "TP-Link TL-WDR4300 v2",
+            "os": "OpenWrt 18.06-SNAPSHOT r7312-e60be11330",
+            "system": "Atheros AR9344 rev 3",
+        }
+        data.update(**kwargs)
+        return data
+
+
+class TestController(
+    TestControllerMixin, CreateConfigTemplateMixin, TestVpnX509Mixin, TestCase
+):
+    """
+    tests for config.controller
+    """
+
     def _check_header(self, response):
         self.assertEqual(response["X-Openwisp-Controller"], "true")
 
@@ -820,7 +836,8 @@ def test_device_registration_update_hw_info_no_config(self):
         self.assertEqual(d.system, params["system"])
         self.assertEqual(d.model, params["model"])
 
-    def test_device_registration_update_hostname(self):
+    @patch.object(Device, 'skip_push_update_on_save')
+    def test_device_registration_update_hostname(self, mocked_method):
         """
         Test that hostname is updated when the name in payload
         is not the MAC address stored in OpenWISP
@@ -832,21 +849,16 @@ def test_device_registration_update_hostname(self):
                 "key": TEST_CONSISTENT_KEY,
             }
         )
-        params = {
-            "secret": TEST_ORG_SHARED_SECRET,
-            "name": "new-custom-hostname",
-            "mac_address": TEST_MACADDR,
-            "key": TEST_CONSISTENT_KEY,
-            "backend": "netjsonconfig.OpenWrt",
-            "model": "TP-Link TL-WDR4300 v2",
-            "os": "OpenWrt 18.06-SNAPSHOT r7312-e60be11330",
-            "system": "Atheros AR9344 rev 3",
-        }
+        params = self._get_reregistration_payload(
+            device=device,
+            name="new-custom-hostname",
+        )
         self.assertNotEqual(device.name, params["name"])
         response = self.client.post(self.register_url, params)
         self.assertEqual(response.status_code, 201)
         device.refresh_from_db()
         self.assertEqual(device.name, "new-custom-hostname")
+        mocked_method.assert_called_once()
 
     def test_device_registration_hostname_not_updated_when_mac_address(self):
         """
@@ -860,16 +872,10 @@ def test_device_registration_hostname_not_updated_when_mac_address(self):
                 "key": TEST_CONSISTENT_KEY,
             }
         )
-        params = {
-            "secret": TEST_ORG_SHARED_SECRET,
-            "name": TEST_MACADDR,
-            "mac_address": TEST_MACADDR,
-            "key": TEST_CONSISTENT_KEY,
-            "backend": "netjsonconfig.OpenWrt",
-            "model": "TP-Link TL-WDR4300 v2",
-            "os": "OpenWrt 18.06-SNAPSHOT r7312-e60be11330",
-            "system": "Atheros AR9344 rev 3",
-        }
+        params = self._get_reregistration_payload(
+            device=device,
+            name=TEST_MACADDR,
+        )
         response = self.client.post(self.register_url, params)
         self.assertEqual(response.status_code, 201)
         device.refresh_from_db()
@@ -881,25 +887,22 @@ def test_device_registration_hostname_comparison_case_insensitive(self):
         with different formats (colons, dashes, no separators)
         """
         mac_address = "00:11:22:33:aa:BB"
-        d = self._create_device_config(
+        device = self._create_device_config(
             device_opts={
                 "mac_address": mac_address,
                 "name": "configured-hostname",
                 "key": TEST_CONSISTENT_KEY,
             }
         )
-        params = {
-            "secret": TEST_ORG_SHARED_SECRET,
-            "name": "00-11-22-33-AA-bb",
-            "mac_address": mac_address,
-            "key": TEST_CONSISTENT_KEY,
-            "backend": "netjsonconfig.OpenWrt",
-        }
+        params = self._get_reregistration_payload(
+            device=device,
+            name="00-11-22-33-aa-bb",
+        )
         response = self.client.post(self.register_url, params)
         self.assertEqual(response.status_code, 201)
-        d.refresh_from_db()
+        device.refresh_from_db()
         # Hostname should not be changed
-        self.assertEqual(d.name, "configured-hostname")
+        self.assertEqual(device.name, "configured-hostname")
 
     def test_device_report_status_running(self):
         """

openwisp_controller/connection/apps.py

@@ -90,7 +90,6 @@ def _launch_update_config(cls, device):
 
         # Check if push update should be skipped
         if device.should_skip_push_update():
-            device.clear_skip_push_update()
             return
         update_config.delay(str(device.pk))
 

openwisp_controller/connection/tests/test_tasks.py

@@ -5,9 +5,9 @@
 
 from celery.exceptions import SoftTimeLimitExceeded
 from django.test import TestCase, TransactionTestCase
-from django.urls import reverse
 from swapper import load_model
 
+from ...config.tests.test_controller import TestControllerMixin
 from .. import tasks
 from .utils import CreateConnectionsMixin
 
@@ -89,28 +89,20 @@ def test_launch_command_exception(self, *args):
         self.assertEqual(command.output, "Internal system error: test error\n")
 
 
-class TestTransactionTasks(CreateConnectionsMixin, TransactionTestCase):
+class TestTransactionTasks(
+    TestControllerMixin, CreateConnectionsMixin, TransactionTestCase
+):
     @mock.patch.object(tasks.update_config, "delay")
     def test_update_config_hostname_changed_on_reregister(self, mocked_update_config):
-        org = self._get_org()
-        config_settings = OrganizationConfigSettings.objects.create(
-            organization=org, context={}, shared_secret="secret"
-        )
-        device = self._create_device(
-            name="AA-BB-CC-DD-EE-FF", organization=org, key="test-key"
-        )
-        self._create_config(device=device)
+        device = self._create_device_config()
         self._create_device_connection(device=device)
         # Trigger re-registration with new hostname
         response = self.client.post(
-            reverse("controller:device_register"),
-            {
-                "name": "new-hostname",
-                "secret": str(config_settings.shared_secret),
-                "key": str(device.key),
-                "mac_address": device.mac_address,
-                "backend": "netjsonconfig.OpenWrt",
-            },
+            self.register_url,
+            self._get_reregistration_payload(
+                device,
+                name="new-hostname",
+            ),
         )
         self.assertEqual(response.status_code, 201)
         mocked_update_config.assert_not_called()