From 718ac0ab80f4e7379b02109e0339cc893d079504 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 16 Sep 2022 09:56:00 +1000
Subject: [PATCH 01/13] 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.
---
 Gemfile      | 2 ++
 Gemfile.lock | 1 +
 2 files changed, 3 insertions(+)

diff --git a/Gemfile b/Gemfile
index 3d8e485e56e..c38020d362c 100644
--- a/Gemfile
+++ b/Gemfile
@@ -136,6 +136,8 @@ gem 'view_component_reflex', '3.1.14.pre9'
 
 gem 'mini_portile2', '~> 2.8'
 
+gem "faraday"
+
 group :production, :staging do
   gem 'ddtrace'
   gem 'rack-timeout'
diff --git a/Gemfile.lock b/Gemfile.lock
index 9a1b5b2bb91..917aa0ee931 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -801,6 +801,7 @@ DEPENDENCIES
   digest
   dotenv-rails
   factory_bot_rails (= 6.2.0)
+  faraday
   ffaker
   flipper
   flipper-active_record

From 9d19f37fecd6e973a3eb8c5efead795011cbe779 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 15 Sep 2022 21:41:05 +1000
Subject: [PATCH 02/13] 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).
---
 app/jobs/webhook_delivery_job.rb       | 31 +++++++++++++++++
 spec/jobs/webhook_delivery_job_spec.rb | 47 ++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)
 create mode 100644 app/jobs/webhook_delivery_job.rb
 create mode 100644 spec/jobs/webhook_delivery_job_spec.rb

diff --git a/app/jobs/webhook_delivery_job.rb b/app/jobs/webhook_delivery_job.rb
new file mode 100644
index 00000000000..c92a8569667
--- /dev/null
+++ b/app/jobs/webhook_delivery_job.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+require "faraday"
+
+# Deliver a webhook payload
+# As a delayed job, it can run asynchronously and handle retries.
+class WebhookDeliveryJob < ApplicationJob
+  queue_as :default
+
+  def perform(url, event, payload)
+    body = {
+      id: job_id,
+      at: Time.zone.now.to_s,
+      event: event,
+      data: payload,
+    }
+
+    notify_endpoint(url, body)
+  end
+
+  def notify_endpoint(url, body)
+    connection = Faraday.new(
+      request: { timeout: 30 },
+      headers: {
+        'User-Agent' => 'openfoodnetwork_webhook/1.0',
+        'Content-Type' => 'application/json',
+      }
+    )
+    connection.post(url, body.to_json)
+  end
+end
diff --git a/spec/jobs/webhook_delivery_job_spec.rb b/spec/jobs/webhook_delivery_job_spec.rb
new file mode 100644
index 00000000000..7f1525d56ed
--- /dev/null
+++ b/spec/jobs/webhook_delivery_job_spec.rb
@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe WebhookDeliveryJob do
+  subject { WebhookDeliveryJob.new(url, event, data) }
+  let(:url) { 'https://test/endpoint' }
+  let(:event) { 'order_cycle.opened' }
+  let(:data) {
+    {
+      order_cycle_id: 123, name: "Order cycle 1", open_at: 1.minute.ago.to_s, tags: ["tag1", "tag2"]
+    }
+  }
+
+  before do
+    stub_request(:post, url)
+  end
+
+  it "sends a request to specified url" do
+    subject.perform_now
+    expect(a_request(:post, url)).to have_been_made.once
+  end
+
+  it "delivers a payload" do
+    Timecop.freeze(Time.zone.now) do
+      expected_body = {
+        id: /.+/,
+        at: Time.zone.now.to_s,
+        event: event,
+        data: data,
+      }
+
+      subject.perform_now
+      expect(a_request(:post, url).with(body: expected_body)).
+        to have_been_made.once
+    end
+  end
+
+  # To be implemented in following commits
+  pending "can't access local secrets" # see https://medium.com/in-the-weeds/all-about-paperclips-cve-2017-0889-server-side-request-forgery-ssrf-vulnerability-8cb2b1c96fe8
+
+  describe "retrying" do
+    pending "doesn't retry on internal failure"
+    pending "retries after external failure"
+    pending "stops retrying after a while"
+  end
+end

From 974193595579803c77f3b1338e09d3083219b9c2 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 20 Sep 2022 17:04:47 +1000
Subject: [PATCH 03/13] 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.
---
 app/jobs/webhook_delivery_job.rb       | 10 +++++++++-
 spec/jobs/webhook_delivery_job_spec.rb | 12 ++++++++----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/app/jobs/webhook_delivery_job.rb b/app/jobs/webhook_delivery_job.rb
index c92a8569667..50be1e1daa8 100644
--- a/app/jobs/webhook_delivery_job.rb
+++ b/app/jobs/webhook_delivery_job.rb
@@ -5,6 +5,10 @@
 # Deliver a webhook payload
 # As a delayed job, it can run asynchronously and handle retries.
 class WebhookDeliveryJob < ApplicationJob
+  # General failed request error that we're going to use to signal
+  # the job runner to retry our webhook worker.
+  class FailedWebhookRequestError < StandardError; end
+
   queue_as :default
 
   def perform(url, event, payload)
@@ -26,6 +30,10 @@ def notify_endpoint(url, body)
         'Content-Type' => 'application/json',
       }
     )
-    connection.post(url, body.to_json)
+    response = connection.post(url, body.to_json)
+
+    # Raise a failed request error and let job runner handle retrying.
+    # In theory, only 5xx errors should be retried, but who knows.
+    raise FailedWebhookRequestError, response.status.to_s unless response.success?
   end
 end
diff --git a/spec/jobs/webhook_delivery_job_spec.rb b/spec/jobs/webhook_delivery_job_spec.rb
index 7f1525d56ed..579ed0086bd 100644
--- a/spec/jobs/webhook_delivery_job_spec.rb
+++ b/spec/jobs/webhook_delivery_job_spec.rb
@@ -39,9 +39,13 @@
   # To be implemented in following commits
   pending "can't access local secrets" # see https://medium.com/in-the-weeds/all-about-paperclips-cve-2017-0889-server-side-request-forgery-ssrf-vulnerability-8cb2b1c96fe8
 
