From ded56c0798f626714f660a7983ad964ab2ebcdb8 Mon Sep 17 00:00:00 2001 From: Gregor Vostrak Date: Fri, 2 Oct 2026 14:20:47 +0200 Subject: [PATCH] add time entry filter normalizeation --- app/Service/Dto/ReportPropertiesDto.php | 20 ++--- app/Service/TimeEntryFilter.php | 15 +++- .../Endpoint/Api/V1/ReportEndpointTest.php | 45 +++++++++++ .../Service/Dto/ReportPropertiesDtoTest.php | 21 ++++++ tests/Unit/Service/TimeEntryFilterTest.php | 75 +++++++++++++++++++ 5 files changed, 165 insertions(+), 11 deletions(-) diff --git a/app/Service/Dto/ReportPropertiesDto.php b/app/Service/Dto/ReportPropertiesDto.php index 6f4ea450..fbbbc8a7 100644 --- a/app/Service/Dto/ReportPropertiesDto.php +++ b/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->memberIds = $data->memberIds !== null ? ReportPropertiesDto::idArrayToCollection($data->memberIds) : null; + $dto->setMemberIds($data->memberIds !== null ? (array) $data->memberIds : null); $dto->billable = $data->billable; - $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->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->tagMatchType = isset($data->tagMatchType) ? TagMatchType::from($data->tagMatchType) : null; - $dto->taskIds = $data->taskIds ? ReportPropertiesDto::idArrayToCollection($data->taskIds) : null; + $dto->setTaskIds($data->taskIds !== null ? (array) $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 ? ReportPropertiesDto::idArrayToCollection($memberIds) : null; + $this->memberIds = $memberIds !== null && count($memberIds) > 0 ? ReportPropertiesDto::idArrayToCollection($memberIds) : null; } /** @@ -213,7 +213,7 @@ class ReportPropertiesDto implements Castable */ public function setClientIds(?array $clientIds): void { - $this->clientIds = $clientIds !== null ? ReportPropertiesDto::idArrayToCollection($clientIds) : null; + $this->clientIds = $clientIds !== null && count($clientIds) > 0 ? ReportPropertiesDto::idArrayToCollection($clientIds) : null; } /** @@ -221,7 +221,7 @@ class ReportPropertiesDto implements Castable */ public function setProjectIds(?array $projectIds): void { - $this->projectIds = $projectIds !== null ? ReportPropertiesDto::idArrayToCollection($projectIds) : null; + $this->projectIds = $projectIds !== null && count($projectIds) > 0 ? ReportPropertiesDto::idArrayToCollection($projectIds) : null; } /** @@ -229,7 +229,7 @@ class ReportPropertiesDto implements Castable */ public function setTagIds(?array $tagIds): void { - $this->tagIds = $tagIds !== null ? ReportPropertiesDto::idArrayToCollection($tagIds) : null; + $this->tagIds = $tagIds !== null && count($tagIds) > 0 ? 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 ? ReportPropertiesDto::idArrayToCollection($taskIds) : null; + $this->taskIds = $taskIds !== null && count($taskIds) > 0 ? ReportPropertiesDto::idArrayToCollection($taskIds) : null; } } diff --git a/app/Service/TimeEntryFilter.php b/app/Service/TimeEntryFilter.php index 1f4b8541..950370c5 100644 --- a/app/Service/TimeEntryFilter.php +++ b/app/Service/TimeEntryFilter.php @@ -111,7 +111,8 @@ class TimeEntryFilter */ public function addMemberIdsFilter(?array $memberIds): self { - if ($memberIds === null) { + // An empty selection is no constraint, the same as null + if ($memberIds === null || count($memberIds) === 0) { return $this; } $this->builder->whereIn('member_id', $memberIds); @@ -181,6 +182,10 @@ 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) { @@ -204,6 +209,10 @@ 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) { @@ -269,6 +278,10 @@ 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) { diff --git a/tests/Unit/Endpoint/Api/V1/ReportEndpointTest.php b/tests/Unit/Endpoint/Api/V1/ReportEndpointTest.php index bf6b75c0..62cb72c8 100644 --- a/tests/Unit/Endpoint/Api/V1/ReportEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/ReportEndpointTest.php @@ -606,6 +606,51 @@ 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 diff --git a/tests/Unit/Service/Dto/ReportPropertiesDtoTest.php b/tests/Unit/Service/Dto/ReportPropertiesDtoTest.php index 7169c6de..e416643e 100644 --- a/tests/Unit/Service/Dto/ReportPropertiesDtoTest.php +++ b/tests/Unit/Service/Dto/ReportPropertiesDtoTest.php @@ -88,4 +88,25 @@ 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); + } } diff --git a/tests/Unit/Service/TimeEntryFilterTest.php b/tests/Unit/Service/TimeEntryFilterTest.php index 74e58416..857502f6 100644 --- a/tests/Unit/Service/TimeEntryFilterTest.php +++ b/tests/Unit/Service/TimeEntryFilterTest.php @@ -416,6 +416,81 @@ 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