bug #296 Always create RefundPayment after CreditMemo (Zales0123)

This PR was merged into the 1.0-dev branch.

Discussion
----------

Partially solves #91

With the current implementation, both **CreditMemo** and **RefundPayment** are generated separately, which can results in a situation when one of them exists and the other does not. With the proposed change, we're protected from such a situation.

For sure, the solution is not perfect. The whole compensation/rollback functionality is based on the [DoctrineTransactionMiddleware](https://github.com/symfony/doctrine-bridge/blob/5.4/Messenger/DoctrineTransactionMiddleware.php#L37), which works in this case but does not have to be enough for some more complicated processes. A possible improvement of the proposed architecture would be adding the `prev/compensate/rollback` function to the `UnitsRefundedProcessStepInterface` to implement a more mature and reliable saga pattern 🖖 

Commits
-------

261e0d3 Change current process manager to process steps
82f42a1 Describe desired post-refund behaviour with scenarios
7cf06b7 Tag services and document the whole process
90db258 PR review fixes

README.md

@@ -161,6 +161,23 @@ as shown below:
 Online refund logic should be implemented if you need it.
 As the first try for the possible customization, you can check out `Sylius\RefundPlugin\Event\UnitsRefunded` event.
 
+## Post-refunding process
+
+After units are refunded, there are multiple other processes that should be triggered. By default, after units refund, there should be **CreditMemo** and
+**RefundPayment** generated. As they're strictly coupled with each other, **RefundPayment** is always created after the **CreditMemo**. Moreover, if **RefundPayment**
+fails, related **CreditMemo** should not be created as well.
+
+`Sylius\RefundPlugin\ProcessManager\UnitsRefundedProcessManager` service facilitates the whole process. If you want to add one or more steps to it, you should create
+a service implementing `Sylius\RefundPlugin\ProcessManager\UnitsRefundedProcessStepInterface`, and register if with proper tag:
+
+```yaml
+    App\ProcessManager\CustomAfterRefundProcessManager:
+       tags:
+          - { name: sylius_refund.units_refunded.process_step, priority: 0 }
+```
+
+Tagged services would be executed according to their priority (descending). 
+
 ## Security issues
 
 If you think that you have found a security issue, please do not use the issue tracker and do not post it publicly.

UPGRADE.md

@@ -26,6 +26,13 @@ is created by `Sylius\RefundPlugin\Factory\CreditMemoFactory`.
         }
     ```
 
+1. Post-units refunded process (containing credit memo and refund payment generation) was changed to synchronous step. Refund payment is therefore
+always generated after credit memo. Technical changes:
+    * `Sylius\RefundPlugin\ProcessManager\CreditMemoProcessManager` and `Sylius\RefundPlugin\ProcessManager\RefundPaymentProcessManager` no longer 
+       directly listen to `Sylius\RefundPlugin\Event\UnitsRefunded` event. `Sylius\RefundPlugin\ProcessManager\UnitsRefundedProcessManager` uses them to 
+       facilitate post-units refunding process.
+    * Their `__invoke` methods were replaced by `Sylius\RefundPlugin\ProcessManager\UnitsRefundedProcessStepInterface::next(UnitsRefunded $unitsRefunded)`. 
+
 ### UPGRADE FROM 1.0.0-RC.7 TO 1.0.0-RC.8
 
 1. The `fully_refunded` state and the `refund` transition have been removed from `sylius_order` state machine.

features/not_seeing_refund_payments_and_credit_memos_on_admin_order_view.feature

@@ -0,0 +1,33 @@
+@refunds
+Feature: Not seeing refund payments and credit memos on admin order view
+    In order to have consistent documents and payment in a refunded order
+    As an Administrator
+    I don't want to have a credit memo or refund payment generated alone
+
+    Background:
+        Given the store operates on a single green channel in "United States"
+        And the store has a product "Mr. Meeseeks T-Shirt" priced at "$10.00"
+        And the store has a product "Angel T-Shirt" priced at "$5.00"
+        And the store allows shipping with "Galaxy Post"
+        And the store allows paying with "Space money"
+        And there is a customer "[email protected]" that placed an order "#00000023"
+        And the customer bought 2 "Mr. Meeseeks T-Shirt" products
+        And the customer chose "Galaxy Post" shipping method to "United States" with "Space money" payment
+        And the order "#00000023" is already paid
+        And I am logged in as an administrator
+
+    @ui
+    Scenario: Not seeing credit memo and refund payment on order view if credit memo generation failed
+        Given the credit memo generation is broken
+        And I decided to refund 1st "Mr. Meeseeks T-Shirt" product of the order "00000023" with "Space money" payment
+        When I view the summary of the order "#00000023"
+        Then I should not see any refund payments
+        And I should not see any credit memos
+
+    @ui
+    Scenario: Not seeing credit memo and refund payment on order view if refund generation failed
+        Given the refund payment generation is broken
+        And I decided to refund 1st "Mr. Meeseeks T-Shirt" product of the order "00000023" with "Space money" payment
+        When I view the summary of the order "#00000023"
+        Then I should not see any refund payments
+        And I should not see any credit memos

spec/ProcessManager/CreditMemoProcessManagerSpec.php

@@ -9,6 +9,7 @@
 use Sylius\RefundPlugin\Event\UnitsRefunded;
 use Sylius\RefundPlugin\Model\OrderItemUnitRefund;
 use Sylius\RefundPlugin\Model\ShipmentRefund;
+use Sylius\RefundPlugin\ProcessManager\UnitsRefundedProcessStepInterface;
 use Symfony\Component\Messenger\Envelope;
 use Symfony\Component\Messenger\MessageBusInterface;
 
@@ -19,6 +20,11 @@ function let(MessageBusInterface $commandBus): void
         $this->beConstructedWith($commandBus);
     }
 
+    function it_implements_units_refunded_process_step_interface(): void
+    {
+        $this->shouldImplement(UnitsRefundedProcessStepInterface::class);
+    }
+
     function it_reacts_on_units_generated_event_and_dispatch_generate_credit_memo_command(MessageBusInterface $commandBus)
     {
         $unitRefunds = [new OrderItemUnitRefund(1, 1000), new OrderItemUnitRefund(3, 2000), new OrderItemUnitRefund(5, 3000)];
@@ -27,6 +33,6 @@ function it_reacts_on_units_generated_event_and_dispatch_generate_credit_memo_co
         $command = new GenerateCreditMemo('000222', 3000, $unitRefunds, $shipmentRefunds, 'Comment');
         $commandBus->dispatch($command)->willReturn(new Envelope($command))->shouldBeCalled();
 
-        $this(new UnitsRefunded('000222', $unitRefunds, $shipmentRefunds, 1, 3000, 'USD', 'Comment'));
+        $this->next(new UnitsRefunded('000222', $unitRefunds, $shipmentRefunds, 1, 3000, 'USD', 'Comment'));
     }
 }

spec/ProcessManager/RefundPaymentProcessManagerSpec.php

@@ -12,6 +12,7 @@
 use Sylius\RefundPlugin\Factory\RefundPaymentFactoryInterface;
 use Sylius\RefundPlugin\Model\OrderItemUnitRefund;
 use Sylius\RefundPlugin\Model\ShipmentRefund;
+use Sylius\RefundPlugin\ProcessManager\UnitsRefundedProcessStepInterface;
 use Sylius\RefundPlugin\Provider\RelatedPaymentIdProviderInterface;
 use Sylius\RefundPlugin\StateResolver\OrderFullyRefundedStateResolverInterface;
 use Symfony\Component\Messenger\Envelope;
@@ -35,6 +36,11 @@ function let(
         );
     }
 
+    function it_implements_units_refunded_process_step_interface(): void
+    {
+        $this->shouldImplement(UnitsRefundedProcessStepInterface::class);
+    }
+
     function it_reacts_on_units_refunded_event_and_creates_refund_payment(
         OrderFullyRefundedStateResolverInterface $orderFullyRefundedStateResolver,
         RelatedPaymentIdProviderInterface $relatedPaymentIdProvider,
@@ -65,6 +71,14 @@ function it_reacts_on_units_refunded_event_and_creates_refund_payment(
         $event = new RefundPaymentGenerated(10, '000222', 1000, 'USD', 1, 3);
         $eventBus->dispatch($event)->willReturn(new Envelope($event))->shouldBeCalled();
 
-        $this(new UnitsRefunded('000222', [new OrderItemUnitRefund(1, 500), new OrderItemUnitRefund(2, 500)], [new ShipmentRefund(1, 300)], 1, 1000, 'USD', 'Comment'));
+        $this->next(new UnitsRefunded(
+            '000222',
+            [new OrderItemUnitRefund(1, 500), new OrderItemUnitRefund(2, 500)],
+            [new ShipmentRefund(1, 300)],
+            1,
+            1000,
+            'USD',
+            'Comment'
+        ));
     }
 }

spec/ProcessManager/UnitsRefundedProcessManagerSpec.php

@@ -0,0 +1,41 @@
+<?php
+
+declare(strict_types=1);
+
+namespace spec\Sylius\RefundPlugin\ProcessManager;
+
+use PhpSpec\ObjectBehavior;
+use Sylius\RefundPlugin\Event\UnitsRefunded;
+use Sylius\RefundPlugin\Model\OrderItemUnitRefund;
+use Sylius\RefundPlugin\Model\ShipmentRefund;
+use Sylius\RefundPlugin\ProcessManager\UnitsRefundedProcessManagerInterface;
+use Sylius\RefundPlugin\ProcessManager\UnitsRefundedProcessStepInterface;
+
+final class UnitsRefundedProcessManagerSpec extends ObjectBehavior
+{
+    function let(
+        UnitsRefundedProcessStepInterface $creditMemoProcessManager,
+        UnitsRefundedProcessStepInterface $refundPaymentProcessManager
+    ): void {
+        $this->beConstructedWith([$creditMemoProcessManager, $refundPaymentProcessManager]);
+    }
+
+    function it_implements_units_refunded_process_manager_interface(): void
+    {
+        $this->shouldImplement(UnitsRefundedProcessManagerInterface::class);
+    }
+
+    function it_triggers_all_process_steps_if_all_are_successful(
+        UnitsRefundedProcessStepInterface $creditMemoProcessManager,
+        UnitsRefundedProcessStepInterface $refundPaymentProcessManager
+    ): void {
+        $unitRefunds = [new OrderItemUnitRefund(1, 1000), new OrderItemUnitRefund(3, 2000), new OrderItemUnitRefund(5, 3000)];
+        $shipmentRefunds = [new ShipmentRefund(1, 500), new ShipmentRefund(2, 1000)];
+        $event = new UnitsRefunded('000222', $unitRefunds, $shipmentRefunds, 1, 1500, 'USD', 'Comment');
+
+        $creditMemoProcessManager->next($event)->shouldBeCalled();
+        $refundPaymentProcessManager->next($event)->shouldBeCalled();
+
+        $this($event);
+    }
+}

src/ProcessManager/CreditMemoProcessManager.php

@@ -8,7 +8,7 @@
 use Sylius\RefundPlugin\Event\UnitsRefunded;
 use Symfony\Component\Messenger\MessageBusInterface;
 
-final class CreditMemoProcessManager
+final class CreditMemoProcessManager implements UnitsRefundedProcessStepInterface
 {
     /** @var MessageBusInterface */
     private $commandBus;
@@ -18,14 +18,14 @@ public function __construct(MessageBusInterface $commandBus)
         $this->commandBus = $commandBus;
     }
 
-    public function __invoke(UnitsRefunded $event): void
+    public function next(UnitsRefunded $unitsRefunded): void
     {
         $this->commandBus->dispatch(new GenerateCreditMemo(
-            $event->orderNumber(),
-            $event->amount(),
-            $event->units(),
-            $event->shipments(),
-            $event->comment()
+            $unitsRefunded->orderNumber(),
+            $unitsRefunded->amount(),
+            $unitsRefunded->units(),
+            $unitsRefunded->shipments(),
+            $unitsRefunded->comment()
         ));
     }
 }

src/ProcessManager/RefundPaymentProcessManager.php

@@ -13,7 +13,7 @@
 use Sylius\RefundPlugin\StateResolver\OrderFullyRefundedStateResolverInterface;
 use Symfony\Component\Messenger\MessageBusInterface;
 
-final class RefundPaymentProcessManager
+final class RefundPaymentProcessManager implements UnitsRefundedProcessStepInterface
 {
     /** @var OrderFullyRefundedStateResolverInterface */
     private $orderFullyRefundedStateResolver;
@@ -44,28 +44,28 @@ public function __construct(
         $this->eventBus = $eventBus;
     }
 
-    public function __invoke(UnitsRefunded $event): void
+    public function next(UnitsRefunded $unitsRefunded): void
     {
         $refundPayment = $this->refundPaymentFactory->createWithData(
-            $event->orderNumber(),
-            $event->amount(),
-            $event->currencyCode(),
+            $unitsRefunded->orderNumber(),
+            $unitsRefunded->amount(),
+            $unitsRefunded->currencyCode(),
             RefundPaymentInterface::STATE_NEW,
-            $event->paymentMethodId()
+            $unitsRefunded->paymentMethodId()
         );
 
         $this->entityManager->persist($refundPayment);
         $this->entityManager->flush();
 
         $this->eventBus->dispatch(new RefundPaymentGenerated(
             $refundPayment->getId(),
-            $event->orderNumber(),
-            $event->amount(),
-            $event->currencyCode(),
-            $event->paymentMethodId(),
+            $unitsRefunded->orderNumber(),
+            $unitsRefunded->amount(),
+            $unitsRefunded->currencyCode(),
+            $unitsRefunded->paymentMethodId(),
             $this->relatedPaymentIdProvider->getForRefundPayment($refundPayment)
         ));
 
-        $this->orderFullyRefundedStateResolver->resolve($event->orderNumber());
+        $this->orderFullyRefundedStateResolver->resolve($unitsRefunded->orderNumber());
     }
 }

src/ProcessManager/UnitsRefundedProcessManager.php

@@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sylius\RefundPlugin\ProcessManager;
+
+use Sylius\RefundPlugin\Event\UnitsRefunded;
+
+final class UnitsRefundedProcessManager implements UnitsRefundedProcessManagerInterface
+{
+    /** @var iterable|UnitsRefundedProcessStepInterface[] */
+    private $steps;
+
+    public function __construct(iterable $steps)
+    {
+        $this->steps = $steps;
+    }
+
+    public function __invoke(UnitsRefunded $event): void
+    {
+        /** @var UnitsRefundedProcessStepInterface $step */
+        foreach ($this->steps as $step) {
+            $step->next($event);
+        }
+    }
+}

src/ProcessManager/UnitsRefundedProcessManagerInterface.php

@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sylius\RefundPlugin\ProcessManager;
+
+use Sylius\RefundPlugin\Event\UnitsRefunded;
+
+interface UnitsRefundedProcessManagerInterface
+{
+    public function __invoke(UnitsRefunded $event): void;
+}

src/ProcessManager/UnitsRefundedProcessStepInterface.php

@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sylius\RefundPlugin\ProcessManager;
+
+use Sylius\RefundPlugin\Event\UnitsRefunded;
+
+interface UnitsRefundedProcessStepInterface
+{
+    public function next(UnitsRefunded $unitsRefunded): void;
+}

src/Resources/config/services/event_bus.xml

@@ -19,8 +19,11 @@
             <tag name="messenger.message_handler" bus="sylius.event_bus" />
         </service>
 
-        <service id="Sylius\RefundPlugin\ProcessManager\CreditMemoProcessManager">
-            <argument type="service" id="sylius.command_bus" />
+        <service
+            id="Sylius\RefundPlugin\ProcessManager\UnitsRefundedProcessManagerInterface"
+            class="Sylius\RefundPlugin\ProcessManager\UnitsRefundedProcessManager"
+        >
+            <argument type="tagged_iterator" tag="sylius_refund.units_refunded.process_step"/>
             <tag name="messenger.message_handler" bus="sylius.event_bus" />
         </service>
 
@@ -30,7 +33,12 @@
             <argument type="service" id="Sylius\RefundPlugin\Factory\RefundPaymentFactory" />
             <argument type="service" id="doctrine.orm.default_entity_manager" />
             <argument type="service" id="sylius.event_bus" />
-            <tag name="messenger.message_handler" bus="sylius.event_bus" />
+            <tag name="sylius_refund.units_refunded.process_step" priority="50" />
+        </service>
+
+        <service id="Sylius\RefundPlugin\ProcessManager\CreditMemoProcessManager">
+            <argument type="service" id="sylius.command_bus" />
+            <tag name="sylius_refund.units_refunded.process_step" priority="100" />
         </service>
     </services>
 </container>

tests/Application/config/services_test.yaml

@@ -1,3 +1,14 @@
 imports:
     - { resource: "../../../vendor/sylius/sylius/src/Sylius/Behat/Resources/config/services.xml" }
     - { resource: "../../Behat/Resources/services.xml" }
+
+services:
+    Tests\Sylius\RefundPlugin\Behat\Services\Generator\FailedCreditMemoGenerator:
+        decorates: 'Sylius\RefundPlugin\Generator\CreditMemoGenerator'
+        arguments:
+            - '@Tests\Sylius\RefundPlugin\Behat\Services\Generator\FailedCreditMemoGenerator.inner'
+
+    Tests\Sylius\RefundPlugin\Behat\Services\Factory\FailedRefundPaymentFactory:
+        decorates: 'Sylius\RefundPlugin\Factory\RefundPaymentFactory'
+        arguments:
+            - '@Tests\Sylius\RefundPlugin\Behat\Services\Factory\FailedRefundPaymentFactory.inner'