From 7ca0ba75674966c34df41757ebd6481462a9bfc5 Mon Sep 17 00:00:00 2001 From: edwh Date: Fri, 25 Sep 2026 18:32:15 +0100 Subject: [PATCH] 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 | 36 +++++++--- resources/js/components/GroupActions.test.js | 49 ++++++++++++++ resources/js/components/GroupActions.vue | 15 ++--- tests/Feature/Groups/GroupEditTest.php | 70 ++++++++++++++++++++ 4 files changed, 152 insertions(+), 18 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 d2e6941bfd..4424a3c555 100644 --- a/app/Http/Controllers/API/GroupController.php +++ b/app/Http/Controllers/API/GroupController.php @@ -1060,25 +1060,43 @@ 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, ]; + $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; + } if ($user->hasRole('Administrator') || ($user->hasRole('NetworkCoordinator') && $isCoordinatorForGroup)) { // Got permission to update these. - $data['area'] = $area; - $data['postcode'] = $postcode; - $data['archived_at'] = $archived_at; + foreach (['area' => $area, 'postcode' => $postcode, 'archived_at' => $archived_at] as $column => $value) { + if ($request->has($column)) { + $data[$column] = $value; + } + } } if (isset($_FILES) && !empty($_FILES)) { @@ -1158,7 +1176,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 7534a6d6fe..0b7870b277 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 image_upload(): void { Storage::fake('avatars');