From 032741c54f5395fd5332ed07f5206b4be7a2ae4f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 29 Sep 2019 16:20:20 +0100 Subject: [PATCH] Refactor ProductsRenderer variants queries This removes another N+1 and allows pagination applied to the inital query to also affect the returned variants --- app/services/shop_products_service.rb | 14 ++- lib/open_food_network/products_renderer.rb | 37 +++--- spec/services/shop_products_service_spec.rb | 128 ++++++++++++-------- 3 files changed, 104 insertions(+), 75 deletions(-) diff --git a/app/services/shop_products_service.rb b/app/services/shop_products_service.rb index 85497356f7..795cecc6b9 100644 --- a/app/services/shop_products_service.rb +++ b/app/services/shop_products_service.rb @@ -1,4 +1,4 @@ -# Returns a (paginatable) AR object for the products in stock for a given distributor and OC. +# Returns a (paginatable) AR object for the products or variants in stock for a given shop and OC. # The stock-checking includes on_demand and stock level overrides from variant_overrides. class ShopProductsService @@ -7,13 +7,19 @@ class ShopProductsService @order_cycle = order_cycle end - def relation - Spree::Product.where(id: distributed_products) + def products_relation + Spree::Product.where(id: stocked_products) + end + + def variants_relation + @order_cycle. + variants_distributed_by(@distributor). + merge(stocked_variants_and_overrides) end private - def distributed_products + def stocked_products @order_cycle. variants_distributed_by(@distributor). merge(stocked_variants_and_overrides). diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb index 4c2e876c08..297209fee9 100644 --- a/lib/open_food_network/products_renderer.rb +++ b/lib/open_food_network/products_renderer.rb @@ -30,13 +30,17 @@ module OpenFoodNetwork def shop_products return unless @order_cycle - @shop_products ||= ShopProductsService.new(@distributor, @order_cycle).relation. - order(taxon_order). - each { |product| scoper.scope(product) } + @shop_products ||= begin + scoper = ScopeProductToHub.new(@distributor) + + shop_products_service.products_relation. + order(taxon_order). + each { |product| scoper.scope(product) } + end end - def scoper - ScopeProductToHub.new(@distributor) + def shop_products_service + ShopProductsService.new(@distributor, @order_cycle) end def taxon_order @@ -50,25 +54,22 @@ module OpenFoodNetwork end end - def all_variants_for_shop - @all_variants_for_shop ||= begin - # We use the in_stock? method here instead of the in_stock scope - # because we need to look up the stock as overridden by - # VariantOverrides, and the scope method is not affected by them. - scoper = OpenFoodNetwork::ScopeVariantToHub.new(@distributor) - Spree::Variant. - for_distribution(@order_cycle, @distributor). - each { |v| scoper.scope(v) }. - select(&:in_stock?) - end + def variants_for_shop + @variants_for_shop ||= begin + scoper = OpenFoodNetwork::ScopeVariantToHub.new(@distributor) + + shop_products_service.variants_relation. + where(product_id: shop_products). + each { |v| scoper.scope(v) } + end end def variants_for_shop_by_id - index_by_product_id all_variants_for_shop.reject(&:is_master) + index_by_product_id variants_for_shop.reject(&:is_master) end def master_variants_for_shop_by_id - index_by_product_id all_variants_for_shop.select(&:is_master) + index_by_product_id variants_for_shop.select(&:is_master) end def index_by_product_id(variants) diff --git a/spec/services/shop_products_service_spec.rb b/spec/services/shop_products_service_spec.rb index 70840418ad..fa0c51c01e 100644 --- a/spec/services/shop_products_service_spec.rb +++ b/spec/services/shop_products_service_spec.rb @@ -1,68 +1,90 @@ require 'spec_helper' describe ShopProductsService do - let(:distributor) { create(:distributor_enterprise) } - let(:product) { create(:product) } - let(:variant) { product.variants.first } - let(:order_cycle) do - create(:simple_order_cycle, distributors: [distributor], variants: [variant]) - end - - describe "product distributed by distributor in the OC" do - it "returns products" do - expect(described_class.new(distributor, order_cycle).relation).to eq([product]) - end - end - - describe "product distributed by distributor in another OC" do - let(:reference_variant) { create(:product).variants.first } + describe "#products_relation" do + let(:distributor) { create(:distributor_enterprise) } + let(:product) { create(:product) } + let(:variant) { product.variants.first } let(:order_cycle) do - create(:simple_order_cycle, distributors: [distributor], variants: [reference_variant]) - end - let(:another_order_cycle) do create(:simple_order_cycle, distributors: [distributor], variants: [variant]) end - it "does not return product" do - expect(described_class.new(distributor, order_cycle).relation).to_not include product + describe "product distributed by distributor in the OC" do + it "returns products" do + expect(described_class.new(distributor, order_cycle).products_relation).to eq([product]) + end + end + + describe "product distributed by distributor in another OC" do + let(:reference_variant) { create(:product).variants.first } + let(:order_cycle) do + create(:simple_order_cycle, distributors: [distributor], variants: [reference_variant]) + end + let(:another_order_cycle) do + create(:simple_order_cycle, distributors: [distributor], variants: [variant]) + end + + it "does not return product" do + expect(described_class.new(distributor, order_cycle).products_relation).to_not include product + end + end + + describe "product distributed by another distributor in the OC" do + let(:another_distributor) { create(:distributor_enterprise) } + let(:order_cycle) do + create(:simple_order_cycle, distributors: [another_distributor], variants: [variant]) + end + + it "does not return product" do + expect(described_class.new(distributor, order_cycle).products_relation).to_not include product + end + end + + describe "filtering products that are out of stock" do + context "with regular variants" do + it "returns product when variant is in stock" do + expect(described_class.new(distributor, order_cycle).products_relation).to include product + end + + it "does not return product when variant is out of stock" do + variant.update_attribute(:on_hand, 0) + expect(described_class.new(distributor, order_cycle).products_relation).to_not include product + end + end + + context "with variant overrides" do + let!(:override) { create(:variant_override, hub: distributor, variant: variant, count_on_hand: 0) } + + it "does not return product when an override is out of stock" do + expect(described_class.new(distributor, order_cycle).products_relation).to_not include product + end + + it "returns product when an override is in stock" do + variant.update_attribute(:on_hand, 0) + override.update_attribute(:count_on_hand, 10) + expect(described_class.new(distributor, order_cycle).products_relation).to include product + end + end end end - describe "product distributed by another distributor in the OC" do - let(:another_distributor) { create(:distributor_enterprise) } - let(:order_cycle) do - create(:simple_order_cycle, distributors: [another_distributor], variants: [variant]) + describe "#variants_relation" do + let(:distributor) { create(:distributor_enterprise) } + let(:oc) { create(:simple_order_cycle, distributors: [distributor], variants: [v1, v3]) } + let(:product) { create(:simple_product) } + let!(:v1) { create(:variant, product: product) } + let!(:v2) { create(:variant, product: product) } + let!(:v3) { create(:variant, product: product) } + let!(:vo) { create(:variant_override, hub: distributor, variant_id: v3.id, count_on_hand: 0) } + let(:variants) { described_class.new(distributor, oc).variants_relation } + + it "returns variants in the oc" do + expect(variants).to include v1 + expect(variants).to_not include v2 end - it "does not return product" do - expect(described_class.new(distributor, order_cycle).relation).to_not include product - end - end - - describe "filtering products that are out of stock" do - context "with regular variants" do - it "returns product when variant is in stock" do - expect(described_class.new(distributor, order_cycle).relation).to include product - end - - it "does not return product when variant is out of stock" do - variant.update_attribute(:on_hand, 0) - expect(described_class.new(distributor, order_cycle).relation).to_not include product - end - end - - context "with variant overrides" do - let!(:override) { create(:variant_override, hub: distributor, variant: variant, count_on_hand: 0) } - - it "does not return product when an override is out of stock" do - expect(described_class.new(distributor, order_cycle).relation).to_not include product - end - - it "returns product when an override is in stock" do - variant.update_attribute(:on_hand, 0) - override.update_attribute(:count_on_hand, 10) - expect(described_class.new(distributor, order_cycle).relation).to include product - end + it "does not return variants where override is out of stock" do + expect(variants).to_not include v3 end end end