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

Payment Integration #48

Closed
dyjo opened this issue May 2, 2013 · 33 comments
Closed

Payment Integration #48

dyjo opened this issue May 2, 2013 · 33 comments

Comments

@dyjo
Copy link

dyjo commented May 2, 2013

Hello,

First: Thank you for the awesome library!

Sylius would be perfect in a product I have scheduled for imminent release, but I was confused about some things regarding the payment process. I'm hoping to figure out what's missing so I can bridge the gap for my project, and submit some pull requests if the code might help others.

  • Is Omnipay missing from the standard distro? The two options set in sylius/app/config/sylius.yml are 'dummy' and 'stripe', which are names of omnipay gateways. But neither the omnipay library nor sylius/omnipay-bundle is included in sylius/sylius or sylius/payments-bundle.
  • (related) Is the CreditCard From Type missing? I see the PaymentStep in the Checkout process and the PaymentMethodType in the PaymentBundle. But I can't seem to find how the actual entry of credit card information is facilitated? Is this a work in progress related to the integration of the Omnipay library?
  • What else? I've only started reading the source in the past day, so I'm sure there are things I've missed or overlooked. Not to say "hurry up and finish!", only that it would be nice to have a list of things that need to be done before using the lib in its current state. I.e. "If you plan to use Sylius in a live project, you will have to do X, Y, Z."

Again, thanks for the awesome library; I look forward to seeing what you come up with next, and hope to have an opportunity to contribute in the future!

  • dyjo
@pjedrzejewski
Copy link
Member

Hey @dyjo!

Thanks for the good words, it's really motivating! :)
As I stated in the blog post on Sylius.com - the payment gateway handling is our next step.
We're 100% sure we'll use Omnipay for this - so yeah, the integration is a work in progress - feel free to share any ideas about that - I'd be very happy to hear them and discuss.

I'd love to handle several different flows for the payment out of the box, but to keep it flexible at the same time.
Handle standard CC info, Transparent Redirect etc. but it requires some work. :)

We can perhaps rename this issue to Omnipay integration and discuss everything here, what do you think?

Thanks for jumping in!

@dyjo
Copy link
Author

dyjo commented May 10, 2013

Narrowing it down to Omnipay Integration helps. My original question was about a checklist of items that would create a working deployment, so I've tried to create one here. I've also tried to keep the descriptions as simple as possible, to prevent incorrect assumptions about the intended architecture. Just tell me if any corrections should be made, but I think maintaining this list will be useful as a roadmap for end-users or new contributors.

  • Integrate a CreditCard Entity to SyliusCoreBundle.
  • Map an Association between CreditCard and Payment.
  • Add a CreditCardType form type.
  • Add the CreditCardType to the PaymentStepType of the CheckoutProcess.
  • Add validation for the CreditCard Entity.
  • Map an Association between Order and Payment.
  • Integrate submission and capturing of payments to the PaymentStep of the CheckoutProcess.
  • Persist captured payments to the database.

I'll be posting my suggestions for these items in separate comments, and hope others do as well.

@winzou
Copy link
Contributor

winzou commented May 10, 2013

Would be great if you can help Sylius move forward on these tasks!

@winzou
Copy link
Contributor

winzou commented May 10, 2013

Why do you want to integrate a CreditCard entity to the CoreBundle? There is already one in PaymentBundle, it is even linked to the User class defined in config.

@dyjo
Copy link
Author

dyjo commented May 10, 2013

@winzou Thanks for pointing that out; I've been having trouble understanding how the DI occurs. I see that credit_card_owner is defined in the config, but I didn't see the credit_card model defined, and I didn't see a default defined in the DI Configuration.

@winzou
Copy link
Contributor

winzou commented May 10, 2013

Everything is in PaymentBundle/Entity, and the doctrine mapping in PaymentBundle/Resources/config/doctrine. There is also the relation between CreditCard and Payment.

You can tick the first 2 items ;)

@dyjo
Copy link
Author

dyjo commented May 10, 2013

@winzou I've looked at both of those, but was still confused: how do i access the credit card entity? is it available in the service container as 'sylius.model.credit_card.class'? Because I couldn't find where the default was set to Sylius\Bundle\PaymentsBundle\Entity\CreditCard.

@winzou
Copy link
Contributor