-  describe "retrying" do
-    pending "doesn't retry on internal failure"
-    pending "retries after external failure"
-    pending "stops retrying after a while"
+  # Exceptions are considered a job failure, which the job runner
+  # (Sidekiq) and/or ActiveJob will handle and retry later.
+  describe "failure" do
+    it "raises error on server error" do
+      stub_request(:post, url).to_return(status: [500, "Internal Server Error"])
+
+      expect{ subject.perform_now }.to raise_error(StandardError, "500")
+    end
   end
 end

From de9546587a13a3063348e31f9a97e1d997d2df24 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 16 Sep 2022 14:42:34 +1000
Subject: [PATCH 04/13] 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.
---
 Gemfile                                |  1 +
 Gemfile.lock                           |  2 ++
 app/jobs/webhook_delivery_job.rb       | 13 ++++++++-
 spec/jobs/webhook_delivery_job_spec.rb | 40 ++++++++++++++++++++++++--
 4 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/Gemfile b/Gemfile
index c38020d362c..608c8703629 100644
--- a/Gemfile
+++ b/Gemfile
@@ -137,6 +137,7 @@ gem 'view_component_reflex', '3.1.14.pre9'
 gem 'mini_portile2', '~> 2.8'
 
 gem "faraday"
+gem "private_address_check"
 
 group :production, :staging do
   gem 'ddtrace'
diff --git a/Gemfile.lock b/Gemfile.lock
index 917aa0ee931..caa5a208cb5 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -474,6 +474,7 @@ GEM
       ttfunk
     pg (1.2.3)
     power_assert (2.0.2)
+    private_address_check (0.5.0)
     pry (0.13.1)
       coderay (~> 1.1)
       method_source (~> 1.0)
@@ -844,6 +845,7 @@ DEPENDENCIES
   paypal-sdk-merchant (= 1.117.2)
   pdf-reader
   pg (~> 1.2.3)
+  private_address_check
   pry (~> 0.13.0)
   puma
   rack-mini-profiler (< 3.0.0)
diff --git a/app/jobs/webhook_delivery_job.rb b/app/jobs/webhook_delivery_job.rb
index 50be1e1daa8..7eb0a44b832 100644
--- a/app/jobs/webhook_delivery_job.rb
+++ b/app/jobs/webhook_delivery_job.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 require "faraday"
+require "private_address_check"
+require "private_address_check/tcpsocket_ext"
 
 # Deliver a webhook payload
 # As a delayed job, it can run asynchronously and handle retries.
@@ -19,7 +21,16 @@ def perform(url, event, payload)
       data: payload,
     }
 
-    notify_endpoint(url, body)
+    # Request user-submitted url, preventing any private connections being made
+    # (SSRF).
+    # This method may allow the socket to open, but is necessary in order to
+    # protect from TOC/TOU.
+    # Note that private_address_check provides some methods for pre-validating,
+    # but they're not as comprehensive and so unnecessary here. Simply
+    # momentarily opening sockets probably can't cause DoS or other damage.
+    PrivateAddressCheck.only_public_connections do
+      notify_endpoint(url, body)
+    end
   end
 
   def notify_endpoint(url, body)
diff --git a/spec/jobs/webhook_delivery_job_spec.rb b/spec/jobs/webhook_delivery_job_spec.rb
index 579ed0086bd..8f90c3c4e50 100644
--- a/spec/jobs/webhook_delivery_job_spec.rb
+++ b/spec/jobs/webhook_delivery_job_spec.rb
@@ -36,8 +36,44 @@
     end
   end
 
-  # To be implemented in following commits
-  pending "can't access local secrets" # see https://medium.com/in-the-weeds/all-about-paperclips-cve-2017-0889-server-side-request-forgery-ssrf-vulnerability-8cb2b1c96fe8
+  # Ensure responses from a local network aren't allowed, to prevent a user
+  # seeing a private response or initiating an unauthorised action (SSRF).
+  # Currently, we're not doing anything with responses. When we do, we should
+  # update this to confirm the response isn't exposed.
+  describe "server side request forgery" do
+    describe "private addresses" do
+      private_addresses = [
+        "http://127.0.0.1/all_the_secrets",
+        "http://localhost/all_the_secrets",
+      ]
+
+      private_addresses.each do |url|
+        # Unfortunately this isn't allowed in CI, but it should work in your
+        # development environment.
+        xit "rejects private address #{url}" do
+          expect {
+            WebhookDeliveryJob.perform_now(url, event, data)
+          }.to raise_error(PrivateAddressCheck::PrivateConnectionAttemptedError)
+        end
+      end
+    end
+
+    describe "redirects" do
+      it "doesn't follow a redirect" do
+        other_url = 'http://localhost/all_the_secrets'
+
+        stub_request(:post, url).
+          to_return(status: 302, headers: { 'Location' => other_url })
+        stub_request(:any, other_url)
+
+        expect {
+          subject.perform_now
+        }.to raise_error(StandardError, "302")
+
+        expect(a_request(:any, other_url)).not_to have_been_made
+      end
+    end
+  end
 
   # Exceptions are considered a job failure, which the job runner
   # (Sidekiq) and/or ActiveJob will handle and retry later.

From 85c98c6d3e998bdb761fc6c36032fac426cfaf0d Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 16 Sep 2022 15:16:27 +1000
Subject: [PATCH 05/13] 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/
---
 app/models/webhook_endpoint.rb                        |  6 ++++++
 db/migrate/20221028051650_create_webhook_endpoints.rb | 11 +++++++++++
 db/schema.rb                                          |  6 ++++++
 spec/models/webhook_endpoint_spec.rb                  |  9 +++++++++
 4 files changed, 32 insertions(+)
 create mode 100644 app/models/webhook_endpoint.rb
 create mode 100644 db/migrate/20221028051650_create_webhook_endpoints.rb
 create mode 100644 spec/models/webhook_endpoint_spec.rb

diff --git a/app/models/webhook_endpoint.rb b/app/models/webhook_endpoint.rb
new file mode 100644
index 00000000000..8626e11b934
--- /dev/null
+++ b/app/models/webhook_endpoint.rb
@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+# Records a webhook url to send notifications to
+class WebhookEndpoint < ApplicationRecord
+  validates :url, presence: true
+end
diff --git a/db/migrate/20221028051650_create_webhook_endpoints.rb b/db/migrate/20221028051650_create_webhook_endpoints.rb
new file mode 100644
index 00000000000..034d1141d2f
--- /dev/null
+++ b/db/migrate/20221028051650_create_webhook_endpoints.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class CreateWebhookEndpoints < ActiveRecord::Migration[6.1]
+  def change
+    create_table :webhook_endpoints do |t|
+      t.string :url, null: false
+
+      t.timestamps null: false
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 77975612f5d..d28ee2ba800 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1189,6 +1189,12 @@
     t.index ["item_type", "item_id"], name: "index_versions_on_item_type_and_item_id"
   end
 
