Fix order state with customer returns when receiving return items

backend/app/controllers/spree/admin/customer_returns_controller.rb

@@ -70,8 +70,6 @@ def build_return_items_from_params
           return_item = item_params[:id] ? Spree::ReturnItem.find(item_params[:id]) : Spree::ReturnItem.new
           return_item.assign_attributes(item_params)
 
-          return_item.skip_customer_return_processing = true
-
           if item_params[:reception_status_event].blank?
             return redirect_to(new_object_url, flash: { error: 'Reception status choice required' })
           end

backend/spec/controllers/spree/admin/return_items_controller_spec.rb

@@ -8,22 +8,53 @@
   describe '#update' do
     let(:customer_return) { create(:customer_return) }
     let(:return_item) { customer_return.return_items.first }
-    let(:old_acceptance_status) { 'pending' }
-    let(:new_acceptance_status) { 'rejected' }
 
-    subject do
-      put :update, params: { id: return_item.to_param, return_item: { acceptance_status: new_acceptance_status } }
-    end
+    describe ':acceptance_status' do
+      let(:old_acceptance_status) { 'pending' }
+      let(:new_acceptance_status) { 'rejected' }
+
+      subject do
+        put :update, params: { id: return_item.to_param, return_item: { acceptance_status: new_acceptance_status } }
+      end
+
+      it 'updates the return item' do
+        expect {
+          subject
+        }.to change { return_item.reload.acceptance_status }.from(old_acceptance_status).to(new_acceptance_status)
+      end
 
-    it 'updates the return item' do
-      expect {
+      it 'redirects to the customer return' do
         subject
-      }.to change { return_item.reload.acceptance_status }.from(old_acceptance_status).to(new_acceptance_status)
+        expect(response).to redirect_to spree.edit_admin_order_customer_return_path(customer_return.order, customer_return)
+      end
     end
 
-    it 'redirects to the customer return' do
-      subject
-      expect(response).to redirect_to spree.edit_admin_order_customer_return_path(customer_return.order, customer_return)
+    describe ':reception_status' do
+      let(:old_reception_status) { 'in_transit' }
+      let(:new_reception_status) { 'received' }
+      let(:reception_status_event) { 'receive' }
+
+      before do
+        allow(Spree::Deprecation).to receive(:warn).with(a_string_matching('#process_inventory_unit! will not call'))
+
+        return_item.update! reception_status: 'in_transit'
+      end
+
+      subject do
+        put :update, params: { id: return_item.to_param, return_item: { reception_status_event: reception_status_event } }
+      end
+
+      it 'updates the return item' do
+        expect {
+          subject
+        }.to change { return_item.reload.reception_status }.from(old_reception_status).to(new_reception_status)
+        expect(customer_return.order.state).to eq('returned')
+      end
+
+      it 'redirects to the customer return' do
+        subject
+        expect(response).to redirect_to spree.edit_admin_order_customer_return_path(customer_return.order, customer_return)
+      end
     end
   end
 end

backend/spec/features/admin/orders/customer_returns_spec.rb

@@ -5,37 +5,82 @@
 describe 'Customer returns', type: :feature do
   stub_authorization!
 
-  context 'when the order has more than one line item' do
-    let(:order) { create :shipped_order, line_items_count: 2 }
+  def create_customer_return(value)
+    find('#select-all').click
+    page.execute_script "$('select.add-item').val(#{value.to_s.inspect})"
+    select 'NY Warehouse', from: 'Stock Location'
+    click_button 'Create'
+  end
 
-    def create_customer_return
-      find('#select-all').click
-      page.execute_script "$('select.add-item').val('receive')"
-      select 'NY Warehouse', from: 'Stock Location'
-      click_button 'Create'
-    end
+  def order_state_label
+    find('dd.order-state').text
+  end
 
-    before do
-      visit spree.new_admin_order_customer_return_path(order)
+  before do
+    allow(Spree::Deprecation).to receive(:warn) do |message|
+      expect(message).to match('#process_inventory_unit! will not call')
     end
 
+    visit spree.new_admin_order_customer_return_path(order)
+  end
+
+  context 'when the order has more than one line item' do
+    let(:order) { create :shipped_order, line_items_count: 2 }
+
     context 'when creating a return with state "Received"' do
       it 'marks the order as returned', :js do
-        create_customer_return
+        create_customer_return('receive')
 
         expect(page).to have_content 'Customer Return has been successfully created'
-        within 'dd.order-state' do
-          expect(page).to have_content 'Returned'
-        end
+
+        expect(order_state_label).to eq('Returned')
       end
     end
 
     it 'disables the button at submit', :js do
       page.execute_script "$('form').submit(function(e) { e.preventDefault()})"
 
-      create_customer_return
+      create_customer_return('receive')
 
       expect(page).to have_button("Create", disabled: true)
     end
+
+    context 'when creating a return with state "In Transit" and then marking it as "Received"' do
+      it 'marks the order as returned', :js do
+        create_customer_return('in_transit')
+        expect(page).to have_content 'Customer Return has been successfully created'
+        expect(order_state_label).to eq('Complete')
+
+        within('[data-hook="rejected_return_items"] tbody tr:nth-child(1)') { click_button('Receive') }
+        expect(order_state_label).to eq('Complete')
+
+        within('[data-hook="rejected_return_items"] tbody tr:nth-child(2)') { click_button('Receive') }
+        expect(order_state_label).to eq('Returned')
+      end
+    end
+  end
+
+  context 'when the order has only one line item' do
+    let(:order) { create :shipped_order, line_items_count: 1 }
+
+    context 'when creating a return with state "Received"' do
+      it 'marks the order as returned', :js do
+        create_customer_return('receive')
+
+        expect(page).to have_content 'Customer Return has been successfully created'
+        expect(order_state_label).to eq('Returned')
+      end
+    end
+
+    context 'when creating a return with state "In Transit" and then marking it as "Received"' do
+      it 'marks the order as returned', :js do
+        create_customer_return('in_transit')
+        expect(page).to have_content 'Customer Return has been successfully created'
+        expect(order_state_label).to eq('Complete')
+
+        within('[data-hook="rejected_return_items"] tbody tr:nth-child(1)') { click_button('Receive') }
+        expect(order_state_label).to eq('Returned')
+      end
+    end
   end
 end

core/app/models/spree/order.rb

@@ -288,7 +288,11 @@ def allow_cancel?
     end
 
     def all_inventory_units_returned?
-      inventory_units.all?(&:returned?)
+      # Inventory units are transitioned to the "return" state through CustomerReturn and
+      # ReturnItem instead of using Order#inventory_units, thus making the latter method
+      # potentially return stale data. This situation requires to *reload* `inventory_units`
+      # in order to pick-up the latest changes and make the check on `returned?` reliable.
+      inventory_units.reload.all?(&:returned?)
     end
 
     def contents

core/app/models/spree/return_item.rb

@@ -90,8 +90,13 @@ def reception_completed?
       COMPLETED_RECEPTION_STATUSES.map(&:to_s).include?(reception_status.to_s)
     end
 
-
-    attr_accessor :skip_customer_return_processing
+    def skip_customer_return_processing=(value)
+      @skip_customer_return_processing = value
+      Deprecation.warn \
+        'From Solidus v2.11 onwards, #skip_customer_return_processing does ' \
+        'nothing, and #process_inventory_unit! will restore calling ' \
+        'customer_return#process_return!', caller(1)
+    end
 
     # @param inventory_unit [Spree::InventoryUnit] the inventory for which we
     #   want a return item
@@ -196,13 +201,19 @@ def currency
 
     def process_inventory_unit!
       inventory_unit.return!
-      if customer_return
-        customer_return.stock_location.restock(inventory_unit.variant, 1, customer_return) if should_restock?
-        unless skip_customer_return_processing
-          Deprecation.warn 'From Solidus v2.9 onwards, #process_inventory_unit! will not call customer_return#process_return!'
-          customer_return.process_return!
-        end
+
+      if should_restock?
+        customer_return.stock_location.restock(inventory_unit.variant, 1, customer_return)
       end
+
+      unless @skip_customer_return_processing.nil?
+        Deprecation.warn \
+          'From Solidus v2.11 onwards, #skip_customer_return_processing does ' \
+          'nothing, and #process_inventory_unit! will restore calling ' \
+          'customer_return#process_return!'
+      end
+
+      customer_return&.process_return!
     end
 
     def sibling_intended_for_exchange(status)
@@ -276,9 +287,9 @@ def cancel_others
     end
 
     def should_restock?
-      resellable? &&
+      customer_return &&
+        resellable? &&
         variant.should_track_inventory? &&
-        customer_return &&
         customer_return.stock_location.restock_inventory?
     end
   end

core/lib/spree/core/state_machines/return_item/reception_status.rb

@@ -19,8 +19,8 @@ module ReceptionStatus
 
           included do
             state_machine :reception_status, initial: :awaiting do
