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 webhook triggered on Order Cycle Open #9687

Merged

Commits on Mar 7, 2023

  1. Add Faraday for making HTTP requests [add gem]

    It's the most popular and flexible option, so should be able to cater for our future needs best.
    dacook committed Mar 7, 2023
    Configuration menu
    Copy the full SHA
    718ac0a View commit details
    Browse the repository at this point in the history
  2. 1. Add WebhookDeliveryJob

    This job is responsible for delivering a payload for one webhook event only. It allows the action to run asynchronously (and not slow down the calling process).
    dacook committed Mar 7, 2023
    Configuration menu
    Copy the full SHA
    9d19f37 View commit details
    Browse the repository at this point in the history
  3. Raise error on server error

    And thus retry later.
    I tried to test that it actually retries, or ensuring the job remained in the queue to be retried, but couldn't get it to work.
    dacook committed Mar 7, 2023
    Configuration menu
    Copy the full SHA
    9741935 View commit details
    Browse the repository at this point in the history
  4. Prevent webhooks to private addresses (SSRF) [add gem]

    Best reviewed with whitespace hidden.
    
    Unfortunately the spec isn't allowed in CI. But it worked on my environment, I promise.
    I chose `xit` so that it doesn't run unnecessarily. Perhaps we could use `pending` instead, which would execute, and notify us if it suddenly started working one day. But I doubt it.
    dacook committed Mar 7, 2023
    Configuration menu
    Copy the full SHA
    de95465 View commit details
    Browse the repository at this point in the history
  5. 2. Add model WebhookEndpoint [migration]

    This will store the URL for each user that wants a notification.
    
    We probably don't need URL validation (it's not done on Enterprise for example). It could be validated by browser input, and anyway will be validated if the webhook actually works or not.
    
    Inspired by Keygen: https://keygen.sh/blog/how-to-build-a-webhook-system-in-rails-using-sidekiq/
    dacook committed Mar 7, 2023
    Configuration menu
    Copy the full SHA
    85c98c6 View commit details
    Browse the repository at this point in the history
  6. User may have many WebhookEndpoints [migration]

    Although we won't be allowing multiple in the this PR, we certainly plan to in the future.
    
    The migration helper add_reference couldn't handle the custom column name, so I had to put it together manually.
    dacook committed Mar 7, 2023
    Configuration menu
    Copy the full SHA
    778baba View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    ba152f1 View commit details
    Browse the repository at this point in the history
  8. Also send webhook payloads for distributor owners

    But not supplier owners.
    dacook committed Mar 7, 2023
    Configuration menu
    Copy the full SHA
    b91cabc View commit details
    Browse the repository at this point in the history
  9. Configuration menu
    Copy the full SHA
    739df4b View commit details
    Browse the repository at this point in the history
  10. Prevent creating duplicate webhook notifications [migration]

    Using the clever concurrency testing borrowed from SubscriptionPlacementJob, but I thought a shorter pause time (just 100ms) would be sufficient.
    
    I considered doing this with a new 'state' field (upcoming/open/close), but decided to keep it simple.
    dacook committed Mar 7, 2023
    Configuration menu
    Copy the full SHA
    3d81a6e View commit details
    Browse the repository at this point in the history
  11. 6. Add webhook endpoints to user developer settings screen

    Allowing creation and deleting via the user association.
    It probably won't be much effort to allow editing and multiple records, but I cut it down to the minimum needed to avoid any further delays.
    
    I couldn't find a way to test a failure in the destroy method, but decided to keep the condition because I thought it was worth having.
    dacook committed Mar 7, 2023
    Configuration menu
    Copy the full SHA
    00a823b View commit details
    Browse the repository at this point in the history
  12. Apply suggestions from code review

    Co-authored-by: Maikel <[email protected]>
    dacook and mkllnk committed Mar 7, 2023
    Configuration menu
    Copy the full SHA
    9d5ca22 View commit details
    Browse the repository at this point in the history

Commits on Mar 15, 2023

  1. Tidy up spec

    The best way to check if something changed or not, is with 'change' of course.
    dacook committed Mar 15, 2023
    Configuration menu
    Copy the full SHA
    d59074d View commit details
    Browse the repository at this point in the history