fix(webhook): settle the right order when several share a quote (#69)

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,

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.

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);
     }

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],

Controller/Payment/Webhook.php

@@ -51,14 +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 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 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
@@ -123,19 +118,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);
@@ -263,7 +248,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)) {

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;
 
@@ -57,20 +58,32 @@ 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, ...). 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
+     * 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(). 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. Never RETRYABLE_FOR_CUSTOMER
+     * — money moved, fail closed.
+     */
+    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 —
@@ -128,6 +141,7 @@ class TransactionValidator
         self::REASON_CURRENCY_MISMATCH,
         self::REASON_ZERO_TOTAL,
         self::REASON_REFERENCE_BOUND_ELSEWHERE,
+        self::REASON_ORDER_CLOSED,
     ];
 
     /** @var LoggerInterface */
@@ -168,6 +182,41 @@ 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;
+    }
+
+    /**
+     * 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 —
@@ -220,6 +269,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.