diff --git a/CLAUDE.md b/CLAUDE.md index 0287dd3..c673028 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -58,13 +58,23 @@ All HTTP and (de)serialization is delegated to [`setono/quickpay-php-sdk`](https typed `payments()` endpoints, request/response DTOs, non-exhaustive `PaymentState`/`OperationType` enums, a `QuickpayException` hierarchy, and a timing-safe `CallbackValidator`. Basic auth, the mandatory `Accept-Version: v10` header and host pinning to `api.quickpay.net` all live in the SDK. The SDK went -stable on 2026-08-10, so the constraint is a plain `^1.0` and **consumers need no `minimum-stability` -setting at all** — that requirement, and the docs describing it, are gone. +stable on 2026-08-10, so the constraint is a plain caret and **consumers need no `minimum-stability` +setting at all** — that requirement, and the docs describing it, are gone. The constraint is **`^1.1`** +(2026-08-17): 1.1 is additive but the gateway uses what it added — `Operation::isApproved()` (not +pending AND `20000`; every `Operations` helper builds on it) and `isOfType()`, `Shopsystem` on the +create request (`ConvertPaymentAction` identifies the integration as `setono/payum-quickpay` + +installed version via Composer's runtime API), and `PaymentsEndpoint::findByOrderId()` for the +find-or-create in `Convert`. Not used on purpose: the SDK's `Payment::latestOperation()` / +`hasPendingOperation()` are type-agnostic and its amount helpers sum — the actions need the per-type, +last-approved views `Operations` provides. `cache:` on the SDK `Client` is reachable through the +`quickpay.client` option (build the client with it) rather than a gateway option of its own, so the +package does not have to depend on Valinor's cache interface. Since 1.1 an empty body (cancel) is sent +as `{}`, not as nothing. The pre-release history is worth remembering only because it explains why the constraint used to be so specific: the gateway cannot run on alpha.1 (no client-wide `synchronized`) and breaks on alpha.1–2 (request fields only became required constructor params in alpha.3), so the constraint had to keep -excluding them. `^1.0` excludes every pre-release outright, so that is no longer a concern. +excluding them. A caret on a stable release excludes every pre-release outright, so that is no longer a concern. ### Wiring @@ -109,8 +119,15 @@ the `quickpayPaymentId` (int) it carries is the single source of truth — actio `Api::payments()->getById()` rather than passing the model through as API params. Request → Action flow (amounts are integer minor units everywhere — no conversion): -- **Convert** → `ConvertPaymentAction` — turns a Payum `PaymentInterface` into the details array; creates - the Quickpay payment (via `CreatePaymentRequest`) if absent and stores **only scalars** — +- **Convert** → `ConvertPaymentAction` — turns a Payum `PaymentInterface` into the details array; + **finds or creates** the Quickpay payment if the details carry no id: `findByOrderId()` first — Quickpay + enforces `order_id` uniqueness per account (a second create is a 400 "already exists on another + payment", verified live 2026-08-17) and under Sylius the Payum payment number is the *order* number, so + a retry after a decline collides — and an existing payment is adopted only if nothing was ever + approved on it (`findReusablePayment()`: initial/declined/authorize-in-flight; the currency must + match). One with an approved operation is a `LogicException` naming it — never adopt money silently: + it is either this order's earlier paid payment or another environment's under a shared prefix. Otherwise + it creates the payment (via `CreatePaymentRequest`, with `Shopsystem` = this package + version) and stores **only scalars** — `quickpayPaymentId`, `amount`, `currency`, `order_id` — plus `continue_url` (the token's **target** url, so the customer's return re-executes the `Capture`/`Authorize` that sent them out — Payum's return-trip convention) and `cancel_url` (the token's after url). It never persists DTO/model objects. On the create path only it asserts its inputs: diff --git a/README.md b/README.md index aab1514..d79ed11 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,8 @@ The gateway accepts either shape, so a list is fine where that reads better: Quickpay is an authorize/capture PSP behind a hosted payment window, and the gateway maps that onto Payum's requests the way Payum's own controllers — and Sylius — expect: -1. **`Convert`** turns your Payum payment into the details array and creates the payment at Quickpay. +1. **`Convert`** turns your Payum payment into the details array and creates the payment at Quickpay — + or picks up the one that already exists under the same order id, if nobody has paid it (see below). 2. **`Capture`** or **`Authorize`** against the fresh payment creates the payment link and **redirects the customer to the Quickpay payment window** (Payum's `HttpRedirect` reply). Which one you execute is how you say what the window should do once the card is authorized: @@ -152,6 +153,17 @@ The `auto_capture` gateway option is **deprecated** in favour of this: it made a capture on authorization too, which is exactly what executing `Capture` means. It still works for existing configurations and goes in 3.0. +**Retrying a checkout.** Quickpay's `order_id` — `order_prefix` + the Payum payment number — must be +unique per account, and under Sylius the payment number is the *order* number, so a customer who was +declined, came back to the shop and pays again arrives with the same order id. `Convert` therefore +looks the order id up first: a payment that already exists under it and **was never successfully paid** +(the window was never completed, the attempt was declined) is picked up where it was left, and the +customer is sent back to the window on it. A payment that *has* an approved operation — authorized, +captured, refunded, cancelled — is never adopted silently: `Convert` throws a `LogicException` naming it, +because it is either this order's earlier payment that really was paid (carry its `quickpayPaymentId` +over) or another shop or environment sharing the account under a prefix that should not be shared +(give each its own `order_prefix`). Either way that is your call, not a claim of money. + ### Callbacks Quickpay confirms operations with a signed server-to-server callback. `NotifyAction` verifies the diff --git a/composer.json b/composer.json index d0553b1..186793b 100644 --- a/composer.json +++ b/composer.json @@ -15,9 +15,10 @@ "ext-filter": "*", "ext-hash": "*", "ext-json": "*", + "composer-runtime-api": "^2.0", "payum/core": "^1.7.5", "php-http/message-factory": "^1.0", - "setono/quickpay-php-sdk": "^1.0" + "setono/quickpay-php-sdk": "^1.1" }, "require-dev": { "ergebnis/composer-normalize": "^2.52", diff --git a/docs/UPGRADE-2.0.md b/docs/UPGRADE-2.0.md index c533f33..05a0482 100644 --- a/docs/UPGRADE-2.0.md +++ b/docs/UPGRADE-2.0.md @@ -13,7 +13,8 @@ handling, and modernizes the test suite. This is a major release with breaking c - The SDK is built on PSR-18 / PSR-17 and discovers them via `php-http/discovery`. Make sure your project provides a PSR-18 client and PSR-17 factories (e.g. `composer require kriswallsmith/buzz nyholm/psr7`, or any other implementation). -- `setono/quickpay-php-sdk` is stable as of `1.0.0`, so **no `minimum-stability` change is needed**. If +- `setono/quickpay-php-sdk` **`^1.1`** (stable since `1.0.0`, and 2.0 uses what 1.1 added), so **no + `minimum-stability` change is needed**. If you tracked the 2.0 alphas and added `"minimum-stability": "beta"` for the SDK's pre-releases, you can drop it again (assuming nothing else in your project needs it). @@ -257,6 +258,31 @@ pre-operation balance. `GetStatus` used to answer `pending` for that; it now ans already been approved (`authorized` with a capture queued, `captured` with a refund queued) and says `pending` only when nothing has been approved yet. +## `Convert` finds or creates — a retried checkout no longer fails on a duplicate order id + +> Changed after `2.0.0-beta.1`. + +Quickpay enforces `order_id` uniqueness per account: creating a second payment under an existing order +id is a `400` "order_id already exists on another payment" (verified live). The gateway builds the order +id from the Payum payment **number**, which under Sylius is the **order** number — so a customer who was +declined, came back to the shop and pays again arrived at `Convert` with an order id Quickpay already +had, and the retry died with a `ValidationException`. (The same on plain Payum whenever a number is +reused for a new payment.) + +`ConvertPaymentAction` now looks the order id up first (`PaymentsEndpoint::findByOrderId()`, SDK ≥ 1.1): + +| Under that order id Quickpay has… | `Convert` | +|---|---| +| nothing | creates the payment, as before | +| a payment nobody has paid — no approved operation: created but never completed, declined, an authorize still in flight — in the same currency | **adopts it**: `quickpayPaymentId`, `order_id`, `currency` come from it, and the checkout continues on it (a new link is created; the entry-point actions treat a declined attempt like a fresh payment) | +| a payment with an approved operation (authorized / captured / refunded / cancelled) | throws a `LogicException` naming it — never adopts money silently: it is either this order's earlier payment that really was paid (carry its `quickpayPaymentId` over yourself) or another shop or environment sharing the account under a prefix that should not be shared (give each its own `order_prefix`) | +| a payment in another currency | throws a `LogicException` — the payment is what Quickpay charges in | + +The lookup is one `GET /payments?order_id=…` per *creating* Convert; a model that already carries a +`quickpayPaymentId` makes no request, as before. The create request now also carries Quickpay's +`shopsystem` (`setono/payum-quickpay` + the installed version), so the manager shows what created a +payment; a Convert action of your own may say something more specific. + ## Order ids are now validated before they are sent `ConvertPaymentAction` builds `order_id` as `order_prefix` + the Payum payment number, and now enforces @@ -286,8 +312,9 @@ These were internal implementation details; they are gone in 2.0: ## Injecting a custom SDK client You can pass a preconfigured SDK client through the new `quickpay.client` option (a -`Setono\Quickpay\Client\ClientInterface`) — useful for wiring a cached Valinor builder or a specific -PSR-18 client. When omitted, the gateway builds one from `api_key` via discovery. +`Setono\Quickpay\Client\ClientInterface`) — useful for a specific PSR-18 client, or for the SDK's mapper +cache in production (`new Client($apiKey, synchronized: …, cache: new FileSystemCache($dir))`, one +argument since SDK 1.1). When omitted, the gateway builds one from `api_key` via discovery, uncached. The `synchronized` option is carried by the SDK client itself (`Client::__construct(..., synchronized: true)`), and it can only be set there. An injected client must therefore be constructed with the same diff --git a/src/Action/ConvertPaymentAction.php b/src/Action/ConvertPaymentAction.php index 11290c5..ff2cda1 100644 --- a/src/Action/ConvertPaymentAction.php +++ b/src/Action/ConvertPaymentAction.php @@ -4,6 +4,7 @@ namespace Setono\Payum\Quickpay\Action; +use Composer\InstalledVersions; use Payum\Core\Action\ActionInterface; use Payum\Core\ApiAwareInterface; use Payum\Core\Bridge\Spl\ArrayObject; @@ -14,13 +15,18 @@ use Payum\Core\Model\PaymentInterface; use Payum\Core\Request\Convert; use Setono\Payum\Quickpay\Action\Api\ApiAwareTrait; +use Setono\Payum\Quickpay\Operations; use Setono\Quickpay\Request\Payment\CreatePaymentRequest; +use Setono\Quickpay\Request\Payment\Shopsystem; +use Setono\Quickpay\Response\Payment\Payment; class ConvertPaymentAction implements ActionInterface, ApiAwareInterface, GatewayAwareInterface { use GatewayAwareTrait; use ApiAwareTrait; + private const PACKAGE = 'setono/payum-quickpay'; + /** * Quickpay's accepted `order_id` length, verified against the live API. */ @@ -50,10 +56,16 @@ public function execute($request): void $currency = self::assertNotEmptyString($paymentModel->getCurrencyCode(), 'currency code', $number); $orderId = self::assertOrderId($this->api->getOrderPrefix() . $number); - $payment = $this->api->payments()->create(new CreatePaymentRequest( - orderId: $orderId, - currency: $currency, - )); + $payment = $this->findReusablePayment($orderId, $currency) + ?? $this->api->payments()->create(new CreatePaymentRequest( + orderId: $orderId, + currency: $currency, + // Quickpay's "shopsystem" is what the payment was created with — it shows in the + // manager and tells Quickpay support which integration they are looking at. A + // shop's own Convert action (the Sylius plugin has one) can say something more + // specific. + shopsystem: new Shopsystem(name: self::PACKAGE, version: self::version()), + )); $details['quickpayPaymentId'] = $payment->id; $details['order_id'] = $payment->orderId; @@ -143,6 +155,75 @@ private static function assertCurrencyUnchanged(ArrayObject $details, mixed $cur $details['currency'] = $currency; } + /** + * Find-or-create, the safe half: the Quickpay payment that already carries this order id, if it + * can be picked up where it was left — or null, meaning "create one". + * + * Quickpay enforces `order_id` uniqueness per account (a second create is a 400 "order_id already + * exists on another payment" — verified live), and the order id is built from the Payum payment + * NUMBER, which under Sylius is the ORDER number: every retry payment for an order — the customer + * was declined, came back to the shop and pays again — carries the same number. Without this, + * that retry died at Convert with a validation error, for the most ordinary of reasons. + * + * A payment is picked up only if nothing was ever approved on it — the window was never completed, + * or the attempt was declined. That is exactly the retry case, and it can never make anything look + * paid that is not: the entry-point actions treat such a payment like a fresh one and send the + * customer back to the window. A payment that HAS an approved operation — authorized, captured, + * refunded, cancelled — is never adopted silently: it may be this order's earlier payment that + * really was paid, or another environment's payment under a prefix that should not be shared, and + * either way it is for the shop to decide, so it is a clear exception rather than a claim of money. + * The currency has to match too — the payment is what Quickpay charges in. + * + * @throws LogicException if a payment with this order id exists but cannot be adopted + */ + private function findReusablePayment(string $orderId, string $currency): ?Payment + { + $existing = $this->api->payments()->findByOrderId($orderId); + + if (null === $existing) { + return null; + } + + $approved = Operations::latestApproved($existing->operations); + if (null !== $approved) { + throw new LogicException(sprintf( + 'A Quickpay payment with order id "%s" already exists (id %d, state %s) and has an approved %s. ' + . 'The gateway will not adopt it: if it is this order\'s earlier payment, carry its ' + . 'quickpayPaymentId over; if another shop or environment shares this Quickpay account, ' + . 'give each its own "order_prefix".', + $orderId, + $existing->id, + $existing->state, + $approved->type, + )); + } + + if ($existing->currency !== $currency) { + throw new LogicException(sprintf( + 'A Quickpay payment with order id "%s" already exists (id %d) in %s, but this payment is in %s. ' + . 'A Quickpay payment cannot change currency; give the retry a different order id.', + $orderId, + $existing->id, + $existing->currency, + $currency, + )); + } + + return $existing; + } + + /** + * The installed version of this package, for the `shopsystem` Quickpay records on the payment. + * Composer's runtime API knows it wherever the package was installed by Composer; "unknown" covers + * a vendored copy. + */ + private static function version(): string + { + return InstalledVersions::isInstalled(self::PACKAGE) + ? (InstalledVersions::getPrettyVersion(self::PACKAGE) ?? 'unknown') + : 'unknown'; + } + /** * Quickpay requires `order_id` to be 4–20 characters, and the gateway builds it by concatenating * the `order_prefix` option with the Payum payment number. Checking the result here turns two diff --git a/src/Exception/OperationRejectedException.php b/src/Exception/OperationRejectedException.php index 11fa423..d7988da 100644 --- a/src/Exception/OperationRejectedException.php +++ b/src/Exception/OperationRejectedException.php @@ -50,7 +50,8 @@ public static function assertNotRejected(int $paymentId, Payment $payment, Opera { $operation = Operations::latestOfType($payment->operations, $type); - if (null === $operation || $operation->pending || Operations::isApproved($operation)) { + // isApproved() is false for a pending operation too; the explicit check says why it passes. + if (null === $operation || $operation->pending || $operation->isApproved()) { return; } diff --git a/src/Operations.php b/src/Operations.php index b82aa5d..ea544f7 100644 --- a/src/Operations.php +++ b/src/Operations.php @@ -8,19 +8,18 @@ use Setono\Quickpay\Response\Payment\Operation; /** - * Stateless helpers over a payment's list of {@see Operation}s. + * Stateless helpers over a payment's list of {@see Operation}s — the questions the actions ask that + * the SDK's own helpers do not answer as such. * - * This replaces the behavior that used to live on the deleted `QuickpayPayment` / - * `QuickpayPaymentOperation` models — the SDK response DTOs are readonly data holders without - * behavior of their own. + * "Is this operation approved / of that type" is the SDK's ({@see Operation::isApproved()} — not + * pending AND status `20000` — and {@see Operation::isOfType()}), and everything here builds on those. + * What the SDK's {@see \Setono\Quickpay\Response\Payment\Payment} helpers offer is either the wrong + * shape for the actions (`latestOperation()` regardless of type, `hasPendingOperation()` regardless of + * type, summed amounts) or answers a question they do not ask; the per-type, last-approved views live + * here. */ final class Operations { - /** - * Quickpay's status code for an approved operation. - */ - public const APPROVED_STATUS_CODE = '20000'; - private function __construct() { } @@ -38,7 +37,7 @@ private function __construct() public static function latestApproved(array $operations): ?Operation { foreach (array_reverse($operations) as $operation) { - if (self::isApproved($operation)) { + if ($operation->isApproved()) { return $operation; } } @@ -57,7 +56,7 @@ public static function latestApproved(array $operations): ?Operation public static function latestOfType(array $operations, OperationType $type): ?Operation { foreach (array_reverse($operations) as $operation) { - if ($type === $operation->type()) { + if ($operation->isOfType($type)) { return $operation; } } @@ -65,11 +64,6 @@ public static function latestOfType(array $operations, OperationType $type): ?Op return null; } - public static function isApproved(Operation $operation): bool - { - return self::APPROVED_STATUS_CODE === $operation->qpStatusCode; - } - /** * Whether any operation of the given type has been approved — regardless of what came after it. * "Has this payment ever been authorized?" is this question; the last-approved-operation view @@ -80,7 +74,7 @@ public static function isApproved(Operation $operation): bool public static function hasApproved(array $operations, OperationType $type): bool { foreach ($operations as $operation) { - if ($type === $operation->type() && self::isApproved($operation)) { + if ($operation->isOfType($type) && $operation->isApproved()) { return true; } } @@ -98,7 +92,7 @@ public static function hasApproved(array $operations, OperationType $type): bool public static function hasPending(array $operations, OperationType $type): bool { foreach ($operations as $operation) { - if ($type === $operation->type() && $operation->pending) { + if ($operation->isOfType($type) && $operation->pending) { return true; } } @@ -108,6 +102,6 @@ public static function hasPending(array $operations, OperationType $type): bool public static function isApprovedOfType(?Operation $operation, OperationType $type): bool { - return null !== $operation && $type === $operation->type() && self::isApproved($operation); + return null !== $operation && $operation->isOfType($type) && $operation->isApproved(); } } diff --git a/tests/Action/CancelActionTest.php b/tests/Action/CancelActionTest.php index cae6f10..fb5365c 100644 --- a/tests/Action/CancelActionTest.php +++ b/tests/Action/CancelActionTest.php @@ -51,8 +51,9 @@ public function shouldCancelPayment(): void self::assertCount(2, $requests); $this->assertRequest($requests[0], 'GET', '#/payments/1001$#'); $this->assertRequest($requests[1], 'POST', '#/payments/1001/cancel$#'); - // Cancel takes no body. - self::assertSame('', (string) $requests[1]->getBody()); + // Cancel takes no body: the SDK (>= 1.1) sends an empty JSON object, which the API accepts + // where a literal empty array would be rejected. + self::assertSame('{}', (string) $requests[1]->getBody()); self::assertSame(0, $details['balance']); } diff --git a/tests/Action/ConvertPaymentActionTest.php b/tests/Action/ConvertPaymentActionTest.php index ae86927..a7f9fb9 100644 --- a/tests/Action/ConvertPaymentActionTest.php +++ b/tests/Action/ConvertPaymentActionTest.php @@ -16,6 +16,7 @@ use PHPUnit\Framework\TestCase; use Setono\Payum\Quickpay\Action\ConvertPaymentAction; use Setono\Payum\Quickpay\Tests\ApiTestTrait; +use Setono\Quickpay\Enum\PaymentState; use stdClass; /** @@ -59,6 +60,8 @@ public function shouldCreatePaymentAndStoreOnlyScalarDetails(): void $convert = new Convert($payment, 'array', $token); + // The lookup (nothing under that order id yet), then the create. + $this->queueResponse('[]'); $this->queuePayment(['id' => 1001, 'order_id' => 'ut000000000001']); $action = new ConvertPaymentAction(); @@ -86,15 +89,22 @@ public function shouldCreatePaymentAndStoreOnlyScalarDetails(): void self::assertIsNotObject($value, 'Details must not contain objects'); } - // The create request carries only order_id + currency. + // Find-or-create: an exact order_id lookup first, then the create — which carries only + // order_id + currency (+ the shopsystem). $requests = $this->getRequests(); - self::assertCount(1, $requests); - $this->assertRequest($requests[0], 'POST', '#/payments$#'); - $body = $this->decodeBody($requests[0]); + self::assertCount(2, $requests); + $this->assertRequest($requests[0], 'GET', '#/payments$#'); + self::assertSame('order_id=ut000000000001&page=1&page_size=1', urldecode($requests[0]->getUri()->getQuery())); + $this->assertRequest($requests[1], 'POST', '#/payments$#'); + $body = $this->decodeBody($requests[1]); self::assertSame('ut000000000001', $body['order_id']); self::assertSame('DKK', $body['currency']); self::assertArrayNotHasKey('card', $body); self::assertArrayNotHasKey('payment', $body); + // The integration identifies itself: Quickpay records `shopsystem` on the payment. + self::assertSame('setono/payum-quickpay', $body['shopsystem']['name']); + self::assertIsString($body['shopsystem']['version']); + self::assertNotSame('', $body['shopsystem']['version']); } #[Test] @@ -199,6 +209,7 @@ public function shouldAcceptOrderIdsAtBothEndsOfTheRange(): void $action->setGateway($this->gateway); $action->setApi($this->api); + $this->queueResponse('[]'); $this->queuePayment(['id' => 1001, 'order_id' => $expectedOrderId]); $action->execute($convert); @@ -208,6 +219,123 @@ public function shouldAcceptOrderIdsAtBothEndsOfTheRange(): void } } + // -- Find-or-create ---------------------------------------------------------------------- + + /** + * Quickpay enforces order_id uniqueness per account (a second create is a 400 "order_id already + * exists on another payment" — verified live), and under Sylius the Payum payment number is the + * ORDER number, so a customer who was declined and pays again carries the same order id. A payment + * that already exists under it and was never successfully paid — the window never completed, or the + * attempt was declined — is picked up where it was left instead of the create failing. + * + * @param array $existing + */ + #[Test] + #[DataProvider('reusablePaymentProvider')] + public function shouldPickUpAnExistingPaymentNobodyHasPaid(array $existing): void + { + $payment = $this->createPayment(); + $convert = new Convert($payment, 'array'); + + $this->queueResponse('[' . $this->paymentJson(array_replace(['id' => 4242, 'order_id' => 'ut000000000001'], $existing)) . ']'); + + $action = new ConvertPaymentAction(); + $action->setGateway($this->gateway); + $action->setApi($this->api); + $action->execute($convert); + + /** @var array $result */ + $result = $convert->getResult(); + + self::assertSame(4242, $result['quickpayPaymentId']); + self::assertSame('ut000000000001', $result['order_id']); + self::assertSame('DKK', $result['currency']); + + $requests = $this->getRequests(); + self::assertCount(1, $requests, 'Only the lookup — nothing is created'); + $this->assertRequest($requests[0], 'GET', '#/payments$#'); + } + + /** + * @return iterable}> + */ + public static function reusablePaymentProvider(): iterable + { + yield 'created, window never completed' => [['state' => PaymentState::Initial->value, 'operations' => []]]; + yield 'declined attempt' => [['state' => PaymentState::Rejected->value, 'operations' => [ + ['id' => 1, 'type' => 'authorize', 'amount' => 100, 'pending' => false, 'qp_status_code' => '40000'], + ]]]; + yield 'authorize still in flight (3-D Secure in another tab)' => [['state' => PaymentState::Pending->value, 'operations' => [ + ['id' => 1, 'type' => 'authorize', 'amount' => 100, 'pending' => true, 'qp_status_code' => null], + ]]]; + } + + /** + * A payment that HAS an approved operation is never adopted silently: it may be this order's + * earlier payment that really was paid, or another environment's payment under a prefix that + * should not be shared. Either way that is the shop's call, so it is a clear exception — never a + * claim of money. + * + * @param list> $operations + */ + #[Test] + #[DataProvider('paidPaymentProvider')] + public function shouldRefuseToAdoptAnExistingPaymentThatWasPaid(string $state, array $operations, string $expectedType): void + { + $convert = new Convert($this->createPayment(), 'array'); + + $this->queueResponse('[' . $this->paymentJson(['id' => 4242, 'order_id' => 'ut000000000001', 'state' => $state, 'operations' => $operations]) . ']'); + + $action = new ConvertPaymentAction(); + $action->setGateway($this->gateway); + $action->setApi($this->api); + + $this->expectException(LogicException::class); + $this->expectExceptionMessage(sprintf('already exists (id 4242, state %s) and has an approved %s', $state, $expectedType)); + + try { + $action->execute($convert); + } finally { + self::assertCount(1, $this->getRequests(), 'Only the lookup — nothing is created'); + } + } + + /** + * @return iterable>, string}> + */ + public static function paidPaymentProvider(): iterable + { + $authorize = ['id' => 1, 'type' => 'authorize', 'amount' => 100, 'pending' => false, 'qp_status_code' => '20000']; + + yield 'authorized' => [PaymentState::New->value, [$authorize], 'authorize']; + yield 'captured' => [PaymentState::Processed->value, [$authorize, ['id' => 2, 'type' => 'capture', 'amount' => 100, 'pending' => false, 'qp_status_code' => '20000']], 'capture']; + yield 'cancelled' => [PaymentState::Processed->value, [$authorize, ['id' => 2, 'type' => 'cancel', 'amount' => 100, 'pending' => false, 'qp_status_code' => '20000']], 'cancel']; + } + + /** + * The existing payment is what Quickpay charges in; a retry in another currency cannot reuse it. + */ + #[Test] + public function shouldRefuseToAdoptAnExistingPaymentInAnotherCurrency(): void + { + $convert = new Convert($this->createPayment(), 'array'); + + $this->queueResponse('[' . $this->paymentJson(['id' => 4242, 'order_id' => 'ut000000000001', 'currency' => 'EUR', 'state' => PaymentState::Initial->value]) . ']'); + + $action = new ConvertPaymentAction(); + $action->setGateway($this->gateway); + $action->setApi($this->api); + + $this->expectException(LogicException::class); + $this->expectExceptionMessage('already exists (id 4242) in EUR, but this payment is in DKK'); + + try { + $action->execute($convert); + } finally { + self::assertCount(1, $this->getRequests()); + } + } + /** * A Quickpay payment's currency is fixed at creation — the authorize happens in that currency no * matter what the details say. Overwriting the stored currency, as the action used to, let the diff --git a/tests/ApiTestTrait.php b/tests/ApiTestTrait.php index 892a72b..b6f33d4 100644 --- a/tests/ApiTestTrait.php +++ b/tests/ApiTestTrait.php @@ -13,10 +13,10 @@ use Setono\Payum\Quickpay\Action\Api\ConfirmPaymentAction; use Setono\Payum\Quickpay\Action\Api\CreatePaymentLinkAction; use Setono\Payum\Quickpay\Api; -use Setono\Payum\Quickpay\Operations; use Setono\Quickpay\Client\Client; use Setono\Quickpay\Enum\OperationType; use Setono\Quickpay\Enum\PaymentState; +use Setono\Quickpay\Response\Payment\Operation; /** * Shared setup for tests exercising the actions and the {@see Api}. The SDK client is built around a @@ -136,7 +136,7 @@ protected function link(bool $autoCapture): array * * @return array */ - protected function operation(OperationType $type, ?string $statusCode = Operations::APPROVED_STATUS_CODE, int $amount = 100, bool $pending = false): array + protected function operation(OperationType $type, ?string $statusCode = Operation::QP_STATUS_APPROVED, int $amount = 100, bool $pending = false): array { return [ 'id' => 1, diff --git a/tests/GatewayIntegrationTest.php b/tests/GatewayIntegrationTest.php index a8f984b..f8d7360 100644 --- a/tests/GatewayIntegrationTest.php +++ b/tests/GatewayIntegrationTest.php @@ -88,6 +88,7 @@ public function shouldRunTheCaptureDrivenCheckoutThroughTheFactoryWiredGateway() $token->setAfterUrl('https://shop.example/after'); $token->setGatewayName(QuickpayGatewayFactory::NAME); + $this->queueResponse('[]'); $this->queuePayment(['id' => 2002, 'order_id' => 'it000000000001']); $convert = new Convert($payment, 'array', $token); @@ -162,10 +163,11 @@ public function shouldRunTheCaptureDrivenCheckoutThroughTheFactoryWiredGateway() // -- The wire log: exactly the calls the flow implies, in order, authenticated with the // integration credentials — and NOT ONE capture issued by the gateway itself. $requests = $this->httpClient->getRequests(); - self::assertCount(6, $requests); + self::assertCount(7, $requests); $expected = [ - ['POST', '#/payments$#'], // Convert + ['GET', '#/payments$#'], // Convert: is there a payment under this order id? + ['POST', '#/payments$#'], // Convert: no — create it ['GET', '#/payments/2002$#'], // Capture #1: where is the payment? ['PUT', '#/payments/2002/link$#'], // Capture #1: the link ['GET', '#/payments/2002$#'], // Notify → ConfirmPayment @@ -183,7 +185,7 @@ public function shouldRunTheCaptureDrivenCheckoutThroughTheFactoryWiredGateway() ); } - $link = $this->decodeBody($requests[2]); + $link = $this->decodeBody($requests[3]); self::assertTrue($link['auto_capture'], 'Capture-driven: the link captures at authorization'); self::assertSame('https://shop.example/notify?payum_token=stub-notify', $link['callback_url'], 'The minted notify token url is what Quickpay is given'); self::assertSame('https://shop.example/capture?payum_token=cap', $link['continue_url']); diff --git a/tests/OperationsTest.php b/tests/OperationsTest.php index a4834e4..96828c4 100644 --- a/tests/OperationsTest.php +++ b/tests/OperationsTest.php @@ -94,12 +94,18 @@ public function latestOfTypeReturnsTheNewestOperationOfThatTypeRegardlessOfOutco self::assertNull(Operations::latestOfType([], OperationType::Capture)); } + /** + * "Approved" is the SDK's definition (Operation::isApproved(): not pending AND status 20000), and + * every helper here builds on it — a pending operation is never approved, whatever its code says. + */ #[Test] - public function isApprovedReflectsTheStatusCode(): void + public function approvalIsTheSdksDefinition(): void { - self::assertTrue(Operations::isApproved($this->operation(1, OperationType::Capture, '20000'))); - self::assertFalse(Operations::isApproved($this->operation(1, OperationType::Capture, '40000'))); - self::assertFalse(Operations::isApproved($this->operation(1, OperationType::Capture, null))); + $pendingWithCode = new Operation(id: 1, type: OperationType::Capture->value, amount: 100, pending: true, qpStatusCode: '20000'); + + self::assertFalse(Operations::isApprovedOfType($pendingWithCode, OperationType::Capture)); + self::assertFalse(Operations::hasApproved([$pendingWithCode], OperationType::Capture)); + self::assertNull(Operations::latestApproved([$pendingWithCode])); } #[Test]