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

Moved event emitter middleware to api app #10233

Merged
merged 1 commit into from
Dec 4, 2018

Conversation

rishabhgrg
Copy link
Contributor

@rishabhgrg rishabhgrg commented Dec 3, 2018

closes #10226

  • Moved middleware emitting site.changed events from v2 admin api to apiApp.

Context

We added a new webhook recently which is triggered on site.changed event. The middleware responsible for emitting this event was configured directly to v2 admin api as Admin client was already updated to use v2 APIs.

Problem

While most of the APIs were moved to v2, the post-scheduling adapter is still using default API set which is v0.1 currently. So any scheduled post is published via old APIs which in turn does not trigger the event/webhook for site.changed.

FIX: Moving the emit-event middleware one level up to api app, so it listens to all API response across api versions and fires site.changed event/webhook when needed. Afaik it shouldn't have any side effects on existing event flow as its based on cache-invalidation header in response. The alternate is to narrow it down and add the middleware directly to v0.1 app but till the time we don't have api version specific middleware for this event, it made more sense to add it here.

@kirrg001
Copy link
Contributor

kirrg001 commented Dec 3, 2018

Really nice PR description/context 👍

I was wondering if the site.changed event needs to be triggered for v0.1 changes anyway?
The admin client uses V2 by default, but any other external client could use v0.1. If yes, then the fix is 100% correct. If not, the fix here feels temporary.

In general: we should finish up #10060 asap 😁

@naz
Copy link
Member

naz commented Dec 3, 2018

This change seems logical to me as site.changed can be triggered by both v0.1 and v2 (maybe somebody just has a preference or a client using v0.1 🤷‍♂️)

@rishabhgrg
Copy link
Contributor Author

rishabhgrg commented Dec 3, 2018

I was wondering if the site.changed event needs to be triggered for v0.1 changes anyway?

We can decide to go either way here, It doesn't need to work with v0.1 necessarily. But since currently we have not made explicit that new webhook is limited to v2 APIs, and also since there is no side-effect or changes needed in v0.1 APIs to make it work, I thought it made sense to allow it to avoid confusion. 😄 Wdyt @kirrg001 ?

Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

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

👍 Feel free to self merge

closes TryGhost#10226

- Middleware emits site-changed event used to trigger webhook, was configured to v2 admin api only.
- Change allows all versions of api to emit event in case of cache invalidation
@allouis allouis merged commit 033ddf1 into TryGhost:master Dec 4, 2018
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.

Site changed webhook not triggered on scheduled content
4 participants