From a8490a9b11b6c65df0780f15d97224da993eb3e2 Mon Sep 17 00:00:00 2001 From: David Cook Date: Fri, 21 Feb 2025 12:16:45 +1100 Subject: [PATCH] Record the exact event time for the webhook There might be a delay before it gets sent, so it's better to record the time the event occurred at. It would have been simpler to just add it to the data hash, but I felt it was an important detail for an event and should be at the top level along with event name. In the case of order cycle opening, this is the same as opened_at. I've included this in the payload for clarity too. --- app/jobs/order_cycle_opened_job.rb | 2 +- app/jobs/webhook_delivery_job.rb | 4 ++-- app/services/order_cycles/webhook_service.rb | 6 +++--- spec/jobs/webhook_delivery_job_spec.rb | 5 +++-- .../services/order_cycles/webhook_service_spec.rb | 15 ++++++++++----- 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/app/jobs/order_cycle_opened_job.rb b/app/jobs/order_cycle_opened_job.rb index bcd0b1bcc4..352b823244 100644 --- a/app/jobs/order_cycle_opened_job.rb +++ b/app/jobs/order_cycle_opened_job.rb @@ -43,7 +43,7 @@ class OrderCycleOpenedJob < ApplicationJob order_cycle.update_columns(opened_at:, updated_at: opened_at) # And notify any subscribers - OrderCycles::WebhookService.create_webhook_job(order_cycle, 'order_cycle.opened') + OrderCycles::WebhookService.create_webhook_job(order_cycle, 'order_cycle.opened', opened_at) end end end diff --git a/app/jobs/webhook_delivery_job.rb b/app/jobs/webhook_delivery_job.rb index 1196e6a1d6..6004b0d6f1 100644 --- a/app/jobs/webhook_delivery_job.rb +++ b/app/jobs/webhook_delivery_job.rb @@ -13,11 +13,11 @@ class WebhookDeliveryJob < ApplicationJob queue_as :default - def perform(url, event, payload) + def perform(url, event, payload, at: Time.zone.now) body = { id: job_id, - at: Time.zone.now.to_s, event:, + at: at.to_s, data: payload, } diff --git a/app/services/order_cycles/webhook_service.rb b/app/services/order_cycles/webhook_service.rb index f12c94ea1b..b45c5c6e41 100644 --- a/app/services/order_cycles/webhook_service.rb +++ b/app/services/order_cycles/webhook_service.rb @@ -5,9 +5,9 @@ module OrderCycles class WebhookService - def self.create_webhook_job(order_cycle, event) + def self.create_webhook_job(order_cycle, event, at) webhook_payload = order_cycle - .slice(:id, :name, :orders_open_at, :orders_close_at, :coordinator_id) + .slice(:id, :name, :orders_open_at, :opened_at, :orders_close_at, :coordinator_id) .merge(coordinator_name: order_cycle.coordinator.name) # Endpoints for coordinator owner @@ -17,7 +17,7 @@ module OrderCycles webhook_endpoints |= order_cycle.distributors.map(&:owner).flat_map(&:webhook_endpoints) webhook_endpoints.each do |endpoint| - WebhookDeliveryJob.perform_later(endpoint.url, event, webhook_payload) + WebhookDeliveryJob.perform_later(endpoint.url, event, webhook_payload, at:) end end end diff --git a/spec/jobs/webhook_delivery_job_spec.rb b/spec/jobs/webhook_delivery_job_spec.rb index 0b09ed7bde..5383f5e87c 100644 --- a/spec/jobs/webhook_delivery_job_spec.rb +++ b/spec/jobs/webhook_delivery_job_spec.rb @@ -3,9 +3,10 @@ require 'spec_helper' RSpec.describe WebhookDeliveryJob do - subject { WebhookDeliveryJob.new(url, event, data) } + subject { WebhookDeliveryJob.new(url, event, data, at:) } let(:url) { 'https://test/endpoint' } let(:event) { 'order_cycle.opened' } + let(:at) { 1.second.ago } let(:data) { { order_cycle_id: 123, name: "Order cycle 1", open_at: 1.minute.ago.to_s, tags: ["tag1", "tag2"] @@ -25,7 +26,7 @@ RSpec.describe WebhookDeliveryJob do Timecop.freeze do expected_body = { id: /.+/, - at: Time.zone.now.to_s, + at: at.to_s, event:, data:, } diff --git a/spec/services/order_cycles/webhook_service_spec.rb b/spec/services/order_cycles/webhook_service_spec.rb index 3ab38cd86f..a006d1e68e 100644 --- a/spec/services/order_cycles/webhook_service_spec.rb +++ b/spec/services/order_cycles/webhook_service_spec.rb @@ -8,12 +8,14 @@ RSpec.describe OrderCycles::WebhookService do :simple_order_cycle, name: "Order cycle 1", orders_open_at: Time.zone.parse("2022-09-19 09:00:00"), + opened_at: Time.zone.parse("2022-09-19 09:00:01"), orders_close_at: Time.zone.parse("2022-09-19 17:00:00"), coordinator:, ) } let(:coordinator) { create :distributor_enterprise, name: "Starship Enterprise" } - subject { OrderCycles::WebhookService.create_webhook_job(order_cycle, "order_cycle.opened") } + let(:at) { Time.zone.parse("2022-09-19 09:00:02") } + subject { OrderCycles::WebhookService.create_webhook_job(order_cycle, "order_cycle.opened", at) } describe "creating payloads" do it "doesn't create webhook payload for enterprise users" do @@ -44,6 +46,7 @@ RSpec.describe OrderCycles::WebhookService do id: order_cycle.id, name: "Order cycle 1", orders_open_at: Time.zone.parse("2022-09-19 09:00:00"), + opened_at: Time.zone.parse("2022-09-19 09:00:01"), orders_close_at: Time.zone.parse("2022-09-19 17:00:00"), coordinator_id: coordinator.id, coordinator_name: "Starship Enterprise", @@ -51,7 +54,7 @@ RSpec.describe OrderCycles::WebhookService do expect{ subject } .to enqueue_job(WebhookDeliveryJob).exactly(1).times - .with("http://coordinator_owner_url", "order_cycle.opened", hash_including(data)) + .with("http://coordinator_owner_url", "order_cycle.opened", hash_including(data), at:) end end @@ -87,9 +90,11 @@ RSpec.describe OrderCycles::WebhookService do expect{ subject } .to enqueue_job(WebhookDeliveryJob).with("http://distributor1_owner_url", - "order_cycle.opened", hash_including(data)) + "order_cycle.opened", hash_including(data), + at:) .and enqueue_job(WebhookDeliveryJob).with("http://distributor2_owner_url", - "order_cycle.opened", hash_including(data)) + "order_cycle.opened", hash_including(data), + at:) end end @@ -137,7 +142,7 @@ RSpec.describe OrderCycles::WebhookService do context "without webhook subscribed to enterprise" do it "doesn't create webhook payload" do - expect{ OrderCycles::WebhookService.create_webhook_job(order_cycle, "order_cycle.opened") } + expect{ subject } .not_to enqueue_job(WebhookDeliveryJob) end end