From 8cb75fc6d8e87fad1e57e20574cee41f03680e15 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 7 Aug 2020 19:51:10 +0100
Subject: [PATCH 01/17] Bring models from spree_core: Spree::Product and
 Spree::Variant!

EPIC COMMIT ALERT :-)
---
 app/models/spree/option_type.rb               |  12 +
 app/models/spree/option_value.rb              |   9 +
 app/models/spree/price.rb                     |  48 +++
 app/models/spree/product.rb                   | 250 +++++++++++++
 app/models/spree/product_option_type.rb       |   7 +
 app/models/spree/product_property.rb          |  25 ++
 app/models/spree/variant.rb                   | 189 ++++++++++
 spec/models/spree/product_option_type_spec.rb |   5 +
 spec/models/spree/product_property_spec.rb    |  14 +
 spec/models/spree/product_spec.rb             | 344 ++++++++++++++++++
 spec/models/spree/variant_spec.rb             | 342 +++++++++++++++++
 11 files changed, 1245 insertions(+)
 create mode 100644 app/models/spree/option_type.rb
 create mode 100644 app/models/spree/option_value.rb
 create mode 100644 app/models/spree/price.rb
 create mode 100755 app/models/spree/product.rb
 create mode 100644 app/models/spree/product_option_type.rb
 create mode 100644 app/models/spree/product_property.rb
 create mode 100644 app/models/spree/variant.rb
 create mode 100644 spec/models/spree/product_option_type_spec.rb
 create mode 100644 spec/models/spree/product_property_spec.rb

