feat: own the sync watermark in an itk_data_api_modified column

[tuj]

Ticket

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

Description

Base branch is feature/8000-sync-errors (#18), not develop.

This supersedes #17 — close it unmerged. Its ticket.date → ticket.modified
fix and its implicit-nullable parameter fixes are both included here.

Why

The modifiedAfter contract says: give me everything that changed since this
watermark. That rested on Leantime core's modified column, which core does not
maintain on every write path.

• Timesheets written through ON DUPLICATE KEY UPDATE (addTime,
  upsertTimesheetEntry, punchOut) leave modified untouched, so time edited
  from the weekly grid never reached consumers.
• Tickets and milestones were filtered on ticket.date — the creation date —
  so editing an existing ticket never showed up in a sync at all.
• Legacy rows carry NULL or "0000-00-00 00:00:00", which a modified >= ?
  predicate silently drops.
• Nothing stops the next core write path from having the same omission.

Economics invoices off this data, so a missed edit is a wrong invoice.

What changed

The plugin now owns itk_data_api_modified on zp_projects, zp_tickets,
zp_timesheets and zp_user, maintained by database triggers. A trigger cannot
be bypassed — not by core, not by this plugin, not by manual SQL. Core's own
modified column is left completely alone.

The JSON field is still called modified, sourced from the new column, so
consumers need no change. users gains a modified field, so all four
entity types now expose a watermark.

Alongside that, the DDL moved out of Services/APIData.php into a new
Repositories/SchemaRepository.php:

• Statements execute one at a time. The old code prepared one
  multi-statement string and executed it once, so PDO reported only the first
  statement's error and every later failure was swallowed.
• install() is idempotent. Leantime calls install() on every install and
  offers no separate upgrade hook, so it is both installer and migrator. Column
  and index existence are checked through information_schema; every trigger is
  dropped with IF EXISTS before being created. This also fixes reinstalling,
  which used to throw on the delete triggers the previous install left behind
  (swallowed by installPlugin into report($e) + return false).
• The SQL is unit tested without a database — the builders are pure static
  methods, and the tests pin the properties that would otherwise only fail in
  production.

Two smaller fixes carried along: WorkerData was missed by #18's nullability
sweep — CONCAT(firstname, ' ', lastname) returns NULL for a user with no
surname, which failed the whole /users response — and APIData::getWorkers()
was passing a fifth argument to a four-parameter repository method.

Why triggers rather than DATETIME ... ON UPDATE CURRENT_TIMESTAMP

An auto-updating column would need no triggers at all, but ON UPDATE CURRENT_TIMESTAMP writes the session timezone's clock — the exact ambiguity
that makes the current handling unreliable. A trigger writes UTC_TIMESTAMP()
explicitly, so every value means one thing regardless of DB, PHP or Leantime
timezone config. The read side already parses as UTC. For the same reason, the
delete triggers now stamp dateDeleted explicitly instead of relying on the
column's DEFAULT NOW().

MySQL 8.4

Production is MySQL 8.4, so no CREATE OR REPLACE TRIGGER and no
ADD COLUMN IF NOT EXISTS — both MariaDB-only, and what #14 was rejected for.
There is a test asserting no statement uses either.

Risks

• One full resync after this deploys, for all four entity types. Installing
  stamps every existing row with the install time, by design: the alternative is
  guessing at a watermark for rows that never had a reliable one.
• The plugin adds columns to core Leantime tables. Safe for normal
  operation — core writes use explicit column lists, so a nullable extra column
  is ignored — but a future core migration that rebuilds a zp_* table could
  drop it. Recovery is a plugin reinstall, which re-adds and re-backfills.
• Deliberate over-syncing. A BEFORE UPDATE trigger fires on any row write,
  including ones that change nothing the API exposes. zp_user is the notable
  case: Auth::updateUserSession() writes lastlogin, session and
  sessiontime on login, and invalidateSession() writes on logout, so a user
  re-syncs on every login. There is also a KeepAlive endpoint calling the same
  update — worth checking on production whether the frontend polls it, since
  that would make active users re-sync continuously. Over-syncing beats missing
  updates, which is the trade-off already accepted in Keep timesheets modified current via insert/update triggers #14, but it is worth
  knowing the volume.
• Existing dateDeleted values keep their local-time stamps. Only new
  deletions are UTC. On a Europe/Copenhagen server the historical rows are
  1–2 hours ahead, so a consumer polling /deleted may re-see one historical
  deletion once around the switchover. Deletions are idempotent, so this is
  cosmetic.
• First backfill on a large instance is a long UPDATE inside the
  plugin-install request. Worth timing against a production-sized dump before
  installing; batch by id range if it is slow.

Verification

task lint and task test pass — 30 tests, 259 assertions.

Not yet done, and needing a real Leantime instance:

• Install, then SHOW TRIGGERS (expect 11) and
  SELECT COUNT(*) FROM zp_timesheets WHERE itk_data_api_modified IS NULL
  (expect 0).
• Re-run install() on an already-installed database: must not throw, must not
  duplicate the index, must not re-stamp rows that already have a value.
  Uninstall and reinstall: triggers return, column and data survive.
• Save time from the weekly grid and confirm itk_data_api_modified advances
  while core's modified stays put, and that the entry comes back from
  /apidata/api/timesheets.
• Edit an old ticket's headline and confirm it now comes back from
  /apidata/api/tickets — the fix: fixed modifiedAfter for tickets and milestones #17 case.
• Set the DB session timezone to something other than UTC, edit an entity, and
  confirm the returned modified matches wall-clock UTC.
• Run the install against MariaDB as well as MySQL 8.4.

Two placeholders to settle before opening it: the CHANGELOG links to PR-20 (correct it if GitHub assigns another number), and the KeepAlive polling question is stated as something to check
rather than a claim — I found the endpoint at app/Domain/Auth/Controllers/KeepAlive.php:38 but no frontend caller in the checkout.

[turegjorup]

Reviewed locally. Reproduced the stated verification in the compose container: 30 tests, 259 assertions, and task lint clean.

The design holds up and the PR body is unusually candid about its own risks — most of what I would normally raise on a schema change is already in the Risks section. The items below are what remains after discussing the alternatives (see the note at the bottom for what those discussions closed).

1. install()'s orchestration has no test

The 30 tests all exercise the pure static builders. The sequence in SchemaRepository.php:43-62 has none — including the "triggers before backfill" argument, which lives only in the comment at lines 57-58, and the hasColumn/hasIndex guards that make reinstalling idempotent. Extracting the per-table sequence into a pure method, something like installStatements(string $table, bool $hasColumn, bool $hasIndex): array, would pin the order and matches the PR's own goal of having the SQL unit tested.

2. The README does not state the new privilege requirement

Install now needs ALTER on four core zp_* tables on top of the existing CREATE and TRIGGER. If the Leantime database user lacks it, install fails — loudly now, which is the improvement, but the new "What installation changes in the database" section is the place to say what grants are needed.

3. Write down the precondition, and correct the backfill docblock

The design is safe because plugin updates happen with the site down, so no writes land while the triggers are absent. That assumption is load-bearing and currently written down nowhere. Worth stating in the SchemaRepository class docblock and the README, since hot-swapping the plugin on a live site would silently lose every edit made in that window — the backfill's WHERE ... IS NULL only recovers inserts.

Related: the docblock at SchemaRepository.php:92-96 says the backfill heals "rows written while the plugin was uninstalled and the triggers were gone". That is true of inserts only, and under the operating assumption above the set is empty anyway. Reword or drop it rather than leave a claim that does not hold.

4. The index is built before the backfill

install() adds the index (lines 53-55) and then runs a full-table UPDATE (line 60), so the index is built over an all-NULL column and every entry is immediately rewritten. Backfilling first and indexing after roughly halves the work. The triggers must still precede the backfill; the index carries no such constraint. Free reorder, and it goes directly to the "first backfill on a large instance" risk already listed.

5. CONCAT versus CONCAT_WS is a response-contract question

WorkerData takes the nullable route, so a user with no surname returns name: null rather than "Anne". CONCAT_WS(' ', firstname, lastname) would give the partial name. For a consumer that invoices, a partial name may well beat a null one — worth confirming with economics before this lands, since it is a change to what the endpoint returns.

6. The zp_user re-sync question is still open

Distinct from the write-cost discussion: login writes change lastlogin, session and sessiontime, so they are genuine updates and will bump the watermark and re-sync the user regardless. The PR body already flags checking whether the frontend polls KeepAlive. If the volume turns out to matter, a NULL-safe comparison in the update trigger — stamping only when a column the API actually exposes changed — is the lever, purely for sync volume.

7. The deleted-tables DDL has no migration path

CREATE TABLE IF NOT EXISTS is a bootstrap, not a declaration: every database that already has the table ignores it forever, so editing those statements only changes what fresh installs get and the two populations drift apart silently. The docblock at SchemaRepository.php:160-164 identifies this but stops at "do not change them", which leaves no path at all.

The hasColumn/hasIndex helpers are already the primitive needed. Freezing the CREATEs as the historical baseline and expressing every later change as a conditional ALTER after deletedTableStatements() converges both populations through one code path.

The concrete case: dateDeleted DEFAULT NOW() is noted as unreachable now that the triggers set the column, but it is still in the DDL on every install. If a trigger ever goes missing, the default silently resumes writing local time into a column the read side parses as UTC. ALTER COLUMN dateDeleted DROP DEFAULT turns that into a visible NULL instead — a change the current design cannot ship.

Separately, int(11) needs none of this. Display width is cosmetic and produces an identical column, so those statements can be edited in place: fresh installs stop emitting the 8.0.17 deprecation, existing installs keep int(11), and the tables are the same.

8. One small thing, easily dismissed

ON DUPLICATE KEY UPDATE reports affected-rows 0 when values are unchanged and 2 when it genuinely updates; the trigger makes it always 2. That is a behaviour change in core rather than a performance one, so a single grep through the timesheet write paths for a rowCount() or affected-rows branch would close it.

---

Closed by discussion, recorded so they are not re-raised: maintaining the watermark in a dedicated delta table instead of altering core tables; DEFAULT (UTC_TIMESTAMP()) on the column to drop the four insert triggers; and the write amplification from the update trigger defeating InnoDB's no-op-update skip. The WorkerData nullability gap I raised on #18 is fixed here, and all six implicit-nullable parameter deprecations are now gone — I verified none remain.

Items that apply to the whole stack rather than this PR are in my comment on #18 and are unchanged on this branch.

[turegjorup]

Approved. See comment for minor issues flagged in review