From 114a32536df5f3fbc1c4f44ad34d4963e57af09c Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Fri, 17 Jul 2026 18:59:44 +0200 Subject: [PATCH] Fixed update of member_id in time_entries.update and time_entries.updateMultiple Removed usage of legacy user_id in TimeEntryController --- .../Api/V1/TimeEntryController.php | 74 ++++--- .../Endpoint/Api/V1/TimeEntryEndpointTest.php | 197 +++++++++++++++++- 2 files changed, 242 insertions(+), 29 deletions(-) diff --git a/app/Http/Controllers/Api/V1/TimeEntryController.php b/app/Http/Controllers/Api/V1/TimeEntryController.php index cb1c37dd..b0a3bf31 100644 --- a/app/Http/Controllers/Api/V1/TimeEntryController.php +++ b/app/Http/Controllers/Api/V1/TimeEntryController.php @@ -67,7 +67,7 @@ class TimeEntryController extends Controller $query = TimeEntry::query() ->where('organization_id', $organization->getKey()) - ->where('user_id', $member->user_id) + ->where('member_id', $member->getKey()) ->when($exclude !== null, function (Builder $q) use ($exclude): void { $q->where('id', '!=', $exclude->getKey()); }) @@ -107,8 +107,8 @@ class TimeEntryController extends Controller /** * Get time entries in organization * - * If you only need time entries for a specific user, you can filter by `user_id`. - * Users with the permission `time-entries:view:own` can only use this endpoint with their own user ID in the user_id filter. + * If you only need time entries for a specific user, you can filter by `member_id`. + * Users with the permission `time-entries:view:own` can only use this endpoint with their own member ID in the member_id filter. * * @return TimeEntryCollection * @@ -118,16 +118,17 @@ class TimeEntryController extends Controller */ public function index(Organization $organization, TimeEntryIndexRequest $request): JsonResource { - /** @var Member|null $member */ - $member = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null; - if ($member !== null && $member->user_id === Auth::id()) { + $member = $this->member($organization); + /** @var Member|null $memberFilter */ + $memberFilter = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null; + if ($memberFilter !== null && $memberFilter->getKey() === $member->getKey()) { $this->checkPermission($organization, 'time-entries:view:own'); } else { $this->checkPermission($organization, 'time-entries:view:all'); } $canAccessPremiumFeatures = $this->canAccessPremiumFeatures($organization); - $timeEntriesQuery = $this->getTimeEntriesQuery($organization, $request, $member, $canAccessPremiumFeatures); + $timeEntriesQuery = $this->getTimeEntriesQuery($organization, $request, $memberFilter, $canAccessPremiumFeatures); $totalCount = $timeEntriesQuery->count(); @@ -158,7 +159,7 @@ class TimeEntryController extends Controller if ($timeEntries->count() === 0) { Log::warning('User has has more than '.$limit.' time entries on one date', [ 'date' => $lastDate->toDateString(), - 'user_id' => $request->input('user_id'), + 'member_id' => $request->input('member_id'), 'auth_user_id' => Auth::id(), 'limit' => $limit, ]); @@ -221,9 +222,10 @@ class TimeEntryController extends Controller */ public function indexExport(Organization $organization, TimeEntryIndexExportRequest $request, TimeEntryAggregationService $timeEntryAggregationService): JsonResponse { - /** @var Member|null $member */ - $member = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null; - if ($member !== null && $member->user_id === Auth::id()) { + $member = $this->member($organization); + /** @var Member|null $memberFilter */ + $memberFilter = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null; + if ($memberFilter !== null && $memberFilter->getKey() === $member->getKey()) { $this->checkPermission($organization, 'time-entries:view:own'); } else { $this->checkPermission($organization, 'time-entries:view:all'); @@ -240,7 +242,7 @@ class TimeEntryController extends Controller $roundingType = $canAccessPremiumFeatures ? $request->getRoundingType() : null; $roundingMinutes = $canAccessPremiumFeatures ? $request->getRoundingMinutes() : null; - $timeEntriesQuery = $this->getTimeEntriesQuery($organization, $request, $member, $canAccessPremiumFeatures); + $timeEntriesQuery = $this->getTimeEntriesQuery($organization, $request, $memberFilter, $canAccessPremiumFeatures); $timeEntriesQuery->with([ 'task', 'client', @@ -263,7 +265,7 @@ class TimeEntryController extends Controller if ($viewFile === false) { throw new \LogicException('View file not found'); } - $timeEntriesAggregateQuery = $this->getTimeEntriesAggregateQuery($organization, $request, $member); + $timeEntriesAggregateQuery = $this->getTimeEntriesAggregateQuery($organization, $request, $memberFilter); $aggregatedData = $timeEntryAggregationService->getAggregatedTimeEntries( $timeEntriesAggregateQuery, null, @@ -370,9 +372,10 @@ class TimeEntryController extends Controller */ public function aggregate(Organization $organization, TimeEntryAggregateRequest $request, TimeEntryAggregationService $timeEntryAggregationService): array { - /** @var Member|null $member */ - $member = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null; - if ($member !== null && $member->user_id === Auth::id()) { + $member = $this->member($organization); + /** @var Member|null $memberFilter */ + $memberFilter = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null; + if ($memberFilter !== null && $memberFilter->getKey() === $member->getKey()) { $this->checkPermission($organization, 'time-entries:view:own'); } else { $this->checkPermission($organization, 'time-entries:view:all'); @@ -383,7 +386,7 @@ class TimeEntryController extends Controller $group1Type = $request->getGroup(); $group2Type = $request->getSubGroup(); - $timeEntriesAggregateQuery = $this->getTimeEntriesAggregateQuery($organization, $request, $member); + $timeEntriesAggregateQuery = $this->getTimeEntriesAggregateQuery($organization, $request, $memberFilter); $roundingType = $canAccessPremiumFeatures ? $request->getRoundingType() : null; $roundingMinutes = $canAccessPremiumFeatures ? $request->getRoundingMinutes() : null; @@ -419,9 +422,10 @@ class TimeEntryController extends Controller */ public function aggregateExport(Organization $organization, TimeEntryAggregateExportRequest $request, TimeEntryAggregationService $timeEntryAggregationService): JsonResponse { - /** @var Member|null $member */ - $member = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null; - if ($member !== null && $member->user_id === Auth::id()) { + $member = $this->member($organization); + /** @var Member|null $memberFilter */ + $memberFilter = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null; + if ($memberFilter !== null && $memberFilter->getKey() === $member->getKey()) { $this->checkPermission($organization, 'time-entries:view:own'); } else { $this->checkPermission($organization, 'time-entries:view:all'); @@ -437,7 +441,7 @@ class TimeEntryController extends Controller $group = $request->getGroup(); $subGroup = $request->getSubGroup(); - $timeEntriesAggregateQuery = $this->getTimeEntriesAggregateQuery($organization, $request, $member); + $timeEntriesAggregateQuery = $this->getTimeEntriesAggregateQuery($organization, $request, $memberFilter); $roundingType = $canAccessPremiumFeatures ? $request->getRoundingType() : null; $roundingMinutes = $canAccessPremiumFeatures ? $request->getRoundingMinutes() : null; @@ -580,7 +584,7 @@ class TimeEntryController extends Controller { /** @var Member $member */ $member = Member::query()->findOrFail($request->input('member_id')); - if ($member->user_id === Auth::id()) { + if ($member->getKey() === $this->member($organization)->getKey()) { $this->checkPermission($organization, 'time-entries:create:own'); } else { $this->checkPermission($organization, 'time-entries:create:all'); @@ -627,9 +631,10 @@ class TimeEntryController extends Controller */ public function update(Organization $organization, TimeEntry $timeEntry, TimeEntryUpdateRequest $request): JsonResource { - /** @var Member|null $member */ - $member = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null; - if ($timeEntry->member->user_id === Auth::id() && ($member === null || $member->user_id === Auth::id())) { + $member = $this->member($organization); + /** @var Member|null $newMember */ + $newMember = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null; + if ($timeEntry->member_id === $member->getKey() && ($newMember === null || $newMember->getKey() === $member->getKey())) { $this->checkPermission($organization, 'time-entries:update:own', $timeEntry); } else { $this->checkPermission($organization, 'time-entries:update:all', $timeEntry); @@ -661,6 +666,10 @@ class TimeEntryController extends Controller } $timeEntry->fill($request->validated()); + if ($newMember !== null) { + $timeEntry->member()->associate($newMember); + $timeEntry->user()->associate($newMember->user); + } $timeEntry->description = $request->input('description', $timeEntry->description) ?? ''; $timeEntry->setComputedAttributeValue('billable_rate'); $timeEntry->save(); @@ -690,6 +699,7 @@ class TimeEntryController extends Controller */ public function updateMultiple(Organization $organization, TimeEntryUpdateMultipleRequest $request): JsonResponse { + $member = $this->member($organization); $this->checkAnyPermission($organization, ['time-entries:update:all', 'time-entries:update:own']); $canAccessAll = $this->hasPermission($organization, 'time-entries:update:all'); @@ -714,6 +724,9 @@ class TimeEntryController extends Controller throw new AuthorizationException; } + /** @var Member|null $newMember */ + $newMember = isset($changes['member_id']) ? Member::query()->findOrFail($changes['member_id']) : null; + $project = null; $client = null; $overwriteClient = false; @@ -740,7 +753,7 @@ class TimeEntryController extends Controller continue; } - if (! $canAccessAll && $timeEntry->user_id !== Auth::id()) { + if (! $canAccessAll && $timeEntry->member_id !== $member->getKey()) { $error->push($id); continue; @@ -750,6 +763,10 @@ class TimeEntryController extends Controller $oldTask = $timeEntry->task; $timeEntry->fill($changes); + if ($newMember !== null) { + $timeEntry->member()->associate($newMember); + $timeEntry->user_id = $newMember->user_id; + } // If project is changed, but task is not, we remove the old task from the time entry if ($oldProject !== null && $project !== null && $oldProject->isNot($project) && $task === null) { $timeEntry->task()->disassociate(); @@ -790,7 +807,8 @@ class TimeEntryController extends Controller */ public function destroy(Organization $organization, TimeEntry $timeEntry): JsonResponse { - if ($timeEntry->member->user_id === Auth::id()) { + $member = $this->member($organization); + if ($timeEntry->member_id === $member->getKey()) { $this->checkPermission($organization, 'time-entries:delete:own', $timeEntry); } else { $this->checkPermission($organization, 'time-entries:delete:all', $timeEntry); @@ -847,7 +865,7 @@ class TimeEntryController extends Controller continue; } - if (! $canDeleteAll && $timeEntry->user_id !== Auth::id()) { + if (! $canDeleteAll && $timeEntry->member_id !== $this->member($organization)->getKey()) { $error->push($id); continue; diff --git a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php index 1dc38a8b..cbd2b49f 100644 --- a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php @@ -92,6 +92,30 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract $response->assertJsonPath('data.0.id', $timeEntry->getKey()); } + public function test_index_endpoint_filters_by_member_id_instead_of_legacy_user_id(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:view:own', + ]); + $legacyUser = User::factory()->create(); + $timeEntry = TimeEntry::factory()->forMember($data->member)->create([ + 'user_id' => $legacyUser->getKey(), + ]); + Passport::actingAs($data->user); + + // Act + $response = $this->getJson(route('api.v1.time-entries.index', [ + $data->organization->getKey(), + 'member_id' => $data->member->getKey(), + ])); + + // Assert + $this->assertResponseCode($response, 200); + $response->assertJsonCount(1, 'data'); + $response->assertJsonPath('data.0.id', $timeEntry->getKey()); + } + public function test_index_endpoint_fails_if_user_filter_is_from_different_organization(): void { // Arrange @@ -126,7 +150,10 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract Passport::actingAs($data->user); // Act - $response = $this->getJson(route('api.v1.time-entries.index', [$data->organization->getKey(), 'user_id' => $user->getKey()])); + $response = $this->getJson(route('api.v1.time-entries.index', [ + $data->organization->getKey(), + 'member_id' => $member->getKey(), + ])); // Assert $this->assertResponseCode($response, 200); @@ -1772,6 +1799,29 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract ]); } + public function test_aggregate_endpoint_filters_by_member_id_instead_of_legacy_user_id(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:view:own', + ]); + $legacyUser = User::factory()->create(); + TimeEntry::factory()->forMember($data->member)->startWithDuration(Carbon::now(), 100)->create([ + 'user_id' => $legacyUser->getKey(), + ]); + Passport::actingAs($data->user); + + // Act + $response = $this->getJson(route('api.v1.time-entries.aggregate', [ + $data->organization->getKey(), + 'member_id' => $data->member->getKey(), + ])); + + // Assert + $response->assertSuccessful(); + $response->assertJsonPath('data.seconds', 100); + } + public function test_aggregate_endpoint_groups_by_two_groups(): void { // Arrange @@ -2819,6 +2869,32 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract ]); } + public function test_update_endpoint_updates_user_id_when_member_id_changes(): 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()->forMember($data->member)->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->assertValid(); + $this->assertResponseCode($response, 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 @@ -3155,6 +3231,40 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract ]); } + public function test_destroy_multiple_uses_member_id_for_own_permission_checks(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:delete:own', + ]); + $otherUser = User::factory()->create(); + $otherMember = Member::factory()->forOrganization($data->organization)->forUser($otherUser)->role(Role::Employee)->create(); + $timeEntry = TimeEntry::factory()->forMember($otherMember)->create([ + 'user_id' => $data->user->getKey(), + ]); + Passport::actingAs($data->user); + + // Act + $response = $this->deleteJson(route('api.v1.time-entries.destroy-multiple', [$data->organization->getKey()]), [ + 'ids' => [ + $timeEntry->getKey(), + ], + ]); + + // Assert + $response->assertValid(); + $this->assertResponseCode($response, 200); + $response->assertExactJson([ + 'success' => [], + 'error' => [ + $timeEntry->getKey(), + ], + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $timeEntry->getKey(), + ]); + } + public function test_destroy_multiple_deletes_all_time_entries_and_fails_for_time_entries_of_other_users_and_and_other_organizations_with_all_time_entries_permission(): void { // Arrange @@ -3566,6 +3676,46 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract ]); } + public function test_update_multiple_uses_member_id_for_own_permission_checks(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + 'projects:view:all', + ]); + $otherUser = User::factory()->create(); + $otherMember = Member::factory()->forOrganization($data->organization)->forUser($otherUser)->role(Role::Employee)->create(); + $timeEntry = TimeEntry::factory()->forMember($otherMember)->create([ + 'user_id' => $data->user->getKey(), + ]); + $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' => [ + $timeEntry->getKey(), + ], + 'changes' => [ + 'description' => $timeEntriesFake->description, + ], + ]); + + // Assert + $response->assertValid(); + $this->assertResponseCode($response, 200); + $response->assertExactJson([ + 'success' => [], + 'error' => [ + $timeEntry->getKey(), + ], + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $timeEntry->getKey(), + 'description' => $timeEntry->description, + ]); + } + public function test_update_multiple_updates_sets_description_to_empty_if_the_client_sends_null(): void { // Arrange @@ -3612,6 +3762,51 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract ]); } + public function test_update_multiple_updates_user_id_when_member_id_changes(): 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(); + $timeEntry1 = TimeEntry::factory()->forMember($data->member)->create(); + $timeEntry2 = 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' => [ + $timeEntry1->getKey(), + $timeEntry2->getKey(), + ], + 'changes' => [ + 'member_id' => $otherMember->getKey(), + ], + ]); + + // Assert + $response->assertValid(); + $response->assertStatus(200); + $response->assertExactJson([ + 'success' => [ + $timeEntry1->getKey(), + $timeEntry2->getKey(), + ], + 'error' => [], + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $timeEntry1->getKey(), + 'member_id' => $otherMember->getKey(), + 'user_id' => $otherUser->getKey(), + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $timeEntry2->getKey(), + 'member_id' => $otherMember->getKey(), + 'user_id' => $otherUser->getKey(), + ]); + } + 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