f9073c515e3f2b997b1301a7b90263df0ef96d82
421
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  bermudalambandClaude Opus 5
|
b8549e9c72 |
feat: let a customer resend their own verification email (#110)
A verification email was sent once, at registration. If it was lost, filtered, or arrived after the 24-hour token had expired, the customer had no route back except registering again with a different address. POST /api/customers/resend-verification mints a fresh token and sends the mail, and the account page grows a "Send it again" button beside the warning that was already there. The button only exists while the address is unverified: on a verified account it would be a control whose only outcome is a refusal, and the endpoint refuses that case rather than sending a pointless email. The mint-token-and-send sequence now lives in one place. Registration and the email change already did the same three steps - supersede any outstanding token, mint a new one, send it - and this would have been a third copy. The step most likely to be dropped in a copy is the supersede, and it is the one that matters: without it an older message still sitting in the inbox goes on verifying. Anything that makes the server send mail on request is an abuse vector, so this is rate limited to three an hour, keyed on the customer id. That is tighter than either existing limiter and sidesteps #84's IPv6 problem entirely, since a signed-in caller has an identity better than an address to count against and cannot escape the bucket by moving within a delegated prefix. The refusal says the mail probably did send and to check the spam folder, which is both more useful and more honest than a bare 429. The claim that keying on customer id also solved test isolation was wrong, and the tests caught it. resetDb truncates with RESTART IDENTITY, so every integration test's first customer is id 1: three tests that each sent once left the fourth starting at its limit, and two tests failed on a 429 they never asked for. A "fresh customer per test" is a fresh row with a recycled identity. The limiter now has an explicit exported store the suite clears between tests, and the comment that claimed otherwise has been corrected rather than left to mislead the next reader. Verification: seven integration tests covering the unauthenticated refusal, the send, the new token invalidating the old one - asserted through the endpoint that would honour the stale link rather than by counting rows - the new link actually verifying, the already-verified refusal, the allowance stopping the fourth send rather than merely reporting it, and one customer's exhausted allowance leaving another's intact. Three end-to-end tests for the button, its confirmation and the message on the fourth click. The 33 integration tests across the three suites this touched all pass, as do the 199 backend unit tests. tsc clean on both sides, ESLint no errors. Closes #110 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
748c42628b |
Merge pull request 'feat: filter by Sold / Not sold / All on the storefront and the admin (#105)' (#129) from feature/105-sold-filter into main
Reviewed-on: #129 |
||
|
bermudalambandClaude Opus 5
|
32b3f616d9 |
feat: filter by Sold / Not sold / All on the storefront and the admin (#105)
Three decisions were taken before any code, and are recorded on the issue. The status filter is generalised to accept several values rather than gaining a second `sold` dimension beside it. "Not sold" is not a status: it is available-or-reserved on the storefront and includes pending in the admin, neither of which is one value. `?status=available,reserved` and `i.status = ANY($n::text[])` express that with one concept, so there is no way to write a contradiction like `?status=sold&sold=no`. A single status still parses to a list of one, which is how the admin's existing `?status=sold` keeps working untouched. The storefront now defaults to Not sold. That is a change in what every customer sees, not just a new control: the black SOLD ribbons leave the default view on a catalogue where they were evidence the shop sells things, and every storefront link shared so far quietly changes meaning. Accepted deliberately, with the default named in STOREFRONT_DEFAULT_STATUSES rather than implied by the absence of a parameter. The admin's four-way status dropdown is replaced rather than joined. That gives up isolating a single status: there is no longer a way to view only Reserved, or only Pending, and Not sold folds pending in with the rest. The pending workflow from #90 is the likeliest thing to miss it, and if it does, the fix is to put isolation back beside the preset rather than to remove the preset. The e2e test that covered "which is how Reserved is reached" is renamed and narrowed to what survives, rather than deleted. One thing the issue did not anticipate, found by a test rather than by reading. The favorites view deliberately showed sold favorites - "a favorite that has just sold is often exactly what the customer came to look at", and they have just been emailed to say so. Defaulting the storefront to Not sold reversed that silently and broke the test asserting it. Favorites therefore keep their own default of everything, on the server and in the control's displayed position, while an explicit ?status= still wins. That interaction is the kind a single-feature change quietly breaks, and it was caught only because the previous decision had been written down as an assertion. "All" still means different things in the two places, as the issue set out: available + reserved + sold on the storefront, all four in the admin. Pending remains unreachable from every public read - the storefront's unconditional exclusion clause is untouched - and the pending guard now checks every requested status rather than a single one, so `?status=available,pending` is refused for naming pending at all rather than accepted because the first name happened to be allowed. The control sits in the filter bar rather than in the drawer, since the default now hides sold pieces and a customer who never opens the drawer would otherwise have no way to know they exist. It is consequently excluded from the "Filters (N)" count, which describes the drawer, while still counting toward hasActiveFilters so that an empty result reads as "no items match these filters" with a way out rather than as an empty shop. Verification: 13 integration tests covering the default, each preset, the favorites exception and its override, and pending's unreachability under every accepted combination; 38 parser unit tests including multi-value parsing, an unknown name in a list being refused rather than dropped, and a list that names nothing; 6 new end-to-end tests for the storefront control, its URL round-trip, and the default staying out of the URL. 204 backend unit tests and 76 integration tests across the four affected suites pass. Two full end-to-end runs: 110 and 111 passing against the same 3 pre-existing failures, one run also showing a pending-publish failure that passes in isolation and did not recur - the cross-suite database contention filed as #116. tsc, ESLint and the production build are clean. Closes #105 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
e5ff980eae |
Merge pull request 'feat: tabs and a rendered preview for the email templates (#119)' (#128) from feature/119-email-template-tabs into main
Reviewed-on: #128 |
||
|
bermudalambandClaude Opus 5
|
1fa723bd19 |
feat: tabs and a rendered preview for the email templates (#119)
Follow-up to #92, which shipped the editable templates as a column of stacked cards. With six templates the cart reminder sat below five editors, so reaching it meant scrolling past all of them and which one you were editing was knowable only from a card title you had already scrolled past. They are tabs now, and the Default/Customised tag moves onto the tab label, so which templates have been changed is visible without opening each one. The larger gap was that there was no way to see what the email would look like. The editor is a markdown textarea; what gets sent is rendered HTML with placeholders substituted and, for the two favorite templates, a consent footer appended by the server. An admin editing copy could not tell whether the result read correctly. POST /api/admin/email-templates/:key/preview renders the draft in the editor rather than what is stored, so the effect of an edit is visible before committing to it. It renders on the server deliberately: renderTemplate is the only thing in the system that turns this markdown into HTML, and markdown-it is configured there with html: false, which is the control that stops an admin putting script into a customer's inbox. A renderer in the browser would be a second implementation of both, and a preview that disagreed with the mailer would be worse than none. It does not enforce required placeholders - saving refuses a body that dropped one, and previewing it is how the admin sees what they have done. The preview renders into a sandboxed iframe rather than through dangerouslySetInnerHTML. The markup is safe by construction, but an email is its own styling context: rendered inline, the admin theme's CSS would change how it looks and the preview would lie about the result. Sample values live beside the template definitions rather than in the route, so adding a placeholder puts the missing sample next to the change that needs it. A unit test asserts every available placeholder has one, because a missing sample renders a literal {{placeholder}} into the preview and teaches the admin their copy is broken when it is not. This also fixes a test that has been failing on main. email-templates.spec.ts located the Save button by filtering .ant-card for the template name, which matched an outer card containing every template's Save button - six of them - and died on a strict mode violation, taking two more tests with it as unrun. Only the active tab's editor is mounted now, so the labels are unambiguous and the filter is gone. Verification: eight end-to-end tests, four for editing and four for the preview, covering the draft being previewed rather than the stored copy, sample values replacing placeholders, raw HTML being escaped exactly as the mailer escapes it, and the consent footer appearing on a favorite template and not on a password reset. The full suite goes from 100 passed / 3 failed / 2 unrun to 112 passed / 2 failed / 0 unrun; the two that remain are the pre-existing password-reset failures that need a database on port 55432 and fail identically on main. 38 backend unit tests pass, tsc and ESLint are clean. Closes #119 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
f2ab4e6565 |
Merge pull request 'ci: fold the Tests workflow into SonarQube Analysis and delete it (#123)' (#127) from feature/123-consolidate-test-workflows into main
Reviewed-on: #127 |
||
|
bermudalambandClaude Opus 5
|
f7d85736dd |
ci: fold the Tests workflow into SonarQube Analysis and delete it (#123)
tests.yml and sonarqube.yml had identical triggers and ran the same tests. Where tests.yml ran test:unit:json and playwright test, sonarqube.yml ran test:unit:cov and test:e2e:cov: the same two suites, differing only in reporter and instrumentation. Every pull request therefore installed dependencies twice, migrated twice, built and started the backend twice, installed a Playwright browser twice and ran the whole end-to-end suite twice. With one runner the second copy did not run in parallel, it queued. Two things in tests.yml were not duplicates and are folded in rather than dropped. The summarize steps, which render pass and fail counts and name the failing tests, where sonarqube.yml offered only raw jest and Playwright output. And the continue-on-error plus fail-at-the-end pattern that lets those summaries render on a failing run at all: sonarqube.yml used to abort at the first failing step, so a broken unit test meant no end-to-end results, no coverage and no scan, which is one fact per run when a run costs many minutes. The trade-off is fast feedback. A broken unit test used to fail tests.yml in a couple of minutes; now it is reported when the whole pipeline finishes. That is worth it with one runner, where the fast job was queued behind the slow one anyway, and where a single run reporting unit results, end-to-end results, coverage and the scan together beats two runs each reporting a fragment. Worth revisiting if the runner count changes. Two details that would each have silently broken something: The coverage scripts do not emit the JSON the summarizers read - test:unit:cov has no --json, and test:e2e:cov does not set the JSON reporter. Both are appended at the step rather than baked into package.json, since coverage and a results file are only wanted together in this one place. The Playwright step uses --reporter=list,json so the log still names the failing test rather than only writing a file. The "Backend log" step was keyed on failure(). With continue-on-error the job is not in a failed state when it runs, so failure() would never fire and the log explaining an end-to-end failure would have gone unprinted exactly when it was wanted. It is now keyed on the step outcome. The header comment also records something the old comments obscured: the integration suite runs here on every push and pull request, despite backend-integration.yml describing it as manual. That quarantine only ever applied to tests.yml. Verification, run rather than assumed: both suites executed locally with the exact commands the workflow now uses. The unit run wrote unit-results.json alongside its coverage and summarize-jest.js rendered 199 passed from it. The end-to-end run wrote playwright-results.json and summarize-playwright.js rendered 100 passed, 3 failed, 2 skipped and named all three failures - a failing run, which is the case if: always() exists for. The three failures are pre-existing and fail identically on main. Parsing the remaining workflows confirms sonarqube.yml declares one job carrying both new step ids, lint.yml and backend-integration.yml are untouched, and tests.yml is gone. The two results files are added to .gitignore. CI never committed them, but the commands are now documented and runnable locally, which makes an accidental commit a matter of time. Closes #123 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
d82d0a8da3 |
Merge pull request 'feat(scripts): a PowerShell script to start the local environment (#125)' (#126) from feature/125-local-env-script into main
Reviewed-on: #126 |
||
|
bermudalamb
|
40b483fc30 |
feat(scripts): a PowerShell script to start the local environment (#125)
Bringing the app up locally was seven or eight commands in a particular order: a Postgres container on a port that is not blocked, the six environment variables the backend refuses to boot without, migrations, a TypeScript build, the backend, then Vite. None of it hard, all of it tedious, and documented nowhere outside CI workflows written for a Linux runner. scripts/start-local.ps1 does the sequence. It reuses an existing container rather than recreating one, skips npm install when node_modules is already there, leaves alone anything already listening on a port it wanted, and waits on pg_isready and a 200 from /api/config rather than sleeping a fixed number of seconds. -Fresh recreates the database, -Stop tears everything down by the process ids it recorded rather than by port, since killing by port would also kill whatever else happened to be listening. Two things this got wrong first time round, both found by running it rather than by reading it. $ErrorActionPreference = 'Stop' does not stop a PowerShell script when a native executable exits non-zero, only when a cmdlet throws. Every command here is node, npm or docker, so the first run printed a stack trace from a failed migration, carried straight on, and reported a healthy stack sitting on a database with no tables in it. That is the worst kind of wrong: a green summary over a broken environment. Native calls now go through Invoke-Checked, which tests $LASTEXITCODE and throws. The migration failed because node on the PATH was v18.16.1. node-pg-migrate pulls in an lru-cache that calls diagnostics_channel.tracingChannel, which does not exist before Node 20, and the failure surfaces as "(0 , U.tracingChannel) is not a function" from a minified file - which says nothing whatsoever about Node versions. The script now checks the major version first and says what to do about it, so the confusing crash becomes one clear line before anything else runs. The port default is 55500 rather than anything near 55432, which is reserved by Hyper-V on this machine. Docker's message when it cannot bind a reserved port does not mention reservations, so the failure path names the likely cause and prints the netsh command that lists the reserved ranges. Verification, all observed rather than assumed: the version guard was made to fire on Node 18 and produced the intended message. A -Fresh run on Node 24 applied all six migrations, and psql then showed thirteen tables where the broken run had none. /api/config and the storefront both answer 200. -Stop stopped both tracked processes and the container. A second run with the dev server already up detected it and left it alone rather than failing. .local/ holds the logs, pids and uploads, and is gitignored. Closes #125 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
46f88b4bbb |
Merge pull request 'feat(frontend): give order history a page of its own (#121)' (#124) from feature/121-orders-page-impl into main
Reviewed-on: #124 |
||
|
bermudalamb
|
9bb3cc86b6 |
feat(frontend): give order history a page of its own (#121)
The account modal had accumulated: a profile line, a name form, two collapsed panels for changing email and password, two consent switches, an order table and four controls. The table was the piece that fitted worst, being the only tabular data in a 700px dialog whose body is capped at 70vh. The scroll={{ x: 'max-content' }} already on it was a workaround for being in the wrong container rather than a layout choice.
It moves to /orders, an ordinary page in the same Routes block as /cart and /privacy, rather than another entry in MODAL_ROUTES. Order history is a list you read, like the cart, not a dialog you dismiss. A modal at /account/orders would have been the smaller change and was rejected: it inherits the same width and the same scroll cap, so it moves the table without giving it anything.
The page shell follows Cart.tsx, which is the established shape here: a Layout with a Header carrying Back to Shop and the title, and the same guard sending a signed-out visitor to /login. The account modal keeps a View order history button where the table used to be, because that is where a customer looks for it.
One thing changes rather than moves. The old effect caught a failed load with a toast and left orders as an empty array. The toast faded and the empty table did not, so from then on a customer whose request failed saw exactly what a customer with no orders saw, and the page asserted something false. Loading, failed and empty are now three distinct states, and the failed one carries a Retry: a transient failure would otherwise strand someone on a page that needs a full reload to recover.
OrdersBody sits at module level rather than nested inside Orders(). A function declared inside a component counts toward that component's cognitive complexity, which is what made Customers() hard to bring back under the threshold in #81.
The two assertions in account-modal.spec.ts that looked for the text "Order History" inside the modal are updated to look for the link, not deleted. They were the only coverage that the account view still offers any route to the orders, which is exactly what this change could have silently broken.
Verification, against a real backend and database: five new tests covering the signed-out redirect, the empty state, Back to Shop, the link from My Account, and that the page renders as a page rather than a modal over the storefront - that last one is what would catch /orders being added to MODAL_ROUTES and quietly undoing the change. The full suite goes from 100 to 105 passing with no new failures; the three that fail did so before this branch and fail identically on main. tsc and the production build are clean, ESLint reports no errors.
Closes #121
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalamb
|
6aa633d109 |
Merge pull request 'docs: design for moving Order History onto its own page (#121)' (#122) from feature/121-orders-page into main
Reviewed-on: #122 |
||
|
bermudalambandClaude Opus 5
|
d65eb7b981 |
docs: design for moving Order History onto its own page (#121)
Records why /orders is a page rather than another modal route. The app has both precedents: /account, /login and /register are in MODAL_ROUTES and render over a backdrop, while /cart and /privacy are ordinary pages. Order history is closer to the cart, a list you read rather than a dialog you dismiss. A modal at /account/orders was the smaller change and was rejected: it inherits the same 700px width and 70vh scroll cap, so it moves the table without giving it anything. Tabs inside the modal were rejected for the same reason, since they fix the scrolling and leave the cramping. The doc also records the one part of this that is a fix rather than a move. A failed load currently shows a toast and leaves the table empty, and the toast goes away while the empty table does not, so a customer whose request failed sees exactly what a customer with no orders sees. Loading, failed and empty become three distinct states, with a retry on the failed one. Per-order detail and server-side paging are written down as deliberately out of scope, so that leaving them out reads as a decision rather than an oversight. Refs #121 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
704aca0b84 |
Merge pull request 'feat(frontend): let a customer change their own name, password and email (#111)' (#120) from feature/111-account-ui into main
Reviewed-on: #120 |
||
|
bermudalamb
|
84db0e7ca2 |
feat(frontend): let a customer change their own name, password and email (#111)
The three endpoints have been on main since PR #113 with nothing calling them. This adds the UI, which is what the issue is actually about: its title is that PUT /api/customers/me has no caller. The name form sits open on the account view. Changing an email address or a password does not, because both are rare and deliberate, and leaving them expanded would push order history and the account controls below the fold for everyone who never uses them. They go in a collapse instead. Both of those carry a consequence the form cannot show. A new address has to be verified before it can be used to sign in or reset a password, and the old address is told that the change happened. A password change ends every other session. Each is stated above its fields rather than reported afterwards, so the surprise arrives while there is still a chance to back out. The email form asks for the current password. A live session is not enough to move the address a password reset would be sent to, which is the whole reason the server asks for it too. Server refusals are shown as they arrive rather than replaced with something generic: the message names which of the two passwords was wrong, or which name was left blank, and that is the only useful thing to say. The forms live in their own component rather than in Account.tsx. Three forms inline would have roughly doubled that component, and nested JSX bodies count toward the parent's cognitive complexity - the same thing that made Customers() hard to bring back under the threshold in #81. Verification, all against a real backend and database rather than mocks: six new end-to-end tests covering the name surviving a reload, a blank name being refused, the old password ceasing to work while the new one starts working, a wrong current password being refused for both the password and the email change, and an email change marking the account unverified again. The password test asserts the old credential no longer opens the account rather than that the form said something reassuring. The 21 existing account and auth tests still pass, and tsc and ESLint are clean. Closes #111 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
e234240440 |
Merge pull request 'ci: give linting its own workflow (#113)' (#114) from feature/113-split-lint-workflow into main
Reviewed-on: #114 |
||
|
bermudalambandClaude Opus 5
|
005146742e |
ci: give linting its own workflow (#113)
The lint job moves out of tests.yml into lint.yml, unchanged in what it runs. Lint is the fastest check in the pipeline and the one most often broken, and reporting it as one job among the suites made a lint failure and a test failure look alike at a glance. The split left two comments in the wrong place, both artifacts of the copy rather than of the intent. tests.yml kept the six-line note explaining the lint policy — that only defect-catching rules fail the build, that everything else warns, and why no --max-warnings flag appears. With the job gone it sat directly above backend-unit, where a reader would fairly take it as describing the unit tests. It has moved to lint.yml, with the job it actually describes. lint.yml inherited tests.yml's header about backend-integration living in its own manual workflow after it held the runner for three hours. That is worth saying where someone might expect integration tests to run; in a workflow that only runs ESLint it explains the absence of something nobody was looking for. Replaced with why this workflow exists at all. Verified by parsing both files rather than by reading them: lint.yml declares one job, lint; tests.yml declares backend-unit and frontend-e2e. No job was lost in the move and none is now declared twice, which is the failure a copy-and-delete edit invites. Refs #113 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
edf61abda4 |
Merge pull request 'feat(backend): let a customer change their own name, password and email (#111)' (#113) from feature/111-manage-account-details into main
SonarQube Analysis / sonarqube (push) Failing after 12m11s
Reviewed-on: #113 |
||
|
bermudalambandClaude Opus 5
|
7c3463650d |
feat(backend): let a customer change their own name, password and email (#111)
SonarQube Analysis / sonarqube (pull_request) Failing after 12m11s
Two of the three already existed on the backend and had no caller. PUT /api/customers/me updated the name; POST /api/customers/change-password already demanded the current password and enforced the eight-character minimum. Neither was reachable from the frontend, which is why the gap was easy to miss — the API looked finished. The name endpoint accepted empty values and wrote nulls, letting a customer clear fields registration refuses to let them skip. That is the same rule disagreeing with itself, so it now refuses each by name exactly as registration does. Changing a password now ends other sessions and keeps the one making the change. Reset already deleted every session for the customer, on the reasoning that a password is changed precisely when the old one may be known to someone else — change reached the opposite conclusion for no recorded reason, and a session opened with a leaked password outlived the change meant to lock it out. The current session is spared so the change does not eject the person making it. Changing the email address is new. It asks for the current password, because swapping the address a password reset goes to is how an account is taken over and a live session alone is not enough; that also matches what change-password already required. The address is normalised and validated, an address another account holds is refused with the same 409 as registration, and on success the row is marked unverified and any outstanding verification token superseded — one already sitting in the old inbox must not be able to verify the new address. Two emails then go out, to different places. Verification to the new address, and a notice to the old one naming what the address was changed to. The notice is the only thing that tells a real owner their account was taken, and one that does not say where the address went is nearly useless to someone checking whether it was them. Both sends happen after the row is written, never before, so a change that failed cannot produce mail saying it succeeded. That notice is a sixth template in #92's system, which cost a definition and a default body. The unit tests iterate every template, so its defaults were checked against its own required placeholder without writing a new test. Verified: 199 unit and 208 integration passing. The session test signs in on a second agent, changes the password on the first, and asserts the second is refused while the first still works — the property being claimed rather than the code path being executed. One of my own assertions was wrong on the way: /me answers an unauthenticated caller with 401 and an error body, not an empty one, and the frontend is what turns that into null. Refs #111 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
51c4f3f813 |
Merge pull request 'Feature/92 editable email templates' (#112) from feature/92-editable-email-templates into main
Reviewed-on: #112 |
||
|
bermudalambandClaude Opus 5
|
6baa769520 |
feat(frontend): edit the customer emails from Admin → Settings (#92)
SonarQube Analysis / sonarqube (pull_request) Failing after 13m7s
A card per email under the existing settings screen: subject, body, the placeholders it understands, and which of them it cannot lose. Each card starts from the copy that is actually in use — the stored version if there is one, the built-in default otherwise — rather than an empty box, so editing means changing words rather than writing the email from scratch. A badge distinguishes customised from default, which is why the API reports an unedited template as null rather than as its default text: the two are different states and the screen has to be able to tell them apart. Restore default is offered only when there is something to restore, so it is never a button that looks like it did something and did not. It removes the stored rows rather than writing the defaults into them, which is what keeps the badge honest afterwards. The server's refusal is shown verbatim. When a body drops a placeholder it needs, the message names which one, and that message is the entire value of the validation — replacing it with a generic failure would leave an admin guessing at which of five templates and which of three placeholders they broke. A textarea rather than the markdown editor already used for item descriptions. That editor is a heavy dependency to load into the settings screen for five short bodies, and its preview would render markdown as the browser shows it rather than as the email renderer will — a preview that quietly disagrees with the output is worse than none. Worth revisiting if the copy gets longer. Two things the end-to-end spec found rather than assumed. The refusal assertion first matched three elements, because the placeholder appears as the required marker, as an available tag, and inside the error — it now asserts the whole sentence. And the four tests raced each other: the suite runs fully parallel and they all edit one shared stored template, so one asserted a template was unset while another had just saved it. That describe block now runs serially, which is the honest fix for tests that mutate shared server state rather than making the assertions vaguer. Verified: 99 end-to-end tests passing on a freshly created container, up from 95, with the whole suite run rather than the new spec alone — precisely because these tests write state other suites read. Build clean, lint unchanged at 27 warnings. Refs #92 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
4ed9513ad2 |
feat(backend): make the five customer emails editable copy (#92)
Every customer email was a template literal in the route that sent it, so changing a word meant a code change, a review and a deploy. All five now render from markdown that an admin can edit: verification, password reset, favorite sold, favorite withdrawn, and the cart reminder. Five, not the four the issue counted — the favorite alerts have separate copy for sold and withdrawn.
markdown-it runs with html disabled, which is its default and the reason for choosing it over marked. Raw HTML in a stored body is escaped rather than passed through, so editing copy cannot put script into a customer's inbox. That is a stronger guarantee than sanitising output, because there is no output to sanitise.
Values are substituted into the markdown before it renders, which means a value that should become a list has to arrive as markdown. The cart reminder previously built li elements by hand; those would now be escaped and shown to the customer as literal angle brackets, so it emits a markdown list instead. The greeting is one placeholder rather than a bare name, so a template author writes {{greeting}} instead of "Hi {{firstName}}," — which reads as "Hi ," for anyone who registered before first names were required.
Saving is refused when a body has dropped a placeholder it needs, naming all of them rather than the first. This is the rule that separates a convenience from a way to break password resets from a settings screen: a reset email with no link still sends, still looks correct in the log, and is useless to everyone who receives it.
The favorite alerts' consent sentence is appended by the server and is not editable. It explains why the customer is receiving the mail, which is a compliance artifact rather than copy, and editing wording should not be able to delete it.
Unset templates fall back to the built-in defaults, so an install that never touches the settings screen behaves exactly as it did. The API reports an uncustomised template as null rather than as its default text, so "never edited" stays distinguishable from "edited to something identical", and DELETE restores the default by forgetting the row rather than writing the default into it.
Two problems surfaced during verification, both worth recording.
Five favorite-alert tests failed with no error and no mail. The cause was not this code: resetDb does not truncate admin_settings, so a subject of "Gone" stored by the new template tests survived into a later suite and changed the mail it was asserting on. Cleaning up inside the template tests would have fixed only that pairing, so resetDb now clears stored templates for every suite — template rows are test data like any other, and one outliving the suite that wrote it makes a failure appear somewhere unrelated.
The withdrawal notification then failed on timing. Loading copy from the database made the sender async, and the removal path was fire-and-forget, so the response could beat the mail out of the door. Dispatch was previously synchronous even though the sends themselves were not awaited; that is now restored by awaiting it.
Verified: 197 unit and 195 integration passing, lint unchanged at 4 warnings. The admin screen for editing these follows in the next commit.
Refs #92
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalamb
|
3b3888fd3c |
Merge pull request 'Feature/106 customer first last name' (#109) from feature/106-customer-first-last-name into main
Reviewed-on: #109 |
||
|
bermudalambandClaude Opus 5
|
b287c07747 |
feat: capture first and last name so emails can greet informally (#106)
Registration collected one optional Name, so every greeting had only a whole name to use: "Hi Thom Lamb," rather than "Hi Thom,". Both parts are now captured, and the cart reminder greets by first name. Both are required of anyone new, refused individually rather than as a single "name is required" so a form that filled one and not the other is told which. The columns are nullable even so, and that is deliberate. Marking them NOT NULL would mean backfilling legacy rows with empty strings, which asserts that every customer has a name — untrue of anyone who registered while the field was optional. The table records what is actually the case; the rule that new registrations must supply both lives in the route, where a missing field can produce a message naming it. The backfill splits on the first space, and it is lossy in a way no version of this avoids. "Thom Lamb" becomes Thom and Lamb; "Mary Jane Smith" gets a last name of "Jane Smith"; names that are not two parts fare worse. It was chosen over leaving the columns empty because nothing currently lets a customer correct their own name — PUT /api/customers/me exists but no frontend calls it — so empty would have meant permanently unpersonalised for every existing customer. The migration says so, so nobody later reads backfilled values as data the customer supplied in that shape. Verified against a seeded database rather than reasoned about, because this is the part that cannot be covered by the suite: migrations run in globalSetup before any test, and the old column is gone afterwards. Six representative rows through the real migration gave Thom/Lamb, Mary/"Jane Smith", Cher/null, " Padded Name " trimmed to Padded/Name, and null and whitespace-only names left as null on both. The down migration rejoins the parts and returns all six to their original strings. The old column is dropped rather than kept alongside, so there is one source of truth instead of two that drift. The admin keeps receiving a single composed display name. It only ever shows one — the list cell and the drawer title — and never edits one, so giving it both parts plus the joining logic would be work for no reader. Churn was the bulk of this: 14 backend registrations and 10 end-to-end registration forms. A first attempt at the backend fixtures also added names to login and password-reset payloads, which would still have passed since the server ignores unknown fields, but a login test implying login takes a name is a small lie; that was reverted and redone against register calls only. Verified: 172 unit, 183 integration and 95 end-to-end passing, lint unchanged at 4 backend and 27 frontend warnings. Not covered: the cart reminder itself, which runs from a cron and had no test before this either. The greeting change is a one-line substitution in that query. Refs #106 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
f76db6c8fe |
fix(test): make the compose guard survive a CRLF checkout (#107)
The guard added for #107 finds nothing on a checkout with CRLF line endings, which is every fresh clone on Windows. Splitting on a bare newline leaves a trailing carriage return, the end-of-line anchor in the entry pattern then cannot match, and all ten assertions in the file fail together. Worth being precise about why this shipped, because the process that was supposed to prevent it ran and did not. That guard was fired deliberately before committing: the UPLOADS_DIR line was removed, two tests failed, the line was restored, ten passed. What the exercise never varied was the file's line endings — and by then the working copy happened to be LF, because the backup-and-restore used to fire the guard had rewritten it that way. So the deliberate firing proved the guard catches a missing variable, on a file shaped exactly as the test run had shaped it, and proved nothing about the shape it meets in a clean clone. The failure mode is the one the file already worried about: parsing that matches nothing makes every other assertion vacuously true. Here it failed loudly instead only because the "parsed some entries at all" case exists — which is the case that turned a silent pass into a visible failure, and is the reason this was noticed at all rather than sitting green and checking nothing. Splitting on an optional carriage return fixes it. 172 unit tests pass on the CRLF checkout that was failing. Found while verifying #106, whose branch could not go green until this was fixed, which is why the fix lands there rather than on its own. Refs #107 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
0e9f2a3d82 |
Merge pull request 'Feature/107 compose required env' (#108) from feature/107-compose-required-env into main
Reviewed-on: #108 |
||
|
bermudalamb
|
a2500a9901 |
test(backend): fail the build when the compose file lacks a required variable (#107)
The one-line compose fix in the previous commit unblocks QA. This is the part that stops it happening again, and it is the more useful half. The failure was not really a missing variable. It was that nothing connected two files: envValidation.ts gained a required variable, docker-compose.qa.yml did not set it, and nothing noticed until a container refused to boot on deploy. CI passed the whole time, because CI supplies its own environment and never reads the compose file — which is exactly why "CI is green" was the wrong evidence to have offered. So a unit test now reads the compose file and asserts it sets everything the validator demands. It imports ALWAYS_REQUIRED rather than restating it, which is the only version of this test worth having: a copied list would pass forever while the next variable added to the validator went unguarded in precisely the same way. Two further assertions earn their place. UPLOADS_DIR is hardcoded rather than taken from a stack variable on the grounds that it must agree with the volume mapping, so the test checks it against the mount rather than leaving that a claim in a comment. And ADMIN_GATE_SECRET must be present as an interpolation rather than a literal, since a secret in the repository would defeat the point of having one. There is also a test guarding the test: a regex that matched nothing would make every other assertion in the file vacuously true, so one case asserts that parsing found entries at all. Fired deliberately rather than assumed. Removing the UPLOADS_DIR line reproduces the original failure as two failing tests; restoring it returns to ten passing. A guard that has only ever been observed passing is not known to guard anything. What this cannot do is check production, which runs from a Portainer stack outside this repository. That gap is now written into the README beside the validation rules, along with the reason a variable set only in Portainer's stack UI never reaches the container: stack variables are interpolated into the compose file, not handed to the service. 172 unit tests pass, lint unchanged at 4 warnings. Refs #107 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
0710cccac1 |
fix(qa): give the container the environment variables it now requires (#107)
QA refuses to start: "UPLOADS_DIR is required and is not set." #64 made UPLOADS_DIR always required, on the reasoning that its fallback of /app/uploads is correct inside the container and wrong everywhere else, so nothing should inherit it silently. That reasoning stands — but docker-compose.qa.yml had never set it either. QA was relying on exactly the fallback that change set out to stop people relying on, so the first deploy after it merged is the first one to fail. That is an incomplete check, and one that looked confident. #64 verified both CI workflows set every always-required variable and said so. CI is not what deploys. The compose file, which is, was never opened. ADMIN_GATE_SECRET was missing for a different reason, and the container reported it unset even after it was added to the Portainer stack. That is not a mistake, it is how compose works: stack variables are substituted into this file as ${VAR}, not handed to the container. A service receives exactly what its environment block lists. #87 already wrote that down; this is the first time it has bitten. The two get different treatment for a reason. UPLOADS_DIR is hardcoded because it is not a secret and because it has to match the right-hand side of the volume mapping — splitting one value across two places in the same file is how they drift apart. ADMIN_GATE_SECRET is interpolated from the stack so the secret itself never enters the repository, and the header comment now lists it among the required stack variables. Verified by feeding the environment docker compose config actually renders into validateEnv, the same function that was rejecting it: zero errors and zero warnings, the absence of warnings confirming the admin gate is now configured rather than merely quiet. Production runs from a stack outside this repository with the same history and will refuse to boot on its next rebuild unless UPLOADS_DIR is set there first. This commit does not fix that. Closes #107 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
afb3182ea7 |
Merge pull request 'feat(backend): accept only real images in the inventory upload (#95)' (#104) from feature/95-upload-type-validation into main
Reviewed-on: #104 |
||
|
bermudalambandClaude Opus 5
|
35b242a66f |
feat(backend): accept only real images in the inventory upload (#95)
The upload bounded size and count and nothing else: POST /api/admin/items would take a PDF, a zip or an executable and store it as an item image, under an extension copied from whatever the caller named their file. Those files are served by express.static from the application's own origin, so a stored .html came back as text/html and a .svg as image/svg+xml — both able to run script as the site. Three types are accepted: JPEG, PNG and WebP. SVG is excluded deliberately even though it is an image, because it executes script when navigated to directly, which is the exposure #103 describes; a photograph of a one-of-a-kind item is never a vector drawing, so nothing real is lost. GIF is excluded as simply not wanted for product stills. Validation happens twice, because once is not enough. The declared content type is checked in multer's fileFilter, before a byte is written — that catches picking a PDF by accident, which is most of what goes wrong. But file.mimetype is whatever the caller wrote in the multipart headers, so the bytes are checked too: each stored file's leading bytes must match the format it claimed. That is what stops evil.html renamed to photo.jpg and declared image/jpeg, which an allowlist on the declared type alone waves straight through. The byte check cannot live in fileFilter — that runs before multer has read the stream, so there is nothing to look at yet. It runs after the write instead, and a failure removes every file from the request rather than only the offending one: accepting the good half of a refused upload would leave files on the volume that nothing references. Handles are closed before anything is unlinked, because an open handle makes the unlink fail on Windows. The stored name now takes its extension from the validated type rather than from path.extname(file.originalname), so the name on disk cannot disagree with what the file is. The random UUID is unchanged — that was already right, and its comment explains why. The picker offers exactly those three types rather than image/*, so a choice the API will refuse is not on the menu in the first place. That is a convenience, not a control: the operating system's All files option remains, drag-and-drop ignores accept, and anything calling the API directly never sees it. The server is the control. Nine integration tests, and they are the first in this project to upload real file content — which is why none of this was noticed. They cover a genuine PNG accepted, a PDF refused, SVG refused, HTML wearing image/jpeg refused, nothing left on the volume after a refusal, a mixed request discarding its valid file too, and no item created when the upload fails. Plus 21 unit tests on the pure signature checks, including a RIFF container that is not WebP. Verified: 162 unit, 178 integration, 94 end-to-end on a fresh container. Backend lint holds at 4 warnings — it caught the now-unused path import, which is exactly what it is for. Refs #95 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
eda62cf354 |
Merge pull request 'refactor: standardise antd imports and remove the avoidable any (#65)' (#102) from feature/65-imports-and-any into main
Reviewed-on: #102 |
||
|
bermudalambandClaude Opus 5
|
a700597440 |
refactor: standardise antd imports and remove the avoidable any (#65)
Stage 1 of #65: this issue's original two lists. The type-checked gate it also owns follows in later stages. Nine files imported antd from the barrel while the rest of the codebase used deep imports from antd/es. Both resolve to the same modules under antd v5 and Vite, so this is not the tree-shaking problem it would have been under v4 — the cost was that a documented convention had two spellings, and nobody reading a file could tell whether its style was deliberate or just old. Eighty-two imports converted, and every antd/es path was checked to exist before generating any of them rather than trusting a name-mangling rule. The three `client: any` parameters in cartCheckout are now PoolClient. These functions run inside a transaction, and `any` removed exactly the check that would catch a pool-versus-client mix-up — which in this codebase means a query silently running outside the transaction it was meant to be part of, on the path that takes money. publicCustomer took `any` and now takes a CustomerRow describing what it actually reads. Typed as its own shape rather than the whole table so that adding a column later — a password hash, a token, an internal note — cannot quietly start being echoed back to a customer. The three `(window as any).paypal` casts are replaced by a declared interface for the injected SDK. It is deliberately narrow: it describes the three things this app calls, not the whole SDK, because a wider guess would be fiction and a wrong shape typed confidently is worse than an honest cast. The property is optional, since the SDK is absent until its script has loaded — which is the check both call sites already make. Verified: 141 unit, 169 integration and 94 end-to-end passing. The end-to-end run is the one that matters here — an antd import migration can build cleanly and still break at runtime through styles or context, so a green tsc proves less than it appears to. Lint drops from 8 warnings to 4 in the backend and 30 to 27 in the frontend, all of them the no-explicit-any this change removed. As a side effect the no-unsafe count that later stages exist to clear falls from 259 to 216 in the backend and 73 to 67 in the frontend, measured rather than estimated. Refs #65 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
567e8ba650 |
Merge pull request 'feat(backend): check the environment at boot instead of discovering it later (#64)' (#96) from feature/64-env-validation into main
Reviewed-on: #96 |
||
|
bermudalamb
|
9c9e9c3ded |
feat(backend): check the environment at boot instead of discovering it later (#64)
The backend reads environment variables in a couple of dozen places and validated none of them. A missing or misspelled one was undefined until the first line of code that happened to need it, which could be a long time after the container reported healthy — and several of those failures are silent and customer-visible. DEMO_MODE is the one that mattered most. It was read as "demo unless the value is exactly the string false", so DEMO_MODE=False, DEMO_MODE=0, or any typo meant demo mode stayed on and the shop quietly stopped charging anyone. It is now required and strict: exactly 'true' or 'false', and anything else refuses to start while quoting the value it was given, so the typo is visible in the message rather than inferred. Two requirements are conditional, and that is what makes them expressible at all. PayPal credentials are demanded only when DEMO_MODE=false, because QA runs with none of them on purpose and an unconditional rule would be simply wrong there. PUBLIC_URL is demanded only when SMTP is configured, because its only job is building links in email — an environment that cannot send mail does not need it, and requiring it everywhere would break every existing local setup to prevent nothing. UPLOADS_DIR gets no such reprieve: its fallback is correct inside the container and wrong everywhere else, so inheriting it writes uploads somewhere nobody is looking. Every problem is reported at once rather than one per restart, and the process then exits — the same shape as the container refusing to start on a failed migration rather than serving against a schema it does not match. Warnings are printed but do not stop anything: SMTP absent, the admin gate inactive, or an allowlist missing while mail can be sent. That last one is new and earns its place, since SMTP with no allowlist means the environment can reach real customers, which is what #87 exists to prevent. The admin-gate warning moved here from server.ts, so one place says what this container is and is not configured to do. validateEnv is a pure function of the environment handed to it rather than a reader of process.env, so it is tested exhaustively without booting anything or mutating global state. It is called from server.ts and deliberately not from app.ts: the integration suite imports app directly and would otherwise become a configuration exercise. Its rules are one small function each at module level, because cognitive complexity counts everything declared inside a function and the first version scored 24 against a limit of 15. Verified as a real process, not only in tests. A missing DEMO_MODE, a DEMO_MODE of 'False', real payments with no PayPal credentials, and half-configured SMTP each exit 1 with the problems listed; a valid environment starts and serves. Note the exit codes were checked without a pipe, because $? after `| head` reports head rather than node and had first suggested a clean exit. 141 unit, 169 integration and 94 end-to-end passing, lint unchanged at 0 errors and 8 warnings. Both CI workflows already set all six always-required variables plus DEMO_MODE, so the pipeline is unaffected. Refs #64 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
984d91f00a |
Merge pull request 'Feature/63 admin gate' (#94) from feature/63-admin-gate into main
Reviewed-on: #94 |
||
|
bermudalamb
|
89fc7c5c1b |
feat(backend): add an application-layer gate to the admin API (#63)
Authorization for the admin panel and the admin API has lived entirely in one auth_request regex in an Nginx Proxy Manager config outside this repository. That control is real and it works — nothing is publicly exposed today — but it is invisible from the code, untested here, and not reviewed when this code changes. Three things follow from that, and the first is the one worth the change. An admin route added at a path the regex does not match is unprotected the moment it is written, and nothing in Express indicates that. Anything reaching the published container port directly bypasses authentik entirely. And locally there is no gate at all, so no developer ever sees the boundary being enforced. requireAdminGate is attached to each admin router rather than to a path prefix, which is what makes it useful rather than merely redundant with the proxy. An admin router added later at some other path inherits the gate; because the proxy only injects the header on paths its regex matches, that router refuses on its first request instead of being quietly public. A 403 in that situation is the boundary reporting that it has drifted. The gate is optional, and unset means exactly today's behaviour. That keeps local development and all 113 existing admin test call sites working untouched, and means shipping the image before configuring the proxy cannot take the admin panel down. What it does not do is stay silent about it: the server warns at boot when the gate is inactive, naming what is unprotected. This project has been bitten repeatedly by controls that report success while doing nothing, and an unconfigured gate should be a visible choice rather than an invisible one. An empty value is treated as unset rather than as a secret, because enforcing an empty secret would admit any caller sending an empty header. Comparison is timing-safe over SHA-256 digests of both sides: timingSafeEqual throws on buffers of unequal length, so comparing raw values would turn a short header into a 500 rather than a 403, and a length check first would leak the secret's length. Turning it on requires the secret in two places at once — the stack environment and a proxy_set_header line on the gated location in NPM. Setting only one gives 403s until the other catches up. That coupling, and the three consequences above, are now written into the README beside the deployment section, since none of it is visible from the code. Verified over real HTTP as well as in tests. Booting without the secret logs the warning and serves admin normally; booting with it returns 403 for a missing header, 403 for a wrong one, 200 for the right one, and leaves the public storefront at 200 throughout, with each refusal logged distinguishably and without echoing the value it was sent. 8 new unit tests, 9 new integration tests covering every admin router separately — a correct middleware nobody mounted would pass the unit tests and protect nothing. 106 unit and 153 integration passing, lint 0 errors and 8 warnings unchanged. Refs #63 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
105bf141f5 |
Merge pull request 'feat: stage new items as pending until an admin publishes them (#90)' (#93) from feature/90-pending-status into main
Reviewed-on: #93 |
||
|
bermudalambandClaude Opus 5
|
ecc2219fa5 |
feat: stage new items as pending until an admin publishes them (#90)
An item used to be live on the storefront the instant it was created. Now it starts pending, and a customer sees it only once it is published. The migration changes the column default and nothing else. Backfilling would un-publish the entire live catalogue, which is the one thing it must not do. Hiding a pending item took four separate changes, not one, and that is the part worth knowing. The storefront's item routes had no status filter at all — sold items are listed and rendered with a Sold badge deliberately — so pending could not be expressed as one more optional filter. GET /api/items now carries an exclusion the caller cannot opt out of; GET /api/items/:id carries the same, because hiding an item from the list while still serving it by id would leave it reachable to anyone who kept a link; and GET /api/filters excludes pending from both aggregates it computes. That last one is the least obvious: a pending item would have inflated its tags' counts, so a customer would read "Rare (1)", filter by it, and be told nothing matches — and its price would have stretched the slider to a range no visible item occupies. The tag count is computed over the joined items rather than filtered with a WHERE. A WHERE would have dropped the row for a tag whose only item is pending, and the tag would have vanished from the drawer instead of showing zero. There is a test for exactly that, because the first version of this query had that bug. parseItemFilters is shared by the storefront and admin routes, so 'pending' parses on both. The public route refuses it explicitly rather than answering with an empty list, which would read as "no items match" instead of "you may not ask that". The storefront's URL reader is deliberately left not accepting it either, with a comment saying so, since a request guaranteed to fail is not worth constructing. Publishing is the existing mark-available: same transition, same UPDATE, so the admin UI labels that button "Publish" when the item is pending rather than adding a second endpoint that does the same thing. Unpublish is new and is not symmetrical — it is refused for a reserved item, which someone is holding in their cart right now, and for a sold one, which is a record of something that happened rather than a draft. Both refusals name their reason, and the buttons are hidden in those states so the refusal is not how you find out. Changing a column default has reach, and it surfaced eight test fixtures that silently depended on it. Each is now explicit about the status it wants rather than inheriting one — better practice regardless, and immune to the next default change. Two tests also used 'pending' as their example of an *unknown* status; both would have quietly become tautologies, so they now use one that is genuinely unknown. Verified: 98 unit, 160 integration and 94 end-to-end passing, the last on a freshly created container. One earlier run showed a single failure in favorites.spec.ts; it passes in isolation and on a clean container, and is the cross-spec interference already recorded against the suite rather than anything from this change. Refs #90 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
adf480faf6 |
Merge pull request 'feat(frontend): preview an inventory item as a customer sees it (#89)' (#91) from feature/89-item-preview-panel into main
Reviewed-on: #91 |
||
|
bermudalambandClaude Opus 5
|
03f08074d1 |
feat(frontend): preview an inventory item as a customer sees it (#89)
Clicking an item's name in the admin Inventory opens a drawer rendering the real storefront ItemCard for it, so the way a listing will look can be checked without publishing it and going to see. The name cell is a link-styled button rather than a clickable cell, so it stays reachable by keyboard and announces itself as an action. Admin already imports Item from ../api, the same type the storefront uses, so the row object goes straight into the card with no adapter and nothing to drift. The part that needed care is that ItemCard is not a passive component. It wires into the cart and favorites contexts and has working buttons, and both providers wrap the whole app — so a naive preview would have been fully functional, and an admin browsing inventory could have added their own stock to their own cart. On a one-of-a-kind catalogue that reserves the item and takes it off sale. ItemCard therefore takes an optional preview prop that short-circuits its two click handlers. Those two are the only entry points, so guarding them also covers the shared auth modal and the favorite-alerts consent prompt hanging off them. Deliberately not `disabled` on the buttons. A disabled antd button renders in a different colour with a different cursor and no hover, and the whole point of this panel is to show what a customer will actually see. The controls keep their normal appearance and their correct state for the item's status; only the handlers stop. The comment on the prop says so, because "simplifying" this to a disabled button would quietly defeat the feature while appearing to implement it. Four end-to-end tests, two of which are the ones worth having. Clicking Add to Cart in the preview must do nothing — asserted by the sign-in prompt never appearing, which a real click on a signed-out card always raises, so its absence proves the handler stopped before doing any work. And the storefront card must still be live where it is actually used, or this change would have quietly broken buying things. ItemCard's props are now Readonly, which was an existing lint warning on a file this change already touches: frontend warnings drop from 31 to 30. Verified: build clean, lint 0 errors, 91 end-to-end tests passing against a freshly created database. Refs #89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
e3d5475fa6 |
Merge pull request 'feat: let QA send real email, guarded by a recipient allowlist (#87)' (#88) from feature/87-qa-mail-allowlist into main
Reviewed-on: #88 |
||
|
bermudalambandClaude Opus 5
|
0c90e18205 |
feat: let QA send real email, guarded by a recipient allowlist (#87)
QA has never been able to send mail. The compose file set no SMTP variables and the mailer skips sending when it finds none, which was deliberate — a QA run must not be able to email a real customer if a fixture ever holds a real address. The cost is that four customer-facing flows have never been exercised anywhere but production: verification, password reset, favorite-sold alerts, and the cart-reminder cron that already has a known silent failure mode. MAIL_ALLOWLIST replaces the blanket mute. Unset means unrestricted, which is production and must stay so. Set means only matching recipients are delivered to; anything else is skipped with a [mail-blocked] warning naming the address and subject. An entry is either a full address, which also covers its plus-suffixed variants, or @domain for every mailbox there — plus-addressing is how these tests get written, and nobody should have to edit an allowlist to invent a new suffix mid-run. The guard sits in the mailer, not at the four call sites, so every sender is covered by construction and a fifth added later cannot bypass it by forgetting. It skips rather than throws: three callers already swallow send failures into a log, so throwing would mostly be caught anyway while risking a 500 on the signup path. The flow under test finishes and the log says why no mail arrived, which is exactly what was missing when QA was simply muted. Two details are load-bearing enough to state. Comparison is exact equality on both halves of the address rather than a suffix test, so a lookalike domain ending in an allowed one cannot get through — there is a test for that specifically. And a present-but-empty value refuses everyone rather than allowing everyone: writing MAIL_ALLOWLIST= expresses an intent to restrict, and reading it as "no restriction" would turn a typo into an outbound mail incident. This inverts the failure mode, so the allowlist is hardcoded in docker-compose.qa.yml rather than read from a stack variable. The safety property must not depend on remembering to set something in Portainer, where an omission would mean unrestricted sending from an environment full of fixtures. The comment says removing the line disables the restriction rather than the mail. QA points at Brevo, reusing the existing account rather than a separate QA sender — a deliberate choice that puts QA volume behind production's sending reputation and quota, acceptable for now. Host, port and secure are pinned in the compose because the mailer's fallbacks are Gmail's and Brevo needs 587 with STARTTLS; that mismatch fails at send time rather than at boot, which is #64's territory. Verified: 12 new unit tests on the matching function, which is where a mistake would actually be dangerous — 98 unit and 144 integration passing, lint 0 errors and 8 warnings unchanged, and the compose renders the expected values under docker compose config. Refs #87 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
d4d602da3e |
Merge pull request 'Feature/84 ipv6 rate limit key' (#86) from feature/84-ipv6-rate-limit-key into main
Reviewed-on: #86 |
||
|
bermudalamb
|
2f6e855596 |
fix(backend): stop IPv6 callers bypassing the password-reset rate limit (#84)
The QA stack has been logging ERR_ERL_KEY_GEN_IPV6 at every boot, and express-rate-limit was right to complain. keyByCallerAndEmail built its key from req.ip raw. For an IPv4 caller that is one address and the limiter worked as intended. For an IPv6 caller it is the full 128 bits — and a residential IPv6 customer is delegated an entire prefix and can source every request from a different address inside it at no cost. Keyed that way, each request counted as a new caller and the allowance of five per fifteen minutes never bound at all. That matters more here than it would elsewhere, because of what this limiter is for. Its own comment says it: without one, anyone can make the server send unlimited mail to any address they choose. For IPv6 clients there effectively was no limiter, while the code read as though there were. The caller half of the key now goes through express-rate-limit's ipKeyGenerator, which groups IPv6 by prefix and returns IPv4 unchanged. The helper's default is /56 rather than /64, and that default is kept deliberately: /56 covers a whole delegated site, so an attacker cannot escape their bucket by moving within their own allocation. It does mean several households behind one delegation share an allowance — acceptable only because the key also carries the email address, so they collide just when targeting the same account. The reasoning sits next to the code, because a future reader tightening it to /64 would silently reopen the hole. keyByCallerAndEmail is now exported so it can be tested directly. The limiter's allowance is still not asserted anywhere, and should not be: its store is process-wide, so a test that exhausts it leaks into every later test from the same address and fails something unrelated later. The key function is pure, and it is where the bug was. Verified by firing the guard rather than reasoning about it: building main and loading the module reproduces the ValidationError, and the same load with this change is silent. Seven new unit tests cover an IPv4 caller unchanged, two addresses in one delegation collapsing to a single key, separate delegations staying apart, an IPv4-mapped address keying the same as the plain IPv4 one, email normalisation, a non-string email, and a request with no address at all. 86 unit tests pass, 144 integration, lint 0 errors and 8 warnings — unchanged. Closes #84 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
cf45b7a8eb |
Merge pull request 'Feature/62 error boundary' (#85) from feature/62-error-boundary into main
Reviewed-on: #85 |
||
|
bermudalambandClaude Opus 5
|
71cbd142c3 |
fix: address the final review of the error boundaries (#62)
Five findings from a whole-branch review, two of which mattered. The catalogue boundary could not be recovered by the very controls it exists to keep alive. When the item grid threw, the header, filter chips and Clear filters stayed mounted — that was the point — but clicking Clear filters only changed the URL. A boundary does not reset on a client-side state change, so the fallback went on rendering over a catalogue that would by then have loaded perfectly well, and the only offered action reloaded the same failing URL. The shop read as permanently broken, which is the failure this whole change exists to prevent, reintroduced one level down. ErrorBoundary now takes an optional resetKey and clears itself when it changes; the catalogue boundary passes the filter key. The page and modal boundaries deliberately do not take one, because their escapes are hard navigations that remount the tree already — recorded on the prop so nobody completes the pattern by symmetry. The client-error endpoint could fill the disk. It is unauthenticated, each accepted report wrote about 8.7 KB, and Docker's default json-file driver has no size cap — so the rate limiter bounded a render loop, as its comment claims, but not a few hundred cheap source addresses. Stack and component stack now truncate at 1000 rather than 4000, which is still around fifteen frames and cuts the worst case to under 3 KB, and the QA compose file caps and rotates the log. Production is a Portainer stack outside this repository and needs the same option applied there; noted in the design doc rather than left implied. Three smaller things. A falsy thrown value defeated the boundary entirely: throw null is legal, and branching on the error object alone treated it as no error, re-rendered the children, threw again, and would have taken the root down — a blank page, the one outcome this is all here to avoid. The boundary now tracks hasError separately and synthesises a real Error for non-Error throws. The modal fallback announced its title twice to a screen reader, once as the dialog's name and once as the heading inside it, so the redundant dialog title is gone. And the design doc claimed the development-only detail shows the component stack when it only ever showed the message; corrected, with a note that the stack still reaches the server log, which is where it is useful. Verified after all five: backend lint 0 errors, 144 integration tests, frontend lint 0 errors and 31 warnings, 87 end-to-end tests, all against a freshly created database. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
557703f86d |
docs: mark the error-boundary design implemented (#62)
Records the two things the design got wrong. antd's Result renders its title as a plain div, so the design's Result usage and its getByRole('heading') assertions contradicted each other and the tests could never have passed as written — resolved by giving the title real heading semantics rather than by loosening the assertion, because an error page with no heading leaves a screen-reader user navigating by headings nothing to find. And import.meta.env had no ambient declaration anywhere in the app, so the DEV gate did not type-check until vite-env.d.ts was added.
The Vite error overlay risk the design flagged did not materialise.
Refs #62
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
f156cecd87 |
feat(frontend): mount error boundaries at the root, the item grid and the modals (#62)
Three mount points, so a render error costs the smallest part of the page it can. The catalogue boundary is the one that earns its keep. The likeliest throw in this app is a component rendering data from the API, and the item grid renders the most of it per page — contained there, the header, cart badge, filters and footer all survive, so a customer can still navigate instead of being handed one dead page. The modal boundary exists because the modal-route arrangement couples two independent trees. /account, /login and the rest render as modals over the storefront as a backdrop, so without a boundary between them a throw in Account blanks the storefront behind it and a throw in the storefront takes the open modal with it. One boundary separates them in both directions. Every escape action is a hard navigation rather than a Link. This is worth stating because the obvious implementation is wrong: a boundary does not reset when the route changes, so a Link would change the URL and go on rendering the fallback, which reads as the app being permanently broken. ErrorFallback changed too, outside this change's original scope and for a reason worth recording. antd's Result renders its title as a plain div with no heading semantics, so a page whose entire content is an error message offered a screen-reader user navigating by headings nothing at all to find. The title is now wrapped in Typography.Title. The tests assert a heading role and were right to; the component was what needed fixing, not the assertion. DevThrow throws on ?boom=<scope> and is mounted only behind import.meta.env.DEV, so Rollup drops it from a production build. Checked in both directions rather than trusted: the dev server serves it, and a production bundle greps to zero occurrences of its marker. A gate that is silently always-off looks identical to one that works. Verified: 87 end-to-end tests pass, 4 of them new — each boundary catches rather than blanking, the header survives a catalogue throw, the storefront survives a modal throw, and the report is observed reaching /api/client-errors on the wire rather than assumed. Build clean, lint 0 errors and 31 warnings. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
3e9d9a57c0 |
fix(frontend): make the error reporter genuinely unable to throw (#62)
The comment claimed the reporter could not throw and the code did not deliver it. JSON.stringify(report) and the call to fetch both run synchronously, as arguments, before the promise carrying the .catch exists — so a throw from either escaped straight out of componentDidCatch, where nothing remains to catch it. The boundary that exists to stop errors would itself have been the thing that crashed. Not merely theoretical: React does not guarantee the value handed to componentDidCatch is a real Error despite the parameter's type, because code can throw anything. An object whose message or stack is circular makes JSON.stringify throw. The body is now wrapped in try/catch for the synchronous part, and the existing .catch still covers rejection once the request is in flight. Neither covers the other, so both are kept, and the comment now says so rather than asserting a guarantee the code did not make. Verified: build clean, lint 0 errors and 31 warnings, unchanged. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
9599387f34 |
Merge pull request 'docs: design for React error boundaries (#62)' (#83) from feature/62-error-boundary into main
Reviewed-on: #83 |