895c08d1a741a822341f2cdb5eaa071668e616cf
87
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  bermudalambandClaude Opus 5
|
895c08d1a7 |
test(e2e): replace unchecked lookups, widen the assertion timeout, restore the skipped test (#241)
Tasks 2 to 4 of the plan. Nine `collection.find(...)` dereferences become findOrFail, so a missing row fails as a named assertion naming what was wanted and how many rows were searched, rather than "Cannot read properties of undefined" pointing at test plumbing. Where the old code followed the lookup with expect(x).toBeTruthy(), that assertion is dropped: findOrFail already guarantees it, and with a better message. The expect timeout goes from Playwright's default 5s to 10s. It costs nothing on a green run — it bounds how long a failing assertion waits, not how long a passing one takes — and #239 died reporting exactly Timeout: 5000ms on a runner that also builds, migrates and runs three other suites. The admin-save happy path comes back from the #245 skip. It is the only end-to-end check that adding an item reaches the database rather than merely firing a toast, and it passed both full parallel runs and in isolation. filters.spec.ts:216 is deliberately untouched: its .find() searches CSS class names on a string array, not test data, and has no missing-row failure mode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
bcecda9122 |
fix(intake): stop a throttled sender being told their link is dead (#222)
Adding e2e specs for the submission page found a defect in the page they were written for, which is what they were for. One limiter counted page loads and submissions against the same twenty-per-quarter-hour allowance, so a sender working through a box of stock ran out after ten items — the exact person the feature exists for, and the exact case the limiter's own comment said must not be refused. The comment said refusing them costs a consignment while the number quietly did it. Worse, the page could not tell a 429 from a 404. `fetchIntakeLink` treated any non-OK response as "no link", so a throttled sender was told "This link is not active" and sent to ask for a replacement — which could not have helped, because the problem was their address and a minute of patience. Two conditions needing opposite reactions were sharing a message. Now two limiters, because the two requests cost different things. Reading a link hits one indexed row and writes nothing, so that allowance is generous at 120: someone re-reading the form or losing their signal should never be told to wait. Submitting writes up to six files, so that is the one worth bounding, at 30 — more than anyone photographing items can manage and far less than a script would want. The page gains a third state. Unknown, revoked and used-up still collapse into one "not active" card, because whether a link exists is not something a stranger needs to learn. Throttled is deliberately kept apart from them, since "wait a moment" and "go and ask for another link" are opposite instructions. Measured rather than assumed, on a freshly started process both times: before, 25 page loads produced 14 rejections; after, 40 produce none. The first attempt at that measurement was wrong and worth recording — the restart had failed with EADDRINUSE, so it read 30 of 30 against the old process's already-exhausted store. The two specs now pass in a full parallel run alongside everything else. They are scoped the way #241 asks: unique run ids, assertions naming only this run's rows, nothing asserted about the table as a whole. Backend: 284 integration, 309 unit. Frontend: build clean, lint unchanged at 2 pre-existing warnings. Ref #222, #241 |
||
|
bermudalamb
|
9b4d7f2d03 |
feat(intake): manage upload links from the admin (#222)
An Upload links tab beside Tags: issue a named link, see how much of its allowance is spent, revoke it. Until now the only way to create one was curl, which is how the earlier tasks were exercised. The token is shown once, in an alert that says so plainly, because the server stores only a digest and genuinely cannot produce it again. A refresh loses it — that is the honest behaviour rather than a bug, so the copy says to revoke and reissue if it is lost instead of leaving somebody hunting for a reveal button. The cap field starts at 25 and unlimited is a checkbox rather than an empty field. Blank-means-unlimited would make the least deliberate action produce the least bounded link, and this screen sends all three cases explicitly so the server's default only ever has to cover callers that are not this screen. Two things came out of driving it in a browser rather than reading it. The revoke confirmation said "OK", and every other destructive confirm in this admin names its action — Delete, Disable, Re-enable — so it now says Revoke, in danger styling. A confirm button reading OK makes the reader go back and re-read the question to find out what they are agreeing to. And Popconfirm turned out to be a component nothing else here uses; the rest use Modal.confirm with an explicit okText. Keeping Popconfirm but matching its labelling to the established pattern seemed the smaller inconsistency, since the interaction is a row action rather than a page-level one. The load-on-mount effect carries the same eslint-disable and reasoning Tags and Categories already use, rather than a new shape. Verified in a browser: create shows the one-time reveal, the row lists as 0 of 25 and Active, revoke flips it to Revoked, and an explicitly unlimited link shows a bare count with no cap. The database then confirmed a default of 25, a null for the unlimited one, and a stamped revoked_at. Frontend: build clean, lint unchanged at 2 pre-existing warnings, 30 unit tests pass. Ref #222 |
||
|
bermudalamb
|
e7b01fdb36 |
feat(intake): add the public submission page (#222)
Where someone with no account sends in photos of one item. Route /submit/:token, outside the authentik gate by design: the token in the URL is the whole access control, which is what #222 chose deliberately over accounts. One state for every refusal, matching the server's single 404. Unknown, revoked and used-up links all render the same "this link is not active" card, because saying which kind of dead it was would tell a stranger whether a link they guessed at exists — the server is careful about that and the page must not undo it. `beforeUpload` returns false so antd keeps the files rather than uploading each one as it is picked. The submission is then a single request the server can accept or refuse as a unit, which is what makes the transaction on the other side meaningful. The accepted types and the six-file cap are stated here so the picker offers exactly what will be taken, but both are checked again server-side, because everything on this page is under the sender's control. The fetch effect guards against a late response from a previous token overwriting the current answer, which is reachable simply by editing the URL. TypeScript caught a real mistake rather than a stylistic one: `.filter((f): f is File => ...)` on antd's originFileObj does not narrow, because RcFile extends File and the predicate would widen rather than narrow. flatMap avoids the predicate entirely. Verified in a browser rather than by inspection: a throwaway Playwright run against the live stack confirmed the form renders for a good token, the inactive card renders for a bad one, and a photo can actually be sent and acknowledged. The database then showed the item at status pending with the default price, the draft carrying the note and its originating link, the image row written, the link's counter at one — and zero storefront-visible items, which is the property that matters most. Ref #222 |
||
|
bermudalamb
|
64324609e1 |
test(e2e): add a find-or-fail helper for collection lookups (#241)
Nine sites across five specs do `collection.find(...)` and dereference the result immediately. When the row is missing the test dies with "Cannot read properties of undefined" naming a line of test plumbing, which says nothing about what was expected — and that is exactly how favorites-filter:169 failed without producing a usable signal. The message names what was wanted and how many rows were searched. That distinction carries real diagnostic weight: "0 rows" means the fixture never landed, "37 rows" means it landed and the predicate is wrong, and those are different bugs to chase. It lives in its own module importing nothing, rather than in support/api.ts. That file imports @playwright/test, and vitest.config.ts runs tests/unit with environment: 'node' — putting six lines of pure logic there would drag a browser harness into the unit suite to test them. api.ts re-exports it so specs still reach it through fixtures. Throws rather than returning null, because every caller wants the row: an error at the point of the miss beats a null threaded through three more lines before something unrelated fails. Frontend: 30 unit tests pass, lint unchanged at 2 pre-existing warnings, build clean. Ref #241 |
||
|
bermudalamb
|
66c696ce40 |
test(e2e): skip the admin save happy-path test while #241 stands (#245)
`main` has been failing on one e2e test since the sold-filter fix landed, and it is a different test from the one #239 corrected: admin-save-failures' "saves an item successfully when the server accepts it". Skipped rather than fixed, deliberately. It fails in CI and passes locally, and which test fails moves around — a local parallel run of the whole suite on the same commit failed four *different* specs (admin-inventory-filters, auth, favorites-filter, resend-verification) and not this one. That is #241: fullyParallel against a single shared database. Fixing this test on its own would be guessing at a symptom that reappears somewhere else next run. Ruled out before disabling anything: the re-encoding from #226 is not involved. AdminInventory.addItem fills a name and a price and saves, attaching no files, so stripUploadedImages iterates an empty array and the image path is never entered. Checked rather than assumed, because this spec is on the admin save route and that is exactly where a regression of mine would surface. What this stops covering is not trivial, and the comment says so at the call site: it is the only end-to-end check that adding an item actually reaches the database rather than merely firing a toast. #245 exists so that it is un-skipped when #241 lands, rather than left behind. A skipped test on the core admin save path is worse than a red build, because a red build is at least visible. Ref #245, #241 |
||
|
bermudalamb
|
30227fb1e1 |
fix(test): correct the sold-filter tally assertion stranded by #188 (#239)
main has been red since 2026-08-25. Every SonarQube run reported 147 passed, 1 failed, and it was this test every time — expected "Filters", received "Filters (1)". The test is stale, not the code. It was last touched on 2026-08-23 in #137; the tally logic changed on 2026-08-25 in #188, which never touched the spec. #188 redefined the tally as the number of chips and moved the availability preset into the dimension system as a bar dimension — one that still emits a chip for any non-default status, deliberately, because without it `?status=reserved` is an empty grid with no Clear filters button and no way out but editing the URL. The test asserted the rule that held before that change. Counting bar chips differently from drawer chips would restore exactly the per-screen special-casing #188 removed, and the drift it fixed was the admin's tally disagreeing with the storefront's. So the assertion moves, not the tally. The replacement also checks the tally comes back down when the default is restored. The original only ever asserted one direction, which would pass against a count that incremented and never decremented — worth fixing while the test is open rather than leaving a second gap behind the first. There is a real wart left standing: `Filters (1)` opens a drawer with nothing selected in it, because the filter it is counting lives in the bar. That is a cost of #188's design rather than a defect in it, and the comment now says so rather than leaving the next reader to rediscover it. Verified by running the spec in isolation with a single worker: 6 passed. Closes #239 |
||
|
bermudalamb
|
44328d0b5c |
feat(admin): show the deployed commit and build time in the admin (#233)
There was no way to tell which build an environment was running. That is not hypothetical: minutes after #232 merged, `npm run backfill:images` in QA failed with `tsx: not found` because the container was still serving the pre-merge image, and the only thing that revealed it was npm echoing the old script line. Had the change been anywhere other than a package.json script, the container would have looked healthy while running the wrong code.
The header now reads something like `a5076cc · built 29 Aug 20:36`. The commit answers "is this the code I expect"; the build time answers "did my redeploy actually rebuild", which is a different question and the one that would have caught the case above.
The commit is read out of `.git` directly rather than by shelling out, because node:20-bookworm-slim has no git binary and adding an apt layer so the image can print seven characters is a poor trade. `.git` is copied into the build stage only — verified absent from the final image — so no repository history reaches a deployed container.
Resolution is pure and separately tested across every shape that actually occurs: a detached HEAD holding the object name, which is what a checkout of a ref produces; a symbolic HEAD followed to a loose ref file; the same followed to packed-refs, which is what a fresh clone commonly has; peeled `^` tag lines ignored so an annotated tag cannot yield the wrong commit; and every failure path returning `unknown`. That last part is the one that matters most — this runs during a Docker build, and a version stamp must never be the thing that stops a deploy.
Served from a gated /api/admin/version rather than folded into /api/config. That endpoint is public, and a commit hash there would tell any storefront visitor exactly which revision of a public repository is deployed. An integration test asserts the gate and asserts the public config does not carry it, because the boundary is the whole point rather than an implementation detail.
Verified in the built image rather than argued: the stamp inside it reads
|
||
|
bermudalambandClaude Opus 5
|
b897e99363 |
fix(cart): say what the demo button does rather than what the shop does (#203)
#195 added a notice reading "Demonstration only — This shop is not taking payments at the moment", gated on `demoMode` alone. That claim is false in a configuration the ops runbook actively steers towards. `demoMode` and `paypalClientId` are independent. `checkDemoMode` and `checkPayPal` only make the PayPal secrets *required* when `DEMO_MODE=false`; nothing forbids them while it is `true`. And `production-stack-cutover.md:65` says flipping to `false` without all three crash-loops the container — so the only safe order is to populate the secrets while demo mode is still on, verify, then flip. In that window `Cart.tsx` renders live PayPal buttons directly beneath a banner telling the customer the shop takes no payments, and it is precisely the window in which someone is clicking around production checking their work. That is the same failure #195 fixed, pointed the other way: silent where a warning was needed, then confidently wrong where a customer can actually be charged. Telling someone nothing will be shipped above a live PayPal button is worse than saying nothing. The notice now describes the button instead of the shop, which is true in both configurations and stays visible in the one with two controls that do different things — where a customer most needs to be told they differ. Gating it on `!paypalClientId` would also have removed the false claim, by hiding the notice exactly there, which is the worse trade. The test for it was also not testing it. "Says so before the customer commits" seeded an address with `isDefault: true`, and the cart auto-selects the default on load — so an address was already selected and the button already rendered when it asserted. It would have passed with the notice moved inside the `selectedAddressId` guard, which is the regression it exists to catch. It now seeds no address and asserts the notice is up while the checkout button is absent, which states the property directly. Mutation-tested rather than assumed: moving the Alert inside that guard fails the new test, and would not have failed the old one. Verified: 3 end-to-end tests pass against a browser, frontend build clean, lint 0 errors (2 pre-existing warnings in `src/filters.ts`). Closes #203 Refs #195 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
39c82ff3a4 |
fix(orders): mark a demo order in the history rather than leaving it to read as real (#205)
#195 and #203 made the cart say a demo order is a demo order. That message is an antd toast lasting about three seconds, after which the cart empties and the card unmounts. Order history is what the customer comes back to when they wonder where their item is, and it said nothing. A demo row was a real row: item name, `$80.00`, status `completed` rendered as a neutral tag because `STATUS_COLORS` has no `completed` key, and `demo` printed raw under a heading reading "Processor". That is not an explanation — a customer has no reason to read `demo` as "this did not happen", and "processor" is not a word they have any reason to know. Two things now say it, for the same reason the cart needed two. The `demo` cell renders as a tag reading "Demo (not charged)", which marks *which* order. A notice above the table, shown only when there is one, says what that means — a tag reading "Demo" still assumes the reader knows what a demo order is, and what they actually want to know is whether to expect a parcel. Nothing changes on the backend: `orders.processor = 'demo'` was already written at checkout and already selected for this page. The row is a real row in a real table and stays visible, because hiding it would be its own kind of lie — the customer did do something, and it did have an effect on the catalogue. Written test-first against a browser: the new case drives a real demo purchase through the cart, opens `/orders`, and failed on both assertions before the change. Verified: 4 end-to-end tests in this spec pass, the 5 existing order-history tests still pass, frontend build clean, lint 0 errors (2 pre-existing warnings in `src/filters.ts`). Closes #205 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
8572e62514 |
fix(cart): say it is a demo where the customer can see it (#195)
Production runs demo mode with no PayPal credentials — that combination is the whole reason #191 turned it on — and in exactly that configuration the storefront rendered a full-width primary button reading plainly `Checkout`. The `(Demo)` suffix was gated on a PayPal client id being present, so the one configuration that needs the word was the only one that never got it. The button is not decorative. It posts to `/demo/purchase`, which marks the item `sold`, writes a `completed` row into `orders` at the real price, and emails everyone who favorited it through production's real SMTP. Nobody is charged, which is what the compose banner promises and is true — but a customer cannot tell they have placed a pretend order, the inventory says otherwise, other customers are told it sold, and nobody is expecting to ship anything. #190, #191 and #192 all reason carefully about not charging by accident; none of them consider accepting an order by accident. Three things now say so, because one of them was never going to be enough: The label is unconditional. `type` still follows the PayPal client id — secondary when real PayPal buttons sit above it, primary when it is the only way to check out — and that distinction is worth keeping, but it is about prominence rather than about what the order is. A notice sits above it for the whole of demo mode, before an address is picked and whether or not PayPal is configured. A parenthesis on the control someone has already decided to press is the weakest possible moment to tell them. The confirmation stopped saying `Order complete!`, which is exactly what a real order says. It now names the two things a customer would otherwise assume: nothing was charged, and nothing will be shipped. Three end-to-end tests, written first and failing first against a browser — the label assertion failed with `Expected "Checkout (Demo)", Received "Checkout"` on an `ant-btn-primary ant-btn-block` element, which is the defect exactly as reported. The suite already runs `DEMO_MODE=true` with no PayPal credentials, so it reproduces production's configuration without any new fixture. `CartPage.checkoutButton` matches the label by prefix rather than in full, deliberately: a locator naming the correct label would have gone looking for the right button and found nothing, which is how a test for this can quietly pass by being wrong in the same direction as the bug. Verified: 3 new tests pass, frontend build clean, lint 0 errors, 25 unit tests pass, and the cart-countdown and orders suites still pass. The favorites and favorites-filter suites fail here, and fail identically with this change stashed — 9 failures without it, 8 with — so they are pre-existing and not from this. Worth their own issue. Closes #195 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
3242018782 |
fix(filters): address the final review findings (#188)
Restores the escape hatch for a status list matching no preset. filtersFromSearchParams accepts any non-empty subset of {available, reserved, sold}, and only three of those seven lists are presets; the other four reported as the not-sold fallback and produced no chip, so ?status=reserved was an empty grid reading "No items yet - check back soon" with no Clear filters button and no way out but editing the URL. hasActiveFilters covered all seven before this branch, so that was a regression against main. availabilityDimension now emits a chip for any status that is not the not-sold preset, labelled from SALE_STATE_LABELS when the list matches one and from statusLabel when it does not. The Segmented still reads "Not sold" beside such a chip, which is a cosmetic wart and the cheaper half of the trade.
Runs the frontend unit suite in CI. The 22 tests added over the dimensions were invoked by nothing: the workflow's only frontend steps were the build and the end-to-end run, and its test:unit:cov step is the backend's. The new step is guarded and named in the gate like every other suite, per the invariant workflowGate.test.ts asserts.
Restores the comment explaining why availabilityDimension's render and chips pass different fallbacks to saleStateFromStatuses. The control must read "All" while sold favorites are on screen; chips must stay silent because the customer never chose it. Unifying them would give every signed-in favorites view a phantom All chip and a tally of 2, and nothing said so after the old markup was deleted.
Derives the storefront's "is anything filtered" and FilterBar's tally through one exported chipsFor rather than two expressions over two identically-built contexts. They agreed only by convention, which is the exact defect this branch exists to remove.
Documents that statusDimension and availabilityDimension are alternatives over one field, since composing both type-checks and would render two controls that double-count it, and asserts the price chip's label text, which was the only chip label nothing checked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalamb
|
8771deeaca | test(filters): cover the tally and availability chip behaviour changes (#188) | ||
|
bermudalamb
|
147e280f88 |
refactor(filters): compose the storefront from dimensions and delete the old components (#188)
Replaces App.tsx's hand-written Segmented/Filters-button/ActiveFilterChips region with a single FilterBar composed from five dimensions, and removes the FilterDrawer and ActiveFilterChips components along with the activeFilterCount and hasActiveFilters helpers they were the only callers of. Catalogue now receives a filtered boolean computed the same way FilterBar computes its own chip tally, rather than the ItemFilters object it only ever used for that one check. |
||
|
bermudalamb
|
2f0da9afe1 | refactor(admin): compose the inventory filters from dimensions (#188) | ||
|
bermudalamb
|
50d048a1d6 | feat(filters): add FilterBar, one component both screens compose (#188) | ||
|
bermudalamb
|
918d6eeab9 | refactor(filters): extract the chip row from ActiveFilterChips (#188) | ||
|
bermudalamb
|
1d7f928c48 | feat(filters): move the availability preset into a bar dimension (#188) | ||
|
bermudalamb
|
eb5799dd17 | feat(filters): add favorites and status dimensions (#188) | ||
|
bermudalamb
|
abd3dd4e2e | feat(filters): add tag and price dimensions (#188) | ||
|
bermudalamb
|
5d0b66a982 | test(filters): add a unit runner and the filter dimension contract (#188) | ||
|
bermudalamb
|
d6e0942487 |
feat(filters): show a tag's own colour on its active filter chip (#185)
A tag carries a colour, and every place a tag appears shows it — a product card, the filter drawer's control, the admin taxonomy screen — except the removable chips beside the Filters button, which rendered every filter as a default grey. Picking `vintage` from a control that showed it in red produced a grey chip of the same name right next to it.
Only tags get a colour, because only tags have one. Category, price, favorites and status keep the default, and that asymmetry is the point: in a row mixing four kinds of filter, colour now means "this is a tag". Nothing depends on it — every chip still carries its label — so this reads the same to anyone who cannot distinguish the colours.
The close control inherits the tag's text colour, so a coloured chip gets a matching cross rather than a grey one on a coloured ground. A tag not yet in the loaded options has no colour to use and keeps the default, which is the same window the existing `Tag {id}` label fallback covers.
The test asserts the chip's colour equals the same tag's colour on a product card, rather than asserting it is red. The colour is derived from the tag's name and free to change; what must hold is that a tag looks like itself wherever it appears, and comparing the two places says that directly. Confirmed to fail without the change — the old grey chip sets no colour class at all.
Verified visually as well as by assertion: three tags selected together render in the drawer, in the chip row and on the card in the same colours.
Closes #185
|
||
|
bermudalamb
|
61c12fd438 |
refactor: remove the duplicated blocks SonarQube found (#182)
Three of the four candidates were real. The fourth was my mistake in the issue. **The category tree adapter**, duplicated verbatim between `CategoryTreeSelect.tsx` and `FilterDrawer.tsx`. This one was mine: #139 moved the storefront filter to a `TreeSelect` and copied the admin's adapter rather than sharing it, with a comment saying the shape "matches the admin's CategoryTreeSelect so the two stay comparable" — an argument for one implementation that instead produced two. It now lives in `filters.ts` beside `buildCategoryTree`, which was already shared for exactly the same reason: one meaning, one implementation. `Categories.tsx` keeps its own. It builds a different shape for a real antd `Tree`, keyed rather than valued, with a title that is a React node carrying that screen's buttons. Genuinely different, and folding it in would mean a parameterised adapter that serves neither case clearly. **The `item_images` insert loop**, written separately by create and update and differing only in where the id came from and where the sort order started. Both are parameters now, which also means the `/uploads/` prefix is written once — #103 made that the value `uploadUrl` joins an origin onto, so it is a contract rather than a string. Extracting it turned up two things the inline versions hid. Create indexed `files[i]?.filename ?? ''`, so a missing element would have stored a path pointing at the uploads directory itself; iterating by entry removes the possibility rather than defending against it. And the helper's typed `itemId` surfaced that `req.params.id` is `string | undefined` under `noUncheckedIndexedAccess`, which the old inline `unknown[]` swallowed — now `Number()`, as the `setItemTags` call two lines above already did. **The optional-field guards**, eight identical lines opening both routes. The distinction worth preserving is that `undefined` means "not submitted", which update reads as "leave as-is", so an unparseable value has to be told apart from an absent one. That is what makes it more than a null check and worth stating once. **`TAG_COLORS` was not a duplication.** The issue listed four files on the strength of a grep that also matched `STATUS_TAG_COLORS` in `Admin.tsx` — a status-to-colour map for the inventory table, unrelated to the tag palette. What remains is one definition in `backend/src/utils.ts` and one mirror in `frontend/src/admin/Tags.tsx`, already carrying a comment pointing at the other, which is the same treatment `ALLOWED_IMAGE_TYPES` gets and is correct: there is no shared package, and creating one for a colour list would cost more than it saves. Verified beyond the type checker, since three of these are pure moves that compile either way: 278 unit and 254 integration tests, and the end-to-end specs covering both consumers of the shared adapter — the storefront drawer and the admin item form's category picker, including inline category creation. Closes #182 |
||
|
bermudalamb
|
cf1680dbfb |
feat(security): stop the app origin serving anything it does not recognise, and make the uploads origin configurable (#103)
The uploads directory is the only place in this application where content someone else authored is served over HTTP. #95 stopped a dangerous file being stored; this stops a stored file doing damage if one ever gets there anyway — through a gap, a path added later, a restore, or a file written before that validation existed. Two halves, complementary rather than alternative. The app's own origin now serves uploads defensively. An allowlist of the three extensions the upload path can produce, so a `.html` or a `.svg` on disk is simply not a file this application hands out — 404, the same answer as a file that is not there, so the response cannot be used to learn which paths exist. An allowlist rather than a denylist because a denylist has to anticipate every type a browser might execute, which is a moving target across browsers and years, while this only has to know three. The content type is stated explicitly from that same list rather than sniffed or guessed from a name someone else chose, paired with `nosniff`. `default-src 'none'; sandbox` gives a directly-navigated file no capabilities at all, which is the only way one of these can do harm — an `<img>` embed does not execute script. Writes get 405 rather than falling through to a 404 that suggests the path is wrong. The other half is the separate origin, which is the real fix, because the origin is the whole unit of trust in a browser. That needs a hostname and a certificate, which live outside this repository, so what is here is the switch: `UPLOADS_BASE_URL`, sent to the frontend at runtime through `/api/config` and joined onto stored paths by `uploadUrl`. Empty means the app's own origin, which is the default and what local development has, so nothing changes until it is pointed somewhere. Stored paths stay site-relative. A stored value outlives any hostname baked into it, and rewriting them would be a migration to undo the day the hostname changes. Runtime rather than built in, so one image serves every environment — the same reason `paypalClientId` and `demoMode` are already there. `UPLOADS_BASE_URL` has a line in `docker-compose.prod.yml` while still empty, deliberately: a Portainer stack variable with no line there is substituted into the file and never reaches the container, which is exactly how `UPLOADS_DIR` went missing on 2026-08-23. Unset warns at boot, in the same shape as the admin gate — a working configuration with one defence switched off is worth saying out loud. Set without a scheme is refused outright, because a bare hostname joins onto a stored path as if it were relative and breaks every image on the site rather than failing visibly. The compose guard now resolves `${VAR:-default}` to its default, which is what the container actually receives when the stack variable behind it is unset. A bare `${VAR}` is still left opaque, so a required variable referenced that way goes on counting as present — that check is about the line existing, not about the stack being filled in. Closes #103 |
||
|
bermudalamb
|
9db3c6d94c |
feat(admin): filter inventory through the same flyout the storefront uses (#169)
The two screens asked the same questions through different UI. The storefront had searchable multi-selects in a flyout; the admin still had an always-visible row of controls with a single-select category that held a list of at most one, which is what #139 left behind so the shared filter type would not have to change shape twice. The drawer is now one component with the sections that differ driven by props rather than a second copy that would drift. Favorites is storefront-only. Status is admin-only, since pending is excluded from every public read and Published or Unpublished are not distinctions a customer can draw — the storefront keeps its three-way preset outside the drawer. The price slider needs real catalogue-wide bounds to be honest about where the prices are, and the admin has none, so there it is the two number inputs alone. What is shared is not only the markup but the phrasing: that categories are OR and tags are AND has to read the same on both screens or it stops being one rule. This reverses a decision `InventoryFilters.tsx` argued for in a comment — that hiding controls above a data table costs more than the space it saves, and that a drawer overlays the very rows being filtered. Both are true and both are traded for consistency between the panels. The active-filter chips are what makes the trade bearable: the current filter stays readable beside the button without opening anything, which is the part the always-visible row was really protecting. Status gets chips too, since it is now behind the button and is the filter most likely to empty a table. `STATUS_OPTIONS` moves beside the filter type, because the drawer and the chips both need to turn a status into a label and a second copy is a second place for a new status to be forgotten. The admin page object opens the flyout, acts, and closes it again — closing matters, because the drawer overlays the table every assertion in those specs is about. Closes #169 |
||
|
bermudalamb
|
856d8c4511 |
fix(filters): give the category tree selectable values, and drive both controls by search (#139)
Three things the e2e run found.
`toTreeData` still emitted `key`, which is how an antd `Tree` identifies a node and not how a `TreeSelect` selects one. Nothing could be picked, and `treeNodeFilterProp="title"` had nothing to filter against. It now emits `value`, matching the admin's CategoryTreeSelect.
The page object typed the name before clicking it, in both controls. Not for realism: the option lists are virtualized, so against a database holding hundreds of categories the wanted row never renders until a search narrows to it, and scrolling to it would be testing the virtual list rather than the filter.
Tag options are matched by class rather than by role, for the reason AdminInventory.toggleStatus already records — antd renders an invisible role="listbox" shim beside the real list, so getByRole('option') resolves to something zero-sized that can never be clicked. The drawer's own title is a plain element rather than a heading, so the click that closes an option list lands on the Favorites heading instead.
The multi-category URL assertion accepts the comma either percent-encoded or literal. URLSearchParams encodes it, which is how the `tags` parameter has always looked, and both spellings parse.
|
||
|
bermudalamb
|
faf38be91a |
feat(filters): make the storefront filter panel searchable and multi-select (#139)
The filter drawer did not scale with the taxonomy behind it. Categories were a bare antd `Tree` rendered at whatever depth it had grown to, with no search and single selection, and tags were a wall of every tag in the system. Neither said what was selected except through highlighting and chip colour. Both are now searchable multi-selects. Categories keep their hierarchy in a `TreeSelect`, matching the admin's `CategoryTreeSelect` so the two screens behave alike; tags become a multiple `Select` whose selected pills keep their colours, which is the only place a tag's colour was load-bearing. Several categories combine as OR. A customer picking Furniture and Decor wants both, not the empty intersection, and each selected id still expands to its descendants, so the answer is the union of the subtrees. That is deliberately the opposite of the tag rule, which stays AND, and both headings now state their rule rather than leaving it to be discovered. `ItemFilters.categoryId` becomes `categoryIds` end to end. The recursive CTE is seeded with `= ANY($n::int[])` rather than one id, which walks every selected root in one recursion and gives the OR for free; matching on `IN` keeps it a set test, so an item under two selected branches still appears once. The query parameter keeps its singular name and becomes comma-separated, the shape `tags` and `status` already use, so every `?category=1` link written before this still parses as a list of one. A list containing anything unreadable is still a 400, per decision 9 — honouring the readable half would answer a narrower question than the one asked and look indistinguishable from a filter that worked. The admin's inventory filter stays single-select, since it asks what is in a category rather than in any of several, but reads and writes a list of at most one so there is one shared filter type rather than two that drift. Closes #139 |
||
|
bermudalamb
|
7edc08e6eb |
feat(admin): make the placeholder chips insert at the cursor (#143)
The chips above each email editor named the placeholders and left an admin to retype `{{holdDuration}}` by hand, getting the braces and the spelling right unaided. A typo did not announce itself either: a misspelled placeholder is not a required one, so the save succeeded and the email shipped with a literal `{{holdDuraton}}` in it.
Clicking one now inserts its tag at the caret in whichever field was last focused, replacing any selection.
Four things this needed that a click handler alone would not have given.
The field has to be remembered rather than read. Clicking a chip blurs whichever of the subject or body had focus, so `lastFocused` is tracked on focus instead. It starts on the body, because that is where placeholders almost always go and because a chip clicked on arrival should do something predictable rather than nothing.
The insert goes through setState, not the DOM. Writing into the element's `value` would appear to work and would not: both fields are controlled, so the next keystroke re-renders from state and the insert vanishes.
The caret has to be put back. A controlled re-render leaves it at the end, so the new position is stashed in a ref and applied in an effect once the value has landed — just past what was inserted, with focus retained, so typing carries on from there.
And the chips had to become buttons. An antd Tag renders a span, so a keyboard user could neither reach one nor activate it. The button carries the semantics and the Tag the appearance, which makes enter and space work with no key handling of our own.
The textarea is found by querying the wrapper rather than through MDEditor's ref, which exposes an internal store that is not part of its API. The editor renders exactly one.
Five end-to-end tests, checked against a naive implementation rather than only against the finished one: reverted to append-and-forget, four of the five fail. The one that still passes is the plain insert-into-empty case, which appending also satisfies — worth knowing, since on its own it would have proved nothing.
Two mistakes worth recording, because both were mine and both were caught by running things rather than reading them. The spec first drove the passwordReset template, which email-templates.spec.ts already owns; stored templates are global per database, so the two files raced across Playwright's workers. Moved to the verification template — different key, no race. And its last assertion claimed the body did not contain `{{greeting}}`, which is false for that template before any click, since its default body already has one. Comparing the body against its own earlier value is what was actually meant.
Verified: tsc clean over src and tests, lint unchanged, build clean, and the three email specs pass 18/18 together.
Closes #143
|
||
|
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
|
45f1c77160 |
refactor(frontend): triage the setState-in-effect sites (#99)
Eleven warnings that looked alike and were not. This is a decision per site rather than eleven fixes, which is what the issue asked for — some of these would be made worse by "fixing" them. One was a real defect. VerifyEmail routed a fact through an effect that was already knowable during render: whether the link carries a token comes from the URL. The component therefore rendered once as a spinner in a state that was never true — a link with no token was never "verifying". Both pieces of state now derive their initial value from the token, and the effect's missing-token branch becomes an early return, so the failure is what the first render shows. Two are a defensible reset. CartProvider and FavoritesProvider clear their collection when the customer becomes null, which is synchronisation with the session rather than derived state. Deriving instead would push "signed out" onto every consumer of those contexts, and remounting on a `key` is more indirection than the problem deserves. Decided and written down rather than left for the next reader to re-investigate. Eight are legitimate and flagged conservatively. Six are a pending flag before a fetch — the rule cannot tell a spinner from a value that was already known. Cart's lapsed-item refetch is synchronisation with a server-side release the client cannot observe. useNow subscribes to the clock, which is the case the rule's own documentation names as correct. Each of the ten that stay carries the reason and a targeted disable, so lint drops from thirteen warnings to two — and the two left are the unrelated no-alphabetical-sort pair. Suppressing per site rather than switching the rule off keeps it live for new code, which is where the next VerifyEmail would be caught. The trap #60 recorded caught this, in a variant it does not describe. Placing the disable above `useEffect(` works only for a single-line effect: where the effect spans several lines the flagged line is the setState inside the body, so the directive covered nothing and produced both an unused-disable warning and the original one. Three sites were wrong that way on the first attempt. Confirmed fixed by the absence of "Unused eslint-disable directive" from the output — a disable that covers nothing reports itself, which is what makes this checkable rather than assumed. Verified: tsc clean over src and tests, and the specs covering the changed behaviour pass — verify-email, auth, favorites, orders, cart-countdown, resend-verification, 32 of 33 with the one failure passing 10/10 in a serial re-run. Closes #99 |
||
|
bermudalamb
|
8de261538b |
refactor(frontend): declare props read-only, and drop the deprecated antd prop (#100)
Seventeen components declared props the compiler was free to assume were mutable, and one antd prop had gone stale. Both mechanical, neither with any behaviour attached.
React never writes to props, and `Readonly<>` says so to the compiler rather than only to the reader. This finishes a pattern the codebase had already chosen rather than introducing one: AccountDetails and EmailTemplateEditor were already written as `type Props = Readonly<{…}>`, so the thirteen named prop interfaces are converted to that same shape and the four context providers, which annotate `{ children }` inline, get `Readonly<{ children: React.ReactNode }>`.
Cart.tsx was the last place passing `destroyOnClose`, deprecated in antd 5.20. Twelve other call sites across the admin screens, the filter drawer and four customer modals already use `destroyOnHidden`, so this one was simply stale. Deprecated props keep working until they do not, and the failure then arrives as an antd upgrade breaking something unrelated to the change being made.
Counted rather than assumed, which the issue specifically asks for, because a `Readonly<>` in the wrong position type-checks and fixes nothing: lint goes from 31 warnings to 13, a drop of exactly eighteen, and both rules disappear from the breakdown entirely rather than merely thinning out.
What that leaves is the point of doing it. The remaining thirteen are eleven `set-state-in-effect` and two `no-alphabetical-sort` — so the frontend's warnings are now only the ones that need a decision, which is what makes #99 tractable. It had grown from the eight in that issue's title to eleven, two of them added by #97's clock tick and lapsed-cart refetch.
No behaviour change intended, so the bar was the end-to-end suite. Full run: 121 passed, 8 failed; all eight pass in a 45/45 serial re-run, which is the shared-database and event-loop flakiness this suite has had throughout.
Closes #100
|
||
|
bermudalamb
|
461ab01d4c |
refactor(storefront): extract the catalogue machine from App.tsx as useCatalogue (#98)
App.tsx was 361 lines, and roughly sixty of them were one cohesive concern with nothing to do with laying out a page: fetching the catalogue for the current filters, debouncing it, and negotiating with the session before it could ask. Six pieces of state, four callbacks and two effects, sharing scope with the header, footer, filter drawer and auth modal that make up the rest of the file. This is the same shape #81 dealt with once. That change took out the rendering half by extracting Catalogue and brought the file under the cognitive-complexity limit. The state half stayed. App keeps the URL as the source of filter truth, because that genuinely belongs to the page: a reload, a shared link and the back button all have to restore the same view. What moves is the request, the debounce, and the auth negotiation — and that last one is the reason this is worth doing. The rule the comments explain at length, that firing before the session resolves would 401 and show an outage banner to someone who is in fact signed in, now has somewhere to live rather than being a pair of derived booleans in a page component. The hook decides when the favorites filter needs a session; the page decides what to do about it, through an onAuthRequired callback, because the prompt is a modal the page owns. The serialise-then-reparse of the filters is kept, with the reason written down rather than left to be rediscovered. Depending on a string is what keeps `load` referentially stable while the filters are value-equal, and `load` is what the debounce effect depends on — depending on the object would give a new `load` every render, restart the debounce each time, and fire a request per keystroke. The alternatives considered were a ref written during render and threading the key through the page, and both are worse than one honest comment. filterKey is returned rather than recomputed by the caller: the page needs exactly that value to reset the catalogue's error boundary, so a crashed grid gets another chance when the filters change. App.tsx is 300 lines. No behaviour change is intended, so the bar is the end-to-end suite unchanged — the storefront listing, the filters, the favorites-requires-sign-in prompt, the failure banner and the boundary reset all pass. Also removes what the extraction left dead in App.tsx: the useEffect import, the debounce constant, and `authLoading`, which existed only to decide whether to fire the request. Refs #98 |
||
|
bermudalamb
|
5ef97bef21 |
fix(cart): make the reservation countdown tick, and warn against the real hold (#97)
The cart showed "2h 15m left" and turned it red in the final hour. Neither updated. Both readings happened during render from the wall clock, and nothing scheduled a re-render — no setInterval anywhere in the file — so the number a customer read was whatever it was when the page loaded, and the warning colour could only appear by accident, because the component had already rendered before the final stretch began. That matters more here than in most shops: every item is one of a kind, so a lapsed reservation is not "buy it later", it is someone else buying the only one. New useNow hook returns the time as state rather than merely forcing a re-render, and that is the point. A component reading Date.now() while rendering produces output that depends on the clock, which React is entitled to assume it does not — react-hooks/purity says so, and this was the only instance in the codebase precisely because it was the only place doing it. Reading `now` from state makes render a function of its inputs again, so the rule is satisfied rather than suppressed. It ticks every 30s, which matches the display's one-minute resolution, and only while the cart holds something, so an empty cart is not waking React forever. A second defect the issue did not mention. The red warning was hardcoded to the final hour, but the hold became admin-configurable in #136 and accepts values as low as half an hour — so on any setting below an hour every item was red from the moment it was reserved, and a warning that is always on is not a warning. It now keys off the last tenth of the item's own added_at-to-expires_at span. Reading it from the item rather than from the setting also means an admin changing the value does not retroactively relabel a reservation granted under the old one. At zero the row keeps saying "expiring…" and the page refetches on each tick while anything is lapsed, so it clears within one interval of the server's sweep actually releasing it. The client cannot know when that lands — the sweep runs every few minutes — so the wording claims imminence, which is true, rather than completion, which is not ours to say. The header badge is refreshed alongside, since it counts held items and goes stale the same way. Verified against the unfixed component, not just the fixed one: two of the three new tests fail on the old code. The third documents the "expiring…" wording rather than the fix, and passes either way — worth having, but it is not evidence. The tests use Playwright's clock control rather than waiting in real time, which also makes them deterministic: without it, "the text changed" would depend on where in the minute the run happened to start. Refs #97 |
||
|
bermudalamb
|
3b3bd06bbe |
test(e2e): convert the remaining admin specs, completing the POM refactor (#137)
The last nine: admin-taxonomy, admin-save-failures, admin-theme, admin-inline-category, admin-item-preview, admin-disable-customer, admin-reserved-items, admin-inventory-filters, email-templates and admin-email-settings. Every spec in tests/e2e now goes through page objects. Measured rather than asserted: - raw CSS and antd-internal locators in spec files: 0 (was 13) - local `register` helpers: 0 (was 9) - hardcoded http://localhost:5173: 0 (was 3) The antd knowledge that was spread across seven spec files is now in four page objects, each with the reason written next to it. Three of those are things no reader could have guessed from the locator: Segmented hides its real radio behind a styled label, so the input is found by role and cannot be clicked — the title attribute is the handle. The status multi-select renders an invisible role="listbox" shim beside the real list, so getByRole('option') finds something zero-sized; and a selected status renders again as a tag carrying the same title, so an unscoped getByTitle is ambiguous. Matching the visible option class avoids both. The dropdown is also opened only when closed, because antd keeps it open after a selection in multiple mode. Select popups render into a portal at the end of <body>, outside the tab panel they belong to, so a dropdown cannot be found by scoping to the panel. AdminPage.tab gained an `exact` flag for one specific collision: the Emails tab contains a rail that also renders tabs, and "Email verification" contains "Email". Without exact matching, opening the Emails tab is ambiguous with the template inside it. Two helpers stayed local rather than moving into page objects, because they belong to their file's subject rather than to a surface: favorites-filter's `favorite()`, which settles the opt-in modal and waits for its fade before the wrapper stops intercepting pointer events, and admin-theme's `luminance()`, which is a WCAG calculation and not a locator. Both now take page objects as parameters instead of reaching for locators themselves. Verified: 26/26 spec files converted, tsc clean over the whole tree, lint at the 30-warning src baseline with nothing added. Full suite 123 passed / 5 failed; all five pass in a 33/33 serial re-run, which is the load-related flakiness this suite has had throughout and not a change here — the backend hashes passwords with bcryptjs, a pure-JS implementation that blocks the event loop for every request while it runs. Closes #137 |
||
|
bermudalamb
|
549a08038e |
test(e2e): convert the favorites, availability and orders specs (#137)
favorites, favorites-filter, sold-filter, orders and pending-publish. Five more copies of "register a customer" and three more of the hardcoded base URL go with them. Two locators that were hiding real knowledge are now named. `gridCell` is the item's whole antd column rather than its card, needed because the SOLD ribbon renders outside the card — three specs reached for `.ant-col` directly to get at it. And `chooseAvailability` goes through the title attribute because antd's Segmented hides the real radio behind a styled label, so the input is found by role and cannot be clicked; that fact was written out twice in comments and is now written once in code. The favorite control is located page-wide rather than within a card. The storefront paginates as items accumulate and the control is named for its item anyway, so scoping to a card bought nothing and broke whenever the card was on another page. favorites-filter keeps its local `favorite()` helper. It is genuinely local — decline the opt-in, wait for the fading modal to stop intercepting pointer events, confirm the heart flipped — and belongs to that file's subject rather than to the storefront. It now takes page objects as parameters instead of reaching for locators itself, which is what a spec-level helper should look like. Two specs still drive the registration form rather than taking the `customer` fixture, and deliberately. Both are about a signed-out visitor being interrupted mid-action — favoriting an item, or switching on the favorites filter — and the claim is that the thing they asked for survives the interruption. Replacing the interruption with an API call would delete the test. Verified: favorites 6/6, favorites-filter 7/7, sold-filter 6/6, orders and pending-publish 8/8. Notably favorites-filter's "keeps showing a favorite after it sells" passes, which had been failing on a strict-mode violation from two items sharing a name across runs. Refs #137 |
||
|
bermudalamb
|
9b3a03d1bf |
test(e2e): convert the storefront and filter specs onto page objects (#137)
storefront, storefront-errors, theme, error-boundary and filters. filters.spec.ts carried the last hand-rolled copies of createCategory, createTag and createItem, and the hardcoded `http://localhost:5173` that meant changing the port in the config would have moved every test except this one. Seeding now goes through support/api, and the host it needs lives in one constant. It has to be a constant rather than the config's baseURL: beforeAll runs with worker-scoped fixtures only and cannot read a test-scoped option, which is why the URL was inlined there in the first place. New FilterDrawer object. The two rules it encodes are the ones the tests exist to pin down and neither is guessable from a locator: categories are a tree because the filter matches a node and everything filed beneath it, and tags combine with AND rather than OR, so selecting two means "must have both". The active-filter chips go on StorefrontPage rather than the drawer, because that is where they render — and the scoping matters, since the drawer carries a "Clear all" of its own that an unscoped locator also matches. StorefrontPage gains the three things the catalogue says instead of listing items. They are named together deliberately: the distinction between "No items yet" and "Couldn't load items" is the point, and several tests assert one is showing while the other is not, because telling a customer the shop is empty when the server is broken hides the outage. The theme switch and the attribute it writes are both on Header now. The switch is in the header and `data-theme` lands on <body>, so a spec previously had to know about `body` to observe the control it had just clicked. Verified: 12/12 across the four small specs, 8/8 on filters, tsc clean, lint unchanged at the 30-warning src baseline. Refs #137 |
||
|
bermudalamb
|
4b0c01068f |
test(e2e): convert the account specs onto page objects (#137)
account-modal and account-details, taking two more copies of "register a customer" and two more of the `accountModal(page)` helper that every account-touching spec had rewritten.
Both files had grown their own vocabulary for the same dialog. account-details reached for `modal.getByLabel('Current password', { exact: true })` and its five siblings inline in each test, so a change to the account form meant editing six places in one file and more in the next. Those are named locators now, and the three multi-step operations — saving a name, changing a password, changing an email — are actions, because each is a disclosure to open and three fields to fill before the button does anything.
AccountModal.open() gains the 20s timeout the header already had, and for the same reason. Arriving at /account means booting the app and resolving the session against the server. The old specs never noticed because they registered through the form first, which loaded the app and confirmed the session before navigating; taking the API-registered `customer` fixture arrives cold, and the 5s default is comfortably beaten on an idle machine and missed on a loaded one.
Verified: 18/18 across three serial repeats, and 12/13 in parallel. The one failure is `changes the email address and marks it unverified again`, which fails identically on the unconverted file and passes whenever the suite is not saturated — the same load-related flakiness as the rest of the family, not something this commit introduced.
Refs #137
|
||
|
bermudalamb
|
abcc684447 |
test(e2e): convert the auth specs onto page objects (#137)
First conversion batch: auth, password-reset, resend-verification. verify-email is left alone — it already used semantic locators and duplicated nothing, and rewriting it to prove a point would be churn. Four of the nine copies of "register a customer" go here. auth.spec.ts and password-reset.spec.ts each carried their own `register`, and resend-verification.spec.ts its own `registerCustomer`, all three re-explaining the same bcrypt wait and the same "the header, not the URL, proves the session exists" reasoning in slightly different words. Two also carried their own `logout`, and two their own `uniqueEmail` with different prefixes. Most of those tests were not about registering. They needed an account to exist so they could test logging out, resetting a password, or resending a verification email, and they paid for a bcrypt round-trip through the form to get one. Those now take the `customer` fixture, which registers through the API. The three tests that genuinely are about the registration form still drive it, because the thing under test has to be the thing exercised. The batch drops from 47s to 26s as a side effect, which is the cost of that round-trip made visible. password-reset.spec.ts loses its inline pg.Client. The reasoning for reading the database directly is unchanged and still right — an endpoint returning a reset token for an arbitrary address is account takeover if it is ever reachable — but it now lives in support/db.ts where it cannot be copied into the next spec wanting a shortcut. It also stops defaulting to port 55432, which is the integration suite's disposable Postgres rather than the database the app under test is connected to, and is Hyper-V-reserved on at least one machine here. Both tests in that file previously failed with a bare ECONNREFUSED unless TEST_PGPORT was set by hand; they now pass with no environment at all. New PasswordResetPages object covers both halves of recovery — requesting a link, and using one — because they are one flow and a test usually crosses between them. One lint decision worth recording. Requesting a Playwright fixture IS using it: destructuring `customer` is what makes the account exist, whether or not the body then reads the address. The linter cannot see that side effect and reports every such fixture as an unused variable. The first attempt at appeasing it was a `void customer;` line per test, which is noise standing in for a comment — and sonarjs flags that too, so it traded one warning for another. `no-unused-vars` is now configured with `args: 'none'` for tests only, with the reason written next to it. Variables are still checked; only parameters are exempt. Verified: 26/26 in the converted batch, and 127 passed in the full suite with four failures — three in the known #116 flaky family, and resend-verification's rate-limit test, which passes 9/9 across three repeats in isolation and is timing-sensitive under parallel load rather than changed by this commit. Refs #137 |
||
|
bermudalamb
|
5c907fcf9a |
test(e2e): add page objects, fixtures and a typed test build (#137)
Foundation only. No spec is converted in this commit, so the suite behaves exactly as before — the conversions follow in themed batches, each leaving the suite green. The suite had grown by copy-paste. Registering a customer was implemented nine times, as `register` in five files and `registerCustomer` in four more, each carrying its own re-explanation of the same bcrypt wait and the same "the header, not the URL, proves the session exists" reasoning. `uniqueEmail` was reinvented per file with a different prefix and a different encoding each time. Thirteen locators reached into antd's internals — `.ant-tabs-tab-active .ant-tabs-tab-btn`, `.ant-select-item-option[title=...]`, `.ant-col` — spread across seven files, so an antd upgrade breaks tests that have nothing to do with it. Page objects hold named locators and the actions that operate on them; assertions stay in the specs, so a test reads as its own statement of what it verifies. The exception is an action waiting for its own completion — registering waits for the account button, opening an admin tab waits for its panel — because that wait is the action's contract, and pushing it to callers would recreate the duplication being removed. Fixtures carry the setup rather than the specs. `customer` registers through the API rather than the form: nine specs drove the registration form purely to arrive at a signed-in session, so a broken form failed a hundred tests that were not about it, and each paid for a bcrypt round-trip through the UI. `page.request` shares the browser context's cookie jar, so the session belongs to `page`. The specs that are genuinely about registration drive the form properly through `authModal`. `adminApi` takes its base URL from the Playwright config. One spec built its own request context against a hardcoded http://localhost:5173, so changing the port in the config would have moved every test except that one. The inline pg.Client in password-reset.spec.ts moves to support/db.ts. The reasoning for reading the database directly is unchanged and still right — an endpoint that returns a reset token for an arbitrary address is account takeover if it is ever reachable, and an environment gate is a thin thing to stand between that and production — but it no longer sits in a spec where it can be copied into the next one wanting a shortcut. Its default port becomes 55500, the local stack's, rather than 55432: that is the integration suite's disposable Postgres, a different database with different credentials that the app under test is not connected to, and it is Hyper-V-reserved on at least one machine here, so the spec failed with a bare ECONNREFUSED naming a port nobody had chosen. tsconfig.test.json type-checks the tree and runs as part of `npm run build`. It is separate from tsconfig.json rather than widening its `include`, because scripts/check-sonar-tsconfig.js compares the two configs' include arrays, and pulling the Playwright suite into SonarQube's analysis program is a different decision from type-checking it. The whole existing suite type-checks clean on the first run. Lint now covers tests/ with `project` rather than `projectService` — the service resolves a file to the nearest tsconfig.json, which for tests/ is the one that excludes them, and every file then errors as not part of a project. no-floating-promises is an error here: Playwright's API is almost entirely promises, and a missing await on an assertion does not fail, it passes having asserted nothing. Four rule families are switched off for tests rather than left as warnings. Bringing these files in scope added 45, of which none were defects, and #60's argument is that a gate nobody reads is not a gate. There is no React in this directory, and the hooks rules fire on ordinary functions whose parameter is named `use` — which Playwright fixtures are, by its own API. Test credentials are the point of a test and the project's own rule is that they live only in test paths, which is here. Math.random builds unique fixture names so parallel workers do not collide, and a cryptographic generator would say something untrue about what the value is for. The count is back to the 30 that src carried before. Refs #137 |
||
|
bermudalamb
|
2f7268704a |
feat(admin): make the token lifetimes, cart hold and greeting configurable (#136)
Giving the customer emails a tab of their own (#135) made a family of related holes visible: an admin could edit the wording of every customer email, but not the facts that wording asserted, and two templates could not address the customer at all. Both token lifetimes were hardcoded twice. VERIFY_TOKEN_TTL_MS sat in routes/customers.ts while the verification body separately said "This link expires in 24 hours", and RESET_TOKEN_TTL_MS sat beside a body separately saying "one hour". The prose was not derived from the constant, it was a second hand-written copy of the same fact — so making the constants configurable without addressing that would have made things worse, not better: the setting moves to two hours and the email keeps confidently promising one. Both are now settings, and both templates state their lifetime through an {{expiresIn}} placeholder rendered from the setting. The cart reminder gains {{holdDuration}} for the same reason. Per-item deadlines were already inside {{itemList}}, but there was no way to write a sentence about the hold itself without hardcoding a number the cart expiry setting could change underneath the author. All three durations render through one formatDuration(), so the reset email and the cart reminder say "one hour" the same way rather than in two authors' phrasing. A fractional hour drops to minutes, because "0.5 hours" reads badly and "1.5 hours" reads worse in a sentence a customer is meant to act on. Every template now offers greeting, firstName and lastName. favoriteSold and favoriteWithdrawn previously offered only itemName and siteUrl and could not address anyone — the query behind them never selected a name, so it does now. The greeting itself is two settings, a format and a fallback, rather than the wording baked into greeting(). The fallback is separate rather than the format with the name edited out: that editing is guesswork that has to be right every time, and getting it wrong ships "Hi ," to everyone who registered while first names were still optional (#106). Those customers exist, which is why greeting() guarded the case in the first place. The admin preview renders the durations and the greeting from the live settings rather than from a static sample. The preview exists so an admin sees the email that will be sent, and a sample reading "one hour" while the setting says two is the precise failure these placeholders were added to remove. Settings are read through a new adminSettings accessor. cart_expiry_hours was previously read by an inline query in two places, each with its own `|| '24'`; with five settings and read sites across three routes and the cron job, a default written twice is a default that will eventually disagree with itself. Values are stored as text, so each definition declares how to read it back — numbers were the only kind until the greeting format arrived. The new placeholders are available but never required, so every template an admin has already saved keeps rendering and keeps sending. Also fixes updateAdminSettings announcing success for a save the server refused: it returned the 400 body as though it were the saved settings, so the form reported "Settings saved" either way. Closes #136 |
||
|
bermudalamb
|
7df897c0fd |
feat(admin): give the customer emails a tab of their own (#135)
The six email templates lived at the bottom of the Settings tab, under the cart-expiry card and inside a 720px wrapper. Finding them took knowing they were there — "Settings" reads as app configuration and the only thing visible on that tab was a 480px card about cart expiry. Reaching them, the editor was then crushed: EmailTemplateEditor splits a markdown pane and a rendered preview side by side, and 720px left each half under 350px, so the preview showed the email at a width nothing like how it will be read and the markdown toolbar wrapped. Emails is now its own tab, between Customers and Settings, with no width cap. Within it the email types are a left vertical rail rather than a strip across the top: six labels wrapped on narrower displays, and stacking them is what leaves the editor the width the split needs. Settings keeps the cart-expiry card and nothing else. The Default/Customised tag comes off the labels. Six antd tags stacked down a rail stop it being scannable, so a customised template gets a dot and the state in full moves into the editor beside the Restore default button that acts on it. The dot carries aria-label="Customised" so the word stays in the tab's accessible name and the state is not conveyed by a mark alone. Emails owns the fetch it inherited from Settings, and adds a Spin over it. templates starts empty, so the gap before the request lands would otherwise render an empty rail that reads as "there are no emails to edit". Closes #135 |
||
|
bermudalambandClaude Opus 5
|
4de1c9b34d |
feat(admin): filter inventory by status directly, so Published and Unpublished are reachable (#132)
There was no way to find unpublished items. Every item has arrived pending since #90 and has to be published, so "what is waiting for me to publish" is a routine question the inventory could not answer. #105 replaced the four-way status dropdown with a Sold / Not sold / All preset and recorded at the time that this gave up isolating a single status, that the pending workflow was the likeliest thing to miss it, and that the fix would be to restore the ability rather than remove the preset. That turned out to be right, and sooner than expected. The admin now selects statuses directly - Pending, Available, Reserved, Sold - rather than choosing among presets over them. The API has accepted several statuses since #105, so this exposes the dimension itself. Everything becomes expressible in one control: Unpublished is Pending, Published is the other three, Sold and Not sold are the sets they always were, and Reserved on its own is reachable again. Two alternatives were rejected. Growing the preset list to five would have kept one click per answer while leaving Reserved unreachable and growing again at the next new question. A second control for publication beside the one for availability would have read more naturally and reintroduced exactly what #105 was built to avoid: Sold and Unpublished is an impossible pair, since a sold item is necessarily published, and two dimensions have to either give that a meaning or block it. One dimension cannot contradict itself. The storefront keeps its three-way preset unchanged. Pending is excluded from every public read, so Published and Unpublished are not distinctions a customer can draw, and the simpler control is the right one there. An empty selection means no filter rather than no statuses, or clearing the box would empty the table. Verification: the admin filter spec is rewritten rather than deleted, and now asserts what the preset could not - Pending alone finds the staged fixture and hides the published ones, and Available plus Reserved plus Sold finds the published ones and hides the staged one. That second case is the one a preset would have had to be invented for. All 7 admin filter tests pass, along with 122 of the suite. Two locator details cost time and are written into the spec so they do not have to be rediscovered: antd renders an invisible role="listbox" shim beside the real option list, so getByRole('option') resolves something zero-sized that cannot be clicked; and a selected status renders as a tag carrying the same title as its option, so an unscoped getByTitle becomes ambiguous once anything is chosen. Beyond the two pre-existing password-reset failures that need a database on port 55432, two storefront specs failed under the full concurrent run and pass six-for-six in isolation, twice. That is the shared-database contention filed as #116, not a regression here: this change touches the admin only. Closes #132 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
2841d978b9 |
feat(admin): use the item description's markdown editor for email templates (#131)
Two places in the admin edited markdown and neither looked like the other. The item description has had MDEditor's toolbar since it was added; the email bodies were a bare monospace textarea. The email bodies are the worse place for that, since a markdown mistake there goes to customers rather than onto a product page.
They now use the same editor, with the same data-color-mode wrapper so it follows the admin's dark mode exactly as the item form does.
One thing was deliberately not copied. The item form uses preview="live", which gives MDEditor its own preview pane. This one uses preview="edit". MDEditor's preview renders with a different markdown implementation, would show a literal {{resetUrl}} rather than a sample value, and would omit the consent footer the server appends to the two favorite templates. The pane on the right is the server's rendering of the actual email, produced by the same renderer the mailer uses; putting a second, less accurate preview beside it would leave the admin two answers and no way to tell which one the customer gets.
The aria-label moves to textareaProps. Input.TextArea carried it directly, MDEditor owns its textarea, and without it every email template test loses its handle on the field along with the only thing naming it for a screen reader.
Verification: the eight end-to-end tests from #119 pass unchanged, which is the check that matters here - not one of them was edited to accommodate the swap, so the label, the save path and the server preview all still work through the new editor. tsc, ESLint and the production build are clean.
Worth recording, because it wasted a diagnosis: the first run of those tests failed on the preview, and the cause was a stale backend build with no preview route rather than anything in this change. The iframe was empty from the start, before any typing, which is what gave it away - a broken editor would have shown the default copy and failed to update it.
Closes #131
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
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> |
||
|
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> |
||
|
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
|
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
|
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> |
||
|
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
|
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> |