diff --git a/app/models/spree/option_type.rb b/app/models/spree/option_type.rb
new file mode 100644
index 0000000000..5246243809
--- /dev/null
+++ b/app/models/spree/option_type.rb
@@ -0,0 +1,12 @@
+module Spree
+  class OptionType < ActiveRecord::Base
+    has_many :option_values, -> { order(:position) }, dependent: :destroy
+    has_many :product_option_types, dependent: :destroy
+    has_and_belongs_to_many :prototypes, join_table: 'spree_option_types_prototypes'
+
+    validates :name, :presentation, presence: true
+    default_scope -> { order("#{self.table_name}.position") }
+
+    accepts_nested_attributes_for :option_values, reject_if: lambda { |ov| ov[:name].blank? || ov[:presentation].blank? }, allow_destroy: true
+  end
+end
diff --git a/app/models/spree/option_value.rb b/app/models/spree/option_value.rb
new file mode 100644
index 0000000000..dade69e534
--- /dev/null
+++ b/app/models/spree/option_value.rb
@@ -0,0 +1,9 @@
+module Spree
+  class OptionValue < ActiveRecord::Base
+    belongs_to :option_type
+    acts_as_list scope: :option_type
+    has_and_belongs_to_many :variants, join_table: 'spree_option_values_variants', class_name: "Spree::Variant"
+
+    validates :name, :presentation, presence: true
+  end
+end
diff --git a/app/models/spree/price.rb b/app/models/spree/price.rb
new file mode 100644
index 0000000000..ed85b5675e
--- /dev/null
+++ b/app/models/spree/price.rb
@@ -0,0 +1,48 @@
+module Spree
+  class Price < ActiveRecord::Base
+    belongs_to :variant, class_name: 'Spree::Variant'
+
+    validate :check_price
+    validates :amount, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true
+
+    def display_amount
+      money
+    end
+    alias :display_price :display_amount
+
+    def money
+      Spree::Money.new(amount || 0, { currency: currency })
+    end
+
+    def price
+      amount
+    end
+
+    def price=(price)
+      self[:amount] = parse_price(price)
+    end
+
+    private
+    def check_price
+      raise "Price must belong to a variant" if variant.nil?
+
+      if currency.nil?
+        self.currency = Spree::Config[:currency]
+      end
+    end
+
+    # strips all non-price-like characters from the price, taking into account locale settings
+    def parse_price(price)
+      return price unless price.is_a?(String)
+
+      separator, delimiter = I18n.t([:'number.currency.format.separator', :'number.currency.format.delimiter'])
+      non_price_characters = /[^0-9\-#{separator}]/
+      price.gsub!(non_price_characters, '') # strip everything else first
+      price.gsub!(separator, '.') unless separator == '.' # then replace the locale-specific decimal separator with the standard separator if necessary
+
+      price.to_d
+    end
+
+  end
+end
+
diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
new file mode 100755
index 0000000000..14e01acfa1
--- /dev/null
+++ b/app/models/spree/product.rb
@@ -0,0 +1,250 @@
+# PRODUCTS
+# Products represent an entity for sale in a store.
+# Products can have variations, called variants
+# Products properties include description, permalink, availability,
+#   shipping category, etc. that do not change by variant.
+#
+# MASTER VARIANT
+# Every product has one master variant, which stores master price and sku, size and weight, etc.
+# The master variant does not have option values associated with it.
+# Price, SKU, size, weight, etc. are all delegated to the master variant.
+# Contains on_hand inventory levels only when there are no variants for the product.
+#
+# VARIANTS
+# All variants can access the product properties directly (via reverse delegation).
+# Inventory units are tied to Variant.
+# The master variant can have inventory units, but not option values.
+# All other variants have option values and may have inventory units.
+# Sum of on_hand each variant's inventory level determine "on_hand" level for the product.
+#
+
+module Spree
+  class Product < ActiveRecord::Base
+    acts_as_paranoid
+    has_many :product_option_types, dependent: :destroy
+    has_many :option_types, through: :product_option_types
+    has_many :product_properties, dependent: :destroy
+    has_many :properties, through: :product_properties
+
+    has_many :classifications, dependent: :delete_all
+    has_many :taxons, through: :classifications
+    has_and_belongs_to_many :promotion_rules, join_table: :spree_products_promotion_rules
+
+    belongs_to :tax_category, class_name: 'Spree::TaxCategory'
+    belongs_to :shipping_category, class_name: 'Spree::ShippingCategory'
+
+    has_one :master,
+      -> { where is_master: true },
+      class_name: 'Spree::Variant',
+      dependent: :destroy
+
+    has_many :variants,
+      -> { where(is_master: false).order("#{::Spree::Variant.quoted_table_name}.position ASC") },
+      class_name: 'Spree::Variant'
+
+    has_many :variants_including_master,
+      -> { order("#{::Spree::Variant.quoted_table_name}.position ASC") },
+      class_name: 'Spree::Variant',
+      dependent: :destroy
+
+    has_many :prices, -> { order('spree_variants.position, spree_variants.id, currency') }, through: :variants
+
+    has_many :stock_items, through: :variants
+
+    delegate_belongs_to :master, :sku, :price, :currency, :display_amount, :display_price, :weight, :height, :width, :depth, :is_master, :has_default_price?, :cost_currency, :price_in, :amount_in
+    delegate_belongs_to :master, :cost_price if Variant.table_exists? && Variant.column_names.include?('cost_price')
+
+    after_create :set_master_variant_defaults
+    after_create :add_properties_and_option_types_from_prototype
+    after_create :build_variants_from_option_values_hash, if: :option_values_hash
+    after_save :save_master
+
+    delegate :images, to: :master, prefix: true
+    alias_method :images, :master_images
+
+    has_many :variant_images, -> { order(:position) }, source: :images, through: :variants_including_master
+
+    accepts_nested_attributes_for :variants, allow_destroy: true
+
+    validates :name, presence: true
+    validates :permalink, presence: true
+    validates :price, presence: true, if: proc { Spree::Config[:require_master_price] }
+    validates :shipping_category_id, presence: true
+
+    attr_accessor :option_values_hash
+
+    accepts_nested_attributes_for :product_properties, allow_destroy: true, reject_if: lambda { |pp| pp[:property_name].blank? }
+
+    make_permalink order: :name
+
+    alias :options :product_option_types
+
+    after_initialize :ensure_master
+
+    before_destroy :punch_permalink
+
+    def to_param
+      permalink.present? ? permalink : (permalink_was || name.to_s.to_url)
+    end
+
+    # the master variant is not a member of the variants array
+    def has_variants?
+      variants.any?
+    end
+
+    def tax_category
+      if self[:tax_category_id].nil?
+        TaxCategory.where(is_default: true).first
+      else
+        TaxCategory.find(self[:tax_category_id])
+      end
+    end
+
+    # Adding properties and option types on creation based on a chosen prototype
+    attr_reader :prototype_id
+    def prototype_id=(value)
+      @prototype_id = value.to_i
+    end
+
+    # Ensures option_types and product_option_types exist for keys in option_values_hash
+    def ensure_option_types_exist_for_values_hash
+      return if option_values_hash.nil?
+      option_values_hash.keys.map(&:to_i).each do |id|
+        self.option_type_ids << id unless option_type_ids.include?(id)
+        product_option_types.create(option_type_id: id) unless product_option_types.pluck(:option_type_id).include?(id)
+      end
+    end
+
+    # for adding products which are closely related to existing ones
+    # define "duplicate_extra" for site-specific actions, eg for additional fields
+    def duplicate
+      duplicator = ProductDuplicator.new(self)
+      duplicator.duplicate
+    end
+
+    # use deleted? rather than checking the attribute directly. this
+    # allows extensions to override deleted? if they want to provide
+    # their own definition.
+    def deleted?
+      !!deleted_at
+    end
+
+    def available?
+      !(available_on.nil? || available_on.future?)
+    end
+
+    # split variants list into hash which shows mapping of opt value onto matching variants
+    # eg categorise_variants_from_option(color) => {"red" -> [...], "blue" -> [...]}
+    def categorise_variants_from_option(opt_type)
+      return {} unless option_types.include?(opt_type)
+      variants.active.group_by { |v| v.option_values.detect { |o| o.option_type == opt_type} }
+    end
+
+    def self.like_any(fields, values)
+      where fields.map { |field|
+        values.map { |value|
+          arel_table[field].matches("%#{value}%")
+        }.inject(:or)
+      }.inject(:or)
+    end
+
+    # Suitable for displaying only variants that has at least one option value.
+    # There may be scenarios where an option type is removed and along with it
+    # all option values. At that point all variants associated with only those
+    # values should not be displayed to frontend users. Otherwise it breaks the
+    # idea of having variants
+    def variants_and_option_values(current_currency = nil)
+      variants.includes(:option_values).active(current_currency).select do |variant|
+        variant.option_values.any?
+      end
+    end
+
+    def empty_option_values?
+      options.empty? || options.any? do |opt|
+        opt.option_type.option_values.empty?
+      end
+    end
+
+    def property(property_name)
+      return nil unless prop = properties.find_by(name: property_name)
+      product_properties.find_by(property: prop).try(:value)
+    end
+
+    def set_property(property_name, property_value)
+      ActiveRecord::Base.transaction do
+        property = Property.where(name: property_name).first_or_create!(presentation: property_name)
+        product_property = ProductProperty.where(product: self, property: property).first_or_initialize
+        product_property.value = property_value
+        product_property.save!
+      end
+    end
+
+    def possible_promotions
+      promotion_ids = promotion_rules.map(&:activator_id).uniq
+      Spree::Promotion.advertised.where(id: promotion_ids).reject(&:expired?)
+    end
+
+    def total_on_hand
+      if Spree::Config.track_inventory_levels
+        self.stock_items.sum(&:count_on_hand)
+      else
+        Float::INFINITY
+      end
+    end
+
+    # Master variant may be deleted (i.e. when the product is deleted)
+    # which would make AR's default finder return nil.
+    # This is a stopgap for that little problem.
+    def master
+      super || variants_including_master.with_deleted.where(is_master: true).first
+    end
+
+    private
+
+      # Builds variants from a hash of option types & values
+      def build_variants_from_option_values_hash
+        ensure_option_types_exist_for_values_hash
+        values = option_values_hash.values
+        values = values.inject(values.shift) { |memo, value| memo.product(value).map(&:flatten) }
+
+        values.each do |ids|
+          variant = variants.create(
+            option_value_ids: ids,
+            price: master.price
+          )
+        end
+        save
+      end
+
+      def add_properties_and_option_types_from_prototype
+        if prototype_id && prototype = Spree::Prototype.find_by(id: prototype_id)
+          prototype.properties.each do |property|
+            product_properties.create(property: property)
+          end
+          self.option_types = prototype.option_types
+        end
+      end
+
+      # ensures the master variant is flagged as such
+      def set_master_variant_defaults
+        master.is_master = true
+      end
+
+      # there's a weird quirk with the delegate stuff that does not automatically save the delegate object
+      # when saving so we force a save using a hook.
+      def save_master
+        master.save if master && (master.changed? || master.new_record? || (master.default_price && (master.default_price.changed || master.default_price.new_record)))
+      end
+
+      def ensure_master
+        return unless new_record?
+        self.master ||= Variant.new
+      end
+
+      def punch_permalink
+        update_attribute :permalink, "#{Time.now.to_i}_#{permalink}" # punch permalink with date prefix
+      end
+  end
+end
+
+require_dependency 'spree/product/scopes'
diff --git a/app/models/spree/product_option_type.rb b/app/models/spree/product_option_type.rb
new file mode 100644
index 0000000000..2a63434c48
--- /dev/null
+++ b/app/models/spree/product_option_type.rb
@@ -0,0 +1,7 @@
+module Spree
+  class ProductOptionType < ActiveRecord::Base
+    belongs_to :product, class_name: 'Spree::Product'
+    belongs_to :option_type, class_name: 'Spree::OptionType'
+    acts_as_list scope: :product
+  end
+end
diff --git a/app/models/spree/product_property.rb b/app/models/spree/product_property.rb
new file mode 100644
index 0000000000..aa947d878d
--- /dev/null
+++ b/app/models/spree/product_property.rb
@@ -0,0 +1,25 @@
+module Spree
+  class ProductProperty < ActiveRecord::Base
+    belongs_to :product,  class_name: 'Spree::Product'
+    belongs_to :property, class_name: 'Spree::Property'
+
+    validates :property, presence: true
+    validates :value, length: { maximum: 255 }
+
+    default_scope -> { order("#{self.table_name}.position") }
+
+    # virtual attributes for use with AJAX completion stuff
+    def property_name
+      property.name if property
+    end
+
+    def property_name=(name)
+      unless name.blank?
+        unless property = Property.find_by(name: name)
+          property = Property.create(name: name, presentation: name)
+        end
+        self.property = property
+      end
+    end
+  end
+end
diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
new file mode 100644
index 0000000000..9b9fb1f329
--- /dev/null
+++ b/app/models/spree/variant.rb
@@ -0,0 +1,189 @@
+module Spree
+  class Variant < ActiveRecord::Base
+    acts_as_paranoid
+
+    belongs_to :product, touch: true, class_name: 'Spree::Product'
+
+    delegate_belongs_to :product, :name, :description, :permalink, :available_on,
+                        :tax_category_id, :shipping_category_id, :meta_description,
+                        :meta_keywords, :tax_category, :shipping_category
+
+    has_many :inventory_units
+    has_many :line_items
+
+    has_many :stock_items, dependent: :destroy
+    has_many :stock_locations, through: :stock_items
+    has_many :stock_movements
+
+    has_and_belongs_to_many :option_values, join_table: :spree_option_values_variants
+    has_many :images, -> { order(:position) }, as: :viewable, dependent: :destroy, class_name: "Spree::Image"
+
+    has_one :default_price,
+      -> { where currency: Spree::Config[:currency] },
+      class_name: 'Spree::Price',
+      dependent: :destroy
+
+    delegate_belongs_to :default_price, :display_price, :display_amount, :price, :price=, :currency if Spree::Price.table_exists?
+
+    has_many :prices,
+      class_name: 'Spree::Price',
+      dependent: :destroy
+
+    validate :check_price
+    validates :price, numericality: { greater_than_or_equal_to: 0 }, presence: true, if: proc { Spree::Config[:require_master_price] }
+    validates :cost_price, numericality: { greater_than_or_equal_to: 0, allow_nil: true } if self.table_exists? && self.column_names.include?('cost_price')
+
+    before_validation :set_cost_currency
+    after_save :save_default_price
+    after_create :create_stock_items
+    after_create :set_position
+
+    # default variant scope only lists non-deleted variants
+    scope :deleted, lambda { where('deleted_at IS NOT NULL') }
+
+    def self.active(currency = nil)
+      joins(:prices).where(deleted_at: nil).where('spree_prices.currency' => currency || Spree::Config[:currency]).where('spree_prices.amount IS NOT NULL')
+    end
+
+    def cost_price=(price)
+      self[:cost_price] = parse_price(price) if price.present?
+    end
+
+    # returns number of units currently on backorder for this variant.
+    def on_backorder
+      inventory_units.with_state('backordered').size
+    end
+
+    def options_text
+      values = self.option_values.joins(:option_type).order("#{Spree::OptionType.table_name}.position asc")
+
+      values.map! do |ov|
+        "#{ov.option_type.presentation}: #{ov.presentation}"
+      end
+
+      values.to_sentence({ words_connector: ", ", two_words_connector: ", " })
+    end
+
+    def gross_profit
+      cost_price.nil? ? 0 : (price - cost_price)
+    end
+
+    # use deleted? rather than checking the attribute directly. this
+    # allows extensions to override deleted? if they want to provide
+    # their own definition.
+    def deleted?
+      deleted_at
+    end
+
+    def set_option_value(opt_name, opt_value)
+      # no option values on master
+      return if self.is_master
+
+      option_type = Spree::OptionType.where(name: opt_name).first_or_initialize do |o|
+        o.presentation = opt_name
+        o.save!
+      end
+
+      current_value = self.option_values.detect { |o| o.option_type.name == opt_name }
+
+      unless current_value.nil?
+        return if current_value.name == opt_value
+        self.option_values.delete(current_value)
+      else
+        # then we have to check to make sure that the product has the option type
+        unless self.product.option_types.include? option_type
+          self.product.option_types << option_type
+          self.product.save
+        end
+      end
+
+      option_value = Spree::OptionValue.where(option_type_id: option_type.id, name: opt_value).first_or_initialize do |o|
+        o.presentation = opt_value
+        o.save!
+      end
+
+      self.option_values << option_value
+      self.save
+    end
+
+    def option_value(opt_name)
+      self.option_values.detect { |o| o.option_type.name == opt_name }.try(:presentation)
+    end
+
+    def has_default_price?
+      !self.default_price.nil?
+    end
+
+    def price_in(currency)
+      prices.select{ |price| price.currency == currency }.first || Spree::Price.new(variant_id: self.id, currency: currency)
+    end
+
+    def amount_in(currency)
+      price_in(currency).try(:amount)
+    end
+
+    def name_and_sku
+      "#{name} - #{sku}"
+    end
+
+    # Product may be created with deleted_at already set,
+    # which would make AR's default finder return nil.
+    # This is a stopgap for that little problem.
+    def product
+      Spree::Product.unscoped { super }
+    end
+    
+    def in_stock?(quantity=1)
+      Spree::Stock::Quantifier.new(self).can_supply?(quantity)
+    end
+
+    def total_on_hand
+      Spree::Stock::Quantifier.new(self).total_on_hand
+    end
+
+    private
+      # strips all non-price-like characters from the price, taking into account locale settings
+      def parse_price(price)
+        return price unless price.is_a?(String)
+
+        separator, delimiter = I18n.t([:'number.currency.format.separator', :'number.currency.format.delimiter'])
+        non_price_characters = /[^0-9\-#{separator}]/
+        price.gsub!(non_price_characters, '') # strip everything else first
+        price.gsub!(separator, '.') unless separator == '.' # then replace the locale-specific decimal separator with the standard separator if necessary
+
+        price.to_d
+      end
+
+      # Ensures a new variant takes the product master price when price is not supplied
+      def check_price
+        if price.nil? && Spree::Config[:require_master_price]
+          raise 'No master variant found to infer price' unless (product && product.master)
+          raise 'Must supply price for variant or master.price for product.' if self == product.master
+          self.price = product.master.price
+        end
+        if currency.nil?
+          self.currency = Spree::Config[:currency]
+        end
+      end
+
+      def save_default_price
+        default_price.save if default_price && (default_price.changed? || default_price.new_record?)
+      end
+
+      def set_cost_currency
+        self.cost_currency = Spree::Config[:currency] if cost_currency.nil? || cost_currency.empty?
+      end
+
+      def create_stock_items
+        StockLocation.all.each do |stock_location|
+          stock_location.propagate_variant(self) if stock_location.propagate_all_variants?
+        end
+      end
+
+      def set_position
+        self.update_column(:position, product.variants.maximum(:position).to_i + 1)
+      end
+  end
+end
+
+require_dependency 'spree/variant/scopes'
diff --git a/spec/models/spree/product_option_type_spec.rb b/spec/models/spree/product_option_type_spec.rb
new file mode 100644
index 0000000000..8bf1870013
--- /dev/null
+++ b/spec/models/spree/product_option_type_spec.rb
@@ -0,0 +1,5 @@
+require 'spec_helper'
+
+describe Spree::ProductOptionType do
+
+end
diff --git a/spec/models/spree/product_property_spec.rb b/spec/models/spree/product_property_spec.rb
new file mode 100644
index 0000000000..e979efb769
--- /dev/null
+++ b/spec/models/spree/product_property_spec.rb
@@ -0,0 +1,14 @@
+require 'spec_helper'
+
+describe Spree::ProductProperty do
+
+  context "validations" do
+    it "should validate length of value" do
+      pp = create(:product_property)
+      pp.value = "x" * 256
+      pp.should_not be_valid
+    end
+
+  end
+
+end
diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb
index 6bae30417c..e5c0aa072c 100644
--- a/spec/models/spree/product_spec.rb
+++ b/spec/models/spree/product_spec.rb
@@ -2,6 +2,350 @@ require 'spec_helper'
 
 module Spree
   describe Product do
+    context 'product instance' do
+      let(:product) { create(:product) }
+
+      context '#duplicate' do
+        before do
+          product.stub :taxons => [create(:taxon)]
+        end
+
+        it 'duplicates product' do
+          clone = product.duplicate
+          clone.name.should == 'COPY OF ' + product.name
+          clone.master.sku.should == 'COPY OF ' + product.master.sku
+          clone.taxons.should == product.taxons
+          clone.images.size.should == product.images.size
+        end
+
+        it 'calls #duplicate_extra' do
+          Spree::Product.class_eval do
+            def duplicate_extra(old_product)
+              self.name = old_product.name.reverse
+            end
+          end
+
+          clone = product.duplicate
+          clone.name.should == product.name.reverse
+        end
+      end
+
+      context "product has no variants" do
+        context "#destroy" do
+          it "should set deleted_at value" do
+            product.destroy
+            product.deleted_at.should_not be_nil
+            product.master.deleted_at.should_not be_nil
+          end
+        end
+      end
+
+      context "product has variants" do
+        before do
+          create(:variant, :product => product)
+        end
+
+        context "#destroy" do
+          it "should set deleted_at value" do
+            product.destroy
+            product.deleted_at.should_not be_nil
+            product.variants_including_master.all? { |v| !v.deleted_at.nil? }.should be_true
+          end
+        end
+      end
+
+      context "#price" do
+        # Regression test for Spree #1173
+        it 'strips non-price characters' do
+          product.price = "$10"
+          product.price.should == 10.0
+        end
+      end
+
+      context "#display_price" do
+        before { product.price = 10.55 }
+
+        context "with display_currency set to true" do
+          before { Spree::Config[:display_currency] = true }
+
+          it "shows the currency" do
+            product.display_price.to_s.should == "$10.55 USD"
+          end
+        end
+
+        context "with display_currency set to false" do
+          before { Spree::Config[:display_currency] = false }
+
+          it "does not include the currency" do
+            product.display_price.to_s.should == "$10.55"
+          end
+        end
+
+        context "with currency set to JPY" do
+          before do
+            product.master.default_price.currency = 'JPY'
+            product.master.default_price.save!
+            Spree::Config[:currency] = 'JPY'
+          end
+
+          it "displays the currency in yen" do
+            product.display_price.to_s.should == "¥11"
+          end
+        end
+      end
+
+      context "#available?" do
+        it "should be available if date is in the past" do
+          product.available_on = 1.day.ago
+          product.should be_available
+        end
+
+        it "should not be available if date is nil or in the future" do
+          product.available_on = nil
+          product.should_not be_available
+
+          product.available_on = 1.day.from_now
+          product.should_not be_available
+        end
+      end
+
+      context "variants_and_option_values" do
+        let!(:high) { create(:variant, product: product) }
+        let!(:low) { create(:variant, product: product) }
+
+        before { high.option_values.destroy_all }
+
+        it "returns only variants with option values" do
+          product.variants_and_option_values.should == [low]
+        end
+      end
+
+      describe 'Variants sorting' do
+        context 'without master variant' do
+          it 'sorts variants by position' do
+            product.variants.to_sql.should match(/ORDER BY (\`|\")spree_variants(\`|\").position ASC/)
+          end
+        end
+
+        context 'with master variant' do
+          it 'sorts variants by position' do
+            product.variants_including_master.to_sql.should match(/ORDER BY (\`|\")spree_variants(\`|\").position ASC/)
+          end
+        end
+      end
+
+      context "has stock movements" do
+        let(:product) { create(:product) }
+        let(:variant) { product.master }
+        let(:stock_item) { variant.stock_items.first }
+
+        it "doesnt raise ReadOnlyRecord error" do
+          Spree::StockMovement.create!(stock_item: stock_item, quantity: 1)
+          expect { product.destroy }.not_to raise_error
+        end
+      end
+    end
+
+    context "permalink" do
+      context "build product with similar name" do
+        let!(:other) { create(:product, :name => 'foo bar') }
+        let(:product) { build(:product, :name => 'foo') }
+
+        before { product.valid? }
+
+        it "increments name" do
+          product.permalink.should == 'foo-1'
+        end
+      end
+
+      context "build permalink with quotes" do
+        it "saves quotes" do
+          product = create(:product, :name => "Joe's", :permalink => "joe's")
+          product.permalink.should == "joe's"
+        end
+      end
+
+      context "permalinks must be unique" do
+        before do
+          @product1 = create(:product, :name => 'foo')
+        end
+
+        it "cannot create another product with the same permalink" do
+          pending '[Spree build] Failing spec'        
+          @product2 = create(:product, :name => 'foo')
+          lambda do
+            @product2.update_attributes(:permalink => @product1.permalink)
+          end.should raise_error(ActiveRecord::RecordNotUnique)
+        end
+      end
+
+      it "supports Chinese" do
+        create(:product, :name => "你好").permalink.should == "ni-hao"
+      end
+
+      context "manual permalink override" do
+        let(:product) { create(:product, :name => "foo") }
+
+        it "calling save_permalink with a parameter" do
+          product.name = "foobar"
+          product.save
+          product.permalink.should == "foo"
+
+          product.save_permalink(product.name)
+          product.permalink.should == "foobar"
+        end
+      end
+
+      context "override permalink of deleted product" do 
+        let(:product) { create(:product, :name => "foo") } 
+
+        it "should create product with same permalink from name like deleted product" do 
+          product.permalink.should == "foo" 
+          product.destroy 
+          
+          new_product = create(:product, :name => "foo") 
+          new_product.permalink.should == "foo" 
+        end 
+      end 
+    end
+
+    context "properties" do
+      let(:product) { create(:product) }
+
+      it "should properly assign properties" do
+        product.set_property('the_prop', 'value1')
+        product.property('the_prop').should == 'value1'
+
+        product.set_property('the_prop', 'value2')
+        product.property('the_prop').should == 'value2'
+      end
+
+      it "should not create duplicate properties when set_property is called" do
+        expect {
+          product.set_property('the_prop', 'value2')
+          product.save
+          product.reload
+        }.not_to change(product.properties, :length)
+
+        expect {
+          product.set_property('the_prop_new', 'value')
+          product.save
+          product.reload
+          product.property('the_prop_new').should == 'value'
+        }.to change { product.properties.length }.by(1)
+      end
+
+      # Regression test for #2455
+      it "should not overwrite properties' presentation names" do
+        Spree::Property.where(:name => 'foo').first_or_create!(:presentation => "Foo's Presentation Name")
+        product.set_property('foo', 'value1')
+        product.set_property('bar', 'value2')
+        Spree::Property.where(:name => 'foo').first.presentation.should == "Foo's Presentation Name"
+        Spree::Property.where(:name => 'bar').first.presentation.should == "bar"
+      end
+    end
+
+    context '#create' do
+      let!(:prototype) { create(:prototype) }
+      let!(:product) { Spree::Product.new(name: "Foo", price: 1.99, shipping_category_id: create(:shipping_category).id) }
+
+      before { product.prototype_id = prototype.id }
+
+      context "when prototype is supplied" do
+        it "should create properties based on the prototype" do
+          product.save
+          product.properties.count.should == 1
+        end
+      end
+
+      context "when prototype with option types is supplied" do
+        def build_option_type_with_values(name, values)
+          ot = create(:option_type, :name => name)
+          values.each do |val|
+            ot.option_values.create(:name => val.downcase, :presentation => val)
+          end
+          ot
+        end
+
+        let(:prototype) do
+          size = build_option_type_with_values("size", %w(Small Medium Large))
+          create(:prototype, :name => "Size", :option_types => [ size ])
+        end
+
+        let(:option_values_hash) do
+          hash = {}
+          prototype.option_types.each do |i|
+            hash[i.id.to_s] = i.option_value_ids
+          end
+          hash
+        end
+
+        it "should create option types based on the prototype" do
+          product.save
+          product.option_type_ids.length.should == 1
+          product.option_type_ids.should == prototype.option_type_ids
+        end
+
+        it "should create product option types based on the prototype" do
+          product.save
+          product.product_option_types.pluck(:option_type_id).should == prototype.option_type_ids
+        end
+
+        it "should create variants from an option values hash with one option type" do
+          product.option_values_hash = option_values_hash
+          product.save
+          product.variants.length.should == 3
+        end
+
+        it "should still create variants when option_values_hash is given but prototype id is nil" do
+          product.option_values_hash = option_values_hash
+          product.prototype_id = nil
+          product.save
+          product.option_type_ids.length.should == 1
+          product.option_type_ids.should == prototype.option_type_ids
+          product.variants.length.should == 3
+        end
+
+        it "should create variants from an option values hash with multiple option types" do
+          color = build_option_type_with_values("color", %w(Red Green Blue))
+          logo  = build_option_type_with_values("logo", %w(Ruby Rails Nginx))
+          option_values_hash[color.id.to_s] = color.option_value_ids
+          option_values_hash[logo.id.to_s] = logo.option_value_ids
+          product.option_values_hash = option_values_hash
+          product.save
+          product.reload
+          product.option_type_ids.length.should == 3
+          product.variants.length.should == 27
+        end
+      end
+    end
+
+    # Regression tests for Spree #2352
+    context "classifications and taxons" do
+      it "is joined through classifications" do
+        reflection = Spree::Product.reflect_on_association(:taxons)
+        reflection.options[:through] = :classifications
+      end
+
+      it "will delete all classifications" do
+        reflection = Spree::Product.reflect_on_association(:classifications)
+        reflection.options[:dependent] = :delete_all
+      end
+    end
+
+    describe '#total_on_hand' do
+      it 'should be infinite if track_inventory_levels is false' do
+        Spree::Config[:track_inventory_levels] = false
+        build(:product).total_on_hand.should eql(Float::INFINITY)
+      end
+
+      it 'should return sum of stock items count_on_hand' do
+        product = build(:product)
+        product.stub stock_items: [double(Spree::StockItem, count_on_hand: 5)]
+        product.total_on_hand.should eql(5)
+      end
+    end
+
     describe "associations" do
       it { is_expected.to belong_to(:supplier) }
       it { is_expected.to belong_to(:primary_taxon) }
diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb
index 3b68572281..6db5047062 100644
--- a/spec/models/spree/variant_spec.rb
+++ b/spec/models/spree/variant_spec.rb
@@ -3,6 +3,348 @@ require 'variant_units/option_value_namer'
 
 module Spree
   describe Variant do
+    let!(:variant) { create(:variant) }
+
+    context "validations" do
+      it "should validate price is greater than 0" do
+        variant.price = -1
+        variant.should be_invalid
+      end
+
+      it "should validate price is 0" do
+        variant.price = 0
+        variant.should be_valid
+      end
+    end
+
+    context "after create" do
+      let!(:product) { create(:product) }
+
+      it "propagate to stock items" do
+        Spree::StockLocation.any_instance.should_receive(:propagate_variant)
+        product.variants.create(:name => "Foobar")
+      end
+
+      context "stock location has disable propagate all variants" do
+        before { Spree::StockLocation.any_instance.stub(propagate_all_variants?: false) }
+
+        it "propagate to stock items" do
+          Spree::StockLocation.any_instance.should_not_receive(:propagate_variant)
+          product.variants.create(:name => "Foobar")
+        end
+      end
+    end
+
+    context "product has other variants" do
+      describe "option value accessors" do
+        before {
+          @multi_variant = FactoryGirl.create :variant, :product => variant.product
+          variant.product.reload
+        }
+
+        let(:multi_variant) { @multi_variant }
+
+        it "should set option value" do
+          multi_variant.option_value('media_type').should be_nil
+
+          multi_variant.set_option_value('media_type', 'DVD')
+          multi_variant.option_value('media_type').should == 'DVD'
+
+          multi_variant.set_option_value('media_type', 'CD')
+          multi_variant.option_value('media_type').should == 'CD'
+        end
+
+        it "should not duplicate associated option values when set multiple times" do
+          multi_variant.set_option_value('media_type', 'CD')
+
+          expect {
+           multi_variant.set_option_value('media_type', 'DVD')
+          }.to_not change(multi_variant.option_values, :count)
+
+          expect {
+            multi_variant.set_option_value('coolness_type', 'awesome')
+          }.to change(multi_variant.option_values, :count).by(1)
+        end
+      end
+
+      context "product has other variants" do
+        describe "option value accessors" do
+          before {
+            @multi_variant = create(:variant, :product => variant.product)
+            variant.product.reload
+          }
+
+          let(:multi_variant) { @multi_variant }
+
+          it "should set option value" do
+            multi_variant.option_value('media_type').should be_nil
+
+            multi_variant.set_option_value('media_type', 'DVD')
+            multi_variant.option_value('media_type').should == 'DVD'
+
+            multi_variant.set_option_value('media_type', 'CD')
+            multi_variant.option_value('media_type').should == 'CD'
+          end
+
+          it "should not duplicate associated option values when set multiple times" do
+            multi_variant.set_option_value('media_type', 'CD')
+
+            expect {
+             multi_variant.set_option_value('media_type', 'DVD')
+            }.to_not change(multi_variant.option_values, :count)
+
+            expect {
+              multi_variant.set_option_value('coolness_type', 'awesome')
+            }.to change(multi_variant.option_values, :count).by(1)
+          end
+        end
+      end
+    end
+
+    context "price parsing" do
+      before(:each) do
+        I18n.locale = I18n.default_locale
+        I18n.backend.store_translations(:de, { :number => { :currency => { :format => { :delimiter => '.', :separator => ',' } } } })
+      end
+
+      after do
+        I18n.locale = I18n.default_locale
+      end
+
+      context "price=" do
+        context "with decimal point" do
+          it "captures the proper amount for a formatted price" do
+            variant.price = '1,599.99'
+            variant.price.should == 1599.99
+          end
+        end
+
+        context "with decimal comma" do
+          it "captures the proper amount for a formatted price" do
+            I18n.locale = :de
+            variant.price = '1.599,99'
+            variant.price.should == 1599.99
+          end
+        end
+
+        context "with a numeric price" do
+          it "uses the price as is" do
+            I18n.locale = :de
+            variant.price = 1599.99
+            variant.price.should == 1599.99
+          end
+        end
+      end
+
+      context "cost_price=" do
+        context "with decimal point" do
+          it "captures the proper amount for a formatted price" do
+            variant.cost_price = '1,599.99'
+            variant.cost_price.should == 1599.99
+          end
+        end
+
+        context "with decimal comma" do
+          it "captures the proper amount for a formatted price" do
+            I18n.locale = :de
+            variant.cost_price = '1.599,99'
+            variant.cost_price.should == 1599.99
+          end
+        end
+
+        context "with a numeric price" do
+          it "uses the price as is" do
+            I18n.locale = :de
+            variant.cost_price = 1599.99
+            variant.cost_price.should == 1599.99
+          end
+        end
+      end
+    end
+
+    context "#currency" do
+      it "returns the globally configured currency" do
+        variant.currency.should == "USD"
+      end
+    end
+
+    context "#display_amount" do
+      it "returns a Spree::Money" do
+        variant.price = 21.22
+        variant.display_amount.to_s.should == "$21.22"
+      end
+    end
+
+    context "#cost_currency" do
+      context "when cost currency is nil" do
+        before { variant.cost_currency = nil }
+        it "populates cost currency with the default value on save" do
+          variant.save!
+          variant.cost_currency.should == "USD"
+        end
+      end
+    end
+
+    describe '.price_in' do
+      before do
+        variant.prices << create(:price, :variant => variant, :currency => "EUR", :amount => 33.33)
+      end
+      subject { variant.price_in(currency).display_amount }
+
+      context "when currency is not specified" do
+        let(:currency) { nil }
+
+        it "returns 0" do
+          subject.to_s.should == "$0.00"
+        end
+      end
+
+      context "when currency is EUR" do
+        let(:currency) { 'EUR' }
+
+        it "returns the value in the EUR" do
+          subject.to_s.should == "€33.33"
+        end
+      end
+
+      context "when currency is USD" do
+        let(:currency) { 'USD' }
+
+        it "returns the value in the USD" do
+          subject.to_s.should == "$19.99"
+        end
+      end
+    end
+
+    describe '.amount_in' do
+      before do
+        variant.prices << create(:price, :variant => variant, :currency => "EUR", :amount => 33.33)
+      end
+
+      subject { variant.amount_in(currency) }
+
+      context "when currency is not specified" do
+        let(:currency) { nil }
+
+        it "returns nil" do
+          subject.should be_nil
+        end
+      end
+
+      context "when currency is EUR" do
+        let(:currency) { 'EUR' }
+
+        it "returns the value in the EUR" do
+          subject.should == 33.33
+        end
+      end
+
+      context "when currency is USD" do
+        let(:currency) { 'USD' }
+
+        it "returns the value in the USD" do
+          subject.should == 19.99
+        end
+      end
+    end
+
+    # Regression test for #2432
+    describe 'options_text' do
+      before do
+        option_type = double("OptionType", :presentation => "Foo")
+        option_values = [double("OptionValue", :option_type => option_type, :presentation => "bar")]
+        variant.stub(:option_values).and_return(option_values)
+      end
+
+      it "orders options correctly" do
+        variant.option_values.should_receive(:joins).with(:option_type).and_return(scope = double)
+        scope.should_receive(:order).with('spree_option_types.position asc').and_return(variant.option_values)
+        variant.options_text
+      end
+    end
+
+    # Regression test for #2744
+    describe "set_position" do
+      it "sets variant position after creation" do
+        variant = create(:variant)
+        variant.position.should_not be_nil
+      end
+    end
+
+    describe '#in_stock?' do
+      before do
+        Spree::Config.track_inventory_levels = true
+      end
+
+      context 'when stock_items are not backorderable' do
+        before do
+          Spree::StockItem.any_instance.stub(backorderable: false)
+        end
+
+        context 'when stock_items in stock' do
+          before do
+            Spree::StockItem.any_instance.stub(count_on_hand: 10)
+          end
+
+          it 'returns true if stock_items in stock' do
+            variant.in_stock?.should be_true
+          end
+        end
+
+        context 'when stock_items out of stock' do
+          before do
+            Spree::StockItem.any_instance.stub(backorderable: false)
+            Spree::StockItem.any_instance.stub(count_on_hand: 0)
+          end
+
+          it 'return false if stock_items out of stock' do
+            variant.in_stock?.should be_false
+          end
+        end
+
+        context 'when providing quantity param' do
+          before do
+            variant.stock_items.first.update_attribute(:count_on_hand, 10)
+          end
+
+          it 'returns correctt value' do
+            variant.in_stock?.should be_true
+            variant.in_stock?(2).should be_true
+            variant.in_stock?(10).should be_true
+            variant.in_stock?(11).should be_false
+          end
+        end
+      end
+
+      context 'when stock_items are backorderable' do
+        before do
+          Spree::StockItem.any_instance.stub(backorderable: true)
+        end
+
+        context 'when stock_items out of stock' do
+          before do
+            Spree::StockItem.any_instance.stub(count_on_hand: 0)
+          end
+
+          it 'returns true if stock_items in stock' do
+            variant.in_stock?.should be_true
+          end
+        end
+      end
+    end
+
+    describe '#total_on_hand' do
+      it 'should be infinite if track_inventory_levels is false' do
+        Spree::Config[:track_inventory_levels] = false
+        build(:variant).total_on_hand.should eql(Float::INFINITY)
+      end
+
+      it 'should match quantifier total_on_hand' do
+        variant = build(:variant)
+        expect(variant.total_on_hand).to eq(Spree::Stock::Quantifier.new(variant).total_on_hand)
+      end
+    end
+    
     describe "double loading" do
       # app/models/spree/variant_decorator.rb  may be double-loaded in delayed job environment,
       # so we need to be able to do so without error.

From 751beceb34d2d6a2873cb485aa1ddb7654b06fc8 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 7 Aug 2020 20:14:03 +0100
Subject: [PATCH 02/17] Merge decorators with original spree files

---
 app/models/spree/option_type.rb               |   1 +
 app/models/spree/option_type_decorator.rb     |   5 -
 app/models/spree/price.rb                     |  12 +-
 app/models/spree/price_decorator.rb           |  18 --
 app/models/spree/product.rb                   | 260 ++++++++++++++++-
 app/models/spree/product_decorator.rb         | 265 ------------------
 app/models/spree/product_option_type.rb       |   9 +
 .../spree/product_option_type_decorator.rb    |  10 -
 app/models/spree/product_property.rb          |   2 +-
 .../spree/product_property_decorator.rb       |   5 -
 app/models/spree/variant.rb                   | 147 ++++++++--
 app/models/spree/variant_decorator.rb         | 141 ----------
 12 files changed, 404 insertions(+), 471 deletions(-)
 delete mode 100644 app/models/spree/option_type_decorator.rb
 delete mode 100644 app/models/spree/price_decorator.rb
 delete mode 100644 app/models/spree/product_decorator.rb
 delete mode 100644 app/models/spree/product_option_type_decorator.rb
 delete mode 100644 app/models/spree/product_property_decorator.rb
 delete mode 100644 app/models/spree/variant_decorator.rb

diff --git a/app/models/spree/option_type.rb b/app/models/spree/option_type.rb
index 5246243809..075c597481 100644
--- a/app/models/spree/option_type.rb
+++ b/app/models/spree/option_type.rb
@@ -1,5 +1,6 @@
 module Spree
   class OptionType < ActiveRecord::Base
+    has_many :products, through: :product_option_types
     has_many :option_values, -> { order(:position) }, dependent: :destroy
     has_many :product_option_types, dependent: :destroy
     has_and_belongs_to_many :prototypes, join_table: 'spree_option_types_prototypes'
diff --git a/app/models/spree/option_type_decorator.rb b/app/models/spree/option_type_decorator.rb
deleted file mode 100644
index 1ef46caa4e..0000000000
--- a/app/models/spree/option_type_decorator.rb
+++ /dev/null
@@ -1,5 +0,0 @@
-module Spree
-  OptionType.class_eval do
-    has_many :products, through: :product_option_types
-  end
-end
diff --git a/app/models/spree/price.rb b/app/models/spree/price.rb
index ed85b5675e..878c689873 100644
--- a/app/models/spree/price.rb
+++ b/app/models/spree/price.rb
@@ -1,5 +1,7 @@
 module Spree
   class Price < ActiveRecord::Base
+    acts_as_paranoid without_default_scope: true
+
     belongs_to :variant, class_name: 'Spree::Variant'
 
     validate :check_price
@@ -22,10 +24,14 @@ module Spree
       self[:amount] = parse_price(price)
     end
 
-    private
-    def check_price
-      raise "Price must belong to a variant" if variant.nil?
+    # Allow prices to access associated soft-deleted variants.
+    def variant
+      Spree::Variant.unscoped { super }
+    end
 
+    private
+
+    def check_price
       if currency.nil?
         self.currency = Spree::Config[:currency]
       end
diff --git a/app/models/spree/price_decorator.rb b/app/models/spree/price_decorator.rb
deleted file mode 100644
index 6c67ca5949..0000000000
--- a/app/models/spree/price_decorator.rb
+++ /dev/null
@@ -1,18 +0,0 @@
-module Spree
-  Price.class_eval do
-    acts_as_paranoid without_default_scope: true
-
-    # Allow prices to access associated soft-deleted variants.
-    def variant
-      Spree::Variant.unscoped { super }
-    end
-
-    private
-
-    def check_price
-      if currency.nil?
-        self.currency = Spree::Config[:currency]
-      end
-    end
-  end
-end
diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index 14e01acfa1..f165b539f7 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -1,3 +1,7 @@
+require 'open_food_network/permalink_generator'
+require 'open_food_network/property_merge'
+require 'concerns/product_stock'
+
 # PRODUCTS
 # Products represent an entity for sale in a store.
 # Products can have variations, called variants
@@ -17,12 +21,18 @@
 # All other variants have option values and may have inventory units.
 # Sum of on_hand each variant's inventory level determine "on_hand" level for the product.
 #
-
 module Spree
   class Product < ActiveRecord::Base
+    include PermalinkGenerator
+    include ProductStock
+
     acts_as_paranoid
+
     has_many :product_option_types, dependent: :destroy
-    has_many :option_types, through: :product_option_types
+    # We have an after_destroy callback on Spree::ProductOptionType. However, if we
+    # don't specify dependent => destroy on this association, it is not called. See:
+    # https://github.com/rails/rails/issues/7618
+    has_many :option_types, through: :product_option_types, dependent: :destroy
     has_many :product_properties, dependent: :destroy
     has_many :properties, through: :product_properties
 
@@ -32,6 +42,8 @@ module Spree
 
     belongs_to :tax_category, class_name: 'Spree::TaxCategory'
     belongs_to :shipping_category, class_name: 'Spree::ShippingCategory'
+    belongs_to :supplier, class_name: 'Enterprise', touch: true
+    belongs_to :primary_taxon, class_name: 'Spree::Taxon', touch: true
 
     has_one :master,
       -> { where is_master: true },
@@ -51,8 +63,9 @@ module Spree
 
     has_many :stock_items, through: :variants
 
-    delegate_belongs_to :master, :sku, :price, :currency, :display_amount, :display_price, :weight, :height, :width, :depth, :is_master, :has_default_price?, :cost_currency, :price_in, :amount_in
+    delegate_belongs_to :master, :sku, :price, :currency, :display_amount, :display_price, :weight, :height, :width, :depth, :is_master, :has_default_price?, :cost_currency, :price_in, :amount_in, :unit_value, :unit_description
     delegate_belongs_to :master, :cost_price if Variant.table_exists? && Variant.column_names.include?('cost_price')
+    delegate :images_attributes=, :display_as=, to: :master
 
     after_create :set_master_variant_defaults
     after_create :add_properties_and_option_types_from_prototype
@@ -71,6 +84,16 @@ module Spree
     validates :price, presence: true, if: proc { Spree::Config[:require_master_price] }
     validates :shipping_category_id, presence: true
 
+    validates :supplier, presence: true
+    validates :primary_taxon, presence: true
+    validates :tax_category_id, presence: true, if: "Spree::Config.products_require_tax_category"
+
+    validates :variant_unit, presence: true
+    validates :variant_unit_scale,
+              presence: { if: ->(p) { %w(weight volume).include? p.variant_unit } }
+    validates :variant_unit_name,
+              presence: { if: ->(p) { p.variant_unit == 'items' } }
+
     attr_accessor :option_values_hash
 
     accepts_nested_attributes_for :product_properties, allow_destroy: true, reject_if: lambda { |pp| pp[:property_name].blank? }
@@ -80,9 +103,111 @@ module Spree
     alias :options :product_option_types
 
     after_initialize :ensure_master
+    after_initialize :set_available_on_to_now, if: :new_record?
+
+    before_validation :sanitize_permalink
+    before_save :add_primary_taxon_to_taxons
+    after_save :remove_previous_primary_taxon_from_taxons
+    after_save :ensure_standard_variant
+    after_save :update_units
 
     before_destroy :punch_permalink
 
+    # -- Joins
+    scope :with_order_cycles_outer, -> {
+      joins("
+        LEFT OUTER JOIN spree_variants AS o_spree_variants
+          ON (o_spree_variants.product_id = spree_products.id)").
+        joins("
+          LEFT OUTER JOIN exchange_variants AS o_exchange_variants
+            ON (o_exchange_variants.variant_id = o_spree_variants.id)").
+        joins("
+          LEFT OUTER JOIN exchanges AS o_exchanges
+            ON (o_exchanges.id = o_exchange_variants.exchange_id)").
+        joins("
+          LEFT OUTER JOIN order_cycles AS o_order_cycles
+            ON (o_order_cycles.id = o_exchanges.order_cycle_id)")
+    }
+
+    scope :imported_on, lambda { |import_date|
+      import_date = Time.zone.parse import_date if import_date.is_a? String
+      import_date = import_date.to_date
+      joins(:variants).merge(Spree::Variant.where(import_date: import_date.beginning_of_day..import_date.end_of_day))
+    }
+
+    scope :with_order_cycles_inner, -> {
+      joins(variants_including_master: { exchanges: :order_cycle })
+    }
+
+    scope :visible_for, lambda { |enterprise|
+      joins('LEFT OUTER JOIN spree_variants AS o_spree_variants ON (o_spree_variants.product_id = spree_products.id)').
+        joins('LEFT OUTER JOIN inventory_items AS o_inventory_items ON (o_spree_variants.id = o_inventory_items.variant_id)').
+        where('o_inventory_items.enterprise_id = (?) AND visible = (?)', enterprise, true)
+    }
+
+    # -- Scopes
+    scope :in_supplier, lambda { |supplier| where(supplier_id: supplier) }
+
+    # Products distributed via the given distributor through an OC
+    scope :in_distributor, lambda { |distributor|
+      distributor = distributor.respond_to?(:id) ? distributor.id : distributor.to_i
+
+      with_order_cycles_outer.
+        where('(o_exchanges.incoming = ? AND o_exchanges.receiver_id = ?)', false, distributor).
+        select('distinct spree_products.*')
+    }
+
+    scope :in_distributors, lambda { |distributors|
+      with_order_cycles_outer.
+        where('(o_exchanges.incoming = ? AND o_exchanges.receiver_id IN (?))', false, distributors).
+        uniq
+    }
+
+    # Products supplied by a given enterprise or distributed via that enterprise through an OC
+    scope :in_supplier_or_distributor, lambda { |enterprise|
+      enterprise = enterprise.respond_to?(:id) ? enterprise.id : enterprise.to_i
+
+      with_order_cycles_outer.
+        where("
+          spree_products.supplier_id = ?
+          OR (o_exchanges.incoming = ? AND o_exchanges.receiver_id = ?)
+        ", enterprise, false, enterprise).
+        select('distinct spree_products.*')
+    }
+
+    # Products distributed by the given order cycle
+    scope :in_order_cycle, lambda { |order_cycle|
+      with_order_cycles_inner.
+        merge(Exchange.outgoing).
+        where('order_cycles.id = ?', order_cycle)
+    }
+
+    scope :in_an_active_order_cycle, lambda {
+      with_order_cycles_inner.
+        merge(OrderCycle.active).
+        merge(Exchange.outgoing).
+        where('order_cycles.id IS NOT NULL')
+    }
+
+    scope :by_producer, -> { joins(:supplier).order('enterprises.name') }
+    scope :by_name, -> { order('name') }
+
+    scope :managed_by, lambda { |user|
+      if user.has_spree_role?('admin')
+        where(nil)
+      else
+        where('supplier_id IN (?)', user.enterprises.select("enterprises.id"))
+      end
+    }
+
+    scope :stockable_by, lambda { |enterprise|
+      return where('1=0') if enterprise.blank?
+
+      permitted_producer_ids = EnterpriseRelationship.joins(:parent).permitting(enterprise.id)
+        .with_permission(:add_to_order_cycle).where(enterprises: { is_primary_producer: true }).pluck(:parent_id)
+      return where('spree_products.supplier_id IN (?)', [enterprise.id] | permitted_producer_ids)
+    }
+
     def to_param
       permalink.present? ? permalink : (permalink_was || name.to_s.to_url)
     end
@@ -122,6 +247,12 @@ module Spree
       duplicator.duplicate
     end
 
+    # Called by Spree::Product::duplicate before saving.
+    def duplicate_extra(_parent)
+      # Spree sets the SKU to "COPY OF #{parent sku}".
+      master.sku = ''
+    end
+
     # use deleted? rather than checking the attribute directly. this
     # allows extensions to override deleted? if they want to provide
     # their own definition.
@@ -199,6 +330,60 @@ module Spree
       super || variants_including_master.with_deleted.where(is_master: true).first
     end
 
+    def properties_including_inherited
+      # Product properties override producer properties
+      ps = product_properties.all
+
+      if inherits_properties
+        ps = OpenFoodNetwork::PropertyMerge.merge(ps, supplier.producer_properties)
+      end
+
+      ps.
+        sort_by(&:position).
+        map { |pp| { id: pp.property.id, name: pp.property.presentation, value: pp.value } }
+    end
+
+    def in_distributor?(distributor)
+      self.class.in_distributor(distributor).include? self
+    end
+
+    def in_order_cycle?(order_cycle)
+      self.class.in_order_cycle(order_cycle).include? self
+    end
+
+    def variants_distributed_by(order_cycle, distributor)
+      order_cycle.variants_distributed_by(distributor).where(product_id: self)
+    end
+
+    # Get the most recent import_date of a product's variants
+    def import_date
+      variants.map(&:import_date).compact.max
+    end
+
+    def variant_unit_option_type
+      if variant_unit.present?
+        option_type_name = "unit_#{variant_unit}"
+        option_type_presentation = variant_unit.capitalize
+
+        Spree::OptionType.find_by(name: option_type_name) ||
+          Spree::OptionType.create!(name: option_type_name,
+                                    presentation: option_type_presentation)
+      end
+    end
+
+    def destroy_with_delete_from_order_cycles
+      transaction do
+        touch_distributors
+
+        ExchangeVariant.
+          where('exchange_variants.variant_id IN (?)', variants_including_master.with_deleted.
+          select(:id)).destroy_all
+
+        destroy_without_delete_from_order_cycles
+      end
+    end
+    alias_method_chain :destroy, :delete_from_order_cycles
+
     private
 
       # Builds variants from a hash of option types & values
@@ -230,10 +415,26 @@ module Spree
         master.is_master = true
       end
 
-      # there's a weird quirk with the delegate stuff that does not automatically save the delegate object
-      # when saving so we force a save using a hook.
+      # This fixes any problems arising from failing master saves, without the need for a validates_associated on
+      # master, while giving us more specific errors as to why saving failed
       def save_master
-        master.save if master && (master.changed? || master.new_record? || (master.default_price && (master.default_price.changed || master.default_price.new_record)))
+        if master && (
+            master.changed? || master.new_record? || (
+              master.default_price && (
+                master.default_price.changed? || master.default_price.new_record?
+              )
+            )
+          )
+          master.save!
+        end
+
+        # If the master cannot be saved, the Product object will get its errors
+        # and will be destroyed
+      rescue ActiveRecord::RecordInvalid
+        master.errors.each do |att, error|
+          errors.add(att, error)
+        end
+        raise
       end
 
       def ensure_master
@@ -244,6 +445,53 @@ module Spree
       def punch_permalink
         update_attribute :permalink, "#{Time.now.to_i}_#{permalink}" # punch permalink with date prefix
       end
+
+    def set_available_on_to_now
+      self.available_on ||= Time.zone.now
+    end
+
+    def update_units
+      if variant_unit_changed?
+        option_types.delete self.class.all_variant_unit_option_types
+        option_types << variant_unit_option_type if variant_unit.present?
+        variants_including_master.each(&:update_units)
+      end
+    end
+
+    def touch_distributors
+      Enterprise.distributing_products(id).each(&:touch)
+    end
+
+    def add_primary_taxon_to_taxons
+      taxons << primary_taxon unless taxons.include? primary_taxon
+    end
+
+    def remove_previous_primary_taxon_from_taxons
+      return unless primary_taxon_id_changed? && primary_taxon_id_was
+
+      taxons.destroy(primary_taxon_id_was)
+    end
+
+    def self.all_variant_unit_option_types
+      Spree::OptionType.where('name LIKE ?', 'unit_%%')
+    end
+
+    def ensure_standard_variant
+      if master.valid? && variants.empty?
+        variant = master.dup
+        variant.product = self
+        variant.is_master = false
+        variants << variant
+      end
+    end
+
+    # Spree creates a permalink already but our implementation fixes an edge case.
+    def sanitize_permalink
+      if permalink.blank? || permalink_changed?
+        requested = permalink.presence || permalink_was.presence || name.presence || 'product'
+        self.permalink = create_unique_permalink(requested.parameterize)
+      end
+    end
   end
 end
 
diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb
deleted file mode 100644
index 554309f687..0000000000
--- a/app/models/spree/product_decorator.rb
+++ /dev/null
@@ -1,265 +0,0 @@
-require 'open_food_network/permalink_generator'
-require 'open_food_network/property_merge'
-require 'concerns/product_stock'
-
-Spree::Product.class_eval do
-  include PermalinkGenerator
-  include ProductStock
-
-  # We have an after_destroy callback on Spree::ProductOptionType. However, if we
-  # don't specify dependent => destroy on this association, it is not called. See:
-  # https://github.com/rails/rails/issues/7618
-  has_many :option_types, through: :product_option_types, dependent: :destroy
-
-  belongs_to :supplier, class_name: 'Enterprise', touch: true
-  belongs_to :primary_taxon, class_name: 'Spree::Taxon', touch: true
-
-  delegate_belongs_to :master, :unit_value, :unit_description
-  delegate :images_attributes=, :display_as=, to: :master
-
-  validates :supplier, presence: true
-  validates :primary_taxon, presence: true
-  validates :tax_category_id, presence: true, if: "Spree::Config.products_require_tax_category"
-
-  validates :variant_unit, presence: true
-  validates :variant_unit_scale,
-            presence: { if: ->(p) { %w(weight volume).include? p.variant_unit } }
-  validates :variant_unit_name,
-            presence: { if: ->(p) { p.variant_unit == 'items' } }
-
-  after_initialize :set_available_on_to_now, if: :new_record?
-  before_validation :sanitize_permalink
-  before_save :add_primary_taxon_to_taxons
-  after_save :remove_previous_primary_taxon_from_taxons
-  after_save :ensure_standard_variant
-  after_save :update_units
-
-  # -- Joins
-  scope :with_order_cycles_outer, -> {
-    joins("
-      LEFT OUTER JOIN spree_variants AS o_spree_variants
-        ON (o_spree_variants.product_id = spree_products.id)").
-      joins("
-        LEFT OUTER JOIN exchange_variants AS o_exchange_variants
-          ON (o_exchange_variants.variant_id = o_spree_variants.id)").
-      joins("
-        LEFT OUTER JOIN exchanges AS o_exchanges
-          ON (o_exchanges.id = o_exchange_variants.exchange_id)").
-      joins("
-        LEFT OUTER JOIN order_cycles AS o_order_cycles
-          ON (o_order_cycles.id = o_exchanges.order_cycle_id)")
-  }
-
-  scope :imported_on, lambda { |import_date|
-    import_date = Time.zone.parse import_date if import_date.is_a? String
-    import_date = import_date.to_date
-    joins(:variants).merge(Spree::Variant.where(import_date: import_date.beginning_of_day..import_date.end_of_day))
-  }
-
-  scope :with_order_cycles_inner, -> {
-    joins(variants_including_master: { exchanges: :order_cycle })
-  }
-
-  scope :visible_for, lambda { |enterprise|
-    joins('LEFT OUTER JOIN spree_variants AS o_spree_variants ON (o_spree_variants.product_id = spree_products.id)').
-      joins('LEFT OUTER JOIN inventory_items AS o_inventory_items ON (o_spree_variants.id = o_inventory_items.variant_id)').
-      where('o_inventory_items.enterprise_id = (?) AND visible = (?)', enterprise, true)
-  }
-
-  # -- Scopes
-  scope :in_supplier, lambda { |supplier| where(supplier_id: supplier) }
-
-  # Products distributed via the given distributor through an OC
-  scope :in_distributor, lambda { |distributor|
-    distributor = distributor.respond_to?(:id) ? distributor.id : distributor.to_i
-
-    with_order_cycles_outer.
-      where('(o_exchanges.incoming = ? AND o_exchanges.receiver_id = ?)', false, distributor).
-      select('distinct spree_products.*')
-  }
-
-  scope :in_distributors, lambda { |distributors|
-    with_order_cycles_outer.
-      where('(o_exchanges.incoming = ? AND o_exchanges.receiver_id IN (?))', false, distributors).
-      uniq
-  }
-
-  # Products supplied by a given enterprise or distributed via that enterprise through an OC
-  scope :in_supplier_or_distributor, lambda { |enterprise|
-    enterprise = enterprise.respond_to?(:id) ? enterprise.id : enterprise.to_i
-
-    with_order_cycles_outer.
-      where("
-        spree_products.supplier_id = ?
-        OR (o_exchanges.incoming = ? AND o_exchanges.receiver_id = ?)
-      ", enterprise, false, enterprise).
-      select('distinct spree_products.*')
-  }
-
-  # Products distributed by the given order cycle
-  scope :in_order_cycle, lambda { |order_cycle|
-    with_order_cycles_inner.
-      merge(Exchange.outgoing).
-      where('order_cycles.id = ?', order_cycle)
-  }
-
-  scope :in_an_active_order_cycle, lambda {
-    with_order_cycles_inner.
-      merge(OrderCycle.active).
-      merge(Exchange.outgoing).
-      where('order_cycles.id IS NOT NULL')
-  }
-
-  scope :by_producer, -> { joins(:supplier).order('enterprises.name') }
-  scope :by_name, -> { order('name') }
-
-  scope :managed_by, lambda { |user|
-    if user.has_spree_role?('admin')
-      where(nil)
-    else
-      where('supplier_id IN (?)', user.enterprises.select("enterprises.id"))
-    end
-  }
-
-  scope :stockable_by, lambda { |enterprise|
-    return where('1=0') if enterprise.blank?
-
-    permitted_producer_ids = EnterpriseRelationship.joins(:parent).permitting(enterprise.id)
-      .with_permission(:add_to_order_cycle).where(enterprises: { is_primary_producer: true }).pluck(:parent_id)
-    return where('spree_products.supplier_id IN (?)', [enterprise.id] | permitted_producer_ids)
-  }
-
-  # -- Methods
-
-  # Called by Spree::Product::duplicate before saving.
-  def duplicate_extra(_parent)
-    # Spree sets the SKU to "COPY OF #{parent sku}".
-    master.sku = ''
-  end
-
-  def properties_including_inherited
-    # Product properties override producer properties
-    ps = product_properties.all
-
-    if inherits_properties
-      ps = OpenFoodNetwork::PropertyMerge.merge(ps, supplier.producer_properties)
-    end
-
-    ps.
-      sort_by(&:position).
-      map { |pp| { id: pp.property.id, name: pp.property.presentation, value: pp.value } }
-  end
-
-  def in_distributor?(distributor)
-    self.class.in_distributor(distributor).include? self
-  end
-
-  def in_order_cycle?(order_cycle)
-    self.class.in_order_cycle(order_cycle).include? self
-  end
-
-  def variants_distributed_by(order_cycle, distributor)
-    order_cycle.variants_distributed_by(distributor).where(product_id: self)
-  end
-
-  # Get the most recent import_date of a product's variants
-  def import_date
-    variants.map(&:import_date).compact.max
-  end
-
-  def variant_unit_option_type
-    if variant_unit.present?
-      option_type_name = "unit_#{variant_unit}"
-      option_type_presentation = variant_unit.capitalize
-
-      Spree::OptionType.find_by(name: option_type_name) ||
-        Spree::OptionType.create!(name: option_type_name,
-                                  presentation: option_type_presentation)
-    end
-  end
-
-  def destroy_with_delete_from_order_cycles
-    transaction do
-      touch_distributors
-
-      ExchangeVariant.
-        where('exchange_variants.variant_id IN (?)', variants_including_master.with_deleted.
-        select(:id)).destroy_all
-
-      destroy_without_delete_from_order_cycles
-    end
-  end
-  alias_method_chain :destroy, :delete_from_order_cycles
-
-  private
-
-  def set_available_on_to_now
-    self.available_on ||= Time.zone.now
-  end
-
-  def update_units
-    if variant_unit_changed?
-      option_types.delete self.class.all_variant_unit_option_types
-      option_types << variant_unit_option_type if variant_unit.present?
-      variants_including_master.each(&:update_units)
-    end
-  end
-
-  def touch_distributors
-    Enterprise.distributing_products(id).each(&:touch)
-  end
-
-  def add_primary_taxon_to_taxons
-    taxons << primary_taxon unless taxons.include? primary_taxon
-  end
-
-  def remove_previous_primary_taxon_from_taxons
-    return unless primary_taxon_id_changed? && primary_taxon_id_was
-
-    taxons.destroy(primary_taxon_id_was)
-  end
-
-  def self.all_variant_unit_option_types
-    Spree::OptionType.where('name LIKE ?', 'unit_%%')
-  end
-
-  def ensure_standard_variant
-    if master.valid? && variants.empty?
-      variant = master.dup
-      variant.product = self
-      variant.is_master = false
-      variants << variant
-    end
-  end
-
-  # Override Spree's old save_master method and replace it with the most recent method from spree repository
-  # This fixes any problems arising from failing master saves, without the need for a validates_associated on
-  # master, while giving us more specific errors as to why saving failed
-  def save_master
-    if master && (
-        master.changed? || master.new_record? || (
-          master.default_price && (
-            master.default_price.changed? || master.default_price.new_record?
-          )
-        )
-      )
-      master.save!
-    end
-
-    # If the master cannot be saved, the Product object will get its errors
-    # and will be destroyed
-  rescue ActiveRecord::RecordInvalid
-    master.errors.each do |att, error|
-      errors.add(att, error)
-    end
-    raise
-  end
-
-  # Spree creates a permalink already but our implementation fixes an edge case.
-  def sanitize_permalink
-    if permalink.blank? || permalink_changed?
-      requested = permalink.presence || permalink_was.presence || name.presence || 'product'
-      self.permalink = create_unique_permalink(requested.parameterize)
-    end
-  end
-end
diff --git a/app/models/spree/product_option_type.rb b/app/models/spree/product_option_type.rb
index 2a63434c48..47f438c7b4 100644
--- a/app/models/spree/product_option_type.rb
+++ b/app/models/spree/product_option_type.rb
@@ -1,7 +1,16 @@
 module Spree
   class ProductOptionType < ActiveRecord::Base
+    after_destroy :remove_option_values
+
     belongs_to :product, class_name: 'Spree::Product'
     belongs_to :option_type, class_name: 'Spree::OptionType'
     acts_as_list scope: :product
+
+    def remove_option_values
+      product.variants_including_master.each do |variant|
+        option_values = variant.option_values.where(option_type_id: option_type)
+        variant.option_values.destroy(*option_values)
+      end
+    end
   end
 end
diff --git a/app/models/spree/product_option_type_decorator.rb b/app/models/spree/product_option_type_decorator.rb
deleted file mode 100644
index 8e5a909058..0000000000
--- a/app/models/spree/product_option_type_decorator.rb
+++ /dev/null
@@ -1,10 +0,0 @@
-Spree::ProductOptionType.class_eval do
-  after_destroy :remove_option_values
-
-  def remove_option_values
-    product.variants_including_master.each do |variant|
-      option_values = variant.option_values.where(option_type_id: option_type)
-      variant.option_values.destroy(*option_values)
-    end
-  end
-end
diff --git a/app/models/spree/product_property.rb b/app/models/spree/product_property.rb
index aa947d878d..a62ea5d7ee 100644
--- a/app/models/spree/product_property.rb
+++ b/app/models/spree/product_property.rb
@@ -1,6 +1,6 @@
 module Spree
   class ProductProperty < ActiveRecord::Base
-    belongs_to :product,  class_name: 'Spree::Product'
+    belongs_to :product, class_name: "Spree::Product", touch: true
     belongs_to :property, class_name: 'Spree::Property'
 
     validates :property, presence: true
diff --git a/app/models/spree/product_property_decorator.rb b/app/models/spree/product_property_decorator.rb
deleted file mode 100644
index 58ee8fb1ad..0000000000
--- a/app/models/spree/product_property_decorator.rb
+++ /dev/null
@@ -1,5 +0,0 @@
-module Spree
-  ProductProperty.class_eval do
-    belongs_to :product, class_name: "Spree::Product", touch: true
-  end
-end
diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
index 9b9fb1f329..f23e54c9a9 100644
--- a/app/models/spree/variant.rb
+++ b/app/models/spree/variant.rb
@@ -1,9 +1,16 @@
+require 'open_food_network/enterprise_fee_calculator'
+require 'variant_units/variant_and_line_item_naming'
+require 'concerns/variant_stock'
+
 module Spree
   class Variant < ActiveRecord::Base
+    extend Spree::LocalizedNumber
+    include VariantUnits::VariantAndLineItemNaming
+    include VariantStock
+
     acts_as_paranoid
 
     belongs_to :product, touch: true, class_name: 'Spree::Product'
-
     delegate_belongs_to :product, :name, :description, :permalink, :available_on,
                         :tax_category_id, :shipping_category_id, :meta_description,
                         :meta_keywords, :tax_category, :shipping_category
@@ -16,54 +23,150 @@ module Spree
     has_many :stock_movements
 
     has_and_belongs_to_many :option_values, join_table: :spree_option_values_variants
+
     has_many :images, -> { order(:position) }, as: :viewable, dependent: :destroy, class_name: "Spree::Image"
+    accepts_nested_attributes_for :images
 
     has_one :default_price,
       -> { where currency: Spree::Config[:currency] },
       class_name: 'Spree::Price',
       dependent: :destroy
-
-    delegate_belongs_to :default_price, :display_price, :display_amount, :price, :price=, :currency if Spree::Price.table_exists?
-
     has_many :prices,
       class_name: 'Spree::Price',
       dependent: :destroy
+    delegate_belongs_to :default_price, :display_price, :display_amount, :price, :price=, :currency if Spree::Price.table_exists?
+
+    has_many :exchange_variants
+    has_many :exchanges, through: :exchange_variants
+    has_many :variant_overrides
+    has_many :inventory_items
+
+    localize_number :price, :cost_price, :weight
 
     validate :check_price
     validates :price, numericality: { greater_than_or_equal_to: 0 }, presence: true, if: proc { Spree::Config[:require_master_price] }
     validates :cost_price, numericality: { greater_than_or_equal_to: 0, allow_nil: true } if self.table_exists? && self.column_names.include?('cost_price')
 
+    validates :unit_value, presence: true, if: ->(variant) {
+      %w(weight volume).include?(variant.product.andand.variant_unit)
+    }
+
+    validates :unit_description, presence: true, if: ->(variant) {
+      variant.product.andand.variant_unit.present? && variant.unit_value.nil?
+    }
+
     before_validation :set_cost_currency
+    before_validation :update_weight_from_unit_value, if: ->(v) { v.product.present? }
+
     after_save :save_default_price
+    after_save :update_units
+
     after_create :create_stock_items
     after_create :set_position
 
+    around_destroy :destruction
+
     # default variant scope only lists non-deleted variants
     scope :deleted, lambda { where('deleted_at IS NOT NULL') }
 
+    scope :with_order_cycles_inner, -> { joins(exchanges: :order_cycle) }
+
+    scope :not_master, -> { where(is_master: false) }
+    scope :in_order_cycle, lambda { |order_cycle|
+      with_order_cycles_inner.
+        merge(Exchange.outgoing).
+        where('order_cycles.id = ?', order_cycle).
+        select('DISTINCT spree_variants.*')
+    }
+
+    scope :in_schedule, lambda { |schedule|
+      joins(exchanges: { order_cycle: :schedules }).
+        merge(Exchange.outgoing).
+        where(schedules: { id: schedule }).
+        select('DISTINCT spree_variants.*')
+    }
+
+    scope :for_distribution, lambda { |order_cycle, distributor|
+      where('spree_variants.id IN (?)', order_cycle.variants_distributed_by(distributor).select(&:id))
+    }
+
+    scope :visible_for, lambda { |enterprise|
+      joins(:inventory_items).
+        where(
+          'inventory_items.enterprise_id = (?) AND inventory_items.visible = (?)',
+          enterprise,
+          true
+        )
+    }
+
+    scope :not_hidden_for, lambda { |enterprise|
+      return where("1=0") if enterprise.blank?
+
+      joins("
+        LEFT OUTER JOIN (SELECT *
+                           FROM inventory_items
+                           WHERE enterprise_id = #{sanitize enterprise.andand.id})
+          AS o_inventory_items
+          ON o_inventory_items.variant_id = spree_variants.id")
+        .where("o_inventory_items.id IS NULL OR o_inventory_items.visible = (?)", true)
+    }
+
+    scope :stockable_by, lambda { |enterprise|
+      return where("1=0") if enterprise.blank?
+
+      joins(:product).
+        where(spree_products: { id: Spree::Product.stockable_by(enterprise).pluck(:id) })
+    }
+
+    # Define sope as class method to allow chaining with other scopes filtering id.
+    # In Rails 3, merging two scopes on the same column will consider only the last scope.
+    def self.in_distributor(distributor)
+      where(id: ExchangeVariant.select(:variant_id).
+                joins(:exchange).
+                where('exchanges.incoming = ? AND exchanges.receiver_id = ?', false, distributor))
+    end
+
+    def self.indexed
+      scoped.index_by(&:id)
+    end
+
     def self.active(currency = nil)
-      joins(:prices).where(deleted_at: nil).where('spree_prices.currency' => currency || Spree::Config[:currency]).where('spree_prices.amount IS NOT NULL')
+      # "where(id:" is necessary so that the returned relation has no includes
+      # The relation without includes will not be readonly and allow updates on it
+      where("spree_variants.id in (?)", joins(:prices).
+                                          where(deleted_at: nil).
+                                          where('spree_prices.currency' =>
+                                            currency || Spree::Config[:currency]).
+                                          where('spree_prices.amount IS NOT NULL').
+                                          select("spree_variants.id"))
     end
 
     def cost_price=(price)
       self[:cost_price] = parse_price(price) if price.present?
     end
 
+    # Allow variant to access associated soft-deleted prices.
+    def default_price
+      Spree::Price.unscoped { super }
+    end
+
+    def price_with_fees(distributor, order_cycle)
+      price + fees_for(distributor, order_cycle)
+    end
+
+    def fees_for(distributor, order_cycle)
+      OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle).fees_for self
+    end
+
+    def fees_by_type_for(distributor, order_cycle)
+      OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle).fees_by_type_for self
+    end
+
     # returns number of units currently on backorder for this variant.
     def on_backorder
       inventory_units.with_state('backordered').size
     end
 
-    def options_text
-      values = self.option_values.joins(:option_type).order("#{Spree::OptionType.table_name}.position asc")
-
-      values.map! do |ov|
-        "#{ov.option_type.presentation}: #{ov.presentation}"
-      end
-
-      values.to_sentence({ words_connector: ", ", two_words_connector: ", " })
-    end
-
     def gross_profit
       cost_price.nil? ? 0 : (price - cost_price)
     end
@@ -133,8 +236,9 @@ module Spree
       Spree::Product.unscoped { super }
     end
     
-    def in_stock?(quantity=1)
-      Spree::Stock::Quantifier.new(self).can_supply?(quantity)
+    # can_supply? is implemented in VariantStock
+    def in_stock?(quantity = 1)
+      can_supply?(quantity)
     end
 
     def total_on_hand
@@ -183,6 +287,15 @@ module Spree
       def set_position
         self.update_column(:position, product.variants.maximum(:position).to_i + 1)
       end
+
+    def update_weight_from_unit_value
+      self.weight = weight_from_unit_value if product.variant_unit == 'weight' && unit_value.present?
+    end
+
+    def destruction
+      exchange_variants(:reload).destroy_all
+      yield
+    end
   end
 end
 
diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb
deleted file mode 100644
index 737b2a8da4..0000000000
--- a/app/models/spree/variant_decorator.rb
+++ /dev/null
@@ -1,141 +0,0 @@
-require 'open_food_network/enterprise_fee_calculator'
-require 'variant_units/variant_and_line_item_naming'
-require 'concerns/variant_stock'
-
-Spree::Variant.class_eval do
-  extend Spree::LocalizedNumber
-  # Remove method From Spree, so method from the naming module is used instead
-  # This file may be double-loaded in delayed job environment, so we check before
-  # removing the Spree method to prevent error.
-  remove_method :options_text if instance_methods(false).include? :options_text
-  include VariantUnits::VariantAndLineItemNaming
-  include VariantStock
-
-  has_many :exchange_variants
-  has_many :exchanges, through: :exchange_variants
-  has_many :variant_overrides
-  has_many :inventory_items
-
-  accepts_nested_attributes_for :images
-
-  validates :unit_value, presence: true, if: ->(variant) {
-    %w(weight volume).include?(variant.product.andand.variant_unit)
-  }
-
-  validates :unit_description, presence: true, if: ->(variant) {
-    variant.product.andand.variant_unit.present? && variant.unit_value.nil?
-  }
-
-  before_validation :update_weight_from_unit_value, if: ->(v) { v.product.present? }
-  after_save :update_units
-  around_destroy :destruction
-
-  scope :with_order_cycles_inner, -> { joins(exchanges: :order_cycle) }
-
-  scope :not_master, -> { where(is_master: false) }
-  scope :in_order_cycle, lambda { |order_cycle|
-    with_order_cycles_inner.
-      merge(Exchange.outgoing).
-      where('order_cycles.id = ?', order_cycle).
-      select('DISTINCT spree_variants.*')
-  }
-
-  scope :in_schedule, lambda { |schedule|
-    joins(exchanges: { order_cycle: :schedules }).
-      merge(Exchange.outgoing).
-      where(schedules: { id: schedule }).
-      select('DISTINCT spree_variants.*')
-  }
-
-  scope :for_distribution, lambda { |order_cycle, distributor|
-    where('spree_variants.id IN (?)', order_cycle.variants_distributed_by(distributor).select(&:id))
-  }
-
-  scope :visible_for, lambda { |enterprise|
-    joins(:inventory_items).
-      where(
-        'inventory_items.enterprise_id = (?) AND inventory_items.visible = (?)',
-        enterprise,
-        true
-      )
-  }
-
-  scope :not_hidden_for, lambda { |enterprise|
-    return where("1=0") if enterprise.blank?
-
-    joins("
-      LEFT OUTER JOIN (SELECT *
-                         FROM inventory_items
-                         WHERE enterprise_id = #{sanitize enterprise.andand.id})
-        AS o_inventory_items
-        ON o_inventory_items.variant_id = spree_variants.id")
-      .where("o_inventory_items.id IS NULL OR o_inventory_items.visible = (?)", true)
-  }
-
-  localize_number :price, :cost_price, :weight
-
-  scope :stockable_by, lambda { |enterprise|
-    return where("1=0") if enterprise.blank?
-
-    joins(:product).
-      where(spree_products: { id: Spree::Product.stockable_by(enterprise).pluck(:id) })
-  }
-
-  # Define sope as class method to allow chaining with other scopes filtering id.
-  # In Rails 3, merging two scopes on the same column will consider only the last scope.
-  def self.in_distributor(distributor)
-    where(id: ExchangeVariant.select(:variant_id).
-              joins(:exchange).
-              where('exchanges.incoming = ? AND exchanges.receiver_id = ?', false, distributor))
-  end
-
-  def self.indexed
-    scoped.index_by(&:id)
-  end
-
-  def self.active(currency = nil)
-    # "where(id:" is necessary so that the returned relation has no includes
-    # The relation without includes will not be readonly and allow updates on it
-    where("spree_variants.id in (?)", joins(:prices).
-                                        where(deleted_at: nil).
-                                        where('spree_prices.currency' =>
-                                          currency || Spree::Config[:currency]).
-                                        where('spree_prices.amount IS NOT NULL').
-                                        select("spree_variants.id"))
-  end
-
-  # We override in_stock? to avoid depending
-  #   on the non-overridable method Spree::Stock::Quantifier.can_supply?
-  #   VariantStock implements can_supply? itself which depends on overridable methods
-  def in_stock?(quantity = 1)
-    can_supply?(quantity)
-  end
-
-  # Allow variant to access associated soft-deleted prices.
-  def default_price
-    Spree::Price.unscoped { super }
-  end
-
-  def price_with_fees(distributor, order_cycle)
-    price + fees_for(distributor, order_cycle)
-  end
-
-  def fees_for(distributor, order_cycle)
-    OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle).fees_for self
-  end
-
-  def fees_by_type_for(distributor, order_cycle)
-    OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle).fees_by_type_for self
-  end
-
-  private
-
-  def update_weight_from_unit_value
-    self.weight = weight_from_unit_value if product.variant_unit == 'weight' && unit_value.present?
-  end
-
-  def destruction
-    exchange_variants(:reload).destroy_all
-    yield
-  end
-end

From f85044e035bc371119e906e44a4a5882d673f046 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 7 Aug 2020 20:16:56 +0100
Subject: [PATCH 03/17] Run rubocop autocorrect

---
 app/models/spree/option_type.rb         |   4 +-
 app/models/spree/option_value.rb        |   2 +
 app/models/spree/price.rb               |   4 +-
 app/models/spree/product.rb             | 124 +++++++++++++-----------
 app/models/spree/product_option_type.rb |   2 +
 app/models/spree/product_property.rb    |   8 +-
 app/models/spree/variant.rb             | 111 +++++++++++----------
 7 files changed, 137 insertions(+), 118 deletions(-)

diff --git a/app/models/spree/option_type.rb b/app/models/spree/option_type.rb
index 075c597481..3bfebdc6be 100644
--- a/app/models/spree/option_type.rb
+++ b/app/models/spree/option_type.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Spree
   class OptionType < ActiveRecord::Base
     has_many :products, through: :product_option_types
@@ -6,7 +8,7 @@ module Spree
     has_and_belongs_to_many :prototypes, join_table: 'spree_option_types_prototypes'
 
     validates :name, :presentation, presence: true
-    default_scope -> { order("#{self.table_name}.position") }
+    default_scope -> { order("#{table_name}.position") }
 
     accepts_nested_attributes_for :option_values, reject_if: lambda { |ov| ov[:name].blank? || ov[:presentation].blank? }, allow_destroy: true
   end
diff --git a/app/models/spree/option_value.rb b/app/models/spree/option_value.rb
index dade69e534..cad9ba464a 100644
--- a/app/models/spree/option_value.rb
+++ b/app/models/spree/option_value.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Spree
   class OptionValue < ActiveRecord::Base
     belongs_to :option_type
diff --git a/app/models/spree/price.rb b/app/models/spree/price.rb
index 878c689873..69670223dd 100644
--- a/app/models/spree/price.rb
+++ b/app/models/spree/price.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Spree
   class Price < ActiveRecord::Base
     acts_as_paranoid without_default_scope: true
@@ -48,7 +50,5 @@ module Spree
 
       price.to_d
     end
-
   end
 end
-
diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index f165b539f7..7e5aaf8d95 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'open_food_network/permalink_generator'
 require 'open_food_network/property_merge'
 require 'concerns/product_stock'
@@ -46,18 +48,18 @@ module Spree
     belongs_to :primary_taxon, class_name: 'Spree::Taxon', touch: true
 
     has_one :master,
-      -> { where is_master: true },
-      class_name: 'Spree::Variant',
-      dependent: :destroy
+            -> { where is_master: true },
+            class_name: 'Spree::Variant',
+            dependent: :destroy
 
     has_many :variants,
-      -> { where(is_master: false).order("#{::Spree::Variant.quoted_table_name}.position ASC") },
-      class_name: 'Spree::Variant'
+             -> { where(is_master: false).order("#{::Spree::Variant.quoted_table_name}.position ASC") },
+             class_name: 'Spree::Variant'
 
     has_many :variants_including_master,
-      -> { order("#{::Spree::Variant.quoted_table_name}.position ASC") },
-      class_name: 'Spree::Variant',
-      dependent: :destroy
+             -> { order("#{::Spree::Variant.quoted_table_name}.position ASC") },
+             class_name: 'Spree::Variant',
+             dependent: :destroy
 
     has_many :prices, -> { order('spree_variants.position, spree_variants.id, currency') }, through: :variants
 
@@ -234,8 +236,9 @@ module Spree
     # Ensures option_types and product_option_types exist for keys in option_values_hash
     def ensure_option_types_exist_for_values_hash
       return if option_values_hash.nil?
+
       option_values_hash.keys.map(&:to_i).each do |id|
-        self.option_type_ids << id unless option_type_ids.include?(id)
+        option_type_ids << id unless option_type_ids.include?(id)
         product_option_types.create(option_type_id: id) unless product_option_types.pluck(:option_type_id).include?(id)
       end
     end
@@ -268,7 +271,8 @@ module Spree
     # eg categorise_variants_from_option(color) => {"red" -> [...], "blue" -> [...]}
     def categorise_variants_from_option(opt_type)
       return {} unless option_types.include?(opt_type)
-      variants.active.group_by { |v| v.option_values.detect { |o| o.option_type == opt_type} }
+
+      variants.active.group_by { |v| v.option_values.detect { |o| o.option_type == opt_type } }
     end
 
     def self.like_any(fields, values)
@@ -298,6 +302,7 @@ module Spree
 
     def property(property_name)
       return nil unless prop = properties.find_by(name: property_name)
+
       product_properties.find_by(property: prop).try(:value)
     end
 
@@ -317,7 +322,7 @@ module Spree
 
     def total_on_hand
       if Spree::Config.track_inventory_levels
-        self.stock_items.sum(&:count_on_hand)
+        stock_items.sum(&:count_on_hand)
       else
         Float::INFINITY
       end
@@ -386,65 +391,66 @@ module Spree
 
     private
 
-      # Builds variants from a hash of option types & values
-      def build_variants_from_option_values_hash
-        ensure_option_types_exist_for_values_hash
-        values = option_values_hash.values
-        values = values.inject(values.shift) { |memo, value| memo.product(value).map(&:flatten) }
+    # Builds variants from a hash of option types & values
+    def build_variants_from_option_values_hash
+      ensure_option_types_exist_for_values_hash
+      values = option_values_hash.values
+      values = values.inject(values.shift) { |memo, value| memo.product(value).map(&:flatten) }
 
-        values.each do |ids|
-          variant = variants.create(
-            option_value_ids: ids,
-            price: master.price
-          )
+      values.each do |ids|
+        variant = variants.create(
+          option_value_ids: ids,
+          price: master.price
+        )
+      end
+      save
+    end
+
+    def add_properties_and_option_types_from_prototype
+      if prototype_id && prototype = Spree::Prototype.find_by(id: prototype_id)
+        prototype.properties.each do |property|
+          product_properties.create(property: property)
         end
-        save
+        self.option_types = prototype.option_types
       end
+    end
 
-      def add_properties_and_option_types_from_prototype
-        if prototype_id && prototype = Spree::Prototype.find_by(id: prototype_id)
-          prototype.properties.each do |property|
-            product_properties.create(property: property)
-          end
-          self.option_types = prototype.option_types
-        end
-      end
+    # ensures the master variant is flagged as such
+    def set_master_variant_defaults
+      master.is_master = true
+    end
 
-      # ensures the master variant is flagged as such
-      def set_master_variant_defaults
-        master.is_master = true
-      end
-
-      # This fixes any problems arising from failing master saves, without the need for a validates_associated on
-      # master, while giving us more specific errors as to why saving failed
-      def save_master
-        if master && (
-            master.changed? || master.new_record? || (
-              master.default_price && (
-                master.default_price.changed? || master.default_price.new_record?
-              )
+    # This fixes any problems arising from failing master saves, without the need for a validates_associated on
+    # master, while giving us more specific errors as to why saving failed
+    def save_master
+      if master && (
+          master.changed? || master.new_record? || (
+            master.default_price && (
+              master.default_price.changed? || master.default_price.new_record?
             )
           )
-          master.save!
-        end
-
-        # If the master cannot be saved, the Product object will get its errors
-        # and will be destroyed
-      rescue ActiveRecord::RecordInvalid
-        master.errors.each do |att, error|
-          errors.add(att, error)
-        end
-        raise
+        )
+        master.save!
       end
 
-      def ensure_master
-        return unless new_record?
-        self.master ||= Variant.new
+      # If the master cannot be saved, the Product object will get its errors
+      # and will be destroyed
+    rescue ActiveRecord::RecordInvalid
+      master.errors.each do |att, error|
+        errors.add(att, error)
       end
+      raise
+    end
 
-      def punch_permalink
-        update_attribute :permalink, "#{Time.now.to_i}_#{permalink}" # punch permalink with date prefix
-      end
+    def ensure_master
+      return unless new_record?
+
+      self.master ||= Variant.new
+    end
+
+    def punch_permalink
+      update_attribute :permalink, "#{Time.now.to_i}_#{permalink}" # punch permalink with date prefix
+    end
 
     def set_available_on_to_now
       self.available_on ||= Time.zone.now
diff --git a/app/models/spree/product_option_type.rb b/app/models/spree/product_option_type.rb
index 47f438c7b4..b02db6a440 100644
--- a/app/models/spree/product_option_type.rb
+++ b/app/models/spree/product_option_type.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Spree
   class ProductOptionType < ActiveRecord::Base
     after_destroy :remove_option_values
diff --git a/app/models/spree/product_property.rb b/app/models/spree/product_property.rb
index a62ea5d7ee..68e17bfa86 100644
--- a/app/models/spree/product_property.rb
+++ b/app/models/spree/product_property.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Spree
   class ProductProperty < ActiveRecord::Base
     belongs_to :product, class_name: "Spree::Product", touch: true
@@ -6,15 +8,15 @@ module Spree
     validates :property, presence: true
     validates :value, length: { maximum: 255 }
 
-    default_scope -> { order("#{self.table_name}.position") }
+    default_scope -> { order("#{table_name}.position") }
 
     # virtual attributes for use with AJAX completion stuff
     def property_name
-      property.name if property
+      property&.name
     end
 
     def property_name=(name)
-      unless name.blank?
+      if name.present?
         unless property = Property.find_by(name: name)
           property = Property.create(name: name, presentation: name)
         end
diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
index f23e54c9a9..13d7e769d6 100644
--- a/app/models/spree/variant.rb
+++ b/app/models/spree/variant.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'open_food_network/enterprise_fee_calculator'
 require 'variant_units/variant_and_line_item_naming'
 require 'concerns/variant_stock'
@@ -28,12 +30,12 @@ module Spree
     accepts_nested_attributes_for :images
 
     has_one :default_price,
-      -> { where currency: Spree::Config[:currency] },
-      class_name: 'Spree::Price',
-      dependent: :destroy
+            -> { where currency: Spree::Config[:currency] },
+            class_name: 'Spree::Price',
+            dependent: :destroy
     has_many :prices,
-      class_name: 'Spree::Price',
-      dependent: :destroy
+             class_name: 'Spree::Price',
+             dependent: :destroy
     delegate_belongs_to :default_price, :display_price, :display_amount, :price, :price=, :currency if Spree::Price.table_exists?
 
     has_many :exchange_variants
@@ -45,7 +47,7 @@ module Spree
 
     validate :check_price
     validates :price, numericality: { greater_than_or_equal_to: 0 }, presence: true, if: proc { Spree::Config[:require_master_price] }
-    validates :cost_price, numericality: { greater_than_or_equal_to: 0, allow_nil: true } if self.table_exists? && self.column_names.include?('cost_price')
+    validates :cost_price, numericality: { greater_than_or_equal_to: 0, allow_nil: true } if table_exists? && column_names.include?('cost_price')
 
     validates :unit_value, presence: true, if: ->(variant) {
       %w(weight volume).include?(variant.product.andand.variant_unit)
@@ -180,24 +182,25 @@ module Spree
 
     def set_option_value(opt_name, opt_value)
       # no option values on master
-      return if self.is_master
+      return if is_master
 
       option_type = Spree::OptionType.where(name: opt_name).first_or_initialize do |o|
         o.presentation = opt_name
         o.save!
       end
 
-      current_value = self.option_values.detect { |o| o.option_type.name == opt_name }
+      current_value = option_values.detect { |o| o.option_type.name == opt_name }
 
-      unless current_value.nil?
-        return if current_value.name == opt_value
-        self.option_values.delete(current_value)
-      else
+      if current_value.nil?
         # then we have to check to make sure that the product has the option type
-        unless self.product.option_types.include? option_type
-          self.product.option_types << option_type
-          self.product.save
+        unless product.option_types.include? option_type
+          product.option_types << option_type
+          product.save
         end
+      else
+        return if current_value.name == opt_value
+
+        option_values.delete(current_value)
       end
 
       option_value = Spree::OptionValue.where(option_type_id: option_type.id, name: opt_value).first_or_initialize do |o|
@@ -205,20 +208,20 @@ module Spree
         o.save!
       end
 
-      self.option_values << option_value
-      self.save
+      option_values << option_value
+      save
     end
 
     def option_value(opt_name)
-      self.option_values.detect { |o| o.option_type.name == opt_name }.try(:presentation)
+      option_values.detect { |o| o.option_type.name == opt_name }.try(:presentation)
     end
 
     def has_default_price?
-      !self.default_price.nil?
+      !default_price.nil?
     end
 
     def price_in(currency)
-      prices.select{ |price| price.currency == currency }.first || Spree::Price.new(variant_id: self.id, currency: currency)
+      prices.select{ |price| price.currency == currency }.first || Spree::Price.new(variant_id: id, currency: currency)
     end
 
     def amount_in(currency)
@@ -235,7 +238,7 @@ module Spree
     def product
       Spree::Product.unscoped { super }
     end
-    
+
     # can_supply? is implemented in VariantStock
     def in_stock?(quantity = 1)
       can_supply?(quantity)
@@ -246,47 +249,49 @@ module Spree
     end
 
     private
-      # strips all non-price-like characters from the price, taking into account locale settings
-      def parse_price(price)
-        return price unless price.is_a?(String)
 
-        separator, delimiter = I18n.t([:'number.currency.format.separator', :'number.currency.format.delimiter'])
-        non_price_characters = /[^0-9\-#{separator}]/
-        price.gsub!(non_price_characters, '') # strip everything else first
-        price.gsub!(separator, '.') unless separator == '.' # then replace the locale-specific decimal separator with the standard separator if necessary
+    # strips all non-price-like characters from the price, taking into account locale settings
+    def parse_price(price)
+      return price unless price.is_a?(String)
 
-        price.to_d
+      separator, delimiter = I18n.t([:'number.currency.format.separator', :'number.currency.format.delimiter'])
+      non_price_characters = /[^0-9\-#{separator}]/
+      price.gsub!(non_price_characters, '') # strip everything else first
+      price.gsub!(separator, '.') unless separator == '.' # then replace the locale-specific decimal separator with the standard separator if necessary
+
+      price.to_d
+    end
+
+    # Ensures a new variant takes the product master price when price is not supplied
+    def check_price
+      if price.nil? && Spree::Config[:require_master_price]
+        raise 'No master variant found to infer price' unless product&.master
+        raise 'Must supply price for variant or master.price for product.' if self == product.master
+
+        self.price = product.master.price
       end
-
-      # Ensures a new variant takes the product master price when price is not supplied
-      def check_price
-        if price.nil? && Spree::Config[:require_master_price]
-          raise 'No master variant found to infer price' unless (product && product.master)
-          raise 'Must supply price for variant or master.price for product.' if self == product.master
-          self.price = product.master.price
-        end
-        if currency.nil?
-          self.currency = Spree::Config[:currency]
-        end
+      if currency.nil?
+        self.currency = Spree::Config[:currency]
       end
+    end
 
-      def save_default_price
-        default_price.save if default_price && (default_price.changed? || default_price.new_record?)
-      end
+    def save_default_price
+      default_price.save if default_price && (default_price.changed? || default_price.new_record?)
+    end
 
-      def set_cost_currency
-        self.cost_currency = Spree::Config[:currency] if cost_currency.nil? || cost_currency.empty?
-      end
+    def set_cost_currency
+      self.cost_currency = Spree::Config[:currency] if cost_currency.blank?
+    end
 
-      def create_stock_items
-        StockLocation.all.each do |stock_location|
-          stock_location.propagate_variant(self) if stock_location.propagate_all_variants?
-        end
+    def create_stock_items
+      StockLocation.all.find_each do |stock_location|
+        stock_location.propagate_variant(self) if stock_location.propagate_all_variants?
       end
+    end
 
-      def set_position
-        self.update_column(:position, product.variants.maximum(:position).to_i + 1)
-      end
+    def set_position
+      update_column(:position, product.variants.maximum(:position).to_i + 1)
+    end
 
     def update_weight_from_unit_value
       self.weight = weight_from_unit_value if product.variant_unit == 'weight' && unit_value.present?

From 2f8198eeccca7e2a41f1e51e228fff898a55c575 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 7 Aug 2020 20:35:22 +0100
Subject: [PATCH 04/17] Fix some easy rubocop issues

---
 app/models/spree/option_value.rb     |   3 +-
 app/models/spree/price.rb            |  15 ++--
 app/models/spree/product.rb          | 113 +++++++++++++++------------
 app/models/spree/product_property.rb |  10 +--
 app/models/spree/variant.rb          |  37 ++++++---
 5 files changed, 105 insertions(+), 73 deletions(-)

diff --git a/app/models/spree/option_value.rb b/app/models/spree/option_value.rb
index cad9ba464a..b21eed19ba 100644
--- a/app/models/spree/option_value.rb
+++ b/app/models/spree/option_value.rb
@@ -4,7 +4,8 @@ module Spree
   class OptionValue < ActiveRecord::Base
     belongs_to :option_type
     acts_as_list scope: :option_type
-    has_and_belongs_to_many :variants, join_table: 'spree_option_values_variants', class_name: "Spree::Variant"
+    has_and_belongs_to_many :variants, join_table: 'spree_option_values_variants',
+                                       class_name: "Spree::Variant"
 
     validates :name, :presentation, presence: true
   end
diff --git a/app/models/spree/price.rb b/app/models/spree/price.rb
index 69670223dd..d0dab12359 100644
--- a/app/models/spree/price.rb
+++ b/app/models/spree/price.rb
@@ -34,19 +34,22 @@ module Spree
     private
 
     def check_price
-      if currency.nil?
-        self.currency = Spree::Config[:currency]
-      end
+      return unless currency.nil?
+
+      self.currency = Spree::Config[:currency]
     end
 
     # strips all non-price-like characters from the price, taking into account locale settings
     def parse_price(price)
       return price unless price.is_a?(String)
 
-      separator, delimiter = I18n.t([:'number.currency.format.separator', :'number.currency.format.delimiter'])
+      separator, _delimiter = I18n.t([:'number.currency.format.separator',
+                                      :'number.currency.format.delimiter'])
       non_price_characters = /[^0-9\-#{separator}]/
-      price.gsub!(non_price_characters, '') # strip everything else first
-      price.gsub!(separator, '.') unless separator == '.' # then replace the locale-specific decimal separator with the standard separator if necessary
+      # Strip everything else first
+      price.gsub!(non_price_characters, '')
+      # Then replace the locale-specific decimal separator with the standard separator if necessary
+      price.gsub!(separator, '.') unless separator == '.'
 
       price.to_d
     end
diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index 7e5aaf8d95..76ca236351 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -52,21 +52,26 @@ module Spree
             class_name: 'Spree::Variant',
             dependent: :destroy
 
-    has_many :variants,
-             -> { where(is_master: false).order("#{::Spree::Variant.quoted_table_name}.position ASC") },
-             class_name: 'Spree::Variant'
+    has_many :variants, -> {
+      where(is_master: false).order("#{::Spree::Variant.quoted_table_name}.position ASC")
+    }, class_name: 'Spree::Variant'
 
     has_many :variants_including_master,
              -> { order("#{::Spree::Variant.quoted_table_name}.position ASC") },
              class_name: 'Spree::Variant',
              dependent: :destroy
 
-    has_many :prices, -> { order('spree_variants.position, spree_variants.id, currency') }, through: :variants
+    has_many :prices, -> {
+      order('spree_variants.position, spree_variants.id, currency')
+    }, through: :variants
 
     has_many :stock_items, through: :variants
 
-    delegate_belongs_to :master, :sku, :price, :currency, :display_amount, :display_price, :weight, :height, :width, :depth, :is_master, :has_default_price?, :cost_currency, :price_in, :amount_in, :unit_value, :unit_description
-    delegate_belongs_to :master, :cost_price if Variant.table_exists? && Variant.column_names.include?('cost_price')
+    delegate_belongs_to :master, :sku, :price, :currency, :display_amount, :display_price, :weight,
+                        :height, :width, :depth, :is_master, :has_default_price?, :cost_currency,
+                        :price_in, :amount_in, :unit_value, :unit_description
+    delegate_belongs_to :master, :cost_price if Variant.table_exists? &&
+                                                Variant.column_names.include?('cost_price')
     delegate :images_attributes=, :display_as=, to: :master
 
     after_create :set_master_variant_defaults
@@ -77,7 +82,8 @@ module Spree
     delegate :images, to: :master, prefix: true
     alias_method :images, :master_images
 
-    has_many :variant_images, -> { order(:position) }, source: :images, through: :variants_including_master
+    has_many :variant_images, -> { order(:position) }, source: :images,
+                                                       through: :variants_including_master
 
     accepts_nested_attributes_for :variants, allow_destroy: true
 
@@ -98,7 +104,9 @@ module Spree
 
     attr_accessor :option_values_hash
 
-    accepts_nested_attributes_for :product_properties, allow_destroy: true, reject_if: lambda { |pp| pp[:property_name].blank? }
+    accepts_nested_attributes_for :product_properties,
+                                  allow_destroy: true,
+                                  reject_if: lambda { |pp| pp[:property_name].blank? }
 
     make_permalink order: :name
 
@@ -134,7 +142,8 @@ module Spree
     scope :imported_on, lambda { |import_date|
       import_date = Time.zone.parse import_date if import_date.is_a? String
       import_date = import_date.to_date
-      joins(:variants).merge(Spree::Variant.where(import_date: import_date.beginning_of_day..import_date.end_of_day))
+      joins(:variants).merge(Spree::Variant.
+        where(import_date: import_date.beginning_of_day..import_date.end_of_day))
     }
 
     scope :with_order_cycles_inner, -> {
@@ -206,7 +215,9 @@ module Spree
       return where('1=0') if enterprise.blank?
 
       permitted_producer_ids = EnterpriseRelationship.joins(:parent).permitting(enterprise.id)
-        .with_permission(:add_to_order_cycle).where(enterprises: { is_primary_producer: true }).pluck(:parent_id)
+        .with_permission(:add_to_order_cycle)
+        .where(enterprises: { is_primary_producer: true })
+        .pluck(:parent_id)
       return where('spree_products.supplier_id IN (?)', [enterprise.id] | permitted_producer_ids)
     }
 
@@ -221,7 +232,7 @@ module Spree
 
     def tax_category
       if self[:tax_category_id].nil?
-        TaxCategory.where(is_default: true).first
+        TaxCategory.find_by(is_default: true)
       else
         TaxCategory.find(self[:tax_category_id])
       end
@@ -239,7 +250,9 @@ module Spree
 
       option_values_hash.keys.map(&:to_i).each do |id|
         option_type_ids << id unless option_type_ids.include?(id)
-        product_option_types.create(option_type_id: id) unless product_option_types.pluck(:option_type_id).include?(id)
+        unless product_option_types.pluck(:option_type_id).include?(id)
+          product_option_types.create(option_type_id: id)
+        end
       end
     end
 
@@ -309,7 +322,8 @@ module Spree
     def set_property(property_name, property_value)
       ActiveRecord::Base.transaction do
         property = Property.where(name: property_name).first_or_create!(presentation: property_name)
-        product_property = ProductProperty.where(product: self, property: property).first_or_initialize
+        product_property = ProductProperty.where(product: self,
+                                                 property: property).first_or_initialize
         product_property.value = property_value
         product_property.save!
       end
@@ -332,7 +346,7 @@ module Spree
     # which would make AR's default finder return nil.
     # This is a stopgap for that little problem.
     def master
-      super || variants_including_master.with_deleted.where(is_master: true).first
+      super || variants_including_master.with_deleted.find_by(is_master: true)
     end
 
     def properties_including_inherited
@@ -366,14 +380,18 @@ module Spree
     end
 
     def variant_unit_option_type
-      if variant_unit.present?
-        option_type_name = "unit_#{variant_unit}"
-        option_type_presentation = variant_unit.capitalize
+      return if variant_unit.blank?
 
-        Spree::OptionType.find_by(name: option_type_name) ||
-          Spree::OptionType.create!(name: option_type_name,
-                                    presentation: option_type_presentation)
-      end
+      option_type_name = "unit_#{variant_unit}"
+      option_type_presentation = variant_unit.capitalize
+
+      Spree::OptionType.find_by(name: option_type_name) ||
+        Spree::OptionType.create!(name: option_type_name,
+                                  presentation: option_type_presentation)
+    end
+
+    def self.all_variant_unit_option_types
+      Spree::OptionType.where('name LIKE ?', 'unit_%%')
     end
 
     def destroy_with_delete_from_order_cycles
@@ -398,7 +416,7 @@ module Spree
       values = values.inject(values.shift) { |memo, value| memo.product(value).map(&:flatten) }
 
       values.each do |ids|
-        variant = variants.create(
+        variants.create(
           option_value_ids: ids,
           price: master.price
         )
@@ -407,12 +425,12 @@ module Spree
     end
 
     def add_properties_and_option_types_from_prototype
-      if prototype_id && prototype = Spree::Prototype.find_by(id: prototype_id)
-        prototype.properties.each do |property|
-          product_properties.create(property: property)
-        end
-        self.option_types = prototype.option_types
+      return unless prototype_id && prototype = Spree::Prototype.find_by(id: prototype_id)
+
+      prototype.properties.each do |property|
+        product_properties.create(property: property)
       end
+      self.option_types = prototype.option_types
     end
 
     # ensures the master variant is flagged as such
@@ -420,8 +438,8 @@ module Spree
       master.is_master = true
     end
 
-    # This fixes any problems arising from failing master saves, without the need for a validates_associated on
-    # master, while giving us more specific errors as to why saving failed
+    # Here we rescue errors when saving master variants (without the need for a
+    #   validates_associated on master) and we get more specific data about the errors
     def save_master
       if master && (
           master.changed? || master.new_record? || (
@@ -449,7 +467,8 @@ module Spree
     end
 
     def punch_permalink
-      update_attribute :permalink, "#{Time.now.to_i}_#{permalink}" # punch permalink with date prefix
+      # Punch permalink with date prefix
+      update_attribute :permalink, "#{Time.now.to_i}_#{permalink}"
     end
 
     def set_available_on_to_now
@@ -457,11 +476,11 @@ module Spree
     end
 
     def update_units
-      if variant_unit_changed?
-        option_types.delete self.class.all_variant_unit_option_types
-        option_types << variant_unit_option_type if variant_unit.present?
-        variants_including_master.each(&:update_units)
-      end
+      return unless variant_unit_changed?
+
+      option_types.delete self.class.all_variant_unit_option_types
+      option_types << variant_unit_option_type if variant_unit.present?
+      variants_including_master.each(&:update_units)
     end
 
     def touch_distributors
@@ -478,25 +497,21 @@ module Spree
       taxons.destroy(primary_taxon_id_was)
     end
 
-    def self.all_variant_unit_option_types
-      Spree::OptionType.where('name LIKE ?', 'unit_%%')
-    end
-
     def ensure_standard_variant
-      if master.valid? && variants.empty?
-        variant = master.dup
-        variant.product = self
-        variant.is_master = false
-        variants << variant
-      end
+      return unless master.valid? && variants.empty?
+
+      variant = master.dup
+      variant.product = self
+      variant.is_master = false
+      variants << variant
     end
 
     # Spree creates a permalink already but our implementation fixes an edge case.
     def sanitize_permalink
-      if permalink.blank? || permalink_changed?
-        requested = permalink.presence || permalink_was.presence || name.presence || 'product'
-        self.permalink = create_unique_permalink(requested.parameterize)
-      end
+      return unless permalink.blank? || permalink_changed?
+
+      requested = permalink.presence || permalink_was.presence || name.presence || 'product'
+      self.permalink = create_unique_permalink(requested.parameterize)
     end
   end
 end
diff --git a/app/models/spree/product_property.rb b/app/models/spree/product_property.rb
index 68e17bfa86..a86a12f1a9 100644
--- a/app/models/spree/product_property.rb
+++ b/app/models/spree/product_property.rb
@@ -16,12 +16,12 @@ module Spree
     end
 
     def property_name=(name)
-      if name.present?
-        unless property = Property.find_by(name: name)
-          property = Property.create(name: name, presentation: name)
-        end
-        self.property = property
+      return if name.blank?
+
+      unless property = Property.find_by(name: name)
+        property = Property.create(name: name, presentation: name)
       end
+      self.property = property
     end
   end
 end
diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
index 13d7e769d6..1abf737d60 100644
--- a/app/models/spree/variant.rb
+++ b/app/models/spree/variant.rb
@@ -26,7 +26,9 @@ module Spree
 
     has_and_belongs_to_many :option_values, join_table: :spree_option_values_variants
 
-    has_many :images, -> { order(:position) }, as: :viewable, dependent: :destroy, class_name: "Spree::Image"
+    has_many :images, -> { order(:position) }, as: :viewable,
+                                               dependent: :destroy,
+                                               class_name: "Spree::Image"
     accepts_nested_attributes_for :images
 
     has_one :default_price,
@@ -46,7 +48,9 @@ module Spree
     localize_number :price, :cost_price, :weight
 
     validate :check_price
-    validates :price, numericality: { greater_than_or_equal_to: 0 }, presence: true, if: proc { Spree::Config[:require_master_price] }
+    validates :price, numericality: { greater_than_or_equal_to: 0 },
+                      presence: true,
+                      if: proc { Spree::Config[:require_master_price] }
     validates :cost_price, numericality: { greater_than_or_equal_to: 0, allow_nil: true } if table_exists? && column_names.include?('cost_price')
 
     validates :unit_value, presence: true, if: ->(variant) {
@@ -89,7 +93,8 @@ module Spree
     }
 
     scope :for_distribution, lambda { |order_cycle, distributor|
-      where('spree_variants.id IN (?)', order_cycle.variants_distributed_by(distributor).select(&:id))
+      where('spree_variants.id IN (?)', order_cycle.variants_distributed_by(distributor).
+        select(&:id))
     }
 
     scope :visible_for, lambda { |enterprise|
@@ -203,7 +208,8 @@ module Spree
         option_values.delete(current_value)
       end
 
-      option_value = Spree::OptionValue.where(option_type_id: option_type.id, name: opt_value).first_or_initialize do |o|
+      option_value = Spree::OptionValue.where(option_type_id: option_type.id,
+                                              name: opt_value).first_or_initialize do |o|
         o.presentation = opt_value
         o.save!
       end
@@ -221,7 +227,8 @@ module Spree
     end
 
     def price_in(currency)
-      prices.select{ |price| price.currency == currency }.first || Spree::Price.new(variant_id: id, currency: currency)
+      prices.select{ |price| price.currency == currency }.first ||
+        Spree::Price.new(variant_id: id, currency: currency)
     end
 
     def amount_in(currency)
@@ -254,10 +261,13 @@ module Spree
     def parse_price(price)
       return price unless price.is_a?(String)
 
-      separator, delimiter = I18n.t([:'number.currency.format.separator', :'number.currency.format.delimiter'])
+      separator, _delimiter = I18n.t([:'number.currency.format.separator',
+                                      :'number.currency.format.delimiter'])
       non_price_characters = /[^0-9\-#{separator}]/
-      price.gsub!(non_price_characters, '') # strip everything else first
-      price.gsub!(separator, '.') unless separator == '.' # then replace the locale-specific decimal separator with the standard separator if necessary
+      # Strip everything else first
+      price.gsub!(non_price_characters, '')
+      # Then replace the locale-specific decimal separator with the standard separator if necessary
+      price.gsub!(separator, '.') unless separator == '.'
 
       price.to_d
     end
@@ -270,9 +280,10 @@ module Spree
 
         self.price = product.master.price
       end
-      if currency.nil?
-        self.currency = Spree::Config[:currency]
-      end
+
+      return unless currency.nil?
+
+      self.currency = Spree::Config[:currency]
     end
 
     def save_default_price
@@ -294,7 +305,9 @@ module Spree
     end
 
     def update_weight_from_unit_value
-      self.weight = weight_from_unit_value if product.variant_unit == 'weight' && unit_value.present?
+      return unless product.variant_unit == 'weight' && unit_value.present?
+
+      self.weight = weight_from_unit_value
     end
 
     def destruction

From 072cd2bd544c61ddeea1653d57c44e38325969de Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 7 Aug 2020 20:37:49 +0100
Subject: [PATCH 05/17] Run rubocop autocorrect in specs

---
 spec/models/spree/product_option_type_spec.rb |  5 --
 spec/models/spree/product_property_spec.rb    |  5 +-
 spec/models/spree/product_spec.rb             | 54 +++++++++----------
 spec/models/spree/variant_spec.rb             | 24 ++++-----
 4 files changed, 41 insertions(+), 47 deletions(-)
 delete mode 100644 spec/models/spree/product_option_type_spec.rb

diff --git a/spec/models/spree/product_option_type_spec.rb b/spec/models/spree/product_option_type_spec.rb
deleted file mode 100644
index 8bf1870013..0000000000
--- a/spec/models/spree/product_option_type_spec.rb
+++ /dev/null
@@ -1,5 +0,0 @@
-require 'spec_helper'
-
-describe Spree::ProductOptionType do
-
-end
diff --git a/spec/models/spree/product_property_spec.rb b/spec/models/spree/product_property_spec.rb
index e979efb769..34bd96a314 100644
--- a/spec/models/spree/product_property_spec.rb
+++ b/spec/models/spree/product_property_spec.rb
@@ -1,14 +1,13 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
 describe Spree::ProductProperty do
-
   context "validations" do
     it "should validate length of value" do
       pp = create(:product_property)
       pp.value = "x" * 256
       pp.should_not be_valid
     end
-
   end
-
 end
diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb
index e5c0aa072c..6b12dead78 100644
--- a/spec/models/spree/product_spec.rb
+++ b/spec/models/spree/product_spec.rb
@@ -7,7 +7,7 @@ module Spree
 
       context '#duplicate' do
         before do
-          product.stub :taxons => [create(:taxon)]
+          product.stub taxons: [create(:taxon)]
         end
 
         it 'duplicates product' do
@@ -42,7 +42,7 @@ module Spree
 
       context "product has variants" do
         before do
-          create(:variant, :product => product)
+          create(:variant, product: product)
         end
 
         context "#destroy" do
@@ -148,8 +148,8 @@ module Spree
 
     context "permalink" do
       context "build product with similar name" do
-        let!(:other) { create(:product, :name => 'foo bar') }
-        let(:product) { build(:product, :name => 'foo') }
+        let!(:other) { create(:product, name: 'foo bar') }
+        let(:product) { build(:product, name: 'foo') }
 
         before { product.valid? }
 
@@ -160,31 +160,31 @@ module Spree
 
       context "build permalink with quotes" do
         it "saves quotes" do
-          product = create(:product, :name => "Joe's", :permalink => "joe's")
+          product = create(:product, name: "Joe's", permalink: "joe's")
           product.permalink.should == "joe's"
         end
       end
 
       context "permalinks must be unique" do
         before do
-          @product1 = create(:product, :name => 'foo')
+          @product1 = create(:product, name: 'foo')
         end
 
         it "cannot create another product with the same permalink" do
-          pending '[Spree build] Failing spec'        
-          @product2 = create(:product, :name => 'foo')
+          pending '[Spree build] Failing spec'
+          @product2 = create(:product, name: 'foo')
           lambda do
-            @product2.update_attributes(:permalink => @product1.permalink)
+            @product2.update(permalink: @product1.permalink)
           end.should raise_error(ActiveRecord::RecordNotUnique)
         end
       end
 
       it "supports Chinese" do
-        create(:product, :name => "你好").permalink.should == "ni-hao"
+        create(:product, name: "你好").permalink.should == "ni-hao"
       end
 
       context "manual permalink override" do
-        let(:product) { create(:product, :name => "foo") }
+        let(:product) { create(:product, name: "foo") }
 
         it "calling save_permalink with a parameter" do
           product.name = "foobar"
@@ -196,17 +196,17 @@ module Spree
         end
       end
 
-      context "override permalink of deleted product" do 
-        let(:product) { create(:product, :name => "foo") } 
+      context "override permalink of deleted product" do
+        let(:product) { create(:product, name: "foo") }
 
-        it "should create product with same permalink from name like deleted product" do 
-          product.permalink.should == "foo" 
-          product.destroy 
-          
-          new_product = create(:product, :name => "foo") 
-          new_product.permalink.should == "foo" 
-        end 
-      end 
+        it "should create product with same permalink from name like deleted product" do
+          product.permalink.should == "foo"
+          product.destroy
+
+          new_product = create(:product, name: "foo")
+          new_product.permalink.should == "foo"
+        end
+      end
     end
 
     context "properties" do
@@ -237,11 +237,11 @@ module Spree
 
       # Regression test for #2455
       it "should not overwrite properties' presentation names" do
-        Spree::Property.where(:name => 'foo').first_or_create!(:presentation => "Foo's Presentation Name")
+        Spree::Property.where(name: 'foo').first_or_create!(presentation: "Foo's Presentation Name")
         product.set_property('foo', 'value1')
         product.set_property('bar', 'value2')
-        Spree::Property.where(:name => 'foo').first.presentation.should == "Foo's Presentation Name"
-        Spree::Property.where(:name => 'bar').first.presentation.should == "bar"
+        Spree::Property.where(name: 'foo').first.presentation.should == "Foo's Presentation Name"
+        Spree::Property.where(name: 'bar').first.presentation.should == "bar"
       end
     end
 
@@ -260,16 +260,16 @@ module Spree
 
       context "when prototype with option types is supplied" do
         def build_option_type_with_values(name, values)
-          ot = create(:option_type, :name => name)
+          ot = create(:option_type, name: name)
           values.each do |val|
-            ot.option_values.create(:name => val.downcase, :presentation => val)
+            ot.option_values.create(name: val.downcase, presentation: val)
           end
           ot
         end
 
         let(:prototype) do
           size = build_option_type_with_values("size", %w(Small Medium Large))
-          create(:prototype, :name => "Size", :option_types => [ size ])
+          create(:prototype, name: "Size", option_types: [size])
         end
 
         let(:option_values_hash) do
diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb
index 6db5047062..4d1aafc4d5 100644
--- a/spec/models/spree/variant_spec.rb
+++ b/spec/models/spree/variant_spec.rb
@@ -22,7 +22,7 @@ module Spree
 
       it "propagate to stock items" do
         Spree::StockLocation.any_instance.should_receive(:propagate_variant)
-        product.variants.create(:name => "Foobar")
+        product.variants.create(name: "Foobar")
       end
 
       context "stock location has disable propagate all variants" do
@@ -30,7 +30,7 @@ module Spree
 
         it "propagate to stock items" do
           Spree::StockLocation.any_instance.should_not_receive(:propagate_variant)
-          product.variants.create(:name => "Foobar")
+          product.variants.create(name: "Foobar")
         end
       end
     end
@@ -38,7 +38,7 @@ module Spree
     context "product has other variants" do
       describe "option value accessors" do
         before {
-          @multi_variant = FactoryGirl.create :variant, :product => variant.product
+          @multi_variant = FactoryGirl.create :variant, product: variant.product
           variant.product.reload
         }
 
@@ -58,7 +58,7 @@ module Spree
           multi_variant.set_option_value('media_type', 'CD')
 
           expect {
-           multi_variant.set_option_value('media_type', 'DVD')
+            multi_variant.set_option_value('media_type', 'DVD')
           }.to_not change(multi_variant.option_values, :count)
 
           expect {
@@ -70,7 +70,7 @@ module Spree
       context "product has other variants" do
         describe "option value accessors" do
           before {
-            @multi_variant = create(:variant, :product => variant.product)
+            @multi_variant = create(:variant, product: variant.product)
             variant.product.reload
           }
 
@@ -90,7 +90,7 @@ module Spree
             multi_variant.set_option_value('media_type', 'CD')
 
             expect {
-             multi_variant.set_option_value('media_type', 'DVD')
+              multi_variant.set_option_value('media_type', 'DVD')
             }.to_not change(multi_variant.option_values, :count)
 
             expect {
@@ -104,7 +104,7 @@ module Spree
     context "price parsing" do
       before(:each) do
         I18n.locale = I18n.default_locale
-        I18n.backend.store_translations(:de, { :number => { :currency => { :format => { :delimiter => '.', :separator => ',' } } } })
+        I18n.backend.store_translations(:de, { number: { currency: { format: { delimiter: '.', separator: ',' } } } })
       end
 
       after do
@@ -187,7 +187,7 @@ module Spree
 
     describe '.price_in' do
       before do
-        variant.prices << create(:price, :variant => variant, :currency => "EUR", :amount => 33.33)
+        variant.prices << create(:price, variant: variant, currency: "EUR", amount: 33.33)
       end
       subject { variant.price_in(currency).display_amount }
 
@@ -218,7 +218,7 @@ module Spree
 
     describe '.amount_in' do
       before do
-        variant.prices << create(:price, :variant => variant, :currency => "EUR", :amount => 33.33)
+        variant.prices << create(:price, variant: variant, currency: "EUR", amount: 33.33)
       end
 
       subject { variant.amount_in(currency) }
@@ -251,8 +251,8 @@ module Spree
     # Regression test for #2432
     describe 'options_text' do
       before do
-        option_type = double("OptionType", :presentation => "Foo")
-        option_values = [double("OptionValue", :option_type => option_type, :presentation => "bar")]
+        option_type = double("OptionType", presentation: "Foo")
+        option_values = [double("OptionValue", option_type: option_type, presentation: "bar")]
         variant.stub(:option_values).and_return(option_values)
       end
 
@@ -344,7 +344,7 @@ module Spree
         expect(variant.total_on_hand).to eq(Spree::Stock::Quantifier.new(variant).total_on_hand)
       end
     end
-    
+
     describe "double loading" do
       # app/models/spree/variant_decorator.rb  may be double-loaded in delayed job environment,
       # so we need to be able to do so without error.

From b68c5ee0f9293021847b3302f724ec745be17e1e Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 7 Aug 2020 20:57:43 +0100
Subject: [PATCH 06/17] Remove dead code (prototypes) and fix specs

---
 app/models/spree/option_type.rb            |   1 -
 app/models/spree/product.rb                |  28 ----
 config/locales/en.yml                      |   1 -
 spec/models/spree/product_property_spec.rb |   2 +-
 spec/models/spree/product_spec.rb          | 166 +++++----------------
 spec/models/spree/variant_spec.rb          |  64 ++++----
 6 files changed, 67 insertions(+), 195 deletions(-)

diff --git a/app/models/spree/option_type.rb b/app/models/spree/option_type.rb
index 3bfebdc6be..f0260a13d2 100644
--- a/app/models/spree/option_type.rb
+++ b/app/models/spree/option_type.rb
@@ -5,7 +5,6 @@ module Spree
     has_many :products, through: :product_option_types
     has_many :option_values, -> { order(:position) }, dependent: :destroy
     has_many :product_option_types, dependent: :destroy
-    has_and_belongs_to_many :prototypes, join_table: 'spree_option_types_prototypes'
 
     validates :name, :presentation, presence: true
     default_scope -> { order("#{table_name}.position") }
diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index 76ca236351..f4aa0bd8eb 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -75,7 +75,6 @@ module Spree
     delegate :images_attributes=, :display_as=, to: :master
 
     after_create :set_master_variant_defaults
-    after_create :add_properties_and_option_types_from_prototype
     after_create :build_variants_from_option_values_hash, if: :option_values_hash
     after_save :save_master
 
@@ -238,12 +237,6 @@ module Spree
       end
     end
 
-    # Adding properties and option types on creation based on a chosen prototype
-    attr_reader :prototype_id
-    def prototype_id=(value)
-      @prototype_id = value.to_i
-    end
-
     # Ensures option_types and product_option_types exist for keys in option_values_hash
     def ensure_option_types_exist_for_values_hash
       return if option_values_hash.nil?
@@ -265,7 +258,6 @@ module Spree
 
     # Called by Spree::Product::duplicate before saving.
     def duplicate_extra(_parent)
-      # Spree sets the SKU to "COPY OF #{parent sku}".
       master.sku = ''
     end
 
@@ -296,17 +288,6 @@ module Spree
       }.inject(:or)
     end
 
-    # Suitable for displaying only variants that has at least one option value.
-    # There may be scenarios where an option type is removed and along with it
-    # all option values. At that point all variants associated with only those
-    # values should not be displayed to frontend users. Otherwise it breaks the
-    # idea of having variants
-    def variants_and_option_values(current_currency = nil)
-      variants.includes(:option_values).active(current_currency).select do |variant|
-        variant.option_values.any?
-      end
-    end
-
     def empty_option_values?
       options.empty? || options.any? do |opt|
         opt.option_type.option_values.empty?
@@ -424,15 +405,6 @@ module Spree
       save
     end
 
-    def add_properties_and_option_types_from_prototype
-      return unless prototype_id && prototype = Spree::Prototype.find_by(id: prototype_id)
-
-      prototype.properties.each do |property|
-        product_properties.create(property: property)
-      end
-      self.option_types = prototype.option_types
-    end
-
     # ensures the master variant is flagged as such
     def set_master_variant_defaults
       master.is_master = true
diff --git a/config/locales/en.yml b/config/locales/en.yml
index cf4260e055..0074661090 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -3232,7 +3232,6 @@ See the %{link} to find out more about %{sitename}'s features and to start using
         index:
           inherits_properties_checkbox_hint: "Inherit properties from %{supplier}? (unless overridden above)"
           add_product_properties: "Add Product Properties"
-          select_from_prototype: "Select From Prototype"
       properties:
         index:
           properties: "Properties"
diff --git a/spec/models/spree/product_property_spec.rb b/spec/models/spree/product_property_spec.rb
index 34bd96a314..4109db924e 100644
--- a/spec/models/spree/product_property_spec.rb
+++ b/spec/models/spree/product_property_spec.rb
@@ -7,7 +7,7 @@ describe Spree::ProductProperty do
     it "should validate length of value" do
       pp = create(:product_property)
       pp.value = "x" * 256
-      pp.should_not be_valid
+      expect(pp).to_not be_valid
     end
   end
 end
diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb
index 6b12dead78..8c468e8456 100644
--- a/spec/models/spree/product_spec.rb
+++ b/spec/models/spree/product_spec.rb
@@ -12,10 +12,9 @@ module Spree
 
         it 'duplicates product' do
           clone = product.duplicate
-          clone.name.should == 'COPY OF ' + product.name
-          clone.master.sku.should == 'COPY OF ' + product.master.sku
-          clone.taxons.should == product.taxons
-          clone.images.size.should == product.images.size
+          expect(clone.name).to eq 'COPY OF ' + product.name
+          expect(clone.master.sku).to eq ''
+          expect(clone.images.size).to eq product.images.size
         end
 
         it 'calls #duplicate_extra' do
@@ -26,7 +25,7 @@ module Spree
           end
 
           clone = product.duplicate
-          clone.name.should == product.name.reverse
+          expect(clone.name).to eq product.name.reverse
         end
       end
 
@@ -34,8 +33,8 @@ module Spree
         context "#destroy" do
           it "should set deleted_at value" do
             product.destroy
-            product.deleted_at.should_not be_nil
-            product.master.deleted_at.should_not be_nil
+            expect(product.deleted_at).to_not be_nil
+            expect(product.master.deleted_at).to_not be_nil
           end
         end
       end
@@ -48,8 +47,8 @@ module Spree
         context "#destroy" do
           it "should set deleted_at value" do
             product.destroy
-            product.deleted_at.should_not be_nil
-            product.variants_including_master.all? { |v| !v.deleted_at.nil? }.should be_true
+            expect(product.deleted_at).to_not be_nil
+            expect(product.variants_including_master.all? { |v| !v.deleted_at.nil? }).to be_truthy
           end
         end
       end
@@ -58,7 +57,7 @@ module Spree
         # Regression test for Spree #1173
         it 'strips non-price characters' do
           product.price = "$10"
-          product.price.should == 10.0
+          expect(product.price).to eq 10.0
         end
       end
 
@@ -69,7 +68,7 @@ module Spree
           before { Spree::Config[:display_currency] = true }
 
           it "shows the currency" do
-            product.display_price.to_s.should == "$10.55 USD"
+            expect(product.display_price.to_s).to eq "$10.55 #{Spree::Config[:currency]}"
           end
         end
 
@@ -77,7 +76,7 @@ module Spree
           before { Spree::Config[:display_currency] = false }
 
           it "does not include the currency" do
-            product.display_price.to_s.should == "$10.55"
+            expect(product.display_price.to_s).to eq "$10.55"
           end
         end
 
@@ -89,7 +88,7 @@ module Spree
           end
 
           it "displays the currency in yen" do
-            product.display_price.to_s.should == "¥11"
+            expect(product.display_price.to_s).to eq "¥11"
           end
         end
       end
@@ -97,53 +96,31 @@ module Spree
       context "#available?" do
         it "should be available if date is in the past" do
           product.available_on = 1.day.ago
-          product.should be_available
+          expect(product).to be_available
         end
 
         it "should not be available if date is nil or in the future" do
           product.available_on = nil
-          product.should_not be_available
+          expect(product).to_not be_available
 
           product.available_on = 1.day.from_now
-          product.should_not be_available
-        end
-      end
-
-      context "variants_and_option_values" do
-        let!(:high) { create(:variant, product: product) }
-        let!(:low) { create(:variant, product: product) }
-
-        before { high.option_values.destroy_all }
-
-        it "returns only variants with option values" do
-          product.variants_and_option_values.should == [low]
+          expect(product).to_not be_available
         end
       end
 
       describe 'Variants sorting' do
         context 'without master variant' do
           it 'sorts variants by position' do
-            product.variants.to_sql.should match(/ORDER BY (\`|\")spree_variants(\`|\").position ASC/)
+            expect(product.variants.to_sql).to match(/ORDER BY (\`|\")spree_variants(\`|\").position ASC/)
           end
         end
 
         context 'with master variant' do
           it 'sorts variants by position' do
-            product.variants_including_master.to_sql.should match(/ORDER BY (\`|\")spree_variants(\`|\").position ASC/)
+            expect(product.variants_including_master.to_sql).to match(/ORDER BY (\`|\")spree_variants(\`|\").position ASC/)
           end
         end
       end
-
-      context "has stock movements" do
-        let(:product) { create(:product) }
-        let(:variant) { product.master }
-        let(:stock_item) { variant.stock_items.first }
-
-        it "doesnt raise ReadOnlyRecord error" do
-          Spree::StockMovement.create!(stock_item: stock_item, quantity: 1)
-          expect { product.destroy }.not_to raise_error
-        end
-      end
     end
 
     context "permalink" do
@@ -154,14 +131,14 @@ module Spree
         before { product.valid? }
 
         it "increments name" do
-          product.permalink.should == 'foo-1'
+          expect(product.permalink).to eq 'foo-1'
         end
       end
 
       context "build permalink with quotes" do
-        it "saves quotes" do
+        it "does not save quotes" do
           product = create(:product, name: "Joe's", permalink: "joe's")
-          product.permalink.should == "joe's"
+          expect(product.permalink).to eq "joe-s"
         end
       end
 
@@ -173,14 +150,14 @@ module Spree
         it "cannot create another product with the same permalink" do
           pending '[Spree build] Failing spec'
           @product2 = create(:product, name: 'foo')
-          lambda do
+          expect do
             @product2.update(permalink: @product1.permalink)
-          end.should raise_error(ActiveRecord::RecordNotUnique)
+          end.to raise_error(ActiveRecord::RecordNotUnique)
         end
       end
 
       it "supports Chinese" do
-        create(:product, name: "你好").permalink.should == "ni-hao"
+        expect(create(:product, name: "你好").permalink).to eq "ni-hao"
       end
 
       context "manual permalink override" do
@@ -189,10 +166,10 @@ module Spree
         it "calling save_permalink with a parameter" do
           product.name = "foobar"
           product.save
-          product.permalink.should == "foo"
+          expect(product.permalink).to eq "foo"
 
           product.save_permalink(product.name)
-          product.permalink.should == "foobar"
+          expect(product.permalink).to eq "foobar"
         end
       end
 
@@ -200,11 +177,11 @@ module Spree
         let(:product) { create(:product, name: "foo") }
 
         it "should create product with same permalink from name like deleted product" do
-          product.permalink.should == "foo"
+          expect(product.permalink).to eq "foo"
           product.destroy
 
           new_product = create(:product, name: "foo")
-          new_product.permalink.should == "foo"
+          expect(new_product.permalink).to eq "foo"
         end
       end
     end
@@ -214,10 +191,10 @@ module Spree
 
       it "should properly assign properties" do
         product.set_property('the_prop', 'value1')
-        product.property('the_prop').should == 'value1'
+        expect(product.property('the_prop')).to eq 'value1'
 
         product.set_property('the_prop', 'value2')
-        product.property('the_prop').should == 'value2'
+        expect(product.property('the_prop')).to eq 'value2'
       end
 
       it "should not create duplicate properties when set_property is called" do
@@ -231,7 +208,7 @@ module Spree
           product.set_property('the_prop_new', 'value')
           product.save
           product.reload
-          product.property('the_prop_new').should == 'value'
+          expect(product.property('the_prop_new')).to eq 'value'
         }.to change { product.properties.length }.by(1)
       end
 
@@ -240,83 +217,8 @@ module Spree
         Spree::Property.where(name: 'foo').first_or_create!(presentation: "Foo's Presentation Name")
         product.set_property('foo', 'value1')
         product.set_property('bar', 'value2')
-        Spree::Property.where(name: 'foo').first.presentation.should == "Foo's Presentation Name"
-        Spree::Property.where(name: 'bar').first.presentation.should == "bar"
-      end
-    end
-
-    context '#create' do
-      let!(:prototype) { create(:prototype) }
-      let!(:product) { Spree::Product.new(name: "Foo", price: 1.99, shipping_category_id: create(:shipping_category).id) }
-
-      before { product.prototype_id = prototype.id }
-
-      context "when prototype is supplied" do
-        it "should create properties based on the prototype" do
-          product.save
-          product.properties.count.should == 1
-        end
-      end
-
-      context "when prototype with option types is supplied" do
-        def build_option_type_with_values(name, values)
-          ot = create(:option_type, name: name)
-          values.each do |val|
-            ot.option_values.create(name: val.downcase, presentation: val)
-          end
-          ot
-        end
-
-        let(:prototype) do
-          size = build_option_type_with_values("size", %w(Small Medium Large))
-          create(:prototype, name: "Size", option_types: [size])
-        end
-
-        let(:option_values_hash) do
-          hash = {}
-          prototype.option_types.each do |i|
-            hash[i.id.to_s] = i.option_value_ids
-          end
-          hash
-        end
-
-        it "should create option types based on the prototype" do
-          product.save
-          product.option_type_ids.length.should == 1
-          product.option_type_ids.should == prototype.option_type_ids
-        end
-
-        it "should create product option types based on the prototype" do
-          product.save
-          product.product_option_types.pluck(:option_type_id).should == prototype.option_type_ids
-        end
-
-        it "should create variants from an option values hash with one option type" do
-          product.option_values_hash = option_values_hash
-          product.save
-          product.variants.length.should == 3
-        end
-
-        it "should still create variants when option_values_hash is given but prototype id is nil" do
-          product.option_values_hash = option_values_hash
-          product.prototype_id = nil
-          product.save
-          product.option_type_ids.length.should == 1
-          product.option_type_ids.should == prototype.option_type_ids
-          product.variants.length.should == 3
-        end
-
-        it "should create variants from an option values hash with multiple option types" do
-          color = build_option_type_with_values("color", %w(Red Green Blue))
-          logo  = build_option_type_with_values("logo", %w(Ruby Rails Nginx))
-          option_values_hash[color.id.to_s] = color.option_value_ids
-          option_values_hash[logo.id.to_s] = logo.option_value_ids
-          product.option_values_hash = option_values_hash
-          product.save
-          product.reload
-          product.option_type_ids.length.should == 3
-          product.variants.length.should == 27
-        end
+        expect(Spree::Property.where(name: 'foo').first.presentation).to eq "Foo's Presentation Name"
+        expect(Spree::Property.where(name: 'bar').first.presentation).to eq "bar"
       end
     end
 
@@ -336,13 +238,13 @@ module Spree
     describe '#total_on_hand' do
       it 'should be infinite if track_inventory_levels is false' do
         Spree::Config[:track_inventory_levels] = false
-        build(:product).total_on_hand.should eql(Float::INFINITY)
+        expect(build(:product).total_on_hand).to eql(Float::INFINITY)
       end
 
       it 'should return sum of stock items count_on_hand' do
         product = build(:product)
         product.stub stock_items: [double(Spree::StockItem, count_on_hand: 5)]
-        product.total_on_hand.should eql(5)
+        expect(product.total_on_hand).to eql(5)
       end
     end
 
diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb
index 4d1aafc4d5..7a661c256b 100644
--- a/spec/models/spree/variant_spec.rb
+++ b/spec/models/spree/variant_spec.rb
@@ -8,12 +8,12 @@ module Spree
     context "validations" do
       it "should validate price is greater than 0" do
         variant.price = -1
-        variant.should be_invalid
+        expect(variant).to be_invalid
       end
 
       it "should validate price is 0" do
         variant.price = 0
-        variant.should be_valid
+        expect(variant).to be_valid
       end
     end
 
@@ -45,13 +45,13 @@ module Spree
         let(:multi_variant) { @multi_variant }
 
         it "should set option value" do
-          multi_variant.option_value('media_type').should be_nil
+          expect(multi_variant.option_value('media_type')).to be_nil
 
           multi_variant.set_option_value('media_type', 'DVD')
-          multi_variant.option_value('media_type').should == 'DVD'
+          expect(multi_variant.option_value('media_type')).to == 'DVD'
 
           multi_variant.set_option_value('media_type', 'CD')
-          multi_variant.option_value('media_type').should == 'CD'
+          expect(multi_variant.option_value('media_type')).to == 'CD'
         end
 
         it "should not duplicate associated option values when set multiple times" do
@@ -77,13 +77,13 @@ module Spree
           let(:multi_variant) { @multi_variant }
 
           it "should set option value" do
-            multi_variant.option_value('media_type').should be_nil
+            expect(multi_variant.option_value('media_type')).to be_nil
 
             multi_variant.set_option_value('media_type', 'DVD')
-            multi_variant.option_value('media_type').should == 'DVD'
+            expect(multi_variant.option_value('media_type')).to == 'DVD'
 
             multi_variant.set_option_value('media_type', 'CD')
-            multi_variant.option_value('media_type').should == 'CD'
+            expect(multi_variant.option_value('media_type')).to == 'CD'
           end
 
           it "should not duplicate associated option values when set multiple times" do
@@ -115,7 +115,7 @@ module Spree
         context "with decimal point" do
           it "captures the proper amount for a formatted price" do
             variant.price = '1,599.99'
-            variant.price.should == 1599.99
+            expect(variant.price).to == 1599.99
           end
         end
 
@@ -123,7 +123,7 @@ module Spree
           it "captures the proper amount for a formatted price" do
             I18n.locale = :de
             variant.price = '1.599,99'
-            variant.price.should == 1599.99
+            expect(variant.price).to == 1599.99
           end
         end
 
@@ -131,7 +131,7 @@ module Spree
           it "uses the price as is" do
             I18n.locale = :de
             variant.price = 1599.99
-            variant.price.should == 1599.99
+            expect(variant.price).to == 1599.99
           end
         end
       end
@@ -140,7 +140,7 @@ module Spree
         context "with decimal point" do
           it "captures the proper amount for a formatted price" do
             variant.cost_price = '1,599.99'
-            variant.cost_price.should == 1599.99
+            expect(variant.cost_price).to == 1599.99
           end
         end
 
@@ -148,7 +148,7 @@ module Spree
           it "captures the proper amount for a formatted price" do
             I18n.locale = :de
             variant.cost_price = '1.599,99'
-            variant.cost_price.should == 1599.99
+            expect(variant.cost_price).to == 1599.99
           end
         end
 
@@ -156,7 +156,7 @@ module Spree
           it "uses the price as is" do
             I18n.locale = :de
             variant.cost_price = 1599.99
-            variant.cost_price.should == 1599.99
+            expect(variant.cost_price).to == 1599.99
           end
         end
       end
@@ -164,14 +164,14 @@ module Spree
 
     context "#currency" do
       it "returns the globally configured currency" do
-        variant.currency.should == "USD"
+        expect(variant.currency).to == "USD"
       end
     end
 
     context "#display_amount" do
       it "returns a Spree::Money" do
         variant.price = 21.22
-        variant.display_amount.to_s.should == "$21.22"
+        expect(variant.display_amount.to_s).to == "$21.22"
       end
     end
 
@@ -180,7 +180,7 @@ module Spree
         before { variant.cost_currency = nil }
         it "populates cost currency with the default value on save" do
           variant.save!
-          variant.cost_currency.should == "USD"
+          expect(variant.cost_currency).to == "USD"
         end
       end
     end
@@ -195,7 +195,7 @@ module Spree
         let(:currency) { nil }
 
         it "returns 0" do
-          subject.to_s.should == "$0.00"
+          expect(subject.to_s).to == "$0.00"
         end
       end
 
@@ -203,7 +203,7 @@ module Spree
         let(:currency) { 'EUR' }
 
         it "returns the value in the EUR" do
-          subject.to_s.should == "€33.33"
+          expect(subject.to_s).to == "€33.33"
         end
       end
 
@@ -211,7 +211,7 @@ module Spree
         let(:currency) { 'USD' }
 
         it "returns the value in the USD" do
-          subject.to_s.should == "$19.99"
+          expect(subject.to_s).to == "$19.99"
         end
       end
     end
@@ -227,7 +227,7 @@ module Spree
         let(:currency) { nil }
 
         it "returns nil" do
-          subject.should be_nil
+          expect(subject).to be_nil
         end
       end
 
@@ -235,7 +235,7 @@ module Spree
         let(:currency) { 'EUR' }
 
         it "returns the value in the EUR" do
-          subject.should == 33.33
+          expect(subject).to == 33.33
         end
       end
 
@@ -243,7 +243,7 @@ module Spree
         let(:currency) { 'USD' }
 
         it "returns the value in the USD" do
-          subject.should == 19.99
+          expect(subject).to == 19.99
         end
       end
     end
@@ -267,7 +267,7 @@ module Spree
     describe "set_position" do
       it "sets variant position after creation" do
         variant = create(:variant)
-        variant.position.should_not be_nil
+        expect(variant.position).to_not be_nil
       end
     end
 
@@ -287,7 +287,7 @@ module Spree
           end
 
           it 'returns true if stock_items in stock' do
-            variant.in_stock?.should be_true
+            expect(variant.in_stock?).to be_truthy
           end
         end
 
@@ -298,7 +298,7 @@ module Spree
           end
 
           it 'return false if stock_items out of stock' do
-            variant.in_stock?.should be_false
+            expect(variant.in_stock?).to be_falsy
           end
         end
 
@@ -308,10 +308,10 @@ module Spree
           end
 
           it 'returns correctt value' do
-            variant.in_stock?.should be_true
-            variant.in_stock?(2).should be_true
-            variant.in_stock?(10).should be_true
-            variant.in_stock?(11).should be_false
+            expect(variant.in_stock?).to be_truthy
+            expect(variant.in_stock?(2)).to be_truthy
+            expect(variant.in_stock?(10)).to be_truthy
+            expect(variant.in_stock?(11)).to be_falsy
           end
         end
       end
@@ -327,7 +327,7 @@ module Spree
           end
 
           it 'returns true if stock_items in stock' do
-            variant.in_stock?.should be_true
+            expect(variant.in_stock?).to be_truthy
           end
         end
       end
@@ -336,7 +336,7 @@ module Spree
     describe '#total_on_hand' do
       it 'should be infinite if track_inventory_levels is false' do
         Spree::Config[:track_inventory_levels] = false
-        build(:variant).total_on_hand.should eql(Float::INFINITY)
+        expect(build(:variant).total_on_hand).to eql(Float::INFINITY)
       end
 
       it 'should match quantifier total_on_hand' do

From e9f76cb3392a91b08d755d709c709cc86cff1517 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 7 Aug 2020 21:39:14 +0100
Subject: [PATCH 07/17] Remove some dead code (Config.track_inventory_levels),
 remove variant.cost_price= so that localized number is seen, and fix specs

---
 app/models/spree/product.rb       |   6 +-
 app/models/spree/variant.rb       |  22 +----
 spec/models/spree/product_spec.rb |   7 +-
 spec/models/spree/variant_spec.rb | 133 +++++++-----------------------
 4 files changed, 34 insertions(+), 134 deletions(-)

diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index f4aa0bd8eb..612ed9c57f 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -316,11 +316,7 @@ module Spree
     end
 
     def total_on_hand
-      if Spree::Config.track_inventory_levels
-        stock_items.sum(&:count_on_hand)
-      else
-        Float::INFINITY
-      end
+      stock_items.sum(&:count_on_hand)
     end
 
     # Master variant may be deleted (i.e. when the product is deleted)
diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
index 1abf737d60..26a3d0327b 100644
--- a/app/models/spree/variant.rb
+++ b/app/models/spree/variant.rb
@@ -3,6 +3,7 @@
 require 'open_food_network/enterprise_fee_calculator'
 require 'variant_units/variant_and_line_item_naming'
 require 'concerns/variant_stock'
+require 'spree/localized_number'
 
 module Spree
   class Variant < ActiveRecord::Base
@@ -148,10 +149,6 @@ module Spree
                                           select("spree_variants.id"))
     end
 
-    def cost_price=(price)
-      self[:cost_price] = parse_price(price) if price.present?
-    end
-
     # Allow variant to access associated soft-deleted prices.
     def default_price
       Spree::Price.unscoped { super }
@@ -257,21 +254,6 @@ module Spree
 
     private
 
-    # strips all non-price-like characters from the price, taking into account locale settings
-    def parse_price(price)
-      return price unless price.is_a?(String)
-
-      separator, _delimiter = I18n.t([:'number.currency.format.separator',
-                                      :'number.currency.format.delimiter'])
-      non_price_characters = /[^0-9\-#{separator}]/
-      # Strip everything else first
-      price.gsub!(non_price_characters, '')
-      # Then replace the locale-specific decimal separator with the standard separator if necessary
-      price.gsub!(separator, '.') unless separator == '.'
-
-      price.to_d
-    end
-
     # Ensures a new variant takes the product master price when price is not supplied
     def check_price
       if price.nil? && Spree::Config[:require_master_price]
@@ -296,7 +278,7 @@ module Spree
 
     def create_stock_items
       StockLocation.all.find_each do |stock_location|
-        stock_location.propagate_variant(self) if stock_location.propagate_all_variants?
+        stock_location.propagate_variant(self)
       end
     end
 
diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb
index 8c468e8456..76e89b01a0 100644
--- a/spec/models/spree/product_spec.rb
+++ b/spec/models/spree/product_spec.rb
@@ -236,12 +236,7 @@ module Spree
     end
 
     describe '#total_on_hand' do
-      it 'should be infinite if track_inventory_levels is false' do
-        Spree::Config[:track_inventory_levels] = false
-        expect(build(:product).total_on_hand).to eql(Float::INFINITY)
-      end
-
-      it 'should return sum of stock items count_on_hand' do
+      it 'returns sum of stock items count_on_hand' do
         product = build(:product)
         product.stub stock_items: [double(Spree::StockItem, count_on_hand: 5)]
         expect(product.total_on_hand).to eql(5)
diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb
index 7a661c256b..2e7ae925ee 100644
--- a/spec/models/spree/variant_spec.rb
+++ b/spec/models/spree/variant_spec.rb
@@ -1,5 +1,6 @@
 require 'spec_helper'
 require 'variant_units/option_value_namer'
+require 'spree/localized_number'
 
 module Spree
   describe Variant do
@@ -17,24 +18,6 @@ module Spree
       end
     end
 
-    context "after create" do
-      let!(:product) { create(:product) }
-
-      it "propagate to stock items" do
-        Spree::StockLocation.any_instance.should_receive(:propagate_variant)
-        product.variants.create(name: "Foobar")
-      end
-
-      context "stock location has disable propagate all variants" do
-        before { Spree::StockLocation.any_instance.stub(propagate_all_variants?: false) }
-
-        it "propagate to stock items" do
-          Spree::StockLocation.any_instance.should_not_receive(:propagate_variant)
-          product.variants.create(name: "Foobar")
-        end
-      end
-    end
-
     context "product has other variants" do
       describe "option value accessors" do
         before {
@@ -48,10 +31,10 @@ module Spree
           expect(multi_variant.option_value('media_type')).to be_nil
 
           multi_variant.set_option_value('media_type', 'DVD')
-          expect(multi_variant.option_value('media_type')).to == 'DVD'
+          expect(multi_variant.option_value('media_type')).to eq 'DVD'
 
           multi_variant.set_option_value('media_type', 'CD')
-          expect(multi_variant.option_value('media_type')).to == 'CD'
+          expect(multi_variant.option_value('media_type')).to eq 'CD'
         end
 
         it "should not duplicate associated option values when set multiple times" do
@@ -80,10 +63,10 @@ module Spree
             expect(multi_variant.option_value('media_type')).to be_nil
 
             multi_variant.set_option_value('media_type', 'DVD')
-            expect(multi_variant.option_value('media_type')).to == 'DVD'
+            expect(multi_variant.option_value('media_type')).to eq 'DVD'
 
             multi_variant.set_option_value('media_type', 'CD')
-            expect(multi_variant.option_value('media_type')).to == 'CD'
+            expect(multi_variant.option_value('media_type')).to eq 'CD'
           end
 
           it "should not duplicate associated option values when set multiple times" do
@@ -115,48 +98,23 @@ module Spree
         context "with decimal point" do
           it "captures the proper amount for a formatted price" do
             variant.price = '1,599.99'
-            expect(variant.price).to == 1599.99
+            expect(variant.price).to eq 1599.99
           end
         end
 
         context "with decimal comma" do
           it "captures the proper amount for a formatted price" do
-            I18n.locale = :de
+            I18n.locale = :es
             variant.price = '1.599,99'
-            expect(variant.price).to == 1599.99
+            expect(variant.price).to eq 1599.99
           end
         end
 
         context "with a numeric price" do
           it "uses the price as is" do
-            I18n.locale = :de
+            I18n.locale = :es
             variant.price = 1599.99
-            expect(variant.price).to == 1599.99
-          end
-        end
-      end
-
-      context "cost_price=" do
-        context "with decimal point" do
-          it "captures the proper amount for a formatted price" do
-            variant.cost_price = '1,599.99'
-            expect(variant.cost_price).to == 1599.99
-          end
-        end
-
-        context "with decimal comma" do
-          it "captures the proper amount for a formatted price" do
-            I18n.locale = :de
-            variant.cost_price = '1.599,99'
-            expect(variant.cost_price).to == 1599.99
-          end
-        end
-
-        context "with a numeric price" do
-          it "uses the price as is" do
-            I18n.locale = :de
-            variant.cost_price = 1599.99
-            expect(variant.cost_price).to == 1599.99
+            expect(variant.price).to eq 1599.99
           end
         end
       end
@@ -164,23 +122,24 @@ module Spree
 
     context "#currency" do
       it "returns the globally configured currency" do
-        expect(variant.currency).to == "USD"
+        expect(variant.currency).to eq Spree::Config[:currency]
       end
     end
 
     context "#display_amount" do
       it "returns a Spree::Money" do
         variant.price = 21.22
-        expect(variant.display_amount.to_s).to == "$21.22"
+        expect(variant.display_amount.to_s).to eq "$21.22"
       end
     end
 
     context "#cost_currency" do
       context "when cost currency is nil" do
         before { variant.cost_currency = nil }
+
         it "populates cost currency with the default value on save" do
           variant.save!
-          expect(variant.cost_currency).to == "USD"
+          expect(variant.cost_currency).to eq Spree::Config[:currency]
         end
       end
     end
@@ -195,23 +154,23 @@ module Spree
         let(:currency) { nil }
 
         it "returns 0" do
-          expect(subject.to_s).to == "$0.00"
+          expect(subject.to_s).to eq "$0.00"
         end
       end
 
       context "when currency is EUR" do
         let(:currency) { 'EUR' }
 
-        it "returns the value in the EUR" do
-          expect(subject.to_s).to == "€33.33"
+        it "returns the value in EUR" do
+          expect(subject.to_s).to eq "€33.33"
         end
       end
 
-      context "when currency is USD" do
-        let(:currency) { 'USD' }
+      context "when currency is AUD" do
+        let(:currency) { 'AUD' }
 
-        it "returns the value in the USD" do
-          expect(subject.to_s).to == "$19.99"
+        it "returns the value in AUD" do
+          expect(subject.to_s).to eq "$19.99"
         end
       end
     end
@@ -234,35 +193,20 @@ module Spree
       context "when currency is EUR" do
         let(:currency) { 'EUR' }
 
-        it "returns the value in the EUR" do
-          expect(subject).to == 33.33
+        it "returns the value in EUR" do
+          expect(subject).to eq 33.33
         end
       end
 
-      context "when currency is USD" do
-        let(:currency) { 'USD' }
+      context "when currency is AUD" do
+        let(:currency) { 'AUD' }
 
-        it "returns the value in the USD" do
-          expect(subject).to == 19.99
+        it "returns the value in AUD" do
+          expect(subject).to eq 19.99
         end
       end
     end
 
-    # Regression test for #2432
-    describe 'options_text' do
-      before do
-        option_type = double("OptionType", presentation: "Foo")
-        option_values = [double("OptionValue", option_type: option_type, presentation: "bar")]
-        variant.stub(:option_values).and_return(option_values)
-      end
-
-      it "orders options correctly" do
-        variant.option_values.should_receive(:joins).with(:option_type).and_return(scope = double)
-        scope.should_receive(:order).with('spree_option_types.position asc').and_return(variant.option_values)
-        variant.options_text
-      end
-    end
-
     # Regression test for #2744
     describe "set_position" do
       it "sets variant position after creation" do
@@ -272,10 +216,6 @@ module Spree
     end
 
     describe '#in_stock?' do
-      before do
-        Spree::Config.track_inventory_levels = true
-      end
-
       context 'when stock_items are not backorderable' do
         before do
           Spree::StockItem.any_instance.stub(backorderable: false)
@@ -307,7 +247,7 @@ module Spree
             variant.stock_items.first.update_attribute(:count_on_hand, 10)
           end
 
-          it 'returns correctt value' do
+          it 'returns correct value' do
             expect(variant.in_stock?).to be_truthy
             expect(variant.in_stock?(2)).to be_truthy
             expect(variant.in_stock?(10)).to be_truthy
@@ -318,7 +258,7 @@ module Spree
 
       context 'when stock_items are backorderable' do
         before do
-          Spree::StockItem.any_instance.stub(backorderable: true)
+          Spree::StockItem.any_instance.stub(backorderable?: true)
         end
 
         context 'when stock_items out of stock' do
@@ -334,25 +274,12 @@ module Spree
     end
 
     describe '#total_on_hand' do
-      it 'should be infinite if track_inventory_levels is false' do
-        Spree::Config[:track_inventory_levels] = false
-        expect(build(:variant).total_on_hand).to eql(Float::INFINITY)
-      end
-
-      it 'should match quantifier total_on_hand' do
+      it 'matches quantifier total_on_hand' do
         variant = build(:variant)
         expect(variant.total_on_hand).to eq(Spree::Stock::Quantifier.new(variant).total_on_hand)
       end
     end
 
-    describe "double loading" do
-      # app/models/spree/variant_decorator.rb  may be double-loaded in delayed job environment,
-      # so we need to be able to do so without error.
-      it "succeeds without error" do
-        load "#{Rails.root}/app/models/spree/variant_decorator.rb"
-      end
-    end
-
     describe "scopes" do
       describe "finding variants in a distributor" do
         let!(:d1) { create(:distributor_enterprise) }

From d4e4669e49bc02a28dfd437387eba399035c1503 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 7 Aug 2020 21:42:17 +0100
Subject: [PATCH 08/17] Run transpec

---
 spec/models/spree/product_spec.rb |  4 ++--
 spec/models/spree/variant_spec.rb | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb
index 76e89b01a0..3b124574e3 100644
--- a/spec/models/spree/product_spec.rb
+++ b/spec/models/spree/product_spec.rb
@@ -7,7 +7,7 @@ module Spree
 
       context '#duplicate' do
         before do
-          product.stub taxons: [create(:taxon)]
+          allow(product).to receive_messages taxons: [create(:taxon)]
         end
 
         it 'duplicates product' do
@@ -238,7 +238,7 @@ module Spree
     describe '#total_on_hand' do
       it 'returns sum of stock items count_on_hand' do
         product = build(:product)
-        product.stub stock_items: [double(Spree::StockItem, count_on_hand: 5)]
+        allow(product).to receive_messages stock_items: [double(Spree::StockItem, count_on_hand: 5)]
         expect(product.total_on_hand).to eql(5)
       end
     end
diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb
index 2e7ae925ee..0462ea9e61 100644
--- a/spec/models/spree/variant_spec.rb
+++ b/spec/models/spree/variant_spec.rb
@@ -218,12 +218,12 @@ module Spree
     describe '#in_stock?' do
       context 'when stock_items are not backorderable' do
         before do
-          Spree::StockItem.any_instance.stub(backorderable: false)
+          allow_any_instance_of(Spree::StockItem).to receive_messages(backorderable: false)
         end
 
         context 'when stock_items in stock' do
           before do
-            Spree::StockItem.any_instance.stub(count_on_hand: 10)
+            allow_any_instance_of(Spree::StockItem).to receive_messages(count_on_hand: 10)
           end
 
           it 'returns true if stock_items in stock' do
@@ -233,8 +233,8 @@ module Spree
 
         context 'when stock_items out of stock' do
           before do
-            Spree::StockItem.any_instance.stub(backorderable: false)
-            Spree::StockItem.any_instance.stub(count_on_hand: 0)
+            allow_any_instance_of(Spree::StockItem).to receive_messages(backorderable: false)
+            allow_any_instance_of(Spree::StockItem).to receive_messages(count_on_hand: 0)
           end
 
           it 'return false if stock_items out of stock' do
@@ -258,12 +258,12 @@ module Spree
 
       context 'when stock_items are backorderable' do
         before do
-          Spree::StockItem.any_instance.stub(backorderable?: true)
+          allow_any_instance_of(Spree::StockItem).to receive_messages(backorderable?: true)
         end
 
         context 'when stock_items out of stock' do
           before do
-            Spree::StockItem.any_instance.stub(count_on_hand: 0)
+            allow_any_instance_of(Spree::StockItem).to receive_messages(count_on_hand: 0)
           end
 
           it 'returns true if stock_items in stock' do

From cf7d8067df3addbf4ad2141d074d65488898d2d6 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Mon, 31 Aug 2020 12:02:33 +0100
Subject: [PATCH 09/17] Fix easy rubocop issues

---
 app/models/concerns/product_stock.rb      |  4 ++--
 app/models/spree/option_type.rb           |  6 +++++-
 app/models/spree/price.rb                 |  2 +-
 app/models/spree/product.rb               | 12 ++++++++----
 app/models/spree/variant.rb               |  7 ++++---
 lib/spree/product_duplicator.rb           |  2 +-
 spec/lib/spree/product_duplicator_spec.rb |  2 +-
 7 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/app/models/concerns/product_stock.rb b/app/models/concerns/product_stock.rb
index 4f01b3069f..60e98d7791 100644
--- a/app/models/concerns/product_stock.rb
+++ b/app/models/concerns/product_stock.rb
@@ -4,7 +4,7 @@ module ProductStock
   extend ActiveSupport::Concern
 
   def on_demand
-    if has_variants?
+    if variants?
       raise 'Cannot determine product on_demand value of product with multiple variants' if variants.size > 1
 
       variants.first.on_demand
@@ -14,7 +14,7 @@ module ProductStock
   end
 
   def on_hand
-    if has_variants?
+    if variants?
       variants.map(&:on_hand).reduce(:+)
     else
       master.on_hand
diff --git a/app/models/spree/option_type.rb b/app/models/spree/option_type.rb
index f0260a13d2..91b65c97bb 100644
--- a/app/models/spree/option_type.rb
+++ b/app/models/spree/option_type.rb
@@ -9,6 +9,10 @@ module Spree
     validates :name, :presentation, presence: true
     default_scope -> { order("#{table_name}.position") }
 
-    accepts_nested_attributes_for :option_values, reject_if: lambda { |ov| ov[:name].blank? || ov[:presentation].blank? }, allow_destroy: true
+    accepts_nested_attributes_for :option_values,
+                                  reject_if: lambda { |ov|
+                                    ov[:name].blank? || ov[:presentation].blank?
+                                  },
+                                  allow_destroy: true
   end
 end
diff --git a/app/models/spree/price.rb b/app/models/spree/price.rb
index d0dab12359..47c8eb2134 100644
--- a/app/models/spree/price.rb
+++ b/app/models/spree/price.rb
@@ -15,7 +15,7 @@ module Spree
     alias :display_price :display_amount
 
     def money
-      Spree::Money.new(amount || 0, { currency: currency })
+      Spree::Money.new(amount || 0, currency: currency)
     end
 
     def price
diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index 612ed9c57f..5ea1ad0c59 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -68,7 +68,7 @@ module Spree
     has_many :stock_items, through: :variants
 
     delegate_belongs_to :master, :sku, :price, :currency, :display_amount, :display_price, :weight,
-                        :height, :width, :depth, :is_master, :has_default_price?, :cost_currency,
+                        :height, :width, :depth, :is_master, :default_price?, :cost_currency,
                         :price_in, :amount_in, :unit_value, :unit_description
     delegate_belongs_to :master, :cost_price if Variant.table_exists? &&
                                                 Variant.column_names.include?('cost_price')
@@ -150,8 +150,12 @@ module Spree
     }
 
     scope :visible_for, lambda { |enterprise|
-      joins('LEFT OUTER JOIN spree_variants AS o_spree_variants ON (o_spree_variants.product_id = spree_products.id)').
-        joins('LEFT OUTER JOIN inventory_items AS o_inventory_items ON (o_spree_variants.id = o_inventory_items.variant_id)').
+      joins('
+        LEFT OUTER JOIN spree_variants AS o_spree_variants
+          ON (o_spree_variants.product_id = spree_products.id)').
+        joins('
+          LEFT OUTER JOIN inventory_items AS o_inventory_items
+            ON (o_spree_variants.id = o_inventory_items.variant_id)').
         where('o_inventory_items.enterprise_id = (?) AND visible = (?)', enterprise, true)
     }
 
@@ -225,7 +229,7 @@ module Spree
     end
 
     # the master variant is not a member of the variants array
-    def has_variants?
+    def variants?
       variants.any?
     end
 
diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
index 26a3d0327b..8fa350444b 100644
--- a/app/models/spree/variant.rb
+++ b/app/models/spree/variant.rb
@@ -39,7 +39,8 @@ module Spree
     has_many :prices,
              class_name: 'Spree::Price',
              dependent: :destroy
-    delegate_belongs_to :default_price, :display_price, :display_amount, :price, :price=, :currency if Spree::Price.table_exists?
+    delegate_belongs_to :default_price, :display_price, :display_amount,
+                        :price, :price=, :currency
 
     has_many :exchange_variants
     has_many :exchanges, through: :exchange_variants
@@ -52,7 +53,7 @@ module Spree
     validates :price, numericality: { greater_than_or_equal_to: 0 },
                       presence: true,
                       if: proc { Spree::Config[:require_master_price] }
-    validates :cost_price, numericality: { greater_than_or_equal_to: 0, allow_nil: true } if table_exists? && column_names.include?('cost_price')
+    validates :cost_price, numericality: { greater_than_or_equal_to: 0, allow_nil: true }
 
     validates :unit_value, presence: true, if: ->(variant) {
       %w(weight volume).include?(variant.product.andand.variant_unit)
@@ -219,7 +220,7 @@ module Spree
       option_values.detect { |o| o.option_type.name == opt_name }.try(:presentation)
     end
 
-    def has_default_price?
+    def default_price?
       !default_price.nil?
     end
 
diff --git a/lib/spree/product_duplicator.rb b/lib/spree/product_duplicator.rb
index 8866f85fa5..7c27e058d7 100644
--- a/lib/spree/product_duplicator.rb
+++ b/lib/spree/product_duplicator.rb
@@ -12,7 +12,7 @@ module Spree
       new_product = duplicate_product
 
       # don't dup the actual variants, just the characterising types
-      new_product.option_types = product.option_types if product.has_variants?
+      new_product.option_types = product.option_types if product.variants?
 
       # allow site to do some customization
       new_product.__send__(:duplicate_extra, product) if new_product.respond_to?(:duplicate_extra)
diff --git a/spec/lib/spree/product_duplicator_spec.rb b/spec/lib/spree/product_duplicator_spec.rb
index 7978465a5e..0a04ec4161 100644
--- a/spec/lib/spree/product_duplicator_spec.rb
+++ b/spec/lib/spree/product_duplicator_spec.rb
@@ -10,7 +10,7 @@ module Spree
              taxons: [],
              product_properties: [property],
              master: variant,
-             has_variants?: false
+             variants?: false
     end
 
     let(:new_product) do

From 4b8515358cd2b0c4bb4a0a345e4706e697b71d66 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Mon, 31 Aug 2020 12:08:48 +0100
Subject: [PATCH 10/17] Remove reference to FactoryGirl, it's FactoryBot that
 is used in OFN

---
 spec/models/spree/variant_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb
index 0462ea9e61..333b039f39 100644
--- a/spec/models/spree/variant_spec.rb
+++ b/spec/models/spree/variant_spec.rb
@@ -21,7 +21,7 @@ module Spree
     context "product has other variants" do
       describe "option value accessors" do
         before {
-          @multi_variant = FactoryGirl.create :variant, product: variant.product
+          @multi_variant = create(:variant, product: variant.product)
           variant.product.reload
         }
 

From 72a39fdf54ce35fb48dbce0ae00a83f1530efd5b Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Mon, 31 Aug 2020 12:10:49 +0100
Subject: [PATCH 11/17] Add required factory from spree_core

---
 spec/factories/price_factory.rb | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 spec/factories/price_factory.rb

diff --git a/spec/factories/price_factory.rb b/spec/factories/price_factory.rb
new file mode 100644
index 0000000000..fe84e5c8f5
--- /dev/null
+++ b/spec/factories/price_factory.rb
@@ -0,0 +1,7 @@
+FactoryBot.define do
+  factory :price, class: Spree::Price do
+    variant
+    amount 19.99
+    currency 'USD'
+  end
+end

From ba16de6627fcc87cb79ac7b42a01d62545d62fa2 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Tue, 1 Sep 2020 19:23:17 +0100
Subject: [PATCH 12/17] Move product duplicator to lib/spree/core, it's where
 spree_core is currently loading it from

---
 lib/spree/{ => core}/product_duplicator.rb | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename lib/spree/{ => core}/product_duplicator.rb (100%)

diff --git a/lib/spree/product_duplicator.rb b/lib/spree/core/product_duplicator.rb
similarity index 100%
rename from lib/spree/product_duplicator.rb
rename to lib/spree/core/product_duplicator.rb

From 459959c0689fd08f4e5589990d0719130c41b25b Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Thu, 3 Sep 2020 22:58:45 +0100
Subject: [PATCH 13/17] Remove code related to spree promotions

---
 app/models/spree/product.rb | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index 5ea1ad0c59..624cbbbe97 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -40,7 +40,6 @@ module Spree
 
     has_many :classifications, dependent: :delete_all
     has_many :taxons, through: :classifications
-    has_and_belongs_to_many :promotion_rules, join_table: :spree_products_promotion_rules
 
     belongs_to :tax_category, class_name: 'Spree::TaxCategory'
     belongs_to :shipping_category, class_name: 'Spree::ShippingCategory'
@@ -314,11 +313,6 @@ module Spree
       end
     end
 
-    def possible_promotions
-      promotion_ids = promotion_rules.map(&:activator_id).uniq
-      Spree::Promotion.advertised.where(id: promotion_ids).reject(&:expired?)
-    end
-
     def total_on_hand
       stock_items.sum(&:count_on_hand)
     end

From 9d4a15b0e0e0a8759cf117e5319eaca57640af35 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Thu, 3 Sep 2020 20:54:15 +0100
Subject: [PATCH 14/17] Add required product scopes from spree_core
 product_scopes

---
 app/models/spree/product.rb | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index 624cbbbe97..5aeecf9885 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -223,6 +223,14 @@ module Spree
       return where('spree_products.supplier_id IN (?)', [enterprise.id] | permitted_producer_ids)
     }
 
+    scope :active, lambda {
+      where("spree_products.deleted_at IS NULL AND spree_products.available_on <= ?", Time.zone.now)
+    }
+
+    def self.group_by_products_id
+      group(column_names.map { |col_name| "#{table_name}.#{col_name}" })
+    end
+
     def to_param
       permalink.present? ? permalink : (permalink_was || name.to_s.to_url)
     end

From 795b7101ab8718e425d72df3abae379ec5f3e2af Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Sat, 5 Sep 2020 14:57:17 +0100
Subject: [PATCH 15/17] Remove spree requires that are not needed in OFN

---
 app/models/spree/variant.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
index 8fa350444b..9b8fe730ca 100644
--- a/app/models/spree/variant.rb
+++ b/app/models/spree/variant.rb
@@ -299,5 +299,3 @@ module Spree
     end
   end
 end
-
-require_dependency 'spree/variant/scopes'

From 503c17f89655664f6b3c134b5658676cab3dc4d1 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Sat, 26 Sep 2020 08:41:47 +0100
Subject: [PATCH 16/17] Move Spree::ProductDuplicator to Spree::Core namespace

---
 app/models/spree/product.rb                   |   2 +-
 lib/spree/core/product_duplicator.rb          | 100 +++++++++---------
 .../lib/spree/core/product_duplicator_spec.rb |  84 +++++++++++++++
 spec/lib/spree/product_duplicator_spec.rb     |  86 ---------------
 4 files changed, 136 insertions(+), 136 deletions(-)
 create mode 100644 spec/lib/spree/core/product_duplicator_spec.rb
 delete mode 100644 spec/lib/spree/product_duplicator_spec.rb

diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index 5aeecf9885..46d1dd3d7e 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -263,7 +263,7 @@ module Spree
     # for adding products which are closely related to existing ones
     # define "duplicate_extra" for site-specific actions, eg for additional fields
     def duplicate
-      duplicator = ProductDuplicator.new(self)
+      duplicator = Spree::Core::ProductDuplicator.new(self)
       duplicator.duplicate
     end
 
diff --git a/lib/spree/core/product_duplicator.rb b/lib/spree/core/product_duplicator.rb
index 7c27e058d7..c7af50afe6 100644
--- a/lib/spree/core/product_duplicator.rb
+++ b/lib/spree/core/product_duplicator.rb
@@ -1,61 +1,63 @@
 # frozen_string_literal: true
 
 module Spree
-  class ProductDuplicator
-    attr_accessor :product
+  module Core
+    class ProductDuplicator
+      attr_accessor :product
 
-    def initialize(product)
-      @product = product
-    end
-
-    def duplicate
-      new_product = duplicate_product
-
-      # don't dup the actual variants, just the characterising types
-      new_product.option_types = product.option_types if product.variants?
-
-      # allow site to do some customization
-      new_product.__send__(:duplicate_extra, product) if new_product.respond_to?(:duplicate_extra)
-      new_product.save!
-      new_product
-    end
-
-    protected
-
-    def duplicate_product
-      product.dup.tap do |new_product|
-        new_product.name = "COPY OF #{product.name}"
-        new_product.taxons = product.taxons
-        new_product.created_at = nil
-        new_product.deleted_at = nil
-        new_product.updated_at = nil
-        new_product.product_properties = reset_properties
-        new_product.master = duplicate_master
+      def initialize(product)
+        @product = product
       end
-    end
 
-    def duplicate_master
-      master = product.master
-      master.dup.tap do |new_master|
-        new_master.sku = "COPY OF #{master.sku}"
-        new_master.deleted_at = nil
-        new_master.images = master.images.map { |image| duplicate_image image }
-        new_master.price = master.price
-        new_master.currency = master.currency
+      def duplicate
+        new_product = duplicate_product
+
+        # don't dup the actual variants, just the characterising types
+        new_product.option_types = product.option_types if product.variants?
+
+        # allow site to do some customization
+        new_product.__send__(:duplicate_extra, product) if new_product.respond_to?(:duplicate_extra)
+        new_product.save!
+        new_product
       end
-    end
 
-    def duplicate_image(image)
-      new_image = image.dup
-      new_image.assign_attributes(attachment: image.attachment.clone)
-      new_image
-    end
+      protected
 
-    def reset_properties
-      product.product_properties.map do |prop|
-        prop.dup.tap do |new_prop|
-          new_prop.created_at = nil
-          new_prop.updated_at = nil
+      def duplicate_product
+        product.dup.tap do |new_product|
+          new_product.name = "COPY OF #{product.name}"
+          new_product.taxons = product.taxons
+          new_product.created_at = nil
+          new_product.deleted_at = nil
+          new_product.updated_at = nil
+          new_product.product_properties = reset_properties
+          new_product.master = duplicate_master
+        end
+      end
+
+      def duplicate_master
+        master = product.master
+        master.dup.tap do |new_master|
+          new_master.sku = "COPY OF #{master.sku}"
+          new_master.deleted_at = nil
+          new_master.images = master.images.map { |image| duplicate_image image }
+          new_master.price = master.price
+          new_master.currency = master.currency
+        end
+      end
+
+      def duplicate_image(image)
+        new_image = image.dup
+        new_image.assign_attributes(attachment: image.attachment.clone)
+        new_image
+      end
+
+      def reset_properties
+        product.product_properties.map do |prop|
+          prop.dup.tap do |new_prop|
+            new_prop.created_at = nil
+            new_prop.updated_at = nil
+          end
         end
       end
     end
diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb
new file mode 100644
index 0000000000..3f9823f59e
--- /dev/null
+++ b/spec/lib/spree/core/product_duplicator_spec.rb
@@ -0,0 +1,84 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Spree::Core::ProductDuplicator do
+  let(:product) do
+    double 'Product',
+           name: "foo",
+           taxons: [],
+           product_properties: [property],
+           master: variant,
+           variants?: false
+  end
+
+  let(:new_product) do
+    double 'New Product',
+           save!: true
+  end
+
+  let(:property) do
+    double 'Property'
+  end
+
+  let(:new_property) do
+    double 'New Property'
+  end
+
+  let(:variant) do
+    double 'Variant',
+           sku: "12345",
+           price: 19.99,
+           currency: "AUD",
+           images: [image]
+  end
+
+  let(:new_variant) do
+    double 'New Variant',
+           sku: "12345"
+  end
+
+  let(:image) do
+    double 'Image',
+           attachment: double('Attachment')
+  end
+
+  let(:new_image) do
+    double 'New Image'
+  end
+
+  before do
+    expect(product).to receive(:dup).and_return(new_product)
+    expect(variant).to receive(:dup).and_return(new_variant)
+    expect(image).to receive(:dup).and_return(new_image)
+    expect(property).to receive(:dup).and_return(new_property)
+  end
+
+  it "can duplicate a product" do
+    duplicator = Spree::Core::ProductDuplicator.new(product)
+    expect(new_product).to receive(:name=).with("COPY OF foo")
+    expect(new_product).to receive(:taxons=).with([])
+    expect(new_product).to receive(:product_properties=).with([new_property])
+    expect(new_product).to receive(:created_at=).with(nil)
+    expect(new_product).to receive(:updated_at=).with(nil)
+    expect(new_product).to receive(:deleted_at=).with(nil)
+    expect(new_product).to receive(:master=).with(new_variant)
+
+    expect(new_variant).to receive(:sku=).with("COPY OF 12345")
+    expect(new_variant).to receive(:deleted_at=).with(nil)
+    expect(new_variant).to receive(:images=).with([new_image])
+    expect(new_variant).to receive(:price=).with(variant.price)
+    expect(new_variant).to receive(:currency=).with(variant.currency)
+
+    expect(image.attachment).to receive(:clone).and_return(image.attachment)
+
+    expect(new_image).to receive(:assign_attributes).
+      with(attachment: image.attachment).
+      and_return(new_image)
+
+    expect(new_property).to receive(:created_at=).with(nil)
+    expect(new_property).to receive(:updated_at=).with(nil)
+
+    duplicator.duplicate
+  end
+end
diff --git a/spec/lib/spree/product_duplicator_spec.rb b/spec/lib/spree/product_duplicator_spec.rb
deleted file mode 100644
index 0a04ec4161..0000000000
--- a/spec/lib/spree/product_duplicator_spec.rb
+++ /dev/null
@@ -1,86 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-module Spree
-  describe Spree::ProductDuplicator do
-    let(:product) do
-      double 'Product',
-             name: "foo",
-             taxons: [],
-             product_properties: [property],
-             master: variant,
-             variants?: false
-    end
-
-    let(:new_product) do
-      double 'New Product',
-             save!: true
-    end
-
-    let(:property) do
-      double 'Property'
-    end
-
-    let(:new_property) do
-      double 'New Property'
-    end
-
-    let(:variant) do
-      double 'Variant',
-             sku: "12345",
-             price: 19.99,
-             currency: "AUD",
-             images: [image]
-    end
-
-    let(:new_variant) do
-      double 'New Variant',
-             sku: "12345"
-    end
-
-    let(:image) do
-      double 'Image',
-             attachment: double('Attachment')
-    end
-
-    let(:new_image) do
-      double 'New Image'
-    end
-
-    before do
-      expect(product).to receive(:dup).and_return(new_product)
-      expect(variant).to receive(:dup).and_return(new_variant)
-      expect(image).to receive(:dup).and_return(new_image)
-      expect(property).to receive(:dup).and_return(new_property)
-    end
-
-    it "can duplicate a product" do
-      duplicator = Spree::ProductDuplicator.new(product)
-      expect(new_product).to receive(:name=).with("COPY OF foo")
-      expect(new_product).to receive(:taxons=).with([])
-      expect(new_product).to receive(:product_properties=).with([new_property])
-      expect(new_product).to receive(:created_at=).with(nil)
-      expect(new_product).to receive(:updated_at=).with(nil)
-      expect(new_product).to receive(:deleted_at=).with(nil)
-      expect(new_product).to receive(:master=).with(new_variant)
-
-      expect(new_variant).to receive(:sku=).with("COPY OF 12345")
-      expect(new_variant).to receive(:deleted_at=).with(nil)
-      expect(new_variant).to receive(:images=).with([new_image])
-      expect(new_variant).to receive(:price=).with(variant.price)
-      expect(new_variant).to receive(:currency=).with(variant.currency)
-
-      expect(image.attachment).to receive(:clone).and_return(image.attachment)
-
-      expect(new_image).to receive(:assign_attributes).
-        with(attachment: image.attachment).
-        and_return(new_image)
-
-      expect(new_property).to receive(:created_at=).with(nil)
-      expect(new_property).to receive(:updated_at=).with(nil)
-
-      duplicator.duplicate
-    end
-  end
-end

From f6195f1159509f79ec7c5b5c0f9ecdfd06bf98be Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Sat, 26 Sep 2020 08:43:49 +0100
Subject: [PATCH 17/17] Remove duplicate_extra logic from ProductDuplicator

---
 app/models/spree/product.rb                    |  6 ------
 lib/spree/core/product_duplicator.rb           |  4 +---
 spec/lib/spree/core/product_duplicator_spec.rb |  2 +-
 spec/models/spree/product_spec.rb              | 11 -----------
 4 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index 46d1dd3d7e..25c9340473 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -261,17 +261,11 @@ module Spree
     end
 
     # for adding products which are closely related to existing ones
-    # define "duplicate_extra" for site-specific actions, eg for additional fields
     def duplicate
       duplicator = Spree::Core::ProductDuplicator.new(self)
       duplicator.duplicate
     end
 
-    # Called by Spree::Product::duplicate before saving.
-    def duplicate_extra(_parent)
-      master.sku = ''
-    end
-
     # use deleted? rather than checking the attribute directly. this
     # allows extensions to override deleted? if they want to provide
     # their own definition.
diff --git a/lib/spree/core/product_duplicator.rb b/lib/spree/core/product_duplicator.rb
index c7af50afe6..f4b778432f 100644
--- a/lib/spree/core/product_duplicator.rb
+++ b/lib/spree/core/product_duplicator.rb
@@ -15,8 +15,6 @@ module Spree
         # don't dup the actual variants, just the characterising types
         new_product.option_types = product.option_types if product.variants?
 
-        # allow site to do some customization
-        new_product.__send__(:duplicate_extra, product) if new_product.respond_to?(:duplicate_extra)
         new_product.save!
         new_product
       end
@@ -38,7 +36,7 @@ module Spree
       def duplicate_master
         master = product.master
         master.dup.tap do |new_master|
-          new_master.sku = "COPY OF #{master.sku}"
+          new_master.sku = ""
           new_master.deleted_at = nil
           new_master.images = master.images.map { |image| duplicate_image image }
           new_master.price = master.price
diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb
index 3f9823f59e..4026c01c8f 100644
--- a/spec/lib/spree/core/product_duplicator_spec.rb
+++ b/spec/lib/spree/core/product_duplicator_spec.rb
@@ -64,7 +64,7 @@ describe Spree::Core::ProductDuplicator do
     expect(new_product).to receive(:deleted_at=).with(nil)
     expect(new_product).to receive(:master=).with(new_variant)
 
-    expect(new_variant).to receive(:sku=).with("COPY OF 12345")
+    expect(new_variant).to receive(:sku=).with("")
     expect(new_variant).to receive(:deleted_at=).with(nil)
     expect(new_variant).to receive(:images=).with([new_image])
     expect(new_variant).to receive(:price=).with(variant.price)
diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb
index 3b124574e3..16c7006795 100644
--- a/spec/models/spree/product_spec.rb
+++ b/spec/models/spree/product_spec.rb
@@ -16,17 +16,6 @@ module Spree
           expect(clone.master.sku).to eq ''
           expect(clone.images.size).to eq product.images.size
         end
-
-        it 'calls #duplicate_extra' do
-          Spree::Product.class_eval do
-            def duplicate_extra(old_product)
-              self.name = old_product.name.reverse
-            end
-          end
-
-          clone = product.duplicate
-          expect(clone.name).to eq product.name.reverse
-        end
       end
 
       context "product has no variants" do