[chores] Minor improvements and nitpicks suggested by @coderabbitai

Author: nemesifier

openwisp_controller/config/base/whois.py

@@ -82,7 +82,6 @@ def clean(self):
                 raise ValidationError(
                     {"cidr": _("Invalid CIDR format: %(error)s") % {"error": str(e)}}
                 )
-
         if self.coordinates:
             if not (-90 <= self.coordinates.y <= 90):
                 raise ValidationError(
@@ -105,7 +104,6 @@ def device_whois_info_delete_handler(instance, **kwargs):
         when device is deleted.
         """
         Device = load_model("config", "Device")
-
         last_ip = instance.last_ip
         existing_devices = Device.objects.filter(_is_deactivated=False).filter(
             last_ip=last_ip

openwisp_controller/config/management/commands/clear_last_ip.py

@@ -5,7 +5,7 @@
 
 class Command(BaseCommand):
     help = (
-        "Clears the last IP address (if set) for every active device"
+        "Clears the last IP address (if set) for active devices without WHOIS records"
         " across all organizations."
     )
 
@@ -19,7 +19,7 @@ def add_arguments(self, parser):
         )
         return super().add_arguments(parser)
 
-    def handle(self, *args, **options):
+    def handle(self, *_args, **options):
         Device = load_model("config", "Device")
         WHOISInfo = load_model("config", "WHOISInfo")
 

openwisp_controller/config/whois/service.py

@@ -174,9 +174,9 @@ def process_whois_details(self, ip_address):
             message = EXCEPTION_MESSAGES.get(exc_type)
             if exc_type is errors.AddressNotFoundError:
                 message = message.format(ip_address=ip_address)
-            raise exc_type(message)
-        except requests.RequestException as e:
-            raise e
+            raise exc_type(message) from e
+        except requests.RequestException:
+            raise
         else:
             # The attributes are always present in the response,
             # but they can be None, so added fallbacks.
@@ -284,7 +284,6 @@ def _create_or_update_whois(self, whois_details, whois_instance=None):
         Returns the updated or created WHOIS instance along with update fields.
         """
         WHOISInfo = load_model("config", "WHOISInfo")
-
         update_fields = []
         if whois_instance:
             for attr, value in whois_details.items():

openwisp_controller/config/whois/tests/tests.py

@@ -155,14 +155,11 @@ def test_whois_enabled(self):
                 app_settings, "WHOIS_CONFIGURED", False
             ), self.assertRaises(ValidationError) as context_manager:
                 org_settings_obj.full_clean()
-            try:
-                self.assertEqual(
-                    context_manager.exception.message_dict["whois_enabled"][0],
-                    "WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY must be set "
-                    + "before enabling WHOIS feature.",
-                )
-            except AssertionError:
-                self.fail("ValidationError message not equal to expected message.")
+            self.assertEqual(
+                context_manager.exception.message_dict["whois_enabled"][0],
+                "WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY must be set "
+                + "before enabling WHOIS feature.",
+            )
 
         with mock.patch.object(app_settings, "WHOIS_CONFIGURED", True):
             org_settings_obj.full_clean()
@@ -173,13 +170,13 @@ def test_whois_enabled(self):
             org_settings_obj.whois_enabled = True
             org_settings_obj.save(update_fields=["whois_enabled"])
             org_settings_obj.refresh_from_db(fields=["whois_enabled"])
-            self.assertEqual(getattr(org_settings_obj, "whois_enabled"), True)
+            self.assertTrue(org_settings_obj.whois_enabled)
 
         with self.subTest("Test setting WHOIS enabled to False"):
             org_settings_obj.whois_enabled = False
             org_settings_obj.save(update_fields=["whois_enabled"])
             org_settings_obj.refresh_from_db(fields=["whois_enabled"])
