Remove state machine gem from Spree::Shipment #2656

jgayfer · 2018-03-27T20:37:21Z

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

houndci-bot · 2018-03-27T20:37:30Z

core/spec/models/spree/shipment_spec.rb

+
+    context 'when state is ready' do
+      before { shipment.state = 'ready' }
+      it { is_expected.to be true}


Space missing inside }.

tvdeyen

I love the explicitness of this change and think this is the right thing to do.

Have some small change requests and the recommendation to move shared functionality of state changes into a module.

But otherwise very nice work 👌

tvdeyen · 2018-04-04T06:58:47Z

core/app/models/spree/shipment.rb

      !requires_shipment?
    end

    def can_transition_from_pending_to_ready?
+      Spree::Deprecation.warn \


There is a nice little class method from ActiveSupport

def can_transition_from_pending_to_ready? ... end deprecate can_transition_from_pending_to_ready?: :inventory_can_ship?, deprecator: Spree::Deprecation

could we use that instead?

tvdeyen · 2018-04-04T07:00:36Z

core/app/models/spree/shipment.rb

      !requires_shipment?
    end

    def can_transition_from_pending_to_ready?
+      Spree::Deprecation.warn \
+        'Spree::Shipment#can_transition_from_pending_to_ready? is deprecated. ' \
+        'Use Spree::Shipment#inventory_can_ship? instead.'
      order.can_ship? &&
        inventory_units.all? { |iu| iu.shipped? || iu.allow_ship? || iu.canceled? } &&
        (order.paid? || !Spree::Config[:require_payment_to_ship])


could we call inventory_can_ship? in here?

tvdeyen · 2018-04-04T07:04:52Z

core/app/models/spree/shipment.rb

@@ -14,6 +21,10 @@ class Shipment < Spree::Base
    has_many :state_changes, as: :stateful
    has_many :cartons, -> { uniq }, through: :inventory_units

+    validates :state, presence: true, inclusion: { in: [READY, PENDING, SHIPPED, CANCELED] }


presence: true is superfluous if we also check the inclusion in something that's not having nil.

Maybe we could also add default value for the state column?

tvdeyen · 2018-04-04T07:05:17Z

core/app/models/spree/shipment.rb

@@ -14,6 +21,10 @@ class Shipment < Spree::Base
    has_many :state_changes, as: :stateful
    has_many :cartons, -> { uniq }, through: :inventory_units

+    validates :state, presence: true, inclusion: { in: [READY, PENDING, SHIPPED, CANCELED] }
+
+    after_initialize :set_default_state


We could have a default value for the state column instead.

tvdeyen · 2018-04-04T07:08:57Z

core/app/models/spree/shipment.rb

+        update_attributes(
+          state: new_state,
+          updated_at: Time.current
+        )


ActiveRecord already sets updated_at for us.

Also, the bang at the end of the method name indicates that this method raises if the record couldn't be saved. We should use update! here.

update!(state: new_state)

We should consider to move the methods change_state! and store_state_change into a module we share with all models that have state changes.

tvdeyen · 2018-04-04T07:13:30Z

core/app/models/spree/shipment.rb

    def can_transition_from_pending_to_shipped?
+      Spree::Deprecation.warn \
+        'Spree::Shipment#can_transition_from_pending_to_shipped is deprecated. ' \
+        'It is equivalent to the negation of Spree::Shipment#requires_shipment?'


tvdeyen · 2018-04-04T07:15:02Z

core/app/models/spree/shipment.rb

+    end
+
+    def pend!
+      pend || (raise InvalidStateChange)


weird usage of parenthesis here.

Either

raise InvalidStateChange

or

raise(InvalidStateChange)

tvdeyen · 2018-04-04T07:15:46Z

core/app/models/spree/shipment.rb

+    end
+
+    def ready!
+      ready || (raise InvalidStateChange)


Ditto, weird usage of parenthesis

tvdeyen · 2018-04-04T07:16:37Z

core/app/models/spree/shipment.rb

+    end
+
+    def ship!
+      ship || (raise InvalidStateChange)


Ditto, parenthesis

tvdeyen · 2018-04-04T07:22:26Z

core/app/models/spree/shipment.rb

+      end
+    end
+
+    def set_default_state


A database default would be preferable

houndci-bot · 2018-04-04T21:32:40Z

core/app/models/spree/shipment.rb

@@ -14,6 +21,10 @@ class Shipment < Spree::Base
    has_many :state_changes, as: :stateful
    has_many :cartons, -> { uniq }, through: :inventory_units

+    validates :state, inclusion: { in: [READY, PENDING, SHIPPED, CANCELED] }
+
+    #after_initialize :set_default_state


Layout/LeadingCommentSpace: Missing space after #.

jgayfer · 2018-04-06T16:47:12Z

@tvdeyen Thanks for all the great feedback! I've gone ahead and made most of the requested changes.

