From 35b41fc7feb9214ebfcfb04c06343c953d75723a Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sat, 6 May 2023 00:36:24 +0100
Subject: [PATCH] Persist unit_value on variant and "option value text" on
 variant and line_item and improve related AR callbacks

---
 app/models/spree/line_item.rb                 | 14 ++--
 app/models/spree/product.rb                   | 13 ----
 app/models/spree/variant.rb                   | 10 ++-
 .../variant_and_line_item_naming.rb           | 63 ++++------------
 .../20230505165821_variant_options_columns.rb | 74 +++++++++++++++++++
 db/schema.rb                                  |  3 +
 6 files changed, 109 insertions(+), 68 deletions(-)
 create mode 100644 db/migrate/20230505165821_variant_options_columns.rb

diff --git a/app/models/spree/line_item.rb b/app/models/spree/line_item.rb
index edd9c78184..6392718748 100644
--- a/app/models/spree/line_item.rb
+++ b/app/models/spree/line_item.rb
@@ -42,12 +42,16 @@ module Spree
     before_save :update_inventory
     before_save :calculate_final_weight_volume, if: :quantity_changed?,
                                                 unless: :final_weight_volume_changed?
-    after_save :update_order
-    after_save :update_units
-    before_destroy :update_inventory_before_destroy
-    after_destroy :update_order
+    before_save :assign_units, if: ->(line_item) {
+      line_item.new_record? || line_item.changed_attributes.keys.include?("final_weight_volume")
+    }
 
-    delegate :product, :unit_description, :display_name, to: :variant
+    before_destroy :update_inventory_before_destroy
+
+    after_destroy :update_order
+    after_save :update_order
+
+    delegate :product, :variant_unit, :unit_description, :display_name, :display_as, to: :variant
 
     attr_accessor :skip_stock_check, :target_shipment # Allows manual skipping of Stock::AvailabilityValidator
 
diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index 1a97a28ea3..f9ced0f899 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -326,17 +326,6 @@ module Spree
       variants.map(&:import_date).compact.max
     end
 
-    def variant_unit_option_type
-      return if variant_unit.blank?
-
-      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 destroy
       transaction do
         touch_distributors
@@ -396,8 +385,6 @@ module Spree
     def update_units
       return unless saved_change_to_variant_unit? || saved_change_to_variant_unit_name?
 
-      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
 
diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
index 263947905a..8c47473359 100644
--- a/app/models/spree/variant.rb
+++ b/app/models/spree/variant.rb
@@ -17,6 +17,8 @@ module Spree
     searchable_associations :product, :option_values, :default_price
     searchable_scopes :active, :deleted
 
+    NAME_FIELDS = ["display_name", "display_as", "weight", "unit_value", "unit_description"].freeze
+
     belongs_to :product, -> { with_deleted }, touch: true, class_name: 'Spree::Product'
 
     delegate_belongs_to :product, :name, :description, :permalink, :available_on,
@@ -73,14 +75,16 @@ module Spree
     before_validation :ensure_unit_value
     before_validation :update_weight_from_unit_value, if: ->(v) { v.product.present? }
 
+    before_save :convert_variant_weight_to_decimal
+    before_save :assign_units, if: ->(variant) {
+      variant.new_record? || variant.changed_attributes.keys.intersection(NAME_FIELDS).any?
+    }
+
     after_save :save_default_price
-    after_save :update_units
 
     after_create :create_stock_items
     after_create :set_position
 
-    before_save :convert_variant_weight_to_decimal
-
     around_destroy :destruction
 
     # default variant scope only lists non-deleted variants
diff --git a/app/services/variant_units/variant_and_line_item_naming.rb b/app/services/variant_units/variant_and_line_item_naming.rb
index 9980abe083..ae32c01d74 100644
--- a/app/services/variant_units/variant_and_line_item_naming.rb
+++ b/app/services/variant_units/variant_and_line_item_naming.rb
@@ -8,31 +8,12 @@ require 'variant_units/option_value_namer'
 
 module VariantUnits
   module VariantAndLineItemNaming
-    # Copied and modified from Spree::Variant
     def options_text
-      values = if option_values_eager_loaded?
-                 # Don't trigger N+1 queries if option_values are already eager-loaded.
-                 # For best results, use: `Spree::Variant.includes(option_values: :option_type)`
-                 # or: `Spree::Product.includes(variant: {option_values: :option_type})`
-                 option_values.sort_by{ |o| o.option_type.position }
-               else
-                 option_values.joins(:option_type).
-                   order("#{Spree::OptionType.table_name}.position asc")
-               end
-
-      values.map { |option_value|
-        presentation(option_value)
-      }.to_sentence(words_connector: ", ", two_words_connector: ", ")
-    end
-
-    def presentation(option_value)
-      return option_value.presentation unless option_value.option_type.name == "unit_weight"
-
+      return unit_presentation unless variant_unit == "weight"
       return display_as if has_attribute?(:display_as) && display_as.present?
-
       return variant.display_as if variant_display_as?
 
-      option_value.presentation
+      unit_presentation
     end
 
     def variant_display_as?
@@ -68,47 +49,35 @@ module VariantUnits
 
     def unit_to_display
       return display_as if has_attribute?(:display_as) && display_as.present?
-
       return variant.display_as if variant_display_as?
 
       options_text
     end
 
