Add eligibility checking to automatic free shipping promotions #2187
Conversation
There was a problem hiding this comment.
@fylooi thank you!
I think we actually do need to verify eligibility on connected_promotions as well. It is possible for a promotion to become connected to an order (because it was eligible at one point) and then subsequently become ineligible (e.g. removing an item from the cart and shipments getting regenerated) but still connected (we don't disconnect promotions when the order becomes ineligible).
I've sent you a PR to that effect, targeted at the branch you are using in this PR: fylooi#1
FYI we just merged #2135 so you'll need to rebase on the latest master. The file you want to modify is now promotion_handler/shipping.rb instead of promotion_handler/free_shipping.rb. Would you mind doing that?
Thanks again.
| @@ -18,7 +18,7 @@ def activate | |||
| end | |||
|
|
|||
| not_connected_automatic_promotions.each do |promotion| | |||
| promotion.activate(order: order) | |||
| promotion.activate(order: order) if promotion.eligible?(order) | |||
There was a problem hiding this comment.
Would you mind making this a normal if instead of a trailing if? The condition is quite important, as you've pointed out, so it'd be nice to make sure it stands out
| it "creates the adjustment" do | ||
| expect { subject.activate }.to change { shipment.adjustments.count }.by(1) | ||
| context 'for eligible promotion' do | ||
| before { allow_any_instance_of(Spree::Promotion).to receive(:eligible?).and_return(true) } |
There was a problem hiding this comment.
You don't need this because the default promotion has no rules and is eligible, right?
| end | ||
|
|
||
| context 'for ineligible promotion' do | ||
| before { allow_any_instance_of(Spree::Promotion).to receive(:eligible?).and_return(false) } |
There was a problem hiding this comment.
Rather than stubbing would you mind instead creating a promotion that will in fact not be eligible? e.g.
let(:promotion) { create(:promotion, :with_item_total_rule, item_total_threshold_amount: 1_000, ... }
fylooi
force-pushed
the
validate-free-shipping
branch
from
0e74801 to
6a2d014
Compare
|
Thanks for the review. Have applied the requested changes, merged your PR and rebased master. |
|
|
||
| context 'for ineligible promotion' do | ||
| let(:promotion) do | ||
| create(:promotion, :with_item_total_rule, item_total_threshold_amount: 1_000, code: 'freeshipping', promotion_actions: [action]) |
There was a problem hiding this comment.
You want to set apply_automatically: true in this context rather than code: 'freeshipping', right?
There was a problem hiding this comment.
My bad, copied that as I wasn't familiar with the factories. Also needs to be a let!.
fylooi
force-pushed
the
validate-free-shipping
branch
from
6a2d014 to
2c34832
Compare
mamhoff
dismissed
jordan-brough’s stale review
The author addressed all change requests.
|
We hope this fixes the issue, we're reasonably sure it got introduced in #1392 and this seems to fix it. |
This PR is an alternate fix for issue #1999 since the original one seems to have stalled.
Spree::PromotionHandler::FreeShippingcurrently does not check whether the promotion is eligible before activating it. This is an issue for automatic promotions as any associated promotion rules (eg. minimum order total) are ignored.No problem for non-automatic promotions as they are validated upon activation by other handlers.