Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 17 commits into from
Aug 18, 2016
Merged

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

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
929cb02
Remove Adjustment callbacks for update_adjustable_adjustment_total
jordan-brough Aug 16, 2016
601fecc
Remove spec around updating cached items
jordan-brough Aug 16, 2016
1ed8dbe
Add `order.update!` to some specs
jordan-brough Aug 16, 2016
164971b
Add some reloads to order_cancellations_spec.rb
jordan-brough Aug 16, 2016
5e58d10
Update updater: Update in-memory items in #update
jordan-brough Aug 15, 2016
3aecbe9
Fix spec that was using outdated line items cache
jordan-brough Aug 16, 2016
d198e84
Add spec for outdated adjustment totals on update
jhawthorn Aug 16, 2016
09c5f69
Remove `order.reload` from OrderContents
jordan-brough Aug 16, 2016
1da229b
Improve order_cancellations_spec
jordan-brough Aug 16, 2016
c87129d
Undo "Add some reloads to order_cancellations_spec.rb"
jordan-brough Aug 16, 2016
70dbeab
OrderMerger should do a full order.update!
jhawthorn Aug 16, 2016
847a33d
Avoid stale line_items assoc in OrderMerger
jhawthorn Aug 16, 2016
6e74000
Use loaded objects in #update_adjustment_total
jhawthorn Aug 16, 2016
b160c9b
Use assoc.remove(item) in remove_line_item
jhawthorn Aug 16, 2016
4da7e8a
Update OrderUpdater spec
jordan-brough Aug 16, 2016
cb302fb
Improve spec readability
jordan-brough Aug 17, 2016
0ca63c3
Fix flaky specs
jordan-brough Aug 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/spec/models/spree/order_cancellations_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The in-memory objects may be stale. That might be an acceptable risk in the interest of performance.
  2. 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?

Copy link
Contributor

@jhawthorn jhawthorn Aug 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall I merge that into this PR?

👍

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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

and that 0.01 is the tax adjustment above 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jordan-brough jordan-brough Aug 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

expect(line_item.total).to eq 0
end
end
Expand Down