diff --git a/app/Http/Controllers/Api/V1/MemberController.php b/app/Http/Controllers/Api/V1/MemberController.php index 15fe7283..f6da394f 100644 --- a/app/Http/Controllers/Api/V1/MemberController.php +++ b/app/Http/Controllers/Api/V1/MemberController.php @@ -26,7 +26,6 @@ use App\Models\Organization; 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 @@ class MemberController extends Controller * * @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 @@ class MemberController extends Controller } $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 index f3a31c35..4b932db3 100644 --- a/app/Listeners/RemovePlaceholder.php +++ b/app/Listeners/RemovePlaceholder.php @@ -6,7 +6,7 @@ namespace App\Listeners; 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 $query */ @@ -32,7 +35,7 @@ class RemovePlaceholder 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 index c5645595..df435a7e 100644 --- a/app/Service/MemberService.php +++ b/app/Service/MemberService.php @@ -14,9 +14,11 @@ use App\Exceptions\Api\OnlyOwnerCanChangeOwnership; 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 @@ class MemberService } } + 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 $builder */ + $builder->whereHas('members', function (Builder $builder) use ($toMember): void { + /** @var Builder $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 @@ class MemberService $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 index 92d6e7d2..e0871e77 100644 --- a/app/Service/UserService.php +++ b/app/Service/UserService.php @@ -49,24 +49,10 @@ class UserService } /** - * 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() @@ -74,7 +60,6 @@ class UserService ->whereBelongsTo($fromUser, 'user') ->update([ 'user_id' => $toUser->getKey(), - 'member_id' => $toMember->getKey(), ]); // Project members @@ -83,7 +68,6 @@ class UserService ->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 index 6dfb3982..011702af 100644 --- a/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php @@ -350,6 +350,58 @@ class MemberEndpointTest extends ApiEndpointTestAbstract $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 index 202759a3..15969096 100644 --- a/tests/Unit/Service/MemberServiceTest.php +++ b/tests/Unit/Service/MemberServiceTest.php @@ -113,4 +113,94 @@ class MemberServiceTest extends TestCaseWithDatabase $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 index 8871f3c0..a3b24a94 100644 --- a/tests/Unit/Service/UserServiceTest.php +++ b/tests/Unit/Service/UserServiceTest.php @@ -59,22 +59,6 @@ class UserServiceTest extends TestCase $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