From fc5882686a0dfa9625075ca45f703e046e989050 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Mon, 3 Sep 2018 17:44:37 +0200
Subject: [PATCH 1/7] Add unit tests to VariantStock

This also replaces the after_save callback to after_update.

We enforce a stock_item exists when modifying the variant and the
stock_item gets created on variant's after_create. This means it's not
possible to use any of the VariantStock's setters before the variant is
persisted so executing the callback on creation is pointless.
---
 lib/open_food_network/variant_stock.rb        |  83 ++++++++--
 .../open_food_network/variant_stock_spec.rb   | 150 ++++++++++++++++++
 2 files changed, 216 insertions(+), 17 deletions(-)
 create mode 100644 spec/lib/open_food_network/variant_stock_spec.rb

diff --git a/lib/open_food_network/variant_stock.rb b/lib/open_food_network/variant_stock.rb
index fba38ebd57..b94505db60 100644
--- a/lib/open_food_network/variant_stock.rb
+++ b/lib/open_food_network/variant_stock.rb
@@ -1,19 +1,31 @@
 require 'active_support/concern'
 
-# These methods were available in Spree 1, but were removed in Spree 2.
-# We would still like to use them. Therefore we use only a single stock location
-# (default stock location) and use it to track the `count_on_hand` value that
-# was previously a database column on variants.
+# These methods were available in Spree 1, but were removed in Spree 2.  We
+# would still like to use them so that we still give support to the consumers
+# of this methods, making the upgrade backward compatible.
 #
-# We may decide to deprecate these methods after we designed the Network feature.
+# Therefore we use only a single stock item per variant, which is associated to
+# a single stock location per instance (default stock location) and use it to
+# track the `count_on_hand` value that was previously a database column on
+# variants. See
+# https://github.com/openfoodfoundation/openfoodnetwork/wiki/Spree-Upgrade%3A-Stock-locations
+# for details.
+#
+# These methods are or may become deprecated.
 module OpenFoodNetwork
   module VariantStock
     extend ActiveSupport::Concern
 
     included do
-      after_save :save_stock
+      after_update :save_stock
     end
 
+    # Returns the number of items of the variant available in the stock. When
+    # allowing on demand, it returns infinite.
+    #
+    # Spree computes it as the sum of the count_on_hand of all its stock_items.
+    #
+    # @return [Float|Integer]
     def on_hand
       if on_demand
         Float::INFINITY
@@ -22,10 +34,18 @@ module OpenFoodNetwork
       end
     end
 
+    # Returns the number of items available in the stock for this variant
+    #
+    # @return [Float|Integer]
     def count_on_hand
       total_on_hand
     end
 
+    # Sets the stock level when `track_inventory_levels` config is
+    # set. It raises otherwise.
+    #
+    # @raise [StandardError] when the track_inventory_levels config
+    # key is not set.
     def on_hand=(new_level)
       error = 'Cannot set on_hand value when Spree::Config[:track_inventory_levels] is false'
       raise error unless Spree::Config.track_inventory_levels
@@ -33,19 +53,47 @@ module OpenFoodNetwork
       self.count_on_hand = new_level
     end
 
+    # Sets the stock level. As opposed to #on_hand= it does not check
+    # `track_inventory_levels`'s value as it was previously an ActiveModel
+    # setter of te database column of the `spree_variants` table. That is why
+    # #on_hand= is more widely used in Spree's codebase using #count_on_hand=
+    # underneath.
+    #
+    # So, if #count_on_hand= is used, `track_inventory_levels` won't be tacken
+    # into account thus dismissing instance's configuration.
+    #
+    # It does ensure there's a stock item for the variant however. See
+    # #raise_error_if_no_stock_item_available for details.
+    #
+    # @raise [StandardError] when the variant has no stock item yet
     def count_on_hand=(new_level)
       raise_error_if_no_stock_item_available
-      overwrite_stock_levels new_level
+      overwrite_stock_levels(new_level)
     end
 
+    # Checks whether this variant is produced on demand.
+    #
+    # In Spree 2.0 this attribute is removed in favour of
+    # track_inventory_levels only. It was initially introduced in
+    # https://github.com/openfoodfoundation/spree/commit/20b5ad9835dca7f41a40ad16c7b45f987eea6dcc
     def on_demand
-      stock_items.any?(&:backorderable?)
+      warn_deprecation(__method__)
+      stock_items.first.backorderable?
     end
 
