[payum][paypal] add suppor of instant notifactions.

[makasim]

This address #457 RFC. I haven't able to test it in real life but pretty sure it has to work. Any one willing to test it?

TODO:

• add specs
• confirm it work in real life
• update standard edition
• add notify action to payum config.

[kayue]

@makasim I was working on another payment that also needs IPN, and I found a bug where payment token is removed by PurchasStep when cart is abandoned.

https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/PayumBundle/Checkout/Step/PurchaseStep.php#L51

This will result in following error:

A token with hash `vFsXQau62lrYw080mckKPipmM8yKCTvyb4ndrE2V8lY` could not be found.

I will test this PayPal branch soon when I complete mine, thanks!

[makasim]

That's expected Behavior. Token has to be deleted. For IPN you have to
create a notify token. Token factory provide a methode for that.

Pay attention that not all payments allow define notify url per payment.
Be2bill for example cannot, and solution for it would be a bit tricker then
for paypal
12.01.2014 8:32 ÐÏÌØÚÏ×ÁÔÅÌØ "Ka Yue Yeung" notifications@github.com
ÎÁÐÉÓÁÌ:

@makasim https://github.com/makasim I was working on another payment
that also needs IPN, and I found a bug where payment token is removed by
PurchasStep when cart is abandoned.

https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/PayumBundle/Checkout/Step/PurchaseStep.php#L51

This will result in following error:

A token with hash vFsXQau62lrYw080mckKPipmM8yKCTvyb4ndrE2V8lY could not be found.

I will test this PayPal branch soon when I complete mine, thanks!

Reply to this email directly or view it on GitHubhttps://github.com//pull/725#issuecomment-32116452
.

[kayue]

@makasim Thank you for your quick reply, I will have a look at the token factory.

In my case my payment gateway doesn't allow me to define notify url for each payment, but it does pass the token in POST request, so I overrided the default HttpRequestVerifier to read the token.

Thank you again!

[kayue]

Oh I think I knows what you mean. I can define route in the notify token. Interesting.

[makasim]

In my case my payment gateway doesn't allow me to define notify url for each payment, but it does pass the token in POST request, so I overrided the default HttpRequestVerifier to read the token.

We should avoid customizing HttpRequestVerifier. Instead I think we have to provide not secured notify url so you can handle request in the action. I have to add this logic to payum bundle.

[kayue]

@makasim Right. Maybe an action for throwing Response too, my payment gateway require me to return a Response('OK', 200).

For now I am doing everything in a custom route / controller to avoid modifying HttpRequestVerifier and return custom Response. Thanks.

[makasim]

you can do it right now. Just throw ResponseInteractiveRequest with needed response set inside your notify action.

[kayue]

@makasim You are right, forgot about interactive request.

One more thing, I think we should compare the IPN transaction amount against Sylius' order amount. Because if someone use PostRedirectUrlInteractiveRequest, people can modify the transaction amount in the invisible form with Firebug or something.

https://www.paypal.com/cgi-bin/webscr?cmd=p/acc/ipn-info-outside

[makasim]

that's not required for paypal. since we define new notify token for each new payment. User dont see as the notify url passed directly to paypal server from our. But in case of be2bill for example this THE MUST. Be2bill

If you can relly on SecuredNotifyRequest then you may not need to do additinal checks. If you can only use NotifyRequest then you must do checks for sure.

[kayue]

This branch is outdated, @makasim will you have time to update this pull request? Thanks.

[makasim]

@kayue rebased

[makasim]

@stloyd fixed

[winzou]

This should be done between the 2 dispatched events (pre and post), right?

[makasim]

actually not, The pre and post events expect to get new state from payment object where the previous one passed as an event parameter

[winzou]

