Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: add API for gateway webhook events #7664

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

glaubersilva
Copy link
Contributor

@glaubersilva glaubersilva commented Jan 2, 2025

Sample using this API in a practical way: impress-org/givewp-addon-boilerplate#67

Description

This PR implements a new webhook events API and attaches it to the GiveWP gateways API, the main goal here is to facilitate the process of handling webhook notifications sent by external payment gateways to GiveWP gateway integrations.

Before we needed to create an async job explicitly through our Action Scheduler facade class, so the code for a webhook notification related to the transaction status change would be something like this:

AsBackgroundJobs::enqueueAsyncAction(
	'givewp_' . OffSiteGateway::id() . '_event_donation_completed',
	[$webhookNotification->gatewayPaymentId],
	'CUSTOM_ACTION_SCHEDULER_GROUP'
);

However, the code above didn't make everything work automatically. We also needed to attach an event handler class to the hook created in the code above.

So in the service provider, we need to add something like this:

/**
 * We are using the DonationCompleted event handler class provided by Give Core to process the
 * async background event which is created on the OffSiteGatewayWebhookNotificationHandler class.
 *
 * A full list of event handler classes can be found on the following link:
 * @see https://github.com/impress-org/givewp/tree/develop/src/Framework/PaymentGateways/Webhooks/EventHandlers
 */
Hooks::addAction('givewp_' . OffSiteGateway::id() . '_event_donation_completed', DonationCompleted::class);

This approach is ok, but when you have a bunch of different types of notifications/events to handle you need to pay attention to creating a lot of hooks with certain naming standards.

Also, it's a common thing to implement a custom function that does not need to repeat the custom action scheduler group string on each place you need to create an async background job.

Which makes us wonder...

Why a 3rd party developer integrating a a gateway with GiveWP be concerned about it?

In fact, they even need to know (if they don't want to) that GiveWP is using Action Scheduler under the hood.

So, the new gateway webhook events API allows developers to handle the webhook notifications/events easily, just doing something like this:

OffSiteGateway::webhookEvents()->setDonationStatus(DonationStatus::COMPLETE(), $webhookNotification->gatewayPaymentId);

Is just that! They don't need to more spend time naming hooks and enqueueing them with the proper event handler class attached to it, they even need to know anything about Action Scheduler.

Affects

It will make the new gateway integrations easier.

Visuals

image

Testing Instructions

  1. Run the composer test -- --filter WebhooksEvents command in your terminal;
  2. To see it working in the sample gateway, check the instructions of this related PR: Refactor: add support to the gateway webhook events API givewp-addon-boilerplate#67

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Relevant @unreleased tags included in DocBlocks
  • Includes unit tests
  • Reviewed by the designer (if follows a design)
  • Self Review of code and UX completed

@glaubersilva glaubersilva self-assigned this Jan 2, 2025
@glaubersilva glaubersilva marked this pull request as ready for review January 15, 2025 20:14
@jonwaldstein jonwaldstein self-requested a review January 17, 2025 17:24
*
* @return int The webhook event ID. Zero if there was an error setting the event.
*/
public function setDonationStatus(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if these methods are still too contextual and instead we should explore using our Gateway Api Commands like PaymentComplete and PaymentCompleteHandler to abstract out some of the nuance and let the logic be more declarative.

Some pseudo examples:

$gateway->webhookEvents()->paymentComplete($webhookNotification->gatewayPaymentId);
$gateway->webhook()->markPaymentComplete($webhookNotification->gatewayPaymentId);

Open for discussion!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonwaldstein Initially, I went this way to have fewer methods and make the options provided by this API simpler. But thinking better, I think you are right because having more methods (one for each status/action) can simplify it even more and we can keep using the current methods under the hood.

I liked the idea of your last sample because we can expand it (maybe in a subsequent PR) to support other common features when implementing the webhook logic.

For example:

$gateway->webhook()->getUrl();

This way we can remove the necessity to create a route in the gateway integration class. But a change like that also makes me think that we need a specific interface for the "webhookNotificationsListener" route/method.

For example, we could have something like this:

class OffSiteGateway extends PaymentGateway implements WebhookNotificationsListener

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants