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

Remove state machine gem from Spree::Shipment (updated PR #2656) #3038

Closed
wants to merge 4 commits into from

Conversation

cedum
Copy link
Contributor

@cedum cedum commented Jan 16, 2019

This resolves the existing conflict and rebases from the latest master the PR #2656 by @jgayfer:

The state_machines gem has been used throughout Solidus since the earliest days of Spree. We use it to track the "state" our shipments/payments/orders are in, as well as plug custom functionality into the transitions between those states (before_transition/after_transition).

However, our use of intermingled state machines between Payments, Shipments and Orders causes problems for both new and established developers. The state machine transitions are difficult to understand and debug, the order the transitions are executed in can be tough to control, and properly controlling database transactions during a transition is very poorly understood.

We believe the downsides of implementing our state machines using the state_machines gem to be a net negative for the project.

This PR changes the Spree::Shipment class, explicitly defining the important methods that would have been generated from the state_machines metaprogramming. The advantages of doing this are improved code clarity, debug-ability, as well as making it more easy to completely override methods regarding state.

The first two commits of this pull request remove the state_machine from Spree::Shipment without changing the external API of the model. The last two commits go a bit further and deprecate the transition methods that were used by the state machine in an attempt to improve code readability. If we don't want to go ahead and deprecate the transition methods, the first two commits can just be taken on their own, keeping the external API of Spree::Shipment intact.

While we could re-implement the before_transition/after_transition calls on the Spree::Shipment model, we believe the following examples would be a better way to support customising functionality before or after a transition (with ship used as an example).

module AfterShipAction
  # Note that returning true/false is done to preserve the original return values of ship
  # This pattern works for all of ship, resume, pend, ready, and cancel
  def ship
    if super
      additional_after_ship_procedure
    end
  end

  def additional_after_ship_procedure
    # Do something interesting!
    true
  end
end

module BeforeShipCondition
  def ship
    if pre_ship_condition
      super
    end
  end

  def pre_ship_condition
    # Some interesting condition as to when we can ship
    true
  end
end

Spree::Shipment.prepend AfterShipAction
Spree::Shipment.prepend BeforeShipCondition

@cedum cedum changed the title Remove shipment state machine Remove state machine gem from Spree::Shipment [updated PR #2656] Jan 16, 2019
@cedum cedum changed the title Remove state machine gem from Spree::Shipment [updated PR #2656] Remove state machine gem from Spree::Shipment (updated PR #2656) Jan 16, 2019
James Gayfer added 4 commits January 29, 2019 22:15
There are a few reasons for removing this reference. First, we
should be explicit about which values we want to test; reaching into the
state machine adds an extra layer of abstraction that reduces the
readability of the test. Second, by calling
`Spree::Shipment.state_machine`, we are actually instantiating a state
machine on the `Spree::Shipment` class, which should only ever happen
within the `Shipment` class itself.
The state machine gem is something that we don't want around long term,
as it has been known to cause headaches. The state machine in
`Spree::Shipment` is fairly straight forward, which makes it a good
candidate to try and remove. By removing the state machine gem, the
logic for states is now entirely contained within `Spree::Shipment`.
This helps the readability of the code and also makes it easier to
override behaviour that was previously buried within the state machine
gem.
These transition methods were used by the state machine gem. Semantically,
these methods made sense in that context. However, with the removal of
the state machine gem, these methods no longer read very well.

`inventory_can_ship?` is a new method introduced in this commit, which
is just a renaming of `can_transition_from_pending_to_ready?`. The two
other transition methods (`can_transition_from_canceled_to_ready?` and
`can_transition_from_pending_to_shipped?`) can be replaced with a call
to `!requires_shipment?`.

The end result is code that is much easier to parse as the the method
names are much more telling of what is actually happening.
These methods were used by the state machine gem, and no longer make
much semantic sense. The same functionality can be found in
`!requires_shipment?` and `inventory_can_ship?`.
@cedum cedum force-pushed the remove_shipment_state_machine branch from 140a3fa to 9cf129b Compare January 29, 2019 21:15
@kennyadsl kennyadsl added this to the 3.0 milestone May 10, 2019
@kennyadsl
Copy link
Member

Closing in favor of #3356, thanks @cedum!

@kennyadsl kennyadsl closed this Oct 3, 2019
@cedum cedum deleted the remove_shipment_state_machine branch October 4, 2019 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants