From 0caea279bded538698c70b71aed85483d09ccad6 Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski Date: Tue, 15 Sep 2026 09:35:45 +0200 Subject: [PATCH 1/6] feat(api): give every task the time logged against it A task row that cannot say how long it took is not a row anyone can work from, so TaskResource grows a `time` block: minutes logged, the billable and still unbilled part of them, the money waiting for an invoice, whether the task has reached one, and the clocks running on it right now. TaskTimeSummary fills it in for a whole response at once through three grouped reads over the time entries rather than a query per row: one for logged minutes, one for the billable money with the unbilled split and the stamped count taken from a CASE that MySQL, PostgreSQL and SQLite all speak, and one for the running rows. Running entries stay outside the totals, because their duration is zero until the clock stops and the UI ticks them itself. --- app/Application/TaskTimeSummary.php | 215 +++++++++++++++++++++++ app/Http/Controllers/BoardController.php | 9 + app/Http/Resources/TaskResource.php | 8 + app/Models/Task.php | 10 ++ 4 files changed, 242 insertions(+) create mode 100644 app/Application/TaskTimeSummary.php diff --git a/app/Application/TaskTimeSummary.php b/app/Application/TaskTimeSummary.php new file mode 100644 index 0000000..2fb1974 --- /dev/null +++ b/app/Application/TaskTimeSummary.php @@ -0,0 +1,215 @@ + $tasks + */ + public function attach(int $companyId, iterable $tasks): void + { + $tasks = is_array($tasks) ? $tasks : iterator_to_array($tasks); + + if ($tasks === []) { + return; + } + + $summaries = $this->forTasks($companyId, array_map( + static fn (Task $task): int => (int) $task->id, + array_values($tasks), + )); + + foreach ($tasks as $task) { + $task->timeSummary = $summaries[(int) $task->id] ?? self::empty(); + } + } + + /** + * The summary of one task, for the rules that only need the state. + * + * @return array + */ + public function forTask(int $companyId, int $taskId): array + { + return $this->forTasks($companyId, [$taskId])[$taskId] ?? self::empty(); + } + + /** + * Three grouped reads, keyed by task id. + * + * @param list $taskIds + * @return array}> + */ + public function forTasks(int $companyId, array $taskIds): array + { + $taskIds = array_values(array_unique(array_map(intval(...), $taskIds))); + + if ($taskIds === []) { + return []; + } + + $summaries = []; + foreach ($taskIds as $taskId) { + $summaries[$taskId] = self::empty(); + } + + foreach ($this->loggedMinutes($companyId, $taskIds) as $row) { + $summaries[(int) $row->task_id]['logged_minutes'] = (int) $row->logged_minutes; + } + + foreach ($this->billableTotals($companyId, $taskIds) as $row) { + $summaries[(int) $row->task_id] = [ + ...$summaries[(int) $row->task_id], + 'billable_minutes' => (int) $row->billable_minutes, + 'unbilled_minutes' => (int) $row->unbilled_minutes, + 'unbilled_amount' => (int) $row->unbilled_amount, + 'invoiced' => self::stateFor((int) $row->billable_entries, (int) $row->stamped_entries), + ]; + } + + foreach ($this->runningEntries($companyId, $taskIds) as $entry) { + $summaries[(int) $entry->task_id]['running'][] = [ + 'entry_id' => (int) $entry->id, + 'user_id' => (int) $entry->user_id, + 'started_at' => $entry->started_at instanceof Carbon ? $entry->started_at->toIso8601String() : null, + ]; + } + + return $summaries; + } + + /** + * The shape a task with no time at all still answers with. + * + * @return array{logged_minutes: int, billable_minutes: int, unbilled_minutes: int, unbilled_amount: int, invoiced: string, running: list} + */ + public static function empty(): array + { + return [ + 'logged_minutes' => 0, + 'billable_minutes' => 0, + 'unbilled_minutes' => 0, + 'unbilled_amount' => 0, + 'invoiced' => self::NONE, + 'running' => [], + ]; + } + + /** + * Uninvoiced the moment one billable minute is unbilled, invoiced once + * every billable entry is stamped, and none while nothing billable exists. + */ + private static function stateFor(int $billableEntries, int $stampedEntries): string + { + if ($billableEntries === 0) { + return self::NONE; + } + + return $stampedEntries === $billableEntries ? self::INVOICED : self::UNINVOICED; + } + + /** + * Everything logged against the task, billable or not. + * + * @param list $taskIds + * @return Collection + */ + private function loggedMinutes(int $companyId, array $taskIds) + { + return $this->stopped($companyId, $taskIds) + ->selectRaw('task_id, SUM(duration_minutes) as logged_minutes') + ->groupBy('task_id') + ->get(); + } + + /** + * The billable side, split into what is still unbilled and how many entries + * carry an invoice, counted with a CASE all three databases understand. + * + * @param list $taskIds + * @return Collection + */ + private function billableTotals(int $companyId, array $taskIds) + { + return $this->stopped($companyId, $taskIds) + ->where('billable', true) + ->selectRaw(implode(', ', [ + 'task_id', + 'SUM(duration_minutes) as billable_minutes', + 'SUM(CASE WHEN invoice_id IS NULL THEN duration_minutes ELSE 0 END) as unbilled_minutes', + 'SUM(CASE WHEN invoice_id IS NULL THEN amount ELSE 0 END) as unbilled_amount', + 'COUNT(*) as billable_entries', + 'SUM(CASE WHEN invoice_id IS NULL THEN 0 ELSE 1 END) as stamped_entries', + ])) + ->groupBy('task_id') + ->get(); + } + + /** + * The clocks running on these tasks, whoever they belong to. + * + * Totals are open to anyone who may see the task, so the running rows here + * carry a user id and nothing else; the time log is where per-member + * visibility is decided. + * + * @param list $taskIds + * @return \Illuminate\Database\Eloquent\Collection + */ + private function runningEntries(int $companyId, array $taskIds) + { + return TimeEntry::query() + ->forCompany($companyId) + ->whereIn('task_id', $taskIds) + ->whereNotNull('running_user_id') + ->orderBy('started_at') + ->orderBy('id') + ->get(); + } + + /** + * @param list $taskIds + * @return Builder + */ + private function stopped(int $companyId, array $taskIds) + { + return TimeEntry::query() + ->forCompany($companyId) + ->whereIn('task_id', $taskIds) + ->whereNull('running_user_id'); + } +} diff --git a/app/Http/Controllers/BoardController.php b/app/Http/Controllers/BoardController.php index 8be6b55..381f74c 100644 --- a/app/Http/Controllers/BoardController.php +++ b/app/Http/Controllers/BoardController.php @@ -7,6 +7,7 @@ use Illuminate\Http\JsonResponse; use Modules\TasksProjects\Application\BoardQuery; use Modules\TasksProjects\Application\TaskStatusService; +use Modules\TasksProjects\Application\TaskTimeSummary; use Modules\TasksProjects\Http\Requests\BoardRequest; use Modules\TasksProjects\Http\Resources\TaskResource; use Modules\TasksProjects\Http\Resources\TaskStatusResource; @@ -19,6 +20,9 @@ * * A company that has never opened the board has no columns yet, so the four * defaults are created before the first read. + * + * The cards carry the same `time` block as the list, summarised for the whole + * board in one pass rather than per column. */ final class BoardController extends Controller { @@ -26,6 +30,7 @@ public function __construct( Authorizes $authorizes, private readonly BoardQuery $board, private readonly TaskStatusService $statuses, + private readonly TaskTimeSummary $summary, ) { parent::__construct($authorizes); } @@ -44,6 +49,10 @@ public function __invoke(BoardRequest $request): JsonResponse isset($filters['assignee_id']) ? (int) $filters['assignee_id'] : null, ); + $this->summary->attach($context->companyId, array_merge( + ...array_map(static fn (array $column): array => $column['tasks'], $columns), + )); + return response()->json(['data' => array_map(static fn (array $column): array => [ 'status' => TaskStatusResource::make($column['status'])->resolve($request), 'tasks' => TaskResource::collection($column['tasks'])->resolve($request), diff --git a/app/Http/Resources/TaskResource.php b/app/Http/Resources/TaskResource.php index c692e13..3de848a 100644 --- a/app/Http/Resources/TaskResource.php +++ b/app/Http/Resources/TaskResource.php @@ -6,6 +6,7 @@ use Illuminate\Http\Request; use Illuminate\Http\Resources\Json\JsonResource; +use Modules\TasksProjects\Application\TaskTimeSummary; use Modules\TasksProjects\Models\Task; /** @@ -13,6 +14,12 @@ * ordering survives JSON without a float rewriting it, and `rate` is an * override in minor units per hour. * + * `time` is the task's own clock: minutes logged, the billable and still + * unbilled part of them, whether the task has reached an invoice, and the + * timers running on it right now. It is filled in by TaskTimeSummary for the + * whole response at once, and answers zeros for a task nobody has logged time + * against. + * * @property-read Task $resource */ final class TaskResource extends JsonResource @@ -42,6 +49,7 @@ public function toArray(Request $request): array 'creator_id' => $task->creator_id === null ? null : (int) $task->creator_id, 'created_at' => $task->created_at?->toIso8601String(), 'updated_at' => $task->updated_at?->toIso8601String(), + 'time' => $task->timeSummary ?? TaskTimeSummary::empty(), ]; } } diff --git a/app/Models/Task.php b/app/Models/Task.php index d2f73bd..2f356cb 100644 --- a/app/Models/Task.php +++ b/app/Models/Task.php @@ -47,6 +47,16 @@ class Task extends Model self::PRIORITY_URGENT, ]; + /** + * The time block the API renders, hung on the model by TaskTimeSummary. + * + * A real property rather than an attribute: it is derived from the time + * entries, never a column, and must never travel back into a save(). + * + * @var array|null + */ + public ?array $timeSummary = null; + protected $table = 'tp_tasks'; protected $guarded = ['id']; From e0628cdfda4976172ee91de49abb1acad47c524b Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski Date: Tue, 15 Sep 2026 09:36:00 +0200 Subject: [PATCH 2/6] feat(api): start, stop and read the clock from the task itself A play button on a task row has to address that task, not "whatever is running": POST tasks/{id}/start opens the caller's clock on it and POST tasks/{id}/stop closes the clock only while it really runs there. Stopping the wrong task, or stopping with nothing running, is the same mistake to the caller, so both answer 409 timer_mismatch and the timer the user does have keeps running. A second start stays the 409 timer_already_running the timer endpoints already report, which is what the UI turns into "stop and start". The timer/* routes are untouched. GET tasks/{id}/time-log is the grid on the task page: running rows first, then newest first, capped rather than paged. Totals are open to anyone who may see the task, but the rows behind them follow the timesheet rule, so without view-all-time and with the company setting closed the caller sees their own time and nobody else's. --- app/Application/Exceptions/TimerMismatch.php | 25 +++ app/Application/TimerService.php | 27 +++ .../Controllers/TaskTimeLogController.php | 55 ++++++ app/Http/Controllers/TimerController.php | 33 ++++ app/Http/DomainExceptionRenderer.php | 2 + app/Http/Requests/StartTaskTimerRequest.php | 17 ++ tests/Feature/TaskTimeLogApiTest.php | 164 ++++++++++++++++++ tests/Feature/TimerApiTest.php | 129 ++++++++++++++ 8 files changed, 452 insertions(+) create mode 100644 app/Application/Exceptions/TimerMismatch.php create mode 100644 app/Http/Controllers/TaskTimeLogController.php create mode 100644 app/Http/Requests/StartTaskTimerRequest.php create mode 100644 tests/Feature/TaskTimeLogApiTest.php diff --git a/app/Application/Exceptions/TimerMismatch.php b/app/Application/Exceptions/TimerMismatch.php new file mode 100644 index 0000000..8c57432 --- /dev/null +++ b/app/Application/Exceptions/TimerMismatch.php @@ -0,0 +1,25 @@ +duration_minutes = Rounding::roundMinutes( max(0, (int) round($startedAt->diffInSeconds($endedAt, true) / 60)), $this->settings->roundingMinutes($companyId), + $this->settings->roundingDirection($companyId), ); $task = $this->tasks->findForCompany($companyId, (int) $entry->task_id); @@ -103,6 +105,31 @@ public function stop(int $companyId, int $userId): TimeEntry return $entry; } + /** + * Stop the clock the caller is running on one particular task. + * + * Stopping is addressed to a task rather than to "whatever is running", so + * a stale row or a second tab cannot stop a timer the user has since moved + * elsewhere. Nothing running and something else running are the same + * mismatch to the caller, who reloads the timer either way. + * + * @throws TimerMismatch when the caller's timer is not on this task + */ + public function stopOn(int $companyId, int $userId, int $taskId): TimeEntry + { + $task = $this->tasks->findForCompany($companyId, $taskId); + $running = $this->running($companyId, $userId); + + if ($running === null || (int) $running->task_id !== (int) $task->id) { + throw TimerMismatch::forTask( + (int) $task->id, + $running === null ? null : (int) $running->task_id, + ); + } + + return $this->stop($companyId, $userId); + } + /** Throw away the running entry without recording any time. */ public function discard(int $companyId, int $userId): void { diff --git a/app/Http/Controllers/TaskTimeLogController.php b/app/Http/Controllers/TaskTimeLogController.php new file mode 100644 index 0000000..0123749 --- /dev/null +++ b/app/Http/Controllers/TaskTimeLogController.php @@ -0,0 +1,55 @@ +context($request); + $this->authorize($context, Abilities::VIEW_TASK); + + $task = $this->tasks->findForCompany($context->companyId, $id); + + $entries = $this->entries->logForTask( + $context->companyId, + (int) $task->id, + $context->userId, + $this->canSeeAllTime($context, $this->settings), + ); + + return response()->json([ + 'data' => TimeEntryResource::collection($entries)->resolve($request), + ]); + } +} diff --git a/app/Http/Controllers/TimerController.php b/app/Http/Controllers/TimerController.php index 7d0885f..2a8c25c 100644 --- a/app/Http/Controllers/TimerController.php +++ b/app/Http/Controllers/TimerController.php @@ -7,6 +7,7 @@ use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Modules\TasksProjects\Application\TimerService; +use Modules\TasksProjects\Http\Requests\StartTaskTimerRequest; use Modules\TasksProjects\Http\Requests\StartTimerRequest; use Modules\TasksProjects\Http\Resources\TimeEntryResource; use Modules\TasksProjects\Support\Abilities; @@ -17,6 +18,13 @@ * * A second start is a conflict rather than a validation error, because the * first timer is still perfectly valid; the UI offers to stop it. + * + * The same timer is reachable two ways. `timer/start` and `timer/stop` name the + * task in the body and are what the timesheet and the header chip use; the + * `tasks/{id}/start` and `tasks/{id}/stop` pair below addresses the task in the + * URL, which is what a play button on a row or a card needs: it stops that task + * or nothing at all, so a stale row can never stop a clock the user has since + * moved elsewhere. */ final class TimerController extends Controller { @@ -61,6 +69,31 @@ public function stop(Request $request): TimeEntryResource return new TimeEntryResource($this->timer->stop($context->companyId, $context->userId)); } + /** Start the caller's clock on one task, straight from its row or card. */ + public function startOnTask(StartTaskTimerRequest $request, int $id): TimeEntryResource + { + $context = $this->context($request); + $this->authorize($context, Abilities::VIEW_TASK); + $this->authorize($context, Abilities::VIEW_OWN_TIME); + + return new TimeEntryResource($this->timer->start( + $context->companyId, + $context->userId, + $id, + $request->validated()['description'] ?? null, + )); + } + + /** Stop the caller's clock, but only while it is running on this task. */ + public function stopOnTask(Request $request, int $id): TimeEntryResource + { + $context = $this->context($request); + $this->authorize($context, Abilities::VIEW_TASK); + $this->authorize($context, Abilities::VIEW_OWN_TIME); + + return new TimeEntryResource($this->timer->stopOn($context->companyId, $context->userId, $id)); + } + /** Throw the running entry away without recording any time. */ public function destroy(Request $request): JsonResponse { diff --git a/app/Http/DomainExceptionRenderer.php b/app/Http/DomainExceptionRenderer.php index 21972d3..9aecf41 100644 --- a/app/Http/DomainExceptionRenderer.php +++ b/app/Http/DomainExceptionRenderer.php @@ -9,6 +9,7 @@ use Illuminate\Support\Str; use Modules\TasksProjects\Application\Exceptions\TasksProjectsException; use Modules\TasksProjects\Application\Exceptions\TimerAlreadyRunning; +use Modules\TasksProjects\Application\Exceptions\TimerMismatch; use Symfony\Component\HttpFoundation\Response; /** @@ -24,6 +25,7 @@ final class DomainExceptionRenderer /** Statuses that are not the 422 default. */ private const STATUSES = [ TimerAlreadyRunning::class => Response::HTTP_CONFLICT, + TimerMismatch::class => Response::HTTP_CONFLICT, ]; public static function register(Handler $handler): void diff --git a/app/Http/Requests/StartTaskTimerRequest.php b/app/Http/Requests/StartTaskTimerRequest.php new file mode 100644 index 0000000..11c563b --- /dev/null +++ b/app/Http/Requests/StartTaskTimerRequest.php @@ -0,0 +1,17 @@ +> */ + public function rules(): array + { + return [ + 'description' => ['sometimes', 'nullable', 'string'], + ]; + } +} diff --git a/tests/Feature/TaskTimeLogApiTest.php b/tests/Feature/TaskTimeLogApiTest.php new file mode 100644 index 0000000..355caf5 --- /dev/null +++ b/tests/Feature/TaskTimeLogApiTest.php @@ -0,0 +1,164 @@ +makeTask(self::COMPANY); + + $oldest = $this->makeEntry(self::COMPANY, (int) $task->id, [ + 'started_at' => Carbon::parse('2026-09-01 09:00:00'), + 'ended_at' => Carbon::parse('2026-09-01 10:00:00'), + ]); + $newest = $this->makeEntry(self::COMPANY, (int) $task->id, [ + 'started_at' => Carbon::parse('2026-09-03 09:00:00'), + 'ended_at' => Carbon::parse('2026-09-03 10:00:00'), + ]); + $middle = $this->makeEntry(self::COMPANY, (int) $task->id, [ + 'started_at' => Carbon::parse('2026-09-02 09:00:00'), + 'ended_at' => Carbon::parse('2026-09-02 10:00:00'), + ]); + // A clock started long before any of them still belongs at the top. + $running = $this->makeEntry(self::COMPANY, (int) $task->id, [ + 'started_at' => Carbon::parse('2026-08-01 09:00:00'), + 'ended_at' => null, + 'duration_minutes' => 0, + 'running_user_id' => self::DEFAULT_USER, + ]); + + $response = $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks/'.$task->id.'/time-log'); + + $response->assertOk(); + self::assertSame( + [(int) $running->id, (int) $newest->id, (int) $middle->id, (int) $oldest->id], + $response->json('data.*.id'), + ); + $response->assertJsonPath('data.0.is_running', true); + } + + public function test_the_log_only_carries_the_entries_of_its_own_task(): void + { + $task = $this->makeTask(self::COMPANY); + $other = $this->makeTask(self::COMPANY, ['name' => 'Something else']); + + $mine = $this->makeEntry(self::COMPANY, (int) $task->id); + $this->makeEntry(self::COMPANY, (int) $other->id); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks/'.$task->id.'/time-log') + ->assertOk() + ->assertJsonCount(1, 'data') + ->assertJsonPath('data.0.id', (int) $mine->id); + } + + public function test_the_log_is_capped_rather_than_paged(): void + { + $task = $this->makeTask(self::COMPANY); + $rows = TimeEntryService::LOG_LIMIT + 3; + + for ($minute = 0; $minute < $rows; $minute++) { + $this->makeEntry(self::COMPANY, (int) $task->id, [ + 'started_at' => Carbon::parse('2026-09-01 00:00:00')->addMinutes($minute), + 'ended_at' => Carbon::parse('2026-09-01 00:30:00')->addMinutes($minute), + 'duration_minutes' => 30, + ]); + } + + $response = $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks/'.$task->id.'/time-log'); + + $response->assertOk(); + $response->assertJsonCount(TimeEntryService::LOG_LIMIT, 'data'); + self::assertSame($rows, TimeEntry::query()->forCompany(self::COMPANY)->count()); + } + + public function test_without_view_all_time_the_log_is_the_callers_own_rows(): void + { + $task = $this->makeTask(self::COMPANY); + $mine = $this->makeEntry(self::COMPANY, (int) $task->id); + $theirs = $this->makeEntry(self::COMPANY, (int) $task->id, ['user_id' => self::OTHER_USER]); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks/'.$task->id.'/time-log') + ->assertOk() + ->assertJsonCount(2, 'data'); + + $this->authorization->deny(Authorizes::id(Abilities::VIEW_ALL_TIME)); + + $response = $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks/'.$task->id.'/time-log'); + + $response->assertOk(); + $response->assertJsonCount(1, 'data'); + $response->assertJsonPath('data.0.id', (int) $mine->id); + + // The totals on the task still count everybody's time. + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks/'.$task->id) + ->assertJsonPath('data.time.logged_minutes', 120); + + self::assertNotNull($theirs->id); + } + + public function test_the_company_setting_opens_the_log_without_the_ability(): void + { + $task = $this->makeTask(self::COMPANY); + $this->makeEntry(self::COMPANY, (int) $task->id); + $this->makeEntry(self::COMPANY, (int) $task->id, ['user_id' => self::OTHER_USER]); + + $this->authorization->deny(Authorizes::id(Abilities::VIEW_ALL_TIME)); + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'members_see_all_time', true); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks/'.$task->id.'/time-log') + ->assertOk() + ->assertJsonCount(2, 'data'); + } + + public function test_the_log_of_another_companys_task_is_not_found(): void + { + $task = $this->makeTask(self::OTHER_COMPANY); + $this->makeEntry(self::OTHER_COMPANY, (int) $task->id); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks/'.$task->id.'/time-log') + ->assertNotFound(); + } + + public function test_reading_the_log_needs_the_task_view_ability(): void + { + $task = $this->makeTask(self::COMPANY); + + $this->authorization->deny(Authorizes::id(Abilities::VIEW_TASK)); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks/'.$task->id.'/time-log') + ->assertForbidden(); + } +} diff --git a/tests/Feature/TimerApiTest.php b/tests/Feature/TimerApiTest.php index 1d6e618..19be0c1 100644 --- a/tests/Feature/TimerApiTest.php +++ b/tests/Feature/TimerApiTest.php @@ -5,6 +5,7 @@ namespace Modules\TasksProjects\Tests\Feature; use Illuminate\Support\Carbon; +use Modules\TasksProjects\Application\Rounding; use Modules\TasksProjects\Models\TimeEntry; use Modules\TasksProjects\Support\Abilities; use Modules\TasksProjects\Support\Authorizes; @@ -81,6 +82,25 @@ public function test_stopping_rounds_the_elapsed_time_and_freezes_the_rate(): vo ->assertExactJson(['data' => null]); } + public function test_stopping_follows_the_companys_rounding_direction(): void + { + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'rounding_minutes', 15); + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'rounding_direction', Rounding::UP); + $task = $this->taskOnProjectAt(6000); + + Carbon::setTestNow('2026-09-15 09:00:00'); + $this->asCompany(self::COMPANY)->postJson('/api/v1/tasks-projects/timer/start', ['task_id' => $task])->assertCreated(); + + Carbon::setTestNow('2026-09-15 09:50:00'); + + // Nearest would have billed 45 minutes; rounding up takes the hour. + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/timer/stop') + ->assertOk() + ->assertJsonPath('data.duration_minutes', 60) + ->assertJsonPath('data.amount', 6000); + } + public function test_the_stopped_entry_joins_the_timesheet(): void { $task = $this->taskOnProjectAt(6000); @@ -159,6 +179,115 @@ public function test_the_timer_needs_the_own_time_ability(): void $this->asCompany(self::COMPANY)->deleteJson('/api/v1/tasks-projects/timer')->assertForbidden(); } + public function test_a_task_row_starts_the_clock_on_that_task(): void + { + Carbon::setTestNow('2026-09-15 09:00:00'); + $task = $this->taskOnProjectAt(6000); + + $response = $this->asCompany(self::COMPANY)->postJson( + '/api/v1/tasks-projects/tasks/'.$task.'/start', + ['description' => 'Fixing the importer'], + ); + + $response->assertCreated(); + $response->assertJsonPath('data.task_id', $task); + $response->assertJsonPath('data.user_id', self::DEFAULT_USER); + $response->assertJsonPath('data.is_running', true); + $response->assertJsonPath('data.description', 'Fixing the importer'); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks/'.$task) + ->assertJsonPath('data.time.running.0.user_id', self::DEFAULT_USER); + } + + public function test_a_second_task_start_is_the_same_conflict_the_timer_reports(): void + { + $first = $this->taskOnProjectAt(6000); + $second = (int) $this->makeTask(self::COMPANY, ['name' => 'Something else'])->id; + + $this->asCompany(self::COMPANY)->postJson('/api/v1/tasks-projects/tasks/'.$first.'/start')->assertCreated(); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/'.$second.'/start') + ->assertStatus(409) + ->assertJsonPath('error', 'timer_already_running'); + + self::assertSame(1, TimeEntry::query()->forCompany(self::COMPANY)->whereNotNull('running_user_id')->count()); + } + + public function test_stopping_a_task_closes_the_clock_that_runs_on_it(): void + { + $task = $this->taskOnProjectAt(6000); + + Carbon::setTestNow('2026-09-15 09:00:00'); + $this->asCompany(self::COMPANY)->postJson('/api/v1/tasks-projects/tasks/'.$task.'/start')->assertCreated(); + + Carbon::setTestNow('2026-09-15 10:00:00'); + $response = $this->asCompany(self::COMPANY)->postJson('/api/v1/tasks-projects/tasks/'.$task.'/stop'); + + $response->assertOk(); + $response->assertJsonPath('data.is_running', false); + $response->assertJsonPath('data.duration_minutes', 60); + $response->assertJsonPath('data.amount', 6000); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks/'.$task) + ->assertJsonPath('data.time.logged_minutes', 60) + ->assertJsonPath('data.time.unbilled_amount', 6000) + ->assertJsonPath('data.time.running', []); + } + + public function test_stopping_the_wrong_task_is_a_mismatch_rather_than_a_stop(): void + { + $running = $this->taskOnProjectAt(6000); + $idle = (int) $this->makeTask(self::COMPANY, ['name' => 'Idle'])->id; + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/'.$idle.'/stop') + ->assertStatus(409) + ->assertJsonPath('error', 'timer_mismatch'); + + $this->asCompany(self::COMPANY)->postJson('/api/v1/tasks-projects/tasks/'.$running.'/start')->assertCreated(); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/'.$idle.'/stop') + ->assertStatus(409) + ->assertJsonPath('error', 'timer_mismatch'); + + // The clock the caller really had running is untouched. + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/timer') + ->assertJsonPath('data.task_id', $running); + } + + public function test_the_task_routes_stay_inside_the_company(): void + { + $theirs = (int) $this->makeTask(self::OTHER_COMPANY)->id; + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/'.$theirs.'/start') + ->assertNotFound(); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/'.$theirs.'/stop') + ->assertNotFound(); + } + + public function test_the_task_routes_need_both_the_task_and_the_own_time_ability(): void + { + $task = $this->taskOnProjectAt(6000); + + $this->authorization->deny(Authorizes::id(Abilities::VIEW_TASK)); + + $this->asCompany(self::COMPANY)->postJson('/api/v1/tasks-projects/tasks/'.$task.'/start')->assertForbidden(); + $this->asCompany(self::COMPANY)->postJson('/api/v1/tasks-projects/tasks/'.$task.'/stop')->assertForbidden(); + + $this->authorization->denied = [Authorizes::id(Abilities::VIEW_OWN_TIME)]; + + $this->asCompany(self::COMPANY)->postJson('/api/v1/tasks-projects/tasks/'.$task.'/start')->assertForbidden(); + $this->asCompany(self::COMPANY)->postJson('/api/v1/tasks-projects/tasks/'.$task.'/stop')->assertForbidden(); + } + private function taskOnProjectAt(int $rate): int { $project = $this->makeProject(self::COMPANY, ['customer_id' => 42, 'default_rate' => $rate, 'currency_id' => 3]); From b9963c6236b8755747fc1191a6399848cc1b0526 Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski Date: Tue, 15 Sep 2026 09:36:07 +0200 Subject: [PATCH 3/6] feat(settings): round up or down, and carry the new task settings Rounding to the nearest increment is one of three things a firm means by rounding, and the other two are the ones that show up in an engagement letter. Rounding::roundMinutes takes a direction: nearest keeps today's behaviour to the minute, up takes the whole increment, and down drops the part increment and may bill nothing, which is exactly what a firm asking to round down is asking for. The increments now offer 5 and 60 as well. The settings endpoint carries the rest of what the task-centric UI needs: auto_start_tasks, lock_invoiced_tasks, hide_invoiced_on_board and the six invoice line toggles. Every switch lives in one table in ModuleSettings, so the schema the host renders, the typed getters, the settings response and the data cleanup all read from the same list and cannot drift apart. --- app/Application/Rounding.php | 44 ++++++++++--- app/Http/Controllers/SettingsController.php | 16 +++-- app/Lifecycle/DataCleanup.php | 25 +++++-- app/Support/ModuleSettings.php | 72 ++++++++++++++++++++- lang/en/settings.php | 15 +++++ tests/Feature/SettingsApiTest.php | 48 ++++++++++++++ tests/Unit/RoundingTest.php | 36 ++++++++++- 7 files changed, 233 insertions(+), 23 deletions(-) diff --git a/app/Application/Rounding.php b/app/Application/Rounding.php index 77f5805..409d992 100644 --- a/app/Application/Rounding.php +++ b/app/Application/Rounding.php @@ -10,17 +10,33 @@ /** * Billing increments, applied when a time entry is saved rather than when it is * invoiced, so what the user sees on the timesheet is what gets billed. + * + * The direction is a company setting. `nearest` is the historic behaviour and + * stays the default; `up` and `down` are the two ways a firm that bills in + * blocks wants the block chosen, and neither reads as "nearest" to a client. */ final class Rounding { + public const NEAREST = 'nearest'; + + public const UP = 'up'; + + public const DOWN = 'down'; + + /** @var list */ + public const DIRECTIONS = [self::NEAREST, self::UP, self::DOWN]; + /** - * Round a duration to the nearest multiple of the increment. + * Round a duration to a multiple of the increment. * - * Zero stays zero, because an entry with no time is not worth an increment. - * Anything above zero but below one increment rounds up: a two minute call - * on a fifteen minute increment bills a quarter of an hour, never nothing. + * Zero stays zero whichever way the company rounds, because an entry with + * no time is not worth an increment. Rounding `nearest` then treats a spell + * shorter than one increment as a whole one: a two minute call on a fifteen + * minute increment bills a quarter of an hour, never nothing. Rounding + * `down` is the one direction that may answer zero for real work, which is + * exactly what a firm asking to round down is asking for. */ - public static function roundMinutes(int $minutes, int $increment): int + public static function roundMinutes(int $minutes, int $increment, string $direction = self::NEAREST): int { if (! in_array($increment, ModuleSettings::ROUNDING_INCREMENTS, true)) { throw new InvalidArgumentException( @@ -28,14 +44,22 @@ public static function roundMinutes(int $minutes, int $increment): int ); } - if ($minutes <= 0) { - return 0; + if (! in_array($direction, self::DIRECTIONS, true)) { + throw new InvalidArgumentException( + "Rounding direction {$direction} is not one of ".implode(', ', self::DIRECTIONS).'.', + ); } - if ($minutes < $increment) { - return $increment; + if ($minutes <= 0) { + return 0; } - return (int) round($minutes / $increment) * $increment; + return match ($direction) { + self::UP => (int) ceil($minutes / $increment) * $increment, + self::DOWN => intdiv($minutes, $increment) * $increment, + default => $minutes < $increment + ? $increment + : (int) round($minutes / $increment) * $increment, + }; } } diff --git a/app/Http/Controllers/SettingsController.php b/app/Http/Controllers/SettingsController.php index 637ad53..6be541e 100644 --- a/app/Http/Controllers/SettingsController.php +++ b/app/Http/Controllers/SettingsController.php @@ -29,11 +29,19 @@ public function __invoke(Request $request): JsonResponse $context = $this->context($request); $this->authorize($context, Abilities::VIEW_PROJECT); + $companyId = $context->companyId; + + $flags = []; + foreach (array_keys(ModuleSettings::FLAGS) as $key) { + $flags[$key] = $this->settings->flag($companyId, $key); + } + return response()->json(['data' => [ - 'default_rate' => $this->settings->defaultRate($context->companyId), - 'rounding_minutes' => $this->settings->roundingMinutes($context->companyId), - 'week_start' => $this->settings->weekStart($context->companyId), - 'members_see_all_time' => $this->settings->membersSeeAllTime($context->companyId), + 'default_rate' => $this->settings->defaultRate($companyId), + 'rounding_minutes' => $this->settings->roundingMinutes($companyId), + 'rounding_direction' => $this->settings->roundingDirection($companyId), + 'week_start' => $this->settings->weekStart($companyId), + ...$flags, 'rounding_increments' => ModuleSettings::ROUNDING_INCREMENTS, ]]); } diff --git a/app/Lifecycle/DataCleanup.php b/app/Lifecycle/DataCleanup.php index 4346415..2a57b47 100644 --- a/app/Lifecycle/DataCleanup.php +++ b/app/Lifecycle/DataCleanup.php @@ -6,24 +6,39 @@ use InvoiceShelf\Modules\Contracts\DataCleanup as DataCleanupContract; use InvoiceShelf\Modules\Contracts\Host\SettingsStore; +use Modules\TasksProjects\Support\ModuleSettings; /** Removes the module's per-company settings when the host asks to remove module data. */ final class DataCleanup implements DataCleanupContract { - /** Keys stored under `module.tasks-projects.` for each company. */ - private const SETTING_KEYS = [ + /** The keys that are not switches; the switches come from ModuleSettings. */ + private const VALUE_KEYS = [ 'default_rate', 'rounding_minutes', + 'rounding_direction', 'week_start', - 'members_see_all_time', ]; public function __construct(private readonly SettingsStore $settings) {} public function cleanup(): void { - foreach (self::SETTING_KEYS as $key) { - $this->settings->deleteCompanyForAll('module.tasks-projects.'.$key); + foreach (self::settingKeys() as $key) { + $this->settings->deleteCompanyForAll(ModuleSettings::PREFIX.$key); } } + + /** + * Every key stored under `module.tasks-projects.` for a company. + * + * The switches are read from the same table the getters and the settings + * schema use, so a new toggle is added in one place and is cleaned up here + * without anyone remembering to come back. + * + * @return list + */ + public static function settingKeys(): array + { + return [...self::VALUE_KEYS, ...array_keys(ModuleSettings::FLAGS)]; + } } diff --git a/app/Support/ModuleSettings.php b/app/Support/ModuleSettings.php index ec49150..c078676 100644 --- a/app/Support/ModuleSettings.php +++ b/app/Support/ModuleSettings.php @@ -5,6 +5,7 @@ namespace Modules\TasksProjects\Support; use InvoiceShelf\Modules\Contracts\Host\SettingsStore; +use Modules\TasksProjects\Application\Rounding; /** * Typed reader for the module's per-company settings. @@ -18,12 +19,28 @@ final class ModuleSettings public const PREFIX = 'module.tasks-projects.'; /** @var list */ - public const ROUNDING_INCREMENTS = [1, 6, 15, 30]; + public const ROUNDING_INCREMENTS = [1, 5, 6, 15, 30, 60]; public const DEFAULT_ROUNDING_MINUTES = 1; + public const DEFAULT_ROUNDING_DIRECTION = Rounding::NEAREST; + public const DEFAULT_WEEK_START = 1; + /** Every switch the module stores, with the value a company starts from. */ + public const FLAGS = [ + 'members_see_all_time' => false, + 'auto_start_tasks' => false, + 'lock_invoiced_tasks' => false, + 'hide_invoiced_on_board' => false, + 'invoice_project_heading' => false, + 'invoice_task_description' => true, + 'invoice_entry_dates' => true, + 'invoice_entry_times' => false, + 'invoice_entry_hours' => true, + 'invoice_entry_descriptions' => false, + ]; + public function __construct(private readonly SettingsStore $settings) {} /** Company default hourly rate, in minor units per hour. */ @@ -42,6 +59,14 @@ public function roundingMinutes(int $companyId): int return in_array($minutes, self::ROUNDING_INCREMENTS, true) ? $minutes : self::DEFAULT_ROUNDING_MINUTES; } + /** Which way the increment is taken: nearest, up or down. */ + public function roundingDirection(int $companyId): string + { + $direction = (string) $this->read($companyId, 'rounding_direction', self::DEFAULT_ROUNDING_DIRECTION); + + return in_array($direction, Rounding::DIRECTIONS, true) ? $direction : self::DEFAULT_ROUNDING_DIRECTION; + } + /** First day of the timesheet week, 0 (Sunday) through 6 (Saturday). */ public function weekStart(int $companyId): int { @@ -53,10 +78,51 @@ public function weekStart(int $companyId): int /** Whether members without the view-all-time ability still see other members' time. */ public function membersSeeAllTime(int $companyId): bool { - $value = $this->read($companyId, 'members_see_all_time', false); + return $this->flag($companyId, 'members_see_all_time'); + } + + /** Whether creating a task starts its creator's timer straight away. */ + public function autoStartTasks(int $companyId): bool + { + return $this->flag($companyId, 'auto_start_tasks'); + } + + /** Whether a fully invoiced task refuses edits, status moves and deletion. */ + public function lockInvoicedTasks(int $companyId): bool + { + return $this->flag($companyId, 'lock_invoiced_tasks'); + } + + /** Whether a fully invoiced task drops off the board. */ + public function hideInvoicedOnBoard(int $companyId): bool + { + return $this->flag($companyId, 'hide_invoiced_on_board'); + } + + /** + * The invoice line toggles, as the composer reads them. + * + * @return array{project_heading: bool, task_description: bool, entry_dates: bool, entry_times: bool, entry_hours: bool, entry_descriptions: bool} + */ + public function invoiceLineOptions(int $companyId): array + { + return [ + 'project_heading' => $this->flag($companyId, 'invoice_project_heading'), + 'task_description' => $this->flag($companyId, 'invoice_task_description'), + 'entry_dates' => $this->flag($companyId, 'invoice_entry_dates'), + 'entry_times' => $this->flag($companyId, 'invoice_entry_times'), + 'entry_hours' => $this->flag($companyId, 'invoice_entry_hours'), + 'entry_descriptions' => $this->flag($companyId, 'invoice_entry_descriptions'), + ]; + } + + /** One stored switch, read the way the host may have written it. */ + public function flag(int $companyId, string $key): bool + { + $value = $this->read($companyId, $key, self::FLAGS[$key] ?? false); if (is_string($value)) { - return in_array(strtoupper($value), ['YES', 'TRUE', '1'], true); + return in_array(strtoupper($value), ['YES', 'TRUE', '1', 'ON'], true); } return (bool) $value; diff --git a/lang/en/settings.php b/lang/en/settings.php index 53a36fe..9cb1538 100644 --- a/lang/en/settings.php +++ b/lang/en/settings.php @@ -2,9 +2,24 @@ return [ 'general_section' => 'General', + 'invoice_section' => 'Invoice lines', 'default_rate' => 'Default hourly rate', 'rounding_minutes' => 'Rounding increment (minutes)', + 'rounding_direction' => 'Rounding direction', + 'rounding_nearest' => 'Nearest', + 'rounding_up' => 'Up', + 'rounding_down' => 'Down', 'week_start' => 'Week starts on', 'members_see_all_time' => 'Members can see other members\' time', + 'auto_start_tasks' => 'Start the timer when a task is created', + 'lock_invoiced_tasks' => 'Lock tasks once they are invoiced', + 'hide_invoiced_on_board' => 'Hide invoiced tasks on the board', + + 'invoice_project_heading' => 'Show the project name above the task', + 'invoice_task_description' => 'Show the task description', + 'invoice_entry_dates' => 'Show the date of each time entry', + 'invoice_entry_times' => 'Show the start and end time of each entry', + 'invoice_entry_hours' => 'Show the hours of each entry', + 'invoice_entry_descriptions' => 'Show the description of each entry', ]; diff --git a/tests/Feature/SettingsApiTest.php b/tests/Feature/SettingsApiTest.php index d7981d5..1d71135 100644 --- a/tests/Feature/SettingsApiTest.php +++ b/tests/Feature/SettingsApiTest.php @@ -4,6 +4,7 @@ namespace Modules\TasksProjects\Tests\Feature; +use Modules\TasksProjects\Application\Rounding; use Modules\TasksProjects\Support\Abilities; use Modules\TasksProjects\Support\Authorizes; use Modules\TasksProjects\Support\ModuleSettings; @@ -21,12 +22,59 @@ public function test_a_company_that_has_never_saved_a_setting_gets_the_defaults( $response->assertExactJson(['data' => [ 'default_rate' => 0, 'rounding_minutes' => ModuleSettings::DEFAULT_ROUNDING_MINUTES, + 'rounding_direction' => Rounding::NEAREST, 'week_start' => ModuleSettings::DEFAULT_WEEK_START, 'members_see_all_time' => false, + 'auto_start_tasks' => false, + 'lock_invoiced_tasks' => false, + 'hide_invoiced_on_board' => false, + 'invoice_project_heading' => false, + 'invoice_task_description' => true, + 'invoice_entry_dates' => true, + 'invoice_entry_times' => false, + 'invoice_entry_hours' => true, + 'invoice_entry_descriptions' => false, 'rounding_increments' => ModuleSettings::ROUNDING_INCREMENTS, ]]); } + public function test_the_offered_increments_cover_the_way_firms_actually_bill(): void + { + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/settings') + ->assertOk() + ->assertJsonPath('data.rounding_increments', [1, 5, 6, 15, 30, 60]); + } + + public function test_the_new_switches_and_the_direction_come_back_as_stored(): void + { + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'rounding_direction', Rounding::UP); + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'auto_start_tasks', 'YES'); + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'lock_invoiced_tasks', 1); + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'hide_invoiced_on_board', true); + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'invoice_task_description', false); + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'invoice_entry_times', 'true'); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/settings') + ->assertOk() + ->assertJsonPath('data.rounding_direction', Rounding::UP) + ->assertJsonPath('data.auto_start_tasks', true) + ->assertJsonPath('data.lock_invoiced_tasks', true) + ->assertJsonPath('data.hide_invoiced_on_board', true) + ->assertJsonPath('data.invoice_task_description', false) + ->assertJsonPath('data.invoice_entry_times', true); + } + + public function test_a_direction_the_module_does_not_know_falls_back_to_nearest(): void + { + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'rounding_direction', 'sideways'); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/settings') + ->assertJsonPath('data.rounding_direction', Rounding::NEAREST); + } + public function test_stored_values_come_back_typed_whatever_the_host_wrote(): void { $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'default_rate', '12000'); diff --git a/tests/Unit/RoundingTest.php b/tests/Unit/RoundingTest.php index 7c3721f..85ac7d4 100644 --- a/tests/Unit/RoundingTest.php +++ b/tests/Unit/RoundingTest.php @@ -45,8 +45,42 @@ public function test_anything_above_zero_bills_at_least_one_increment(): void public function test_it_refuses_an_increment_the_settings_do_not_offer(): void { $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('Rounding increment 7 is not one of 1, 6, 15, 30.'); + $this->expectExceptionMessage('Rounding increment 7 is not one of 1, 5, 6, 15, 30, 60.'); Rounding::roundMinutes(10, 7); } + + public function test_rounding_up_takes_the_whole_increment_every_time(): void + { + self::assertSame(15, Rounding::roundMinutes(1, 15, Rounding::UP)); + self::assertSame(15, Rounding::roundMinutes(15, 15, Rounding::UP)); + self::assertSame(30, Rounding::roundMinutes(16, 15, Rounding::UP)); + self::assertSame(60, Rounding::roundMinutes(46, 15, Rounding::UP)); + self::assertSame(137, Rounding::roundMinutes(137, 1, Rounding::UP)); + } + + public function test_rounding_down_drops_the_part_increment_and_may_bill_nothing(): void + { + self::assertSame(0, Rounding::roundMinutes(14, 15, Rounding::DOWN)); + self::assertSame(15, Rounding::roundMinutes(15, 15, Rounding::DOWN)); + self::assertSame(15, Rounding::roundMinutes(29, 15, Rounding::DOWN)); + self::assertSame(120, Rounding::roundMinutes(137, 60, Rounding::DOWN)); + self::assertSame(137, Rounding::roundMinutes(137, 1, Rounding::DOWN)); + } + + public function test_nothing_logged_stays_nothing_billed_whichever_way_it_rounds(): void + { + foreach (Rounding::DIRECTIONS as $direction) { + self::assertSame(0, Rounding::roundMinutes(0, 30, $direction)); + self::assertSame(0, Rounding::roundMinutes(-5, 30, $direction)); + } + } + + public function test_it_refuses_a_direction_it_does_not_know(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Rounding direction sideways is not one of nearest, up, down.'); + + Rounding::roundMinutes(10, 15, 'sideways'); + } } From eac64b721fa115baf798710355277ba725614a23 Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski Date: Tue, 15 Sep 2026 09:36:16 +0200 Subject: [PATCH 4/6] feat(api): lock invoiced tasks, auto-start new ones, protect stamped time Three rules that all guard the same thing: what a client was already billed for. lock_invoiced_tasks, when a company turns it on, makes a fully invoiced task refuse an edit, a status move and a delete with 422 task_locked. The check lives in TaskLock so the rule is written once; a task that is only partly invoiced still moves, because the work is not finished. auto_start_tasks starts the creator's clock on a task they have just created, but only when they have no timer running. A creator who is already timing something keeps that timer, and the task is created either way. Independent fix: an invoiced time entry could have its duration, its billable flag and its task rewritten, and a change to the rounding setting could re-round it on any save, silently moving money an invoice had already recorded. A stamped entry now accepts a new description and nothing else, and refuses a real change to the protected fields; posting the row's own values back is not a change, so an edit form still works. The task list also gains ?invoiced=0|1, which asks the entries through exists subqueries rather than caching a state on the task. --- .../Exceptions/EntriesAlreadyInvoiced.php | 12 ++ app/Application/Exceptions/TaskLocked.php | 20 +++ app/Application/TaskLock.php | 36 ++++++ app/Application/TaskService.php | 56 ++++++++- app/Application/TimeEntryService.php | 114 +++++++++++++++++- app/Http/Controllers/TasksController.php | 86 +++++++++++-- app/Http/Requests/ListTasksRequest.php | 1 + tests/Feature/TimeEntriesApiTest.php | 89 ++++++++++++++ tests/TestCase.php | 19 +++ tests/Unit/TaskServiceTest.php | 10 +- tests/Unit/TimeEntryServiceTest.php | 7 +- tests/Unit/TimerServiceTest.php | 7 +- 12 files changed, 420 insertions(+), 37 deletions(-) create mode 100644 app/Application/Exceptions/TaskLocked.php create mode 100644 app/Application/TaskLock.php diff --git a/app/Application/Exceptions/EntriesAlreadyInvoiced.php b/app/Application/Exceptions/EntriesAlreadyInvoiced.php index d29ede9..c4308f2 100644 --- a/app/Application/Exceptions/EntriesAlreadyInvoiced.php +++ b/app/Application/Exceptions/EntriesAlreadyInvoiced.php @@ -18,6 +18,18 @@ public static function forEntry(int $entryId): self return self::forEntries([$entryId]); } + /** + * An edit that would move time an invoice was already raised for. + * + * @param list $fields + */ + public static function forLockedFields(int $entryId, array $fields): self + { + return new self( + "Time entry {$entryId} is already on an invoice: ".implode(', ', $fields).' cannot be changed.', + ); + } + public static function forOtherInvoice(int $entryId, int $invoiceId): self { return new self("Time entry {$entryId} is already stamped with invoice {$invoiceId}."); diff --git a/app/Application/Exceptions/TaskLocked.php b/app/Application/Exceptions/TaskLocked.php new file mode 100644 index 0000000..ae649dd --- /dev/null +++ b/app/Application/Exceptions/TaskLocked.php @@ -0,0 +1,20 @@ +settings->lockInvoicedTasks($companyId)) { + return; + } + + if ($this->summary->forTask($companyId, $taskId)['invoiced'] === TaskTimeSummary::INVOICED) { + throw TaskLocked::forTask($taskId); + } + } +} diff --git a/app/Application/TaskService.php b/app/Application/TaskService.php index 602b01e..27d0e05 100644 --- a/app/Application/TaskService.php +++ b/app/Application/TaskService.php @@ -4,8 +4,10 @@ namespace Modules\TasksProjects\Application; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\ModelNotFoundException; +use Illuminate\Database\Query\Builder as QueryBuilder; use Illuminate\Database\QueryException; use Illuminate\Support\Carbon; use Illuminate\Support\Facades\DB; @@ -47,10 +49,11 @@ public function __construct( private readonly BoardOrderingService $board, private readonly TaskStatusService $statuses, private readonly ProjectService $projects, + private readonly TaskLock $lock, ) {} /** - * @param array{project_id?: int, assignee_id?: int, task_status_id?: int, customer_id?: int, due_before?: string, due_after?: string, search?: string, sort_by?: string, sort_order?: string} $filters + * @param array{project_id?: int, assignee_id?: int, task_status_id?: int, customer_id?: int, invoiced?: bool, due_before?: string, due_after?: string, search?: string, sort_by?: string, sort_order?: string} $filters * @return Collection */ public function listFor(int $companyId, array $filters = []): Collection @@ -75,6 +78,10 @@ public function listFor(int $companyId, array $filters = []): Collection $query->where('name', 'like', '%'.$filters['search'].'%'); } + if (isset($filters['invoiced'])) { + $this->filterByInvoiced($query, $companyId, (bool) $filters['invoiced']); + } + $sorts = $this->sorts(); [$key, $order] = $this->sortFor($filters, $sorts, self::DEFAULT_SORT_KEY, self::DEFAULT_SORT_ORDER); @@ -165,6 +172,7 @@ public function update(int $companyId, int $id, array $attributes): Task { return DB::transaction(function () use ($companyId, $id, $attributes): Task { $task = $this->findForCompany($companyId, $id); + $this->lock->guard($companyId, (int) $task->id); if (array_key_exists('project_id', $attributes)) { $project = $attributes['project_id'] === null @@ -201,6 +209,7 @@ public function delete(int $companyId, int $id): void { DB::transaction(function () use ($companyId, $id): void { $task = $this->findForCompany($companyId, $id); + $this->lock->guard($companyId, (int) $task->id); $invoiced = TimeEntry::query() ->forCompany($companyId) @@ -224,6 +233,7 @@ public function move(int $companyId, int $taskId, int $statusId, ?int $beforeId { return DB::transaction(function () use ($companyId, $taskId, $statusId, $beforeId, $afterId): Task { $task = $this->findForCompany($companyId, $taskId); + $this->lock->guard($companyId, (int) $task->id); $status = $this->statuses->findForCompany($companyId, $statusId); $position = $this->board->positionFor($companyId, (int) $status->id, $beforeId, $afterId); @@ -250,6 +260,50 @@ private function applyStatus(Task $task, TaskStatus $status): void $task->closed_at = null; } + /** + * Narrow the list to tasks whose billable time has, or has not, reached an + * invoice. + * + * The state belongs to the entries, so it is asked of them rather than + * cached on the task: uninvoiced means at least one billable entry is still + * unbilled, and invoiced means a stamped entry exists and no unbilled one + * does. Both are `exists` subqueries, which MySQL, PostgreSQL and SQLite + * all plan off the `(company_id, task_id)` index and all spell the same. + * + * @param Builder $query + */ + private function filterByInvoiced(Builder $query, int $companyId, bool $invoiced): void + { + if (! $invoiced) { + $query->whereExists($this->billableEntries($companyId, stamped: false)); + + return; + } + + $query->whereExists($this->billableEntries($companyId, stamped: true)) + ->whereNotExists($this->billableEntries($companyId, stamped: false)); + } + + /** A correlated subquery over the stopped, billable entries of the task. */ + private function billableEntries(int $companyId, bool $stamped): callable + { + $entries = (new TimeEntry)->getTable(); + $tasks = (new Task)->getTable(); + + return static function (QueryBuilder $sub) use ($companyId, $stamped, $entries, $tasks): void { + $sub->selectRaw('1') + ->from($entries) + ->whereColumn($entries.'.task_id', $tasks.'.id') + ->where($entries.'.company_id', $companyId) + ->whereNull($entries.'.running_user_id') + ->where($entries.'.billable', true); + + $stamped + ? $sub->whereNotNull($entries.'.invoice_id') + : $sub->whereNull($entries.'.invoice_id'); + }; + } + /** @param array $attributes */ private function customerFor(?Project $project, array $attributes): ?int { diff --git a/app/Application/TimeEntryService.php b/app/Application/TimeEntryService.php index a51f136..1d29b79 100644 --- a/app/Application/TimeEntryService.php +++ b/app/Application/TimeEntryService.php @@ -21,6 +21,12 @@ */ final class TimeEntryService { + /** How many rows a task's time log ever answers with. */ + public const LOG_LIMIT = 500; + + /** The fields an invoice owns once it has been raised against an entry. */ + private const STAMPED_FIELDS = ['task_id', 'started_at', 'ended_at', 'duration_minutes', 'billable']; + public function __construct( private readonly RateResolver $rates, private readonly ModuleSettings $settings, @@ -39,6 +45,7 @@ public function create(int $companyId, array $attributes): TimeEntry $minutes = Rounding::roundMinutes( $this->minutesFrom($attributes, $startedAt, $endedAt), $this->settings->roundingMinutes($companyId), + $this->settings->roundingDirection($companyId), ); $billable = (bool) ($attributes['billable'] ?? $task->billable); @@ -66,13 +73,23 @@ public function create(int $companyId, array $attributes): TimeEntry * Edit an entry, re-rounding the duration. * * The rate is re-resolved only while the entry is unbilled: once it is - * stamped with an invoice the money on it belongs to that invoice. + * stamped with an invoice the money on it belongs to that invoice, and so + * does the time it was raised for. A stamped entry therefore accepts a new + * description and nothing else: its minutes are never re-rounded, its rate + * and amount are left exactly as the invoice recorded them, and an attempt + * to move the clock, the billable flag or the task is refused rather than + * quietly ignored. * * @param array $attributes */ public function update(int $companyId, int $id, array $attributes): TimeEntry { $entry = $this->findForCompany($companyId, $id); + + if ($entry->isStamped()) { + return $this->updateStamped($entry, $attributes); + } + $task = $this->tasks->findForCompany($companyId, (int) ($attributes['task_id'] ?? $entry->task_id)); if ((int) $task->id !== (int) $entry->task_id) { @@ -100,13 +117,12 @@ public function update(int $companyId, int $id, array $attributes): TimeEntry $entry->duration_minutes = Rounding::roundMinutes( $this->minutesFrom($attributes, $entry->started_at, $entry->ended_at, (int) $entry->duration_minutes), $this->settings->roundingMinutes($companyId), + $this->settings->roundingDirection($companyId), ); - if (! $entry->isStamped()) { - $entry->rate = array_key_exists('rate', $attributes) && $attributes['rate'] !== null - ? (int) $attributes['rate'] - : $this->rates->resolve($task, (int) $entry->user_id, $this->settings); - } + $entry->rate = array_key_exists('rate', $attributes) && $attributes['rate'] !== null + ? (int) $attributes['rate'] + : $this->rates->resolve($task, (int) $entry->user_id, $this->settings); $entry->amount = self::amountFor((int) $entry->duration_minutes, (int) $entry->rate); $entry->save(); @@ -114,6 +130,58 @@ public function update(int $companyId, int $id, array $attributes): TimeEntry return $entry; } + /** + * Save the one field an invoiced entry still owns. + * + * Sending the unchanged value of a protected field is not an edit, so a + * form that posts the whole row back still works; only a real change is + * refused, and the message names the fields that would have moved. + * + * @param array $attributes + */ + private function updateStamped(TimeEntry $entry, array $attributes): TimeEntry + { + $changed = array_values(array_filter( + self::STAMPED_FIELDS, + static fn (string $field): bool => self::wouldChange($entry, $field, $attributes), + )); + + if ($changed !== []) { + throw EntriesAlreadyInvoiced::forLockedFields((int) $entry->id, $changed); + } + + if (array_key_exists('description', $attributes)) { + $entry->description = $attributes['description']; + } + + $entry->save(); + + return $entry; + } + + /** + * Whether the request really moves a protected field off its stored value. + * + * @param array $attributes + */ + private static function wouldChange(TimeEntry $entry, string $field, array $attributes): bool + { + if (! array_key_exists($field, $attributes)) { + return false; + } + + $wanted = $attributes[$field]; + $current = $entry->{$field}; + + return match ($field) { + 'started_at', 'ended_at' => $wanted === null || $current === null + ? $wanted !== $current + : ! $current->equalTo(Carbon::parse($wanted)), + 'billable' => (bool) $wanted !== (bool) $current, + default => $wanted !== null && (int) $wanted !== (int) $current, + }; + } + /** Invoiced time is history: it can never be deleted from under an invoice. */ public function delete(int $companyId, int $id): void { @@ -179,6 +247,40 @@ public function listFor(int $companyId, array $filters, ?int $viewerUserId, bool return $query->orderByDesc('started_at')->orderByDesc('id')->get(); } + /** + * The time log of one task: the running clocks first, then everything + * logged against it, newest first. + * + * A running entry has no duration yet, so it would sort among the oldest + * rows on `started_at` alone; a CASE every supported database understands + * lifts it to the top instead. The cap keeps a task somebody has been + * logging against for years from answering with a megabyte of JSON. + * + * @return Collection + */ + public function logForTask( + int $companyId, + int $taskId, + ?int $viewerUserId, + bool $canSeeAll, + int $limit = self::LOG_LIMIT, + ): Collection { + $query = TimeEntry::query() + ->forCompany($companyId) + ->where('task_id', $taskId); + + if (! $canSeeAll) { + $query->where('user_id', $viewerUserId); + } + + return $query + ->orderByRaw('CASE WHEN running_user_id IS NULL THEN 1 ELSE 0 END') + ->orderByDesc('started_at') + ->orderByDesc('id') + ->limit($limit) + ->get(); + } + /** The cached money on an entry: minutes as hours, times the frozen rate. */ public static function amountFor(int $minutes, int $rate): int { diff --git a/app/Http/Controllers/TasksController.php b/app/Http/Controllers/TasksController.php index 9c66404..ec97dac 100644 --- a/app/Http/Controllers/TasksController.php +++ b/app/Http/Controllers/TasksController.php @@ -7,19 +7,37 @@ use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Http\Resources\Json\AnonymousResourceCollection; +use Modules\TasksProjects\Application\Exceptions\TimerAlreadyRunning; use Modules\TasksProjects\Application\TaskService; +use Modules\TasksProjects\Application\TaskTimeSummary; +use Modules\TasksProjects\Application\TimerService; use Modules\TasksProjects\Http\Requests\ListTasksRequest; use Modules\TasksProjects\Http\Requests\MoveTaskRequest; use Modules\TasksProjects\Http\Requests\StoreTaskRequest; use Modules\TasksProjects\Http\Requests\UpdateTaskRequest; use Modules\TasksProjects\Http\Resources\TaskResource; +use Modules\TasksProjects\Models\Task; use Modules\TasksProjects\Support\Abilities; use Modules\TasksProjects\Support\Authorizes; - +use Modules\TasksProjects\Support\CompanyContext; +use Modules\TasksProjects\Support\ModuleSettings; + +/** + * Tasks, each answering with the time logged against it. + * + * Every response here carries the task's `time` block, filled in for the whole + * page at once by TaskTimeSummary so a list of fifty tasks still costs three + * queries rather than fifty. + */ final class TasksController extends Controller { - public function __construct(Authorizes $authorizes, private readonly TaskService $tasks) - { + public function __construct( + Authorizes $authorizes, + private readonly TaskService $tasks, + private readonly TaskTimeSummary $summary, + private readonly TimerService $timer, + private readonly ModuleSettings $settings, + ) { parent::__construct($authorizes); } @@ -34,6 +52,7 @@ public function index(ListTasksRequest $request): AnonymousResourceCollection 'assignee_id' => isset($filters['assignee_id']) ? (int) $filters['assignee_id'] : null, 'task_status_id' => isset($filters['task_status_id']) ? (int) $filters['task_status_id'] : null, 'customer_id' => isset($filters['customer_id']) ? (int) $filters['customer_id'] : null, + 'invoiced' => array_key_exists('invoiced', $filters) ? $request->boolean('invoiced') : null, 'due_before' => $filters['due_before'] ?? null, 'due_after' => $filters['due_after'] ?? null, 'search' => $filters['search'] ?? null, @@ -41,18 +60,31 @@ public function index(ListTasksRequest $request): AnonymousResourceCollection 'sort_order' => $filters['sort_order'] ?? null, ], static fn (mixed $value): bool => $value !== null)); - return TaskResource::collection($this->paginate($tasks, $request)); + $page = $this->paginate($tasks, $request); + $this->summary->attach($context->companyId, $page->getCollection()); + + return TaskResource::collection($page); } + /** + * Create a task, and start its creator's clock when the company asks for it. + * + * Auto-start is a convenience, never a precondition: a creator who is + * already timing something else keeps that timer and still gets the task. + */ public function store(StoreTaskRequest $request): TaskResource { $context = $this->context($request); $this->authorize($context, Abilities::CREATE_TASK); - return new TaskResource($this->tasks->create( + $task = $this->tasks->create( $context->companyId, $request->validated() + ['creator_id' => $context->userId], - )); + ); + + $this->autoStart($context, (int) $task->id); + + return $this->withTime($context, $task); } public function show(Request $request, int $id): TaskResource @@ -60,7 +92,7 @@ public function show(Request $request, int $id): TaskResource $context = $this->context($request); $this->authorize($context, Abilities::VIEW_TASK); - return new TaskResource($this->tasks->findForCompany($context->companyId, $id)); + return $this->withTime($context, $this->tasks->findForCompany($context->companyId, $id)); } public function update(UpdateTaskRequest $request, int $id): TaskResource @@ -68,7 +100,7 @@ public function update(UpdateTaskRequest $request, int $id): TaskResource $context = $this->context($request); $this->authorize($context, Abilities::EDIT_TASK); - return new TaskResource($this->tasks->update($context->companyId, $id, $request->validated())); + return $this->withTime($context, $this->tasks->update($context->companyId, $id, $request->validated())); } public function destroy(Request $request, int $id): JsonResponse @@ -89,7 +121,7 @@ public function move(MoveTaskRequest $request, int $id): TaskResource $validated = $request->validated(); - return new TaskResource($this->tasks->move( + return $this->withTime($context, $this->tasks->move( $context->companyId, $id, (int) $validated['task_status_id'], @@ -97,4 +129,40 @@ public function move(MoveTaskRequest $request, int $id): TaskResource isset($validated['after_id']) ? (int) $validated['after_id'] : null, )); } + + /** One task, with its time block filled in. */ + private function withTime(CompanyContext $context, Task $task): TaskResource + { + $this->summary->attach($context->companyId, [$task]); + + return new TaskResource($task); + } + + /** + * Start the creator's timer on a brand new task. + * + * The setting only ever adds a timer: a creator who already has one running + * keeps it, and a race that slips past the check is caught by the same + * unique index the timer relies on, so the create never fails over this. + */ + private function autoStart(CompanyContext $context, int $taskId): void + { + if (! $this->settings->autoStartTasks($context->companyId)) { + return; + } + + if (! $this->allows($context, Abilities::VIEW_OWN_TIME)) { + return; + } + + if ($this->timer->running($context->companyId, $context->userId) !== null) { + return; + } + + try { + $this->timer->start($context->companyId, $context->userId, $taskId); + } catch (TimerAlreadyRunning) { + // Another tab won the race; that timer is as good as this one. + } + } } diff --git a/app/Http/Requests/ListTasksRequest.php b/app/Http/Requests/ListTasksRequest.php index a2b7017..83efddc 100644 --- a/app/Http/Requests/ListTasksRequest.php +++ b/app/Http/Requests/ListTasksRequest.php @@ -16,6 +16,7 @@ public function rules(): array 'assignee_id' => ['sometimes', 'integer', 'min:1'], 'task_status_id' => ['sometimes', 'integer', 'min:1'], 'customer_id' => ['sometimes', 'integer', 'min:1'], + 'invoiced' => ['sometimes', 'boolean'], 'due_before' => ['sometimes', 'date'], 'due_after' => ['sometimes', 'date'], 'search' => ['sometimes', 'string', 'max:255'], diff --git a/tests/Feature/TimeEntriesApiTest.php b/tests/Feature/TimeEntriesApiTest.php index aeb48f7..f6d90a2 100644 --- a/tests/Feature/TimeEntriesApiTest.php +++ b/tests/Feature/TimeEntriesApiTest.php @@ -4,6 +4,7 @@ namespace Modules\TasksProjects\Tests\Feature; +use Modules\TasksProjects\Application\Rounding; use Modules\TasksProjects\Models\TimeEntry; use Modules\TasksProjects\Support\Abilities; use Modules\TasksProjects\Support\Authorizes; @@ -201,6 +202,94 @@ public function test_logging_time_for_someone_else_is_allowed_with_edit_all_time ->assertJsonPath('data.user_id', self::OTHER_USER); } + public function test_an_invoiced_entry_refuses_a_change_to_its_time_or_its_task(): void + { + $task = $this->taskOnProjectAt(6000); + $elsewhere = (int) $this->makeTask(self::COMPANY, ['name' => 'Another task'])->id; + $entry = $this->makeEntry(self::COMPANY, $task, ['invoice_id' => 77, 'invoice_item_id' => 88]); + + $refused = [ + ['duration_minutes' => 240], + ['started_at' => '2026-09-01 08:00:00'], + ['ended_at' => '2026-09-01 12:00:00'], + ['billable' => false], + ['task_id' => $elsewhere], + ]; + + foreach ($refused as $payload) { + $this->asCompany(self::COMPANY) + ->putJson('/api/v1/tasks-projects/time-entries/'.$entry->id, $payload) + ->assertStatus(422) + ->assertJsonPath('error', 'entries_already_invoiced'); + } + + $stored = TimeEntry::query()->findOrFail($entry->id); + self::assertSame(60, (int) $stored->duration_minutes); + self::assertSame(10000, (int) $stored->amount); + self::assertTrue((bool) $stored->billable); + self::assertSame($task, (int) $stored->task_id); + } + + public function test_an_invoiced_entry_still_takes_a_new_description(): void + { + $task = $this->taskOnProjectAt(6000); + $entry = $this->makeEntry(self::COMPANY, $task, ['invoice_id' => 77, 'invoice_item_id' => 88]); + + $this->asCompany(self::COMPANY) + ->putJson('/api/v1/tasks-projects/time-entries/'.$entry->id, ['description' => 'Typo fix']) + ->assertOk() + ->assertJsonPath('data.description', 'Typo fix') + ->assertJsonPath('data.duration_minutes', 60) + ->assertJsonPath('data.amount', 10000); + } + + public function test_an_invoiced_entry_accepts_a_form_that_posts_its_own_values_back(): void + { + $task = $this->taskOnProjectAt(6000); + $entry = $this->makeEntry(self::COMPANY, $task, ['invoice_id' => 77, 'invoice_item_id' => 88]); + + $this->asCompany(self::COMPANY) + ->putJson('/api/v1/tasks-projects/time-entries/'.$entry->id, [ + 'task_id' => $task, + 'started_at' => $entry->started_at->toIso8601String(), + 'ended_at' => $entry->ended_at->toIso8601String(), + 'duration_minutes' => 60, + 'billable' => true, + 'description' => 'Same row, new note', + ]) + ->assertOk() + ->assertJsonPath('data.description', 'Same row, new note'); + } + + public function test_the_company_rounding_direction_is_applied_when_the_entry_is_saved(): void + { + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'rounding_minutes', 15); + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'rounding_direction', Rounding::DOWN); + $task = $this->taskOnProjectAt(6000); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/time-entries', [ + 'task_id' => $task, + 'started_at' => '2026-09-15 09:00:00', + 'ended_at' => '2026-09-15 09:20:00', + ]) + ->assertCreated() + ->assertJsonPath('data.duration_minutes', 15) + ->assertJsonPath('data.amount', 1500); + + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'rounding_direction', Rounding::UP); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/time-entries', [ + 'task_id' => $task, + 'started_at' => '2026-09-15 09:00:00', + 'ended_at' => '2026-09-15 09:20:00', + ]) + ->assertCreated() + ->assertJsonPath('data.duration_minutes', 30) + ->assertJsonPath('data.amount', 3000); + } + public function test_an_invoiced_entry_cannot_be_deleted(): void { $task = $this->taskOnProjectAt(6000); diff --git a/tests/TestCase.php b/tests/TestCase.php index d037519..b0562b7 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -15,6 +15,13 @@ use InvoiceShelf\Modules\Contracts\Host\SettingsStore; use InvoiceShelf\Modules\InvoiceShelfModulesServiceProvider; use InvoiceShelf\Modules\Registry; +use Modules\TasksProjects\Application\BoardOrderingService; +use Modules\TasksProjects\Application\ProjectService; +use Modules\TasksProjects\Application\TaskLock; +use Modules\TasksProjects\Application\TaskNumberSequence; +use Modules\TasksProjects\Application\TaskService; +use Modules\TasksProjects\Application\TaskStatusService; +use Modules\TasksProjects\Application\TaskTimeSummary; use Modules\TasksProjects\Http\DomainExceptionRenderer; use Modules\TasksProjects\Models\Project; use Modules\TasksProjects\Models\ProjectMember; @@ -98,6 +105,18 @@ protected function moduleSettings(): ModuleSettings return new ModuleSettings($this->settings); } + /** A TaskService wired with the collaborators the container gives it. */ + protected function taskService(?TaskStatusService $statuses = null): TaskService + { + return new TaskService( + new TaskNumberSequence, + new BoardOrderingService, + $statuses ?? new TaskStatusService, + new ProjectService($this->companyData), + new TaskLock($this->moduleSettings(), new TaskTimeSummary), + ); + } + /** * Load the module's own route file, the way the provider does in the host. * diff --git a/tests/Unit/TaskServiceTest.php b/tests/Unit/TaskServiceTest.php index a6363b3..d7df584 100644 --- a/tests/Unit/TaskServiceTest.php +++ b/tests/Unit/TaskServiceTest.php @@ -6,10 +6,7 @@ use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Support\Carbon; -use Modules\TasksProjects\Application\BoardOrderingService; use Modules\TasksProjects\Application\Exceptions\EntriesAlreadyInvoiced; -use Modules\TasksProjects\Application\ProjectService; -use Modules\TasksProjects\Application\TaskNumberSequence; use Modules\TasksProjects\Application\TaskService; use Modules\TasksProjects\Application\TaskStatusService; use Modules\TasksProjects\Models\Task; @@ -29,12 +26,7 @@ protected function setUp(): void parent::setUp(); $this->statuses = new TaskStatusService; - $this->tasks = new TaskService( - new TaskNumberSequence, - new BoardOrderingService, - $this->statuses, - new ProjectService($this->companyData), - ); + $this->tasks = $this->taskService($this->statuses); } public function test_it_denormalises_the_customer_from_the_project(): void diff --git a/tests/Unit/TimeEntryServiceTest.php b/tests/Unit/TimeEntryServiceTest.php index 24058a0..5313bf5 100644 --- a/tests/Unit/TimeEntryServiceTest.php +++ b/tests/Unit/TimeEntryServiceTest.php @@ -5,13 +5,8 @@ namespace Modules\TasksProjects\Tests\Unit; use Illuminate\Database\Eloquent\ModelNotFoundException; -use Modules\TasksProjects\Application\BoardOrderingService; use Modules\TasksProjects\Application\Exceptions\EntriesAlreadyInvoiced; -use Modules\TasksProjects\Application\ProjectService; use Modules\TasksProjects\Application\RateResolver; -use Modules\TasksProjects\Application\TaskNumberSequence; -use Modules\TasksProjects\Application\TaskService; -use Modules\TasksProjects\Application\TaskStatusService; use Modules\TasksProjects\Application\TimeEntryService; use Modules\TasksProjects\Models\Project; use Modules\TasksProjects\Models\TimeEntry; @@ -33,7 +28,7 @@ protected function setUp(): void $this->entries = new TimeEntryService( new RateResolver, $this->moduleSettings(), - new TaskService(new TaskNumberSequence, new BoardOrderingService, new TaskStatusService, new ProjectService($this->companyData)), + $this->taskService(), ); } diff --git a/tests/Unit/TimerServiceTest.php b/tests/Unit/TimerServiceTest.php index 9449527..fbec38b 100644 --- a/tests/Unit/TimerServiceTest.php +++ b/tests/Unit/TimerServiceTest.php @@ -7,13 +7,8 @@ use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Support\Carbon; use Illuminate\Support\Facades\DB; -use Modules\TasksProjects\Application\BoardOrderingService; use Modules\TasksProjects\Application\Exceptions\TimerAlreadyRunning; -use Modules\TasksProjects\Application\ProjectService; use Modules\TasksProjects\Application\RateResolver; -use Modules\TasksProjects\Application\TaskNumberSequence; -use Modules\TasksProjects\Application\TaskService; -use Modules\TasksProjects\Application\TaskStatusService; use Modules\TasksProjects\Application\TimerService; use Modules\TasksProjects\Models\TimeEntry; use Modules\TasksProjects\Support\ModuleSettings; @@ -32,7 +27,7 @@ protected function setUp(): void parent::setUp(); $this->timer = new TimerService( - new TaskService(new TaskNumberSequence, new BoardOrderingService, new TaskStatusService, new ProjectService($this->companyData)), + $this->taskService(), new RateResolver, $this->moduleSettings(), ); From abaa325917d2e4de4c1005fb67bc0a497f356c0f Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski Date: Tue, 15 Sep 2026 09:36:23 +0200 Subject: [PATCH 5/6] feat(sidebar): register Projects and Tasks as first-class entries The module had one sidebar entry, filed under Modules, and time logging was reachable only through an unlabelled clock button. Projects and Tasks are two ways into the same module, not a feature and its sub-page, so both join the host's own main group after Items: Projects at priority 40, Tasks at 50. A firm that installs this module lives in it all day, and a Modules heading files it away as an add-on. The registry keys stay separate, so menuFor('tasks-projects') still answers with the module's primary entry and the host's module page lookup is unaffected. lang/en/menu.php keeps `title` for whatever still reads it. invoice-tasks now also depends on the host edit-invoice ability, because invoicing a task ends on the host invoice edit page and a role that may raise the invoice has to be allowed to open it. --- app/Support/Abilities.php | 2 + app/Support/ModuleRegistration.php | 132 +++++++++++++++++++---- lang/en/menu.php | 4 + tests/Feature/ModuleRegistrationTest.php | 125 ++++++++++++++++----- 4 files changed, 217 insertions(+), 46 deletions(-) diff --git a/app/Support/Abilities.php b/app/Support/Abilities.php index b6b2dca..41c535b 100644 --- a/app/Support/Abilities.php +++ b/app/Support/Abilities.php @@ -49,4 +49,6 @@ final class Abilities public const HOST_VIEW_CUSTOMER = 'view-customer'; public const HOST_CREATE_INVOICE = 'create-invoice'; + + public const HOST_EDIT_INVOICE = 'edit-invoice'; } diff --git a/app/Support/ModuleRegistration.php b/app/Support/ModuleRegistration.php index 5f14ed3..44e7c37 100644 --- a/app/Support/ModuleRegistration.php +++ b/app/Support/ModuleRegistration.php @@ -5,6 +5,8 @@ namespace Modules\TasksProjects\Support; use InvoiceShelf\Modules\Registry; +use InvoiceShelf\Modules\Settings\FieldType; +use Modules\TasksProjects\Application\Rounding; final class ModuleRegistration { @@ -13,43 +15,89 @@ public static function register(string $modulePath): void Registry::registerScript('tasks-projects', $modulePath.'/dist/init.js'); Registry::registerStyle('tasks-projects', $modulePath.'/dist/style.css'); + self::registerMenu(); + Registry::registerSettings('tasks-projects', self::settingsSchema()); + self::registerAbilities(); + } + + /** + * Two sidebar entries, in the host's own main group. + * + * Projects and Tasks are two ways into the same module, not one feature and + * its sub-page: people either plan work or do work. They join `main` after + * Items (priorities 10, 20, 30) because a firm that installs this module + * lives in it all day, and a "Modules" heading would file it away as an + * add-on. The registry keys are separate, so `menuFor('tasks-projects')` + * still answers with the module's primary entry. + */ + private static function registerMenu(): void + { Registry::registerMenu('tasks-projects', [ - 'title' => 'tasksprojects::menu.title', + 'title' => 'tasksprojects::menu.projects', + 'link' => '/admin/modules/tasks-projects/projects', + 'icon' => 'FolderIcon', + 'group' => 'main', + 'group_label' => '', + // Lower sorts first within the group; the core entries end at 30. + 'priority' => 40, + ]); + + Registry::registerMenu('tasks-projects.tasks', [ + 'title' => 'tasksprojects::menu.tasks', 'link' => '/admin/modules/tasks-projects', 'icon' => 'ClipboardDocumentListIcon', - // Lower sorts first within the sidebar group; official modules use 10, 20, ... - 'priority' => 10, + 'group' => 'main', + 'group_label' => '', + 'priority' => 50, ]); + } - Registry::registerSettings('tasks-projects', [ + /** + * The per-company settings the host renders and validates. + * + * General holds how time is measured and who may see it; the second section + * is only about what an invoice line says, which is a different question and + * a different audience. + * + * @return array + */ + private static function settingsSchema(): array + { + return [ 'sections' => [ [ 'title' => 'tasksprojects::settings.general_section', 'fields' => [ [ 'key' => 'default_rate', - 'type' => 'number', + 'type' => FieldType::Number->value, 'label' => 'tasksprojects::settings.default_rate', 'default' => 0, 'rules' => ['integer', 'min:0'], ], [ 'key' => 'rounding_minutes', - 'type' => 'select', + 'type' => FieldType::Select->value, 'label' => 'tasksprojects::settings.rounding_minutes', - 'default' => 1, + 'default' => ModuleSettings::DEFAULT_ROUNDING_MINUTES, + 'options' => self::roundingOptions(), + ], + [ + 'key' => 'rounding_direction', + 'type' => FieldType::Select->value, + 'label' => 'tasksprojects::settings.rounding_direction', + 'default' => ModuleSettings::DEFAULT_ROUNDING_DIRECTION, 'options' => [ - 1 => '1', - 6 => '6', - 15 => '15', - 30 => '30', + Rounding::NEAREST => 'tasksprojects::settings.rounding_nearest', + Rounding::UP => 'tasksprojects::settings.rounding_up', + Rounding::DOWN => 'tasksprojects::settings.rounding_down', ], ], [ 'key' => 'week_start', - 'type' => 'select', + 'type' => FieldType::Select->value, 'label' => 'tasksprojects::settings.week_start', - 'default' => 1, + 'default' => ModuleSettings::DEFAULT_WEEK_START, 'options' => [ 0 => 'Sunday', 1 => 'Monday', @@ -60,18 +108,52 @@ public static function register(string $modulePath): void 6 => 'Saturday', ], ], - [ - 'key' => 'members_see_all_time', - 'type' => 'switch', - 'label' => 'tasksprojects::settings.members_see_all_time', - 'default' => false, - ], + self::switchField('members_see_all_time'), + self::switchField('auto_start_tasks'), + self::switchField('lock_invoiced_tasks'), + self::switchField('hide_invoiced_on_board'), + ], + ], + [ + 'title' => 'tasksprojects::settings.invoice_section', + 'fields' => [ + self::switchField('invoice_project_heading'), + self::switchField('invoice_task_description'), + self::switchField('invoice_entry_dates'), + self::switchField('invoice_entry_times'), + self::switchField('invoice_entry_hours'), + self::switchField('invoice_entry_descriptions'), ], ], ], - ]); + ]; + } - self::registerAbilities(); + /** + * One stored switch, taking its default from the same table the readers use. + * + * @return array + */ + private static function switchField(string $key): array + { + return [ + 'key' => $key, + 'type' => FieldType::Switch_->value, + 'label' => 'tasksprojects::settings.'.$key, + 'default' => ModuleSettings::FLAGS[$key], + ]; + } + + /** @return array */ + private static function roundingOptions(): array + { + $options = []; + + foreach (ModuleSettings::ROUNDING_INCREMENTS as $minutes) { + $options[$minutes] = (string) $minutes; + } + + return $options; } /** @@ -102,7 +184,13 @@ private static function registerAbilities(): void [Abilities::VIEW_OWN_TIME, 'View own time', []], [Abilities::VIEW_ALL_TIME, 'View all time', [$viewOwnTime]], [Abilities::EDIT_ALL_TIME, 'Edit all time', [$viewAllTime]], - [Abilities::INVOICE_TASKS, 'Invoice tasks', [$viewAllTime, Abilities::HOST_CREATE_INVOICE]], + // Invoicing a task ends on the host invoice edit page, so the role + // that may raise the invoice must also be allowed to open it. + [Abilities::INVOICE_TASKS, 'Invoice tasks', [ + $viewAllTime, + Abilities::HOST_CREATE_INVOICE, + Abilities::HOST_EDIT_INVOICE, + ]], ]; foreach ($abilities as [$ability, $name, $dependsOn]) { diff --git a/lang/en/menu.php b/lang/en/menu.php index e545096..1758726 100644 --- a/lang/en/menu.php +++ b/lang/en/menu.php @@ -1,5 +1,9 @@ 'Projects', + + 'projects' => 'Projects', + 'tasks' => 'Tasks', ]; diff --git a/tests/Feature/ModuleRegistrationTest.php b/tests/Feature/ModuleRegistrationTest.php index 6244183..2ec7b8a 100644 --- a/tests/Feature/ModuleRegistrationTest.php +++ b/tests/Feature/ModuleRegistrationTest.php @@ -6,14 +6,16 @@ use InvoiceShelf\Modules\Contracts\Host\SettingsStore; use InvoiceShelf\Modules\Registry; +use Modules\TasksProjects\Application\Rounding; use Modules\TasksProjects\Lifecycle\DataCleanup; use Modules\TasksProjects\Support\Abilities; use Modules\TasksProjects\Support\ModuleRegistration; +use Modules\TasksProjects\Support\ModuleSettings; use Modules\TasksProjects\Tests\TestCase; final class ModuleRegistrationTest extends TestCase { - public function test_it_registers_a_local_script_style_sidebar_entry_and_settings_schema(): void + public function test_it_registers_a_local_script_and_style(): void { $modulePath = dirname(__DIR__, 2); @@ -21,28 +23,104 @@ public function test_it_registers_a_local_script_style_sidebar_entry_and_setting self::assertSame(realpath($modulePath.'/dist/init.js'), Registry::scriptFor('tasks-projects')); self::assertSame(realpath($modulePath.'/dist/style.css'), Registry::styleFor('tasks-projects')); + } + + public function test_it_registers_projects_and_tasks_as_two_entries_of_the_core_main_group(): void + { + ModuleRegistration::register(dirname(__DIR__, 2)); + + self::assertSame([ + 'group' => 'main', + 'group_label' => '', + 'priority' => 40, + 'title' => 'tasksprojects::menu.projects', + 'link' => '/admin/modules/tasks-projects/projects', + 'icon' => 'FolderIcon', + ], Registry::menuFor('tasks-projects')); self::assertSame([ - 'group' => 'modules', - 'group_label' => 'navigation.modules', - 'priority' => 10, - 'title' => 'tasksprojects::menu.title', + 'group' => 'main', + 'group_label' => '', + 'priority' => 50, + 'title' => 'tasksprojects::menu.tasks', 'link' => '/admin/modules/tasks-projects', 'icon' => 'ClipboardDocumentListIcon', - ], Registry::menuFor('tasks-projects')); + ], Registry::menuFor('tasks-projects.tasks')); + + // The primary slug still answers, which is what the host's module page + // lookup uses; the second key only ever adds a row to the sidebar. + self::assertSame( + ['tasks-projects', 'tasks-projects.tasks'], + array_keys(Registry::allMenu()), + ); + } + + public function test_the_menu_titles_are_translation_keys_that_exist(): void + { + $menu = require dirname(__DIR__, 2).'/lang/en/menu.php'; + + self::assertSame('Projects', $menu['projects']); + self::assertSame('Tasks', $menu['tasks']); + self::assertArrayHasKey('title', $menu, 'The original key stays for compatibility.'); + } + + public function test_the_settings_schema_covers_every_stored_key(): void + { + ModuleRegistration::register(dirname(__DIR__, 2)); + + $settings = Registry::settingsFor('tasks-projects'); + + self::assertNotNull($settings); + + $fields = array_column($settings->fields(), null, 'key'); + + self::assertSame([ + 'default_rate', + 'rounding_minutes', + 'rounding_direction', + 'week_start', + 'members_see_all_time', + 'auto_start_tasks', + 'lock_invoiced_tasks', + 'hide_invoiced_on_board', + 'invoice_project_heading', + 'invoice_task_description', + 'invoice_entry_dates', + 'invoice_entry_times', + 'invoice_entry_hours', + 'invoice_entry_descriptions', + ], array_keys($fields)); + + self::assertSame(0, $fields['default_rate']['default']); + self::assertSame( + ['1' => '1', '5' => '5', '6' => '6', '15' => '15', '30' => '30', '60' => '60'], + $fields['rounding_minutes']['options'], + ); + self::assertSame(ModuleSettings::DEFAULT_ROUNDING_MINUTES, $fields['rounding_minutes']['default']); + self::assertSame(Rounding::NEAREST, $fields['rounding_direction']['default']); + self::assertSame( + ['nearest', 'up', 'down'], + array_keys($fields['rounding_direction']['options']), + ); + self::assertSame(ModuleSettings::DEFAULT_WEEK_START, $fields['week_start']['default']); + + foreach (ModuleSettings::FLAGS as $key => $default) { + self::assertSame('switch', $fields[$key]['type'], "Setting {$key} is not a switch."); + self::assertSame($default, $fields[$key]['default'], "Setting {$key} has the wrong default."); + } + } + + public function test_the_schema_and_the_cleanup_keys_never_drift_apart(): void + { + ModuleRegistration::register(dirname(__DIR__, 2)); $settings = Registry::settingsFor('tasks-projects'); self::assertNotNull($settings); self::assertSame( - ['default_rate', 'rounding_minutes', 'week_start', 'members_see_all_time'], array_column($settings->fields(), 'key'), + DataCleanup::settingKeys(), ); - self::assertSame(0, $settings->fields()[0]['default']); - self::assertSame(1, $settings->fields()[1]['default']); - self::assertSame(['1' => '1', '6' => '6', '15' => '15', '30' => '30'], $settings->fields()[1]['options']); - self::assertSame(1, $settings->fields()[2]['default']); - self::assertFalse($settings->fields()[3]['default']); } public function test_it_contributes_the_whole_ability_catalogue_namespaced_by_slug(): void @@ -84,14 +162,14 @@ public function test_it_contributes_the_whole_ability_catalogue_namespaced_by_sl ], array_column($abilities, 'name')); } - public function test_billing_depends_on_seeing_all_time_and_on_the_host_invoice_ability(): void + public function test_billing_depends_on_seeing_all_time_and_on_both_host_invoice_abilities(): void { ModuleRegistration::register(dirname(__DIR__, 2)); $abilities = array_column(Registry::abilitiesFor(Abilities::SLUG), 'depends_on', 'ability'); self::assertSame( - ['tasks-projects:view-all-time', 'create-invoice'], + ['tasks-projects:view-all-time', 'create-invoice', 'edit-invoice'], $abilities['tasks-projects:invoice-tasks'], ); self::assertSame( @@ -137,15 +215,14 @@ public function deleteCompanyForAll(string $key): void $cleanup->cleanup(); $cleanup->cleanup(); - self::assertSame([ - 'module.tasks-projects.default_rate', - 'module.tasks-projects.rounding_minutes', - 'module.tasks-projects.week_start', - 'module.tasks-projects.members_see_all_time', - 'module.tasks-projects.default_rate', - 'module.tasks-projects.rounding_minutes', - 'module.tasks-projects.week_start', - 'module.tasks-projects.members_see_all_time', - ], $settings->removedCompanyKeys); + $expected = array_map( + static fn (string $key): string => ModuleSettings::PREFIX.$key, + DataCleanup::settingKeys(), + ); + + self::assertSame([...$expected, ...$expected], $settings->removedCompanyKeys); + self::assertContains(ModuleSettings::PREFIX.'rounding_direction', $settings->removedCompanyKeys); + self::assertContains(ModuleSettings::PREFIX.'lock_invoiced_tasks', $settings->removedCompanyKeys); + self::assertContains(ModuleSettings::PREFIX.'invoice_entry_hours', $settings->removedCompanyKeys); } } From b68808983acab89e97ffb03b86802c03565e6814 Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski Date: Tue, 15 Sep 2026 09:36:29 +0200 Subject: [PATCH 6/6] feat(api): move or delete a selection of tasks in one request POST tasks/bulk applies one action to a selection and answers with both lists: the ids that went through and, for each one that did not, the reason it refused. Each task runs in its own transaction through the same service the single-task routes use, so one locked, invoiced or missing task never takes the rest of the selection with it. Only status and delete are bulk actions here. Starting and stopping timers are not, because one running timer per user is a hard invariant of this module and "start these twelve tasks" has no honest meaning. Invoicing is not either, for the opposite reason: it turns the whole selection into one host document and has to refuse mixed customers as a single failure, which belongs in the billing endpoints. --- app/Http/Controllers/BulkTasksController.php | 77 ++++ app/Http/Requests/BulkTasksRequest.php | 35 ++ routes/api.php | 6 + tests/Feature/TasksApiTest.php | 390 +++++++++++++++++++ tests/Unit/ModuleRoutesTest.php | 4 + 5 files changed, 512 insertions(+) create mode 100644 app/Http/Controllers/BulkTasksController.php create mode 100644 app/Http/Requests/BulkTasksRequest.php diff --git a/app/Http/Controllers/BulkTasksController.php b/app/Http/Controllers/BulkTasksController.php new file mode 100644 index 0000000..97a2fb0 --- /dev/null +++ b/app/Http/Controllers/BulkTasksController.php @@ -0,0 +1,77 @@ + */ + public const ACTIONS = [self::ACTION_STATUS, self::ACTION_DELETE]; + + public function __construct(Authorizes $authorizes, private readonly TaskService $tasks) + { + parent::__construct($authorizes); + } + + public function __invoke(BulkTasksRequest $request): JsonResponse + { + $context = $this->context($request); + $validated = $request->validated(); + $action = (string) $validated['action']; + + $this->authorize( + $context, + $action === self::ACTION_DELETE ? Abilities::DELETE_TASK : Abilities::EDIT_TASK, + ); + + $updated = []; + $failed = []; + + foreach (array_map(intval(...), $validated['ids']) as $taskId) { + try { + $action === self::ACTION_DELETE + ? $this->tasks->delete($context->companyId, $taskId) + : $this->tasks->move($context->companyId, $taskId, (int) $validated['task_status_id']); + + $updated[] = $taskId; + } catch (ModelNotFoundException) { + $failed[] = ['id' => $taskId, 'reason' => 'not_found']; + } catch (TasksProjectsException $exception) { + $failed[] = ['id' => $taskId, 'reason' => DomainExceptionRenderer::errorKey($exception)]; + } + } + + return response()->json(['updated' => $updated, 'failed' => $failed]); + } +} diff --git a/app/Http/Requests/BulkTasksRequest.php b/app/Http/Requests/BulkTasksRequest.php new file mode 100644 index 0000000..9987d4c --- /dev/null +++ b/app/Http/Requests/BulkTasksRequest.php @@ -0,0 +1,35 @@ +> */ + public function rules(): array + { + return [ + 'action' => ['required', 'string', 'in:'.implode(',', BulkTasksController::ACTIONS)], + 'ids' => ['required', 'array', 'min:1', 'max:'.self::MAX_IDS], + 'ids.*' => ['integer', 'min:1'], + 'task_status_id' => [ + 'required_if:action,'.BulkTasksController::ACTION_STATUS, + 'prohibited_if:action,'.BulkTasksController::ACTION_DELETE, + 'integer', + 'min:1', + ], + ]; + } +} diff --git a/routes/api.php b/routes/api.php index 5b26abd..82a244c 100644 --- a/routes/api.php +++ b/routes/api.php @@ -3,6 +3,7 @@ use Illuminate\Support\Facades\Route; use Modules\TasksProjects\Http\Controllers\BillingController; use Modules\TasksProjects\Http\Controllers\BoardController; +use Modules\TasksProjects\Http\Controllers\BulkTasksController; use Modules\TasksProjects\Http\Controllers\MembersController; use Modules\TasksProjects\Http\Controllers\ProjectMembersController; use Modules\TasksProjects\Http\Controllers\ProjectsController; @@ -10,6 +11,7 @@ use Modules\TasksProjects\Http\Controllers\SettingsController; use Modules\TasksProjects\Http\Controllers\TasksController; use Modules\TasksProjects\Http\Controllers\TaskStatusesController; +use Modules\TasksProjects\Http\Controllers\TaskTimeLogController; use Modules\TasksProjects\Http\Controllers\TimeEntriesController; use Modules\TasksProjects\Http\Controllers\TimerController; @@ -38,10 +40,14 @@ Route::get('tasks', [TasksController::class, 'index'])->name('tasks-projects.tasks.index'); Route::post('tasks', [TasksController::class, 'store'])->name('tasks-projects.tasks.store'); + Route::post('tasks/bulk', BulkTasksController::class)->name('tasks-projects.tasks.bulk'); Route::get('tasks/{id}', [TasksController::class, 'show'])->name('tasks-projects.tasks.show'); Route::put('tasks/{id}', [TasksController::class, 'update'])->name('tasks-projects.tasks.update'); Route::delete('tasks/{id}', [TasksController::class, 'destroy'])->name('tasks-projects.tasks.destroy'); Route::post('tasks/{id}/move', [TasksController::class, 'move'])->name('tasks-projects.tasks.move'); + Route::post('tasks/{id}/start', [TimerController::class, 'startOnTask'])->name('tasks-projects.tasks.start'); + Route::post('tasks/{id}/stop', [TimerController::class, 'stopOnTask'])->name('tasks-projects.tasks.stop'); + Route::get('tasks/{id}/time-log', TaskTimeLogController::class)->name('tasks-projects.tasks.time-log'); Route::get('board', BoardController::class)->name('tasks-projects.board.index'); diff --git a/tests/Feature/TasksApiTest.php b/tests/Feature/TasksApiTest.php index 66d7c39..1890a64 100644 --- a/tests/Feature/TasksApiTest.php +++ b/tests/Feature/TasksApiTest.php @@ -5,10 +5,12 @@ namespace Modules\TasksProjects\Tests\Feature; use Illuminate\Support\Carbon; +use Illuminate\Support\Facades\DB; use Modules\TasksProjects\Models\Task; use Modules\TasksProjects\Models\TaskStatus; use Modules\TasksProjects\Support\Abilities; use Modules\TasksProjects\Support\Authorizes; +use Modules\TasksProjects\Support\ModuleSettings; use Modules\TasksProjects\Tests\TestCase; final class TasksApiTest extends TestCase @@ -310,6 +312,394 @@ public function test_every_action_refuses_without_its_ability(): void $this->asCompany(self::COMPANY)->getJson('/api/v1/tasks-projects/board')->assertForbidden(); } + public function test_a_task_carries_the_time_logged_against_it(): void + { + $task = $this->makeTask(self::COMPANY); + $this->makeEntry(self::COMPANY, (int) $task->id, ['duration_minutes' => 60, 'amount' => 10000]); + $this->makeEntry(self::COMPANY, (int) $task->id, ['duration_minutes' => 30, 'billable' => false, 'amount' => 0]); + $this->makeEntry(self::COMPANY, (int) $task->id, [ + 'duration_minutes' => 45, + 'amount' => 7500, + 'invoice_id' => 77, + 'invoice_item_id' => 88, + ]); + $running = $this->makeEntry(self::COMPANY, (int) $task->id, [ + 'running_user_id' => self::DEFAULT_USER, + 'ended_at' => null, + 'duration_minutes' => 0, + 'started_at' => Carbon::parse('2026-09-15 09:00:00'), + ]); + + $response = $this->asCompany(self::COMPANY)->getJson('/api/v1/tasks-projects/tasks/'.$task->id); + + $response->assertOk(); + // The running entry has no duration yet, so it is outside every total. + $response->assertJsonPath('data.time.logged_minutes', 135); + $response->assertJsonPath('data.time.billable_minutes', 105); + $response->assertJsonPath('data.time.unbilled_minutes', 60); + $response->assertJsonPath('data.time.unbilled_amount', 10000); + $response->assertJsonPath('data.time.invoiced', 'uninvoiced'); + $response->assertJsonCount(1, 'data.time.running'); + $response->assertJsonPath('data.time.running.0.entry_id', (int) $running->id); + $response->assertJsonPath('data.time.running.0.user_id', self::DEFAULT_USER); + $response->assertJsonPath('data.time.running.0.started_at', $running->started_at->toIso8601String()); + } + + public function test_the_time_block_costs_three_reads_however_long_the_list_is(): void + { + $status = $this->makeStatus(self::COMPANY); + + foreach (range(1, 5) as $index) { + $task = $this->makeTask(self::COMPANY, ['task_status_id' => $status->id, 'name' => 'Task '.$index]); + $this->makeEntry(self::COMPANY, (int) $task->id); + } + + DB::flushQueryLog(); + DB::enableQueryLog(); + + $this->asCompany(self::COMPANY)->getJson('/api/v1/tasks-projects/tasks')->assertOk(); + + $reads = array_filter( + DB::getQueryLog(), + static fn (array $query): bool => str_contains((string) $query['query'], 'tp_time_entries'), + ); + + DB::disableQueryLog(); + + self::assertCount(3, $reads, 'The summary must stay three grouped reads, whatever the page holds.'); + } + + public function test_the_invoiced_state_runs_none_then_uninvoiced_then_invoiced(): void + { + $untouched = $this->makeTask(self::COMPANY, ['name' => 'Untouched']); + $unpaidWork = $this->makeTask(self::COMPANY, ['name' => 'Free']); + $this->makeEntry(self::COMPANY, (int) $unpaidWork->id, ['billable' => false, 'amount' => 0]); + + $partly = $this->makeTask(self::COMPANY, ['name' => 'Partly']); + $this->makeEntry(self::COMPANY, (int) $partly->id, ['invoice_id' => 77]); + $this->makeEntry(self::COMPANY, (int) $partly->id); + + $billed = $this->makeTask(self::COMPANY, ['name' => 'Billed']); + $this->makeEntry(self::COMPANY, (int) $billed->id, ['invoice_id' => 77]); + $this->makeEntry(self::COMPANY, (int) $billed->id, ['invoice_id' => 78]); + $this->makeEntry(self::COMPANY, (int) $billed->id, ['billable' => false, 'amount' => 0]); + + self::assertSame('none', $this->timeOf((int) $untouched->id)['invoiced']); + self::assertSame('none', $this->timeOf((int) $unpaidWork->id)['invoiced']); + self::assertSame('uninvoiced', $this->timeOf((int) $partly->id)['invoiced']); + self::assertSame('invoiced', $this->timeOf((int) $billed->id)['invoiced']); + + // Time nobody may bill still counts as logged time. + self::assertSame(60, $this->timeOf((int) $unpaidWork->id)['logged_minutes']); + self::assertSame(0, $this->timeOf((int) $unpaidWork->id)['billable_minutes']); + } + + public function test_every_task_response_carries_the_time_block(): void + { + $status = $this->makeStatus(self::COMPANY); + $task = $this->makeTask(self::COMPANY, ['task_status_id' => $status->id]); + $this->makeEntry(self::COMPANY, (int) $task->id, ['duration_minutes' => 90, 'amount' => 15000]); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/tasks') + ->assertOk() + ->assertJsonPath('data.0.time.logged_minutes', 90) + ->assertJsonPath('data.0.time.unbilled_amount', 15000); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/board') + ->assertOk() + ->assertJsonPath('data.0.tasks.0.time.logged_minutes', 90); + + $this->asCompany(self::COMPANY) + ->putJson('/api/v1/tasks-projects/tasks/'.$task->id, ['name' => 'Renamed']) + ->assertOk() + ->assertJsonPath('data.time.logged_minutes', 90); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks', ['name' => 'Brand new']) + ->assertCreated() + ->assertJsonPath('data.time.logged_minutes', 0) + ->assertJsonPath('data.time.invoiced', 'none') + ->assertJsonPath('data.time.running', []); + } + + public function test_a_tasks_time_never_counts_another_companys_entries(): void + { + $mine = $this->makeTask(self::COMPANY); + $theirs = $this->makeTask(self::OTHER_COMPANY); + + $this->makeEntry(self::COMPANY, (int) $mine->id, ['duration_minutes' => 60]); + $this->makeEntry(self::OTHER_COMPANY, (int) $theirs->id, ['duration_minutes' => 300]); + + self::assertSame(60, $this->timeOf((int) $mine->id)['logged_minutes']); + } + + public function test_the_invoiced_filter_splits_billed_tasks_from_unbilled_ones(): void + { + $status = $this->makeStatus(self::COMPANY); + $untouched = $this->makeTask(self::COMPANY, ['task_status_id' => $status->id, 'name' => 'Untouched']); + + $unbilled = $this->makeTask(self::COMPANY, ['task_status_id' => $status->id, 'name' => 'Unbilled']); + $this->makeEntry(self::COMPANY, (int) $unbilled->id); + + $partly = $this->makeTask(self::COMPANY, ['task_status_id' => $status->id, 'name' => 'Partly']); + $this->makeEntry(self::COMPANY, (int) $partly->id, ['invoice_id' => 77]); + $this->makeEntry(self::COMPANY, (int) $partly->id); + + $billed = $this->makeTask(self::COMPANY, ['task_status_id' => $status->id, 'name' => 'Billed']); + $this->makeEntry(self::COMPANY, (int) $billed->id, ['invoice_id' => 77]); + + $this->assertListReturns([$unbilled->id, $partly->id], '?invoiced=0'); + $this->assertListReturns([$billed->id], '?invoiced=1'); + $this->assertListReturns( + [$untouched->id, $unbilled->id, $partly->id, $billed->id], + '', + ); + } + + public function test_a_running_clock_alone_does_not_make_a_task_uninvoiced(): void + { + $task = $this->makeTask(self::COMPANY); + $this->makeEntry(self::COMPANY, (int) $task->id, [ + 'running_user_id' => self::DEFAULT_USER, + 'ended_at' => null, + 'duration_minutes' => 0, + ]); + + self::assertSame('none', $this->timeOf((int) $task->id)['invoiced']); + $this->assertListReturns([], '?invoiced=0'); + } + + public function test_auto_start_runs_the_creators_clock_on_the_new_task(): void + { + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'auto_start_tasks', true); + + $response = $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks', ['name' => 'Start me']); + + $response->assertCreated(); + $response->assertJsonCount(1, 'data.time.running'); + $response->assertJsonPath('data.time.running.0.user_id', self::DEFAULT_USER); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/timer') + ->assertJsonPath('data.task_id', $response->json('data.id')); + } + + public function test_auto_start_leaves_a_timer_that_is_already_running_alone(): void + { + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'auto_start_tasks', true); + $busy = $this->makeTask(self::COMPANY, ['name' => 'Busy']); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/timer/start', ['task_id' => $busy->id]) + ->assertCreated(); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks', ['name' => 'Later']) + ->assertCreated() + ->assertJsonPath('data.time.running', []); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/timer') + ->assertJsonPath('data.task_id', (int) $busy->id); + } + + public function test_without_the_setting_a_new_task_starts_no_clock(): void + { + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks', ['name' => 'Quiet']) + ->assertCreated() + ->assertJsonPath('data.time.running', []); + + $this->asCompany(self::COMPANY) + ->getJson('/api/v1/tasks-projects/timer') + ->assertExactJson(['data' => null]); + } + + public function test_the_lock_refuses_to_edit_move_or_delete_an_invoiced_task(): void + { + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'lock_invoiced_tasks', true); + + $status = $this->makeStatus(self::COMPANY); + $target = $this->makeStatus(self::COMPANY, ['name' => 'Done', 'position' => 2, 'is_default' => false]); + $task = $this->makeTask(self::COMPANY, ['task_status_id' => $status->id]); + $this->makeEntry(self::COMPANY, (int) $task->id, ['invoice_id' => 77, 'invoice_item_id' => 88]); + + $this->asCompany(self::COMPANY) + ->putJson('/api/v1/tasks-projects/tasks/'.$task->id, ['name' => 'Renamed']) + ->assertStatus(422) + ->assertJsonPath('error', 'task_locked'); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/'.$task->id.'/move', ['task_status_id' => $target->id]) + ->assertStatus(422) + ->assertJsonPath('error', 'task_locked'); + + $this->asCompany(self::COMPANY) + ->deleteJson('/api/v1/tasks-projects/tasks/'.$task->id) + ->assertStatus(422) + ->assertJsonPath('error', 'task_locked'); + + self::assertSame('Build the landing page', (string) Task::query()->findOrFail($task->id)->name); + } + + public function test_the_lock_leaves_a_task_that_is_only_partly_invoiced_editable(): void + { + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'lock_invoiced_tasks', true); + + $task = $this->makeTask(self::COMPANY); + $this->makeEntry(self::COMPANY, (int) $task->id, ['invoice_id' => 77]); + $this->makeEntry(self::COMPANY, (int) $task->id); + + $this->asCompany(self::COMPANY) + ->putJson('/api/v1/tasks-projects/tasks/'.$task->id, ['name' => 'Still moving']) + ->assertOk() + ->assertJsonPath('data.name', 'Still moving'); + } + + public function test_an_invoiced_task_is_editable_while_the_lock_is_off(): void + { + $task = $this->makeTask(self::COMPANY); + $this->makeEntry(self::COMPANY, (int) $task->id, ['invoice_id' => 77]); + + $this->asCompany(self::COMPANY) + ->putJson('/api/v1/tasks-projects/tasks/'.$task->id, ['name' => 'Renamed']) + ->assertOk() + ->assertJsonPath('data.name', 'Renamed'); + } + + public function test_a_bulk_status_change_moves_what_it_can_and_reports_what_it_cannot(): void + { + $this->settings->putCompany(self::COMPANY, ModuleSettings::PREFIX.'lock_invoiced_tasks', true); + + $backlog = $this->makeStatus(self::COMPANY); + $done = $this->makeStatus(self::COMPANY, ['name' => 'Done', 'position' => 2, 'is_default' => false, 'is_closed' => true]); + + $first = $this->makeTask(self::COMPANY, ['task_status_id' => $backlog->id]); + $second = $this->makeTask(self::COMPANY, ['task_status_id' => $backlog->id]); + $locked = $this->makeTask(self::COMPANY, ['task_status_id' => $backlog->id]); + $this->makeEntry(self::COMPANY, (int) $locked->id, ['invoice_id' => 77]); + $foreign = $this->makeTask(self::OTHER_COMPANY); + + $response = $this->asCompany(self::COMPANY)->postJson('/api/v1/tasks-projects/tasks/bulk', [ + 'action' => 'status', + 'task_status_id' => $done->id, + 'ids' => [$first->id, $second->id, $locked->id, $foreign->id], + ]); + + $response->assertOk(); + $response->assertExactJson([ + 'updated' => [(int) $first->id, (int) $second->id], + 'failed' => [ + ['id' => (int) $locked->id, 'reason' => 'task_locked'], + ['id' => (int) $foreign->id, 'reason' => 'not_found'], + ], + ]); + + self::assertSame((int) $done->id, (int) Task::query()->findOrFail($first->id)->task_status_id); + self::assertNotNull(Task::query()->findOrFail($second->id)->closed_at); + self::assertSame((int) $backlog->id, (int) Task::query()->findOrFail($locked->id)->task_status_id); + } + + public function test_a_bulk_delete_keeps_the_tasks_it_may_not_delete(): void + { + $status = $this->makeStatus(self::COMPANY); + $gone = $this->makeTask(self::COMPANY, ['task_status_id' => $status->id]); + $invoiced = $this->makeTask(self::COMPANY, ['task_status_id' => $status->id]); + $this->makeEntry(self::COMPANY, (int) $invoiced->id, ['invoice_id' => 77, 'invoice_item_id' => 88]); + + $response = $this->asCompany(self::COMPANY)->postJson('/api/v1/tasks-projects/tasks/bulk', [ + 'action' => 'delete', + 'ids' => [$gone->id, $invoiced->id], + ]); + + $response->assertOk(); + $response->assertExactJson([ + 'updated' => [(int) $gone->id], + 'failed' => [['id' => (int) $invoiced->id, 'reason' => 'entries_already_invoiced']], + ]); + + self::assertNull(Task::query()->find($gone->id)); + self::assertNotNull(Task::query()->find($invoiced->id)); + } + + public function test_a_bulk_request_is_checked_before_anything_moves(): void + { + $task = $this->makeTask(self::COMPANY); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/bulk', ['action' => 'archive', 'ids' => [$task->id]]) + ->assertStatus(422) + ->assertJsonValidationErrors(['action']); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/bulk', ['action' => 'status', 'ids' => [$task->id]]) + ->assertStatus(422) + ->assertJsonValidationErrors(['task_status_id']); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/bulk', ['action' => 'delete', 'ids' => []]) + ->assertStatus(422) + ->assertJsonValidationErrors(['ids']); + + // Deleting is not a status change with an extra field attached. + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/bulk', [ + 'action' => 'delete', + 'ids' => [$task->id], + 'task_status_id' => $task->task_status_id, + ]) + ->assertStatus(422) + ->assertJsonValidationErrors(['task_status_id']); + + self::assertNotNull(Task::query()->find($task->id)); + } + + public function test_each_bulk_action_checks_its_own_ability(): void + { + $task = $this->makeTask(self::COMPANY); + + $this->authorization->deny(Authorizes::id(Abilities::DELETE_TASK)); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/bulk', ['action' => 'delete', 'ids' => [$task->id]]) + ->assertForbidden(); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/bulk', [ + 'action' => 'status', + 'task_status_id' => $task->task_status_id, + 'ids' => [$task->id], + ]) + ->assertOk(); + + $this->authorization->deny(Authorizes::id(Abilities::EDIT_TASK)); + + $this->asCompany(self::COMPANY) + ->postJson('/api/v1/tasks-projects/tasks/bulk', [ + 'action' => 'status', + 'task_status_id' => $task->task_status_id, + 'ids' => [$task->id], + ]) + ->assertForbidden(); + } + + /** + * The time block of one task, as the API answers it. + * + * @return array + */ + private function timeOf(int $taskId): array + { + $response = $this->asCompany(self::COMPANY)->getJson('/api/v1/tasks-projects/tasks/'.$taskId); + + $response->assertOk(); + + return (array) $response->json('data.time'); + } + /** @param list $expected */ private function assertListReturns(array $expected, string $query): void { diff --git a/tests/Unit/ModuleRoutesTest.php b/tests/Unit/ModuleRoutesTest.php index c402200..fabc679 100644 --- a/tests/Unit/ModuleRoutesTest.php +++ b/tests/Unit/ModuleRoutesTest.php @@ -47,10 +47,14 @@ public function test_it_registers_the_documented_route_table(): void ['GET', 'api/v1/tasks-projects/members', 'tasks-projects.members.index'], ['GET', 'api/v1/tasks-projects/tasks', 'tasks-projects.tasks.index'], ['POST', 'api/v1/tasks-projects/tasks', 'tasks-projects.tasks.store'], + ['POST', 'api/v1/tasks-projects/tasks/bulk', 'tasks-projects.tasks.bulk'], ['GET', 'api/v1/tasks-projects/tasks/{id}', 'tasks-projects.tasks.show'], ['PUT', 'api/v1/tasks-projects/tasks/{id}', 'tasks-projects.tasks.update'], ['DELETE', 'api/v1/tasks-projects/tasks/{id}', 'tasks-projects.tasks.destroy'], ['POST', 'api/v1/tasks-projects/tasks/{id}/move', 'tasks-projects.tasks.move'], + ['POST', 'api/v1/tasks-projects/tasks/{id}/start', 'tasks-projects.tasks.start'], + ['POST', 'api/v1/tasks-projects/tasks/{id}/stop', 'tasks-projects.tasks.stop'], + ['GET', 'api/v1/tasks-projects/tasks/{id}/time-log', 'tasks-projects.tasks.time-log'], ['GET', 'api/v1/tasks-projects/board', 'tasks-projects.board.index'], ['GET', 'api/v1/tasks-projects/task-statuses', 'tasks-projects.task-statuses.index'], ['POST', 'api/v1/tasks-projects/task-statuses', 'tasks-projects.task-statuses.store'],