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.

Sign up for GitHub

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

Jump to bottom

Rescue state machine exception from api base controller #3520

Merged
merged 4 commits into from
Jul 10, 2020
Merged

Rescue state machine exception from api base controller #3520

merged 4 commits into from
Jul 10, 2020

Conversation

SamuelMartini
Copy link
Contributor

@SamuelMartini SamuelMartini commented Feb 21, 2020 •
edited
Loading

Description
When an order is not completed cannot be transitioned via cancel!

OrdersController#cancel call @order.cancel! through canceled_by: the state machine
exception results in a 500 error when the action is called with a not completed order.

Pr changes:

  • rescue StateMachines::InvalidTransition exception from the BaseController returning the default json response and 422 status.
  • Remove rescue StateMachines::InvalidTransition from Api/CheckoutsController. Note that this change remove the log message from the update action when the state machine raise an exception. However the invaid_transition method has an explicit message that should work:

Screenshot 2020-03-03 at 18 26 02

The first string in the above image is the removed log message. The last string is what is logged in `invalid_transition`.
  • Remove a double authorization in the next action.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change (if needed)

@SamuelMartini SamuelMartini force-pushed the SamuelMartini/rescue_cancel_order branch from 986b485 to 22c8e18 Compare February 21, 2020 14:45
spaghetticode
spaghetticode reviewed Feb 21, 2020
View reviewed changes
Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

@SamuelMartini thanks for this PR 🎉
The fix looks good, but I left a note for possible improvement.

api/app/controllers/spree/api/orders_controller.rb Outdated Show resolved Hide resolved
@SamuelMartini SamuelMartini force-pushed the SamuelMartini/rescue_cancel_order branch from 22c8e18 to 6b76ec6 Compare February 28, 2020 09:31
@SamuelMartini SamuelMartini changed the title Rescue cancel! transition from orders controller Rescue state machine exception from base controller Mar 3, 2020
@kennyadsl kennyadsl changed the title Rescue state machine exception from base controller Rescue state machine exception from api base controller Mar 3, 2020
kennyadsl
kennyadsl previously approved these changes Mar 3, 2020
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks Samuel!

@kennyadsl kennyadsl dismissed their stale review March 3, 2020 11:23

Dismissing approval, since I found something to discuss

kennyadsl
kennyadsl requested changes Mar 3, 2020
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

I've found a possible issue with current code. Let me know!

api/app/controllers/spree/api/base_controller.rb Outdated Show resolved Hide resolved
@kennyadsl kennyadsl added the changelog:solidus_api Changes to the solidus_api gem label Mar 3, 2020
@SamuelMartini SamuelMartini force-pushed the SamuelMartini/rescue_cancel_order branch from 6b76ec6 to 7737e6c Compare March 4, 2020 14:47
kennyadsl
kennyadsl reviewed Mar 4, 2020
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

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.

api/app/views/spree/api/orders/could_not_transition.json.jbuilder
@@ -1,4 +0,0 @@
# frozen_string_literal: true

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

@SamuelMartini SamuelMartini force-pushed the SamuelMartini/rescue_cancel_order branch from 7737e6c to 85538cd Compare March 12, 2020 15:53
@SamuelMartini SamuelMartini force-pushed the SamuelMartini/rescue_cancel_order branch 2 times, most recently from 54e88e7 to faa487d Compare March 13, 2020 09:10
@SamuelMartini SamuelMartini mentioned this pull request Mar 13, 2020
Merged
2 tasks
kennyadsl
kennyadsl reviewed Apr 8, 2020
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

I have a question on how the deprecation has been implemented, maybe I'm missing something. Thanks Samuel!

api/app/controllers/spree/api/base_controller.rb Outdated Show resolved Hide resolved
@aldesantis aldesantis added this to the 2.11 milestone May 22, 2020
@aldesantis
Copy link
Member

@SamuelMartini if you have time, could you please finalize this by addressing @kennyadsl's comment? It would be great to get this into 2.11 so we can switch to the new behavior in 3.0!

@DanielePalombo DanielePalombo force-pushed the SamuelMartini/rescue_cancel_order branch from 4eaa001 to 3121604 Compare July 3, 2020 14:42
@DanielePalombo DanielePalombo force-pushed the SamuelMartini/rescue_cancel_order branch from 3121604 to 5cca7c1 Compare July 3, 2020 15:35
SamuelMartini and others added 4 commits July 10, 2020 11:34
@SamuelMartini @DanielePalombo
Rescue from invalid state machine exception in base controller
d54ab89 
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.
@SamuelMartini @DanielePalombo
Change template for checkout controller
496e22d 
Remove state machine exception rescue as it is now rescued from Api/BaseController
@SamuelMartini @DanielePalombo
Remove double authorization
18b5370 
Authorization already happen on the first line after the method definition
@DanielePalombo
Deprecate orders/could_not_transition view
e51fe03
@DanielePalombo DanielePalombo force-pushed the SamuelMartini/rescue_cancel_order branch from 5cca7c1 to e51fe03 Compare July 10, 2020 12:45
kennyadsl
kennyadsl approved these changes Jul 10, 2020
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks Samuel and Daniele!

@kennyadsl kennyadsl merged commit a45c362 into solidusio:master Jul 10, 2020
@kennyadsl kennyadsl deleted the SamuelMartini/rescue_cancel_order branch July 10, 2020 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@DanielePalombo DanielePalombo DanielePalombo left review comments

@spaghetticode spaghetticode spaghetticode left review comments

@aldesantis aldesantis aldesantis approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

Assignees
No one assigned
Labels
changelog:solidus_api Changes to the solidus_api gem
Projects
None yet
Milestone
2.11
Development

Successfully merging this pull request may close these issues.
6 participants
@SamuelMartini @aldesantis @spaghetticode @kennyadsl @DanielePalombo @houndci-bot