From 027014750be3ecdf70beb372d80c85dc7283b690 Mon Sep 17 00:00:00 2001 From: Jules Nsenda Date: Tue, 29 Sep 2026 15:24:37 +0200 Subject: [PATCH 1/9] feat(inline): send the placed order's id as metadata.orderId Core calls afterPlaceOrder() with no arguments, so the renderer never knew which order a popup transaction was for. Capture the entity id the payment-information response resolves with, and send it so the webhook can settle the right order when several share a quote (issue #69). Co-Authored-By: Claude Opus 5.5 --- .../method-renderer/pstk_paystack-method.js | 84 ++++++++++++------- 1 file changed, 54 insertions(+), 30 deletions(-) diff --git a/view/frontend/web/js/view/payment/method-renderer/pstk_paystack-method.js b/view/frontend/web/js/view/payment/method-renderer/pstk_paystack-method.js index fb115fc..217407c 100644 --- a/view/frontend/web/js/view/payment/method-renderer/pstk_paystack-method.js +++ b/view/frontend/web/js/view/payment/method-renderer/pstk_paystack-method.js @@ -46,6 +46,23 @@ define( window.location.replace(mageUrl.build(url)); }, + /** + * Core calls afterPlaceOrder() with no arguments, so capture the + * placed order's entity id (the payment-information response) here. + * The webhook uses it as metadata.orderId to pick the right order + * when several share a quote (issue #69). Obsolete once inline + * moves to server-side initialize. + * + * @override + */ + getPlaceOrderDeferredObject: function () { + var self = this; + this.paystackOrderId = null; + return this._super().done(function (orderId) { + self.paystackOrderId = /^[1-9]\d*$/.test(String(orderId)) ? String(orderId) : null; + }); + }, + /** * @override */ @@ -98,6 +115,9 @@ define( var streetAddress = [streetLines[0] || '', streetLines[1] || ''] .filter(Boolean).join(', '); + var placedOrderId = this.paystackOrderId; + this.paystackOrderId = null; + var _this = this; _this.isPlaceOrderActionAllowed(false); @@ -115,42 +135,46 @@ define( return; } var popup = new PaystackPop(); + var metadata = { + quoteId: quoteId, + custom_fields: [ + { + display_name: "QuoteId", + variable_name: "quote id", + value: quoteId + }, + { + display_name: "Address", + variable_name: "address", + value: streetAddress + }, + { + display_name: "Postal Code", + variable_name: "postal_code", + value: paymentData.postcode + }, + { + display_name: "City", + variable_name: "city", + value: paymentData.city + ", " + paymentData.countryId + }, + { + display_name: "Plugin", + variable_name: "plugin", + value: "magento-2" + } + ] + }; + if (placedOrderId) { + metadata.orderId = placedOrderId; + } popup.newTransaction({ key: paystackConfiguration.public_key, email: paymentData.email, amount: Math.round(quote.totals().grand_total * 100), phone: paymentData.telephone, currency: checkoutConfig.totalsData.quote_currency_code, - metadata: { - quoteId: quoteId, - custom_fields: [ - { - display_name: "QuoteId", - variable_name: "quote id", - value: quoteId - }, - { - display_name: "Address", - variable_name: "address", - value: streetAddress - }, - { - display_name: "Postal Code", - variable_name: "postal_code", - value: paymentData.postcode - }, - { - display_name: "City", - variable_name: "city", - value: paymentData.city + ", " + paymentData.countryId - }, - { - display_name: "Plugin", - variable_name: "plugin", - value: "magento-2" - } - ] - }, + metadata: metadata, onSuccess: function (response) { // Invariant: everything below this point runs AFTER Paystack has // taken the customer's money, so this handler fails closed — only From 2ce912abce649afdc56852a826997bb533463445 Mon Sep 17 00:00:00 2001 From: Jules Nsenda Date: Tue, 29 Sep 2026 15:25:34 +0200 Subject: [PATCH 2/9] refactor: one definition of a payable order (TransactionValidator::isPayable) register()'s order-state guard moves onto the new predicate (state new or pending_payment, and a positive base amount due) so the upcoming webhook order resolver can apply exactly the same rule. Recreate keeps its own state list on purpose: it gates an anonymous cancel. Co-Authored-By: Claude Opus 5.5 --- Controller/Payment/Recreate.php | 2 ++ Gateway/Validator/TransactionValidator.php | 18 +++++++++++++ Model/PaymentSettlement.php | 4 +-- .../Validator/TransactionValidatorTest.php | 27 +++++++++++++++++++ 4 files changed, 48 insertions(+), 3 deletions(-) diff --git a/Controller/Payment/Recreate.php b/Controller/Payment/Recreate.php index 3a762f5..51080f3 100644 --- a/Controller/Payment/Recreate.php +++ b/Controller/Payment/Recreate.php @@ -47,6 +47,8 @@ public function execute() { // Allow-list, not deny-list: only the two pre-payment states are // restorable, so a future state this list doesn't know about fails closed. + // Deliberately its own list, not TransactionValidator::isPayable(): this + // gates an anonymous cancel and must not widen if "payable" ever does. $isPrePaymentState = in_array( $order->getState(), [Order::STATE_NEW, Order::STATE_PENDING_PAYMENT], diff --git a/Gateway/Validator/TransactionValidator.php b/Gateway/Validator/TransactionValidator.php index 9c5c16a..19272d9 100644 --- a/Gateway/Validator/TransactionValidator.php +++ b/Gateway/Validator/TransactionValidator.php @@ -3,6 +3,7 @@ namespace Pstk\Paystack\Gateway\Validator; use Magento\Sales\Api\Data\OrderInterface; +use Magento\Sales\Model\Order; use Pstk\Paystack\Model\Payment\Paystack; use Psr\Log\LoggerInterface; @@ -168,6 +169,23 @@ public function isPaystackOrder(OrderInterface $order): bool return $payment !== null && $payment->getMethod() === Paystack::CODE; } + /** + * True when the order can still be registered as paid: in a pre-payment + * state (STATE_NEW/STATE_PENDING_PAYMENT) with a positive base amount due. + * The single definition of that, used by + * Model/PaymentSettlement::register()'s order-state guard and the webhook + * order resolver. Allow-list, not deny-list, so a state this list doesn't + * know about fails closed. + * + * @param OrderInterface $order + * @return bool + */ + public function isPayable(OrderInterface $order): bool + { + return in_array($order->getState(), [Order::STATE_NEW, Order::STATE_PENDING_PAYMENT], true) + && $order->getBaseTotalDue() > 0; + } + /** * The integral subunit amount a verify response reports as paid, or null * when the envelope is unreadable or the amount is not a whole number — diff --git a/Model/PaymentSettlement.php b/Model/PaymentSettlement.php index 8c9fdc1..aa66b77 100644 --- a/Model/PaymentSettlement.php +++ b/Model/PaymentSettlement.php @@ -201,9 +201,7 @@ public function register(object $transactionDetails, OrderInterface $order, bool // resolved. registerCaptureNotification() flips state as a side // effect, so a re-verify of an already-advanced order must hit step // 3's no-op before this guard could mistake it for not-payable. - if (!in_array($freshOrder->getState(), [Order::STATE_NEW, Order::STATE_PENDING_PAYMENT], true) - || $freshOrder->getBaseTotalDue() <= 0 - ) { + if (!$this->transactionValidator->isPayable($freshOrder)) { $this->recordRejection( $freshOrder, $reference, diff --git a/Test/Unit/Gateway/Validator/TransactionValidatorTest.php b/Test/Unit/Gateway/Validator/TransactionValidatorTest.php index 9889d00..42715d6 100644 --- a/Test/Unit/Gateway/Validator/TransactionValidatorTest.php +++ b/Test/Unit/Gateway/Validator/TransactionValidatorTest.php @@ -1171,4 +1171,31 @@ public function testEveryReasonConstantIsAccountedForInClassificationMaps(): voi ); } } + + /** + * @dataProvider isPayableProvider + */ + public function testIsPayable(string $state, float $baseTotalDue, bool $expected): void + { + $order = $this->createMock(Order::class); + $order->method('getState')->willReturn($state); + $order->method('getBaseTotalDue')->willReturn($baseTotalDue); + + $this->assertSame($expected, $this->validator->isPayable($order)); + } + + public static function isPayableProvider(): array + { + return [ + 'new, due' => [Order::STATE_NEW, 5000.00, true], + 'pending_payment, due' => [Order::STATE_PENDING_PAYMENT, 5000.00, true], + 'new, nothing due' => [Order::STATE_NEW, 0.0, false], + 'new, negative due' => [Order::STATE_NEW, -1.0, false], + 'processing' => [Order::STATE_PROCESSING, 5000.00, false], + 'canceled' => [Order::STATE_CANCELED, 5000.00, false], + 'closed' => [Order::STATE_CLOSED, 5000.00, false], + 'complete' => [Order::STATE_COMPLETE, 5000.00, false], + 'holded' => [Order::STATE_HOLDED, 5000.00, false], + ]; + } } From 1dcb9838e6f85e86bc8167cfc5f3cf839c95653e Mon Sep 17 00:00:00 2001 From: Jules Nsenda Date: Tue, 29 Sep 2026 15:27:15 +0200 Subject: [PATCH 3/9] fix(webhook): acknowledge a charge for a closed order once it is recorded A charge whose order can never take it (canceled/closed/complete, or nothing left due) used to return ORDER_NOT_PAYABLE, which the webhook retries for Paystack's whole ~72h budget and which risks endpoint back-off. It now returns the new ORDER_CLOSED, a permanent reason, but only once the rejection is durably on the order's history; if that write fails it stays ORDER_NOT_PAYABLE so the retry keeps trying to record it. Held or payment-review orders keep retrying as before. Co-Authored-By: Claude Opus 5.5 --- Gateway/Validator/TransactionValidator.php | 51 +++++++-- Model/PaymentSettlement.php | 54 ++++++--- .../Validator/TransactionValidatorTest.php | 41 +++++++ Test/Unit/Model/PaymentSettlementTest.php | 103 +++++++++++++++++- 4 files changed, 222 insertions(+), 27 deletions(-) diff --git a/Gateway/Validator/TransactionValidator.php b/Gateway/Validator/TransactionValidator.php index 19272d9..c324a66 100644 --- a/Gateway/Validator/TransactionValidator.php +++ b/Gateway/Validator/TransactionValidator.php @@ -58,20 +58,34 @@ class TransactionValidator public const REASON_REFERENCE_BOUND_ELSEWHERE = 'reference_bound_elsewhere'; /** - * The order is not in a state (STATE_NEW/STATE_PENDING_PAYMENT) that can - * still be registered as paid — e.g. canceled/closed. Never - * RETRYABLE_FOR_CUSTOMER (money moved or the situation is otherwise - * unrecoverable by retry — fail closed there), but deliberately NOT - * PERMANENT_FOR_WEBHOOK, unlike REASON_REFERENCE_BOUND_ELSEWHERE: a - * bank-transfer/USSD charge that is genuinely `pending` at callback time - * can settle minutes later via the webhook, and if the customer used - * `/paystack/payment/recreate` in the meantime the order is now - * `canceled` — a late but genuine `charge.success` must keep retrying + * The order is not payable right now, but the state may still change + * (holded, payment_review, ...) — or it is terminal (see + * REASON_ORDER_CLOSED) but the rejection history could not be durably + * recorded yet. Never RETRYABLE_FOR_CUSTOMER (money moved or the situation + * is otherwise unrecoverable by retry — fail closed there), but + * deliberately NOT PERMANENT_FOR_WEBHOOK, unlike + * REASON_REFERENCE_BOUND_ELSEWHERE: a bank-transfer/USSD charge that is + * genuinely `pending` at callback time can settle minutes later via the + * webhook, and an order that is held or under payment review may become + * payable again — a late but genuine `charge.success` must keep retrying * (Webhook.php's own `NEVER_RECENCY_BOUNDED`), not be permanently * dropped, or the money is captured with no order and no refund path. */ public const REASON_ORDER_NOT_PAYABLE = 'order_not_payable'; + /** + * The order can never take this money: canceled/closed/complete, or nothing + * left due (e.g. already paid by another reference) — see + * isClosedForPayment(). Returned only once the rejection has been durably + * recorded on the order's history, so the webhook acknowledges (permanent, + * 200) instead of retrying for Paystack's ~72h budget, which risks endpoint + * back-off (industry standard: ack with 2xx once the problem is recorded). + * Never RETRYABLE_FOR_CUSTOMER — money moved, fail closed. When the history + * could not be recorded, PaymentSettlement returns REASON_ORDER_NOT_PAYABLE + * instead, so the webhook keeps retrying until it can be. + */ + public const REASON_ORDER_CLOSED = 'order_closed'; + /** * A throw from `Model/PaymentSettlement::register()`'s own bind/register/ * save steps, after every validation and binding check already passed — @@ -129,6 +143,7 @@ class TransactionValidator self::REASON_CURRENCY_MISMATCH, self::REASON_ZERO_TOTAL, self::REASON_REFERENCE_BOUND_ELSEWHERE, + self::REASON_ORDER_CLOSED, ]; /** @var LoggerInterface */ @@ -186,6 +201,24 @@ public function isPayable(OrderInterface $order): bool && $order->getBaseTotalDue() > 0; } + /** + * True when the order can never take a payment: a terminal state + * (canceled/closed/complete) or nothing left due. Distinct from merely + * !isPayable(): holded/payment_review orders are not payable *yet* and are + * deliberately not closed here. + * + * @param OrderInterface $order + * @return bool + */ + public function isClosedForPayment(OrderInterface $order): bool + { + return in_array( + $order->getState(), + [Order::STATE_CANCELED, Order::STATE_CLOSED, Order::STATE_COMPLETE], + true + ) || $order->getBaseTotalDue() <= 0; + } + /** * The integral subunit amount a verify response reports as paid, or null * when the envelope is unreadable or the amount is not a whole number — diff --git a/Model/PaymentSettlement.php b/Model/PaymentSettlement.php index aa66b77..a4785fc 100644 --- a/Model/PaymentSettlement.php +++ b/Model/PaymentSettlement.php @@ -50,6 +50,9 @@ */ class PaymentSettlement { + private const HISTORY_APPENDED = 'appended'; + private const HISTORY_PRESENT = 'present'; + /** @var TransactionValidator */ private $transactionValidator; @@ -202,6 +205,23 @@ public function register(object $transactionDetails, OrderInterface $order, bool // effect, so a re-verify of an already-advanced order must hit step // 3's no-op before this guard could mistake it for not-payable. if (!$this->transactionValidator->isPayable($freshOrder)) { + // A terminal order can never take this money, so once the + // rejection is durably on its history the webhook may acknowledge + // instead of retrying for ~72h. If it could not be recorded, stay + // ORDER_NOT_PAYABLE so the retry keeps trying to record it. + if ($this->transactionValidator->isClosedForPayment($freshOrder)) { + $recorded = $this->recordRejection( + $freshOrder, + $reference, + TransactionValidator::REASON_ORDER_CLOSED, + $transactionDetails + ); + $reason = $recorded + ? TransactionValidator::REASON_ORDER_CLOSED + : TransactionValidator::REASON_ORDER_NOT_PAYABLE; + return ['reason' => $reason, 'order' => $freshOrder]; + } + $this->recordRejection( $freshOrder, $reference, @@ -297,9 +317,10 @@ private function historyMarker(string $reference, string $reasonKey): string * @param string $reference * @param string $reason * @param object $transactionDetails Full envelope PaystackApiClient::verifyTransaction() returns - * @return void + * @return bool True when the rejection is durably on the order's history + * (see writeHistory()). */ - private function recordRejection(Order $order, string $reference, string $reason, object $transactionDetails): void + private function recordRejection(Order $order, string $reference, string $reason, object $transactionDetails): bool { $paidAmount = $this->transactionValidator->paidSubunits($transactionDetails) ?? 'unknown'; // Guarded the same way TransactionValidator::allowListedContext() @@ -324,7 +345,7 @@ private function recordRejection(Order $order, string $reference, string $reason // reason as decided. $isPermanent = $this->transactionValidator->isPermanentForWebhook($reason); - $this->writeHistory( + return $this->writeHistory( $order, $reference, $reason, @@ -353,26 +374,27 @@ private function recordRejection(Order $order, string $reference, string $reason * @param string $reference * @param string $reasonKey * @param string $comment - * @return bool True when a new comment was actually appended (not a - * dedupe no-op or a failed write) — tells writeHistory() below - * whether a save is even warranted. + * @return string|null HISTORY_APPENDED when a new comment was appended, + * HISTORY_PRESENT for a dedupe no-op, null for a failed write — + * tells writeHistory() below whether a save is warranted and whether + * the record already exists. */ - private function appendHistoryComment(Order $order, string $reference, string $reasonKey, string $comment): bool + private function appendHistoryComment(Order $order, string $reference, string $reasonKey, string $comment): ?string { $marker = $this->historyMarker($reference, $reasonKey); try { foreach ($order->getStatusHistories() ?? [] as $history) { $existingComment = $history->getComment() ?? ''; if (strpos($existingComment, $marker) !== false) { - return false; + return self::HISTORY_PRESENT; } } $order->addStatusToHistory($order->getStatus(), $comment . ' ' . $marker); - return true; + return self::HISTORY_APPENDED; } catch (\Throwable $exc) { $this->logger->error('Paystack: failed to write order history', ['error' => $exc->getMessage()]); - return false; + return null; } } @@ -389,18 +411,22 @@ private function appendHistoryComment(Order $order, string $reference, string $r * @param string $reference * @param string $reasonKey * @param string $comment - * @return void + * @return bool True when the (reference, reason) record is durably on the + * order: the marker was already present, or it was appended and saved. */ - private function writeHistory(Order $order, string $reference, string $reasonKey, string $comment): void + private function writeHistory(Order $order, string $reference, string $reasonKey, string $comment): bool { - if (!$this->appendHistoryComment($order, $reference, $reasonKey, $comment)) { - return; + $appended = $this->appendHistoryComment($order, $reference, $reasonKey, $comment); + if ($appended !== self::HISTORY_APPENDED) { + return $appended === self::HISTORY_PRESENT; } try { $this->orderRepository->save($order); + return true; } catch (\Throwable $exc) { $this->logger->error('Paystack: failed to write order history', ['error' => $exc->getMessage()]); + return false; } } } diff --git a/Test/Unit/Gateway/Validator/TransactionValidatorTest.php b/Test/Unit/Gateway/Validator/TransactionValidatorTest.php index 42715d6..d5737b6 100644 --- a/Test/Unit/Gateway/Validator/TransactionValidatorTest.php +++ b/Test/Unit/Gateway/Validator/TransactionValidatorTest.php @@ -858,6 +858,7 @@ public static function terminalReasonProvider(): array 'mode_mismatch' => [TransactionValidator::REASON_MODE_MISMATCH], 'reference_bound_elsewhere' => [TransactionValidator::REASON_REFERENCE_BOUND_ELSEWHERE], 'order_not_payable' => [TransactionValidator::REASON_ORDER_NOT_PAYABLE], + 'order_closed' => [TransactionValidator::REASON_ORDER_CLOSED], 'registration_failed' => [TransactionValidator::REASON_REGISTRATION_FAILED], // Regression test for the whole finding: a reason this class does // not (yet) know about must fail closed, not silently invite a @@ -885,6 +886,9 @@ public static function permanentWebhookReasonProvider(): array // retrying won't change the answer, unlike REASON_ORDER_NOT_PAYABLE // below. 'reference_bound_elsewhere' => [TransactionValidator::REASON_REFERENCE_BOUND_ELSEWHERE], + // The order can never take this money and the rejection is already + // recorded on it — ack instead of retrying for ~72h. + 'order_closed' => [TransactionValidator::REASON_ORDER_CLOSED], ]; } @@ -961,6 +965,7 @@ public static function customerMessageProvider(): array 'mode_mismatch' => [TransactionValidator::REASON_MODE_MISMATCH, $doNotPayAgain], 'reference_bound_elsewhere' => [TransactionValidator::REASON_REFERENCE_BOUND_ELSEWHERE, $doNotPayAgain], 'order_not_payable' => [TransactionValidator::REASON_ORDER_NOT_PAYABLE, $doNotPayAgain], + 'order_closed' => [TransactionValidator::REASON_ORDER_CLOSED, $doNotPayAgain], 'registration_failed' => [TransactionValidator::REASON_REGISTRATION_FAILED, $doNotPayAgain], // Regression test for the whole finding: an unrecognised reason must // fall into the safest copy, exactly like isTerminalForCustomer() @@ -969,6 +974,14 @@ public static function customerMessageProvider(): array ]; } + public function testOrderClosedMessageMatchesOrderNotPayable(): void + { + $this->assertSame( + $this->validator->customerMessage(TransactionValidator::REASON_ORDER_NOT_PAYABLE), + $this->validator->customerMessage(TransactionValidator::REASON_ORDER_CLOSED) + ); + } + public function testIsOverpaymentTrueWhenPaidExceedsExpected(): void { $order = $this->makeOrder(5000.00, 'NGN'); @@ -1113,6 +1126,7 @@ public function testEveryReasonConstantIsAccountedForInClassificationMaps(): voi TransactionValidator::REASON_MODE_MISMATCH, TransactionValidator::REASON_REFERENCE_BOUND_ELSEWHERE, TransactionValidator::REASON_ORDER_NOT_PAYABLE, + TransactionValidator::REASON_ORDER_CLOSED, TransactionValidator::REASON_REGISTRATION_FAILED, ]; @@ -1130,6 +1144,7 @@ public function testEveryReasonConstantIsAccountedForInClassificationMaps(): voi TransactionValidator::REASON_MODE_MISMATCH, TransactionValidator::REASON_REFERENCE_BOUND_ELSEWHERE, TransactionValidator::REASON_ORDER_NOT_PAYABLE, + TransactionValidator::REASON_ORDER_CLOSED, TransactionValidator::REASON_REGISTRATION_FAILED, ]; @@ -1147,6 +1162,7 @@ public function testEveryReasonConstantIsAccountedForInClassificationMaps(): voi TransactionValidator::REASON_MODE_MISMATCH, TransactionValidator::REASON_REFERENCE_BOUND_ELSEWHERE, TransactionValidator::REASON_ORDER_NOT_PAYABLE, + TransactionValidator::REASON_ORDER_CLOSED, TransactionValidator::REASON_REGISTRATION_FAILED, ]; @@ -1198,4 +1214,29 @@ public static function isPayableProvider(): array 'holded' => [Order::STATE_HOLDED, 5000.00, false], ]; } + + /** + * @dataProvider isClosedForPaymentProvider + */ + public function testIsClosedForPayment(string $state, float $baseTotalDue, bool $expected): void + { + $order = $this->createMock(Order::class); + $order->method('getState')->willReturn($state); + $order->method('getBaseTotalDue')->willReturn($baseTotalDue); + + $this->assertSame($expected, $this->validator->isClosedForPayment($order)); + } + + public static function isClosedForPaymentProvider(): array + { + return [ + 'canceled, due' => [Order::STATE_CANCELED, 5000.00, true], + 'closed, due' => [Order::STATE_CLOSED, 5000.00, true], + 'complete, due' => [Order::STATE_COMPLETE, 5000.00, true], + 'new, nothing due' => [Order::STATE_NEW, 0.0, true], + 'new, due' => [Order::STATE_NEW, 5000.00, false], + 'holded, due' => [Order::STATE_HOLDED, 5000.00, false], + 'payment_review, due' => [Order::STATE_PAYMENT_REVIEW, 5000.00, false], + ]; + } } diff --git a/Test/Unit/Model/PaymentSettlementTest.php b/Test/Unit/Model/PaymentSettlementTest.php index edcdfd4..1c33456 100644 --- a/Test/Unit/Model/PaymentSettlementTest.php +++ b/Test/Unit/Model/PaymentSettlementTest.php @@ -325,11 +325,49 @@ public function testDifferentReferenceOnAlreadyRegisteredOrderIsNotTreatedAsIdem $this->assertSame(TransactionValidator::REASON_ORDER_NOT_PAYABLE, $result['reason']); } - public function testOrderNotInPayableStateIsRejected(): void + /** + * @dataProvider terminalStateProvider + */ + public function testTerminalOrderStateIsRejectedAsOrderClosed(string $state): void { $this->noExistingBindings(); - $order = $this->makeOrder($this->makePaystackPayment(), 1, Order::STATE_CANCELED); + $order = $this->makeOrder($this->makePaystackPayment(), 1, $state); + $freshOrder = $this->orderRepository->get(1); + + $freshOrder->getPayment()->expects($this->never())->method('registerCaptureNotification'); + $freshOrder->expects($this->once()) + ->method('addStatusToHistory') + ->with($this->anything(), $this->stringContains('[paystack:PSK_ref_123:order_closed]')); + $this->orderRepository->expects($this->once())->method('save')->with($freshOrder); + + $result = $this->paymentSettlement->register( + (object) ['data' => $this->makeVerifyData()], + $order, + true + ); + + $this->assertSame(TransactionValidator::REASON_ORDER_CLOSED, $result['reason']); + $this->assertSame($freshOrder, $result['order']); + } + + public static function terminalStateProvider(): array + { + return [ + 'canceled' => [Order::STATE_CANCELED], + 'closed' => [Order::STATE_CLOSED], + 'complete' => [Order::STATE_COMPLETE], + ]; + } + + /** + * @dataProvider recoverableNotPayableStateProvider + */ + public function testNonTerminalNotPayableStateStaysOrderNotPayable(string $state): void + { + $this->noExistingBindings(); + + $order = $this->makeOrder($this->makePaystackPayment(), 1, $state); $freshOrder = $this->orderRepository->get(1); $freshOrder->getPayment()->expects($this->never())->method('registerCaptureNotification'); @@ -346,7 +384,64 @@ public function testOrderNotInPayableStateIsRejected(): void $this->assertSame(TransactionValidator::REASON_ORDER_NOT_PAYABLE, $result['reason']); } - public function testZeroBaseTotalDueIsRejectedAsNotPayable(): void + public static function recoverableNotPayableStateProvider(): array + { + return [ + 'holded' => [Order::STATE_HOLDED], + 'payment_review' => [Order::STATE_PAYMENT_REVIEW], + ]; + } + + /** + * Terminal, but the rejection history could not be saved: the webhook must + * keep retrying until it can be recorded, so this is ORDER_NOT_PAYABLE. + */ + public function testTerminalOrderWhoseHistoryCannotBeSavedStaysOrderNotPayable(): void + { + $this->noExistingBindings(); + + $order = $this->makeOrder($this->makePaystackPayment(), 1, Order::STATE_CANCELED); + $this->orderRepository->method('save')->willThrowException(new \RuntimeException('db down')); + + $result = $this->paymentSettlement->register( + (object) ['data' => $this->makeVerifyData()], + $order, + true + ); + + $this->assertSame(TransactionValidator::REASON_ORDER_NOT_PAYABLE, $result['reason']); + } + + /** + * Retry of an already-recorded terminal rejection: the marker is durable + * proof, so ORDER_CLOSED is returned with no second comment or save. + */ + public function testTerminalOrderWithMarkerAlreadyPresentIsClosedWithoutRewriting(): void + { + $this->noExistingBindings(); + + $order = $this->makeOrder($this->makePaystackPayment(), 1, Order::STATE_CANCELED); + $freshOrder = $this->orderRepository->get(1); + + $existingHistory = $this->createMock(OrderStatusHistoryInterface::class); + $existingHistory->method('getComment')->willReturn( + 'Paystack: payment rejected. [paystack:PSK_ref_123:order_closed]' + ); + $freshOrder->method('getStatusHistories')->willReturn([$existingHistory]); + + $freshOrder->expects($this->never())->method('addStatusToHistory'); + $this->orderRepository->expects($this->never())->method('save'); + + $result = $this->paymentSettlement->register( + (object) ['data' => $this->makeVerifyData()], + $order, + true + ); + + $this->assertSame(TransactionValidator::REASON_ORDER_CLOSED, $result['reason']); + } + + public function testZeroBaseTotalDueIsRejectedAsOrderClosed(): void { $this->noExistingBindings(); @@ -384,7 +479,7 @@ public function testZeroBaseTotalDueIsRejectedAsNotPayable(): void true ); - $this->assertSame(TransactionValidator::REASON_ORDER_NOT_PAYABLE, $result['reason']); + $this->assertSame(TransactionValidator::REASON_ORDER_CLOSED, $result['reason']); } /** From 970e50e24710fc115780af29a303c8482204e823 Mon Sep 17 00:00:00 2001 From: Jules Nsenda Date: Tue, 29 Sep 2026 15:30:05 +0200 Subject: [PATCH 4/9] fix(webhook): settle the right order when several share a quote (#69) Inline retries cancel the previous order and reuse its quote, so the webhook's quoteId lookup found several orders, required exactly one, and left the paid order pending. Order lookup moves into WebhookOrderResolver: increment id, then an order the reference is already bound to, then the popup's metadata.orderId (matched with its quoteId), then the quote - a lone order as before, otherwise the single payable Paystack order, and never a guess among several. Metadata is read from the verify response and strictly parsed; repository errors propagate (503) instead of falling through to a weaker step. Co-Authored-By: Claude Opus 5.5 --- .../Payment/AbstractPaystackStandard.php | 14 +- Controller/Payment/Webhook.php | 16 +- Model/WebhookOrderResolver.php | 230 +++++++++ Test/Unit/Controller/Payment/CallbackTest.php | 3 +- Test/Unit/Controller/Payment/RecreateTest.php | 3 +- Test/Unit/Controller/Payment/SetupTest.php | 3 +- Test/Unit/Controller/Payment/WebhookTest.php | 316 ++++++++---- Test/Unit/Model/WebhookOrderResolverTest.php | 460 ++++++++++++++++++ etc/frontend/di.xml | 2 + 9 files changed, 947 insertions(+), 100 deletions(-) create mode 100644 Model/WebhookOrderResolver.php create mode 100644 Test/Unit/Model/WebhookOrderResolverTest.php diff --git a/Controller/Payment/AbstractPaystackStandard.php b/Controller/Payment/AbstractPaystackStandard.php index 47884e1..7a23544 100644 --- a/Controller/Payment/AbstractPaystackStandard.php +++ b/Controller/Payment/AbstractPaystackStandard.php @@ -94,6 +94,12 @@ abstract class AbstractPaystackStandard extends \Magento\Framework\App\Action\Ac */ protected $paymentSettlement; + /** + * + * @var \Pstk\Paystack\Model\WebhookOrderResolver + */ + protected $webhookOrderResolver; + /** * Constructor * @@ -115,7 +121,8 @@ public function __construct( \Psr\Log\LoggerInterface $logger, PaystackApiClient $paystackClient, ?\Pstk\Paystack\Gateway\Validator\TransactionValidator $transactionValidator = null, - ?\Pstk\Paystack\Model\PaymentSettlement $paymentSettlement = null + ?\Pstk\Paystack\Model\PaymentSettlement $paymentSettlement = null, + ?\Pstk\Paystack\Model\WebhookOrderResolver $webhookOrderResolver = null ) { $this->resultPageFactory = $resultPageFactory; $this->orderRepository = $orderRepository; @@ -148,6 +155,11 @@ public function __construct( $this->paymentSettlement = $paymentSettlement ?: \Magento\Framework\App\ObjectManager::getInstance() ->get(\Pstk\Paystack\Model\PaymentSettlement::class); + // Same pattern and BC reasoning again; only Webhook.php calls it, so + // it is Proxy-wired in etc/frontend/di.xml too. + $this->webhookOrderResolver = $webhookOrderResolver + ?: \Magento\Framework\App\ObjectManager::getInstance() + ->get(\Pstk\Paystack\Model\WebhookOrderResolver::class); parent::__construct($context); } diff --git a/Controller/Payment/Webhook.php b/Controller/Payment/Webhook.php index 68c3db2..7c6e300 100644 --- a/Controller/Payment/Webhook.php +++ b/Controller/Payment/Webhook.php @@ -123,19 +123,9 @@ public function execute() { $this->logger->info("Paystack Webhook: verified transaction", ['reference' => $reference]); - $order = $this->orderInterface->loadByIncrementId($reference); - - // In popup mode, reference is generated by Paystack and we provided quoteId instead - if ((!$order || !$order->getId()) && isset($event->data->metadata->quoteId)) { - $this->logger->info("Paystack Webhook: order not found by reference, searching by quoteId", ['quoteId' => $event->data->metadata->quoteId]); - $objectManager = \Magento\Framework\App\ObjectManager::getInstance(); - $searchCriteriaBuilder = $objectManager->create('Magento\Framework\Api\SearchCriteriaBuilder'); - $searchCriteria = $searchCriteriaBuilder->addFilter('quote_id', $event->data->metadata->quoteId, 'eq')->create(); - $items = $this->orderRepository->getList($searchCriteria); - if ($items->getTotalCount() == 1) { - $order = $items->getFirstItem(); - } - } + // Increment id, bound reference, then the verified metadata's + // orderId/quoteId — see WebhookOrderResolver for the order and why. + $order = $this->webhookOrderResolver->resolve($reference, $transactionDetails); if ($order && $order->getId()) { [$code, $body] = $this->resolveSettlement($transactionDetails, $order, $reference); diff --git a/Model/WebhookOrderResolver.php b/Model/WebhookOrderResolver.php new file mode 100644 index 0000000..deb867f --- /dev/null +++ b/Model/WebhookOrderResolver.php @@ -0,0 +1,230 @@ + you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see //www.gnu.org/licenses/>. + */ + +namespace Pstk\Paystack\Model; + +use Magento\Framework\Api\SearchCriteriaBuilder; +use Magento\Sales\Api\Data\OrderInterface; +use Magento\Sales\Api\OrderRepositoryInterface; +use Magento\Sales\Api\TransactionRepositoryInterface; +use Pstk\Paystack\Gateway\Validator\TransactionValidator; +use Psr\Log\LoggerInterface; + +/** + * Finds the order a verified `charge.success` webhook is for. Inline (popup) + * payments use a Paystack-generated reference, so several orders can share one + * `quote_id` once the customer has cancelled and retried (issue #69) and the + * quote alone no longer names the order. First match wins, in this order: + * + * 1. increment id == reference (redirect flow); + * 2. a `sales_payment_transaction` already bound to the reference (redelivery + * after inline verify / an earlier webhook settled it — register() then + * answers idempotently); + * 3. `metadata.orderId` + `metadata.quoteId` naming one Paystack order — + * used in ANY state, so a cancelled attempt's late money is rejected + * against the right order rather than moved to a sibling; + * 4. `metadata.quoteId` alone: a lone order on the quote in any state/method + * (the pre-#69 rule), else the single Paystack order that + * TransactionValidator::isPayable(); several candidates never pick one. + * + * Security: metadata is read from the re-verified Paystack response, never the + * event payload, but `orderId` and `quoteId` are both payer-controlled. The + * quote cross-check is a consistency check, not an authorization boundary — + * the bound on what a tampered id can achieve comes from + * PaymentSettlement::register()'s amount/currency/mode/status checks and its + * one-reference-one-order binding. Ids are logged (type-guarded) but never + * written into order history. + */ +class WebhookOrderResolver +{ + /** @var OrderRepositoryInterface */ + private $orderRepository; + + /** @var SearchCriteriaBuilder */ + private $searchCriteriaBuilder; + + /** @var TransactionRepositoryInterface */ + private $transactionRepository; + + /** @var OrderInterface */ + private $orderInterface; + + /** @var TransactionValidator */ + private $transactionValidator; + + /** @var LoggerInterface */ + private $logger; + + public function __construct( + OrderRepositoryInterface $orderRepository, + SearchCriteriaBuilder $searchCriteriaBuilder, + TransactionRepositoryInterface $transactionRepository, + OrderInterface $orderInterface, + TransactionValidator $transactionValidator, + LoggerInterface $logger + ) { + $this->orderRepository = $orderRepository; + $this->searchCriteriaBuilder = $searchCriteriaBuilder; + $this->transactionRepository = $transactionRepository; + $this->orderInterface = $orderInterface; + $this->transactionValidator = $transactionValidator; + $this->logger = $logger; + } + + /** + * Repository/DB errors deliberately propagate (the webhook answers 503): + * falling through to a weaker step on a transient failure could settle a + * sibling order. + * + * @param string $reference Paystack's echoed reference from the verify response + * @param object $transactionDetails Full envelope PaystackApiClient::verifyTransaction() returns + * @return OrderInterface|null + */ + public function resolve(string $reference, object $transactionDetails): ?OrderInterface + { + // Step 1: increment id (redirect flow). + $order = $this->orderInterface->loadByIncrementId($reference); + if ($order && $order->getId()) { + return $order; + } + + // Step 2: reference already bound to an order. Loaded via getList(), + // not get(), so the repository's registry doesn't hand register() this + // same instance back as its "fresh" re-fetch. + $searchCriteria = $this->searchCriteriaBuilder + ->addFilter('txn_id', $reference, 'eq') + ->create(); + foreach ($this->transactionRepository->getList($searchCriteria)->getItems() as $transaction) { + $boundOrder = $this->findOrder($transaction->getOrderId()); + if (null !== $boundOrder) { + return $boundOrder; + } + } + + $metadata = $this->readMetadata($transactionDetails); + $quoteId = $this->parseId($metadata->quoteId ?? null); + if (null === $quoteId) { + $this->logger->info('Paystack Webhook: no usable quoteId in verified metadata', [ + 'reference' => $reference, + ]); + return null; + } + + // Step 3: the exact order the popup placed, when the checkout sent it. + $orderId = $this->parseId($metadata->orderId ?? null); + if (null !== $orderId) { + $searchCriteria = $this->searchCriteriaBuilder + ->addFilter('entity_id', $orderId, 'eq') + ->addFilter('quote_id', $quoteId, 'eq') + ->create(); + $matches = array_values($this->orderRepository->getList($searchCriteria)->getItems()); + if (1 === count($matches) && $this->transactionValidator->isPaystackOrder($matches[0])) { + return $matches[0]; + } + $this->logger->info('Paystack Webhook: metadata orderId did not match a Paystack order on the quote, using quote lookup', [ + 'reference' => $reference, + 'candidates' => count($matches), + ]); + } + + // Step 4: quote fallback. + $searchCriteria = $this->searchCriteriaBuilder + ->addFilter('quote_id', $quoteId, 'eq') + ->create(); + $candidates = array_values($this->orderRepository->getList($searchCriteria)->getItems()); + + if (1 === count($candidates)) { + return $candidates[0]; + } + + $payable = array_values(array_filter( + $candidates, + function (OrderInterface $candidate): bool { + return $this->transactionValidator->isPaystackOrder($candidate) + && $this->transactionValidator->isPayable($candidate); + } + )); + + $this->logger->info('Paystack Webhook: resolved orders by quoteId', [ + 'reference' => $reference, + 'candidates' => count($candidates), + 'payable' => count($payable), + ]); + + return 1 === count($payable) ? $payable[0] : null; + } + + /** + * @param mixed $orderId + * @return OrderInterface|null + */ + private function findOrder($orderId): ?OrderInterface + { + $searchCriteria = $this->searchCriteriaBuilder + ->addFilter('entity_id', $orderId, 'eq') + ->create(); + $items = array_values($this->orderRepository->getList($searchCriteria)->getItems()); + + return $items[0] ?? null; + } + + /** + * The verify response's metadata as an object, or an empty one. Paystack + * may hand it back as an object, a JSON string, or "" — anything that is + * not (or does not decode to) an object carries no ids. + * + * @param object $transactionDetails + * @return object + */ + private function readMetadata(object $transactionDetails): object + { + $data = $transactionDetails->data ?? null; + $metadata = is_object($data) ? ($data->metadata ?? null) : null; + + if (is_string($metadata)) { + $metadata = json_decode($metadata); + } + + return is_object($metadata) ? $metadata : new \stdClass(); + } + + /** + * Strict positive-integer id as a string, or null: an int > 0, or a + * digit-only string with no leading zero. Floats, bools, arrays and + * anything else are refused rather than coerced. + * + * @param mixed $value + * @return string|null + */ + private function parseId($value): ?string + { + if (is_int($value) && $value > 0) { + return (string) $value; + } + + if (is_string($value) && preg_match('/^[1-9]\d*$/', $value)) { + return $value; + } + + return null; + } +} diff --git a/Test/Unit/Controller/Payment/CallbackTest.php b/Test/Unit/Controller/Payment/CallbackTest.php index 69ec48a..696cc49 100644 --- a/Test/Unit/Controller/Payment/CallbackTest.php +++ b/Test/Unit/Controller/Payment/CallbackTest.php @@ -125,7 +125,8 @@ private function createController(): Callback $this->logger, $this->paystackClient, new TransactionValidator($this->createMock(LoggerInterface::class)), - $paymentSettlement + $paymentSettlement, + $this->createMock(\Pstk\Paystack\Model\WebhookOrderResolver::class) ); } diff --git a/Test/Unit/Controller/Payment/RecreateTest.php b/Test/Unit/Controller/Payment/RecreateTest.php index f32e30b..4a7ddc9 100644 --- a/Test/Unit/Controller/Payment/RecreateTest.php +++ b/Test/Unit/Controller/Payment/RecreateTest.php @@ -78,7 +78,8 @@ private function createController(): Recreate $this->createMock(LoggerInterface::class), $this->createMock(PaystackApiClient::class), new TransactionValidator($this->createMock(LoggerInterface::class)), - $this->createMock(PaymentSettlement::class) + $this->createMock(PaymentSettlement::class), + $this->createMock(\Pstk\Paystack\Model\WebhookOrderResolver::class) ); } diff --git a/Test/Unit/Controller/Payment/SetupTest.php b/Test/Unit/Controller/Payment/SetupTest.php index 3f60b37..435b83d 100644 --- a/Test/Unit/Controller/Payment/SetupTest.php +++ b/Test/Unit/Controller/Payment/SetupTest.php @@ -111,7 +111,8 @@ private function createController(): Setup $this->createMock(LoggerInterface::class), $this->paystackClient, $this->transactionValidator, - $this->createMock(PaymentSettlement::class) + $this->createMock(PaymentSettlement::class), + $this->createMock(\Pstk\Paystack\Model\WebhookOrderResolver::class) ); } diff --git a/Test/Unit/Controller/Payment/WebhookTest.php b/Test/Unit/Controller/Payment/WebhookTest.php index 1b80fbe..07183af 100644 --- a/Test/Unit/Controller/Payment/WebhookTest.php +++ b/Test/Unit/Controller/Payment/WebhookTest.php @@ -10,6 +10,7 @@ use Pstk\Paystack\Gateway\Validator\TransactionValidator; use Pstk\Paystack\Model\Payment\Paystack; use Pstk\Paystack\Model\PaymentSettlement; +use Pstk\Paystack\Model\WebhookOrderResolver; use Pstk\Paystack\Model\Ui\ConfigProvider; use Magento\Framework\Api\SearchCriteriaBuilder; use Magento\Framework\Api\SearchCriteriaInterface; @@ -62,6 +63,9 @@ class WebhookTest extends TestCase /** @var MockObject|TransactionRepositoryInterface */ private $transactionRepository; + /** @var array Constructor args of Webhook, so a test can swap the resolver */ + private $controllerArgs; + protected function setUp(): void { $this->paystackClient = $this->createMock(PaystackApiClient::class); @@ -123,7 +127,19 @@ protected function setUp(): void $this->createMock(LoggerInterface::class) ); - $this->controller = new Webhook( + // A real resolver over the same mocked repositories, so the existing + // tests keep driving lookups through `orderInterface`/`orderRepository` + // exactly as before; resolver-specific cases live in WebhookOrderResolverTest. + $webhookOrderResolver = new WebhookOrderResolver( + $this->orderRepository, + $searchCriteriaBuilder, + $this->transactionRepository, + $this->orderInterface, + new TransactionValidator($this->createMock(LoggerInterface::class)), + $this->createMock(LoggerInterface::class) + ); + + $this->controllerArgs = [ $context, $pageFactory, $this->orderRepository, @@ -138,18 +154,10 @@ protected function setUp(): void $this->logger, $this->paystackClient, new TransactionValidator($this->createMock(LoggerInterface::class)), - $paymentSettlement - ); - } - - protected function tearDown(): void - { - // The webhook's quoteId fallback reaches for ObjectManager::getInstance() - // directly. A test that primes the static instance to exercise that branch - // must not leak it into whichever test runs next in this process. - $reflection = new \ReflectionClass(\Magento\Framework\App\ObjectManager::class); - $property = $reflection->getProperty('_instance'); - $property->setValue(null, null); + $paymentSettlement, + $webhookOrderResolver + ]; + $this->controller = new Webhook(...$this->controllerArgs); } /** @@ -349,45 +357,37 @@ public function testChargeSuccessWithValidOrder(): void $this->controller->execute(); } + /** + * A lone order on the verify response's metadata.quoteId settles through + * the full gate — metadata comes from the re-verified response. + */ public function testChargeSuccessWithQuoteIdFallback(): void { - $rawBody = json_encode([ + $this->request->method('getContent')->willReturn(json_encode([ 'event' => 'charge.success', - 'data' => [ - 'status' => 'success', - 'reference' => 'PSK_ref123', - 'metadata' => ['quoteId' => '55'], - ], - ]); - - $this->request->method('getContent')->willReturn($rawBody); + 'data' => ['status' => 'success', 'reference' => 'PSK_ref123'], + ])); $this->request->method('getHeader')->willReturn('valid_sig'); $this->paystackClient->method('validateWebhookSignature')->willReturn(true); - - $verifyResponse = (object) [ - 'data' => (object) [ - 'reference' => 'PSK_ref123', - 'status' => 'success', - 'metadata' => (object) ['quoteId' => '55'], - ], - ]; - $this->paystackClient->method('verifyTransaction')->willReturn($verifyResponse); + $this->paystackClient->method('verifyTransaction')->willReturn((object) [ + 'data' => (object) $this->settledVerifyData('PSK_ref123', ['metadata' => (object) ['quoteId' => '55']]), + ]); $this->configProvider->method('getPublicKey')->willReturn('pk_test'); - // loadByIncrementId returns empty order (not found by reference) $emptyOrder = $this->createMock(\Magento\Sales\Model\Order::class); $emptyOrder->method('getId')->willReturn(null); $this->orderInterface->method('loadByIncrementId')->willReturn($emptyOrder); - // The webhook uses ObjectManager::getInstance() for the fallback path, which - // cannot be easily unit-tested here without priming the static instance (done - // separately below in testQuoteIdFallbackWithAmountMismatchDoesNotDispatch, the - // test that actually exercises this branch's settlement gate). This test is - // limited to the behavioral floor that holds regardless of how that call - // resolves: the lookup was attempted, and nothing dispatched or saved off it. - $this->orderInterface->expects($this->once())->method('loadByIncrementId'); - $this->eventManager->expects($this->never())->method('dispatch'); - $this->orderRepository->expects($this->never())->method('save'); + $order = $this->createSettledOrder('000000055'); + $this->stubQuoteOrders([$order]); + $this->orderRepository->method('get')->willReturn($order); + $this->orderRepository->expects($this->once())->method('save')->with($order); + $this->eventManager->expects($this->once()) + ->method('dispatch') + ->with('paystack_payment_verify_after', ['paystack_order' => $order]); + + $this->rawResult->expects($this->atLeastOnce())->method('setHttpResponseCode')->with(200); + $this->rawResult->expects($this->atLeastOnce())->method('setContents')->with('success'); $this->controller->execute(); } @@ -398,23 +398,18 @@ public function testChargeSuccessWithQuoteIdFallback(): void */ public function testQuoteIdFallbackWithAmountMismatchDoesNotDispatch(): void { - $rawBody = json_encode([ + $this->request->method('getContent')->willReturn(json_encode([ 'event' => 'charge.success', - 'data' => [ - 'status' => 'success', - 'reference' => 'PSK_ref456', - 'metadata' => ['quoteId' => '77'], - ], - ]); - - $this->request->method('getContent')->willReturn($rawBody); + 'data' => ['status' => 'success', 'reference' => 'PSK_ref456'], + ])); $this->request->method('getHeader')->willReturn('valid_sig'); $this->paystackClient->method('validateWebhookSignature')->willReturn(true); - - $verifyResponse = (object) [ - 'data' => (object) $this->settledVerifyData('PSK_ref456', ['amount' => 499998]), - ]; - $this->paystackClient->method('verifyTransaction')->willReturn($verifyResponse); + $this->paystackClient->method('verifyTransaction')->willReturn((object) [ + 'data' => (object) $this->settledVerifyData('PSK_ref456', [ + 'amount' => 499998, + 'metadata' => (object) ['quoteId' => '77'], + ]), + ]); $this->configProvider->method('getPublicKey')->willReturn('pk_test'); $emptyOrder = $this->createMock(\Magento\Sales\Model\Order::class); @@ -425,39 +420,152 @@ public function testQuoteIdFallbackWithAmountMismatchDoesNotDispatch(): void $order->expects($this->once()) ->method('addStatusToHistory') ->with('pending', $this->stringContains('amount_mismatch')); - - // OrderSearchResultInterface itself has no getFirstItem() — the concrete - // collection orderRepository->getList() actually returns does, which is what - // Webhook.php's fallback branch relies on. - $searchResult = $this->createMock(\Magento\Sales\Model\ResourceModel\Order\Collection::class); - $searchResult->method('getTotalCount')->willReturn(1); - $searchResult->method('getFirstItem')->willReturn($order); - $this->orderRepository->method('getList')->willReturn($searchResult); + $this->stubQuoteOrders([$order]); + $this->orderRepository->method('get')->willReturn($order); $this->orderRepository->expects($this->once())->method('save')->with($order); - $searchCriteriaBuilder = $this->createMock(\Magento\Framework\Api\SearchCriteriaBuilder::class); - $searchCriteriaBuilder->method('addFilter')->willReturnSelf(); - $searchCriteriaBuilder->method('create') - ->willReturn($this->createMock(\Magento\Framework\Api\SearchCriteria::class)); + $this->eventManager->expects($this->never())->method('dispatch'); + + $this->rawResult->expects($this->atLeastOnce())->method('setHttpResponseCode')->with(200); + $this->rawResult->expects($this->atLeastOnce())->method('setContents')->with('rejected'); + + $this->controller->execute(); + } + + /** + * The event payload is not what was re-verified: metadata present only + * there must not find an order. + */ + public function testEventOnlyMetadataIsIgnored(): void + { + $this->request->method('getContent')->willReturn(json_encode([ + 'event' => 'charge.success', + 'data' => [ + 'status' => 'success', + 'reference' => 'PSK_ref789', + 'metadata' => ['quoteId' => '55', 'orderId' => '9'], + ], + ])); + $this->request->method('getHeader')->willReturn('valid_sig'); + $this->paystackClient->method('validateWebhookSignature')->willReturn(true); + $this->paystackClient->method('verifyTransaction')->willReturn((object) [ + 'data' => (object) $this->settledVerifyData('PSK_ref789', ['paid_at' => date('c')]), + ]); - $objectManager = $this->createMock(\Magento\Framework\ObjectManagerInterface::class); - $objectManager->method('create') - ->with('Magento\Framework\Api\SearchCriteriaBuilder') - ->willReturn($searchCriteriaBuilder); - \Magento\Framework\App\ObjectManager::setInstance($objectManager); + $emptyOrder = $this->createMock(\Magento\Sales\Model\Order::class); + $emptyOrder->method('getId')->willReturn(null); + $this->orderInterface->method('loadByIncrementId')->willReturn($emptyOrder); + $this->orderRepository->expects($this->never())->method('getList'); + $this->orderRepository->expects($this->never())->method('save'); $this->eventManager->expects($this->never())->method('dispatch'); - $this->rawResult->expects($this->atLeastOnce()) - ->method('setHttpResponseCode') - ->with(200); - $this->rawResult->expects($this->atLeastOnce()) - ->method('setContents') - ->with('rejected'); + $this->rawResult->expects($this->atLeastOnce())->method('setHttpResponseCode')->with(503); + $this->rawResult->expects($this->atLeastOnce())->method('setContents')->with('order not found'); $this->controller->execute(); } + /** + * A resolver that finds nothing takes the existing order-not-found path: + * transient while recent, permanent once stale. + * + * @dataProvider resolverFindsNothingProvider + */ + public function testResolverReturningNullTakesOrderNotFoundPath(string $paidAt, int $expectedCode): void + { + $resolver = $this->createMock(WebhookOrderResolver::class); + $resolver->expects($this->once())->method('resolve')->with('ORDER_RN')->willReturn(null); + $args = $this->controllerArgs; + $args[15] = $resolver; + $controller = new Webhook(...$args); + + $this->request->method('getContent')->willReturn(json_encode([ + 'event' => 'charge.success', + 'data' => ['status' => 'success', 'reference' => 'ORDER_RN'], + ])); + $this->request->method('getHeader')->willReturn('valid_sig'); + $this->paystackClient->method('validateWebhookSignature')->willReturn(true); + $this->paystackClient->method('verifyTransaction')->willReturn((object) [ + 'data' => (object) $this->settledVerifyData('ORDER_RN', ['paid_at' => $paidAt]), + ]); + + $this->eventManager->expects($this->never())->method('dispatch'); + $this->orderRepository->expects($this->never())->method('save'); + + $this->rawResult->expects($this->atLeastOnce())->method('setHttpResponseCode')->with($expectedCode); + $this->rawResult->expects($this->atLeastOnce())->method('setContents')->with('order not found'); + + $controller->execute(); + } + + public static function resolverFindsNothingProvider(): array + { + return [ + 'recent' => [date('c', time() - 60), 503], + 'stale' => [date('c', time() - 7200), 200], + ]; + } + + /** + * A lone order the quote lookup returns is passed to register() whatever + * its state, so a not-payable one is rejected there (ORDER_NOT_PAYABLE -> + * 503). Uses a HOLDED order rather than a canceled one: item F of the plan + * turns canceled into an acknowledged 200, while holded stays 503, so this + * test stays valid after F. + */ + public function testLoneNotPayableOrderOnQuoteIsRejectedNotPayable503(): void + { + $this->request->method('getContent')->willReturn(json_encode([ + 'event' => 'charge.success', + 'data' => ['status' => 'success', 'reference' => 'PSK_hold'], + ])); + $this->request->method('getHeader')->willReturn('valid_sig'); + $this->paystackClient->method('validateWebhookSignature')->willReturn(true); + $this->paystackClient->method('verifyTransaction')->willReturn((object) [ + 'data' => (object) $this->settledVerifyData('PSK_hold', ['metadata' => (object) ['quoteId' => '88']]), + ]); + + $emptyOrder = $this->createMock(\Magento\Sales\Model\Order::class); + $emptyOrder->method('getId')->willReturn(null); + $this->orderInterface->method('loadByIncrementId')->willReturn($emptyOrder); + + $order = $this->createMock(\Magento\Sales\Model\Order::class); + $order->method('getId')->willReturn(1); + $order->method('getEntityId')->willReturn(1); + $order->method('getIncrementId')->willReturn('000000088'); + $order->method('getState')->willReturn(Order::STATE_HOLDED); + $order->method('getBaseTotalDue')->willReturn(5000.00); + $payment = $this->createMock(\Magento\Sales\Model\Order\Payment::class); + $payment->method('getMethod')->willReturn(Paystack::CODE); + $order->method('getPayment')->willReturn($payment); + $order->method('getGrandTotal')->willReturn(5000.00); + $order->method('getOrderCurrencyCode')->willReturn('NGN'); + $order->method('getStatusHistories')->willReturn([]); + $this->stubQuoteOrders([$order]); + $this->orderRepository->method('get')->willReturn($order); + + $this->eventManager->expects($this->never())->method('dispatch'); + + $this->rawResult->expects($this->atLeastOnce())->method('setHttpResponseCode')->with(503); + $this->rawResult->expects($this->atLeastOnce())->method('setContents')->with('unverified'); + + $this->controller->execute(); + } + + /** + * Makes orderRepository->getList() return these orders, standing in for + * the resolver's quote_id lookup. + * + * @param MockObject[] $orders + */ + private function stubQuoteOrders(array $orders): void + { + $searchResult = $this->createMock(OrderSearchResultInterface::class); + $searchResult->method('getItems')->willReturn($orders); + $this->orderRepository->method('getList')->willReturn($searchResult); + } + public function testInvalidJsonPayloadReturnsInvalidPayload(): void { $this->request->method('getContent')->willReturn('not-json'); @@ -1266,7 +1374,7 @@ public function testOrderNotFoundByEitherLookupReturns503(): void $this->paystackClient->method('verifyTransaction')->willReturn($verifyResponse); $this->configProvider->method('getPublicKey')->willReturn('pk_test'); - // No metadata.quoteId, so the fallback lookup is never attempted. + // No verified metadata.quoteId, so the quote lookup is never attempted. $emptyOrder = $this->createMock(\Magento\Sales\Model\Order::class); $emptyOrder->method('getId')->willReturn(null); $this->orderInterface->method('loadByIncrementId')->willReturn($emptyOrder); @@ -1547,14 +1655,16 @@ public function testOrderNotPayableStaysTransientRegardlessOfAge(string $paidAt) $this->paystackClient->method('verifyTransaction')->willReturn($verifyResponse); $this->configProvider->method('getPublicKey')->willReturn('pk_test'); - // Canceled: not in STATE_NEW/STATE_PENDING_PAYMENT, so - // PaymentSettlement's order-state guard rejects with - // REASON_ORDER_NOT_PAYABLE. + // Held: not in STATE_NEW/STATE_PENDING_PAYMENT, so PaymentSettlement's + // order-state guard rejects with REASON_ORDER_NOT_PAYABLE. (A canceled + // order is now the terminal ORDER_CLOSED — see + // testCanceledOrderIsAcknowledgedOnceRejectionIsRecorded — so it no + // longer exercises the never-recency-bounded retry.) $order = $this->createMock(\Magento\Sales\Model\Order::class); $order->method('getId')->willReturn(1); $order->method('getEntityId')->willReturn(1); $order->method('getIncrementId')->willReturn('ORDER_047'); - $order->method('getState')->willReturn(Order::STATE_CANCELED); + $order->method('getState')->willReturn(Order::STATE_HOLDED); $order->method('getBaseTotalDue')->willReturn(5000.00); $payment = $this->createMock(\Magento\Sales\Model\Order\Payment::class); $payment->method('getMethod')->willReturn(Paystack::CODE); @@ -1584,6 +1694,46 @@ public static function orderNotPayableAgeProvider(): array ]; } + /** + * A charge for a canceled order can never settle it: once the rejection is + * durably on the order's history the webhook acknowledges (200) instead of + * having Paystack retry for its whole budget. + */ + public function testCanceledOrderIsAcknowledgedOnceRejectionIsRecorded(): void + { + $this->request->method('getContent')->willReturn(json_encode([ + 'event' => 'charge.success', + 'data' => ['status' => 'success', 'reference' => 'ORDER_048'], + ])); + $this->request->method('getHeader')->willReturn('valid_sig'); + $this->paystackClient->method('validateWebhookSignature')->willReturn(true); + $this->paystackClient->method('verifyTransaction')->willReturn((object) [ + 'data' => (object) $this->settledVerifyData('ORDER_048'), + ]); + + $order = $this->createMock(\Magento\Sales\Model\Order::class); + $order->method('getId')->willReturn(1); + $order->method('getEntityId')->willReturn(1); + $order->method('getIncrementId')->willReturn('ORDER_048'); + $order->method('getState')->willReturn(Order::STATE_CANCELED); + $order->method('getBaseTotalDue')->willReturn(5000.00); + $payment = $this->createMock(\Magento\Sales\Model\Order\Payment::class); + $payment->method('getMethod')->willReturn(Paystack::CODE); + $order->method('getPayment')->willReturn($payment); + $order->method('getGrandTotal')->willReturn(5000.00); + $order->method('getOrderCurrencyCode')->willReturn('NGN'); + $order->method('getStatusHistories')->willReturn([]); + $order->expects($this->once())->method('addStatusToHistory'); + $this->orderInterface->method('loadByIncrementId')->willReturn($order); + $this->orderRepository->expects($this->once())->method('save')->with($order); + + $this->eventManager->expects($this->never())->method('dispatch'); + $this->rawResult->expects($this->atLeastOnce())->method('setHttpResponseCode')->with(200); + $this->rawResult->expects($this->atLeastOnce())->method('setContents')->with('rejected'); + + $this->controller->execute(); + } + public function testDuplicateWebhookDeliveryWritesHistoryCommentOnlyOnce(): void { $rawBody = json_encode([ diff --git a/Test/Unit/Model/WebhookOrderResolverTest.php b/Test/Unit/Model/WebhookOrderResolverTest.php new file mode 100644 index 0000000..808ab54 --- /dev/null +++ b/Test/Unit/Model/WebhookOrderResolverTest.php @@ -0,0 +1,460 @@ + their field=>value filters */ + private $criteriaFilters; + + /** @var callable|null fn(array $filters): array — orders getList() returns */ + private $orderQuery; + + protected function setUp(): void + { + $this->orderRepository = $this->createMock(OrderRepositoryInterface::class); + $this->transactionRepository = $this->createMock(TransactionRepositoryInterface::class); + $this->orderInterface = $this->createMock(Order::class); + $this->orderInterface->method('getId')->willReturn(null); + $this->criteriaFilters = new \SplObjectStorage(); + + // Stateful like the real builder: filters accumulate until create(). + $builder = $this->createMock(SearchCriteriaBuilder::class); + $builder->method('addFilter')->willReturnCallback(function ($field, $value) use ($builder) { + $this->pendingFilters[$field] = $value; + return $builder; + }); + $builder->method('create')->willReturnCallback(function () { + $criteria = $this->createMock(SearchCriteriaInterface::class); + $this->criteriaFilters[$criteria] = $this->pendingFilters; + $this->pendingFilters = []; + return $criteria; + }); + + $this->givenNoBoundTransaction(); + $this->orderRepository->method('getList')->willReturnCallback(function ($criteria) { + $result = $this->createMock(OrderSearchResultInterface::class); + $result->method('getItems')->willReturn( + $this->orderQuery ? ($this->orderQuery)($this->criteriaFilters[$criteria]) : [] + ); + return $result; + }); + + $this->resolver = new WebhookOrderResolver( + $this->orderRepository, + $builder, + $this->transactionRepository, + $this->orderInterface, + new TransactionValidator($this->createMock(LoggerInterface::class)), + $this->createMock(LoggerInterface::class) + ); + } + + private function givenNoBoundTransaction(): void + { + $none = $this->createMock(TransactionSearchResultInterface::class); + $none->method('getItems')->willReturn([]); + $this->transactionRepository->method('getList')->willReturn($none); + } + + private function makeOrder( + int $id, + string $state = Order::STATE_NEW, + float $due = 100.0, + string $method = Paystack::CODE + ): MockObject { + $order = $this->createMock(Order::class); + $order->method('getId')->willReturn($id); + $order->method('getEntityId')->willReturn($id); + $order->method('getState')->willReturn($state); + $order->method('getBaseTotalDue')->willReturn($due); + $payment = $this->createMock(Order\Payment::class); + $payment->method('getMethod')->willReturn($method); + $order->method('getPayment')->willReturn($payment); + return $order; + } + + private function details(array $data): object + { + return json_decode(json_encode(['data' => $data])); + } + + private function withMetadata($metadata): object + { + return (object) ['data' => (object) ['reference' => 'PSK_1', 'metadata' => $metadata]]; + } + + public function testIncrementIdWinsWithoutFurtherLookups(): void + { + $order = $this->makeOrder(1); + $orderInterface = $this->createMock(Order::class); + $orderInterface->method('getId')->willReturn(1); + $resolver = $this->rebuildWith($orderInterface); + + $this->orderRepository->expects($this->never())->method('getList'); + $this->transactionRepository->expects($this->never())->method('getList'); + + $this->assertSame( + $orderInterface, + $resolver->resolve('000000001', $this->withMetadata((object) ['quoteId' => '5'])) + ); + } + + /** Same wiring as setUp() but with a different increment-id loader. */ + private function rebuildWith(MockObject $orderInterface): WebhookOrderResolver + { + $orderInterface->method('loadByIncrementId')->willReturnSelf(); + $builder = $this->createMock(SearchCriteriaBuilder::class); + $builder->method('addFilter')->willReturnSelf(); + $builder->method('create')->willReturn($this->createMock(SearchCriteriaInterface::class)); + return new WebhookOrderResolver( + $this->orderRepository, + $builder, + $this->transactionRepository, + $orderInterface, + new TransactionValidator($this->createMock(LoggerInterface::class)), + $this->createMock(LoggerInterface::class) + ); + } + + public function testBoundReferenceWinsOverMetadata(): void + { + $bound = $this->makeOrder(5, Order::STATE_PROCESSING, 0.0); + $txn = $this->createMock(TransactionInterface::class); + $txn->method('getOrderId')->willReturn(5); + $found = $this->createMock(TransactionSearchResultInterface::class); + $found->method('getItems')->willReturn([$txn]); + + // Fresh mock: setUp() already configured getList() with "no bindings". + $this->transactionRepository = $this->createMock(TransactionRepositoryInterface::class); + $this->transactionRepository->method('getList')->willReturn($found); + // Loaded via getList() (not get(), whose registry would hand register() + // this same instance back as its "fresh" re-fetch) — exactly one + // lookup, for the bound order; no metadata/quote lookups follow. + $boundResult = $this->createMock(OrderSearchResultInterface::class); + $boundResult->method('getItems')->willReturn([$bound]); + // Fresh mock: setUp() already stubbed getList() with no orders. + $this->orderRepository = $this->createMock(OrderRepositoryInterface::class); + $this->orderRepository->expects($this->never())->method('get'); + $this->orderRepository->expects($this->once())->method('getList')->willReturn($boundResult); + + $this->assertSame( + $bound, + $this->resolverWithTransactions()->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '9', 'orderId' => '7'])) + ); + } + + private function resolverWithTransactions(): WebhookOrderResolver + { + $builder = $this->createMock(SearchCriteriaBuilder::class); + $builder->method('addFilter')->willReturnSelf(); + $builder->method('create')->willReturn($this->createMock(SearchCriteriaInterface::class)); + return new WebhookOrderResolver( + $this->orderRepository, + $builder, + $this->transactionRepository, + $this->orderInterface, + new TransactionValidator($this->createMock(LoggerInterface::class)), + $this->createMock(LoggerInterface::class) + ); + } + + public function testOrderIdAndQuoteIdMatchReturnsThatOrderEvenWhenCanceled(): void + { + $cancelled = $this->makeOrder(12, Order::STATE_CANCELED); + $live = $this->makeOrder(13); + $seen = []; + $this->orderQuery = function (array $f) use ($cancelled, $live, &$seen) { + $seen[] = $f; + return ($f['entity_id'] ?? null) === '12' && ($f['quote_id'] ?? null) === '55' ? [$cancelled] : [$cancelled, $live]; + }; + + $this->assertSame( + $cancelled, + $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55', 'orderId' => '12'])) + ); + $this->assertCount(1, $seen, 'A step-3 hit must not go on to the quote lookup.'); + } + + public function testOrderIdNotMatchingQuoteFallsBackToQuotePath(): void + { + $live = $this->makeOrder(13); + $cancelled = $this->makeOrder(12, Order::STATE_CANCELED); + $this->orderQuery = function (array $f) use ($live, $cancelled) { + return isset($f['entity_id']) ? [] : [$cancelled, $live]; + }; + + $this->assertSame( + $live, + $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55', 'orderId' => '99'])) + ); + } + + public function testOrderIdMatchOnNonPaystackOrderFallsBackToQuotePath(): void + { + $other = $this->makeOrder(12, Order::STATE_NEW, 100.0, 'checkmo'); + $live = $this->makeOrder(13); + $this->orderQuery = function (array $f) use ($other, $live) { + return isset($f['entity_id']) ? [$other] : [$other, $live]; + }; + + $this->assertSame( + $live, + $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55', 'orderId' => '12'])) + ); + } + + public function testOrderIdNotFoundAndNothingOnQuoteReturnsNull(): void + { + $this->orderQuery = function (array $f) { + return []; + }; + + $this->assertNull( + $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55', 'orderId' => '12'])) + ); + } + + public function testOrderIdWithoutQuoteIdIsIgnoredAndReturnsNull(): void + { + $this->orderRepository->expects($this->never())->method('getList'); + + $this->assertNull($this->resolver->resolve('PSK_1', $this->withMetadata((object) ['orderId' => '12']))); + } + + public function testLoneOrderOnQuoteIsUsedInAnyStateAndMethod(): void + { + $lone = $this->makeOrder(3, Order::STATE_CANCELED, 0.0, 'checkmo'); + $this->orderQuery = function () use ($lone) { + return [$lone]; + }; + + $this->assertSame($lone, $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55']))); + } + + public function testCanceledPlusNewPicksTheNewOne(): void + { + $cancelled = $this->makeOrder(3, Order::STATE_CANCELED); + $live = $this->makeOrder(4, Order::STATE_PENDING_PAYMENT); + $this->orderQuery = function () use ($cancelled, $live) { + return [$cancelled, $live]; + }; + + $this->assertSame($live, $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55']))); + } + + public function testTwoPayableOrdersReturnNull(): void + { + $this->orderQuery = function () { + return [$this->makeOrder(3), $this->makeOrder(4)]; + }; + + $this->assertNull($this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55']))); + } + + public function testSeveralCanceledOrdersReturnNull(): void + { + $this->orderQuery = function () { + return [ + $this->makeOrder(3, Order::STATE_CANCELED), + $this->makeOrder(4, Order::STATE_CANCELED), + ]; + }; + + $this->assertNull($this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55']))); + } + + public function testPayableStateWithZeroDueIsExcluded(): void + { + $zeroDue = $this->makeOrder(3, Order::STATE_NEW, 0.0); + $live = $this->makeOrder(4); + $this->orderQuery = function () use ($zeroDue, $live) { + return [$zeroDue, $live]; + }; + $this->assertSame($live, $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55']))); + + $this->orderQuery = function () use ($zeroDue) { + return [$zeroDue, $this->makeOrder(5, Order::STATE_CANCELED)]; + }; + $this->assertNull($this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55']))); + } + + public function testNonPaystackPayableSiblingIsExcluded(): void + { + $live = $this->makeOrder(4); + $this->orderQuery = function () use ($live) { + return [$this->makeOrder(3, Order::STATE_NEW, 100.0, 'checkmo'), $live]; + }; + + $this->assertSame($live, $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55']))); + } + + /** + * @dataProvider metadataShapeProvider + */ + public function testMetadataShapes($metadata, bool $expectFound): void + { + $lone = $this->makeOrder(3); + $this->orderQuery = function () use ($lone) { + return [$lone]; + }; + + $result = $this->resolver->resolve('PSK_1', $this->withMetadata($metadata)); + + $expectFound ? $this->assertSame($lone, $result) : $this->assertNull($result); + } + + public static function metadataShapeProvider(): array + { + return [ + 'object' => [(object) ['quoteId' => '55'], true], + 'JSON string' => ['{"quoteId":"55"}', true], + 'empty string' => ['', false], + 'non-object JSON string' => ['"55"', false], + 'invalid JSON string' => ['{quoteId:', false], + 'list array' => [['quoteId' => '55'], false], + 'null' => [null, false], + 'int' => [55, false], + ]; + } + + public function testMissingMetadataReturnsNull(): void + { + $this->orderRepository->expects($this->never())->method('getList'); + + $this->assertNull($this->resolver->resolve('PSK_1', (object) ['data' => (object) ['reference' => 'PSK_1']])); + $this->assertNull($this->resolver->resolve('PSK_1', (object) [])); + } + + /** + * @dataProvider validIdProvider + */ + public function testValidQuoteIdForms($quoteId, string $expected): void + { + $lone = $this->makeOrder(3); + $seen = null; + $this->orderQuery = function (array $f) use ($lone, &$seen) { + $seen = $f; + return [$lone]; + }; + + $this->assertSame($lone, $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => $quoteId]))); + $this->assertSame(['quote_id' => $expected], $seen); + } + + public static function validIdProvider(): array + { + return [ + 'digit string' => ['12', '12'], + 'int' => [12, '12'], + ]; + } + + /** + * @dataProvider invalidIdProvider + */ + public function testInvalidQuoteIdIsRejectedWithoutAnyOrderLookup($quoteId): void + { + $this->orderRepository->expects($this->never())->method('getList'); + + $this->assertNull($this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => $quoteId]))); + } + + /** + * An invalid orderId is dropped (never coerced) and the quote lookup runs + * with the valid quoteId alone. + * + * @dataProvider invalidIdProvider + */ + public function testInvalidOrderIdSkipsStepThree($orderId): void + { + $lone = $this->makeOrder(3); + $queries = []; + $this->orderQuery = function (array $f) use ($lone, &$queries) { + $queries[] = $f; + return [$lone]; + }; + + $this->assertSame( + $lone, + $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55', 'orderId' => $orderId])) + ); + $this->assertSame([['quote_id' => '55']], $queries); + } + + public static function invalidIdProvider(): array + { + return [ + 'trailing junk' => ['12abc'], + 'exponent' => ['1e1'], + 'int zero' => [0], + 'string zero' => ['0'], + 'negative int' => [-1], + 'negative string' => ['-1'], + 'float' => [12.0], + 'bool' => [true], + 'leading zero' => ['012'], + 'array' => [[12]], + 'empty' => [''], + ]; + } + + public function testRepositoryErrorOnOrderIdLookupPropagates(): void + { + $this->orderQuery = function (array $f) { + throw new \RuntimeException('db gone away'); + }; + + $this->expectException(\RuntimeException::class); + $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55', 'orderId' => '12'])); + } + + public function testRepositoryErrorOnQuoteLookupPropagates(): void + { + $this->orderQuery = function (array $f) { + throw new \RuntimeException('db gone away'); + }; + + $this->expectException(\RuntimeException::class); + $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55'])); + } + + public function testTransactionRepositoryErrorPropagates(): void + { + $this->transactionRepository = $this->createMock(TransactionRepositoryInterface::class); + $this->transactionRepository->method('getList')->willThrowException(new \RuntimeException('db gone away')); + + $this->expectException(\RuntimeException::class); + $this->resolverWithTransactions()->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55'])); + } +} diff --git a/etc/frontend/di.xml b/etc/frontend/di.xml index e78bddd..2a5a167 100644 --- a/etc/frontend/di.xml +++ b/etc/frontend/di.xml @@ -28,6 +28,8 @@ Pstk\Paystack\Model\PaymentSettlement\Proxy + + Pstk\Paystack\Model\WebhookOrderResolver\Proxy From 15b1df8ff5b6a1a9005291c21e8a48fb6180fdb0 Mon Sep 17 00:00:00 2001 From: Jules Nsenda Date: Tue, 29 Sep 2026 15:42:46 +0200 Subject: [PATCH 5/9] fix(webhook): acknowledge a real charge's rejection only once it is recorded Diff review of #69: the record-before-ack rule lived inside the shared settlement service and only covered closed orders. register() now reports historyRecorded on every result and returns ORDER_CLOSED deterministically; the webhook acknowledges any permanent reason only when the rejection is on the order's history or no real money moved, and retries (503) otherwise. Also: closed-order history now says the payment was received but not applied (refund or reconcile) and logs at critical; the resolver skips a non-Paystack order bound to the reference and logs unresolvable quotes at error with the candidate orders; stale retry-policy docblock corrected. Co-Authored-By: Claude Opus 5.5 --- Controller/Payment/Webhook.php | 28 ++++-- Gateway/Validator/TransactionValidator.php | 37 +++++-- Model/PaymentSettlement.php | 98 +++++++++++-------- Model/WebhookOrderResolver.php | 32 +++++- Test/Unit/Controller/Payment/CallbackTest.php | 39 ++++++++ Test/Unit/Controller/Payment/WebhookTest.php | 83 +++++++++++++--- Test/Unit/Model/PaymentManagementTest.php | 39 +++++++- Test/Unit/Model/PaymentSettlementTest.php | 89 ++++++++++++++++- Test/Unit/Model/WebhookOrderResolverTest.php | 64 +++++++++++- 9 files changed, 431 insertions(+), 78 deletions(-) diff --git a/Controller/Payment/Webhook.php b/Controller/Payment/Webhook.php index 7c6e300..f397c24 100644 --- a/Controller/Payment/Webhook.php +++ b/Controller/Payment/Webhook.php @@ -51,14 +51,13 @@ class Webhook extends AbstractPaystackStandard * method mixup is never fixed by a retry either, but recording it here * (rather than as permanent) keeps the retry window open in case the * underlying relation was simply not yet hydrated. - * - REASON_ORDER_NOT_PAYABLE: a bank-transfer/USSD charge that is - * genuinely `pending` at callback time can settle minutes later via - * this webhook — if the customer used `/paystack/payment/recreate` in - * the meantime, the order is now `canceled`, and a late but genuine - * `charge.success` must not be permanently dropped (money captured - * with no order and no refund path). Deliberately NOT - * PERMANENT_FOR_WEBHOOK, unlike REASON_REFERENCE_BOUND_ELSEWHERE, which - * is not time-dependent. + * - REASON_ORDER_NOT_PAYABLE: a not-yet state (holded, payment_review, ...) + * that may become payable, so the charge keeps retrying. Closed orders + * (canceled/closed/complete, incl. cancelled via `/paystack/payment/recreate`) + * return REASON_ORDER_CLOSED instead and are acknowledged once the + * rejection is recorded (see TransactionValidator::REASON_ORDER_CLOSED). + * Deliberately NOT PERMANENT_FOR_WEBHOOK, unlike + * REASON_REFERENCE_BOUND_ELSEWHERE, which is not time-dependent. * - REASON_REGISTRATION_FAILED: a throw from PaymentSettlement::register()'s * own bind/register/save steps after every check already passed — a * transient DB/invoice issue should keep retrying, since money may @@ -253,7 +252,18 @@ private function resolveSettlement(object $transactionDetails, OrderInterface $o $isPermanentReason = $this->transactionValidator->isPermanentForWebhook($reason); if ($isPermanentReason) { - return [200, "rejected"]; + // Acknowledge only once the rejection is durably on the order's + // history: for a real charge that line is the merchant's only trace + // of money held with no order, so retry (503) until it is written. + // A charge that moved no real money (failed, or test domain) is not + // worth a ~72h retry over a lost history line. + if ($registration['historyRecorded'] + || !$this->transactionValidator->chargeIsReal($transactionDetails->data ?? null) + ) { + return [200, "rejected"]; + } + + return [503, "unverified"]; } if (in_array($reason, self::NEVER_RECENCY_BOUNDED, true)) { diff --git a/Gateway/Validator/TransactionValidator.php b/Gateway/Validator/TransactionValidator.php index c324a66..e7480ea 100644 --- a/Gateway/Validator/TransactionValidator.php +++ b/Gateway/Validator/TransactionValidator.php @@ -59,9 +59,8 @@ class TransactionValidator /** * The order is not payable right now, but the state may still change - * (holded, payment_review, ...) — or it is terminal (see - * REASON_ORDER_CLOSED) but the rejection history could not be durably - * recorded yet. Never RETRYABLE_FOR_CUSTOMER (money moved or the situation + * (holded, payment_review, ...). Terminal orders are REASON_ORDER_CLOSED, + * never this. Never RETRYABLE_FOR_CUSTOMER (money moved or the situation * is otherwise unrecoverable by retry — fail closed there), but * deliberately NOT PERMANENT_FOR_WEBHOOK, unlike * REASON_REFERENCE_BOUND_ELSEWHERE: a bank-transfer/USSD charge that is @@ -76,13 +75,13 @@ class TransactionValidator /** * The order can never take this money: canceled/closed/complete, or nothing * left due (e.g. already paid by another reference) — see - * isClosedForPayment(). Returned only once the rejection has been durably - * recorded on the order's history, so the webhook acknowledges (permanent, - * 200) instead of retrying for Paystack's ~72h budget, which risks endpoint - * back-off (industry standard: ack with 2xx once the problem is recorded). - * Never RETRYABLE_FOR_CUSTOMER — money moved, fail closed. When the history - * could not be recorded, PaymentSettlement returns REASON_ORDER_NOT_PAYABLE - * instead, so the webhook keeps retrying until it can be. + * isClosedForPayment(). Always returned for such an order (deterministic, + * whether or not the history write succeeded); PaymentSettlement's + * `historyRecorded` tells the webhook whether the rejection is durably on + * the order, and it acknowledges (200) only then — instead of retrying for + * Paystack's ~72h budget, which risks endpoint back-off (industry standard: + * ack with 2xx once the problem is recorded). Never RETRYABLE_FOR_CUSTOMER + * — money moved, fail closed. */ public const REASON_ORDER_CLOSED = 'order_closed'; @@ -271,6 +270,24 @@ private function requestedSubunits(object $verifyResponse): ?int return (int) $rawAmount; } + /** + * Whether a verify response's `data` describes a charge that actually moved + * money: a successful status on anything but the test domain. An unreadable + * `domain` counts as real — the safe side, since this only ever decides + * whether a lost history line is worth a retry. + * + * @param mixed $data The verify response's `data` property + * @return bool + */ + public function chargeIsReal($data): bool + { + if (!is_object($data) || ($data->status ?? null) !== 'success') { + return false; + } + + return ($data->domain ?? null) !== 'test'; + } + /** * @internal Direct use skips reference binding and payment registration — * callers should use `Model\PaymentSettlement::register()` instead. diff --git a/Model/PaymentSettlement.php b/Model/PaymentSettlement.php index a4785fc..592127c 100644 --- a/Model/PaymentSettlement.php +++ b/Model/PaymentSettlement.php @@ -89,9 +89,14 @@ public function __construct( * `settlementFailureReason()` against *its* instance, not against * whatever else may have written to this order since. * @param bool $testMode Must come from `PaystackApiClient::isTestMode()`. - * @return array{reason: ?string, order: OrderInterface} `reason` is null - * once bound/registered (or already idempotently bound); otherwise - * the first failing REASON_* constant. `order` is the settled + * @return array{reason: ?string, order: OrderInterface, historyRecorded: bool} + * `reason` is null once bound/registered (or already idempotently + * bound); otherwise the first failing REASON_* constant. + * `historyRecorded` is true on success/idempotent paths and when the + * rejection's history line is durably on the order; false when that + * write failed, or when nothing was recorded at all (early MALFORMED + * returns, REASON_IN_FLIGHT) — the webhook only acknowledges a + * permanent rejection of a real charge once this is true. `order` is the settled * instance this method actually mutated and saved (or, when nothing * could safely be validated/registered against, the best available * instance) — callers must dispatch/reference THIS instance, not @@ -112,7 +117,7 @@ public function register(object $transactionDetails, OrderInterface $order, bool $this->logger->warning('Paystack: settlement registration missing readable reference', [ 'order_increment_id' => $order->getIncrementId(), ]); - return ['reason' => TransactionValidator::REASON_MALFORMED, 'order' => $order]; + return ['reason' => TransactionValidator::REASON_MALFORMED, 'order' => $order, 'historyRecorded' => false]; } // Step 1: re-fetch fresh, guard the type, re-verify from scratch — @@ -129,7 +134,7 @@ public function register(object $transactionDetails, OrderInterface $order, bool 'reference' => $reference, 'order_increment_id' => $order->getIncrementId(), ]); - return ['reason' => TransactionValidator::REASON_MALFORMED, 'order' => $order]; + return ['reason' => TransactionValidator::REASON_MALFORMED, 'order' => $order, 'historyRecorded' => false]; } catch (\Throwable $exc) { // Genuinely transient (e.g. a DB hiccup) — fall through to the // type guard below, which falls back to the caller's own concrete @@ -148,7 +153,7 @@ public function register(object $transactionDetails, OrderInterface $order, bool $this->logger->warning('Paystack: settlement registration could not obtain a usable order instance', [ 'reference' => $reference, ]); - return ['reason' => TransactionValidator::REASON_MALFORMED, 'order' => $order]; + return ['reason' => TransactionValidator::REASON_MALFORMED, 'order' => $order, 'historyRecorded' => false]; } $reason = $this->transactionValidator->settlementFailureReason($transactionDetails, $freshOrder, $testMode); @@ -160,10 +165,11 @@ public function register(object $transactionDetails, OrderInterface $order, bool // would be noise, not signal — carried over from the deny-list // behavior Webhook.php's own recordHistory() used to have before // this was hoisted here. + $recorded = false; if ($reason !== TransactionValidator::REASON_IN_FLIGHT) { - $this->recordRejection($freshOrder, $reference, $reason, $transactionDetails); + $recorded = $this->recordRejection($freshOrder, $reference, $reason, $transactionDetails); } - return ['reason' => $reason, 'order' => $freshOrder]; + return ['reason' => $reason, 'order' => $freshOrder, 'historyRecorded' => $recorded]; } // Step 2: cross-order binding check — has this exact reference @@ -175,13 +181,17 @@ public function register(object $transactionDetails, OrderInterface $order, bool foreach ($existing as $item) { if ((int) $item->getOrderId() !== (int) $freshOrder->getEntityId()) { - $this->recordRejection( + $recorded = $this->recordRejection( $freshOrder, $reference, TransactionValidator::REASON_REFERENCE_BOUND_ELSEWHERE, $transactionDetails ); - return ['reason' => TransactionValidator::REASON_REFERENCE_BOUND_ELSEWHERE, 'order' => $freshOrder]; + return [ + 'reason' => TransactionValidator::REASON_REFERENCE_BOUND_ELSEWHERE, + 'order' => $freshOrder, + 'historyRecorded' => $recorded, + ]; } } @@ -196,7 +206,7 @@ public function register(object $transactionDetails, OrderInterface $order, bool 'reference' => $reference, 'order_increment_id' => $freshOrder->getIncrementId(), ]); - return ['reason' => null, 'order' => $freshOrder]; + return ['reason' => null, 'order' => $freshOrder, 'historyRecorded' => true]; } } @@ -205,30 +215,15 @@ public function register(object $transactionDetails, OrderInterface $order, bool // effect, so a re-verify of an already-advanced order must hit step // 3's no-op before this guard could mistake it for not-payable. if (!$this->transactionValidator->isPayable($freshOrder)) { - // A terminal order can never take this money, so once the - // rejection is durably on its history the webhook may acknowledge - // instead of retrying for ~72h. If it could not be recorded, stay - // ORDER_NOT_PAYABLE so the retry keeps trying to record it. - if ($this->transactionValidator->isClosedForPayment($freshOrder)) { - $recorded = $this->recordRejection( - $freshOrder, - $reference, - TransactionValidator::REASON_ORDER_CLOSED, - $transactionDetails - ); - $reason = $recorded - ? TransactionValidator::REASON_ORDER_CLOSED - : TransactionValidator::REASON_ORDER_NOT_PAYABLE; - return ['reason' => $reason, 'order' => $freshOrder]; - } - - $this->recordRejection( - $freshOrder, - $reference, - TransactionValidator::REASON_ORDER_NOT_PAYABLE, - $transactionDetails - ); - return ['reason' => TransactionValidator::REASON_ORDER_NOT_PAYABLE, 'order' => $freshOrder]; + // A terminal order can never take this money, so the reason is + // always ORDER_CLOSED (deterministic for the consumers); whether + // the webhook may acknowledge it depends on `historyRecorded`, so + // it retries until the rejection is durably on the order. + $reason = $this->transactionValidator->isClosedForPayment($freshOrder) + ? TransactionValidator::REASON_ORDER_CLOSED + : TransactionValidator::REASON_ORDER_NOT_PAYABLE; + $recorded = $this->recordRejection($freshOrder, $reference, $reason, $transactionDetails); + return ['reason' => $reason, 'order' => $freshOrder, 'historyRecorded' => $recorded]; } // Steps 5-7: bind + register + persist. Unguarded, a throw here (e.g. @@ -281,16 +276,20 @@ public function register(object $transactionDetails, OrderInterface $order, bool 'reference' => $reference, 'order_increment_id' => $freshOrder->getIncrementId(), ]); - $this->recordRejection( + $recorded = $this->recordRejection( $freshOrder, $reference, TransactionValidator::REASON_REGISTRATION_FAILED, $transactionDetails ); - return ['reason' => TransactionValidator::REASON_REGISTRATION_FAILED, 'order' => $freshOrder]; + return [ + 'reason' => TransactionValidator::REASON_REGISTRATION_FAILED, + 'order' => $freshOrder, + 'historyRecorded' => $recorded, + ]; } - return ['reason' => null, 'order' => $freshOrder]; + return ['reason' => null, 'order' => $freshOrder, 'historyRecorded' => true]; } /** @@ -331,7 +330,11 @@ private function recordRejection(Order $order, string $reference, string $reason $paidCurrency = is_scalar($rawCurrency) ? substr((string) $rawCurrency, 0, 100) : 'unknown'; $expectedSubunits = $this->transactionValidator->expectedSubunits($order); - $this->logger->warning('Paystack: settlement registration rejected', [ + // A closed order that took a real charge is the one rejection nobody + // will notice on their own — the money is with Paystack and no order + // holds it — so it is logged where an alert can see it. + $isClosed = TransactionValidator::REASON_ORDER_CLOSED === $reason; + $this->logger->{$isClosed ? 'critical' : 'warning'}('Paystack: settlement registration rejected', [ 'reason' => $reason, 'reference' => $reference, 'order_increment_id' => $order->getIncrementId(), @@ -345,6 +348,23 @@ private function recordRejection(Order $order, string $reference, string $reason // reason as decided. $isPermanent = $this->transactionValidator->isPermanentForWebhook($reason); + if ($isClosed) { + return $this->writeHistory( + $order, + $reference, + $reason, + sprintf( + 'Paystack: payment received but NOT applied — order is closed (%s): paid %s %s, expected %s %s, reference %s. Refund or reconcile this charge.', + $reason, + $paidAmount, + $paidCurrency, + $expectedSubunits, + $order->getOrderCurrencyCode(), + $reference + ) + ); + } + return $this->writeHistory( $order, $reference, diff --git a/Model/WebhookOrderResolver.php b/Model/WebhookOrderResolver.php index deb867f..6a65ff4 100644 --- a/Model/WebhookOrderResolver.php +++ b/Model/WebhookOrderResolver.php @@ -115,7 +115,9 @@ public function resolve(string $reference, object $transactionDetails): ?OrderIn ->create(); foreach ($this->transactionRepository->getList($searchCriteria)->getItems() as $transaction) { $boundOrder = $this->findOrder($transaction->getOrderId()); - if (null !== $boundOrder) { + // A binding on a non-Paystack order (e.g. another integration + // reused the reference) is not ours to settle against. + if (null !== $boundOrder && $this->transactionValidator->isPaystackOrder($boundOrder)) { return $boundOrder; } } @@ -144,6 +146,10 @@ public function resolve(string $reference, object $transactionDetails): ?OrderIn 'reference' => $reference, 'candidates' => count($matches), ]); + } else { + $this->logger->info('Paystack Webhook: no metadata.orderId, using quote lookup', [ + 'reference' => $reference, + ]); } // Step 4: quote fallback. @@ -170,7 +176,29 @@ function (OrderInterface $candidate): bool { 'payable' => count($payable), ]); - return 1 === count($payable) ? $payable[0] : null; + if (1 === count($payable)) { + return $payable[0]; + } + + if (count($candidates) > 0) { + // No order will be named for this charge, so the webhook + // acknowledges it later with nothing on any order — this line is + // the only way to find which orders it could have been for. + $this->logger->error('Paystack Webhook: quote has orders but none can be chosen for this charge', [ + 'reference' => $reference, + 'candidates' => array_map( + function (OrderInterface $candidate): array { + return [ + 'increment_id' => $candidate->getIncrementId(), + 'state' => $candidate->getState(), + ]; + }, + $candidates + ), + ]); + } + + return null; } /** diff --git a/Test/Unit/Controller/Payment/CallbackTest.php b/Test/Unit/Controller/Payment/CallbackTest.php index 696cc49..dfdc859 100644 --- a/Test/Unit/Controller/Payment/CallbackTest.php +++ b/Test/Unit/Controller/Payment/CallbackTest.php @@ -626,6 +626,45 @@ public function testWrongPaymentMethodDoesNotDispatchEvent(): void $controller->execute(); } + /** + * A charge for a canceled order comes back from register() as + * REASON_ORDER_CLOSED — a reason the customer copy has no branch for, so it + * gets the same fail-closed "do not pay again" message as + * REASON_ORDER_NOT_PAYABLE and never dispatches. + */ + public function testClosedOrderShowsTheSameFailClosedMessageAsNotPayable(): void + { + $controller = $this->createController(); + + $this->request->method('get')->willReturn('000000001'); + $this->paystackClient->method('verifyTransaction') + ->willReturn((object) ['data' => (object) $this->settledVerifyData('000000001')]); + + $order = $this->createMock(\Magento\Sales\Model\Order::class); + $order->method('getIncrementId')->willReturn('000000001'); + $order->method('getEntityId')->willReturn(1); + $order->method('getState')->willReturn(Order::STATE_CANCELED); + $order->method('getBaseTotalDue')->willReturn(5000.00); + $payment = $this->createMock(\Magento\Sales\Model\Order\Payment::class); + $payment->method('getMethod')->willReturn(\Pstk\Paystack\Model\Payment\Paystack::CODE); + $order->method('getPayment')->willReturn($payment); + $order->method('getGrandTotal')->willReturn(5000.00); + $order->method('getOrderCurrencyCode')->willReturn('NGN'); + $this->orderRepository->method('get')->with(1)->willReturn($order); + $this->orderInterface->method('loadByIncrementId')->willReturn($order); + + $expected = (new TransactionValidator($this->createMock(LoggerInterface::class))) + ->customerMessage(TransactionValidator::REASON_ORDER_NOT_PAYABLE); + + $this->eventManager->expects($this->never())->method('dispatch'); + $this->messageManager->expects($this->once()) + ->method('addErrorMessage') + ->with($expected); + $this->messageManager->expects($this->never())->method('addSuccessMessage'); + + $controller->execute(); + } + /** * A settled-looking `success` status against a zero/negative expected or paid * total must not settle the order — the fourth settlement-gate reason, diff --git a/Test/Unit/Controller/Payment/WebhookTest.php b/Test/Unit/Controller/Payment/WebhookTest.php index 07183af..8285d80 100644 --- a/Test/Unit/Controller/Payment/WebhookTest.php +++ b/Test/Unit/Controller/Payment/WebhookTest.php @@ -1486,15 +1486,21 @@ public static function missingDataPropertyProvider(): array } /** - * The signature-verified path writes a merchant-visible history comment on - * rejection. If that write itself throws — including a bare `\Error`, not just - * `\Exception` — it must not turn a clean permanent rejection into a 503: Paystack - * would retry a rejection retrying can never fix. + * A permanent rejection is acknowledged (200) only once its history line is + * durably recorded, unless the charge moved no real money. If the history + * write itself throws — including a bare `\Error`, not just `\Exception` — + * a real (live-domain) charge must keep retrying (503) so the merchant's + * only trace of the money is eventually written; a test-domain charge is + * not worth a retry over a lost history line (200). * * @dataProvider historyWriteFailureProvider */ - public function testHistoryWriteFailureDoesNotTurnRejectionIntoRetry(\Throwable $thrown): void - { + public function testUnrecordedRejectionOfRealChargeRetriesButTestChargeIsAcknowledged( + \Throwable $thrown, + string $domain, + int $expectedCode, + string $expectedBody + ): void { $rawBody = json_encode([ 'event' => 'charge.success', 'data' => [ @@ -1508,7 +1514,7 @@ public function testHistoryWriteFailureDoesNotTurnRejectionIntoRetry(\Throwable $this->paystackClient->method('validateWebhookSignature')->willReturn(true); $verifyResponse = (object) [ - 'data' => (object) $this->settledVerifyData('ORDER_040', ['amount' => 499998]), + 'data' => (object) $this->settledVerifyData('ORDER_040', ['amount' => 499998, 'domain' => $domain]), ]; $this->paystackClient->method('verifyTransaction')->willReturn($verifyResponse); $this->configProvider->method('getPublicKey')->willReturn('pk_test'); @@ -1521,10 +1527,10 @@ public function testHistoryWriteFailureDoesNotTurnRejectionIntoRetry(\Throwable $this->rawResult->expects($this->atLeastOnce()) ->method('setHttpResponseCode') - ->with(200); + ->with($expectedCode); $this->rawResult->expects($this->atLeastOnce()) ->method('setContents') - ->with('rejected'); + ->with($expectedBody); $this->controller->execute(); } @@ -1532,8 +1538,10 @@ public function testHistoryWriteFailureDoesNotTurnRejectionIntoRetry(\Throwable public static function historyWriteFailureProvider(): array { return [ - 'RuntimeException from save()' => [new \RuntimeException('deadlock')], - 'TypeError-shaped failure' => [new \TypeError('unexpected type')], + 'real charge, RuntimeException from save()' => [new \RuntimeException('deadlock'), 'live', 503, 'unverified'], + 'real charge, TypeError-shaped failure' => [new \TypeError('unexpected type'), 'live', 503, 'unverified'], + 'test-domain charge, RuntimeException from save()' => [new \RuntimeException('deadlock'), 'test', 200, 'rejected'], + 'test-domain charge, TypeError-shaped failure' => [new \TypeError('unexpected type'), 'test', 200, 'rejected'], ]; } @@ -1734,6 +1742,59 @@ public function testCanceledOrderIsAcknowledgedOnceRejectionIsRecorded(): void $this->controller->execute(); } + /** + * A canceled order whose ORDER_CLOSED history could not be saved is still + * ORDER_CLOSED, but the webhook only acknowledges it once recorded: a real + * charge retries (503), a test-domain one is acknowledged (200). + * + * @dataProvider closedOrderUnrecordedProvider + */ + public function testCanceledOrderWhoseHistoryCannotBeSavedFollowsChargeRealness( + string $domain, + int $expectedCode, + string $expectedBody + ): void { + $this->request->method('getContent')->willReturn(json_encode([ + 'event' => 'charge.success', + 'data' => ['status' => 'success', 'reference' => 'ORDER_049'], + ])); + $this->request->method('getHeader')->willReturn('valid_sig'); + $this->paystackClient->method('validateWebhookSignature')->willReturn(true); + $this->paystackClient->method('isTestMode')->willReturn('test' === $domain); + $this->paystackClient->method('verifyTransaction')->willReturn((object) [ + 'data' => (object) $this->settledVerifyData('ORDER_049', ['domain' => $domain]), + ]); + + $order = $this->createMock(\Magento\Sales\Model\Order::class); + $order->method('getId')->willReturn(1); + $order->method('getEntityId')->willReturn(1); + $order->method('getIncrementId')->willReturn('ORDER_049'); + $order->method('getState')->willReturn(Order::STATE_CANCELED); + $order->method('getBaseTotalDue')->willReturn(5000.00); + $payment = $this->createMock(\Magento\Sales\Model\Order\Payment::class); + $payment->method('getMethod')->willReturn(Paystack::CODE); + $order->method('getPayment')->willReturn($payment); + $order->method('getGrandTotal')->willReturn(5000.00); + $order->method('getOrderCurrencyCode')->willReturn('NGN'); + $order->method('getStatusHistories')->willReturn([]); + $this->orderInterface->method('loadByIncrementId')->willReturn($order); + $this->orderRepository->method('save')->willThrowException(new \RuntimeException('db down')); + + $this->eventManager->expects($this->never())->method('dispatch'); + $this->rawResult->expects($this->atLeastOnce())->method('setHttpResponseCode')->with($expectedCode); + $this->rawResult->expects($this->atLeastOnce())->method('setContents')->with($expectedBody); + + $this->controller->execute(); + } + + public static function closedOrderUnrecordedProvider(): array + { + return [ + 'live charge retries until recorded' => ['live', 503, 'unverified'], + 'test-domain charge is acknowledged' => ['test', 200, 'rejected'], + ]; + } + public function testDuplicateWebhookDeliveryWritesHistoryCommentOnlyOnce(): void { $rawBody = json_encode([ diff --git a/Test/Unit/Model/PaymentManagementTest.php b/Test/Unit/Model/PaymentManagementTest.php index b0434de..8ca3b05 100644 --- a/Test/Unit/Model/PaymentManagementTest.php +++ b/Test/Unit/Model/PaymentManagementTest.php @@ -96,9 +96,15 @@ protected function setUp(): void * @param string $quoteId * @param float $grandTotal * @param string $currencyCode + * @param string $state * @return MockObject|\Magento\Sales\Model\Order */ - private function stubMatchingOrder(string $quoteId, float $grandTotal = 5000.00, string $currencyCode = 'NGN') + private function stubMatchingOrder( + string $quoteId, + float $grandTotal = 5000.00, + string $currencyCode = 'NGN', + string $state = Order::STATE_NEW + ) { $lastOrder = $this->createMock(\Magento\Sales\Model\Order::class); $lastOrder->method('getIncrementId')->willReturn('000000001'); @@ -116,7 +122,7 @@ private function stubMatchingOrder(string $quoteId, float $grandTotal = 5000.00, $order->method('getEntityId')->willReturn(1); // PaymentSettlement::register()'s order-state guard: payable by // default so the settled-order tests reach registration. - $order->method('getState')->willReturn(Order::STATE_NEW); + $order->method('getState')->willReturn($state); $order->method('getBaseTotalDue')->willReturn($grandTotal); $this->orderInterface->method('loadByIncrementId') @@ -489,6 +495,35 @@ public function testVerifyPaymentAmountBelowToleranceWindowRejected(): void $this->assertTrue($result['final'], 'Terminal: the customer must not be invited to pay again.'); } + /** + * A charge for a canceled order comes back from register() as + * REASON_ORDER_CLOSED: terminal for the customer, with the same fail-closed + * copy as REASON_ORDER_NOT_PAYABLE, and nothing dispatched. + */ + public function testVerifyPaymentClosedOrderIsTerminalWithNotPayableMessage(): void + { + $quoteId = '42'; + $reference = 'PSK_abc123_-~-_' . $quoteId; + + $txData = $this->buildTxData('success', $quoteId); + $this->paystackClient->method('verifyTransaction')->willReturn((object) ['data' => $txData]); + + $this->stubMatchingOrder($quoteId, 5000.00, 'NGN', Order::STATE_CANCELED); + + $this->eventManager->expects($this->never())->method('dispatch'); + + $result = json_decode($this->paymentManagement->verifyPayment($reference), true); + + $this->assertFalse($result['status']); + $this->assertEquals(TransactionValidator::REASON_ORDER_CLOSED, $result['reason']); + $this->assertTrue($result['final'], 'Terminal: the customer must not be invited to pay again.'); + $this->assertSame( + (new TransactionValidator($this->createMock(LoggerInterface::class))) + ->customerMessage(TransactionValidator::REASON_ORDER_NOT_PAYABLE), + $result['message'] + ); + } + public function testVerifyPaymentCurrencyMismatchRejected(): void { $quoteId = '42'; diff --git a/Test/Unit/Model/PaymentSettlementTest.php b/Test/Unit/Model/PaymentSettlementTest.php index 1c33456..e75233f 100644 --- a/Test/Unit/Model/PaymentSettlementTest.php +++ b/Test/Unit/Model/PaymentSettlementTest.php @@ -167,6 +167,7 @@ public function testSuccessfulRegistrationBindsAndCaptures(): void ); $this->assertNull($result['reason']); + $this->assertTrue($result['historyRecorded']); // The settled instance a caller must dispatch/reference — not the // stale $order it called register() with. $this->assertSame($freshOrder, $result['order']); @@ -197,6 +198,7 @@ public function testSettlementFailureIsRejectedAndRecordedAgainstFreshOrder(): v ); $this->assertSame(TransactionValidator::REASON_WRONG_METHOD, $result['reason']); + $this->assertTrue($result['historyRecorded']); $this->assertSame($freshOrder, $result['order']); } @@ -221,6 +223,7 @@ public function testInFlightReasonIsReturnedWithoutRecordingHistory(): void ); $this->assertSame(TransactionValidator::REASON_IN_FLIGHT, $result['reason']); + $this->assertFalse($result['historyRecorded']); } /** @@ -249,6 +252,7 @@ public function testReferenceBoundToDifferentOrderIsRejected(): void ); $this->assertSame(TransactionValidator::REASON_REFERENCE_BOUND_ELSEWHERE, $result['reason']); + $this->assertTrue($result['historyRecorded']); $this->assertSame($freshOrder, $result['order']); } @@ -283,6 +287,7 @@ public function testIdempotentReVerifyOfSameOrderIsANoOp(): void ); $this->assertNull($result['reason']); + $this->assertTrue($result['historyRecorded']); $this->assertSame($freshOrder, $result['order']); } @@ -323,6 +328,7 @@ public function testDifferentReferenceOnAlreadyRegisteredOrderIsNotTreatedAsIdem ); $this->assertSame(TransactionValidator::REASON_ORDER_NOT_PAYABLE, $result['reason']); + $this->assertTrue($result['historyRecorded']); } /** @@ -348,6 +354,7 @@ public function testTerminalOrderStateIsRejectedAsOrderClosed(string $state): vo ); $this->assertSame(TransactionValidator::REASON_ORDER_CLOSED, $result['reason']); + $this->assertTrue($result['historyRecorded']); $this->assertSame($freshOrder, $result['order']); } @@ -382,6 +389,7 @@ public function testNonTerminalNotPayableStateStaysOrderNotPayable(string $state ); $this->assertSame(TransactionValidator::REASON_ORDER_NOT_PAYABLE, $result['reason']); + $this->assertTrue($result['historyRecorded']); } public static function recoverableNotPayableStateProvider(): array @@ -393,10 +401,11 @@ public static function recoverableNotPayableStateProvider(): array } /** - * Terminal, but the rejection history could not be saved: the webhook must - * keep retrying until it can be recorded, so this is ORDER_NOT_PAYABLE. + * Terminal, but the rejection history could not be saved: the reason stays + * the deterministic ORDER_CLOSED; `historyRecorded` false tells the webhook + * to keep retrying until it can be recorded. */ - public function testTerminalOrderWhoseHistoryCannotBeSavedStaysOrderNotPayable(): void + public function testTerminalOrderWhoseHistoryCannotBeSavedIsClosedButNotRecorded(): void { $this->noExistingBindings(); @@ -409,7 +418,71 @@ public function testTerminalOrderWhoseHistoryCannotBeSavedStaysOrderNotPayable() true ); - $this->assertSame(TransactionValidator::REASON_ORDER_NOT_PAYABLE, $result['reason']); + $this->assertSame(TransactionValidator::REASON_ORDER_CLOSED, $result['reason']); + $this->assertFalse($result['historyRecorded']); + } + + /** + * A failing history read or append on a terminal order is swallowed the + * same way: ORDER_CLOSED, not recorded, and nothing to save. + * + * @dataProvider historyWriteThrowsProvider + */ + public function testTerminalOrderWhoseHistoryCannotBeWrittenIsClosedButNotRecorded(string $failingMethod): void + { + $this->noExistingBindings(); + + $order = $this->makeOrder($this->makePaystackPayment(), 1, Order::STATE_CANCELED); + $freshOrder = $this->orderRepository->get(1); + $freshOrder->method($failingMethod)->willThrowException(new \RuntimeException('history broken')); + $this->orderRepository->expects($this->never())->method('save'); + + $result = $this->paymentSettlement->register( + (object) ['data' => $this->makeVerifyData()], + $order, + true + ); + + $this->assertSame(TransactionValidator::REASON_ORDER_CLOSED, $result['reason']); + $this->assertFalse($result['historyRecorded']); + } + + public static function historyWriteThrowsProvider(): array + { + return [ + 'getStatusHistories throws' => ['getStatusHistories'], + 'addStatusToHistory throws' => ['addStatusToHistory'], + ]; + } + + /** + * ORDER_CLOSED gets its own wording: the charge was received but applied + * nowhere, so the comment says so and asks for a refund/reconcile — and it + * is logged at critical. The dedupe marker is unchanged. + */ + public function testOrderClosedHistoryCommentSaysPaymentWasNotAppliedAndLogsCritical(): void + { + $this->noExistingBindings(); + + $order = $this->makeOrder($this->makePaystackPayment(), 1, Order::STATE_CANCELED); + $freshOrder = $this->orderRepository->get(1); + + $freshOrder->expects($this->once()) + ->method('addStatusToHistory') + ->with( + $this->anything(), + $this->logicalAnd( + $this->stringContains('Paystack: payment received but NOT applied — order is closed (order_closed): paid 10000 NGN, expected 10000 NGN, reference PSK_ref_123. Refund or reconcile this charge.'), + $this->stringContains('[paystack:PSK_ref_123:order_closed]') + ) + ); + $this->logger->expects($this->once())->method('critical'); + + $this->paymentSettlement->register( + (object) ['data' => $this->makeVerifyData()], + $order, + true + ); } /** @@ -439,6 +512,7 @@ public function testTerminalOrderWithMarkerAlreadyPresentIsClosedWithoutRewritin ); $this->assertSame(TransactionValidator::REASON_ORDER_CLOSED, $result['reason']); + $this->assertTrue($result['historyRecorded']); } public function testZeroBaseTotalDueIsRejectedAsOrderClosed(): void @@ -480,6 +554,7 @@ public function testZeroBaseTotalDueIsRejectedAsOrderClosed(): void ); $this->assertSame(TransactionValidator::REASON_ORDER_CLOSED, $result['reason']); + $this->assertTrue($result['historyRecorded']); } /** @@ -509,6 +584,7 @@ public function testNonConcreteOrderInterfaceInputReturnsMalformedNotFatal(): vo ); $this->assertSame(TransactionValidator::REASON_MALFORMED, $result['reason']); + $this->assertFalse($result['historyRecorded']); } /** @@ -546,6 +622,7 @@ public function testFreshRefetchNoSuchEntityDoesNotFallBackAndReturnsMalformed() ); $this->assertSame(TransactionValidator::REASON_MALFORMED, $result['reason']); + $this->assertFalse($result['historyRecorded']); } /** @@ -582,6 +659,7 @@ public function testFreshRefetchTransientThrowFallsBackToCallersConcreteOrder(): ); $this->assertNull($result['reason']); + $this->assertTrue($result['historyRecorded']); $this->assertSame($order, $result['order']); } @@ -609,6 +687,7 @@ public function testOverpaymentIsAcceptedAndRecordedInOneSave(): void ); $this->assertNull($result['reason']); + $this->assertTrue($result['historyRecorded']); } /** @@ -641,6 +720,7 @@ public function testRegistrationThrowingIsCaughtRecordedAndReturnsRegistrationFa ); $this->assertSame(TransactionValidator::REASON_REGISTRATION_FAILED, $result['reason']); + $this->assertTrue($result['historyRecorded']); $this->assertSame($freshOrder, $result['order']); } @@ -674,5 +754,6 @@ public function testDuplicateRejectionDoesNotAppendOrSaveAgain(): void ); $this->assertSame(TransactionValidator::REASON_WRONG_METHOD, $result['reason']); + $this->assertTrue($result['historyRecorded']); } } diff --git a/Test/Unit/Model/WebhookOrderResolverTest.php b/Test/Unit/Model/WebhookOrderResolverTest.php index 808ab54..8d03026 100644 --- a/Test/Unit/Model/WebhookOrderResolverTest.php +++ b/Test/Unit/Model/WebhookOrderResolverTest.php @@ -40,6 +40,9 @@ class WebhookOrderResolverTest extends TestCase /** @var callable|null fn(array $filters): array — orders getList() returns */ private $orderQuery; + /** @var MockObject|LoggerInterface */ + private $logger; + protected function setUp(): void { $this->orderRepository = $this->createMock(OrderRepositoryInterface::class); @@ -47,6 +50,7 @@ protected function setUp(): void $this->orderInterface = $this->createMock(Order::class); $this->orderInterface->method('getId')->willReturn(null); $this->criteriaFilters = new \SplObjectStorage(); + $this->logger = $this->createMock(LoggerInterface::class); // Stateful like the real builder: filters accumulate until create(). $builder = $this->createMock(SearchCriteriaBuilder::class); @@ -76,7 +80,7 @@ protected function setUp(): void $this->transactionRepository, $this->orderInterface, new TransactionValidator($this->createMock(LoggerInterface::class)), - $this->createMock(LoggerInterface::class) + $this->logger ); } @@ -252,6 +256,64 @@ public function testOrderIdWithoutQuoteIdIsIgnoredAndReturnsNull(): void $this->assertNull($this->resolver->resolve('PSK_1', $this->withMetadata((object) ['orderId' => '12']))); } + public function testMissingOrderIdLogsQuoteLookupFallback(): void + { + $lone = $this->makeOrder(3); + $this->orderQuery = function () use ($lone) { + return [$lone]; + }; + $messages = []; + $this->logger->method('info')->willReturnCallback(function (string $message) use (&$messages): void { + $messages[] = $message; + }); + $this->logger->expects($this->never())->method('error'); + + $this->assertSame($lone, $this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55']))); + $this->assertContains('Paystack Webhook: no metadata.orderId, using quote lookup', $messages); + } + + public function testUnresolvableQuoteCandidatesAreLoggedAtError(): void + { + // Two canceled siblings: neither is payable, so none can be chosen. + $first = $this->makeOrder(3, Order::STATE_CANCELED); + $second = $this->makeOrder(4, Order::STATE_CANCELED); + $this->orderQuery = function () use ($first, $second) { + return [$first, $second]; + }; + $this->logger->expects($this->once()) + ->method('error') + ->with( + $this->stringContains('none can be chosen'), + $this->callback(function (array $context): bool { + return 'PSK_1' === $context['reference'] && 2 === count($context['candidates']); + }) + ); + + $this->assertNull($this->resolver->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55']))); + } + + public function testBoundReferenceOnNonPaystackOrderIsSkipped(): void + { + $bound = $this->makeOrder(5, Order::STATE_PROCESSING, 0.0, 'checkmo'); + $txn = $this->createMock(TransactionInterface::class); + $txn->method('getOrderId')->willReturn(5); + $found = $this->createMock(TransactionSearchResultInterface::class); + $found->method('getItems')->willReturn([$txn]); + + $this->transactionRepository = $this->createMock(TransactionRepositoryInterface::class); + $this->transactionRepository->method('getList')->willReturn($found); + $boundResult = $this->createMock(OrderSearchResultInterface::class); + $boundResult->method('getItems')->willReturn([$bound]); + $this->orderRepository = $this->createMock(OrderRepositoryInterface::class); + $this->orderRepository->expects($this->once())->method('getList')->willReturn($boundResult); + + // No usable metadata to fall through to, so skipping the binding + // leaves nothing to resolve — it must not settle a non-Paystack order. + $this->assertNull( + $this->resolverWithTransactions()->resolve('PSK_1', $this->withMetadata((object) [])) + ); + } + public function testLoneOrderOnQuoteIsUsedInAnyStateAndMethod(): void { $lone = $this->makeOrder(3, Order::STATE_CANCELED, 0.0, 'checkmo'); From 314ad8ee28ea9a3721aab5cfc5aac306079e150b Mon Sep 17 00:00:00 2001 From: Jules Nsenda Date: Tue, 29 Sep 2026 15:45:15 +0200 Subject: [PATCH 6/9] test: pin the #69 resolver and ack-once-recorded rules against mutation Adds the cases a mutation pass showed missing: an orphaned bound transaction falls through to the quote lookup, chargeIsReal's full table, and historyRecorded=false on the bound-elsewhere and registration-failed paths when the history save fails. Co-Authored-By: Claude Opus 5.5 --- .../Validator/TransactionValidatorTest.php | 23 ++++++++++++ Test/Unit/Model/PaymentSettlementTest.php | 37 +++++++++++++++++++ Test/Unit/Model/WebhookOrderResolverTest.php | 26 +++++++++++++ 3 files changed, 86 insertions(+) diff --git a/Test/Unit/Gateway/Validator/TransactionValidatorTest.php b/Test/Unit/Gateway/Validator/TransactionValidatorTest.php index d5737b6..b0b848f 100644 --- a/Test/Unit/Gateway/Validator/TransactionValidatorTest.php +++ b/Test/Unit/Gateway/Validator/TransactionValidatorTest.php @@ -1239,4 +1239,27 @@ public static function isClosedForPaymentProvider(): array 'payment_review, due' => [Order::STATE_PAYMENT_REVIEW, 5000.00, false], ]; } + + /** + * @dataProvider chargeIsRealProvider + */ + public function testChargeIsReal($data, bool $expected): void + { + $this->assertSame($expected, $this->validator->chargeIsReal($data)); + } + + public static function chargeIsRealProvider(): array + { + return [ + 'success, live' => [(object) ['status' => 'success', 'domain' => 'live'], true], + 'success, domain missing' => [(object) ['status' => 'success'], true], + 'success, domain null' => [(object) ['status' => 'success', 'domain' => null], true], + 'success, test domain' => [(object) ['status' => 'success', 'domain' => 'test'], false], + 'failed, live' => [(object) ['status' => 'failed', 'domain' => 'live'], false], + 'abandoned' => [(object) ['status' => 'abandoned', 'domain' => 'live'], false], + 'status missing' => [(object) ['domain' => 'live'], false], + 'not an object' => ['success', false], + 'null' => [null, false], + ]; + } } diff --git a/Test/Unit/Model/PaymentSettlementTest.php b/Test/Unit/Model/PaymentSettlementTest.php index e75233f..130b666 100644 --- a/Test/Unit/Model/PaymentSettlementTest.php +++ b/Test/Unit/Model/PaymentSettlementTest.php @@ -724,6 +724,43 @@ public function testRegistrationThrowingIsCaughtRecordedAndReturnsRegistrationFa $this->assertSame($freshOrder, $result['order']); } + public function testReferenceBoundElsewhereWhoseHistoryCannotBeSavedIsNotRecorded(): void + { + $this->transactionRepository->method('getList')->willReturn( + $this->searchResult([$this->transactionItem(999)]) + ); + $order = $this->makeOrder($this->makePaystackPayment(), 1); + $this->orderRepository->method('save')->willThrowException(new \RuntimeException('db down')); + + $result = $this->paymentSettlement->register( + (object) ['data' => $this->makeVerifyData()], + $order, + true + ); + + $this->assertSame(TransactionValidator::REASON_REFERENCE_BOUND_ELSEWHERE, $result['reason']); + $this->assertFalse($result['historyRecorded']); + } + + public function testRegistrationFailedWhoseHistoryCannotBeSavedIsNotRecorded(): void + { + $this->noExistingBindings(); + $payment = $this->makePaystackPayment(); + $payment->method('registerCaptureNotification') + ->willThrowException(new \RuntimeException('invoice creation failed')); + $order = $this->makeOrder($payment, 1, Order::STATE_NEW, 100.00); + $this->orderRepository->method('save')->willThrowException(new \RuntimeException('db down')); + + $result = $this->paymentSettlement->register( + (object) ['data' => $this->makeVerifyData()], + $order, + true + ); + + $this->assertSame(TransactionValidator::REASON_REGISTRATION_FAILED, $result['reason']); + $this->assertFalse($result['historyRecorded']); + } + /** * A retry of an already-recorded rejection must not append a duplicate * comment, and — since nothing new was written — must not save() either. diff --git a/Test/Unit/Model/WebhookOrderResolverTest.php b/Test/Unit/Model/WebhookOrderResolverTest.php index 8d03026..382802a 100644 --- a/Test/Unit/Model/WebhookOrderResolverTest.php +++ b/Test/Unit/Model/WebhookOrderResolverTest.php @@ -314,6 +314,32 @@ public function testBoundReferenceOnNonPaystackOrderIsSkipped(): void ); } + public function testOrphanBoundTransactionFallsThroughToQuoteLookup(): void + { + // A sales_payment_transaction names an order that no longer loads: + // step 2 must not return null or throw, it continues to the quote path. + $txn = $this->createMock(TransactionInterface::class); + $txn->method('getOrderId')->willReturn(404); + $found = $this->createMock(TransactionSearchResultInterface::class); + $found->method('getItems')->willReturn([$txn]); + $this->transactionRepository = $this->createMock(TransactionRepositoryInterface::class); + $this->transactionRepository->method('getList')->willReturn($found); + + $lone = $this->makeOrder(3, Order::STATE_NEW); + $empty = $this->createMock(OrderSearchResultInterface::class); + $empty->method('getItems')->willReturn([]); + $loneResult = $this->createMock(OrderSearchResultInterface::class); + $loneResult->method('getItems')->willReturn([$lone]); + $this->orderRepository = $this->createMock(OrderRepositoryInterface::class); + $this->orderRepository->expects($this->exactly(2))->method('getList') + ->willReturnOnConsecutiveCalls($empty, $loneResult); + + $this->assertSame( + $lone, + $this->resolverWithTransactions()->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '9'])) + ); + } + public function testLoneOrderOnQuoteIsUsedInAnyStateAndMethod(): void { $lone = $this->makeOrder(3, Order::STATE_CANCELED, 0.0, 'checkmo'); From 7f3c8620aec460fb36a543be4b46d3239afc9c27 Mon Sep 17 00:00:00 2001 From: Jules Nsenda Date: Tue, 29 Sep 2026 15:50:04 +0200 Subject: [PATCH 7/9] fix(settlement): closed-order history must not misdirect a refund Security re-review of the ack-once-recorded change: an order paid before reference binding existed has no transaction row, so a replay of its own successful charge reaches ORDER_CLOSED. The history now asks the merchant to refund only if the charge is not already reflected on the order. Critical is logged when a closed-order rejection is first recorded or when any history write fails, not on every replay. Co-Authored-By: Claude Opus 5.5 --- Model/PaymentSettlement.php | 52 +++++++++++++-------- Test/Unit/Model/PaymentSettlementTest.php | 56 ++++++++++++++++++++++- 2 files changed, 89 insertions(+), 19 deletions(-) diff --git a/Model/PaymentSettlement.php b/Model/PaymentSettlement.php index 592127c..47ed59e 100644 --- a/Model/PaymentSettlement.php +++ b/Model/PaymentSettlement.php @@ -330,15 +330,7 @@ private function recordRejection(Order $order, string $reference, string $reason $paidCurrency = is_scalar($rawCurrency) ? substr((string) $rawCurrency, 0, 100) : 'unknown'; $expectedSubunits = $this->transactionValidator->expectedSubunits($order); - // A closed order that took a real charge is the one rejection nobody - // will notice on their own — the money is with Paystack and no order - // holds it — so it is logged where an alert can see it. $isClosed = TransactionValidator::REASON_ORDER_CLOSED === $reason; - $this->logger->{$isClosed ? 'critical' : 'warning'}('Paystack: settlement registration rejected', [ - 'reason' => $reason, - 'reference' => $reference, - 'order_increment_id' => $order->getIncrementId(), - ]); // Two wordings, matching Webhook.php's pre-hoist recordHistory(): // "rejected" for reasons a retry can never fix, "not applied yet @@ -354,7 +346,7 @@ private function recordRejection(Order $order, string $reference, string $reason $reference, $reason, sprintf( - 'Paystack: payment received but NOT applied — order is closed (%s): paid %s %s, expected %s %s, reference %s. Refund or reconcile this charge.', + 'Paystack: payment received after this order was closed (%s): paid %s %s, expected %s %s, reference %s. If this charge is not already reflected on the order, refund or reconcile it.', $reason, $paidAmount, $paidCurrency, @@ -419,13 +411,21 @@ private function appendHistoryComment(Order $order, string $reference, string $r } /** - * Appends the history comment and immediately persists it — used only by - * the rejection path, which returns before reaching register()'s own - * single save. Skips the save entirely when nothing was actually - * appended (a dedupe no-op or a failed write) — the same behavior the - * webhook's original recordHistory() had. A failed save must not turn an - * already-decided rejection into a retry that can never fix it — log and - * continue. + * Appends the history comment, immediately persists it, and logs the + * rejection — used only by the rejection path, which returns before + * reaching register()'s own single save. Skips the save entirely when + * nothing was actually appended (a dedupe no-op or a failed write). A + * failed save is logged and reported through the return value, never + * thrown: the caller's `historyRecorded` tells the webhook whether the + * rejection is durably on the order, and it keeps retrying a real-money + * permanent rejection until it is (see Webhook::resolveSettlement). + * + * The log level is chosen here, once the outcome is known. A failed write + * is always critical — a real charge acknowledged later would otherwise + * leave no trace. A closed order that took a real charge is critical the + * first time it is recorded (the money is with Paystack and no order holds + * it, so an alert must see it) and info on replays of an already-recorded + * one; every other reason is a warning. * * @param Order $order * @param string $reference @@ -436,17 +436,33 @@ private function appendHistoryComment(Order $order, string $reference, string $r */ private function writeHistory(Order $order, string $reference, string $reasonKey, string $comment): bool { + $isClosed = TransactionValidator::REASON_ORDER_CLOSED === $reasonKey; + $context = [ + 'reason' => $reasonKey, + 'reference' => $reference, + 'order_increment_id' => $order->getIncrementId(), + ]; + $message = 'Paystack: settlement registration rejected'; + $appended = $this->appendHistoryComment($order, $reference, $reasonKey, $comment); if ($appended !== self::HISTORY_APPENDED) { - return $appended === self::HISTORY_PRESENT; + if ($appended === self::HISTORY_PRESENT) { + $this->logger->{$isClosed ? 'info' : 'warning'}($message, $context); + return true; + } + $this->logger->critical($message, $context); + return false; } try { $this->orderRepository->save($order); - return true; } catch (\Throwable $exc) { $this->logger->error('Paystack: failed to write order history', ['error' => $exc->getMessage()]); + $this->logger->critical($message, $context); return false; } + + $this->logger->{$isClosed ? 'critical' : 'warning'}($message, $context); + return true; } } diff --git a/Test/Unit/Model/PaymentSettlementTest.php b/Test/Unit/Model/PaymentSettlementTest.php index 130b666..7a05712 100644 --- a/Test/Unit/Model/PaymentSettlementTest.php +++ b/Test/Unit/Model/PaymentSettlementTest.php @@ -472,7 +472,7 @@ public function testOrderClosedHistoryCommentSaysPaymentWasNotAppliedAndLogsCrit ->with( $this->anything(), $this->logicalAnd( - $this->stringContains('Paystack: payment received but NOT applied — order is closed (order_closed): paid 10000 NGN, expected 10000 NGN, reference PSK_ref_123. Refund or reconcile this charge.'), + $this->stringContains('Paystack: payment received after this order was closed (order_closed): paid 10000 NGN, expected 10000 NGN, reference PSK_ref_123. If this charge is not already reflected on the order, refund or reconcile it.'), $this->stringContains('[paystack:PSK_ref_123:order_closed]') ) ); @@ -515,6 +515,60 @@ public function testTerminalOrderWithMarkerAlreadyPresentIsClosedWithoutRewritin $this->assertTrue($result['historyRecorded']); } + /** + * A replay of an already-recorded ORDER_CLOSED must not page again: no + * critical, an info instead. + */ + public function testOrderClosedReplayLogsInfoNotCritical(): void + { + $this->noExistingBindings(); + + $order = $this->makeOrder($this->makePaystackPayment(), 1, Order::STATE_CANCELED); + $freshOrder = $this->orderRepository->get(1); + + $existingHistory = $this->createMock(OrderStatusHistoryInterface::class); + $existingHistory->method('getComment')->willReturn('x [paystack:PSK_ref_123:order_closed]'); + $freshOrder->method('getStatusHistories')->willReturn([$existingHistory]); + + $this->logger->expects($this->never())->method('critical'); + $this->logger->expects($this->once())->method('info'); + + $this->paymentSettlement->register( + (object) ['data' => $this->makeVerifyData()], + $order, + true + ); + } + + /** + * A failed history write (save throws) is logged at critical with the + * allow-listed context, so an unrecorded real charge leaves a trace. + */ + public function testFailedHistoryWriteLogsCriticalWithAllowListedContext(): void + { + $this->noExistingBindings(); + + $order = $this->makeOrder($this->makePaystackPayment(), 1, Order::STATE_CANCELED); + $this->orderRepository->method('save')->willThrowException(new \RuntimeException('db down')); + + $this->logger->expects($this->once()) + ->method('critical') + ->with( + $this->anything(), + $this->callback(function (array $context): bool { + return array_keys($context) === ['reason', 'reference', 'order_increment_id'] + && $context['reason'] === TransactionValidator::REASON_ORDER_CLOSED + && $context['reference'] === 'PSK_ref_123'; + }) + ); + + $this->paymentSettlement->register( + (object) ['data' => $this->makeVerifyData()], + $order, + true + ); + } + public function testZeroBaseTotalDueIsRejectedAsOrderClosed(): void { $this->noExistingBindings(); From 7550ca96dcfb9445140d3b28f429b3c9d44e3f31 Mon Sep 17 00:00:00 2001 From: Jules Nsenda Date: Tue, 29 Sep 2026 15:55:41 +0200 Subject: [PATCH 8/9] refactor: simplify the #69 resolver and settlement history code One history sprintf instead of two near-identical blocks, the closed-order decision derived once and passed to writeHistory(), guard clauses instead of nested ifs, a findOrders() helper for the resolver's three order lookups, and the retry-policy rationale kept on the reason constants with pointers elsewhere. No behaviour change. Co-Authored-By: Claude Opus 5.5 --- Controller/Payment/Webhook.php | 10 +-- Gateway/Validator/TransactionValidator.php | 3 +- Model/PaymentSettlement.php | 75 ++++++++++---------- Model/WebhookOrderResolver.php | 29 ++++---- Test/Unit/Model/WebhookOrderResolverTest.php | 39 ++++------ 5 files changed, 70 insertions(+), 86 deletions(-) diff --git a/Controller/Payment/Webhook.php b/Controller/Payment/Webhook.php index f397c24..c6ee2a0 100644 --- a/Controller/Payment/Webhook.php +++ b/Controller/Payment/Webhook.php @@ -51,13 +51,9 @@ class Webhook extends AbstractPaystackStandard * method mixup is never fixed by a retry either, but recording it here * (rather than as permanent) keeps the retry window open in case the * underlying relation was simply not yet hydrated. - * - REASON_ORDER_NOT_PAYABLE: a not-yet state (holded, payment_review, ...) - * that may become payable, so the charge keeps retrying. Closed orders - * (canceled/closed/complete, incl. cancelled via `/paystack/payment/recreate`) - * return REASON_ORDER_CLOSED instead and are acknowledged once the - * rejection is recorded (see TransactionValidator::REASON_ORDER_CLOSED). - * Deliberately NOT PERMANENT_FOR_WEBHOOK, unlike - * REASON_REFERENCE_BOUND_ELSEWHERE, which is not time-dependent. + * - REASON_ORDER_NOT_PAYABLE: a not-yet state that keeps retrying; closed + * orders are REASON_ORDER_CLOSED instead (see the rationale on those two + * TransactionValidator constants). * - REASON_REGISTRATION_FAILED: a throw from PaymentSettlement::register()'s * own bind/register/save steps after every check already passed — a * transient DB/invoice issue should keep retrying, since money may diff --git a/Gateway/Validator/TransactionValidator.php b/Gateway/Validator/TransactionValidator.php index e7480ea..7ab622c 100644 --- a/Gateway/Validator/TransactionValidator.php +++ b/Gateway/Validator/TransactionValidator.php @@ -79,8 +79,7 @@ class TransactionValidator * whether or not the history write succeeded); PaymentSettlement's * `historyRecorded` tells the webhook whether the rejection is durably on * the order, and it acknowledges (200) only then — instead of retrying for - * Paystack's ~72h budget, which risks endpoint back-off (industry standard: - * ack with 2xx once the problem is recorded). Never RETRYABLE_FOR_CUSTOMER + * Paystack's ~72h budget, which risks endpoint back-off. Never RETRYABLE_FOR_CUSTOMER * — money moved, fail closed. */ public const REASON_ORDER_CLOSED = 'order_closed'; diff --git a/Model/PaymentSettlement.php b/Model/PaymentSettlement.php index 47ed59e..4386b64 100644 --- a/Model/PaymentSettlement.php +++ b/Model/PaymentSettlement.php @@ -96,7 +96,8 @@ public function __construct( * rejection's history line is durably on the order; false when that * write failed, or when nothing was recorded at all (early MALFORMED * returns, REASON_IN_FLIGHT) — the webhook only acknowledges a - * permanent rejection of a real charge once this is true. `order` is the settled + * rejection of a real charge once this is true (see + * TransactionValidator::REASON_ORDER_CLOSED). `order` is the settled * instance this method actually mutated and saved (or, when nothing * could safely be validated/registered against, the best available * instance) — callers must dispatch/reference THIS instance, not @@ -215,10 +216,8 @@ public function register(object $transactionDetails, OrderInterface $order, bool // effect, so a re-verify of an already-advanced order must hit step // 3's no-op before this guard could mistake it for not-payable. if (!$this->transactionValidator->isPayable($freshOrder)) { - // A terminal order can never take this money, so the reason is - // always ORDER_CLOSED (deterministic for the consumers); whether - // the webhook may acknowledge it depends on `historyRecorded`, so - // it retries until the rejection is durably on the order. + // Terminal vs not-yet: see TransactionValidator::REASON_ORDER_CLOSED + // / REASON_ORDER_NOT_PAYABLE. $reason = $this->transactionValidator->isClosedForPayment($freshOrder) ? TransactionValidator::REASON_ORDER_CLOSED : TransactionValidator::REASON_ORDER_NOT_PAYABLE; @@ -332,29 +331,21 @@ private function recordRejection(Order $order, string $reference, string $reason $isClosed = TransactionValidator::REASON_ORDER_CLOSED === $reason; - // Two wordings, matching Webhook.php's pre-hoist recordHistory(): - // "rejected" for reasons a retry can never fix, "not applied yet - // (retry pending)" for reasons that may still resolve on their own - // (e.g. REASON_MODE_MISMATCH) — collapsing both to "rejected" - // regardless of permanence would misrepresent a still-retrying - // reason as decided. - $isPermanent = $this->transactionValidator->isPermanentForWebhook($reason); - if ($isClosed) { - return $this->writeHistory( - $order, - $reference, - $reason, - sprintf( - 'Paystack: payment received after this order was closed (%s): paid %s %s, expected %s %s, reference %s. If this charge is not already reflected on the order, refund or reconcile it.', - $reason, - $paidAmount, - $paidCurrency, - $expectedSubunits, - $order->getOrderCurrencyCode(), - $reference - ) - ); + $lead = sprintf('received after this order was closed (%s)', $reason); + $suffix = '. If this charge is not already reflected on the order, refund or reconcile it.'; + } else { + // Two wordings, matching Webhook.php's pre-hoist recordHistory(): + // "rejected" for reasons a retry can never fix, "not applied yet + // (retry pending)" for reasons that may still resolve on their own + // (e.g. REASON_MODE_MISMATCH) — collapsing both to "rejected" + // regardless of permanence would misrepresent a still-retrying + // reason as decided. + $verdict = $this->transactionValidator->isPermanentForWebhook($reason) + ? 'rejected' + : 'not applied (retry pending)'; + $lead = sprintf('%s — %s', $verdict, $reason); + $suffix = ''; } return $this->writeHistory( @@ -362,15 +353,16 @@ private function recordRejection(Order $order, string $reference, string $reason $reference, $reason, sprintf( - 'Paystack: payment %s — %s: paid %s %s, expected %s %s, reference %s', - $isPermanent ? 'rejected' : 'not applied (retry pending)', - $reason, + 'Paystack: payment %s: paid %s %s, expected %s %s, reference %s%s', + $lead, $paidAmount, $paidCurrency, $expectedSubunits, $order->getOrderCurrencyCode(), - $reference - ) + $reference, + $suffix + ), + $isClosed ); } @@ -431,12 +423,17 @@ private function appendHistoryComment(Order $order, string $reference, string $r * @param string $reference * @param string $reasonKey * @param string $comment + * @param bool $isClosed Whether the reason is REASON_ORDER_CLOSED — picks the log levels above. * @return bool True when the (reference, reason) record is durably on the * order: the marker was already present, or it was appended and saved. */ - private function writeHistory(Order $order, string $reference, string $reasonKey, string $comment): bool - { - $isClosed = TransactionValidator::REASON_ORDER_CLOSED === $reasonKey; + private function writeHistory( + Order $order, + string $reference, + string $reasonKey, + string $comment, + bool $isClosed + ): bool { $context = [ 'reason' => $reasonKey, 'reference' => $reference, @@ -445,11 +442,11 @@ private function writeHistory(Order $order, string $reference, string $reasonKey $message = 'Paystack: settlement registration rejected'; $appended = $this->appendHistoryComment($order, $reference, $reasonKey, $comment); + if ($appended === self::HISTORY_PRESENT) { + $this->logger->{$isClosed ? 'info' : 'warning'}($message, $context); + return true; + } if ($appended !== self::HISTORY_APPENDED) { - if ($appended === self::HISTORY_PRESENT) { - $this->logger->{$isClosed ? 'info' : 'warning'}($message, $context); - return true; - } $this->logger->critical($message, $context); return false; } diff --git a/Model/WebhookOrderResolver.php b/Model/WebhookOrderResolver.php index 6a65ff4..10268f0 100644 --- a/Model/WebhookOrderResolver.php +++ b/Model/WebhookOrderResolver.php @@ -134,11 +134,7 @@ public function resolve(string $reference, object $transactionDetails): ?OrderIn // Step 3: the exact order the popup placed, when the checkout sent it. $orderId = $this->parseId($metadata->orderId ?? null); if (null !== $orderId) { - $searchCriteria = $this->searchCriteriaBuilder - ->addFilter('entity_id', $orderId, 'eq') - ->addFilter('quote_id', $quoteId, 'eq') - ->create(); - $matches = array_values($this->orderRepository->getList($searchCriteria)->getItems()); + $matches = $this->findOrders(['entity_id' => $orderId, 'quote_id' => $quoteId]); if (1 === count($matches) && $this->transactionValidator->isPaystackOrder($matches[0])) { return $matches[0]; } @@ -153,10 +149,7 @@ public function resolve(string $reference, object $transactionDetails): ?OrderIn } // Step 4: quote fallback. - $searchCriteria = $this->searchCriteriaBuilder - ->addFilter('quote_id', $quoteId, 'eq') - ->create(); - $candidates = array_values($this->orderRepository->getList($searchCriteria)->getItems()); + $candidates = $this->findOrders(['quote_id' => $quoteId]); if (1 === count($candidates)) { return $candidates[0]; @@ -207,12 +200,20 @@ function (OrderInterface $candidate): array { */ private function findOrder($orderId): ?OrderInterface { - $searchCriteria = $this->searchCriteriaBuilder - ->addFilter('entity_id', $orderId, 'eq') - ->create(); - $items = array_values($this->orderRepository->getList($searchCriteria)->getItems()); + return $this->findOrders(['entity_id' => $orderId])[0] ?? null; + } + + /** + * @param array $filters field => value, all matched with 'eq' + * @return OrderInterface[] + */ + private function findOrders(array $filters): array + { + foreach ($filters as $field => $value) { + $this->searchCriteriaBuilder->addFilter($field, $value, 'eq'); + } - return $items[0] ?? null; + return array_values($this->orderRepository->getList($this->searchCriteriaBuilder->create())->getItems()); } /** diff --git a/Test/Unit/Model/WebhookOrderResolverTest.php b/Test/Unit/Model/WebhookOrderResolverTest.php index 382802a..468e440 100644 --- a/Test/Unit/Model/WebhookOrderResolverTest.php +++ b/Test/Unit/Model/WebhookOrderResolverTest.php @@ -123,7 +123,7 @@ public function testIncrementIdWinsWithoutFurtherLookups(): void $order = $this->makeOrder(1); $orderInterface = $this->createMock(Order::class); $orderInterface->method('getId')->willReturn(1); - $resolver = $this->rebuildWith($orderInterface); + $resolver = $this->buildResolver($orderInterface); $this->orderRepository->expects($this->never())->method('getList'); $this->transactionRepository->expects($this->never())->method('getList'); @@ -134,23 +134,6 @@ public function testIncrementIdWinsWithoutFurtherLookups(): void ); } - /** Same wiring as setUp() but with a different increment-id loader. */ - private function rebuildWith(MockObject $orderInterface): WebhookOrderResolver - { - $orderInterface->method('loadByIncrementId')->willReturnSelf(); - $builder = $this->createMock(SearchCriteriaBuilder::class); - $builder->method('addFilter')->willReturnSelf(); - $builder->method('create')->willReturn($this->createMock(SearchCriteriaInterface::class)); - return new WebhookOrderResolver( - $this->orderRepository, - $builder, - $this->transactionRepository, - $orderInterface, - new TransactionValidator($this->createMock(LoggerInterface::class)), - $this->createMock(LoggerInterface::class) - ); - } - public function testBoundReferenceWinsOverMetadata(): void { $bound = $this->makeOrder(5, Order::STATE_PROCESSING, 0.0); @@ -174,12 +157,20 @@ public function testBoundReferenceWinsOverMetadata(): void $this->assertSame( $bound, - $this->resolverWithTransactions()->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '9', 'orderId' => '7'])) + $this->buildResolver()->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '9', 'orderId' => '7'])) ); } - private function resolverWithTransactions(): WebhookOrderResolver + /** + * A resolver over the current repository mocks with a plain (non-stateful) + * builder — setUp()'s stateful one is only needed by tests that inspect + * filters. Pass $orderInterface to swap in a different increment-id loader. + */ + private function buildResolver(?MockObject $orderInterface = null): WebhookOrderResolver { + if ($orderInterface !== null) { + $orderInterface->method('loadByIncrementId')->willReturnSelf(); + } $builder = $this->createMock(SearchCriteriaBuilder::class); $builder->method('addFilter')->willReturnSelf(); $builder->method('create')->willReturn($this->createMock(SearchCriteriaInterface::class)); @@ -187,7 +178,7 @@ private function resolverWithTransactions(): WebhookOrderResolver $this->orderRepository, $builder, $this->transactionRepository, - $this->orderInterface, + $orderInterface ?? $this->orderInterface, new TransactionValidator($this->createMock(LoggerInterface::class)), $this->createMock(LoggerInterface::class) ); @@ -310,7 +301,7 @@ public function testBoundReferenceOnNonPaystackOrderIsSkipped(): void // No usable metadata to fall through to, so skipping the binding // leaves nothing to resolve — it must not settle a non-Paystack order. $this->assertNull( - $this->resolverWithTransactions()->resolve('PSK_1', $this->withMetadata((object) [])) + $this->buildResolver()->resolve('PSK_1', $this->withMetadata((object) [])) ); } @@ -336,7 +327,7 @@ public function testOrphanBoundTransactionFallsThroughToQuoteLookup(): void $this->assertSame( $lone, - $this->resolverWithTransactions()->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '9'])) + $this->buildResolver()->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '9'])) ); } @@ -543,6 +534,6 @@ public function testTransactionRepositoryErrorPropagates(): void $this->transactionRepository->method('getList')->willThrowException(new \RuntimeException('db gone away')); $this->expectException(\RuntimeException::class); - $this->resolverWithTransactions()->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55'])); + $this->buildResolver()->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '55'])); } } From d540ef81a89cdc53dbfbe3d612b4b5372d7fb5ac Mon Sep 17 00:00:00 2001 From: Jules Nsenda Date: Tue, 29 Sep 2026 15:55:41 +0200 Subject: [PATCH 9/9] docs: webhook order lookup and closed-order acknowledgement (#69) CHANGELOG Unreleased entry, Reference Manual order-lookup and retry semantics, a User Guide troubleshooting entry for the closed-order history comment, and CLAUDE.md's webhook flow and key classes. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 43 +++++++++++++++++++++++++++++ CLAUDE.md | 8 ++++-- marketplace/src/reference-manual.md | 17 ++++++++++-- marketplace/src/user-guide.md | 2 ++ 4 files changed, 64 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61490b1..f08a833 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,49 @@ This project adheres to [Semantic Versioning](https://semver.org/). The entries below cover every release since the last tag, **v3.0.10**. +## [Unreleased] + +### Fixed +- **Inline payments retried on the same cart now confirm via the webhook + (#69).** Closing the Paystack popup cancels the order and reuses its cart, + so a retry left several orders on one quote, and the webhook — which + required exactly one — left the paid order pending whenever it was the path + that confirmed payment (customer closed the tab, verify call failed). Order + lookup now lives in `Model/WebhookOrderResolver`: the reference as an + increment id, then an order the reference is already bound to, then the + order id the checkout now sends in the transaction metadata (matched with + its quote), then the quote — a lone order as before, otherwise the single + still-payable Paystack order, and never a guess among several. +- **A charge for an order that can no longer take it is acknowledged once + recorded, instead of retried for ~72 hours.** Canceled, closed or complete + orders, or orders with nothing left due, now return the new `order_closed` + reason: the webhook answers `200` once a history comment asking the + merchant to refund or reconcile the charge is saved on the order, and logs + it at `critical`. Long runs of `503` are what make Paystack back off or + disable an endpoint. Orders on hold or under payment review keep retrying. +- **A rejected real charge is never acknowledged without a record.** For any + decided rejection where real money moved, the webhook now retries (`503`) + until the order-history comment is saved, rather than answering `200` when + that write failed. + +### Changed +- The inline checkout sends the placed order's id as `metadata.orderId` on + the Paystack transaction. +- The inline REST verify endpoint and the Redirect callback report + `order_closed` (terminal, same customer message as `order_not_payable`) + for canceled/closed/complete or fully-paid orders. + +### Known limitations +- Transactions without `metadata.orderId` (started before this release, or + from checkouts that replace Magento's standard payment renderer, e.g. Hyvä + or one-step checkouts) still resolve by quote: a late bank-transfer/USSD + settlement for a cancelled attempt can bind to the live retry order on the + same quote. The planned move to server-side transaction initialization + removes client-supplied ids altogether. +- A quote with several orders of which none (or more than one) is payable + still ends in "order not found" — now logged at `error` with the candidate + orders, but without an order-history comment. + ## [3.1.0] - 2026-09-29 Every payment-verification path now confirms, from Paystack's verify response, diff --git a/CLAUDE.md b/CLAUDE.md index 47edd54..6b116b6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -80,7 +80,7 @@ Current tests (`Test/Mftf/Test/`): `PaystackPaymentConfigAvailableTest.xml` and ### Unit tests -There are 98 PHPUnit tests in `Test/Unit/`, and they are **not runnable from a fresh checkout** — the shipped `composer.json` has an empty `require` block and no `require-dev` on purpose. The test dependencies live in a CI-only manifest: +There are 400+ PHPUnit tests in `Test/Unit/`, and they are **not runnable from a fresh checkout** — the shipped `composer.json` has an empty `require` block and no `require-dev` on purpose. The test dependencies live in a CI-only manifest: ```bash cd Test/Unit && composer install # 184 packages, pinned by the committed lock @@ -118,7 +118,8 @@ There are two integration types, selectable in admin config: **Webhook** (independent, server-to-server): - `/paystack/payment/webhook` — receives `charge.success` events from Paystack -- Validates HMAC-SHA512 signature, verifies transaction, calls `Model/PaymentSettlement::register()`, dispatches `paystack_payment_verify_after` +- Validates HMAC-SHA512 signature, verifies transaction, finds the order via `Model/WebhookOrderResolver` (increment id → reference already bound → `metadata.orderId`+`quoteId` → quote: lone order, else the single payable Paystack order), calls `Model/PaymentSettlement::register()`, dispatches `paystack_payment_verify_after` +- A decided rejection of a real charge is acknowledged (200) only once its order-history comment is saved (`historyRecorded`); closed orders get `order_closed` (200 once recorded), held/payment-review orders `order_not_payable` (retried) - CSRF validation skipped via `Plugin/CsrfValidatorSkip.php` `Model/PaymentSettlement::register()` is the single class that binds the Paystack reference to an order, registers the captured payment (`total_paid`, invoice, transaction row via `registerCaptureNotification()`), and records rejection history — for all three verification paths above, before any of them dispatches `paystack_payment_verify_after`. Each consumer dispatches the settled order instance `register()` returns, not its own stale one. `ObserverAfterPaymentVerify.php` no longer advances order state at all: it is email-only now, gated on `!$order->getEmailSent()` (register() already saved the order by the time the observer runs). Initial order confirmation email is suppressed by `ObserverBeforeSalesOrderPlace` until payment is verified. Both observers must be registered in **every** DI area a checkout path can place/verify an order from — `Model/PaymentManagement.php` (the inline flow's default integration type) runs in `webapi_rest`, not `frontend`, so both `etc/frontend/events.xml` and `etc/webapi_rest/events.xml` register both events; registering only one observer in `webapi_rest` without the other caused either a missing post-payment confirmation (inline orders stuck unadvanced) or a duplicate confirmation email (placement email unsuppressed, post-payment email also sent). @@ -129,6 +130,7 @@ There are two integration types, selectable in admin config: |---|---| | `Gateway/PaystackApiClient.php` | All Paystack API calls: initialize transaction, verify, validate webhook signature | | `Model/PaymentManagement.php` | REST API endpoint for inline payment verification | +| `Model/WebhookOrderResolver.php` | Finds the order a verified webhook charge is for, including inline retries where several orders share a quote (#69) | | `Model/PaymentSettlement.php` | Binds the Paystack reference to an order, registers the captured payment, and records rejection/overpayment history — shared by all three verification paths | | `Model/Ui/ConfigProvider.php` | Injects public key, integration type, and URLs into checkout JS config | | `Controller/Payment/AbstractPaystackStandard.php` | Base controller with shared utilities (quote loading, message handling) | @@ -149,4 +151,4 @@ All settings live under `payment/pstk_paystack/` in Magento config. Secret keys ### Quote ID as Transaction Anchor -For inline payments, Paystack generates the transaction reference on the client side. The `quoteId` is passed as metadata in the Paystack transaction so the webhook/verification can locate the correct order when no Magento-generated reference is available. +For inline payments, Paystack generates the transaction reference on the client side. The `quoteId` — and, since #69, the placed order's entity id as `orderId` (captured in the renderer's `getPlaceOrderDeferredObject()` override) — are passed as metadata in the Paystack transaction so the webhook/verification can locate the correct order when no Magento-generated reference is available. Closing the popup runs `Recreate`, which cancels the order and reuses the same quote, so one quote can carry several orders. Both ids are browser-supplied; `register()`'s checks are the actual bound. A server-side initialize redesign is planned; the client-side `orderId` becomes obsolete then. diff --git a/marketplace/src/reference-manual.md b/marketplace/src/reference-manual.md index f02a5f7..40838a0 100644 --- a/marketplace/src/reference-manual.md +++ b/marketplace/src/reference-manual.md @@ -116,9 +116,20 @@ The HTTP status the endpoint returns tells Paystack whether redelivering the eve Any condition the module does not specifically recognise is treated as undecided and retried, because wrongly reporting a decision consumes Paystack's retry window and can leave a real payment permanently unconfirmed. -When a settlement is refused, or accepted with an overpayment, the module records a comment on the order's status history giving the reason, the amount paid, the amount expected, and the transaction reference. The comment is written once per transaction and reason, so a retried event does not repeat it. +When a settlement is refused, or accepted with an overpayment, the module records a comment on the order's status history giving the reason, the amount paid, the amount expected, and the transaction reference. The comment is written once per transaction and reason, so a retried event does not repeat it. A decided refusal of a real (live-mode, successful) charge is only acknowledged with `200` once that comment is saved; until then the endpoint returns `503` so the record is never lost. -The webhook's fallback lookup of an order by `quote_id` (used when the popup flow's Paystack-generated reference has no matching order) resolves an ambiguous match by requiring exactly one candidate order, not by disambiguating on amount. This is a known, separate limitation, not addressed by the retry-semantics change above. +A charge for an order that can no longer take it — canceled, closed or complete, or with nothing left due — is refused with reason `order_closed` and acknowledged with `200` once recorded. The comment asks you to refund or reconcile the charge if it is not already reflected on the order, and the module logs it at `critical`. Orders on hold or under payment review are refused with `order_not_payable` and keep being retried, because they may still become payable. + +### Order lookup + +Redirect payments use the order's increment id as the Paystack reference. Inline payments carry a Paystack-generated reference, so `Model\WebhookOrderResolver` locates the order in this order, first match wins: + +1. An order whose increment id equals the reference. +2. A Paystack order the reference is already bound to (a redelivered event). +3. The `orderId` and `quoteId` in the transaction's metadata, naming one Paystack order on that quote — in any state, so a late charge for a cancelled attempt is recorded against that attempt. +4. The `quoteId` alone: the quote's only order, or else its single still-payable Paystack order. When several orders qualify, none is chosen and the event is treated as order-not-found (logged at `error` with the candidate orders). + +Both metadata ids are supplied by the customer's browser; what bounds their effect is the amount, currency, mode and one-reference-one-order checks every settlement goes through. ## Content Security Policy @@ -138,7 +149,7 @@ If your store applies a custom CSP outside Magento's mechanism — at a CDN or r **Inline.** The order is placed, then checkout JavaScript opens the Paystack window. On success it calls `GET /V1/paystack/verify/{reference}_{quoteId}`. The module verifies with Paystack and dispatches `paystack_payment_verify_after`. -Paystack generates the transaction reference on the client for inline payments, so the quote id is passed as transaction metadata. That gives the webhook a reliable way to locate the order when no Magento-generated reference exists. +Paystack generates the transaction reference on the client for inline payments, so the quote id and the placed order's id are passed as transaction metadata. That gives the webhook a reliable way to locate the order when no Magento-generated reference exists — including after the customer closed the Paystack window and retried, which cancels the first order and places a new one on the same quote (see Order lookup above). **Redirect.** `/paystack/payment/setup` initialises the transaction and redirects to Paystack. The customer returns to `/paystack/payment/callback`, which verifies the transaction and dispatches `paystack_payment_verify_after`. Failed or abandoned payments route through `/paystack/payment/recreate`. diff --git a/marketplace/src/user-guide.md b/marketplace/src/user-guide.md index a72b0a3..337f465 100644 --- a/marketplace/src/user-guide.md +++ b/marketplace/src/user-guide.md @@ -94,6 +94,8 @@ To refund a customer, issue the refund directly from your Paystack dashboard (or **Customers report being charged without an order.** Verify the payment reference in your Paystack dashboard, then check the webhook delivery log there. Paystack retries failed webhook deliveries. +**An order's history says "payment received after this order was closed".** Paystack took a payment for an order that was already cancelled, closed, completed or fully paid — for example a bank transfer that settled after the customer closed the payment window and checked out again. Search your orders for the reference in the comment. If the payment is not already reflected on an order, refund it from the Paystack dashboard or contact the customer. + ## Support For issues with this extension, use the [issue tracker](https://github.com/PaystackHQ/plugin-magento-2/issues).