fix(time): PUT /time-entries preserves task_id (and expose it in the response) #248

Merged

nrupard merged 2 commits from fix/PMS-328-time-entry-preserve-task-id into main

2026-06-15 20:46:03 +02:00

nrupard commented

2026-06-15 20:35:44 +02:00

Owner

Summary

Editing a time entry over PUT /time-entries/{id} silently wiped its task_id, dropping the entry's task link so its approved hours stopped rolling up into task/project actuals (PMS-51). The update direct-set task_id = $14 from UpdateTimeEntryRequest.task_id, which defaults to None when the key is absent, and TimeEntryResponse never carried task_id, so a client could neither read the current value nor re-send it: any partial edit cleared the link. Latent until the client gained an edit UI (MAPPS-166).

Changes

Preserve on omit: task_id = COALESCE($14, task_id) in update_time_entry, matching the existing treatment of date/work_type_id/notes/is_billable. A partial PUT that omits task_id keeps the existing link; sending an explicit task_id reassigns it.
Expose the field: add task_id: Option<Uuid> to TimeEntryResponse, the TimeEntryRow, the From impl, and the list + get SELECTs, so clients can display and intentionally change the task.
Create behavior unchanged.

Decision note

The issue's AC3 asked that an explicit null clear the link. With plain Option<Uuid>, serde cannot distinguish an absent key from an explicit null (both deserialize to None), so a simple COALESCE preserves in both cases. Per the PMS-328 decision we kept the minimal COALESCE fix (consistent with how notes/is_billable already behave) rather than introducing an Option<Option<Uuid>> double-option pattern used nowhere else in the codebase. Net: omit preserves, explicit uuid reassigns, explicit-null does not clear. The primary data-loss bug (AC1) is fixed; AC2 (expose) is met; AC3 is met for change but not clear.

Tests

Service-level regression tests in tests/time_tracking.rs, all green against a throwaway Postgres:

update_preserves_task_id_when_omitted (AC1): create with a task, update only notes, assert the link survives.
update_with_explicit_task_id_changes_link (AC3 change): explicit task_id replaces the prior link.
read_paths_expose_task_id (AC2): get and list both surface task_id.

The existing service_desk_time_slice_happy_path still passes (no create regression).

Out of scope but still fixed in this PR

update_time_entry/delete_time_entry gate only on RequireTimeTracking with no owner/approval check, so any tenant time-tracking user can edit/delete/re-price another user's approved entry. Noted in the issue; not addressed here.

#PMS-328

## Summary Editing a time entry over `PUT /time-entries/{id}` silently wiped its `task_id`, dropping the entry's task link so its approved hours stopped rolling up into task/project actuals (PMS-51). The update direct-set `task_id = $14` from `UpdateTimeEntryRequest.task_id`, which defaults to `None` when the key is absent, and `TimeEntryResponse` never carried `task_id`, so a client could neither read the current value nor re-send it: any partial edit cleared the link. Latent until the client gained an edit UI (MAPPS-166). ## Changes - Preserve on omit: `task_id = COALESCE($14, task_id)` in `update_time_entry`, matching the existing treatment of `date`/`work_type_id`/`notes`/`is_billable`. A partial PUT that omits `task_id` keeps the existing link; sending an explicit `task_id` reassigns it. - Expose the field: add `task_id: Option<Uuid>` to `TimeEntryResponse`, the `TimeEntryRow`, the `From` impl, and the list + get SELECTs, so clients can display and intentionally change the task. - Create behavior unchanged. ## Decision note The issue's AC3 asked that an explicit `null` clear the link. With plain `Option<Uuid>`, serde cannot distinguish an absent key from an explicit `null` (both deserialize to `None`), so a simple COALESCE preserves in both cases. Per the PMS-328 decision we kept the minimal COALESCE fix (consistent with how `notes`/`is_billable` already behave) rather than introducing an `Option<Option<Uuid>>` double-option pattern used nowhere else in the codebase. Net: omit preserves, explicit uuid reassigns, explicit-null does not clear. The primary data-loss bug (AC1) is fixed; AC2 (expose) is met; AC3 is met for change but not clear. ## Tests Service-level regression tests in `tests/time_tracking.rs`, all green against a throwaway Postgres: - `update_preserves_task_id_when_omitted` (AC1): create with a task, update only `notes`, assert the link survives. - `update_with_explicit_task_id_changes_link` (AC3 change): explicit `task_id` replaces the prior link. - `read_paths_expose_task_id` (AC2): get and list both surface `task_id`. The existing `service_desk_time_slice_happy_path` still passes (no create regression). ## Out of scope but still fixed in this PR `update_time_entry`/`delete_time_entry` gate only on `RequireTimeTracking` with no owner/approval check, so any tenant time-tracking user can edit/delete/re-price another user's approved entry. Noted in the issue; not addressed here. #PMS-328

nrupard added 1 commit

2026-06-15 20:35:44 +02:00

fix(time): preserve task_id on time-entry update and expose it

E2E / Playwright against staging (pull_request) Successful in 47s

Details

Check / fmt + clippy + compile + unit/doc tests (pull_request) Successful in 1m13s

Details

Integration / integration tests (pull_request) Has been cancelled

Details

106c298bc9

Editing a time entry over PUT /time-entries/{id} silently wiped its task_id. The update direct-set `task_id = $14` from a request that defaults to None whenever the key is absent, and TimeEntryResponse never carried task_id, so a client could neither read nor re-send the current value: any edit dropped the task link, and the entry's approved hours stopped rolling up into task/project actuals.

Preserve on omit: `task_id = COALESCE($14, task_id)`, matching how date/work_type_id/notes/is_billable are already preserved. A partial PUT that omits task_id now keeps the existing link; sending an explicit task_id reassigns it. (Per PMS-328 decision, explicit-null does not clear, consistent with the other COALESCE columns; clearing a link is out of scope for this minimal fix.)

Make it observable: add `task_id: Option<Uuid>` to TimeEntryResponse and include it in the list and get SELECTs so clients can display and intentionally change the task.

Create behavior is unchanged. Service-level regression tests cover preserve-on-omit, explicit reassignment, and the get/list exposure.

#PMS-328

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

nrupard added 1 commit

2026-06-15 20:40:49 +02:00

fix(time): restrict time-entry edit/delete to owner or admin

E2E / Playwright against staging (pull_request) Successful in 47s

Details

Check / fmt + clippy + compile + unit/doc tests (pull_request) Successful in 1m11s

Details

Integration / integration tests (pull_request) Successful in 4m17s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

319b96e6a9

PUT/DELETE /time-entries/{id} gated only on RequireTimeTracking, with no owner check, so any tenant time-tracking user could edit, delete, or re-price another user's entry (including approved ones). create_time_entry already forces a non-admin to their own user_id; this brings update/delete in line.

Both handlers now load the entry first (an unknown id still 404s), then a non-admin acting on an entry they do not own gets 403 Forbidden. Admins retain full access, matching how create lets an admin attribute time to other users. An HTTP-level test covers: a second technician is refused (403) on both edit and delete, while the owner can edit and an admin can delete.

#PMS-328

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>