9e0ac857850023238c573c4537a7e0b7d9bcbd98
8
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  bermudalambandClaude Opus 5
|
41840d4890 |
fix(email): stop a demo purchase telling real customers an item sold (#206)
A demo purchase called `notifyFavoritersOfSale`, which mails everyone who favorited the item through production's configured SMTP: "An item you favorited has been sold to another customer, so it is no longer available… this one will not be restocked." Nobody bought it and nobody is shipping anything, so both halves are false. It is also the only outbound consequence a demo purchase has — everything #195 and #203 fixed is on screen, in front of the person who clicked and who has now been told it is a demo. These recipients never saw the cart. They just get told something they cared about is gone, and while production runs the demo interim (#191) they are real customers on real SMTP. The demo route no longer notifies. The PayPal capture and webhook paths are untouched, because those are sales. The item is still marked `sold`, so the storefront stays truthful about availability and the favoriter who goes looking finds what the database says. Only the claim that somebody bought it goes away. That a demo purchase permanently consumes real production inventory is a larger question than this issue and is left alone. Removing the call broke two tests and quietly hollowed out three more, which is the more interesting half of this change. Five tests in `favorites.integration.test.ts` used the demo purchase as a convenient way to make a sale happen; with the notification gone, the two asserting mail *is* sent failed, and the three asserting it is *not* sent would have passed for the wrong reason for ever. They were always about who gets told rather than about the demo route, so they now call the notifier the way the PayPal routes do — after the purchase, with the sold ids and the buyer. `buyThenNotify` says so at the point of use. Route-level coverage is unaffected: the admin mark-sold path already had its own test, and the new test asserts the demo route notifies nobody. Both halves were mutation-tested rather than assumed. The new test fails without the fix. Dropping the buyer exclusion from `collectFavoriteRecipients` fails "does not tell the buyer their own purchase is unavailable" and "emails every opted-in favoriter except the buyer" — so the restored tests are guarding the logic again rather than passing on an empty inbox. Verified: 255 integration tests pass (the suite needs `--runInBand`; these share one database), 278 unit tests pass, backend build clean. Closes #206 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
f32913ef51 |
refactor: turn on noUncheckedIndexedAccess in both workspaces (#101)
Indexing an array now yields `T | undefined`, which is what it always did — the compiler simply says so. Enabled in backend/tsconfig.json and frontend/tsconfig.json, and in tsconfig.sonar.json alongside it so the drift guard stays satisfied. The sequencing this issue insisted on was right, and the numbers show why. Before #159 typed the query results, `rows[0]` was `any` and `any` indexes to `any`: the flag would have found close to nothing and the count would have changed completely afterwards. With the rows typed it finds 57 — 50 in the backend, 4 in the frontend, 3 in the Playwright suite — and they fall into three kinds. Thirty are `rows[0]` after a `rows.length` guard. TypeScript cannot connect the two, and rewriting them as `const [row] = rows; if (!row) …` makes the guard and the use the same check, which is better code independently of the flag. Ten are rows a statement guarantees — `INSERT … RETURNING`, or a lookup for an id the session middleware has already matched. These get `requireRow(rows, what)`, a new helper in db.ts that throws naming the query. A thrown error rather than a non-null assertion: if the assumption is ever wrong, an assertion hands `undefined` to the next line and fails somewhere unrelated, whereas this fails at the query and says which. asyncRoute turns it into a 500, which is the right answer for "the database did not do what the statement says it does". It also states the assumption once instead of ten times. The rest is ordinary indexing the compiler cannot prove: a regex capture group that the pattern guarantees, `split('+')[0]`, a modulo kept in range, `hasOwnProperty` failing to narrow an index signature, and Express typing route params as an index signature so `req.params.itemId` is `string | undefined` on a route that cannot match without it. One correction to this issue's premise, which matters for what it was expected to find. The body says "in a handful it does not guard at all", and the unguarded-500 risk it describes was not found. Every `rows[0]` either sits behind a length check or behind a statement that guarantees a row. What the flag actually bought was the ten places where that guarantee was real but unstated, and those now say so. Two changes worth calling out because they are not mechanical. TAG_COLORS is typed `[string, ...string[]]` in both copies rather than `as const` — the first attempt used `as const`, which narrowed the elements to literals and broke adminTags, so the annotation keeps `string` while telling the compiler index 0 exists. And the filter drawer's slider falls back to the bounds it was given rather than to null, because null there reads as "no filter" and would widen the results rather than leave them unchanged. Test files needed changes too, since ts-jest compiles them against the same config: a regex destructure in the compose guard, and ten `mock.calls[0][0]` reads where the surrounding assertions already establish the call happened. Verified: tsc clean over backend, frontend src and the Playwright suite; unit 254/254; integration 238/238; frontend build clean; lint unchanged in both workspaces. Closes #101 |
||
|
bermudalamb
|
72c49719fc |
refactor(backend): type the cart and checkout query results (#159)
The transaction paths, taken before the larger files because this is where `any` is most expensive: these are the queries that lock rows, move money and mark items sold, and where a mistyped field reaches a customer as a wrong price rather than a broken page. Typed query sites go from 6 to 22. Only queries whose rows are actually read carry a type. The INSERTs, UPDATEs, DELETEs and the BEGIN/COMMIT/ROLLBACK calls return nothing anyone looks at, and annotating them would be ceremony that makes the ones that matter harder to pick out. The convention is stated once in each file rather than implied, since #159's acceptance criteria say every call is typed "or explicitly exempted with a reason" and this is that reason. Two hand-written annotations are gone as a direct consequence. `items.reduce((sum: number, it: CartItem) => …)` and `checkoutItems.map((ci: { item_id: number }) => …)` existed only because `rows` was `any` and inference had nothing to work from. With the query typed, both infer, and the second one is the more interesting of the two: it was a structural type written inline that duplicated the real row shape and could have drifted from it silently. CART_ITEM_SELECT's type records something the SQL states and no reader would otherwise know: the images aggregate selects only id and image_path, so it is `Pick<ItemImage, 'id' | 'image_path'>[]` rather than `ItemImage[]`. Typing it as the full shape would have promised a sort_order that is not in the projection. The same hand-kept caveat as the item selects applies and is written into both files: `query<T>` asserts a shape rather than checking it, because TypeScript never reads the SQL. The integration suite is what catches a select and its type disagreeing. Verified: tsc clean, and the suites covering these paths pass — cart, favorites and adminInventory 53/53, then cart and soldFilter 19/19. Refs #159 |
||
|
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> |
||
|
bermudalambandClaude Opus 5
|
1d7aba2d60 |
fix(backend): route every async handler through the error middleware (#59)
Express 4 does not forward a rejected promise from an async handler, so an unwrapped async route never responds at all — the request hangs until the client gives up, nothing reaches the error middleware, and monitoring sees an open connection rather than a 500. That silence is the shape of the 2026-08-17 incident, where an unhandled rejection left every item query hanging and the storefront rendered it as an empty shop. `asyncRoute` was written in response, but it was only applied to some routes: 30 handlers added afterwards were still bare, including register, login, the whole cart, and PayPal checkout. Wraps all 30, plus two the issue's inventory missed. `attachCustomer` is a bare async middleware mounted globally in app.ts, so a rejection in its session lookup would hang every request in the application — including the 25 handlers that were already wrapped correctly, which meant the guarantee did not actually hold anywhere. The PayPal webhook registers on a second router named `webhookRouter`, so an audit grepping for `router.` walked straight past it. Adds a unit test that scans the route sources and fails on any registration whose handler is not wrapped. A convention already half-forgotten once will be forgotten again, and enforcement is what the issue asked for; ESLint would be the better home for it but there is no ESLint in this repo yet (#60). The test walks parens rather than lines, so it also catches a handler whose `async` sits on its own line, and it matches any `*Router` name rather than just `router` — the two ways the existing bare handlers escaped notice. It is deleted along with `asyncRoute` if the project moves to Express 5, which forwards rejections natively. No behaviour changes on the success path; the failure path turns a hung request into a logged 500. Closes #59 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
f626f27e75 |
feat: favorite items and notify when a favorite is sold (#34)
Customers can favorite and unfavorite items from the storefront, opt in to being told when a favorite is sold to someone else, and manage that preference from their account page. The opt-in is a consent of its own rather than the existing marketing flag. Being told that a specific item you asked about has gone is a narrower thing than agreeing to marketing, and folding one into the other would leave marketing_consent_text no longer describing what was actually agreed to. It is recorded the same way as the marketing consent — flag, timestamp, and the exact wording shown — and accepting it does not set marketing_consent. The prompt appears only after a customer has actually favorited something, so the reason for asking is concrete rather than an abstract marketing ask, and it says plainly that it is separate from marketing email. Declining keeps the favorite. Notifications fire when an item reaches sold, either through checkout or an admin marking it sold, and never to the buyer — telling someone the item they just bought is unavailable reads as a bug. Reserved is deliberately not a trigger: reservations expire and get released, so a "gone" email would often be about an item still for sale. Disabled accounts are excluded, per #33. completeCheckout now returns the sold item ids and the buyer so its three call sites can notify after COMMIT. Sending inside the transaction would email people about a sale that then rolled back, and would hold the transaction open for SMTP. Each message is sent independently so one bad address cannot stop the rest, and the sale has already succeeded regardless. Favoriting while signed out opens the existing inline register/login modal, exactly as Add to Cart does, and completes the favorite on success. Also fixes a latent bug in the same component: while the session was still resolving, `customer` is null for a signed-in visitor too, so clicking Add to Cart or the new heart in that window prompted them to sign in again. Both now ignore clicks until the session has resolved, and the control shows as loading meanwhile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
977489cbc0 |
refactor: delete superseded checkout routes and dedupe cart checkout
routes/paypal.ts and routes/demo.ts were the pre-cart single-item checkout flow. Nothing has imported them since the cart flow landed: app.ts mounts only cartCheckout, the frontend calls /api/checkout/cart/*, and no test touches them. They duplicated PAYPAL_BASE, getAccessToken, and a second handler for the /webhooks/paypal mount. Also extract openCheckout() from /paypal/create and /demo/purchase in cartCheckout.ts, which repeated the same address-ownership check, cart lock, and checkouts/checkout_items inserts. It returns a discriminated union so callers keep control of the transaction and the response. Add CartItem/LockedCart interfaces, dropping the (it: any) casts. Note: paypal.ts was the only writer of items.reserved_until and items.paypal_order_id. Those columns are now write-dead; the schema is left alone for a separate migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
9ab689e624 | feat: add customer cart with expiry, shipping addresses with USPS validation, and multi-item PayPal checkout |