Merge pull request #7520 from Matt-Yorkley/shipment-controller

Shipment controller: test coverage and improvements

app/controllers/api/v0/shipments_controller.rb

@@ -31,10 +31,12 @@ module Api
         unlock = params[:shipment].delete(:unlock)
 
         if unlock == 'yes'
-          @shipment.fee_adjustment.open
+          @shipment.fee_adjustment.fire_events(:open)
         end
 
-        @shipment.update(shipment_params[:shipment])
+        if @shipment.update(shipment_params)
+          @order.updater.update_totals_and_states
+        end
 
         if unlock == 'yes'
           @shipment.fee_adjustment.close
@@ -94,7 +96,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 +115,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

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?

spec/controllers/api/v0/shipments_controller_spec.rb

@@ -176,7 +176,88 @@ 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
+        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
+
+            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
+          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
+
+            it "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
+          end
+        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

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