-              after_transition to: ::Spree::ReturnItem::COMPLETED_RECEPTION_STATUSES,  do: :attempt_accept
-              after_transition to: ::Spree::ReturnItem::COMPLETED_RECEPTION_STATUSES,  do: :check_unexchange
+              after_transition to: ::Spree::ReturnItem::COMPLETED_RECEPTION_STATUSES, do: :attempt_accept, if: :can_attempt_accept?
+              after_transition to: ::Spree::ReturnItem::COMPLETED_RECEPTION_STATUSES, do: :check_unexchange
               after_transition to: :received, do: :process_inventory_unit!
 
               event(:cancel) { transition to: :cancelled, from: :awaiting }

core/lib/spree/testing_support/factories/return_item_factory.rb

@@ -6,7 +6,6 @@
 
 FactoryBot.define do
   factory :return_item, class: 'Spree::ReturnItem' do
-    skip_customer_return_processing { true }
     association(:inventory_unit, factory: :inventory_unit, state: :shipped)
     association(:return_reason, factory: :return_reason)
     return_authorization do |_return_item|

core/spec/models/spree/order_spec.rb

@@ -1187,6 +1187,18 @@ def generate
         expect(subject).to eq false
       end
     end
+
+    context "all inventory units are returned on the database (e.g. through another association)" do
+      it "is true" do
+        expect {
+          Spree::InventoryUnit
+            .where(id: order.inventory_unit_ids)
+            .update_all(state: 'returned')
+        }.to change {
+          order.all_inventory_units_returned?
+        }.from(false).to(true)
+      end
+    end
   end
 
   context "store credit" do

core/spec/models/spree/return_item_spec.rb

@@ -2,18 +2,18 @@
 
 require 'rails_helper'
 
-RSpec.shared_examples "an invalid state transition" do |status, expected_status|
-  let(:status) { status }
-
-  it "cannot transition to #{expected_status}" do
-    expect { subject }.to raise_error(StateMachines::InvalidTransition)
-  end
-end
-
 RSpec.describe Spree::ReturnItem, type: :model do
   all_reception_statuses = Spree::ReturnItem.state_machines[:reception_status].states.map(&:name).map(&:to_s)
   all_acceptance_statuses = Spree::ReturnItem.state_machines[:acceptance_status].states.map(&:name).map(&:to_s)
 
+  shared_examples "an invalid state transition" do |status, expected_status|
+    let(:status) { status }
+
+    it "cannot transition to #{expected_status}" do
+      expect { subject }.to raise_error(StateMachines::InvalidTransition)
+    end
+  end
+
   before do
     allow_any_instance_of(Spree::Order).to receive(:return!).and_return(true)
   end
@@ -43,8 +43,8 @@
       subject
     end
 
-    context 'when the `skip_customer_return_processing` flag is not set to true' do
-      before { return_item.skip_customer_return_processing = false }
+    context 'when the `skip_customer_return_processing` flag is set' do
+      before { Spree::Deprecation.silence { return_item.skip_customer_return_processing = false } }
 
       it 'shows a deprecation warning' do
         expect(Spree::Deprecation).to receive(:warn)
@@ -222,6 +222,35 @@
     it "starts off in the awaiting state" do
       expect(return_item).to be_awaiting
     end
+
+    context 'when transitioning to :received' do
+      let(:return_item) { create(:return_item) }
+      subject { return_item.receive! }
+
+      # StateMachines has some "smart" code for guessing how many arguments to
+      # send to the callback methods that interferes with rspec-mocks.
+      around { |e| without_partial_double_verification { e.call } }
+
+      it 'calls #attempt_accept, #check_unexchange, and #process_inventory_unit!' do
+        expect(return_item).to receive(:attempt_accept)
+        expect(return_item).to receive(:check_unexchange)
+        expect(return_item).to receive(:process_inventory_unit!)
+
+        subject
+      end
+
+      context 'when the :acceptance_status is in a state that does not support the :attempt_accept event' do
+        before { return_item.require_manual_intervention! }
+
+        it 'only skips the call to :attempt_accept' do
+          expect(return_item).not_to receive(:attempt_accept)
+          expect(return_item).to receive(:check_unexchange)
+          expect(return_item).to receive(:process_inventory_unit!)
+
+          subject
+        end
+      end
+    end
   end
 
   describe "acceptance_status state_machine" do
@@ -782,4 +811,16 @@
       end
     end
   end
+
+  describe '#skip_customer_return_processing=' do
+    context 'when it receives a value' do
+      let(:return_item) { described_class.new }
+      subject { return_item.skip_customer_return_processing = :foo }
+
+      it 'shows a deprecation warning' do
+        expect(Spree::Deprecation).to receive(:warn)
+        subject
+      end
+    end
+  end
 end

sample/db/samples/reimbursements.rb

@@ -38,7 +38,6 @@
   stock_location: stock_location
 )
 return_item.reload
-return_item.skip_customer_return_processing = true
 return_item.receive!
 customer_return.process_return!