Fixed problem with merge into when project members already exist in destination member

korridor · korridor · commit 73ce5f793d84 · 2025-03-10T15:42:43.000+01:00
diff --git a/app/Http/Controllers/Api/V1/MemberController.php b/app/Http/Controllers/Api/V1/MemberController.php
@@ -26,7 +26,6 @@
 use App\Service\BillableRateService;
 use App\Service\InvitationService;
 use App\Service\MemberService;
-use App\Service\UserService;
 use Illuminate\Auth\Access\AuthorizationException;
 use Illuminate\Http\JsonResponse;
 use Illuminate\Http\Resources\Json\JsonResource;
@@ -141,7 +140,7 @@ public function makePlaceholder(Organization $organization, Member $member, Memb
      *
      * @operationId mergeMember
      */
-    public function mergeInto(Organization $organization, Member $member, MemberMergeIntoRequest $request): JsonResponse
+    public function mergeInto(Organization $organization, Member $member, MemberMergeIntoRequest $request, MemberService $memberService): JsonResponse
     {
         $this->checkPermission($organization, 'members:merge-into', $member);
 
@@ -151,8 +150,8 @@ public function mergeInto(Organization $organization, Member $member, MemberMerg
         }
         $memberTo = Member::findOrFail($request->getMemberId());
 
-        DB::transaction(function () use ($organization, $member, $user, $memberTo): void {
-            app(UserService::class)->assignOrganizationEntitiesToDifferentMember($organization, $user, $memberTo->user, $memberTo);
+        DB::transaction(function () use ($organization, $member, $user, $memberTo, $memberService): void {
+            $memberService->assignOrganizationEntitiesToDifferentMember($organization, $member, $memberTo);
             $member->delete();
             $user->delete();
         });
diff --git a/app/Listeners/RemovePlaceholder.php b/app/Listeners/RemovePlaceholder.php
@@ -6,7 +6,7 @@
 
 use App\Models\Member;
 use App\Models\User;
-use App\Service\UserService;
+use App\Service\MemberService;
 use Illuminate\Database\Eloquent\Builder;
 use Laravel\Jetstream\Events\TeamMemberAdded;
 
@@ -17,8 +17,11 @@ class RemovePlaceholder
      */
     public function handle(TeamMemberAdded $event): void
     {
-        /** @var UserService $userService */
-        $userService = app(UserService::class);
+        $memberService = app(MemberService::class);
+        $member = Member::query()
+            ->whereBelongsTo($event->team, 'organization')
+            ->whereBelongsTo($event->user, 'user')
+            ->firstOrFail();
         $placeholders = Member::query()
             ->whereHas('user', function (Builder $query) use ($event): void {
                 /** @var Builder<User> $query */
@@ -32,7 +35,7 @@ public function handle(TeamMemberAdded $event): void
         foreach ($placeholders as $placeholder) {
             /** @var Member $placeholder */
             $placeholderUser = $placeholder->user;
-            $userService->assignOrganizationEntitiesToDifferentUser($event->team, $placeholderUser, $event->user);
+            $memberService->assignOrganizationEntitiesToDifferentMember($event->team, $placeholder, $member);
             $placeholder->delete();
             $placeholderUser->delete();
         }
diff --git a/app/Service/MemberService.php b/app/Service/MemberService.php
@@ -14,9 +14,11 @@
 use App\Exceptions\Api\OrganizationNeedsAtLeastOneOwner;
 use App\Models\Member;
 use App\Models\Organization;
+use App\Models\Project;
 use App\Models\ProjectMember;
 use App\Models\TimeEntry;
 use App\Models\User;
+use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Support\Facades\DB;
 use InvalidArgumentException;
 use Laravel\Jetstream\Events\AddingTeamMember;
@@ -101,6 +103,39 @@ public function changeRole(Member $member, Organization $organization, Role $new
         }
     }
 
+    public function assignOrganizationEntitiesToDifferentMember(Organization $organization, Member $fromMember, Member $toMember): void
+    {
+        // Time entries
+        TimeEntry::query()
+            ->whereBelongsTo($organization, 'organization')
+            ->whereBelongsTo($fromMember, 'member')
+            ->update([
+                'user_id' => $toMember->user_id,
+                'member_id' => $toMember->getKey(),
+            ]);
+
+        // Project members
+        ProjectMember::query()
+            ->whereBelongsToOrganization($organization)
+            ->whereBelongsTo($fromMember, 'member')
+            ->whereDoesntHave('project', function (Builder $builder) use ($toMember): void {
+                /** @var Builder<Project> $builder */
+                $builder->whereHas('members', function (Builder $builder) use ($toMember): void {
+                    /** @var Builder<ProjectMember> $builder */
+                    $builder->where('member_id', $toMember->getKey());
+                });
+            })
+            ->update([
+                'user_id' => $toMember->user_id,
+                'member_id' => $toMember->getKey(),
+            ]);
+
+        ProjectMember::query()
+            ->whereBelongsToOrganization($organization)
+            ->whereBelongsTo($fromMember, 'member')
+            ->delete();
+    }
+
     /**
      * Change the ownership of an organization to a new user.
      * The previous owner will be demoted to an admin.
@@ -137,7 +172,7 @@ public function makeMemberToPlaceholder(Member $member, bool $makeSureUserHasAtL
         $member->role = Role::Placeholder->value;
         $member->save();
 
-        $this->userService->assignOrganizationEntitiesToDifferentMember($member->organization, $user, $placeholderUser, $member);
+        $this->userService->assignOrganizationEntitiesToDifferentUser($member->organization, $user, $placeholderUser);
         if ($makeSureUserHasAtLeastOneOrganization) {
             $this->userService->makeSureUserHasAtLeastOneOrganization($user);
         }
diff --git a/app/Service/UserService.php b/app/Service/UserService.php
@@ -49,32 +49,17 @@ public function createUser(string $name, string $email, string $password, string
     }
 
     /**
-     * Assign all organization entities (time entries, project members) from one user to another.
-     * This is useful when a placeholder user is replaced with a real user.
+     * This does NOT change the member id.
+     * This should only be used in if you want to change a member to a placeholder!
      */
     public function assignOrganizationEntitiesToDifferentUser(Organization $organization, User $fromUser, User $toUser): void
-    {
-        /** @var Member|null $toMember */
-        $toMember = Member::query()
-            ->whereBelongsTo($organization, 'organization')
-            ->whereBelongsTo($toUser, 'user')
-            ->first();
-        if ($toMember === null) {
-            throw new \InvalidArgumentException('User is not a member of the organization');
-        }
-
-        $this->assignOrganizationEntitiesToDifferentMember($organization, $fromUser, $toUser, $toMember);
-    }
-
-    public function assignOrganizationEntitiesToDifferentMember(Organization $organization, User $fromUser, User $toUser, Member $toMember): void
     {
         // Time entries
         TimeEntry::query()
             ->whereBelongsTo($organization, 'organization')
             ->whereBelongsTo($fromUser, 'user')
             ->update([
                 'user_id' => $toUser->getKey(),
-                'member_id' => $toMember->getKey(),
             ]);
 
         // Project members
@@ -83,7 +68,6 @@ public function assignOrganizationEntitiesToDifferentMember(Organization $organi
             ->whereBelongsTo($fromUser, 'user')
             ->update([
                 'user_id' => $toUser->getKey(),
-                'member_id' => $toMember->getKey(),
             ]);
     }
 
diff --git a/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php b/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php
@@ -350,6 +350,58 @@ public function test_merge_into_assigns_resources_of_source_member_to_destinatio
         $memberDestination->refresh();
         $this->assertCount(3, $memberDestination->timeEntries);
         $this->assertCount(1, $memberDestination->projectMembers);
+        $this->assertDatabaseHas(ProjectMember::class, [
+            'project_id' => $project->getKey(),
+            'member_id' => $memberDestination->getKey(),
+            'user_id' => $userDestination->getKey(),
+        ]);
+    }
+
+    public function test_merge_into_assigns_resources_of_source_member_to_destination_member_and_deletes_member_with_existing_destination_resources(): void
+    {
+        // Arrange
+        $data = $this->createUserWithPermission([
+            'members:merge-into',
+        ]);
+        $userSource = User::factory()->placeholder()->create();
+        $memberSource = Member::factory()->forUser($userSource)->forOrganization($data->organization)->role(Role::Placeholder)->create();
+        TimeEntry::factory()->forMember($memberSource)->createMany(3);
+        $project = Project::factory()->forOrganization($data->organization)->create();
+        ProjectMember::factory()->forMember($memberSource)->forProject($project)->create([
+            'billable_rate' => 32100,
+        ]);
+
+        $userDestination = User::factory()->create();
+        $memberDestination = Member::factory()->forUser($userDestination)->forOrganization($data->organization)->role(Role::Admin)->create();
+        ProjectMember::factory()->forMember($memberDestination)->forProject($project)->create([
+            'billable_rate' => 12300,
+        ]);
+        TimeEntry::factory()->forMember($memberDestination)->createMany(3);
+        Passport::actingAs($data->user);
+
+        // Act
+        $response = $this->withoutExceptionHandling()->postJson(route('api.v1.members.merge-into', [$data->organization->getKey(), $memberSource->getKey()]), [
+            'member_id' => $memberDestination->getKey(),
+        ]);
+
+        // Assert
+        $response->assertStatus(204);
+        $this->assertSame('', $response->getContent());
+        $this->assertDatabaseMissing(Member::class, [
+            'id' => $memberSource->getKey(),
+        ]);
+        $this->assertDatabaseMissing(User::class, [
+            'id' => $userSource->getKey(),
+        ]);
+        $memberDestination->refresh();
+        $this->assertCount(6, $memberDestination->timeEntries);
+        $this->assertCount(1, $memberDestination->projectMembers);
+        $this->assertDatabaseHas(ProjectMember::class, [
+            'project_id' => $project->getKey(),
+            'billable_rate' => 12300,
+            'member_id' => $memberDestination->getKey(),
+            'user_id' => $userDestination->getKey(),
+        ]);
     }
 
     public function test_update_member_fails_if_user_tries_to_change_role_of_the_current_owner(): void
diff --git a/tests/Unit/Service/MemberServiceTest.php b/tests/Unit/Service/MemberServiceTest.php
@@ -113,4 +113,94 @@ public function test_make_member_to_placeholder_creates_new_user_based_on_member
         $this->assertSame($otherMember->getKey(), $otherTimeEntry->member_id);
         $this->assertSame(1, $otherUser->organizations()->count());
     }
+
+    public function test_assign_organization_entities_to_different_member_without_any_entries(): void
+    {
+        // Arrange
+        $organization = Organization::factory()->create();
+        $project = Project::factory()->forOrganization($organization)->create();
+        $otherUser = User::factory()->create();
+        $fromUser = User::factory()->create();
+        $toUser = User::factory()->create();
+        $otherUserMember = Member::factory()->forOrganization($organization)->forUser($otherUser)->create();
+        $fromUserMember = Member::factory()->forOrganization($organization)->forUser($fromUser)->create();
+        $toUserMember = Member::factory()->forOrganization($organization)->forUser($toUser)->create();
+        TimeEntry::factory()->forOrganization($organization)->forMember($otherUserMember)->createMany(3);
+        TimeEntry::factory()->forOrganization($organization)->forMember($fromUserMember)->createMany(3);
+        ProjectMember::factory()->forProject($project)->forMember($otherUserMember)->create();
+        ProjectMember::factory()->forProject($project)->forMember($fromUserMember)->create();
+
+        // Act
+        $this->memberService->assignOrganizationEntitiesToDifferentMember($organization, $fromUserMember, $toUserMember);
+
+        // Assert
+        $this->assertSame(3, TimeEntry::query()->whereBelongsTo($toUser, 'user')->count());
+        $this->assertSame(3, TimeEntry::query()->whereBelongsTo($otherUser, 'user')->count());
+        $this->assertSame(0, TimeEntry::query()->whereBelongsTo($fromUser, 'user')->count());
+        $this->assertSame(1, ProjectMember::query()->whereBelongsTo($toUser, 'user')->count());
+        $this->assertSame(1, ProjectMember::query()->whereBelongsTo($otherUser, 'user')->count());
+        $this->assertSame(0, ProjectMember::query()->whereBelongsTo($fromUser, 'user')->count());
+
+        $this->assertSame(3, TimeEntry::query()->whereBelongsTo($toUserMember, 'member')->count());
+        $this->assertSame(3, TimeEntry::query()->whereBelongsTo($otherUserMember, 'member')->count());
+        $this->assertSame(0, TimeEntry::query()->whereBelongsTo($fromUserMember, 'member')->count());
+        $this->assertSame(1, ProjectMember::query()->whereBelongsTo($toUserMember, 'member')->count());
+        $this->assertSame(1, ProjectMember::query()->whereBelongsTo($otherUserMember, 'member')->count());
+        $this->assertSame(0, ProjectMember::query()->whereBelongsTo($fromUserMember, 'member')->count());
+    }
+
+    public function test_assign_organization_entities_to_different_member_with_entries(): void
+    {
+        // Arrange
+        $organization = Organization::factory()->create();
+        $project = Project::factory()->forOrganization($organization)->create();
+        $otherUser = User::factory()->create();
+        $fromUser = User::factory()->create();
+        $toUser = User::factory()->create();
+        $otherUserMember = Member::factory()->forOrganization($organization)->forUser($otherUser)->create();
+        $fromUserMember = Member::factory()->forOrganization($organization)->forUser($fromUser)->create();
+        $toUserMember = Member::factory()->forOrganization($organization)->forUser($toUser)->create();
+        TimeEntry::factory()->forOrganization($organization)->forMember($otherUserMember)->createMany(3);
+        TimeEntry::factory()->forOrganization($organization)->forMember($fromUserMember)->createMany(3);
+        TimeEntry::factory()->forOrganization($organization)->forMember($toUserMember)->createMany(3);
+        ProjectMember::factory()->forProject($project)->forMember($otherUserMember)->create([
+            'billable_rate' => 1,
+        ]);
+        ProjectMember::factory()->forProject($project)->forMember($fromUserMember)->create([
+            'billable_rate' => 2,
+        ]);
+        ProjectMember::factory()->forProject($project)->forMember($toUserMember)->create([
+            'billable_rate' => 3,
+        ]);
+
+        // Act
+        $this->memberService->assignOrganizationEntitiesToDifferentMember($organization, $fromUserMember, $toUserMember);
+
+        // Assert
+        $this->assertSame(6, TimeEntry::query()->whereBelongsTo($toUser, 'user')->count());
+        $this->assertSame(3, TimeEntry::query()->whereBelongsTo($otherUser, 'user')->count());
+        $this->assertSame(0, TimeEntry::query()->whereBelongsTo($fromUser, 'user')->count());
+        $this->assertSame(1, ProjectMember::query()->whereBelongsTo($toUser, 'user')->count());
+        $this->assertSame(1, ProjectMember::query()->whereBelongsTo($otherUser, 'user')->count());
+        $this->assertSame(0, ProjectMember::query()->whereBelongsTo($fromUser, 'user')->count());
+
+        $this->assertSame(6, TimeEntry::query()->whereBelongsTo($toUserMember, 'member')->count());
+        $this->assertSame(3, TimeEntry::query()->whereBelongsTo($otherUserMember, 'member')->count());
+        $this->assertSame(0, TimeEntry::query()->whereBelongsTo($fromUserMember, 'member')->count());
+        $this->assertSame(1, ProjectMember::query()->whereBelongsTo($toUserMember, 'member')->count());
+        $this->assertSame(1, ProjectMember::query()->whereBelongsTo($otherUserMember, 'member')->count());
+        $this->assertSame(0, ProjectMember::query()->whereBelongsTo($fromUserMember, 'member')->count());
+
+        $this->assertDatabaseCount(ProjectMember::class, 2);
+        $this->assertDatabaseHas(ProjectMember::class, [
+            'project_id' => $project->id,
+            'member_id' => $toUserMember->id,
+            'billable_rate' => 3,
+        ]);
+        $this->assertDatabaseHas(ProjectMember::class, [
+            'project_id' => $project->id,
+            'member_id' => $otherUserMember->id,
+            'billable_rate' => 1,
+        ]);
+    }
 }
diff --git a/tests/Unit/Service/UserServiceTest.php b/tests/Unit/Service/UserServiceTest.php
@@ -59,22 +59,6 @@ public function test_assign_organization_entities_to_different_user(): void
         $this->assertSame(0, ProjectMember::query()->whereBelongsTo($fromUser, 'user')->count());
     }
 
-    public function test_assign_organization_entities_to_different_user_fails_if_new_user_is_not_member_of_organization(): void
-    {
-        // Arrange
-        $organization = Organization::factory()->create();
-        $fromUser = User::factory()->create();
-        $toUser = User::factory()->create();
-        $fromUserMember = Member::factory()->forOrganization($organization)->forUser($fromUser)->create();
-
-        // Act
-        try {
-            $this->userService->assignOrganizationEntitiesToDifferentUser($organization, $fromUser, $toUser);
-        } catch (\InvalidArgumentException $e) {
-            $this->assertSame('User is not a member of the organization', $e->getMessage());
-        }
-    }
-
     public function test_change_ownership_changes_ownership_of_organization_to_new_user(): void
     {
         // Arrange
