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

Refactor recreate webhooks logic #1686

Merged
merged 4 commits into from
Apr 27, 2023
Merged

Conversation

nelsonwittwer
Copy link
Contributor

@nelsonwittwer nelsonwittwer commented Apr 24, 2023

What this PR does

Fixes two bugs with webhooks within the app gem:

  1. The webhook generators need to use the session that is now passed in the block when using with_shopify_session
  2. ShopifyApp::WebhhooksManager#recreate_webhooks! was using API libraries methods to recreate webhooks instead of the methods it already had to do the same work. Since there were differing approaches with storing what topics were being used this was introducing bugs to consumers of both libraries.

Should address Shopify/shopify-api-ruby#1123 and #1659

Reviewer's guide to testing

Use this branch with a shopify app created with the CLI. Create webhooks:

irb(main):003:0>  ShopifyApp::WebhooksManager.recreate_webhooks!(session:)
=> [{:topic=>"app/uninstalled", :address=>"api/webhooks/app_uninstalled"}, {:topic=>"orders/create", :address=>"api/webhooks/order_create"}]

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.

Copy link

@cquemin cquemin left a comment

Choose a reason for hiding this comment

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

LGTM!

@zkusznir
Copy link

Hey @nelsonwittwer, thank you for the fix provided in this PR. I've upgraded shopify_app gem in my project from 21.4 to 21.5 and attempted to recreate webhooks for an existing Shop record that already had some webhooks configured. Unfortunately I'm still getting the same exception raised.

Steps to reproduce:

  1. Run app with shopify_app 21.4
  2. Install the app in Shopify Store
  3. Verify webhooks are registered for the new Shop record
[1] pry(main)> shop = Shop.last
=> #<Shop:0x000000010aaa8ee8 ... >
[2] pry(main)> session = ShopifyAPI::Auth::Session.new(shop: shop.shopify_domain, access_token: shop.shopify_token)
=> #<ShopifyAPI::Auth::Session:0x000000010aaeb040 ... >
[2] pry(main)> ShopifyAPI::Webhook.all(session: session).map { |webhook| webhook.original_state[:topic] }
=> ["app_subscriptions/update", "app/uninstalled", "bulk_operations/finish", "products/update", "products/create", "products/delete", "customers/create", "customers/update", "customers/delete"]
  1. Add new "customers/disable" webhook to config.webhooks in shopify_app.rb initializer
  2. Call .recreate_webhooks! on existing Shop record -> we're still on v21.4 so it's expected to fail
[3] pry(main)> ShopifyApp::WebhooksManager.recreate_webhooks!(session: session)
ShopifyAPI::Errors::WebhookRegistrationError: Failed to fetch webhook from Shopify: Argument 'topics' on Field 'webhookSubscriptions' has an invalid value (CUSTOMERS_DATA_REQUEST). Expected type '[WebhookSubscriptionTopic!]'.
from /Users/zuzannakusznir/.rbenv/versions/3.2.0/lib/ruby/gems/3.2.0/gems/shopify_api-12.4.0/lib/shopify_api/webhooks/registry.rb:154:in `get_webhook_id'
  1. Upgrade shopify_app gem version to 21.5
  2. Enter rails c again and call .recreate_webhooks! on existing Shop record
[1] pry(main)> shop = Shop.last
=> #<Shop:0x000000010aaa8ee8 ... >
[2] pry(main)> session = ShopifyAPI::Auth::Session.new(shop: shop.shopify_domain, access_token: shop.shopify_token)
=> #<ShopifyAPI::Auth::Session:0x000000010aaeb040 ... >
[3] pry(main)> ShopifyApp::WebhooksManager.recreate_webhooks!(session: session)
ShopifyAPI::Errors::WebhookRegistrationError: Failed to fetch webhook from Shopify: Argument 'topics' on Field 'webhookSubscriptions' has an invalid value (CUSTOMERS_DATA_REQUEST). Expected type '[WebhookSubscriptionTopic!]'.
from /Users/zuzannakusznir/.rbenv/versions/3.2.0/lib/ruby/gems/3.2.0/gems/shopify_api-13.0.0/lib/shopify_api/webhooks/registry.rb:154:in `get_webhook_id'

Since the app is now running on gem version 21.5 I've expected not to receive this exception anymore - so I wanted to ask if there is anything wrong that you, or anyone else, can see in the steps to reproduce that I've listed above? And if the fix works for anyone else for existing Shop records?

I would be really grateful for help with this one!


  • shopify_api version: 13.0.0
  • shopify_app version: 21.5
  • Ruby version: 3.2.0
  • Operating system: MacOS

@schonert
Copy link

Experiencing the same while upgrading. Could we get some guidance on how to proceed?

Shopify/shopify-api-ruby#1123 (comment)

@gcaprio
Copy link
Contributor

gcaprio commented Jul 17, 2023

Same here. Any guidance on what to do here?

@tomriley
Copy link

Shopify docs state that mandatory webhooks should not be subscribed to via the API. I have submitted a pull request to remove them from generator templates and also fixed the recreation of webhooks in #recreate_webhooks!

Also relates to Shopify/shopify-api-ruby#1123 and #1659

@tomriley
Copy link

Actually, I think the issue is with shopify_app gem treating mandatory webhooks like any other webhook. I think they need to be treated separately as they are not API subscribable, must be (afaik) delivered over HTTP, and are global in scope, not per-install. It shouldn't be required to list them in config.webhooks but they should still be dispatched to a supplied Job class.

@nelsonwittwer
Copy link
Contributor Author

It seems there is an issue with mandatory webhooks. I've created an issue with @zkusznir description and steps to reproduce that we will investigate.

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.

7 participants