From 932d000c9d450253280b75a17b0f670112a1f248 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 29 Apr 2021 14:21:39 +0100 Subject: [PATCH 1/7] Improve shipping method factory You can now specify an amount for the flat_rate option. --- spec/factories/shipping_method_factory.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/factories/shipping_method_factory.rb b/spec/factories/shipping_method_factory.rb index d67a20aaf7..d6957e78bd 100644 --- a/spec/factories/shipping_method_factory.rb +++ b/spec/factories/shipping_method_factory.rb @@ -41,7 +41,8 @@ FactoryBot.define do end trait :flat_rate do - calculator { Calculator::FlatRate.new(preferred_amount: 50.0) } + transient { amount { 50.0 } } + calculator { Calculator::FlatRate.new(preferred_amount: amount) } end trait :expensive_name do From 045ce73c832008aad3fcada5659841d788213e4b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 30 Apr 2021 12:16:22 +0100 Subject: [PATCH 2/7] Simplify shipment controller params --- app/controllers/api/v0/shipments_controller.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/controllers/api/v0/shipments_controller.rb b/app/controllers/api/v0/shipments_controller.rb index c885d1c221..0d92b5915a 100644 --- a/app/controllers/api/v0/shipments_controller.rb +++ b/app/controllers/api/v0/shipments_controller.rb @@ -34,7 +34,7 @@ module Api @shipment.fee_adjustment.open end - @shipment.update(shipment_params[:shipment]) + @shipment.update(shipment_params) if unlock == 'yes' @shipment.fee_adjustment.close @@ -94,7 +94,7 @@ module Api def find_and_update_shipment @shipment = @order.shipments.find_by!(number: params[:id]) - @shipment.update(shipment_params[:shipment]) if shipment_params[:shipment].present? + @shipment.update(shipment_params) @shipment.reload end @@ -113,10 +113,9 @@ module Api end def shipment_params - params.permit( - [:id, :order_id, :variant_id, :quantity, - { shipment: [:tracking, :selected_shipping_rate_id] }] - ) + return {} unless params.has_key? :shipment + + params.require(:shipment).permit(:tracking, :selected_shipping_rate_id) end end end From d3f41f14c73e4f0984381e818bac88024a9a9be5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 30 Apr 2021 08:45:53 +0100 Subject: [PATCH 3/7] Add test coverage for Api::ShipmentsController#update --- .../api/v0/shipments_controller_spec.rb | 62 ++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index b266c1c3e8..86e269cb17 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -176,7 +176,67 @@ describe Api::V0::ShipmentsController, type: :controller do end end - context "can transition a shipment from ready to ship" do + describe "#update" do + let!(:distributor) { create(:distributor_enterprise) } + let!(:shipping_method1) { + create(:shipping_method_with, :flat_rate, distributors: [distributor], amount: 10) + } + let!(:shipping_method2) { + create(:shipping_method_with, :flat_rate, distributors: [distributor], amount: 20) + } + let!(:order_cycle) { create(:order_cycle, distributors: [distributor]) } + let!(:order) { + create(:completed_order_with_totals, order_cycle: order_cycle, distributor: distributor) + } + let(:new_shipping_rate) { + order.shipment.shipping_rates.select{ |sr| sr.shipping_method == shipping_method2 }.first + } + let(:params) { + { + id: order.shipment.number, + order_id: order.number, + shipment: { + selected_shipping_rate_id: new_shipping_rate.id + } + } + } + + before do + order.shipments.first.shipping_methods = [shipping_method1, shipping_method2] + order.select_shipping_method(shipping_method1.id) + order.update! + order.update_columns( + payment_total: 60, + payment_state: "paid" + ) + end + + context "when an order has multiple shipping methods available which could be chosen" do + it "allows changing the selected shipping rate (changing the shipping method)" do + expect(order.shipment.shipping_method).to eq shipping_method1 + expect(order.shipment.cost).to eq 10 + expect(order.total).to eq 60 # item total is 50, shipping cost is 10 + expect(order.payment_state).to eq "paid" # order is fully paid for + + api_put :update, params + expect(response.status).to eq 200 + + order.reload + + expect(order.shipment.shipping_method).to eq shipping_method2 + expect(order.shipment.cost).to eq 20 + expect(order.total).to eq 60 # order totals have not been updated + expect(order.payment_state).to eq "paid" # payment state not updated + + order.update! # Simulate "Update and recalculate fees" button + + expect(order.total).to eq 70 # order total has now changed + expect(order.payment_state).to eq "balance_due" # total changed, payment is due + end + end + end + + context "#ship" do before do allow_any_instance_of(Spree::Order).to receive_messages(paid?: true, complete?: true) # For the shipment notification email From 925676f136af9c17a19595bad9bb35ce8ced1267 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 30 Apr 2021 12:48:53 +0100 Subject: [PATCH 4/7] Split Order::Updater#update into two methods There are two distinct (and very important) operations happening here. The first is to update all adjustments (which can be *incredibly* expensive) and the second is to ensure the order's totals and shipping/payment states are correct. There are lots of places where we currently do both of these things where in fact we only need to do the latter (and skip the really expensive part). --- .../app/services/order_management/order/updater.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/engines/order_management/app/services/order_management/order/updater.rb b/engines/order_management/app/services/order_management/order/updater.rb index c432214751..0aa3a932ef 100644 --- a/engines/order_management/app/services/order_management/order/updater.rb +++ b/engines/order_management/app/services/order_management/order/updater.rb @@ -20,6 +20,10 @@ module OrderManagement # associations try to save and then in turn try to call +update!+ again.) def update update_all_adjustments + update_totals_and_states + end + + def update_totals_and_states update_totals if order.completed? From b843b871f6b92acc6d817022d72752a153cd93a4 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 30 Apr 2021 12:53:12 +0100 Subject: [PATCH 5/7] Ensure order totals and payment/shipment states are correct when changing shipping method on a completed order. This used to happen via an after_save callback in Shipment, which called `order.update!`. That has recently been removed. After changing a shipment's selected shipping rate (shipping method), we need to ensure the order's totals and states are updated. We don't need to update all of the order's adjustments though (see previous commit). FYI Spree handled this issue like this: https://github.com/spree/spree/commit/24388485ed9d2a6f5d5b16e8f1c9528d393d8981, but there are lots of things about that commit that are clearly awful, like: params handling in a model, duplication of Order::Updater logic across the codebase, the Shipment class having responsiblity for knowing which things need to be updated on the order, etc. The result is ultimately the same as what we're doing here though. --- app/controllers/api/v0/shipments_controller.rb | 4 +++- spec/controllers/api/v0/shipments_controller_spec.rb | 7 +------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/app/controllers/api/v0/shipments_controller.rb b/app/controllers/api/v0/shipments_controller.rb index 0d92b5915a..33706ff971 100644 --- a/app/controllers/api/v0/shipments_controller.rb +++ b/app/controllers/api/v0/shipments_controller.rb @@ -34,7 +34,9 @@ module Api @shipment.fee_adjustment.open end - @shipment.update(shipment_params) + if @shipment.update(shipment_params) + @order.updater.update_totals_and_states + end if unlock == 'yes' @shipment.fee_adjustment.close diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index 86e269cb17..a5bde9b708 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -225,12 +225,7 @@ describe Api::V0::ShipmentsController, type: :controller do expect(order.shipment.shipping_method).to eq shipping_method2 expect(order.shipment.cost).to eq 20 - expect(order.total).to eq 60 # order totals have not been updated - expect(order.payment_state).to eq "paid" # payment state not updated - - order.update! # Simulate "Update and recalculate fees" button - - expect(order.total).to eq 70 # order total has now changed + expect(order.total).to eq 70 # item total is 50, shipping cost is 20 expect(order.payment_state).to eq "balance_due" # total changed, payment is due end end From 95a73704a2e366d0d968798c12b37e6d3f288312 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 30 Apr 2021 14:08:22 +0100 Subject: [PATCH 6/7] Add more test coverage to Api::ShipmentController#update --- .../api/v0/shipments_controller_spec.rb | 57 +++++++++++++++---- 1 file changed, 45 insertions(+), 12 deletions(-) diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index a5bde9b708..99b3840aeb 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -212,21 +212,54 @@ describe Api::V0::ShipmentsController, type: :controller do end context "when an order has multiple shipping methods available which could be chosen" do - it "allows changing the selected shipping rate (changing the shipping method)" do - expect(order.shipment.shipping_method).to eq shipping_method1 - expect(order.shipment.cost).to eq 10 - expect(order.total).to eq 60 # item total is 50, shipping cost is 10 - expect(order.payment_state).to eq "paid" # order is fully paid for + context "changing the selected shipping method" do + it "updates the order's totals and states" do + expect(order.shipment.shipping_method).to eq shipping_method1 + expect(order.shipment.cost).to eq 10 + expect(order.total).to eq 60 # item total is 50, shipping cost is 10 + expect(order.payment_state).to eq "paid" # order is fully paid for - api_put :update, params - expect(response.status).to eq 200 + api_put :update, params + expect(response.status).to eq 200 - order.reload + order.reload - expect(order.shipment.shipping_method).to eq shipping_method2 - expect(order.shipment.cost).to eq 20 - expect(order.total).to eq 70 # item total is 50, shipping cost is 20 - expect(order.payment_state).to eq "balance_due" # total changed, payment is due + expect(order.shipment.shipping_method).to eq shipping_method2 + expect(order.shipment.cost).to eq 20 + expect(order.total).to eq 70 # item total is 50, shipping cost is 20 + expect(order.payment_state).to eq "balance_due" # total changed, payment is due + end + + context "using the 'unlock' parameter with closed adjustments" do + before do + order.shipment_adjustments.each(&:close) + end + + it "does not update closed adjustments without unlock option" do + params[:shipment][:unlock] = "no" + + expect { + api_put :update, params + expect(response.status).to eq 200 + }.to_not change { order.reload.shipment.fee_adjustment.amount } + end + + xit "updates closed adjustments with unlock option selected" do + params[:shipment][:unlock] = "yes" + + expect { + api_put :update, params + expect(response.status).to eq 200 + }.to change { order.reload.shipment.fee_adjustment.amount } + end + + it "hits a fatal error when the unlock option is used" do + params[:shipment][:unlock] = "yes" + + api_put :update, params + expect(response.status).to eq 422 + end + end end end end From e290c128bf94a3024fb023f54a77b8a4613bfda7 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 30 Apr 2021 14:10:43 +0100 Subject: [PATCH 7/7] Fix error in Api::ShipmentController#update with :unlock parameter This is a generic issue caused by a clash between state machines trying to define (or failing to define) the #open method on adjustments as part of their state changes interface. This method is already defined in Object. For more details, see: https://github.com/state-machines/state_machines/blob/bb42e33bf736cf3bc3d4ff019be81718fb38e106/lib/state_machines/machine.rb#L323-L350 --- app/controllers/api/v0/shipments_controller.rb | 2 +- spec/controllers/api/v0/shipments_controller_spec.rb | 9 +-------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/app/controllers/api/v0/shipments_controller.rb b/app/controllers/api/v0/shipments_controller.rb index 33706ff971..f2ff7a79d1 100644 --- a/app/controllers/api/v0/shipments_controller.rb +++ b/app/controllers/api/v0/shipments_controller.rb @@ -31,7 +31,7 @@ module Api unlock = params[:shipment].delete(:unlock) if unlock == 'yes' - @shipment.fee_adjustment.open + @shipment.fee_adjustment.fire_events(:open) end if @shipment.update(shipment_params) diff --git a/spec/controllers/api/v0/shipments_controller_spec.rb b/spec/controllers/api/v0/shipments_controller_spec.rb index 99b3840aeb..9720b8df57 100644 --- a/spec/controllers/api/v0/shipments_controller_spec.rb +++ b/spec/controllers/api/v0/shipments_controller_spec.rb @@ -244,7 +244,7 @@ describe Api::V0::ShipmentsController, type: :controller do }.to_not change { order.reload.shipment.fee_adjustment.amount } end - xit "updates closed adjustments with unlock option selected" do + it "updates closed adjustments with unlock option selected" do params[:shipment][:unlock] = "yes" expect { @@ -252,13 +252,6 @@ describe Api::V0::ShipmentsController, type: :controller do expect(response.status).to eq 200 }.to change { order.reload.shipment.fee_adjustment.amount } end - - it "hits a fatal error when the unlock option is used" do - params[:shipment][:unlock] = "yes" - - api_put :update, params - expect(response.status).to eq 422 - end end end end