[Checkout] Shipping fees update, remove order callback #13023

rioug · 2024-12-10T03:48:30Z

What? Why?

Related to [Checkout] Shipping fee not updated after editing order #12907, which is actually fixed by Ensure shipment gets updated when an order is updated #12954

Now that checkout workflow is restarted when an order is updated, it fixes the issue with shipping fee calculation. That said, while investigating the issue, I realised shipment fees were recalculated multiple times when completing an order , which seemed unnecessary. I tracked down the issue to a callback ( 😮 surprise! 😮 surprise! ) :

openfoodnetwork/app/models/spree/order.rb

Line 109 in ee2a6bf

before_save :update_shipping_fees!, if: :complete?

Given that we have https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/models/spree/order_contents.rb that should be used to managed adding/updating/removing an order's line item. Spree::OrderContents already handle updating shipment and shipment fees, so now the before_save callback becomes redundant.

Drawback : shipment fees won't be updated is someone directly update a line item on an order. There isn't anyway to prevent that (as far as I know), we (as the dev team) need to enforce the use Spree::OrderContents for any line item update.

What should we test?

As an enterprise manager:

Create a shipping method with a fee using the flat percent calculator.

As a customer:

start an order with that shipping method and proceed to step three (order summary) of the checkout but don't complete the order.
Take note of the order total and the shipping fee.
Continue shopping, i.e. click on the shop's name next to the cart icon in the top right corner.
Add one or more products to your cart.
Click checkout again - you should land on step 1, continue checking out using the shipping method set up earlier
Take note of the order total and the shipping fee - both should be updated correctly, including your added products.
Complete the order.
Compare the numbers of order total and shipping fee again.
--> they should be matching.

Same scenario as above but this time once on step 3 of checkout, use "Edit" link next to "Order Details" to update your order

As an enterprise manager

Pick a completed order in the backoffice
Update a line item quantity or add a new variant
Check the shipping fees have been updated

As an enterprise manager, in the backoffice

Create a new order
Choose the shipping method with a flat percent calculator created a the start
--> check the shipping fee is calculated correctly
-- Add or update a line item in the order
--> check the shipping fee is updated correctly

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

#12954 needs to be merged first

mkllnk

Cool. That should speed up the app.

mkllnk · 2024-12-10T21:58:54Z

spec/system/admin/orders_spec.rb

+          order2.contents.update_item(Spree::LineItem.where(order_id: order2.id).first,
+                                      { quantity: 5 })


Suggested change

order2.contents.update_item(Spree::LineItem.where(order_id: order2.id).first,

{ quantity: 5 })

order2.contents.update_item(Spree::LineItem.find_by(order: order2), { quantity: 5 })

dacook

Looks good!

It seems more reasonable to be enforcing this at the controller level 👍

spec/models/spree/order_spec.rb

It should be handled in the controller, it's currently handled in `Spree::OrderContents#remove`. As long as we don't manually remove line item from an order we should be good. Currently it gets trigerred each time the order is saved, which seems to happen mutiple time when we finalize an order. It's a bit useless to recalculated the fees over and over Context, it was added here : openfoodfoundation@217eda8

And update related specs

User Order::Contents#update_item to update line item on an order, it ensures the order is properly updated

filipefurtad0 · 2025-01-16T00:00:57Z

Hey @rioug ,

Thanks for outlining the test scenarios, with the focus on the shipping method fees. I went through them, and if a cash payment method is used, I can confirm that adding items after reaching the /summary page correctly updates the shipping fee values.

However, the PR introduces a regression with Stripe, which only occurs, if the shipping method has a shipping fee.

Below, when the order is setup using a shipping method with:

no fee
a flat percent calculator.

We can see, the payment total is twice the value of the order total, which results in a credit owed (order-)payment state.

The same scenarios 1) and 2) in current master:

The Stripe payment total equals the order total, so the (order-)payment state is paid.

In the tests above, the payment method was set with a transaction fee. However, this does not seem to be relevant in this case: the issue is observable as well, when no transaction fee is set:

What do you think is causing this? It might be useful to note that this only occurs on checkout, and not for orders placed in the backoffice. Please let me know if you feel you need more tests to debug the issue.
Moving back to In progress for now.

rioug · 2025-01-20T00:57:30Z

Thanks @filipefurtad0 I managed to reproduce the issue on my machine. I think it might be due to "stripe payment intent" not being updated appropriately when we update the order. I'll have a look at it.