winzou commented May 10, 2013

Does anyone have already use JMSPayment bundles?

@pjedrzejewski wouldn't be a better thing to rely on JMS' bundles as this is a quite complex feature? It seems that JMS' bundles are on an advanced stage, it would save Sylius a lot of effort. Have you ever thought about that?

@dyjo
Copy link
Author

dyjo commented May 10, 2013

@winzou I found Sylius when I was looking for a more full-featured alternative to JMSPayment. The big value added for me is integration with Omnipay, so there's an abstracted method for submitting and capturing payments, and you don't have to define a new provider every time you want to use a new Gateway.

@winzou
Copy link
Contributor

winzou commented May 10, 2013

Do you think this is possible to integrate omnipay in a JMsPaymentCore plugin? It's a real question.

@dyjo
Copy link
Author

dyjo commented May 10, 2013

@winzou I'm sure it's possible, though I don't know why you would. It seems like the JMSPaymentCore GatewayPlugin duplicates a lot of what Omnipay does. At any rate, it don't know that it really relates to the topic of the issue, which is how does one bridge the gaps that exist in SyliusPayments and SyliusOmnipay

@dyjo
Copy link
Author

dyjo commented May 10, 2013

@winzou also a real question: #48 (comment). Figuring that out would be of great help for me, and hopefully flick the switch that helps me start contributing.

@dyjo
Copy link
Author

dyjo commented May 10, 2013

Ok, so my proposals for checking off the boxes on the task list. Please keep in mind this is all messy, just ideas not working code. But I want to get feedback before writing a bunch of stuff that doesn't make sense to maintainers.

Integrate a CreditCard Entity to SyliusCoreBundle

  1. Make Sylius\Bundle\PaymentsBundle\Entity\CreditCard a mapped-superclass, and extend it in the CoreBundle. This follows the same conventions as all entities defined in /Sylius/app/config/sylius.yml as model option. NOTE: may be covered per Payment Integration #48 (comment) above. I could not find where the CreditCard class had been passed into the service container.

Add a CreditCardType form type

  1. Create CreditCardType in SyliusPaymentsBundle with dataClass of CreditCard.
  2. Add validation rules for CreditCard

Map an Association between Order and Payment

  1. Add a bidirectional one-to-one to both entities in the CoreBundle.
  2. Make it nullable on Payment because it will need to be constructed and persisted before Order is built in finalize step; and not all payments necessarily come from an order. This could probably be handled better.

Map an Association between CreditCard and Payment
This one is complicated. Differentiating and defining relationships between Payment, Payment Method, Gateway, Payment Source, and Transaction is my biggest source of confusion right now. The approach outlined below allows for splitting sources of payments, e.g. in a situation where someone pays part of a bill with a credit card and part with a check, or when a bill is split amongst multiple people, like in a restaurant. It also clarifies and facilitates the most important aspect of a commerce codebase: payments!

  1. Convert PaymentSourceInterface to TransactionSourceInterface.
  2. Add a mapped $source property to Transaction.
  3. Convert PaymentMethodInterface to TransactionMethodInterface and move $method property from Payment to Transaction; if the transaction is a record of exchanged payment, then it should have knowledge of the method of exchange.
  4. Make $gateway nullable on TransactionMethod (or current PaymentMethod). Some transactions may occur outside of a gateway.
  5. So in short, Source (e.g. Credit Card) -> Transaction -> Payment

Add the CreditCardType to the PaymentStepType of the CheckoutProcess

  1. Create TransactionType in SyliusPaymentsBundle with dataClass of Transaction.
  2. Use class of the $source property of the transaction to determine what form type to add to TransactionType, e.g. the CreditCardType type created above.

Integrate submission and capturing of payments to the PaymentStep of the CheckoutProcess

  1. Create interface Sylius\Bundle\OmnipayBundle\Model\CreditCardInterface with a mapping method to convert any credit card entity to the Omnipay naming conventions.
  2. Sylius\Bundle\CoreBundle\Entity\CreditCard ... implements CreditCardInterface
  3. Add an obtain() method to TransactionMethodInterface which will be called to actually collect the exchange. Also allows for the differing use of logic between on-site and off-site gateways.
  4. In forwardAction of Sylius\Bundle\CoreBundle\Checkout\Step\PaymentStep, after validation but before forward, add logic for confirming a payment has been captured.

