From 57c67fdad7d3530663158fad783160ccdaef66c7 Mon Sep 17 00:00:00 2001 From: James Gayfer Date: Tue, 3 Apr 2018 11:29:36 -0700 Subject: [PATCH] Remove state machine gem from Spree::InventoryUnit 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. --- core/app/models/spree/inventory_unit.rb | 127 +++++++-- ...459_add_default_state_to_inventory_unit.rb | 7 + core/spec/models/spree/inventory_unit_spec.rb | 255 ++++++++++++++++++ 3 files changed, 365 insertions(+), 24 deletions(-) create mode 100644 core/db/migrate/20180405181459_add_default_state_to_inventory_unit.rb diff --git a/core/app/models/spree/inventory_unit.rb b/core/app/models/spree/inventory_unit.rb index 45a9bc05aab..fb95e7cec90 100644 --- a/core/app/models/spree/inventory_unit.rb +++ b/core/app/models/spree/inventory_unit.rb @@ -4,9 +4,18 @@ module Spree # Tracks the state of line items' fulfillment. # class InventoryUnit < Spree::Base - PRE_SHIPMENT_STATES = %w(backordered on_hand) - POST_SHIPMENT_STATES = %w(returned) - CANCELABLE_STATES = ['on_hand', 'backordered', 'shipped'] + class InvalidStateChange < StandardError; end + + ON_HAND = 'on_hand' + BACKORDERED = 'backordered' + RETURNED = 'returned' + SHIPPED = 'shipped' + CANCELED = 'canceled' + DEFAULT_STATES = [ON_HAND, BACKORDERED, RETURNED, SHIPPED, CANCELED] + + PRE_SHIPMENT_STATES = [BACKORDERED, ON_HAND] + POST_SHIPMENT_STATES = [RETURNED] + CANCELABLE_STATES = [ON_HAND, BACKORDERED, SHIPPED] belongs_to :variant, -> { with_deleted }, class_name: "Spree::Variant", inverse_of: :inventory_units belongs_to :shipment, class_name: "Spree::Shipment", touch: true, inverse_of: :inventory_units @@ -26,18 +35,19 @@ def order=(_) end validates_presence_of :shipment, :line_item, :variant + validate :is_valid_state? before_destroy :ensure_can_destroy scope :pending, -> { where pending: true } - scope :backordered, -> { where state: 'backordered' } - scope :on_hand, -> { where state: 'on_hand' } + scope :backordered, -> { where state: BACKORDERED } + scope :on_hand, -> { where state: ON_HAND } scope :pre_shipment, -> { where(state: PRE_SHIPMENT_STATES) } - scope :shipped, -> { where state: 'shipped' } + scope :shipped, -> { where state: SHIPPED } scope :post_shipment, -> { where(state: POST_SHIPMENT_STATES) } - scope :returned, -> { where state: 'returned' } - scope :canceled, -> { where(state: 'canceled') } - scope :not_canceled, -> { where.not(state: 'canceled') } + scope :returned, -> { where state: RETURNED } + scope :canceled, -> { where(state: CANCELED) } + scope :not_canceled, -> { where.not(state: CANCELED) } scope :cancelable, -> { where(state: Spree::InventoryUnit::CANCELABLE_STATES, pending: false) } scope :backordered_per_variant, ->(stock_item) do includes(:shipment, :order) @@ -59,24 +69,81 @@ def order=(_) scope :shippable, -> { on_hand } - # state machine (see http://github.com/pluginaweek/state_machine/tree/master for details) - state_machine initial: :on_hand do - event :fill_backorder do - transition to: :on_hand, from: :backordered - end - after_transition on: :fill_backorder, do: :fulfill_order + def fill_backorder! + fill_backorder || raise(InvalidStateChange) + end - event :ship do - transition to: :shipped, if: :allow_ship? - end + def fill_backorder + return false unless can_fill_backorder? + change_state!(ON_HAND) + fulfill_order + true + end - event :return do - transition to: :returned, from: :shipped - end + def can_fill_backorder? + backordered? + end - event :cancel do - transition to: :canceled, from: CANCELABLE_STATES.map(&:to_sym) - end + def on_hand? + state == ON_HAND + end + + def backordered? + state == BACKORDERED + end + + def ship! + ship || raise(InvalidStateChange) + end + + def ship + return false unless can_ship? + change_state!(SHIPPED) + true + end + + def can_ship? + allow_ship? + end + + def shipped? + state == SHIPPED + end + + def return! + self.return || raise(InvalidStateChange) + end + + def return + return false unless can_return? + change_state!(RETURNED) + true + end + + def can_return? + shipped? + end + + def returned? + state == RETURNED + end + + def cancel! + cancel || raise(InvalidStateChange) + end + + def cancel + return false unless can_cancel? + change_state!(CANCELED) + true + end + + def can_cancel? + CANCELABLE_STATES.include?(state) + end + + def canceled? + state == CANCELED end # Updates the given inventory units to not be pending. @@ -155,5 +222,17 @@ def ensure_can_destroy throw :abort end end + + def change_state!(new_state) + previous_state = state + return if new_state == previous_state + update!(state: new_state) + end + + def is_valid_state? + unless DEFAULT_STATES.include?(state) + errors.add(:state, "Invalid state") + end + end end end diff --git a/core/db/migrate/20180405181459_add_default_state_to_inventory_unit.rb b/core/db/migrate/20180405181459_add_default_state_to_inventory_unit.rb new file mode 100644 index 00000000000..eded180c40b --- /dev/null +++ b/core/db/migrate/20180405181459_add_default_state_to_inventory_unit.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddDefaultStateToInventoryUnit < ActiveRecord::Migration[5.1] + def change + change_column_default(:spree_inventory_units, :state, 'on_hand') + end +end diff --git a/core/spec/models/spree/inventory_unit_spec.rb b/core/spec/models/spree/inventory_unit_spec.rb index ac0b96edc60..727d9fb87d4 100644 --- a/core/spec/models/spree/inventory_unit_spec.rb +++ b/core/spec/models/spree/inventory_unit_spec.rb @@ -6,6 +6,7 @@ let(:stock_location) { create(:stock_location_with_items) } let(:stock_item) { stock_location.stock_items.order(:id).first } let(:line_item) { create(:line_item, variant: stock_item.variant) } + let(:inventory_unit) { create(:inventory_unit) } describe ".cancelable" do let!(:pending_unit) { create(:inventory_unit, pending: true) } @@ -309,4 +310,258 @@ expect { inventory_unit.reload }.not_to raise_error end end + + describe '#fill_backorder!' do + subject { inventory_unit.fill_backorder! } + + before { inventory_unit.state = 'backordered' } + + it { is_expected.to be true } + + context 'when not able to fill backorder' do + before { inventory_unit.state = 'shipped' } + it 'raises an exception' do + expect { subject }.to raise_error(Spree::InventoryUnit::InvalidStateChange) + end + end + end + + describe '#fill_backorder' do + subject { inventory_unit.fill_backorder } + + before { inventory_unit.state = 'backordered' } + + it { is_expected.to be true } + + it 'changes the state' do + subject + expect(inventory_unit.state).to eq 'on_hand' + end + + context 'when not able to fill backorder' do + before { inventory_unit.state = 'shipped' } + it { is_expected.to be false } + end + end + + describe '#can_fill_backorder?' do + subject { inventory_unit.can_fill_backorder? } + + context 'when state is backordered' do + before { inventory_unit.state = 'backordered' } + it { is_expected.to be true } + end + + ['shipped', 'canceled', 'returned', 'on_hand'].each do |state| + context "when state is #{state}" do + before { inventory_unit.state = state } + it { is_expected.to be false } + end + end + end + + describe '#on_hand?' do + subject { inventory_unit.on_hand? } + + it { is_expected.to be true } + + context 'when not on hand' do + before { inventory_unit.state = 'backordered' } + it { is_expected.to be false } + end + end + + describe '#backordered?' do + subject { inventory_unit.backordered? } + + before { inventory_unit.state = 'backordered' } + + it { is_expected.to be true } + + context 'when not backordered' do + before { inventory_unit.state = 'on_hand' } + it { is_expected.to be false } + end + end + + describe '#ship!' do + subject { inventory_unit.ship! } + + it { is_expected.to be true } + + context 'when not able to ship' do + before { inventory_unit.state = 'shipped' } + it 'raises an exception' do + expect { subject }.to raise_error(Spree::InventoryUnit::InvalidStateChange) + end + end + end + + describe '#ship' do + subject { inventory_unit.ship } + + it { is_expected.to be true } + + it 'changes the state' do + subject + expect(inventory_unit.state).to eq 'shipped' + end + + context 'when not able to ship' do + before { inventory_unit.state = 'shipped' } + it { is_expected.to be false } + end + end + + describe '#can_ship?' do + subject { inventory_unit.can_ship? } + + it { is_expected.to be true } + + ['shipped', 'canceled', 'returned', 'backordered'].each do |state| + context "when state is #{state}" do + before { inventory_unit.state = state } + it { is_expected.to be false } + end + end + end + + describe '#shipped?' do + subject { inventory_unit.shipped? } + + before { inventory_unit.state = 'shipped' } + + it { is_expected.to be true } + + context 'when not shipped' do + before { inventory_unit.state = 'on_hand' } + it { is_expected.to be false } + end + end + + describe '#return!' do + subject { inventory_unit.return! } + + before { inventory_unit.state = 'shipped' } + + it { is_expected.to be true } + + context 'when not able to return' do + before { inventory_unit.state = 'on_hand' } + it 'raises an exception' do + expect { subject }.to raise_error(Spree::InventoryUnit::InvalidStateChange) + end + end + end + + describe '#return' do + subject { inventory_unit.return } + + before { inventory_unit.state = 'shipped' } + + it { is_expected.to be true } + + it 'changes the state' do + subject + expect(inventory_unit.state).to eq 'returned' + end + + context 'when not able to return' do + before { inventory_unit.state = 'on_hand' } + it { is_expected.to be false } + end + end + + describe '#can_return?' do + subject { inventory_unit.can_return? } + + context 'when state is shipped' do + before { inventory_unit.state = 'shipped' } + it { is_expected.to be true } + end + + ['on_hand', 'canceled', 'returned', 'backordered'].each do |state| + context "when state is #{state}" do + before { inventory_unit.state = state } + it { is_expected.to be false } + end + end + end + + describe '#returned?' do + subject { inventory_unit.returned? } + + before { inventory_unit.state = 'returned' } + + it { is_expected.to be true } + + context 'when not returned' do + before { inventory_unit.state = 'on_hand' } + it { is_expected.to be false } + end + end + + describe '#cancel!' do + subject { inventory_unit.cancel! } + + before { inventory_unit.state = 'on_hand' } + + it { is_expected.to be true } + + context 'when not able to cancel' do + before { inventory_unit.state = 'returned' } + it 'raises an exception' do + expect { subject }.to raise_error(Spree::InventoryUnit::InvalidStateChange) + end + end + end + + describe '#cancel' do + subject { inventory_unit.cancel } + + before { inventory_unit.state = 'on_hand' } + + it { is_expected.to be true } + + it 'changes the state' do + subject + expect(inventory_unit.state).to eq 'canceled' + end + + context 'when not able to cancel' do + before { inventory_unit.state = 'returned' } + it { is_expected.to be false } + end + end + + describe '#can_cancel?' do + subject { inventory_unit.can_cancel? } + + ['on_hand', 'shipped', 'backordered'].each do |state| + context "when state is #{state}" do + before { inventory_unit.state = state } + it { is_expected.to be true } + end + end + + ['canceled', 'returned'].each do |state| + context "when state is #{state}" do + before { inventory_unit.state = state } + it { is_expected.to be false } + end + end + end + + describe '#canceled?' do + subject { inventory_unit.canceled? } + + before { inventory_unit.state = 'canceled' } + + it { is_expected.to be true } + + context 'when not canceled' do + before { inventory_unit.state = 'on_hand' } + it { is_expected.to be false } + end + end end