The last submitted patch, 28: commerce_recurring-2919597-28.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- Status changed to Needs work
over 1 year ago 7:52pm 14 July 2023 - 🇮🇱Israel jsacksick
Coming back to this after several years... I'm a bit confused by the changes introduced in comment #18... If we don't close the open recurring order, then we aren't going to collect the payment for current billing period??
Also, because the total number of billing periods is defined at the billing cycle level... Then the number isn't saved when the subscription gets created. I wonder if we should forbid changing the number once the billing cycle is created? Or simply save that number on the subscription? This way if the number goes from 6 to 12, but expected prior subscriptions to stick to 6, then we'd be covered... Probably an edgecase we could safely ignore for now though...
- 🇮🇱Israel jsacksick
I'm thinking that
isRenewable()
should return FALSE if for whatever reason it has an end date in the past (for a subscription that is set to expire, assuming we go ahead with 🐛 Subscription with end date does not expire Needs work , but thinking we should consolidate both issues into one...Thoughts?
- 🇳🇴Norway zaporylie
I've been analyzing this issue first, comment by comment, patch by patch, and I couldn't wrap my head around why in the initial specs this new config belongs to the billing schedule. To me, it seems like it should either belong to the billing schedule plugins where trials and intervals are defined (interval definition + default number of intervals, 0 for evergreen) or set on a per-subscription basis. But then I started thinking about 🐛 Subscription with end date does not expire Needs work and how that would be redundant as we already have the end date defined on the subscription. I agree with #32 that these 2 issues are interconnected and can probably be solved together if we play it smart.
I think it's smart to have
isRenewable()
on the Subscription entity (or should it belong to the manager?) for handling either case. Also for writing custom logic if needed. I also think thattotalBillingPeriods
introduced here should help in establishing the end date when the subscription is initially created rather than being a second factor that can decide whether the subscription has expired or not. In such a way if a subscription is ordered for a limited time only (given max number of intervals set), the end date will be auto-populated and subject to amendments if a customer decides to further extend or even shorten the subscription.As for changes made in #18, it seems like the author wants to have the possibility of creating a subscription with no recurring orders. This scenario is covered by Commerce Core (default order) and therefore should probably not be considered here.
- Merge request !32Issue #2919597: Support stopping billing after a number of billing periods → (Open) created by Unnamed author
- 🇳🇴Norway zaporylie
I opened an MR that moves the responsibility over the total billing period to the billing schedule plugin and adds a new interface that allows plugins to determine the (default) end date for the subscription. This is still considered a draft so I'll keep the needs work status. Furthermore, it requires 🐛 Subscription with end date does not expire Needs work to be fixed first so I'll move over there now and propose a fix.