From d64f0c52bef044e99e60b6b858e7f2869e9dc4a5 Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Fri, 16 May 2025 12:51:27 +0200 Subject: [PATCH] Fixed bugs in current organization; Add database consistency checks; Add foreign key --- .../SelfHost/SelfHostDatabaseConsistency.php | 123 +++++++++++++ app/Console/Kernel.php | 4 + app/Events/DatabaseSeederAfterSeed.php | 14 ++ app/Events/DatabaseSeederBeforeDelete.php | 14 ++ app/Service/DeletionService.php | 6 + app/Service/MemberService.php | 6 + app/Service/UserService.php | 8 +- config/scheduling.php | 1 + ...key_for_current_team_id_in_users_table.php | 43 +++++ database/seeders/DatabaseSeeder.php | 8 + ...SelfHostDatabaseConsistencyCommandTest.php | 163 ++++++++++++++++++ tests/Unit/Service/DeletionServiceTest.php | 18 ++ tests/Unit/Service/MemberServiceTest.php | 35 ++++ 13 files changed, 440 insertions(+), 3 deletions(-) create mode 100644 app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php create mode 100644 app/Events/DatabaseSeederAfterSeed.php create mode 100644 app/Events/DatabaseSeederBeforeDelete.php create mode 100644 database/migrations/2025_05_16_075757_add_foreign_key_for_current_team_id_in_users_table.php create mode 100644 tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php diff --git a/app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php b/app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php new file mode 100644 index 00000000..2c821468 --- /dev/null +++ b/app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php @@ -0,0 +1,123 @@ +select(['time_entries.id as id']) + ->join('tasks', 'time_entries.task_id', '=', 'tasks.id') + ->where('tasks.project_id', '!=', DB::raw('time_entries.project_id')) + ->get(); + $this->logProblems($problems, 'Time entries have a task that does not belong to the project of the time entry', $hadAProblem); + + // Client id is the client id of the project + $problems = DB::table('time_entries') + ->select(['time_entries.id as id']) + ->join('projects', 'time_entries.project_id', '=', 'projects.id') + ->where(DB::raw('coalesce(projects.client_id::varchar, \'\')'), '!=', DB::raw('coalesce(time_entries.client_id::varchar, \'\')')) + ->get(); + $this->logProblems($problems, 'Time entries have a client that does not match the client of the project', $hadAProblem); + + // Client id can only be not null if the project id is not null + $problems = DB::table('time_entries') + ->select(['time_entries.id as id']) + ->whereNotNull('client_id') + ->whereNull('project_id') + ->get(); + $this->logProblems($problems, 'Time entries have a client but no project', $hadAProblem); + + // Every user needs to be a member of at least one organization + $problems = DB::table('users') + ->select(['users.id as id']) + ->leftJoin('members', 'users.id', '=', 'members.user_id') + ->whereNull('members.id') + ->get(); + $this->logProblems($problems, 'Users are not member of any organization', $hadAProblem); + + // Every organization needs at least an owner + $problems = DB::table('organizations') + ->select(['organizations.id as id']) + ->leftJoin('members', function (JoinClause $join): void { + $join->on('organizations.id', '=', 'members.organization_id') + ->where('members.role', '=', 'owner'); + }) + ->whereNull('members.id') + ->get(); + $this->logProblems($problems, 'Organizations without an owner', $hadAProblem); + + // Every member can only have one running time entry + $problems = DB::table('time_entries') + ->select(['user_id as id']) + ->whereNull('end') + ->groupBy('user_id') + ->havingRaw('count(*) > 1') + ->get(['user_id', DB::raw('count(*) as count')]); + $this->logProblems($problems, 'Users with more than one running time entry', $hadAProblem); + + // Users have a current organization that they are not a member of + $problems = DB::table('users') + ->select(['users.id as id']) + ->whereNotNull('current_team_id') + ->whereNotIn('current_team_id', function (Builder $query): void { + $query->select('organization_id') + ->from('members') + ->whereColumn('members.user_id', 'users.id'); + })->get(); + $this->logProblems($problems, 'Users have a current organization that they are not a member of', $hadAProblem); + + return $hadAProblem ? self::FAILURE : self::SUCCESS; + } + + /** + * @param Collection $problems + */ + private function logProblems(Collection $problems, string $message, bool &$hadAProblem): void + { + $message = 'Consistency problem: '.$message; + if ($problems->isNotEmpty()) { + $ids = $problems->pluck('id'); + $hadAProblem = true; + Log::error($message, [ + 'ids' => $ids, + ]); + + $error = $message; + foreach ($ids as $id) { + $error .= "\n - ".$id; + } + $this->error($error); + } + } +} diff --git a/app/Console/Kernel.php b/app/Console/Kernel.php index 00becdf2..285d5aec 100644 --- a/app/Console/Kernel.php +++ b/app/Console/Kernel.php @@ -25,6 +25,10 @@ class Kernel extends ConsoleKernel $schedule->command('self-host:telemetry') ->when(fn (): bool => config('scheduling.tasks.self_hosting_telemetry')) ->twiceDaily(); + + $schedule->command('self-host:database-consistency') + ->when(fn (): bool => config('scheduling.tasks.self_hosting_database_consistency')) + ->twiceDaily(); } /** diff --git a/app/Events/DatabaseSeederAfterSeed.php b/app/Events/DatabaseSeederAfterSeed.php new file mode 100644 index 00000000..8d0fbc87 --- /dev/null +++ b/app/Events/DatabaseSeederAfterSeed.php @@ -0,0 +1,14 @@ +is($ignoreUser)) { continue; } if ($user->is_placeholder) { $user->delete(); } else { + if ($user->current_team_id === $organization->getKey()) { + $user->currentOrganization()->disassociate(); + $user->save(); + } + $this->userService->makeSureUserHasAtLeastOneOrganization($user); $this->userService->makeSureUserHasCurrentOrganization($user); } diff --git a/app/Service/MemberService.php b/app/Service/MemberService.php index df435a7e..fb47fdf3 100644 --- a/app/Service/MemberService.php +++ b/app/Service/MemberService.php @@ -164,6 +164,11 @@ class MemberService public function makeMemberToPlaceholder(Member $member, bool $makeSureUserHasAtLeastOneOrganization = true): void { $user = $member->user; + if ($user->current_team_id === $member->organization_id) { + $user->currentTeam()->disassociate(); + $user->save(); + } + $placeholderUser = $user->replicate(); $placeholderUser->is_placeholder = true; $placeholderUser->save(); @@ -175,6 +180,7 @@ class MemberService $this->userService->assignOrganizationEntitiesToDifferentUser($member->organization, $user, $placeholderUser); if ($makeSureUserHasAtLeastOneOrganization) { $this->userService->makeSureUserHasAtLeastOneOrganization($user); + $this->userService->makeSureUserHasCurrentOrganization($user); } } } diff --git a/app/Service/UserService.php b/app/Service/UserService.php index 6c6f42a0..1cae3307 100644 --- a/app/Service/UserService.php +++ b/app/Service/UserService.php @@ -114,13 +114,15 @@ class UserService public function makeSureUserHasCurrentOrganization(User $user): void { - if ($user->currentOrganization !== null) { + if ($user->current_team_id !== null) { return; } $organization = $user->organizations()->first(); - $user->currentOrganization()->associate($organization); - $user->save(); + if ($organization !== null) { + $user->currentOrganization()->associate($organization); + $user->save(); + } } /** diff --git a/config/scheduling.php b/config/scheduling.php index 71637d2b..ae24cf13 100644 --- a/config/scheduling.php +++ b/config/scheduling.php @@ -8,5 +8,6 @@ return [ 'time_entry_send_still_running_mails' => (bool) env('SCHEDULING_TASK_TIME_ENTRY_SEND_STILL_RUNNING_MAILS', true), 'self_hosting_check_for_update' => (bool) env('SCHEDULING_TASK_SELF_HOSTING_CHECK_FOR_UPDATE', true), 'self_hosting_telemetry' => (bool) env('SCHEDULING_TASK_SELF_HOSTING_TELEMETRY', true), + 'self_hosting_database_consistency' => (bool) env('SCHEDULING_TASK_SELF_HOSTING_DATABASE_CONSISTENCY', false), ], ]; diff --git a/database/migrations/2025_05_16_075757_add_foreign_key_for_current_team_id_in_users_table.php b/database/migrations/2025_05_16_075757_add_foreign_key_for_current_team_id_in_users_table.php new file mode 100644 index 00000000..488754db --- /dev/null +++ b/database/migrations/2025_05_16_075757_add_foreign_key_for_current_team_id_in_users_table.php @@ -0,0 +1,43 @@ +foreign('current_team_id', 'organizations_current_organization_id_foreign') + ->references('id') + ->on('organizations') + ->onDelete('restrict') + ->onUpdate('cascade'); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('users', function (Blueprint $table): void { + $table->dropForeign('organizations_current_organization_id_foreign'); + }); + } +}; diff --git a/database/seeders/DatabaseSeeder.php b/database/seeders/DatabaseSeeder.php index a4a9fdda..30447b8d 100644 --- a/database/seeders/DatabaseSeeder.php +++ b/database/seeders/DatabaseSeeder.php @@ -5,6 +5,8 @@ declare(strict_types=1); namespace Database\Seeders; use App\Enums\Role; +use App\Events\DatabaseSeederAfterSeed; +use App\Events\DatabaseSeederBeforeDelete; use App\Models\Audit; use App\Models\Client; use App\Models\Member; @@ -184,10 +186,13 @@ class DatabaseSeeder extends Seeder 'email' => 'admin@example.com', ]); + DatabaseSeederAfterSeed::dispatch(); } private function deleteAll(): void { + DatabaseSeederBeforeDelete::dispatch(); + // Laravel Passport tables DB::table((new RefreshToken)->getTable())->delete(); DB::table((new Token)->getTable())->delete(); @@ -213,6 +218,9 @@ class DatabaseSeeder extends Seeder DB::table((new Client)->getTable())->delete(); DB::table((new Member)->getTable())->delete(); DB::table((new OrganizationInvitation)->getTable())->delete(); + DB::table((new User)->getTable())->update([ + 'current_team_id' => null, + ]); DB::table((new Organization)->getTable())->delete(); DB::table((new User)->getTable())->delete(); } diff --git a/tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php b/tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php new file mode 100644 index 00000000..f9f728a8 --- /dev/null +++ b/tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php @@ -0,0 +1,163 @@ +createUserWithRole(Role::Owner); + $project1 = Project::factory()->forOrganization($user->organization)->create(); + $project2 = Project::factory()->forOrganization($user->organization)->create(); + $task = Task::factory()->forOrganization($user->organization)->forProject($project1)->create(); + $timeEntry = TimeEntry::factory()->forMember($user->member)->forTask($task)->forProject($project2)->create(); + + // Act + $exitCode = $this->withoutMockingConsoleOutput()->artisan('self-host:database-consistency'); + + // Assert + $this->assertSame(Command::FAILURE, $exitCode); + $output = Artisan::output(); + $this->assertSame("Consistency problem: Time entries have a task that does not belong to the project of the time entry\n - ".$timeEntry->getKey()."\n", $output); + } + + public function test_checks_that_client_id_is_the_client_id_of_the_project(): void + { + // Arrange + $user = $this->createUserWithRole(Role::Owner); + $client1 = Client::factory()->forOrganization($user->organization)->create(); + $client2 = Client::factory()->forOrganization($user->organization)->create(); + $project = Project::factory()->forOrganization($user->organization)->forClient($client1)->create(); + $timeEntry = TimeEntry::factory()->forMember($user->member)->forProject($project)->create([ + 'client_id' => $client2->id, + ]); + + // Act + $exitCode = $this->withoutMockingConsoleOutput()->artisan('self-host:database-consistency'); + + // Assert + $this->assertSame(Command::FAILURE, $exitCode); + $output = Artisan::output(); + $this->assertSame("Consistency problem: Time entries have a client that does not match the client of the project\n - ".$timeEntry->getKey()."\n", $output); + } + + public function test_checks_that_client_id_is_the_client_id_of_the_project_with_no_client_in_time_entry(): void + { + // Arrange + $user = $this->createUserWithRole(Role::Owner); + $client1 = Client::factory()->forOrganization($user->organization)->create(); + $client2 = Client::factory()->forOrganization($user->organization)->create(); + $project = Project::factory()->forOrganization($user->organization)->forClient($client1)->create(); + $timeEntry = TimeEntry::factory()->forMember($user->member)->forProject($project)->create([ + 'client_id' => null, + ]); + + // Act + $exitCode = $this->withoutMockingConsoleOutput()->artisan('self-host:database-consistency'); + + // Assert + $this->assertSame(Command::FAILURE, $exitCode); + $output = Artisan::output(); + $this->assertSame("Consistency problem: Time entries have a client that does not match the client of the project\n - ".$timeEntry->getKey()."\n", $output); + } + + public function test_checks_that_client_id_is_only_null_if_project_is_also_null(): void + { + // Arrange + $user = $this->createUserWithRole(Role::Owner); + $client1 = Client::factory()->forOrganization($user->organization)->create(); + $project = Project::factory()->forOrganization($user->organization)->forClient($client1)->create(); + $timeEntry = TimeEntry::factory()->forMember($user->member)->create([ + 'client_id' => $client1->getKey(), + ]); + + // Act + $exitCode = $this->withoutMockingConsoleOutput()->artisan('self-host:database-consistency'); + + // Assert + $this->assertSame(Command::FAILURE, $exitCode); + $output = Artisan::output(); + $this->assertSame("Consistency problem: Time entries have a client but no project\n - ".$timeEntry->getKey()."\n", $output); + } + + public function test_checks_that_every_user_needs_to_be_a_member_of_at_least_one_organization(): void + { + // Arrange + $user = User::factory()->create(); + + // Act + $exitCode = $this->withoutMockingConsoleOutput()->artisan('self-host:database-consistency'); + + // Assert + $this->assertSame(Command::FAILURE, $exitCode); + $output = Artisan::output(); + $this->assertSame("Consistency problem: Users are not member of any organization\n - ".$user->getKey()."\n", $output); + } + + public function test_checks_that_every_organization_needs_at_least_an_owner(): void + { + // Arrange + $user = $this->createUserWithRole(Role::Owner); + $organization = Organization::factory()->withOwner($user->user)->create(); + + // Act + $exitCode = $this->withoutMockingConsoleOutput()->artisan('self-host:database-consistency'); + + // Assert + $this->assertSame(Command::FAILURE, $exitCode); + $output = Artisan::output(); + $this->assertSame("Consistency problem: Organizations without an owner\n - ".$organization->getKey()."\n", $output); + } + + public function test_checks_that_every_member_can_only_have_one_running_time_entry(): void + { + // Arrange + $user = $this->createUserWithRole(Role::Owner); + $timeEntry1 = TimeEntry::factory()->forMember($user->member)->active()->create(); + $timeEntry2 = TimeEntry::factory()->forMember($user->member)->active()->create(); + + // Act + $exitCode = $this->withoutMockingConsoleOutput()->artisan('self-host:database-consistency'); + + // Assert + $this->assertSame(Command::FAILURE, $exitCode); + $output = Artisan::output(); + $this->assertSame("Consistency problem: Users with more than one running time entry\n - ".$user->user->getKey()."\n", $output); + } + + public function test_checks_that_users_have_a_current_organization_that_they_are_not_a_member_of(): void + { + // Arrange + $user1 = $this->createUserWithRole(Role::Owner); + $user2 = $this->createUserWithRole(Role::Owner); + $user1->user->currentOrganization()->associate($user2->organization); + $user1->user->save(); + + // Act + $exitCode = $this->withoutMockingConsoleOutput()->artisan('self-host:database-consistency'); + + // Assert + $this->assertSame(Command::FAILURE, $exitCode); + $output = Artisan::output(); + $this->assertSame("Consistency problem: Users have a current organization that they are not a member of\n - ".$user1->user->getKey()."\n", $output); + } +} diff --git a/tests/Unit/Service/DeletionServiceTest.php b/tests/Unit/Service/DeletionServiceTest.php index 05bdf4e3..131569e1 100644 --- a/tests/Unit/Service/DeletionServiceTest.php +++ b/tests/Unit/Service/DeletionServiceTest.php @@ -150,6 +150,24 @@ class DeletionServiceTest extends TestCaseWithDatabase $this->assertSame($specialCase ? 7 : 6, TimeEntry::query()->whereBelongsTo($organization, 'organization')->count()); } + public function test_delete_organization_resets_the_current_organization_of_users_that_had_the_deleted_organization_as_current_organization(): void + { + // Arrange + $userOwner = User::factory()->create(); + $organization = Organization::factory()->withOwner($userOwner)->create(); + $userOwner->currentOrganization()->associate($organization); + $userOwner->save(); + + // Act + $this->deletionService->deleteOrganization($organization); + + // Assert + $this->assertOrganizationDeleted($organization); + $userOwner->refresh(); + $this->assertNull($userOwner->current_team_id); + $this->assertNotSame($organization->id, $userOwner->current_team_id); + } + public function test_delete_organization_deletes_all_resources_of_the_organization_but_does_not_delete_other_resources(): void { // Arrange diff --git a/tests/Unit/Service/MemberServiceTest.php b/tests/Unit/Service/MemberServiceTest.php index 15969096..2185396d 100644 --- a/tests/Unit/Service/MemberServiceTest.php +++ b/tests/Unit/Service/MemberServiceTest.php @@ -114,6 +114,41 @@ class MemberServiceTest extends TestCaseWithDatabase $this->assertSame(1, $otherUser->organizations()->count()); } + public function test_make_member_to_placeholder_resets_current_organization_of_user_if_user_is_no_longer_member_to_newly_created_organization(): void + { + // Arrange + $organization = Organization::factory()->create(); + $user = User::factory()->forCurrentOrganization($organization)->create(); + $member = Member::factory()->forOrganization($organization)->forUser($user)->role(Role::Employee)->create(); + + // Act + $this->memberService->makeMemberToPlaceholder($member); + + // Assert + $user->refresh(); + $this->assertNotNull($user->current_team_id); + $this->assertNotSame($organization->id, $user->current_team_id); + } + + public function test_make_member_to_placeholder_resets_current_organization_of_user_if_user_is_no_longer_member_to_already_existing_other_organization(): void + { + // Arrange + $organization = Organization::factory()->create(); + $user = User::factory()->forCurrentOrganization($organization)->create(); + $member = Member::factory()->forOrganization($organization)->forUser($user)->role(Role::Employee)->create(); + + $otherOrganization = Organization::factory()->create(); + $otherMember = Member::factory()->forOrganization($otherOrganization)->forUser($user)->role(Role::Employee)->create(); + + // Act + $this->memberService->makeMemberToPlaceholder($member); + + // Assert + $user->refresh(); + $this->assertNotNull($user->current_team_id); + $this->assertSame($otherOrganization->id, $user->current_team_id); + } + public function test_assign_organization_entities_to_different_member_without_any_entries(): void { // Arrange