From 3dabbebbd392f1aa183c3c43749ea31ff29ffffe Mon Sep 17 00:00:00 2001 From: VINAY MITTAL Date: Thu, 17 Mar 2016 16:07:52 +0530 Subject: [PATCH] reduce n+1 queries and fix specs --- core/app/models/spree/product.rb | 7 ++++--- core/app/models/spree/stock/quantifier.rb | 4 ++-- core/app/models/spree/stock_item.rb | 5 ++++- core/app/models/spree/variant.rb | 3 +++ core/spec/models/spree/line_item_spec.rb | 2 ++ core/spec/models/spree/stock_item_spec.rb | 9 +++++++++ core/spec/models/spree/variant_spec.rb | 10 ++++++++++ frontend/app/controllers/spree/products_controller.rb | 2 +- 8 files changed, 35 insertions(+), 7 deletions(-) diff --git a/core/app/models/spree/product.rb b/core/app/models/spree/product.rb index 1f1de610ddf..8c8f0398004 100644 --- a/core/app/models/spree/product.rb +++ b/core/app/models/spree/product.rb @@ -190,9 +190,10 @@ def self.like_any(fields, values) # values should not be displayed to frontend users. Otherwise it breaks the # idea of having variants def variants_and_option_values(current_currency = nil) - variants.includes(:option_values).active(current_currency).select do |variant| - variant.option_values.any? - end + variants.includes(:option_values, :prices, :stock_items_with_active_location). + active(current_currency).select do |variant| + variant.option_values.any? + end end def empty_option_values? diff --git a/core/app/models/spree/stock/quantifier.rb b/core/app/models/spree/stock/quantifier.rb index 2472a65226d..c100db6c39d 100644 --- a/core/app/models/spree/stock/quantifier.rb +++ b/core/app/models/spree/stock/quantifier.rb @@ -5,12 +5,12 @@ class Quantifier def initialize(variant) @variant = variant - @stock_items = @variant.stock_items.with_active_stock_location + @stock_items = @variant.stock_items_with_active_location end def total_on_hand if variant.should_track_inventory? - stock_items.sum(:count_on_hand) + stock_items.inject(0) { |result, stock_item| result += stock_item.count_on_hand } else Float::INFINITY end diff --git a/core/app/models/spree/stock_item.rb b/core/app/models/spree/stock_item.rb index 074ee0d5b78..78ea98861ca 100644 --- a/core/app/models/spree/stock_item.rb +++ b/core/app/models/spree/stock_item.rb @@ -4,6 +4,9 @@ class StockItem < Spree::Base with_options inverse_of: :stock_items do belongs_to :stock_location, class_name: 'Spree::StockLocation' + belongs_to :active_stock_location, -> { where(active: true) }, + class_name: 'Spree::StockLocation', + foreign_key: :stock_location_id belongs_to :variant, class_name: 'Spree::Variant', counter_cache: true end has_many :stock_movements, inverse_of: :stock_item @@ -24,7 +27,7 @@ class StockItem < Spree::Base self.whitelisted_ransackable_attributes = ['count_on_hand', 'stock_location_id'] - scope :with_active_stock_location, -> { joins(:stock_location).merge(Spree::StockLocation.active) } + scope :with_active_stock_location, -> { joins(:active_stock_location) } def backordered_inventory_units Spree::InventoryUnit.backordered_for_stock_item(self) diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb index fe7161283b5..aa6119acb0d 100644 --- a/core/app/models/spree/variant.rb +++ b/core/app/models/spree/variant.rb @@ -15,6 +15,9 @@ class Variant < Spree::Base with_options inverse_of: :variant do has_many :inventory_units has_many :stock_items, dependent: :destroy + has_many :stock_items_with_active_location, -> { with_active_stock_location }, + class_name: "Spree::StockItem", + dependent: :destroy end has_many :line_items, dependent: :restrict_with_error diff --git a/core/spec/models/spree/line_item_spec.rb b/core/spec/models/spree/line_item_spec.rb index c2677a88c43..502d18d5cf8 100644 --- a/core/spec/models/spree/line_item_spec.rb +++ b/core/spec/models/spree/line_item_spec.rb @@ -243,6 +243,7 @@ it "doesnt allow to increase item quantity" do line_item = order.line_items.first + line_item.reload line_item.quantity += 2 line_item.target_shipment = order.shipments.first @@ -270,6 +271,7 @@ it "doesnt allow to increase quantity over stock availability" do line_item = order.line_items.first + line_item.reload line_item.quantity += 3 line_item.target_shipment = order.shipments.first diff --git a/core/spec/models/spree/stock_item_spec.rb b/core/spec/models/spree/stock_item_spec.rb index 46ccc738b34..75b022d7ba5 100644 --- a/core/spec/models/spree/stock_item_spec.rb +++ b/core/spec/models/spree/stock_item_spec.rb @@ -5,6 +5,15 @@ subject { stock_location.stock_items.order(:id).first } + describe "associations" do + it do + is_expected.to belong_to(:active_stock_location). + conditions(active: true). + class_name("Spree::StockLocation"). + with_foreign_key(:stock_location_id) + end + end + it 'maintains the count on hand for a variant' do expect(subject.count_on_hand).to eq 10 end diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb index c61c119c861..003ef3df144 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -8,6 +8,15 @@ it_behaves_like 'default_price' + describe 'associations' do + it do + is_expected.to have_many(:stock_items_with_active_location). + conditions(:with_active_stock_location). + class_name("Spree::StockItem"). + dependent(:destroy) + end + end + describe 'validations' do it { expect(master_variant).to_not validate_presence_of(:option_values) } it { expect(variant).to validate_presence_of(:option_values) } @@ -58,6 +67,7 @@ context 'when a variant is created' do let!(:new_variant) { create(:variant, product: product) } + before { product.reload } it { expect(product.master).to_not be_in_stock } end diff --git a/frontend/app/controllers/spree/products_controller.rb b/frontend/app/controllers/spree/products_controller.rb index 80598a0c3a1..826cf427c75 100644 --- a/frontend/app/controllers/spree/products_controller.rb +++ b/frontend/app/controllers/spree/products_controller.rb @@ -40,7 +40,7 @@ def load_product else @products = Product.active(current_currency) end - @product = @products.includes(:variants_including_master).friendly.find(params[:id]) + @product = @products.includes(variant_images: :viewable).friendly.find(params[:id]) end def load_taxon