+  create_table "webhook_endpoints", force: :cascade do |t|
+    t.string "url", null: false
+    t.datetime "created_at", precision: 6, null: false
+    t.datetime "updated_at", precision: 6, null: false
+  end
+
   add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id"
   add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id"
   add_foreign_key "adjustment_metadata", "enterprises", name: "adjustment_metadata_enterprise_id_fk"
diff --git a/spec/models/webhook_endpoint_spec.rb b/spec/models/webhook_endpoint_spec.rb
new file mode 100644
index 00000000000..5f1d630f099
--- /dev/null
+++ b/spec/models/webhook_endpoint_spec.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe WebhookEndpoint, type: :model do
+  describe "validations" do
+    it { is_expected.to validate_presence_of(:url) }
+  end
+end

From 778baba118e90b2bc1ab0985ff62979873526763 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 28 Oct 2022 17:00:52 +1100
Subject: [PATCH 06/13] 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.
---
 app/models/spree/user.rb                                 | 2 ++
 ...53214_add_spree_user_reference_to_webhook_endpoint.rb | 9 +++++++++
 db/schema.rb                                             | 3 +++
 spec/models/spree/user_spec.rb                           | 1 +
 4 files changed, 15 insertions(+)
 create mode 100644 db/migrate/20221028053214_add_spree_user_reference_to_webhook_endpoint.rb

diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb
index e95df0fcfd5..599815c055c 100644
--- a/app/models/spree/user.rb
+++ b/app/models/spree/user.rb
@@ -38,6 +38,8 @@ class User < ApplicationRecord
     has_many :customers
     has_many :credit_cards
     has_many :report_rendering_options, class_name: "::ReportRenderingOptions", dependent: :destroy
+    has_many :webhook_endpoints, dependent: :destroy
+
     accepts_nested_attributes_for :enterprise_roles, allow_destroy: true
 
     accepts_nested_attributes_for :bill_address
diff --git a/db/migrate/20221028053214_add_spree_user_reference_to_webhook_endpoint.rb b/db/migrate/20221028053214_add_spree_user_reference_to_webhook_endpoint.rb
new file mode 100644
index 00000000000..462e978b9c1
--- /dev/null
+++ b/db/migrate/20221028053214_add_spree_user_reference_to_webhook_endpoint.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+class AddSpreeUserReferenceToWebhookEndpoint < ActiveRecord::Migration[6.1]
+  def change
+    add_column :webhook_endpoints, :user_id, :bigint, default: 0, null: false
+    add_index :webhook_endpoints, :user_id
+    add_foreign_key :webhook_endpoints, :spree_users, column: :user_id
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index d28ee2ba800..551c5c2e3ac 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1193,6 +1193,8 @@
     t.string "url", null: false
     t.datetime "created_at", precision: 6, null: false
     t.datetime "updated_at", precision: 6, null: false
+    t.bigint "user_id", default: 0, null: false
+    t.index ["user_id"], name: "index_webhook_endpoints_on_user_id"
   end
 
   add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id"
@@ -1299,4 +1301,5 @@
   add_foreign_key "subscriptions", "spree_shipping_methods", column: "shipping_method_id", name: "subscriptions_shipping_method_id_fk"
   add_foreign_key "variant_overrides", "enterprises", column: "hub_id", name: "variant_overrides_hub_id_fk"
   add_foreign_key "variant_overrides", "spree_variants", column: "variant_id", name: "variant_overrides_variant_id_fk"
+  add_foreign_key "webhook_endpoints", "spree_users", column: "user_id"
 end
diff --git a/spec/models/spree/user_spec.rb b/spec/models/spree/user_spec.rb
index a99a18941dc..9dac88ad760 100644
--- a/spec/models/spree/user_spec.rb
+++ b/spec/models/spree/user_spec.rb
@@ -7,6 +7,7 @@
 
   describe "associations" do
     it { is_expected.to have_many(:owned_enterprises) }
+    it { is_expected.to have_many(:webhook_endpoints).dependent(:destroy) }
 
     describe "addresses" do
       let(:user) { create(:user, bill_address: create(:address)) }

From ba152f12ee76d9d7f838117bff5a16adef43684a Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 7 Oct 2022 12:22:03 +1100
Subject: [PATCH 07/13] 3. Add OrderCycleWebhookService to create webhook
 payloads for an order cycle event

---
 app/services/order_cycle_webhook_service.rb   | 18 +++++
 .../order_cycle_webhook_service_spec.rb       | 75 +++++++++++++++++++
 2 files changed, 93 insertions(+)
 create mode 100644 app/services/order_cycle_webhook_service.rb
 create mode 100644 spec/services/order_cycle_webhook_service_spec.rb

