From 2d60b3180e71a5ab08b4a38269e2d6d3368a47c8 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 4 Oct 2018 17:44:34 +0200 Subject: [PATCH 1/9] Wrap and improve comment block readability --- app/models/spree/product_set.rb | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb index 1a73ab00fe..368be9a556 100644 --- a/app/models/spree/product_set.rb +++ b/app/models/spree/product_set.rb @@ -3,11 +3,19 @@ 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? } From f54c69cbbab37caa0d3e448a01daa8981a7f7825 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 4 Oct 2018 18:21:53 +0200 Subject: [PATCH 2/9] Add first test case for ProductSet This covers creation and update of a product. --- spec/models/spree/product_set_spec.rb | 65 +++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 spec/models/spree/product_set_spec.rb diff --git a/spec/models/spree/product_set_spec.rb b/spec/models/spree/product_set_spec.rb new file mode 100644 index 0000000000..543debb0de --- /dev/null +++ b/spec/models/spree/product_set_spec.rb @@ -0,0 +1,65 @@ +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 + let!(:product) do + create( + :simple_product, + variant_unit: 'items', + variant_unit_scale: nil, + variant_unit_name: 'bunches' + ) + end + + let(:collection_hash) do + { + 0 => { + id: product.id, + variant_unit: 'weight', + variant_unit_scale: 1 + } + } + end + + it 'updates all the specified product attributes' do + product_set.save + + expect(product.reload.attributes).to include( + 'variant_unit' => 'weight', + 'variant_unit_scale' => 1 + ) + end + end + end + end +end From d43726504b5dbf23da780a3dd0d087c8781fd1ca Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 4 Oct 2018 19:59:26 +0200 Subject: [PATCH 3/9] Make #update_attributes parseable by humans As it is this is impossible to follow. --- app/models/spree/product_set.rb | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb index 368be9a556..3a40ea635d 100644 --- a/app/models/spree/product_set.rb +++ b/app/models/spree/product_set.rb @@ -17,14 +17,20 @@ class Spree::ProductSet < ModelSet # 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 ) + ( attributes.except(:id, :variants_attributes, :master_attributes).present? ? found_model.update_attributes(attributes.except(:id, :variants_attributes, :master_attributes)) : true) and + (attributes[:variants_attributes] ? update_variants_attributes(found_model, attributes[:variants_attributes]) : true ) and + (attributes[:master_attributes] ? update_variant(found_model, attributes[:master_attributes]) : true ) end end From 575d76e23e03fb40c59f80edf47faa71213ec0c1 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 4 Oct 2018 20:25:24 +0200 Subject: [PATCH 4/9] Cover variant creation and update with basic tests --- spec/models/spree/product_set_spec.rb | 30 +++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/spec/models/spree/product_set_spec.rb b/spec/models/spree/product_set_spec.rb index 543debb0de..a53b9817e1 100644 --- a/spec/models/spree/product_set_spec.rb +++ b/spec/models/spree/product_set_spec.rb @@ -59,6 +59,36 @@ describe Spree::ProductSet do 'variant_unit_scale' => 1 ) end + + context 'when :master_attributes is passed' do + 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 From a2228d4131aaf80639df9f67ce276dddeb91e53f Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 4 Oct 2018 20:26:49 +0200 Subject: [PATCH 5/9] Make ProductSet parseable by humans Now it's imposible to understand what is really going on. Feels more like assembler than Ruby. --- app/models/model_set.rb | 7 +++-- app/models/spree/product_set.rb | 46 +++++++++++++++++++++++++++------ 2 files changed, 43 insertions(+), 10 deletions(-) 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 3a40ea635d..db5fbfda2c 100644 --- a/app/models/spree/product_set.rb +++ b/app/models/spree/product_set.rb @@ -28,9 +28,35 @@ class Spree::ProductSet < ModelSet if found_model.nil? @klass.new(attributes).save unless @reject_if.andand.call(attributes) else - ( attributes.except(:id, :variants_attributes, :master_attributes).present? ? found_model.update_attributes(attributes.except(:id, :variants_attributes, :master_attributes)) : true) and - (attributes[:variants_attributes] ? update_variants_attributes(found_model, attributes[:variants_attributes]) : true ) and - (attributes[:master_attributes] ? update_variant(found_model, 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) + if attributes.except(:id, :variants_attributes, :master_attributes).present? + product.update_attributes( + attributes.except(:id, :variants_attributes, :master_attributes) + ) + else + true + end + end + + def update_product_variants(product, attributes) + if attributes[:variants_attributes] + update_variants_attributes(product, attributes[:variants_attributes]) + else + true + end + end + + def update_product_master(product, attributes) + if attributes[:master_attributes] + update_variant(product, attributes[:master_attributes]) + else + true end end @@ -41,16 +67,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 From cbac916e668e075e049b4f967c8d5479aca67216 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Oct 2018 16:45:37 +0200 Subject: [PATCH 6/9] Validate unit value when updating variant_unit Variants whose product's variant_unit is weight or volume require a unit_value. --- app/models/spree/product_set.rb | 21 ++++- .../spree/admin/products_controller_spec.rb | 77 ++++++++++++++++--- spec/models/spree/product_set_spec.rb | 59 ++++++++------ 3 files changed, 119 insertions(+), 38 deletions(-) diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb index db5fbfda2c..a681f689f3 100644 --- a/app/models/spree/product_set.rb +++ b/app/models/spree/product_set.rb @@ -35,15 +35,28 @@ class Spree::ProductSet < ModelSet end def update_product_only_attributes(product, attributes) - if attributes.except(:id, :variants_attributes, :master_attributes).present? - product.update_attributes( - attributes.except(:id, :variants_attributes, :master_attributes) - ) + variant_related_attrs = [:id, :variants_attributes, :master_attributes] + + if attributes.except(*variant_related_attrs).present? + product.assign_attributes(attributes.except(*variant_related_attrs)) + + product.variants.each do |variant| + validate_presence_of_unit_value(product, variant) + end + + product.save if errors.empty? else true end 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) if attributes[:variants_attributes] update_variants_attributes(product, attributes[:variants_attributes]) 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 index a53b9817e1..7f03f594f3 100644 --- a/spec/models/spree/product_set_spec.rb +++ b/spec/models/spree/product_set_spec.rb @@ -32,35 +32,50 @@ describe Spree::ProductSet do end context 'when the product does exist' do - let!(:product) do - create( - :simple_product, - variant_unit: 'items', - variant_unit_scale: nil, - variant_unit_name: 'bunches' - ) - end + 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 + let(:collection_hash) do + { + 0 => { + id: product.id, + variant_unit: 'weight', + variant_unit_scale: 1 + } } - } - end + end - it 'updates all the specified product attributes' do - product_set.save + it 'does not update the product' do + product_set.save - expect(product.reload.attributes).to include( - 'variant_unit' => 'weight', - 'variant_unit_scale' => 1 - ) + 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 From c8c16f0e8a5f411cd694dc0b7510358b3d578062 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Oct 2018 16:54:22 +0200 Subject: [PATCH 7/9] Use Rails 3.2 validates syntax --- app/models/spree/variant_decorator.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 From 5bd375d422463fbc3a7104a78a57529568daceef Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Oct 2018 21:23:08 +0200 Subject: [PATCH 8/9] Favor early return over dumb else branch --- app/models/spree/product_set.rb | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb index a681f689f3..d8716b821f 100644 --- a/app/models/spree/product_set.rb +++ b/app/models/spree/product_set.rb @@ -36,18 +36,17 @@ class Spree::ProductSet < ModelSet def update_product_only_attributes(product, attributes) variant_related_attrs = [:id, :variants_attributes, :master_attributes] + product_related_attrs = attributes.except(*variant_related_attrs) - if attributes.except(*variant_related_attrs).present? - product.assign_attributes(attributes.except(*variant_related_attrs)) + return true if product_related_attrs.blank? - product.variants.each do |variant| - validate_presence_of_unit_value(product, variant) - end + product.assign_attributes(product_related_attrs) - product.save if errors.empty? - else - true + 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) @@ -58,19 +57,13 @@ class Spree::ProductSet < ModelSet end def update_product_variants(product, attributes) - if attributes[:variants_attributes] - update_variants_attributes(product, attributes[:variants_attributes]) - else - true - end + return true unless attributes[:variants_attributes] + update_variants_attributes(product, attributes[:variants_attributes]) end def update_product_master(product, attributes) - if attributes[:master_attributes] - update_variant(product, attributes[:master_attributes]) - else - true - end + return true unless attributes[:master_attributes] + update_variant(product, attributes[:master_attributes]) end def update_variants_attributes(product, variants_attributes) From 60d05a941c611954260b5f4fe0c3f4840068b4ea Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 10 Oct 2018 14:03:14 +0200 Subject: [PATCH 9/9] Fix variants with 'weight' and missing unit_value This adds a data migration to fix those cases. It defaults to showing 1 unit of the specified weight. That is, if the user chose Kg, it'll display 1 as unit. Note that migration logs the process in a log/migrate.log file separate from the regular Rails log/production.log file. When you run the migration you'll see something like: Fixing variants missing unit_value... Processing variant 12... Succesfully fixed variant 12 Done! This helps auditing the changes applied and debug any possible failure scenarios. You can delete it once all is ok. --- ...0093850_fix_variants_missing_unit_value.rb | 49 +++++++++++++++++++ db/schema.rb | 2 +- 2 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20181010093850_fix_variants_missing_unit_value.rb 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