Add declarative webhooks processing #138
Conversation
The webhook registry in the package assumes that all webhooks to be processed will be defined in the config It also couples subscribing to processing This PR removes the registration loggic from the config. It uses the provided webhook verification concern with new controllers
shopify.app.toml
Outdated
| @@ -6,17 +6,17 @@ scopes = "write_products" | |||
| api_version = "2024-07" | |||
|
|
|||
| [[webhooks.subscriptions]] | |||
| uri = "/api/webhooks/app_uninstalled" | |||
| uri = "/api/shopify_webhooks/app_uninstalled" | |||
There was a problem hiding this comment.
The shopify_app gem uses the /webhook route, so I have defined these routes as /shopify_webhooks.
We do have the ability to change the gem webhook route in the app config. So we could default that to something else and define these under /webhook
There was a problem hiding this comment.
I think I'd prefer to use /webhooks for the declarative hooks - this is the easiest and preferred way to set up webhooks, so I'd like to make it the easiest to use.
There was a problem hiding this comment.
I misunderstood what config.webhook_jobs_namespace does. It only changes the namespace of the jobs, not the route.
But I figured out if we change the order of how we define the routes, it allows us to send both types of webhooks to /webhooks.
lizkenyon
force-pushed
the
liz/declarative-webhooks
branch
from
6dff26d
to
f4e9cb1
Compare
There was a problem hiding this comment.
This works well with declarative webhooks.
This overwrites the existing webhook registration routes with explicit routes and controllers that handle the appropriate webhook registrations in the shopify.app.toml file.
The caveat is one needs to know that if they use config.webhooks for registering webhooks, they don't need to make a new controller and only need to add an active job corresponding to that webhook topic.
lizkenyon
force-pushed
the
liz/declarative-webhooks
branch
from
1252462
to
e4f686e
Compare
lizkenyon
force-pushed
the
liz/declarative-webhooks
branch
from
e4f686e
to
42e7a1e
Compare
lizkenyon
force-pushed
the
liz/declarative-webhooks
branch
from
42e7a1e
to
9f31bff
Compare
lizkenyon
force-pushed
the
liz/declarative-webhooks
branch
from
ff14c2a
to
637fe6e
Compare
| @@ -18,6 +18,12 @@ | |||
| get :count | |||
| end | |||
| end | |||
| namespace :webhooks do | |||
There was a problem hiding this comment.
Should we call out that routes will need to be added? Or can we make the topic generic so each one doesn't have to be explicitly added?
There was a problem hiding this comment.
I didn't see this comment, before merging, But I do have docs in the shopify_app gem. But I would also be good to add comments explaining this
There was a problem hiding this comment.
Each webhook needs a route, but the generators I created add the route for you.
There was a problem hiding this comment.
Oh nice, then that's probably fine then, right?
WHY are these changes introduced?
Fixes ShopifyAPI::Errors::NoWebhookHandler for mandatory webhooks
Adds the ability to process webhooks that have been declared in the
shopify.app.tomlThe
shopify_appgem couples the creating of the webhook subscription with the processing of the webhook events.To support declarative webhooks, the template needs to be able to handle webhook events that were not subscribed to via the gem.
WHAT is this pull request doing?
registry.processfunction does####Questions
shopify_appthat generates a new controller + job?Checklist
Note: once this PR is merged, it becomes a new release for this template.
README.mdfile and other related documentation, if applicable