- ๐ณ๐ด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.
- ๐ณ๐ด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.
- @zaporylie opened merge request.
- First commit to issue fork.
- Issue created by @zaporylie
-
jsacksick โ
committed 67721719 on 8.x-1.x authored by
zaporylie โ
Issue #2931290 by zaporylie, heddn, tBKoT, jonathanshaw, jsacksick,...
-
jsacksick โ
committed 67721719 on 8.x-1.x authored by
zaporylie โ
- ๐ฎ๐ฑIsrael jsacksick
Went ahead and merged the request, let's move forward and address what's remaining as followup issues like suggested.
- ๐ณ๐ฟ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. - ๐บ๐ธ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.)