Keep timesheets modified current via insert/update triggers

[turegjorup]

Link to ticket

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

Description

Timesheet changes were not all picked up by modifiedAfter sync because some core write paths leave zp_timesheets.modified stale — specifically the ON DUPLICATE KEY UPDATE branches of addTime, upsertTimesheetEntry and punchOut, which accumulate hours without updating modified.

This adds two triggers on zp_timesheets, installed alongside the existing deleted-tracking triggers:

• itk_timesheets_modified_insert_trigger (BEFORE INSERT) sets modified = NOW().
• itk_timesheets_modified_update_trigger (BEFORE UPDATE) sets modified = NOW().

Both are dropped on uninstall. This guarantees modified reflects the last change regardless of which write path made it.

All triggers are now installed with CREATE OR REPLACE TRIGGER (MariaDB) so install() is idempotent and always (re)installs the current definition — the previous plain CREATE TRIGGER would error on a re-run even though the tables use CREATE TABLE IF NOT EXISTS. The uninstall drops use IF EXISTS. This applies to the existing deleted-tracking triggers too.

Note: existing rows with stale/zero modified values are not backfilled; they update on their next write (or via a one-time UPDATE).

Steps to reproduce

1. Log time on a ticket for a given day and kind, then sync — the entry is picked up.
2. Log time again on the same ticket, day and kind (matching the (userId, ticketId, workDate, kind) unique key). Hours accumulate into the existing row via ON DUPLICATE KEY UPDATE.
3. Inspect the row: hours has increased but modified is unchanged (still the original value).
4. Run an incremental sync with modifiedAfter set after the original modified — the updated entry is not returned, so the hours change is missed.

With the triggers installed, step 3 shows modified advancing to NOW() and step 4 returns the entry.

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.

[tuj]

We may need the same modified triggers for projects and tickets.

---

AI Review

---

Review — PR #14: Keep timesheets modified current via insert/update triggers

Branch: feature/timesheets-modified-trigger → develop
Files changed: CHANGELOG.md, README.md, Services/APIData.php (+29/−6)
Verdict: Correct fix for a real bug. Two changes requested, one item to verify on production.

Summary

The diagnosis is right. upsertTimesheetEntry()
(~/qork/leantime/app/Domain/Timesheets/Repositories/Timesheets.php:736-754) calls Laravel
upsert() with update columns ['hours'], so on duplicate key modified is never written.
Those rows go invisible to the modifiedAfter filter in
Repositories/ApiDataRepository.php:73 and the sync silently misses them. Triggers on
zp_timesheets are the right way to close it.

---

1. CREATE OR REPLACE TRIGGER locks us to MariaDB — request change

Services/APIData.php:55, 63, 71, 79, 86

This works in our environment (MariaDB has supported it since 10.1.4), so it isn't broken
today. But MySQL has no OR REPLACE for triggers at all — the 8.4 grammar is
CREATE [DEFINER = user] TRIGGER [IF NOT EXISTS] trigger_name …. On MySQL, install() aborts
at the first of these statements, which means not only do the two new triggers not get
created, the three pre-existing itk_*_deleted_trigger definitions stop being created too.
Leantime itself ships mysql:8.4 in all three of its compose files, so we shouldn't take the
dependency.

Use DROP TRIGGER IF EXISTS + plain CREATE TRIGGER. Valid on MySQL 5.0→8.4 and every
MariaDB, and more idempotent than CREATE TRIGGER IF NOT EXISTS — that variant silently
keeps an outdated trigger body rather than redefining it, which defeats the purpose of making
install() re-runnable.

DROP TRIGGER IF EXISTS itk_projects_deleted_trigger;
CREATE TRIGGER itk_projects_deleted_trigger
AFTER DELETE ON zp_projects
FOR EACH ROW
BEGIN
   INSERT INTO itk_projects_deleted(entryId)
   VALUES (OLD.id);
END;

…for all five triggers. The two new ones keep the bodies already in the PR:

  DROP TRIGGER IF EXISTS itk_timesheets_modified_update_trigger;
  CREATE TRIGGER itk_timesheets_modified_update_trigger
  BEFORE UPDATE ON zp_timesheets
  FOR EACH ROW
  BEGIN
     SET NEW.modified = NOW();
  END;

No DELIMITER needed — that's a mysql CLI feature; both servers parse BEGIN … END;
correctly over the wire, as the existing code already relies on.

2. Errors after the first statement are silently swallowed — request change

Services/APIData.php:97-102 and 123-128

install() sends one multi-statement batch through PDO and calls execute() once. PDO
reports only the first statement's outcome; failures in later statements surface on
nextRowset(), which is never called. So a fresh install can create the three tables, fail
on trigger #1, and report success.

