Basic implementation of webhooks for moriana integration #4322
Conversation
… webhook request; enable webhooks globally or by location
… webhook request; enable webhooks globally or by location
| @@ -34,8 +34,8 @@ class ApiClientService { | |||
| return execute(new HttpGet(url)) | |||
| } | |||
|
|
|||
| JSONObject post(String url, Map requestData) { | |||
| return execute(new HttpPost(url), requestData) | |||
| JSONObject post(String url, Map payload, Map headers) { | |||
There was a problem hiding this comment.
Looks like this is used in the RecaptchaService. I think you should either add a default option for headers here or add an empty map there.
There was a problem hiding this comment.
Good point. I thought I had left the previous method, but I guess not. I like using a default option instead of chaining overriding methods so I'll go with that approach.
| class WebhookPublisherService { | ||
|
|
||
| def apiClientService | ||
| //def grailsApplication |
There was a problem hiding this comment.
If not required, then please remove
| return | ||
| } | ||
|
|
||
| Map payload = [ |
There was a problem hiding this comment.
Perhaps it's worth moving into Shipment, as toWebhookJson() or something like that (?)
There was a problem hiding this comment.
Or this should be related to a specific webhook? What I imagine is there might be a couple of webhooks configured at the same time and each could have its own "mapping". So perhaps there would be a better place for webhook payload models instead of having this in the shipment?
There was a problem hiding this comment.
I was letting the implementation of this marinate in my head a bit. I agree each domain should know what it's payload is, but this isn't just a webhook thing. Instead it needs to support generic notifications about each domain and state change. We are planning to deal with this in the improved webhook feature in OBGM, so I think we can implement it there. You can review Chetan's PR #4300 and provide feedback there
But essentially, I want the application to emit application-level and database-level events for everything that happens in the system (i.e. ShipmentStatusUpdateEvent, HibernateUpdateEvent, ProductCreatedEvent, UserLoginEvent, etc).
A system administrator will be able to define different notification mechanisms: webhook, email, SMS, and push notifications. The application will allow the user to subscribe to any of the events that implement the NotifiableEvent interface (or extend it if we decide it should be an abstract class). For now, these subscriptions will be configured in the application configuration, but eventually, we'll have a table where we can manage the event notification subscriptions.
When an application event is fired (we need to manually add these in the codebase for the app-level events, but we'll have a Hibernate persistence listener to publish the low-level database events) those events will be sent to the event bus. We'll have a listener listening for instances of NotifiableEvent or whatever we call them. These events need to provide enough information for us to send a notification, so we will convert them, I think, to a NotificationEvent which has a context (based on the source object) and configuration (type = webhook, SMS, email, templates, etc). The NotificationEvent is published to the same event bus but now the listeners are going to be implementations of a NotificationEventService which know how to publish to SMTP server, SMS endpoint, or webhook. The notification event service takes the notification context and the config to create the components needed for each type of notification i.e. email requires an string subject, HTML body, email recipients; webhook requires a URL, payload, headers.
Anyway, that's the idea behind the improved version (#4300) but I still need to review the code to make sure we implemented it that way.
I think the latest discussion of the webhooks happened in the #moriana Slack channel, but it might be buried in a document or ticket somewhere as well.
https://openboxes.slack.com/archives/C05SDV26YKC/p1695918514682159
Here's the original ticket
https://pihemr.atlassian.net/browse/OBS-1515
cc @awalkowiak @drodzewicz @alannadolny @kkaczmarczyk @chetanmaharishi
| @@ -63,6 +63,7 @@ class ShipmentService { | |||
| def documentService | |||
| def personDataService | |||
| def productAvailabilityService | |||
| def webhookPublisherService | |||
* WIP: Initial implementation of webhook event publisher * WIP: Added shipment details to webhook payload; added auth headers to webhook request; enable webhooks globally or by location * WIP: Initial implementation of webhook event publisher * WIP: Added shipment details to webhook payload; added auth headers to webhook request; enable webhooks globally or by location * OBPIH-5891 Cannot invoke method format on null object * OBS-1515 Fixed API client service bug; removed unnecessary dependency injections
No description provided.