+    # Sets whether the variant can be ordered on demand or not. Note that
+    # although this modifies the stock item, it is not persisted in DB. This
+    # may be done to fire a single UPDATE statement when changing various
+    # variant attributes, for performance reasons.
+    #
+    # @raise [StandardError] when the variant has no stock item yet
     def on_demand=(new_value)
       raise_error_if_no_stock_item_available
 
       # There should be only one at the default stock location.
+      #
+      # This would be better off as `stock_items.first.save` but then, for
+      # unknown reasons, it does not pass the test.
       stock_items.each do |item|
         item.backorderable = new_value
       end
@@ -53,8 +101,14 @@ module OpenFoodNetwork
 
     private
 
+    # Persists the single stock item associated to this variant. As defined in
+    # the top-most comment, as there's a single stock location in the whole
+    # database, there can only be a stock item per variant. See StockItem's
+    # definition at
+    # https://github.com/openfoodfoundation/spree/blob/43950c3689a77a7f493cc6d805a0edccfe75ebc2/core/app/models/spree/stock_item.rb#L3-L4
+    # for details.
     def save_stock
-      stock_items.each(&:save)
+      stock_items.first.save
     end
 
     def raise_error_if_no_stock_item_available
@@ -67,14 +121,9 @@ module OpenFoodNetwork
     # takes a value to add to the current stock level and uses proper locking.
     # But this should work the same as in Spree 1.3.
     def overwrite_stock_levels(new_level)
-      stock_items.first.send :count_on_hand=, new_level
-
-      # There shouldn't be any other stock items, because we should have only one
-      # stock location. But in case there are, the total should be new_level,
-      # so all others need to be zero.
-      stock_items[1..-1].each do |item|
-        item.send :count_on_hand=, 0
-      end
+      # There shouldn't be any other stock items, because we should
+      # have only one stock location.
+      stock_items.first.__send__(:count_on_hand=, new_level)
     end
   end
 end
diff --git a/spec/lib/open_food_network/variant_stock_spec.rb b/spec/lib/open_food_network/variant_stock_spec.rb
new file mode 100644
index 0000000000..1c3efb88ca
--- /dev/null
+++ b/spec/lib/open_food_network/variant_stock_spec.rb
@@ -0,0 +1,150 @@
+require 'spec_helper'
+
+describe OpenFoodNetwork::VariantStock do
+  let(:variant) { create(:variant) }
+
+  describe '#after_save' do
+    context 'when updating a variant' do
+      let(:variant) { create(:variant) }
+      let(:stock_item) { variant.stock_items.first }
+
+      before { allow(stock_item).to receive(:save) }
+
+      it 'saves its stock item' do
+        variant.save
+        expect(stock_item).to have_received(:save)
+      end
+    end
+  end
+
+  describe '#on_hand' do
+    context 'when the variant is ordered on demand' do
+      before do
+        variant.stock_items.first.update_attribute(
+          :backorderable, true
+        )
+      end
+
+      it 'returns infinite' do
+        expect(variant.on_hand).to eq(Float::INFINITY)
+      end
+    end
+
+    context 'when the variant is not ordered on demand' do
+      before do
+        variant.stock_items.first.update_attribute(
+          :backorderable, false
+        )
+      end
+
+      it 'returns the total items in stock' do
+        expect(variant.on_hand)
+          .to eq(variant.stock_items.sum(&:count_on_hand))
+      end
+    end
+  end
+
+  describe '#count_on_hand' do
+    before { allow(variant).to receive(:total_on_hand) }
+
+    it 'delegates to #total_on_hand' do
+      variant.count_on_hand
+      expect(variant).to have_received(:total_on_hand)
+    end
+  end
+
+  describe '#on_hand=' do
+    context 'when track_inventory_levels is set' do
+      before do
+        allow(variant).to receive(:count_on_hand=)
+        allow(Spree::Config)
+          .to receive(:track_inventory_levels) { true }
+      end
+
+      it 'delegates to #count_on_hand=' do
+        variant.on_hand = 3
+        expect(variant)
+          .to have_received(:count_on_hand=).with(3)
+      end
+    end
+
+    context 'when track_inventory_levels is not set' do
+      before do
+        allow(Spree::Config)
+          .to receive(:track_inventory_levels) { false }
+      end
+
+      it 'raises' do
+        expect { variant.on_hand = 3 }
+          .to raise_error(StandardError)
+      end
+    end
+  end
+
+  describe '#count_on_hand=' do
+    context 'when the variant has a stock item' do
+      let(:variant) { create(:variant) }
+
+      it 'sets the new level as the stock item\'s count_on_hand' do
+        variant.count_on_hand = 3
+        unique_stock_item = variant.stock_items.first
+        expect(unique_stock_item.count_on_hand).to eq(3)
+      end
+    end
+
+    context 'when the variant has no stock item' do
+      let(:variant) { build(:variant) }
+
+      it 'raises' do
+        expect { variant.count_on_hand = 3 }
+          .to raise_error(StandardError)
+      end
+    end
+  end
+
+  describe '#on_demand' do
+    context 'when the stock items is backorderable' do
+      before do
+        variant.stock_items.first.update_attribute(
+          :backorderable, true
+        )
+      end
+
+      it 'returns true' do
+        expect(variant.on_demand).to be_truthy
+      end
+    end
+
+    context 'when the stock items is backorderable' do
+      before do
+        variant.stock_items.first.update_attribute(
+          :backorderable, false
+        )
+      end
+
+      it 'returns false' do
+        expect(variant.on_demand).to be_falsy
+      end
+    end
+  end
+
+  describe '#on_demand=' do
+    context 'when the variant has a stock item' do
+      let(:variant) { create(:variant, on_demand: true) }
+
+      it 'sets the value as the stock item\'s backorderable value' do
+        variant.on_demand = false
+        stock_item = variant.stock_items.first
+        expect(stock_item.backorderable).to eq(false)
+      end
+    end
+
+    context 'when the variant has no stock item' do
+      let(:variant) { build(:variant) }
+
+      it 'raises' do
+        expect { variant.on_demand = 3 }.to raise_error(StandardError)
+      end
+    end
+  end
+end

From 59883f95095dafd20de6af3b88bbb9088c49ae0f Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Mon, 3 Sep 2018 18:06:53 +0200
Subject: [PATCH 2/7] Add deprecation warnings for VariantStock methods

This will prevent other devs from relying on this methods and will tell
us the exact lines that call this methods, which will come in handy when
removing this module.
---
 lib/open_food_network/variant_stock.rb | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/lib/open_food_network/variant_stock.rb b/lib/open_food_network/variant_stock.rb
index b94505db60..0b2773542d 100644
--- a/lib/open_food_network/variant_stock.rb
+++ b/lib/open_food_network/variant_stock.rb
@@ -27,6 +27,8 @@ module OpenFoodNetwork
     #
     # @return [Float|Integer]
     def on_hand
+      warn_deprecation(__method__, '#total_on_hand')
+
       if on_demand
         Float::INFINITY
       else
@@ -38,6 +40,7 @@ module OpenFoodNetwork
     #
     # @return [Float|Integer]
     def count_on_hand
+      warn_deprecation(__method__, '#total_on_hand')
       total_on_hand
     end
 
@@ -47,6 +50,8 @@ module OpenFoodNetwork
     # @raise [StandardError] when the track_inventory_levels config
     # key is not set.
     def on_hand=(new_level)
+      warn_deprecation(__method__, '#total_on_hand')
+
       error = 'Cannot set on_hand value when Spree::Config[:track_inventory_levels] is false'
       raise error unless Spree::Config.track_inventory_levels
 
@@ -67,6 +72,8 @@ module OpenFoodNetwork
     #
     # @raise [StandardError] when the variant has no stock item yet
     def count_on_hand=(new_level)
+      warn_deprecation(__method__, '#total_on_hand')
+
       raise_error_if_no_stock_item_available
       overwrite_stock_levels(new_level)
     end
@@ -77,7 +84,7 @@ module OpenFoodNetwork
     # track_inventory_levels only. It was initially introduced in
     # https://github.com/openfoodfoundation/spree/commit/20b5ad9835dca7f41a40ad16c7b45f987eea6dcc
     def on_demand
-      warn_deprecation(__method__)
+      warn_deprecation(__method__, 'Spree::Config[:track_inventory_levels]')
       stock_items.first.backorderable?
     end
 
@@ -88,6 +95,8 @@ module OpenFoodNetwork
     #
     # @raise [StandardError] when the variant has no stock item yet
     def on_demand=(new_value)
+      warn_deprecation(__method__, 'Spree::Config[:track_inventory_levels]')
+
       raise_error_if_no_stock_item_available
 
       # There should be only one at the default stock location.