diff --git a/app/services/order_cycle_webhook_service.rb b/app/services/order_cycle_webhook_service.rb
new file mode 100644
index 00000000000..376eb7980aa
--- /dev/null
+++ b/app/services/order_cycle_webhook_service.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+# Create a webhook payload for an order cycle event.
+# The payload will be delivered asynchronously.
+class OrderCycleWebhookService
+  def self.create_webhook_job(order_cycle, event)
+    webhook_payload = order_cycle
+      .slice(:id, :name, :orders_open_at, :orders_close_at, :coordinator_id)
+      .merge(coordinator_name: order_cycle.coordinator.name)
+
+    # Endpoints for coordinator owner
+    webhook_endpoints = order_cycle.coordinator.owner.webhook_endpoints
+
+    webhook_endpoints.each do |endpoint|
+      WebhookDeliveryJob.perform_later(endpoint.url, event, webhook_payload)
+    end
+  end
+end
diff --git a/spec/services/order_cycle_webhook_service_spec.rb b/spec/services/order_cycle_webhook_service_spec.rb
new file mode 100644
index 00000000000..a2b29f65d4c
--- /dev/null
+++ b/spec/services/order_cycle_webhook_service_spec.rb
@@ -0,0 +1,75 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe OrderCycleWebhookService do
+  let(:order_cycle) {
+    create(
+      :simple_order_cycle,
+      name: "Order cycle 1",
+      orders_open_at: "2022-09-19 09:00:00".to_time,
+      orders_close_at: "2022-09-19 17:00:00".to_time,
+      coordinator: coordinator,
+    )
+  }
+  let(:coordinator) { create :distributor_enterprise, name: "Starship Enterprise" }
+
+  describe "creating payloads" do
+    it "doesn't create webhook payload for enterprise users" do
+      # The co-ordinating enterprise has a non-owner user with an endpoint.
+      # They shouldn't receive a notification.
+      coordinator_user = create(:user, enterprises: [coordinator])
+      coordinator_user.webhook_endpoints.create!(url: "http://coordinator_user_url")
+
+      expect{ OrderCycleWebhookService.create_webhook_job(order_cycle, "order_cycle.opened") }
+        .to_not enqueue_job(WebhookDeliveryJob).with("http://coordinator_user_url", any_args)
+    end
+
+    context "coordinator owner has endpoint configured" do
+      before do
+        coordinator.owner.webhook_endpoints.create! url: "http://coordinator_owner_url"
+      end
+
+      it "creates webhook payload for order cycle coordinator" do
+        expect{ OrderCycleWebhookService.create_webhook_job(order_cycle, "order_cycle.opened") }
+          .to enqueue_job(WebhookDeliveryJob).with("http://coordinator_owner_url", any_args)
+      end
+
+      it "creates webhook payload with details for the specified order cycle only" do
+        # The coordinating enterprise has another OC. It should be ignored.
+        order_cycle.dup.save
+
+        data = {
+          id: order_cycle.id,
+          name: "Order cycle 1",
+          orders_open_at: "2022-09-19 09:00:00".to_time,
+          orders_close_at: "2022-09-19 17:00:00".to_time,
+          coordinator_id: coordinator.id,
+          coordinator_name: "Starship Enterprise",
+        }
+
+        expect{ OrderCycleWebhookService.create_webhook_job(order_cycle, "order_cycle.opened") }
+          .to enqueue_job(WebhookDeliveryJob).exactly(1).times
+          .with("http://coordinator_owner_url", "order_cycle.opened", hash_including(data))
+      end
+    end
+
+    context "coordinator owner doesn't have endpoint configured" do
+      it "doesn't create webhook payload" do
+        expect{ OrderCycleWebhookService.create_webhook_job(order_cycle, "order_cycle.opened") }
+          .not_to enqueue_job(WebhookDeliveryJob)
+      end
+    end
+
+    pending "creates webhook payload for order cycle distributor"
+
+    pending "doesn't create webhook payload for order cycle supplier"
+  end
+
+  context "without webhook subscribed to enterprise" do
+    it "doesn't create webhook payload" do
+      expect{ OrderCycleWebhookService.create_webhook_job(order_cycle, "order_cycle.opened") }
+        .not_to enqueue_job(WebhookDeliveryJob)
+    end
+  end
+end

From b91cabc510fbd57666f41e299fccaa8387642940 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 16 Feb 2023 11:13:08 +1100
Subject: [PATCH 08/13] Also send webhook payloads for distributor owners

But not supplier owners.
---
 app/services/order_cycle_webhook_service.rb   |  3 +
 .../order_cycle_webhook_service_spec.rb       | 72 ++++++++++++++++++-
 2 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/app/services/order_cycle_webhook_service.rb b/app/services/order_cycle_webhook_service.rb
index 376eb7980aa..f2d4df152ae 100644
--- a/app/services/order_cycle_webhook_service.rb
+++ b/app/services/order_cycle_webhook_service.rb
@@ -11,6 +11,9 @@ def self.create_webhook_job(order_cycle, event)
     # Endpoints for coordinator owner
     webhook_endpoints = order_cycle.coordinator.owner.webhook_endpoints
 
+    # Plus unique endpoints for distributor owners (ignore duplicates)
+    webhook_endpoints |= order_cycle.distributors.map(&:owner).flat_map(&:webhook_endpoints)
+
     webhook_endpoints.each do |endpoint|
       WebhookDeliveryJob.perform_later(endpoint.url, event, webhook_payload)
     end
diff --git a/spec/services/order_cycle_webhook_service_spec.rb b/spec/services/order_cycle_webhook_service_spec.rb
index a2b29f65d4c..e5c1285e2be 100644
--- a/spec/services/order_cycle_webhook_service_spec.rb
+++ b/spec/services/order_cycle_webhook_service_spec.rb
@@ -61,9 +61,77 @@
       end
     end
 
-    pending "creates webhook payload for order cycle distributor"
+    describe "distributors" do
+      context "multiple distributors have owners with endpoint configured" do
+        let(:order_cycle) {
+          create(
+            :simple_order_cycle,
+            coordinator: coordinator,
+            distributors: two_distributors,
+          )
+        }
+        let(:two_distributors) {
+          (1..2).map do |i|
+            user = create(:user)
+            user.webhook_endpoints.create!(url: "http://distributor#{i}_owner_url")
+            create(:distributor_enterprise, owner: user)
+          end
+        }
+
+        it "creates webhook payload for each order cycle distributor" do
+          data = {
+            coordinator_id: order_cycle.coordinator_id,
+            coordinator_name: "Starship Enterprise",
+          }
+
+          expect{ OrderCycleWebhookService.create_webhook_job(order_cycle, "order_cycle.opened") }
+            .to enqueue_job(WebhookDeliveryJob).with("http://distributor1_owner_url",
+                                                     "order_cycle.opened", hash_including(data))
+            .and enqueue_job(WebhookDeliveryJob).with("http://distributor2_owner_url",
+                                                      "order_cycle.opened", hash_including(data))
+        end
+      end
+
+      context "distributor owner is same user as coordinator owner" do
+        let(:user) { coordinator.owner }
+        let(:order_cycle) {
+          create(
+            :simple_order_cycle,
+            coordinator: coordinator,
+            distributors: [create(:distributor_enterprise, owner: user)],
+          )
+        }
+
+        it "creates only one webhook payload for the user's endpoint" do
+          user.webhook_endpoints.create! url: "http://coordinator_owner_url"
 
