From 49f93807bc864d72676027f0e9c56700adeede3c Mon Sep 17 00:00:00 2001 From: edwh Date: Thu, 15 Jan 2026 14:56:08 +0000 Subject: [PATCH 1/2] Fix negative volunteer counts caused by observer bug The EventsUsersObserver::deleted() method was incorrectly decrementing the volunteer count for ALL deleted events_users records, not just confirmed volunteers. Bug: Line 89 passed 4 parameters to removed() which only accepts 3. The 4th parameter ($eu->status == 1) was silently ignored, so $count was always true, causing incorrect decrements when invitations (non-confirmed users) were deleted. Fix: Pass $eu->status == 1 as the 3rd parameter instead of true. Also includes: - Migration to fix existing incorrect volunteer counts - Three new tests to prevent regression - Updated CLAUDE.md with Task/Docker documentation --- CLAUDE.md | 41 +++- app/Observers/EventsUsersObserver.php | 2 +- ...5_145509_fix_negative_volunteer_counts.php | 50 +++++ .../Feature/Events/AddRemoveVolunteerTest.php | 198 ++++++++++++++++++ 4 files changed, 281 insertions(+), 10 deletions(-) create mode 100644 database/migrations/2026_01_15_145509_fix_negative_volunteer_counts.php diff --git a/CLAUDE.md b/CLAUDE.md index 6d24af4689..2eab5727ff 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -15,17 +15,34 @@ This is a Laravel 9 application with PHP 8+ that integrates with external servic ## Development Commands ### Local Development Setup + +**Prerequisites**: Install [Task](https://taskfile.dev/installation/) - the project uses Taskfile for Docker management. + +See `docs/local-development.md` for full setup instructions. + ```bash -# Using Docker (recommended for full development environment) -docker-compose up -d +# Start Docker environment (uses Task - do NOT use docker-compose directly) +task docker:up-core # Core only (app + database) +task docker:up-debug # With phpMyAdmin and Mailhog +task docker:up-discourse # With Discourse integration +task docker:up-all # All services + +# Stop Docker environment +task docker:down-core # Stop core services +task docker:down-debug # Stop debug services +task docker:down-all # Stop all services + +# Other Docker commands +task docker:logs # View container logs +task docker:shell # Open shell in container +task docker:run:artisan -- migrate # Run artisan commands +task docker:run:bash -- "command" # Run bash commands # The application will be available at: -# - Restarters: http://www.example.com:8001 -# - phpMyAdmin: http://www.example.com:8002 -# - Discourse: http://www.example.com:8003 +# - Restarters: http://localhost:8001 (Admin: jane@bloggs.net / passw0rd) +# - phpMyAdmin: http://localhost:8002 (Host: restarters_db, User: root, Pass: s3cr3t) # - Mailhog: http://localhost:8025 - -# Note: Add www.example.com to your hosts file pointing to your Docker host +# - Discourse: http://localhost:8003 ``` ### Common Development Commands @@ -53,11 +70,17 @@ php artisan key:generate ### Testing ```bash -# Run PHP unit tests +# Run PHP unit tests (inside Docker container) +task docker:shell +# Then inside container: +export DB_TEST_HOST=restarters_db ./vendor/bin/phpunit # Run specific test file -./vendor/bin/phpunit tests/Unit/ExampleTest.php +./vendor/bin/phpunit tests/Feature/Events/AddRemoveVolunteerTest.php + +# Run specific test method +./vendor/bin/phpunit --filter testMethodName # Run JavaScript tests npm run jest diff --git a/app/Observers/EventsUsersObserver.php b/app/Observers/EventsUsersObserver.php index cff09f19b2..2c32cce6c2 100644 --- a/app/Observers/EventsUsersObserver.php +++ b/app/Observers/EventsUsersObserver.php @@ -86,7 +86,7 @@ public function deleted(EventsUsers $eu) $user = $iduser ? User::find($iduser) : null; // Make sure they are not on the thread. If they were confirmed, we need to update the volunteer count. - $this->removed($event, $user, true, $eu->status == 1); + $this->removed($event, $user, $eu->status == 1); } /** diff --git a/database/migrations/2026_01_15_145509_fix_negative_volunteer_counts.php b/database/migrations/2026_01_15_145509_fix_negative_volunteer_counts.php new file mode 100644 index 0000000000..ba8f029533 --- /dev/null +++ b/database/migrations/2026_01_15_145509_fix_negative_volunteer_counts.php @@ -0,0 +1,50 @@ +removed($event, $user, true, $eu->status == 1); + * The removed() method only takes 3 parameters, so the 4th parameter was silently ignored, + * causing $count to always be true and incorrectly decrementing for invited (non-confirmed) users. + * + * @return void + */ + public function up() + { + // Fix all events where the stored volunteer count doesn't match the actual + // count of confirmed volunteers (status = '1' or NULL). + DB::statement(" + UPDATE events e + SET e.volunteers = ( + SELECT COUNT(*) + FROM events_users eu + WHERE eu.event = e.idevents + AND (eu.status = '1' OR eu.status IS NULL) + ) + WHERE e.volunteers != ( + SELECT COUNT(*) + FROM events_users eu2 + WHERE eu2.event = e.idevents + AND (eu2.status = '1' OR eu2.status IS NULL) + ) + "); + } + + /** + * This migration cannot be reversed as we don't know the original incorrect values. + * + * @return void + */ + public function down() + { + // Cannot reverse - the incorrect counts are not recoverable and shouldn't be restored anyway. + } +}; diff --git a/tests/Feature/Events/AddRemoveVolunteerTest.php b/tests/Feature/Events/AddRemoveVolunteerTest.php index db6c9486cb..1658ab8347 100644 --- a/tests/Feature/Events/AddRemoveVolunteerTest.php +++ b/tests/Feature/Events/AddRemoveVolunteerTest.php @@ -271,4 +271,202 @@ public function testAdminRemoveReaddHost() { $response->assertStatus(200); $response->assertSee('', false); } + + /** + * Test that deleting an invited (non-confirmed) volunteer does NOT decrement the volunteer count. + * + * This test demonstrates a bug in EventsUsersObserver::deleted() where the volunteer count + * is decremented for ALL deleted events_users records, not just confirmed ones. + * + * The bug is on line 89 of EventsUsersObserver.php: + * $this->removed($event, $user, true, $eu->status == 1); + * + * The removed() method only takes 3 parameters, so the 4th parameter ($eu->status == 1) + * is silently ignored. This means $count is always true, causing incorrect decrements. + */ + public function testDeletingInvitedVolunteerDoesNotDecrementCount() + { + $this->withoutExceptionHandling(); + Queue::fake(); + + // Create an admin user to perform the operations + $admin = User::factory()->administrator()->create(); + $this->actingAs($admin); + + // Create a group and event + $group = Group::factory()->create(); + $network = Network::factory()->create(); + $network->addGroup($group); + + $event = Party::factory()->create([ + 'group' => $group->idgroups, + 'event_start_utc' => '2130-01-01T12:13:00+00:00', + 'event_end_utc' => '2130-01-01T13:14:00+00:00', + ]); + + // Verify initial state: volunteer count should be 0 + $event->refresh(); + $this->assertEquals(0, $event->volunteers, 'Initial volunteer count should be 0'); + + // Create a user to invite + $invitee = User::factory()->restarter()->create(); + + // Invite the user (this creates an events_users record with status = hash token) + $response = $this->post('/party/invite', [ + 'group_name' => $group->name, + 'event_id' => $event->idevents, + 'manual_invite_box' => $invitee->email, + 'message_to_restarters' => 'Please join our event', + ]); + $response->assertSessionHas('success'); + + // Verify the invitation was created with a hash status (not '1') + $invitation = EventsUsers::where('event', $event->idevents) + ->where('user', $invitee->id) + ->first(); + $this->assertNotNull($invitation, 'Invitation should exist'); + $this->assertNotEquals('1', $invitation->status, 'Invited user should have hash status, not confirmed'); + $this->assertNotNull($invitation->status, 'Invited user should have a status (the hash token)'); + + // Verify volunteer count is still 0 (invited users don't count) + $event->refresh(); + $this->assertEquals(0, $event->volunteers, 'Volunteer count should still be 0 after invitation'); + + // Now delete the invitation (this is where the bug manifests) + $invitation->delete(); + + // THE KEY ASSERTION: After deleting an INVITED (not confirmed) user, + // the volunteer count should still be 0, NOT -1 + $event->refresh(); + $this->assertEquals( + 0, + $event->volunteers, + 'BUG: Deleting an invited (non-confirmed) volunteer should NOT decrement the count. ' . + 'Expected 0, got ' . $event->volunteers . '. ' . + 'This indicates the bug in EventsUsersObserver::deleted() where the 4th parameter is ignored.' + ); + } + + /** + * Test that deleting a CONFIRMED volunteer DOES decrement the volunteer count correctly. + * + * This is the counterpart to testDeletingInvitedVolunteerDoesNotDecrementCount() and verifies + * that confirmed volunteers are handled correctly. + */ + public function testDeletingConfirmedVolunteerDecrementsCount() + { + $this->withoutExceptionHandling(); + Queue::fake(); + + // Create an admin user + $admin = User::factory()->administrator()->create(); + $this->actingAs($admin); + + // Create a group and event + $group = Group::factory()->create(); + $network = Network::factory()->create(); + $network->addGroup($group); + + $event = Party::factory()->create([ + 'group' => $group->idgroups, + 'event_start_utc' => '2130-01-01T12:13:00+00:00', + 'event_end_utc' => '2130-01-01T13:14:00+00:00', + ]); + + // Verify initial state + $event->refresh(); + $this->assertEquals(0, $event->volunteers, 'Initial volunteer count should be 0'); + + // Create and add a confirmed volunteer + $volunteer = User::factory()->restarter()->create(); + + // Add the volunteer as confirmed (status = 1) + $response = $this->put('/api/events/' . $event->idevents . '/volunteers', [ + 'api_token' => $admin->api_token, + 'volunteer_email_address' => $volunteer->email, + 'full_name' => $volunteer->name, + 'user' => $volunteer->id, + ]); + $response->assertJson(['success' => 'success']); + + // Verify volunteer count increased to 1 + $event->refresh(); + $this->assertEquals(1, $event->volunteers, 'Volunteer count should be 1 after adding confirmed volunteer'); + + // Get the events_users record and verify it's confirmed + $eventsUser = EventsUsers::where('event', $event->idevents) + ->where('user', $volunteer->id) + ->first(); + $this->assertNotNull($eventsUser); + $this->assertEquals('1', $eventsUser->status, 'Volunteer should be confirmed (status = 1)'); + + // Delete the confirmed volunteer + $eventsUser->delete(); + + // Verify volunteer count decreased back to 0 + $event->refresh(); + $this->assertEquals( + 0, + $event->volunteers, + 'Volunteer count should be 0 after deleting confirmed volunteer' + ); + } + + /** + * Test multiple invitation deletions cause increasingly negative counts (demonstrates severity of bug). + */ + public function testMultipleInvitationDeletionsCauseNegativeCount() + { + $this->withoutExceptionHandling(); + Queue::fake(); + + $admin = User::factory()->administrator()->create(); + $this->actingAs($admin); + + $group = Group::factory()->create(); + $network = Network::factory()->create(); + $network->addGroup($group); + + $event = Party::factory()->create([ + 'group' => $group->idgroups, + 'event_start_utc' => '2130-01-01T12:13:00+00:00', + 'event_end_utc' => '2130-01-01T13:14:00+00:00', + ]); + + $event->refresh(); + $this->assertEquals(0, $event->volunteers, 'Initial volunteer count should be 0'); + + // Create and invite 5 users + $invitees = []; + for ($i = 0; $i < 5; $i++) { + $invitee = User::factory()->restarter()->create(); + $invitees[] = $invitee; + + $this->post('/party/invite', [ + 'group_name' => $group->name, + 'event_id' => $event->idevents, + 'manual_invite_box' => $invitee->email, + 'message_to_restarters' => 'Please join', + ]); + } + + // Verify count is still 0 + $event->refresh(); + $this->assertEquals(0, $event->volunteers, 'Volunteer count should be 0 after 5 invitations'); + + // Delete all invitations + $invitations = EventsUsers::where('event', $event->idevents)->get(); + foreach ($invitations as $invitation) { + $invitation->delete(); + } + + // THE BUG: This will be -5 instead of 0 + $event->refresh(); + $this->assertEquals( + 0, + $event->volunteers, + 'BUG: After deleting 5 invitations (not confirmed), count should be 0, not ' . $event->volunteers . + '. Each deleted invitation incorrectly decrements the count.' + ); + } } From 0396aa2060fd3678168fe42d712f181466089609 Mon Sep 17 00:00:00 2001 From: edwh Date: Tue, 24 Feb 2026 15:03:33 +0000 Subject: [PATCH 2/2] Replace migration with artisan command for fixing negative volunteer counts Migration was too aggressive - volunteer counts can be manually adjusted, so mismatches aren't necessarily wrong. Only negative counts are always wrong. Replace with targeted artisan command (dev:fixvolunteercount). Co-Authored-By: Claude Opus 4.6 --- app/Console/Commands/FixVolunteerCount.php | 30 +++++------ ...5_145509_fix_negative_volunteer_counts.php | 50 ------------------- 2 files changed, 15 insertions(+), 65 deletions(-) delete mode 100644 database/migrations/2026_01_15_145509_fix_negative_volunteer_counts.php diff --git a/app/Console/Commands/FixVolunteerCount.php b/app/Console/Commands/FixVolunteerCount.php index 1ff50f3e3b..4d0eb9defb 100644 --- a/app/Console/Commands/FixVolunteerCount.php +++ b/app/Console/Commands/FixVolunteerCount.php @@ -20,32 +20,32 @@ class FixVolunteerCount extends Command * * @var string */ - protected $description = 'Fix the volunteer count for all events'; + protected $description = 'Fix negative volunteer counts for events'; /** * Execute the console command. * + * Volunteer counts can be manually incremented/decremented, so we only + * fix cases where the count has gone negative - that is always wrong. + * * @return mixed */ public function handle() { - $events = Party::all(); + $events = Party::where('volunteers', '<', 0)->get(); + + if ($events->isEmpty()) { + $this->info('No events with negative volunteer counts found.'); + return; + } foreach ($events as $event) { $actual = DB::table('events_users')->where('event', $event->idevents)->where('status', 1)->count(); - - if ($actual > $event->volunteers) { - if ($event->volunteers < 0) { - $this->info("Event {$event->idevents} has negative count {$event->volunteers}, $actual have confirmed"); - } else { - $this->info("Event {$event->idevents} has count {$event->volunteers}, but more ($actual) have confirmed"); - } - - $event->volunteers = $actual; - $event->save(); - } else if ($event->volunteers < 0) { - $this->info("Event {$event->idevents} has negative count {$event->volunteers}, fewewr ($actual) have confirmed"); - } + $this->info("Event {$event->idevents}: volunteer count is {$event->volunteers}, setting to {$actual}"); + $event->volunteers = $actual; + $event->save(); } + + $this->info("Fixed {$events->count()} event(s)."); } } diff --git a/database/migrations/2026_01_15_145509_fix_negative_volunteer_counts.php b/database/migrations/2026_01_15_145509_fix_negative_volunteer_counts.php deleted file mode 100644 index ba8f029533..0000000000 --- a/database/migrations/2026_01_15_145509_fix_negative_volunteer_counts.php +++ /dev/null @@ -1,50 +0,0 @@ -removed($event, $user, true, $eu->status == 1); - * The removed() method only takes 3 parameters, so the 4th parameter was silently ignored, - * causing $count to always be true and incorrectly decrementing for invited (non-confirmed) users. - * - * @return void - */ - public function up() - { - // Fix all events where the stored volunteer count doesn't match the actual - // count of confirmed volunteers (status = '1' or NULL). - DB::statement(" - UPDATE events e - SET e.volunteers = ( - SELECT COUNT(*) - FROM events_users eu - WHERE eu.event = e.idevents - AND (eu.status = '1' OR eu.status IS NULL) - ) - WHERE e.volunteers != ( - SELECT COUNT(*) - FROM events_users eu2 - WHERE eu2.event = e.idevents - AND (eu2.status = '1' OR eu2.status IS NULL) - ) - "); - } - - /** - * This migration cannot be reversed as we don't know the original incorrect values. - * - * @return void - */ - public function down() - { - // Cannot reverse - the incorrect counts are not recoverable and shouldn't be restored anyway. - } -};