- 🇮🇱Israel jsacksick
So I'm guessing the query should filter out subscription with NULL end dates? Is it NULL or 0?
- 🇮🇱Israel jsacksick
So as soon as we address #29, we also should refactor the following code:
+ if (!in_array($subscription->getState()->getId(), ['pending', 'trial', 'active'], TRUE)) { + return JobResult::failure(sprintf('Unsupported subscription status. Supported statuses: ("trial", "pending", "active"), Actual: "%s").', $subscription->getState()->getId())); + }
To use the
isTransitionAllowed()
method. - 🇮🇱Israel jsacksick
The attached patch addresses the concerns from comment #29.
Regarding:
Because the cancellation is enqueued now, is there any possibility that a draft order will be triggered before the cancellation? If that were to happen, then the customer would be charged, receive a receipt, and immediately have the subscription cancelled.
I think you're right, but even if the subscription was to expire right away (instead of via a queue), the open recurring orders are closed & renewed prior to applying changes to the subscriptions, due to:
$this->enqueueOrders($recurring_queue); $this->enqueueSubscriptions($recurring_queue);
So we'd need to change the orders and not queue the expiration. The problem is, not queuing the subscription expiration has a performance impact... This could work for installs with a relatively low number of subscriptions to expire... But let's imagine a 1000 subscriptions that have to be expired... The cron job could potentially crash...
- 🇮🇱Israel jsacksick
Also... wondering if this shouldn't be closed in favor of ✨ Support stopping billing after a number of billing periods Needs work ... These 2 issues are kinda related...
I'm thinking that the isRenewable() method introduced there should check whether the subscription has an end date in the past.
- 🇮🇱Israel jsacksick
Also, one thing that isn't really clear to me with the changes introduced here is... When is the end date actually set? Is it expected to set it via custom code or from the UI manually?
- 🇮🇱Israel jsacksick
Back to this again and sorry for the noise...
The end time is set when the subscription is canceled or scheduled for cancelation... I'm now wondering why we have an expire transition? What would be the usecase? What is the practical difference between expiring a subscription and canceling it?
Also when a subscription is expired, there is no way to "unexpire", so in practice, it is the same as a cancelation? - First commit to issue fork.
- Merge request !33Draft: Subscription with end date does not expire → (Open) created by Unnamed author
- 🇳🇴Norway zaporylie
I did some manual testing around this bug report and I agree with #35 about the
isRenewable
approach.This MR is where test implementation was done. Current WIP still lacks automated tests, but I validated this approach with manual testing with rather positive results.
Still needs work but feedback is welcome.