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 diff --git a/core/app/models/spree/adjustment.rb b/core/app/models/spree/adjustment.rb index 12d9ab54237..fbd205a8085 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,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 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 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})" 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})" 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 9df6a89a408..eb79072e54b 100644 --- a/core/app/models/spree/tax/order_adjuster.rb +++ b/core/app/models/spree/tax/order_adjuster.rb @@ -16,8 +16,6 @@ def initialize(order) def adjust! return unless order_tax_zone(order) - order.all_adjustments.tax.destroy_all - (order.line_items + order.shipments).each do |item| ItemAdjuster.new(item, order_wide_options).adjust! end @@ -30,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/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/app/models/spree/unit_cancel.rb b/core/app/models/spree/unit_cancel.rb index 0e33d09f2ac..19c9b652284 100644 --- a/core/app/models/spree/unit_cancel.rb +++ b/core/app/models/spree/unit_cancel.rb @@ -16,8 +16,8 @@ def adjust! amount = compute_amount(inventory_unit.line_item) - create_adjustment!( - adjustable: inventory_unit.line_item, + self.adjustment = inventory_unit.line_item.adjustments.create!( + source: self, amount: amount, order: inventory_unit.order, label: "#{Spree.t(:cancellation)} - #{reason}", diff --git a/core/lib/spree/testing_support/factories/adjustment_factory.rb b/core/lib/spree/testing_support/factories/adjustment_factory.rb index 65e3015f6bf..581f1d77f7d 100644 --- a/core/lib/spree/testing_support/factories/adjustment_factory.rb +++ b/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 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 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) 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 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 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