This is important for offsite payments, where the payment might arrive after the order has been placed.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 2856586-16.patch | 5.79 KB | bojanz |
| #10 | order-paid-in-full-event-2856586.patch | 43.72 KB | jackbravo |
| #10 | order-paid-in-full-event-2856586.diff | 23.82 KB | jackbravo |
Comments
Comment #2
dom. commentedUsing transition events from state machine, would the event
commerce_order.fulfill.post_transitionbe enough to execute code when an order has been properly fully paid (from onsite or offsite payment) ? Specially since fulfill seems to be the transition state from fulfillment to completed according to commerce_order.workflows.yml
Can I use this in a 'Commerce License'-like context ?
Comment #3
mglamanI don't think
commerce_order.fulfill.post_transitionis enough. I wonder if we need something in the Payment storage handler. On saving (or post save) of a transaction, we recalculate the order's balance. If its zero then fire the event. We cannot assume thefulfillstate. We only assume draft and some sort of place event.Comment #4
vasikei'll try.
Comment #5
bojanz commentedThis is blocked on having a payment balance on the order entity, for which we have a PR up.
Comment #6
vasikeAdd the related issue #2804227: Add getTotalPaid() and getBalance() methods to orders
Comment #7
dom. commentedAny updates ?
Comment #8
jackbravo commentedI'm working on this right now. But it depends on issue [2862339](https://www.drupal.org/node/2862339#comment-12255848). That one should go first. I expect to provide a patch, based on that one in a couple of days.
Comment #9
jackbravo commentedThere is a PR now for this issue, which includes the changes for #2804227: Add getTotalPaid() and getBalance() methods to orders and #2862339: No events thrown for Payment CRUD. It is here: https://github.com/drupalcommerce/commerce/pull/792
It still needs work, it needs some tests.
Comment #10
jackbravo commentedHere is the patch with tests. It no longer depends on #2862339: No events thrown for Payment CRUD, only on #2804227: Add getTotalPaid() and getBalance() methods to orders.
The PR on github is: https://github.com/drupalcommerce/commerce/pull/804
Comment #11
Paul B commentedCan we also have an option to send the confirmation mail when the order is paid, instead of when it is placed?
Comment #13
vasikeComment #14
dom. commentedCan I have an update on this and question at #11 ? Also may I help somehow ?
Comment #15
bojanz commented#2804227: Add getTotalPaid() and getBalance() methods to orders has landed, this issue can now proceed. We need the event to be an order event, not a payment event.
#11 is out of scope for this issue, feel free to discuss it in a followup.
Comment #16
bojanz commentedInternal discussions concluded that it's fine to call it the "order paid" event, and have an $order->isPaid() helper, instead of doing "order paid in full" and $order->isPaidInFull(). Others (spree/shopify) already use just "paid", with "partially paid" used for partial payments. So we should be fine with brevity, as long as the docblocks are longer (using "fully paid" there).
Additional requirements:
- The event must only be dispatched once, matching 1.x semantics.
- We must prevent the event from firing on orders completed before Commerce 2.10.
Right now this scenario is possible:
1) Order was completed at some point.
2) Order gets its total_paid field updated (triggered by a payment update or the manual code I put in the change record)
3) The order now has a zero balance for the first time, and is considered paid. Event fires, doing something unexpected (customer gets another email, a deploy is triggered, etc).
We've also discussed whether we should move the order receipt email to the "paid" event, decided to leave it on the order placed event for now, since that matches the 1.x logic. But happy to discuss that in a followup issue.
Attached is a patch written from scratch, no tests yet.
Comment #19
bojanz commentedFixed the test fail, added test coverage.
Gave the commit credit to jackbravo since he spent so much time on this issue.
Thanks, everyone!
Comment #20
bojanz commentedUpdating credits.
Comment #21
dom. commented@bojanz about #16
Does this means we can't yet rely on this event to conclude an order is paid and finished once and for all ? If there are issues of multi-firing known, do you have a sub-issues / follow-up to send me to so I can follow it please ?
Comment #22
bojanz commented@Dom.
The point of my reroll was to eliminate the multi-firing issues, so it should be fine now.
Comment #23
dom. commented@bojanz : Thanks a lot, for the answer and all of your work !
Comment #24
dom. commentedAgain some more questions, sorry and thanks for your help !
After testing this in version 2.10: The event seems to be fired before the order is actually made : is that expected ?
- if I have a promotion that is set to "100% free off total" and a coupon called FREE for that promotion
- I have a 100€ product sent to my cart which now worth 100€ and I start proceeding with the order
- in the order process, at the stage of entering information, I have a the coupon redemption pane in the sidebar. If I enter the coupon here, the total balance goes to 0, and the OrderEvents::ORDER_PAID is fired.
But at that stage: the client has not paid He maybe just "tried" his coupon, or will change his mind (and in that case the event is not sent anymore) so it seems a too early stage to fire this event.
Maybe the condition for being paid should be both : "balance get's to 0" and "order is placed" rather than just checking balance in the isPaid() helper.
Is the firing at that stage supposed to happen ? You said at #16, that receipt could be sent on that event, but it can't for the order is not actually made here: customer can change his mind.
I have cases with an offsite-gateway that payment gets refused because 3DS fails (case 3DS is not valid: the user did not had his for with him to get the verification code in time), and yet in that case an email receipt is sent. I hoped OrderEvents::ORDER_PAID would solve my issue but maybe I do not understood properly it's usage. Could you advice on that please ?
Comment #25
bojanz commentedThat's definitely not intended. But we can't add a "order is placed" check because the regular payment done via PaymentProcess also happens before the order is placed. Instead, we should expand the isPaid() check to be "balance is 0 AND total_paid is not zero". That will resolve the problem.
Can you open a followup issue for this?
Comment #26
dom. commentedDone at #3008175: "Order paid" event is not triggered correctly for free orders