eileenmcnaughton / nz.co.fuzion.omnipaymultiprocessor

Omnipay Multi Processor Payment Processor For CiviCRM
Other
13 stars 44 forks source link

What happens when ProcessRecurring misses a day? #199

Open michaelmcandrew opened 3 years ago

michaelmcandrew commented 3 years ago

Imagine that you have a weekly recurring payment set up with PayPal checkout but that, for some reason, Job.ProcessRecurring does not get run. Does this effectively break that recurring contribution?

My reading of https://github.com/eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor/blob/master/api/v3/Job/ProcessRecurring.php#L14 is that it will never get processed because only recurring contributions on the day are considered due.

This seems safe but I wonder if we need a way to mark recurring contributions as overdue / needing attention?

Michael

eileenmcnaughton commented 3 years ago

@michaelmcandrew yes - I think you are right & that it is a weakness. I'm inclined to think we should actually pick up ones missed in the past few days

michaelmcandrew commented 3 years ago

That's a good idea @eileenmcnaughton. I am worried about the danger of bringing very old overdue recurring contributions back to life. A 5 day limit, for example, would solve that problem.

A default setting for how many days to go back might work nicely. We can add that if you like.

We would want to be sure about how the next scheduled date is set in these cases. Is it relative to the last scheduled date? Or is is relative to the date that the contribution was taken? Relative to the last scheduled date seems most appropriate but there is the edge case (depending on how far we go back) of missing multiple payments. For example, lets say we have a weekly payment that missed 4 payments and we were going back 30 days. We would likely see a sequence of 4 daily payments while we caught up, which may or may not be desirable. It would certainly likely surprise the contributor.

Checking whether next_sched_contribution_date is in the past and if so, moving to the next next_sched_contribution_date until it is in the future would be one way to fix this. We would need to think about how and when to run this.

Additionally, I think we need a way of identifying payments that didn't get picked in the window that we set, say if the payment processor was not running for a couple of weeks. Searching for recurring contributions with a next scheduled date in the past would be good enough, I suppose. Or perhaps searching for recurring contributions that have a status of overdue.

On that note, do we have a recurring contribution status processor that can run through recurring contributions and mark them as overdue if the payment is in the past? That (or something similar) seems like it would be useful else the overdue status will not get set (for these type of payments at least).

Any thoughts you have on this would be well received @eileenmcnaughton and then @elcapo @kurund and I can come up with a spec to contribute this back.

eileenmcnaughton commented 3 years ago

All good thoughts @michaelmcandrew - I would say that a setting for how many days to go back would be fine - or we could just make it 5 days & add a setting later if there is a request (settings can be a pain from a maintenance point of view). Also having it be 5 days makes the next question a bit simpler - ie it's easier to come up with a next date formula if we know we are dealing with 'only just missed' payments.

Most people only pay their cards once a month so either the missed date or the actual date is probably OK. However, if they have a configured cycle_day (I think that's the field name) we should respect it - although I think support for cycle day is sketchy pretty much everywhere

I'm not too sure how recurrings get set to overdue - probably a scheduled job in core should exist if it doesn't

Last point - yes I agree with your offer to include unit tests :-)

mattwire commented 3 years ago

The GoCardless extension implements and documents a good overdue workflow which will soon be implemented in Stripe as well: https://docs.civicrm.org/gocardless/en/latest/reference/technical/

Note that Stripe/GoCardless etc. all rely on the payment provider to actually manage the payment schedule so you never miss payments whereas iATS, omnipay paypal rely on CiviCRM for each payment. iATS does implement scheduled jobs to "catch up" and you should look there / discuss with @KarinG before re-inventing the wheel :-)

michaelmcandrew commented 3 years ago

Thanks for the pointers, Mat - will take a look.

elcapo commented 3 years ago

Thanks Mat!

Arguments of the Job of the iATS Extension

I think that a good starting point would be to consider using an interface similar to the one defined by the Iatsrecurringcontributions job. In particular, it defines:

In principle, it seems reasonable to me, in order to standardize, make an attempt to implement a close interface with just the two changes described below.

Moving the Next Scheduled Date Properly

The catchup argument, changes the way that the next scheduled date is calculated. When the option is used, the next scheduled date is calculated by adding the corresponding time (frequency units * frequency interval) to the value of the next scheduled date before the payment is processed.

Change 1: Tweaking the Failure Count Filter

I'm not fully convinced by the way the failure_count is being used in the iATS job, as it's using it to limit the process to payments with a specific number of failures. I'd suggest defining a max_failure_count instead.

Change 2: Adding an Argument: Maximum Days in the Past

Still, I think that an extra parameter as Michael suggested, in the lines of max_days_in_the_past, may be useful. As the iATS extension is processing all recurring contributions with a next scheduled date lower or equal to the end of the current date. What means that, although the catchup argument would ensure that the next scheduled dates were moved properly, all the recurring contributions that were missed, would be processed.

@michaelmcandrew How does this sound?

eileenmcnaughton commented 3 years ago

Re cycle_day only process contributions that match a specific cycle day - note that this is often not set / left as 1 which we wind up treating as 'meh, whatever'

michaelmcandrew commented 3 years ago

thanks @elcapo - lets arrange a time to talk this through face to face.

The GoCardless extension implements and documents a good overdue workflow which will soon be implemented in Stripe as well: https://docs.civicrm.org/gocardless/en/latest/reference/technical/

@mattwire - have come across that before and agree that is it good. If you are considering adding this to Stripe as well, have you thought about adding it to core instead? It feels to me like more of a missing feature than something specific to these payment processors.

mattwire commented 3 years ago

have come across that before and agree that is it good. If you are considering adding this to Stripe as well, have you thought about adding it to core instead?

@michaelmcandrew The recurring status flow is tightly tied what the payment processor provides (ie. webhooks / notifications). I normally evaluate what could be added to core to help but it's more likely that we'll add this logic to mjwshared/paymentshared extension so it's available for any processor using that library.

adixon commented 3 years ago

I wrote this up (some time ago, it might not be fully up to date) about handing recurring contribution failures for iATS:

https://github.com/iATSPayments/com.iatspayments.civicrm/wiki/Recurring-Contribution-Failure-Handling

My conclusion is that failure handling is essentially hard. I suspect most recurring logic at the payment processor end is opaque because they're equally uncertain about what the right way to handle it is.

The only big bit I think you've missed in your discussion above is the 'failure reason' aspect - how your processor handles a failure should be dependent on why it failed. So for iATS, a "missing card" will stop immediately, whereas a 'general failure' kind of reason will keep trying for a few days and then give up until the next month. This chunk of logic requires that the processing code can properly classify the failure reason like this.

"a few days" is configured by default to 3 but you can change that in the iATS extension settings.

I agree w/ @eileenmcnaughton that the cycle day is not well supported in core. iATS has a feature that allows you as the administrator to limit which days recurring payments go out on (e.g. if you want all your recurring payments to happen on the 1st of the month). But I don't think it's very widely used. I had a client who wanted to limit their payments to the 1 and 15th, for legacy reasons and also so that their administrator could organize their time/workflows better (they had a lot of recurring payments).

One last bit that still needs work is the handling of processor server and communication failures - i.e. what happens if you don't get any answer back from the payment processor. Those are tricky because sometimes you don't even know if the payment got processed, or even whether the failure happened due to e.g. a network error on the civicrm server. These are dangerous because if you don't think them through you end up potentially reprocessing the same payment multiple times, resulting in very annoyed donors!

eileenmcnaughton commented 3 years ago

@adixon I have been reluctant to improve cycle handling in core since I've been worried it might impact on IATS & maybe SEPA - but I do think ideally we should improve core handling of cycle day