@@ -125,5 +134,12 @@ module OpenFoodNetwork
       # have only one stock location.
       stock_items.first.__send__(:count_on_hand=, new_level)
     end
+
+    def warn_deprecation(method_name, new_method_name)
+      ActiveSupport::Deprecation.warn(
+        "`##{method_name}` is deprecated and will be removed. " \
+        "Please use `#{new_method_name}` instead."
+      )
+    end
   end
 end

From 8848af15ee4eeaa03c2b19258b0620155de57199 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Tue, 4 Sep 2018 13:26:42 +0200
Subject: [PATCH 3/7] Raise when a variant has more than a stock item

This means we violated the business rules of having a single stock
location for the OFN instance and hence a single stock item per variant.

Although it is too late to preserve the data integrity we can know data
needs to be cleaned up.
---
 lib/open_food_network/variant_stock.rb           |  6 ++++++
 spec/lib/open_food_network/variant_stock_spec.rb | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/lib/open_food_network/variant_stock.rb b/lib/open_food_network/variant_stock.rb
index 0b2773542d..98bde544f7 100644
--- a/lib/open_food_network/variant_stock.rb
+++ b/lib/open_food_network/variant_stock.rb
@@ -98,6 +98,7 @@ module OpenFoodNetwork
       warn_deprecation(__method__, 'Spree::Config[:track_inventory_levels]')
 
       raise_error_if_no_stock_item_available
+      raise_error_if_multiple_stock_items
 
       # There should be only one at the default stock location.
       #
@@ -125,6 +126,11 @@ module OpenFoodNetwork
       raise message if stock_items.empty?
     end
 
+    def raise_error_if_multiple_stock_items
+      message = 'A variant cannot have more than a stock item.'
+      raise message if stock_items.size > 1
+    end
+
     # Backwards compatible setting of stock levels in Spree 2.0.
     # It would be better to use `Spree::StockItem.adjust_count_on_hand` which
     # takes a value to add to the current stock level and uses proper locking.
diff --git a/spec/lib/open_food_network/variant_stock_spec.rb b/spec/lib/open_food_network/variant_stock_spec.rb
index 1c3efb88ca..b0818f6690 100644
--- a/spec/lib/open_food_network/variant_stock_spec.rb
+++ b/spec/lib/open_food_network/variant_stock_spec.rb
@@ -129,6 +129,21 @@ describe OpenFoodNetwork::VariantStock do
   end
 
   describe '#on_demand=' do
+    context 'when the variant has multiple stock items' do
+      let(:variant) { create(:variant) }
+
+      before do
+        # Spree creates a stock_item for each variant when creating a stock
+        # location by means of #propagate_variant
+        create(:stock_location, name: 'location')
+        create(:stock_location, name: 'another location')
+      end
+
+      it 'raises' do
+        expect { variant.on_demand = true }.to raise_error(StandardError)
+      end
+    end
+
     context 'when the variant has a stock item' do
       let(:variant) { create(:variant, on_demand: true) }
 

From cd53ec1a4f84948e7d449e26bf19da2e0c7ef737 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Thu, 6 Sep 2018 13:16:20 +0200
Subject: [PATCH 4/7] Replace exception with DB uniqueness constraint

By forbidding more than a row per variant in the spree_stock_items we
can ensure all variants have a single stock_item associated.
---
 ...641_add_uniqueness_of_variant_id_to_spree_stock_items.rb | 5 +++++
 db/schema.rb                                                | 3 ++-
 lib/open_food_network/variant_stock.rb                      | 6 ------
 3 files changed, 7 insertions(+), 7 deletions(-)
 create mode 100644 db/migrate/20180906094641_add_uniqueness_of_variant_id_to_spree_stock_items.rb

diff --git a/db/migrate/20180906094641_add_uniqueness_of_variant_id_to_spree_stock_items.rb b/db/migrate/20180906094641_add_uniqueness_of_variant_id_to_spree_stock_items.rb
new file mode 100644
index 0000000000..bc7713f428
--- /dev/null
+++ b/db/migrate/20180906094641_add_uniqueness_of_variant_id_to_spree_stock_items.rb
@@ -0,0 +1,5 @@
+class AddUniquenessOfVariantIdToSpreeStockItems < ActiveRecord::Migration
+  def change
+    add_index :spree_stock_items, :variant_id, unique: true
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 1d7f167724..a93d5819f8 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended to check this file into your version control system.
 
