Return to payment step if payment failed. #817

kayue · 2014-01-11T03:12:19Z

Related to #811.

kayue · 2014-01-11T03:14:37Z

src/Sylius/Bundle/PayumBundle/Checkout/Step/PurchaseStep.php

+        );
+
+        // Complete checkout if payment success
+        if ($status->isSuccess()) {


@makasim Should we consider pending payment as a "success" payment here?

Because PayPal payment doesn't go through imminently (wait for IPN), I don't know does Payum mark those payment as success or pending.

Thank you.

I would treat pending here as success too. because if user redirected to payment step again and his first payment accepter (the one which is pending) he would end up with two payments.

makasim · 2014-01-14T07:52:34Z

Since other checkout events are all using GenericEvent, in order to be able to set Response to the event, so I guess I have to create a new event Sylius\Bundle\PayumBundle\Event\ResponseEvent that extend GenericEvent.

I think the event class has to be PurchaseCompleteEvent and has to be stored in core bundle. ResponseEvent in payum names looks weird.

Should we pass PurchaseStep ($this) as a argument to the event, so people can call $step->complete() in event listener?

I would not pass it, but maybe cannot proceed without it? I dont know.

kepeket · 2014-01-24T12:59:01Z

To add to to that, I think that the Cart should not be cleared to soon during the PurchaseStep.

See

    public function forwardAction(ProcessContextInterface $context)
    {
        $token = $this->getHttpRequestVerifier()->verify($this->getRequest());
        $this->getHttpRequestVerifier()->invalidate($token);
        $this->getCartProvider()->abandonCart();

        $status = new StatusRequest($token);
        $this->getPayum()->getPayment($token->getPaymentName())->execute($status);

the getCartProvider()->abandonCart() should only occur if the payment has been successful IMHO.

docteurklein · 2014-01-24T14:53:07Z

Like said in #811, it should be configurable.
There are so much possible flows we can't even think about, but that sylius users will have to implement :)

Vespolina, in its early stages, tried to resolve this by using a workflow framework at the core of the step process.
Currently, every user could override some parts of the steps by extending or replacing the PurchaseStep service.

There could be a more offical way by using the strategy pattern or something (similar to this finally), but simpler to hook in, and normalized.

This way, there is no hardcoded default checkout process flow, just a default strategy provided by sylius.

pjedrzejewski · 2014-01-24T16:00:48Z

@docteurklein This is how it works now, SyliusFlowBundle allows various different flows (including non-linear processes), what we are evaluating here is the default, most common checkout flow. In my projects I sometimes had 2-3 different checkout flows, built using the same steps, provided by Sylius. Of course, they have to appear in some logical order if one steps requires some information which is provided in other step.

If you look at - https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/CoreBundle/Checkout/CheckoutProcessScenario.php#L46 - It's kind of process builder (ala forms), you can construct the flow dynamically ... and so on. It's pretty flexible, not sure if we can call it workflow framework, but it is something similar. We use it for both checkout & installer.

What we could do is to make it easier to configure this, maybe we could load the steps from configuration file. Right now if you want to change the flow, you have to override this class.

kepeket · 2014-01-24T16:05:36Z

@docteurklein if it has to be configurable, the better would be to use the service tag system, to dynamically register steps.

Then we would call on each class a getPosition method, that whould return an int, somehow ordering the classes. That would be easy to insert a step into a basic workflow.

pjedrzejewski · 2014-01-24T16:10:30Z

Steps are registered via tags, just like form types so I think some kind of configuration file where you define various flows using these step names would be awesome. Instead of the requirement to create new process class.

docteurklein · 2014-01-24T16:49:52Z

oh sorry, that's indeed the kind of customisation I was talking about. Sorry for the noise.

umpirsky · 2014-01-28T18:24:40Z

@kayue @makasim @pjedrzejewski Please check this PR now. I am ready to push this forward until it is merged.

umpirsky · 2014-01-28T18:26:25Z

PurchaseStep specs are on my todo list.

stloyd · 2014-01-28T18:27:00Z

src/Sylius/Bundle/CoreBundle/EventListener/PurchaseListener.php

@@ -0,0 +1,35 @@
+<?php
+
+namespace Sylius\Bundle\CoreBundle\EventListener;


Missing license.

stloyd · 2014-01-28T18:32:04Z

Skipping those few comments it's 👍 =)

makasim · 2014-01-28T18:32:05Z

src/Sylius/Bundle/CoreBundle/EventListener/PurchaseListener.php

+    {
+        $status = $event->getArgument('status');
+
+        if ($status->isSuccess() || $status->isPending()) {


This introduce dependency on StatusRequestInterface from payum. So maybe this listener has to be moved to sylius payum bundle?

or maybe we can just pass PaymentInterface constant and you it in if statement

No, as this would add dependency on CartBundle for PayumBundle which is not needed, also CoreBundle already depends on PayumBundle so it's ok with this dependency.

Plus redirect to sylius_checkout_payment, which is core again.

then it`s ok

I disagree. Here is what I have in mind:

We add PRE_PURCHASE and POST_PURCHASE events in SyliusCheckoutEvents (not in sylius_payum_*)

We pass $payment to it (which is actually already done)

Then we keep this listener in CoreBundle and look at the status of the payment ($payment = $event->getSubject())

So that everything is clean, and we don't mix core and payum. What do you think?

makasim · 2014-01-28T18:38:17Z

also some methods missed docblocks

umpirsky · 2014-01-28T18:39:11Z

@makasim That was intentional. cc @pjedrzejewski

makasim · 2014-01-28T18:40:30Z

in general this approach looks good to me

umpirsky · 2014-01-28T18:43:19Z

Thanks guys, will fix comments, polish and fix specs...

makasim · 2014-01-28T18:45:06Z

src/Sylius/Bundle/CoreBundle/Resources/config/services.xml

+        <service id="sylius.listener.purchase" class="%sylius.listener.purchase.class%">
+            <argument type="service" id="sylius.cart_provider" />
+            <argument type="service" id="router" />
+            <tag name="kernel.event_listener" event="sylius.payum.post_purchase_step" method="onPostPurchaseStep" />


why not event subscriber? asking just out of curiosity

I inherited this code, so it was nobrainer. But as we dont change the events listener is registered for at runtime, I do not see any advantage of subscriber. Do you?

umpirsky · 2014-01-29T09:25:59Z

Fixed comments. Ready for 2nd review. Will fix specs now...

winzou · 2014-01-29T09:40:10Z

src/Sylius/Bundle/PayumBundle/SyliusCheckoutEvents.php

+
+namespace Sylius\Bundle\PayumBundle\Checkout;
+
+final class SyliusCheckoutEvents


There is already a SyliusCheckoutEvents in CoreBundle, I think you should use it instead of creating a new one. Purchase step is not specific to payum, only its implementation is. So the event (which is dispatched with payum or with anything else) should live in Core.

pjedrzejewski · 2014-01-29T10:34:05Z

I'm lost in this amount of comments and new github UI. Kinda weird. :/

umpirsky · 2014-01-29T11:41:47Z

Fixed specs, reabasing...

pjedrzejewski · 2014-01-29T11:46:08Z

Am I missing something or this will go through the finalize step again if the payment fails? I am wondering about the implications of that...

umpirsky · 2014-01-29T11:49:28Z

Rebased. Ready for final review.

@pjedrzejewski Hehum, yes, I think it will.

makasim · 2014-01-29T11:55:31Z

src/Sylius/Bundle/CoreBundle/EventListener/PurchaseListener.php

+        }
+
+        $event->setResponse(new RedirectResponse(
+            $this->router->generate('sylius_checkout_payment')


should the route name be configurable?

Maybe. @pjedrzejewski @stloyd @winzou ?

Yeah, but let's do it when I'll rework the flow bundle a bit. :)

umpirsky · 2014-01-29T12:24:08Z

@pjedrzejewski Good to be merged?

pjedrzejewski · 2014-01-29T12:38:36Z

Let's wait for travis and I want to test it myself to verify there is no harm in going through finalize step more than once. Sounds suspicious to me. :)

winzou · 2014-01-29T14:53:16Z

@pjedrzejewski if there is an issue then we should fix it, but going twice (or even more) through the finalize step must not be an issue.

pjedrzejewski · 2014-01-29T16:26:10Z

@winzou That's my point, it should be fine, but we need to check it.

umpirsky · 2014-01-30T09:29:46Z

@pjedrzejewski Piiing :)

kayue · 2014-01-30T16:33:23Z

Ping @pjedrzejewski

stloyd · 2014-01-30T19:42:19Z

@umpirsky This needs to be rebased.

pjedrzejewski · 2014-01-30T20:03:34Z

I tested it and it works fine, just please rebase it and we're good to merge.

umpirsky · 2014-01-30T21:00:40Z

@pjedrzejewski @stloyd Rebased.

pjedrzejewski · 2014-01-30T21:25:11Z

Waiting for travis.

Return to payment step if payment failed.

pjedrzejewski · 2014-01-30T21:54:00Z

Thanks guys! 👍

umpirsky · 2014-01-30T23:05:40Z

@pjedrzejewski Thanks for merging it. Please check my other PRs when you find some time.

winzou · 2014-01-31T07:54:11Z

@umpirsky @pjedrzejewski the flash message should not be set in the same method as the abandon cart, because we cannot reuse it if we don't want flashes... Can you trigger it separately? (set the method to public and add it to listen an event from the config)

umpirsky · 2014-01-31T08:32:48Z

@winzou Have sense. Will do it in separate PR. Thanks.

umpirsky · 2014-01-31T10:15:10Z

@winzou @pjedrzejewski #953

kayue reviewed Jan 11, 2014
View reviewed changes

makasim mentioned this pull request Jan 23, 2014

[PayumBundle][PaypalEC] Tax inclusion/exclusion leading to paypal error 10413 #902

Closed

stloyd reviewed Jan 28, 2014
View reviewed changes

makasim reviewed Jan 28, 2014
View reviewed changes

winzou reviewed Jan 29, 2014
View reviewed changes

makasim reviewed Jan 29, 2014
View reviewed changes

kayue and others added 4 commits January 30, 2014 21:53

Return to payment step if payment failed.

993e796

Add translation message.

7afc555

Add event.

515b548

Implemented PurchaseListener

6f2b3e3

pjedrzejewski pushed a commit that referenced this pull request Jan 30, 2014

Merge pull request #817 from kayue/feature/failure-payment-flow

b689f5d

Return to payment step if payment failed.

pjedrzejewski merged commit b689f5d into Sylius:master Jan 30, 2014

umpirsky deleted the feature/failure-payment-flow branch January 30, 2014 23:05

		@@ -0,0 +1,35 @@
		<?php

		namespace Sylius\Bundle\CoreBundle\EventListener;


		namespace Sylius\Bundle\PayumBundle\Checkout;

		final class SyliusCheckoutEvents

Return to payment step if payment failed. #817

Return to payment step if payment failed. #817

Conversation

kayue commented Jan 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makasim commented Jan 14, 2014

kepeket commented Jan 24, 2014

docteurklein commented Jan 24, 2014

pjedrzejewski commented Jan 24, 2014

kepeket commented Jan 24, 2014

pjedrzejewski commented Jan 24, 2014

docteurklein commented Jan 24, 2014

umpirsky commented Jan 28, 2014

umpirsky commented Jan 28, 2014

Choose a reason for hiding this comment

stloyd commented Jan 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makasim commented Jan 28, 2014

umpirsky commented Jan 28, 2014

makasim commented Jan 28, 2014

umpirsky commented Jan 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umpirsky commented Jan 29, 2014

Choose a reason for hiding this comment

pjedrzejewski commented Jan 29, 2014

umpirsky commented Jan 29, 2014

pjedrzejewski commented Jan 29, 2014

umpirsky commented Jan 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umpirsky commented Jan 29, 2014

pjedrzejewski commented Jan 29, 2014

winzou commented Jan 29, 2014

pjedrzejewski commented Jan 29, 2014

umpirsky commented Jan 30, 2014

kayue commented Jan 30, 2014

stloyd commented Jan 30, 2014

pjedrzejewski commented Jan 30, 2014

umpirsky commented Jan 30, 2014

pjedrzejewski commented Jan 30, 2014

pjedrzejewski commented Jan 30, 2014

umpirsky commented Jan 30, 2014

winzou commented Jan 31, 2014

umpirsky commented Jan 31, 2014

umpirsky commented Jan 31, 2014