diff --git a/app/Http/Controllers/API/GroupController.php b/app/Http/Controllers/API/GroupController.php index 41a3f23ec7..33743321eb 100644 --- a/app/Http/Controllers/API/GroupController.php +++ b/app/Http/Controllers/API/GroupController.php @@ -1218,6 +1218,8 @@ private function validateGroupParams(Request $request, $create, ?Group $existing 'description' => ['required'], 'website' => ['nullable', 'url', 'max:255'], 'postcode' => ['nullable', 'max:32'], + 'lat' => ['nullable', 'numeric', 'between:-90,90', 'required_with:lng'], + 'lng' => ['nullable', 'numeric', 'between:-180,180', 'required_with:lat'], ]); } else { $request->validate([ @@ -1226,6 +1228,8 @@ private function validateGroupParams(Request $request, $create, ?Group $existing 'website' => ['nullable', 'url', 'max:255'], 'archived_at' => ['nullable', 'date'], 'postcode' => ['nullable', 'max:32'], + 'lat' => ['nullable', 'numeric', 'between:-90,90', 'required_with:lng'], + 'lng' => ['nullable', 'numeric', 'between:-180,180', 'required_with:lat'], ]); } @@ -1249,6 +1253,12 @@ private function validateGroupParams(Request $request, $create, ?Group $existing throw ValidationException::withMessages(['location ' => __('partials.validate_timezone')]); } + // The add/edit page shows a draggable map, so the client can send explicit + // coordinates that take precedence over geocoding the location text. + $clientLat = $request->input('lat'); + $clientLng = $request->input('lng'); + $hasClientCoords = $clientLat !== null && $clientLng !== null; + if (!empty($location)) { // Skip geocoding when the location string hasn't changed — avoids unnecessary API calls // and prevents failures when editing other fields (e.g. group icon) in environments @@ -1264,18 +1274,27 @@ private function validateGroupParams(Request $request, $create, ?Group $existing $geocoded = $geocoder->geocode($location); if (empty($geocoded)) { - throw ValidationException::withMessages(['location ' => __('groups.geocode_failed')]); - } - - $latitude = $geocoded['latitude']; - $longitude = $geocoded['longitude']; + // With an explicit pin we can still place the group; without + // one, an ungeocodable location is fatal as before. + if (!$hasClientCoords) { + throw ValidationException::withMessages(['location ' => __('groups.geocode_failed')]); + } + } else { + $latitude = $geocoded['latitude']; + $longitude = $geocoded['longitude']; - // Note that the country returned by the geocoder is already in English, which is what we need for the - // value in the database. - $country_code = $geocoded['country_code'] ?? null; + // Note that the country returned by the geocoder is already in English, which is what we need for the + // value in the database. + $country_code = $geocoded['country_code'] ?? null; + } } } + if ($hasClientCoords) { + $latitude = (float) $clientLat; + $longitude = (float) $clientLng; + } + return [ $name, $area, diff --git a/lang/en/partials.php b/lang/en/partials.php index 95cec7a55c..ba27f121b3 100644 --- a/lang/en/partials.php +++ b/lang/en/partials.php @@ -84,6 +84,7 @@ 'notification_greeting' => 'Hello!', 'confirm' => 'Confirm', 'validate_timezone' => 'Please select a valid timezone.', + 'dragmap' => 'Pin not quite in the right place? Drag it to the right spot.', 'share_this' => 'Share this', 'download' => 'Download', 'share_modal_title' => 'Shareable Statistics', diff --git a/lang/fr-BE/partials.php b/lang/fr-BE/partials.php index ab28284b87..6a02cce647 100644 --- a/lang/fr-BE/partials.php +++ b/lang/fr-BE/partials.php @@ -87,6 +87,7 @@ 'notification_greeting' => 'Bonjour !', 'notification_footer' => 'Si vous souhaitez ne plus recevoir ces courriels, veuillez consulter vos préférences sur votre compte.', 'validate_timezone' => 'Veuillez sélectionner un fuseau horaire valide.', + 'dragmap' => 'Le repère n\'est pas tout à fait au bon endroit ? Faites-le glisser pour le corriger.', 'share_this' => 'Partager ceci', 'download' => 'Télécharger', 'share_modal_title' => 'Statistiques à partager', diff --git a/lang/fr/partials.php b/lang/fr/partials.php index ab28284b87..6a02cce647 100644 --- a/lang/fr/partials.php +++ b/lang/fr/partials.php @@ -87,6 +87,7 @@ 'notification_greeting' => 'Bonjour !', 'notification_footer' => 'Si vous souhaitez ne plus recevoir ces courriels, veuillez consulter vos préférences sur votre compte.', 'validate_timezone' => 'Veuillez sélectionner un fuseau horaire valide.', + 'dragmap' => 'Le repère n\'est pas tout à fait au bon endroit ? Faites-le glisser pour le corriger.', 'share_this' => 'Partager ceci', 'download' => 'Télécharger', 'share_modal_title' => 'Statistiques à partager', diff --git a/resources/js/components/GroupAddEdit.vue b/resources/js/components/GroupAddEdit.vue index fa87ffbda8..f249278042 100644 --- a/resources/js/components/GroupAddEdit.vue +++ b/resources/js/components/GroupAddEdit.vue @@ -58,11 +58,10 @@ ref="location" /> n.id)), network_data: JSON.stringify(this.networkData), archived_at: this.archived_at, + lat: this.lat !== null ? String(this.lat) : null, + lng: this.lng !== null ? String(this.lng) : null, } console.log('Edit', JSON.stringify(payload)) diff --git a/resources/js/components/GroupLocationMap.test.js b/resources/js/components/GroupLocationMap.test.js new file mode 100644 index 0000000000..126284b2b8 --- /dev/null +++ b/resources/js/components/GroupLocationMap.test.js @@ -0,0 +1,91 @@ +import Vue from 'vue' +import { BootstrapVue } from 'bootstrap-vue' +Vue.use(BootstrapVue) + +import { mount } from '@vue/test-utils' +import LangMixin from 'resources/js/mixins/lang.js' +import GroupLocationMap from './GroupLocationMap.vue' + +const LMapStub = { + name: 'l-map', + props: ['center', 'zoom'], + template: '
' +} + +const LMarkerStub = { + name: 'l-marker', + props: ['latLng', 'draggable'], + template: '
' +} + +function mountMap (props = {}) { + return mount(GroupLocationMap, { + mixins: [LangMixin], + propsData: { lat: 51.5, lng: -0.12, ...props }, + stubs: { + 'l-map': LMapStub, + 'l-tile-layer': true, + 'l-marker': LMarkerStub + } + }) +} + +// What Leaflet passes to a marker's dragend handler. +function dragMarkerTo (wrapper, lat, lng) { + wrapper.findComponent(LMarkerStub).vm.$emit('dragend', { target: { getLatLng: () => ({ lat, lng }) } }) +} + +test('shows the drag hint', () => { + const wrapper = mountMap() + expect(wrapper.text()).toContain('partials.dragmap') +}) + +test('the marker is draggable and starts zoomed in on the location', () => { + const wrapper = mountMap() + expect(wrapper.findComponent(LMarkerStub).props('draggable')).toBe(true) + expect(wrapper.findComponent(LMapStub).props('zoom')).toBeGreaterThanOrEqual(15) + expect(wrapper.findComponent(LMapStub).props('center')).toEqual([51.5, -0.12]) +}) + +test('dragging the marker emits updated lat/lng', async () => { + const wrapper = mountMap() + + dragMarkerTo(wrapper, 52.1, 1.3) + await wrapper.vm.$nextTick() + + expect(wrapper.emitted('update:lat').pop()).toEqual([52.1]) + expect(wrapper.emitted('update:lng').pop()).toEqual([1.3]) + expect(wrapper.findComponent(LMarkerStub).props('latLng')).toEqual([52.1, 1.3]) +}) + +test('panning or zooming the map does not move the marker', async () => { + const wrapper = mountMap() + + wrapper.findComponent(LMapStub).vm.$emit('update:center', { lat: 48.85, lng: 2.35 }) + wrapper.findComponent(LMapStub).vm.$emit('update:zoom', 12) + await wrapper.vm.$nextTick() + + expect(wrapper.emitted('update:lat')).toBeUndefined() + expect(wrapper.findComponent(LMarkerStub).props('latLng')).toEqual([51.5, -0.12]) +}) + +test('an external lat/lng change (new geocode) moves the marker and the map', async () => { + const wrapper = mountMap() + + await wrapper.setProps({ lat: 40.7, lng: -74.0 }) + + expect(wrapper.findComponent(LMarkerStub).props('latLng')).toEqual([40.7, -74.0]) + expect(wrapper.findComponent(LMapStub).props('center')).toEqual([40.7, -74.0]) +}) + +test('prop echo of a drag does not recentre the map', async () => { + const wrapper = mountMap() + + dragMarkerTo(wrapper, 52.1, 1.3) + await wrapper.vm.$nextTick() + + // Parent syncs the emitted values straight back down. + await wrapper.setProps({ lat: 52.1, lng: 1.3 }) + + expect(wrapper.findComponent(LMapStub).props('center')).toEqual([51.5, -0.12]) +}) diff --git a/resources/js/components/GroupLocationMap.vue b/resources/js/components/GroupLocationMap.vue index 84d7ddb9d8..f3aaf42ff9 100644 --- a/resources/js/components/GroupLocationMap.vue +++ b/resources/js/components/GroupLocationMap.vue @@ -1,18 +1,25 @@ \ No newline at end of file + diff --git a/tests/Feature/Groups/GroupLatLngOverrideTest.php b/tests/Feature/Groups/GroupLatLngOverrideTest.php new file mode 100644 index 0000000000..fcbd202ecf --- /dev/null +++ b/tests/Feature/Groups/GroupLatLngOverrideTest.php @@ -0,0 +1,171 @@ +administrator()->create(['api_token' => '1234']); + $network = Network::factory()->create(['shortname' => 'network']); + $user->repair_network = $network->id; + $user->save(); + + return $user; + } + + /** @test */ + public function create_uses_client_lat_lng_when_supplied(): void + { + $this->apiUser(); + + // Deliberately different from the geocoder's London (51.5073, -0.1277) + // so we can tell whose coordinates won. + $response = $this->post('/api/v2/groups?api_token=1234', [ + 'name' => 'Dragged Group', + 'location' => 'London', + 'description' => 'Some text.', + 'timezone' => 'Europe/London', + 'lat' => 51.5197, + 'lng' => -0.1409, + ]); + + $response->assertSuccessful(); + $group = Group::findOrFail(json_decode($response->getContent(), true)['id']); + + $this->assertEqualsWithDelta(51.5197, $group->latitude, 0.00001); + $this->assertEqualsWithDelta(-0.1409, $group->longitude, 0.00001); + // Country still comes from geocoding the location text. + $this->assertEquals('GB', $group->country_code); + } + + /** @test */ + public function create_without_lat_lng_still_geocodes(): void + { + $this->apiUser(); + + $response = $this->post('/api/v2/groups?api_token=1234', [ + 'name' => 'Geocoded Group', + 'location' => 'London', + 'description' => 'Some text.', + 'timezone' => 'Europe/London', + ]); + + $response->assertSuccessful(); + $group = Group::findOrFail(json_decode($response->getContent(), true)['id']); + + $this->assertEqualsWithDelta(51.5073509, $group->latitude, 0.00001); + $this->assertEqualsWithDelta(-0.1277583, $group->longitude, 0.00001); + } + + /** @test */ + public function create_with_lat_lng_survives_geocode_failure(): void + { + $this->apiUser(); + + // The location text doesn't geocode, but the user has placed the pin + // themselves — that shouldn't block creation. + $response = $this->post('/api/v2/groups?api_token=1234', [ + 'name' => 'Pin Only Group', + 'location' => 'ForceGeocodeFailure', + 'description' => 'Some text.', + 'timezone' => 'Europe/London', + 'lat' => 48.8566, + 'lng' => 2.3522, + ]); + + $response->assertSuccessful(); + $group = Group::findOrFail(json_decode($response->getContent(), true)['id']); + + $this->assertEqualsWithDelta(48.8566, $group->latitude, 0.00001); + $this->assertEqualsWithDelta(2.3522, $group->longitude, 0.00001); + $this->assertNull($group->country_code); + } + + /** @test */ + public function create_rejects_out_of_range_lat_lng(): void + { + $this->apiUser(); + + $this->expectException(\Illuminate\Validation\ValidationException::class); + + $this->post('/api/v2/groups?api_token=1234', [ + 'name' => 'Bad Coords Group', + 'location' => 'London', + 'description' => 'Some text.', + 'timezone' => 'Europe/London', + 'lat' => 123.0, + 'lng' => -190.0, + ]); + } + + /** @test */ + public function edit_uses_client_lat_lng_when_supplied(): void + { + $this->apiUser(); + + $group = Group::factory()->create([ + 'location' => 'London', + 'latitude' => 51.5073509, + 'longitude' => -0.1277583, + 'country_code' => 'GB', + ]); + + // Location text unchanged, pin dragged. + $response = $this->patch('/api/v2/groups/' . $group->idgroups . '?api_token=1234', [ + 'name' => $group->name, + 'location' => 'London', + 'description' => 'Some text.', + 'timezone' => 'Europe/London', + 'lat' => 51.5540, + 'lng' => -0.1744, + ]); + + $response->assertSuccessful(); + $group->refresh(); + + $this->assertEqualsWithDelta(51.5540, $group->latitude, 0.00001); + $this->assertEqualsWithDelta(-0.1744, $group->longitude, 0.00001); + // Unchanged location text keeps the existing country. + $this->assertEquals('GB', $group->country_code); + } + + /** @test */ + public function edit_without_lat_lng_keeps_existing_behaviour(): void + { + $this->apiUser(); + + $group = Group::factory()->create([ + 'location' => 'London', + 'latitude' => 10.0, + 'longitude' => 20.0, + 'country_code' => 'GB', + ]); + + // Unchanged location, no coords: keeps existing coords (no re-geocode). + $response = $this->patch('/api/v2/groups/' . $group->idgroups . '?api_token=1234', [ + 'name' => $group->name, + 'location' => 'London', + 'description' => 'Some text.', + 'timezone' => 'Europe/London', + ]); + + $response->assertSuccessful(); + $group->refresh(); + + $this->assertEqualsWithDelta(10.0, $group->latitude, 0.00001); + $this->assertEqualsWithDelta(20.0, $group->longitude, 0.00001); + } +}