main
552
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 bermudalamb
|
949734d1e1 |
docs(ci): add the working document for the #154 schema-loss investigation (#154)
The diagnosis so far, what has been ruled out, and the three read-only checks that would confirm or refute it — with slots to paste the output into and a note against each saying what the answer means. Written as a working document rather than a summary because the decisive check has a timing constraint that is easy to miss: the Postgres service container is deleted when the job finishes, so watching it has to happen during the run. Discovering that after the fact costs another full run. It also records the negative result deliberately. If the container is healthy throughout, the hypothesis is wrong and the document says which suspect is next, rather than leaving an abandoned theory for the next reader to re-derive. Refs #154 |
||
|
bermudalamb
|
c55b2b3a25 |
Merge pull request 'ci: let the summarisers summarise and the gate do the failing (#142)' (#155) from feature/142-ci-failure-reporting into main
Reviewed-on: #155 |
||
|
bermudalamb
|
491c2652f3 |
ci: let the summarisers summarise and the gate do the failing (#142)
A failing end-to-end run reported itself like this:
Run node scripts/summarize-playwright.js frontend/playwright-results.json
❌ Failure - Main Summarize end-to-end tests
exitcode '1': failure
which reads as a broken summary script. It was not — it was the summariser correctly reporting that tests had failed, with nothing said about it.
Two things combined to produce that.
The summariser doubled as the gate. It ended with `process.exit(stats.unexpected > 0 ? 1 : 0)`, so it failed the job itself. The workflow already has a step written for exactly that — `Fail if either suite failed`, whose comment explains it exists so `continue-on-error` on the suites cannot turn a failing suite into a passing job. That step was being skipped while its condition was true, because a step whose `if:` omits `always()` still implicitly requires its predecessors to have succeeded, and the summariser had already failed the job one step earlier. The gate written to be the place the job fails was dead code.
And the explanation went where the log is not. `report()` writes to GITEA_STEP_SUMMARY when it is set, which under Actions is always, so the counts and the list of failing tests landed in the Summary tab while the log showed a bare exit with no output at all.
So both scripts now exit 0 whatever they find, both print one line of counts to stdout as well as the markdown to the summary, and the gate carries `always() &&` so it actually runs. A failing suite now fails at a step named for what failed, and the log says how many.
Verified by executing both scripts against crafted results rather than by reading them: failing counts, passing counts, and a missing results file, each with and without GITEA_STEP_SUMMARY set. All four exit 0, the headline appears on stdout in every case, and the step summary still receives the full table and the failure list.
Not covered: nothing in this repository runs the scripts under `scripts/`. Jest's testMatch is scoped to backend/tests/unit, so a permanent regression test would need a runner these files do not have. Worth its own issue rather than widening the backend suite's roots to reach the repository root.
Closes #142
|
||
|
bermudalamb
|
d4e1b3ac51 |
Merge pull request 'refactor(storefront): extract the catalogue machine from App.tsx as useCatalogue (#98)' (#153) from feature/98-use-catalogue into main
Reviewed-on: #153 |
||
|
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
|
c779b5a180 |
Merge pull request 'fix(cart): make the reservation countdown tick, and warn against the real hold (#97)' (#152) from feature/97-cart-countdown into main
Reviewed-on: #152 |
||
|
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
|
507c56bbb8 |
Merge pull request 'Feature/137 convert account specs' (#151) from feature/137-convert-account-specs into main
Reviewed-on: #151 |
||
|
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
|
ed679986de |
Merge pull request 'test(e2e): convert the auth specs onto page objects (#137)' (#150) from feature/137-convert-auth-specs into main
Reviewed-on: #150 |
||
|
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
|
882f42447b |
Merge pull request 'test(e2e): add page objects, fixtures and a typed test build (#137)' (#149) from feature/137-playwright-page-objects into main
Reviewed-on: #149 |
||
|
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
|
b2afa40800 |
Merge pull request 'fix(deploy): run the image QA reviewed rather than rebuilding production (#146)' (#148) from feature/146-promote-tested-image into main
Reviewed-on: #148 |
||
|
bermudalamb
|
e48c7f585b |
fix(deploy): run the image QA reviewed rather than rebuilding production (#146)
The production compose committed in #145 carried `build:` and `pull_policy: build`, copied from the QA stack without thinking about what they mean there. QA builds from this repository because QA is where a change is first assembled and reviewed. Production is not that; production is where the reviewed thing runs. Worse, it contradicted the deploy this repository already documents. README's production steps promote the exact image QA reviewed — `docker tag redefined-designs:qa redefined-designs:latest` — with the stated reason that what ships is what was tested. A compose file that rebuilds instead quietly overrode that, and the two would have disagreed at the moment it mattered. Rebuilding would be a defensible shortcut if a rebuild of the same commit produced the same image. It does not. The Dockerfile copies package.json without package-lock.json and installs with `npm install`, so both lockfiles in this repository are ignored in every build stage and every dependency range is resolved afresh. Two builds of one commit, minutes apart, can differ in any transitive dependency that published in between. "Same git ref" is therefore not "same image", and the reviewed bytes are the only thing that is. So the service now declares `image: redefined-designs:latest` and nothing else. The image has to exist before the stack starts; a first deploy or a pruned NAS fails with "image not found" rather than silently building something new. That is the intended behaviour and is written into the file rather than left to be discovered. The header records what removing `pull_policy: build` costs, because it is not free. That option exists in QA to stop a redeploy reusing a stale tag and appearing to succeed while running old code. Production reintroduces the same risk by a different route — a redeploy that reuses the previous `latest` because nobody re-tagged — so the promotion is load-bearing rather than a convenience, and the deploy has to say which image it is promoting. Also corrects the README's claim that production runs from a stack outside this repository and so cannot be checked. That was true when it was written and stopped being true in #118; leaving it would have taught the next reader that the environment which just failed to boot is the one nothing watches. The remaining half of #146 — copying the lockfiles and installing with `npm ci` so any rebuild means something — is not in this commit. It changes what CI and QA build as well as production, and belongs with its own verification that an unchanged commit produces an unchanged dependency tree. Refs #146 |
||
|
bermudalamb
|
50c9b0dd39 |
Merge pull request 'fix(deploy): commit production's compose and bring it under the drift guard (#118)' (#145) from feature/118-production-compose into main
Reviewed-on: #145 |
||
|
bermudalamb
|
3fde6fc6bf |
fix(deploy): commit production's compose and bring it under the drift guard (#118)
Production refused to boot with "UPLOADS_DIR is required and is not set" while UPLOADS_DIR was set in Portainer's stack variables. Both statements were true at once. Portainer substitutes stack variables into the compose file rather than handing them to the container, so a variable with no line in the file never reaches the app — behaviour the QA compose already warns about at its ADMIN_GATE_SECRET entry, hit in production where nothing was watching for it.
Nothing could have caught it. The drift guard reads docker-compose.qa.yml, and production ran from a Portainer stack outside the repository that no test could see. That is worse than an even gap: UPLOADS_DIR is in ALWAYS_REQUIRED and the test was green, so the natural reading was that the deploying environments set it. QA did. Production did not.
So production's compose is now a file in the repository, deployed as a git repository stack rather than pasted into the web editor — otherwise the committed copy and the running copy drift apart again, which is the whole problem.
Values are hardcoded rather than interpolated wherever they are not secrets. Only a secret has a reason to stay out of the repository, and every interpolation is another chance for the failure above. UPLOADS_DIR in particular has to agree with the volume mapping, and splitting it across two files is how they drift.
The guard now runs over every deployment rather than QA alone, and checks each by handing its parsed entries to validateEnv itself rather than restating the rules. A restatement is one more copy to drift; running the real validator means the file is checked against exactly what the container checks at boot. Interpolated ${SECRET} values count as present, which is right — what is being guarded is that the line exists, since that is what decides whether the value reaches the container.
Environments differ on purpose, so the expectations are registered per file rather than shared: QA is demo mode with a mail allowlist and no PayPal credentials, production is the reverse of all three. A root-level compose file that is not registered fails the last test, so adding an environment forces the decision instead of silently inheriting whatever the loop asserted.
Verified by removing the UPLOADS_DIR line from the production file and confirming three tests fail, one of them reproducing the exact boot error. A guard of this kind that has never been seen to fire is indistinguishable from one that cannot.
Two things found while writing this and deliberately not changed here. RESERVATION_MINUTES is set in QA's compose and read nowhere in the code — drift in the opposite direction, which #118's scan half should catch. And production publishes 32750 on every interface exactly as QA does; that is #117, and the reasoning is recorded in a comment at the ports block rather than acted on, since changing it needs the proxy host entry repointed in the same pass.
Refs #118
|
||
|
bermudalamb
|
c798ba08d7 |
Merge pull request 'feat(scripts): switch Node automatically, and add a test runner (#140)' (#144) from feature/140-node-scripts into main
Reviewed-on: #144 |
||
|
bermudalamb
|
9ac2fa3ba1 |
feat(scripts): switch Node automatically, and add a test runner (#140)
start-local.ps1 knew exactly what was wrong when Node was too old and then made you fix it by hand. Assert-NodeVersion read node --version, found a major below 20, and threw a message telling you to run `nvm use 24.13.1` and start again in a new shell. A good error for a problem the script could simply solve — and since nvm's default here is 18.16.1, it was hit on every fresh shell. It now runs `nvm use latest` itself and -Stop puts the machine back to 18.16.1. The revert also runs when a start fails partway: without that, a run dying in migrations leaves the machine switched with nothing started, and the -Stop that would restore it is never reached. nvm rewrites a machine-global symlink rather than changing one shell, so this changes the Node version for every terminal on the machine while a script runs. That is the intent — the point is to work in whatever shell is already open — but it is announced every time rather than done quietly. The switch is verified rather than trusted. nvm-windows exits 0 for switches that did not happen: a version it cannot find, a symlink it cannot rewrite without elevation, and — observed here — a rewrite issued immediately after another one, where the directory symlink is briefly still the old target. That last case turned up while testing this change: `nvm use latest` reported success and left Node on 18.16.1. So the result is read back and retried once, and nvm's own output is captured rather than discarded, because suppressing it hid the only message that explained the failure. run-tests.ps1 runs the suites: -Suite unit|integration|e2e|all. One script with a parameter rather than three, because the version switch, the database bring-up and the TEST_PGPORT handling are shared and three copies would drift. The integration suite gets its own throwaway Postgres started and stopped around it, in a finally so a failing suite still tidies up. The e2e suite checks the backend is answering first and says what to start, rather than leaving twenty-five specs to fail on a refused connection that names nothing. `all` runs cheapest and most isolated first, so a break several suites would show is reported by the one that localises it best. The version switching lives in scripts/NodeVersion.ps1, dot-sourced by both, since two copies of it would drift and the half that drifts is the half nobody runs. Closes #140 |
||
|
bermudalamb
|
792daccafb |
Merge pull request 'feat(admin): make the token lifetimes, cart hold and greeting configurable (#136)' (#141) from feature/136-configurable-token-lifetimes into main
Reviewed-on: #141 |
||
|
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
|
6c837725bd |
Merge pull request 'feat(admin): give the customer emails a tab of their own (#135)' (#138) from feature/135-emails-tab into main
Reviewed-on: #138 |
||
|
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 |
||
|
bermudalamb
|
717a32c923 |
Merge pull request 'feat(admin): filter inventory by status directly, so Published and Unpublished are reachable (#132)' (#134) from feature/132-admin-status-multiselect into main
Reviewed-on: #134 |
||
 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> |
||
|
bermudalamb
|
7798c847c1 |
Merge pull request 'feat(admin): use the item description's markdown editor for email templates (#131)' (#133) from feature/131-md-editor-for-emails into main
Reviewed-on: #133 |
||
|
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>
|
||
|
bermudalamb
|
7ca07bd4a0 |
Merge pull request 'feat: let a customer resend their own verification email (#110)' (#130) from feature/110-resend-verification into main
Reviewed-on: #130 |
||
|
bermudalambandClaude Opus 5
|
b8549e9c72 |
feat: let a customer resend their own verification email (#110)
A verification email was sent once, at registration. If it was lost, filtered, or arrived after the 24-hour token had expired, the customer had no route back except registering again with a different address. POST /api/customers/resend-verification mints a fresh token and sends the mail, and the account page grows a "Send it again" button beside the warning that was already there. The button only exists while the address is unverified: on a verified account it would be a control whose only outcome is a refusal, and the endpoint refuses that case rather than sending a pointless email. The mint-token-and-send sequence now lives in one place. Registration and the email change already did the same three steps - supersede any outstanding token, mint a new one, send it - and this would have been a third copy. The step most likely to be dropped in a copy is the supersede, and it is the one that matters: without it an older message still sitting in the inbox goes on verifying. Anything that makes the server send mail on request is an abuse vector, so this is rate limited to three an hour, keyed on the customer id. That is tighter than either existing limiter and sidesteps #84's IPv6 problem entirely, since a signed-in caller has an identity better than an address to count against and cannot escape the bucket by moving within a delegated prefix. The refusal says the mail probably did send and to check the spam folder, which is both more useful and more honest than a bare 429. The claim that keying on customer id also solved test isolation was wrong, and the tests caught it. resetDb truncates with RESTART IDENTITY, so every integration test's first customer is id 1: three tests that each sent once left the fourth starting at its limit, and two tests failed on a 429 they never asked for. A "fresh customer per test" is a fresh row with a recycled identity. The limiter now has an explicit exported store the suite clears between tests, and the comment that claimed otherwise has been corrected rather than left to mislead the next reader. Verification: seven integration tests covering the unauthenticated refusal, the send, the new token invalidating the old one - asserted through the endpoint that would honour the stale link rather than by counting rows - the new link actually verifying, the already-verified refusal, the allowance stopping the fourth send rather than merely reporting it, and one customer's exhausted allowance leaving another's intact. Three end-to-end tests for the button, its confirmation and the message on the fourth click. The 33 integration tests across the three suites this touched all pass, as do the 199 backend unit tests. tsc clean on both sides, ESLint no errors. Closes #110 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
748c42628b |
Merge pull request 'feat: filter by Sold / Not sold / All on the storefront and the admin (#105)' (#129) from feature/105-sold-filter into main
Reviewed-on: #129 |
||
|
bermudalambandClaude Opus 5
|
32b3f616d9 |
feat: filter by Sold / Not sold / All on the storefront and the admin (#105)
Three decisions were taken before any code, and are recorded on the issue. The status filter is generalised to accept several values rather than gaining a second `sold` dimension beside it. "Not sold" is not a status: it is available-or-reserved on the storefront and includes pending in the admin, neither of which is one value. `?status=available,reserved` and `i.status = ANY($n::text[])` express that with one concept, so there is no way to write a contradiction like `?status=sold&sold=no`. A single status still parses to a list of one, which is how the admin's existing `?status=sold` keeps working untouched. The storefront now defaults to Not sold. That is a change in what every customer sees, not just a new control: the black SOLD ribbons leave the default view on a catalogue where they were evidence the shop sells things, and every storefront link shared so far quietly changes meaning. Accepted deliberately, with the default named in STOREFRONT_DEFAULT_STATUSES rather than implied by the absence of a parameter. The admin's four-way status dropdown is replaced rather than joined. That gives up isolating a single status: there is no longer a way to view only Reserved, or only Pending, and Not sold folds pending in with the rest. The pending workflow from #90 is the likeliest thing to miss it, and if it does, the fix is to put isolation back beside the preset rather than to remove the preset. The e2e test that covered "which is how Reserved is reached" is renamed and narrowed to what survives, rather than deleted. One thing the issue did not anticipate, found by a test rather than by reading. The favorites view deliberately showed sold favorites - "a favorite that has just sold is often exactly what the customer came to look at", and they have just been emailed to say so. Defaulting the storefront to Not sold reversed that silently and broke the test asserting it. Favorites therefore keep their own default of everything, on the server and in the control's displayed position, while an explicit ?status= still wins. That interaction is the kind a single-feature change quietly breaks, and it was caught only because the previous decision had been written down as an assertion. "All" still means different things in the two places, as the issue set out: available + reserved + sold on the storefront, all four in the admin. Pending remains unreachable from every public read - the storefront's unconditional exclusion clause is untouched - and the pending guard now checks every requested status rather than a single one, so `?status=available,pending` is refused for naming pending at all rather than accepted because the first name happened to be allowed. The control sits in the filter bar rather than in the drawer, since the default now hides sold pieces and a customer who never opens the drawer would otherwise have no way to know they exist. It is consequently excluded from the "Filters (N)" count, which describes the drawer, while still counting toward hasActiveFilters so that an empty result reads as "no items match these filters" with a way out rather than as an empty shop. Verification: 13 integration tests covering the default, each preset, the favorites exception and its override, and pending's unreachability under every accepted combination; 38 parser unit tests including multi-value parsing, an unknown name in a list being refused rather than dropped, and a list that names nothing; 6 new end-to-end tests for the storefront control, its URL round-trip, and the default staying out of the URL. 204 backend unit tests and 76 integration tests across the four affected suites pass. Two full end-to-end runs: 110 and 111 passing against the same 3 pre-existing failures, one run also showing a pending-publish failure that passes in isolation and did not recur - the cross-suite database contention filed as #116. tsc, ESLint and the production build are clean. Closes #105 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
e5ff980eae |
Merge pull request 'feat: tabs and a rendered preview for the email templates (#119)' (#128) from feature/119-email-template-tabs into main
Reviewed-on: #128 |
||
|
bermudalambandClaude Opus 5
|
1fa723bd19 |
feat: tabs and a rendered preview for the email templates (#119)
Follow-up to #92, which shipped the editable templates as a column of stacked cards. With six templates the cart reminder sat below five editors, so reaching it meant scrolling past all of them and which one you were editing was knowable only from a card title you had already scrolled past. They are tabs now, and the Default/Customised tag moves onto the tab label, so which templates have been changed is visible without opening each one. The larger gap was that there was no way to see what the email would look like. The editor is a markdown textarea; what gets sent is rendered HTML with placeholders substituted and, for the two favorite templates, a consent footer appended by the server. An admin editing copy could not tell whether the result read correctly. POST /api/admin/email-templates/:key/preview renders the draft in the editor rather than what is stored, so the effect of an edit is visible before committing to it. It renders on the server deliberately: renderTemplate is the only thing in the system that turns this markdown into HTML, and markdown-it is configured there with html: false, which is the control that stops an admin putting script into a customer's inbox. A renderer in the browser would be a second implementation of both, and a preview that disagreed with the mailer would be worse than none. It does not enforce required placeholders - saving refuses a body that dropped one, and previewing it is how the admin sees what they have done. The preview renders into a sandboxed iframe rather than through dangerouslySetInnerHTML. The markup is safe by construction, but an email is its own styling context: rendered inline, the admin theme's CSS would change how it looks and the preview would lie about the result. Sample values live beside the template definitions rather than in the route, so adding a placeholder puts the missing sample next to the change that needs it. A unit test asserts every available placeholder has one, because a missing sample renders a literal {{placeholder}} into the preview and teaches the admin their copy is broken when it is not. This also fixes a test that has been failing on main. email-templates.spec.ts located the Save button by filtering .ant-card for the template name, which matched an outer card containing every template's Save button - six of them - and died on a strict mode violation, taking two more tests with it as unrun. Only the active tab's editor is mounted now, so the labels are unambiguous and the filter is gone. Verification: eight end-to-end tests, four for editing and four for the preview, covering the draft being previewed rather than the stored copy, sample values replacing placeholders, raw HTML being escaped exactly as the mailer escapes it, and the consent footer appearing on a favorite template and not on a password reset. The full suite goes from 100 passed / 3 failed / 2 unrun to 112 passed / 2 failed / 0 unrun; the two that remain are the pre-existing password-reset failures that need a database on port 55432 and fail identically on main. 38 backend unit tests pass, tsc and ESLint are clean. Closes #119 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
f2ab4e6565 |
Merge pull request 'ci: fold the Tests workflow into SonarQube Analysis and delete it (#123)' (#127) from feature/123-consolidate-test-workflows into main
Reviewed-on: #127 |
||
|
bermudalambandClaude Opus 5
|
f7d85736dd |
ci: fold the Tests workflow into SonarQube Analysis and delete it (#123)
tests.yml and sonarqube.yml had identical triggers and ran the same tests. Where tests.yml ran test:unit:json and playwright test, sonarqube.yml ran test:unit:cov and test:e2e:cov: the same two suites, differing only in reporter and instrumentation. Every pull request therefore installed dependencies twice, migrated twice, built and started the backend twice, installed a Playwright browser twice and ran the whole end-to-end suite twice. With one runner the second copy did not run in parallel, it queued. Two things in tests.yml were not duplicates and are folded in rather than dropped. The summarize steps, which render pass and fail counts and name the failing tests, where sonarqube.yml offered only raw jest and Playwright output. And the continue-on-error plus fail-at-the-end pattern that lets those summaries render on a failing run at all: sonarqube.yml used to abort at the first failing step, so a broken unit test meant no end-to-end results, no coverage and no scan, which is one fact per run when a run costs many minutes. The trade-off is fast feedback. A broken unit test used to fail tests.yml in a couple of minutes; now it is reported when the whole pipeline finishes. That is worth it with one runner, where the fast job was queued behind the slow one anyway, and where a single run reporting unit results, end-to-end results, coverage and the scan together beats two runs each reporting a fragment. Worth revisiting if the runner count changes. Two details that would each have silently broken something: The coverage scripts do not emit the JSON the summarizers read - test:unit:cov has no --json, and test:e2e:cov does not set the JSON reporter. Both are appended at the step rather than baked into package.json, since coverage and a results file are only wanted together in this one place. The Playwright step uses --reporter=list,json so the log still names the failing test rather than only writing a file. The "Backend log" step was keyed on failure(). With continue-on-error the job is not in a failed state when it runs, so failure() would never fire and the log explaining an end-to-end failure would have gone unprinted exactly when it was wanted. It is now keyed on the step outcome. The header comment also records something the old comments obscured: the integration suite runs here on every push and pull request, despite backend-integration.yml describing it as manual. That quarantine only ever applied to tests.yml. Verification, run rather than assumed: both suites executed locally with the exact commands the workflow now uses. The unit run wrote unit-results.json alongside its coverage and summarize-jest.js rendered 199 passed from it. The end-to-end run wrote playwright-results.json and summarize-playwright.js rendered 100 passed, 3 failed, 2 skipped and named all three failures - a failing run, which is the case if: always() exists for. The three failures are pre-existing and fail identically on main. Parsing the remaining workflows confirms sonarqube.yml declares one job carrying both new step ids, lint.yml and backend-integration.yml are untouched, and tests.yml is gone. The two results files are added to .gitignore. CI never committed them, but the commands are now documented and runnable locally, which makes an accidental commit a matter of time. Closes #123 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
d82d0a8da3 |
Merge pull request 'feat(scripts): a PowerShell script to start the local environment (#125)' (#126) from feature/125-local-env-script into main
Reviewed-on: #126 |
||
|
bermudalamb
|
40b483fc30 |
feat(scripts): a PowerShell script to start the local environment (#125)
Bringing the app up locally was seven or eight commands in a particular order: a Postgres container on a port that is not blocked, the six environment variables the backend refuses to boot without, migrations, a TypeScript build, the backend, then Vite. None of it hard, all of it tedious, and documented nowhere outside CI workflows written for a Linux runner. scripts/start-local.ps1 does the sequence. It reuses an existing container rather than recreating one, skips npm install when node_modules is already there, leaves alone anything already listening on a port it wanted, and waits on pg_isready and a 200 from /api/config rather than sleeping a fixed number of seconds. -Fresh recreates the database, -Stop tears everything down by the process ids it recorded rather than by port, since killing by port would also kill whatever else happened to be listening. Two things this got wrong first time round, both found by running it rather than by reading it. $ErrorActionPreference = 'Stop' does not stop a PowerShell script when a native executable exits non-zero, only when a cmdlet throws. Every command here is node, npm or docker, so the first run printed a stack trace from a failed migration, carried straight on, and reported a healthy stack sitting on a database with no tables in it. That is the worst kind of wrong: a green summary over a broken environment. Native calls now go through Invoke-Checked, which tests $LASTEXITCODE and throws. The migration failed because node on the PATH was v18.16.1. node-pg-migrate pulls in an lru-cache that calls diagnostics_channel.tracingChannel, which does not exist before Node 20, and the failure surfaces as "(0 , U.tracingChannel) is not a function" from a minified file - which says nothing whatsoever about Node versions. The script now checks the major version first and says what to do about it, so the confusing crash becomes one clear line before anything else runs. The port default is 55500 rather than anything near 55432, which is reserved by Hyper-V on this machine. Docker's message when it cannot bind a reserved port does not mention reservations, so the failure path names the likely cause and prints the netsh command that lists the reserved ranges. Verification, all observed rather than assumed: the version guard was made to fire on Node 18 and produced the intended message. A -Fresh run on Node 24 applied all six migrations, and psql then showed thirteen tables where the broken run had none. /api/config and the storefront both answer 200. -Stop stopped both tracked processes and the container. A second run with the dev server already up detected it and left it alone rather than failing. .local/ holds the logs, pids and uploads, and is gitignored. Closes #125 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
46f88b4bbb |
Merge pull request 'feat(frontend): give order history a page of its own (#121)' (#124) from feature/121-orders-page-impl into main
Reviewed-on: #124 |
||
|
bermudalamb
|
9bb3cc86b6 |
feat(frontend): give order history a page of its own (#121)
The account modal had accumulated: a profile line, a name form, two collapsed panels for changing email and password, two consent switches, an order table and four controls. The table was the piece that fitted worst, being the only tabular data in a 700px dialog whose body is capped at 70vh. The scroll={{ x: 'max-content' }} already on it was a workaround for being in the wrong container rather than a layout choice.
It moves to /orders, an ordinary page in the same Routes block as /cart and /privacy, rather than another entry in MODAL_ROUTES. Order history is a list you read, like the cart, not a dialog you dismiss. A modal at /account/orders would have been the smaller change and was rejected: it inherits the same width and the same scroll cap, so it moves the table without giving it anything.
The page shell follows Cart.tsx, which is the established shape here: a Layout with a Header carrying Back to Shop and the title, and the same guard sending a signed-out visitor to /login. The account modal keeps a View order history button where the table used to be, because that is where a customer looks for it.
One thing changes rather than moves. The old effect caught a failed load with a toast and left orders as an empty array. The toast faded and the empty table did not, so from then on a customer whose request failed saw exactly what a customer with no orders saw, and the page asserted something false. Loading, failed and empty are now three distinct states, and the failed one carries a Retry: a transient failure would otherwise strand someone on a page that needs a full reload to recover.
OrdersBody sits at module level rather than nested inside Orders(). A function declared inside a component counts toward that component's cognitive complexity, which is what made Customers() hard to bring back under the threshold in #81.
The two assertions in account-modal.spec.ts that looked for the text "Order History" inside the modal are updated to look for the link, not deleted. They were the only coverage that the account view still offers any route to the orders, which is exactly what this change could have silently broken.
Verification, against a real backend and database: five new tests covering the signed-out redirect, the empty state, Back to Shop, the link from My Account, and that the page renders as a page rather than a modal over the storefront - that last one is what would catch /orders being added to MODAL_ROUTES and quietly undoing the change. The full suite goes from 100 to 105 passing with no new failures; the three that fail did so before this branch and fail identically on main. tsc and the production build are clean, ESLint reports no errors.
Closes #121
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalamb
|
6aa633d109 |
Merge pull request 'docs: design for moving Order History onto its own page (#121)' (#122) from feature/121-orders-page into main
Reviewed-on: #122 |
||
|
bermudalambandClaude Opus 5
|
d65eb7b981 |
docs: design for moving Order History onto its own page (#121)
Records why /orders is a page rather than another modal route. The app has both precedents: /account, /login and /register are in MODAL_ROUTES and render over a backdrop, while /cart and /privacy are ordinary pages. Order history is closer to the cart, a list you read rather than a dialog you dismiss. A modal at /account/orders was the smaller change and was rejected: it inherits the same 700px width and 70vh scroll cap, so it moves the table without giving it anything. Tabs inside the modal were rejected for the same reason, since they fix the scrolling and leave the cramping. The doc also records the one part of this that is a fix rather than a move. A failed load currently shows a toast and leaves the table empty, and the toast goes away while the empty table does not, so a customer whose request failed sees exactly what a customer with no orders sees. Loading, failed and empty become three distinct states, with a retry on the failed one. Per-order detail and server-side paging are written down as deliberately out of scope, so that leaving them out reads as a decision rather than an oversight. Refs #121 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
704aca0b84 |
Merge pull request 'feat(frontend): let a customer change their own name, password and email (#111)' (#120) from feature/111-account-ui into main
Reviewed-on: #120 |
||
|
bermudalamb
|
84db0e7ca2 |
feat(frontend): let a customer change their own name, password and email (#111)
The three endpoints have been on main since PR #113 with nothing calling them. This adds the UI, which is what the issue is actually about: its title is that PUT /api/customers/me has no caller. The name form sits open on the account view. Changing an email address or a password does not, because both are rare and deliberate, and leaving them expanded would push order history and the account controls below the fold for everyone who never uses them. They go in a collapse instead. Both of those carry a consequence the form cannot show. A new address has to be verified before it can be used to sign in or reset a password, and the old address is told that the change happened. A password change ends every other session. Each is stated above its fields rather than reported afterwards, so the surprise arrives while there is still a chance to back out. The email form asks for the current password. A live session is not enough to move the address a password reset would be sent to, which is the whole reason the server asks for it too. Server refusals are shown as they arrive rather than replaced with something generic: the message names which of the two passwords was wrong, or which name was left blank, and that is the only useful thing to say. The forms live in their own component rather than in Account.tsx. Three forms inline would have roughly doubled that component, and nested JSX bodies count toward the parent's cognitive complexity - the same thing that made Customers() hard to bring back under the threshold in #81. Verification, all against a real backend and database rather than mocks: six new end-to-end tests covering the name surviving a reload, a blank name being refused, the old password ceasing to work while the new one starts working, a wrong current password being refused for both the password and the email change, and an email change marking the account unverified again. The password test asserts the old credential no longer opens the account rather than that the form said something reassuring. The 21 existing account and auth tests still pass, and tsc and ESLint are clean. Closes #111 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
e234240440 |
Merge pull request 'ci: give linting its own workflow (#113)' (#114) from feature/113-split-lint-workflow into main
Reviewed-on: #114 |
||
|
bermudalambandClaude Opus 5
|
005146742e |
ci: give linting its own workflow (#113)
The lint job moves out of tests.yml into lint.yml, unchanged in what it runs. Lint is the fastest check in the pipeline and the one most often broken, and reporting it as one job among the suites made a lint failure and a test failure look alike at a glance. The split left two comments in the wrong place, both artifacts of the copy rather than of the intent. tests.yml kept the six-line note explaining the lint policy — that only defect-catching rules fail the build, that everything else warns, and why no --max-warnings flag appears. With the job gone it sat directly above backend-unit, where a reader would fairly take it as describing the unit tests. It has moved to lint.yml, with the job it actually describes. lint.yml inherited tests.yml's header about backend-integration living in its own manual workflow after it held the runner for three hours. That is worth saying where someone might expect integration tests to run; in a workflow that only runs ESLint it explains the absence of something nobody was looking for. Replaced with why this workflow exists at all. Verified by parsing both files rather than by reading them: lint.yml declares one job, lint; tests.yml declares backend-unit and frontend-e2e. No job was lost in the move and none is now declared twice, which is the failure a copy-and-delete edit invites. Refs #113 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
edf61abda4 |
Merge pull request 'feat(backend): let a customer change their own name, password and email (#111)' (#113) from feature/111-manage-account-details into main
SonarQube Analysis / sonarqube (push) Failing after 12m11s
Reviewed-on: #113 |
||
|
bermudalambandClaude Opus 5
|
7c3463650d |
feat(backend): let a customer change their own name, password and email (#111)
SonarQube Analysis / sonarqube (pull_request) Failing after 12m11s
Two of the three already existed on the backend and had no caller. PUT /api/customers/me updated the name; POST /api/customers/change-password already demanded the current password and enforced the eight-character minimum. Neither was reachable from the frontend, which is why the gap was easy to miss — the API looked finished. The name endpoint accepted empty values and wrote nulls, letting a customer clear fields registration refuses to let them skip. That is the same rule disagreeing with itself, so it now refuses each by name exactly as registration does. Changing a password now ends other sessions and keeps the one making the change. Reset already deleted every session for the customer, on the reasoning that a password is changed precisely when the old one may be known to someone else — change reached the opposite conclusion for no recorded reason, and a session opened with a leaked password outlived the change meant to lock it out. The current session is spared so the change does not eject the person making it. Changing the email address is new. It asks for the current password, because swapping the address a password reset goes to is how an account is taken over and a live session alone is not enough; that also matches what change-password already required. The address is normalised and validated, an address another account holds is refused with the same 409 as registration, and on success the row is marked unverified and any outstanding verification token superseded — one already sitting in the old inbox must not be able to verify the new address. Two emails then go out, to different places. Verification to the new address, and a notice to the old one naming what the address was changed to. The notice is the only thing that tells a real owner their account was taken, and one that does not say where the address went is nearly useless to someone checking whether it was them. Both sends happen after the row is written, never before, so a change that failed cannot produce mail saying it succeeded. That notice is a sixth template in #92's system, which cost a definition and a default body. The unit tests iterate every template, so its defaults were checked against its own required placeholder without writing a new test. Verified: 199 unit and 208 integration passing. The session test signs in on a second agent, changes the password on the first, and asserts the second is refused while the first still works — the property being claimed rather than the code path being executed. One of my own assertions was wrong on the way: /me answers an unauthenticated caller with 401 and an error body, not an empty one, and the frontend is what turns that into null. Refs #111 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |