Is Process Manager a suitable solution when refunding order units? #91
Comments
|
While it's true, that whole process should be atomic and generate either all or nothing, I do not agree, that Refund Payment and Credit memo shouldn't be related. Credit Memos are not generated without reason and if for some reason you don't want to add strict DB relationship, it's possible to add something like For example, I need to implement Google Analytics refunds and supply transaction id, I may use Credit Memo number for that. Additionally, I think Credit Memo should be generated only after Refund Payment is completed, not at the same time as Refunds are created. (Credit Memo is a document for refund payment, so I don't see how they can be unrelated). On separate note, I think Refunds should be grouped somehow per request. It should be possible to different between multiple Refunds by in which request they were done (Source of the Refund Payment and Credit Memo) as right now it's not possible to know which Refunds were the source of which Refund Payments and Credit Memos |
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
|
Closing as it has been fixed in #296 |
Here's the thing:
We, as Sylius Core Team members, decided to avoid a direct relation between Credit Memo and Refund Payment entities in order to make Refund Plugin's logic possibly flexible and extendable.
Hence, we implemented two separate ProcessManagers that are listeners for UnitsRefunded event - one of them is responsible for creating Credit Memo, the other for creating Refund Payment. Technically, there is no relation between these two entities but...
Let's assume that CreditMemoProcessManger is the first listener to receive an event. When the process crashes, one can always stop event's propagation in
catchclause and avoid processing it by RefundPaymentProcessManager.However, since both services are Process Managers and there is no rollback mechanism, there's nothing we can do when an error occurs in the last manager. It can result in two possible inconsistent states when there's a Credit Memo with no Refund Payment or a Refund Payment without Credit Memo.
During small office discussion we've thought about reimplementing UnitsRefunded event listeners in order to create a mechanism similar to a saga with proper rollback mechanisms implemented.
What do you guys think?
The text was updated successfully, but these errors were encountered: