Merge pull request #4385 from nebulab/waiting-for-dev/duplicated_opti…

…on_value_endpoints

Deprecate dangling option_values and duplicated routes

api/app/controllers/spree/api/option_values_controller.rb

@@ -13,11 +13,20 @@ def index
       end
 
       def show
+        warn_if_nested_member_route
+
         @option_value = scope.find(params[:id])
         respond_with(@option_value)
       end
 
       def create
+        Spree::Deprecation.warn <<~MSG unless request.path.include?('option_types')
+          This route is deprecated, as it'll be no longer possible to create an
+          option_value without an associated option_type. Please, use instead:
+
+            POST api/option_types/{option_type_id}/option_values
+        MSG
+
         authorize! :create, Spree::OptionValue
         @option_value = scope.new(option_value_params)
         if @option_value.save
@@ -28,6 +37,8 @@ def create
       end
 
       def update
+        warn_if_nested_member_route
+
         @option_value = scope.accessible_by(current_ability, :update).find(params[:id])
         if @option_value.update(option_value_params)
           render :show
@@ -37,6 +48,8 @@ def update
       end
 
       def destroy
+        warn_if_nested_member_route
+
         @option_value = scope.accessible_by(current_ability, :destroy).find(params[:id])
         @option_value.destroy
         render plain: nil, status: 204
@@ -55,6 +68,14 @@ def scope
       def option_value_params
         params.require(:option_value).permit(permitted_option_value_attributes)
       end
+
+      def warn_if_nested_member_route
+        Spree::Deprecation.warn <<~MSG if request.path.include?('option_types')
+          This route is deprecated. Use shallow version instead:
+
+            #{request.method.upcase} api/option_values/:id
+        MSG
+      end
     end
   end
 end

api/config/routes.rb

@@ -52,8 +52,11 @@
     end
 
     resources :option_types do
+      # TODO: Use shallow option on Solidus v4.0
       resources :option_values
     end
+    # TODO: Use only: :index on Solidus v4.0 and use shallow option on the nested routes
+    # within option_types
     resources :option_values
 
     get '/orders/mine', to: 'orders#mine', as: 'my_orders'

api/openapi/solidus-api.oas.yml

@@ -2544,6 +2544,8 @@ paths:
           $ref: '#/components/responses/unprocessable-entity'
       summary: Create option value
       description: |-
+        **DEPRECATED**: Use *POST /option_types/{option_type_id}/option_values* instead
+
         Creates an option value.
 
         Only users with the `create` permission on `Spree::OptionValue` can perform this action.
@@ -2715,7 +2717,10 @@ paths:
         '404':
           $ref: '#/components/responses/not-found'
       summary: Get option type value
-      description: Retrieves an option type's value.
+      description: |-
+        **DEPRECATED**: Use *GET /option_values/{id}* instead
+
+        Retrieves an option type's value.
       operationId: get-option-type-value
       tags:
         - Option values
@@ -2749,7 +2754,10 @@ paths:
         '422':
           $ref: '#/components/responses/delete-restriction'
       summary: Delete option type value
-      description: Deletes an option type's value.
+      description: |-
+        **DEPRECATED**: Use *DELETE /option_values/{id}* instead
+
+        Deletes an option type's value.
       operationId: delete-option-type-value
       tags:
         - Option values
@@ -2771,6 +2779,8 @@ paths:
           $ref: '#/components/responses/unprocessable-entity'
       summary: Update option type value
       description: |-
+        **DEPRECATED**: Use *PATCH /option_values/{id}* instead
+
         Updates an option type's value.
 
         Only users with the `update` permission on the option value can perform this action.

api/spec/requests/spree/api/option_values_spec.rb

@@ -53,6 +53,12 @@ module Spree::Api
         expect(json_response).to have_attributes(attributes)
       end
 
+      it "deprecates listing a single option value through a nested route" do
+        expect(Spree::Deprecation).to receive(:warn).with(/deprecated/)
+
+        get spree.api_option_type_option_value_path(option_type, option_value)
+      end
+
       it "cannot create a new option value" do
         post spree.api_option_type_option_values_path(option_type), params: {
                           option_value: {
@@ -110,7 +116,17 @@ module Spree::Api
           expect(option_value.name).to eq("Option Value")
         end
 
-        it "can create an option value" do
+        it "deprecates updating an option value through a nested route" do
+          expect(Spree::Deprecation).to receive(:warn).with(/deprecated/)
+
+          put spree.api_option_type_option_value_path(option_type, option_value), params: { option_value: {
+                                name: "Option Value"
+                              } }
+        end
+
+        it "can create but deprecates creating an option value without option type" do
+          expect(Spree::Deprecation).to receive(:warn).with(/deprecated/).at_least(:once)
+
           post spree.api_option_values_path, params: { option_value: {
                                 name: "Option Value",
                                 presentation: 'option value'
@@ -136,6 +152,12 @@ module Spree::Api
           delete spree.api_option_value_path(option_value)
           expect(response.status).to eq(204)
         end
+
+        it "deprecates deleting an option value through a nested route" do
+          expect(Spree::Deprecation).to receive(:warn).with(/deprecated/)
+
+          delete spree.api_option_type_option_value_path(option_type, option_value)
+        end
       end
     end
   end

core/app/models/spree/option_value.rb

@@ -2,6 +2,8 @@
 
 module Spree
   class OptionValue < Spree::Base
+    # TODO: Remove optional on Solidus v4.0. Don't forget adding a migration to
+    # enforce at the database layer
     belongs_to :option_type, class_name: 'Spree::OptionType', inverse_of: :option_values, optional: true
     acts_as_list scope: :option_type
 
@@ -13,7 +15,14 @@ class OptionValue < Spree::Base
 
     after_save :touch, if: :saved_changes?
     after_touch :touch_all_variants
+    after_save do
+      Spree::Deprecation.warn <<~MSG if option_type.nil?
+        Having an option_value with no associated option_type will be deprecated
+        on Solidus v4.0
+      MSG
+    end
 
+    # TODO: Remove allow_nil once option_type is required on Solidus v4.0
     delegate :name, :presentation, to: :option_type, prefix: :option_type, allow_nil: true
 
     self.whitelisted_ransackable_attributes = %w[name presentation]

core/spec/models/spree/option_value_spec.rb

@@ -49,4 +49,10 @@
       expect(subject).to eq "Size - S"
     end
   end
+
+  it 'deprecates creating an option_value with no associated option_type' do
+    expect(Spree::Deprecation).to receive(:warn).with(/deprecated/)
+
+    create(:option_value, option_type: nil)
+  end
 end