[WebHooks] - expose allowed webhook events in webhook store #16835
Conversation
jhoward1994
added
the
pr: enhancement
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@Marc-Roig & @nathan-pichon - This PR amends the BE of the webhooks as we discussed the other day Do you think it's okay to merge this first and handle the emitting of the review workflow events in a separate PR? |
packages/core/admin/ee/server/services/review-workflows/review-workflows.js
Show resolved
Hide resolved
There was a problem hiding this comment.
nice work on webhooks 🚀
Added some minor comments, and just to throw my random thought
await this.emitEvent(strapi.webhookStore.allowedEvents.get('MEDIA_UPDATE'), res);
I was trying to think shorter ways to get the event name, because I feel it's a bit too much, but other than using a constant in each package I can't think of anything else 🤔
Yes :) I think is the best approach to do that |
|
@Marc-Roig & @nathan-pichon this is ready for another review now 🙏🏻 |
|
Hey @nathan-pichon & @Marc-Roig sorry for the delay this is ready for review again. The only thing I'm unsure of is whether the entity service events should be registered in the bootstrap phase like other plugins or from the entity service. wdyt? For other plugins I had to register them in the bootstrap phase as the services aren't initialised until you're using the plugin - e.g. having navigated to the upload plugin |
|
The Entity service is responsible for the business code around entities, so, it makes sense that the Entity service put its own event in the webhook module. I would say that the second option is better, as a service should be capable of being instantiated multiple times (we might not want to add events each time the service is instantiated, even if the map will just overwrite the overlapping events). |
|
as of today bootstrap is referred to a lifecycle of a package, so I guess services do not have that. What we can do today is execute code before initialising as you are doing in the PR already. I actually think that is ok, as a service does not really need a register/bootstrap/destroy lifecycle. |
okay, happy to go with this implementation then. I was unsure as it will make the entity service inconsistent with how we register events elsewhere |
| const event = ALLOWED_WEBHOOK_EVENTS[eventName]; | ||
| if (!event) { | ||
| throw new ApplicationError(`The webhook event ${eventName} doesn't exist.`); | ||
| } |
There was a problem hiding this comment.
why do we need this?
could we export from constants something like this:
const { ENTRY_PUBLISH, ENTRY_UNPUBLISH } = require('../constants/webhooks');I think we could do something as before, but importing those webhook events from the package constants.
Sorry for the back and forth, just want to make sure this is as clean as possible!
There was a problem hiding this comment.
No worries that's a good point, I've addressed it in c53b7a7 (#16835)
What does it do?
Changes how we store and validate webhook events in the backend
Storing them in
strapi.webhookStore.allowedEventsAdds relevant events from the upload plugin and ee directory
Why is it needed?
Making webhooks more modular, allowing plugins to extend the webhook store
Related issue(s)/PR(s)
CONTENT-1451