From 50738f28e9f5180eed650e521507a7c6ff0a9faf Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Thu, 21 Apr 2016 14:17:44 +1000 Subject: [PATCH] Refactoring tag rule application To allow rules to be loaded and counted before being checked for relevance --- app/helpers/enterprises_helper.rb | 6 ++- app/models/enterprise.rb | 12 +++-- app/models/spree/order_decorator.rb | 2 +- app/models/tag_rule.rb | 14 ++++-- spec/helpers/injection_helper_spec.rb | 2 +- spec/models/enterprise_spec.rb | 50 +++++++++++++++++++ spec/models/tag_rule/discount_order_spec.rb | 6 +-- .../tag_rule/filter_shipping_methods_spec.rb | 2 +- spec/models/tag_rule_spec.rb | 34 +++++++------ 9 files changed, 97 insertions(+), 31 deletions(-) diff --git a/app/helpers/enterprises_helper.rb b/app/helpers/enterprises_helper.rb index 93b7c96f95..b4375b2577 100644 --- a/app/helpers/enterprises_helper.rb +++ b/app/helpers/enterprises_helper.rb @@ -6,7 +6,11 @@ module EnterprisesHelper def available_shipping_methods shipping_methods = current_distributor.shipping_methods if current_distributor.present? - current_distributor.apply_tag_rules_to(shipping_methods, customer: current_order.customer) + current_distributor.apply_tag_rules( + type: "FilterShippingMethods", + subject: shipping_methods, + customer_tags: current_order.andand.customer.andand.tag_list + ) end shipping_methods.uniq end diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index 9f839f9d08..fbff30b153 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -344,9 +344,9 @@ class Enterprise < ActiveRecord::Base abn.present? end - def apply_tag_rules_to(subject, context) - tag_rules.each do |rule| - rule.set_context(subject,context) + def apply_tag_rules(context) + tag_rules_for(context).each do |rule| + rule.set_context(context) rule.apply end end @@ -459,4 +459,10 @@ class Enterprise < ActiveRecord::Base def initialize_permalink self.permalink = Enterprise.find_available_permalink(name) end + + def tag_rules_for(context) + rules = context[:rules] || tag_rules + return rules unless context[:type] + rules.merge(TagRule.of_type(context[:type])) + end end diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index ad1a877455..ee485e4afd 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -180,7 +180,7 @@ Spree::Order.class_eval do end if distributor.present? && customer.present? - distributor.apply_tag_rules_to(self, customer: customer) + distributor.apply_tag_rules(type: "DiscountOrder", subject: self, customer_tags: customer.andand.tag_list) end end end diff --git a/app/models/tag_rule.rb b/app/models/tag_rule.rb index 8d385673db..ce8cc7183d 100644 --- a/app/models/tag_rule.rb +++ b/app/models/tag_rule.rb @@ -9,11 +9,15 @@ class TagRule < ActiveRecord::Base attr_accessible :enterprise, :enterprise_id, :preferred_customer_tags - scope :for, lambda { |enterprises| where(enterprise_id: enterprises) } + scope :for, ->(enterprise) { where(enterprise_id: enterprise) } + scope :of_type, ->(type) { where(type: "TagRule::#{type}") } + + def set_context(context) + raise "Context for tag rule cannot be nil" if context.nil? + raise "Subject for tag rule cannot be nil" if context[:subject].nil? - def set_context(subject, context) - @subject = subject @context = context + @subject = context[:subject] end def apply @@ -50,8 +54,8 @@ class TagRule < ActiveRecord::Base end def customer_tags_match? - context_customer_tags = context.andand[:customer].andand.tag_list || [] + context_customer_tags = context[:customer_tags] || [] preferred_tags = preferred_customer_tags.split(",") - ( context_customer_tags & preferred_tags ).any? + (context_customer_tags & preferred_tags).any? end end diff --git a/spec/helpers/injection_helper_spec.rb b/spec/helpers/injection_helper_spec.rb index 3b62fb2660..d5ec204286 100644 --- a/spec/helpers/injection_helper_spec.rb +++ b/spec/helpers/injection_helper_spec.rb @@ -30,7 +30,7 @@ describe InjectionHelper do shipping_methods = double(:shipping_methods, uniq: [sm]) current_distributor = double(:distributor, shipping_methods: shipping_methods) allow(helper).to receive(:current_distributor) { current_distributor } - allow(current_distributor).to receive(:apply_tag_rules_to).with(shipping_methods, {customer: nil} ) + allow(current_distributor).to receive(:apply_tag_rules).with({type: "FilterShippingMethods", subject: shipping_methods, customer_tags: nil}) helper.inject_available_shipping_methods.should match sm.id.to_s helper.inject_available_shipping_methods.should match sm.compute_amount(order).to_s end diff --git a/spec/models/enterprise_spec.rb b/spec/models/enterprise_spec.rb index b8360094ec..f0fd6bca4c 100644 --- a/spec/models/enterprise_spec.rb +++ b/spec/models/enterprise_spec.rb @@ -859,4 +859,54 @@ describe Enterprise do end end end + + describe "finding tag rules" do + let(:enterprise) { create(:enterprise) } + let(:tag_rule1) { create(:filter_products_tag_rule, enterprise: enterprise) } + let(:tag_rule2) { create(:filter_shipping_methods_tag_rule, enterprise: enterprise) } + let(:tag_rule3) { create(:tag_rule, enterprise: enterprise) } + let(:context) { { subject: "something" } } + + context "when a set of rules are passed in with the context" do + let(:tag_rules) { TagRule.where(id: [tag_rule1.id, tag_rule3.id]) } + before { context[:rules] = tag_rules } + + context "when a rule type has been passed in with the context" do + before { context[:type] = "FilterProducts" } + + it "returns rules of the specified type from within the list of passed-in rules" do + rules = enterprise.send(:tag_rules_for, context) + expect(rules).to include tag_rule1 + expect(rules).to_not include tag_rule2, tag_rule3 + end + end + + context "when a rule type has not been passed in with the context" do + it "returns the list of passed-in rules" do + rules = enterprise.send(:tag_rules_for, context) + expect(rules).to include tag_rule1, tag_rule3 + expect(rules).to_not include tag_rule2 + end + end + end + + context "when a set of rules are not passed in with the context" do + context "when a rule type has been passed in with the context" do + before { context[:type] = "FilterShippingMethods" } + + it "returns rules of the specified type belonging to the enterprise" do + rules = enterprise.send(:tag_rules_for, context) + expect(rules).to include tag_rule2 + expect(rules).to_not include tag_rule1, tag_rule3 + end + end + + context "when a rule type has not been passed in with the context" do + it "returns all rules belonging to the enterprise" do + rules = enterprise.send(:tag_rules_for, context) + expect(rules).to include tag_rule1, tag_rule2, tag_rule3 + end + end + end + end end diff --git a/spec/models/tag_rule/discount_order_spec.rb b/spec/models/tag_rule/discount_order_spec.rb index dab901dfbf..efb2d6e12c 100644 --- a/spec/models/tag_rule/discount_order_spec.rb +++ b/spec/models/tag_rule/discount_order_spec.rb @@ -7,7 +7,7 @@ describe TagRule::DiscountOrder, type: :model do let(:subject) { double(:subject) } before do - tag_rule.set_context(subject,{}) + tag_rule.set_context({subject: subject}) allow(tag_rule).to receive(:customer_tags_match?) { true } allow(subject).to receive(:class) { Spree::Order } end @@ -34,7 +34,7 @@ describe TagRule::DiscountOrder, type: :model do let!(:adjustment) { order.adjustments.create({:amount => 12.34, :source => order, :originator => tag_rule, :label => 'discount' }, :without_protection => true) } before do - tag_rule.set_context(order, nil) + tag_rule.set_context({subject: order}) end context "where adjustments originating from the rule already exist" do @@ -56,7 +56,7 @@ describe TagRule::DiscountOrder, type: :model do before do order.update_distribution_charge! tag_rule.calculator.update_attribute(:preferred_flat_percent, -10.00) - tag_rule.set_context(order, nil) + tag_rule.set_context({subject: order}) end context "in a simple scenario" do diff --git a/spec/models/tag_rule/filter_shipping_methods_spec.rb b/spec/models/tag_rule/filter_shipping_methods_spec.rb index ae05478587..7783e27b18 100644 --- a/spec/models/tag_rule/filter_shipping_methods_spec.rb +++ b/spec/models/tag_rule/filter_shipping_methods_spec.rb @@ -41,7 +41,7 @@ describe TagRule::FilterShippingMethods, type: :model do before do tag_rule.update_attribute(:preferred_shipping_method_tags, "tag2") - tag_rule.set_context(shipping_methods, nil) + tag_rule.set_context({subject: shipping_methods}) end context "apply!" do diff --git a/spec/models/tag_rule_spec.rb b/spec/models/tag_rule_spec.rb index 549c2f88aa..49b3e90ee2 100644 --- a/spec/models/tag_rule_spec.rb +++ b/spec/models/tag_rule_spec.rb @@ -11,9 +11,18 @@ describe TagRule, type: :model do describe 'setting the context' do let(:subject) { double(:subject) } - let(:context) { double(:context) } + let(:context) { { subject: subject, some_other_property: "yay"} } + + it "raises an error when context is nil" do + expect{ tag_rule.set_context(nil) }.to raise_error "Context for tag rule cannot be nil" + end + + it "raises an error when subject is nil" do + expect{ tag_rule.set_context({}) }.to raise_error "Subject for tag rule cannot be nil" + end + it "stores the subject and context provided as instance variables on the model" do - tag_rule.set_context(subject, context) + tag_rule.set_context(context) expect(tag_rule.subject).to eq subject expect(tag_rule.context).to eq context expect(tag_rule.instance_variable_get(:@subject)).to eq subject @@ -32,7 +41,7 @@ describe TagRule, type: :model do let(:subject) { double(:subject) } before do - tag_rule.set_context(subject,{}) + tag_rule.set_context({subject: subject}) allow(tag_rule).to receive(:customer_tags_match?) { :customer_tags_match_result } allow(tag_rule).to receive(:subject_class) { Spree::Order} end @@ -93,27 +102,20 @@ describe TagRule, type: :model do end describe "determining whether specified customer tags match the given context" do - context "when the context is nil" do - before { tag_rule.set_context(nil, nil) } - it "returns false" do - expect(tag_rule.send(:customer_tags_match?)).to be false - end - end + context "when the context has no customer tags specified" do + let(:context) { { subject: double(:something), not_tags: double(:not_tags) } } - context "when the context has no customer specified" do - let(:context) { { something_that_is_not_a_customer: double(:something) } } - - before { tag_rule.set_context(nil, context) } + before { tag_rule.set_context(context) } it "returns false" do expect(tag_rule.send(:customer_tags_match?)).to be false end end - context "when the context has a customer specified" do - let(:context) { { customer: double(:customer, tag_list: ["member","local","volunteer"] ) } } + context "when the context has customer tags specified" do + let(:context) { { subject: double(:something), customer_tags: ["member","local","volunteer"] } } - before { tag_rule.set_context(nil, context) } + before { tag_rule.set_context(context) } context "when the rule has no preferred customer tags specified" do before do