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

[WIP] Payments #310

Open
wants to merge 3 commits into
base: 1.5
Choose a base branch
from
Open

Conversation

pjedrzejewski
Copy link
Member

Hello!

Please bear with me, as I have not coded in a while. :D

This PR is an attempt to implement Payum on the API layer. So far I've implemented an action, that will return payment instructions in JSON, so that the frontend client (VUE.js/React/whatever) can act accordingly. In current production sites that use Sylius Headless, this is still handled by hitting Sylius instance directly to the standard urls. My goal with this PR is to make it possible to hide Sylius completely, as "ecommerce.xyz.com", so that "xyz.com" can handle the payment operations (post checkout) with API.

What do I need to finish this one?

  1. Some help with how to obtain the proper redirect URL from Payum? I currently managed to get the target/after urls but these still point back to Sylius (I assume that's internal Payum thing), but I'd like to get the PayPal url, so that the frontend client can simply redirect the customer. (CC @makasim, Your help here would be priceless!)
  2. Some feedback on the idea: I'd like to maybe implement the Strategy/Adapter Pattern, to react appropriately to specific gateway types (offline vs. redirect to Hosted Payment Page vs. token authorize like Stripe, etc.). The current implementation is a bit dirty, as I handle it in the controller directly and hardcode "Offline" use-case.
  3. Regular code review. :)

@peterukena
Copy link

So this PR basically came from our request here, so I'd like to chime in with my current solution/wip.

I took this way, because Payums paypal-rest gateway does not work, not even in a bare Sylius. I dont have my evidence right now, because I was getting real frustrated with it. If needed, I will re-dive into it and provide my findings. :)

What I did is: I implemented the paypal checkout button in our client system and handled the payment capturing process (using paypal "sale" mode) entirely in the client system.
I then send the paymentID with the payment-selection endpoint to sylius, to be stored with the payment. The payment itself is an offline payum method named "paypal_checkout". This way, the order can be handled normally in sylius.
For refunds, I made a StateMachine::PRE_TRANSITION listener that checks if the transition is the right one (Payment, state:"completed", transition: "refund") which takes the paymentID and refunds the payment behind that.

The only "issue" with that is, that both systems have to know about the Paypal REST credentials, but for us that is not a big issue, since we deal with many credentials for our different shops anyway.

PS: We will probably go the same way with other "pre-paid" payments like creditcard and things like that. Capturing the payment before completing the checkout is good, because that also enables us to not even complete a checkout, if the payment went wrong.

@lchrusciel
Copy link
Member

Hello Peter, your solution seems to be not bad. And it is even better if it is working right now. The only doubt I have is, what will happen if the customer pays, but resign at complete step. The logic you have proposed should instead work with authorization flow. But it is just my generic experience. It can be totally fine in your case.

use Sylius\ShopApiPlugin\Payment\InstructionResolver;
use Sylius\ShopApiPlugin\Payment\InstructionResolverInterface;

class InstructionResolverSpec extends ObjectBehavior
Copy link
Member

Choose a reason for hiding this comment

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

Missing final keyword.

{
/**
* @var Payum
*/
Copy link
Member

Choose a reason for hiding this comment

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

As far as I see, there is a convention kept in this plugin with short docblocks, so it should be

/** @var Payum */
private $payum;

The same for the rest of properties

]
]);

return $this->viewHandler->handle($view);
Copy link
Member

Choose a reason for hiding this comment

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

There is a lot of logic in this action :( Maybe we could extract some service/services to handle it? Here we could only get the token from the request, pass it to the service that would return some kind of VO containing data required to build a view (method, type and content), wdyt?


namespace Sylius\ShopApiPlugin\Payment;

class Instruction
Copy link
Member

Choose a reason for hiding this comment

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

Missing final and docblocks in properties... And maybe it should be placed in src/Model? And have private properties set by constructor and getters? :)


use Sylius\Component\Core\Model\PaymentInterface;

class InstructionResolver implements InstructionResolverInterface
Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see, so there are some services, but not used in the action 😄 We should use them definitely

Copy link
Member

Choose a reason for hiding this comment

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

And missing final xD


