[Spree Upgrade] Subs - Extend Order cleanly for subscriptions

[mkllnk]

What? Why?

Part of #2520. It unblocks #2771.

Orders belonging to subscriptions get completed without payment. That
requires overriding Spree's functionality. Here we move this
customisation into its own file and override a different method to make
it less invasive and compatible with Spree 2.

In Spree 2, an order in payment state without pending payments is invalid.
Instead we skip the payment state by not requiring a payment for
automatically generated orders until the order cycle is closed.

This is the change that comes with Spree 2:

diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb
index fb5cea69b..25c91568a 100644
--- a/app/models/spree/order_decorator.rb
+++ b/app/models/spree/order_decorator.rb
@@ -336,6 +336,9 @@ Spree::Order.class_eval do
   # See commit: https://github.com/spree/spree/commit/5fca58f658273451193d5711081d018c317814ed
   # Allows GatewayError to show useful error messages in checkout
     def process_payments!
+      if pending_payments.empty?
+        raise Core::GatewayError.new Spree.t(:no_pending_payments)
+      else
         pending_payments.each do |payment|
           break if payment_total >= total
 
@@ -345,6 +348,7 @@ Spree::Order.class_eval do
             self.payment_total += payment.amount
           end
         end
+      end
     rescue Spree::Core::GatewayError => e # This section changed
       result = !!Spree::Config[:allow_checkout_on_gateway_error]
       errors.add(:base, e.message) and return result

What should we test?

• Checkout with several payment methods.
• Create a subscription.
• Let the order cycle close by passing the close-by date.
• Make sure the payment is triggered and a confirmation is sent.

Release notes

Changed: Payment processing logic has been prepared for future software updates.

Changelog Category: Changed

How is this related to the Spree upgrade?

It makes our code more compatible with Spree 2.

[mkllnk]

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?

[luisramos0]

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.

[sauloperez]

I am sure @sauloperez will be thinking about spree hooks

I'm too foreseeable . 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?

[luisramos0]

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.

[luisramos0]

re "Orders belonging to subscriptions get completed without payment."
This is an exception and creates complexity. Can we avoid it?
Can we say subscription orders must have payments? We can inject a pending payment in the subscriptions orders based on info on the subscription.
I think this would be much better. We have to make an effort to avoid creating these exceptions in the system, they add up and make the system complex.

[luisramos0]

Interesting challenge we have here.

[luisramos0]

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.

[Matt-Yorkley]

Injecting a new "pending" payment object is basically what happens on normal orders, right? It's not a payment as such, more like a placeholder that says "this can be paid in the future". It seems like a good idea to follow the same process. Or is the issue that the payment method hasn't been chosen at that point, so a pending payment can't be created?

[mkllnk]

@luisramos0 @Matt-Yorkley Thank you for your great feedback. It makes me look at the bigger picture again.

The Spree model

If payment is not required:

• The checkout controller advances the order with order.next.
• The order goes from state delivery to complete.
• Done.

If payments are required:

• The user chooses a payment method.
• A payment is created and has the initial state checkout.
• All payments in state checkout are listed with order.pending_payments.
• The checkout controller advances the order with order.next.
• order.process_payments! is triggered before going to the complete state.
• The order calls payment.process! on each of the payments.
• If the payment succeeds, the totals are updated and the order is complete.
• Done.

The special need of subscriptions

Subscriptions place an order and advance it to complete without triggering a payment. Payments are processed later in a delayed job. In the mean time, the user can call up and request to change the order before it has been paid. The card is charged only once.

This model aligns with conventional veggie box deliveries on a weekly or fortnightly basis. Customers don't want to get charged without confirmation. If they don't change the order after they got notified when the order cycle opened, we see that as consent. It avoids double charging and therefore transaction fees. Most customers also prefer being charged later than earlier.

But I'm also realising now that this looks very different from a CSA perspective. People pay the farmer early and get what the farmer can provide. Changes may be possible through additional orders with additional charges, cancelling orders, refunding or dealing with account credit. But discussing this logic needs to go through a broader inception phase.

Possible implementations

