Remove state machine gem from Spree::Reimbursement

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/reimbursement.rb

@@ -3,6 +3,12 @@
 module Spree
   class Reimbursement < Spree::Base
     class IncompleteReimbursementError < StandardError; end
+    class InvalidStateChange < StandardError; end
+
+    PENDING = 'pending'
+    ERRORED = 'errored'
+    REIMBURSED = 'reimbursed'
+    DEFAULT_REIMBURSEMENT_STATUSES = [PENDING, ERRORED, REIMBURSED]
 
     belongs_to :order, inverse_of: :reimbursements
     belongs_to :customer_return, inverse_of: :reimbursements, touch: true
@@ -14,13 +20,14 @@ class IncompleteReimbursementError < StandardError; end
 
     validates :order, presence: true
     validate :validate_return_items_belong_to_same_order
+    validate :is_valid_reimbursement_status?
 
     accepts_nested_attributes_for :return_items, allow_destroy: true
 
     before_create :generate_number
     before_create :calculate_total
 
-    scope :reimbursed, -> { where(reimbursement_status: 'reimbursed') }
+    scope :reimbursed, -> { where(reimbursement_status: REIMBURSED) }
 
     # The reimbursement_tax_calculator property should be set to an object that responds to "call"
     # and accepts a reimbursement object. Invoking "call" should update the tax fields on the
@@ -59,14 +66,44 @@ class IncompleteReimbursementError < StandardError; end
     class_attribute :reimbursement_failure_hooks
     self.reimbursement_failure_hooks = []
 
-    state_machine :reimbursement_status, initial: :pending do
-      event :errored do
-        transition to: :errored, from: [:pending, :errored]
-      end
+    def errored!
+      errored || raise(InvalidStateChange)
+    end
 
-      event :reimbursed do
-        transition to: :reimbursed, from: [:pending, :errored]
-      end
+    def errored
+      return false unless can_errored?
+      change_status!(ERRORED)
+      true
+    end
+
+    def errored?
+      reimbursement_status == ERRORED
+    end
+
+    def can_errored?
+      pending? || errored?
+    end
+
+    def reimbursed!
+      reimbursed || raise(InvalidStateChange)
+    end
+
+    def reimbursed
+      return false unless can_reimbursed?
+      change_status!(REIMBURSED)
+      true
+    end
+
+    def reimbursed?
+      reimbursement_status == REIMBURSED
+    end
+
+    def can_reimbursed?
+      pending? || errored?
+    end
+
+    def pending?
+      reimbursement_status == PENDING
     end
 
     class << self
@@ -176,6 +213,12 @@ def validate_return_items_belong_to_same_order
       end
     end
 
+    def is_valid_reimbursement_status?
+      unless DEFAULT_REIMBURSEMENT_STATUSES.include?(reimbursement_status)
+        errors.add(:reimbursement_status, "Invalid reimbursement_status")
+      end
+    end
+
     def send_reimbursement_email
       Spree::Config.reimbursement_mailer_class.reimbursement_email(id).deliver_later
     end
@@ -197,5 +240,10 @@ def unpaid_amount_within_tolerance?
                  end
       unpaid_amount.abs.between?(0, leniency)
     end
+
+    def change_status!(new_status)
+      return if new_status == reimbursement_status
+      update!(reimbursement_status: new_status)
+    end
   end
 end

core/db/migrate/20180404235352_add_default_status_to_reimbursements.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddDefaultStatusToReimbursements < ActiveRecord::Migration[5.1]
+  def change
+    change_column_default(:spree_reimbursements, :reimbursement_status, 'pending')
+  end
+end

core/spec/models/spree/reimbursement_spec.rb

@@ -3,6 +3,9 @@
 require 'rails_helper'
 
 RSpec.describe Spree::Reimbursement, type: :model do
