From d7e4962fba6d630fceda5a4cb54078d28878244f Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 1 Mar 2023 12:05:32 +1100 Subject: [PATCH 01/10] Add method for easier reading --- app/services/order_available_shipping_methods.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/services/order_available_shipping_methods.rb b/app/services/order_available_shipping_methods.rb index 9751ee2d64..8e81eb417b 100644 --- a/app/services/order_available_shipping_methods.rb +++ b/app/services/order_available_shipping_methods.rb @@ -16,9 +16,7 @@ class OrderAvailableShippingMethods shipping_methods = shipping_methods_before_tag_rules_applied - applicator = OpenFoodNetwork::TagRuleApplicator.new(distributor, - "FilterShippingMethods", customer&.tag_list) - applicator.filter!(shipping_methods) + tag_rules.filter!(shipping_methods) shipping_methods.uniq end @@ -40,4 +38,10 @@ class OrderAvailableShippingMethods .where(distributor_id: distributor.id) .select(:shipping_method_id) end + + def tag_rules + OpenFoodNetwork::TagRuleApplicator.new( + distributor, "FilterShippingMethods", customer&.tag_list + ) + end end From 6f3a0e8812b95fb9d74937395db2eec50cbd4b82 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 1 Mar 2023 12:10:26 +1100 Subject: [PATCH 02/10] De-duplicate early for better performance --- app/services/order_available_shipping_methods.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/order_available_shipping_methods.rb b/app/services/order_available_shipping_methods.rb index 8e81eb417b..d689e88e22 100644 --- a/app/services/order_available_shipping_methods.rb +++ b/app/services/order_available_shipping_methods.rb @@ -18,7 +18,7 @@ class OrderAvailableShippingMethods tag_rules.filter!(shipping_methods) - shipping_methods.uniq + shipping_methods end private @@ -30,7 +30,7 @@ class OrderAvailableShippingMethods distributor.shipping_methods.where( id: available_distributor_shipping_methods_ids ) - end.frontend.to_a + end.frontend.to_a.uniq end def available_distributor_shipping_methods_ids From e227cb912b254b0d936ed3264ea374da0383fd6b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 1 Mar 2023 13:25:55 +1100 Subject: [PATCH 03/10] Simplify tag rule spec We don't need to know the internals. We want to know that the applicator is not raising an error when we give `nil` or an empty collection. --- .../tag_rule_applicator_spec.rb | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/spec/lib/open_food_network/tag_rule_applicator_spec.rb b/spec/lib/open_food_network/tag_rule_applicator_spec.rb index 82d7fac903..486474c2f3 100644 --- a/spec/lib/open_food_network/tag_rule_applicator_spec.rb +++ b/spec/lib/open_food_network/tag_rule_applicator_spec.rb @@ -112,22 +112,12 @@ module OpenFoodNetwork describe "filter!" do let(:applicator) { OpenFoodNetwork::TagRuleApplicator.new(enterprise, "FilterProducts", []) } - context "when the subject is nil" do - let(:subject) { double(:subject, reject!: false) } - - it "returns immediately" do - applicator.filter!(subject) - expect(subject).to_not have_received(:reject!) - end + it "handles nil" do + applicator.filter!(nil) end - context "when subject is empty" do - let(:subject) { double(:subject, reject!: false) } - - it "returns immediately" do - applicator.filter!(subject) - expect(subject).to_not have_received(:reject!) - end + it "handles an empty collection" do + applicator.filter!(OrderCycle.none) end context "when subject is an array" do From eee5e8eee744c828f439074bbf532b4f7565a2e0 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 1 Mar 2023 13:27:08 +1100 Subject: [PATCH 04/10] Remove unnecessary tag rule code The applicator converts the input to an array, even `nil` becomes an empty array. Iterating over an empty array doesn't do anything either, the result is the same. --- lib/open_food_network/tag_rule_applicator.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/open_food_network/tag_rule_applicator.rb b/lib/open_food_network/tag_rule_applicator.rb index 09060a8ecf..b606e57438 100644 --- a/lib/open_food_network/tag_rule_applicator.rb +++ b/lib/open_food_network/tag_rule_applicator.rb @@ -14,8 +14,6 @@ module OpenFoodNetwork end def filter!(subject) - return unless subject.respond_to?(:any?) && subject.any? - subject.to_a.reject! do |element| if rule_class.respond_to?(:tagged_children_for) children = rule_class.tagged_children_for(element) From 4c2d7c0d1c4c4e34c2236d8545578123db0f6e23 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 1 Mar 2023 13:31:48 +1100 Subject: [PATCH 05/10] Rename confusing test variable The "subject" is usually the code under test. In this spec it would be the TagRuleApplicator and not the parameter given to it. So I renamed it to avoid confusion here. --- .../tag_rule_applicator_spec.rb | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/spec/lib/open_food_network/tag_rule_applicator_spec.rb b/spec/lib/open_food_network/tag_rule_applicator_spec.rb index 486474c2f3..2dd78b63a6 100644 --- a/spec/lib/open_food_network/tag_rule_applicator_spec.rb +++ b/spec/lib/open_food_network/tag_rule_applicator_spec.rb @@ -120,9 +120,9 @@ module OpenFoodNetwork applicator.filter!(OrderCycle.none) end - context "when subject is an array" do + context "when filtering an array" do let(:element) { double(:element, ) } - let(:subject) { [element] } + let(:list) { [element] } context "when rule_class reponds to tagged_children_for" do let(:child1) { double(:child) } @@ -136,15 +136,15 @@ module OpenFoodNetwork before do allow(applicator).to receive(:reject?).with(child1) { true } allow(applicator).to receive(:reject?).with(child2) { false } - applicator.filter!(subject) + applicator.filter!(list) end it "rejects the specified children from the array" do expect(children).to eq [child2] end - it "does not remove the element from the original subject" do - expect(subject).to eq [element] + it "does not remove the element from the original list" do + expect(list).to eq [element] end end @@ -152,15 +152,15 @@ module OpenFoodNetwork before do allow(applicator).to receive(:reject?).with(child1) { true } allow(applicator).to receive(:reject?).with(child2) { true } - applicator.filter!(subject) + applicator.filter!(list) end it "removes all children from the array" do expect(children).to eq [] end - it "removes the element from the original subject" do - expect(subject).to eq [] + it "removes the element from the original list" do + expect(list).to eq [] end end end @@ -173,22 +173,22 @@ module OpenFoodNetwork context "when reject? returns false for the element" do before do allow(applicator).to receive(:reject?).with(element) { false } - applicator.filter!(subject) + applicator.filter!(list) end - it "does not remove the element from the original subject" do - expect(subject).to eq [element] + it "does not remove the element from the original list" do + expect(list).to eq [element] end end context "when reject? returns true for the element" do before do allow(applicator).to receive(:reject?).with(element) { true } - applicator.filter!(subject) + applicator.filter!(list) end - it "removes the element from the original subject" do - expect(subject).to eq [] + it "removes the element from the original list" do + expect(list).to eq [] end end end From fcc68a0a19dd28b0b3e8888d0be525ac49d27516 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 1 Mar 2023 14:05:06 +1100 Subject: [PATCH 06/10] Return new filtered list instead of modifying This makes a clear distinction between the unfiltered and filtered list. There may also be some gotchas when modifying the array of an ActiveRecord relation. It also allows us to write shorter code without storing a separate variable. --- .../order_available_payment_methods.rb | 4 +-- .../order_available_shipping_methods.rb | 2 -- app/services/shop/order_cycles_list.rb | 4 --- lib/open_food_network/tag_rule_applicator.rb | 2 +- .../tag_rule_applicator_spec.rb | 34 ++++++++++--------- 5 files changed, 20 insertions(+), 26 deletions(-) diff --git a/app/services/order_available_payment_methods.rb b/app/services/order_available_payment_methods.rb index c3f71cc1da..b5c9a27dd2 100644 --- a/app/services/order_available_payment_methods.rb +++ b/app/services/order_available_payment_methods.rb @@ -19,8 +19,6 @@ class OrderAvailablePaymentMethods applicator = OpenFoodNetwork::TagRuleApplicator.new(distributor, "FilterPaymentMethods", customer&.tag_list) applicator.filter!(payment_methods) - - payment_methods.uniq end private @@ -32,7 +30,7 @@ class OrderAvailablePaymentMethods distributor.payment_methods.where( id: available_distributor_payment_methods_ids ) - end.available.select(&:configured?) + end.available.select(&:configured?).uniq end def available_distributor_payment_methods_ids diff --git a/app/services/order_available_shipping_methods.rb b/app/services/order_available_shipping_methods.rb index d689e88e22..8c861211ad 100644 --- a/app/services/order_available_shipping_methods.rb +++ b/app/services/order_available_shipping_methods.rb @@ -17,8 +17,6 @@ class OrderAvailableShippingMethods shipping_methods = shipping_methods_before_tag_rules_applied tag_rules.filter!(shipping_methods) - - shipping_methods end private diff --git a/app/services/shop/order_cycles_list.rb b/app/services/shop/order_cycles_list.rb index 6203637372..7607cdf9ab 100644 --- a/app/services/shop/order_cycles_list.rb +++ b/app/services/shop/order_cycles_list.rb @@ -29,15 +29,11 @@ module Shop private - # order_cycles is a ActiveRecord::Relation that is modified with reject in the TagRuleApplicator - # If this relation is reloaded (for example by calling count on it), the modifications are lost def apply_tag_rules!(order_cycles) applicator = OpenFoodNetwork::TagRuleApplicator.new(@distributor, "FilterOrderCycles", @customer&.tag_list) applicator.filter!(order_cycles) - - order_cycles end end end diff --git a/lib/open_food_network/tag_rule_applicator.rb b/lib/open_food_network/tag_rule_applicator.rb index b606e57438..547e5bd52b 100644 --- a/lib/open_food_network/tag_rule_applicator.rb +++ b/lib/open_food_network/tag_rule_applicator.rb @@ -14,7 +14,7 @@ module OpenFoodNetwork end def filter!(subject) - subject.to_a.reject! do |element| + subject.to_a.reject do |element| if rule_class.respond_to?(:tagged_children_for) children = rule_class.tagged_children_for(element) children.reject! { |child| reject?(child) } diff --git a/spec/lib/open_food_network/tag_rule_applicator_spec.rb b/spec/lib/open_food_network/tag_rule_applicator_spec.rb index 2dd78b63a6..cc6354ea9a 100644 --- a/spec/lib/open_food_network/tag_rule_applicator_spec.rb +++ b/spec/lib/open_food_network/tag_rule_applicator_spec.rb @@ -136,15 +136,16 @@ module OpenFoodNetwork before do allow(applicator).to receive(:reject?).with(child1) { true } allow(applicator).to receive(:reject?).with(child2) { false } - applicator.filter!(list) end it "rejects the specified children from the array" do + applicator.filter!(list) expect(children).to eq [child2] end - it "does not remove the element from the original list" do - expect(list).to eq [element] + it "does not remove the element from the result" do + result = applicator.filter!(list) + expect(result).to eq [element] end end @@ -152,15 +153,16 @@ module OpenFoodNetwork before do allow(applicator).to receive(:reject?).with(child1) { true } allow(applicator).to receive(:reject?).with(child2) { true } - applicator.filter!(list) end it "removes all children from the array" do + applicator.filter!(list) expect(children).to eq [] end - it "removes the element from the original list" do - expect(list).to eq [] + it "removes the element from the result" do + result = applicator.filter!(list) + expect(result).to eq [] end end end @@ -173,22 +175,22 @@ module OpenFoodNetwork context "when reject? returns false for the element" do before do allow(applicator).to receive(:reject?).with(element) { false } - applicator.filter!(list) end - it "does not remove the element from the original list" do - expect(list).to eq [element] + it "does not remove the element from the result" do + result = applicator.filter!(list) + expect(result).to eq [element] end end context "when reject? returns true for the element" do before do allow(applicator).to receive(:reject?).with(element) { true } - applicator.filter!(list) end - it "removes the element from the original list" do - expect(list).to eq [] + it "removes the element from the result" do + result = applicator.filter!(list) + expect(result).to eq [] end end end @@ -258,8 +260,8 @@ module OpenFoodNetwork } it "applies the default rule" do - applicator.filter!(products_array) - expect(products_array).to eq [ + result = applicator.filter!(products_array) + expect(result).to eq [ { :id => 2, :name => 'product 2', "variants" => [{ :id => 9, "tag_list" => ["tag2"] }] }, product3 ] @@ -273,8 +275,8 @@ module OpenFoodNetwork it "applies those rules" do # product_tag_rule1 and product_tag_rule2 are being applied - applicator.filter!(products_array) - expect(products_array).to eq [product1, product2] + result = applicator.filter!(products_array) + expect(result).to eq [product1, product2] end end end From b5f43b3c1c2119469666d6c2369bcdb4659b969a Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 3 Mar 2023 11:55:51 +1100 Subject: [PATCH 07/10] Rename method because it's not changing the input A common Ruby convention is to to use the bang naming `!` to mark method which change the given parameter instead of returning a copy. --- .../order_available_payment_methods.rb | 2 +- .../order_available_shipping_methods.rb | 2 +- app/services/shop/order_cycles_list.rb | 2 +- lib/open_food_network/tag_rule_applicator.rb | 2 +- .../tag_rule_applicator_spec.rb | 22 +++++++++---------- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/services/order_available_payment_methods.rb b/app/services/order_available_payment_methods.rb index b5c9a27dd2..dc0784d909 100644 --- a/app/services/order_available_payment_methods.rb +++ b/app/services/order_available_payment_methods.rb @@ -18,7 +18,7 @@ class OrderAvailablePaymentMethods applicator = OpenFoodNetwork::TagRuleApplicator.new(distributor, "FilterPaymentMethods", customer&.tag_list) - applicator.filter!(payment_methods) + applicator.filter(payment_methods) end private diff --git a/app/services/order_available_shipping_methods.rb b/app/services/order_available_shipping_methods.rb index 8c861211ad..b9a6cb3a7e 100644 --- a/app/services/order_available_shipping_methods.rb +++ b/app/services/order_available_shipping_methods.rb @@ -16,7 +16,7 @@ class OrderAvailableShippingMethods shipping_methods = shipping_methods_before_tag_rules_applied - tag_rules.filter!(shipping_methods) + tag_rules.filter(shipping_methods) end private diff --git a/app/services/shop/order_cycles_list.rb b/app/services/shop/order_cycles_list.rb index 7607cdf9ab..2328b0b0b7 100644 --- a/app/services/shop/order_cycles_list.rb +++ b/app/services/shop/order_cycles_list.rb @@ -33,7 +33,7 @@ module Shop applicator = OpenFoodNetwork::TagRuleApplicator.new(@distributor, "FilterOrderCycles", @customer&.tag_list) - applicator.filter!(order_cycles) + applicator.filter(order_cycles) end end end diff --git a/lib/open_food_network/tag_rule_applicator.rb b/lib/open_food_network/tag_rule_applicator.rb index 547e5bd52b..d37d485e1d 100644 --- a/lib/open_food_network/tag_rule_applicator.rb +++ b/lib/open_food_network/tag_rule_applicator.rb @@ -13,7 +13,7 @@ module OpenFoodNetwork @customer_tags = customer_tags || [] end - def filter!(subject) + def filter(subject) subject.to_a.reject do |element| if rule_class.respond_to?(:tagged_children_for) children = rule_class.tagged_children_for(element) diff --git a/spec/lib/open_food_network/tag_rule_applicator_spec.rb b/spec/lib/open_food_network/tag_rule_applicator_spec.rb index cc6354ea9a..7ec0e29f2a 100644 --- a/spec/lib/open_food_network/tag_rule_applicator_spec.rb +++ b/spec/lib/open_food_network/tag_rule_applicator_spec.rb @@ -109,15 +109,15 @@ module OpenFoodNetwork end end - describe "filter!" do + describe "filter" do let(:applicator) { OpenFoodNetwork::TagRuleApplicator.new(enterprise, "FilterProducts", []) } it "handles nil" do - applicator.filter!(nil) + applicator.filter(nil) end it "handles an empty collection" do - applicator.filter!(OrderCycle.none) + applicator.filter(OrderCycle.none) end context "when filtering an array" do @@ -139,12 +139,12 @@ module OpenFoodNetwork end it "rejects the specified children from the array" do - applicator.filter!(list) + applicator.filter(list) expect(children).to eq [child2] end it "does not remove the element from the result" do - result = applicator.filter!(list) + result = applicator.filter(list) expect(result).to eq [element] end end @@ -156,12 +156,12 @@ module OpenFoodNetwork end it "removes all children from the array" do - applicator.filter!(list) + applicator.filter(list) expect(children).to eq [] end it "removes the element from the result" do - result = applicator.filter!(list) + result = applicator.filter(list) expect(result).to eq [] end end @@ -178,7 +178,7 @@ module OpenFoodNetwork end it "does not remove the element from the result" do - result = applicator.filter!(list) + result = applicator.filter(list) expect(result).to eq [element] end end @@ -189,7 +189,7 @@ module OpenFoodNetwork end it "removes the element from the result" do - result = applicator.filter!(list) + result = applicator.filter(list) expect(result).to eq [] end end @@ -260,7 +260,7 @@ module OpenFoodNetwork } it "applies the default rule" do - result = applicator.filter!(products_array) + result = applicator.filter(products_array) expect(result).to eq [ { :id => 2, :name => 'product 2', "variants" => [{ :id => 9, "tag_list" => ["tag2"] }] }, product3 @@ -275,7 +275,7 @@ module OpenFoodNetwork it "applies those rules" do # product_tag_rule1 and product_tag_rule2 are being applied - result = applicator.filter!(products_array) + result = applicator.filter(products_array) expect(result).to eq [product1, product2] end end From 73fa6295ecaeac77ad6de3cc44dafe85b607ce3f Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 1 Mar 2023 14:10:58 +1100 Subject: [PATCH 08/10] Simplify shipping method selection code --- app/services/order_available_shipping_methods.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/services/order_available_shipping_methods.rb b/app/services/order_available_shipping_methods.rb index b9a6cb3a7e..0a2cdd15ba 100644 --- a/app/services/order_available_shipping_methods.rb +++ b/app/services/order_available_shipping_methods.rb @@ -14,14 +14,12 @@ class OrderAvailableShippingMethods def to_a return [] if distributor.blank? - shipping_methods = shipping_methods_before_tag_rules_applied - tag_rules.filter(shipping_methods) end private - def shipping_methods_before_tag_rules_applied + def shipping_methods if order_cycle.nil? || order_cycle.simple? distributor.shipping_methods else From 6b25c36476dbd800c15bc72b4d639a6048dd6fdb Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 3 Mar 2023 11:12:24 +1100 Subject: [PATCH 09/10] Filter ship methods by required shipping category The products in an order may require several shipping categories like refrigeration or express. We now filter the categories according to their support of shipping categories. --- .../order_available_shipping_methods.rb | 12 +++++- .../order_available_shipping_methods_spec.rb | 38 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/app/services/order_available_shipping_methods.rb b/app/services/order_available_shipping_methods.rb index 0a2cdd15ba..d70d15e331 100644 --- a/app/services/order_available_shipping_methods.rb +++ b/app/services/order_available_shipping_methods.rb @@ -14,11 +14,21 @@ class OrderAvailableShippingMethods def to_a return [] if distributor.blank? - tag_rules.filter(shipping_methods) + filter_by_category(tag_rules.filter(shipping_methods)) end private + def filter_by_category(methods) + required_category_ids = order.products.pluck(:shipping_category_id).to_set + return methods if required_category_ids.empty? + + methods.select do |method| + provided_category_ids = method.shipping_categories.pluck(:id).to_set + required_category_ids.subset?(provided_category_ids) + end + end + def shipping_methods if order_cycle.nil? || order_cycle.simple? distributor.shipping_methods diff --git a/spec/services/order_available_shipping_methods_spec.rb b/spec/services/order_available_shipping_methods_spec.rb index dc4c80134d..33a4550fdf 100644 --- a/spec/services/order_available_shipping_methods_spec.rb +++ b/spec/services/order_available_shipping_methods_spec.rb @@ -214,4 +214,42 @@ describe OrderAvailableShippingMethods do end end end + + context "when certain shipping categories are required" do + subject { OrderAvailableShippingMethods.new(order) } + let(:order) { + build(:order, distributor: distributor, order_cycle: oc) + } + let(:oc) { create(:order_cycle) } + let(:distributor) { oc.distributors.first } + let(:standard_shipping) { + create(:shipping_method, distributors: [distributor], shipping_categories: [bike_transport]) + } + let(:cooled_shipping) { + create(:shipping_method, distributors: [distributor], shipping_categories: [refrigerated]) + } + let(:bike_transport) { Spree::ShippingCategory.new(name: "bike") } + let(:refrigerated) { Spree::ShippingCategory.new(name: "fridge") } + + before { + standard_shipping + cooled_shipping + } + + it "provides all shipping methods for an empty order" do + expect(subject.to_a).to match_array [standard_shipping, cooled_shipping] + end + + it "provides all shipping methods for normal products" do + order.line_items << build(:line_item) + expect(subject.to_a).to match_array [standard_shipping, cooled_shipping] + end + + it "filters shipping methods for products needing refrigeration" do + product = oc.products.first + product.update!(shipping_category: refrigerated) + order.line_items << build(:line_item, variant: product.variants.first) + expect(subject.to_a).to eq [cooled_shipping] + end + end end From 31db35675bbcfa7d8f5e107ea3d182cd26410ece Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 20 Mar 2023 11:04:16 +1100 Subject: [PATCH 10/10] Add feature toggle for filtering shipping methods --- app/services/order_available_shipping_methods.rb | 3 +++ spec/services/order_available_shipping_methods_spec.rb | 2 ++ 2 files changed, 5 insertions(+) diff --git a/app/services/order_available_shipping_methods.rb b/app/services/order_available_shipping_methods.rb index d70d15e331..95603f9a68 100644 --- a/app/services/order_available_shipping_methods.rb +++ b/app/services/order_available_shipping_methods.rb @@ -20,6 +20,9 @@ class OrderAvailableShippingMethods private def filter_by_category(methods) + return methods unless OpenFoodNetwork::FeatureToggle.enabled?(:match_shipping_categories, + distributor&.owner) + required_category_ids = order.products.pluck(:shipping_category_id).to_set return methods if required_category_ids.empty? diff --git a/spec/services/order_available_shipping_methods_spec.rb b/spec/services/order_available_shipping_methods_spec.rb index 33a4550fdf..315f2397c4 100644 --- a/spec/services/order_available_shipping_methods_spec.rb +++ b/spec/services/order_available_shipping_methods_spec.rb @@ -234,6 +234,8 @@ describe OrderAvailableShippingMethods do before { standard_shipping cooled_shipping + + Flipper.enable(:match_shipping_categories) } it "provides all shipping methods for an empty order" do