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

Add ability to remove promotions via Promotion#remove_from #1451

Merged
Merged
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
14 changes: 14 additions & 0 deletions core/app/models/spree/promotion.rb
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,20 @@ def used_by?(user, excluded_orders = [])
end
end

# 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 [void]
def remove_from(order)
actions.each do |action|
action.remove_from(order)
end
# note: this destroys the join table entry, not the promotion itself
order.promotions.destroy(self)
order.order_promotions.reset
order_promotions.reset
end

private

def blacklisted?(promotable)
Expand Down
11 changes: 11 additions & 0 deletions core/app/models/spree/promotion/actions/create_adjustment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@ 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Thanks.

# @return [void]
def remove_from(order)
order.adjustments.each do |adjustment|
if adjustment.source == self
order.adjustments.destroy(adjustment)
end
end
end

private

# Tells us if there if the specified promotion is already associated with the line item
Expand Down
14 changes: 14 additions & 0 deletions core/app/models/spree/promotion/actions/create_item_adjustments.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,20 @@ def compute_amount(adjustable)
[adjustable.amount, promotion_amount].min * -1
end

# Removes any adjustments generated by this action from the order's
# line items.
# @param order [Spree::Order] the order to remove the action from.
# @return [void]
def remove_from(order)
order.line_items.each do |line_item|
line_item.adjustments.each do |adjustment|
if adjustment.source == self
line_item.adjustments.destroy(adjustment)
end
end
end
end

private

def create_adjustment(adjustable, order, promotion_code)
Expand Down
14 changes: 14 additions & 0 deletions core/app/models/spree/promotion/actions/free_shipping.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,20 @@ def compute_amount(shipment)
shipment.cost * -1
end

# Removes any adjustments generated by this action from the order's
# shipments.
# @param order [Spree::Order] the order to remove the action from.
# @return [void]
def remove_from(order)
order.shipments.each do |shipment|
shipment.adjustments.each do |adjustment|
if adjustment.source == self
shipment.adjustments.destroy(adjustment)
end
end
end
end

private

def promotion_credit_exists?(shipment)
Expand Down
17 changes: 17 additions & 0 deletions core/app/models/spree/promotion_action.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,22 @@ class PromotionAction < Spree::Base
def perform(_options = {})
raise 'perform should be implemented in a sub-class of PromotionAction'
end

# Removes the action from an order
#
# @note This method should be overriden in subclassses.
#
# @param order [Spree::Order] the order to remove the action from
# @return [void]
def remove_from(order)
Spree::Deprecation.warn("#{self.class.name.inspect} does not define #remove_from. The default behavior may be incorrect and will be removed in a future version of Solidus.", caller)
[order, *order.line_items, *order.shipments].each do |item|
item.adjustments.each do |adjustment|
if adjustment.source == self
item.adjustments.destroy(adjustment)
end
end
end
end
end
end
20 changes: 20 additions & 0 deletions core/spec/models/spree/promotion/actions/create_adjustment_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,26 @@
end
end

describe '#remove_from' do
let(:action) { promotion.actions.first! }
let(:promotion) { create(:promotion, :with_order_adjustment) }

let!(:unrelated_adjustment) { create(:adjustment, order: order, source: nil) }

before do
action.perform(payload)
@action_adjustment = order.adjustments.where(source: action).first!
end

it 'removes the action adjustment' do
expect(order.adjustments).to match_array([unrelated_adjustment, @action_adjustment])

action.remove_from(order)

expect(order.adjustments).to eq([unrelated_adjustment])
end
end

context "#destroy" do
before(:each) do
action.calculator = Spree::Calculator::FlatRate.new(preferred_amount: 10)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ module Spree
class Promotion
module Actions
describe CreateItemAdjustments, type: :model do
let(:order) { create(:order) }
let(:order) { create(:order_with_line_items, line_items_count: 1) }
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 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The to_a shouldn't be needed

Copy link
Contributor Author

@jordan-brough jordan-brough Sep 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also prefer keeping the to_a.first

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

let(:payload) { { order: order, promotion: promotion } }

before do
Expand Down Expand Up @@ -122,32 +122,56 @@ module Actions
end
end

describe '#remove_from' do
# this adjustment should not get removed
let!(:other_adjustment) { create(:adjustment, adjustable: line_item, order: order, source: nil) }

before do
action.perform(payload)
@action_adjustment = line_item.adjustments.where(source: action).first!
end

it 'removes the action adjustment' do
expect(line_item.adjustments).to match_array([other_adjustment, @action_adjustment])

action.remove_from(order)

expect(line_item.adjustments).to eq([other_adjustment])
end
end

context "#destroy" do
let!(:action) { promotion.actions.first }
let(:other_action) { other_promotion.actions.first }
let(:promotion) { create(:promotion, :with_line_item_adjustment) }
let(:other_promotion) { create(:promotion, :with_line_item_adjustment) }

it "destroys adjustments for incompleted orders" do
order = Order.create
action.adjustments.create!(label: "Check", amount: 0, order: order, adjustable: order)
context 'with incomplete orders' do
let(:order) { create(:order) }

expect {
action.destroy
}.to change { Adjustment.count }.by(-1)
it 'destroys adjustments' do
order.adjustments.create!(label: 'Check', amount: 0, order: order, source: action)

expect {
action.destroy
}.to change { Adjustment.count }.by(-1)
end
end

it "nullifies adjustments for completed orders" do
order = Order.create(completed_at: Time.current)
adjustment = action.adjustments.create!(label: "Check", amount: 0, order: order, adjustable: order)
context 'with complete orders' do
let(:order) { create(:completed_order_with_totals) }

expect {
action.destroy
}.to change { adjustment.reload.source_id }.from(action.id).to nil
it 'nullifies adjustments for completed orders' do
adjustment = order.adjustments.create!(label: 'Check', amount: 0, order: order, source: action)

expect {
action.destroy
}.to change { adjustment.reload.source_id }.from(action.id).to nil
end
end

it "doesnt mess with unrelated adjustments" do
other_action.adjustments.create!(label: "Check", amount: 0, order: order, adjustable: order)
order.adjustments.create!(label: "Check", amount: 0, order: order, source: action)

expect {
action.destroy
Expand Down
25 changes: 22 additions & 3 deletions core/spec/models/spree/promotion/actions/free_shipping_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@

describe Spree::Promotion::Actions::FreeShipping, type: :model do
let(:order) { create(:completed_order_with_totals) }
let(:promotion_code) { create(:promotion_code, value: 'somecode') }
let(:promotion) { promotion_code.promotion }
let(:action) { Spree::Promotion::Actions::FreeShipping.create }
let(:shipment) { order.shipments.to_a.first }
let(:promotion) { create(:promotion, code: 'somecode', promotion_actions: [action]) }
let(:action) { Spree::Promotion::Actions::FreeShipping.new }
let(:payload) { { order: order, promotion_code: promotion_code } }
let(:promotion_code) { promotion.codes.first! }

# From promotion spec:
context "#perform" do
Expand Down Expand Up @@ -37,4 +38,22 @@
end
end
end

describe '#remove_from' do
# this adjustment should not get removed
let!(:other_adjustment) { create(:adjustment, adjustable: shipment, order: order, source: nil) }

before do
action.perform(payload)
@action_adjustment = shipment.adjustments.where(source: action).first!
end

it 'removes the action adjustment' do
expect(shipment.adjustments).to match_array([other_adjustment, @action_adjustment])

action.remove_from(order)

expect(shipment.adjustments).to eq([other_adjustment])
end
end
end
38 changes: 38 additions & 0 deletions core/spec/models/spree/promotion_action_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
require 'spec_helper'

describe Spree::PromotionAction, type: :model do
describe '#remove_from' do
class MyPromotionAction < Spree::PromotionAction
def perform(options = {})
order = options[:order]
order.adjustments.create!(amount: 1, order: order, source: self, label: 'foo')
true
end
end

let(:action) { promotion.actions.first! }
let!(:promotion) { create(:promotion, promotion_actions: [MyPromotionAction.new]) }
let(:order) { create(:order) }

# this adjustment should not get removed
let!(:other_adjustment) { create(:adjustment, order: order, source: nil) }

before do
action.perform(order: order)
@action_adjustment = order.adjustments.where(source: action).first!
end

it 'removes the action adjustment' do
expect(order.adjustments).to match_array([other_adjustment, @action_adjustment])

expect(Spree::Deprecation).to(
receive(:warn).
with(/"MyPromotionAction" does not define #remove_from/, anything)
)

action.remove_from(order)

expect(order.adjustments).to eq([other_adjustment])
end
end
end
19 changes: 19 additions & 0 deletions core/spec/models/spree/promotion_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,25 @@
end
end

describe '#remove_from' do
let(:promotion) { create(:promotion, :with_line_item_adjustment) }
let(:order) { create(:order_with_line_items) }

before do
promotion.activate(order: order)
end

it 'removes the promotion' do
expect(order.promotions).to include(promotion)
expect(order.line_items.flat_map(&:adjustments)).to be_present

promotion.remove_from(order)

expect(order.promotions).to be_empty
expect(order.line_items.flat_map(&:adjustments)).to be_empty
end
end

describe "#usage_limit_exceeded?" do
subject { promotion.usage_limit_exceeded? }

Expand Down