Extracting variant and option_value naming out into a lib class that is now shared between line items and variants, line_items have option_values

app/models/spree/line_item_decorator.rb

@@ -1,6 +1,14 @@
+require 'open_food_network/variant_and_line_item_naming'
+
 Spree::LineItem.class_eval do
+  include OpenFoodNetwork::VariantAndLineItemNaming
+  has_and_belongs_to_many :option_values, join_table: 'spree_option_values_line_items', class_name: 'Spree::OptionValue'
+
   attr_accessible :max_quantity, :final_weight_volume
   attr_accessible :final_weight_volume, :price, :as => :api
+  after_save :update_units
+
+  delegate :unit_description, to: :variant
 
   # -- Scopes
   scope :managed_by, lambda { |user|
@@ -57,4 +65,9 @@ Spree::LineItem.class_eval do
   def display_amount_with_adjustments
     Spree::Money.new(amount_with_adjustments, { :currency => currency })
   end
+
+  def unit_value
+    return 0 if quantity == 0
+    final_weight_volume / quantity unless final_weight_volume.nil?
+  end
 end

app/models/spree/order_decorator.rb

@@ -130,6 +130,7 @@ Spree::Order.class_eval do
     else
       current_item = Spree::LineItem.new(:quantity => quantity, max_quantity: max_quantity)
       current_item.variant = variant
+      current_item.option_values = variant.option_values
       if variant.unit_value
         current_item.final_weight_volume = variant.unit_value * quantity
       else

app/models/spree/variant_decorator.rb

@@ -1,7 +1,9 @@
 require 'open_food_network/enterprise_fee_calculator'
-require 'open_food_network/option_value_namer'
+require 'open_food_network/variant_and_line_item_naming'
 
 Spree::Variant.class_eval do
+  include OpenFoodNetwork::VariantAndLineItemNaming
+
   has_many :exchange_variants, dependent: :destroy
   has_many :exchanges, through: :exchange_variants
   has_many :variant_overrides
@@ -62,44 +64,7 @@ Spree::Variant.class_eval do
     OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle).fees_by_type_for self
   end
 
-
-  # Copied and modified from Spree::Variant
-  def options_text
-    values = self.option_values.joins(:option_type).order("#{Spree::OptionType.table_name}.position asc")
-
-    values.map! &:presentation    # This line changed
-
-    values.to_sentence({ :words_connector => ", ", :two_words_connector => ", " })
-  end
-
-  def delete_unit_option_values
-    ovs = self.option_values.where(option_type_id: Spree::Product.all_variant_unit_option_types)
-    self.option_values.destroy ovs
-  end
-
-  # Used like "product.name - full_name". If called like this, a product with
-  # name "Bread" would be displayed as one of these:
-  #     Bread - 1kg                     # if display_name blank
-  #     Bread - Spelt Sourdough, 1kg    # if display_name is "Spelt Sourdough, 1kg"
-  #     Bread - 1kg Spelt Sourdough     # if unit_to_display is "1kg Spelt Sourdough"
-  #     Bread - Spelt Sourdough (1kg)   # if display_name is "Spelt Sourdough" and unit_to_display is "1kg"
-  def full_name
-    return unit_to_display if display_name.blank?
-    return display_name    if display_name.downcase.include? unit_to_display.downcase
-    return unit_to_display if unit_to_display.downcase.include? display_name.downcase
-    "#{display_name} (#{unit_to_display})"
-  end
-
-  def name_to_display
-    return product.name if display_name.blank?
-    display_name
-  end
-
-  def unit_to_display
-    return options_text if display_as.blank?
-    display_as
-  end
-
+  # TODO: Should this be moved into VariantAndLineItemNaming?
   def product_and_variant_name
     name = product.name
 
@@ -109,17 +74,6 @@ Spree::Variant.class_eval do
     name
   end
 
