Merge pull request #2845 from coopdevs/fix-invalid-variant-creation

Fix invalid variant creation

app/models/model_set.rb

@@ -30,8 +30,11 @@ class ModelSet
 
   def errors
     errors = ActiveModel::Errors.new self
-    full_messages = @collection.map { |ef| ef.errors.full_messages }.flatten
-    full_messages.each { |fm| errors.add(:base, fm) }
+    full_messages = @collection
+      .map { |model| model.errors.full_messages }
+      .flatten
+
+    full_messages.each { |message| errors.add(:base, message) }
     errors
   end
 

app/models/spree/product_set.rb

@@ -3,23 +3,69 @@ class Spree::ProductSet < ModelSet
     super(Spree::Product, [], attributes, proc { |attrs| attrs[:product_id].blank? })
   end
 
-  # A separate method of updating products was required due to an issue with the way Rails' assign_attributes and updates_attributes behave when delegated attributes of a nested
-  # object are updated via the parent object (ie. price of variants). Updating such attributes by themselves did not work using:
-  # product.update_attributes( { variants_attributes: [ { id: y, price: xx.x } ] } )
-  # and so an explicit call to update attributes on each individual variant was required. ie:
-  # variant.update_attributes( { price: xx.x } )
+  # A separate method of updating products was required due to an issue with
+  # the way Rails' assign_attributes and updates_attributes behave when
+  # delegated attributes of a nested object are updated via the parent object
+  # (ie. price of variants). Updating such attributes by themselves did not
+  # work using:
+  #
+  #   product.update_attributes(variants_attributes: [{ id: y, price: xx.x }])
+  #
+  # and so an explicit call to update attributes on each individual variant was
+  # required. ie:
+  #
+  #   variant.update_attributes( { price: xx.x } )
+  #
   def update_attributes(attributes)
-    attributes[:taxon_ids] = attributes[:taxon_ids].split(',')  if attributes[:taxon_ids].present?
-    e = @collection.detect { |e| e.id.to_s == attributes[:id].to_s && !e.id.nil? }
-    if e.nil?
+    if attributes[:taxon_ids].present?
+      attributes[:taxon_ids] = attributes[:taxon_ids].split(',')
+    end
+
+    found_model = @collection.find do |model|
+      model.id.to_s == attributes[:id].to_s && model.persisted?
+    end
+
+    if found_model.nil?
       @klass.new(attributes).save unless @reject_if.andand.call(attributes)
     else
-      ( attributes.except(:id, :variants_attributes, :master_attributes).present? ? e.update_attributes(attributes.except(:id, :variants_attributes, :master_attributes)) : true) and
-      (attributes[:variants_attributes] ? update_variants_attributes(e, attributes[:variants_attributes]) : true ) and
-      (attributes[:master_attributes] ? update_variant(e, attributes[:master_attributes]) : true )
+      update_product_only_attributes(found_model, attributes) &&
+        update_product_variants(found_model, attributes) &&
+        update_product_master(found_model, attributes)
     end
   end
 
+  def update_product_only_attributes(product, attributes)
+    variant_related_attrs = [:id, :variants_attributes, :master_attributes]
+    product_related_attrs = attributes.except(*variant_related_attrs)
+
+    return true if product_related_attrs.blank?
+
+    product.assign_attributes(product_related_attrs)
+
+    product.variants.each do |variant|
+      validate_presence_of_unit_value(product, variant)
+    end
+
+    product.save if errors.empty?
+  end
+
+  def validate_presence_of_unit_value(product, variant)
+    return unless %w(weight volume).include?(product.variant_unit)
+    return if variant.unit_value.present?
+
+    product.errors.add(:unit_value, "can't be blank")
+  end
+
+  def update_product_variants(product, attributes)
+    return true unless attributes[:variants_attributes]
+    update_variants_attributes(product, attributes[:variants_attributes])
+  end
+
+  def update_product_master(product, attributes)
+    return true unless attributes[:master_attributes]
+    update_variant(product, attributes[:master_attributes])
+  end
+
   def update_variants_attributes(product, variants_attributes)
     variants_attributes.each do |attributes|
       update_variant(product, attributes)
@@ -27,16 +73,20 @@ class Spree::ProductSet < ModelSet
   end
 
   def update_variant(product, variant_attributes)
