diff --git a/src/Action/ConvertPaymentAction.php b/src/Action/ConvertPaymentAction.php index 19a0903..ec2772e 100644 --- a/src/Action/ConvertPaymentAction.php +++ b/src/Action/ConvertPaymentAction.php @@ -40,7 +40,6 @@ public function execute($request): void $details = ArrayObject::ensureArrayObject($paymentModel->getDetails()); $details['amount'] = $paymentModel->getTotalAmount(); - $details['currency'] = $paymentModel->getCurrencyCode(); // Only scalars are stored in the details so they survive serialization by the consumer. // `quickpayPaymentId` is the single source of truth; the payment is re-fetched when needed. @@ -58,6 +57,9 @@ public function execute($request): void $details['quickpayPaymentId'] = $payment->id; $details['order_id'] = $payment->orderId; + $details['currency'] = $currency; + } else { + self::assertCurrencyUnchanged($details, $paymentModel->getCurrencyCode()); } if (null !== $token = $request->getToken()) { @@ -98,6 +100,42 @@ private static function assertNotEmptyString(mixed $value, string $what, string return $value; } + /** + * A Quickpay payment's currency is fixed when it is created — the authorize will happen in that + * currency no matter what the details say. Before this guard, a Payum payment whose currency + * changed after conversion simply had its stored `currency` overwritten: the shop then believed + * one currency while Quickpay kept charging in the other, with the amount silently read in the + * wrong unit. A drifted currency is a hard error; the payment must be cancelled and a fresh one + * converted instead. + * + * A model without a currency (both properties are nullable on Payum's model) leaves the stored + * value alone, and a model that agrees is a no-op refresh. + * + * @param ArrayObject $details + * + * @throws LogicException if the model's currency no longer matches the created payment's + */ + private static function assertCurrencyUnchanged(ArrayObject $details, mixed $currency): void + { + if (!is_string($currency) || '' === $currency) { + return; + } + + $stored = $details['currency'] ?? null; + + if (is_string($stored) && '' !== $stored && $stored !== $currency) { + throw new LogicException(sprintf( + 'The Quickpay payment %s was created in %s, but the Payum payment now says %s. A Quickpay ' + . 'payment cannot change currency — cancel it and convert a new payment instead.', + is_scalar($details['quickpayPaymentId']) ? (string) $details['quickpayPaymentId'] : '(unknown)', + $stored, + $currency, + )); + } + + $details['currency'] = $currency; + } + /** * 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/tests/Action/ConvertPaymentActionTest.php b/tests/Action/ConvertPaymentActionTest.php index 7a8272c..986624e 100644 --- a/tests/Action/ConvertPaymentActionTest.php +++ b/tests/Action/ConvertPaymentActionTest.php @@ -216,6 +216,63 @@ public function shouldAcceptOrderIdsAtBothEndsOfTheRange(): void } } + /** + * 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 + * shop believe one currency while Quickpay kept charging in the other. + * + * @test + */ + public function shouldThrowWhenTheCurrencyChangedAfterCreation(): void + { + $payment = $this->createPayment(); + $payment->setCurrencyCode('EUR'); + $payment->setDetails(['quickpayPaymentId' => 555, 'currency' => 'DKK']); + + $convert = new Convert($payment, 'array'); + + $action = new ConvertPaymentAction(); + $action->setGateway($this->gateway); + $action->setApi($this->api); + + $this->expectException(LogicException::class); + $this->expectExceptionMessage('cannot change currency'); + + try { + $action->execute($convert); + } finally { + self::assertCount(0, $this->getRequests(), 'No request may be issued for a drifted payment'); + } + } + + /** + * Payum's model allows a null currency. That is nothing to compare against, so the stored value + * — the one the Quickpay payment was actually created with — must survive instead of being + * overwritten with null. + * + * @test + */ + public function shouldKeepTheStoredCurrencyWhenTheModelCarriesNone(): void + { + $payment = new Payment(); + $payment->setNumber('000000000001'); + $payment->setTotalAmount(100); + $payment->setDetails(['quickpayPaymentId' => 555, 'currency' => 'DKK']); + + $convert = new Convert($payment, 'array'); + + $action = new ConvertPaymentAction(); + $action->setGateway($this->gateway); + $action->setApi($this->api); + $action->execute($convert); + + /** @var array $result */ + $result = $convert->getResult(); + + self::assertSame('DKK', $result['currency']); + self::assertCount(0, $this->getRequests()); + } + /** * @test */