solidusio
Promotion eligibility with line item adjustments is inaccurate
What's the problem?
Promotions that create line item adjustments do not have the eligibility of the adjustment correctly updated after future changes to the order.
Steps to reproduce
The easiest way to reproduce this issue is with the product rule.
Using a sandbox store, create a promotion that uses the product rule and the "match all" option with a line item adjustment action.
Next, create an order an order and successfully apply the promotion to it.
Finally, remove one of the items required for the promotion to be eligible.
Expected behavior
The adjustments on the remaining item(s) in the cart are marked ineligible.
Actual behavior
The adjustments on the remaining item(s) in the cart are still eligible.
Why isn't this a bigger issue?
Our two promotion actions that add item-level adjustments, CreateItemAdjustments and CreateQuantityAdjustments, happen to have some extra code in their compute_amount() methods that will set the amount to $0 when the promotion becomes ineligible.
How's it happening?
The code for determining the eligibility of an adjustment is unfortunately a bit of a mess that spans at least three classes across four files.
We start our journey in Spree::Adjustment where we check the eligibility any time a promotion adjustment is recalculated. This code simply delegates the check to the promotion itself, passing through the adjustable which will be a Spree::LineItem.
# models/spree/adjustment.rb : line 138
def calculate_eligibility
if !finalized? && source && promotion?
source.promotion.eligible?(adjustable, promotion_code: promotion_code)
else
eligible?
end
end
Spree::Promotion's eligibility check goes through a few simpler checks before getting to the rules associated to the order.
# models/spree/promotion.rb : line 123
def eligible?(promotable, promotion_code: nil)
return false if inactive?
return false if usage_limit_exceeded?
return false if promotion_code && promotion_code.usage_limit_exceeded?
return false if blacklisted?(promotable)
!!eligible_rules(promotable, {})
end
One of the first things Spree::Promotion does when checking eligible_rules() is find the list of rules that "applicable" to the current promotable object. (As a reminder, in this case that will be the Spree::LineItem our adjustment applies to.)
# models/spree/promotion.rb : line 134
def eligible_rules(promotable, options = {})
# Promotions without rules are eligible by default.
return [] if rules.none?
eligible = lambda { |r| r.eligible?(promotable, options) }
specific_rules = rules.for(promotable) # THIS IS THE LINE IN QUESTION
return [] if specific_rules.none?
...
Spree::PromotionRule.for() will go through the rules and select the ones where the rule is "applicable" to the promotable.
# models/spree/promotion_rule.rb : line 11
def self.for(promotable)
all.select { |rule| rule.applicable?(promotable) }
end
A quick search will tell you that all of the rules in core are only "applicable" to a Spree::Order. As a result, no rules will be returned and specific_rules will be empty.
This will bubble up the chain resulting the the adjustment being considered eligible despite the promotion no longer being eligible for the order.
How could we fix it?
I don't believe the concept of "applicable" on Spree::PromotionRule serves any practical purpose. I'm in favour of removing it entirely which would help simplify a large portion of this code. However, I can understand some trepidation in removing something that has been around since the introduction of item-level adjustments.
One possible, less extreme, solution would be to update our rules to work regardless of the passed promotable being a Spree::Order or not. (i.e.: If a line item or shipment is passed, fetching the order through its associations.) Alternatively, we could update Spree::Promotion to always pass the order through to the rules and remove the specific_rules() scoping altogether.
We actually run into an issue similar. I explain it here in case it helps to have more information for a possible solution. After investigating the bug we got to the point where these 2 lines were causing the bug.
# models/spree/promotion.rb : line 134
specific_rules = rules.for(promotable) # THIS IS THE LINE IN QUESTION
return [] if specific_rules.none?
In our case we noted that for a promotion of type "free shipping", when we add a line item to the cart, we have:
promotion.elligible?(line_item) == true
This is because specific_rules == [] and thus eligible_rules(line_item) == []
It does not seem logical. I was expecting a free shipping promotion to not be eligible for a line item. That may be the case when running the promotion against the order but not against the line item.
as a custom fix for our app, I followed your advice to remove completely the specific_rules, and always work with the order and do the elligible tests agains the rules like this:
def eligible_rules(promotable, options = {})
# Promotions without rules are eligible by default.
return [] if rules.none?
eligible = ->(r) do
order = promotable.is_a?(Spree::Order) ? promotable : promotable.order
r.eligible?(order, options)
end
if match_all?
# If there are rules for this promotion, but no rules for this
# particular promotable, then the promotion is ineligible by default.
unless rules.all?(&eligible)
@eligibility_errors = rules.map(&:eligibility_errors).find(&:present?)
return nil
end
rules
else
unless rules.any?(&eligible)
@eligibility_errors = rules.map(&:eligibility_errors).find(&:present?)
return nil
end
rules.select(&eligible)
end
end
This seems to be working for us so far...
@loicginoux we've been using your fix within our app, but I just had to make a small change to it to fix a puzzling problem I've been banging my head over:
In the locations you're calling the eligible Lambda with ampersand you're turning eligible into a Proc.
rules.all?(&eligible)
You should change all instances to a block instead to maintain the Lambda behavior:
rules.all? { |rule| eligible.call(rule) }
Or else you're going to wind up with all sorts of promotion issues that are hard to track down. Checking a promotion eligibility with a Proc can end up causing methods to not be invoked properly as the Lambda will sometimes prevent later lines of code from running as expected.
So now it'd be:
def eligible_rules(promotable, options = {})
# Promotions without rules are eligible by default.
return [] if rules.none?
eligible = ->(r) do
order = promotable.is_a?(Spree::Order) ? promotable : promotable.order
r.eligible?(order, options)
end
if match_all?
# If there are rules for this promotion, but no rules for this
# particular promotable, then the promotion is ineligible by default.
unless rules.all? { |rule| eligible.call(rule) }
@eligibility_errors = rules.map(&:eligibility_errors).find(&:present?)
return nil
end
rules
else
unless rules.any? { |rule| eligible.call(rule) }
@eligibility_errors = rules.map(&:eligibility_errors).find(&:present?)
return nil
end
rules.select { |rule| eligible.call(rule) }
end
end
http://vaidehijoshi.github.io/blog/2015/06/02/code-smells-and-ruby-shorthand-unpacking-ampersand-plus-to-proc/ https://docs.ruby-lang.org/en/2.6.0/Proc.html
Hi,
Thanks for all that stuff.
Any updates on this one ?
The trouble is still available on 3.0.8.
Another track on it, is around the adjustment call in the calculate_eligibility. For me it is strange to try to call applicable? on the adjustable object.
I don't see, any links between the object on which apply the promotion with the rules and the object on which we make adjustment, can you explain that part of code please ?
Maybe we should save the object on which we apply the promotion with the rules at the first time in the PromotionHandler part ? And next reuse it in the adjustment update part ?
Thanks
Just for information another way to handle only the adjustment eligibility part is to decorate the Spree::Promotion::OrderAdjustmentsRecalculator (tested in version 3.3.0) :
# frozen_string_literal: true
module Spree
module OrderAdjustmentsRecalculatorDecorator
# Calculates based on attached promotion (if this is a promotion
# adjustment) whether this promotion is still eligible.
# @api private
# @return [true,false] Whether this adjustment is eligible
def calculate_eligibility(adjustment)
if !adjustment.finalized? && adjustment.source
if adjustment.adjustable.is_a?(Spree::Order)
adjustment.source.promotion.eligible?(adjustment.adjustable, promotion_code: adjustment.promotion_code)
else
adjustment.source.promotion.eligible?(adjustment.order, promotion_code: adjustment.promotion_code)
end
else
adjustment.eligible?
end
end
Promotion::OrderAdjustmentsRecalculator.prepend self
end
end
By assuming all the PromotionRule apply to order.