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

[Payum] Be2bill onsite implementation #812

Merged
merged 8 commits into from
Jan 22, 2014
Merged

Conversation

winzou
Copy link
Contributor

@winzou winzou commented Jan 10, 2014

Be2bill onsite form implementation


$request->setModel($order);

//$this->httpRequest->getSession()->set('payum_token', $captureToken->getHash());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@makasim How do I get this $captureToken variable to put its hash in the session?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only difference from other payments that you have to store token hash to session and configure return\cancel urls in the be2bill admin to redirect to http://your-domain-here.dev/payment/capture/session-token

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be possible that httpRequest does not have a session. You have to check it and throw exception if session not set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I get this $captureToken variable to put its hash in the session?

@winzou since you are dealing with SecuredCaptureRequest you can get the token from it:

<?php
$request->getToken()->getHash();

@makasim
Copy link
Contributor

makasim commented Jan 10, 2014

pay attention to payum bundle misses CaptureOnsiteAction service in 0.7.0 version. I've already added to 0.7-dev but not yet released.

@winzou
Copy link
Contributor Author

winzou commented Jan 10, 2014

Hey, thanks to your feedback I fix a bit the action.
But I'm facing a RequestNotSupportedException:

[1] Payum\Core\Exception\RequestNotSupportedException: Request SecuredCaptureRequest{model: ArrayObject} is not supported.
at n/a
    in C:\wamp\www\sylius\vendor\payum\payum\src\Payum\Core\Exception\RequestNotSupportedException.php line 16

at Payum\Core\Exception\RequestNotSupportedException::create(object(SecuredCaptureRequest))
    in C:\wamp\www\sylius\vendor\payum\payum\src\Payum\Core\Payment.php line 98

at Payum\Core\Payment->execute(object(SecuredCaptureRequest))
    in C:\wamp\www\sylius\src\Sylius\Bundle\PayumBundle\Payum\Action\ExecuteSameRequestWithPaymentDetailsAction.php line 39

at Sylius\Bundle\PayumBundle\Payum\Action\ExecuteSameRequestWithPaymentDetailsAction->execute(object(SecuredCaptureRequest))
    in C:\wamp\www\sylius\vendor\payum\payum\src\Payum\Core\Payment.php line 103

at Payum\Core\Payment->execute(object(SecuredCaptureRequest))
    in C:\wamp\www\sylius\src\Sylius\Bundle\PayumBundle\Payum\Be2bill\Action\CaptureOrderViaBe2billFormAction.php line 68

at Sylius\Bundle\PayumBundle\Payum\Be2bill\Action\CaptureOrderViaBe2billFormAction->execute(object(SecuredCaptureRequest))
    in C:\wamp\www\sylius\vendor\payum\payum\src\Payum\Core\Payment.php line 103

I don't really get all the logic of these actions, so I don't know where to investigate?

And what is the CaptureOnsiteAction service you're talking about?

@makasim
Copy link
Contributor

makasim commented Jan 10, 2014

@winzou try to update payum bundle to 0.7-dev versoin

@winzou
Copy link
Contributor Author

winzou commented Jan 12, 2014

@makasim It's working with payum/payum 0.7.*@dev and payum/payum-bundle 0.7.1.
So good to merge here as soon as you release 0.7.1 of payum/payum, let us know :)
Thanks!

@ghost ghost assigned winzou Jan 12, 2014

$request->setModel($order);

$this->httpRequest->getSession()->set('payum_token', $request->getToken()->getHash());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in't this too late to set payum_token. I suppose payum->execute($request) at line 68 throws PostRedirectInteractiveRequest.

@makasim
Copy link
Contributor

makasim commented Jan 12, 2014

in general it looks really good, nice work @winzou!

@winzou
Copy link
Contributor Author

winzou commented Jan 12, 2014

@makasim Actually, you don't listen at be2bill notification, am I right?

You only use the return URL that the user will request just after its payment. I find it a bit dangerous, what if the user is badly redirected, or close its tab, or whatever? The payment is done on be2bill side, but we don't know on our side. I think we should rely on the notification URL that be2bill calls. Is it possible?

@makasim
Copy link
Contributor

makasim commented Jan 12, 2014

I think we should rely on the notification URL that be2bill calls. Is it possible?

sure it is possible, though it is harder then ipn for paypal. Because we cannot define a token per payment. Same problem was with return\canel url but it was solved by storing token to session. Here in the notifcation we can rely on ORDERID field. Fetch needed order by its value and update payment details and payment state.

@winzou
Copy link
Contributor Author

winzou commented Jan 13, 2014

@makasim so one solution is to build a controller in Payum\PayumBundle that takes the http request, reads the ORDERID and finds the matching token in the db, right? Then, how is the best way to send it back to the classic payum process?

@kayue
Copy link
Contributor

kayue commented Jan 13, 2014

@winzou I was discussing the same thing yesterday with @makasim in #725 :)

@winzou
Copy link
Contributor Author

winzou commented Jan 13, 2014

Ah right, I haven't realized. So if you need it too, we should do something generic.
Have you started to write something? Do you agree with the solution I've drafted in my previous comment?

