diff --git a/Gemfile b/Gemfile index 7f69dd3a643..921dbf3848e 100644 --- a/Gemfile +++ b/Gemfile @@ -21,7 +21,7 @@ gem 'pg', '~> 1.0', require: false if dbs.match?(/all|postgres/) gem 'fast_sqlite', require: false if dbs.match?(/all|sqlite/) gem 'sqlite3', '~> 1.4', require: false if dbs.match?(/all|sqlite/) - +gem 'db-query-matchers' gem 'database_cleaner', '~> 2.0', require: false gem 'rspec-activemodel-mocks', '~> 1.1', require: false gem 'rspec-rails', '~> 6.0.3', require: false diff --git a/TODO.md b/TODO.md new file mode 100644 index 00000000000..f83e2e289b9 --- /dev/null +++ b/TODO.md @@ -0,0 +1,9 @@ +In-Memory Order Updater TODO +=== + +- [x] Add additional cases to item_total_updater_spec (doesn't currently account for included adjustments) +- [ ] Consider Sofia's recommendation to break this class into POROs to simplify testing +- [ ] Add test coverage for `update_item_total` when line item totals change +- [ ] Test coverage to ensure state changes aren't persisted (if someone changes current implementation) +- [ ] Handle persistence in `update_promotions` +- [ ] Handle persistence in `update_taxes` diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb new file mode 100644 index 00000000000..963257bdd1c --- /dev/null +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -0,0 +1,250 @@ +# frozen_string_literal: true + +module Spree + class InMemoryOrderUpdater + attr_reader :order + + delegate :payments, :line_items, :adjustments, :all_adjustments, :shipments, :quantity, to: :order + + def initialize(order) + @order = order + end + + # This is a multi-purpose method for processing logic related to changes in the Order. + # It is meant to be called from various observers so that the Order is aware of changes + # that affect totals and other values stored in the Order. + # + # This method should never do anything to the Order that results in a save call on the + # object with callbacks (otherwise you will end up in an infinite recursion as the + # associations try to save and then in turn try to call +update!+ again.) + def recalculate(persist: true) + order.transaction do + recalculate_item_count + update_shipment_amounts(persist:) + update_totals(persist:) + if order.completed? + recalculate_payment_state + update_shipments + recalculate_shipment_state + end + + Spree::Bus.publish(:order_recalculated, order:) + + persist_totals if persist + end + end + alias_method :update, :recalculate + deprecate update: :recalculate, deprecator: Spree.deprecator + + # Recalculates the +shipment_state+ attribute according to the following logic: + # + # shipped when all Shipments are in the "shipped" state + # partial when at least one Shipment has a state of "shipped" and there is another Shipment with a state other than "shipped" + # or there are InventoryUnits associated with the order that have a state of "sold" but are not associated with a Shipment. + # ready when all Shipments are in the "ready" state + # backorder when there is backordered inventory associated with an order + # pending when all Shipments are in the "pending" state + # + # The +shipment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention. + def recalculate_shipment_state + log_state_change('shipment') do + order.shipment_state = determine_shipment_state + end + + order.shipment_state + end + alias_method :update_shipment_state, :recalculate_shipment_state + deprecate update_shipment_state: :recalculate_shipment_state, deprecator: Spree.deprecator + + # Recalculates the +payment_state+ attribute according to the following logic: + # + # paid when +payment_total+ is equal to +total+ + # balance_due when +payment_total+ is less than +total+ + # credit_owed when +payment_total+ is greater than +total+ + # failed when most recent payment is in the failed state + # void when the order has been canceled and the payment total is 0 + # + # The +payment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention. + def recalculate_payment_state + log_state_change('payment') do + order.payment_state = determine_payment_state + end + + order.payment_state + end + alias_method :update_payment_state, :recalculate_shipment_state + deprecate update_payment_state: :recalculate_shipment_state, deprecator: Spree.deprecator + + private + + def determine_payment_state + if payments.present? && payments.valid.empty? && order.outstanding_balance != 0 + 'failed' + elsif order.state == 'canceled' && order.payment_total.zero? + 'void' + elsif order.outstanding_balance > 0 + 'balance_due' + elsif order.outstanding_balance < 0 + 'credit_owed' + else + # outstanding_balance == 0 + 'paid' + end + end + + def determine_shipment_state + if order.backordered? + 'backorder' + else + # get all the shipment states for this order + shipment_states = shipments.states + if shipment_states.size > 1 + # multiple shiment states means it's most likely partially shipped + 'partial' + else + # will return nil if no shipments are found + shipment_states.first + end + end + end + + # This will update and select the best promotion adjustment, update tax + # adjustments, update cancellation adjustments, and then update the total + # fields (promo_total, included_tax_total, additional_tax_total, and + # adjustment_total) on the item. + # @return [void] + def update_adjustments(persist:) + # Promotion adjustments must be applied first, then tax adjustments. + # This fits the criteria for VAT tax as outlined here: + # http://www.hmrc.gov.uk/vat/managing/charging/discounts-etc.htm#1 + # It also fits the criteria for sales tax as outlined here: + # http://www.boe.ca.gov/formspubs/pub113/ + update_promotions(persist:) + update_tax_adjustments + recalculate_item_totals + persist_changes_to_items if persist + end + + # Updates the following Order total values: + # + # +payment_total+ The total value of all finalized Payments (NOTE: non-finalized Payments are excluded) + # +item_total+ The total value of all LineItems + # +adjustment_total+ The total value of all adjustments (promotions, credits, etc.) + # +promo_total+ The total value of all promotion adjustments + # +total+ The so-called "order total." This is equivalent to +item_total+ plus +adjustment_total+. + def update_totals(persist:) + recalculate_payment_total + recalculate_item_total + recalculate_shipment_total + update_adjustment_total(persist:) + end + + def update_shipment_amounts(persist:) + shipments.each { _1.update_amounts(persist:) } + end + + def update_adjustment_total(persist:) + update_adjustments(persist:) + + all_items = line_items + shipments + order_tax_adjustments = adjustments.select(&:tax?) + + order.adjustment_total = all_items.sum(&:adjustment_total) + adjustments.sum(&:amount) + order.included_tax_total = all_items.sum(&:included_tax_total) + order_tax_adjustments.select(&:included?).sum(&:amount) + order.additional_tax_total = all_items.sum(&:additional_tax_total) + order_tax_adjustments.reject(&:included?).sum(&:amount) + + recalculate_order_total + end + + def update_promotions(persist:) + if persist + Spree::Config.promotions.order_adjuster_class + else + InMemoryOrderAdjuster + end.new(order).call + end + + # TODO: split implementation based on 'persist' + def update_tax_adjustments + Spree::Config.tax_adjuster_class.new(order).adjust! + end + + def update_cancellations + end + deprecate :update_cancellations, deprecator: Spree.deprecator + + def recalculate_item_totals + [*line_items, *shipments].each do |item| + Spree::ItemTotal.new(item).recalculate! + end + end + + def persist_item_changes + [*line_items, *shipments].each do |item| + next unless item.changed? + + item.update_columns( + promo_total: item.promo_total, + included_tax_total: item.included_tax_total, + additional_tax_total: item.additional_tax_total, + adjustment_total: item.adjustment_total, + updated_at: Time.current, + ) + end + end + + # give each of the shipments a chance to update themselves + def update_shipments + shipments.each(&:update_state) + end + + def recalculate_payment_total + order.payment_total = payments.completed.includes(:refunds).sum { |payment| payment.amount - payment.refunds.sum(:amount) } + end + + def recalculate_shipment_total + order.shipment_total = shipments.to_a.sum(&:cost) + recalculate_order_total + end + + def recalculate_order_total + order.total = order.item_total + order.shipment_total + order.adjustment_total + end + + def recalculate_item_count + order.item_count = line_items.to_a.sum(&:quantity) + end + + def recalculate_item_total + order.item_total = line_items.to_a.sum(&:amount) + recalculate_order_total + end + + def persist_totals + order.save! + end + + def log_state_change(name) + state = "#{name}_state" + old_state = order.public_send(state) + yield + new_state = order.public_send(state) + if old_state != new_state + order.state_changes.new( + previous_state: old_state, + next_state: new_state, + user_id: order.user_id, + name: + ) + end + end + + class InMemoryOrderAdjuster + def initialize(order) + end + + def call + end + end + end +end diff --git a/core/app/models/spree/item_total.rb b/core/app/models/spree/item_total.rb new file mode 100644 index 00000000000..3ef666e39e8 --- /dev/null +++ b/core/app/models/spree/item_total.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class Spree::ItemTotal + def initialize(item) + @item = item + end + + def recalculate! + tax_adjustments = item.adjustments.select(&:tax?) + + # Included tax adjustments are those which are included in the price. + # These ones should not affect the eventual total price. + # + # Additional tax adjustments are the opposite, affecting the final total. + item.included_tax_total = tax_adjustments.select(&:included?).sum(&:amount) + item.additional_tax_total = tax_adjustments.reject(&:included?).sum(&:amount) + + item.adjustment_total = item.adjustments.reject(&:included?).sum(&:amount) + end + + private + + attr_reader :item +end diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index 6808518448d..310d74fb327 100644 --- a/core/app/models/spree/shipment.rb +++ b/core/app/models/spree/shipment.rb @@ -258,10 +258,10 @@ def tracking_url @tracking_url ||= shipping_method.build_tracking_url(tracking) end - def update_amounts + def update_amounts(persist: true) if selected_shipping_rate self.cost = selected_shipping_rate.cost - if changed? + if changed? && persist update_columns( cost:, updated_at: Time.current diff --git a/core/spec/models/spree/in_memory_order_updater_spec.rb b/core/spec/models/spree/in_memory_order_updater_spec.rb new file mode 100644 index 00000000000..f754d603301 --- /dev/null +++ b/core/spec/models/spree/in_memory_order_updater_spec.rb @@ -0,0 +1,454 @@ +# frozen_string_literal: true + +require 'rails_helper' + +module Spree + RSpec.describe InMemoryOrderUpdater, type: :model do + let!(:store) { create :store } + let(:order) { Spree::Order.create } + let(:updater) { described_class.new(order) } + + describe "#recalculate" do + subject { updater.recalculate(persist:) } + + let(:new_store) { create(:store) } + + context "when the persist flag is set to 'false'" do + let(:persist) { false } + + it "does not persist changes to order" do + order.store = new_store + + expect { + subject + }.not_to make_database_queries(manipulative: true) + + expect(order.store).to eq new_store + expect(order.reload.store).not_to eq new_store + end + + it "does not persist changes to the item count" do + order.line_items << build(:line_item) + + expect { + subject + }.not_to make_database_queries(manipulative: true) + + expect(order.item_count).to eq 1 + expect(order.reload.item_count).to eq 0 + end + + context 'when a shipment is attached to the order' do + let(:shipment) { create(:shipment) } + + before do + order.shipments << shipment + end + + it 'does not make manipulative database queries' do + expect { + subject + }.not_to make_database_queries(manipulative: true) + end + + context 'when the shipment has a selected shipping rate' do + let(:shipment) { create(:shipment, shipping_rates: [build(:shipping_rate, selected: true)]) } + + it 'does not make manipulative database queries' do + expect { + subject + }.not_to make_database_queries(manipulative: true) + end + end + end + end + + context "when the persist flag is set to 'true'" do + let(:persist) { true } + + it "persists any changes to order" do + order.store = new_store + + expect { + subject + }.to make_database_queries(manipulative: true) + + expect(order.reload.store).to eq new_store + end + end + end + + context "order totals" do + before do + 2.times do + create(:line_item, order: order, price: 10) + end + end + + context 'with refund' do + it "updates payment totals" do + create(:payment_with_refund, order: order, amount: 33.25, refund_amount: 3) + updater.recalculate + expect(order.payment_total).to eq(30.25) + end + end + + it "update item total" do + expect { + updater.recalculate + }.to change { order.item_total }.to 20 + end + + it "update shipment total" do + create(:shipment, order: order, cost: 10) + expect { + updater.recalculate + }.to change { order.shipment_total }.to 10 + end + + context 'with a source-less line item adjustment' do + let(:line_item) { create(:line_item, order: order, price: 10) } + before do + create(:adjustment, source: nil, adjustable: line_item, order: order, amount: -5) + end + + it "updates the line item total" do + expect { updater.recalculate }.to change { line_item.reload.adjustment_total }.from(0).to(-5) + end + end + + it "update order adjustments" do + create(:adjustment, adjustable: order, order: order, source: nil, amount: 10) + + expect { + updater.recalculate + }.to change { + order.adjustment_total + }.from(0).to(10) + end + end + + describe 'promotion recalculation' do + it "calls the Promotion Adjustments Recalculator" do + adjuster = double(:call) + expect(Spree::Config.promotions.order_adjuster_class).to receive(:new).and_return(adjuster) + expect(adjuster).to receive(:call) + order.recalculate + end + end + + describe 'tax recalculation' do + let!(:ship_address) { create(:address) } + let!(:tax_zone) { create(:global_zone) } # will include the above address + let!(:tax_rate) { create(:tax_rate, zone: tax_zone, tax_categories: [tax_category]) } + + let(:order) do + create( + :order_with_line_items, + line_items_attributes: [{ price: 10, variant: variant }], + ship_address: ship_address, + ) + end + let(:line_item) { order.line_items[0] } + + let(:variant) { create(:variant, tax_category: tax_category) } + let(:tax_category) { create(:tax_category) } + + context 'when the item quantity has changed' do + before do + line_item.update!(quantity: 2) + end + + it 'updates the promotion amount' do + expect { + order.recalculate + }.to change { + line_item.additional_tax_total + }.from(1).to(2) + end + + context "when recalculating the order in memory" do + it "raises an error" do + order_adjuster = double + allow(order_adjuster).to receive(:call) { raise NotImplementedError } + allow(Spree::InMemoryOrderUpdater::InMemoryOrderAdjuster).to receive(:new).and_return(order_adjuster) + + expect{ described_class.new(order).recalculate(persist: false) } + .to raise_error(NotImplementedError) + end + end + end + + context 'with a custom tax_calculator_class' do + let(:custom_calculator_class) { double } + let(:custom_calculator_instance) { double } + + before do + order # generate this first so we can expect it + stub_spree_preferences(tax_calculator_class: custom_calculator_class) + + allow(custom_calculator_class).to receive(:new).and_return(custom_calculator_instance) + allow(custom_calculator_instance).to receive(:calculate).and_return( + Spree::Tax::OrderTax.new( + order_id: order.id, + order_taxes: [ + Spree::Tax::ItemTax.new( + label: "Delivery Fee", + tax_rate: tax_rate, + amount: 2.60, + included_in_price: false + ) + ], + line_item_taxes: [ + Spree::Tax::ItemTax.new( + item_id: line_item.id, + label: "Item Tax", + tax_rate: tax_rate, + amount: 1.40, + included_in_price: false + ) + ], + shipment_taxes: [] + ) + ) + end + + it 'uses the configured class' do + order.recalculate + + expect(custom_calculator_class).to have_received(:new).with(order).at_least(:once) + expect(custom_calculator_instance).to have_received(:calculate).at_least(:once) + end + + it 'updates the aggregate columns' do + expect { + order.recalculate + }.to change { order.reload.additional_tax_total }.to(4.00) + .and change { order.reload.adjustment_total }.to(4.00) + end + end + end + + context "updating shipment state" do + before do + allow(order).to receive_messages backordered?: false + end + + it "is backordered" do + allow(order).to receive_messages backordered?: true + updater.recalculate_shipment_state + + expect(order.shipment_state).to eq('backorder') + end + + it "is nil" do + updater.recalculate_shipment_state + expect(order.shipment_state).to be_nil + end + + ["shipped", "ready", "pending"].each do |state| + it "is #{state}" do + create(:shipment, order: order, state: state) + updater.recalculate_shipment_state + expect(order.shipment_state).to eq(state) + end + end + + it "is partial" do + create(:shipment, order: order, state: 'pending') + create(:shipment, order: order, state: 'ready') + updater.recalculate_shipment_state + expect(order.shipment_state).to eq('partial') + end + end + + context "updating payment state" do + let(:order) { build(:order) } + before { allow(order).to receive(:refund_total).and_return(0) } + + context 'no valid payments with non-zero order total' do + it "is failed" do + create(:payment, order: order, state: 'invalid') + order.total = 1 + order.payment_total = 0 + + updater.recalculate_payment_state + expect(order.payment_state).to eq('failed') + end + end + + context 'invalid payments are present but order total is zero' do + it 'is paid' do + order.payments << Spree::Payment.new(state: 'invalid') + order.total = 0 + order.payment_total = 0 + + expect { + updater.recalculate_payment_state + }.to change { order.payment_state }.to 'paid' + end + end + + context "payment total is greater than order total" do + it "is credit_owed" do + order.payment_total = 2 + order.total = 1 + + expect { + updater.recalculate_payment_state + }.to change { order.payment_state }.to 'credit_owed' + end + end + + context "order total is greater than payment total" do + it "is balance_due" do + order.payment_total = 1 + order.total = 2 + + expect { + updater.recalculate_payment_state + }.to change { order.payment_state }.to 'balance_due' + end + end + + context "order total equals payment total" do + it "is paid" do + order.payment_total = 30 + order.total = 30 + + expect { + updater.recalculate_payment_state + }.to change { order.payment_state }.to 'paid' + end + end + + context "order is canceled" do + before do + order.state = 'canceled' + end + + context "and is still unpaid" do + it "is void" do + order.payment_total = 0 + order.total = 30 + expect { + updater.recalculate_payment_state + }.to change { order.payment_state }.to 'void' + end + end + + context "and is paid" do + it "is credit_owed" do + order.payment_total = 30 + order.total = 30 + create(:payment, order: order, state: 'completed', amount: 30) + expect { + updater.recalculate_payment_state + }.to change { order.payment_state }.to 'credit_owed' + end + end + + context "and payment is refunded" do + it "is void" do + order.payment_total = 0 + order.total = 30 + expect { + updater.recalculate_payment_state + }.to change { order.payment_state }.to 'void' + end + end + end + end + + context "completed order" do + before { allow(order).to receive_messages completed?: true } + + it "updates payment state" do + expect(updater).to receive(:recalculate_payment_state) + updater.recalculate + end + + it "updates shipment state" do + expect(updater).to receive(:recalculate_shipment_state) + updater.recalculate + end + + context 'with a shipment' do + before { create(:shipment, order: order) } + let(:shipment){ order.shipments[0] } + + it "updates each shipment" do + expect(shipment).to receive(:update_state) + updater.recalculate + end + + it "updates the shipment amount" do + expect(shipment).to receive(:update_amounts) + updater.recalculate + end + end + end + + context "incompleted order" do + before { allow(order).to receive_messages completed?: false } + + it "doesnt update payment state" do + expect(updater).not_to receive(:recalculate_payment_state) + updater.recalculate + end + + it "doesnt update shipment state" do + expect(updater).not_to receive(:recalculate_shipment_state) + updater.recalculate + end + + it "doesnt update each shipment" do + shipment = stub_model(Spree::Shipment) + order.shipments = [shipment] + allow(order.shipments).to receive_messages(states: [], ready: [], pending: [], shipped: []) + allow(updater).to receive(:update_totals) # Otherwise this gets called and causes a scene + expect(updater).not_to receive(:update_shipments) + updater.recalculate + end + end + + context "with item with no adjustment and incorrect totals" do + let!(:line_item) { create(:line_item, order: order, price: 10) } + + it "updates the totals" do + line_item.update!(adjustment_total: 100) + expect { + order.recalculate + }.to change { line_item.reload.adjustment_total }.from(100).to(0) + end + end + + context "with 'order_recalculated' event subscription" do + let(:item) { spy('object') } + let(:bus) { Spree::Bus } + + let!(:subscription) do + bus.subscribe :order_recalculated do + item.do_something + end + end + + after { bus.unsubscribe subscription } + + it "fires the 'order_recalculated' event" do + order.recalculate + + expect(item).to have_received(:do_something) + end + end + + context "with invalid associated objects" do + let(:order) { Spree::Order.create(ship_address: Spree::Address.new) } + subject { updater.recalculate } + + it "raises because of the invalid object" do + expect { subject }.to raise_error(ActiveRecord::RecordInvalid) + end + end + end +end diff --git a/core/spec/models/spree/item_total_spec.rb b/core/spec/models/spree/item_total_spec.rb new file mode 100644 index 00000000000..1f6a2433a64 --- /dev/null +++ b/core/spec/models/spree/item_total_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Spree::ItemTotal do + describe "#recalculate!" do + subject { described_class.new(item).recalculate! } + + let(:item) { create :line_item, adjustments: } + + let(:tax_rate) { create(:tax_rate) } + + let(:arbitrary_adjustment) { create :adjustment, amount: 1, source: nil } + let(:included_tax_adjustment) { create :adjustment, amount: 2, source: tax_rate, included: true } + let(:additional_tax_adjustment) { create :adjustment, amount: 3, source: tax_rate, included: false } + + context "with multiple types of adjustments" do + let(:adjustments) { [arbitrary_adjustment, included_tax_adjustment, additional_tax_adjustment] } + + it "updates item totals" do + expect { + subject + }.to change(item, :adjustment_total).from(0).to(4). + and change { item.included_tax_total }.from(0).to(2). + and change { item.additional_tax_total }.from(0).to(3) + end + end + + context "with only an arbitrary adjustment" do + let(:adjustments) { [arbitrary_adjustment] } + + it "updates the adjustment total" do + expect { + subject + }.to change { item.adjustment_total }.from(0).to(1) + end + end + + context "with only tax adjustments" do + let(:adjustments) { [included_tax_adjustment, additional_tax_adjustment] } + + it "updates the adjustment total" do + expect { + subject + }.to change { item.adjustment_total }.from(0).to(3). + and change { item.included_tax_total }.from(0).to(2). + and change { item.additional_tax_total }.from(0).to(3) + end + end + end +end diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb index 384413166d6..cbbefe292b8 100644 --- a/core/spec/models/spree/shipment_spec.rb +++ b/core/spec/models/spree/shipment_spec.rb @@ -507,14 +507,42 @@ end end - context "updates cost when selected shipping rate is present" do - let(:shipment) { create(:shipment) } - before { shipment.selected_shipping_rate.update!(cost: 5) } + describe "#update_amounts" do + subject { shipment.update_amounts(persist: persist) } - it "updates shipment totals" do - expect { - shipment.update_amounts - }.to change { shipment.cost }.to(5) + let(:persist) { true } + let(:shipment) { create(:shipment, cost: 1) } + + context 'when the selected shipping rate cost is different than the current shipment cost' do + before { shipment.selected_shipping_rate.update!(cost: 999) } + + it "changes and persists the shipments cost" do + expect { + subject + }.to change { shipment.reload.cost }.to(999) + end + + it 'changes and persists the updated_at column' do + expect { + subject + }.to change { shipment.reload.updated_at } + end + + context 'when `persist: false` is passed' do + let(:persist) { false } + + it 'does not perform any database writes' do + expect { + subject + }.not_to make_database_queries(manipulative: true) + end + + it "changes but does not persist the shipments cost" do + subject + expect(shipment.cost).to eq 999 + expect(shipment.reload.cost).to eq 1 + end + end end end diff --git a/core/spec/rails_helper.rb b/core/spec/rails_helper.rb index f3b6e161509..c135d3893f4 100644 --- a/core/spec/rails_helper.rb +++ b/core/spec/rails_helper.rb @@ -13,6 +13,7 @@ require 'rspec/rails' require 'rspec-activemodel-mocks' require 'database_cleaner' +require 'db-query-matchers' Dir["./spec/support/**/*.rb"].sort.each { |f| require f }