🔴 Critical — honor-system payment on a public route. confirmGuestPayment transitions PENDING_PAYMENT → PROCESSING purely on the client's button click, with no Swish payment verification. Pre-existing on the authed confirmPayment, but this guest endpoint is JWT-less, so the only barrier is knowing the guest token. At minimum set amountPaid (and a paid_at) here so finance can reconcile against the Swish payout report; ideally gate the transition on a Swish Commerce payment request + callback.
🟢 No tests for the new guest methods. OrderServiceTest already covers createOrder/confirmPayment/cancelOrder/updatePendingOrder with the existing Mockito pattern. Please mirror that for createGuestOrder (plate uppercased, email lowercased, guestToken set via onCreate), getOrderByGuestToken (404 when userId is non-null — the defensive guard deserves a regression test of its own), and confirmGuestPayment (transition + notifyOrderProcessing invocation).
🟡 Missing DB-level orphan guard. The Order Javadoc asserts "Either userId or guestToken is set; never both, never neither" — but only onCreate() enforces this in Java. A stray INSERT (admin tooling, future script) can violate it. Suggest adding after the column additions:
Verdict: Well-structured guest-checkout scaffold — validation parity with the authed flow, defensive token lifecycle (only guest orders get a token, lookups refuse to serve user-owned orders), and a partial-unique-index migration that is backfill-safe. Frontend type-checks pass per the PR. Two blockers worth resolving before merge: the payment-confirmation path is honor-system (carried over from the authed path, but now on a public, JWT-less route), and the three new OrderService methods have no tests.
💡 Suggestion: Good resilience pattern — the silent catch degrades gracefully. Consider adding a test for this path: mockToDataURL.mockRejectedValue(...) → assert the Swish payment link and manual fallback still render.
💡 Suggestion: The comment says "a leading +" but /[\s+]/g strips all + characters anywhere in the string, not just a leading one. This is harmless for real phone numbers, but the comment slightly misrepresents the behaviour. Consider number.replace(/^\+/, '').replace(/\s/g, '') or adjusting the comment to say "any + characters".
Verdict: LGTM — solid fix for a real production bug. QR params now match the ISO/IEC 18004 spec, and the error-handling refactor is a good resilience improvement.
Here's the payment page spun up and screenshotted 👇
What's visible:
- QR code —…
⚠️ QR generation failure masks the payment UI
Advisory review — looks good with one warning
Root cause
The new E2E test shows QR code for desktop scanning used plate JKL012 — the same plate seeded as a processing order in V7__seed_processing_order.sql and used by…