From 08009d40205f454b1de7da10d892527f69111cb3 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 21 Feb 2014 15:40:54 +1100 Subject: [PATCH 01/11] Extract application of enterprise fees as adjustments into its own class --- app/models/exchange.rb | 4 ++ app/models/order_cycle.rb | 26 +++-------- .../enterprise_fee_applicator.rb | 16 +++++++ .../enterprise_fee_applicator_spec.rb | 37 ++++++++++++++++ spec/models/exchange_spec.rb | 14 ++++++ spec/models/order_cycle_spec.rb | 44 ++++--------------- 6 files changed, 86 insertions(+), 55 deletions(-) create mode 100644 lib/open_food_network/enterprise_fee_applicator.rb create mode 100644 spec/lib/open_food_network/enterprise_fee_applicator_spec.rb diff --git a/app/models/exchange.rb b/app/models/exchange.rb index 0f8b472443..ce698c1cd0 100644 --- a/app/models/exchange.rb +++ b/app/models/exchange.rb @@ -35,6 +35,10 @@ class Exchange < ActiveRecord::Base receiver == order_cycle.coordinator end + def role + incoming? ? 'supplier' : 'distributor' + end + def to_h(core=false) h = attributes.merge({ 'variant_ids' => variant_ids.sort, 'enterprise_fee_ids' => enterprise_fee_ids.sort }) h.reject! { |k| %w(id order_cycle_id created_at updated_at).include? k } if core diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index d710befc10..1dbdbbed18 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -1,3 +1,5 @@ +require 'open_food_network/enterprise_fee_applicator' + class OrderCycle < ActiveRecord::Base belongs_to :coordinator, :class_name => 'Enterprise' has_and_belongs_to_many :coordinator_fees, :class_name => 'EnterpriseFee', :join_table => 'coordinator_fees' @@ -144,12 +146,12 @@ class OrderCycle < ActiveRecord::Base # -- Fees def fees_for(variant, distributor) - enterprise_fees_for(variant, distributor).sum do |fee| + enterprise_fees_for(variant, distributor).sum do |applicator| # Spree's Calculator interface accepts Orders or LineItems, # so we meet that interface with a struct. # Amount is faked, this is a method on LineItem line_item = OpenStruct.new variant: variant, quantity: 1, amount: (variant.price) - fee[:enterprise_fee].compute_amount(line_item) + applicator.enterprise_fee.compute_amount(line_item) end end @@ -157,7 +159,7 @@ class OrderCycle < ActiveRecord::Base variant = line_item.variant distributor = line_item.order.distributor - enterprise_fees_for(variant, distributor).each { |fee| create_adjustment_for_fee line_item, fee[:enterprise_fee], fee[:label], fee[:role] } + enterprise_fees_for(variant, distributor).each { |applicator| applicator.create_line_item_adjustment(line_item) } end private @@ -168,31 +170,17 @@ class OrderCycle < ActiveRecord::Base exchanges_carrying(variant, distributor).each do |exchange| exchange.enterprise_fees.each do |enterprise_fee| - role = exchange.incoming? ? 'supplier' : 'distributor' - fees << {enterprise_fee: enterprise_fee, - label: adjustment_label_for(variant, enterprise_fee, role), - role: role} + fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, variant, exchange.role) end end coordinator_fees.each do |enterprise_fee| - fees << {enterprise_fee: enterprise_fee, - label: adjustment_label_for(variant, enterprise_fee, 'coordinator'), - role: 'coordinator'} + fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, variant, 'coordinator') end fees end - def create_adjustment_for_fee(line_item, enterprise_fee, label, role) - a = enterprise_fee.create_locked_adjustment(label, line_item.order, line_item, true) - AdjustmentMetadata.create! adjustment: a, enterprise: enterprise_fee.enterprise, fee_name: enterprise_fee.name, fee_type: enterprise_fee.fee_type, enterprise_role: role - end - - def adjustment_label_for(variant, enterprise_fee, role) - "#{variant.product.name} - #{enterprise_fee.fee_type} fee by #{role} #{enterprise_fee.enterprise.name}" - end - def exchanges_carrying(variant, distributor) exchanges.to_enterprises([coordinator, distributor]).with_variant(variant) end diff --git a/lib/open_food_network/enterprise_fee_applicator.rb b/lib/open_food_network/enterprise_fee_applicator.rb new file mode 100644 index 0000000000..c2b3001bca --- /dev/null +++ b/lib/open_food_network/enterprise_fee_applicator.rb @@ -0,0 +1,16 @@ +module OpenFoodNetwork + class EnterpriseFeeApplicator < Struct.new(:enterprise_fee, :variant, :role) + def create_line_item_adjustment(line_item) + a = enterprise_fee.create_locked_adjustment(adjustment_label, line_item.order, line_item, true) + AdjustmentMetadata.create! adjustment: a, enterprise: enterprise_fee.enterprise, fee_name: enterprise_fee.name, fee_type: enterprise_fee.fee_type, enterprise_role: role + end + + + private + + def adjustment_label + "#{variant.product.name} - #{enterprise_fee.fee_type} fee by #{role} #{enterprise_fee.enterprise.name}" + end + end +end + diff --git a/spec/lib/open_food_network/enterprise_fee_applicator_spec.rb b/spec/lib/open_food_network/enterprise_fee_applicator_spec.rb new file mode 100644 index 0000000000..345eae845c --- /dev/null +++ b/spec/lib/open_food_network/enterprise_fee_applicator_spec.rb @@ -0,0 +1,37 @@ +require 'open_food_network/enterprise_fee_applicator' + +module OpenFoodNetwork + describe EnterpriseFeeApplicator do + it "creates an adjustment for a line item" do + line_item = create(:line_item) + enterprise_fee = create(:enterprise_fee) + product = create(:simple_product) + + efa = EnterpriseFeeApplicator.new enterprise_fee, product.master, 'role' + efa.stub(:adjustment_label) { 'label' } + efa.create_line_item_adjustment line_item + + adjustment = Spree::Adjustment.last + adjustment.label.should == 'label' + adjustment.adjustable.should == line_item.order + adjustment.source.should == line_item + adjustment.originator.should == enterprise_fee + adjustment.should be_mandatory + + md = adjustment.metadata + md.enterprise.should == enterprise_fee.enterprise + md.fee_name.should == enterprise_fee.name + md.fee_type.should == enterprise_fee.fee_type + md.enterprise_role.should == 'role' + end + + it "makes adjustment labels" do + variant = double(:variant, product: double(:product, name: 'Bananas')) + enterprise_fee = double(:enterprise_fee, fee_type: 'packing', enterprise: double(:enterprise, name: 'Ballantyne')) + + efa = EnterpriseFeeApplicator.new enterprise_fee, variant, 'distributor' + + efa.send(:adjustment_label).should == "Bananas - packing fee by distributor Ballantyne" + end + end +end diff --git a/spec/models/exchange_spec.rb b/spec/models/exchange_spec.rb index f4ebfc2d8a..df26d20ac0 100644 --- a/spec/models/exchange_spec.rb +++ b/spec/models/exchange_spec.rb @@ -62,6 +62,20 @@ describe Exchange do end end + describe "reporting its role" do + it "returns 'supplier' when it is an incoming exchange" do + e = Exchange.new + e.stub(:incoming?) { true } + e.role.should == 'supplier' + end + + it "returns 'distributor' when it is an outgoing exchange" do + e = Exchange.new + e.stub(:incoming?) { false } + e.role.should == 'distributor' + end + end + describe "scopes" do let(:supplier) { create(:supplier_enterprise) } let(:coordinator) { create(:distributor_enterprise) } diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index bd8d835d91..36480ca74b 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -348,9 +348,9 @@ describe OrderCycle do let(:line_item) { double(:line_item, variant: variant, order: order) } it "creates adjustment for each fee" do - fee = {enterprise_fee: 'ef', label: 'label', role: 'role'} - oc.should_receive(:enterprise_fees_for).with(variant, distributor) { [fee] } - oc.should_receive(:create_adjustment_for_fee).with(line_item, 'ef', 'label', 'role') + applicator = double(:enterprise_fee_applicator) + applicator.should_receive(:create_line_item_adjustment).with(line_item) + oc.should_receive(:enterprise_fees_for).with(variant, distributor) { [applicator] } oc.send(:create_adjustments_for, line_item) end @@ -360,44 +360,16 @@ describe OrderCycle do ef1 = double(:enterprise_fee) ef2 = double(:enterprise_fee) ef3 = double(:enterprise_fee) - incoming_exchange = double(:exchange, enterprise_fees: [ef1], incoming?: true) - outgoing_exchange = double(:exchange, enterprise_fees: [ef2], incoming?: false) + incoming_exchange = double(:exchange, enterprise_fees: [ef1], role: 'supplier') + outgoing_exchange = double(:exchange, enterprise_fees: [ef2], role: 'distributor') oc.stub(:exchanges_carrying) { [incoming_exchange, outgoing_exchange] } oc.stub(:coordinator_fees) { [ef3] } - oc.stub(:adjustment_label_for) { 'label' } oc.send(:enterprise_fees_for, line_item.variant, distributor).should == - [{enterprise_fee: ef1, label: 'label', role: 'supplier'}, - {enterprise_fee: ef2, label: 'label', role: 'distributor'}, - {enterprise_fee: ef3, label: 'label', role: 'coordinator'}] - end - - it "creates an adjustment for a fee" do - line_item = create(:line_item) - enterprise_fee = create(:enterprise_fee) - - oc.send(:create_adjustment_for_fee, line_item, enterprise_fee, 'label', 'role') - - adjustment = Spree::Adjustment.last - adjustment.label.should == 'label' - adjustment.adjustable.should == line_item.order - adjustment.source.should == line_item - adjustment.originator.should == enterprise_fee - adjustment.should be_mandatory - - md = adjustment.metadata - md.enterprise.should == enterprise_fee.enterprise - md.fee_name.should == enterprise_fee.name - md.fee_type.should == enterprise_fee.fee_type - md.enterprise_role.should == 'role' - end - - it "makes adjustment labels" do - variant = double(:variant, product: double(:product, name: 'Bananas')) - enterprise_fee = double(:enterprise_fee, fee_type: 'packing', enterprise: double(:enterprise, name: 'Ballantyne')) - - oc.send(:adjustment_label_for, variant, enterprise_fee, 'distributor').should == "Bananas - packing fee by distributor Ballantyne" + [OpenFoodNetwork::EnterpriseFeeApplicator.new(ef1, line_item.variant, 'supplier'), + OpenFoodNetwork::EnterpriseFeeApplicator.new(ef2, line_item.variant, 'distributor'), + OpenFoodNetwork::EnterpriseFeeApplicator.new(ef3, line_item.variant, 'coordinator')] end end From da8a8e8a1af7bce1acd2cd455620790332d06010 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 21 Feb 2014 15:49:10 +1100 Subject: [PATCH 02/11] Rename method --- app/models/order_cycle.rb | 6 +++--- spec/models/order_cycle_spec.rb | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 1dbdbbed18..057f1c4211 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -146,7 +146,7 @@ class OrderCycle < ActiveRecord::Base # -- Fees def fees_for(variant, distributor) - enterprise_fees_for(variant, distributor).sum do |applicator| + per_item_enterprise_fee_applicators_for(variant, distributor).sum do |applicator| # Spree's Calculator interface accepts Orders or LineItems, # so we meet that interface with a struct. # Amount is faked, this is a method on LineItem @@ -159,13 +159,13 @@ class OrderCycle < ActiveRecord::Base variant = line_item.variant distributor = line_item.order.distributor - enterprise_fees_for(variant, distributor).each { |applicator| applicator.create_line_item_adjustment(line_item) } + per_item_enterprise_fee_applicators_for(variant, distributor).each { |applicator| applicator.create_line_item_adjustment(line_item) } end private # -- Fees - def enterprise_fees_for(variant, distributor) + def per_item_enterprise_fee_applicators_for(variant, distributor) fees = [] exchanges_carrying(variant, distributor).each do |exchange| diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 36480ca74b..7ab229d0cc 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -350,7 +350,7 @@ describe OrderCycle do it "creates adjustment for each fee" do applicator = double(:enterprise_fee_applicator) applicator.should_receive(:create_line_item_adjustment).with(line_item) - oc.should_receive(:enterprise_fees_for).with(variant, distributor) { [applicator] } + oc.should_receive(:per_item_enterprise_fee_applicators_for).with(variant, distributor) { [applicator] } oc.send(:create_adjustments_for, line_item) end @@ -366,7 +366,7 @@ describe OrderCycle do oc.stub(:exchanges_carrying) { [incoming_exchange, outgoing_exchange] } oc.stub(:coordinator_fees) { [ef3] } - oc.send(:enterprise_fees_for, line_item.variant, distributor).should == + oc.send(:per_item_enterprise_fee_applicators_for, line_item.variant, distributor).should == [OpenFoodNetwork::EnterpriseFeeApplicator.new(ef1, line_item.variant, 'supplier'), OpenFoodNetwork::EnterpriseFeeApplicator.new(ef2, line_item.variant, 'distributor'), OpenFoodNetwork::EnterpriseFeeApplicator.new(ef3, line_item.variant, 'coordinator')] From febbe087e9d06bb52045c99632a9d40a468e7bcd Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 26 Feb 2014 10:39:39 +1100 Subject: [PATCH 03/11] Find EnterpriseFees with per-item calculators --- app/models/enterprise_fee.rb | 4 ++++ spec/models/enterprise_fee_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index 8f3f038ded..2d8ecbd828 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -21,6 +21,10 @@ class EnterpriseFee < ActiveRecord::Base end } + scope :per_item, lambda { + joins(:calculator).where('spree_calculators.type NOT IN (?)', ['Spree::Calculator::FlatRate', 'Spree::Calculator::FlexiRate']) + } + def self.clear_all_adjustments_for(line_item) line_item.order.adjustments.where(originator_type: 'EnterpriseFee', source_id: line_item, source_type: 'Spree::LineItem').destroy_all end diff --git a/spec/models/enterprise_fee_spec.rb b/spec/models/enterprise_fee_spec.rb index 98484d6545..45ba74caac 100644 --- a/spec/models/enterprise_fee_spec.rb +++ b/spec/models/enterprise_fee_spec.rb @@ -9,6 +9,26 @@ describe EnterpriseFee do it { should validate_presence_of(:name) } end + describe "scopes" do + describe "finding per-item enterprise fees" do + it "does not return fees with FlatRate and FlexiRate calculators" do + create(:enterprise_fee, calculator: Spree::Calculator::FlatRate.new) + create(:enterprise_fee, calculator: Spree::Calculator::FlexiRate.new) + + EnterpriseFee.per_item.should be_empty + end + + it "returns fees with any other calculator" do + ef1 = create(:enterprise_fee, calculator: Spree::Calculator::DefaultTax.new) + ef2 = create(:enterprise_fee, calculator: Spree::Calculator::FlatPercentItemTotal.new) + ef3 = create(:enterprise_fee, calculator: Spree::Calculator::PerItem.new) + ef4 = create(:enterprise_fee, calculator: Spree::Calculator::PriceSack.new) + + EnterpriseFee.per_item.sort.should == [ef1, ef2, ef3, ef4].sort + end + end + end + 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) From 75c8da1774599902d5713cfba96ad5eb62110e36 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 26 Feb 2014 12:07:38 +1100 Subject: [PATCH 04/11] Rename create_adjustments_for to create_line_item_adjustments_for, only show per-item fees --- app/models/order_cycle.rb | 8 ++++---- app/models/spree/order_decorator.rb | 2 +- spec/models/order_cycle_spec.rb | 10 ++++++---- spec/models/spree/order_spec.rb | 2 +- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 057f1c4211..7e85258b19 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -150,12 +150,12 @@ class OrderCycle < ActiveRecord::Base # Spree's Calculator interface accepts Orders or LineItems, # so we meet that interface with a struct. # Amount is faked, this is a method on LineItem - line_item = OpenStruct.new variant: variant, quantity: 1, amount: (variant.price) + line_item = OpenStruct.new variant: variant, quantity: 1, amount: variant.price applicator.enterprise_fee.compute_amount(line_item) end end - def create_adjustments_for(line_item) + def create_line_item_adjustments_for(line_item) variant = line_item.variant distributor = line_item.order.distributor @@ -169,12 +169,12 @@ class OrderCycle < ActiveRecord::Base fees = [] exchanges_carrying(variant, distributor).each do |exchange| - exchange.enterprise_fees.each do |enterprise_fee| + exchange.enterprise_fees.per_item.each do |enterprise_fee| fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, variant, exchange.role) end end - coordinator_fees.each do |enterprise_fee| + coordinator_fees.per_item.each do |enterprise_fee| fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, variant, 'coordinator') end diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 45caf95df9..20c90fe133 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -80,7 +80,7 @@ Spree::Order.class_eval do line_items.each do |line_item| if provided_by_order_cycle? line_item - order_cycle.create_adjustments_for line_item + order_cycle.create_line_item_adjustments_for line_item else pd = product_distribution_for line_item diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 7ab229d0cc..ca11a24ded 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -352,7 +352,7 @@ describe OrderCycle do applicator.should_receive(:create_line_item_adjustment).with(line_item) oc.should_receive(:per_item_enterprise_fee_applicators_for).with(variant, distributor) { [applicator] } - oc.send(:create_adjustments_for, line_item) + oc.send(:create_line_item_adjustments_for, line_item) end it "finds fees for a line item" do @@ -360,11 +360,13 @@ describe OrderCycle do ef1 = double(:enterprise_fee) ef2 = double(:enterprise_fee) ef3 = double(:enterprise_fee) - incoming_exchange = double(:exchange, enterprise_fees: [ef1], role: 'supplier') - outgoing_exchange = double(:exchange, enterprise_fees: [ef2], role: 'distributor') + incoming_exchange = double(:exchange, role: 'supplier') + outgoing_exchange = double(:exchange, role: 'distributor') + incoming_exchange.stub_chain(:enterprise_fees, :per_item) { [ef1] } + outgoing_exchange.stub_chain(:enterprise_fees, :per_item) { [ef2] } oc.stub(:exchanges_carrying) { [incoming_exchange, outgoing_exchange] } - oc.stub(:coordinator_fees) { [ef3] } + oc.stub_chain(:coordinator_fees, :per_item) { [ef3] } oc.send(:per_item_enterprise_fee_applicators_for, line_item.variant, distributor).should == [OpenFoodNetwork::EnterpriseFeeApplicator.new(ef1, line_item.variant, 'supplier'), diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 06f82dacac..5206aae0b2 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -61,7 +61,7 @@ describe Spree::Order do subject.stub(:provided_by_order_cycle?) { true } order_cycle = double(:order_cycle) - order_cycle.should_receive(:create_adjustments_for).with(line_item) + order_cycle.should_receive(:create_line_item_adjustments_for).with(line_item) subject.stub(:order_cycle) { order_cycle } subject.update_distribution_charge! From 890af85d307a6e0bc3a30fa55238d5f097986ab8 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 26 Feb 2014 13:28:05 +1100 Subject: [PATCH 05/11] Create per-order adjustments with EnterpriseFeeApplicator --- .../enterprise_fee_applicator.rb | 19 ++++++++-- .../enterprise_fee_applicator_spec.rb | 38 +++++++++++++++++-- 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/lib/open_food_network/enterprise_fee_applicator.rb b/lib/open_food_network/enterprise_fee_applicator.rb index c2b3001bca..45905bfaca 100644 --- a/lib/open_food_network/enterprise_fee_applicator.rb +++ b/lib/open_food_network/enterprise_fee_applicator.rb @@ -1,15 +1,28 @@ module OpenFoodNetwork class EnterpriseFeeApplicator < Struct.new(:enterprise_fee, :variant, :role) def create_line_item_adjustment(line_item) - a = enterprise_fee.create_locked_adjustment(adjustment_label, line_item.order, line_item, true) + a = enterprise_fee.create_locked_adjustment(line_item_adjustment_label, line_item.order, line_item, true) + AdjustmentMetadata.create! adjustment: a, enterprise: enterprise_fee.enterprise, fee_name: enterprise_fee.name, fee_type: enterprise_fee.fee_type, enterprise_role: role + end + + def create_order_adjustment(order) + a = enterprise_fee.create_locked_adjustment(order_adjustment_label, order, order, true) AdjustmentMetadata.create! adjustment: a, enterprise: enterprise_fee.enterprise, fee_name: enterprise_fee.name, fee_type: enterprise_fee.fee_type, enterprise_role: role end private - def adjustment_label - "#{variant.product.name} - #{enterprise_fee.fee_type} fee by #{role} #{enterprise_fee.enterprise.name}" + def line_item_adjustment_label + "#{variant.product.name} - #{base_adjustment_label}" + end + + def order_adjustment_label + "Whole order - #{base_adjustment_label}" + end + + def base_adjustment_label + "#{enterprise_fee.fee_type} fee by #{role} #{enterprise_fee.enterprise.name}" end end end diff --git a/spec/lib/open_food_network/enterprise_fee_applicator_spec.rb b/spec/lib/open_food_network/enterprise_fee_applicator_spec.rb index 345eae845c..d10a5e9b72 100644 --- a/spec/lib/open_food_network/enterprise_fee_applicator_spec.rb +++ b/spec/lib/open_food_network/enterprise_fee_applicator_spec.rb @@ -8,7 +8,7 @@ module OpenFoodNetwork product = create(:simple_product) efa = EnterpriseFeeApplicator.new enterprise_fee, product.master, 'role' - efa.stub(:adjustment_label) { 'label' } + efa.stub(:line_item_adjustment_label) { 'label' } efa.create_line_item_adjustment line_item adjustment = Spree::Adjustment.last @@ -25,13 +25,45 @@ module OpenFoodNetwork md.enterprise_role.should == 'role' end - it "makes adjustment labels" do + it "creates an adjustment for an order" do + order = create(:order) + #line_item = create(:line_item) + enterprise_fee = create(:enterprise_fee) + product = create(:simple_product) + + efa = EnterpriseFeeApplicator.new enterprise_fee, nil, 'role' + efa.stub(:order_adjustment_label) { 'label' } + efa.create_order_adjustment order + + adjustment = Spree::Adjustment.last + adjustment.label.should == 'label' + adjustment.adjustable.should == order + adjustment.source.should == order + adjustment.originator.should == enterprise_fee + adjustment.should be_mandatory + + md = adjustment.metadata + md.enterprise.should == enterprise_fee.enterprise + md.fee_name.should == enterprise_fee.name + md.fee_type.should == enterprise_fee.fee_type + md.enterprise_role.should == 'role' + end + + it "makes an adjustment label for a line item" do variant = double(:variant, product: double(:product, name: 'Bananas')) enterprise_fee = double(:enterprise_fee, fee_type: 'packing', enterprise: double(:enterprise, name: 'Ballantyne')) efa = EnterpriseFeeApplicator.new enterprise_fee, variant, 'distributor' - efa.send(:adjustment_label).should == "Bananas - packing fee by distributor Ballantyne" + efa.send(:line_item_adjustment_label).should == "Bananas - packing fee by distributor Ballantyne" + end + + it "makes an adjustment label for an order" do + enterprise_fee = double(:enterprise_fee, fee_type: 'packing', enterprise: double(:enterprise, name: 'Ballantyne')) + + efa = EnterpriseFeeApplicator.new enterprise_fee, nil, 'distributor' + + efa.send(:order_adjustment_label).should == "Whole order - packing fee by distributor Ballantyne" end end end From 5057e236a96110c6d690d874177ad28ebbc7220f Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 26 Feb 2014 13:48:51 +1100 Subject: [PATCH 06/11] Find enterprise fees with per-order calculators --- app/models/enterprise_fee.rb | 7 ++++++- spec/models/enterprise_fee_spec.rb | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index 2d8ecbd828..33f41b5fec 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -6,6 +6,8 @@ class EnterpriseFee < ActiveRecord::Base attr_accessible :enterprise_id, :fee_type, :name, :calculator_type FEE_TYPES = %w(packing transport admin sales) + PER_ORDER_CALCULATORS = ['Spree::Calculator::FlatRate', 'Spree::Calculator::FlexiRate'] + validates_inclusion_of :fee_type, :in => FEE_TYPES validates_presence_of :name @@ -22,7 +24,10 @@ class EnterpriseFee < ActiveRecord::Base } scope :per_item, lambda { - joins(:calculator).where('spree_calculators.type NOT IN (?)', ['Spree::Calculator::FlatRate', 'Spree::Calculator::FlexiRate']) + joins(:calculator).where('spree_calculators.type NOT IN (?)', PER_ORDER_CALCULATORS) + } + scope :per_order, lambda { + joins(:calculator).where('spree_calculators.type IN (?)', PER_ORDER_CALCULATORS) } def self.clear_all_adjustments_for(line_item) diff --git a/spec/models/enterprise_fee_spec.rb b/spec/models/enterprise_fee_spec.rb index 45ba74caac..4408c069bc 100644 --- a/spec/models/enterprise_fee_spec.rb +++ b/spec/models/enterprise_fee_spec.rb @@ -27,6 +27,24 @@ describe EnterpriseFee do EnterpriseFee.per_item.sort.should == [ef1, ef2, ef3, ef4].sort end end + + describe "finding per-order enterprise fees" do + it "returns fees with FlatRate and FlexiRate calculators" do + ef1 = create(:enterprise_fee, calculator: Spree::Calculator::FlatRate.new) + ef2 = create(:enterprise_fee, calculator: Spree::Calculator::FlexiRate.new) + + EnterpriseFee.per_order.sort.should == [ef1, ef2].sort + end + + it "does not return fees with any other calculator" do + ef1 = create(:enterprise_fee, calculator: Spree::Calculator::DefaultTax.new) + ef2 = create(:enterprise_fee, calculator: Spree::Calculator::FlatPercentItemTotal.new) + ef3 = create(:enterprise_fee, calculator: Spree::Calculator::PerItem.new) + ef4 = create(:enterprise_fee, calculator: Spree::Calculator::PriceSack.new) + + EnterpriseFee.per_order.should be_empty + end + end end describe "clearing all enterprise fee adjustments for a line item" do From 9dec40703a5e16c2484eed2b4d4cfa8be534a6b2 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 26 Feb 2014 13:49:10 +1100 Subject: [PATCH 07/11] Find exchanges with any of a number of variants --- app/models/exchange.rb | 1 + spec/models/exchange_spec.rb | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/app/models/exchange.rb b/app/models/exchange.rb index ce698c1cd0..90db51c36f 100644 --- a/app/models/exchange.rb +++ b/app/models/exchange.rb @@ -20,6 +20,7 @@ class Exchange < ActiveRecord::Base scope :from_enterprises, lambda { |enterprises| where('exchanges.sender_id IN (?)', enterprises) } scope :to_enterprises, lambda { |enterprises| where('exchanges.receiver_id IN (?)', enterprises) } scope :with_variant, lambda { |variant| joins(:exchange_variants).where('exchange_variants.variant_id = ?', variant) } + scope :any_variant, lambda { |variants| joins(:exchange_variants).where('exchange_variants.variant_id IN (?)', variants) } scope :with_product, lambda { |product| joins(:exchange_variants).where('exchange_variants.variant_id IN (?)', product.variants_including_master) } def clone!(new_order_cycle) diff --git a/spec/models/exchange_spec.rb b/spec/models/exchange_spec.rb index df26d20ac0..7095dd03f9 100644 --- a/spec/models/exchange_spec.rb +++ b/spec/models/exchange_spec.rb @@ -111,6 +111,15 @@ describe Exchange do Exchange.with_variant(v).should == [ex] end + it "finds exchanges with any of a number of variants" do + v1 = create(:variant) + v2 = create(:variant) + ex = create(:exchange) + ex.variants << v1 + + Exchange.any_variant([v1, v2]).should == [ex] + end + it "finds exchanges with a particular product's master variant" do p = create(:simple_product) ex = create(:exchange) From b720a1d8f2f11f2940b188c2f21fd1857a17da57 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 26 Feb 2014 14:33:28 +1100 Subject: [PATCH 08/11] EnterpriseFee.clear_all_adjustments_on_order clears adjustments from per-order fees --- app/models/enterprise_fee.rb | 2 +- spec/models/enterprise_fee_spec.rb | 13 ++++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index 33f41b5fec..148c18d367 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -35,7 +35,7 @@ class EnterpriseFee < ActiveRecord::Base end def self.clear_all_adjustments_on_order(order) - order.adjustments.where(originator_type: 'EnterpriseFee', source_type: 'Spree::LineItem').destroy_all + order.adjustments.where(originator_type: 'EnterpriseFee').destroy_all end # Create an adjustment that starts as locked. Preferable to making an adjustment and locking it since diff --git a/spec/models/enterprise_fee_spec.rb b/spec/models/enterprise_fee_spec.rb index 4408c069bc..806eb7759c 100644 --- a/spec/models/enterprise_fee_spec.rb +++ b/spec/models/enterprise_fee_spec.rb @@ -76,7 +76,7 @@ describe EnterpriseFee do end describe "clearing all enterprise fee adjustments on an order" do - it "clears adjustments from many fees and one all line items" do + it "clears adjustments from many fees and on all line items" do order = create(:order) p1 = create(:simple_product) @@ -98,6 +98,17 @@ describe EnterpriseFee do end.to change(order.adjustments, :count).by(-4) end + it "clears adjustments from per-order fees" do + order = create(:order) + ef = create(:enterprise_fee) + efa = OpenFoodNetwork::EnterpriseFeeApplicator.new(ef, nil, 'coordinator') + efa.create_order_adjustment(order) + + expect do + EnterpriseFee.clear_all_adjustments_on_order order + end.to change(order.adjustments, :count).by(-1) + end + it "does not clear adjustments from another originator" do order = create(:order) tax_rate = create(:tax_rate, calculator: stub_model(Spree::Calculator)) From 11fb6c96a1940af30099ef51018c377b6d12ac36 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 26 Feb 2014 14:34:30 +1100 Subject: [PATCH 09/11] Charge per-order fees on orders exactly once --- app/models/order_cycle.rb | 32 +++++++++++++++++++- app/models/spree/order_decorator.rb | 2 ++ spec/features/consumer/checkout_spec.rb | 24 +++++++-------- spec/models/order_cycle_spec.rb | 39 +++++++++++++++++++++++-- spec/models/spree/order_spec.rb | 21 +++++++++++++ 5 files changed, 101 insertions(+), 17 deletions(-) diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 7e85258b19..519e693b1e 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -159,9 +159,18 @@ class OrderCycle < ActiveRecord::Base variant = line_item.variant distributor = line_item.order.distributor - per_item_enterprise_fee_applicators_for(variant, distributor).each { |applicator| applicator.create_line_item_adjustment(line_item) } + per_item_enterprise_fee_applicators_for(variant, distributor).each do |applicator| + applicator.create_line_item_adjustment(line_item) + end end + def create_order_adjustments_for(order) + per_order_enterprise_fee_applicators_for(order).each do |applicator| + applicator.create_order_adjustment(order) + end + end + + private # -- Fees @@ -181,7 +190,28 @@ class OrderCycle < ActiveRecord::Base fees end + def per_order_enterprise_fee_applicators_for(order) + fees = [] + + exchanges_supplying(order).each do |exchange| + exchange.enterprise_fees.per_order.each do |enterprise_fee| + fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, nil, exchange.role) + end + end + + coordinator_fees.per_order.each do |enterprise_fee| + fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, nil, 'coordinator') + end + + fees + end + def exchanges_carrying(variant, distributor) exchanges.to_enterprises([coordinator, distributor]).with_variant(variant) end + + def exchanges_supplying(order) + variants = order.line_items.map(&:variant) + exchanges.to_enterprises([coordinator, order.distributor]).any_variant(variants) + end end diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 20c90fe133..35688259dc 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -87,6 +87,8 @@ Spree::Order.class_eval do pd.create_adjustment_for line_item if pd end end + + order_cycle.create_order_adjustments_for self if order_cycle end def set_variant_attributes(variant, attributes) diff --git a/spec/features/consumer/checkout_spec.rb b/spec/features/consumer/checkout_spec.rb index 0c21352400..a81b7e10f2 100644 --- a/spec/features/consumer/checkout_spec.rb +++ b/spec/features/consumer/checkout_spec.rb @@ -128,16 +128,14 @@ feature %q{ ["Bananas - transport fee by supplier Supplier 1", "$4.00", ""], ["Bananas - packing fee by distributor FruitAndVeg", "$7.00", ""], ["Bananas - transport fee by distributor FruitAndVeg", "$8.00", ""], - ["Bananas - admin fee by coordinator My coordinator", "$1.00", ""], - ["Bananas - sales fee by coordinator My coordinator", "$2.00", ""], ["Zucchini - admin fee by supplier Supplier 2", "$5.00", ""], ["Zucchini - sales fee by supplier Supplier 2", "$6.00", ""], ["Zucchini - packing fee by distributor FruitAndVeg", "$7.00", ""], ["Zucchini - transport fee by distributor FruitAndVeg", "$8.00", ""], - ["Zucchini - admin fee by coordinator My coordinator", "$1.00", ""], - ["Zucchini - sales fee by coordinator My coordinator", "$2.00", ""]] + ["Whole order - admin fee by coordinator My coordinator", "$1.00", ""], + ["Whole order - sales fee by coordinator My coordinator", "$2.00", ""]] - page.should have_selector 'span.distribution-total', :text => '$54.00' + page.should have_selector 'span.distribution-total', :text => '$51.00' end scenario "attempting to purchase products that mix product and order cycle distribution", future: true do @@ -396,7 +394,7 @@ feature %q{ # -- Checkout: Delivery order_charges = page.all("tbody#summary-order-charges tr").map {|row| row.all('td').map(&:text)}.take(2) order_charges.should == [["Delivery:", "$0.00"], - ["Distribution:", "$54.00"]] + ["Distribution:", "$51.00"]] click_checkout_continue_button # -- Checkout: Payment @@ -411,12 +409,12 @@ feature %q{ page.should have_selector 'figure#logo h1', text: @distributor_oc.name page.should have_selector 'tfoot#order-charges tr.total td', text: 'Distribution' - page.should have_selector 'tfoot#order-charges tr.total td', text: '54.00' + page.should have_selector 'tfoot#order-charges tr.total td', text: '51.00' # -- Checkout: Email email = ActionMailer::Base.deliveries.last email.reply_to.include?(@distributor_oc.email).should == true - email.body.should =~ /Distribution[\s+]\$54.00/ + email.body.should =~ /Distribution[\s+]\$51.00/ end scenario "when I have past orders, it fills in my address", :js => true do @@ -481,7 +479,7 @@ feature %q{ # -- Checkout: Delivery order_charges = page.all("tbody#summary-order-charges tr").map {|row| row.all('td').map(&:text)}.take(2) order_charges.should == [["Delivery:", "$0.00"], - ["Distribution:", "$54.00"]] + ["Distribution:", "$51.00"]] click_checkout_continue_button # -- Checkout: Payment @@ -495,11 +493,11 @@ feature %q{ page.should have_content @payment_method_distributor_oc.description page.should have_selector 'tfoot#order-charges tr.total td', text: 'Distribution' - page.should have_selector 'tfoot#order-charges tr.total td', text: '54.00' + page.should have_selector 'tfoot#order-charges tr.total td', text: '51.00' # -- Checkout: Email email = ActionMailer::Base.deliveries.last - email.body.should =~ /Distribution[\s+]\$54.00/ + email.body.should =~ /Distribution[\s+]\$51.00/ end @@ -509,8 +507,8 @@ feature %q{ @order_cycle = oc = create(:simple_order_cycle, coordinator: create(:distributor_enterprise, name: 'My coordinator')) # Coordinator - coordinator_fee1 = create(:enterprise_fee, enterprise: oc.coordinator, fee_type: 'admin', amount: 1) - coordinator_fee2 = create(:enterprise_fee, enterprise: oc.coordinator, fee_type: 'sales', amount: 2) + coordinator_fee1 = create(:enterprise_fee, enterprise: oc.coordinator, fee_type: 'admin', calculator: Spree::Calculator::FlatRate.new(preferred_amount: 1)) + coordinator_fee2 = create(:enterprise_fee, enterprise: oc.coordinator, fee_type: 'sales', calculator: Spree::Calculator::FlatRate.new(preferred_amount: 2)) oc.coordinator_fees << coordinator_fee1 oc.coordinator_fees << coordinator_fee2 diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index ca11a24ded..39c9a87412 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -347,7 +347,7 @@ describe OrderCycle do let(:order) { double(:order, distributor: distributor) } let(:line_item) { double(:line_item, variant: variant, order: order) } - it "creates adjustment for each fee" do + it "creates an adjustment for each fee" do applicator = double(:enterprise_fee_applicator) applicator.should_receive(:create_line_item_adjustment).with(line_item) oc.should_receive(:per_item_enterprise_fee_applicators_for).with(variant, distributor) { [applicator] } @@ -355,7 +355,7 @@ describe OrderCycle do oc.send(:create_line_item_adjustments_for, line_item) end - it "finds fees for a line item" do + it "makes fee applicators for a line item" do distributor = double(:distributor) ef1 = double(:enterprise_fee) ef2 = double(:enterprise_fee) @@ -374,7 +374,40 @@ describe OrderCycle do OpenFoodNetwork::EnterpriseFeeApplicator.new(ef3, line_item.variant, 'coordinator')] end end - + + describe "creating adjustments for an order" do + let(:oc) { OrderCycle.new } + let(:distributor) { double(:distributor) } + let(:order) { double(:order, distributor: distributor) } + + it "creates an adjustment for each fee" do + applicator = double(:enterprise_fee_applicator) + applicator.should_receive(:create_order_adjustment).with(order) + oc.should_receive(:per_order_enterprise_fee_applicators_for).with(order) { [applicator] } + + oc.send(:create_order_adjustments_for, order) + end + + it "makes fee applicators for an order" do + distributor = double(:distributor) + ef1 = double(:enterprise_fee) + ef2 = double(:enterprise_fee) + ef3 = double(:enterprise_fee) + incoming_exchange = double(:exchange, role: 'supplier') + outgoing_exchange = double(:exchange, role: 'distributor') + incoming_exchange.stub_chain(:enterprise_fees, :per_order) { [ef1] } + outgoing_exchange.stub_chain(:enterprise_fees, :per_order) { [ef2] } + + oc.stub(:exchanges_supplying) { [incoming_exchange, outgoing_exchange] } + oc.stub_chain(:coordinator_fees, :per_order) { [ef3] } + + oc.send(:per_order_enterprise_fee_applicators_for, order).should == + [OpenFoodNetwork::EnterpriseFeeApplicator.new(ef1, nil, 'supplier'), + OpenFoodNetwork::EnterpriseFeeApplicator.new(ef2, nil, 'distributor'), + OpenFoodNetwork::EnterpriseFeeApplicator.new(ef3, nil, 'coordinator')] + end + end + describe "finding recently closed order cycles" do it "should give the most recently closed order cycle for a distributor" do distributor = create(:distributor_enterprise) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 5206aae0b2..69ff50bd3b 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -54,6 +54,15 @@ describe Spree::Order do subject.update_distribution_charge! end + it "skips order cycle per-order adjustments for orders that don't have an order cycle" do + EnterpriseFee.stub(:clear_all_adjustments_on_order) + subject.stub(:line_items) { [] } + + subject.stub(:order_cycle) { nil } + + subject.update_distribution_charge! + end + it "ensures the correct adjustment(s) are created for order cycles" do EnterpriseFee.stub(:clear_all_adjustments_on_order) line_item = double(:line_item) @@ -62,6 +71,18 @@ describe Spree::Order do order_cycle = double(:order_cycle) order_cycle.should_receive(:create_line_item_adjustments_for).with(line_item) + order_cycle.stub(:create_order_adjustments_for) + subject.stub(:order_cycle) { order_cycle } + + subject.update_distribution_charge! + end + + it "ensures the correct per-order adjustment(s) are created for order cycles" do + EnterpriseFee.stub(:clear_all_adjustments_on_order) + subject.stub(:line_items) { [] } + + order_cycle = double(:order_cycle) + order_cycle.should_receive(:create_order_adjustments_for).with(subject) subject.stub(:order_cycle) { order_cycle } subject.update_distribution_charge! From f7e1befc75e2b91cff96a3a8de43daeccc731148 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 26 Feb 2014 15:16:30 +1100 Subject: [PATCH 10/11] Spec fees_for only sums per-item fees --- spec/models/order_cycle_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 39c9a87412..47e49a73e3 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -310,16 +310,18 @@ describe OrderCycle do end describe "calculating fees for a variant via a particular distributor" do - it "sums all the fees for the variant in the specified hub + order cycle" do + it "sums all the per-item fees for the variant in the specified hub + order cycle" do coordinator = create(:distributor_enterprise) distributor = create(:distributor_enterprise) order_cycle = create(:simple_order_cycle) enterprise_fee1 = create(:enterprise_fee, amount: 20) enterprise_fee2 = create(:enterprise_fee, amount: 3) + enterprise_fee3 = create(:enterprise_fee, + calculator: Spree::Calculator::FlatRate.new(preferred_amount: 2)) product = create(:simple_product) create(:exchange, order_cycle: order_cycle, sender: coordinator, receiver: distributor, - enterprise_fees: [enterprise_fee1, enterprise_fee2], variants: [product.master]) + enterprise_fees: [enterprise_fee1, enterprise_fee2, enterprise_fee3], variants: [product.master]) order_cycle.fees_for(product.master, distributor).should == 23 end From adf4c0e387292ea6395f39b6389fa28900eacd92 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 26 Feb 2014 15:38:41 +1100 Subject: [PATCH 11/11] Sort enterprises by name on enterprise fees admin page --- app/controllers/admin/enterprise_fees_controller.rb | 1 + app/views/admin/enterprise_fees/index.html.haml | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin/enterprise_fees_controller.rb b/app/controllers/admin/enterprise_fees_controller.rb index 6e1a7da113..085828d7b3 100644 --- a/app/controllers/admin/enterprise_fees_controller.rb +++ b/app/controllers/admin/enterprise_fees_controller.rb @@ -8,6 +8,7 @@ module Admin def index @include_calculators = params[:include_calculators].present? @enterprise = current_enterprise + @enterprises = Enterprise.managed_by(spree_current_user).by_name blank_enterprise_fee = EnterpriseFee.new blank_enterprise_fee.enterprise = current_enterprise diff --git a/app/views/admin/enterprise_fees/index.html.haml b/app/views/admin/enterprise_fees/index.html.haml index a59ffc99e0..d3e9a0b88c 100644 --- a/app/views/admin/enterprise_fees/index.html.haml +++ b/app/views/admin/enterprise_fees/index.html.haml @@ -25,7 +25,7 @@ %tr{'ng-repeat' => 'enterprise_fee in enterprise_fees | filter:query'} %td = f.ng_hidden_field :id - = f.ng_collection_select :enterprise_id, Enterprise.managed_by(spree_current_user), :id, :name, 'enterprise_fee.enterprise_id', :include_blank => true + = f.ng_collection_select :enterprise_id, @enterprises, :id, :name, 'enterprise_fee.enterprise_id', :include_blank => true %td= f.ng_select :fee_type, enterprise_fee_type_options, 'enterprise_fee.fee_type' %td= f.ng_text_field :name %td= f.ng_collection_select :calculator_type, @calculators, :name, :description, 'enterprise_fee.calculator_type', {'class' => 'calculator_type', 'ng-model' => 'calculatorType', 'spree-ensure-calculator-preferences-match-type' => "1"}