I haven't pulled out the change_state! method into a module yet, as I wasn't able to decide on the best way to do so. While there is shared functionality in the change_state! method, the four models in question use it a bit differently. For example, not all of the models need to create a StateChange, creating a StateChange requires a value for it's name, and Reimbursement uses the reimbursement_status attribute instead of state. It's entirely possible to pull it out into a module, but because of the reasons just mentioned, my initial sketch had more parameters than I would have liked.

With all that being said, I'd love to hear any insight you may have on a better way to pull out the shared functionality!

Lastly, in regards to things like CANCELABLE_STATES on inventory units (and other constants) and potentially turning them into methods (or adding them to the config), I haven't made any changes yet. I wasn't sure if this was something we were wanting to look into now, or if it should be a future change instead.

Thanks again for taking time to review my work

tvdeyen

Thanks @jgayfer this makes total sense to me.

I still have questions about using ActiveModel::Dirty, but this is not mandatory as this still works without, but maybe worth trying.

Thanks for your work on this 👏

tvdeyen · 2018-04-20T07:45:35Z

core/db/migrate/20180404185904_add_default_state_to_shipment.rb

+
+class AddDefaultStateToShipment < ActiveRecord::Migration[5.1]
+  def change
+    change_column_default(:spree_shipments, :state, Spree::Shipment::PENDING)


We usually don't want to rely on code inside the migrations. Although very unlikely that the value for Spree::Shipment::PENDING will change ever, it still is more reliable to use the value here instead.

tvdeyen · 2018-04-20T07:47:33Z

core/app/models/spree/shipment.rb

+
+    def change_state!(new_state)
+      previous_state = state
+      return if new_state == previous_state


We may can make use of ActiveModel::Dirty here?

http://api.rubyonrails.org/classes/ActiveModel/Dirty.html

tvdeyen · 2018-04-20T07:48:04Z

core/app/models/spree/shipment.rb

+      previous_state = state
+      change_state!(SHIPPED)
+      after_ship
+      after_resume if previous_state == CANCELED


Can we try to make use of ActiveModel::Dirty here?

http://api.rubyonrails.org/classes/ActiveModel/Dirty.html

ccarruitero · 2018-05-08T02:53:12Z

I'm not agree with remove the state_machines gem.

I feel that reimplementing state_machines functionality on each model is something repetitive.
Aside, we are assuming that everyone want to use the default states, I think if someone require states completely different for their use case, would be necessary more work to do than is currently.

Like you mention maybe state_machines is difficult to understand, but instead remove I think we should improve documentation in that field.

Finally if you want to remove state_machines, I think a better way could be find and alternative or do a module replacement for that, and allow configure in some way what to use for the states.

houndci-bot · 2018-05-18T16:49:05Z

core/spec/models/spree/shipment_spec.rb

+
+    it { is_expected.to include carton }
+  end
+


Layout/TrailingWhitespace: Trailing whitespace detected.

jgayfer · 2018-05-18T22:55:38Z

@tvdeyen I made the change regarding the database migration (which definitely makes sense). I will make this change to my other PRs early next week.

Regarding ActiveModel::Dirty, I just want to confirm that the goal is to use it to check what the previous state was without having to assign an extra variable? (For example, when I use previous_state to make note of what the original state was).

tvdeyen · 2018-05-22T18:57:09Z

Regarding ActiveModel::Dirty, I just want to confirm that the goal is to use it to check what the previous state was without having to assign an extra variable? (For example, when I use previous_state to make note of what the original state was).

@jgayfer yes. Like I said not mandatory, but still worth trying, as we are duplicating AR behavior here.

tvdeyen

Would you please extract this state validation into a method? Thanks

tvdeyen · 2018-06-07T06:02:30Z

core/app/models/spree/shipment.rb

@@ -14,6 +21,8 @@ class Shipment < Spree::Base
    has_many :state_changes, as: :stateful
    has_many :cartons, -> { distinct }, through: :inventory_units

+    validates :state, inclusion: { in: [READY, PENDING, SHIPPED, CANCELED] }


We should extract this into a method, as stores might want to extend/change states.

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.

jgayfer · 2018-06-07T20:56:25Z

@tvdeyen Changes have been made.

I wasn't entirely sure exactly how someone might want to go about changing what states are valid, so I tried to keep the options open.

There is a new constant VALID_STATES which can be overridden should someone desire to change what states are allowed. I also added a validation function validate_state which checks to see if the current state is included in VALID_STATES. The inclusion of this function allows for a potentially more detailed custom state validation process as changing the VALID_STATES constant may not cover every use case.

tvdeyen

I wasn't entirely sure exactly how someone might want to go about changing what states are valid, so I tried to keep the options open.

@jgayfer lots of stores add states to core models. Additional shipment states, payment states, etc.

Please let's not encourage people to overwrite constants as extension point. That's why I think we should rename this constant.

tvdeyen · 2018-06-08T14:26:00Z

core/app/models/spree/shipment.rb