-ActiveRecord::Schema.define(:version => 20180812214434) do
+ActiveRecord::Schema.define(:version => 20180906094641) do
 
   create_table "account_invoices", :force => true do |t|
     t.integer  "user_id",    :null => false
@@ -961,6 +961,7 @@ ActiveRecord::Schema.define(:version => 20180812214434) do
 
   add_index "spree_stock_items", ["stock_location_id", "variant_id"], :name => "stock_item_by_loc_and_var_id"
   add_index "spree_stock_items", ["stock_location_id"], :name => "index_spree_stock_items_on_stock_location_id"
+  add_index "spree_stock_items", ["variant_id"], :name => "index_spree_stock_items_on_variant_id", :unique => true
 
   create_table "spree_stock_locations", :force => true do |t|
     t.string   "name"
diff --git a/lib/open_food_network/variant_stock.rb b/lib/open_food_network/variant_stock.rb
index 98bde544f7..0b2773542d 100644
--- a/lib/open_food_network/variant_stock.rb
+++ b/lib/open_food_network/variant_stock.rb
@@ -98,7 +98,6 @@ module OpenFoodNetwork
       warn_deprecation(__method__, 'Spree::Config[:track_inventory_levels]')
 
       raise_error_if_no_stock_item_available
-      raise_error_if_multiple_stock_items
 
       # There should be only one at the default stock location.
       #
@@ -126,11 +125,6 @@ module OpenFoodNetwork
       raise message if stock_items.empty?
     end
 
-    def raise_error_if_multiple_stock_items
-      message = 'A variant cannot have more than a stock item.'
-      raise message if stock_items.size > 1
-    end
-
     # Backwards compatible setting of stock levels in Spree 2.0.
     # It would be better to use `Spree::StockItem.adjust_count_on_hand` which
     # takes a value to add to the current stock level and uses proper locking.

From 8d8670bdf4655dad11103c3657809f2225b5d275 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Mon, 10 Sep 2018 14:26:19 +0200
Subject: [PATCH 5/7] Fix typos in comments

---
 lib/open_food_network/variant_stock.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/open_food_network/variant_stock.rb b/lib/open_food_network/variant_stock.rb
index 0b2773542d..18c9be1a79 100644
--- a/lib/open_food_network/variant_stock.rb
+++ b/lib/open_food_network/variant_stock.rb
@@ -60,11 +60,11 @@ module OpenFoodNetwork
 
     # Sets the stock level. As opposed to #on_hand= it does not check
     # `track_inventory_levels`'s value as it was previously an ActiveModel
-    # setter of te database column of the `spree_variants` table. That is why
+    # setter of the database column of the `spree_variants` table. That is why
     # #on_hand= is more widely used in Spree's codebase using #count_on_hand=
     # underneath.
     #
-    # So, if #count_on_hand= is used, `track_inventory_levels` won't be tacken
+    # So, if #count_on_hand= is used, `track_inventory_levels` won't be taken
     # into account thus dismissing instance's configuration.
     #
     # It does ensure there's a stock item for the variant however. See

From 314ad5400f994682b171637a37fc4ebce94136b1 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Mon, 10 Sep 2018 18:58:42 +0200
Subject: [PATCH 6/7] Move variant_stock.rb to concerns/

---
 app/models/concerns/variant_stock.rb          | 143 +++++++++++++++++
 app/models/spree/variant_decorator.rb         |   4 +-
 lib/open_food_network/variant_stock.rb        | 145 ------------------
 .../concerns}/variant_stock_spec.rb           |   2 +-
 4 files changed, 146 insertions(+), 148 deletions(-)
 create mode 100644 app/models/concerns/variant_stock.rb
 delete mode 100644 lib/open_food_network/variant_stock.rb
 rename spec/{lib/open_food_network => models/concerns}/variant_stock_spec.rb (99%)

diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb
new file mode 100644
index 0000000000..2011f85c46
--- /dev/null
+++ b/app/models/concerns/variant_stock.rb
@@ -0,0 +1,143 @@
+require 'active_support/concern'
+
+# These methods were available in Spree 1, but were removed in Spree 2.  We
+# would still like to use them so that we still give support to the consumers
+# of this methods, making the upgrade backward compatible.
+#
+# Therefore we use only a single stock item per variant, which is associated to
+# a single stock location per instance (default stock location) and use it to
+# track the `count_on_hand` value that was previously a database column on
+# variants. See
+# https://github.com/openfoodfoundation/openfoodnetwork/wiki/Spree-Upgrade%3A-Stock-locations
+# for details.
+#
+# These methods are or may become deprecated.
+module VariantStock
+  extend ActiveSupport::Concern
+
+  included do
+    after_update :save_stock
+  end
+
+  # Returns the number of items of the variant available in the stock. When
+  # allowing on demand, it returns infinite.
+  #
+  # Spree computes it as the sum of the count_on_hand of all its stock_items.
+  #
+  # @return [Float|Integer]
+  def on_hand
+    warn_deprecation(__method__, '#total_on_hand')
+
+    if on_demand
+      Float::INFINITY
+    else
+      total_on_hand
+    end
+  end
+
+  # Returns the number of items available in the stock for this variant
+  #
+  # @return [Float|Integer]
+  def count_on_hand
+    warn_deprecation(__method__, '#total_on_hand')
+    total_on_hand
+  end
+
+  # Sets the stock level when `track_inventory_levels` config is
+  # set. It raises otherwise.
+  #
+  # @raise [StandardError] when the track_inventory_levels config
+  # key is not set.
+  def on_hand=(new_level)
+    warn_deprecation(__method__, '#total_on_hand')
+
+    error = 'Cannot set on_hand value when Spree::Config[:track_inventory_levels] is false'
+    raise error unless Spree::Config.track_inventory_levels
+
+    self.count_on_hand = new_level
+  end
+
+  # Sets the stock level. As opposed to #on_hand= it does not check
+  # `track_inventory_levels`'s value as it was previously an ActiveModel
+  # setter of the database column of the `spree_variants` table. That is why
+  # #on_hand= is more widely used in Spree's codebase using #count_on_hand=
+  # underneath.
+  #
+  # So, if #count_on_hand= is used, `track_inventory_levels` won't be taken
+  # into account thus dismissing instance's configuration.
+  #
+  # It does ensure there's a stock item for the variant however. See
+  # #raise_error_if_no_stock_item_available for details.
+  #
+  # @raise [StandardError] when the variant has no stock item yet
+  def count_on_hand=(new_level)
+    warn_deprecation(__method__, '#total_on_hand')
+
+    raise_error_if_no_stock_item_available
+    overwrite_stock_levels(new_level)
+  end
+
+  # Checks whether this variant is produced on demand.
+  #
+  # In Spree 2.0 this attribute is removed in favour of
+  # track_inventory_levels only. It was initially introduced in
+  # https://github.com/openfoodfoundation/spree/commit/20b5ad9835dca7f41a40ad16c7b45f987eea6dcc
+  def on_demand
+    warn_deprecation(__method__, 'Spree::Config[:track_inventory_levels]')
+    stock_items.first.backorderable?
+  end
+
+  # Sets whether the variant can be ordered on demand or not. Note that
+  # although this modifies the stock item, it is not persisted in DB. This
+  # may be done to fire a single UPDATE statement when changing various
+  # variant attributes, for performance reasons.
+  #
+  # @raise [StandardError] when the variant has no stock item yet
+  def on_demand=(new_value)
+    warn_deprecation(__method__, 'Spree::Config[:track_inventory_levels]')
+
+    raise_error_if_no_stock_item_available
+
+    # There should be only one at the default stock location.
+    #
+    # This would be better off as `stock_items.first.save` but then, for
+    # unknown reasons, it does not pass the test.
+    stock_items.each do |item|
+      item.backorderable = new_value
+    end
+  end
+
+  private
+
+  # Persists the single stock item associated to this variant. As defined in
+  # the top-most comment, as there's a single stock location in the whole
+  # database, there can only be a stock item per variant. See StockItem's
+  # definition at
+  # https://github.com/openfoodfoundation/spree/blob/43950c3689a77a7f493cc6d805a0edccfe75ebc2/core/app/models/spree/stock_item.rb#L3-L4
+  # for details.
+  def save_stock
+    stock_items.first.save
+  end
+
+  def raise_error_if_no_stock_item_available
+    message = 'You need to save the variant to create a stock item before you can set stock levels.'
+    raise message if stock_items.empty?
+  end
+
+  # Backwards compatible setting of stock levels in Spree 2.0.
+  # It would be better to use `Spree::StockItem.adjust_count_on_hand` which
+  # takes a value to add to the current stock level and uses proper locking.
+  # But this should work the same as in Spree 1.3.
+  def overwrite_stock_levels(new_level)
+    # There shouldn't be any other stock items, because we should
+    # have only one stock location.
+    stock_items.first.__send__(:count_on_hand=, new_level)
+  end
+
+  def warn_deprecation(method_name, new_method_name)
+    ActiveSupport::Deprecation.warn(
+      "`##{method_name}` is deprecated and will be removed. " \
+      "Please use `#{new_method_name}` instead."
+    )
+  end
+end
diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb
index e53e91ca29..41529e3949 100644
--- a/app/models/spree/variant_decorator.rb
+++ b/app/models/spree/variant_decorator.rb
@@ -1,6 +1,6 @@
 require 'open_food_network/enterprise_fee_calculator'
 require 'open_food_network/variant_and_line_item_naming'
