Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of Taxon promotion rule #1409

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 25 additions & 24 deletions core/app/models/spree/promotion/rules/taxon.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down
28 changes: 14 additions & 14 deletions core/spec/models/spree/promotion/rules/taxon_spec.rb
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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

Expand All @@ -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]
Expand All @@ -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

Expand Down