From d571bc731becb7ce476da98e9fc4b4203b43ef52 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 25 Mar 2021 16:03:30 +1100 Subject: [PATCH 01/10] Style spec with rubocop -a --- spec/controllers/api/v0/shipments_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index 26e6a6b29c..b1d4ed80f3 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -65,7 +65,7 @@ describe Api::V0::ShipmentsController, type: :controller do spree_post :create, params - expect(json_response["id"]). to eq(original_shipment_id) + expect(json_response["id"]).to eq(original_shipment_id) expect_valid_response expect(order.shipment.reload.inventory_units.size).to eq 2 expect(order.reload.line_items.first.variant.price).to eq(variant.price) From 65b37b249d95ce2259bec27a31a99ccde1836b28 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 25 Mar 2021 16:47:11 +1100 Subject: [PATCH 02/10] Prepare spec code for re-use --- .../api/v0/shipments_controller_spec.rb | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index b1d4ed80f3..f15e389cf3 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -112,26 +112,32 @@ describe Api::V0::ShipmentsController, type: :controller do context 'for completed shipments' do let(:order) { create :completed_order_with_totals } + let(:line_item) { order.line_items.first } + let(:existing_variant) { line_item.variant } + let(:new_variant) { create(:variant) } + let(:params) { + { + quantity: 2, + order_id: order.to_param, + id: order.shipments.first.to_param + } + } + + before do + line_item.update!(quantity: 3) + end it 'adds a variant to a shipment' do - api_put :add, variant_id: variant.to_param, - quantity: 2, - order_id: order.to_param, - id: order.shipments.first.to_param - + api_put :add, params.merge(variant_id: new_variant.to_param) expect(response.status).to eq(200) - expect(inventory_units_for(variant).size).to eq 2 + expect(inventory_units_for(new_variant).size).to eq 2 end it 'removes a variant from a shipment' do - order.contents.add(variant, 2) - api_put :remove, variant_id: variant.to_param, - quantity: 1, - order_id: order.to_param, - id: order.shipments.first.to_param + api_put :remove, params.merge(variant_id: existing_variant.to_param) expect(response.status).to eq(200) - expect(inventory_units_for(variant).size).to eq(1) + expect(inventory_units_for(existing_variant).size).to eq(1) end end From c5e72f8563230536c34ff3d3a922ab5aefe321cb Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 25 Mar 2021 16:50:55 +1100 Subject: [PATCH 03/10] Test shipment changes more precisely --- .../api/v0/shipments_controller_spec.rb | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index f15e389cf3..0b8428d137 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -128,16 +128,17 @@ describe Api::V0::ShipmentsController, type: :controller do end it 'adds a variant to a shipment' do - api_put :add, params.merge(variant_id: new_variant.to_param) - expect(response.status).to eq(200) - expect(inventory_units_for(new_variant).size).to eq 2 + expect { + api_put :add, params.merge(variant_id: new_variant.to_param) + expect(response.status).to eq(200) + }.to change { inventory_units_for(new_variant).size }.by(2) end it 'removes a variant from a shipment' do - api_put :remove, params.merge(variant_id: existing_variant.to_param) - - expect(response.status).to eq(200) - expect(inventory_units_for(existing_variant).size).to eq(1) + expect { + api_put :remove, params.merge(variant_id: existing_variant.to_param) + expect(response.status).to eq(200) + }.to change { inventory_units_for(existing_variant).size }.by(-2) end end From e1d22aec8391cf038396ace37985d62f1f224f30 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 25 Mar 2021 16:54:26 +1100 Subject: [PATCH 04/10] Prepare for more shipment specs with different context This commit is best viewed ignoring whitespaces. --- .../api/v0/shipments_controller_spec.rb | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index 0b8428d137..aff739a8d2 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -110,7 +110,7 @@ describe Api::V0::ShipmentsController, type: :controller do expect(response.status).to eq(422) end - context 'for completed shipments' do + describe "#add and #remove" do let(:order) { create :completed_order_with_totals } let(:line_item) { order.line_items.first } let(:existing_variant) { line_item.variant } @@ -127,18 +127,20 @@ describe Api::V0::ShipmentsController, type: :controller do line_item.update!(quantity: 3) end - it 'adds a variant to a shipment' do - expect { - api_put :add, params.merge(variant_id: new_variant.to_param) - expect(response.status).to eq(200) - }.to change { inventory_units_for(new_variant).size }.by(2) - end + context 'for completed shipments' do + it 'adds a variant to a shipment' do + expect { + api_put :add, params.merge(variant_id: new_variant.to_param) + expect(response.status).to eq(200) + }.to change { inventory_units_for(new_variant).size }.by(2) + end - it 'removes a variant from a shipment' do - expect { - api_put :remove, params.merge(variant_id: existing_variant.to_param) - expect(response.status).to eq(200) - }.to change { inventory_units_for(existing_variant).size }.by(-2) + it 'removes a variant from a shipment' do + expect { + api_put :remove, params.merge(variant_id: existing_variant.to_param) + expect(response.status).to eq(200) + }.to change { inventory_units_for(existing_variant).size }.by(-2) + end end end From 8079fb031513eb1e12e333ddb56af738570a704f Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 25 Mar 2021 16:58:42 +1100 Subject: [PATCH 05/10] Add specs for stock changes --- spec/controllers/api/v0/shipments_controller_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index aff739a8d2..cf04568a78 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -135,12 +135,24 @@ describe Api::V0::ShipmentsController, type: :controller do }.to change { inventory_units_for(new_variant).size }.by(2) end + it 'adjusts stock when adding a variant' do + expect { + api_put :add, params.merge(variant_id: new_variant.to_param) + }.to change { new_variant.reload.on_hand }.by(-2) + end + it 'removes a variant from a shipment' do expect { api_put :remove, params.merge(variant_id: existing_variant.to_param) expect(response.status).to eq(200) }.to change { inventory_units_for(existing_variant).size }.by(-2) end + + it 'adjusts stock when removing a variant' do + expect { + api_put :remove, params.merge(variant_id: existing_variant.to_param) + }.to change { existing_variant.reload.on_hand }.by(2) + end end end From f2a2cbd3f9c1822b18f46450da1e911071fd00e0 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 25 Mar 2021 17:09:09 +1100 Subject: [PATCH 06/10] Spec stock bug when changing canceled orders We decided to disallow changing canceled orders in a way that would affect stock or totals. --- .../api/v0/shipments_controller_spec.rb | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index cf04568a78..fe249ba2b7 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -154,6 +154,30 @@ describe Api::V0::ShipmentsController, type: :controller do }.to change { existing_variant.reload.on_hand }.by(2) end end + + context "for canceled orders" do + before do + expect(order.cancel).to eq true + end + + it "doesn't adjusts stock when adding a variant" do + pending "https://github.com/openfoodfoundation/openfoodnetwork/issues/7166" + + expect { + api_put :add, params.merge(variant_id: existing_variant.to_param) + expect(response.status).to eq(422) + }.to_not change { existing_variant.reload.on_hand } + end + + it "doesn't adjusts stock when removing a variant" do + pending "https://github.com/openfoodfoundation/openfoodnetwork/issues/7166" + + expect { + api_put :remove, params.merge(variant_id: existing_variant.to_param) + expect(response.status).to eq(422) + }.to_not change { existing_variant.reload.on_hand } + end + end end context "can transition a shipment from ready to ship" do From f0d5bf0ab52e8c3da35bb25a0f1f5d2736b80bdf Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 31 Mar 2021 11:53:31 +1100 Subject: [PATCH 07/10] Disallow changes of canceled order --- app/controllers/api/v0/shipments_controller.rb | 5 +++++ spec/controllers/api/v0/shipments_controller_spec.rb | 4 ---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/controllers/api/v0/shipments_controller.rb b/app/controllers/api/v0/shipments_controller.rb index ec452b2869..d144fa96ba 100644 --- a/app/controllers/api/v0/shipments_controller.rb +++ b/app/controllers/api/v0/shipments_controller.rb @@ -8,6 +8,7 @@ module Api respond_to :json before_action :find_order + before_action :refuse_changing_cancelled_orders, only: [:add, :remove] before_action :find_and_update_shipment, only: [:ship, :ready, :add, :remove] def create @@ -95,6 +96,10 @@ module Api @shipment.reload end + def refuse_changing_cancelled_orders + render status: :unprocessable_entity if @order.canceled? + end + def scoped_variant(variant_id) variant = Spree::Variant.find(variant_id) OpenFoodNetwork::ScopeVariantToHub.new(@order.distributor).scope(variant) diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index fe249ba2b7..188183e198 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -161,8 +161,6 @@ describe Api::V0::ShipmentsController, type: :controller do end it "doesn't adjusts stock when adding a variant" do - pending "https://github.com/openfoodfoundation/openfoodnetwork/issues/7166" - expect { api_put :add, params.merge(variant_id: existing_variant.to_param) expect(response.status).to eq(422) @@ -170,8 +168,6 @@ describe Api::V0::ShipmentsController, type: :controller do end it "doesn't adjusts stock when removing a variant" do - pending "https://github.com/openfoodfoundation/openfoodnetwork/issues/7166" - expect { api_put :remove, params.merge(variant_id: existing_variant.to_param) expect(response.status).to eq(422) From bfa5d443f16736d151ac6d38fe864786ef7520dc Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 31 Mar 2021 14:38:40 +1100 Subject: [PATCH 08/10] Don't change canceled orders We have a PR already that removes the UI for this when the order is canceled. Implementing it on controller-side makes sure that it doesn't happen accidentally if the user has multiple tabs open. --- .../spree/admin/adjustments_controller.rb | 5 ++++ .../admin/adjustments_controller_spec.rb | 28 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/app/controllers/spree/admin/adjustments_controller.rb b/app/controllers/spree/admin/adjustments_controller.rb index ed6b9d2cde..2712b5858f 100644 --- a/app/controllers/spree/admin/adjustments_controller.rb +++ b/app/controllers/spree/admin/adjustments_controller.rb @@ -5,6 +5,7 @@ module Spree prepend_before_action :set_included_tax, only: [:create, :update] before_action :set_order_id, only: [:create, :update] + before_action :skip_changing_canceled_orders, only: [:create, :update] after_action :update_order, only: [:create, :update, :destroy] before_action :set_default_tax_rate, only: :edit before_action :enable_updates, only: :update @@ -28,6 +29,10 @@ module Spree @adjustment.order_id = parent.id end + def skip_changing_canceled_orders + redirect_to admin_order_adjustments_path(@order) if @order.canceled? + end + # Choose a default tax rate to show on the edit form. The adjustment stores its included # tax in dollars, but doesn't store the source of the tax (ie. TaxRate that generated it). # We guess which tax rate here, choosing: diff --git a/spec/controllers/spree/admin/adjustments_controller_spec.rb b/spec/controllers/spree/admin/adjustments_controller_spec.rb index 87b372c3b9..8dafb83493 100644 --- a/spec/controllers/spree/admin/adjustments_controller_spec.rb +++ b/spec/controllers/spree/admin/adjustments_controller_spec.rb @@ -90,5 +90,33 @@ module Spree end end end + + describe "with a cancelled order" do + let(:order) { create(:completed_order_with_totals) } + let(:tax_rate) { create(:tax_rate, amount: 0.1, calculator: ::Calculator::DefaultTax.new) } + let(:adjustment) { + create(:adjustment, adjustable: order, order: order, amount: 1100, included_tax: 100) + } + + before do + expect(order.cancel).to eq true + end + + it "doesn't create adjustments" do + expect { + spree_post :create, order_id: order.number, adjustment: { label: "Testing", amount: "110" }, tax_rate_id: "" + }.to_not change { [Adjustment.count, order.reload.total] } + + expect(response).to redirect_to spree.admin_order_adjustments_path(order) + end + + it "doesn't change adjustments" do + expect { + spree_put :update, order_id: order.number, id: adjustment.id, adjustment: { label: "Testing", amount: "110" }, tax_rate_id: "" + }.to_not change { [adjustment.reload.amount, order.reload.total] } + + expect(response).to redirect_to spree.admin_order_adjustments_path(order) + end + end end end From cf22a864f3270765c02053290f1b30e4698abdd3 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 7 Apr 2021 16:38:08 +1000 Subject: [PATCH 09/10] Fix typo in spec description --- spec/controllers/api/v0/shipments_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index 188183e198..722b821dc5 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -160,14 +160,14 @@ describe Api::V0::ShipmentsController, type: :controller do expect(order.cancel).to eq true end - it "doesn't adjusts stock when adding a variant" do + it "doesn't adjust stock when adding a variant" do expect { api_put :add, params.merge(variant_id: existing_variant.to_param) expect(response.status).to eq(422) }.to_not change { existing_variant.reload.on_hand } end - it "doesn't adjusts stock when removing a variant" do + it "doesn't adjust stock when removing a variant" do expect { api_put :remove, params.merge(variant_id: existing_variant.to_param) expect(response.status).to eq(422) From 42543bfaf721bfb7fac296bd9750dba96e778379 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 7 Apr 2021 16:53:21 +1000 Subject: [PATCH 10/10] Add flash when order cannot be changed --- app/controllers/spree/admin/adjustments_controller.rb | 3 +++ config/locales/en.yml | 2 ++ 2 files changed, 5 insertions(+) diff --git a/app/controllers/spree/admin/adjustments_controller.rb b/app/controllers/spree/admin/adjustments_controller.rb index 2712b5858f..f4fccde7ca 100644 --- a/app/controllers/spree/admin/adjustments_controller.rb +++ b/app/controllers/spree/admin/adjustments_controller.rb @@ -30,6 +30,9 @@ module Spree end def skip_changing_canceled_orders + return unless @order.canceled? + + flash[:error] = t("admin.adjustments.skipped_changing_canceled_order") redirect_to admin_order_adjustments_path(@order) if @order.canceled? end diff --git a/config/locales/en.yml b/config/locales/en.yml index a110afc92e..cbd249e33a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -366,6 +366,8 @@ en: rename: "Rename" admin: + adjustments: + skipped_changing_canceled_order: "You can't change a cancelled order." # Common properties / models begins_at: Begins At begins_on: Begins On