feat: allowed null values in API models by tuj · Pull Request #18 · ITK-Leantime/data-api

tuj · 2026-07-30T05:42:58Z

Link to ticket

https://leantime.itkdev.dk/#/tickets/showTicket/8000

Description

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.

Relates to itk-dev/economics#325 that handles the null values.

Checklist

My code is covered by test cases.
My code passes our test (all our tests).
My code passes our static analysis suite.
My code passes our continuous integration process.

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.

turegjorup

Approved with comments. Highly recommend fixing.

turegjorup · 2026-08-10T09:02:56Z

+      fail-fast: false
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4


actions/checkout@v7 is the latest

turegjorup · 2026-08-10T09:03:54Z

+
+      # Matches the PHP version in Dockerfile (itkdev/php8.3-fpm).
+      - name: Setup PHP
+        uses: shivammathur/setup-php@v2


This should run in the projects docker compose stack, not using shivammathur/setup-php

turegjorup · 2026-08-10T09:17:51Z

    "require-dev": {
+        "illuminate/database": "^11.0",
+        "nesbot/carbon": "^2.72.2 || ^3.0",
+        "phpunit/phpunit": "^11.0"


"PHPUnit 11 and older versions no longer receive bug fixes", Latest is 13, no reason to start on an older version

changed to phpunit 12, that supports php 8.3 which the current version of leantime runs.

turegjorup · 2026-08-10T09:35:03Z

Follow-up to my review above. Ran the suite in the compose container first: 14 tests, 45 assertions, OK on PHP 8.3.31.

A. `WorkerData` is the one model left non-nullable, and it has the same failure mode

Model/WorkerData.php:11-12 still declares public string $email and public string $name. Repositories/ApiDataRepository.php:86 builds that name as CONCAT(worker.firstname, ' ', worker.lastname), and MySQL CONCAT() returns NULL if any argument is NULL. One user with a missing firstname or lastname gives name = null, which TypeErrors at Services/APIData.php:199 and fails the whole /workers response — the same incident shape as the ticket, on the endpoint this PR doesn't touch.

I have not confirmed zp_user.firstname/lastname nullability against the schema, so that's worth checking, but the CONCAT() behaviour makes it an unguarded path regardless. CONCAT_WS(' ', firstname, lastname) may beat a nullable type here, since a partial name is more useful than a null one. Note also that $email is fed from worker.username, which the timesheet change just conceded can be null.

B. N+1 on the line this PR is already rewriting

Services/APIData.php:151 calls getStateLabels($value->projectId) inside array_map, once per ticket — 100 repository round-trips at the default limit, for what is usually one or two distinct projects. The expression is being restructured anyway, so memoising into a local [$projectId => $labels] array is close to free.

C. The new `.claude/settings.json` will ship inside the release tarball

Verified by running rsync against bin/release-exclude.txt: .git* catches .git, .github and .gitignore, but not .claude. The same run showed .idea/ and compose.yml surviving too — the exclude file lists compose.yaml, but the file is named compose.yml (and this PR edits it). The compose and .idea leaks are pre-existing, but since release-exclude.txt is already being edited here, it's the natural place to add .claude, .idea, and fix the compose filename.

D. Six implicit-nullable deprecations that PHP 8.4 will flag

Confirmed by loading the classes under PHP 8.5:

APIData::getMilestones() — $modifiedAfter
APIData::getTickets() — $modifiedAfter, $ids
ApiDataRepository::getMilestones() — $modifiedAfter
ApiDataRepository::getTickets() — $modifiedAfter, $ids

This PR already fixes a sibling deprecation, the required-after-optional $kind in TimesheetData, so it's the same cleanup. This ties into my CI comment: pinning to 8.3 is what hides them, whether via setup-php or the compose image.

E. `failOnDeprecation` is missing

phpunit.xml.dist sets failOnWarning and failOnNotice but not failOnDeprecation. For a PR whose central fix is a deprecation, that's the one flag worth having on.

F. CI installs with no lock file

composer.lock is gitignored, so composer install quietly degrades to resolving from scratch and floats. Related: illuminate/database ^11.0 and nesbot/carbon ^2.72.2 || ^3.0 in require-dev are effectively a guess at what Leantime v3.9.7 ships — if they drift, the suite validates against different library versions than production runs. Moving to the compose stack fixes the PHP version but not this; the constraints should be pinned to Leantime's actual ones.

G. Dead `strategy` block

strategy: fail-fast: false in the new test job has no matrix under it, so it does nothing.

H. Coverage gaps

Service-level mapping tests cover only getTimesheets and getTickets. getWorkers — the unfixed path in A — has none, and neither does getDeleted, which does an unguarded entryId to id mapping.

Worth keeping as-is: the switch to named arguments at Services/APIData.php:176-187 plus the test pinning field-to-property mapping is the right guard for that class of bug, and the stub's docblock is honest about drift rather than pretending the signature is verified.

Minor: DeletedData is the only model not declared readonly.

Allowed a missing worker name, and stopped returning a whitespace-only name for a worker whose firstname and lastname are both blank. Ticket status labels are looked up once per project instead of once per ticket, and the remaining implicit-nullable parameters are now explicit. Committed composer.lock and pinned illuminate/database and nesbot/carbon to the versions Leantime v3.9.7 locks, so the tests run against the code Leantime itself runs. PHPUnit moved to 12, the newest release that still runs on PHP 8.3. The pull request test job runs in the project's Docker Compose stack, so the PHP version comes from the Dockerfile rather than being repeated in the workflow. Releases no longer ship .claude, .idea or compose.yml.

The container ran as `deploy` (uid 1000) while the CI checkout belongs to the runner's uid, so composer could not create `vendor`. The compose user now comes from COMPOSE_USER, which the workflow sets to the checkout owner; the image carries a `runner` user at that uid, so it still has a writable home. Only `bin/create-release` needs the rsync the Dockerfile adds, so tests and linting run on itkdev/php8.3-fpm directly and no longer build an image. The built image moved to a `php-release` service, used by the release workflow alone.

tuj · 2026-08-11T09:23:46Z

The CONCAT NULL path isn't reachable on a conformant schema — zp_user.username/.firstname/.lastname are all NOT NULL in 3.9.7 (Install.php:684-687), and no migration relaxes them. The reachable bug was blank name parts yielding " ". The nullable model is deliberate defence-in-depth; NULLIF is what actually fixes the observable output.

Matches how itk-dev/economics does it, and drops a shell step.

tuj added 2 commits July 29, 2026 15:43

Entries referencing deleted users or tickets no longer fail the whole

2998f43

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.

docs: updated changelog

cb0fdb4

tuj self-assigned this Jul 30, 2026

tuj added bug Something isn't working enhancement New feature or request labels Jul 30, 2026

tuj changed the title ~~Allowed null values in API models~~ feat: allowed null values in API models Jul 30, 2026

tuj mentioned this pull request Jul 30, 2026

fix: stop the Leantime sync halting silently on a bad row itk-dev/economics#325

Open

4 tasks

chore: removed economics fro gitignore

54c9ddf

tuj requested a review from turegjorup August 4, 2026 06:37

tuj mentioned this pull request Aug 5, 2026

feat: own the sync watermark in an itk_data_api_modified column #20

Open

turegjorup approved these changes Aug 10, 2026

View reviewed changes

tuj added 2 commits August 11, 2026 08:45

ci: set the compose user via a workflow env variable

db81c43

Matches how itk-dev/economics does it, and drops a shell step.

tuj requested a review from turegjorup August 14, 2026 09:11

turegjorup approved these changes Aug 14, 2026

View reviewed changes

tuj merged commit 8914a03 into develop Aug 15, 2026
3 checks passed

tuj deleted the feature/8000-sync-errors branch August 15, 2026 13:56

Navigation Menu

Uh oh!

feat: allowed null values in API models - #18

feat: allowed null values in API models#18
tuj merged 6 commits into
developfrom
feature/8000-sync-errors

tuj commented Jul 30, 2026 •

edited

Loading

Uh oh!

turegjorup left a comment

Uh oh!

turegjorup Aug 10, 2026

Uh oh!

tuj Aug 11, 2026

Uh oh!

turegjorup Aug 10, 2026

Uh oh!

tuj Aug 11, 2026

Uh oh!

turegjorup Aug 10, 2026

Uh oh!

tuj Aug 11, 2026 •

edited

Loading

Uh oh!

turegjorup commented Aug 10, 2026

Uh oh!

tuj commented Aug 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

tuj commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to ticket

Description

Checklist

Uh oh!

turegjorup left a comment

Choose a reason for hiding this comment

Uh oh!

turegjorup Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

tuj Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

turegjorup Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

tuj Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

turegjorup Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

tuj Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turegjorup commented Aug 10, 2026

A. WorkerData is the one model left non-nullable, and it has the same failure mode

B. N+1 on the line this PR is already rewriting

C. The new .claude/settings.json will ship inside the release tarball

D. Six implicit-nullable deprecations that PHP 8.4 will flag

E. failOnDeprecation is missing

F. CI installs with no lock file

G. Dead strategy block

H. Coverage gaps

Uh oh!

tuj commented Aug 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tuj commented Jul 30, 2026 •

edited

Loading

tuj Aug 11, 2026 •

edited

Loading

A. `WorkerData` is the one model left non-nullable, and it has the same failure mode

C. The new `.claude/settings.json` will ship inside the release tarball

E. `failOnDeprecation` is missing

G. Dead `strategy` block