diff --git a/backend/app/controllers/spree/admin/customer_returns_controller.rb b/backend/app/controllers/spree/admin/customer_returns_controller.rb index 3fd65fa4ad2..a1201399f68 100644 --- a/backend/app/controllers/spree/admin/customer_returns_controller.rb +++ b/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 diff --git a/backend/spec/controllers/spree/admin/return_items_controller_spec.rb b/backend/spec/controllers/spree/admin/return_items_controller_spec.rb index 7257a5e9ae2..0ad7bdcabd6 100644 --- a/backend/spec/controllers/spree/admin/return_items_controller_spec.rb +++ b/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 diff --git a/backend/spec/features/admin/orders/customer_returns_spec.rb b/backend/spec/features/admin/orders/customer_returns_spec.rb index cb422518ab2..0e592bbd9fa 100644 --- a/backend/spec/features/admin/orders/customer_returns_spec.rb +++ b/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 diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 020e98ba18d..ba4537c14cd 100644 --- a/core/app/models/spree/order.rb +++ b/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 diff --git a/core/app/models/spree/return_item.rb b/core/app/models/spree/return_item.rb index d08de997d44..bc8212d0b60 100644 --- a/core/app/models/spree/return_item.rb +++ b/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 diff --git a/core/lib/spree/core/state_machines/return_item/reception_status.rb b/core/lib/spree/core/state_machines/return_item/reception_status.rb index faf9211c204..fa71fe83c6c 100644 --- a/core/lib/spree/core/state_machines/return_item/reception_status.rb +++ b/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 } diff --git a/core/lib/spree/testing_support/factories/return_item_factory.rb b/core/lib/spree/testing_support/factories/return_item_factory.rb index f2492ebba62..ce5abbaa811 100644 --- a/core/lib/spree/testing_support/factories/return_item_factory.rb +++ b/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| diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb index cc02cd7348b..84c412fc711 100644 --- a/core/spec/models/spree/order_spec.rb +++ b/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 diff --git a/core/spec/models/spree/return_item_spec.rb b/core/spec/models/spree/return_item_spec.rb index 08525126b39..841b7988983 100644 --- a/core/spec/models/spree/return_item_spec.rb +++ b/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 diff --git a/sample/db/samples/reimbursements.rb b/sample/db/samples/reimbursements.rb index 4b7990a68b6..fce000df514 100644 --- a/sample/db/samples/reimbursements.rb +++ b/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!