Remove state machine gem from Spree::InventoryUnit #2668
Conversation
| end | ||
|
|
||
| def can_cancel? | ||
| CANCELABLE_STATES.include?(state) |
There was a problem hiding this comment.
Later we should change CANCELABLE_STATES to be a method as stores might want to change that.
Ideally this would even be configurable, so stores won't have to class_eval or Module#prepend this class
|
|
||
| PRE_SHIPMENT_STATES = [BACKORDERED, ON_HAND] | ||
| POST_SHIPMENT_STATES = [RETURNED] | ||
| CANCELABLE_STATES = [ON_HAND, BACKORDERED, SHIPPED] |
There was a problem hiding this comment.
I know this is just keeping things like before, but we should consider to make this configurable in the future. Either via a method or configuration.
jgayfer
force-pushed
the
remove_inventory_unit_state_machine
branch
from
3af5fb6 to
30765e4
Compare
| @@ -155,5 +221,11 @@ def ensure_can_destroy | |||
| throw :abort | |||
| end | |||
| end | |||
|
|
|||
| def change_state!(new_state) | |||
| previous_state = state | |||
There was a problem hiding this comment.
Can we try to make use of ActiveModel::Dirty here?
|
|
||
| class AddDefaultStateToInventoryUnit < ActiveRecord::Migration[5.1] | ||
| def change | ||
| change_column_default(:spree_inventory_units, :state, Spree::InventoryUnit::ON_HAND) |
There was a problem hiding this comment.
We should use the value of the constant here so we do not rely on the code may change in the future.
jgayfer
force-pushed
the
remove_inventory_unit_state_machine
branch
from
30765e4 to
4e56678
Compare
| @@ -26,18 +34,19 @@ def order=(_) | |||
| end | |||
|
|
|||
| validates_presence_of :shipment, :line_item, :variant | |||
| validates :state, inclusion: { in: [ON_HAND, BACKORDERED, RETURNED, SHIPPED, CANCELED] } | |||
There was a problem hiding this comment.
The states may change for stores. We should allow them to overwrite a method instead.
jgayfer
force-pushed
the
remove_inventory_unit_state_machine
branch
from
4e56678 to
27aed26
Compare
The state machine is a problem for a few reasons: transitions are hard to understand and debug, transition order is tough to control, and database transactions during a transition aren't well understood. Removing it brings increased code clarity, as well as the ability to more easily extend the functionality that was previously hidden by the state machine.
jgayfer
force-pushed
the
remove_inventory_unit_state_machine
branch
from
27aed26 to
57c67fd
Compare
|
@tvdeyen Changes have been made 👍 |
|
reopened in #3041 |
This PR removes the state machine gem from
Spree::InventoryUnitwhile keeping the external API intact. The logic that was previously hidden within the state machine is now contained within theSpree::InventoryUnitmodel.Please see #2656 for the rationale behind these changes.