Use in-memory objects in OrderUpdater and remove Adjustment callbacks #1389

jordan-brough · 2016-08-16T15:34:34Z

My primary goal here is to make it easy to integrate 3rd party tax services
in a clean and reliable way by removing extraneous calls to
ItemAdjustments#update. (See #1252.)

Use in-memory objects in OrderUpdater:

Operate on order.line_items and order.shipments inside OrderUpdater
Instead of loading separate copies from the DB. This should give us:

Better performance
Return more accurate results. I.e. when calling order.update! the
order.line_items on that order object will get updated.

The second point is extra important as we work on removing extraneous calls
to ItemAdjustments (see below), which previously had the side effect of
updating some in-memory objects that won't otherwise get updated until
a .reload is invoked.

Remove Adjustment callbacks:

Same idea as #1356, and likewise related to #1252.

Currently we do this:

Add adjustment to item
- Trigger ItemAdjustments#update via an after_create on Adjustment
Call order.update! on items order
- Trigger ItemAdjustments#update via OrderUpdater#update_totals

We should be able to eliminate the first call to
ItemAdjustments#update because we can rely on order.update! being
called. If it weren't called then order-level amounts would be
incorrect.

Currently we do this: - Add adjustment to item - Trigger `ItemAdjustments#update` via an after_create on Adjustment - Call `order.update!` on items order - Trigger `ItemAdjustments#update` via OrderUpdater#update_totals We should be able to eliminate the first call to `ItemAdjustments#update` because we can rely on `order.update!` being called. If it weren't called then order-level amounts would be incorrect.

We now wait until OrderUpdater#update to update the in-memory items.

In real usage `order.update!` would be called after performing these actions (activating a promotion, adding tax).

Previously we did this: - Call ItemAdjustments#update on the in-memory line item - Call ItemAdjustments#update again, but *not* on the in-memory line item We've now eliminated the first call to ItemAdjustments#update, so the in-memory line item doesn't get updated.

jordan-brough · 2016-08-16T16:28:30Z

core/spec/models/spree/order_cancellations_spec.rb

        order.cancellations.short_ship([inventory_unit_2])
+        line_item.reload