-  def update_units
-    delete_unit_option_values
-
-    option_type = self.product.variant_unit_option_type
-    if option_type
-      name = option_value_name
-      ov = Spree::OptionValue.where(option_type_id: option_type, name: name, presentation: name).first || Spree::OptionValue.create!({option_type: option_type, name: name, presentation: name}, without_protection: true)
-      option_values << ov
-    end
-  end
-
   def delete
     if product.variants == [self] # Only variant left on product
       errors.add :product, "must have at least one variant"
@@ -133,19 +87,9 @@ Spree::Variant.class_eval do
     end
   end
 
-
   private
 
   def update_weight_from_unit_value
     self.weight = unit_value / 1000 if self.product.variant_unit == 'weight' && unit_value.present?
   end
-
-  def option_value_name
-    if display_as.present?
-      display_as
-    else
-      option_value_namer = OpenFoodNetwork::OptionValueNamer.new self
-      option_value_namer.name
-    end
-  end
 end

db/migrate/20150924054538_add_option_values_line_items_join_table.rb

@@ -0,0 +1,14 @@
+class AddOptionValuesLineItemsJoinTable < ActiveRecord::Migration
+  def change
+    create_table :spree_option_values_line_items, :id => false, :force => true do |t|
+      t.integer  :line_item_id
+      t.integer  :option_value_id
+    end
+
+    Spree::LineItem.all.each do |line_item|
+      line_item.update_units
+    end
+
+    add_index :spree_option_values_line_items, :line_item_id, :name => 'index_option_values_line_items_on_line_item_id'
+  end
+end

db/schema.rb

@@ -635,6 +635,13 @@ ActiveRecord::Schema.define(:version => 20151002020537) do
     t.datetime "updated_at",     :null => false
   end
 
+  create_table "spree_option_values_line_items", :id => false, :force => true do |t|
+    t.integer "line_item_id"
+    t.integer "option_value_id"
+  end
+
+  add_index "spree_option_values_line_items", ["line_item_id"], :name => "index_option_values_line_items_on_line_item_id"
+
   create_table "spree_option_values_variants", :id => false, :force => true do |t|
     t.integer "variant_id"
     t.integer "option_value_id"

lib/open_food_network/variant_and_line_item_naming.rb

@@ -0,0 +1,70 @@
+# This module is included in both the Spree::Variant and Spree::LineItem model decorators
+# It contains all of our logic for creating and naming option values (which are associated
+# with both models) and methods for printing human readable "names" for instances of these models.
+
+require 'open_food_network/option_value_namer'
+
+module OpenFoodNetwork
+  module VariantAndLineItemNaming
+
+    # Copied and modified from Spree::Variant
+    def options_text
+      values = self.option_values.joins(:option_type).order("#{Spree::OptionType.table_name}.position asc")
+
+      values.map! &:presentation    # This line changed
+
+      values.to_sentence({ :words_connector => ", ", :two_words_connector => ", " })
+    end
+
+    # Used like "product.name - full_name". If called like this, a product with
+    # name "Bread" would be displayed as one of these:
+    #     Bread - 1kg                     # if display_name blank
+    #     Bread - Spelt Sourdough, 1kg    # if display_name is "Spelt Sourdough, 1kg"
+    #     Bread - 1kg Spelt Sourdough     # if unit_to_display is "1kg Spelt Sourdough"
+    #     Bread - Spelt Sourdough (1kg)   # if display_name is "Spelt Sourdough" and unit_to_display is "1kg"
+    def full_name
+      return unit_to_display if !self.has_attribute?(:display_name) || display_name.blank?
+      return display_name    if display_name.downcase.include? unit_to_display.downcase
+      return unit_to_display if unit_to_display.downcase.include? display_name.downcase
+      "#{display_name} (#{unit_to_display})"
+    end
+
+    def name_to_display
+      return product.name if !self.has_attribute?(:display_name) || display_name.blank?
+      display_name
+    end
+
+    def unit_to_display
+      return options_text if !self.has_attribute?(:display_as) || display_as.blank?
+      display_as
+    end
+
+
+    def update_units
+      delete_unit_option_values
+
+      option_type = self.product.variant_unit_option_type
+      if option_type
+        name = option_value_name
+        ov = Spree::OptionValue.where(option_type_id: option_type, name: name, presentation: name).first || Spree::OptionValue.create!({option_type: option_type, name: name, presentation: name}, without_protection: true)
+        option_values << ov
+      end
+    end
+
+    def delete_unit_option_values
+      ovs = self.option_values.where(option_type_id: Spree::Product.all_variant_unit_option_types)
+      self.option_values.destroy ovs
+    end
+
+    private
+
+    def option_value_name
+      if self.has_attribute?(:display_as) && display_as.present?
+        display_as
+      else
+        option_value_namer = OpenFoodNetwork::OptionValueNamer.new self
+        option_value_namer.name
+      end
+    end
+  end
+end

