From 1863953a23e9589ceb4ce792c577713c70c29ab9 Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Tue, 13 Oct 2026 17:08:25 +0200 Subject: [PATCH] Reject changing the member of running time entries Moving a running time entry to another member could give that member's user a second running time entry. Stopping the time entry and changing the member in the same request is still allowed. --- ...anNotBeChangedWhileRunningApiException.php | 15 +++ .../Api/V1/TimeEntryController.php | 14 ++- lang/en/exceptions.php | 2 + .../Endpoint/Api/V1/TimeEntryEndpointTest.php | 119 ++++++++++++++++++ .../Unit/Exceptions/Api/ApiExceptionTest.php | 2 + 5 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 app/Exceptions/Api/TimeEntryMemberCanNotBeChangedWhileRunningApiException.php diff --git a/app/Exceptions/Api/TimeEntryMemberCanNotBeChangedWhileRunningApiException.php b/app/Exceptions/Api/TimeEntryMemberCanNotBeChangedWhileRunningApiException.php new file mode 100644 index 00000000..b39367d7 --- /dev/null +++ b/app/Exceptions/Api/TimeEntryMemberCanNotBeChangedWhileRunningApiException.php @@ -0,0 +1,15 @@ +has('end') ? ($request->input('end') !== null ? Carbon::parse($request->input('end')) : null) : $timeEntry->end; $this->assertNoOverlap($organization, $effectiveMember, $effectiveStart, $effectiveEnd, $timeEntry); + // The new member could already have a running time entry + if ($effectiveEnd === null && $newMember !== null && $newMember->getKey() !== $timeEntry->member_id) { + throw new TimeEntryMemberCanNotBeChangedWhileRunningApiException; + } + $oldProject = $timeEntry->project; $oldTask = $timeEntry->task; @@ -819,6 +825,12 @@ class TimeEntryController extends Controller continue; } + // The new member could already have a running time entry + if ($newMember !== null && $timeEntry->end === null && $newMember->getKey() !== $timeEntry->member_id) { + $error->push($id); + + continue; + } $oldProject = $timeEntry->project; $oldTask = $timeEntry->task; diff --git a/lang/en/exceptions.php b/lang/en/exceptions.php index 485eb145..110125b0 100644 --- a/lang/en/exceptions.php +++ b/lang/en/exceptions.php @@ -19,6 +19,7 @@ use App\Exceptions\Api\PdfRendererIsNotConfiguredException; use App\Exceptions\Api\PersonalAccessClientIsNotConfiguredException; use App\Exceptions\Api\ThisPlaceholderCanNotBeInvitedUseTheMergeToolInsteadException; use App\Exceptions\Api\TimeEntryCanNotBeRestartedApiException; +use App\Exceptions\Api\TimeEntryMemberCanNotBeChangedWhileRunningApiException; use App\Exceptions\Api\TimeEntryStillRunningApiException; use App\Exceptions\Api\UserIsAlreadyMemberOfOrganizationApiException; use App\Exceptions\Api\UserIsAlreadyMemberOfProjectApiException; @@ -31,6 +32,7 @@ return [ TimeEntryStillRunningApiException::KEY => 'Time entry is still running', UserNotPlaceholderApiException::KEY => 'The given user is not a placeholder', TimeEntryCanNotBeRestartedApiException::KEY => 'Time entry is already stopped and can not be restarted', + TimeEntryMemberCanNotBeChangedWhileRunningApiException::KEY => 'The member of a running time entry can not be changed', InactiveUserCanNotBeUsedApiException::KEY => 'Inactive user can not be used', UserIsAlreadyMemberOfOrganizationApiException::KEY => 'User is already a member of the organization', UserIsAlreadyMemberOfProjectApiException::KEY => 'User is already a member of the project', diff --git a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php index 42c32413..a0c0c1c6 100644 --- a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php @@ -3332,6 +3332,80 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract ]); } + public function test_update_endpoint_fails_if_member_of_running_time_entry_is_changed(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:all', + ]); + $otherMember = Member::factory()->forOrganization($data->organization)->role(Role::Employee)->create(); + $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->active()->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [ + 'member_id' => $otherMember->getKey(), + ]); + + // Assert + $response->assertStatus(400); + $response->assertJsonPath('key', 'time_entry_member_can_not_be_changed_while_running'); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $timeEntry->getKey(), + 'member_id' => $data->member->getKey(), + 'user_id' => $data->user->getKey(), + ]); + } + + public function test_update_endpoint_can_update_running_time_entry_with_unchanged_member_id(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->active()->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [ + 'member_id' => $data->member->getKey(), + 'description' => 'New description', + ]); + + // Assert + $response->assertStatus(200); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $timeEntry->getKey(), + 'description' => 'New description', + ]); + } + + public function test_update_endpoint_can_stop_running_time_entry_and_change_member_in_the_same_request(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:all', + ]); + $otherUser = User::factory()->create(); + $otherMember = Member::factory()->forOrganization($data->organization)->forUser($otherUser)->role(Role::Employee)->create(); + $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->active()->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [ + 'member_id' => $otherMember->getKey(), + 'end' => Carbon::now()->toIso8601ZuluString(), + ]); + + // Assert + $response->assertStatus(200); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $timeEntry->getKey(), + 'member_id' => $otherMember->getKey(), + 'user_id' => $otherUser->getKey(), + ]); + } + public function test_update_endpoint_can_update_project_and_automatically_set_client(): void { // Arrange @@ -4199,6 +4273,51 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract ]); } + public function test_update_multiple_does_not_change_member_of_running_time_entries(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:all', + ]); + $otherMember = Member::factory()->forOrganization($data->organization)->role(Role::Employee)->create(); + $runningTimeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->active()->create(); + $runningTimeEntryOfOtherMember = TimeEntry::factory()->forOrganization($data->organization)->forMember($otherMember)->active()->create(); + $finishedTimeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->patchJson(route('api.v1.time-entries.update-multiple', [$data->organization->getKey()]), [ + 'ids' => [ + $runningTimeEntry->getKey(), + $runningTimeEntryOfOtherMember->getKey(), + $finishedTimeEntry->getKey(), + ], + 'changes' => [ + 'member_id' => $otherMember->getKey(), + ], + ]); + + // Assert + $response->assertStatus(200); + $response->assertExactJson([ + 'success' => [ + $runningTimeEntryOfOtherMember->getKey(), + $finishedTimeEntry->getKey(), + ], + 'error' => [ + $runningTimeEntry->getKey(), + ], + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $runningTimeEntry->getKey(), + 'member_id' => $data->member->getKey(), + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $finishedTimeEntry->getKey(), + 'member_id' => $otherMember->getKey(), + ]); + } + public function test_update_multiple_updates_user_id_when_member_id_changes(): void { // Arrange diff --git a/tests/Unit/Exceptions/Api/ApiExceptionTest.php b/tests/Unit/Exceptions/Api/ApiExceptionTest.php index e52bff0d..c698913f 100644 --- a/tests/Unit/Exceptions/Api/ApiExceptionTest.php +++ b/tests/Unit/Exceptions/Api/ApiExceptionTest.php @@ -22,6 +22,7 @@ use App\Exceptions\Api\PdfRendererIsNotConfiguredException; use App\Exceptions\Api\PersonalAccessClientIsNotConfiguredException; use App\Exceptions\Api\ThisPlaceholderCanNotBeInvitedUseTheMergeToolInsteadException; use App\Exceptions\Api\TimeEntryCanNotBeRestartedApiException; +use App\Exceptions\Api\TimeEntryMemberCanNotBeChangedWhileRunningApiException; use App\Exceptions\Api\TimeEntryStillRunningApiException; use App\Exceptions\Api\UserIsAlreadyMemberOfOrganizationApiException; use App\Exceptions\Api\UserIsAlreadyMemberOfProjectApiException; @@ -53,6 +54,7 @@ class ApiExceptionTest extends TestCase yield OverlappingTimeEntryApiException::class => [new OverlappingTimeEntryApiException]; yield ThisPlaceholderCanNotBeInvitedUseTheMergeToolInsteadException::class => [new ThisPlaceholderCanNotBeInvitedUseTheMergeToolInsteadException]; yield TimeEntryCanNotBeRestartedApiException::class => [new TimeEntryCanNotBeRestartedApiException]; + yield TimeEntryMemberCanNotBeChangedWhileRunningApiException::class => [new TimeEntryMemberCanNotBeChangedWhileRunningApiException]; yield TimeEntryStillRunningApiException::class => [new TimeEntryStillRunningApiException]; yield UserIsAlreadyMemberOfOrganizationApiException::class => [new UserIsAlreadyMemberOfOrganizationApiException]; yield UserIsAlreadyMemberOfProjectApiException::class => [new UserIsAlreadyMemberOfProjectApiException];