This is pre-existing, but it's exactly how issue #1 would have reached production looking
like a clean install — and it will swallow the next problem too (a missing TRIGGER grant,
say). Both methods duplicate the same PDO block, so extract one helper:

  /**
   * Runs a multi-statement SQL batch.
   *
   * PDO surfaces only the first statement's outcome, so the remaining result sets have to
   * be drained for a failure in any later statement to be reported at all.
   */
  private function executeMultiStatement(string $sql): void
  {
      // Laravel's statement() does not handle multi-statement SQL, so go through PDO.
      $pdo = app('db')->connection()->getPdo();
      $stmn = $pdo->prepare($sql);

      $stmn->execute();

      do {
          // Errors in statements after the first are raised here, not by execute().
      } while ($stmn->nextRowset());

      $stmn->closeCursor();
  }

3. Verify NOW() matches Leantime's clock on production — review item

The trigger's NOW() uses the database session time_zone. Leantime writes the same
column with PHP date('Y-m-d H:i:s') under app.timezone
(~/qork/leantime/app/Core/Bootstrap/LoadConfig.php:106; note configuration.sample.php:36
ships America/Los_Angeles). The API then reads every value back as UTC regardless
(Services/APIData.php:228).

If those two clocks differ, modified ends up holding mixed-timezone values, and a
trigger-written row can land behind a PHP-written one — which reopens the exact miss this PR
is closing, just with a smaller window. Same on MariaDB and MySQL.

Before merge, confirm on production:

SELECT @@session.time_zone, @@global.time_zone, NOW(), UTC_TIMESTAMP();

Compare against the configured Leantime timezone. They're aligned in the default Docker setup,
so this is likely fine — but it's cheap to check and expensive to debug later. If they differ,
align the DB session timezone rather than papering over it in the trigger body. Worth noting
the pre-existing dateDeleted datetime DEFAULT NOW() on the itk_*_deleted tables
(Services/APIData.php:36, 44, 51) has the same exposure, so a mismatch is worth fixing
centrally.

4. Existing rows are not backfilled — handled operationally

zp_timesheets.modified is datetime DEFAULT NULL
(~/qork/leantime/app/Domain/Install/Repositories/Install.php:674), and NULL never satisfies
the modifiedAfter comparison — so those rows stay invisible to incremental sync
indefinitely, not just until the next write. Agreed to handle this with one full resync
(no modifiedAfter) after deploy rather than backfill SQL. Worth a line in the README so the
next person deploying this knows.

---

Accepted as-is

• Unconditional SET NEW.modified = NOW() on UPDATE. saveWeeklyTimesheetEntries()
  (~/qork/leantime/app/Domain/Timesheets/Services/Timesheets.php:400-463) re-upserts every
  cell of the weekly grid whether it changed or not, and both engines fire BEFORE UPDATE on the
  ON DUPLICATE KEY UPDATE branch — so one grid save bumps modified across that week and the
  next sync re-pulls it. Churn, not corruption, and over-syncing beats the under-syncing we
  have now. Deliberate tradeoff, noted here so it isn't rediscovered as a bug later.
• DROP TRIGGER IF EXISTS in uninstall() (lines 108-112) is a real fix — the old bare
  DROP TRIGGER aborted on the first already-missing trigger.
• The BEFORE INSERT trigger is worth keeping even though the current insert path already
  sets modified: it guarantees no other writer can leave the column NULL, and NULL is
  permanently invisible to incremental sync.
• Changelog matches house format and satisfies the CI check (.github/workflows/pr.yml).

Minor

The PR checklist ticks "covered by test cases" / "passes our tests", but the repo has no test
suite and CI only checks the changelog. Not blocking — the template asks for a comment
explaining the exemption instead of ticked boxes.

---

Verification before merge

Run against both engines: MariaDB (production) and MySQL 8.4 (what Leantime ships, in
~/qork/leantime/.docker/docker-compose.yml). The MySQL run is the whole point of change #1
and is what the PR currently fails.

1. Fresh install() → SHOW TRIGGERS FROM ; lists all five triggers, on both engines.
2. Run install() again → succeeds, still exactly five triggers.
3. Insert a timesheet row → modified is set, not NULL.
4. UPDATE zp_timesheets SET hours = hours + 1 WHERE id = ?; → modified advances.
5. The actual bug path: save the weekly timesheet grid so upsertTimesheetEntry() takes
   its duplicate-key branch, and confirm modified advances. Before this PR it does not.
6. POST /apidata/api/timesheets with modifiedAfter set just before step 5 → row is returned.
7. uninstall() twice → no error on the second run; the three itk_*_deleted tables and their
   data survive.
8. Error-drain check: temporarily append an invalid statement to the install() batch and
   confirm it now throws instead of passing silently.
9. Timezone check from item 3 on production.

[tuj]

Superseded by #20