spec/models/spree/line_item_spec.rb

@@ -78,5 +78,96 @@ module Spree
         li_no_tax.should_not have_tax
       end
     end
+
+    describe "unit value/description" do
+      describe "generating the full name" do
+        let(:li) { LineItem.new }
+
+        before do
+          li.stub(:unit_to_display) { 'unit_to_display' }
+        end
+
+        it "returns unit_to_display" do
+          li.full_name.should == 'unit_to_display'
+        end
+      end
+
+      describe "getting name for display" do
+        it "returns product name" do
+          li = create(:variant, product: create(:product))
+          li.name_to_display.should == li.product.name
+        end
+      end
+
+      describe "getting unit for display" do
+        it "returns options_text" do
+          li = create(:line_item)
+          li.stub(:options_text).and_return "ponies"
+          li.unit_to_display.should == "ponies"
+        end
+      end
+
+      context "when the line_item already has a final_weight_volume set (and all required option values do not exist)" do
+        let!(:p0) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) }
+        let!(:v) { create(:variant, product: p0, unit_value: 10, unit_description: 'bar') }
+
+        let!(:p) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) }
+        let!(:li) { create(:line_item, product: p, final_weight_volume: 5) }
+
+        it "removes the old option value and assigns the new one" do
+          ov_orig = li.option_values.last
+          ov_var  = v.option_values.last
+          allow(li).to receive(:unit_description) { 'foo' }
+
+          expect {
+            li.update_attributes!(final_weight_volume: 10)
+          }.to change(Spree::OptionValue, :count).by(1)
+
+          li.option_values.should_not include ov_orig
+          li.option_values.should_not include ov_var
+          ov = li.option_values.last
+          ov.name.should == "10g foo"
+        end
+      end
+
+      context "when the variant already has a value set (and all required option values exist)" do
+        let!(:p0) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) }
+        let!(:v) { create(:variant, product: p0, unit_value: 10, unit_description: 'bar') }
+
+        let!(:p) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) }
+        let!(:li) { create(:line_item, product: p, final_weight_volume: 5) }
+
+        it "removes the old option value and assigns the new one" do
+          ov_orig = li.option_values.last
+          ov_new  = v.option_values.last
+          allow(li).to receive(:unit_description) { 'bar' }
+
+          expect {
+            li.update_attributes!(final_weight_volume: 10)
+          }.to change(Spree::OptionValue, :count).by(0)
+
+          li.option_values.should_not include ov_orig
+          li.option_values.should     include ov_new
+        end
+      end
+    end
+
+    describe "deleting unit option values" do
+      let!(:p) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) }
+      let!(:ot) { Spree::OptionType.find_by_name 'unit_weight' }
+      let!(:li) { create(:line_item, product: p) }
+
+      it "removes option value associations for unit option types" do
+        expect {
+          li.delete_unit_option_values
+        }.to change(li.option_values, :count).by(-1)
+      end
+
+      it "does not delete option values" do
+        expect {
+          li.delete_unit_option_values
+        }.to change(Spree::OptionValue, :count).by(0)
+      end
+    end
   end
 end

spec/models/spree/variant_spec.rb

