9d424d0dbb88f6cddafae027b629aca42ecdc005
278
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Paul Nothaft
|
9d424d0dbb |
Merge pull request #596 from Luca-Timo/bugfix/crm-backup
Backup & Restore hardening — close the silent files-only data-loss class |
||
|
Luca
|
7988c18972 |
fix(restore): set was_successful=true on the completed update
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. |
||
|
Luca
|
20e3092c14 |
fix(restore): move operator-meta replay after post-restore verification (PR #596 round 3)
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.
|
||
|
Luca
|
354fbed182 |
fix(restore): coerce pg bigint counts to Number before comparing (PR #596 round 2)
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. |
||
|
Luca
|
3322a1d998 |
feat(restore): docker-logs visibility + ADMIN_CREDENTIALS.txt restore notice
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. |
||
|
Luca
|
a23fa3bb12 |
fix(restore): hoist preservedMeta above SQLite/PG split (PR #596 blocker)
`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.
|
||
|
Luca
|
07f9110674 |
chore(migrations): renumber 108_add_backup_paths to 109 to avoid upstream collision
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.
|
||
|
Luca
|
09f6a1af6a |
fix(backup-ui): respect general_date_format + general_time_format
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. |
||
|
Luca
|
83fdb47fbf |
feat(installer): install picpeak directly from a backup via trigger file
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.
|
||
|
Paul Nothaft
|
8c6525af01 |
test(v1/events): update mock chains to cover new app_settings probes
The #592 fix added a devtools-detection probe, and the #592 follow-up added a require_password probe + a branding-defaults whereIn().select(). Both shift the db() call indices the existing #550 test relied on, and the branding probe needed `.select()` to resolve to an array (the mock chain wasn't thenable, so `for..of` on the result threw → 500 on every test that hit BASE_BODY). Add `whereIn` + `selectResult` to buildChain so the branding probe yields an iterable. Factor the three pre-slug app_settings chains into a baseSettingsChains() helper and update each test's queued sequence and toHaveBeenNthCalledWith / toHaveBeenCalledTimes expectations to match the new shape. No behaviour change in v1/events.js — only the test scaffolding moves. |
||
|
Luca
|
e7dffa656b |
feat(backup-stats): per-Stage-B-path counters in backup statistics
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).
|
||
|
Luca
|
e0ace0864e |
fix(restore): preserve operator-meta settings across restore
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.
|
||
|
Paul Nothaft
|
791e9974eb |
fix(gallery): preserve per-viewer is_liked across hard refresh (#590 follow-up)
The in-session toggle fix in
|
||
|
Paul Nothaft
|
2d44b1ab2d |
fix(api/v1/events): also honour require_password + branding defaults (#592 follow-up)
Same class of bug as the devtools-detection gap landed in
|
||
|
Luca
|
989b42c2f1 |
fix(restore-wizard): warn on backups without a database dump
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.
|
||
|
Luca
|
155aa63103 |
fix(backup-dashboard): show last SUCCESSFUL backup + last attempt separately
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.
|
||
|
Luca
|
48e9c9c79a |
fix(restore): re-init knex pool after DROP/CREATE DATABASE
`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.
|
||
|
Paul Nothaft
|
2304b25624 |
fix(api/v1/events): honour global devtools-detection default on create (#592)
Same class of bug as #550 part 2 (feedback default ignored on API
events): the events table column default for enable_devtools_protection
is true, so an admin who disabled detection globally still got it ON
for every API-created gallery.
Mirror the feedback fallback that landed in
|
||
|
Luca
|
c435263744 |
fix(restore): default restore_allow_force=true + auto-upgrade existing installs
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.
|
||
|
Luca
|
dbcecfe2aa |
feat(restore): self-heal restore_allow_force default ON at boot
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. |
||
|
Luca
|
cfaa7eb095 |
fix(restore): re-sync PostgreSQL sequences after psql load
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.
|
||
|
Luca
|
a39def672e |
fix(restore): evict active sessions before dropping target DB
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.
|
||
|
Luca
|
4c31a22626 |
fix(restore): DROP/CREATE DATABASE needs explicit -d maintenance DB
`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. |
||
|
Luca
|
5c0be66a14 |
fix(restore): resolve local source + always rollback on failure
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. |
||
|
Luca
|
44c7935b84 |
fix(restore): resolve 'local' source to backup_destination_path
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.
|
||
|
Luca
|
f664fea60c |
fix(restore): discover backups from disk, not just the DB
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.
|
||
|
Luca
|
0ad14899fa |
ix(database-backup): drop bogus --single-transaction flag from pg_dump
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). |
||
|
Luca
|
d34036c4ef |
fix(safe-exec): Node 22-compatible stdio + error-bridge for spawnTo/FromFile
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.
|
||
|
Luca
|
f741e88acb |
fix(database-backup): Postgres-safe insert destructure (runs the inline 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.
|
||
|
Paul Nothaft
|
dfcebccee9 |
feat(admin/users): reactivate + delete actions for deactivated admin users
#574 follow-up — @blazmaric flagged that once an admin user is deactivated, the UI loses every affordance to manage that record. The deactivate button hides (rightly — they're already deactivated) but nothing replaces it, leaving the row stranded in the list with no path to either restore access or permanently remove it. ## Backend New on `userManagementService`: - **`activateAdminUser(id, activatedById)`** — symmetric to `deactivateAdminUser`. Flips `is_active` back to true, logs `admin_user_activated` activity. Idempotent: already-active target short-circuits without bumping `updated_at`. No "can't activate yourself" guard needed (actor is by definition already active). - **`deleteAdminUser(id, deletedById)`** — hard-deletes the row. Same self-action and last-super-admin guards as deactivate. Last-super-admin guard counts ACTIVE super admins excluding the target — so an already-deactivated super_admin can still be deleted when an active super_admin remains. FK ON DELETE rules in core migrations handle the cascade: SET NULL on `created_by_admin_id` everywhere (events, photos, quotes, invoices, contracts, customer_accounts, …); CASCADE on the user's own `api_tokens` + their pending admin / customer invitations. New routes on `adminUsers.js`: - `POST /api/admin/users/:id/activate` — `users.delete` permission (same tier as deactivate; reverting deactivation is the same scope of action as performing it). - `DELETE /api/admin/users/:id` — `users.delete`. ## Frontend `UserManagementPage.tsx`: - New mutation hooks: `activateUserMutation`, `deleteUserMutation`. - The row's action cell now branches on `user.isActive`: active users see Edit + Deactivate (unchanged); deactivated users see Edit + Reactivate (`UserCheck` icon, green hover) + Delete (`Trash2` icon, red hover). - The shared `ConfirmDialog` handles all four action types (deactivate / activate / delete / cancelInvitation) via per-type title / message / confirmText / variant lookup. `userManagement.service.ts`: - New `activateUser(id)` and `deleteUser(id)` methods mirroring the existing `deactivateUser` shape. i18n keys are added with English fallbacks via `t(key, fallback)` so the page works on every locale without a missing-translation warning. Native translations can be filled in via a follow-up. ## Test plan - [x] 8 new service tests pin: activate happy-path, idempotency on already-active, NotFoundError on missing target, activity log emitted, delete self-refusal, last-super-admin guard for both active and already-deactivated super_admin targets, hard-delete success, delete activity log. - [x] Frontend type-check clean. - [x] Frontend lint clean for the changed files. - [x] Backend lint clean. - [ ] Manual: deactivate a user → row now shows Reactivate + Delete → reactivate → user can log in again. Then deactivate again → delete → row vanishes, pending tokens for that user invalidated. Closes the UX gap blazmaric called out in https://github.com/the-luap/picpeak/pull/579#issuecomment-... . |
||
|
Luca
|
03e6617f38 |
feat(backup): coverage diagnostic — what will the next backup miss?
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
|
||
|
Luca
|
302fc6b937 |
feat(backup): config-driven walker via backup_paths table
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.
|
||
|
Paul Nothaft
|
433af15146 |
Merge pull request #582 from the-luap/feat/slovenian-locale-580
feat(i18n): add Slovenian (sl) language support |
||
|
Luca
|
7fdf01ad21 |
fix(backup): inline DB dump + fail-loud guard so "Run Backup Now" can't ship files-only
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.
|
||
|
Luca
|
7c230bdc24 |
fix(backup): inline DB dump + fail-loud guard so "Run Backup Now" can't ship files-only
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.
|
||
|
Paul Nothaft
|
37cc3631d8 |
feat(i18n): add Slovenian (sl) language support
Closes #580. Slovenian community contribution from @blazmaric (filed as an issue with attached files rather than as a PR — files inlined here unchanged except for the migration number). ## Changes - **`frontend/src/i18n/locales/sl.json`** — full Slovenian UI translations. Covers every top-level key present in `en.json` as of pre-CRM beta. The new CRM-module keys (`bills`, `businessProfile`, `calendar`, `contracts`, `crm`, `crmDev`, `crmSettings`, `dealLineage`, `eventReminderOverride`, `hoursLogging`) are not yet translated and will fall back to English — same posture as FR / NL / PT / RU / ES currently have for the CRM module (see PR #555 description). - **`frontend/src/components/common/LanguageSelector.tsx`** — adds `SLFlag` SVG component + registers `{ code: 'sl', name: 'Slovenščina', Flag: SLFlag }` in `SUPPORTED_LANGUAGES`. Frontend i18n auto-discovers locale files via `import.meta.glob` so no separate config registration is needed. - **`backend/migrations/core/108_seed_sl_email_template_translations.js`** — contribution-author's `107_*` filename renumbered to `108_` to avoid collision with `107_crm_consolidated.js` that landed on beta in the meantime. Idempotent insert via (template_id, language) uniqueness check — re-runnable, never overwrites admin edits. Covers 17 templates: admin invitation / password reset, archive complete, backup completed / failed, customer gallery assigned, customer invitation / password reset, database backup completed / failed, expiration warning, gallery created / expired, restore completed / failed, version update available / test. - **`backend/src/services/emailProcessor.js`** — adds `.si → sl` to the email-domain → language inference map, matching the pattern for every other supported locale. A customer with `@example.si` now gets Slovenian emails automatically without needing to set their preferred_language explicitly. ## Out of scope (consistent with existing locales) - CRM email templates (quote_sent, invoice_sent, contract_sent, etc., seeded at boot by `crmEmailTemplates.ensureCrmEmailTemplatesSeeded`) will fall back to English for Slovenian customers — those seeders only emit EN + DE rows today across every locale. - CRM UI strings under the missing top-level keys listed above will fall back to English. Both gaps mirror the existing FR / NL / PT / RU / ES situation. |
||
|
Paul Nothaft
|
975a815f99 |
Merge branch 'beta' into fix/email-normalization-574
Resolves a conflict with the CRM merge (#555) that landed on beta between when this branch was cut and now. Two conflict regions in backend/src/routes/adminCustomers.js: 1. **Require block** — both branches added new requires after customerAccountsService. Kept both: this branch's emailNormalization import AND beta's customerHoursService + invoiceService imports (the CRM merge added the hours-billing + invoice-creation paths to this router). 2. **Edit-customer validators** — both branches changed the same set of body() validators in the PUT /:id handler. This branch added the IDENTITY_PRESERVING_NORMALIZE_EMAIL options arg to normalizeEmail; beta changed every body() to optional({ nullable: true }) so passive-customer records that store nulls for missing profile fields don't reject on save. Kept both: the nullable pattern from beta + the email-normalization options from this branch. Preserved beta's explanatory comment about the nullable choice. Also patched one NEW normalizeEmail site the CRM merge introduced: - backend/src/routes/adminCustomers.js:231 — POST /admin/customers now exists (CRM-era customer-create endpoint). Same options arg applied. backend/src/routes/adminBusinessProfile.js has an isEmail() WITHOUT normalizeEmail() on the issuer email — intentional (no normalization means no risk of the Gmail dot-strip bug for that field), no change needed. All 18 normalizeEmail sites now pass IDENTITY_PRESERVING_NORMALIZE_EMAIL. 7/7 regression tests still pass. Lint clean on the merged file. |
||
|
Luca
|
3f5d006625 |
fix(test-infra): scope databaseBackup fs.unlink stub so it doesn't leak
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. |
||
|
Luca
|
ecb2aeacf9 |
fix(test-infra): unref sessionTimeout cleanup interval so workers exit gracefully
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). |
||
|
Luca
|
4812fcdec3 |
feat(backup): admin endpoint to verify CRM document-artefact integrity
Diagnostic for the bug fixed in
|
||
|
Luca
|
a9280ea9ba |
fix(backup): include storage/business-docs/ in the in-app backup walker
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.
|
||
|
Paul Nothaft
|
075b45f020 |
fix(email): preserve dots + subaddresses across all normalization sites (#574)
Closes #574. Reporter (@blazmaric) identified the root cause cleanly: express-validator's `.normalizeEmail()` applies provider-specific canonicalization by default — Gmail dot-stripping, +tag stripping, googlemail → gmail folding, etc. That's wrong for identity: PicPeak uses email as a login identifier, so `[email protected]` getting silently stored as `[email protected]` means the user can't log in with the address they were invited with. The bug existed at 17 call sites across the codebase (auth, admin user create/update, customer create/update, event create/update on three different routes, customer login, feedback submission). All of them are identity-bearing — none had a legitimate reason to strip dots for deduplication. Fix: introduce one shared options object in `utils/emailNormalization` disabling every provider-specific normalization (gmail_remove_dots, gmail_remove_subaddress, gmail_convert_googlemaildotcom, outlookdotcom_remove_subaddress, yahoo_remove_subaddress, icloud_remove_subaddress). The only default left enabled is `all_lowercase`, which is safe — local-parts are case-insensitive in practice on every major provider, and lowercasing keeps login lookup consistent. Every call site updated to pass the shared options. 7 unit tests pin the preserved-dots, preserved-subaddress, preserved-googlemail-domain, and still-lowercase behaviours so a future refactor can't silently regress. ## Migration note Existing accounts whose emails were already stripped before this fix remain with the stripped form in the DB. The fix takes effect for new invitations going forward. If an admin re-invites an existing user with the un-stripped address, that would create a duplicate account — out of scope here; if it becomes a real problem we can add a backward-compat login fallback (try lookup with dot-stripped form too) as a separate change. |
||
|
Paul Nothaft
|
48cf1121e5 |
Merge pull request #575 from the-luap/feat/clickable-version-links-566
feat(admin): clickable version links + update-available modal with changelog & upgrade command |
||
|
Paul Nothaft
|
832f7bad45 |
feat(admin): update-available modal with aggregated changelog + upgrade command (#567)
Closes #567. The sidebar already had a "vX.Y.Z available" indicator (#566 made it a link to that release's page) but there was no way to read the actual changelog inline or to grab a copy-paste upgrade command. This adds the modal the issue spec'd, layered on top of the existing updateCheckService / environmentService backend infrastructure that already shipped. ## Backend - `updateCheckService.fetchAvailableVersions` now returns full release objects (tag, name, body, publishedAt, htmlUrl) instead of just version strings — body data is what the changelog modal renders. `checkForUpdates` extracts the version strings for its existing consumers; no API change visible to callers. - New `getReleasesSince(currentVersion, channel)` returns the list of releases strictly newer than current, filtered to the user's channel. Reuses the same 1-hour cache as `checkForUpdates` so the modal opening doesn't trigger an extra GitHub round-trip. - New `GET /admin/system/updates/changelog` route in `adminSystem.js`, same auth + UPDATE_CHECK_ENABLED gating as the existing /updates and /updates/instructions endpoints. - 4 unit tests (axios mocked) pin: strictly-newer filtering, channel-scoped, empty array on GitHub fetch failure, empty array when already on latest. ## Frontend - New `UpdateAvailableModal.tsx` — opens from the sidebar chip. Two sections: 1. **How to upgrade** — fetches /updates/instructions for the environment-detected copy-paste command (Docker compose / git / standalone). Copy-to-clipboard button per step. 2. **Release notes** — fetches /updates/changelog for every version between current and latest in the user's channel. Latest is auto-expanded; older releases are collapsed by default (click to expand). Each release also has a "View on GitHub" link to the canonical release page. - Renders release body markdown through the existing safe MarkdownContent component (marked + DOMPurify allowlist). - New `updateDismissal.ts` helper — single localStorage key holds the last-dismissed version. Chip stays hidden until a STRICTLY newer version appears, using the same compare semantics as the backend (stable > beta, higher beta > lower beta, semantic numeric on major.minor.patch). 9 unit tests pin the rules. - `VersionInfo.tsx` — chip is now a button that opens the modal instead of an external link (the #566 link-to-release behaviour is preserved on the modal's per-release "View on GitHub" affordance). Dismissal triggers an immediate re-render so the chip disappears without waiting for the next route change. No new dependencies — uses `marked` + `DOMPurify` that were already present in the bundle for the contract block renderer. |
||
|
Luca
|
d1aecaa180 |
fix(crm): thread trx through sequence-claim sites to unblock SQLite
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). |
||
|
Luca
|
83933baeec |
fix(crm): self-heal missing CRM email templates at boot + recover queue
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. |
||
|
Luca
|
09c5110d2b |
feat(crm): route billing docs to billing_email when set
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.
|
||
|
Luca
|
3d37324080 |
feat(crm): allow negative line items for manual discount/Rabatt rows
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.
|
||
|
Luca
|
6d302e7998 |
feat(crm): add event_reminder_* templates to dev email tester
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).
|
||
|
Luca
|
b9cadf002c |
test(crm): update mocks for new createInvitation + OG date-format behavior
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.
|