Fixed update of member_id in time_entries.update and time_entries.updateMultiple

Removed usage of legacy user_id in TimeEntryController

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`.
+     * 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 user ID in the user_id filter.
+     * 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<TimeEntryResource>
      *
@@ -118,16 +118,17 @@ class TimeEntryController extends Controller
      */
     public function index(Organization $organization, TimeEntryIndexRequest $request): JsonResource
     {
-        /** @var Member|null $member */
+        $member = $this->member($organization);
-        $member = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null;
+        /** @var Member|null $memberFilter */
-        if ($member !== null && $member->user_id === Auth::id()) {
+        $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 = $this->member($organization);
-        $member = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null;
+        /** @var Member|null $memberFilter */
-        if ($member !== null && $member->user_id === Auth::id()) {
+        $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 = $this->member($organization);
-        $member = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null;
+        /** @var Member|null $memberFilter */
-        if ($member !== null && $member->user_id === Auth::id()) {
+        $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 = $this->member($organization);
-        $member = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null;
+        /** @var Member|null $memberFilter */
-        if ($member !== null && $member->user_id === Auth::id()) {
+        $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 = $this->member($organization);
-        $member = $request->has('member_id') ? Member::query()->findOrFail($request->input('member_id')) : null;
+        /** @var Member|null $newMember */
-        if ($timeEntry->member->user_id === Auth::id() && ($member === null || $member->user_id === Auth::id())) {
+        $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;

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