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

Add session argument in ShopifyApp::WebhooksManager.destroy_webhooks #1569

Merged
merged 4 commits into from
Dec 15, 2022

Conversation

m11o
Copy link
Contributor

@m11o m11o commented Nov 4, 2022

What this PR does

Closes: #1568
When ShopifyApp::WebhooksManager.recreate_webhooks! is executed, it occurs bellow error.

session = ShopifyAPI::Auth::Session.new(shop: 'example.myshopify.com', access_token: 'access_token')
ShopifyApp::WebhooksManager.recreate_webhooks!(session: session)
ArgumentError (missing keyword: :session):
/vendor/bundle/ruby/2.7.0/gems/shopify_api-12.2.1/lib/shopify_api/webhooks/registry.rb:99:in `unregister'
/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10324/lib/types/private/methods/call_validation.rb:157:in `bind_call'
/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10324/lib/types/private/methods/call_validation.rb:157:in `validate_call'
/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10324/lib/types/private/methods/call_validation.rb:89:in `block in create_validator_slow'
/vendor/bundle/ruby/2.7.0/gems/shopify_app-21.1.1/lib/shopify_app/managers/webhooks_manager.rb:33:in `block in destroy_webhooks'
/vendor/bundle/ruby/2.7.0/gems/shopify_app-21.1.1/lib/shopify_app/managers/webhooks_manager.rb:32:in `each'
/vendor/bundle/ruby/2.7.0/gems/shopify_app-21.1.1/lib/shopify_app/managers/webhooks_manager.rb:32:in `destroy_webhooks'
/vendor/bundle/ruby/2.7.0/gems/shopify_app-21.1.1/lib/shopify_app/managers/webhooks_manager.rb:22:in `recreate_webhooks!'

I think the reason the error occurred is ShopifyApp::WebhooksManager.recreate_webhooks! calls destroy_webhooks in it and calls ShopifyAPI::Webhooks::Registry.unregister in destroy_webhooks method.
But, destroy_webhooks doesn't receive any arguments, and it doesn't pass a session argument to unregister method.

So, This PR adds a session argument in destroy_webhooks method and passes it in recreate_webhooks!
Also, I add tests for destroy_webhooks because there aren't tests.

Things to focus on

  1. lib/shopify_app/managers/webhooks_manager.rb

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.

Please review them.
Thanks

@m11o m11o changed the title Add session arguments in ShopifyApp::WebhooksManager.destroy_webhooks Add session argument in ShopifyApp::WebhooksManager.destroy_webhooks Nov 4, 2022
Copy link

@dasmodal dasmodal left a comment

Choose a reason for hiding this comment

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

That looks good!

@klenotiw
Copy link
Contributor

Yeah this looks great! Thanks so much for the contribution. If you can rebase and fix the conflicts we would love to get this in the next release.

@m11o
Copy link
Contributor Author

m11o commented Dec 15, 2022

@klenotiw
I fixed the conflict. Would you please review it?

@klenotiw klenotiw merged commit 82616dd into Shopify:main Dec 15, 2022
@klenotiw
Copy link
Contributor

Thanks for the contribution! 🎉

@m11o m11o deleted the feature/fix-recreate-webhook-bug branch December 16, 2022 01:04
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems January 5, 2023 21:04 Inactive
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.

Not working ShopifyApp::WebhookManager.recreate_webhooks!
3 participants