From 3a28677d5037c9333c6156ea6dc13d0536397e04 Mon Sep 17 00:00:00 2001
From: Martin Tomov <mtomov@outlook.com>
Date: Mon, 29 Aug 2016 10:12:56 +0100
Subject: [PATCH] Improve performance of Taxon promotion rule

  1) Before #actionable? was bringing all product ids of the rule's taxons
     from the database. On a particular case in a store with 1mil
     products, the promotion was bringing 0.6mil of them, with a query ~ 5sec
     and 5mil ids in Ruby.

     After, the exists? query is a milliseconds one.


  2) Before #eligible? was fetching all parent taxons of each taxon on a
     product, which is not required for the match process.

     So, given an order with 10 line items, with each line item having a
     product with 3 taxons, each of them having 2 parent taxons
     would mean 30 queries to the db to fetch the ancestors, bringing a
     total of 60 taxons in Ruby memory.

     All of them unnecessary, as the matching processes can be done solely
     based on the rule's taxons, which if 5 would mean 5 queries to the DB
     (self_and_descendants)


  3) Fix incorrect test on #eligible?#all for taxon child of a taxon rule
     case.

     * there were no children tested in the case


  4) On #eligible?#all, take only the IDs from the DB to
     reduce memory overhead
---
 .../app/models/spree/promotion/rules/taxon.rb | 49 ++++++++++---------
 .../spree/promotion/rules/taxon_spec.rb       | 28 +++++------
 2 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/core/app/models/spree/promotion/rules/taxon.rb b/core/app/models/spree/promotion/rules/taxon.rb
index 7c1bbd3e077..4ff7cf919fc 100644
--- a/core/app/models/spree/promotion/rules/taxon.rb
+++ b/core/app/models/spree/promotion/rules/taxon.rb
@@ -16,25 +16,29 @@ 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
@@ -42,16 +46,24 @@ def eligible?(order, _options = {})
           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
diff --git a/core/spec/models/spree/promotion/rules/taxon_spec.rb b/core/spec/models/spree/promotion/rules/taxon_spec.rb
index 600e67c9ebd..3ac66d92a14 100644
--- a/core/spec/models/spree/promotion/rules/taxon_spec.rb
+++ b/core/spec/models/spree/promotion/rules/taxon_spec.rb
@@ -1,29 +1,30 @@
 require 'spec_helper'
 
 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