Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are a few correctness/robustness issues around idempotency and API validation (notably mark_paid mail-marker race handling and overly strict schema validation) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the print-shop order lifecycle so offline_invoice orders behave like Stripe orders by starting in pending_payment, requiring a manual mark_paid confirmation that records a payment reference, and ensuring paid-confirmation mails are idempotent.
Changes:
- Start all print orders in
pending_paymentand requirepaymentReferencewhen markingoffline_invoiceorders as paid. - Add
paymentReferenceto the data model, API typing, and Studio order-detail UI (prompt + display). - Set the
mails_sent_paidmarker intransitionOrder()to prevent the mail sweeper from re-sending “paid” emails.
File summaries
| File | Description |
|---|---|
| apps/frontend/src/lib/i18n/it.ts | Adds Italian UI strings for payment reference prompt/label. |
| apps/frontend/src/lib/i18n/fi.ts | Adds Finnish UI strings for payment reference prompt/label. |
| apps/frontend/src/lib/i18n/en.ts | Adds English UI strings for payment reference prompt/label. |
| apps/frontend/src/lib/i18n/de.ts | Adds German UI strings for payment reference prompt/label. |
| apps/frontend/src/lib/api.ts | Extends API typings to include paymentReference in transitions and order detail. |
| apps/frontend/src/app/studio/print-shop/orders/[id]/page.tsx | Prompts for and displays paymentReference for offline_invoice orders. |
| apps/api/src/services/print/orders.ts | Removes offline-invoice auto-paid path; validates/stores payment reference; fixes paid-mail marker sequencing. |
| apps/api/src/services/print/orders.test.ts | Adds unit tests for isMissingRequiredPaymentReference. |
| apps/api/src/services/print-mail-sweeper.ts | Updates sweeper docs to reflect new offline-invoice mail flow. |
| apps/api/src/routes/print-shop.ts | Accepts optional paymentReference in transitions endpoint request body. |
| apps/api/prisma/schema.prisma | Adds nullable paymentReference field and updates lifecycle docs. |
| apps/api/prisma/migrations/20260908090000_print_order_payment_reference/migration.sql | Adds paymentReference column to print_orders. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Required by transitionOrder() when marking an offline_invoice | ||
| // order paid — validated there, not here, since it depends on the | ||
| // order's paymentMode. | ||
| paymentReference: z.string().min(1).max(200).optional(), | ||
| }); |
| if (t.type === "mark_paid") { | ||
| // Marker VOR dem Mail-Versand setzen (nicht danach) — sonst findet | ||
| // der print-mail-sweeper (laeuft alle 30s) dieselbe Order noch | ||
| // ohne Marker und verschickt die 'paid'-Mail ein zweites Mal. | ||
| await prisma.printOrderEvent.create({ | ||
| data: { | ||
| printOrderId: orderId, | ||
| eventType: "mails_sent_paid", | ||
| actor: "system", | ||
| data: { trigger: "mark_paid_transition" } as never, | ||
| }, | ||
| }); | ||
| void sendOrderMails(orderId, "paid").catch((err) => | ||
| logger.warn({ err, orderId }, "print.order.mail_failed") | ||
| ); |
| /** Finds paid orders with no 'mails_sent_paid' event yet — in practice | ||
| * only Stripe-webhook-triggered ones, since every other path to 'paid' | ||
| * (transitionOrder()'s mark_paid branch) sets that marker itself. */ |
| throw new Error( | ||
| "paymentReference is required to mark an offline_invoice order as paid" | ||
| ); |
…ine invoice orders offline_invoice checkouts previously landed straight on 'paid' the instant a guest submitted the cart, with a mail sent inline from createOrder(). They now start 'pending_payment' like stripe_connect orders, and require a studio staff member to confirm payment via the existing mark_paid transition with a payment reference (invoice/receipt number) — the mail then fires from transitionOrder() instead. Also fixes a latent double-mail bug: transitionOrder()'s mark_paid branch sent the 'paid' mail directly but never set the mails_sent_paid marker, so the print-mail-sweeper (which polls for un-marked paid orders) would re-send the same mail ~30s later. That path is now the primary route to 'paid' for every offline_invoice order, so the fix matters far more than when it was Stripe-manual-confirmation-only. Closes markusthiel#35 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- mark_paid's mails_sent_paid marker was inserted unconditionally, so two (near-)concurrent mark_paid calls on the same order — a double-click, a retried request — could both create their own marker and both send the "paid" confirmation mail. Now uses the same transactional existence-check-then-create pattern the mail sweeper already relies on: only whichever call actually creates the marker sends mail. Verified live by firing 5 concurrent mark_paid calls at the same order — exactly one mails_sent_paid marker and one email resulted, down from up to 5. - The transitions route's paymentReference schema had .min(1), which rejects paymentReference: "" for every transition type, not just the offline_invoice mark_paid case that actually needs a non-blank value. The real requirement is already enforced downstream in transitionOrder() via isMissingRequiredPaymentReference(); the route schema now only caps length. - Fixed a stale comment in print-mail-sweeper.ts's runOnce() claiming the query filters on a mark_paid event — it doesn't; status='paid' plus the absent mails_sent_paid marker is the only real criterion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The last Copilot finding on this PR: the mark_paid validation error was the only English string in transitionOrder(), inconsistent with every other error in this service (Order nicht gefunden, Cart leer, etc.) and returned to the client as-is via the route. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ff4dda8 to
6b230f8
Compare
|
Rebased onto current The conflict: The last open Copilot finding: the Verified after rebase: Ready for review. |
Summary
offline_invoicecheckouts (self-print, no Stripe Connect) previously landed straight onpaidthe instant a guest submitted the cart — no manual confirmation, no way to record a receipt/invoice reference. They now startpending_payment, exactly likestripe_connectorders.offline_invoiceorder now requires apaymentReference(added as a nullable column onPrintOrder), passed on the existingmark_paidtransition (POST /print-shop/orders/:id/transitions).stripe_connectorders are unaffected — they keep usingstripeChargeIdas their natural reference, no new input required.offline_invoice; the stored reference is shown next to the payment-mode label.transitionOrder()'smark_paidbranch sent the "paid" confirmation mail directly but never set themails_sent_paididempotency marker that theprint-mail-sweeperrelies on, so the same mail would be re-sent ~30s later by the sweeper. This mattered little whileoffline_invoicebypassedmark_paidentirely, but now everyoffline_invoiceorder goes through this exact path, so it's fixed as part of this change (marker is now set before the mail fires, mirroring the pattern the old inlinecreateOrder()code already used).No new order-status values — reuses the existing
pending_payment→paidstates andtransitionOrderstate machine (apps/api/src/services/print/orders.ts).Closes #35
Test plan
npx tsc --noEmitclean inapps/apiandapps/frontendnpx vitest run— 195/195 tests pass (25 files), including 4 new tests for the pureisMissingRequiredPaymentReferencehelpernpm run check:i18nclean (new keys added to all 4 locales)createOrder/transitionOrderdirectly:offline_invoiceorder created →pending_payment,paidAt: nullmark_paidwithoutpaymentReferenceon that order → rejected, status unchangedmark_paidwithpaymentReference: "INV-2026-042"→paid,paidAtset, reference storedmails_sent_paidevent recorded (confirms the double-mail fix — checked the actual mail log, one guest + one studio mail, not two)stripe_connectorder → still startspending_payment;mark_paidstill succeeds with no reference required🤖 Generated with Claude Code