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/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/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/Controller/Payment/Webhook.php b/Controller/Payment/Webhook.php index 68c3db2..c6ee2a0 100644 --- a/Controller/Payment/Webhook.php +++ b/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)) { diff --git a/Gateway/Validator/TransactionValidator.php b/Gateway/Validator/TransactionValidator.php index 9c5c16a..7ab622c 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; @@ -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. diff --git a/Model/PaymentSettlement.php b/Model/PaymentSettlement.php index 8c9fdc1..4386b64 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; @@ -86,9 +89,15 @@ 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 + * 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 @@ -109,7 +118,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 — @@ -126,7 +135,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 @@ -145,7 +154,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); @@ -157,10 +166,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 @@ -172,13 +182,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, + ]; } } @@ -193,7 +207,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]; } } @@ -201,16 +215,14 @@ 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 - ) { - $this->recordRejection( - $freshOrder, - $reference, - TransactionValidator::REASON_ORDER_NOT_PAYABLE, - $transactionDetails - ); - return ['reason' => TransactionValidator::REASON_ORDER_NOT_PAYABLE, 'order' => $freshOrder]; + if (!$this->transactionValidator->isPayable($freshOrder)) { + // 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; + $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. @@ -263,16 +275,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]; } /** @@ -299,9 +315,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() @@ -312,34 +329,40 @@ 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', [ - 'reason' => $reason, - 'reference' => $reference, - 'order_increment_id' => $order->getIncrementId(), - ]); + $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) { + $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 = ''; + } - $this->writeHistory( + return $this->writeHistory( $order, $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 ); } @@ -355,54 +378,88 @@ 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; } } /** - * 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 * @param string $reasonKey * @param string $comment - * @return void + * @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): void - { - if (!$this->appendHistoryComment($order, $reference, $reasonKey, $comment)) { - return; + private function writeHistory( + Order $order, + string $reference, + string $reasonKey, + string $comment, + bool $isClosed + ): bool { + $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_PRESENT) { + $this->logger->{$isClosed ? 'info' : 'warning'}($message, $context); + return true; + } + if ($appended !== self::HISTORY_APPENDED) { + $this->logger->critical($message, $context); + return false; } try { $this->orderRepository->save($order); } 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/Model/WebhookOrderResolver.php b/Model/WebhookOrderResolver.php new file mode 100644 index 0000000..10268f0 --- /dev/null +++ b/Model/WebhookOrderResolver.php @@ -0,0 +1,259 @@ + 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()); + // 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; + } + } + + $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) { + $matches = $this->findOrders(['entity_id' => $orderId, 'quote_id' => $quoteId]); + 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), + ]); + } else { + $this->logger->info('Paystack Webhook: no metadata.orderId, using quote lookup', [ + 'reference' => $reference, + ]); + } + + // Step 4: quote fallback. + $candidates = $this->findOrders(['quote_id' => $quoteId]); + + 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), + ]); + + 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; + } + + /** + * @param mixed $orderId + * @return OrderInterface|null + */ + private function findOrder($orderId): ?OrderInterface + { + 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 array_values($this->orderRepository->getList($this->searchCriteriaBuilder->create())->getItems()); + } + + /** + * 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..dfdc859 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) ); } @@ -625,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/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..8285d80 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); @@ -1378,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' => [ @@ -1400,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'); @@ -1413,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(); } @@ -1424,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'], ]; } @@ -1547,14 +1663,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 +1702,99 @@ 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(); + } + + /** + * 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/Gateway/Validator/TransactionValidatorTest.php b/Test/Unit/Gateway/Validator/TransactionValidatorTest.php index 9889d00..b0b848f 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, ]; @@ -1171,4 +1187,79 @@ 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], + ]; + } + + /** + * @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], + ]; + } + + /** + * @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/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 edcdfd4..7a05712 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,13 +328,53 @@ public function testDifferentReferenceOnAlreadyRegisteredOrderIsNotTreatedAsIdem ); $this->assertSame(TransactionValidator::REASON_ORDER_NOT_PAYABLE, $result['reason']); + $this->assertTrue($result['historyRecorded']); } - 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->assertTrue($result['historyRecorded']); + $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'); @@ -344,9 +389,187 @@ public function testOrderNotInPayableStateIsRejected(): void ); $this->assertSame(TransactionValidator::REASON_ORDER_NOT_PAYABLE, $result['reason']); + $this->assertTrue($result['historyRecorded']); + } + + 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 reason stays + * the deterministic ORDER_CLOSED; `historyRecorded` false tells the webhook + * to keep retrying until it can be recorded. + */ + public function testTerminalOrderWhoseHistoryCannotBeSavedIsClosedButNotRecorded(): 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_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'], + ]; } - public function testZeroBaseTotalDueIsRejectedAsNotPayable(): void + /** + * 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 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]') + ) + ); + $this->logger->expects($this->once())->method('critical'); + + $this->paymentSettlement->register( + (object) ['data' => $this->makeVerifyData()], + $order, + true + ); + } + + /** + * 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']); + $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(); @@ -384,7 +607,8 @@ public function testZeroBaseTotalDueIsRejectedAsNotPayable(): void true ); - $this->assertSame(TransactionValidator::REASON_ORDER_NOT_PAYABLE, $result['reason']); + $this->assertSame(TransactionValidator::REASON_ORDER_CLOSED, $result['reason']); + $this->assertTrue($result['historyRecorded']); } /** @@ -414,6 +638,7 @@ public function testNonConcreteOrderInterfaceInputReturnsMalformedNotFatal(): vo ); $this->assertSame(TransactionValidator::REASON_MALFORMED, $result['reason']); + $this->assertFalse($result['historyRecorded']); } /** @@ -451,6 +676,7 @@ public function testFreshRefetchNoSuchEntityDoesNotFallBackAndReturnsMalformed() ); $this->assertSame(TransactionValidator::REASON_MALFORMED, $result['reason']); + $this->assertFalse($result['historyRecorded']); } /** @@ -487,6 +713,7 @@ public function testFreshRefetchTransientThrowFallsBackToCallersConcreteOrder(): ); $this->assertNull($result['reason']); + $this->assertTrue($result['historyRecorded']); $this->assertSame($order, $result['order']); } @@ -514,6 +741,7 @@ public function testOverpaymentIsAcceptedAndRecordedInOneSave(): void ); $this->assertNull($result['reason']); + $this->assertTrue($result['historyRecorded']); } /** @@ -546,9 +774,47 @@ public function testRegistrationThrowingIsCaughtRecordedAndReturnsRegistrationFa ); $this->assertSame(TransactionValidator::REASON_REGISTRATION_FAILED, $result['reason']); + $this->assertTrue($result['historyRecorded']); $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. @@ -579,5 +845,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 new file mode 100644 index 0000000..468e440 --- /dev/null +++ b/Test/Unit/Model/WebhookOrderResolverTest.php @@ -0,0 +1,539 @@ + their field=>value filters */ + private $criteriaFilters; + + /** @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); + $this->transactionRepository = $this->createMock(TransactionRepositoryInterface::class); + $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); + $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->logger + ); + } + + 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->buildResolver($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'])) + ); + } + + 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->buildResolver()->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '9', 'orderId' => '7'])) + ); + } + + /** + * 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)); + return new WebhookOrderResolver( + $this->orderRepository, + $builder, + $this->transactionRepository, + $orderInterface ?? $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 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->buildResolver()->resolve('PSK_1', $this->withMetadata((object) [])) + ); + } + + 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->buildResolver()->resolve('PSK_1', $this->withMetadata((object) ['quoteId' => '9'])) + ); + } + + 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->buildResolver()->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 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). 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