diff --git a/app/components/products_table_component.rb b/app/components/products_table_component.rb index 44e06e8eb1..1eba6f1391 100644 --- a/app/components/products_table_component.rb +++ b/app/components/products_table_component.rb @@ -173,8 +173,7 @@ class ProductsTableComponent < ViewComponentReflex::Component :default_price, :stock_locations, :stock_items, - :variant_overrides, - { option_values: :option_type } + :variant_overrides ] ] end diff --git a/app/controllers/admin/bulk_line_items_controller.rb b/app/controllers/admin/bulk_line_items_controller.rb index bf4b16fc70..c10460b8d8 100644 --- a/app/controllers/admin/bulk_line_items_controller.rb +++ b/app/controllers/admin/bulk_line_items_controller.rb @@ -11,7 +11,7 @@ module Admin @line_items = order_permissions. editable_line_items.where(order_id: orders). - includes(variant: { option_values: :option_type }). + includes(:variant). ransack(params[:q]).result. reorder('spree_line_items.order_id ASC, spree_line_items.id ASC') diff --git a/app/controllers/admin/enterprises_controller.rb b/app/controllers/admin/enterprises_controller.rb index a9017ea6c9..6ec6bafce4 100644 --- a/app/controllers/admin/enterprises_controller.rb +++ b/app/controllers/admin/enterprises_controller.rb @@ -29,7 +29,6 @@ module Admin after_action :geocode_address_if_use_geocoder, only: [:create, :update] - helper 'spree/products' include OrderCyclesHelper def index @@ -187,7 +186,7 @@ module Admin if enterprises.present? enterprises.includes( supplied_products: - [:supplier, { master: [:images], variants: { option_values: :option_type } }] + [:supplier, :variants, { master: [:images] }] ) end when :index diff --git a/app/controllers/api/v0/products_controller.rb b/app/controllers/api/v0/products_controller.rb index c15a3d60b3..0363c21e56 100644 --- a/app/controllers/api/v0/products_controller.rb +++ b/app/controllers/api/v0/products_controller.rb @@ -117,8 +117,7 @@ module Api def product_query_includes [ master: { images: { attachment_attachment: :blob } }, - variants: [:default_price, :stock_locations, :stock_items, :variant_overrides, - { option_values: :option_type }] + variants: [:default_price, :stock_locations, :stock_items, :variant_overrides] ] end diff --git a/app/controllers/api/v0/variants_controller.rb b/app/controllers/api/v0/variants_controller.rb index e1846ebe32..684ab9064a 100644 --- a/app/controllers/api/v0/variants_controller.rb +++ b/app/controllers/api/v0/variants_controller.rb @@ -9,12 +9,12 @@ module Api before_action :product def index - @variants = scope.includes(option_values: :option_type).ransack(params[:q]).result + @variants = scope.ransack(params[:q]).result render json: @variants, each_serializer: Api::VariantSerializer end def show - @variant = scope.includes(option_values: :option_type).find(params[:id]) + @variant = scope.find(params[:id]) render json: @variant, serializer: Api::VariantSerializer end diff --git a/app/controllers/enterprises_controller.rb b/app/controllers/enterprises_controller.rb index 058c6528ed..1043fdb4c6 100644 --- a/app/controllers/enterprises_controller.rb +++ b/app/controllers/enterprises_controller.rb @@ -4,7 +4,6 @@ require 'open_food_network/enterprise_injection_data' class EnterprisesController < BaseController layout "darkswarm" - helper Spree::ProductsHelper include OrderCyclesHelper include SerializerHelper include WhiteLabel diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb index 8ee9450326..a92b04bac3 100644 --- a/app/controllers/spree/admin/products_controller.rb +++ b/app/controllers/spree/admin/products_controller.rb @@ -7,7 +7,6 @@ require 'open_food_network/permissions' module Spree module Admin class ProductsController < ::Admin::ResourceController - helper 'spree/products' include OpenFoodNetwork::SpreeApiKeyLoader include OrderCyclesHelper include EnterprisesHelper @@ -113,7 +112,6 @@ module Spree def load_data @taxons = Taxon.order(:name) - @option_types = OptionType.order(:name) @tax_categories = TaxCategory.order(:name) @shipping_categories = ShippingCategory.order(:name) end @@ -123,7 +121,7 @@ module Spree end def product_includes - [{ variants: [:images, { option_values: :option_type }] }, + [{ variants: [:images] }, { master: [:images, :default_price] }] end diff --git a/app/controllers/spree/admin/variants_controller.rb b/app/controllers/spree/admin/variants_controller.rb index c888acd20f..49abb361a2 100644 --- a/app/controllers/spree/admin/variants_controller.rb +++ b/app/controllers/spree/admin/variants_controller.rb @@ -5,7 +5,6 @@ require 'open_food_network/scope_variants_for_search' module Spree module Admin class VariantsController < ::Admin::ResourceController - helper 'spree/products' belongs_to 'spree/product', find_by: :permalink before_action :assign_default_attributes, only: :new @@ -81,8 +80,6 @@ module Spree end def create_before - option_values = params[:new_variant] - option_values&.each_value { |id| @object.option_values << OptionValue.find(id) } @object.save end diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb index 026590c494..ded1115e75 100644 --- a/app/controllers/spree/orders_controller.rb +++ b/app/controllers/spree/orders_controller.rb @@ -10,7 +10,7 @@ module Spree layout 'darkswarm' rescue_from ActiveRecord::RecordNotFound, with: :render_404 - helper 'spree/products', 'spree/orders' + helper 'spree/orders' respond_to :html, :json diff --git a/app/helpers/spree/products_helper.rb b/app/helpers/spree/products_helper.rb deleted file mode 100644 index 9e367fb010..0000000000 --- a/app/helpers/spree/products_helper.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module Spree - module ProductsHelper - def product_has_variant_unit_option_type?(product) - product.option_types.any? { |option_type| variant_unit_option_type? option_type } - end - - def variant_unit_option_type?(option_type) - Spree::Product.all_variant_unit_option_types.include? option_type - end - end -end diff --git a/app/mailers/producer_mailer.rb b/app/mailers/producer_mailer.rb index 1d8a473454..b9dd977c78 100644 --- a/app/mailers/producer_mailer.rb +++ b/app/mailers/producer_mailer.rb @@ -60,7 +60,7 @@ class ProducerMailer < ApplicationMailer def line_items_from(order_cycle, producer) @line_items ||= Spree::LineItem. - includes(:option_values, variant: [:product, { option_values: :option_type }]). + includes(variant: [:product]). from_order_cycle(order_cycle). sorted_by_name_and_unit_value. merge(Spree::Product.with_deleted.in_supplier(producer)). diff --git a/app/models/spree/ability.rb b/app/models/spree/ability.rb index f0ae8fda0c..c9ea4514fc 100644 --- a/app/models/spree/ability.rb +++ b/app/models/spree/ability.rb @@ -22,8 +22,6 @@ module Spree can :manage, :all else can [:index, :read], Country - can [:index, :read], OptionType - can [:index, :read], OptionValue can :create, Order can :read, Order do |order, token| order.user == user || order.token && token == order.token diff --git a/app/models/spree/line_item.rb b/app/models/spree/line_item.rb index edd9c78184..ef0ea3a86b 100644 --- a/app/models/spree/line_item.rb +++ b/app/models/spree/line_item.rb @@ -9,7 +9,7 @@ module Spree include LineItemStockChanges searchable_attributes :price, :quantity, :order_id, :variant_id, :tax_category_id - searchable_associations :order, :order_cycle, :variant, :product, :supplier, :tax_category, :option_values + searchable_associations :order, :order_cycle, :variant, :product, :supplier, :tax_category searchable_scopes :with_tax, :without_tax belongs_to :order, class_name: "Spree::Order", inverse_of: :line_items @@ -22,9 +22,6 @@ module Spree has_many :adjustments, as: :adjustable, dependent: :destroy - has_and_belongs_to_many :option_values, join_table: 'spree_option_values_line_items', - class_name: 'Spree::OptionValue' - before_validation :adjust_quantity before_validation :copy_price before_validation :copy_tax_category @@ -42,12 +39,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.final_weight_volume_changed? + } - 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/option_type.rb b/app/models/spree/option_type.rb deleted file mode 100644 index fe0ea71138..0000000000 --- a/app/models/spree/option_type.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module Spree - class OptionType < ApplicationRecord - has_many :option_values, -> { order(:position) }, dependent: :destroy - has_many :product_option_types, dependent: :destroy - has_many :products, through: :product_option_types - - 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 - end -end diff --git a/app/models/spree/option_value.rb b/app/models/spree/option_value.rb deleted file mode 100644 index f03301a5e5..0000000000 --- a/app/models/spree/option_value.rb +++ /dev/null @@ -1,12 +0,0 @@ -# frozen_string_literal: true - -module Spree - class OptionValue < ApplicationRecord - 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/option_values_line_item.rb b/app/models/spree/option_values_line_item.rb deleted file mode 100644 index 0b6632e7e3..0000000000 --- a/app/models/spree/option_values_line_item.rb +++ /dev/null @@ -1,8 +0,0 @@ -# frozen_string_literal: true - -module Spree - class OptionValuesLineItem < ApplicationRecord - belongs_to :line_item, class_name: 'Spree::LineItem' - belongs_to :option_value, class_name: 'Spree::OptionValue' - end -end diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index e08012beea..c25e0575b8 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -11,7 +11,6 @@ require 'concerns/product_stock' # # 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. # @@ -33,11 +32,6 @@ module Spree searchable_associations :supplier, :properties, :primary_taxon, :variants, :master searchable_scopes :active, :with_properties - has_many :product_option_types, dependent: :destroy - # 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 @@ -87,7 +81,6 @@ module Spree delegate :images_attributes=, :display_as=, to: :master after_create :set_master_variant_defaults - after_create :build_variants_from_option_values_hash, if: :option_values_hash after_save :save_master delegate :images, to: :master, prefix: true @@ -116,16 +109,12 @@ module Spree presence: { if: ->(p) { p.variant_unit == 'items' } } validate :validate_image_for_master - 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 after_initialize :set_available_on_to_now, if: :new_record? @@ -260,32 +249,12 @@ module Spree end 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| - option_type_ids << id unless option_type_ids.include?(id) - unless product_option_types.pluck(:option_type_id).include?(id) - product_option_types.create(option_type_id: id) - end - end - end - # for adding products which are closely related to existing ones def duplicate duplicator = Spree::Core::ProductDuplicator.new(self) duplicator.duplicate 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| @@ -351,21 +320,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 self.all_variant_unit_option_types - Spree::OptionType.where('name LIKE ?', 'unit_%%') - end - def destroy transaction do touch_distributors @@ -380,21 +334,6 @@ 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) } - - values.each do |ids| - variants.create( - option_value_ids: ids, - price: master.price - ) - end - save - end - # ensures the master variant is flagged as such def set_master_variant_defaults master.is_master = true @@ -440,8 +379,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/product_option_type.rb b/app/models/spree/product_option_type.rb deleted file mode 100644 index 9ef5496480..0000000000 --- a/app/models/spree/product_option_type.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module Spree - class ProductOptionType < ApplicationRecord - 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/variant.rb b/app/models/spree/variant.rb index 263947905a..1b3625ba01 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -14,9 +14,11 @@ module Spree acts_as_paranoid searchable_attributes :sku, :display_as, :display_name - searchable_associations :product, :option_values, :default_price + searchable_associations :product, :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, @@ -29,9 +31,6 @@ module Spree has_many :stock_items, dependent: :destroy, inverse_of: :variant 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" @@ -73,14 +72,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 @@ -177,10 +178,6 @@ module Spree order_cycle).fees_name_by_type_for self end - def option_value(opt_name) - option_values.detect { |o| o.option_type.name == opt_name }.try(:presentation) - end - def price_in(currency) prices.select{ |price| price.currency == currency }.first || Spree::Price.new(variant_id: id, currency: currency) diff --git a/app/services/variant_units/option_value_namer.rb b/app/services/variant_units/option_value_namer.rb index 085a73b256..6a36684fb3 100644 --- a/app/services/variant_units/option_value_namer.rb +++ b/app/services/variant_units/option_value_namer.rb @@ -4,18 +4,18 @@ require "open_food_network/i18n_inflections" module VariantUnits class OptionValueNamer - def initialize(variant = nil) - @variant = variant + # nameable can be either a Spree::LineItem or a Spree::Variant + def initialize(nameable = nil) + @nameable = nameable end - def name(obj = nil) - @variant = obj unless obj.nil? + def name value, unit = option_value_value_unit separator = value_scaled? ? '' : ' ' name_fields = [] name_fields << "#{value}#{separator}#{unit}" if value.present? && unit.present? - name_fields << @variant.unit_description if @variant.unit_description.present? + name_fields << @nameable.unit_description if @nameable.unit_description.present? name_fields.join ' ' end @@ -32,16 +32,16 @@ module VariantUnits private def value_scaled? - @variant.product.variant_unit_scale.present? + @nameable.product.variant_unit_scale.present? end def option_value_value_unit - if @variant.unit_value.present? - if %w(weight volume).include? @variant.product.variant_unit + if @nameable.unit_value.present? && @nameable.product&.persisted? + if %w(weight volume).include? @nameable.product.variant_unit value, unit_name = option_value_value_unit_scaled else - value = @variant.unit_value - unit_name = pluralize(@variant.product.variant_unit_name, value) + value = @nameable.unit_value + unit_name = pluralize(@nameable.product.variant_unit_name, value) end value = value.to_i if value == value.to_i @@ -56,13 +56,13 @@ module VariantUnits def option_value_value_unit_scaled unit_scale, unit_name = scale_for_unit_value - value = (@variant.unit_value / unit_scale).to_d.truncate(2) + value = (@nameable.unit_value / unit_scale).to_d.truncate(2) [value, unit_name] end def scale_for_unit_value - WeightsAndMeasures.new(@variant).scale_for_unit_value + WeightsAndMeasures.new(@nameable).scale_for_unit_value end def pluralize(unit_name, count) 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..e26c02b342 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,28 +49,23 @@ 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 @@ -98,17 +74,10 @@ module VariantUnits 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/app/views/spree/admin/orders/_line_item.html.haml b/app/views/spree/admin/orders/_line_item.html.haml index b1164c1667..6f985eb0e1 100644 --- a/app/views/spree/admin/orders/_line_item.html.haml +++ b/app/views/spree/admin/orders/_line_item.html.haml @@ -1,7 +1,7 @@ %tr{id: "#{spree_dom_id(f.object)}", class: "#{cycle('odd', 'even')}"} %td = f.object.variant.product.name - = "(#{f.object.full_name})" unless f.object.variant.option_values.empty? + = "(#{f.object.full_name})" %td.price.align-center = f.object.single_display_amount %td.qty diff --git a/app/views/spree/admin/variants/_form.html.haml b/app/views/spree/admin/variants/_form.html.haml index 4990c64e4b..35a41f666d 100644 --- a/app/views/spree/admin/variants/_form.html.haml +++ b/app/views/spree/admin/variants/_form.html.haml @@ -6,28 +6,18 @@ = f.label :display_as, t('.display_as') = f.text_field :display_as, class: "fullwidth", placeholder: t('.display_as_placeholder') - - if product_has_variant_unit_option_type?(@product) - - if @product.variant_unit != 'items' - .field - = label_tag :unit_value_human, "#{t('admin.'+@product.variant_unit)} ({{unitName(#{@product.variant_unit_scale}, '#{@product.variant_unit}')}})" - = hidden_field_tag 'product_variant_unit_scale', @product.variant_unit_scale - = number_field_tag :unit_value_human, nil, {class: "fullwidth", step: 0.01, 'ng-model' => 'unit_value_human', 'ng-change' => 'updateValue()'} - = f.number_field :unit_value, {hidden: true, 'ng-value' => 'unit_value'} - + - if @product.variant_unit != 'items' .field - = f.label :unit_description, t(:spree_admin_unit_description) - = f.text_field :unit_description, class: "fullwidth", placeholder: t('admin.products.unit_name_placeholder') + = label_tag :unit_value_human, "#{t('admin.'+@product.variant_unit)} ({{unitName(#{@product.variant_unit_scale}, '#{@product.variant_unit}')}})" + = hidden_field_tag 'product_variant_unit_scale', @product.variant_unit_scale + = number_field_tag :unit_value_human, nil, {class: "fullwidth", step: 0.01, 'ng-model' => 'unit_value_human', 'ng-change' => 'updateValue()'} + = f.number_field :unit_value, {hidden: true, 'ng-value' => 'unit_value'} + + .field + = f.label :unit_description, t(:spree_admin_unit_description) + = f.text_field :unit_description, class: "fullwidth", placeholder: t('admin.products.unit_name_placeholder') %div - - @product.option_types.each do |option_type| - - unless variant_unit_option_type?(option_type) - .field - = label :new_variant, option_type.presentation - - if @variant.new_record? - = select(:new_variant, option_type.presentation, option_type.option_values.collect {|ov| [ ov.presentation, ov.id ] }, {}, {class: 'select2 fullwidth'}) - - else - - if opt = @variant.option_values.detect {|o| o.option_type == option_type }.try(:presentation) - = text_field(:new_variant, option_type.presentation, value: opt, disabled: 'disabled', class: 'fullwidth') .field = f.label :sku, t('.sku') = f.text_field :sku, class: 'fullwidth' 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" diff --git a/engines/order_management/spec/services/order_management/subscriptions/form_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/form_spec.rb index a86dbac0e9..56122e12e6 100644 --- a/engines/order_management/spec/services/order_management/subscriptions/form_spec.rb +++ b/engines/order_management/spec/services/order_management/subscriptions/form_spec.rb @@ -12,14 +12,14 @@ module OrderManagement let!(:product2) { create(:product, supplier: shop) } let!(:product3) { create(:product, supplier: shop) } let!(:variant1) { - create(:variant, product: product1, unit_value: '100', price: 12.00, option_values: []) + create(:variant, product: product1, unit_value: '100', price: 12.00) } let!(:variant2) { - create(:variant, product: product2, unit_value: '1000', price: 6.00, option_values: []) + create(:variant, product: product2, unit_value: '1000', price: 6.00) } let!(:variant3) { create(:variant, product: product2, unit_value: '1000', - price: 2.50, option_values: [], on_hand: 1) + price: 2.50, on_hand: 1) } let!(:enterprise_fee) { create(:enterprise_fee, amount: 1.75) } let!(:order_cycle1) { diff --git a/lib/open_food_network/scope_variants_for_search.rb b/lib/open_food_network/scope_variants_for_search.rb index a36be8267e..f633d7c3c6 100644 --- a/lib/open_food_network/scope_variants_for_search.rb +++ b/lib/open_food_network/scope_variants_for_search.rb @@ -34,7 +34,6 @@ module OpenFoodNetwork def query_scope Spree::Variant.where(is_master: false). - includes(option_values: :option_type). ransack(search_params.merge(m: 'or')). result. order("spree_products.name, display_name, display_as, spree_products.variant_unit_name"). diff --git a/lib/reporting/queries/joins.rb b/lib/reporting/queries/joins.rb index 88690d2ac7..39d73fd05e 100644 --- a/lib/reporting/queries/joins.rb +++ b/lib/reporting/queries/joins.rb @@ -40,13 +40,6 @@ module Reporting def joins_order_bill_address reflect query.join(association(Spree::Order, :bill_address, bill_address_alias)) end - - def join_line_item_option_values - reflect query. - join(association(Spree::LineItem, :option_values)). - join(association(Spree::OptionValuesLineItem, :option_value) - ) - end end end end diff --git a/lib/reporting/queries/query_builder.rb b/lib/reporting/queries/query_builder.rb index 4def4b6c5d..0f20f77c19 100644 --- a/lib/reporting/queries/query_builder.rb +++ b/lib/reporting/queries/query_builder.rb @@ -65,7 +65,7 @@ module Reporting def variant_full_name display_name = variant_table[:display_name] display_as = variant_table[:display_as] - options_text = option_value_table[:presentation] + options_text = variant_table[:unit_presentation] unit_to_display = coalesce(nullify_empty_strings(display_as), options_text) combined_description = sql_concat(display_name, raw("' ('"), unit_to_display, raw("')'")) diff --git a/lib/reporting/queries/tables.rb b/lib/reporting/queries/tables.rb index a6843706f4..0ba99a7e23 100644 --- a/lib/reporting/queries/tables.rb +++ b/lib/reporting/queries/tables.rb @@ -39,10 +39,6 @@ module Reporting Spree::Order.arel_table.alias(:managed_orders) end - def option_value_table - Spree::OptionValue.arel_table - end - def shipping_category_table Spree::ShippingCategory.arel_table end diff --git a/lib/reporting/reports/bulk_coop/base.rb b/lib/reporting/reports/bulk_coop/base.rb index e6d0c0229a..bf48a6a9d8 100644 --- a/lib/reporting/reports/bulk_coop/base.rb +++ b/lib/reporting/reports/bulk_coop/base.rb @@ -22,9 +22,8 @@ module Reporting [ { order: [:bill_address], - variant: [{ option_values: :option_type }, { product: :supplier }] - }, - :option_values + variant: { product: :supplier } + } ] end diff --git a/lib/reporting/reports/orders_and_fulfillment/order_cycle_customer_totals.rb b/lib/reporting/reports/orders_and_fulfillment/order_cycle_customer_totals.rb index 651c6f0165..caf65c8165 100644 --- a/lib/reporting/reports/orders_and_fulfillment/order_cycle_customer_totals.rb +++ b/lib/reporting/reports/orders_and_fulfillment/order_cycle_customer_totals.rb @@ -91,7 +91,7 @@ module Reporting end def line_item_includes - [{ variant: [{ option_values: :option_type }, { product: :supplier }], + [{ variant: { product: :supplier }, order: [:bill_address, :ship_address, :order_cycle, :adjustments, :payments, :user, :distributor, :shipments] }] end diff --git a/lib/reporting/reports/orders_and_fulfillment/order_cycle_distributor_totals_by_supplier.rb b/lib/reporting/reports/orders_and_fulfillment/order_cycle_distributor_totals_by_supplier.rb index c0622cff0a..9e2dcd4d43 100644 --- a/lib/reporting/reports/orders_and_fulfillment/order_cycle_distributor_totals_by_supplier.rb +++ b/lib/reporting/reports/orders_and_fulfillment/order_cycle_distributor_totals_by_supplier.rb @@ -40,7 +40,7 @@ module Reporting :adjustments, { shipments: { shipping_rates: :shipping_method } } ], - variant: [{ option_values: :option_type }, { product: :supplier }] + variant: { product: :supplier } }] end end diff --git a/lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals.rb b/lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals.rb index 3eb7d222f6..fef10bca3c 100644 --- a/lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals.rb +++ b/lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals.rb @@ -36,7 +36,7 @@ module Reporting end def line_item_includes - [{ variant: [{ option_values: :option_type }, { product: :supplier }] }] + [{ variant: { product: :supplier } }] end def query_result diff --git a/lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor.rb b/lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor.rb index 728f01c35b..3d7c211126 100644 --- a/lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor.rb +++ b/lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor.rb @@ -41,7 +41,7 @@ module Reporting def line_item_includes [{ order: :distributor, - variant: [{ option_values: :option_type }, { product: :supplier }] }] + variant: { product: :supplier } }] end end end diff --git a/lib/reporting/reports/packing/base.rb b/lib/reporting/reports/packing/base.rb index 65a81c53e2..19e60155b2 100644 --- a/lib/reporting/reports/packing/base.rb +++ b/lib/reporting/reports/packing/base.rb @@ -20,7 +20,6 @@ module Reporting joins_variant_product. joins_product_supplier. joins_product_shipping_category. - join_line_item_option_values. selecting(select_fields). ordered_by(ordering_fields) end diff --git a/lib/reporting/reports/products_and_inventory/base.rb b/lib/reporting/reports/products_and_inventory/base.rb index 907c7f13f8..efae30ca67 100644 --- a/lib/reporting/reports/products_and_inventory/base.rb +++ b/lib/reporting/reports/products_and_inventory/base.rb @@ -34,7 +34,6 @@ module Reporting def child_variants Spree::Variant. where(is_master: false). - includes(option_values: :option_type). joins(:product). merge(visible_products). order('spree_products.name') diff --git a/lib/reporting/reports/xero_invoices/base.rb b/lib/reporting/reports/xero_invoices/base.rb index 5ece80fc9f..efdc8633f1 100644 --- a/lib/reporting/reports/xero_invoices/base.rb +++ b/lib/reporting/reports/xero_invoices/base.rb @@ -62,7 +62,7 @@ module Reporting def line_item_includes [:bill_address, :adjustments, - { line_items: { variant: [{ option_values: :option_type }, { product: :supplier }] } }] + { line_items: { variant: { product: :supplier } } }] end def detail_rows_for_order(order, invoice_number, opts) diff --git a/lib/spree/core/product_duplicator.rb b/lib/spree/core/product_duplicator.rb index 15c999cf70..83f80376ee 100644 --- a/lib/spree/core/product_duplicator.rb +++ b/lib/spree/core/product_duplicator.rb @@ -11,8 +11,6 @@ module Spree def duplicate new_product = duplicate_product - new_product.option_types = product.option_types - new_product.save! new_product end diff --git a/lib/tasks/data/remove_transient_data.rb b/lib/tasks/data/remove_transient_data.rb index 08411af0c4..36136ceff8 100644 --- a/lib/tasks/data/remove_transient_data.rb +++ b/lib/tasks/data/remove_transient_data.rb @@ -27,11 +27,9 @@ class RemoveTransientData merge(orders_without_payments) old_cart_line_items = Spree::LineItem.where(order_id: old_carts) - old_line_item_options = Spree::OptionValuesLineItem.where(line_item_id: old_cart_line_items) old_cart_adjustments = Spree::Adjustment.where(order_id: old_carts) old_cart_adjustments.delete_all - old_line_item_options.delete_all old_cart_line_items.delete_all old_carts.delete_all end diff --git a/spec/controllers/admin/subscription_line_items_controller_spec.rb b/spec/controllers/admin/subscription_line_items_controller_spec.rb index 639cce1538..897c8f858e 100644 --- a/spec/controllers/admin/subscription_line_items_controller_spec.rb +++ b/spec/controllers/admin/subscription_line_items_controller_spec.rb @@ -11,7 +11,7 @@ describe Admin::SubscriptionLineItemsController, type: :controller do let(:unmanaged_shop) { create(:enterprise) } let!(:product) { create(:product) } let!(:variant) { - create(:variant, product: product, unit_value: '100', price: 15.00, option_values: []) + create(:variant, product: product, unit_value: '100', price: 15.00) } let!(:outgoing_exchange) { order_cycle.exchanges.create(sender: shop, receiver: shop, variants: [variant], diff --git a/spec/controllers/admin/subscriptions_controller_spec.rb b/spec/controllers/admin/subscriptions_controller_spec.rb index 327fa5bf49..1445afbc33 100644 --- a/spec/controllers/admin/subscriptions_controller_spec.rb +++ b/spec/controllers/admin/subscriptions_controller_spec.rb @@ -262,7 +262,7 @@ describe Admin::SubscriptionsController, type: :controller do let!(:customer) { create(:customer, enterprise: shop) } let!(:product1) { create(:product, supplier: shop) } let!(:variant1) { - create(:variant, product: product1, unit_value: '100', price: 12.00, option_values: []) + create(:variant, product: product1, unit_value: '100', price: 12.00) } let!(:enterprise_fee) { create(:enterprise_fee, amount: 1.75) } let!(:order_cycle) { @@ -373,7 +373,7 @@ describe Admin::SubscriptionsController, type: :controller do context 'with subscription_line_items params' do let!(:product2) { create(:product) } let!(:variant2) { - create(:variant, product: product2, unit_value: '1000', price: 6.00, option_values: []) + create(:variant, product: product2, unit_value: '1000', price: 6.00) } before do diff --git a/spec/controllers/api/v0/products_controller_spec.rb b/spec/controllers/api/v0/products_controller_spec.rb index 1d9938117d..e4c8f1a9f7 100644 --- a/spec/controllers/api/v0/products_controller_spec.rb +++ b/spec/controllers/api/v0/products_controller_spec.rb @@ -61,7 +61,7 @@ describe Api::V0::ProductsController, type: :controller do api_get :show, id: product.to_param expect(json_response["permalink_live"]).to match(/and-1-ways/) - product.destroy + product.reload.destroy api_get :show, id: other_product.id expect(json_response["permalink_live"]).to match(/droids/) diff --git a/spec/controllers/api/v0/variants_controller_spec.rb b/spec/controllers/api/v0/variants_controller_spec.rb index fb50f7679c..c64b705e5e 100644 --- a/spec/controllers/api/v0/variants_controller_spec.rb +++ b/spec/controllers/api/v0/variants_controller_spec.rb @@ -24,7 +24,6 @@ describe Api::V0::VariantsController, type: :controller do let!(:product) { create(:product) } let!(:variant) do variant = product.master - variant.option_values << create(:option_value) variant end @@ -46,17 +45,18 @@ describe Api::V0::VariantsController, type: :controller do # Regression test for spree#2141 context "a deleted variant" do before do + expect(Spree::Variant.count).to eq 11 variant.update_column(:deleted_at, Time.zone.now) end it "is not returned in the results" do api_get :index - expect(json_response.count).to eq(10) # there are 11 variants + expect(json_response.count).to eq(10) end it "is not returned even when show_deleted is passed" do api_get :index, show_deleted: true - expect(json_response.count).to eq(10) # there are 11 variants + expect(json_response.count).to eq(10) end end diff --git a/spec/factories/options_factory.rb b/spec/factories/options_factory.rb deleted file mode 100644 index 772c4f8320..0000000000 --- a/spec/factories/options_factory.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -FactoryBot.define do - factory :option_value, class: Spree::OptionValue do - name { 'Size' } - presentation { 'S' } - option_type - end - - factory :option_type, class: Spree::OptionType do - name { 'foo-size' } - presentation { 'Size' } - - # Prevent inconsistent ordering in specs when all option types have the same (0) position - sequence(:position) - end -end diff --git a/spec/factories/product_factory.rb b/spec/factories/product_factory.rb index 24ff5cf531..1d936699ec 100644 --- a/spec/factories/product_factory.rb +++ b/spec/factories/product_factory.rb @@ -34,10 +34,7 @@ FactoryBot.define do after(:create) do |product, evaluator| product.master.on_hand = evaluator.on_hand product.variants.first.on_hand = evaluator.on_hand - end - - factory :product_with_option_types do - after(:create) { |product| create(:product_option_type, product: product) } + product.reload end end end @@ -60,6 +57,7 @@ FactoryBot.define do product.master.on_hand = evaluator.on_hand product.variants.first.on_demand = evaluator.on_demand product.variants.first.on_hand = evaluator.on_hand + product.reload end end diff --git a/spec/factories/product_option_type_factory.rb b/spec/factories/product_option_type_factory.rb deleted file mode 100644 index 22a8b6f96a..0000000000 --- a/spec/factories/product_option_type_factory.rb +++ /dev/null @@ -1,8 +0,0 @@ -# frozen_string_literal: true - -FactoryBot.define do - factory :product_option_type, class: Spree::ProductOptionType do - product - option_type - end -end diff --git a/spec/factories/variant_factory.rb b/spec/factories/variant_factory.rb index 61badeed3e..cd4b322a02 100644 --- a/spec/factories/variant_factory.rb +++ b/spec/factories/variant_factory.rb @@ -12,7 +12,6 @@ FactoryBot.define do depth { generate(:random_float) } product { |p| p.association(:base_product) } - option_values { [create(:option_value)] } # ensure stock item will be created for this variant before(:create) { create(:stock_location) if Spree::StockLocation.count.zero? } diff --git a/spec/fixtures/reports/orders_and_fulfillment/order_cycle_customer_totals_report.csv b/spec/fixtures/reports/orders_and_fulfillment/order_cycle_customer_totals_report.csv index ef3fc83faa..7eda73d726 100644 --- a/spec/fixtures/reports/orders_and_fulfillment/order_cycle_customer_totals_report.csv +++ b/spec/fixtures/reports/orders_and_fulfillment/order_cycle_customer_totals_report.csv @@ -1,2 +1,2 @@ Hub,Customer,Email,Phone,Producer,Product,Variant,Quantity,Item ($),Item + Fees ($),Admin & Handling ($),Ship ($),Pay fee ($),Total ($),Paid?,Shipping,Delivery?,Ship Street,Ship Street 2,Ship City,Ship Postcode,Ship State,Comments,SKU,Order Cycle,Payment Method,Customer Code,Tags,Billing Street,Billing Street 2,Billing City,Billing Postcode,Billing State,Order number,Date -Apple Market,John Doe,john@example.net,123-456-7890,Apple Farmer,Apples,"1g, S",1,10.0,10.0,"","","","",false,UPS Ground,true,10 Lovely Street,Northwest,Herndon,20170,Victoria,"",APP,,,JHN,"",10 Lovely Street,Northwest,Herndon,20170,Victoria,R644360121,2022-05-26 00:00:00 +Apple Market,John Doe,john@example.net,123-456-7890,Apple Farmer,Apples,1g,1,10.0,10.0,"","","","",false,UPS Ground,true,10 Lovely Street,Northwest,Herndon,20170,Victoria,"",APP,,,JHN,"",10 Lovely Street,Northwest,Herndon,20170,Victoria,R644360121,2022-05-26 00:00:00 diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb index 58047b1224..1d4bc777af 100644 --- a/spec/lib/spree/core/product_duplicator_spec.rb +++ b/spec/lib/spree/core/product_duplicator_spec.rb @@ -9,8 +9,7 @@ describe Spree::Core::ProductDuplicator do taxons: [], product_properties: [property], master: master_variant, - variants: [variant], - option_types: [] + variants: [variant] end let(:new_product) do @@ -89,7 +88,6 @@ describe Spree::Core::ProductDuplicator do 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_master_variant) - expect(new_product).to receive(:option_types=).with([]) expect(new_product).to receive(:variants=).with([new_variant]) expect(new_master_variant).to receive(:sku=).with("") diff --git a/spec/lib/tasks/data/remove_transient_data_spec.rb b/spec/lib/tasks/data/remove_transient_data_spec.rb index a26673b805..9cd1091b7c 100644 --- a/spec/lib/tasks/data/remove_transient_data_spec.rb +++ b/spec/lib/tasks/data/remove_transient_data_spec.rb @@ -63,13 +63,6 @@ describe RemoveTransientData do expect{ old_line_item.reload }.to raise_error ActiveRecord::RecordNotFound expect{ old_adjustment.reload }.to raise_error ActiveRecord::RecordNotFound end - - it "removes any defunct line item option value records" do - line_item.delete - - expect{ RemoveTransientData.new.call }. - to change{ Spree::OptionValuesLineItem.count }.by(-1) - end end end end diff --git a/spec/models/spree/ability_spec.rb b/spec/models/spree/ability_spec.rb index 68b45fdbcd..7f87ff62d9 100644 --- a/spec/models/spree/ability_spec.rb +++ b/spec/models/spree/ability_spec.rb @@ -71,20 +71,6 @@ describe Spree::Ability do end end - context 'for OptionType' do - let(:resource) { Spree::OptionType.new } - context 'requested by any user' do - it_should_behave_like 'read only' - end - end - - context 'for OptionValue' do - let(:resource) { Spree::OptionType.new } - context 'requested by any user' do - it_should_behave_like 'read only' - end - end - context 'for Order' do let(:resource) { Spree::Order.new } diff --git a/spec/models/spree/line_item_spec.rb b/spec/models/spree/line_item_spec.rb index f6db90d611..082d6fbd0f 100644 --- a/spec/models/spree/line_item_spec.rb +++ b/spec/models/spree/line_item_spec.rb @@ -178,12 +178,12 @@ module Spree before do li1 - li2 + li2.reload li1.adjustments << adjustment1 end it "finds line items with tax" do - expect(LineItem.with_tax).to eq([li1]) + expect(LineItem.with_tax.to_a).to eq([li1]) end it "finds line items without tax" do @@ -708,47 +708,41 @@ module Spree end end - context "when the line_item already has a final_weight_volume set (and all required option values do not exist)" do + context "when the line_item has a final_weight_volume set" 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 + it "assigns the new value" do + expect(li.unit_presentation).to eq "5g" + expect(v.unit_presentation).to eq "10g bar" + allow(li).to receive(:unit_description) { 'foo' } - expect { - li.update_attribute(:final_weight_volume, 10) - }.to change(Spree::OptionValue, :count).by(1) + li.update_attribute(:final_weight_volume, 10) - expect(li.option_values).not_to include ov_orig - expect(li.option_values).not_to include ov_var - ov = li.option_values.last - expect(ov.name).to eq("10g foo") + expect(li.unit_presentation).to eq "10g foo" end end - context "when the variant already has a value set (and all required option values exist)" do + context "when the variant already has a value set" 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 + it "assigns the new value" do + expect(li.unit_presentation).to eq "5g" + expect(v.unit_presentation).to eq "10g bar" + allow(li).to receive(:unit_description) { 'bar' } - expect { - li.update_attribute(:final_weight_volume, 10) - }.to change(Spree::OptionValue, :count).by(0) + li.update_attribute(:final_weight_volume, 10) - expect(li.option_values).not_to include ov_orig - expect(li.option_values).to include ov_new + expect(li.unit_presentation).to eq "10g bar" end end @@ -792,24 +786,6 @@ module Spree 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 - describe "when the associated variant is soft-deleted" do let!(:variant) { create(:variant) } let!(:line_item) { create(:line_item, variant: variant) } diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 63f8e1b9a0..c3edb52816 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -881,7 +881,7 @@ module Spree end describe "variant units" do - context "when the product already has a variant unit set (and all required option types exist)" do + context "when the product already has a variant unit set" do let!(:p) { create(:simple_product, variant_unit: 'weight', @@ -889,84 +889,27 @@ module Spree variant_unit_name: nil) } - let!(:ot_volume) { create(:option_type, name: 'unit_volume', presentation: 'Volume') } - - it "removes the old option type and assigns the new one" do - p.update!(variant_unit: 'volume', variant_unit_scale: 0.001) - expect(p.option_types).to eq([ot_volume]) - end - - it "does not remove and re-add the option type if it is not changed" do - expect(p.option_types).to receive(:delete).never - p.update!(name: 'foo') - end - - it "removes the related option values from all its variants and replaces them" do - ot = Spree::OptionType.find_by name: 'unit_weight' + it "updates its variants unit values" do v = create(:variant, unit_value: 1, product: p) p.reload - expect(v.option_values.map(&:name).include?("1L")).to eq(false) - expect(v.option_values.map(&:name).include?("1g")).to eq(true) - expect { - p.update!(variant_unit: 'volume', variant_unit_scale: 0.001) - }.to change(p.master.option_values.reload, :count).by(0) - v.reload - expect(v.option_values.map(&:name).include?("1L")).to eq(true) - expect(v.option_values.map(&:name).include?("1g")).to eq(false) + expect(v.unit_presentation).to eq "1g" + + p.update!(variant_unit: 'volume', variant_unit_scale: 0.001) + + expect(v.reload.unit_presentation).to eq "1L" end - it "removes the related option values from its master variant and replaces them" do - ot = Spree::OptionType.find_by name: 'unit_weight' + it "updates its master variant's unit values" do p.master.update!(unit_value: 1) p.reload - expect(p.master.option_values.map(&:name).include?("1L")).to eq(false) - expect(p.master.option_values.map(&:name).include?("1g")).to eq(true) - expect { - p.update!(variant_unit: 'volume', variant_unit_scale: 0.001) - }.to change(p.master.option_values.reload, :count).by(0) + expect(p.master.unit_presentation).to eq "1g" + + p.update!(variant_unit: 'volume', variant_unit_scale: 0.001) p.reload - expect(p.master.option_values.map(&:name).include?("1L")).to eq(true) - expect(p.master.option_values.map(&:name).include?("1g")).to eq(false) - end - end - it "finds all variant unit option types" do - ot1 = create(:option_type, name: 'unit_weight', presentation: 'Weight') - ot2 = create(:option_type, name: 'unit_volume', presentation: 'Volume') - ot3 = create(:option_type, name: 'unit_items', presentation: 'Items') - ot4 = create(:option_type, name: 'foo_unit_bar', presentation: 'Foo') - - expect(Spree::Product.all_variant_unit_option_types).to match_array [ot1, ot2, ot3] - end - end - - describe "option types" do - describe "removing an option type" do - it "removes the associated option values from all variants" do - # Given a product with a variant unit option type and values - p = create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) - v1 = create(:variant, product: p, unit_value: 100, option_values: []) - v2 = create(:variant, product: p, unit_value: 200, option_values: []) - - # And a custom option type and values - ot = create(:option_type, name: 'foo', presentation: 'foo') - p.option_types << ot - ov1 = create(:option_value, option_type: ot, name: 'One', presentation: 'One') - ov2 = create(:option_value, option_type: ot, name: 'Two', presentation: 'Two') - v1.option_values << ov1 - v2.option_values << ov2 - - # When we remove the custom option type - p.option_type_ids = p.option_type_ids.reject { |id| id == ot.id } - - # Then the associated option values should have been removed from the variants - expect(v1.option_values.reload).not_to include ov1 - expect(v2.option_values.reload).not_to include ov2 - - # And the option values themselves should still exist - expect(Spree::OptionValue.where(id: [ov1.id, ov2.id]).count).to eq(2) + expect(p.master.unit_presentation).to eq "1L" end end end diff --git a/spec/models/spree/shipment_spec.rb b/spec/models/spree/shipment_spec.rb index f1f13edbb7..d9c4c5f295 100644 --- a/spec/models/spree/shipment_spec.rb +++ b/spec/models/spree/shipment_spec.rb @@ -48,7 +48,7 @@ describe Spree::Shipment do context "manifest" do let(:order) { Spree::Order.create } - let(:variant) { create(:variant) } + let!(:variant) { create(:variant) } let!(:line_item) { order.contents.add variant } let!(:shipment) { order.create_proposed_shipments.first } diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 20c39737f8..72c897eb79 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -567,8 +567,7 @@ describe Spree::Variant do end describe "unit value/description" do - let(:v) { Spree::Variant.new(option_values: [option_value]) } - let(:option_value) { build(:option_value, presentation: "small") } + let(:v) { Spree::Variant.new(unit_presentation: "small" ) } describe "generating the full name" do it "returns unit_to_display when display_name is blank" do @@ -583,7 +582,7 @@ describe Spree::Variant do it "returns unit_to_display when it contains display_name" do v.display_name = "small" - v.option_values[0].presentation = "small size" + v.unit_presentation = "small size" expect(v.full_name).to eq "small size" end @@ -594,7 +593,7 @@ describe Spree::Variant do it "is resilient to regex chars" do v.display_name = ")))" - v.option_values[0].presentation = ")))" + v.unit_presentation = ")))" expect(v.full_name).to eq(")))") end end @@ -665,38 +664,16 @@ describe Spree::Variant do end end - context "when the variant already has a value set (and all required option values do not exist)" do + context "when the variant already has a value set" 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 + it "assigns the new option value" do + expect(v.unit_presentation).to eq "5g bar" - expect { - v.update!(unit_value: 10, unit_description: 'foo') - }.to change(Spree::OptionValue, :count).by(1) + v.update!(unit_value: 10, unit_description: 'foo') - expect(v.option_values).not_to include ov_orig - 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') } - - 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!(unit_value: 10, unit_description: 'foo') - }.to change(Spree::OptionValue, :count).by(0) - - expect(v.option_values).not_to include ov_orig - expect(v.option_values).to include ov_new + expect(v.unit_presentation).to eq "10g foo" end end @@ -706,11 +683,10 @@ describe Spree::Variant do create(:variant, product: p, unit_value: 5, unit_description: 'bar', display_as: '') } - it "requests the name of the new option_value from OptionValueName" do + it "requests the new value from OptionValueName" do expect_any_instance_of(VariantUnits::OptionValueNamer).to receive(:name).exactly(1).times.and_call_original v.update(unit_value: 10, unit_description: 'foo') - ov = v.option_values.last - expect(ov.name).to eq("10g foo") + expect(v.unit_presentation).to eq "10g foo" end end @@ -720,35 +696,14 @@ describe Spree::Variant do create(:variant, product: p, unit_value: 5, unit_description: 'bar', display_as: 'FOOS!') } - it "does not request the name of the new option_value from OptionValueName" do + it "does not request the new value from OptionValueName" do expect_any_instance_of(VariantUnits::OptionValueNamer).not_to receive(:name) v.update!(unit_value: 10, unit_description: 'foo') - ov = v.option_values.last - expect(ov.name).to eq("FOOS!") + expect(v.unit_presentation).to eq("FOOS!") end end end - describe "deleting unit option values" do - before do - p = create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) - ot = Spree::OptionType.find_by name: 'unit_weight' - @v = create(:variant, product: p) - end - - it "removes option value associations for unit option types" do - expect { - @v.delete_unit_option_values - }.to change(@v.option_values, :count).by(-1) - end - - it "does not delete option values" do - expect { - @v.delete_unit_option_values - }.to change(Spree::OptionValue, :count).by(0) - end - end - context "extends LocalizedNumber" do subject! { build_stubbed(:variant) } diff --git a/spec/services/variant_units/option_value_namer_spec.rb b/spec/services/variant_units/option_value_namer_spec.rb index 936edbbfb6..a902d35306 100644 --- a/spec/services/variant_units/option_value_namer_spec.rb +++ b/spec/services/variant_units/option_value_namer_spec.rb @@ -6,34 +6,35 @@ module VariantUnits describe OptionValueNamer do describe "generating option value name" do let(:v) { Spree::Variant.new } - let(:subject) { OptionValueNamer.new } + let(:p) { Spree::Product.new } + let(:subject) { OptionValueNamer.new(v) } it "when description is blank" do allow(v).to receive(:unit_description) { nil } allow(subject).to receive(:value_scaled?) { true } allow(subject).to receive(:option_value_value_unit) { %w(value unit) } - expect(subject.name(v)).to eq("valueunit") + expect(subject.name).to eq("valueunit") end it "when description is present" do allow(v).to receive(:unit_description) { 'desc' } allow(subject).to receive(:option_value_value_unit) { %w(value unit) } allow(subject).to receive(:value_scaled?) { true } - expect(subject.name(v)).to eq("valueunit desc") + expect(subject.name).to eq("valueunit desc") end it "when value is blank and description is present" do allow(v).to receive(:unit_description) { 'desc' } allow(subject).to receive(:option_value_value_unit) { [nil, nil] } allow(subject).to receive(:value_scaled?) { true } - expect(subject.name(v)).to eq("desc") + expect(subject.name).to eq("desc") end it "spaces value and unit when value is unscaled" do allow(v).to receive(:unit_description) { nil } allow(subject).to receive(:option_value_value_unit) { %w(value unit) } allow(subject).to receive(:value_scaled?) { false } - expect(subject.name(v)).to eq("value unit") + expect(subject.name).to eq("value unit") end end @@ -64,6 +65,7 @@ module VariantUnits it "generates simple values" do p = double(:product, variant_unit: 'weight', variant_unit_scale: 1.0) allow(v).to receive(:product) { p } + allow(p).to receive(:persisted?) { true } allow(v).to receive(:unit_value) { 100 } expect(subject.send(:option_value_value_unit)).to eq [100, 'g'] @@ -72,6 +74,7 @@ module VariantUnits it "generates values when unit value is non-integer" do p = double(:product, variant_unit: 'weight', variant_unit_scale: 1.0) allow(v).to receive(:product) { p } + allow(p).to receive(:persisted?) { true } allow(v).to receive(:unit_value) { 123.45 } expect(subject.send(:option_value_value_unit)).to eq [123.45, 'g'] @@ -80,6 +83,7 @@ module VariantUnits it "returns a value of 1 when unit value equals the scale" do p = double(:product, variant_unit: 'weight', variant_unit_scale: 1000.0) allow(v).to receive(:product) { p } + allow(p).to receive(:persisted?) { true } allow(v).to receive(:unit_value) { 1000.0 } expect(subject.send(:option_value_value_unit)).to eq [1, 'kg'] @@ -88,6 +92,7 @@ module VariantUnits it "returns only values that are in the same measurement systems" do p = double(:product, variant_unit: 'weight', variant_unit_scale: 1.0) allow(v).to receive(:product) { p } + allow(p).to receive(:persisted?) { true } allow(v).to receive(:unit_value) { 500 } # 500g would convert to > 1 pound, but we don't want the namer to use # pounds since it's in a different measurement system. @@ -99,6 +104,7 @@ module VariantUnits [1_000_000.0, 'T']].each do |scale, unit| p = double(:product, variant_unit: 'weight', variant_unit_scale: scale) allow(v).to receive(:product) { p } + allow(p).to receive(:persisted?) { true } allow(v).to receive(:unit_value) { 10.0 * scale } expect(subject.send(:option_value_value_unit)).to eq [10, unit] end @@ -108,6 +114,7 @@ module VariantUnits [[0.001, 'mL'], [1.0, 'L'], [1000.0, 'kL']].each do |scale, unit| p = double(:product, variant_unit: 'volume', variant_unit_scale: scale) allow(v).to receive(:product) { p } + allow(p).to receive(:persisted?) { true } allow(v).to receive(:unit_value) { 100 * scale } expect(subject.send(:option_value_value_unit)).to eq [100, unit] end @@ -116,6 +123,7 @@ module VariantUnits it "chooses the correct scale when value is very small" do p = double(:product, variant_unit: 'volume', variant_unit_scale: 0.001) allow(v).to receive(:product) { p } + allow(p).to receive(:persisted?) { true } allow(v).to receive(:unit_value) { 0.0001 } expect(subject.send(:option_value_value_unit)).to eq [0.1, 'mL'] end @@ -125,6 +133,7 @@ module VariantUnits p = double(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: unit) allow(v).to receive(:product) { p } + allow(p).to receive(:persisted?) { true } allow(v).to receive(:unit_value) { 100 } expect(subject.send(:option_value_value_unit)).to eq [100, unit.pluralize] end @@ -134,6 +143,7 @@ module VariantUnits p = double(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: 'packet') allow(v).to receive(:product) { p } + allow(p).to receive(:persisted?) { true } allow(v).to receive(:unit_value) { 1 } expect(subject.send(:option_value_value_unit)).to eq [1, 'packet'] end @@ -150,6 +160,7 @@ module VariantUnits oz_scale = 28.35 p = double(:product, variant_unit: 'weight', variant_unit_scale: oz_scale) allow(v).to receive(:product) { p } + allow(p).to receive(:persisted?) { true } # The unit_value is stored rounded to 2 decimals allow(v).to receive(:unit_value) { (12.5 * oz_scale).round(2) } expect(subject.send(:option_value_value_unit)).to eq [BigDecimal(12.5, 6), 'oz'] diff --git a/spec/system/admin/bulk_order_management_spec.rb b/spec/system/admin/bulk_order_management_spec.rb index 6e3cff258a..7019e65f64 100644 --- a/spec/system/admin/bulk_order_management_spec.rb +++ b/spec/system/admin/bulk_order_management_spec.rb @@ -249,7 +249,7 @@ describe ' } let!(:li1) { create(:line_item_with_shipment, order: o1) } let!(:li2) { - create(:line_item_with_shipment, order: o2, product: create(:product_with_option_types) ) + create(:line_item_with_shipment, order: o2, product: create(:product) ) } before :each do @@ -428,8 +428,8 @@ describe ' end let!(:p1) { - create(:product_with_option_types, group_buy: true, group_buy_unit_size: 5000, - variant_unit: "weight", variants: [create(:variant, unit_value: 1000)] ) + create(:product, group_buy: true, group_buy_unit_size: 5000, + variant_unit: "weight", variants: [create(:variant, unit_value: 1000)] ) } let!(:v1) { p1.variants.first } let!(:o1) { @@ -1068,8 +1068,8 @@ describe ' let!(:li1) { create(:line_item_with_shipment, order: o1 ) } let!(:li2) { create(:line_item_with_shipment, order: o2 ) } let!(:p3) { - create(:product_with_option_types, group_buy: true, group_buy_unit_size: 5000, - variant_unit: "weight", variants: [create(:variant, unit_value: 1000)] ) + create(:product, group_buy: true, group_buy_unit_size: 5000, + variant_unit: "weight", variants: [create(:variant, unit_value: 1000)] ) } let!(:v3) { p3.variants.first } let!(:o3) { diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb index 28abf72e1c..44121e5bd1 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -113,8 +113,7 @@ describe ' expect(product.shipping_category).to eq(shipping_category) expect(product.description).to eq("
A description...
") expect(product.group_buy).to be_falsey - expect(product.master.option_values.map(&:name)).to eq(['5kg']) - expect(product.master.options_text).to eq("5kg") + expect(product.master.unit_presentation).to eq("5kg") end it "creating an on-demand product" do diff --git a/spec/system/admin/reports/orders_and_fulfillment_spec.rb b/spec/system/admin/reports/orders_and_fulfillment_spec.rb index 72a425c35d..9a6642405a 100644 --- a/spec/system/admin/reports/orders_and_fulfillment_spec.rb +++ b/spec/system/admin/reports/orders_and_fulfillment_spec.rb @@ -237,12 +237,12 @@ describe "Orders And Fulfillment" do table = rows.map { |r| r.all("td").map { |c| c.text.strip } } expect(table).to include [ - "Supplier Name", "Baked Beans", "1g Big, S", + "Supplier Name", "Baked Beans", "1g Big", "3", "0.003", "10.0", "30.0" ] expect(table).to include [ - "Supplier Name", "Baked Beans", "1g Small, S", + "Supplier Name", "Baked Beans", "1g Small", "7", "0.007", "10.0", "70.0" ] expect(table[2]).to eq [ diff --git a/spec/system/admin/reports_spec.rb b/spec/system/admin/reports_spec.rb index 98edf6ed45..edde70b1e1 100644 --- a/spec/system/admin/reports_spec.rb +++ b/spec/system/admin/reports_spec.rb @@ -380,8 +380,6 @@ describe ' variant2.update!(sku: "sku2") variant3.on_hand = 9 variant3.update!(sku: "") - variant1.option_values = [create(:option_value, presentation: "Test")] - variant2.option_values = [create(:option_value, presentation: "Something")] end it "shows products and inventory report" do @@ -399,12 +397,12 @@ describe ' expect(page).to have_table_row [product1.supplier.name, product1.supplier.address.city, "Product Name", product1.properties.map(&:presentation).join(", "), - product1.primary_taxon.name, "Test", "100.0", + product1.primary_taxon.name, "1g", "100.0", "none", "", "sku1"] expect(page).to have_table_row [product1.supplier.name, product1.supplier.address.city, "Product Name", product1.properties.map(&:presentation).join(", "), - product1.primary_taxon.name, "Something", "80.0", + product1.primary_taxon.name, "1g", "80.0", "none", "", "sku2"] expect(page).to have_table_row [product2.supplier.name, product1.supplier.address.city, "Product 2", diff --git a/spec/system/admin/subscriptions_spec.rb b/spec/system/admin/subscriptions_spec.rb index 204dd50241..ffd4257cf0 100644 --- a/spec/system/admin/subscriptions_spec.rb +++ b/spec/system/admin/subscriptions_spec.rb @@ -209,11 +209,11 @@ describe 'Subscriptions' do } let!(:test_product) { create(:product, supplier: shop) } let!(:test_variant) { - create(:variant, product: test_product, unit_value: "100", price: 12.00, option_values: []) + create(:variant, product: test_product, unit_value: "100", price: 12.00) } let!(:shop_product) { create(:product, supplier: shop) } let!(:shop_variant) { - create(:variant, product: shop_product, unit_value: "1000", price: 6.00, option_values: []) + create(:variant, product: shop_product, unit_value: "1000", price: 6.00) } let!(:enterprise_fee) { create(:enterprise_fee, amount: 1.75) } let!(:order_cycle) { @@ -453,13 +453,13 @@ describe 'Subscriptions' do let!(:product2) { create(:product, supplier: shop) } let!(:product3) { create(:product, supplier: shop) } let!(:variant1) { - create(:variant, product: product1, unit_value: '100', price: 12.00, option_values: []) + create(:variant, product: product1, unit_value: '100', price: 12.00) } let!(:variant2) { - create(:variant, product: product2, unit_value: '1000', price: 6.00, option_values: []) + create(:variant, product: product2, unit_value: '1000', price: 6.00) } let!(:variant3) { - create(:variant, product: product3, unit_value: '10000', price: 22.00, option_values: []) + create(:variant, product: product3, unit_value: '10000', price: 22.00) } let!(:enterprise_fee) { create(:enterprise_fee, amount: 1.75) } let!(:order_cycle) { @@ -606,10 +606,10 @@ describe 'Subscriptions' do let!(:product1) { create(:product, supplier: shop) } let!(:product2) { create(:product, supplier: shop) } let!(:variant1) { - create(:variant, product: product1, unit_value: '100', price: 12.00, option_values: []) + create(:variant, product: product1, unit_value: '100', price: 12.00) } let!(:variant2) { - create(:variant, product: product2, unit_value: '1000', price: 6.00, option_values: []) + create(:variant, product: product2, unit_value: '1000', price: 6.00) } let!(:enterprise_fee) { create(:enterprise_fee, amount: 1.75) } let!(:order_cycle) { diff --git a/spec/system/admin/variant_overrides_spec.rb b/spec/system/admin/variant_overrides_spec.rb index fb7f2aa419..3229f4a2fc 100644 --- a/spec/system/admin/variant_overrides_spec.rb +++ b/spec/system/admin/variant_overrides_spec.rb @@ -74,11 +74,6 @@ describe " let!(:product_unrelated) { create(:simple_product, supplier: producer_unrelated) } - before do - # Remove 'S' option value - variant.option_values.first.destroy - end - context "when a hub is selected" do before do visit '/admin/inventory' diff --git a/spec/system/admin/variants_spec.rb b/spec/system/admin/variants_spec.rb index 293dced46f..df0d4326b3 100644 --- a/spec/system/admin/variants_spec.rb +++ b/spec/system/admin/variants_spec.rb @@ -127,9 +127,6 @@ describe ' variant = product.variants.first variant.update( unit_value: 1, unit_description: 'foo' ) - # And the product has option types for the unit-related and non-unit-related option values - product.option_types << variant.option_values.first.option_type - # When I view the variant login_as_admin visit spree.admin_product_variants_path product