feat(reports): measured duration per client-request type #499

Merged

longjacksonle merged 3 commits from feat/PMS-732-request-type-durations into main

2026-08-06 21:05:59 +02:00

longjacksonle commented

2026-08-06 21:03:38 +02:00

Owner

No schema change

The issue was written when two things were missing: a request type to aggregate by, and a ticket-to-article association. Both arrived in the meantime (#497 gave the request type, #495 and #498 the associations), so the chain time_entries -> tickets -> form_submissions -> form_definitions -> kb_articles already exists and every join column is already indexed. This is a derived report over what the request flow records, with no new tables and nothing to keep in sync.

What is here

GET /api/v1/reports/request-types?from=&to=: measured duration per request type, with the KB article each one belongs to.
GET /api/v1/kb/articles/{id}/measured-duration?from=&to=: the same measurement for the article, summed across every request type pointing at it.

Decisions worth reviewing

Walking through form_submissions is what scopes the measurement. Only a ticket created from a client request has a submission row, so an ad-hoc ticket in the same category is excluded. That is the issue's "report only over tickets that came from a client request submission", and it is what keeps this a measurement of the request TYPE rather than of the category. Pinned by a test where an ad-hoc ticket carrying 9000 minutes against the same article fails to move the number.

No data is null, never zero. A request type nobody has tracked time against reports null for every measurement field. Zero minutes is a measurement ("we did these and they took no time"); no data is the absence of one. Collapsing them would put a confident "0 min" estimate on an article, which is worse than the hand-written guess this replaces. This is the issue's fourth acceptance criterion and it is tested on both surfaces.

ticket_count is the sample, not the demand. It counts tickets with time in the period, not requests received, so the average is interpretable. Named and documented rather than left for a reader to infer.

The two surfaces have different default periods, on purpose. The report defaults to the current calendar month (per the issue). The article defaults to a trailing 90 days. They answer different questions: the report is period accounting ("what did this month cost"), where a calendar boundary is the right unit, while the article figure is an estimate for the person about to do the work, and an estimate needs a sample. On the 2nd of the month a calendar-month window would report "no data" for almost every article, which is accurate and useless. Both responses state the period they cover, and both accept an explicit from/to, so neither number is ambiguous.

The article surface is a sub-resource, not a field. Computing the measurement for every row of an article list would join the whole time table, so a field on KbArticleResponse would be real on GET-one and always null on GET-many, which is exactly the shallow-DTO trap docs/dev-docs/codebase-state.md warns about. A sub-resource has no such ambiguity and is trivially cacheable.

Inconsistency worth a look

/reports/time defaults to a trailing 30 days while this endpoint defaults to the calendar month the issue specified. The two sit next to each other in the same router. I followed the spec rather than the neighbour, but the divergence is worth a decision rather than an accident.

Verification

Full just test-integration green, plus fmt, clippy -D warnings, and all six guard scripts. Three new tests cover aggregation across period boundaries (with entries either side of the window excluded and then picked up by a widened one, pinning the BETWEEN as inclusive), the no-data case on both surfaces, and the ad-hoc exclusion.

The tests build their tickets through the real PMS-730 magic-link flow rather than inserting submission rows, because a ticket that never went through a request form is precisely the case the report must exclude; faking the link would test the wrong thing.

Note on CI

The Playwright job fails on staging for a reason unrelated to this branch: the bunyip hub rejects the E2E account's TOTP at /login/2fa, reproducing on main and every open PR. Diagnosis is on #496.

Implements PMS-732, the last piece of the client-request loop: time tracked against request tickets aggregates per request type, and the measured duration is readable from the KB article that documents the procedure. ## No schema change The issue was written when two things were missing: a request type to aggregate by, and a ticket-to-article association. Both arrived in the meantime (#497 gave the request type, #495 and #498 the associations), so the chain time_entries -> tickets -> form_submissions -> form_definitions -> kb_articles already exists and every join column is already indexed. This is a derived report over what the request flow records, with no new tables and nothing to keep in sync. ## What is here - `GET /api/v1/reports/request-types?from=&to=`: measured duration per request type, with the KB article each one belongs to. - `GET /api/v1/kb/articles/{id}/measured-duration?from=&to=`: the same measurement for the article, summed across every request type pointing at it. ## Decisions worth reviewing **Walking through `form_submissions` is what scopes the measurement.** Only a ticket created from a client request has a submission row, so an ad-hoc ticket in the same category is excluded. That is the issue's "report only over tickets that came from a client request submission", and it is what keeps this a measurement of the request TYPE rather than of the category. Pinned by a test where an ad-hoc ticket carrying 9000 minutes against the same article fails to move the number. **No data is null, never zero.** A request type nobody has tracked time against reports `null` for every measurement field. Zero minutes is a measurement ("we did these and they took no time"); no data is the absence of one. Collapsing them would put a confident "0 min" estimate on an article, which is worse than the hand-written guess this replaces. This is the issue's fourth acceptance criterion and it is tested on both surfaces. **`ticket_count` is the sample, not the demand.** It counts tickets with time in the period, not requests received, so the average is interpretable. Named and documented rather than left for a reader to infer. **The two surfaces have different default periods, on purpose.** The report defaults to the current calendar month (per the issue). The article defaults to a trailing 90 days. They answer different questions: the report is period accounting ("what did this month cost"), where a calendar boundary is the right unit, while the article figure is an estimate for the person about to do the work, and an estimate needs a sample. On the 2nd of the month a calendar-month window would report "no data" for almost every article, which is accurate and useless. Both responses state the period they cover, and both accept an explicit from/to, so neither number is ambiguous. **The article surface is a sub-resource, not a field.** Computing the measurement for every row of an article list would join the whole time table, so a field on `KbArticleResponse` would be real on GET-one and always null on GET-many, which is exactly the shallow-DTO trap `docs/dev-docs/codebase-state.md` warns about. A sub-resource has no such ambiguity and is trivially cacheable. ## Inconsistency worth a look `/reports/time` defaults to a trailing 30 days while this endpoint defaults to the calendar month the issue specified. The two sit next to each other in the same router. I followed the spec rather than the neighbour, but the divergence is worth a decision rather than an accident. ## Verification Full `just test-integration` green, plus fmt, clippy `-D warnings`, and all six guard scripts. Three new tests cover aggregation across period boundaries (with entries either side of the window excluded and then picked up by a widened one, pinning the BETWEEN as inclusive), the no-data case on both surfaces, and the ad-hoc exclusion. The tests build their tickets through the real PMS-730 magic-link flow rather than inserting submission rows, because a ticket that never went through a request form is precisely the case the report must exclude; faking the link would test the wrong thing. ## Note on CI The Playwright job fails on staging for a reason unrelated to this branch: the bunyip hub rejects the E2E account's TOTP at `/login/2fa`, reproducing on `main` and every open PR. Diagnosis is on #496.

longjacksonle added 3 commits

2026-08-06 21:03:38 +02:00

feat(reports): aggregate tracked time by client-request type d908b79275

PMS-732: how long each class of client request actually takes, measured rather than guessed.

No schema is needed. The chain time_entries -> tickets -> form_submissions -> form_definitions already exists after PMS-730, and every join column is indexed, so this is a derived report over what the request flow already records.

Walking through `form_submissions` is what scopes the measurement. Only a ticket created from a client request has a submission row, so an ad-hoc ticket in the same category is excluded, which is PMS-732's "report only over tickets that came from a client request submission" and is what keeps this a measurement of the request TYPE rather than of the category.

Every request type is returned, including ones with no tracked time, and their measurement fields are null rather than zero. Zero minutes is a measurement ("we did these and they took no time"); no data is the absence of one. Collapsing the two would put a confident "0 min" estimate on an article nobody has ever tracked time against, which is worse than the hand-written guess this replaces.

`ticket_count` is the number of tickets with time in the period, not the number of requests received: it is the sample the average is drawn from, and saying so in the response keeps the average interpretable.

The default period is the current calendar month, per the issue. Note this differs from `/reports/time`, which defaults to a trailing 30 days; the two sit next to each other in the same router, so the inconsistency is worth a look even though this side follows the spec.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011myibMMwyb6za3GVWJGkiX

feat(kb): surface the measured duration on the article 074e5f654e

The point of PMS-732 is the estimate reaching the person about to do the work, so the measurement is readable from the article that documents the procedure.

A sub-resource (`GET /kb/articles/{id}/measured-duration`) rather than a field on the article response. Computing it for every row of a list would join the whole time table, so a field would be real on GET-one and always null on GET-many, which is exactly the shallow-DTO trap docs/dev-docs/codebase-state.md warns about.

The default window is a trailing 90 days, deliberately NOT the calendar month `/reports/request-types` defaults to, because the two answer different questions. The report is period accounting ("what did this month cost"), where a calendar boundary is the right unit. The article figure is an estimate, and an estimate needs a sample: on the 2nd of the month a calendar-month window would report "no data" for almost every article, which is accurate and useless. Both responses state the period they cover, so neither number is ambiguous, and both accept an explicit from/to.

A missing article is a 404; a real article with no request type pointing at it is data (null), not an error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011myibMMwyb6za3GVWJGkiX

test(reports): cover period boundaries and the no-data case

Create release / Gate (release-branch merges only) (pull_request) Successful in 1s

Details

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

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 4m15s

Details

E2E / Playwright against staging (pull_request) Failing after 6m20s

Details

Integration / integration tests (pull_request) Successful in 12m49s

Details

4bdd790c04

Builds its tickets through the real PMS-730 magic-link flow rather than inserting submission rows, because a ticket that never went through a request form is precisely the case the report must exclude; faking the link would test the wrong thing.

Covered: aggregation across period boundaries, with entries the day before the window opens and the day after it closes excluded and then picked up by a widened window, which pins the BETWEEN as inclusive on both ends; a request type that has been requested but never tracked reporting null rather than zero, on both the report and the article; and an ad-hoc ticket carrying 9000 minutes against the same article failing to skew the measurement, which is the join through form_submissions doing its job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011myibMMwyb6za3GVWJGkiX