For payment that complete during the checkout (Paypal, Stripe) the amount was recorded twice against `order.payment_total`. This is because the `payment_total` gets updated in an afer_save Payment callback when a payment is completed, and then once more when we process payment in `Spree::Order#process_each_payment`. This is an existing issue on master, but it was hidden by the `update_shipping_fees!` callback, it trigerred an update of the order's total, which then updated `order.payment_total` with the correct value. Now that we removed the callback, the bug showed up. Note, I updated the stripe specs for consistency even though they are currently disabled.

Use a completed payment, instead of trying to complete a payment

Payment needs to be linked to the order, in order for the payment callback to update `order.payment_total`

rioug · 2025-01-21T00:17:27Z

@openfoodfoundation/reviewers Bug fix start at : c71ae35

mkllnk · 2025-01-21T00:43:00Z

app/models/spree/order.rb

-
-        if payment.completed?
-          self.payment_total += payment.amount
-        end


It's a bit weird to take a payment without updating the total. But I guess that we are just moving the responsibility of updating the total out of the callback. And at some point, we may not want payments in a callback either.

It was cause for me to look it up to make sure I understood. I found that four controllers update the order.payment_total (via OrderManagement::Order::Updater#update_totals_and_states):
checkout_controller.rb, voucher_adjustments_controller.rb, api/v0/shipments_controller.rb, admin/adjustments_controller.rb

It is updated by counting up all payments. That seems like a much safer method than the incrementing above.

https://github.com/openfoodfoundation/openfoodnetwork/blob/master/engines/order_management/app/services/order_management/order/updater.rb#L60

order.payment_total = payments.completed.sum(:amount)

Very good!

The issue was actually caused by this :

openfoodnetwork/app/models/spree/payment.rb

Lines 235 to 237 in 31c7173

def update_order

OrderManagement::Order::Updater.new(order).after_payment_update(self)

end

which is triggered by an after_save callback.
So by the time the code reach these deleted lines the payment was already added to order.payment_total. It was hidden by the fact OrderManagement::Order::Updater#update_totals_and_states was triggered further down the line, which now does not always happen.

I see. I can see that this triggers the same code (order.payment_total = payments.completed.sum(:amount))

If we can be sure that update_totals_and_states is always triggered down the line (by a controller), then perhaps we can remove this model callback also! But I'm afraid to suggest it in this PR..

I think that would need more investigation. I believe the payment callback is there to ensure the order.payment_total is updated when a payment is completed outside the checkout. Ie, capturing a cash payment in the backoffice. There might be other scenarios as well, It's not something I would want to do here.

dacook

Great, I have a suggestion to potentially simplify the spec, but it's fine either way.

dacook · 2025-01-21T01:22:04Z

spec/system/consumer/shopping/checkout_paypal_spec.rb

@@ -69,6 +69,7 @@

        click_on "Complete order"
        expect(page).to have_content "Your order has been processed successfully"
+        expect(page.find("#amount-paid").text).to have_content "$19.99"


Instead of using an ID, can we test for the associated label instead? (also below)

Suggested change

expect(page.find("#amount-paid").text).to have_content "$19.99"

expect(page).to have_content "Amount Paid\n$19.99"

dacook · 2025-01-21T01:23:41Z

spec/models/spree/order_spec.rb

    let(:payment) { build(:payment) }
    before { allow(order).to receive_messages pending_payments: [payment], total: 10 }

-    it "should return false if no pending_payments available" do
+    it "returns false if no pending_payments available" do


Thanks for tidying these 👍

dacook · 2025-01-21T01:38:24Z

app/models/spree/order.rb

-
-        if payment.completed?
-          self.payment_total += payment.amount
-        end


It was cause for me to look it up to make sure I understood. I found that four controllers update the order.payment_total (via OrderManagement::Order::Updater#update_totals_and_states):
checkout_controller.rb, voucher_adjustments_controller.rb, api/v0/shipments_controller.rb, admin/adjustments_controller.rb

It is updated by counting up all payments. That seems like a much safer method than the incrementing above.

https://github.com/openfoodfoundation/openfoodnetwork/blob/master/engines/order_management/app/services/order_management/order/updater.rb#L60

order.payment_total = payments.completed.sum(:amount)

Very good!

filipefurtad0 · 2025-01-24T00:50:19Z

Hey @rioug,

Nice, the issue with the payment is fixed now 🎉
Going though the test cases you've outlined:

As a customer:

Having an order on step/state summary and updating it:
i) clicking on the shop's name next to the cart icon in the top right corner
ii) using the "Edit" link next to "Order Details"
iii) editing the cart (clicking top right corner)