For example:

    $omnipayCard = $form->get('creditCard')->mapToOmnipay();
    $omnipayGateway = $this->get('sylius.omnipay.gateway' . $this->getCurrentCart()->getPaymentMethod()->getGateway());
    $transaction = $omnipayGateway->purchase($omnipayCard);

Persist Captured Payments to Database

  1. Before sending the user to the finalize step

Hopefully this is easy enough to follow, and hopefully others can help me understand the code that's already there to make this stuff happen. If any of these sounds reasonable, let me know and I'll get to work on a pull request.

@winzou
Copy link
Contributor

winzou commented May 11, 2013

Why do you want CreditCard entity to be in CoreBundle? It doesn't need any customization (a credit card is a credit card), so the entity can (and should) live in PaymentBundle. Like we do with many other entities. And it isn't available from the service container afaik.

I totally agree with all your following points. There is just one thing I want to raise: the use of credit card. In quite many cases I think end-users will choose to go through the bank interface: it means that the ecommerce website doesn't have to ask for and store the credit card information, avoiding some obligations. For us, it means that the payment process differs from one payment method to another, and we don't show the credit card form everytime.

@jjanvier
Copy link
Contributor

I totally agree with winzou on the use of credit card. Storing credit card data is very very discouraged. If Sylius wants to store these kind of information, it should follow the PCI-DSS standard. Here is some documentation :

I don't know if these requirements have been taken into consideration at the moment. But if not, we have to understand that :

  1. this is mandatory
  2. this can lead to quite a big amount of work :)

@pjedrzejewski
Copy link
Member

The actual use case for CreditCard entity is to store the name, 4 last digits and the token for gateways which support token billing. So the store does not store any sensitive data and customer can enter the info only once, and later just select his credit card from the list. There is no plan to save any cc information on our side, but of course it's up to developer what he'll do!

@dyjo
Copy link
Author

dyjo commented May 11, 2013

Re: #48 (comment)
...and other comments about where the credit card entity is made available in the core bundle. I just looked at the driver/doctrine/orm.xml file in Payments and saw that the default configuration occurs there. This solves a lot of my confusion regarding the handling of sources and credit cards. Accordingly, I've checked off the first item in the task list.

I've left the second item unchecked because I still don't see a concrete mapping between a payment and a credit card (not just association in schema, but forms and embedded forms) in the checkout flow.

@winzou re: #48 (comment).
I agree, if you look at my (admittedly long) set of suggestions, I propose abstracting the payment source so multiple sources can be supported (credit card, off-site transfer, electronic check, heck, even cash). I only mentioned credit card specifically because it is the only source available in Payments at the moment.

But, in the event a credit card is available as a payment source, persistence should definitely be available. Storing a token is very useful for recurring payments, storing the last 4 and a reference to the owner doesn't require PCI compliance, but does allow a user to select the method for a future payment. Further, someone using the bundle independently may want to store more info, and could develop PCI compliance.

@Eponymi
Copy link
Contributor

Eponymi commented May 11, 2013