I meant: what is the point of dispatching 2 events with exact same parameters? (the $order and $previousState variables don't vary)

[makasim]

here it mean 'just to be compatible". Pre event means before we actually saved anything to db, so you can change state to another one, post event means we cannot change anything in payment object.

Since the save will be done later (when we return from this action) we cannot do save between pre and post without injecting extra dependency. To keep things simple I decided to dispatch events so you can change state in post event too.

[winzou]

So maybe just dispatch the "pre" event then, to avoid any confusion
Le 15 janv. 2014 15:58, "Maksim Kotlyar" notifications@github.com a écrit
:

In src/Sylius/Bundle/PayumBundle/Payum/Paypal/Action/NotifyOrderAction.php:

• {

•    /*\* @var $request SecuredNotifyRequest */
  

•    if (!$this->supports($request)) {
  

•        throw RequestNotSupportedException::createActionNotSupported($this, $request);
  

•    }
  

•    /*\* @var OrderInterface $order */
  

•    $order = $request->getModel();
  

•    $payment = $order->getPayment();
  

•    $previousState = $payment->getState();
  

•    $this->payment->execute(new SyncRequest($order));
  

•    $status = new StatusRequest($order);
  

•    $this->payment->execute($status);
  

•    $payment->setState($status->getStatus());
  

here it mean 'just to be compatible". Pre event means before we actually
saved anything to db, so you can change state to another one, post event
means we cannot change anything in payment object.

Since the save will be done later (when we return from this action) we
cannot do save between pre and post without injecting extra dependency. To
keep things simple I decided to dispatch events so you can change state in
post event too.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/725/files#r8888660
.

[makasim]

@winzou I dont think its a good idea, One can have a listener subscribed to post event (let's say it sends an email to admin) and he would be surprised to see its not working. IMO it is better to dispatch both.

[kayue]

@makasim Testing in Sandbox mode, the notify controller returning Request SyncRequest{model: Order} is not supported. error.

[1] Payum\Core\Exception\RequestNotSupportedException: Request SyncRequest{model: Order} is not supported.
    at n/a
        in /Users/kayue/Sites/Sylius/vendor/payum/payum/src/Payum/Core/Exception/RequestNotSupportedException.php line 16

    at Payum\Core\Exception\RequestNotSupportedException::create(object(SyncRequest))
        in /Users/kayue/Sites/Sylius/vendor/payum/payum/src/Payum/Core/Payment.php line 98

    at Payum\Core\Payment->execute(object(SyncRequest))
        in /Users/kayue/Sites/Sylius/src/Sylius/Bundle/PayumBundle/Payum/Paypal/Action/NotifyOrderAction.php line 54

    at Sylius\Bundle\PayumBundle\Payum\Paypal\Action\NotifyOrderAction->execute(object(SecuredNotifyRequest))
        in /Users/kayue/Sites/Sylius/vendor/payum/payum/src/Payum/Core/Payment.php line 103

    at Payum\Core\Payment->execute(object(SecuredNotifyRequest))
        in /Users/kayue/Sites/Sylius/vendor/payum/payum/src/Payum/Core/Action/ExecuteSameRequestWithModelDetailsAction.php line 22

    at Payum\Core\Action\ExecuteSameRequestWithModelDetailsAction->execute(object(SecuredNotifyRequest))
        in /Users/kayue/Sites/Sylius/vendor/payum/payum/src/Payum/Core/Payment.php line 103

    at Payum\Core\Payment->execute(object(SecuredNotifyRequest))
        in /Users/kayue/Sites/Sylius/vendor/payum/payum-bundle/Payum/Bundle/PayumBundle/Controller/NotifyController.php line 19

    at Payum\Bundle\PayumBundle\Controller\NotifyController->doAction(object(Request))
        in  line 

    at call_user_func_array(array(object(NotifyController), 'doAction'), array(object(Request)))
        in /Users/kayue/Sites/Sylius/app/bootstrap.php.cache line 2844

[kayue]

@makasim will you have time to work on this pull request? I think I can help you with fixing the exception and test it, but I don't know how to write php specs and make it standalone. Do you must have them before merge?

Thanks!

[makasim]

@kayue have you added notify action in payum.yml?

                actions:
                    - sylius.payum.paypal.action.notify_order

[kayue]

@makasim I did.

I guess PaymentDetailsSyncAction supposed to be executed, but it doesn't support Order.

public function supports($request)
{
    if (false == $request instanceof SyncRequest) {
        return false;
    }

    $model = $request->getModel();
    if (false == $model instanceof \ArrayAccess) {
        return false;
    }

    return isset($model['PAYMENTREQUEST_0_AMT']) && null !== $model['PAYMENTREQUEST_0_AMT'];
}

Maybe we need another ExecuteSameRequestWithPaymentDetailsAction to transform Order to Payment.

public function supports($request)
{
    if (false == $request instanceof SyncRequest) {
        return false;
    }

    $model = $request->getModel();
    if (false == $model instanceof \ArrayAccess) {
        return false;
    }

    return isset($model['PAYMENTREQUEST_0_AMT']) && null !== $model['PAYMENTREQUEST_0_AMT'];
}

[makasim]

@kayue I hope I fixed SyncRequest not supported issue with last commit (formapro-forks@3b2750d)

[kayue]

@makasim Working now 👍

[makasim]

cool, I would try to add specs on coming weekend.

[makasim]

specs added, ready for final review and merge!

[kayue]

Thank you so much @makasim ! 👍

[pjedrzejewski]

@makasim Looks good to me. Awesome work. Green light from my side so when the build turns green as well, go ahead and merge it. Thank you!

[pjedrzejewski]

👍