-
Notifications
You must be signed in to change notification settings - Fork 116
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
[Woo POS] Always create a new order when checking out #15427
[Woo POS] Always create a new order when checking out #15427
Conversation
|
The repro steps don't always work. When I come back at step 11, it sometimes still shows the invalid coupons error. Is that expected at this point, and unrelated to the PR? I'm guessing that removing the coupon isn't noticed as a change to the cart, so the order isn't recalculated... but it has worked sometimes. Adding a coupon to the cart is reliably noticed as a change, but not removing it. coupon-error-remains.mp4Edit: I guess that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this – It's a nice simplification of the logic. Other than the issue mentioned about not reliably recognising invalid coupon removal, I found it worked well in my testing...
return order | ||
private extension Order { | ||
func addItems(_ cartItems: [POSCartItem]) -> Order { | ||
let itemsToAdd = Array(cartItems.createGroupedOrderSyncProductInputs().values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to call sanitizingLocalItems
on these, or is that only relevant for updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😨 I missed it.
Great observation. We do need it. As the comment in the sanitizingLocalItems
says, not applying it breaks the calculations if prices_include_tax
is true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, glad I spotted it then! I wasn't sure because it could have been something only required on update... though, our products are all added as an update, aren't they? Rather than in the initial POST request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though, our products are all added as an update, aren't they? Rather than in the initial POST request?
No. We add them as initial request when creating an order. createGroupedOrderSyncProductInputs
turns products into order items. sanitize
makes sure totals and subtotals are cleared and don't influence backend calculations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, makes sense! Yeah, I'm all for just sending up an ID and a quantity as our input. Factors in to the line/cart-level discount/taxes discussion as well.
Ideally, if we ever allow merchants to add line-level discounts like they can in IPP, we could make a change in the API that lets us specify a discount directly, and whether that's before or after tax.
Sanitizing removes order item totals and subtotals, allowing the backend to calculate it. It's critical for correct calculations when prices are inputted with taxes.
Good catch. Why it happens:
The way to get around this cleanly is to always sync the next time after an error is returned from the API. |
Closes: #15426
Discussion: woocommerce/woocommerce-android#12385
Description
Always create a new order when checking out. This matches the Android solution. Updating the order requires additional diffing logic on the mobile side and sometimes it introduces issues when remote and local orders become out-of-sync.
auto-draft
orders are removed by WordPress once 7 days..We could consider creating
checkout-draft
orders, that are removed once a day by a cron job.Steps to reproduce
The original issue I was addressing
Prerequisites: Enable
enableCouponsInPointOfSale
feature flag inDefaultFeatureFlagService
Regression
The changes affect the checkout flow, which could be triggered in two ways:
Test with various types of products:
Testing information
I tested various scenarios with different types of products, variations, and coupons. Also, cash payment and emails.
The order creation logic is not changed, I use the same method for adding order items to the order. Order editing works as order creation now.
Screenshots
RELEASE-NOTES.txt
if necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: