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

Add registration method for webhook handlers processing events from the WCPay server #599

Closed
jrodger opened this issue Apr 17, 2020 · 3 comments
Labels
category: engineering For product engineering, architecture work, tech debt and so on. needs prioritisation Triage finished and issues are ready for the following processing. type: enhancement The issue is a request for an enhancement. type: good for architectural review This issue/PR is related to / good for architectural review.

Comments

@jrodger
Copy link
Contributor

jrodger commented Apr 17, 2020

In #411 we're adding the ability for the plugin to process webhooks events sent from the WCPay server. Related to server#227, it would be good if we didn't make the controller class too cluttered.

We could add a registration mechanism so that all of the actual event processing is handled in dedicated service classes.

@jrodger jrodger added the type: good for architectural review This issue/PR is related to / good for architectural review. label Apr 17, 2020
@DanReyLop DanReyLop added the priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. label May 6, 2020
@vbelolapotkov vbelolapotkov added category: engineering For product engineering, architecture work, tech debt and so on. type: enhancement The issue is a request for an enhancement. needs prioritisation Triage finished and issues are ready for the following processing. labels Dec 28, 2021
@naman03malhotra naman03malhotra removed the priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. label Jan 18, 2022
@RadoslavGeorgiev
Copy link
Contributor

@marcinbot
Copy link
Contributor

Looks like this is similar, but on the client-side. We still process webhooks inside WC_REST_Payments_Webhook_Controller which will probably grow and contain all sorts of business logic that could be easily split across dedicated services for clarity.

That said, we don't really have that many services in the client either. Maybe dedicated handlers would be a good idea, similar to that server change.

@RadoslavGeorgiev
Copy link
Contributor

WC_REST_Payments_Webhook_Controller is now pretty minimal (91 lines incl. comments and white-space), as most of the logic got moved to WC_Payments_Webhook_Processing_Service.

class-wc-payments-webhook-processing-service.php is still on the longer side (650) lines, but most of the code is related to reading out the necessary REST properties, and forwarding them to the right service. At this point it seems like this issue has been addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: engineering For product engineering, architecture work, tech debt and so on. needs prioritisation Triage finished and issues are ready for the following processing. type: enhancement The issue is a request for an enhancement. type: good for architectural review This issue/PR is related to / good for architectural review.
Projects
None yet
Development

No branches or pull requests

6 participants