From 1e60956955b784829a989fee051e138297422237 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Tue, 23 Aug 2016 21:31:09 -0600 Subject: [PATCH 01/17] Automatically update `adjustments` relationships So that in-memory associations stay up to date for calculations in OrderUpdater and elsewhere. E.g. if you do this: Spree::Adjustment.create!(adjustable: line_item, ...) Then `line_item.adjustments` does not get updated if it's already loaded. Also this: source.adjustments.create!(adjustable: line_item, ...) likewise doesn't update `line_item.adjustments`. The code in this commit is not the most lovely code, but I couldn't think of a better option for the time being, given that we expose all of ActiveRecord as an interface and it's likely that extensions and/or stores themselves have code like the above in, for example, their promotion actions. --- core/app/models/spree/adjustment.rb | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/core/app/models/spree/adjustment.rb b/core/app/models/spree/adjustment.rb index 12d9ab54237..e8e061bc5c3 100644 --- a/core/app/models/spree/adjustment.rb +++ b/core/app/models/spree/adjustment.rb @@ -26,6 +26,11 @@ class Adjustment < Spree::Base validates :amount, numericality: true validates :promotion_code, presence: true, if: :require_promotion_code? + # We need to use `after_commit` here because otherwise it's too early to + # tell if any repair is needed. + after_commit :repair_adjustments_associations_on_create, on: [:create] + after_commit :repair_adjustments_associations_on_destroy, on: [:destroy] + scope :not_finalized, -> { where(finalized: false) } scope :open, -> do Spree::Deprecation.warn "Adjustment.open is deprecated. Instead use Adjustment.not_finalized", caller @@ -177,5 +182,21 @@ def update!(target = nil) def require_promotion_code? promotion? && source.promotion.codes.any? end + + def repair_adjustments_associations_on_create + if adjustable.adjustments.loaded? && !adjustable.adjustments.include?(self) + # Note: I will update this to a deprecation once I've gotten rid of all the occurrences in Solidus + puts("Adjustment was not added to #{adjustable.class}. Add adjustments via `adjustable.adjustments.create!`. Partial call stack: #{caller.select { |line| line =~ %r(/(app|spec)/) }}.", caller) + adjustable.adjustments.proxy_association.add_to_target(self) + end + end + + def repair_adjustments_associations_on_destroy + if adjustable.adjustments.loaded? && adjustable.adjustments.include?(self) + # Note: I will update this to a deprecation once I've gotten rid of all the occurrences in Solidus + puts("Adjustment was not removed from #{adjustable.class}. Remove adjustments via `adjustable.adjustments.destroy`. Partial call stack: #{caller.select { |line| line =~ %r(/(app|spec)/) }}.", caller) + adjustable.adjustments.proxy_association.target.delete(self) + end + end end end From dd22bd0874b142fe6ed2af8358b1c229c72f0caf Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:34:11 -0600 Subject: [PATCH 02/17] Don't use all_adjustments in OrderAdjuster Using all_adjustments bypasses the individual `adjustments` associations (order.adjustments, line_item.adjustments & shipment.adjustments) which makes them outdated for further use. --- core/app/models/spree/tax/order_adjuster.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/tax/order_adjuster.rb b/core/app/models/spree/tax/order_adjuster.rb index 9df6a89a408..c2b2d4ab210 100644 --- a/core/app/models/spree/tax/order_adjuster.rb +++ b/core/app/models/spree/tax/order_adjuster.rb @@ -16,7 +16,9 @@ def initialize(order) def adjust! return unless order_tax_zone(order) - order.all_adjustments.tax.destroy_all + [order, *order.line_items, *order.shipments].each do |item| + item.adjustments.tax.destroy_all + end (order.line_items + order.shipments).each do |item| ItemAdjuster.new(item, order_wide_options).adjust! From df3135bd70b0069db5daa9250f4901750d0ecd70 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:34:32 -0600 Subject: [PATCH 03/17] Update spec to use line_item.inventory_units Using order.inventory_units bypasses `line_item.adjustments` which makes them outdated for further use. --- core/spec/lib/tasks/order_capturing_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/spec/lib/tasks/order_capturing_spec.rb b/core/spec/lib/tasks/order_capturing_spec.rb index 5642d6cc3f0..a650bcc062e 100644 --- a/core/spec/lib/tasks/order_capturing_spec.rb +++ b/core/spec/lib/tasks/order_capturing_spec.rb @@ -18,7 +18,7 @@ context "with a mix of canceled and shipped inventory" do before do - Spree::OrderCancellations.new(order).short_ship([order.inventory_units.first]) + Spree::OrderCancellations.new(order).short_ship([order.line_items.first.inventory_units.first]) order.shipping.ship_shipment(order.shipments.first) order.update_attributes!(payment_state: 'balance_due') end From 57a12341a2812010f68cf7b3d348749432af50c9 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:34:41 -0600 Subject: [PATCH 04/17] Fix spec to use correct in-memory object This ensures that the same object is used as the one inside `item.adjustments` so that it's kept up to date correctly --- core/spec/models/spree/item_adjustments_spec.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/spec/models/spree/item_adjustments_spec.rb b/core/spec/models/spree/item_adjustments_spec.rb index edab4b1ae5c..933b859825a 100644 --- a/core/spec/models/spree/item_adjustments_spec.rb +++ b/core/spec/models/spree/item_adjustments_spec.rb @@ -272,6 +272,9 @@ def create_adjustment(label, amount) context "multiple updates" do let(:adjustment) { create(:tax_adjustment, amount: -10) } let(:item) { adjustment.adjustable } + # we need to get this from the line item so that we're modifying the same + # tax rate that is cached by line_item.adjustments + let(:source) { item.adjustments.to_a.first.source } def update described_class.new(item).update @@ -283,18 +286,17 @@ def db_record end it "persists each change" do - adjustment.source.update_attributes!(amount: 0.1) + source.update_attributes!(amount: 0.1) update expect(item).not_to be_changed expect(db_record).to have_attributes(adjustment_total: 1) - adjustment.source.update_attributes!(amount: 0.20) - item.reload + source.update_attributes!(amount: 0.20) update expect(item).not_to be_changed expect(db_record).to have_attributes(adjustment_total: 2) - adjustment.source.update_attributes!(amount: 0.10) + source.update_attributes!(amount: 0.10) update expect(item).not_to be_changed expect(db_record).to have_attributes(adjustment_total: 1) From 6ce4f9016535b9359f84a58d363af630d27bf6db Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:34:54 -0600 Subject: [PATCH 05/17] Use in-memory objects for adjustments in ItemAdjustments This avoids extra queries and should improve performance. It also should make refactoring easier without adding extra queries, including some upcoming tax-refactoring I'm working on. --- core/app/models/spree/item_adjustments.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/core/app/models/spree/item_adjustments.rb b/core/app/models/spree/item_adjustments.rb index 52d9787d49c..0985e1a9c71 100644 --- a/core/app/models/spree/item_adjustments.rb +++ b/core/app/models/spree/item_adjustments.rb @@ -83,11 +83,7 @@ def update private def adjustments - # This is done intentionally to avoid loading the association. If the - # association is loaded, the records may become stale due to code - # elsewhere in spree. When that is remedied, this should be changed to - # just item.adjustments - @adjustments ||= item.adjustments.all.to_a + @adjustments ||= item.adjustments.to_a end end end From 69982a8474d786b8d52919a628609c93a195306f Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:34:57 -0600 Subject: [PATCH 06/17] Fix Promotion::Actions::CreateAdjustment to update order.adjustments So that the in-memory objects are updated. --- core/app/models/spree/promotion/actions/create_adjustment.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/app/models/spree/promotion/actions/create_adjustment.rb b/core/app/models/spree/promotion/actions/create_adjustment.rb index c08c16cc5a6..6735ec0bb9a 100644 --- a/core/app/models/spree/promotion/actions/create_adjustment.rb +++ b/core/app/models/spree/promotion/actions/create_adjustment.rb @@ -22,10 +22,9 @@ def perform(options = {}) return if promotion_credit_exists?(order) amount = compute_amount(order) - Spree::Adjustment.create!( + order.adjustments.create!( amount: amount, order: order, - adjustable: order, source: self, promotion_code: options[:promotion_code], label: "#{Spree.t(:promotion)} (#{promotion.name})" From 7af58983c29373edf1f9bf190ba419b971b7cdb0 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:35:00 -0600 Subject: [PATCH 07/17] Promotion::Actions::CreateItemAdjustment update in-memory adjustments This switches to update the adjustable `adjustments` instead of the promotion action adjustments. The former is most likely more important. --- .../models/spree/promotion/actions/create_item_adjustments.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/promotion/actions/create_item_adjustments.rb b/core/app/models/spree/promotion/actions/create_item_adjustments.rb index a78905721ac..11236cbde39 100644 --- a/core/app/models/spree/promotion/actions/create_item_adjustments.rb +++ b/core/app/models/spree/promotion/actions/create_item_adjustments.rb @@ -40,9 +40,9 @@ def compute_amount(adjustable) def create_adjustment(adjustable, order, promotion_code) amount = compute_amount(adjustable) return if amount == 0 - adjustments.create!( + adjustable.adjustments.create!( + source: self, amount: amount, - adjustable: adjustable, order: order, promotion_code: promotion_code, label: "#{Spree.t(:promotion)} (#{promotion.name})" From 03f3c821ad27e44ba8772e75b1477ad0c429d2f8 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:35:07 -0600 Subject: [PATCH 08/17] Tax::OrderAdjuster update in-memory adjustments This keeps `item.adjustments` up to date. --- core/app/models/spree/tax/order_adjuster.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/app/models/spree/tax/order_adjuster.rb b/core/app/models/spree/tax/order_adjuster.rb index c2b2d4ab210..07122f22742 100644 --- a/core/app/models/spree/tax/order_adjuster.rb +++ b/core/app/models/spree/tax/order_adjuster.rb @@ -17,7 +17,7 @@ def adjust! return unless order_tax_zone(order) [order, *order.line_items, *order.shipments].each do |item| - item.adjustments.tax.destroy_all + item.adjustments.destroy(item.adjustments.select(&:tax?)) end (order.line_items + order.shipments).each do |item| From 1fa59b15fb859a19cb6fd71b2326bb3e1ae1714a Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:35:13 -0600 Subject: [PATCH 09/17] Spree::TaxRate update in-memory adjustments This switches to update the adjustable `adjustments` instead of the tax rate adjustments. The former is most likely more important. --- core/app/models/spree/tax_rate.rb | 6 +++--- core/spec/models/spree/tax/item_adjuster_spec.rb | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb index 005f210d14b..88803592832 100644 --- a/core/app/models/spree/tax_rate.rb +++ b/core/app/models/spree/tax_rate.rb @@ -85,13 +85,13 @@ def adjust(order_tax_zone, item) included = included_in_price && amount > 0 - adjustments.create!({ - adjustable: item, + item.adjustments.create!( + source: self, amount: amount, order_id: item.order_id, label: adjustment_label(amount), included: included - }) + ) end # This method is used by Adjustment#update to recalculate the cost. diff --git a/core/spec/models/spree/tax/item_adjuster_spec.rb b/core/spec/models/spree/tax/item_adjuster_spec.rb index f8bdfe9b0f1..1f58f9f1dfe 100644 --- a/core/spec/models/spree/tax/item_adjuster_spec.rb +++ b/core/spec/models/spree/tax/item_adjuster_spec.rb @@ -2,7 +2,7 @@ RSpec.describe Spree::Tax::ItemAdjuster do subject(:adjuster) { described_class.new(item) } - let(:order) { Spree::Order.new } + let(:order) { create(:order) } let(:item) { Spree::LineItem.new(order: order) } before do @@ -64,7 +64,6 @@ before { allow(item).to receive(:tax_category).and_return(item_tax_category) } it 'creates an adjustment for every matching rate' do - expect(rate_1).to receive_message_chain(:adjustments, :create!) expect(adjuster.adjust!.length).to eq(1) end end From bcea90299b26ce9c01077eeb79e70bce75b52699 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:35:16 -0600 Subject: [PATCH 10/17] order_cancellations_spec.rb update in-memory adjustments This keeps `line_item.adjustments` up to date. --- core/spec/models/spree/order_cancellations_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/spec/models/spree/order_cancellations_spec.rb b/core/spec/models/spree/order_cancellations_spec.rb index 2e5468ce4da..4222631e094 100644 --- a/core/spec/models/spree/order_cancellations_spec.rb +++ b/core/spec/models/spree/order_cancellations_spec.rb @@ -139,9 +139,9 @@ order.contents.add(line_item.variant) # make the total $1.67 so it divides unevenly - Spree::Adjustment.tax.create!( + line_item.adjustments.create!( + source_type: 'Spree::TaxRate', order: order, - adjustable: line_item, amount: 0.01, label: 'some fake tax', finalized: true From d27d0925b0482a7d34fd525d42ddd3c8ce5e1012 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:35:24 -0600 Subject: [PATCH 11/17] Change UnitCancel adjustments association to has_many This makes it the same as all the other `adjustments` relationships which makes the repair code in Adjustment simpler, and it makes it easier to query for duplicates. Also, use `line_item.adjustments.create` instead of `unit_cancel.adjustments.create`. The latter bypasses `line_item.adjustments` which makes them outdated for further use. --- core/app/models/spree/unit_cancel.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/unit_cancel.rb b/core/app/models/spree/unit_cancel.rb index 0e33d09f2ac..931808e6a18 100644 --- a/core/app/models/spree/unit_cancel.rb +++ b/core/app/models/spree/unit_cancel.rb @@ -6,18 +6,18 @@ class Spree::UnitCancel < Spree::Base DEFAULT_REASON = 'Cancel' belongs_to :inventory_unit - has_one :adjustment, as: :source, dependent: :destroy + has_many :adjustments, as: :source, dependent: :destroy validates :inventory_unit, presence: true # Creates necessary cancel adjustments for the line item. def adjust! - raise "Adjustment is already created" if adjustment + raise "Adjustment is already created" if adjustments.exists? amount = compute_amount(inventory_unit.line_item) - create_adjustment!( - adjustable: inventory_unit.line_item, + inventory_unit.line_item.adjustments.create!( + source: self, amount: amount, order: inventory_unit.order, label: "#{Spree.t(:cancellation)} - #{reason}", From 9b4e49315714f7c1d519861ca291954778561834 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:35:30 -0600 Subject: [PATCH 12/17] Adjustment factories update in-memory objects `create(:adjustment)` seems like a reasonble thing to do, but it doesn't update, e.g., `line_item.adjustments` and triggers the warning we have in place in Adjustment. --- .../factories/adjustment_factory.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/core/lib/spree/testing_support/factories/adjustment_factory.rb b/core/lib/spree/testing_support/factories/adjustment_factory.rb index 65e3015f6bf..1ed41bb211c 100644 --- a/core/lib/spree/testing_support/factories/adjustment_factory.rb +++ b/core/lib/spree/testing_support/factories/adjustment_factory.rb @@ -12,6 +12,13 @@ label 'Shipping' association(:source, factory: :tax_rate) eligible true + + after(:build) do |adjustment| + adjustments = adjustment.adjustable.adjustments + if adjustments.loaded? && !adjustments.include?(adjustment) + adjustments.proxy_association.add_to_target(adjustment) + end + end end factory :tax_adjustment, class: Spree::Adjustment do @@ -22,6 +29,13 @@ association(:source, factory: :tax_rate) eligible true + after(:build) do |adjustment| + adjustments = adjustment.adjustable.adjustments + if adjustments.loaded? && !adjustments.include?(adjustment) + adjustments.proxy_association.add_to_target(adjustment) + end + end + after(:create) do |adjustment| # Set correct tax category, so that adjustment amount is not 0 if adjustment.adjustable.is_a?(Spree::LineItem) From 1f8972ecd1a0beee3b052339d2606e764d8c4973 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:35:34 -0600 Subject: [PATCH 13/17] Refactor adjustment_factory slightly Nest :tax_adjustment factory under :adjustment factory Avoids duplicated `after_build` code. --- .../factories/adjustment_factory.rb | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/core/lib/spree/testing_support/factories/adjustment_factory.rb b/core/lib/spree/testing_support/factories/adjustment_factory.rb index 1ed41bb211c..581f1d77f7d 100644 --- a/core/lib/spree/testing_support/factories/adjustment_factory.rb +++ b/core/lib/spree/testing_support/factories/adjustment_factory.rb @@ -19,29 +19,20 @@ adjustments.proxy_association.add_to_target(adjustment) end end - end - factory :tax_adjustment, class: Spree::Adjustment do - order { adjustable.order } - association(:adjustable, factory: :line_item) - amount 10.0 - label 'VAT 5%' - association(:source, factory: :tax_rate) - eligible true - - after(:build) do |adjustment| - adjustments = adjustment.adjustable.adjustments - if adjustments.loaded? && !adjustments.include?(adjustment) - adjustments.proxy_association.add_to_target(adjustment) - end - end + factory :tax_adjustment, class: Spree::Adjustment do + order { adjustable.order } + association(:adjustable, factory: :line_item) + amount 10.0 + label 'VAT 5%' - after(:create) do |adjustment| - # Set correct tax category, so that adjustment amount is not 0 - if adjustment.adjustable.is_a?(Spree::LineItem) - adjustment.source.tax_category = adjustment.adjustable.tax_category - adjustment.source.save - adjustment.update! + after(:create) do |adjustment| + # Set correct tax category, so that adjustment amount is not 0 + if adjustment.adjustable.is_a?(Spree::LineItem) + adjustment.source.tax_category = adjustment.adjustable.tax_category + adjustment.source.save + adjustment.update! + end end end end From bbfc0680ad6bc57e806c754a61a1c73645964431 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 06:46:47 -0600 Subject: [PATCH 14/17] Change `puts` to deprecation warning This shouldn't happen in Solidus core anymore. --- core/app/models/spree/adjustment.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/adjustment.rb b/core/app/models/spree/adjustment.rb index e8e061bc5c3..fbd205a8085 100644 --- a/core/app/models/spree/adjustment.rb +++ b/core/app/models/spree/adjustment.rb @@ -185,16 +185,14 @@ def require_promotion_code? def repair_adjustments_associations_on_create if adjustable.adjustments.loaded? && !adjustable.adjustments.include?(self) - # Note: I will update this to a deprecation once I've gotten rid of all the occurrences in Solidus - puts("Adjustment was not added to #{adjustable.class}. Add adjustments via `adjustable.adjustments.create!`. Partial call stack: #{caller.select { |line| line =~ %r(/(app|spec)/) }}.", caller) + Spree::Deprecation.warn("Adjustment was not added to #{adjustable.class}. Add adjustments via `adjustable.adjustments.create!`. Partial call stack: #{caller.select { |line| line =~ %r(/(app|spec)/) }}.", caller) adjustable.adjustments.proxy_association.add_to_target(self) end end def repair_adjustments_associations_on_destroy if adjustable.adjustments.loaded? && adjustable.adjustments.include?(self) - # Note: I will update this to a deprecation once I've gotten rid of all the occurrences in Solidus - puts("Adjustment was not removed from #{adjustable.class}. Remove adjustments via `adjustable.adjustments.destroy`. Partial call stack: #{caller.select { |line| line =~ %r(/(app|spec)/) }}.", caller) + Spree::Deprecation.warn("Adjustment was not removed from #{adjustable.class}. Remove adjustments via `adjustable.adjustments.destroy`. Partial call stack: #{caller.select { |line| line =~ %r(/(app|spec)/) }}.", caller) adjustable.adjustments.proxy_association.target.delete(self) end end From 9ad5b2381f05133b12fc3283b097ccef925ca0ab Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 24 Aug 2016 11:57:43 -0600 Subject: [PATCH 15/17] Update adjustment destroying in tax adjusters This `destroy_all` was on a scope and thus could cause data to be out of sync. And, as mentioned by jhawthorn in the PR review we can move tax adjustment destroys to ItemAdjuster because: > orders here should never have tax adjustments (they've been only on > items since spree 2.2) --- core/app/models/spree/tax/item_adjuster.rb | 5 ++--- core/app/models/spree/tax/order_adjuster.rb | 5 ----- core/spec/models/spree/tax/order_adjuster_spec.rb | 2 -- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/core/app/models/spree/tax/item_adjuster.rb b/core/app/models/spree/tax/item_adjuster.rb index 8d0c4ffad0b..067392ae961 100644 --- a/core/app/models/spree/tax/item_adjuster.rb +++ b/core/app/models/spree/tax/item_adjuster.rb @@ -15,7 +15,6 @@ def initialize(item, options = {}) @rates_for_order_zone = options[:rates_for_order_zone] @rates_for_default_zone = options[:rates_for_default_zone] @order_tax_zone = options[:order_tax_zone] - @skip_destroy_adjustments = options[:skip_destroy_adjustments] end # Deletes all existing tax adjustments and creates new adjustments for all @@ -27,8 +26,8 @@ def initialize(item, options = {}) # @return [Array] newly created adjustments def adjust! return unless order_tax_zone(order) - # Using .destroy_all to make sure callbacks fire - item.adjustments.tax.destroy_all unless @skip_destroy_adjustments + + item.adjustments.destroy(item.adjustments.select(&:tax?)) rates_for_item(item).map { |rate| rate.adjust(order_tax_zone(order), item) } end diff --git a/core/app/models/spree/tax/order_adjuster.rb b/core/app/models/spree/tax/order_adjuster.rb index 07122f22742..eb79072e54b 100644 --- a/core/app/models/spree/tax/order_adjuster.rb +++ b/core/app/models/spree/tax/order_adjuster.rb @@ -16,10 +16,6 @@ def initialize(order) def adjust! return unless order_tax_zone(order) - [order, *order.line_items, *order.shipments].each do |item| - item.adjustments.destroy(item.adjustments.select(&:tax?)) - end - (order.line_items + order.shipments).each do |item| ItemAdjuster.new(item, order_wide_options).adjust! end @@ -32,7 +28,6 @@ def order_wide_options rates_for_order_zone: rates_for_order_zone(order), rates_for_default_zone: rates_for_default_zone, order_tax_zone: order_tax_zone(order), - skip_destroy_adjustments: true } end end diff --git a/core/spec/models/spree/tax/order_adjuster_spec.rb b/core/spec/models/spree/tax/order_adjuster_spec.rb index f4cd2b01235..8cfd7b84f85 100644 --- a/core/spec/models/spree/tax/order_adjuster_spec.rb +++ b/core/spec/models/spree/tax/order_adjuster_spec.rb @@ -31,7 +31,6 @@ rates_for_order_zone: rates_for_order_zone, rates_for_default_zone: [], order_tax_zone: zone, - skip_destroy_adjustments: true ).and_return(item_adjuster) expect(Spree::Tax::ItemAdjuster).to receive(:new). with( @@ -39,7 +38,6 @@ rates_for_order_zone: rates_for_order_zone, rates_for_default_zone: [], order_tax_zone: zone, - skip_destroy_adjustments: true ).and_return(item_adjuster) expect(item_adjuster).to receive(:adjust!).twice From a0234dbe4f11bd67b8ea5f2df9405777f9654892 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Tue, 30 Aug 2016 16:30:32 -0600 Subject: [PATCH 16/17] Alternate solution for UnitCancel adjustment creation Per PR feedback. Avoid changing the `has_many`/`has_one` relationship while still keeping in-memory objects up to date. --- core/app/models/spree/unit_cancel.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/unit_cancel.rb b/core/app/models/spree/unit_cancel.rb index 931808e6a18..19c9b652284 100644 --- a/core/app/models/spree/unit_cancel.rb +++ b/core/app/models/spree/unit_cancel.rb @@ -6,17 +6,17 @@ class Spree::UnitCancel < Spree::Base DEFAULT_REASON = 'Cancel' belongs_to :inventory_unit - has_many :adjustments, as: :source, dependent: :destroy + has_one :adjustment, as: :source, dependent: :destroy validates :inventory_unit, presence: true # Creates necessary cancel adjustments for the line item. def adjust! - raise "Adjustment is already created" if adjustments.exists? + raise "Adjustment is already created" if adjustment amount = compute_amount(inventory_unit.line_item) - inventory_unit.line_item.adjustments.create!( + self.adjustment = inventory_unit.line_item.adjustments.create!( source: self, amount: amount, order: inventory_unit.order, From e8a6a70d199fcff75844bbb89ab846593c6cfc5b Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Tue, 30 Aug 2016 16:53:47 -0600 Subject: [PATCH 17/17] Add Changelog entry for in-memory updates --- CHANGELOG.md | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d644dc7f77f..fe263e0919d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,46 @@ ## Solidus 1.4.0 (unreleased) +* Use in-memory objects in OrderUpdater and related areas. + + Solidus now uses in-memory data for updating orders in and around + OrderUpdater. E.g. if an order already has `order.line_items` loaded into + memory when OrderUpdater is run then it will use that information rather + than requerying the database for it. This should help performance and makes + some upcoming refactoring easier. + + Warning: If you bypass ActiveRecord while making updates to your orders you + run the risk of generating invalid data. Example: + + order.line_items.to_a + order.line_items.update_all(price: ...) + order.update! + + Will now result in incorrect calculations in OrderUpdater because the line + items will not be refetched. + + In particular, when creating adjustments, you should always create the + adjustment using the adjustable relationship. + + Good example: + + line_item.adjustments.create!(source: tax_rate, ...) + + Bad examples: + + tax_rate.adjustments.create!(adjustable: line_item, ...) + Spree::Adjustment.create!(adjustable: line_item, source: tax_rate, ...) + + We try to detect the latter examples and repair the in-memory objects (with + a deprecation warning) but you should ensure that your code is keeping the + adjustable's in-memory associations up to date. Custom promotion actions are + an area likely to have this issue. + + https://github.com/solidusio/solidus/pull/1356 + https://github.com/solidusio/solidus/pull/1389 + https://github.com/solidusio/solidus/pull/1400 + https://github.com/solidusio/solidus/pull/1401 + * Make some 'wallet' behavior configurable NOTE: `Order#persist_user_credit_card` has been renamed to