-    def update_units
-      delete_unit_option_values
-
-      option_type = 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)
-        option_values << ov
-      end
+    def assign_units
+      assign_attributes(unit_value_attributes)
     end
 
-    def delete_unit_option_values
-      ovs = option_values.where(option_type_id: Spree::Product.all_variant_unit_option_types)
-      option_values.destroy ovs
+    def update_units
+      update_columns(unit_value_attributes)
+    end
+
+    def unit_value_attributes
+      units = { unit_presentation: option_value_name }
+      units.merge!({ variant_unit: product.variant_unit }) if has_attribute?(:variant_unit)
+      units
     end
 
     def weight_from_unit_value
-      (unit_value || 0) / 1000 if product.variant_unit == 'weight'
+      (unit_value || 0) / 1000 if variant_unit == 'weight'
     end
 
     private
 
-    def option_values_eager_loaded?
-      option_values.loaded?
-    end
-
     def option_value_name
-      if has_attribute?(:display_as) && display_as.present?
-        display_as
-      else
-        option_value_namer = VariantUnits::OptionValueNamer.new self
-        option_value_namer.name
-      end
+      return display_as if has_attribute?(:display_as) && display_as.present?
+
+      VariantUnits::OptionValueNamer.new(self).name
     end
   end
 end
diff --git a/db/migrate/20230505165821_variant_options_columns.rb b/db/migrate/20230505165821_variant_options_columns.rb
new file mode 100644
index 0000000000..67d063da2b
--- /dev/null
+++ b/db/migrate/20230505165821_variant_options_columns.rb
@@ -0,0 +1,74 @@
+class VariantOptionsColumns < ActiveRecord::Migration[7.0]
+  def up
+    add_column :spree_variants, :variant_unit, :string
+    add_column :spree_variants, :unit_presentation, :string
+    add_column :spree_line_items, :unit_presentation, :string
+
+    migrate_variant_unit
+    migrate_variant_presentation
+    migrate_line_item_presentation
+  end
+
+  def down
+    remove_column :spree_variants, :variant_unit
+    remove_column :spree_variants, :unit_presentation
+    remove_column :spree_line_items, :unit_presentation
+  end
+
+  # Migrates variant's product's variant_unit value onto variant
+  #
+  # Spree::Variant.includes(:product).each do |variant|
+  #   variant.update_columns(variant_unit: variant.product.variant_unit)
+  # end
+  def migrate_variant_unit
+    ActiveRecord::Base.connection.execute(<<-SQL
+      UPDATE spree_variants
+      SET variant_unit = product.variant_unit
+      FROM spree_products AS product
+      WHERE spree_variants.product_id = product.id
+    SQL
+    )
+  end
+
+  # Migrates the variants' option_value's presentation onto the variant's unit_presentation
+  #
+  # Spree::Variant.includes(:option_values: :option_type).each do |variant|
+  #   variant.update_columns(unit_presentation: variant.option_values.first.presentation)
+  # end
+  def migrate_variant_presentation
+    ActiveRecord::Base.connection.execute(<<-SQL
+      UPDATE spree_variants
+      SET unit_presentation = option_values.presentation
+      FROM (
+        SELECT 
+          DISTINCT ON (spree_option_values_variants.variant_id) variant_id,
+          spree_option_values.presentation AS presentation
+        FROM spree_option_values_variants
+          LEFT JOIN spree_option_values ON spree_option_values.id = spree_option_values_variants.option_value_id
+      ) option_values
+      WHERE spree_variants.id = option_values.variant_id
+    SQL
+    )
+  end
+
+  # Migrates the line_items' option_value's presentation onto the line_items's unit_presentation
+  #
+  # Spree::LineItem.includes(:option_values: :option_type).each do |line_item|
+  #   line_item.update_columns(unit_presentation: line_item.option_values.first.presentation)
+  # end
+  def migrate_line_item_presentation
+    ActiveRecord::Base.connection.execute(<<-SQL
+      UPDATE spree_line_items
+      SET unit_presentation = option_values.presentation
+      FROM (
+        SELECT 
+          DISTINCT ON (spree_option_values_line_items.line_item_id) line_item_id,
+          spree_option_values.presentation AS presentation
+        FROM spree_option_values_line_items
+          LEFT JOIN spree_option_values ON spree_option_values.id = spree_option_values_line_items.option_value_id
+      ) option_values
+      WHERE spree_line_items.id = option_values.line_item_id
+    SQL
+    )
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 474bf94ec0..1241b84131 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -549,6 +549,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_05_22_120633) do
     t.decimal "height", precision: 8, scale: 2
     t.decimal "width", precision: 8, scale: 2
     t.decimal "depth", precision: 8, scale: 2
+    t.string "unit_presentation"
     t.index ["order_id"], name: "index_line_items_on_order_id"
     t.index ["variant_id"], name: "index_line_items_on_variant_id"
   end
@@ -1076,6 +1077,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_05_22_120633) do
     t.string "display_name", limit: 255
     t.string "display_as", limit: 255
     t.datetime "import_date", precision: nil
+    t.string "variant_unit"
+    t.string "unit_presentation"
     t.index ["product_id"], name: "index_variants_on_product_id"
     t.index ["sku"], name: "index_spree_variants_on_sku"
     t.check_constraint "unit_value > 0::double precision", name: "positive_unit_value"