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 #2656

Closed
Closed
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
182 changes: 141 additions & 41 deletions core/app/models/spree/shipment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,14 @@ module Spree
# An order's planned shipments including tracking and cost.
#
class Shipment < Spree::Base
class InvalidStateChange < StandardError; end

READY = 'ready'
PENDING = 'pending'
SHIPPED = 'shipped'
CANCELED = 'canceled'
DEFAULT_STATES = [READY, PENDING, SHIPPED, CANCELED]

belongs_to :order, class_name: 'Spree::Order', touch: true, inverse_of: :shipments
belongs_to :stock_location, class_name: 'Spree::StockLocation'

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

validate :is_valid_state?

before_validation :set_cost_zero_when_nil

before_destroy :ensure_can_destroy
Expand All @@ -26,69 +36,138 @@ class Shipment < Spree::Base

make_permalink field: :number, length: 11, prefix: 'H'

scope :pending, -> { with_state('pending') }
scope :ready, -> { with_state('ready') }
scope :shipped, -> { with_state('shipped') }
scope :pending, -> { with_state(PENDING) }
scope :ready, -> { with_state(READY) }
scope :shipped, -> { with_state(SHIPPED) }
scope :trackable, -> { where("tracking IS NOT NULL AND tracking != ''") }
scope :with_state, ->(*s) { where(state: s) }
# sort by most recent shipped_at, falling back to created_at. add "id desc" to make specs that involve this scope more deterministic.
scope :reverse_chronological, -> { order('coalesce(spree_shipments.shipped_at, spree_shipments.created_at) desc', id: :desc) }
scope :by_store, ->(store) { joins(:order).merge(Spree::Order.by_store(store)) }

# shipment state machine (see http://github.com/pluginaweek/state_machine/tree/master for details)
state_machine initial: :pending, use_transactions: false do
event :ready do
transition from: :pending, to: :shipped, if: :can_transition_from_pending_to_shipped?
transition from: :pending, to: :ready, if: :can_transition_from_pending_to_ready?
end
def cancel!
cancel || raise(InvalidStateChange)
end

event :pend do
transition from: :ready, to: :pending
end
def cancel
return false unless can_cancel?
change_state!(CANCELED)
after_cancel
true
end

event :ship do
transition from: [:ready, :canceled], to: :shipped
end
after_transition to: :shipped, do: :after_ship
def can_cancel?
ready_or_pending?
end

event :cancel do
transition to: :canceled, from: [:pending, :ready]
end
after_transition to: :canceled, do: :after_cancel
def canceled?
state == CANCELED
end

event :resume do
transition from: :canceled, to: :ready, if: :can_transition_from_canceled_to_ready?
transition from: :canceled, to: :pending
end
after_transition from: :canceled, to: [:pending, :ready, :shipped], do: :after_resume
def resume!
resume || raise(InvalidStateChange)
end

after_transition do |shipment, transition|
shipment.state_changes.create!(
previous_state: transition.from,
next_state: transition.to,
name: 'shipment'
)
def resume
return false unless can_resume?
inventory_can_ship? ? change_state!(READY) : change_state!(PENDING)
after_resume
true
end

def can_resume?
canceled?
end

def ship!
ship || raise(InvalidStateChange)
end

def ship
return false unless can_ship?
previous_state = state
change_state!(SHIPPED)
after_ship
after_resume if previous_state == CANCELED
Copy link
Member

Choose a reason for hiding this comment

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

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

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

true
end

def can_ship?
ready? || canceled?
end

def shipped?
state == SHIPPED
end

def ready!
ready || raise(InvalidStateChange)
end

def ready
return false unless state == PENDING
if !requires_shipment?
change_state!(SHIPPED)
after_ship
elsif inventory_can_ship?
change_state!(READY)
else
return false
end
true
end

def ready?
state == READY
end

def can_ready?
pending? && (!requires_shipment? || inventory_can_ship?)
end

def pend!
pend || raise(InvalidStateChange)
end

def pend
return false unless can_pend?
change_state!(PENDING)
true
end

def pending?
state == PENDING
end

def can_pend?
ready?
end

self.whitelisted_ransackable_associations = ['order']
self.whitelisted_ransackable_attributes = ['number']

delegate :tax_category, :tax_category_id, to: :selected_shipping_rate, allow_nil: true

def inventory_can_ship?
order.can_ship? &&
inventory_units.all? { |iu| iu.shipped? || iu.allow_ship? || iu.canceled? } &&
(order.paid? || !Spree::Config[:require_payment_to_ship])
end

def can_transition_from_pending_to_shipped?
!requires_shipment?
end
deprecate can_transition_from_pending_to_shipped?: '!requires_shipment?', deprecator: Spree::Deprecation

def can_transition_from_pending_to_ready?
order.can_ship? &&
inventory_units.all? { |iu| iu.shipped? || iu.allow_ship? || iu.canceled? } &&
(order.paid? || !Spree::Config[:require_payment_to_ship])
inventory_can_ship?
end
deprecate can_transition_from_pending_to_ready?: :inventory_can_ship?, deprecator: Spree::Deprecation

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

extend DisplayMoney
money_methods(
Expand Down Expand Up @@ -264,13 +343,13 @@ def selected_shipping_rate_id=(id)
# shipped if already shipped (ie. does not change the state)
# ready all other cases
def determine_state(order)
return 'canceled' if order.canceled?
return 'shipped' if shipped?
return 'pending' unless order.can_ship?
if can_transition_from_pending_to_ready?
'ready'
return CANCELED if order.canceled?
return SHIPPED if shipped?
return PENDING unless order.can_ship?
if inventory_can_ship?
READY
else
'pending'
PENDING
end
end

Expand Down Expand Up @@ -355,7 +434,7 @@ def update_state
state: new_state,
updated_at: Time.current
)
after_ship if new_state == 'shipped'
after_ship if new_state == SHIPPED
end
end

Expand Down Expand Up @@ -430,5 +509,26 @@ def ensure_can_destroy
throw :abort
end
end

def store_state_change(previous_state, new_state)
state_changes.create!(
previous_state: previous_state,
next_state: new_state,
name: 'shipment'
)
end

def change_state!(new_state)
previous_state = state
return if new_state == previous_state
Copy link
Member

Choose a reason for hiding this comment

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

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

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

update!(state: new_state)
store_state_change(previous_state, new_state)
end

def is_valid_state?
unless DEFAULT_STATES.include?(state)
errors.add(:state, "Invalid state")
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

class AddDefaultStateToShipment < ActiveRecord::Migration[5.1]
def change
change_column_default(:spree_shipments, :state, 'pending')
end
end
2 changes: 1 addition & 1 deletion core/spec/models/spree/order/state_machine_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
end
end

(Spree::Shipment.state_machine.states.keys - states).each do |shipment_state|
['shipped', 'canceled'].each do |shipment_state|
it "should be false if shipment_state is #{shipment_state}" do
expect(order).to be_completed
order.shipment_state = shipment_state
Expand Down
Loading