[feature] Set location estimated flag to False on manual update #1036

Closes #1036

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

Author: DragnEmperor

docs/user/estimated-location.rst

@@ -66,3 +66,7 @@ includes indicators for the estimated status.
   <ip_address>)**.
 - A warning on top of the page.
 - **Is Estimated** field.
+
+Changes to the ``coordinates`` and ``geometry`` of the estimated location
+will set the ``is_estimated`` field to ``False`` and remove the
+"(Estimated Location)" suffix with IP from the location name.

openwisp_controller/config/whois/service.py

@@ -71,6 +71,15 @@ def get_org_config_settings(org_id):
             )
         return org_settings
 
+    @staticmethod
+    def check_estimate_location_configured(org_id):
+        if not org_id:
+            return False
+        if not app_settings.WHOIS_CONFIGURED:
+            return False
+        org_settings = WHOISService.get_org_config_settings(org_id=org_id)
+        return getattr(org_settings, "estimated_location_enabled")
+
     @property
     def is_whois_enabled(self):
         """
@@ -85,11 +94,7 @@ def is_estimated_location_enabled(self):
         Check if the Estimated location feature is enabled.
         """
         org_settings = self.get_org_config_settings(org_id=self.device.organization.pk)
-        return getattr(
-            org_settings,
-            "estimated_location_enabled",
-            app_settings.ESTIMATED_LOCATION_ENABLED,
-        )
+        return getattr(org_settings, "estimated_location_enabled")
 
     def _need_whois_lookup(self, new_ip):
         """

openwisp_controller/config/whois/test_whois.py

@@ -217,7 +217,6 @@ def test_whois_details_device_api(self):
         with self.subTest(
             "Device Detail API has whois_info when WHOIS_CONFIGURED is True"
         ):
-
             response = self.client.get(
                 reverse("config_api:device_detail", args=[device.pk])
             )

openwisp_controller/geo/admin.py

@@ -12,11 +12,11 @@
 )
 from swapper import load_model
 
+from openwisp_controller.config.whois.service import WHOISService
 from openwisp_users.multitenancy import MultitenantOrgFilter
 
 from ..admin import MultitenantAdminMixin
 from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdminExportable
-from .estimated_location.utils import check_estimate_location_configured
 from .exportable import GeoDeviceResource
 
 DeviceLocation = load_model("geo", "DeviceLocation")
@@ -103,24 +103,24 @@ class LocationAdmin(MultitenantAdminMixin, AbstractLocationAdmin):
 
     def get_fields(self, request, obj=None):
         fields = super().get_fields(request, obj)
-        if obj and not check_estimate_location_configured(obj.organization_id):
+        org_id = obj.organization_id if obj else None
+        if not WHOISService.check_estimate_location_configured(org_id):
             if "is_estimated" in fields:
                 fields.remove("is_estimated")
         return fields
 
     def get_readonly_fields(self, request, obj=None):
         fields = super().get_readonly_fields(request, obj)
-        if obj and check_estimate_location_configured(obj.organization_id):
+        org_id = obj.organization_id if obj else None
+        if obj and WHOISService.check_estimate_location_configured(org_id):
             fields = fields + ("is_estimated",)
         return fields
 
     def change_view(self, request, object_id, form_url="", extra_context=None):
         obj = self.get_object(request, object_id)
-        extra_context = {
-            "estimated_configured": check_estimate_location_configured(
-                obj.organization_id
-            )
-        }
+        org_id = obj.organization_id if obj else None
+        estimated_configured = WHOISService.check_estimate_location_configured(org_id)
+        extra_context = {"estimated_configured": estimated_configured}
         return super().change_view(request, object_id, form_url, extra_context)
 
 

openwisp_controller/geo/base/models.py

@@ -1,5 +1,8 @@
+import re
+
 from django.contrib.gis.db import models
 from django.core.exceptions import ValidationError
+from django.utils.translation import gettext
 from django.utils.translation import gettext_lazy as _
 from django_loci.base.models import (
     AbstractFloorPlan,
@@ -8,12 +11,13 @@
 )
 from swapper import get_model_name
 
+from openwisp_controller.config.whois.service import WHOISService
 from openwisp_users.mixins import OrgMixin, ValidateOrgMixin
 
-from ..estimated_location.utils import check_estimate_location_configured
-
 
 class BaseLocation(OrgMixin, AbstractLocation):
+    _changed_checked_fields = ["is_estimated", "address", "geometry"]
+
     is_estimated = models.BooleanField(
         default=False,
         help_text=_("Whether the location's coordinates are estimated."),
@@ -24,15 +28,25 @@ class Meta(AbstractLocation.Meta):
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
-        self._initial_is_estimated = self.is_estimated
+        self._set_initial_values_for_changed_checked_fields()
+
+    def _set_initial_values_for_changed_checked_fields(self):
+        deferred_fields = self.get_deferred_fields()
+        for field in self._changed_checked_fields:
+            if field in deferred_fields:
+                setattr(self, f"_initial_{field}", models.DEFERRED)
+            else:
+                setattr(self, f"_initial_{field}", getattr(self, field))
 
     def clean(self):
         # Raise validation error if `is_estimated` is True but estimated feature is
         # disabled.
         if (
             (self._state.adding or self._initial_is_estimated != self.is_estimated)
             and self.is_estimated
-            and not check_estimate_location_configured(self.organization_id)
+            and not WHOISService.check_estimate_location_configured(
+                self.organization_id
+            )
         ):
             raise ValidationError(
                 {
@@ -43,6 +57,35 @@ def clean(self):
             )
         return super().clean()
 
+    def save(self, *args, _set_estimated=False, **kwargs):
+        """
+        Save the location object with special handling for estimated locations.
+
+        Parameters:
+            _set_estimated: Boolean flag to indicate if this save is being performed
+            by the estimated location system. When False (default),
+            manual edits will clear the estimated status.
+            *args, **kwargs: Arguments passed to the parent save method.
+
+        Returns:
+            The result of the parent save method.
+        """
+        if WHOISService.check_estimate_location_configured(self.organization_id):
+            if not _set_estimated and (
+                self._initial_address != self.address
+                or self._initial_geometry != self.geometry
+            ):
+                self.is_estimated = False
+                estimated_string = gettext("Estimated Location")
+                if self.name and estimated_string in self.name:
+                    # remove string starting with "(Estimated Location"
+                    self.name = re.sub(
+                        rf"\s\({estimated_string}.*", "", self.name, flags=re.IGNORECASE
+                    )
+        else:
+            self.is_estimated = self._initial_is_estimated
+        return super().save(*args, **kwargs)
+
 
 class BaseFloorPlan(OrgMixin, AbstractFloorPlan):
     location = models.ForeignKey(get_model_name("geo", "Location"), models.CASCADE)

openwisp_controller/geo/estimated_location/tasks.py

@@ -36,7 +36,7 @@ def _create_estimated_location(device_location, location_defaults):
         with transaction.atomic():
             location = Location(**location_defaults, is_estimated=True)
             location.full_clean()
-            location.save()
+            location.save(_set_estimated=True)
             device_location.location = location
             device_location.full_clean()
             device_location.save()
@@ -72,7 +72,9 @@ def _update_or_create_estimated_location(
                         setattr(current_location, attr, value)
                         update_fields.append(attr)
                 if update_fields:
-                    current_location.save(update_fields=update_fields)
+                    current_location.save(
+                        update_fields=update_fields, _set_estimated=True
+                    )
                     logger.info(
                         f"Estimated location saved successfully for {device_pk}"
                         f" for IP: {ip_address}"

openwisp_controller/geo/estimated_location/test_estimated_location.py

@@ -294,7 +294,7 @@ def _verify_location_details(device, mocked_response):
             device.last_ip = "172.217.22.11"
             device.devicelocation.location.is_estimated = False
             mock_client.return_value.city.return_value = self._mocked_client_response()
-            device.devicelocation.location.save()
+            device.devicelocation.location.save(_set_estimated=True)
             device.save()
             device.refresh_from_db()
 
@@ -431,6 +431,7 @@ def _verify_notification(device, messages, notify_level="info"):
             for message in messages:
                 self.assertIn(message, notification.message)
             self.assertIn(device.last_ip, notification.rendered_description)
+            self.assertIn("#devicelocation-group", notification.target_url)
 
         with self.subTest("Test Notification for location create"):
             mocked_response = self._mocked_client_response()
@@ -468,3 +469,44 @@ def _verify_notification(device, messages, notify_level="info"):
             )
             messages = ["Unable to create estimated location for device"]
             _verify_notification(device3, messages, "error")
+
+    @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True)
+    @mock.patch(_WHOIS_GEOIP_CLIENT)
+    def test_estimate_location_status_remove(self, mock_client):
+        mocked_response = self._mocked_client_response()
+        mock_client.return_value.city.return_value = mocked_response
+        device = self._create_device(last_ip="172.217.22.10")
+        location = device.devicelocation.location
+        self.assertTrue(location.is_estimated)
+        org = self._get_org()
+
+        with self.subTest(
+            "Test Estimated Status unchanged if Estimated feature is disabled"
+        ):
+            org.config_settings.estimated_location_enabled = False
+            org.config_settings.save()
+            location.geometry = GEOSGeometry("POINT(12.512124 41.898903)", srid=4326)
+            location.save()
+            self.assertTrue(location.is_estimated)
+            self.assertIn(f"(Estimated Location: {device.last_ip})", location.name)
+
+        with self.subTest(
+            "Test Estimated Status unchanged if Estimated feature is enabled"
+            " and desired fields not changed"
+        ):
+            org.config_settings.estimated_location_enabled = True
+            org.config_settings.save()
+            location._set_initial_values_for_changed_checked_fields()
+            location.type = "outdoor"
+            location.is_mobile = True
+            location.save()
+            self.assertTrue(location.is_estimated)
+
+        with self.subTest(
+            "Test Estimated Status changed if Estimated feature is enabled"
+            " and desired fields changed"
+        ):
+            location.geometry = GEOSGeometry("POINT(15.512124 45.898903)", srid=4326)
+            location.save()
+            self.assertFalse(location.is_estimated)
+            self.assertNotIn(f"(Estimated Location: {device.last_ip})", location.name)

openwisp_controller/geo/estimated_location/utils.py

@@ -1,21 +1,5 @@
 from openwisp_notifications.utils import _get_object_link
 
-from openwisp_controller.config import settings as config_app_settings
-from openwisp_controller.config.whois.service import WHOISService
-
-
-def check_estimate_location_configured(org_id):
-    if not org_id:
-        return False
-    if not config_app_settings.WHOIS_CONFIGURED:
-        return False
-    org_settings = WHOISService.get_org_config_settings(org_id=org_id)
-    return getattr(
-        org_settings,
-        "estimated_location_enabled",
-        config_app_settings.ESTIMATED_LOCATION_ENABLED,
-    )
-
 
 def get_device_location_notification_target_url(obj, field, absolute_url=True):
     url = _get_object_link(obj._related_object(field), absolute_url)