More backend issues resolved #3850
Conversation
📝 WalkthroughWalkthroughThis PR modifies checkout payment handling to capture DummyResponse transaction references and persist them, removes non-redirect Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks❌ Failed checks (1 warning)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
tests/**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
tests/Unit/**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/Http/Requests/**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (9)📓 Common learnings📚 Learning: 2025-09-13T11:30:21.765ZApplied to files:
📚 Learning: 2025-11-17T11:36:52.888ZApplied to files:
📚 Learning: 2025-09-21T20:19:42.669ZApplied to files:
📚 Learning: 2025-09-21T20:08:02.626ZApplied to files:
📚 Learning: 2025-09-21T20:07:17.804ZApplied to files:
📚 Learning: 2025-09-13T13:58:16.970ZApplied to files:
📚 Learning: 2025-09-13T13:58:16.970ZApplied to files:
📚 Learning: 2025-08-12T11:31:09.853ZApplied to files:
🧬 Code graph analysis (2)tests/Webshop/OrderManagement/OrderControllerMarkAsDeliveredTest.php (1)
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.php (2)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/Http/Resources/Shop/CheckoutResource.php (1)
19-26: Newcomplete_urlfield is fine; ensure all call sites and TS types are aligned.Adding
public readonly ?string $complete_url = nullfits the Data resource and the updated controller flow. Please ensure:
- All
CheckoutResourceinstantiations use named arguments or have been updated for the new parameter position.- The generated/checked TypeScript
CheckoutResourcetype also includescomplete_urlso the frontend model matches this payload.app/Http/Controllers/Shop/CheckoutController.php (1)
112-118: Includingorderin the generic process response is reasonable; consider payload impact.Always attaching
OrderResource::fromModel($order)(even for redirect flows) is useful, but it does increase response size. If the order has many items, ensure this remains acceptable for the checkout entrypoint; otherwise, consider returning a lighter representation here if needed later.app/Actions/Shop/CheckoutService.php (1)
106-124: Non‑redirect success path and earlytransaction_idoverwrite need cross‑flow validation.The new logic for non‑redirect responses:
- Stores metadata for
DummyResponsein the session, mirroring redirect flows.- Sets
$order->transaction_idto$response->getTransactionReference()and saves before finalize/completePayment()runs.This is coherent with the Dummy/“no redirect” flow and works with
CheckoutController::process()buildingcomplete_urlafter$order->refresh(). However, it does move the overwrite oftransaction_id(from UUID to gateway reference) earlier than the previously documented behavior, at least for non‑redirect flows.Please verify that:
- No other code still relies on
transaction_idremaining the original UUID betweenprocess()andfinalize()for these flows.- The Dummy gateway’s
completePurchase()indeed expects/uses the stored metadata as you’re now persisting it.If everything is aligned, this centralizes behavior nicely for both redirect and dummy providers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Actions/Shop/CheckoutService.php(2 hunks)app/Http/Controllers/Shop/CheckoutController.php(1 hunks)app/Http/Requests/Order/MarkAsDeliveredOrderRequest.php(2 hunks)app/Http/Resources/Shop/CheckoutResource.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case
Apply PSR-4 coding standard
in_array() should be used with true as the third parameter for strict comparison
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty()
Use the moneyphp/money library for handling monetary values and currencies in PHP
Never use floats or doubles to represent monetary values; use integers to represent the smallest currency unit (e.g., cents for USD)
Files:
app/Http/Resources/Shop/CheckoutResource.phpapp/Http/Controllers/Shop/CheckoutController.phpapp/Http/Requests/Order/MarkAsDeliveredOrderRequest.phpapp/Actions/Shop/CheckoutService.php
**/Http/Resources/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Resource classes should extend from Spatie Data instead of JsonResource
Files:
app/Http/Resources/Shop/CheckoutResource.php
**/Http/Requests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In Requests, if a user is provided by the query parameter, it should be placed in $this->user2; $this->user is reserved for the user making the request
Files:
app/Http/Requests/Order/MarkAsDeliveredOrderRequest.php
🧠 Learnings (10)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3810
File: app/Actions/Shop/CheckoutService.php:70-72
Timestamp: 2025-11-17T11:36:52.888Z
Learning: For Mollie and Stripe payment providers in the CheckoutService, the payment flow uses redirects rather than POST callbacks. When the user completes payment, they are redirected back to Lychee with query parameters in the URL (not POST data). The purchase parameters stored in the session are used with completePurchase() to verify the payment, and any necessary callback data comes from the query parameters in the redirect URL, which Omnipay handles automatically.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3702
File: app/Actions/Shop/CheckoutService.php:143-147
Timestamp: 2025-09-21T20:19:42.669Z
Learning: In the CheckoutService, the Order.transaction_id field is intentionally overwritten with the gateway's transaction reference during payment completion via Order::markAsPaid(). This is the intended design per ildyria's confirmation, despite the transaction_id originally being a UUID used for return/cancel URLs.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3702
File: app/Actions/Shop/CheckoutService.php:143-147
Timestamp: 2025-09-21T20:07:17.804Z
Learning: In the CheckoutService, the Order.transaction_id field is intentionally overwritten with the gateway's transaction reference during payment completion via Order::markAsPaid(). This is the intended design despite the transaction_id originally being a UUID used for return/cancel URLs.
📚 Learning: 2025-10-03T10:58:59.321Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3732
File: app/Http/Resources/Shop/OrderResource.php:62-64
Timestamp: 2025-10-03T10:58:59.321Z
Learning: In app/Http/Resources/Shop/OrderResource.php, the can_process_payment flag is intentionally set to false in order listings (via OrderService::getAll()) because items are explicitly excluded with without(['items']). This is by design: listings are lightweight, and the actual payment capability is only computed when fetching individual orders with full item data loaded.
Applied to files:
app/Http/Controllers/Shop/CheckoutController.php
📚 Learning: 2025-11-17T11:36:52.888Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3810
File: app/Actions/Shop/CheckoutService.php:70-72
Timestamp: 2025-11-17T11:36:52.888Z
Learning: For Mollie and Stripe payment providers in the CheckoutService, the payment flow uses redirects rather than POST callbacks. When the user completes payment, they are redirected back to Lychee with query parameters in the URL (not POST data). The purchase parameters stored in the session are used with completePurchase() to verify the payment, and any necessary callback data comes from the query parameters in the redirect URL, which Omnipay handles automatically.
Applied to files:
app/Http/Controllers/Shop/CheckoutController.phpapp/Actions/Shop/CheckoutService.php
📚 Learning: 2025-11-23T23:44:13.346Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3810
File: app/Http/Controllers/Shop/CheckoutController.php:154-180
Timestamp: 2025-11-23T23:44:13.346Z
Learning: In the Lychee webshop, the OrderCompleted event should only be dispatched after payment is confirmed. The OFFLINE payment status (PaymentStatusType::OFFLINE) indicates that payment has not yet been confirmed, so OrderCompleted should NOT be dispatched when an order is marked as OFFLINE. The event is dispatched in the finalize() method for online payments after status reaches COMPLETED, and for offline orders, the event should be dispatched when the order is later confirmed as paid (likely through the markAsPaid transition).
Applied to files:
app/Http/Controllers/Shop/CheckoutController.php
📚 Learning: 2025-09-21T20:08:02.626Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3702
File: app/Models/Order.php:123-137
Timestamp: 2025-09-21T20:08:02.626Z
Learning: In the Shop system Order model, the markAsPaid method intentionally overwrites the internal transaction_id with the payment provider's transaction reference when completing a payment. This is the intended behavior per ildyria's design decision.
Applied to files:
app/Http/Controllers/Shop/CheckoutController.phpapp/Actions/Shop/CheckoutService.php
📚 Learning: 2025-08-12T11:31:09.853Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3605
File: docs/Lifecycle-of-a-request-photo-upload.md:144-151
Timestamp: 2025-08-12T11:31:09.853Z
Learning: In Lychee, the BaseApiRequest class has been modified to change Laravel's default execution order - processValidatedValues() runs BEFORE authorize(), not after. This means that in Lychee request classes, $this->album and other processed properties are available during authorization checks, unlike standard Laravel behavior.
Applied to files:
app/Http/Requests/Order/MarkAsDeliveredOrderRequest.php
📚 Learning: 2025-09-21T20:19:42.669Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3702
File: app/Actions/Shop/CheckoutService.php:143-147
Timestamp: 2025-09-21T20:19:42.669Z
Learning: In the CheckoutService, the Order.transaction_id field is intentionally overwritten with the gateway's transaction reference during payment completion via Order::markAsPaid(). This is the intended design per ildyria's confirmation, despite the transaction_id originally being a UUID used for return/cancel URLs.
Applied to files:
app/Actions/Shop/CheckoutService.php
📚 Learning: 2025-09-21T20:07:17.804Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3702
File: app/Actions/Shop/CheckoutService.php:143-147
Timestamp: 2025-09-21T20:07:17.804Z
Learning: In the CheckoutService, the Order.transaction_id field is intentionally overwritten with the gateway's transaction reference during payment completion via Order::markAsPaid(). This is the intended design despite the transaction_id originally being a UUID used for return/cancel URLs.
Applied to files:
app/Actions/Shop/CheckoutService.php
📚 Learning: 2025-09-13T13:58:16.970Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Actions/Shop/CheckoutService.php:50-68
Timestamp: 2025-09-13T13:58:16.970Z
Learning: In the Shop system, Order models are always created via OrderService::createOrder() which automatically assigns a UUID to the transaction_id field during creation (line 42: 'transaction_id' => Str::uuid()). Therefore, transaction_id is never null or empty when orders reach the CheckoutService for payment processing.
Applied to files:
app/Actions/Shop/CheckoutService.php
📚 Learning: 2025-09-13T13:58:16.970Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Actions/Shop/CheckoutService.php:50-68
Timestamp: 2025-09-13T13:58:16.970Z
Learning: In the Shop system, Order models are always created via OrderService::createOrder() which automatically assigns a UUID to the transaction_id field during creation. Therefore, transaction_id is never null or empty when orders reach the CheckoutService for payment processing.
Applied to files:
app/Actions/Shop/CheckoutService.php
🧬 Code graph analysis (2)
app/Http/Controllers/Shop/CheckoutController.php (2)
resources/js/lychee.d.ts (2)
CheckoutResource(906-912)OrderResource(943-956)app/Http/Resources/Shop/OrderResource.php (1)
fromModel(48-72)
app/Http/Requests/Order/MarkAsDeliveredOrderRequest.php (4)
app/Models/OrderItem.php (1)
order(103-106)app/Models/Order.php (1)
items(121-124)app/Eloquent/FixedQueryBuilderTrait.php (2)
where(66-76)orWhere(283-293)resources/js/lychee.d.ts (1)
PaymentStatusType(108-108)
🔇 Additional comments (6)
app/Http/Requests/Order/MarkAsDeliveredOrderRequest.php (2)
37-44: Authorization item check remains correct and strict.The
array_diff(array_column($this->items, 'id'), $this->order->items->pluck('id')->toArray()) === 0condition still cleanly enforces that all provided item IDs belong to the order; no issues here.
64-73: Status filter change to COMPLETED/CLOSED looks coherent; just confirm business intent.Wrapping the status checks in a closure so the query becomes
(status = COMPLETED OR status = CLOSED) AND id = :order_idis correct and avoids precedence pitfalls. Given this request “marks an order as delivered”, please double‑check that allowing both COMPLETED and CLOSED for retrieval matches the intended lifecycle (i.e. no need to restrict to one of them only).app/Http/Controllers/Shop/CheckoutController.php (1)
100-109: Non‑redirect success branch is consistent with the finalize flow.Returning a
CheckoutResourcewithis_redirect = false, acomplete_urlpointing toshop.checkout.return, and a refreshedOrderResourcekeeps the non‑redirect (e.g. dummy) flow aligned with the existing finalize logic. Just confirm that the frontend is now expected to always followcomplete_urlon this branch instead of relying on the previously dispatched event alone.app/Actions/Shop/CheckoutService.php (3)
23-25: DummyResponse import appears correct; verify against installed Omnipay Dummy version.Using
Omnipay\Dummy\Message\Responsealiased asDummyResponseis the right shape for type checks here; just confirm it matches the actual class name in the omnipay/dummy version installed in this project.
84-106: Redirect branch metadata handling for FetchTransactionResponse looks sound.Capturing
$response->getMetadata(), injectingtransactionReference, and persisting undermetadata.{order_id}in the session giveshandlePaymentReturn()what it needs forcompletePurchase(). The explicitRedirectResponseInterfaceguard is also a good safety check.
134-148: Exception handling and FAILED status update look appropriate.Catching gateway exceptions, logging with order and provider context, transitioning the order to
FAILED, and returning a generic user‑facing message is a good balance between observability and not leaking internal error details.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
| redirect_url: $redirect_url, | ||
| ); | ||
| } elseif ($response->isSuccessful()) { | ||
| if ($response instanceof DummyResponse) { |
There was a problem hiding this comment.
For now, only Dummy is successful directly. Later consider the other cases but I expect most integration will have a redirection.
|
|
||
| // Payment was successful | ||
| $this->completePayment($order, $response); | ||
| $order->transaction_id = $response->getTransactionReference(); |
There was a problem hiding this comment.
We only save the transaction id to make sure that we do a proper finalization.
| // If we have a success directly without redirection mark order as completed | ||
| if ($result->is_success && !$result->is_redirect) { | ||
| OrderCompleted::dispatchIf(Configs::getValueAsBool('webshop_auto_fulfill_enabled'), $order->id); | ||
| // This is a success we now need to complete the order. |
There was a problem hiding this comment.
This way the OrderCompleted event is only dispatched in the finalize step, not in the processing step.
Furthermore that clarifies that we have complete_url and redirect_url, showing different intent.
| $order_id = intval($values['order_id'] ?? null); | ||
| $this->order = Order::query() | ||
| ->where('status', '=', PaymentStatusType::COMPLETED) | ||
| ->where(fn ($query) => $query |
There was a problem hiding this comment.
Mark as delivered can be used to provide links/update links even when the request is closed. hence the change.
JasonMillward
left a comment
JasonMillward
left a comment
There was a problem hiding this comment.
Appreciate the comments
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.