feat(rm-handlers): W3 — TimeEntry list + detail (+ factor HandlerError to common)#12
Merged
Merged
Conversation
… to common
W3 from the Redmine Integration Plan, third Phase-1 width track.
TimeEntry (canonical `billable_work_entry`, codebook 0x0103) — hours
booked against a project / work item. Redmine + OpenProject both
call this `TimeEntry`; both ports converge here per
`ogar_vocab::ports::*Port::class_id`.
# Routes
- GET /time_entries — list page (Date / Hours / Comments)
- GET /time_entries/:id — detail page (URL `:id` = SurrealDB
record key, same shape as W1)
# Plan §1.6 factoring ("three points form a line")
W3 lands as the third caller of the per-resource scaffolding —
`HandlerError`, `record_id_to_u64`, `identifier_to_u64`,
`wrap_in_doc`. With three identical `HandlerError` enums shipped
across `issues.rs`, `projects.rs`, `time_entries.rs`, the
duplication factored into [`common::HandlerError`] (one impl, one
`IntoResponse`). The per-resource hash helpers similarly moved into
common (`record_id_to_u64` + `identifier_to_u64`).
# Schema-divergence note
The MVP TimeEntry row uses lowercase `time_entry` as the table
name + carries Redmine-shape fields (`hours`, `spent_on`,
`comments`). The OGAR canonical schema emits PascalCase
`BillableWorkEntry` SCHEMAFULL with `billable: boolean` + 12 record
edges to upstream canonical types. The current store call
(`db.create("time_entry").content(...)`) lands the row in an
undeclared SCHEMALESS sibling table that does NOT engage the
SCHEMAFULL DDL applied at `Store::open`. Same divergence applies
to W1 (`Issue` / `project_work_item`) and W2 (`Project` / `project`).
Resolution lands when port-specific DDL emission ships alongside
the canonical (a separate sprint).
# DoD pinned by 11 new tests
Store (5):
- create + find round-trip with 2.5h + comment
- find returns NotFound for unknown id
- list empty for fresh store
- list returns 3 inserted rows (hours sum = 3.5)
- create without comment round-trips
Handlers (4) + common (1 new):
- list empty-state with data-class-id=0x0103
- list seeded entries with date + comment + hrefs
- detail by record key with subtitle = comment
- detail unknown id → 404
- common::identifier_to_u64 deterministic + distinguishes
# Status: 3/8 width tracks shipped
W1 ✓ W2 ✓ W3 ✓. Remaining: W4 User/Member/Role, W5 Taxonomy,
W6 Comms, W7 SCM-light, W8 Queries/Relations.
cargo fmt --check + cargo clippy --all-targets -- -D warnings +
cargo test --workspace all green locally; 76 tests across 5 crates.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9fdd5dfff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+109
to
+112
| let headline = format!( | ||
| "<a href=\"{}\" class=\"primary-link\">{} — {}h</a>", | ||
| href, &entry.spent_on, hours_str | ||
| ); |
There was a problem hiding this comment.
When a time entry is imported or created with a non-date spent_on string (Store::create_time_entry accepts any String), this value is interpolated into the headline_html passed to render_detail, whose contract treats the headline as already-rendered HTML. The list/detail cells go through escaping CellData, but this headline path will render raw markup/script on /time_entries/:id; escape the label or build the headline through a cell template before passing it as safe HTML.
Useful? React with 👍 / 👎.
AdaWorldAPI
added a commit
that referenced
this pull request
Jun 21, 2026
…ail-fast Closes the codex items left open across my Phase 0/1 PRs: # P1 — XSS via raw user content in handler-built HTML (#10, #12) `wrap_in_doc(title, body)` interpolated the raw `title` straight into the document's `<title>` element. With user-controlled inputs (issue subjects, time-entry spent_on, project names, user display names) reaching this fn, a payload like `</title><script>alert(1)</script>` is a stored XSS. The `render_detail(..., headline_html, ...)` parameter is contractually pre-rendered HTML (kit `|safe`s it through the template), so handlers that compose it from user data must escape themselves. W1 (Issue), W2 (Project), W3 (TimeEntry), W4a (User), W4b (Role) all did this with raw interpolation. Fix: - `common::html_escape` — minimal `& < > " '` escape, no extra dep. - `wrap_in_doc` escapes its `title` parameter. - Every handler that composes `headline_html` from user data now calls `html_escape` first. - New regression test `detail_escapes_xss_in_subject_for_title_and_headline` in `issues.rs` exercises the full XSS surface. # P2 — URL slug encoding (#13) `/roles/<name>`, `/projects/<identifier>`, `/users/<login>` paths interpolated the slug raw. A name with `#`/`?`/`/` would break the browser's URL parse (`#` = fragment, `/` = path segment). Fix: - `common::encode_path_segment` — percent-encodes everything outside RFC-3986 unreserved + path-safe chars. No new dep (rule is small). - Roles, projects, users all route hrefs through it. - New regression test `list_percent_encodes_role_names_with_reserved_chars` in `roles.rs` (Q/A → Q%2FA, R&D → R%26D). # P1 — RM_BIND fail-fast (#7) `std::env::var("RM_BIND").ok().and_then(|s| s.parse().ok())` collapsed unset and malformed into the same fallback. A typo silently started the server on the default. Now we distinguish: - Unset → default - Malformed → return `io::Error::other(...)` immediately # Skipped - #9 (auth routes not reachable) — already fixed by #10's `build_router_with` refactor that mounts `rm_auth::router(cfg)`. - #8 (lowercase vs PascalCase OGAR table names) — schema- divergence noted in W3's doc, lands in a dedicated PR; needs the per-port DDL emission story W4-followups will revisit. cargo fmt --check + cargo clippy --all-targets -- -D warnings + cargo test --workspace all green locally; 108 tests across 5 crates.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
W3 from the Redmine Integration Plan
Third Phase-1 width track. TimeEntry (canonical
billable_work_entry, codebook0x0103) — hours booked against a project / work item.GET /time_entriesGET /time_entries/:id:id= SurrealDB record key, like W1)Plan §1.6 factoring — "three points form a line"
W3 lands as the third caller of the per-resource scaffolding. With identical
HandlerErrorenums shipped across W1 (issues.rs), W2 (projects.rs), W3 (time_entries.rs), the duplication moves intocommon::HandlerError. Same forrecord_id_to_u64(W1/W3 use it) andidentifier_to_u64(W2 uses it; promoted to common since it's idiomatic for slug-keyed resources).After this PR a new width track is
~150 LOC of handlersinstead of~250 LOC of handlers + boilerplate.Schema-divergence note (forward concern)
The MVP TimeEntry row uses lowercase
time_entryas the table name + Redmine-shape fields (hours,spent_on,comments). The OGAR canonical schema emits PascalCaseBillableWorkEntry SCHEMAFULLwithbillable: boolean+ 12 record edges to canonical upstream types (Worker,Duration,RatePolicy, etc).The store call (
db.create("time_entry")...) lands the row in an undeclared SCHEMALESS sibling table, not the SCHEMAFULL DDL applied atStore::open. Same divergence applies to W1 (Issue) and W2 (Project). Resolution lands when port-specific DDL emission ships alongside the canonical (separate sprint).DoD pinned by 11 new tests
rm-store/src/time_entry.rsrm-handlers/src/time_entries.rs0x0103, list seeded entries + hrefs, detail by record key, 404 for unknownrm-handlers/src/common.rsidentifier_to_u64testAll three CI gates pass locally: fmt + clippy + cargo test --workspace (76 tests across 5 crates).
Status: 3 of 8 width tracks shipped
W1 ✓ W2 ✓ W3 ✓. Remaining tracks (parallel-ready per Plan §8 file ownership): W4 User/Member/Role, W5 Tracker/IssueStatus/IssuePriority/CustomField, W6 Comment/News/Message/Forum/Wiki/Attachment, W7 Repository/Changeset (read-only), W8 Query/IssueRelation.
🤖 Generated with Claude Code