From 2998f43e6dfe50b2303e5b55cd17d89f1a8b7568 Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Wed, 29 Jul 2026 15:43:03 +0200 Subject: [PATCH 01/14] Entries referencing deleted users or tickets no longer fail the whole request. Added userId to timesheets and stopped resolving ticket status against the session project when a ticket has no project. Added PHPUnit setup and a Taskfile for running tests. --- .claude/settings.json | 9 ++ .github/workflows/pr.yml | 22 ++++ .gitignore | 2 + CHANGELOG.md | 6 + Model/DeletedData.php | 4 +- Model/MilestoneData.php | 4 +- Model/ProjectData.php | 2 +- Model/TicketData.php | 4 +- Model/TimesheetData.php | 9 +- Repositories/ApiDataRepository.php | 2 +- Services/APIData.php | 29 +++-- Taskfile.yml | 62 +++++++++ bin/release-exclude.txt | 5 + compose.yml | 1 + composer.json | 13 ++ phpunit.xml.dist | 21 +++ tests/Model/DeletedDataTest.php | 25 ++++ tests/Model/MilestoneDataTest.php | 25 ++++ tests/Model/ProjectDataTest.php | 22 ++++ tests/Model/TicketDataTest.php | 45 +++++++ tests/Model/TimesheetDataTest.php | 97 ++++++++++++++ tests/Service/APIDataTest.php | 197 +++++++++++++++++++++++++++++ tests/Stub/LeantimeTickets.php | 40 ++++++ 23 files changed, 624 insertions(+), 22 deletions(-) create mode 100644 .claude/settings.json create mode 100644 Taskfile.yml create mode 100644 phpunit.xml.dist create mode 100644 tests/Model/DeletedDataTest.php create mode 100644 tests/Model/MilestoneDataTest.php create mode 100644 tests/Model/ProjectDataTest.php create mode 100644 tests/Model/TicketDataTest.php create mode 100644 tests/Model/TimesheetDataTest.php create mode 100644 tests/Service/APIDataTest.php create mode 100644 tests/Stub/LeantimeTickets.php diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000..56fc68a --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,9 @@ +{ + "permissions": { + "allow": [ + "Bash(task test:*)", + "Bash(task composer:*)", + "Bash(task lint:*)" + ] + } +} diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 4ed1f3c..a32163b 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -15,3 +15,25 @@ jobs: - name: Check that changelog has been updated. run: git diff --exit-code origin/${{ github.base_ref }} -- CHANGELOG.md && exit 1 || exit 0 + + test: + runs-on: ubuntu-latest + name: Unit tests + strategy: + fail-fast: false + steps: + - name: Checkout + uses: actions/checkout@v4 + + # Matches the PHP version in Dockerfile (itkdev/php8.3-fpm). + - name: Setup PHP + uses: shivammathur/setup-php@v2 + with: + php-version: '8.3' + coverage: none + + - name: Install dependencies + run: composer install --no-interaction --prefer-dist --no-progress + + - name: Run tests + run: vendor/bin/phpunit diff --git a/.gitignore b/.gitignore index e0d067c..a3fd9f3 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,5 @@ composer.lock release/ checksum.txt *.tar.gz + +economics diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bfa34e..e6c8054 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## [Unreleased] +* [PR-14](https://github.com/ITK-Leantime/data-api/pull/14) + * Allowed null values in API models, so entries referencing deleted users or deleted tickets no longer fail the whole request. + * Added userId to timesheets, so hours logged by a deleted user stay attributable. + * Stopped resolving ticket status against the session's project when a ticket has no project. + * Added PHPUnit test setup and a Taskfile for running it. + ## [0.1.2] - 2026-03-06 * [PR-12](https://github.com/ITK-Leantime/data-api/pull/12) diff --git a/Model/DeletedData.php b/Model/DeletedData.php index 74e379c..9433a0c 100644 --- a/Model/DeletedData.php +++ b/Model/DeletedData.php @@ -7,7 +7,7 @@ class DeletedData { public function __construct( - public int $id, - public CarbonInterface $deletedDate, + public ?int $id, + public ?CarbonInterface $deletedDate, ) {} } diff --git a/Model/MilestoneData.php b/Model/MilestoneData.php index 0a06bd2..f222b7c 100644 --- a/Model/MilestoneData.php +++ b/Model/MilestoneData.php @@ -8,8 +8,8 @@ { public function __construct( public int $id, - public int $projectId, - public string $name, + public ?int $projectId, + public ?string $name, public ?CarbonInterface $modified, ) {} } diff --git a/Model/ProjectData.php b/Model/ProjectData.php index 0c07e16..388ada8 100644 --- a/Model/ProjectData.php +++ b/Model/ProjectData.php @@ -8,7 +8,7 @@ { public function __construct( public int $id, - public string $name, + public ?string $name, public ?CarbonInterface $modified, ) {} } diff --git a/Model/TicketData.php b/Model/TicketData.php index 8826a6a..6ff561f 100644 --- a/Model/TicketData.php +++ b/Model/TicketData.php @@ -8,8 +8,8 @@ { public function __construct( public int $id, - public int $projectId, - public string $name, + public ?int $projectId, + public ?string $name, public ?string $status, public ?int $milestoneId, public array $tags, diff --git a/Model/TimesheetData.php b/Model/TimesheetData.php index b14c145..cd726b5 100644 --- a/Model/TimesheetData.php +++ b/Model/TimesheetData.php @@ -8,13 +8,14 @@ { public function __construct( public int $id, - public int $ticketId, - public int $projectId, + public ?int $ticketId, + public ?int $projectId, public ?string $description, public float $hours, - public string $username, + public ?int $userId, + public ?string $username, + public ?string $kind, public ?CarbonInterface $workDate = null, public ?CarbonInterface $modified = null, - public string $kind, ) {} } diff --git a/Repositories/ApiDataRepository.php b/Repositories/ApiDataRepository.php index f5cc80a..e6460db 100644 --- a/Repositories/ApiDataRepository.php +++ b/Repositories/ApiDataRepository.php @@ -65,7 +65,7 @@ public function getTimesheets(int $startId, int $limit, ?int $modifiedAfter = nu { return $this->query() ->from("zp_timesheets", "timesheet") - ->select(["timesheet.id", "timesheet.description", "timesheet.hours", "timesheet.workDate", "timesheet.modified", "timesheet.ticketId", "timesheet.kind", "user.username", "ticket.projectId"]) + ->select(["timesheet.id", "timesheet.description", "timesheet.hours", "timesheet.workDate", "timesheet.modified", "timesheet.ticketId", "timesheet.userId", "timesheet.kind", "user.username", "ticket.projectId"]) ->where("timesheet.id", ">=", $startId) ->whereNotNull("timesheet.hours") ->leftJoin('zp_user as user', "user.id", "=", "timesheet.userId") diff --git a/Services/APIData.php b/Services/APIData.php index 8929f87..af28091 100644 --- a/Services/APIData.php +++ b/Services/APIData.php @@ -144,7 +144,12 @@ public function getTickets(int $startId, int $limit, int $modifiedAfter = null, $values = $this->apiDataRepository->getTickets($startId, $limit, $modifiedAfter, $ids, $projectIds); return array_map(function ($value) { - $projectStatuses = $this->ticketRepository->getStateLabels($value->projectId); + // Asked for labels without a project id, Leantime falls back to + // session('currentProject'), which would resolve the status against + // an unrelated project. + $projectStatuses = $value->projectId !== null + ? $this->ticketRepository->getStateLabels($value->projectId) + : []; return new TicketData( $value->id, @@ -168,16 +173,20 @@ public function getTimesheets(int $startId, int $limit, ?int $modifiedAfter = nu $values = $this->apiDataRepository->getTimesheets($startId, $limit, $modifiedAfter, $ids, $projectIds); return array_map(function ($value) { + // Named arguments: CarbonImmutable has a __toString(), so a + // mis-ordered date would be coerced into one of the string + // parameters instead of raising a TypeError. return new TimesheetData( - $value->id, - $value->ticketId, - $value->projectId, - $value->description, - $value->hours, - $value->username, - $this->getCarbonFromDatabaseValue($value->workDate), - $this->getCarbonFromDatabaseValue($value->modified), - $value->kind, + id: $value->id, + ticketId: $value->ticketId, + projectId: $value->projectId, + description: $value->description, + hours: $value->hours, + userId: $value->userId, + username: $value->username, + kind: $value->kind, + workDate: $this->getCarbonFromDatabaseValue($value->workDate), + modified: $this->getCarbonFromDatabaseValue($value->modified), ); }, $values); } diff --git a/Taskfile.yml b/Taskfile.yml new file mode 100644 index 0000000..4601cfe --- /dev/null +++ b/Taskfile.yml @@ -0,0 +1,62 @@ +# https://taskfile.dev — install with `brew install go-task` (or see docs). +# Run `task` (or `task --list-all`) to see all available commands. +# +# This plugin has no long-running stack, so everything runs in a one-off +# container built from the local Dockerfile (itkdev/php8.3-fpm). + +version: "3" + +vars: + # https://taskfile.dev/reference/templating/ + DOCKER_COMPOSE: '{{ .TASK_DOCKER_COMPOSE | default "docker compose" }}' + +tasks: + default: + desc: List all tasks + cmds: + - task --list-all + silent: true + + # -------------------------------------------------------------- Wrappers --- + + compose: + desc: "Run a docker compose command. Example: task compose -- build php." + cmds: + - "{{ .DOCKER_COMPOSE }} {{ .CLI_ARGS }}" + + php: + desc: "Run a command in a one-off php container. Example: task php -- php --version." + cmds: + - task compose -- run --rm --no-deps php {{ .CLI_ARGS }} + silent: true + + composer: + desc: "Run a composer command. Example: task composer -- install." + cmds: + - task php -- composer {{ .CLI_ARGS }} + silent: true + + # ------------------------------------------------------------ Lifecycle --- + + setup: + desc: Build the php image and install dev dependencies. + cmds: + - task compose -- build php + - task composer -- install + + # ------------------------------------------------------------ PHP tests --- + + test: + desc: "Run the PHP test suite. Example: task test -- --filter TimesheetData." + cmds: + - task php -- vendor/bin/phpunit {{ .CLI_ARGS }} + silent: true + + lint: + desc: Syntax-check all PHP files. + cmds: + - >- + task php -- sh -c + 'find Controllers Model Repositories Services -name "*.php" -print0 + | xargs -0 -n1 -- php -l' + silent: true diff --git a/bin/release-exclude.txt b/bin/release-exclude.txt index 3331c0c..0e9974d 100755 --- a/bin/release-exclude.txt +++ b/bin/release-exclude.txt @@ -1,6 +1,7 @@ *.tar.gz .git* .php-cs-fixer.dist.php +.phpunit.cache .twig-cs-fixer.dist.php bin checksum.txt @@ -8,3 +9,7 @@ compose.yaml composer.lock vendor Dockerfile +phpunit.xml +phpunit.xml.dist +Taskfile.yml +tests diff --git a/compose.yml b/compose.yml index b40cbb9..908a2c5 100644 --- a/compose.yml +++ b/compose.yml @@ -1,5 +1,6 @@ services: php: build: . + working_dir: /app volumes: - .:/app diff --git a/composer.json b/composer.json index bb27426..f1e60a9 100644 --- a/composer.json +++ b/composer.json @@ -12,9 +12,22 @@ ], "homepage": "https://github.com/ITK-Leantime/leantime-data-api", "require-dev": { + "illuminate/database": "^11.0", + "nesbot/carbon": "^2.72.2 || ^3.0", + "phpunit/phpunit": "^11.0" }, "config": { }, + "autoload-dev": { + "psr-4": { + "Leantime\\Plugins\\APIData\\": "", + "Leantime\\Plugins\\APIData\\Tests\\": "tests/" + }, + "classmap": [ + "tests/Stub/" + ] + }, "scripts": { + "test": "phpunit" } } diff --git a/phpunit.xml.dist b/phpunit.xml.dist new file mode 100644 index 0000000..2a306e4 --- /dev/null +++ b/phpunit.xml.dist @@ -0,0 +1,21 @@ + + + + + tests + tests/Stub + + + + + Model + Services + + + diff --git a/tests/Model/DeletedDataTest.php b/tests/Model/DeletedDataTest.php new file mode 100644 index 0000000..26e42d2 --- /dev/null +++ b/tests/Model/DeletedDataTest.php @@ -0,0 +1,25 @@ +assertNull($deleted->id); + $this->assertNull($deleted->deletedDate); + } +} diff --git a/tests/Model/MilestoneDataTest.php b/tests/Model/MilestoneDataTest.php new file mode 100644 index 0000000..3ebb4ac --- /dev/null +++ b/tests/Model/MilestoneDataTest.php @@ -0,0 +1,25 @@ +assertSame(5, $milestone->id); + $this->assertNull($milestone->projectId); + $this->assertNull($milestone->name); + } +} diff --git a/tests/Model/ProjectDataTest.php b/tests/Model/ProjectDataTest.php new file mode 100644 index 0000000..859a12d --- /dev/null +++ b/tests/Model/ProjectDataTest.php @@ -0,0 +1,22 @@ +assertSame(92, $project->id); + $this->assertNull($project->name); + } +} diff --git a/tests/Model/TicketDataTest.php b/tests/Model/TicketDataTest.php new file mode 100644 index 0000000..069fd5b --- /dev/null +++ b/tests/Model/TicketDataTest.php @@ -0,0 +1,45 @@ +makeTicket(['projectId' => null, 'name' => null]); + + $this->assertNull($ticket->projectId); + $this->assertNull($ticket->name); + } + + /** + * @param array $overrides + */ + private function makeTicket(array $overrides = []): TicketData + { + return new TicketData(...[ + 'id' => 4711, + 'projectId' => 92, + 'name' => 'Fix the thing', + 'status' => 'NEW', + 'milestoneId' => null, + 'tags' => [], + 'worker' => 'anne@aarhus.dk', + 'plannedHours' => null, + 'remainingHours' => null, + 'dueDate' => null, + 'resolutionDate' => null, + 'modified' => null, + ...$overrides, + ]); + } +} diff --git a/tests/Model/TimesheetDataTest.php b/tests/Model/TimesheetDataTest.php new file mode 100644 index 0000000..12ac0c9 --- /dev/null +++ b/tests/Model/TimesheetDataTest.php @@ -0,0 +1,97 @@ +makeTimesheet(['username' => null]); + + $this->assertNull($timesheet->username); + $this->assertSame(7.5, $timesheet->hours); + } + + /** + * `ticket.projectId` comes from the other left join, so a timesheet whose + * ticket was deleted has neither a ticket id nor a project id. + */ + public function testAcceptsNullTicketIdAndProjectIdWhenTheTicketWasDeleted(): void + { + $timesheet = $this->makeTimesheet(['ticketId' => null, 'projectId' => null]); + + $this->assertNull($timesheet->ticketId); + $this->assertNull($timesheet->projectId); + } + + public function testAcceptsNullKind(): void + { + $timesheet = $this->makeTimesheet(['kind' => null]); + + $this->assertNull($timesheet->kind); + } + + /** + * Hours logged by a deleted user stay attributable through the user id, so + * a consumer can group them per departed worker instead of collapsing them + * into one anonymous pile. + */ + public function testExposesUserIdSoOrphanedHoursStayAttributable(): void + { + $timesheet = $this->makeTimesheet(['userId' => 57, 'username' => null]); + + $this->assertSame(57, $timesheet->userId); + $this->assertNull($timesheet->username); + } + + /** + * `$kind` has to move ahead of the optional date parameters, since a + * required parameter after an optional one is deprecated. CarbonImmutable + * implements __toString() and this codebase does not use strict_types, so a + * date landing in a string parameter would be coerced silently instead of + * raising a TypeError. Pin the declarations so a swap cannot go unnoticed. + */ + public function testKindHoldsAStatusStringAndDatesHoldCarbonInstances(): void + { + $timesheet = $this->makeTimesheet([ + 'kind' => 'GENERAL_BILLABLE', + 'workDate' => CarbonImmutable::parse('2026-03-02 09:00:00'), + ]); + + $this->assertSame('GENERAL_BILLABLE', $timesheet->kind); + $this->assertInstanceOf(CarbonInterface::class, $timesheet->workDate); + $this->assertSame('2026-03-02 09:00:00', $timesheet->workDate->format('Y-m-d H:i:s')); + } + + /** + * @param array $overrides + */ + private function makeTimesheet(array $overrides = []): TimesheetData + { + return new TimesheetData(...[ + 'id' => 1, + 'ticketId' => 2, + 'projectId' => 3, + 'description' => 'Worked on stuff', + 'hours' => 7.5, + 'userId' => 57, + 'username' => 'anne@aarhus.dk', + 'kind' => 'GENERAL_BILLABLE', + 'workDate' => null, + 'modified' => null, + ...$overrides, + ]); + } +} diff --git a/tests/Service/APIDataTest.php b/tests/Service/APIDataTest.php new file mode 100644 index 0000000..6d1435d --- /dev/null +++ b/tests/Service/APIDataTest.php @@ -0,0 +1,197 @@ +makeServiceReturningTimesheets([ + $this->timesheetRow(['username' => null, 'userId' => 57]), + ]); + + $timesheets = $service->getTimesheets(0, 100); + + $this->assertCount(1, $timesheets); + $this->assertNull($timesheets[0]->username); + $this->assertSame(57, $timesheets[0]->userId); + $this->assertSame(7.5, $timesheets[0]->hours); + } + + /** + * Guards the constructor argument order. `$kind` moved ahead of the two + * optional date parameters, and CarbonImmutable implements __toString(), so + * a mis-ordered date would land in `$kind` as a coerced string instead of + * raising a TypeError. Only asserting each field individually catches that. + */ + public function testGetTimesheetsMapsEveryFieldToItsOwnProperty(): void + { + $service = $this->makeServiceReturningTimesheets([$this->timesheetRow()]); + + $timesheet = $service->getTimesheets(0, 100)[0]; + + $this->assertSame(4711, $timesheet->id); + $this->assertSame(12, $timesheet->ticketId); + $this->assertSame(92, $timesheet->projectId); + $this->assertSame('Worked on stuff', $timesheet->description); + $this->assertSame(7.5, $timesheet->hours); + $this->assertSame(57, $timesheet->userId); + $this->assertSame('anne@aarhus.dk', $timesheet->username); + $this->assertSame('GENERAL_BILLABLE', $timesheet->kind); + + $this->assertInstanceOf(CarbonInterface::class, $timesheet->workDate); + $this->assertSame('2026-03-02 09:00:00', $timesheet->workDate->format('Y-m-d H:i:s')); + $this->assertSame('UTC', $timesheet->workDate->timezoneName); + + $this->assertInstanceOf(CarbonInterface::class, $timesheet->modified); + $this->assertSame('2026-03-03 11:30:00', $timesheet->modified->format('Y-m-d H:i:s')); + } + + /** + * Leantime stores "no date" as the zero date rather than null. Carbon throws + * on a format mismatch, so this guard has to stay in place. + */ + public function testGetTimesheetsTreatsTheZeroDateSentinelAsNull(): void + { + $service = $this->makeServiceReturningTimesheets([ + $this->timesheetRow(['workDate' => '0000-00-00 00:00:00', 'modified' => null]), + ]); + + $timesheet = $service->getTimesheets(0, 100)[0]; + + $this->assertNull($timesheet->workDate); + $this->assertNull($timesheet->modified); + } + + /** + * Leantime's `getStateLabels()` falls back to `session('currentProject')` + * when it is handed a null project id, so asking it for labels would return + * some unrelated project's status list and put a wrong status on the ticket. + * A ticket without a project has no status to resolve. + */ + public function testGetTicketsDoesNotLookUpStatusLabelsForATicketWithoutAProject(): void + { + $ticketRepository = $this->createMock(TicketRepository::class); + $ticketRepository->expects($this->never())->method('getStateLabels'); + + $repository = $this->createMock(ApiDataRepository::class); + $repository->method('getTickets')->willReturn([$this->ticketRow(['projectId' => null])]); + + $tickets = (new APIData($ticketRepository, $repository))->getTickets(0, 100); + + $this->assertNull($tickets[0]->projectId); + $this->assertNull($tickets[0]->status); + } + + /** + * The normal path still has to resolve the status against the ticket's own + * project. + */ + public function testGetTicketsResolvesStatusFromTheProjectsOwnLabels(): void + { + $ticketRepository = $this->createMock(TicketRepository::class); + $ticketRepository->expects($this->once()) + ->method('getStateLabels') + ->with(92) + ->willReturn($this->stateLabels()); + + $repository = $this->createMock(ApiDataRepository::class); + $repository->method('getTickets')->willReturn([ + $this->ticketRow(['projectId' => 92, 'status' => 3]), + ]); + + $tickets = (new APIData($ticketRepository, $repository))->getTickets(0, 100); + + $this->assertSame(92, $tickets[0]->projectId); + $this->assertSame('NEW', $tickets[0]->status); + } + + /** + * @param list $rows + */ + private function makeServiceReturningTimesheets(array $rows): APIData + { + $repository = $this->createMock(ApiDataRepository::class); + $repository->method('getTimesheets')->willReturn($rows); + + return new APIData($this->createMock(TicketRepository::class), $repository); + } + + /** + * A row as the repository returns it — the select list in + * `ApiDataRepository::getTimesheets()` decides these keys. + * + * @param array $overrides + */ + private function timesheetRow(array $overrides = []): object + { + return (object) array_merge([ + 'id' => 4711, + 'ticketId' => 12, + 'projectId' => 92, + 'description' => 'Worked on stuff', + 'hours' => 7.5, + 'userId' => 57, + 'username' => 'anne@aarhus.dk', + 'kind' => 'GENERAL_BILLABLE', + 'workDate' => '2026-03-02 09:00:00', + 'modified' => '2026-03-03 11:30:00', + ], $overrides); + } + + /** + * A row as `ApiDataRepository::getTickets()` returns it. + * + * @param array $overrides + */ + private function ticketRow(array $overrides = []): object + { + return (object) array_merge([ + 'id' => 4711, + 'projectId' => 92, + 'headline' => 'Fix the thing', + 'status' => 3, + 'milestoneid' => 0, + 'tags' => '', + 'username' => 'anne@aarhus.dk', + 'planHours' => null, + 'hourRemaining' => null, + 'dateToFinish' => null, + 'editTo' => null, + 'modified' => null, + ], $overrides); + } + + /** + * Mirrors the shape of Leantime v3.9.7's `$statusListSeed`: keyed by the + * integer status id, each entry carrying a `statusType`. + * + * @return array> + */ + private function stateLabels(): array + { + return [ + 3 => [ + 'name' => 'status.new', + 'class' => 'label-info', + 'statusType' => 'NEW', + 'kanbanCol' => true, + 'sortKey' => 1, + ], + ]; + } +} diff --git a/tests/Stub/LeantimeTickets.php b/tests/Stub/LeantimeTickets.php new file mode 100644 index 0000000..be2abf8 --- /dev/null +++ b/tests/Stub/LeantimeTickets.php @@ -0,0 +1,40 @@ + [ + * 'name' => 'status.new', + * 'class' => 'label-info', + * 'statusType' => 'NEW', + * 'kanbanCol' => true, + * 'sortKey' => 1, + * ] + * + * Note that Leantime falls back to `session('currentProject')` when $projectId + * is null, which is why `APIData::getTickets()` must not call this with a null + * project id. + */ +class Tickets +{ + public function getStateLabels($projectId = null): array + { + throw new \LogicException( + 'Leantime\Domain\Tickets\Repositories\Tickets is a test stub and must be mocked.' + ); + } +} From cb0fdb4f7e8bd664775a41e9e701d40ff9d31436 Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Thu, 30 Jul 2026 07:41:36 +0200 Subject: [PATCH 02/14] docs: updated changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6c8054..2d0ae4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## [Unreleased] -* [PR-14](https://github.com/ITK-Leantime/data-api/pull/14) +* [PR-18](https://github.com/ITK-Leantime/data-api/pull/18) * Allowed null values in API models, so entries referencing deleted users or deleted tickets no longer fail the whole request. * Added userId to timesheets, so hours logged by a deleted user stay attributable. * Stopped resolving ticket status against the session's project when a ticket has no project. From 6961e0199054673d576ab8c949bdc64e3896ccff Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:49:26 +0200 Subject: [PATCH 03/14] feat: validated request parameters, so malformed input answers 400 instead of 500 --- CHANGELOG.md | 6 + Controllers/API.php | 61 +++---- Model/CoercesRequestInput.php | 74 +++++++++ Model/DeletedRequestParameters.php | 81 ++++++++++ Model/InvalidRequestException.php | 10 ++ Model/RequestParameters.php | 84 ++++++++++ README.md | 15 +- tests/Model/DeletedRequestParametersTest.php | 75 +++++++++ tests/Model/RequestParametersTest.php | 158 +++++++++++++++++++ 9 files changed, 534 insertions(+), 30 deletions(-) create mode 100644 Model/CoercesRequestInput.php create mode 100644 Model/DeletedRequestParameters.php create mode 100644 Model/InvalidRequestException.php create mode 100644 Model/RequestParameters.php create mode 100644 tests/Model/DeletedRequestParametersTest.php create mode 100644 tests/Model/RequestParametersTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d0ae4b..cdecd7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## [Unreleased] +* [PR-19](https://github.com/ITK-Leantime/data-api/pull/19) + * Validated request parameters, so malformed input answers 400 with a reason instead of failing with a 500. + * Rejected a limit below 1, which previously dropped the LIMIT clause and returned every row, and capped limit at 1000. + * Accepted comma separated ids, projectIds and types, since the endpoints are documented as GET with query parameters. + * Defaulted types to all supported types on the deleted endpoint, and stopped an unknown type reaching the error page. + * [PR-18](https://github.com/ITK-Leantime/data-api/pull/18) * Allowed null values in API models, so entries referencing deleted users or deleted tickets no longer fail the whole request. * Added userId to timesheets, so hours logged by a deleted user stay attributable. diff --git a/Controllers/API.php b/Controllers/API.php index 548c1e6..95aec9b 100644 --- a/Controllers/API.php +++ b/Controllers/API.php @@ -3,6 +3,9 @@ namespace Leantime\Plugins\APIData\Controllers; use Leantime\Core\Controller\Controller; +use Leantime\Plugins\APIData\Model\DeletedRequestParameters; +use Leantime\Plugins\APIData\Model\InvalidRequestException; +use Leantime\Plugins\APIData\Model\RequestParameters; use Leantime\Plugins\APIData\Model\ResponseData; use Leantime\Plugins\APIData\Services\APIData; use Symfony\Component\HttpFoundation\JsonResponse; @@ -21,49 +24,61 @@ public function init(APIData $dataAPIService): void public function deleted(array $input): JsonResponse { - return new JsonResponse($this->getDeleted($input)); + return $this->respond(fn () => $this->getDeleted($input)); } public function projects(array $input): JsonResponse { - return new JsonResponse($this->getResults($input, APIData::TYPE_PROJECTS)); + return $this->respond(fn () => $this->getResults($input, APIData::TYPE_PROJECTS)); } public function milestones(array $input): JsonResponse { - return new JsonResponse($this->getResults($input, APIData::TYPE_MILESTONES)); + return $this->respond(fn () => $this->getResults($input, APIData::TYPE_MILESTONES)); } public function tickets(array $input): JsonResponse { - return new JsonResponse($this->getResults($input, APIData::TYPE_TICKETS)); + return $this->respond(fn () => $this->getResults($input, APIData::TYPE_TICKETS)); } public function timesheets(array $input): JsonResponse { - return new JsonResponse($this->getResults($input, APIData::TYPE_TIMESHEETS)); + return $this->respond(fn () => $this->getResults($input, APIData::TYPE_TIMESHEETS)); } public function workers(array $input): JsonResponse { - return new JsonResponse($this->getResults($input, APIData::TYPE_WORKERS)); + return $this->respond(fn () => $this->getResults($input, APIData::TYPE_WORKERS)); + } + + /** + * A parameter the caller got wrong is their error, not ours, so it answers + * 400 with the reason instead of Leantime's 500 error page. + */ + private function respond(callable $resolve): JsonResponse + { + try { + return new JsonResponse($resolve()); + } catch (InvalidRequestException $exception) { + return new JsonResponse(['error' => $exception->getMessage()], JsonResponse::HTTP_BAD_REQUEST); + } } private function getDeleted(array $input): array { - $types = $input['types']; - $deleted = $input['deleted'] ?? null; + $parameters = DeletedRequestParameters::fromInput($input); $deletedEntries = []; $count = 0; - foreach ($types as $type) { - $deletedEntries[$type] = $this->dataAPIService->getDeleted($type, $deleted); + foreach ($parameters->types as $type) { + $deletedEntries[$type] = $this->dataAPIService->getDeleted($type, $parameters->deleted); $count = $count + count($deletedEntries[$type]); } return (new ResponseData( - ['types' => $types], + $parameters->toArray(), $count, $deletedEntries, ))->toArray(); @@ -71,28 +86,18 @@ private function getDeleted(array $input): array private function getResults(array $input, string $type): array { - $start = (int) ($input['start'] ?? 0); - $limit = (int) ($input['limit'] ?? 100); - $modifiedAfter = $input['modifiedAfter'] ?? null; - $ids = $input['ids'] ?? null; - $projectIds = $input['projectIds'] ?? null; + $parameters = RequestParameters::fromInput($input); $results = match ($type) { - APIData::TYPE_PROJECTS => $this->dataAPIService->getProjects($start, $limit, $modifiedAfter, $ids), - APIData::TYPE_MILESTONES => $this->dataAPIService->getMilestones($start, $limit, $modifiedAfter, $ids, $projectIds), - APIData::TYPE_TICKETS => $this->dataAPIService->getTickets($start, $limit, $modifiedAfter, $ids, $projectIds), - APIData::TYPE_TIMESHEETS => $this->dataAPIService->getTimesheets($start, $limit, $modifiedAfter, $ids, $projectIds), - APIData::TYPE_WORKERS => $this->dataAPIService->getWorkers($start, $limit, $modifiedAfter, $ids), + APIData::TYPE_PROJECTS => $this->dataAPIService->getProjects($parameters->start, $parameters->limit, $parameters->modifiedAfter, $parameters->ids), + APIData::TYPE_MILESTONES => $this->dataAPIService->getMilestones($parameters->start, $parameters->limit, $parameters->modifiedAfter, $parameters->ids, $parameters->projectIds), + APIData::TYPE_TICKETS => $this->dataAPIService->getTickets($parameters->start, $parameters->limit, $parameters->modifiedAfter, $parameters->ids, $parameters->projectIds), + APIData::TYPE_TIMESHEETS => $this->dataAPIService->getTimesheets($parameters->start, $parameters->limit, $parameters->modifiedAfter, $parameters->ids, $parameters->projectIds), + APIData::TYPE_WORKERS => $this->dataAPIService->getWorkers($parameters->start, $parameters->limit, $parameters->modifiedAfter, $parameters->ids), }; return (new ResponseData( - [ - 'start' => $start, - 'limit' => $limit, - 'modifiedAfter' => $modifiedAfter, - 'ids' => $ids, - 'projectIds' => $projectIds, - ], + $parameters->toArray(), count($results), $results, ))->toArray(); diff --git a/Model/CoercesRequestInput.php b/Model/CoercesRequestInput.php new file mode 100644 index 0000000..15f90e7 --- /dev/null +++ b/Model/CoercesRequestInput.php @@ -0,0 +1,74 @@ +|null + */ + private static function toList(mixed $value, string $name): ?array + { + if ($value === null) { + return null; + } + + if (is_array($value)) { + foreach ($value as $element) { + if (!is_scalar($element)) { + throw new InvalidRequestException(sprintf('%s must be a list of values.', $name)); + } + } + + return array_values($value); + } + + if (!is_scalar($value)) { + throw new InvalidRequestException(sprintf('%s must be a list of values.', $name)); + } + + $elements = array_filter( + array_map('trim', explode(',', (string) $value)), + fn ($element) => $element !== '', + ); + + return [] === $elements ? null : array_values($elements); + } +} diff --git a/Model/DeletedRequestParameters.php b/Model/DeletedRequestParameters.php new file mode 100644 index 0000000..55299fc --- /dev/null +++ b/Model/DeletedRequestParameters.php @@ -0,0 +1,81 @@ + $types + */ + public function __construct( + public array $types, + public ?int $deleted, + ) {} + + /** + * @param array $input + */ + public static function fromInput(array $input): self + { + return new self( + types: self::toTypes($input['types'] ?? null), + deleted: self::toTimestamp($input['deleted'] ?? null, 'deleted'), + ); + } + + /** + * @return array + */ + public function toArray(): array + { + return [ + 'types' => $this->types, + 'deleted' => $this->deleted, + ]; + } + + /** + * @return list + */ + public static function supportedTypes(): array + { + return [ + APIData::TYPE_PROJECTS, + APIData::TYPE_MILESTONES, + APIData::TYPE_TICKETS, + APIData::TYPE_TIMESHEETS, + ]; + } + + /** + * @return list + */ + private static function toTypes(mixed $value): array + { + $types = self::toList($value, 'types'); + + if ($types === null) { + return self::supportedTypes(); + } + + foreach ($types as $type) { + if (!in_array($type, self::supportedTypes(), true)) { + throw new InvalidRequestException(sprintf( + 'types must only contain: %s.', + implode(', ', self::supportedTypes()), + )); + } + } + + return array_values(array_unique($types)); + } +} diff --git a/Model/InvalidRequestException.php b/Model/InvalidRequestException.php new file mode 100644 index 0000000..f7ea2c4 --- /dev/null +++ b/Model/InvalidRequestException.php @@ -0,0 +1,10 @@ +|null $ids + * @param list|null $projectIds + */ + public function __construct( + public int $start, + public int $limit, + public ?int $modifiedAfter, + public ?array $ids, + public ?array $projectIds, + ) {} + + /** + * @param array $input + */ + public static function fromInput(array $input): self + { + return new self( + start: self::toNonNegativeInt($input['start'] ?? 0, 'start'), + limit: self::toLimit($input['limit'] ?? self::DEFAULT_LIMIT), + modifiedAfter: self::toTimestamp($input['modifiedAfter'] ?? null, 'modifiedAfter'), + ids: self::toIds($input['ids'] ?? null, 'ids'), + projectIds: self::toIds($input['projectIds'] ?? null, 'projectIds'), + ); + } + + /** + * The effective parameters, so a caller can see the limit it actually got. + * + * @return array + */ + public function toArray(): array + { + return [ + 'start' => $this->start, + 'limit' => $this->limit, + 'modifiedAfter' => $this->modifiedAfter, + 'ids' => $this->ids, + 'projectIds' => $this->projectIds, + ]; + } + + private static function toLimit(mixed $value): int + { + $limit = self::toNonNegativeInt($value, 'limit'); + + if ($limit < 1) { + throw new InvalidRequestException('limit must be at least 1.'); + } + + return min($limit, self::MAX_LIMIT); + } + + /** + * @return list|null + */ + private static function toIds(mixed $value, string $name): ?array + { + $ids = self::toList($value, $name); + + if ($ids === null) { + return null; + } + + return array_map(fn ($id) => self::toNonNegativeInt($id, $name), $ids); + } +} diff --git a/README.md b/README.md index 38f8a8e..9a22f1e 100644 --- a/README.md +++ b/README.md @@ -30,9 +30,10 @@ TYPE: projects, milestones, tickets, timesheets Attach query/body parameters to the request: * start: Starting id of the results. -* limit: Maximum number of results to get from start id in ascending order. +* limit: Maximum number of results to get from start id in ascending order. Must be at least 1, + and is capped at 1000. The limit that was actually applied is echoed in `parameters`. * modifiedAfter: Only retrieve entries that have a modified later than modifiedAfter (unix timestamp). -* ids: Array of ids to retrieve. +* ids: Array of ids to retrieve. A comma separated string is also accepted, e.g. `?ids=1,2,3`. * projectIds: Array of projectIds. Limits the entities to those attached to projects in projectIds. Only applies for types: milestone, tickets, timesheets. @@ -52,6 +53,7 @@ GET/POST: `https://{{YOUR_DOMAIN}}/apidata/api/deleted` Attach query/body parameters to the request: * types: Array of types to get deleted entities for: projects, milestones, tickets, timesheets. + Defaults to all four when left out, and a comma separated string is also accepted. * deleted: Unix timestamp. Only retrieve ids of entities deleted after this timestamp. Example request: @@ -63,6 +65,15 @@ curl https://leantime.local.itkdev.dk/apidata/api/deleted -d '{"deleted":1759906882,"types":["projects","milestones","tickets","timesheets"]}' ``` +## Errors + +A parameter that cannot be interpreted answers `400` with the reason, e.g. a non numeric +`modifiedAfter`, a `limit` below 1, an id that is not a number, or an unknown `type`: + +```json +{"error": "modifiedAfter must be a whole number."} +``` + ## API Key To use the plugin you need an API key for leantime. diff --git a/tests/Model/DeletedRequestParametersTest.php b/tests/Model/DeletedRequestParametersTest.php new file mode 100644 index 0000000..3f78976 --- /dev/null +++ b/tests/Model/DeletedRequestParametersTest.php @@ -0,0 +1,75 @@ +assertSame(DeletedRequestParameters::supportedTypes(), $parameters->types); + $this->assertNull($parameters->deleted); + } + + public function testACommaSeparatedListIsAcceptedForTypes(): void + { + $parameters = DeletedRequestParameters::fromInput(['types' => 'tickets, timesheets']); + + $this->assertSame(['tickets', 'timesheets'], $parameters->types); + } + + public function testDuplicateTypesAreCollapsed(): void + { + $parameters = DeletedRequestParameters::fromInput(['types' => ['tickets', 'tickets']]); + + $this->assertSame(['tickets'], $parameters->types); + } + + /** + * An unknown type used to reach the repository's match arm, which threw with + * the raw value in the message and rendered it on the error page. + */ + public function testAnUnknownTypeIsRejectedWithoutEchoingTheInput(): void + { + $this->expectException(InvalidRequestException::class); + $this->expectExceptionMessage('types must only contain: projects, milestones, tickets, timesheets.'); + + DeletedRequestParameters::fromInput(['types' => '