Remaining tasks
- Zero-rate licenses that are scheduled for cancellation instead of outright removing them from the order. Another solution may be best, but the goal is for the recurring order not to get prematurely deleted before the license status is actually changed.
- Apply scheduled changes to prepaid licenses before charging them.
- Ensure that postpaid metered billing on prepaid-cycle products does not break. This might require treating scheduled product
changes differently from status
changes. Cancellation should happen prior to charging. Product changes should happen after postpaid metered billing has been charged, but before the next month's prepayment is charged.
This is a hard one to solve, and it is probably best solved in the interface, or maybe even at the implementation level. And, thinking about it more, it might be ideal to, at closing time, move postpaid metered usage on prepaid-product license orders to a separate, fixed order.
Then we can apply scheduled changes as we wish, knowing we've already extracted the metered usage...oh, hey, wait. We don't have to make a separate order. We can do this within the same order right before we charge it — basically add a fixed line item with the final metered usage. If this doesn't happen already — I don't think it does.
Original issue
Is it right that order is first charged before scheduled changes are applied? It think it should apply schedule changes first, and only then, if the license is still active, charge it and renew it.
Now the user is charged and his license becomes inactive immediately (prepaid billing), which I think is wrong.
Charging code is in commerce_license_billing_cycle_close_queue_process function, and applying a scheduled change runs in commerce_license_billing_cycle_renew_queue_process function which runs after the first.
Comments
Comment #1
bojanz CreditAttribution: bojanz commentedThe current flow goes like this:
1) Billing cycle expires.
2) Scheduled changes are applied
3) If there are any active licenses left, open a new order and billing cycle.
At the same time, we try to close and charge the old order. This might happen right after #3, or it might happen in 3 days, if we are waiting for usage data.
You'll have to give me a better example so I can understand which part is causing troubles for you.
Comment #2
mindaugasd CreditAttribution: mindaugasd commentedI added some more steps.
First of all, user unsubscribes and his license is scheduled to expire at the end of billing cycle, she shouldn't be charged again, because his billing is prepaid.
At the end of billing cycle the cycle is closed:
Renews billing cycle:
I don’t use “usage based billing”. The problem is that “commerce_license_billing_cycle_close_queue_process” function always charge an order, even then the user already unsubscribed and shouldn't be charged again.
Comment #3
bojanz CreditAttribution: bojanz commentedI get it now. The user is charged an extra month because of this.
(We don't use prepaid in production, so it has always been buggier :/)
Comment #4
ShaunDychko CreditAttribution: ShaunDychko commentedIs the Commerce Recurring Framework a good approach to use until this issue for prepaid cancellations is sorted out?
Comment #5
bojanz CreditAttribution: bojanz commentedCommerce Recurring has no plans, no usage, no integration with commerce_license.
It also has no release of any kind (but probably fewer bugs due to having less code).
Your choice.
Comment #6
mindaugasd CreditAttribution: mindaugasd commentedAttached the patch solving the problem.
Then user cancel subscription, his order status is changed from 'recurring_open' to "cancelled" with custom code. So this patch checks whether user has active orders before making a payment.
Comment #7
bojanz CreditAttribution: bojanz commentedI'm working on a comprehensive fix for this. The tests aren't passing yet, so it will probably be up tomorrow.
Comment #8
bojanz CreditAttribution: bojanz commentedPushed a fix.
Prepaid licenses scheduled for cancellation don't show up in the recurring order anymore.
Trying to close an empty recurring order with cause it to be deleted.
We now have full test coverage for this.
Comment #11
tame4tex CreditAttribution: tame4tex commentedI am still having the same issue with scheduling cancellations for prepaid licenses.
I am using version 7.x-1.0-beta3.
I am scheduling the cancellation with the following code:
commerce_license_billing_change_status($licenseEntity, COMMERCE_LICENSE_REVOKED);
The associate prepaid license line item is deleted from the Recurring Order as expected. When advanced queue (from cron) runs the Recurring Order is deleted as expected.
However, the license remains with the status of active it doesn't get revoked or expired.
I am guessing that it is because when commerce_license_billing_cycle_renew_queue_process() runs there are no licenses returned buy commerce_license_billing_get_recurring_order_licenses($order), therefore commerce_license_billing_apply_changes($license, $changes) doesn't run.
Comment #12
tame4tex CreditAttribution: tame4tex commentedComment #13
tame4tex CreditAttribution: tame4tex commentedI should also add that your test makes the assumption that commerce_license_billing_cycle_renew_queue_process is run before commerce_license_billing_cycle_close_queue_process. This does not appear to be the case. In debugging this issue it appears that commerce_license_billing_cycle_close_queue_process is run first.
Comment #14
wesjones CreditAttribution: wesjones commentedCan we expect any update on this issue? I am having the same problem. Prepaid license, set to suspended... still gets charged when advanced queue runs. Using version 7.x-1.0-beta4.
Comment #15
wizonesolutionsYeah, main problem here is that
commerce_license_billing_cycle_close_queue_process()
, at least on my site, always runs before renewal. And it makes sense — if payment fails, the license should not get renewed. But it's unfortunate because it means that scheduled changes get applied too late. In the case of cancellations, the license status never gets updated becausecommerce_license_billing_collect_charges()
knows that it's going to be canceled and shows no price, and this causes the order, and therefore the billing cycle to be deleted. I work around this by scheduling a status change toCOMMERCE_LICENSE_EXPIRED
instead.This also affects downgrades because payment happens at close, not renew. But status changes don't happen until renew.
I think I'm going to propose a patch where for prepaid billing cycles, scheduled changes get run at close. For postpaid, we definitely want them to be run at renew time (because payment happens after usage).
Comment #16
wizonesolutionsWorking on this for about an hour today.
Comment #17
wizonesolutionsMostly just looked at the existing code today and reasoned about it. I'm pretty much just going to scratch my itch and post an incomplete patch with what I did. There should be tests for some stuff, but I'm not going to write them because I only need this to work in my case:
Assuming we apply scheduled license changes even before we potentially delete an empty order:
- What happens if you change a prepaid product to postpaid or vice-versa? Same behavior? (I think it'd be fine because most of the logic works for both prepaid and postpaid. Also, this is probably an extreme edge case — more likely the product change would either be applied immediately (postpaid -> prepaid) or Just Work since there would be no money to charge right away (prepaid -> postpaid).
That's actually the only thing I got curious about. Other scheduled changes like status should be just fine. In my case, I want
COMMERCE_LICENSE_REVOKED
applied to the license at close time if it's using a prepaid product so that it properly gets revoked before the billing cycle disappears. And I also want the product to change *before* we charge them so that the right amount gets charged (when downgrading).For postpaid, changes will be applied at renewal like now.
Will be doing more on this tomorrow.
Comment #18
wizonesolutionsComment #19
wizonesolutions12:25:44 oh......I just thought of another use case that might break...well, I'll test for it. Basically — prepaid license that supports metered billing gets canceled. I might have to handle that case as well...hmm...
...
12:29:27 I guess logically it's not too bad, though. Main thing to do is to ensure that the correct charges get collected even if the license is excluded at collect-charges time. And the renew process won't remove orders that have line items...so yeah, it should actually Just Work...I think. Well, I will test.
...
12:40:02 A big problem here is that the license line item actually gets _removed_ in the order refresh...so it's not even there for changes to be applied to later on. What if I schedule cancellation and then undo it though...I wonder if the order rebuilds properly...
...
13:00:36 Oh, my...a *scheduled* cancellation results in *permanent* deletion of line items from the recurring order. If I undo the cancellation, the line item's still gone. That's another thing to fix.
Comment #20
wizonesolutionsMaking progress on this. I have the recurring order showing scheduled plan changes and cancellations. Now I need to apply the scheduled changes at the optimal time so that the charge actually reflects what the order says. (It's a display thing only during the order refresh, but the billing cycle closure process doesn't get the price the same way or something.)
Comment #21
wizonesolutionsOK, here is the CLB part of the patch. It needs this patch to be applied first to work properly: #2736709: Allow modules to back-date $entity->revision_created.
Basic summary of changes:
commerce_license_billing_collect_charges()
to handle scheduled changes appropriately. Namely, it doesn't exclude providing a charge array for a license unless it's actually canceled. This allows upgrade/downgrade forms that delay cancellation (cough, mine, cough) to let the user undo their cancellation. It also ensures that metered usage will get billed properly, and doesn't remove the licenses in that case. It just zero-rates canceled (COMMERCE_LICENSE_REVOKED
) licenses, so they won't charge anything anyway.It works well, although I haven't run the tests again yet. Hopefully testbot will...now.
Comment #22
arthur_drupal CreditAttribution: arthur_drupal commentedHi everybody,
I will do more tests but i think i have resolved the issue without need to patch.
I have created a Rule that schedule the licence status when a license is revoked:
commerce_license_billing_change_status($license, COMMERCE_LICENSE_REVOKED);
You can easily create a Rule action like this
And your callback look like:
As I said, i did a successfull test but i will do again, i will tell you informed,
P.S : You can trigger this action when the License is changed to revoked
Best,
Arthur
Comment #23
wizonesolutions@arthur_drupal: Main problem with that is that it won't work with metered billing and it cannot be undone. For cancel workflows that allow the customer to undo their decision (for scheduled cancellations) before the end of the billing period, it would be a problem. This is because it results in the line item being physically removed on order refresh without any way to get it back. This happens even before the billing cycle is over.
If you are e.g. canceling and refunding prorated though then this might work out OK.
Comment #24
Lord Pachelbel CreditAttribution: Lord Pachelbel commented#21 worked for me.
I see that it changes the order status to "Recurring: Payment Pending." What needs to happen to that order for the customer to cancel their cancellation, i.e. to resubscribe?
Of course they could simply purchase a new subscription which would create a new license and a new set of orders and billing cycles.
Comment #25
swelljoe CreditAttribution: swelljoe commentedI've applied the patches referenced in #21, but canceled orders are still being charged (orders end up in "Payment Failed (hard decline)" state because I've disabled the charge callback method in my payment method because of this bug). Is there something else I need to do to make canceled orders stop being charged?
We're making sure both orders (the current one and the "Recurring: Open" one are canceled (and the "Canceled Date" still shows the canceled date even after it's been changed to Payment Failed status), and the license is definitely Revoked. So, why is it still charging the customer and how do we stop it? What do I need to do to make it leave canceled orders alone?
Comment #26
wizonesolutions@swelljoe: Hmm. And you are using prepaid billing? Should work. You should change status on the license to COMMERCE_LICENSE_REVOKED. Don't touch the recurring order — the order refresh process will update that itself. Also, don't touch already-completed orders. At any one time, there is only one active recurring order anyway. If you have two, sounds like something is wrong.
Hope this helps.
@Lord Pachelbel: After the billing cycle, there should be no charge assessed, and the recurring order should simply disappear. It shouldn't go into
recurring_payment_pending
status — that is for when the payment method fails to run.Since cancellation logic is generally custom in Commerce License, I need more info to answer better. I developed these patches against my own project and upgrade/downgrade form :)
Comment #27
swelljoe CreditAttribution: swelljoe commented@wizeonesolutions: We cancel the initial order because we refund the payment. We offer a "risk free" trial (because our preferred use case of offering one month free doesn't allow collection of payment data), so most of our cancellations are people who get their money back. I'm not sure how we would accurately represent that process without cancelling the order for the current month plus the "Recurring: Open" order.
But, I guess we can try leaving the Recurring: Open order alone, and see what happens with that..but, I'm sure I've seen orders where we've accidentally left it in that state and it still flipped to Payment Failed at the end of the cycle. But, maybe not since the patch. I'll try leaving the latter order alone and see what state it puts it in. I'm confused why it being in Canceled state would make it attempt to charge for a revoked license, though. Seems buggy (not a bug in your patch, just a bug in the general case, that I've been desperately hoping to find some fix for).
Comment #28
Lord Pachelbel CreditAttribution: Lord Pachelbel commentedI figured out how to correctly cancel prepaid subscriptions a while back but didn't post it in here. See https://www.drupal.org/node/2807515#comment-11670037 which will refer back to the patch from #21 in this thread.
Comment #29
g33kg1rl CreditAttribution: g33kg1rl commentedIs the patch from #21 going to be committed or is there another solution?
Comment #30
Lord Pachelbel CreditAttribution: Lord Pachelbel commentedIt turns out my comment from #28 was incorrect. Cancelling prepaid subscriptions is still broken.
When our site launched we were using the patch from #21 and commerce_license_cancel. That module calls commerce_license_billing_change_status() to schedule the cancellations, similar to the rule code posted in #22. When the subscriptions are cancelled in this manner the customer's "Recurring: Open" order's total is set to $0.00.
Over the past year we've had a few customers who have tried to cancel their subscriptions, and they continued to be charged after their licenses expired, repeatedly, and so we had to manually refund them repeatedly.
As a stopgap we're now manually deleting their stored credit card data so that they won't be charged every time the billing cycle fires, but because of that, commerce_license_dunning is now sending emails about failed payments to those customers every month.
We've tried manually setting their active billing cycle statuses to 0, but that hasn't helped. Every month a new active billing cycle is generated for them, their monthly licenses are still active, and their license expiration dates are advanced by one month.
Comment #31
Lord Pachelbel CreditAttribution: Lord Pachelbel commentedUsing the code in https://www.drupal.org/project/commerce_license_billing/issues/2828290 I think I got things working correctly. I'm not sure if that code is doing things the "right" way in the sense commerce_license_billing would expect it to be done, but I think it will fix my problems.
Comment #32
matthensley CreditAttribution: matthensley commentedI've been using this patch for over a year and it seems to be working for the most part, but I think I've pinpointed a snag that's affected a couple of orders and am curious to hear anyone else's thoughts (though this understandably isn't really a hotbed of development at the moment).
I've built in the ability for subscribers to my site to change plans, implementing
commerce_license_billing_change_plan()
. I've found that if a user happens to change plans multiple times, things can get out of line to a situation where the product attached to the license is different than the one they are being billed for. I think I've maybe traced it to this line incommerce_license_billing_collect_charges()
:<?php if ($scheduled_change['property'] == 'product' && $scheduled_change['value'] != $product_wrapper->getIdentifier()) { ?>
If I have two scheduled changes, and the second one is switching back to the initial product, that second condition causes us not to schedule the change at billing time. Upon the initial "scheduling" of the plan change, the line item in the "pending" order is updated to the requested product and the product associated with the license is updated, but when it comes time to collect charges, the product in the recurring order is swapped out for the incorrect one.
Does that seem possible here, or am I missing something?
Comment #33
maskedjellybeanAfter applying the patch in #21 I'm seeing this error when running
drush cron
. Any ideas?WD advancedqueue: [commerce_license_billing_cycle_renew:755084288] failed processing: EntityMetadataWrapperException: Invalid data value given. Be sure it matches the required data type and format. Value at commerce_order()
Comment #34
maskedjellybeanOk I realized the problem.
$order = $billing_cycle->getOrder();
can return false. The code currently assumes it will always return an order. It seems this may be somewhat unrelated to the patch and is a separate issue. I'm not really sure, but I created a new version of the patch that resolves it. It just adds this check in two places after calling$order = $billing_cycle->getOrder();
:I can't say whether this is the correct thing to do, but we were already doing the same thing if a billing cycle is not found:
To be clear, I can't say whether this patch resolves the actual issue this thread is about. I needed to fix this bug before I can test the patch.