Repository navigation
feat(catalog): rename, reorder and remove a product variant - #22
Merged
Merged
Conversation
product-variants.md left the three out on purpose: "creating is safe; removing needs a policy for what was already sold, and a policy is a decision, not a detail". This writes the policy. Removal hits three walls, and only one of them is negotiable. The last variant never goes, because every product has at least one size and one with none is unbuyable. A size somebody bought never goes, because order_items RESTRICTs it -- and when that wall closes, RENAMING is the way out, which is what actually unblocks the twelve AVESSO pieces stuck on `Unico`. The third wall is the one the schema does not settle. CartItem.variant cascades, so creating the route makes a silent deletion real. Refusing while a cart holds the size would hand an abandoned bag a veto over the catalogue -- this cart has no TTL, no reservation and no expiry. Letting it cascade shrinks somebody's bag with no explanation. So: refuse by default, report the count, and require a confirmation of two halves -- discardCartLines authorises the destruction, expectedCartLineCount confirms its size. The count is retaken under a row lock inside the transaction; if it moved, nothing is deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e else The suite TRUNCATEs the tables it touches, and it had no database of its own: DATABASE_URL points at the development project, which is also the one serving the published store. There are only two Supabase projects and no third to spend, so the answer is a dedicated SCHEMA inside the development database -- built by `pnpm e2e:setup` from the real migrations, in order, then the production seed, so the test database cannot drift into a shape production never had. Isolation rides on `options=-c search_path=<schema>` in E2E_DATABASE_URL, and the trap that cost this two attempts is that search_path moves raw SQL and does NOT move Prisma. With no schema on the driver adapter the query compiler qualifies everything with `public` whatever the path says. The first build reported "3 roles, 14 permissions" while the test schema sat empty -- it had seeded the development database and read its own answer back. A suite running that way truncates empty test tables and then reads and writes real data, green the whole time. So the schema is parsed out of the URL ONCE, in connection-schema.ts, and feeds both halves; they cannot disagree by construction. Production URLs carry no `options`, so the parser returns undefined and the generated SQL is byte-identical to before. Two guards, because a mistake here is unrecoverable: E2E_DATABASE_URL is required and never falls back to DATABASE_URL, and after connecting the suite asks the server `SELECT current_schema()` and aborts on `public`. The parsed string is a claim; the server's answer is the fact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes the three variant operations v1 left out, and with them the only item that blocked real data: the twelve AVESSO pieces carry one variant labelled `Unico` that the #19 migration created, and the store sells clothes with no size until that can be changed. PATCH /products/{id}/variants/order products.update PATCH /products/{id}/variants/{variantId} products.update DELETE /products/{id}/variants/{variantId} products.delete Reorder restates the whole list -- a partial one does not say where the sizes it omitted should go -- and writes the positions in one transaction. Rename cannot touch history by construction: order_items.variant_label is a snapshot, which is exactly why it exists, and it is also the way out when a sold size can never be deleted. DELETE is gated on products.delete rather than products.update, unlike the two PATCHes. It changes nothing today, since only admin holds either; it matters the day somebody gets products.update to fix labels and stock, and should not thereby be able to discard sold-out sizes and other people's cart lines. The removal policy is docs/specs/variant-management.md. Everything destructive runs in one transaction that opens by locking the product's variant rows: inserting a cart line takes FOR KEY SHARE on the variant it points at, which FOR UPDATE conflicts with, so no cart can slip in between the recount and the delete, and two concurrent removals cannot both read "two variants left" and both delete one. The cart lines are then deleted by this code, not by the FK cascade -- the cascade stays as a referential net, but a rule that deletes customer data has to be a line somebody can read and test. RESTRICT firing on a sold size becomes the same 409 as the pre-check, never a 500. discardCartLines is this repo's first boolean query parameter, and `@Type(() => Boolean)` would have inverted it: Boolean('false') is true, so the value asking for the protection would have switched it off. It gets a strict transform, and an e2e that pins it. Also fixes a latent bug the removal route makes easy to hit: addVariant defaulted a new position to the COUNT of variants rather than one past the highest, so with sparse positions -- which callers could already produce, and deleting from the middle now produces routinely -- a new size landed on top of an existing one and a UUID tiebreaker decided the display order. 492 unit tests, 206 e2e in 9 files, all green. openapi.json regenerated: 39 operations to 42. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It was resolving through @prisma/adapter-pg's own dependency, which works under a hoisted node_modules and does not under pnpm's strict layout: CI failed typecheck with "Cannot find module 'pg'" on a script that runs fine locally. A direct import needs a direct dependency. devDependencies rather than dependencies: nothing in src/ imports pg -- the application reaches Postgres through the Prisma adapter -- and prisma/e2e-schema.ts is a development tool. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Item 1 of the order suggested in
docs/admin-api.md— the only one thatblocks real data today. The twelve AVESSO pieces carry a single variant
labelled
Únicothat the #19 migration created, and the store sells clotheswith no size until that can be changed.
Stacked on #21 (docs only). Merge that first and this rebases clean.
The routes
PATCH/products/{id}/variants/orderproducts.updatePATCH/products/{id}/variants/{variantId}products.updateDELETE/products/{id}/variants/{variantId}products.delete39 operations → 42. Spec:
docs/specs/variant-management.md.DELETEasks forproducts.deleterather thanproducts.updateon purpose.It changes nothing today — only
adminholds either — and it matters the daysomeone gets
products.updateto fix labels and stock, and should not therebybe able to discard sold-out sizes and other people's cart lines.
The removal policy, which is most of the spec
Three walls, and only the third is negotiable.
The last variant never goes. Every product has at least one size;
one with none is unbuyable. No parameter overrides it — archive the product
instead.
A sold size never goes.
order_itemsRESTRICTs it. The route turnsthat refusal into a
409with a sentence rather than a leaked500— andwhen it closes, renaming is the way out, which still unblocks the twelve
even in the worst case, because
variantLabelis a snapshot.Carts do not veto, but nothing is discarded by accident.
CartItemcascades, so creating this route makes a silent deletion real. Refusing
while a cart holds the size would hand an abandoned bag a veto over the
catalogue — this cart has no TTL, no reservation, no expiry, and nothing
ever clears it. Letting it cascade shrinks somebody's bag with no
explanation.
So: refuse by default with the count, and require a confirmation of two
halves —
discardCartLines=trueauthorises the destruction,expectedCartLineCount=Nconfirms its size. If the number moved ineither direction, nothing is deleted and the
409carries the new one.Concurrency
The destructive path runs in one transaction that opens by locking the
product's variant rows. Inserting a cart line takes
FOR KEY SHAREon thevariant it points at, and
FOR UPDATEconflicts with that — so no cart canslip in between the recount and the delete, and two concurrent removals cannot
both read "two variants left" and both delete one. The cart lines are then
deleted by this code, not by the FK cascade: the cascade stays as a
referential net, but a rule that deletes customer data should be a line
somebody can read and test.
Two things found on the way
discardCartLinesis this repo's first boolean query parameter, and theobvious implementation inverts it.
@Type(() => Boolean)runsBoolean('false'), which istrue— the value asking for the protectionwould have switched it off. Strict transform, with an e2e pinning it.
addVariantdefaulted a new position to the count of variants, not onepast the highest. Sparse positions were already reachable, and deleting
from the middle produces them routinely; a new size then landed on top of an
existing one and a UUID tiebreaker decided display order. Fixed.
The e2e had nowhere safe to run
The suite TRUNCATEs, and
DATABASE_URLpoints at the development project —which is also the one serving the published store. With no third Supabase
project to spend, it now runs in a dedicated schema built by
pnpm e2e:setupfrom the real migrations plus the production seed.Worth a look in review:
search_pathmoves raw SQL and does not movePrisma. With no schema on the driver adapter the query compiler qualifies
everything with
public. The first build of this reported success while thetest schema sat empty — it had seeded the development database and read its
own answer back. A suite in that state truncates empty test tables and then
reads and writes real data, green the whole time. The schema is now parsed out
of the URL once and feeds both halves, and the suite refuses to start without
E2E_DATABASE_URLor if the server answerspublic.Verified
492 unit tests (24 new) · 206 e2e in 9 files (17 new, no regressions in the
existing 189) ·
lint:check,typecheck,build·openapi:generatereproduces the committed document.
Three e2e assertions only a real database can falsify: a sold size refused
with the order intact, a cart line disappearing with the variant and the
owner's cart re-totalling, and the recount aborting when a fourth cart arrives
between the warning and the confirmation.
Not in this PR
docs/status.mdstill says "39 rotas" and "468 testes unitários". Left alonebecause
chore/stale-test-countsis already in flight on exactly that.🤖 Generated with Claude Code