[PAYSHIP-3135] Refacto CapturePayPalOrderCommandHandler #1299
Conversation
| @@ -0,0 +1,31 @@ | |||
| <?php | |||
|
|
|||
There was a problem hiding this comment.
You need to add the license header (stuff like * @license https://opensource.org/licenses/AFL-3.0 Academic Free License version 3.0 ...) 😉
There was a problem hiding this comment.
There a composer script set-license-header ;)
|
CorentinFlament
changed the title
|
|
||
| class CapturePayloadBuilder | ||
| { | ||
| private $payPalOrderRepository; |
There was a problem hiding this comment.
| private $payPalOrderRepository; | |
| private PayPalOrderRepository $payPalOrderRepository; |
| $this->payPalOrderRepository = $payPalOrderRepository; | ||
| } | ||
|
|
||
| public function buildPayload($mode, $orderId, $merchantId) |
There was a problem hiding this comment.
Can you add phpdoc and type hinting, please ?
|
|
||
| class CaptureOrderPayloadDTO | ||
| { | ||
| private $mode; |
| private $payee; | ||
| private $vault = false; | ||
|
|
||
| private function __construct($mode, $orderId, $payee, $vault) |
| $this->vault = $vault; | ||
| } | ||
|
|
||
| public static function create($mode, $orderId, $merchantId, $vault) |
There was a problem hiding this comment.
Can you add phpdoc and type hinting ?
| return $paymentToken; | ||
| } | ||
|
|
||
| private function processTheCapture($orderPayPal) |
There was a problem hiding this comment.
| private function processTheCapture($orderPayPal) | |
| private function processCapture($orderPayPal) |
| } | ||
| } | ||
|
|
||
| return $this->processTheCapture($orderPayPal); |
There was a problem hiding this comment.
| return $this->processTheCapture($orderPayPal); | |
| return $this->processCapture($orderPayPal); |
|
|
||
| class CaptureOrderDTO | ||
| { | ||
| private $toto; |
| /** | ||
| * @var string | ||
| */ | ||
| private $environment; |
There was a problem hiding this comment.
Is it the paypal mode ? SANDBOX or LIVE ? If it's the case, it's better call it $mode
|
Missing before OK:
|
|
|
||
| if ($orderPayPal['status'] === PayPalOrderStatus::COMPLETED) { | ||
| $this->logger->info(''); |
|
|
||
| if (isset($orderPayPal['payment_source'][$capturePayPalOrderCommand->getFundingSource()]['attributes']['vault'])) { | ||
| $vault = $orderPayPal['payment_source'][$capturePayPalOrderCommand->getFundingSource()]['attributes']['vault']; | ||
| $vault = $this->savePrestaShopPayPalCustomerRelationship($vault); |
There was a problem hiding this comment.
To me this part of overwriting $vault variable feels a bit off. Why not wrap it in try-catch?
| } catch (\Exception $exception) { | ||
| $this->logger->error(sprintf('An error occurs during process : %s', $exception->getMessage())); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
If you decide to keep catch clauses here, then you don't need a return at each one. Just one at the end of the function
No description provided.