-            self.assertEqual(getattr(org_settings_obj, "whois_enabled"), False)
+            self.assertFalse(org_settings_obj.whois_enabled)
 
         with self.subTest(
             "Test setting WHOIS enabled to None fallbacks to global setting"
@@ -190,7 +187,7 @@ def test_whois_enabled(self):
             org_settings_obj.save(update_fields=["whois_enabled"])
             org_settings_obj.refresh_from_db(fields=["whois_enabled"])
             self.assertEqual(
-                getattr(org_settings_obj, "whois_enabled"),
+                org_settings_obj.whois_enabled,
                 app_settings.WHOIS_ENABLED,
             )
 
@@ -335,28 +332,22 @@ def test_whois_model_fields_validation(self):
 
         with self.assertRaises(ValidationError) as context_manager:
             self._create_whois_info(ip_address="127.0.0.1")
-        try:
-            self.assertEqual(
-                context_manager.exception.message_dict["ip_address"][0],
-                "WHOIS information cannot be created for private IP addresses.",
-            )
-        except AssertionError:
-            self.fail("ValidationError message not equal to expected message.")
+        self.assertEqual(
+            context_manager.exception.message_dict["ip_address"][0],
+            "WHOIS information cannot be created for private IP addresses.",
+        )
 
         with self.assertRaises(ValidationError):
             self._create_whois_info(timezone="a" * 36)
 
         with self.assertRaises(ValidationError) as context_manager:
             self._create_whois_info(cidr="InvalidCIDR")
-        try:
-            # Not using assertEqual here because we are adding error message raised by
-            # ipaddress module to the ValidationError message.
-            self.assertIn(
-                "Invalid CIDR format: 'InvalidCIDR'",
-                context_manager.exception.message_dict["cidr"][0],
-            )
-        except AssertionError:
-            self.fail("ValidationError message not equal to expected message.")
+        # Not using assertEqual here because we are adding error message raised by
+        # ipaddress module to the ValidationError message.
+        self.assertIn(
+            "Invalid CIDR format: 'InvalidCIDR'",
+            context_manager.exception.message_dict["cidr"][0],
+        )
 
         with self.assertRaises(ValidationError):
             self._create_whois_info(asn="InvalidASNNumber")
@@ -372,13 +363,10 @@ def test_whois_model_fields_validation(self):
             with self.assertRaises(ValidationError) as context_manager:
                 point = Point(longitude, latitude, srid=4326)
                 self._create_whois_info(coordinates=point)
-            try:
-                self.assertEqual(
-                    context_manager.exception.message_dict["coordinates"][0],
-                    expected_msg,
-                )
-            except AssertionError:
-                self.fail("ValidationError message not equal to expected message.")
+            self.assertEqual(
+                context_manager.exception.message_dict["coordinates"][0],
+                expected_msg,
+            )
 
 
 class TestWHOISTransaction(

openwisp_controller/config/whois/tests/utils.py

@@ -130,7 +130,8 @@ def _task_called(self, mocked_task, task_name="WHOIS lookup"):
         mocked_task.reset_mock()
 
         with self.subTest(
-            f"{task_name} task not called via DeviceChecksumView for no WHOIS record"
+            f"{task_name} task not called via DeviceChecksumView "
+            "if no WHOIS record and IP unchanged"
         ):
             WHOISInfo.objects.all().delete()
             device.refresh_from_db()

openwisp_controller/geo/base/models.py

@@ -96,7 +96,9 @@ def save(self, *args, _set_estimated=False, **kwargs):
             changed_fields = {"is_estimated"}
         if update_fields := kwargs.get("update_fields"):
             kwargs["update_fields"] = set(update_fields) | changed_fields
-        return super().save(*args, **kwargs)
+        result = super().save(*args, **kwargs)
+        self._set_initial_values_for_changed_checked_fields()
+        return result
 
 
 class BaseFloorPlan(OrgMixin, AbstractFloorPlan):

openwisp_controller/geo/estimated_location/tests/tests.py

@@ -59,16 +59,11 @@ def test_estimated_location_configuration_setting(self):
                 org_settings_obj.whois_enabled = False
                 org_settings_obj.estimated_location_enabled = True
                 org_settings_obj.full_clean()
-            try:
-                self.assertEqual(
-                    context_manager.exception.message_dict[
-                        "estimated_location_enabled"
-                    ][0],
-                    "Estimated Location feature requires "
-                    + "WHOIS Lookup feature to be enabled.",
-                )
-            except AssertionError:
-                self.fail("ValidationError message not equal to expected message.")
+            self.assertEqual(
+                context_manager.exception.message_dict["estimated_location_enabled"][0],
+                "Estimated Location feature requires "
+                + "WHOIS Lookup feature to be enabled.",
+            )
 
         with self.subTest(
             "Test Estimated Location field visible on admin when "
@@ -116,13 +111,10 @@ def test_estimated_location_field(self):
         org.refresh_from_db()
         with self.assertRaises(ValidationError) as context_manager:
             self._create_location(organization=org, is_estimated=True)
-        try:
-            self.assertEqual(
-                context_manager.exception.message_dict["is_estimated"][0],
-                "Estimated Location feature required to be configured.",
-            )
-        except AssertionError:
-            self.fail("ValidationError message not equal to expected message.")
+        self.assertEqual(
+            context_manager.exception.message_dict["is_estimated"][0],
+            "Estimated Location feature required to be configured.",
+        )
 
     @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True)
     def test_estimated_location_admin(self):
@@ -865,7 +857,8 @@ def test_estimated_location_filter_admin(self):
                 " when WHOIS not configured"
             ):
                 for i in ["estimated", "outdoor", "indoor"]:
-                    response = self.client.get(path, {"with_geo": i})
-                    self.assertContains(response, estimated_device.id)
-                    self.assertContains(response, outdoor_device.id)
-                    self.assertContains(response, indoor_device.id)
+                    with self.subTest(filter_value=i):
+                        response = self.client.get(path, {"with_geo": i})
+                        self.assertContains(response, estimated_device.id)
+                        self.assertContains(response, outdoor_device.id)
+                        self.assertContains(response, indoor_device.id)