-    pending "doesn't create webhook payload for order cycle supplier"
+          expect{ OrderCycleWebhookService.create_webhook_job(order_cycle, "order_cycle.opened") }
+            .to enqueue_job(WebhookDeliveryJob).with("http://coordinator_owner_url", any_args)
+        end
+      end
+    end
+
+    describe "suppliers" do
+      context "supplier has owner with endpoint configured" do
+        let(:order_cycle) {
+          create(
+            :simple_order_cycle,
+            coordinator: coordinator,
+            suppliers: [supplier],
+          )
+        }
+        let(:supplier) {
+          user = create(:user)
+          user.webhook_endpoints.create!(url: "http://supplier_owner_url")
+          create(:supplier_enterprise, owner: user)
+        }
+
+        it "doesn't create a webhook payload for supplier owner" do
+          expect{ OrderCycleWebhookService.create_webhook_job(order_cycle, "order_cycle.opened") }
+            .to_not enqueue_job(WebhookDeliveryJob).with("http://supplier_owner_url", any_args)
+        end
+      end
+    end
   end
 
   context "without webhook subscribed to enterprise" do

From 739df4be014a99d8cf2694ecca69d24341111176 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 4 Nov 2022 16:43:27 +1100
Subject: [PATCH 09/13] 4. OrderCycleOpenedJob triggers webhook

---
 app/jobs/order_cycle_opened_job.rb       | 17 ++++++++++++++
 config/sidekiq.yml                       |  2 ++
 spec/jobs/order_cycle_opened_job_spec.rb | 30 ++++++++++++++++++++++++
 3 files changed, 49 insertions(+)
 create mode 100644 app/jobs/order_cycle_opened_job.rb
 create mode 100644 spec/jobs/order_cycle_opened_job_spec.rb

diff --git a/app/jobs/order_cycle_opened_job.rb b/app/jobs/order_cycle_opened_job.rb
new file mode 100644
index 00000000000..0b9dfceb447
--- /dev/null
+++ b/app/jobs/order_cycle_opened_job.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+# Trigger jobs for any order cycles that recently opened
+class OrderCycleOpenedJob < ApplicationJob
+  def perform
+    recently_opened_order_cycles.each do |order_cycle|
+      OrderCycleWebhookService.create_webhook_job(order_cycle, 'order_cycle.opened')
+    end
+  end
+
+  private
+
+  def recently_opened_order_cycles
+    OrderCycle
+      .where(orders_open_at: 1.hour.ago..Time.zone.now)
+  end
+end
diff --git a/config/sidekiq.yml b/config/sidekiq.yml
index c58aca0333c..eab2b061dbc 100644
--- a/config/sidekiq.yml
+++ b/config/sidekiq.yml
@@ -15,5 +15,7 @@
       every: "5m"
     SubscriptionConfirmJob:
       every: "5m"
+    OrderCycleOpenedJob:
+      every: "5m"
     OrderCycleClosingJob:
       every: "5m"
diff --git a/spec/jobs/order_cycle_opened_job_spec.rb b/spec/jobs/order_cycle_opened_job_spec.rb
new file mode 100644
index 00000000000..b36a18e94e8
--- /dev/null
+++ b/spec/jobs/order_cycle_opened_job_spec.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe OrderCycleOpenedJob do
+  let(:oc_opened_before) {
+    create(:order_cycle, orders_open_at: Time.zone.now - 1.hour)
+  }
+  let(:oc_opened_now) {
+    create(:order_cycle, orders_open_at: Time.zone.now)
+  }
+  let(:oc_opening_soon) {
+    create(:order_cycle, orders_open_at: Time.zone.now + 1.minute)
+  }
+
+  it "enqueues jobs for recently opened order cycles only" do
+    expect(OrderCycleWebhookService)
+      .to receive(:create_webhook_job).with(oc_opened_now, 'order_cycle.opened')
+
+    expect(OrderCycleWebhookService)
+      .to_not receive(:create_webhook_job).with(oc_opened_before, 'order_cycle.opened')
+
+    expect(OrderCycleWebhookService)
+      .to_not receive(:create_webhook_job).with(oc_opening_soon, 'order_cycle.opened')
+
+    OrderCycleOpenedJob.perform_now
+  end
+
+  pending "doesn't trigger jobs open more than once"
+end

From 3d81a6e2807423cfe256425774380b01c8749fc5 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Mon, 10 Oct 2022 12:42:03 +1100
Subject: [PATCH 10/13] 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.
---
 app/jobs/order_cycle_opened_job.rb            | 16 +++++++--
 app/models/order_cycle.rb                     |  9 +++++
 ...0919063831_add_opened_at_to_order_cycle.rb |  7 ++++
 db/schema.rb                                  |  1 +
 spec/jobs/order_cycle_opened_job_spec.rb      | 34 ++++++++++++++++++-
 spec/models/order_cycle_spec.rb               | 28 +++++++++++++--
 6 files changed, 89 insertions(+), 6 deletions(-)
 create mode 100644 db/migrate/20220919063831_add_opened_at_to_order_cycle.rb

diff --git a/app/jobs/order_cycle_opened_job.rb b/app/jobs/order_cycle_opened_job.rb
index 0b9dfceb447..9d84027fe30 100644
--- a/app/jobs/order_cycle_opened_job.rb
+++ b/app/jobs/order_cycle_opened_job.rb
@@ -3,15 +3,25 @@
 # Trigger jobs for any order cycles that recently opened
 class OrderCycleOpenedJob < ApplicationJob
   def perform
-    recently_opened_order_cycles.each do |order_cycle|
-      OrderCycleWebhookService.create_webhook_job(order_cycle, 'order_cycle.opened')
+    ActiveRecord::Base.transaction do
+      recently_opened_order_cycles.find_each do |order_cycle|
+        OrderCycleWebhookService.create_webhook_job(order_cycle, 'order_cycle.opened')
+      end
+      mark_as_opened(recently_opened_order_cycles)
     end
   end
 
   private
 
   def recently_opened_order_cycles
-    OrderCycle
+    @recently_opened_order_cycles ||= OrderCycle
+      .where(opened_at: nil)
       .where(orders_open_at: 1.hour.ago..Time.zone.now)
+      .lock.order(:id)
+  end
+
+  def mark_as_opened(order_cycles)
+    now = Time.zone.now
+    order_cycles.update_all(opened_at: now, updated_at: now)
   end
 end
diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb
index ce1846cf540..4eb3bc90608 100644
--- a/app/models/order_cycle.rb
+++ b/app/models/order_cycle.rb
@@ -34,6 +34,7 @@ class OrderCycle < ApplicationRecord
 
   attr_accessor :incoming_exchanges, :outgoing_exchanges
 
