From 12f61c9953f81bece8f38526635756fdd4e39383 Mon Sep 17 00:00:00 2001 From: edwh Date: Tue, 14 Jul 2026 08:51:40 +0100 Subject: [PATCH 1/2] RES-1863: draggable map for group location fine-tuning Re-implements the intent of the stale RES-1862_draggable_map branch on the current codebase (which already gained a static location map and Places autocomplete since that branch was written). The Mapbox geocoder switch bundled into the old branch is deliberately NOT included - that is RES-1858's concern. - GroupLocationMap becomes draggable: the marker is pinned to the map centre, dragging emits update:lat/update:lng, and a fresh geocode from the location field still recentres the map (with an epsilon guard so the parent echoing our own drag back down doesn't fight the user) - GroupAddEdit syncs the dragged coordinates and sends lat/lng with create/edit; the :id remount hack is gone (it would have destroyed and recreated the map on every drag) - API v2 group create/update accept optional lat/lng, which take precedence over the server-side geocode of the location text; country still comes from geocoding (or the existing record when the location text is unchanged). With an explicit pin, an ungeocodable location string is no longer fatal - the pin wins and country is left null - New partials.dragmap hint (en/fr/fr-BE) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01TzY1phBjyT3HogpeS53zon --- app/Http/Controllers/API/GroupController.php | 35 +++- lang/en/partials.php | 1 + lang/fr-BE/partials.php | 1 + lang/fr/partials.php | 1 + resources/js/components/GroupAddEdit.vue | 13 +- .../js/components/GroupLocationMap.test.js | 75 ++++++++ resources/js/components/GroupLocationMap.vue | 68 ++++--- .../Groups/GroupLatLngOverrideTest.php | 171 ++++++++++++++++++ 8 files changed, 329 insertions(+), 36 deletions(-) create mode 100644 resources/js/components/GroupLocationMap.test.js create mode 100644 tests/Feature/Groups/GroupLatLngOverrideTest.php diff --git a/app/Http/Controllers/API/GroupController.php b/app/Http/Controllers/API/GroupController.php index c4b5abfc21..cf4f30a7b5 100644 --- a/app/Http/Controllers/API/GroupController.php +++ b/app/Http/Controllers/API/GroupController.php @@ -1103,6 +1103,8 @@ private function validateGroupParams(Request $request, $create, ?Group $existing 'location' => ['required', 'max:255'], 'description' => ['required'], 'website' => ['nullable', 'url', 'max:255'], + 'lat' => ['nullable', 'numeric', 'between:-90,90', 'required_with:lng'], + 'lng' => ['nullable', 'numeric', 'between:-180,180', 'required_with:lat'], ]); } else { $request->validate([ @@ -1110,6 +1112,8 @@ private function validateGroupParams(Request $request, $create, ?Group $existing 'location' => ['max:255'], 'website' => ['nullable', 'url', 'max:255'], 'archived_at' => ['nullable', 'date'], + 'lat' => ['nullable', 'numeric', 'between:-90,90', 'required_with:lng'], + 'lng' => ['nullable', 'numeric', 'between:-180,180', 'required_with:lat'], ]); } @@ -1133,6 +1137,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 @@ -1148,18 +1158,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..476b3de7ac 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' => 'Map marker not quite right? Feel free to drag around to fix it.', '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..6a8ccfea92 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' => 'Marqueur de carte pas tout à fait exact ? N\'hésitez pas à le faire 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..6a8ccfea92 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' => 'Marqueur de carte pas tout à fait exact ? N\'hésitez pas à le faire 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 686b33de52..9b38dee670 100644 --- a/resources/js/components/GroupAddEdit.vue +++ b/resources/js/components/GroupAddEdit.vue @@ -59,11 +59,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..4a32495a7e --- /dev/null +++ b/resources/js/components/GroupLocationMap.test.js @@ -0,0 +1,75 @@ +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', + template: '
' +} + +const LMarkerStub = { + name: 'l-marker', + props: ['latLng'], + 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 + } + }) +} + +test('shows the drag hint', () => { + const wrapper = mountMap() + expect(wrapper.text()).toContain('partials.dragmap') +}) + +test('dragging the map emits updated lat/lng', async () => { + const wrapper = mountMap() + + wrapper.findComponent(LMapStub).vm.$emit('update:center', { lat: 52.1, lng: 1.3 }) + await wrapper.vm.$nextTick() + + expect(wrapper.emitted('update:lat').pop()).toEqual([52.1]) + expect(wrapper.emitted('update:lng').pop()).toEqual([1.3]) +}) + +test('marker follows a dragged center', async () => { + const wrapper = mountMap() + + wrapper.findComponent(LMapStub).vm.$emit('update:center', { lat: 48.85, lng: 2.35 }) + await wrapper.vm.$nextTick() + + expect(wrapper.findComponent(LMarkerStub).props('latLng')).toEqual([48.85, 2.35]) +}) + +test('an external lat/lng change (new geocode) recentres 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]) +}) + +test('prop echo of a drag does not re-emit', async () => { + const wrapper = mountMap() + + wrapper.findComponent(LMapStub).vm.$emit('update:center', { lat: 52.1, lng: 1.3 }) + await wrapper.vm.$nextTick() + const emitsAfterDrag = wrapper.emitted('update:lat').length + + // Parent syncs the emitted values straight back down. + await wrapper.setProps({ lat: 52.1, lng: 1.3 }) + + expect(wrapper.emitted('update:lat').length).toBe(emitsAfterDrag) +}) diff --git a/resources/js/components/GroupLocationMap.vue b/resources/js/components/GroupLocationMap.vue index 84d7ddb9d8..308cae6ea3 100644 --- a/resources/js/components/GroupLocationMap.vue +++ b/resources/js/components/GroupLocationMap.vue @@ -1,18 +1,28 @@ \ 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); + } +} From d574d6f782bab249525ab3d6d26f8e0a246400fa Mon Sep 17 00:00:00 2001 From: edwh Date: Wed, 23 Sep 2026 16:01:11 +0100 Subject: [PATCH 2/2] Group location map: drag the pin, not the map User-testing feedback on #895: dragging the map to move a centre-pinned marker was counter-intuitive, wheel-zoom moved the location, you couldn't pan without moving the pin, and the initial view was too far out. - The marker is draggable and is the location; panning and zooming (wheel or buttons) just move the view. - Start at zoom 16 on the location. - Help text is a small muted line below the map, like the form's other hints, and says to drag the pin. Co-Authored-By: Claude Opus 5.5 (1M context) --- lang/en/partials.php | 2 +- lang/fr-BE/partials.php | 2 +- lang/fr/partials.php | 2 +- .../js/components/GroupLocationMap.test.js | 36 +++++++++++----- resources/js/components/GroupLocationMap.vue | 42 +++++++++---------- 5 files changed, 50 insertions(+), 34 deletions(-) diff --git a/lang/en/partials.php b/lang/en/partials.php index 476b3de7ac..ba27f121b3 100644 --- a/lang/en/partials.php +++ b/lang/en/partials.php @@ -84,7 +84,7 @@ 'notification_greeting' => 'Hello!', 'confirm' => 'Confirm', 'validate_timezone' => 'Please select a valid timezone.', - 'dragmap' => 'Map marker not quite right? Feel free to drag around to fix it.', + '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 6a8ccfea92..6a02cce647 100644 --- a/lang/fr-BE/partials.php +++ b/lang/fr-BE/partials.php @@ -87,7 +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' => 'Marqueur de carte pas tout à fait exact ? N\'hésitez pas à le faire glisser pour le corriger.', + '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 6a8ccfea92..6a02cce647 100644 --- a/lang/fr/partials.php +++ b/lang/fr/partials.php @@ -87,7 +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' => 'Marqueur de carte pas tout à fait exact ? N\'hésitez pas à le faire glisser pour le corriger.', + '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/GroupLocationMap.test.js b/resources/js/components/GroupLocationMap.test.js index 4a32495a7e..126284b2b8 100644 --- a/resources/js/components/GroupLocationMap.test.js +++ b/resources/js/components/GroupLocationMap.test.js @@ -8,12 +8,13 @@ import GroupLocationMap from './GroupLocationMap.vue' const LMapStub = { name: 'l-map', + props: ['center', 'zoom'], template: '
' } const LMarkerStub = { name: 'l-marker', - props: ['latLng'], + props: ['latLng', 'draggable'], template: '
' } @@ -29,47 +30,62 @@ function mountMap (props = {}) { }) } +// 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('dragging the map emits updated lat/lng', async () => { +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() - wrapper.findComponent(LMapStub).vm.$emit('update:center', { lat: 52.1, lng: 1.3 }) + 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('marker follows a dragged center', async () => { +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.findComponent(LMarkerStub).props('latLng')).toEqual([48.85, 2.35]) + 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) recentres the map', async () => { +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 re-emit', async () => { +test('prop echo of a drag does not recentre the map', async () => { const wrapper = mountMap() - wrapper.findComponent(LMapStub).vm.$emit('update:center', { lat: 52.1, lng: 1.3 }) + dragMarkerTo(wrapper, 52.1, 1.3) await wrapper.vm.$nextTick() - const emitsAfterDrag = wrapper.emitted('update:lat').length // Parent syncs the emitted values straight back down. await wrapper.setProps({ lat: 52.1, lng: 1.3 }) - expect(wrapper.emitted('update:lat').length).toBe(emitsAfterDrag) + 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 308cae6ea3..f3aaf42ff9 100644 --- a/resources/js/components/GroupLocationMap.vue +++ b/resources/js/components/GroupLocationMap.vue @@ -1,19 +1,16 @@