Conversation
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
6ea3976 to
3202ac0
Compare
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
adumont-payplug
left a comment
There was a problem hiding this comment.
Review of the whole change (PHPUnit on the touched areas: pass, 14 tests; PHPStan level max and ECS: clean).
Verdict: request changes. Fix the two blocking items, make version mandatory, cap the amount input, and justify or trim composer.lock. Decide on the currency-exponent and Payment::amount points (fix or document).
Missing tests:
- The flush-fails-after-UPC-accepts path.
- Two requests racing against a slow flush.
- A missing
versionfield. - Oversized or zero-decimal amounts.
- The webhook path with an unknown operation id on a deferred payment.
Positives:
- The UPC 4011 and no-amount cancellation reasoning is documented in the code.
- A refusal records nothing.
- Routing is deliberately in YAML under the admin prefix, with a role check and an order/payment ownership check.
- Async failures are flagged and logged at
critical. - The FR/EN/IT translations are complete and consistent.
| $this->assertOperable($payment, $expectedVersion, AuthorizationDetails::OPERATION_CAPTURE); | ||
| $this->captureRecorded($payment, $amount); | ||
|
|
||
| if (0 === AuthorizationDetails::fromDetails($payment->getDetails())->remainingAmount()) { |
There was a problem hiding this comment.
[Medium] Partial cancellation followed by a capture of the rest completes the payment, but Payment::amount keeps the full amount.
Sylius's order payment-state resolver and RefundPlugin read Payment::amount, so the order can read "paid" for more than was captured. RefundPaymentProcessor correctly refunds the captured amount, but the RefundPlugin refundable total is unaffected. Please verify this, then either lower the payment amount or document and test the behaviour.
There was a problem hiding this comment.
I kept Payment::amount at the authorized amount because Sylius has no notion of a partial capture or void, and lowering it would change what RefundPlugin and the order payment-state resolver see, with a risk for other plugins. The captured amount is tracked in the payment details and RefundPaymentProcessor refunds it. Documented in the processor docblock and doc/authorized_payment.md (commit 8cce4ca).
- Flush recorded operation before releasing the lock so a second request cannot capture the same money twice - Warn merchant that money may have moved after an unexpected error instead of saying nothing changed - Require version on capture and cancel requests, a missing value now returns a 400 - Cap amount at 9 integer digits so an overflow is refused as invalid, not logged as critical - Convert amounts with UPC AmountHelper - Align cancel parameters order with capture - Log only status and execCode when no operationIds - Document lock TTL and why Payment amount is untouched
Sylius 2.3 drops knp-gaufrette-bundle, which RefundPlugin still loads, so the PHP 8.4 install step fails. Keep the matrix on 2.2 until RefundPlugin follows.
Description
Lets a merchant capture (in full or partially, possibly several times) and cancel (in full or partially) deferred-capture Hosted Fields payments from the admin order screen, built on UPC's
capturePayment()/cancelPayment()(PRE-3670).capture=falsewhen deferred capture is enabled. The Sylius payment staysauthorized, neverpaid(synchronous response, webhook or notification).AuthorizedPaymentOperationProcessor: eligibility rules, form lock + version (anti double-click / replay) and Sylius transitions (partial capture →authorized, last capture →completed, full cancellation →cancelled). A UPC refusal records nothing and changes no state.operationId; an asynchronous failure is recorded and logged atcritical.completetransition (cron, merchant listener): captures the remaining amount before completing./adminprefix) + admin role check (ROLE_ADMINISTRATION_ACCESS).payplug/unified-plugin-corebumped^1.1.2→^1.1.3.composer.lock: refreshed with a fullcomposer update, not only the UPC bump. The goal is UPC 1.3.0 (floor^1.1.3); other packages moved with it (Symfony 6.4.x patches, PHPUnit 9.6.37 and others).doctrine/ormis still inside its>=3.5 <3.7pin, at 3.6.9.doc/authorized_payment.md, entry inCHANGELOG.md.Motivation: Hosted Fields payments had no way to be captured later or partially; the merchant could only rely on the legacy redirected flow, which captures the whole authorized amount at once.
Related issue(s): Closes PRE-3673
Type of Change
Checklist
Code Quality
Testing
Security & Ops