From eeda9211f6e76932a32edb8c2d5e857d20f2ab29 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Mon, 19 Sep 2016 12:26:51 -0600 Subject: [PATCH 1/6] Break ItemAdjustments apart and put the logic in OrderUpdater With ItemAdjustments we updated items in this order: - item 1 - update promotions - update taxes - update cancellations - update totals - item 2 - update promotions - update taxes - update cancellations - update totals However, we need to invoke tax calculation on the order as a whole (even though the results will be stored at the line/shipment level) in order to provide a good extension point for external tax services. That means we need to do this: - update promotions - item 1 - item 2 - update taxes - item 1 - item 2 - update cancellations - item 1 - item 2 - update totals - item 1 - item 2 I couldn't think of a reasonable way to refactor ItemAdjustments to be able to do the above, without breaking backward compatibility in a significant way. And if we're going to break backward compatibility then moving this logic into OrderUpdater seemed like a better change to me as a first step. Once we have this in place we can start working on providing an extension point in the "update taxes" step where external tax services can be plugged in. --- core/app/models/spree/item_adjustments.rb | 82 ----- core/app/models/spree/order_updater.rb | 55 +++- .../models/spree/item_adjustments_spec.rb | 306 ------------------ core/spec/models/spree/order_updater_spec.rb | 239 ++++++++++++++ 4 files changed, 289 insertions(+), 393 deletions(-) delete mode 100644 core/app/models/spree/item_adjustments.rb delete mode 100644 core/spec/models/spree/item_adjustments_spec.rb diff --git a/core/app/models/spree/item_adjustments.rb b/core/app/models/spree/item_adjustments.rb deleted file mode 100644 index d4d6504ac55..00000000000 --- a/core/app/models/spree/item_adjustments.rb +++ /dev/null @@ -1,82 +0,0 @@ -module Spree - # Manage (recalculate) item (LineItem or Shipment) adjustments - class ItemAdjustments - attr_reader :item - - # @param item [Order, LineItem, Shipment] the item whose adjustments should be updated - def initialize(item) - @item = item - end - - # Update the item's adjustments and totals - # - # If the item is an {Order}, this will update and select the best - # promotion adjustment. - # - # If it is a {LineItem} or {Shipment}, it 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. - # - # This is a noop if the item is not persisted. - def update - return @item unless item.persisted? - - # 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/ - # - # Tax adjustments come in not one but *two* exciting flavours: - # Included & additional - - # 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. - - promotion_adjustments = adjustments.select(&:promotion?) - promotion_adjustments.each(&:update!) - - promo_total = Spree::Config.promotion_chooser_class.new(promotion_adjustments).update - - # Calculating the totals for the order here would be incorrect. Order's - # totals are the sum of the adjustments on all child models, as well as - # its own. - # - # We want to select the best promotion for the order, but the remainder - # of the calculations here are done in the OrderUpdater instead. - return if item.is_a?(Spree::Order) - - @item.promo_total = promo_total - - tax = adjustments.select(&:tax?) - - @item.included_tax_total = tax.select(&:included?).map(&:update!).compact.sum - @item.additional_tax_total = tax.reject(&:included?).map(&:update!).compact.sum - - item_cancellation_total = adjustments.select(&:cancellation?).map(&:update!).compact.sum - - @item.adjustment_total = @item.promo_total + @item.additional_tax_total + item_cancellation_total - - @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 - ) if @item.changed? - - @item - end - - private - - def adjustments - @adjustments ||= item.adjustments.to_a - end - end -end diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index 3a37e985c33..200f6f01612 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -31,11 +31,10 @@ def run_hooks end def recalculate_adjustments - adjustables = [*line_items, *shipments, order] - - adjustables.each do |adjustable| - Spree::ItemAdjustments.new(adjustable).update - end + update_promotions + update_taxes + update_cancellations + update_item_totals end # Updates the following Order total values: @@ -164,5 +163,51 @@ def update_payment_state def round_money(n) (n * 100).round / 100.0 end + + def update_promotions + [*line_items, *shipments, order].each do |item| + promotion_adjustments = item.adjustments.select(&:promotion?) + + promotion_adjustments.each(&:update!) + Spree::Config.promotion_chooser_class.new(promotion_adjustments).update + item.promo_total = promotion_adjustments.select(&:eligible?).sum(&:amount) + end + end + + def update_taxes + [*line_items, *shipments].each do |item| + tax_adjustments = item.adjustments.select(&:tax?) + + tax_adjustments.each(&:update!) + item.included_tax_total = tax_adjustments.select(&:included?).sum(&:amount) + item.additional_tax_total = tax_adjustments.reject(&:included?).sum(&:amount) + end + end + + def update_cancellations + [*line_items, *shipments].each do |item| + item.adjustments.select(&:cancellation?).each(&:update!) + end + end + + def update_item_totals + [*line_items, *shipments].each do |item| + item_cancellation_total = item.adjustments.select(&:cancellation?).sum(&:amount) + + item.adjustment_total = item.promo_total + + item.additional_tax_total + + item_cancellation_total + + if 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 + end end end diff --git a/core/spec/models/spree/item_adjustments_spec.rb b/core/spec/models/spree/item_adjustments_spec.rb deleted file mode 100644 index 933b859825a..00000000000 --- a/core/spec/models/spree/item_adjustments_spec.rb +++ /dev/null @@ -1,306 +0,0 @@ -require 'spec_helper' - -module Spree - describe ItemAdjustments, type: :model do - let(:order) { create :order_with_line_items, line_items_count: 1 } - let(:line_item) { order.line_items.first } - - let(:subject) { ItemAdjustments.new(line_item) } - - context '#update' do - it "updates a linked adjustment" do - tax_rate = create(:tax_rate, amount: 0.05) - create(:adjustment, order: order, source: tax_rate, adjustable: line_item) - line_item.price = 10 - line_item.tax_category = tax_rate.tax_category - - subject.update - expect(line_item.adjustment_total).to eq(0.5) - expect(line_item.additional_tax_total).to eq(0.5) - end - end - - context "taxes and promotions" do - let!(:tax_rate) do - create(:tax_rate, amount: 0.05) - end - - let!(:promotion) do - Spree::Promotion.create(name: "$10 off") - end - - let!(:promotion_action) do - calculator = Calculator::FlatRate.new(preferred_amount: 10) - Promotion::Actions::CreateItemAdjustments.create calculator: calculator, promotion: promotion - end - - before do - line_item.price = 20 - line_item.tax_category = tax_rate.tax_category - line_item.save - create(:adjustment, order: order, source: promotion_action, adjustable: line_item) - end - - context "tax included in price" do - before do - create(:adjustment, - source: tax_rate, - adjustable: line_item, - order: order, - included: true - ) - end - - it "tax has no bearing on final price" do - subject.update - line_item.reload - expect(line_item.included_tax_total).to eq(0.5) - expect(line_item.additional_tax_total).to eq(0) - expect(line_item.promo_total).to eq(-10) - expect(line_item.adjustment_total).to eq(-10) - end - - it "tax linked to order" do - order.update! - order.reload - expect(order.included_tax_total).to eq(0.5) - expect(order.additional_tax_total).to eq(00) - end - end - - context "tax excluded from price" do - before do - create(:adjustment, - source: tax_rate, - adjustable: line_item, - order: order, - included: false - ) - end - - it "tax applies to line item" do - subject.update - line_item.reload - # Taxable amount is: $20 (base) - $10 (promotion) = $10 - # Tax rate is 5% (of $10). - expect(line_item.included_tax_total).to eq(0) - expect(line_item.additional_tax_total).to eq(0.5) - expect(line_item.promo_total).to eq(-10) - expect(line_item.adjustment_total).to eq(-9.5) - end - - it "tax linked to order" do - order.update! - expect(order.included_tax_total).to eq(0) - expect(order.additional_tax_total).to eq(0.5) - end - end - end - - context "promotion chooser customization" do - before do - class Spree::TestPromotionChooser - def initialize(_adjustments) - raise "Custom promotion chooser" - end - end - - Spree::Config.promotion_chooser_class = Spree::TestPromotionChooser - end - - it "uses the defined promotion chooser" do - expect { subject.update }.to raise_error("Custom promotion chooser") - end - end - - context "default promotion chooser (best promotion is always applied)" do - let(:calculator) { Calculator::FlatRate.new(preferred_amount: 10) } - - let(:source) do - Promotion::Actions::CreateItemAdjustments.create!( - calculator: calculator, - promotion: promotion - ) - end - let(:promotion) { create(:promotion) } - - def create_adjustment(label, amount) - create(:adjustment, order: order, - adjustable: line_item, - source: source, - amount: amount, - finalized: true, - label: label) - end - - it "should make all but the most valuable promotion adjustment ineligible, leaving non promotion adjustments alone" do - create_adjustment("Promotion A", -100) - create_adjustment("Promotion B", -200) - create_adjustment("Promotion C", -300) - create(:adjustment, order: order, - adjustable: line_item, - source: nil, - amount: -500, - finalized: true, - label: "Some other credit") - line_item.adjustments.each { |a| a.update_column(:eligible, true) } - - subject.update - - expect(line_item.adjustments.promotion.eligible.count).to eq(1) - expect(line_item.adjustments.promotion.eligible.first.label).to eq('Promotion C') - end - - it "should choose the most recent promotion adjustment when amounts are equal" do - # Using Timecop is a regression test - Timecop.freeze do - create_adjustment("Promotion A", -200) - create_adjustment("Promotion B", -200) - end - line_item.adjustments.each { |a| a.update_column(:eligible, true) } - - subject.update - - expect(line_item.adjustments.promotion.eligible.count).to eq(1) - expect(line_item.adjustments.promotion.eligible.first.label).to eq('Promotion B') - end - - it "should choose the most recent promotion adjustment when amounts are equal" do - # Using Timecop is a regression test - Timecop.freeze do - create_adjustment("Promotion A", -200) - create_adjustment("Promotion B", -200) - end - line_item.adjustments.each { |a| a.update_column(:eligible, true) } - - subject.update - - expect(line_item.adjustments.promotion.eligible.count).to eq(1) - expect(line_item.adjustments.promotion.eligible.first.label).to eq('Promotion B') - end - - context "when previously ineligible promotions become available" do - let(:order_promo1) { create(:promotion, :with_order_adjustment, :with_item_total_rule, weighted_order_adjustment_amount: 5, item_total_threshold_amount: 10) } - let(:order_promo2) { create(:promotion, :with_order_adjustment, :with_item_total_rule, weighted_order_adjustment_amount: 10, item_total_threshold_amount: 20) } - let(:order_promos) { [order_promo1, order_promo2] } - let(:line_item_promo1) { create(:promotion, :with_line_item_adjustment, :with_item_total_rule, adjustment_rate: 2.5, item_total_threshold_amount: 10, apply_automatically: true) } - let(:line_item_promo2) { create(:promotion, :with_line_item_adjustment, :with_item_total_rule, adjustment_rate: 5, item_total_threshold_amount: 20, apply_automatically: true) } - let(:line_item_promos) { [line_item_promo1, line_item_promo2] } - let(:order) { create(:order_with_line_items, line_items_count: 1) } - - # Apply promotions in different sequences. Results should be the same. - promo_sequences = [ - [0, 1], - [1, 0] - ] - - promo_sequences.each do |promo_sequence| - it "should pick the best order-level promo according to current eligibility" do - # apply both promos to the order, even though only promo1 is eligible - order_promos[promo_sequence[0]].activate order: order - order_promos[promo_sequence[1]].activate order: order - - order.update! - order.reload - expect(order.all_adjustments.count).to eq(2), "Expected two adjustments (using sequence #{promo_sequence})" - expect(order.all_adjustments.eligible.count).to eq(1), "Expected one elegible adjustment (using sequence #{promo_sequence})" - expect(order.all_adjustments.eligible.first.source.promotion).to eq(order_promo1), "Expected promo1 to be used (using sequence #{promo_sequence})" - - order.contents.add create(:variant, price: 10), 1 - order.save - - order.reload - expect(order.all_adjustments.count).to eq(2), "Expected two adjustments (using sequence #{promo_sequence})" - expect(order.all_adjustments.eligible.count).to eq(1), "Expected one elegible adjustment (using sequence #{promo_sequence})" - expect(order.all_adjustments.eligible.first.source.promotion).to eq(order_promo2), "Expected promo2 to be used (using sequence #{promo_sequence})" - end - end - - promo_sequences.each do |promo_sequence| - it "should pick the best line-item-level promo according to current eligibility" do - # apply both promos to the order, even though only promo1 is eligible - line_item_promos[promo_sequence[0]].activate order: order - line_item_promos[promo_sequence[1]].activate order: order - - order.reload - expect(order.all_adjustments.count).to eq(1), "Expected one adjustment (using sequence #{promo_sequence})" - expect(order.all_adjustments.eligible.count).to eq(1), "Expected one elegible adjustment (using sequence #{promo_sequence})" - # line_item_promo1 is the only one that has thus far met the order total threshold, it is the only promo which should be applied. - expect(order.all_adjustments.first.source.promotion).to eq(line_item_promo1), "Expected line_item_promo1 to be used (using sequence #{promo_sequence})" - - order.contents.add create(:variant, price: 10), 1 - order.save - - order.reload - expect(order.all_adjustments.count).to eq(4), "Expected four adjustments (using sequence #{promo_sequence})" - expect(order.all_adjustments.eligible.count).to eq(2), "Expected two elegible adjustments (using sequence #{promo_sequence})" - order.all_adjustments.eligible.each do |adjustment| - expect(adjustment.source.promotion).to eq(line_item_promo2), "Expected line_item_promo2 to be used (using sequence #{promo_sequence})" - end - end - end - end - - context "multiple adjustments and the best one is not eligible" do - let!(:promo_a) { create_adjustment("Promotion A", -100) } - let!(:promo_c) { create_adjustment("Promotion C", -300) } - - before do - promo_a.update_column(:eligible, true) - promo_c.update_column(:eligible, false) - end - - # regression for https://github.com/spree/spree/issues/3274 - it "still makes the previous best eligible adjustment valid" do - subject.update - expect(line_item.adjustments.promotion.eligible.first.label).to eq('Promotion A') - end - end - - it "should only leave one adjustment even if 2 have the same amount" do - create_adjustment("Promotion A", -100) - create_adjustment("Promotion B", -200) - create_adjustment("Promotion C", -200) - - subject.update - - expect(line_item.adjustments.promotion.eligible.count).to eq(1) - expect(line_item.adjustments.promotion.eligible.first.amount.to_i).to eq(-200) - end - end - - 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 - end - - # "fresh" record from the DB - def db_record - Spree::LineItem.find(item.id) - end - - it "persists each change" do - source.update_attributes!(amount: 0.1) - update - expect(item).not_to be_changed - expect(db_record).to have_attributes(adjustment_total: 1) - - source.update_attributes!(amount: 0.20) - update - expect(item).not_to be_changed - expect(db_record).to have_attributes(adjustment_total: 2) - - source.update_attributes!(amount: 0.10) - update - expect(item).not_to be_changed - expect(db_record).to have_attributes(adjustment_total: 1) - end - end - end -end diff --git a/core/spec/models/spree/order_updater_spec.rb b/core/spec/models/spree/order_updater_spec.rb index 5973adf5465..5cd8750e29f 100644 --- a/core/spec/models/spree/order_updater_spec.rb +++ b/core/spec/models/spree/order_updater_spec.rb @@ -67,6 +67,245 @@ module Spree end end + describe '#recalculate_adjustments ' do + describe 'promotion recalculation' do + let(:order) { create(:order_with_line_items, line_items_count: 1, line_items_price: 10) } + let(:line_item) { order.line_items[0] } + + context 'when the item quantity has changed' do + let(:promotion) { create(:promotion, promotion_actions: [promotion_action]) } + let(:promotion_action) { Spree::Promotion::Actions::CreateItemAdjustments.new(calculator: calculator) } + let(:calculator) { Spree::Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10) } + + before do + promotion.activate(order: order) + order.update! + line_item.update!(quantity: 2) + end + + it 'updates the promotion amount' do + expect { + order.update! + }.to change { + line_item.promo_total + }.from(-1).to(-2) + end + end + + context 'promotion chooser customization' do + before do + class Spree::TestPromotionChooser + def initialize(_adjustments) + raise 'Custom promotion chooser' + end + end + + Spree::Config.promotion_chooser_class = Spree::TestPromotionChooser + end + + it 'uses the defined promotion chooser' do + expect { order.update! }.to raise_error('Custom promotion chooser') + end + end + + context 'default promotion chooser (best promotion is always applied)' do + let(:calculator) { Calculator::FlatRate.new(preferred_amount: 10) } + + let(:source) do + Promotion::Actions::CreateItemAdjustments.create!( + calculator: calculator, + promotion: promotion, + ) + end + let(:promotion) { create(:promotion) } + + def create_adjustment(label, amount) + create( + :adjustment, + order: order, + adjustable: line_item, + source: source, + amount: amount, + finalized: true, + label: label, + ) + end + + it 'should make all but the most valuable promotion adjustment ineligible, leaving non promotion adjustments alone' do + create_adjustment('Promotion A', -100) + create_adjustment('Promotion B', -200) + create_adjustment('Promotion C', -300) + create(:adjustment, order: order, + adjustable: line_item, + source: nil, + amount: -500, + finalized: true, + label: 'Some other credit') + line_item.adjustments.each { |a| a.update_column(:eligible, true) } + + order.update! + + expect(line_item.adjustments.promotion.eligible.count).to eq(1) + expect(line_item.adjustments.promotion.eligible.first.label).to eq('Promotion C') + end + + it 'should choose the most recent promotion adjustment when amounts are equal' do + # Using Timecop is a regression test + Timecop.freeze do + create_adjustment('Promotion A', -200) + create_adjustment('Promotion B', -200) + end + line_item.adjustments.each { |a| a.update_column(:eligible, true) } + + order.update! + + expect(line_item.adjustments.promotion.eligible.count).to eq(1) + expect(line_item.adjustments.promotion.eligible.first.label).to eq('Promotion B') + end + + it 'should choose the most recent promotion adjustment when amounts are equal' do + # Using Timecop is a regression test + Timecop.freeze do + create_adjustment('Promotion A', -200) + create_adjustment('Promotion B', -200) + end + line_item.adjustments.each { |a| a.update_column(:eligible, true) } + + order.update! + + expect(line_item.adjustments.promotion.eligible.count).to eq(1) + expect(line_item.adjustments.promotion.eligible.first.label).to eq('Promotion B') + end + + context 'when previously ineligible promotions become available' do + let(:order_promo1) { create(:promotion, :with_order_adjustment, :with_item_total_rule, weighted_order_adjustment_amount: 5, item_total_threshold_amount: 10) } + let(:order_promo2) { create(:promotion, :with_order_adjustment, :with_item_total_rule, weighted_order_adjustment_amount: 10, item_total_threshold_amount: 20) } + let(:order_promos) { [order_promo1, order_promo2] } + let(:line_item_promo1) { create(:promotion, :with_line_item_adjustment, :with_item_total_rule, adjustment_rate: 2.5, item_total_threshold_amount: 10, apply_automatically: true) } + let(:line_item_promo2) { create(:promotion, :with_line_item_adjustment, :with_item_total_rule, adjustment_rate: 5, item_total_threshold_amount: 20, apply_automatically: true) } + let(:line_item_promos) { [line_item_promo1, line_item_promo2] } + let(:order) { create(:order_with_line_items, line_items_count: 1) } + + # Apply promotions in different sequences. Results should be the same. + promo_sequences = [ + [0, 1], + [1, 0], + ] + + promo_sequences.each do |promo_sequence| + context "with promo sequence #{promo_sequence}" do + it 'should pick the best order-level promo according to current eligibility' do + # apply both promos to the order, even though only promo1 is eligible + order_promos[promo_sequence[0]].activate order: order + order_promos[promo_sequence[1]].activate order: order + + order.update! + order.reload + expect(order.all_adjustments.count).to eq(2), 'Expected two adjustments' + expect(order.all_adjustments.eligible.count).to eq(1), 'Expected one elegible adjustment' + expect(order.all_adjustments.eligible.first.source.promotion).to eq(order_promo1), 'Expected promo1 to be used' + + order.contents.add create(:variant, price: 10), 1 + order.save + + order.reload + expect(order.all_adjustments.count).to eq(2), 'Expected two adjustments' + expect(order.all_adjustments.eligible.count).to eq(1), 'Expected one elegible adjustment' + expect(order.all_adjustments.eligible.first.source.promotion).to eq(order_promo2), 'Expected promo2 to be used' + end + end + end + + promo_sequences.each do |promo_sequence| + context 'with promo sequence #{promo_sequence}' do + it 'should pick the best line-item-level promo according to current eligibility' do + # apply both promos to the order, even though only promo1 is eligible + line_item_promos[promo_sequence[0]].activate order: order + line_item_promos[promo_sequence[1]].activate order: order + + order.reload + expect(order.all_adjustments.count).to eq(1), 'Expected one adjustment' + expect(order.all_adjustments.eligible.count).to eq(1), 'Expected one elegible adjustment' + # line_item_promo1 is the only one that has thus far met the order total threshold, it is the only promo which should be applied. + expect(order.all_adjustments.first.source.promotion).to eq(line_item_promo1), 'Expected line_item_promo1 to be used' + + order.contents.add create(:variant, price: 10), 1 + order.save + + order.reload + expect(order.all_adjustments.count).to eq(4), 'Expected four adjustments' + expect(order.all_adjustments.eligible.count).to eq(2), 'Expected two elegible adjustments' + order.all_adjustments.eligible.each do |adjustment| + expect(adjustment.source.promotion).to eq(line_item_promo2), 'Expected line_item_promo2 to be used' + end + end + end + end + end + + context 'multiple adjustments and the best one is not eligible' do + let!(:promo_a) { create_adjustment('Promotion A', -100) } + let!(:promo_c) { create_adjustment('Promotion C', -300) } + + before do + promo_a.update_column(:eligible, true) + promo_c.update_column(:eligible, false) + end + + # regression for https://github.com/spree/spree/issues/3274 + it 'still makes the previous best eligible adjustment valid' do + order.update! + expect(line_item.adjustments.promotion.eligible.first.label).to eq('Promotion A') + end + end + + it 'should only leave one adjustment even if 2 have the same amount' do + create_adjustment('Promotion A', -100) + create_adjustment('Promotion B', -200) + create_adjustment('Promotion C', -200) + + order.update! + + expect(line_item.adjustments.promotion.eligible.count).to eq(1) + expect(line_item.adjustments.promotion.eligible.first.amount.to_i).to eq(-200) + end + 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_category: tax_category) } + + let(:order) do + create( + :order_with_line_items, + line_items_count: 1, + 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.update! + }.to change { + line_item.additional_tax_total + }.from(1).to(2) + end + end + end + end + context "updating shipment state" do before do allow(order).to receive_messages backordered?: false From bdf3254433141ad491eabee784e5903d8e3ed4d2 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Thu, 22 Sep 2016 21:35:13 -0600 Subject: [PATCH 2/6] Don't include shipments in item cancellations Shipments can't have item cancellations. --- core/app/models/spree/order_updater.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index 200f6f01612..e505fa64645 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -185,8 +185,8 @@ def update_taxes end def update_cancellations - [*line_items, *shipments].each do |item| - item.adjustments.select(&:cancellation?).each(&:update!) + line_items.each do |line_item| + line_item.adjustments.select(&:cancellation?).each(&:update!) end end From 0c084596847cee4a1d2e85118c2070df70818750 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Thu, 22 Sep 2016 21:39:33 -0600 Subject: [PATCH 3/6] Add comments and split apart updating order & line item promos Bring in a bunch of helpful comments that were in the ItemAdjustments class. And split apart update_item_promotions and update_order_promotions to make the code clearer since they are handled slightly differently. --- core/app/models/spree/order_updater.rb | 35 +++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index e505fa64645..ec4bd732e59 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -30,8 +30,19 @@ def run_hooks update_hooks.each { |hook| order.send hook } 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 recalculate_adjustments - update_promotions + # 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_item_promotions + update_order_promotions update_taxes update_cancellations update_item_totals @@ -164,21 +175,39 @@ def round_money(n) (n * 100).round / 100.0 end - def update_promotions - [*line_items, *shipments, order].each do |item| + def update_item_promotions + [*line_items, *shipments].each do |item| promotion_adjustments = item.adjustments.select(&:promotion?) promotion_adjustments.each(&:update!) Spree::Config.promotion_chooser_class.new(promotion_adjustments).update + item.promo_total = promotion_adjustments.select(&:eligible?).sum(&:amount) end end + # Update and select the best promotion adjustment for the order. + # We don't update the order.promo_total yet. Order totals are updated later + # in #update_adjustment_total since they include the totals from the ordre's + # line items and/or shipments. + def update_order_promotions + promotion_adjustments = order.adjustments.select(&:promotion?) + promotion_adjustments.each(&:update!) + Spree::Config.promotion_chooser_class.new(promotion_adjustments).update + end + def update_taxes [*line_items, *shipments].each do |item| tax_adjustments = item.adjustments.select(&:tax?) tax_adjustments.each(&:update!) + # Tax adjustments come in not one but *two* exciting flavours: + # Included & additional + + # 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) end From ba17bba5e5eea9765593001e8f4c5959953e5543 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Thu, 22 Sep 2016 21:52:05 -0600 Subject: [PATCH 4/6] Group two identical loops in order_updater_spec Also fixes single quotes that were prevent interpolation. --- core/spec/models/spree/order_updater_spec.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/core/spec/models/spree/order_updater_spec.rb b/core/spec/models/spree/order_updater_spec.rb index 5cd8750e29f..f6dc1dff4dd 100644 --- a/core/spec/models/spree/order_updater_spec.rb +++ b/core/spec/models/spree/order_updater_spec.rb @@ -213,11 +213,7 @@ def create_adjustment(label, amount) expect(order.all_adjustments.eligible.count).to eq(1), 'Expected one elegible adjustment' expect(order.all_adjustments.eligible.first.source.promotion).to eq(order_promo2), 'Expected promo2 to be used' end - end - end - promo_sequences.each do |promo_sequence| - context 'with promo sequence #{promo_sequence}' do it 'should pick the best line-item-level promo according to current eligibility' do # apply both promos to the order, even though only promo1 is eligible line_item_promos[promo_sequence[0]].activate order: order From 8d99136c531e082307f66174f80f71e44cae1445 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Thu, 22 Sep 2016 21:53:49 -0600 Subject: [PATCH 5/6] Add comment --- core/app/models/spree/order_updater.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index ec4bd732e59..a864d925796 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -221,6 +221,8 @@ def update_cancellations def update_item_totals [*line_items, *shipments].each do |item| + # The cancellation_total isn't persisted anywhere but is included in + # the adjustment_total item_cancellation_total = item.adjustments.select(&:cancellation?).sum(&:amount) item.adjustment_total = item.promo_total + From 36fc4e44fff249184ba29ec20dbe8d273578553b Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Sat, 24 Sep 2016 07:18:25 -0600 Subject: [PATCH 6/6] Fix typo in comment --- core/app/models/spree/order_updater.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index a864d925796..38a7c70158a 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -188,7 +188,7 @@ def update_item_promotions # Update and select the best promotion adjustment for the order. # We don't update the order.promo_total yet. Order totals are updated later - # in #update_adjustment_total since they include the totals from the ordre's + # in #update_adjustment_total since they include the totals from the order's # line items and/or shipments. def update_order_promotions promotion_adjustments = order.adjustments.select(&:promotion?)