From 7011f10a04fea77cef56dc8ef6cb3b5996965485 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Tue, 24 Mar 2026 00:54:52 +0530 Subject: [PATCH 01/10] [chores:bug] Separated openwisp_controller.geo from openwisp_controller.config #1224 Closes #1224 --- docs/developer/extending.rst | 7 +- docs/user/rest-api.rst | 28 ++ openwisp_controller/config/admin.py | 8 +- .../config/base/multitenancy.py | 14 - openwisp_controller/config/base/whois.py | 13 - ...ngs_estimated_location_enabled_and_more.py | 20 ++ openwisp_controller/config/settings.py | 13 +- openwisp_controller/config/tests/test_api.py | 2 - openwisp_controller/config/whois/service.py | 103 +----- openwisp_controller/config/whois/tasks.py | 17 - openwisp_controller/geo/admin.py | 37 ++- openwisp_controller/geo/api/serializers.py | 11 +- openwisp_controller/geo/api/views.py | 15 + openwisp_controller/geo/apps.py | 25 +- openwisp_controller/geo/base/models.py | 58 +++- .../geo/estimated_location/handlers.py | 21 ++ .../geo/estimated_location/mixins.py | 10 +- .../geo/estimated_location/service.py | 98 ++++++ .../geo/estimated_location/tasks.py | 13 +- .../geo/estimated_location/tests/tests.py | 147 +++++---- .../geo/estimated_location/tests/utils.py | 9 +- .../geo/estimated_location/utils.py | 19 ++ .../0005_organizationgeosettings.py | 62 ++++ ...6_create_geo_settings_for_existing_orgs.py | 36 +++ .../geo/migrations/__init__.py | 17 +- openwisp_controller/geo/models.py | 13 +- openwisp_controller/geo/settings.py | 18 ++ openwisp_controller/geo/tests/test_api.py | 304 ++++++++++++++++++ openwisp_controller/geo/utils.py | 5 + openwisp_controller/settings.py | 7 +- tests/openwisp2/sample_geo/models.py | 7 + tests/openwisp2/settings.py | 1 + 32 files changed, 901 insertions(+), 257 deletions(-) create mode 100644 openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py create mode 100644 openwisp_controller/geo/estimated_location/service.py create mode 100644 openwisp_controller/geo/migrations/0005_organizationgeosettings.py create mode 100644 openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py create mode 100644 openwisp_controller/geo/settings.py diff --git a/docs/developer/extending.rst b/docs/developer/extending.rst index eda5c0b05..ce8d74706 100644 --- a/docs/developer/extending.rst +++ b/docs/developer/extending.rst @@ -350,6 +350,7 @@ Once you have created the models, add the following to your GEO_LOCATION_MODEL = "sample_geo.Location" GEO_FLOORPLAN_MODEL = "sample_geo.FloorPlan" GEO_DEVICELOCATION_MODEL = "sample_geo.DeviceLocation" + GEO_ORGANIZATIONGEOSETTINGS_MODEL = "sample_geo.OrganizationGeoSettings" CONNECTION_CREDENTIALS_MODEL = "sample_connection.Credentials" CONNECTION_DEVICECONNECTION_MODEL = "sample_connection.DeviceConnection" CONNECTION_COMMAND_MODEL = "sample_connection.Command" @@ -456,7 +457,11 @@ For example: .. code-block:: python - from openwisp_controller.geo.admin import FloorPlanAdmin, LocationAdmin + from openwisp_controller.geo.admin import ( + FloorPlanAdmin, + LocationAdmin, + GeoSettingsInline, + ) FloorPlanAdmin.fields += ["example"] # <-- monkey patching example diff --git a/docs/user/rest-api.rst b/docs/user/rest-api.rst index 2ec0b48ed..30387172e 100644 --- a/docs/user/rest-api.rst +++ b/docs/user/rest-api.rst @@ -801,6 +801,34 @@ device is updating it's position. }, }' +Get Organization Geographic Settings +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +.. code-block:: text + + GET /api/v1/controller/organization/{organization_pk}/geo-settings/ + +This endpoint allows retrieving geographic settings for a specific +organization. + +Update Organization Geographic Settings +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This endpoint allows updating geographic settings for a specific +organization. + +.. code-block:: text + + PUT /api/v1/controller/organization/{organization_pk}/geo-settings/ + +.. code-block:: text + + curl -X PUT \ + 'http://127.0.0.1:8000/api/v1/controller/organization/8a85cc23-bad5-4c7e-b9f4-ffe298defb5c/geo-settings/' \ + -H 'authorization: Bearer ' \ + -H 'content-type: application/json' \ + -d '{"estimated_location_enabled": true}' + List Locations ~~~~~~~~~~~~~~ diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 7c328a353..bae451912 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -694,8 +694,7 @@ def change_group(self, request, queryset): "action_checkbox_name": helpers.ACTION_CHECKBOX_NAME, "opts": self.model._meta, "changelist_url": ( - f"{request.resolver_match.app_name}:" - f"{request.resolver_match.url_name}" + f"{request.resolver_match.app_name}:{request.resolver_match.url_name}" ), } @@ -1104,8 +1103,7 @@ def save_clones(view, user, queryset, organization=None): validated_org = Organization.objects.get(pk=organization) except (ValidationError, Organization.DoesNotExist) as e: logger.warning( - "Detected tampering in clone template " - f"form by user {user}: {e}" + f"Detected tampering in clone template form by user {user}: {e}" ) return if not user.is_superuser and not user.is_manager(organization): @@ -1393,7 +1391,7 @@ def get_fields(self, request, obj=None): if app_settings.REGISTRATION_ENABLED: fields += ["registration_enabled", "shared_secret"] if app_settings.WHOIS_CONFIGURED: - fields += ["whois_enabled", "estimated_location_enabled"] + fields += ["whois_enabled"] fields += ["context"] return fields diff --git a/openwisp_controller/config/base/multitenancy.py b/openwisp_controller/config/base/multitenancy.py index c2434e413..07c34bb68 100644 --- a/openwisp_controller/config/base/multitenancy.py +++ b/openwisp_controller/config/base/multitenancy.py @@ -39,11 +39,6 @@ class AbstractOrganizationConfigSettings(UUIDModel): fallback=app_settings.WHOIS_ENABLED, verbose_name=_("WHOIS Enabled"), ) - estimated_location_enabled = FallbackBooleanChoiceField( - help_text=_("Whether the estimated location feature is enabled"), - fallback=app_settings.ESTIMATED_LOCATION_ENABLED, - verbose_name=_("Estimated Location Enabled"), - ) context = JSONField( blank=True, default=dict, @@ -77,15 +72,6 @@ def clean(self): ) } ) - if not self.whois_enabled and self.estimated_location_enabled: - raise ValidationError( - { - "estimated_location_enabled": _( - "Estimated Location feature requires " - "WHOIS Lookup feature to be enabled." - ) - } - ) return super().clean() def save( diff --git a/openwisp_controller/config/base/whois.py b/openwisp_controller/config/base/whois.py index 6a2abac38..10a8f5b3c 100644 --- a/openwisp_controller/config/base/whois.py +++ b/openwisp_controller/config/base/whois.py @@ -150,16 +150,3 @@ def _location_name(self): } # Use named placeholder for consistency return _("Estimated Location: %(ip)s") % {"ip": self.ip_address} - - def _get_defaults_for_estimated_location(self): - """ - Used to get default values for creating or updating - an estimated location based on the WHOIS information. - """ - return { - "name": self._location_name, - "type": "outdoor", - "is_mobile": False, - "geometry": self.coordinates, - "address": self.formatted_address, - } diff --git a/openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py b/openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py new file mode 100644 index 000000000..426b2e51e --- /dev/null +++ b/openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py @@ -0,0 +1,20 @@ +# Generated by Django 5.2.12 on 2026-03-23 14:59 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ( + "config", + "0063_organizationconfigsettings_estimated_location_enabled_and_more", + ), + ] + + operations = [ + migrations.RemoveField( + model_name="organizationconfigsettings", + name="estimated_location_enabled", + ), + ] diff --git a/openwisp_controller/config/settings.py b/openwisp_controller/config/settings.py index 6c591c489..55ad86ce3 100644 --- a/openwisp_controller/config/settings.py +++ b/openwisp_controller/config/settings.py @@ -1,14 +1,11 @@ import logging -from django.conf import settings from django.core.exceptions import ImproperlyConfigured from django.utils.translation import gettext_lazy as _ -logger = logging.getLogger(__name__) - +from ..settings import get_setting -def get_setting(option, default): - return getattr(settings, f"OPENWISP_CONTROLLER_{option}", default) +logger = logging.getLogger(__name__) BACKENDS = get_setting( @@ -85,9 +82,3 @@ def get_setting(option, default): raise ImproperlyConfigured( "OPENWISP_CONTROLLER_WHOIS_REFRESH_THRESHOLD_DAYS must be a positive integer" ) -ESTIMATED_LOCATION_ENABLED = get_setting("ESTIMATED_LOCATION_ENABLED", False) -if ESTIMATED_LOCATION_ENABLED and not WHOIS_ENABLED: - raise ImproperlyConfigured( - "OPENWISP_CONTROLLER_WHOIS_ENABLED must be set to True before " - "setting OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED to True." - ) diff --git a/openwisp_controller/config/tests/test_api.py b/openwisp_controller/config/tests/test_api.py index fe12fe2cf..516b67fc3 100644 --- a/openwisp_controller/config/tests/test_api.py +++ b/openwisp_controller/config/tests/test_api.py @@ -29,8 +29,6 @@ Device = load_model("config", "Device") Config = load_model("config", "Config") DeviceGroup = load_model("config", "DeviceGroup") -DeviceLocation = load_model("geo", "DeviceLocation") -Location = load_model("geo", "Location") OrganizationUser = load_model("openwisp_users", "OrganizationUser") diff --git a/openwisp_controller/config/whois/service.py b/openwisp_controller/config/whois/service.py index d6f72d016..663833516 100644 --- a/openwisp_controller/config/whois/service.py +++ b/openwisp_controller/config/whois/service.py @@ -2,7 +2,6 @@ from ipaddress import ip_address as ip_addr import requests -from celery import current_app from django.contrib.gis.geos import Point from django.core.cache import cache from django.db import transaction @@ -14,7 +13,7 @@ from openwisp_controller.config import settings as app_settings from .tasks import fetch_whois_details -from .utils import EXCEPTION_MESSAGES, send_whois_task_notification +from .utils import EXCEPTION_MESSAGES class WHOISService: @@ -111,15 +110,6 @@ def get_org_config_settings(org_id): ) return org_settings - @staticmethod - def check_estimated_location_enabled(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 org_settings.estimated_location_enabled - @property def is_whois_enabled(self): """ @@ -130,16 +120,6 @@ def is_whois_enabled(self): org_settings = self.get_org_config_settings(org_id=self.device.organization.pk) return org_settings.whois_enabled - @property - def is_estimated_location_enabled(self): - """ - Check if the Estimated location feature is enabled. - """ - if not app_settings.WHOIS_CONFIGURED: - return False - org_settings = self.get_org_config_settings(org_id=self.device.organization.pk) - return org_settings.estimated_location_enabled - def process_whois_details(self, ip_address): """ Fetch WHOIS details for a given IP address and return only @@ -214,26 +194,6 @@ def _need_whois_lookup(self, new_ip): return False return True - def _need_estimated_location_management(self, new_ip): - """ - Used to determine if Estimated locations need to be created/updated - or not during WHOIS lookup. - """ - if not self.is_whois_enabled: - return False - if not self.is_valid_public_ip_address(new_ip): - return False - if not self.is_estimated_location_enabled: - return False - return True - - def trigger_estimated_location_task(self, ip_address): - """Helper method to trigger the estimated location task.""" - current_app.send_task( - "whois_estimated_location_task", - kwargs={"device_pk": self.device.pk, "ip_address": ip_address}, - ) - def get_device_whois_info(self): """ If the WHOIS lookup feature is enabled and the device ``last_ip`` @@ -246,9 +206,7 @@ def get_device_whois_info(self): def process_ip_data_and_location(self, force_lookup=False): """ - Trigger WHOIS lookup based on the conditions of `_need_whois_lookup` - and also manage estimated locations based on the conditions of - `_need_estimated_location_management`. + Trigger WHOIS lookup based on the conditions of `_need_whois_lookup`. Tasks are triggered on commit to ensure redundant data is not created. """ new_ip = self.device.last_ip @@ -260,15 +218,6 @@ def process_ip_data_and_location(self, force_lookup=False): initial_ip_address=initial_ip, ) ) - # To handle the case when WHOIS already exists as in that case - # WHOIS lookup is not triggered but we still need to - # manage estimated locations. - elif self._need_estimated_location_management(new_ip): - transaction.on_commit( - lambda: self.trigger_estimated_location_task( - ip_address=new_ip, - ) - ) def update_whois_info(self): """ @@ -313,51 +262,3 @@ def _create_or_update_whois(self, whois_details, whois_instance=None): whois_instance.full_clean() whois_instance.save(force_insert=True) return whois_instance, update_fields - - def _create_or_update_estimated_location( - self, location_defaults, attached_devices_exists - ): - """ - Create or update estimated location for the device based on the - given location defaults. - """ - Location = load_model("geo", "Location") - DeviceLocation = load_model("geo", "DeviceLocation") - - if not (device_location := getattr(self.device, "devicelocation", None)): - device_location = DeviceLocation(content_object=self.device) - - current_location = device_location.location - - if not current_location or ( - attached_devices_exists and current_location.is_estimated - ): - with transaction.atomic(): - current_location = Location(**location_defaults, is_estimated=True) - current_location.full_clean() - current_location.save(_set_estimated=True) - device_location.location = current_location - device_location.full_clean() - device_location.save() - send_whois_task_notification( - device=self.device, - notify_type="estimated_location_created", - actor=current_location, - ) - elif current_location.is_estimated: - update_fields = [] - for attr, value in location_defaults.items(): - if getattr(current_location, attr) != value: - setattr(current_location, attr, value) - update_fields.append(attr) - if update_fields: - with transaction.atomic(): - current_location.save( - update_fields=update_fields, _set_estimated=True - ) - send_whois_task_notification( - device=self.device, - notify_type="estimated_location_updated", - actor=current_location, - ) - return current_location diff --git a/openwisp_controller/config/whois/tasks.py b/openwisp_controller/config/whois/tasks.py index 78955600e..fbfd32b7f 100644 --- a/openwisp_controller/config/whois/tasks.py +++ b/openwisp_controller/config/whois/tasks.py @@ -90,23 +90,6 @@ def fetch_whois_details(self, device_pk, initial_ip_address): # execute synchronously as we're already in a background task lambda: delete_whois_record(ip_address=initial_ip_address) ) - if not device._get_organization__config_settings().estimated_location_enabled: - return - # the estimated location task should not run if old record is updated - # and location related fields are not updated - device_location = getattr(device, "devicelocation", None) - if ( - device_location - and device_location.location - and update_fields - and not any(i in update_fields for i in ["address", "coordinates"]) - ): - return - transaction.on_commit( - lambda: whois_service.trigger_estimated_location_task( - ip_address=new_ip_address, - ) - ) @shared_task diff --git a/openwisp_controller/geo/admin.py b/openwisp_controller/geo/admin.py index f08960b8c..780fdb322 100644 --- a/openwisp_controller/geo/admin.py +++ b/openwisp_controller/geo/admin.py @@ -13,16 +13,22 @@ from swapper import load_model from openwisp_controller.config import settings as config_app_settings -from openwisp_controller.config.whois.service import WHOISService +from openwisp_users.admin import OrganizationAdmin from openwisp_users.multitenancy import MultitenantOrgFilter from ..admin import MultitenantAdminMixin -from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdminExportable +from ..config.admin import ( + DeactivatedDeviceReadOnlyMixin, + DeviceAdminExportable, + OrganizationLimitsInline, +) +from .estimated_location.service import EstimatedLocationService from .exportable import GeoDeviceResource DeviceLocation = load_model("geo", "DeviceLocation") FloorPlan = load_model("geo", "FloorPlan") Location = load_model("geo", "Location") +OrganizationGeoSettings = load_model("geo", "OrganizationGeoSettings") class FloorPlanForm(AbstractFloorPlanForm): @@ -121,7 +127,7 @@ def get_fields(self, request, obj=None): fields = list(super().get_fields(request, obj)) # makes a copy # do not show the is_estimated field on add pages # or if the estimated location feature is not enabled for the organization - if not obj or not WHOISService.check_estimated_location_enabled( + if not obj or not EstimatedLocationService.check_estimated_location_enabled( obj.organization_id ): fields.remove("is_estimated") @@ -129,14 +135,18 @@ def get_fields(self, request, obj=None): def get_readonly_fields(self, request, obj=None): fields = super().get_readonly_fields(request, obj) - if obj and WHOISService.check_estimated_location_enabled(obj.organization_id): + if obj and EstimatedLocationService.check_estimated_location_enabled( + obj.organization_id + ): fields = ("is_estimated",) + fields # creates a new tuple return fields def change_view(self, request, object_id, form_url="", extra_context=None): obj = self.get_object(request, object_id) org_id = obj.organization_id if obj else None - estimated_enabled = WHOISService.check_estimated_location_enabled(org_id) + estimated_enabled = EstimatedLocationService.check_estimated_location_enabled( + org_id + ) extra_context = extra_context or {} extra_context["estimated_enabled"] = estimated_enabled return super().change_view(request, object_id, form_url, extra_context) @@ -199,6 +209,23 @@ def queryset(self, request, queryset): return queryset.filter(devicelocation__isnull=self.value() == "false") +class GeoSettingsInline(admin.StackedInline): + model = OrganizationGeoSettings + + def get_readonly_fields(self, request, obj=None): + fields = list(super().get_readonly_fields(request, obj)) + if ( + "estimated_location_enabled" in fields + and not config_app_settings.WHOIS_CONFIGURED + ): + fields = ["estimated_location_enabled"] + fields + return fields + + +OrganizationAdmin.inlines.insert( + OrganizationAdmin.inlines.index(OrganizationLimitsInline) + 1, + GeoSettingsInline, +) # Prepend DeviceLocationInline to config.DeviceAdminExportable DeviceAdminExportable.inlines.insert(1, DeviceLocationInline) DeviceAdminExportable.list_filter.append(DeviceLocationFilter) diff --git a/openwisp_controller/geo/api/serializers.py b/openwisp_controller/geo/api/serializers.py index 3d7cc61e3..4d60d7a1e 100644 --- a/openwisp_controller/geo/api/serializers.py +++ b/openwisp_controller/geo/api/serializers.py @@ -20,6 +20,14 @@ Location = load_model("geo", "Location") DeviceLocation = load_model("geo", "DeviceLocation") FloorPlan = load_model("geo", "FloorPlan") +OrganizationGeoSettings = load_model("geo", "OrganizationGeoSettings") + + +class OrganizationGeoSettingsSerializer(ValidatedModelSerializer): + class Meta: + model = OrganizationGeoSettings + fields = "__all__" + read_only_fields = ["id", "organization"] class LocationDeviceSerializer(ValidatedModelSerializer): @@ -156,8 +164,7 @@ def validate(self, data): raise serializers.ValidationError( { "type": _( - "Floorplan can only be added with location of " - "the type indoor" + "Floorplan can only be added with location of the type indoor" ) } ) diff --git a/openwisp_controller/geo/api/views.py b/openwisp_controller/geo/api/views.py index ae87c34df..b3825055c 100644 --- a/openwisp_controller/geo/api/views.py +++ b/openwisp_controller/geo/api/views.py @@ -30,12 +30,14 @@ IndoorCoordinatesSerializer, LocationDeviceSerializer, LocationSerializer, + OrganizationGeoSettingsSerializer, ) Device = load_model("config", "Device") Location = load_model("geo", "Location") DeviceLocation = load_model("geo", "DeviceLocation") FloorPlan = load_model("geo", "FloorPlan") +OrganizationGeoSettings = load_model("geo", "OrganizationGeoSettings") class DevicePermission(BasePermission): @@ -344,6 +346,18 @@ class LocationDetailView( queryset = Location.objects.all() +class OrganizationGeoSettingsView(ProtectedAPIMixin, generics.RetrieveUpdateAPIView): + serializer_class = OrganizationGeoSettingsSerializer + queryset = OrganizationGeoSettings.objects.all() + + def get_object(self): + org_id = self.kwargs.get("organization_pk") + try: + return self.get_queryset().get(organization_id=org_id) + except OrganizationGeoSettings.DoesNotExist: + raise Http404(_("Geo settings not found for this organization.")) + + # add with_geo filter to device API DeviceListCreateView.filterset_class = DeviceListFilter @@ -356,3 +370,4 @@ class LocationDetailView( indoor_coordinates_list = IndoorCoordinatesList.as_view() list_location = LocationListCreateView.as_view() detail_location = LocationDetailView.as_view() +organization_geo_settings = OrganizationGeoSettingsView.as_view() diff --git a/openwisp_controller/geo/apps.py b/openwisp_controller/geo/apps.py index 0e2cc6264..d77006d5d 100644 --- a/openwisp_controller/geo/apps.py +++ b/openwisp_controller/geo/apps.py @@ -14,7 +14,11 @@ from openwisp_utils.admin_theme import register_dashboard_chart from openwisp_utils.admin_theme.menu import register_menu_group -from .estimated_location.handlers import register_estimated_location_notification_types +from ..config import settings as config_app_settings +from .estimated_location.handlers import ( + register_estimated_location_notification_types, + whois_info_post_save_handler, +) class GeoConfig(LociConfig): @@ -24,15 +28,34 @@ class GeoConfig(LociConfig): def __setmodels__(self): self.location_model = swapper.load_model("geo", "Location") + self.org_geo_settings_model = swapper.load_model( + "geo", "OrganizationGeoSettings" + ) + self.whois_info_model = swapper.load_model("config", "WHOISInfo") + self.organization_model = swapper.load_model("openwisp_users", "Organization") def ready(self): super().ready() self.register_dashboard_charts() self.register_menu_groups() + self.connect_receivers() register_estimated_location_notification_types() if getattr(settings, "TESTING", False): self._add_params_to_test_config() + def connect_receivers(self): + post_save.connect( + self.org_geo_settings_model.organization_post_save_receiver, + sender=self.organization_model, + dispatch_uid="organization_geo_settings_post_save", + ) + if config_app_settings.WHOIS_CONFIGURED: + post_save.connect( + whois_info_post_save_handler, + sender=self.whois_info_model, + dispatch_uid="whois_info_estimated_location_handler", + ) + def _add_params_to_test_config(self): """ this methods adds the management fields of DeviceLocationInline diff --git a/openwisp_controller/geo/base/models.py b/openwisp_controller/geo/base/models.py index c26aca6cb..ea23f994f 100644 --- a/openwisp_controller/geo/base/models.py +++ b/openwisp_controller/geo/base/models.py @@ -1,5 +1,6 @@ from typing import ClassVar +import swapper from django.contrib.gis.db import models from django.core.exceptions import ValidationError from django.utils.translation import gettext_lazy as _ @@ -10,8 +11,12 @@ ) from swapper import get_model_name -from openwisp_controller.config.whois.service import WHOISService +from openwisp_controller.config import settings as config_settings +from openwisp_controller.geo import settings as geo_settings +from openwisp_controller.geo.estimated_location.service import EstimatedLocationService from openwisp_users.mixins import OrgMixin, ValidateOrgMixin +from openwisp_utils.base import UUIDModel +from openwisp_utils.fields import FallbackBooleanChoiceField class BaseLocation(OrgMixin, AbstractLocation): @@ -52,7 +57,9 @@ def clean(self): if ( (self._state.adding or estimated_status_changed) and self.is_estimated - and not WHOISService.check_estimated_location_enabled(self.organization_id) + and not EstimatedLocationService.check_estimated_location_enabled( + self.organization_id + ) ): raise ValidationError( { @@ -78,7 +85,9 @@ def save(self, *args, _set_estimated=False, **kwargs): The result of the parent save method. """ changed_fields = set() - if WHOISService.check_estimated_location_enabled(self.organization_id): + if EstimatedLocationService.check_estimated_location_enabled( + self.organization_id + ): address_changed = ( self._initial_address is not models.DEFERRED and self._initial_address != self.address @@ -150,3 +159,46 @@ def device(self): @property def organization_id(self): return self.device.organization_id + + +class AbstractOrganizationGeoSettings(UUIDModel): + organization = models.OneToOneField( + swapper.get_model_name("openwisp_users", "Organization"), + verbose_name=_("organization"), + related_name="geo_settings", + on_delete=models.CASCADE, + ) + estimated_location_enabled = FallbackBooleanChoiceField( + help_text=_("Whether the estimated location feature is enabled"), + fallback=geo_settings.ESTIMATED_LOCATION_ENABLED, + verbose_name=_("Estimated Location Enabled"), + ) + + class Meta: + verbose_name = _("Geographic settings") + verbose_name_plural = verbose_name + abstract = True + + def __str__(self): + return f"Geo settings for {self.organization.name}" + + def clean(self): + if not config_settings.WHOIS_CONFIGURED and self.estimated_location_enabled: + raise ValidationError( + { + "estimated_location_enabled": _( + "WHOIS_GEOIP_ACCOUNT and WHOIS_GEOIP_KEY must be set " + "before enabling Estimated Location feature." + ) + } + ) + return super().clean() + + @classmethod + def organization_post_save_receiver(cls, sender, instance, created, **kwargs): + """ + Create OrganizationGeoSettings when a new Organization is created. + This signal handler is called when an Organization is saved. + """ + if created: + cls.objects.get_or_create(organization=instance) diff --git a/openwisp_controller/geo/estimated_location/handlers.py b/openwisp_controller/geo/estimated_location/handlers.py index 2b66a6d94..70e21cb76 100644 --- a/openwisp_controller/geo/estimated_location/handlers.py +++ b/openwisp_controller/geo/estimated_location/handlers.py @@ -4,6 +4,7 @@ from openwisp_controller.config import settings as config_app_settings +from .service import EstimatedLocationService from .utils import MESSAGE_MAP @@ -33,3 +34,23 @@ def register_estimated_location_notification_types(): }, models=[Device, Location], ) + + +def whois_info_post_save_handler(sender, instance, created, **kwargs): + """ + Signal handler to create/update estimated location when WHOISInfo is saved. + This is triggered when WHOISInfo is created or updated. + """ + if not instance.coordinates: + return + Device = load_model("config", "Device") + try: + device = Device.objects.get(last_ip=instance.ip_address) + except Device.DoesNotExist: + return + if not EstimatedLocationService.check_estimated_location_enabled( + device.organization_id + ): + return + estimated_location_service = EstimatedLocationService(device) + estimated_location_service.trigger_estimated_location_task(instance.ip_address) diff --git a/openwisp_controller/geo/estimated_location/mixins.py b/openwisp_controller/geo/estimated_location/mixins.py index 222eaa8b7..9761616f1 100644 --- a/openwisp_controller/geo/estimated_location/mixins.py +++ b/openwisp_controller/geo/estimated_location/mixins.py @@ -1,4 +1,4 @@ -from openwisp_controller.config.whois.service import WHOISService +from openwisp_controller.geo.estimated_location.service import EstimatedLocationService # These mixins are required as estimated location is an organization level feature. @@ -11,7 +11,9 @@ class EstimatedLocationMixin: def to_representation(self, obj): data = super().to_representation(obj) - if WHOISService.check_estimated_location_enabled(obj.organization_id): + if EstimatedLocationService.check_estimated_location_enabled( + obj.organization_id + ): data["is_estimated"] = obj.is_estimated else: data.pop("is_estimated", None) @@ -26,7 +28,9 @@ class EstimatedLocationGeoJsonMixin: def to_representation(self, obj): data = super().to_representation(obj) - if WHOISService.check_estimated_location_enabled(obj.organization_id): + if EstimatedLocationService.check_estimated_location_enabled( + obj.organization_id + ): data["properties"]["is_estimated"] = obj.is_estimated else: data["properties"].pop("is_estimated", None) diff --git a/openwisp_controller/geo/estimated_location/service.py b/openwisp_controller/geo/estimated_location/service.py new file mode 100644 index 000000000..0837058d2 --- /dev/null +++ b/openwisp_controller/geo/estimated_location/service.py @@ -0,0 +1,98 @@ +import logging + +from django.conf import settings +from django.db import transaction +from swapper import load_model + +from openwisp_controller.config import settings as config_app_settings + +logger = logging.getLogger(__name__) + + +class EstimatedLocationService: + def __init__(self, device): + self.device = device + + @staticmethod + def check_estimated_location_enabled(org_id): + if not org_id: + return False + if not config_app_settings.WHOIS_CONFIGURED: + return False + OrganizationGeoSettings = load_model("geo", "OrganizationGeoSettings") + try: + org_settings = OrganizationGeoSettings.objects.get(organization_id=org_id) + except OrganizationGeoSettings.DoesNotExist: + from .. import settings as geo_app_settings + + return geo_app_settings.ESTIMATED_LOCATION_ENABLED + return org_settings.estimated_location_enabled + + @property + def is_estimated_location_enabled(self): + if not config_app_settings.WHOIS_CONFIGURED: + return False + return self.check_estimated_location_enabled(self.device.organization_id) + + def trigger_estimated_location_task(self, ip_address): + current_app = settings.CELERY_APP + current_app.send_task( + "whois_estimated_location_task", + kwargs={"device_pk": self.device.pk, "ip_address": ip_address}, + ) + + def _create_or_update_estimated_location( + self, location_defaults, attached_devices_exists + ): + """ + Create or update estimated location for the device based on the + given location defaults. + """ + Location = load_model("geo", "Location") + DeviceLocation = load_model("geo", "DeviceLocation") + + if not (device_location := getattr(self.device, "devicelocation", None)): + device_location = DeviceLocation(content_object=self.device) + + current_location = device_location.location + + if not current_location or ( + attached_devices_exists and current_location.is_estimated + ): + with transaction.atomic(): + current_location = Location(**location_defaults, is_estimated=True) + current_location.full_clean() + current_location.save(_set_estimated=True) + device_location.location = current_location + device_location.full_clean() + device_location.save() + from openwisp_controller.config.whois.utils import ( + send_whois_task_notification, + ) + + send_whois_task_notification( + device=self.device, + notify_type="estimated_location_created", + actor=current_location, + ) + elif current_location.is_estimated: + update_fields = [] + for attr, value in location_defaults.items(): + if getattr(current_location, attr) != value: + setattr(current_location, attr, value) + update_fields.append(attr) + if update_fields: + with transaction.atomic(): + current_location.save( + update_fields=update_fields, _set_estimated=True + ) + from openwisp_controller.config.whois.utils import ( + send_whois_task_notification, + ) + + send_whois_task_notification( + device=self.device, + notify_type="estimated_location_updated", + actor=current_location, + ) + return current_location diff --git a/openwisp_controller/geo/estimated_location/tasks.py b/openwisp_controller/geo/estimated_location/tasks.py index 58eb863d1..415e20a99 100644 --- a/openwisp_controller/geo/estimated_location/tasks.py +++ b/openwisp_controller/geo/estimated_location/tasks.py @@ -4,6 +4,10 @@ from swapper import load_model from openwisp_controller.config.whois.utils import send_whois_task_notification +from openwisp_controller.geo.estimated_location.service import EstimatedLocationService +from openwisp_controller.geo.estimated_location.utils import ( + get_location_defaults_from_whois, +) logger = logging.getLogger(__name__) @@ -61,7 +65,7 @@ def _handle_attach_existing_location( return location_defaults = { - **whois_obj._get_defaults_for_estimated_location(), + **get_location_defaults_from_whois(whois_obj), "organization_id": device.organization_id, } # Create new location only if location is changed. @@ -78,13 +82,12 @@ def _handle_attach_existing_location( return # create new location if no location exists for device or the estimated location # of device is shared. - whois_service = device.whois_service - whois_service._create_or_update_estimated_location( + estimated_location_service = EstimatedLocationService(device) + estimated_location_service._create_or_update_estimated_location( location_defaults, attached_devices_exists ) logger.info( - f"Estimated location saved successfully for {device.pk}" - f" for IP: {ip_address}" + f"Estimated location saved successfully for {device.pk} for IP: {ip_address}" ) diff --git a/openwisp_controller/geo/estimated_location/tests/tests.py b/openwisp_controller/geo/estimated_location/tests/tests.py index 70794f44c..c3133a788 100644 --- a/openwisp_controller/geo/estimated_location/tests/tests.py +++ b/openwisp_controller/geo/estimated_location/tests/tests.py @@ -30,46 +30,37 @@ WHOISInfo = load_model("config", "WHOISInfo") Notification = load_model("openwisp_notifications", "Notification") OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings") +OrganizationGeoSettings = load_model("geo", "OrganizationGeoSettings") def _notification_qs(): return Notification.objects.all() -class TestEstimatedLocation(TestAdminMixin, TestCase): +class TestEstimatedLocation( + TestEstimatedLocationMixin, TestAdminMixin, TestGeoMixin, TestCase +): @override_settings( OPENWISP_CONTROLLER_WHOIS_GEOIP_ACCOUNT="test_account", OPENWISP_CONTROLLER_WHOIS_GEOIP_KEY="test_key", ) def test_estimated_location_configuration_setting(self): - # reload app_settings to apply the overridden + # reload app_settings to apply the overridden settings self.addCleanup(importlib.reload, config_app_settings) importlib.reload(config_app_settings) with self.subTest( - "ImproperlyConfigured raised when ESTIMATED_LOCATION_ENABLED is True " - "and WHOIS_ENABLED is False globally" + "Test WHOIS not configured does not allow enabling Estimated Location" ): - with override_settings( - OPENWISP_CONTROLLER_WHOIS_ENABLED=False, - OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED=True, - ): - with self.assertRaises(ImproperlyConfigured): - # reload app_settings to apply the overridden settings - importlib.reload(config_app_settings) - - with self.subTest( - "Test WHOIS not enabled does not allow enabling Estimated Location" - ): - org_settings_obj = OrganizationConfigSettings(organization=self._get_org()) - with self.assertRaises(ValidationError) as context_manager: - org_settings_obj.whois_enabled = False - org_settings_obj.estimated_location_enabled = True - org_settings_obj.full_clean() - self.assertEqual( - context_manager.exception.message_dict["estimated_location_enabled"][0], - "Estimated Location feature requires " - + "WHOIS Lookup feature to be enabled.", - ) + org = self._get_org() + geo_settings = org.geo_settings + geo_settings.estimated_location_enabled = True + with mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", False): + with self.assertRaises(ValidationError) as context_manager: + geo_settings.full_clean() + self.assertIn( + "estimated_location_enabled", + context_manager.exception.message_dict, + ) with self.subTest( "Test Estimated Location field visible on admin when " @@ -83,14 +74,10 @@ def test_estimated_location_configuration_setting(self): ) response = self.client.get(url) self.assertContains( - response, 'name="config_settings-0-estimated_location_enabled"' + response, 'name="geo_settings-0-estimated_location_enabled"' ) - with override_settings( - OPENWISP_CONTROLLER_WHOIS_GEOIP_ACCOUNT=None, - OPENWISP_CONTROLLER_WHOIS_GEOIP_KEY=None, - ): - importlib.reload(config_app_settings) + with mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", False): with self.subTest( "Test Estimated Location field hidden on admin when " "WHOIS_CONFIGURED is False" @@ -103,18 +90,37 @@ def test_estimated_location_configuration_setting(self): ) response = self.client.get(url) self.assertNotContains( - response, 'name="config_settings-0-estimated_location_enabled"' + response, 'name="geo_settings-0-estimated_location_enabled"' ) + def test_organization_geo_settings_validation(self): + """Test OrganizationGeoSettings model validation.""" + org = self._get_org() + + with self.subTest("WHOIS must be configured to enable estimated location"): + geo_settings = org.geo_settings + geo_settings.estimated_location_enabled = True + with mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", False): + with self.assertRaises(ValidationError) as context_manager: + geo_settings.full_clean() + self.assertIn( + "estimated_location_enabled", + context_manager.exception.message_dict, + ) + + with self.subTest("Estimated location can be enabled when WHOIS is configured"): + geo_settings = org.geo_settings + geo_settings.estimated_location_enabled = True + # Should not raise + geo_settings.full_clean() -class TestEstimatedLocationField(TestEstimatedLocationMixin, TestGeoMixin, TestCase): location_model = Location def test_estimated_location_field(self): org = self._get_org() - org.config_settings.estimated_location_enabled = False - org.config_settings.save() - org.refresh_from_db() + # Disable estimated_location_enabled via OrganizationGeoSettings + org.geo_settings.estimated_location_enabled = False + org.geo_settings.save() with self.assertRaises(ValidationError) as context_manager: self._create_location(organization=org, is_estimated=True) self.assertEqual( @@ -125,7 +131,7 @@ def test_estimated_location_field(self): @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) def test_estimated_location_admin(self): connect_whois_handlers() - admin = self._create_admin() + admin = self._get_admin() self.client.force_login(admin) org = self._get_org() location = self._create_location(organization=org, is_estimated=True) @@ -146,9 +152,9 @@ def test_estimated_location_admin(self): response = self.client.get(add_path) self.assertNotContains(response, "field-is_estimated") - org.config_settings.estimated_location_enabled = False - org.config_settings.save() - org.config_settings.refresh_from_db() + org.geo_settings.estimated_location_enabled = False + org.geo_settings.save() + org.geo_settings.refresh_from_db() with self.subTest( "is-estimated field hidden when estimated location is disabled" @@ -167,7 +173,7 @@ def test_estimated_location_admin(self): @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", False) def test_estimated_location_admin_add_whois_disabled(self): - admin = self._create_admin() + admin = self._get_admin() self.client.force_login(admin) path = reverse("admin:geo_location_add") response = self.client.get(path) @@ -224,13 +230,17 @@ def test_estimated_location_task_called( WHOISInfo.objects.all().delete() org = self._get_org() org.config_settings.whois_enabled = True - org.config_settings.estimated_location_enabled = True org.config_settings.save() + org.geo_settings.estimated_location_enabled = True + org.geo_settings.save() with self.subTest("Estimated location task called when last_ip is public"): - with mock.patch( - "django.core.cache.cache.get", return_value=None - ) as mocked_get, mock.patch("django.core.cache.cache.set") as mocked_set: + with ( + mock.patch( + "django.core.cache.cache.get", return_value=None + ) as mocked_get, + mock.patch("django.core.cache.cache.set") as mocked_set, + ): device = self._create_device(last_ip="172.217.22.14") mocked_estimated_location_task.assert_called() expected_cache_set_calls = [ @@ -250,9 +260,10 @@ def test_estimated_location_task_called( with self.subTest( "Estimated location task called when last_ip is changed and is public" ): - with mock.patch("django.core.cache.cache.get") as mocked_get, mock.patch( - "django.core.cache.cache.set" - ) as mocked_set: + with ( + mock.patch("django.core.cache.cache.get") as mocked_get, + mock.patch("django.core.cache.cache.set") as mocked_set, + ): device.last_ip = "172.217.22.10" device.save() device.refresh_from_db() @@ -269,9 +280,10 @@ def test_estimated_location_task_called( with self.subTest( "Estimated location task called when last_ip has related WhoIsInfo" ): - with mock.patch("django.core.cache.cache.get") as mocked_get, mock.patch( - "django.core.cache.cache.set" - ) as mocked_set: + with ( + mock.patch("django.core.cache.cache.get") as mocked_get, + mock.patch("django.core.cache.cache.set") as mocked_set, + ): self._create_config(device=device) device.last_ip = "172.217.22.14" self._create_whois_info(ip_address=device.last_ip) @@ -607,14 +619,14 @@ def test_estimated_location_handling_on_whois_update( threshold = config_app_settings.WHOIS_REFRESH_THRESHOLD_DAYS + 1 new_time = timezone.now() - timedelta(days=threshold) org = self._get_org() - org.config_settings.estimated_location_enabled = False - org.config_settings.save() + org.geo_settings.estimated_location_enabled = False + org.geo_settings.save() device = self._create_device(last_ip="172.217.22.10") with self.assertRaises(Device.devicelocation.RelatedObjectDoesNotExist): # Accessing devicelocation to verify it doesn't exist (raises if not) device.devicelocation - org.config_settings.estimated_location_enabled = True - org.config_settings.save() + org.geo_settings.estimated_location_enabled = True + org.geo_settings.save() whois_obj = device.whois_service.get_device_whois_info() WHOISInfo.objects.filter(pk=whois_obj.pk).update(modified=new_time) device.name = "test.new.name" @@ -786,9 +798,9 @@ def test_estimate_location_status_remove(self, mock_client, _): with self.subTest( "Test Estimated Status unchanged if Estimated feature is disabled" ): - org.config_settings.estimated_location_enabled = False - org.config_settings.save() - org.config_settings.refresh_from_db() + org.geo_settings.estimated_location_enabled = False + org.geo_settings.save() + org.geo_settings.refresh_from_db() location.geometry = GEOSGeometry("POINT(12.512124 41.898903)", srid=4326) location.save() location.refresh_from_db() @@ -799,9 +811,9 @@ def test_estimate_location_status_remove(self, mock_client, _): "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() - org.config_settings.refresh_from_db() + org.geo_settings.estimated_location_enabled = True + org.geo_settings.save() + org.geo_settings.refresh_from_db() location._set_initial_values_for_changed_checked_fields() location.type = "outdoor" location.is_mobile = True @@ -830,7 +842,7 @@ class TestEstimatedLocationFieldFilters( def setUp(self): super().setUp() - admin = self._create_admin() + admin = self._get_admin() self.client.force_login(admin) @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) @@ -840,7 +852,6 @@ def test_estimated_location_api_status_configured(self): OrganizationConfigSettings.objects.create( organization=org2, whois_enabled=False, - estimated_location_enabled=False, ) org1_location = self._create_location( name="org1-location", organization=org1, is_estimated=True @@ -853,7 +864,7 @@ def test_estimated_location_api_status_configured(self): with self.subTest("Test Estimated Location in Locations List"): path = reverse("geo_api:list_location") - with self.assertNumQueries(4): + with self.assertNumQueries(6): response = self.client.get(path) self.assertEqual(response.status_code, 200) self.assertEqual(response.data["count"], 2) @@ -865,19 +876,19 @@ def test_estimated_location_api_status_configured(self): with self.subTest("Test Estimated Location in Device Locations List"): path = reverse("geo_api:device_location", args=[org1_device.pk]) - with self.assertNumQueries(4): + with self.assertNumQueries(5): response = self.client.get(path) self.assertEqual(response.status_code, 200) self.assertIn("is_estimated", response.data["location"]["properties"]) path = reverse("geo_api:device_location", args=[org2_device.pk]) - with self.assertNumQueries(4): + with self.assertNumQueries(5): response = self.client.get(path) self.assertEqual(response.status_code, 200) self.assertNotIn("is_estimated", response.data["location"]["properties"]) with self.subTest("Test Estimated Location in GeoJSON List"): path = reverse("geo_api:location_geojson") - with self.assertNumQueries(3): + with self.assertNumQueries(5): response = self.client.get(path) self.assertEqual(response.status_code, 200) self.assertEqual(response.data["count"], 2) @@ -945,7 +956,7 @@ def test_estimated_location_filter_list_api(self): "Test Estimated Location filter available in location list " "when WHOIS is configured" ): - with self.assertNumQueries(4): + with self.assertNumQueries(5): response = self.client.get(path, {"is_estimated": True}) self.assertEqual(response.status_code, 200) self.assertEqual(response.data["count"], 1) diff --git a/openwisp_controller/geo/estimated_location/tests/utils.py b/openwisp_controller/geo/estimated_location/tests/utils.py index 0ca26f335..f56ee3c82 100644 --- a/openwisp_controller/geo/estimated_location/tests/utils.py +++ b/openwisp_controller/geo/estimated_location/tests/utils.py @@ -6,17 +6,22 @@ from ..tasks import manage_estimated_locations OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings") +OrganizationGeoSettings = load_model("geo", "OrganizationGeoSettings") class TestEstimatedLocationMixin(CreateWHOISMixin): def setUp(self): # skip the org config settings creation from the parent mixin super(CreateWHOISMixin, self).setUp() + org = self._get_org() OrganizationConfigSettings.objects.create( - organization=self._get_org(), + organization=org, whois_enabled=True, - estimated_location_enabled=True, ) + # OrganizationGeoSettings is auto-created by signal, + # update estimated_location_enabled + org.geo_settings.estimated_location_enabled = True + org.geo_settings.save() # helper to mock send_task to directly call the task function synchronously @staticmethod diff --git a/openwisp_controller/geo/estimated_location/utils.py b/openwisp_controller/geo/estimated_location/utils.py index f7516afdc..b83ab5bb8 100644 --- a/openwisp_controller/geo/estimated_location/utils.py +++ b/openwisp_controller/geo/estimated_location/utils.py @@ -47,3 +47,22 @@ def get_device_location_notification_target_url(obj, field, absolute_url=True): url = _get_object_link(obj._related_object(field), absolute_url) return f"{url}#devicelocation-group" + + +def get_location_defaults_from_whois(whois_obj): + """ + Create default location data from a WHOISInfo object. + + Args: + whois_obj: WHOISInfo instance with WHOIS data + + Returns: + dict: Default values for creating an estimated Location instance + """ + return { + "name": whois_obj._location_name, + "type": "outdoor", + "is_mobile": False, + "geometry": whois_obj.coordinates, + "address": whois_obj.formatted_address, + } diff --git a/openwisp_controller/geo/migrations/0005_organizationgeosettings.py b/openwisp_controller/geo/migrations/0005_organizationgeosettings.py new file mode 100644 index 000000000..4bb42362b --- /dev/null +++ b/openwisp_controller/geo/migrations/0005_organizationgeosettings.py @@ -0,0 +1,62 @@ +# Generated by Django 5.2.12 on 2026-03-23 14:59 + +import uuid + +import django.db.models.deletion +import swapper +from django.db import migrations, models + +import openwisp_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ("geo", "0004_location_is_estimated"), + ("openwisp_users", "0021_rename_user_id_email_openwisp_us_id_06c07a_idx"), + ] + + operations = [ + migrations.CreateModel( + name="OrganizationGeoSettings", + fields=[ + ( + "id", + models.UUIDField( + default=uuid.uuid4, + editable=False, + primary_key=True, + serialize=False, + ), + ), + ( + "estimated_location_enabled", + openwisp_utils.fields.FallbackBooleanChoiceField( + blank=True, + default=None, + fallback=False, + help_text="Whether the estimated location feature is enabled", + null=True, + verbose_name="Estimated Location Enabled", + ), + ), + ( + "organization", + models.OneToOneField( + on_delete=django.db.models.deletion.CASCADE, + related_name="geo_settings", + to="openwisp_users.organization", + verbose_name="organization", + ), + ), + ], + options={ + "verbose_name": "Geographic settings", + "verbose_name_plural": "Geographic settings", + "abstract": False, + "swappable": swapper.swappable_setting( + "geo", "OrganizationGeoSettings" + ), + }, + ), + ] diff --git a/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py b/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py new file mode 100644 index 000000000..7c19a1587 --- /dev/null +++ b/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py @@ -0,0 +1,36 @@ +# Generated by Django 5.2.12 on 2026-03-23 14:59 + +from django.db import migrations + +from . import assign_geo_settings_permissions_to_groups + + +def create_geo_settings_for_existing_orgs(apps, schema_editor): + """ + Create OrganizationGeoSettings for all existing organizations + that don't have one yet. + """ + Organization = apps.get_model("openwisp_users", "Organization") + OrganizationGeoSettings = apps.get_model("geo", "OrganizationGeoSettings") + + for org in Organization.objects.all(): + OrganizationGeoSettings.objects.get_or_create(organization_id=org.pk) + + +class Migration(migrations.Migration): + + dependencies = [ + ("geo", "0005_organizationgeosettings"), + ("openwisp_users", "0021_rename_user_id_email_openwisp_us_id_06c07a_idx"), + ] + + operations = [ + migrations.RunPython( + assign_geo_settings_permissions_to_groups, + reverse_code=migrations.RunPython.noop, + ), + migrations.RunPython( + create_geo_settings_for_existing_orgs, + reverse_code=migrations.RunPython.noop, + ), + ] diff --git a/openwisp_controller/geo/migrations/__init__.py b/openwisp_controller/geo/migrations/__init__.py index d2fb75c6a..59d48f401 100644 --- a/openwisp_controller/geo/migrations/__init__.py +++ b/openwisp_controller/geo/migrations/__init__.py @@ -3,9 +3,8 @@ from ...migrations import create_default_permissions, get_swapped_model -def assign_permissions_to_groups(apps, schema_editor): +def _assign_permissions_to_groups(apps, schema_editor, operators_and_admins_can_change): create_default_permissions(apps, schema_editor) - operators_and_admins_can_change = ["location", "floorplan", "devicelocation"] manage_operations = ["add", "change", "delete"] Group = get_swapped_model(apps, "openwisp_users", "Group") @@ -24,3 +23,17 @@ def assign_permissions_to_groups(apps, schema_editor): ) admin.permissions.add(permission.pk) operator.permissions.add(permission.pk) + + +def assign_permissions_to_groups(apps, schema_editor): + _assign_permissions_to_groups( + apps, + schema_editor, + operators_and_admins_can_change=["location", "floorplan", "devicelocation"], + ) + + +def assign_geo_settings_permissions_to_groups(apps, schema_editor): + _assign_permissions_to_groups( + apps, schema_editor, operators_and_admins_can_change=["organizationgeosettings"] + ) diff --git a/openwisp_controller/geo/models.py b/openwisp_controller/geo/models.py index effc93e36..a399b0950 100644 --- a/openwisp_controller/geo/models.py +++ b/openwisp_controller/geo/models.py @@ -1,6 +1,11 @@ import swapper -from .base.models import BaseDeviceLocation, BaseFloorPlan, BaseLocation +from .base.models import ( + AbstractOrganizationGeoSettings, + BaseDeviceLocation, + BaseFloorPlan, + BaseLocation, +) class Location(BaseLocation): @@ -21,6 +26,12 @@ class Meta(BaseDeviceLocation.Meta): swappable = swapper.swappable_setting("geo", "DeviceLocation") +class OrganizationGeoSettings(AbstractOrganizationGeoSettings): + class Meta(AbstractOrganizationGeoSettings.Meta): + abstract = False + swappable = swapper.swappable_setting("geo", "OrganizationGeoSettings") + + # maintain compatibility with django_loci Location.objectlocation_set = Location.devicelocation_set FloorPlan.objectlocation_set = FloorPlan.devicelocation_set diff --git a/openwisp_controller/geo/settings.py b/openwisp_controller/geo/settings.py new file mode 100644 index 000000000..b3434b945 --- /dev/null +++ b/openwisp_controller/geo/settings.py @@ -0,0 +1,18 @@ +import logging + +from django.core.exceptions import ImproperlyConfigured + +from ..config import settings as config_settings +from ..settings import get_setting + +logger = logging.getLogger(__name__) + +ESTIMATED_LOCATION_ENABLED = get_setting("ESTIMATED_LOCATION_ENABLED", False) + +# Validate that WHOIS is enabled if estimated location is enabled +if ESTIMATED_LOCATION_ENABLED: + if not config_settings.WHOIS_ENABLED: + raise ImproperlyConfigured( + "OPENWISP_CONTROLLER_WHOIS_ENABLED must be set to True before " + "setting OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED to True." + ) diff --git a/openwisp_controller/geo/tests/test_api.py b/openwisp_controller/geo/tests/test_api.py index d8743fa8c..748a49d6d 100644 --- a/openwisp_controller/geo/tests/test_api.py +++ b/openwisp_controller/geo/tests/test_api.py @@ -4,6 +4,7 @@ from unittest.mock import patch from django.contrib.auth import get_user_model +from django.contrib.contenttypes.models import ContentType from django.contrib.gis.geos import Point from django.test import TestCase from django.test.client import BOUNDARY, MULTIPART_CONTENT, encode_multipart @@ -27,6 +28,7 @@ Location = load_model("geo", "Location") FloorPlan = load_model("geo", "FloorPlan") DeviceLocation = load_model("geo", "DeviceLocation") +OrganizationGeoSettings = load_model("geo", "OrganizationGeoSettings") OrganizationUser = load_model("openwisp_users", "OrganizationUser") Group = load_model("openwisp_users", "Group") User = get_user_model() @@ -1353,3 +1355,305 @@ def test_indoor_coordinates_list_api(self): self.assertEqual(len(response.data["results"]), 1) self.assertEqual(response.data["results"][0]["device_name"], "device-0") self.assertEqual(response.data["results"][0]["floor"], 0) + + def _add_model_permission(self, user, model, perms): + content_type = ContentType.objects.get_for_model(model) + for perm in perms: + user.user_permissions.add( + content_type.permission_set.get( + codename=f"{perm}_{model._meta.model_name}" + ) + ) + + def test_organization_geo_settings_retrieve(self): + org1 = self._create_org(name="Org 1") + org2 = self._create_org(name="Org 2") + admin_user = self._create_user(username="admin_user", email="admin@test.com") + OrganizationUser.objects.create( + user=admin_user, organization=org1, is_admin=True + ) + org1_geo_settings = OrganizationGeoSettings.objects.get(organization=org1) + url = reverse("geo_api:organization_geo_settings", args=[org1.pk]) + org2_url = reverse("geo_api:organization_geo_settings", args=[org2.pk]) + + with self.subTest("Unauthenticated access"): + response = self.client.get(url) + self.assertEqual(response.status_code, 401) + response = self.client.put( + url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 401) + response = self.client.patch( + url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 401) + + self.client.force_login(user=admin_user) + with self.subTest("Access without permission"): + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + response = self.client.put( + url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 403) + response = self.client.patch( + url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 403) + + self._add_model_permission(admin_user, OrganizationGeoSettings, ["view"]) + with self.subTest("Retrieve organization geo settings"): + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + data = response.data + self.assertEqual(data["id"], str(org1_geo_settings.pk)) + self.assertEqual(str(data["organization"]), str(org1.pk)) + self.assertEqual( + data["estimated_location_enabled"], + org1_geo_settings.estimated_location_enabled, + ) + + with self.subTest("Cannot access geo settings from other organization"): + response = self.client.get(org2_url) + self.assertEqual(response.status_code, 404) + + with self.subTest("Cannot update without change permission"): + response = self.client.put( + url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 403) + response = self.client.patch( + url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 403) + + with self.subTest("Superuser can access any organization's geo settings"): + superuser = self._get_admin() + self.client.force_login(user=superuser) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + response = self.client.get(org2_url) + self.assertEqual(response.status_code, 200) + + def test_organization_geo_settings_update(self): + org1 = self._create_org(name="Org 1") + org2 = self._create_org(name="Org 2") + admin_user = self._create_user(username="admin_user", email="admin@test.com") + OrganizationUser.objects.create( + user=admin_user, organization=org1, is_admin=True + ) + org1_geo_settings = OrganizationGeoSettings.objects.get(organization=org1) + url = reverse("geo_api:organization_geo_settings", args=[org1.pk]) + org2_url = reverse("geo_api:organization_geo_settings", args=[org2.pk]) + + self.client.force_login(user=admin_user) + self._add_model_permission( + admin_user, OrganizationGeoSettings, ["view", "change"] + ) + + with self.subTest("PUT operation"): + response = self.client.put( + url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + org1_geo_settings.refresh_from_db() + self.assertEqual(org1_geo_settings.estimated_location_enabled, False) + + with self.subTest("PATCH operation"): + response = self.client.patch( + url, + {"estimated_location_enabled": True}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + org1_geo_settings.refresh_from_db() + self.assertEqual(org1_geo_settings.estimated_location_enabled, True) + + with self.subTest("PUT with organization field should be ignored"): + response = self.client.put( + url, + {"estimated_location_enabled": False, "organization": str(org2.pk)}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + org1_geo_settings.refresh_from_db() + self.assertEqual(org1_geo_settings.organization, org1) + self.assertEqual(org1_geo_settings.estimated_location_enabled, False) + + with self.subTest("Cannot update geo settings of other organization"): + response = self.client.put( + org2_url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 404) + response = self.client.patch( + org2_url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 404) + + with self.subTest("Validation error when WHOIS not configured"): + with patch.object(config_app_settings, "WHOIS_CONFIGURED", False): + response = self.client.put( + url, + {"estimated_location_enabled": True}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + self.assertIn("estimated_location_enabled", response.data) + org1_geo_settings.refresh_from_db() + self.assertEqual(org1_geo_settings.estimated_location_enabled, True) + + response = self.client.patch( + url, + {"estimated_location_enabled": True}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + self.assertIn("estimated_location_enabled", response.data) + + with self.subTest("Superuser can update any organization's geo settings"): + superuser = self._get_admin() + self.client.force_login(user=superuser) + org2_geo_settings = OrganizationGeoSettings.objects.get(organization=org2) + response = self.client.put( + org2_url, + {"estimated_location_enabled": True}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + org2_geo_settings.refresh_from_db() + self.assertEqual(org2_geo_settings.estimated_location_enabled, True) + + def test_organization_geo_settings_multi_tenancy(self): + org1 = self._create_org(name="Org 1") + org2 = self._create_org(name="Org 2") + admin_user = self._create_user(username="admin_user", email="admin@test.com") + OrganizationUser.objects.create( + user=admin_user, organization=org1, is_admin=True + ) + self._add_model_permission( + admin_user, OrganizationGeoSettings, ["view", "change"] + ) + url = reverse("geo_api:organization_geo_settings", args=[org1.pk]) + org2_url = reverse("geo_api:organization_geo_settings", args=[org2.pk]) + + with self.subTest("User cannot access organization they don't manage"): + self.client.force_login(user=admin_user) + response = self.client.get(org2_url) + self.assertEqual(response.status_code, 404) + response = self.client.put( + org2_url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 404) + + with self.subTest("User can access organization they manage"): + self.client.force_login(user=admin_user) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + + with self.subTest("User managing multiple organizations can access both"): + OrganizationUser.objects.create( + user=admin_user, organization=org2, is_admin=True + ) + self.client.force_login(user=admin_user) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + response = self.client.get(org2_url) + self.assertEqual(response.status_code, 200) + + def test_organization_geo_settings_user_access_levels(self): + org1 = self._create_org(name="Org 1") + regular_member = self._create_user( + username="regular_member", email="member@test.com" + ) + OrganizationUser.objects.create(user=regular_member, organization=org1) + url = reverse("geo_api:organization_geo_settings", args=[org1.pk]) + + with self.subTest("Regular organization member cannot access"): + self.client.force_login(user=regular_member) + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + + with self.subTest("Organization admin can access"): + org_admin = self._create_user( + username="org_admin", email="orgadmin@test.com" + ) + OrganizationUser.objects.create( + user=org_admin, organization=org1, is_admin=True + ) + self._add_model_permission(org_admin, OrganizationGeoSettings, ["view"]) + self.client.force_login(user=org_admin) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + + with self.subTest("Superuser can access without explicit permissions"): + superuser = self._get_admin() + self.client.force_login(user=superuser) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + + def test_organization_geo_settings_non_existent_organization(self): + org = self._create_org(name="Org 1") + admin_user = self._create_user(username="admin_user", email="admin@test.com") + OrganizationUser.objects.create( + user=admin_user, organization=org, is_admin=True + ) + self._add_model_permission( + admin_user, OrganizationGeoSettings, ["view", "change"] + ) + self.client.force_login(user=admin_user) + fake_uuid = uuid.uuid4() + url = reverse("geo_api:organization_geo_settings", args=[fake_uuid]) + + with self.subTest("GET returns 404"): + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + with self.subTest("PUT returns 404"): + response = self.client.put( + url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 404) + + with self.subTest("PATCH returns 404"): + response = self.client.patch( + url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 404) + + with self.subTest("Cannot update without change permission"): + response = self.client.put( + url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 403) + response = self.client.patch( + url, + {"estimated_location_enabled": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 403) diff --git a/openwisp_controller/geo/utils.py b/openwisp_controller/geo/utils.py index c113b116b..09a37d20c 100644 --- a/openwisp_controller/geo/utils.py +++ b/openwisp_controller/geo/utils.py @@ -46,4 +46,9 @@ def get_geo_urls(geo_views): geo_views.indoor_coordinates_list, name="indoor_coordinates_list", ), + path( + "api/v1/controller/organization//geo-settings/", + geo_views.organization_geo_settings, + name="organization_geo_settings", + ), ] diff --git a/openwisp_controller/settings.py b/openwisp_controller/settings.py index 50d1d074e..a84da494e 100644 --- a/openwisp_controller/settings.py +++ b/openwisp_controller/settings.py @@ -1,3 +1,8 @@ from django.conf import settings -OPENWISP_CONTROLLER_API_HOST = getattr(settings, "OPENWISP_CONTROLLER_API_HOST", None) + +def get_setting(option, default): + return getattr(settings, f"OPENWISP_CONTROLLER_{option}", default) + + +OPENWISP_CONTROLLER_API_HOST = get_setting("API_HOST", None) diff --git a/tests/openwisp2/sample_geo/models.py b/tests/openwisp2/sample_geo/models.py index a5ffd55b5..dc3383255 100644 --- a/tests/openwisp2/sample_geo/models.py +++ b/tests/openwisp2/sample_geo/models.py @@ -1,6 +1,8 @@ +import swapper from django.db import models from openwisp_controller.geo.base.models import ( + AbstractOrganizationGeoSettings, BaseDeviceLocation, BaseFloorPlan, BaseLocation, @@ -29,6 +31,11 @@ class Meta(BaseDeviceLocation.Meta): abstract = False +class OrganizationGeoSettings(AbstractOrganizationGeoSettings): + class Meta(AbstractOrganizationGeoSettings.Meta): + abstract = False + + # maintain compatibility with django_loci Location.objectlocation_set = Location.devicelocation_set FloorPlan.objectlocation_set = FloorPlan.devicelocation_set diff --git a/tests/openwisp2/settings.py b/tests/openwisp2/settings.py index ea1a79a56..64cf854af 100644 --- a/tests/openwisp2/settings.py +++ b/tests/openwisp2/settings.py @@ -280,6 +280,7 @@ GEO_LOCATION_MODEL = "sample_geo.Location" GEO_FLOORPLAN_MODEL = "sample_geo.FloorPlan" GEO_DEVICELOCATION_MODEL = "sample_geo.DeviceLocation" + GEO_ORGANIZATIONGEOSETTINGS_MODEL = "sample_geo.OrganizationGeoSettings" CONNECTION_CREDENTIALS_MODEL = "sample_connection.Credentials" CONNECTION_DEVICECONNECTION_MODEL = "sample_connection.DeviceConnection" CONNECTION_COMMAND_MODEL = "sample_connection.Command" From b39485c67b64f6e43d3a7181abf48b8443143de3 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Tue, 24 Mar 2026 16:56:15 +0530 Subject: [PATCH 02/10] [fix] Fixes by @coderabbitai --- docs/developer/utils.rst | 14 +++ docs/user/rest-api.rst | 4 + ...ngs_estimated_location_enabled_and_more.py | 2 +- openwisp_controller/config/signals.py | 4 + openwisp_controller/config/whois/service.py | 1 + openwisp_controller/config/whois/tasks.py | 9 ++ .../config/whois/tests/tests.py | 13 ++- openwisp_controller/geo/admin.py | 6 +- openwisp_controller/geo/apps.py | 26 ++++-- .../geo/estimated_location/handlers.py | 30 ++++--- .../geo/estimated_location/service.py | 54 ++++++++---- .../geo/estimated_location/tests/tests.py | 87 +++++++++++++++---- .../geo/estimated_location/tests/utils.py | 5 +- ...6_create_geo_settings_for_existing_orgs.py | 29 ++++++- openwisp_controller/geo/settings.py | 4 - tests/openwisp2/sample_geo/models.py | 3 +- 16 files changed, 226 insertions(+), 65 deletions(-) diff --git a/docs/developer/utils.rst b/docs/developer/utils.rst index 8cd9a47b8..e861eaaf7 100644 --- a/docs/developer/utils.rst +++ b/docs/developer/utils.rst @@ -379,3 +379,17 @@ The signal is emitted when the peers of VPN server gets changed. It is only emitted for ``Vpn`` object with **WireGuard** or **VXLAN over WireGuard** backend. + +``whois_fetched`` +~~~~~~~~~~~~~~~~~ + +**Path**: ``openwisp_controller.config.signals.whois_fetched`` + +**Arguments**: + +- ``whois``: instance of ``WHOISInfo`` that was created or updated +- ``updated_fields``: list of fields updated in the lookup +- ``device``: optional instance of ``Device`` related to this WHOIS lookup + +This signal is emitted when a WHOIS lookup task successfully creates or +updates a ``WHOISInfo`` record. diff --git a/docs/user/rest-api.rst b/docs/user/rest-api.rst index 30387172e..774f3c196 100644 --- a/docs/user/rest-api.rst +++ b/docs/user/rest-api.rst @@ -821,6 +821,10 @@ organization. PUT /api/v1/controller/organization/{organization_pk}/geo-settings/ +.. code-block:: text + + PATCH /api/v1/controller/organization/{organization_pk}/geo-settings/ + .. code-block:: text curl -X PUT \ diff --git a/openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py b/openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py index 426b2e51e..ec96dc609 100644 --- a/openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py +++ b/openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py @@ -4,12 +4,12 @@ class Migration(migrations.Migration): - dependencies = [ ( "config", "0063_organizationconfigsettings_estimated_location_enabled_and_more", ), + ("geo", "0006_create_geo_settings_for_existing_orgs"), ] operations = [ diff --git a/openwisp_controller/config/signals.py b/openwisp_controller/config/signals.py index 325cfb628..eeb2bf9b2 100644 --- a/openwisp_controller/config/signals.py +++ b/openwisp_controller/config/signals.py @@ -65,3 +65,7 @@ vpn_server_modified.__doc__ = """ providing arguments: ['instance'] """ +whois_fetched = Signal() +whois_fetched.__doc__ = """ +Providing arguments: ['whois', 'updated_fields', 'device'] +""" diff --git a/openwisp_controller/config/whois/service.py b/openwisp_controller/config/whois/service.py index 663833516..55b14a806 100644 --- a/openwisp_controller/config/whois/service.py +++ b/openwisp_controller/config/whois/service.py @@ -261,4 +261,5 @@ def _create_or_update_whois(self, whois_details, whois_instance=None): whois_instance = WHOISInfo(**whois_details) whois_instance.full_clean() whois_instance.save(force_insert=True) + update_fields = list(whois_details.keys()) return whois_instance, update_fields diff --git a/openwisp_controller/config/whois/tasks.py b/openwisp_controller/config/whois/tasks.py index fbfd32b7f..27532b9fc 100644 --- a/openwisp_controller/config/whois/tasks.py +++ b/openwisp_controller/config/whois/tasks.py @@ -6,6 +6,7 @@ from geoip2 import errors from swapper import load_model +from openwisp_controller.config.signals import whois_fetched from openwisp_utils.tasks import OpenwispCeleryTask from .. import settings as app_settings @@ -85,6 +86,14 @@ def fetch_whois_details(self, device_pk, initial_ip_address): fetched_details, whois_obj ) logger.info(f"Successfully fetched WHOIS details for {new_ip_address}.") + transaction.on_commit( + lambda: whois_fetched.send( + sender=WHOISInfo, + whois=whois_obj, + updated_fields=update_fields, + device=device, + ) + ) if initial_ip_address: transaction.on_commit( # execute synchronously as we're already in a background task diff --git a/openwisp_controller/config/whois/tests/tests.py b/openwisp_controller/config/whois/tests/tests.py index c3622ec96..0308c3e44 100644 --- a/openwisp_controller/config/whois/tests/tests.py +++ b/openwisp_controller/config/whois/tests/tests.py @@ -21,7 +21,8 @@ from selenium.webdriver.common.by import By from swapper import load_model -from openwisp_utils.tests import SeleniumTestMixin +from openwisp_controller.config.signals import whois_fetched +from openwisp_utils.tests import SeleniumTestMixin, catch_signal from ....tests.utils import TestAdminMixin from ... import settings as app_settings @@ -450,6 +451,16 @@ def test_process_whois_details_handles_missing_coordinates(self, mock_client): whois_details = device.whois_service.process_whois_details(device.last_ip) self.assertIsNone(whois_details.get("coordinates")) + @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) + @mock.patch(_WHOIS_GEOIP_CLIENT) + def test_fetch_whois_emits_signal(self, mock_client): + connect_whois_handlers() + mock_client.return_value.city.return_value = self._mocked_client_response() + device = self._create_device(last_ip="172.217.22.14") + with catch_signal(whois_fetched) as handler: + fetch_whois_details(device_pk=device.pk, initial_ip_address=None) + handler.assert_called() + @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) @mock.patch("openwisp_controller.config.whois.tasks.fetch_whois_details.delay") def test_whois_task_called(self, mocked_lookup_task): diff --git a/openwisp_controller/geo/admin.py b/openwisp_controller/geo/admin.py index 780fdb322..1a404d3f0 100644 --- a/openwisp_controller/geo/admin.py +++ b/openwisp_controller/geo/admin.py @@ -214,9 +214,11 @@ class GeoSettingsInline(admin.StackedInline): def get_readonly_fields(self, request, obj=None): fields = list(super().get_readonly_fields(request, obj)) + # If WHOIS is NOT configured, make the estimated_location_enabled + # field readonly so it cannot be toggled in the admin. if ( - "estimated_location_enabled" in fields - and not config_app_settings.WHOIS_CONFIGURED + not config_app_settings.WHOIS_CONFIGURED + and "estimated_location_enabled" not in fields ): fields = ["estimated_location_enabled"] + fields return fields diff --git a/openwisp_controller/geo/apps.py b/openwisp_controller/geo/apps.py index d77006d5d..4e7cf1d5c 100644 --- a/openwisp_controller/geo/apps.py +++ b/openwisp_controller/geo/apps.py @@ -6,19 +6,21 @@ from django.conf import settings from django.db import transaction from django.db.models import Case, Count, Sum, When -from django.db.models.signals import post_save +from django.db.models.signals import post_delete, post_save from django.utils.translation import gettext_lazy as _ from django_loci.apps import LociConfig from swapper import get_model_name +from openwisp_controller.config.signals import whois_fetched from openwisp_utils.admin_theme import register_dashboard_chart from openwisp_utils.admin_theme.menu import register_menu_group from ..config import settings as config_app_settings from .estimated_location.handlers import ( register_estimated_location_notification_types, - whois_info_post_save_handler, + whois_fetched_handler, ) +from .estimated_location.service import EstimatedLocationService class GeoConfig(LociConfig): @@ -49,11 +51,23 @@ def connect_receivers(self): sender=self.organization_model, dispatch_uid="organization_geo_settings_post_save", ) + # invalidate cached OrganizationGeoSettings when an OrganizationGeoSettings + # instance is updated or deleted + post_save.connect( + EstimatedLocationService.invalidate_org_settings_cache, + sender=self.org_geo_settings_model, + dispatch_uid="invalidate_org_geo_settings_cache_on_save", + ) + post_delete.connect( + EstimatedLocationService.invalidate_org_settings_cache, + sender=self.org_geo_settings_model, + dispatch_uid="invalidate_org_geo_settings_cache_on_delete", + ) if config_app_settings.WHOIS_CONFIGURED: - post_save.connect( - whois_info_post_save_handler, - sender=self.whois_info_model, - dispatch_uid="whois_info_estimated_location_handler", + # connect estimated location handler to whois_fetched signal + whois_fetched.connect( + whois_fetched_handler, + dispatch_uid="whois_fetched_estimated_location_handler", ) def _add_params_to_test_config(self): diff --git a/openwisp_controller/geo/estimated_location/handlers.py b/openwisp_controller/geo/estimated_location/handlers.py index 70e21cb76..ec0b75beb 100644 --- a/openwisp_controller/geo/estimated_location/handlers.py +++ b/openwisp_controller/geo/estimated_location/handlers.py @@ -36,21 +36,27 @@ def register_estimated_location_notification_types(): ) -def whois_info_post_save_handler(sender, instance, created, **kwargs): +def whois_fetched_handler(sender, whois, updated_fields, device=None, **kwargs): """ - Signal handler to create/update estimated location when WHOISInfo is saved. - This is triggered when WHOISInfo is created or updated. + Signal handler triggered when WHOIS details are fetched. """ - if not instance.coordinates: - return - Device = load_model("config", "Device") - try: - device = Device.objects.get(last_ip=instance.ip_address) - except Device.DoesNotExist: + if ( + not updated_fields + or not device + or not EstimatedLocationService.check_estimated_location_enabled( + device.organization_id + ) + ): return - if not EstimatedLocationService.check_estimated_location_enabled( - device.organization_id + # the estimated location task should not run if old record is updated + # and location related fields are not updated + device_location = getattr(device, "devicelocation", None) + if ( + device_location + and device_location.location + and updated_fields + and not any(i in updated_fields for i in ["address", "coordinates"]) ): return estimated_location_service = EstimatedLocationService(device) - estimated_location_service.trigger_estimated_location_task(instance.ip_address) + estimated_location_service.trigger_estimated_location_task(whois.ip_address) diff --git a/openwisp_controller/geo/estimated_location/service.py b/openwisp_controller/geo/estimated_location/service.py index 0837058d2..791ee9e08 100644 --- a/openwisp_controller/geo/estimated_location/service.py +++ b/openwisp_controller/geo/estimated_location/service.py @@ -1,10 +1,14 @@ import logging -from django.conf import settings +from celery import current_app +from django.core.cache import cache from django.db import transaction from swapper import load_model from openwisp_controller.config import settings as config_app_settings +from openwisp_controller.config.whois.utils import send_whois_task_notification + +from .. import settings as geo_settings logger = logging.getLogger(__name__) @@ -15,27 +19,49 @@ def __init__(self, device): @staticmethod def check_estimated_location_enabled(org_id): + """ + Return whether estimated location is enabled for the given organization. + + OrganizationGeoSettings are cached to avoid a DB hit on every check. + If no settings exist for the organization, an empty instance is used so + that the FallbackBooleanChoiceField can provide the global default. + """ if not org_id: return False if not config_app_settings.WHOIS_CONFIGURED: return False - OrganizationGeoSettings = load_model("geo", "OrganizationGeoSettings") - try: - org_settings = OrganizationGeoSettings.objects.get(organization_id=org_id) - except OrganizationGeoSettings.DoesNotExist: - from .. import settings as geo_app_settings - return geo_app_settings.ESTIMATED_LOCATION_ENABLED + OrganizationGeoSettings = load_model("geo", "OrganizationGeoSettings") + cache_key = EstimatedLocationService.get_cache_key(org_id) + org_settings = cache.get(cache_key) + if org_settings is None: + try: + org_settings = OrganizationGeoSettings.objects.get( + organization_id=org_id + ) + except OrganizationGeoSettings.DoesNotExist: + return geo_settings.estimated_location_enabled + cache.set(cache_key, org_settings, timeout=24 * 7 * 3600) return org_settings.estimated_location_enabled + @staticmethod + def get_cache_key(org_id): + """Return cache key used for caching OrganizationGeoSettings.""" + return f"organization_geo_{org_id}" + + @classmethod + def invalidate_org_settings_cache(cls, instance, **kwargs): + """ + Invalidate the cache for Organization geo settings on update/delete of + OrganizationGeoSettings instance. + """ + cache.delete(cls.get_cache_key(instance.organization_id)) + @property def is_estimated_location_enabled(self): - if not config_app_settings.WHOIS_CONFIGURED: - return False return self.check_estimated_location_enabled(self.device.organization_id) def trigger_estimated_location_task(self, ip_address): - current_app = settings.CELERY_APP current_app.send_task( "whois_estimated_location_task", kwargs={"device_pk": self.device.pk, "ip_address": ip_address}, @@ -66,9 +92,6 @@ def _create_or_update_estimated_location( device_location.location = current_location device_location.full_clean() device_location.save() - from openwisp_controller.config.whois.utils import ( - send_whois_task_notification, - ) send_whois_task_notification( device=self.device, @@ -82,14 +105,11 @@ def _create_or_update_estimated_location( setattr(current_location, attr, value) update_fields.append(attr) if update_fields: + current_location.full_clean() with transaction.atomic(): current_location.save( update_fields=update_fields, _set_estimated=True ) - from openwisp_controller.config.whois.utils import ( - send_whois_task_notification, - ) - send_whois_task_notification( device=self.device, notify_type="estimated_location_updated", diff --git a/openwisp_controller/geo/estimated_location/tests/tests.py b/openwisp_controller/geo/estimated_location/tests/tests.py index c3133a788..c383974f6 100644 --- a/openwisp_controller/geo/estimated_location/tests/tests.py +++ b/openwisp_controller/geo/estimated_location/tests/tests.py @@ -12,14 +12,14 @@ from openwisp_notifications.types import unregister_notification_type from swapper import load_model -from openwisp_controller.config import settings as config_app_settings from openwisp_controller.config.whois.handlers import connect_whois_handlers from openwisp_controller.config.whois.tests.utils import WHOISTransactionMixin -from openwisp_controller.geo import estimated_location from ....tests.utils import TestAdminMixin +from ... import estimated_location from ...tests.utils import TestGeoMixin from ..handlers import register_estimated_location_notification_types +from ..service import EstimatedLocationService, config_app_settings from ..tasks import manage_estimated_locations from .utils import TestEstimatedLocationMixin @@ -210,9 +210,7 @@ def setUp(self): register_estimated_location_notification_types() @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) - @mock.patch( - "openwisp_controller.config.whois.service.WHOISService.trigger_estimated_location_task" # noqa: E501 - ) + @mock.patch.object(EstimatedLocationService, "trigger_estimated_location_task") @mock.patch(_WHOIS_GEOIP_CLIENT) def test_estimated_location_task_called( self, mocked_client, mocked_estimated_location_task @@ -350,13 +348,11 @@ def test_estimated_location_task_called( mocked_estimated_location_task.reset_mock() @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) - @mock.patch("openwisp_controller.config.whois.service.send_whois_task_notification") + @mock.patch("openwisp_controller.config.whois.utils.send_whois_task_notification") @mock.patch( "openwisp_controller.geo.estimated_location.tasks.send_whois_task_notification" ) - @mock.patch( - "openwisp_controller.config.whois.service.WHOISService.trigger_estimated_location_task" # noqa: E501 - ) + @mock.patch.object(EstimatedLocationService, "trigger_estimated_location_task") @mock.patch(_ESTIMATED_LOCATION_INFO_LOGGER) @mock.patch(_WHOIS_GEOIP_CLIENT) def test_estimated_location_creation_and_update( @@ -606,7 +602,7 @@ def test_manage_estimated_location_device_not_found(self, mock_warn): ) @mock.patch( - "openwisp_controller.config.whois.service.current_app.send_task", + "openwisp_controller.geo.estimated_location.service.current_app.send_task", side_effect=TestEstimatedLocationMixin.run_task, ) @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) @@ -638,7 +634,7 @@ def test_estimated_location_handling_on_whois_update( @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) @mock.patch( - "openwisp_controller.config.whois.service.current_app.send_task", + "openwisp_controller.geo.estimated_location.service.current_app.send_task", side_effect=TestEstimatedLocationMixin.run_task, ) @mock.patch(_WHOIS_GEOIP_CLIENT) @@ -685,7 +681,7 @@ def test_unchanged_whois_data_no_location_recreation(self, mock_client, _): @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) @mock.patch( - "openwisp_controller.config.whois.service.current_app.send_task", + "openwisp_controller.geo.estimated_location.service.current_app.send_task", side_effect=TestEstimatedLocationMixin.run_task, ) @mock.patch(_ESTIMATED_LOCATION_INFO_LOGGER) @@ -750,12 +746,13 @@ def _verify_notification(device, messages, notify_level="info"): _verify_notification(device3, messages, "error") @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) - @mock.patch("openwisp_controller.config.whois.service.send_whois_task_notification") + @mock.patch("openwisp_controller.config.whois.utils.send_whois_task_notification") @mock.patch( "openwisp_controller.geo.estimated_location.tasks.send_whois_task_notification" ) - @mock.patch( - "openwisp_controller.config.whois.service.WHOISService.trigger_estimated_location_task" # noqa: E501 + @mock.patch.object( + EstimatedLocationService, + "trigger_estimated_location_task", ) @mock.patch(_WHOIS_GEOIP_CLIENT) def test_manage_estimated_locations_no_coordinates_warning( @@ -782,7 +779,7 @@ def test_manage_estimated_locations_no_coordinates_warning( @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) @mock.patch( - "openwisp_controller.config.whois.service.current_app.send_task", + "openwisp_controller.geo.estimated_location.service.current_app.send_task", side_effect=TestEstimatedLocationMixin.run_task, ) @mock.patch(_WHOIS_GEOIP_CLIENT) @@ -1109,3 +1106,61 @@ def test_estimated_location_filter_admin(self): self.assertContains(response, outdoor_device.id) self.assertContains(response, indoor_device.id) self.assertContains(response, "3 Devices") + + +class TestEstimatedLocationCache(TestEstimatedLocationMixin, TestGeoMixin, TestCase): + """Tests for caching behavior of OrganizationGeoSettings used by + EstimatedLocationService.check_estimated_location_enabled.""" + + @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) + def test_check_estimated_location_enabled_caches_result(self): + org = self._get_org() + cache_key = EstimatedLocationService.get_cache_key(org.pk) + + # When cache is empty, the function should fetch from DB and set cache + with ( + mock.patch("django.core.cache.cache.get", return_value=None) as mocked_get, + mock.patch("django.core.cache.cache.set") as mocked_set, + ): + result = EstimatedLocationService.check_estimated_location_enabled(org.pk) + mocked_get.assert_called_with(cache_key) + # cache.set should be called to store the org settings + mocked_set.assert_called() + self.assertEqual(result, org.geo_settings.estimated_location_enabled) + + @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) + def test_check_estimated_location_enabled_uses_cache(self): + org = self._get_org() + cache_key = EstimatedLocationService.get_cache_key(org.pk) + + # If cache.get returns an instance, no DB lookup should occur + with ( + mock.patch( + "django.core.cache.cache.get", return_value=org.geo_settings + ) as mocked_get, + mock.patch.object( + OrganizationGeoSettings.objects, + "get", + side_effect=AssertionError("DB hit"), + ), + ): + result = EstimatedLocationService.check_estimated_location_enabled(org.pk) + mocked_get.assert_called_with(cache_key) + self.assertEqual(result, org.geo_settings.estimated_location_enabled) + + @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) + def test_cache_invalidation_on_org_geo_settings_change(self): + org = self._get_org() + cache_key = EstimatedLocationService.get_cache_key(org.pk) + + with mock.patch("django.core.cache.cache.delete") as mocked_delete: + # Save should invalidate cache + org.geo_settings.estimated_location_enabled = ( + not org.geo_settings.estimated_location_enabled + ) + org.geo_settings.save() + mocked_delete.assert_called_with(cache_key) + mocked_delete.reset_mock() + # Delete should also invalidate cache + org.geo_settings.delete() + mocked_delete.assert_called_with(cache_key) diff --git a/openwisp_controller/geo/estimated_location/tests/utils.py b/openwisp_controller/geo/estimated_location/tests/utils.py index f56ee3c82..2d7f1f8f4 100644 --- a/openwisp_controller/geo/estimated_location/tests/utils.py +++ b/openwisp_controller/geo/estimated_location/tests/utils.py @@ -18,8 +18,9 @@ def setUp(self): organization=org, whois_enabled=True, ) - # OrganizationGeoSettings is auto-created by signal, - # update estimated_location_enabled + # OrganizationGeoSettings is auto-created by signal on Organization + # creation. Access the related object and update + # estimated_location_enabled to enable the feature for tests. org.geo_settings.estimated_location_enabled = True org.geo_settings.save() diff --git a/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py b/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py index 7c19a1587..66d5393c4 100644 --- a/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py +++ b/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py @@ -13,12 +13,33 @@ def create_geo_settings_for_existing_orgs(apps, schema_editor): Organization = apps.get_model("openwisp_users", "Organization") OrganizationGeoSettings = apps.get_model("geo", "OrganizationGeoSettings") - for org in Organization.objects.all(): + for org in Organization.objects.iterator(): OrganizationGeoSettings.objects.get_or_create(organization_id=org.pk) -class Migration(migrations.Migration): +def copy_estimated_location_enabled(apps, schema_editor): + """ + Copy the boolean field estimated_location_enabled from + OrganizationConfigSettings into OrganizationGeoSettings so that + removing the field from config does not lose data. + """ + OrganizationConfigSettings = apps.get_model("config", "OrganizationConfigSettings") + OrganizationGeoSettings = apps.get_model("geo", "OrganizationGeoSettings") + + # Iterate using iterator() to avoid loading all rows into memory. + for cfg in OrganizationConfigSettings.objects.iterator(): + geo, _ = OrganizationGeoSettings.objects.get_or_create( + organization_id=cfg.organization_id + ) + # Copy the value if different + if getattr(geo, "estimated_location_enabled", None) != getattr( + cfg, "estimated_location_enabled", None + ): + geo.estimated_location_enabled = cfg.estimated_location_enabled + geo.save(update_fields=["estimated_location_enabled"]) + +class Migration(migrations.Migration): dependencies = [ ("geo", "0005_organizationgeosettings"), ("openwisp_users", "0021_rename_user_id_email_openwisp_us_id_06c07a_idx"), @@ -33,4 +54,8 @@ class Migration(migrations.Migration): create_geo_settings_for_existing_orgs, reverse_code=migrations.RunPython.noop, ), + migrations.RunPython( + copy_estimated_location_enabled, + reverse_code=migrations.RunPython.noop, + ), ] diff --git a/openwisp_controller/geo/settings.py b/openwisp_controller/geo/settings.py index b3434b945..7f08d83cd 100644 --- a/openwisp_controller/geo/settings.py +++ b/openwisp_controller/geo/settings.py @@ -1,12 +1,8 @@ -import logging - from django.core.exceptions import ImproperlyConfigured from ..config import settings as config_settings from ..settings import get_setting -logger = logging.getLogger(__name__) - ESTIMATED_LOCATION_ENABLED = get_setting("ESTIMATED_LOCATION_ENABLED", False) # Validate that WHOIS is enabled if estimated location is enabled diff --git a/tests/openwisp2/sample_geo/models.py b/tests/openwisp2/sample_geo/models.py index dc3383255..cb83c0d02 100644 --- a/tests/openwisp2/sample_geo/models.py +++ b/tests/openwisp2/sample_geo/models.py @@ -1,4 +1,3 @@ -import swapper from django.db import models from openwisp_controller.geo.base.models import ( @@ -31,7 +30,7 @@ class Meta(BaseDeviceLocation.Meta): abstract = False -class OrganizationGeoSettings(AbstractOrganizationGeoSettings): +class OrganizationGeoSettings(DetailsModel, AbstractOrganizationGeoSettings): class Meta(AbstractOrganizationGeoSettings.Meta): abstract = False From c1808aec502ae1367cde2ad180b43a534d53f5d1 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Tue, 24 Mar 2026 21:00:51 +0530 Subject: [PATCH 03/10] [fix] Fixes by @coderabbitai --- docs/developer/utils.rst | 14 ++++++++ docs/user/rest-api.rst | 17 +++++++++- openwisp_controller/config/signals.py | 4 +++ openwisp_controller/config/whois/service.py | 4 +++ .../config/whois/tests/tests.py | 13 +++++++- openwisp_controller/geo/admin.py | 2 ++ openwisp_controller/geo/apps.py | 7 +++- .../geo/estimated_location/handlers.py | 13 ++++++++ .../geo/estimated_location/service.py | 21 ++++++++---- .../geo/estimated_location/tests/tests.py | 33 ++++++++++--------- .../geo/estimated_location/tests/utils.py | 11 ++++--- ...6_create_geo_settings_for_existing_orgs.py | 10 +++--- openwisp_controller/geo/settings.py | 6 ++-- openwisp_controller/geo/tests/test_api.py | 2 +- tests/openwisp2/sample_geo/views.py | 8 +++++ 15 files changed, 128 insertions(+), 37 deletions(-) diff --git a/docs/developer/utils.rst b/docs/developer/utils.rst index e861eaaf7..f2270fb7e 100644 --- a/docs/developer/utils.rst +++ b/docs/developer/utils.rst @@ -393,3 +393,17 @@ WireGuard** backend. This signal is emitted when a WHOIS lookup task successfully creates or updates a ``WHOISInfo`` record. + +``whois_lookup_skipped`` +~~~~~~~~~~~~~~~~~~~~~~~~ + +**Path**: ``openwisp_controller.config.signals.whois_lookup_skipped`` + +**Arguments**: + +- ``device``: instance of ``Device`` for which the WHOIS lookup was + skipped + +This signal is emitted when a WHOIS lookup is not triggered because the +lookup conditions were not met (for example, an up-to-date WHOIS record +already exists). diff --git a/docs/user/rest-api.rst b/docs/user/rest-api.rst index 774f3c196..c9702920d 100644 --- a/docs/user/rest-api.rst +++ b/docs/user/rest-api.rst @@ -821,13 +821,28 @@ organization. PUT /api/v1/controller/organization/{organization_pk}/geo-settings/ +.. code-block:: text + + curl -X PUT \ + 'http://127.0.0.1:8000/api/v1/controller/organization/8a85cc23-bad5-4c7e-b9f4-ffe298defb5c/geo-settings/' \ + -H 'authorization: Bearer ' \ + -H 'content-type: application/json' \ + -d '{"estimated_location_enabled": true}' + +Partially Update Organization Geographic Settings +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This endpoint allows partial updates of Organization geographic settings +using the PATCH method. PATCH accepts a subset of fields and applies a +partial update to the resource at the same endpoint path. + .. code-block:: text PATCH /api/v1/controller/organization/{organization_pk}/geo-settings/ .. code-block:: text - curl -X PUT \ + curl -X PATCH \ 'http://127.0.0.1:8000/api/v1/controller/organization/8a85cc23-bad5-4c7e-b9f4-ffe298defb5c/geo-settings/' \ -H 'authorization: Bearer ' \ -H 'content-type: application/json' \ diff --git a/openwisp_controller/config/signals.py b/openwisp_controller/config/signals.py index eeb2bf9b2..a8731ce38 100644 --- a/openwisp_controller/config/signals.py +++ b/openwisp_controller/config/signals.py @@ -69,3 +69,7 @@ whois_fetched.__doc__ = """ Providing arguments: ['whois', 'updated_fields', 'device'] """ +whois_lookup_skipped = Signal() +whois_lookup_skipped.__doc__ = """ +Providing arguments: ['device'] +""" diff --git a/openwisp_controller/config/whois/service.py b/openwisp_controller/config/whois/service.py index 55b14a806..43a6ab3db 100644 --- a/openwisp_controller/config/whois/service.py +++ b/openwisp_controller/config/whois/service.py @@ -11,6 +11,7 @@ from swapper import load_model from openwisp_controller.config import settings as app_settings +from openwisp_controller.config.signals import whois_lookup_skipped from .tasks import fetch_whois_details from .utils import EXCEPTION_MESSAGES @@ -218,6 +219,9 @@ def process_ip_data_and_location(self, force_lookup=False): initial_ip_address=initial_ip, ) ) + elif self.is_whois_enabled and self.is_valid_public_ip_address(new_ip): + # Emit signal when lookup is skipped so receivers can react + whois_lookup_skipped.send(sender=self.__class__, device=self.device) def update_whois_info(self): """ diff --git a/openwisp_controller/config/whois/tests/tests.py b/openwisp_controller/config/whois/tests/tests.py index 0308c3e44..f4ec0b858 100644 --- a/openwisp_controller/config/whois/tests/tests.py +++ b/openwisp_controller/config/whois/tests/tests.py @@ -21,7 +21,7 @@ from selenium.webdriver.common.by import By from swapper import load_model -from openwisp_controller.config.signals import whois_fetched +from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped from openwisp_utils.tests import SeleniumTestMixin, catch_signal from ....tests.utils import TestAdminMixin @@ -461,6 +461,17 @@ def test_fetch_whois_emits_signal(self, mock_client): fetch_whois_details(device_pk=device.pk, initial_ip_address=None) handler.assert_called() + @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) + def test_whois_lookup_skipped_emits_signal(self): + """Assert whois_lookup_skipped is emitted when lookup is not needed.""" + connect_whois_handlers() + ip = "172.217.22.14" + self._create_whois_info(ip_address=ip) + device = self._create_device(last_ip=ip) + with catch_signal(whois_lookup_skipped) as handler: + device.whois_service.process_ip_data_and_location() + handler.assert_called() + @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) @mock.patch("openwisp_controller.config.whois.tasks.fetch_whois_details.delay") def test_whois_task_called(self, mocked_lookup_task): diff --git a/openwisp_controller/geo/admin.py b/openwisp_controller/geo/admin.py index 1a404d3f0..e3afaf378 100644 --- a/openwisp_controller/geo/admin.py +++ b/openwisp_controller/geo/admin.py @@ -211,6 +211,8 @@ def queryset(self, request, queryset): class GeoSettingsInline(admin.StackedInline): model = OrganizationGeoSettings + can_delete = False + max_num = 1 def get_readonly_fields(self, request, obj=None): fields = list(super().get_readonly_fields(request, obj)) diff --git a/openwisp_controller/geo/apps.py b/openwisp_controller/geo/apps.py index 4e7cf1d5c..a52ad66ae 100644 --- a/openwisp_controller/geo/apps.py +++ b/openwisp_controller/geo/apps.py @@ -11,7 +11,7 @@ from django_loci.apps import LociConfig from swapper import get_model_name -from openwisp_controller.config.signals import whois_fetched +from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped from openwisp_utils.admin_theme import register_dashboard_chart from openwisp_utils.admin_theme.menu import register_menu_group @@ -19,6 +19,7 @@ from .estimated_location.handlers import ( register_estimated_location_notification_types, whois_fetched_handler, + whois_lookup_skipped_handler, ) from .estimated_location.service import EstimatedLocationService @@ -69,6 +70,10 @@ def connect_receivers(self): whois_fetched_handler, dispatch_uid="whois_fetched_estimated_location_handler", ) + whois_lookup_skipped.connect( + whois_lookup_skipped_handler, + dispatch_uid="whois_lookup_skipped_estimated_location_handler", + ) def _add_params_to_test_config(self): """ diff --git a/openwisp_controller/geo/estimated_location/handlers.py b/openwisp_controller/geo/estimated_location/handlers.py index ec0b75beb..bb9ec6314 100644 --- a/openwisp_controller/geo/estimated_location/handlers.py +++ b/openwisp_controller/geo/estimated_location/handlers.py @@ -60,3 +60,16 @@ def whois_fetched_handler(sender, whois, updated_fields, device=None, **kwargs): return estimated_location_service = EstimatedLocationService(device) estimated_location_service.trigger_estimated_location_task(whois.ip_address) + + +def whois_lookup_skipped_handler(sender, device, **kwargs): + """ + Handler for skipped WHOIS lookups. If estimated location is enabled for + the device's organization, trigger an estimated location task. + """ + if not EstimatedLocationService.check_estimated_location_enabled( + device.organization_id + ): + return + estimated_location_service = EstimatedLocationService(device) + estimated_location_service.trigger_estimated_location_task(device.last_ip) diff --git a/openwisp_controller/geo/estimated_location/service.py b/openwisp_controller/geo/estimated_location/service.py index 791ee9e08..9b749eadd 100644 --- a/openwisp_controller/geo/estimated_location/service.py +++ b/openwisp_controller/geo/estimated_location/service.py @@ -40,7 +40,9 @@ def check_estimated_location_enabled(org_id): organization_id=org_id ) except OrganizationGeoSettings.DoesNotExist: - return geo_settings.estimated_location_enabled + # Cache a sentinel object (empty settings instance) so subsequent + # calls do not hit the database repeatedly. + org_settings = OrganizationGeoSettings(organization_id=org_id) cache.set(cache_key, org_settings, timeout=24 * 7 * 3600) return org_settings.estimated_location_enabled @@ -62,10 +64,18 @@ def is_estimated_location_enabled(self): return self.check_estimated_location_enabled(self.device.organization_id) def trigger_estimated_location_task(self, ip_address): - current_app.send_task( - "whois_estimated_location_task", - kwargs={"device_pk": self.device.pk, "ip_address": ip_address}, - ) + try: + current_app.send_task( + "whois_estimated_location_task", + kwargs={"device_pk": self.device.pk, "ip_address": ip_address}, + ) + except Exception as exc: # pragma: no cover - defensive logging + logger.error( + "Failed to enqueue estimated location task for device %s ip %s: %s", + getattr(self.device, "pk", None), + ip_address, + exc, + ) def _create_or_update_estimated_location( self, location_defaults, attached_devices_exists @@ -81,7 +91,6 @@ def _create_or_update_estimated_location( device_location = DeviceLocation(content_object=self.device) current_location = device_location.location - if not current_location or ( attached_devices_exists and current_location.is_estimated ): diff --git a/openwisp_controller/geo/estimated_location/tests/tests.py b/openwisp_controller/geo/estimated_location/tests/tests.py index c383974f6..a65716b39 100644 --- a/openwisp_controller/geo/estimated_location/tests/tests.py +++ b/openwisp_controller/geo/estimated_location/tests/tests.py @@ -247,6 +247,11 @@ def test_estimated_location_task_called( org.config_settings, timeout=Config._CHECKSUM_CACHE_TIMEOUT, ), + mock.call( + f"organization_geo_{org.pk}", + org.geo_settings, + timeout=604800, + ), mock.call( f"{self._WHOIS_TASK_NAME}_last_operation", "success", None ), @@ -348,15 +353,13 @@ def test_estimated_location_task_called( mocked_estimated_location_task.reset_mock() @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) - @mock.patch("openwisp_controller.config.whois.utils.send_whois_task_notification") - @mock.patch( - "openwisp_controller.geo.estimated_location.tasks.send_whois_task_notification" - ) + @mock.patch.object(estimated_location.tasks, "send_whois_task_notification") + @mock.patch.object(estimated_location.service, "send_whois_task_notification") @mock.patch.object(EstimatedLocationService, "trigger_estimated_location_task") @mock.patch(_ESTIMATED_LOCATION_INFO_LOGGER) @mock.patch(_WHOIS_GEOIP_CLIENT) def test_estimated_location_creation_and_update( - self, mock_client, mock_info, _mocked_task, _mocked_notify, _mocked_notify2 + self, mock_client, mock_info, *args ): connect_whois_handlers() @@ -411,7 +414,7 @@ def _verify_location_details(device, mocked_response): mock_client.return_value.city.return_value = mocked_response device.save() device.refresh_from_db() - with self.assertNumQueries(7): + with self.assertNumQueries(8): manage_estimated_locations(device.pk, device.last_ip) location = device.devicelocation.location @@ -435,7 +438,7 @@ def _verify_location_details(device, mocked_response): mock_client.return_value.city.return_value = mocked_response device.save() device.refresh_from_db() - with self.assertNumQueries(7): + with self.assertNumQueries(8): manage_estimated_locations(device.pk, device.last_ip) location = device.devicelocation.location @@ -746,10 +749,8 @@ def _verify_notification(device, messages, notify_level="info"): _verify_notification(device3, messages, "error") @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) - @mock.patch("openwisp_controller.config.whois.utils.send_whois_task_notification") - @mock.patch( - "openwisp_controller.geo.estimated_location.tasks.send_whois_task_notification" - ) + @mock.patch.object(estimated_location.tasks, "send_whois_task_notification") + @mock.patch.object(estimated_location.service, "send_whois_task_notification") @mock.patch.object( EstimatedLocationService, "trigger_estimated_location_task", @@ -861,7 +862,7 @@ def test_estimated_location_api_status_configured(self): with self.subTest("Test Estimated Location in Locations List"): path = reverse("geo_api:list_location") - with self.assertNumQueries(6): + with self.assertNumQueries(4): response = self.client.get(path) self.assertEqual(response.status_code, 200) self.assertEqual(response.data["count"], 2) @@ -873,19 +874,19 @@ def test_estimated_location_api_status_configured(self): with self.subTest("Test Estimated Location in Device Locations List"): path = reverse("geo_api:device_location", args=[org1_device.pk]) - with self.assertNumQueries(5): + with self.assertNumQueries(4): response = self.client.get(path) self.assertEqual(response.status_code, 200) self.assertIn("is_estimated", response.data["location"]["properties"]) path = reverse("geo_api:device_location", args=[org2_device.pk]) - with self.assertNumQueries(5): + with self.assertNumQueries(4): response = self.client.get(path) self.assertEqual(response.status_code, 200) self.assertNotIn("is_estimated", response.data["location"]["properties"]) with self.subTest("Test Estimated Location in GeoJSON List"): path = reverse("geo_api:location_geojson") - with self.assertNumQueries(5): + with self.assertNumQueries(3): response = self.client.get(path) self.assertEqual(response.status_code, 200) self.assertEqual(response.data["count"], 2) @@ -953,7 +954,7 @@ def test_estimated_location_filter_list_api(self): "Test Estimated Location filter available in location list " "when WHOIS is configured" ): - with self.assertNumQueries(5): + with self.assertNumQueries(4): response = self.client.get(path, {"is_estimated": True}) self.assertEqual(response.status_code, 200) self.assertEqual(response.data["count"], 1) diff --git a/openwisp_controller/geo/estimated_location/tests/utils.py b/openwisp_controller/geo/estimated_location/tests/utils.py index 2d7f1f8f4..dfc92bfb2 100644 --- a/openwisp_controller/geo/estimated_location/tests/utils.py +++ b/openwisp_controller/geo/estimated_location/tests/utils.py @@ -18,11 +18,12 @@ def setUp(self): organization=org, whois_enabled=True, ) - # OrganizationGeoSettings is auto-created by signal on Organization - # creation. Access the related object and update - # estimated_location_enabled to enable the feature for tests. - org.geo_settings.estimated_location_enabled = True - org.geo_settings.save() + # Ensure OrganizationGeoSettings exists (signals usually create it on + # Organization creation, but make this explicit to avoid RelatedObject + # errors in some test setups). + org_geo_settings, _ = OrganizationGeoSettings.objects.get_or_create(organization=org) + org_geo_settings.estimated_location_enabled = True + org_geo_settings.save() # helper to mock send_task to directly call the task function synchronously @staticmethod diff --git a/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py b/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py index 66d5393c4..4858ab71b 100644 --- a/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py +++ b/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py @@ -3,7 +3,7 @@ from django.db import migrations from . import assign_geo_settings_permissions_to_groups - +from ...migrations import get_swapped_model def create_geo_settings_for_existing_orgs(apps, schema_editor): """ @@ -11,7 +11,7 @@ def create_geo_settings_for_existing_orgs(apps, schema_editor): that don't have one yet. """ Organization = apps.get_model("openwisp_users", "Organization") - OrganizationGeoSettings = apps.get_model("geo", "OrganizationGeoSettings") + OrganizationGeoSettings = get_swapped_model(apps, "geo", "OrganizationGeoSettings") for org in Organization.objects.iterator(): OrganizationGeoSettings.objects.get_or_create(organization_id=org.pk) @@ -23,8 +23,10 @@ def copy_estimated_location_enabled(apps, schema_editor): OrganizationConfigSettings into OrganizationGeoSettings so that removing the field from config does not lose data. """ - OrganizationConfigSettings = apps.get_model("config", "OrganizationConfigSettings") - OrganizationGeoSettings = apps.get_model("geo", "OrganizationGeoSettings") + OrganizationConfigSettings = get_swapped_model( + apps, "config", "OrganizationConfigSettings" + ) + OrganizationGeoSettings = get_swapped_model(apps, "geo", "OrganizationGeoSettings") # Iterate using iterator() to avoid loading all rows into memory. for cfg in OrganizationConfigSettings.objects.iterator(): diff --git a/openwisp_controller/geo/settings.py b/openwisp_controller/geo/settings.py index 7f08d83cd..08008fbcf 100644 --- a/openwisp_controller/geo/settings.py +++ b/openwisp_controller/geo/settings.py @@ -5,9 +5,11 @@ ESTIMATED_LOCATION_ENABLED = get_setting("ESTIMATED_LOCATION_ENABLED", False) -# Validate that WHOIS is enabled if estimated location is enabled if ESTIMATED_LOCATION_ENABLED: - if not config_settings.WHOIS_ENABLED: + # Ensure WHOIS is properly configured (credentials present) when + # estimated location is enabled. WHOIS_CONFIGURED checks for required + # credentials like WHOIS_GEOIP_ACCOUNT/WHOIS_GEOIP_KEY. + if not config_settings.WHOIS_CONFIGURED: raise ImproperlyConfigured( "OPENWISP_CONTROLLER_WHOIS_ENABLED must be set to True before " "setting OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED to True." diff --git a/openwisp_controller/geo/tests/test_api.py b/openwisp_controller/geo/tests/test_api.py index 748a49d6d..4827347c6 100644 --- a/openwisp_controller/geo/tests/test_api.py +++ b/openwisp_controller/geo/tests/test_api.py @@ -693,7 +693,7 @@ def test_change_location_type_to_outdoor_api(self): def test_delete_location_detail(self): l1 = self._create_location() path = reverse("geo_api:detail_location", args=[l1.pk]) - with self.assertNumQueries(5): + with self.assertNumQueries(8): response = self.client.delete(path) self.assertEqual(response.status_code, 204) diff --git a/tests/openwisp2/sample_geo/views.py b/tests/openwisp2/sample_geo/views.py index 6aae10d31..1e0e24049 100644 --- a/tests/openwisp2/sample_geo/views.py +++ b/tests/openwisp2/sample_geo/views.py @@ -25,6 +25,9 @@ from openwisp_controller.geo.api.views import ( LocationListCreateView as BaseLocationListCreateView, ) +from openwisp_controller.geo.api.views import ( + OrganizationGeoSettingsView as BaseOrganizationGeoSettingsView, +) class DeviceCoordinatesView(BaseDeviceCoordinatesView): @@ -63,6 +66,10 @@ class IndoorCoordinatesList(BaseIndoorCoordinatesList): pass +class OrganizationGeoSettingsView(BaseOrganizationGeoSettingsView): + pass + + device_coordinates = DeviceCoordinatesView.as_view() device_location = DeviceLocationView.as_view() geojson = GeoJsonLocationList.as_view() @@ -72,3 +79,4 @@ class IndoorCoordinatesList(BaseIndoorCoordinatesList): detail_floorplan = FloorPlanDetailView.as_view() list_location = LocationListCreateView.as_view() detail_location = LocationDetailView.as_view() +organization_geo_settings = OrganizationGeoSettingsView.as_view() From cc3d21a6694ab381e2cf1d29c251804f263bc13a Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Tue, 24 Mar 2026 22:07:14 +0530 Subject: [PATCH 04/10] [tests] Fixed tests --- .../geo/estimated_location/tests/tests.py | 2 +- openwisp_controller/geo/settings.py | 6 ++---- openwisp_controller/geo/tests/test_api.py | 18 ++++++++++++++---- openwisp_controller/geo/tests/utils.py | 2 +- 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/openwisp_controller/geo/estimated_location/tests/tests.py b/openwisp_controller/geo/estimated_location/tests/tests.py index a65716b39..f6f6bd7bc 100644 --- a/openwisp_controller/geo/estimated_location/tests/tests.py +++ b/openwisp_controller/geo/estimated_location/tests/tests.py @@ -392,7 +392,7 @@ def _verify_location_details(device, mocked_response): with self.subTest("Test Estimated location created when device is created"): device = self._create_device(last_ip="172.217.22.14") - with self.assertNumQueries(13): + with self.assertNumQueries(14): manage_estimated_locations(device.pk, device.last_ip) location = device.devicelocation.location mocked_response.ip_address = device.last_ip diff --git a/openwisp_controller/geo/settings.py b/openwisp_controller/geo/settings.py index 08008fbcf..7f08d83cd 100644 --- a/openwisp_controller/geo/settings.py +++ b/openwisp_controller/geo/settings.py @@ -5,11 +5,9 @@ ESTIMATED_LOCATION_ENABLED = get_setting("ESTIMATED_LOCATION_ENABLED", False) +# Validate that WHOIS is enabled if estimated location is enabled if ESTIMATED_LOCATION_ENABLED: - # Ensure WHOIS is properly configured (credentials present) when - # estimated location is enabled. WHOIS_CONFIGURED checks for required - # credentials like WHOIS_GEOIP_ACCOUNT/WHOIS_GEOIP_KEY. - if not config_settings.WHOIS_CONFIGURED: + if not config_settings.WHOIS_ENABLED: raise ImproperlyConfigured( "OPENWISP_CONTROLLER_WHOIS_ENABLED must be set to True before " "setting OPENWISP_CONTROLLER_ESTIMATED_LOCATION_ENABLED to True." diff --git a/openwisp_controller/geo/tests/test_api.py b/openwisp_controller/geo/tests/test_api.py index 4827347c6..65fde0ca8 100644 --- a/openwisp_controller/geo/tests/test_api.py +++ b/openwisp_controller/geo/tests/test_api.py @@ -693,7 +693,7 @@ def test_change_location_type_to_outdoor_api(self): def test_delete_location_detail(self): l1 = self._create_location() path = reverse("geo_api:detail_location", args=[l1.pk]) - with self.assertNumQueries(8): + with self.assertNumQueries(5): response = self.client.delete(path) self.assertEqual(response.status_code, 204) @@ -1377,6 +1377,7 @@ def test_organization_geo_settings_retrieve(self): org2_url = reverse("geo_api:organization_geo_settings", args=[org2.pk]) with self.subTest("Unauthenticated access"): + self.client.logout() response = self.client.get(url) self.assertEqual(response.status_code, 401) response = self.client.put( @@ -1518,7 +1519,7 @@ def test_organization_geo_settings_update(self): self.assertEqual(response.status_code, 400) self.assertIn("estimated_location_enabled", response.data) org1_geo_settings.refresh_from_db() - self.assertEqual(org1_geo_settings.estimated_location_enabled, True) + self.assertEqual(org1_geo_settings.estimated_location_enabled, False) response = self.client.patch( url, @@ -1645,14 +1646,23 @@ def test_organization_geo_settings_non_existent_organization(self): self.assertEqual(response.status_code, 404) with self.subTest("Cannot update without change permission"): + # remove change permission for this user and attempt update on a + # real organization's geo settings URL so the request fails with + # HTTP 403 (forbidden) instead of 404 (not found). + content_type = ContentType.objects.get_for_model(OrganizationGeoSettings) + change_perm = content_type.permission_set.get( + codename=f"change_{OrganizationGeoSettings._meta.model_name}" + ) + admin_user.user_permissions.remove(change_perm) + real_url = reverse("geo_api:organization_geo_settings", args=[org.pk]) response = self.client.put( - url, + real_url, {"estimated_location_enabled": False}, content_type="application/json", ) self.assertEqual(response.status_code, 403) response = self.client.patch( - url, + real_url, {"estimated_location_enabled": False}, content_type="application/json", ) diff --git a/openwisp_controller/geo/tests/utils.py b/openwisp_controller/geo/tests/utils.py index a81fcf331..3dd6880c0 100644 --- a/openwisp_controller/geo/tests/utils.py +++ b/openwisp_controller/geo/tests/utils.py @@ -21,7 +21,7 @@ def _create_organization(self, **kwargs): def _add_default_org(self, kwargs): if "organization" not in kwargs: - kwargs["organization"] = self._create_organization() + kwargs["organization"] = self._get_org() return kwargs def _create_object(self, **kwargs): From aa8ff5b6b68d03dc273d502393ad5ff557598dc3 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Tue, 24 Mar 2026 23:33:53 +0530 Subject: [PATCH 05/10] [fix] Fixed tests --- .../config/whois/tests/tests.py | 3 +- openwisp_controller/geo/apps.py | 20 +++---- .../geo/estimated_location/service.py | 2 - .../geo/estimated_location/tests/tests.py | 3 +- .../geo/estimated_location/tests/utils.py | 4 +- .../0005_organizationgeosettings.py | 2 +- ...6_create_geo_settings_for_existing_orgs.py | 3 +- openwisp_controller/geo/tests/utils.py | 2 +- openwisp_controller/tests/mixins.py | 5 ++ ...s_approximate_location_enabled_and_more.py | 12 ---- .../0005_organizationgeosettings.py | 60 +++++++++++++++++++ 11 files changed, 84 insertions(+), 32 deletions(-) create mode 100644 tests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.py diff --git a/openwisp_controller/config/whois/tests/tests.py b/openwisp_controller/config/whois/tests/tests.py index f4ec0b858..05aaf644e 100644 --- a/openwisp_controller/config/whois/tests/tests.py +++ b/openwisp_controller/config/whois/tests/tests.py @@ -456,9 +456,8 @@ def test_process_whois_details_handles_missing_coordinates(self, mock_client): def test_fetch_whois_emits_signal(self, mock_client): connect_whois_handlers() mock_client.return_value.city.return_value = self._mocked_client_response() - device = self._create_device(last_ip="172.217.22.14") with catch_signal(whois_fetched) as handler: - fetch_whois_details(device_pk=device.pk, initial_ip_address=None) + self._create_device(last_ip="172.217.22.14") handler.assert_called() @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) diff --git a/openwisp_controller/geo/apps.py b/openwisp_controller/geo/apps.py index a52ad66ae..b3ac812d0 100644 --- a/openwisp_controller/geo/apps.py +++ b/openwisp_controller/geo/apps.py @@ -15,7 +15,6 @@ from openwisp_utils.admin_theme import register_dashboard_chart from openwisp_utils.admin_theme.menu import register_menu_group -from ..config import settings as config_app_settings from .estimated_location.handlers import ( register_estimated_location_notification_types, whois_fetched_handler, @@ -64,16 +63,15 @@ def connect_receivers(self): sender=self.org_geo_settings_model, dispatch_uid="invalidate_org_geo_settings_cache_on_delete", ) - if config_app_settings.WHOIS_CONFIGURED: - # connect estimated location handler to whois_fetched signal - whois_fetched.connect( - whois_fetched_handler, - dispatch_uid="whois_fetched_estimated_location_handler", - ) - whois_lookup_skipped.connect( - whois_lookup_skipped_handler, - dispatch_uid="whois_lookup_skipped_estimated_location_handler", - ) + # connect estimated location handler to whois_fetched signal + whois_fetched.connect( + whois_fetched_handler, + dispatch_uid="whois_fetched_estimated_location_handler", + ) + whois_lookup_skipped.connect( + whois_lookup_skipped_handler, + dispatch_uid="whois_lookup_skipped_estimated_location_handler", + ) def _add_params_to_test_config(self): """ diff --git a/openwisp_controller/geo/estimated_location/service.py b/openwisp_controller/geo/estimated_location/service.py index 9b749eadd..a9fc2443d 100644 --- a/openwisp_controller/geo/estimated_location/service.py +++ b/openwisp_controller/geo/estimated_location/service.py @@ -8,8 +8,6 @@ from openwisp_controller.config import settings as config_app_settings from openwisp_controller.config.whois.utils import send_whois_task_notification -from .. import settings as geo_settings - logger = logging.getLogger(__name__) diff --git a/openwisp_controller/geo/estimated_location/tests/tests.py b/openwisp_controller/geo/estimated_location/tests/tests.py index f6f6bd7bc..ffac3556e 100644 --- a/openwisp_controller/geo/estimated_location/tests/tests.py +++ b/openwisp_controller/geo/estimated_location/tests/tests.py @@ -93,6 +93,7 @@ def test_estimated_location_configuration_setting(self): response, 'name="geo_settings-0-estimated_location_enabled"' ) + @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) def test_organization_geo_settings_validation(self): """Test OrganizationGeoSettings model validation.""" org = self._get_org() @@ -392,7 +393,7 @@ def _verify_location_details(device, mocked_response): with self.subTest("Test Estimated location created when device is created"): device = self._create_device(last_ip="172.217.22.14") - with self.assertNumQueries(14): + with self.assertNumQueries(13): manage_estimated_locations(device.pk, device.last_ip) location = device.devicelocation.location mocked_response.ip_address = device.last_ip diff --git a/openwisp_controller/geo/estimated_location/tests/utils.py b/openwisp_controller/geo/estimated_location/tests/utils.py index dfc92bfb2..a2dea2f13 100644 --- a/openwisp_controller/geo/estimated_location/tests/utils.py +++ b/openwisp_controller/geo/estimated_location/tests/utils.py @@ -21,7 +21,9 @@ def setUp(self): # Ensure OrganizationGeoSettings exists (signals usually create it on # Organization creation, but make this explicit to avoid RelatedObject # errors in some test setups). - org_geo_settings, _ = OrganizationGeoSettings.objects.get_or_create(organization=org) + org_geo_settings, _ = OrganizationGeoSettings.objects.get_or_create( + organization=org + ) org_geo_settings.estimated_location_enabled = True org_geo_settings.save() diff --git a/openwisp_controller/geo/migrations/0005_organizationgeosettings.py b/openwisp_controller/geo/migrations/0005_organizationgeosettings.py index 4bb42362b..8ba0866a4 100644 --- a/openwisp_controller/geo/migrations/0005_organizationgeosettings.py +++ b/openwisp_controller/geo/migrations/0005_organizationgeosettings.py @@ -45,7 +45,7 @@ class Migration(migrations.Migration): models.OneToOneField( on_delete=django.db.models.deletion.CASCADE, related_name="geo_settings", - to="openwisp_users.organization", + to=swapper.get_model_name("openwisp_users", "Organization"), verbose_name="organization", ), ), diff --git a/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py b/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py index 4858ab71b..c9789b7b4 100644 --- a/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py +++ b/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py @@ -2,8 +2,9 @@ from django.db import migrations -from . import assign_geo_settings_permissions_to_groups from ...migrations import get_swapped_model +from . import assign_geo_settings_permissions_to_groups + def create_geo_settings_for_existing_orgs(apps, schema_editor): """ diff --git a/openwisp_controller/geo/tests/utils.py b/openwisp_controller/geo/tests/utils.py index 3dd6880c0..a81fcf331 100644 --- a/openwisp_controller/geo/tests/utils.py +++ b/openwisp_controller/geo/tests/utils.py @@ -21,7 +21,7 @@ def _create_organization(self, **kwargs): def _add_default_org(self, kwargs): if "organization" not in kwargs: - kwargs["organization"] = self._get_org() + kwargs["organization"] = self._create_organization() return kwargs def _create_object(self, **kwargs): diff --git a/openwisp_controller/tests/mixins.py b/openwisp_controller/tests/mixins.py index ae2efa259..c24eb5dcb 100644 --- a/openwisp_controller/tests/mixins.py +++ b/openwisp_controller/tests/mixins.py @@ -11,6 +11,11 @@ def _get_org_edit_form_inline_params(self, user, org): "config_settings-INITIAL_FORMS": 0, "config_settings-MIN_NUM_FORMS": 0, "config_settings-MAX_NUM_FORMS": 0, + # geo inline + "geo_settings-TOTAL_FORMS": 0, + "geo_settings-INITIAL_FORMS": 0, + "geo_settings-MIN_NUM_FORMS": 0, + "geo_settings-MAX_NUM_FORMS": 0, # device limit inline "config_limits-TOTAL_FORMS": 0, "config_limits-INITIAL_FORMS": 0, diff --git a/tests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.py b/tests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.py index f94921b0c..ae3a21b0b 100644 --- a/tests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.py +++ b/tests/openwisp2/sample_config/migrations/0009_organizationconfigsettings_approximate_location_enabled_and_more.py @@ -13,18 +13,6 @@ class Migration(migrations.Migration): ] operations = [ - migrations.AddField( - model_name="organizationconfigsettings", - name="estimated_location_enabled", - field=openwisp_utils.fields.FallbackBooleanChoiceField( - blank=True, - default=None, - fallback=False, - help_text="Whether the estimated location feature is enabled", - null=True, - verbose_name="Estimated Location Enabled", - ), - ), migrations.AddField( model_name="whoisinfo", name="coordinates", diff --git a/tests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.py b/tests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.py new file mode 100644 index 000000000..db2e2f844 --- /dev/null +++ b/tests/openwisp2/sample_geo/migrations/0005_organizationgeosettings.py @@ -0,0 +1,60 @@ +# Generated by Django 5.2.12 on 2026-03-24 19:45 + +import uuid + +import django.db.models.deletion +from django.db import migrations, models +from swapper import get_model_name + +import openwisp_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ("sample_geo", "0004_location_is_estimated"), + ("sample_users", "0004_default_groups"), + ] + + operations = [ + migrations.CreateModel( + name="OrganizationGeoSettings", + fields=[ + ( + "id", + models.UUIDField( + default=uuid.uuid4, + editable=False, + primary_key=True, + serialize=False, + ), + ), + ( + "estimated_location_enabled", + openwisp_utils.fields.FallbackBooleanChoiceField( + blank=True, + default=None, + fallback=False, + help_text="Whether the estimated location feature is enabled", + null=True, + verbose_name="Estimated Location Enabled", + ), + ), + ("details", models.CharField(blank=True, max_length=64, null=True)), + ( + "organization", + models.OneToOneField( + on_delete=django.db.models.deletion.CASCADE, + related_name="geo_settings", + to=get_model_name("openwisp_users", "Organization"), + verbose_name="organization", + ), + ), + ], + options={ + "verbose_name": "Geographic settings", + "verbose_name_plural": "Geographic settings", + "abstract": False, + }, + ), + ] From 9b336071c6f9669d574fb27e2618f7a86e50d5e0 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 25 Mar 2026 15:27:09 +0530 Subject: [PATCH 06/10] [fix] Fixed tests --- openwisp_controller/geo/tests/test_api.py | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/openwisp_controller/geo/tests/test_api.py b/openwisp_controller/geo/tests/test_api.py index 65fde0ca8..5276f3b0a 100644 --- a/openwisp_controller/geo/tests/test_api.py +++ b/openwisp_controller/geo/tests/test_api.py @@ -1448,6 +1448,7 @@ def test_organization_geo_settings_retrieve(self): response = self.client.get(org2_url) self.assertEqual(response.status_code, 200) + @patch.object(config_app_settings, "WHOIS_CONFIGURED", True) def test_organization_geo_settings_update(self): org1 = self._create_org(name="Org 1") org2 = self._create_org(name="Org 2") @@ -1509,26 +1510,6 @@ def test_organization_geo_settings_update(self): ) self.assertEqual(response.status_code, 404) - with self.subTest("Validation error when WHOIS not configured"): - with patch.object(config_app_settings, "WHOIS_CONFIGURED", False): - response = self.client.put( - url, - {"estimated_location_enabled": True}, - content_type="application/json", - ) - self.assertEqual(response.status_code, 400) - self.assertIn("estimated_location_enabled", response.data) - org1_geo_settings.refresh_from_db() - self.assertEqual(org1_geo_settings.estimated_location_enabled, False) - - response = self.client.patch( - url, - {"estimated_location_enabled": True}, - content_type="application/json", - ) - self.assertEqual(response.status_code, 400) - self.assertIn("estimated_location_enabled", response.data) - with self.subTest("Superuser can update any organization's geo settings"): superuser = self._get_admin() self.client.force_login(user=superuser) From aae73a5de80540598bb56e2844d338824f6b25e4 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 25 Mar 2026 19:12:28 +0530 Subject: [PATCH 07/10] [fix] Fixed the OrganizationGeoSettingsSerializer --- openwisp_controller/geo/api/serializers.py | 16 +++++++++++++++- openwisp_controller/geo/tests/test_api.py | 20 ++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/openwisp_controller/geo/api/serializers.py b/openwisp_controller/geo/api/serializers.py index 4d60d7a1e..3bb4b3358 100644 --- a/openwisp_controller/geo/api/serializers.py +++ b/openwisp_controller/geo/api/serializers.py @@ -1,6 +1,8 @@ +from copy import copy + from django.contrib.humanize.templatetags.humanize import ordinal from django.core.exceptions import ValidationError -from django.db import transaction +from django.db import models, transaction from django.urls import reverse from django.utils.translation import gettext_lazy as _ from rest_framework import serializers @@ -29,6 +31,18 @@ class Meta: fields = "__all__" read_only_fields = ["id", "organization"] + # Workaround for https://github.com/openwisp/openwisp-utils/issues/633 + # TODO: Remove when the Bug is fixed in openwisp-utils + def validate(self, data): + Model = self.Meta.model + instance = copy(self.instance) if self.instance else Model() + for key, value in data.items(): + # avoid direct assignment for m2m (not allowed) + if not isinstance(Model._meta.get_field(key), models.ManyToManyField): + setattr(instance, key, value) + instance.full_clean(exclude=self.exclude_validation) + return data + class LocationDeviceSerializer(ValidatedModelSerializer): admin_edit_url = SerializerMethodField("get_admin_edit_url") diff --git a/openwisp_controller/geo/tests/test_api.py b/openwisp_controller/geo/tests/test_api.py index 5276f3b0a..010e29572 100644 --- a/openwisp_controller/geo/tests/test_api.py +++ b/openwisp_controller/geo/tests/test_api.py @@ -1510,6 +1510,26 @@ def test_organization_geo_settings_update(self): ) self.assertEqual(response.status_code, 404) + with self.subTest("Validation error when WHOIS not configured"): + with patch.object(config_app_settings, "WHOIS_CONFIGURED", False): + response = self.client.put( + url, + {"estimated_location_enabled": True}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + self.assertIn("estimated_location_enabled", response.data) + org1_geo_settings.refresh_from_db() + self.assertEqual(org1_geo_settings.estimated_location_enabled, False) + + response = self.client.patch( + url, + {"estimated_location_enabled": True}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + self.assertIn("estimated_location_enabled", response.data) + with self.subTest("Superuser can update any organization's geo settings"): superuser = self._get_admin() self.client.force_login(user=superuser) From 364ab5d74711b70a5905ac31de1cc9bc67a43b45 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 25 Mar 2026 22:00:43 +0530 Subject: [PATCH 08/10] [fix] Fixed the order of inline admins --- ...ationconfigsettings_estimated_location_enabled_and_more.py | 3 +++ openwisp_controller/geo/admin.py | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py b/openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py index ec96dc609..578427363 100644 --- a/openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py +++ b/openwisp_controller/config/migrations/0064_remove_organizationconfigsettings_estimated_location_enabled_and_more.py @@ -9,6 +9,9 @@ class Migration(migrations.Migration): "config", "0063_organizationconfigsettings_estimated_location_enabled_and_more", ), + # This dependency on the geo app is required to ensure that + # the data from OrganizationConfigSettings is properly migrated to GeoSettings + # before we remove the field. ("geo", "0006_create_geo_settings_for_existing_orgs"), ] diff --git a/openwisp_controller/geo/admin.py b/openwisp_controller/geo/admin.py index e3afaf378..c890ad4a7 100644 --- a/openwisp_controller/geo/admin.py +++ b/openwisp_controller/geo/admin.py @@ -18,9 +18,9 @@ from ..admin import MultitenantAdminMixin from ..config.admin import ( + ConfigSettingsInline, DeactivatedDeviceReadOnlyMixin, DeviceAdminExportable, - OrganizationLimitsInline, ) from .estimated_location.service import EstimatedLocationService from .exportable import GeoDeviceResource @@ -227,7 +227,7 @@ def get_readonly_fields(self, request, obj=None): OrganizationAdmin.inlines.insert( - OrganizationAdmin.inlines.index(OrganizationLimitsInline) + 1, + OrganizationAdmin.inlines.index(ConfigSettingsInline) + 1, GeoSettingsInline, ) # Prepend DeviceLocationInline to config.DeviceAdminExportable From 57807fda36357a07618e3bb53347c5b0ae2a6784 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 27 Mar 2026 15:29:43 +0530 Subject: [PATCH 09/10] [fix] Fixes by @coderabbitai --- openwisp_controller/geo/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openwisp_controller/geo/admin.py b/openwisp_controller/geo/admin.py index c890ad4a7..c29bcb0d1 100644 --- a/openwisp_controller/geo/admin.py +++ b/openwisp_controller/geo/admin.py @@ -227,7 +227,7 @@ def get_readonly_fields(self, request, obj=None): OrganizationAdmin.inlines.insert( - OrganizationAdmin.inlines.index(ConfigSettingsInline) + 1, + max(0, OrganizationAdmin.inlines.index(ConfigSettingsInline) + 1), GeoSettingsInline, ) # Prepend DeviceLocationInline to config.DeviceAdminExportable From 3816541c20124f9910174dc4cbc53f87fcb16261 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 27 Mar 2026 21:49:18 +0530 Subject: [PATCH 10/10] [fix] Fixes by @coderabbitai --- openwisp_controller/geo/api/views.py | 8 ++++---- .../geo/estimated_location/service.py | 7 +++++++ ...6_create_geo_settings_for_existing_orgs.py | 19 +++++++++++++------ openwisp_controller/geo/tests/test_api.py | 2 ++ 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/openwisp_controller/geo/api/views.py b/openwisp_controller/geo/api/views.py index b3825055c..ffc1ce18b 100644 --- a/openwisp_controller/geo/api/views.py +++ b/openwisp_controller/geo/api/views.py @@ -5,6 +5,7 @@ from django_filters import rest_framework as filters from rest_framework import generics, pagination, status from rest_framework.exceptions import NotFound, PermissionDenied +from rest_framework.generics import get_object_or_404 from rest_framework.permissions import BasePermission from rest_framework.request import clone_request from rest_framework.response import Response @@ -352,10 +353,9 @@ class OrganizationGeoSettingsView(ProtectedAPIMixin, generics.RetrieveUpdateAPIV def get_object(self): org_id = self.kwargs.get("organization_pk") - try: - return self.get_queryset().get(organization_id=org_id) - except OrganizationGeoSettings.DoesNotExist: - raise Http404(_("Geo settings not found for this organization.")) + obj = get_object_or_404(self.get_queryset(), organization_id=org_id) + self.check_object_permissions(self.request, obj) + return obj # add with_geo filter to device API diff --git a/openwisp_controller/geo/estimated_location/service.py b/openwisp_controller/geo/estimated_location/service.py index a9fc2443d..64f753530 100644 --- a/openwisp_controller/geo/estimated_location/service.py +++ b/openwisp_controller/geo/estimated_location/service.py @@ -89,6 +89,13 @@ def _create_or_update_estimated_location( device_location = DeviceLocation(content_object=self.device) current_location = device_location.location + # Re-check whether estimated locations are enabled for the device's + # organization. The check is needed here so the celery worker + # honors current org settings and avoids persisting estimated + # locations when the feature has been disabled since the task was + # enqueued. + if not self.check_estimated_location_enabled(self.device.organization_id): + return current_location if not current_location or ( attached_devices_exists and current_location.is_estimated ): diff --git a/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py b/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py index c9789b7b4..fe7c4d520 100644 --- a/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py +++ b/openwisp_controller/geo/migrations/0006_create_geo_settings_for_existing_orgs.py @@ -11,11 +11,15 @@ def create_geo_settings_for_existing_orgs(apps, schema_editor): Create OrganizationGeoSettings for all existing organizations that don't have one yet. """ - Organization = apps.get_model("openwisp_users", "Organization") + Organization = get_swapped_model(apps, "openwisp_users", "Organization") OrganizationGeoSettings = get_swapped_model(apps, "geo", "OrganizationGeoSettings") - for org in Organization.objects.iterator(): - OrganizationGeoSettings.objects.get_or_create(organization_id=org.pk) + # Use the migration's DB alias so non-default databases are respected + db_alias = schema_editor.connection.alias + for org in Organization.objects.using(db_alias).iterator(): + OrganizationGeoSettings.objects.using(db_alias).get_or_create( + organization_id=org.pk + ) def copy_estimated_location_enabled(apps, schema_editor): @@ -29,9 +33,11 @@ def copy_estimated_location_enabled(apps, schema_editor): ) OrganizationGeoSettings = get_swapped_model(apps, "geo", "OrganizationGeoSettings") + # Use the migration's DB alias so non-default databases are respected + db_alias = schema_editor.connection.alias # Iterate using iterator() to avoid loading all rows into memory. - for cfg in OrganizationConfigSettings.objects.iterator(): - geo, _ = OrganizationGeoSettings.objects.get_or_create( + for cfg in OrganizationConfigSettings.objects.using(db_alias).iterator(): + geo, _ = OrganizationGeoSettings.objects.using(db_alias).get_or_create( organization_id=cfg.organization_id ) # Copy the value if different @@ -39,7 +45,8 @@ def copy_estimated_location_enabled(apps, schema_editor): cfg, "estimated_location_enabled", None ): geo.estimated_location_enabled = cfg.estimated_location_enabled - geo.save(update_fields=["estimated_location_enabled"]) + # Ensure save uses the same DB alias + geo.save(update_fields=["estimated_location_enabled"], using=db_alias) class Migration(migrations.Migration): diff --git a/openwisp_controller/geo/tests/test_api.py b/openwisp_controller/geo/tests/test_api.py index 010e29572..7bf8cadf9 100644 --- a/openwisp_controller/geo/tests/test_api.py +++ b/openwisp_controller/geo/tests/test_api.py @@ -1529,6 +1529,8 @@ def test_organization_geo_settings_update(self): ) self.assertEqual(response.status_code, 400) self.assertIn("estimated_location_enabled", response.data) + org1_geo_settings.refresh_from_db() + self.assertEqual(org1_geo_settings.estimated_location_enabled, False) with self.subTest("Superuser can update any organization's geo settings"): superuser = self._get_admin()