+  before_update :reset_opened_at, if: :will_save_change_to_orders_open_at?
   before_update :reset_processed_at, if: :will_save_change_to_orders_close_at?
   after_save :sync_subscriptions, if: :opening?
 
@@ -333,6 +334,14 @@ def orders_close_at_after_orders_open_at?
     errors.add(:orders_close_at, :after_orders_open_at)
   end
 
+  def reset_opened_at
+    # Reset only if order cycle is opening again at a later date
+    return unless orders_open_at.present? && orders_open_at_was.present?
+    return unless orders_open_at > orders_open_at_was
+
+    self.opened_at = nil
+  end
+
   def reset_processed_at
     return unless orders_close_at.present? && orders_close_at_was.present?
     return unless orders_close_at > orders_close_at_was
diff --git a/db/migrate/20220919063831_add_opened_at_to_order_cycle.rb b/db/migrate/20220919063831_add_opened_at_to_order_cycle.rb
new file mode 100644
index 00000000000..802ce193557
--- /dev/null
+++ b/db/migrate/20220919063831_add_opened_at_to_order_cycle.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddOpenedAtToOrderCycle < ActiveRecord::Migration[6.1]
+  def change
+    add_column :order_cycles, :opened_at, :timestamp
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 551c5c2e3ac..8e6da5fe19f 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -310,6 +310,7 @@
     t.datetime "processed_at"
     t.boolean "automatic_notifications", default: false
     t.boolean "mails_sent", default: false
+    t.datetime "opened_at"
   end
 
   create_table "order_cycles_distributor_payment_methods", id: false, force: :cascade do |t|
diff --git a/spec/jobs/order_cycle_opened_job_spec.rb b/spec/jobs/order_cycle_opened_job_spec.rb
index b36a18e94e8..142cc01b0e8 100644
--- a/spec/jobs/order_cycle_opened_job_spec.rb
+++ b/spec/jobs/order_cycle_opened_job_spec.rb
@@ -26,5 +26,37 @@
     OrderCycleOpenedJob.perform_now
   end
 
-  pending "doesn't trigger jobs open more than once"
+  describe "concurrency", concurrency: true do
+    let(:breakpoint) { Mutex.new }
+
+    it "doesn't place duplicate job when run concurrently" do
+      oc_opened_now
+
+      # Pause jobs when placing new job:
+      breakpoint.lock
+      allow(OrderCycleOpenedJob).to(
+        receive(:new).and_wrap_original do |method, *args|
+          breakpoint.synchronize {}
+          method.call(*args)
+        end
+      )
+
+      expect(OrderCycleWebhookService)
+        .to receive(:create_webhook_job).with(oc_opened_now, 'order_cycle.opened').once
+
+      # Start two jobs in parallel:
+      threads = [
+        Thread.new { OrderCycleOpenedJob.perform_now },
+        Thread.new { OrderCycleOpenedJob.perform_now },
+      ]
+
+      # Wait for both to jobs to pause.
+      # This can reveal a race condition.
+      sleep 0.1
+
+      # Resume and complete both jobs:
+      breakpoint.unlock
+      threads.each(&:join)
+    end
+  end
 end
diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb
index 818b900e196..fa9550039fb 100644
--- a/spec/models/order_cycle_spec.rb
+++ b/spec/models/order_cycle_spec.rb
@@ -551,6 +551,30 @@
     end
   end
 
