Add checks for placeholder invitation; Fixed bug in member deletion

korridor · korridor · commit 47aa65d95937 · 2025-07-08T16:49:05.000+02:00
diff --git a/app/Console/Kernel.php b/app/Console/Kernel.php
@@ -28,7 +28,7 @@ protected function schedule(Schedule $schedule): void
 
         $schedule->command('self-host:database-consistency')
             ->when(fn (): bool => config('scheduling.tasks.self_hosting_database_consistency'))
-            ->twiceDaily();
+            ->everySixHours();
     }
 
     /**
diff --git a/app/Exceptions/Api/InvitationForTheEmailAlreadyExistsApiException.php b/app/Exceptions/Api/InvitationForTheEmailAlreadyExistsApiException.php
@@ -0,0 +1,10 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Exceptions\Api;
+
+class InvitationForTheEmailAlreadyExistsApiException extends ApiException
+{
+    public const string KEY = 'invitation_for_the_email_already_exists';
+}
diff --git a/app/Http/Controllers/Api/V1/InvitationController.php b/app/Http/Controllers/Api/V1/InvitationController.php
@@ -4,6 +4,7 @@
 
 namespace App\Http\Controllers\Api\V1;
 
+use App\Exceptions\Api\InvitationForTheEmailAlreadyExistsApiException;
 use App\Exceptions\Api\UserIsAlreadyMemberOfOrganizationApiException;
 use App\Http\Requests\V1\Invitation\InvitationIndexRequest;
 use App\Http\Requests\V1\Invitation\InvitationStoreRequest;
@@ -50,6 +51,7 @@ public function index(Organization $organization, InvitationIndexRequest $reques
      *
      * @throws AuthorizationException
      * @throws UserIsAlreadyMemberOfOrganizationApiException
+     * @throws InvitationForTheEmailAlreadyExistsApiException
      *
      * @operationId invite
      */
diff --git a/app/Http/Controllers/Api/V1/MemberController.php b/app/Http/Controllers/Api/V1/MemberController.php
@@ -10,6 +10,7 @@
 use App\Exceptions\Api\ChangingRoleOfPlaceholderIsNotAllowed;
 use App\Exceptions\Api\ChangingRoleToPlaceholderIsNotAllowed;
 use App\Exceptions\Api\EntityStillInUseApiException;
+use App\Exceptions\Api\InvitationForTheEmailAlreadyExistsApiException;
 use App\Exceptions\Api\OnlyOwnerCanChangeOwnership;
 use App\Exceptions\Api\OnlyPlaceholdersCanBeMergedIntoAnotherMember;
 use App\Exceptions\Api\OrganizationNeedsAtLeastOneOwner;
