From 4b74e5035392e3f437855d3819ecc1ec6eb08c7f Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Sat, 8 Dec 2018 15:11:41 +0000 Subject: [PATCH 1/3] Make ProductsController#bulk_update work by making ProductSet#create_variant not mass-assigning the provided on_hand and on_demand values and set them after each variant is created --- app/models/spree/product_set.rb | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb index d8716b821f..c91241d494 100644 --- a/app/models/spree/product_set.rb +++ b/app/models/spree/product_set.rb @@ -63,16 +63,16 @@ class Spree::ProductSet < ModelSet def update_product_master(product, attributes) return true unless attributes[:master_attributes] - update_variant(product, attributes[:master_attributes]) + create_or_update_variant(product, attributes[:master_attributes]) end def update_variants_attributes(product, variants_attributes) variants_attributes.each do |attributes| - update_variant(product, attributes) + create_or_update_variant(product, attributes) end end - def update_variant(product, variant_attributes) + def create_or_update_variant(product, variant_attributes) found_variant = product.variants_including_master.find do |variant| variant.id.to_s == variant_attributes[:id].to_s && variant.persisted? end @@ -80,10 +80,20 @@ class Spree::ProductSet < ModelSet if found_variant.present? found_variant.update_attributes(variant_attributes.except(:id)) else - product.variants.create(variant_attributes) + create_variant(product, variant_attributes) end end + def create_variant(product, variant_attributes) + on_hand = variant_attributes.delete(:on_hand) + on_demand = variant_attributes.delete(:on_demand) + + variant = product.variants.create(variant_attributes) + + variant.on_demand = on_demand if on_demand.present? + variant.on_hand = on_hand.to_i if on_hand.present? + end + def collection_attributes=(attributes) @collection = Spree::Product .where(id: attributes.each_value.map { |product| product[:id] }) From 66e69edca432db0f26589d726168845264bbe47a Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Sat, 8 Dec 2018 22:59:38 +0000 Subject: [PATCH 2/3] Rename ProductOnDemand to ProductStock; move product.on_hand into it; add product.on_demand; and remove unnecessary on_demand= --- app/models/concerns/product_on_demand.rb | 10 ---- app/models/concerns/product_stock.rb | 22 +++++++ app/models/concerns/variant_stock.rb | 5 ++ app/models/spree/product_decorator.rb | 12 +--- knapsack_rspec_report.json | 2 +- .../models/concerns/product_on_demand_spec.rb | 28 --------- spec/models/concerns/product_stock_spec.rb | 57 +++++++++++++++++++ spec/models/concerns/variant_stock_spec.rb | 42 +++++++++----- 8 files changed, 114 insertions(+), 64 deletions(-) delete mode 100644 app/models/concerns/product_on_demand.rb create mode 100644 app/models/concerns/product_stock.rb delete mode 100644 spec/models/concerns/product_on_demand_spec.rb create mode 100644 spec/models/concerns/product_stock_spec.rb diff --git a/app/models/concerns/product_on_demand.rb b/app/models/concerns/product_on_demand.rb deleted file mode 100644 index 0ad101e7ca..0000000000 --- a/app/models/concerns/product_on_demand.rb +++ /dev/null @@ -1,10 +0,0 @@ -require 'active_support/concern' - -module ProductOnDemand - extend ActiveSupport::Concern - - def on_demand=(value) - raise 'cannot set on_demand of product with variants' if variants.any? - master.on_demand = value - end -end diff --git a/app/models/concerns/product_stock.rb b/app/models/concerns/product_stock.rb new file mode 100644 index 0000000000..f9ce20b177 --- /dev/null +++ b/app/models/concerns/product_stock.rb @@ -0,0 +1,22 @@ +require 'active_support/concern' + +module ProductStock + extend ActiveSupport::Concern + + def on_demand + if has_variants? + raise 'Cannot determine product on_demand value of product with multiple variants' if variants.size > 1 + variants.first.on_demand + else + master.on_demand + end + end + + def on_hand + if has_variants? + variants.map(&:on_hand).reduce(:+) + else + master.on_hand + end + end +end diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb index 9b636efb8f..f5b34d3d10 100644 --- a/app/models/concerns/variant_stock.rb +++ b/app/models/concerns/variant_stock.rb @@ -85,6 +85,11 @@ module VariantStock # https://github.com/openfoodfoundation/spree/commit/20b5ad9835dca7f41a40ad16c7b45f987eea6dcc def on_demand warn_deprecation(__method__, 'StockItem#backorderable?') + + # A variant that has not been saved yet, doesn't have a stock item + # This provides a default value for variant.on_demand using Spree::StockLocation.backorderable_default + return Spree::StockLocation.first.backorderable_default if stock_items.empty? + stock_item.backorderable? end diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb index b836cb58e4..e436dc989b 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -1,10 +1,10 @@ require 'open_food_network/permalink_generator' require 'open_food_network/property_merge' -require 'concerns/product_on_demand' +require 'concerns/product_stock' Spree::Product.class_eval do include PermalinkGenerator - include ProductOnDemand + include ProductStock # We have an after_destroy callback on Spree::ProductOptionType. However, if we # don't specify dependent => destroy on this association, it is not called. See: @@ -130,14 +130,6 @@ Spree::Product.class_eval do # -- Methods - def on_hand - if has_variants? - variants.map(&:on_hand).reduce(:+) - else - master.on_hand - end - end - # Called by Spree::Product::duplicate before saving. def duplicate_extra(parent) # Spree sets the SKU to "COPY OF #{parent sku}". diff --git a/knapsack_rspec_report.json b/knapsack_rspec_report.json index a2039ffdcd..12f72f882c 100644 --- a/knapsack_rspec_report.json +++ b/knapsack_rspec_report.json @@ -158,7 +158,7 @@ "spec/requests/checkout/failed_checkout_spec.rb": 1.9082491397857666, "spec/requests/embedded_shopfronts_headers_spec.rb": 0.8479242324829102, "spec/models/spree/calculator/price_sack_spec.rb": 0.671210527420044, - "spec/models/concerns/product_on_demand_spec.rb": 0.43450260162353516, + "spec/models/concerns/product_stock_spec.rb": 0.43450260162353516, "spec/lib/open_food_network/order_and_distributor_report_spec.rb": 0.61724853515625, "spec/features/admin/caching_spec.rb": 0.7170243263244629, "spec/controllers/api/customers_controller_spec.rb": 0.49866557121276855, diff --git a/spec/models/concerns/product_on_demand_spec.rb b/spec/models/concerns/product_on_demand_spec.rb deleted file mode 100644 index 8ff11302cd..0000000000 --- a/spec/models/concerns/product_on_demand_spec.rb +++ /dev/null @@ -1,28 +0,0 @@ -require 'spec_helper' - -describe ProductOnDemand do - describe '#on_demand=' do - context 'when the product has no variants' do - let(:product) { create(:simple_product) } - - before do - product.variants.first.destroy - product.variants.reload - end - - it 'sets the value on master.on_demand' do - product.on_demand = false - expect(product.master.on_demand).to eq(false) - end - end - - context 'when the product has variants' do - let(:product) { create(:simple_product) } - - it 'raises' do - expect { product.on_demand = true } - .to raise_error(StandardError, /cannot set on_demand/) - end - end - end -end diff --git a/spec/models/concerns/product_stock_spec.rb b/spec/models/concerns/product_stock_spec.rb new file mode 100644 index 0000000000..cf91b4d671 --- /dev/null +++ b/spec/models/concerns/product_stock_spec.rb @@ -0,0 +1,57 @@ +require "spec_helper" + +describe ProductStock do + let(:product) { create(:simple_product) } + + context "when product has no variants" do + before do + product.variants.first.destroy + product.variants.reload + end + + describe "product.on_demand" do + it "is master.on_demand" do + expect(product.on_demand).to eq(product.master.on_demand) + end + end + + describe "product.on_hand" do + it "is master.on_hand" do + expect(product.on_hand).to eq(product.master.on_hand) + end + end + end + + context "when product has one variant" do + describe "product.on_demand" do + it "is the products first variant on_demand" do + expect(product.on_demand).to eq(product.variants.first.on_demand) + end + end + + describe "product.on_hand" do + it "is the products first variant on_hand" do + expect(product.on_hand).to eq(product.variants.first.on_hand) + end + end + end + + context 'when product has more than one variant' do + before do + product.variants << create(:variant, product: product) + end + + describe "product.on_demand" do + it "raises error" do + expect { product.on_demand } + .to raise_error(StandardError, /Cannot determine product on_demand value/) + end + end + + describe "product.on_hand" do + it "is the sum of the products variants on_hand values" do + expect(product.on_hand).to eq(product.variants.first.on_hand + product.variants.second.on_hand) + end + end + end +end diff --git a/spec/models/concerns/variant_stock_spec.rb b/spec/models/concerns/variant_stock_spec.rb index ff95b84526..36fc33df24 100644 --- a/spec/models/concerns/variant_stock_spec.rb +++ b/spec/models/concerns/variant_stock_spec.rb @@ -103,27 +103,39 @@ describe VariantStock do end describe '#on_demand' do - context 'when the stock items is backorderable' do - before do - variant.stock_items.first.update_attribute( - :backorderable, true - ) + context 'when the variant has a stock item' do + let(:variant) { create(:variant) } + + context 'when the stock item is backorderable' do + before do + variant.stock_items.first.update_attribute( + :backorderable, true + ) + end + + it 'returns true' do + expect(variant.on_demand).to be_truthy + end end - it 'returns true' do - expect(variant.on_demand).to be_truthy + context 'when the stock items is not backorderable' do + before do + variant.stock_items.first.update_attribute( + :backorderable, false + ) + end + + it 'returns false' do + expect(variant.on_demand).to be_falsy + end end end - context 'when the stock items is backorderable' do - before do - variant.stock_items.first.update_attribute( - :backorderable, false - ) - end + context 'when the variant has no stock item' do + let(:variant) { build(:variant) } - it 'returns false' do - expect(variant.on_demand).to be_falsy + it 'returns stock location default' do + expect(variant.on_demand).to be_truthy end end end From 47be452ca0850fccee3242b4224af8f78fecf9ae Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Sat, 8 Dec 2018 23:00:02 +0000 Subject: [PATCH 3/3] Make ProductsController#create and #update work by not mass-assigning the provided on_hand and on_demand values and set them in product variant after the product is created --- .../admin/products_controller_decorator.rb | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/app/controllers/spree/admin/products_controller_decorator.rb b/app/controllers/spree/admin/products_controller_decorator.rb index 3ee88fc18d..c9d0f7c36d 100644 --- a/app/controllers/spree/admin/products_controller_decorator.rb +++ b/app/controllers/spree/admin/products_controller_decorator.rb @@ -31,6 +31,18 @@ Spree::Admin::ProductsController.class_eval do @show_latest_import = params[:latest_import] || false end + def create + delete_stock_params_and_set_after do + super + end + end + + def update + delete_stock_params_and_set_after do + super + end + end + def bulk_update collection_hash = Hash[params[:products].each_with_index.map { |p,i| [i,p] }] product_set = Spree::ProductSet.new({:collection_attributes => collection_hash}) @@ -119,4 +131,22 @@ Spree::Admin::ProductsController.class_eval do end end end + + def delete_stock_params_and_set_after + on_demand = params[:product].delete(:on_demand) + on_hand = params[:product].delete(:on_hand) + + yield + + set_stock_levels(@product, on_hand, on_demand) if @product.valid? + end + + def set_stock_levels(product, on_hand, on_demand) + variant = product.master + if product.variants.any? + variant = product.variants.first + end + variant.on_demand = on_demand if on_demand.present? + variant.on_hand = on_hand.to_i if on_hand.present? + end end