@kayue
Copy link
Contributor

kayue commented Jan 13, 2014

I think @makasim's solution is to execute NotifyRequest after received a notification, and then we verify the HTTP request our own action instead.

https://github.com/Payum/PayumBundle/blob/master/Controller/NotifyController.php#L12-L19

@makasim
Copy link
Contributor

makasim commented Jan 13, 2014

so one solution is to build a controller in Payum\PayumBundle that takes the http request, reads the ORDERID and finds the matching token in the db, right? Then, how is the best way to send it back to the classic payum process?

payum bundle has to provide similar controller it already has but without token verification. the payment name can be taken from $request->get('payum_payment_name');

Later in the payum action we can find Order by its Id stored in ORDERID field. Having order and notification we can update payment details and dispatch state changed event if required.

@makasim
Copy link
Contributor

makasim commented Jan 13, 2014

@kayue exactly! The only problem here we have to pass paymentName. I suggest simply pass it as query parameter or as a route one.

@kayue
Copy link
Contributor

kayue commented Jan 13, 2014

@makasim Sounds good to me, thanks 👍

@makasim
Copy link
Contributor

makasim commented Jan 13, 2014

I start implementing unsafe notificatoins: Payum/PayumBundle#89.

@makasim
Copy link
Contributor

makasim commented Jan 13, 2014

just tested notifactions on sandbox, see on http://sandbox.payum.forma-dev.com/payment/notifications/list. The last unknown notification come from be2bill.

@makasim makasim mentioned this pull request Jan 14, 2014
{
protected $httpRequest;

public function setRequest(Request $request = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docblock?

@makasim
Copy link
Contributor

makasim commented Jan 15, 2014

could you add some specs?

@makasim
Copy link
Contributor

makasim commented Jan 16, 2014

PayumBundle 0.7.2 contains a notify action to catch unsafe notifications (one without token). This could be reused here to catch be2bill notifications. I tested it on sandbox, works quite well.

Pay attention that you still have to do some checks before you can trust the notifications data.

@winzou
Copy link
Contributor Author

winzou commented Jan 16, 2014

@makasim See my last commit. How do I transform the request to get it handled by the rest of the process? Which action is the next, CaptureOnsiteAction?

@winzou
Copy link
Contributor Author

winzou commented Jan 16, 2014

btw I need to return http code 200 for be2bill, so request has to be handled for our side (change payment status) + return http 200.

@makasim
Copy link
Contributor

makasim commented Jan 16, 2014

btw I need to return http code 200 for be2bill, so request has to be handled for our side (change payment status) + return http 200.

<?php

throw ResponseInteractiveRequest(new Response('', 200));

as the last line of notify action.

@makasim
Copy link
Contributor

makasim commented Jan 16, 2014

How do I transform the request to get it handled by the rest of the process? Which action is the next, CaptureOnsiteAction?

there is not standard logic to process notification, you have to put all it here:

  • get payume status $this->payment->execute(new StatusRequest($order));
  • dispatch event if state has been changed

@winzou
Copy link
Contributor Author

winzou commented Jan 16, 2014

@makasim how better is it now?


$previousState = $order->getPayment()->getState();

$status = new StatusRequest($order);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm before getting status you have to update payment details, just merge notifaction with one payment details already have, and set it back to payment

@winzou
Copy link
Contributor Author

winzou commented Jan 20, 2014

@makasim cannot update Payum with Composer:

Problem 1
  - Installation request for payum/payum-bundle ~0.7.2 -> satisfiable by payum/payum-bundle[0.7.2].
  - payum/payum-bundle 0.7.2 requires payum/core 0.8.*@dev -> no matching package found.

I don't think this 0.8.*@dev is good on the requirements for payum-bundle.

@makasim
Copy link
Contributor

makasim commented Jan 20, 2014

@winzou my bad, 0.7.3 should be fine.

@winzou
Copy link
Contributor Author

winzou commented Jan 22, 2014

All green and tested with a be2bill account! Good for review and merge @makasim @pjedrzejewski

@@ -46,6 +46,7 @@ public function createPayment(OrderInterface $order)
$payment = $this->paymentRepository->createNew();

$payment->setCurrency($order->getCurrency());
$payment->setAmount($order->getTotal());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Welcome to the pre-alpha where we forget such things. :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say it was a feature: free orders for everyone at anytime with no minimum spent 👯

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wut? I completely out of the context

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I wrote this payment processor, I forgot to set the correct amount on the payment.

@pjedrzejewski
Copy link
Member

Looks good to me! The notify action (except the API part) looks like could be abstracted / made more generic in future. Anyway, that's something we do later with next gateway.

👍 from me, I'll give final word on this to @makasim.

makasim added a commit that referenced this pull request Jan 22, 2014
[Payum] Be2bill onsite implementation
@makasim makasim merged commit a8f1b9d into Sylius:master Jan 22, 2014
@winzou winzou deleted the be2bill-onsite branch January 22, 2014 08:02
@pjedrzejewski
Copy link
Member

Thanks guys! 👍

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

Successfully merging this pull request may close these issues.

None yet

4 participants