From 1bceb0748afdba108b91c4f4cfee05f6838906b2 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 18 Jan 2019 16:48:36 +0100 Subject: [PATCH 1/9] Do not modify config's state in test --- spec/controllers/spree/orders_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index 43ab94211e..578d9299de 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -181,8 +181,8 @@ describe Spree::OrdersController, type: :controller do } } } } before do - Spree::Config.shipment_inc_vat = true - Spree::Config.shipping_tax_rate = 0.25 + allow(Spree::Config).to receive(:shipment_inc_vat) { true } + allow(Spree::Config).to receive(:shipping_tax_rate) { 0.25 } # Sanity check the fees expect(order.adjustments.length).to eq 2 From f589d4125d50ee6c3aa3de9d4d5dd44b0904f1d1 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 18 Jan 2019 16:49:06 +0100 Subject: [PATCH 2/9] Restore distributor in test that relies on it --- spec/controllers/spree/orders_controller_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index 578d9299de..78e1dc6e5a 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -168,7 +168,8 @@ describe Spree::OrdersController, type: :controller do describe "removing items from a completed order" do context "with shipping and transaction fees" do - let(:order) { create(:completed_order_with_fees, shipping_fee: shipping_fee, payment_fee: payment_fee) } + let(:distributor) { create(:distributor_enterprise, charges_sales_tax: true, allow_order_changes: true) } + let(:order) { create(:completed_order_with_fees, distributor: distributor, shipping_fee: shipping_fee, payment_fee: payment_fee) } let(:line_item1) { order.line_items.first } let(:line_item2) { order.line_items.second } let(:shipping_fee) { 3 } From 9d1d8513e7cd9e24c984bd5231acb8e220ee14d3 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 21 Jan 2019 12:32:48 +0100 Subject: [PATCH 3/9] Make controller req. params a bit more obvious --- spec/controllers/spree/orders_controller_spec.rb | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index 78e1dc6e5a..9b71d9e636 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -176,10 +176,6 @@ describe Spree::OrdersController, type: :controller do let(:payment_fee) { 5 } let(:item_num) { order.line_items.length } let(:expected_fees) { item_num * (shipping_fee + payment_fee) } - let(:params) { { order: { line_items_attributes: { - "0" => {id: line_item1.id, quantity: 1}, - "1" => {id: line_item2.id, quantity: 0} - } } } } before do allow(Spree::Config).to receive(:shipment_inc_vat) { true } @@ -196,10 +192,13 @@ describe Spree::OrdersController, type: :controller do end it "updates the fees" do - # Setting quantity of an item to zero - spree_post :update, params + spree_post :update, { + order: { line_items_attributes: { + "0" => { id: line_item1.id, quantity: 1 }, + "1" => { id: line_item2.id, quantity: 0 } + } } + } - # Check if fees got updated order.reload expect(order.line_items.count).to eq 1 expect(order.adjustment_total).to eq expected_fees - shipping_fee - payment_fee From 0b62b4621ffa52e1227dbd26d6dd1c617aaad551 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 21 Jan 2019 12:33:41 +0100 Subject: [PATCH 4/9] Make expectation easier to reason about --- spec/controllers/spree/orders_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index 9b71d9e636..58989ce86c 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -201,7 +201,7 @@ describe Spree::OrdersController, type: :controller do order.reload expect(order.line_items.count).to eq 1 - expect(order.adjustment_total).to eq expected_fees - shipping_fee - payment_fee + expect(order.adjustment_total).to eq((item_num - 1) * (shipping_fee + payment_fee)) expect(order.shipment.adjustment.included_tax).to eq 0.6 end end From c9429760cd57d02b41a5a996bde670fbb8004cba Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 21 Jan 2019 12:34:16 +0100 Subject: [PATCH 5/9] Trigger the appropriate callbacks from controller --- app/controllers/spree/orders_controller_decorator.rb | 10 +++++++++- spec/controllers/spree/orders_controller_spec.rb | 1 - 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/app/controllers/spree/orders_controller_decorator.rb b/app/controllers/spree/orders_controller_decorator.rb index af7c31aa4c..ebee6c053c 100644 --- a/app/controllers/spree/orders_controller_decorator.rb +++ b/app/controllers/spree/orders_controller_decorator.rb @@ -42,7 +42,11 @@ Spree::OrdersController.class_eval do end if @order.update_attributes(params[:order]) - @order.line_items = @order.line_items.select {|li| li.quantity > 0 } + discard_empty_line_items + + @order.adjustments.each(&:open) + @order.update! + @order.shipment.ensure_correct_adjustment_with_included_tax if @order.shipment render :edit and return unless apply_coupon_code @@ -130,6 +134,10 @@ Spree::OrdersController.class_eval do private + def discard_empty_line_items + @order.line_items = @order.line_items.select { |li| li.quantity > 0 } + end + def require_order_authentication return if session[:access_token] || params[:token] || spree_current_user diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index 58989ce86c..49262fe5cb 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -199,7 +199,6 @@ describe Spree::OrdersController, type: :controller do } } } - order.reload expect(order.line_items.count).to eq 1 expect(order.adjustment_total).to eq((item_num - 1) * (shipping_fee + payment_fee)) expect(order.shipment.adjustment.included_tax).to eq 0.6 From 8eab3c5ec11825a457c51cfc147665dff34fc8ed Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 21 Jan 2019 14:39:09 +0100 Subject: [PATCH 6/9] Extract meaningful methods from controller action This sets the ground for future refactorings while providing some more readability and context. --- .../spree/orders_controller_decorator.rb | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/app/controllers/spree/orders_controller_decorator.rb b/app/controllers/spree/orders_controller_decorator.rb index ebee6c053c..a76e76becd 100644 --- a/app/controllers/spree/orders_controller_decorator.rb +++ b/app/controllers/spree/orders_controller_decorator.rb @@ -43,10 +43,7 @@ Spree::OrdersController.class_eval do if @order.update_attributes(params[:order]) discard_empty_line_items - - @order.adjustments.each(&:open) - @order.update! - @order.shipment.ensure_correct_adjustment_with_included_tax if @order.shipment + with_open_adjustments { update_totals_and_taxes } render :edit and return unless apply_coupon_code @@ -134,6 +131,20 @@ Spree::OrdersController.class_eval do private + # Updates the various denormalized total attributes of the order and + # recalculates the shipment taxes + def update_totals_and_taxes + @order.updater.update_totals + @order.shipment.ensure_correct_adjustment_with_included_tax if @order.shipment + end + + # Sets the adjustments to open to perform the block's action and closes them back again + def with_open_adjustments + @order.adjustments.each(&:open) + yield + @order.adjustments.each(&:close) + end + def discard_empty_line_items @order.line_items = @order.line_items.select { |li| li.quantity > 0 } end From f4b790eefa2a89f00d6cc1935ea14e60e15cc904 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 22 Jan 2019 13:22:43 +0100 Subject: [PATCH 7/9] Restore adjustments state after updating We don't know exactly if all adjustments are closed in all circumstances, so it's better to be conservative until we figure out. Chances are that non-completed orders have them open. --- .../spree/orders_controller_decorator.rb | 12 ++++++++-- .../spree/orders_controller_spec.rb | 23 +++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/app/controllers/spree/orders_controller_decorator.rb b/app/controllers/spree/orders_controller_decorator.rb index a76e76becd..206a598c33 100644 --- a/app/controllers/spree/orders_controller_decorator.rb +++ b/app/controllers/spree/orders_controller_decorator.rb @@ -138,11 +138,19 @@ Spree::OrdersController.class_eval do @order.shipment.ensure_correct_adjustment_with_included_tax if @order.shipment end - # Sets the adjustments to open to perform the block's action and closes them back again + # Sets the adjustments to open to perform the block's action and restores + # their state to whatever the they had. def with_open_adjustments + previous_states = @order.adjustments.each_with_object({}) do |adjustment, hash| + hash[adjustment.id] = adjustment.state + end @order.adjustments.each(&:open) + yield - @order.adjustments.each(&:close) + + @order.adjustments.each_with_index do |adjustment, index| + adjustment.update_attribute(:state, previous_states[adjustment.id]) + end end def discard_empty_line_items diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index 49262fe5cb..75f26c67b6 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -164,6 +164,18 @@ describe Spree::OrdersController, type: :controller do "1" => {quantity: "99", id: li.id} }) end + + it "keeps the adjustments' previous state" do + order = subject.current_order(true) + line_item = order.add_variant(create(:simple_product, on_hand: 110).variants.first) + adjustment = create(:adjustment, adjustable: order) + + spree_get :update, order: { line_items_attributes: { + "1" => { quantity: "99", id: line_item.id } + }} + + expect(adjustment.state).to eq('open') + end end describe "removing items from a completed order" do @@ -203,6 +215,17 @@ describe Spree::OrdersController, type: :controller do expect(order.adjustment_total).to eq((item_num - 1) * (shipping_fee + payment_fee)) expect(order.shipment.adjustment.included_tax).to eq 0.6 end + + it "keeps the adjustments' previous state" do + spree_post :update, { + order: { line_items_attributes: { + "0" => { id: line_item1.id, quantity: 1 }, + "1" => { id: line_item2.id, quantity: 0 } + } } + } + + expect(order.adjustments.map(&:state)).to eq(['closed', 'closed', 'closed']) + end end context "with enterprise fees" do From 59f39b14f4249236bc91fd3f604103803ade2357 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 23 Jan 2019 10:48:20 +0100 Subject: [PATCH 8/9] Do not restore adjustments that aren't in the hash This way we don't modify adjustments that get created as part of the passed block. We don't know how this might be used in the future. --- app/controllers/spree/orders_controller_decorator.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/controllers/spree/orders_controller_decorator.rb b/app/controllers/spree/orders_controller_decorator.rb index 206a598c33..397b7b966c 100644 --- a/app/controllers/spree/orders_controller_decorator.rb +++ b/app/controllers/spree/orders_controller_decorator.rb @@ -139,17 +139,21 @@ Spree::OrdersController.class_eval do end # Sets the adjustments to open to perform the block's action and restores - # their state to whatever the they had. + # their state to whatever the they had. Note that it does not change any new + # adjustments that might get created in the yielded block. def with_open_adjustments previous_states = @order.adjustments.each_with_object({}) do |adjustment, hash| - hash[adjustment.id] = adjustment.state + hash[adjustment.id] = { adjustment: adjustment, previous_state: adjustment.state } end @order.adjustments.each(&:open) yield - @order.adjustments.each_with_index do |adjustment, index| - adjustment.update_attribute(:state, previous_states[adjustment.id]) + previous_states.each do |adjustment_id, adjustment_pair| + adjustment = adjustment_pair[:adjustment] + previous_state = adjustment_pair[:previous_state] + + adjustment.update_attribute(:state, previous_state) end end From c50fa1224bceb17b0562701e137462dfe3a53014 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 29 Jan 2019 09:39:15 +0100 Subject: [PATCH 9/9] Do not restore adjustments that got deleted If something in the block deleted the adjustment update_attribute will fail. --- app/controllers/spree/orders_controller_decorator.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/controllers/spree/orders_controller_decorator.rb b/app/controllers/spree/orders_controller_decorator.rb index 397b7b966c..4aea409444 100644 --- a/app/controllers/spree/orders_controller_decorator.rb +++ b/app/controllers/spree/orders_controller_decorator.rb @@ -143,17 +143,15 @@ Spree::OrdersController.class_eval do # adjustments that might get created in the yielded block. def with_open_adjustments previous_states = @order.adjustments.each_with_object({}) do |adjustment, hash| - hash[adjustment.id] = { adjustment: adjustment, previous_state: adjustment.state } + hash[adjustment.id] = adjustment.state end @order.adjustments.each(&:open) yield - previous_states.each do |adjustment_id, adjustment_pair| - adjustment = adjustment_pair[:adjustment] - previous_state = adjustment_pair[:previous_state] - - adjustment.update_attribute(:state, previous_state) + @order.adjustments.each do |adjustment| + previous_state = previous_states[adjustment.id] + adjustment.update_attribute(:state, previous_state) if previous_state end end