Multiple payments support. #1203

kayue · 2014-03-14T04:37:11Z

#834

QuingKhaos · 2014-03-14T06:16:19Z

👍

stloyd · 2014-03-14T08:27:06Z

Do we really need bidirectional relation here? IMO not (and with this change we don't need additional interface),
we should have additional method that checks the amount in payment(s) is same as total "price" of order,
IMO hasPayments() can be removed, it's just syntetic sugar we don't use anywhere,
where you would call the hasPayment()? TBH. I don't see any use case right now,

pjedrzejewski · 2014-03-14T08:35:46Z

This is how I always imagined multiple payment support.

Order can have many Payments. (as opposed to current Order.payment)
In the checkout, just like for shipments, we render a method selection form for every payment.
So, if someone customizes the payment factory (or we implement something in core in future) the user has to select a payment method for every payment. (try adding more than 1 shipment to the order right now, it will render more forms)
Of course, only "new" state payments are taken into account in the checkout payment step.
In purchase step, we loop through payments and every single payment is handled (just like we do now, just multiple times) until all payments are not "new".
If payment is failed for some reason, the customer can retry paying through external controller. (outside the checkout) and NEW payment is created for this payment, we keep the other "failed" payment for history purposes.
To check if order is paid, we simply check if successful payments total is equal to order total. (basically, if balance is zero)
I think we could use Payment with negative amount as refund. (with state refund)
I'm on the edge about bidirectional relation... For sure the Payment.order should be nullable, I'd love to have an ability to just create Payment in the backend (without order) to charge customer. (give him a link to pay etc.)

Some random thoughts about recurring payments and remembering payment methods.

I already have introduced CreditCard model. Basically, payment gateways which are capable of token billing, should save the CreditCard (PaymentSourceInterface) and attach it to the user with a token. Payment has both "method" and "source" properties, so in checkout, we'll have additional form with list of user payment sources. If he selects source instead of a method, he'll be billed automatically via token.

I can assure you that all this is possible, because I implemented it in Sylius app already. (just not the way I'd like it to be in core - quality, coupling)

Thoughts?

kayue · 2014-03-14T08:46:42Z

@stloyd

Do we really need bidirectional relation here?

Why not bidirectional? It is very useful to be able to access order payments without injecting a repository.

... with this change we don't need additional interface

Did you mean PaymentInterface? We need to add this interface because PaymentsBundle is not depending on OrderBundle.

We should have additional method that checks the amount in payment(s) is same as total "price" of order,

What do you think about this PR? #1131

IMO hasPayments() can be removed, it's just syntetic sugar we don't use anywhere,
where you would call the hasPayment()? TBH. I don't see any use case right now.

I agree it is redundant, I was just copying shipments' implementation here.
FYI We call hasPayment in addPayment, making sure it won't duplicate.

Will make the changes @pjedrzejewski suggested..

pjedrzejewski · 2014-03-14T08:48:19Z

Thanks for starting on this @kayue, you're awesome!

kayue · 2014-03-14T10:54:15Z

In purchase step, we loop through payments and every single payment is handled (just like we do now, just multiple times) until all payments are not "new".

@pjedrzejewski I am not sure is this can work. At the moment, a new payment is created every time customer visit Payment Step. This means there are multiple payment associated with order already (just missing the linkage). All of these payment state are new or pending, except the successful one.

Are we going to change this behaviour?

kayue · 2014-03-14T10:59:32Z

@pjedrzejewski Just a reminder, we cannot rely on payment state failed, because this state is not supported by all payment gateway, and even the gateway supports it, only customer who cancel payment properly will trigger the failed state. Otherwise the payment will remain as new.

winzou · 2014-03-14T11:10:16Z

failed != cancelled

pjedrzejewski · 2014-03-14T11:43:22Z

@kayue Is #823 the change we're talking about? I think payment step should create new payment only if there is no other payment with state "new". (assuming we have 1:n relation)

kayue · 2014-03-14T12:09:00Z

@pjedrzejewski Yes.

Can I do front-end in another PR because it is a lot more complex, and it is not a must have.

Multi-payment can still be useful in backend when it comes to order refund: #1202

What do you think?

Richtermeister · 2014-03-14T18:07:38Z

src/Sylius/Bundle/CoreBundle/Model/Order.php

+        if (!$this->hasPayment($payment)) {
+            $payment->setOrder($this);
+            $this->payments->add($payment);
+        }


Can you make this fluid please? Same with the next method.

@Richtermeister Fluid? what does it means?

winzou · 2014-04-14T07:15:53Z

@kayue what is the status on this? :)

kayue · 2014-04-14T07:20:41Z

@winzou Not much, there was too many changes happening in Sylius in the past few weeks, and I wasn't able to adopt them to my project yet.

Feel free to work on this if you wish.

winzou · 2014-04-14T07:25:26Z

I'm currently upgrading my sylius, and it's actually quite smooth!
Will try to have a look, but this idea of 24 hours in one day is definitely
not enough!

Alexandre Bacco

+65 8671 7859
http://alex.bacco.fr

On Mon, Apr 14, 2014 at 4:20 PM, Ka Yue Yeung notifications@github.comwrote:

@winzou https://github.com/winzou Not much, there was too many changes
happening in Sylius in the past few weeks, and I wasn't able to adopt them
to my project yet.

Feel free to work on this if you wish.

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

kayue · 2014-04-17T14:06:18Z

Let me give this a 2nd try this weekend

kayue · 2014-04-17T16:20:50Z

@pjedrzejewski Will you accept if I only deprecated the getPayment method instead of removing it?

https://github.com/kayue/Sylius/blob/c47ab403236e5b8344d276ce6d644949e9aba4a2/src/Sylius/Component/Core/Model/Order.php#L319-L333

public function getPayment()
{
    return $this->getPayments()->last();
}

Removing getPayment() method require massive amount of work and tests, we will need to revamp the entire checkout process... It is more than what I am capable of.

Can I only focus on creating 1:n order payment relationship here?

kayue · 2014-04-20T12:45:04Z

@pjedrzejewski @winzou What do you guys think?

In order to support real multiple payment like you said ("loop through payments until all payments are not "new"), we will need to tweak both PurchaseStep and PayumBundle. I think it deserve another pull request.

stloyd · 2014-04-20T12:52:49Z

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

@@ -108,7 +109,9 @@ public function voidOrderPayment(GenericEvent $event)
            );
        }

