mirror of
https://github.com/solidtime-io/solidtime.git
synced 2026-10-14 00:33:17 +01:00
Prevent time entries with end before start
Partial updates of time entries only validated end against start if both were part of the payload. Start and end are now also validated against the persisted value if only one of them is sent (fixes #1189). The importers now fail if the end of a time entry is before the start and the database consistency check reports such time entries.
This commit is contained in:
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user