diff --git a/app/Console/Commands/Correction/CorrectionPlaceholderMembersCommand.php b/app/Console/Commands/Correction/CorrectionPlaceholderMembersCommand.php new file mode 100644 index 00000000..2f666fd4 --- /dev/null +++ b/app/Console/Commands/Correction/CorrectionPlaceholderMembersCommand.php @@ -0,0 +1,59 @@ +comment('Sets all members who belong to a placeholder user to role placeholder...'); + $dryRun = (bool) $this->option('dry-run'); + if ($dryRun) { + $this->comment('Running in dry-run mode. Nothing will be saved to the database.'); + } + + $members = Member::query() + ->where('role', '!=', Role::Placeholder->value) + ->whereHas('user', function (Builder $builder): void { + /** @var Builder $builder */ + $builder->where('is_placeholder', '=', true); + }) + ->get(); + foreach ($members as $member) { + /** @var Member $member */ + $member->role = Role::Placeholder->value; + if (! $dryRun) { + $member->save(); + } + $this->line('Set role of member (id='.$member->getKey().') to placeholder'); + } + + return self::SUCCESS; + } +} diff --git a/app/Exceptions/Api/ChangingRoleOfPlaceholderIsNotAllowed.php b/app/Exceptions/Api/ChangingRoleOfPlaceholderIsNotAllowed.php new file mode 100644 index 00000000..4f18c5a9 --- /dev/null +++ b/app/Exceptions/Api/ChangingRoleOfPlaceholderIsNotAllowed.php @@ -0,0 +1,10 @@ +role === Role::Owner->value) { throw new CanNotRemoveOwnerFromOrganization; } + if ($member->role === Role::Placeholder->value) { + throw new ChangingRoleOfPlaceholderIsNotAllowed; + } $memberService->makeMemberToPlaceholder($member); @@ -122,10 +134,37 @@ class MemberController extends Controller return response()->json(null, 204); } + /** + * @throws AuthorizationException + * @throws OnlyPlaceholdersCanBeMergedIntoAnotherMember + * @throws \Throwable + */ + public function mergeInto(Organization $organization, Member $member, MemberMergeIntoRequest $request): JsonResponse + { + $this->checkPermission($organization, 'members:merge-into', $member); + + $user = $member->user; + if ($member->role !== Role::Placeholder->value || ! $user->is_placeholder) { + throw new OnlyPlaceholdersCanBeMergedIntoAnotherMember; + } + $memberTo = Member::findOrFail($request->getMemberId()); + + DB::transaction(function () use ($organization, $member, $user, $memberTo): void { + app(UserService::class)->assignOrganizationEntitiesToDifferentMember($organization, $user, $memberTo->user, $memberTo); + $member->delete(); + $user->delete(); + }); + + return response()->json(null, 204); + } + /** * Invite a placeholder member to become a real member of the organization * - * @throws AuthorizationException|UserNotPlaceholderApiException + * @throws AuthorizationException + * @throws UserNotPlaceholderApiException + * @throws UserIsAlreadyMemberOfOrganizationApiException + * @throws ThisPlaceholderCanNotBeInvitedUseTheMergeToolInsteadException * * @operationId invitePlaceholder */ @@ -138,6 +177,10 @@ class MemberController extends Controller throw new UserNotPlaceholderApiException; } + if (Str::endsWith($user->email, '@solidtime-import.test')) { + throw new ThisPlaceholderCanNotBeInvitedUseTheMergeToolInsteadException; + } + $invitationService->inviteUser($organization, $user->email, Role::Employee); return response()->json(null, 204); diff --git a/app/Http/Requests/V1/Member/MemberMergeIntoRequest.php b/app/Http/Requests/V1/Member/MemberMergeIntoRequest.php new file mode 100644 index 00000000..b341811a --- /dev/null +++ b/app/Http/Requests/V1/Member/MemberMergeIntoRequest.php @@ -0,0 +1,42 @@ +> + */ + public function rules(): array + { + return [ + // ID of the member to which the data should be transferred (destination) + 'member_id' => [ + 'string', + ExistsEloquent::make(Member::class, null, function (Builder $builder): Builder { + /** @var Builder $builder */ + return $builder->whereBelongsTo($this->organization, 'organization'); + })->uuid(), + ], + ]; + } + + public function getMemberId(): string + { + return (string) $this->input('member_id'); + } +} diff --git a/app/Models/Member.php b/app/Models/Member.php index 2c9fba3c..694ec26e 100644 --- a/app/Models/Member.php +++ b/app/Models/Member.php @@ -7,6 +7,7 @@ namespace App\Models; use App\Models\Concerns\CustomAuditable; use App\Models\Concerns\HasUuids; use Database\Factories\MemberFactory; +use Illuminate\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Relations\HasMany; @@ -24,6 +25,8 @@ use OwenIt\Auditing\Contracts\Auditable as AuditableContract; * @property Carbon|null $updated_at * @property-read Organization $organization * @property-read User $user + * @property-read Collection $projectMembers + * @property-read Collection $timeEntries * * @method static MemberFactory factory() */ @@ -59,6 +62,14 @@ class Member extends JetstreamMembership implements AuditableContract return $this->belongsTo(Organization::class, 'organization_id'); } + /** + * @return HasMany + */ + public function timeEntries(): HasMany + { + return $this->hasMany(TimeEntry::class, 'member_id'); + } + /** * @return HasMany */ diff --git a/app/Service/MemberService.php b/app/Service/MemberService.php index b5c1f6e0..c5645595 100644 --- a/app/Service/MemberService.php +++ b/app/Service/MemberService.php @@ -7,6 +7,7 @@ namespace App\Service; use App\Enums\Role; use App\Events\MemberRemoved; use App\Exceptions\Api\CanNotRemoveOwnerFromOrganization; +use App\Exceptions\Api\ChangingRoleOfPlaceholderIsNotAllowed; use App\Exceptions\Api\ChangingRoleToPlaceholderIsNotAllowed; use App\Exceptions\Api\EntityStillInUseApiException; use App\Exceptions\Api\OnlyOwnerCanChangeOwnership; @@ -75,6 +76,7 @@ class MemberService * @throws ChangingRoleToPlaceholderIsNotAllowed * @throws OnlyOwnerCanChangeOwnership * @throws OrganizationNeedsAtLeastOneOwner + * @throws ChangingRoleOfPlaceholderIsNotAllowed */ public function changeRole(Member $member, Organization $organization, Role $newRole, bool $allowOwnerChange): void { @@ -82,6 +84,9 @@ class MemberService if ($oldRole === Role::Owner) { throw new OrganizationNeedsAtLeastOneOwner; } + if ($oldRole === Role::Placeholder) { + throw new ChangingRoleOfPlaceholderIsNotAllowed; + } if ($newRole === Role::Placeholder) { throw new ChangingRoleToPlaceholderIsNotAllowed; } diff --git a/lang/en/exceptions.php b/lang/en/exceptions.php index a69e2848..832a9898 100644 --- a/lang/en/exceptions.php +++ b/lang/en/exceptions.php @@ -4,15 +4,18 @@ declare(strict_types=1); use App\Exceptions\Api\CanNotDeleteUserWhoIsOwnerOfOrganizationWithMultipleMembers; use App\Exceptions\Api\CanNotRemoveOwnerFromOrganization; +use App\Exceptions\Api\ChangingRoleOfPlaceholderIsNotAllowed; use App\Exceptions\Api\ChangingRoleToPlaceholderIsNotAllowed; use App\Exceptions\Api\EntityStillInUseApiException; use App\Exceptions\Api\FeatureIsNotAvailableInFreePlanApiException; use App\Exceptions\Api\InactiveUserCanNotBeUsedApiException; use App\Exceptions\Api\OnlyOwnerCanChangeOwnership; +use App\Exceptions\Api\OnlyPlaceholdersCanBeMergedIntoAnotherMember; use App\Exceptions\Api\OrganizationHasNoSubscriptionButMultipleMembersException; use App\Exceptions\Api\OrganizationNeedsAtLeastOneOwner; use App\Exceptions\Api\PdfRendererIsNotConfiguredException; use App\Exceptions\Api\PersonalAccessClientIsNotConfiguredException; +use App\Exceptions\Api\ThisPlaceholderCanNotBeInvitedUseTheMergeToolInsteadException; use App\Exceptions\Api\TimeEntryCanNotBeRestartedApiException; use App\Exceptions\Api\TimeEntryStillRunningApiException; use App\Exceptions\Api\UserIsAlreadyMemberOfOrganizationApiException; @@ -39,6 +42,9 @@ return [ PdfRendererIsNotConfiguredException::KEY => 'PDF renderer is not configured', FeatureIsNotAvailableInFreePlanApiException::KEY => 'Feature is not available in free plan', PersonalAccessClientIsNotConfiguredException::KEY => 'Personal access client is not configured', + 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', ], 'unknown_error_in_admin_panel' => 'An unknown error occurred. Please check the logs.', ]; diff --git a/lang/en/importer.php b/lang/en/importer.php index 2c6cec3d..7d5f13a8 100644 --- a/lang/en/importer.php +++ b/lang/en/importer.php @@ -13,7 +13,7 @@ return [ '
4. Now click Export -> Save as CSV. The Export dropdown is in the header of the export table left of the printer symbol. '. '

Before you import make sure that the Timezone settings in Clockify are the same as in solidtime.', ], - 'generic_project' => [ + 'generic_projects' => [ 'name' => 'Generic Projects', 'description' => 'If you want to import many projects yourself this importer the right choice. Please see our docs for more information about the CSV structure', ], diff --git a/routes/api.php b/routes/api.php index ac3fc5c3..1c38f348 100644 --- a/routes/api.php +++ b/routes/api.php @@ -51,6 +51,7 @@ Route::prefix('v1')->name('v1.')->group(static function (): void { Route::delete('/members/{member}', [MemberController::class, 'destroy'])->name('destroy'); Route::post('/members/{member}/invite-placeholder', [MemberController::class, 'invitePlaceholder'])->name('invite-placeholder'); Route::post('/members/{member}/make-placeholder', [MemberController::class, 'makePlaceholder'])->name('make-placeholder'); + Route::post('member/{member}/merge-into', [MemberController::class, 'mergeInto'])->name('merge-into'); }); // User routes diff --git a/tests/Unit/Console/Commands/Correction/CorrectionPlaceholderMembersCommandTest.php b/tests/Unit/Console/Commands/Correction/CorrectionPlaceholderMembersCommandTest.php new file mode 100644 index 00000000..651f1fb4 --- /dev/null +++ b/tests/Unit/Console/Commands/Correction/CorrectionPlaceholderMembersCommandTest.php @@ -0,0 +1,68 @@ +create(); + $user1 = User::factory()->placeholder()->create(); + $member1 = Member::factory()->forOrganization($organization)->forUser($user1)->role(Role::Admin)->create(); + $user2 = User::factory()->create(); + $member2 = Member::factory()->forOrganization($organization)->forUser($user2)->role(Role::Admin)->create(); + + // Act + $exitCode = $this->withoutMockingConsoleOutput()->artisan('correction:placeholder-members'); + + // Assert + $this->assertSame(Command::SUCCESS, $exitCode); + $output = Artisan::output(); + $member1->refresh(); + $this->assertSame(Role::Placeholder->value, $member1->role); + $member2->refresh(); + $this->assertSame(Role::Admin->value, $member2->role); + $this->assertSame("Sets all members who belong to a placeholder user to role placeholder...\n". + 'Set role of member (id='.$member1->getKey().") to placeholder\n", $output); + } + + public function test_sets_member_role_to_placeholder_if_user_is_placeholder_dry_run(): void + { + // Arrange + $organization = Organization::factory()->create(); + $user1 = User::factory()->placeholder()->create(); + $member1 = Member::factory()->forOrganization($organization)->forUser($user1)->role(Role::Admin)->create(); + $user2 = User::factory()->create(); + $member2 = Member::factory()->forOrganization($organization)->forUser($user2)->role(Role::Admin)->create(); + + // Act + $exitCode = $this->withoutMockingConsoleOutput()->artisan('correction:placeholder-members --dry-run'); + + // Assert + $this->assertSame(Command::SUCCESS, $exitCode); + $output = Artisan::output(); + $member1->refresh(); + $this->assertSame(Role::Admin->value, $member1->role); + $member2->refresh(); + $this->assertSame(Role::Admin->value, $member2->role); + $this->assertSame("Sets all members who belong to a placeholder user to role placeholder...\n". + "Running in dry-run mode. Nothing will be saved to the database.\n". + 'Set role of member (id='.$member1->getKey().") to placeholder\n", $output); + } +} diff --git a/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php b/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php index 8f826117..6dfb3982 100644 --- a/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php @@ -194,6 +194,164 @@ class MemberEndpointTest extends ApiEndpointTestAbstract $response->assertJsonPath('message', 'Only owner can change ownership'); } + public function test_update_member_fails_if_user_tries_to_change_the_role_of_a_placeholder(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'members:update', + ]); + $user = User::factory()->placeholder()->create(); + $member = Member::factory()->forOrganization($data->organization)->forUser($user)->role(Role::Placeholder)->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.members.update', [$data->organization->getKey(), $member->getKey()]), [ + 'role' => Role::Admin->value, + ]); + + // Assert + $response->assertStatus(400); + $response->assertExactJson([ + 'error' => true, + 'key' => 'changing_role_of_placeholder_is_not_allowed', + 'message' => 'Changing role of placeholder is not allowed', + ]); + } + + public function test_merge_into_fails_if_url_member_is_not_part_of_organization(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'members:merge-into', + ]); + $userSource = User::factory()->placeholder()->create(); + $memberSource = Member::factory()->forUser($userSource)->role(Role::Placeholder)->create(); + + $userDestination = User::factory()->create(); + $memberDestination = Member::factory()->forUser($userDestination)->forOrganization($data->organization)->role(Role::Admin)->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->postJson(route('api.v1.members.merge-into', [$data->organization->getKey(), $memberSource->getKey()]), [ + 'member_id' => $memberDestination->getKey(), + ]); + + // Assert + $response->assertForbidden(); + } + + public function test_merge_into_returns_validation_error_if_member_in_body_does_not_belong_to_organization(): 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(); + + $userDestination = User::factory()->create(); + $memberDestination = Member::factory()->forUser($userDestination)->role(Role::Admin)->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->postJson(route('api.v1.members.merge-into', [$data->organization->getKey(), $memberSource->getKey()]), [ + 'member_id' => $memberDestination->getKey(), + ]); + + // Assert + $response->assertStatus(422); + $response->assertExactJson([ + 'errors' => [ + 'member_id' => [ + 'The resource does not exist.', + ], + ], + 'message' => 'The resource does not exist.', + ]); + } + + public function test_merge_into_fails_if_from_member_is_not_a_placeholder(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'members:merge-into', + ]); + $userSource = User::factory()->placeholder()->create(); + $memberSource = Member::factory()->forUser($userSource)->forOrganization($data->organization)->role(Role::Admin)->create(); + + $userDestination = User::factory()->create(); + $memberDestination = Member::factory()->forUser($userDestination)->forOrganization($data->organization)->role(Role::Admin)->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->postJson(route('api.v1.members.merge-into', [$data->organization->getKey(), $memberSource->getKey()]), [ + 'member_id' => $memberDestination->getKey(), + ]); + + // Assert + $response->assertStatus(400); + $response->assertExactJson([ + 'error' => true, + 'key' => 'only_placeholders_can_be_merged_into_another_member', + 'message' => 'Only placeholders can be merged into another member', + ]); + } + + public function test_merge_into_fails_if_user_has_no_permission_to_merge_members(): void + { + // Arrange + $data = $this->createUserWithPermission([]); + $userSource = User::factory()->placeholder()->create(); + $memberSource = Member::factory()->forUser($userSource)->forOrganization($data->organization)->role(Role::Placeholder)->create(); + + $userDestination = User::factory()->create(); + $memberDestination = Member::factory()->forUser($userDestination)->forOrganization($data->organization)->role(Role::Admin)->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->postJson(route('api.v1.members.merge-into', [$data->organization->getKey(), $memberSource->getKey()]), [ + 'member_id' => $memberDestination->getKey(), + ]); + + // Assert + $response->assertForbidden(); + } + + public function test_merge_into_assigns_resources_of_source_member_to_destination_member_and_deletes_member(): 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(); + + $userDestination = User::factory()->create(); + $memberDestination = Member::factory()->forUser($userDestination)->forOrganization($data->organization)->role(Role::Admin)->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->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(3, $memberDestination->timeEntries); + $this->assertCount(1, $memberDestination->projectMembers); + } + public function test_update_member_fails_if_user_tries_to_change_role_of_the_current_owner(): void { // Arrange @@ -281,6 +439,7 @@ class MemberEndpointTest extends ApiEndpointTestAbstract public function test_invite_placeholder_succeeds_if_data_is_valid(): void { + // Arrange $data = $this->createUserWithPermission([ 'members:invite-placeholder', ]); @@ -301,6 +460,38 @@ class MemberEndpointTest extends ApiEndpointTestAbstract $response->assertStatus(204); } + public function test_invite_placeholder_fails_if_the_placeholder_has_a_invalid_email_from_an_import(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'members:invite-placeholder', + ]); + $user = User::factory()->create([ + 'is_placeholder' => true, + 'email' => 'some.user@solidtime-import.test', + ]); + $member = Member::factory() + ->forUser($user) + ->forOrganization($data->organization) + ->role(Role::Placeholder) + ->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->postJson(route('api.v1.members.invite-placeholder', [ + 'organization' => $data->organization->getKey(), + 'member' => $member->getKey(), + ])); + + // Assert + $response->assertStatus(400); + $response->assertExactJson([ + 'error' => true, + 'key' => 'this_placeholder_can_not_be_invited_use_the_merge_tool_instead_api_exception', + 'message' => 'This placeholder can not be invited use the merge tool instead', + ]); + } + public function test_destroy_member_fails_if_user_has_no_permission_to_delete_members(): void { // Arrange @@ -455,6 +646,34 @@ class MemberEndpointTest extends ApiEndpointTestAbstract Event::assertNotDispatched(MemberMadeToPlaceholder::class); } + public function test_make_placeholder_fails_if_user_is_already_a_placeholder(): void + { + // Arrange + Event::fake([ + MemberMadeToPlaceholder::class, + ]); + $data = $this->createUserWithPermission([ + 'members:make-placeholder', + ]); + $user = User::factory()->placeholder()->create(); + $member = Member::factory()->forUser($user)->forOrganization($data->organization)->role(Role::Placeholder)->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->postJson(route('api.v1.members.make-placeholder', [ + 'organization' => $data->organization->getKey(), + 'member' => $member->getKey(), + ])); + + // Assert + $response->assertStatus(400); + $response->assertExactJson([ + 'error' => true, + 'key' => 'changing_role_of_placeholder_is_not_allowed', + 'message' => 'Changing role of placeholder is not allowed', + ]); + } + public function test_make_placeholder_fails_if_member_is_owner(): void { // Arrange