From 3dcc3fae3075fff18f3bb216e2a9a29e3a42e128 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc=20Busqu=C3=A9?= Date: Thu, 26 May 2022 07:15:13 +0200 Subject: [PATCH 1/2] Deprecate the creation of option_values without an option_type It makes no sense to have dangling option_values without an associated option_type. Ref #3309 & #3517 --- .../controllers/spree/api/option_values_controller.rb | 7 +++++++ api/config/routes.rb | 1 + api/openapi/solidus-api.oas.yml | 2 ++ api/spec/requests/spree/api/option_values_spec.rb | 4 +++- core/app/models/spree/option_value.rb | 9 +++++++++ core/spec/models/spree/option_value_spec.rb | 6 ++++++ 6 files changed, 28 insertions(+), 1 deletion(-) diff --git a/api/app/controllers/spree/api/option_values_controller.rb b/api/app/controllers/spree/api/option_values_controller.rb index 0feea2337e5..abf1f4e590d 100644 --- a/api/app/controllers/spree/api/option_values_controller.rb +++ b/api/app/controllers/spree/api/option_values_controller.rb @@ -18,6 +18,13 @@ def show 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 diff --git a/api/config/routes.rb b/api/config/routes.rb index fd22f60b94e..49fa8e13b81 100644 --- a/api/config/routes.rb +++ b/api/config/routes.rb @@ -54,6 +54,7 @@ resources :option_types do resources :option_values end + # TODO: Remove :create once option_type is required on Solidus v4.0 resources :option_values get '/orders/mine', to: 'orders#mine', as: 'my_orders' diff --git a/api/openapi/solidus-api.oas.yml b/api/openapi/solidus-api.oas.yml index f9797420dcb..129ff156a89 100644 --- a/api/openapi/solidus-api.oas.yml +++ b/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. diff --git a/api/spec/requests/spree/api/option_values_spec.rb b/api/spec/requests/spree/api/option_values_spec.rb index e31c68e4157..0360b076fe5 100644 --- a/api/spec/requests/spree/api/option_values_spec.rb +++ b/api/spec/requests/spree/api/option_values_spec.rb @@ -110,7 +110,9 @@ module Spree::Api expect(option_value.name).to eq("Option Value") end - it "can create an option value" do + 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' diff --git a/core/app/models/spree/option_value.rb b/core/app/models/spree/option_value.rb index 4d529b4d0ef..bfbf89a49b6 100644 --- a/core/app/models/spree/option_value.rb +++ b/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] diff --git a/core/spec/models/spree/option_value_spec.rb b/core/spec/models/spree/option_value_spec.rb index 409f931f2a9..6c42f441102 100644 --- a/core/spec/models/spree/option_value_spec.rb +++ b/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 From 7c49645dd9098722524767323e82441b326e9a4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc=20Busqu=C3=A9?= Date: Thu, 26 May 2022 07:43:15 +0200 Subject: [PATCH 2/2] Deprecate nested option_type->option_value routes for member actions There's no need to fetch the parent id for those routes. --- .../spree/api/option_values_controller.rb | 14 +++++++++++++ api/config/routes.rb | 4 +++- api/openapi/solidus-api.oas.yml | 12 +++++++++-- .../requests/spree/api/option_values_spec.rb | 20 +++++++++++++++++++ 4 files changed, 47 insertions(+), 3 deletions(-) diff --git a/api/app/controllers/spree/api/option_values_controller.rb b/api/app/controllers/spree/api/option_values_controller.rb index abf1f4e590d..5363862e36f 100644 --- a/api/app/controllers/spree/api/option_values_controller.rb +++ b/api/app/controllers/spree/api/option_values_controller.rb @@ -13,6 +13,8 @@ def index end def show + warn_if_nested_member_route + @option_value = scope.find(params[:id]) respond_with(@option_value) end @@ -35,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 @@ -44,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 @@ -62,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 diff --git a/api/config/routes.rb b/api/config/routes.rb index 49fa8e13b81..88c22be2ab4 100644 --- a/api/config/routes.rb +++ b/api/config/routes.rb @@ -52,9 +52,11 @@ end resources :option_types do + # TODO: Use shallow option on Solidus v4.0 resources :option_values end - # TODO: Remove :create once option_type is required on Solidus v4.0 + # 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' diff --git a/api/openapi/solidus-api.oas.yml b/api/openapi/solidus-api.oas.yml index 129ff156a89..83f35045a67 100644 --- a/api/openapi/solidus-api.oas.yml +++ b/api/openapi/solidus-api.oas.yml @@ -2717,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 @@ -2751,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 @@ -2773,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. diff --git a/api/spec/requests/spree/api/option_values_spec.rb b/api/spec/requests/spree/api/option_values_spec.rb index 0360b076fe5..cc5d52fc001 100644 --- a/api/spec/requests/spree/api/option_values_spec.rb +++ b/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,6 +116,14 @@ module Spree::Api expect(option_value.name).to eq("Option Value") end + 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) @@ -138,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