Simplify Coupon PromotionHandler #521
Conversation
|
As discussed IRL hawth is going to throw some test cases about what didn't work before and how the new behaviour should be. |
|
jhawthorn
force-pushed
the
coupon_code_handler
branch
from
b7cd745
to
7baa41c
Compare
|
@cbrunsdon Added a test for the previously broken behaviour. It fails on versions before 02ed14f (because successful? is false, despite applying the coupon) and passes now. |
|
To be clear: there were no tests testing the :coupon_code_not_found and :coupon_code_better_exists logic that was previously being returned and we are changing that behaviour. Yes, those happen under pretty dubious circumstances, but that is the existing behaviour. Does anyone want to chip in on if they rely on those being returned or not? Maybe @alepore or @tvdeyen? |
|
@jhawthorn do we need to revisit this one? |
Use any? on an array of results instead of or-ing on each iteration.
jhawthorn
force-pushed
the
coupon_code_handler
branch
from
f033ebf
to
7deb986
Compare
core/app/models/spree/promotion.rb
Outdated
| @@ -237,8 +237,7 @@ def blacklisted?(promotable) | |||
| when Spree::LineItem | |||
| !promotable.product.promotionable? | |||
| when Spree::Order | |||
| promotable.line_items.any? && | |||
| promotable.line_items.joins(:product).where(spree_products: { promotionable: false }).any? | |||
| promotable.line_items.joins(:product).where(spree_products: {promotionable: false}).exists? | |||
There was a problem hiding this comment.
Space inside { missing.
Space inside } missing.
This was running one query to check if there were any line items and a second to check if any were from unpromotable products. The first check is redundant.
Rather than refering to it as order.coupon_code.downcase throughout. This could later be refactored to it being passed into the class rather than stored as as non-volatile on the order.
Removed determine_promotion_application_result. This code previously looked through all of the adjustments on the order in an attempt to determine if this promotion was in effect, or if a previously applied promotion was taking precedence. I've removed this logic for a few reasons: * It could return false negatives. It only considered the first matching adjustment, so it would return an error even if that adjustment was ineligible even if a later one was in use. * It doesn't make sense to return an error here because adjustments and an OrderPromotion have been created. Even if they are now ineligible they could become eligible later. * This ties coupon application to actions which create adjustments. Other actions (like the defunct CreateLineItems) would either have to be added here. * Our test suites did not cover either of these failure cases.
jhawthorn
force-pushed
the
coupon_code_handler
branch
from
7deb986
to
2c2115d
Compare
The controversial commit should be the last one where I remove some functionality. Hopefully I explained my reasoning well in the commit message.