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

WebhooksManager#recreate_webhooks! should creates webhooks #1718

Closed

Conversation

tomriley
Copy link

What this PR does

#recreate_webhooks! doesn't seem to attempt to create webhooks after destroying them. This patch fixes that.

I have also removed the suggestion of including manual registration of mandatory webhooks from the generator template. Shopify graphql API documentation states that these shouldn't be registered manually via the API and instead need be set up on the partner dashboard (in App Settings). From the docs:

"You don't create webhook subscriptions to mandatory webhooks. Instead, you configure mandatory webhooks in your Partner Dashboard as part of your app setup."

To avoid errors, existing users should remove the three mandatory webhooks from their ShopifyApp.configuration.webhooks.

Reviewer's guide to testing

Mandatory webhook configuration should be removed before testing on a real app.

Things to focus on

  1. Is Shopify documentation correct? Are all these manually webhook API subscriptions just being ignored? Why are the subscription not failing? So many questions...

Checklist

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users
  • Update README.md, if appropriate.
  • Update any relevant pages in /docs, if necessary
  • For security fixes, the Disclosure Policy must be followed.

@tomriley
Copy link
Author

I have signed the CLA!

@tomriley
Copy link
Author

Okay, I better understand the issue now - the config is required for ShopifyApp::WebhooksController to hand of handling of a mandatory webhook to ShopifyAPI::Webhooks::Registry. I still think this is a bug in shopify_app and mandatory webhooks should be handled as a special case. They have to be statically configured through the partner dashboard and not per-install. Treating them the same as regular webhooks seems to be the problem.

I will look at updating ShopifyApp::WebhooksController...

@nelsonwittwer
Copy link
Contributor

Thanks for your help finding the solution here. I stumbled across this same solution with #1743 while trying to track down another issue. Totally agree that mandatory webhooks are goofy here within the app gem. I'll get that working before I merge in that PR. Closing this and consolidating with #1743

@nelsonwittwer
Copy link
Contributor

Friction with mandatory topics has been addressed here - Shopify/shopify-api-ruby#1237

Thanks for your help identifying this problem!

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.

2 participants