refactor where mandatory webhooks are skipped #1249
Conversation
lib/shopify_api/webhooks/registry.rb
Outdated
| params(topic: String).returns(RegisterResult) | ||
| end | ||
| def mandatory_registration_result(topic) | ||
| RegisterResult.new(topic: topic, success: true, body: nil) |
There was a problem hiding this comment.
Would this be confusing to return that the mandatory webhooks are successfully registered, when they have they have not actually been registered? i.e. will this further confuse people into thinking they can register these webhook topics via the API?
There was a problem hiding this comment.
Due to an unfortunate coupling of this registry and the async job processor, we still need to add mandatory topics to the @registry. You and I had the same thought, and in my last PR that caused this bug used a similar approach to the one you are suggesting.
We'll still need to add mandatory hooks to the @registry even if we don't explicitly register to them with the API. This is a gross boundary and I'd love to explore streamlining webhook registrations so we don't have to have this silly flow :(
There was a problem hiding this comment.
I get that we need to add them to the registry currently. But do we need to return that we successfully registered them?
Could we skip pushing them to the array of topics we have registered? Or is there a way to return that they have not been successfully registered?
There was a problem hiding this comment.
Or do we think removing them from the response of registered topics would break something else?
nelsonwittwer
changed the title
* refactor where mandatory webhooks are skipped * changelog * more explicit mandatory registration result * lint
Description
Fixes #1248
We need to still skip registration/unregistration of mandatory webhooks since they are handled in the partner dashboard, but we need to do so in a way that doesn't break the handler.
How has this been tested?
🎩 testing performed for this fix:
Tested on a test app
Checklist: