diff --git a/.circleci/config.yml b/.circleci/config.yml index 093b16bfdc..b73430b316 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -48,12 +48,12 @@ jobs: echo "MAPBOX_TOKEN=$MAPBOX_TOKEN" >> .env # CircleCI does not give project environment variables to jobs built - # from forked pull requests, so those runs have no geocoding key. + # from forked pull requests, so those runs have no Mapbox token. # Without this every group creation 422s on geocode_failed and the # whole Playwright suite fails at its first createGroup(), which # reads as the contributor having broken something. - if [ -z "$GOOGLE_API_CONSOLE_KEY" ]; then - echo "No geocoding key available - using the stub geocoder." + if [ -z "$MAPBOX_TOKEN" ]; then + echo "No Mapbox token available - using the stub geocoder." echo "GEOCODER_STUB=true" >> .env fi diff --git a/app/Helpers/Geocoder.php b/app/Helpers/Geocoder.php index d185ca87fb..682e7cdede 100644 --- a/app/Helpers/Geocoder.php +++ b/app/Helpers/Geocoder.php @@ -2,66 +2,50 @@ namespace App\Helpers; +use Geocoder\Provider\Mapbox\Mapbox; +use Geocoder\Query\GeocodeQuery; +use Illuminate\Support\Facades\Log; + class Geocoder { public function __construct() { } - private function googleKey() - { - // We have this so that we can change the key in testing. - return config('GOOGLE_API_CONSOLE_KEY') ?? env('GOOGLE_API_CONSOLE_KEY'); - } - - /** - * Fetch a URL. Separated out so that tests can exercise the parsing below - * without calling Google, which makes them depend on the network, the key - * and the quota all being healthy. - */ - protected function fetch($url) - { - return file_get_contents($url); - } - public function geocode($location) { if ($location != 'ForceGeocodeFailure') { - $json = $this->fetch('https://maps.googleapis.com/maps/api/geocode/json?address='.urlencode($location).'&key='.$this->googleKey()); - - if ($json) { - $res = json_decode($json); - - if ($res && $res->results && count($res->results)) { - $decoded = json_decode($json)->results[0]; - - $latitude = $decoded->{'geometry'}->{'location'}->lat; - $longitude = $decoded->{'geometry'}->{'location'}->lng; - - foreach ($decoded->{'address_components'} as $component) { - if ($component->types && count($component->types) && $component->types[0] === 'country') { - $country_code = $component->short_name; - } - } + try { + // Mapbox, configured in config/geocoder.php and resolved via the container so tests can bind a + // fake in its place. There is no fallback provider. This is the same geocoder that + // App\Console\Commands\SetPlaceNetworkData uses for reverse geocoding. + $geocodeResponse = app('geocoder')->geocodeQuery( + GeocodeQuery::create($location)->withData('location_type', [Mapbox::TYPE_PLACE, Mapbox::TYPE_ADDRESS]) + ); + $addressCollection = $geocodeResponse->get(); + // get(0) throws OutOfBounds on an empty collection, so an address the + // provider can't place would crash rather than returning false. + $address = $addressCollection->isEmpty() ? null : $addressCollection->first(); + + if ($address && $address->getCoordinates()) { + // The provider gives us both the country name and its ISO code - use the code directly rather + // than trying to match the name against our own country list. + $country = $address->getCountry(); return [ - 'latitude' => $latitude, - 'longitude' => $longitude, - 'country_code' => $country_code, + 'latitude' => $address->getCoordinates()->getLatitude(), + 'longitude' => $address->getCoordinates()->getLongitude(), + 'country_code' => $country ? $country->getCode() : null, ]; } + } catch (\Throwable $e) { + // A bad token, an exhausted quota or Mapbox being unreachable is a failed geocode as far as the + // caller is concerned: it becomes a validation message, not a 500. Log it so a misconfiguration + // is still visible. + Log::error("Geocoding '{$location}' failed: " . $e->getMessage()); } } return false; } - - public function reverseGeocode($lat, $lng) - { - $json = $this->fetch("https://maps.googleapis.com/maps/api/geocode/json?latlng=$lat,$lng&key=".$this->googleKey()); - - $decoded = json_decode($json)->results[0]; - - return $decoded; - } } diff --git a/app/Helpers/StubGeocoder.php b/app/Helpers/StubGeocoder.php index 6e4580baa0..1cf8b8f015 100644 --- a/app/Helpers/StubGeocoder.php +++ b/app/Helpers/StubGeocoder.php @@ -3,17 +3,17 @@ namespace App\Helpers; /** - * A Geocoder that answers from a fixed table instead of calling Google. + * A Geocoder that answers from a fixed table instead of calling Mapbox. * * CircleCI does not pass project environment variables to jobs built from - * forked pull requests, so GOOGLE_API_CONSOLE_KEY is empty on those runs. + * forked pull requests, so MAPBOX_TOKEN is empty on those runs. * Every geocode then returns false, group creation 422s on * groups.geocode_failed, and the whole Playwright suite fails at its first * createGroup() - which looks exactly like the contributor having broken * something. This stub keeps those runs meaningful, and keeps the suite off - * the live Google API generally. + * the live Mapbox API generally. * - * Only used when GEOCODER_STUB is set, which CI does when there is no key. + * Only used when GEOCODER_STUB is set, which CI does when there is no token. */ class StubGeocoder extends Geocoder { diff --git a/config/geocoder.php b/config/geocoder.php index 2e65da6ba4..256dfffc76 100644 --- a/config/geocoder.php +++ b/config/geocoder.php @@ -1,7 +1,5 @@ using('google_maps')`. + | Mapbox is the only geocoder. There is deliberately no fallback provider: + | the address geocoder in App\Helpers\Geocoder and the reverse geocoder in + | App\Console\Commands\SetPlaceNetworkData both need a provider that + | understands street addresses, and a chain that fell through to an + | IP-address geocoder could never place one. If Mapbox can't answer, the + | geocode fails and the caller says so. | | Please consult the official Geocoder documentation for more info. | https://github.com/geocoder-php/Geocoder#providers | */ 'providers' => [ - Chain::class => [ - Mapbox::class => [ - env('MAPBOX_TOKEN'), - ], - GeoPlugin::class => [], + Mapbox::class => [ + env('MAPBOX_TOKEN'), ], ], diff --git a/docs/local-development.md b/docs/local-development.md index 364c0671b7..8163228ba6 100644 --- a/docs/local-development.md +++ b/docs/local-development.md @@ -167,19 +167,18 @@ task docker:test:playwright-autocomplete ### Prerequisites for Testing -Before running tests, you need to configure the Google API key for geocoding functionality: +Geocoding (turning an address into coordinates when a group, event or profile is saved) uses Mapbox. The PHPUnit suite binds a deterministic fake geocoder, so it needs no token; the Playwright suite runs against the real application, so it does: -1. **Set Google API Key**: Add a valid Google Maps API key to your `.env` file: +1. **Set the Mapbox token**: Add a Mapbox access token to your `.env` file: ```bash - GOOGLE_API_CONSOLE_KEY=your_actual_google_api_key_here + MAPBOX_TOKEN=your_actual_mapbox_token_here ``` + Or set `GEOCODER_STUB=true` to answer from a fixed table of places instead, as CI does on forked pull requests, which get no secrets. -2. **API Key Requirements**: The key must have the following APIs enabled: - - Geocoding API - - Maps JavaScript API (for location validation) +2. **Google API key**: the maps and the address autocomplete in the browser still use Google, so `GOOGLE_API_CONSOLE_KEY` is needed for those pages to look right, but not for geocoding. > [!WARNING] -> Without a valid `GOOGLE_API_CONSOLE_KEY`, tests that create groups or events will fail with location validation errors. +> Without a valid `MAPBOX_TOKEN` (or `GEOCODER_STUB=true`), Playwright tests that create groups or events will fail with location validation errors. > [!NOTE] > The PHPUnit task will automatically upload coverage to Coveralls if the `COVERALLS_REPO_TOKEN` environment variable is set. diff --git a/tests/Feature/Users/EditProfileTest.php b/tests/Feature/Users/EditProfileTest.php index 4a1d828bc6..cb509ae3ae 100644 --- a/tests/Feature/Users/EditProfileTest.php +++ b/tests/Feature/Users/EditProfileTest.php @@ -11,7 +11,6 @@ use Carbon\Carbon; use DB; use Illuminate\Foundation\Testing\RefreshDatabase; -use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Storage; use Tests\TestCase; @@ -111,10 +110,7 @@ public function test_location_update(): void $this->assertEquals(51.507, round($user->latitude, 3)); $this->assertEquals(-0.128, round($user->longitude, 3)); - $good = Config::get('GOOGLE_API_CONSOLE_KEY'); - Config::set('GOOGLE_API_CONSOLE_KEY', 'zzz'); - - // Supply the id. + // Supply the id, and a town the test geocoder doesn't know so geocoding fails. $this->post('/profile/edit-info', [ 'id' => $user->id, 'name' => $user->name, @@ -124,8 +120,6 @@ public function test_location_update(): void 'townCity' => 'ZZZZ', ]); - Config::set('GOOGLE_API_CONSOLE_KEY', $good); - $user = $user->fresh(); $this->assertNull($user->latitude); $this->assertNull($user->longitude); diff --git a/tests/Feature/Users/Registration/AccountCreationTest.php b/tests/Feature/Users/Registration/AccountCreationTest.php index 7e56990f1b..a39314500a 100644 --- a/tests/Feature/Users/Registration/AccountCreationTest.php +++ b/tests/Feature/Users/Registration/AccountCreationTest.php @@ -6,7 +6,6 @@ use App\User; use DB; use Hash; -use Illuminate\Support\Facades\Config; use Mockery; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Tests\TestCase; @@ -54,15 +53,10 @@ public function testRegisterInvalidAddress(): void { $userAttributes = $this->userAttributes(); - // Specify an invalid city and force geocoding to fail by invalidating the Google key. - $good = Config::get('GOOGLE_API_CONSOLE_KEY'); - Config::set('GOOGLE_API_CONSOLE_KEY', 'zzz'); - + // A city the test geocoder doesn't know, so geocoding fails. $userAttributes['city'] = 'zzzzzzz'; $response = $this->post('/user/register/', $userAttributes); - Config::set('GOOGLE_API_CONSOLE_KEY', $good); - $response->assertStatus(302); $response->assertRedirect('dashboard'); $this->assertDatabaseHas('users', [ diff --git a/tests/Support/GeocoderMock.php b/tests/Support/GeocoderMock.php index 1e888d3133..53204507e8 100644 --- a/tests/Support/GeocoderMock.php +++ b/tests/Support/GeocoderMock.php @@ -7,10 +7,10 @@ /** * Deterministic, location-aware Geocoder for the test suite. * - * The real Geocoder makes a live HTTP call to the Google Maps Geocoding API, which is + * The real Geocoder makes a live HTTP call to the Mapbox Geocoding API, which is * rate-limited and non-deterministic. Tests that geocode a location were at the mercy - * of Google's responses — e.g. APIv2GroupTest::testLocales flipped between Belgium and - * the United Kingdom, and the "bad location" tests only passed while Google happened to + * of the provider's responses — e.g. APIv2GroupTest::testLocales flipped between Belgium and + * the United Kingdom, and the "bad location" tests only passed while the provider happened to * reject the gibberish string. * * This fake is bound globally in Tests\TestCase::setUp() so no test hits the live API. diff --git a/tests/TestCase.php b/tests/TestCase.php index 9fc66af8ee..66ea3d7f38 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -101,9 +101,9 @@ protected function setUp(): void $this->withoutExceptionHandling(); app('honeypot')->disable(); - // Bind a deterministic geocoder so the suite never makes live calls to the - // Google Maps Geocoding API (rate-limited / intermittently failing, which - // made any geocoding test flaky). Tests that need the geocode-failure path + // Bind a deterministic geocoder so the suite never makes live calls to + // Mapbox (rate-limited / intermittently failing, which made any geocoding + // test flaky). Tests that need the geocode-failure path // bind their own failing mock in setUp(), overriding this. $this->app->bind(Geocoder::class, function () { return new GeocoderMock(); diff --git a/tests/Unit/GeocoderTest.php b/tests/Unit/GeocoderTest.php index 83f1a4972b..9519904571 100644 --- a/tests/Unit/GeocoderTest.php +++ b/tests/Unit/GeocoderTest.php @@ -1,76 +1,148 @@ 'OK', - 'results' => [ - [ - 'geometry' => [ - 'location' => ['lat' => 51.4643585, 'lng' => -0.1135401], - ], - 'address_components' => [ - ['short_name' => 'SW9 7QD', 'types' => ['postal_code']], - ['short_name' => 'GB', 'types' => ['country', 'political']], - ], - ], - ], - ]); + $collection = new AddressCollection($addresses); + + // instance(), not bind(): geocoder-laravel registers a singleton, and by + // the time a test runs it may already be resolved - in which case bind() + // is ignored and the real Mapbox provider answers. That failure looks like a + // wrong country code rather than a network call, so it is worth being + // explicit about. + $fake = new class($collection) + { + private $collection; + + public function __construct($collection) + { + $this->collection = $collection; + } + + public function geocodeQuery($query) + { + return new class($this->collection) + { + private $collection; + + public function __construct($collection) + { + $this->collection = $collection; + } + + public function get() + { + return $this->collection; + } + }; + } + }; + + // Both keys: geocoder-laravel binds the string 'geocoder' through to + // ProviderAndDumperAggregator, which is itself a singleton, so replacing + // only the string leaves the real provider reachable - and a test that + // "passes" then is really talking to Mapbox. + $this->app->instance('geocoder', $fake); + $this->app->instance(\Geocoder\Laravel\ProviderAndDumperAggregator::class, $fake); } - private function geocoderReturning($body): \App\Helpers\Geocoder + private function bindGeocoderThrowing(\Throwable $e): void { - return new class($body) extends \App\Helpers\Geocoder { - private $body; - - public function __construct($body) + $fake = new class($e) { - parent::__construct(); - $this->body = $body; - } + private $e; - protected function fetch($url) - { - return $this->body; - } - }; + public function __construct($e) + { + $this->e = $e; + } + + public function geocodeQuery($query) + { + throw $this->e; + } + }; + + $this->app->instance('geocoder', $fake); + $this->app->instance(\Geocoder\Laravel\ProviderAndDumperAggregator::class, $fake); + } + + private function address(array $over = []): Address + { + return Address::createFromArray(array_merge([ + 'providedBy' => 'test', + 'latitude' => 51.4643585, + 'longitude' => -0.1135401, + 'country' => 'United Kingdom', + 'countryCode' => 'GB', + ], $over)); } public function testGeocode(): void { - $ret = $this->geocoderReturning($this->cannedResponse()) - ->geocode('6 Canterbury Crescent, London SW9 7QD'); + $this->bindGeocoderReturning([$this->address()]); + + $ret = (new \App\Helpers\Geocoder)->geocode('6 Canterbury Crescent, London SW9 7QD'); $this->assertEquals(round(51.4643585, 2), round($ret['latitude'], 2)); $this->assertEquals(round(-0.1135401, 2), round($ret['longitude'], 2)); $this->assertEquals('GB', $ret['country_code']); } + public function testGeocodeUsesTheProvidersCountryCodeRatherThanItsName(): void + { + // The whole point of reading getCountry()->getCode(): no matching a + // display name against our own country list. + $this->bindGeocoderReturning([$this->address([ + 'country' => 'Belgique', + 'countryCode' => 'BE', + ])]); + + $this->assertEquals('BE', (new \App\Helpers\Geocoder)->geocode('Brussels')['country_code']); + } + public function testGeocodeReturnsFalseWhenNothingFound(): void { - $empty = json_encode(['status' => 'ZERO_RESULTS', 'results' => []]); + $this->bindGeocoderReturning([]); - $this->assertFalse($this->geocoderReturning($empty)->geocode('nowhere at all')); + $this->assertFalse((new \App\Helpers\Geocoder)->geocode('nowhere at all')); } - public function testGeocodeReturnsFalseWhenTheRequestFails(): void + public function testGeocodeCopesWithAnAddressCarryingNoCountry(): void { - // file_get_contents returns false on a failed request. This is the case - // that made the old test error with "array offset on value of type bool" - // whenever Google was unreachable or the key was unhappy. - $this->assertFalse($this->geocoderReturning(false)->geocode('6 Canterbury Crescent')); + $this->bindGeocoderReturning([$this->address(['country' => null, 'countryCode' => null])]); + + $ret = (new \App\Helpers\Geocoder)->geocode('somewhere'); + + $this->assertNull($ret['country_code']); + $this->assertEquals(round(51.4643585, 2), round($ret['latitude'], 2)); + } + + public function testGeocodeReturnsFalseWhenTheProviderFails(): void + { + // With no fallback provider there is no chain to swallow a bad token or + // an unreachable Mapbox: the helper has to, so the caller gets a failed + // geocode (a validation message) rather than a 500. + $this->bindGeocoderThrowing(new \Geocoder\Exception\InvalidCredentials('Invalid token')); + + $this->assertFalse((new \App\Helpers\Geocoder)->geocode('6 Canterbury Crescent')); } public function testForcedFailureShortCircuits(): void { - $this->assertFalse($this->geocoderReturning($this->cannedResponse())->geocode('ForceGeocodeFailure')); + $this->bindGeocoderReturning([$this->address()]); + + $this->assertFalse((new \App\Helpers\Geocoder)->geocode('ForceGeocodeFailure')); } }