Hello! I'd like to join in, plan to use Sylius for some jobs soon. I've been working on a pull request for credit card form type. I've run into a problem with where to get a list of supported brands/types. Different gateways support different brands depending on locale (https://stripe.com/us/help/faq#types-of-payments). So the question is how to get that list. Some ideas

  • a config variable set by end user (seems most flexible but most prone to error)
  • a Gateway method like getSupportedBrands(); (seems flawed because it's only for credit cards).
  • a new class, maybe SourceTypeResolver that takes a locale, a source type (cc, ACH), and a gateway and spits out a list of supported brands.

In the meantime I'm just using the constants from Omnipay Credit Card class (https://github.com/adrianmacneil/omnipay/blob/master/src/Omnipay/Common/CreditCard.php)

@winzou
Copy link
Contributor

winzou commented May 11, 2013

I think this is up to end-users, they can choose to enable or not certain types of credit cards in their country.

@pjedrzejewski
Copy link
Member

Hey guys, thanks for all the input. 👍

Just to clarify confusion about Payment -> CreditCard association - it's there https://github.com/Sylius/SyliusPaymentsBundle/blob/master/Model/Payment.php#L63.

But there are no dedicated get/setters, you can set and get it via get|setSource methods.
Also, credit card is associated with User (which needs to implement CreditCardOwnerInterface.

This may be confusing as it's hidden behind Doctrine RTEL magic. See this cookbook entry.

This is how all Sylius relations work, so if you want to change one entity class (via configuration) there is no need to remap or redefine all entities. Relation are updated automatically.

This also works for CreditCard.user association, if you configure FOSUserBundle User entity class (or any other user entity you want) it will be associated with CC. I think this is sensible default.

Yes, Order should be associated with Payment. And I agree that Payments do not always have to come from Order, so let's keep the foreign key on Order side.

The plan was to create payment via PaymentFactoryInterface service, which would take Order & PaymentMethod as argument.

The PaymentSourceInterface is handly not only for CC, I also used it for recurring payments via PayPal, ELV, BankTransfer. Where each of these had a entity similar to CreditCard. holding some data and token for recurring billing.

@dyjo Thanks for your research, I fully agree that we should be able to set method/source per transaction, not only for payment.

My idea would be to remove Transaction model completely, and use offset payments like Spree does it.
Payment.offsets holds collection of Payment, so each Payment can (but does not need to) have a 1-level-tree structure. This solves the problem of renaming PaymentMethod to TransactionMethod, as each Payment already can hold the method source. All is described here.

@dyjo What do you think about handling this like Spree? We'd get rid of Transaction model, which otherwise would become very similar to Payment itself.

@ghost
Copy link

ghost commented May 16, 2013

@pjedrzejewski Thanks for clearing all that up! The way Spree handles this looks perfect; certainly clears up the confusion about what is a transaction and what is a payment.

I'm not too familiar with Doctrine Extensions; I have worked with multi-level trees but not 1-level trees. Is there any special configuration required to limit a tree to a single level?

Additionally, curious about integrating two things you mentioned:

  1. recurring payments
  2. BankTransfer

Is there any plan to incorporate these into the payments bundle? They would be very helpful for standardizing transactions. I know that Omnipay has "off-site" gateways, so that should make using PayPal easy enough, but I haven't seen anything about Bank Transfer, ACH, etc.

@pjedrzejewski
Copy link
Member

Actually, I think we do not really need to use DoctrineExtensions (we use it for taxonomies) for this. I think that Payment.parent -> Payment association will be enough. With reverse side on Payment.offsets -> many Payments. We can simply perform a check to disallow setting Payment as parent if it is already a "offset" (or child) payment. This way we'll limit it to just one level tree.

Actually I was using Adyen payments system directly (via SyliusPaymentsBundle models), not Omnipay. And I think that Omnipay currently does not support token billing (or maybe it's in dev).

Nevertheless, we can simply contribute to Omnipay with features we need!

Forgive me guys but I need a bit of clarification who is who haha! @dyjo @Eponymi @dylanjohnson? :D
I'm not sure who should I ping. I think the Spree solution is the way to go, I'm currently focused on products refactoring and Order & Cart duplication. I'd be more than happy to help you with a PR for these change though, feel free to post any questions you may have during work on this.

@Eponymi
Copy link
Contributor

Eponymi commented May 16, 2013

Fair confusion! I'm Carl, I'm the boss :) . I just hired Dylan a couple weeks ago as a contractor, and he's now working with us full-time. So he's been using this account to make comments sometimes, but I've asked him to use his personal account (thought it was @dyjo, don't know why he has @dylanjohnson) for comments/issues now. He does all the code writing through this account. So pinging this account is probably best way to go.

So in short: any comments/issues coming from this account will be me, any code writing is Dylan.

And @dyjo @dylanjohnson pick one personal account name!

@dyjo
Copy link
Author

dyjo commented May 16, 2013

Woops. I deleted the other one. But now I'm also Ghost (#48 (comment)). Sorry about that!

@pjedrzejewski
Copy link
Member

@pjedrzejewski
Copy link
Member

@dyjo My plan is to work hard to finish the 2 important things I mentioned (product + cart&order) by the end of this week. Then I can code the Payments stuff with you if you wish, but of course you can start whenever you want and I'll be happy to help/discuss/review a PR! That'd be awesome help. 👍 Thanks for pushing this important feature forward!

@dyjo
Copy link
Author

dyjo commented May 16, 2013

@pjedrzejewski I'll have a PR up on PaymentsBundle in the morning tomorrow following the Spree model, and I'll make sure to put in all specs. Hopefully that will be good enough to merge, and if so, I'll get the stuff together for CoreBundle tomorrow afternoon.

@pjedrzejewski
Copy link
Member

@dyjo That's great but don't set so hard deadlines for yourself. :) Take your time to implement it!

