diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 753aaee48e..a0c6a0b30c 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -363,6 +363,13 @@ Rails/UniqueValidationWithoutIndex: - 'app/models/customer.rb' - 'app/models/exchange.rb' +# Offense count: 2 +# Configuration parameters: Include. +# Include: app/models/**/*.rb +Rails/UniqueValidationWithoutIndex: + Exclude: + - 'app/models/spree/stock_item.rb' + # Offense count: 1 # Configuration parameters: Environments. # Environments: development, test, production diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb new file mode 100644 index 0000000000..e6df19b03a --- /dev/null +++ b/app/models/spree/stock_item.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +module Spree + class StockItem < ActiveRecord::Base + acts_as_paranoid + + belongs_to :stock_location, class_name: 'Spree::StockLocation' + belongs_to :variant, class_name: 'Spree::Variant' + has_many :stock_movements, dependent: :destroy + + validates :stock_location, :variant, presence: true + validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] } + validates :count_on_hand, numericality: { greater_than_or_equal_to: 0, unless: :backorderable? } + + delegate :weight, to: :variant + delegate :name, to: :variant, prefix: true + + def backordered_inventory_units + Spree::InventoryUnit.backordered_for_stock_item(self) + end + + def adjust_count_on_hand(value) + with_lock do + self.count_on_hand = count_on_hand + value + process_backorders if in_stock? + + save! + end + end + + def in_stock? + count_on_hand.positive? + end + + # Tells whether it's available to be included in a shipment + def available? + in_stock? || backorderable? + end + + def variant + Spree::Variant.unscoped { super } + end + + private + + def count_on_hand=(value) + self[:count_on_hand] = value + end + + def process_backorders + backordered_inventory_units.each do |unit| + break unless in_stock? + + unit.fill_backorder + end + end + end +end diff --git a/db/migrate/20200512070717_add_lock_version_to_stock_items.rb b/db/migrate/20200512070717_add_lock_version_to_stock_items.rb new file mode 100644 index 0000000000..416a43f62e --- /dev/null +++ b/db/migrate/20200512070717_add_lock_version_to_stock_items.rb @@ -0,0 +1,5 @@ +class AddLockVersionToStockItems < ActiveRecord::Migration + def change + add_column :spree_stock_items, :lock_version, :integer, default: 0 + end +end diff --git a/db/migrate/20200514174526_reset_negative_nonbackorderable_count_on_hand_in_stock_items.rb b/db/migrate/20200514174526_reset_negative_nonbackorderable_count_on_hand_in_stock_items.rb new file mode 100644 index 0000000000..c57730c99a --- /dev/null +++ b/db/migrate/20200514174526_reset_negative_nonbackorderable_count_on_hand_in_stock_items.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class ResetNegativeNonbackorderableCountOnHandInStockItems < ActiveRecord::Migration + module Spree + class StockItem < ActiveRecord::Base + self.table_name = "spree_stock_items" + end + end + + def up + Spree::StockItem.where(backorderable: false) + .where("count_on_hand < 0") + .update_all(count_on_hand: 0) + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/schema.rb b/db/schema.rb index 97ddcb63d2..8df4304ca2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -901,6 +901,7 @@ ActiveRecord::Schema.define(version: 20200702112157) do t.datetime "updated_at", null: false t.boolean "backorderable", default: false t.datetime "deleted_at" + t.integer "lock_version", default: 0 end add_index "spree_stock_items", ["stock_location_id", "variant_id"], name: "stock_item_by_loc_and_var_id", using: :btree diff --git a/spec/controllers/checkout_controller_concurrency_spec.rb b/spec/controllers/checkout_controller_concurrency_spec.rb index 51eefe98ae..266f9a4fbe 100644 --- a/spec/controllers/checkout_controller_concurrency_spec.rb +++ b/spec/controllers/checkout_controller_concurrency_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' # This is the first example of testing concurrency in the Open Food Network. @@ -15,6 +17,20 @@ describe CheckoutController, concurrency: true, type: :controller do let(:payment_method) { create(:payment_method, distributors: [distributor]) } let(:breakpoint) { Mutex.new } + let(:address_params) { address.attributes.except("id") } + let(:order_params) { + { + "payments_attributes" => [ + { + "payment_method_id" => payment_method.id, + "amount" => order.total + } + ], + "bill_address_attributes" => address_params, + "ship_address_attributes" => address_params, + } + } + before do # Create a valid order ready for checkout: create(:shipping_method, distributors: [distributor]) @@ -26,7 +42,9 @@ describe CheckoutController, concurrency: true, type: :controller do allow(controller).to receive(:spree_current_user).and_return(order.user) allow(controller).to receive(:current_distributor).and_return(order.distributor) allow(controller).to receive(:current_order_cycle).and_return(order.order_cycle) + end + it "handles two concurrent orders successfully" do # New threads start running straight away. The breakpoint is after loading # the order and before advancing the order's state and making payments. breakpoint.lock @@ -36,21 +54,6 @@ describe CheckoutController, concurrency: true, type: :controller do # I did not find out how to call the original code otherwise. ActiveSupport::Notifications.instrument("spree.checkout.update") end - end - - it "waits for concurrent checkouts" do - # Basic data the user submits during checkout: - address_params = address.attributes.except("id") - order_params = { - "payments_attributes" => [ - { - "payment_method_id" => payment_method.id, - "amount" => order.total - } - ], - "bill_address_attributes" => address_params, - "ship_address_attributes" => address_params, - } # Starting two checkout threads. The controller code will determine if # these two threads are synchronised correctly or run into a race condition. diff --git a/spec/models/spree/line_item_spec.rb b/spec/models/spree/line_item_spec.rb index bef32dc8d7..2836c2c51c 100644 --- a/spec/models/spree/line_item_spec.rb +++ b/spec/models/spree/line_item_spec.rb @@ -97,7 +97,7 @@ module Spree end it "caps at zero when stock is negative" do - v.update! on_hand: -2 + v.__send__(:stock_item).update_column(:count_on_hand, -2) li.cap_quantity_at_stock! expect(li.reload.quantity).to eq 0 end @@ -123,7 +123,7 @@ module Spree before { vo.update(count_on_hand: -3) } it "caps at zero" do - v.update(on_hand: -2) + v.__send__(:stock_item).update_column(:count_on_hand, -2) li.cap_quantity_at_stock! expect(li.reload.quantity).to eq 0 end diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb new file mode 100644 index 0000000000..9c9b233986 --- /dev/null +++ b/spec/models/spree/stock_item_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Spree::StockItem do + let(:stock_location) { create(:stock_location_with_items) } + + subject { stock_location.stock_items.order(:id).first } + + describe "validation" do + let(:stock_item) { stock_location.stock_items.first } + + it "requires count_on_hand to be positive if not backorderable" do + stock_item.backorderable = false + + stock_item.__send__(:count_on_hand=, 1) + expect(stock_item.valid?).to eq(true) + + stock_item.__send__(:count_on_hand=, 0) + expect(stock_item.valid?).to eq(true) + + stock_item.__send__(:count_on_hand=, -1) + expect(stock_item.valid?).to eq(false) + end + + it "allows count_on_hand to be negative if backorderable" do + stock_item.backorderable = true + + stock_item.__send__(:count_on_hand=, 1) + expect(stock_item.valid?).to eq(true) + + stock_item.__send__(:count_on_hand=, -1) + expect(stock_item.valid?).to eq(true) + end + end + + it 'maintains the count on hand for a variant' do + expect(subject.count_on_hand).to eq 15 + end + + it "can return the stock item's variant's name" do + expect(subject.variant_name).to eq(subject.variant.name) + end + + context "available to be included in shipment" do + context "has stock" do + it { expect(subject).to be_available } + end + + context "backorderable" do + before { subject.backorderable = true } + it { expect(subject).to be_available } + end + + context "no stock and not backorderable" do + before do + subject.backorderable = false + allow(subject).to receive_messages(count_on_hand: 0) + end + + it { expect(subject).not_to be_available } + end + end + + context "adjust count_on_hand" do + let!(:current_on_hand) { subject.count_on_hand } + + it 'is updated pessimistically' do + copy = Spree::StockItem.find(subject.id) + + subject.adjust_count_on_hand(5) + expect(subject.count_on_hand).to eq(current_on_hand + 5) + + expect(copy.count_on_hand).to eq(current_on_hand) + copy.adjust_count_on_hand(5) + expect(copy.count_on_hand).to eq(current_on_hand + 10) + end + + context "item out of stock (by two items)" do + let(:inventory_unit) { double('InventoryUnit') } + let(:inventory_unit_2) { double('InventoryUnit2') } + + before do + allow(subject).to receive(:backorderable?).and_return(true) + subject.adjust_count_on_hand(- (current_on_hand + 2)) + end + + it "doesn't process backorders" do + expect(subject).not_to receive(:backordered_inventory_units) + subject.adjust_count_on_hand(1) + end + + context "adds new items" do + before { allow(subject).to receive_messages(backordered_inventory_units: [inventory_unit, inventory_unit_2]) } + + it "fills existing backorders" do + expect(inventory_unit).to receive(:fill_backorder) + expect(inventory_unit_2).to receive(:fill_backorder) + + subject.adjust_count_on_hand(3) + expect(subject.count_on_hand).to eq(1) + end + end + end + end +end