Use in-memory objects for adjustments in ItemAdjustments

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,19 @@ 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)
+        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)
+        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
   end
 end

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

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})"

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})"

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.destroy(item.adjustments.select(&:tax?))
+        end
 
         (order.line_items + order.shipments).each do |item|
           ItemAdjuster.new(item, order_wide_options).adjust!

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.

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}",

core/lib/spree/testing_support/factories/adjustment_factory.rb

@@ -12,22 +12,27 @@
     label 'Shipping'
     association(:source, factory: :tax_rate)
     eligible true
-  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

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

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)

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

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