-    e = product.variants_including_master.detect { |e| e.id.to_s == variant_attributes[:id].to_s && !e.id.nil? }
-    if e.present?
-      e.update_attributes(variant_attributes.except(:id))
+    found_variant = product.variants_including_master.find do |variant|
+      variant.id.to_s == variant_attributes[:id].to_s && variant.persisted?
+    end
+
+    if found_variant.present?
+      found_variant.update_attributes(variant_attributes.except(:id))
     else
-      product.variants.create variant_attributes
+      product.variants.create(variant_attributes)
     end
   end
 
   def collection_attributes=(attributes)
-    @collection = Spree::Product.where( :id => attributes.each_value.map{ |p| p[:id] } )
+    @collection = Spree::Product
+      .where(id: attributes.each_value.map { |product| product[:id] })
     @collection_hash = attributes
   end
 

app/models/spree/variant_decorator.rb

@@ -18,11 +18,13 @@ Spree::Variant.class_eval do
   attr_accessible :unit_value, :unit_description, :images_attributes, :display_as, :display_name, :import_date
   accepts_nested_attributes_for :images
 
-  validates_presence_of :unit_value,
-    if: -> v { %w(weight volume).include? v.product.andand.variant_unit }
+  validates :unit_value, presence: true, if: -> (variant) {
+    %w(weight volume).include?(variant.product.andand.variant_unit)
+  }
 
-  validates_presence_of :unit_description,
-    if: -> v { v.product.andand.variant_unit.present? && v.unit_value.nil? }
+  validates :unit_description, presence: true, if: -> (variant) {
+    variant.product.andand.variant_unit.present? && variant.unit_value.nil?
+  }
 
   before_validation :update_weight_from_unit_value, if: -> v { v.product.present? }
   after_save :update_units

db/migrate/20181010093850_fix_variants_missing_unit_value.rb

@@ -0,0 +1,49 @@
+# Fixes variants whose product.variant_unit is 'weight' and miss a unit_value,
+# showing 1 unit of the specified weight. That is, if the user chose Kg, it'll
+# display 1 as unit.
+class FixVariantsMissingUnitValue < ActiveRecord::Migration
+  HUMAN_UNIT_VALUE = 1
+
+  def up
+    logger.info "Fixing variants missing unit_value...\n"
+
+    variants_missing_unit_value.find_each do |variant|
+      logger.info "Processing variant #{variant.id}..."
+
+      fix_unit_value(variant)
+    end
+
+    logger.info "Done!"
+  end
+
+  def down
+  end
+
+  private
+
+  def variants_missing_unit_value
+    Spree::Variant
+      .joins(:product)
+      .readonly(false)
+      .where(
+        spree_products: { variant_unit: 'weight' },
+        spree_variants: { unit_value: nil }
+    )
+  end
+
+  def fix_unit_value(variant)
+    variant.unit_value = HUMAN_UNIT_VALUE * variant.product.variant_unit_scale
+
+    if variant.save
+      logger.info "Successfully fixed variant #{variant.id}"
+    else
+      logger.info "Failed fixing variant #{variant.id}"
+    end
+
+    logger.info ""
+  end
+
+  def logger
+    @logger ||= Logger.new('log/migrate.log')
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended to check this file into your version control system.
 
-ActiveRecord::Schema.define(:version => 20180919102548) do
+ActiveRecord::Schema.define(:version => 20181010093850) do
 
   create_table "account_invoices", :force => true do |t|
     t.integer  "user_id",    :null => false

spec/controllers/spree/admin/products_controller_spec.rb

@@ -1,22 +1,75 @@
 require 'spec_helper'
 
 describe Spree::Admin::ProductsController, type: :controller do
-  describe "updating a product we do not have access to" do
-    let(:s_managed) { create(:enterprise) }
-    let(:s_unmanaged) { create(:enterprise) }
-    let(:p) { create(:simple_product, supplier: s_unmanaged, name: 'Peas') }
+  describe 'bulk_update' do
+    context "updating a product we do not have access to" do
+      let(:s_managed) { create(:enterprise) }
+      let(:s_unmanaged) { create(:enterprise) }
+      let(:product) do
+        create(:simple_product, supplier: s_unmanaged, name: 'Peas')
+      end
 
-    before do
-      login_as_enterprise_user [s_managed]
-      spree_post :bulk_update, {"products" => [{"id" => p.id, "name" => "Pine nuts"}]}
+      before do
+        login_as_enterprise_user [s_managed]
+        spree_post :bulk_update, {
+          "products" => [{"id" => product.id, "name" => "Pine nuts"}]
+        }
+      end
+
+      it "denies access" do
+        response.should redirect_to spree.unauthorized_url
+      end
+
+      it "does not update any product" do
+        product.reload.name.should_not == "Pine nuts"
+      end
     end
 
