Refactor BeforeShipmentObserver (#753)

Instead of using a big nest of ifs and try-catch, this makes use of early returns to leave the bulk of the logic readable.

It also adds some logging to the class.
Co-authored-by: Attila Kiss <42297201+cyattilakiss@users.noreply.github.com>

Observer/Adminhtml/BeforeShipmentObserver.php

@@ -24,60 +24,110 @@
 
 namespace Adyen\Payment\Observer\Adminhtml;
 
+use Adyen\Payment\Helper\Data as AdyenHelper;
+use Adyen\Payment\Logger\AdyenLogger;
+use Adyen\Payment\Observer\AdyenHppDataAssignObserver;
+use Exception;
 use Magento\Framework\Event\Observer;
 use Magento\Payment\Observer\AbstractDataAssignObserver;
+use Magento\Sales\Model\Order\InvoiceRepository;
+use Magento\Sales\Model\Order\Shipment;
+use Psr\Log\LoggerInterface;
+use Throwable;
 
 class BeforeShipmentObserver extends AbstractDataAssignObserver
 {
     private $adyenHelper;
+    /**
+     * @var LoggerInterface
+     */
+    private $logger;
+
+    /**
+     * @var InvoiceRepository
+     */
+    private $invoiceRepository;
 
     /**
      * BeforeShipmentObserver constructor.
      *
-     * @param \Adyen\Payment\Helper\Data $adyenHelper
+     * @param AdyenHelper $adyenHelper
+     * @param AdyenLogger $logger
+     * @param InvoiceRepository $invoiceRepository
      */
     public function __construct(
-        \Adyen\Payment\Helper\Data $adyenHelper
+        AdyenHelper $adyenHelper,
+        AdyenLogger $logger,
+        InvoiceRepository $invoiceRepository
     ) {
         $this->adyenHelper = $adyenHelper;
+        $this->logger = $logger;
+        $this->invoiceRepository = $invoiceRepository;
     }
 
     /**
      * @param Observer $observer
-     * @throws \Magento\Framework\Exception\LocalizedException
+     * @throws Exception
      */
     public function execute(Observer $observer)
     {
-        $shipment = $observer->getEvent()->getShipment();
+        /** @var Shipment $shipment */
+        $shipment = $observer->getEvent()->getData('shipment');
         $order = $shipment->getOrder();
+
+        if (!$this->isPaymentMethodAdyen($order)) {
+            $this->logger->info(
+                "Payment method is not from Adyen for order id {$order->getId()}",
+                ['observer' => 'BeforeShipmentObserver']
+            );
+            return;
+        }
+
         $captureOnShipment = $this->adyenHelper->getConfigData(
             'capture_on_shipment',
             'adyen_abstract',
             $order->getStoreId()
         );
 
-        if ($this->isPaymentMethodAdyen($order) && $captureOnShipment) {
-            $payment = $order->getPayment();
-            $brandCode = $payment->getAdditionalInformation(
-                \Adyen\Payment\Observer\AdyenHppDataAssignObserver::BRAND_CODE
+        if (!$captureOnShipment) {
+            $this->logger->info(
+                "Capture on shipment not configured for order id {$order->getId()}",
+                ['observer' => 'BeforeShipmentObserver']
             );
+            return;
+        }
+
+        $payment = $order->getPayment();
+        $brandCode = $payment->getAdditionalInformation(AdyenHppDataAssignObserver::BRAND_CODE);
+
+        if (!$this->adyenHelper->isPaymentMethodOpenInvoiceMethod($brandCode)) {
+            $this->logger->info(
+                "Payment method is from Adyen but isn't OpenInvoice for order id {$order->getId()}",
+                ['observer' => 'BeforeShipmentObserver']
+            );
+            return;
+        }
+
+        if (!$order->canInvoice()) {
+            $this->logger->info(
+                "Cannot invoice order with id {$order->getId()}",
+                ['observer' => 'BeforeShipmentObserver']
+            );
+            return;
+        }
 
-            if ($this->adyenHelper->isPaymentMethodOpenInvoiceMethod($brandCode)) {
-                if ($order->canInvoice()) {
-                    try {
-                        $invoice = $order->prepareInvoice();
-                        $invoice->getOrder()->setIsInProcess(true);
+        try {
+            $invoice = $order->prepareInvoice();
+            $invoice->getOrder()->setIsInProcess(true);
 
-                        // set transaction id so you can do a online refund from credit memo
-                        $pspReference = $order->getPayment()->getAdyenPspReference();
-                        $invoice->setTransactionId($pspReference);
-                        $invoice->register()->pay();
-                        $invoice->save();
-                    } catch (\Exception $e) {
-                        throw new \Exception(sprintf('Error saving invoice. The error message is:', $e->getMessage()));
-                    }
-                }
-            }
+            // set transaction id so you can do a online refund from credit memo
+            $pspReference = $order->getPayment()->getAdyenPspReference();
+            $invoice->setTransactionId($pspReference);
+            $invoice->register()->pay();
+            $this->invoiceRepository->save($invoice);
+        } catch (Throwable $e) {
+            $this->logger->error($e);
+            throw new Exception('Error saving invoice. The error message is: ' . $e->getMessage());
         }
     }
 

etc/di.xml

@@ -900,13 +900,6 @@
         </arguments>
     </virtualType>
 
-    <!--Checkout Response validator-->
-    <virtualType name="CheckoutResponseValidator" type="Adyen\Payment\Gateway\Validator\CheckoutResponseValidator">
-        <arguments>
-            <argument name="loggerInterface" xsi:type="object">Adyen\Payment\Logger\AdyenLogger</argument>
-        </arguments>
-    </virtualType>
-
     <virtualType name="PosCloudResponseValidator" type="Adyen\Payment\Gateway\Validator\PosCloudResponseValidator">
         <arguments>
             <argument name="loggerInterface" xsi:type="object">Adyen\Payment\Logger\AdyenLogger</argument>