-
Notifications
You must be signed in to change notification settings - Fork 684
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
Allow specifying namespace for webhooks jobs #463
Allow specifying namespace for webhooks jobs #463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can you add a note in the changelog and see if there is a section in the README where it makes sense to add a note about this.
1b4b40d
to
1679842
Compare
Yep! Updated the readme and added a CHANGELOG line as well. |
Actually, please hold on the merge on this -- I'm exploring a tweak. I'll comment again when it's good to go. |
1679842
to
32b93f8
Compare
OK - this is good to go now. In order to support plural namespaces (e.g. |
Can you add a test? |
32b93f8
to
548e007
Compare
Yep! Tests added. |
Awesome thanks ❤️ |
This tweak allow specifying a namespace for the webhook jobs.
Rationale: I'm trying to build a generic ecommerce app, keeping in mind that I may expand to support non-Shopify stores in the future. With that in mind, I've namespaced my ShopifyApp webhook jobs under
jobs/shopify/xxx_job.rb
, but despite mounting ShopifyApp under '/shopify', the webhook controller was always raising ShopifyApp::MissingWebhookJobError because it was hardcoded to assume the webhook job names were in the top-level namespace.