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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz’s picture

The 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.

mindaugasd’s picture

I 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:

  • Billing cycle expires.
  • Recurring order status is set to “recurring_payment_pending” (that means the order is charged for the next billing cycle, though it should not happen because the user already unsubscribed)

Renews billing cycle:

  • Scheduled changes are applied
  • If there are any active licenses left, open a new order and 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.

bojanz’s picture

Title: Applying scheduled changes runs after charging an order on prepaid billing » Scheduling cancellations doesn't work properly for prepaid licenses
Priority: Normal » Major

I 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 :/)

ShaunDychko’s picture

Is the Commerce Recurring Framework a good approach to use until this issue for prepaid cancellations is sorted out?

bojanz’s picture

Commerce 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.

mindaugasd’s picture

Status: Active » Needs review
FileSize
791 bytes

Attached 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.

bojanz’s picture

Status: Needs review » Needs work

I'm working on a comprehensive fix for this. The tests aren't passing yet, so it will probably be up tomorrow.

bojanz’s picture

Status: Needs work » Fixed

Pushed 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.

  • Commit 37306a7 on 7.x-1.x by bojanz:
    Issue #2247703: Fixed Scheduling cancellations doesn't work properly for...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tame4tex’s picture

I 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.

tame4tex’s picture

Status: Closed (fixed) » Active
tame4tex’s picture

I 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.

wesjones’s picture

Can 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.

wizonesolutions’s picture

Yeah, 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 because commerce_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 to COMMERCE_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).

wizonesolutions’s picture

Assigned: Unassigned » wizonesolutions

Working on this for about an hour today.

wizonesolutions’s picture

Assigned: wizonesolutions » Unassigned

Mostly 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.

wizonesolutions’s picture

Assigned: Unassigned » wizonesolutions
wizonesolutions’s picture

Assigned: wizonesolutions » Unassigned
Issue summary: View changes

12: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.

wizonesolutions’s picture

Making 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.)

wizonesolutions’s picture

Status: Active » Needs review
FileSize
8.65 KB

OK, 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:

  • Apply scheduled changes to prepaid licenses at the time of closing the order
  • Refresh the order after that so that line items get updated appropriately
  • Modifies 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.

arthur_drupal’s picture

Hi 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

    'NAME_stop_cl_billing' => array(
      'label' => t('Stop a Commerce License recurring billing'),
      'parameter' => array(
        'license' => array(
          'type' => 'commerce_license',
          'label' => t('License to stop'),
        ),
      ),
      'group' => t('MY GROUP'),
      'base' => 'MY_FUNCTION_stop_cl_billing',
    )

And your callback look like:

function MY_FUNCTION_stop_cl_billing(CommerceLicenseInterface $license) {
  commerce_license_billing_change_status($license, COMMERCE_LICENSE_REVOKED);
}

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

wizonesolutions’s picture

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

Lord Pachelbel’s picture

#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.

swelljoe’s picture

I'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?

wizonesolutions’s picture

@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 :)

swelljoe’s picture

@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).

Lord Pachelbel’s picture

I 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.

g33kg1rl’s picture

Is the patch from #21 going to be committed or is there another solution?

Lord Pachelbel’s picture

It 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.

Lord Pachelbel’s picture

Using 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.

matthensley’s picture

I'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 in commerce_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?

maskedjellybean’s picture

After 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()

maskedjellybean’s picture

Ok 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();:

  if (!$order) {
    return array(
      'status' => ADVANCEDQUEUE_STATUS_FAILURE,
      'result' => 'Could not find the order.',
    );
  }

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:

  if (!$billing_cycle) {
    // The billing cycle has been deleted in the meantime. Nothing more to do.
    return array(
      'status' => ADVANCEDQUEUE_STATUS_FAILURE,
      'result' => 'Could not find the billing cycle.',
    );
  }

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.