Cancel payment action #33
Conversation
src/Action/CancelAction.php
Outdated
| private const FINAL_STATUSES = [ | ||
| OrderStatus::Completed, | ||
| OrderStatus::Canceled, | ||
| ]; |
There was a problem hiding this comment.
move it to OrderStatus class,
or create a public method like OrderStatus->isFinal()
src/Action/CancelAction.php
Outdated
| $model = Model::ensureArrayObject($request->getModel()); | ||
|
|
||
| $payment = PaymentHelper::ensurePayment($request->getFirstModel()); | ||
| Assert::notNull($payment, 'Payment must be set on cancel action.'); |
There was a problem hiding this comment.
ensurePayment method returns PaymentInterface, not null. PHPStan does not report this to you? Maybe we need to update PHPStan 🤔
There was a problem hiding this comment.
no, PHPStan didn't report anything :(
src/Action/CancelAction.php
Outdated
|
|
||
| try { | ||
| if (!$this->canCancelPayment($model, $payment)) { | ||
| return; |
There was a problem hiding this comment.
if we can't cancel Payment we nned to know about it. Throw custom exception here like CannotCancelPaymentException
src/Action/CancelAction.php
Outdated
|
|
||
| $this->orderRequestService->cancel($model->orderId(), PaymentHelper::getConfigKey($model, $payment)); | ||
| } catch (\Throwable $throwable) { | ||
| $this->logger->critical('Cannot cancel payment.', ['exception' => $throwable]); |
There was a problem hiding this comment.
You can't catch whole exceptions and only log it, we need to know if something happened. Throw custom exception CancelPaymentException and pass parent to it
There was a problem hiding this comment.
I removed the try catch completely, it is not needed here
src/Action/CancelAction.php
Outdated
| $response = $this->orderRequestService->retrieve($model->orderId(), PaymentHelper::getConfigKey($model, $payment)); | ||
| $status = $response->orders[0]->status; | ||
|
|
||
| return !in_array($status, self::FINAL_STATUSES, true); |
There was a problem hiding this comment.
create OrderStatus enum from $status and use method ->isFinal() as I mention above
There was a problem hiding this comment.
I have some doubts about checking this on CancelAction method. Maybe we need to check it before run CancelAction but this action should proceed only cancel request 🤔
There was a problem hiding this comment.
Api always returns a status code of 200, regardless of whether the payment was successfully canceled or not. For example, when you send a transaction cancellation request for a transaction already paid, you get a status of 200, but after checking the status it is still paid. For this reason, I would prefer to check the status before sending the cancellation and throw a custom exception.
| $payment = new Payment(); | ||
| $payment->setDetails(FileTestUtil::decodeJsonFromFile(__DIR__ . '/data/detailsWithOrderId.json')); | ||
| $expectedBaseDetails = $this->getExpectedBaseDetails(); | ||
| self::assertSame($expectedBaseDetails, $payment->getDetails()); |
There was a problem hiding this comment.
you just set the details. We need to check it again?
There was a problem hiding this comment.
it is not needed, removed
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Answear\Payum\PayU\Tests\Integration\Action; |
There was a problem hiding this comment.
this test seems like the unit test, not integration
| $this->expectException(PayUAuthorizationException::class); | ||
| $this->expectExceptionCode(401); | ||
| $this->getOrderRequestService()->cancel('WZHF5FFDRJ140731GUEST000P01', null); | ||
| } |
There was a problem hiding this comment.
test with 400-500 code from PayU is missing
There was a problem hiding this comment.
added missing tests
| } | ||
|
|
||
| public static function isFinal(OrderStatus $status): bool | ||
| { |
There was a problem hiding this comment.
maybe not static will be better? #whatever
No description provided.