From 542e4444b7e34441a6f3fb9343f19df447430c93 Mon Sep 17 00:00:00 2001 From: edwh Date: Wed, 23 Sep 2026 16:05:30 +0100 Subject: [PATCH 1/4] Hosts can edit their group's postcode Previously only admins and network coordinators could, although hosts set it when creating the group. Area stays admin/coordinator only. The postcode is only written when the request sends it, so partial updates don't blank it. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/Http/Controllers/API/GroupController.php | 7 ++++- resources/js/components/GroupAddEdit.vue | 1 - resources/js/components/GroupLocation.vue | 7 +---- tests/Feature/Groups/GroupEditTest.php | 27 ++++++++++++++++++++ 4 files changed, 34 insertions(+), 8 deletions(-) diff --git a/app/Http/Controllers/API/GroupController.php b/app/Http/Controllers/API/GroupController.php index d2e6941bfd..3d8b69c066 100644 --- a/app/Http/Controllers/API/GroupController.php +++ b/app/Http/Controllers/API/GroupController.php @@ -1074,10 +1074,15 @@ public function updateGroupv2(Request $request, $idGroup): JsonResponse { 'email' => $email, ]; + // Hosts can edit the postcode (area stays admin/coordinator only, below). Only touch it when sent, so a + // partial update doesn't blank it. + if ($request->has('postcode')) { + $data['postcode'] = $postcode; + } + if ($user->hasRole('Administrator') || ($user->hasRole('NetworkCoordinator') && $isCoordinatorForGroup)) { // Got permission to update these. $data['area'] = $area; - $data['postcode'] = $postcode; $data['archived_at'] = $archived_at; } diff --git a/resources/js/components/GroupAddEdit.vue b/resources/js/components/GroupAddEdit.vue index 686b33de52..fa87ffbda8 100644 --- a/resources/js/components/GroupAddEdit.vue +++ b/resources/js/components/GroupAddEdit.vue @@ -53,7 +53,6 @@ :lat.sync="lat" :lng.sync="lng" :postcode.sync="postcode" - :can-edit-postcode="canApprove" class="group-location" :has-error="$v.location.$error" ref="location" diff --git a/resources/js/components/GroupLocation.vue b/resources/js/components/GroupLocation.vue index 5d94e467b9..eb40780b3c 100644 --- a/resources/js/components/GroupLocation.vue +++ b/resources/js/components/GroupLocation.vue @@ -24,7 +24,7 @@ - + {{ __('groups.groups_postcode_small') }} @@ -77,11 +77,6 @@ export default { type: Boolean, required: false, default: false - }, - canEditPostcode: { - type: Boolean, - required: false, - default: false } }, components: { diff --git a/tests/Feature/Groups/GroupEditTest.php b/tests/Feature/Groups/GroupEditTest.php index 7534a6d6fe..10d6e3949a 100644 --- a/tests/Feature/Groups/GroupEditTest.php +++ b/tests/Feature/Groups/GroupEditTest.php @@ -92,6 +92,33 @@ public function invalid_location(): void ]); } + /** @test */ + public function host_can_edit_postcode_but_not_area(): void { + $group = Group::factory()->create(['postcode' => 'SW9 7QD', 'area' => 'London']); + + $host = User::factory()->host()->create(); + $group->addVolunteer($host); + $group->makeMemberAHost($host); + $this->actingAs($host); + + $response = $this->patch('/api/v2/groups/' . $group->idgroups, [ + 'postcode' => 'E8 1AA', + 'area' => 'Elsewhere', + ]); + $response->assertSuccessful(); + + $group->refresh(); + $this->assertEquals('E8 1AA', $group->postcode); + $this->assertEquals('London', $group->area); + + // A partial update that doesn't send the postcode leaves it alone. + $response = $this->patch('/api/v2/groups/' . $group->idgroups, [ + 'network_data' => ['foo' => 'bar'], + ]); + $response->assertSuccessful(); + $this->assertEquals('E8 1AA', $group->refresh()->postcode); + } + /** @test */ public function image_upload(): void { Storage::fake('avatars'); From 810a2f745e6a1cb3115bbdd824020f33c0adcb92 Mon Sep 17 00:00:00 2001 From: edwh Date: Fri, 25 Sep 2026 16:35:38 +0100 Subject: [PATCH 2/4] Group postcode: max 32 characters (the column size), server and form Co-Authored-By: Claude Opus 5.5 (1M context) --- app/Http/Controllers/API/GroupController.php | 2 ++ resources/js/components/GroupLocation.vue | 2 +- tests/Feature/Groups/GroupEditTest.php | 6 ++++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/Http/Controllers/API/GroupController.php b/app/Http/Controllers/API/GroupController.php index 3d8b69c066..41a3f23ec7 100644 --- a/app/Http/Controllers/API/GroupController.php +++ b/app/Http/Controllers/API/GroupController.php @@ -1217,6 +1217,7 @@ private function validateGroupParams(Request $request, $create, ?Group $existing 'location' => ['required', 'max:255'], 'description' => ['required'], 'website' => ['nullable', 'url', 'max:255'], + 'postcode' => ['nullable', 'max:32'], ]); } else { $request->validate([ @@ -1224,6 +1225,7 @@ private function validateGroupParams(Request $request, $create, ?Group $existing 'location' => ['max:255'], 'website' => ['nullable', 'url', 'max:255'], 'archived_at' => ['nullable', 'date'], + 'postcode' => ['nullable', 'max:32'], ]); } diff --git a/resources/js/components/GroupLocation.vue b/resources/js/components/GroupLocation.vue index eb40780b3c..9ddaa199e8 100644 --- a/resources/js/components/GroupLocation.vue +++ b/resources/js/components/GroupLocation.vue @@ -24,7 +24,7 @@ - + {{ __('groups.groups_postcode_small') }} diff --git a/tests/Feature/Groups/GroupEditTest.php b/tests/Feature/Groups/GroupEditTest.php index 10d6e3949a..f02bc3d600 100644 --- a/tests/Feature/Groups/GroupEditTest.php +++ b/tests/Feature/Groups/GroupEditTest.php @@ -117,6 +117,12 @@ public function host_can_edit_postcode_but_not_area(): void { ]); $response->assertSuccessful(); $this->assertEquals('E8 1AA', $group->refresh()->postcode); + + // Longer than the column. + $this->expectException(\Illuminate\Validation\ValidationException::class); + $this->patch('/api/v2/groups/' . $group->idgroups, [ + 'postcode' => str_repeat('X', 33), + ]); } /** @test */ From de5e71833eee672138cbe44f21d240da6bdd06a3 Mon Sep 17 00:00:00 2001 From: edwh Date: Fri, 25 Sep 2026 18:32:15 +0100 Subject: [PATCH 3/4] Group updates change only the fields sent; archive sends only the date - updateGroupv2 updates only the fields present in the request (location brings its geocoded lat/lng/country with it). A partial update previously blanked everything else: e.g. sending just phone wiped the name, location, coordinates, description, website, timezone, email and network_data. - A future-event timezone change only happens when timezone is sent; omitting it no longer counts as a change to null. - The Archive group action sends only {id, archived_at}. It sent the whole store group back, which saved network_data as "[object Object]". Co-Authored-By: Claude Opus 5.5 (1M context) --- app/Http/Controllers/API/GroupController.php | 40 +++++++---- resources/js/components/GroupActions.test.js | 49 ++++++++++++++ resources/js/components/GroupActions.vue | 15 ++--- tests/Feature/Groups/GroupEditTest.php | 70 ++++++++++++++++++++ 4 files changed, 152 insertions(+), 22 deletions(-) create mode 100644 resources/js/components/GroupActions.test.js diff --git a/app/Http/Controllers/API/GroupController.php b/app/Http/Controllers/API/GroupController.php index 41a3f23ec7..b9934db967 100644 --- a/app/Http/Controllers/API/GroupController.php +++ b/app/Http/Controllers/API/GroupController.php @@ -1060,30 +1060,44 @@ public function updateGroupv2(Request $request, $idGroup): JsonResponse { $old_zone = $group->timezone; - $data = [ + // Only update the fields the request sends. A partial update (e.g. the archive action, or an API client + // setting one field) must leave the rest of the group alone rather than blanking it. + $fields = [ 'name' => $name, 'website' => $website, - 'location' => $location, - 'latitude' => $latitude, - 'longitude' => $longitude, - 'country_code' => $country, 'free_text' => $description, 'timezone' => $timezone, 'phone' => $phone, 'network_data' => $network_data, 'email' => $email, + 'postcode' => $postcode, ]; + $requestKeys = [ + 'free_text' => 'description', + ]; + + $data = []; + foreach ($fields as $column => $value) { + if ($request->has($requestKeys[$column] ?? $column)) { + $data[$column] = $value; + } + } - // Hosts can edit the postcode (area stays admin/coordinator only, below). Only touch it when sent, so a - // partial update doesn't blank it. - if ($request->has('postcode')) { - $data['postcode'] = $postcode; + if ($request->has('location')) { + // The coordinates and country come from geocoding the location. + $data['location'] = $location; + $data['latitude'] = $latitude; + $data['longitude'] = $longitude; + $data['country_code'] = $country; } if ($user->hasRole('Administrator') || ($user->hasRole('NetworkCoordinator') && $isCoordinatorForGroup)) { - // Got permission to update these. - $data['area'] = $area; - $data['archived_at'] = $archived_at; + // Got permission to update these. Area is admin/coordinator only; hosts can edit the postcode. + foreach (['area' => $area, 'archived_at' => $archived_at] as $column => $value) { + if ($request->has($column)) { + $data[$column] = $value; + } + } } if (isset($_FILES) && !empty($_FILES)) { @@ -1163,7 +1177,7 @@ public function updateGroupv2(Request $request, $idGroup): JsonResponse { } } - if ($timezone != $old_zone) { + if (array_key_exists('timezone', $data) && $timezone != $old_zone) { // The timezone of the group has changed. Update the zone of any future events. This happens // sometimes when a group is created and events are created before the group is approved (and therefore // before the admin has a chance to set the zone on the group. diff --git a/resources/js/components/GroupActions.test.js b/resources/js/components/GroupActions.test.js new file mode 100644 index 0000000000..dbba05c525 --- /dev/null +++ b/resources/js/components/GroupActions.test.js @@ -0,0 +1,49 @@ +import Vue from 'vue' +import Vuex from 'vuex' +import { BootstrapVue } from 'bootstrap-vue' +import { createLocalVue, shallowMount } from '@vue/test-utils' +import LangMixin from 'resources/js/mixins/lang.js' +import GroupActions from './GroupActions.vue' + +const localVue = createLocalVue() +localVue.use(Vuex) +Vue.use(BootstrapVue) + +function makeStore (edit, fetch) { + return new Vuex.Store({ + modules: { + groups: { + namespaced: true, + getters: { + // The store's copy of a group, including object-valued fields. + get: () => (id) => ({ + idgroups: id, + name: 'Test Group', + free_text: 'About us', + network_data: { foo: 'bar' }, + networks: [{ id: 1 }], + }), + }, + actions: { edit, fetch }, + }, + }, + }) +} + +test('archiving sends only the group id and archive date', async () => { + const edit = jest.fn() + const wrapper = shallowMount(GroupActions, { + localVue, + store: makeStore(edit, jest.fn()), + mixins: [LangMixin], + propsData: { idgroups: 42, canPerformArchive: true }, + }) + + await wrapper.vm.archiveConfirmed() + + expect(edit).toHaveBeenCalledTimes(1) + const payload = edit.mock.calls[0][1] + expect(Object.keys(payload).sort()).toEqual(['archived_at', 'id']) + expect(payload.id).toBe(42) + expect(isNaN(Date.parse(payload.archived_at))).toBe(false) +}) diff --git a/resources/js/components/GroupActions.vue b/resources/js/components/GroupActions.vue index f61e856896..778ec7e975 100644 --- a/resources/js/components/GroupActions.vue +++ b/resources/js/components/GroupActions.vue @@ -126,15 +126,12 @@ export default { form.submit() }, async archiveConfirmed() { - const group = this.group - group.id = this.idgroups - group.archived_at = (new Date()).toISOString() - group.description = group.free_text - - // Make sure we don't stomp on the networks. - delete group.networks - - await this.$store.dispatch('groups/edit', group) + // Send only the archive date: the API updates just the fields it's given. Sending the whole group back + // posted object fields such as network_data as the string "[object Object]". + await this.$store.dispatch('groups/edit', { + id: this.idgroups, + archived_at: (new Date()).toISOString() + }) await this.$store.dispatch('groups/fetch', { id: this.idgroups diff --git a/tests/Feature/Groups/GroupEditTest.php b/tests/Feature/Groups/GroupEditTest.php index f02bc3d600..8c8d0d2b7b 100644 --- a/tests/Feature/Groups/GroupEditTest.php +++ b/tests/Feature/Groups/GroupEditTest.php @@ -92,6 +92,76 @@ public function invalid_location(): void ]); } + /** @test */ + public function partial_update_only_changes_the_fields_sent(): void { + $group = Group::factory()->create([ + 'name' => 'Partial Group', + 'location' => 'Hackney, London', + 'latitude' => 51.545, + 'longitude' => -0.0553, + 'country_code' => 'GB', + 'free_text' => 'About us', + 'website' => 'https://example.org', + 'timezone' => 'Europe/London', + 'email' => 'group@example.org', + 'postcode' => 'E8 1AA', + 'area' => 'London', + 'network_data' => ['foo' => 'bar'], + ]); + $event = \App\Party::factory()->create([ + 'group' => $group->idgroups, + 'event_start_utc' => Carbon::now()->addWeek()->toIso8601String(), + 'event_end_utc' => Carbon::now()->addWeek()->addHours(2)->toIso8601String(), + 'timezone' => 'Europe/London', + ]); + + $host = User::factory()->host()->create(); + $group->addVolunteer($host); + $group->makeMemberAHost($host); + $this->actingAs($host); + + $this->patch('/api/v2/groups/' . $group->idgroups, ['phone' => '999'])->assertSuccessful(); + + $group->refresh(); + $this->assertEquals('999', $group->phone); + $this->assertEquals('Partial Group', $group->name); + $this->assertEquals('Hackney, London', $group->location); + $this->assertEquals(51.545, $group->latitude); + $this->assertEquals(-0.0553, $group->longitude); + $this->assertEquals('GB', $group->country_code); + $this->assertEquals('About us', $group->free_text); + $this->assertEquals('https://example.org', $group->website); + $this->assertEquals('Europe/London', $group->timezone); + $this->assertEquals('group@example.org', $group->email); + $this->assertEquals('E8 1AA', $group->postcode); + $this->assertEquals('London', $group->area); + $this->assertEquals(['foo' => 'bar'], $group->network_data); + + // Not sending the timezone isn't a timezone change: future events keep theirs. + $this->assertEquals('Europe/London', $event->fresh()->timezone); + } + + /** @test */ + public function archiving_with_only_the_date_keeps_the_rest_of_the_group(): void { + $group = Group::factory()->create([ + 'name' => 'Archive Me', + 'network_data' => ['foo' => 'bar'], + ]); + + $admin = User::factory()->administrator()->create(); + $this->actingAs($admin); + + // What the Archive group action sends. + $this->patch('/api/v2/groups/' . $group->idgroups, [ + 'archived_at' => Carbon::now()->toIso8601String(), + ])->assertSuccessful(); + + $group->refresh(); + $this->assertNotNull($group->archived_at); + $this->assertEquals('Archive Me', $group->name); + $this->assertEquals(['foo' => 'bar'], $group->network_data); + } + /** @test */ public function host_can_edit_postcode_but_not_area(): void { $group = Group::factory()->create(['postcode' => 'SW9 7QD', 'area' => 'London']); From 759b85d4ab8aebc1411bd4cfceda7385ca9e6225 Mon Sep 17 00:00:00 2001 From: edwh Date: Thu, 1 Oct 2026 17:44:35 +0100 Subject: [PATCH 4/4] Revert "Group updates change only the fields sent; archive sends only the date" This reverts commit de5e71833eee672138cbe44f21d240da6bdd06a3. --- app/Http/Controllers/API/GroupController.php | 40 ++++------- resources/js/components/GroupActions.test.js | 49 -------------- resources/js/components/GroupActions.vue | 15 +++-- tests/Feature/Groups/GroupEditTest.php | 70 -------------------- 4 files changed, 22 insertions(+), 152 deletions(-) delete mode 100644 resources/js/components/GroupActions.test.js diff --git a/app/Http/Controllers/API/GroupController.php b/app/Http/Controllers/API/GroupController.php index b9934db967..41a3f23ec7 100644 --- a/app/Http/Controllers/API/GroupController.php +++ b/app/Http/Controllers/API/GroupController.php @@ -1060,44 +1060,30 @@ public function updateGroupv2(Request $request, $idGroup): JsonResponse { $old_zone = $group->timezone; - // Only update the fields the request sends. A partial update (e.g. the archive action, or an API client - // setting one field) must leave the rest of the group alone rather than blanking it. - $fields = [ + $data = [ 'name' => $name, 'website' => $website, + 'location' => $location, + 'latitude' => $latitude, + 'longitude' => $longitude, + 'country_code' => $country, 'free_text' => $description, 'timezone' => $timezone, 'phone' => $phone, 'network_data' => $network_data, 'email' => $email, - 'postcode' => $postcode, ]; - $requestKeys = [ - 'free_text' => 'description', - ]; - - $data = []; - foreach ($fields as $column => $value) { - if ($request->has($requestKeys[$column] ?? $column)) { - $data[$column] = $value; - } - } - if ($request->has('location')) { - // The coordinates and country come from geocoding the location. - $data['location'] = $location; - $data['latitude'] = $latitude; - $data['longitude'] = $longitude; - $data['country_code'] = $country; + // Hosts can edit the postcode (area stays admin/coordinator only, below). Only touch it when sent, so a + // partial update doesn't blank it. + if ($request->has('postcode')) { + $data['postcode'] = $postcode; } if ($user->hasRole('Administrator') || ($user->hasRole('NetworkCoordinator') && $isCoordinatorForGroup)) { - // Got permission to update these. Area is admin/coordinator only; hosts can edit the postcode. - foreach (['area' => $area, 'archived_at' => $archived_at] as $column => $value) { - if ($request->has($column)) { - $data[$column] = $value; - } - } + // Got permission to update these. + $data['area'] = $area; + $data['archived_at'] = $archived_at; } if (isset($_FILES) && !empty($_FILES)) { @@ -1177,7 +1163,7 @@ public function updateGroupv2(Request $request, $idGroup): JsonResponse { } } - if (array_key_exists('timezone', $data) && $timezone != $old_zone) { + if ($timezone != $old_zone) { // The timezone of the group has changed. Update the zone of any future events. This happens // sometimes when a group is created and events are created before the group is approved (and therefore // before the admin has a chance to set the zone on the group. diff --git a/resources/js/components/GroupActions.test.js b/resources/js/components/GroupActions.test.js deleted file mode 100644 index dbba05c525..0000000000 --- a/resources/js/components/GroupActions.test.js +++ /dev/null @@ -1,49 +0,0 @@ -import Vue from 'vue' -import Vuex from 'vuex' -import { BootstrapVue } from 'bootstrap-vue' -import { createLocalVue, shallowMount } from '@vue/test-utils' -import LangMixin from 'resources/js/mixins/lang.js' -import GroupActions from './GroupActions.vue' - -const localVue = createLocalVue() -localVue.use(Vuex) -Vue.use(BootstrapVue) - -function makeStore (edit, fetch) { - return new Vuex.Store({ - modules: { - groups: { - namespaced: true, - getters: { - // The store's copy of a group, including object-valued fields. - get: () => (id) => ({ - idgroups: id, - name: 'Test Group', - free_text: 'About us', - network_data: { foo: 'bar' }, - networks: [{ id: 1 }], - }), - }, - actions: { edit, fetch }, - }, - }, - }) -} - -test('archiving sends only the group id and archive date', async () => { - const edit = jest.fn() - const wrapper = shallowMount(GroupActions, { - localVue, - store: makeStore(edit, jest.fn()), - mixins: [LangMixin], - propsData: { idgroups: 42, canPerformArchive: true }, - }) - - await wrapper.vm.archiveConfirmed() - - expect(edit).toHaveBeenCalledTimes(1) - const payload = edit.mock.calls[0][1] - expect(Object.keys(payload).sort()).toEqual(['archived_at', 'id']) - expect(payload.id).toBe(42) - expect(isNaN(Date.parse(payload.archived_at))).toBe(false) -}) diff --git a/resources/js/components/GroupActions.vue b/resources/js/components/GroupActions.vue index 778ec7e975..f61e856896 100644 --- a/resources/js/components/GroupActions.vue +++ b/resources/js/components/GroupActions.vue @@ -126,12 +126,15 @@ export default { form.submit() }, async archiveConfirmed() { - // Send only the archive date: the API updates just the fields it's given. Sending the whole group back - // posted object fields such as network_data as the string "[object Object]". - await this.$store.dispatch('groups/edit', { - id: this.idgroups, - archived_at: (new Date()).toISOString() - }) + const group = this.group + group.id = this.idgroups + group.archived_at = (new Date()).toISOString() + group.description = group.free_text + + // Make sure we don't stomp on the networks. + delete group.networks + + await this.$store.dispatch('groups/edit', group) await this.$store.dispatch('groups/fetch', { id: this.idgroups diff --git a/tests/Feature/Groups/GroupEditTest.php b/tests/Feature/Groups/GroupEditTest.php index 8c8d0d2b7b..f02bc3d600 100644 --- a/tests/Feature/Groups/GroupEditTest.php +++ b/tests/Feature/Groups/GroupEditTest.php @@ -92,76 +92,6 @@ public function invalid_location(): void ]); } - /** @test */ - public function partial_update_only_changes_the_fields_sent(): void { - $group = Group::factory()->create([ - 'name' => 'Partial Group', - 'location' => 'Hackney, London', - 'latitude' => 51.545, - 'longitude' => -0.0553, - 'country_code' => 'GB', - 'free_text' => 'About us', - 'website' => 'https://example.org', - 'timezone' => 'Europe/London', - 'email' => 'group@example.org', - 'postcode' => 'E8 1AA', - 'area' => 'London', - 'network_data' => ['foo' => 'bar'], - ]); - $event = \App\Party::factory()->create([ - 'group' => $group->idgroups, - 'event_start_utc' => Carbon::now()->addWeek()->toIso8601String(), - 'event_end_utc' => Carbon::now()->addWeek()->addHours(2)->toIso8601String(), - 'timezone' => 'Europe/London', - ]); - - $host = User::factory()->host()->create(); - $group->addVolunteer($host); - $group->makeMemberAHost($host); - $this->actingAs($host); - - $this->patch('/api/v2/groups/' . $group->idgroups, ['phone' => '999'])->assertSuccessful(); - - $group->refresh(); - $this->assertEquals('999', $group->phone); - $this->assertEquals('Partial Group', $group->name); - $this->assertEquals('Hackney, London', $group->location); - $this->assertEquals(51.545, $group->latitude); - $this->assertEquals(-0.0553, $group->longitude); - $this->assertEquals('GB', $group->country_code); - $this->assertEquals('About us', $group->free_text); - $this->assertEquals('https://example.org', $group->website); - $this->assertEquals('Europe/London', $group->timezone); - $this->assertEquals('group@example.org', $group->email); - $this->assertEquals('E8 1AA', $group->postcode); - $this->assertEquals('London', $group->area); - $this->assertEquals(['foo' => 'bar'], $group->network_data); - - // Not sending the timezone isn't a timezone change: future events keep theirs. - $this->assertEquals('Europe/London', $event->fresh()->timezone); - } - - /** @test */ - public function archiving_with_only_the_date_keeps_the_rest_of_the_group(): void { - $group = Group::factory()->create([ - 'name' => 'Archive Me', - 'network_data' => ['foo' => 'bar'], - ]); - - $admin = User::factory()->administrator()->create(); - $this->actingAs($admin); - - // What the Archive group action sends. - $this->patch('/api/v2/groups/' . $group->idgroups, [ - 'archived_at' => Carbon::now()->toIso8601String(), - ])->assertSuccessful(); - - $group->refresh(); - $this->assertNotNull($group->archived_at); - $this->assertEquals('Archive Me', $group->name); - $this->assertEquals(['foo' => 'bar'], $group->network_data); - } - /** @test */ public function host_can_edit_postcode_but_not_area(): void { $group = Group::factory()->create(['postcode' => 'SW9 7QD', 'area' => 'London']);