-    it "denies access" do
-      response.should redirect_to spree.unauthorized_url
-    end
+    context "when changing a product's variant_unit" do
+      let(:producer) { create(:enterprise) }
+      let!(:product) do
+        create(
+          :simple_product,
+          supplier: producer,
+          variant_unit: 'items',
+          variant_unit_scale: nil,
+          variant_unit_name: 'bunches',
+          unit_value: nil,
+          unit_description: 'some description'
+        )
+      end
 
-    it "does not update any product" do
-      p.reload.name.should_not == "Pine nuts"
+      before { login_as_enterprise_user([producer]) }
+
+      it 'fails' do
+        spree_post :bulk_update, {
+          "products" => [
+            {
+              "id" => product.id,
+              "variant_unit" => "weight",
+              "variant_unit_scale" => 1
+            }
+          ]
+        }
+
+        expect(response).to have_http_status(400)
+      end
+
+      it 'does not redirect to bulk_products' do
+        spree_post :bulk_update, {
+          "products" => [
+            {
+              "id" => product.id,
+              "variant_unit" => "weight",
+              "variant_unit_scale" => 1
+            }
+          ]
+        }
+
+        expect(response).not_to redirect_to(
+          '/api/products/bulk_products?page=1;per_page=500;'
+        )
+      end
     end
   end
 

spec/models/spree/product_set_spec.rb

@@ -0,0 +1,110 @@
+require 'spec_helper'
+
+describe Spree::ProductSet do
+  describe '#save' do
+    context 'when passing :collection_attributes' do
+      let(:product_set) do
+        described_class.new(collection_attributes: collection_hash)
+      end
+
+      context 'when the product does not exist yet' do
+        let(:collection_hash) do
+          {
+            0 => {
+              product_id: 11,
+              name: 'a product',
+              price: 2.0,
+              supplier_id: create(:enterprise).id,
+              primary_taxon_id: create(:taxon).id,
+              unit_description: 'description',
+              variant_unit: 'items',
+              variant_unit_name: 'bunches'
+            }
+          }
+        end
+
+        it 'creates it with the specified attributes' do
+          product_set.save
+
+          expect(Spree::Product.last.attributes)
+            .to include('name' => 'a product')
+        end
+      end
+
+      context 'when the product does exist' do
+        context 'when a different varian_unit is passed' do
+          let!(:product) do
+            create(
+              :simple_product,
+              variant_unit: 'items',
+              variant_unit_scale: nil,
+              variant_unit_name: 'bunches',
+              unit_value: nil,
+              unit_description: 'some description'
+            )
+          end
+
+          let(:collection_hash) do
+            {
+              0 => {
+                id: product.id,
+                variant_unit: 'weight',
+                variant_unit_scale: 1
+              }
+            }
+          end
+
+          it 'does not update the product' do
+            product_set.save
+
+            expect(product.reload.attributes).to include(
+              'variant_unit' => 'items'
+            )
+          end
+
+          it 'adds an error' do
+            product_set.save
+            expect(product_set.errors.get(:base))
+              .to include("Unit value can't be blank")
+          end
+
+          it 'returns false' do
+            expect(product_set.save).to eq(false)
+          end
+        end
+
+        context 'when :master_attributes is passed' do
+          let!(:product) { create(:simple_product) }
+          let(:collection_hash) { { 0 => { id: product.id } } }
+          let(:master_attributes) { { sku: '123' } }
+
+          before do
+            collection_hash[0][:master_attributes] = master_attributes
+          end
+
+          context 'and the variant does exist' do
+            let!(:variant) { create(:variant, product: product) }
+
+            before { master_attributes[:id] = variant.id }
+
+            it 'updates the attributes of the master variant' do
+              product_set.save
+              expect(variant.reload.sku).to eq('123')
+            end
+          end
+
+          context 'and the variant does not exist' do
+            let(:master_attributes) do
+              attributes_for(:variant).merge(sku: '123')
+            end
+
+            it 'creates it with the specified attributes' do
+              product_set.save
+              expect(Spree::Variant.last.sku).to eq('123')
+            end
+          end
+        end
+      end
+    end
+  end
+end