1. The current implementation overrides order.pending_payments to return an empty array for subscriptions before the end of an order cycle. That does the job, but blows up in Spree 2 if we remove our override of process_payments!. If we leave that override there, it will still work as before.

2. My implementation allows to remove our override of process_payments!. Instead of overriding pending_payments it overrides payment_required? so that the whole payment step is skipped. This avoids a lot of complication and seems the least invasive code to me.

3. We could also override process_payments! to explicitly deal with subscriptions. We can skip the processing for subscriptions there and call the original code otherwise. It's very similar to my implementation, just hooking in at a different method. Is that what you meant @sauloperez?

4. We could somehow make the payment go in the pending state. Spree uses that for a two step payment. First a payment is authorised and goes to pending. Then the payment is actually processed and it goes to complete later. I haven't found the code for the second step yet. The problem is that order.pending_payments doesn't list those payments in state pending and it would blow up in Spree 2 again. This sounds like what @Matt-Yorkley suggested, but the pending state doesn't seem to solve the problem. It looks like we need to hook in somewhere else as well. Do you have a good idea for that @Matt-Yorkley?

5. When payment.process! is called, it basically calls gateway.purchase(amount). We could override something in the payment model to skip processing for subscriptions. But it seems to become more hidden the further we go up the call stack.

What do you think now? Do you have a preference or other good ideas?

[luisramos0]

Great analysis @mkllnk we should move this to a wiki page :-)

After reading your analysis, I think we cannot change OFN core orders workflow just because of subscriptions. IMO there should be no reference to subscriptions on ofn orders code.
If we need to change order workflow or payment workflow because of subscriptions, we need to do it because it makes sense in the broader picture and as a global change to orders, not as an exception for subscriptions.
Basically, there should be no "if subscriptions" code in ofn core code but instead, if really needed, we should implement configurable endpoints that subscriptions code can use.
Another way of putting it, we have to make an effort to avoid using the word override :-)
So, I think we should explore the only implementation option that doesn't contain the word override, which is 4.
Regarding this option 4, in subscriptions, if the payment is not complete, the order is not complete. Can we make subscriptions work with incomplete orders? Do these orders need to be in the complete state? Why?
(I am sorry for asking so many questions but I believe this is very important)

[luisramos0]

This discussion and solution may take us some time. I think we should focus on core spree upgrade and leave subscriptions for later. Maybe just move this into #2953
There are only 154 broken tests in build!!! Lets do it :-)

[Matt-Yorkley]

I was thinking of cash payments for example, that can be present on a completed order even though the payment hasn't been made yet. The enterprise user can click the "capture" button once it's been paid to trigger the payment state changing.

[sauloperez]

Another way of putting it, we have to make an effort to avoid using the word override :-)

You literally read my mind! I think we have to make the effort to stop using this word. Let's talk about OOP instead. I don't know the inner details of subscriptions but in my mind I see a PaymentWithParticularImplementation object that sticks to the Spree::Payment interface but does what we want #polymorphism.

[luisramos0]

@sauloperez yes, that is that possibility

• AND there is the possibility of making subscriptions use a new payment thing that is implemented in standard payment process, but it's not called "subscriptions something"
• AND even better, there is the possibility of making subscriptions work with the current order and payment process, with no exceptional logic.

[mkllnk]

@luisramos0 asked:

Can we make subscriptions work with incomplete orders? Do these orders need to be in the complete state? Why?

The actual requirements were the following use case:

• A subscription is created for a series of order cycles.
• At the beginning of an order cycle (when it opens) orders are placed for all subscriptions.
• Users receive order confirmations (which are triggered by completing orders).
• Users can still modify the order before the end of the order cycle.
• Users get charged when the order cycle closes.

Rob did think a lot about how to achieve this. We could have created incomplete orders at the beginning of the order cycle. But incomplete orders don't reserve stock levels. So if we want to model that an order is actually placed and the stock is reserved for the user, a complete order seems reasonable.

Postponing this issue

I will move this PR and #2771 (which is blocked by this) to the non-core epic #2953. A proper solution seems a bit more complicated and I guess we can leave the current override of pending_payments. If that override becomes a problem, we need to revisit this.

A better solution

