[Adjustments] Reduce adjustments callbacks #7036

Matt-Yorkley · 2021-03-07T14:37:19Z

What? Why?

Removes the after_save callbacks in Spree::Adjustment that trigger expensive calls to order.update!, which in turn can trigger a number of other transactions and callbacks.

Some quick testing shows this cuts the number of transactions during checkout processing in half. Using production data and submitting a fairly simple order with 3 line items triggers over 1100 SQL hits, and order.update! appears 700 times in the backtraces for those queries/transactions. After this, the total SQL count is down to ~500. This should have a similar effect in various other parts of the app where adjustments are touched, like updating orders via admin (which is also a big pain point currently).

🎉

Note: target branch needs to be updated after #6858 is merged.

What should we test?

Order totals should be correct when editing an order or it's adjustments.

Release notes

Removed adjustment callbacks that call order.update!

Changelog Category: Technical changes

Matt-Yorkley · 2021-03-07T17:10:53Z

app/services/order_fees_handler.rb

@@ -20,6 +20,8 @@ def recreate_all_fees!
      create_line_item_fees!
      create_order_fees!
    end
+
+    order.update!


Calling order.update! here (in #recreate_all_fees!) is useful, it means we can stop manually calling order.update! in various places where we call order.recreate_all_fees!, which simplifies things.

Matt-Yorkley · 2021-03-07T17:14:11Z

app/models/spree/return_authorization.rb

@@ -95,6 +95,7 @@ def process_return
      credit.save

      order.return if inventory_units.all?(&:returned?)
+      order.update!


Return authorizations add an adjustment on the order (which was updating the order totals automatically via the callback). It needs to be called once after a return is processed, so we can just do it explicitly.

Matt-Yorkley · 2021-03-07T17:14:32Z

app/models/spree/adjustment.rb

-    def self.without_callbacks
-      skip_callback :save, :after, :update_adjustable
-      skip_callback :destroy, :after, :update_adjustable
-


Matt-Yorkley · 2021-03-07T17:15:05Z

app/controllers/cart_controller.rb

-    # during cart population, for both taxation and enterprise fees. This operation triggers a
-    # costly Spree::Order#update!, which only needs to be run once. We avoid this by disabling
-    # callbacks on Spree::Adjustment and then manually invoke Spree::Order#update! on success.
-    Spree::Adjustment.without_callbacks do


Matt-Yorkley · 2021-03-09T13:15:14Z

Not sure how indicative this is, but the build just finished in ~16 minutes total, and other builds seem to be taking ~24 minutes total...

andrewpbrett

This looks pretty reasonable, and like it will definitely help with performance. It is a little surprising that we didn't end up with more failing tests that needed to be changed. I feel like the test coverage around order totals is pretty robust though, right? So the green build should give us a good degree of confidence here.

app/controllers/cart_controller.rb

mkllnk

🔥 😄

I'm wondering if we need some more specific testing notes. The risk is that we are forgetting about cases that need to update the order.

filipefurtad0 · 2021-03-10T09:38:17Z

Note: target branch needs to be updated after #6858 is merged.

Should #6858 be tested/merged before this PR?

Matt-Yorkley · 2021-03-10T10:34:57Z

I'm wondering if we need some more specific testing notes.

Lets see... cart and checkout, then admin order edit (line items and adjustments sections), customer order edit (if the hub allows editing open orders), Bulk Order Management, any other places? I guess subs as well.

Should #6858 be tested/merged before this PR?

If we stage this PR it'll include #6858, so it should be fine. But yeah if we merge #6858 I'll update this PR to point at master.

filipefurtad0 · 2021-03-10T10:54:09Z

Ok, great - so let's stage this PR, and include the test cases from #6858 as well as those you just pointed out Matt, thanks 👍

lin-d-hop · 2021-03-10T11:50:25Z

🎉 Hooray for this one team :-D
🤞 we can get it in the next release!

filipefurtad0 · 2021-03-10T21:49:38Z

Hey @Matt-Yorkley ,

I'm splitting the testing in two comments, to reflect the separate PRs. So this first go at this concerns the changes introduced by #6858.

Test cases on shipping fees

I think this PR does not really touch much zones/shipping categories, at least directly. Anyway, I thought it's probably a good idea to have a quick test to make sure that:

Setting SM1 (Zone A) and SM2 (Zone B), customers with a billing address in Zone A can checkout (shopfront)
i) choosing SM1 - OK
ii) choosing SM2 - OK
iii) respective shipment fees apply, for test cases i) and ii) - OK
Setting an inclusive tax on the shipment fee, under /admin/tax_settings/edit and
i) placing an order -> correctly calculates inclusive tax and includes it in the price - OK