This and the change above it are unfortunate. :( See the commit message for details.

Here's an idea for getting OrderUpdater to operate on in-memory objects, to try to avoid problems like the above.

diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index caff2a6..b21a70c 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -31,7 +31,15 @@ module Spree end def recalculate_adjustments - all_adjustments.includes(:adjustable).map(&:adjustable).uniq.each { |adjustable| Spree::ItemAdjustments.new(adjustable).update } + adjustables = Set.new + + # Prefer using in-memory objects when possible: + adjustables << order + adjustables.merge(order.line_items) + adjustables.merge(order.shipments) + # In case someone is attaching adjustments to something other than LineItems and Shipments: + adjustables.merge(all_adjustments.includes(:adjustable).map(&:adjustable)) + + adjustables.each do |adjustable| + Spree::ItemAdjustments.new(adjustable).update + end end

Two risks I can think of:

The in-memory objects may be stale. That might be an acceptable risk in the interest of performance.

The above code will invoke ItemAdjustments#update on all shipments & line items, even if they don't currently have any adjustments on them. That will result in an extra select * from spree_adjustments where adjustable_id = ? for every line item/shipment that doesn't have at least one adjustment (extra compared to the existing code).

Any thoughts?

I really want to do this.

I don't think we should concern ourselves with someone attaching adjustments to something other than LineItems and Shipments, since the rest of the methods in OrderUpdater won't take that into account.

@jhawthorn great point. Here's a PR (against this PR) to do that: jordan-brough#7 Shall I merge that into this PR?

Shall I merge that into this PR?

👍

cbrunsdon · 2016-08-16T16:35:38Z

I'm totally aware this might break some code as stores might not be expecting us to operate on the in-memory objects, but I'm 100% for the change. I think the order updater should always be operating on the in-memory objects and the more of these changes we make the faster and cleaner our updating process will be.

jhawthorn · 2016-08-16T17:36:10Z

My main concern with this is that the after_destroy is currently necessary. I'm worried that when destroying the last adjustment on and item, an order.update! won't successfully update the item's total since it won't be included in all_adjustments.map(&:adjustable). The change to recalculate_adjustments should fix that, but we should probably make a failing test to be sure.

So that any already-loaded line item or shipment objects will be updated. To make it more likely that code like this: order.update! order.line_items.each { ... } will have the correct data without having to issue an `order.reload` after the `order.update!`.

jhawthorn · 2016-08-16T18:47:36Z

core/spec/models/spree/order_updater_spec.rb

@@ -56,13 +56,17 @@ module Spree
      end

      it "update order adjustments" do
+        # TODO: Make this into a more valid test, so that stubbing


I'm actually encouraged that this test is breaking. It's testing an obviously wrong thing (that the adjustment totals aren't updated despite being wrong). We could probably remove it.

Great! I've gone ahead and made it simpler but more correct.

We shouldn't need to do this anymore since: - OrderUpdater now updates the in-memory LineItems and Shipments - This commit updates OrderContents#remove_from_line_item to remove destroyed line items from the order's cache.

Easier to understand what's wrong when it fails.

Don't need these reloads now that we operate on in-memory objects in OrderUpdater.

Previously the line_items association became stale in the order merger because items were being assigned to the order. By using line_items.<< we can keep the association up to date.

jhawthorn · 2016-08-16T21:29:48Z

core/spec/models/spree/order_cancellations_spec.rb

@@ -155,7 +155,7 @@
        inventory_unit_2.line_item.reload # UnitCancel#compute_amount needs updated amounts
        order.cancellations.short_ship([inventory_unit_2])
        line_item.reload
-        expect(line_item.adjustments.non_tax.sum(:amount)).to eq(-1.67)
+        expect(line_item.adjustments.map(&:amount)).to match_array([0.01, -0.83, -0.84])


[0.01, -0.83, -0.84].inject(:+) #=> -1.66

and that 0.01 is the tax adjustment above 👍

jordan-brough · 2016-08-17T13:46:12Z

FYI I'm a bit worried that I've made the spec/features/admin/orders/order_details_spec.rb flaky somehow. I haven't been able to repro these failures for that spec locally:
https://circleci.com/gh/solidusio/solidus/4156
https://circleci.com/gh/solidusio/solidus/4161
https://circleci.com/gh/solidusio/solidus/4151

mamhoff · 2016-08-17T13:46:40Z

core/spec/models/spree/order_cancellations_spec.rb

@@ -153,7 +152,7 @@
      it "generates the correct total amount" do
        order.cancellations.short_ship([inventory_unit_1])
        order.cancellations.short_ship([inventory_unit_2])
-        expect(line_item.adjustments.non_tax.sum(:amount)).to eq(-1.67)
+        expect(line_item.adjustments.map(&:amount)).to match_array([0.01, -0.83, -0.84])


This is a little strange, as the three values now add up to -1.66. Is there a tax adjustment hidden there?

Got it. Don't answer, Line 144-152 explain it all.

@mamhoff I went ahead and updated a bit because this spec was hard for me to follow when I first read it and @jhawthorn also left a comment similar to yours.

mamhoff · 2016-08-17T13:48:36Z

This is good. 👍 !

mtomov · 2016-08-18T08:18:42Z

core/app/models/spree/order_updater.rb

+      order.included_tax_total = all_items.sum(&:included_tax_total)
+      order.additional_tax_total = all_items.sum(&:additional_tax_total)
+
+      order.promo_total = all_items.sum(&:promo_total) + adjustments.promotion.eligible.sum(:amount)

      update_order_total
    end


That's all pretty great! Thank you for it!

Just wanted to ask you on why you didn't go about to change adjustments.eligible to an in-memory call as well? I think the collection has already been loaded by now somewhere in the ItemAdjustments, and here you're calling a sum twice, so it should be a two SQL lookup save. A #promotion? method already exists on Adjustment, only an #eligible? one would need be added, which could be useful.

@jhawthorn any thoughts?

👍 I really want to do this. I tried it and it caused a few errors, so I'd like to do that as a separate follow-up to this PR

jordan-brough · 2016-08-18T16:40:34Z

Note: I found that the sporadic spec failures are specific to Postgres so I'm guessing it's something to do with DB result ordering but in any case at least I can repro the failures now.

We are now triggering caching for shipments an line items when we call `order.contents.add`. This meant that `order.shipments` and `order.line_items` needed to be reloaded for these specs. Also, the `order.shipments.reload` that I had added caused flakiness because in Postgres they would sometimes reload in a random order and then `shipments.first` and `shipments.last` wouldn't be the expected items.

mtomov · 2016-08-18T19:49:18Z

core/app/models/spree/adjustment.rb

-      # Cause adjustable's total to be recalculated
-      ItemAdjustments.new(adjustable).update
-    end
-
    def require_promotion_code?
      promotion? && source.promotion.codes.any?
    end


Just wanted to point out this bad boy here:

https://github.com/solidusio/solidus/blob/master/core/app/models/concerns/spree/adjustment_source.rb#L7

It sort of relied on the "automatic" after destroy of adjustments. Should we maybe add an `order.update!' to all those incomplete orders, for which we have deleted an adjustment?

I'm thinking perhaps we should leave that for a separate PR? If it was relying on the after destroy then it was leaving the order in an inconsistent state anyway, because it didn't update the order-level totals.

Sure; it was like that indeed 👍

Added a github issue for that: #1396

jhawthorn · 2016-08-18T20:09:59Z

bbuchalter · 2016-08-23T07:11:47Z

Great work on moving this forward everyone. Thank you! ❤️

jordan-brough force-pushed the remove-adjustment-after-callbacks branch from 3aa916f to 929cb02 Compare August 16, 2016 15:36

jordan-brough added 3 commits August 16, 2016 09:37

Remove spec around updating cached items

601fecc

We now wait until OrderUpdater#update to update the in-memory items.

Add order.update! to some specs

1ed8dbe

In real usage `order.update!` would be called after performing these actions (activating a promotion, adding tax).

jordan-brough reviewed Aug 16, 2016
View reviewed changes

jordan-brough mentioned this pull request Aug 16, 2016

Update updater: Update in-memory items in #update jordan-brough/solidus#7

Merged

jordan-brough and others added 2 commits August 16, 2016 11:50

Fix spec that was using outdated line items cache

3aecbe9

Add spec for outdated adjustment totals on update

d198e84

jhawthorn reviewed Aug 16, 2016
View reviewed changes

jordan-brough and others added 4 commits August 16, 2016 14:28

Remove order.reload from OrderContents

09c5f69

We shouldn't need to do this anymore since: - OrderUpdater now updates the in-memory LineItems and Shipments - This commit updates OrderContents#remove_from_line_item to remove destroyed line items from the order's cache.

Improve order_cancellations_spec

1da229b

Easier to understand what's wrong when it fails.

Undo "Add some reloads to order_cancellations_spec.rb"

c87129d

Don't need these reloads now that we operate on in-memory objects in OrderUpdater.

OrderMerger should do a full order.update!

70dbeab

jordan-brough force-pushed the remove-adjustment-after-callbacks branch from 3169161 to 09c5f69 Compare August 16, 2016 20:33

jhawthorn added 2 commits August 16, 2016 15:14

Avoid stale line_items assoc in OrderMerger

847a33d

Previously the line_items association became stale in the order merger because items were being assigned to the order. By using line_items.<< we can keep the association up to date.

Use loaded objects in #update_adjustment_total

6e74000

jordan-brough force-pushed the remove-adjustment-after-callbacks branch from 4720879 to 847a33d Compare August 16, 2016 21:17

jhawthorn reviewed Aug 16, 2016
View reviewed changes

jhawthorn and others added 2 commits August 16, 2016 16:03

Use assoc.remove(item) in remove_line_item

b160c9b

Update OrderUpdater spec

4da7e8a

jordan-brough changed the title ~~Remove Adjustment callbacks for update_adjustable_adjustment_total~~ Use in-memory objects in OrderUpdater and remove Adjustment callbacks Aug 17, 2016

jordan-brough mentioned this pull request Aug 17, 2016

Provide a way to use third party tax services #1252

Closed

mamhoff reviewed Aug 17, 2016
View reviewed changes

Improve spec readability

cb302fb

mtomov reviewed Aug 18, 2016
View reviewed changes

jordan-brough force-pushed the remove-adjustment-after-callbacks branch from eb99086 to 0ca63c3 Compare August 18, 2016 19:48

mtomov reviewed Aug 18, 2016
View reviewed changes

jordan-brough mentioned this pull request Aug 18, 2016

Deal with deals_with_adjustments_for_deleted_source #1396

Closed

jordan-brough merged commit 7b9559d into solidusio:master Aug 18, 2016

jordan-brough deleted the remove-adjustment-after-callbacks branch August 18, 2016 20:10

jhawthorn mentioned this pull request Aug 18, 2016

Use in-memory adjustments in OrderUpdater #1397

Merged

jordan-brough mentioned this pull request Aug 20, 2016

Remove more usage of ItemAdjustments outside of OrderUpdater #1400

Merged

This was referenced Aug 24, 2016

Use in-memory objects for adjustments in ItemAdjustments #1401

Merged

Add inverse_of to order.payments #1406

Merged

jordan-brough mentioned this pull request Sep 22, 2016

Remove outdated comment in Tax::ItemAdjuster #1465

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use in-memory objects in OrderUpdater and remove Adjustment callbacks #1389

Use in-memory objects in OrderUpdater and remove Adjustment callbacks #1389

jordan-brough commented Aug 16, 2016 •

edited

Loading

jordan-brough Aug 16, 2016

jordan-brough Aug 16, 2016

jhawthorn Aug 16, 2016 •

edited

Loading

jordan-brough Aug 16, 2016

jhawthorn Aug 16, 2016

cbrunsdon commented Aug 16, 2016

jhawthorn commented Aug 16, 2016

jhawthorn Aug 16, 2016 •

edited

Loading

jordan-brough Aug 17, 2016

jhawthorn Aug 16, 2016

jordan-brough Aug 17, 2016

jordan-brough commented Aug 17, 2016

mamhoff Aug 17, 2016

mamhoff Aug 17, 2016

jordan-brough Aug 17, 2016 •

edited

Loading

mamhoff commented Aug 17, 2016

mtomov Aug 18, 2016

jordan-brough Aug 18, 2016

jhawthorn Aug 18, 2016

jordan-brough commented Aug 18, 2016

mtomov Aug 18, 2016

jordan-brough Aug 18, 2016

mtomov Aug 18, 2016

jordan-brough Aug 18, 2016

jhawthorn commented Aug 18, 2016

bbuchalter commented Aug 23, 2016

		order.cancellations.short_ship([inventory_unit_2])
		line_item.reload

Use in-memory objects in OrderUpdater and remove Adjustment callbacks #1389

Use in-memory objects in OrderUpdater and remove Adjustment callbacks #1389

Conversation

jordan-brough commented Aug 16, 2016 • edited Loading