class Instruction
{
public $method;
Copy link
Member

Choose a reason for hiding this comment

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

Missing typehint declarations

use Sylius\ShopApiPlugin\Payment\InstructionResolver;
use Sylius\ShopApiPlugin\Payment\InstructionResolverInterface;

class InstructionResolverSpec extends ObjectBehavior
Copy link
Member

Choose a reason for hiding this comment

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

final

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

Copy link
Member

Choose a reason for hiding this comment

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

missing strict type declaration


use Sylius\Component\Core\Model\PaymentInterface;

class InstructionResolver implements InstructionResolverInterface
Copy link
Member

Choose a reason for hiding this comment

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

Is this class used anywhere?

@peterukena
Copy link

@lchrusciel You are completely right about that. So in my case on the final step of the checkout, we replace the normal "buy" button with a "buy" button that does two things - execute the payment and complete the checkout. There is no chance to pay and not complete.
The way I use paypal is by "creating" a payment in the select-payment step and then "execute" the payment on the complete step. This is not the paypal authorization process, but paypal keeps payments valid for 3 hours after creating, so there is enough time to execute the payment in a normal checkout fashion.

If a customer waits too long, the buy button will stop with an error message and tell the customer to try again.

Its also possible to re-work this to the auth/capture flow, but in my first attempt it did not work properly and I had other things to work on at that time. The auth/capture process would be the proper way to do it :)

paypal checkout with button

@peterukena
Copy link

If we ever pick up on this one again - I can provide more details about how we finally implemented this thing.

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Jul 25, 2019

Any updates if payments get integrated or was this PR replaced with some other integration?

@tudorradubarbu
Copy link

Any news? I guess payments/payment gateways are a vital part of a headless e-commerce solution.

@alexander-schranz
Copy link
Contributor

@tudorradubarbu just want to mention a workaround here I did tried in a prototype:

If you have the shop bundle installed after calling the complete endpoint redirect them to the sylius_shop_order_pay action (/{_locale}/order/{tokenValue}/pay) then he will redirected correctly to the payment provider:

and then I did overwrite the thank you route to redirect back to my application:

sylius_shop_order_thank_you:
    path: /{_locale}/thank-you
    defaults:
        _controller: FrameworkBundle:Redirect:urlRedirect
        path: '%application_thank_you_url%'

Its definitly not the best way but it does its job.

For my case I couldn't use that as I need Paypal Plus/Paypal Rest as payment provider which is not supported by sylius or payum correctly.

@peterukena
Copy link

@alexander-schranz this totally depends on if you want your customer hit sylius for payment or not. For example, we are doing headless as well and the customer never hits our sylius for nothing. We have everything wrapped in our frontend system and make calls to our payment providers directly without using payum as well.

I think we should not build a solution in shop-api that forces users to use sylius for payments and leave it as optional as possible :)

@tudorradubarbu
Copy link

@alexander-schranz 's idea is nice in a quick-mode hack. Thank you very much.
Indeed, for a headless interface, usually, you use an offsite solution.

@kortwotze Can you please help me with a solution for this? I'm struggling with this for weeks.

@peterukena
Copy link

@tudorradubarbu what exactly do you want to achieve? If you get on the sylius dev-slack and pipe up in #support we can have a chat there :)

@jdeveloper
Copy link

We are implementing payments for android using ShopApiPlugin.The way we handle payments is the way @kortwotze mentions, calling the sylius_shop_order_pay route. The problem is, in order to work, the checkout state must change, so we call to complete checkout. So, this has the problem that if you go to paypal and cancel, it is no more possible to change payment method.

@peterukena
Copy link

@jdeveloper yeah, my approach does not really accommodate for payment changes. Paypal keeps your created payment valid for a couple of hours, so you might be able to store the data locally and use it.
After you have completed a checkout, changing payment should not work if you ask me, but if you don't execute a payment, you should be able to implement a change of payment method.

@tudorradubarbu
Copy link

C'mmon boys :) What happened with this? The ShopAPI isn't complete without Payments.

@peterukena
Copy link

Well, you can use offline payments as described above. If you need a payment integration with payum or others, I suggest getting to work on your own and share your results/insights with us, so we can built it for everyone ;)

I might have to take another look at "Payments over ShopAPI" in the near future, so this might gain some traction again.

@pierre-H
Copy link

pierre-H commented Jun 3, 2020

Hello !
Any news ?
Thanks !

@mamazu
Copy link
Member

mamazu commented Jun 3, 2020

Nope, no news. Implementing payments is probably best left to the user of the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants