Fix: ShopifyAPI::Webhooks::Registry doesn't update webhooks if metafield_namespaces has changed

[andy-liuu]

TODO:

• missing edgecase: arrays have same contents but different orders
• missing edgecase: nil and empty array check should be good
• make solution more ruby-like?
• try to reduce duplicate code?
• add more unit tests to not update when nothing changes
• tophat
• changelog

Description

Fixes #1323

What is the problem it is solving?

• Previously, the ruby api would only update webhooks if the topic already existed but the callback url / endpoint was different. It did not consider the scenario where the same topic and callback url were supplied, but the fields or metafield namespaces were updated. I chose to only check fields and metafield namespaces because those are the only attributes accepted by the ruby api (despite the actual gql object having other (deprecated) fields.

What is the context of these changes?

• refactor the existing unit tests so that the shared functions are more flexibly called via adding new parameters
  • Now, each test defines it's own expected queries, responses and values, instead of having an if statement in the helper function to deal with it
• remove unused code in the unit testing file
• write tests and add to existing ones for ...
  • having an update mutation called when just path, fields, or metafieldNamespaces is changed
  • adding a new webhook with each of these params
  • not updating when an existing identical webhook exists
  • & hardcode expected queries in the testing helper file
• update the check for existing webhook query for each of http, pubsub, and eventbridge
• update the parsing method for each of http, pubsub, eventbridge
  • pulling shared code into registration superclass. allowing subclass to implement a specific detail
• update the logic in webhook_registration_needed to check for diffs in fields and metafieldNamespaces

What is the impact of this PR?

Fixing #1323

How has this been tested?

• Made new unit tests and strengthened existing tests to account for the new scenarios
  • the previous tests were running the exact same "update path" test regardless of parameters in do_register_test.

Checklist:

• My commit message follow the pattern described in here
• I have performed a self-review of my own code.
• I have added tests that prove my fix is effective or that my feature works.
• I have updated the project documentation.
• I have added a changelog line.

[andy-liuu]

In ruby unit tests, is it better to have code be copypaste-ish (like right now)? Or better to introduce logic to make it more elegant? e.g. looping thru each of [http, pubsub, eventbridge], helpers to build the queries we assert on

Pros:

• easier to tell if a specific scenario fails vs one of many scenarios
• less mental overhead (less likely that test logic is buggy and not the actual code)

Cons:

• More code to maintain if these tests become outdated
• looks dumb

[sle-c]

i think these test cases are a little over abstracted. There are a few things I'd recommend to do:

• Rename webhook_registration_queries.rb to match the class defined in it. I know this has nothing to do with this PR but it'll help with readability of this file.
• I agree with using array to loop through similar test cases

[:http, :pubsub, :eventbridge].each do |protocol|
  define_method("test_#{protocol}_registration_is_not_needed_if_identical_webhook_exists") do
    # setup helpers
    # execution
    # assertions
  end
end

• Instead of giant do_.... methods, can we break them down to multiple smaller helpers?

[andy-liuu]

made the following refactors:

• rename webhook_registration_queries.rb to registry_test_queries.rb
• broke apart do_registration_test megafunction to setup_queries_and_responses helper, add_and_register_webhook helper, and assertion statements
• looping thru http, pub_sub, event_bridge (and their valid address formats) to define similar unit tests for those scenarios
  • also got rid of do_registration_check_error_test because there was only one instance of it being called after this
• consolidated redundant tests

could you take a 2nd lookthru? thanks!

[sle-c]

beautiful! Thank you 💯