-        $order->getPayment()->setState(PaymentInterface::STATE_VOID);
+        if (false === $order->getPayments()->isEmpty()) {


Just !$order->getPayments()->isEmpty()

winzou · 2014-04-20T13:43:55Z

This is a bit weird, as the aim of this PR is just to add the ability to use multiple payments, but it does not add multiple payments. To be honest this leads to some wtf in the code. I think it's a bit risky to have this merged like this, as we won't be able to identify what is proper code and what is temporary workaround.

@kayue is it possible for you to adapt all the logic of the different actors (paymentProcessor, listeners, etc. no need to go up to the purchase step) to be able to really handle multiple payments?

winzou · 2014-04-20T13:46:03Z

To be clear: what is done is good, a nice step towards multiple payments support. Thanks for that! But there are some temporary workarounds in listeners and processors that deserve to be dealt with before merging I think :)

kayue · 2014-04-20T13:59:09Z

@winzou Sounds fair, I agree we should remove all the temporary workaround. I believe this list has everything we want to fix already: https://scrutinizer-ci.com/g/Sylius/Sylius/inspections/023d68fa-c0e7-4d63-958d-79f2d98dea26/issues/

They can all be fixed by changing Payum's SecurityToken model from Order to Payment. After that we can completely remove the getPayment() and setPayment() method in Order.

Did I missed anything here?

winzou · 2014-04-20T14:03:46Z

I was thinking of giving the payment in addition to just the order on all the listeners and processor modified in your PR. The payments->last() shouldn't be used. We should always know which payment is to be changed.

kayue · 2014-05-08T09:21:06Z

@pjedrzejewski I agree what you says, but at the current stage it is safer to just get the last "new" payment. I think there is a "bug" in Sylius where an order could have multiple "new payment" when user trying to pay with different payment method. (maybe I am using an older version).

As discussed earlier we will not implement multiple payments UI logic in this PR. We can do it in another PR.

pjedrzejewski · 2014-05-08T09:36:51Z

@kayue Deal, let's go with the getLastPayment() method for now. :)

kayue · 2014-05-09T22:50:55Z

@pjedrzejewski This PR is ready!

pjedrzejewski · 2014-05-10T08:34:43Z

@kayue Awesome work, looks good to me, but I'd love to get some feedback from @makasim. And from @winzou, because not sure which PR should go first - which will be easier to update afterwards. (state machine or this one)

winzou · 2014-05-10T14:19:58Z

Good to me as well! You can go with this one first, mine is blocked by
Finite.
Le 10 mai 2014 16:34, "Paweł Jędrzejewski" notifications@github.com a
écrit :

@kayue https://github.com/kayue Awesome work, looks good to me, but I'd
love to get some feedback from @maksim https://github.com/maksim. And
from @winzou https://github.com/winzou, because not sure which PR
should go first - which will be easier to update afterwards. (state machine
or this one)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1203#issuecomment-42736219
.

stloyd · 2014-05-10T15:25:09Z

src/Sylius/Component/Core/Model/OrderInterface.php

+     *
+     * @return PaymentInterface
+     */
+    public function getLastNewPayment();


What about: getLastPayment(PaymentInterface::STATE_NEW)?

@stloyd I don't think it will works because it is not supported by PropertyAccess in form.

