Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enhancement proposal] Free orders customizations #11927

Closed
Prometee opened this issue Oct 13, 2020 · 7 comments
Closed

[Enhancement proposal] Free orders customizations #11927

Prometee opened this issue Oct 13, 2020 · 7 comments
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).

Comments

@Prometee
Copy link
Contributor

When a free order is done sometime it could be require to have a payment to handle a subscription through a payment gateway for exemple.

Today when you create a free order in Sylius a service is removing all payments records (state === cart) thanks to the service sylius.order_processing.order_payment_processor.checkout. If you want to have a payment anyway you will need to re-add a new payment thanks to a new sylius.order_processor tagged service with a priority less than 0.

Then when you will have to add a state machine callback or to decorate the service sylius.order_processing.order_payment_processor.after_checkout which is using the same class Sylius\Component\Core\OrderProcessing\OrderPaymentProcessor to allow having payment record when a payment fail or is canceled.

What I propose is to :

  • separate the part removing payment records to a dedicated sylius.order_processor tagged service to allow developper to disable or modify more easily the behaviour of a free order.
  • add a new fail or cancel callback to sylius_payment state machine to make the same thing.

I'm open to another better idea or enhancement then I will make a PR to handle this particular case.

@Prometee
Copy link
Contributor Author

Simple solution could be to make a service deciding if it's a paymentless order or not :

if ($this->orderPaymentlessResolver->resolve($order)) {
  // delete payments with state === 'cart'
}

@Zales0123 Zales0123 added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Oct 15, 2020
@NoResponseMate
Copy link
Contributor

Hi @Prometee
It's been a while but are you still willing to provide a PR for this?

@Prometee
Copy link
Contributor Author

Hi @NoResponseMate
Before making a PR, I expected an answer about this topic to decide if it's a good idea or not to do this kind of modification.

@NoResponseMate
Copy link
Contributor

If it eases customization then it is worth it.
Unfortunately, the proposal with an additional order processor could be considered a BC break since it would change the flow and potentially break applications because someone might've already customized the process, decorated the processor in question, and so on.
IMO the second idea with simply extracting the decision on whether payments should be removed or not is the way to go.

@Prometee
Copy link
Contributor Author

@NoResponseMate that's exactly what I though too. I add this to my other tasks and will see if I can manage to create a PR within a month.

@Rafikooo
Copy link
Contributor

Hey @Prometee,

This is a friendly reminder in case you forgot about it 😆

Best wishes,
Rafał

@Prometee
Copy link
Contributor Author

Hello @Rafikooo,

I'm very sorry, I'm afraid I won't have enough time to create the PR this time. Are you able to create it ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

No branches or pull requests

4 participants