+  describe "opened_at " do
+    let!(:oc) {
+      create(:simple_order_cycle, orders_open_at: 2.days.ago, orders_close_at: 1.day.ago, opened_at: 1.week.ago)
+    }
+
+    it "reset opened_at if open date change in future" do
+      expect(oc.opened_at).to_not be_nil
+      oc.update!(orders_open_at: 1.week.from_now, orders_close_at: 2.weeks.from_now)
+      expect(oc.opened_at).to be_nil
+    end
+
+    it "it does not reset opened_at if open date is changed to be earlier" do
+      expect(oc.opened_at).to_not be_nil
+      oc.update!(orders_open_at: 3.days.ago)
+      expect(oc.opened_at).to_not be_nil
+    end
+
+    it "it does not reset opened_at if open date does not change" do
+      expect(oc.opened_at).to_not be_nil
+      oc.update!(orders_close_at: 1.day.from_now)
+      expect(oc.opened_at).to_not be_nil
+    end
+  end
+
   describe "processed_at " do
     let!(:oc) {
       create(:simple_order_cycle, orders_open_at: 1.week.ago, orders_close_at: 1.day.ago, processed_at: 1.hour.ago)
@@ -562,13 +586,13 @@
       expect(oc.processed_at).to be_nil
     end
 
-    it "it does not reset processed_at if close date change in the past" do
+    it "it does not reset processed_at if close date is changed to be earlier" do
       expect(oc.processed_at).to_not be_nil
       oc.update!(orders_close_at: 2.days.ago)
       expect(oc.processed_at).to_not be_nil
     end
 
-    it "it does not reset processed_at if close date do not change" do
+    it "it does not reset processed_at if close date does not change" do
       expect(oc.processed_at).to_not be_nil
       oc.update!(orders_open_at: 2.weeks.ago)
       expect(oc.processed_at).to_not be_nil

From 00a823b2fc3ddbf689bff00e0219d06522badc75 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 4 Nov 2022 16:32:32 +1100
Subject: [PATCH 11/13] 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.
---
 .../webhook_endpoints_controller.rb           | 47 +++++++++++++
 app/models/spree/user.rb                      |  1 +
 app/services/permitted_attributes/user.rb     |  5 +-
 .../spree/users/_developer_settings.html.haml |  1 +
 .../spree/users/_webhook_endpoints.html.haml  | 33 +++++++++
 config/locales/en.yml                         | 18 +++++
 config/routes/spree.rb                        |  4 +-
 .../webhook_endpoints_controller_spec.rb      | 67 +++++++++++++++++++
 .../account/developer_settings_spec.rb        | 16 +++++
 9 files changed, 190 insertions(+), 2 deletions(-)
 create mode 100644 app/controllers/webhook_endpoints_controller.rb
 create mode 100644 app/views/spree/users/_webhook_endpoints.html.haml
 create mode 100644 spec/controllers/webhook_endpoints_controller_spec.rb

diff --git a/app/controllers/webhook_endpoints_controller.rb b/app/controllers/webhook_endpoints_controller.rb
new file mode 100644
index 00000000000..c9493f42bb4
--- /dev/null
+++ b/app/controllers/webhook_endpoints_controller.rb
@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+class WebhookEndpointsController < ::BaseController
+  before_action :load_resource, only: :destroy
+
+  def create
+    webhook_endpoint = spree_current_user.webhook_endpoints.new(webhook_endpoint_params)
+
+    if webhook_endpoint.save
+      flash[:success] = t('.success')
+    else
+      flash[:error] = t('.error')
+    end
+
+    redirect_to redirect_path
+  end
+
+  def destroy
+    if @webhook_endpoint.destroy
+      flash[:success] = t('.success')
+    else
+      flash[:error] = t('.error')
+    end
+
+    redirect_to redirect_path
+  end
+
+  def load_resource
+    @webhook_endpoint = spree_current_user.webhook_endpoints.find(params[:id])
+  end
+
+  def webhook_endpoint_params
+    params.require(:webhook_endpoint).permit(:url)
+  end
+
+  def redirect_path
+    if request.referer.blank? || request.referer.include?(spree.account_path)
+      developer_settings_path
+    else
+      request.referer
+    end
+  end
+
+  def developer_settings_path
+    "#{spree.account_path}#/developer_settings"
+  end
+end
diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb
index 599815c055c..879825e9baf 100644
--- a/app/models/spree/user.rb
+++ b/app/models/spree/user.rb
@@ -41,6 +41,7 @@ class User < ApplicationRecord
     has_many :webhook_endpoints, dependent: :destroy
 
     accepts_nested_attributes_for :enterprise_roles, allow_destroy: true
+    accepts_nested_attributes_for :webhook_endpoints
 
     accepts_nested_attributes_for :bill_address
     accepts_nested_attributes_for :ship_address
diff --git a/app/services/permitted_attributes/user.rb b/app/services/permitted_attributes/user.rb
index 0b511bab218..4bb29bbcc48 100644
--- a/app/services/permitted_attributes/user.rb
+++ b/app/services/permitted_attributes/user.rb
@@ -15,7 +15,10 @@ def call(extra_permitted_attributes = [])
     private
 
     def permitted_attributes
-      [:email, :password, :password_confirmation, :disabled]
+      [
+        :email, :password, :password_confirmation, :disabled,
+        { webhook_endpoints_attributes: [:id, :url] },
+      ]
     end
   end
 end
diff --git a/app/views/spree/users/_developer_settings.html.haml b/app/views/spree/users/_developer_settings.html.haml
index b98ed527d51..fe6f6c5d046 100644
--- a/app/views/spree/users/_developer_settings.html.haml
+++ b/app/views/spree/users/_developer_settings.html.haml
@@ -1,3 +1,4 @@
 %script{ type: "text/ng-template", id: "account/developer_settings.html" }
   %h3= t('.title')
   = render partial: 'api_keys'
+  = render partial: 'webhook_endpoints'
diff --git a/app/views/spree/users/_webhook_endpoints.html.haml b/app/views/spree/users/_webhook_endpoints.html.haml
new file mode 100644
index 00000000000..8bd60e2f8b4
--- /dev/null
+++ b/app/views/spree/users/_webhook_endpoints.html.haml
@@ -0,0 +1,33 @@
+%section{ id: "webhook_endpoints" }
+  %hr
+  %h3= t('.title')
+  %p= t('.description')
+
+  %table{width: "100%"}
+    %thead
+      %tr
+        %th= t('.event_type.header')
+        %th= t('.url.header')
+        %th.actions
+    %tbody
+      -# Existing endpoints
+      - @user.webhook_endpoints.each do |webhook_endpoint|
+        %tr
+          %td= t('.event_types.order_cycle_opened') # For now, we only support one type.
+          %td= webhook_endpoint.url
+          %td.actions
+            - if webhook_endpoint.persisted?
+              = button_to account_webhook_endpoint_path(webhook_endpoint), method: :delete,
+                class: "tiny alert no-margin",
+                data: { confirm: I18n.t(:are_you_sure)} do
+                = I18n.t(:delete)
+
+      -# Create new
+      - if @user.webhook_endpoints.empty? # Only one allowed for now.
+        %tr
+          %td= t('.event_types.order_cycle_opened') # For now, we only support one type.
+          %td
+            = form_for(@user.webhook_endpoints.build, url: account_webhook_endpoints_path, id: 'new_webhook_endpoint') do |f|
+              = f.url_field :url, placeholder: t('.url.create_placeholder'), required: true, size: 64
+          %td.actions
+            = button_tag t(:create), class: 'button primary tiny no-margin', form: 'new_webhook_endpoint'
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 761411574e9..81e2ace90de 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -3562,6 +3562,14 @@ See the %{link} to find out more about %{sitename}'s features and to start using
   previous: "Previous"
   last: "Last"
 
+  webhook_endpoints:
+    create:
+      success: Webhook endpoint successfully created
+      error: Webhook endpoint failed to create
+    destroy:
+      success: Webhook endpoint successfully deleted
+      error: Webhook endpoint failed to delete
+
   spree:
     order_updated: "Order Updated"
     add_country: "Add country"
@@ -4357,6 +4365,16 @@ See the %{link} to find out more about %{sitename}'s features and to start using
       api_keys:
         regenerate_key: "Regenerate Key"
         title: API key
+      webhook_endpoints:
+        title: Webhook Endpoints
+        description: Events in the system may trigger webhooks to external systems.
+        event_types:
+          order_cycle_opened: Order Cycle Opened
+        event_type:
+          header: Event type
+        url:
+          header: Endpoint URL
+          create_placeholder: Enter the URL of the remote webhook endpoint
       developer_settings:
         title: Developer Settings
       form:
diff --git a/config/routes/spree.rb b/config/routes/spree.rb
index c658cbdc214..9844b800e50 100644
--- a/config/routes/spree.rb
+++ b/config/routes/spree.rb
@@ -32,7 +32,9 @@
     put '/password/change' => 'user_passwords#update', :as => :update_password
   end
 
-  resource :account, :controller => 'users'
+  resource :account, :controller => 'users' do
+    resources :webhook_endpoints, only: [:create, :destroy], controller: '/webhook_endpoints'
+  end
 
   match '/admin/orders/bulk_management' => 'admin/orders#bulk_management', :as => "admin_bulk_order_management", via: :get
   match '/admin/payment_methods/show_provider_preferences' => 'admin/payment_methods#show_provider_preferences', :via => :get
diff --git a/spec/controllers/webhook_endpoints_controller_spec.rb b/spec/controllers/webhook_endpoints_controller_spec.rb
new file mode 100644
index 00000000000..c36720edb8f
--- /dev/null
+++ b/spec/controllers/webhook_endpoints_controller_spec.rb
@@ -0,0 +1,67 @@
+# frozen_string_literal: false
+
+require 'spec_helper'
+require 'open_food_network/order_cycle_permissions'
+
+describe WebhookEndpointsController, type: :controller do
+  let(:user) { create(:admin_user) }
+
+  before { allow(controller).to receive(:spree_current_user) { user } }
+
+  describe "#create" do
+    it "creates a webhook_endpoint" do
+      expect {
+        spree_post :create, { url: "https://url" }
+      }.to change {
+        user.webhook_endpoints.count
+      }.by(1)
+
+      expect(flash[:success]).to be_present
+      expect(flash[:error]).to be_blank
+      expect(user.webhook_endpoints.first.url).to eq "https://url"
+    end
+
+    it "shows error if parameters not specified" do
+      expect {
+        spree_post :create, { url: "" }
+      }.to_not change {
+        user.webhook_endpoints.count
+      }
+
+      expect(flash[:success]).to be_blank
+      expect(flash[:error]).to be_present
+    end
+
+    it "redirects back to referrer" do
+      spree_post :create, { url: "https://url" }
+
+      expect(response).to redirect_to "/account#/developer_settings"
+    end
+  end
+
+  describe "#destroy" do
+    let!(:webhook_endpoint) { user.webhook_endpoints.create(url: "https://url") }
+
+    it "destroys a webhook_endpoint" do
+      webhook_endpoint2 = user.webhook_endpoints.create!(url: "https://url2")
+
+      expect {
+        spree_delete :destroy, { id: webhook_endpoint.id }
+      }.to change {
+        user.webhook_endpoints.count
+      }.by(-1)
+
+      expect(flash[:success]).to be_present
+      expect(flash[:error]).to be_blank
+
+      expect{ webhook_endpoint.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      expect(webhook_endpoint2.reload).to be_present
+    end
+
+    it "redirects back to developer settings tab" do
+      spree_delete :destroy, id: webhook_endpoint.id
+
+      expect(response).to redirect_to "/account#/developer_settings"
+    end
+  end
+end
diff --git a/spec/system/consumer/account/developer_settings_spec.rb b/spec/system/consumer/account/developer_settings_spec.rb
index 4385a4aff4b..40bf4a59d63 100644
--- a/spec/system/consumer/account/developer_settings_spec.rb
+++ b/spec/system/consumer/account/developer_settings_spec.rb
@@ -35,6 +35,22 @@
           expect(page).to have_content "Key generated"
           expect(page).to have_input "api_key", with: user.reload.spree_api_key
         end
+
+        describe "Webhook Endpoints" do
+          it "creates a new webhook endpoint and deletes it" do
+            within "#webhook_endpoints" do
+              fill_in "webhook_endpoint_url", with: "https://url"
+
+              click_button I18n.t(:create)
+              expect(page.document).to have_content I18n.t('webhook_endpoints.create.success')
+              expect(page).to have_content "https://url"
+
+              click_button I18n.t(:delete)
+              expect(page.document).to have_content I18n.t('webhook_endpoints.destroy.success')
+              expect(page).to_not have_content "https://url"
+            end
+          end
+        end
       end
     end
 

From 9d5ca2255b86423aaac7eb7d45eda593e3522939 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 7 Mar 2023 09:22:45 +1100
Subject: [PATCH 12/13] Apply suggestions from code review

Co-authored-by: Maikel <maikel@email.org.au>
---
 spec/jobs/webhook_delivery_job_spec.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/spec/jobs/webhook_delivery_job_spec.rb b/spec/jobs/webhook_delivery_job_spec.rb
index 8f90c3c4e50..7add4ca4e1e 100644
--- a/spec/jobs/webhook_delivery_job_spec.rb
+++ b/spec/jobs/webhook_delivery_job_spec.rb
@@ -22,7 +22,7 @@
   end
 
   it "delivers a payload" do
-    Timecop.freeze(Time.zone.now) do
+    Timecop.freeze do
       expected_body = {
         id: /.+/,
         at: Time.zone.now.to_s,
@@ -48,9 +48,9 @@
       ]
 
       private_addresses.each do |url|
-        # Unfortunately this isn't allowed in CI, but it should work in your
-        # development environment.
-        xit "rejects private address #{url}" do
+        it "rejects private address #{url}" do
+          # Github Actions doesn't allow local connections.
+          pending if ENV["CI"]
           expect {
             WebhookDeliveryJob.perform_now(url, event, data)
           }.to raise_error(PrivateAddressCheck::PrivateConnectionAttemptedError)

From d59074dabd33c9e493efc96c30b70e3ed27b9e18 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 15 Mar 2023 11:55:08 +1100
Subject: [PATCH 13/13] Tidy up spec

The best way to check if something changed or not, is with 'change' of course.
---
 spec/models/order_cycle_spec.rb | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb
index fa9550039fb..fe80bd3d789 100644
--- a/spec/models/order_cycle_spec.rb
+++ b/spec/models/order_cycle_spec.rb
@@ -557,21 +557,18 @@
     }
 
     it "reset opened_at if open date change in future" do
-      expect(oc.opened_at).to_not be_nil
-      oc.update!(orders_open_at: 1.week.from_now, orders_close_at: 2.weeks.from_now)
-      expect(oc.opened_at).to be_nil
+      expect{ oc.update!(orders_open_at: 1.week.from_now, orders_close_at: 2.weeks.from_now) }
+        .to change { oc.opened_at }.to be_nil
     end
 
     it "it does not reset opened_at if open date is changed to be earlier" do
-      expect(oc.opened_at).to_not be_nil
-      oc.update!(orders_open_at: 3.days.ago)
-      expect(oc.opened_at).to_not be_nil
+      expect{ oc.update!(orders_open_at: 3.days.ago) }
+        .to_not change { oc.opened_at }
     end
 
     it "it does not reset opened_at if open date does not change" do
-      expect(oc.opened_at).to_not be_nil
-      oc.update!(orders_close_at: 1.day.from_now)
-      expect(oc.opened_at).to_not be_nil
+      expect{ oc.update!(orders_close_at: 1.day.from_now) }
+        .to_not change { oc.opened_at }
     end
   end