From e45f7571fbdb9fbc386b3ea8e38428e190672b74 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 9 Jan 2019 15:20:28 +0000 Subject: [PATCH 1/7] Fix order_with_totals_and_distribution by updating shipping fees --- spec/factories.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/factories.rb b/spec/factories.rb index 58273f7cb5..df027dfbaf 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -303,6 +303,9 @@ FactoryBot.define do p = create(:simple_product, distributors: [order.distributor]) FactoryBot.create(:line_item_with_shipment, shipping_fee: proxy.shipping_fee, order: order, product: p) order.reload + + # this will update order shipping fees and also order totals (through order.update!) + order.update_shipping_fees! end end From e787a317dccf1a4b8136405a2774d944bb20454a Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 9 Jan 2019 16:02:09 +0000 Subject: [PATCH 2/7] Adapt user_balance_calculator_spec to new order_with_totals_and_distribution with shipping fee correctly included in its total --- .../user_balance_calculator_spec.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/lib/open_food_network/user_balance_calculator_spec.rb b/spec/lib/open_food_network/user_balance_calculator_spec.rb index 6f2d7cfcde..1e5786753f 100644 --- a/spec/lib/open_food_network/user_balance_calculator_spec.rb +++ b/spec/lib/open_food_network/user_balance_calculator_spec.rb @@ -12,11 +12,11 @@ module OpenFoodNetwork let!(:o1) { create(:order_with_totals_and_distribution, user: user1, distributor: hub1, completed_at: 1.day.ago) - } #total=10 + } #total=13 (10 + 3 shipping fee) let!(:o2) { create(:order_with_totals_and_distribution, user: user1, distributor: hub1, completed_at: 1.day.ago) - } #total=10 + } #total=13 (10 + 3 shipping fee) let!(:p1) { create(:payment, order: o1, amount: 15.00, state: "completed") } @@ -25,7 +25,7 @@ module OpenFoodNetwork } it "finds the correct balance for this email and enterprise" do - UserBalanceCalculator.new(o1.email, hub1).balance.should == -3 + UserBalanceCalculator.new(o1.email, hub1).balance.should == -9 # = 15 + 2 - 13 - 13 end context "with another hub" do @@ -33,13 +33,13 @@ module OpenFoodNetwork let!(:o3) { create(:order_with_totals_and_distribution, user: user1, distributor: hub2, completed_at: 1.day.ago) - } #total=10 + } #total=13 (10 + 3 shipping fee) let!(:p3) { create(:payment, order: o3, amount: 15.00, state: "completed") } it "does not find the balance for other enterprises" do - UserBalanceCalculator.new(o3.email, hub2).balance.should == 5 + UserBalanceCalculator.new(o3.email, hub2).balance.should == 2 # = 15 - 13 end end @@ -48,13 +48,13 @@ module OpenFoodNetwork let!(:o4) { create(:order_with_totals_and_distribution, user: user2, distributor: hub1, completed_at: 1.day.ago) - } #total=10 + } #total=13 (10 + 3 shipping fee) let!(:p3) { create(:payment, order: o4, amount: 20.00, state: "completed") } it "does not find the balance for other users" do - UserBalanceCalculator.new(o4.email, hub1).balance.should == 10 + UserBalanceCalculator.new(o4.email, hub1).balance.should == 7 # = 20 - 13 end end @@ -68,7 +68,7 @@ module OpenFoodNetwork } it "does not include canceled orders in the balance" do - UserBalanceCalculator.new(o4.email, hub1).balance.should == -3 + UserBalanceCalculator.new(o4.email, hub1).balance.should == -9 # = 15 + 2 - 13 - 13 end end end From 292f22af4ed9c5b9ec574475bd7bb52c8831318c Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 9 Jan 2019 23:31:40 +0000 Subject: [PATCH 3/7] Adapt factory completed_order_with_fees to new parent order_with_totals_and_distribution by cleaning up duplicate shipping fees --- spec/factories.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/factories.rb b/spec/factories.rb index df027dfbaf..84162e0915 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -432,6 +432,7 @@ FactoryBot.define do create(:payment, order: order, amount: order.total, payment_method: payment_method, state: 'checkout') while !order.completed? do break unless order.next! end + order.updater.update_adjustments # this is required to clean up duplicated shipping fees end end From 24189ca88e7b8ae5cedb40e51b5cec9e3afaa5e4 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 12 Feb 2019 12:11:55 +0100 Subject: [PATCH 4/7] Create shipping_rate earlier for callback to work If the shipping_rate is not created at the time the `#after_save`'s `#ensure_correct_adjustment` callback is executed its call to `shipping_method.create_adjustment` won't be executed either. As a result, the OrderUpdater wasn't able to see any adjustments related to the shipment causing the order total to be outdated. --- spec/factories.rb | 9 +++------ .../open_food_network/user_balance_calculator_spec.rb | 2 -- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/spec/factories.rb b/spec/factories.rb index 688d5d52be..d21d1143fd 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -303,9 +303,6 @@ FactoryBot.define do p = create(:simple_product, distributors: [order.distributor]) FactoryBot.create(:line_item_with_shipment, shipping_fee: proxy.shipping_fee, order: order, product: p) order.reload - - # this will update order shipping fees and also order totals (through order.update!) - order.update_shipping_fees! end end @@ -403,9 +400,10 @@ FactoryBot.define do transient do shipping_method { create(:shipping_method) } end - after(:create) do |shipment, evaluator| - shipment.add_shipping_method(evaluator.shipping_method, true) + shipping_rates { [Spree::ShippingRate.create(shipping_method: shipping_method, selected: true)] } + + after(:create) do |shipment, evaluator| shipment.order.line_items.each do |line_item| line_item.quantity.times { shipment.inventory_units.create(variant_id: line_item.variant_id) } end @@ -432,7 +430,6 @@ FactoryBot.define do create(:payment, order: order, amount: order.total, payment_method: payment_method, state: 'checkout') while !order.completed? do break unless order.next! end - order.updater.update_adjustments # this is required to clean up duplicated shipping fees end end diff --git a/spec/lib/open_food_network/user_balance_calculator_spec.rb b/spec/lib/open_food_network/user_balance_calculator_spec.rb index 1e5786753f..675b3a91cd 100644 --- a/spec/lib/open_food_network/user_balance_calculator_spec.rb +++ b/spec/lib/open_food_network/user_balance_calculator_spec.rb @@ -3,9 +3,7 @@ require 'spec_helper' module OpenFoodNetwork describe UserBalanceCalculator do - describe "finding the account balance of a user with a hub" do - let!(:user1) { create(:user) } let!(:hub1) { create(:distributor_enterprise) } From e9173f440f4d5510ee314dfa242a7c3ecf4a11f2 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 14 Feb 2019 13:02:02 +0100 Subject: [PATCH 5/7] Associate shipment to appropriate order in specs --- spec/models/spree/adjustment_spec.rb | 23 ++++++------ spec/models/spree/order_spec.rb | 55 +++++++++++++++++++++------- 2 files changed, 53 insertions(+), 25 deletions(-) diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 66b2b2f3ab..dca4da1cfe 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -59,35 +59,36 @@ module Spree end describe "Shipment adjustments" do - let(:shipping_method) { create(:shipping_method_with, :flat_rate) } - let(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method) } - let(:order) { create(:order, distributor: hub) } let(:hub) { create(:distributor_enterprise, charges_sales_tax: true) } + let(:order) { create(:order, distributor: hub) } let(:line_item) { create(:line_item, order: order) } - let(:adjustment) { order.adjustments(:reload).shipping.first } + + let(:shipping_method) { create(:shipping_method_with, :flat_rate) } + let(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) } + describe "the shipping charge" do it "is the adjustment amount" do order.shipments = [shipment] - - adjustment.amount.should == 50 + expect(order.adjustments.first.amount).to eq(50) end end describe "when tax on shipping is disabled" do before { Config.shipment_inc_vat = false } + it "records 0% tax on shipment adjustments" do Config.shipping_tax_rate = 0 order.shipments = [shipment] - adjustment.included_tax.should == 0 + expect(order.adjustments.first.included_tax).to eq(0) end it "records 0% tax on shipments when a rate is set but shipment_inc_vat is false" do Config.shipping_tax_rate = 0.25 order.shipments = [shipment] - adjustment.included_tax.should == 0 + expect(order.adjustments.first.included_tax).to eq(0) end end @@ -102,21 +103,21 @@ module Spree # total - ( total / (1 + rate) ) # 50 - ( 50 / (1 + 0.25) ) # = 10 - adjustment.included_tax.should == 10.00 + expect(order.adjustments.first.included_tax).to eq(10.00) end it "records 0% tax on shipments when shipping_tax_rate is not set" do Config.shipping_tax_rate = nil order.shipments = [shipment] - adjustment.included_tax.should == 0 + expect(order.adjustments.first.included_tax).to eq(0) end it "records 0% tax on shipments when the distributor does not charge sales tax" do order.distributor.update_attributes! charges_sales_tax: false order.shipments = [shipment] - adjustment.included_tax.should == 0 + expect(order.adjustments.first.included_tax).to eq(0) end end end diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 9cb79e4d3d..cc95a675a4 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -224,9 +224,8 @@ describe Spree::Order do end describe "getting the shipping tax" do + let(:order) { create(:order) } let(:shipping_method) { create(:shipping_method_with, :flat_rate) } - let(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method) } - let(:order) { create(:order, shipments: [shipment]) } context "with a taxed shipment" do before do @@ -234,13 +233,17 @@ describe Spree::Order do Spree::Config.shipping_tax_rate = 0.25 end + let!(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) } + it "returns the shipping tax" do order.shipping_tax.should == 10 end end - it "returns zero when the order has not been shipped" do - order.shipping_tax.should == 0 + context 'when the order has not been shipped' do + it "returns zero when the order has not been shipped" do + order.shipping_tax.should == 0 + end end end @@ -257,21 +260,33 @@ describe Spree::Order do end describe "getting the total tax" do - let(:shipping_method) { create(:shipping_method_with, :flat_rate) } - let(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method) } - let(:order) { create(:order, shipments: [shipment]) } - let(:enterprise_fee) { create(:enterprise_fee) } - before do Spree::Config.shipment_inc_vat = true Spree::Config.shipping_tax_rate = 0.25 + end - create(:adjustment, adjustable: order, originator: enterprise_fee, label: "EF", amount: 123, included_tax: 2) + let(:order) { create(:order) } + let(:shipping_method) { create(:shipping_method_with, :flat_rate) } + let!(:shipment) do + create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) + end + let(:enterprise_fee) { create(:enterprise_fee) } + + before do + create( + :adjustment, + adjustable: order, + originator: enterprise_fee, + label: "EF", + amount: 123, + included_tax: 2 + ) order.reload end it "returns a sum of all tax on the order" do - order.total_tax.should == 12 + # 12 = 2 (of the enterprise fee adjustment) + 10 (of the shipment adjustment) + expect(order.total_tax).to eq(12) end end @@ -289,19 +304,31 @@ describe Spree::Order do let(:tax_category25) { create(:tax_category, tax_rates: [tax_rate25]) } let(:variant) { create(:variant, product: create(:product, tax_category: tax_category10)) } - let(:shipping_method) { create(:shipping_method, calculator: Spree::Calculator::FlatRate.new(preferred_amount: 46.0)) } - let(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method) } let(:enterprise_fee) { create(:enterprise_fee, enterprise: coordinator, tax_category: tax_category20, calculator: Spree::Calculator::FlatRate.new(preferred_amount: 48.0)) } let(:additional_adjustment) { create(:adjustment, amount: 50.0, included_tax: tax_rate25.compute_tax(50.0)) } let(:order_cycle) { create(:simple_order_cycle, coordinator: coordinator, coordinator_fees: [enterprise_fee], distributors: [coordinator], variants: [variant]) } let(:line_item) { create(:line_item, variant: variant, price: 44.0) } - let(:order) { create(:order, line_items: [line_item], shipments: [shipment], bill_address: create(:address), order_cycle: order_cycle, distributor: coordinator, adjustments: [additional_adjustment]) } + let(:order) do + create( + :order, + line_items: [line_item], + bill_address: create(:address), + order_cycle: order_cycle, + distributor: coordinator, + adjustments: [additional_adjustment] + ) + end before do Spree::Config.shipment_inc_vat = true Spree::Config.shipping_tax_rate = tax_rate15.amount + end + let(:shipping_method) { create(:shipping_method, calculator: Spree::Calculator::FlatRate.new(preferred_amount: 46.0)) } + let!(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) } + + before do order.create_tax_charge! order.update_distribution_charge! end From 609ff737ca364d5bcd02cbb7a15fff05a50806d5 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 14 Feb 2019 16:55:33 +0100 Subject: [PATCH 6/7] Fix and simplify feature spec --- spec/features/consumer/shopping/orders_spec.rb | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/spec/features/consumer/shopping/orders_spec.rb b/spec/features/consumer/shopping/orders_spec.rb index d67ce5aea9..b8bf8fe149 100644 --- a/spec/features/consumer/shopping/orders_spec.rb +++ b/spec/features/consumer/shopping/orders_spec.rb @@ -88,15 +88,25 @@ feature "Order Management", js: true do let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true, charges_sales_tax: true) } let(:order_cycle) { create(:order_cycle) } let(:shipping_method) { distributor.shipping_methods.first } - let(:order) { create(:completed_order_with_totals, order_cycle: order_cycle, distributor: distributor, user: user, bill_address: address, ship_address: address) } + let(:order) do + create( + :completed_order_with_totals, + order_cycle: order_cycle, + distributor: distributor, + user: user, + bill_address: address, + ship_address: address + ) + end let!(:item1) { order.reload.line_items.first } let!(:item2) { create(:line_item, order: order) } let!(:item3) { create(:line_item, order: order) } before do - shipping_method.calculator.update_attributes(preferred_amount: 5.0) - order.shipments = [create(:shipment_with, :shipping_method, shipping_method: shipping_method)] - order.reload.save + order.shipment.shipping_method.calculator.update_attributes(preferred_amount: 5.0) + order.save + order.reload + quick_login_as user end From caf4441fa345b34ad9fb8dec511cc843c5a26d56 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 14 Feb 2019 13:11:49 +0100 Subject: [PATCH 7/7] Do not mutate config's state in specs --- spec/models/spree/adjustment_spec.rb | 16 ++++++++++------ spec/models/spree/order_spec.rb | 12 ++++++------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index dca4da1cfe..a8693843b0 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -75,17 +75,19 @@ module Spree end describe "when tax on shipping is disabled" do - before { Config.shipment_inc_vat = false } + before do + allow(Config).to receive(:shipment_inc_vat).and_return(false) + end it "records 0% tax on shipment adjustments" do - Config.shipping_tax_rate = 0 + allow(Config).to receive(:shipping_tax_rate).and_return(0) order.shipments = [shipment] expect(order.adjustments.first.included_tax).to eq(0) end it "records 0% tax on shipments when a rate is set but shipment_inc_vat is false" do - Config.shipping_tax_rate = 0.25 + allow(Config).to receive(:shipping_tax_rate).and_return(0.25) order.shipments = [shipment] expect(order.adjustments.first.included_tax).to eq(0) @@ -93,10 +95,12 @@ module Spree end describe "when tax on shipping is enabled" do - before { Config.shipment_inc_vat = true } + before do + allow(Config).to receive(:shipment_inc_vat).and_return(true) + end it "takes the shipment adjustment tax included from the system setting" do - Config.shipping_tax_rate = 0.25 + allow(Config).to receive(:shipping_tax_rate).and_return(0.25) order.shipments = [shipment] # Finding the tax included in an amount that's already inclusive of tax: @@ -107,7 +111,7 @@ module Spree end it "records 0% tax on shipments when shipping_tax_rate is not set" do - Config.shipping_tax_rate = nil + allow(Config).to receive(:shipping_tax_rate).and_return(0) order.shipments = [shipment] expect(order.adjustments.first.included_tax).to eq(0) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index cc95a675a4..316c186787 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -229,8 +229,8 @@ describe Spree::Order do context "with a taxed shipment" do before do - Spree::Config.shipment_inc_vat = true - Spree::Config.shipping_tax_rate = 0.25 + allow(Spree::Config).to receive(:shipment_inc_vat).and_return(true) + allow(Spree::Config).to receive(:shipping_tax_rate).and_return(0.25) end let!(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) } @@ -261,8 +261,8 @@ describe Spree::Order do describe "getting the total tax" do before do - Spree::Config.shipment_inc_vat = true - Spree::Config.shipping_tax_rate = 0.25 + allow(Spree::Config).to receive(:shipment_inc_vat).and_return(true) + allow(Spree::Config).to receive(:shipping_tax_rate).and_return(0.25) end let(:order) { create(:order) } @@ -321,8 +321,8 @@ describe Spree::Order do end before do - Spree::Config.shipment_inc_vat = true - Spree::Config.shipping_tax_rate = tax_rate15.amount + allow(Spree::Config).to receive(:shipment_inc_vat).and_return(true) + allow(Spree::Config).to receive(:shipping_tax_rate).and_return(tax_rate15.amount) end let(:shipping_method) { create(:shipping_method, calculator: Spree::Calculator::FlatRate.new(preferred_amount: 46.0)) }