From 83b90f3167002e60bdd5202e67c46dc62c301952 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 27 Mar 2020 13:31:39 +0100 Subject: [PATCH 1/7] Add spec variant override test to VariantsStockLevels --- spec/services/variants_stock_levels_spec.rb | 26 +++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/spec/services/variants_stock_levels_spec.rb b/spec/services/variants_stock_levels_spec.rb index f4a77d7018..8aaa6503bf 100644 --- a/spec/services/variants_stock_levels_spec.rb +++ b/spec/services/variants_stock_levels_spec.rb @@ -48,4 +48,30 @@ describe VariantsStockLevels do ) end end + + describe "when the variant has an override" do + let!(:distributor) { create(:distributor_enterprise) } + let(:supplier) { variant_in_the_order.product.supplier } + let!(:order_cycle) { + create(:simple_order_cycle, suppliers: [supplier], distributors: [distributor], + variants: [variant_in_the_order]) + } + let!(:variant_override) { + create(:variant_override, hub: distributor, + variant: variant_in_the_order, + count_on_hand: 404) + } + + before do + order.order_cycle = order_cycle + order.distributor = distributor + order.save + end + + xit "returns the on_hand value of the override" do + expect(variant_stock_levels.call(order, [variant_in_the_order.id])).to eq( + variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 404, on_demand: false } + ) + end + end end From fbfe663ebcc1991571507e7e43311e6bce2dc271 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 27 Mar 2020 13:38:57 +0100 Subject: [PATCH 2/7] Add variant scoping to VariantStockLevels --- app/services/variants_stock_levels.rb | 20 +++++++++++++++++--- spec/services/variants_stock_levels_spec.rb | 6 +++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/app/services/variants_stock_levels.rb b/app/services/variants_stock_levels.rb index 69dbc49dee..2d0628f5cb 100644 --- a/app/services/variants_stock_levels.rb +++ b/app/services/variants_stock_levels.rb @@ -1,6 +1,8 @@ # Report the stock levels of: # - all variants in the order # - all requested variant ids +require 'open_food_network/scope_variant_to_hub' + class VariantsStockLevels def call(order, requested_variant_ids) variant_stock_levels = variant_stock_levels(order.line_items) @@ -34,12 +36,24 @@ class VariantsStockLevels def variant_stock_levels(line_items) Hash[ line_items.map do |line_item| - [line_item.variant.id, + variant = scoped_variant(line_item) + + [variant.id, { quantity: line_item.quantity, max_quantity: line_item.max_quantity, - on_hand: line_item.variant.on_hand, - on_demand: line_item.variant.on_demand }] + on_hand: variant.on_hand, + on_demand: variant.on_demand }] end ] end + + def scoped_variant(line_item) + distributor = line_item.order.distributor + variant = line_item.variant + + return variant if distributor.blank? + + OpenFoodNetwork::ScopeVariantToHub.new(distributor).scope(variant) + variant + end end diff --git a/spec/services/variants_stock_levels_spec.rb b/spec/services/variants_stock_levels_spec.rb index 8aaa6503bf..ecd0656bc0 100644 --- a/spec/services/variants_stock_levels_spec.rb +++ b/spec/services/variants_stock_levels_spec.rb @@ -59,7 +59,7 @@ describe VariantsStockLevels do let!(:variant_override) { create(:variant_override, hub: distributor, variant: variant_in_the_order, - count_on_hand: 404) + count_on_hand: 200) } before do @@ -68,9 +68,9 @@ describe VariantsStockLevels do order.save end - xit "returns the on_hand value of the override" do + it "returns the on_hand value of the override" do expect(variant_stock_levels.call(order, [variant_in_the_order.id])).to eq( - variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 404, on_demand: false } + variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 200, on_demand: false } ) end end From 857cacb74bf30b532dfa315f530cfc2c2024f5e0 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 27 Mar 2020 14:03:17 +0100 Subject: [PATCH 3/7] Add test for additional case where variant is not in the order --- spec/services/variants_stock_levels_spec.rb | 29 ++++++++++++++++----- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/spec/services/variants_stock_levels_spec.rb b/spec/services/variants_stock_levels_spec.rb index ecd0656bc0..dc90cf3f9a 100644 --- a/spec/services/variants_stock_levels_spec.rb +++ b/spec/services/variants_stock_levels_spec.rb @@ -54,13 +54,18 @@ describe VariantsStockLevels do let(:supplier) { variant_in_the_order.product.supplier } let!(:order_cycle) { create(:simple_order_cycle, suppliers: [supplier], distributors: [distributor], - variants: [variant_in_the_order]) + variants: [variant_in_the_order, variant_not_in_the_order]) } - let!(:variant_override) { + let!(:variant_override_in_order) { create(:variant_override, hub: distributor, variant: variant_in_the_order, count_on_hand: 200) } + let!(:variant_override_not_in_order) { + create(:variant_override, hub: distributor, + variant: variant_not_in_the_order, + count_on_hand: 404) + } before do order.order_cycle = order_cycle @@ -68,10 +73,22 @@ describe VariantsStockLevels do order.save end - it "returns the on_hand value of the override" do - expect(variant_stock_levels.call(order, [variant_in_the_order.id])).to eq( - variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 200, on_demand: false } - ) + context "when the variant is in the order" do + it "returns the on_hand value of the override" do + expect(variant_stock_levels.call(order, [variant_in_the_order.id])).to eq( + variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 200, on_demand: false } + ) + end + end + + context "with variants that are not in the order" do + xit "returns the on_hand value of the override" do + variant_ids = [variant_in_the_order.id, variant_not_in_the_order.id] + expect(variant_stock_levels.call(order, variant_ids)).to eq( + variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 200, on_demand: false }, + variant_not_in_the_order.id => { quantity: 0, max_quantity: 0, on_hand: 404, on_demand: false } + ) + end end end end From 7d33a237d07b7167f9972fdc97b4b6a4064d81b2 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 27 Mar 2020 14:15:15 +0100 Subject: [PATCH 4/7] Add scoping to VariantsStockLevels when variant is not in the order --- app/services/variants_stock_levels.rb | 9 +++------ spec/services/variants_stock_levels_spec.rb | 16 +++++++++++----- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/app/services/variants_stock_levels.rb b/app/services/variants_stock_levels.rb index 2d0628f5cb..44e2611f04 100644 --- a/app/services/variants_stock_levels.rb +++ b/app/services/variants_stock_levels.rb @@ -12,7 +12,7 @@ class VariantsStockLevels order_variant_ids = variant_stock_levels.keys missing_variant_ids = requested_variant_ids - order_variant_ids missing_variant_ids.each do |variant_id| - variant = Spree::Variant.find(variant_id) + variant = scoped_variant(order.distributor, Spree::Variant.find(variant_id)) variant_stock_levels[variant_id] = { quantity: 0, max_quantity: 0, on_hand: variant.on_hand, on_demand: variant.on_demand } end @@ -36,7 +36,7 @@ class VariantsStockLevels def variant_stock_levels(line_items) Hash[ line_items.map do |line_item| - variant = scoped_variant(line_item) + variant = scoped_variant(line_item.order.distributor, line_item.variant) [variant.id, { quantity: line_item.quantity, @@ -47,10 +47,7 @@ class VariantsStockLevels ] end - def scoped_variant(line_item) - distributor = line_item.order.distributor - variant = line_item.variant - + def scoped_variant(distributor, variant) return variant if distributor.blank? OpenFoodNetwork::ScopeVariantToHub.new(distributor).scope(variant) diff --git a/spec/services/variants_stock_levels_spec.rb b/spec/services/variants_stock_levels_spec.rb index dc90cf3f9a..5977845665 100644 --- a/spec/services/variants_stock_levels_spec.rb +++ b/spec/services/variants_stock_levels_spec.rb @@ -64,7 +64,7 @@ describe VariantsStockLevels do let!(:variant_override_not_in_order) { create(:variant_override, hub: distributor, variant: variant_not_in_the_order, - count_on_hand: 404) + count_on_hand: 201) } before do @@ -76,17 +76,23 @@ describe VariantsStockLevels do context "when the variant is in the order" do it "returns the on_hand value of the override" do expect(variant_stock_levels.call(order, [variant_in_the_order.id])).to eq( - variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 200, on_demand: false } + variant_in_the_order.id => { + quantity: 2, max_quantity: 3, on_hand: 200, on_demand: false + } ) end end context "with variants that are not in the order" do - xit "returns the on_hand value of the override" do + it "returns the on_hand value of the override" do variant_ids = [variant_in_the_order.id, variant_not_in_the_order.id] expect(variant_stock_levels.call(order, variant_ids)).to eq( - variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 200, on_demand: false }, - variant_not_in_the_order.id => { quantity: 0, max_quantity: 0, on_hand: 404, on_demand: false } + variant_in_the_order.id => { + quantity: 2, max_quantity: 3, on_hand: 200, on_demand: false + }, + variant_not_in_the_order.id => { + quantity: 0, max_quantity: 0, on_hand: 201, on_demand: false + } ) end end From 71f00f9283f9801754397ee49a01ed51eae303f0 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 27 Mar 2020 14:23:48 +0100 Subject: [PATCH 5/7] Remove comment warning about this issue --- app/services/variants_stock_levels.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/services/variants_stock_levels.rb b/app/services/variants_stock_levels.rb index 44e2611f04..2865d44e39 100644 --- a/app/services/variants_stock_levels.rb +++ b/app/services/variants_stock_levels.rb @@ -7,8 +7,6 @@ class VariantsStockLevels def call(order, requested_variant_ids) variant_stock_levels = variant_stock_levels(order.line_items) - # Variants are not scoped here and so the stock levels reported are incorrect - # See cart_controller_spec for more details and #3222 order_variant_ids = variant_stock_levels.keys missing_variant_ids = requested_variant_ids - order_variant_ids missing_variant_ids.each do |variant_id| From f7c047b798f474c647bb41426435817497249199 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 30 Mar 2020 20:33:27 +0200 Subject: [PATCH 6/7] Memoize ScopeVariantToHub to avoid fetching the hub's overrides each time --- app/services/variants_stock_levels.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/services/variants_stock_levels.rb b/app/services/variants_stock_levels.rb index 2865d44e39..7a36658f31 100644 --- a/app/services/variants_stock_levels.rb +++ b/app/services/variants_stock_levels.rb @@ -48,7 +48,11 @@ class VariantsStockLevels def scoped_variant(distributor, variant) return variant if distributor.blank? - OpenFoodNetwork::ScopeVariantToHub.new(distributor).scope(variant) + scoper(distributor).scope(variant) variant end + + def scoper(distributor) + @scoper ||= OpenFoodNetwork::ScopeVariantToHub.new(distributor) + end end From 09c8819e5a573f87c42863bbc8ee24860ff5c305 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 30 Mar 2020 20:36:45 +0200 Subject: [PATCH 7/7] Remove unnecessary Bugsnag calls The Bugsnag notification was just here to see if this was dead code. It's not. --- app/services/variants_stock_levels.rb | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/app/services/variants_stock_levels.rb b/app/services/variants_stock_levels.rb index 7a36658f31..73f6e68278 100644 --- a/app/services/variants_stock_levels.rb +++ b/app/services/variants_stock_levels.rb @@ -14,23 +14,11 @@ class VariantsStockLevels variant_stock_levels[variant_id] = { quantity: 0, max_quantity: 0, on_hand: variant.on_hand, on_demand: variant.on_demand } end - # The code above is most probably dead code, this bugsnag notification will confirm it - notify_bugsnag(order, requested_variant_ids, order_variant_ids) if missing_variant_ids.present? - variant_stock_levels end private - def notify_bugsnag(order, requested_variant_ids, order_variant_ids) - error_msg = "VariantsStockLevels.call with variants in the request that are not in the order" - Bugsnag.notify(RuntimeError.new(error_msg), - requested_variant_ids: requested_variant_ids.as_json, - order_variant_ids: order_variant_ids.as_json, - order: order.as_json, - line_items: order.line_items.as_json) - end - def variant_stock_levels(line_items) Hash[ line_items.map do |line_item|