artfulrobot / uk.artfulrobot.civicrm.gocardless

A CiviCRM extension providing GoCardless integration to handle UK Direct Debits.
GNU Affero General Public License v3.0
5 stars 18 forks source link

Extension won't process paid_out payments #92

Closed cheveuxroux69 closed 3 years ago

cheveuxroux69 commented 3 years ago

The info says "Note: the following working day the GoCardless payment status is changed from confirmed to paid_out. Normally the confirmed webhook will have processed the payment before this happens, but the extension will allow processing of payment confirmed webhooks if the payment's status is paid_out too. This can be helpful if there has been a problem with the webhook and you need to replay some." This won't happen because "paid_out" is not an implemented webhook in CRM/GoCardless/Page/Webhook.php line 20.

public static $implemented_webhooks = [
    'payments' => ['confirmed', 'failed'],
    'subscriptions'  => ['cancelled', 'finished'],
  ];

may need to be changed to

public static $implemented_webhooks = [
    'payments' => ['confirmed', 'paid_out', 'failed'],
    'subscriptions'  => ['cancelled', 'finished'],
  ];
cheveuxroux69 commented 3 years ago

There is also the issue that the code tries

foreach ($this->events as $event) {
      try {
        $method = 'do' . ucfirst($event->resource_type) . ucfirst($event->action);

but if resource_type is payments and action is paid_out, the method doPaymentsPaid_out is called, which doesn't exist.

artfulrobot commented 3 years ago

Hi @cheveuxroux69 If I can highlight a different bit of your quoted text:

the extension will allow processing of payment confirmed webhooks if the payment's status is paid_out too

So you're right, the extension does not process the event that a payment entered paid_out status, but it shouldn't need to - most of those events would be duplicating work, since they always occur after[※] the payment enters confirmed status, which we do listent to.

after: So paid_out happens in real world time after completed. However there's a chance that the notification webhook for the completed event is received after the payment has entered paid_out status. The webhook should still be sent, just late.

So the code allows for:

  1. Receive a completed webhook

  2. Look up the payment and find that instead of completed it now has the status paid_out instead of completed as we might have naively expected. That's ok though, the payment has still completed, so we should continue to process it as such.

Thoughts?

artfulrobot commented 3 years ago

(sorry, hit close by accident)

cheveuxroux69 commented 3 years ago

Yes, you're correct and it looks like I misunderstood the text, I thought it meant that webhooks containing 'payment-paid_out' events would also be processed. In fact it meant that webhooks with a 'payment-confirmed' event (due to webhook delays) but having a 'payment-paid_out' API (actual) status would still be processed. Looking at the code again I can see you check this in getAndCheckGoCardlessPayment We have some 400 'payment-confirmed' events that were not processed and I was hoping that the 'paid-out' webhooks would be processed instead the next business day but they weren't. The 'confirmed' webhook was acknowledged so it wasn't resent. I'm going to have to replay 80 'confirmed' webhooks one by one, slowly - at least they will be accepted because you've coded for this! Duplicate payments are not recorded (Failed event. Reason: failed to load related objectsDuplicate error - existing contribution record(s) have a matching Transaction ID or Invoice ID.) so this should be OK to do. You're good to close this now! Thanks for the clarification.

artfulrobot commented 3 years ago

wow how come they weren't processed?

artfulrobot commented 3 years ago

If it's 400 to process, you might want to look at the import script (you have to change it for your needs) rather than replaying from GC end. Alternatively you'll find the payloads of the failed ones in your log files and you could script the (re)processing of those.

But first find out why those weren't processed and do feel free to open a new issue if you've found a problem that might affect others.

Thanks for your engagement :+1: