From c80cba7fa5416fc0ecf11a6ec0fdb94712fe2b40 Mon Sep 17 00:00:00 2001 From: Rob H Date: Sun, 2 Jun 2013 08:52:41 +0530 Subject: [PATCH] BPUR: Don't use master variant to update attributes. Changes to product filtering. --- .../javascripts/admin/bulk_product_update.js | 29 ++++++----- app/models/spree/product_decorator.rb | 3 +- .../spree/admin/products/bulk_index.html.haml | 8 ++-- app/views/spree/admin/products/bulk_index.rep | 7 +-- spec/controllers/product_controller_spec.rb | 16 ++----- .../admin/bulk_product_update_spec.rb | 48 +++++++++---------- .../unit/bulk_product_update_spec.js | 27 +++++++++-- 7 files changed, 74 insertions(+), 64 deletions(-) diff --git a/app/assets/javascripts/admin/bulk_product_update.js b/app/assets/javascripts/admin/bulk_product_update.js index 8af03310e5..5b5acb30e7 100644 --- a/app/assets/javascripts/admin/bulk_product_update.js +++ b/app/assets/javascripts/admin/bulk_product_update.js @@ -50,16 +50,15 @@ productsApp.controller('AdminBulkProductsCtrl', function($scope, $timeout, $http }); }; - //accessible from scope - $scope.onHand = function(products){ - return onHand(products); + $scope.updateOnHand = function(product){ + product.on_hand = onHand(product); } $scope.updateStatusMessage = { text: "", style: {} } - + $scope.updateProducts = function(productsToSubmit){ $scope.displayUpdating(); $http({ @@ -226,12 +225,9 @@ function filterSubmitProducts(productsToFilter){ for (i in productsToFilter) { if (productsToFilter[i].hasOwnProperty("id")){ var filteredProduct = {}; - filteredProduct.id = productsToFilter[i].id; - if (productsToFilter[i].hasOwnProperty("supplier_id")) filteredProduct.supplier_id = productsToFilter[i].supplier_id; - if (productsToFilter[i].hasOwnProperty("name")) filteredProduct.name = productsToFilter[i].name; - if (productsToFilter[i].hasOwnProperty("available_on")) filteredProduct.available_on = productsToFilter[i].available_on; + var filteredVariants = []; + if (productsToFilter[i].hasOwnProperty("variants")){ - var filteredVariants = []; for (j in productsToFilter[i].variants){ if (productsToFilter[i].variants[j].deleted_at == null && productsToFilter[i].variants[j].hasOwnProperty("id")){ filteredVariants[j] = {}; @@ -240,10 +236,19 @@ function filterSubmitProducts(productsToFilter){ if (productsToFilter[i].variants[j].hasOwnProperty("price")) filteredVariants[j].price = productsToFilter[i].variants[j].price; } } - filteredProduct.variants_attributes = filteredVariants; // Note that the name of the property changes to enable mass assignment of variants attributes with rails } - if (productsToFilter[i].hasOwnProperty("master")) filteredProduct.master_attributes = productsToFilter[i].master - filteredProducts.push(filteredProduct); + + var hasUpdatableProperty = false; + filteredProduct.id = productsToFilter[i].id; + if (productsToFilter[i].hasOwnProperty("name")) { filteredProduct.name = productsToFilter[i].name; hasUpdatableProperty = true; } + if (productsToFilter[i].hasOwnProperty("supplier_id")) { filteredProduct.supplier_id = productsToFilter[i].supplier_id; hasUpdatableProperty = true; } + //if (productsToFilter[i].hasOwnProperty("master")) filteredProduct.master_attributes = productsToFilter[i].master + if (productsToFilter[i].hasOwnProperty("price")) { filteredProduct.price = productsToFilter[i].price; hasUpdatableProperty = true; } + if (productsToFilter[i].hasOwnProperty("on_hand") && filteredVariants.length == 0) { filteredProduct.on_hand = productsToFilter[i].on_hand; hasUpdatableProperty = true; } //only update if no variants present + if (productsToFilter[i].hasOwnProperty("available_on")) { filteredProduct.available_on = productsToFilter[i].available_on; hasUpdatableProperty = true; } + if (filteredVariants.length > 0) { filteredProduct.variants_attributes = filteredVariants; hasUpdatableProperty = true; } // Note that the name of the property changes to enable mass assignment of variants attributes with rails + + if (hasUpdatableProperty) filteredProducts.push(filteredProduct); } } } diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb index 80464ce86d..cd809d880e 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -5,9 +5,8 @@ Spree::Product.class_eval do has_many :distributors, :through => :product_distributions accepts_nested_attributes_for :product_distributions, :allow_destroy => true - accepts_nested_attributes_for :master, :allow_destroy => false - attr_accessible :supplier_id, :distributor_ids, :product_distributions_attributes, :group_buy, :group_buy_unit_size, :master_attributes + attr_accessible :supplier_id, :distributor_ids, :product_distributions_attributes, :group_buy, :group_buy_unit_size validates_presence_of :supplier diff --git a/app/views/spree/admin/products/bulk_index.html.haml b/app/views/spree/admin/products/bulk_index.html.haml index cc8a163cf8..871b9939df 100644 --- a/app/views/spree/admin/products/bulk_index.html.haml +++ b/app/views/spree/admin/products/bulk_index.html.haml @@ -27,10 +27,10 @@ %td %select.select2{ :name => 'supplier_id', 'ng-model' => 'product.supplier_id', 'ng-options' => 's.id as s.name for s in suppliers' } %td - %input{ 'ng-model' => 'product.master.price', 'ng-decimal' => :true, :name => 'master_price', :type => 'text' } + %input{ 'ng-model' => 'product.price', 'ng-decimal' => :true, :name => 'price', :type => 'text' } %td - %span{ 'ng-bind' => 'onHand(product)', :name => 'on_hand', 'ng-show' => 'product.variants.length > 0' } - %input.field{ 'ng-model' => 'product.master.on_hand', :name => 'master_on_hand', 'ng-show' => 'product.variants.length == 0', :type => 'number' } + %span{ 'ng-bind' => 'product.on_hand', :name => 'on_hand', 'ng-show' => 'product.variants.length > 0' } + %input.field{ 'ng-model' => 'product.on_hand', :name => 'on_hand', 'ng-show' => 'product.variants.length == 0', :type => 'number' } %td %input{ 'ng-model' => 'product.available_on', :name => 'available_on', :type => 'text' } %tr{ 'ng-repeat' => 'variant in product.variants' } @@ -40,7 +40,7 @@ %td %input{ 'ng-model' => 'variant.price', 'ng-decimal' => :true, :name => 'variant_price', :type => 'text' } %td - %input.field{ 'ng-model' => 'variant.on_hand', :name => 'variant_on_hand', :type => 'number' } + %input.field{ 'ng-model' => 'variant.on_hand', 'ng-change' => 'updateOnHand(product)', :name => 'variant_on_hand', :type => 'number' } %td %input{:type => 'button', :value => 'Update', 'ng-click' => 'prepareProductsForSubmit()'} %span{ id: "update-status-message", 'ng-style' => 'updateStatusMessage.style' } diff --git a/app/views/spree/admin/products/bulk_index.rep b/app/views/spree/admin/products/bulk_index.rep index 4ad7766301..88c4163954 100644 --- a/app/views/spree/admin/products/bulk_index.rep +++ b/app/views/spree/admin/products/bulk_index.rep @@ -3,11 +3,8 @@ r.list_of :products, @collection do r.element :name r.element :supplier_id r.element :available_on, Proc.new{ |product| product.available_on.strftime("%F %T") } - r.element :master do - r.element :id - r.element :price - r.element :on_hand - end + r.element :price + r.element :on_hand r.list_of :variants, Proc.new{ |product| product.variants.sort { |a,b| a.id <=> b.id } } do r.element :id r.element :options_text diff --git a/spec/controllers/product_controller_spec.rb b/spec/controllers/product_controller_spec.rb index ac3c442466..1b7aafe5c9 100644 --- a/spec/controllers/product_controller_spec.rb +++ b/spec/controllers/product_controller_spec.rb @@ -13,7 +13,6 @@ describe Spree::Admin::ProductsController do p1 = FactoryGirl.create(:product) p2 = FactoryGirl.create(:product) spree_get :bulk_index, { format: :json } - #process :bulk_index, {:use_route=> :spree}, nil, nil, "GET" assigns[:collection].should_not be_empty assigns[:collection].should == [ p1, p2 ] @@ -33,11 +32,8 @@ describe Spree::Admin::ProductsController do "name" => p1.name, "supplier_id" => p1.supplier_id, "available_on" => p1.available_on.strftime("%F %T"), - "master" => { - "id" => p1.master.id, - "price" => p1.master.price.to_s, - "on_hand" => p1.master.on_hand - }, + "price" => p1.price.to_s, + "on_hand" => ( v11.on_hand + v12.on_hand + v13.on_hand ), "variants" => [ #ordered by id { "id" => v11.id, "options_text" => v11.options_text, "price" => v11.price.to_s, "on_hand" => v11.on_hand }, { "id" => v12.id, "options_text" => v12.options_text, "price" => v12.price.to_s, "on_hand" => v12.on_hand }, @@ -49,17 +45,13 @@ describe Spree::Admin::ProductsController do "name" => p2.name, "supplier_id" => p2.supplier_id, "available_on" => p2.available_on.strftime("%F %T"), - "master" => { - "id" => p2.master.id, - "price" => p2.master.price.to_s, - "on_hand" => p2.master.on_hand - }, + "price" => p2.price.to_s, + "on_hand" => v21.on_hand, "variants" => [ #ordered by id { "id" => v21.id, "options_text" => v21.options_text, "price" => v21.price.to_s, "on_hand" => v21.on_hand } ] } json_response = JSON.parse(response.body) - #json_response = Hash[json_response.map{ |k, v| [k.to_sym, v] }] json_response.should == [ p1r, p2r ] end end diff --git a/spec/features/admin/bulk_product_update_spec.rb b/spec/features/admin/bulk_product_update_spec.rb index 123b6233d5..3fd26e0da9 100644 --- a/spec/features/admin/bulk_product_update_spec.rb +++ b/spec/features/admin/bulk_product_update_spec.rb @@ -54,49 +54,49 @@ feature %q{ page.should have_field "available_on", with: p2.available_on.strftime("%F %T") end - it "displays a price input (for master variant) for each product" do + it "displays a price input for each product (ie. for master variant)" do p1 = FactoryGirl.create(:product) p2 = FactoryGirl.create(:product) - p1.master.price = 22.00 - p2.master.price = 44.00 + p1.price = 22.00 + p2.price = 44.00 p1.save! p2.save! visit '/admin/products/bulk_index' - page.should have_field "master_price", with: "22.0" - page.should have_field "master_price", with: "44.0" + page.should have_field "price", with: "22.0" + page.should have_field "price", with: "44.0" end - it "displays an on hand count input (for master variant) for each product if no other variants exist" do + it "displays an on hand count input for each product (ie. for master variant) if no regular variants exist" do p1 = FactoryGirl.create(:product) p2 = FactoryGirl.create(:product) - p1.master.on_hand = 15 - p2.master.on_hand = 12 + p1.on_hand = 15 + p2.on_hand = 12 p1.save! p2.save! visit '/admin/products/bulk_index' page.should_not have_selector "span[name='on_hand']", text: "0" - page.should have_field "master_on_hand", with: "15" - page.should have_field "master_on_hand", with: "12" + page.should have_field "on_hand", with: "15" + page.should have_field "on_hand", with: "12" end - it "displays an on hand count in a span (for master variant) for each product if other variants exist" do + it "displays an on hand count in a span for each product (ie. for master variant) if other variants exist" do p1 = FactoryGirl.create(:product) p2 = FactoryGirl.create(:product) v1 = FactoryGirl.create(:variant, product: p1, is_master: false, on_hand: 4) - p1.master.on_hand = 15 - p2.master.on_hand = 12 + p1.on_hand = 15 + p2.on_hand = 12 p1.save! p2.save! visit '/admin/products/bulk_index' - page.should_not have_field "master_on_hand", with: "15" + page.should_not have_field "on_hand", with: "15" page.should have_selector "span[name='on_hand']", text: "4" - page.should have_field "master_on_hand", with: "12" + page.should have_field "on_hand", with: "12" end end @@ -137,7 +137,7 @@ feature %q{ visit '/admin/products/bulk_index' - page.should have_field "master_price", with: "2.0" + page.should have_field "price", with: "2.0" page.should have_field "variant_price", with: "12.75" page.should have_field "variant_price", with: "2.5" end @@ -172,8 +172,8 @@ feature %q{ s1 = FactoryGirl.create(:supplier_enterprise) s2 = FactoryGirl.create(:supplier_enterprise) p = FactoryGirl.create(:product, supplier: s1, available_on: Date.today) - p.master.price = 10.0 - p.master.on_hand = 6; + p.price = 10.0 + p.on_hand = 6; p.save! login_to_admin_section @@ -183,14 +183,14 @@ feature %q{ page.should have_field "product_name", with: p.name page.should have_select "supplier_id", selected: s1.name page.should have_field "available_on", with: p.available_on.strftime("%F %T") - page.should have_field "master_price", with: "10.0" - page.should have_field "master_on_hand", with: "6" + page.should have_field "price", with: "10.0" + page.should have_field "on_hand", with: "6" fill_in "product_name", with: "Big Bag Of Potatoes" select(s2.name, :from => 'supplier_id') fill_in "available_on", with: (Date.today-3).strftime("%F %T") - fill_in "master_price", with: "20" - fill_in "master_on_hand", with: "18" + fill_in "price", with: "20" + fill_in "on_hand", with: "18" click_button 'Update' page.find("span#update-status-message").should have_content "Update complete" @@ -200,8 +200,8 @@ feature %q{ page.should have_field "product_name", with: "Big Bag Of Potatoes" page.should have_select "supplier_id", selected: s2.name page.should have_field "available_on", with: (Date.today-3).strftime("%F %T") - page.should have_field "master_price", with: "20.0" - page.should have_field "master_on_hand", with: "18" + page.should have_field "price", with: "20.0" + page.should have_field "on_hand", with: "18" end scenario "updating a product with variants" do diff --git a/spec/javascripts/unit/bulk_product_update_spec.js b/spec/javascripts/unit/bulk_product_update_spec.js index 47b623236e..d54ed9ab66 100644 --- a/spec/javascripts/unit/bulk_product_update_spec.js +++ b/spec/javascripts/unit/bulk_product_update_spec.js @@ -140,7 +140,7 @@ describe("Auxillary functions", function(){ expect( getDirtyObjects(c, d) ).toEqual( [ { id: 2, value: 12 }, { id: 3, value2: 15 } ] ); }); }); - + describe("filtering products", function(){ it("only accepts and returns an array", function(){ expect( filterSubmitProducts( [] ) ).toEqual([]); @@ -155,9 +155,14 @@ describe("Auxillary functions", function(){ expect( filterSubmitProducts( [ { id: 1, name: "p1" }, { notanid: 2, name: "p2"} ] ) ).toEqual( [ { id: 1, name: "p1" } ]); }); - it("returns variants as variants_attributes", function(){ + it("does not return a product object for products which have no propeties other than an id", function(){ + expect( filterSubmitProducts( [ { id: 1, someunwantedproperty: "something" }, { id: 2, name: "p2"} ] ) ).toEqual( [ { id: 2, name: "p2" } ]); + }); + + it("does not return an on_hand count when a product has variants", function(){ var testProduct = { id: 1, + on_hand: 5, variants: [ { id: 1, on_hand: 5, @@ -174,10 +179,10 @@ describe("Auxillary functions", function(){ } ] ); }); - it("returns master as master_attributes", function(){ + it("returns variants as variants_attributes", function(){ var testProduct = { id: 1, - master: [ { + variants: [ { id: 1, on_hand: 5, price: 12.0 @@ -185,7 +190,7 @@ describe("Auxillary functions", function(){ }; expect( filterSubmitProducts( [ testProduct ] ) ).toEqual( [ { id: 1, - master_attributes: [ { + variants_attributes: [ { id: 1, on_hand: 5, price: 12.0 @@ -222,6 +227,18 @@ describe("Auxillary functions", function(){ } ] ); }); + it("does not return variants_attributes property if variants is an empty array", function(){ + var testProduct = { + id: 1, + price: 10, + variants: [] + }; + expect( filterSubmitProducts( [ testProduct ] ) ).toEqual( [ { + id: 1, + price: 10 + } ] ); + }); + // TODO Not an exhaustive test, is there a better way to do this? it("only returns properties the properties of products which ought to be updated", function(){ var testProduct = {