diff --git a/app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php b/app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php index 88a11184..5c96eebd 100644 --- a/app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php +++ b/app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php @@ -59,6 +59,14 @@ class SelfHostDatabaseConsistency extends Command ->get(); $this->logProblems($problems, 'Time entries have a client but no project', $hadAProblem); + // End of time entries can not be before the start + $problems = DB::table('time_entries') + ->select(['id']) + ->whereNotNull('end') + ->whereColumn('end', '<', 'start') + ->get(); + $this->logProblems($problems, 'Time entries have an end that is before the start', $hadAProblem); + // Every user needs to be a member of at least one organization $problems = DB::table('users') ->select(['users.id as id']) diff --git a/app/Http/Requests/V1/TimeEntry/TimeEntryUpdateRequest.php b/app/Http/Requests/V1/TimeEntry/TimeEntryUpdateRequest.php index 92c84f32..c679d383 100644 --- a/app/Http/Requests/V1/TimeEntry/TimeEntryUpdateRequest.php +++ b/app/Http/Requests/V1/TimeEntry/TimeEntryUpdateRequest.php @@ -13,9 +13,11 @@ use App\Models\Tag; use App\Models\Task; use App\Models\TimeEntry; use App\Service\PermissionStore; +use Carbon\Exceptions\InvalidFormatException; use Closure; use Illuminate\Contracts\Validation\ValidationRule; use Illuminate\Database\Eloquent\Builder; +use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Auth; use Illuminate\Validation\ConditionalRules; use Illuminate\Validation\Rule; @@ -90,12 +92,32 @@ class TimeEntryUpdateRequest extends BaseFormRequest // Start of time entry (Format: "Y-m-d\TH:i:s\Z", UTC timezone, Example: "2000-02-22T14:58:59Z") 'start' => [ 'date_format:Y-m-d\TH:i:s\Z', + function (string $attribute, mixed $value, Closure $fail) use ($timeEntry): void { + // If the payload does not contain an end, the start needs to be validated against the persisted end + if ($this->has('end') || $timeEntry?->end === null) { + return; + } + $start = $this->parseDate($value); + if ($start !== null && $start->gt($timeEntry->end)) { + $fail('The start field must be a date before or equal to end.'); + } + }, ], // End of time entry (Format: "Y-m-d\TH:i:s\Z", UTC timezone, Example: "2000-02-22T14:58:59Z") 'end' => [ 'nullable', 'date_format:Y-m-d\TH:i:s\Z', 'after_or_equal:start', + function (string $attribute, mixed $value, Closure $fail) use ($timeEntry): void { + // If the payload does not contain a start, the end needs to be validated against the persisted start + if ($this->has('start') || $timeEntry === null) { + return; + } + $end = $this->parseDate($value); + if ($end !== null && $end->lt($timeEntry->start)) { + $fail('The end field must be a date after or equal to start.'); + } + }, ], // Whether time entry is billable 'billable' => [ @@ -137,4 +159,16 @@ class TimeEntryUpdateRequest extends BaseFormRequest ], ]; } + + private function parseDate(mixed $value): ?Carbon + { + if (! is_string($value)) { + return null; + } + try { + return Carbon::createFromFormat('Y-m-d\TH:i:s\Z', $value, 'UTC'); + } catch (InvalidFormatException) { + return null; + } + } } diff --git a/app/Service/Import/Importers/ClockifyTimeEntriesImporter.php b/app/Service/Import/Importers/ClockifyTimeEntriesImporter.php index f74919b2..7ec2d2be 100644 --- a/app/Service/Import/Importers/ClockifyTimeEntriesImporter.php +++ b/app/Service/Import/Importers/ClockifyTimeEntriesImporter.php @@ -190,6 +190,9 @@ class ClockifyTimeEntriesImporter extends DefaultImporter if ($end === null) { throw new ImportException('End date ("'.$endDateStr.'") or time ("'.$endTimeStr.'") are invalid'); } + if ($end->lt($start)) { + throw new ImportException('End ("'.$endStr.'") is before start ("'.$startStr.'")'); + } $timeEntry->end = $end->utc(); $timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations( diff --git a/app/Service/Import/Importers/GenericTimeEntriesImporter.php b/app/Service/Import/Importers/GenericTimeEntriesImporter.php index bb19ac0e..c0fe90b7 100644 --- a/app/Service/Import/Importers/GenericTimeEntriesImporter.php +++ b/app/Service/Import/Importers/GenericTimeEntriesImporter.php @@ -155,6 +155,9 @@ class GenericTimeEntriesImporter extends DefaultImporter if ($end === null) { throw new ImportException('Value of end ("'.$record['end'].'") is invalid'); } + if ($end->lt($start)) { + throw new ImportException('Value of end ("'.$record['end'].'") is before start ("'.$record['start'].'")'); + } $timeEntry->end = $end->utc(); $timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations( $timeEntry, diff --git a/app/Service/Import/Importers/HarvestTimeEntriesImporter.php b/app/Service/Import/Importers/HarvestTimeEntriesImporter.php index 57a66f49..77e965ef 100644 --- a/app/Service/Import/Importers/HarvestTimeEntriesImporter.php +++ b/app/Service/Import/Importers/HarvestTimeEntriesImporter.php @@ -136,6 +136,9 @@ class HarvestTimeEntriesImporter extends DefaultImporter throw new ImportException('Hours ("'.$record['Hours'].'") is invalid'); } $hours = (float) $hoursField; + if ($hours < 0) { + throw new ImportException('Hours ("'.$record['Hours'].'") is negative'); + } $timeEntry->start = $date->copy()->startOfDay()->utc(); $timeEntry->end = $date->copy()->startOfDay()->addHours($hours)->utc(); $timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations( diff --git a/app/Service/Import/Importers/SolidtimeImporter.php b/app/Service/Import/Importers/SolidtimeImporter.php index f6eade0e..7089d469 100644 --- a/app/Service/Import/Importers/SolidtimeImporter.php +++ b/app/Service/Import/Importers/SolidtimeImporter.php @@ -280,6 +280,9 @@ class SolidtimeImporter extends DefaultImporter if ($end === null) { throw new ImportException('End date ("'.$timeEntryRow['end'].'") is invalid'); } + if ($end->lt($start)) { + throw new ImportException('End date ("'.$timeEntryRow['end'].'") is before start date ("'.$timeEntryRow['start'].'")'); + } $timeEntry->end = $end->utc(); } else { $timeEntry->end = null; diff --git a/app/Service/Import/Importers/TogglTimeEntriesImporter.php b/app/Service/Import/Importers/TogglTimeEntriesImporter.php index e18cf606..c2f70843 100644 --- a/app/Service/Import/Importers/TogglTimeEntriesImporter.php +++ b/app/Service/Import/Importers/TogglTimeEntriesImporter.php @@ -139,6 +139,9 @@ class TogglTimeEntriesImporter extends DefaultImporter if ($end === null) { throw new ImportException('End date ("'.$record['End date'].'") or time ("'.$record['End time'].'") are invalid'); } + if ($end->lt($start)) { + throw new ImportException('End ("'.$record['End date'].' '.$record['End time'].'") is before start ("'.$record['Start date'].' '.$record['Start time'].'")'); + } $timeEntry->end = $end->utc(); $timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations( $timeEntry, diff --git a/tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php b/tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php index 26491860..b235d38a 100644 --- a/tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php +++ b/tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php @@ -13,6 +13,7 @@ use App\Models\Task; use App\Models\TimeEntry; use App\Models\User; use Illuminate\Console\Command; +use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Artisan; use PHPUnit\Framework\Attributes\CoversClass; use Tests\TestCaseWithDatabase; @@ -158,4 +159,26 @@ class SelfHostDatabaseConsistencyCommandTest extends TestCaseWithDatabase $output = Artisan::output(); $this->assertSame("Consistency problem: Users have a current organization that they are not a member of\n - ".$user1->user->getKey()."\n", $output); } + + public function test_checks_that_end_of_time_entries_is_not_before_start(): void + { + // Arrange + $user = $this->createUserWithRole(Role::Owner); + $timeEntry = TimeEntry::factory()->forMember($user->member)->create([ + 'start' => Carbon::parse('2026-08-01T03:00:00Z'), + 'end' => Carbon::parse('2026-08-01T02:59:55Z'), + ]); + TimeEntry::factory()->forMember($user->member)->create([ + 'start' => Carbon::parse('2026-08-01T04:00:00Z'), + 'end' => Carbon::parse('2026-08-01T04:00:00Z'), + ]); + + // 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 an end that is before the start\n - ".$timeEntry->getKey()."\n", $output); + } } diff --git a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php index 733693cb..fb32eaff 100644 --- a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php @@ -2961,6 +2961,101 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract ]); } + public function test_update_endpoint_validation_fails_if_only_end_is_sent_and_it_is_before_the_persisted_start(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->create([ + 'start' => Carbon::parse('2026-08-01T03:00:00Z'), + 'end' => Carbon::parse('2026-08-01T03:01:00Z'), + ]); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [ + 'end' => '2026-08-01T02:59:55Z', + ]); + + // Assert + $response->assertStatus(422); + $response->assertJsonValidationErrors([ + 'end' => 'The end field must be a date after or equal to start.', + ]); + $timeEntry->refresh(); + $this->assertSame('2026-08-01T03:01:00Z', $timeEntry->end->toIso8601ZuluString()); + } + + public function test_update_endpoint_validation_fails_if_only_start_is_sent_and_it_is_after_the_persisted_end(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->create([ + 'start' => Carbon::parse('2026-08-01T03:00:00Z'), + 'end' => Carbon::parse('2026-08-01T03:01:00Z'), + ]); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [ + 'start' => '2026-08-01T03:01:05Z', + ]); + + // Assert + $response->assertStatus(422); + $response->assertJsonValidationErrors([ + 'start' => 'The start field must be a date before or equal to end.', + ]); + $timeEntry->refresh(); + $this->assertSame('2026-08-01T03:00:00Z', $timeEntry->start->toIso8601ZuluString()); + } + + public function test_update_endpoint_allows_updating_only_start_of_running_time_entry(): 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()]), [ + 'start' => '2026-08-01T03:00:00Z', + ]); + + // Assert + $response->assertStatus(200); + $timeEntry->refresh(); + $this->assertSame('2026-08-01T03:00:00Z', $timeEntry->start->toIso8601ZuluString()); + } + + public function test_update_endpoint_allows_updating_only_end_if_it_is_after_the_persisted_start(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->create([ + 'start' => Carbon::parse('2026-08-01T03:00:00Z'), + 'end' => Carbon::parse('2026-08-01T03:01:00Z'), + ]); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [ + 'end' => '2026-08-01T03:00:00Z', + ]); + + // Assert + $response->assertStatus(200); + $timeEntry->refresh(); + $this->assertSame('2026-08-01T03:00:00Z', $timeEntry->end->toIso8601ZuluString()); + } + public function test_update_endpoint_validation_fails_if_project_id_is_missing_but_request_has_task_id(): void { // Arrange diff --git a/tests/Unit/Service/Import/Importers/ClockifyTimeEntriesImporterTest.php b/tests/Unit/Service/Import/Importers/ClockifyTimeEntriesImporterTest.php index 22d6d693..15be2f52 100644 --- a/tests/Unit/Service/Import/Importers/ClockifyTimeEntriesImporterTest.php +++ b/tests/Unit/Service/Import/Importers/ClockifyTimeEntriesImporterTest.php @@ -231,4 +231,29 @@ class ClockifyTimeEntriesImporterTest extends ImporterTestAbstract $this->assertSame(0, Tag::query()->count()); $this->assertSame(0, Client::query()->count()); } + + public function test_import_fails_if_end_is_before_start(): void + { + // Arrange + $organization = Organization::factory()->create(); + $timezone = 'Europe/Vienna'; + $importer = new ClockifyTimeEntriesImporter; + $importer->init($organization); + $data = <<<'CSV' + "Project","Client","Description","Task","User","Group","Email","Tags","Billable","Start Date","Start Time","End Date","End Time","Duration (h)","Duration (decimal)","Billable Rate (USD)","Billable Amount (USD)" + "Project","Client","Working hard","","Peter Tester","","peter.test@email.test","","Yes","03/04/2024","10:30:00 AM","03/04/2024","10:00:00 AM","00:30:00","0.50","0.00","0.00" + CSV; + + // Act + try { + $importer->importData($data, $timezone); + } catch (ImportException $e) { + // Assert + $this->assertSame('End ("03/04/2024 10:00:00 AM") is before start ("03/04/2024 10:30:00 AM")', $e->getMessage()); + $this->assertSame(0, TimeEntry::query()->count()); + + return; + } + $this->fail(); + } } diff --git a/tests/Unit/Service/Import/Importers/GenericTimeEntriesImporterTest.php b/tests/Unit/Service/Import/Importers/GenericTimeEntriesImporterTest.php index 69db995c..fb9e76fd 100644 --- a/tests/Unit/Service/Import/Importers/GenericTimeEntriesImporterTest.php +++ b/tests/Unit/Service/Import/Importers/GenericTimeEntriesImporterTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Tests\Unit\Service\Import\Importers; use App\Models\Organization; +use App\Models\TimeEntry; use App\Service\Import\Importers\DefaultImporter; use App\Service\Import\Importers\GenericTimeEntriesImporter; use App\Service\Import\Importers\ImportException; @@ -89,4 +90,27 @@ class GenericTimeEntriesImporterTest extends ImporterTestAbstract } $this->fail(); } + + public function test_import_fails_if_end_is_before_start(): void + { + // Arrange + $organization = Organization::factory()->create(); + $timezone = 'Europe/Vienna'; + $importer = new GenericTimeEntriesImporter; + $importer->init($organization); + $data = "description,billable,client,project,tags,start,end,task,user_name,user_email\n". + '"Working hard","true","Big Company","Project for Big Company","","2024-03-04T10:23:00Z","2024-03-04T09:23:00Z","","Peter Tester","peter.test@email.test"'; + + // Act + try { + $importer->importData($data, $timezone); + } catch (ImportException $e) { + // Assert + $this->assertSame('Value of end ("2024-03-04T09:23:00Z") is before start ("2024-03-04T10:23:00Z")', $e->getMessage()); + $this->assertSame(0, TimeEntry::query()->count()); + + return; + } + $this->fail(); + } } diff --git a/tests/Unit/Service/Import/Importers/HarvestTimeEntriesImporterTest.php b/tests/Unit/Service/Import/Importers/HarvestTimeEntriesImporterTest.php index c94f93c5..81f986b1 100644 --- a/tests/Unit/Service/Import/Importers/HarvestTimeEntriesImporterTest.php +++ b/tests/Unit/Service/Import/Importers/HarvestTimeEntriesImporterTest.php @@ -105,4 +105,27 @@ class HarvestTimeEntriesImporterTest extends ImporterTestAbstract $this->assertSame(2, $report->projectsCreated); $this->assertSame(1, $report->clientsCreated); } + + public function test_import_fails_if_hours_are_negative(): void + { + // Arrange + $organization = Organization::factory()->create(); + $timezone = 'Europe/Vienna'; + $importer = new HarvestTimeEntriesImporter; + $importer->init($organization); + $data = "Date,Client,Project,Project Code,Task,Notes,Hours,Billable?,Invoiced?,Approved?,First Name,Last Name,Roles,Employee?,Billable Rate,Billable Amount,Cost Rate,Cost Amount,Currency,External Reference URL\n". + '2024-03-04,,Project without Client,,,"","-2,0",No,No,No,Peter,Tester,,Yes,"100,0","2.000,0","0,0","0,0",Euro - EUR,'; + + // Act + try { + $importer->importData($data, $timezone); + } catch (ImportException $e) { + // Assert + $this->assertSame('Hours ("-2,0") is negative', $e->getMessage()); + $this->assertSame(0, TimeEntry::query()->count()); + + return; + } + $this->fail(); + } } diff --git a/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php b/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php index 3e93d675..dc390dc6 100644 --- a/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php +++ b/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php @@ -15,7 +15,9 @@ use App\Service\Import\Importers\SolidtimeImporter; use Exception; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Queue; +use Illuminate\Support\Str; use PHPUnit\Framework\Attributes\CoversClass; +use ZipArchive; #[CoversClass(SolidtimeImporter::class)] #[CoversClass(ImportException::class)] @@ -177,4 +179,32 @@ class SolidtimeImporterTest extends ImporterTestAbstract Queue::assertPushed(RecalculateSpentTimeForProject::class, 1); Queue::assertPushed(RecalculateSpentTimeForTask::class, 1); } + + public function test_import_fails_if_end_of_time_entry_is_before_start(): void + { + // Arrange + $organization = Organization::factory()->create(); + $timezone = 'Europe/Vienna'; + $zipPath = $this->createTestZip('solidtime_import_test_1'); + $zip = new ZipArchive; + $zip->open($zipPath); + $timeEntries = $zip->getFromName('time_entries.csv'); + $timeEntries = Str::replaceFirst(',2024-03-04T09:23:52Z,2024-03-04T09:23:52Z,', ',2024-03-04T09:23:52Z,2024-03-04T08:23:52Z,', $timeEntries); + $zip->addFromString('time_entries.csv', $timeEntries); + $zip->close(); + $importer = new SolidtimeImporter; + $importer->init($organization); + $data = file_get_contents($zipPath); + + // Act + try { + $importer->importData($data, $timezone); + } catch (ImportException $e) { + // Assert + $this->assertSame('End date ("2024-03-04T08:23:52Z") is before start date ("2024-03-04T09:23:52Z")', $e->getMessage()); + + return; + } + $this->fail(); + } } diff --git a/tests/Unit/Service/Import/Importers/TogglTimeEntriesImporterTest.php b/tests/Unit/Service/Import/Importers/TogglTimeEntriesImporterTest.php index 719bea69..61e9f11a 100644 --- a/tests/Unit/Service/Import/Importers/TogglTimeEntriesImporterTest.php +++ b/tests/Unit/Service/Import/Importers/TogglTimeEntriesImporterTest.php @@ -120,4 +120,27 @@ class TogglTimeEntriesImporterTest extends ImporterTestAbstract Queue::assertPushed(RecalculateSpentTimeForProject::class, 2); Queue::assertPushed(RecalculateSpentTimeForTask::class, 1); } + + public function test_import_fails_if_end_is_before_start(): void + { + // Arrange + $organization = Organization::factory()->create(); + $timezone = 'Europe/Vienna'; + $importer = new TogglTimeEntriesImporter; + $importer->init($organization); + $data = "User,Email,Client,Project,Task,Description,Billable,Start date,Start time,End date,End time,Duration,Tags,Amount (EUR)\n". + 'Peter Tester,peter.test@email.test,,Project without Client,,"",No,2024-03-04,10:23:52,2024-03-04,09:23:52,-01:00:00,"",'; + + // Act + try { + $importer->importData($data, $timezone); + } catch (ImportException $e) { + // Assert + $this->assertSame('End ("2024-03-04 09:23:52") is before start ("2024-03-04 10:23:52")', $e->getMessage()); + $this->assertSame(0, TimeEntry::query()->count()); + + return; + } + $this->fail(); + } }