From d5a4df738fe750c1d08a97f33a6a83a4e8bcff18 Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Tue, 8 Oct 2024 19:15:16 +0200 Subject: [PATCH] Fix bug in time-entry.update-multiple; Add computed property for client_id --- .../Api/V1/TimeEntryController.php | 4 ++ app/Models/TimeEntry.php | 40 +++++++++++++++ .../Endpoint/Api/V1/TimeEntryEndpointTest.php | 49 ++++++++++++++++++- tests/Unit/Model/TimeEntryModelTest.php | 31 ++++++++++++ 4 files changed, 123 insertions(+), 1 deletion(-) diff --git a/app/Http/Controllers/Api/V1/TimeEntryController.php b/app/Http/Controllers/Api/V1/TimeEntryController.php index 1bd74d2d..61bdf76f 100644 --- a/app/Http/Controllers/Api/V1/TimeEntryController.php +++ b/app/Http/Controllers/Api/V1/TimeEntryController.php @@ -373,6 +373,10 @@ class TimeEntryController extends Controller $oldTask = $timeEntry->task; $timeEntry->fill($changes); + // 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(); + } if ($overwriteClient) { $timeEntry->client()->associate($client); } diff --git a/app/Models/TimeEntry.php b/app/Models/TimeEntry.php index 9412ab60..b60b659c 100644 --- a/app/Models/TimeEntry.php +++ b/app/Models/TimeEntry.php @@ -13,6 +13,7 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; +use Illuminate\Database\Eloquent\Relations\Relation; use Illuminate\Support\Carbon; use Korridor\LaravelComputedAttributes\ComputedAttributes; use OwenIt\Auditing\Contracts\Auditable as AuditableContract; @@ -79,6 +80,7 @@ class TimeEntry extends Model implements AuditableContract */ protected array $computed = [ 'billable_rate', + 'client_id', ]; /** @@ -95,6 +97,44 @@ class TimeEntry extends Model implements AuditableContract return app(BillableRateService::class)->getBillableRateForTimeEntry($this); } + public function getClientIdComputed(): ?string + { + return $this->project_id === null ? null : $this->project->client_id; + } + + /** + * This scope will be applied during the computed property generation with artisan computed-attributes:generate. + * + * @param Builder $builder + * @param array $attributes Attributes that will be generated. + * @return Builder + */ + public function scopeComputedAttributesGenerate(Builder $builder, array $attributes): Builder + { + if (in_array('client_id', $attributes, true)) { + $builder->with([ + 'project' => function (Relation $builder): void { + /** @var Builder $builder */ + $builder->select('id', 'client_id'); + }, + ]); + } + + return $builder; + } + + /** + * This scope will be applied during the computed property validation with artisan computed-attributes:validate. + * + * @param Builder $builder + * @param array $attributes Attributes that will be validated. + * @return Builder + */ + public function scopeComputedAttributesValidate(Builder $builder, array $attributes): Builder + { + return $this->scopeComputedAttributesGenerate($builder, $attributes); + } + public function getDuration(): ?CarbonInterval { return $this->end === null ? null : $this->start->diffAsCarbonInterval($this->end); diff --git a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php index c5bc5884..6209a797 100644 --- a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php @@ -1883,6 +1883,53 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract $response->assertForbidden(); } + public function test_update_multiple_remove_task_from_time_entries_only_if_project_is_set_to_a_new_value_without_setting_a_new_task(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $project1 = Project::factory()->forOrganization($data->organization)->create(); + $project2 = Project::factory()->forOrganization($data->organization)->create(); + $task1 = Task::factory()->forProject($project1)->forOrganization($data->organization)->create(); + $task2 = Task::factory()->forProject($project2)->forOrganization($data->organization)->create(); + $timeEntry1 = TimeEntry::factory()->forOrganization($data->organization)->forProject($project1)->forTask($task1)->forMember($data->member)->create(); + $timeEntry2 = TimeEntry::factory()->forOrganization($data->organization)->forProject($project2)->forTask($task2)->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' => [ + 'project_id' => $project2->getKey(), + ], + ]); + + // Assert + $response->assertValid(); + $response->assertStatus(200); + $response->assertExactJson([ + 'success' => [ + $timeEntry1->getKey(), + $timeEntry2->getKey(), + ], + 'error' => [], + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $timeEntry1->getKey(), + 'project_id' => $project2->getKey(), + 'task_id' => null, + ]); + $this->assertDatabaseHas(TimeEntry::class, [ + 'id' => $timeEntry2->getKey(), + 'project_id' => $project2->getKey(), + 'task_id' => $task2->getKey(), + ]); + } + 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 @@ -2096,7 +2143,7 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract Passport::actingAs($data->user); // Act - $response = $this->patchJson(route('api.v1.time-entries.update-multiple', [$data->organization->getKey()]), [ + $response = $this->withoutExceptionHandling()->patchJson(route('api.v1.time-entries.update-multiple', [$data->organization->getKey()]), [ 'ids' => [ $ownTimeEntry->getKey(), $otherTimeEntry->getKey(), diff --git a/tests/Unit/Model/TimeEntryModelTest.php b/tests/Unit/Model/TimeEntryModelTest.php index 48826896..a790c429 100644 --- a/tests/Unit/Model/TimeEntryModelTest.php +++ b/tests/Unit/Model/TimeEntryModelTest.php @@ -148,4 +148,35 @@ class TimeEntryModelTest extends ModelTestAbstract $this->assertCount(1, $result); $this->assertTrue($result->first()->is($timeEntry1)); } + + public function test_computed_client_id_returns_null_when_no_project_is_assigned(): void + { + // Arrange + $timeEntry = TimeEntry::factory()->forProject(null)->create(); + $timeEntry->client_id = null; + $timeEntry->save(); + + // Act + $timeEntry->setComputedAttributeValue('client_id'); + $clientId = $timeEntry->client_id; + + // Assert + $this->assertNull($clientId); + } + + public function test_computed_client_id_returns_project_client_id(): void + { + // Arrange + $project = Project::factory()->create(); + $timeEntry = TimeEntry::factory()->forProject($project)->create(); + $timeEntry->client_id = null; + $timeEntry->save(); + + // Act + $timeEntry->setComputedAttributeValue('client_id'); + $clientId = $timeEntry->client_id; + + // Assert + $this->assertSame($project->client_id, $clientId); + } }