Use Cart's needs payment check instead of Order's #4955
Conversation
senadir
added
type: bug
rubikuserbot
requested review from
a team and
nielslange
and removed request for
a team
|
@mikejolley to give this an eye and answer the question above. |
|
Size Change: 0 B Total Size: 1.1 MB ℹ️ View Unchanged
|
|
@senadir Pardon me if missing the point here, but the error in #4948 indicates that we're attempting to take a payment when no payment is required? Setting payment method blank (changed in this PR) is ok. Why do we need an extra check there? I see our usage as follows:
I don't think we should mix these up between the 2 endpoints.
Is there a bug in this function? It would only bail if |
My point around using the cart's method was because we rely on to decide if we want to capture a payment on Checkout (Payment block, Express Payment block). So having a parity between if we decide to capture payment and if we should set a payment method. Do you think all plugins who accept zeored orders would hook into both |
The checkout one could probably switch if we have an order by that point.
I don't have an answer for this but I believe you'd need both..there are flows in core when you'd want both, e.g. paying for an order from My Account. |
I tried doing that, we can do one of two:
Should they be assumed to be similar is the issue here, meaning would it always be |
|
Ok, I'm starting to understand this a bit more after seeing the diff in #4854 If we leave the cart/order checks alone and assume that cart data is synced to the order before these checks run and just focus on the change in #4854, that PR is problematic because it assumes a payment method is sent with all requests when I believe it's optional if no payment is required. To solve the original "Fix free trial subscriptions orders missing payment method meta" issue we can handle it in 2 ways:
Do you agree with that @senadir? This minimises the changes we're making our side. We should revert #4854 because |
|
2 doesn't seem like a possible solution, right? I don't think subs has access to the payment id if it's not saved on the order.
This could be possible but it might be a bit risky of a change, but we this is one possible solution, however, it only fixes it for Subs, the issue if |
|
@senadir only in the client though? Whenever the checkout endpoint is hit (so when you make payment) it syncs with cart: https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/trunk/src/StoreApi/Routes/Checkout.php#L370-L382 When else would they differ? |
It only syncs the content, so the core logic of |
If we are leaving the code as is with the change that saved payment method for all requests, then we need to remove the validation inside to: OR remove the validation inside Then we just need to ensure validation runs before the actual payment is attempted. This should satisfy both use case? |
There was a problem hiding this comment.
I checked both manual test cases against the current trunk and against your PR and your fix works as expected.
As for the points @mikejolley mentioned, I cannot say too much about this yet, so I'd leave that conversation up to you, @senadir. In case this PR remains as it is, feel free to merge it.
|
@mikejolley using Worked like a charm! |
senadir
added
the
status: blocker
senadir
force-pushed
the
fix/broken-free-orders
branch
from
4141e57 to
31c9da8
Compare
* Revert "Fix free orders missing payment method (woocommerce#4854)" This reverts commit 90e513b. * use Cart needs payment instead of Order needs payment * switch to nullish coalescing * remove extra line
* Revert "Fix free orders missing payment method (woocommerce#4854)" This reverts commit 90e513b. * use Cart needs payment instead of Order needs payment * switch to nullish coalescing * remove extra line
#4854 Removed a check to see if the order needs payment before setting the payment ID on the order. This caused a regression (#4948) In which free orders didn't pass because there are no payment method.
Reverting #4854 fixes #4948 but regresses #4853 , so this PR reverts it and fixes the original issue in both cases.
What happened?
$cart->needs_payment(). Plugins (like WC Subscriptions) hooks intoneeds_paymentand sets it to true based on conditions like trial and such. However, when setting a payment method, we check$order->needs_payment.WooCommerce Subscriptions does hook into
$order->needs_paymentbut that didn't work, because that value is false, causing the subscription recurring payment method to beManual renewaleven if a payment was still captured.This PR changes that check into
$cart->needs_paymentinstead, why?I believe, at that moment in the checkout lifecycle, using
$cart->needs_paymentmakes more sense than order given that we still didn't submit the order, and that we use that exact check to decide if we should capture payment or not, so they should be consistent.Question
There are several other instances in
Routes/Checkout.phpthat uses$order->needs_payment, should I evaluate them to see if they should be$cart->needs_paymentor not? In total, there are two other instances left.Fixes #4948
Reverts #4854
Fixes #4853
Testing
Automated Tests
Manual Testing
We're testing two issues here, #4948, and #4853
#4948
#4853
Changelog