@@ -115,44 +115,6 @@ module Spree
       end
     end
 
-    describe "generating the full name" do
-      let(:v) { Variant.new }
-
-      before do
-        v.stub(:display_name) { 'display_name' }
-        v.stub(:unit_to_display) { 'unit_to_display' }
-      end
-
-      it "returns unit_to_display when display_name is blank" do
-        v.stub(:display_name) { '' }
-        v.full_name.should == 'unit_to_display'
-      end
-
-      it "returns display_name when it contains unit_to_display" do
-        v.stub(:display_name) { 'DiSpLaY_name' }
-        v.stub(:unit_to_display) { 'name' }
-        v.full_name.should == 'DiSpLaY_name'
-      end
-
-      it "returns unit_to_display when it contains display_name" do
-        v.stub(:display_name) { '_to_' }
-        v.stub(:unit_to_display) { 'unit_TO_display' }
-        v.full_name.should == 'unit_TO_display'
-      end
-
-      it "returns a combination otherwise" do
-        v.stub(:display_name) { 'display_name' }
-        v.stub(:unit_to_display) { 'unit_to_display' }
-        v.full_name.should == 'display_name (unit_to_display)'
-      end
-
-      it "is resilient to regex chars" do
-        v = Variant.new display_name: ")))"
-        v.stub(:unit_to_display) { ")))" }
-        v.full_name.should == ")))"
-      end
-    end
-
     describe "generating the product and variant name" do
       let(:v) { Variant.new }
       let(:p) { double(:product, name: 'product') }
@@ -282,6 +244,44 @@ module Spree
     end
 
     describe "unit value/description" do
+      describe "generating the full name" do
+        let(:v) { Variant.new }
+
+        before do
+          v.stub(:display_name) { 'display_name' }
+          v.stub(:unit_to_display) { 'unit_to_display' }
+        end
+
+        it "returns unit_to_display when display_name is blank" do
+          v.stub(:display_name) { '' }
+          v.full_name.should == 'unit_to_display'
+        end
+
+        it "returns display_name when it contains unit_to_display" do
+          v.stub(:display_name) { 'DiSpLaY_name' }
+          v.stub(:unit_to_display) { 'name' }
+          v.full_name.should == 'DiSpLaY_name'
+        end
+
+        it "returns unit_to_display when it contains display_name" do
+          v.stub(:display_name) { '_to_' }
+          v.stub(:unit_to_display) { 'unit_TO_display' }
+          v.full_name.should == 'unit_TO_display'
+        end
+
+        it "returns a combination otherwise" do
+          v.stub(:display_name) { 'display_name' }
+          v.stub(:unit_to_display) { 'unit_to_display' }
+          v.full_name.should == 'display_name (unit_to_display)'
+        end
+
+        it "is resilient to regex chars" do
+          v = Variant.new display_name: ")))"
+          v.stub(:unit_to_display) { ")))" }
+          v.full_name.should == ")))"
+        end
+      end
+
       describe "getting name for display" do
         it "returns display_name if present" do
           v = create(:variant, display_name: "foo")
@@ -347,23 +347,18 @@ module Spree
         end
       end
 
-      context "when the variant already has a value set (and all required option values exist)" do
-        let!(:p0) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) }
-        let!(:v0) { create(:variant, product: p0, unit_value: 10, unit_description: 'foo') }
-
+      context "when the variant already has a value set (and all required option values do not exist)" do
         let!(:p) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) }
         let!(:v) { create(:variant, product: p, unit_value: 5, unit_description: 'bar') }
 
         it "removes the old option value and assigns the new one" do
           ov_orig = v.option_values.last
-          ov_new  = v0.option_values.last
 
           expect {
             v.update_attributes!(unit_value: 10, unit_description: 'foo')
-          }.to change(Spree::OptionValue, :count).by(0)
+          }.to change(Spree::OptionValue, :count).by(1)
 
           v.option_values.should_not include ov_orig
-          v.option_values.should     include ov_new
         end
       end