-require 'open_food_network/variant_stock'
+require 'concerns/variant_stock'
 require 'open_food_network/products_cache'
 
 Spree::Variant.class_eval do
@@ -10,7 +10,7 @@ Spree::Variant.class_eval do
   # removing the Spree method to prevent error.
   remove_method :options_text if instance_methods(false).include? :options_text
   include OpenFoodNetwork::VariantAndLineItemNaming
-  include OpenFoodNetwork::VariantStock
+  include VariantStock
 
   has_many :exchange_variants
   has_many :exchanges, through: :exchange_variants
diff --git a/lib/open_food_network/variant_stock.rb b/lib/open_food_network/variant_stock.rb
deleted file mode 100644
index 18c9be1a79..0000000000
--- a/lib/open_food_network/variant_stock.rb
+++ /dev/null
@@ -1,145 +0,0 @@
-require 'active_support/concern'
-
-# These methods were available in Spree 1, but were removed in Spree 2.  We
-# would still like to use them so that we still give support to the consumers
-# of this methods, making the upgrade backward compatible.
-#
-# Therefore we use only a single stock item per variant, which is associated to
-# a single stock location per instance (default stock location) and use it to
-# track the `count_on_hand` value that was previously a database column on
-# variants. See
-# https://github.com/openfoodfoundation/openfoodnetwork/wiki/Spree-Upgrade%3A-Stock-locations
-# for details.
-#
-# These methods are or may become deprecated.
-module OpenFoodNetwork
-  module VariantStock
-    extend ActiveSupport::Concern
-
-    included do
-      after_update :save_stock
-    end
-
-    # Returns the number of items of the variant available in the stock. When
-    # allowing on demand, it returns infinite.
-    #
-    # Spree computes it as the sum of the count_on_hand of all its stock_items.
-    #
-    # @return [Float|Integer]
-    def on_hand
-      warn_deprecation(__method__, '#total_on_hand')
-
-      if on_demand
-        Float::INFINITY
-      else
-        total_on_hand
-      end
-    end
-
-    # Returns the number of items available in the stock for this variant
-    #
-    # @return [Float|Integer]
-    def count_on_hand
-      warn_deprecation(__method__, '#total_on_hand')
-      total_on_hand
-    end
-
-    # Sets the stock level when `track_inventory_levels` config is
-    # set. It raises otherwise.
-    #
-    # @raise [StandardError] when the track_inventory_levels config
-    # key is not set.
-    def on_hand=(new_level)
-      warn_deprecation(__method__, '#total_on_hand')
-
-      error = 'Cannot set on_hand value when Spree::Config[:track_inventory_levels] is false'
-      raise error unless Spree::Config.track_inventory_levels
-
-      self.count_on_hand = new_level
-    end
-
-    # Sets the stock level. As opposed to #on_hand= it does not check
-    # `track_inventory_levels`'s value as it was previously an ActiveModel
-    # setter of the database column of the `spree_variants` table. That is why
-    # #on_hand= is more widely used in Spree's codebase using #count_on_hand=
-    # underneath.
-    #
-    # So, if #count_on_hand= is used, `track_inventory_levels` won't be taken
-    # into account thus dismissing instance's configuration.
-    #
-    # It does ensure there's a stock item for the variant however. See
-    # #raise_error_if_no_stock_item_available for details.
-    #
-    # @raise [StandardError] when the variant has no stock item yet
-    def count_on_hand=(new_level)
-      warn_deprecation(__method__, '#total_on_hand')
-
-      raise_error_if_no_stock_item_available
-      overwrite_stock_levels(new_level)
-    end
-
-    # Checks whether this variant is produced on demand.
-    #
-    # In Spree 2.0 this attribute is removed in favour of
-    # track_inventory_levels only. It was initially introduced in
-    # https://github.com/openfoodfoundation/spree/commit/20b5ad9835dca7f41a40ad16c7b45f987eea6dcc
-    def on_demand
-      warn_deprecation(__method__, 'Spree::Config[:track_inventory_levels]')
-      stock_items.first.backorderable?
-    end
-
-    # Sets whether the variant can be ordered on demand or not. Note that
-    # although this modifies the stock item, it is not persisted in DB. This
-    # may be done to fire a single UPDATE statement when changing various
-    # variant attributes, for performance reasons.
-    #
-    # @raise [StandardError] when the variant has no stock item yet
-    def on_demand=(new_value)
-      warn_deprecation(__method__, 'Spree::Config[:track_inventory_levels]')
-
-      raise_error_if_no_stock_item_available
-
-      # There should be only one at the default stock location.
-      #
-      # This would be better off as `stock_items.first.save` but then, for
-      # unknown reasons, it does not pass the test.
-      stock_items.each do |item|
-        item.backorderable = new_value
-      end
-    end
-
-    private
-
-    # Persists the single stock item associated to this variant. As defined in
-    # the top-most comment, as there's a single stock location in the whole
-    # database, there can only be a stock item per variant. See StockItem's
-    # definition at
-    # https://github.com/openfoodfoundation/spree/blob/43950c3689a77a7f493cc6d805a0edccfe75ebc2/core/app/models/spree/stock_item.rb#L3-L4
-    # for details.
-    def save_stock
-      stock_items.first.save
-    end
-
-    def raise_error_if_no_stock_item_available
-      message = 'You need to save the variant to create a stock item before you can set stock levels.'
-      raise message if stock_items.empty?
-    end
-
-    # Backwards compatible setting of stock levels in Spree 2.0.
-    # It would be better to use `Spree::StockItem.adjust_count_on_hand` which
-    # takes a value to add to the current stock level and uses proper locking.
-    # But this should work the same as in Spree 1.3.
-    def overwrite_stock_levels(new_level)
-      # There shouldn't be any other stock items, because we should
-      # have only one stock location.
-      stock_items.first.__send__(:count_on_hand=, new_level)
-    end
-
-    def warn_deprecation(method_name, new_method_name)
-      ActiveSupport::Deprecation.warn(
-        "`##{method_name}` is deprecated and will be removed. " \
-        "Please use `#{new_method_name}` instead."
-      )
-    end
-  end
-end
diff --git a/spec/lib/open_food_network/variant_stock_spec.rb b/spec/models/concerns/variant_stock_spec.rb
similarity index 99%
rename from spec/lib/open_food_network/variant_stock_spec.rb
rename to spec/models/concerns/variant_stock_spec.rb
index b0818f6690..ff95b84526 100644
--- a/spec/lib/open_food_network/variant_stock_spec.rb
+++ b/spec/models/concerns/variant_stock_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe OpenFoodNetwork::VariantStock do
+describe VariantStock do
   let(:variant) { create(:variant) }
 
   describe '#after_save' do

From 0491a96d440895ba9814439fa55615a8c6a89fa5 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Wed, 19 Sep 2018 15:12:38 +0200
Subject: [PATCH 7/7] Ask to use backorderable? instead of on_demand

---
 app/models/concerns/variant_stock.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb
index 2011f85c46..2e1495be1b 100644
--- a/app/models/concerns/variant_stock.rb
+++ b/app/models/concerns/variant_stock.rb
@@ -83,7 +83,7 @@ module VariantStock
   # track_inventory_levels only. It was initially introduced in
   # https://github.com/openfoodfoundation/spree/commit/20b5ad9835dca7f41a40ad16c7b45f987eea6dcc
   def on_demand
-    warn_deprecation(__method__, 'Spree::Config[:track_inventory_levels]')
+    warn_deprecation(__method__, 'StockItem#backorderable?')
     stock_items.first.backorderable?
   end
 
@@ -94,7 +94,7 @@ module VariantStock
   #
   # @raise [StandardError] when the variant has no stock item yet
   def on_demand=(new_value)
-    warn_deprecation(__method__, 'Spree::Config[:track_inventory_levels]')
+    warn_deprecation(__method__, 'StockItem#backorderable=')
 
     raise_error_if_no_stock_item_available