diff --git a/app/models/model_set.rb b/app/models/model_set.rb index b73c15f7df..0236a00776 100644 --- a/app/models/model_set.rb +++ b/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 diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb index 1a73ab00fe..d8716b821f 100644 --- a/app/models/spree/product_set.rb +++ b/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 diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 4c7e6ed5ab..cefb7917da 100644 --- a/app/models/spree/variant_decorator.rb +++ b/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 diff --git a/db/migrate/20181010093850_fix_variants_missing_unit_value.rb b/db/migrate/20181010093850_fix_variants_missing_unit_value.rb new file mode 100644 index 0000000000..c4226d0dc2 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 91ef541a32..c29fa95b9e 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/controllers/spree/admin/products_controller_spec.rb b/spec/controllers/spree/admin/products_controller_spec.rb index d5b1f861b5..35fdf054c4 100644 --- a/spec/controllers/spree/admin/products_controller_spec.rb +++ b/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 diff --git a/spec/models/spree/product_set_spec.rb b/spec/models/spree/product_set_spec.rb new file mode 100644 index 0000000000..7f03f594f3 --- /dev/null +++ b/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