Merge pull request #2258 from solidusio/mtomov-improve-performance-of…

…-taxon-rule

Improve performance of Taxon promotion rule

core/app/models/spree/promotion/rules/taxon.rb

@@ -16,42 +16,54 @@ def applicable?(promotable)
         end
 
         def eligible?(order, _options = {})
-          order_taxons = taxons_in_order_including_parents(order)
+          order_taxons = taxons_in_order(order)
 
           case preferred_match_policy
           when 'all'
-            unless (taxons.to_a - order_taxons).empty?
+            matches_all = taxons.all? do |rule_taxon|
+              order_taxons.where(id: rule_taxon.self_and_descendants.ids).exists?
+            end
+
+            unless matches_all
               eligibility_errors.add(:base, eligibility_error_message(:missing_taxon))
             end
           when 'any'
-            unless taxons.any?{ |taxon| order_taxons.include? taxon }
+            unless order_taxons.where(id: rule_taxon_ids_with_children).exists?
               eligibility_errors.add(:base, eligibility_error_message(:no_matching_taxons))
             end
           when 'none'
-            unless taxons.none?{ |taxon| order_taxons.include? taxon }
+            if order_taxons.where(id: rule_taxon_ids_with_children).exists?
               eligibility_errors.add(:base, eligibility_error_message(:has_excluded_taxon))
             end
           else
             # Change this to an exception in a future version of Solidus
             warn_invalid_match_policy(assume: 'any')
-            unless taxons.any? { |taxon| order_taxons.include? taxon }
+            unless order_taxons.where(id: rule_taxon_ids_with_children).exists?
               eligibility_errors.add(:base, eligibility_error_message(:no_matching_taxons))
             end
           end
 
           eligibility_errors.empty?
         end
 
+        # TODO: Fix bug - well described by jhawthorn in #1409:
+        # `eligible?` checks the configured taxons and all descendants,
+        # `actionable?` only seems to check against the taxons themselves (not children)
         def actionable?(line_item)
+          found = Spree::Classification.where(
+            product_id: line_item.variant.product_id,
+            taxon_id: taxon_ids
+          ).exists?
+
           case preferred_match_policy
           when 'any', 'all'
-            taxon_product_ids.include?(line_item.variant.product_id)
+            found
           when 'none'
-            taxon_product_ids.exclude? line_item.variant.product_id
+            !found
           else
             # Change this to an exception in a future version of Solidus
             warn_invalid_match_policy(assume: 'any')
-            taxon_product_ids.include?(line_item.variant.product_id)
+            found
           end
         end
 
@@ -74,27 +86,16 @@ def warn_invalid_match_policy(assume:)
         end
 
         # All taxons in an order
-        def order_taxons(order)
-          Spree::Taxon.joins(products: { variants_including_master: :line_items }).where(spree_line_items: { order_id: order.id }).distinct
+        def taxons_in_order(order)
+          Spree::Taxon.joins(products: { variants_including_master: :line_items })
+            .where(spree_line_items: { order_id: order.id }).distinct
         end
 
         # ids of taxons rules and taxons rules children
-        def taxons_including_children_ids
-          taxons.flat_map { |taxon| taxon.self_and_descendants.ids }
-        end
-
-        # taxons order vs taxons rules and taxons rules children
-        def order_taxons_in_taxons_and_children(order)
-          order_taxons(order).where(id: taxons_including_children_ids)
-        end
-
-        def taxons_in_order_including_parents(order)
-          order_taxons_in_taxons_and_children(order).flat_map(&:self_and_ancestors).uniq
+        def rule_taxon_ids_with_children
+          taxons.flat_map { |taxon| taxon.self_and_descendants.ids }.uniq
         end
 
-        def taxon_product_ids
-          Spree::Product.joins(:taxons).where(spree_taxons: { id: taxons.pluck(:id) }).pluck(:id).uniq
-        end
       end
     end
   end

core/spec/models/spree/promotion/rules/taxon_spec.rb

@@ -1,29 +1,30 @@
 require 'rails_helper'
 
 RSpec.describe Spree::Promotion::Rules::Taxon, type: :model do
+  let(:taxon)   { create :taxon, name: 'first' }
+  let(:taxon2)  { create :taxon, name: 'second' }
+  let(:order)   { create :order_with_line_items }
+  let(:product) { order.products.first }
+
   let(:rule) do
     Spree::Promotion::Rules::Taxon.create!(promotion: create(:promotion))
   end
 
   context '#eligible?(order)' do
-    let(:taxon){ create :taxon, name: 'first' }
-    let(:taxon2){ create :taxon, name: 'second' }
-    let(:order){ create :order_with_line_items }
-
     context 'with any match policy' do
       before do
         rule.update!(preferred_match_policy: 'any')
       end
 
       it 'is eligible if order does have any prefered taxon' do
-        order.products.first.taxons << taxon
+        product.taxons << taxon
         rule.taxons << taxon
         expect(rule).to be_eligible(order)
       end
 
       context 'when order contains items from different taxons' do
         before do
-          order.products.first.taxons << taxon
+          product.taxons << taxon
           rule.taxons << taxon
         end
 
@@ -50,7 +51,7 @@
       context 'when a product has a taxon child of a taxon rule' do
         before do
           taxon.children << taxon2
-          order.products.first.taxons << taxon2
+          product.taxons << taxon2
           rule.taxons << taxon
         end
 
@@ -64,7 +65,7 @@
       end
 
       it 'is eligible order has all prefered taxons' do
-        order.products.first.taxons << taxon2
+        product.taxons << taxon2
         order.products.last.taxons << taxon
 
         rule.taxons = [taxon, taxon2]
@@ -83,17 +84,16 @@
       end
 
       context 'when a product has a taxon child of a taxon rule' do
-        let(:taxon3){ create :taxon }
+        let(:taxon3) { create :taxon }
 
         before do
           taxon.children << taxon2
-          order.products.first.taxons << taxon2
-          order.products.last.taxons << taxon3
-          rule.taxons << taxon2
-          rule.taxons << taxon3
+          taxon.save!
+          product.taxons = [taxon2, taxon3]
+          rule.taxons = [taxon, taxon3]
         end
 
-        it{ expect(rule).to be_eligible(order) }
+        it { expect(rule).to be_eligible(order) }
       end
     end