@@ -173,6 +174,7 @@ public function mergeInto(Organization $organization, Member $member, MemberMerg
      * @throws UserNotPlaceholderApiException
      * @throws UserIsAlreadyMemberOfOrganizationApiException
      * @throws ThisPlaceholderCanNotBeInvitedUseTheMergeToolInsteadException
+     * @throws InvitationForTheEmailAlreadyExistsApiException
      *
      * @operationId invitePlaceholder
      */
diff --git a/app/Http/Controllers/Controller.php b/app/Http/Controllers/Controller.php
@@ -43,7 +43,10 @@ protected function member(Organization $organization): Member
         /** @var Member|null $member */
         $member = Member::query()->whereBelongsTo($organization, 'organization')->whereBelongsTo($user, 'user')->first();
         if ($member === null) {
-            Log::error('This function should only be called in authenticated context after checking the user is a member of the organization');
+            Log::error('This function should only be called in authenticated context after checking the user is a member of the organization', [
+                'user' => $user->getKey(),
+                'organization' => $organization->getKey(),
+            ]);
             throw new AuthorizationException;
         }
 
diff --git a/app/Http/Requests/V1/Invitation/InvitationStoreRequest.php b/app/Http/Requests/V1/Invitation/InvitationStoreRequest.php
@@ -7,11 +7,8 @@
 use App\Enums\Role;
 use App\Http\Requests\V1\BaseFormRequest;
 use App\Models\Organization;
-use App\Models\OrganizationInvitation;
 use Illuminate\Contracts\Validation\ValidationRule;
-use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Validation\Rule;
-use Korridor\LaravelModelValidationRules\Rules\UniqueEloquent;
 
 /**
  * @property Organization $organization
@@ -29,10 +26,6 @@ public function rules(): array
             'email' => [
                 'required',
                 'email',
-                UniqueEloquent::make(OrganizationInvitation::class, 'email', function (Builder $builder): Builder {
-                    /** @var Builder<OrganizationInvitation> $builder */
-                    return $builder->whereBelongsTo($this->organization, 'organization');
-                })->withCustomTranslation('validation.invitation_already_exists'),
             ],
             'role' => [
                 'required',
diff --git a/app/Service/InvitationService.php b/app/Service/InvitationService.php
@@ -5,6 +5,7 @@
 namespace App\Service;
 
 use App\Enums\Role;
+use App\Exceptions\Api\InvitationForTheEmailAlreadyExistsApiException;
 use App\Exceptions\Api\UserIsAlreadyMemberOfOrganizationApiException;
 use App\Mail\OrganizationInvitationMail;
 use App\Models\Member;
@@ -16,7 +17,7 @@
 class InvitationService
 {
     /**
-     * @throws UserIsAlreadyMemberOfOrganizationApiException
+     * @throws UserIsAlreadyMemberOfOrganizationApiException|InvitationForTheEmailAlreadyExistsApiException
      */
     public function inviteUser(Organization $organization, string $email, Role $role): OrganizationInvitation
     {
@@ -28,6 +29,13 @@ public function inviteUser(Organization $organization, string $email, Role $role
             throw new UserIsAlreadyMemberOfOrganizationApiException;
         }
 
+        if (OrganizationInvitation::query()
+            ->where('email', $email)
+            ->whereBelongsTo($organization, 'organization')
+            ->exists()) {
+            throw new InvitationForTheEmailAlreadyExistsApiException;
+        }
+
         InvitingTeamMember::dispatch($organization, $email, $role->value);
 
         $invitation = new OrganizationInvitation;
diff --git a/app/Service/MemberService.php b/app/Service/MemberService.php
@@ -67,6 +67,14 @@ public function removeMember(Member $member, Organization $organization, bool $w
             throw new CanNotRemoveOwnerFromOrganization;
         }
 
+        $user = $member->user;
+        $isPlaceholder = $user->is_placeholder;
+
+        if (! $isPlaceholder && $user->current_team_id === $member->organization_id) {
+            $user->currentTeam()->disassociate();
+            $user->save();
+        }
+
         if ($withRelations) {
             TimeEntry::query()->where('user_id', $member->user_id)->whereBelongsTo($organization, 'organization')->delete();
             ProjectMember::query()->whereBelongsToOrganization($organization)->where('user_id', $member->user_id)->delete();
@@ -80,6 +88,14 @@ public function removeMember(Member $member, Organization $organization, bool $w
         }
 
         $member->delete();
+
+        if ($isPlaceholder) {
+            $user->delete();
+        } else {
+            $this->userService->makeSureUserHasAtLeastOneOrganization($user);
+            $this->userService->makeSureUserHasCurrentOrganization($user);
+        }
+
         MemberRemoved::dispatch($member, $organization);
     }
 
diff --git a/lang/en/exceptions.php b/lang/en/exceptions.php
@@ -9,6 +9,7 @@
 use App\Exceptions\Api\EntityStillInUseApiException;
 use App\Exceptions\Api\FeatureIsNotAvailableInFreePlanApiException;
 use App\Exceptions\Api\InactiveUserCanNotBeUsedApiException;
+use App\Exceptions\Api\InvitationForTheEmailAlreadyExistsApiException;
 use App\Exceptions\Api\OnlyOwnerCanChangeOwnership;
 use App\Exceptions\Api\OnlyPlaceholdersCanBeMergedIntoAnotherMember;
 use App\Exceptions\Api\OrganizationHasNoSubscriptionButMultipleMembersException;
@@ -45,6 +46,7 @@
         ChangingRoleOfPlaceholderIsNotAllowed::KEY => 'Changing role of placeholder is not allowed',
         OnlyPlaceholdersCanBeMergedIntoAnotherMember::KEY => 'Only placeholders can be merged into another member',
         ThisPlaceholderCanNotBeInvitedUseTheMergeToolInsteadException::KEY => 'This placeholder can not be invited use the merge tool instead',
+        InvitationForTheEmailAlreadyExistsApiException::KEY => 'The email has already been invited to the organization. Please wait for the user to accept the invitation or resend the invitation email.',
     ],
     'unknown_error_in_admin_panel' => 'An unknown error occurred. Please check the logs.',
 ];
diff --git a/tests/Unit/Endpoint/Api/V1/InvitationEndpointTest.php b/tests/Unit/Endpoint/Api/V1/InvitationEndpointTest.php
@@ -129,26 +129,31 @@ public function test_store_fails_if_user_invites_user_who_is_already_member_of_o
         $response->assertJsonPath('message', 'User is already a member of the organization');
     }
 
