From cbac916e668e075e049b4f967c8d5479aca67216 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Oct 2018 16:45:37 +0200 Subject: [PATCH] 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