From 2e0db46fff096dde705054e4822d2cce128891c6 Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Sun, 19 May 2024 12:05:08 +0200 Subject: [PATCH] Added endpoint to update multiple time entries at once --- app/Http/Controllers/Api/V1/Controller.php | 31 ++ .../Api/V1/TimeEntryController.php | 51 +++ .../TimeEntryUpdateMultipleRequest.php | 102 +++++ routes/api.php | 1 + .../Api/V1/ApiEndpointTestAbstract.php | 8 +- .../Endpoint/Api/V1/TimeEntryEndpointTest.php | 352 ++++++++++++++++++ 6 files changed, 542 insertions(+), 3 deletions(-) create mode 100644 app/Http/Requests/V1/TimeEntry/TimeEntryUpdateMultipleRequest.php diff --git a/app/Http/Controllers/Api/V1/Controller.php b/app/Http/Controllers/Api/V1/Controller.php index 21340039..5d97fab7 100644 --- a/app/Http/Controllers/Api/V1/Controller.php +++ b/app/Http/Controllers/Api/V1/Controller.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Http\Controllers\Api\V1; +use App\Models\Member; use App\Models\Organization; use App\Models\User; use App\Service\PermissionStore; @@ -28,6 +29,21 @@ class Controller extends \App\Http\Controllers\Controller } } + /** + * @param array $permissions + * + * @throws AuthorizationException + */ + protected function checkAnyPermission(Organization $organization, array $permissions): void + { + foreach ($permissions as $permission) { + if ($this->permissionStore->has($organization, $permission)) { + return; + } + } + throw new AuthorizationException(); + } + protected function hasPermission(Organization $organization, string $permission): bool { return $this->permissionStore->has($organization, $permission); @@ -47,4 +63,19 @@ class Controller extends \App\Http\Controllers\Controller return $user; } + + /** + * @throws AuthorizationException + */ + protected function member(Organization $organization): Member + { + $user = $this->user(); + $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'); + throw new AuthorizationException(); + } + + return $member; + } } diff --git a/app/Http/Controllers/Api/V1/TimeEntryController.php b/app/Http/Controllers/Api/V1/TimeEntryController.php index 036de1d8..04cbfc88 100644 --- a/app/Http/Controllers/Api/V1/TimeEntryController.php +++ b/app/Http/Controllers/Api/V1/TimeEntryController.php @@ -10,6 +10,7 @@ use App\Exceptions\Api\TimeEntryStillRunningApiException; use App\Http\Requests\V1\TimeEntry\TimeEntryAggregateRequest; use App\Http\Requests\V1\TimeEntry\TimeEntryIndexRequest; use App\Http\Requests\V1\TimeEntry\TimeEntryStoreRequest; +use App\Http\Requests\V1\TimeEntry\TimeEntryUpdateMultipleRequest; use App\Http\Requests\V1\TimeEntry\TimeEntryUpdateRequest; use App\Http\Resources\V1\TimeEntry\TimeEntryCollection; use App\Http\Resources\V1\TimeEntry\TimeEntryResource; @@ -359,6 +360,56 @@ class TimeEntryController extends Controller return new TimeEntryResource($timeEntry); } + /** + * @throws AuthorizationException + */ + public function updateMultiple(Organization $organization, TimeEntryUpdateMultipleRequest $request): JsonResponse + { + $this->checkAnyPermission($organization, ['time-entries:update:all', 'time-entries:update:own']); + $canAccessAll = $this->hasPermission($organization, 'time-entries:update:all'); + + $ids = $request->get('ids'); + + $timeEntries = TimeEntry::query() + ->whereBelongsTo($organization, 'organization') + ->whereIn('id', $ids) + ->get(); + + $changes = $request->get('changes'); + + if (isset($changes['member_id']) && ! $canAccessAll && $this->member($organization)->getKey() !== $changes['member_id']) { + throw new AuthorizationException(); + } + + $success = new Collection(); + $error = new Collection(); + + foreach ($ids as $id) { + $timeEntry = $timeEntries->firstWhere('id', $id); + if ($timeEntry === null) { + // Note: ID wrong or time entry in different organization + $error->push($id); + + continue; + } + if (! $canAccessAll && $timeEntry->user_id !== Auth::id()) { + $error->push($id); + + continue; + + } + + $timeEntry->fill($changes); + $timeEntry->save(); + $success->push($id); + } + + return response()->json([ + 'success' => $success->toArray(), + 'error' => $error->toArray(), + ]); + } + /** * Delete time entry * diff --git a/app/Http/Requests/V1/TimeEntry/TimeEntryUpdateMultipleRequest.php b/app/Http/Requests/V1/TimeEntry/TimeEntryUpdateMultipleRequest.php new file mode 100644 index 00000000..d4d27767 --- /dev/null +++ b/app/Http/Requests/V1/TimeEntry/TimeEntryUpdateMultipleRequest.php @@ -0,0 +1,102 @@ +> + */ + public function rules(): array + { + return [ + 'ids' => [ + 'required', + 'array', + ], + 'ids.*' => [ + 'string', + 'uuid', + ], + 'changes' => [ + 'required', + 'array', + ], + // ID of the organization member that the time entry should belong to + 'changes.member_id' => [ + 'string', + 'uuid', + new ExistsEloquent(Member::class, null, function (Builder $builder): Builder { + /** @var Builder $builder */ + return $builder->whereBelongsTo($this->organization, 'organization'); + }), + ], + // ID of the project that the time entry should belong to + 'changes.project_id' => [ + 'nullable', + 'string', + 'uuid', + 'required_with:task_id', + new ExistsEloquent(Project::class, null, function (Builder $builder): Builder { + /** @var Builder $builder */ + return $builder->whereBelongsTo($this->organization, 'organization'); + }), + ], + // ID of the task that the time entry should belong to + 'changes.task_id' => [ + 'nullable', + 'string', + 'uuid', + new ExistsEloquent(Task::class, null, function (Builder $builder): Builder { + /** @var Builder $builder */ + return $builder->whereBelongsTo($this->organization, 'organization'); + }), + (new ExistsEloquent(Task::class, null, function (Builder $builder): Builder { + /** @var Builder $builder */ + return $builder->whereBelongsTo($this->organization, 'organization') + ->where('project_id', $this->input('changes.project_id')); + }))->withMessage(__('validation.task_belongs_to_project')), + ], + // Whether time entry is billable + 'changes.billable' => [ + 'boolean', + ], + // Description of time entry + 'changes.description' => [ + 'nullable', + 'string', + 'max:500', + ], + // List of tag IDs + 'changes.tags' => [ + 'nullable', + 'array', + ], + 'changes.tags.*' => [ + 'string', + 'uuid', + new ExistsEloquent(Tag::class, null, function (Builder $builder): Builder { + /** @var Builder $builder */ + return $builder->whereBelongsTo($this->organization, 'organization'); + }), + ], + ]; + } +} diff --git a/routes/api.php b/routes/api.php index a7520fff..38823edf 100644 --- a/routes/api.php +++ b/routes/api.php @@ -76,6 +76,7 @@ Route::middleware([ Route::get('/organizations/{organization}/time-entries/aggregate', [TimeEntryController::class, 'aggregate'])->name('aggregate'); Route::post('/organizations/{organization}/time-entries', [TimeEntryController::class, 'store'])->name('store'); Route::put('/organizations/{organization}/time-entries/{timeEntry}', [TimeEntryController::class, 'update'])->name('update'); + Route::patch('/organizations/{organization}/time-entries', [TimeEntryController::class, 'updateMultiple'])->name('update-multiple'); Route::delete('/organizations/{organization}/time-entries/{timeEntry}', [TimeEntryController::class, 'destroy'])->name('destroy'); }); diff --git a/tests/Unit/Endpoint/Api/V1/ApiEndpointTestAbstract.php b/tests/Unit/Endpoint/Api/V1/ApiEndpointTestAbstract.php index 1d590b23..4d8efb74 100644 --- a/tests/Unit/Endpoint/Api/V1/ApiEndpointTestAbstract.php +++ b/tests/Unit/Endpoint/Api/V1/ApiEndpointTestAbstract.php @@ -8,6 +8,7 @@ use App\Models\Member; use App\Models\Organization; use App\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Str; use Laravel\Jetstream\Jetstream; use Tests\TestCase; @@ -19,9 +20,10 @@ class ApiEndpointTestAbstract extends TestCase * @param array $permissions * @return object{user: User, organization: Organization, member: Member} */ - protected function createUserWithPermission(array $permissions, bool $isOwner = false): object + protected function createUserWithPermission(array $permissions = [], bool $isOwner = false): object { - Jetstream::role('custom-test', 'Custom Test', $permissions) + $roleName = 'custom-test-'.Str::uuid(); + Jetstream::role($roleName, 'Custom Test', $permissions) ->description('Role custom for testing'); $user = User::factory()->create(); if ($isOwner) { @@ -30,7 +32,7 @@ class ApiEndpointTestAbstract extends TestCase $organization = Organization::factory()->create(); } $member = Member::factory()->forUser($user)->forOrganization($organization)->create([ - 'role' => 'custom-test', + 'role' => $roleName, ]); return (object) [ diff --git a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php index a00ed300..933d734c 100644 --- a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php @@ -5,8 +5,10 @@ declare(strict_types=1); namespace Tests\Unit\Endpoint\Api\V1; use App\Enums\Role; +use App\Exceptions\Api\TimeEntryCanNotBeRestartedApiException; use App\Models\Member; use App\Models\Project; +use App\Models\Task; use App\Models\TimeEntry; use App\Models\User; use Carbon\Carbon; @@ -860,6 +862,32 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract ]); } + public function test_update_endpoint_fails_if_user_tries_to_reactivate_a_time_entry(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->create(); + $timeEntryFake = TimeEntry::factory()->forOrganization($data->organization)->make(); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [ + 'description' => $timeEntryFake->description, + 'start' => $timeEntryFake->start->toIso8601ZuluString(), + 'end' => null, + 'tags' => $timeEntryFake->tags, + 'member_id' => $data->member->getKey(), + 'task_id' => $timeEntryFake->task_id, + ]); + + // Assert + $response->assertStatus(400); + $response->assertJsonPath('error', true); + $response->assertJsonPath('message', __('exceptions.api.'.TimeEntryCanNotBeRestartedApiException::KEY)); + } + public function test_update_endpoint_updates_time_entry_of_other_user_in_organization(): void { // Arrange @@ -999,4 +1027,328 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract 'id' => $timeEntry->getKey(), ]); } + + public function test_update_multiple_endpoint_fails_if_user_has_no_permission_to_update_own_time_entries_or_all_time_entries(): void + { + // Arrange + $data = $this->createUserWithPermission(); + $timeEntries = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->createMany(3); + $timeEntriesFake = TimeEntry::factory()->forOrganization($data->organization)->make(); + Passport::actingAs($data->user); + + // Act + $response = $this->patchJson(route('api.v1.time-entries.update-multiple', [$data->organization->getKey()]), [ + 'ids' => $timeEntries->pluck('id')->toArray(), + 'changes' => [ + 'description' => $timeEntriesFake->description, + ], + ]); + + // Assert + $response->assertValid(); + $response->assertForbidden(); + } + + public function test_update_multiple_updates_own_time_entries_and_fails_for_time_entries_of_other_users_and_and_other_organizations_with_own_time_entries_permission(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $otherData = $this->createUserWithPermission(); + $otherUser = User::factory()->create(); + $otherMember = Member::factory()->forOrganization($data->organization)->forUser($otherUser)->role(Role::Employee)->create(); + + $ownTimeEntry = TimeEntry::factory()->forMember($data->member)->create(); + $otherTimeEntry = TimeEntry::factory()->forMember($otherMember)->create(); + $otherOrganizationTimeEntry = TimeEntry::factory()->forMember($otherData->member)->create(); + $timeEntriesFake = TimeEntry::factory()->forOrganization($data->organization)->make(); + $wrongId = Str::uuid(); + Passport::actingAs($data->user); + + // Act + $response = $this->patchJson(route('api.v1.time-entries.update-multiple', [$data->organization->getKey()]), [ + 'ids' => [ + $ownTimeEntry->getKey(), + $otherTimeEntry->getKey(), + $otherOrganizationTimeEntry->getKey(), + $wrongId, + ], + 'changes' => [ + 'description' => $timeEntriesFake->description, + ], + ]); + + // Assert + $response->assertValid(); + $response->assertStatus(200); + $response->assertExactJson([ + 'success' => [ + $ownTimeEntry->getKey(), + ], + 'error' => [ + $otherTimeEntry->getKey(), + $otherOrganizationTimeEntry->getKey(), + $wrongId, + ], + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $ownTimeEntry->getKey(), + 'description' => $timeEntriesFake->description, + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $otherOrganizationTimeEntry->getKey(), + 'description' => $otherOrganizationTimeEntry->description, + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $otherTimeEntry->getKey(), + 'description' => $otherTimeEntry->description, + ]); + } + + public function test_update_multiple_updates_own_time_entries_and_fails_for_time_entries_of_other_users_and_and_other_organizations_with_own_time_entries_permission_and_full_changeset(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $otherData = $this->createUserWithPermission(); + $otherUser = User::factory()->create(); + $otherMember = Member::factory()->forOrganization($data->organization)->forUser($otherUser)->role(Role::Employee)->create(); + + $ownTimeEntry = TimeEntry::factory()->forMember($data->member)->create(); + $otherTimeEntry = TimeEntry::factory()->forMember($otherMember)->create(); + $otherOrganizationTimeEntry = TimeEntry::factory()->forMember($otherData->member)->create(); + $timeEntriesFake = TimeEntry::factory()->forOrganization($data->organization)->withTags($data->organization)->make(); + $project = Project::factory()->forOrganization($data->organization)->create(); + $task = Task::factory()->forProject($project)->forOrganization($data->organization)->create(); + $wrongId = Str::uuid(); + Passport::actingAs($data->user); + + // Act + $response = $this->patchJson(route('api.v1.time-entries.update-multiple', [$data->organization->getKey()]), [ + 'ids' => [ + $ownTimeEntry->getKey(), + $otherTimeEntry->getKey(), + $otherOrganizationTimeEntry->getKey(), + $wrongId, + ], + 'changes' => [ + 'member_id' => $data->member->getKey(), + 'project_id' => $project->getKey(), + 'task_id' => $task->getKey(), + 'billable' => $timeEntriesFake->billable, + 'description' => $timeEntriesFake->description, + 'tags' => $timeEntriesFake->tags, + ], + ]); + + // Assert + $response->assertValid(); + $response->assertStatus(200); + $response->assertExactJson([ + 'success' => [ + $ownTimeEntry->getKey(), + ], + 'error' => [ + $otherTimeEntry->getKey(), + $otherOrganizationTimeEntry->getKey(), + $wrongId, + ], + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $ownTimeEntry->getKey(), + 'member_id' => $data->member->getKey(), + 'project_id' => $project->getKey(), + 'task_id' => $task->getKey(), + 'billable' => $timeEntriesFake->billable, + 'description' => $timeEntriesFake->description, + 'tags' => json_encode($timeEntriesFake->tags), + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $otherOrganizationTimeEntry->getKey(), + 'member_id' => $otherOrganizationTimeEntry->member_id, + 'project_id' => $otherOrganizationTimeEntry->project_id, + 'task_id' => $otherOrganizationTimeEntry->task_id, + 'billable' => $otherOrganizationTimeEntry->billable, + 'description' => $otherOrganizationTimeEntry->description, + 'tags' => json_encode($otherOrganizationTimeEntry->tags), + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $otherTimeEntry->getKey(), + 'member_id' => $otherTimeEntry->member_id, + 'project_id' => $otherTimeEntry->project_id, + 'task_id' => $otherTimeEntry->task_id, + 'billable' => $otherTimeEntry->billable, + 'description' => $otherTimeEntry->description, + 'tags' => json_encode($otherTimeEntry->tags), + ]); + } + + public function test_update_multiple_updates_all_time_entries_and_fails_for_time_entries_of_other_users_and_and_other_organizations_with_all_time_entries_permission(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:all', + ]); + $otherData = $this->createUserWithPermission(); + $otherUser = User::factory()->create(); + $otherMember = Member::factory()->forOrganization($data->organization)->forUser($otherUser)->role(Role::Employee)->create(); + + $ownTimeEntry = TimeEntry::factory()->forMember($data->member)->create(); + $otherTimeEntry = TimeEntry::factory()->forMember($otherMember)->create(); + $otherOrganizationTimeEntry = TimeEntry::factory()->forMember($otherData->member)->create(); + $timeEntriesFake = TimeEntry::factory()->forOrganization($data->organization)->make(); + $wrongId = Str::uuid(); + Passport::actingAs($data->user); + + // Act + $response = $this->patchJson(route('api.v1.time-entries.update-multiple', [$data->organization->getKey()]), [ + 'ids' => [ + $ownTimeEntry->getKey(), + $otherTimeEntry->getKey(), + $otherOrganizationTimeEntry->getKey(), + $wrongId, + ], + 'changes' => [ + 'description' => $timeEntriesFake->description, + ], + ]); + + // Assert + $response->assertValid(); + $response->assertStatus(200); + $response->assertExactJson([ + 'success' => [ + $ownTimeEntry->getKey(), + $otherTimeEntry->getKey(), + ], + 'error' => [ + $otherOrganizationTimeEntry->getKey(), + $wrongId, + ], + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $ownTimeEntry->getKey(), + 'description' => $timeEntriesFake->description, + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $otherOrganizationTimeEntry->getKey(), + 'description' => $otherOrganizationTimeEntry->description, + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $otherTimeEntry->getKey(), + 'description' => $timeEntriesFake->description, + ]); + } + + public function test_update_multiple_updates_all_time_entries_and_fails_for_time_entries_of_other_users_and_and_other_organizations_with_all_time_entries_permission_and_full_changeset(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:all', + ]); + $otherData = $this->createUserWithPermission(); + $otherUser = User::factory()->create(); + $otherMember = Member::factory()->forOrganization($data->organization)->forUser($otherUser)->role(Role::Employee)->create(); + + $ownTimeEntry = TimeEntry::factory()->forMember($data->member)->create(); + $otherTimeEntry = TimeEntry::factory()->forMember($otherMember)->create(); + $otherOrganizationTimeEntry = TimeEntry::factory()->forMember($otherData->member)->create(); + $timeEntriesFake = TimeEntry::factory()->forOrganization($data->organization)->withTags($data->organization)->make(); + $project = Project::factory()->forOrganization($data->organization)->create(); + $task = Task::factory()->forProject($project)->forOrganization($data->organization)->create(); + $wrongId = Str::uuid(); + Passport::actingAs($data->user); + + // Act + $response = $this->patchJson(route('api.v1.time-entries.update-multiple', [$data->organization->getKey()]), [ + 'ids' => [ + $ownTimeEntry->getKey(), + $otherTimeEntry->getKey(), + $otherOrganizationTimeEntry->getKey(), + $wrongId, + ], + 'changes' => [ + 'member_id' => $otherMember->getKey(), + 'project_id' => $project->getKey(), + 'task_id' => $task->getKey(), + 'billable' => $timeEntriesFake->billable, + 'description' => $timeEntriesFake->description, + 'tags' => $timeEntriesFake->tags, + ], + ]); + + // Assert + $response->assertValid(); + $response->assertStatus(200); + $response->assertExactJson([ + 'success' => [ + $ownTimeEntry->getKey(), + $otherTimeEntry->getKey(), + ], + 'error' => [ + $otherOrganizationTimeEntry->getKey(), + $wrongId, + ], + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $ownTimeEntry->getKey(), + 'member_id' => $otherMember->getKey(), + 'project_id' => $project->getKey(), + 'task_id' => $task->getKey(), + 'billable' => $timeEntriesFake->billable, + 'description' => $timeEntriesFake->description, + 'tags' => json_encode($timeEntriesFake->tags), + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $otherOrganizationTimeEntry->getKey(), + 'member_id' => $otherOrganizationTimeEntry->member_id, + 'project_id' => $otherOrganizationTimeEntry->project_id, + 'task_id' => $otherOrganizationTimeEntry->task_id, + 'billable' => $otherOrganizationTimeEntry->billable, + 'description' => $otherOrganizationTimeEntry->description, + 'tags' => json_encode($otherOrganizationTimeEntry->tags), + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $otherTimeEntry->getKey(), + 'member_id' => $otherMember->getKey(), + 'project_id' => $project->getKey(), + 'task_id' => $task->getKey(), + 'billable' => $timeEntriesFake->billable, + 'description' => $timeEntriesFake->description, + 'tags' => json_encode($timeEntriesFake->tags), + ]); + } + + public function test_update_multiple_updates_own_time_entries_fails_if_member_id_is_not_your_own_and_you_dont_have_update_all_permission(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $otherUser = User::factory()->create(); + $otherMember = Member::factory()->forOrganization($data->organization)->forUser($otherUser)->role(Role::Employee)->create(); + + $ownTimeEntry = TimeEntry::factory()->forMember($data->member)->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->patchJson(route('api.v1.time-entries.update-multiple', [$data->organization->getKey()]), [ + 'ids' => [ + $ownTimeEntry->getKey(), + ], + 'changes' => [ + 'member_id' => $otherMember->getKey(), + ], + ]); + + // Assert + $response->assertValid(); + $response->assertStatus(403); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $ownTimeEntry->getKey(), + 'member_id' => $ownTimeEntry->member_id, + ]); + } }