From 0e711832fdff9f91ac2e70b9c1038d60cb1961e8 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 04:13:21 +0800 Subject: [PATCH 01/19] Bring Spree::StockItem code from spree_core into the app --- app/models/spree/stock_item.rb | 52 +++++++++++++++++++ spec/models/spree/stock_item_spec.rb | 74 ++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 app/models/spree/stock_item.rb create mode 100644 spec/models/spree/stock_item_spec.rb diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb new file mode 100644 index 0000000000..9dc322b447 --- /dev/null +++ b/app/models/spree/stock_item.rb @@ -0,0 +1,52 @@ +module Spree + class StockItem < ActiveRecord::Base + belongs_to :stock_location, class_name: 'Spree::StockLocation' + belongs_to :variant, class_name: 'Spree::Variant' + has_many :stock_movements, dependent: :destroy + + validates_presence_of :stock_location, :variant + validates_uniqueness_of :variant_id, scope: :stock_location_id + + attr_accessible :count_on_hand, :variant, :stock_location, :backorderable, :variant_id + + delegate :weight, to: :variant + + def backordered_inventory_units + Spree::InventoryUnit.backordered_for_stock_item(self) + end + + def variant_name + variant.name + end + + def adjust_count_on_hand(value) + self.with_lock do + self.count_on_hand = self.count_on_hand + value + process_backorders if in_stock? + + self.save! + end + end + + def in_stock? + self.count_on_hand > 0 + end + + # Tells whether it's available to be included in a shipment + def available? + self.in_stock? || self.backorderable? + end + + private + def count_on_hand=(value) + write_attribute(:count_on_hand, value) + end + + def process_backorders + backordered_inventory_units.each do |unit| + return unless in_stock? + unit.fill_backorder + end + end + end +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..da10d44efa --- /dev/null +++ b/spec/models/spree/stock_item_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' + +describe Spree::StockItem do + let(:stock_location) { create(:stock_location_with_items) } + + subject { stock_location.stock_items.order(:id).first } + + it 'maintains the count on hand for a variant' do + subject.count_on_hand.should eq 10 + end + + it "can return the stock item's variant's name" do + subject.variant_name.should == subject.variant.name + end + + context "available to be included in shipment" do + context "has stock" do + it { subject.should be_available } + end + + context "backorderable" do + before { subject.backorderable = true } + it { subject.should be_available } + end + + context "no stock and not backorderable" do + before do + subject.backorderable = false + subject.stub(count_on_hand: 0) + end + + it { subject.should_not 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) + subject.count_on_hand.should eq(current_on_hand + 5) + + copy.count_on_hand.should eq(current_on_hand) + copy.adjust_count_on_hand(5) + copy.count_on_hand.should 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 { subject.adjust_count_on_hand(- (current_on_hand + 2)) } + + it "doesn't process backorders" do + subject.should_not_receive(:backordered_inventory_units) + subject.adjust_count_on_hand(1) + end + + context "adds new items" do + before { subject.stub(:backordered_inventory_units => [inventory_unit, inventory_unit_2]) } + + it "fills existing backorders" do + inventory_unit.should_receive(:fill_backorder) + inventory_unit_2.should_receive(:fill_backorder) + + subject.adjust_count_on_hand(3) + subject.count_on_hand.should == 1 + end + end + end + end +end From 84d973d383d816b2760a4c0c4bf926875a341cae Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 04:19:24 +0800 Subject: [PATCH 02/19] Specify RSpec.describe in StockItem spec file --- spec/models/spree/stock_item_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb index da10d44efa..2e6b757a71 100644 --- a/spec/models/spree/stock_item_spec.rb +++ b/spec/models/spree/stock_item_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Spree::StockItem do +RSpec.describe Spree::StockItem do let(:stock_location) { create(:stock_location_with_items) } subject { stock_location.stock_items.order(:id).first } From b78311870065f63246831d30dbf1ad91ea9c292d Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 04:40:13 +0800 Subject: [PATCH 03/19] Auto-correct violationso of Rubocop Style/RedundantSelf --- app/models/spree/stock_item.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 9dc322b447..70bb741e8d 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -20,21 +20,21 @@ module Spree end def adjust_count_on_hand(value) - self.with_lock do - self.count_on_hand = self.count_on_hand + value + with_lock do + self.count_on_hand = count_on_hand + value process_backorders if in_stock? - self.save! + save! end end def in_stock? - self.count_on_hand > 0 + count_on_hand > 0 end # Tells whether it's available to be included in a shipment def available? - self.in_stock? || self.backorderable? + in_stock? || backorderable? end private From 0fd66f9a558caccfa639a864c636261bf17a0962 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 04:44:07 +0800 Subject: [PATCH 04/19] Auto-correct violationso of Rubocop Style/* --- app/models/spree/stock_item.rb | 1 + spec/models/spree/stock_item_spec.rb | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 70bb741e8d..d1c9faa983 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true module Spree class StockItem < ActiveRecord::Base belongs_to :stock_location, class_name: 'Spree::StockLocation' diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb index 2e6b757a71..f58a0bd09c 100644 --- a/spec/models/spree/stock_item_spec.rb +++ b/spec/models/spree/stock_item_spec.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true require 'spec_helper' RSpec.describe Spree::StockItem do @@ -59,7 +60,7 @@ RSpec.describe Spree::StockItem do end context "adds new items" do - before { subject.stub(:backordered_inventory_units => [inventory_unit, inventory_unit_2]) } + before { subject.stub(backordered_inventory_units: [inventory_unit, inventory_unit_2]) } it "fills existing backorders" do inventory_unit.should_receive(:fill_backorder) From d1725014c4495481c83e9dd0b88febbd1b150e4e Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 04:44:23 +0800 Subject: [PATCH 05/19] Auto-correct violationso of Rubocop Layout/* --- app/models/spree/stock_item.rb | 19 +++++++++++-------- spec/models/spree/stock_item_spec.rb | 1 + 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index d1c9faa983..29ec2401fa 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + module Spree class StockItem < ActiveRecord::Base belongs_to :stock_location, class_name: 'Spree::StockLocation' @@ -39,15 +40,17 @@ module Spree end private - def count_on_hand=(value) - write_attribute(:count_on_hand, value) - end - def process_backorders - backordered_inventory_units.each do |unit| - return unless in_stock? - unit.fill_backorder - end + def count_on_hand=(value) + write_attribute(:count_on_hand, value) + end + + def process_backorders + backordered_inventory_units.each do |unit| + return unless in_stock? + + unit.fill_backorder end + end end end diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb index f58a0bd09c..c59416371d 100644 --- a/spec/models/spree/stock_item_spec.rb +++ b/spec/models/spree/stock_item_spec.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + require 'spec_helper' RSpec.describe Spree::StockItem do From 22c0693bebdf65346f11ab914db87b35a1166fe5 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 04:45:52 +0800 Subject: [PATCH 06/19] Address violation of Rubocop Style/NumericPredicate --- app/models/spree/stock_item.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 29ec2401fa..5c9f390567 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -31,7 +31,7 @@ module Spree end def in_stock? - count_on_hand > 0 + count_on_hand.positive? end # Tells whether it's available to be included in a shipment From 1e8543dfe7a9a1c420fc0011a1012a064d8daf97 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 04:50:57 +0800 Subject: [PATCH 07/19] Address violation of Rubocop Rails/ReadWriteAttribute --- app/models/spree/stock_item.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 5c9f390567..47bbf14fbd 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -42,7 +42,7 @@ module Spree private def count_on_hand=(value) - write_attribute(:count_on_hand, value) + self[:count_on_hand] = value end def process_backorders From 2acf61fd0f08ab8424c3d8422c469d82cc1dc226 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 04:54:50 +0800 Subject: [PATCH 08/19] Address violation of Rubocop Rails/Delegate --- app/models/spree/stock_item.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 47bbf14fbd..44aa72f422 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -17,9 +17,7 @@ module Spree Spree::InventoryUnit.backordered_for_stock_item(self) end - def variant_name - variant.name - end + delegate :name, to: :variant, prefix: true def adjust_count_on_hand(value) with_lock do From bc530b92b540bf2639872f8832633230c35e85eb Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 04:55:42 +0800 Subject: [PATCH 09/19] Address violation of Rubocop Rails/Validation: --- app/models/spree/stock_item.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 44aa72f422..47c311b205 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -6,8 +6,8 @@ module Spree belongs_to :variant, class_name: 'Spree::Variant' has_many :stock_movements, dependent: :destroy - validates_presence_of :stock_location, :variant - validates_uniqueness_of :variant_id, scope: :stock_location_id + validates :stock_location, :variant, presence: true + validates :variant_id, uniqueness: { scope: :stock_location_id } attr_accessible :count_on_hand, :variant, :stock_location, :backorderable, :variant_id From 0a1cb71ee4f8b64c52ade2a98d73563e2f524460 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 05:00:52 +0800 Subject: [PATCH 10/19] Ignore Rails/UniqueValidationWithoutIndex for unique index of StockItem#stock_location --- app/models/spree/stock_item.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 47c311b205..9eca73d0c8 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -7,7 +7,9 @@ module Spree has_many :stock_movements, dependent: :destroy validates :stock_location, :variant, presence: true + # rubocop:disable Rails/UniqueValidationWithoutIndex validates :variant_id, uniqueness: { scope: :stock_location_id } + # rubocop:enable Rails/UniqueValidationWithoutIndex attr_accessible :count_on_hand, :variant, :stock_location, :backorderable, :variant_id From fb20f220c04f83f938fee31f7023d9c8e01b88e7 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 05:16:58 +0800 Subject: [PATCH 11/19] Use break instead of return in StockItem#process_backorders We are not using the return value of this method anywhere. --- app/models/spree/stock_item.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 9eca73d0c8..b3e14969eb 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -47,7 +47,7 @@ module Spree def process_backorders backordered_inventory_units.each do |unit| - return unless in_stock? + break unless in_stock? unit.fill_backorder end From 13ecf0ec73ba49782fe1a92e1c186196c14598fb Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 15:20:20 +0800 Subject: [PATCH 12/19] Update specs for StockItem with transpec --- spec/models/spree/stock_item_spec.rb | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb index c59416371d..ce5c7fafd0 100644 --- a/spec/models/spree/stock_item_spec.rb +++ b/spec/models/spree/stock_item_spec.rb @@ -8,30 +8,30 @@ RSpec.describe Spree::StockItem do subject { stock_location.stock_items.order(:id).first } it 'maintains the count on hand for a variant' do - subject.count_on_hand.should eq 10 + expect(subject.count_on_hand).to eq 10 end it "can return the stock item's variant's name" do - subject.variant_name.should == subject.variant.name + expect(subject.variant_name).to eq(subject.variant.name) end context "available to be included in shipment" do context "has stock" do - it { subject.should be_available } + it { expect(subject).to be_available } end context "backorderable" do before { subject.backorderable = true } - it { subject.should be_available } + it { expect(subject).to be_available } end context "no stock and not backorderable" do before do subject.backorderable = false - subject.stub(count_on_hand: 0) + allow(subject).to receive_messages(count_on_hand: 0) end - it { subject.should_not be_available } + it { expect(subject).not_to be_available } end end @@ -42,11 +42,11 @@ RSpec.describe Spree::StockItem do copy = Spree::StockItem.find(subject.id) subject.adjust_count_on_hand(5) - subject.count_on_hand.should eq(current_on_hand + 5) + expect(subject.count_on_hand).to eq(current_on_hand + 5) - copy.count_on_hand.should eq(current_on_hand) + expect(copy.count_on_hand).to eq(current_on_hand) copy.adjust_count_on_hand(5) - copy.count_on_hand.should eq(current_on_hand + 10) + expect(copy.count_on_hand).to eq(current_on_hand + 10) end context "item out of stock (by two items)" do @@ -56,19 +56,19 @@ RSpec.describe Spree::StockItem do before { subject.adjust_count_on_hand(- (current_on_hand + 2)) } it "doesn't process backorders" do - subject.should_not_receive(:backordered_inventory_units) + expect(subject).not_to receive(:backordered_inventory_units) subject.adjust_count_on_hand(1) end context "adds new items" do - before { subject.stub(backordered_inventory_units: [inventory_unit, inventory_unit_2]) } + before { allow(subject).to receive_messages(backordered_inventory_units: [inventory_unit, inventory_unit_2]) } it "fills existing backorders" do - inventory_unit.should_receive(:fill_backorder) - inventory_unit_2.should_receive(:fill_backorder) + expect(inventory_unit).to receive(:fill_backorder) + expect(inventory_unit_2).to receive(:fill_backorder) subject.adjust_count_on_hand(3) - subject.count_on_hand.should == 1 + expect(subject.count_on_hand).to eq(1) end end end From 774b3720d5853a408de6b890add2df54981bec71 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 14 May 2020 15:23:31 +0800 Subject: [PATCH 13/19] Update stock item count on hand in Spree core specs --- spec/models/spree/stock_item_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb index ce5c7fafd0..679af05f90 100644 --- a/spec/models/spree/stock_item_spec.rb +++ b/spec/models/spree/stock_item_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Spree::StockItem do subject { stock_location.stock_items.order(:id).first } it 'maintains the count on hand for a variant' do - expect(subject.count_on_hand).to eq 10 + expect(subject.count_on_hand).to eq 15 end it "can return the stock item's variant's name" do From e53913756c17f90d3405af8258c15c8a66fba028 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Tue, 12 May 2020 15:09:53 +0800 Subject: [PATCH 14/19] Add lock_version to Spree::StockItem --- db/migrate/20200512070717_add_lock_version_to_stock_items.rb | 5 +++++ db/schema.rb | 1 + 2 files changed, 6 insertions(+) create mode 100644 db/migrate/20200512070717_add_lock_version_to_stock_items.rb 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/schema.rb b/db/schema.rb index d166fdfd3c..d153eec8ef 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -899,6 +899,7 @@ ActiveRecord::Schema.define(version: 20200623140437) do t.datetime "updated_at" 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 From 4694f1b21a67467738e83859aa2ed537b4850dac Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Tue, 12 May 2020 15:28:26 +0800 Subject: [PATCH 15/19] Require count on hand in non backorderable StockItem to be positive or zero Fix setting of count on hand in line item specs --- app/models/spree/stock_item.rb | 1 + spec/models/spree/line_item_spec.rb | 4 ++-- spec/models/spree/stock_item_spec.rb | 32 +++++++++++++++++++++++++++- 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index b3e14969eb..f71df2b942 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -10,6 +10,7 @@ module Spree # rubocop:disable Rails/UniqueValidationWithoutIndex validates :variant_id, uniqueness: { scope: :stock_location_id } # rubocop:enable Rails/UniqueValidationWithoutIndex + validates :count_on_hand, numericality: { greater_than_or_equal_to: 0, unless: :backorderable? } attr_accessible :count_on_hand, :variant, :stock_location, :backorderable, :variant_id diff --git a/spec/models/spree/line_item_spec.rb b/spec/models/spree/line_item_spec.rb index ca4a5353d9..ff00240aaf 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 index 679af05f90..9c9b233986 100644 --- a/spec/models/spree/stock_item_spec.rb +++ b/spec/models/spree/stock_item_spec.rb @@ -7,6 +7,33 @@ RSpec.describe Spree::StockItem do 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 @@ -53,7 +80,10 @@ RSpec.describe Spree::StockItem do let(:inventory_unit) { double('InventoryUnit') } let(:inventory_unit_2) { double('InventoryUnit2') } - before { subject.adjust_count_on_hand(- (current_on_hand + 2)) } + 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) From 20fd3c2642d2abdc262d6dff3d88c1c6c5be6e72 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 15 May 2020 01:56:34 +0800 Subject: [PATCH 16/19] Reset negative count on hand in existing non backorderable stock items --- ...korderable_count_on_hand_in_stock_items.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 db/migrate/20200514174526_reset_negative_nonbackorderable_count_on_hand_in_stock_items.rb 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 From e12e50aa84b5621bbb2e71ca06206fc4368402c0 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Mon, 22 Jun 2020 17:55:54 +0100 Subject: [PATCH 17/19] Move rubocop exception to rubocop todo --- .rubocop_todo.yml | 7 +++++++ app/models/spree/stock_item.rb | 2 -- 2 files changed, 7 insertions(+), 2 deletions(-) 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 index f71df2b942..cf86103453 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -7,9 +7,7 @@ module Spree has_many :stock_movements, dependent: :destroy validates :stock_location, :variant, presence: true - # rubocop:disable Rails/UniqueValidationWithoutIndex validates :variant_id, uniqueness: { scope: :stock_location_id } - # rubocop:enable Rails/UniqueValidationWithoutIndex validates :count_on_hand, numericality: { greater_than_or_equal_to: 0, unless: :backorderable? } attr_accessible :count_on_hand, :variant, :stock_location, :backorderable, :variant_id From 34207fc20ffbd5a5463366e573daf4fd06d4ff3d Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Mon, 22 Jun 2020 18:19:05 +0100 Subject: [PATCH 18/19] Bring changes to stock_item from spree 2.1, the previous version was from spree 2.0.4 --- app/models/spree/stock_item.rb | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index cf86103453..e6df19b03a 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -2,24 +2,23 @@ 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 } + validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] } validates :count_on_hand, numericality: { greater_than_or_equal_to: 0, unless: :backorderable? } - attr_accessible :count_on_hand, :variant, :stock_location, :backorderable, :variant_id - delegate :weight, to: :variant + delegate :name, to: :variant, prefix: true def backordered_inventory_units Spree::InventoryUnit.backordered_for_stock_item(self) end - delegate :name, to: :variant, prefix: true - def adjust_count_on_hand(value) with_lock do self.count_on_hand = count_on_hand + value @@ -38,6 +37,10 @@ module Spree in_stock? || backorderable? end + def variant + Spree::Variant.unscoped { super } + end + private def count_on_hand=(value) From ba50491c6d2e6699368aea68259fc99c3089597a Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Mon, 22 Jun 2020 19:28:28 +0100 Subject: [PATCH 19/19] Restructure the spec a little --- .../checkout_controller_concurrency_spec.rb | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) 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.