I totally agree with avoiding overrides. Yes for good object oriented design. I'm not sure polymorphism is the right word for the solution here.

I'm assuming that we still want a completed order and delay the payment. Spree processes payments in payment methods. The cash payment method allows to check out without paying. Rob always saw Stripe as requirement for subscriptions, because it allows capturing the money automatically. But maybe we need a subscription payment method that decorates another payment method. It can contain the logic of delaying the payment while the order cycle is open. But once it's closed, it just calls the associated payment method. This type of composition could avoid overrides and separate the code better. But it may well be that there are some unknown obstacles that make this really difficult. Worth a try after the upgrade?

[luisramos0]

i like your line of thought Maikel! i wonder how the "cash payment method allows to check out without paying" is implemented...

[sauloperez]

I like it too

[mkllnk]

@luisramos0 Have a look at:

https://github.com/openfoodfoundation/spree/blob/41906362d931695e0616194341a68d2c4c85aaaf/core/app/models/spree/payment_method/check.rb#L17

It basically has a capture method returning true without doing anything.

[mkllnk]

@luisramos0 We deferred this to phase 2. Now we are in phase 2 and I just had a look at it again. What do you think we should do with these pull requests now?

The current implementation seems to work. In this pull request and #2771 I thought I could improve our way of overriding and make it more compliant with Spree 2. You and Pau pointed out that it's still overriding core logic for subscriptions and that it's not ideal. I agree. But I also don't see a reasonable way of implementing our ideal solution. I see three ways forward:

1 - We close these pull requests and leave everything as it is. It works. The comment about obsolete code in the decorator code may be confusing though.

openfoodnetwork/app/models/spree/order_decorator.rb

Lines 327 to 344 in 32846af

	# Override of existing Spree method. Can remove when we reach 2-0-stable
	# See commit: https://github.com/spree/spree/commit/5fca58f658273451193d5711081d018c317814ed
	# Allows GatewayError to show useful error messages in checkout
	def process_payments!
	pending_payments.each do |payment|
	break if payment_total >= total
	payment.process!
	if payment.completed?
	self.payment_total += payment.amount
	end
	end
	rescue Spree::Core::GatewayError => e # This section changed
	result = !!Spree::Config[:allow_checkout_on_gateway_error]
	errors.add(:base, e.message) and return result
	end

2 - I incorporate your feedback about not creating a new extension and we go with this pull request. It's not ideal but I think that it's better than what we have at the moment. We see the separation of core code and subscriptions as separate issue out of scope.
3 - We invest more time to try and separate subscriptions and core logic. I suspect that it may not be possible with current Spree code and that we either need to override something or start defining our own models because subscriptions require something that is restricted in the Spree world.

[luisramos0]

Given where we are with the upgrade and looking at this code again, I'd go for option 2.

I dont mind the extension but I'd put those 2 short methods in the order_decorator itself. But if you prefer to keep the extension it's fine (I'd call it OrderSubscriptionsPaymentsExtension).

So, if you rename the extension (or better imo, if you move the methods to order_decorator), I am ok with going ahead with this PR. It's not perfect but definitely better than what it was.

Otherwise, I tend more to option 1 than option 3.

[mkllnk]

Okay, I had another go at this. I realised several things:

1. Moving the methods into the override means that I can't use super. Since the implementation of payment_required? changes between Spree 1 and Spree 2, I thought its better to apply this change to the v2 branch instead of master.
2. Using v2 I came across merge conflicts and needed to update my spec. So it's better to do that now instead of when merging later. Otherwise you would have cursed me during the next merge. 😉
3. Using v2 also allows me to include the removal of process_payments! which was the original purpose of the endeavour.

@luisramos0 Have a look at https://github.com/openfoodfoundation/openfoodnetwork/compare/2-0-stable...mkllnk:2520-spree2-order-subscription?expand=1. Should I open a new pull request and close this one and #2771?

I think that an extension would have been cleaner but I went with your preference of using the decorator now. We will see what the next Spree upgrade requires. 😉

[luisramos0]

yeah, that's it. awesome! yes, it's not so clean to have them in the order_decorator, but it makes it more obvious we need to take this out of here soon :-)