- 🇺🇸United States rszrama
+1'ing this issue, as we've just encountered it. The scenario is exactly the same: we're using a prepaid billing schedule and are seeing draft recurring orders that appear to be for the previous billing period, the one that was already prepaid. That said, I'm not sure I fully understand the proposed solution or latest patch.
The original post states that "the first recurring order should be treated as the trial order and its price should be set to 0." This is either imprecise or just not correct for all scenarios. In our case, we still charge the customer the current month's prorated fee upon checkout, so it would not be appropriate for either the initial order or the first recurring order to be set to $0.
Additionally, the patch separates the single
Cron::enqueueOrders()
function into separate functions for prepaid vs. postpaid billing schedules. I don't see why this is necessary. Can we not just use a single query that uses condition groups to find those prepaid orders whose start date has passed and postpaid orders whose end date has passed? This will reduce code duplication, given we have to perform the same checks on the loaded orders for cancellation regardless.At the end of the day, what we need to see are:
- Subscriptions with prepaid billing schedules: my draft recurring order should be for the billing period that ends after my current one. Thus, if I subscribe today with a fixed monthly billing schedule that renews on the 1st of the month, my draft recurring order should show that it is for August 1 - September 1.
- Subscriptions with postpaid billing schedules: my draft recurring order should be for the current billing period. Thus, if I subscribe today with a fixed monthly billing schedule that renews on the 1st of the month, my draft recurring order should show that it is for July 1 - August 1.
There are some nuances we need to explore with respect to trial periods, but the gist of the fix for the initial order placement is that prepaid billing schedules should result in the next billing period being generated after the first is. (In the patch, I'm not sure I follow the logic of the change for this in
RecurringOrderManager::startRecurring()
.)I'm also not sure what we should do for the billing period with to fixed prepaid schedules and free trials, as the adjusted start date code is a little funky ... but maybe the answer is that free trials in these cases just aren't super appropriate? If a subscription always renews on the 1st of the month, what's the use case for a free trial vs. a 100% off coupon for the first month or something? 🧐
- 🇮🇱Israel jsacksick
@rszrama: There is currently nothing we could query on to simply adapt the query. There is a billing period field on the order, and that just holds the start and the end date, nothing else.
The subscription references the billing schedule entity, and the billing type (prepaid vs postpaid) is stored on that config entity, so unfortunately, nothing we can query on.
I believe this is why the current logic works as is... We always close recurring orders at the end of the billing period... I wonder if we shouldn't find a way to simply "cheat" and just display a different billing period. This is far from a perfect solution, but it'd solve the display problem I suppose...
- 🇮🇱Israel jsacksick
So actually revising what I said in #13.
What we could do is a following, loop on all billing schedules, and separate prepaid from postpaid billing schedules. We essentially need an array containing prepaid billing schedule IDS and another one containing postpaid billing schedule IDS.
Then, assuming we fix the initial billing period, we could adjust the query to do the following:
- Create a first "AND" condition group to have a condition on the both the end date and a condition to get orders referencing postpaid billing schedules.
- Create a second AND condition group to have a condition on the start date AND a reference to a "prepaid" billing schedule.
- Then an OR condition group to get either recurring orders with a billing period end date in the past referencing postpaid billing schedules OR recurring orders with a billing period start date in the past AND referencing a prepaid billing schedule.
But what about existing orders? This would be a breaking change as we wouldn't bill existing orders properly unfortunately.
- 🇮🇱Israel jsacksick
I think we need an update hook updating all opened recurring draft orders referencing prepaid billing schedules, to ensure their billing periods is adjusted. I'm just hoping that doing this isn't going to cause a billing cycle to be skipped or something.
- 🇳🇿New Zealand john pitcairn
This makes me very nervous, and not just about the possibility of skipping a cycle. All our orders are prepaid.
If we only update draft order billing periods, pushing them out by 1 cycle, then we will have a "missing" billing period in the subscription. At some point admin will spot that and freak out. The customer may think they've "skipped" a billing period and request support. And it may screw up any reporting or other operations that use the billing period.
I think we'd need to update all prepaid orders, no? Ouch.
- 🇳🇿New Zealand john pitcairn
Can't help but thinking we have a naming problem here, ie we call it the "billing period", but it's always used as if it were post-paid – so it is not the same as the subscription period for pre-paid subscriptions.
- 🇮🇱Israel jsacksick
We use billing period because that is how it's called in the code everywhere...
We may have to update all prepaid orders yes, I understand this is making you nervous, however this has to be fixed as the current situation is really confusing and is far from ideal... - 🇳🇿New Zealand john pitcairn
Sure. Was just wondering about keeping it as is and adding a calculated
subscription_period
field, using that for display, email etc. So the current assumptions and data would not change. - 🇺🇸United States rszrama
I do think there's some small risk here that subscriptions payments could be impacted, but I'm struggling to conceive the hypothetical scenario.
For example, let's say the "close order" queue was populated and then the module gets updated before all queued items can be processed. The update then updates all those draft recurring orders to use the new billing period... well, there's no double checking in the queue processor to see if it's really the right time to close the order. They'd close like normal.
Additionally, let's say the order got updated right around the time it was supposed to be enqueued for closing. (Or let's say the queue processor actually did double check to make sure it's the right time for an order to close.) Well, the coordinated change to the logic regarding when a prepaid subscription's draft recurring order should close (switching from the start to the end of the billing period) will be in place. No harm, no foul.
I think the concerns (closing previously enqueued orders vs. updating draft orders) are separate enough that there's little risk. Maybe we just need to make sure we leave non-draft recurring orders for prepaid subscriptions alone ... that will create some apparent billing duplication, but that's easily explained if it becomes a customer service issue.
Can anyone else think of a scenario where a payment may actually be double charged or skipped? Maybe we just need to ensure the enqueueing of an order for closure can't happen twice? (But even then, if closure was attempted a second time, the default implementation in
RecurringOrderManager
will skip the order if it is completed or paid.) - 🇳🇿New Zealand john pitcairn
OK fair enough. Update drafts only.
I guess we can run our own update to fix past completed/failed orders after all orders in
needs_payment
at initial update time have either completed or failed. So we have consistent data. - 🇳🇴Norway zaporylie
Can anyone else think of a scenario where a payment may actually be double charged or skipped? Maybe we just need to ensure the enqueueing of an order for closure can't happen twice? (But even then, if closure was attempted a second time, the default implementation in RecurringOrderManager will skip the order if it is completed or paid.)
I think there's a risk in the billing period being updated after the order renewal job is triggered but before the order closing job commences. It can happen as advancedqueue doesn't guarantee in which order jobs are executed. In such a scenario, the new draft will cover the same dates as the order destined to be closed, with a modified billing period. This could be avoided if commerce_recurring_order_renew checks if the order is closed before attempting a renewal.
I believe we'd need to have a change record if we do update to billing periods.
Finally, while limiting the scope for updated orders to drafts only, perhaps making it possible for site owners to decide whether they want all the orders or only draft orders (default) would be a good strategy here.