feat: add non postcode geocoding and map click behaviour #483
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for geocoding non-UK addresses using Mapbox's geocoding API and implements map click behavior for reading groups. The changes extend the existing postcode-based geocoding (UK only) to support international addresses.
Changes:
- Added Mapbox-based address geocoding for international locations
- Implemented map click handler to scroll to reading group details when map markers are clicked
- Added country field to ReadingGroup model to support international events
- Removed debug console.log statement from main.ts
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/main.ts | Removed debug console.log statement |
| frontend/controllers/map-click-reading-group-controller.ts | New controller for handling map clicks and marker interactions |
| app/utils/geo.py | Added address_geo function for Mapbox-based geocoding of international addresses |
| app/models/wagtail.py | Added in_person_country field and updated save method to use address geocoding for non-GB locations |
| app/templates/app/includes/reading_groups_list_and_map.html | Added map-click-reading-group controller to enable click behavior |
| app/migrations/0107_readinggroup_country_and_more.py | Migration to add country field with GB default |
| app/migrations/0108_rename_country_readinggroup_in_person_country.py | Migration to rename country field for clarity |
| cache/tmp0t82g09q | Binary cache file that should not be committed |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from urllib.parse import quote | ||
|
|
||
|
|
||
| def address_geo(address: str, postcode: Union[str, None] = "", country: Union[str, None] = "GB"): |
There was a problem hiding this comment.
The new address_geo function lacks documentation. Please add a docstring that explains the purpose, parameters, return values, and behavior when the API key is missing or when geocoding fails. This will help other developers understand how to use this function correctly.
| def address_geo(address: str, postcode: Union[str, None] = "", country: Union[str, None] = "GB"): | |
| def address_geo(address: str, postcode: Union[str, None] = "", country: Union[str, None] = "GB"): | |
| """ | |
| Geocode a postal address using the Mapbox Geocoding API and return a GIS Point. | |
| This function builds a search query from the provided address and optional | |
| postcode, sends it to the Mapbox Geocoding API, and returns the first | |
| matching location as a ``django.contrib.gis.geos.Point`` instance | |
| (longitude, latitude). If geocoding fails or no results are found, it | |
| returns ``None``. | |
| Parameters | |
| ---------- | |
| address : str | |
| The street address or place name to geocode. This is the primary | |
| component of the search query. | |
| postcode : str or None, optional | |
| An optional postcode to append to the address to improve geocoding | |
| accuracy. If an empty string or ``None`` is provided, only ``address`` | |
| is used in the query. Defaults to an empty string. | |
| country : str or None, optional | |
| An ISO 3166-1 alpha-2 country code (e.g. ``"GB"``) used to limit | |
| results to a specific country. If ``None`` is provided, the API may | |
| return results from any country, depending on Mapbox behavior. | |
| Defaults to ``"GB"``. | |
| Returns | |
| ------- | |
| django.contrib.gis.geos.Point or None | |
| A Point instance representing the geocoded location (longitude, latitude) | |
| if a result is found, otherwise ``None``. ``None`` is also returned if | |
| the HTTP request fails or an error occurs while calling the Mapbox API. | |
| Raises | |
| ------ | |
| ValueError | |
| If the ``MAPBOX_PRIVATE_API_KEY`` environment variable is not set. | |
| Notes | |
| ----- | |
| - The Mapbox API key is read from the ``MAPBOX_PRIVATE_API_KEY`` environment | |
| variable; if it is missing, this function will raise ``ValueError`` before | |
| making any network request. | |
| - Network errors, HTTP errors, or unexpected Mapbox responses are caught and | |
| cause the function to return ``None`` rather than raising. | |
| """ |
| except requests.exceptions.RequestException as e: | ||
| return None |
There was a problem hiding this comment.
The exception variable 'e' is caught but never used or logged. This makes debugging API failures difficult. Consider logging the error with appropriate context before returning None, so that failures can be investigated in production.
| if country_code == "GB" and self.in_person_postcode and not self.coordinates: | ||
| postcode_result = postcode_geo(self.in_person_postcode) | ||
| point = point_from_postcode_result(postcode_result) | ||
| if point: | ||
| self.coordinates = point | ||
| if self.in_person_location and not self.coordinates: | ||
| point = address_geo(address=self.in_person_location, postcode=self.in_person_postcode, country=country_code) | ||
| if point: | ||
| self.coordinates = point |
There was a problem hiding this comment.
The geocoding logic has a potential issue. When coordinates already exist, the address geocoding at lines 158-161 will not execute. However, if a user updates the location without updating coordinates, or if the GB postcode geocoding succeeds at lines 153-157, the address geocoding will be skipped even when it should run for non-GB countries. Consider restructuring this logic to prioritize the appropriate geocoding method based on the country code, or only skip geocoding if coordinates were successfully populated in this save operation.
| if country_code == "GB" and self.in_person_postcode and not self.coordinates: | |
| postcode_result = postcode_geo(self.in_person_postcode) | |
| point = point_from_postcode_result(postcode_result) | |
| if point: | |
| self.coordinates = point | |
| if self.in_person_location and not self.coordinates: | |
| point = address_geo(address=self.in_person_location, postcode=self.in_person_postcode, country=country_code) | |
| if point: | |
| self.coordinates = point | |
| # Track whether coordinates were set during this save call | |
| coordinates_set_this_save = False | |
| # For GB, first try postcode-based geocoding | |
| if country_code == "GB" and self.in_person_postcode: | |
| postcode_result = postcode_geo(self.in_person_postcode) | |
| point = point_from_postcode_result(postcode_result) | |
| if point: | |
| self.coordinates = point | |
| coordinates_set_this_save = True | |
| # Fallback to address-based geocoding if we still don't have coordinates from this save | |
| if self.in_person_location and not coordinates_set_this_save: | |
| point = address_geo( | |
| address=self.in_person_location, | |
| postcode=self.in_person_postcode, | |
| country=country_code, | |
| ) | |
| if point: | |
| self.coordinates = point | |
| coordinates_set_this_save = True |
| def address_geo(address: str, postcode: Union[str, None] = "", country: Union[str, None] = "GB"): | ||
| api_token = os.getenv('MAPBOX_PRIVATE_API_KEY') | ||
| if not api_token: | ||
| raise ValueError("MAPBOX_PRIVATE_API_KEY environment variable not set") | ||
|
|
||
| search_query = address | ||
| if postcode: | ||
| search_query = f"{address}, {postcode}" | ||
|
|
||
| base_url = "https://api.mapbox.com/geocoding/v5/mapbox.places" | ||
| url = f"{base_url}/{quote(search_query)}.json" | ||
|
|
||
| # Parameters for the API request | ||
| params = { | ||
| 'access_token': api_token, | ||
| 'country': country, | ||
| 'limit': 1 | ||
| } | ||
|
|
||
| try: | ||
| # Make the API request | ||
| response = requests.get(url, params=params) | ||
| response.raise_for_status() | ||
|
|
||
| # Parse the JSON response | ||
| data = response.json() | ||
|
|
||
| # Check if we got any results | ||
| if data.get('features') and len(data['features']) > 0: | ||
| coordinates = data['features'][0]['geometry']['coordinates'] | ||
| return Point(coordinates[0], coordinates[1]) # (longitude, latitude) | ||
| else: | ||
| return None | ||
|
|
||
| except requests.exceptions.RequestException as e: | ||
| return None |
There was a problem hiding this comment.
The new address_geo function lacks test coverage. Since this repository has comprehensive test coverage for other utility functions (e.g., postcode_geo functions in the same file), consider adding tests to verify the behavior of address_geo including successful geocoding, handling of missing API keys, handling of API failures, and handling of empty results.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
Motivation and Context
How Can It Be Tested?
How Will This Be Deployed?
Screenshots (if appropriate):
Types of changes
Checklist: