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

[Spree Upgrade] Subs - Extend Order cleanly for subscriptions #3012

Closed
wants to merge 1 commit into from
Closed
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 app/models/extensions/order_subscriptions_extensions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Used to prevent payments on subscriptions from being processed in the normal way.
# Payments are skipped until after the order cycle has closed.
module OrderSubscriptionsExtensions
Copy link
Member Author

Choose a reason for hiding this comment

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

Dear reviewers,

This file introduces a new structure. I simply didn't know where to put this file. It is not a Concern, but an Extension. It is very tightly coupled to Spree::Order and Subscription logic. So it doesn't fit into lib. It's not a service and it's not a model on its own. Is this the right place?

Copy link
Contributor

Choose a reason for hiding this comment

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

The name OrderSubscriptionsExtensions implies we can add more to this module. Modules and classes should have single responsibility and as such I'd name the module to what it is doing here and nothing else: OrderSubscriptionsPaymentRequired or OrderSubscriptionsProcessPayments.

re /extensions, I'd do an effort not to create another type of file in the codebase, it adds complexity that we should avoid.
I dont think it would be perfect, but I think it'd be better than this extension: a concern with a alias_method_chain on payment_required? or better, process_payments.

I am sure @sauloperez will be thinking about spree hooks. The right solution for this would be to patch spree code to add a "before_process_payments" hook and then make this change in ofn code reimplementing the hook.

Personally, I'd go for the alias_method_chain on process_payments inside a concern.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am sure @sauloperez will be thinking about spree hooks

I'm too foreseeable :trollface: . That is exactly what I though right after reading the description. I tend to dislike alias_method_chain for what I shared in #3009 (comment).

The right solution for this would be to patch spree code to add a "before_process_payments" hook and then make this change in ofn code reimplementing the hook.

Then, why not this? or any other extension hook we think best fits here?

Copy link
Contributor

Choose a reason for hiding this comment

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

With "The right solution" I meant "the solution that @sauloperez thinks is right" :-)
I think we have to explore option 4 below. Try to remove subscriptions code from order.

# Override Spree method.
def payment_required?
super && !skip_payment_for_subscription?
end

private

def skip_payment_for_subscription?
subscription.present? && order_cycle.orders_close_at.andand > Time.zone.now
end
end
10 changes: 3 additions & 7 deletions app/models/spree/order_decorator.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require 'extensions/order_subscriptions_extensions'
require 'open_food_network/enterprise_fee_calculator'
require 'open_food_network/distribution_change_validator'
require 'open_food_network/feature_toggle'
Expand All @@ -8,6 +9,8 @@
end

Spree::Order.class_eval do
prepend OrderSubscriptionsExtensions

belongs_to :order_cycle
belongs_to :distributor, class_name: 'Enterprise'
belongs_to :customer
Expand Down Expand Up @@ -347,13 +350,6 @@ def process_payments!
errors.add(:base, e.message) and return result
end

# Override or Spree method. Used to prevent payments on subscriptions from being processed in the normal way.
# ie. they are 'hidden' from processing logic until after the order cycle has closed.
def pending_payments
return [] if subscription.present? && order_cycle.orders_close_at.andand > Time.zone.now
payments.select {|p| p.state == "checkout"} # Original definition
end

private

def shipping_address_from_distributor
Expand Down
1 change: 1 addition & 0 deletions spec/jobs/subscription_confirm_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@
before do
allow(order).to receive(:payment_total) { 0 }
allow(order).to receive(:total) { 10 }
allow(order).to receive(:payment_required?) { true }
allow(order).to receive(:pending_payments) { [payment] }
end

Expand Down
45 changes: 36 additions & 9 deletions spec/models/spree/order_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -818,13 +818,22 @@
end
end

describe "finding pending_payments" do
let!(:order) { create(:order ) }
let!(:payment) { create(:payment, order: order, state: 'checkout') }
describe "payments" do
let(:payment_method) { create(:payment_method) }
let(:shipping_method) { create(:shipping_method) }
let(:order) { create(:order_with_totals) }

context "when the order is not a subscription" do
it "returns the payments on the order" do
expect(order.reload.pending_payments).to eq [payment]
it "it requires a payment" do
expect(order.payment_required?).to be true
end

it "advances to payment state" do
advance_to_delivery_state(order)

order.next!

expect(order.state).to eq "payment"
end
end

Expand All @@ -835,27 +844,45 @@
context "and order_cycle has no order_close_at set" do
before { order.order_cycle.update_attributes(orders_close_at: nil) }

it "returns the payments on the order" do
expect(order.reload.pending_payments).to eq [payment]
it "requires a payment" do
expect(order.payment_required?).to be true
end
end

context "and the order_cycle has closed" do
before { order.order_cycle.update_attributes(orders_close_at: 5.minutes.ago) }

it "returns the payments on the order" do
expect(order.reload.pending_payments).to eq [payment]
expect(order.payment_required?).to be true
end
end

context "and the order_cycle has not yet closed" do
before { order.order_cycle.update_attributes(orders_close_at: 5.minutes.from_now) }

it "returns an empty array" do
expect(order.reload.pending_payments).to eq []
expect(order.payment_required?).to be false
end

it "skips the payment state" do
advance_to_delivery_state(order)

order.next!

expect(order.state).to eq "complete"
end
end
end

def advance_to_delivery_state(order)
# advance to address state
order.shipping_method = shipping_method
order.next!

# advance to delivery state
create(:payment, order: order, payment_method: payment_method)
order.next!
end
end

describe '#restart_checkout!' do
Expand Down