refund: basic refund implemented #10
Conversation
online transactions are detected by payment information sent by magneto
| @@ -14,6 +14,7 @@ | |||
| use Psr\Log\LoggerInterface; | |||
| use Magento\Quote\Api\Data\PaymentInterface; | |||
| use Aligent\Pinpay\Helper\Pinpay as PinHelper; | |||
| use Magento; | |||
There was a problem hiding this comment.
This doesn't look like it belongs here, right?
There was a problem hiding this comment.
@moloughlin it is here so we don't need to access root namespace \Magento which PhpStorm shows as a warning
| $this->_logger->error("Payment Error: " . $e->getMessage()); | ||
| throw new LocalizedException(__($e->getMessage())); | ||
| } | ||
| } |
There was a problem hiding this comment.
This function should return $this to adhere to interface spec.
There was a problem hiding this comment.
will make this change
| $response = null; | ||
| try { | ||
| $response = $client->request(); | ||
| $this->_handleResponse($response, $payment); |
There was a problem hiding this comment.
Is it correct to be updating the Payment object in the refund case? I'd have thought this should hold onto the original transaction data instead, but maybe this pattern is standard elsewhere?
There was a problem hiding this comment.
here is the example of magneto core DirectPost refund
magento/vendor/magento/module-authorizenet/Model/Directpost.php:403
$payment->setAmount($amount);
$payment->setXTransId($this->getRealParentTransactionId($payment));```
in other places too it is changing payment object in refund
| @@ -461,6 +471,11 @@ protected function _handleResponse($response, $payment) | |||
| } elseif ($error) { | |||
| throw new LocalizedException(__($result->getErrorDescription())); | |||
| } | |||
| //success is set to null in response but approved (like in pending credit memo) | |||
| elseif($result->isApproved()){ | |||
There was a problem hiding this comment.
Could we just roll this up with the if to avoid repetition?
if ($result->isSuccess() || $result->isApproved()) {...}
A bit confused by the comment as well, does the refund response just have a slightly different structure to the capture response?
There was a problem hiding this comment.
@moloughlin yes refund response seem to be slightly different
here is the response from curl request (formatted JSON for readibility)
"response": {
"token": "rf_removed_for_security",
"success": null,
"amount": 10,
"currency": "AUD",
"charge": "ch_removed_for_security",
"created_at": "2018-05-24T02:52:34Z",
"error_message": null,
"status_message": "Pending"
}
}
so success, so isset returns false and function returns false for isSuccess returns false
And there is no error_message either
HTTP response code is 201 which pinpay for successful.
What makes this confusing is JSON success is different to response code 201 which is another kind of Success.
There was a problem hiding this comment.
I had a second look in our modules, both magento1 and magento2
magento2-pinpayments/Model/Result.php
Line 10 in eed7ad5
and https://github.com/aligent/PINpayments/blob/48fd120be7c530a3e69c6bbbd060878e0835a305/app/code/local/Dwyera/Pinpay/Model/Result.php#L13
we call it
ApprovedHTTP_RESPONSE_CODE_APPROVED
but in API its referred to as https://pinpayments.com/developers/api-reference/cards
Created
According to API documentation overview/errors section:
https://pinpayments.com/developers/api-reference
If an API request results in an error, a non-2xx status code will be returned, along with a JSON object containing error and error_description keys.
So not sure how should we treat absence of success (or null) in response.
A bit nervous to touch capture/authorize code which is already working.
online transactions are detected by payment information sent by magneto
@moloughlin please review