resulted in correct update of the shipping fee, which included added or removed items.

As an example, with a 10% rate on shipping, changing the line item quantity from 2:

to 4:

As an enterprise manager

i) editing line items quantity
ii) creating an order

resulted in correct update of the shipping fee, which included added or removed items.

Same example as above, with a 10% rate on shipping, changing the line item quantity from 2:

to 4:

Looks good, I think we're ready to merge this one. Thanks again 💪

github-actions bot assigned rioug Dec 10, 2024

rioug marked this pull request as ready for review December 10, 2024 04:27

rioug added the user facing changes Thes pull requests affect the user experience label Dec 10, 2024

rioug changed the title ~~12907 fix checkout shipping fee~~ [Checkout] Shipping fees update, remove order callback Dec 10, 2024

mkllnk approved these changes Dec 10, 2024

View reviewed changes

dacook approved these changes Dec 16, 2024

View reviewed changes

spec/models/spree/order_spec.rb Outdated Show resolved Hide resolved

drummer83 force-pushed the 12907-fix-checkout-shipping-fee branch from d7216ae to 14e18f7 Compare January 9, 2025 19:15

rioug added 5 commits January 15, 2025 15:44

Add spec for #update_shipping_fees!

f9bd720

And update related specs

Update spec to properly test shipping fee update

9e7e40a

Update spec to properly update line items on an order

a8d1d0c

User Order::Contents#update_item to update line item on an order, it ensures the order is properly updated

Per review, small improvment

1afa7fe

filipefurtad0 force-pushed the 12907-fix-checkout-shipping-fee branch from 14e18f7 to 1afa7fe Compare January 15, 2025 21:44

filipefurtad0 self-assigned this Jan 15, 2025

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Jan 15, 2025

rioug added 5 commits January 20, 2025 16:07

Small linting fix

17e7f7d

Per review, specify the actual expected value

d7ae91c

Fix spec

0ca164a

Use a completed payment, instead of trying to complete a payment

Fix spec

fcd366c

Payment needs to be linked to the order, in order for the payment callback to update `order.payment_total`

mkllnk approved these changes Jan 21, 2025

View reviewed changes

mkllnk reviewed Jan 21, 2025

View reviewed changes

dacook approved these changes Jan 21, 2025

View reviewed changes

filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Jan 23, 2025

filipefurtad0 merged commit 5d36073 into openfoodfoundation:master Jan 24, 2025
53 checks passed

filipefurtad0 added technical changes only These pull requests do not contain user facing changes and are grouped in release notes and removed pr-staged-fr staging.coopcircuits.fr user facing changes Thes pull requests affect the user experience labels Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Checkout] Shipping fees update, remove order callback #13023

[Checkout] Shipping fees update, remove order callback #13023

rioug commented Dec 10, 2024 •

edited

Loading

mkllnk left a comment

mkllnk Dec 10, 2024

dacook left a comment

filipefurtad0 commented Jan 16, 2025 •

edited

Loading

rioug commented Jan 20, 2025

rioug commented Jan 21, 2025

mkllnk Jan 21, 2025

dacook Jan 21, 2025

rioug Jan 21, 2025 •

edited

Loading

dacook Jan 21, 2025

rioug Jan 21, 2025

dacook left a comment

dacook Jan 21, 2025

dacook Jan 21, 2025

dacook Jan 21, 2025

filipefurtad0 commented Jan 24, 2025

		order2.contents.update_item(Spree::LineItem.where(order_id: order2.id).first,
		{ quantity: 5 })

	def update_order
	OrderManagement::Order::Updater.new(order).after_payment_update(self)
	end

	expect(page.find("#amount-paid").text).to have_content "$19.99"
	expect(page).to have_content "Amount Paid\n$19.99"

[Checkout] Shipping fees update, remove order callback #13023

[Checkout] Shipping fees update, remove order callback #13023

Conversation

rioug commented Dec 10, 2024 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Jan 16, 2025 • edited Loading

rioug commented Jan 20, 2025

rioug commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rioug Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 commented Jan 24, 2025

rioug commented Dec 10, 2024 •

edited

Loading

filipefurtad0 commented Jan 16, 2025 •

edited

Loading

rioug Jan 21, 2025 •

edited

Loading