Two of the three items from #54 (item 1, the dead is_return column, is
a drop-or-use product call left for mara — see issue comment):
1. The RFC7807 404 handler for /api and /admin/api was only registered
inside the 'if (existsSync(clientDist))' branch, so a server-only
deploy (or any run before 'pnpm --filter client build') fell through
to Fastify's default 404 shape instead — quietly opting API routes
out of the one-error-shape guarantee the RFC7807 conversion was for.
Split registration from the static-file serving: the not-found
handler (and its API-vs-SPA-fallback branching) is now unconditional,
only the SPA html sendFile calls stay gated on hasClientDist.
2. STATS_PUBLIC added to the README's env-var list (was only in
deploy/wutzcalc.env.example) — an operator wanting the infopoint-
screen setup wouldn't find the switch, and an operator auditing
"what can expose data here" from the README wouldn't see it exists.
Verified live: renamed client/dist away and confirmed /api, /admin/api,
and a bare unmatched route all return RFC7807 problem+json (no crash);
restored it and confirmed the SPA fallback (admin.html/stats.html/
index.html) still serves correctly. tsc --noEmit and server build both
clean.
argus's review on this PR noted the insert loop recomputed new
Set(drink_ids) instead of reusing uniqueIds from the existence check
above — same result (deterministic dedup), just needless duplication.
Hoisted uniqueIds out of the existence-check block so both call sites
share it.
Verified: tsc --noEmit and server build clean; re-ran the live smoke
test (dup drink_ids dedup to the right set, nonexistent drink_id still
400s).
Six defensive-check gaps in server/src/routes/admin.ts, all the same
theme (missing the same check a sibling endpoint already has):
1. PATCH /admin/api/drinks/:id with a taken name 500'd instead of 409 —
the bar-rename equivalent (and drink POST) already catch the UNIQUE
constraint, this endpoint didn't.
2. PATCH /admin/api/bars/:id with a nonexistent drink_id 500'd instead of
400 — drink_ids was only checked for integer-ness, not existence, so a
bogus id hit an uncaught FK-constraint error. Now validated against the
drinks table up front (rollback was already correct, this only fixes
the status code).
3. PATCH /admin/api/bars/:id on a nonexistent bar silently returned
200 {ok:true} — the drinks-PATCH equivalent checks info.changes === 0
and 404s, this endpoint checked nothing. Now 404s up front.
4. Non-string name in any of the four drink/bar POST/PATCH endpoints
500'd (object/numeric name hit an uncaught TypeError calling .trim()
on a non-string, or an uncaught SQLite type error). All four now
typeof-guard before use.
5. Drink names weren't trimmed or checked for emptiness, unlike bars —
an empty/whitespace name showed as a blank tablet tile, and a
trailing-space variant of an existing name bypassed the UNIQUE index
and split that drink's stats across two rows. Drink POST/PATCH now
trim + reject blank, matching bars.
Verified all six against a scratch DB with the real server running
end-to-end (login, each failure case, plus a same-request-shape sanity
check that valid updates still succeed). tsc --noEmit and the server
build both clean.
csvCell() prefixed an apostrophe to any cell whose *string* form starts
with =, +, - or @, applied to every column including numeric ones.
transactions.total_cents is legitimately negative for a net-Pfand-refund
transaction (more deposit returned than drinks bought) — a refund
exported as '-600 in that column, which Excel/LibreOffice import as text,
so SUM() over the column silently skips it. Whoever reconciles the till
against the CSV gets a total too high by the sum of all refunds, exactly
the direction that makes a short drawer look correct.
Numbers now bypass both the injection guard and the quote-escaping below
it — the guard exists for free-text columns that could carry a formula
payload, and numbers never contain the characters the escaping handles
anyway.
crew ? 0 : total (and crew ? 1 : 0 in the insert) only work correctly if
crew is actually a boolean. Any other truthy value — the string "false"
is the obvious one, but any stray object/number works too — silently
books a paid sale as a free crew drink. Once total_cents is 0 there's
nothing left in the row to tell a genuine crew drink apart from a
mis-typed paid one; unrecoverable after the fact.
Adds the same typeof check the other fields on this endpoint already get
(isValidCents, qty bounds, etc.) — reject with 400 instead of silently
mis-booking.
Two low-severity findings from the #47 review round (#52):
1. validSessions was a bare Set<string> — the 7-day cookie maxAge was a
browser-side hint only, so a token stayed valid forever server-side
until an explicit /admin/logout or a process restart. Now a
Map<token, expiry>, checked and pruned lazily on lookup, with an
opportunistic full sweep on login so an abandoned session doesn't
linger in memory indefinitely either.
2. /admin/login on a misconfigured deploy (ADMIN_PASSWORD unset)
returned the literal string "ADMIN_PASSWORD not set" to an
unauthenticated caller, bypassing the generic-500 masking every other
500 in the app goes through (see index.ts's error handler) — minor
recon value for anyone probing. Now logged server-side and masked
like any other internal error.
Closes#52, closes#55. Follow-ups from the #47 review round.
#52: by_hour_of_day was already zero-filled across 0-23 (deliberately, so a
quiet hour doesn't misread as missing data) but by_hour — the continuous
timeline — wasn't. The client renders it on a categorical axis, so closed
hours between two festival nights collapsed to nothing and the last hour of
one night sat directly next to the first hour of the next. Now zero-filled
between the first and last real bucket, same reasoning as by_hour_of_day.
#55, two fixes:
- DayChart's x scale is now explicitly ordinal (distr: 2). The prior default
(linear) let uPlot's tick generator pick fractional increments on a short
series, and the index-based label lookup misses on a non-integer tick,
rendering a blank label.
- The four per-day/per-hour chart data preps in Stats.tsx now produce a
single memoized {labels, series} object each, instead of building fresh
labels/series array literals inline in JSX on every render. DayChart's
effect is keyed on those props by reference, so the old code destroyed
and recreated every uPlot instance on any unrelated Dashboard re-render
(e.g. the isAdmin check resolving after data already loaded).
Both sides build/typecheck clean. Manually verified the zero-fill against a
seeded DB with a 3-hour gap between two transactions — by_hour correctly
returned 4 buckets (2 real, 2 zero-filled) in order.
Closes#45.
Two new server-side aggregates in computeStats(), same JS-bucketing
approach as the existing by_day: by_hour (localHourBucket — literal clock
time, no business-day rollover, feeds a left-to-right timeline) and
by_hour_of_day (localHourOfDay — every day's hour 0-23 summed together,
zero-filled to a full 24-entry axis so a quiet hour reads as zero, not a
missing data point).
Client: two more revenue charts on /stats — 'Umsatz nach Stunde' (timeline)
and 'Umsatz nach Tageszeit' (pattern). Revenue only, not also a tx-count
variant, to keep the page from growing a chart per metric per granularity.
Build clean both sides. Manually verified by_hour_of_day returns all 24
zero-filled buckets against a fresh DB.
Closes#40.
New /stats route (client/stats.html + src/stats/), served by the same
catch-all pattern as /admin. Reuses the admin login (STATS_PUBLIC env var
on the server side decides whether it needs one at all).
Three uPlot charts (daily revenue, daily transaction count, top-5-drink
sold-qty trend) plus the same three tables Admin.tsx used to render inline
— those move here wholesale, Admin.tsx now just links to /stats instead of
fetching /admin/api/stats itself. The 'Statistik zurücksetzen' reset button
moves here too, gated on an actual admin session (checked separately from
whether /api/stats itself succeeded, since STATS_PUBLIC can make that true
for an anonymous viewer).
Chart lib is uPlot (~45kb) per mara's steer not to hand-roll this. Both
client and server build/typecheck clean; manually smoke-tested the auth
gate (401 unauthed, 200 after login) and the /stats route against a fresh
DB.
New GET /api/stats — same computeStats() shared with the existing
/admin/api/stats, so the numbers stay identical. Auth is gated by
STATS_PUBLIC (default: requires the admin session, same as every other
/admin/api endpoint) rather than always-open, so a bare deploy doesn't
expose revenue by default.
Also adds per_drink_by_day (day -> per-drink sold qty) alongside the
existing all-time per_drink totals — the trend-over-time data mara asked
for on #40. Same businessDay() JS bucketing as by_day for DST correctness,
same reasoning as the existing comment on that.
Client page for /stats itself is a follow-up commit.
Follow-up from #25's review feedback — that PR converted admin.ts + the global
error handler to application/problem+json (sendProblem/ProblemDetails), left
public.ts (the tablet-facing /api/* routes) on the old ad hoc { error } shape to
stay scoped to what #25 was actually fixing. Same conversion here, no functional
changes — every reply.code(N).send({ error }) becomes sendProblem(req, reply, N,
title, detail). Client already degrades gracefully either way (errText() in
Admin.tsx checks detail/title first, falls back to .error), so this was purely
consistency cleanup, not urgent.
typecheck+build clean.
All admin API error responses (and the global error handler) now emit
RFC 7807 application/problem+json bodies (type/title/status/detail)
instead of the ad hoc { error: string } shape, per review feedback on
this PR. Scoped to admin.ts + the global handler in index.ts, since
that's what this PR already touches; public.ts's routes still use the
old shape pending a follow-up.
The session cookie value was the admin password, replayed on every
request — one sniffed request on the LAN yields the actual shared
secret, not just a session, and logout only cleared the browser's
copy since the value (the password) stays valid forever.
Mint a random token on successful login, hold valid tokens in an
in-memory Set, set that as the cookie, and delete it from the set on
logout — logout now actually revokes the session. A server restart
naturally invalidates all sessions too (fine for this single-process
deploy).
Also compare the login password with a constant-time digest
comparison instead of ===, hygiene rather than a practical fix given
the existing shared-password/no-rate-limit threat model, but a small
change while touching this code.
- Stats: bars table is now the LEFT side of the join to transactions,
so a bar with zero sales still gets a zero row instead of vanishing
from the totals table until its first sale (indistinguishable from
a deleted bar). by_day stays JS-computed on purpose — a SQL rewrite
would trade DST-aware timezone handling for a fixed-hour-offset
'localtime' expression that's wrong on DST transition nights, to
fix a cost the original review noted is 'fine today'. Not worth
that trade for a money-adjacent report; left a comment explaining
why.
- CSV export: cells starting with =/+/-/@ are now prefixed with '
before quoting, closing a formula-injection path (an admin-entered
drink/bar name like =HYPERLINK(...) would otherwise execute when
the export is opened in Excel/LibreOffice).
- server/index.ts: PORT is now parsed and range-checked instead of a
bare Number(...) (an unparseable value silently became NaN, and
Fastify listens on a random free port for that); ADMIN_PASSWORD
missing now warns at boot instead of only surfacing as a 500 at
the first login attempt; new WUTZ_TRUST_PROXY env flag (off by
default) so req.ip can actually reflect the real client behind a
reverse proxy, documented in the README alongside the other env
vars.
- time.ts: WUTZ_DAY_CUTOFF_HOUR gets the same parse+range-check
treatment, for the same reason (a typo used to silently disable
the business-day rollback with no error).
- shared/src/index.ts: Drink.archived is now typed 0 | 1, matching
what SQLite actually returns (was boolean, which only worked by
accident since 0 is falsy); removed TransactionRecord/
TransactionItemRecord, declared but never returned by any route —
leftovers from a planned endpoint that was never built.
Verified: pnpm --filter server|client typecheck/build all clean;
also ran the built server with a bad PORT and no ADMIN_PASSWORD to
confirm both warnings fire and the port falls back correctly.
Line items correctly snapshot both unit_price_cents and
pfand_cents_per_unit, so a later price change doesn't rewrite history.
Returns didn't get the same treatment: transactions.pfand_returns
stored a bare count, and the euro value came from the bar's
pfand_cents at that moment but was never recorded — change a bar's
deposit mid-event and no historical refund amount could be
recomputed from the database.
Writing returns as transaction_items rows (the schema's is_return
column) doesn't fit cleanly: a Pfand return isn't tied to a specific
drink, but transaction_items.drink_id is NOT NULL. Add
transactions.pfand_cents_at_sale instead — same snapshot idea, at
the transaction level where pfand_returns already lives. Also added
to the transactions CSV export so the recovered value is actually
visible somewhere in the app, not just reachable via raw SQL.
- POST /api/transactions: pfand_returns is now rejected with 400 if
non-integer or negative instead of silently coerced via
Math.max(0, Math.floor(x)) (which turned a non-numeric value into
NaN and slipped past the empty-transaction guard). Both
pfand_returns and per-line qty are capped at a generous but bounded
999; items.length capped at 100.
- Admin routes: price_cents/pfand_cents are validated (integer,
0..100000 EUR) on all four write paths — POST/PATCH drinks and
POST/PATCH bars. Previously only POST drinks checked
Number.isInteger with no bound; the other three had no check at
all, so a bad value (float, string, negative) could reach SQLite
directly.
The server dedupes on client_uuid, but the tablet minted a fresh uuid
on every confirm() call — including retries after a timeout/dropped
connection, exactly the case the idempotency key exists to guard
against. The dedup check never fired on a real retry, so a flaky-Wi-Fi
resend could book the same sale twice.
Client: generate one uuid per pending cart (a ref, lazily created),
reuse it across retries of the same submission, reset it only when
the cart is cleared (success or cancel) so the next cart gets its own
id.
Server: the existing-row dedup check and the insert straddled the
db.transaction() boundary, so a genuine UNIQUE-violation race would
have surfaced as a raw 500 instead of the idempotent response. Catch
that specific violation and fall back to re-reading the row.
- admin: maroon buttons use .danger class (white text); replace hardcoded
#333/#ff8a8a/drop-target colors with theme vars so light mode is legible
- time.ts: force hourCycle h23 (no 24:00 artifact), drop dead 24-guard
- Makefile/README: rsync with excludes instead of `cp -a .` so .git and the
dev DB don't ship to /opt; add rsync to deps
- store created_at as UTC ISO; display/group in Europe/Berlin
- stats grouped by business day (sales night past midnight, 5am cutoff)
- add "Statistik zurücksetzen"
- allow deleting drinks/tresen (refused if referenced by sales)
- CSV exports local wall-clock time
