Detect time entries with a task but no project in consistency check

The comparison used != which evaluates to NULL when the time entry has no
project, so entries that kept their task after the project was removed
were not reported.

app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php

@@ -39,7 +39,7 @@ class SelfHostDatabaseConsistency extends Command
         $problems = DB::table('time_entries')
             ->select(['time_entries.id as id'])
             ->join('tasks', 'time_entries.task_id', '=', 'tasks.id')
-            ->where('tasks.project_id', '!=', DB::raw('time_entries.project_id'))
+            ->whereRaw('tasks.project_id is distinct from time_entries.project_id')
             ->get();
         $this->logProblems($problems, 'Time entries have a task that does not belong to the project of the time entry', $hadAProblem);
 

app/Http/Controllers/Api/V1/TimeEntryController.php

@@ -679,6 +679,10 @@ class TimeEntryController extends Controller
             $timeEntry->member()->associate($newMember);
             $timeEntry->user()->associate($newMember->user);
         }
+        // If project is changed, but task is not, we remove the old task from the time entry
+        if ($request->has('project_id') && ! $request->has('task_id') && $oldTask !== null && $oldTask->project_id !== $project?->getKey()) {
+            $timeEntry->task()->disassociate();
+        }
         $timeEntry->description = $request->input('description', $timeEntry->description) ?? '';
         $timeEntry->setComputedAttributeValue('billable_rate');
         $timeEntry->save();