@@ -14,6 +22,8 @@ class Shipment < Spree::Base
    has_many :state_changes, as: :stateful
    has_many :cartons, -> { distinct }, through: :inventory_units

+    validate :validate_state


Can we call this method valid_state or is_valid_state? Reads better.

tvdeyen · 2018-06-08T14:28:07Z

core/app/models/spree/shipment.rb

+    PENDING = 'pending'
+    SHIPPED = 'shipped'
+    CANCELED = 'canceled'
+    VALID_STATES = [READY, PENDING, SHIPPED, CANCELED]


Can we rename this constant to DEFAULT_STATES? We don't want to give the impression that overwriting a constant is a good extension point (because it's not)

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?`.

jgayfer · 2018-06-08T18:50:32Z

@tvdeyen Changes have been made 👍

tvdeyen

Thanks 👌

kennyadsl · 2019-01-30T15:58:18Z

reopened in #3038

houndci-bot reviewed Mar 27, 2018

View reviewed changes

jgayfer force-pushed the remove_shipment_state_machine branch 2 times, most recently from 10177b8 to 0b44a29 Compare March 28, 2018 19:47

This was referenced Mar 29, 2018

Remove state machine gem from Spree::Payment #2664

Closed

Remove state machine gem from Spree::Reimbursement #2666

Closed

Remove state machine gem from Spree::InventoryUnit #2668

Closed

tvdeyen requested changes Apr 4, 2018

View reviewed changes

jgayfer force-pushed the remove_shipment_state_machine branch from 0b44a29 to 0a9a56c Compare April 4, 2018 21:32

houndci-bot reviewed Apr 4, 2018

View reviewed changes

jgayfer force-pushed the remove_shipment_state_machine branch from 0a9a56c to f0cc82b Compare April 4, 2018 21:37

tvdeyen reviewed Apr 20, 2018

View reviewed changes

tvdeyen approved these changes Apr 20, 2018

View reviewed changes

jgayfer force-pushed the remove_shipment_state_machine branch from f0cc82b to 515288c Compare May 18, 2018 16:48

houndci-bot reviewed May 18, 2018

View reviewed changes

jgayfer force-pushed the remove_shipment_state_machine branch 2 times, most recently from 436f8e8 to dade297 Compare May 18, 2018 22:46

tvdeyen requested changes Jun 7, 2018

View reviewed changes

jgayfer force-pushed the remove_shipment_state_machine branch from dade297 to 8398bba Compare June 7, 2018 18:21

tvdeyen requested changes Jun 8, 2018

View reviewed changes

James Gayfer added 3 commits June 8, 2018 09:24

Deprecate transition methods in Spree::Shipment

8cea551

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?`.

jgayfer force-pushed the remove_shipment_state_machine branch from 8398bba to 8cea551 Compare June 8, 2018 16:24

tvdeyen approved these changes Jun 8, 2018

View reviewed changes

tvdeyen added this to the 3.0 milestone Jun 13, 2018

kennyadsl closed this Jan 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove state machine gem from Spree::Shipment #2656

Remove state machine gem from Spree::Shipment #2656

jgayfer commented Mar 27, 2018

houndci-bot Mar 27, 2018

tvdeyen left a comment

tvdeyen Apr 4, 2018

tvdeyen Apr 4, 2018

tvdeyen Apr 4, 2018

tvdeyen Apr 4, 2018

tvdeyen Apr 4, 2018

tvdeyen Apr 4, 2018

tvdeyen Apr 4, 2018

tvdeyen Apr 4, 2018

tvdeyen Apr 4, 2018

tvdeyen Apr 4, 2018

tvdeyen Apr 4, 2018

houndci-bot Apr 4, 2018

jgayfer commented Apr 6, 2018

tvdeyen left a comment

tvdeyen Apr 20, 2018

tvdeyen Apr 20, 2018

tvdeyen Apr 20, 2018

ccarruitero commented May 8, 2018

houndci-bot May 18, 2018

jgayfer commented May 18, 2018

tvdeyen commented May 22, 2018

tvdeyen left a comment

tvdeyen Jun 7, 2018

jgayfer commented Jun 7, 2018

tvdeyen left a comment

tvdeyen Jun 8, 2018

tvdeyen Jun 8, 2018

jgayfer commented Jun 8, 2018

tvdeyen left a comment

kennyadsl commented Jan 30, 2019

Remove state machine gem from Spree::Shipment #2656

Remove state machine gem from Spree::Shipment #2656

Conversation

jgayfer commented Mar 27, 2018

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgayfer commented Apr 6, 2018

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccarruitero commented May 8, 2018

Choose a reason for hiding this comment

jgayfer commented May 18, 2018

tvdeyen commented May 22, 2018

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgayfer commented Jun 7, 2018

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgayfer commented Jun 8, 2018

tvdeyen left a comment

Choose a reason for hiding this comment

kennyadsl commented Jan 30, 2019