ソースを参照

Address PR review: move _build_coords_url to module level, use str.replace, suppress address link for GPS MAPS_URL

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Brian Tiemann 10 時間 前
コミット
a463972a71

+ 4 - 1
docs/configuration/miscellaneous.md

@@ -197,12 +197,15 @@ This specifies the URL to use when presenting a map of a physical location by st
 **For GPS coordinates**, two formats are supported:
 **For GPS coordinates**, two formats are supported:
 
 
 * **Simple prefix** (default behavior): The latitude and longitude are appended as a comma-separated pair. For example, `https://maps.google.com/?q=` produces `https://maps.google.com/?q=48.858,2.294`.
 * **Simple prefix** (default behavior): The latitude and longitude are appended as a comma-separated pair. For example, `https://maps.google.com/?q=` produces `https://maps.google.com/?q=48.858,2.294`.
-* **Format string**: Include `{lat}` and/or `{lon}` placeholders anywhere in the URL for full control over placement. For example:
+* **Coordinate placeholders**: Include `{lat}` and/or `{lon}` anywhere in the URL. Only these two literal placeholders are supported. For example:
 
 
 ```
 ```
 MAPS_URL = "https://www.openstreetmap.org/?mlat={lat}&mlon={lon}#map=16/{lat}/{lon}"
 MAPS_URL = "https://www.openstreetmap.org/?mlat={lat}&mlon={lon}#map=16/{lat}/{lon}"
 ```
 ```
 
 
+!!! note
+    When `MAPS_URL` contains `{lat}` or `{lon}` placeholders, the "map it" button will only appear on pages with GPS coordinates — address-based map links will be suppressed, since the coordinate-format URL cannot be used with a plain address string.
+
 ---
 ---
 
 
 ## MAX_PAGE_SIZE
 ## MAX_PAGE_SIZE

+ 30 - 8
netbox/netbox/tests/test_ui.py

@@ -441,11 +441,11 @@ class GPSCoordinatesAttrTestCase(TestCase):
         self.assertEqual(attr.render(obj, {'name': 'coordinates'}), attr.placeholder)
         self.assertEqual(attr.render(obj, {'name': 'coordinates'}), attr.placeholder)
 
 
     def test_build_coords_url_legacy_prefix(self):
     def test_build_coords_url_legacy_prefix(self):
-        url = attrs.GPSCoordinatesAttr._build_coords_url('https://maps.google.com/?q=', 48.858, 2.294)
+        url = attrs._build_coords_url('https://maps.google.com/?q=', 48.858, 2.294)
         self.assertEqual(url, 'https://maps.google.com/?q=48.858,2.294')
         self.assertEqual(url, 'https://maps.google.com/?q=48.858,2.294')
 
 
     def test_build_coords_url_lat_lon_placeholders(self):
     def test_build_coords_url_lat_lon_placeholders(self):
