Sylius / RefundPlugin

Basic refunds functionality for Sylius
MIT License
63 stars 68 forks source link

Is Process Manager a suitable solution when refunding order units? #91

Closed bartoszpietrzak1994 closed 3 years ago

bartoszpietrzak1994 commented 6 years ago

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 catch clause 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?

diimpp commented 4 years ago

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 author_id at Sylius\Core\Model\Image, that can be reused in different ways.

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

GSadee commented 3 years ago

Closing as it has been fixed in #296