@@ -790,7 +794,7 @@ class TimeEntryController extends Controller
                 $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) {
+            if ($request->has('changes.project_id') && ! $request->has('changes.task_id') && $oldTask !== null && $oldTask->project_id !== $project?->getKey()) {
                 $timeEntry->task()->disassociate();
             }
             if ($overwriteClient) {

app/Service/Dto/ReportPropertiesDto.php

@@ -117,13 +117,13 @@ class ReportPropertiesDto implements Castable
                 $dto->end = $data->end !== null ? Carbon::createFromFormat('Y-m-d\TH:i:s\Z', $data->end) : null;
                 $dto->start = $data->start !== null ? Carbon::createFromFormat('Y-m-d\TH:i:s\Z', $data->start) : null;
                 $dto->active = $data->active;
-                $dto->setMemberIds($data->memberIds !== null ? (array) $data->memberIds : null);
+                $dto->memberIds = $data->memberIds !== null ? ReportPropertiesDto::idArrayToCollection($data->memberIds) : null;
                 $dto->billable = $data->billable;
-                $dto->setClientIds($data->clientIds !== null ? (array) $data->clientIds : null);
-                $dto->setProjectIds($data->projectIds !== null ? (array) $data->projectIds : null);
-                $dto->setTagIds($data->tagIds !== null ? (array) $data->tagIds : null);
+                $dto->clientIds = $data->clientIds !== null ? ReportPropertiesDto::idArrayToCollection($data->clientIds) : null;
+                $dto->projectIds = $data->projectIds !== null ? ReportPropertiesDto::idArrayToCollection($data->projectIds) : null;
+                $dto->tagIds = $data->tagIds !== null ? ReportPropertiesDto::idArrayToCollection($data->tagIds) : null;
                 $dto->tagMatchType = isset($data->tagMatchType) ? TagMatchType::from($data->tagMatchType) : null;
-                $dto->setTaskIds($data->taskIds !== null ? (array) $data->taskIds : null);
+                $dto->taskIds = $data->taskIds ? ReportPropertiesDto::idArrayToCollection($data->taskIds) : null;
                 $dto->group = TimeEntryAggregationType::from($data->group);
                 $dto->subGroup = TimeEntryAggregationType::from($data->subGroup);
                 $dto->historyGroup = TimeEntryAggregationTypeInterval::from($data->historyGroup);
@@ -205,7 +205,7 @@ class ReportPropertiesDto implements Castable
      */
     public function setMemberIds(?array $memberIds): void
     {
-        $this->memberIds = $memberIds !== null && count($memberIds) > 0 ? ReportPropertiesDto::idArrayToCollection($memberIds) : null;
+        $this->memberIds = $memberIds !== null ? ReportPropertiesDto::idArrayToCollection($memberIds) : null;
     }
 
     /**
@@ -213,7 +213,7 @@ class ReportPropertiesDto implements Castable
      */
     public function setClientIds(?array $clientIds): void
     {
-        $this->clientIds = $clientIds !== null && count($clientIds) > 0 ? ReportPropertiesDto::idArrayToCollection($clientIds) : null;
+        $this->clientIds = $clientIds !== null ? ReportPropertiesDto::idArrayToCollection($clientIds) : null;
     }
 
     /**
@@ -221,7 +221,7 @@ class ReportPropertiesDto implements Castable
      */
     public function setProjectIds(?array $projectIds): void
     {
-        $this->projectIds = $projectIds !== null && count($projectIds) > 0 ? ReportPropertiesDto::idArrayToCollection($projectIds) : null;
+        $this->projectIds = $projectIds !== null ? ReportPropertiesDto::idArrayToCollection($projectIds) : null;
     }
 
     /**
@@ -229,7 +229,7 @@ class ReportPropertiesDto implements Castable
      */
     public function setTagIds(?array $tagIds): void
     {
-        $this->tagIds = $tagIds !== null && count($tagIds) > 0 ? ReportPropertiesDto::idArrayToCollection($tagIds) : null;
+        $this->tagIds = $tagIds !== null ? ReportPropertiesDto::idArrayToCollection($tagIds) : null;
     }
 
     public function setTagMatchType(?TagMatchType $tagMatchType): void
@@ -242,6 +242,6 @@ class ReportPropertiesDto implements Castable
      */
     public function setTaskIds(?array $taskIds): void
     {
-        $this->taskIds = $taskIds !== null && count($taskIds) > 0 ? ReportPropertiesDto::idArrayToCollection($taskIds) : null;
+        $this->taskIds = $taskIds !== null ? ReportPropertiesDto::idArrayToCollection($taskIds) : null;
     }
 }

app/Service/TimeEntryFilter.php

@@ -111,8 +111,7 @@ class TimeEntryFilter
      */
     public function addMemberIdsFilter(?array $memberIds): self
     {
-        // An empty selection is no constraint, the same as null
-        if ($memberIds === null || count($memberIds) === 0) {
+        if ($memberIds === null) {
             return $this;
         }
         $this->builder->whereIn('member_id', $memberIds);
@@ -182,10 +181,6 @@ class TimeEntryFilter
         }
         $includeNone = in_array(self::NONE_VALUE, $clientIds, true);
         $clientIds = array_values(array_filter($clientIds, fn (string $id): bool => $id !== self::NONE_VALUE));
-        // An empty selection (no client IDs and not filtering for "none") is no constraint, so apply nothing
-        if (count($clientIds) === 0 && ! $includeNone) {
-            return $this;
-        }
 
         $this->builder->where(function (Builder $builder) use ($clientIds, $includeNone): void {
             if (count($clientIds) > 0) {
@@ -209,10 +204,6 @@ class TimeEntryFilter
         }
         $includeNone = in_array(self::NONE_VALUE, $projectIds, true);
         $projectIds = array_values(array_filter($projectIds, fn (string $id): bool => $id !== self::NONE_VALUE));
-        // An empty selection (no project IDs and not filtering for "none") is no constraint, so apply nothing
-        if (count($projectIds) === 0 && ! $includeNone) {
-            return $this;
-        }
 
         $this->builder->where(function (Builder $builder) use ($projectIds, $includeNone): void {
             if (count($projectIds) > 0) {
@@ -278,10 +269,6 @@ class TimeEntryFilter
         }
         $includeNone = in_array(self::NONE_VALUE, $taskIds, true);
         $taskIds = array_values(array_filter($taskIds, fn (string $id): bool => $id !== self::NONE_VALUE));
-        // An empty selection (no task IDs and not filtering for "none") is no constraint, so apply nothing
-        if (count($taskIds) === 0 && ! $includeNone) {
-            return $this;
-        }
 
         $this->builder->where(function (Builder $builder) use ($taskIds, $includeNone): void {
             if (count($taskIds) > 0) {

tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php

@@ -38,6 +38,26 @@ class SelfHostDatabaseConsistencyCommandTest extends TestCaseWithDatabase
         $this->assertSame("Consistency problem: Time entries have a task that does not belong to the project of the time entry\n  - ".$timeEntry->getKey()."\n", $output);
     }
 
+    public function test_checks_that_task_need_to_be_part_of_project_in_time_entries_with_no_project_in_time_entry(): void
+    {
+        // Arrange
+        $user = $this->createUserWithRole(Role::Owner);
+        $project = Project::factory()->forOrganization($user->organization)->create();
+        $task = Task::factory()->forOrganization($user->organization)->forProject($project)->create();
+        $timeEntry = TimeEntry::factory()->forMember($user->member)->forTask($task)->create([
+            'project_id' => null,
+            'client_id' => null,
+        ]);
+
+        // Act
+        $exitCode = $this->withoutMockingConsoleOutput()->artisan('self-host:database-consistency');
+
+        // Assert
+        $this->assertSame(Command::FAILURE, $exitCode);
+        $output = Artisan::output();
+        $this->assertSame("Consistency problem: Time entries have a task that does not belong to the project of the time entry\n  - ".$timeEntry->getKey()."\n", $output);
+    }
+
     public function test_checks_that_client_id_is_the_client_id_of_the_project(): void
     {
         // Arrange

tests/Unit/Endpoint/Api/V1/ReportEndpointTest.php

@@ -606,51 +606,6 @@ class ReportEndpointTest extends ApiEndpointTestAbstract
         );
     }
 
-    public function test_store_endpoint_stores_empty_id_filters_as_null(): void
-    {
-        // Arrange
-        $data = $this->createUserWithPermission([
-            'reports:create',
-        ]);
-        Passport::actingAs($data->user);
-
-        // Act
-        $response = $this->postJson(route('api.v1.reports.store', [$data->organization->getKey()]), [
-            'name' => 'Test Report with empty Filters',
-            'is_public' => false,
-            'properties' => [
-                'start' => Carbon::now()->subDays(30)->toIso8601ZuluString(),
-                'end' => Carbon::now()->toIso8601ZuluString(),
-                'group' => TimeEntryAggregationType::Project->value,
-                'sub_group' => TimeEntryAggregationType::Task->value,
-                'history_group' => TimeEntryAggregationType::Day->value,
-                'member_ids' => [],
-                'project_ids' => [],
-                'client_ids' => [],
-                'tag_ids' => [],
-                'task_ids' => [],
-            ],
-        ]);
-
-        // Assert
-        $response->assertStatus(201);
-        $response->assertJson(fn (AssertableJson $json) => $json
-            ->where('data.properties.member_ids', null)
-            ->where('data.properties.project_ids', null)
-            ->where('data.properties.client_ids', null)
-            ->where('data.properties.tag_ids', null)
-            ->where('data.properties.task_ids', null)
-            ->etc()
-        );
-        /** @var Report $report */
-        $report = Report::query()->findOrFail($response->json('data.id'));
-        $this->assertNull($report->properties->memberIds);
-        $this->assertNull($report->properties->projectIds);
-        $this->assertNull($report->properties->clientIds);
-        $this->assertNull($report->properties->tagIds);
-        $this->assertNull($report->properties->taskIds);
-    }
-
     public function test_store_endpoint_creates_report_with_none_filter_values(): void
     {
         // Arrange

tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php

@@ -2780,6 +2780,90 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract
         });
     }
 
+    public function test_update_endpoint_removes_task_if_project_is_changed_without_setting_a_new_task(): void
+    {
+        // Arrange
+        $data = $this->createUserWithPermission([
+            'time-entries:update:own',
+            'projects:view:all',
+        ]);
+        $project1 = Project::factory()->forOrganization($data->organization)->create();
+        $project2 = Project::factory()->forOrganization($data->organization)->create();
+        $task1 = Task::factory()->forProject($project1)->forOrganization($data->organization)->create();
+        $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forProject($project1)->forTask($task1)->forMember($data->member)->create();
+        Passport::actingAs($data->user);
+
+        // Act
+        $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [
+            'project_id' => $project2->getKey(),
+        ]);
+
+        // Assert
+        $response->assertValid();
+        $this->assertResponseCode($response, 200);
+        $response->assertJsonPath('data.project_id', $project2->getKey());
+        $response->assertJsonPath('data.task_id', null);
+        $this->assertDatabaseHas(TimeEntry::class, [
+            'id' => $timeEntry->getKey(),
+            'project_id' => $project2->getKey(),
+            'task_id' => null,
+        ]);
+    }
+
+    public function test_update_endpoint_removes_task_if_project_is_removed_without_removing_the_task(): void
+    {
+        // Arrange
+        $data = $this->createUserWithPermission([
+            'time-entries:update:own',
+            'projects:view:all',
+        ]);
+        $project = Project::factory()->forOrganization($data->organization)->create();
+        $task = Task::factory()->forProject($project)->forOrganization($data->organization)->create();
+        $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forProject($project)->forTask($task)->forMember($data->member)->create();
+        Passport::actingAs($data->user);
+
+        // Act
+        $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [
+            'project_id' => null,
+        ]);
+
+        // Assert
+        $response->assertValid();
+        $this->assertResponseCode($response, 200);
+        $this->assertDatabaseHas(TimeEntry::class, [
+            'id' => $timeEntry->getKey(),
+            'project_id' => null,
+            'task_id' => null,
+        ]);
+    }
+
+    public function test_update_endpoint_keeps_task_if_project_is_set_to_the_project_of_the_task(): void
+    {
+        // Arrange
+        $data = $this->createUserWithPermission([
+            'time-entries:update:own',
+            'projects:view:all',
+        ]);
+        $project = Project::factory()->forOrganization($data->organization)->create();
+        $task = Task::factory()->forProject($project)->forOrganization($data->organization)->create();
+        $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forProject($project)->forTask($task)->forMember($data->member)->create();
+        Passport::actingAs($data->user);
+
+        // Act
+        $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [
+            'project_id' => $project->getKey(),
+        ]);
+
+        // Assert
+        $response->assertValid();
+        $this->assertResponseCode($response, 200);
+        $this->assertDatabaseHas(TimeEntry::class, [
+            'id' => $timeEntry->getKey(),
+            'project_id' => $project->getKey(),
+            'task_id' => $task->getKey(),
+        ]);
+    }
+
     public function test_update_endpoint_fails_if_employee_tries_to_update_time_entry_to_private_project_without_access(): void
     {
         // Arrange
@@ -3807,6 +3891,44 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract
         ]);
     }
 
+    public function test_update_multiple_removes_task_from_time_entries_if_project_is_removed_without_removing_the_task(): void
+    {
+        // Arrange
+        $data = $this->createUserWithPermission([
+            'time-entries:update:own',
+            'projects:view:all',
+        ]);
+        $project = Project::factory()->forOrganization($data->organization)->create();
+        $task = Task::factory()->forProject($project)->forOrganization($data->organization)->create();
+        $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forProject($project)->forTask($task)->forMember($data->member)->create();
+        Passport::actingAs($data->user);
+
+        // Act
+        $response = $this->patchJson(route('api.v1.time-entries.update-multiple', [$data->organization->getKey()]), [
+            'ids' => [
+                $timeEntry->getKey(),
+            ],
+            'changes' => [
+                'project_id' => null,
+            ],
+        ]);
+
+        // Assert
+        $response->assertValid();
+        $this->assertResponseCode($response, 200);
+        $response->assertExactJson([
+            'success' => [
+                $timeEntry->getKey(),
+            ],
+            'error' => [],
+        ]);
+        $this->assertDatabaseHas(TimeEntry::class, [
+            'id' => $timeEntry->getKey(),
+            'project_id' => null,
+            'task_id' => null,
+        ]);
+    }
+
     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

tests/Unit/Service/Dto/ReportPropertiesDtoTest.php

