From 7c9e3d7f06d7185a1e0d740e1664c443c5408d74 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 20 Aug 2019 10:19:54 +1000 Subject: [PATCH 1/3] Spec combination of all variant filters --- .../products_and_inventory_report_spec.rb | 48 +++++++++++++++++-- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/spec/lib/open_food_network/products_and_inventory_report_spec.rb b/spec/lib/open_food_network/products_and_inventory_report_spec.rb index 97cb4de17b..6e948d12c4 100644 --- a/spec/lib/open_food_network/products_and_inventory_report_spec.rb +++ b/spec/lib/open_food_network/products_and_inventory_report_spec.rb @@ -171,10 +171,49 @@ module OpenFoodNetwork end it "should do all the filters at once" do + pending "the order cycle filter is overriden by the distributor filter" + # The following data ensures that this spec fails if any of the + # filters fail. It's testing the filters are not impacting each other. distributor = create(:distributor_enterprise) - product1 = create(:simple_product, supplier: supplier) - product2 = create(:simple_product, supplier: supplier) - order_cycle = create(:simple_order_cycle, suppliers: [supplier], distributors: [distributor], variants: [product1.variants.first]) + other_distributor = create(:distributor_enterprise) + other_supplier = create(:supplier_enterprise) + not_filtered_variant = create(:simple_product, supplier: supplier).variants.first + variant_filtered_by_order_cycle = create(:simple_product, supplier: supplier).variants.first + variant_filtered_by_distributor = create(:simple_product, supplier: supplier).variants.first + variant_filtered_by_supplier = create(:simple_product, supplier: other_supplier).variants.first + variant_filtered_by_stock = create(:simple_product, supplier: supplier, on_hand: 0).variants.first + + # This OC contains all products except the one that should be filtered + # by order cycle. We create a separate OC further down to proof that + # the product is passing all other filters. + order_cycle = create( + :simple_order_cycle, + suppliers: [supplier, other_supplier], + distributors: [distributor, other_distributor], + variants: [ + not_filtered_variant, + variant_filtered_by_distributor, + variant_filtered_by_supplier, + variant_filtered_by_stock + ] + ) + + # Remove the distribution of one product for one distributor but still + # sell it through the other distributor. + order_cycle.exchanges.outgoing.find_by_receiver_id(distributor.id). + exchange_variants. + find_by_variant_id(variant_filtered_by_distributor). + destroy + + # Make product available to be filtered later. See OC comment above. + create( + :simple_order_cycle, + suppliers: [supplier], + distributors: [distributor, other_distributor], + variants: [ + variant_filtered_by_order_cycle + ] + ) allow(subject).to receive(:params).and_return( order_cycle_id: order_cycle.id, @@ -182,7 +221,8 @@ module OpenFoodNetwork distributor_id: distributor.id, report_type: 'inventory' ) - subject.filter(variants) + + expect(subject.filter(variants)).to match_array [not_filtered_variant] end end From 6944fe1e467dbfc0e41a5d593120a97400cbb7bd Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 20 Aug 2019 10:32:59 +1000 Subject: [PATCH 2/3] Make order cycle filter chainable with other filters --- .../products_and_inventory_report_base.rb | 10 +++++++++- .../products_and_inventory_report_spec.rb | 1 - 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/open_food_network/products_and_inventory_report_base.rb b/lib/open_food_network/products_and_inventory_report_base.rb index 04a587d215..55d50b80bf 100644 --- a/lib/open_food_network/products_and_inventory_report_base.rb +++ b/lib/open_food_network/products_and_inventory_report_base.rb @@ -65,7 +65,15 @@ module OpenFoodNetwork def filter_to_order_cycle(variants) if params[:order_cycle_id].to_i > 0 order_cycle = OrderCycle.find params[:order_cycle_id] - variants.where(id: order_cycle.variants) + # There are two quirks here: + # + # 1. Rails 3 uses only the last `where` clause of a column. So we can't + # use `variants.where(id: order_cycle.variants)` until we upgrade to + # Rails 4. + # + # 2. `order_cycle.variants` returns an array. So we need to use map + # instead of pluck. + variants.where("spree_variants.id in (?)", order_cycle.variants.map(&:id)) else variants end diff --git a/spec/lib/open_food_network/products_and_inventory_report_spec.rb b/spec/lib/open_food_network/products_and_inventory_report_spec.rb index 6e948d12c4..8575d57bb6 100644 --- a/spec/lib/open_food_network/products_and_inventory_report_spec.rb +++ b/spec/lib/open_food_network/products_and_inventory_report_spec.rb @@ -171,7 +171,6 @@ module OpenFoodNetwork end it "should do all the filters at once" do - pending "the order cycle filter is overriden by the distributor filter" # The following data ensures that this spec fails if any of the # filters fail. It's testing the filters are not impacting each other. distributor = create(:distributor_enterprise) From f623446e3ef77d8e232c24e7785f8549bcf1d5e3 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 5 Sep 2019 11:43:32 +1000 Subject: [PATCH 3/3] Avoid additional query in inventory reports --- .../products_and_inventory_report_base.rb | 14 +++++--------- .../products_and_inventory_report_spec.rb | 3 +++ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/open_food_network/products_and_inventory_report_base.rb b/lib/open_food_network/products_and_inventory_report_base.rb index 55d50b80bf..e7583f2c0c 100644 --- a/lib/open_food_network/products_and_inventory_report_base.rb +++ b/lib/open_food_network/products_and_inventory_report_base.rb @@ -65,15 +65,11 @@ module OpenFoodNetwork def filter_to_order_cycle(variants) if params[:order_cycle_id].to_i > 0 order_cycle = OrderCycle.find params[:order_cycle_id] - # There are two quirks here: - # - # 1. Rails 3 uses only the last `where` clause of a column. So we can't - # use `variants.where(id: order_cycle.variants)` until we upgrade to - # Rails 4. - # - # 2. `order_cycle.variants` returns an array. So we need to use map - # instead of pluck. - variants.where("spree_variants.id in (?)", order_cycle.variants.map(&:id)) + variant_ids = Exchange.in_order_cycle(order_cycle). + joins("INNER JOIN exchange_variants ON exchanges.id = exchange_variants.exchange_id"). + select("DISTINCT exchange_variants.variant_id") + + variants.where("spree_variants.id IN (#{variant_ids.to_sql})") else variants end diff --git a/spec/lib/open_food_network/products_and_inventory_report_spec.rb b/spec/lib/open_food_network/products_and_inventory_report_spec.rb index 8575d57bb6..7457e2ed0c 100644 --- a/spec/lib/open_food_network/products_and_inventory_report_spec.rb +++ b/spec/lib/open_food_network/products_and_inventory_report_spec.rb @@ -222,6 +222,9 @@ module OpenFoodNetwork ) expect(subject.filter(variants)).to match_array [not_filtered_variant] + + # And it integrates with the ordering of the `variants` method. + expect(subject.variants).to match_array [not_filtered_variant] end end