Native <input type="time"> ignores general_time_format (browser-locale
controlled; lang hint failed in Safari and Ralf's Chrome for both en-GB
and de-DE). Replace with a custom TimeField text input that displays per
general_time_format (24h "13:00" / 12h "01:00 PM") and stores canonical
HH:MM, parsing tolerant free-text on blur. Keeps the fixed-width
alignment fix.
Business-hours time pickers used lang="en-GB", which didn't render 24h in
Chrome. Switch to lang={timeFormat==='12h'?'en-US':'de-DE'} — the same hint
HoursSection/Quote/Bill/Contract editors use — so the picker shows 24h in
Chrome/Edge. Keep the fixed-width plain <input> for column alignment.
The shared Input wraps fields in a w-full div, so two per flex row split
the width and the trailing +/trash buttons knocked columns out of
alignment. Use a plain fixed-width <input> for the start/end time fields.
Route 10 surfaces that hardcoded 12-hour date-fns patterns
('h:mm a', 'PPp', 'p', toLocaleTimeString) through useLocalizedDate's
formatDateTime/formatTime so they respect general_date_format +
general_time_format: backup/restore, archives, photo viewer, feedback,
event details, gallery timeline, public quote page, CMS save indicator.
Also pin a lang hint on the business-hours native time inputs (Chrome/Edge
render 24h). Drops now-unused date-fns imports.
Native <input type="time"> renders AM/PM from the browser locale, ignoring
general_time_format. Pin a lang hint (en-GB for 24h, en-US for 12h) so
Chrome/Edge render the admin's chosen format. Display-only; the stored
value was already 24h HH:MM.
The t() calls on the Features tab, CRM settings tab, contract editor,
and settings nav carried English fallbacks but the keys were absent from
both locale files, so DE rendered English. Add ~67 keys to en.json +
de.json (DE native): the full contracts.editor.* subtree (whole page was
English), settings.features.{contracts,crmDevelopment,hoursLogging}.*,
crmSettings contracts/dashboard-overview/ToS labels, and two settings
nav titles. Also drop a dead duplicate bills.field.sourceQuote key in de.
No code changes — additive translations only.
Add a paginated, filterable view of the email_queue (recipient, type,
status, queued/sent timestamps, error, event link) as a third tab in
Email config, beside SMTP + Templates. Filters: status, recipient/type
search, created-at range. email_data is never exposed. Pairs with the
"Send queued emails now" flush — flush, then watch what sent/failed.
Add a "Decline on behalf" action mirroring accept-on-behalf, for when a
customer says no by phone/email. Flips a draft/sent/expired quote to
declined, stamps declined_at, closes the public response window, and
invalidates outstanding accept/decline tokens so the emailed link can't
toggle it back. Optional free-text reason persisted to a new
quotes.decline_reason column (migration 115) and shown on the quote
detail page. Hard-delete intentionally not included.
Move the scheduled-email business-hours floor onto the business profile
as Google-style per-weekday opening blocks (multiple blocks/day for lunch
breaks). Migration 114 adds business_profile.business_hours (JSON) +
scheduled_email_floor_enabled; emailProcessor snaps a queued email to the
next open block, read in the profile timezone. Editor lives under
Settings → Business profile.
Add an admin "Send queued emails now" flush (POST /admin/email/flush-queue)
that drains the queue immediately, ignoring the business-hours floor — the
escape hatch before maintenance/updates. processEmailQueue now takes
{ignoreSchedule, limit} and returns send counts; the scheduled interval
run is unchanged.
When no customer is selected, list every customer with unbilled hour
entries — entry count, total hours, and open amount (resolved via the
override → customer-rate → install-default chain). Rows with no
resolvable rate are flagged "Rate not set" rather than undercounted.
Click a row to drill into the per-customer logging section.
Backend: getUnbilledSummaryByCustomer() + GET
/api/admin/customers/hour-entries/unbilled-summary (customers.view).
Hour-entry saves hard-failed with an English-only error when a customer
had no rate, and the standalone hours page showed a disabled rate field
that looked set. Add a global business_profile default_hourly_rate_minor
(migration 113) as the last link in the rate chain
(entry override → customer → install default), so saves succeed with the
global rate. When no rate resolves anywhere, replace the save-time error
with a read-only resolved-rate display + a CTA to set a customer or
install-wide rate, disable Add-entry until a rate/override exists, and
translate the backend HOURLY_RATE_REQUIRED toast (en+de).
Replace the sort <select> dropdowns on the invoice, quote and contract
list pages with clickable column headers that toggle asc/desc and show a
chevron indicator. Adds a shared SortableHeader component + useColumnSort
hook that maps clickable columns onto the server-side sort enum.
Make issue date (newest first) the standard sort on all three lists,
set at the frontend, route and service layers. Adds issue_asc/issue_desc
to invoices and an "Issued" column to the bills table so the default is
visible and toggleable. Extends sort coverage so every clickable column
has both directions (+customer_desc on all; +issue_asc/desc on
quotes/contracts). Storno rows remain listed.
The Anlass field rendered inv.eventDate verbatim (raw ISO from pg
date-as-Date serialization) while every other date on the card went
through useLocalizedDate. Route it through fmtDate so it honors the
general_date_format setting. (The event_id → /admin/events linkify was
already in place.)
The historical-invoice import form had no event field, so imported rows
landed with event_name = NULL even when the admin knew the occasion. Add
free-text Event name + Event date inputs to the import modal, thread them
through billsService.importHistorical and the POST /admin/invoices/import
validator, and store them in the event_name/event_date snapshot columns
(migration 107). event_id stays NULL — no FK, since the event may predate
picpeak. Autocomplete-to-event_id linking deferred as a future bonus.
The business-profile country field — the seed source for the customer-create
country default — was still a free-text input placeholdered "FL", which could
reintroduce the non-ISO "FL" code that migration 110 normalized to "LI" and
re-open the create/edit CH-vs-FL default mismatch. Swap it for the shared
CountrySelect so every surface stores ISO alpha-2. The free-text countryName
verbatim-PDF override (migration 107) is unchanged.
Adds customer_accounts.skonto_disabled (migration 112) so a customer
that negotiated "no early-payment discount" can be flagged once instead
of ticking the per-invoice toggle on every invoice. resolveSkontoPercent
ForInvoice and the PDF render context both honour it, extending the
resolution chain to customer → invoice → snapshot → quote → global.
Checkbox added to the customer detail Billing card (en + de).
A scheduled invoice's issue_date was stamped at creation, so a long-
scheduled invoice printed a stale date by the time it shipped — the
relative Skonto window ("pay within N working days") and the net-days
due date were then counted from the authoring day, not the send day.
sendInvoice now stamps issue_date = send date on the first send and
re-derives the due date from it, preserving a manual due-date override.
Adds resolveNetDaysForRow to read net days from the persisted snapshot.
Deselecting Skonto before the scheduled send already propagates (the
scheduler re-reads the row fresh and the render context honours
skonto_disabled); no change needed there.
The Anlass / event name on the invoice detail page and the bills list
now links through to /admin/events/:id when the invoice references a
real event row. The list link stops propagation so it doesn't trigger
the row's invoice navigation. Falls back to plain text when the invoice
carries only a free-text event snapshot. Customer portal unchanged
(no admin route access).
Due date now derives from (scheduled send date else issue date) plus the
selected Net-days template, both in the editor and on save. The bill
editor renders it read-only with an Override toggle for manual entry;
existing invoices preserve their stored due date. Backend adds a single
resolveNetDays resolver that honors the split payment-net-days template
(previously only the legacy FK was read) and the
crm_payment_default_net_days setting, used by createInvoice and the
installment-spawn path alike.
The invoice-import endpoint stamped sent_at and paid_at with the moment
of import (new Date()) instead of the document's historical dates. The
CRM dashboard "Revenue · last 30 days" card keys on paid_at, so a
year-old paid invoice imported today wrongly counted toward the rolling
window. The dashboard windowing is correct (cash-basis "received in the
window") — the bug was the wrong paid_at on imported rows.
POST /admin/invoices/import now anchors sent_at to issue_date and
paid_at to issue_date (or an optional new paidAt param when the admin
knows the real payment date), never to import time.
Migration 111 backfills rows imported under the old behaviour: for every
invoice with imported_pdf_path set, sent_at/paid_at are reset to
issue_date. The old code never captured a real payment date, so
issue_date is the only sensible anchor. Idempotent and scoped strictly
to imported rows, so picpeak-issued invoices are untouched.
paid_at/sent_at are operational timestamps, not the invoice's immutable
legal content, so correcting the import-time error is safe under the
§14/§11 UStG immutability rule.
Replace the free-text 2-char country code field on the inline customer
create form and the customer detail page with a dropdown that shows
localized country names (Intl.DisplayNames, no hardcoded map) while
still storing the ISO 3166-1 alpha-2 code. The create form now seeds the
default country from the business profile instead of leaving it blank or
guessing CH/FL. The free-text countryName override is kept for the rare
case where an operator wants a custom display string.
Standardize Liechtenstein on the ISO code LI instead of the colloquial
plate code FL so it matches the PDF renderer's locale-aware lookup and
the new dropdown. Migration 110 normalizes existing FL rows to LI on
customer_accounts and business_profile (idempotent, case-insensitive).
Require at least one human-readable identifier (company name or a
contact name) at create time so the form can't produce a nameless row
that's impossible to recognise in lists later. Enforced on both the
frontend (isValid + toast) and the backend POST /admin/customers
validator so the API can't be bypassed.
i18n: en + de updated; other locales fall back to inline English
defaults and should get a native review before release.
Admin date inputs were inconsistent: raw <input type="date"> on event
creation and the bill editor rendered in the browser locale (en-US users
saw MM/DD/YYYY regardless of Settings -> General), while the historical-
invoice import modal used a private LocalizedDateField that displayed the
configured format but showed a text box plus a tiny native date stub
side-by-side ("two date fields, looks corrupted").
Extract a single shared LocalizedDateInput that displays/parses in the
configured general_date_format on every browser and opens the native
picker via a calendar icon button (showPicker on a visually-hidden native
input), so there is one date field, not two. Wire it into event creation,
the bill editor (event/issue/due dates), the import modal, and the tax-
report range filters (dropping the Chromium-only lang={dateInputLang}
workaround there).
Caught during the round-4 e2e validation on real PG: every
successful restore landed with `status='completed', was_successful=false`
because the success-branch update only wrote `status` but not
`was_successful` (column default is false). Visible side effect: the
BackupDashboard's "last successful restore" filter would skip the
row + any future audit query gating on was_successful would miss it.
One-line cure: include `was_successful: true` in the success-branch
update payload. Inline comment explains why and references the
review note so future edits keep the two fields together.
Source-inspection test in restoreService.pgBranch.test.js pins the
contract: after `performPostRestoreVerification(...)`, the
`status: 'completed'` update payload must also contain
`was_successful: true`. Future refactors of the success payload that
drop the flag fail the test before merge.
36/36 backup-related integration tests pass.
End-to-end DR cycle surfaced one more PG-only landmine — and it
turned out to be a side-effect of the round-1 replay placement, not
a new bug. Round 2 fixed the comparison logic; round 3 fixes the
ordering.
Symptom on real PG install:
[install-from-backup] FAILED — Post-restore verification failed:
Table app_settings row count mismatch: expected 190, got 191.
Trigger file left in place for retry.
Root cause: the operator-meta replay (introduced in round 1) ran
INSIDE performDatabaseRestore, lined up BEFORE the post-restore
verification step in the parent restore() method. So:
1. psql restores app_settings → 190 rows (matches backup)
2. Replay upserts `restore_allow_force_auto_upgraded` (which the
fresh-install seeded but the backup didn't have) → 191 rows
3. performPostRestoreVerification counts 191, manifest says 190,
verification fails the row-count check.
Replay is doing the right thing (preserving operator policy). The
verification is doing the right thing (counts must match). They
disagree because the replay landed in the wrong sequence relative
to verification.
Cure: move the replay out of performDatabaseRestore and into
restore() AFTER `performPostRestoreVerification` passes.
Verification now sees the as-restored DB (matches the backup
exactly), replay layers on top once verification has signed off.
Mechanism: snapshot stashed on `this.preservedMetaSnapshot`
(initialised in constructor, reset per run at the top of restore()).
performDatabaseRestore writes it in the PG branch before DROP;
restore() drains it after verification. SQLite leaves it empty,
both steps no-op there.
Tests:
- Updated `restoreService.pgBranch.test.js` to pin the new shape:
* `this.preservedMetaSnapshot` is initialised in the constructor
* No stray `let preservedMeta = []` local declarations anywhere
* Replay drain (`this.preservedMetaSnapshot.length > 0`) sits in
restore() AFTER `performPostRestoreVerification(...)` and is
lexically OUTSIDE `performDatabaseRestore`.
- The bigint-as-string contract from round 2 still holds.
34/34 backup-related integration tests pass.
pg-driver serialises `bigint` (which is what `COUNT(*)` returns) as a
JavaScript STRING to preserve precision for huge counts. The manifest
stores `expected.rowCount` as a JS number (parseInt'd at
databaseBackup.js:118). Strict `!==` in performPostRestoreVerification
flagged every match as a mismatch on PG:
Table activity_logs row count mismatch: expected 16, got 16
Table admin_users row count mismatch: expected 1, got 1
Table app_settings row count mismatch: expected 165, got 165
... (every table, all matching)
Symptom matched the preservedMeta scope leak from round 1: install-
from-backup logged FAILED, trigger file wasn't cleaned, data was
actually intact. Caught on PR #596 e2e re-run.
Cure: coerce both sides with `Number(...)` at the comparison AND in
the interpolated value so the warning text renders `16` not `"16"`.
Pre-emptive: lines 448 + 458-459 had the same string-vs-number issue
masked by `>` (JS coerces operands for `>`), but the warning text
printed `"5"` on PG vs `5` on SQLite, and a future patch changing
`>` to `=== 0` or `!== expectedCount` would silently break on PG.
Coerced at the read site into `eventCountN` / `activeUsersN` locals
+ added a comment block explaining the contract so future edits
don't drop the Number() calls without re-auditing.
New source-inspection test: pins the contract that every `.count`
result in restoreService.js MUST be wrapped in `Number(...)` when
used in a comparison (===/!==/>/</>=/<=). Same source-inspection
pattern as the preservedMeta test added round 1 — pragmatic until
the real-PG integration test follow-up lands.
The maintainer's audit of the rest of the backup/restore surface
(_installFromBackupBoot, _restoreSettingsBoot, _backupPathsBoot,
backupCoverageService, backupIntegrityService, backupService,
databaseBackup) confirmed no other bigint-as-string sites — the
class is now closed in the audited scope.
Two nice-to-haves from the PR #596 review.
1. Install-from-backup logging mirrors to stdout
The winston logger writes to /app/logs/combined.log and may not
tee to stdout. Operators tailing `docker logs picpeak-beta-backend`
after a `compose up` saw the migration sweep + npm notice and
nothing about the restore. Three key events now also fire through
`console.log` with a `[install-from-backup] ` prefix:
- "trigger file detected → <manifest>"
- "starting restore from <manifest>"
- "restore completed successfully" / "FAILED — <reason>"
Plus the "skipping — existing data" branch.
docker-logs surface now tells the restore story without requiring
an `exec into the container` step.
2. ADMIN_CREDENTIALS.txt flags stale creds when restore is queued
Migration 001 detects a pending `RESTORE_ON_INSTALL` file BEFORE
writing the fresh-install credentials file. If a trigger will fire
on the next boot, the file now opens with a clear warning:
⚠️ RESTORE_ON_INSTALL TRIGGER DETECTED ⚠️
These credentials are temporary. An install-from-backup run is
queued to fire on the next server start, which will REPLACE
this admin row with the one from the backup. After the restore
completes, log in with your ORIGINAL pre-disaster credentials
— not the ones below. If the restore fails for some reason,
the credentials below remain valid as a fallback recovery path.
Doesn't skip the file (so a failed restore still has the fallback
credentials), just annotates it. Closes the maintainer's "stale
junk credentials" observation.
`preservedMeta` was declared with `let` INSIDE the PostgreSQL else
branch of performDatabaseRestore (~L850), then read AFTER the else
block closed at the shared replay site (~L1030). On every real PG
restore, this threw:
ReferenceError: preservedMeta is not defined
after psql had already loaded the data successfully. Knock-on
effects per the maintainer's review:
- Loud `Install-from-backup: FAILED` line in combined.log even
though the data restored cleanly
- Trigger file in `_installFromBackupBoot.js` was left in place
because the success branch never ran — admin had to manually
rm it before the next boot
- The operator-meta replay (restore_allow_force,
restore_allow_force_auto_upgraded) silently dropped, exactly
the chicken-and-egg the snapshot was added to close.
`restore_allow_force` reverted to the backup's value on every
PG restore.
CI missed it because integration tests around `performFullRestore`
only exercise the SQLite branch (`this.dbType === 'sqlite'`). The PG
branch requires a real psql binary + cluster, which lives in the
"real-PG integration test in CI" follow-up.
Cure: hoist the `const PRESERVED_META_KEYS = [...]` + `let
preservedMeta = []` declarations above the SQLite/PG split. SQLite
leaves them empty; PG branch fills them; replay block at the bottom
reads them on both paths (no-op on SQLite).
New test: `restoreService.pgBranch.test.js` pins the scope contract
via source inspection. Two assertions:
1. Exactly one `let preservedMeta = []` declaration in the file,
positioned before the SQLite/PG branch split
2. The replay block `if (preservedMeta.length > 0)` sits outside
the else block (closing ` }` exists between the branch
opener and the replay site)
Source-inspection beats a runtime test here because (a) it doesn't
need a real PG cluster + psql binary, (b) it pins the EXACT property
that broke, more directly than a runtime test would.
Closes PR #596 review blocker.
upstream/beta independently shipped 108_seed_sl_email_template_translations.js
(Slovenian email template translations) using the migration number
this branch had already claimed for 108_add_backup_paths.js. Knex's
filename-based ordering would have caused both to attempt the slot
at merge time.
Renamed via `git mv` so file history is preserved. All five
references updated in lockstep:
- backend/src/services/_backupPathsBoot.js (require + comments)
- backend/src/services/backupService.js (LEGACY_BACKUP_PATHS comment)
- 3 integration test files (require + "migration 108" prose)
- migration's own header comment, with a paragraph explaining the
rename so reviewers don't wonder why the number jumped
**No data-migration impact for installs that already ran the
108-named version** (Ralf's beta, primarily): the migration's body
is idempotent — createTable is guarded by `hasTable`, and the seed
uses `onConflict('path').ignore()`. So when 109 runs against an
install whose backup_paths table is already populated, both the
schema step and the seed step no-op cleanly. The orphaned
`108_add_backup_paths.js` row in the `migrations` tracking table
sits harmlessly alongside the new `109_add_backup_paths.js` row.
No data lost, no double-insert, no schema drift. Mechanical rename
ahead of the PR opening.
The previous split (separate docs/install-from-backup.md + separate
README link for "Disaster Recovery") fragmented what's conceptually
one workflow: backup → restore. DR is a specific scenario of restore
(the destination is wiped), not a separate feature.
This merge:
- Folds install-from-backup content into docs/backup-restore.md
as a "Disaster recovery (install from a backup)" section with
its own table-of-contents anchor.
- Adds an explicit ToC at the top so admins land on what they
need in one click.
- Frames the two restore paths up front: "live install" → wizard,
"fresh / wiped install" → trigger file. Admins encountering DR
in panic mode don't need to know to look under a separate link.
- Drops the duplicate "Disaster Recovery" README bullet. The
"Backup & Restore" blurb now mentions DR explicitly so it's
still findable via Ctrl+F on the README.
- Removes docs/install-from-backup.md (its content is now in
backup-restore.md's DR section).
Single source of truth = less risk of one doc going stale relative
to the other when the feature evolves. Maintainer-facing surface
on docs.picpeak.app shrinks back to one /guides/backup-restore page.
The four backup admin panes (BackupHistory, BackupDashboard,
BackupCoverageCard, BackupIntegrityCard) used raw date-fns
`format()` with hard-coded tokens like 'p' (12-hour AM/PM), 'PP',
'PPP', 'PPp', and 'yyyy-MM-dd HH:mm:ss' — ignoring the admin's
configured `general_date_format` and `general_time_format`
settings.
Net effect on a 24h-configured install: backup History row showed
"11:25 PM" instead of "23:25", and the Coverage tab's "Last dump"
+ "Coverage generated" timestamps were stuck on
yyyy-MM-dd HH:mm:ss regardless of the admin's date-format choice.
All four panes now route through `useLocalizedDate()` which honors
both settings + the active i18n locale (per the existing
[[feedback_respect_general_format_settings]] pattern).
Tokens replaced:
format(date, 'p') → formatTime(date)
format(date, 'PP') → format(date)
format(date, 'PPP') → format(date)
format(date, 'PPp') → formatDateTime(date)
format(date, 'yyyy-MM-dd HH:mm:ss') → formatDateTime(date)
format(date, 'yyyy-MM-dd HH:mm') → formatDateTime(date)
No backend changes — settings already shipped via /admin/settings;
this just makes the consumers actually read them.
Closes the six-step DR dance ("onboard throwaway admin → restore via
wizard → log out → log back in with originals") by letting admins
recover an install with zero clicks past `docker compose up`.
Convention: drop a file named `RESTORE_ON_INSTALL` (no extension OR
.txt) into the existing `/backup` bind mount. On next container
start, the new boot hook detects it, runs the restore, and starts
the server with the restored state. Admin opens the browser, login
works first try.
Payload variants:
- empty file → auto-picks newest backup-manifest-*.json from
/backup/manifests/. Useful for "restore the latest".
- path inside the file → uses that specific manifest. Useful for
"I want this older backup, not the most recent".
Safety gates (three layers):
1. Trigger file must exist — no auto-magic, admin signals intent
2. DB must be empty (no events, ≤1 admin) — refuses to clobber
production data
3. Restore failure leaves the trigger file in place for retry on
next container start. Success deletes it so subsequent boots
don't redo the work.
Override hook: INSTALL_FROM_BACKUP_FORCE=true skips guard #2 for the
"I know what I'm doing" edge case (dev env rebuilds, etc).
No docker-compose changes required — uses the bind mount picpeak
already has, env vars are optional. The minimal admin workflow now
matches the bare-minimum mental model: "copy my backup files,
restart the container, log in with original credentials."
Tests: 7 scenarios covering trigger detection, payload variants,
safety gates, success/failure trigger-file lifecycle.
Closes the last gap from tonight's backup-hardening: backup_runs.
statistics now carries a `per_path` map keyed by backup_paths.path
(e.g. `events/active`, `business-docs`), with per-bucket count + size.
Backend (backupService.js):
- new `computePerPathStats(backedUpFiles, allFiles)` helper that
bucket-sorts each backed-up file into its owning backup_paths row
by longest-prefix match. Reuses the same backup_paths source the
walker reads, so toggling include_in_default off propagates
correctly. Falls back to LEGACY_BACKUP_PATHS if the table is
missing.
- runBackupInternal calls it after the destination implementation
reports back, includes the result in statistics under both
snake_case (`per_path`) and camelCase (`perPath`) keys for the
same alias treatment the existing fields get.
Frontend (BackupHistory.jsx):
- Backup History detail pane now renders one row per per_path entry
when present, with path label + count + formatted size.
- Falls back to the legacy Photos / Archives / "Other" rendering
when the field is absent (backups taken before this commit). No
breaking change for stored history.
Tests: new backupService.perPathStats.test.js — 2 scenarios pinning
attribution behaviour (single-path, nested-paths-don't-collide).
Plus a NOTE comment about overlapping-path walker behaviour (out of
scope; canonical seed doesn't hit it).
Closes the chicken-and-egg where `restore_allow_force` (and its
auto-upgrade tracking flag) got overwritten on every restore by
whatever value happened to be in the backup. Net effect:
1. Admin enables Force Restore (via tonight's default-ON migration
edit, or hand-SQL on older installs).
2. Restore runs successfully.
3. Restored DB has `restore_allow_force = <backup's old value>`.
4. Next restore attempt: "Force restore is not allowed by system
settings" — admin needs the SQL workaround AGAIN.
Cure: snapshot a small list of operator-meta keys BEFORE the DROP
DATABASE (while we still have a working pool against the OLD DB),
then UPSERT them back AFTER the psql restore + migrate.latest.
The preserved set is intentionally narrow — currently just
`restore_allow_force` and `restore_allow_force_auto_upgraded`. These
are about how the operator wants the install to behave, not user-
facing state. Adding more keys is a one-line addition to the
PRESERVED_META_KEYS constant.
Survives both:
- backup is OLDER than the operator's most recent setting change
- backup is NEWER but had a different operator policy
Either way, the post-restore install reflects the LIVE operator
policy, not the backup's snapshot of it.
Closes the loop on the original 2026-05-29 data-loss class: Ralf had
four "Run Backup Now" manifests sitting on disk with
database.backup_file = null because Stage A wasn't yet in place.
The restore wizard would have happily restored any of those four,
bringing back files (photos, PDFs) but leaving the database empty —
silently re-creating the exact data loss the rest of this branch
prevents going forward.
The /restore/list-backups endpoint now returns `database_included`
per row (parsed from the manifest at discovery time). The wizard
uses it to:
- Per-row badge: red "No DB" pill next to any backup where
database_included === false. Tooltip explains the consequence
in plain English: "restoring this will NOT recover the database".
- Selected-card callout: full red banner under the chosen row
when database_included is false, restating the warning + giving
the admin a clear path: "pick a different backup if you have
one with a database dump, or proceed only if files-only is
what you want."
The wizard does NOT block the restore — the admin may genuinely want
a files-only restore (e.g. recovering a deleted photo while keeping
current DB state). The warnings make sure that choice is informed.
The dashboard widget used `lastBackup.created_at` for the "Last
successful backup: X ago" text — but lastBackup is the most recent
row of any status. So a crashed restore (status=running, never
updated) or a recent failure showed up labeled as the last
successful backup. Same "silent failure not surfaced" class the
restore wizard had.
Backend now returns:
lastSuccessfulBackup — most recent backup_runs with status='completed'
zombieRuns — running rows older than 30 min (likely crashed mid-flight)
lastBackup — unchanged (most recent any status)
Frontend renders:
- "Last successful backup: X ago" — always from lastSuccessfulBackup
- "Last attempt: Y ago · failed/running" — when lastBackup differs
from lastSuccessful. failed shows the first line of error_message
in red; running stays neutral.
- Zombie callout — "N backup(s) running >30min — may have crashed"
in amber, so admin sees stuck rows at a glance.
- Health score downgrades from "excellent" to "warning" if the
latest attempt failed, even when older successes keep the age
fresh — surfaces regressions without erasing the green history.
The progress step used a binary `isRunning ? "in progress" : "completed"`
check. So when the backend rejected the restore (pre-flight validator
threw, path error, etc.) the wizard cheerfully rendered "Restore
completed" with 0% progress and no error context — the admin had to
SSH into the server and inspect `restore_runs.error_message` to find
out what happened.
Now reads the most recent row from `restoreStatus.history[0]` and
renders one of three states:
- running → blue text, progress bar updates
- succeeded → green tick + post-restore actions (existing behaviour)
- failed → red banner with the first line of error_message, and
a callout if was_rollback_attempted is true so the
admin knows the destination is safe to retry on top of.
Net: the wizard now tells the truth about what just happened.
`db.destroy()` during restore tore down the in-process connection
pool to release PG sessions so DROP DATABASE could succeed. After
CREATE DATABASE + psql restore, the old code did
`require('../database/db')` expecting a fresh instance — but Node
caches require results, so it got the SAME destroyed instance back.
Every subsequent query in the process failed with "Unable to acquire
a connection" until the container was manually restarted, even
though the restore technically succeeded.
Net effect for admins: login showed "An error occurred", customer /
invoice / quote pages were blank, no surface hinted at the dead pool.
Cure: db.js now wraps the live knex instance in a Proxy that forwards
to a mutable internal reference, with a `reinitPool()` function that
destroys the old instance + builds a fresh one + probes with `SELECT 1`
so any reconnect failure surfaces immediately. The thousands of
existing `const { db } = require(...)` imports work unchanged — they
capture the Proxy once, and every call goes through to the current pool.
restoreService calls reinitPool() after CREATE DATABASE and before
migrate.latest(), so the rest of the request + every subsequent admin
action runs against the fresh pool. Container restart no longer
needed after restore.
Root cause of the persistent "Force restore is not allowed by system
settings" error even on fresh installs after `docker compose down -v`:
migrations/core/032_add_restore_runs_table.js seeded the row with
`JSON.stringify(false)` = the literal string 'false'.
So every install (fresh OR upgraded) wrote restore_allow_force=false
at migration time. The boot self-heal added earlier today saw the row
and respected "admin policy" per its safety design — never noticing
that the row was the deprecated migration default, not an explicit
admin choice.
Cure follows [[feedback_migration_no_compensation]] +
[[feedback_self_heal_pattern]]:
1. Edit migration 032 IN PLACE — flip seed value from false to
true. Fresh installs forward get the correct default at install
time, no boot helper needed.
2. One-time auto-upgrade in _restoreSettingsBoot.js for installs
that already ran the OLD migration. Bumps restore_allow_force
to 'true' iff the current value is the deprecated literal
'false' AND the new tracking key
`restore_allow_force_auto_upgraded` doesn't yet exist. The
tracking flag is always written after the first boot pass, so
subsequent admin choices (e.g. deliberately disabling force)
are preserved on every boot after.
3. Defensive: adminRestore.js getRestoreSettings() now normalizes
'true'/'false'/'"true"'/'"false"' string shapes to JS booleans,
not just '1'/'0'. Belt-and-suspenders so any future seeder that
uses a different boolean serialization doesn't silently break
the !settings.restore_allow_force gate.
Net effect: any picpeak install pulling this image — fresh or
existing — gets restore_allow_force=true on first boot after the
upgrade. The catch-22 that forced every disaster-recovery admin to
hand-write SQL before their FIRST restore is closed.
Fresh installs of picpeak had `restore_allow_force` defaulting to
false (or missing entirely). Combined with the "1 active admin
user" pre-restore warning that the fresh-install admin auto-creates,
this meant the very first restore on every new install hit:
Force restore is not allowed by system settings
Admins then had to hand-craft SQL to flip the setting before they
could recover their data — at the worst possible moment, when they
were already mid-disaster.
This isn't security: the admin who can SQL the setting on can also
flip it via the UI. It's just a sharp edge that bites every new
install once.
Cure: boot-time self-heal that seeds restore_allow_force=true only
when the row doesn't exist. Existing installs that explicitly set
the row (true OR false) are NOT touched — admin policy wins.
Pattern mirrors _backupPathsBoot.js and _emailTemplateBoot.js.
Default-ON rationale matches Stage A's principle: the cost of
forgetting (= can't recover from a disaster) outweighs the friction
saved (= adversarial admins can't run forced restores). Audit
logging keeps the accountability story intact.
The "Content Backed Up" panel in Backup History only counted two
categories (Photos + Archives), so a 3-file backup that landed all
3 in business-docs (Ralf's case after the storage truncation +
restore tonight) showed:
Photos (0 of 0)
Archives (0)
→ total: 3 files
The discrepancy made admins wonder where the 3 files actually went.
Adds two rows:
- "Business documents & other" = files_processed - photos - archives
- "Total files" = files_processed
So the math adds up regardless of which Stage B path the files came
from. Properly per-path-category breakdown requires backend-side
per-path counters (separate follow-up); this commit closes the
visible-discrepancy gap without that schema change.
i18n: en + de added; other locales fall back to en until reviewed.
pg_dump emits setval() statements for SERIAL/IDENTITY columns, but
they don't always land cleanly: --clean ordering, knex pool sequence
caching, rows inserted mid-restore (the pre-restore safety backup
writes a database_backup_runs row before DROP), etc. Net result on
Ralf's install after a successful restore:
- "A record with this value already exists" on every CRUD action
- duplicate key value violates unique constraint
"database_backup_runs_pkey" on the next Run Backup Now
Same root cause: every SERIAL column's sequence was pointing at or
below MAX(id), so the next INSERT collided.
Fix: append a DO block after the psql restore that walks pg_class +
pg_attribute and setval()s every public-schema sequence to
GREATEST(MAX(<col>), 1). Cheap (a few ms even on large schemas),
safe (read-only on row data), idempotent — re-running it just
re-asserts the same values.
Seventh latent PG-restore bug discovered on Ralf's install tonight.
Manual hand-fix worked; this commit makes the fix automatic for
every future restore.
PostgreSQL refuses DROP DATABASE while any session is connected:
ERROR: database "picpeak_prod" is being accessed by other users
DETAIL: There are 6 other sessions using the database.
The backend's own knex pool holds 5-25 active connections to the
target DB. So even after closing the request that initiated the
restore, the pool keeps the DB busy and the DROP statement fails.
Three-layered cure, all in the restore service's PG branch:
1. Call `db.destroy()` first to close the in-process knex pool so
we don't fight ourselves. Knex will lazily re-open on the next
query via db.js's retry logic, so this is safe to do mid-restore.
2. SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE
datname=<target> AND pid<>pg_backend_pid() — evicts any sessions
from other processes (other server replicas, leftover idle
transactions, things our own pool destroy missed).
3. DROP DATABASE IF EXISTS "<target>" WITH (FORCE) — PG13+ kills
remaining connections atomically with the DROP. Falls back to
plain DROP on older Postgres where WITH (FORCE) is a syntax error.
Surfaced as the FIFTH latent bug in the restore path tonight: the
DROP DATABASE statement always assumed a quiescent destination, but
the live backend keeps the destination busy at all times. Every
previous PG install of picpeak that ever tried Restore would have
hit this — meaning the disaster-recovery feature has shipped broken
for a long time without anyone exercising it end-to-end.
`psql` with no -d connects to a database whose name matches the
connecting user. On installs where the user's home DB doesn't exist
(common pattern: DB_USER=picpeak, DB_NAME=picpeak_prod, no `picpeak`
DB), the restore's DROP DATABASE / CREATE DATABASE statements failed
with:
FATAL: database "picpeak" does not exist
even though the target DB (picpeak_prod) was alive and connectable.
And of course you can't connect to the target DB itself for DROP —
PostgreSQL refuses while a connection is open to it.
Fix: explicitly connect to `postgres` (the maintenance DB every PG
cluster ships with) for the DROP/CREATE statements. Override via
DB_CHECK_DB env var if the `postgres` DB is restricted to superusers
on the cluster — matches the pattern wait-for-db.sh already exposes.
Also quote the database name in the SQL so installs whose DB has
unusual characters (numbers, hyphens) don't break the statement.
Surfaced during Ralf's end-to-end restore validation — yet another
"never been tested on a real PG install" latent bug exposed by the
Stage A inline-dump path actually being able to produce a restorable
manifest for the first time on his install.
Two changes that close the disaster-recovery loop the Stage A-B-C
backup-hardening plan opened:
1. Resolve 'local' source to backup_destination_path
The wizard passes options.source = 'local' (the SOURCE TYPE
string). The old code assigned that verbatim to localBackupPath
and every downstream path.join() ended up with junk like
'local/database/<file>.sql.gz'. Fixed by looking up
backup_destination_path from app_settings when source='local',
plus a layered candidate fallback in performDatabaseRestore so
absolute paths in manifests are honoured first.
2. Auto-rollback on ANY failure during restore
Previously rollback only fired when post-restore VERIFICATION
failed (inside the try block). Anything that threw earlier —
path bugs, pg_restore failure, file copy errors — left the
destination half-clobbered with no automatic recovery. Now the
catch block always invokes attemptRollback if a pre-restore
backup exists, and persists rollback status in
was_rollback_attempted + an enriched error_message so the admin
can tell at a glance whether the destination is safe to retry
on top of or needs manual inspection first.
Surfaced during Ralf's validation of the end-to-end backup +
restore cycle (`docker compose down -v` then restore from disk).
Every prior failed attempt left stray PDFs behind that the next
attempt had to navigate around — exactly the "every failure makes
the next worse" pattern this fix kills.
Two stacked bugs in the disaster-recovery path:
1. The wizard passes `options.source = 'local'` (the source TYPE
string) and the service assigned it verbatim to `localBackupPath`.
Every downstream `path.join(localBackupPath, ...)` ended up with
junk like `local/database/<file>.sql.gz` and `local/events/...`.
2. performDatabaseRestore reconstructed the dump path from the
manifest by basename-only:
path.join(backupPath, 'database', path.basename(dbBackupFile))
discarding the absolute path the manifest actually recorded.
Cure:
- At the entry point, if `options.source === 'local'`, look up
`backup_destination_path` from app_settings and use that as the
local root. Honour s3:// downloads via the existing branch.
- In performDatabaseRestore, try the manifest's absolute path
first, then `localRoot + manifest_value`, then the legacy
`localRoot + 'database' + basename` reconstruct as a final
fallback. First hit wins; error message lists every candidate
so future failures are diagnosable.
Surfaced during Ralf's end-to-end validation of the Stage A-B-C
backup-hardening plan — restored fresh after `down -v`, the wizard
failed silently with `Database backup file not found: local/database/...`
even though the dump existed at the path the manifest recorded.
With this fix, the same destruction-and-recovery sequence completes.
The Restore wizard's "Choose Backup to Restore" list was driven only
by the backup_runs table. After `docker compose down -v` (the disaster
this whole hardening effort is designed to recover from), the DB is
empty and the wizard shows "No backups found in selected source" —
exactly when it's needed most. The manifest JSONs are still on disk;
the wizard just can't see them.
Adds disk-first discovery:
- Walks backup_destination_path AND backup_manifest_path (manifests
can live in a sibling directory under the canonical
<root>/manifests/backup-manifest-<id>.json layout). Depth-limited
recursion (3 levels) so the scan doesn't enumerate the photo tree.
- Matches backup-manifest-*.json|yaml AND legacy bare manifest.json.
- Parses each manifest for real metadata (timestamp, size, file
count, database.backup_file presence) instead of showing the
admin opaque filenames.
- Layers in surviving backup_runs rows, deduping by manifest_id.
Applied to both GET /available-backups (legacy) and POST /list-backups
(the one the frontend actually calls). Same helper, two call sites.
Side benefit: each returned row now carries `databaseIncluded` — so a
future Restore UI iteration can show a "this backup has no DB dump"
warning before the admin picks a files-only backup. Exactly the
surface that would have caught Ralf's original four files-only
manifests if it had existed.
`BackupHistory.jsx` opened `/admin/backup/download/<id>` via window.open,
which goes to the React SPA's router — no matching route, so it
rendered the "Page Not Found" screen.
The actual download endpoint lives at `/api/admin/backup/download/:id`
on the backend (adminBackup.js:685). Cookie-based admin auth already
supports the implicit cookie sent by window.open, so the URL prefix
was the only thing missing.
Predates today's backup-hardening work — the bug has existed since
this download button shipped. Surfaced now because Ralf finally has a
completed backup to try downloading after the Stage A inline-dump
guard started working.
pg_dump rejects `--single-transaction` — it's a pg_restore / psql flag,
never a pg_dump one. Triggered as soon as the inline-dump path landed
on Ralf's install:
pg_dump: unrecognized option: single-transaction
pg_dump: hint: Try "pg_dump --help" for more information.
pg_dump already wraps the entire export in a single REPEATABLE READ
snapshot automatically (since Postgres 9.x), so the original intent —
consistent snapshot of the live DB — is preserved by removing the
flag. Same "latent until Stage A wired it in" pattern as the three
prior bugs this rollout has surfaced (PG insert destructure → bind-
mount EACCES → Node 22 stdio strict mode → this).
spawnToFile and spawnFromFile passed an unopened WriteStream/ReadStream
directly as a stdio entry to child_process.spawn. Older Node versions
auto-extracted .fd; Node 22 throws synchronously:
The argument 'stdio' is invalid.
Received WriteStream { fd: null, path: '/backup/database/...sql', ... }
Bug bit Ralf's install once today's `bugfix/crm-backup` image landed —
Node 22 came with that image, and Stage A's inline-dump path is the
first caller of spawnToFile on this install. Latent on the previous
image (Node 20); fatal on this one. restoreService's pre-restore
safety snapshot uses the same helper and would have hit it next time
a restore ran.
Cure: stdio: ['ignore', 'pipe', 'pipe'] (and ['pipe', 'pipe', 'pipe']
for spawnFromFile) + manual pipe of child.stdout/stdin through the
file stream. Works on every Node version. Also wires the WriteStream's
'error' event to the promise via settleReject so a future EACCES /
ENOSPC reaches the caller's try/catch instead of becoming a process-
fatal unhandled error event — closing the same "Stage A guard
bypassed" hole noted in the spawned follow-up task.
Side benefit: outStream.end() now awaits flush before resolving, so
fast pg_dump runs can no longer produce a truncated dump.
databaseBackupService.backup() did `const [runId] = await db(...).insert({...})`
without a .returning() — works on SQLite (knex returns [lastInsertId]) but
throws "(intermediate value) is not iterable" on Postgres (knex returns
a non-iterable shape).
Bug was latent until Stage A of the backup-hardening plan wired this
method into the "Run Backup Now" inline-dump path. Before Stage A only
the scheduled cron + the dedicated admin-DB-backup page called it, and
Ralf's install had never exercised either — so the inline-dump default
landing in production was the first time the destructure ran on his PG.
Cure: same explicit .returning('id') + dual-shape coalesce pattern that
backupService.js uses for its own backup_runs insert (line 949).
Two more sibling files have the same anti-pattern (userManagementService,
customerAccountsService — invitation flows) and will bite under the
same conditions; spawned a follow-up task to fix them in a separate PR.
Stage C of the three-stage backup-hardening plan (Stage A: inline
DB dump + fail-loud landed in 7fdf01a; Stage B: config-driven walker
in 302fc6b). Answers the "what would I lose if I clicked Run Backup
Now right now?" question that Stage B made possible to answer.
Backend:
- new backupCoverageService.js: per-path coverage classification,
drift detection (top-level subdirs not in backup_paths and not
in the backups/tmp allow-list), DB-dump mode + staleness block
- new GET /api/admin/system-health/backup-coverage route, same
auth + settings.view permission as /backup-integrity
- 7 integration scenarios pinning the classifier behaviour
Frontend:
- new BackupCoverageCard with auto-fetch (cheap; no recursion)
- new Coverage tab on BackupManagement next to Integrity
- en + de i18n; other locales fall back to en keys until a native
speaker reviews
Verification:
- 26/26 backup integration tests pass (Stage A 5 + Stage B 7 +
Stage C 7 + adminBackupIntegrity 4 + businessDocs 3)
- frontend build clean
- 4 pre-existing integration failures confirmed unrelated
Stage B of the three-stage backup-hardening plan (Stage A:
inline-DB-dump + fail-loud guard already landed). The file-backup
walker used to hard-code its subdirectory list inside
`getFilesToBackupInternal`, which is the same footgun that hid the
`business-docs` gap for ~6 months — a new feature drops artefacts
under STORAGE_PATH and the maintainer has to remember to edit the
walker.
Now driven by a `backup_paths` table:
- Migration 108 creates the table and seeds the 7 canonical
defaults (events/active, events/archived, thumbnails, previews,
heroes, uploads, business-docs). Seed data lives on the
migration as `DEFAULT_PATHS` so the boot self-heal can re-use it.
- `_backupPathsBoot.js` mirrors `_emailTemplateBoot.js`: on every
boot it diffs the canonical list against the current rows and
`INSERT ... ON CONFLICT DO NOTHING`s the missing ones. Keeps
admin edits intact, picks up new defaults shipped after the
install (Knex won't re-run migration 108). Wired into server.js
just before `startBackupService()`.
- Walker now calls `resolveBackupPaths(config)` which:
* reads `backup_paths WHERE include_in_default=true ORDER BY
display_order`
* falls back to a hard-coded `LEGACY_BACKUP_PATHS` if the
table is missing OR empty (defense in depth — never silently
scans nothing)
* gates each row by its `feature_flag` column (matches how
`backup_include_archived` already worked; data-driven now)
- Backward compatible: `getFilesToBackup(true|false)` still works
for legacy callers and the existing businessDocs test. New
callers should pass the full config object so feature gates
other than `backup_include_archived` evaluate correctly.
Tests:
- new: `backupService.configurableWalker.test.js` — 7 cases
covering canonical seed, toggling include_in_default, runtime
INSERT picked up without restart, feature_flag gating both on
and off, empty-table → LEGACY fallback, boolean backward compat
- all 15 backup-walker integration tests pass
(configurableWalker 7 + inlineDbDump 5 + businessDocs 3)
- frontend build clean
- 4 pre-existing integration failures (webhookDelivery, storage
backend, adminPhotos.reference, imageProcessor.storage) confirmed
unrelated via `git stash` baseline run
Stage C (CRM feature coverage audit + diagnostic UI) follows
in a separate commit.
The previous file-backup workflow only LOOKED UP an existing
database dump via getDatabaseBackupInfo() and silently shipped a
files-only manifest when none was found. Admins clicking "Run
Backup Now" (or relying on the schedule) got an apparent success
that omitted every customer / quote / invoice / contract / payment-
log row. The data-loss footgun was discovered 2026-05-29 when an
admin who'd been "backing up" for weeks via the UI lost the entire
CRM after a routine docker compose down -v — every produced
manifest had database: { backup_file: null, size: 0, tables: {} }.
New helper `ensureDatabaseDumpForBackup(config)` encapsulates:
1. Inline pg_dump (or SQLite copy) before the file scan, via
databaseBackupService.backup(). Result lands in
database_backup_runs and is picked up by the existing
getDatabaseBackupInfo lookup that writes the manifest.
2. Fail-loud guard: if no usable dump file is reachable (path
missing, 0 bytes, or never existed), throw — the existing
catch in runBackupInternal marks the backup_runs row failed
with the error_message and emails the admin if configured.
No more silent files-only manifests.
3. Opt-out: `backup_database_inline_dump = false` skips the
inline dump for admins who already run their own scheduled
`backup_database_schedule`. The fail-loud guard still
applies, so an opted-out install with no recent dump still
aborts loudly instead of producing a partial backup. Default
ON is encoded as "skip only when explicitly false" — undefined
(existing installs upgrading) falls through to the safe-
default ON branch.
The helper returns the verified `databaseInfo` so the manifest-build
step at runBackupInternal:917 reuses it instead of calling
getDatabaseBackupInfo a second time. S3/future destinations that
override `result.databaseInfo` are still respected (the existing
`result.databaseInfo ||` fallback shape stays put).
Test suite covers: default-on happy path, dump-throws-aborts-run,
opt-out + recent dump + proceeds, opt-out + no-dump + fail-loud,
opt-out + 0-byte dump + fail-loud. Mocks
databaseBackupService.backup so the tests don't depend on pg_dump
or sqlite3 CLI binaries being installed.
Stage A of three-stage backup hardening plan. Stage B (config-
driven walker) and Stage C (audit + diagnostic UI) follow in
separate commits.
The previous file-backup workflow only LOOKED UP an existing database
dump via getDatabaseBackupInfo() and silently shipped a files-only
manifest when none was found. Admins clicking "Run Backup Now" (or
relying on the schedule) got an apparent success that omitted every
customer / quote / invoice / contract / payment-log row. The
data-loss footgun was discovered 2026-05-29 when an admin who'd been
"backing up" for weeks via the UI lost the entire CRM after a routine
docker compose down -v — every produced manifest had database:
{ backup_file: null, size: 0, tables: {} }.
Changes to runBackupInternal:
1. Inline pg_dump (or SQLite copy) before the file scan, via
databaseBackupService.backup(). Result lands in
database_backup_runs and is picked up by the existing
getDatabaseBackupInfo lookup that writes the manifest.
2. Fail-loud guard after the dump step: if no usable dump file is
reachable (path missing, 0 bytes, or never existed), throw —
the existing catch block marks the backup_runs row failed with
the error_message and emails the admin if configured. No more
silent files-only manifests.
3. Opt-out: `backup_database_inline_dump = false` skips the inline
dump for admins who already run their own scheduled
`backup_database_schedule`. The fail-loud guard still applies,
so an opted-out install with no recent dump still aborts loudly
instead of producing a partial backup. Default ON is encoded
as "skip only when explicitly false" — undefined (existing
installs upgrading) falls through to the safe-default ON path.
Test suite covers: default-on happy path, dump-throws-aborts-run,
opt-out + recent dump + proceeds, opt-out + no-dump + fail-loud,
opt-out + 0-byte dump + fail-loud. Mocks
databaseBackupService.backup so the tests don't depend on pg_dump
or sqlite3 CLI binaries being installed.
Stage A of three-stage backup hardening plan. Stage B (config-driven
walker) and Stage C (audit + diagnostic UI) follow in separate
commits.
The fix shipped in 3ab3756 added /backup to the boot-time chown list.
That broke installs that don't bind-mount ./backup:/backup — the
single greedy `chown -R /a /b /c /backup` returned non-zero on any
individual failure, exiting the script and putting the backend into
a restart loop.
Reverting to the upstream-stable version. The original EACCES at
backup time is better fixed by admins pointing the backup destination
at a writable path via the admin UI (e.g. /app/storage/backups,
which the script already chowns) rather than baking a /backup
assumption into every install's boot path.
The docker-compose `./backup:/backup` mount was the only bind mount
not included in wait-for-db.sh's startup chown step. On a fresh
install (or any time the mount point is recreated), it stays
owned by root, and the nodejs (UID 1001) process running the
backup service gets EACCES when trying to mkdir under /backup.
Added /backup to both the chown list (root branch) and the
writable-check list (compose `user:` override branch), each guarded
by `[ -d /backup ]` so installs that don't use the bind mount —
native deployments, k8s with a different backup destination, etc. —
still boot cleanly.
Existing installs hit by this need a one-time host-side
sudo chown -R 1001:1001 <host-mount-for-/backup>
because the on-disk ownership won't fix itself; the script only
chowns at startup, and the directory was already created with
the wrong ownership by Docker's mount-point auto-creation. From
this commit onward, fresh installs are correct from the first
boot.
Line 205 of databaseBackup.test.js reassigned `fs.unlink` directly
(`fs.unlink = jest.fn(...)`), which permanently mutated the global
fs.promises module. Every test running after this in the same jest
worker process inherited the no-op stub, including
integration/storageBackend.test.js — whose LocalFsStorage.delete()
silently became a no-op, making the subsequent exists() assertion
flip from false to true.
Confirmed by adding a diagnostic patch to LocalFsStorage.delete:
post-await fsp.unlink, fs.existsSync(abs) returned true. unlink had
resolved without throwing but the file was still there → the unlink
was a mock.
Fix: jest.spyOn(fs, 'unlink').mockResolvedValue(undefined) + a
matching mockRestore() at the end of the test. Behaviour is
identical inside this test; the original fs.unlink is restored
when the test finishes, so subsequent tests get real fs.unlink
again.
Pre-existing issue — has been latent on upstream/beta forever.
Only surfaces consistently when CI load shifts jest's worker
allocation such that databaseBackup and storageBackend land in
the same worker process. This PR's extra integration test files
made that allocation deterministic locally and frequent enough on
CI to fail reliably.
The 5-minute session-sweep interval at sessionTimeout.js:17 fired at
module-load time without .unref(), so every jest worker that
transitively required this module (server.js → middleware → most
of the route layer) kept the event loop alive forever. The worker
then got force-killed on shutdown, surfacing as the longstanding
"worker failed to exit gracefully" warning at the end of every CI
run on upstream/beta.
Under enough I/O / memory pressure on a CI runner, the force-kill
could land MID-test rather than after the suite finished, taking
out whatever else was running on that worker — most visibly
integration/storageBackend.test.js on PR #555's runs.
.unref() makes the timer not keep the loop alive on its own.
Production behaviour is unchanged: the timer still fires every
5 min as long as anything else is holding the loop open (the HTTP
server, always).
CI's SQLite returned `[N]` (plain int) from `.insert().returning('id')`
while local SQLite returned `[{ id: N }]` (object form). The brittle
`const [{ id }] = ...` destructure crashed on the int shape. Switched
to the unwrap pattern used by the existing crmDb test harness so the
suite runs on both PG and every SQLite/knex combo the project supports.
Frontend half of the diagnostic shipped in 4812fcd. Adds:
- BackupIntegrityCard component — runs the check on demand, surfaces
the five summary counters (total / verifiedOk / existsButNoHash /
missing / hashMismatches), and expands collapsible result tables
for missing files + hash mismatches. existsButNoHash is exposed as
a separate amber-toned bucket so admins can distinguish hash-
verified evidence from existence-only at a glance — the latter is
explicitly weaker in a legal dispute and the UI says so.
- "Integrity" tab on BackupManagement, alongside the existing
Dashboard / Configuration / History / Restore tabs. Card is
portable — when the System Health page (backlog item) lands it
can lift the component without changes.
- Post-restore CTA on the RestoreWizard success card (D2 follow-
through): "Verify document integrity now" button that switches
the parent tab to Integrity. The audit trail captured at sign /
issue time is worth nothing if the documents it refers to are
missing from the restored copy — verifier surfaces that drift
in one click before the admin trusts the restored state.
i18n strings added in EN + DE (per user_languages — only those two
are native; other locales fall back to the English defaults and
should be flagged for native-speaker review per
feedback_translation_flagging if anyone picks them up).
Diagnostic for the bug fixed in a9280ea — confirms every *_path
column on quotes / contracts / invoices points at a file that
actually exists on disk and (where a *_sha256 column is set) the
file's bytes still hash to the expected value. Read-only;
on-demand only; no scheduler.
Per the design decisions locked in this PR's design call:
D1 — on-demand only for v1; scheduling deferred until we have
runtime data on large installs
D2 — not auto-triggered after restore; surface a "verify
integrity now" CTA on the restore-completed screen instead
D3 — wet-upload contracts hash-verified same as system-rendered
(signed_pdf_sha256 is computed at upload time, no special
case needed in the verifier)
Coverage (single source of truth in backupIntegrityService.CHECKS):
quotes.pdf_path existence
contracts.pdf_path + pdf_sha256 existence + hash
contracts.signed_pdf_path + signed_pdf_sha256 existence + hash
contracts.signed_customer_signature_path existence (PNG/JPG, no hash)
contracts.signed_admin_signature_path existence (PNG/JPG, no hash)
invoices.pdf_path existence
invoices.imported_pdf_path existence (admin-uploaded scans)
Report shape buckets each row into verifiedOk / missing /
hashMismatches / existsButNoHash so callers can distinguish hash-
verified from existence-only — the latter is weaker evidence in
a legal dispute and the UI should reflect that.
Route GET /api/admin/system-health/backup-integrity accepts an
optional ?scope= CSV filter (quote | contract | contract-signature
| invoice). Unknown scope tokens are rejected with a 400 +
BACKUP_INTEGRITY_UNKNOWN_SCOPE code rather than silently scanning
everything.
Frontend half (BackupIntegrityCard on a System Health page) is
deferred until backlog #11 (System Health page) is scaffolded.
The endpoint is independently useful via curl in the meantime.
backupService.getFilesToBackupInternal() enumerated a fixed list of
storage subdirectories (events/active, events/archived, thumbnails,
previews, heroes, uploads) and silently omitted the entire
business-docs/ tree. Every CRM PDF artefact and signature image fell
outside the in-app scheduled backup — restoring the DB without the
PDFs would have left every *_path column on quotes/contracts/invoices
as a broken FK and lost forensic evidence (the customer signature
PNG/JPG drawn on the public signing page is referenced by
contracts.signed_customer_signature_path; the rendered contract PDF
is referenced by signed_pdf_path with a stored signed_pdf_sha256
that would have nothing to verify against; wet-uploaded contracts
and admin-imported historical invoices are irrecoverable by design
since no renderer can reproduce them).
Single new scanDirectory call after the existing uploads scan,
covering:
- business-docs/quote/<year>/*.pdf
- business-docs/contract/<year>/*.pdf
- business-docs/contract/signatures/<contract_id>/*.{png,jpg}
- business-docs/invoice/<year>/*.pdf
- business-docs/invoice-imports/<year>/*.pdf
- and incidentally business-docs/dev-test/ (managed by adminDev.js,
bounded to 7 newest files, harmless to back up)
Verified that no migration is needed: hasFileChanged returns
!existing || checksum mismatch, so the first backup after this lands
flags every business-docs/** file as new and copies it. Restore path
in restoreService.performFilesRestore uses fs.mkdir({ recursive:
true }) on path.dirname(targetPath), so business-docs subdirectories
are recreated automatically from manifest entries — no restore-side
code change required.
Integration test pins the contract so a future refactor cannot
silently drop business-docs again.
The shell-script backup at scripts/backup.sh already covered all of
this via blanket `tar -czf storage`; only the in-app service was
affected.
Reviewer feedback on #555: nextQuoteNumber inside createQuote's
db.transaction was called without passing the outer trx, so
claimNextSequence opened its own connection — Postgres tolerated this
via the pool, SQLite (1-connection default) deadlocked on every quote
creation.
Audited the same pattern across invoiceService + contractService and
found five more matching call sites:
- createInvoice (single-row path after installment auto-route)
- spawnInstallmentInvoices (per-sibling claim inside the loop)
- createStorno
- createContract
- createFromQuote
All now thread trx through to nextXxxNumber → claimNextSequence so
the claim joins the caller's transaction on both engines.
convertToInvoiceOnly's Path B (standalone-contract) is the lone
remaining nextInvoiceNumber() call without trx — that path isn't
wrapped in a transaction at all (separate concern: sequence-number
leak on insert failure, tracked separately).
Previous fix (45f0606) papered over the bug by changing the German
wording from "innerhalb von {{minutes}} Minuten" to "bis {{at}}" —
that worked but changed the UX intent. The original German wording
("you have N minutes left") was deliberate and clearer than an
absolute clock time; the actual bug was that no caller ever computed
`minutes` from `responseLockedAt`.
Revert the DE translation to its original wording, then build a
{ at, minutes } object at the call site so EN ("until {{at}}") and
DE ("innerhalb von {{minutes}} Minuten") each pick up the variable
they need. `minutes` rounds UP so a 14m 32s remainder displays as
"15 Minuten" rather than promising 14 the customer can't actually
hit.
The German string used `{{minutes}}` while the call site at
QuoteResponsePage.tsx:300 passes `{ at: <localized time> }`, matching
the English string's `{{at}}`. Result on the public quote page when
the customer had already responded: the literal text "{{minutes}}"
rendered instead of the unlock time.
Switched the German wording to match the English semantics
("until X:XX") since the underlying value is an absolute time, not a
minutes-remaining count — the previous DE wording was also wrong about
WHAT the variable meant.
The CRM template seeders (crmEmailTemplates / contractEmailTemplates /
eventReminderTemplates) were idempotent and ready, but only
contractEmailTemplates was actually called (lazily, by contractService
sends). crmEmailTemplates had no caller anywhere — every install that
didn't pre-exist its templates failed every quote_sent / invoice_sent /
storno_issued / invoice_reminder_* send with "Email template '<key>'
not found". The queue processor retries 3 times then leaves the row
in status='pending', retry_count=3, silently dead with no admin
surface (see project_crm_backlog for the eventual System Health page).
Fix: wire all three seeders into server.js startServer() right before
startEmailQueueProcessor. The new _emailTemplateBoot.js orchestrates
all three and then, for any template_key it just inserted, resets
retry_count on stuck email_queue rows of that email_type so the
queue processor's next tick picks them back up. Recovery is targeted:
unrelated retry-exhausted rows (e.g. SMTP-timeout failures) are not
touched.
Integration test boots a fresh CRM DB, pre-seeds a stuck quote_sent
row plus an unrelated stuck row, runs the boot helper, and asserts:
templates landed, stuck quote_sent row was reset, unrelated row was
left alone.
Already-deployed installs heal automatically on the next backend
restart after this lands.
Wires customer_accounts.billing_email into the invoice, Storno, and
payment-reminder send paths. Previously the column existed on the
schema and the customer-detail page rendered an input for it, but no
send path read it — every outbound email landed on customer_accounts.email
regardless. That mismatch is the failure mode flagged in
feedback_data_driven_completeness: a UI field that promises behavior
the backend silently doesn't deliver.
Routing matrix:
- invoice / Storno / payment reminder
To: billing_email (fallback email when unset)
CC: email (when billing_email took the To slot) + per-doc cc_pdf_email
- quote / contract / event reminder / gallery share
To: email (unchanged — decision-maker address)
- payment-check / paid-notification
To: admin contact (unchanged — internal flow)
A new resolveBillingRecipients helper centralises the rules:
prefer billing_email, dedupe addresses case-insensitively, keep
per-doc cc_pdf_email as a supplemental CC. Lives in its own file
(_billingRecipients.js) to match the _renderContext.js convention.
Drops the isInt({ min: 0 }) constraint on lineItems.*.unitPriceMinor
in both the adminInvoices and adminQuotes POST/PUT validators so
admins can add Treuerabatt / Frühbucherrabatt rows as standalone
negative-priced lines (matches standard DE/CH invoice practice).
A service-layer guard rejects saves whose computed total goes below
zero (INVOICE_TOTAL_NEGATIVE / QUOTE_TOTAL_NEGATIVE, both 400) so a
mis-typed discount can't accidentally mint a credit-balance invoice
that would masquerade as a regular row in dashboards. Credit notes
still belong in the Storno path (createStorno), which is unchanged.
Quote-side integration coverage is omitted for now — createQuote's
cold-require path takes ~30s under the test harness; the invoice
test exercises the same validator + guard shape.
The pre-event reminder feature shipped with 5 seeded templates
(event_reminder_default + wedding/birthday/corporate/other) but the
CRM → Development "Send any CRM email to me" picker only listed the
quote/invoice/contract templates. Maintainer can now eyeball each
reminder category's body without staging a real event.
Backend:
- Extend TEMPLATES_KEYS in adminDev.js with all 5 reminder keys.
- Add event_date (today+2d), days_before (2), business_name (from
business_profile.legal_name) to the common payload so the
{{tokens}} in the reminder bodies resolve.
Frontend:
- Extend CrmEmailTemplateKey union.
- Add TEMPLATE_LABEL_KEYS entries.
- EN+DE i18n labels under crmDev.templates.label.event_reminder_*.
No PDF attachment — reminders are body-only emails (matches the
real flow).
The full reminder-email implementation (eventReminderService,
eventReminderTemplates self-heal, ReminderTemplatesPage,
EventReminderOverrideCard) shipped in the CRM bundle but the
FeaturesTab card kept lockedReason=NOT_YET_AVAILABLE — so the
working feature was invisible.
Flip the card to the same shape as customerPortal: status="beta",
real setFlag handler, no disabled/lockedReason. The sub-tab in
Settings → Reminder templates already self-mounts when the flag
is on, and the per-event override card already self-renders on
the event detail page.
Description copy + EN/DE i18n updated to describe what the feature
actually does (per-category pre-event nudge) instead of the old
"coming soon" placeholder.
Adds two pieces:
- __tests__/integration/helpers/crmDb.js — boots a temp-SQLite test
DB by invoking every migrations/core/*.up() directly. Bypasses
knex's Migrator because its exclusive write lock deadlocks
001_init's nested initializeDatabase() call. ~1 second cold start.
- __tests__/integration/crmSchema.test.js — 36 assertions on the
table + column layout after the consolidated CRM migration runs.
Pins:
- every CRM table present (quotes, contracts, invoices + the
eight supporting tables)
- deal_uuid columns on all three lineage tables (the column
DocumentLineageCard joins on — drop it anywhere and the card
silently returns partial data)
- back-pointer FKs (converted_contract_id, source_contract_id,
source_quote_id) — the exact columns that triggered the
Postgres FK-ordering bug fixed earlier in this PR
- Storno discriminator (kind, cancels_invoice_id, replaces_
invoice_id) per feedback_storno_filter_everywhere
- event time columns from migration 137
A full quote→contract→invoice lineage walk is deferred — quote
service's nextQuoteNumber() opens an inner transaction from inside
the createQuote outer transaction, which deadlocks SQLite's default
1-connection pool. Postgres dev DBs never see it. Either fix the
service to thread trx through, or run lineage tests against a real
Postgres in CI (mirror schema-drift.yml). Filed as separate work.
The suites already existed (538 backend tests, 40 frontend tests, with
solid CRM coverage on quoteService/contractService/invoiceService/
customerHoursService/eventService.calendar) but no CI workflow invoked
them. Wire both into a single Tests workflow that triggers on any push
or PR to main/beta.
Six backend suites are excluded — they fail on upstream/beta too
(supertest fixture + knex mock chain issues unrelated to CRM). The
explicit ignore pattern keeps the workflow green on day 1; each
excluded suite is listed inline as test-infra debt to fix individually.
Backend job pins SKIP_S3_TESTS=true (the same default the test setup
file applies) so the backup-service integration doesn't try a real S3
round-trip when no MinIO is provisioned.
Two upstream tests regressed because the CRM PR added expected behavior
they didn't anticipate:
- galleryOgService.shareImage.test.js: formatEventDate is now async and
routes through utils/dateFormatter so the OG card respects the admin's
general_date_format setting (per feedback_respect_general_format_settings).
That adds a third db('app_settings') call on every buildOgMetadata path.
Mock the formatter module directly — the format itself is irrelevant
to the cover-vs-logo contract this file pins.
- customerAccountsService.test.js: createInvitation now allows a duplicate
email when the existing row is PASSIVE (password_hash IS NULL) — that's
the "promote passive customer to portal" path. The active-customer
rejection mock now has to set password_hash so the guard fires.
Both are test-only changes; no service code touched.
quotes.converted_contract_id and invoices.source_contract_id were
declared with inline FKs to contracts(id), but contracts is created
later in the same migration. SQLite accepted the forward reference;
Postgres rejected it ("relation \"contracts\" does not exist"), which
broke the Schema drift (#530) workflow and any fresh Postgres install.
Same pattern as events.hero_photo_id → photos.id in db.js: declare the
column without a constraint, then add the FK in a separate alterTable
after both sides exist. Wrapped in try/catch so re-runs against a DB
that already has the constraint are a no-op.
Verified locally against the #530 recovery scenario (initializeDatabase
then migrate:safe) and the fresh-install path: both converge cleanly,
both FKs land on the expected tables.
Adds a top-level disclaimer section to README + a dedicated
docs/crm-disclaimers.md spelling out two areas where picpeak ships
defaults the operator MUST review before going live:
1. Contract blocks (image rights, NDA, model release, cancellation,
jurisdiction, …) — written by the maintainer, NOT by a lawyer.
Every operator must have their lawyer review and adapt them
before sending any contract to a customer.
2. QR-bills and SEPA EPC payloads — rendered from the data the
operator typed. Picpeak is open source; we recommend scanning a
test invoice with the operator's bank app to verify the QR
actually works.
Matches the on-screen amber disclaimers already shown on the
Contract Block Library page and the Business Profile payment-block
editor.
~940 new keys per primary locale covering every CRM surface:
quote / invoice / contract editor + list + detail + public response
pages, calendar, hours, tax report, deals lineage, reminder emails,
feature toggles, settings tabs, error toasts.
en.json + de.json are hand-translated by the maintainer and are
authoritative. fr / nl / pt / ru received the same key set but
machine-derived strings — flagged for native review in the PR
description per project policy (see memory feedback_translation_flagging).
3-way merge note: 1 conflict (fr.json) hand-resolved to keep
upstream's improved phrasing for previewLayout / livePreview /
heroPlaceholderText alongside feat/crm's pdfTypography keys.
Luca