From d54ab89aacdf9e147289d95d3d76ee617e192e52 Mon Sep 17 00:00:00 2001 From: SamuelMartini Date: Wed, 4 Mar 2020 14:51:34 +0100 Subject: [PATCH 1/4] Rescue from invalid state machine exception in base controller The rescue is moved on Spree::Api::BaseController so that it can be inherited from those controllers that deal with state machine transition. This commit also adds a test to ensure OrdersController#cancel does not return 500 when trying to transition to `cancel` an order that is not completed. When an order is not completed cannot be transitioned. When called in OrdersController#cancel through `canceled_by` the state machine exception result in a 500 error. --- api/app/controllers/spree/api/base_controller.rb | 7 +++++++ .../spree/api/errors/could_not_transition.json.jbuilder | 4 ++++ api/config/locales/en.yml | 2 ++ api/spec/requests/spree/api/orders_controller_spec.rb | 7 +++++++ 4 files changed, 20 insertions(+) create mode 100644 api/app/views/spree/api/errors/could_not_transition.json.jbuilder diff --git a/api/app/controllers/spree/api/base_controller.rb b/api/app/controllers/spree/api/base_controller.rb index 357df060bf1..47b84b55249 100644 --- a/api/app/controllers/spree/api/base_controller.rb +++ b/api/app/controllers/spree/api/base_controller.rb @@ -29,6 +29,7 @@ class BaseController < ActionController::Base rescue_from ActiveRecord::RecordNotFound, with: :not_found rescue_from CanCan::AccessDenied, with: :unauthorized rescue_from Spree::Core::GatewayError, with: :gateway_error + rescue_from StateMachines::InvalidTransition, with: :invalid_transition helper Spree::Api::ApiHelpers @@ -189,6 +190,12 @@ def paginate(resource) def default_per_page Kaminari.config.default_per_page end + + def invalid_transition(error) + logger.error("invalid_transition #{error.event} from #{error.from} for #{error.object.class.name}. Error: #{error.inspect}") + + render "spree/api/errors/could_not_transition", locals: { resource: error.object }, status: :unprocessable_entity + end end end end diff --git a/api/app/views/spree/api/errors/could_not_transition.json.jbuilder b/api/app/views/spree/api/errors/could_not_transition.json.jbuilder new file mode 100644 index 00000000000..82c62d86ae6 --- /dev/null +++ b/api/app/views/spree/api/errors/could_not_transition.json.jbuilder @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +json.error(I18n.t(:could_not_transition, scope: "spree.api", resource: resource.class.name.demodulize.underscore)) +json.errors(resource.errors.to_hash) diff --git a/api/config/locales/en.yml b/api/config/locales/en.yml index a0ba0428da2..dff3bf16387 100644 --- a/api/config/locales/en.yml +++ b/api/config/locales/en.yml @@ -8,6 +8,8 @@ en: invalid_resource: Invalid resource. Please fix errors and try again. invalid_taxonomy_id: Invalid taxonomy id. must_specify_api_key: You must specify an API key. + could_not_transition: The %{resource} could not be transitioned. Please fix the + errors and try again. order: could_not_transition: The order could not be transitioned. Please fix the errors and try again. diff --git a/api/spec/requests/spree/api/orders_controller_spec.rb b/api/spec/requests/spree/api/orders_controller_spec.rb index a643ea2a824..ec97e0c47e7 100644 --- a/api/spec/requests/spree/api/orders_controller_spec.rb +++ b/api/spec/requests/spree/api/orders_controller_spec.rb @@ -883,6 +883,13 @@ module Spree expect(response.status).to eq 200 expect(json_response["user_id"]).to eq(user.id) end + + it "cannot cancel not completed order" do + put spree.cancel_api_order_path(order) + + expect(json_response['error']).to eq(I18n.t(:could_not_transition, scope: "spree.api", resource: 'order')) + expect(response.status).to eq(422) + end end context "can cancel an order" do From 496e22d16fec39dbdac42a2360f70c9f65a8c940 Mon Sep 17 00:00:00 2001 From: SamuelMartini Date: Wed, 4 Mar 2020 15:23:57 +0100 Subject: [PATCH 2/4] Change template for checkout controller Remove state machine exception rescue as it is now rescued from Api/BaseController --- .../controllers/spree/api/checkouts_controller.rb | 11 +---------- .../spree/api/checkouts_controller_spec.rb | 14 +++++++++++++- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/api/app/controllers/spree/api/checkouts_controller.rb b/api/app/controllers/spree/api/checkouts_controller.rb index 4126e656f62..b315fcd866e 100644 --- a/api/app/controllers/spree/api/checkouts_controller.rb +++ b/api/app/controllers/spree/api/checkouts_controller.rb @@ -23,9 +23,6 @@ def next authorize! :update, @order, order_token @order.next! respond_with(@order, default_template: 'spree/api/orders/show', status: 200) - rescue StateMachines::InvalidTransition => error - logger.error("invalid_transition #{error.event} from #{error.from} for #{error.object.class.name}. Error: #{error.inspect}") - respond_with(@order, default_template: 'spree/api/orders/could_not_transition', status: 422) end def advance @@ -42,9 +39,6 @@ def complete @order.complete! respond_with(@order, default_template: 'spree/api/orders/show', status: 200) end - rescue StateMachines::InvalidTransition => error - logger.error("invalid_transition #{error.event} from #{error.from} for #{error.object.class.name}. Error: #{error.inspect}") - respond_with(@order, default_template: 'spree/api/orders/could_not_transition', status: 422) end def update @@ -57,12 +51,9 @@ def update return if after_update_attributes - if @order.completed? || @order.next + if @order.completed? || @order.next! state_callback(:after) respond_with(@order, default_template: 'spree/api/orders/show') - else - logger.error("failed_to_transition_errors=#{@order.errors.full_messages}") - respond_with(@order, default_template: 'spree/api/orders/could_not_transition', status: 422) end else invalid_resource!(@order) diff --git a/api/spec/requests/spree/api/checkouts_controller_spec.rb b/api/spec/requests/spree/api/checkouts_controller_spec.rb index 9b50b9f58aa..43df553ea78 100644 --- a/api/spec/requests/spree/api/checkouts_controller_spec.rb +++ b/api/spec/requests/spree/api/checkouts_controller_spec.rb @@ -388,7 +388,6 @@ module Spree state: 'address', email: nil ) - put spree.next_api_checkout_path(order), params: { id: order.to_param, order_token: order.guest_token } expect(response.status).to eq(422) expect(json_response['error']).to match(/could not be transitioned/) @@ -431,6 +430,19 @@ module Spree expect(json_response['errors']['expected_total']).to include(I18n.t('spree.api.order.expected_total_mismatch')) end end + + context 'when cannot complete' do + let(:order) { create(:order) } + + before { order.update(state: 'cart') } + + it 'returns a state machine error' do + subject + + expect(json_response['error']).to eq(I18n.t(:could_not_transition, scope: "spree.api", resource: 'order')) + expect(response.status).to eq(422) + end + end end end From 18b537026eb83db19fa8896ed7b1fe9b8f40625a Mon Sep 17 00:00:00 2001 From: SamuelMartini Date: Wed, 4 Mar 2020 15:46:09 +0100 Subject: [PATCH 3/4] Remove double authorization Authorization already happen on the first line after the method definition --- api/app/controllers/spree/api/checkouts_controller.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/api/app/controllers/spree/api/checkouts_controller.rb b/api/app/controllers/spree/api/checkouts_controller.rb index b315fcd866e..51b51798464 100644 --- a/api/app/controllers/spree/api/checkouts_controller.rb +++ b/api/app/controllers/spree/api/checkouts_controller.rb @@ -20,7 +20,6 @@ def next respond_with(@order, default_template: 'spree/api/orders/expected_total_mismatch', status: 400) return end - authorize! :update, @order, order_token @order.next! respond_with(@order, default_template: 'spree/api/orders/show', status: 200) end From e51fe03170fea3379fcdfd45ffb8c26676cafbd2 Mon Sep 17 00:00:00 2001 From: Daniele Palombo Date: Fri, 3 Jul 2020 12:39:12 +0200 Subject: [PATCH 4/4] Deprecate orders/could_not_transition view --- .../orders/could_not_transition.json.jbuilder | 5 +++++ ...could_not_transition.json.jbuilder_spec.rb | 20 +++++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 api/spec/views/spree/api/orders/could_not_transition.json.jbuilder_spec.rb diff --git a/api/app/views/spree/api/orders/could_not_transition.json.jbuilder b/api/app/views/spree/api/orders/could_not_transition.json.jbuilder index c371c8e605c..fe716983a24 100644 --- a/api/app/views/spree/api/orders/could_not_transition.json.jbuilder +++ b/api/app/views/spree/api/orders/could_not_transition.json.jbuilder @@ -1,4 +1,9 @@ # frozen_string_literal: true +Spree::Deprecation.warn( + 'spree/api/orders/could_not_transition is deprecated.' \ + ' Please use spree/api/errors/could_not_transition' +) + json.error(I18n.t(:could_not_transition, scope: "spree.api.order")) json.errors(@order.errors.to_hash) diff --git a/api/spec/views/spree/api/orders/could_not_transition.json.jbuilder_spec.rb b/api/spec/views/spree/api/orders/could_not_transition.json.jbuilder_spec.rb new file mode 100644 index 00000000000..78e33c82c9f --- /dev/null +++ b/api/spec/views/spree/api/orders/could_not_transition.json.jbuilder_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe 'spree/api/orders/could_not_transition.json.jbuilder' do + helper(Spree::Api::ApiHelpers) + + let(:template_deprecation_error) do + 'spree/api/orders/could_not_transition is deprecated.' \ + ' Please use spree/api/errors/could_not_transition' + end + + it 'shows a deprecation error' do + assign(:order, instance_double(Spree::Order, errors: {})) + + expect(Spree::Deprecation).to receive(:warn).with(template_deprecation_error) + + render + end +end