Merge pull request #4387 from nebulab/kennyadsl/api/deprecate-creatin…

…g-shipment-with-items

Deprecate creating new shipment with an item via API

api/app/controllers/spree/api/shipments_controller.rb

@@ -40,13 +40,31 @@ def select_shipping_method
 
       def create
         authorize! :create, Shipment
-        quantity = params[:quantity].to_i
+
         @shipment = @order.shipments.create(stock_location_id: params.fetch(:stock_location_id))
-        @order.contents.add(variant, quantity, { shipment: @shipment })
 
-        @shipment.save!
+        if passing_deprecated_params_on_create?
+          Spree::Deprecation.warn <<~MSG
+          Passing `quantity` or `variant_id` to
 
-        respond_with(@shipment.reload, default_template: :show)
+              POST /api/shipments
+
+          is deprecated and won't be allowed anymore starting from Solidus 4.0.
+          Instead, create an empty shipment and add items to it subsequently using
+          the dedicated endpoint:
+
+              PUT /api/shipments/{shipment_number}/add
+
+          MSG
+
+          quantity = params[:quantity].to_i
+          variant = Spree::Variant.unscoped.find(params[:variant_id])
+          @order.contents.add(variant, quantity, { shipment: @shipment })
+          @shipment.save!
+          @shipment.reload
+        end
+
+        respond_with(@shipment, default_template: :show)
       end
 
       def update
@@ -130,6 +148,10 @@ def load_transfer_params
         authorize! :create, Shipment
       end
 
+      def passing_deprecated_params_on_create?
+        params[:variant_id] || params[:quantity]
+      end
+
       def find_order_on_create
         @order = Spree::Order.find_by!(number: params[:shipment][:order_id])
         authorize! :show, @order

api/openapi/solidus-api.oas.yml

@@ -5591,6 +5591,10 @@ paths:
         Creates a shipment.
 
         Please note that this request can be only performed by users with the `create` permission on the shipment.
+
+        **Deprecation Warning**: Adding items to the shipment via this endpoint
+        is deprecated. Instead, create an empty shipment and populate it with
+        the dedicated endpoint [to add items to the shipment](/docs/solidus/7078dbcf415ac-add-shipment-item).
       operationId: create-shipment
       tags:
         - Shipments
@@ -5606,8 +5610,10 @@ paths:
                   type: integer
                 variant_id:
                   type: integer
+                  deprecated: true
                 quantity:
                   type: integer
+                  deprecated: true
             examples:
               Example:
                 value:

api/spec/requests/spree/api/shipments_spec.rb

@@ -49,11 +49,9 @@ module Spree::Api
 
       sign_in_as_admin!
 
-      # Start writing this spec a bit differently than before....
       describe 'POST #create' do
         let(:params) do
           {
-            variant_id: stock_location.stock_items.first.variant.to_param,
             shipment: { order_id: order.number },
             stock_location_id: stock_location.to_param
           }
@@ -63,24 +61,77 @@ module Spree::Api
           post spree.api_shipments_path, params: params
         end
 
-        [:variant_id, :stock_location_id].each do |field|
-          context "when #{field} is missing" do
-            before do
-              params.delete(field)
-            end
+        it 'creates a new shipment' do
+          subject
+          expect(response).to be_ok
+          expect(json_response).to have_attributes(attributes)
+        end
 
-            it 'should return proper error' do
-              subject
-              expect(response.status).to eq(422)
-              expect(json_response['exception']).to eq("param is missing or the value is empty: #{field}")
-            end
+        context "when stock_location_id is missing" do
+          before do
+            params.delete(:stock_location_id)
+          end
+
+          it 'returns proper error' do
+            subject
+            expect(response.status).to eq(422)
+            expect(json_response['exception']).to eq("param is missing or the value is empty: stock_location_id")
           end
         end
 
-        it 'should create a new shipment' do
-          subject
-          expect(response).to be_ok
-          expect(json_response).to have_attributes(attributes)
+        context 'when passing a variant along' do
+          let(:params) do
+            {
+              variant_id: stock_location.stock_items.first.variant.to_param,
+              shipment: { order_id: order.number },
+              stock_location_id: stock_location.to_param
+            }
+          end
+
+          it 'creates a new shipment with a deprecation message' do
+            expect(Spree::Deprecation).to receive(:warn).with(/variant_id/)
+
+            subject
+            expect(response).to be_ok
+            expect(json_response).to have_attributes(attributes)
+          end
+        end
+
+        context 'when passing a variant and a quantity along' do
+          let(:params) do
+            {
+              variant_id: stock_location.stock_items.first.variant.to_param,
+              quantity: 2,
+              shipment: { order_id: order.number },
+              stock_location_id: stock_location.to_param
+            }
+          end
+
+          it 'creates a new shipment with a deprecation message' do
+            expect(Spree::Deprecation).to receive(:warn).with(/variant_id/)
+
+            subject
+            expect(response).to be_ok
+            expect(json_response).to have_attributes(attributes)
+          end
+        end
+
+        context 'when passing a quantity along (without a variant_id)' do
+          let(:params) do
+            {
+              quantity: 1,
+              shipment: { order_id: order.number },
+              stock_location_id: stock_location.to_param
+            }
+          end
+
+          it 'returns proper error with a deprecation warning' do
+            expect(Spree::Deprecation).to receive(:warn).with(/variant_id/)
+
+            subject
+            expect(response.status).to eq(404)
+            expect(json_response['error']).to eq("The resource you were looking for could not be found.")
+          end
         end
       end
 
@@ -173,6 +224,17 @@ module Spree::Api
         end
       end
 
+      context 'for empty shipments' do
+        let(:order) { create :completed_order_with_totals }
+        let(:shipment) { order.shipments.create(stock_location: stock_location) }
+
+        it 'adds a variant to a shipment' do
+          put spree.add_api_shipment_path(shipment), params: { variant_id: variant.to_param, quantity: 2 }
+          expect(response.status).to eq(200)
+          expect(json_response['manifest'].detect { |h| h['variant']['id'] == variant.id }["quantity"]).to eq(2)
+        end
+      end
+
       describe '#mine' do
         subject do
           get spree.mine_api_shipments_path, params: params