+  let(:order) { Spree::Order.create }
+  let(:reimbursement) { Spree::Reimbursement.create(order: order) }
+
   describe ".create" do
     let(:customer_return) { create(:customer_return) }
     let(:order) { customer_return.order }
@@ -274,4 +277,131 @@
       expect { subject }.to change { reimbursement.refunds.count }.by(1)
     end
   end
+
+  describe '#errored!' do
+    subject { reimbursement.errored! }
+
+    it { is_expected.to be true }
+
+    context 'when not able to change status to errored' do
+      before { reimbursement.reimbursement_status = 'reimbursed' }
+      it 'raises an error' do
+        expect { subject }.to raise_error(Spree::Reimbursement::InvalidStateChange)
+      end
+    end
+  end
+
+  describe '#errored' do
+    subject { reimbursement.errored }
+
+    it { is_expected.to be true }
+
+    it 'changes the status to errored' do
+      subject
+      expect(reimbursement.reimbursement_status).to eq 'errored'
+    end
+
+    context 'when not able to change status to errored' do
+      before { reimbursement.reimbursement_status = 'reimbursed' }
+      it { is_expected.to be false }
+    end
+  end
+
+  describe '#errored?' do
+    subject { reimbursement.errored? }
+
+    before { reimbursement.reimbursement_status = 'errored' }
+
+    it { is_expected.to be true }
+
+    context 'when status is not errored' do
+      before { reimbursement.reimbursement_status = 'pending' }
+      it { is_expected.to be false }
+    end
+  end
+
+  describe '#can_errored?' do
+    subject { reimbursement.can_errored? }
+
+    ['pending', 'errored'].each do |status|
+      context "when status is #{status}" do
+        before { reimbursement.reimbursement_status = status }
+        it { is_expected.to be true }
+      end
+    end
+
+    context 'when status is reimbursed' do
+      before { reimbursement.reimbursement_status = 'reimbursed' }
+      it { is_expected.to be false }
+    end
+  end
+
+  describe '#reimbursed!' do
+    subject { reimbursement.reimbursed! }
+
+    it { is_expected.to be true }
+
+    context 'when not able to change status to reimbursed' do
+      before { reimbursement.reimbursement_status = 'reimbursed' }
+      it 'raises an error' do
+        expect { subject }.to raise_error(Spree::Reimbursement::InvalidStateChange)
+      end
+    end
+  end
+
+  describe '#reimbursed' do
+    subject { reimbursement.reimbursed }
+
+    it { is_expected.to be true }
+
+    it 'changes the status to reimbursed' do
+      subject
+      expect(reimbursement.reimbursement_status).to eq 'reimbursed'
+    end
+
+    context 'when not able to change status to reimbursed' do
+      before { reimbursement.reimbursement_status = 'reimbursed' }
+      it { is_expected.to be false }
+    end
+  end
+
+  describe '#reimbursed?' do
+    subject { reimbursement.reimbursed? }
+
+    before { reimbursement.reimbursement_status = 'reimbursed' }
+
+    it { is_expected.to be true }
+
+    context 'when status is not reimbursed' do
+      before { reimbursement.reimbursement_status = 'pending' }
+      it { is_expected.to be false }
+    end
+  end
+
+  describe '#can_reimbursed?' do
+    subject { reimbursement.can_reimbursed? }
+
+    ['pending', 'errored'].each do |status|
+      context "when status is #{status}" do
+        before { reimbursement.reimbursement_status = status }
+        it { is_expected.to be true }
+      end
+    end
+
+    context 'when status is reimbursed' do
+      before { reimbursement.reimbursement_status = 'reimbursed' }
+      it { is_expected.to be false }
+    end
+  end
+
+  describe '#pending?' do
+    subject { reimbursement.pending? }
+
+    it { is_expected.to be true }
+
+    context 'when status is not pending' do
+      before { reimbursement.reimbursement_status = 'errored' }
+      it { is_expected.to be false }
+    end
+  end
 end