3.a) While editing orders (for SM with per order calculators)
i) it is possible to change a shipping method - OK
ii) which updates the order total, accordingly - OK

3.b) While editing orders (for SM with per item/amount calculators)
i) it is possible to change a shipping method - OK
ii) which updates the order total, accordingly - OK

CRUD operations on shipping methods - OK (other than some known issues)

This is looking pretty good - that's awesome 🎉
For the sake of keeping scope: Anything else which you feel should be tested, concerning #6858 @Matt-Yorkley ?

(To be continued, on what concerns #7036)

Matt-Yorkley · 2021-03-10T23:32:41Z

Anything else which you feel should be tested, concerning #6858 @Matt-Yorkley ?

With #6858 it's mostly just that the shipping cost should be recorded correctly and show up on the order and in the totals as usual. I think it's pretty well covered by specs as well 👍

filipefurtad0 · 2021-03-11T10:28:39Z

Ok, thanks @Matt-Yorkley 👍

Continuing with testing this PR...

Observed no significant difference in the deployment time, in staging-UK:

Master:

This PR

Checked updating order totals with taxes and fees as:

as Customer, at

cart - OK
checkout - OK
order edit -OK

as admin, while

updating shipping fee: it works, but one needs to set "Associated Adjustment Closed" as "Yes"; only after doing so, changes take effect (this was already the case before this PR)
editing an order (order edit) - OK, but (and related to the point above), the shipping fee does not update by clicking "Update and Recalculate Fees"; this needs to be done explicitly by manually changing the fee (related to the previous point, see video below)
editing an order (bulk order management) - OK
placing a subscription - OK

Comparison between backoffice and frontoffice. The mismatch after clicking "Update and Recalculate Fees" relates to the shipment fee, which needs to be updated manually - I think this the expected behavior:

I found this it was possible to update shipment fees, manually - is this the expected behavior @Matt-Yorkley? I'm not so clear on the "Associated Adjustment Closed" - is there some inception or documentation on this? (adding the feedback label)

So I think we are good here. Moving to ready to go.

PS: It seems we do observe bug #5368 (shipping fee with inclusive tax) I've scratched this from the previous comment. Although the extra tax value does not end up on the order total.

Matt-Yorkley · 2021-03-11T10:59:54Z

the shipping fee does not update by clicking "Update and Recalculate Fees"; this needs to be done explicitly by manually changing the fee

So when changing an order's contents in admin, you have to explicitly update the shipping fee if you want it to be recalculated? Sounds pretty reasonable to me. There haven't been any specific changes made to that part of the form.

It works and all the totals are correct, right?

filipefurtad0 · 2021-03-11T11:06:37Z

It works and all the totals are correct, right?

Yes, totals match 👍

I'm wondering if the fact that it needs to be done explicitly does not contribute to the shipment of orders with incorrect shipping fees, after editing line items, but agree: this does not related to this PR.

The removed test here was checking for adjustments that have an amount of zero and are eligible. If the amount is zero, it will already be marked as ineligible.

Matt-Yorkley self-assigned this Mar 7, 2021

Matt-Yorkley force-pushed the adjustments-callbacks branch 2 times, most recently from e0d9b0a to 09f731d Compare March 7, 2021 16:12

Matt-Yorkley changed the title ~~[Adjustments] Reduce callbacks~~ [Adjustments] Reduce adjustments callbacks Mar 7, 2021

Matt-Yorkley commented Mar 7, 2021

View reviewed changes

Matt-Yorkley marked this pull request as ready for review March 8, 2021 16:42

Matt-Yorkley force-pushed the adjustments-callbacks branch from 7a1db62 to 1b97e65 Compare March 9, 2021 12:37

Matt-Yorkley mentioned this pull request Mar 9, 2021

[UK] Servers using Swap, Slow Responses. Failing Orders #7058

Closed

andrewpbrett approved these changes Mar 9, 2021

View reviewed changes

app/controllers/cart_controller.rb Show resolved Hide resolved

mkllnk approved these changes Mar 9, 2021

View reviewed changes

filipefurtad0 self-assigned this Mar 10, 2021

filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Mar 10, 2021

filipefurtad0 added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-fr staging.coopcircuits.fr pr-staged-au staging.openfoodnetwork.org.au labels Mar 10, 2021

filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Mar 11, 2021

filipefurtad0 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Mar 11, 2021

filipefurtad0 removed the feedback-needed label Mar 11, 2021

Matt-Yorkley changed the base branch from adjustments-shipping-cost to master March 11, 2021 11:18

Matt-Yorkley added 5 commits March 11, 2021 11:20

Remove callbacks in Adjustment that call order.update!

4c64aae

Update totals when processing a return

9071264

Update test setup in checkout helper spec

714e4f7

Update order totals during CheckoutController#update

c5a47b5

Improve OrdersHelper spec and delete dead code

29c5703

The removed test here was checking for adjustments that have an amount of zero and are eligible. If the amount is zero, it will already be marked as ineligible.

Matt-Yorkley force-pushed the adjustments-callbacks branch from 1b97e65 to 29c5703 Compare March 11, 2021 11:22

Matt-Yorkley merged commit 120a2c4 into openfoodfoundation:master Mar 11, 2021

filipefurtad0 mentioned this pull request Jan 26, 2023

Fix When user deletes a shipping method existing orders are updated with a random shipping method #10135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Adjustments] Reduce adjustments callbacks #7036

[Adjustments] Reduce adjustments callbacks #7036

Matt-Yorkley commented Mar 7, 2021 •

edited

Loading

Matt-Yorkley Mar 7, 2021 •

edited

Loading

Matt-Yorkley Mar 7, 2021 •

edited

Loading

Matt-Yorkley Mar 7, 2021

Matt-Yorkley Mar 7, 2021

Matt-Yorkley commented Mar 9, 2021

andrewpbrett left a comment

mkllnk left a comment

filipefurtad0 commented Mar 10, 2021

Matt-Yorkley commented Mar 10, 2021

filipefurtad0 commented Mar 10, 2021 •

edited

Loading

lin-d-hop commented Mar 10, 2021

filipefurtad0 commented Mar 10, 2021 •

edited

Loading

Matt-Yorkley commented Mar 10, 2021

filipefurtad0 commented Mar 11, 2021

Matt-Yorkley commented Mar 11, 2021

filipefurtad0 commented Mar 11, 2021

[Adjustments] Reduce adjustments callbacks #7036

[Adjustments] Reduce adjustments callbacks #7036

Conversation

Matt-Yorkley commented Mar 7, 2021 • edited Loading

What? Why?

What should we test?

Release notes

Matt-Yorkley Mar 7, 2021 • edited Loading

Choose a reason for hiding this comment

Matt-Yorkley Mar 7, 2021 • edited Loading

Choose a reason for hiding this comment

Matt-Yorkley Mar 7, 2021

Choose a reason for hiding this comment

Matt-Yorkley Mar 7, 2021

Choose a reason for hiding this comment

Matt-Yorkley commented Mar 9, 2021

andrewpbrett left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Mar 10, 2021

Matt-Yorkley commented Mar 10, 2021

filipefurtad0 commented Mar 10, 2021 • edited Loading

lin-d-hop commented Mar 10, 2021

filipefurtad0 commented Mar 10, 2021 • edited Loading

Matt-Yorkley commented Mar 10, 2021

filipefurtad0 commented Mar 11, 2021

Matt-Yorkley commented Mar 11, 2021

filipefurtad0 commented Mar 11, 2021

Matt-Yorkley commented Mar 7, 2021 •

edited

Loading

Matt-Yorkley Mar 7, 2021 •

edited

Loading

Matt-Yorkley Mar 7, 2021 •

edited

Loading

filipefurtad0 commented Mar 10, 2021 •

edited

Loading

filipefurtad0 commented Mar 10, 2021 •

edited

Loading