@@ -88,25 +88,4 @@ class ReportPropertiesDtoTest extends TestCase
         $this->assertSame(TimeEntryType::Work, $dtoWork->timeEntryType);
         $this->assertSame(TimeEntryType::Break, $dtoBreak->timeEntryType);
     }
-
-    public function test_empty_id_filters_of_a_persisted_report_are_read_as_null(): void
-    {
-        // Arrange
-        $properties = $this->getBaseProperties();
-        $properties['memberIds'] = [];
-        $properties['clientIds'] = [];
-        $properties['projectIds'] = [];
-        $properties['tagIds'] = [];
-        $properties['taskIds'] = [];
-
-        // Act
-        $dto = $this->castFromJson($properties);
-
-        // Assert
-        $this->assertNull($dto->memberIds);
-        $this->assertNull($dto->clientIds);
-        $this->assertNull($dto->projectIds);
-        $this->assertNull($dto->tagIds);
-        $this->assertNull($dto->taskIds);
-    }
 }

tests/Unit/Service/TimeEntryFilterTest.php

@@ -416,81 +416,6 @@ class TimeEntryFilterTest extends TestCaseWithDatabase
         $this->assertCount(3, $builderContains->get());
     }
 
-    public function test_add_member_ids_filter_with_empty_array_applies_no_filter(): void
-    {
-        // Arrange
-        TimeEntry::factory()->createMany(2);
-
-        $builder = TimeEntry::query();
-        $filter = new TimeEntryFilter($builder);
-
-        // Act
-        $filter->addMemberIdsFilter([]);
-
-        // Assert
-        $this->assertCount(2, $builder->get());
-    }
-
-    public function test_add_project_ids_filter_with_empty_array_applies_no_filter(): void
-    {
-        // Arrange
-        $project = Project::factory()->create();
-        TimeEntry::factory()->create([
-            'project_id' => $project->getKey(),
-            'organization_id' => $project->organization_id,
-        ]);
-        TimeEntry::factory()->create(['project_id' => null]);
-
-        $builder = TimeEntry::query();
-        $filter = new TimeEntryFilter($builder);
-
-        // Act
-        $filter->addProjectIdsFilter([]);
-
-        // Assert
-        $this->assertCount(2, $builder->get());
-    }
-
-    public function test_add_task_ids_filter_with_empty_array_applies_no_filter(): void
-    {
-        // Arrange
-        $task = Task::factory()->create();
-        TimeEntry::factory()->create([
-            'task_id' => $task->getKey(),
-            'organization_id' => $task->organization_id,
-        ]);
-        TimeEntry::factory()->create(['task_id' => null]);
-
-        $builder = TimeEntry::query();
-        $filter = new TimeEntryFilter($builder);
-
-        // Act
-        $filter->addTaskIdsFilter([]);
-
-        // Assert
-        $this->assertCount(2, $builder->get());
-    }
-
-    public function test_add_client_ids_filter_with_empty_array_applies_no_filter(): void
-    {
-        // Arrange
-        $client = Client::factory()->create();
-        TimeEntry::factory()->create([
-            'client_id' => $client->getKey(),
-            'organization_id' => $client->organization_id,
-        ]);
-        TimeEntry::factory()->create(['client_id' => null]);
-
-        $builder = TimeEntry::query();
-        $filter = new TimeEntryFilter($builder);
-
-        // Act
-        $filter->addClientIdsFilter([]);
-
-        // Assert
-        $this->assertCount(2, $builder->get());
-    }
-
     public function test_add_tag_ids_filter_with_null_match_type_defaults_to_contains(): void
     {
         // Arrange