@dyjo
Copy link
Author

dyjo commented May 16, 2013

Fair enough, thanks for the help!

@dyjo
Copy link
Author

dyjo commented May 17, 2013

@pjedrzejewski Any ideas on sources that don't require a form? Off-site sources like PayPal for example. I have a couple ideas but wanted to get your thoughts.

  1. on radio select a button to appears instead of a form.
  2. instead of a radio button, if a method is off-site, a button for that method is rendered.

Number one would be quite a bit easier to implement with current code base. Number two seems like a much better UI/UX, but would require a lot more work and would mean that any off-site payment method would have to add a button view, or at the very least, an URL pointing to the gateway.

@winzou
Copy link
Contributor

winzou commented May 18, 2013

I think we should do number 1 in a first iteration, and then improve UI over time.

@pjedrzejewski
Copy link
Member

@dyjo @winzou I'd vote for number 1 too, let's do smaller steps, we have a lot to do in terms of UI anyway.

@dyjo
Copy link
Author

dyjo commented May 25, 2013

Ok, I've had a lot of trouble figuring out how to implement this abstraction. I'm going to defer to others on this integration and just use some hard-coding in the PaymentStep for the time being. Hopefully someone will be able to put something together for this, I'll be looking for it!

EDIT: If anyone is interested in seeing the route I was going, you can check here: https://github.com/Eponymi/SyliusPaymentsBundle/tree/payment_refactor_branch

@dyjo
Copy link
Author

dyjo commented May 28, 2013

Closed in favor of: #112

@dyjo dyjo closed this as completed May 28, 2013
pjedrzejewski pushed a commit that referenced this issue Aug 26, 2013
Solution to Issue #48 - "Incorrect quantity when adding the same product on dev-master". 

The changes made to the cart's item collection happen on the inverse side of the relationship, according to doctrine "Changes made only to the inverse side of an association are ignored. Make sure to update both sides of a bidirectional association (or at least the owning side, from Doctrine’s point of view)". 

Solution is to explicitly persist the items in the refreshCart function. More info at http://docs.doctrine-project.org/en/latest/reference/unitofwork-associations.html
pjedrzejewski pushed a commit that referenced this issue Aug 26, 2013
Fixed transitive association persistence issue (Issue #48)
pjedrzejewski pushed a commit that referenced this issue Aug 26, 2013
load metadata event listener
pjedrzejewski pushed a commit that referenced this issue Aug 27, 2013
Company field in the address.
pjedrzejewski pushed a commit that referenced this issue Aug 29, 2013
Solution to Issue #48 - "Incorrect quantity when adding the same product on dev-master". 

The changes made to the cart's item collection happen on the inverse side of the relationship, according to doctrine "Changes made only to the inverse side of an association are ignored. Make sure to update both sides of a bidirectional association (or at least the owning side, from Doctrine’s point of view)". 

Solution is to explicitly persist the items in the refreshCart function. More info at http://docs.doctrine-project.org/en/latest/reference/unitofwork-associations.html
pjedrzejewski pushed a commit that referenced this issue Aug 29, 2013
Fixed transitive association persistence issue (Issue #48)
pjedrzejewski pushed a commit that referenced this issue Aug 29, 2013
Company field in the address.
pjedrzejewski pushed a commit that referenced this issue Aug 29, 2013
load metadata event listener
mtotheikle pushed a commit to Limelyte/Sylius that referenced this issue Jan 22, 2014
-n
Merge pull request Sylius#48 from molchanoviv/master

load metadata event listener
pamil pushed a commit to pamil/Sylius that referenced this issue Mar 21, 2016
CoderMaggie pushed a commit to CoderMaggie/Sylius that referenced this issue Jun 1, 2016
pamil pushed a commit to pamil/Sylius that referenced this issue May 7, 2019
load metadata event listener
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants