Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rescue state machine exception from api base controller #3520

Merged
merged 4 commits into from
Jul 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions api/app/controllers/spree/api/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
12 changes: 1 addition & 11 deletions api/app/controllers/spree/api/checkouts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,8 @@ 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)
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
Expand All @@ -42,9 +38,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
Expand All @@ -57,12 +50,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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
Original file line number Diff line number Diff line change
@@ -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'
)
aldesantis marked this conversation as resolved.
Show resolved Hide resolved

json.error(I18n.t(:could_not_transition, scope: "spree.api.order"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe someone is using this template in their store, we should keep it, print a deprecation warning and render the generic partial.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can also add some specs for other resources' controllers that have state machines? Just to be really sure it's working with any of them.

I could not find any other controller which raise an exception because of the failed state machine transition.

There are controllers that prevent any exception by controlling the flow:
return_authorization#cancel

inventory_unit prepare event

shipment#ready

They also return a different message so it will be a breaking change to make them use the generic error message.

Added deprecation for order error messages:
https://github.com/solidusio/solidus/pull/3520/files#diff-72139884662b996fcc462020939b7807R195

json.errors(@order.errors.to_hash)
2 changes: 2 additions & 0 deletions api/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
14 changes: 13 additions & 1 deletion api/spec/requests/spree/api/checkouts_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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/)
Expand Down Expand Up @@ -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

Expand Down
7 changes: 7 additions & 0 deletions api/spec/requests/spree/api/orders_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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