Add ability to remove promotions via Promotion#remove_from #1451
Conversation
To use in-memory items more.
jordan-brough
force-pushed
the
add-promotion-remove-from
branch
from
ad06809 to
0be285f
Compare
| # Removes a promotion and any adjustments or other side effects from an | ||
| # order. | ||
| # @param order [Spree::Order] the order to remove the promotion from. | ||
| # @return [undefined] |
There was a problem hiding this comment.
If you don't care about the return value @return [void] should be used.
There was a problem hiding this comment.
@gmacdougall thanks! Updated and opened a PR to remove more of these: #1454
| @@ -39,6 +39,16 @@ def compute_amount(calculable) | |||
| [(calculable.item_total + calculable.ship_total), amount].min * -1 | |||
| end | |||
|
|
|||
| # Removes any adjustments generated by this action from the order. | |||
| # @param order [Spree::Order] the order to remove the action from. | |||
There was a problem hiding this comment.
Updated. Thanks.
| let(:promotion) { create(:promotion, :with_line_item_adjustment, adjustment_rate: adjustment_amount) } | ||
| let(:adjustment_amount) { 10 } | ||
| let(:action) { promotion.actions.first! } | ||
| let!(:line_item) { create(:line_item, order: order) } | ||
| let(:line_item) { order.line_items.to_a.first } |
There was a problem hiding this comment.
The to_a shouldn't be needed
There was a problem hiding this comment.
@gmacdougall this ensures that line_item refers to the same object that OrderUpdater and the promo code & etc will modify. i.e. order.line_items.first.object_id == order.line_items.to_a.first.object_id is not always true in general.
Our current implementation of the order factory happens to preload order.line_items such that the to_a isn't needed here, but that's not tested and I'm not sure it was intentional either. Perhaps I could remove it here but make an additional PR that tests this explicitly in order_factory_spec?
There was a problem hiding this comment.
I'm also prefer keeping the to_a.first
| let(:action) { promotion.actions.first! } | ||
| let(:promotion) { create(:promotion, :with_order_adjustment) } | ||
|
|
||
| # this adjustment should not get removed |
There was a problem hiding this comment.
Could we name the other_adjustment something that implies this more directly?
There was a problem hiding this comment.
@gmacdougall sure, I've updated it to unrelated_adjustment.
Make it more readable.
Also: Add a CHANGELOG entry
|
Currently I can see the |
Extracted from #1433 as per a chat with @jhawthorn.
This will allow promotions to be removed from orders and allows promotion actions to define how to reverse their side effects on an order.
For now this provides a default
remove_fromon thePromotionActionbase class, with a deprecation warning that subclasses should define their ownremove_frommethod.