[refactor] Improve queries, remove redundant check in DeviceChecksumView

Check in DeviceCheckSumView was redundant as the same check is executed
in trigger_whois_lookup task as well. Also we need estimated location
feature to work if WHOIS record already exists so this handles that
as well.

Signed-off-by: DragnEmperor <[email protected]>

Author: DragnEmperor

docs/user/estimated-location.rst

@@ -50,3 +50,7 @@ the device.
 If there is **no matching location**, a new estimated location is created
 or the existing one is updated using coordinates from the WHOIS record,
 but only if the existing location is estimated.
+
+If two devices share the same IP address and are assigned to the same
+location, and the last IP of one of the devices is updated, the system
+will create a new estimated location for that device.

openwisp_controller/config/controller/views.py

@@ -155,12 +155,9 @@ def get(self, request, pk):
             self.update_device_cache(device)
         # When update fields are present then save() will run the WHOIS
         # lookup. But if there are no update fields, we still want to
-        # trigger the WHOIS lookup if there is no record for the device's
-        # last_ip.
-        elif (
-            app_settings.WHOIS_CONFIGURED
-            and not device.whois_service.get_device_whois_info()
-        ):
+        # trigger the WHOIS lookup which checks if WHOIS lookup is required
+        # or not
+        elif app_settings.WHOIS_CONFIGURED:
             device.whois_service.trigger_whois_lookup()
         checksum_requested.send(
             sender=device.__class__, instance=device, request=request

openwisp_controller/config/whois/service.py

@@ -149,6 +149,6 @@ def trigger_whois_lookup(self):
         elif self._need_estimated_location_management(new_ip):
             transaction.on_commit(
                 lambda: manage_estimated_locations.delay(
-                    device_pk=self.device.pk, ip_address=new_ip, add_existing=True
+                    device_pk=self.device.pk, ip_address=new_ip
                 )
             )

openwisp_controller/config/whois/test_whois.py

@@ -347,12 +347,30 @@ def test_whois_task_called(self, mocked_lookup_task):
         with self.subTest(
             "WHOIS lookup task not called when last_ip has related WhoIsInfo"
         ):
+            device.organization.config_settings.whois_enabled = True
+            device.organization.config_settings.save()
             device.last_ip = "172.217.22.14"
             self._create_whois_info(ip_address=device.last_ip)
             device.save()
             mocked_lookup_task.assert_not_called()
         mocked_lookup_task.reset_mock()
 
+        with self.subTest(
+            "WHOIS lookup task not called via DeviceChecksumView when "
+            "last_ip has related WhoIsInfo"
+        ):
+            WHOISInfo.objects.all().delete()
+            self._create_whois_info(ip_address=device.last_ip)
+            self._create_config(device=device)
+            response = self.client.get(
+                reverse("controller:device_checksum", args=[device.pk]),
+                {"key": device.key},
+                REMOTE_ADDR=device.last_ip,
+            )
+            self.assertEqual(response.status_code, 200)
+            mocked_lookup_task.assert_not_called()
+        mocked_lookup_task.reset_mock()
+
     @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True)
     @mock.patch("openwisp_controller.config.whois.tasks.fetch_whois_details.delay")
     def test_whois_multiple_orgs(self, mocked_task):

openwisp_controller/geo/estimated_location/tasks.py

@@ -10,20 +10,22 @@
 
 
 @shared_task
-def manage_estimated_locations(device_pk, ip_address, add_existing=False):
+def manage_estimated_locations(device_pk, ip_address):
     """
     Creates/updates estimated location for a device based on the latitude and
-    longitude or attaches an existing location if `add_existing` is True.
+    longitude or attaches an existing location.
     Existing location here means a location of another device whose last_ip matches
     the given ip_address.
+    Does not alters the existing location if it is not estimated.
 
-    When `add_existing` is True:
-    - If the current device has no location, attach the existing one; if it does,
-    update it with the existing one only if it is estimated (fuzzy).
+    - If the current device has no location or location is estimate, either update
+    to an existing location; if it exists, else
 
-    When `add_existing` is False:
     - A new location is created if no location exists for current device, or
-    existing one is updated using coords from WHOIS record if it is estimated (fuzzy).
+    existing one is updated using coords from WHOIS record if it is estimated.
+
+    In case of multiple devices with same last_ip, the task will send a notification
+    to the user to resolve the conflict manually.
     """
     Device = load_model("config", "Device")
     Location = load_model("geo", "Location")
@@ -123,10 +125,14 @@ def _handle_attach_existing_location(
         )
 
     whois_obj = WHOISInfo.objects.filter(ip_address=ip_address).first()
-    device = Device.objects.get(pk=device_pk)
-    device_location, _ = DeviceLocation.objects.select_related(
-        "location"
-    ).get_or_create(content_object_id=device_pk)
+    device = (
+        Device.objects.select_related("devicelocation__location", "organization")
+        .only("organization_id", "devicelocation")
+        .get(pk=device_pk)
+    )
+
+    if not (device_location := getattr(device, "devicelocation", None)):
+        device_location = DeviceLocation(content_object=device)
 
     attached_devices_exists = False
     if current_location := device_location.location:
@@ -136,12 +142,7 @@ def _handle_attach_existing_location(
             .exists()
         )
 
-    if add_existing and (not current_location or current_location.is_estimated):
+    if not current_location or current_location.is_estimated:
         _handle_attach_existing_location(
             device, device_location, whois_obj, attached_devices_exists
         )
-
-    else:
-        _update_or_create_estimated_location(
-            device_location, whois_obj, attached_devices_exists
-        )

openwisp_controller/geo/estimated_location/test_estimated_location.py

@@ -16,6 +16,7 @@
 
 Device = load_model("config", "Device")
 Location = load_model("geo", "Location")
+WHOISInfo = load_model("config", "WHOISInfo")
 Notification = load_model("openwisp_notifications", "Notification")
 OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings")
 
@@ -143,6 +144,22 @@ def test_estimated_location_task_called(
             mocked_estimated_location_task.assert_called()
         mocked_estimated_location_task.reset_mock()
 
+        with self.subTest(
+            "Estimated location task not called via DeviceChecksumView when "
+            "last_ip has related WhoIsInfo"
+        ):
+            WHOISInfo.objects.all().delete()
+            self._create_whois_info(ip_address=device.last_ip)
+            self._create_config(device=device)
+            response = self.client.get(
+                reverse("controller:device_checksum", args=[device.pk]),
+                {"key": device.key},
+                REMOTE_ADDR=device.last_ip,
+            )
+            self.assertEqual(response.status_code, 200)
+            mocked_estimated_location_task.assert_called()
+        mocked_estimated_location_task.reset_mock()
+
     @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True)
     @mock.patch(_ESTIMATED_LOCATION_INFO_LOGGER)
     @mock.patch(_WHOIS_GEOIP_CLIENT)