From be1127b414c911d803742fa4adc846eabd2233c9 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Mon, 5 Aug 2013 17:11:21 +1000 Subject: [PATCH 1/6] Migrate line item shipping_method cache to distribution_fee/shipping_method_name --- ...20130805050109_update_line_item_caching.rb | 41 +++++++++++++++++++ db/schema.rb | 13 +++--- 2 files changed, 48 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20130805050109_update_line_item_caching.rb diff --git a/db/migrate/20130805050109_update_line_item_caching.rb b/db/migrate/20130805050109_update_line_item_caching.rb new file mode 100644 index 0000000000..1663b8ab4a --- /dev/null +++ b/db/migrate/20130805050109_update_line_item_caching.rb @@ -0,0 +1,41 @@ +class UpdateLineItemCaching < ActiveRecord::Migration + + class SpreeLineItem < ActiveRecord::Base + belongs_to :shipping_method, class_name: 'Spree::ShippingMethod' + + def itemwise_shipping_cost + order = OpenStruct.new :line_items => [self] + shipping_method.compute_amount(order) + end + end + + + def up + add_column :spree_line_items, :distribution_fee, :decimal, precision: 10, scale: 2 + add_column :spree_line_items, :shipping_method_name, :string + + SpreeLineItem.all.each do |line_item| + line_item.update_column(:distribution_fee, line_item.itemwise_shipping_cost) + line_item.update_column(:shipping_method_name, line_item.shipping_method.name) + end + + remove_column :spree_line_items, :shipping_method_id + end + + def down + add_column :spree_line_items, :shipping_method_id, :integer + + SpreeLineItem.all.each do |line_item| + shipping_method = Spree::ShippingMethod.find_by_name(line_item.shipping_method_name) + unless shipping_method + say "Shipping method #{line_item.shipping_method_name} not found, using the first available shipping method for LineItem #{line_item.id}" + shipping_method = Spree::ShippingMethod.where("name != 'Delivery'").first + end + + line_item.update_column(:shipping_method_id, shipping_method.id) + end + + remove_column :spree_line_items, :distribution_fee + remove_column :spree_line_items, :shipping_method_name + end +end diff --git a/db/schema.rb b/db/schema.rb index c051264988..5d091c29bf 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20130801012854) do +ActiveRecord::Schema.define(:version => 20130805050109) do create_table "cms_blocks", :force => true do |t| t.integer "page_id", :null => false @@ -364,13 +364,14 @@ ActiveRecord::Schema.define(:version => 20130801012854) do create_table "spree_line_items", :force => true do |t| t.integer "order_id" t.integer "variant_id" - t.integer "quantity", :null => false - t.decimal "price", :precision => 8, :scale => 2, :null => false - t.datetime "created_at", :null => false - t.datetime "updated_at", :null => false + t.integer "quantity", :null => false + t.decimal "price", :precision => 8, :scale => 2, :null => false + t.datetime "created_at", :null => false + t.datetime "updated_at", :null => false t.integer "max_quantity" - t.integer "shipping_method_id" t.string "currency" + t.decimal "distribution_fee", :precision => 10, :scale => 2 + t.string "shipping_method_name" end add_index "spree_line_items", ["order_id"], :name => "index_line_items_on_order_id" From ec3e00c1289df3ee6dd8ae48850236e7ad713eae Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Mon, 5 Aug 2013 17:15:57 +1000 Subject: [PATCH 2/6] Refactor LineItem and clients for new fields --- .../shipping_methods_controller_decorator.rb | 6 --- app/helpers/spree/orders_helper.rb | 2 +- .../open_food_web/calculator/itemwise.rb | 2 +- app/models/spree/line_item_decorator.rb | 28 ++++++----- app/models/spree/order_decorator.rb | 2 +- .../spree/orders/_distributor_fees.html.haml | 4 +- .../order_and_distributor_report.rb | 2 +- spec/factories.rb | 4 -- spec/features/admin/shipping_methods_spec.rb | 20 -------- spec/features/consumer/add_to_cart_spec.rb | 5 +- .../order_and_distributor_report_spec.rb | 2 +- spec/models/calculator/itemwise_spec.rb | 2 +- spec/models/line_item_spec.rb | 50 +++++++++++++++---- 13 files changed, 67 insertions(+), 62 deletions(-) diff --git a/app/controllers/spree/admin/shipping_methods_controller_decorator.rb b/app/controllers/spree/admin/shipping_methods_controller_decorator.rb index 84af3ef896..9077c7a41e 100644 --- a/app/controllers/spree/admin/shipping_methods_controller_decorator.rb +++ b/app/controllers/spree/admin/shipping_methods_controller_decorator.rb @@ -16,12 +16,6 @@ module Spree flash[:error] = "That shipping method cannot be deleted as it is referenced by a product distribution: #{p.id} - #{p.name}." redirect_to collection_url and return end - - line_item = LineItem.where(:shipping_method_id => @object).first - if line_item - flash[:error] = "That shipping method cannot be deleted as it is referenced by a line item in order: #{line_item.order.number}." - redirect_to collection_url and return - end end end end diff --git a/app/helpers/spree/orders_helper.rb b/app/helpers/spree/orders_helper.rb index 09faba61ee..745d95cf02 100644 --- a/app/helpers/spree/orders_helper.rb +++ b/app/helpers/spree/orders_helper.rb @@ -7,7 +7,7 @@ module Spree def order_delivery_fee_subtotal(order, options={}) options.reverse_merge! :format_as_currency => true - amount = order.line_items.map { |li| li.itemwise_shipping_cost }.sum + amount = OpenFoodWeb::Calculator::Itemwise.new.compute(order) options.delete(:format_as_currency) ? number_to_currency(amount) : amount end diff --git a/app/models/open_food_web/calculator/itemwise.rb b/app/models/open_food_web/calculator/itemwise.rb index 5a955a639a..76f54cc62c 100644 --- a/app/models/open_food_web/calculator/itemwise.rb +++ b/app/models/open_food_web/calculator/itemwise.rb @@ -7,7 +7,7 @@ module OpenFoodWeb def compute(object) # Given an order, sum the shipping on each individual item - object.line_items.map { |li| li.itemwise_shipping_cost }.inject(:+) || 0 + object.line_items.sum { |li| li.distribution_fee } || 0 end end end diff --git a/app/models/spree/line_item_decorator.rb b/app/models/spree/line_item_decorator.rb index ec4f83e5e2..edca66c6fb 100644 --- a/app/models/spree/line_item_decorator.rb +++ b/app/models/spree/line_item_decorator.rb @@ -1,24 +1,28 @@ Spree::LineItem.class_eval do - belongs_to :shipping_method - attr_accessible :max_quantity - before_create :set_itemwise_shipping_method + before_create :set_distribution_fee - def itemwise_shipping_cost - order = OpenStruct.new :line_items => [self] - shipping_method.compute_amount(order) - end - - def update_itemwise_shipping_method_without_callbacks!(distributor) - update_column(:shipping_method_id, self.product.shipping_method_for_distributor(distributor).id) + def update_distribution_fee_without_callbacks!(distributor) + set_distribution_fee(distributor) + update_column(:distribution_fee, distribution_fee) + update_column(:shipping_method_name, shipping_method_name) end private - def set_itemwise_shipping_method - self.shipping_method = self.product.shipping_method_for_distributor(self.order.distributor) + def shipping_method(distributor=nil) + distributor ||= self.order.distributor + self.product.shipping_method_for_distributor(distributor) + end + + def set_distribution_fee(distributor=nil) + order = OpenStruct.new :line_items => [self] + sm = shipping_method(distributor) + + self.distribution_fee = sm.compute_amount(order) + self.shipping_method_name = sm.name end end diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index b1dba6cec3..0ab43efe51 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -99,7 +99,7 @@ Spree::Order.class_eval do def update_line_item_shipping_methods if %w(cart address delivery resumed).include? state - self.line_items.each { |li| li.update_itemwise_shipping_method_without_callbacks!(distributor) } + self.line_items.each { |li| li.update_distribution_fee_without_callbacks!(distributor) } self.update! end end diff --git a/app/views/spree/orders/_distributor_fees.html.haml b/app/views/spree/orders/_distributor_fees.html.haml index ff3e640c2d..015d4d77ee 100644 --- a/app/views/spree/orders/_distributor_fees.html.haml +++ b/app/views/spree/orders/_distributor_fees.html.haml @@ -14,8 +14,8 @@ %tr{:class => tr_class} %td %h4= link_to line_item.product.name, product_path(line_item.product) - %td.item-shipping-method= line_item.shipping_method.name - %td.item-shipping-cost= number_to_currency line_item.itemwise_shipping_cost + %td.item-shipping-method= line_item.shipping_method_name + %td.item-shipping-cost= number_to_currency line_item.distribution_fee .subtotal{'data-hook' => 'delivery_fees_subtotal'} %h5 diff --git a/lib/open_food_web/order_and_distributor_report.rb b/lib/open_food_web/order_and_distributor_report.rb index bcd7aba08d..04a3596441 100644 --- a/lib/open_food_web/order_and_distributor_report.rb +++ b/lib/open_food_web/order_and_distributor_report.rb @@ -20,7 +20,7 @@ module OpenFoodWeb order.line_items.each do |line_item| order_and_distributor_details << [order.created_at, order.id, order.bill_address.full_name, order.email, order.bill_address.phone, order.bill_address.city, - line_item.product.sku, line_item.product.name, line_item.variant.options_text, line_item.quantity, line_item.max_quantity, line_item.price * line_item.quantity, line_item.itemwise_shipping_cost, + line_item.product.sku, line_item.product.name, line_item.variant.options_text, line_item.quantity, line_item.max_quantity, line_item.price * line_item.quantity, line_item.distribution_fee, order.payments.first.payment_method.andand.name, order.distributor.andand.name, order.distributor.address.address1, order.distributor.address.city, order.distributor.address.zipcode, order.special_instructions ] end diff --git a/spec/factories.rb b/spec/factories.rb index 84140468d1..15fa2a80b7 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -139,10 +139,6 @@ FactoryGirl.modify do end end - factory :line_item do - shipping_method { |li| li.product.shipping_method_for_distributor(li.order.distributor) } - end - factory :shipping_method do display_on '' end diff --git a/spec/features/admin/shipping_methods_spec.rb b/spec/features/admin/shipping_methods_spec.rb index baa1b6757e..9f4f1f9490 100644 --- a/spec/features/admin/shipping_methods_spec.rb +++ b/spec/features/admin/shipping_methods_spec.rb @@ -37,24 +37,4 @@ feature 'shipping methods' do page.should have_content "That shipping method cannot be deleted as it is referenced by a product distribution: #{p.id} - #{p.name}." Spree::ShippingMethod.find(@sm.id).should_not be_nil end - - scenario "deleting a shipping method referenced by a line item" do - sm2 = create(:shipping_method) - d = create(:distributor_enterprise) - - p = create(:product) - create(:product_distribution, product: p, distributor: d, shipping_method: sm2) - - o = create(:order, distributor: d) - o.shipping_method = sm2 - o.save! - li = create(:line_item, order: o, product: p) - li.shipping_method = @sm - li.save! - - visit_delete spree.admin_shipping_method_path(@sm) - - page.should have_content "That shipping method cannot be deleted as it is referenced by a line item in order: #{o.number}." - Spree::ShippingMethod.find(@sm.id).should_not be_nil - end end diff --git a/spec/features/consumer/add_to_cart_spec.rb b/spec/features/consumer/add_to_cart_spec.rb index 5423a0ce99..c5e01c1efb 100644 --- a/spec/features/consumer/add_to_cart_spec.rb +++ b/spec/features/consumer/add_to_cart_spec.rb @@ -54,11 +54,12 @@ feature %q{ page.should have_selector 'span.shipping-total', :text => '$1.23' page.should have_selector 'span.grand-total', :text => '$13.57' - # And the item should be in my cart, with shipping method set for the line item + # And the item should be in my cart, with the distribution info set for the line item order = Spree::Order.last line_item = order.line_items.first line_item.product.should == p - line_item.shipping_method.should == p.product_distributions.first.shipping_method + line_item.distribution_fee.should == 1.23 + line_item.shipping_method_name.should == p.product_distributions.first.shipping_method.name # And my order should have its distributor set to the chosen distributor order.distributor.should == d1 diff --git a/spec/lib/open_food_web/order_and_distributor_report_spec.rb b/spec/lib/open_food_web/order_and_distributor_report_spec.rb index 8081319391..cabeb5010f 100644 --- a/spec/lib/open_food_web/order_and_distributor_report_spec.rb +++ b/spec/lib/open_food_web/order_and_distributor_report_spec.rb @@ -39,7 +39,7 @@ module OpenFoodWeb table[0].should == [@order.created_at, @order.id, @bill_address.full_name, @order.email, @bill_address.phone, @bill_address.city, - @line_item.product.sku, @line_item.product.name, @line_item.variant.options_text, @line_item.quantity, @line_item.max_quantity, @line_item.price * @line_item.quantity, @line_item.itemwise_shipping_cost, + @line_item.product.sku, @line_item.product.name, @line_item.variant.options_text, @line_item.quantity, @line_item.max_quantity, @line_item.price * @line_item.quantity, @line_item.distribution_fee, @payment_method.name, @distributor.name, @distributor.address.address1, @distributor.address.city, @distributor.address.zipcode, @shipping_instructions ] end diff --git a/spec/models/calculator/itemwise_spec.rb b/spec/models/calculator/itemwise_spec.rb index 98677d6106..781a285e80 100644 --- a/spec/models/calculator/itemwise_spec.rb +++ b/spec/models/calculator/itemwise_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe OpenFoodWeb::Calculator::Itemwise do it "computes the shipping cost for each line item in an order" do line_item = double(:line_item) - line_item.should_receive(:itemwise_shipping_cost).exactly(3).times.and_return(10) + line_item.should_receive(:distribution_fee).exactly(3).times.and_return(10) order = double(:order) order.stub(:line_items).and_return([line_item]*3) diff --git a/spec/models/line_item_spec.rb b/spec/models/line_item_spec.rb index c447d4eb64..2b46b7d7f7 100644 --- a/spec/models/line_item_spec.rb +++ b/spec/models/line_item_spec.rb @@ -3,21 +3,51 @@ require 'spec_helper' module Spree describe LineItem do describe "computing shipping cost for its product" do - let(:shipping_method) do - sm = create(:shipping_method) + let(:shipping_method_10) do + sm = create(:shipping_method, name: 'SM10') sm.calculator.set_preference :amount, 10 sm end - let(:order) { double(:order, :distributor => nil, :state => 'complete') } - let(:line_item) do - li = LineItem.new - li.stub(:shipping_method).and_return(shipping_method) - li.stub(:order).and_return(order) - li + let(:shipping_method_20) do + sm = create(:shipping_method, name: 'SM20') + sm.calculator.set_preference :amount, 20 + sm + end + let(:distributor1) { create(:distributor_enterprise) } + let(:distributor2) { create(:distributor_enterprise) } + let!(:product) do + p = create(:product) + create(:product_distribution, product: p, distributor: distributor1, shipping_method: shipping_method_10) + create(:product_distribution, product: p, distributor: distributor2, shipping_method: shipping_method_20) + p + end + let(:order_d1) { create(:order, distributor: distributor1, state: 'complete') } + + it "sets distribution fee and shipping method name on creation" do + li = create(:line_item, order: order_d1, product: product) + li.distribution_fee.should == 10 + li.shipping_method_name.should == 'SM10' end - it "computes shipping cost for its product" do - line_item.itemwise_shipping_cost.should == 10 + it "updates its distribution fee & shipping method name" do + li = create(:line_item, order: order_d1, product: product) + + li.update_distribution_fee_without_callbacks! distributor2 + + li.distribution_fee.should == 20 + li.shipping_method_name.should == 'SM20' + end + + describe "fetching its shipping method" do + it "fetches the shipping method for its product when distributor is supplied" do + li = create(:line_item, order: order_d1, product: product) + li.send(:shipping_method, distributor2).should == shipping_method_20 + end + + it "uses the order's distributor when no other distributor is provided" do + li = create(:line_item, order: order_d1, product: product) + li.send(:shipping_method).should == shipping_method_10 + end end end end From d8b7a0544105e44956a8ed61b4cf22d833d47386 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Tue, 6 Aug 2013 09:34:41 +1000 Subject: [PATCH 3/6] Add EnterpriseFee to ProductDistributions --- app/models/product_distribution.rb | 1 + ...0805232516_add_enterprise_fee_to_product_distributions.rb | 5 +++++ db/schema.rb | 3 ++- 3 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20130805232516_add_enterprise_fee_to_product_distributions.rb diff --git a/app/models/product_distribution.rb b/app/models/product_distribution.rb index 026322afd6..eefde4dd63 100644 --- a/app/models/product_distribution.rb +++ b/app/models/product_distribution.rb @@ -2,6 +2,7 @@ class ProductDistribution < ActiveRecord::Base belongs_to :product, :class_name => 'Spree::Product' belongs_to :distributor, :class_name => 'Enterprise' belongs_to :shipping_method, :class_name => 'Spree::ShippingMethod' + belongs_to :enterprise_fee validates_presence_of :product_id, :on => :update validates_presence_of :distributor_id, :shipping_method_id diff --git a/db/migrate/20130805232516_add_enterprise_fee_to_product_distributions.rb b/db/migrate/20130805232516_add_enterprise_fee_to_product_distributions.rb new file mode 100644 index 0000000000..b6154c1dd1 --- /dev/null +++ b/db/migrate/20130805232516_add_enterprise_fee_to_product_distributions.rb @@ -0,0 +1,5 @@ +class AddEnterpriseFeeToProductDistributions < ActiveRecord::Migration + def change + add_column :product_distributions, :enterprise_fee_id, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 5d091c29bf..5868457a6e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20130805050109) do +ActiveRecord::Schema.define(:version => 20130805232516) do create_table "cms_blocks", :force => true do |t| t.integer "page_id", :null => false @@ -218,6 +218,7 @@ ActiveRecord::Schema.define(:version => 20130805050109) do t.integer "shipping_method_id" t.datetime "created_at" t.datetime "updated_at" + t.integer "enterprise_fee_id" end create_table "spree_activators", :force => true do |t| From cfb8c05cb510225e1069bb0f57e7d52be6d76a62 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Tue, 6 Aug 2013 09:35:18 +1000 Subject: [PATCH 4/6] Remove association that's already added by Spree's calculated_adjustments --- app/models/enterprise_fee.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index 8107f1fdf7..8edd76e60b 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -2,7 +2,6 @@ class EnterpriseFee < ActiveRecord::Base belongs_to :enterprise calculated_adjustments - has_one :calculator, :as => :calculable, :dependent => :destroy, :class_name => 'Spree::Calculator' attr_accessible :enterprise_id, :fee_type, :name, :calculator_type From 8fdf0b6ff9e130ab29bc4265438877cc9e042ad4 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Tue, 6 Aug 2013 11:25:57 +1000 Subject: [PATCH 5/6] Fix payment not being captured due to being to small to cover order's distribution fee --- spec/factories.rb | 1 + spec/features/admin/order_spec.rb | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/spec/factories.rb b/spec/factories.rb index 15fa2a80b7..4d91282be1 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -110,6 +110,7 @@ FactoryGirl.define do after(:create) do |order| p = create(:simple_product, :distributors => [order.distributor]) FactoryGirl.create(:line_item, :order => order, :product => p) + order.reload end end end diff --git a/spec/features/admin/order_spec.rb b/spec/features/admin/order_spec.rb index 9f51f1f0f4..72a78c9b4d 100644 --- a/spec/features/admin/order_spec.rb +++ b/spec/features/admin/order_spec.rb @@ -12,13 +12,15 @@ feature %q{ @order = create(:order_with_totals_and_distributor, :user => @user, :state => 'complete', :payment_state => 'balance_due') # ensure order has a payment to capture - create :check_payment, order: @order, amount: @order.amount + distribution_fee = 1 + create :check_payment, order: @order, amount: (@order.amount + distribution_fee) @order.finalize! end context "managing orders" do scenario "capture multiple payments from the orders index page" do # d.cook: could also test for an order that has had payment voided, then a new check payment created but not yet captured. But it's not critical and I know it works anyway. + login_to_admin_section click_link 'Orders' From f1dbc034010dd19b58fcc0a92c4c830534bd2f7a Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Tue, 6 Aug 2013 13:46:32 +1000 Subject: [PATCH 6/6] Use correct amount for order total in spec --- spec/features/admin/order_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/admin/order_spec.rb b/spec/features/admin/order_spec.rb index 72a78c9b4d..a7e8542be9 100644 --- a/spec/features/admin/order_spec.rb +++ b/spec/features/admin/order_spec.rb @@ -12,9 +12,9 @@ feature %q{ @order = create(:order_with_totals_and_distributor, :user => @user, :state => 'complete', :payment_state => 'balance_due') # ensure order has a payment to capture - distribution_fee = 1 - create :check_payment, order: @order, amount: (@order.amount + distribution_fee) @order.finalize! + + create :check_payment, order: @order, amount: @order.total end context "managing orders" do