The Events - Checkout Revolution

[winzou]

The aim of this PR is to get these checkout events under control.
Better separation of checkout process / order creation events.

• Add checkout events to security step
• Add order creation related events to all checkout steps. These events deal with the order creation mechanism, independently of the checkout process. So they will be reused for backend order creation for example. We keep all the current checkout events for things related to the checkout process itself. => Let's do it in another PR.
• Add post creation events: pay, ship, return, cancel
• Add CompleteOrderListener in OrderBundle to do the $order->complete() instead of having this in FinalizeStep
• Add a method to OrderPaymentListener which listens to payment.pre_state_change. So that when the state of a payment changes to complete, this method is called and dispatches again the order.pre_pay event. (currently, nothing is done!)
• Make OrderStateListener listen at every order post creation events, so that it updates its state accordingly (priority -100 to be called after all order modification)
• Flush between payment.pre_state and payment.post_state so that modifications are persisted in db. This is done in all NotifyAction in PayumBundle
• ...

[pjedrzejewski]

👍

[pjedrzejewski]

Basically we have two types of events related to order.

1. CRUD events, but order creation is not as simple as for other models, it's a multistage process.
2. Checkout events.

Perhaps we should add another group of events which are more specific ... like "paid", "shipped", "returned", "cancelled"? What do you think @winzou?

[winzou]

I was thinking like you:

• Keep current checkout events, there are good
• Add simple step events for order creation: like address, inventory_unit, shipment, payment. Maybe with a verb (create_*). I don't think we need pre_* and post_* here. Each event aims to add/create/update the related stuffs, and throws exception if it cannot.
• Post creation events: paid (this is where the order is actually completed), shipped, etc. Exactly what you said.

Creation events and post-creation events go in an OrderEvents class.

[pjedrzejewski]

I think we should do this event changes together with the order creation in backend. Even most basic one, this will allow us to clearly separate the processing logic from checkout itself. (because there are several other ways to create orders, in my apps I've been importing them, generating and so on... and processing it properly was a bit harder than it should be with our cleanly decoupled processors)

[winzou]

Uhm sorry I don't get your point. Once you import all cart data (order + order items + address + shipping method + payment method), you just need to dispatch each event one by one. On what rock did you fall with these decoupled processors?

[winzou]

I will stop here for this PR. I've updated the description to say what I've done.
Next steps are:

• Create backend process to add Order and adapt events
• Handle the difference between orders that are completed but not paid, and those paid.

Feedbacks welcome on this PR :)
(still this issue with Travis, but tests are OK)

[stloyd]

Maybe instead of such long checks, introduce new exception that do this in __construct() instead? I.e.:

class InvalidEventArgumentException extends \InvalidArgumentException
{
    public function __construct($listener, $passed, $expected)
    {
        $passed = is_object($passed) ? get_class($passed) : gettype($passed);

        parent::__construct(sprintf('%s listener requires event subject to be instance of "%s", %s given.', $listener, $expected, $passed));
    }
}

thrown new InvalidEventArgumentException('Order payment', $payment, 'Sylius\Bundle\PaymentsBundle\Model\PaymentInterface');

[winzou]

Will fix in another PR.
For many of them, it will be naturaly fixed by custom Event class.

[pjedrzejewski]

Yes, I'd prefer to do this with the custom event classes. I'm working on this for the resource refactoring PR.

[winzou]

Uhm regarding:

Handle the difference between orders that are completed but not paid, and those paid.

Do we want to consider an order placed but not paid as "completed"? We actually need to do some stuffs (set inventory units as sold, decrease stock, etc) even if order is not paid at the end, just to make sure we don't over use stocks, etc. But then we wait for the order to be paid before preparing shipments...

I think we should consider the status "completed" when the order is placed. But in that case we need to handle properly the end of life when order is never paid, etc. (need to release inventory units). What do you think?

[kayue]

@winzou When you say order is "completed", did you mean marking order as OrderStates::ORDER_CONFIRMED?

Just for your reference, we did the following modifications to order and inventory unit to make Sylius fit our workflow:

1. Order received, waiting for payment
   Change order status to "Pending"
   Change inventory unit state to "on-hold" for 15 minutes, on-hold unit is not available for purchase.
2. Order confirmed, payment received
   Change order status to "New"
   Change inventory unit state to "sold"

(please note order status is a new property we added to Sylius's order model)

[winzou]

@kayue thanks for your doc, it's helpful we should have done that way earlier! Note that the order status already exists in sylius, defined in Cart model in CartBundle (that CoreBundle:Order inherits at the end).

Here is a draft for Sylius: https://github.com/Sylius/Sylius/wiki/Status Please @Sylius feel free to improve it.

The completed stuff I was talking about is just the completedAt attribute of our Order model. Do we fill it when order is placed, or when payment is received?

[winzou]

@stloyd I don't agree with your change of payment status from "new" to "processing". As long as we don't have any feedback from the payment gateway, we cannot say it's "processing". "New" seems better to me, and we keep "processing" for cases where payments are not instants (check validation, status not "OK" from paypal but "waiting", etc.). What do you think?

[pjedrzejewski]

@winzou I'm in favor of "new" as well, processing could be used when we're waiting for notification from paypal etc.

[stloyd]

@winzou @pjedrzejewski Agree, missed that there is one point missing where that processing should be put, it's after Checkout finished & before Payment received, as well as for Time out after as it should be more generic like:
"Got negative feedback from payment gateway, i.e. timeout, cancellation, not enough funds etc.".

[winzou]

@stloyd I've added a "payment cancelled/failed" workflow. The idea is not to drop the cart, as the visitor may want to re-trigger a payment, or change the cart before starting a new payment.

[kayue]

@winzou Thanks for the wiki, super useful, exactly what I was looking for :)

So are we introducing new order status? I can only found the following status

const CART            = 1;
const CART_LOCKED     = 2;
const ORDER           = 3;
const ORDER_CONFIRMED = 4;

[winzou]

@kayue yes, the attribute already exists but its content needs to be completed.
@ALL: please continue the discussion on the issue I've just opened: #915
@pjedrzejewski apart from these statuses that need further discussions, ready to review and merge.

[pjedrzejewski]

@winzou I'll have a final look today for sure, thank you so much for pushing this forward.

[winzou]

@pjedrzejewski by today you meant which timezone? :D

[pjedrzejewski]

As we're not using anything ORM specific here, let's typehint ObjectManager interface.

[makasim]

agree

[winzou]

@pjedrzejewski correct, I must have been a bit lazy on this commit! Changed to ObjectManager.

[stloyd]

Please change also EventDispatcher to use EventDispatcherInterface to match other constructors.

[winzou]

@pjedrzejewski green and need to have it merged :)

[QuingKhaos]

All this cool stuff, is it going to documented anywhere?

[winzou]

I'm keeping it up to date:

• https://github.com/Sylius/Sylius/wiki/Status
• https://github.com/Sylius/Sylius/wiki/Events

But sure documentation will be at the end the reference

[pjedrzejewski]

Dammit, am I really the only one annoyed by the new UI? It looks kinda cool, but unreadable so far ... The travis 5.4 failed for random reason. I am having final look and will merge in next few minutes. (most likely)

[pjedrzejewski]

Should be OrderRepositoryInterface instead of EntityRepository.

[pjedrzejewski]

I'll change it in another PR. Let's not delay this anymore.

[pjedrzejewski]

Great work Alexandre, thank you very much. I was expecting that our whole order flow will be clarifying when we move closer to basic feature-set. I'm happy.

[jjanvier]

@pjedrzejewski no you are not the only one.. everything is too big