[qa] Refactoring, added validations, test cases

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

Author: DragnEmperor

openwisp_controller/config/whois/service.py

@@ -43,42 +43,48 @@ def _get_whois_info_from_db(ip_address):
 
         return WHOISInfo.objects.filter(ip_address=ip_address)
 
-    @property
-    def is_whois_enabled(self):
+    @staticmethod
+    def get_org_config_settings(org_id):
         """
-        Check if the WHOIS lookup feature is enabled.
-        The OrganizationConfigSettings are cached as these settings
+        Caches the OrganizationConfigSettings as these settings
         are not expected to change frequently. The timeout for the cache
         is set to the same as the checksum cache timeout for consistency
         with DeviceChecksumView.
         """
         OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings")
         Config = load_model("config", "Config")
 
-        org_id = self.device.organization.pk
-        org_settings = cache.get(self.get_cache_key(org_id=org_id))
+        cache_key = WHOISService.get_cache_key(org_id=org_id)
+        org_settings = cache.get(cache_key)
         if org_settings is None:
             try:
                 org_settings = OrganizationConfigSettings.objects.get(
                     organization=org_id
                 )
             except OrganizationConfigSettings.DoesNotExist:
                 # If organization settings do not exist, fall back to global setting
-                return app_settings.WHOIS_ENABLED
+                return None
             cache.set(
-                self.get_cache_key(org_id=org_id),
+                cache_key,
                 org_settings,
                 timeout=Config._CHECKSUM_CACHE_TIMEOUT,
             )
+        return org_settings
+
+    @property
+    def is_whois_enabled(self):
+        """
+        Check if the WHOIS lookup feature is enabled.
+        """
+        org_settings = self.get_org_config_settings(org_id=self.device.organization.pk)
         return getattr(org_settings, "whois_enabled", app_settings.WHOIS_ENABLED)
 
     @property
     def is_estimated_location_enabled(self):
         """
         Check if the Estimated location feature is enabled.
-        This does not require to set cache as `is_whois_enabled` already sets it
         """
-        org_settings = cache.get(self.get_cache_key(org_id=self.device.organization.pk))
+        org_settings = self.get_org_config_settings(org_id=self.device.organization.pk)
         return getattr(
             org_settings,
             "estimated_location_enabled",

openwisp_controller/geo/admin.py

@@ -16,6 +16,7 @@
 
 from ..admin import MultitenantAdminMixin
 from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdminExportable
+from .estimated_location.utils import check_estimate_location_configured
 from .exportable import GeoDeviceResource
 
 DeviceLocation = load_model("geo", "DeviceLocation")
@@ -100,6 +101,12 @@ class LocationAdmin(MultitenantAdminMixin, AbstractLocationAdmin):
     list_select_related = ("organization",)
     readonly_fields = ("is_estimated",)
 
+    def get_fields(self, request, obj=None):
+        fields = super().get_fields(request, obj)
+        if obj and not check_estimate_location_configured(obj.organization_id):
+            fields.remove("is_estimated")
+        return fields
+
 
 LocationAdmin.list_display.insert(1, "organization")
 LocationAdmin.list_filter.insert(0, MultitenantOrgFilter)

openwisp_controller/geo/base/models.py

@@ -1,4 +1,5 @@
 from django.contrib.gis.db import models
+from django.core.exceptions import ValidationError
 from django.utils.translation import gettext_lazy as _
 from django_loci.base.models import (
     AbstractFloorPlan,
@@ -9,6 +10,8 @@
 
 from openwisp_users.mixins import OrgMixin, ValidateOrgMixin
 
+from ..estimated_location.utils import check_estimate_location_configured
+
 
 class BaseLocation(OrgMixin, AbstractLocation):
     is_estimated = models.BooleanField(
@@ -19,6 +22,19 @@ class BaseLocation(OrgMixin, AbstractLocation):
     class Meta(AbstractLocation.Meta):
         abstract = True
 
+    def clean(self):
+        if self.is_estimated and not check_estimate_location_configured(
+            self.organization_id
+        ):
+            raise ValidationError(
+                {
+                    "is_estimated": _(
+                        "Estimated Location feature required to be configured."
+                    )
+                }
+            )
+        return super().clean()
+
 
 class BaseFloorPlan(OrgMixin, AbstractFloorPlan):
     location = models.ForeignKey(get_model_name("geo", "Location"), models.CASCADE)

openwisp_controller/geo/estimated_location/test_estimated_location.py

@@ -12,6 +12,7 @@
 from openwisp_controller.config.whois.tests_utils import WHOISTransactionMixin
 
 from ...tests.utils import TestAdminMixin
+from ..tests.utils import TestGeoMixin
 from .tests_utils import TestEstimatedLocationMixin
 
 Device = load_model("config", "Device")
@@ -98,6 +99,46 @@ def test_estimated_location_configuration_setting(self):
                 )
 
 
+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()
+        with self.assertRaises(ValidationError) as context_manager:
+            self._create_location(organization=org, is_estimated=True)
+        try:
+            self.assertEqual(
+                context_manager.exception.message_dict["is_estimated"][0],
+                "Estimated Location feature required to be configured.",
+            )
+        except AssertionError:
+            self.fail("ValidationError message not equal to expected message.")
+
+    @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True)
+    def test_estimated_location_admin(self):
+        connect_whois_handlers()
+        admin = self._create_admin()
+        self.client.force_login(admin)
+        org = self._get_org()
+        location = self._create_location(organization=org, is_estimated=True)
+        path = reverse("admin:geo_location_change", args=[location.pk])
+        response = self.client.get(path)
+        self.assertContains(response, "field-is_estimated")
+        self.assertContains(
+            response, "Whether the location's coordinates are estimated."
+        )
+        org.config_settings.estimated_location_enabled = False
+        org.config_settings.save()
+        response = self.client.get(path)
+        self.assertNotContains(response, "field-is_estimated")
+        self.assertNotContains(
+            response, "Whether the location's coordinates are estimated."
+        )
+
+
 class TestEstimatedLocationTransaction(
     TestEstimatedLocationMixin, WHOISTransactionMixin, TransactionTestCase
 ):

openwisp_controller/geo/estimated_location/utils.py

@@ -0,0 +1,15 @@
+from openwisp_controller.config import settings as config_app_settings
+from openwisp_controller.config.whois.service import WHOISService
+
+
+def check_estimate_location_configured(org_id):
+    if not org_id:
+        return False
+    if not config_app_settings.WHOIS_CONFIGURED:
+        return False
+    org_settings = WHOISService.get_org_config_settings(org_id=org_id)
+    return getattr(
+        org_settings,
+        "estimated_location_enabled",
+        config_app_settings.ESTIMATED_LOCATION_ENABLED,
+    )