Merge pull request #6383 from mamhoff/in-memory-conditions

Allow promotion conditions to work with in-memory line items, improve performance

Author: mamhoff

promotions/app/models/concerns/solidus_promotions/conditions/taxon_condition.rb

@@ -25,11 +25,52 @@ def taxon_ids_string=(taxon_ids)
         self.taxons = Spree::Taxon.find(taxon_ids)
       end
 
+      def taxons_ids_with_children=(args)
+        @taxon_ids_with_children = args
+      end
+
       private
 
-      # ids of taxons conditions and taxons conditions children
-      def condition_taxon_ids_with_children
-        taxons.flat_map { |taxon| taxon.self_and_descendants.ids }.uniq
+      # Returns the cached list of taxon subtree id collections for the selected taxons.
+      #
+      # Executes a single SQL query using the nested set (lft/rgt) boundaries to
+      # fetch each root taxon (one of this condition's taxons) together with all
+      # of its descendants. The result is memoized for the lifetime of the
+      # condition instance.
+      #
+      # Each inner array contains the IDs of a root taxon and all of its
+      # descendants (including the root itself). The outer array is ordered by
+      # the root taxon id.
+      #
+      # @return [Array<Array<Integer>>] array of arrays of taxon ids, one per root taxon
+      # @example
+      #   # For condition with taxons [10, 42]
+      #   condition.condition_taxon_ids_with_children
+      #   # => [[10, 11, 12], [42, 43]]
+      def taxon_ids_with_children
+        @taxon_ids_with_children ||= load_taxon_ids_with_children
+      end
+
+      def load_taxon_ids_with_children
+        aggregation_function = if ActiveRecord::Base.connection.adapter_name.downcase.match?(/postgres/)
+          "string_agg(child.id::text, ',')"
+        else
+          "group_concat(child.id, ',')"
+        end
+
+        sql = <<~SQL
+          SELECT
+            parent.id AS root_id,
+            #{aggregation_function} AS descendant_ids
+          FROM spree_taxons AS parent
+          JOIN spree_taxons AS child
+            ON child.lft BETWEEN parent.lft AND parent.rgt
+          WHERE parent.id IN (#{taxon_ids.join(',')})
+          GROUP BY parent.id
+          ORDER BY parent.id
+        SQL
+        rows = ActiveRecord::Base.connection.exec_query(sql)
+        rows.map { |r| r["descendant_ids"].split(",").map(&:to_i) }
       end
     end
   end

promotions/app/models/solidus_promotions/conditions/line_item_option_value.rb

@@ -10,7 +10,7 @@ class LineItemOptionValue < Condition
 
       def line_item_eligible?(line_item, _options = {})
         pid = line_item.product.id
-        ovids = line_item.variant.option_values.pluck(:id)
+        ovids = line_item.variant.option_value_ids
 
         product_ids.include?(pid) && (value_ids(pid) & ovids).present?
       end

promotions/app/models/solidus_promotions/conditions/line_item_taxon.rb

@@ -15,16 +15,13 @@ class LineItemTaxon < Condition
       preference :match_policy, :string, default: MATCH_POLICIES.first
 
       def line_item_eligible?(line_item, _options = {})
-        found = Spree::Classification.where(
-          product_id: line_item.variant.product_id,
-          taxon_id: condition_taxon_ids_with_children
-        ).exists?
+        line_item_taxon_ids = line_item.variant.product.classifications.map(&:taxon_id)
 
         case preferred_match_policy
         when "include"
-          found
+          taxon_ids_with_children.any? { |taxon_and_descendant_ids| (line_item_taxon_ids & taxon_and_descendant_ids).any? }
         when "exclude"
-          !found
+          taxon_ids_with_children.none? { |taxon_and_descendant_ids| (line_item_taxon_ids & taxon_and_descendant_ids).any? }
         else
           raise "unexpected match policy: #{preferred_match_policy.inspect}"
         end

promotions/app/models/solidus_promotions/conditions/order_taxon.rb

@@ -12,27 +12,26 @@ class OrderTaxon < Condition
       preference :match_policy, :string, default: MATCH_POLICIES.first
 
       def order_eligible?(order, _options = {})
-        order_taxons = taxons_in_order(order)
+        line_item_taxon_ids = taxon_ids_in_order(order)
 
         case preferred_match_policy
         when "all"
-          matches_all = taxons.all? do |condition_taxon|
-            order_taxons.where(id: condition_taxon.self_and_descendants.ids).exists?
-          end
+          unless taxon_ids_with_children.all? { |taxon_and_descendant_ids| (line_item_taxon_ids & taxon_and_descendant_ids).any? }
 
-          unless matches_all
             eligibility_errors.add(:base, eligibility_error_message(:missing_taxon), error_code: :missing_taxon)
           end
         when "any"
-          unless order_taxons.where(id: condition_taxon_ids_with_children).exists?
+          if taxon_ids_with_children.none? { |taxon_and_descendant_ids| (line_item_taxon_ids & taxon_and_descendant_ids).any? }
+
             eligibility_errors.add(
               :base,
               eligibility_error_message(:no_matching_taxons),
               error_code: :no_matching_taxons
             )
           end
         when "none"
-          if order_taxons.where(id: condition_taxon_ids_with_children).exists?
+          if taxon_ids_with_children.any? { |taxon_and_descendant_ids| (line_item_taxon_ids & taxon_and_descendant_ids).any? }
+
             eligibility_errors.add(
               :base,
               eligibility_error_message(:has_excluded_taxon),
@@ -50,12 +49,11 @@ def to_partial_path
 
       private
 
-      # All taxons in an order
-      def taxons_in_order(order)
-        Spree::Taxon
-          .joins(products: { variants_including_master: :line_items })
-          .where(spree_line_items: { order_id: order.id })
-          .distinct
+      # All taxon IDs in an order
+      def taxon_ids_in_order(order)
+        order.line_items.flat_map do |line_item|
+          line_item.variant.product.classifications.map(&:taxon_id)
+        end
       end
     end
   end

promotions/app/models/solidus_promotions/conditions/taxon.rb

@@ -15,6 +15,10 @@ class Taxon < Condition
 
       def order_eligible?(order, _options = {})
         order_condition = OrderTaxon.new(taxons:, preferred_match_policy:)
+
+        # Hydrate the instance cache with our @taxon_ids_with_children cache
+        order_condition.taxons_ids_with_children = taxon_ids_with_children
+
         order_condition.order_eligible?(order)
         @eligibility_errors = order_condition.eligibility_errors
         eligibility_errors.empty?
@@ -23,6 +27,10 @@ def order_eligible?(order, _options = {})
       def line_item_eligible?(line_item, _options = {})
         line_item_match_policy = preferred_match_policy.in?(%w[any all]) ? "include" : "exclude"
         line_item_condition = LineItemTaxon.new(taxons:, preferred_match_policy: line_item_match_policy)
+
+        # Hydrate the instance cache with our @taxon_ids_with_children cache\
+        line_item_condition.taxons_ids_with_children = taxon_ids_with_children
+
         result = line_item_condition.line_item_eligible?(line_item)
         @eligibility_errors = line_item_condition.eligibility_errors
         result

promotions/app/models/solidus_promotions/distributed_amounts_handler.rb

@@ -12,7 +12,7 @@ def initialize(line_items, total_amount)
     # @param line_item [LineItem] one of the line_items distributed over
     # @return [BigDecimal] the weighted adjustment for this line_item
     def amount(line_item)
-      distributed_amounts[line_item.id].to_d
+      distributed_amounts[line_item].to_d
     end
 
     private
@@ -21,11 +21,7 @@ def amount(line_item)
     # @return [Hash<Integer, BigDecimal>] a hash of line item IDs and their
     #   corresponding weighted adjustments
     def distributed_amounts
-      line_item_ids.zip(allocated_amounts).to_h
-    end
-
-    def line_item_ids
-      line_items.map(&:id)
+      line_items.zip(allocated_amounts).to_h
     end
 
     def elligible_amounts

promotions/spec/models/solidus_promotions/conditions/order_taxon_spec.rb

@@ -8,13 +8,17 @@
   let(:condition) do
     described_class.create!(benefit: promotion_benefit)
   end
-  let(:product) { order.products.first }
-  let(:order) { create :order_with_line_items }
+  let(:product) { create(:product) }
+  let(:order) { create :order }
   let(:taxon_one) { create :taxon, name: "first" }
   let(:taxon_two) { create :taxon, name: "second" }
 
   it_behaves_like "a taxon condition"
 
+  before do
+    order.line_items.new(quantity: 1, variant: product.master)
+  end
+
   it { is_expected.to be_updateable }
 
   describe "#eligible?(order)" do
@@ -66,7 +70,7 @@
 
       it "is eligible order has all prefered taxons" do
         product.taxons << taxon_two
-        order.products.last.taxons << taxon_one
+        product.taxons << taxon_one
 
         condition.taxons = [taxon_one, taxon_two]
 
@@ -121,7 +125,7 @@
 
       context "one of the order's products is in a listed taxon" do
         before do
-          order.products.first.taxons << taxon_one
+          product.taxons << taxon_one
           condition.taxons << taxon_one
         end
 

promotions/spec/models/solidus_promotions/conditions/taxon_spec.rb

@@ -187,4 +187,21 @@
       end
     end
   end
+
+  describe "performance" do
+    let(:product_one) { create(:product, taxons: [taxon_one]) }
+    let(:product_two) { create(:product, taxons: [taxon_two]) }
+    let!(:promotion) { create(:solidus_promotion, :with_adjustable_benefit, apply_automatically: true) }
+    let(:benefit) { promotion.benefits.first }
+    let!(:condition) { described_class.create!(taxons: [taxon_one, taxon_two], benefit:) }
+    let!(:order) { create(:order_with_line_items, line_items_attributes: [{ variant: product_one.master }, { variant: product_two.master }]) }
+
+    subject { order.recalculate }
+
+    it "only loads the taxon IDs with children once" do
+      expect_any_instance_of(SolidusPromotions::Conditions::TaxonCondition).to receive(:load_taxon_ids_with_children).once { [[taxon_one.id], [taxon_two.id]] }
+
+      subject
+    end
+  end
 end