-        url = attrs.GPSCoordinatesAttr._build_coords_url(
+        url = attrs._build_coords_url(
             'https://www.openstreetmap.org/?mlat={lat}&mlon={lon}#map=16/{lat}/{lon}',
             'https://www.openstreetmap.org/?mlat={lat}&mlon={lon}#map=16/{lat}/{lon}',
             48.858,
             48.858,
             2.294,
             2.294,
@@ -453,21 +453,21 @@ class GPSCoordinatesAttrTestCase(TestCase):
         self.assertEqual(url, 'https://www.openstreetmap.org/?mlat=48.858&mlon=2.294#map=16/48.858/2.294')
         self.assertEqual(url, 'https://www.openstreetmap.org/?mlat=48.858&mlon=2.294#map=16/48.858/2.294')
 
 
     def test_build_coords_url_lat_placeholder_only(self):
     def test_build_coords_url_lat_placeholder_only(self):
-        url = attrs.GPSCoordinatesAttr._build_coords_url('https://example.com/?lat={lat}', 48.858, 2.294)
+        url = attrs._build_coords_url('https://example.com/?lat={lat}', 48.858, 2.294)
         self.assertEqual(url, 'https://example.com/?lat=48.858')
         self.assertEqual(url, 'https://example.com/?lat=48.858')
 
 
     def test_build_coords_url_lon_placeholder_only(self):
     def test_build_coords_url_lon_placeholder_only(self):
-        url = attrs.GPSCoordinatesAttr._build_coords_url('https://example.com/?lon={lon}', 48.858, 2.294)
+        url = attrs._build_coords_url('https://example.com/?lon={lon}', 48.858, 2.294)
         self.assertEqual(url, 'https://example.com/?lon=2.294')
         self.assertEqual(url, 'https://example.com/?lon=2.294')
 
 
     def test_build_coords_url_unknown_placeholder_falls_back_to_legacy(self):
     def test_build_coords_url_unknown_placeholder_falls_back_to_legacy(self):
         # URL with only an unknown placeholder (no {lat}/{lon}) → legacy append
         # URL with only an unknown placeholder (no {lat}/{lon}) → legacy append
-        url = attrs.GPSCoordinatesAttr._build_coords_url('https://example.com/?q={unknown}', 48.858, 2.294)
+        url = attrs._build_coords_url('https://example.com/?q={unknown}', 48.858, 2.294)
         self.assertEqual(url, 'https://example.com/?q={unknown}48.858,2.294')
         self.assertEqual(url, 'https://example.com/?q={unknown}48.858,2.294')
 
 
     def test_build_coords_url_known_and_unknown_placeholder(self):
     def test_build_coords_url_known_and_unknown_placeholder(self):
         # {lat} is substituted; unknown key is left as a literal placeholder
         # {lat} is substituted; unknown key is left as a literal placeholder
-        url = attrs.GPSCoordinatesAttr._build_coords_url(
+        url = attrs._build_coords_url(
             'https://example.com/?lat={lat}&layer={layer}', 48.858, 2.294
             'https://example.com/?lat={lat}&layer={layer}', 48.858, 2.294
         )
         )
         self.assertEqual(url, 'https://example.com/?lat=48.858&layer={layer}')
         self.assertEqual(url, 'https://example.com/?lat=48.858&layer={layer}')
@@ -475,7 +475,7 @@ class GPSCoordinatesAttrTestCase(TestCase):
     def test_build_coords_url_decimal_values_no_locale_separator(self):
     def test_build_coords_url_decimal_values_no_locale_separator(self):
         # Decimal field values must format with '.' as the decimal separator regardless of locale;
         # Decimal field values must format with '.' as the decimal separator regardless of locale;
         # a locale-style comma separator would produce e.g. '48,858258' and break the URL
         # a locale-style comma separator would produce e.g. '48,858258' and break the URL
-        url = attrs.GPSCoordinatesAttr._build_coords_url(
+        url = attrs._build_coords_url(
             'https://maps.google.com/?q=',
             'https://maps.google.com/?q=',
             Decimal('48.858258'),
             Decimal('48.858258'),
             Decimal('2.294498'),
             Decimal('2.294498'),
@@ -483,7 +483,7 @@ class GPSCoordinatesAttrTestCase(TestCase):
         self.assertEqual(url, 'https://maps.google.com/?q=48.858258,2.294498')
         self.assertEqual(url, 'https://maps.google.com/?q=48.858258,2.294498')
 
 
     def test_build_coords_url_decimal_with_placeholders_no_locale_separator(self):
     def test_build_coords_url_decimal_with_placeholders_no_locale_separator(self):
-        url = attrs.GPSCoordinatesAttr._build_coords_url(
+        url = attrs._build_coords_url(
             'https://www.openstreetmap.org/?mlat={lat}&mlon={lon}',
             'https://www.openstreetmap.org/?mlat={lat}&mlon={lon}',
             Decimal('48.858258'),
             Decimal('48.858258'),
             Decimal('2.294498'),
             Decimal('2.294498'),
@@ -491,6 +491,28 @@ class GPSCoordinatesAttrTestCase(TestCase):
         self.assertEqual(url, 'https://www.openstreetmap.org/?mlat=48.858258&mlon=2.294498')
         self.assertEqual(url, 'https://www.openstreetmap.org/?mlat=48.858258&mlon=2.294498')
 
 
 
 
+class AddressAttrTestCase(TestCase):
+
+    def test_plain_prefix_map_url_is_passed_through(self):
+        attr = attrs.AddressAttr('address', map_url='https://maps.google.com/?q=')
+        obj = SimpleNamespace(address='1 Main St')
+        context = attr.get_context(obj, 'address', '1 Main St', {})
+        self.assertEqual(context['map_url'], 'https://maps.google.com/?q=')
+
+    def test_gps_format_map_url_is_suppressed_for_addresses(self):
+        # A GPS-format URL cannot render address links; map_url should be None
+        attr = attrs.AddressAttr('address', map_url='https://maps.example.com/?mlat={lat}&mlon={lon}')
+        obj = SimpleNamespace(address='1 Main St')
+        context = attr.get_context(obj, 'address', '1 Main St', {})
+        self.assertIsNone(context['map_url'])
+
+    def test_no_map_url(self):
+        attr = attrs.AddressAttr('address', map_url=False)
+        obj = SimpleNamespace(address='1 Main St')
+        context = attr.get_context(obj, 'address', '1 Main St', {})
+        self.assertIsNone(context['map_url'])
+
+
 class DateTimeAttrTestCase(TestCase):
 class DateTimeAttrTestCase(TestCase):
 
 
     def test_default_spec(self):
     def test_default_spec(self):

+ 15 - 13
netbox/netbox/ui/attrs.py

@@ -457,6 +457,15 @@ class GenericForeignKeyAttr(ObjectAttribute):
         }
         }
 
 
 
 
+def _build_coords_url(map_url, latitude, longitude):
+    """Build a GPS map URL, substituting {lat}/{lon} placeholders or appending as a comma-separated pair."""
+    lat_str = str(latitude)
+    lon_str = str(longitude)
+    if '{lat}' in map_url or '{lon}' in map_url:
+        return map_url.replace('{lat}', lat_str).replace('{lon}', lon_str)
+    return f'{map_url}{lat_str},{lon_str}'
+
+
 class AddressAttr(MapURLMixin, ObjectAttribute):
 class AddressAttr(MapURLMixin, ObjectAttribute):
     """
     """
     A physical or mailing address.
     A physical or mailing address.
@@ -472,8 +481,12 @@ class AddressAttr(MapURLMixin, ObjectAttribute):
         self._map_url = map_url
         self._map_url = map_url
 
 
     def get_context(self, obj, attr, value, context):
     def get_context(self, obj, attr, value, context):
+        map_url = self.map_url
+        # A GPS-format MAPS_URL (containing {lat}/{lon}) cannot be used for address rendering
+        if map_url and ('{lat}' in map_url or '{lon}' in map_url):
+            map_url = None
         return {
         return {
-            'map_url': self.map_url,
+            'map_url': map_url,
         }
         }
 
 
 
 
@@ -502,7 +515,7 @@ class GPSCoordinatesAttr(MapURLMixin, ObjectAttribute):
             return self.placeholder
             return self.placeholder
         map_url = self.map_url
         map_url = self.map_url
         if map_url:
         if map_url:
-            map_url = self._build_coords_url(map_url, latitude, longitude)
+            map_url = _build_coords_url(map_url, latitude, longitude)
         return render_to_string(self.template_name, {
         return render_to_string(self.template_name, {
             'name': context['name'],
             'name': context['name'],
             'latitude': latitude,
             'latitude': latitude,
@@ -510,17 +523,6 @@ class GPSCoordinatesAttr(MapURLMixin, ObjectAttribute):
             'map_url': map_url,
             'map_url': map_url,
         })
         })
 
 
-    @staticmethod
-    def _build_coords_url(map_url, latitude, longitude):
-        if '{lat}' in map_url or '{lon}' in map_url:
-            # Substitute only {lat}/{lon}; leave any other placeholders intact
-            # so a misconfigured URL still resolves the known tokens correctly.
-            class _SubstituteKnown(dict):
-                def __missing__(self, key):
-                    return '{' + key + '}'
-            return map_url.format_map(_SubstituteKnown(lat=latitude, lon=longitude))
-        return f'{map_url}{latitude},{longitude}'
-
 
 
 class DateTimeAttr(ObjectAttribute):
 class DateTimeAttr(ObjectAttribute):
     """
     """