Why? If you call without value, it will use default one, so it will be PaymentInterface::STATE_NEW.

makasim · 2014-05-10T16:28:33Z

Awesome work, looks good to me, but I'd love to get some feedback from @makasim.

I'd be able to review it on Monday. Dont hesitate to ping me.

stloyd · 2014-05-10T17:06:08Z

src/Sylius/Component/Payment/Model/PaymentsSubjectInterface.php

+     * Check if the payment subject has certain payment.
+     *
+     * @param PaymentInterface $payment
+     * @return Boolean


Please add new line to stop CS fixed complain about this =)

Also IIRC we use @return bool same as Symfony.

kayue · 2014-05-12T06:57:15Z

@stloyd Fixed, thanks for reviewing.
@makasim Please have a look if you have time, I don't want to break anything here. Thank you very much! :)

makasim · 2014-05-13T18:38:51Z

src/Sylius/Component/Core/Model/Payment.php

+ *
+ * @author Ka Yue Yeung <kayuey@gmail.com>
+ */
+class Payment extends BasePayment implements PaymentInterface


Just to mention. This could be named OrderAwarePayment and moved to order or payment component. I dont a big fun of dependencies on core component.

See for the referance #964

I agree, but let's do this in a separate PR! This one is big enough. :)

@pjedrzejewski sure, just mentioned it

makasim · 2014-05-13T19:07:24Z

Great job @kayue.

kayue · 2014-05-14T09:56:42Z

Thanks for review @makasim. I have fixed the comments.

@pjedrzejewski Good to merge?

Multiple payments support.

pjedrzejewski · 2014-05-15T08:24:58Z

Awesome awesome awesome... thank you Ka Yue! 👍

winzou · 2014-05-15T08:34:26Z

Great!
Let's rebase state machine PR now.

pliashkou · 2014-05-15T09:37:53Z

Good work!!

Multiple payments support.

stloyd added the Enhancement label Mar 14, 2014

stloyd added this to the 1.0.0-BETA1 milestone Mar 14, 2014

Richtermeister reviewed Mar 14, 2014
View reviewed changes

kayue changed the title ~~[WIP] Multiple payments support.~~ Multiple payments support. Apr 20, 2014

stloyd reviewed Apr 20, 2014
View reviewed changes

kayue added 2 commits May 10, 2014 06:43

Add getLastNewPayment.

53a36d8

Update PayPal INVNUM and Be2Bill ORDERID format.

3c30cbc

stloyd reviewed May 10, 2014
View reviewed changes

Fix comment.

3e51dd7

makasim reviewed May 13, 2014
View reviewed changes

Rename CaptureOrder classes to CapturePayment.

ab4bfe5

pjedrzejewski pushed a commit that referenced this pull request May 15, 2014

Merge pull request #1203 from kayue/feature/payment-order

bb64dac

Multiple payments support.

pjedrzejewski merged commit bb64dac into Sylius:master May 15, 2014

kayue deleted the feature/payment-order branch May 15, 2014 08:23

winzou mentioned this pull request May 15, 2014

Fix Be2bill multiple payment #1508

Merged

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019

Merge pull request Sylius#1203 from kayue/feature/payment-order

155de68

Multiple payments support.

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019

Merge pull request Sylius#1203 from kayue/feature/payment-order

c7d9517

Multiple payments support.

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019

Merge pull request Sylius#1203 from kayue/feature/payment-order

58519d6

Multiple payments support.

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019

Merge pull request Sylius#1203 from kayue/feature/payment-order

93e2192

Multiple payments support.

Multiple payments support. #1203

Multiple payments support. #1203

Conversation

kayue commented Mar 14, 2014

QuingKhaos commented Mar 14, 2014

stloyd commented Mar 14, 2014

pjedrzejewski commented Mar 14, 2014

kayue commented Mar 14, 2014

pjedrzejewski commented Mar 14, 2014

kayue commented Mar 14, 2014

kayue commented Mar 14, 2014

winzou commented Mar 14, 2014

pjedrzejewski commented Mar 14, 2014

kayue commented Mar 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winzou commented Apr 14, 2014

kayue commented Apr 14, 2014

winzou commented Apr 14, 2014

kayue commented Apr 17, 2014

kayue commented Apr 17, 2014

kayue commented Apr 20, 2014

Choose a reason for hiding this comment

winzou commented Apr 20, 2014

winzou commented Apr 20, 2014

kayue commented Apr 20, 2014

winzou commented Apr 20, 2014

kayue commented May 8, 2014

pjedrzejewski commented May 8, 2014

kayue commented May 9, 2014

pjedrzejewski commented May 10, 2014

winzou commented May 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makasim commented May 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kayue commented May 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makasim commented May 13, 2014

kayue commented May 14, 2014

pjedrzejewski commented May 15, 2014

winzou commented May 15, 2014

pliashkou commented May 15, 2014