-    public function test_store_fails_if_user_invites_user_who_is_already_invited_to_organization(): void
+    public function test_store_fails_if_an_invitation_with_the_same_email_already_exists(): void
     {
         // Arrange
         $data = $this->createUserWithPermission([
             'invitations:create',
         ]);
         Passport::actingAs($data->user);
-        $invitation = OrganizationInvitation::factory()->forOrganization($data->organization)->create();
+        $email = 'user@email.test';
+        $invitation = OrganizationInvitation::factory()->forOrganization($data->organization)->create([
+            'email' => $email,
+        ]);
 
         // Act
         $response = $this->postJson(route('api.v1.invitations.store', $data->organization->getKey()), [
-            'email' => $invitation->email,
+            'email' => $email,
             'role' => Role::Employee->value,
         ]);
 
         // Assert
-        $response->assertInvalid([
-            'email' => 'The email has already been invited to the organization. Please wait for the user to accept the invitation or resend the invitation email.',
+        $response->assertStatus(400);
+        $response->assertExactJson([
+            'error' => true,
+            'key' => 'invitation_for_the_email_already_exists',
+            'message' => 'The email has already been invited to the organization. Please wait for the user to accept the invitation or resend the invitation email.',
         ]);
-        $response->assertStatus(422);
     }
 
     public function test_store_works_if_user_invites_user_who_is_also_a_placeholder(): void
diff --git a/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php b/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php
@@ -10,6 +10,7 @@
 use App\Http\Controllers\Api\V1\MemberController;
 use App\Models\Member;
 use App\Models\Organization;
+use App\Models\OrganizationInvitation;
 use App\Models\Project;
 use App\Models\ProjectMember;
 use App\Models\TimeEntry;
@@ -653,6 +654,103 @@ public function test_destroy_endpoint_fails_if_member_is_still_in_use_by_a_proje
         Event::assertNotDispatched(MemberRemoved::class);
     }
 
+    public function test_destroy_endpoint_also_deletes_user_if_member_is_placeholder(): void
+    {
+        // Arrange
+        $data = $this->createUserWithPermission([
+            'members:delete',
+        ]);
+        $user = User::factory()->placeholder()->create();
+        $member = Member::factory()->forUser($user)->forOrganization($data->organization)->role(Role::Placeholder)->create();
+        Passport::actingAs($data->user);
+        Event::fake([
+            MemberRemoved::class,
+        ]);
+
+        // Act
+        $response = $this->deleteJson(route('api.v1.members.destroy', [$data->organization->getKey(), $member->getKey()]));
+
+        // Assert
+        $response->assertStatus(204);
+        $this->assertDatabaseMissing(Member::class, [
+            'id' => $member->getKey(),
+        ]);
+        $this->assertDatabaseMissing(User::class, [
+            'id' => $user->getKey(),
+        ]);
+        Event::assertDispatched(function (MemberRemoved $event) use ($data, $member): bool {
+            return $event->organization->is($data->organization) &&
+                $event->member->is($member);
+        }, 1);
+    }
+
+    public function test_destroy_endpoint_sets_current_organization_to_organization_the_user_is_still_member_of(): void
+    {
+        // Arrange
+        $data = $this->createUserWithPermission([
+            'members:delete',
+        ]);
+        $user = $data->user;
+        $otherOrganization = Organization::factory()->create();
+        $otherMember = Member::factory()->forOrganization($otherOrganization)->forUser($user)->role(Role::Employee)->create();
+        Passport::actingAs($user);
+        Event::fake([
+            MemberRemoved::class,
+        ]);
+
+        // Act
+        $response = $this->deleteJson(route('api.v1.members.destroy', [$data->organization->getKey(), $data->member->getKey()]));
+
+        // Assert
+        $response->assertStatus(204);
+        $this->assertDatabaseMissing(Member::class, [
+            'id' => $data->member->getKey(),
+        ]);
+        $user->refresh();
+        $this->assertSame($otherOrganization->getKey(), $user->currentOrganization->getKey());
+        Event::assertDispatched(function (MemberRemoved $event) use ($data): bool {
+            return $event->organization->is($data->organization) &&
+                $event->member->is($data->member);
+        }, 1);
+    }
+
+    public function test_destroy_endpoint_creates_new_organization_and_sets_the_current_organization_to_it_if_user_is_not_member_of_any_other_organization(): void
+    {
+        // Arrange
+        $data = $this->createUserWithPermission([
+            'members:delete',
+        ]);
+        $organization = $data->organization;
+        $user = $data->user;
+        Passport::actingAs($user);
+        Event::fake([
+            MemberRemoved::class,
+        ]);
+        $this->assertDatabaseCount(Organization::class, 1);
+
+        // Act
+        $response = $this->deleteJson(route('api.v1.members.destroy', [$data->organization->getKey(), $data->member->getKey()]));
+
+        // Assert
+        $response->assertStatus(204);
+        $this->assertDatabaseCount(Organization::class, 2);
+        $newOrganization = Organization::where('id', '!=', $organization->getKey())->first();
+        $this->assertNotNull($newOrganization);
+        $this->assertDatabaseMissing(Member::class, [
+            'id' => $data->member->getKey(),
+        ]);
+        $this->assertDatabaseHas(Member::class, [
+            'organization_id' => $newOrganization->getKey(),
+            'user_id' => $user->getKey(),
+        ]);
+        $user->refresh();
+        $this->assertNotNull($user->currentOrganization);
+        Event::assertDispatched(function (MemberRemoved $event) use ($data): bool {
+            return $event->organization->is($data->organization) &&
+                $event->member->is($data->member);
+        }, 1);
+    }
+
     public function test_destroy_endpoint_succeeds_if_member_is_still_in_use_by_a_project_member_and_delete_related_is_active(): void
     {
         // Arrange
@@ -937,6 +1035,37 @@ public function test_invite_placeholder_fails_if_user_is_not_part_of_organizatio
         $response->assertForbidden();
     }
 
+    public function test_invite_placeholder_fails_if_there_is_already_an_invitation_with_the_same_email(): void
+    {
+        // Arrange
+        $data = $this->createUserWithPermission([
+            'members:invite-placeholder',
+            'invitations:create',
+        ]);
+        $placeholder = User::factory()->placeholder()->create([
+            'email' => 'user@mail.test',
+        ]);
+        $placeholderMember = Member::factory()->forUser($placeholder)->forOrganization($data->organization)->role(Role::Placeholder)->create();
+        OrganizationInvitation::factory()->forOrganization($data->organization)->create([
+            'email' => $placeholder->email,
+        ]);
+        Passport::actingAs($data->user);
+
+        // Act
+        $response = $this->postJson(route('api.v1.members.invite-placeholder', [
+            'organization' => $data->organization->id,
+            'member' => $placeholderMember->id,
+        ]));
+
+        // Assert
+        $response->assertStatus(400);
+        $response->assertExactJson([
+            'error' => true,
+            'key' => 'invitation_for_the_email_already_exists',
+            'message' => 'The email has already been invited to the organization. Please wait for the user to accept the invitation or resend the invitation email.',
+        ]);
+    }
+
     public function test_invite_placeholder_returns_400_if_user_is_not_placeholder(): void
     {
         // Arrange

Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ protected function schedule(Schedule $schedule): void`
`28`	`28`
`29`	`29`	`$schedule->command('self-host:database-consistency')`
`30`	`30`	`->when(fn (): bool => config('scheduling.tasks.self_hosting_database_consistency'))`
`31`		`- ->twiceDaily();`
	`31`	`+ ->everySixHours();`
`32`	`32`	`}`
`33`	`33`
`34`	`34`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -4,6 +4,7 @@`
`4`	`4`
`5`	`5`	`namespace App\Http\Controllers\Api\V1;`
`6`	`6`
	`7`	`+use App\Exceptions\Api\InvitationForTheEmailAlreadyExistsApiException;`
`7`	`8`	`use App\Exceptions\Api\UserIsAlreadyMemberOfOrganizationApiException;`
`8`	`9`	`use App\Http\Requests\V1\Invitation\InvitationIndexRequest;`
`9`	`10`	`use App\Http\Requests\V1\Invitation\InvitationStoreRequest;`
`@@ -50,6 +51,7 @@ public function index(Organization $organization, InvitationIndexRequest $reques`
`50`	`51`	`*`
`51`	`52`	`* @throws AuthorizationException`
`52`	`53`	`* @throws UserIsAlreadyMemberOfOrganizationApiException`
	`54`	`+ * @throws InvitationForTheEmailAlreadyExistsApiException`
`53`	`55`	`*`
`54`	`56`	`* @operationId invite`
`55`	`57`	`*/`