diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 29bb53f0b73..9a62af0973f 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -502,7 +502,8 @@ def create_proposed_shipments end def create_shipments_for_line_item(line_item) - units = Spree::Stock::InventoryUnitBuilder.new(self).missing_units_for_line_item(line_item) + units = Spree::Config.stock.inventory_unit_builder_class.new(self).missing_units_for_line_item(line_item) + Spree::Config.stock.coordinator_class.new(self, units).shipments.each do |shipment| shipments << shipment end @@ -789,9 +790,12 @@ def require_email def ensure_inventory_units if has_checkout_step?("delivery") - inventory_validator = Spree::Stock::InventoryValidator.new + inventory_validator = Spree::Config.stock.inventory_validator_class.new + + errors = line_items.map { |line_item| + inventory_validator.validate(line_item) + }.compact - errors = line_items.map { |line_item| inventory_validator.validate(line_item) }.compact raise InsufficientStock if errors.any? end end @@ -809,8 +813,15 @@ def ensure_promotions_eligible end def validate_line_item_availability - availability_validator = Spree::Stock::AvailabilityValidator.new - raise InsufficientStock unless line_items.all? { |line_item| availability_validator.validate(line_item) } + availability_validator = Spree::Config.stock.availability_validator_class.new + + # NOTE: This code assumes that the availability validator will return + # true for success and false for failure. This is not normally the + # behaviour of validators, as the framework only cares about the + # population of the errors, not the return value of the validate method. + raise InsufficientStock unless line_items.all? { |line_item| + availability_validator.validate(line_item) + } end def ensure_line_items_present diff --git a/core/app/models/spree/stock/simple_coordinator.rb b/core/app/models/spree/stock/simple_coordinator.rb index de523ef804d..60913cf372d 100644 --- a/core/app/models/spree/stock/simple_coordinator.rb +++ b/core/app/models/spree/stock/simple_coordinator.rb @@ -24,7 +24,8 @@ class SimpleCoordinator def initialize(order, inventory_units = nil) @order = order - @inventory_units = inventory_units || InventoryUnitBuilder.new(order).units + @inventory_units = + inventory_units || Spree::Config.stock.inventory_unit_builder_class.new(order).units @splitters = Spree::Config.environment.stock_splitters filtered_stock_locations = Spree::Config.stock.location_filter_class.new(Spree::StockLocation.all, @order).filter diff --git a/core/lib/spree/core/stock_configuration.rb b/core/lib/spree/core/stock_configuration.rb index b4da441742a..e3a3c9d07c2 100644 --- a/core/lib/spree/core/stock_configuration.rb +++ b/core/lib/spree/core/stock_configuration.rb @@ -8,6 +8,9 @@ class StockConfiguration attr_writer :location_filter_class attr_writer :location_sorter_class attr_writer :allocator_class + attr_writer :inventory_unit_builder_class + attr_writer :availability_validator_class + attr_writer :inventory_validator_class def coordinator_class @coordinator_class ||= '::Spree::Stock::SimpleCoordinator' @@ -33,6 +36,21 @@ def allocator_class @allocator_class ||= '::Spree::Stock::Allocator::OnHandFirst' @allocator_class.constantize end + + def inventory_unit_builder_class + @inventory_unit_builder_class ||= '::Spree::Stock::InventoryUnitBuilder' + @inventory_unit_builder_class.constantize + end + + def availability_validator_class + @availability_validator_class ||= '::Spree::Stock::AvailabilityValidator' + @availability_validator_class.constantize + end + + def inventory_validator_class + @inventory_validator_class ||= '::Spree::Stock::InventoryValidator' + @inventory_validator_class.constantize + end end end end diff --git a/core/spec/lib/spree/core/stock_configuration_spec.rb b/core/spec/lib/spree/core/stock_configuration_spec.rb index 0c03644880e..8b082840c1b 100644 --- a/core/spec/lib/spree/core/stock_configuration_spec.rb +++ b/core/spec/lib/spree/core/stock_configuration_spec.rb @@ -3,93 +3,153 @@ require 'rails_helper' RSpec.describe Spree::Core::StockConfiguration do + let(:stock_configuration) { described_class.new } + describe '#coordinator_class' do - let(:stock_configuration) { described_class.new } subject { stock_configuration.coordinator_class } - it "returns Spree::Stock::Coordinator" do - is_expected.to be ::Spree::Stock::SimpleCoordinator + it "returns Spree::Stock::Coordinator by default" do + expect(subject).to be ::Spree::Stock::SimpleCoordinator end - context "with another constant name assiged" do + it "can be reassigned" do MyCoordinator = Class.new - before { stock_configuration.coordinator_class = MyCoordinator.to_s } + stock_configuration.coordinator_class = MyCoordinator.to_s + + expect(subject).to be MyCoordinator - it "returns the constant" do - is_expected.to be MyCoordinator - end + ensure + Object.send(:remove_const, :MyCoordinator) end end describe '#estimator_class' do - let(:stock_configuration) { described_class.new } subject { stock_configuration.estimator_class } it "returns Spree::Stock::Estimator" do - is_expected.to be ::Spree::Stock::Estimator + expect(subject).to be ::Spree::Stock::Estimator end - context "with another constant name assiged" do + it "can be reassigned" do MyEstimator = Class.new - before { stock_configuration.estimator_class = MyEstimator.to_s } + stock_configuration.estimator_class = MyEstimator.to_s + + expect(subject).to be MyEstimator - it "returns the constant" do - is_expected.to be MyEstimator - end + ensure + Object.send(:remove_const, :MyEstimator) end end describe '#location_filter_class' do - let(:stock_configuration) { described_class.new } subject { stock_configuration.location_filter_class } - it "returns Spree::Stock::LocationFilter::Active" do - is_expected.to be ::Spree::Stock::LocationFilter::Active + it "returns Spree::Stock::LocationFilter::Active by default" do + expect(subject).to be ::Spree::Stock::LocationFilter::Active end - context "with another constant name assiged" do + it "can be reassigned" do MyFilter = Class.new - before { stock_configuration.location_filter_class = MyFilter.to_s } + stock_configuration.location_filter_class = MyFilter.to_s + + expect(subject).to be MyFilter - it "returns the constant" do - is_expected.to be MyFilter - end + ensure + Object.send(:remove_const, :MyFilter) end end describe '#location_sorter_class' do - let(:stock_configuration) { described_class.new } subject { stock_configuration.location_sorter_class } - it "returns Spree::Stock::LocationSorter::Unsorted" do - is_expected.to be ::Spree::Stock::LocationSorter::Unsorted + it "returns Spree::Stock::LocationSorter::Unsorted by default" do + expect(subject).to be ::Spree::Stock::LocationSorter::Unsorted end - context "with another constant name assiged" do + it "can be reassigned" do MySorter = Class.new - before { stock_configuration.location_sorter_class = MySorter.to_s } + stock_configuration.location_sorter_class = MySorter.to_s - it "returns the constant" do - is_expected.to be MySorter - end + expect(subject).to be MySorter + + ensure + Object.send(:remove_const, :MySorter) end end describe '#allocator_class' do - let(:stock_configuration) { described_class.new } subject { stock_configuration.allocator_class } - it "returns Spree::Stock::Allocator::OnHandFirst" do - is_expected.to be ::Spree::Stock::Allocator::OnHandFirst + it "returns Spree::Stock::Allocator::OnHandFirst by default" do + expect(subject).to be ::Spree::Stock::Allocator::OnHandFirst end - context "with another constant name assiged" do + it "can be reassigned" do MyAllocator = Class.new - before { stock_configuration.allocator_class = MyAllocator.to_s } + stock_configuration.allocator_class = MyAllocator.to_s + + expect(subject).to be MyAllocator + + ensure + Object.send(:remove_const, :MyAllocator) + end + end + + describe '#inventory_unit_builder_class' do + subject { stock_configuration.inventory_unit_builder_class } + + it "returns Spree::Stock::InventoryUnitBuilder by default" do + expect(subject).to be ::Spree::Stock::InventoryUnitBuilder + end + + it "can be reassigned" do + MyInventoryUnitBuilder = Class.new + stock_configuration.inventory_unit_builder_class = MyInventoryUnitBuilder.to_s + + expect(subject).to be MyInventoryUnitBuilder + + ensure + Object.send(:remove_const, :MyInventoryUnitBuilder) + end + end + + describe '#availability_validator_class' do + subject { stock_configuration.availability_validator_class } + + let(:stock_configuration) { described_class.new } + + it "returns Spree::Stock::AvailabilityValidator" do + is_expected.to be ::Spree::Stock::AvailabilityValidator + end + + it "can be reassigned" do + MyAvailabilityValidator = Class.new + stock_configuration.availability_validator_class = MyAvailabilityValidator.to_s + + expect(subject).to be MyAvailabilityValidator + + ensure + Object.send(:remove_const, :MyAvailabilityValidator) + end + end + + describe '#inventory_validator_class' do + subject { stock_configuration.inventory_validator_class } + + let(:stock_configuration) { described_class.new } + + it "returns Spree::Stock::InventoryValidator" do + is_expected.to be ::Spree::Stock::InventoryValidator + end + + it "can be reassigned" do + MyInventoryValidator = Class.new + stock_configuration.inventory_validator_class = MyInventoryValidator.to_s + + expect(subject).to be MyInventoryValidator - it "returns the constant" do - is_expected.to be MyAllocator - end + ensure + Object.send(:remove_const, :MyInventoryValidator) end end end diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb index 6a068a2eceb..ac00e2f4d03 100644 --- a/core/spec/models/spree/order_spec.rb +++ b/core/spec/models/spree/order_spec.rb @@ -1643,11 +1643,47 @@ def generate describe '#create_shipments_for_line_item' do subject { order.create_shipments_for_line_item(line_item) } - let(:order) { create :order_with_line_items } + let(:order) { create :order, shipments: [] } let(:line_item) { build(:line_item, order: order) } it 'creates at least one new shipment for the order' do - expect { subject }.to change { order.shipments.count }.by 1 + expect { subject }.to change { order.shipments.count }.from(0).to(1) + end + + context "with a custom inventory unit builder" do + before do + # Because the defined method runs in the context of the instance of our + # test inventory unit builder, not the RSpec example context, we need + # to make this value available as a local variable. We're using + # Class.new and define_method to avoid creating scope gates that would + # take this local variable out of scope. + inventory_unit = arbitrary_inventory_unit + TestInventoryUnitBuilder = Class.new do + def initialize(order) + end + + define_method(:missing_units_for_line_item) { |line_item| + [inventory_unit] + } + end + + test_stock_config = Spree::Core::StockConfiguration.new + test_stock_config.inventory_unit_builder_class = TestInventoryUnitBuilder.to_s + stub_spree_preferences(stock: test_stock_config) + end + + after do + Object.send(:remove_const, :TestInventoryUnitBuilder) + end + + let(:arbitrary_inventory_unit) { build :inventory_unit, order: order, line_item: line_item, variant: line_item.variant } + + it "relies on the custom builder" do + expect { subject }.to change { order.shipments.count }.from(0).to(1) + + expect(order.shipments.order(:created_at).first.inventory_units) + .to contain_exactly arbitrary_inventory_unit + end end end @@ -1665,4 +1701,85 @@ def generate expect(subject).to eq 40 end end + + describe "#ensure_inventory_units" do + subject { order.send(:ensure_inventory_units) } + + before do + class TestValidator + def validate(line_item) + if line_item.quantity != 1 + line_item.errors.add(:quantity, ":(") + end + end + end + + test_stock_config = Spree::Core::StockConfiguration.new + test_stock_config.inventory_validator_class = TestValidator.to_s + stub_spree_preferences(stock: test_stock_config) + end + + let(:order) { create :order_with_line_items, line_items_count: 2 } + + it "uses the configured validator" do + expect_any_instance_of(TestValidator).to receive(:validate).twice.and_call_original + + subject + end + + context "when the line items are valid" do + it "doesn't raise an exception" do + expect { subject }.not_to raise_error + end + end + + context "when the line items are not valid" do + before do + order.line_items.last.quantity = 2 + end + + it "raises an exception" do + expect { subject }.to raise_error(Spree::Order::InsufficientStock) + end + end + end + + describe "#validate_line_item_availability" do + subject { order.send(:validate_line_item_availability) } + + before do + class TestValidator + def validate(line_item) + if line_item.variant.sku == "UNAVAILABLE" + line_item.errors.add(:quantity, ":(") + false + else + true + end + end + end + + test_stock_config = Spree::Core::StockConfiguration.new + test_stock_config.availability_validator_class = TestValidator.to_s + stub_spree_preferences(stock: test_stock_config) + end + + let(:order) { create :order_with_line_items, line_items_count: 2 } + + context "when the line items are valid" do + it "doesn't raise an exception" do + expect { subject }.not_to raise_error + end + end + + context "when the line items are not valid" do + before do + order.line_items.last.variant.sku = "UNAVAILABLE" + end + + it "raises an exception" do + expect { subject }.to raise_error(Spree::Order::InsufficientStock) + end + end + end end diff --git a/core/spec/models/spree/stock/simple_coordinator_spec.rb b/core/spec/models/spree/stock/simple_coordinator_spec.rb index 34554740302..02a245b1288 100644 --- a/core/spec/models/spree/stock/simple_coordinator_spec.rb +++ b/core/spec/models/spree/stock/simple_coordinator_spec.rb @@ -38,6 +38,14 @@ module Stock subject.shipments.count == StockLocation.count end + it 'uses the pluggable inventory unit builder class' do + expect(Spree::Config.stock) + .to receive(:inventory_unit_builder_class) + .and_call_original + + subject.shipments + end + context "missing stock items in active stock location" do let!(:another_location) { create(:stock_location, propagate_all_variants: false) } diff --git a/frontend/app/controllers/spree/checkout_controller.rb b/frontend/app/controllers/spree/checkout_controller.rb index afd71eecdf7..7dc1d71629f 100644 --- a/frontend/app/controllers/spree/checkout_controller.rb +++ b/frontend/app/controllers/spree/checkout_controller.rb @@ -230,7 +230,7 @@ def insufficient_stock_error flash[:error] = I18n.t('spree.insufficient_stock_for_order') redirect_to cart_path else - availability_validator = Spree::Stock::AvailabilityValidator.new + availability_validator = Spree::Config.stock.availability_validator_class.new unavailable_items = @order.line_items.reject { |line_item| availability_validator.validate(line_item) } if unavailable_items.any? item_names = unavailable_items.map(&:name).to_sentence