From b4a7ccf1b407aa568bbb4746595df9914dee44e9 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 9 Aug 2013 14:14:48 +1000 Subject: [PATCH] Adjustments on LineItems don't modify the order total. Make adjustments on Order instead. --- app/models/product_distribution.rb | 20 +++--- app/models/spree/adjustment_decorator.rb | 5 ++ app/models/spree/order_decorator.rb | 6 +- spec/models/order_spec.rb | 5 -- spec/models/product_distribution_spec.rb | 81 ++++++++++++++---------- 5 files changed, 66 insertions(+), 51 deletions(-) create mode 100644 app/models/spree/adjustment_decorator.rb diff --git a/app/models/product_distribution.rb b/app/models/product_distribution.rb index 42354ff636..7ef2a90cdf 100644 --- a/app/models/product_distribution.rb +++ b/app/models/product_distribution.rb @@ -11,29 +11,29 @@ class ProductDistribution < ActiveRecord::Base def ensure_correct_adjustment_for(line_item) if enterprise_fee - clear_all_enterprise_fee_adjustments_on line_item - create_adjustment_on line_item + clear_all_enterprise_fee_adjustments_for line_item + create_adjustment_for line_item end end - def adjustment_on(line_item) - adjustments = line_item.adjustments.where(originator_id: enterprise_fee) + def adjustment_for(line_item) + adjustments = line_item.order.adjustments.enterprise_fee.where(originator_id: enterprise_fee) raise "Multiple adjustments for this enterprise fee on this line item. This method is not designed to deal with this scenario." if adjustments.count > 1 adjustments.first end - def create_adjustment_on(line_item) - enterprise_fee.create_adjustment(adjustment_label, line_item, line_item, true) + def create_adjustment_for(line_item) + enterprise_fee.create_adjustment(adjustment_label_for(line_item), line_item.order, line_item, true) end - def clear_all_enterprise_fee_adjustments_on(line_item) - line_item.adjustments.where(originator_type: 'EnterpriseFee').destroy_all + def clear_all_enterprise_fee_adjustments_for(line_item) + line_item.order.adjustments.where(originator_type: 'EnterpriseFee', source_id: line_item, source_type: 'Spree::LineItem').destroy_all end - def adjustment_label - "Product distribution by #{distributor.name}" + def adjustment_label_for(line_item) + "Product distribution by #{distributor.name} for #{line_item.product.name}" end end diff --git a/app/models/spree/adjustment_decorator.rb b/app/models/spree/adjustment_decorator.rb new file mode 100644 index 0000000000..6d089ee3b3 --- /dev/null +++ b/app/models/spree/adjustment_decorator.rb @@ -0,0 +1,5 @@ +module Spree + Adjustment.class_eval do + scope :enterprise_fee, where(originator_type: 'EnterpriseFee') + end +end diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index f5d05b429c..290fb45a83 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -1,5 +1,9 @@ require 'open_food_web/distribution_change_validator' +ActiveSupport::Notifications.subscribe('spree.order.contents_changed') do |name, start, finish, id, payload| + payload[:order].reload.update_distribution_charge! +end + Spree::Order.class_eval do belongs_to :order_cycle belongs_to :distributor, :class_name => 'Enterprise' @@ -13,8 +17,6 @@ Spree::Order.class_eval do before_save :update_line_item_shipping_methods after_create :set_default_shipping_method - register_update_hook :update_distribution_charge! - # -- Scopes scope :managed_by, lambda { |user| if user.has_spree_role?('admin') diff --git a/spec/models/order_spec.rb b/spec/models/order_spec.rb index 016b23099b..eb3a968a5c 100644 --- a/spec/models/order_spec.rb +++ b/spec/models/order_spec.rb @@ -31,11 +31,6 @@ describe Spree::Order do describe "updating the distribution charge" do let(:order) { build(:order) } - it "updates distribution charge after save" do - order.should_receive(:update_distribution_charge!).at_least(:once) - order.save! - end - it "ensures the correct adjustment(s) are created for the product distribution" do line_item = double(:line_item) subject.stub(:line_items) { [line_item] } diff --git a/spec/models/product_distribution_spec.rb b/spec/models/product_distribution_spec.rb index e649b3918a..a1f92d2cb3 100644 --- a/spec/models/product_distribution_spec.rb +++ b/spec/models/product_distribution_spec.rb @@ -30,7 +30,7 @@ describe ProductDistribution do order = create(:order, distributor: distributor) # And a product with a product distribution with an enterprise fee - product = create(:product) + product = create(:product, name: 'Pear') enterprise_fee = create(:enterprise_fee, calculator: build(:calculator)) enterprise_fee.calculator.preferred_amount = 1.23 enterprise_fee.calculator.save! @@ -40,17 +40,20 @@ describe ProductDistribution do expect do op = Spree::OrderPopulator.new order, 'AU' op.populate products: {product.id => product.master.id}, quantity: 1, distributor_id: distributor.id + + # Normally the controller would fire this event when the order's contents are changed + fire_order_contents_changed_event(order.user, order) end.to change(Spree::Adjustment, :count).by(1) # And it should have the correct data order.reload - order.line_items.count.should == 1 - order.line_items.last.adjustments.count.should == 1 - adjustment = order.line_items.last.adjustments.last + adjustments = order.adjustments.where(:originator_type => 'EnterpriseFee') + adjustments.count.should == 1 + adjustment = adjustments.first adjustment.source.should == order.line_items.last adjustment.originator.should == enterprise_fee - adjustment.label.should == "Product distribution by #{distributor.name}" + adjustment.label.should == "Product distribution by #{distributor.name} for Pear" adjustment.amount.should == 1.23 # And it should have some associated metadata @@ -67,102 +70,104 @@ describe ProductDistribution do # TODO: This spec will go away once enterprise_fee is required it "does nothing if there is no enterprise fee set" do pd.enterprise_fee = nil - pd.should_receive(:adjustment_on).never + pd.should_receive(:clear_all_enterprise_fee_adjustments_for).never + pd.should_receive(:create_adjustment_for).never pd.ensure_correct_adjustment_for line_item end describe "adding items to cart" do it "clears all enterprise fee adjustments on the line item" do - pd.should_receive(:clear_all_enterprise_fee_adjustments_on).with(line_item) - pd.stub(:create_adjustment_on) + pd.should_receive(:clear_all_enterprise_fee_adjustments_for).with(line_item) + pd.stub(:create_adjustment_for) pd.ensure_correct_adjustment_for line_item end it "creates an adjustment on the line item" do - pd.stub(:clear_all_enterprise_fee_adjustments_on) - pd.should_receive(:create_adjustment_on).with(line_item) + pd.stub(:clear_all_enterprise_fee_adjustments_for) + pd.should_receive(:create_adjustment_for).with(line_item) pd.ensure_correct_adjustment_for line_item end end describe "changing distributor" do - it "clears and re-creates the adjustment on the line item" do + it "clears and re-creates the adjustment for the line item" do # Given a line item with an adjustment via one enterprise fee p = create(:simple_product) d1, d2 = create(:distributor_enterprise), create(:distributor_enterprise) pd1 = create(:product_distribution, product: p, distributor: d1) pd2 = create(:product_distribution, product: p, distributor: d2) line_item = create(:line_item, product: p) - pd1.enterprise_fee.create_adjustment('foo', line_item, line_item, true) + pd1.enterprise_fee.create_adjustment('foo', line_item.order, line_item, true) # When I ensure correct adjustment through the other product distribution pd2.ensure_correct_adjustment_for line_item # Then I should have only an adjustment originating from the other product distribution - line_item.reload - line_item.adjustments.count.should == 1 - line_item.adjustments.first.originator.should == pd2.enterprise_fee + line_item.order.reload + adjustments = line_item.order.adjustments.enterprise_fee + adjustments.count.should == 1 + adjustments.first.originator.should == pd2.enterprise_fee end end end - describe "finding our adjustment on a line item" do + describe "finding our adjustment for a line item" do it "returns nil when not present" do line_item = build(:line_item) pd = ProductDistribution.new - pd.send(:adjustment_on, line_item).should be_nil + pd.send(:adjustment_for, line_item).should be_nil end it "returns the adjustment when present" do pd = create(:product_distribution) line_item = create(:line_item) - adjustment = pd.enterprise_fee.create_adjustment('foo', line_item, line_item, true) + adjustment = pd.enterprise_fee.create_adjustment('foo', line_item.order, line_item, true) - pd.send(:adjustment_on, line_item).should == adjustment + pd.send(:adjustment_for, line_item).should == adjustment end it "raises an error when there are multiple adjustments for this enterprise fee" do pd = create(:product_distribution) line_item = create(:line_item) - pd.enterprise_fee.create_adjustment('one', line_item, line_item, true) - pd.enterprise_fee.create_adjustment('two', line_item, line_item, true) + pd.enterprise_fee.create_adjustment('one', line_item.order, line_item, true) + pd.enterprise_fee.create_adjustment('two', line_item.order, line_item, true) expect do - pd.send(:adjustment_on, line_item) + pd.send(:adjustment_for, line_item) end.to raise_error "Multiple adjustments for this enterprise fee on this line item. This method is not designed to deal with this scenario." end end - describe "creating an adjustment on a line item" do + describe "creating an adjustment for a line item" do it "creates the adjustment via the enterprise fee" do pd = create(:product_distribution) - pd.stub(:adjustment_label) { 'label' } + pd.stub(:adjustment_label_for) { 'label' } line_item = create(:line_item) - expect { pd.send(:create_adjustment_on, line_item) }.to change(Spree::Adjustment, :count).by(1) + expect { pd.send(:create_adjustment_for, line_item) }.to change(Spree::Adjustment, :count).by(1) adjustment = Spree::Adjustment.last adjustment.label.should == 'label' - adjustment.adjustable.should == line_item + adjustment.adjustable.should == line_item.order adjustment.source.should == line_item adjustment.originator.should == pd.enterprise_fee adjustment.should be_mandatory end end - describe "clearing all enterprise fee adjustments on a line item" do + describe "clearing all enterprise fee adjustments for a line item" do it "clears adjustments originating from many different enterprise fees" do p = create(:simple_product) d1, d2 = create(:distributor_enterprise), create(:distributor_enterprise) pd1 = create(:product_distribution, product: p, distributor: d1) pd2 = create(:product_distribution, product: p, distributor: d2) line_item = create(:line_item, product: p) - pd1.enterprise_fee.create_adjustment('foo1', line_item, line_item, true) - pd2.enterprise_fee.create_adjustment('foo2', line_item, line_item, true) + pd1.enterprise_fee.create_adjustment('foo1', line_item.order, line_item, true) + pd2.enterprise_fee.create_adjustment('foo2', line_item.order, line_item, true) expect do - pd1.send(:clear_all_enterprise_fee_adjustments_on, line_item) - end.to change(line_item.adjustments, :count).by(-2) + pd1.send(:clear_all_enterprise_fee_adjustments_for, line_item) + end.to change(line_item.order.adjustments, :count).by(-2) end it "does not clear adjustments originating from another source" do @@ -170,12 +175,20 @@ describe ProductDistribution do pd = create(:product_distribution) line_item = create(:line_item, product: pd.product) tax_rate = create(:tax_rate, calculator: build(:calculator, preferred_amount: 10)) - tax_rate.create_adjustment('foo', line_item, line_item) + tax_rate.create_adjustment('foo', line_item.order, line_item) expect do - pd.send(:clear_all_enterprise_fee_adjustments_on, line_item) - end.to change(line_item.adjustments, :count).by(0) + pd.send(:clear_all_enterprise_fee_adjustments_for, line_item) + end.to change(line_item.order.adjustments, :count).by(0) end end end + + + private + + def fire_order_contents_changed_event(user, order) + ActiveSupport::Notifications.instrument('spree.order.contents_changed', {user: user, order: order}) + end + end